From e7ba1715688433c92586e7493decc11e424e804e Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Thu, 9 Apr 2026 13:54:34 -0400 Subject: [PATCH] Clean up orphaned files on git worktree creation failure (#53287) Update `await_and_rollback_on_failure` in `agent_panel.rs` to comprehensively clean up both git metadata and filesystem artifacts when worktree creation fails. Release Notes: - Clean up files and git metadata when worktree creation fails during new agent thread setup. --- crates/agent_ui/src/agent_panel.rs | 369 +++++++++++++++++++++++++++-- 1 file changed, 343 insertions(+), 26 deletions(-) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index f5bc572f853d770981d36853222cf10f7108a26b..3c735832f7ca30deca30977f12506697df25841f 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -2795,6 +2795,7 @@ impl AgentPanel { PathBuf, futures::channel::oneshot::Receiver>, )>, + fs: Arc, cx: &mut AsyncWindowContext, ) -> Result> { let mut created_paths: Vec = Vec::new(); @@ -2803,10 +2804,10 @@ impl AgentPanel { let mut first_error: Option = None; for (repo, new_path, receiver) in creation_infos { + repos_and_paths.push((repo.clone(), new_path.clone())); match receiver.await { Ok(Ok(())) => { - created_paths.push(new_path.clone()); - repos_and_paths.push((repo, new_path)); + created_paths.push(new_path); } Ok(Err(err)) => { if first_error.is_none() { @@ -2825,34 +2826,66 @@ impl AgentPanel { return Ok(created_paths); }; - // Rollback all successfully created worktrees - let mut rollback_receivers = Vec::new(); + // Rollback all attempted worktrees (both successful and failed) + let mut rollback_futures = Vec::new(); for (rollback_repo, rollback_path) in &repos_and_paths { - if let Ok(receiver) = cx.update(|_, cx| { - rollback_repo.update(cx, |repo, _cx| { - repo.remove_worktree(rollback_path.clone(), true) + let receiver = cx + .update(|_, cx| { + rollback_repo.update(cx, |repo, _cx| { + repo.remove_worktree(rollback_path.clone(), true) + }) }) - }) { - rollback_receivers.push((rollback_path.clone(), receiver)); - } + .ok(); + + rollback_futures.push((rollback_path.clone(), receiver)); } + let mut rollback_failures: Vec = Vec::new(); - for (path, receiver) in rollback_receivers { - match receiver.await { - Ok(Ok(())) => {} - Ok(Err(rollback_err)) => { - log::error!( - "failed to rollback worktree at {}: {rollback_err}", - path.display() - ); - rollback_failures.push(format!("{}: {rollback_err}", path.display())); + for (path, receiver_opt) in rollback_futures { + let mut git_remove_failed = false; + + if let Some(receiver) = receiver_opt { + match receiver.await { + Ok(Ok(())) => {} + Ok(Err(rollback_err)) => { + log::error!( + "git worktree remove failed for {}: {rollback_err}", + path.display() + ); + git_remove_failed = true; + } + Err(canceled) => { + log::error!( + "git worktree remove failed for {}: {canceled}", + path.display() + ); + git_remove_failed = true; + } } - Err(rollback_err) => { - log::error!( - "failed to rollback worktree at {}: {rollback_err}", - path.display() - ); - rollback_failures.push(format!("{}: {rollback_err}", path.display())); + } else { + log::error!( + "failed to dispatch git worktree remove for {}", + path.display() + ); + git_remove_failed = true; + } + + // `git worktree remove` normally removes this directory, but since + // `git worktree remove` failed (or wasn't dispatched), manually rm the directory. + if git_remove_failed { + if let Err(fs_err) = fs + .remove_dir( + &path, + fs::RemoveOptions { + recursive: true, + ignore_if_not_exists: true, + }, + ) + .await + { + let msg = format!("{}: failed to remove directory: {fs_err}", path.display()); + log::error!("{}", msg); + rollback_failures.push(msg); } } } @@ -3058,8 +3091,10 @@ impl AgentPanel { } }; + let fs = cx.update(|_, cx| ::global(cx))?; + let created_paths = - match Self::await_and_rollback_on_failure(creation_infos, cx).await { + match Self::await_and_rollback_on_failure(creation_infos, fs, cx).await { Ok(paths) => paths, Err(err) => { this.update_in(cx, |this, window, cx| { @@ -4769,6 +4804,7 @@ mod tests { }; use acp_thread::{StubAgentConnection, ThreadStatus}; use agent_servers::CODEX_ID; + use feature_flags::FeatureFlagAppExt; use fs::FakeFs; use gpui::{TestAppContext, VisualTestContext}; use project::Project; @@ -6671,6 +6707,287 @@ mod tests { }); } + #[gpui::test] + async fn test_rollback_all_succeed_returns_ok(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + cx.update(|cx| { + cx.update_flags(true, vec!["agent-v2".to_string()]); + agent::ThreadStore::init_global(cx); + language_model::LanguageModelRegistry::test(cx); + ::set_global(fs.clone(), cx); + }); + + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + cx.executor().run_until_parked(); + + let repository = project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + + let path_a = PathBuf::from("/worktrees/branch/project_a"); + let path_b = PathBuf::from("/worktrees/branch/project_b"); + + let (sender_a, receiver_a) = futures::channel::oneshot::channel::>(); + let (sender_b, receiver_b) = futures::channel::oneshot::channel::>(); + sender_a.send(Ok(())).unwrap(); + sender_b.send(Ok(())).unwrap(); + + let creation_infos = vec![ + (repository.clone(), path_a.clone(), receiver_a), + (repository.clone(), path_b.clone(), receiver_b), + ]; + + let fs_clone = fs.clone(); + let result = multi_workspace + .update(cx, |_, window, cx| { + window.spawn(cx, async move |cx| { + AgentPanel::await_and_rollback_on_failure(creation_infos, fs_clone, cx).await + }) + }) + .unwrap() + .await; + + let paths = result.expect("all succeed should return Ok"); + assert_eq!(paths, vec![path_a, path_b]); + } + + #[gpui::test] + async fn test_rollback_on_failure_attempts_all_worktrees(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + cx.update(|cx| { + cx.update_flags(true, vec!["agent-v2".to_string()]); + agent::ThreadStore::init_global(cx); + language_model::LanguageModelRegistry::test(cx); + ::set_global(fs.clone(), cx); + }); + + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + cx.executor().run_until_parked(); + + let repository = project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + + // Actually create a worktree so it exists in FakeFs for rollback to find. + let success_path = PathBuf::from("/worktrees/branch/project"); + cx.update(|cx| { + repository.update(cx, |repo, _| { + repo.create_worktree( + git::repository::CreateWorktreeTarget::NewBranch { + branch_name: "branch".to_string(), + base_sha: None, + }, + success_path.clone(), + ) + }) + }) + .await + .unwrap() + .unwrap(); + cx.executor().run_until_parked(); + + // Verify the worktree directory exists before rollback. + assert!( + fs.is_dir(&success_path).await, + "worktree directory should exist before rollback" + ); + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + + // Build creation_infos: one success, one failure. + let failed_path = PathBuf::from("/worktrees/branch/failed_project"); + + let (sender_ok, receiver_ok) = futures::channel::oneshot::channel::>(); + let (sender_err, receiver_err) = futures::channel::oneshot::channel::>(); + sender_ok.send(Ok(())).unwrap(); + sender_err + .send(Err(anyhow!("branch already exists"))) + .unwrap(); + + let creation_infos = vec![ + (repository.clone(), success_path.clone(), receiver_ok), + (repository.clone(), failed_path.clone(), receiver_err), + ]; + + let fs_clone = fs.clone(); + let result = multi_workspace + .update(cx, |_, window, cx| { + window.spawn(cx, async move |cx| { + AgentPanel::await_and_rollback_on_failure(creation_infos, fs_clone, cx).await + }) + }) + .unwrap() + .await; + + assert!( + result.is_err(), + "should return error when any creation fails" + ); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.contains("branch already exists"), + "error should mention the original failure: {err_msg}" + ); + + // The successful worktree should have been rolled back by git. + cx.executor().run_until_parked(); + assert!( + !fs.is_dir(&success_path).await, + "successful worktree directory should be removed by rollback" + ); + } + + #[gpui::test] + async fn test_rollback_on_canceled_receiver(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + cx.update(|cx| { + cx.update_flags(true, vec!["agent-v2".to_string()]); + agent::ThreadStore::init_global(cx); + language_model::LanguageModelRegistry::test(cx); + ::set_global(fs.clone(), cx); + }); + + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + cx.executor().run_until_parked(); + + let repository = project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + + let path = PathBuf::from("/worktrees/branch/project"); + + // Drop the sender to simulate a canceled receiver. + let (_sender, receiver) = futures::channel::oneshot::channel::>(); + drop(_sender); + + let creation_infos = vec![(repository.clone(), path.clone(), receiver)]; + + let fs_clone = fs.clone(); + let result = multi_workspace + .update(cx, |_, window, cx| { + window.spawn(cx, async move |cx| { + AgentPanel::await_and_rollback_on_failure(creation_infos, fs_clone, cx).await + }) + }) + .unwrap() + .await; + + assert!( + result.is_err(), + "should return error when receiver is canceled" + ); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.contains("canceled"), + "error should mention cancellation: {err_msg}" + ); + } + + #[gpui::test] + async fn test_rollback_cleans_up_orphan_directories(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.executor()); + cx.update(|cx| { + cx.update_flags(true, vec!["agent-v2".to_string()]); + agent::ThreadStore::init_global(cx); + language_model::LanguageModelRegistry::test(cx); + ::set_global(fs.clone(), cx); + }); + + fs.insert_tree( + "/project", + json!({ + ".git": {}, + "src": { "main.rs": "fn main() {}" } + }), + ) + .await; + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + cx.executor().run_until_parked(); + + let repository = project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + + let multi_workspace = + cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + + // Simulate the orphan state: create_dir_all was called but git + // worktree add failed, leaving a directory with leftover files. + let orphan_path = PathBuf::from("/worktrees/branch/orphan_project"); + fs.insert_tree( + "/worktrees/branch/orphan_project", + json!({ "leftover.txt": "junk" }), + ) + .await; + + assert!( + fs.is_dir(&orphan_path).await, + "orphan dir should exist before rollback" + ); + + let (sender, receiver) = futures::channel::oneshot::channel::>(); + sender.send(Err(anyhow!("hook failed"))).unwrap(); + + let creation_infos = vec![(repository.clone(), orphan_path.clone(), receiver)]; + + let fs_clone = fs.clone(); + let result = multi_workspace + .update(cx, |_, window, cx| { + window.spawn(cx, async move |cx| { + AgentPanel::await_and_rollback_on_failure(creation_infos, fs_clone, cx).await + }) + }) + .unwrap() + .await; + + cx.executor().run_until_parked(); + + assert!(result.is_err()); + assert!( + !fs.is_dir(&orphan_path).await, + "orphan worktree directory should be removed by filesystem cleanup" + ); + } + #[gpui::test] async fn test_worktree_creation_for_remote_project( cx: &mut TestAppContext,