diff --git a/codex-rs/app-server/src/request_processors/thread_attachments.rs b/codex-rs/app-server/src/request_processors/thread_attachments.rs index 2cdaaeca65..40d8850897 100644 --- a/codex-rs/app-server/src/request_processors/thread_attachments.rs +++ b/codex-rs/app-server/src/request_processors/thread_attachments.rs @@ -284,7 +284,10 @@ fn thread_attachment_store_error( ThreadStoreError::ThreadNotFound { thread_id } => { invalid_params(format!("thread not found: {thread_id}")) } - error @ (ThreadStoreError::Conflict { .. } | ThreadStoreError::Internal { .. }) => { + error @ (ThreadStoreError::Conflict { .. } + | ThreadStoreError::Retryable { .. } + | ThreadStoreError::OutcomeUnknown { .. } + | ThreadStoreError::Internal { .. }) => { internal_error(format!("failed to process {operation}: {error}")) } } diff --git a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs index f1bd1bfe92..a42e06dc9d 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs @@ -1,3 +1,44 @@ +use codex_app_server_protocol::JSONRPCErrorError; +use codex_thread_store::ThreadStoreError; +use pretty_assertions::assert_eq; + +#[test] +fn storage_failure_classification_does_not_change_mutation_error_policy() { + for error in [ + ThreadStoreError::Internal { + message: "retryable conflict; outcome unknown".to_owned(), + }, + ThreadStoreError::Retryable { + message: "store temporarily unavailable".to_owned(), + }, + ThreadStoreError::OutcomeUnknown { + message: "connection lost after write".to_owned(), + }, + ] { + let mapped = super::thread_store_mutation_error("archive", error.clone()); + let expected = JSONRPCErrorError { + code: -32603, + message: format!("failed to archive session: {error}"), + data: None, + }; + assert_eq!(mapped, expected); + } + + assert_eq!( + super::thread_store_mutation_error( + "archive", + ThreadStoreError::Conflict { + message: "writer already active".to_owned(), + }, + ), + JSONRPCErrorError { + code: -32600, + message: "writer already active".to_owned(), + data: None, + }, + ); +} + mod thread_list_cwd_filter_tests { use super::super::normalize_thread_list_cwd_filters; use codex_app_server_protocol::ThreadListCwdFilter; diff --git a/codex-rs/app-server/src/request_processors/thread_sections.rs b/codex-rs/app-server/src/request_processors/thread_sections.rs index 6491b3bc78..31f5b3f7e4 100644 --- a/codex-rs/app-server/src/request_processors/thread_sections.rs +++ b/codex-rs/app-server/src/request_processors/thread_sections.rs @@ -224,6 +224,8 @@ fn thread_section_store_error( ThreadStoreError::InvalidRequest { message } => invalid_params(message), error @ (ThreadStoreError::ThreadNotFound { .. } | ThreadStoreError::Conflict { .. } + | ThreadStoreError::Retryable { .. } + | ThreadStoreError::OutcomeUnknown { .. } | ThreadStoreError::Internal { .. }) => { let action = operation .strip_prefix("threadSection/") diff --git a/codex-rs/core/src/agent/control/runtime_tests.rs b/codex-rs/core/src/agent/control/runtime_tests.rs index 2884575ab3..2fc7e07108 100644 --- a/codex-rs/core/src/agent/control/runtime_tests.rs +++ b/codex-rs/core/src/agent/control/runtime_tests.rs @@ -91,13 +91,22 @@ async fn report_display_includes_safe_cause_without_raw_error_payload() { .expect("teardown should be admitted") .into_teardown_guard("session_startup", Some(thread_id)); let raw_payload = "sensitive thread-store error payload"; - let error = ThreadStoreError::Internal { - message: raw_payload.to_owned(), - }; - teardown.record_shutdown_failure( - "discard_persistence", - crate::thread_manager::thread_store_error_kind(&error), - ); + for error in [ + ThreadStoreError::Internal { + message: raw_payload.to_owned(), + }, + ThreadStoreError::Retryable { + message: raw_payload.to_owned(), + }, + ThreadStoreError::OutcomeUnknown { + message: raw_payload.to_owned(), + }, + ] { + teardown.record_shutdown_failure( + "discard_persistence", + crate::thread_manager::thread_store_error_kind(&error), + ); + } let shutdown = runtime.request_shutdown(); teardown.complete(); @@ -109,7 +118,7 @@ async fn report_display_includes_safe_cause_without_raw_error_payload() { assert_eq!( displayed, format!( - "agent tree shutdown did not complete cleanly: tree_id={}, failures=[{{operation=session_startup, phase=discard_persistence, thread_id={thread_id}, reason=operation_failed, error_kind=thread_store_internal}}]; omitted_failures=0", + "agent tree shutdown did not complete cleanly: tree_id={}, failures=[{{operation=session_startup, phase=discard_persistence, thread_id={thread_id}, reason=operation_failed, error_kind=thread_store_internal}}, {{operation=session_startup, phase=discard_persistence, thread_id={thread_id}, reason=operation_failed, error_kind=thread_store_retryable}}, {{operation=session_startup, phase=discard_persistence, thread_id={thread_id}, reason=operation_failed, error_kind=thread_store_outcome_unknown}}]; omitted_failures=0", report.tree_id, ) ); diff --git a/codex-rs/core/src/session_rollout_init_error.rs b/codex-rs/core/src/session_rollout_init_error.rs index 7e62462add..68581e217d 100644 --- a/codex-rs/core/src/session_rollout_init_error.rs +++ b/codex-rs/core/src/session_rollout_init_error.rs @@ -19,6 +19,8 @@ pub(crate) fn map_session_init_error(err: &anyhow::Error, codex_home: &Path) -> } ThreadStoreError::ThreadNotFound { .. } | ThreadStoreError::InvalidRequest { .. } + | ThreadStoreError::Retryable { .. } + | ThreadStoreError::OutcomeUnknown { .. } | ThreadStoreError::Internal { .. } => {} } } @@ -65,3 +67,7 @@ fn map_rollout_io_error(io_err: &std::io::Error, codex_home: &Path) -> Option &'static str ThreadStoreError::InvalidRequest { .. } => "thread_store_invalid_request", ThreadStoreError::Conflict { .. } => "thread_store_conflict", ThreadStoreError::Unsupported { .. } => "thread_store_unsupported", + ThreadStoreError::Retryable { .. } => "thread_store_retryable", + ThreadStoreError::OutcomeUnknown { .. } => "thread_store_outcome_unknown", ThreadStoreError::Internal { .. } => "thread_store_internal", } } diff --git a/codex-rs/thread-store/src/error.rs b/codex-rs/thread-store/src/error.rs index bc5e8ed7c4..d0ab6d8826 100644 --- a/codex-rs/thread-store/src/error.rs +++ b/codex-rs/thread-store/src/error.rs @@ -16,7 +16,11 @@ pub(crate) fn reject_paginated_history_mode( } /// Error type shared by thread-store implementations. -#[derive(Debug, thiserror::Error)] +/// +/// Implementations must explicitly distinguish known transient failures from writes whose +/// outcome is unknown. These categories carry information only: the operation owner decides +/// whether and how to recover, and no variant automatically triggers a retry. +#[derive(Clone, Debug, thiserror::Error)] pub enum ThreadStoreError { /// The requested thread does not exist in this store. #[error("thread {thread_id} not found")] @@ -32,7 +36,7 @@ pub enum ThreadStoreError { message: String, }, - /// The operation conflicted with current store state. + /// The operation conflicted with current store state. This alone does not imply retryability. #[error("thread-store conflict: {message}")] Conflict { /// User-facing explanation of the conflict. @@ -46,10 +50,37 @@ pub enum ThreadStoreError { operation: &'static str, }, + /// A transient failure for which retrying the operation is safe. + /// + /// For a write, the implementation must know the failed attempt did not commit. Otherwise + /// use [`Self::OutcomeUnknown`]. Retry scheduling and limits belong to the operation owner. + #[error("thread-store retryable error: {message}")] + Retryable { + /// User-facing explanation of the transient failure. + message: String, + }, + + /// A write failed to report a definitive outcome and may already have committed. + /// + /// The operation owner must reconcile the stored state or use an idempotent replay strategy + /// before retrying. This must not be treated as an ordinary retryable failure. + #[error("thread-store write outcome unknown: {message}")] + OutcomeUnknown { + /// User-facing explanation of why the write outcome is uncertain. + message: String, + }, + /// Catch-all for implementation failures that do not fit a more specific category. + /// + /// This includes unclassified failures: callers must not infer retryability or whether a + /// write committed from this variant or its message. #[error("thread-store internal error: {message}")] Internal { /// User-facing explanation of the implementation failure. message: String, }, } + +#[cfg(test)] +#[path = "error_tests.rs"] +mod tests; diff --git a/codex-rs/thread-store/src/error_tests.rs b/codex-rs/thread-store/src/error_tests.rs new file mode 100644 index 0000000000..4b56fa89bc --- /dev/null +++ b/codex-rs/thread-store/src/error_tests.rs @@ -0,0 +1,23 @@ +//! Verifies conservative classification at the shared storage error boundary. + +use super::ThreadStoreError; +use pretty_assertions::assert_eq; + +#[test] +fn unclassified_sqlx_failures_remain_internal() { + for error in [ + sqlx::Error::PoolTimedOut, + sqlx::Error::Io(std::io::Error::new( + std::io::ErrorKind::ConnectionReset, + "connection lost after write", + )), + sqlx::Error::Protocol("retryable conflict; outcome unknown".to_owned()), + ] { + let expected = format!("failed to access thread history: {error}"); + let error = ThreadStoreError::from(error); + let ThreadStoreError::Internal { message } = error else { + panic!("unclassified SQLx failure must remain internal: {error:?}"); + }; + assert_eq!(message, expected); + } +}