review(site): I'm not super happy with how this turned out. I kind of wanted to represent this in the updateCheckXService, but I couldn't figure out how to do inter-machine dependencies.

This snippet here enables e.g. the following scenario:

1. User is loggedin as non-owner
2. User logs out
3. User logs in as owner
4. Update notification is shown

Yeah, inter-machine dependencies are tricky; I'm not sure if there's a better way.@presleyp do you have any workarounds? I think you looked at something like this recently with pagination.

review(site): Dismissal as any user (practically only an owner) will prevent notification from showing up until browser window is reloaded, even if logged in/out in-between. This seems like acceptable behavior (managed via XState service).

review(api): Technically, we restrict access in the WebUI so that only owners can view this information. It's not sensitive information by any means, but it's somewhat pointless to show to users. That's why we do not require authorization here (but I can change it if this behavior seems too weird).

Unless the "updatecheck" can trigger a real update and potentially crash the deployment, I guess that it's safe to expose it to everyone. On the other hand, if there is a known security issue, all users would be informed that there is an update to install, so the system is currently vulnerable.

The updatecheck is read-only, so it won't do anything scandalous. If we ever add a POST endpoint, that should obviously be protected. You raised a good point about vulnerabilities. Since Coder is an open source product, this information would be available to anyone checking the GitHub releases too. More than protecting this endpoint, we should consider how we convey this information through our releases.

Instead of all replicas doing the update check, could you lock the config table row so only one replica will attempt to do an update check at a period of ~24 hours?

Do you have a suggestion for the how to do the lock or do we have a previous implementation of it? I'm not sure how to implement it withsqlc.

Essentially I'm imagining, start transaction => lock table in row exclusive mode => select row => (if it's time to update, fetch => update) => commit. But alas, sqlc.. 😄

Postgres advisory locks are very easy to use, you just select the lock and it'll automatically get released when either the transaction ends or the connection ends.

SELECT pg_advisory_lock("some_name");-- returns an error if already locked (I think)-- orSELECT pg_try_advisory_lock("some_name");-- returns true if successfully lockedSELECT pg_advisory_unlock("some_name");

Here's an example from v1:https://github.com/coder/v1/blob/7259332777bae82e5bef3fa2afacb63f5606306d/product/coder/pkg/dlock/pglock/pglock.go

usepg_advisory_xact_lock for transactions for it to be auto-released if the transaction ends***

I haven't used those before. Good suggestion, it looks promising but I don't see us having any precedent for using these. I guess there's always a first time 😄.

They work well for our use in v1.

Instead of all replicas doing the update check, could you lock the config table row so only one replica will attempt to do an update check at a period of ~24 hours?

I might be blind here, but what exactly are the penalties? It doesn't seem to be frequent db access.

Seems like a waste to do one update check for each replica if only one result will ever be stored and it's very easy to wrap in a transaction with a lock

I'll defer this to a follow-up PR, this is a convenience fix more than anything. Ultimately I'd like for us to have a plan for how to deal with replicas, we can obviously have it be the wild west where the quickest draw wins, but assigning/limiting responsibilities via primary/secondary seems logical IMO.

I think we should avoid doing an update check immediately on startup, this could be delayed by 10 minutes or something to avoid:

• every replica doing database queries immediately on startup to try to do an update check,
• any potential panics in this code causing coder to instantly crash on start

We only do the update check if it's been >24h since the last one. And unless all replicas start up at the same time, this shouldn't be a problem, especially if we add the table locking.

Regarding the panics, ultimately we mustn't have any, but if we did, I would say it's better the sooner it happens. Wouldn't want it to happen while Coder is in the middle of provisioning workspaces, etc.

every replica doing database queries immediately on startup to try to do an update check,

And unless all replicas start up at the same time, this shouldn't be a problem

It's a common pattern to define a startup/access delay for workers that hit the same resource, usually 0-60 sec. Maybe it can help also here?

I'll think about this for a follow up PR, I don't imagine a single database query will be problematic at startup, though. If we want to spread out service starts, it would be good to have a more structured plan than doing it individually and uniquely for each service.

every replica doing database queries immediately on startup to try to do an update check,

And unless all replicas start up at the same time, this shouldn't be a problem

It's a common pattern to define a startup/access delay for workers that hit the same resource, usually 0-60 sec. Maybe it can help also here?

Instead of all replicas doing the update check, could you lock the config table row so only one replica will attempt to do an update check at a period of ~24 hours?

I might be blind here, but what exactly are the penalties? It doesn't seem to be frequent db access.

Unless the "updatecheck" can trigger a real update and potentially crash the deployment, I guess that it's safe to expose it to everyone. On the other hand, if there is a known security issue, all users would be informed that there is an update to install, so the system is currently vulnerable.

Just curious why we throw an error if the check fails. Perhaps it would be better if this failed silently (we could throw something in the terminal if we wanted)? Maybe not, just a thought after seeing the error on local:

I thought about it and I worried we wouldn't notice if it stops working for whatever reason. I can make it silent or add a prefix (Update check failed: Route not found.). Ideally we would keep it silent and log it to Sentry or a similar bug tracker.

Yeah, maybe we could just add a console.error since we don't have Sentry.

I refactored this now, it looks like this:https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=6388bfa22e0a55f2ff0c1f1e

I'm still game to remove it if that's preferred. 👍🏻

@mafredri nope looks good!

nice

I'm not sure why we need to export these constants or why we need to cast asconst although I see this pattern in our other machines.@presley (or@mafredri) is there something dumb I'm missing?

Same here, I thought it was weird but I kept them just in case. I'll remove them and see if something breaks 👍🏻