GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix(coderd/database): lock the workspaces row when claiming a prebuild by dylanhuff-at-coder · Pull Request #31518 · coder/coder

Conversation

dylanhuff-at-coder commented Oct 29, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

ClaimPrebuiltWorkspace locked the workspace_prebuilds view with FOR UPDATE ... SKIP LOCKED, but the view is built from CTEs, so nothing got locked. Concurrent claims picked the same prebuild and the losers got a 409 (a customer saw 28 of 30 fail).

This locks the workspaces row directly (FOR NO KEY UPDATE SKIP LOCKED), rechecks owner and deleted on it, and checks eligibility with per-workspace index lookups instead of the views. It uses = instead of IN so one claim can't take two prebuilds, filters on the preset's template so other templates' prebuilds aren't scanned, and claims oldest first so prebuilds are used before they hit the preset TTL. Query-only, no migration, applies cleanly to release/2.38. Builds on #31468 by Kevin Ha (Canva).

Benchmarks (Postgres 17, 20k users, 320k builds, 600 ready prebuilds): ~12 ms per claim (was ~640 ms), and 500 concurrent claims got distinct prebuilds in ~1.5 s.

Known limitations:

Each claim still checks every prebuild of the preset's template (other presets included) before sorting. That's a few ms at hundreds of prebuilds per template.
Pre-existing: a reconciler delete during a claim can still 409. Follow-up is a deleting_at marker the claim skips.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

github-actions Bot assigned dylanhuff-at-coder

Oct 29, 2026

dylanhuff-at-coder force-pushed the dylan/prebuild-claim-rewrite branch from 6e095b7 to fb5461f Compare

October 29, 2026 19:37


          fix(coderd/database): lock the workspaces row when claiming a prebuild

84f50e6

ClaimPrebuiltWorkspace locked the workspace_prebuilds view with
FOR UPDATE ... SKIP LOCKED. The view is built from CTEs, so the lock
took no row locks and concurrent claims picked the same prebuild. The
losing request failed with 409 "A workspace build is already active."

Select the candidate from the workspaces table directly and lock that
row with FOR NO KEY UPDATE SKIP LOCKED. Ownership and deletion are
checked on the locked row so a prebuild claimed by a just-committed
transaction is skipped. The eligibility checks use per-workspace index
lookups instead of the views, so skipping a locked row no longer
repeats deployment-wide work. Compare with = so the locking subquery
runs once, and claim the newest prebuild first because the reconciler
deletes the oldest first.

Co-authored-by: Kevin Ha <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>

dylanhuff-at-coder force-pushed the dylan/prebuild-claim-rewrite branch from fb5461f to 84f50e6 Compare

October 29, 2026 19:51

dylanhuff-at-coder changed the title ~~fix(coderd/database): claim prebuilds without scanning the prebuild views~~ fix(coderd/database): lock the workspaces row when claiming a prebuild

Oct 29, 2026

dylanhuff-at-coder marked this pull request as ready for review

October 29, 2026 19:53

dylanhuff-at-coder requested a review from Emyrk

October 29, 2026 19:56

BobbyHo commented Oct 29, 2026

Copy link

Copy Markdown

Contributor

/coder-agents-review Please limit the review to P0, P1, and P2 issues only.

coder-agents-review[bot] reacted with rocket emoji

All reactions

🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coder-agents-review Bot commented Oct 29, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Chat: Review posted | View chat
Requested: 2026-10-29 20:11 UTC by @BobbyHo

Review history

R1 (2026-10-29): 13 reviewers, 3 Nit, 1 P2, 8 P3, 4 P4, COMMENT. Review

deep-review v0.13.0 | Round 1 | 1acacc9..84f50e6

Last posted: Round 1, 16 findings (1 P2, 8 P3, 4 P4, 3 Nit), COMMENT. Review

Finding inventory

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	P3	Open	prebuilds.sql:61	No eligibility case for a latest build whose only agents are deleted; EXISTS `NOT wa.deleted` filter is untested	R1	Netero	Yes
CRF-2	Note	Note (body)	querier_test.go:8096	RechecksLockedRow passes the seven query params positionally, coupled to generated order	R1	Netero	No
CRF-3	P2	Open	prebuilds.sql:37	Claim scans every prebuild across all templates; plan cost crosses jit_above_cost; template_id narrowing uses an existing index	R1	Killua P2, Knuckle P3, Hisoka P3, Mafuuu P4	Yes
CRF-4	P3	Open	prebuilds.sql:77	NO KEY UPDATE does not conflict with the reconciler delete build's KEY SHARE, so the delete-race 409 remains; FOR UPDATE tradeoff is a deadlock	R1	Pariston P3, Mafuuu Note	Yes
CRF-5	P3	Open	prebuilds.sql:77	No behavioral test pins the lock mode; only a query-text check fails	R1	Bisky	Yes
CRF-6	P3	Open	prebuilds.sql:51	No eligibility case for a latest build with NULL preset; `IS NOT NULL` filter is untested	R1	Bisky P3, Hisoka P4	Yes
CRF-7	P3	Open	prebuilds.sql:24	Claim and test oracle pinned to workspace_prebuilds (no other readers); reconciler readiness ignores sub-agents	R1	Ryosuke	Yes
CRF-8	P4	Open	prebuilds.sql:23	Comment says each SKIP LOCKED skip redoes the views' work; EXPLAIN shows it does not	R1	Ryosuke	Yes
CRF-9	P3	Open	prebuilds.sql:72	Newest-first claiming lets older TTL-preset prebuilds expire and get rebuilt	R1	Hisoka	Yes
CRF-10	P3	Open	prebuilds.sql:71	"avoids racing it" overstates newest-first ordering; the race remains	R1	Gon P3, Leorio P4	Yes
CRF-11	P3	Open	prebuilds.sql:55	"Ready" comment omits the non-deleted agent condition	R1	Gon P3, Leorio P4	Yes
CRF-12	Nit	Open	prebuilds.sql:73	Lock comment's first sentence explains the previous query	R1	Gon P2, Zoro Nit	Yes
CRF-13	P4	Open	querier_test.go:8178	Test comment "when it read the view" refers to the pre-PR claim	R1	Gon P2	Yes
CRF-14	P2	Dropped by orchestrator (comment states the invariant under test, "while claim A still holds its lock", which the helper name does not)	querier_test.go:8027	Comment restates helper name	R1	Gon	No
CRF-15	P2	Dropped by orchestrator (comment explains why the pg_stat_activity poll exists)	querier_test.go:8103	Comment narrates next statements	R1	Gon	No
CRF-16	Nit	Open	querier_test.go:7906	`succeeded = false` creates a pending job, not a failed one	R1	Gon	Yes
CRF-17	Nit	Open	querier_test.go:7880	`claimTestPreset` is a fixture, not a preset	R1	Gon	Yes
CRF-18	Note	Dropped by orchestrator (restates CRF-8, CRF-10 to CRF-13)	n/a	Comment accuracy is a pattern in this PR	R1	Gon	No
CRF-19	P4	Open	prebuilds.sql:56	EXISTS/NOT EXISTS pair can be one bool_and subquery matching the view	R1	Zoro	Yes
CRF-20	P4	Open	querier_test.go:8000	Use sync.OnceFunc instead of sync.Once plus closure	R1	Ging-Go	Yes
CRF-21	P3	Dropped by orchestrator (merged: scan-cost limitation into CRF-3, reconciler-delete limitation into out-of-scope list with human-decision line)	PR description	Known-limitation follow-ups have no linked issues	R1	Leorio P3, Pariston Note	No
CRF-22	Note	Note (body)	prebuilds.sql:77	SKIP LOCKED also skips prebuilds locked by non-claim writers	R1	Takumi	No
CRF-23	Note	Note (body)	prebuilds.sql:77	Lock recheck relies on READ COMMITTED; REPEATABLE READ caller would get 40001	R1	Knuckle	No
CRF-24	Note	Note (body)	querier_test.go:8059	ClaimsOne catches IN revert only while the planner reruns the subquery	R1	Bisky	No
CRF-25	Note	Dropped by orchestrator (confirms correctness; no action)	prebuilds.sql:77	Locking plan holds; per-workspace lookups use indexes	R1	Killua	No
CRF-26	Note	Dropped by orchestrator (confirms correctness; no action)	prebuilds.sql:20-28	SQL comments checked and hold	R1	Leorio	No
CRF-27	Note	Dropped by orchestrator (confirms test works; no action)	querier_test.go:8059	ClaimsOne catches the IN regression	R1	Hisoka	No
CRF-28	Note	Dropped by orchestrator (merged into CRF-7)	prebuilds.sql:24	Third copy of ready-prebuild logic; test pins only the view	R1	Zoro	No
CRF-29	Note	Dropped by orchestrator (reviewer judged justified; no action)	querier_test.go:7925	Second prebuild fixture builder next to createPrebuiltWorkspace	R1	Zoro	No
CRF-30	OOS	Out of scope (body)	reconcile.go:955	provisionDelete reads owner unlocked; reconciler delete races claim, claim 409s	R1	Knuckle, Takumi, Pariston, Mafuuu	No
CRF-31	OOS	Out of scope (body)	prebuilds.sql:35	EPQ recheck keeps snapshot values for build/preset/agent joins; post-snapshot delete build unseen	R1	Komugi	No
CRF-32	OOS	Out of scope (body)	prebuilds.sql:16	Claim ignores preset TTL and last_invalidated_at	R1	Hisoka	No
CRF-33	OOS	Out of scope (body)	dbtestutil/db.go:172	Test DBs run with jit = off; JIT plan-cost regressions undetectable	R1	Killua	No

Contested and acknowledged

Law analysis

Not run. Effective additions 403 (threshold 1000).

Round log

Round 1

Full panel (round 1). Netero first pass: 1 P3, 1 Note, no P0, so panel ran. Panel: Knuckle, Ging-Go, Takumi, Killua, Ryosuke (Go + SQL specialists), Pariston, Mafuuu (core), Bisky (tests), Gon, Leorio (once-per-PR floor), Komugi, Hisoka (pool), Zoro (wildcard). Multi-domain (Go and SQL, concurrency): 13 panel reviewers, size target 9-12 exceeded by 1 because Gon and Leorio default to round 1 alongside the five triggered specialists. Contradiction: Pariston recommends FOR UPDATE; Takumi and Mafuuu argue NO KEY UPDATE avoids a claim/reconciler deadlock; Bisky asks for a behavioral test of NO KEY UPDATE. Posted as CRF-4 (tradeoff) and CRF-5 (test for whichever mode is kept). Gon P2 comment findings downgraded: CRF-12 to Nit (keep-argument: stale rationale in concurrency-critical SQL misleads; drop reason: sentence is true and explains why the views are not locked), CRF-13 to P4 (keep-argument: reader cannot resolve the reference; drop reason: test-only comment, no behavior effect). Result: 1 P2, 8 P3, 4 P4, 3 Nit posted. Reviewed against 1acacc9..84f50e6.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Komugi	flake/determinism
Kurapika	security
Law	decomposition
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

🤖 Managed by Coder Agents.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BobbyHo approved these changes

Oct 29, 2026

View reviewed changes

BobbyHo left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code changes LGTM! Just curious if you've considered how TTL interacts with newest-first ordering. If demand stays below the pool size, older prebuilds might never get picked and could end up expiring and being rebuilt unnecessarily. Oldest-first could help avoid this churn, though I understand there's an existing reconciler race.

Just something that came to mind, but not a blocker. I think the changes in this PR look good overall! 🙂

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dylanhuff-at-coder reacted with rocket emoji

All reactions

🚀 1 reaction

coder-agents-review Bot reviewed

Oct 29, 2026

View reviewed changes

coder-agents-review Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR rewrites ClaimPrebuiltWorkspace to lock the workspaces row with SKIP LOCKED, check eligibility per workspace, and claim newest first. Findings: 1 P2, 8 P3, 4 P4, 3 Nit.

The two comments on prebuilds.sql:77 are linked: one weighs FOR UPDATE against FOR NO KEY UPDATE, and the other asks for a behavioral test of the mode you keep.

Out of scope (needs a ticket or explicit acceptance by a human):

enterprise/coderd/prebuilds/reconcile.go:955: provisionDelete reads the workspace owner without locking the row, so a reconciler delete build can race a claim, and the claim returns 409 when it inserts its build second. The PR description lists this as a follow-up with no linked issue.
coderd/database/queries/prebuilds.sql:35: when Postgres rechecks a locked pw row, the latest-build join and the preset and agent subqueries keep their statement-snapshot values, so a delete build committed after the snapshot is not seen and the claim hands out a workspace that is being deleted.
coderd/database/queries/prebuilds.sql:16: the claim does not check the preset TTL or last_invalidated_at, so it can take a prebuild the reconciler has classified as expired and is deleting.
coderd/database/dbtestutil/db.go:172: test databases run with jit = off, so no test or benchmark can detect a query whose plan cost crosses jit_above_cost in production.

Notes:

SKIP LOCKED also skips prebuilds locked by other writers such as UpdateWorkspacesTTLByTemplateID; a claim during such a statement gets sql.ErrNoRows and the user gets a new workspace build instead.
The recheck of a locked row relies on READ COMMITTED (coderd/workspaces.go:712 calls InTx(..., nil)); a caller that wraps the claim in REPEATABLE READ would get serialization errors instead of skips.
TestClaimPrebuiltWorkspaceClaimsOne catches a revert to IN only while the planner reruns the subquery per outer row, which it did on Postgres 13.
TestClaimPrebuiltWorkspaceRechecksLockedRow passes the seven query parameters positionally in the generated order (querier_test.go:8096).

🤖 This review was automatically generated with Coder Agents.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dylanhuff-at-coder reacted with thumbs up emoji

All reactions

👍 1 reaction

f0ssel approved these changes

Oct 29, 2026

View reviewed changes


          fix(coderd/database): claim the oldest prebuild and filter by the pre…

f509c1b

…set's template

Claim prebuilds oldest first so they are used before they reach the
preset TTL. Filter candidates by the preset's template so Postgres can use
workspace_template_id_idx and skip other templates' prebuilds.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

dylanhuff-at-coder commented Oct 29, 2026

Copy link

Copy Markdown

Contributor Author

Overall, the code changes LGTM! Just curious if you've considered how TTL interacts with newest-first ordering. If demand stays below the pool size, older prebuilds might never get picked and could end up expiring and being rebuilt unnecessarily. Oldest-first could help avoid this churn, though I understand there's an existing reconciler race.

Just something that came to mind, but not a blocker. I think the changes in this PR look good overall! 🙂

Good call, switched to oldest-first in f509c1b. Newest-first was meant to dodge the reconciler delete race, but with claims down to ~12ms that window is tiny, and the TTL churn you described would be a constant cost on any preset with a TTL. Thanks for flagging it!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BobbyHo approved these changes

Oct 29, 2026

View reviewed changes

BobbyHo left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Thanks for addressing my comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dylanhuff-at-coder reacted with thumbs up emoji

All reactions

👍 1 reaction

Emyrk reviewed

Oct 29, 2026

View reviewed changes

Emyrk left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments from Steven, drafted with Coder Agents. The concurrent-claim lock and oldest-first selection look good; the three inline comments cover the reconciler race, agent nesting, and regression coverage.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

coderd/database/queries/prebuilds.sql

		-- NO KEY UPDATE matches the outer UPDATE and, unlike FOR UPDATE, does not block FK checks
		-- from inserts that reference the workspace, such as a reconciler delete build.
		LIMIT 1
		FOR NO KEY UPDATE OF pw SKIP LOCKED -- Ensure that a concurrent request will not select the same prebuild.

Emyrk Oct 29, 2026

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is from an agent (Coder Agents generated). The claim-versus-reconciler-delete 409 remains possible with this lock mode, as noted in the description. We should investigate alternatives that coordinate claims and deletion, such as implementing a queue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dylanhuff-at-coder Oct 29, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this lock mode doesn’t close the claim vs delete race. It’s pre-existing and only ever a 409. I’d like to do the proper coordination as a follow-up rather than in the patch

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Emyrk reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

coderd/database/queries/prebuilds.sql Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

coderd/database/querier_test.go Resolved

Uh oh!

There was an error while loading. Please reload this page.

Emyrk approved these changes

Oct 29, 2026

View reviewed changes

Emyrk left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am approving the overall idea. I did not look at the new query.

TIL locking a view is a no-op? And postgres just allows that with no error? Feels like an easy mistake to make.

I think we should take step back and re-evaluate the design once we patch this immediate bug. This is my first time looking at this code, but the discussion with an agent suggestions that this code might be kind of fragile?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dylanhuff-at-coder reacted with thumbs up emoji

All reactions

👍 1 reaction


          test(coderd/database): cover deleted agents and preset-less builds in…

5dc7541

… claim eligibility

Add eligibility cases for a build whose only agent is deleted and for a
latest build without a preset after an earlier build had one. Drop the
locked-row recheck test and its test-only query export; the concurrent
and eligibility tests already fail without the owner_id predicate.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

dylanhuff-at-coder mentioned this pull request

Oct 29, 2026

fix(coderd/database): lock the workspaces row when claiming a prebuild (backport 2.38) #31556

Closed

dylanhuff-at-coder merged commit 5557383 into main

Oct 30, 2026

27 checks passed

Uh oh!

There was an error while loading. Please reload this page.

dylanhuff-at-coder deleted the dylan/prebuild-claim-rewrite branch

October 30, 2026 15:43

github-actions Bot locked and limited conversation to collaborators

Oct 30, 2026

Reviewers

Emyrk

Emyrk approved these changes

f0ssel

f0ssel approved these changes

BobbyHo

BobbyHo approved these changes

+1 more reviewer

coder-agents-review[bot]

coder-agents-review[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

dylanhuff-at-coder

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

dylanhuff-at-coder commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BobbyHo commented Oct 29, 2026

Uh oh!

coder-agents-review Bot commented Oct 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Finding inventory

Findings

Contested and acknowledged

Law analysis

Round log

Round 1

Uh oh!

BobbyHo left a comment

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylanhuff-at-coder commented Oct 29, 2026

Uh oh!

BobbyHo left a comment

Choose a reason for hiding this comment

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Emyrk Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

dylanhuff-at-coder Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dylanhuff-at-coder commented Oct 29, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

coder-agents-review Bot commented Oct 29, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.