feat(server): status compare-and-set + locked-baseline patch + terminal-reopen gate (RBR-929/950/951/953) - #30
Merged
Conversation
…al-reopen gate (RBR-929/950/951/953) Consolidates the three RBR-929 step branches (RBR-950/951/953) that were verified passing on rbr953-terminal-reopen-gate but never merged to fork/master, cherry-picked and rebased onto current tip. RBR-950 (AC1/AC2/AC4): opt-in status compare-and-set on issuesSvc.update. `expectedStatus`/`expectedStatuses` becomes a SQL WHERE predicate (inArray(issues.status, ...)) on the write itself, so a losing write from a stale snapshot affects zero rows instead of clobbering. A losing CAS throws `conflict` (409), distinguishable from `null` (not-found). RBR-951 (AC3): the row-lock half of the fix, minus the blocked-bookkeeping test (rbr951-locked-baseline-patch.test.ts) and its assignee-derived cleanup logic, which depend on unblockDescriptor/blockedTransitionAt/ blockedOwnerNotifiedAt columns from an unrelated, unmerged upstream PR (paperclipai#10112) not present in this schema. Kept applyLockedBaselinePatchFields for the assignee-derived lock-clearing fields (checkoutRunId/executionRunId/ executionAgentNameKey/executionLockedAt), which are real columns here, and the re-assertion of assertTransition against the locked read inside runUpdate (the actual RBR-864 fix: a stale non-terminal snapshot cannot regress a row that has since committed done). RBR-953: gates terminal (done/cancelled) -> non-terminal transitions behind an explicit allowTerminalReopen opt-in, independent of the CAS opt-in. This is the actual fix for the RBR-864 class of incident (recovery reconciler reverting a completed issue back to blocked from a stale snapshot). allowTerminalReopen is granted to the 3 legitimate reopen callers that exist in this codebase: board/human comment reopen and the explicit PATCH /issues/:id status-write path (server/src/routes/issues.ts), and the deferred-comment-wake reopen (server/src/services/heartbeat.ts). A 4th caller from the original RBR-953 diff, the status-card compile reopen (server/src/services/status-cards.ts), does not exist in this codebase and was dropped. Also grants it to the task-watchdog revive path (server/src/services/task-watchdogs.ts), present here as `shouldReopen`. Test currency fix: the RBR-950 "stays opt-in" test asserted a plain `svc.update(id, { status: "blocked" })` with no CAS keys could still clobber `done`. That was true when RBR-950 landed alone but is stale now that RBR-953's assertTransition gate refuses *any* ungated done -> blocked write regardless of CAS keys. Split into two tests: the original opt-in behaviour now scoped to non-terminal -> non-terminal transitions (still true), plus a new test asserting the CAS opt-in and the terminal-reopen opt-in are independent gates (per RBR-1103 review). Verified on this branch (embedded Postgres, not July-era production): - rbr929-update-status-cas.test.ts: 9/9 pass (was 7/8 with the stale assertion; now 9/9 with the split test) - rbr953-terminal-reopen-gate.test.ts: 10/10 pass - tsc --noEmit: identical error set to unmodified fork/master (adapter-acpx module resolution + 2 pre-existing plugin-host-services.ts errors), confirmed by diffing against a clean fork/master worktree with the same node_modules — this diff introduces zero new typecheck errors. RBR-929, RBR-950, RBR-951, RBR-953, RBR-1103
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
2 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.
Lands the RBR-929 root primitive: consolidates RBR-950/951/953, which were verified passing on
fork/rbr953-terminal-reopen-gatebut never merged anywhere (RBR-1103). Cherry-picked cleanly onto currentfork/mastertip (only issues.ts conflicted; resolved by hand and diffed against the pre-resolved worktree — same net change).What's in it
issuesSvc.update.expectedStatus/expectedStatusesbecomes a SQLWHEREpredicate (inArray(issues.status, ...)) on the write itself. A losing CAS throwsconflict(409), distinguishable fromnull(not-found).assertTransitionand the assignee-derived lock-clearing fields (checkoutRunId/executionRunId/executionAgentNameKey/executionLockedAt) are now computed from the row read under.for("update"), not the pre-transaction snapshot. Dropped the blocked-bookkeeping half (unblockDescriptor/blockedTransitionAt/blockedOwnerNotifiedAt) — those columns don't exist in this schema; they come from an unrelated, unmerged upstream PR (Route blocked transitions to explicit unblock owners paperclipai/paperclip#10112). Also droppedrbr951-locked-baseline-patch.test.ts, which tested only the dropped half.done/cancelled) → non-terminal transitions behind an explicitallowTerminalReopenopt-in, independent of the CAS opt-in. This is the actual RBR-864 fix. Granted to the 3 legitimate reopen callers present in this codebase: board/human comment reopen + explicitPATCH /issues/:idstatus-write path (routes/issues.ts), deferred-comment-wake reopen (services/heartbeat.ts), and task-watchdog revive (services/task-watchdogs.ts,shouldReopen). The 4th caller in the original diff,status-cards.ts, doesn't exist in this codebase and was dropped.Test currency fix
The RBR-950 "stays opt-in" test asserted a plain
svc.update(id, { status: "blocked" })with no CAS keys could still clobberdone. True when RBR-950 landed alone, stale once RBR-953's gate landed (it refuses any ungateddone -> blocked, CAS keys or not). Split into: the original opt-in behaviour rescoped to non-terminal↔non-terminal transitions, plus a new test asserting the CAS opt-in and the terminal-reopen opt-in are independent gates.Verification (embedded Postgres, this branch)
rbr929-update-status-cas.test.ts: 9/9 passrbr953-terminal-reopen-gate.test.ts: 10/10 passtsc --noEmit: identical error set to a clean, unmodifiedfork/masterworktree (adapter-acpx module resolution + 2 pre-existingplugin-host-services.tserrors) — diffed directly, this PR introduces zero new typecheck errors.Closes RBR-1103. Unblocks RBR-933/RBR-885/RBR-864 once merged.