From 785595bd35790154098beab422662862d655adde Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 31 Jul 2025 17:00:01 -0400 Subject: [PATCH] Remove retry wrapper when matching on error kind (#14996) ## Summary We often match on `ErrorKind` to figure out how to handle an error (e.g., to treat a 404 as "Not found" rather than aborting the program). Unfortunately, if we retry, we wrap the error in a new kind that includes the retry count. This PR adds an unwrapping mechanism to ensure that callers always look at the underlying error. Closes https://github.com/astral-sh/uv/issues/14941. Closes https://github.com/astral-sh/uv/issues/14989. --- crates/uv-client/src/cached_client.rs | 12 +---- crates/uv-client/src/error.rs | 50 ++++++++++++++++++--- crates/uv-client/src/registry_client.rs | 12 ++--- crates/uv-publish/src/lib.rs | 4 +- crates/uv-resolver/src/resolver/provider.rs | 35 ++++++++------- crates/uv/src/commands/pip/latest.rs | 4 +- 6 files changed, 77 insertions(+), 40 deletions(-) diff --git a/crates/uv-client/src/cached_client.rs b/crates/uv-client/src/cached_client.rs index 4219decd5..349506325 100644 --- a/crates/uv-client/src/cached_client.rs +++ b/crates/uv-client/src/cached_client.rs @@ -176,20 +176,12 @@ impl + std::error::Error + 'static> From> for CachedClientError::Client { retries: Some(retries), err, - } => ErrorKind::RequestWithRetries { - source: Box::new(err.into_kind()), - retries, - } - .into(), + } => Error::new(err.into_kind(), retries), CachedClientError::Client { retries: None, err } => err, CachedClientError::Callback { retries: Some(retries), err, - } => ErrorKind::RequestWithRetries { - source: Box::new(err.into().into_kind()), - retries, - } - .into(), + } => Error::new(err.into().into_kind(), retries), CachedClientError::Callback { retries: None, err } => err.into(), } } diff --git a/crates/uv-client/src/error.rs b/crates/uv-client/src/error.rs index 035cdea71..e00d1a952 100644 --- a/crates/uv-client/src/error.rs +++ b/crates/uv-client/src/error.rs @@ -11,19 +11,56 @@ use uv_redacted::DisplaySafeUrl; use crate::middleware::OfflineError; use crate::{FlatIndexError, html}; -#[derive(Debug, thiserror::Error)] -#[error(transparent)] +#[derive(Debug)] pub struct Error { kind: Box, + retries: u32, +} + +impl Display for Error { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + if self.retries > 0 { + write!( + f, + "Request failed after {retries} retries", + retries = self.retries + ) + } else { + Display::fmt(&self.kind, f) + } + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + if self.retries > 0 { + Some(&self.kind) + } else { + self.kind.source() + } + } } impl Error { - /// Convert this error into its [`ErrorKind`] variant. + /// Create a new [`Error`] with the given [`ErrorKind`] and number of retries. + pub fn new(kind: ErrorKind, retries: u32) -> Self { + Self { + kind: Box::new(kind), + retries, + } + } + + /// Return the number of retries that were attempted before this error was returned. + pub fn retries(&self) -> u32 { + self.retries + } + + /// Convert this error into an [`ErrorKind`]. pub fn into_kind(self) -> ErrorKind { *self.kind } - /// Get a reference to the [`ErrorKind`] variant of this error. + /// Return the [`ErrorKind`] of this error. pub fn kind(&self) -> &ErrorKind { &self.kind } @@ -78,7 +115,7 @@ impl Error { // The server returned a "Method Not Allowed" error, indicating it doesn't support // HEAD requests, so we can't check for range requests. - ErrorKind::WrappedReqwestError(_url, err) => { + ErrorKind::WrappedReqwestError(_, err) => { if let Some(status) = err.status() { // If the server doesn't support HEAD requests, we can't check for range // requests. @@ -143,6 +180,7 @@ impl From for Error { fn from(kind: ErrorKind) -> Self { Self { kind: Box::new(kind), + retries: 0, } } } @@ -265,10 +303,12 @@ pub enum ErrorKind { } impl ErrorKind { + /// Create an [`ErrorKind`] from a [`reqwest::Error`]. pub(crate) fn from_reqwest(url: DisplaySafeUrl, error: reqwest::Error) -> Self { Self::WrappedReqwestError(url, WrappedReqwestError::from(error)) } + /// Create an [`ErrorKind`] from a [`reqwest_middleware::Error`]. pub(crate) fn from_reqwest_middleware( url: DisplaySafeUrl, err: reqwest_middleware::Error, diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs index c21aa3b31..73932dbd4 100644 --- a/crates/uv-client/src/registry_client.rs +++ b/crates/uv-client/src/registry_client.rs @@ -549,11 +549,11 @@ impl RegistryClient { match result { Ok(metadata) => Ok(SimpleMetadataSearchOutcome::Found(metadata)), - Err(err) => match err.into_kind() { + Err(err) => match err.kind() { // The package could not be found in the remote index. - ErrorKind::WrappedReqwestError(url, err) => { - let Some(status_code) = err.status() else { - return Err(ErrorKind::WrappedReqwestError(url, err).into()); + ErrorKind::WrappedReqwestError(.., reqwest_err) => { + let Some(status_code) = reqwest_err.status() else { + return Err(err); }; let decision = status_code_strategy.handle_status_code(status_code, index, capabilities); @@ -562,7 +562,7 @@ impl RegistryClient { status_code, StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN ) { - return Err(ErrorKind::WrappedReqwestError(url, err).into()); + return Err(err); } } Ok(SimpleMetadataSearchOutcome::from(decision)) @@ -574,7 +574,7 @@ impl RegistryClient { // The package could not be found in the local index. ErrorKind::FileNotFound(_) => Ok(SimpleMetadataSearchOutcome::NotFound), - err => Err(err.into()), + _ => Err(err), }, } } diff --git a/crates/uv-publish/src/lib.rs b/crates/uv-publish/src/lib.rs index f3dc768c6..df97a1c10 100644 --- a/crates/uv-publish/src/lib.rs +++ b/crates/uv-publish/src/lib.rs @@ -501,7 +501,7 @@ pub async fn check_url( { Ok(response) => response, Err(err) => { - return match err.into_kind() { + return match err.kind() { uv_client::ErrorKind::PackageNotFound(_) => { // The package doesn't exist, so we can't have uploaded it. warn!( @@ -509,7 +509,7 @@ pub async fn check_url( ); Ok(false) } - kind => Err(PublishError::CheckUrlIndex(kind.into())), + _ => Err(PublishError::CheckUrlIndex(err)), }; } }; diff --git a/crates/uv-resolver/src/resolver/provider.rs b/crates/uv-resolver/src/resolver/provider.rs index 18a901604..b06067df5 100644 --- a/crates/uv-resolver/src/resolver/provider.rs +++ b/crates/uv-resolver/src/resolver/provider.rs @@ -199,7 +199,7 @@ impl ResolverProvider for DefaultResolverProvider<'_, Con }) .collect(), )), - Err(err) => match err.into_kind() { + Err(err) => match err.kind() { uv_client::ErrorKind::PackageNotFound(_) => { if let Some(flat_index) = flat_index .and_then(|flat_index| flat_index.get(package_name)) @@ -232,7 +232,7 @@ impl ResolverProvider for DefaultResolverProvider<'_, Con Ok(VersionsResponse::Offline) } } - kind => Err(kind.into()), + _ => Err(err), }, } } @@ -246,20 +246,25 @@ impl ResolverProvider for DefaultResolverProvider<'_, Con { Ok(metadata) => Ok(MetadataResponse::Found(metadata)), Err(err) => match err { - uv_distribution::Error::Client(client) => match client.into_kind() { - uv_client::ErrorKind::Offline(_) => { - Ok(MetadataResponse::Unavailable(MetadataUnavailable::Offline)) + uv_distribution::Error::Client(client) => { + let retries = client.retries(); + match client.into_kind() { + uv_client::ErrorKind::Offline(_) => { + Ok(MetadataResponse::Unavailable(MetadataUnavailable::Offline)) + } + uv_client::ErrorKind::MetadataParseError(_, _, err) => { + Ok(MetadataResponse::Unavailable( + MetadataUnavailable::InvalidMetadata(Arc::new(*err)), + )) + } + uv_client::ErrorKind::Metadata(_, err) => { + Ok(MetadataResponse::Unavailable( + MetadataUnavailable::InvalidStructure(Arc::new(err)), + )) + } + kind => Err(uv_client::Error::new(kind, retries).into()), } - uv_client::ErrorKind::MetadataParseError(_, _, err) => { - Ok(MetadataResponse::Unavailable( - MetadataUnavailable::InvalidMetadata(Arc::new(*err)), - )) - } - uv_client::ErrorKind::Metadata(_, err) => Ok(MetadataResponse::Unavailable( - MetadataUnavailable::InvalidStructure(Arc::new(err)), - )), - kind => Err(uv_client::Error::from(kind).into()), - }, + } uv_distribution::Error::WheelMetadataVersionMismatch { .. } => { Ok(MetadataResponse::Unavailable( MetadataUnavailable::InconsistentMetadata(Arc::new(err)), diff --git a/crates/uv/src/commands/pip/latest.rs b/crates/uv/src/commands/pip/latest.rs index 87e26f3f5..c72903ad3 100644 --- a/crates/uv/src/commands/pip/latest.rs +++ b/crates/uv/src/commands/pip/latest.rs @@ -45,11 +45,11 @@ impl LatestClient<'_> { { Ok(archives) => archives, Err(err) => { - return match err.into_kind() { + return match err.kind() { uv_client::ErrorKind::PackageNotFound(_) => Ok(None), uv_client::ErrorKind::NoIndex(_) => Ok(None), uv_client::ErrorKind::Offline(_) => Ok(None), - kind => Err(kind.into()), + _ => Err(err), }; } };