fix(coderd): mark sub agent deletion via boolean instead of delete #18411
Conversation
a473f80 to
db5700a
Compare
db5700a to
7d73585
Compare
|
|
||
| // Add a test antagonist. For every agent we add a deleted sub agent | ||
| // to discover cases where deletion should be handled. | ||
| subAgt, err := db.InsertWorkspaceAgent(genCtx, database.InsertWorkspaceAgentParams{ |
There was a problem hiding this comment.
Review: This may be a bit unusual, but it already helped me catch an issue with the workspace_prebuilds VIEW.
Ideally this potential cause would somehow be propagated to the user in case of test failure, but not sure how that could be done. I'll add a log entry to help with discovery.
There was a problem hiding this comment.
I like this a lot! I do agree that it could get confusing to troubleshoot, but the log message in combination with dbtestutil.DumpOnFailure should help. You could also potentially set some relevant fields of the sub-agent so that its purpose is clear.
There was a problem hiding this comment.
How about this? 😆 bc370a8 (#18411)
|
|
||
| // Add a test antagonist. For every agent we add a deleted sub agent | ||
| // to discover cases where deletion should be handled. | ||
| subAgt, err := db.InsertWorkspaceAgent(genCtx, database.InsertWorkspaceAgentParams{ |
There was a problem hiding this comment.
I like this a lot! I do agree that it could get confusing to troubleshoot, but the log message in combination with dbtestutil.DumpOnFailure should help. You could also potentially set some relevant fields of the sub-agent so that its purpose is clear.
| UPDATE | ||
| workspace_agents | ||
| SET | ||
| deleted = TRUE | ||
| WHERE | ||
| id = $1 | ||
| AND parent_id IS NOT NULL | ||
| AND deleted = FALSE; |
There was a problem hiding this comment.
👍 literally impossible to delete a 'top-level' agent.
There was a problem hiding this comment.
obligatory reminder to check migration number!
Since deletion of data is uncommon in our database, the introduction of sub agents and their deletion introduced issues with foreign key assumptions in coder/internal#685. We could have only addressed the specific case by allowing cascade deletion of stats as well as handling in the stats collector, but it's unclear how many more such edge-cases we would've run into.
For this reason, we decided to mark the rows as deleted via boolean instead, and filter them out in all queries that seem relevant so that coderd doesn't see the deleted agents.
Testing is covered by updating the relevant
agentapitests as well as adding an antagonist deleted agent todbgenwhich automatically gets applied to all tests.Fixes coder/internal#685
Note that there are some queries that read
workspace_agentsthat were not updated with deletion handling, but each one was analyzed and deemed unnecessary because of how they are used, doing so may introduce more harm than good. On that note, many of these queries can return results for non-latest builds which essentially means, "deleted" agents.