Skip to content

Commit 44dd77b

Browse files
anp-oaicopyberry
authored andcommitted
Honor Retry-After headers in failed Responses events (#50418)
## Why Streamed `response.failed` events can carry retry advice in `response.error.headers`. Ignoring it loses server-provided delays or uses a conflicting delay from a rate-limit message. ## What changed - Parse nested `Retry-After` headers using the existing HTTP header validation and retry parser, and preserve them for rate-limit, overload, and generic retryable failures. - Prefer valid headers over rate-limit message advice, falling back to the message when the header is absent or invalid. Keep terminal error classifications unchanged. - Reject unrepresentable plaintext retry durations without panicking. ## Testing Add regression coverage for header precedence, validation, and terminal errors, plus retry-loop coverage for SSE sampling, remote compaction, and WebSocket failures. GitOrigin-RevId: 46433c2e9e6d5e804211ef96950fadd70e322b49
1 parent dff5270 commit 44dd77b

4 files changed

Lines changed: 293 additions & 168 deletions

File tree

‎codex-rs/codex-api/src/sse/responses.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,10 @@ async fn process_sse_with_treatment(
649649
}
650650
}
651651

652+
#[cfg(test)]
653+
#[path = "responses_error_tests.rs"]
654+
mod error_tests;
655+
652656
#[cfg(test)]
653657
mod tests {
654658
use super::*;

‎codex-rs/codex-api/src/sse/responses_error.rs‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1-
//! Classifies failed Responses events and their rate-limit retry delays.
1+
//! Classifies failed Responses events and their retry advice.
22
//! Only rate-limit codes derive retry delays from plaintext messages.
33
44
use crate::error::ApiError;
55
use crate::error::parse_flex_unavailable;
6+
use crate::responses_headers::json_headers_to_http_headers;
67
use codex_http_client::RetryAfter;
78
use codex_protocol::protocol::MisalignmentErrorDetails;
89
use serde::Deserialize;
@@ -23,19 +24,24 @@ struct Error {
2324
}
2425

2526
pub(super) fn parse_failed_response(response: Option<Value>) -> ApiError {
26-
if let Some(error) = response
27-
.as_ref()
28-
.and_then(|response| response.get("error"))
29-
.and_then(parse_flex_unavailable)
30-
{
27+
let error = response.as_ref().and_then(|response| response.get("error"));
28+
let retry_after_header = error
29+
.and_then(|error| error.get("headers"))
30+
.and_then(Value::as_object)
31+
.and_then(|headers| RetryAfter::from_headers(&json_headers_to_http_headers(headers)));
32+
if let Some(error) = error.and_then(parse_flex_unavailable) {
3133
return error;
3234
}
33-
let Some(error) = response
34-
.as_ref()
35-
.and_then(|response| response.get("error"))
36-
.and_then(|error| serde_json::from_value::<Error>(error.clone()).ok())
35+
let Some(error) = error.and_then(|error| serde_json::from_value::<Error>(error.clone()).ok())
3736
else {
38-
return ApiError::Stream("response.failed event received".into());
37+
let message = "response.failed event received".to_string();
38+
return match retry_after_header {
39+
Some(_) => ApiError::Retryable {
40+
message,
41+
retry_after: retry_after_header,
42+
},
43+
None => ApiError::Stream(message),
44+
};
3945
};
4046

4147
match error.code.as_deref() {
@@ -78,17 +84,20 @@ pub(super) fn parse_failed_response(response: Option<Value>) -> ApiError {
7884
.message
7985
.unwrap_or_else(|| "Invalid request.".to_string()),
8086
},
81-
Some("server_is_overloaded") => ApiError::ServerOverloaded { retry_after: None },
87+
Some("server_is_overloaded") => ApiError::ServerOverloaded {
88+
retry_after: retry_after_header,
89+
},
8290
Some("rate_limit_exceeded" | "slow_down") => {
83-
let retry_after = try_parse_retry_delay(&error).and_then(RetryAfter::from_delay);
91+
let retry_after = retry_after_header
92+
.or_else(|| try_parse_retry_delay(&error).and_then(RetryAfter::from_delay));
8493
ApiError::RateLimitExceeded {
8594
message: error.message.unwrap_or_default(),
8695
retry_after,
8796
}
8897
}
8998
_ => ApiError::Retryable {
9099
message: error.message.unwrap_or_default(),
91-
retry_after: None,
100+
retry_after: retry_after_header,
92101
},
93102
}
94103
}
@@ -106,7 +115,7 @@ fn try_parse_retry_delay(err: &Error) -> Option<Duration> {
106115
let unit = unit.as_str().to_ascii_lowercase();
107116

108117
if unit == "s" || unit.starts_with("second") {
109-
return Some(Duration::from_secs_f64(value));
118+
return Duration::try_from_secs_f64(value).ok();
110119
} else if unit == "ms" {
111120
return Some(Duration::from_millis(value as u64));
112121
}
Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
//! Regression coverage for retry advice carried by failed Responses events.
2+
3+
use super::process_responses_event;
4+
use super::spawn_response_stream;
5+
use crate::api_bridge::map_api_error;
6+
use crate::common::ResponseEvent;
7+
use bytes::Bytes;
8+
use codex_client::StreamResponse;
9+
use codex_http_client::RetryAfter;
10+
use codex_protocol::protocol::CodexErrorInfo;
11+
use futures::StreamExt;
12+
use futures::stream;
13+
use http::HeaderMap;
14+
use http::StatusCode;
15+
use pretty_assertions::assert_eq;
16+
use serde_json::json;
17+
use std::time::Duration;
18+
19+
/// Flex failures remain terminal even with retry headers and a later completed event.
20+
#[tokio::test(start_paused = true)]
21+
async fn flex_failure_with_retry_header_ends_stream_immediately() {
22+
let error = json!({
23+
"code": "flex_unavailable",
24+
"plan_type": 42,
25+
"headers": {"retry-after": "300"}
26+
});
27+
for failure in [
28+
json!({"type": "error", "error": error}),
29+
json!({"type": "response.failed", "response": {"error": error}}),
30+
] {
31+
let completed = json!({"type": "response.completed", "response": {"id": "later"}});
32+
let bytes = stream::iter([Ok(Bytes::from(format!(
33+
"data: {failure}\n\ndata: {completed}\n\n"
34+
)))])
35+
.chain(stream::pending());
36+
let mut stream = spawn_response_stream(
37+
StreamResponse {
38+
status: StatusCode::OK,
39+
headers: HeaderMap::new(),
40+
bytes: Box::pin(bytes),
41+
},
42+
Duration::from_secs(60),
43+
/*telemetry*/ None,
44+
/*turn_state*/ None,
45+
);
46+
let error = loop {
47+
match stream.next().await {
48+
Some(Ok(ResponseEvent::RateLimits(_))) => {}
49+
Some(Err(error)) => break map_api_error(error),
50+
event => panic!("expected a Flex error, got {event:?}"),
51+
}
52+
};
53+
assert_eq!(
54+
(
55+
error.to_codex_protocol_error(),
56+
error.retry_delay(/*retry_count*/ 1)
57+
),
58+
(CodexErrorInfo::FlexUnavailable, None)
59+
);
60+
assert!(stream.next().await.is_none());
61+
}
62+
}
63+
64+
/// Error headers beat rate-limit message advice; invalid ones fall back.
65+
#[tokio::test(start_paused = true)]
66+
async fn rate_limit_plaintext_loses_to_valid_error_header() {
67+
for code in ["rate_limit_exceeded", "slow_down"] {
68+
for (error_header, expected_seconds) in [
69+
(Some("2"), 2),
70+
(Some("0"), 0),
71+
(Some("Wed, 21 Oct 2015 07:28:00 GMT"), 0),
72+
(Some("bad"), 35),
73+
(None, 35),
74+
] {
75+
let event = serde_json::from_value(json!({
76+
"type": "response.failed",
77+
"response": {
78+
"error": {
79+
"code": code,
80+
"message": "Try again in 35 seconds.",
81+
"headers": {"Retry-After": error_header}
82+
}
83+
}
84+
}))
85+
.unwrap();
86+
let error = map_api_error(process_responses_event(event).unwrap_err().into_api_error());
87+
assert_eq!(
88+
error.retry_after(),
89+
RetryAfter::from_delay(Duration::from_secs(expected_seconds))
90+
);
91+
}
92+
}
93+
}
94+
95+
/// Retry advice uses the same HTTP value validation as other event headers.
96+
#[tokio::test(start_paused = true)]
97+
async fn streamed_retry_after_uses_http_header_validation() {
98+
for (error_headers, expected_seconds) in [
99+
(json!({"retry-after": "\n5\n"}), 12),
100+
(json!({"retry-after": "\t5\t"}), 5),
101+
(json!({"Retry-After": "5", "retry-after": "30"}), 30),
102+
] {
103+
let event = serde_json::from_value(json!({
104+
"type": "response.failed",
105+
"response": {"error": {"code": "rate_limit_exceeded", "message": "Try again in 12 seconds.", "headers": error_headers}}
106+
}))
107+
.unwrap();
108+
let error = map_api_error(process_responses_event(event).unwrap_err().into_api_error());
109+
assert_eq!(
110+
error.retry_after(),
111+
RetryAfter::from_delay(Duration::from_secs(expected_seconds))
112+
);
113+
}
114+
}
115+
116+
/// Event headers retain their deadlines for overload and generic failures.
117+
#[tokio::test(start_paused = true)]
118+
async fn retry_header_survives_other_failed_responses() {
119+
for (code, expected_code) in [
120+
("server_is_overloaded", CodexErrorInfo::ServerOverloaded),
121+
("unknown_failure", CodexErrorInfo::Other),
122+
] {
123+
let event = serde_json::from_value(json!({
124+
"type": "response.failed",
125+
"response": {"error": {"code": code, "headers": {"retry-after": "7"}}},
126+
}))
127+
.unwrap();
128+
let error = map_api_error(process_responses_event(event).unwrap_err().into_api_error());
129+
assert_eq!(
130+
(
131+
error.to_codex_protocol_error(),
132+
error.retry_after(),
133+
error.retry_delay(/*retry_count*/ 1),
134+
),
135+
(
136+
expected_code,
137+
RetryAfter::from_delay(Duration::from_secs(7)),
138+
Some(Duration::from_secs(7)),
139+
)
140+
);
141+
}
142+
}
143+
144+
/// An event header cannot make a quota or policy failure retryable.
145+
#[tokio::test(start_paused = true)]
146+
async fn retry_header_does_not_override_terminal_errors() {
147+
for code in ["insufficient_quota", "cyber_policy", "bio_policy"] {
148+
let event = serde_json::from_value(json!({
149+
"type": "response.failed",
150+
"response": {"error": {"code": code, "headers": {"retry-after": "1"}}}
151+
}))
152+
.unwrap();
153+
let error = map_api_error(process_responses_event(event).unwrap_err().into_api_error());
154+
assert_eq!(error.retry_delay(/*retry_count*/ 1), None, "{code}");
155+
}
156+
}

0 commit comments

Comments
 (0)