advance takes the board lock before discovering a no-op, so an idempotent confirm replay can fail on contention

[pbean]

Follow-up residual from PR #726 (the #286 + #469 ledger/board cross-process serialization
program). Filed rather than fixed there because the remedy contradicts an invariant that PR
deliberately pinned with a test.

The defect

sprintstatus.advance acquires the board lock before _advance_locked discovers there is
nothing to write. An idempotent bmad-loop confirm replay against a story already at or
beyond target — previously a successful no-op — can therefore fail when the lock is
unavailable, because it acquires before learning it had no work to do.

Reported by codex on PR #726 and verified correct against the code:
#726 (comment)

This is the third instance of one class

The general shape: a lock taken for an operation that will not write turns a previously
successful no-op into a failure. PR #726 fixed the other two instances on exactly that
reasoning:

• archive_closed over a missing ledger (now skips the lock)
• empty mark_done_many / mark_open_many batches (now skip the lock)

Consistency argues for fixing this site too. A fix should sweep the class rather than patch
this one report — a reviewer found the same shape at two sites across consecutive rounds, so
a per-report patch is likely to leave another instance standing.

Why it was not fixed in #726

The suggested remedy — probe for the already-at-target and absent-story cases before
acquiring — puts a board read above the lock. story_status goes through load, and
test_advance_holds_the_lock_across_every_read_and_the_write asserts
events[0] == "lock-enter" with events[1:-1] == ["load", "load", "write"].

That assertion is not incidental. It encodes #726's decision that all three reads move
under the hold, which is what closes the intra-call TOCTOU between the never-regress
decision and the bytes that decision is applied to. A pre-lock probe reopens a read outside
the lock and reddens that test by construction.

The actual trade-off

Either:

1. Keep every read under the hold and accept that a no-op advance can fail when the lock
   is unavailable (status quo), or
2. Admit an advisory pre-lock probe as an early-out, with the authoritative never-regress
   decision still made under the lock, and relax the pinned ordering to "the reads that
   decide the published bytes are inside the lock".

Option 2 is defensible — the probe would be advisory only, and the published bytes would
still be decided under the hold — but it changes a reviewed decision and requires rewriting
the test that protects it. That is a design call, not a repair, which is why it is here
rather than in #726.

Related: #726, #286, #469, #735.

[pbean]

Planning complete (2026-08-26, session-program). Decisions:

• Full class sweep, per this issue's mandate:
  • sprintstatus.advance: an advisory pre-lock probe (absent row → None; already at/past target → current status) with the authoritative never-regress decision still made under the hold. The pinned ordering test is rewritten to the relaxed invariant: the reads that decide the published bytes are inside the lock. Also cures the StateRootError-on-no-op failure mode (lock_path_for is only reached when there is work).
  • Missing-ledger check-before-acquire guards (the archive_closed pattern) for record_decision, _mark_done_many, mark_open_many.
  • Advisory pure-applier probes for the read-dependent no-ops: entry-not-found decisions, all-ids-already-done closes/reopens, every-spec-dedupes appends, nothing-eligible archives. One pre-lock read runs the same pure decision logic the locked pass runs; only a "would write nothing" answer is acted on (the call linearizes at the probe read); anything else — including any probe fault — falls through to the locked path unchanged.
• Declined, with rationale recorded: archive_closed's eligible dry_run stays under the hold (deliberate one-code-path design; only the nothing-eligible case joins the probe class); the CAS restores' under-lock skips stay (their acquisition is load-bearing for the write decision — their anchor defect is _restore_ledger: the reset_owned CAS anchor trusts a post-reset observation and can overwrite a concurrent writer #735, the other half of this PR).
• Governing principle: a probe may authorize a no-op answer, never a write.

Lands with #735 as one PR off main (post-#726) on branch pbean/noop-lock-blob-anchor-735-736; phases 1–2 of the 5-phase plan cover this issue. Plan: ~/.claude/plans/mighty-bubbling-badger.md (local).

[pbean]

Phase 1 of 5 landed — sprintstatus.advance's advisory pre-lock probe.

Branch pbean/noop-lock-blob-anchor-735-736 (off origin/main @ c308ae5b), commit 94de03e1.

What landed

• New _row_at_or_past(current, target) — the three-clause never-regress comparison that was inlined in _advance_locked, factored to module level so the probe and the authoritative locked decision run one body and cannot drift. Deliberately not engine._at_or_past: that reader-side twin counts an exact match outside STATUS_ORDER as reached, which is right for reading advance's return and wrong as input to its write decision — that case's no-op is owned by _set_mapping_value under the lock (verified: it returns False on a value it already holds).
• advance now runs one advisory read between the missing-board guard and the acquisition, answering the two cases that would write nothing: an absent row (None) and a row already at or past target (the current status). Everything else — including any exception raised while probing — falls through to the lock, which re-reads and decides authoritatively. The try encloses only the probe.
• _advance_locked uses the shared helper; its body is otherwise untouched.
• Docstrings record the relaxed invariant: every read that decides the published bytes stays inside the hold; a probe may authorize a no-op answer, never a write, and such a call linearizes at the probe read.

What this fixes

An idempotent replay no longer risks a failure it had no work to earn: the confirm replay against an already-done board (ParkedStory.resumable accepts it by design), and _carry_board_advance's routine no-op on a tracked board, no longer contend on the lock — nor fail on a StateRootError from lock_path_for, which is not an OSError and escapes on its own taxonomy. Write arms still fail loudly.

An audit of all four advance call sites (engine.py ×3, cli.py:3179) confirms the early returns are values those callers already handle: two discard the return, engine.py:6957 reads it through _at_or_past, and cli.py:3179 tests != "done" — which an at-or-past echo can only satisfy. advanced_bytes calls _advance_locked directly and is unaffected.

Tests

tests/test_sprintstatus_advance.py: 6954 → 6959 passed, 49 skipped (full suite, -n logical). pyright 0 errors; trunk check --all --no-fix clean.

The pinned ordering test was rewritten rather than deleted — test_advance_holds_the_lock_across_every_read_and_the_write → test_the_reads_that_decide_the_published_bytes_are_inside_the_lock (the old name is in its docstring for greps). It now pins exactly one advisory read before lock-enter and the three deciding events inside, so a genuine lock-hoisting regression stays distinguishable from the intended change. Five new rows: at-or-past takes no lock, absent row takes no lock, a probe-satisfied no-op survives an underivable state root (with the write arm still raising), a malformed board falls through and raises from under the lock, and the authoritative never-regress decision is made under the lock against a rival that moved the row past target.

Every negative assertion was ablation-proven — five ablations run singly against cp backups, each graded on its intended oracle. Two worth recording: on the malformed-board row the acquisition count is the oracle, not the raise (pytest.raises alone survives removing the try/except, since the probe's own uncaught error is the same class from the same reader); and threading the probe's answer into _advance_locked reddens the racing row with a real board regression, which is the guarantee that read is there to provide.

Phases 2–5 (the deferredwork class sweep, and #735's blob-derived reset anchors) follow on this branch; the PR opened in phase 5 carries the closing keywords for both issues.

[pbean]

Progress — S2 of 5: the deferredwork class sweep (branch pbean/noop-lock-blob-anchor-735-736, not yet a PR).

S1 fixed sprintstatus.advance. This session sweeps the same class through deferredwork.py, in two commits.

defb1238 — pure extraction, zero behavior change. Each locked mutator folded its decision inline, so it only existed inside the hold. Extracted to _apply_done_many, _apply_open_many, _apply_appends and _eligible_for_archive, and rewired the locked bodies through them. _apply_done_many carries undo_owner deliberately: the reopenable arm's LINE_BREAK refusal is part of the decision, so a hand-rolled scan for open entries would answer differently. The reads and the atomic_write_text call sites did not move.

3dd1b56c — the guards and probes, all above the lock and below every validation:

• Missing-ledger is_file guards on _mark_done_many, mark_open_many and record_decision — the pattern archive_closed already kept. The rechecks under the hold stay, creation being able to race the answer. append_entries_published deliberately gets none: an absent ledger there means CREATE, which is a write, and a write takes the lock. That sentence is now in its docstring.
• One advisory pre-lock read per mutator, running the same pure helper the locked pass runs, so probe and authority cannot drift. Only a "would write nothing" answer is acted on, and such a call linearizes at the probe read — it publishes no bytes, so there is nothing for a rival to interleave with. Every other answer, and any fault during the probe, falls through to the hold, which re-reads and decides authoritatively. Sites: every id already done or missing; no id eligible to reopen; a decision on an entry that is not there; every spec deduping; nothing eligible to archive.

archive_closed's probe sits above the dry_run branch, so a nothing-eligible dry run skips the lock too. An eligible dry run still runs under the hold — that one-code-path design is deliberate and stays.

Released-behavior delta: bmad-loop sweep --archive with nothing eligible, under a dead lock or where no state root can be derived, goes from rc 1 ("may hold its ledger lock") to rc 0 ("no closed entries to archive"). An eligible archive under a dead lock still fails exactly as before, and every write arm still fails loudly.

Tests. NOOP_MUTATORS — the deliberate inverse of LOCKED_MUTATORS, every row seeded to write nothing — drives test_a_read_dependent_noop_takes_no_lock and test_a_noop_mutation_succeeds_when_no_state_root_is_derivable across all ten public entry points, plus test_mutators_take_no_lock_for_a_missing_ledger.

The one worth naming: test_a_failing_probe_read_falls_through_to_the_locked_path faults only the probe read, so the same no-op inputs reach the hold. That is what keeps the under-lock no-write guards ablation-provable now that the probe answers those inputs first — two existing tests had documented ablations that would otherwise have silently gone green, and both were rewritten to say so and point at it.

18 ablations run singly, each reddening its intended oracle: 5 probe deletions, 5 except removals, 5 under-lock guard deletions, 3 is_file guard removals. No green ablation.

Full suite 6959 → 6987 passed / 49 skipped, pyright 0, trunk check --all --no-fix clean.

Remaining in the program: S3–S5 cover #735 (blob-derived reset anchors) and then the PR, which will close this issue and #735 together. Leaving this open until then.

[pbean]

Session 5/5 — docs, audits and the PR. The class sweep this issue asks for is complete and up for review in #737 (ready for review; it closes this issue and #735 on merge — not merged, that is the maintainer's call).

What landed for #736 across phases 1–2:

• sprintstatus.advance answers an absent row and a row already at or past target from one advisory pre-lock read, through the new module-level _row_at_or_past that _advance_locked also uses.
• deferredwork gets the same treatment at five sites — _mark_done_many, mark_open_many, record_decision, append_entries_published, archive_closed — each probe running the pure decision helper its locked body runs, extracted first in a zero-behavior-change commit so probe and authority cannot drift.
• Missing-ledger is_file guards above the lock at three of those; append_entries_published deliberately gets none, because an absent ledger there means CREATE.

The pinned test this issue names was renamed, not deleted: test_advance_holds_the_lock_across_every_read_and_the_write → test_the_reads_that_decide_the_published_bytes_are_inside_the_lock, with the old name kept in its docstring for greps. Its assertion moves from events[1:-1] == ["load", "load", "write"] to events[:enter] == ["load"] — exactly one advisory read — plus the unchanged deciding-reads-and-write segment inside the hold. That is option 2 from the issue's trade-off, taken deliberately: the invariant is relaxed to "the reads that decide the published bytes are inside the lock", never wider.

Declined with rationale, both recorded in the PR: archive_closed's eligible dry_run stays under the hold (the one-code-path design; only the nothing-eligible case is a probe answer, and the eligible path is verified byte-identical to main), and the CAS restores' under-lock skips stay, since their acquisition is load-bearing for the write decision that follows.

Released-behavior delta: bmad-loop sweep --archive with nothing eligible, under a dead lock or where no state root can be derived, goes rc 1 → rc 0. An eligible archive under a dead lock still fails exactly as before, as does every write arm.

Audits: an AST call-graph over src/bmad_loop reports 11 lock spans, 0 reaching a subprocess spawn — believed only because the checker first fired on a sensitivity control (_ledger_is_gits_to_restore → verify.path_tracked → _run_git → subprocess.run). Suite 6954 → 7000 passed / 49 skipped, pyright 0, trunk check --all --no-fix clean.