Should this case also verify that the state isn't actually disconnected either via timeout or disonnected_at?

I extended the condition.

Frankly speaking,, I'm thinking about refactoring that part of the code and creating an extra "runtime" column for agent status. I'm not quite sure if it's possible to use SQL clauses to reflect this logic. I really wouldn't like to create functions...

I guess that you can take a look at the CASE logic implementation one more time as I've rewritten it to look similar to the Go code.

@mtojek I think the case logic is exactly what you want. But you would have to add that case logic to all reads. So either remove all GetWorkspaceAgentsByXXX and make 1 with a filter (like workspaces/template/etc). Or you could also make a view. I looked into this before with sqlc in an issue here:

#2201

Hey @Emyrk, thanks for jumping in!

I had a chat about this issue with @mafredri to try WITH expression first. I guess that I can transform it into a VIEW if there are performance/clean-sql concerns. Otherwise, I am happy to do this in a follow-up.

The only reason I mentioned a view is because sqlc creates our model types from the sql.

So you have GetWorkspaceAgentByAuthToken, GetWorkspaceAgentByID, GetWorkspaceAgentByInstanceID, GetWorkspaceAgentsByResourceIDs, GetWorkspaceAgentsCreatedAfter. If you update 1 to select this extra "status" column dynamically, sqlc creates a new model type for this. It will not be type WorkspaceAgent. I think it has the name of the function, eg GetWorkspaceAgentByXXX (iirc?). So you will need to add this new column to all the calls, and I think each call gets a unique model type. A way around this is to consolidate all these calls too.

A "view" is a way to repackage the extra dynamic column in a way sqlc still sees it as a single type. So all the above queries go from ... FROM workspace_agents ... to ... FROM workspace_agents_view .... The sqlc type will be type WorkspaceAgentsView.

So tl;dr the view is just a way to keep the sqlc clean. You'll see when you run a make gen with a dynamic column (SELECT *, "unknown" as status FROM workspace_agents).

So either remove all GetWorkspaceAgentsByXXX and make 1 with a filter (like workspaces/template/etc)

Fortunately, at moment we only need it to search workspaces using filtering. I guess that we don't need to hurry up and inject the case logic everywhere.

The only reason I mentioned a view is because sqlc creates our model types from the sql.

In terms of clean code and the benefits of sqlc, I totally agree with you. I admit that I wouldn't like to get stuck in this pull request with too many changes and I'm wondering if I can split it into multiple PRs.

Follow-up ideas:

1. Replace the WITH expression with a view. Adjust the rest of the codebase to use it.
2. Make agent status dynamically set in SQL. Remove the agent status logic from Go code - we don't need it in two places, except for databasefake.

Frankly speaking, if there aren't big performance or clean code concerns around this PR, I would leave it as is, and focus on improvements once the requested feature is delivered.

Let me know what you think.

@Emyrk @mafredri After further research I noticed a possible blocker for a view. The CASE logic
requires agent_inactive_disconnect_timeout_seconds, which is a coderd option, and it's currently provided via query.

Unless we can change it to a static value, then I suppose we can't use views here.

That's a good point @mtojek, it definitely hampers the use case. Ideally this value would come from either the terraform provider or be stored in the database by some other means, at which point it can be part of the query.

I don't find it unthinkable that we would serialize certain coder server runtime properties in the database, either as a temp table or one that is (re)written on startup. The inactivity seconds could then be stored there and joined.