From d5b29951aca5205bc8415120b56dd4bc295f5c83 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Fri, 18 Sep 2026 20:05:59 +0000 Subject: [PATCH] Centralize retry decisions and delays in `CodexErr` (#46540) ## What changed Replace `is_retryable()` with `retry_delay(retry_count)`, which returns `None` for terminal errors and a delay for retryable errors. Prefer server advice when present; otherwise use exponential backoff with jitter. Keep `server_retry_delay()` for callers that need only the server-provided delay. Route sampling and remote compaction errors through the shared response stream handler, which now checks retryability and obtains the delay together. Preserve server-only retry timing when the retry budget is exhausted. ## Testing Add coverage for attempt-dependent backoff, zero-duration server advice, and terminal errors that remain non-retryable despite server advice. Update API mapping and provider tests to use the combined retry API. GitOrigin-RevId: 903f92935e71dea043afe1fad8ccb9389471aa75 --- codex-rs/codex-api/src/api_bridge_tests.rs | 21 +++++++----- codex-rs/codex-client/src/retry.rs | 1 + codex-rs/core/src/client.rs | 2 +- codex-rs/core/src/compact_remote_v2.rs | 5 ++- codex-rs/core/src/responses_retry.rs | 18 +++++----- codex-rs/core/src/session/turn.rs | 8 ++--- .../amazon_bedrock/credential_export_tests.rs | 7 ++-- .../src/amazon_bedrock/error.rs | 2 +- .../src/amazon_bedrock/error_tests.rs | 8 ++++- codex-rs/model-provider/src/provider.rs | 2 +- codex-rs/protocol/src/error.rs | 31 +++++++++++------ codex-rs/protocol/src/error_tests.rs | 34 ++++++++++++++++++- 12 files changed, 94 insertions(+), 45 deletions(-) diff --git a/codex-rs/codex-api/src/api_bridge_tests.rs b/codex-rs/codex-api/src/api_bridge_tests.rs index 882c1fdf6c..595232d2d9 100644 --- a/codex-rs/codex-api/src/api_bridge_tests.rs +++ b/codex-rs/codex-api/src/api_bridge_tests.rs @@ -35,15 +35,15 @@ fn map_api_error_preserves_retry_delay() { assert_eq!( ( err.to_codex_protocol_error(), - err.retry_delay(), - err.is_retryable(), + err.retry_delay(/*retry_count*/ 1), + err.server_retry_delay(), err.http_status_code_value(), err.to_string(), ), ( expected_code, Some(retry_delay), - true, + Some(retry_delay), None, expected_message.to_string(), ) @@ -71,7 +71,10 @@ fn map_api_error_distinguishes_capacity_from_slow_down() { ), })); assert_eq!( - (err.to_codex_protocol_error(), err.is_retryable()), + ( + err.to_codex_protocol_error(), + err.retry_delay(/*retry_count*/ 1).is_some() + ), (expected, retryable) ); } @@ -188,7 +191,7 @@ fn map_api_error_preserves_bio_policy() { }); assert_eq!(err.to_codex_protocol_error(), CodexErrorInfo::BioPolicy); assert_eq!(err.to_string(), "This request was blocked by bio policy."); - assert!(!err.is_retryable()); + assert_eq!(err.retry_delay(/*retry_count*/ 1), None); } #[test] @@ -223,7 +226,7 @@ fn map_api_error_maps_http_and_wrapped_websocket_bio_policy() { }; assert_eq!(message, expected); assert_eq!(err.to_codex_protocol_error(), CodexErrorInfo::BioPolicy); - assert!(!err.is_retryable()); + assert_eq!(err.retry_delay(/*retry_count*/ 1), None); } } } @@ -263,7 +266,7 @@ fn assert_misalignment_policy_violation_from_http_body(status: http::StatusCode) }; assert_eq!(message, "This request violated the misalignment policy."); assert_eq!(misalignment, &None); - assert!(!err.is_retryable()); + assert_eq!(err.retry_delay(/*retry_count*/ 1), None); } #[test] @@ -305,7 +308,7 @@ fn map_api_error_preserves_misalignment_details_from_403_body() { }), }) ); - assert!(!err.is_retryable()); + assert_eq!(err.retry_delay(/*retry_count*/ 1), None); } #[test] @@ -352,7 +355,7 @@ fn map_api_error_preserves_misalignment_details_from_wrapped_websocket_error() { }), }) ); - assert!(!err.is_retryable()); + assert_eq!(err.retry_delay(/*retry_count*/ 1), None); } #[test] diff --git a/codex-rs/codex-client/src/retry.rs b/codex-rs/codex-client/src/retry.rs index 86071851f6..fd6bf007d8 100644 --- a/codex-rs/codex-client/src/retry.rs +++ b/codex-rs/codex-client/src/retry.rs @@ -98,6 +98,7 @@ where .should_retry(&err, attempt, policy.max_attempts) => { let retry_attempt = attempt + 1; + // TODO(anp): Respect Retry-After from HTTP responses before retrying the request. let delay = backoff(policy.base_delay, retry_attempt); crate::record_retry!(retry_attempt, delay, RetryOperation::HttpRequest); tokio::time::sleep(delay).await; diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 922e57c4f7..1c5b49353a 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -2577,7 +2577,7 @@ async fn handle_unauthorized( original_error = %original, "provider authentication recovery failed" ); - return Err(if error.is_retryable() { + return Err(if error.retry_delay(/*retry_count*/ 1).is_some() { original } else { error diff --git a/codex-rs/core/src/compact_remote_v2.rs b/codex-rs/core/src/compact_remote_v2.rs index 680deca87c..e0dd97b999 100644 --- a/codex-rs/core/src/compact_remote_v2.rs +++ b/codex-rs/core/src/compact_remote_v2.rs @@ -24,7 +24,7 @@ use crate::responses_metadata::CodexResponsesMetadata; use crate::responses_metadata::CompactionTurnMetadata; use crate::responses_retry::ResponsesStreamRequest; use crate::responses_retry::ResponsesStreamRetryState; -use crate::responses_retry::handle_retryable_response_stream_error; +use crate::responses_retry::handle_response_stream_error; use crate::session::RequestEffortUsage; use crate::session::session::Session; use crate::session::step_context::StepContext; @@ -417,9 +417,8 @@ async fn run_remote_compaction_request_v2( match result { Ok(compaction_output) => return Ok(compaction_output), - Err(err) if !err.is_retryable() => return Err(err), Err(err) => { - handle_retryable_response_stream_error( + handle_response_stream_error( &mut retry_state, max_retries, err, diff --git a/codex-rs/core/src/responses_retry.rs b/codex-rs/core/src/responses_retry.rs index 462bb95ba8..03acba415c 100644 --- a/codex-rs/core/src/responses_retry.rs +++ b/codex-rs/core/src/responses_retry.rs @@ -5,7 +5,6 @@ use std::time::Duration; use crate::client::ModelClientSession; use crate::session::session::Session; use crate::session::turn_context::TurnContext; -use crate::util::backoff; use codex_client::RetryOperation; use codex_features::Feature; use codex_protocol::error::CodexErr; @@ -46,9 +45,9 @@ pub(crate) struct ExhaustedResponseRetry { pub(crate) retry_at: Option, } -/// Handles a retryable stream error and returns `Ok(())` when the caller should -/// retry the request loop. -pub(crate) async fn handle_retryable_response_stream_error( +/// Returns `Ok(())` when the caller should retry the request loop, or the original error when +/// it is terminal or the retry budget is exhausted. +pub(crate) async fn handle_response_stream_error( retry_state: &mut ResponsesStreamRetryState, max_retries: u64, err: CodexErr, @@ -61,6 +60,10 @@ pub(crate) async fn handle_retryable_response_stream_error( ResponsesStreamRequest::Sampling => RetryOperation::Sampling, ResponsesStreamRequest::RemoteCompactionV2 => RetryOperation::RemoteCompactionV2, }; + let retry_count = retry_state.retries.saturating_add(1); + let Some(delay) = err.retry_delay(retry_count) else { + return Err(err); + }; if turn_context .config @@ -89,6 +92,7 @@ pub(crate) async fn handle_retryable_response_stream_error( return Ok(()); } + // TODO(anp): Respect server retry advice before issuing the fallback HTTP request. if retry_state.retries >= max_retries && client_session.try_switch_fallback_transport( &turn_context.session_telemetry, @@ -107,9 +111,7 @@ pub(crate) async fn handle_retryable_response_stream_error( } if retry_state.retries < max_retries { - retry_state.retries += 1; - let retry_count = retry_state.retries; - let delay = err.retry_delay().unwrap_or_else(|| backoff(retry_count)); + retry_state.retries = retry_count; log_retry(request, turn_context, &err, retry_count, max_retries, delay); // In release builds, hide the first websocket retry notification to reduce noisy @@ -137,7 +139,7 @@ pub(crate) async fn handle_retryable_response_stream_error( .insert(ExhaustedResponseRetry { turn_id: turn_context.sub_id.clone(), retry_at: err - .retry_delay() + .server_retry_delay() .and_then(|delay| tokio::time::Instant::now().checked_add(delay)), }); Err(err) diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 7b624d8509..3f396ef5db 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -32,7 +32,7 @@ use crate::responses_metadata::CodexResponsesMetadata; use crate::responses_metadata::CodexResponsesRequestKind; use crate::responses_retry::ResponsesStreamRequest; use crate::responses_retry::ResponsesStreamRetryState; -use crate::responses_retry::handle_retryable_response_stream_error; +use crate::responses_retry::handle_response_stream_error; use crate::session::PreviousTurnSettings; use crate::session::TurnInput; use crate::session::daemon_recovery::RecordedTurnInput; @@ -1644,11 +1644,7 @@ async fn run_sampling_request( original_input = Some(prompt.input); } - if !err.is_retryable() { - return Err(err); - } - - handle_retryable_response_stream_error( + handle_response_stream_error( &mut retry_state, max_retries, err, diff --git a/codex-rs/model-provider/src/amazon_bedrock/credential_export_tests.rs b/codex-rs/model-provider/src/amazon_bedrock/credential_export_tests.rs index b027757a61..fa5db51a8e 100644 --- a/codex-rs/model-provider/src/amazon_bedrock/credential_export_tests.rs +++ b/codex-rs/model-provider/src/amazon_bedrock/credential_export_tests.rs @@ -324,10 +324,11 @@ async fn credential_export_reuses_cache_and_coalesces_refreshes() -> io::Result< } let (credentials, refresh) = tokio::join!(credentials, refresh); assert_eq!(credentials?, rotated); - assert!( - !refresh + assert_eq!( + refresh .expect_err("queued login recovery must still run") - .is_retryable() + .retry_delay(/*retry_count*/ 1), + None ); assert!(exporter.cached.lock().await.is_none()); diff --git a/codex-rs/model-provider/src/amazon_bedrock/error.rs b/codex-rs/model-provider/src/amazon_bedrock/error.rs index dc71b47979..5274029daa 100644 --- a/codex-rs/model-provider/src/amazon_bedrock/error.rs +++ b/codex-rs/model-provider/src/amazon_bedrock/error.rs @@ -27,7 +27,7 @@ pub(super) fn map_api_error(error: ApiError) -> CodexErr { let mut response = response.clone(); response.user_message = Some(BEDROCK_EXPIRED_SIGNATURE_MESSAGE.to_string()); let mapped_error = CodexErr::new(CodexErrorDetails::UnexpectedStatus(response)); - return match error.retry_delay() { + return match error.server_retry_delay() { Some(retry_delay) => mapped_error.with_retry_delay(retry_delay), None => mapped_error, }; diff --git a/codex-rs/model-provider/src/amazon_bedrock/error_tests.rs b/codex-rs/model-provider/src/amazon_bedrock/error_tests.rs index 7ecfeda5a5..e08d61f811 100644 --- a/codex-rs/model-provider/src/amazon_bedrock/error_tests.rs +++ b/codex-rs/model-provider/src/amazon_bedrock/error_tests.rs @@ -208,6 +208,12 @@ fn credential_export_errors_distinguish_configuration_failures_from_retryable_au unreachable!("all test cases are transport errors"); }; assert_eq!(provider.is_recoverable_auth_error(transport), recoverable); - assert_eq!(provider.map_api_error(error).is_retryable(), retryable); + assert_eq!( + provider + .map_api_error(error) + .retry_delay(/*retry_count*/ 1) + .is_some(), + retryable + ); } } diff --git a/codex-rs/model-provider/src/provider.rs b/codex-rs/model-provider/src/provider.rs index 691d75bfba..5f7bd4b035 100644 --- a/codex-rs/model-provider/src/provider.rs +++ b/codex-rs/model-provider/src/provider.rs @@ -993,7 +993,7 @@ mod tests { .recover_from_unauthorized() .await .expect_err("non-aws command should be rejected"); - assert!(!error.is_retryable()); + assert_eq!(error.retry_delay(/*retry_count*/ 1), None); assert_eq!(error.to_string(), "AWS auth refresh command must be `aws`"); let (first_result, second_result) = tokio::join!( diff --git a/codex-rs/protocol/src/error.rs b/codex-rs/protocol/src/error.rs index b62943646d..609cdcab95 100644 --- a/codex-rs/protocol/src/error.rs +++ b/codex-rs/protocol/src/error.rs @@ -16,6 +16,7 @@ use chrono::Datelike; use chrono::Local; use chrono::Utc; use codex_async_utils::CancelErr; +use codex_async_utils::backoff; use codex_http_client::HttpError; use codex_utils_string::truncate_middle_chars; use codex_utils_string::truncate_middle_with_token_budget; @@ -70,7 +71,7 @@ pub enum SandboxErr { pub struct CodexErr { details: CodexErrorDetails, - retry_delay: Option, + server_retry_delay: Option, } /// The semantic category and diagnostic payload for a [`CodexErr`]. @@ -204,7 +205,7 @@ impl fmt::Debug for CodexErr { CodexErrorDetails::Stream(message) => formatter .debug_tuple("Stream") .field(message) - .field(&self.retry_delay) + .field(&self.server_retry_delay) .finish(), details => fmt::Debug::fmt(details, formatter), } @@ -227,7 +228,7 @@ impl From for CodexErr { fn from(details: CodexErrorDetails) -> Self { Self { details, - retry_delay: None, + server_retry_delay: None, } } } @@ -291,7 +292,7 @@ macro_rules! codex_err_unit_constructors { #[allow(non_upper_case_globals)] pub const $variant: Self = Self { details: CodexErrorDetails::$variant, - retry_delay: None, + server_retry_delay: None, }; )* }; @@ -371,7 +372,11 @@ impl CodexErr { &self.details } - pub fn is_retryable(&self) -> bool { + /// Returns the delay before the given retry attempt, or `None` for a terminal error. + /// + /// The first retry is attempt one. Retryable errors use server advice when available and + /// otherwise use exponential backoff with jitter. Callers enforce their own retry budgets. + pub fn retry_delay(&self, retry_count: u64) -> Option { match self.details() { CodexErrorDetails::TurnAborted | CodexErrorDetails::SessionBudgetExceeded @@ -397,7 +402,7 @@ impl CodexErr { | CodexErrorDetails::ServerOverloaded | CodexErrorDetails::CyberPolicy { .. } | CodexErrorDetails::BioPolicy { .. } - | CodexErrorDetails::MisalignmentPolicyViolation { .. } => false, + | CodexErrorDetails::MisalignmentPolicyViolation { .. } => None, CodexErrorDetails::Stream(..) | CodexErrorDetails::RateLimitExceeded(_) | CodexErrorDetails::Timeout @@ -409,18 +414,22 @@ impl CodexErr { | CodexErrorDetails::InternalAgentDied | CodexErrorDetails::Io(_) | CodexErrorDetails::Json(_) - | CodexErrorDetails::TokioJoin(_) => true, + | CodexErrorDetails::TokioJoin(_) => Some( + self.server_retry_delay + .unwrap_or_else(|| backoff(retry_count)), + ), #[cfg(target_os = "linux")] - CodexErrorDetails::LandlockRuleset(_) | CodexErrorDetails::LandlockPathFd(_) => false, + CodexErrorDetails::LandlockRuleset(_) | CodexErrorDetails::LandlockPathFd(_) => None, } } - pub fn retry_delay(&self) -> Option { - self.retry_delay + /// Returns only the delay advised by the server, without applying local retry policy. + pub fn server_retry_delay(&self) -> Option { + self.server_retry_delay } pub fn with_retry_delay(mut self, retry_delay: Duration) -> Self { - self.retry_delay = Some(retry_delay); + self.server_retry_delay = Some(retry_delay); self } diff --git a/codex-rs/protocol/src/error_tests.rs b/codex-rs/protocol/src/error_tests.rs index 5357867ef0..921fb79ce7 100644 --- a/codex-rs/protocol/src/error_tests.rs +++ b/codex-rs/protocol/src/error_tests.rs @@ -69,13 +69,45 @@ fn retryability_preserves_error_details_distinctions() { for (err, expected) in errors { assert_eq!( - err.is_retryable(), + err.retry_delay(/*retry_count*/ 1).is_some(), expected, "unexpected retryability for {err:?}" ); } } +/// Retryable errors prefer server advice and otherwise back off by attempt; advice does not +/// make a terminal error retryable. +#[test] +fn retry_delay_distinguishes_server_advice_backoff_and_terminal_errors() { + let error = CodexErr::InternalServerError; + for (retry_count, expected_millis) in [(1, 180..220), (3, 720..880)] { + let delay = error.retry_delay(retry_count).expect("retryable error"); + assert!(expected_millis.contains(&delay.as_millis())); + } + assert_eq!(error.server_retry_delay(), None); + + let advice = Duration::ZERO; + let error = error.with_retry_delay(advice); + assert_eq!( + ( + error.retry_delay(/*retry_count*/ 1), + error.retry_delay(/*retry_count*/ 3), + error.server_retry_delay(), + ), + (Some(advice), Some(advice), Some(advice)), + ); + + let error = CodexErr::QuotaExceeded.with_retry_delay(advice); + assert_eq!( + ( + error.retry_delay(/*retry_count*/ 1), + error.server_retry_delay(), + ), + (None, Some(advice)), + ); +} + fn rate_limit_snapshot() -> RateLimitSnapshot { let primary_reset_at = Utc .with_ymd_and_hms(2024, 1, 1, 1, 0, 0)