From fbf5bfd1a930932d4b22d04efba45c67f835d298 Mon Sep 17 00:00:00 2001 From: Anton Panasenko Date: Mon, 6 Jul 2026 15:19:26 -0700 Subject: [PATCH] clarify terminal persistence cleanup ownership --- codex-rs/core/src/session/handlers.rs | 7 ++++--- codex-rs/thread-store/src/live_thread.rs | 6 +++++- codex-rs/thread-store/src/local/live_writer.rs | 5 +++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/session/handlers.rs b/codex-rs/core/src/session/handlers.rs index 4c7400ccb1..a2200f6051 100644 --- a/codex-rs/core/src/session/handlers.rs +++ b/codex-rs/core/src/session/handlers.rs @@ -630,9 +630,10 @@ async fn shutdown_thread_persistence( return ThreadPersistenceShutdown::Closed; }; let failure_context = failure_context.to_owned(); - // Terminal cleanup must outlive the submission loop. If the loop is canceled while a - // graceful shutdown is still waiting on I/O, the same task still reaches the discard - // fallback and releases the live-writer lease. + // Await a child task so ordinary shutdown still waits for cleanup before completion. If the + // submission loop is canceled while graceful shutdown is waiting on I/O, dropping the + // JoinHandle detaches the child; it still reaches the discard fallback and releases the + // live-writer lease. tokio::spawn(async move { if let Err(err) = live_thread.shutdown().await { warn!("{failure_context}: {err}"); diff --git a/codex-rs/thread-store/src/live_thread.rs b/codex-rs/thread-store/src/live_thread.rs index 793bbf9965..60a1c2f575 100644 --- a/codex-rs/thread-store/src/live_thread.rs +++ b/codex-rs/thread-store/src/live_thread.rs @@ -60,12 +60,16 @@ impl LiveThreadInitGuard { } pub async fn discard(&mut self) { - let Some(live_thread) = self.live_thread.take() else { + let Some(live_thread) = self.live_thread.as_ref().cloned() else { return; }; if let Err(err) = live_thread.discard().await { warn!("failed to discard thread persistence for failed session init: {err}"); + return; } + // Keep the guard armed until discard completes so cancellation still leaves Drop able to + // retry terminal cleanup. + self.live_thread = None; } } diff --git a/codex-rs/thread-store/src/local/live_writer.rs b/codex-rs/thread-store/src/local/live_writer.rs index 1b91b83ac5..df9ad2df7c 100644 --- a/codex-rs/thread-store/src/local/live_writer.rs +++ b/codex-rs/thread-store/src/local/live_writer.rs @@ -162,6 +162,9 @@ pub(super) async fn shutdown_thread( ) -> ThreadStoreResult<()> { let recorder = store.live_recorder(thread_id).await?; let store = store.clone(); + // The writer can close before this future removes its live-recorder entry. Keep those steps + // in one detached task so canceling an arbitrary ThreadStore caller cannot strand a closed + // recorder that blocks a later resume. tokio::spawn(async move { let rollout_path = recorder.rollout_path().to_path_buf(); recorder.shutdown().await.map_err(thread_store_io_error)?; @@ -190,6 +193,8 @@ pub(super) async fn discard_thread( Err(ThreadStoreError::ThreadNotFound { .. }) => return Ok(()), Err(err) => return Err(err), }; + // Discard closes stale recorder clones before removing the lease. Keep that sequence alive if + // an arbitrary ThreadStore caller is canceled between the two steps. spawn_discard_cleanup(store.clone(), thread_id, recorder) .await .map_err(|err| ThreadStoreError::Internal {