fix(dispatcher): recover legacy merge-ready continuations

[coreplane-switchboard]

Requested by justin · Thread

Repairs legacy merge-ready continuation only when durable and live evidence agree, then transfers sole pull-request ownership to the new attempt before work starts. This keeps exact-head continuation usable without reopening displacement or rollback races.

Why: Issue #2265 left otherwise safe legacy rows unable to continue. The exact-head re-review request requires recovery to preserve the agent-ship contract.

Where to look

1. Future merge-ready head receipt The driver records the exact final reviewed head so future continuations have durable expected-head evidence. ⚠ A wrong head could authorize an unreviewed revision.
2. Same-unit evidence recovery Recovery admits only completed coding and posted approval records attributable to the exact unit, requester and repository. ⚠ Broader evidence could adopt another unit's head.
3. Fresh pull-request receipt Continuation freshly requires an open same-repository pull request whose bound ref and live full head still match. ⚠ Partial or stale GitHub facts must never start work.
4. Guarded ownership transfer The fence transfers only when both the reservation token and current owner match, retaining ownership under the new attempt. ⚠ A permissive transfer would displace a newer runner.
5. CAS rollback and transfer protocol Ownership races surface failed reverse CAS results; winners transfer through the exact token and conditionally clean up. ⚠ Incorrect cleanup could overwrite newer state or leak a claim.
6. Commit-before-create boundary The hand-off transfers ownership after durable rows but before Workflow create, so transfer failure starts no work. ⚠ Moving this after create would permit ownerless live work.
7. Publication-owner end-to-end proof The real reissue's newly bound coding spawn reaches the admin gate and is accepted with the transferred publication owner. ⚠ A stub-only proof could miss the production owner gate.

Feedback wanted: Please verify the token/current-owner transfer timing and that every losing rollback path preserves newer durable and process-local ownership.

Risk: 1,478 changed lines across 19 files; splitting bot/store from ownership/hand-off was rejected because either half is unsafe. Failure can block continuation or misassign publication; rollback is one revert.

Verified: Focused 589 tests, root typecheck, changed-file Prettier/ESLint, hygiene, specs, coverage and title passed; full GitHub CI is in progress.

Decisions (4)

• Transfer before Workflow create. Claiming after create was rejected: non-displacing claim correctly leaves the ended owner in place, so the new attempt fails its publication-owner gate. The hand-off now knows the durable target owner and transfers before any Workflow can start.
• Require token and current owner. Owner identity alone cannot distinguish duplicate recovery callers. Transfer requires both the unforgeable reservation token and the still-current owner, so stale callers cannot displace or adopt a successor.
• Rollback by exact full-row CAS. An unconditional compensating write could erase concurrent state. Cleanup compares the exact recovered row before restoring the exact legacy row, and a losing reverse CAS is surfaced rather than ignored.
• Keep one cross-boundary safety commit. The durable Worker CAS, evidence gates, ownership reservation/transfer, hand-off lifecycle and regressions establish one protocol. Splitting them would leave an unsafe intermediate change.

Validation (9 criteria)

Criterion	Proof
The findings reproduce before implementation	Initial focused run failed 8 tests, including missing transferReservation, old-owner publication failure, ignored rollback CAS, and post-create commit ordering.
Exact token/current-owner transfer and conditional cleanup	src/core/runnerOwnership.test.ts transfer and stale-release regressions passed in the 589-test focused run.
Reissued attempt passes the production publication-owner gate	dispatcher test “a real reissue transfers…” uses real ship hand-off plus admin spawn; it passed.
Rollback CAS failure preserves newer state and starts nothing	dispatcher test “an ownership change whose exact rollback loses a CAS race…” passed and asserts the newer full row remains exact.
Focused behavior remains green after rebase	npx vitest run src/core/runnerOwnership.test.ts src/core/coordinator/handOff.test.ts src/core/dispatcher.test.ts — 3 files, 589 passed.
Root types remain valid	NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json — passed.
Changed files meet style gates	npx prettier --check on all 19 changed files and ESLint on changed code — passed.
Specs and public-tree consistency remain valid	hygiene:check, specs:check and specs:coverage --changed origin/main...HEAD --test-guard passed; 51 specs and 5,897 references checked.
Title contract remains valid	npm run check:pr-title -- "fix(dispatcher): recover legacy merge-ready continuations" — passed.

For agents

Rebased onto origin/main 652eb70 and force-pushed only plan/fix-issue-2265-s-missing-b41c79/u1. One coherent commit at 5ad8596. Review the durable full-row CAS, process-local reservation transfer, pre-create hand-off boundary and conditional abort as one protocol. No #2312 released files or nominal files changed. Full GitHub CI started at this head and was still in progress when this map was submitted.

🤖 Generated with Claude Code

[coreplane-switchboard]

Changes requested: The legacy repair can overwrite newer unit state and continue after stale ownership checks under concurrent replies.

Warning

Changes requested · head 378c911 · 1 finding: 1 major

Severity	Finding	Where
major	F1 Legacy recovery replaces the unit row without a concurrency fence	src/core/dispatcher.ts:1339

Full review

F1 — Major, high confidence. putUnits replaces the entire unit row after separate ownership and GitHub checks, without a compare-and-set or shared reservation. Two concurrent replies can both read the legacy row and pass those checks; after the first reissues and begins updating the unit, the second can write its stale recoveredUnit, erase newer progress, and continue using an ownership decision that is no longer current. Make recovery a conditional atomic transition that verifies the expected legacy row and reserves sole ownership before replacing it; a losing or stale caller must fail closed.

[justinhelmer]

Re-review requested at 0789d7c — F1 now uses a durable full-row compare-and-replace, claims local ownership only for the winning transition, and has a two-concurrent-replies regression proving the stale loser fails closed. The branch is one coherent commit, the PR map and full-SHA pointers are current, and all CI checks are green.

[coreplane-switchboard]

Changes requested: The row-level compare-and-replace is correct in the store, the Worker and the double. The ownership check still happens before an await and the claim after it, so a runner that claims the PR during the store round-trip can be displaced.

Warning

Changes requested · head 0789d7c · 2 findings: 1 minor, 1 nit

Severity	Finding	Where
minor	F1 Runner owner is checked before the awaited CAS and claimed after it; a runner claiming the PR in between is overwritten	src/core/dispatcher.ts:1356
nit	F2 Durable repair and fence claim run before the agent/profile gates and the hand-off; a later refusal leaves an unreleased claim keyed to the ended instance	src/core/dispatcher.ts:1340

Full review

The earlier F1 is fixed: the Worker, the in-memory double and the Worker-backed store all do a real compare-and-replace on the whole row, the loser fails closed, and the tests cover all three. What remains is a small race in the ownership fence.

F1. runnerOwnership.owner(...) is read at about line 1310, before claimLegacyContinuation is awaited. runnerOwnership!.claim(...) then runs at line 1356, after that await, and does not check the owner again. The CAS only protects this unit's row. It does not protect the fence entry for (repo, pr). A live runner can claim the same PR during the store round-trip: a different instance adopting it, or /admin/coordinator/rebase claiming by PR number. When that happens, claim silently overwrites claimedOwners with the ended instance. The spawn check at adminCoordinator.ts:909 then rejects the real runner's next coding child with publication_ownership_changed. That displaces another runner, which the new runnerOwnership doc comment says can never happen. The window is one Durable Object round-trip, so the chance is low, but it is exactly the check-then-act pattern this PR set out to remove. Fix: re-read owner() right before claim, with nothing awaited in between, and fail closed if a foreign owner shows up. Better still, make the fence method a single claimIfUnowned(owner) that returns false. Add a test where owner answers undefined at the first read and a foreign owner at the second.

F2. The row repair and the fence claim both happen before authorizeAgent, authorizeProfile and the ship hand-off run. If one of those later refuses, the row keeps lastPush and publication, which is harmless. But the fence keeps a claim for an ended instance, and nothing releases it until another unit-end or a restart. Either move the claim after the hand-off is admitted, or release it on the refusal paths.

The one test-guard check, the renamed driver test in driver.test.ts, is a refactor with verification intact. Its assertions were strengthened to include headSha, and the spec rows were updated to the new title.

[justinhelmer]

Re-review requested at 5fe3e1f — F1 now reserves ownership synchronously with an identity token before the awaited CAS, rechecks after CAS, and proves an intervening claimant makes recovery fail closed without work. F2 now defers repair to the final hand-off start gate and releases/rolls back on every non-started exit, with agent-gate and creation-failure regressions. The branch is one coherent commit, the full-SHA reader map is current, and all CI is green.

[coreplane-switchboard]

Changes requested: Both earlier findings are fixed. But claim no longer displaces an owner, and the committed reservation stays under the ended instance's id, so the reissued attempt can never take ownership of its own PR and its coding children fail the publication-owner gate.

Warning

Changes requested · head 5fe3e1f · 3 findings: 1 major, 1 minor, 1 nit

Severity	Finding	Where
major	F1 Committed reservation leaves the ended instance as PR owner; the new attempt's non-displacing claim is refused, so its existing-PR coding spawn gets publication_ownership_changed	src/core/dispatcher.ts:1335
minor	F2 Legacy recovery tests stub shipBranch, so nothing proves the reissued attempt can own and publish to the PR after commit	src/core/dispatcher.test.ts
nit	F3 Rollback CAS result is ignored on the ownership-changed path, so a failed restore is silent	src/core/dispatcher.ts:1397

Full review

Both earlier findings are fixed, but the fix blocks the reissued attempt from taking ownership of its own PR, so continuation stalls at its first coding step.

F1 (major). The reservation is made and committed under the ended instance's identity (owner.instanceId, which is plan-<id>). The ended instance never gets a unit-end or release, so after commitReservation it stays the fence owner of (repo, pr) until the process restarts. The hand-off then starts a new attempt, plan-<id>-2. rowsFor gives that attempt's row a publication.owner of {instanceId: "plan-<id>-2", unit}.

• When the new attempt reaches unit-start or a transition, adminCoordinator.ts:1831, :1877 and :2952 call claim. claim is now non-displacing, so it returns false against the old instance's owner, and those callers ignore the result.
• The existing-PR coding spawn gate (about line 909) then compares the fence owner, which is still plan-<id>, with publication.owner, which is plan-<id>-2. It answers publication_ownership_changed.
• So the recovered legacy continuation starts a runner that can never run its coding child, which is the flow this PR is meant to unblock.
• The same non-displacing claim also affects any ordinary re-issue where an older attempt's claim was never released.
• Fix: bind the reservation to the new attempt's instance id and unit, known only inside start(). Or have commit hand the reservation over to {instance.id, unit} of the created attempt. Also make the adminCoordinator callers fail closed when claim returns false, rather than proceeding.

F2 (minor). Every legacy-recovery dispatcher test replaces shipBranch with a stub that calls beforeCoordinatorStart and commit directly. No test runs the real hand-off and then a coding spawn through adminCoordinator for the new attempt, which is why F1 slipped through. Add one regression that recovers, hands off for real, and checks that the reissued attempt's unit-start and coding spawn pass the publication-owner gate.

F3 (nit). On the "changed runner ownership" path, the rollback claimLegacyContinuation(recoveredUnit, legacyUnit) swallows its result. If the restore is stale or unavailable, the repaired row is left behind silently. abort throws in the same situation, so this path should at least log or report it the same way.

[justinhelmer]

Re-review requested at 5ad8596 — F1 now transfers ownership before Workflow creation only when the exact reservation token and current owner match; transfer/create failures restore the exact legacy row and release only their owner. F2 adds a real dispatch→hand-off→new-attempt→admin-spawn proof that the transferred owner passes the publication gate. F3 surfaces reverse-CAS rollback loss deterministically without clobbering newer state or starting work. The branch is one coherent commit and the full-SHA reader map is current. One unrelated shell-timeout shard flaked; its failed jobs are retrying while every other check passed.

[coreplane-switchboard]

LGTM: Both previous findings and the nit are cleanly addressed in this push. Ownership transfer safely prevents duplicate attempts and lost bindings.

Note

Approved · head 5ad8596 · no findings

Full review

Both previous findings and the nit are cleanly addressed in this repush.

• F1 (major): The new transferReservation safely migrates the reservation to the durable new attempt under its exact token and owner identity. A transfer or create failure restores the exact legacy row and releases only the relevant owner, perfectly closing the ownership gap without clobbering newer state.
• F2 (minor): The new dispatcher test effectively proves the ownership transfer logic by walking through the entire reissue cycle from hand-off to the spawn passing the PR's publication-owner gate.
• F3 (nit): Reverse-CAS rollback failures (stale or unavailable) now surface deterministically within the refusal messages and never start continuation silently.

The test-guard correctly flags the renaming in driver.test.ts as licensed by the updated spec rows. The delta holds no new spec contradictions. Great work.

[github-actions]

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.