Restore concurrent worktree staging in step prune

src/commands/step/prune.rs

@@ -6,11 +6,9 @@
 //! 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`]). 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
+//! ([`removal_needs_write`]). Repository operations coordinate the narrower
+//! Git worktree-registry reads and teardowns themselves, so status checks,
+//! fsmonitor shutdown, and trash renames still overlap. 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
@@ -255,25 +253,6 @@ 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`]
@@ -297,15 +276,8 @@ 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 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
+/// Everything else fans out on the read side. The Git worktree-registry calls
+/// inside those removals take their own repository-scoped lock; see
 /// [`prune_worktree_entry`](Repository::prune_worktree_entry).
 fn removal_needs_write(kind: CandidateKind, plan: &RemovalPlan, ctx: &RemovalContext<'_>) -> bool {
     if matches!(kind, CandidateKind::Current) {
@@ -324,25 +296,6 @@ 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(fate))` if removed
 /// — the executed outcome the summary counts and `--format=json` names —
 /// `Ok(None)` if the removal turned out to be a no-op, `Err` on execution
@@ -366,11 +319,6 @@ fn try_remove(
         // 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`
@@ -403,24 +351,6 @@ 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,
     // so the candidate counts as removed only once the worktree and branch
@@ -1136,13 +1066,11 @@ 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/remove.rs

@@ -703,6 +703,39 @@ mod tests {
     use super::*;
     use crate::testing::TestRepo;
 
+    /// Registry serialization starts after the fast-path rename, so worktree
+    /// staging can overlap while metadata teardown remains exclusive.
+    #[test]
+    fn stages_worktree_before_waiting_for_registry_lock() {
+        let mut test = TestRepo::with_initial_commit();
+        let worktree_path = test.add_worktree("feature");
+        let repo = Repository::at(test.root_path()).unwrap();
+        let worker_repo = repo.clone();
+        let worker_path = worktree_path.clone();
+
+        let registry_guard = repo.worktree_registry_write();
+        let worker = std::thread::spawn(move || {
+            stage_worktree_removal(&worker_repo, &worker_path, Some("feature"), false)
+        });
+
+        let deadline = std::time::Instant::now() + Duration::from_secs(10);
+        while worktree_path.exists() && std::time::Instant::now() < deadline {
+            std::thread::sleep(Duration::from_millis(10));
+        }
+        let renamed_before_unlock = !worktree_path.exists();
+
+        drop(registry_guard);
+        let result = worker.join().expect("staging thread should not panic");
+        assert!(
+            renamed_before_unlock,
+            "worktree should be renamed before registry teardown acquires the lock; result: {result:?}"
+        );
+        assert!(
+            result.unwrap().is_some(),
+            "worktree should use the rename fast path"
+        );
+    }
+
     /// When the branch tip moves between snapshot capture and the deletion
     /// attempt, the atomic compare-and-swap rejects the delete and surfaces
     /// `RetainedRaced` rather than dropping the new commits silently.

src/git/repository/mod.rs

@@ -125,7 +125,7 @@
 use std::ffi::{OsStr, OsString};
 use std::path::{Path, PathBuf};
 use std::process::Stdio;
-use std::sync::{Arc, LazyLock, OnceLock};
+use std::sync::{Arc, LazyLock, OnceLock, RwLock, RwLockReadGuard, RwLockWriteGuard};
 
 use crate::shell_exec::Cmd;
 
@@ -503,6 +503,23 @@ static DEFAULT_BASE_PATH: LazyLock<PathBuf> = LazyLock::new(|| PathBuf::from("."
 /// equality on the raw path is sufficient.
 static GIT_COMMON_DIR_CACHE: LazyLock<DashMap<PathBuf, PathBuf>> = LazyLock::new(DashMap::new);
 
+/// Process-local coordination for Git's worktree registry, keyed by the
+/// canonical Git common directory. Every [`Repository`] for the same common
+/// directory shares one read/write lock. The dedicated
+/// [`Repository::list_worktrees`] accessor takes the read side, while
+/// [`Repository::prune_worktree_entry`] and [`Repository::remove_worktree`]
+/// take the write side.
+///
+/// Guards are non-reentrant: a guarded operation must not call another of
+/// these accessors. In `wt step prune`, the lock order is the command's
+/// `check_lock` followed by this registry lock; code holding a registry guard
+/// must never acquire `check_lock`.
+///
+/// External Git processes and raw worktree commands issued through
+/// [`Repository::run_command`] do not honor this lock.
+static WORKTREE_REGISTRY_LOCKS: LazyLock<DashMap<PathBuf, Arc<RwLock<()>>>> =
+    LazyLock::new(DashMap::new);
+
 /// Process-wide map of `worktree_path -> canonicalized worktree root`,
 /// keyed by the canonicalized path used as the cache key (same convention as
 /// [`Repository::worktree_at`] / [`WorkingTree`]).
@@ -751,6 +768,8 @@ pub struct Repository {
     git_common_dir: PathBuf,
     /// Cached data for this repository. Shared across clones via Arc.
     pub(super) cache: Arc<RepoCache>,
+    /// Shared by every `Repository` that resolves to `git_common_dir`.
+    worktree_registry_lock: Arc<RwLock<()>>,
     /// When set, object-writing git plumbing is redirected into a temporary
     /// object database. `None` for the normal persistent path. See
     /// [`Repository::redirect_objects_for_observation`].
@@ -808,6 +827,10 @@ impl Repository {
     pub fn at(path: impl Into<PathBuf>) -> anyhow::Result<Self> {
         let discovery_path = path.into();
         let git_common_dir = Self::resolve_git_common_dir(&discovery_path)?;
+        let worktree_registry_lock = WORKTREE_REGISTRY_LOCKS
+            .entry(git_common_dir.clone())
+            .or_insert_with(|| Arc::new(RwLock::new(())))
+            .clone();
 
         let cache = RepoCache::default();
         // Consume any `git config --list -z` map preloaded by
@@ -835,10 +858,25 @@ impl Repository {
             discovery_path,
             git_common_dir,
             cache: Arc::new(cache),
+            worktree_registry_lock,
             temporary_object_store: None,
         })
     }
 
+    /// Share registry coordination across fresh repository caches.
+    pub(super) fn worktree_registry_read(&self) -> RwLockReadGuard<'_, ()> {
+        self.worktree_registry_lock
+            .read()
+            .unwrap_or_else(|error| error.into_inner())
+    }
+
+    /// Exclude registry readers and other teardowns for this repository.
+    pub(super) fn worktree_registry_write(&self) -> RwLockWriteGuard<'_, ()> {
+        self.worktree_registry_lock
+            .write()
+            .unwrap_or_else(|error| error.into_inner())
+    }
+
     /// Return a clone whose object-writing git plumbing is redirected into a
     /// temporary object database.
     ///

src/git/repository/tests.rs

@@ -407,6 +407,7 @@ fn repo_path_error_when_is_bare_fails() {
         discovery_path: PathBuf::from("/nonexistent/repo"),
         git_common_dir: PathBuf::from("/nonexistent/.git"),
         cache: Arc::new(RepoCache::default()),
+        worktree_registry_lock: Arc::new(std::sync::RwLock::new(())),
         temporary_object_store: None,
     };
 
@@ -450,6 +451,7 @@ fn repo_path_ignores_non_local_core_worktree() {
         discovery_path: tmp.path().to_path_buf(),
         git_common_dir: git_dir.clone(),
         cache: Arc::new(cache),
+        worktree_registry_lock: Arc::new(std::sync::RwLock::new(())),
         temporary_object_store: None,
     };
 
@@ -673,6 +675,7 @@ fn is_builtin_fsmonitor_enabled_variants() {
             discovery_path: PathBuf::from("/nonexistent/repo"),
             git_common_dir: PathBuf::from("/nonexistent/.git"),
             cache: Arc::new(cache),
+            worktree_registry_lock: Arc::new(std::sync::RwLock::new(())),
             temporary_object_store: None,
         }
     }
@@ -1354,6 +1357,23 @@ fn prewarm_after_early_repository_still_preloads_config() {
     );
 }
 
+#[test]
+fn repository_instances_share_worktree_registry_coordination() {
+    use crate::git::Repository;
+    use crate::testing::TestRepo;
+
+    let mut test = TestRepo::with_initial_commit();
+    let linked = test.add_worktree("registry-lock-linked");
+    let first = Repository::at(test.root_path()).unwrap();
+    let second = Repository::at(linked).unwrap();
+
+    let _write = first.worktree_registry_write();
+    assert!(
+        second.worktree_registry_lock.try_read().is_err(),
+        "fresh repository handles for one common directory must share the registry lock"
+    );
+}
+
 /// A worktree answers to its branch and to its own path, and the branch wins.
 ///
 /// Both spellings reaching the same worktree is the point of routing every

src/git/repository/worktrees.rs

@@ -36,7 +36,10 @@ impl Repository {
         self.cache
             .worktrees
             .get_or_try_init(|| {
-                let stdout = self.run_command(&["worktree", "list", "--porcelain"])?;
+                let stdout = {
+                    let _registry = self.worktree_registry_read();
+                    self.run_command(&["worktree", "list", "--porcelain"])?
+                };
                 let raw_worktrees = WorktreeInfo::parse_porcelain_list(&stdout)?;
                 let mut worktrees: Vec<_> =
                     raw_worktrees.into_iter().filter(|wt| !wt.bare).collect();
@@ -221,18 +224,17 @@ impl Repository {
     ///
     /// # Concurrent calls
     ///
-    /// `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*.
+    /// This method and [`Repository::remove_worktree`] serialize their `git
+    /// worktree remove` commands with each other for the same repository.
+    /// 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).
+    /// That is Git's own TOCTOU between the enumerator's `readdir` and its
+    /// `open`. The repository-scoped write lock closes the in-process window;
+    /// [`Repository::list_worktrees`] takes the matching read side.
     ///
     /// Git also `rmdir`s the containing `.git/worktrees` once the last entry
     /// goes, but that only succeeds on an already-empty directory, and every
@@ -247,6 +249,7 @@ impl Repository {
         // porcelain as UTF-8, so this only fires if that edge ever stops
         // guaranteeing it — a bare `?` rather than a rendered path.
         let path_str = path.to_str().context("worktree path is not valid UTF-8")?;
+        let _registry = self.worktree_registry_write();
         self.run_command(&["worktree", "remove", path_str])?;
         Ok(())
     }
@@ -294,6 +297,17 @@ impl Repository {
         } else {
             self.worktree_at(path).has_initialized_submodules()?
         };
+        let mut args = vec!["worktree", "remove"];
+        if use_force {
+            args.push("--force");
+        }
+        args.push(path_str);
+
+        // The synthesized-force cleanliness check and destructive command are
+        // one critical section. If this waited for another registry teardown
+        // after the check, a concurrent writer could dirty the worktree before
+        // `--force` bypassed Git's own dirty gate.
+        let _registry = self.worktree_registry_write();
         if use_force && !force {
             // Synthesized force (submodule worktree, not user-requested).
             // `--force` will suppress git's dirty check, so re-validate
@@ -307,11 +321,6 @@ impl Repository {
             )?;
             tracing::debug!("Using --force for worktree removal due to initialized submodules");
         }
-        let mut args = vec!["worktree", "remove"];
-        if use_force {
-            args.push("--force");
-        }
-        args.push(path_str);
 
         self.run_command(&args)?;
         Ok(())