From b72d6476ae13b025680f0508c209aa497b0a4a49 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Mon, 28 Apr 2025 20:57:02 -0500 Subject: [PATCH] feat(network): use Retry-After header for HTTP 429 responses --- crates/cargo-test-support/src/registry.rs | 13 ++++ src/cargo/util/network/retry.rs | 93 ++++++++++++++++++++++- tests/testsuite/registry.rs | 51 +++++++++++++ 3 files changed, 153 insertions(+), 4 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index c547921ceb9..e23e8ac7b59 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -1057,6 +1057,19 @@ impl HttpServer { } } + /// Return too many requests (HTTP 429) + pub fn too_many_requests(&self, _req: &Request, delay: std::time::Duration) -> Response { + Response { + code: 429, + headers: vec![format!("Retry-After: {}", delay.as_secs())], + body: format!( + "too many requests, try again in {} seconds", + delay.as_secs() + ) + .into_bytes(), + } + } + /// Serve the download endpoint pub fn dl(&self, req: &Request) -> Response { let file = self diff --git a/src/cargo/util/network/retry.rs b/src/cargo/util/network/retry.rs index 41fecd97435..5d4e58024d5 100644 --- a/src/cargo/util/network/retry.rs +++ b/src/cargo/util/network/retry.rs @@ -104,8 +104,8 @@ impl<'a> Retry<'a> { pub fn r#try(&mut self, f: impl FnOnce() -> CargoResult) -> RetryResult { match f() { Err(ref e) if maybe_spurious(e) && self.retries < self.max_retries => { - let err_msg = e - .downcast_ref::() + let err = e.downcast_ref::(); + let err_msg = err .map(|http_err| http_err.display_short()) .unwrap_or_else(|| e.root_cause().to_string()); let left_retries = self.max_retries - self.retries; @@ -118,7 +118,12 @@ impl<'a> Retry<'a> { return RetryResult::Err(e); } self.retries += 1; - RetryResult::Retry(self.next_sleep_ms()) + let sleep = err + .and_then(|v| Self::parse_retry_after(v, &jiff::Timestamp::now())) + // Limit the Retry-After to a maximum value to avoid waiting too long. + .map(|retry_after| retry_after.min(MAX_RETRY_SLEEP_MS)) + .unwrap_or_else(|| self.next_sleep_ms()); + RetryResult::Retry(sleep) } Err(e) => RetryResult::Err(e), Ok(r) => RetryResult::Success(r), @@ -141,6 +146,42 @@ impl<'a> Retry<'a> { ) } } + + /// Parse the HTTP `Retry-After` header. + /// Returns the number of milliseconds to wait before retrying according to the header. + fn parse_retry_after(response: &HttpNotSuccessful, now: &jiff::Timestamp) -> Option { + // Only applies to HTTP 429 (too many requests) and 503 (service unavailable). + if !matches!(response.code, 429 | 503) { + return None; + } + + // Extract the Retry-After header value. + let retry_after = response + .headers + .iter() + .filter_map(|h| h.split_once(':')) + .map(|(k, v)| (k.trim(), v.trim())) + .find(|(k, _)| k.eq_ignore_ascii_case("retry-after"))? + .1; + + // First option: Retry-After is a positive integer of seconds to wait. + if let Ok(delay_secs) = retry_after.parse::() { + return Some(delay_secs as u64 * 1000); + } + + // Second option: Retry-After is a future HTTP date string that tells us when to retry. + if let Ok(retry_time) = jiff::fmt::rfc2822::parse(retry_after) { + let diff_ms = now + .until(&retry_time) + .unwrap() + .total(jiff::Unit::Millisecond) + .unwrap(); + if diff_ms > 0.0 { + return Some(diff_ms as u64); + } + } + None + } } fn maybe_spurious(err: &Error) -> bool { @@ -169,7 +210,7 @@ fn maybe_spurious(err: &Error) -> bool { } } if let Some(not_200) = err.downcast_ref::() { - if 500 <= not_200.code && not_200.code < 600 { + if 500 <= not_200.code && not_200.code < 600 || not_200.code == 429 { return true; } } @@ -317,3 +358,47 @@ fn curle_http2_stream_is_spurious() { let err = curl::Error::new(code); assert!(maybe_spurious(&err.into())); } + +#[test] +fn retry_after_parsing() { + use crate::core::Shell; + fn spurious(code: u32, header: &str) -> HttpNotSuccessful { + HttpNotSuccessful { + code, + url: "Uri".to_string(), + ip: None, + body: Vec::new(), + headers: vec![header.to_string()], + } + } + + // Start of year 2025. + let now = jiff::Timestamp::new(1735689600, 0).unwrap(); + let headers = spurious(429, "Retry-After: 10"); + assert_eq!(Retry::parse_retry_after(&headers, &now), Some(10_000)); + let headers = spurious(429, "retry-after: Wed, 01 Jan 2025 00:00:10 GMT"); + let actual = Retry::parse_retry_after(&headers, &now).unwrap(); + assert_eq!(10000, actual); + + let headers = spurious(429, "Content-Type: text/html"); + assert_eq!(Retry::parse_retry_after(&headers, &now), None); + + let headers = spurious(429, "retry-after: Fri, 01 Jan 2000 00:00:00 GMT"); + assert_eq!(Retry::parse_retry_after(&headers, &now), None); + + let headers = spurious(429, "retry-after: -1"); + assert_eq!(Retry::parse_retry_after(&headers, &now), None); + + let headers = spurious(400, "retry-after: 1"); + assert_eq!(Retry::parse_retry_after(&headers, &now), None); + + let gctx = GlobalContext::default().unwrap(); + *gctx.shell() = Shell::from_write(Box::new(Vec::new())); + let mut retry = Retry::new(&gctx).unwrap(); + match retry + .r#try(|| -> CargoResult<()> { Err(anyhow::Error::from(spurious(429, "Retry-After: 7"))) }) + { + RetryResult::Retry(sleep) => assert_eq!(sleep, 7_000), + _ => panic!("unexpected non-retry"), + } +} diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index b4e9fbbc60c..adf2d3cd090 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -3881,6 +3881,57 @@ fn dl_retry_multiple() { .run(); } +#[cargo_test] +fn retry_too_many_requests() { + let fail_count = Mutex::new(0); + let _registry = RegistryBuilder::new() + .http_index() + .add_responder("/index/3/b/bar", move |req, server| { + let mut fail_count = fail_count.lock().unwrap(); + if *fail_count < 1 { + *fail_count += 1; + server.too_many_requests(req, std::time::Duration::from_secs(1)) + } else { + server.index(req) + } + }) + .build(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + bar = ">= 0.0.0" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + Package::new("bar", "0.0.1").publish(); + + p.cargo("check") + .with_stderr_data(str![[r#" +[UPDATING] `dummy-registry` index +[WARNING] spurious network error (3 tries remaining): failed to get successful HTTP response from `[..]/index/3/b/bar` ([..]), got 429 +body: +too many requests, try again in 1 seconds +[LOCKING] 1 package to latest compatible version +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `dummy-registry`) +[CHECKING] bar v0.0.1 +[CHECKING] foo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]).run(); +} + #[cargo_test] fn deleted_entry() { // Checks the behavior when a package is removed from the index.