From 12cfcc822616967abffe98f9b8870fed8d84930c Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 28 Aug 2026 20:04:05 -0700 Subject: [PATCH 1/3] Restore concurrent prune worktree staging --- src/commands/step/prune.rs | 82 +--------------- src/git/repository/mod.rs | 40 +++++++- src/git/repository/tests.rs | 20 ++++ src/git/repository/worktrees.rs | 35 ++++--- tests/integration_tests/step_prune.rs | 130 ++++++++++++++++++++++---- 5 files changed, 196 insertions(+), 111 deletions(-) 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/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..1fa5341df 100644 --- a/tests/integration_tests/step_prune.rs +++ b/tests/integration_tests/step_prune.rs @@ -1711,26 +1711,74 @@ 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. +/// Hook-free worktree removals overlap their staging work. +/// +/// A Git shim makes the fsmonitor-stop calls for two candidates rendezvous. +/// Those calls occur only during removal staging, so the barrier resolves only +/// when staging is concurrent; a candidate-wide registry lock makes the first +/// call time out. +#[cfg(unix)] +#[rstest] +fn test_prune_worktree_staging_runs_concurrently(mut repo: TestRepo) { + repo.commit("initial"); + + let worktrees = [repo.add_worktree("stage-a"), repo.add_worktree("stage-b")]; + + let mut cmd = repo.wt_command(); + cmd.env("RAYON_NUM_THREADS", "2"); + let git_wrapper_dir = repo.home_path().join("git-wrapper"); + std::fs::create_dir_all(&git_wrapper_dir).unwrap(); + write_fsmonitor_stop_barrier_git_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_STAGE_A_DIR", worktrees[0].file_name().unwrap()); + cmd.env("WT_TEST_STAGE_B_DIR", worktrees[1].file_name().unwrap()); + + let output = cmd + .args(["step", "prune", "--yes", "--min-age=0s"]) + .output() + .unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(output.status.success(), "prune should succeed:\n{stderr}"); + for path in &worktrees { + let name = path.file_name().unwrap().to_string_lossy(); + assert!( + barrier_dir.join(format!("started-{name}")).exists(), + "the fsmonitor-stop staging gate did not run for {name}" + ); + assert!( + barrier_dir.join(format!("paired-{name}")).exists(), + "the fsmonitor-stop staging gate did not overlap for {name}" + ); + assert!( + !barrier_dir.join(format!("timeout-{name}")).exists(), + "worktree staging ran serially for {name}" + ); + assert!(!path.exists(), "worktree should be removed: {path:?}"); + } +} + +/// 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 /// prune in place of a removal). Each unregisters its own metadata with /// `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. +/// another worker is mid-deleting and fail. The shim probes for both +/// teardown/teardown and fresh-list/teardown overlap while each teardown holds +/// an atomic `mkdir` lock. Serialized operations leave no `overlap-` sentinel; +/// if coordination regresses, the four workers collide in that window. +/// Unix-only for the same `CreateProcess` shim reason as the canary above. /// /// `--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 operations serialize inside `Repository` regardless. #[cfg(unix)] #[rstest] fn test_prune_metadata_removals_serialize( @@ -1762,7 +1810,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 +1849,7 @@ 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:?}" + "Git worktree registry operations overlapped: {overlaps:?}" ); let list = repo.git_output(&["worktree", "list", "--porcelain"]); assert!( @@ -1985,13 +2032,10 @@ exit 1 std::fs::set_permissions(&path, permissions).unwrap(); } -/// 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. +/// A Git shim whose `worktree remove` arms a probe for registry overlap. Each +/// teardown records that it started, then takes an atomic `mkdir` lock for a +/// fixed window. Concurrent teardowns and `worktree list` calls drop an +/// `overlap-` sentinel. Everything else passes through to the real Git. #[cfg(unix)] fn write_overlap_probe_worktree_remove_wrapper(dir: &std::path::Path, real_git: &std::path::Path) { use std::os::unix::fs::PermissionsExt; @@ -2000,6 +2044,12 @@ fn write_overlap_probe_worktree_remove_wrapper(dir: &std::path::Path, real_git: let script = format!( r#"#!/bin/sh case "$1 $2" in + "worktree list") + if [ -d "$WT_TEST_BARRIER_DIR/active" ]; then + : > "$WT_TEST_BARRIER_DIR/overlap-list" + fi + exec {real_git} "$@" + ;; "worktree remove") ;; *) exec {real_git} "$@" ;; esac @@ -2021,6 +2071,46 @@ exec {real_git} "$@" std::fs::set_permissions(&path, permissions).unwrap(); } +/// A Git shim that rendezvouses the fsmonitor-stop step of two removals. +#[cfg(unix)] +fn write_fsmonitor_stop_barrier_git_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()); + let script = format!( + r#"#!/bin/sh +if [ "$1" != "fsmonitor--daemon" ] || [ "$2" != "stop" ]; then + exec {real_git} "$@" +fi +own=$(basename "$PWD") +case "$own" in + "$WT_TEST_STAGE_A_DIR") other="$WT_TEST_STAGE_B_DIR" ;; + "$WT_TEST_STAGE_B_DIR") other="$WT_TEST_STAGE_A_DIR" ;; + *) exec {real_git} "$@" ;; +esac +: > "$WT_TEST_BARRIER_DIR/started-$own" +i=0 +while [ ! -e "$WT_TEST_BARRIER_DIR/started-$other" ]; do + i=$((i+1)) + if [ "$i" -gt 20 ]; then + : > "$WT_TEST_BARRIER_DIR/timeout-$own" + break + fi + sleep 0.05 +done +if [ -e "$WT_TEST_BARRIER_DIR/started-$other" ]; then + : > "$WT_TEST_BARRIER_DIR/paired-$own" +fi +exec {real_git} "$@" +"# + ); + let path = dir.join("git"); + std::fs::write(&path, script).unwrap(); + let mut permissions = std::fs::metadata(&path).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&path, permissions).unwrap(); +} + /// A `git` shim whose `update-ref -d refs/heads/para-{a,b}` arms rendezvous /// with each other (see `test_prune_removals_run_concurrently`); everything /// else passes through to the real git. From 8a19fa16ead2d2f025a3b7b240b4adb4ea059373 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 28 Aug 2026 21:07:53 -0700 Subject: [PATCH 2/3] Simplify prune concurrency coverage --- tests/integration_tests/step_prune.rs | 124 ++++---------------------- 1 file changed, 17 insertions(+), 107 deletions(-) diff --git a/tests/integration_tests/step_prune.rs b/tests/integration_tests/step_prune.rs index 1fa5341df..9bd1676c4 100644 --- a/tests/integration_tests/step_prune.rs +++ b/tests/integration_tests/step_prune.rs @@ -1711,56 +1711,6 @@ fn test_prune_removals_run_concurrently(repo: TestRepo) { } } -/// Hook-free worktree removals overlap their staging work. -/// -/// A Git shim makes the fsmonitor-stop calls for two candidates rendezvous. -/// Those calls occur only during removal staging, so the barrier resolves only -/// when staging is concurrent; a candidate-wide registry lock makes the first -/// call time out. -#[cfg(unix)] -#[rstest] -fn test_prune_worktree_staging_runs_concurrently(mut repo: TestRepo) { - repo.commit("initial"); - - let worktrees = [repo.add_worktree("stage-a"), repo.add_worktree("stage-b")]; - - let mut cmd = repo.wt_command(); - cmd.env("RAYON_NUM_THREADS", "2"); - let git_wrapper_dir = repo.home_path().join("git-wrapper"); - std::fs::create_dir_all(&git_wrapper_dir).unwrap(); - write_fsmonitor_stop_barrier_git_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_STAGE_A_DIR", worktrees[0].file_name().unwrap()); - cmd.env("WT_TEST_STAGE_B_DIR", worktrees[1].file_name().unwrap()); - - let output = cmd - .args(["step", "prune", "--yes", "--min-age=0s"]) - .output() - .unwrap(); - let stderr = String::from_utf8_lossy(&output.stderr); - - assert!(output.status.success(), "prune should succeed:\n{stderr}"); - for path in &worktrees { - let name = path.file_name().unwrap().to_string_lossy(); - assert!( - barrier_dir.join(format!("started-{name}")).exists(), - "the fsmonitor-stop staging gate did not run for {name}" - ); - assert!( - barrier_dir.join(format!("paired-{name}")).exists(), - "the fsmonitor-stop staging gate did not overlap for {name}" - ); - assert!( - !barrier_dir.join(format!("timeout-{name}")).exists(), - "worktree staging ran serially for {name}" - ); - assert!(!path.exists(), "worktree should be removed: {path:?}"); - } -} - /// The removals that unregister stale worktree metadata serialize through the /// repository registry lock — one `git worktree remove` teardown at a time. /// @@ -1769,16 +1719,18 @@ fn test_prune_worktree_staging_runs_concurrently(mut repo: TestRepo) { /// prune in place of a removal). Each unregisters its own metadata with /// `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. The shim probes for both -/// teardown/teardown and fresh-list/teardown overlap while each teardown holds -/// an atomic `mkdir` lock. Serialized operations leave no `overlap-` sentinel; -/// if coordination regresses, the four workers collide in that window. -/// Unix-only for the same `CreateProcess` shim reason as the canary above. +/// 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. 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 operations serialize inside `Repository` regardless. +/// nothing — the registry teardowns serialize inside `Repository` regardless. #[cfg(unix)] #[rstest] fn test_prune_metadata_removals_serialize( @@ -1849,7 +1801,8 @@ fn test_prune_metadata_removals_serialize( .collect(); assert!( overlaps.is_empty(), - "Git worktree registry operations overlapped: {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!( @@ -2032,10 +1985,13 @@ exit 1 std::fs::set_permissions(&path, permissions).unwrap(); } -/// A Git shim whose `worktree remove` arms a probe for registry overlap. Each -/// teardown records that it started, then takes an atomic `mkdir` lock for a -/// fixed window. Concurrent teardowns and `worktree list` calls drop an -/// `overlap-` sentinel. Everything else passes through to the real Git. +/// 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 +/// 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_overlap_probe_worktree_remove_wrapper(dir: &std::path::Path, real_git: &std::path::Path) { use std::os::unix::fs::PermissionsExt; @@ -2044,12 +2000,6 @@ fn write_overlap_probe_worktree_remove_wrapper(dir: &std::path::Path, real_git: let script = format!( r#"#!/bin/sh case "$1 $2" in - "worktree list") - if [ -d "$WT_TEST_BARRIER_DIR/active" ]; then - : > "$WT_TEST_BARRIER_DIR/overlap-list" - fi - exec {real_git} "$@" - ;; "worktree remove") ;; *) exec {real_git} "$@" ;; esac @@ -2071,46 +2021,6 @@ exec {real_git} "$@" std::fs::set_permissions(&path, permissions).unwrap(); } -/// A Git shim that rendezvouses the fsmonitor-stop step of two removals. -#[cfg(unix)] -fn write_fsmonitor_stop_barrier_git_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()); - let script = format!( - r#"#!/bin/sh -if [ "$1" != "fsmonitor--daemon" ] || [ "$2" != "stop" ]; then - exec {real_git} "$@" -fi -own=$(basename "$PWD") -case "$own" in - "$WT_TEST_STAGE_A_DIR") other="$WT_TEST_STAGE_B_DIR" ;; - "$WT_TEST_STAGE_B_DIR") other="$WT_TEST_STAGE_A_DIR" ;; - *) exec {real_git} "$@" ;; -esac -: > "$WT_TEST_BARRIER_DIR/started-$own" -i=0 -while [ ! -e "$WT_TEST_BARRIER_DIR/started-$other" ]; do - i=$((i+1)) - if [ "$i" -gt 20 ]; then - : > "$WT_TEST_BARRIER_DIR/timeout-$own" - break - fi - sleep 0.05 -done -if [ -e "$WT_TEST_BARRIER_DIR/started-$other" ]; then - : > "$WT_TEST_BARRIER_DIR/paired-$own" -fi -exec {real_git} "$@" -"# - ); - let path = dir.join("git"); - std::fs::write(&path, script).unwrap(); - let mut permissions = std::fs::metadata(&path).unwrap().permissions(); - permissions.set_mode(0o755); - std::fs::set_permissions(&path, permissions).unwrap(); -} - /// A `git` shim whose `update-ref -d refs/heads/para-{a,b}` arms rendezvous /// with each other (see `test_prune_removals_run_concurrently`); everything /// else passes through to the real git. From 96c44aba4e12554e065ce435b548526886cf5796 Mon Sep 17 00:00:00 2001 From: Maximilian Roos Date: Fri, 28 Aug 2026 21:26:51 -0700 Subject: [PATCH 3/3] Test prune staging lock boundary in Rust --- src/git/remove.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) 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.