feat(coderd): process on_success workspace build orchestration - #25759
Conversation
58700d9 to
3edff4e
Compare
096cb57 to
998848a
Compare
3edff4e to
21fa1f4
Compare
998848a to
cfa75f9
Compare
21fa1f4 to
dca2e69
Compare
cfa75f9 to
06af5be
Compare
dca2e69 to
2e97f53
Compare
06af5be to
0ddfc81
Compare
2e97f53 to
fa1fd82
Compare
0ddfc81 to
a1b168a
Compare
Add the background worker that claims pending workspace build orchestration rows, resolves terminal parent build outcomes, and creates child builds after successful parents. Wire the worker into coderd startup and pubsub wake-ups from workspace build completion paths. Add retry limiting, delayed retries, and a backup poll so missed wake signals do not leave ready rows pending indefinitely. Add coverage for happy-path restart, parent failed/canceled outcomes, child build failure handling, retry delays, and pubsub wake behavior. Ref: https://linear.app/codercom/issue/PLAT-143/add-workspace-restart-functionality-to-api Ref: #5800
a1b168a to
5c38677
Compare
|
/coder-agents-review |
|
Chat: Review in progress (6/6 reviewers complete) | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 18 findings (4 P2, 10 P3, 4 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-5 (P3, workspacebuilds_on_success_test.go:676) - No test for retry exhaustion at max attempts
CRF-11 (P3, workspacebuildorchestrator.go:62) - Subscribe-with-retry duplicates acquirer.go
CRF-12 (P3, workspacebuildorchestrator.go:441) - BuildError classification diverges from prebuilds reconciler
Law analysisEffective LOC: 1095 (+504 prod, +591 test). Head SHA: 5c38677. Verdict: Don't split. Enforcement: Advisory. Round logRound 1Panel (17 reviewers). 3 P2, 9 P3, 4 Nit. Reviewed against 583b339..5c38677. Round 2Churn guard: 13 addressed, 2 contested, 1 acknowledged (human @Emyrk). Override BLOCKED->PROCEED (CRF-11 has explicit human decision). Panel (11 reviewers). Contested CRF-5 closed (6/6), CRF-12 closed (5/5). 1 P2, 1 P3 new. Reviewed against 583b339..69c59d1. Round 3Churn guard: 2 addressed, 0 contested, 0 silent. PROCEED. Panel (4 reviewers). 0 new findings. All 18 findings resolved. APPROVE. Reviewed against 583b339..b127602. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Well-structured background worker that mirrors established codebase patterns (lifecycle executor, provisioner acquirer). The problem-solution alignment is tight: orchestration rows need a consumer, this is the consumer. Test density is strong at 54%, covering happy paths, parent failures, cancellations, retryable vs non-retryable child errors, and cross-workspace isolation. Error classification in childBuildErrorShouldFailOrchestration is well-calibrated, and the post-transaction race handling with sql.ErrNoRows is correct and documented.
As one reviewer put it: "This is how you document a security-sensitive bypass: name the prior authorization, name the constraint, name what is still present. Every authorization bypass in this codebase should read like this."
Severity count: 3 P2, 9 P3, 4 Nit.
The three P2s are: (1) a missing workspace.Deleted guard that lets the orchestrator start builds on soft-deleted workspaces, (2) head-of-line blocking when a non-child-build transaction error permanently fails for one row, and (3) TestPostWorkspaceBuildsOnSuccessParentCanceled exercising the ProvisionerJobStatusFailed branch instead of the ProvisionerJobStatusCanceled branch it claims to test.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. All 13 R1 fixes verified by multiple reviewers: deleted-workspace guard, head-of-line blocking fix, cancel test, wake signal decoupling, goroutine shutdown join, nil auth callback, log improvements, quartz clock, naming, errors.AsType, and comment cleanup. The package extraction into coderd/wsbuildorchestrator/ is clean and well-motivated.
Contested findings resolved by panel vote: CRF-5 closed (6/6, querier test covers the SQL boundary), CRF-12 closed (5/5, divergence from prebuilds is intentional).
Severity count for new findings: 1 P2, 1 P3.
As one reviewer noted: "The error handling architecture is well-considered. Carrying childBuildErr out of the transaction and making the fail-vs-retry decision outside the tx is a clean separation."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3. All 18 findings from R1-R2 are resolved: 15 fixed (with tests), 2 closed by panel vote (6/6 and 5/5), 1 acknowledged by human reviewer. The R2 fixes (dormancy guard, preset test assertion) are clean and verified. The markFailed dry-refactor reduced ~60 lines of repeated boilerplate.
Across three rounds, 32 reviewers examined this PR. The dormant-workspace and deleted-workspace guards, the head-of-line blocking fix, the goroutine shutdown join, the nil auth callback, and the wake signal decoupling all addressed real issues. The test suite (59% density, 933 lines) covers happy paths, parent failures, cancellations, deleted/dormant workspaces, retryable/non-retryable child errors, and concurrent worker behavior.
No new findings from the R3 panel (Bisky, Mafuuu, Hisoka, Kite).
🤖 This review was automatically generated with Coder Agents.
59461aa
into
george/plat-143/2-on-success-workspace-build-request-handling
This PR is part of a stack that adds durable server-side workspace restart support
to the API.
Add the coderd background worker that processes persisted
on_successworkspacebuild orchestration rows. The worker claims ready rows, resolves terminal parent
build outcomes, and creates the child build when the parent completes
successfully.
This wires the worker into coderd startup and wake-ups from workspace build
completion paths. It also adds retry limiting, delayed retries, and a backup poll
so missed wake signals or transient child build failures do not leave
orchestration rows pending indefinitely.
Ref: https://linear.app/codercom/issue/PLAT-143/add-workspace-restart-functionality-to-api
Ref: #5800
This PR is part of a stack that merges into
main:Created with stakk