diff --git a/AGENTS.md b/AGENTS.md index 90ef578d..89e4e97f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,7 +24,7 @@ Things that break silently. Never violate; when in doubt, read the named module' - No LLM calls in the orchestrator control loop. - Sessions complete only on hook Stop events or window death — never on LLM prose. Never add another completion path. Post-session state is re-verified deterministically (`verify.py`). -- `sprintstatus.advance()` is the orchestrator's sole write path to sprint-status.yaml (the dev skill flips only spec frontmatter; the engine mirrors it onto the board pre-verify). Legal phase transitions live only in `statemachine.py`. +- `sprintstatus.advance()` is the orchestrator's sole write path to sprint-status.yaml (internally serialized cross-process since #469; the dev skill flips only spec frontmatter; the engine mirrors it onto the board pre-verify). Legal phase transitions live only in `statemachine.py`. - All git subprocess calls in `src/bmad_loop` go through the `_run_git` chokepoint in `verify.py` (timeouts, `LC_ALL=C`) — no bare subprocess git; `tests/test_portability_guard.py` enforces it. Tests, `scripts/`, and CI workflows deliberately spawn their own git: a harness must not depend on the artifact it validates. - Every new policy field needs an entry in `src/bmad_loop/data/settings/core.toml` (a sync test enforces defaults/options match `policy.py`). New core env vars register in `envvars.py`; plugin-owned env-var families stay with their plugin. - Version strings are stamped only by `scripts/sync_version.py` from `src/bmad_loop/__init__.py` — never hand-edit pyproject.toml, module.yaml, marketplace.json, or uv.lock versions. diff --git a/CHANGELOG.md b/CHANGELOG.md index dc31b485..e55a57fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,19 @@ breaking changes may land in a minor release. block that holds its body. Refuses while any engine run is live. Pure deterministic Python — no LLM involvement. +- **Batched deferred-work ledger primitives** (#286, #469). `append_entries`, + `mark_open_many`, `record_decision` and `mark_done_many`'s per-id `notes=` each collapse a + sequence that used to be one write per row — or one write per half of an append-then-close + pair — into a single read-modify-write. Each is byte-identical to the serial sequence it + replaces, validating the whole batch before it takes the lock, minting sequential ids and + deduplicating in-call twins exactly as the loop it stands in for did, so a caller adopting one + changes how many windows it leaves open and nothing else. `append_entries_published` + additionally hands back the text it wrote, so a caller that has to record what it published — + rather than what the file happens to hold afterwards — takes its anchor from inside the hold + instead of reading the ledger back once the lock is gone. An empty batch takes no lock at + all, matching the per-id loop it replaces, so a caller that batches nothing cannot begin + failing on a lock it never needed. + ### Changed - **A published run archive now lands at mode `0600`** instead of a umask-derived mode (#591). @@ -117,6 +130,109 @@ breaking changes may land in a minor release. triage never passes `validate_triage`, and a fresh triage can renumber the option it named — applied by journaled discard rather than by error: the build decision is honored under the always-legal `decision-` fallback name. +- **Deferred-work ledger mutators serialize on a cross-process lock** (#286, #469). Every + mutator was an unlocked read-modify-write of the whole file, so two orchestrator processes — + a second `bmad-loop run`, a run plus a sweep, a run plus the TUI decision modal, a run plus + `sweep --archive` — both read, both edited, and the last atomic write won: entries lost, + closures silently reverted, and two appenders minting the same `DW-` because each read + `next_seq` from the text it had just read. Every mutator now holds an advisory lock across its + whole read-modify-write, and the orchestrator's remaining multi-write sequences adopt the + batched primitives above, so each is one locked pass rather than one open window per row. The + lock lives at `/locks/-.lock`, out of the repository rather than + beside the ledger, because the ledger is tracked by design and the engine stages with + `git add -A`; it is keyed on the resolved path, so every spelling of one file contends on one + lock. Readers stay lock-free — every writer already replaced the file atomically, so a reader + sees one whole version or another. Nested acquisition raises instead of self-deadlocking, and a + lock that cannot be taken fails the write rather than proceeding unlocked. The dev/review + session's own ledger writes are unchanged and still take no lock. `bmad-loop sweep --archive` + names the ledger lock in its failure message rather than printing a bare `errno` — as a + possibility rather than a verdict, since the same arm also catches the archive's own read + and write failures and a full disk must not send an operator hunting a rival process. And + `bmad-loop +decisions` and the TUI decision modal now also catch the state-root failure that deriving a + lock path can raise — in the TUI an uncaught one escaped into the Textual event loop and took + the dashboard down mid-walk. A project with no ledger at all is still answered without taking a + lock, so `--archive` keeps reporting it as the success it always was rather than failing + wherever no state root can be derived. `--archive`'s refusal while a run is live is + unchanged and deliberately kept: it is coarser than the lock, refusing the archive rewrite outright rather + than merely serializing it. +- **`sprint-status.yaml` advances serialize on a cross-process lock** (#286, #469). Being the + board's sole writer was never mutual exclusion — a second orchestrator process runs that same + sole writer — and an advance is a read-modify-write of the whole board, so two of them both + read, both edited, and the last atomic write won: a story flipped by one run silently reverted + to its earlier status, and the run simply walked past it. `advance` now holds the board's own + sidecar lock across all three of its reads and its write, which also closes the gap inside a + single call between the never-regress decision and the bytes that decision was applied to. A + board that does not exist is still reported missing without creating a lock file at all, and a + lock that cannot be taken fails the advance on the channel that already carries its errors + rather than rewriting the board unserialized. Recomputing an advance for the isolated-run + ownership check runs the locked body directly against its private throwaway copy: it needs no + exclusion, nobody else being able to name that copy, and taking a lock anyway would strand one + more sidecar keyed on a path that exists only for that call, since lock files are never + reaped. The atomic, symlink-following, + read-only-refusing write itself is unchanged. +- **A failed commit now rolls back only the ledger entries the story itself closed** (#286). + The window between a story's declared `closes_deferred:` closure and its commit spans git + spawns and, on the escalation leg, a pause for a human, so it is long enough for another + writer to reach the same ledger — and the rollback used to rewrite the whole document from + the pre-close text, taking whatever had arrived with it: an entry another process filed + vanished (and its `DW-` was handed out again), and a closure someone else had verified + silently reverted to `open`. A story close is therefore written the way a sweep bundle's + has been since #284, with a durable undo marker owned by that close, and the rollback + reopens exactly those entries in one locked read-modify-write. Concurrent appends, closes + and recorded decisions are left standing. An armed entry whose marker has since been + displaced — a foreign line inserted between the status and its marker breaks the pairing — + is left `done` with the foreign content intact and journaled as + `deferred-close-reopen-unmatched`, rather than overwritten around; `deferred-close-rolled-back` + now names the ids it reopened. The rollback stays advisory: it still never raises out of + the failure arm it runs inside, so the commit's own escalation remains the disposition. + **Ledger format:** a story close now leaves a permanent + `resolution-undo: ` line beside its `resolution:` line, in the same + committed ledger — the format `sweep` bundle closes already publish, now used by one more + writer. Readers that ignore unknown fields are unaffected; `bmad-loop sweep --archive` + already preserves the line. +- **A rolled-back defer no longer restores its ledger over a writer that arrived during + the rollback** (#286). The three ledger-restore windows that remain all span `git reset --hard` + and its preflight spawns, so a lock must not cover them; each is instead compare-and-set + against the ledger as observed the instant the rollback returned. The defer restore is also + gated on git owning the file, which inverts the old guard's worst case: on an untracked or + external ledger — the one kind `reset --hard` cannot have touched — every difference from the + snapshot was by definition somebody else's write, and rewriting the file on exactly that + difference is what destroyed it. Such a ledger is now left alone. When the text does move + between the observation and the lock, the snapshot is republished by APPENDING the entries disk + has since lost, keyed by DW- id and carrying their bodies verbatim, so a concurrent append + survives the restore instead of being rolled back with it; `defer-ledger-restore-diverged` + names the ids moved. Flat appender blocks, which belong to no canonical entry, are reported + rather than guessed at (`flat_remainder`) — the merge never invents a boundary the parser does + not model. Entries are matched by id AND body, so an id the reset removed and a rival then + re-minted for an entry of its own is reported as an `id_collisions` conflict rather than + silently accepted as already-there: matching on the id alone dropped the very entry the + merge exists to carry, and re-appending it would publish a duplicate `DW-` instead. A + write or lock fault still propagates, as the unguarded write always did. +- **A failed legacy-ledger migration refuses to restore over a ledger that changed + underneath it** (#286). The sweep's post-reset rewrite of the pre-migration text had the + same unguarded window; on a difference it now journals `sweep-migration-restore-diverged` and + escalates for a human to re-run the sweep rather than writing. Deliberately no merge and + no silent skip: leaving the rejected rewrite standing is the "never re-prompt over a + half-broken rewrite" failure the restore exists to prevent, and a migration input that + moved after the attempt was graded against it is a human problem — the same call the + duplicate-id refusal already makes. +- **A rejected attempt's ledger retraction no longer overwrites, or deletes, a concurrent + writer's work** (#286). This restore compares against two anchors — a persisted digest of the + bytes the engine itself last published, and the ledger as observed the instant the rollback + returned when git owns the file and its `reset --hard` republished it. Matching neither means + the text is somebody else's, and the restore degrades to a journaled + `ledger-restore-skipped-diverged` skip rather than a write. Deliberately no merge: a + retraction cannot be expressed as an append. The skip is the safe direction — the + harvest entries left standing are real findings, `append_entry`'s idempotence stops the + next attempt filing them twice, and a non-restored ledger already reads as "changed" to + the attribution rebase, which stands the harvest exclusion down and exposes more of the + tree to the proof-of-work gate. The `snapshot is None` arm, which retracts a ledger the + harvest itself created, is gated on the same digest, closing a latent data loss: it + previously deleted whatever it found at that path, so a ledger a concurrent writer had + created inside the window went with it. A tracked ledger absent at snapshot time is + still never deleted, and is now answered before any lock is taken. A write or lock fault + is journaled as `ledger-restore-failed` and preserves an in-flight pause, as before. ### Security diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 72b5925f..c5c6c5d0 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -36,6 +36,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Automated per-story pipeline: `dev → verify → review → verify → commit`, end-to-end, no human in the loop. - Deterministic control flow in plain Python — story selection, retry budgets, gate checks, and completion checks are code, not an LLM session. - Owns `sprint-status.yaml`, the single source of truth: `bmad-sprint-planning` generates it, and while a run is in flight the orchestrator is its sole writer (`sprintstatus.advance` — idempotent, never-regress) while the dev and review sessions it dispatches are told never to write or revert it; your own BMAD skill runs still edit the board outside a run. Selects the next `ready-for-dev` story; advances by epic/story. +- Two orchestrator processes can no longer interleave a board advance (#286/#469): `sprintstatus.advance` holds an advisory cross-process lock across all three of its reads and its write, so the never-regress decision and the bytes that decision is applied to can no longer be separated by somebody else's write. Being the board's sole write path was never mutual exclusion on its own — a second `bmad-loop` process runs that same sole writer. Readers stay lock-free, a board that does not exist is still reported missing without a lock being created at all, and a lock that cannot be taken fails the advance on the channel that already carries its errors rather than rewriting the board unserialized. Same sidecar mechanism as the deferred-work ledger, described under _Deferred-work sweeps_. - Scoping flags: `--epic N`, `--story KEY`, `--max-stories N`, `--dry-run` (prints the plan, spawns nothing). ### Spec + implementation (dev stage) @@ -135,6 +136,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Repeat mode (`--repeat` / `[sweep] repeat`): re-triages after each cycle to absorb newly generated deferred work, stopping when a cycle does nothing addressable or hits `max_cycles`. - Ledger archiving (`sweep --archive`, optionally `--before DATE` to bound by close date; `--dry-run` previews): moves closed (`status: done `) entries to sibling `deferred-work-archive.md` (body preserved, an `archived: ` marker appended), leaving an id-preserving stub (`status: done ` + `archived: `) so grep and `closes_deferred` cross-references keep resolving and the live ledger stays proportional to open work rather than all history. The stub keeps load-bearing field lines — `gate:` (validate's closed-gate report keeps speaking), `origin:`/`source_spec:` (the harvest-replay dedupe stays status-agnostic), and the reopenable-close undo tail (a paused sweep's bundle rollback still works). Reopening an archived stub — what that rollback does — demotes its `archived:` stamp to `archived-body:` rather than dropping it: the entry is live work again, so the stamp would be a lie and its shape would strand the entry outside every future archive, but the body its close moved out is still in the archive file and that line is what a later triage follows to it. Deterministic, no LLM, atomic writes with crash-safe ordering (archive before ledger; a retry keyed on id + close date completes the move without duplicate bodies). Refuses while any engine run is live or its liveness is unverifiable — it is the one out-of-band ledger writer. When the ledger is tracked, the move is durable only once both files are committed; a gitignored ledger — the default shape — or an artifact dir outside the repo has nothing to commit. Unrelated to `bmad-loop archive ` (run-tarball archiving). - Sweeps are their own resumable runs (`bmad-loop resume `). An escalated bundle resolves like a story escalation, including intent-gap patch-restore: `bmad-loop resolve --restore-patch ` re-arms the bundle spec to `in-review` and the re-driven bundle session resumes review on the re-applied patch instead of re-implementing. +- Ledger writes serialize across processes (#286/#469). Every orchestrator mutation of `deferred-work.md` — an append, a close, a reopen, a recorded decision, `sweep --archive`'s two-file rewrite — holds an advisory lock for its whole read-modify-write, so a second `bmad-loop run`, a run plus a sweep, or a run plus the TUI decision modal can no longer both read, both edit, and let the last atomic write win (lost entries, silently reverted closures, two appenders minting the same `DW-`); multi-row work is batched into one locked pass rather than one per row. The lock is a sidecar under the state root (`/locks/-.lock`), never beside the ledger, because the ledger is tracked by design and the engine stages with `git add -A`; it is keyed on the resolved path, so every spelling of one file contends on one lock while two worktrees' in-tree ledgers correctly get their own. Readers stay lock-free — every writer already replaces the file atomically, so a reader sees one whole version or another. The wait is platform-asymmetric: POSIX blocks, while Windows bounds it at roughly ten seconds and then surfaces contention as an error rather than proceeding unlocked. A dev or review session's own ledger writes are deliberately outside this — the orchestrator sequences its writes against the sessions it dispatches. The rollback paths that span a `git reset --hard` cannot be covered by a lock at all, so each is instead compare-and-set against the ledger as observed the instant the rollback returned, degrading to a journaled `defer-ledger-restore-diverged`, `ledger-restore-skipped-diverged` or `sweep-migration-restore-diverged` rather than writing over a concurrent writer; and a failed commit reopens exactly the entries the story itself closed, journaling `deferred-close-rolled-back` with their ids and `deferred-close-reopen-unmatched` for an entry whose undo marker a foreign edit has displaced. ### Stories mode (folder+id dispatch) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 27999463..f537be18 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -2281,6 +2281,15 @@ def _sweep_archive(project: Path, paths: bmadconfig.ProjectPaths, args: argparse was removed still owns its ``engine.pid`` and still writes this ledger, and the gated view would report it as no run at all. An unreadable runs root answers nothing, so it refuses too — same conservative side.""" + # The pid-liveness gate below is now belt-and-braces: `archive_closed` takes + # the cross-process ledger lock beneath it (#286/#469), so a run that started + # after this check still cannot interleave its writes with the archive's. The + # gate STAYS, because its semantics are deliberately coarser than the lock's: + # the lock only serializes the two writers' read->edit->write cycles, while + # the gate refuses to rewrite the archive AT ALL while any run is live — + # including a run that would merely be surprised to find its open entries + # moved out from under a plan it has already read. Removing it would trade a + # refusal a human can act on for a race the lock does not cover. run_dirs = runs.all_run_dirs(project) if run_dirs is None: print( @@ -2315,6 +2324,27 @@ def _sweep_archive(project: Path, paths: bmadconfig.ProjectPaths, args: argparse except ValueError as exc: print(f"error: {exc}", file=sys.stderr) return ExitCode.FAILURE + except (OSError, runs.StateRootError) as exc: + # `archive_closed` serializes on the ledger's sidecar lock (#286/#469). + # THREE ways this arm is reached, not two: the acquisition raises + # `OSError` (a rival holder outlasting the blocking retry, or an + # unwritable locks dir); deriving the sidecar's path raises + # `runs.StateRootError` — NOT an OSError — when the environment names no + # usable state root; and the archive's own I/O raises `OSError` too, for + # the ledger read and for either atomic write. Naming the lock is what + # makes the message actionable — a bare `error: [Errno 11] ...` from a + # command with no other lock in sight reads as a bug in the archive — but + # the message must not ASSERT contention, or a full disk sends the + # operator hunting a rival process that was never there. So it names both + # possibilities and lets the carried cause decide between them. Existing + # FAILURE path, no new exit code, and `--archive` has no --json arm. + print( + f"error: cannot archive the deferred-work ledger ({exc}) — another " + "bmad-loop process may hold its ledger lock, or the ledger or its " + "archive could not be read or written", + file=sys.stderr, + ) + return ExitCode.FAILURE if not ledger.is_file(): print(f"no deferred-work ledger at {ledger}") return ExitCode.OK @@ -3228,7 +3258,7 @@ def cmd_decisions(args: argparse.Namespace) -> int: option = prompter.ask(decision) try: decisions.apply_pre_answer(project, decision, option, date=today) - except (OSError, bmadconfig.BmadConfigError, ValueError) as e: + except (OSError, bmadconfig.BmadConfigError, ValueError, runs.StateRootError) as e: # What this buys is the `{decision.id}` in the message, and only # that: `main`'s tail catches BmadConfigError by name and everything # else through a bare `except Exception`, so none of these ever @@ -3237,9 +3267,15 @@ def cmd_decisions(args: argparse.Namespace) -> int: # printed an outcome line per answered decision, and a bare # `error: ` after them does not say which one did not land. # - # ValueError is the ledger writers' `date` precondition, unreachable - # from the strftime above and here for the same reason as the TUI's - # copy of this call. BmadConfigError is the reachable one: + # The inventory: OSError covers the ledger and store writes and the + # ledger lock's own acquisition (#286/#469). StateRootError is that + # lock's other failure — deriving its state-root sidecar from an + # environment that names no usable root — and it is NOT an OSError, so + # leaving it out would let it fall through to `main`'s bare tail and + # lose the attribution this handler exists for. ValueError is the + # ledger writers' `date` precondition, unreachable from the strftime + # above and here for the same reason as the TUI's copy of this call. + # BmadConfigError is the reachable one: # `apply_pre_answer` re-reads the BMAD config on every call and # `prompter.ask` blocks on the human in between, so a config removed # or broken mid-prompt raises here even though the read at the top of diff --git a/src/bmad_loop/decisions.py b/src/bmad_loop/decisions.py index 21a47f9d..c358eb9c 100644 --- a/src/bmad_loop/decisions.py +++ b/src/bmad_loop/decisions.py @@ -175,11 +175,21 @@ def apply_pre_answer( paths = bmadconfig.load_paths(project) ledger = paths.deferred_work detail = option.resolution or option.intent - deferredwork.append_decision(ledger, decision.id, date, option.label, detail) + close_note = None if option.effect == "close": - note = "closed by human decision" + (f": {option.resolution}" if option.resolution else "") - deferredwork.mark_done(ledger, decision.id, date, note) - else: + close_note = "closed by human decision" + ( + f": {option.resolution}" if option.resolution else "" + ) + # ONE locked read->edit->write (#286/#469). As the `append_decision` + + # `mark_done` pair it was two acquisitions with a window between them, and a + # rival writer landing there left the entry carrying a decision that says + # "close it" over a status that still says open. The bytes are identical to + # the pair's. The commit below stays OUTSIDE any lock — locks are held only + # around file I/O, never across a subprocess (#286). + deferredwork.record_decision( + ledger, decision.id, date, option.label, detail, close_note=close_note + ) + if option.effect != "close": record_pre_answer(project, decision.id, option, date=date) if commit: try: diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index 0d6e8c05..a5d53141 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -12,21 +12,36 @@ canonical entries. The orchestrator never trusts an LLM to have edited it — status flips and decision records happen here, and gates re-read the file from disk. + +Concurrency (#286/#469): every mutator below is a read->edit->write of the whole +file, so two orchestrator processes — a second `bmad-loop run`, a run plus a +sweep, a run plus the TUI decision modal, a run plus `sweep --archive` — would +otherwise both read, both edit, and let the last atomic write win. Each leaf +mutator therefore runs its whole read->edit->write under :func:`ledger_lock`, a +cross-process mutex on an out-of-repo sidecar. Readers stay lock-free on +purpose: every writer replaces the file atomically, so a reader already sees one +whole version or another, and taking the lock to read would buy nothing while +adding a way to deadlock. Out of scope by #286's own non-goals: the dev/review +LLM session writes this file directly and does NOT take the lock — orchestrator +writes are sequenced against sessions today, so the exposure this closes is +orchestrator-vs-orchestrator. """ from __future__ import annotations import hashlib import re +import threading from bisect import bisect_right -from collections.abc import Sequence +from collections.abc import Iterator, Sequence +from contextlib import contextmanager from dataclasses import dataclass from datetime import date as calendar_date from pathlib import Path from . import sprintstatus from .fences import fenced_spans -from .platform_util import atomic_write_text, neutralize_surrogates +from .platform_util import atomic_write_text, file_lock, neutralize_surrogates HEADING_RE = re.compile(r"^### (DW-\d+): (.+?)\s*$", re.MULTILINE) # Where a canonical entry ENDS, in every shape CommonMark spells an ATX heading: @@ -734,6 +749,57 @@ def _operation_digest(operation_id: str) -> str: return hashlib.sha256(operation_id.encode("utf-8")).hexdigest() +# Per-thread reentrancy guard for :func:`ledger_lock`. `file_lock` is per open +# fd, so a second acquisition from the same process does not merely queue — on +# POSIX `flock` it blocks forever against a lock this very thread holds, with no +# timeout and no traceback. Thread-local rather than a plain module global +# because the state being tracked is "does THIS thread already hold it", and two +# threads legitimately contend through the OS lock. +_LOCK_STATE = threading.local() + + +@contextmanager +def ledger_lock(path: Path) -> Iterator[None]: + """Cross-process mutual exclusion for one ledger (#286/#469). + + Held only around a single read->edit->write of `path` — never across a + subprocess, a coding-CLI session, or an operator pause. That is an acceptance + criterion of #286 rather than a style preference: `file_lock`'s Windows + branch gives up after ~10 s and raises, so a holder that waits on anything + slower converts a contended run into a failed one. It is also why the + engine's rollback/restore windows, which span git spawns, get compare-and-set + semantics instead of a lock around the window. + + Acquired in exactly two strata: the leaf mutators in this module, and the + engine's CAS restores, which do pure in-memory text work under the hold. + Never call a mutator while holding it — every mutator takes this lock itself, + and the nested acquisition would deadlock. + + Nesting raises :class:`RuntimeError` rather than deadlocking. The guard is + deliberately path-agnostic: two *different* ledgers would not self-deadlock + on the OS lock, but nesting is still a lock-ordering hazard, and no caller + has a reason to hold two ledgers at once. The lock file itself lives out of + the repository — see :func:`~bmad_loop.runs.lock_path_for` for why a sidecar + beside the tracked ledger would be committed by the engine's own `git add + -A`. Propagates `OSError` from acquisition and + :class:`~bmad_loop.runs.StateRootError` when no state root can be derived: a + write that could not be serialized must fail loudly, not proceed unlocked. + """ + # Lazy, and it has to stay lazy: `runs` imports `verify`, which imports this + # module, so a top-level import here closes the cycle. + from . import runs + + if getattr(_LOCK_STATE, "held", False): + raise RuntimeError("ledger lock is not reentrant") + lock_path = runs.lock_path_for(path) + _LOCK_STATE.held = True + try: + with file_lock(lock_path): + yield + finally: + _LOCK_STATE.held = False + + def _apply_done( text: str, dw_id: str, @@ -786,31 +852,70 @@ def _mark_done_many( note: str, *, operation_id: str | None = None, + notes: Sequence[str] | None = None, ) -> list[str]: - """Shared atomic implementation for the public close operations.""" + """Shared atomic implementation for the public close operations. + + ONE locked read->edit->write: the whole cycle runs under the cross-process + ledger lock (#286/#469), so concurrent mutators — a second run, a sweep, the + TUI decision modal, ``sweep --archive`` — serialize here rather than trading + last-write-wins. Validation stays ABOVE the lock, so a programmer bug reports + itself without first waiting on another process. + + ``notes`` supplies a per-id resolution note, positionally paired with + ``dw_ids``; ``note`` is the fallback for every id when it is None. A length + mismatch raises before any I/O rather than closing a prefix under the wrong + evidence — the pairing is positional, so a short list is a caller bug that + would otherwise mis-attribute notes silently. + """ _require_iso_date(date) + if notes is not None and len(notes) != len(dw_ids): + raise ValueError(f"notes must be one per dw_id: {len(notes)} for {len(dw_ids)} ids") undo_owner = _operation_digest(operation_id) if operation_id is not None else None - if not path.is_file(): - return [] - text = path.read_text(encoding="utf-8") - marked: list[str] = [] - for dw_id in dw_ids: - updated = _apply_done(text, dw_id, date, note, undo_owner=undo_owner) - if updated is None: - continue - text = updated - marked.append(dw_id) - if not marked: + if not dw_ids: + # Nothing to serialize against, so nothing to take a lock for — the same + # early return `append_entries` makes, for the same reason. Below the + # validation above, so an empty batch still reports a bad date or a bad + # operation id; above the lock, so a caller that batches an empty set + # cannot start failing on a lock it never needed. The per-id loop this + # primitive replaced took no lock at all when handed nothing, and that + # identity is part of what "byte-identical to the serial sequence" buys. return [] - atomic_write_text(path, text) - return marked + with ledger_lock(path): + if not path.is_file(): + return [] + text = path.read_text(encoding="utf-8") + marked: list[str] = [] + for index, dw_id in enumerate(dw_ids): + entry_note = note if notes is None else notes[index] + updated = _apply_done(text, dw_id, date, entry_note, undo_owner=undo_owner) + if updated is None: + continue + text = updated + marked.append(dw_id) + if not marked: + return [] + atomic_write_text(path, text) + return marked -def mark_done_many(path: Path, dw_ids: Sequence[str], date: str, note: str) -> list[str]: +def mark_done_many( + path: Path, + dw_ids: Sequence[str], + date: str, + note: str, + *, + notes: Sequence[str] | None = None, +) -> list[str]: """Flip every entry in `dw_ids` to `status: done ` + a resolution note, in ONE read and ONE atomic write. Returns the ids actually flipped (missing and already-done ids are skipped), in the order given. + ``notes[i]`` overrides `note` for ``dw_ids[i]`` — the shape a caller closing + several entries under per-entry evidence needs, which otherwise costs one + read-modify-write cycle per id. A length mismatch raises `ValueError` before + any I/O. + All-or-nothing on purpose. A per-id read-modify-write loop leaves marks on disk when it raises partway through several ids — a half-applied closure the caller never gets to journal, so the ledger claims resolutions the run has no @@ -825,7 +930,7 @@ def mark_done_many(path: Path, dw_ids: Sequence[str], date: str, note: str) -> l ``date`` is validated before the ``is_file`` short-circuit so a programmer bug fails the same way whether or not a ledger happens to exist — a guard that only fires when the file is present is one an absent fixture hides.""" - return _mark_done_many(path, dw_ids, date, note) + return _mark_done_many(path, dw_ids, date, note, notes=notes) def mark_done_many_reopenable( @@ -863,54 +968,56 @@ def mark_done(path: Path, dw_id: str, date: str, note: str) -> bool: ) -def mark_open(path: Path, dw_id: str, note: str, operation_id: str) -> bool: - """Undo one close written by :func:`mark_done_many_reopenable`. +def _apply_open(text: str, dw_id: str, note: str, undo_owner: str) -> str | None: + """Undo one reopenable close *within* `text`. None when the entry is missing, + already open, or does not carry this operation's adjacent resolution and + undo-marker lines. - The entry must still carry the operation's adjacent resolution and undo-marker - lines. A standard or earlier close has no matching marker and cannot be - reopened merely because it reused the same human-readable note. + Pure by construction — text in, text out, no `Path` and no I/O — which is + what keeps :func:`mark_open_many` able to run it several times inside a + single :func:`ledger_lock` hold. A version of this that touched the file + would have to take the lock itself, and the nested acquisition is exactly the + self-deadlock the guard on `ledger_lock` exists to convert into an error. + + A standard or earlier close has no matching marker and cannot be reopened + merely because it reused the same human-readable note. A live ``archived:`` stamp is demoted to :data:`_ARCHIVED_BODY_FIELD` rather than dropped: the reopened entry is no longer archived, but the body its close moved out still is, and that line is the only thing a later triage has - to find it with. - """ - undo_owner = _operation_digest(operation_id) - if not path.is_file(): - return False - text = path.read_text(encoding="utf-8") + to find it with.""" entry = _find_entry(text, dw_id) if entry is None or entry.open: - return False + return None if entry.status_span is None: # parse_ledger deliberately tolerates status-less entries. This primitive # is later called from _defer, where an AttributeError would crash the run # instead of completing the deferral. - return False + return None status_line = entry.body[entry.status_span[0] : entry.status_span[1]] try: _require_canonical_status(entry.status) except ValueError: # Only a canonical status written by mark_done is eligible for undo. # Preserve malformed or human-authored statuses for validation/reporting. - return False + return None res_m = _MARK_DONE_TAIL_RE.match(entry.body, entry.status_span[1]) if res_m is None: - return False + return None if res_m.group(1).strip() != _one_line(note).strip() or res_m.group(2) != undo_owner: - return False + return None if status_line != f"status: done {res_m.group(3)}": - return False + return None try: previous_status_line = bytes.fromhex(res_m.group(4)).decode("utf-8") except (UnicodeDecodeError, ValueError): - return False + return None if LINE_BREAK_RE.search(previous_status_line): - return False + return None previous_status_m = STATUS_RE.fullmatch(previous_status_line) previous_status = previous_status_m.group(1).strip() if previous_status_m else "" if not previous_status or previous_status.split()[0] != "open": - return False + return None start = entry.span[0] + entry.status_span[0] end = entry.span[0] + res_m.end() # Demote the entry's live `archived:` stamps along with the close they @@ -951,19 +1058,123 @@ def mark_open(path: Path, dw_id: str, note: str, operation_id: str) -> bool: ) for cut_start, cut_end, replacement in sorted(cuts, reverse=True): text = text[:cut_start] + replacement + text[cut_end:] - atomic_write_text(path, text) - return True + return text + + +def mark_open_many(path: Path, dw_ids: Sequence[str], note: str, operation_id: str) -> list[str]: + """Undo every close in `dw_ids` written by :func:`mark_done_many_reopenable` + under `operation_id`, in ONE read and ONE atomic write. Returns the ids + actually reopened, in the order given; missing and ineligible ids are + skipped, and an entry whose marker does not match this operation is left + exactly as it was. + + ONE locked read->edit->write: the whole cycle runs under the cross-process + ledger lock (#286/#469), so concurrent mutators — a second run, a sweep, the + TUI decision modal, ``sweep --archive`` — serialize here rather than trading + last-write-wins. A per-id loop over :func:`mark_open` would instead take the + lock once per id, leaving a rival writer a window between every pair of + undos in what a rollback needs to be one step. + + Nothing is written when no id was eligible, so a replayed rollback over + already-reopened entries leaves the file untouched rather than rewriting it + byte-for-byte.""" + undo_owner = _operation_digest(operation_id) + if not dw_ids: + # No ids, no lock — see `_mark_done_many`. The `operation_id` above is + # still validated, so an empty reopen cannot smuggle a bad one through. + return [] + with ledger_lock(path): + if not path.is_file(): + return [] + text = path.read_text(encoding="utf-8") + reopened: list[str] = [] + for dw_id in dw_ids: + updated = _apply_open(text, dw_id, note, undo_owner) + if updated is None: + continue + text = updated + reopened.append(dw_id) + if not reopened: + return [] + atomic_write_text(path, text) + return reopened -def append_decision(path: Path, dw_id: str, date: str, label: str, detail: str) -> bool: - """Record a human decision on an entry without changing its status. +def mark_open(path: Path, dw_id: str, note: str, operation_id: str) -> bool: + """Undo one close written by :func:`mark_done_many_reopenable`. + + A one-id wrapper over :func:`mark_open_many`, which is where the lock is + taken and the contract documented. It delegates rather than duplicating the + read->edit->write so that one public call is exactly one acquisition — a + wrapper that took the lock itself and then called the batch would nest, and + `ledger_lock` raises on that rather than deadlocking.""" + return bool(mark_open_many(path, [dw_id], note, operation_id)) + + +def _apply_decision(text: str, dw_id: str, date: str, label: str, detail: str) -> str | None: + """Insert one `decision: