Skip to content

Commit eafced3

Browse files
max-sixtyclaude
andcommitted
fix(remove): atomic CAS branch deletion unifies safe-delete semantics
Three follow-ups to #2870 collapse into one rewrite of `delete_branch_if_safe`: it now performs an atomic compare-and-swap delete via `git update-ref -d refs/heads/<branch> <expected-sha>`, where the expected SHA comes from the snapshot the integration check already consulted. If the ref moves between the integration check and the delete (a hook, a concurrent push), git rejects the CAS — the branch is retained (fail-closed) and we surface the new `BranchDeletionOutcome::RetainedRaced` outcome rather than dropping the unmerged commits silently. Force-delete still uses `git branch -D` (the user's explicit override). This unifies the divergent safe-delete semantics in `execute_instant_removal_or_fallback`: - Fast path and `SynchronousForNonCurrent` fallback already routed through `delete_branch_if_safe`, so they pick up CAS for free. - Detached fallback used to bake `&& git branch -d <branch>` into the shell, which only honored git's reachability-from-HEAD check (so squash-merged / patch-id / ancestor branches the planner accepted as integrated were rejected at delete time). Now `build_cas_branch_delete_tail` runs worktrunk's full integration check in the foreground and, if integrated, appends an atomic `git update-ref -d <ref> <expected-sha>` to the detached shell — same semantics as the other paths, plus CAS safety against tip movement between the foreground check and the detached delete. - `handle_branch_only_output` also switches its safe-delete path from `git branch -D` (an unsafe force-delete after a stale integration check) to `delete_branch_if_safe`, so the CAS protects branch-only deletions the same way. The display layer absorbs the new variant: `flag_note` treats `RetainedRaced` like `NotDeleted` for the success badge, `handle_branch_deletion_result` shows the unmerged hint, and the branch-only output path emits a dedicated "moved during deletion" warning with a `wt remove --force-delete <branch>` recovery hint. CAS protects the TOCTOU window inside `delete_branch_if_safe` (between its own snapshot capture and `update-ref -d`). Hook-driven races are still caught by the existing fresh integration check that runs after pre-remove hooks; CAS narrows the residual unprotected window from \"between check and delete\" to \"never\". Tests: `git update-ref -d <ref> <sha>` is structured — exit 0 on deletion, non-zero on stale-SHA rejection (\"cannot lock ref ... is at X but expected Y\"). The CAS helper re-checks ref presence via `rev-parse --verify --quiet` rather than parsing the error message, keeping with the structured-output policy. Two new unit tests in `git::remove::tests`: - `cas_rejects_delete_when_branch_advances` — captures a snapshot, moves the branch externally, then calls `delete_branch_if_safe` and asserts `RetainedRaced` plus surviving tip at the post-race SHA. Pins the CAS rejection mechanism. - `cas_deletes_when_branch_unchanged` — sanity check the common integrated case still deletes. `test_prune_fallback_config_race_canary`'s wrapper-git script now matches `update-ref -d refs/heads/<branch>` alongside the `branch -d|-D <branch>` shapes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 0fbf824 commit eafced3

3 files changed

Lines changed: 280 additions & 32 deletions

File tree

src/git/remove.rs

Lines changed: 145 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,12 @@ impl BranchDeletionMode {
225225
pub enum BranchDeletionOutcome {
226226
/// Branch was not deleted — it was not integrated, and deletion was not forced.
227227
NotDeleted,
228+
/// Branch was integrated but the atomic compare-and-swap deletion was
229+
/// rejected because the ref moved between the integration check and the
230+
/// delete attempt — e.g. a hook or concurrent process advanced it. The
231+
/// branch is retained (fail-closed), and the caller surfaces this as a
232+
/// warning so the user can decide whether to re-check and delete.
233+
RetainedRaced,
228234
/// Branch was force-deleted without an integration check.
229235
ForceDeleted,
230236
/// Branch was deleted because it was integrated (the specific reason is attached).
@@ -412,8 +418,26 @@ pub fn delete_branch_if_safe(
412418

413419
let outcome = match reason {
414420
Some(r) => {
415-
repo.run_command(&["branch", "-D", branch_name])?;
416-
BranchDeletionOutcome::Integrated(r)
421+
// Atomic compare-and-swap against the snapshotted SHA. If the ref
422+
// moved between `integration_reason` and the delete (e.g. a hook
423+
// advanced the branch), `git update-ref -d <ref> <expected>` fails
424+
// closed: the branch is retained and we surface a `RetainedRaced`
425+
// outcome rather than dropping the unmerged commits silently.
426+
match snapshot_sha(snapshot, branch_name) {
427+
Some(expected_sha) => {
428+
cas_delete_branch_outcome(repo, branch_name, expected_sha, r)?
429+
}
430+
// Snapshot doesn't carry the branch SHA — extremely unusual
431+
// (the caller just captured refs, the branch is present in the
432+
// integration check). Fall through to a non-CAS delete rather
433+
// than failing the whole operation: this preserves the
434+
// pre-CAS behavior in a corner case rather than introducing a
435+
// new error class.
436+
None => {
437+
repo.run_command(&["branch", "-D", branch_name])?;
438+
BranchDeletionOutcome::Integrated(r)
439+
}
440+
}
417441
}
418442
None => BranchDeletionOutcome::NotDeleted,
419443
};
@@ -424,6 +448,54 @@ pub fn delete_branch_if_safe(
424448
})
425449
}
426450

451+
/// Look up a local branch's tip SHA in the snapshot.
452+
///
453+
/// Pulls from the inventory rather than `resolve()` so the result reflects the
454+
/// scanned `refs/heads/` walk, the same source `integration_reason` consulted.
455+
fn snapshot_sha<'a>(snapshot: &'a crate::git::RefSnapshot, branch: &str) -> Option<&'a str> {
456+
snapshot.local_branch(branch).map(|b| b.commit_sha.as_str())
457+
}
458+
459+
/// Atomically delete `refs/heads/<branch>` iff it currently points at
460+
/// `expected_sha`, and translate the result into a [`BranchDeletionOutcome`].
461+
///
462+
/// `git update-ref -d <ref> <oid>` is git's compare-and-swap delete primitive:
463+
/// the ref is removed only if its current value matches `<oid>`. If it has
464+
/// moved (a hook or concurrent process advanced the branch), the command
465+
/// exits non-zero with a `cannot lock ref` message and the ref is left alone —
466+
/// fail-closed semantics that protect unmerged commits.
467+
///
468+
/// When `update-ref` fails, distinguishes "ref moved" (the ref still exists
469+
/// → `RetainedRaced`) from "real error" (the ref is gone or git itself
470+
/// failed → propagate the original error) by re-checking with `rev-parse
471+
/// --verify --quiet`, which has a structured exit code (0 = present, 1 =
472+
/// absent) rather than relying on locale-sensitive error-message text.
473+
fn cas_delete_branch_outcome(
474+
repo: &Repository,
475+
branch_name: &str,
476+
expected_sha: &str,
477+
reason: IntegrationReason,
478+
) -> anyhow::Result<BranchDeletionOutcome> {
479+
let ref_name = format!("refs/heads/{branch_name}");
480+
let update_err = match repo.run_command(&["update-ref", "-d", &ref_name, expected_sha]) {
481+
Ok(_) => return Ok(BranchDeletionOutcome::Integrated(reason)),
482+
Err(e) => e,
483+
};
484+
485+
// CAS failed. Re-check the ref to distinguish a race rejection (ref
486+
// moved → still present) from a true error (refs DB I/O, permissions,
487+
// git missing → propagate). `rev-parse --verify --quiet` returns exit
488+
// 0 when the ref exists, exit 1 when it does not — no message parsing.
489+
if repo
490+
.run_command(&["rev-parse", "--verify", "--quiet", &ref_name])
491+
.is_ok()
492+
{
493+
Ok(BranchDeletionOutcome::RetainedRaced)
494+
} else {
495+
Err(update_err)
496+
}
497+
}
498+
427499
/// Generate a staging path for worktree removal.
428500
///
429501
/// Places the staging directory inside `<git-common-dir>/wt/trash/` so it is
@@ -445,12 +517,83 @@ pub(crate) fn generate_removing_path(trash_dir: &Path, worktree_path: &Path) ->
445517
#[cfg(test)]
446518
mod tests {
447519
use super::*;
520+
use crate::testing::TestRepo;
521+
522+
/// When the branch tip moves between snapshot capture and the deletion
523+
/// attempt, the atomic compare-and-swap rejects the delete and surfaces
524+
/// `RetainedRaced` rather than dropping the new commits silently.
525+
///
526+
/// The setup mimics a hook (or any concurrent writer) that advances the
527+
/// branch after the planner observed it as integrated: we capture refs
528+
/// when `feature` is at the same commit as `main` (trivially integrated),
529+
/// then move `feature` forward, then call `delete_branch_if_safe` with
530+
/// the stale snapshot. Integration check still says "integrated" (the
531+
/// snapshotted SHA is reachable from main), but CAS catches the live
532+
/// tip having moved and refuses the delete.
533+
#[test]
534+
fn cas_rejects_delete_when_branch_advances() {
535+
let test = TestRepo::with_initial_commit();
536+
test.run_git(&["branch", "feature"]);
537+
let repo = Repository::at(test.root_path()).unwrap();
538+
539+
// Snapshot captures `feature` at the initial commit (same as `main`).
540+
let snapshot = repo.capture_refs().unwrap();
541+
let original_sha = snapshot.local_branch("feature").unwrap().commit_sha.clone();
542+
543+
// Race: advance `feature` after the snapshot.
544+
test.run_git(&["checkout", "feature"]);
545+
std::fs::write(test.root_path().join("race.txt"), "boom\n").unwrap();
546+
test.run_git(&["add", "race.txt"]);
547+
test.run_git(&["commit", "-m", "post-snapshot advance"]);
548+
test.run_git(&["checkout", "main"]);
549+
550+
let advanced_sha = test.git_output(&["rev-parse", "feature"]);
551+
assert_ne!(
552+
original_sha, advanced_sha,
553+
"test setup: tip must have moved"
554+
);
555+
556+
let result = delete_branch_if_safe(&repo, &snapshot, "feature", "main", false).unwrap();
557+
assert!(
558+
matches!(result.outcome, BranchDeletionOutcome::RetainedRaced),
559+
"expected RetainedRaced, got a different outcome"
560+
);
561+
562+
// Branch survives, still at the post-race SHA.
563+
let live = test.git_output(&["rev-parse", "feature"]);
564+
assert_eq!(live, advanced_sha, "branch must not be deleted nor reset");
565+
}
566+
567+
/// Plain integrated case still deletes via CAS. Sanity check that the
568+
/// new code path doesn't break the common case.
569+
#[test]
570+
fn cas_deletes_when_branch_unchanged() {
571+
let test = TestRepo::with_initial_commit();
572+
test.run_git(&["branch", "feature"]);
573+
let repo = Repository::at(test.root_path()).unwrap();
574+
575+
let snapshot = repo.capture_refs().unwrap();
576+
let result = delete_branch_if_safe(&repo, &snapshot, "feature", "main", false).unwrap();
577+
assert!(
578+
matches!(result.outcome, BranchDeletionOutcome::Integrated(_)),
579+
"expected Integrated, got a different outcome"
580+
);
581+
582+
// Ref should be gone.
583+
let exit = std::process::Command::new("git")
584+
.args(["rev-parse", "--verify", "--quiet", "refs/heads/feature"])
585+
.current_dir(test.root_path())
586+
.status()
587+
.unwrap();
588+
assert!(!exit.success(), "branch should have been deleted");
589+
}
448590

449591
#[test]
450592
fn test_branch_deletion_outcome_matching() {
451593
// Ensure the match patterns work correctly
452594
let outcomes = [
453595
(BranchDeletionOutcome::NotDeleted, false),
596+
(BranchDeletionOutcome::RetainedRaced, false),
454597
(BranchDeletionOutcome::ForceDeleted, true),
455598
(
456599
BranchDeletionOutcome::Integrated(IntegrationReason::SameCommit),

0 commit comments

Comments
 (0)