diff --git a/crates/uv-client/src/base_client.rs b/crates/uv-client/src/base_client.rs index 33cd9e0d6..e57a53702 100644 --- a/crates/uv-client/src/base_client.rs +++ b/crates/uv-client/src/base_client.rs @@ -20,8 +20,8 @@ use reqwest::{Client, ClientBuilder, IntoUrl, Proxy, Request, Response, multipar use reqwest_middleware::{ClientWithMiddleware, Middleware}; use reqwest_retry::policies::ExponentialBackoff; use reqwest_retry::{ - DefaultRetryableStrategy, RetryTransientMiddleware, Retryable, RetryableStrategy, - default_on_request_error, + RetryTransientMiddleware, Retryable, RetryableStrategy, default_on_request_error, + default_on_request_success, }; use thiserror::Error; use tracing::{debug, trace}; @@ -1015,21 +1015,15 @@ impl<'a> RequestBuilder<'a> { } } -/// Extends [`DefaultRetryableStrategy`], to log transient request failures and additional retry cases. +/// An extension over [`DefaultRetryableStrategy`] that logs transient request failures and +/// adds additional retry cases. pub struct UvRetryableStrategy; impl RetryableStrategy for UvRetryableStrategy { fn handle(&self, res: &Result) -> Option { - // Use the default strategy and check for additional transient error cases. - let retryable = match DefaultRetryableStrategy.handle(res) { - None | Some(Retryable::Fatal) - if res - .as_ref() - .is_err_and(|err| is_transient_network_error(err)) => - { - Some(Retryable::Transient) - } - default => default, + let retryable = match res { + Ok(success) => default_on_request_success(success), + Err(err) => retryable_on_request_failure(err), }; // Log on transient errors @@ -1055,11 +1049,13 @@ impl RetryableStrategy for UvRetryableStrategy { /// Whether the error looks like a network error that should be retried. /// -/// There are two cases that the default retry strategy is missing: +/// This is an extension over [`reqwest_middleware::default_on_request_failure`], which is missing +/// a number of cases: /// * Inside the reqwest or reqwest-middleware error is an `io::Error` such as a broken pipe /// * When streaming a response, a reqwest error may be hidden several layers behind errors -/// of different crates processing the stream, including `io::Error` layers. -pub fn is_transient_network_error(err: &(dyn Error + 'static)) -> bool { +/// of different crates processing the stream, including `io::Error` layers +/// * Any `h2` error +fn retryable_on_request_failure(err: &(dyn Error + 'static)) -> Option { // First, try to show a nice trace log if let Some((Some(status), Some(url))) = find_source::(&err) .map(|request_err| (request_err.status(), request_err.url())) @@ -1074,29 +1070,32 @@ pub fn is_transient_network_error(err: &(dyn Error + 'static)) -> bool { // crates let mut current_source = Some(err); while let Some(source) = current_source { - if let Some(reqwest_err) = source.downcast_ref::() { - has_known_error = true; - if let reqwest_middleware::Error::Reqwest(reqwest_err) = &**reqwest_err { - if default_on_request_error(reqwest_err) == Some(Retryable::Transient) { - trace!("Retrying nested reqwest middleware error"); - return true; - } - if is_retryable_status_error(reqwest_err) { - trace!("Retrying nested reqwest middleware status code error"); - return true; - } - } + // Handle different kinds of reqwest error nesting not accessible by downcast. + let reqwest_err = if let Some(reqwest_err) = source.downcast_ref::() { + Some(reqwest_err) + } else if let Some(reqwest_err) = source + .downcast_ref::() + .and_then(|err| err.inner()) + { + Some(reqwest_err) + } else if let Some(reqwest_middleware::Error::Reqwest(reqwest_err)) = + source.downcast_ref::() + { + Some(reqwest_err) + } else { + None + }; - trace!("Cannot retry nested reqwest middleware error"); - } else if let Some(reqwest_err) = source.downcast_ref::() { + if let Some(reqwest_err) = reqwest_err { has_known_error = true; + // Ignore the default retry strategy returning fatal. if default_on_request_error(reqwest_err) == Some(Retryable::Transient) { trace!("Retrying nested reqwest error"); - return true; + return Some(Retryable::Transient); } if is_retryable_status_error(reqwest_err) { trace!("Retrying nested reqwest status code error"); - return true; + return Some(Retryable::Transient); } trace!("Cannot retry nested reqwest error"); @@ -1104,7 +1103,7 @@ pub fn is_transient_network_error(err: &(dyn Error + 'static)) -> bool { // All h2 errors look like errors that should be retried // https://github.com/astral-sh/uv/issues/15916 trace!("Retrying nested h2 error"); - return true; + return Some(Retryable::Transient); } else if let Some(io_err) = source.downcast_ref::() { has_known_error = true; let retryable_io_err_kinds = [ @@ -1121,7 +1120,7 @@ pub fn is_transient_network_error(err: &(dyn Error + 'static)) -> bool { ]; if retryable_io_err_kinds.contains(&io_err.kind()) { trace!("Retrying error: `{}`", io_err.kind()); - return true; + return Some(Retryable::Transient); } trace!( @@ -1136,7 +1135,12 @@ pub fn is_transient_network_error(err: &(dyn Error + 'static)) -> bool { if !has_known_error { trace!("Cannot retry error: Neither an IO error nor a reqwest error"); } - false + + None +} + +pub fn is_transient_network_error(err: &(dyn Error + 'static)) -> bool { + retryable_on_request_failure(err) == Some(Retryable::Transient) } /// Whether the error is a status code error that is retryable. @@ -1397,7 +1401,7 @@ mod tests { .await; let middleware_retry = - DefaultRetryableStrategy.handle(&response) == Some(Retryable::Transient); + UvRetryableStrategy.handle(&response) == Some(Retryable::Transient); let response = client .get(format!("{}/{}", server.uri(), status)) diff --git a/crates/uv-client/src/error.rs b/crates/uv-client/src/error.rs index 21180dc20..47e3646be 100644 --- a/crates/uv-client/src/error.rs +++ b/crates/uv-client/src/error.rs @@ -425,13 +425,33 @@ impl WrappedReqwestError { problem_details: Option, ) -> Self { Self { - error, + error: Self::filter_retries_from_error(error), problem_details: problem_details.map(Box::new), } } + /// Drop `RetryError::WithRetries` to avoid reporting the number of retries twice. + /// + /// We attach the number of errors outside by adding the retry counts from the retry middleware + /// and from uv's outer retry loop for streaming bodies. Stripping the inner count from the + /// error context avoids showing two numbers. + fn filter_retries_from_error(error: reqwest_middleware::Error) -> reqwest_middleware::Error { + match error { + reqwest_middleware::Error::Middleware(error) => { + match error.downcast::() { + Ok( + reqwest_retry::RetryError::WithRetries { err, .. } + | reqwest_retry::RetryError::Error(err), + ) => err, + Err(error) => reqwest_middleware::Error::Middleware(error), + } + } + error @ reqwest_middleware::Error::Reqwest(_) => error, + } + } + /// Return the inner [`reqwest::Error`] from the error chain, if it exists. - fn inner(&self) -> Option<&reqwest::Error> { + pub fn inner(&self) -> Option<&reqwest::Error> { match &self.error { reqwest_middleware::Error::Reqwest(err) => Some(err), reqwest_middleware::Error::Middleware(err) => err.chain().find_map(|err| { @@ -495,6 +515,7 @@ impl WrappedReqwestError { impl From for WrappedReqwestError { fn from(error: reqwest::Error) -> Self { Self { + // No need to filter retries as this error does not have retries. error: error.into(), problem_details: None, } @@ -504,7 +525,7 @@ impl From for WrappedReqwestError { impl From for WrappedReqwestError { fn from(error: reqwest_middleware::Error) -> Self { Self { - error, + error: Self::filter_retries_from_error(error), problem_details: None, } } diff --git a/crates/uv-python/src/downloads.rs b/crates/uv-python/src/downloads.rs index f500d3e1d..9c687e688 100644 --- a/crates/uv-python/src/downloads.rs +++ b/crates/uv-python/src/downloads.rs @@ -131,14 +131,11 @@ impl Error { if let Self::NetworkErrorWithRetries { retries, .. } = self { return retries + 1; } - // TODO(jack): let-chains are stable as of Rust 1.88. We should use them here as soon as - // our rust-version is high enough. - if let Self::NetworkMiddlewareError(_, anyhow_error) = self { - if let Some(RetryError::WithRetries { retries, .. }) = + if let Self::NetworkMiddlewareError(_, anyhow_error) = self + && let Some(RetryError::WithRetries { retries, .. }) = anyhow_error.downcast_ref::() - { - return retries + 1; - } + { + return retries + 1; } 1 } diff --git a/crates/uv/tests/it/network.rs b/crates/uv/tests/it/network.rs index 991ad27c3..17e964617 100644 --- a/crates/uv/tests/it/network.rs +++ b/crates/uv/tests/it/network.rs @@ -83,8 +83,8 @@ async fn simple_io_err() { ----- stdout ----- ----- stderr ----- - error: Failed to fetch: `[SERVER]/tqdm/` - Caused by: Request failed after 3 retries + error: Request failed after 3 retries + Caused by: Failed to fetch: `[SERVER]/tqdm/` Caused by: error sending request for url ([SERVER]/tqdm/) Caused by: client error (SendRequest) Caused by: connection closed before message completed @@ -141,8 +141,8 @@ async fn find_links_io_error() { ----- stderr ----- error: Failed to read `--find-links` URL: [SERVER]/ - Caused by: Failed to fetch: `[SERVER]/` Caused by: Request failed after 3 retries + Caused by: Failed to fetch: `[SERVER]/` Caused by: error sending request for url ([SERVER]/) Caused by: client error (SendRequest) Caused by: connection closed before message completed @@ -200,8 +200,8 @@ async fn direct_url_io_error() { ----- stderr ----- × Failed to download `tqdm @ [SERVER]/packages/d0/30/dc54f88dd4a2b5dc8a0279bdd7270e735851848b762aeb1c1184ed1f6b14/tqdm-4.67.1-py3-none-any.whl` - ├─▶ Failed to fetch: `[SERVER]/packages/d0/30/dc54f88dd4a2b5dc8a0279bdd7270e735851848b762aeb1c1184ed1f6b14/tqdm-4.67.1-py3-none-any.whl` ├─▶ Request failed after 3 retries + ├─▶ Failed to fetch: `[SERVER]/packages/d0/30/dc54f88dd4a2b5dc8a0279bdd7270e735851848b762aeb1c1184ed1f6b14/tqdm-4.67.1-py3-none-any.whl` ├─▶ error sending request for url ([SERVER]/packages/d0/30/dc54f88dd4a2b5dc8a0279bdd7270e735851848b762aeb1c1184ed1f6b14/tqdm-4.67.1-py3-none-any.whl) ├─▶ client error (SendRequest) ╰─▶ connection closed before message completed