From df74ccee6e979d1a4647bcb97f53b41fa898c245 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:12:37 +0100 Subject: [PATCH 1/6] fix: serialize capture orphan sweep and commit write on the ledger lock 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 --- internal/core/capture/alloc.go | 91 ++++++++--------------- internal/core/capture/lockrace_test.go | 99 ++++++++++++++++++++++++++ internal/core/capture/workflow.go | 46 ++++++++---- internal/fsutil/flock.go | 94 ++++++++++++++++++++++++ 4 files changed, 254 insertions(+), 76 deletions(-) create mode 100644 internal/core/capture/lockrace_test.go create mode 100644 internal/fsutil/flock.go diff --git a/internal/core/capture/alloc.go b/internal/core/capture/alloc.go index 84bc78c..30cc362 100644 --- a/internal/core/capture/alloc.go +++ b/internal/core/capture/alloc.go @@ -1,6 +1,7 @@ package capture import ( + "errors" "fmt" "math" "os" @@ -9,6 +10,8 @@ import ( "strconv" "syscall" "time" + + "github.com/REPPL/abcd-cli/internal/fsutil" ) const lockFilename = ".iss-alloc.lock" @@ -24,6 +27,12 @@ const orphanAgeThreshold = 60 * time.Second // test can shorten it to exercise contention without a multi-second wait. var lockTimeout = 5 * time.Second +// beforeOrphanRemoveHook, when non-nil, fires immediately before the sweep +// unlinks a classified orphan placeholder. It is a test-only seam (nil in +// production, zero overhead) used to force the iss-102 commit-in-the-unlink-window +// interleaving deterministically. +var beforeOrphanRemoveHook func(cand string) + var rePlaceholderName = regexp.MustCompile(`^iss-[0-9]+(-[a-z0-9]+(-[a-z0-9]+)*)?\.md$`) var reMaxIssN = regexp.MustCompile(`^iss-([0-9]+)(?:-[a-z0-9-]+)?\.md$`) @@ -69,26 +78,24 @@ func safeMkdirLeaf(target string) error { } // withLedgerLock runs fn while holding the exclusive allocator flock, so every -// ledger mutation — id allocation AND status transitions — serializes on one -// lock. It creates the ledger dirs, opens the lock with the symlink/regular-file -// guards, acquires the flock within lockTimeout, runs fn, then releases. +// ledger mutation — id allocation, status transitions, AND the capture commit +// write — serializes on one lock. It creates the ledger dirs, then routes through +// the shared fsutil.WithFileLock primitive (the one-canonical inter-process +// load-modify-write lock), mapping its sentinels back to the capture-facing +// ErrAllocatorContention / ErrPathUnsafe the ledger callers already test. func withLedgerLock(issuesRoot string, fn func() error) error { if err := ensureLedgerDirs(issuesRoot); err != nil { return err } lockPath := filepath.Join(issuesRoot, lockFilename) - lockFd, err := safeOpenLockFd(lockPath) - if err != nil { - return err - } - defer syscall.Close(lockFd) - - if err := acquireFlock(lockFd, lockTimeout); err != nil { - return err - } - defer syscall.Flock(lockFd, syscall.LOCK_UN) - - return fn() + err := fsutil.WithFileLock(lockPath, lockTimeout, fn) + switch { + case errors.Is(err, fsutil.ErrLockContention): + return fmt.Errorf("%w: could not acquire allocator lock within %s", ErrAllocatorContention, lockTimeout) + case errors.Is(err, fsutil.ErrLockPathUnsafe): + return fmt.Errorf("%w: allocator lock path is unsafe: %s", ErrPathUnsafe, lockPath) + } + return err } // reservePath reserves an iss-N id and creates a zero-byte placeholder under @@ -160,54 +167,6 @@ func reservePath(issuesRoot, slug, forceID string, refFloor int) (string, string return resID, resTarget, nil } -// safeOpenLockFd opens the allocator lock with O_NOFOLLOW and verifies it is a -// regular file, refusing a symlinked or non-regular lock path. -func safeOpenLockFd(lockPath string) (int, error) { - fd, err := syscall.Open(lockPath, syscall.O_CREAT|syscall.O_RDWR|syscall.O_NOFOLLOW, 0o644) - if err != nil { - if err == syscall.ELOOP { - return -1, fmt.Errorf("%w: allocator lock path is a symlink: %s", ErrPathUnsafe, lockPath) - } - return -1, err - } - var st syscall.Stat_t - if err := syscall.Fstat(fd, &st); err != nil { - syscall.Close(fd) - return -1, err - } - if st.Mode&syscall.S_IFMT != syscall.S_IFREG { - syscall.Close(fd) - return -1, fmt.Errorf("%w: allocator lock path is not a regular file: %s", ErrPathUnsafe, lockPath) - } - return fd, nil -} - -// acquireFlock polls for an exclusive flock until timeout elapses. -func acquireFlock(fd int, timeout time.Duration) error { - deadline := time.Now().Add(timeout) - backoff := 5 * time.Millisecond - for { - err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB) - if err == nil { - return nil - } - if err != syscall.EWOULDBLOCK && err != syscall.EAGAIN { - return err - } - remaining := time.Until(deadline) - if remaining <= 0 { - return fmt.Errorf("%w: could not acquire allocator lock within %s", ErrAllocatorContention, timeout) - } - if backoff > remaining { - backoff = remaining - } - time.Sleep(backoff) - if backoff < 100*time.Millisecond { - backoff *= 2 - } - } -} - // createPlaceholder does an O_EXCL|O_NOFOLLOW create of the placeholder file. func createPlaceholder(target string) (int, error) { fd, err := syscall.Open(target, syscall.O_CREAT|syscall.O_EXCL|syscall.O_WRONLY|syscall.O_NOFOLLOW, 0o644) @@ -349,6 +308,12 @@ func cleanOrphanPlaceholders(issuesRoot string) error { if !orphanStillRemovable(cand, cfi) { continue } + // Test-only seam (nil in production): fires in the residual window between + // the pre-unlink re-check and the unlink, to force the documented iss-102 + // interleaving where a stalled commit's fill lands here. + if beforeOrphanRemoveHook != nil { + beforeOrphanRemoveHook(cand) + } os.Remove(cand) } return nil diff --git a/internal/core/capture/lockrace_test.go b/internal/core/capture/lockrace_test.go new file mode 100644 index 0000000..59162b4 --- /dev/null +++ b/internal/core/capture/lockrace_test.go @@ -0,0 +1,99 @@ +package capture + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// TestOrphanSweepDoesNotDeleteCommittedFile is the iss-102 repro. It forces the +// documented interleaving: the orphan sweep classifies a >60s-stalled, zero-byte +// placeholder as removable, then — in the residual window immediately before the +// unlink — a stalled commit fills that exact placeholder. Pre-fix (sweep and +// commit both lockless) the sweep's os.Remove deletes the file the commit just +// wrote, AFTER the commit reported success — silent data loss. Post-fix the sweep +// runs under the ledger lock and the commit write does too, so the commit cannot +// land inside the sweep's window: it blocks on the lock, the sweep removes the +// still-empty placeholder, and the commit then fails loudly (missing placeholder) +// instead of the sweep destroying a committed file. +// +// The invariant asserted is exactly the issue's failure mode: a commit that +// reports success must leave its file present. Pre-fix that invariant is violated; +// post-fix the racing commit never reports success. +func TestOrphanSweepDoesNotDeleteCommittedFile(t *testing.T) { + repo, ir := ledger(t) + if err := ensureLedgerDirs(ir); err != nil { + t.Fatal(err) + } + + // Reserve a placeholder by hand and age it past the sweep threshold, standing + // in for a capture that stalled >60s between reservation and commit. + placeholder := filepath.Join(ir, "open", "iss-1-stall.md") + if err := os.WriteFile(placeholder, []byte{}, 0o644); err != nil { + t.Fatal(err) + } + old := time.Now().Add(-2 * orphanAgeThreshold) + if err := os.Chtimes(placeholder, old, old); err != nil { + t.Fatal(err) + } + + req := CaptureRequest{ + RepoRoot: repo, IssuesRoot: ir, Text: "stalled commit body\n", + Severity: SeverityMinor, Category: "bug", Source: "manual-test", + Slug: "stall", FoundDuring: "iss-102 repro", + } + + // The committer fires when the sweep reaches its pre-unlink window, and reports + // its outcome once. Buffered so the send never blocks whether the hook or the + // test drains it. + trigger := make(chan struct{}) + result := make(chan error, 1) + go func() { + <-trigger + _, err := commitCapture(ir, req, "iss-1", "stall", placeholder) + result <- err + }() + + // Shorten the lock budget so the post-fix "commit blocked on the sweep's lock" + // path resolves within the test window instead of the 5s default. + origTimeout := lockTimeout + lockTimeout = 200 * time.Millisecond + defer func() { lockTimeout = origTimeout }() + + var commitErr error + commitDone := false + beforeOrphanRemoveHook = func(cand string) { + beforeOrphanRemoveHook = nil // fire exactly once + close(trigger) + select { + case commitErr = <-result: + commitDone = true // pre-fix: the commit wrote and returned before the unlink + case <-time.After(500 * time.Millisecond): + // post-fix: the commit is blocked on the ledger lock this sweep holds. + } + } + defer func() { beforeOrphanRemoveHook = nil }() + + // Run the sweep through the production preamble so post-fix it holds the lock. + if err := mutationPreamble(ir); err != nil { + t.Fatalf("mutationPreamble: %v", err) + } + + if !commitDone { + commitErr = <-result // post-fix: the commit unblocks once the sweep released + } + + // The core invariant: if the commit reported success, its file must exist and + // be non-empty. Pre-fix the sweep deleted the just-committed file, so a nil + // commitErr coincides with a missing file — the violation. + if commitErr == nil { + fi, err := os.Stat(placeholder) + if err != nil { + t.Fatalf("commit reported success but the committed file is gone: %v", err) + } + if fi.Size() == 0 { + t.Fatalf("commit reported success but the committed file is empty") + } + } +} diff --git a/internal/core/capture/workflow.go b/internal/core/capture/workflow.go index 5325321..e555bc4 100644 --- a/internal/core/capture/workflow.go +++ b/internal/core/capture/workflow.go @@ -16,8 +16,15 @@ import ( // mutationPreamble runs the idempotent pre-mutation steps: sweep orphan // placeholders and (re-)assert the symlink-refused directory shape. Read-only // entry points (List, Status) deliberately skip it. +// +// The orphan sweep runs UNDER the ledger lock (iss-102): a >60s-stalled capture +// fills its reserved placeholder via commitCapture, which now also holds the +// ledger lock, so the sweep's classify-then-unlink can no longer interleave with +// a commit's fill and delete a just-committed issue file. func mutationPreamble(issuesRoot string) error { - if err := cleanOrphanPlaceholders(issuesRoot); err != nil { + if err := withLedgerLock(issuesRoot, func() error { + return cleanOrphanPlaceholders(issuesRoot) + }); err != nil { return err } return ensureLedgerDirs(issuesRoot) @@ -59,7 +66,7 @@ func Capture(req CaptureRequest) (CaptureResult, error) { return CaptureResult{}, err } - result, err := commitCapture(req, issID, slugNorm, placeholder) + result, err := commitCapture(issuesRoot, req, issID, slugNorm, placeholder) if err != nil { _ = cancelReservation(placeholder) return CaptureResult{}, err @@ -71,7 +78,7 @@ func Capture(req CaptureRequest) (CaptureResult, error) { return result, nil } -func commitCapture(req CaptureRequest, issID, slug, placeholder string) (CaptureResult, error) { +func commitCapture(issuesRoot string, req CaptureRequest, issID, slug, placeholder string) (CaptureResult, error) { fields := []kv{ {"schema_version", 1}, {"id", issID}, @@ -119,19 +126,32 @@ func commitCapture(req CaptureRequest, issID, slug, placeholder string) (Capture return CaptureResult{}, err } - // Guard the overwrite: the placeholder must still be the zero-byte file we - // reserved (expected_checksum = sha256("")). - _, checksum, err := readWithChecksum(placeholder) + // The checksum re-read + fill runs UNDER the ledger lock (iss-102): the orphan + // sweep (mutationPreamble) also holds this lock, so a >60s-stalled commit can no + // longer land its fill in the sweep's classify-then-unlink window and have the + // just-committed file deleted. If the sweep reclaimed the placeholder first, the + // re-read fails and the capture reports an error rather than a false success. + var result CaptureResult + err = withLedgerLock(issuesRoot, func() error { + // Guard the overwrite: the placeholder must still be the zero-byte file we + // reserved (expected_checksum = sha256("")). + _, checksum, rerr := readWithChecksum(placeholder) + if rerr != nil { + return rerr + } + if checksum != emptyChecksum { + return fmt.Errorf("%w: placeholder %s changed since reservation", ErrChecksumMismatch, placeholder) + } + if werr := fsutil.WriteFileAtomicPreserveMode(placeholder, []byte(content)); werr != nil { + return werr + } + result = CaptureResult{ID: issID, Slug: slug, Path: placeholder, Status: StateOpen} + return nil + }) if err != nil { return CaptureResult{}, err } - if checksum != emptyChecksum { - return CaptureResult{}, fmt.Errorf("%w: placeholder %s changed since reservation", ErrChecksumMismatch, placeholder) - } - if err := fsutil.WriteFileAtomicPreserveMode(placeholder, []byte(content)); err != nil { - return CaptureResult{}, err - } - return CaptureResult{ID: issID, Slug: slug, Path: placeholder, Status: StateOpen}, nil + return result, nil } // Resolve moves an open issue to resolved/, writing the resolution note and the diff --git a/internal/fsutil/flock.go b/internal/fsutil/flock.go new file mode 100644 index 0000000..7cb22c7 --- /dev/null +++ b/internal/fsutil/flock.go @@ -0,0 +1,94 @@ +package fsutil + +import ( + "errors" + "fmt" + "syscall" + "time" +) + +// ErrLockContention and ErrLockPathUnsafe are the file-lock sentinels: the +// exclusive lock could not be acquired within the caller's timeout, and the lock +// path is a symlink or not a regular file. +var ( + ErrLockContention = errors.New("fsutil: lock contention") + ErrLockPathUnsafe = errors.New("fsutil: lock path unsafe") +) + +// WithFileLock acquires an exclusive advisory (flock) lock on lockPath, holds it +// across fn, and releases it when fn returns. The lock file is opened O_NOFOLLOW +// (a symlinked lock path is refused) and verified on the same descriptor to be a +// regular file, both reported as ErrLockPathUnsafe; the exclusive flock is polled +// for up to timeout, after which ErrLockContention is returned. fn's own error is +// returned unwrapped. +// +// It is the single inter-process load-modify-write lock primitive: capture's +// ledger allocator and ahoy's ~/.abcd history registry both route through it +// rather than keep divergent flock loops (the one-canonical-primitive invariant). +// The lock is advisory — every writer of the guarded state must take it — and its +// scope is one lock file; callers that must not deadlock keep their acquisitions +// unnested. +func WithFileLock(lockPath string, timeout time.Duration, fn func() error) error { + fd, err := openLockFd(lockPath) + if err != nil { + return err + } + defer syscall.Close(fd) + + if err := acquireFlock(fd, timeout); err != nil { + return err + } + defer syscall.Flock(fd, syscall.LOCK_UN) + + return fn() +} + +// openLockFd opens lockPath with O_CREAT|O_RDWR|O_NOFOLLOW and verifies, on the +// same descriptor, that it is a regular file — refusing a symlinked or +// non-regular lock path with ErrLockPathUnsafe. +func openLockFd(lockPath string) (int, error) { + fd, err := syscall.Open(lockPath, syscall.O_CREAT|syscall.O_RDWR|syscall.O_NOFOLLOW, 0o644) + if err != nil { + if err == syscall.ELOOP { + return -1, fmt.Errorf("%w: lock path is a symlink: %s", ErrLockPathUnsafe, lockPath) + } + return -1, err + } + var st syscall.Stat_t + if err := syscall.Fstat(fd, &st); err != nil { + syscall.Close(fd) + return -1, err + } + if st.Mode&syscall.S_IFMT != syscall.S_IFREG { + syscall.Close(fd) + return -1, fmt.Errorf("%w: lock path is not a regular file: %s", ErrLockPathUnsafe, lockPath) + } + return fd, nil +} + +// acquireFlock polls for an exclusive flock until timeout elapses, returning +// ErrLockContention on timeout. +func acquireFlock(fd int, timeout time.Duration) error { + deadline := time.Now().Add(timeout) + backoff := 5 * time.Millisecond + for { + err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return nil + } + if err != syscall.EWOULDBLOCK && err != syscall.EAGAIN { + return err + } + remaining := time.Until(deadline) + if remaining <= 0 { + return fmt.Errorf("%w: could not acquire lock within %s", ErrLockContention, timeout) + } + if backoff > remaining { + backoff = remaining + } + time.Sleep(backoff) + if backoff < 100*time.Millisecond { + backoff *= 2 + } + } +} From 2b2a7d3858eb220529703750ebbce9d4a1f53a13 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:12:47 +0100 Subject: [PATCH 2/6] fix: lock the history-index load-modify-write across concurrent installs 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 --- CHANGELOG.md | 21 +++ internal/core/ahoy/apply.go | 92 +++++++++--- internal/core/ahoy/lockrace_test.go | 213 ++++++++++++++++++++++++++++ internal/core/ahoy/store.go | 108 +++++++++++++- 4 files changed, 408 insertions(+), 26 deletions(-) create mode 100644 internal/core/ahoy/lockrace_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a318961..cc06e01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,27 @@ called out in a **Breaking** section. user-facing by definition). `capture wontfix` is unchanged — a non-action ships nothing, so `wontfix/` carries no impact. +### Fixed + +- **Concurrent runs can no longer drop a repo registration or delete a + just-committed issue file** (iss-101, iss-102). Two `abcd ahoy install` runs + from different worktrees shared one `~/.abcd/history/index.json`, and its + registration was an unlocked load-modify-write: atomic rename kept the file + intact but the last writer clobbered the other's update, silently erasing a + repo entry or a re-founding lineage link. The history registry now serializes + its load-modify-write behind an inter-process lock and re-loads inside it, so + concurrent registrations compose instead of overwriting; the store bootstrap + creates `index.json` with an exclusive create, so exactly one racing run seeds + it. The re-founding lineage confirmation is still asked before the lock is + taken — never across an interactive prompt — and the state it validated is + re-checked under the lock, surfacing a conflict rather than writing a link the + user approved against a stale index. Separately, the capture ledger's orphan + sweep and its commit write now take the same ledger lock, closing a window in + which a capture stalled more than sixty seconds could have its committed issue + file swept away after the capture reported success. The inter-process lock is a + single shared primitive; the capture allocator and the history registry both + route through it. + ## [0.4.0] - 2026-07-22 ### Breaking diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index c6f2730..11029dc 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -457,35 +457,89 @@ func (a *applyCtx) stepHistory() { // registerRepo registers or refreshes this repo's entry in index.json by its // immutable root_commit. Re-founding lineage is only set on explicit confirm. +// +// The load-modify-write runs UNDER withHistoryLock with a re-load inside the lock +// (iss-101), so two concurrent installs cannot drop a registration by racing +// last-rename-wins. The re-founding confirmation, which can block on user input, +// is asked BEFORE the lock is acquired — the lock is never held across a prompt. +// The state that prompt validated (the candidate lineage exists and is still +// linkable) is RE-CHECKED against the re-loaded index inside the lock: if a +// concurrent install changed it, the lineage link the user approved against stale +// state is refused rather than silently applied. func (a *applyCtx) registerRepo(sha string) { idx, err := loadHistoryIndex() if err != nil || idx == nil { return } id := a.det.RepoIdentity - if e := indexEntry(idx, sha); e != nil { - e.Name, e.Github, e.Path = id.Name, id.Github, a.cwd // refresh mutable labels - if e.Status == "" { - e.Status = "active" + + // Decide, OUTSIDE the lock, whether a re-founding prompt is needed. A prompt is + // only relevant for a not-yet-registered sha with a lineage candidate; the + // refresh path (sha already present) involves no prompt. + linkLineage := false + var candSHA string + if indexEntry(idx, sha) == nil { + if cand := findRefoundingCandidate(idx, id); cand != nil { + candSHA = cand.RootCommit + linkLineage = a.prompter.Confirm("Re-founded from " + shortSHA(candSHA) + "? Link lineage?") + } + } + + wrote := false + lineageConflict := false + _ = withHistoryLock(func() error { + // Re-load inside the lock: the pre-lock read may be stale after a concurrent + // install's write, and the mutation must build on the current index. + locked, lerr := loadHistoryIndex() + if lerr != nil || locked == nil { + return lerr } - _ = writeHistoryIndex(idx) + if e := indexEntry(locked, sha); e != nil { + e.Name, e.Github, e.Path = id.Name, id.Github, a.cwd // refresh mutable labels + if e.Status == "" { + e.Status = "active" + } + if werr := writeHistoryIndex(locked); werr != nil { + return werr + } + wrote = true + return nil + } + newEntry := historyRepo{RootCommit: sha, Name: id.Name, Github: id.Github, Path: a.cwd, Status: "active"} + if linkLineage { + // Re-check the answer-relevant state under the lock: the candidate we + // prompted about must still exist by its root_commit and still be + // linkable (not already superseded by a concurrent install). A changed + // state is a conflict — skip the lineage link rather than write one the + // user approved against a now-stale index. + cand := indexEntry(locked, candSHA) + if cand == nil || cand.SupersededBy != "" { + linkLineage = false + lineageConflict = true + } else { + newEntry.Supersedes = candSHA + cand.SupersededBy = sha + cand.Status = "superseded" + } + } + locked.Repos = append(locked.Repos, newEntry) + if werr := writeHistoryIndex(locked); werr != nil { + return werr + } + wrote = true + return nil + }) + if lineageConflict { + // Surface the refused link rather than silently proceed: the re-founding + // candidate the user confirmed changed under the lock, so the repo is + // registered without the lineage link they approved against stale state. + a.changes = append(a.changes, + "history lineage: link to "+shortSHA(candSHA)+" skipped (candidate changed under concurrent install)") + } + if wrote { if root, e2 := historyRoot(); e2 == nil { a.note(filepath.Join(root, "index.json")) } - return - } - newEntry := historyRepo{RootCommit: sha, Name: id.Name, Github: id.Github, Path: a.cwd, Status: "active"} - if cand := findRefoundingCandidate(idx, id); cand != nil { - if a.prompter.Confirm("Re-founded from " + shortSHA(cand.RootCommit) + "? Link lineage?") { - newEntry.Supersedes = cand.RootCommit - cand.SupersededBy = sha - cand.Status = "superseded" - } - } - idx.Repos = append(idx.Repos, newEntry) - _ = writeHistoryIndex(idx) - if root, e2 := historyRoot(); e2 == nil { - a.note(filepath.Join(root, "index.json")) } } diff --git a/internal/core/ahoy/lockrace_test.go b/internal/core/ahoy/lockrace_test.go new file mode 100644 index 0000000..364245f --- /dev/null +++ b/internal/core/ahoy/lockrace_test.go @@ -0,0 +1,213 @@ +package ahoy + +import ( + "path/filepath" + "syscall" + "testing" +) + +// TestRegisterRepoConcurrentDoesNotLoseUpdate is the iss-101 lost-update repro. +// Two registerRepo sequences for distinct repos interleave so the second's +// mutation section runs while the first is paused mid-critical-section. Pre-fix +// (unlocked load-modify-write, no re-load) each writes its own stale in-memory +// index and last-rename-wins drops one registration; post-fix each mutation runs +// under the history lock and re-loads inside it, so both registrations survive. +func TestRegisterRepoConcurrentDoesNotLoseUpdate(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if _, err := bootstrapHistory(); err != nil { + t.Fatal(err) + } + + // One goroutine grabs the single token, becoming the paused "victim": it signals + // it has loaded inside the lock and waits. The other proceeds to a full + // registration; then the victim is released. + tokens := make(chan struct{}, 1) + tokens <- struct{}{} + loaded := make(chan struct{}) + release := make(chan struct{}) + afterHistoryReloadHook = func() { + select { + case <-tokens: + close(loaded) + <-release + default: + } + } + defer func() { afterHistoryReloadHook = nil }() + + mk := func(sha, name string) *applyCtx { + return &applyCtx{ + cwd: filepath.Join(home, name), + det: DetectionResult{RepoIdentity: RepoIdentity{Name: name, RootSHA: sha}}, + prompter: RefusingPrompter{}, + } + } + + done := make(chan struct{}, 2) + go func() { mk("sha-aaaaaaaa", "repo-a").registerRepo("sha-aaaaaaaa"); done <- struct{}{} }() + go func() { mk("sha-bbbbbbbb", "repo-b").registerRepo("sha-bbbbbbbb"); done <- struct{}{} }() + + <-loaded // victim loaded inside the lock and is paused + close(release) // let it resume; the other has been blocked on the lock + <-done + <-done + + idx, err := loadHistoryIndex() + if err != nil || idx == nil { + t.Fatalf("load index: %v", err) + } + if !indexHasRoot(idx, "sha-aaaaaaaa") || !indexHasRoot(idx, "sha-bbbbbbbb") { + t.Fatalf("lost update: both registrations must survive, got %d repos: %+v", len(idx.Repos), idx.Repos) + } +} + +// TestBootstrapHistoryExclusiveCreate is the iss-101 bootstrap check-then-act +// repro. The first bootstrapper pauses just before its create and lets a second +// full bootstrap complete in that window. Post-fix (O_EXCL create) exactly one +// wins the create and the other observes the existing file (no write); pre-fix +// (check-then-act + last-rename-wins) both report a create. +func TestBootstrapHistoryExclusiveCreate(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + first := true + var secondWrote bool + var secondErr error + beforeHistoryIndexCreateHook = func() { + if !first { + return + } + first = false + beforeHistoryIndexCreateHook = nil // disarm so the nested bootstrap runs clean + secondWrote, secondErr = bootstrapHistory() + } + defer func() { beforeHistoryIndexCreateHook = nil }() + + firstWrote, err := bootstrapHistory() + if err != nil { + t.Fatalf("first bootstrap: %v", err) + } + if secondErr != nil { + t.Fatalf("second bootstrap: %v", secondErr) + } + if firstWrote == secondWrote { + t.Fatalf("exactly one bootstrap must create index.json; got first=%v second=%v", firstWrote, secondWrote) + } + + // The surviving index.json must be valid, not an empty-clobbered file. + idx, lerr := loadHistoryIndex() + if lerr != nil || idx == nil { + t.Fatalf("index.json unreadable after bootstrap race: %v", lerr) + } +} + +// TestBootstrapHistoryNoEmptyReadWindow guards the atomic-seed property (the +// reviewer FIX-FIRST for iss-101): a bootstrap must never publish a 0-byte +// index.json that a concurrent loadHistoryIndex could parse-fail on and drop its +// own registration. A read taken at the publish point sees either no file yet or +// a complete one — never a malformed empty file. +func TestBootstrapHistoryNoEmptyReadWindow(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + var readErr error + beforeHistoryIndexCreateHook = func() { + beforeHistoryIndexCreateHook = nil + // A reader racing the publish must not observe a malformed index.json. + _, readErr = loadHistoryIndex() + } + defer func() { beforeHistoryIndexCreateHook = nil }() + + wrote, err := bootstrapHistory() + if err != nil { + t.Fatalf("bootstrap: %v", err) + } + if !wrote { + t.Fatal("expected the bootstrap to seed the index") + } + if readErr != nil { + t.Fatalf("concurrent read in the bootstrap publish window saw a malformed index: %v", readErr) + } + idx, lerr := loadHistoryIndex() + if lerr != nil || idx == nil { + t.Fatalf("final index invalid after bootstrap: %v", lerr) + } +} + +// lockProbingPrompter records, on each Confirm, whether the ~/.abcd/history lock +// is held at prompt time — the no-lock-across-prompt guard for iss-101. +type lockProbingPrompter struct { + t *testing.T + lockPath string + confirmed bool + lockHeld bool +} + +func (p *lockProbingPrompter) Confirm(string) bool { + p.confirmed = true + fd, err := syscall.Open(p.lockPath, syscall.O_CREAT|syscall.O_RDWR|syscall.O_NOFOLLOW, 0o644) + if err != nil { + p.t.Fatalf("probe open lock: %v", err) + } + defer syscall.Close(fd) + if err := syscall.Flock(fd, syscall.LOCK_EX|syscall.LOCK_NB); err == nil { + syscall.Flock(fd, syscall.LOCK_UN) + } else { + // Could not acquire: registerRepo is holding the history lock across this + // interactive prompt — exactly the condition iss-101 forbids. + p.lockHeld = true + } + return true // confirm the lineage link +} + +func (p *lockProbingPrompter) Prompt(_ string, _ []string, def string) string { return def } + +// TestRegisterRepoDoesNotHoldLockAcrossPrompt proves the re-founding confirmation +// is asked with the history lock NOT held (iss-101 hard constraint). A prior +// entry with a matching name makes the new registration trigger the lineage +// prompt; the prompter probes the lock while answering. +func TestRegisterRepoDoesNotHoldLockAcrossPrompt(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if _, err := bootstrapHistory(); err != nil { + t.Fatal(err) + } + root, err := historyRoot() + if err != nil { + t.Fatal(err) + } + lockPath := filepath.Join(root, historyLockFilename) + + // Seed a re-founding candidate: same name, different root_commit. + seed := &applyCtx{ + cwd: filepath.Join(home, "old"), + det: DetectionResult{RepoIdentity: RepoIdentity{Name: "myrepo", RootSHA: "sha-old"}}, + prompter: RefusingPrompter{}, + } + seed.registerRepo("sha-old") + + probe := &lockProbingPrompter{t: t, lockPath: lockPath} + nu := &applyCtx{ + cwd: filepath.Join(home, "new"), + det: DetectionResult{RepoIdentity: RepoIdentity{Name: "myrepo", RootSHA: "sha-new"}}, + prompter: probe, + } + nu.registerRepo("sha-new") + + if !probe.confirmed { + t.Fatal("the re-founding lineage prompt was never triggered; test did not exercise the constraint") + } + if probe.lockHeld { + t.Fatal("history lock was held across the interactive prompt (iss-101 violation)") + } + // The lineage link the user confirmed must have landed. + idx, err := loadHistoryIndex() + if err != nil || idx == nil { + t.Fatalf("load index: %v", err) + } + e := indexEntry(idx, "sha-new") + if e == nil || e.Supersedes != "sha-old" { + t.Fatalf("confirmed lineage link not recorded: %+v", e) + } +} diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 5941b1f..0a80f25 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "time" "github.com/REPPL/abcd-cli/internal/fsutil" "github.com/REPPL/abcd-cli/internal/gitutil" @@ -227,25 +228,118 @@ func findRefoundingCandidate(idx *historyIndex, id RepoIdentity) *historyRepo { return nil } +// historyLockFilename is the ~/.abcd/history lock file guarding the index.json +// load-modify-write. It sits beside index.json. +const historyLockFilename = ".index.lock" + +// historyLockTimeout bounds acquisition of the history-index lock. A var (not +// const) so a test can shorten it to exercise contention without a multi-second +// wait, mirroring capture's lockTimeout. +var historyLockTimeout = 5 * time.Second + +// afterHistoryReloadHook, when non-nil, fires inside withHistoryLock immediately +// after the lock is acquired and before fn runs (fn re-loads the index under the +// lock and writes it). It is a test-only seam (nil in production) used to force +// the iss-101 lost-update interleaving deterministically: the goroutine that grabs +// the lock pauses here, holding it, while another registration is admitted. +var afterHistoryReloadHook func() + +// beforeHistoryIndexCreateHook, when non-nil, fires in bootstrapHistory just +// before the atomic link that publishes index.json. Test-only seam (nil in +// production) to force the iss-101 bootstrap check-then-act interleaving +// deterministically. +var beforeHistoryIndexCreateHook func() + +// withHistoryLock runs fn while holding the exclusive ~/.abcd/history lock, so a +// registerRepo load-modify-write of index.json serializes across concurrent +// `abcd ahoy install` runs from different worktrees (iss-101). It routes through +// the shared fsutil.WithFileLock primitive; the lock file lives beside +// index.json. The lock is NEVER held across an interactive prompt — callers do +// any prompting before acquiring it and re-check the answer-relevant state inside +// fn after re-loading. +func withHistoryLock(fn func() error) error { + root, err := historyRoot() + if err != nil { + return err + } + if err := os.MkdirAll(root, 0o755); err != nil { + return err + } + lockPath := filepath.Join(root, historyLockFilename) + return fsutil.WithFileLock(lockPath, historyLockTimeout, func() error { + if afterHistoryReloadHook != nil { + afterHistoryReloadHook() + } + return fn() + }) +} + // bootstrapHistory creates ~/.abcd/history/ + index.json when absent. Idempotent. +// The seed is published atomically by an exclusive hard link (iss-101): a +// fully-formed temp file is written, then os.Link'd into place. link fails EEXIST +// if index.json already exists, so under two concurrent bootstraps exactly one +// wins and every other observes the existing file and reports no write — and, +// unlike an O_EXCL create followed by a separate content write, no reader ever +// observes a 0-byte index.json, so a concurrent loadHistoryIndex cannot parse-fail +// and drop its own registration. func bootstrapHistory() (bool, error) { root, err := historyRoot() if err != nil { return false, err } - if isDir(root) { - if _, err := os.Stat(filepath.Join(root, "index.json")); err == nil { - return false, nil - } - } if err := os.MkdirAll(root, 0o755); err != nil { return false, err } + path := filepath.Join(root, "index.json") + // Fast path: already seeded (the common idempotent re-run). + if _, err := os.Stat(path); err == nil { + return false, nil + } idx := historyIndex{Schema: 1, Description: historyIndexDescription, Repos: []historyRepo{}} - return true, writeJSON(filepath.Join(root, "index.json"), idx) + data, err := json.MarshalIndent(idx, "", " ") + if err != nil { + return false, err + } + data = append(data, '\n') + + // Write a complete temp file first, then link it into place as the atomic, + // single-winner publish. The temp is always cleaned up. + tmp, err := os.CreateTemp(root, ".index-*.tmp") + if err != nil { + return false, err + } + tmpName := tmp.Name() + defer os.Remove(tmpName) + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return false, err + } + if err := tmp.Chmod(0o644); err != nil { + tmp.Close() + return false, err + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return false, err + } + if err := tmp.Close(); err != nil { + return false, err + } + + if beforeHistoryIndexCreateHook != nil { + beforeHistoryIndexCreateHook() + } + if err := os.Link(tmpName, path); err != nil { + if os.IsExist(err) { + return false, nil // already seeded (or won by a concurrent run) + } + return false, err + } + return true, nil } -// writeHistoryIndex persists idx. +// writeHistoryIndex persists idx. It must be called from inside withHistoryLock +// (registerRepo) so a re-loaded index is not clobbered by a concurrent writer. func writeHistoryIndex(idx *historyIndex) error { root, err := historyRoot() if err != nil { From edc4bb865ec30ecbf3588b1bd86297bf4b2084ad Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:13:12 +0100 Subject: [PATCH 3/6] chore: resolve iss-101 and iss-102 in the ledger 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 --- .../iss-101-history-index-lost-update-concurrent-install.md | 2 ++ .../iss-102-capture-orphan-sweep-commit-race.md | 2 ++ 2 files changed, 4 insertions(+) rename .abcd/work/issues/{open => resolved}/iss-101-history-index-lost-update-concurrent-install.md (79%) rename .abcd/work/issues/{open => resolved}/iss-102-capture-orphan-sweep-commit-race.md (79%) diff --git a/.abcd/work/issues/open/iss-101-history-index-lost-update-concurrent-install.md b/.abcd/work/issues/resolved/iss-101-history-index-lost-update-concurrent-install.md similarity index 79% rename from .abcd/work/issues/open/iss-101-history-index-lost-update-concurrent-install.md rename to .abcd/work/issues/resolved/iss-101-history-index-lost-update-concurrent-install.md index 96f548a..d8e7f4b 100644 --- a/.abcd/work/issues/open/iss-101-history-index-lost-update-concurrent-install.md +++ b/.abcd/work/issues/resolved/iss-101-history-index-lost-update-concurrent-install.md @@ -6,6 +6,8 @@ severity: "minor" category: "bug" source: "agent-finding" found_during: "multi-agent-bughunt" +resolution: "History-index registration now serializes on an inter-process lock in ~/.abcd/history with a re-load inside the lock, and bootstrap seeds index.json via an atomic exclusive hard link; the re-founding prompt is asked before the lock and its state re-checked under it." +impact: fix --- registerRepo does an unlocked load-modify-write of ~/.abcd/history/index.json (loadHistoryIndex -> mutate -> writeHistoryIndex, apply.go), and bootstrapHistory is check-then-act. WriteFileAtomic prevents torn files but not lost updates (last rename wins), so two concurrent 'abcd ahoy install' runs from different worktrees can drop a repo registration or clobber the registry with an empty Repos list, erasing lineage/supersedes links set on explicit user confirmation. Deferred from the multi-agent bug hunt (was B30): a correct fix needs an inter-process O_EXCL/flock lock in ~/.abcd/history held across the load-mutate-write, with a re-load inside the lock and bootstrapHistory creating index.json with O_EXCL. The registerRepo sequence spans an interactive prompter.Confirm (re-founding lineage), so the lock must NOT be held across that prompt (risk of blocking other processes on user input). Low severity, self-heals on next install. Needs a dedicated change. \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-102-capture-orphan-sweep-commit-race.md b/.abcd/work/issues/resolved/iss-102-capture-orphan-sweep-commit-race.md similarity index 79% rename from .abcd/work/issues/open/iss-102-capture-orphan-sweep-commit-race.md rename to .abcd/work/issues/resolved/iss-102-capture-orphan-sweep-commit-race.md index eadaa42..d66e0ca 100644 --- a/.abcd/work/issues/open/iss-102-capture-orphan-sweep-commit-race.md +++ b/.abcd/work/issues/resolved/iss-102-capture-orphan-sweep-commit-race.md @@ -6,6 +6,8 @@ severity: "minor" category: "bug" source: "agent-finding" found_during: "multi-agent-bughunt" +resolution: "The capture orphan sweep and the commit write now both run under withLedgerLock, so a >60s-stalled commit can no longer land in the sweep's classify-then-unlink window; a raced commit fails loudly instead of the sweep deleting a committed file." +impact: fix --- cleanOrphanPlaceholders (capture/alloc.go) runs locklessly from mutationPreamble at the start of every Capture/Resolve/Wontfix, while commitCapture (workflow.go) fills the reserved placeholder OUTSIDE the ledger flock. A capture stalled >60s between reservation and commit can have its just-committed issue file deleted: the sweep's Lstat passes (size 0, aged) then the stalled commit's atomic rename lands, then the sweep's os.Remove deletes the fully-committed file — after Capture returned success. Deferred from the multi-agent bug hunt (was B26): a pre-unlink re-check (orphanStillRemovable: re-Lstat + SameFile + size==0) already mitigates the common interleaving, but full elimination requires running the sweep AND the commit write under withLedgerLock (the commit write path is in workflow.go). Low severity; data-loss only under a >60s stall plus concurrency. \ No newline at end of file From 853878a92ea3664f86b07f81081bf9d1804ab231 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:16:54 +0100 Subject: [PATCH 4/6] chore: capture iss-127, the repo-local LMW residue 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 --- .../open/iss-127-repo-local-lmw-sites-unguarded.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md diff --git a/.abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md b/.abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md new file mode 100644 index 0000000..5fde996 --- /dev/null +++ b/.abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-127" +slug: "repo-local-lmw-sites-unguarded" +severity: "minor" +category: "tech-debt" +source: "agent-finding" +found_during: "iss-101/102 class sweep (2026-07-24 run queue, burst 3)" +found_at: "internal/core/intent/lifecycle.go" +--- + +repo-local load-modify-write sites remain unguarded after the iss-101/102 class fix: intent transitions (review.go, lifecycle.go — strongest candidate), ahoy config.json stepConfigValues, gitignore/marker block rewrites, and the CHANGELOG release path all do load-mutate-write without a lock; they lack the home-global cross-worktree exposure the fixed sites had, so risk is same-worktree concurrency only \ No newline at end of file From 0576929ad2191c05f2a678d3dd10e482375bbe35 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:21:09 +0100 Subject: [PATCH 5/6] chore: capture iss-128 and iss-129 from the lock-class reviews 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 --- ...128-registerrepo-best-effort-edges-lose-signal.md | 12 ++++++++++++ .../open/iss-129-consolidate-bespoke-flock-loops.md | 12 ++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 .abcd/work/issues/open/iss-128-registerrepo-best-effort-edges-lose-signal.md create mode 100644 .abcd/work/issues/open/iss-129-consolidate-bespoke-flock-loops.md diff --git a/.abcd/work/issues/open/iss-128-registerrepo-best-effort-edges-lose-signal.md b/.abcd/work/issues/open/iss-128-registerrepo-best-effort-edges-lose-signal.md new file mode 100644 index 0000000..c5019b4 --- /dev/null +++ b/.abcd/work/issues/open/iss-128-registerrepo-best-effort-edges-lose-signal.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-128" +slug: "registerrepo-best-effort-edges-lose-signal" +severity: "minor" +category: "bug" +source: "impl-review" +found_during: "iss-101/102 reviews (2026-07-24 run queue, burst 3)" +found_at: "internal/core/ahoy/apply.go" +--- + +registerRepo's best-effort history paths lose signal silently in two edges the iss-101/102 reviews confirmed: (1) a history-lock contention timeout skips registration with no surfaced note (apply.go registerRepo discards the withHistoryLock error); (2) under concurrent double-install of the same new repo with divergent re-founding answers, the lock loser takes the refresh branch which never consults linkLineage, silently dropping a human-approved lineage link with no lineageConflict message \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-129-consolidate-bespoke-flock-loops.md b/.abcd/work/issues/open/iss-129-consolidate-bespoke-flock-loops.md new file mode 100644 index 0000000..57cc612 --- /dev/null +++ b/.abcd/work/issues/open/iss-129-consolidate-bespoke-flock-loops.md @@ -0,0 +1,12 @@ +--- +schema_version: 1 +id: "iss-129" +slug: "consolidate-bespoke-flock-loops" +severity: "minor" +category: "tech-debt" +source: "impl-review" +found_during: "iss-101/102 reviews (2026-07-24 run queue, burst 3)" +found_at: "internal/fsutil/flock.go" +--- + +four bespoke LOCK_EX flock loops remain (memory/writer.go, intent/create.go, spec/store.go, history/store.go) now that fsutil.WithFileLock is the canonical inter-process lock primitive; consolidate them onto it (one-canonical-primitive) — pre-existing, no defect, pure debt \ No newline at end of file From a532da23123f0a9c42f8acc2a66d372765bf646b Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:22:28 +0100 Subject: [PATCH 6/6] docs: correct bootstrap-seed comments to describe write-then-link 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 --- internal/core/ahoy/lockrace_test.go | 7 ++++--- internal/core/ahoy/store.go | 15 ++++++++------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/internal/core/ahoy/lockrace_test.go b/internal/core/ahoy/lockrace_test.go index 364245f..fd60a6a 100644 --- a/internal/core/ahoy/lockrace_test.go +++ b/internal/core/ahoy/lockrace_test.go @@ -63,9 +63,10 @@ func TestRegisterRepoConcurrentDoesNotLoseUpdate(t *testing.T) { } // TestBootstrapHistoryExclusiveCreate is the iss-101 bootstrap check-then-act -// repro. The first bootstrapper pauses just before its create and lets a second -// full bootstrap complete in that window. Post-fix (O_EXCL create) exactly one -// wins the create and the other observes the existing file (no write); pre-fix +// repro. The first bootstrapper pauses just before it publishes and lets a second +// full bootstrap complete in that window. Post-fix the seed is published by an +// atomic hard link (write temp, sync, os.Link), so exactly one link wins and the +// other gets EEXIST and observes the existing file (no write); pre-fix // (check-then-act + last-rename-wins) both report a create. func TestBootstrapHistoryExclusiveCreate(t *testing.T) { home := t.TempDir() diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 0a80f25..a215b21 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -275,13 +275,14 @@ func withHistoryLock(fn func() error) error { } // bootstrapHistory creates ~/.abcd/history/ + index.json when absent. Idempotent. -// The seed is published atomically by an exclusive hard link (iss-101): a -// fully-formed temp file is written, then os.Link'd into place. link fails EEXIST -// if index.json already exists, so under two concurrent bootstraps exactly one -// wins and every other observes the existing file and reports no write — and, -// unlike an O_EXCL create followed by a separate content write, no reader ever -// observes a 0-byte index.json, so a concurrent loadHistoryIndex cannot parse-fail -// and drop its own registration. +// The seed is published atomically (iss-101): a fully-formed temp file is written, +// synced, and os.Link'd into place. The link is the single-winner publish — it +// fails EEXIST if index.json already exists, so under two concurrent bootstraps +// exactly one wins and every other observes the existing file and reports no +// write. Because the file only ever appears complete (the link, never a bare +// create followed by a separate content write), a reader that races the bootstrap +// sees either no file yet or the finished index — never a 0-byte one that would +// make a concurrent loadHistoryIndex parse-fail and drop its own registration. func bootstrapHistory() (bool, error) { root, err := historyRoot() if err != nil {