From 7eca6d65a39193687133793a206fa3b2eba986ea Mon Sep 17 00:00:00 2001 From: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:36:42 +0000 Subject: [PATCH 1/2] fix(prune): serialize worktree-registry teardowns behind a lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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/`). 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 --- src/commands/step/prune.rs | 77 +++++++++++++++++++++++++-- src/git/repository/worktrees.rs | 30 +++++++---- tests/integration_tests/step_prune.rs | 73 +++++++++++++------------ 3 files changed, 130 insertions(+), 50 deletions(-) diff --git a/src/commands/step/prune.rs b/src/commands/step/prune.rs index b09614496..066a2e1b3 100644 --- a/src/commands/step/prune.rs +++ b/src/commands/step/prune.rs @@ -6,9 +6,14 @@ //! sized like rayon's ([`RemovalJob`]). Checks and hook-free removals hold //! the read side of [`RemovalContext::check_lock`] and run concurrently; the //! exceptional removals serialize on the write side -//! ([`removal_needs_write`]). One FIFO queue carrying both removals and skip -//! lines means a single worker (`RAYON_NUM_THREADS=1`) reproduces the serial -//! total order the deterministic-output tests pin. The first failing removal +//! ([`removal_needs_write`]). A second lock, [`RemovalContext::registry_lock`], +//! serializes worktree-registry teardowns (`git worktree remove`) against each +//! other and against concurrent registry reads ([`removal_mutates_registry`]) — +//! a git-level TOCTOU on `.git/worktrees/` that the two-locks split keeps out +//! of the integration-check fan-out's way (issue #3661). One FIFO queue carrying +//! both removals and skip lines means a single worker (`RAYON_NUM_THREADS=1`) +//! reproduces the serial total order the deterministic-output tests pin. The +//! first failing removal //! flips an abort flag that drains the remaining queue unexecuted; the //! current worktree is removed last, after the fan-out, because its removal //! cd's the shell to the primary. @@ -235,6 +240,25 @@ struct RemovalContext<'a> { /// unreachable here because the chain captures the snapshot immediately /// before consulting it.) check_lock: &'a RwLock<()>, + /// Serializes access to the worktree registry (`.git/worktrees/`), + /// independently of `check_lock`. + /// + /// `git worktree remove` enumerates *every* sibling entry and reads each + /// one's `commondir` while resolving its target, so two overlapping + /// teardowns — or a teardown overlapping a branch delete's + /// `list_worktrees` checkout probe — can read an entry another worker is + /// mid-way through deleting and fail (`failed to read …/commondir` / + /// `Invalid path …/.git/worktrees/`). That is a genuine git-level + /// TOCTOU, not a wt bug (issue #3661). A removal that unregisters an entry + /// ([`removal_mutates_registry`]) holds the write side; a removal that only + /// reads the registry 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 (it plans off the cached + /// `list_worktrees` snapshot), so it must keep overlapping removals rather + /// than serialize behind them. Removals acquire `check_lock` first, then + /// `registry_lock`, so the two never deadlock. + registry_lock: &'a RwLock<()>, } /// Which removals must hold the write side of [`RemovalContext::check_lock`] @@ -281,6 +305,25 @@ fn removal_needs_write(kind: CandidateKind, plan: &RemovalPlan, ctx: &RemovalCon } } +/// Whether a removal's execution unregisters a worktree entry — a `git worktree +/// remove` teardown, which enumerates every sibling's `commondir` and so must +/// hold the write side of [`RemovalContext::registry_lock`]. +/// +/// A `Worktree` plan always tears down (either the rename fast path's scoped +/// prune or the direct `git worktree remove` fallback); a `BranchOnly` plan +/// tears down only when it carries a stale `prune_entry`. A plain `BranchOnly` +/// deletion touches the registry just to read it (`list_worktrees`, to refuse +/// deleting a still-checked-out branch) and then deletes the ref with a CAS +/// `update-ref -d` that never reads the registry, so it takes the read side and +/// keeps running concurrently with its peers. `StaleDetached` never reaches +/// here — [`try_remove`] prunes its entry directly under the write side. +fn removal_mutates_registry(plan: &RemovalPlan) -> bool { + match plan { + RemovalPlan::Worktree { .. } => true, + RemovalPlan::BranchOnly { prune_entry, .. } => prune_entry.is_some(), + } +} + /// Try to remove a candidate immediately. Returns `Ok(Some(branch_deleted))` /// if removed — the executed outcome the summary counts — `Ok(None)` if the /// removal turned out to be a no-op, `Err` on execution error. @@ -300,9 +343,14 @@ fn try_remove( let _span = Span::new(format!("prune-remove:{}", candidate.label)); if matches!(candidate.kind, CandidateKind::StaleDetached) { - // The scoped metadata prune is read-side-safe (see - // `prune_worktree_entry`), so hold the parallel default. + // Output side: no exclusive output here (no spinner, no hook stream), + // so join the parallel read side of `check_lock`. let _read = ctx.check_lock.read().unwrap_or_else(|e| e.into_inner()); + // Registry side: `git worktree remove` on this stale entry is a + // teardown that enumerates every sibling's `commondir`, so hold the + // write side of `registry_lock` — no concurrent teardown or registry + // read may overlap it (issue #3661). + let _registry = ctx.registry_lock.write().unwrap_or_else(|e| e.into_inner()); // Name the stale entry rather than sweeping the repository, so a // sibling whose directory is merely absent right now (unmounted // volume, half-finished `mv`) keeps its registration. `gather_check_items` @@ -335,6 +383,23 @@ fn try_remove( None, ) }; + // Registry side, acquired *after* `check_lock` (fixed order → no deadlock). + // A removal that unregisters a worktree entry (`git worktree remove`) takes + // the write side so it can't overlap another teardown's `commondir` read or + // a branch delete's `list_worktrees` probe; one that only reads the registry + // takes the read side and keeps running concurrently. See `registry_lock`'s + // spec and `removal_mutates_registry` (issue #3661). + let (_registry_read, _registry_write) = if removal_mutates_registry(&plan) { + ( + None, + Some(ctx.registry_lock.write().unwrap_or_else(|e| e.into_inner())), + ) + } else { + ( + Some(ctx.registry_lock.read().unwrap_or_else(|e| e.into_inner())), + None, + ) + }; let mut announcer = HookAnnouncer::new(ctx.repo, true); // `SynchronousForNonCurrent`: a rename-failure fallback completes inline, @@ -1050,11 +1115,13 @@ pub fn step_prune( let mut skipped_approval: Vec = Vec::new(); let check_lock = RwLock::new(()); + let registry_lock = RwLock::new(()); let removal_ctx = RemovalContext { repo: &repo, foreground, hook_plan: &hook_plan, check_lock: &check_lock, + registry_lock: ®istry_lock, }; // Flipped by the first failing removal: the rest of the queue drains // without executing (matching the serial loop's abort-on-first-error), diff --git a/src/git/repository/worktrees.rs b/src/git/repository/worktrees.rs index 625f1b181..4476c9339 100644 --- a/src/git/repository/worktrees.rs +++ b/src/git/repository/worktrees.rs @@ -152,19 +152,27 @@ impl Repository { /// /// # Concurrent calls /// - /// `wt step prune` calls this from several removals at once. Each names one - /// entry, so no call deletes another's `.git/worktrees/`. Git also - /// `rmdir`s the containing `.git/worktrees` once the last entry goes, but - /// that only succeeds on an already-empty directory: `git worktree remove` - /// enumerates the directory before deleting its own entry, and every + /// `wt step prune` removes several entries at once, but **serializes the + /// teardowns** — this and every other `git worktree remove` — behind + /// `RemovalContext::registry_lock` (see the `prune` module). It has to: + /// naming one entry bounds what a call *deletes*, not what it *reads*. + /// `git worktree remove` enumerates *every* sibling under `.git/worktrees/` + /// and reads each one's `commondir` while resolving its target, so a + /// teardown overlapping another worker's teardown — or a branch delete's + /// `list_worktrees` probe — can read an entry mid-deletion and fail + /// (`failed to read …/commondir` / `Invalid path …/.git/worktrees/`). + /// That is git's own TOCTOU between the enumerator's `readdir` and its + /// `open`; it holds however wt schedules its removals, so wt closes the + /// window by not letting two registry mutations overlap (issue #3661). + /// + /// Git also `rmdir`s the containing `.git/worktrees` once the last entry + /// goes, but that only succeeds on an already-empty directory, and every /// per-worktree command in the removal chain (`git status`, the fsmonitor /// stop) runs while that worktree is still registered, so an emptied - /// directory has no in-flight reader left to strand. - /// - /// A `git worktree list` in another process is the one thing that - /// disappearing directory can still fail (`fatal: Invalid path - /// '…/.git/worktrees'`, between its readdir and its realpath). That is - /// git's own race, and it holds however wt schedules its removals. + /// directory has no in-flight reader left to strand. A `git worktree list` + /// in an *unrelated* process — outside wt's lock — remains exposed to the + /// same `Invalid path` race; wt's serialization only covers its own + /// removals. pub fn prune_worktree_entry(&self, path: &Path) -> anyhow::Result<()> { // Every caller's path came from `list_worktrees`, which parses git's // porcelain as UTF-8, so this only fires if that edge ever stops diff --git a/tests/integration_tests/step_prune.rs b/tests/integration_tests/step_prune.rs index 94df80268..16d7808ed 100644 --- a/tests/integration_tests/step_prune.rs +++ b/tests/integration_tests/step_prune.rs @@ -1711,25 +1711,29 @@ fn test_prune_removals_run_concurrently(repo: TestRepo) { } } -/// The removals that unregister stale worktree metadata are on the read side -/// too, not held back one at a time. +/// The removals that unregister stale worktree metadata serialize behind +/// `registry_lock` — one `git worktree remove` teardown at a time. /// /// Four stale entries: two carrying a branch (`BranchOnly` plans whose /// `prune_entry` executes the prune) and two detached (`StaleDetached`, which /// prune in place of a removal). Each unregisters its own metadata with -/// `git worktree remove `, and the shim makes every one of them wait for -/// all four to start before proceeding. Serialized, the first would wait out -/// the shim's 15 s timeout and drop a sentinel file, which the test asserts -/// absent. Unix-only for the same `CreateProcess` shim reason as the canary +/// `git worktree remove `, which enumerates every sibling's `commondir` +/// as it resolves its target — so two overlapping teardowns can read an entry +/// another worker is mid-deleting and fail (issue #3661). The shim probes for +/// that overlap with an atomic `mkdir` lock held across a fixed window around +/// each teardown; serialized, no two ever hold it at once, so the test asserts +/// no `overlap-` sentinel appears (and every entry still pruned). If the lock +/// regressed, all four teardowns would fire at once and three would collide in +/// the window. Unix-only for the same `CreateProcess` shim reason as the canary /// above. /// -/// `--foreground` runs both ways too. It reserves the write side for the TTY -/// trash-cleanup spinner, which only a worktree removal paints; every candidate -/// here plans a pure branch deletion or a bare prune, so the flag has no -/// spinner to protect and must not serialize them. +/// `--foreground` runs both ways. It reserves `check_lock`'s write side for the +/// TTY trash-cleanup spinner, which only a worktree removal paints; every +/// candidate here plans a branch deletion or a bare prune, so the flag changes +/// nothing — the registry teardowns serialize on `registry_lock` regardless. #[cfg(unix)] #[rstest] -fn test_prune_metadata_removals_run_concurrently( +fn test_prune_metadata_removals_serialize( mut repo: TestRepo, #[values(false, true)] foreground: bool, ) { @@ -1758,17 +1762,16 @@ fn test_prune_metadata_removals_run_concurrently( let mut cmd = repo.wt_command(); // The removal pool is sized from the rayon thread count; pin it to four so - // the barrier can resolve even on a single-core runner (the workers block - // in subprocess waits, so four threads don't need four CPUs). + // all four teardowns would run at once if `registry_lock` regressed (the + // workers block in subprocess waits, so four threads don't need four CPUs). cmd.env("RAYON_NUM_THREADS", "4"); let git_wrapper_dir = repo.home_path().join("git-wrapper"); std::fs::create_dir_all(&git_wrapper_dir).unwrap(); - write_barrier_worktree_remove_wrapper(&git_wrapper_dir, &which::which("git").unwrap()); + write_overlap_probe_worktree_remove_wrapper(&git_wrapper_dir, &which::which("git").unwrap()); prepend_path(&mut cmd, &git_wrapper_dir); let barrier_dir = repo.home_path().join("barrier"); std::fs::create_dir_all(&barrier_dir).unwrap(); cmd.env("WT_TEST_BARRIER_DIR", &barrier_dir); - cmd.env("WT_TEST_BARRIER_COUNT", stale.len().to_string()); cmd.args(["step", "prune", "--yes", "--min-age=0s"]); if foreground { @@ -1792,13 +1795,14 @@ fn test_prune_metadata_removals_run_concurrently( stale.len(), "every stale entry should have pruned its own metadata: {started:?}" ); - let timeouts: Vec<&String> = sentinels + let overlaps: Vec<&String> = sentinels .iter() - .filter(|n| n.starts_with("timeout-")) + .filter(|n| n.starts_with("overlap-")) .collect(); assert!( - timeouts.is_empty(), - "a metadata prune waited out the barrier — it ran on the write side: {timeouts:?}" + overlaps.is_empty(), + "two `git worktree remove` teardowns overlapped — `registry_lock` did \ + not serialize them (issue #3661): {overlaps:?}" ); let list = repo.git_output(&["worktree", "list", "--porcelain"]); assert!( @@ -1981,12 +1985,15 @@ exit 1 std::fs::set_permissions(&path, permissions).unwrap(); } -/// A `git` shim whose `worktree remove` arms rendezvous with each other (see -/// `test_prune_metadata_removals_run_concurrently`): each records that it -/// started, then waits until `$WT_TEST_BARRIER_COUNT` of them have. Everything -/// else passes through to the real git. +/// A `git` shim whose `worktree remove` arms probe for overlap (see +/// `test_prune_metadata_removals_serialize`): each records that it started, +/// then takes an atomic `mkdir` lock for a fixed window. Under the registry +/// serialization this is testing, no two teardowns ever hold it at once, so a +/// failed `mkdir` — a concurrent teardown mid-window — drops an `overlap-` +/// sentinel the test asserts absent. Everything else passes through to the real +/// git. #[cfg(unix)] -fn write_barrier_worktree_remove_wrapper(dir: &std::path::Path, real_git: &std::path::Path) { +fn write_overlap_probe_worktree_remove_wrapper(dir: &std::path::Path, real_git: &std::path::Path) { use std::os::unix::fs::PermissionsExt; let real_git = shell_escape::unix::escape(real_git.to_string_lossy()); @@ -1996,16 +2003,14 @@ case "$1 $2" in "worktree remove") ;; *) exec {real_git} "$@" ;; esac -: > "$WT_TEST_BARRIER_DIR/started-$(basename "$3")" -i=0 -while [ "$(ls "$WT_TEST_BARRIER_DIR" | grep -c '^started-')" -lt "$WT_TEST_BARRIER_COUNT" ]; do - i=$((i+1)) - if [ "$i" -gt 300 ]; then - : > "$WT_TEST_BARRIER_DIR/timeout-$(basename "$3")" - break - fi - sleep 0.05 -done +own=$(basename "$3") +: > "$WT_TEST_BARRIER_DIR/started-$own" +if mkdir "$WT_TEST_BARRIER_DIR/active" 2>/dev/null; then + sleep 0.2 + rmdir "$WT_TEST_BARRIER_DIR/active" +else + : > "$WT_TEST_BARRIER_DIR/overlap-$own" +fi exec {real_git} "$@" "# ); From da6db0b2884d7d0b2e2c9f139cc7dee9724b02a2 Mon Sep 17 00:00:00 2001 From: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:44:11 +0000 Subject: [PATCH 2/2] docs(prune): fix stale concurrency note on removal_needs_write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/commands/step/prune.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/commands/step/prune.rs b/src/commands/step/prune.rs index 066a2e1b3..9e13818c5 100644 --- a/src/commands/step/prune.rs +++ b/src/commands/step/prune.rs @@ -282,10 +282,14 @@ struct RemovalContext<'a> { /// a hook body nor a spinner, whatever selected it. `StaleDetached` never /// reaches here — [`try_remove`] prunes its entry and returns. /// -/// Everything else fans out, including both mutations that unregister stale -/// worktree metadata: `StaleDetached`'s prune, and the one a `BranchOnly` -/// plan carries as `prune_entry`. Each names its own entry, so concurrent -/// prunes are safe against each other and against the scan — see the +/// Everything else fans out on the read side, including both mutations that +/// unregister stale worktree metadata: `StaleDetached`'s prune, and the one a +/// `BranchOnly` plan carries as `prune_entry`. Naming one entry bounds what +/// each *deletes* but not what `git worktree remove` *reads* (it enumerates +/// every sibling), so those teardowns are not safe to overlap — that is +/// [`RemovalContext::registry_lock`]'s job, orthogonal to `check_lock`: they +/// take its write side via [`removal_mutates_registry`] and so serialize +/// against each other and against the scan's registry reads. See the /// concurrency section on /// [`prune_worktree_entry`](Repository::prune_worktree_entry). fn removal_needs_write(kind: CandidateKind, plan: &RemovalPlan, ctx: &RemovalContext<'_>) -> bool {