From 020c60f60ab83a9c7ea61cda0bb54471a1657b8c Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:19:26 -0400 Subject: [PATCH 1/7] docs(plan): implementation plan for change 0453 stale-receipt rotation guard Docket-Plan-Path: docs/superpowers/plans/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md --- ...g-one-stale-predecessor-receipt-can-sti.md | 316 ++++++++++++++++++ 1 file changed, 316 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md diff --git a/docs/superpowers/plans/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md b/docs/superpowers/plans/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md new file mode 100644 index 000000000..bd83bb385 --- /dev/null +++ b/docs/superpowers/plans/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md @@ -0,0 +1,316 @@ + +> ↩ **[Change 0453 — Two successors sharing one stale predecessor receipt can still free a live worktree slot](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0453-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md)** + +# Successor stale-receipt must not rotate a live worktree slot — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. For this change execution is by the docket-build skill. + +**Goal:** In `admitScopedWorktree`, refuse a successor whose predecessor receipt no longer names the scope's CURRENT drive with `ErrStalePredecessor` BEFORE it rotates an executing same-scope worktree slot, so a second successor sharing a now-stale receipt can never rotate — and later release — the first successor's live slot. + +**Architecture:** One guard inserted in the `admissionExecuting` arm of `admitScopedWorktree` (`internal/gatedrive/driver.go`), between the existing receipt-less `ErrScopeSecondDrive` refusal and `rotateWorktreeExecutionForSuccessor`. It loads the scope record and applies the exact predicate `reserveScopeDrive` already owns (`receipt drive id != scope current drive id` → `ErrStalePredecessor`), evaluated earlier to protect the one mutating step that precedes it. No new field, lock, error kind, or store function; `reserveScopeDrive` stays the authority for the scope slot, and nothing about the admission order (ADR-0118), `releasable`, or `isSameScopeRaceLoss` changes. + +**Tech Stack:** Go; deterministic in-package tests in `internal/gatedrive` using the existing fakes (`fakeClock`, `fakeProc`, `stableGit`, `scopedTestDriver`, `prepareScopedStart`). + +**Spec:** `docs/superpowers/specs/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-design.md` (on the `docket` metadata branch). + +## Global Constraints + +- The guard duplicates `reserveScopeDrive`'s predicate by value; per the spec that duplication carries the WHOLE predicate for this case (drive-id equality producing `ErrStalePredecessor`), and the code comment at the new site must name `reserveScopeDrive` as the authority so the twin is findable. +- Fail closed: a `LoadScope` error is returned as-is without touching the slot — never swallowed into a default that would let the comparison run against a zero-valued record. +- Every verification run defeats Go's test cache: `go test -count=1 …` always (a bare `go test` can serve a cached pass against a tree you just mutated). +- Mutation discipline: the red-first run of the new regression test against the unguarded code IS the spec's required mutation check; record its failure output before implementing the guard. Any later mutation probe restores `driver.go` only from committed state (`git checkout -- internal/gatedrive/driver.go`), never the uncommitted test file. +- The whole suite runs at the BUILD gate via whatever `build.test_command` resolves to (docket-build owns that); the per-task commands below are focused convenience runs, not the gate. +- Out of scope (spec): any admission/arbitration path not involving receipt staleness on rotation; change 0452's receipt-less fix; the ADR-0118 admission order. + +## Review Focus + +1. A stale-receipt successor reaching an executing same-scope slot must be refused `ErrStalePredecessor` with the slot's state, token, and `ExecutionGen` untouched — Task 1's test pins it. +2. A scope record that cannot be read (corrupt/IO) at the new guard must fail closed with the load error itself, not rotate and not degrade into `ErrStalePredecessor` against a zero record — Task 2's test pins it. +3. A FRESH successor (receipt naming the scope's current drive) must still rotate and launch exactly as before — Task 1 re-runs `TestBarrierSuccessorUnderCancel` and the successor admission tests. +4. A receipt-less first start reaching the executing slot must still refuse `ErrScopeSecondDrive` (change 0452's behavior) — Task 1 re-runs `TestSameScopeFirstStartLateLoserDoesNotRotate`. +5. An end-to-end `Start` by the stale second successor must launch nothing and leave the winner's slot intact (whatever typed refusal its precheck produces) — extra asserts at the end of Task 1's test. + +--- + +### Task 1: Stale-receipt rotation guard, TDD + +**Files:** +- Modify: `internal/gatedrive/driver.go` (function `admitScopedWorktree`, the `case admissionExecuting:` arm, and the function's doc comment) +- Test: `internal/gatedrive/driver_concurrency_test.go` (append the new test directly after `TestSameScopeFirstStartLateLoserDoesNotRotate`) + +**Interfaces:** +- Consumes: `d.store.LoadScope(id string) (scopeRecord, error)`; `scopeRecord.CurrentDriveID string`; `ownershipErr(kind OwnershipErrorKind, op string) error`; `ErrStalePredecessor`; existing test helpers `prepareScopedStart(t, store) (ScopeGrant, StartRequest)`, `scopedTestDriver(store, clk, proc, git) *Driver`, `store.ownerCAS(driveID string, mutate func(*driveRecord) error) error`, `store.LoadWorktreeExecution(worktree string)`, `isOwnershipKind(err, kind) bool`, `fakeProc.launchN`. +- Produces: `TestSameScopeSuccessorStaleReceiptDoesNotRotate` (Task 2 sits beside it and reuses its fixture shape); the guarded `admitScopedWorktree` behavior Task 2's fail-closed test depends on. + +- [ ] **Step 1: Write the failing regression test** + +Append to `internal/gatedrive/driver_concurrency_test.go`, immediately after `TestSameScopeFirstStartLateLoserDoesNotRotate` (keep its style — same fixtures, same untouched-slot assert shape): + +```go +// TestSameScopeSuccessorStaleReceiptDoesNotRotate deterministically pins the +// two-successor sibling of TestSameScopeFirstStartLateLoserDoesNotRotate +// (change 0453): successors S1 and S2 both present predecessor P's receipt and +// both passed precheckScopedStart before S1 retired P. S1 wins — launches and +// leaves the worktree slot executing under its own token. S2's worktree +// admission runs only now, with a receipt that no longer names the scope's +// CURRENT drive: it must refuse typed ErrStalePredecessor WITHOUT rotating, +// so its failure cleanup can never release S1's live reservation — same +// state, same token, same ExecutionGen. +func TestSameScopeSuccessorStaleReceiptDoesNotRotate(t *testing.T) { + clk := &fakeClock{now: startEpoch()} + store := OpenStore(testsupport.TempDir(t)) + proc := &fakeProc{} + d := scopedTestDriver(store, clk, proc, stableGit()) + _, req := prepareScopedStart(t, store) + + // Predecessor P: a full first start that launches, then settles to a + // durable PASSED in the terminal-before-release window (the + // TestBarrierSuccessorUnderCancel pattern), so successors may present it. + first, err := d.Start(req) + if err != nil { + t.Fatalf("predecessor Start: %v", err) + } + if first.Outcome != WAITING { + t.Fatalf("predecessor must WAIT, got %s (%s)", first.Outcome, first.Cause) + } + if err := store.ownerCAS(first.DriveID, func(r *driveRecord) error { + r.LastOutcome = PASSED + return nil + }); err != nil { + t.Fatalf("settle predecessor terminal: %v", err) + } + + // Successor S1 with P's receipt: rotates P's slot, launches, and leaves the + // worktree slot executing under S1's OWN token. + succ := req + succ.PredecessorDriveID = first.DriveID + succ.PredecessorOwnerGen = first.Generation + s1, err := d.Start(succ) + if err != nil { + t.Fatalf("successor S1 Start: %v", err) + } + if s1.Outcome != WAITING { + t.Fatalf("S1 must WAIT, got %s (%s)", s1.Outcome, s1.Cause) + } + before, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution: %v", err) + } + if before.State != admissionExecuting { + t.Fatalf("precondition: S1's slot must be executing, got %q", before.State) + } + + // S2: the SAME (now-stale) P receipt reaches worktree admission only now. + // Calling admitScopedWorktree directly models the successor that already + // passed its precheck before P was retired; admission must refuse typed + // and must not rotate S1's live reservation. + _, _, _, _, _, aerr := d.admitScopedWorktree(succ) + if !isOwnershipKind(aerr, ErrStalePredecessor) { + t.Fatalf("a stale-receipt successor must refuse ErrStalePredecessor, got %v", aerr) + } + after, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after refusal: %v", err) + } + if after.State != admissionExecuting || after.ReservationToken != before.ReservationToken || after.ExecutionGen != before.ExecutionGen { + t.Fatalf("S1's executing reservation must be untouched: state %q->%q, token changed=%v, gen %d->%d", + before.State, after.State, after.ReservationToken != before.ReservationToken, before.ExecutionGen, after.ExecutionGen) + } + + // Belt and suspenders: the FULL Start path for S2 must also launch nothing + // and leave S1's slot intact, whatever typed refusal its precheck produces. + launchesBefore := proc.launchN + if _, serr := d.Start(succ); serr == nil { + t.Fatalf("a stale-receipt successor Start must refuse") + } + if proc.launchN != launchesBefore { + t.Fatalf("a stale-receipt successor must never launch, launched %d->%d", launchesBefore, proc.launchN) + } + final, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after full Start: %v", err) + } + if final.State != admissionExecuting || final.ReservationToken != before.ReservationToken || final.ExecutionGen != before.ExecutionGen { + t.Fatalf("S1's executing reservation must survive S2's full Start: state %q, token changed=%v, gen %d->%d", + final.State, final.ReservationToken != before.ReservationToken, before.ExecutionGen, final.ExecutionGen) + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails against the unguarded code (this is the spec's mutation check)** + +Run: `go test -count=1 ./internal/gatedrive/ -run TestSameScopeSuccessorStaleReceiptDoesNotRotate -v` +Expected: FAIL at the direct `admitScopedWorktree` assert — the unguarded code rotates and returns a nil error, so the message is `a stale-receipt successor must refuse ErrStalePredecessor, got `. If it instead fails earlier (in fixture setup), fix the fixture — the red must come from the assert that pins the defect. Record this output; it is the evidence that the test discriminates. + +- [ ] **Step 3: Implement the guard** + +In `internal/gatedrive/driver.go`, inside `admitScopedWorktree`'s `case admissionExecuting:` arm, insert between the receipt-less refusal (`if req.PredecessorDriveID == "" { … ErrScopeSecondDrive … }`) and the comment block above `rotateWorktreeExecutionForSuccessor`: + +```go + // The receipt must still name the scope's CURRENT drive before the one + // mutating admission step (the rotation) runs. reserveScopeDrive stays + // the authority for the scope slot — this is its own staleness predicate + // (receipt drive id vs the scope's CurrentDriveID) evaluated earlier, so + // a second successor holding a retired predecessor's receipt never + // rotates a live slot that its inevitable ErrStalePredecessor cleanup + // would then release (change 0453). Reading the scope AFTER the slot + // read is sufficient: a slot executing under a successor's token was + // confirmed only after that successor's reserveScopeDrive advanced the + // scope, and the scope never moves back to an earlier drive; a racer + // holding an older slot token is refused by the rotation's own token + // check. A scope load failure fails closed unchanged, like the + // unreadable-slot leg above. + scope, serr := d.store.LoadScope(req.ScopeID) + if serr != nil { + return "", false, false, false, nil, serr + } + if scope.CurrentDriveID != req.PredecessorDriveID { + return "", false, false, false, nil, ownershipErr(ErrStalePredecessor, "start") + } +``` + +Also extend the function's doc comment: after the sentence ending "the successor confirms and owns its own post-launch failure legs (ownsSlot=true).", add: + +``` +// Rotation additionally requires the presented receipt to still name the +// scope's CURRENT drive (reserveScopeDrive's own staleness predicate, applied +// before the mutating step): a successor whose predecessor was already +// superseded is refused typed ErrStalePredecessor without touching the slot. +``` + +- [ ] **Step 4: Run the new test and the neighboring coverage** + +Run: `go test -count=1 ./internal/gatedrive/ -run 'TestSameScopeSuccessorStaleReceiptDoesNotRotate|TestSameScopeFirstStartLateLoserDoesNotRotate|TestBarrierSuccessorUnderCancel|TestBarrierSameScopeFirstStartContention' -v` +Expected: all PASS — the new refusal, 0452's receipt-less refusal, and the fresh-successor rotation path all intact. + +- [ ] **Step 5: Run the whole package** + +Run: `go test -count=1 ./internal/gatedrive/` +Expected: PASS (this includes `admission_successor_test.go`'s successor coverage the spec names). + +- [ ] **Step 6: Commit** + +```bash +git add internal/gatedrive/driver.go internal/gatedrive/driver_concurrency_test.go +git commit -m "fix(gatedrive): refuse a stale predecessor receipt before rotating an executing slot (change 0453)" +``` + +### Task 2: Fail-closed scope-read leg + +**Files:** +- Test: `internal/gatedrive/driver_concurrency_test.go` (append directly after `TestSameScopeSuccessorStaleReceiptDoesNotRotate`) + +**Interfaces:** +- Consumes: the Task 1 guard (its `LoadScope` call and error return); `store.scopeDir(id string) (string, error)` and the package constant `recordFileName` (both in-package, used to corrupt the stored scope record); everything Task 1's test consumes. +- Produces: `TestSameScopeSuccessorScopeReadFailureFailsClosed`. + +- [ ] **Step 1: Write the fail-closed test** + +```go +// TestSameScopeSuccessorScopeReadFailureFailsClosed pins the guard's error leg +// (change 0453): when the scope record cannot be read at the pre-rotation +// staleness check, admission must fail closed with the load error itself — +// never rotate, and never degrade into an ErrStalePredecessor verdict computed +// against a zero-valued record. +func TestSameScopeSuccessorScopeReadFailureFailsClosed(t *testing.T) { + clk := &fakeClock{now: startEpoch()} + store := OpenStore(testsupport.TempDir(t)) + proc := &fakeProc{} + d := scopedTestDriver(store, clk, proc, stableGit()) + _, req := prepareScopedStart(t, store) + + // Predecessor P launches and settles terminal; successor S1 rotates, + // launches, and leaves the slot executing under its own token (the + // TestSameScopeSuccessorStaleReceiptDoesNotRotate fixture). + first, err := d.Start(req) + if err != nil { + t.Fatalf("predecessor Start: %v", err) + } + if err := store.ownerCAS(first.DriveID, func(r *driveRecord) error { + r.LastOutcome = PASSED + return nil + }); err != nil { + t.Fatalf("settle predecessor terminal: %v", err) + } + succ := req + succ.PredecessorDriveID = first.DriveID + succ.PredecessorOwnerGen = first.Generation + if _, err := d.Start(succ); err != nil { + t.Fatalf("successor S1 Start: %v", err) + } + before, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution: %v", err) + } + if before.State != admissionExecuting { + t.Fatalf("precondition: S1's slot must be executing, got %q", before.State) + } + + // Corrupt the stored scope record so the guard's LoadScope fails. + dir, err := store.scopeDir(req.ScopeID) + if err != nil { + t.Fatalf("scopeDir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, recordFileName), []byte("{corrupt"), 0o644); err != nil { + t.Fatalf("corrupt scope record: %v", err) + } + + _, _, _, _, _, aerr := d.admitScopedWorktree(succ) + if aerr == nil { + t.Fatalf("a failed scope read must refuse admission") + } + if isOwnershipKind(aerr, ErrStalePredecessor) { + t.Fatalf("a failed scope read must surface the load error, not a staleness verdict: %v", aerr) + } + after, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after refusal: %v", err) + } + if after.State != admissionExecuting || after.ReservationToken != before.ReservationToken || after.ExecutionGen != before.ExecutionGen { + t.Fatalf("a failed scope read must leave S1's reservation untouched: state %q->%q, token changed=%v, gen %d->%d", + before.State, after.State, after.ReservationToken != before.ReservationToken, before.ExecutionGen, after.ExecutionGen) + } +} +``` + +If `os`/`path/filepath` are not already imported by `driver_concurrency_test.go`, add them to its import block. + +- [ ] **Step 2: Run it** + +Run: `go test -count=1 ./internal/gatedrive/ -run TestSameScopeSuccessorScopeReadFailureFailsClosed -v` +Expected: PASS (the guard from Task 1 is already in place). If `LoadScope` on the corrupt record somehow returns nil, that is a finding about the store, not a reason to weaken the asserts — stop and investigate (`readStoredScope` documents fail-closed on a corrupt document). + +- [ ] **Step 3: Mutation-check the error leg** + +In `internal/gatedrive/driver.go`, temporarily make the guard ignore the load error — replace `scope, serr := d.store.LoadScope(req.ScopeID)` and its `if serr != nil { … }` return with `scope, _ := d.store.LoadScope(req.ScopeID)` — then run: + +`go test -count=1 ./internal/gatedrive/ -run TestSameScopeSuccessorScopeReadFailureFailsClosed -v` + +Expected: FAIL with `a failed scope read must surface the load error, not a staleness verdict` (the zero record's empty `CurrentDriveID` mismatches the receipt). Then restore ONLY the committed guard file — the Task 1 commit contains it, so this is safe: + +```bash +git checkout -- internal/gatedrive/driver.go +``` + +Do not `git checkout` the test file — it is still uncommitted. Re-run the test once more after the restore and confirm PASS (with `-count=1`; never trust a cached verdict for either direction of a mutation probe). + +- [ ] **Step 4: Run the whole package** + +Run: `go test -count=1 ./internal/gatedrive/` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add internal/gatedrive/driver_concurrency_test.go +git commit -m "test(gatedrive): pin fail-closed scope read at the pre-rotation staleness guard (change 0453)" +``` + +--- + +## Self-Review (performed at plan time) + +- Spec coverage: the guard (spec "Design", all three bullets — LoadScope, fail-closed error, staleness comparison, otherwise rotate unchanged) is Task 1 Step 3; the regression test with the spec's four numbered steps and untouched-slot asserts is Task 1 Step 1; the mutation check against unguarded code is Task 1 Step 2 (red-first); existing successor coverage staying green is Task 1 Steps 4–5; the whole suite runs at the docket-build gate. The spec's "why an unlocked read is sufficient" argument is preserved in the new code comment; the rejected alternative (threading run identity into the locked rotate CAS) is correctly not built. +- Placeholders: none — every step carries the exact code, command, and expected output. +- Type consistency: `LoadScope` returns `(scopeRecord, error)`; `scopeRecord.CurrentDriveID` and `req.PredecessorDriveID` are both `string`; `admitScopedWorktree` returns six values and every new return spells all six; helper names (`ownerCAS`, `scopeDir`, `recordFileName`, `isOwnershipKind`, `launchN`) verified against the current tree. +- Review Focus: all five lines have owning tests — 1, 3, 4, 5 in Task 1; 2 in Task 2. From ef72476b3c35b3ecae6fba2c6a6a4eda141ee398 Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:26:03 -0400 Subject: [PATCH 2/7] fix(gatedrive): refuse a stale predecessor receipt before rotating an executing slot (change 0453) Apply reserveScopeDrive's staleness predicate in admitScopedWorktree's executing arm before rotateWorktreeExecutionForSuccessor, so a second successor sharing a now-stale receipt can never rotate (and later release) the first successor's live slot. A scope load failure fails closed. TestSuccessorAdmissionFailureLegsReleaseRotatedSlot pinned the old rotate-then-release behavior for a stale receipt; it now asserts the pre-rotation refusal and reaches the post-rotation release leg through a scope closed between precheck and admission (epoch-gate seam). --- .../gatedrive/admission_successor_test.go | 54 +++++++++-- internal/gatedrive/driver.go | 26 +++++- internal/gatedrive/driver_concurrency_test.go | 89 +++++++++++++++++++ 3 files changed, 162 insertions(+), 7 deletions(-) diff --git a/internal/gatedrive/admission_successor_test.go b/internal/gatedrive/admission_successor_test.go index eda522c00..2e3dddda9 100644 --- a/internal/gatedrive/admission_successor_test.go +++ b/internal/gatedrive/admission_successor_test.go @@ -209,10 +209,18 @@ func TestLatePredecessorReleaseCannotFreeSuccessor(t *testing.T) { // TestSuccessorAdmissionFailureLegsReleaseRotatedSlot proves a post-rotation // admission-half failure releases the rotated reservation (never leaks it, never -// leaves it blocking). A stale receipt — naming a reusable PASSED drive that is -// NOT the scope's current drive — passes the unlocked precheck but is refused by -// the reserveScopeDrive authority AFTER the rotation ran; the rotated slot must be -// released, and the scope state stays byte-unchanged from the failure's contract. +// leaves it blocking), and that a stale receipt never reaches that leg at all. +// +// A stale receipt — naming a reusable PASSED drive that is NOT the scope's current +// drive — passes the unlocked precheck, but admitScopedWorktree applies +// reserveScopeDrive's staleness predicate BEFORE the rotation (change 0453): it is +// refused ErrStalePredecessor with the executing slot unrotated. +// +// A FRESH receipt whose scope closes between the unlocked precheck and admission +// (modelled through the epoch-gate seam, which runs after precheck and wraps the +// admission body) rotates the slot and is then refused ErrScopeClosed by the +// reserveScopeDrive authority: the rotated slot must be released, and the scope +// record stays byte-unchanged by the failed reservation. func TestSuccessorAdmissionFailureLegsReleaseRotatedSlot(t *testing.T) { clk := &fakeClock{now: startEpoch()} store := OpenStore(testsupport.TempDir(t)) @@ -261,7 +269,41 @@ func TestSuccessorAdmissionFailureLegsReleaseRotatedSlot(t *testing.T) { slot, _, err := store.LoadWorktreeExecution(req.Worktree) if err != nil { - t.Fatalf("LoadWorktreeExecution after refusal: %v", err) + t.Fatalf("LoadWorktreeExecution after stale refusal: %v", err) + } + if slot.State != admissionExecuting || slot.ReservationToken != predSlot.ReservationToken || slot.ExecutionGen != predSlot.ExecutionGen { + t.Fatalf("a stale receipt must be refused BEFORE rotation: state %q, token changed=%v, gen %d->%d", + slot.State, slot.ReservationToken != predSlot.ReservationToken, predSlot.ExecutionGen, slot.ExecutionGen) + } + if !bytes.Equal(scopeBefore, readScopeBytes(t, store, req.ScopeID)) { + t.Fatalf("a refused stale successor must leave the scope record byte-unchanged") + } + + // Post-rotation failure leg: a fresh receipt whose scope closes after the + // unlocked precheck. The gate closes the scope, then runs the admission body. + var scopeClosed []byte + d.SetEpochLaunchGate(func(_, _ string, reserve func() error) error { + if err := store.scopeCAS(req.ScopeID, func(rec *scopeRecord) error { + rec.Closed = true + return nil + }); err != nil { + t.Fatalf("close scope mid-admission: %v", err) + } + scopeClosed = readScopeBytes(t, store, req.ScopeID) + return reserve() + }) + fresh := req + fresh.PredecessorDriveID = cur.DriveID + fresh.PredecessorOwnerGen = cur.Generation + if _, serr := d.Start(fresh); !isOwnership(serr, ErrScopeClosed) { + t.Fatalf("a successor whose scope closed mid-admission must be refused ErrScopeClosed, got %v", serr) + } + if proc.launchN != launchesBefore { + t.Fatalf("a refused successor must never launch, launched %d->%d", launchesBefore, proc.launchN) + } + slot, _, err = store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after post-rotation refusal: %v", err) } if slot.State != admissionReleased { t.Fatalf("a post-rotation failure must RELEASE the rotated reservation, got %q", slot.State) @@ -269,7 +311,7 @@ func TestSuccessorAdmissionFailureLegsReleaseRotatedSlot(t *testing.T) { if slot.ReservationToken == predSlot.ReservationToken { t.Fatalf("the rotation must have minted a fresh token before the failing leg") } - if !bytes.Equal(scopeBefore, readScopeBytes(t, store, req.ScopeID)) { + if scopeClosed == nil || !bytes.Equal(scopeClosed, readScopeBytes(t, store, req.ScopeID)) { t.Fatalf("a failed successor reservation must leave the scope record byte-unchanged") } } diff --git a/internal/gatedrive/driver.go b/internal/gatedrive/driver.go index 834b46d86..1a4c5f515 100644 --- a/internal/gatedrive/driver.go +++ b/internal/gatedrive/driver.go @@ -1072,7 +1072,11 @@ func (d *Driver) launchScoped(t *AdmissionTicket, claim *relaunchClaim) (DriveDo // continuing the sequence in the terminal-before-release window) ROTATES it to its // OWN fresh reservation — a new ReservationToken and bumped ExecutionGen — so the // predecessor's stale token can never free or poison the successor's slot, and the -// successor confirms and owns its own post-launch failure legs (ownsSlot=true). A +// successor confirms and owns its own post-launch failure legs (ownsSlot=true). +// Rotation additionally requires the presented receipt to still name the +// scope's CURRENT drive (reserveScopeDrive's own staleness predicate, applied +// before the mutating step): a successor whose predecessor was already +// superseded is refused typed ErrStalePredecessor without touching the slot. A // RECEIPT-LESS first start that finds a same-scope executing slot has raced an // already-launched drive and is refused typed ErrScopeSecondDrive without touching the // slot. A slot held by a DIFFERENT scope, or in a stopping/unresolved state, is a @@ -1114,6 +1118,26 @@ func (d *Driver) admitScopedWorktree(req StartRequest) (token string, reservedFr if req.PredecessorDriveID == "" { return "", false, false, false, nil, ownershipErr(ErrScopeSecondDrive, "start") } + // The receipt must still name the scope's CURRENT drive before the one + // mutating admission step (the rotation) runs. reserveScopeDrive stays + // the authority for the scope slot — this is its own staleness predicate + // (receipt drive id vs the scope's CurrentDriveID) evaluated earlier, so + // a second successor holding a retired predecessor's receipt never + // rotates a live slot that its inevitable ErrStalePredecessor cleanup + // would then release (change 0453). Reading the scope AFTER the slot + // read is sufficient: a slot executing under a successor's token was + // confirmed only after that successor's reserveScopeDrive advanced the + // scope, and the scope never moves back to an earlier drive; a racer + // holding an older slot token is refused by the rotation's own token + // check. A scope load failure fails closed unchanged, like the + // unreadable-slot leg above. + scope, serr := d.store.LoadScope(req.ScopeID) + if serr != nil { + return "", false, false, false, nil, serr + } + if scope.CurrentDriveID != req.PredecessorDriveID { + return "", false, false, false, nil, ownershipErr(ErrStalePredecessor, "start") + } // A same-scope successor continues over the executing slot: rotate it to // this start's OWN fresh reservation rather than reusing the predecessor's // token. The successor then confirms and owns its slot (ownsSlot=true), and diff --git a/internal/gatedrive/driver_concurrency_test.go b/internal/gatedrive/driver_concurrency_test.go index 50ee48e02..105fc3b47 100644 --- a/internal/gatedrive/driver_concurrency_test.go +++ b/internal/gatedrive/driver_concurrency_test.go @@ -1522,6 +1522,95 @@ func TestSameScopeFirstStartLateLoserDoesNotRotate(t *testing.T) { } } +// TestSameScopeSuccessorStaleReceiptDoesNotRotate deterministically pins the +// two-successor sibling of TestSameScopeFirstStartLateLoserDoesNotRotate +// (change 0453): successors S1 and S2 both present predecessor P's receipt and +// both passed precheckScopedStart before S1 retired P. S1 wins — launches and +// leaves the worktree slot executing under its own token. S2's worktree +// admission runs only now, with a receipt that no longer names the scope's +// CURRENT drive: it must refuse typed ErrStalePredecessor WITHOUT rotating, +// so its failure cleanup can never release S1's live reservation — same +// state, same token, same ExecutionGen. +func TestSameScopeSuccessorStaleReceiptDoesNotRotate(t *testing.T) { + clk := &fakeClock{now: startEpoch()} + store := OpenStore(testsupport.TempDir(t)) + proc := &fakeProc{} + d := scopedTestDriver(store, clk, proc, stableGit()) + _, req := prepareScopedStart(t, store) + + // Predecessor P: a full first start that launches, then settles to a + // durable PASSED in the terminal-before-release window (the + // TestBarrierSuccessorUnderCancel pattern), so successors may present it. + first, err := d.Start(req) + if err != nil { + t.Fatalf("predecessor Start: %v", err) + } + if first.Outcome != WAITING { + t.Fatalf("predecessor must WAIT, got %s (%s)", first.Outcome, first.Cause) + } + if err := store.ownerCAS(first.DriveID, func(r *driveRecord) error { + r.LastOutcome = PASSED + return nil + }); err != nil { + t.Fatalf("settle predecessor terminal: %v", err) + } + + // Successor S1 with P's receipt: rotates P's slot, launches, and leaves the + // worktree slot executing under S1's OWN token. + succ := req + succ.PredecessorDriveID = first.DriveID + succ.PredecessorOwnerGen = first.Generation + s1, err := d.Start(succ) + if err != nil { + t.Fatalf("successor S1 Start: %v", err) + } + if s1.Outcome != WAITING { + t.Fatalf("S1 must WAIT, got %s (%s)", s1.Outcome, s1.Cause) + } + before, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution: %v", err) + } + if before.State != admissionExecuting { + t.Fatalf("precondition: S1's slot must be executing, got %q", before.State) + } + + // S2: the SAME (now-stale) P receipt reaches worktree admission only now. + // Calling admitScopedWorktree directly models the successor that already + // passed its precheck before P was retired; admission must refuse typed + // and must not rotate S1's live reservation. + _, _, _, _, _, aerr := d.admitScopedWorktree(succ) + if !isOwnershipKind(aerr, ErrStalePredecessor) { + t.Fatalf("a stale-receipt successor must refuse ErrStalePredecessor, got %v", aerr) + } + after, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after refusal: %v", err) + } + if after.State != admissionExecuting || after.ReservationToken != before.ReservationToken || after.ExecutionGen != before.ExecutionGen { + t.Fatalf("S1's executing reservation must be untouched: state %q->%q, token changed=%v, gen %d->%d", + before.State, after.State, after.ReservationToken != before.ReservationToken, before.ExecutionGen, after.ExecutionGen) + } + + // Belt and suspenders: the FULL Start path for S2 must also launch nothing + // and leave S1's slot intact, whatever typed refusal its precheck produces. + launchesBefore := proc.launchN + if _, serr := d.Start(succ); serr == nil { + t.Fatalf("a stale-receipt successor Start must refuse") + } + if proc.launchN != launchesBefore { + t.Fatalf("a stale-receipt successor must never launch, launched %d->%d", launchesBefore, proc.launchN) + } + final, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after full Start: %v", err) + } + if final.State != admissionExecuting || final.ReservationToken != before.ReservationToken || final.ExecutionGen != before.ExecutionGen { + t.Fatalf("S1's executing reservation must survive S2's full Start: state %q, token changed=%v, gen %d->%d", + final.State, final.ReservationToken != before.ReservationToken, before.ExecutionGen, final.ExecutionGen) + } +} + // TestBarrierSuccessorUnderCancel proves the successor path under a mid-flight // fence: a fenced successor start refuses without launching, and it leaves the slot // EITHER the predecessor's executing reservation (refused before rotation) OR From ad3f6dd4d4237befddcc7230c065c431199b8fd5 Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:28:49 -0400 Subject: [PATCH 3/7] test(gatedrive): pin fail-closed scope read at the pre-rotation staleness guard (change 0453) --- internal/gatedrive/driver_concurrency_test.go | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/internal/gatedrive/driver_concurrency_test.go b/internal/gatedrive/driver_concurrency_test.go index 105fc3b47..a6e4c9860 100644 --- a/internal/gatedrive/driver_concurrency_test.go +++ b/internal/gatedrive/driver_concurrency_test.go @@ -1611,6 +1611,74 @@ func TestSameScopeSuccessorStaleReceiptDoesNotRotate(t *testing.T) { } } +// TestSameScopeSuccessorScopeReadFailureFailsClosed pins the guard's error leg +// (change 0453): when the scope record cannot be read at the pre-rotation +// staleness check, admission must fail closed with the load error itself — +// never rotate, and never degrade into an ErrStalePredecessor verdict computed +// against a zero-valued record. +func TestSameScopeSuccessorScopeReadFailureFailsClosed(t *testing.T) { + clk := &fakeClock{now: startEpoch()} + store := OpenStore(testsupport.TempDir(t)) + proc := &fakeProc{} + d := scopedTestDriver(store, clk, proc, stableGit()) + _, req := prepareScopedStart(t, store) + + // Predecessor P launches and settles terminal; successor S1 rotates, + // launches, and leaves the slot executing under its own token (the + // TestSameScopeSuccessorStaleReceiptDoesNotRotate fixture). + first, err := d.Start(req) + if err != nil { + t.Fatalf("predecessor Start: %v", err) + } + if err := store.ownerCAS(first.DriveID, func(r *driveRecord) error { + r.LastOutcome = PASSED + return nil + }); err != nil { + t.Fatalf("settle predecessor terminal: %v", err) + } + succ := req + succ.PredecessorDriveID = first.DriveID + succ.PredecessorOwnerGen = first.Generation + if _, err := d.Start(succ); err != nil { + t.Fatalf("successor S1 Start: %v", err) + } + before, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution: %v", err) + } + if before.State != admissionExecuting { + t.Fatalf("precondition: S1's slot must be executing, got %q", before.State) + } + + // Corrupt the stored scope record so the guard's LoadScope fails. + dir, err := store.scopeDir(req.ScopeID) + if err != nil { + t.Fatalf("scopeDir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, recordFileName), []byte("{corrupt"), 0o644); err != nil { + t.Fatalf("corrupt scope record: %v", err) + } + + _, _, _, _, _, aerr := d.admitScopedWorktree(succ) + if aerr == nil { + t.Fatalf("a failed scope read must refuse admission") + } + if isOwnershipKind(aerr, ErrStalePredecessor) { + t.Fatalf("a failed scope read must surface the load error, not a staleness verdict: %v", aerr) + } + if !isStoreKind(aerr, ErrCorruptRecord) { + t.Fatalf("a failed scope read must surface the scope load error itself (ErrCorruptRecord), got %v", aerr) + } + after, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after refusal: %v", err) + } + if after.State != admissionExecuting || after.ReservationToken != before.ReservationToken || after.ExecutionGen != before.ExecutionGen { + t.Fatalf("a failed scope read must leave S1's reservation untouched: state %q->%q, token changed=%v, gen %d->%d", + before.State, after.State, after.ReservationToken != before.ReservationToken, before.ExecutionGen, after.ExecutionGen) + } +} + // TestBarrierSuccessorUnderCancel proves the successor path under a mid-flight // fence: a fenced successor start refuses without launching, and it leaves the slot // EITHER the predecessor's executing reservation (refused before rotation) OR From 2c3c282c5263c899c44b157d70121d719dc89454 Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:50:28 -0400 Subject: [PATCH 4/7] fix(gatedrive): keep a sibling-adopted reservation when a successor loses to a stale receipt (change 0453) Review finding (important): two successors presenting the same predecessor receipt P on an unserialized (epoch-less) scope could still free a live slot. S2 passes the pre-rotation staleness guard while P is current and rotates the slot to T (reserved); S1, holding the same receipt, finds a same-scope reserved slot and adopts T, wins reserveScopeDrive, retires P, launches, and confirms T executing. S2's reserveScopeDrive then refuses ErrStalePredecessor, which isSameScopeRaceLoss does not cover, so S2 (rotated) released T and freed S1's executing slot. A freshly reserving successor had the same leg. admitScoped's reserveScopeDrive failure leg now also keeps the reservation when siblingMayHoldReservation holds: the error is ErrStalePredecessor on a successor AND the reloaded scope's PriorDriveID still names the receipt's drive (a sibling consumed this receipt, necessarily by adopting T for a rotated start, possibly still on its way to the scope slot), or the scope's current drive carries T as its AdmissionToken (a later drive adopted T), or either record is unreadable. Keeping is the fail-closed direction: a leaked reserved slot is adopted by the scope's next start and blocks other admissions until recovery, while releasing an adopted one frees a live slot. A stale receipt with neither sign still releases, so a bogus receipt leaks nothing. The admitScopedWorktree guard comment no longer claims the unlocked scope read is sufficient: it excludes only a successor arriving after a sibling launched. TestStaleSuccessorLeavesSiblingAdoptedReservation drives the interleaving deterministically through a package-private scopedAdmissionHook (fired between worktree admission and reserveScopeDrive): rotated and fresh sibling-adopts- and-wins, a later drive adopting after the scope moved on (token clause only), and an adopter still pending when S2 loses (PriorDriveID clause only). All four subtests are red with the guard disabled; dropping either clause reddens exactly its subtest. --- internal/gatedrive/driver.go | 97 +++++- internal/gatedrive/driver_concurrency_test.go | 289 ++++++++++++++++++ 2 files changed, 375 insertions(+), 11 deletions(-) diff --git a/internal/gatedrive/driver.go b/internal/gatedrive/driver.go index 1a4c5f515..aec0d5364 100644 --- a/internal/gatedrive/driver.go +++ b/internal/gatedrive/driver.go @@ -844,6 +844,12 @@ func (d *Driver) launchScopeless(t *AdmissionTicket, claim *relaunchClaim) (Driv return startDocWithLegacy(doc, derr, t.legacy) } +// scopedAdmissionHook is a package-private test seam fired once per scoped admission, +// after admitScopedWorktree has returned this start's worktree token and before +// reserveScopeDrive arbitrates the scope slot. Production leaves it nil; a test sets +// it to land a same-scope sibling's start at exactly that instant (change 0453). +var scopedAdmissionHook func(req StartRequest) + // admitScoped runs the pre-launch admission half of the pinned scoped-start order // (change 0405 Tasks 3–4 plus change 0375 worktree admission): // @@ -882,6 +888,9 @@ func (d *Driver) admitScoped(req StartRequest, rec driveRecord, ownerGen string) if aerr != nil { return nil, aerr } + if scopedAdmissionHook != nil { + scopedAdmissionHook(req) + } rec.AdmissionToken = token // releasable unifies "freshly reserved" and "rotated": either way this start // alone owns the reservation and must release it on a genuine pre-launch failure. @@ -911,10 +920,12 @@ func (d *Driver) admitScoped(req StartRequest, rec driveRecord, ownerGen string) // Release the worktree slot ONLY when THIS start freshly reserved it AND the // loss is not a same-scope race: a same-scope peer that beat us to the scope slot // has adopted our reservation (there is at most one fresh reservation per worktree - // at a time), so releasing it would free a slot the winner is using. A genuine - // failure (scope closed, an IO fault, an identity mismatch) has no adopter, so the - // fresh (or rotated) reservation must be released rather than leaked. - if releasable && !isSameScopeRaceLoss(rerr) { + // at a time), so releasing it would free a slot the winner is using. A successor + // refused ErrStalePredecessor because a SIBLING consumed the same receipt is the + // same race (siblingMayHoldReservation). A genuine failure (scope closed, an IO + // fault, an identity mismatch) has no adopter, so the fresh (or rotated) + // reservation must be released rather than leaked. + if releasable && !isSameScopeRaceLoss(rerr) && !d.siblingMayHoldReservation(req.ScopeID, receipt, token, rerr) { _ = d.store.ReleaseWorktreeExecution(req.Worktree, token) } return nil, rerr @@ -1124,13 +1135,19 @@ func (d *Driver) admitScopedWorktree(req StartRequest) (token string, reservedFr // (receipt drive id vs the scope's CurrentDriveID) evaluated earlier, so // a second successor holding a retired predecessor's receipt never // rotates a live slot that its inevitable ErrStalePredecessor cleanup - // would then release (change 0453). Reading the scope AFTER the slot - // read is sufficient: a slot executing under a successor's token was - // confirmed only after that successor's reserveScopeDrive advanced the - // scope, and the scope never moves back to an earlier drive; a racer - // holding an older slot token is refused by the rotation's own token - // check. A scope load failure fails closed unchanged, like the - // unreadable-slot leg above. + // would then release (change 0453). This unlocked read excludes only a + // successor that arrives AFTER a sibling launched on the same receipt: + // for that ordering, reading the scope after the slot read suffices, + // because a slot executing under a successor's token was confirmed only + // after that successor's reserveScopeDrive advanced the scope, the scope + // never moves back to an earlier drive, and a racer holding an older + // slot token is refused by the rotation's own token check. It does NOT + // serialize two successors that both pass it while the receipt is still + // current: the second can adopt this start's rotated reservation and win + // the scope slot, so this start's later ErrStalePredecessor must leave + // the reservation to that adopter — admitScoped's reserveScopeDrive + // failure leg does (siblingMayHoldReservation). A scope load failure + // fails closed unchanged, like the unreadable-slot leg above. scope, serr := d.store.LoadScope(req.ScopeID) if serr != nil { return "", false, false, false, nil, serr @@ -1196,6 +1213,64 @@ func isSameScopeRaceLoss(err error) bool { return oe.Kind == ErrScopeBusy || oe.Kind == ErrScopeSecondDrive } +// siblingMayHoldReservation reports whether a successor start whose reserveScopeDrive +// was refused ErrStalePredecessor may have had its fresh or rotated worktree +// reservation ADOPTED by a same-scope sibling, so the reservation must be left in +// place rather than released (change 0453). It is the successor counterpart of +// isSameScopeRaceLoss. +// +// Two successors can present the same predecessor receipt P. Admissions are not +// serialized across them (an epoch-less scope runs the admission body directly), so +// the pre-rotation staleness guard in admitScopedWorktree does not exclude this +// interleaving: S2 passes the guard while P is current and rotates (or freshly +// reserves) the slot to T; S1 finds a same-scope RESERVED slot and adopts T; S1 wins +// reserveScopeDrive, retires P, launches, and confirms the slot executing under T; +// only then does S2's reserveScopeDrive refuse ErrStalePredecessor. Releasing T there +// would free S1's live slot. +// +// The reservation is left in place when the reloaded scope shows either sign of a +// sibling: +// +// - PriorDriveID still names the receipt's drive: a sibling consumed THIS receipt. +// A sibling that did so while this start held T could take the worktree only by +// adopting T — always the case for a rotated start, whose pre-rotation guard saw +// the receipt current — and a sibling adopting T may still be on its way to the +// scope slot. (A freshly reserving start can also lose to a sibling that +// consumed the receipt earlier and released its own slot; T is then merely +// leaked, which fails closed as below.) +// - The scope's current drive carries T as its AdmissionToken: a later drive of +// the sequence adopted T after the scope moved past the receipt's successor. +// +// An unreadable scope or current drive record cannot prove T unadopted and also +// keeps it. Keeping is the fail-closed direction: a leaked reserved slot is adopted +// by the scope's next start and refuses every other admission until recovery, +// whereas releasing an adopted one frees a live slot. Every other rejection, and a +// stale receipt showing neither sign (a receipt the scope never advanced from, or +// one it has moved two drives past with T unadopted), has no adopter and is released. +func (d *Driver) siblingMayHoldReservation(scopeID string, receipt predecessorReceipt, token string, rerr error) bool { + if receipt.empty() { + return false + } + if oe, ok := AsOwnershipError(rerr); !ok || oe.Kind != ErrStalePredecessor { + return false + } + scope, err := d.store.LoadScope(scopeID) + if err != nil { + return true + } + if scope.PriorDriveID == receipt.DriveID { + return true + } + if scope.CurrentDriveID == "" { + return false + } + cur, err := d.store.Load(scope.CurrentDriveID) + if err != nil { + return true + } + return cur.AdmissionToken == token +} + // Advance resumes a drive through at most one slice. It loads the durable record // (the only source of truth), verifies the presented owner generation, and — for // a still-live drive — runs one slice and persists the transition. A record that diff --git a/internal/gatedrive/driver_concurrency_test.go b/internal/gatedrive/driver_concurrency_test.go index a6e4c9860..1f741a9b9 100644 --- a/internal/gatedrive/driver_concurrency_test.go +++ b/internal/gatedrive/driver_concurrency_test.go @@ -1679,6 +1679,295 @@ func TestSameScopeSuccessorScopeReadFailureFailsClosed(t *testing.T) { } } +// hookGit is a GitSeam whose onHead fires once, on the next fingerprint's first +// read. Start fingerprints AFTER precheckScopedStart and BEFORE worktree admission, +// so a test uses it to land a sibling's whole start between the two. +type hookGit struct { + *fakeGit + onHead func() +} + +func (g *hookGit) HeadOID(dir string) (string, error) { + if fn := g.onHead; fn != nil { + g.onHead = nil + fn() + } + return g.fakeGit.HeadOID(dir) +} + +// siblingFixture is an epoch-less scope whose first drive P has launched and +// settled PASSED, so successors may present P's receipt. +type siblingFixture struct { + d *Driver + store *Store + proc *fakeProc + git *hookGit + req StartRequest + pred DriveDoc +} + +func newSiblingFixture(t *testing.T) *siblingFixture { + t.Helper() + clk := &fakeClock{now: startEpoch()} + store := OpenStore(testsupport.TempDir(t)) + proc := &fakeProc{} + git := &hookGit{fakeGit: stableGit()} + d := scopedTestDriver(store, clk, proc, git) + _, req := prepareScopedStart(t, store) + if req.RunEpochID != "" { + t.Fatalf("precondition: the scope must be epoch-less (admissions unserialized), got epoch %q", req.RunEpochID) + } + f := &siblingFixture{d: d, store: store, proc: proc, git: git, req: req} + f.pred = f.startWaiting(t, req, "predecessor P") + f.settlePassed(t, f.pred) + return f +} + +func (f *siblingFixture) successorOf(doc DriveDoc) StartRequest { + s := f.req + s.PredecessorDriveID = doc.DriveID + s.PredecessorOwnerGen = doc.Generation + return s +} + +func (f *siblingFixture) startWaiting(t *testing.T, req StartRequest, who string) DriveDoc { + t.Helper() + doc, err := f.d.Start(req) + if err != nil { + t.Fatalf("%s Start: %v", who, err) + } + if doc.Outcome != WAITING { + t.Fatalf("%s must WAIT, got %s (%s)", who, doc.Outcome, doc.Cause) + } + return doc +} + +func (f *siblingFixture) settlePassed(t *testing.T, doc DriveDoc) { + t.Helper() + if err := f.store.ownerCAS(doc.DriveID, func(r *driveRecord) error { + r.LastOutcome = PASSED + return nil + }); err != nil { + t.Fatalf("settle %s PASSED: %v", doc.DriveID, err) + } +} + +func (f *siblingFixture) slot(t *testing.T) admissionRecord { + t.Helper() + slot, _, err := f.store.LoadWorktreeExecution(f.req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution: %v", err) + } + return slot +} + +// releaseSlot releases the settled drive's worktree slot, closing the +// terminal-before-release window so the next start freshly reserves. +func (f *siblingFixture) releaseSlot(t *testing.T) { + t.Helper() + if err := f.store.ReleaseWorktreeExecution(f.req.Worktree, f.slot(t).ReservationToken); err != nil { + t.Fatalf("release settled slot: %v", err) + } +} + +// assertExecutingUnder asserts the worktree slot is executing under token AND +// that token is the named winner's own admission token — the winner's live slot. +func (f *siblingFixture) assertExecutingUnder(t *testing.T, winnerID, token string) { + t.Helper() + slot := f.slot(t) + if slot.State != admissionExecuting || slot.ReservationToken != token { + t.Fatalf("the winner's slot must stay executing under the adopted token: state %q, token kept=%v", + slot.State, slot.ReservationToken == token) + } + rec, err := f.store.Load(winnerID) + if err != nil { + t.Fatalf("load winner: %v", err) + } + if rec.AdmissionToken != token { + t.Fatalf("the winner must run under the adopted token") + } +} + +func setScopedAdmissionHook(t *testing.T, fn func(StartRequest)) { + t.Helper() + scopedAdmissionHook = fn + t.Cleanup(func() { scopedAdmissionHook = nil }) +} + +// TestStaleSuccessorLeavesSiblingAdoptedReservation pins change 0453's +// admitScoped failure leg: two successors present the same predecessor receipt on +// an epoch-less scope (admissions unserialized). S2 takes the worktree first — +// rotating P's executing slot, or freshly reserving a released one — to token T, +// then pauses before reserveScopeDrive (scopedAdmissionHook). A same-scope sibling +// adopts T, wins the scope slot, launches, and confirms T executing. S2's +// reserveScopeDrive then refuses ErrStalePredecessor, and S2 must NOT release T: +// the sibling's slot stays executing under T. Each subtest isolates one of +// siblingMayHoldReservation's signs. +func TestStaleSuccessorLeavesSiblingAdoptedReservation(t *testing.T) { + // runS2 starts S2 with P's receipt and asserts its admission reached the seam. + // hookOnce fires onAdmitted exactly once, at S2's own scopedAdmissionHook, with + // S2's worktree token (which it asserts is RESERVED). + runS2 := func(t *testing.T, f *siblingFixture, armed *bool) error { + t.Helper() + _, err := f.d.Start(f.successorOf(f.pred)) + if *armed { + t.Fatalf("S2's admission never reached the scope-reservation seam") + } + return err + } + hookOnce := func(t *testing.T, f *siblingFixture, armed *bool, onAdmitted func(token string)) { + setScopedAdmissionHook(t, func(r StartRequest) { + if !*armed || r.PredecessorDriveID != f.pred.DriveID { + return + } + *armed = false + slot := f.slot(t) + if slot.State != admissionReserved { + t.Fatalf("S2 must hold a RESERVED worktree slot at the seam, got %q", slot.State) + } + onAdmitted(slot.ReservationToken) + }) + } + + // P's receipt consumed by a sibling that adopted S2's token: both signs hold. + for _, tc := range []struct { + name string + releasePred bool // P's slot released before S2: S2 freshly reserves instead of rotating + }{ + {name: "rotated", releasePred: false}, + {name: "fresh", releasePred: true}, + } { + t.Run(tc.name+"/sibling-adopts-and-wins", func(t *testing.T) { + f := newSiblingFixture(t) + predToken := f.slot(t).ReservationToken + if tc.releasePred { + f.releaseSlot(t) + } + launches := f.proc.launchN + var s2Token string + var s1 DriveDoc + armed := true + hookOnce(t, f, &armed, func(token string) { + s2Token = token + // S1: the SAME receipt. It adopts S2's reserved token, wins the + // scope slot, retires P, launches, and confirms the slot executing. + s1 = f.startWaiting(t, f.successorOf(f.pred), "sibling S1") + }) + s2Err := runS2(t, f, &armed) + if !isOwnershipKind(s2Err, ErrStalePredecessor) { + t.Fatalf("S2 must lose the scope slot ErrStalePredecessor, got %v", s2Err) + } + if s2Token == predToken { + t.Fatalf("S2 must have taken its OWN token (rotated or fresh) before losing") + } + if f.proc.launchN != launches+1 { + t.Fatalf("exactly the winner S1 must launch, launched %d->%d", launches, f.proc.launchN) + } + f.assertExecutingUnder(t, s1.DriveID, s2Token) + }) + } + + // The scope has moved past the receipt's consumer: only the current drive's + // AdmissionToken shows the adoption. + t.Run("fresh/later-drive-adopts-and-wins", func(t *testing.T) { + f := newSiblingFixture(t) + f.releaseSlot(t) + armed := false + var c DriveDoc + // Between S2's precheck (P current) and its worktree admission, sibling C + // consumes P's receipt, completes, and releases its own slot. + f.git.onHead = func() { + c = f.startWaiting(t, f.successorOf(f.pred), "sibling C") + f.settlePassed(t, c) + f.releaseSlot(t) + armed = true + } + var s2Token string + var cNext DriveDoc + hookOnce(t, f, &armed, func(token string) { + s2Token = token + // C's successor adopts S2's fresh token and wins the scope slot. + cNext = f.startWaiting(t, f.successorOf(c), "C's successor") + }) + s2Err := runS2(t, f, &armed) + if !isOwnershipKind(s2Err, ErrStalePredecessor) { + t.Fatalf("S2 must lose the scope slot ErrStalePredecessor, got %v", s2Err) + } + scope, err := f.store.LoadScope(f.req.ScopeID) + if err != nil { + t.Fatalf("LoadScope: %v", err) + } + if scope.PriorDriveID == f.pred.DriveID { + t.Fatalf("precondition: the scope must have moved past P's consumer") + } + f.assertExecutingUnder(t, cNext.DriveID, s2Token) + }) + + // The receipt's consumer ran earlier on its own token; an adopter of S2's token + // is still on its way to the scope slot when S2 loses. Only PriorDriveID shows it. + t.Run("fresh/adopter-pending-when-s2-loses", func(t *testing.T) { + f := newSiblingFixture(t) + f.releaseSlot(t) + armed := false + var c DriveDoc + f.git.onHead = func() { + c = f.startWaiting(t, f.successorOf(f.pred), "sibling C") + f.settlePassed(t, c) + f.releaseSlot(t) + armed = true + } + type result struct { + doc DriveDoc + err error + } + adopted := make(chan struct{}) + proceed := make(chan struct{}) + done := make(chan result, 1) + var s2Token string + var early *result + setScopedAdmissionHook(t, func(r StartRequest) { + if r.PredecessorDriveID == c.DriveID && c.DriveID != "" { + // C's successor, in its own goroutine: it has ADOPTED S2's token and + // now pauses before its reserveScopeDrive. + adopted <- struct{}{} + <-proceed + return + } + if !armed || r.PredecessorDriveID != f.pred.DriveID { + return + } + armed = false + s2Token = f.slot(t).ReservationToken + next := f.successorOf(c) + go func() { + doc, err := f.d.Start(next) + done <- result{doc, err} + }() + select { + case <-adopted: + case r := <-done: + early = &r + } + }) + s2Err := runS2(t, f, &armed) + close(proceed) + if early != nil { + t.Fatalf("C's successor finished before adopting S2's token: %+v", *early) + } + if !isOwnershipKind(s2Err, ErrStalePredecessor) { + t.Fatalf("S2 must lose the scope slot ErrStalePredecessor, got %v", s2Err) + } + r := <-done + if r.err != nil { + t.Fatalf("the pending adopter must still launch on the token S2 left it: %v", r.err) + } + if r.doc.Outcome != WAITING { + t.Fatalf("the pending adopter must WAIT, got %s (%s)", r.doc.Outcome, r.doc.Cause) + } + f.assertExecutingUnder(t, r.doc.DriveID, s2Token) + }) +} + // TestBarrierSuccessorUnderCancel proves the successor path under a mid-flight // fence: a fenced successor start refuses without launching, and it leaves the slot // EITHER the predecessor's executing reservation (refused before rotation) OR From 6d39ed6a134624ff09ded028d6fa83084474e5b8 Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:57:09 -0400 Subject: [PATCH 5/7] fix(gatedrive): apply reserveScopeDrive's whole ordered predicate at the pre-rotation guard (change 0453) Review finding (minor, "duplicated-gate-copies-the-whole-predicate"): the pre-rotation guard in admitScopedWorktree copied only the final staleness clause of reserveScopeDrive's ordered predicate (capability, closed, half-filled receipt, reserved ErrScopeBusy, pending ack ErrUnresolvedLaunchTransition, then staleness). A closed scope was reported to a late stale successor as ErrStalePredecessor instead of ErrScopeClosed, and a receipt naming the current drive that failed an earlier clause (a half-filled receipt, a closed scope, a wrong capability) passed the guard and rotated the live slot before the authority refused it. Extract the ordered checks into one pure helper, scopeReserveRefusal(rec, childCapability, receipt, op), called by reserveScopeDrive under the scope lock and by the guard on its unlocked snapshot; the guard refuses on any non-nil result without touching the slot. Every clause is kept at the guard: none can turn away a successor the authority would admit (capability hash is immutable, close is one-way, the authority refuses a reserved or pending-ack slot whatever the receipt, and the scope never moves back). The fail-closed LoadScope error leg and siblingMayHoldReservation are unchanged. TestSameScopeSuccessorGuardAppliesWholeReservePredicate covers each earlier clause (closed, capability, reserved, pending ack with a stale receipt; a half-filled and a closed receipt naming the current drive): typed refusal, slot and scope record untouched, and parity with reserveScopeDrive's verdict on the same snapshot. All six subtests are red on the single-clause guard. TestSuccessorAdmissionFailureLegsReleaseRotatedSlot's post-rotation release leg now closes the scope through scopedAdmissionHook (after the rotation), since a scope closed before admission is now refused before rotating. --- .../gatedrive/admission_successor_test.go | 16 +- internal/gatedrive/driver.go | 31 ++-- internal/gatedrive/driver_concurrency_test.go | 142 ++++++++++++++++++ internal/gatedrive/scope.go | 98 ++++++++---- 4 files changed, 241 insertions(+), 46 deletions(-) diff --git a/internal/gatedrive/admission_successor_test.go b/internal/gatedrive/admission_successor_test.go index 2e3dddda9..65751f812 100644 --- a/internal/gatedrive/admission_successor_test.go +++ b/internal/gatedrive/admission_successor_test.go @@ -216,11 +216,13 @@ func TestLatePredecessorReleaseCannotFreeSuccessor(t *testing.T) { // reserveScopeDrive's staleness predicate BEFORE the rotation (change 0453): it is // refused ErrStalePredecessor with the executing slot unrotated. // -// A FRESH receipt whose scope closes between the unlocked precheck and admission -// (modelled through the epoch-gate seam, which runs after precheck and wraps the -// admission body) rotates the slot and is then refused ErrScopeClosed by the +// A FRESH receipt whose scope closes after the rotation but before the scope +// reservation (modelled through scopedAdmissionHook, which fires between worktree +// admission and reserveScopeDrive) is refused ErrScopeClosed by the // reserveScopeDrive authority: the rotated slot must be released, and the scope -// record stays byte-unchanged by the failed reservation. +// record stays byte-unchanged by the failed reservation. (A scope already closed +// when the guard reads it is refused before the rotation — +// TestSameScopeSuccessorGuardAppliesWholeReservePredicate.) func TestSuccessorAdmissionFailureLegsReleaseRotatedSlot(t *testing.T) { clk := &fakeClock{now: startEpoch()} store := OpenStore(testsupport.TempDir(t)) @@ -280,9 +282,10 @@ func TestSuccessorAdmissionFailureLegsReleaseRotatedSlot(t *testing.T) { } // Post-rotation failure leg: a fresh receipt whose scope closes after the - // unlocked precheck. The gate closes the scope, then runs the admission body. + // rotation. The hook fires once worktree admission (the rotation) has run and + // closes the scope before reserveScopeDrive arbitrates it. var scopeClosed []byte - d.SetEpochLaunchGate(func(_, _ string, reserve func() error) error { + setScopedAdmissionHook(t, func(StartRequest) { if err := store.scopeCAS(req.ScopeID, func(rec *scopeRecord) error { rec.Closed = true return nil @@ -290,7 +293,6 @@ func TestSuccessorAdmissionFailureLegsReleaseRotatedSlot(t *testing.T) { t.Fatalf("close scope mid-admission: %v", err) } scopeClosed = readScopeBytes(t, store, req.ScopeID) - return reserve() }) fresh := req fresh.PredecessorDriveID = cur.DriveID diff --git a/internal/gatedrive/driver.go b/internal/gatedrive/driver.go index aec0d5364..42cf13c5a 100644 --- a/internal/gatedrive/driver.go +++ b/internal/gatedrive/driver.go @@ -1084,10 +1084,12 @@ func (d *Driver) launchScoped(t *AdmissionTicket, claim *relaunchClaim) (DriveDo // OWN fresh reservation — a new ReservationToken and bumped ExecutionGen — so the // predecessor's stale token can never free or poison the successor's slot, and the // successor confirms and owns its own post-launch failure legs (ownsSlot=true). -// Rotation additionally requires the presented receipt to still name the -// scope's CURRENT drive (reserveScopeDrive's own staleness predicate, applied +// Rotation additionally requires the scope to still admit the presented receipt +// (reserveScopeDrive's own ordered predicate, scopeReserveRefusal, applied // before the mutating step): a successor whose predecessor was already -// superseded is refused typed ErrStalePredecessor without touching the slot. A +// superseded is refused typed ErrStalePredecessor, and one refused by an earlier +// clause (a closed scope, a busy slot, ...) gets that clause's typed refusal, +// all without touching the slot. A // RECEIPT-LESS first start that finds a same-scope executing slot has raced an // already-launched drive and is refused typed ErrScopeSecondDrive without touching the // slot. A slot held by a DIFFERENT scope, or in a stopping/unresolved state, is a @@ -1129,13 +1131,17 @@ func (d *Driver) admitScopedWorktree(req StartRequest) (token string, reservedFr if req.PredecessorDriveID == "" { return "", false, false, false, nil, ownershipErr(ErrScopeSecondDrive, "start") } - // The receipt must still name the scope's CURRENT drive before the one - // mutating admission step (the rotation) runs. reserveScopeDrive stays - // the authority for the scope slot — this is its own staleness predicate - // (receipt drive id vs the scope's CurrentDriveID) evaluated earlier, so - // a second successor holding a retired predecessor's receipt never - // rotates a live slot that its inevitable ErrStalePredecessor cleanup - // would then release (change 0453). This unlocked read excludes only a + // The scope must still admit this successor before the one mutating + // admission step (the rotation) runs. reserveScopeDrive stays the + // authority for the scope slot — this evaluates its WHOLE ordered + // predicate (scopeReserveRefusal: capability, closed, receipt shape, + // reserved slot, pending ack, then staleness) earlier, on an unlocked + // snapshot, so a second successor holding a retired predecessor's + // receipt never rotates a live slot that its inevitable + // ErrStalePredecessor cleanup would then release (change 0453), and a + // condition the authority checks before staleness (a closed scope, say) + // surfaces its own typed refusal rather than ErrStalePredecessor. Any + // refusal leaves the slot untouched. This unlocked read excludes only a // successor that arrives AFTER a sibling launched on the same receipt: // for that ordering, reading the scope after the slot read suffices, // because a slot executing under a successor's token was confirmed only @@ -1152,8 +1158,9 @@ func (d *Driver) admitScopedWorktree(req StartRequest) (token string, reservedFr if serr != nil { return "", false, false, false, nil, serr } - if scope.CurrentDriveID != req.PredecessorDriveID { - return "", false, false, false, nil, ownershipErr(ErrStalePredecessor, "start") + receipt := predecessorReceipt{DriveID: req.PredecessorDriveID, OwnerGen: req.PredecessorOwnerGen} + if refusal := scopeReserveRefusal(scope, req.ChildCapability, receipt, "start"); refusal != nil { + return "", false, false, false, nil, refusal } // A same-scope successor continues over the executing slot: rotate it to // this start's OWN fresh reservation rather than reusing the predecessor's diff --git a/internal/gatedrive/driver_concurrency_test.go b/internal/gatedrive/driver_concurrency_test.go index 1f741a9b9..d0619e46a 100644 --- a/internal/gatedrive/driver_concurrency_test.go +++ b/internal/gatedrive/driver_concurrency_test.go @@ -1,6 +1,7 @@ package gatedrive import ( + "bytes" "errors" "fmt" "os" @@ -1679,6 +1680,147 @@ func TestSameScopeSuccessorScopeReadFailureFailsClosed(t *testing.T) { } } +// TestSameScopeSuccessorGuardAppliesWholeReservePredicate pins that the +// pre-rotation guard in admitScopedWorktree applies reserveScopeDrive's WHOLE +// ordered predicate (scopeReserveRefusal), not only its final staleness clause +// (change 0453 review finding): a scope condition that the authority checks +// BEFORE staleness must surface its own typed refusal — never be masked as +// ErrStalePredecessor — and a receipt that names the current drive but fails an +// earlier clause must be refused before the rotation. Every case refuses with +// S1's executing reservation and the scope record untouched, and the guard's +// verdict equals the reserveScopeDrive authority's on the same scope snapshot. +func TestSameScopeSuccessorGuardAppliesWholeReservePredicate(t *testing.T) { + // setScope edits the stored scope record directly (an out-of-band state). + setScope := func(t *testing.T, store *Store, scopeID string, fn func(*scopeRecord)) { + t.Helper() + if err := store.scopeCAS(scopeID, func(rec *scopeRecord) error { + fn(rec) + return nil + }); err != nil { + t.Fatalf("mutate scope: %v", err) + } + } + cases := []struct { + name string + // mutate edits the scope record (and may edit S2's request) after S1 is + // executing; s1ID is the scope's current drive. + mutate func(t *testing.T, store *Store, req *StartRequest, s1ID string) + want OwnershipErrorKind + }{ + { + name: "closed scope with a stale receipt is ErrScopeClosed", + mutate: func(t *testing.T, store *Store, req *StartRequest, _ string) { + setScope(t, store, req.ScopeID, func(rec *scopeRecord) { rec.Closed = true }) + }, + want: ErrScopeClosed, + }, + { + name: "capability mismatch with a stale receipt is ErrScopeCapabilityMismatch", + mutate: func(_ *testing.T, _ *Store, req *StartRequest, _ string) { + req.ChildCapability = "not-the-scope-capability" + }, + want: ErrScopeCapabilityMismatch, + }, + { + name: "reserved scope slot with a stale receipt is ErrScopeBusy", + mutate: func(t *testing.T, store *Store, req *StartRequest, _ string) { + setScope(t, store, req.ScopeID, func(rec *scopeRecord) { rec.CurrentDriveState = scopeStateReserved }) + }, + want: ErrScopeBusy, + }, + { + name: "pending ack with a stale receipt is ErrUnresolvedLaunchTransition", + mutate: func(t *testing.T, store *Store, req *StartRequest, _ string) { + setScope(t, store, req.ScopeID, func(rec *scopeRecord) { + rec.PendingAckDriveID = "unretired-predecessor" + rec.PendingAckOwnerGen = "unretired-generation" + }) + }, + want: ErrUnresolvedLaunchTransition, + }, + { + name: "half-filled receipt naming the current drive is ErrStalePredecessor before rotation", + mutate: func(_ *testing.T, _ *Store, req *StartRequest, s1ID string) { + req.PredecessorDriveID = s1ID + req.PredecessorOwnerGen = "" + }, + want: ErrStalePredecessor, + }, + { + name: "closed scope with a receipt naming the current drive is ErrScopeClosed before rotation", + mutate: func(t *testing.T, store *Store, req *StartRequest, s1ID string) { + setScope(t, store, req.ScopeID, func(rec *scopeRecord) { rec.Closed = true }) + req.PredecessorDriveID = s1ID + req.PredecessorOwnerGen = "any-generation" + }, + want: ErrScopeClosed, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + clk := &fakeClock{now: startEpoch()} + store := OpenStore(testsupport.TempDir(t)) + d := scopedTestDriver(store, clk, &fakeProc{}, stableGit()) + _, req := prepareScopedStart(t, store) + + // P launches and settles PASSED; S1 presents P's receipt, rotates, and + // leaves the worktree slot executing under its own token. + first, err := d.Start(req) + if err != nil { + t.Fatalf("predecessor Start: %v", err) + } + if err := store.ownerCAS(first.DriveID, func(r *driveRecord) error { + r.LastOutcome = PASSED + return nil + }); err != nil { + t.Fatalf("settle predecessor terminal: %v", err) + } + succ := req + succ.PredecessorDriveID = first.DriveID + succ.PredecessorOwnerGen = first.Generation + s1, err := d.Start(succ) + if err != nil { + t.Fatalf("successor S1 Start: %v", err) + } + before, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution: %v", err) + } + if before.State != admissionExecuting { + t.Fatalf("precondition: S1's slot must be executing, got %q", before.State) + } + + // S2 keeps P's (now-stale) receipt unless the case rewrites it. + s2 := succ + tc.mutate(t, store, &s2, s1.DriveID) + scopeBefore := readScopeBytes(t, store, req.ScopeID) + + _, _, _, _, _, aerr := d.admitScopedWorktree(s2) + if !isOwnershipKind(aerr, tc.want) { + t.Fatalf("the guard must refuse %v (reserveScopeDrive's ordered verdict), got %v", tc.want, aerr) + } + after, _, err := store.LoadWorktreeExecution(req.Worktree) + if err != nil { + t.Fatalf("LoadWorktreeExecution after refusal: %v", err) + } + if after.State != admissionExecuting || after.ReservationToken != before.ReservationToken || after.ExecutionGen != before.ExecutionGen { + t.Fatalf("S1's executing reservation must be untouched: state %q->%q, token changed=%v, gen %d->%d", + before.State, after.State, after.ReservationToken != before.ReservationToken, before.ExecutionGen, after.ExecutionGen) + } + if !bytes.Equal(scopeBefore, readScopeBytes(t, store, req.ScopeID)) { + t.Fatalf("a guard refusal must leave the scope record byte-unchanged") + } + + // Parity: the reserveScopeDrive authority, on the same snapshot, refuses + // with the same kind (and, refusing, writes nothing). + receipt := predecessorReceipt{DriveID: s2.PredecessorDriveID, OwnerGen: s2.PredecessorOwnerGen} + if rerr := store.reserveScopeDrive(req.ScopeID, s2.ChildCapability, "parity-probe", receipt); !isOwnershipKind(rerr, tc.want) { + t.Fatalf("parity: reserveScopeDrive must refuse %v on the same snapshot, got %v", tc.want, rerr) + } + }) + } +} + // hookGit is a GitSeam whose onHead fires once, on the next fingerprint's first // read. Start fingerprints AFTER precheckScopedStart and BEFORE worktree admission, // so a test uses it to land a sibling's whole start between the two. diff --git a/internal/gatedrive/scope.go b/internal/gatedrive/scope.go index f9fe86201..da8f2d51c 100644 --- a/internal/gatedrive/scope.go +++ b/internal/gatedrive/scope.go @@ -279,42 +279,23 @@ func (s *Store) LoadScope(id string) (scopeRecord, error) { // (reserved), records the predecessor as PriorDriveID, journals the pending ack, // and increments DriveCount. Retiring the predecessor's recovery authority is the // caller's journaled second half (Task 4 retirePredecessor + clearPendingAck). +// The ordered refusal predicate is scopeReserveRefusal, shared with the +// pre-rotation guard in admitScopedWorktree. func (s *Store) reserveScopeDrive(scopeID, childCapability, newDriveID string, receipt predecessorReceipt) error { return s.scopeCAS(scopeID, func(rec *scopeRecord) error { - if childCapability == "" || rec.ChildCapHash != capHash(childCapability) { - return ownershipErr(ErrScopeCapabilityMismatch, "reserve-scope-drive") - } - if rec.Closed { - return ownershipErr(ErrScopeClosed, "reserve-scope-drive") - } - if receipt.halfFilled() { - return ownershipErr(ErrStalePredecessor, "reserve-scope-drive") + if err := scopeReserveRefusal(*rec, childCapability, receipt, "reserve-scope-drive"); err != nil { + return err } if rec.CurrentDriveID == "" { - // Empty slot: only a first start (empty receipt) may fill it. - if !receipt.empty() { - return ownershipErr(ErrStalePredecessor, "reserve-scope-drive") - } + // Empty slot: a first start (scopeReserveRefusal admitted only an empty + // receipt here) fills it. rec.CurrentDriveID = newDriveID rec.CurrentDriveState = scopeStateReserved rec.DriveCount++ return nil } - // Occupied slot: a mid-transition state fails closed before the receipt is - // even considered, so a reservation in flight or an unretired predecessor is - // never overwritten. - if rec.CurrentDriveState == scopeStateReserved { - return ownershipErr(ErrScopeBusy, "reserve-scope-drive") - } - if rec.PendingAckDriveID != "" { - return ownershipErr(ErrUnresolvedLaunchTransition, "reserve-scope-drive") - } - if receipt.empty() { - return ownershipErr(ErrScopeSecondDrive, "reserve-scope-drive") - } - if receipt.DriveID != rec.CurrentDriveID { - return ownershipErr(ErrStalePredecessor, "reserve-scope-drive") - } + // Occupied slot: scopeReserveRefusal admitted only a successor whose receipt + // names the current launched drive with no pending ack. rec.PriorDriveID = rec.CurrentDriveID rec.CurrentDriveID = newDriveID rec.CurrentDriveState = scopeStateReserved @@ -325,6 +306,69 @@ func (s *Store) reserveScopeDrive(scopeID, childCapability, newDriveID string, r }) } +// scopeReserveRefusal is reserveScopeDrive's ordered refusal predicate as a pure +// function of one scope record snapshot: it returns the typed refusal +// reserveScopeDrive would give a start presenting childCapability and receipt +// against rec, or nil when that start would be admitted to the slot. The order is +// the authority's — capability, closed, receipt shape, then slot state — so the +// FIRST failing clause names the refusal: +// +// - a missing or wrong child capability is ErrScopeCapabilityMismatch; +// - a closed scope is ErrScopeClosed; +// - a half-filled receipt is ErrStalePredecessor; +// - an EMPTY slot admits only an empty receipt (else ErrStalePredecessor); +// - an OCCUPIED slot refuses a reserved (unconfirmed) current drive +// ErrScopeBusy, a journaled pending ack ErrUnresolvedLaunchTransition, an +// empty receipt ErrScopeSecondDrive, and a receipt naming a non-current drive +// ErrStalePredecessor. +// +// op names the calling operation in the returned error. reserveScopeDrive +// evaluates it under the scope lock (the authority); admitScopedWorktree +// evaluates the same whole predicate on an unlocked snapshot before its one +// mutating step (the successor rotation), so a condition the authority checks +// before staleness surfaces its own typed refusal there rather than being +// reported as ErrStalePredecessor, and a receipt naming the current drive that +// fails an earlier clause is refused before it rotates a live slot. No clause +// wrongly refuses a legitimate successor at that earlier point: the capability +// hash never changes after prepare, a close is one-way, the authority refuses a +// reserved or pending-ack slot whatever the receipt (and a drive still reserved in +// its scope has no durable result a successor could acknowledge), and the scope +// never moves back to an earlier drive — so a refusal on the snapshot never turns +// away a successor the authority would admit. +func scopeReserveRefusal(rec scopeRecord, childCapability string, receipt predecessorReceipt, op string) error { + if childCapability == "" || rec.ChildCapHash != capHash(childCapability) { + return ownershipErr(ErrScopeCapabilityMismatch, op) + } + if rec.Closed { + return ownershipErr(ErrScopeClosed, op) + } + if receipt.halfFilled() { + return ownershipErr(ErrStalePredecessor, op) + } + if rec.CurrentDriveID == "" { + if !receipt.empty() { + return ownershipErr(ErrStalePredecessor, op) + } + return nil + } + // Occupied slot: a mid-transition state fails closed before the receipt is + // even considered, so a reservation in flight or an unretired predecessor is + // never overwritten. + if rec.CurrentDriveState == scopeStateReserved { + return ownershipErr(ErrScopeBusy, op) + } + if rec.PendingAckDriveID != "" { + return ownershipErr(ErrUnresolvedLaunchTransition, op) + } + if receipt.empty() { + return ownershipErr(ErrScopeSecondDrive, op) + } + if receipt.DriveID != rec.CurrentDriveID { + return ownershipErr(ErrStalePredecessor, op) + } + return nil +} + // confirmScopeLaunch flips the slot's current drive from reserved to launched // once its process launch has been persisted, completing the visible half of a // start. It acts only on the matching current drive id: a mismatched id is a From ea79c0852766a0a11f4537b00c0e3c971c326186 Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:57:53 -0400 Subject: [PATCH 6/7] docs(results): change 0453 results --- ...ale-predecessor-receipt-can-sti-results.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md diff --git a/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md b/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md new file mode 100644 index 000000000..e95fbe564 --- /dev/null +++ b/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md @@ -0,0 +1,25 @@ +# Two successors sharing one stale predecessor receipt can still free a live worktree slot — Results + +**Human action:** No action is required before merge. One optional item is worth a look: a narrow leftover race on freshly reserved successor starts (see Known issues). + +## Outcome + +Before this change, a gate start that presented an out-of-date predecessor receipt could still take over a worktree slot that another start was actively running a test gate in. It then failed its own scope check and released the slot, so a third start could launch a second gate in the same worktree. That breaks the rule that one worktree runs one gate at a time. + +What changed, all in `internal/gatedrive`: + +- **Refuse before rotating.** Before rotating an executing same-scope slot, admission now loads the scope record and runs the same ordered checks the scope reservation uses. That covers capability, closed scope, half-filled receipt, busy, pending acknowledgement and staleness. Any refusal leaves the slot untouched. If the scope record can't be read, admission fails closed. The checks now live in one shared helper, `scopeReserveRefusal`, which both call sites use, so the two can't drift apart. That also answers the review's minor finding: callers now see the same typed refusal from both paths. +- **Don't free a slot a sibling adopted (review finding, important).** The review found a second interleaving the design didn't cover. It needs admissions that aren't serialized by the run-epoch lock, which means epoch-less scopes. In it, a stale start rotates first, a sibling adopts its reservation and launches, and then the stale start's cleanup releases the sibling's live slot. A successor that loses with `ErrStalePredecessor` now keeps its reservation when the reloaded scope shows a sibling consumed the same receipt, or when the current drive holds this token. An unreadable record also keeps it. Keeping a reserved slot is the fail-closed direction. +- An existing test (`TestSuccessorAdmissionFailureLegsReleaseRotatedSlot`) had asserted the old rotate-then-release behavior on a stale receipt. It was revised to assert the new refusal. Its original purpose, releasing a rotated slot when admission fails after rotation, is still exercised through a scope closed after the rotation by a test-only hook. + +## Verification performed + +- Every new test was confirmed red against the unfixed code before the fix went in: the stale-receipt rotation test, the fail-closed scope read, the sibling-adopted reservation test (4 subtests, each condition mutation-checked separately), and the whole-predicate guard test (6 subtests). +- `go test -count=1 ./internal/gatedrive/` passed after each commit. The relevant subsets also passed under `-race`. +- The full suite (`go run ./cmd/docket development test`) passed at the build head before review: 54/54 files. The build-evidence block in the PR records the full-suite result for the final head. + +## Known issues and follow-ups + +### Leftover check-then-release race for freshly reserved successors + +This needs an epoch-less scope, a successor start that reserved a released slot fresh (not by rotation), and a stall long enough for the scope to move two drives past the receipt. In that window, a new adopter can take its token between the start's check and its release. The effect would be the same one-gate-per-worktree breach, but the window is much narrower than the bug this change fixes. Rotated starts are not affected. The defect is suspected from code reading and has not been reproduced. Suggested next step: a human decides whether to capture a follow-up change that makes the release conditional under the scope lock. From ade8b517a62dde94c54834b5619948c6c18dbcae Mon Sep 17 00:00:00 2001 From: Daniel Hanold Date: Fri, 25 Sep 2026 05:58:08 -0400 Subject: [PATCH 7/7] docs(results): stamp change 0453 results backlink --- ...rs-sharing-one-stale-predecessor-receipt-can-sti-results.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md b/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md index e95fbe564..e08d7aaaa 100644 --- a/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md +++ b/docs/results/2026-09-25-two-successors-sharing-one-stale-predecessor-receipt-can-sti-results.md @@ -1,3 +1,6 @@ + +> ↩ **[Change 0453 — Two successors sharing one stale predecessor receipt can still free a live worktree slot](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0453-two-successors-sharing-one-stale-predecessor-receipt-can-sti.md)** + # Two successors sharing one stale predecessor receipt can still free a live worktree slot — Results **Human action:** No action is required before merge. One optional item is worth a look: a narrow leftover race on freshly reserved successor starts (see Known issues).