Skip to content

Commit

Permalink
Treat resolver failures as fatal in lockfile validation (#8083)
Browse files Browse the repository at this point in the history
## 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 #8074.
  • Loading branch information
charliermarsh authored Oct 10, 2024
1 parent dc3f628 commit 7bac708
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 23 deletions.
32 changes: 22 additions & 10 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -3758,6 +3754,13 @@ fn normalize_requirement(
#[error(transparent)]
pub struct LockError(Box<LockErrorKind>);

impl LockError {
/// Returns true if the [`LockError`] is a resolver error.
pub fn is_resolution(&self) -> bool {
matches!(&*self.0, LockErrorKind::Resolution { .. })
}
}

impl<E> From<E> for LockError
where
LockErrorKind: From<E>,
Expand Down Expand Up @@ -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}`")]
Expand Down
21 changes: 9 additions & 12 deletions crates/uv/src/commands/project/lock.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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
Expand All @@ -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),
Expand Down Expand Up @@ -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: {:?}",
Expand Down
2 changes: 1 addition & 1 deletion crates/uv/tests/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`
"###);

Expand Down

0 comments on commit 7bac708

Please sign in to comment.