fix(prune): serialize integration checks against branch -D (Windows .git/config race) - #2808
Merged
Conversation
…git/config race) On Windows, `wt step prune` intermittently failed with `unable to access '.git/config': Permission denied`: parallel `integration_reason` checks read `.git/config` while the inline removal's synchronous `git branch -D` rewrites it via lockfile+rename. Windows holds the file with delete access during the rename and a concurrent reader's `fopen` (no FILE_SHARE_DELETE, no retry) fails; POSIX is unaffected. Add a prune-local `RwLock<()>`: each `integration_reason` call takes a read guard, `try_remove` takes the write guard, so no reader git process is in flight while the synchronous fast-path `branch -D` runs. Parallel checks themselves are not serialized; streaming/interleaved output is unchanged. The cross-fs/.gitmodules/Windows-file-lock fallback defers `branch -D` into a detached command (the worktree still references the branch), which runs outside the guard — documented in-code as a known residual gap, not closed. `test_prune_fallback_config_race_canary` exercises that fallback under parallel fan-out: a deterministic POSIX pass, a Windows canary for whether the residual gap matters in practice. Ref #2801 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 19, 2026
max-sixty
added a commit
that referenced
this pull request
May 19, 2026
…Context (#2811) Follow-up to #2808. That PR fixed the Windows `.git/config` race in `wt step prune` by adding a prune-local `RwLock` parameter to `try_remove`, pushing it to 8 parameters and a `#[allow(clippy::too_many_arguments)]` — deliberately deferred as an optional standalone cleanup to keep the race fix minimal. Of `try_remove`'s 8 arguments, only `candidate` varies; the other 7 are identical at all three call sites in `step_prune` (two in the `rx` loop, one for `deferred_current`). This bundles them into a `RemovalContext<'a>` struct of borrows, constructed once and passed by reference, following the existing `CommandContext` / `RemovedWorktreeOutputContext` / `TemplateContext` pattern. Each call site collapses from a 9-line argument block to one line, the loop-invariant relationship is now expressed in the type rather than re-asserted by hand three times, and the `#[allow]` is removed (clippy is clean without it). The B-prime concurrency semantics are intact: the struct carries `&RwLock<()>` like the other borrows, and `try_remove` still acquires the `check_lock` write guard as the first statement of its body, held over the whole body — load-bearing for the Windows fix. Pure refactor, no behavior change. Rebased onto main after #2806 (the approval-boundary TOCTOU fix), which renamed `try_remove`'s `run_hooks: bool` to `hook_plan: &ApprovedHookPlan`; `RemovalContext` carries that field unchanged. Follow-up commit: the struct refactor incidentally pushed `handle_remove_output(...)` past rustfmt's `fn_call_width` (the `ctx.`-prefixed args), splitting it multi-line and moving the trailing `)?;` onto its own line — which LLVM-cov flags as a `codecov/patch` miss despite the line executing (line 205 is covered and 205→210 is straight-line). Reading the `Copy` fields into locals keeps that call byte-identical to its covered form on main, so the diff stays a faithful no-behavior-change refactor. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 25, 2026
max-sixty
added a commit
that referenced
this pull request
Jun 28, 2026
…#2903) Three follow-ups to #2870 plus a CAS-based unification of safe-delete semantics. (#2900 was the small/incremental path for the first two items; it has been closed as superseded — its commits are all contained here.) ## Item 1 — drop the defensive `check_lock` (refactor) The `check_lock` `RwLock<()>` in `step_prune` was carried over from #2808's Windows `.git/config` race fix. After #2870 restructured the flow, the phase ordering already serializes integration-check readers against removals: the background `par_iter` is the only reader, the `for ... in rx` loop exits only once that thread has finished, and removals run strictly after. Belt-and-suspenders per CLAUDE.md — removed. ## Item 2 — warn when background branch deletion silently retains (fix) Both branch-deletion paths in `execute_instant_removal_or_fallback` swallowed errors at `log::debug!`. Promoted the surprise `Ok(NotDeleted)` (planner predicted deletion, branch survived — a hook moved the tip) to a `warning_message` with a `wt remove -D <branch>` recovery hint. Suppressed when the planner already predicted retention so the existing `print_hints` unmerged-branch message doesn't duplicate. `Err` failures now log at warn level (developer-facing) rather than user-actionable warnings — the failure modes (`git update-ref` exec error, refs DB I/O) aren't actionable beyond re-running. ## Item 3 / CAS rewrite — atomic safe-delete (the big one) `delete_branch_if_safe` now uses `git update-ref -d refs/heads/<branch> <expected-sha>` instead of `git branch -D`. The expected SHA comes from the snapshot the integration check already consulted, so the check→delete sequence becomes one atomic step against a known SHA. If anything moved the ref in between, git rejects the CAS — the new `BranchDeletionOutcome::RetainedRaced` outcome is returned and surfaced to the user. Force-delete keeps `git branch -D` (explicit override). This unifies the divergent safe-delete semantics in `execute_instant_removal_or_fallback`: - **Fast path** + **SynchronousForNonCurrent fallback** routed through `delete_branch_if_safe`, so they pick up CAS for free. - **Detached fallback** (rename failed AND current worktree): the shell `&& git branch -d <branch>` is replaced by a foreground integration check + atomic `&& git update-ref -d <ref> <expected-sha>` tail (`build_cas_branch_delete_tail`). Squash-merged / patch-id / ancestor branches the planner accepts are now accepted at delete time too, matching the fast path. - **Branch-only deletion** (`handle_branch_only_output`) switches from bare `git branch -D` (effectively a force-delete after a stale integration check) to `delete_branch_if_safe`. CAS protects it too. ## Follow-up consolidation (commit `971e8b4`) The "branch kept because its tip moved during the delete" message had drifted into three shapes — two near-identical inline strings plus a gap: the foreground worktree path routed `RetainedRaced` through `show_unmerged_hint` and printed the generic "Branch unmerged; run `-D`" hint, whose bare `-D` would force-delete the just-arrived racing commits. All three emit paths (`warn_if_branch_retained`, `handle_branch_only_output`, `print_hints`) now route through one `retained_raced_branch_message` helper, with a dedicated `RetainedRaced` early-return arm in `print_hints`; `show_unmerged_hint` is now `NotDeleted`-only. Also converged the recovery command to the canonical `wt remove -D <branch>` (via `suggest_command`) and inlined the single-caller `snapshot_sha` wrapper. ### Race coverage CAS closes the TOCTOU window inside `delete_branch_if_safe` (between its own `capture_refs` and `update-ref -d`). Hook-driven races continue to be caught by the existing fresh integration check that runs after pre-remove hooks; CAS narrows the residual window from "between check and delete" to "never". The race rejection is unit-simulated (the ref is moved externally), not yet driven end-to-end through a real `wt remove` + hook. ### Tests Unit tests in `git::remove::tests`: `cas_rejects_delete_when_branch_advances` (pins the CAS rejection mechanism), `cas_deletes_when_branch_unchanged`, `cas_propagates_error_when_ref_vanished`, and `deletes_via_fallback_when_branch_absent_from_snapshot`. In `output::handlers::tests`: every `warn_if_branch_retained` arm, `build_remove_command_with_tail_appends_only_when_present`, and `retained_raced_branch_message_lead_in_varies`. `test_prune_fallback_config_race_canary`'s wrapper-git script matches the new `update-ref -d refs/heads/<branch>` shape. `cargo run -- hook pre-merge --yes` passes locally: 4263 nextest tests + doctests + pre-commit + clippy + docs, no snapshot drift. ### Known gaps (deferred) - No end-to-end integration test of a real mid-delete race (a `pre-remove` hook advancing the branch); the mechanism is unit-covered. - The FAQ [What can Worktrunk delete?](docs/content/faq.md#what-can-worktrunk-delete) doesn't yet mention the fail-closed-on-race behavior. > _This was written by Claude Code on behalf of Maximilian Roos_ --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Windows,
wt step pruneintermittently panickedprune should succeedwithunable to access '.git/config': Permission denied.pruneruns parallelintegration_reasonbranch-integration checks (each spawns git subprocesses that read.git/config) on a background thread while the main loop performs inline removals — and the fast-path removal synchronously runsgit branch -D, which rewrites.git/config(it removes the branch's[branch "<name>"]section) via git's lockfile + atomic-rename. On Windows the rename briefly holds.git/configwith delete access; a concurrent reader'sfopen(...,"r")does not passFILE_SHARE_DELETEand is not retried, so it fails. POSIXopen()/rename()semantics never exhibit this — Linux/macOS CI is green.Approach (the "B-prime" option)
A prune-local
Arc<RwLock<()>>: eachintegration_reasoncall is wrapped in a read guard (scoped to drop before the channel send);try_removetakes the write guard over its body. The write guard blocks until every in-flight reader has dropped and blocks new ones until the removal completes, so no integration-check git process is alive while the synchronousgit branch -Drewrites.git/config. The parallel checks themselves are not serialized — only a reader-vs-removal overlap is excluded — so prune's streaming/interleaved output (the property that fixed the historical start-silence) is unchanged. The lock is confined toprune.rs; the read path is a single guarded scope with no changes tointegration_reason,Cmd, orhandlers.rs.Alternatives considered and rejected: full serialization (reintroduces start-silence);
git update-ref -d+ deferred config-section purge (a Ctrl-C between ref delete and purge leaves a silent ghost-upstream that can corrupt prune's own integration decisions — a data-safety regression); a global git mutex (invasive, kills the parallelism). Poisoning is recovered viaunwrap_or_else(|e| e.into_inner())rather than.expect()— the lock guards(), so a poisoned lock is meaningless and panicking would only cascade an unrelated panic across every later removal.Known residual gap (deliberate, documented — not closed here)
B-prime closes the race on the instant-rename fast path, which is the path the observed CI failure took (synchronous
git branch -Dunder the guard). On the cross-filesystem /.gitmodules/ Windows-file-lock fallback,git branch -Dmust be deferred into a detachedgit worktree remove && git branch -D(the worktree still references the branch until it is removed, so an in-processbranch -Dwould fail) — that deferred write runs after the guard drops and is not covered. The fallback is rare for prune targets (integrated worktrees the user is done with; the lock-prone current worktree is deferred last, after the check thread has drained), so the residual exposure is small but non-zero. Thecheck_lockrationale comment documents this precisely rather than overclaiming an unconditional guarantee.test_prune_fallback_config_race_canaryis the empirical probe for whether that residual gap matters: it forces the fallback for one non-current integrated worktree (pre-blocking its staged path) while several other integrated worktrees keep the parallel integration fan-out running, so the deferred config-rewritingbranch -Doverlaps live.git/configreaders. On POSIX it is a deterministic fallback-path smoke test (a concurrent read and rename never collide). On Windows it is a canary: a flake here during normal CI runs is empirical proof the fallback path needs closing (which would require synchronous/foreground fallback removal — a real perf regression, hence deferred to evidence rather than decided on speculation). The canary asserts only the immediate race signal (prune exit + absence of the.git/configerror); removal-completion polling was intentionally dropped after it proved load-sensitive (a 60swait_fortimeout under heavy concurrent load) and carries no canary value.Testing
pre-commit run --all-filesclean; full suite green (3737 passed, 0 skipped); prune suite36 passed, 0 failed. The fix's correctness is established by macOS reasoning + the green macOS suite; the actual Windows-only behavior can only be confirmed bytest (windows)here (the originally-flakingtest_prune_locally_merged_when_upstream_divergedgoing/staying green) and by the canary's flake rate over subsequent Windows runs.Ref #2801