fix(farm): serialize git worktree registry access so a task stops escalating under load (#515) - #534
Merged
Merged
Conversation
…alating under load (#515) `.git/worktrees/` is a SHARED REGISTRY and git mutates it non-atomically. `git worktree add` scans the existing entries while creating its own, so a second concurrent add can read a sibling's directory after it exists but before its `commondir` is written: fatal: failed to read .git/worktrees/bulk0/commondir: No error prepareWorktree ran `worktree remove` -> rm -> `branch -D` -> `worktree add` with no lock, from up to FARM_CONCURRENCY workers at once. One task would intermittently escalate at attempts:0 and flip a green run's exit code to 2 - #515, reproduced here at 1 of 3 full coverage runs. AC-1, the cause: not mock-server contention and not a gate timeout, which were the issue's other two candidates. It is a genuine race between concurrent git invocations over shared repository state. #518's diagnostic named it on the first reproduction, which is exactly what it was built for. Measured directly against real git, outside the suite: six concurrent prepare sequences over 40 rounds produced 2 failures with that exact signature, and 0 when serialized. AC-3, fixed in farm.ts: worktree creation, removal and prune now run under a dedicated worktreeChain lock, separate from mergeChain so short registry calls never queue behind a long merge; nothing takes both in the opposite order so they cannot deadlock. `prune` is the most dangerous of the three to leave unserialized - it DELETES sibling metadata directories, which is precisely what a concurrent add is scanning. AC-4 is satisfied rather than dodged: FARM_CONCURRENCY stays 6 and parallelism is unchanged. The worker API call, the gate and the tests all still run concurrently; only the brief registry mutations queue, and the retry backoff runs OUTSIDE the lock so one worker's retry schedule cannot stall the others. prepareWorktree now takes an injectable git runner, defaulted, for the same reason removeWorktreeVerified already did: it is the function the race lives in, and a test that cannot observe its git calls cannot prove they are serialized. Mutation testing is what surfaced that - three of four mutants initially survived, including one that removed the lock from prepareWorktree entirely. The tests assert the PROPERTY - no two registry commands ever in flight at once - rather than the absence of the flake, which would have passed ~95% of the time on the unfixed dispatcher. 4/4 mutants killed. Also pins LF for the TS/JS/JSON plugin and site trees. Not cosmetic: an editor pass flipped farm.ts and farm.unit.test.ts to CRLF, git read both entire files as ADDED, and the H-09b backstop scans added lines for sensitive patterns - so pre-existing hash calls and credential-shaped test fixtures counted as newly introduced, and this commit was blocked demanding a security pass for a change touching none of that. #533 pinned the workflow tree for the cosmetic half of this; this is the half with teeth. ca 2.10.0 -> 2.10.1: farm.js is a declared shipped artifact, so this is a payload change and the gate landed in #532 requires the manifest to advance. Claude-Session: https://claude.ai/code/session_01WJgVfZw7J81PB7mwpHyUxx
Machine-written audit sink; append-only per H-05. Claude-Session: https://claude.ai/code/session_01WJgVfZw7J81PB7mwpHyUxx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #515.
AC-1 — the cause
Neither of the issue's leading candidates. Not mock-server contention, not a gate timeout under load: it is a race between concurrent
gitinvocations over shared repository state..git/worktrees/is a shared registry and git mutates it non-atomically.git worktree addscans the existing entries while creating its own, so a second concurrent add can read a sibling's directory after it exists but before itscommondiris written.prepareWorktreeranworktree remove→rm→branch -D→worktree addwith no lock, from up toFARM_CONCURRENCYworkers at once.Reproduced at 1 of 3 full coverage runs. #518's diagnostic named it on the first reproduction — exactly what it was built for:
Then measured directly against real git, outside the suite entirely — six concurrent prepare sequences, 40 rounds each way:
AC-3 — fixed in
farm.tsWorktree creation, removal and prune now run under a dedicated
worktreeChainlock.mergeChaindeliberately. A merge holds its lock across a git merge plus its gate, which is long; registry calls are short. Sharing one lock would park every worktree setup behind an unrelated merge for no correctness gain. Nothing takes both in the opposite order, so they cannot deadlock.pruneis the most dangerous of the three to leave unserialized — it deletes sibling metadata directories, which is precisely what a concurrentaddis scanning.AC-4 — satisfied, not dodged
FARM_CONCURRENCYstays at 6 and the dispatcher's parallelism is unchanged. The worker API call, the gate and the tests — everything that actually takes time — still run concurrently. Only the brief registry mutations queue.Verification
Post-fix: 4/4 coverage runs green (the mode that reproduced it), full suite 521 passed.
The tests assert the property — no two registry commands ever in flight at once — rather than the absence of the flake. Asserting "the suite went green" would have passed ~95% of the time on the unfixed dispatcher and proved nothing.
4/4 mutants killed:
lock-is-a-passthrough,chain-head-never-advances,prepare-drops-the-lock,remove-drops-the-lock.What mutation testing caught
Three of four mutants survived the first round, including one that removed the lock from
prepareWorktreeentirely — the function the bug is actually in. Every test I had written exercisedremoveWorktreeVerifiedand the lock helper directly, and none of them touchedprepareWorktree, which used a module-scopegitand so could not be observed at all.It now takes an injectable runner, defaulted, for the same reason
removeWorktreeVerifiedalready did. Two other survivors were genuinely equivalent mutants and are documented as such in the runner rather than papered over with a test; one test that could not distinguish its own mutant was deleted rather than kept.Line endings — the half with teeth
This commit was blocked by H-09b demanding a crypto-compliance pass, for a concurrency fix that touches no crypto.
An editor pass flipped
farm.tsandfarm.unit.test.tsto CRLF. Git then read both entire files as added — and the H-09b backstop scans added lines for sensitive patterns. Every pre-existing hash call and every credential-shaped test fixture in those files counted as newly introduced.README.mdwas immune, and its diff stayed at 2 lines whilefarm.ts's was 6165 — because/*.mdwas already pinned in.gitattributesand the.tstrees were not.So
.gitattributesnow pins LF for the TS/JS/JSON plugin and site trees. #533 pinned the workflow tree for the cosmetic half of this; this is the half that fires gates on code nobody wrote. Verified before adding: no committed file under those globs is CRLF today, so it normalizes nothing retroactively.Version
ca2.10.0 → 2.10.1.farm.jsis a declared shipped artifact, so this is a payload change and the gate landed in #532 requires the manifest to advance — its first real exercise, and it behaved correctly.