Skip to content

fix: lock the lockless load-modify-write paths#138

Merged
REPPL merged 6 commits into
mainfrom
fix/iss-101-102-lockless-lmw-class
Jul 26, 2026
Merged

fix: lock the lockless load-modify-write paths#138
REPPL merged 6 commits into
mainfrom
fix/iss-101-102-lockless-lmw-class

Conversation

@REPPL

@REPPL REPPL commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Locks the lockless load-modify-write class (ledger issues iss-101 + iss-102, both minor bugs from the multi-agent hunt): the history index could drop a repo registration under two concurrent installs, and a >60s-stalled capture could have its just-committed issue file deleted by the orphan sweep after Capture returned success.

Chained PR: base is fix/iss-115-120-id-allocator-class (#137) — this branch touches the same capture files #137 changed and ledger-resolves with the --impact verb from #136. Merge order #136#137 → this; GitHub retargets automatically.

What changed

  • One canonical inter-process lock primitive, fsutil.WithFileLock (O_NOFOLLOW + regular-file guard, polled non-blocking flock with timeout, fd released on every path). capture's withLedgerLock and the new ahoy withHistoryLock both route through it — no third flock. Pre-existing bespoke lock loops elsewhere are untouched (flagged, not expanded).
  • iss-101: registerRepo's load-mutate-write of the history index runs inside the lock with a re-load; the interactive re-founding prompt stays strictly outside the lock (per the issue's hard constraint), and the prompt-validated state is re-checked in-lock — a changed candidate surfaces as a conflict, never a stale write. bootstrapHistory seeds atomically (write-then-link), so no O_EXCL zero-byte publish window.
  • iss-102: the orphan sweep and the commit write both run under the ledger lock. Realized guarantee: success ⇒ file present — a commit losing the race fails loudly instead of being silently deleted. The pre-unlink re-check stays as defence in depth.
  • Class sweep: every other load-modify-write site inventoried — already-locked (memory registry, spec/intent mints, history transcripts) or repo-local-only residue, recorded as iss-127 on this branch rather than fixed (out of the queued item's scope).

Verification

  • Three failing reproductions watched first via deterministic interleaving seams (lost update; bootstrap check-then-act; sweep-deletes-committed-file), each failing pre-fix for the right reason and passing post-fix; plus a no-lock-across-prompt test whose prompter probes the flock mid-prompt, and a bootstrap publish-window test.
  • Gate run independently by the orchestrator after all commits: make preflight exit 0, go test -race ./internal/... exit 0, record-lint exit 0.
  • iss-101 and iss-102 resolved open/ → resolved/ on this branch with impact: fix.
  • Independent reviews before this PR opened: security APPROVE with zero findings (attacked the flock fd-pinned TOCTOU and type guards, FIFO/device planting, the bootstrap temp/link race and permissions, in-lock parse bounds, and every silent-fallback path — nothing survived refutation); correctness PROMOTE with zero blockers. Review NOTES were dispositioned, not dropped: iss-128 (registerRepo's best-effort edges lose signal silently — contention skip and a marginal concurrent-double-install lineage-drop) and iss-129 (consolidate the four pre-existing bespoke flock loops onto the new primitive) are captured on this branch, and the stale "O_EXCL" comments were corrected to describe the write-then-link seed.

REPPL added 6 commits July 25, 2026 16:12
The capture ledger's orphan sweep (cleanOrphanPlaceholders, run from
mutationPreamble) executed locklessly while commitCapture filled the
reserved placeholder OUTSIDE the ledger flock. A capture stalled more than
sixty seconds between reservation and commit could have its just-committed
issue file deleted: the sweep classified the placeholder as an aged orphan,
the stalled commit's atomic rename then landed, and the sweep's os.Remove
deleted the fully-committed file after Capture had returned success (iss-102).

Run both the sweep and the commit's checksum-reread+write under withLedgerLock,
so a commit that races the sweep blocks on the lock; the sweep unlinks the
still-empty placeholder and the commit then fails loudly on the missing
placeholder instead of the sweep destroying a committed file. The existing
orphanStillRemovable pre-unlink re-check stays as defence in depth.

Promote the flock core to a single shared primitive, fsutil.WithFileLock, so
capture's withLedgerLock (and, next, ahoy's history registry) route through one
inter-process load-modify-write lock rather than divergent flock loops.

Assisted-by: Claude:claude-opus-4-8
registerRepo did an unlocked load-modify-write of ~/.abcd/history/index.json
(loadHistoryIndex -> mutate -> writeHistoryIndex) and bootstrapHistory was
check-then-act. Atomic rename keeps the file intact but not lost-update-free:
two `abcd ahoy install` runs from different worktrees share one index, so the
last writer clobbered the other's update, silently dropping a repo registration
or a re-founding lineage link (iss-101).

Serialize the registerRepo load-modify-write behind an inter-process lock in
~/.abcd/history (the shared fsutil.WithFileLock primitive) and re-load the index
INSIDE the lock, so concurrent registrations compose instead of overwriting.
Bootstrap now publishes index.json atomically via an exclusive hard link
(write temp -> os.Link), so exactly one racing run seeds it and no reader ever
observes a 0-byte index that would parse-fail and drop its own registration.

The re-founding lineage confirmation is asked BEFORE the lock is acquired --
never across the interactive prompt -- and the state it validated (the candidate
still present and still linkable) is re-checked under the lock after the reload:
a candidate changed by a concurrent install surfaces as a conflict rather than
writing a lineage link the user approved against a stale index.

Assisted-by: Claude:claude-opus-4-8
Both records move open/ -> resolved/ with impact: fix, closing the
lockless load-modify-write class fixed on this branch.

Assisted-by: Claude:claude-opus-4-8
The iss-101/102 class sweep confirmed the remaining load-modify-write
sites are repo-local only (no home-global cross-worktree exposure).
Recorded rather than fixed: out of the queued item's scope.

Assisted-by: Claude:claude-fable-5
Review NOTES recorded, not fixed: registerRepo's best-effort edges
lose signal silently (iss-128), and four pre-existing bespoke flock
loops should consolidate onto fsutil.WithFileLock (iss-129).

Assisted-by: Claude:claude-fable-5
The history-index seed comments still described an "O_EXCL create"; the final
implementation publishes via an atomic hard link (write temp, sync, os.Link;
EEXIST means a concurrent run already seeded it). Reword the bootstrapHistory
doc and the bootstrap-race test comment to match. Comments only.

Assisted-by: Claude:claude-opus-4-8
Base automatically changed from fix/iss-115-120-id-allocator-class to main July 26, 2026 07:49
@REPPL
REPPL merged commit 1ae055f into main Jul 26, 2026
12 checks passed
@REPPL
REPPL deleted the fix/iss-101-102-lockless-lmw-class branch July 26, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant