From a6b993c6d322003b9e6e5f89e9b4d9daa3c36edb Mon Sep 17 00:00:00 2001 From: Tryanks Date: Tue, 29 Sep 2026 05:28:33 +0800 Subject: [PATCH] fix(worktree): keep worktrees a thread or uncommitted work still uses A fork keeps its source's cwd but deliberately carries no worktree ownership marker, so both the delete dialog and the startup orphan sweep treated the source's worktree as unused once the source was gone. SessionMeta::shares_worktree_with now counts any session whose cwd lies inside the worktree as well as one owning the same branch. The delete dialog and the host both use it, so deleting the source no longer removes the directory its fork works in. The orphan sweep keeps any directory a session's cwd lies in, and no longer forces removal: git refuses to remove a worktree with modified or untracked files, so a worktree kept on delete loses no uncommitted work. A clean kept worktree is still reclaimed; its tcode/ branch survives. Recording keep decisions belongs to #530. Closes #517 Refs #516 --- crates/core/src/project.rs | 39 +++++++++++++++++++++++++ crates/runtime/src/app/sessions.rs | 30 +++++++++++++++---- crates/services/src/worktree.rs | 46 +++++++++++++++++++++--------- crates/ui/src/store/mod.rs | 19 ++++++------ 4 files changed, 104 insertions(+), 30 deletions(-) diff --git a/crates/core/src/project.rs b/crates/core/src/project.rs index 731e815d..797ead89 100644 --- a/crates/core/src/project.rs +++ b/crates/core/src/project.rs @@ -140,6 +140,21 @@ pub struct SessionMeta { } impl SessionMeta { + /// Whether `other` works in the worktree this session owns. A fork keeps + /// the source's cwd without the `worktree` ownership marker, so the cwd + /// decides as well as the branch. + pub fn shares_worktree_with(&self, other: &SessionMeta) -> bool { + let Some(worktree) = &self.worktree else { + return false; + }; + other.id != self.id + && (other.cwd.starts_with(&self.cwd) + || other + .worktree + .as_ref() + .is_some_and(|other| other.branch == worktree.branch)) + } + /// The key [`Settings::provider_color`] resolves this thread's color from: /// a user profile id, an ACP agent (`acp:`), or the built-in /// [`provider_key`]. A user profile is its own provider to the user even @@ -540,6 +555,30 @@ mod tests { assert_eq!(acp.provider_color_key(), "acp:gemini"); } + #[test] + fn worktree_is_shared_by_a_fork_in_its_cwd_but_not_by_a_sibling_directory() { + let mut owner = SessionMeta::new(ProviderKind::Codex, PathBuf::from("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/wt/source"), None); + owner.worktree = Some(WorktreeInfo { + root_project_path: PathBuf::from("/repo"), + base: "main".into(), + branch: "tcode/source".into(), + }); + let fork = SessionMeta::new(ProviderKind::Codex, owner.cwd.clone(), None); + let nested = SessionMeta::new(ProviderKind::Codex, owner.cwd.join("crates/core"), None); + let sibling = SessionMeta::new(ProviderKind::Codex, PathBuf::from("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/wt/source-2"), None); + let checkout = SessionMeta::new(ProviderKind::Codex, PathBuf::from("/repo"), None); + + assert!(owner.shares_worktree_with(&fork)); + assert!(owner.shares_worktree_with(&nested)); + assert!(!owner.shares_worktree_with(&sibling)); + assert!(!owner.shares_worktree_with(&checkout)); + assert!(!owner.shares_worktree_with(&owner)); + assert!( + !fork.shares_worktree_with(&owner), + "a fork owns no worktree" + ); + } + fn candidates( sessions: &[SessionMeta], now: u64, diff --git a/crates/runtime/src/app/sessions.rs b/crates/runtime/src/app/sessions.rs index daad0547..92d09119 100644 --- a/crates/runtime/src/app/sessions.rs +++ b/crates/runtime/src/app/sessions.rs @@ -974,10 +974,20 @@ impl AppState { } self.close_orchestrator_children(session_id, cx); let worktree_remove = meta.as_ref().and_then(|meta| { - (remove_worktree && meta.worktree.is_some()).then(|| { - let worktree = meta.worktree.as_ref().unwrap(); - (worktree.root_project_path.clone(), meta.cwd.clone()) - }) + let worktree = meta.worktree.as_ref().filter(|_| remove_worktree)?; + let shared = self + .sessions + .iter() + .chain(self.residents.live.values().map(|session| &session.meta)) + .any(|other| meta.shares_worktree_with(other)); + if shared { + log::info!( + "keeping worktree {} in use by another thread", + meta.cwd.display() + ); + return None; + } + Some((worktree.root_project_path.clone(), meta.cwd.clone())) }); self.settings.last_visited.remove(session_id); self.settings @@ -1110,16 +1120,24 @@ impl AppState { .is_some_and(|&visited| meta.updated_at > visited) } - /// Remove app-owned worktrees whose session is absent from the loaded store. + /// Remove app-owned worktrees that no session in the loaded store owns or + /// works in. pub(crate) fn recover_orphaned_worktrees(&self, cx: &mut HostCx) { let known_ids = self .sessions .iter() .map(|session| session.id.clone()) .collect(); + let cwds: Vec<_> = self + .sessions + .iter() + .map(|session| session.cwd.clone()) + .collect(); let host_cx = cx.clone(); HostCx::spawn_detached(cx, async move { - let summary = host_cx.unblock(move || cleanup_orphans(&known_ids)).await; + let summary = host_cx + .unblock(move || cleanup_orphans(&known_ids, &cwds)) + .await; if !summary.removed.is_empty() || !summary.skipped.is_empty() { log::info!( "worktree orphan recovery removed {}, left {}", diff --git a/crates/services/src/worktree.rs b/crates/services/src/worktree.rs index e7d62e7e..2722c68e 100644 --- a/crates/services/src/worktree.rs +++ b/crates/services/src/worktree.rs @@ -398,15 +398,20 @@ fn worktrees_root() -> PathBuf { }) } -/// Remove old app-owned worktrees whose directory names are not known session ids. +/// Remove old, clean app-owned worktrees that no known session owns or works in. /// /// Tests and isolated processes may set `TCODE_WORKTREES_DIR`; production falls /// back to `~/.tcode/worktrees`. Fresh unknown entries are presumed live and -/// preserved for at least [`ORPHAN_MIN_AGE`]. -pub fn cleanup_orphans(known_session_ids: &HashSet) -> CleanupSummary { +/// preserved for at least [`ORPHAN_MIN_AGE`]. Removal is never forced: a +/// worktree the user kept on delete may hold uncommitted work. +pub fn cleanup_orphans( + known_session_ids: &HashSet, + session_cwds: &[PathBuf], +) -> CleanupSummary { cleanup_orphans_at( &worktrees_root(), known_session_ids, + session_cwds, SystemTime::now(), ORPHAN_MIN_AGE, ) @@ -415,6 +420,7 @@ pub fn cleanup_orphans(known_session_ids: &HashSet) -> CleanupSummary { fn cleanup_orphans_at( worktrees: &Path, known_session_ids: &HashSet, + session_cwds: &[PathBuf], now: SystemTime, minimum_age: Duration, ) -> CleanupSummary { @@ -436,7 +442,10 @@ fn cleanup_orphans_at( let is_directory = entry .file_type() .is_ok_and(|kind| kind.is_dir() && !kind.is_symlink()); - if known_session_ids.contains(&session_id) || !is_directory { + if known_session_ids.contains(&session_id) + || session_cwds.iter().any(|cwd| cwd.starts_with(&path)) + || !is_directory + { continue; } let modified = match entry.metadata().and_then(|metadata| metadata.modified()) { @@ -481,12 +490,7 @@ fn cleanup_orphans_at( }; let output = crate::process::command("git") .current_dir(&removal_root) - .args([ - "worktree", - "remove", - "--force", - ®istered.to_string_lossy(), - ]) + .args(["worktree", "remove", ®istered.to_string_lossy()]) .output(); match output { Ok(output) if output.status.success() => { @@ -994,34 +998,48 @@ mod tests { let orphan = provision_for_test(&root, "orphan", &worktrees).path; let modified = std::fs::metadata(&orphan).unwrap().modified().unwrap(); let kept = provision_for_test(&root, "kept", &worktrees).path; + // A deleted source whose fork still runs in its worktree. + let forked = provision_for_test(&root, "deleted-source", &worktrees).path; + // Deleted with "Keep worktree" while holding uncommitted work. + let dirty = provision_for_test(&root, "kept-on-delete", &worktrees).path; + std::fs::write(dirty.join("notes.txt"), "uncommitted\n").unwrap(); let known = HashSet::from(["kept".to_string()]); + let cwds = [forked.clone()]; - let fresh = cleanup_orphans_at(&worktrees, &known, modified, ORPHAN_MIN_AGE); + let fresh = cleanup_orphans_at(&worktrees, &known, &cwds, modified, ORPHAN_MIN_AGE); assert!(fresh.removed.is_empty()); - assert_eq!(fresh.skipped.as_slice(), std::slice::from_ref(&orphan)); + let mut skipped = fresh.skipped; + skipped.sort(); + assert_eq!(skipped, [dirty.clone(), orphan.clone()]); assert!(orphan.exists()); let old = cleanup_orphans_at( &worktrees, &known, + &cwds, modified + ORPHAN_MIN_AGE + Duration::from_secs(1), ORPHAN_MIN_AGE, ); assert_eq!(old.removed.as_slice(), std::slice::from_ref(&orphan)); - assert!(old.skipped.is_empty()); + assert_eq!(old.skipped.as_slice(), std::slice::from_ref(&dirty)); assert!(!orphan.exists()); assert!(kept.join("tracked.txt").exists()); + assert!(forked.join("tracked.txt").exists()); + assert!(dirty.join("notes.txt").exists()); let unrelated = worktrees.join("unregistered"); std::fs::create_dir(&unrelated).unwrap(); let modified = std::fs::metadata(&unrelated).unwrap().modified().unwrap(); let summary = cleanup_orphans_at( &worktrees, &known, + &cwds, modified + ORPHAN_MIN_AGE + Duration::from_secs(1), ORPHAN_MIN_AGE, ); assert!(summary.removed.is_empty()); - assert_eq!(summary.skipped.as_slice(), std::slice::from_ref(&unrelated)); + let mut skipped = summary.skipped; + skipped.sort(); + assert_eq!(skipped, [dirty, unrelated.clone()]); assert!(unrelated.exists()); let _ = std::fs::remove_dir_all(temp); } diff --git a/crates/ui/src/store/mod.rs b/crates/ui/src/store/mod.rs index 31bb9e63..1833de3a 100644 --- a/crates/ui/src/store/mod.rs +++ b/crates/ui/src/store/mod.rs @@ -2824,16 +2824,15 @@ impl WorkspaceStore { .iter() .find(|meta| meta.id == session_id)?; let worktree = meta.worktree.clone()?; - let shared = self.index_replica.0.iter().any(|meta| { - meta.id != session_id - && meta - .worktree - .as_ref() - .is_some_and(|other| other.branch == worktree.branch) - }) || self - .index_summary - .archived_worktree_branches - .contains(&worktree.branch); + let shared = self + .index_replica + .0 + .iter() + .any(|other| meta.shares_worktree_with(other)) + || self + .index_summary + .archived_worktree_branches + .contains(&worktree.branch); (!shared).then_some(worktree) } }