Restore concurrent worktree staging in step prune

[max-sixty]

wt step prune currently holds its worktree-registry lock across each complete removal to avoid Git's .git/worktrees/ enumeration race. That also serializes safe work such as dirty checks, fsmonitor shutdown, and renaming the worktree into trash.

This moves coordination into the dedicated Repository operations, keyed by canonical Git common directory. Fresh git worktree list calls take the read side, while targeted and direct git worktree remove calls take the write side. Removal staging remains concurrent, and the synthesized---force submodule fallback keeps its final dirty check and destructive command in one critical section.

The canonical prune_e2e/live benchmark improved from a 376.61 ms median on current main to 221.83 ms here, about 41%. Existing prune tests cover concurrent scheduling and serialized metadata teardown. Focused Rust tests verify lock sharing across distinct worktree discovery paths and that the fast-path rename precedes registry teardown serialization.

Follow-up to #3661 and #3692.

This was written by Codex on behalf of max-sixty

[worktrunk-bot]

Holding approval on this one — not on the merits of the locking, which I traced and think is right, but because the diff edits the synthesized---force path in src/git/repository/worktrees.rs (git worktree remove --force in Repository::remove_worktree), which this repo's review guidance treats as hold-for-human: .claude/skills/running-tend/references/review-pr.md, Data-Loss Surface: Hold for Human Review. That rule also says to request review from @max-sixty, which is moot here since you authored it.

What I checked and did not find a problem with, so it doesn't have to be re-derived: there is no lock-order cycle. check_lock is only ever acquired at the top of try_remove and around check_one, always before any Repository call, so the ordering is still check_lock → registry lock with no path taking them the other way. And nothing runs under a registry write guard that re-enters the lock — prune_worktree_entry holds it across run_command alone, and remove_worktree holds it across worktree_at(path).ensure_clean(…), which bottoms out in git status --porcelain (dirty_files) and never touches list_worktrees. fresh_branch_checkout's fresh Repository::at does land on the same Arc<RwLock<()>>, since git_common_dir is canonicalized before it becomes the map key — the new unit test pins that.

Two smaller things:

Stale symbol in a test comment. tests/integration_tests/step_prune.rs still names the removed lock inside test_prune_metadata_removals_serialize — the comment above cmd.env("RAYON_NUM_THREADS", "4") reads "all four teardowns would run at once if registry_lock regressed". It's outside the diff hunks so I can't suggest it inline; "if the registry lock regressed" would do.

The staging-concurrency barrier keys on call ordinal, not phase. write_second_status_barrier_git_wrapper passes the first git status --porcelain per worktree through and rendezvous on the second. That's the planning call and the execution call today, but it's an implicit dependency on the count: if planning ever issues two status calls per worktree, the rendezvous silently moves into the planning phase, both candidates still meet there (planning is already concurrent), and the test keeps passing while no longer proving what its name and the "the execution-time status gate did not run" message claim. Not a blocker — worth a marker tied to the removal phase rather than to "second call" if that's cheap to key on.

[worktrunk-bot]

The barrier swap addresses the phase-coupling point from the last round — git fsmonitor--daemon stop only runs in stage_worktree_removal, so the rendezvous is now pinned to the phase rather than to a call ordinal. But the command it moved onto is bounded in a way git status --porcelain was not, and that kills the test's serialization detector.

stop_fsmonitor_daemon (in src/git/remove.rs, the Cmd::new("git").args(["fsmonitor--daemon", "stop"]) call) attaches .timeout(FSMONITOR_STOP_TIMEOUT), and that constant is Duration::from_secs(2). The shim's give-up path needs i > 300 at sleep 0.05, i.e. ~15 s. Cmd's timeout puts the child in its own process group and tears the whole tree down on expiry (kill_timed_out_tree in run_with_timeout_impl), so under serialization the first shim is killed at 2 s — 13 s before it would write timeout-$own. The second removal then finds started-<first> already on disk and passes straight through. Both started- files exist, no timeout- file is ever written, and test_prune_worktree_staging_runs_concurrently goes green with staging fully serialized. The assertion at !barrier_dir.join(format!("timeout-{name}")).exists() cannot fail.

The fix is to keep the rendezvous inside the 2 s bound and assert a positive sentinel instead of the absence of a negative one — under serialization the first shim gives up without writing it, which is the signal a killed process can't deliver. Suggestions inline; 20 iterations is 1 s, which leaves ~2× margin against the 2 s kill while still being tens of times the skew between two workers reaching stop_fsmonitor_daemon (one git status apart). Pick a different cap if you'd rather, as long as it lands under FSMONITOR_STOP_TIMEOUT.

The lock move itself still traces clean, including the narrowing: the .git/worktrees/<id> deletion that issue #3661's TOCTOU turns on happens inside git worktree remove/prune, both of which hold the write side, and list_worktrees holds the matching read side — so the rename and the fsmonitor stop leaving the critical section doesn't reopen the window. Neither write-side body re-enters list_worktrees (ensure_clean bottoms out in git status --porcelain; Repository::run_command builds its Cmd straight from discovery_path), so there's no self-deadlock on the non-reentrant guard today — which is what the open thread on the WORKTREE_REGISTRY_LOCKS docstring is asking you to write down.

Still holding approval for the same reason as last time — the diff edits the synthesized---force path in Repository::remove_worktree.

[worktrunk-bot]

Both open items from the last round are addressed, and I verified the fix does what it claims rather than taking the shim at its word.

The staging test is now a real detector. With the cap at 20 iterations (1 s, inside the 2 s FSMONITOR_STOP_TIMEOUT) and the positive paired- sentinel, forcing staging to serialize makes test_prune_worktree_staging_runs_concurrently fail in 1.26 s on the fsmonitor-stop staging gate did not overlap for repo.stage-a. That was the exact failure the previous shape could not produce — the 15 s give-up was killed at 2 s and the test went green with staging fully serialized. Resolved that thread and the docstring one; the WORKTREE_REGISTRY_LOCKS spec now records the non-reentrancy rule and the check_lock → registry ordering in both directions.

One note on the head before this one. a051e134 had the RAYON_NUM_THREADS values for test_prune_hook_announcements_include_branch and test_prune_removals_run_concurrently swapped, which made the latter fail deterministically (a deletion waited out the barrier — removals ran serially: ["timeout-para-a"], 16 s per run). 12cfcc82 reverts both, so this is already fixed — flagging only because CI hadn't reached the test job yet when the push landed, so nothing else recorded it. On the current head the whole step_prune module is 52/52 in 4.8 s, and git::repository is 190/190 including the new sharing test.

The lock move itself still traces clean on this head. Nothing under either write guard re-enters a registry accessor: prune_worktree_entry holds it across one run_command, and remove_worktree holds it across worktree_at (canonicalize only) and ensure_clean → dirty_files → git status --porcelain, with run_command_bounded building its Cmd straight from discovery_path. Hoisting the args construction above the guard keeps the synthesized---force check and the destructive command in one critical section, which is the property that matters here. Coverage is also strictly wider than the old registry_lock: every in-process list_worktrees now takes the read side rather than just prune's, and the one registry read that still escapes — the git worktree list --porcelain inside build_cas_branch_delete_tail's shell tail — ran in a detached subprocess under the old scheme too, so it is not a regression.

Still holding approval for the same reason as the previous two rounds: the diff edits the synthesized---force path in Repository::remove_worktree (git worktree remove --force) in src/git/repository/worktrees.rs, which this repo's review guidance treats as hold-for-human under Data-Loss Surface: Hold for Human Review in .claude/skills/running-tend/references/review-pr.md. That is the only thing outstanding — no code change is pending from me.

[worktrunk-bot]

Deleting test_prune_worktree_staging_runs_concurrently leaves this PR's headline behavior with no detector. Nothing else in the suite goes red if remove_worktree's write guard is later hoisted back over staging: test_prune_removals_run_concurrently uses orphan BranchOnly candidates that never take the registry write side (they pass against main's whole-removal registry_lock too), and test_prune_metadata_removals_serialize asserts the absence of overlap, so a wider lock only makes it more true. The benchmark isn't a substitute — .github/workflows/benchmarks.yaml is scheduled, so it never runs on a PR. So "status checks, fsmonitor shutdown, and trash renames still overlap", which the step_prune module docstring now states as fact, is asserted nowhere.

I'm not asking for the fsmonitor shim back. Dropping the "worktree list" case from write_overlap_probe_worktree_remove_wrapper is right for a reason worth recording, since it also rules out re-adding a list probe later: build_cas_branch_delete_tail runs git worktree list --porcelain in a detached shell tail (src/output/handlers.rs, the worktrunk_worktrees=$(git worktree list --porcelain) command string), which takes no registry read guard, so it could land inside the shim's 0.2 s active window and drop a false overlap-list — and the two BranchOnly candidates in test_prune_metadata_removals_serialize are exactly the ones that arm it.

If the staging half is worth keeping, it doesn't need a git shim or a timing barrier. worktree_registry_write is pub(super) on crate::git::repository, so src/git/remove.rs's test module can reach it: take the write guard on a Repository for the same common dir, run stage_worktree_removal on another thread, and assert the trash rename has landed while the guard is still held. That passes today — the guard is taken only by prune_worktree_entry, after fs::rename — and fails deterministically the moment the guard covers the dirty check, the fsmonitor stop, or the rename. Your call whether that's worth the test; the alternative is saying in the docstring that the overlap is unasserted, rather than leaving it read as a tested property.

Still holding approval for the same reason as the previous three rounds, unchanged by this commit: the diff edits the synthesized---force path in Repository::remove_worktree (git worktree remove --force) in src/git/repository/worktrees.rs, which this repo's review guidance treats as hold-for-human under Data-Loss Surface: Hold for Human Review in .claude/skills/running-tend/references/review-pr.md. The lock move itself still traces clean on this head, and test (linux), code-coverage, and codecov/patch are green here; test (macos) and test (windows) were still running when I looked.

[worktrunk-bot]

The last open item is closed: stages_worktree_before_waiting_for_registry_lock puts a detector back under the overlap the step_prune module docstring asserts, and I verified it's a real one rather than taking the shape at its word.

Baseline on 96c44aba it passes in 0.04 s. Widening the registry write guard over staging — a scoped worktree_registry_write() around the stop_fsmonitor_daemon call in stage_worktree_removal, which is the narrowest widening that doesn't self-deadlock against prune_worktree_entry's own write acquisition — makes it fail deterministically in 10.04 s on worktree should be renamed before registry teardown acquires the lock. So the assertion is reachable, which is what the previous shape couldn't say for itself.

One scope note, not a request: the detector sits on stage_worktree_removal, so it catches a guard widened inside the staging prelude. A future lock re-added one level up — around try_remove in the prune command, where registry_lock used to live — would leave it green. That's a narrower guarantee than the deleted integration test had, and it's the trade you took knowingly; recording it so the next reader doesn't over-read the test's name.

Nothing else outstanding from me on this head. The lock move still traces clean: handle_remove_output's prune_entry path builds a fresh Repository::current() before prune_worktree_entry (src/output/handlers.rs), so the WORKTREE_REGISTRY_LOCKS DashMap keyed on the canonical common dir is doing load-bearing work there, and repository_instances_share_worktree_registry_coordination pins it. Nothing under either write guard re-enters a registry accessor — remove_worktree's ensure_clean bottoms out in git status --porcelain via dirty_files, and worktree_at is canonicalize-only. list_worktrees scopes its read guard to the run_command alone, with parsing outside it. CI is green on this head: test (linux|macos|windows), code-coverage, and codecov/patch all pass; only affected tests (macos, advisory) was still running.

Still holding approval for the same reason as the previous rounds, unchanged by this commit: the diff edits the synthesized---force path in Repository::remove_worktree (git worktree remove --force) in src/git/repository/worktrees.rs, which this repo's review guidance treats as hold-for-human under Data-Loss Surface: Hold for Human Review in .claude/skills/running-tend/references/review-pr.md. That guidance also says to request review from @max-sixty, which is moot since you authored it.