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 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 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 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..fd60a6a --- /dev/null +++ b/internal/core/ahoy/lockrace_test.go @@ -0,0 +1,214 @@ +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 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() + 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..a215b21 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,119 @@ 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 (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 { 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 { 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 + } + } +}