Fix lost-update race in workbench settings participant mutations - #507
Merged
Merged
Conversation
store.ts's updateWorkbenchSettings issues a blind UPDATE ... SET settings = $1 with no version guard and no merge — a whole-blob replace. Every participant add/remove path was check-then-act off a snapshot read earlier in the same request (joinHumanParticipant, removeWorkbenchParticipant, launchAndJoinAgent, joinRunParticipant): two concurrent invites each rebuilt the full settings object from their own stale snapshot, and the second write silently discarded the first's participant. Adds mutateWorkbenchParticipants, a targeted counterpart that wraps the read and write in a transaction with a SELECT ... FOR UPDATE row lock and writes back only the chat/participants JSONB key via jsonb_set — the same locking mechanism PR #490 (CL-7232) uses for the identical lost-update shape in access-policy's upsertPolicy. An optimistic wall-clock version stamp was rejected there for the reason that applies here too: two transactions starting in the same tick can both pass it. All four participant add/remove paths now call mutateWorkbenchParticipants with a pure fold over the current list instead of reading and rewriting a full settings snapshot, which drops the now-unnecessary existingSettings field from their inputs (mechanical call-site updates in routes.ts, workflow-participant-routes.ts, and apps/hub's slack-tag-mount.ts). updateWorkbenchSettings itself is unchanged and still serves callers that legitimately replace non-list settings keys. Regression coverage: two real concurrent transactions against a scratch Postgres database (settings-participants.drizzle.test.ts), verified by hand to fail against a naive non-transactional read-then-write and pass against the locked implementation; unit tests for the in-memory store's targeted-merge behavior; and end-to-end tests proving joinHumanParticipant and the invite route were correctly rewired off caller-rebuilt stale snapshots (these last two don't exercise real concurrency — the in-memory store's implementation has no await between its read and write, so Promise.all'd calls against it can't interleave — the real race proof is the Postgres-backed test). A known, ticketed gap (CL-7251) is left open: the settings PATCH route in routes.ts (owned by a parallel in-flight lane) still whole-blob-writes chat/participants on every settings PATCH, so it can still revert a concurrent invite. Closing it needs a more general targeted-merge primitive and touches a file this change doesn't own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
packages/chat/src/store.ts'supdateWorkbenchSettingsissued a blindUPDATE ... SET settings = $1— no version guard, no JSONB merge, whole-blob replace. Every participant add/remove path (joinHumanParticipant,removeWorkbenchParticipant,launchAndJoinAgentinworkbench-service.ts,joinRunParticipantinrun-participant.ts) was check-then-act off a settings snapshot read earlier in the same request. Two concurrent invites to the same workbench each rebuilt the full settings object from their own stale snapshot, and the second write silently discarded the first's participant — no error anywhere.Fixes CL-7194.
Concurrency mechanism
Adds
mutateWorkbenchParticipantstoChatStore: wraps the read and write in adb.transactionwith aSELECT ... FOR UPDATErow lock, folds a caller-supplied puremutateclosure over the currentchat/participantslist, and writes back only that JSONB key viajsonb_set— never touching other settings keys.This is the same mechanism PR #490 (CL-7232) uses for the identical lost-update shape in
packages/access-policy'supsertPolicy. That PR's review explicitly rejected an optimistic wall-clock version-stamp check for the same reason it doesn't work here: two transactions starting in the same tick can both pass it. Reusing the transactional row lock keeps the two fixes consistent rather than inventing a second mechanism for the same bug shape.All four participant add/remove paths now call
mutateWorkbenchParticipantswith a pure fold instead of reading and rewriting a full settings snapshot, dropping the now-unnecessaryexistingSettingsfield from their inputs. This forced mechanical call-site updates (just dropping the field being passed, no behavioral changes) inroutes.ts,workflow-participant-routes.ts, andapps/hub/src/slack-tag-mount.ts(a narrowedPick<ChatStore>type there needed widening to include the new method).updateWorkbenchSettingsitself is unchanged and still serves callers that legitimately replace non-list settings keys.No migration needed —
workbench_settingsand its(tenantId, workbenchId)lookup already exist.Tests
settings-participants.drizzle.test.ts: two real concurrent transactions against a scratch Postgres database, for both a two-invite race and a removal racing an addition. Verified by hand to fail against a naive non-transactional read-then-write implementation and pass against the locked one.store.test.ts: unit tests for the in-memory store's targeted-merge behavior (add, remove, missing-workbench error).workbench-service.test.ts/routes.test.ts: end-to-end tests provingjoinHumanParticipant,removeWorkbenchParticipant, and the invite route were correctly rewired off caller-rebuilt stale snapshots. These don't exercise real concurrency — the in-memory store's implementation has noawaitbetween its read and write, soPromise.all'd calls against it can't interleave — the Postgres-backed test is the actual race proof; named and commented accordingly.Known gap (ticketed, not fixed here)
routes.ts's workbench settings PATCH handler (owned by a parallel in-flight lane, CL-7192/PR #498) still whole-blob-writeschat/participantson every settings PATCH — not gated on the patch touching that key — so it can still silently revert a concurrent invite/removal. Filed as CL-7251 with the corrected scope. Closing it needs a more general targeted-merge primitive and touches a file this change doesn't own.Review process note
Greybeard reviewed the approach before implementation (confirmed the missing-row and JSONB-null edge cases were already handled correctly, and flagged the redundant existence pre-check in
joinRunParticipant, since removed). Critique reviewed the committed diff and found a real build-breaking regression (apps/hub/src/slack-tag-mount.ts's narrowedPick<ChatStore>type was never widened — now fixed), a tests-first/implementation commit split that didn't compile standalone (squashed into one commit), and two test descriptions that overstated what an in-memory-store race test actually proves (reworded). CL-7251's own description was also corrected for a scope inaccuracy Critique caught.DO NOT MERGE.