diff --git a/src/commands/step/prune.rs b/src/commands/step/prune.rs index 270e3c440..c0b5fc233 100644 --- a/src/commands/step/prune.rs +++ b/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/`). 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 = 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/remove.rs b/src/git/remove.rs index ed1607fe2..7f0c555a6 100644 --- a/src/git/remove.rs +++ b/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. diff --git a/src/git/repository/mod.rs b/src/git/repository/mod.rs index 0dd035b04..98d9b1e0d 100644 --- a/src/git/repository/mod.rs +++ b/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 = LazyLock::new(|| PathBuf::from("." /// equality on the raw path is sufficient. static GIT_COMMON_DIR_CACHE: LazyLock> = 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>>> = + 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, + /// Shared by every `Repository` that resolves to `git_common_dir`. + worktree_registry_lock: Arc>, /// 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) -> anyhow::Result { 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. /// diff --git a/src/git/repository/tests.rs b/src/git/repository/tests.rs index fd03e00fc..54f97b6d2 100644 --- a/src/git/repository/tests.rs +++ b/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 diff --git a/src/git/repository/worktrees.rs b/src/git/repository/worktrees.rs index 53d1db4b8..47cbde66a 100644 --- a/src/git/repository/worktrees.rs +++ b/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/`). - /// 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(()) diff --git a/tests/integration_tests/step_prune.rs b/tests/integration_tests/step_prune.rs index ae4ef73a7..9bd1676c4 100644 --- a/tests/integration_tests/step_prune.rs +++ b/tests/integration_tests/step_prune.rs @@ -1711,8 +1711,8 @@ fn test_prune_removals_run_concurrently(repo: TestRepo) { } } -/// The removals that unregister stale worktree metadata serialize behind -/// `registry_lock` — one `git worktree remove` teardown at a time. +/// The removals that unregister stale worktree metadata serialize through the +/// repository 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 @@ -1730,7 +1730,7 @@ fn test_prune_removals_run_concurrently(repo: TestRepo) { /// `--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. +/// nothing — the registry teardowns serialize inside `Repository` regardless. #[cfg(unix)] #[rstest] fn test_prune_metadata_removals_serialize( @@ -1762,7 +1762,7 @@ fn test_prune_metadata_removals_serialize( let mut cmd = repo.wt_command(); // The removal pool is sized from the rayon thread count; pin it to four so - // all four teardowns would run at once if `registry_lock` regressed (the + // all four teardowns would run at once if the 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"); @@ -1801,8 +1801,8 @@ fn test_prune_metadata_removals_serialize( .collect(); assert!( overlaps.is_empty(), - "two `git worktree remove` teardowns overlapped — `registry_lock` did \ - not serialize them (issue #3661): {overlaps:?}" + "two `git worktree remove` teardowns overlapped — the repository registry \ + lock did not serialize them (issue #3661): {overlaps:?}" ); let list = repo.git_output(&["worktree", "list", "--porcelain"]); assert!( @@ -1985,7 +1985,7 @@ exit 1 std::fs::set_permissions(&path, permissions).unwrap(); } -/// A `git` shim whose `worktree remove` arms probe for overlap (see +/// A `git` shim whose `worktree remove` arms a 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