perf(remove, prune): one check per guarantee in the removal chain - #3617
Conversation
Prune now passes each candidate's scan-time removal plan through to the serial removal instead of re-deriving it under the write lock; the post-pre-remove safety refresh is deleted (prepare computes the display verdict, and the pre-rename clean check plus the branch-delete CAS carry the guarantees); the clean check moves before the fsmonitor-daemon stop, so the daemon serves the status instead of a post-stop full re-stat; and prune's gather reads the main worktree from list order rather than running git rev-parse per worktree. prune-4-8 criterion: probe-cold scan -25%, warm scan -43%, live -21%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A git shim fails 'worktree prune' so a Prunable candidate's preparation errors at removal time; prune must skip it and exit successfully with the branch intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewed the removal-chain refactor closely, and traced the data-safety invariants end to end rather than trusting the (thorough) PR description. Not auto-approving: this is worktrunk's deletion surface (src/git/remove.rs removal core + --force fallback, src/output/handlers.rs, src/commands/step/prune.rs), which per the repo's review policy is a human's call to merge — flagging that explicitly so silence isn't read as a rubber stamp.
The refactor looks correct. What I verified independently:
ensure_cleangate preserved exactly once on every non-force path. Foreground / silent-picker / detached-foreground now gate insideremove_worktree_with_cleanup; background gates inexecute_instant_removal_or_fallback(pre-existing). None lost the check and none doubled it — and the old background path actually ranensure_cleantwice (refresh_removal_safety_after_pre_remove+execute_instant), so this reduces it to one. The gate still runs afterexecute_pre_remove_hooks_if_needed, so a hook-dirtied worktree is still caught.- Scan-time plan reuse is safe. The
check_oneread lock andtry_removewrite lock serialize on the sameRwLock; the only load-bearing fields at execution (dirtiness, branch integration) are re-validated by the execution-timeensure_cleananddelete_branch_if_safe's fresh-ref CAS.integration_reasonis display-only, as documented. TheOrphanchange from unconditionalremovable = truetoplan.is_some()matches the old net outcome (oldtry_removere-prepared and skipped on error). idx == 0 && !is_barematches the documentedlist_worktrees()ordering guarantee (main first for normal repos; bare entry filtered, no main for bare) — equivalent to the old!is_linked()probe, including the submodule path-correction case.
One small non-blocking observation on the background path: because the clean check moved into execute_instant_removal_or_fallback (after print_message/print_hints in handle_named_removed_worktree_background), a hook-dirtied worktree removed in background mode now prints "Removing … in background" and the retention hint before failing the clean check, where previously the refresh gate failed before any chrome. No data loss (it still fails before the rename, no false "Removed"), just slightly noisier output in that corner — worth a glance to confirm it's intended.
…in trim (#3622) Re-measurement on the rebuilt `rust-prune-12-24` fixture after #3617 trimmed the removal chain: live one-shot ~12 s to ~2.9 s (two runs, 2.91 s / 2.89 s), worktree candidates ~155-210 ms each now that the status is fsmonitor-served, branch-only ~40-100 ms. Full-cold dry-run unchanged in shape. > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Resolves the conflicts with #3607 (path resolution) and #3617 (the removal chain's one-check-per-guarantee refactor), and reworks the retention onto their shape: - One `live_sibling_checkout` predicate replaces the two divergent sibling scans. A sibling entry whose directory is already gone no longer retains the branch and names a path that isn't there. - `wt merge` and `wt step prune` ask it too. Both reached the same ref deletion outside the guard; merge asserted the invariant in a comment instead of checking it. - `wt step prune` targets candidates by path, matching `wt remove`. Targeting a stale entry by branch name resolved to a live worktree the same prune had skipped as too young, then removed it. - A `-D` this refuses warns instead of passing quietly. - #3480's duplicate hint points at `wt remove <path>`, now the safe answer, and `wt remove --help` states the retention. Tests: remove (path, name, refused -D, pruned fallback, stale sibling must not retain), prune (stale entry with a live sibling), merge (duplicate checkout). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…3690) Closes #3646. **Recommendation: keep the daemon-served gate. No behavior change.** This PR records that as a decision so the next release review doesn't re-litigate it. `wt remove`'s final dirty-worktree check is answered by the fsmonitor daemon when `core.fsmonitor` is set. #3646 asked whether the default removal path should trust that for the last gate before a destructive rename. Three findings decide it, all verified against git 2.55.0 and the current tree: **`git worktree remove` gates on the same daemon-served status.** With `core.fsmonitor=true`, it detects a modification through `fsmonitor_refresh_callback` and refuses. So `wt remove`'s gate is exactly as daemon-trusting as the command it replaces; forcing a non-daemon status would make `wt` stricter than git, which is the thing `src/commands/worktree/push.rs` already declines to do for ignored-file overwrites. **The posited failure mode is guarded in git's builtin daemon.** All three backends meet detected event loss by making clients rescan: `fsm-listen-darwin.c` calls `fsmonitor_force_resync()` on `ef_is_dropped()`, `fsm-listen-win32.c` calls it on the `watch->count == 0` kernel-buffer overflow, and `fsm-listen-linux.c` force-shuts the daemon down on `IN_Q_OVERFLOW`. A stale clean answer therefore needs an *undetected* loss, which lies to the user's own `git status` in that worktree just as readily. **Only `-c core.fsmonitor=` would close the gap, and it costs exactly what #3617 removed.** Confirmed as @worktrunk-bot reported: `--no-optional-locks` still queries the daemon. A non-fsmonitor status is the full-tree re-stat that #3617 measured as the dominant per-removal cost at rust-lang/rust scale, and it would fall only on repos whose owners enabled fsmonitor to avoid full scans. <details><summary>git 2.55.0 traces</summary> `git worktree remove` on a dirty worktree, `core.fsmonitor=true`: ``` fsmonitor.c:534 refresh fsmonitor fsmonitor.c:443 fsmonitor_refresh_callback 'f1.txt' (pos 0) fsmonitor.c:202 fsmonitor_refresh_callback INV: 'f1.txt' fatal: '.../wt-test' contains modified or untracked files, use --force to delete it ``` `--no-optional-locks` vs `-c core.fsmonitor=`: ``` === git --no-optional-locks status --porcelain === fsmonitor.c:534 refresh fsmonitor # still queries the daemon === git -c core.fsmonitor= status --porcelain === fsmonitor.c:786 remove fsmonitor # bypasses the daemon ``` </details> ### Two premises in the issue that don't hold Neither changes the conclusion, but both should leave the record. **There is no 24-hour recovery window.** The issue's third "acceptable" reason says a worktree lost this way is recoverable for a day. On the normal path the detached process runs `rm -rf` on the staged directory immediately — `build_remove_command_staged` emits `rm -rf -- <staged>`. The 24-hour sweep (`TRASH_STALE_THRESHOLD_SECS`) collects entries orphaned when a background removal was *interrupted*. So the gate is the last line with nothing behind it, which weakens the acceptance case; the two findings above carry it instead. No user-facing doc claims otherwise — the FAQ and `config state` both describe trash accurately. **The wedged daemon worktrunk already handles fails safe.** The issue cites `stop_fsmonitor_daemon` force-killing a wedged daemon as evidence that wedged daemons are real. That function's own spec describes the case it handles as "one that has stopped answering its socket — the common failure, which also hangs `git status` in that worktree". A daemon that stops answering makes status hang or full-scan; it doesn't return stale-clean. That's evidence for the safe failure direction, not the dangerous one. ### The change Comments only. The removal module spec gains a `# The dirty-worktree gate follows git` section stating the decision and why `-c core.fsmonitor=` isn't the fix, and `CLAUDE.md`'s "stop where git's own protections stop" paragraph gains this as its second instance, so a release data-loss review lands on it. Lint, clippy, and rustdoc under `-Dwarnings` pass; no behavior or tests change. > _This was written by Claude Code on behalf of max-sixty_ Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
wt step prunescans in parallel, but each removal ran a serial chain of ~17 git subprocesses under the scan write lock: a re-run ofprepare_worktree_removal(the scan had already run it and kept onlyis_ok()), a post-hook "safety refresh" (status + fresh snapshot + integration re-compute) that ran even when no pre-remove hooks executed, and a final pre-rename clean check that ran right after the fsmonitor daemon stop, paying a full un-fsmonitored re-stat. At rust-lang/rust scale that post-stop status is the 0.5-1.7 s per-removal driver.This trims the chain to one mechanism per guarantee, with no behavioral additions:
CheckOutcome.plan) instead of re-preparing under the write lock.Prunablecandidates still prepare intry_removebecause preparing them prunes stale metadata, which must stay serialized.refresh_removal_safety_after_pre_removeis deleted.prepare_worktree_removalnow computes the integration verdict and effective target for display (carried onRemoveResult::RemovedWorktree), the pre-rename clean check catches hook-dirtied worktrees, anddelete_branch_if_safe's CAS re-decides the branch deletion against fresh refs. The clean check also moves intoremove_worktree_with_cleanup, so the foreground and picker paths get their final gate adjacent to the rename rather than relying on the deleted refresh.prune-gatherstops shelling out: the main worktree comes fromgit worktree listordering (documented: main lists first;list_worktrees()filters bare entries) instead of agit rev-parse --git-dirper worktree.Criterion on the
prune-4-8fixture:dry_run_probe_cold197 ms to 147 ms (-25%),dry_run_warm110 ms to 62 ms (-43%),live783 ms to 615 ms (-21%). Aprune-12-8one-shot timeline (24 candidates, live): wall 3.14 s to 2.09 s; per-worktree removals 170-210 ms to 112-134 ms, now dominated by the ~60 ms daemon stop. The rust-scale numbers inbenches/CLAUDE.mdare marked for re-measurement on the next fixture rebuild.Consistency corner, unchanged in kind but wider in trigger: a worktree that becomes dirty after
handle_removed_worktree_outputstarts (in practice: a pre-remove hook dirtying it) now fails at the pre-rename check, which is after the cd directive is written (and, in background mode, after the "Removing … in background" message and retention hint print), and the shell wrapper applies the cd file regardless of exit code. So in that corner the shell moves to the primary worktree while the dirty worktree is preserved with an error. The same outcome already existed for a file appearing between the refresh and the final check; nothing is removed in either case.Testing: full pre-merge gate green (4,578 tests, no snapshot changes), plus a new shim-based test covering the skip path when a stale (
Prunable) candidate's preparation fails at removal time. The hook-dirtied corner has no dedicated test; existing dirty-worktree removal tests cover the command-entry gate. The one remaining uncovered added line is the?propagation closing the live scan closure (prune.rs:1023), which has no deterministic trigger.