diff --git a/Cargo.lock b/Cargo.lock index b57a35d49..9daf3d3ec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3358,21 +3358,21 @@ dependencies = [ [[package]] name = "reqwest-middleware" version = "0.4.2" -source = "git+https://github.com/astral-sh/reqwest-middleware?rev=ad8b9d332d1773fde8b4cd008486de5973e0a3f8#ad8b9d332d1773fde8b4cd008486de5973e0a3f8" +source = "git+https://github.com/astral-sh/reqwest-middleware?rev=7650ed76215a962a96d94a79be71c27bffde7ab2#7650ed76215a962a96d94a79be71c27bffde7ab2" dependencies = [ "anyhow", "async-trait", "http", "reqwest", "serde", - "thiserror 1.0.69", + "thiserror 2.0.16", "tower-service", ] [[package]] name = "reqwest-retry" version = "0.7.0" -source = "git+https://github.com/astral-sh/reqwest-middleware?rev=ad8b9d332d1773fde8b4cd008486de5973e0a3f8#ad8b9d332d1773fde8b4cd008486de5973e0a3f8" +source = "git+https://github.com/astral-sh/reqwest-middleware?rev=7650ed76215a962a96d94a79be71c27bffde7ab2#7650ed76215a962a96d94a79be71c27bffde7ab2" dependencies = [ "anyhow", "async-trait", @@ -3383,7 +3383,7 @@ dependencies = [ "reqwest", "reqwest-middleware", "retry-policies", - "thiserror 1.0.69", + "thiserror 2.0.16", "tokio", "tracing", "wasmtimer", @@ -4990,6 +4990,7 @@ dependencies = [ "predicates", "regex", "reqwest", + "reqwest-retry", "rkyv", "rustc-hash", "self-replace", diff --git a/Cargo.toml b/Cargo.toml index 36b81d30e..c56753d4a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -149,8 +149,8 @@ reflink-copy = { version = "0.1.19" } regex = { version = "1.10.6" } regex-automata = { version = "0.4.8", default-features = false, features = ["dfa-build", "dfa-search", "perf", "std", "syntax"] } reqwest = { version = "0.12.22", default-features = false, features = ["json", "gzip", "deflate", "zstd", "stream", "system-proxy", "rustls-tls", "rustls-tls-native-roots", "socks", "multipart", "http2", "blocking"] } -reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "ad8b9d332d1773fde8b4cd008486de5973e0a3f8", features = ["multipart"] } -reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "ad8b9d332d1773fde8b4cd008486de5973e0a3f8" } +reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "7650ed76215a962a96d94a79be71c27bffde7ab2", features = ["multipart"] } +reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "7650ed76215a962a96d94a79be71c27bffde7ab2" } rkyv = { version = "0.8.8", features = ["bytecheck"] } rmp-serde = { version = "1.3.0" } rust-netrc = { version = "0.1.2" } @@ -322,5 +322,5 @@ codegen-units = 1 inherits = "release" [patch.crates-io] -reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "ad8b9d332d1773fde8b4cd008486de5973e0a3f8" } -reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "ad8b9d332d1773fde8b4cd008486de5973e0a3f8" } +reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "7650ed76215a962a96d94a79be71c27bffde7ab2" } +reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "7650ed76215a962a96d94a79be71c27bffde7ab2" } diff --git a/crates/uv-bin-install/src/lib.rs b/crates/uv-bin-install/src/lib.rs index 5e6857245..c263fa82c 100644 --- a/crates/uv-bin-install/src/lib.rs +++ b/crates/uv-bin-install/src/lib.rs @@ -10,6 +10,7 @@ use std::time::{Duration, SystemTime}; use futures::TryStreamExt; use reqwest_retry::RetryPolicy; +use reqwest_retry::policies::ExponentialBackoff; use std::fmt; use thiserror::Error; use tokio::io::{AsyncRead, ReadBuf}; @@ -19,7 +20,7 @@ use url::Url; use uv_distribution_filename::SourceDistExtension; use uv_cache::{Cache, CacheBucket, CacheEntry}; -use uv_client::{BaseClient, is_extended_transient_error}; +use uv_client::{BaseClient, is_transient_network_error}; use uv_extract::{Error as ExtractError, stream}; use uv_pep440::Version; use uv_platform::Platform; @@ -160,6 +161,7 @@ pub async fn bin_install( binary: Binary, version: &Version, client: &BaseClient, + retry_policy: &ExponentialBackoff, cache: &Cache, reporter: &dyn Reporter, ) -> Result { @@ -195,6 +197,7 @@ pub async fn bin_install( binary, version, client, + retry_policy, cache, reporter, &platform_name, @@ -227,6 +230,7 @@ async fn download_and_unpack_with_retry( binary: Binary, version: &Version, client: &BaseClient, + retry_policy: &ExponentialBackoff, cache: &Cache, reporter: &dyn Reporter, platform_name: &str, @@ -237,7 +241,6 @@ async fn download_and_unpack_with_retry( let mut total_attempts = 0; let mut retried_here = false; let start_time = SystemTime::now(); - let retry_policy = client.retry_policy(); loop { let result = download_and_unpack( @@ -259,7 +262,7 @@ async fn download_and_unpack_with_retry( total_attempts += err.attempts(); let past_retries = total_attempts - 1; - if is_extended_transient_error(&err) { + if is_transient_network_error(&err) { let retry_decision = retry_policy.should_retry(start_time, past_retries); if let reqwest_retry::RetryDecision::Retry { execute_after } = retry_decision { debug!( diff --git a/crates/uv-client/src/base_client.rs b/crates/uv-client/src/base_client.rs index 8c05383e1..53a2da0f1 100644 --- a/crates/uv-client/src/base_client.rs +++ b/crates/uv-client/src/base_client.rs @@ -21,6 +21,7 @@ use reqwest_middleware::{ClientWithMiddleware, Middleware}; use reqwest_retry::policies::ExponentialBackoff; use reqwest_retry::{ DefaultRetryableStrategy, RetryTransientMiddleware, Retryable, RetryableStrategy, + default_on_request_error, }; use thiserror::Error; use tracing::{debug, trace}; @@ -38,10 +39,10 @@ use uv_static::EnvVars; use uv_version::version; use uv_warnings::warn_user_once; -use crate::Connectivity; use crate::linehaul::LineHaul; use crate::middleware::OfflineMiddleware; use crate::tls::read_identity; +use crate::{Connectivity, WrappedReqwestError}; /// Do not use this value directly outside tests, use [`retries_from_env`] instead. pub const DEFAULT_RETRIES: u32 = 3; @@ -916,7 +917,7 @@ impl RetryableStrategy for UvRetryableStrategy { None | Some(Retryable::Fatal) if res .as_ref() - .is_err_and(|err| is_extended_transient_error(err)) => + .is_err_and(|err| is_transient_network_error(err)) => { Some(Retryable::Transient) } @@ -944,12 +945,15 @@ impl RetryableStrategy for UvRetryableStrategy { } } -/// Check for additional transient error kinds not supported by the default retry strategy in `reqwest_retry`. +/// Whether the error looks like a network error that should be retried. /// -/// These cases should be safe to retry with [`Retryable::Transient`]. -pub fn is_extended_transient_error(err: &dyn Error) -> bool { +/// There are two cases that the default retry strategy is missing: +/// * Inside the 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 { // First, try to show a nice trace log - if let Some((Some(status), Some(url))) = find_source::(&err) + if let Some((Some(status), Some(url))) = find_source::(&err) .map(|request_err| (request_err.status(), request_err.url())) { trace!("Considering retry of response HTTP {status} for {url}"); @@ -957,38 +961,83 @@ pub fn is_extended_transient_error(err: &dyn Error) -> bool { trace!("Considering retry of error: {err:?}"); } - // IO Errors may be nested through custom IO errors. - let mut has_io_error = false; - for io_err in find_sources::(&err) { - has_io_error = true; - let retryable_io_err_kinds = [ - // https://github.com/astral-sh/uv/issues/12054 - io::ErrorKind::BrokenPipe, - // From reqwest-middleware - io::ErrorKind::ConnectionAborted, - // https://github.com/astral-sh/uv/issues/3514 - io::ErrorKind::ConnectionReset, - // https://github.com/astral-sh/uv/issues/14699 - io::ErrorKind::InvalidData, - // https://github.com/astral-sh/uv/issues/9246 - io::ErrorKind::UnexpectedEof, - ]; - if retryable_io_err_kinds.contains(&io_err.kind()) { - trace!("Retrying error: `{}`", io_err.kind()); - return true; + let mut has_known_error = false; + // IO Errors or reqwest errors may be nested through custom IO errors or stream processing + // crates + let mut current_source = err.source(); + 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; + } + } + + trace!("Cannot retry nested reqwest middleware error"); + } else if let Some(reqwest_err) = source.downcast_ref::() { + has_known_error = true; + if default_on_request_error(reqwest_err) == Some(Retryable::Transient) { + trace!("Retrying nested reqwest error"); + return true; + } + if is_retryable_status_error(reqwest_err) { + trace!("Retrying nested reqwest status code error"); + return true; + } + + trace!("Cannot retry nested reqwest error"); + } else if let Some(io_err) = source.downcast_ref::() { + has_known_error = true; + let retryable_io_err_kinds = [ + // https://github.com/astral-sh/uv/issues/12054 + io::ErrorKind::BrokenPipe, + // From reqwest-middleware + io::ErrorKind::ConnectionAborted, + // https://github.com/astral-sh/uv/issues/3514 + io::ErrorKind::ConnectionReset, + // https://github.com/astral-sh/uv/issues/14699 + io::ErrorKind::InvalidData, + // https://github.com/astral-sh/uv/issues/9246 + io::ErrorKind::UnexpectedEof, + ]; + if retryable_io_err_kinds.contains(&io_err.kind()) { + trace!("Retrying error: `{}`", io_err.kind()); + return true; + } + + trace!( + "Cannot retry IO error `{}`, not a retryable IO error kind", + io_err.kind() + ); } - trace!( - "Cannot retry IO error `{}`, not a retryable IO error kind", - io_err.kind() - ); + + current_source = source.source(); } - if !has_io_error { - trace!("Cannot retry error: not an extended IO error"); + if !has_known_error { + trace!("Cannot retry error: Neither an IO error nor a reqwest error"); } false } +/// Whether the error is a status code error that is retryable. +/// +/// Port of `reqwest_retry::default_on_request_success`. +fn is_retryable_status_error(reqwest_err: &reqwest::Error) -> bool { + let Some(status) = reqwest_err.status() else { + return false; + }; + status.is_server_error() + || status == StatusCode::REQUEST_TIMEOUT + || status == StatusCode::TOO_MANY_REQUESTS +} + /// Find the first source error of a specific type. /// /// See @@ -1003,15 +1052,6 @@ fn find_source(orig: &dyn Error) -> Option<&E> { None } -/// Return all errors in the chain of a specific type. -/// -/// This handles cases such as nested `io::Error`s. -/// -/// See -fn find_sources(orig: &dyn Error) -> impl Iterator { - iter::successors(find_source::(orig), |&err| find_source(err)) -} - // TODO(konsti): Remove once we find a native home for `retries_from_env` #[derive(Debug, Error)] pub enum RetryParsingError { diff --git a/crates/uv-client/src/cached_client.rs b/crates/uv-client/src/cached_client.rs index 127b74f4f..59b32184f 100644 --- a/crates/uv-client/src/cached_client.rs +++ b/crates/uv-client/src/cached_client.rs @@ -14,7 +14,7 @@ use uv_fs::write_atomic; use uv_redacted::DisplaySafeUrl; use crate::BaseClient; -use crate::base_client::is_extended_transient_error; +use crate::base_client::is_transient_network_error; use crate::{ Error, ErrorKind, httpcache::{AfterResponse, BeforeRequest, CachePolicy, CachePolicyBuilder}, @@ -141,7 +141,7 @@ impl CachedClientError &dyn std::error::Error { + fn error(&self) -> &(dyn std::error::Error + 'static) { match self { Self::Client { err, .. } => err, Self::Callback { err, .. } => err, @@ -452,7 +452,8 @@ impl CachedClient { .await } - #[instrument(name="read_and_parse_cache", skip_all, fields(file = %cache_entry.path().display()))] + #[instrument(name = "read_and_parse_cache", skip_all, fields(file = %cache_entry.path().display() + ))] async fn read_cache(cache_entry: &CacheEntry) -> Option { match DataWithCachePolicy::from_path_async(cache_entry.path()).await { Ok(data) => Some(data), @@ -680,7 +681,7 @@ impl CachedClient { if result .as_ref() - .is_err_and(|err| is_extended_transient_error(err.error())) + .is_err_and(|err| is_transient_network_error(err.error())) { // If middleware already retried, consider that in our retry budget let total_retries = past_retries + middleware_retries; @@ -739,7 +740,7 @@ impl CachedClient { if result .as_ref() .err() - .is_some_and(|err| is_extended_transient_error(err.error())) + .is_some_and(|err| is_transient_network_error(err.error())) { let total_retries = past_retries + middleware_retries; let retry_decision = retry_policy.should_retry(start_time, total_retries); diff --git a/crates/uv-client/src/lib.rs b/crates/uv-client/src/lib.rs index f877afb2b..44e333efc 100644 --- a/crates/uv-client/src/lib.rs +++ b/crates/uv-client/src/lib.rs @@ -1,7 +1,7 @@ pub use base_client::{ AuthIntegration, BaseClient, BaseClientBuilder, DEFAULT_RETRIES, ExtraMiddleware, RedirectClientWithMiddleware, RequestBuilder, RetryParsingError, UvRetryableStrategy, - is_extended_transient_error, retries_from_env, + is_transient_network_error, retries_from_env, }; pub use cached_client::{CacheControl, CachedClient, CachedClientError, DataWithCachePolicy}; pub use error::{Error, ErrorKind, WrappedReqwestError}; diff --git a/crates/uv-python/src/downloads.rs b/crates/uv-python/src/downloads.rs index 372d18214..f9ee7b5e5 100644 --- a/crates/uv-python/src/downloads.rs +++ b/crates/uv-python/src/downloads.rs @@ -12,6 +12,7 @@ use futures::TryStreamExt; use itertools::Itertools; use once_cell::sync::OnceCell; use owo_colors::OwoColorize; +use reqwest_retry::policies::ExponentialBackoff; use reqwest_retry::{RetryError, RetryPolicy}; use serde::Deserialize; use thiserror::Error; @@ -21,7 +22,7 @@ use tokio_util::either::Either; use tracing::{debug, instrument}; use url::Url; -use uv_client::{BaseClient, WrappedReqwestError, is_extended_transient_error}; +use uv_client::{BaseClient, WrappedReqwestError, is_transient_network_error}; use uv_distribution_filename::{ExtensionError, SourceDistExtension}; use uv_extract::hash::Hasher; use uv_fs::{Simplified, rename_with_retry}; @@ -930,6 +931,7 @@ impl ManagedPythonDownload { pub async fn fetch_with_retry( &self, client: &BaseClient, + retry_policy: &ExponentialBackoff, installation_dir: &Path, scratch_dir: &Path, reinstall: bool, @@ -940,7 +942,6 @@ impl ManagedPythonDownload { let mut total_attempts = 0; let mut retried_here = false; let start_time = SystemTime::now(); - let retry_policy = client.retry_policy(); loop { let result = self .fetch( @@ -961,7 +962,7 @@ impl ManagedPythonDownload { total_attempts += err.attempts(); // We currently interpret e.g. "3 retries" to mean we should make 4 attempts. let n_past_retries = total_attempts - 1; - if is_extended_transient_error(&err) { + if is_transient_network_error(&err) { let retry_decision = retry_policy.should_retry(start_time, n_past_retries); if let reqwest_retry::RetryDecision::Retry { execute_after } = retry_decision diff --git a/crates/uv-python/src/installation.rs b/crates/uv-python/src/installation.rs index 20b0a8da8..31d029317 100644 --- a/crates/uv-python/src/installation.rs +++ b/crates/uv-python/src/installation.rs @@ -5,10 +5,11 @@ use std::str::FromStr; use indexmap::IndexMap; use ref_cast::RefCast; +use reqwest_retry::policies::ExponentialBackoff; use tracing::{debug, info}; use uv_cache::Cache; -use uv_client::BaseClientBuilder; +use uv_client::{BaseClientBuilder, retries_from_env}; use uv_pep440::{Prerelease, Version}; use uv_platform::{Arch, Libc, Os, Platform}; use uv_preview::Preview; @@ -228,12 +229,17 @@ impl PythonInstallation { let scratch_dir = installations.scratch(); let _lock = installations.lock().await?; - let client = client_builder.build(); + // Python downloads are performing their own retries to catch stream errors, disable the + // default retries to avoid the middleware from performing uncontrolled retries. + let retry_policy = + ExponentialBackoff::builder().build_with_max_retries(retries_from_env()?); + let client = client_builder.clone().retries(0).build(); info!("Fetching requested Python..."); let result = download .fetch_with_retry( &client, + &retry_policy, installations_dir, &scratch_dir, false, diff --git a/crates/uv-python/src/lib.rs b/crates/uv-python/src/lib.rs index 9209a23ea..5fbea593e 100644 --- a/crates/uv-python/src/lib.rs +++ b/crates/uv-python/src/lib.rs @@ -99,6 +99,9 @@ pub enum Error { #[error(transparent)] InvalidEnvironment(#[from] environment::InvalidEnvironment), + + #[error(transparent)] + RetryParsing(#[from] uv_client::RetryParsingError), } impl Error { diff --git a/crates/uv/Cargo.toml b/crates/uv/Cargo.toml index 9058f8403..48cd8a93a 100644 --- a/crates/uv/Cargo.toml +++ b/crates/uv/Cargo.toml @@ -88,6 +88,7 @@ owo-colors = { workspace = true } petgraph = { workspace = true } regex = { workspace = true } reqwest = { workspace = true } +reqwest-retry = { workspace = true } rkyv = { workspace = true } rustc-hash = { workspace = true } serde = { workspace = true } diff --git a/crates/uv/src/commands/project/format.rs b/crates/uv/src/commands/project/format.rs index 2d8d60451..baa4e7fb0 100644 --- a/crates/uv/src/commands/project/format.rs +++ b/crates/uv/src/commands/project/format.rs @@ -2,11 +2,12 @@ use std::path::Path; use std::str::FromStr; use anyhow::{Context, Result}; +use reqwest_retry::policies::ExponentialBackoff; use tokio::process::Command; use uv_bin_install::{Binary, bin_install}; use uv_cache::Cache; -use uv_client::BaseClientBuilder; +use uv_client::{BaseClientBuilder, retries_from_env}; use uv_pep440::Version; use uv_preview::{Preview, PreviewFeatures}; use uv_warnings::warn_user; @@ -45,15 +46,25 @@ pub(crate) async fn format( // Parse version if provided let version = version.as_deref().map(Version::from_str).transpose()?; - let client = client_builder.build(); + // Python downloads are performing their own retries to catch stream errors, disable the + // default retries to avoid the middleware from performing uncontrolled retries. + let retry_policy = ExponentialBackoff::builder().build_with_max_retries(retries_from_env()?); + let client = client_builder.retries(0).build(); // Get the path to Ruff, downloading it if necessary let reporter = BinaryDownloadReporter::single(printer); let default_version = Binary::Ruff.default_version(); let version = version.as_ref().unwrap_or(&default_version); - let ruff_path = bin_install(Binary::Ruff, version, &client, &cache, &reporter) - .await - .with_context(|| format!("Failed to install ruff {version}"))?; + let ruff_path = bin_install( + Binary::Ruff, + version, + &client, + &retry_policy, + &cache, + &reporter, + ) + .await + .with_context(|| format!("Failed to install ruff {version}"))?; let mut command = Command::new(&ruff_path); // Run ruff in the project root diff --git a/crates/uv/src/commands/python/install.rs b/crates/uv/src/commands/python/install.rs index 96a304e5a..05beb7e3c 100644 --- a/crates/uv/src/commands/python/install.rs +++ b/crates/uv/src/commands/python/install.rs @@ -11,9 +11,11 @@ use futures::stream::FuturesUnordered; use indexmap::IndexSet; use itertools::{Either, Itertools}; use owo_colors::{AnsiColors, OwoColorize}; +use reqwest_retry::policies::ExponentialBackoff; use rustc_hash::{FxHashMap, FxHashSet}; use tracing::{debug, trace}; -use uv_client::BaseClientBuilder; + +use uv_client::{BaseClientBuilder, retries_from_env}; use uv_fs::Simplified; use uv_platform::{Arch, Libc}; use uv_preview::{Preview, PreviewFeatures}; @@ -401,8 +403,11 @@ pub(crate) async fn install( .unique_by(|download| download.key()) .collect::>(); - // Download and unpack the Python versions concurrently - let client = client_builder.build(); + // Python downloads are performing their own retries to catch stream errors, disable the + // default retries to avoid the middleware from performing uncontrolled retries. + let retry_policy = ExponentialBackoff::builder().build_with_max_retries(retries_from_env()?); + let client = client_builder.retries(0).build(); + let reporter = PythonDownloadReporter::new(printer, downloads.len() as u64); let mut tasks = FuturesUnordered::new(); @@ -413,6 +418,7 @@ pub(crate) async fn install( download .fetch_with_retry( &client, + &retry_policy, installations_dir, &scratch_dir, reinstall, diff --git a/crates/uv/tests/it/network.rs b/crates/uv/tests/it/network.rs index c27bf5ef2..565fc0104 100644 --- a/crates/uv/tests/it/network.rs +++ b/crates/uv/tests/it/network.rs @@ -293,8 +293,8 @@ async fn python_install_io_error() { ----- stderr ----- error: Failed to install cpython-3.10.0-macos-aarch64-none - Caused by: Failed to download [SERVER]/astral-sh/python-build-standalone/releases/download/20211017/cpython-3.10.0-aarch64-apple-darwin-pgo%2Blto-20211017T1616.tar.zst Caused by: Request failed after 3 retries + Caused by: Failed to download [SERVER]/astral-sh/python-build-standalone/releases/download/20211017/cpython-3.10.0-aarch64-apple-darwin-pgo%2Blto-20211017T1616.tar.zst Caused by: error sending request for url ([SERVER]/astral-sh/python-build-standalone/releases/download/20211017/cpython-3.10.0-aarch64-apple-darwin-pgo%2Blto-20211017T1616.tar.zst) Caused by: client error (SendRequest) Caused by: connection closed before message completed