Skip to content

_restore_ledger: the reset_owned CAS anchor trusts a post-reset observation and can overwrite a concurrent writer #735

Description

@pbean

Follow-up residual from PR #726 (the #286 + #469 ledger/board cross-process
serialization program). Filed rather than fixed there because every candidate remedy
changes a design decision that PR reviewed and pinned, and it surfaced in the
finalization phase of an eight-phase program.

The defect

Engine._restore_ledger (src/bmad_loop/engine.py) gates its pre-harvest restore on two
compare-and-set anchors. The second is, in effect:

reset_owned = (current == observed) and <git owns the ledger>

where observed is read immediately after _rollback_or_pause() returns.

If another process writes a tracked ledger in the window between git reset --hard
completing inside _rollback_or_pause() and that unlocked observed read, then observed
is the rival's text. Later current == observed holds, the branch labels the rival's
write reset_owned, and the restore overwrites it with snapshot — silently dropping the
other process's entries.

Reading sooner only narrows the window; it cannot close it. The anchor is derived from an
observation taken after the event it is meant to attest.

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

Severity: residual, not a regression

Before #726 this site performed an unconditional overwrite with no anchor at all. The
guarded version is strictly narrower — it now requires a rival to land inside a small
post-reset window rather than at any point in the restore. So #726 is a large net
improvement at this site, and this issue tracks the part it did not close.

Candidate remedies

  1. Derive the expected state independently of the post-reset read — e.g. take the
    committed blob from git rather than trusting an observation of the working tree. Costs a
    new subprocess spawn inside a restore path plus its failure modes, and ledger_lock
    deliberately covers file I/O only, so the probe would have to sit outside the lock (the
    same hoist S7 already had to make for the git-ownership probe).
  2. Treat the ambiguous state as divergent and refuse to write — journal and escalate
    instead of restoring, which is exactly what _ensure_migration's CAS already does on
    divergence. Cheaper and loud, but it gives up the restore in cases where it would have
    been correct.

Option 2 matches the degrade policy already established elsewhere in the family; option 1
preserves more of the restore's usefulness. The choice is a design call, not a repair.

Constraints any fix must respect

  • Locks never span a subprocess, session, or pause (deferred-work.md: concurrent writers can lose entries, duplicate DW ids, and revert closures #286 acceptance criterion) — a lock
    around the reset window is not available as a remedy.
  • Exclusion probes use blocking=False + pytest.raises(OSError), never sleeps.
  • A restore-write test over a tracked ledger is vacuous: git reset --hard restores the
    file regardless of whether the restore code runs. Any test for this must use an untracked
    fixture or assert on the journal, a point proven by control in session 6 of the program.

Related: #726, #286, #469.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions