Repository navigation
fix(prune): serialize worktree-registry teardowns behind a lock - #3692
Conversation
`git worktree remove` enumerates every entry under `.git/worktrees/` and reads each sibling's `commondir` while resolving its target. When `wt step prune` runs several removals at once, one worker can read a sibling's `commondir` in the window after a concurrent worker has begun tearing that sibling's admin dir down — a git-level TOCTOU between the enumerator's readdir and its open. The same window fails a branch delete's `list_worktrees` checkout probe (`Invalid path .../.git/worktrees/<id>`). Both surfaced in CI (#3661). Add a second lock, `RemovalContext::registry_lock`, that serializes the registry-mutating removals: a removal that unregisters an entry (`git worktree remove` — every `Worktree` plan, a `BranchOnly` plan with a stale `prune_entry`, and `StaleDetached`) holds the write side, while a removal that only reads the registry (a plain branch deletion's checkout probe) holds the read side, so those still run concurrently. Kept distinct from `check_lock` on purpose: the integration-check fan-out never enumerates the registry live, so it keeps overlapping removals rather than serializing behind them — the `.git/config`-race canary's premise is preserved. Removals acquire `check_lock` then `registry_lock` in a fixed order, so the two never deadlock. `test_prune_metadata_removals_run_concurrently` is reworked into `test_prune_metadata_removals_serialize`: its shim now probes for teardown overlap with an atomic `mkdir` lock and asserts none occurs (verified to fail when the write lock is downgraded to a read lock). Closes #3661
The registry-lock change made teardowns serialize, but this docstring still claimed concurrent prunes are safe against each other and the scan — the exact invariant the fix overturns — and cross-referenced a section that now says the opposite. Correct it to point at registry_lock.
worktrunk-bot
left a comment
There was a problem hiding this comment.
Independent review of the concurrency change. No defect found — the two-lock design holds up under tracing — but flagging for maintainer sign-off per the repo's data-safety policy, since this reworks the wt step prune worktree-removal (git worktree remove) scheduling path (src/commands/step/prune.rs, src/git/repository/worktrees.rs).
What I verified:
- No deadlock. Every removal acquires
check_lockbeforeregistry_lockin a fixed order (try_remove'sStaleDetachedbranch and its main branch both do so), so the two locks can't deadlock. - Every live registry enumeration is covered. The only operations that walk
.git/worktrees/live during the fan-out are thegit worktree removeteardowns (prune_worktree_entry/remove_worktree) andfresh_branch_checkout'slist_worktreesprobe (the freshgit worktree list --porcelainindelete_branch_if_safe). Teardowns holdregistry_lockwrite; the plain branch-delete probe holds read; both run synchronously inside the guard scope (prune_worktree_entryandremove_worktreeare blockingrun_commands, and the fast path'sstage_worktree_removalprunes inline before backgrounding only therm -rf). The integration check andensure_clean'sgit statusdon't enumerate siblings, so leaving the check fan-out on the read side without serializing behindregistry_lockis sound. - The reworked test is a genuine regressor.
test_prune_metadata_removals_serialize'smkdir-based overlap probe fires only if two teardown subprocesses run concurrently, which the write guard prevents; downgrading it to a read guard would drop theoverlap-sentinel the test asserts absent.
The one load-bearing assumption worth a second pair of eyes: that the integration-check fan-out never enumerates the registry live (the premise for keeping it overlapping removals rather than serializing behind registry_lock, and for preserving the .git/config-race canary). It held everywhere I traced, but it's the assumption that would make the fix incomplete if wrong.
Not approving (self-authored, and data-loss surface).
`wt step prune` currently holds its worktree-registry lock across each complete removal to avoid Git's `.git/worktrees/` enumeration race. That also serializes safe work such as dirty checks, fsmonitor shutdown, and renaming the worktree into trash. This moves coordination into the dedicated `Repository` operations, keyed by canonical Git common directory. Fresh `git worktree list` calls take the read side, while targeted and direct `git worktree remove` calls take the write side. Removal staging remains concurrent, and the synthesized-`--force` submodule fallback keeps its final dirty check and destructive command in one critical section. The canonical `prune_e2e/live` benchmark improved from a 376.61 ms median on current main to 221.83 ms here, about 41%. Existing prune tests cover concurrent scheduling and serialized metadata teardown. Focused Rust tests verify lock sharing across distinct worktree discovery paths and that the fast-path rename precedes registry teardown serialization. Follow-up to #3661 and #3692. > _This was written by Codex on behalf of max-sixty_
worktrunk 0.72.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Release Notes ### Improved - **A host carrying a forge's name anywhere resolves to that forge again**: 0.71.0 required `github`, `gitlab`, or `gitea` as a whole dot-separated label, which read as an ownership check but wasn't one — an attacker controls their own DNS — while shutting out self-hosters with hyphenated names. `github-enterprise.acme.com`, `mygithub.life-white.uk`, and the `github-personal` SSH alias classify again, so CI status, `wt switch --prs`, and `repo.provider` work with no config. ([#3673](max-sixty/worktrunk#3673)) - **One `[projects."…"]` entry can cover every repository on a host, and can set the forge**: A key containing `*` matches any run of characters, `/` included, so `[projects."git.company.example/*"]` covers a whole host; every matching entry applies, least- to most-specific. The table also gained `forge.platform` and `forge.hostname`, so a self-hosted host needs one entry here rather than a block in every repo. [Docs](https://worktrunk.dev/config/#user-project-specific-settings) ([#3701](max-sixty/worktrunk#3701), thanks @chrishas35 for the request and @witt-bit for the workspace-scoped case it partly serves) - **`wt merge` and `wt step push` leave the target worktree's uncommitted changes in place**: The autostash that held a dirty target's changes restored them as unstaged; both now advance the target with a compare-and-swap `update-ref` and `read-tree -m -u`, which leaves uncommitted work untouched. (Breaking: the fast-forward path no longer runs `git push`, so `pre-push` and the receive-side hooks no longer fire, and a failed sync errors with the ref rolled back.) ([#3703](max-sixty/worktrunk#3703), [#3684](max-sixty/worktrunk#3684), [#3693](max-sixty/worktrunk#3693), thanks @gubasso for reporting) - **Approval state and branch-removal outcomes are machine-readable**: `wt config approvals list --format=json` reports whether a non-interactive run would stop for approval. `wt remove` and `wt step prune` replace `branch_deleted` with `branch_outcome`: `deleted`, `deferred`, `not_attempted`, `retained_unmerged`, `retained_checked_out`, `retained_raced`, `retained_failed`. (Breaking.) ([#3710](max-sixty/worktrunk#3710), thanks @NathanaelRea for the requests) - **Every commit hash worktrunk prints follows `core.abbrev`**: The `wt list` table, `wt switch --prs`'s `log` tab, and `wt config state`'s CI cache table sliced to 8 characters while `--format=json` carried git's `%h`. All now ask git how wide it abbreviates in this repo. ([#3676](max-sixty/worktrunk#3676), [#3677](max-sixty/worktrunk#3677)) - **`wt list --format=json` schema 2 has a published JSON Schema**: [worktrunk.dev/schema/list-v2.json](https://worktrunk.dev/schema/list-v2.json) holds the contract, and `wt list --print-schema` prints the same document. Four fields that were bare strings are now enumerated vocabularies; the emitted JSON is unchanged. ([#3747](max-sixty/worktrunk#3747)) - **A detached worktree is named by its commit, not `-`**: The Branch cell hardcoded `-`, which reads as missing data rather than a state; it now carries the row's abbreviated HEAD, and the picker and statusline name the worktree the same way. ([#3675](max-sixty/worktrunk#3675)) ### Fixed - **Piped output is plain and no longer panics**: `wt list | head -3` exited 101 with `failed printing to stdout: Broken pipe`, and `wt list` wrote ANSI to a pipe unconditionally. Every stdout surface now exits cleanly, and the human-read ones are plain when piped unless `CLICOLOR_FORCE=1`. ([#3746](max-sixty/worktrunk#3746), [#3766](max-sixty/worktrunk#3766)) - **A CI check that hasn't finished no longer reads as passed**: Each forge's status parser missed documented values, so a GitHub PR parked on an approval gate showed green, and GitLab's `canceling` and Azure DevOps's `postponed` read as no CI at all. ([#3741](max-sixty/worktrunk#3741), [#3740](max-sixty/worktrunk#3740)) - **The shell wrappers survive an `rm` alias and a failing `--execute`**: Aliases bake into the wrapper at parse time, so `alias rm='rm -v'` reached its cleanup — noise on zsh and bash, and on nushell an abort that leaked three temp files, as a failing `--execute` body also did. ([#3714](max-sixty/worktrunk#3714), thanks @Ar4l), ([#3732](max-sixty/worktrunk#3732), [#3734](max-sixty/worktrunk#3734)) - **An alias or hook wrapping `wt switch` or `wt remove` keeps your subdirectory**: The user's position came from the `wt` process's cwd, which inside an alias body is the worktree root, so an aliased `wt remove` from `feature/apps/gateway` landed at the primary worktree's root. Fixes [#3723](max-sixty/worktrunk#3723). ([#3724](max-sixty/worktrunk#3724), thanks @vivienm for reporting) - **`wt merge` and `wt step push` refuse a target worktree parked mid-operation**: The two-tree sync refuses an unmerged index but not a stopped cherry-pick or rebase whose conflict was already staged, so the push range could land in a paused target and be committed by `--continue`. ([#3759](max-sixty/worktrunk#3759)) - **The Claude plugin's worktree-remove hook resolves against the worktree path**: The hook anchored at `CLAUDE_PROJECT_DIR`, which the `claude agents` view routinely leaves outside any repository, so `wt remove` died with `not a git repository` and the session became undeletable. Its guard now also requires a `.git` entry. ([#3754](max-sixty/worktrunk#3754), [#3767](max-sixty/worktrunk#3767), thanks @judewang for the fix and the report) - **A Gitea API error is reported as one, not as a parse failure**: `tea api` exits 0 whatever the HTTP status, so both call sites guessed from the body's shape and blamed an API change for an API error. `--include` surfaces the status instead. ([#3713](max-sixty/worktrunk#3713), [#3600](max-sixty/worktrunk#3600)) - **`--print-schema` and the doc-generation help flags name the right command**: All three found the subcommand by scanning `argv` for a `/wt` suffix, which never matches `wt.exe` under a backslash path, so on Windows they read the binary's own path as the command. ([#3762](max-sixty/worktrunk#3762)) - **A multibyte shell name no longer panics**: `extract_filename_from_path` sliced at `len() - 4` to test for `.exe` with no char-boundary check: `SHELL=/bin/日本語 wt config show` panicked, and on macOS every process name goes through it during shell detection. ([#3727](max-sixty/worktrunk#3727)) - **`wt` installed under a dotted name generates shell integration for that name**: `binary_name` used `file_stem`, which cuts at the last dot, so `wt config shell init bash` under `wt.old` emitted a wrapper for `wt`. It now strips only the executable suffix. ([#3719](max-sixty/worktrunk#3719)) - **Concurrent `wt step prune` removals no longer race the worktree registry**: `git worktree remove` reads every sibling under `.git/worktrees/`, so two overlapping removals could have one read a sibling mid-teardown. Registry-mutating removals now serialize behind a second lock. ([#3692](max-sixty/worktrunk#3692)) - **The `wt switch` first-run offer previews the legacy files it removes**: Accepting "Install shell integration?" could delete a deprecated worktrunk-managed wrapper the prompt never named. What gets removed is unchanged. ([#3656](max-sixty/worktrunk#3656)) - **`wt config create --project` writes a resolvable link**: The comment it writes into `.config/wt.toml` carried a raw Zola target, because the link-conversion regex stopped at the first `]` — here the one closing a nested code span. ([#3731](max-sixty/worktrunk#3731)) - **`wt list --branches` counts a local branch containing `/` as local**: The summary tally classified branch-only rows by `branch.contains('/')`, so a local `feature/login` counted under "N remote branches". ([#3687](max-sixty/worktrunk#3687)) - **`wt step relocate`'s human summary counts template-error branches as skipped**: `--format=json` already folded them into `skipped`; the human tally undercounted by the number of branches whose `worktree-path` template failed to expand. ([#3688](max-sixty/worktrunk#3688)) ### Documentation - **SignPath attribution appears with the artifacts it describes**: The code-signing notice and a route to the policy now sit in the install section's Windows block on the README and the docs landing page, as SignPath Foundation's OSS program requires. ([#3709](max-sixty/worktrunk#3709)) - **`wt step copy-ignored`'s `--require-include` example renders as a terminal block**: It was the only `console` block in the command's long help missing the `$ ` prefix. ([#3706](max-sixty/worktrunk#3706)) ### Internal - **Library API rework** (Breaking library API): `cargo-semver-checks` fails five lints — `LegacyForgeAlias` and `Repository::legacy_forge_alias` removed with the forge-classification revert, `Repository::forge_platform_override` removed for one shared resolver, `stage_worktree_removal` gained two parameters, `UserProjectOverrides` gained a `forge` field, and the `real-repo-benches` feature was removed. ([#3673](max-sixty/worktrunk#3673), [#3694](htt See merge request: Harmonybrew/homebrew-core!16275
Problem
Concurrent
wt step pruneremovals race at the git level on the worktree registry (issue #3661).git worktree removeenumerates every entry under.git/worktrees/and reads each sibling'scommondirwhile resolving its target, so when two removals overlap, one can read a sibling'scommondirin the window after a concurrent worker has begun tearing that sibling's admin dir down — a TOCTOU between the enumerator'sreaddirand itsopen. The same window fails a branch delete'slist_worktreescheckout probe withInvalid path .../.git/worktrees/<id>. Both surfaced in CI: thecommondirfailure in the flaky coverage run, theInvalid pathfailure in the nightly benchmarks run.Per the maintainer's call on #3661: serialize now, move things off the serial path later.
Solution
A second lock,
RemovalContext::registry_lock, serializes the registry-mutating removals:git worktree remove— everyWorktreeplan, aBranchOnlyplan carrying a staleprune_entry, andStaleDetached) holds the write side.list_worktreescheckout probe) holds the read side, so those still run concurrently.Kept distinct from the existing
check_lockon purpose: the integration-check fan-out never enumerates the registry live (it plans off the cachedlist_worktreessnapshot), so it keeps overlapping removals rather than serializing behind them — which preserves the.git/config-race canary's premise. Removals acquirecheck_lockfirst, thenregistry_lock, in a fixed order, so the two never deadlock.This gives up the parallel prune of many stale entries (a cheap, rare operation) in exchange for correctness, and avoids parsing git's raw
fatal:strings — the tradeoff the issue laid out for option (1).Testing
test_prune_metadata_removals_run_concurrentlyis reworked intotest_prune_metadata_removals_serialize: the shim now probes for teardown overlap with an atomicmkdirlock and asserts none occurs. Verified it's a genuine regression detector — downgrading the write guard to a read guard makes it fail withtwo git worktree remove teardowns overlapped. The concurrency-preserving tests still pass unchanged:test_prune_removals_run_concurrently(branch-only removals stay parallel) andtest_prune_fallback_config_race_canary(removals still overlap the scan). Fullstep_prunemodule green (51 tests); clippy + fmt clean.Closes #3661