fix(prune): serialize worktree-registry teardowns behind a lock

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/<id>`). 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`]
@@ -258,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 {
@@ -281,6 +309,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 +347,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 +387,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 +1119,13 @@ pub fn step_prune(
     let mut skipped_approval: Vec<SkippedApproval> = 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: &registry_lock,
     };
     // Flipped by the first failing removal: the rest of the queue drains
     // without executing (matching the serial loop's abort-on-first-error),

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/<id>`. 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/<id>`).
+    /// 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

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 <path>`, 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 <path>`, 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} "$@"
 "#
     );