perf(prune): parallelize removals on the scan lock's read side - #3631
Conversation
Removals ran serially under check_lock's write side; the CAS update-ref -d never rewrites .git/config (only branch -D did, which the chain no longer uses), so hook-free removals now fan out on a worker pool under the read side. The write side remains for hook-bearing, --foreground, and metadata-pruning candidates plus the deferred current worktree. prune_e2e/live ~615ms -> ~398ms; rust-scale live 2.9s -> 0.56s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
This touches the deletion surface (try_remove / delete_branch_if_safe in src/git/remove.rs, and the git worktree remove / git worktree prune / git branch -D chain), and moving those removals off the write side of the scan lock is exactly the class of change the repo's review guidance holds for human sign-off rather than a bot approval. So I'm not approving — flagging for a maintainer's read. The analysis in the description is unusually thorough and I couldn't find a correctness bug; the notes below are what I verified independently and the one residual I'd want eyes on.
The branch -D avoidance holds structurally, not just empirically. The read-side removals plan with BranchDeletionMode::SafeDelete, so delete_branch_if_safe takes the non-force path and routes branch deletion through cas_delete_branch_outcome's git update-ref -d — no .git/config rewrite. The one git branch -D fallback inside that function is the snapshot.local_branch(branch_name) == None arm, and that's unreachable on this path: the integration check that produced reason = Some(..) reads the same frozen snapshot, so if the branch is absent from the snapshot the code lands on the None => NotDeleted arm and never reaches the CAS block at all. The force arm is gated on is_force(), which SafeDelete never returns. That part of the safety argument is solid.
The residual worth a careful look is the other half of the premise: that git worktree remove (rename-failure fallback) and the fail-soft git worktree prune never touch .git/config. Unlike the branch -D avoidance above, that rests on the inode-watching observation, not on a structural guarantee — so it's a property of the git versions you measured against, and a future git that grew a config side effect on either command would silently reintroduce the #2801 read/rename race under concurrency, with no test to catch it (the canary only exercises a config-rewriting branch deletion). If that's an accepted risk given how unlikely a worktree remove/prune config write is, no change needed — but it's the assumption I'd want a human to bless before this lands, since it's the deletion surface.
One smaller thing to confirm rather than fix: on a mid-scan check error, abort is set and the early ? return drops the done_rx drain, so removals already in flight when the error fired can complete (branches/worktrees genuinely gone) without being reflected in removed or any summary. That matches the serial loop's "no summary on abort" and every such removal was a validated integrated candidate, so it's not data loss — just worth confirming it's the intended abort semantics rather than a surprise.
Two shim-driven tests: a single-worker FIFO run where the first failing removal must leave the queued candidates untouched, and a barrier forcing two concurrent failures where only the first is reported. Covers the fan-out's abort/error arms that had no deterministic trigger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
lint is red on this commit: clippy 1.96's large-enum-variant fires on RemovalJob in src/commands/step/prune.rs — the Remove variant is ~248 bytes (dominated by Option<RemoveResult>, whose RemovedWorktree arm is large) against 24 for PrintSkip. It was green at the last commit I reviewed; the trip came from main's toolchain bump to 1.96 merging in, not from this PR's test-only commit — but it's this PR's own new enum, and it's a deterministic -D warnings error, so the lint job stays red until it's addressed.
Boxing the heavy field is clippy's own suggestion and it's a mechanical, three-site change (the enum field plus the two plan handoffs). Inline suggestions below — applying all three composes into a compiling change (try_remove keeps its Option<RemoveResult> param and receives *plan). Happy to push the commit instead if you'd rather.
(The prior review's hold for a human read on the deletion surface is unchanged — this is just the new CI signal, not a re-flag.)
#3533 grew RemoveResult, tripping clippy on the merge commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nt (#3637) Follow-up to #3633, which patched the most visible symptom (prune's summary counting candidate kinds). This lands the structural fix behind that class: the removal chain returned nothing from execution, so everything downstream reported the plan's intent as if it were the outcome. The type even said so — `RemoveResult` was produced by `prepare_worktree_removal` before anything ran, and the codebase couldn't agree on what it was (`RemovePlans` in remove.rs, `plan` in prune, `result` in the handler). ## The plan/outcome split `RemoveResult` → `RemovalPlan` (variant `RemovedWorktree` → `Worktree`), and `handle_remove_output` now returns a `BranchFate`: `Deleted` / `Retained` / `NotAttempted` observed synchronously, `Deferred` only for the detached legacy fallback whose outcome this process never sees. Consumers report the fate: - `wt remove --format=json` said `"branch_deleted": true` while stderr said `▲ Removed worktree but kept branch feature (not integrated)` — a pre-remove hook had advanced the branch, the SafeDelete re-check declined, and stdout reported the plan anyway. Now `false`. - `wt step prune`'s summary counts executed outcomes, so a deletion declined mid-run reads `✓ Pruned 1 worktree`, matching the per-item line above it. A declined *orphan* deletion (no worktree entry, nothing removed at all) now drops out of the removed list entirely instead of being counted as either lie. - The detached fallback emits the "kept branch" correction in the one case it can know (no CAS tail to append), instead of letting the progress message's "worktree & branch" stand uncorrected. The picker's `foreground: true, silent: true` contradiction collapses into one `RemovalExecution::{Foreground, Background(fallback), Silent}` axis, and the four hand-rolled capture-refs-then-CAS-delete sites share one `worktrunk::git::execute_branch_deletion` — the single spelling of "the plan said delete; do it now, against fresh refs". ## For the reviewer `src/output/handlers.rs` carries the bulk: each execution path constructs its fate where it observes the result, and rendering decisions (foreground propagates deletion errors, background warns, silent does neither) are unchanged. The merge with #3631's concurrent prune rework is the diff's riskiest region — the worker result channel widened from `Result<bool>` to `Result<Option<bool>>` so fates flow back through the done-channel and the collector stamps `candidate.deletes_branch` before the summary and JSON read it. Deliberately *not* done: reusing the planner from `wt merge`'s finish path (its divergences — clean-check strictness, preserve-vs-error on primary, merge-target — are intentional), and threading the fallback mode structurally (it just relocates an unreachable arm). Zero snapshot churn: the change is provably invisible wherever intent matched outcome. The divergence cases are pinned end-to-end by tests whose hook must be the branch tip to pass. The one deterministically unreachable arm is the detached-fallback warning (needs a cross-filesystem rename failure); expect `codecov/patch` to flag those few lines. > _This was written by Claude Code on behalf of max_ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
wt step pruneexecuted removals one at a time under the write side ofcheck_lockwhile the candidate scan fanned out on the read side. On the rust-scale fixture that serialized ~2.6 s of a ~2.9 s live prune. Removals now run concurrently: the main thread streams scan results into a FIFO queue of per-candidate jobs, and a worker pool sized like the rayon pool executes them under the read side of the lock.Why the read side is safe. The lock exists for the Windows
.git/configrace (#2801):git branch -Drewrites config via lockfile plus atomic rename, and a concurrent reader fails on the rename. The removal chain has since moved to the CASgit update-ref -d, and (verified empirically, by inode-watching.git/configacross each command) neither it norgit worktree remove/git worktree prunetouches config;branch -Dremains reachable only through arms prune never takes. Concurrentgit worktree pruneruns are idempotent and exit 0, andupdate-ref -ddoes not consult worktree checkout state, so nothing in the hook-free chain needs exclusivity. The write side remains for the removals that do: hook-bearing candidates (a foregroundpre-removebody is an arbitrary command with streamed output),--foregroundcandidates (TTY spinner), the fail-hard metadata-pruning candidates (StaleDetached, plan-lessPrunable), and the deferred current worktree.removal_needs_writeinsrc/commands/step/prune.rsis the decision table; thecheck_lockfield doc carries the full analysis.Output and determinism. Skip lines ride the same FIFO queue as removals, so
RAYON_NUM_THREADS=1(one worker) reproduces the exact serial output order the snapshot tests pin; with more workers, whole lines interleave (oneanstreameprintln!writes a line under a single stderr lock). Live--format=jsonoutput is now sorted by scan index, matching dry-run, with the current worktree still last. The first failing removal flips an abort flag that drains the rest of the queue unexecuted, matching the serial loop's abort-on-first-error.Measured.
prune_e2e/livecriterion: ~615 ms to ~398 ms. Rust-scale live one-shot (36 worktrees, 24 candidates, warm): 2.9 s to 0.56 s wall, all 24 removals overlapping in the trace (6.5 s of subprocess time).Abort semantics. The first failing removal flips an abort flag: in-flight removals complete (their per-candidate lines already printed), queued ones drain unexecuted, no summary prints, and the first error propagates — the same abort-on-first-error the serial loop had, where an error mid-loop likewise left earlier removals done and later candidates untouched. A mid-scan check error takes the same abort path.
Testing. New
test_prune_removals_run_concurrentlyproves the overlap causally: two orphan-branch removals barrier on each other inside a git shim, and serialized removals trip the shim's timeout sentinel (verified: the test fails under forcedRAYON_NUM_THREADS=1).test_prune_removal_failure_aborts_remaining_queueandtest_prune_concurrent_removal_failures_report_firstpin the abort semantics deterministically (single-worker FIFO, and a barrier forcing two concurrent failures). The config-race canary keeps its role with its premise rewritten: a regression to a config-rewriting deletion is what it would now catch. The deterministic-output snapshot tests pass unchanged. The one patch line no test reaches deterministically is the worker's earlyreturnwhen the result receiver is gone (a scan error racing an in-flight removal).Accepted residuals, documented in code: a
packed-refs.lockcontended past git's ~1 s retry budget reads asRetainedRaced(fail-closed; unobserved at 24-way concurrency on local disks), and concurrent fail-softgit worktree pruneraces leave at worst stale metadata for the next prune to collect.🤖 Generated with Claude Code