From 6cc57451d382aaee80636d022eeee72101477181 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 07:41:22 +0000 Subject: [PATCH 1/4] fix: retry pinned binary downloads on transient failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Downloads of pinned binaries (memtrack/exec-harness/mongo-tracer installers, valgrind .deb) intermittently fail in CI with "Failed to download file: error sending request for url (...)", and re-running the job almost always fixes it. The retry middleware on REQUEST_CLIENT only covers send() — up to the response headers — with a ~7s total backoff window, and classifies several transient network errors (e.g. BrokenPipe/UnexpectedEof io errors) as fatal. Body-read failures and torn transfers caught by the SHA-256 check were never retried at all. Wrap the whole download-and-verify in an outer retry loop (3 retries, 2s-30s exponential backoff) that retries any transient failure: request errors, retryable HTTP statuses, body-read errors, and hash mismatches. Client errors like 404 and local filesystem errors still fail immediately. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0184GnvJzAEgQQekzGvZq1ec --- src/cli/run/helpers/download_file.rs | 274 +++++++++++++++++++++++++-- 1 file changed, 254 insertions(+), 20 deletions(-) diff --git a/src/cli/run/helpers/download_file.rs b/src/cli/run/helpers/download_file.rs index 7b9e67b7a..b2056aa41 100644 --- a/src/cli/run/helpers/download_file.rs +++ b/src/cli/run/helpers/download_file.rs @@ -1,49 +1,283 @@ use crate::binary_pins::PinnedBinary; use crate::{prelude::*, request_client::REQUEST_CLIENT}; +use reqwest_retry::{ + RetryDecision, RetryPolicy, Retryable, default_on_request_success, policies::ExponentialBackoff, +}; use std::path::Path; +use std::time::SystemTime; use url::Url; -async fn download_file(url: &Url, path: &Path) -> Result<()> { +/// Number of whole-download retries on top of the per-request retries already +/// performed by the middleware on [`REQUEST_CLIENT`]. The middleware only +/// covers `send()` (until response headers are received), so body-read +/// failures, hash mismatches from torn downloads, and outages longer than its +/// backoff window still need retrying here. +const DOWNLOAD_RETRY_COUNT: u32 = 3; + +/// Backoff policy for whole-download retries. Under `cfg(test)` the intervals +/// are shrunk to milliseconds so retry tests don't sleep through the real +/// exponential backoff. +fn download_backoff() -> ExponentialBackoff { + let builder = ExponentialBackoff::builder(); + #[cfg(test)] + let builder = builder.retry_bounds( + std::time::Duration::from_millis(1), + std::time::Duration::from_millis(5), + ); + #[cfg(not(test))] + let builder = builder.retry_bounds( + std::time::Duration::from_secs(2), + std::time::Duration::from_secs(30), + ); + builder.build_with_max_retries(DOWNLOAD_RETRY_COUNT) +} + +/// Error from a single download-and-verify attempt, split by whether another +/// attempt can help. +enum AttemptError { + /// Another attempt cannot succeed (e.g. 404, local filesystem error). + Fatal(Error), + /// Network flakiness or a corrupted transfer; worth re-downloading. + Transient(Error), +} + +async fn download_file(url: &Url, path: &Path) -> Result<(), AttemptError> { debug!("Downloading file: {url}"); let response = REQUEST_CLIENT .get(url.clone()) .send() .await - .map_err(|e| anyhow!("Failed to download file: {e}"))?; + .map_err(|e| AttemptError::Transient(anyhow!("Failed to download file: {e}")))?; if !response.status().is_success() { - bail!("Failed to download file: {}", response.status()); + let error = anyhow!("Failed to download file: {}", response.status()); + return Err(match default_on_request_success(&response) { + Some(Retryable::Fatal) => AttemptError::Fatal(error), + _ => AttemptError::Transient(error), + }); } - let mut file = std::fs::File::create(path) - .map_err(|e| anyhow!("Failed to create file: {}, {}", path.display(), e))?; + let mut file = std::fs::File::create(path).map_err(|e| { + AttemptError::Fatal(anyhow!("Failed to create file: {}, {}", path.display(), e)) + })?; let content = response .bytes() .await - .map_err(|e| anyhow!("Failed to read response: {e}"))?; - std::io::copy(&mut content.as_ref(), &mut file) - .map_err(|e| anyhow!("Failed to write to file: {}, {}", path.display(), e))?; + .map_err(|e| AttemptError::Transient(anyhow!("Failed to read response: {e}")))?; + std::io::copy(&mut content.as_ref(), &mut file).map_err(|e| { + AttemptError::Fatal(anyhow!( + "Failed to write to file: {}, {}", + path.display(), + e + )) + })?; Ok(()) } -/// Download a `PinnedBinary` and verify its bytes against its pinned -/// SHA-256. On mismatch the partial file is +async fn download_and_verify_once( + url: &Url, + expected_sha256: &str, + path: &Path, +) -> Result<(), AttemptError> { + download_file(url, path).await?; + + let actual = sha256::try_digest(path).map_err(|e| { + AttemptError::Fatal( + anyhow!(e).context(format!("failed to compute sha256 of {}", path.display())), + ) + })?; + + if actual != expected_sha256 { + let _ = std::fs::remove_file(path); + return Err(AttemptError::Transient(anyhow!( + "Hash mismatch for {url}: expected {expected_sha256}, got {actual}. The downloaded file has been deleted." + ))); + } + + debug!("Verified sha256 of {url}"); + Ok(()) +} + +/// Download a URL and verify its bytes against an expected SHA-256, retrying +/// the whole download on transient failures (network errors, retryable HTTP +/// statuses, and hash mismatches from torn transfers). On a final mismatch the +/// partial file is removed and an error is returned. +async fn download_and_verify(url: &Url, expected_sha256: &str, path: &Path) -> Result<()> { + let policy = download_backoff(); + let start = SystemTime::now(); + let mut n_past_retries = 0; + + loop { + let error = match download_and_verify_once(url, expected_sha256, path).await { + Ok(()) => return Ok(()), + Err(AttemptError::Fatal(error)) => return Err(error), + Err(AttemptError::Transient(error)) => error, + }; + + match policy.should_retry(start, n_past_retries) { + RetryDecision::Retry { execute_after } => { + let wait = execute_after + .duration_since(SystemTime::now()) + .unwrap_or_default(); + warn!("Downloading {url} failed: {error}. Retrying in {wait:?}"); + tokio::time::sleep(wait).await; + n_past_retries += 1; + } + RetryDecision::DoNotRetry => return Err(error), + } + } +} + +/// Download a `PinnedBinary` and verify its bytes against its pinned SHA-256, +/// retrying transient failures. On a final mismatch the partial file is /// removed and an error is returned. pub async fn download_pinned_file(binary: PinnedBinary, path: &Path) -> Result<()> { let url_str = binary.url(); let url = Url::parse(&url_str).context("failed to parse pinned URL")?; - download_file(&url, path).await?; + download_and_verify(&url, binary.sha256(), path).await +} - let actual = sha256::try_digest(path) - .with_context(|| format!("failed to compute sha256 of {}", path.display()))?; - let expected = binary.sha256(); +#[cfg(test)] +mod tests { + use super::*; + use std::io::{Read, Write}; + use std::net::TcpListener; + use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; + use tempfile::NamedTempFile; - if actual != expected { - let _ = std::fs::remove_file(path); - bail!( - "Hash mismatch for {url_str}: expected {expected}, got {actual}. The downloaded file has been deleted." + const GOOD_BODY: &[u8] = b"expected file content"; + const BAD_BODY: &[u8] = b"corrupted file content"; + + enum ScriptedResponse { + /// Respond 200 with the given body. + Body(&'static [u8]), + /// Respond with the given status code and an empty body. + Status(u16), + /// Close the connection without responding. + Abort, + } + + /// Serve one scripted response per connection, then stop listening. + /// Every response closes the connection so each request is a new + /// connection, making the accept counter a request counter. + fn spawn_scripted_server(script: Vec) -> (Url, Arc) { + let listener = TcpListener::bind("127.0.0.1:0").expect("failed to bind test server"); + let url = Url::parse(&format!("http://{}/file", listener.local_addr().unwrap())).unwrap(); + let request_count = Arc::new(AtomicUsize::new(0)); + + let counter = Arc::clone(&request_count); + std::thread::spawn(move || { + for response in script { + let (mut stream, _) = match listener.accept() { + Ok(connection) => connection, + Err(_) => return, + }; + counter.fetch_add(1, Ordering::SeqCst); + + // Read until the end of the request headers. + let mut request = Vec::new(); + let mut buf = [0u8; 1024]; + loop { + match stream.read(&mut buf) { + Ok(0) | Err(_) => break, + Ok(n) => { + request.extend_from_slice(&buf[..n]); + if request.windows(4).any(|w| w == b"\r\n\r\n") { + break; + } + } + } + } + + match response { + ScriptedResponse::Body(body) => { + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + let _ = stream.write_all(body); + } + ScriptedResponse::Status(status) => { + let _ = write!( + stream, + "HTTP/1.1 {status} Test\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + ); + } + ScriptedResponse::Abort => {} + } + } + }); + + (url, request_count) + } + + #[tokio::test] + async fn retries_hash_mismatch_and_recovers() { + let (url, request_count) = spawn_scripted_server(vec![ + ScriptedResponse::Body(BAD_BODY), + ScriptedResponse::Body(GOOD_BODY), + ]); + let file = NamedTempFile::new().unwrap(); + + download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect("download should recover from a corrupted transfer"); + + assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + } + + #[tokio::test] + async fn does_not_retry_client_errors() { + let (url, request_count) = spawn_scripted_server(vec![ScriptedResponse::Status(404)]); + let file = NamedTempFile::new().unwrap(); + + let error = download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect_err("a 404 should fail the download"); + + assert!( + error.to_string().contains("404"), + "unexpected error: {error}" ); + assert_eq!(request_count.load(Ordering::SeqCst), 1); } - debug!("Verified sha256 of {url_str}"); - Ok(()) + #[tokio::test] + async fn fails_after_exhausting_retries_on_persistent_hash_mismatch() { + let attempts = (DOWNLOAD_RETRY_COUNT + 1) as usize; + let (url, request_count) = spawn_scripted_server( + (0..attempts) + .map(|_| ScriptedResponse::Body(BAD_BODY)) + .collect(), + ); + let file = NamedTempFile::new().unwrap(); + + let error = download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect_err("a persistent hash mismatch should fail the download"); + + assert!( + error.to_string().contains("Hash mismatch"), + "unexpected error: {error}" + ); + assert_eq!(request_count.load(Ordering::SeqCst), attempts); + assert!(!file.path().exists(), "partial file should be deleted"); + } + + #[tokio::test] + async fn recovers_from_aborted_connection() { + let (url, _) = spawn_scripted_server(vec![ + ScriptedResponse::Abort, + ScriptedResponse::Body(GOOD_BODY), + ]); + let file = NamedTempFile::new().unwrap(); + + download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect("download should recover from an aborted connection"); + + assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); + } } From 2cc113152c26e53edfe9a17cb5c9d22ca4eb8016 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 07:47:58 +0000 Subject: [PATCH 2/4] refactor: make the whole-download loop the single retry path Downloads now use the client without retry middleware, so retries are no longer nested: the outer download-and-verify loop is the only retry mechanism, covering send errors, retryable HTTP statuses, body-read failures, and hash mismatches uniformly. Retry count is bumped from 3 to 5 since there are no inner per-request retries anymore. With a single path, retry behavior is deterministic per failure, so the tests now assert exact request counts for aborted connections and transient 500s. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0184GnvJzAEgQQekzGvZq1ec --- src/cli/run/helpers/download_file.rs | 37 ++++++++++++++++++++-------- src/request_client.rs | 5 +++- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/src/cli/run/helpers/download_file.rs b/src/cli/run/helpers/download_file.rs index b2056aa41..686fc5b48 100644 --- a/src/cli/run/helpers/download_file.rs +++ b/src/cli/run/helpers/download_file.rs @@ -1,5 +1,5 @@ use crate::binary_pins::PinnedBinary; -use crate::{prelude::*, request_client::REQUEST_CLIENT}; +use crate::{prelude::*, request_client::STREAMING_CLIENT}; use reqwest_retry::{ RetryDecision, RetryPolicy, Retryable, default_on_request_success, policies::ExponentialBackoff, }; @@ -8,12 +8,12 @@ use std::time::SystemTime; use url::Url; -/// Number of whole-download retries on top of the per-request retries already -/// performed by the middleware on [`REQUEST_CLIENT`]. The middleware only -/// covers `send()` (until response headers are received), so body-read -/// failures, hash mismatches from torn downloads, and outages longer than its -/// backoff window still need retrying here. -const DOWNLOAD_RETRY_COUNT: u32 = 3; +/// Number of whole-download retries. This is the only retry path for +/// downloads: [`STREAMING_CLIENT`] has no retry middleware, so each attempt is +/// a single request and every failure mode — send errors, retryable HTTP +/// statuses, body-read failures, and hash mismatches from torn downloads — is +/// retried here. +const DOWNLOAD_RETRY_COUNT: u32 = 5; /// Backoff policy for whole-download retries. Under `cfg(test)` the intervals /// are shrunk to milliseconds so retry tests don't sleep through the real @@ -27,7 +27,7 @@ fn download_backoff() -> ExponentialBackoff { ); #[cfg(not(test))] let builder = builder.retry_bounds( - std::time::Duration::from_secs(2), + std::time::Duration::from_secs(1), std::time::Duration::from_secs(30), ); builder.build_with_max_retries(DOWNLOAD_RETRY_COUNT) @@ -44,7 +44,7 @@ enum AttemptError { async fn download_file(url: &Url, path: &Path) -> Result<(), AttemptError> { debug!("Downloading file: {url}"); - let response = REQUEST_CLIENT + let response = STREAMING_CLIENT .get(url.clone()) .send() .await @@ -268,7 +268,7 @@ mod tests { #[tokio::test] async fn recovers_from_aborted_connection() { - let (url, _) = spawn_scripted_server(vec![ + let (url, request_count) = spawn_scripted_server(vec![ ScriptedResponse::Abort, ScriptedResponse::Body(GOOD_BODY), ]); @@ -279,5 +279,22 @@ mod tests { .expect("download should recover from an aborted connection"); assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + } + + #[tokio::test] + async fn retries_server_errors_and_recovers() { + let (url, request_count) = spawn_scripted_server(vec![ + ScriptedResponse::Status(500), + ScriptedResponse::Body(GOOD_BODY), + ]); + let file = NamedTempFile::new().unwrap(); + + download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect("download should recover from a transient server error"); + + assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); + assert_eq!(request_count.load(Ordering::SeqCst), 2); } } diff --git a/src/request_client.rs b/src/request_client.rs index ce3ebbf7e..f765020bc 100644 --- a/src/request_client.rs +++ b/src/request_client.rs @@ -28,7 +28,10 @@ pub static REQUEST_CLIENT: LazyLock = LazyLock::new(|| { .build() }); -/// Client without retry middleware for streaming uploads (can't be cloned) +/// Client without retry middleware, for requests whose retries are handled by +/// a manual outer loop: streaming uploads (the middleware can't replay a +/// consumed stream) and pinned-binary downloads (the whole download-and-verify +/// is retried in `download_file.rs`). pub static STREAMING_CLIENT: LazyLock = LazyLock::new(|| ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()); From c91645cb6b59ac8bb507721cf60f0d013fbbbbe5 Mon Sep 17 00:00:00 2001 From: Arthur Pastel Date: Wed, 5 Aug 2026 23:32:43 +0200 Subject: [PATCH 3/4] refactor: retry downloads in middleware instead of an outer loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pinned binary downloads previously wrapped the whole download-and-verify in a manual retry loop. Replace it with a dedicated DOWNLOAD_CLIENT whose middleware uses a custom RetryableStrategy: downloads are idempotent GETs, so every request-level failure is retried, while responses keep the default status-based classification. This is what actually fixes the CI flakiness — the default strategy declines to retry a reqwest "error sending request" whose source is an UnexpectedEof, BrokenPipe or TimedOut io error, so a single GitHub blip failed a run after the benchmarks had already completed. A hash mismatch is now fatal instead of retried. The bytes arrived intact, since a torn transfer fails earlier on the body read, so a mismatch means the pin is wrong and retrying only delays the error. Body-read errors are no longer retried: the middleware only covers the send phase, up to the response headers. Co-Authored-By: Claude --- src/cli/run/helpers/download_file.rs | 191 ++++++--------------------- src/request_client.rs | 61 ++++++++- 2 files changed, 100 insertions(+), 152 deletions(-) diff --git a/src/cli/run/helpers/download_file.rs b/src/cli/run/helpers/download_file.rs index 686fc5b48..e1f842fad 100644 --- a/src/cli/run/helpers/download_file.rs +++ b/src/cli/run/helpers/download_file.rs @@ -1,135 +1,53 @@ use crate::binary_pins::PinnedBinary; -use crate::{prelude::*, request_client::STREAMING_CLIENT}; -use reqwest_retry::{ - RetryDecision, RetryPolicy, Retryable, default_on_request_success, policies::ExponentialBackoff, -}; +use crate::{prelude::*, request_client::DOWNLOAD_CLIENT}; use std::path::Path; -use std::time::SystemTime; use url::Url; -/// Number of whole-download retries. This is the only retry path for -/// downloads: [`STREAMING_CLIENT`] has no retry middleware, so each attempt is -/// a single request and every failure mode — send errors, retryable HTTP -/// statuses, body-read failures, and hash mismatches from torn downloads — is -/// retried here. -const DOWNLOAD_RETRY_COUNT: u32 = 5; - -/// Backoff policy for whole-download retries. Under `cfg(test)` the intervals -/// are shrunk to milliseconds so retry tests don't sleep through the real -/// exponential backoff. -fn download_backoff() -> ExponentialBackoff { - let builder = ExponentialBackoff::builder(); - #[cfg(test)] - let builder = builder.retry_bounds( - std::time::Duration::from_millis(1), - std::time::Duration::from_millis(5), - ); - #[cfg(not(test))] - let builder = builder.retry_bounds( - std::time::Duration::from_secs(1), - std::time::Duration::from_secs(30), - ); - builder.build_with_max_retries(DOWNLOAD_RETRY_COUNT) -} - -/// Error from a single download-and-verify attempt, split by whether another -/// attempt can help. -enum AttemptError { - /// Another attempt cannot succeed (e.g. 404, local filesystem error). - Fatal(Error), - /// Network flakiness or a corrupted transfer; worth re-downloading. - Transient(Error), -} - -async fn download_file(url: &Url, path: &Path) -> Result<(), AttemptError> { +async fn download_file(url: &Url, path: &Path) -> Result<()> { debug!("Downloading file: {url}"); - let response = STREAMING_CLIENT + let response = DOWNLOAD_CLIENT .get(url.clone()) .send() .await - .map_err(|e| AttemptError::Transient(anyhow!("Failed to download file: {e}")))?; + .map_err(|e| anyhow!("Failed to download file: {e}"))?; if !response.status().is_success() { - let error = anyhow!("Failed to download file: {}", response.status()); - return Err(match default_on_request_success(&response) { - Some(Retryable::Fatal) => AttemptError::Fatal(error), - _ => AttemptError::Transient(error), - }); + bail!("Failed to download file: {}", response.status()); } - let mut file = std::fs::File::create(path).map_err(|e| { - AttemptError::Fatal(anyhow!("Failed to create file: {}, {}", path.display(), e)) - })?; + let mut file = std::fs::File::create(path) + .map_err(|e| anyhow!("Failed to create file: {}, {}", path.display(), e))?; let content = response .bytes() .await - .map_err(|e| AttemptError::Transient(anyhow!("Failed to read response: {e}")))?; - std::io::copy(&mut content.as_ref(), &mut file).map_err(|e| { - AttemptError::Fatal(anyhow!( - "Failed to write to file: {}, {}", - path.display(), - e - )) - })?; + .map_err(|e| anyhow!("Failed to read response: {e}"))?; + std::io::copy(&mut content.as_ref(), &mut file) + .map_err(|e| anyhow!("Failed to write to file: {}, {}", path.display(), e))?; Ok(()) } -async fn download_and_verify_once( - url: &Url, - expected_sha256: &str, - path: &Path, -) -> Result<(), AttemptError> { +/// Download a URL and verify its bytes against an expected SHA-256. Transient +/// request failures are retried by the middleware on [`DOWNLOAD_CLIENT`]. A +/// mismatch is not retried — the bytes arrived intact (a torn transfer fails +/// earlier, on the body read), so it means the pin is wrong rather than the +/// download. The partial file is removed and an error is returned. +async fn download_and_verify(url: &Url, expected_sha256: &str, path: &Path) -> Result<()> { download_file(url, path).await?; - let actual = sha256::try_digest(path).map_err(|e| { - AttemptError::Fatal( - anyhow!(e).context(format!("failed to compute sha256 of {}", path.display())), - ) - })?; + let actual = sha256::try_digest(path) + .with_context(|| format!("failed to compute sha256 of {}", path.display()))?; if actual != expected_sha256 { let _ = std::fs::remove_file(path); - return Err(AttemptError::Transient(anyhow!( + bail!( "Hash mismatch for {url}: expected {expected_sha256}, got {actual}. The downloaded file has been deleted." - ))); + ); } debug!("Verified sha256 of {url}"); Ok(()) } -/// Download a URL and verify its bytes against an expected SHA-256, retrying -/// the whole download on transient failures (network errors, retryable HTTP -/// statuses, and hash mismatches from torn transfers). On a final mismatch the -/// partial file is removed and an error is returned. -async fn download_and_verify(url: &Url, expected_sha256: &str, path: &Path) -> Result<()> { - let policy = download_backoff(); - let start = SystemTime::now(); - let mut n_past_retries = 0; - - loop { - let error = match download_and_verify_once(url, expected_sha256, path).await { - Ok(()) => return Ok(()), - Err(AttemptError::Fatal(error)) => return Err(error), - Err(AttemptError::Transient(error)) => error, - }; - - match policy.should_retry(start, n_past_retries) { - RetryDecision::Retry { execute_after } => { - let wait = execute_after - .duration_since(SystemTime::now()) - .unwrap_or_default(); - warn!("Downloading {url} failed: {error}. Retrying in {wait:?}"); - tokio::time::sleep(wait).await; - n_past_retries += 1; - } - RetryDecision::DoNotRetry => return Err(error), - } - } -} - -/// Download a `PinnedBinary` and verify its bytes against its pinned SHA-256, -/// retrying transient failures. On a final mismatch the partial file is -/// removed and an error is returned. +/// Download a `PinnedBinary` and verify its bytes against its pinned SHA-256. pub async fn download_pinned_file(binary: PinnedBinary, path: &Path) -> Result<()> { let url_str = binary.url(); let url = Url::parse(&url_str).context("failed to parse pinned URL")?; @@ -213,16 +131,32 @@ mod tests { } #[tokio::test] - async fn retries_hash_mismatch_and_recovers() { + async fn recovers_from_aborted_connection() { + let (url, request_count) = spawn_scripted_server(vec![ + ScriptedResponse::Abort, + ScriptedResponse::Body(GOOD_BODY), + ]); + let file = NamedTempFile::new().unwrap(); + + download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect("download should recover from an aborted connection"); + + assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + } + + #[tokio::test] + async fn retries_server_errors_and_recovers() { let (url, request_count) = spawn_scripted_server(vec![ - ScriptedResponse::Body(BAD_BODY), + ScriptedResponse::Status(500), ScriptedResponse::Body(GOOD_BODY), ]); let file = NamedTempFile::new().unwrap(); download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) .await - .expect("download should recover from a corrupted transfer"); + .expect("download should recover from a transient server error"); assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); assert_eq!(request_count.load(Ordering::SeqCst), 2); @@ -245,56 +179,19 @@ mod tests { } #[tokio::test] - async fn fails_after_exhausting_retries_on_persistent_hash_mismatch() { - let attempts = (DOWNLOAD_RETRY_COUNT + 1) as usize; - let (url, request_count) = spawn_scripted_server( - (0..attempts) - .map(|_| ScriptedResponse::Body(BAD_BODY)) - .collect(), - ); + async fn does_not_retry_hash_mismatch() { + let (url, request_count) = spawn_scripted_server(vec![ScriptedResponse::Body(BAD_BODY)]); let file = NamedTempFile::new().unwrap(); let error = download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) .await - .expect_err("a persistent hash mismatch should fail the download"); + .expect_err("a hash mismatch should fail the download"); assert!( error.to_string().contains("Hash mismatch"), "unexpected error: {error}" ); - assert_eq!(request_count.load(Ordering::SeqCst), attempts); + assert_eq!(request_count.load(Ordering::SeqCst), 1); assert!(!file.path().exists(), "partial file should be deleted"); } - - #[tokio::test] - async fn recovers_from_aborted_connection() { - let (url, request_count) = spawn_scripted_server(vec![ - ScriptedResponse::Abort, - ScriptedResponse::Body(GOOD_BODY), - ]); - let file = NamedTempFile::new().unwrap(); - - download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) - .await - .expect("download should recover from an aborted connection"); - - assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); - assert_eq!(request_count.load(Ordering::SeqCst), 2); - } - - #[tokio::test] - async fn retries_server_errors_and_recovers() { - let (url, request_count) = spawn_scripted_server(vec![ - ScriptedResponse::Status(500), - ScriptedResponse::Body(GOOD_BODY), - ]); - let file = NamedTempFile::new().unwrap(); - - download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) - .await - .expect("download should recover from a transient server error"); - - assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); - assert_eq!(request_count.load(Ordering::SeqCst), 2); - } } diff --git a/src/request_client.rs b/src/request_client.rs index f765020bc..2f04b393e 100644 --- a/src/request_client.rs +++ b/src/request_client.rs @@ -2,10 +2,14 @@ use std::sync::LazyLock; use reqwest::ClientBuilder; use reqwest_middleware::{ClientBuilder as ClientWithMiddlewareBuilder, ClientWithMiddleware}; -use reqwest_retry::{RetryTransientMiddleware, policies::ExponentialBackoff}; +use reqwest_retry::{ + RetryTransientMiddleware, Retryable, RetryableStrategy, default_on_request_success, + policies::ExponentialBackoff, +}; pub const UPLOAD_RETRY_COUNT: u32 = 3; const OIDC_RETRY_COUNT: u32 = 10; +const DOWNLOAD_RETRY_COUNT: u32 = 5; const USER_AGENT: &str = "codspeed-runner"; /// Shared backoff policy for upload retries, used both by the retry middleware on @@ -22,16 +26,63 @@ pub fn upload_backoff() -> ExponentialBackoff { builder.build_with_max_retries(UPLOAD_RETRY_COUNT) } +/// Backoff policy for pinned binary downloads. Under `cfg(test)` the intervals +/// are shrunk to milliseconds so retry tests don't sleep through the real +/// exponential backoff. +fn download_backoff() -> ExponentialBackoff { + let builder = ExponentialBackoff::builder(); + #[cfg(test)] + let builder = builder.retry_bounds( + std::time::Duration::from_millis(1), + std::time::Duration::from_millis(5), + ); + #[cfg(not(test))] + let builder = builder.retry_bounds( + std::time::Duration::from_secs(1), + std::time::Duration::from_secs(30), + ); + builder.build_with_max_retries(DOWNLOAD_RETRY_COUNT) +} + +/// Retry strategy for downloads. `DefaultRetryableStrategy` classifies several +/// transient network errors as fatal — an `UnexpectedEof`, `BrokenPipe`, or +/// `TimedOut` io error surfaces as a reqwest "error sending request" that it +/// declines to retry — which made a single GitHub blip fail a whole CI run. +/// Downloads are idempotent GETs, so every request-level failure is safe to +/// retry; only responses keep the default status-based classification. +struct RetryAllRequestErrors; + +impl RetryableStrategy for RetryAllRequestErrors { + fn handle( + &self, + res: &Result, + ) -> Option { + match res { + Ok(success) => default_on_request_success(success), + // A failure in our own middleware stack won't fix itself. + Err(reqwest_middleware::Error::Middleware(_)) => Some(Retryable::Fatal), + Err(reqwest_middleware::Error::Reqwest(_)) => Some(Retryable::Transient), + } + } +} + pub static REQUEST_CLIENT: LazyLock = LazyLock::new(|| { ClientWithMiddlewareBuilder::new(ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()) .with(RetryTransientMiddleware::new_with_policy(upload_backoff())) .build() }); -/// Client without retry middleware, for requests whose retries are handled by -/// a manual outer loop: streaming uploads (the middleware can't replay a -/// consumed stream) and pinned-binary downloads (the whole download-and-verify -/// is retried in `download_file.rs`). +/// Client for pinned binary downloads, retrying any transient request failure. +pub static DOWNLOAD_CLIENT: LazyLock = LazyLock::new(|| { + ClientWithMiddlewareBuilder::new(ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()) + .with(RetryTransientMiddleware::new_with_policy_and_strategy( + download_backoff(), + RetryAllRequestErrors, + )) + .build() +}); + +/// Client without retry middleware for streaming uploads (can't be cloned) pub static STREAMING_CLIENT: LazyLock = LazyLock::new(|| ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()); From 4d2160350078655c07eca9d9d46c85bd3d7155f6 Mon Sep 17 00:00:00 2001 From: Arthur Pastel Date: Thu, 6 Aug 2026 00:00:42 +0200 Subject: [PATCH 4/4] fix: retry complete pinned binary downloads Retry the full request and response-body read so transient truncation does not fail benchmark runs. Keep permanent HTTP, filesystem, and hash errors fatal. Co-Authored-By: Claude --- src/cli/run/helpers/download_file.rs | 160 ++++++++++++++++++++++++--- src/request_client.rs | 59 +--------- 2 files changed, 147 insertions(+), 72 deletions(-) diff --git a/src/cli/run/helpers/download_file.rs b/src/cli/run/helpers/download_file.rs index e1f842fad..11a1fa65c 100644 --- a/src/cli/run/helpers/download_file.rs +++ b/src/cli/run/helpers/download_file.rs @@ -1,52 +1,118 @@ use crate::binary_pins::PinnedBinary; use crate::{prelude::*, request_client::DOWNLOAD_CLIENT}; +use reqwest_retry::{ + RetryDecision, RetryPolicy, Retryable, default_on_request_success, policies::ExponentialBackoff, +}; use std::path::Path; +use std::time::SystemTime; use url::Url; -async fn download_file(url: &Url, path: &Path) -> Result<()> { +const DOWNLOAD_RETRY_COUNT: u32 = 5; + +fn download_backoff() -> ExponentialBackoff { + let builder = ExponentialBackoff::builder(); + #[cfg(test)] + let builder = builder.retry_bounds( + std::time::Duration::from_millis(1), + std::time::Duration::from_millis(5), + ); + #[cfg(not(test))] + let builder = builder.retry_bounds( + std::time::Duration::from_secs(1), + std::time::Duration::from_secs(30), + ); + builder.build_with_max_retries(DOWNLOAD_RETRY_COUNT) +} + +enum AttemptError { + Fatal(Error), + Transient(Error), +} + +async fn download_file(url: &Url, path: &Path) -> Result<(), AttemptError> { debug!("Downloading file: {url}"); let response = DOWNLOAD_CLIENT .get(url.clone()) .send() .await - .map_err(|e| anyhow!("Failed to download file: {e}"))?; + .map_err(|e| AttemptError::Transient(anyhow!("Failed to download file: {e}")))?; + if !response.status().is_success() { - bail!("Failed to download file: {}", response.status()); + let error = anyhow!("Failed to download file: {}", response.status()); + return Err(match default_on_request_success(&response) { + Some(Retryable::Transient) => AttemptError::Transient(error), + _ => AttemptError::Fatal(error), + }); } - let mut file = std::fs::File::create(path) - .map_err(|e| anyhow!("Failed to create file: {}, {}", path.display(), e))?; + let content = response .bytes() .await - .map_err(|e| anyhow!("Failed to read response: {e}"))?; - std::io::copy(&mut content.as_ref(), &mut file) - .map_err(|e| anyhow!("Failed to write to file: {}, {}", path.display(), e))?; + .map_err(|e| AttemptError::Transient(anyhow!("Failed to read response: {e}")))?; + let mut file = std::fs::File::create(path).map_err(|e| { + AttemptError::Fatal(anyhow!("Failed to create file: {}, {}", path.display(), e)) + })?; + std::io::copy(&mut content.as_ref(), &mut file).map_err(|e| { + AttemptError::Fatal(anyhow!( + "Failed to write to file: {}, {}", + path.display(), + e + )) + })?; Ok(()) } -/// Download a URL and verify its bytes against an expected SHA-256. Transient -/// request failures are retried by the middleware on [`DOWNLOAD_CLIENT`]. A -/// mismatch is not retried — the bytes arrived intact (a torn transfer fails -/// earlier, on the body read), so it means the pin is wrong rather than the -/// download. The partial file is removed and an error is returned. -async fn download_and_verify(url: &Url, expected_sha256: &str, path: &Path) -> Result<()> { +async fn download_and_verify_once( + url: &Url, + expected_sha256: &str, + path: &Path, +) -> Result<(), AttemptError> { download_file(url, path).await?; - let actual = sha256::try_digest(path) - .with_context(|| format!("failed to compute sha256 of {}", path.display()))?; + let actual = sha256::try_digest(path).map_err(|e| { + AttemptError::Fatal( + anyhow!(e).context(format!("failed to compute sha256 of {}", path.display())), + ) + })?; if actual != expected_sha256 { let _ = std::fs::remove_file(path); - bail!( + return Err(AttemptError::Fatal(anyhow!( "Hash mismatch for {url}: expected {expected_sha256}, got {actual}. The downloaded file has been deleted." - ); + ))); } debug!("Verified sha256 of {url}"); Ok(()) } +async fn download_and_verify(url: &Url, expected_sha256: &str, path: &Path) -> Result<()> { + let policy = download_backoff(); + let start = SystemTime::now(); + let mut n_past_retries = 0; + + loop { + let error = match download_and_verify_once(url, expected_sha256, path).await { + Ok(()) => return Ok(()), + Err(AttemptError::Fatal(error)) => return Err(error), + Err(AttemptError::Transient(error)) => error, + }; + + match policy.should_retry(start, n_past_retries) { + RetryDecision::Retry { execute_after } => { + let wait = execute_after + .duration_since(SystemTime::now()) + .unwrap_or_default(); + warn!("Downloading {url} failed: {error}. Retrying in {wait:?}."); + tokio::time::sleep(wait).await; + n_past_retries += 1; + } + RetryDecision::DoNotRetry => return Err(error), + } + } +} + /// Download a `PinnedBinary` and verify its bytes against its pinned SHA-256. pub async fn download_pinned_file(binary: PinnedBinary, path: &Path) -> Result<()> { let url_str = binary.url(); @@ -69,6 +135,11 @@ mod tests { enum ScriptedResponse { /// Respond 200 with the given body. Body(&'static [u8]), + /// Respond 200 with fewer bytes than declared by `Content-Length`. + TruncatedBody { + body: &'static [u8], + declared_length: usize, + }, /// Respond with the given status code and an empty body. Status(u16), /// Close the connection without responding. @@ -116,6 +187,16 @@ mod tests { ); let _ = stream.write_all(body); } + ScriptedResponse::TruncatedBody { + body, + declared_length, + } => { + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\nContent-Length: {declared_length}\r\nConnection: close\r\n\r\n" + ); + let _ = stream.write_all(body); + } ScriptedResponse::Status(status) => { let _ = write!( stream, @@ -146,6 +227,49 @@ mod tests { assert_eq!(request_count.load(Ordering::SeqCst), 2); } + #[tokio::test] + async fn recovers_from_truncated_body() { + let (url, request_count) = spawn_scripted_server(vec![ + ScriptedResponse::TruncatedBody { + body: GOOD_BODY, + declared_length: GOOD_BODY.len() + 1, + }, + ScriptedResponse::Body(GOOD_BODY), + ]); + let file = NamedTempFile::new().unwrap(); + + download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect("download should recover from a truncated response body"); + + assert_eq!(std::fs::read(file.path()).unwrap(), GOOD_BODY); + assert_eq!(request_count.load(Ordering::SeqCst), 2); + } + + #[tokio::test] + async fn fails_after_exhausting_retries_on_truncated_bodies() { + let attempts = (DOWNLOAD_RETRY_COUNT + 1) as usize; + let (url, request_count) = spawn_scripted_server( + (0..attempts) + .map(|_| ScriptedResponse::TruncatedBody { + body: GOOD_BODY, + declared_length: GOOD_BODY.len() + 1, + }) + .collect(), + ); + let file = NamedTempFile::new().unwrap(); + + let error = download_and_verify(&url, &sha256::digest(GOOD_BODY), file.path()) + .await + .expect_err("persistent truncated bodies should fail the download"); + + assert!( + error.to_string().contains("Failed to read response"), + "unexpected error: {error}" + ); + assert_eq!(request_count.load(Ordering::SeqCst), attempts); + } + #[tokio::test] async fn retries_server_errors_and_recovers() { let (url, request_count) = spawn_scripted_server(vec![ diff --git a/src/request_client.rs b/src/request_client.rs index 2f04b393e..ab1734290 100644 --- a/src/request_client.rs +++ b/src/request_client.rs @@ -2,14 +2,10 @@ use std::sync::LazyLock; use reqwest::ClientBuilder; use reqwest_middleware::{ClientBuilder as ClientWithMiddlewareBuilder, ClientWithMiddleware}; -use reqwest_retry::{ - RetryTransientMiddleware, Retryable, RetryableStrategy, default_on_request_success, - policies::ExponentialBackoff, -}; +use reqwest_retry::{RetryTransientMiddleware, policies::ExponentialBackoff}; pub const UPLOAD_RETRY_COUNT: u32 = 3; const OIDC_RETRY_COUNT: u32 = 10; -const DOWNLOAD_RETRY_COUNT: u32 = 5; const USER_AGENT: &str = "codspeed-runner"; /// Shared backoff policy for upload retries, used both by the retry middleware on @@ -26,61 +22,16 @@ pub fn upload_backoff() -> ExponentialBackoff { builder.build_with_max_retries(UPLOAD_RETRY_COUNT) } -/// Backoff policy for pinned binary downloads. Under `cfg(test)` the intervals -/// are shrunk to milliseconds so retry tests don't sleep through the real -/// exponential backoff. -fn download_backoff() -> ExponentialBackoff { - let builder = ExponentialBackoff::builder(); - #[cfg(test)] - let builder = builder.retry_bounds( - std::time::Duration::from_millis(1), - std::time::Duration::from_millis(5), - ); - #[cfg(not(test))] - let builder = builder.retry_bounds( - std::time::Duration::from_secs(1), - std::time::Duration::from_secs(30), - ); - builder.build_with_max_retries(DOWNLOAD_RETRY_COUNT) -} - -/// Retry strategy for downloads. `DefaultRetryableStrategy` classifies several -/// transient network errors as fatal — an `UnexpectedEof`, `BrokenPipe`, or -/// `TimedOut` io error surfaces as a reqwest "error sending request" that it -/// declines to retry — which made a single GitHub blip fail a whole CI run. -/// Downloads are idempotent GETs, so every request-level failure is safe to -/// retry; only responses keep the default status-based classification. -struct RetryAllRequestErrors; - -impl RetryableStrategy for RetryAllRequestErrors { - fn handle( - &self, - res: &Result, - ) -> Option { - match res { - Ok(success) => default_on_request_success(success), - // A failure in our own middleware stack won't fix itself. - Err(reqwest_middleware::Error::Middleware(_)) => Some(Retryable::Fatal), - Err(reqwest_middleware::Error::Reqwest(_)) => Some(Retryable::Transient), - } - } -} - pub static REQUEST_CLIENT: LazyLock = LazyLock::new(|| { ClientWithMiddlewareBuilder::new(ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()) .with(RetryTransientMiddleware::new_with_policy(upload_backoff())) .build() }); -/// Client for pinned binary downloads, retrying any transient request failure. -pub static DOWNLOAD_CLIENT: LazyLock = LazyLock::new(|| { - ClientWithMiddlewareBuilder::new(ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()) - .with(RetryTransientMiddleware::new_with_policy_and_strategy( - download_backoff(), - RetryAllRequestErrors, - )) - .build() -}); +/// Client without retry middleware for pinned binary downloads. The downloader +/// retries complete attempts so response-body failures are covered too. +pub static DOWNLOAD_CLIENT: LazyLock = + LazyLock::new(|| ClientBuilder::new().user_agent(USER_AGENT).build().unwrap()); /// Client without retry middleware for streaming uploads (can't be cloned) pub static STREAMING_CLIENT: LazyLock =