From c2e4631a34cfcc89828e71952a7a0ffeb36858ac Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 30 Oct 2026 03:50:12 +0000 Subject: [PATCH] Preserve unrelated side conversations when archiving or deleting threads (#58237) ## Why Archiving or deleting a thread in the command center shuts down side conversations belonging to other threads. ## What changed Limit side conversation shutdown to those whose parents belong to the affected thread's known subtree. Proceed once those conversations close successfully, even if unrelated side conversations remain. Continue shutting down all side conversations on exit. ## Testing Extend the delete lifecycle test to verify that an unrelated side conversation remains open after deletion. GitOrigin-RevId: f27efcd1fc49cf5c617c9fa454d83eed91ae185f --- .../tui/src/app/agents_overview_actions.rs | 57 ++++++++++--------- codex-rs/tui/src/app/event_dispatch.rs | 24 ++++---- .../app/tests/session_lifecycle_requests.rs | 8 ++- codex-rs/tui/src/app/thread_routing.rs | 27 +++++++-- 4 files changed, 74 insertions(+), 42 deletions(-) diff --git a/codex-rs/tui/src/app/agents_overview_actions.rs b/codex-rs/tui/src/app/agents_overview_actions.rs index 782e78bba4..38492f6026 100644 --- a/codex-rs/tui/src/app/agents_overview_actions.rs +++ b/codex-rs/tui/src/app/agents_overview_actions.rs @@ -237,6 +237,36 @@ impl App { }); } + pub(super) fn known_thread_subtree(&self, thread_id: ThreadId) -> HashSet { + let mut removed = HashSet::from([thread_id]); + loop { + let previous_len = removed.len(); + for (id, thread) in &self.agents_overview.threads { + let Some(thread) = thread else { continue }; + let parent = thread + .parent_thread_id + .as_deref() + .and_then(|id| ThreadId::from_string(id).ok()) + .or(match &thread.source { + SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id, + .. + }) => Some(*parent_thread_id), + _ => None, + }); + if thread.session_id == thread_id.to_string() + || parent.is_some_and(|parent| removed.contains(&parent)) + { + removed.insert(*id); + } + } + if removed.len() == previous_len { + break; + } + } + removed + } + pub(super) async fn run_agents_overview_action( &mut self, tui: &mut tui::Tui, @@ -339,32 +369,7 @@ impl App { } if result.is_ok() { // Invalidate cached details for the displayed group, without inferring child outcomes. - let mut removed = HashSet::from([thread_id]); - loop { - let previous_len = removed.len(); - for (id, thread) in &self.agents_overview.threads { - let Some(thread) = thread else { continue }; - let parent = thread - .parent_thread_id - .as_deref() - .and_then(|id| ThreadId::from_string(id).ok()) - .or(match &thread.source { - SessionSource::SubAgent(SubAgentSource::ThreadSpawn { - parent_thread_id, - .. - }) => Some(*parent_thread_id), - _ => None, - }); - if thread.session_id == thread_id.to_string() - || parent.is_some_and(|parent| removed.contains(&parent)) - { - removed.insert(*id); - } - } - if removed.len() == previous_len { - break; - } - } + let mut removed = self.known_thread_subtree(thread_id); if let Some(primary) = self.primary_thread_id && removes_primary { diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 406c2f0316..3fa49e6ff2 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -3859,7 +3859,7 @@ impl App { if !self.side_threads.is_empty() && tokio::time::timeout( SHUTDOWN_FIRST_EXIT_TIMEOUT, - self.shutdown_side_threads(app_server), + self.shutdown_side_threads(app_server, /*root*/ None), ) .await .is_err() @@ -3917,11 +3917,12 @@ impl App { return Ok(AppRunControl::Continue); } - if self.app_server_target.supports_command_center() { - self.shutdown_side_threads(app_server).await; - if !self.side_threads.is_empty() { - return Ok(AppRunControl::Continue); - } + if self.app_server_target.supports_command_center() + && !self + .shutdown_side_threads(app_server, Some(thread_id)) + .await + { + return Ok(AppRunControl::Continue); } let result = async { @@ -3985,11 +3986,12 @@ impl App { return Ok(AppRunControl::Continue); } - if self.app_server_target.supports_command_center() { - self.shutdown_side_threads(app_server).await; - if !self.side_threads.is_empty() { - return Ok(AppRunControl::Continue); - } + if self.app_server_target.supports_command_center() + && !self + .shutdown_side_threads(app_server, Some(thread_id)) + .await + { + return Ok(AppRunControl::Continue); } let result = async { diff --git a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs index c63fc9a293..efe15c722f 100644 --- a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs +++ b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs @@ -1089,6 +1089,9 @@ async fn delete_current_thread_navigates_only_after_success() -> Result<()> { let side_id = side.session.thread_id; app.side_threads .insert(side_id, SideThreadState::new(thread_id)); + let unrelated_side = ThreadId::new(); + app.side_threads + .insert(unrelated_side, SideThreadState::new(ThreadId::new())); if !matches!(app.app_server_target, AppServerTarget::Embedded) { // The recording proxy rejects the next unsubscribe after a failed fork. side_config.cwd = side_config.cwd.join("failure"); @@ -1128,7 +1131,10 @@ async fn delete_current_thread_navigates_only_after_success() -> Result<()> { assert!(recorded_params(&requests, "thread/unsubscribe").is_empty()); } else { assert_matches!(control, AppRunControl::Continue); - assert!(app.side_threads.is_empty()); + assert_eq!( + app.side_threads.keys().copied().collect::>(), + vec![unrelated_side] + ); assert_eq!( recorded_params(&requests, "thread/unsubscribe"), vec![serde_json::json!({"threadId": side_id.to_string()}); 2] diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index bf86ffecd8..d488964e61 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -49,11 +49,30 @@ impl App { .await; } - pub(super) async fn shutdown_side_threads(&mut self, app_server: &mut AppServerSession) { - let side_thread_ids: Vec = self.side_threads.keys().copied().collect(); - for side_thread_id in side_thread_ids { - self.discard_side_thread(app_server, side_thread_id).await; + pub(super) async fn shutdown_side_threads( + &mut self, + app_server: &mut AppServerSession, + root: Option, + ) -> bool { + if self.side_threads.is_empty() { + return true; } + let subtree = root.map(|id| self.known_thread_subtree(id)); + let ids: Vec<_> = self + .side_threads + .iter() + .filter(|(_, side)| { + subtree + .as_ref() + .is_none_or(|ids| ids.contains(&side.parent_thread_id)) + }) + .map(|(id, _)| *id) + .collect(); + let mut closed = true; + for id in ids { + closed &= self.discard_side_thread(app_server, id).await; + } + closed } pub(super) fn abort_thread_event_listener(&mut self, thread_id: ThreadId) {