From 7bac708b97bcd032fa8b03cf34b45938f4933469 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 10 Oct 2024 16:01:20 +0200 Subject: [PATCH] Treat resolver failures as fatal in lockfile validation (#8083) ## Summary In the routine we use to verify whether the lockfile is up-to-date, we sometimes have to resolve package metadata. If that resolution step fails, the resolver is left in a bad state, as various tasks are marked as pending despite the error. Treating that as a recoverable failure thus leads to a deadlock. This PR modifies the errors to be treated as fatal. I think a more holistic fix here would be to add some kind of guard to ensure that any tasks that fail are no longer marked as pending (or enforce this in the type system). Closes https://github.com/astral-sh/uv/issues/8074. --- crates/uv-resolver/src/lock/mod.rs | 32 ++++++++++++++++++-------- crates/uv/src/commands/project/lock.rs | 21 ++++++++--------- crates/uv/tests/lock.rs | 2 +- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/crates/uv-resolver/src/lock/mod.rs b/crates/uv-resolver/src/lock/mod.rs index ce1be4cd0def..9bf742e403af 100644 --- a/crates/uv-resolver/src/lock/mod.rs +++ b/crates/uv-resolver/src/lock/mod.rs @@ -1158,15 +1158,13 @@ impl Lock { build_options, )?; - let Ok(archive) = database + let archive = database .get_or_build_wheel_metadata(&dist, HashPolicy::None) .await - else { - return Ok(SatisfiesResult::MissingMetadata( - &package.id.name, - &package.id.version, - )); - }; + .map_err(|err| LockErrorKind::Resolution { + id: package.id.clone(), + err, + })?; // Validate the `requires-dist` metadata. { @@ -1312,8 +1310,6 @@ pub enum SatisfiesResult<'lock> { MissingRemoteIndex(&'lock PackageName, &'lock Version, &'lock UrlString), /// The lockfile referenced a local index that was not provided MissingLocalIndex(&'lock PackageName, &'lock Version, &'lock PathBuf), - /// The resolver failed to generate metadata for a given package. - MissingMetadata(&'lock PackageName, &'lock Version), /// A package in the lockfile contains different `requires-dist` metadata than expected. MismatchedRequiresDist( &'lock PackageName, @@ -3758,6 +3754,13 @@ fn normalize_requirement( #[error(transparent)] pub struct LockError(Box); +impl LockError { + /// Returns true if the [`LockError`] is a resolver error. + pub fn is_resolution(&self) -> bool { + matches!(&*self.0, LockErrorKind::Resolution { .. }) + } +} + impl From for LockError where LockErrorKind: From, @@ -4042,10 +4045,19 @@ enum LockErrorKind { /// The ID of the package. name: PackageName, }, + /// An error that occurs when resolving metadata for a package. + #[error("failed to generate package metadata for `{id}`")] + Resolution { + /// The ID of the distribution that failed to resolve. + id: PackageId, + /// The inner error we forward. + #[source] + err: uv_distribution::Error, + }, } /// An error that occurs when a source string could not be parsed. -#[derive(Clone, Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error)] enum SourceParseError { /// An error that occurs when the URL in the source is invalid. #[error("invalid URL in source `{given}`")] diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index 7c6b99fe99f7..aa56cff36202 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -1,13 +1,13 @@ #![allow(clippy::single_match_else)] -use anstream::eprint; -use owo_colors::OwoColorize; -use rustc_hash::{FxBuildHasher, FxHashMap}; use std::collections::BTreeSet; use std::fmt::Write; use std::path::Path; -use tracing::debug; +use anstream::eprint; +use owo_colors::OwoColorize; +use rustc_hash::{FxBuildHasher, FxHashMap}; +use tracing::debug; use uv_auth::store_credentials_from_url; use uv_cache::Cache; use uv_client::{Connectivity, FlatIndexClient, RegistryClientBuilder}; @@ -468,6 +468,11 @@ async fn do_lock( .await { Ok(result) => Some(result), + Err(ProjectError::Lock(err)) if err.is_resolution() => { + // Resolver errors are not recoverable, as such errors can leave the resolver in a + // broken state. Specifically, tasks that fail with an error can be left as pending. + return Err(ProjectError::Lock(err)); + } Err(err) => { warn_user!("Failed to validate existing lockfile: {err}"); None @@ -489,8 +494,6 @@ async fn do_lock( // The lockfile did not contain enough information to obtain a resolution, fallback // to a fresh resolve. _ => { - debug!("Starting clean resolution"); - // Determine whether we can reuse the existing package versions. let versions_lock = existing_lock.as_ref().and_then(|lock| match &lock { ValidatedLock::Satisfies(lock) => Some(lock), @@ -847,12 +850,6 @@ impl ValidatedLock { ); Ok(Self::Preferable(lock)) } - SatisfiesResult::MissingMetadata(name, version) => { - debug!( - "Ignoring existing lockfile due to missing metadata for: `{name}=={version}`" - ); - Ok(Self::Preferable(lock)) - } SatisfiesResult::MismatchedRequiresDist(name, version, expected, actual) => { debug!( "Ignoring existing lockfile due to mismatched `requires-dist` for: `{name}=={version}`\n Expected: {:?}\n Actual: {:?}", diff --git a/crates/uv/tests/lock.rs b/crates/uv/tests/lock.rs index 7f1ff8659b51..2752d686bb5d 100644 --- a/crates/uv/tests/lock.rs +++ b/crates/uv/tests/lock.rs @@ -1069,7 +1069,7 @@ fn lock_wheel_url() -> Result<()> { ----- stdout ----- ----- stderr ----- - error: Failed to download: `anyio @ https://files.pythonhosted.org/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl` + error: failed to generate package metadata for `anyio==4.3.0 @ direct+https://files.pythonhosted.org/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl` Caused by: Network connectivity is disabled, but the requested data wasn't found in the cache for: `https://files.pythonhosted.org/packages/14/fd/2f20c40b45e4fb4324834aea24bd4afdf1143390242c0b33774da0e2e34f/anyio-4.3.0-py3-none-any.whl` "###);