From 8c083db5b14495c85fae37c60fb1fc1133caed83 Mon Sep 17 00:00:00 2001 From: Jonathan Liebig Date: Thu, 22 Oct 2026 18:32:47 +0200 Subject: [PATCH] fix: scope worktree leftover cleanup to failed removals Require a prior partial-removal lease before cleaning a checkout whose .git is gone, and refuse a replacement repository at the path. Check submodule state without spawning git, and wrap the long force warning. refs #5478 --- src/app/api/worktrees.rs | 6 + src/app/api/worktrees/deferred.rs | 28 ++-- src/app/mod.rs | 6 +- .../tests/agents_worktrees_notifications.rs | 2 +- src/client/shell/worktree_overlays.rs | 17 +- src/events.rs | 3 + src/server/headless/tests/mod.rs | 1 + src/worktree.rs | 149 ++++++++++++------ 8 files changed, 145 insertions(+), 67 deletions(-) diff --git a/src/app/api/worktrees.rs b/src/app/api/worktrees.rs index f213c2b18..ca31292be 100644 --- a/src/app/api/worktrees.rs +++ b/src/app/api/worktrees.rs @@ -2694,8 +2694,12 @@ mod tests { respond_to, }), result: Err("simulated remove failure".into()), + leftover_pending: true, }); assert!(pane_updates.is_empty()); + assert!(app + .pending_worktree_remove_leftovers + .contains(&crate::worktree::canonical_or_original(&checkout))); let response = response_rx .recv_timeout(std::time::Duration::from_secs(2)) @@ -2781,6 +2785,7 @@ mod tests { respond_to, }), result: Ok(()), + leftover_pending: false, }); let response = response_rx @@ -2851,6 +2856,7 @@ mod tests { respond_to, }), result: Ok(()), + leftover_pending: false, }); let response = response_rx diff --git a/src/app/api/worktrees/deferred.rs b/src/app/api/worktrees/deferred.rs index eb4d2a904..7750bcf14 100644 --- a/src/app/api/worktrees/deferred.rs +++ b/src/app/api/worktrees/deferred.rs @@ -310,10 +310,7 @@ impl App { ); return; } - if crate::worktree::checkout_has_initialized_submodules( - &space.checkout_path, - params.trust_repository, - ) { + if crate::worktree::checkout_has_initialized_submodules(&space.checkout_path) { Self::send_api_response( respond_to, encode_error( @@ -329,6 +326,12 @@ impl App { let workspace_internal_id = self.state.workspaces[ws_idx].id.clone(); let checkout_key = crate::worktree::canonical_or_original(&space.checkout_path); + // Only a path that an earlier forced attempt left behind may be cleaned + // without its `.git` still proving ownership. + let allow_unregistered_leftover = params.force + && self + .pending_worktree_remove_leftovers + .contains(&checkout_key); if self .pending_api_worktree_removes .contains_key(&workspace_internal_id) @@ -396,22 +399,22 @@ impl App { forced: params.force, api_request: Some(api_request), result: Ok(()), + leftover_pending: false, }; let spawned = crate::thread_spawn::spawn_named_with( "herdr-worktree-remove", finished, move |mut finished| { - // The workspace membership was verified before spawning this - // thread, so a forced request may clean a checkout that Git has - // already unregistered but could not delete. - finished.result = crate::worktree::run_worktree_remove_command_with_recovery( + let outcome = crate::worktree::run_worktree_remove_command_with_recovery( &command, &repo_root, &finished.path, finished.forced, trust_repository, - finished.forced, + allow_unregistered_leftover, ); + finished.result = outcome.result; + finished.leftover_pending = outcome.leftover_pending; let _ = event_tx.blocking_send(AppEvent::WorktreeRemoveFinished(Box::new(finished))); }, @@ -566,6 +569,13 @@ impl App { .remove(&result.workspace_id); self.pending_api_worktree_remove_paths .remove(&api.checkout_key); + if result.leftover_pending { + self.pending_worktree_remove_leftovers + .insert(api.checkout_key.clone()); + } else if result.result.is_ok() { + self.pending_worktree_remove_leftovers + .remove(&api.checkout_key); + } if let Err(message) = result.result { let pane_updates = self.restore_shutdown_worktree_panes( diff --git a/src/app/mod.rs b/src/app/mod.rs index 6effa3a57..e33c876c8 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -30,7 +30,7 @@ mod theme_sync; mod window_title; mod worktrees; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; #[cfg(unix)] use std::io; use std::sync::Arc; @@ -130,6 +130,9 @@ pub struct App { pub(crate) worktree_read_slots: std::sync::Arc, pub(crate) pending_api_worktree_removes: HashMap, pub(crate) pending_api_worktree_remove_paths: HashMap, + /// Forced removals that left an unregistered checkout behind. A retry may + /// finish deleting that specific leftover, but never a replacement repo. + pub(crate) pending_worktree_remove_leftovers: HashSet, pub(crate) pending_worktree_remove_runtime_exits: HashMap, pub(crate) pending_worktree_remove_runtime_restores: HashMap, pub(crate) next_api_worktree_operation_id: u64, @@ -639,6 +642,7 @@ impl App { worktree_read_slots: std::sync::Arc::new(tokio::sync::Semaphore::new(8)), pending_api_worktree_removes: HashMap::new(), pending_api_worktree_remove_paths: HashMap::new(), + pending_worktree_remove_leftovers: HashSet::new(), pending_worktree_remove_runtime_exits: HashMap::new(), pending_worktree_remove_runtime_restores: HashMap::new(), next_api_worktree_operation_id: 1, diff --git a/src/client/shell/tests/agents_worktrees_notifications.rs b/src/client/shell/tests/agents_worktrees_notifications.rs index 78f8ce1e1..d92984c14 100644 --- a/src/client/shell/tests/agents_worktrees_notifications.rs +++ b/src/client/shell/tests/agents_worktrees_notifications.rs @@ -1487,7 +1487,7 @@ fn worktree_remove_escalates_recoverable_failure_to_force_confirmation() { continue; } assert!(remove.error.is_none()); - let frame = state.compose(106, 30).expect("force remove modal"); + let frame = state.compose(60, 20).expect("force remove modal"); let text = frame .cells .chunks(frame.width as usize) diff --git a/src/client/shell/worktree_overlays.rs b/src/client/shell/worktree_overlays.rs index f9b3fda72..583cfea9c 100644 --- a/src/client/shell/worktree_overlays.rs +++ b/src/client/shell/worktree_overlays.rs @@ -353,16 +353,25 @@ pub(super) fn render_worktree_remove_overlay( inner.x, inner.y + 4, inner.width, - " Dirty files and unpushed submodule work will be permanently deleted.", + " Uncommitted or unpushed work in this checkout", Style::default().fg(p.red).bg(p.panel_bg), ); - } - if remove.removing { put_text( b, inner.x, inner.y + 5, inner.width, + " or its submodules will be permanently deleted.", + Style::default().fg(p.red).bg(p.panel_bg), + ); + } + let status_y = inner.y + if remove.force_confirmation { 6 } else { 5 }; + if remove.removing { + put_text( + b, + inner.x, + status_y, + inner.width, " removing…", Style::default().fg(p.accent).bg(p.panel_bg), ); @@ -370,7 +379,7 @@ pub(super) fn render_worktree_remove_overlay( put_text( b, inner.x, - inner.y + 5, + status_y, inner.width, &format!(" {error}"), Style::default().fg(p.red).bg(p.panel_bg), diff --git a/src/events.rs b/src/events.rs index 3faf8989a..7d8d11e0b 100644 --- a/src/events.rs +++ b/src/events.rs @@ -50,6 +50,9 @@ pub struct WorktreeRemoveResult { pub forced: bool, pub api_request: Option, pub result: Result<(), String>, + /// Git unregistered the checkout but left files behind that could not be + /// removed yet. A later forced retry may finish cleaning this path. + pub leftover_pending: bool, } #[derive(Debug)] diff --git a/src/server/headless/tests/mod.rs b/src/server/headless/tests/mod.rs index 3ab7ec0fd..a446df418 100644 --- a/src/server/headless/tests/mod.rs +++ b/src/server/headless/tests/mod.rs @@ -5357,6 +5357,7 @@ fn terminal_attach_client_exits_when_worktree_remove_succeeds() { respond_to, }), result: Ok(()), + leftover_pending: false, } ))) ); diff --git a/src/worktree.rs b/src/worktree.rs index d30d8da16..a06775c70 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -246,23 +246,13 @@ pub(crate) fn checkout_has_dirty_files( } #[cfg(any(windows, test))] -pub(crate) fn checkout_has_initialized_submodules(path: &Path, trust_repository: bool) -> bool { - if !path.join(".gitmodules").is_file() { - return false; - } - let Ok(output) = repository_git_command(path, trust_repository) - .args(["submodule", "status", "--recursive"]) - .output() - else { +pub(crate) fn checkout_has_initialized_submodules(path: &Path) -> bool { + let Some(gitdir) = checkout_gitdir(path) else { return false; }; - if !output.status.success() { - return false; - } - // `git submodule status` prefixes uninitialized entries with `-`. - String::from_utf8_lossy(&output.stdout) - .lines() - .any(|line| matches!(line.as_bytes().first(), Some(b' ' | b'+' | b'U'))) + std::fs::read_dir(gitdir.join("modules")) + .map(|mut entries| entries.next().is_some()) + .unwrap_or(false) } pub(crate) fn build_worktree_add_new_branch_command( @@ -372,6 +362,11 @@ pub(crate) fn run_worktree_command(command: &WorktreeCommand) -> Result<(), Stri }) } +pub(crate) struct WorktreeRemoveOutcome { + pub result: Result<(), String>, + pub leftover_pending: bool, +} + pub(crate) fn run_worktree_remove_command_with_recovery( command: &WorktreeCommand, repo_root: &Path, @@ -379,37 +374,74 @@ pub(crate) fn run_worktree_remove_command_with_recovery( force: bool, trust_repository: bool, allow_unregistered_leftover: bool, -) -> Result<(), String> { +) -> WorktreeRemoveOutcome { // Capture the checkout identity before Git removes `.git`. A forced removal // can unregister the worktree and then fail to delete the directory, at // which point the checkout no longer proves it belongs to this repository. - let checkout_gitdir = force.then(|| checkout_gitdir(path)).flatten(); - match run_worktree_command(command) { - Ok(()) => Ok(()), - Err(err) if force => { - if worktree_list_contains_path(repo_root, path, trust_repository)? { - return Err(err); - } - if !path.exists() { - return Ok(()); - } - let known_checkout = checkout_gitdir + let checkout_identity = force.then(|| checkout_gitdir(path)).flatten(); + let Err(err) = run_worktree_command(command) else { + return WorktreeRemoveOutcome { + result: Ok(()), + leftover_pending: false, + }; + }; + if !force { + return WorktreeRemoveOutcome { + result: Err(err), + leftover_pending: false, + }; + } + match worktree_list_contains_path(repo_root, path, trust_repository) { + Ok(true) => { + return WorktreeRemoveOutcome { + result: Err(err), + leftover_pending: false, + }; + } + Ok(false) => {} + Err(list_err) => { + return WorktreeRemoveOutcome { + result: Err(list_err), + leftover_pending: false, + }; + } + } + if !path.exists() { + return WorktreeRemoveOutcome { + result: Ok(()), + leftover_pending: false, + }; + } + let current_identity = checkout_gitdir(path) + .as_deref() + .is_some_and(|gitdir| gitdir_belongs_to_repo(gitdir, repo_root, trust_repository)); + // A replacement repository at the checkout path has its own `.git`, which + // does not identify this repository's linked worktree. Never delete it. + let replacement_checkout = path.join(".git").exists() && !current_identity; + let known_checkout = !replacement_checkout + && (current_identity + || checkout_identity .as_deref() .is_some_and(|gitdir| gitdir_belongs_to_repo(gitdir, repo_root, trust_repository)) - || leftover_worktree_checkout_matches_repo(repo_root, path, trust_repository) - || allow_unregistered_leftover; - if !known_checkout { - return Err(err); - } - std::fs::remove_dir_all(path).map_err(|remove_err| { - format!( - "{err}; failed to remove leftover checkout {}: {remove_err}", - path.display() - ) - })?; - Ok(()) - } - Err(err) => Err(err), + || allow_unregistered_leftover); + if !known_checkout { + return WorktreeRemoveOutcome { + result: Err(err), + leftover_pending: false, + }; + } + match std::fs::remove_dir_all(path) { + Ok(()) => WorktreeRemoveOutcome { + result: Ok(()), + leftover_pending: false, + }, + Err(remove_err) => WorktreeRemoveOutcome { + result: Err(format!( + "{err}; failed to remove leftover checkout {}: {remove_err}", + path.display() + )), + leftover_pending: true, + }, } } @@ -434,15 +466,6 @@ fn gitdir_belongs_to_repo(gitdir: &Path, repo_root: &Path, trust_repository: boo canonical_or_original(gitdir).starts_with(canonical_or_original(&worktrees_dir)) } -fn leftover_worktree_checkout_matches_repo( - repo_root: &Path, - path: &Path, - trust_repository: bool, -) -> bool { - checkout_gitdir(path) - .is_some_and(|gitdir| gitdir_belongs_to_repo(&gitdir, repo_root, trust_repository)) -} - fn git_common_worktrees_dir(repo_root: &Path, trust_repository: bool) -> Option { let output = repository_git_command(repo_root, trust_repository) .args(["rev-parse", "--git-common-dir"]) @@ -958,7 +981,7 @@ prunable stale false, ); run_worktree_command(&add).unwrap(); - assert!(!checkout_has_initialized_submodules(&checkout, false)); + assert!(!checkout_has_initialized_submodules(&checkout)); run_git( &checkout, &[ @@ -970,7 +993,7 @@ prunable stale "--quiet", ], ); - assert!(checkout_has_initialized_submodules(&checkout, false)); + assert!(checkout_has_initialized_submodules(&checkout)); assert!(!checkout_has_dirty_files(&checkout, false).unwrap()); let remove = build_worktree_remove_command(&repo, &checkout, false, false); @@ -985,6 +1008,7 @@ prunable stale let forced = build_worktree_remove_command(&repo, &checkout, true, false); run_worktree_remove_command_with_recovery(&forced, &repo, &checkout, true, false, false) + .result .unwrap(); assert!(!checkout.exists()); assert!(!worktree_list_contains_path(&repo, &checkout, false).unwrap()); @@ -1092,6 +1116,7 @@ prunable stale std::fs::write(checkout.join("leftover"), "leftover\n").unwrap(); run_worktree_remove_command_with_recovery(&remove, &repo, &checkout, true, false, false) + .result .unwrap(); assert!(!checkout.exists()); @@ -1115,12 +1140,31 @@ prunable stale // remove the directory. A confirmed forced removal of the workspace's // checkout may still clean up the leftover. run_worktree_remove_command_with_recovery(&remove, &repo, &checkout, true, false, true) + .result .unwrap(); assert!(!checkout.exists()); let _ = std::fs::remove_dir_all(repo); } + #[test] + fn forced_worktree_remove_recovery_keeps_replacement_repository_with_leftover_lease() { + let repo = create_committed_repo("worktree-recovery-replacement-repo"); + let checkout = unique_temp_path("worktree-recovery-replacement-checkout"); + std::fs::create_dir_all(&checkout).unwrap(); + run_git(&checkout, &["init", "--quiet"]); + + let remove = build_worktree_remove_command(&repo, &checkout, true, false); + let outcome = + run_worktree_remove_command_with_recovery(&remove, &repo, &checkout, true, false, true); + + assert!(outcome.result.is_err()); + assert!(!outcome.leftover_pending); + assert!(checkout.join(".git").exists()); + let _ = std::fs::remove_dir_all(checkout); + let _ = std::fs::remove_dir_all(repo); + } + #[test] fn forced_worktree_remove_recovery_keeps_unrelated_replacement_directory() { let repo = create_committed_repo("worktree-recovery-unrelated-repo"); @@ -1137,6 +1181,7 @@ prunable stale let err = run_worktree_remove_command_with_recovery( &remove, &repo, &checkout, true, false, false, ) + .result .expect_err("unrelated replacement directory should not be removed"); assert!(is_not_working_tree_remove_error(&err));