Exclude helper's read-modify-write is unserialized: concurrent worktree provisioning silently loses patterns

[pbean]

Found while reviewing #376. That PR made the exclude write atomic (atomic_write_text), which closes the truncation hole (#375) and the fixed-.tmp-name collision. It does not — and cannot — close the read-modify-write around it.

install._worktree_local_exclude reads the exclude, merges the new patterns, and writes the whole file back:

existing = exclude.read_text(encoding="utf-8") if exclude.is_file() else ""   # install.py:793
present = set(existing.splitlines())
new = [p for p in patterns if p not in present]
...
atomic_write_text(exclude, prefix + "\n".join(new) + "\n")                     # install.py:812

The read and the write are not serialized. Atomicity per write is not isolation across the pair, so two provisioning runs that interleave produce a clean lost update.

This is specifically reachable here rather than theoretical: every linked worktree of a repo resolves to the same --git-common-dir, so all worktrees of one repo contend on one .git/info/exclude. The comment at install.py:806-811 posits exactly this scenario ("two runs provisioning against one repo") as the motivation for the unique temp name.

Reproduction

Two linked worktrees of one repo; wt2's full cycle interleaved between wt1's read and wt1's write:

wt1 returned: None
/from-wt1 present: True
/from-wt2 present: False   <-- lost

Both calls returned None. Script:

import subprocess, sys, tempfile
from pathlib import Path
sys.path.insert(0, "src")
from bmad_loop import install

d = Path(tempfile.mkdtemp())
repo = d / "repo"; repo.mkdir()
run = lambda *a: subprocess.run(a, cwd=str(repo), check=True, capture_output=True)
run("git", "init", "-q"); run("git", "config", "user.email", "t@t"); run("git", "config", "user.name", "t")
(repo / "f").write_text("x"); run("git", "add", "-A"); run("git", "commit", "-qm", "init")
run("git", "worktree", "add", "-q", "-b", "wt1", str(d / "wt1"))
run("git", "worktree", "add", "-q", "-b", "wt2", str(d / "wt2"))

orig = install.atomic_write_text
def interleaved(path, text):
    install.atomic_write_text = orig                       # wt2 runs uninterrupted
    install._worktree_local_exclude(d / "wt2", ["/from-wt2"])
    return orig(path, text)                                # wt1 writes from its stale read
install.atomic_write_text = interleaved
print("wt1 returned:", install._worktree_local_exclude(d / "wt1", ["/from-wt1"]))
install.atomic_write_text = orig

body = (repo / ".git" / "info" / "exclude").read_text()
print("/from-wt1:", "/from-wt1" in body, "| /from-wt2:", "/from-wt2" in body)

Why the silence is the harmful part

The helper's docstring is explicit about what a missing exclude costs:

Silence here is not cosmetic: without the exclude the unit's git add -A commits the tool files this provisioning just wrote into the story's merge.

A lost update produces precisely that outcome, with a None return — the "nothing to report" signal — on both sides. The degrade-reason channel this helper exists to feed never fires.

Scope / relationship to other issues

• Not a regression from _worktree_local_exclude's exclude write is not atomic: a short write corrupts the shared exclude #375 or fix(install): close the exclude helper's two residual holes (#374, #375) #376: write_text lost the update the same way. The PR narrows the window (the write is now one os.replace) but the race is between the read and the write, so narrowing does not remove it.
• Not covered by Non-atomic read-modify-rewrite in ~10 places; a short write truncates ledgers, specs, and the board #379, which is scoped to truncation from non-atomic rewrites and states "none introduced by fix(install): guard the worktree exclude's filesystem tail (#359) #373 or fix(install): close the exclude helper's two residual holes (#374, #375) #376". Swapping a site to atomic_write_text fixes Non-atomic read-modify-rewrite in ~10 places; a short write truncates ledgers, specs, and the board #379's defect and leaves this one. Worth checking the Non-atomic read-modify-rewrite in ~10 places; a short write truncates ledgers, specs, and the board #379 sites for read-modify-write pairs once they are converted — deferredwork (next_seq() derives ids from surviving text) and sprintstatus (the board) look like the same shape.

Reachability

No in-process concurrency: engine.py / worktree_flow.py run no threads or pools, so a single bmad-loop run cannot hit this. It needs two bmad-loop run processes against one repo — the scenario the code comment itself posits.

Fix shapes

1. Lock the common dir — O_EXCL lockfile next to the exclude (git's own .lock convention), read-merge-write inside it, best-effort on failure to stay consistent with the helper's contract. Matches what git does to the same directory.
2. Retry on changed mtime/size — re-read and re-merge if the file moved under us. Cheaper, still racy in principle, but the merge is idempotent (set union), so a bounded retry converges.
3. Append instead of rewrite — open("a") a single anchored block. Sidesteps the read entirely for the common case, but loses dedup against existing lines and cannot be made idempotent across runs.

(1) is the honest fix; (2) is the cheap one. Either way, a lost update should surface through the existing degrade-reason channel rather than returning None.

[coderabbitai]

🔗 Related PRs

#103 - fix(platform): overwrite-safe deferred stash + ref-safe unit branch names [merged]
#182 - fix(worktree): degrade post-merge teardown failures instead of crashing the run (#139) [merged]
#233 - fix: report worktree_seed entries skipped as no-ops [merged]
#257 - feat(worktree): seed the absent children of an existing seed dir (#235) [merged]
#344 - fix(verify): translate spawn OSError to GitSpawnError in _run_git (#343) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[pbean]

Status update from #385 (open, stacked follow-up in #386): this race is mooted for all new writes, though not by serializing anything.

The reachability argument in the report was that every linked worktree of a repo resolves to the same --git-common-dir, so all worktrees of one repo contend on one .git/info/exclude. #385 removes that premise entirely. The shield now writes <worktree gitdir>/info/exclude — .git/worktrees/<id>/info/exclude — activated with a worktree-scoped core.excludesFile. The two runs that used to race here no longer share a target at all; each writes a file inside its own worktree's private gitdir. The repository-wide .git/info/exclude is never written by bmad-loop again, on any path (a fallback to it would reintroduce #384, so there deliberately isn't one).

The read-modify-write itself is unchanged and still not serialized — worth being precise about, since "mooted" is not "fixed". What changed is that it no longer has a second writer:

• Only a second provisioning of the same worktree can reach a given private exclude, and the engine does that serially.
• atomic_write_text stays for _worktree_local_exclude's exclude write is not atomic: a short write corrupts the shared exclude #375, whose fault is a short write and needs no second writer.

So the lost-update reproduction in this issue is no longer constructible through provision_worktree once #385 lands — the two worktrees in that script would write two different files. #385's _worktree_local_exclude docstring records this reasoning inline so it does not have to be rediscovered.

What remains in scope for the shared file is read-only: #386 adds a validate warning that reads .git/info/exclude and reports pre-#384 shield patterns still sitting in it. It never writes or removes them (line authorship is ambiguous), so it introduces no new contention on that file.

Whether to close this or keep it open for the residual same-worktree re-provision case is a maintainer call — flagging rather than deciding, since the RMW pattern is still there and only the concurrency that made it reachable is gone.