Delete worktree directory before git metadata cleanup during archive (#53959)

Richard Feldman created

When archiving a thread's linked worktree, `git worktree remove` can
fail to delete the working directory while still removing the admin
metadata in `.git/worktrees/<name>`. This leaves an orphaned folder on
disk that no longer appears in `git worktree list`.

## Fix

Delete the directory ourselves first with `std::fs::remove_dir_all`,
then call `git worktree remove --force` to clean up only the admin
entry. Git already handles the case where the working tree is already
gone (since
[git/git@ee6763a](https://github.com/git/git/commit/ee6763af0a3b97225803c6c908a29de40336cf38)).

Also fixes `FakeGitRepository::remove_worktree` to handle a missing
working tree directory (matching real git behavior) by scanning
`.git/worktrees/` entries when the `.git` pointer file can't be read.

Release Notes:

- Fixed archiving an agent thread sometimes leaving the worktree folder
on disk even though the git worktree was removed.

Change summary

crates/agent_ui/src/thread_worktree_archive.rs | 225 +++++++++++++++++++
crates/fs/src/fake_git_repo.rs                 |  54 +++-
2 files changed, 260 insertions(+), 19 deletions(-)

Detailed changes

crates/agent_ui/src/thread_worktree_archive.rs 🔗

@@ -189,20 +189,40 @@ async fn remove_root_after_worktree_removal(
         }
     }
 
+    // Delete the directory ourselves first, then tell git to clean up the
+    // metadata. This avoids a problem where `git worktree remove` can
+    // remove the metadata in `.git/worktrees/<name>` but fail to delete
+    // the directory (git continues past directory-removal errors), leaving
+    // an orphaned folder on disk. By deleting the directory first, we
+    // guarantee it's gone, and `git worktree remove --force` with a
+    // missing working tree just cleans up the admin entry.
+    let root_path = root.root_path.clone();
+    cx.background_executor()
+        .spawn(async move {
+            match std::fs::remove_dir_all(&root_path) {
+                Ok(()) => Ok(()),
+                Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()),
+                Err(error) => Err(error),
+            }
+        })
+        .await
+        .with_context(|| {
+            format!(
+                "failed to delete worktree directory '{}'",
+                root.root_path.display()
+            )
+        })?;
+
     let (repo, _temp_project) = find_or_create_repository(&root.main_repo_path, cx).await?;
-    // force=true is required because the working directory is still dirty
-    // — persist_worktree_state captures state into detached commits without
-    // modifying the real index or working tree, so git refuses to delete
-    // the worktree without --force.
     let receiver = repo.update(cx, |repo: &mut Repository, _cx| {
         repo.remove_worktree(root.root_path.clone(), true)
     });
     let result = receiver
         .await
-        .map_err(|_| anyhow!("git worktree removal was canceled"))?;
+        .map_err(|_| anyhow!("git worktree metadata cleanup was canceled"))?;
     // Keep _temp_project alive until after the await so the headless project isn't dropped mid-operation
     drop(_temp_project);
-    result.context("git worktree removal failed")?;
+    result.context("git worktree metadata cleanup failed")?;
 
     remove_empty_parent_dirs_up_to_worktrees_base(
         root.root_path.clone(),
@@ -792,7 +812,7 @@ fn current_app_state(cx: &mut AsyncApp) -> Option<Arc<AppState>> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use fs::FakeFs;
+    use fs::{FakeFs, Fs as _};
     use git::repository::Worktree as GitWorktree;
     use gpui::TestAppContext;
     use project::Project;
@@ -1029,4 +1049,195 @@ mod tests {
             );
         });
     }
+
+    #[gpui::test]
+    async fn test_remove_root_deletes_directory_and_git_metadata(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/project",
+            json!({
+                ".git": {},
+                "src": { "main.rs": "fn main() {}" }
+            }),
+        )
+        .await;
+        fs.set_branch_name(Path::new("/project/.git"), Some("main"));
+        fs.insert_branches(Path::new("/project/.git"), &["main", "feature"]);
+
+        fs.add_linked_worktree_for_repo(
+            Path::new("/project/.git"),
+            true,
+            GitWorktree {
+                path: PathBuf::from("/linked-worktree"),
+                ref_name: Some("refs/heads/feature".into()),
+                sha: "abc123".into(),
+                is_main: false,
+                is_bare: false,
+            },
+        )
+        .await;
+
+        let project = Project::test(
+            fs.clone(),
+            [Path::new("/project"), Path::new("/linked-worktree")],
+            cx,
+        )
+        .await;
+        project
+            .update(cx, |project, cx| project.git_scans_complete(cx))
+            .await;
+
+        let multi_workspace =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
+        let workspace = multi_workspace
+            .read_with(cx, |mw, _cx| mw.workspace().clone())
+            .unwrap();
+
+        cx.run_until_parked();
+
+        // Build the root plan while the worktree is still loaded.
+        let root = workspace
+            .read_with(cx, |_workspace, cx| {
+                build_root_plan(
+                    Path::new("/linked-worktree"),
+                    std::slice::from_ref(&workspace),
+                    cx,
+                )
+            })
+            .expect("should produce a root plan for the linked worktree");
+
+        assert!(fs.is_dir(Path::new("/linked-worktree")).await);
+
+        // Remove the root.
+        let task = cx.update(|cx| cx.spawn(async move |cx| remove_root(root, cx).await));
+        task.await.expect("remove_root should succeed");
+
+        cx.run_until_parked();
+
+        // The FakeFs directory should be gone (removed by the FakeGitRepository
+        // backend's remove_worktree implementation).
+        assert!(
+            !fs.is_dir(Path::new("/linked-worktree")).await,
+            "linked worktree directory should be removed from FakeFs"
+        );
+    }
+
+    #[gpui::test]
+    async fn test_remove_root_succeeds_when_directory_already_gone(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/project",
+            json!({
+                ".git": {},
+                "src": { "main.rs": "fn main() {}" }
+            }),
+        )
+        .await;
+        fs.set_branch_name(Path::new("/project/.git"), Some("main"));
+        fs.insert_branches(Path::new("/project/.git"), &["main", "feature"]);
+
+        fs.add_linked_worktree_for_repo(
+            Path::new("/project/.git"),
+            true,
+            GitWorktree {
+                path: PathBuf::from("/linked-worktree"),
+                ref_name: Some("refs/heads/feature".into()),
+                sha: "abc123".into(),
+                is_main: false,
+                is_bare: false,
+            },
+        )
+        .await;
+
+        let project = Project::test(
+            fs.clone(),
+            [Path::new("/project"), Path::new("/linked-worktree")],
+            cx,
+        )
+        .await;
+        project
+            .update(cx, |project, cx| project.git_scans_complete(cx))
+            .await;
+
+        let multi_workspace =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
+        let workspace = multi_workspace
+            .read_with(cx, |mw, _cx| mw.workspace().clone())
+            .unwrap();
+
+        cx.run_until_parked();
+
+        let root = workspace
+            .read_with(cx, |_workspace, cx| {
+                build_root_plan(
+                    Path::new("/linked-worktree"),
+                    std::slice::from_ref(&workspace),
+                    cx,
+                )
+            })
+            .expect("should produce a root plan for the linked worktree");
+
+        // Manually remove the worktree directory from FakeFs before calling
+        // remove_root, simulating the directory being deleted externally.
+        fs.as_ref()
+            .remove_dir(
+                Path::new("/linked-worktree"),
+                fs::RemoveOptions {
+                    recursive: true,
+                    ignore_if_not_exists: false,
+                },
+            )
+            .await
+            .unwrap();
+        assert!(!fs.as_ref().is_dir(Path::new("/linked-worktree")).await);
+
+        // remove_root should still succeed — the std::fs::remove_dir_all
+        // handles NotFound, and git worktree remove handles a missing
+        // working tree directory.
+        let task = cx.update(|cx| cx.spawn(async move |cx| remove_root(root, cx).await));
+        task.await
+            .expect("remove_root should succeed even when directory is already gone");
+    }
+
+    #[test]
+    fn test_remove_dir_all_deletes_real_directory() {
+        let tmp = TempDir::new().unwrap();
+        let worktree_dir = tmp.path().join("linked-worktree");
+        std::fs::create_dir_all(worktree_dir.join("src")).unwrap();
+        std::fs::write(worktree_dir.join("src/main.rs"), "fn main() {}").unwrap();
+        std::fs::write(worktree_dir.join("README.md"), "# Hello").unwrap();
+
+        assert!(worktree_dir.is_dir());
+
+        // This is the same pattern used in remove_root_after_worktree_removal.
+        match std::fs::remove_dir_all(&worktree_dir) {
+            Ok(()) => {}
+            Err(error) if error.kind() == std::io::ErrorKind::NotFound => {}
+            Err(error) => panic!("unexpected error: {error}"),
+        }
+
+        assert!(
+            !worktree_dir.exists(),
+            "worktree directory should be deleted"
+        );
+    }
+
+    #[test]
+    fn test_remove_dir_all_handles_not_found() {
+        let tmp = TempDir::new().unwrap();
+        let nonexistent = tmp.path().join("does-not-exist");
+
+        assert!(!nonexistent.exists());
+
+        // Should not panic — NotFound is handled gracefully.
+        match std::fs::remove_dir_all(&nonexistent) {
+            Ok(()) => panic!("expected NotFound error"),
+            Err(error) if error.kind() == std::io::ErrorKind::NotFound => {}
+            Err(error) => panic!("unexpected error: {error}"),
+        }
+    }
 }

crates/fs/src/fake_git_repo.rs 🔗

@@ -1,3 +1,5 @@
+use std::path::Path;
+
 use crate::{FakeFs, FakeFsEntry, Fs, RemoveOptions, RenameOptions};
 use anyhow::{Context as _, Result, bail};
 use collections::{HashMap, HashSet};
@@ -106,6 +108,28 @@ impl FakeGitRepository {
         }
         .boxed()
     }
+
+    /// Scans `.git/worktrees/*/gitdir` to find the admin entry directory for a
+    /// worktree at the given checkout path. Used when the working tree directory
+    /// has already been deleted and we can't read its `.git` pointer file.
+    async fn find_worktree_entry_dir_by_path(&self, path: &Path) -> Option<PathBuf> {
+        use futures::StreamExt;
+
+        let worktrees_dir = self.common_dir_path.join("worktrees");
+        let mut entries = self.fs.read_dir(&worktrees_dir).await.ok()?;
+        while let Some(Ok(entry_path)) = entries.next().await {
+            if let Ok(gitdir_content) = self.fs.load(&entry_path.join("gitdir")).await {
+                let worktree_path = PathBuf::from(gitdir_content.trim())
+                    .parent()
+                    .map(PathBuf::from)
+                    .unwrap_or_default();
+                if worktree_path == path {
+                    return Some(entry_path);
+                }
+            }
+        }
+        None
+    }
 }
 
 impl GitRepository for FakeGitRepository {
@@ -688,24 +712,30 @@ impl GitRepository for FakeGitRepository {
         async move {
             executor.simulate_random_delay().await;
 
-            // Read the worktree's .git file to find its entry directory.
+            // Try to read the worktree's .git file to find its entry
+            // directory. If the working tree is already gone (e.g. the
+            // caller deleted it before asking git to clean up), fall back
+            // to scanning `.git/worktrees/*/gitdir` for a matching path,
+            // mirroring real git's behavior with `--force`.
             let dot_git_file = path.join(".git");
-            let content = fs
-                .load(&dot_git_file)
-                .await
-                .with_context(|| format!("no worktree found at path: {}", path.display()))?;
-            let gitdir = content
-                .strip_prefix("gitdir:")
-                .context("invalid .git file in worktree")?
-                .trim();
-            let worktree_entry_dir = PathBuf::from(gitdir);
+            let worktree_entry_dir = if let Ok(content) = fs.load(&dot_git_file).await {
+                let gitdir = content
+                    .strip_prefix("gitdir:")
+                    .context("invalid .git file in worktree")?
+                    .trim();
+                PathBuf::from(gitdir)
+            } else {
+                self.find_worktree_entry_dir_by_path(&path)
+                    .await
+                    .with_context(|| format!("no worktree found at path: {}", path.display()))?
+            };
 
-            // Remove the worktree checkout directory.
+            // Remove the worktree checkout directory if it still exists.
             fs.remove_dir(
                 &path,
                 RemoveOptions {
                     recursive: true,
-                    ignore_if_not_exists: false,
+                    ignore_if_not_exists: true,
                 },
             )
             .await?;