deferred-work.md: concurrent writers can lose entries, duplicate DW ids, and revert closures

[pbean]

Split out of the PR #284 review, where it came up on both sides: the reviewer raised it under Deferred observation ("concurrent ledger writers remain unsafe … I would track full concurrent-run safety separately"), and the remediation agreed it was pre-existing and out of scope there. Nothing here was introduced by #234 — that PR only added one more writer, and it did land the unique temp name that removed the newly-introduced temp-name collision. This is the remaining, older half.

The problem

Every mutation of deferred-work.md is an unlocked read-modify-write of the whole file: read the full text, edit it in memory, write the full text back. Nothing coordinates two writers, so the loser of any interleaving is not partially applied — it is erased, including edits it never touched.

There is no single-writer invariant to lean on. The ledger is deliberately shared:

• ProjectPaths.rebased (src/bmad_loop/bmadconfig.py:43-55) leaves an artifact dir configured outside the project tree unmoved, "shared, not per-checkout" — so every worktree of a scm.isolation = worktree run points at one ledger file.
• Nothing prevents a second engine. cmd_run (cli.py:725) gates on worktree_clean (760) but never checks for a live engine; the TUI's _guarded (tui/app.py:193-205) only warns ("launching another engine on the same project may conflict") behind a "launch anyway" confirm. The clean-tree gate is not an accidental mutex: under scm.isolation = worktree the main tree stays clean for the whole run, and in-place it is clean between stories.
• A sweep run is a separate engine over the same ledger, and it is the writer with the widest windows.

The writers

Site	Write	Hazard
deferredwork.append_entry (deferredwork.py:264,287)	path.write_text	next_seq(text) read at 264, write at 287
deferredwork.mark_done_many (deferredwork.py:193,203)	atomic_write_text	read at 193, replace at 203
deferredwork.append_decision (deferredwork.py:217,223)	path.write_text	read/write, non-atomic
Engine._defer (engine.py:2977-2989)	write_text(snapshot)	whole-file restore spanning _rollback_or_pause
Engine._restore_deferred_closes (engine.py:2271)	atomic_write_text(before)	whole-file restore of a pre-close snapshot
SweepEngine migration (sweep.py:788)	ledger.write_text(text)	whole-file rewrite after _safe_reset

Callers reach these from engine.py:1639 / engine.py:2919 (review-followup refiles), engine.py:2297 (declared closure, #234), sweep.py:911, sweep.py:1028, sweep.py:1254, and decisions.py:147,150.

Concrete failure modes

1. Duplicate DW ids, and one entry lost. Two engines refile a review finding at once. Both append_entry calls read the same text, both compute the same DW-<n> from next_seq, both write. The ledger ends up with one entry — the other engine's finding is gone, and its journal says it was filed. If the interleaving instead lands both (a later append over a stale read is not the only shape), sweep.py:273's duplicate DW ids validation fails the migration.

2. A closure silently reverts an append. mark_done_many reads at deferredwork.py:193; an append_entry from another engine lands; the atomic_write_text at 203 replaces the file with text that predates the append. The write is atomic, which is exactly why it is quiet — no torn file, just a vanished entry.

3. A whole-file restore erases another engine's work. _defer snapshots the entire ledger at engine.py:2977, runs _rollback_or_pause, then writes the snapshot back at 2989. That window contains a rollback and possibly a pause. Anything a concurrent engine appended inside it is reverted. _restore_deferred_closes has the same shape over a shorter window.

4. A reader sees a truncated ledger. append_entry and append_decision use path.write_text, which truncates before writing. A concurrent parse_ledger — the TUI (tui/data.py:903), verify.py:1489, cli.py:1021, any gate — can read a file mid-write and conclude entries are missing. verify.py:1482 describes gates re-reading from disk precisely to be authoritative; here that read is not.

Widening every window: SweepEngine._close_resolved (sweep.py:906-915) still loops mark_done per id, i.e. N separate read-modify-write cycles where mark_done_many does one. sweep.py:1028 and decisions.py:147,150 do the same across append_decision + mark_done.

The primitive already exists

platform_util.file_lock (platform_util.py:224-261) is a cross-platform exclusive advisory lock — fcntl.flock on POSIX, msvcrt.locking on Windows — released by the kernel when the holder dies, so there is no stale-lockfile scheme. Its docstring already states the rule this needs: lock a dedicated sibling file, never data swapped via atomic_replace, because the lock rides the open fd's inode.

It has unit tests (tests/test_platform_util.py:274-303) and zero production call sites. It was added by d946a2a "fix(mux): lock herdr sidecar read-modify-write cycles" — the identical bug class, fixed once already — and went unused when the herdr backend was extracted out-of-tree. The fix here is to point it at the second sidecar it was always going to be needed for.

Proposed fix

1. Add a locked mutation seam in deferredwork.py — a context manager taking the lock on a sidecar (deferred-work.md.lock, next to the ledger so it follows an external artifact dir) and doing read → edit → write inside it. Every mutating entry point (append_entry, append_decision, mark_done_many) goes through it.
2. Move the two whole-file restores (engine.py:2989, engine.py:2271) under the same lock. The _defer restore should hold it across snapshot-and-restore, or be narrowed so it no longer round-trips the entire file.
3. Bring sweep.py:788's migration rewrite under the lock.
4. Move append_entry / append_decision onto atomic_write_text so readers never observe a truncated file even without holding the lock, and so mode/symlink survive (same reason as 2c81215).
5. Collapse the remaining per-id mark_done loops (sweep.py:906-915, sweep.py:1028, decisions.py:147-150) onto the batch primitive.
6. Readers stay lock-free. atomic_write_text already gives them a consistent snapshot once (4) lands; blocking the TUI on a sweep's lock would be a worse trade.

Everything above is in-process-plus-cross-process by construction — flock/msvcrt are OS-level, so two bmad-loop processes are covered, not just two threads.

Non-goals

• Full concurrent-run safety. This issue is the ledger only. Sprint status, decisions, run state and the artifact dirs generally have their own exposure; scoping this to the ledger keeps it verifiable.
• Making concurrent runs a supported mode. The TUI warning stays. This makes the ledger survive a conflict, not endorse it.
• The inner LLM session's own appends. The dev/review session writes the ledger directly and will not take the lock. Orchestrator writes are sequenced against sessions today; if that changes it is a separate issue.

Acceptance criteria

• Two processes appending concurrently produce two entries with distinct ids, and neither is lost.
• A close interleaved with an append preserves both.
• A _defer restore interleaved with another writer's append does not revert it.
• No reader ever observes a truncated ledger (all writers atomic).
• Locks are held only around file I/O — never across a subprocess, a session, or a pause.
• Windows CI covers the locked paths (msvcrt.locking differs from flock: 1-byte region, ~10 s blocking retry surfacing contention as OSError).
• Exclusion is asserted with blocking=False rather than a sleep-based negative assertion, per the primitive's docstring.

[coderabbitai]

🔗 Related PRs

#103 - fix(platform): overwrite-safe deferred stash + ref-safe unit branch names [merged]
#113 - fix: degrade spec read faults instead of crashing the run [merged]
#152 - fix(worktree): detach kept worktree so branch_per=run survives a kept failure (#138) [merged]
#274 - fix(deferredwork): keep flat appends visible after canonical entries [open]
#277 - feat(adapters,engine,devcontract): deterministic missing-marker catch + repair [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[pbean]

Triage: standalone primary, bug + area:deferred-work.

Cross-linking the ledger family so the boundaries are clear:

• Auto-annotate deferred-work.md entries as resolved at story-close #234 / PR feat: auto-resolve deferred-work entries a story declares (closes #234) #284 (auto-resolve declared closures) — adds one more writer and fixed the
  temp-name collision it introduced. Nothing in this issue was introduced by that PR; this is the
  older half, and feat: auto-resolve deferred-work entries a story declares (closes #234) #284 should not wait on it.
• PR fix(deferredwork): keep flat appends visible after canonical entries #274 (keep flat appends visible after canonical entries) — a third open deferred-work.md
  defect, currently with no linked issue.
• Ingest sprint-status action_items into the sweep/deferred-work flow (stable id + ref) #251 (ingest sprint-status action_items) would add yet another writer, and is itself
  blocked on upstream BMAD-METHOD#2612.

Confirming the key premise of the proposal above, since it is the thing that makes this cheap:
platform_util.file_lock (src/bmad_loop/platform_util.py:224-261) already exists, is
cross-platform (fcntl.flock / msvcrt.locking), is released by the kernel on holder death so
there is no stale-lockfile scheme, has unit tests at tests/test_platform_util.py:274-303 — and
has zero production call sites. It was added for the herdr sidecar's read-modify-write cycles
(the identical bug class) and went unused when that backend was extracted out-of-tree. Its
docstring already carries the rule this needs: lock a dedicated sibling file, never data swapped
via atomic_replace, because the lock rides the open fd's inode.