Skip to content

Commit 41e2ed5

Browse files
REMOTE-2931: carry HTTP status on public-API errors so deterministic 4xx stop retrying
error_from_response converted non-success public API responses into ClientError/CloudAgentCapacityError/AIApiError, none of which carry an HTTP status. is_transient_http_error defaults to transient when no HttpStatusError is in the error chain, so checkpoint commit retried a deterministic 400 three times before giving up. Wrap every branch of error_from_response as anyhow::Error::new(HttpStatusError { status, body }).context(...), preserving the existing Display text and downcast_ref::<T>() behavior (anyhow's context-chain downcasting matches on the context type directly), while making the HttpStatusError reachable via e.chain() for is_transient_http_error and is_auth_error. commit_snapshot (checkpoint commit) is the only current with_bounded_retry caller that goes through this path for POST/PUT/PATCH/DELETE public-API calls, since GET already carried status correctly via warp_server_client::public_api. No other retry behavior changes today.
1 parent 213c9b3 commit 41e2ed5

2 files changed

Lines changed: 130 additions & 7 deletions

File tree

app/src/server/server_api.rs

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ use warp_core::context_flag::ContextFlag;
4444
use warp_core::telemetry::TelemetryEvent;
4545
use warp_errors::{AnyhowErrorExt, ErrorExt, register_error, report_error};
4646
use warp_managed_secrets::client::ManagedSecretsClient;
47+
use warp_server_client::HttpStatusError;
4748
use warp_server_client::auth::{AuthClientImpl, AuthEvent, EXPERIMENT_ID_HEADER};
4849
use warp_server_client::base_client::{
4950
AmbientHeaderPolicy, AuthenticatedGraphqlConfig, BaseClient, GraphqlRoutingConfig,
@@ -676,7 +677,11 @@ impl ServerApi {
676677
}
677678
}
678679

679-
/// Converts a non-success public API response into the most specific client error available.
680+
/// Converts a non-success public API response into the most specific client error
681+
/// available. The returned error always carries an [`HttpStatusError`] in its chain
682+
/// (via [`anyhow::Error::context`]) so callers retrying through
683+
/// [`is_transient_http_error`](super::retry_strategies::is_transient_http_error) fail
684+
/// fast on a deterministic 4xx instead of defaulting to a transient retry.
680685
async fn error_from_response(response: http_client::Response) -> anyhow::Error {
681686
let status = response.status();
682687
let is_at_capacity = response
@@ -692,28 +697,32 @@ impl ServerApi {
692697

693698
// Get the response text first since we may need to try multiple deserializations.
694699
let response_text = response.text().await.unwrap_or_default();
700+
let status_error = HttpStatusError {
701+
status: status.as_u16(),
702+
body: response_text.clone(),
703+
};
695704

696705
// Check for AT_CAPACITY error code header.
697706
if is_at_capacity
698707
&& let Ok(capacity_error) =
699708
serde_json::from_str::<CloudAgentCapacityError>(&response_text)
700709
{
701-
return capacity_error.into();
710+
return anyhow::Error::new(status_error).context(capacity_error);
702711
}
703712
if status == StatusCode::TOO_MANY_REQUESTS && is_out_of_credits {
704713
let user_display_message = serde_json::from_str::<OutOfCreditsResponse>(&response_text)
705714
.ok()
706715
.and_then(|r| r.user_display_message);
707-
return AIApiError::QuotaLimit {
716+
return anyhow::Error::new(status_error).context(AIApiError::QuotaLimit {
708717
user_display_message,
709-
}
710-
.into();
718+
});
711719
}
712720

713721
// Try to deserialize error response as { "error": "message" }
714722
match serde_json::from_str::<ClientError>(&response_text) {
715-
Ok(error_response) => error_response.into(),
716-
Err(_) => anyhow!("API request failed with status {status}"),
723+
Ok(error_response) => anyhow::Error::new(status_error).context(error_response),
724+
Err(_) => anyhow::Error::new(status_error)
725+
.context(format!("API request failed with status {status}")),
717726
}
718727
}
719728

@@ -1416,3 +1425,7 @@ impl Entity for ServerApiProvider {
14161425
}
14171426

14181427
impl SingletonEntity for ServerApiProvider {}
1428+
1429+
#[cfg(test)]
1430+
#[path = "server_api_tests.rs"]
1431+
mod tests;

app/src/server/server_api_tests.rs

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
use futures::executor::block_on;
2+
use mockito::Server;
3+
4+
use super::*;
5+
use crate::server::retry_strategies::is_transient_http_error;
6+
7+
/// Sends a GET request to a mock endpoint returning `status`/`headers`/`body`, then feeds the
8+
/// resulting response through [`ServerApi::error_from_response`].
9+
fn error_from_mock_response(status: usize, headers: &[(&str, &str)], body: &str) -> anyhow::Error {
10+
let mut server = Server::new();
11+
let mut mock = server
12+
.mock("GET", "/error")
13+
.with_status(status)
14+
.with_body(body);
15+
for (name, value) in headers {
16+
mock = mock.with_header(*name, value);
17+
}
18+
mock.create();
19+
20+
let url = format!("{}/error", server.url());
21+
block_on(async move {
22+
let response = http_client::Client::new_for_test()
23+
.get(url)
24+
.send()
25+
.await
26+
.unwrap();
27+
ServerApi::error_from_response(response).await
28+
})
29+
}
30+
31+
/// The status carried by the [`HttpStatusError`] in `err`'s chain, if any.
32+
fn status_in_chain(err: &anyhow::Error) -> Option<u16> {
33+
err.chain()
34+
.find_map(|cause| cause.downcast_ref::<HttpStatusError>())
35+
.map(|status_error| status_error.status)
36+
}
37+
38+
#[test]
39+
fn permanent_4xx_client_error_carries_status_and_fails_fast() {
40+
let err = error_from_mock_response(
41+
403,
42+
&[],
43+
r#"{"error": "checkpoint generation is incomplete"}"#,
44+
);
45+
46+
assert_eq!(status_in_chain(&err), Some(403));
47+
assert!(!is_transient_http_error(&err));
48+
assert_eq!(err.to_string(), "checkpoint generation is incomplete");
49+
assert_eq!(
50+
err.downcast_ref::<ClientError>().unwrap().error,
51+
"checkpoint generation is incomplete"
52+
);
53+
}
54+
55+
#[test]
56+
fn permanent_4xx_without_parseable_body_still_carries_status() {
57+
let err = error_from_mock_response(404, &[], "not found");
58+
59+
assert_eq!(status_in_chain(&err), Some(404));
60+
assert!(!is_transient_http_error(&err));
61+
assert_eq!(
62+
err.to_string(),
63+
"API request failed with status 404 Not Found"
64+
);
65+
}
66+
67+
#[test]
68+
fn transient_5xx_still_retries() {
69+
let err = error_from_mock_response(503, &[], "unavailable");
70+
71+
assert_eq!(status_in_chain(&err), Some(503));
72+
assert!(is_transient_http_error(&err));
73+
}
74+
75+
#[test]
76+
fn at_capacity_header_wraps_capacity_error_and_still_carries_status() {
77+
let err = error_from_mock_response(
78+
403,
79+
&[(WARP_ERROR_CODE_HEADER, WARP_ERROR_CODE_AT_CAPACITY)],
80+
r#"{"error": "at capacity", "running_agents": 5}"#,
81+
);
82+
83+
assert_eq!(status_in_chain(&err), Some(403));
84+
assert!(!is_transient_http_error(&err));
85+
assert_eq!(
86+
err.downcast_ref::<CloudAgentCapacityError>()
87+
.unwrap()
88+
.running_agents,
89+
5
90+
);
91+
}
92+
93+
#[test]
94+
fn out_of_credits_429_wraps_quota_limit_and_stays_transient() {
95+
let err = error_from_mock_response(
96+
429,
97+
&[(WARP_ERROR_CODE_HEADER, WARP_ERROR_CODE_OUT_OF_CREDITS)],
98+
r#"{"userDisplayMessage": "You're out of credits"}"#,
99+
);
100+
101+
// 429 always retries regardless of error code, matching every other public-API caller.
102+
assert_eq!(status_in_chain(&err), Some(429));
103+
assert!(is_transient_http_error(&err));
104+
assert!(matches!(
105+
err.downcast_ref::<AIApiError>().unwrap(),
106+
AIApiError::QuotaLimit {
107+
user_display_message: Some(message)
108+
} if message == "You're out of credits"
109+
));
110+
}

0 commit comments

Comments
 (0)