fix(remove): atomic CAS branch deletion unifies safe-delete semantics

[max-sixty]

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? doesn't yet mention the fail-closed-on-race behavior.

This was written by Claude Code on behalf of Maximilian Roos

[worktrunk-bot]

The CAS approach unifies the four SafeDelete paths cleanly: one library helper (delete_branch_if_safe) for fast/synchronous/branch-only, plus a CAS-tail builder for the Detached fallback. The update-ref -d <ref> <oid> primitive picks up IntegrationReason semantics for free across all paths — the description of the prior && git branch -d <branch> shell tail silently losing squash-merged/patch-id matches is accurate. The race-rejection discrimination (try update-ref -d, then rev-parse --verify --quiet to distinguish "ref still present" from "real error") is the right shape per the structured-output rule.

The only gap is the one you raised in test plan item 3, and it shows up the most on handle_named_removed_worktree_foreground. In that path output.branch_result carries Ok(RetainedRaced) straight through RemovalDisplayInfo::from_branch_result → handle_branch_deletion_result, which sets show_unmerged_hint = true. RemovalDisplayInfo::print_message then prints "Removed feature worktree" with no mention that the branch was kept (since branch_deleted() is false), and print_hints falls into the self.show_unmerged_hint || … arm and emits "Branch unmerged; to delete, run wt remove -D". The branch wasn't unmerged from the planner's view — it was integrated and then moved — so that hint mis-attributes the cause.

The other three paths handle this correctly: the rename-into-trash fast path and SynchronousForNonCurrent go through warn_if_branch_retained (handlers.rs:311), and handle_branch_only_output has its own dedicated branch (handlers.rs:1123). The detached fallback can't surface it (CAS happens in the detached child), so foreground is the one missing case.

Cleanest fix is probably either: (1) split show_unmerged_hint in BranchDeletionDisplay from a show_race_warning flag for RetainedRaced, then have print_hints emit the dedicated "moved during deletion" message; or (2) keep the field but add a RetainedRaced early branch in print_hints that prints the warning before falling through.

Nit: assert_ne!(original_sha, advanced_sha) works because TestRepo::git_output trims (src/testing/mod.rs:1120), but it's worth knowing — if that trim ever goes away, the assertion becomes vacuous since the trailing \n would always make the strings differ.

[worktrunk-bot]

The CAS rewrite addresses the previous review's structural concerns. One small follow-up on the new test (inline).

[worktrunk-bot]

codecov/patch is failing on the CAS changes. Triaging the report:

False positives (test exists, llvm-cov mis-attributes the middle lines of eprintln!(warning_message(cformat!(...)))): src/output/handlers.rs L324 and L335 in warn_if_branch_retained. The eprintln! openers at L322 and L333 are hit (L322 hits=2, L333 hits=1 in cobertura.xml), so the new warn_if_branch_retained_arms test added in 80e4b5e is doing its job — the gap is reporting noise inside the macro expansion.

Genuine gaps (hits=0 in cobertura):

• src/output/handlers.rs L1127/L1129 — the RetainedRaced arm of handle_branch_only_output. Distinct from warn_if_branch_retained; the new unit test doesn't reach this function. Easiest fix: a sibling unit test that calls handle_branch_only_output with a BranchDeletionOutcome::RetainedRaced and asserts it doesn't panic, mirroring the pattern at handlers.rs:2016.
• src/output/handlers.rs L393 — the None => remove_command branch of build_remove_command_with_tail. The Some(tail) arm is exercised (L391 hits=1); the no-tail arm is trivial to cover with a unit test on the helper.
• src/git/remove.rs L437-438 — the snapshot_sha returns None fallback. By the inline comment this is intentionally a corner-case preservation of pre-CAS behavior, reachable only via a synthetic snapshot/branch mismatch. Either construct a RefSnapshot that hits it, or — if the path is genuinely unreachable — remove it and let the caller fall through to a single deletion code path per the CLAUDE.md coverage guidance ("remove unused code… where falling through to a general handler suffices").
• src/git/remove.rs L495 — the real-error propagation in cas_delete_branch_outcome when both update-ref -d and the post-failure rev-parse --verify fail. Hardest to exercise without a fault-injection hook.

I'm dismissing my prior approval per CLAUDE.md's "never merge a PR with failing codecov/patch without explicit user approval" — the two helper-function gaps look mechanical to close.

codecov/patch failing — coverage gaps detailed in follow-up comment

[worktrunk-bot]

Both concerns from the prior reviews are now resolved by the three new commits:

• Foreground RetainedRaced mis-attribution (971e8b4) — the worktree path no longer folds RetainedRaced into show_unmerged_hint; print_hints gets a dedicated early-return arm, and all three emit paths (warn_if_branch_retained, handle_branch_only_output, print_hints) now route through one retained_raced_branch_message helper. I traced the readers: show_unmerged_hint is now NotDeleted-only, and every remaining NotDeleted | RetainedRaced grouping (flag_note) is correct since both yield an empty note. The raced framing ("inspect commits, then run wt remove -D if safe") is the right fix over the generic "Branch unmerged; run -D" hint, which mis-described an integrated-but-moved branch.
• codecov/patch — now passing on dca30c2. The new tests (c58a41, 813f63) target the exact gaps flagged: the CAS true-error arm, the no-tail build_remove_command_with_tail branch, and the snapshot-skewed non-CAS fallback.

This change touches the branch-deletion / force-delete surface (src/git/remove.rs, src/output/handlers.rs, the wt remove -D recovery command), so per the repo's data-loss-surface review policy I'm not issuing an approval — that merge call is yours. No new findings on the incremental diff.