From 04f20169dbc28d1648d298e009eb031ae6dc10f4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 2 Mar 2025 18:39:17 -0800 Subject: [PATCH] Avoid silently dropping errors in directory enumeration (#11890) ## Summary Right now, _all_ errors are dropped here, which seems wrong. We should only return an empty iterator if the directory doesn't exist. --- crates/uv-cache/src/lib.rs | 20 ++++---- .../src/index/built_wheel_index.rs | 2 +- .../src/index/registry_wheel_index.rs | 6 +-- .../src/source/built_wheel_metadata.rs | 11 +++-- crates/uv-distribution/src/source/mod.rs | 8 ++++ crates/uv-fs/src/lib.rs | 46 +++++++++++-------- crates/uv-tool/src/lib.rs | 2 +- crates/uv/src/commands/python/uninstall.rs | 4 +- crates/uv/src/commands/tool/uninstall.rs | 4 +- 9 files changed, 62 insertions(+), 41 deletions(-) diff --git a/crates/uv-cache/src/lib.rs b/crates/uv-cache/src/lib.rs index aeabeca55..e1456b7a1 100644 --- a/crates/uv-cache/src/lib.rs +++ b/crates/uv-cache/src/lib.rs @@ -1027,14 +1027,14 @@ impl CacheBucket { // For alternate indices, we expect a directory for every index (under an `index` // subdirectory), followed by a directory per package (indexed by name). let root = cache.bucket(self).join(WheelCacheKind::Index); - for directory in directories(root) { + for directory in directories(root)? { summary += rm_rf(directory.join(name.to_string()))?; } // For direct URLs, we expect a directory for every URL, followed by a // directory per package (indexed by name). let root = cache.bucket(self).join(WheelCacheKind::Url); - for directory in directories(root) { + for directory in directories(root)? { summary += rm_rf(directory.join(name.to_string()))?; } } @@ -1046,7 +1046,7 @@ impl CacheBucket { // For alternate indices, we expect a directory for every index (under an `index` // subdirectory), followed by a directory per package (indexed by name). let root = cache.bucket(self).join(WheelCacheKind::Index); - for directory in directories(root) { + for directory in directories(root)? { summary += rm_rf(directory.join(name.to_string()))?; } @@ -1054,8 +1054,8 @@ impl CacheBucket { // directory per version. To determine whether the URL is relevant, we need to // search for a wheel matching the package name. let root = cache.bucket(self).join(WheelCacheKind::Url); - for url in directories(root) { - if directories(&url).any(|version| is_match(&version, name)) { + for url in directories(root)? { + if directories(&url)?.any(|version| is_match(&version, name)) { summary += rm_rf(url)?; } } @@ -1064,8 +1064,8 @@ impl CacheBucket { // directory per version. To determine whether the path is relevant, we need to // search for a wheel matching the package name. let root = cache.bucket(self).join(WheelCacheKind::Path); - for path in directories(root) { - if directories(&path).any(|version| is_match(&version, name)) { + for path in directories(root)? { + if directories(&path)?.any(|version| is_match(&version, name)) { summary += rm_rf(path)?; } } @@ -1074,8 +1074,8 @@ impl CacheBucket { // directory for every SHA. To determine whether the SHA is relevant, we need to // search for a wheel matching the package name. let root = cache.bucket(self).join(WheelCacheKind::Git); - for repository in directories(root) { - for sha in directories(repository) { + for repository in directories(root)? { + for sha in directories(repository)? { if is_match(&sha, name) { summary += rm_rf(sha)?; } @@ -1090,7 +1090,7 @@ impl CacheBucket { // For alternate indices, we expect a directory for every index (under an `index` // subdirectory), followed by a directory per package (indexed by name). let root = cache.bucket(self).join(WheelCacheKind::Index); - for directory in directories(root) { + for directory in directories(root)? { summary += rm_rf(directory.join(format!("{name}.rkyv")))?; } } diff --git a/crates/uv-distribution/src/index/built_wheel_index.rs b/crates/uv-distribution/src/index/built_wheel_index.rs index 9e5551d59..06a347e8b 100644 --- a/crates/uv-distribution/src/index/built_wheel_index.rs +++ b/crates/uv-distribution/src/index/built_wheel_index.rs @@ -203,7 +203,7 @@ impl<'a> BuiltWheelIndex<'a> { let mut candidate: Option = None; // Unzipped wheels are stored as symlinks into the archive directory. - for wheel_dir in uv_fs::entries(shard) { + for wheel_dir in uv_fs::entries(shard).ok().into_iter().flatten() { // Ignore any `.lock` files. if wheel_dir .extension() diff --git a/crates/uv-distribution/src/index/registry_wheel_index.rs b/crates/uv-distribution/src/index/registry_wheel_index.rs index 09fc9b794..256b44d19 100644 --- a/crates/uv-distribution/src/index/registry_wheel_index.rs +++ b/crates/uv-distribution/src/index/registry_wheel_index.rs @@ -103,7 +103,7 @@ impl<'a> RegistryWheelIndex<'a> { // For registry wheels, the cache structure is: `//.http` // or `///.rev`. - for file in files(&wheel_dir) { + for file in files(&wheel_dir).ok().into_iter().flatten() { match index.url() { // Add files from remote registries. IndexUrl::Pypi(_) | IndexUrl::Url(_) => { @@ -170,7 +170,7 @@ impl<'a> RegistryWheelIndex<'a> { ); // For registry source distributions, the cache structure is: `///`. - for shard in directories(&cache_shard) { + for shard in directories(&cache_shard).ok().into_iter().flatten() { let cache_shard = cache_shard.shard(shard); // Read the revision from the cache. @@ -205,7 +205,7 @@ impl<'a> RegistryWheelIndex<'a> { cache_shard.shard(cache_digest(build_configuration)) }; - for wheel_dir in uv_fs::entries(cache_shard) { + for wheel_dir in uv_fs::entries(cache_shard).ok().into_iter().flatten() { // Ignore any `.lock` files. if wheel_dir .extension() diff --git a/crates/uv-distribution/src/source/built_wheel_metadata.rs b/crates/uv-distribution/src/source/built_wheel_metadata.rs index 43eb2cbea..6b6089ea3 100644 --- a/crates/uv-distribution/src/source/built_wheel_metadata.rs +++ b/crates/uv-distribution/src/source/built_wheel_metadata.rs @@ -28,16 +28,19 @@ pub(crate) struct BuiltWheelMetadata { impl BuiltWheelMetadata { /// Find a compatible wheel in the cache. - pub(crate) fn find_in_cache(tags: &Tags, cache_shard: &CacheShard) -> Option { - for file in files(cache_shard) { + pub(crate) fn find_in_cache( + tags: &Tags, + cache_shard: &CacheShard, + ) -> Result, std::io::Error> { + for file in files(cache_shard)? { if let Some(metadata) = Self::from_path(file, cache_shard) { // Validate that the wheel is compatible with the target platform. if metadata.filename.is_compatible(tags) { - return Some(metadata); + return Ok(Some(metadata)); } } } - None + Ok(None) } /// Try to parse a distribution from a cached directory name (like `typing-extensions-4.8.0-py3-none-any.whl`). diff --git a/crates/uv-distribution/src/source/mod.rs b/crates/uv-distribution/src/source/mod.rs index ebd3674cb..0ed2560e4 100644 --- a/crates/uv-distribution/src/source/mod.rs +++ b/crates/uv-distribution/src/source/mod.rs @@ -425,6 +425,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { // If the cache contains a compatible wheel, return it. if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard) + .ok() + .flatten() .filter(|built_wheel| built_wheel.matches(source.name(), source.version())) { return Ok(built_wheel.with_hashes(revision.into_hashes())); @@ -795,6 +797,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { // If the cache contains a compatible wheel, return it. if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard) + .ok() + .flatten() .filter(|built_wheel| built_wheel.matches(source.name(), source.version())) { return Ok(built_wheel); @@ -1097,6 +1101,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { // If the cache contains a compatible wheel, return it. if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard) + .ok() + .flatten() .filter(|built_wheel| built_wheel.matches(source.name(), source.version())) { return Ok(built_wheel); @@ -1474,6 +1480,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { // If the cache contains a compatible wheel, return it. if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard) + .ok() + .flatten() .filter(|built_wheel| built_wheel.matches(source.name(), source.version())) { return Ok(built_wheel); diff --git a/crates/uv-fs/src/lib.rs b/crates/uv-fs/src/lib.rs index ea7cdffd7..e13672f6c 100644 --- a/crates/uv-fs/src/lib.rs +++ b/crates/uv-fs/src/lib.rs @@ -1,4 +1,5 @@ use std::fmt::Display; +use std::io; use std::path::{Path, PathBuf}; use fs2::FileExt; @@ -516,60 +517,69 @@ pub fn persist_with_retry_sync( /// Iterate over the subdirectories of a directory. /// /// If the directory does not exist, returns an empty iterator. -pub fn directories(path: impl AsRef) -> impl Iterator { - path.as_ref() - .read_dir() - .ok() +pub fn directories(path: impl AsRef) -> Result, io::Error> { + let entries = match path.as_ref().read_dir() { + Ok(entries) => Some(entries), + Err(err) if err.kind() == io::ErrorKind::NotFound => None, + Err(err) => return Err(err), + }; + Ok(entries .into_iter() .flatten() .filter_map(|entry| match entry { Ok(entry) => Some(entry), Err(err) => { - warn!("Failed to read entry: {}", err); + warn!("Failed to read entry: {err}"); None } }) .filter(|entry| entry.file_type().is_ok_and(|file_type| file_type.is_dir())) - .map(|entry| entry.path()) + .map(|entry| entry.path())) } /// Iterate over the entries in a directory. /// /// If the directory does not exist, returns an empty iterator. -pub fn entries(path: impl AsRef) -> impl Iterator { - path.as_ref() - .read_dir() - .ok() +pub fn entries(path: impl AsRef) -> Result, io::Error> { + let entries = match path.as_ref().read_dir() { + Ok(entries) => Some(entries), + Err(err) if err.kind() == io::ErrorKind::NotFound => None, + Err(err) => return Err(err), + }; + Ok(entries .into_iter() .flatten() .filter_map(|entry| match entry { Ok(entry) => Some(entry), Err(err) => { - warn!("Failed to read entry: {}", err); + warn!("Failed to read entry: {err}"); None } }) - .map(|entry| entry.path()) + .map(|entry| entry.path())) } /// Iterate over the files in a directory. /// /// If the directory does not exist, returns an empty iterator. -pub fn files(path: impl AsRef) -> impl Iterator { - path.as_ref() - .read_dir() - .ok() +pub fn files(path: impl AsRef) -> Result, io::Error> { + let entries = match path.as_ref().read_dir() { + Ok(entries) => Some(entries), + Err(err) if err.kind() == io::ErrorKind::NotFound => None, + Err(err) => return Err(err), + }; + Ok(entries .into_iter() .flatten() .filter_map(|entry| match entry { Ok(entry) => Some(entry), Err(err) => { - warn!("Failed to read entry: {}", err); + warn!("Failed to read entry: {err}"); None } }) .filter(|entry| entry.file_type().is_ok_and(|file_type| file_type.is_file())) - .map(|entry| entry.path()) + .map(|entry| entry.path())) } /// Returns `true` if a path is a temporary file or directory. diff --git a/crates/uv-tool/src/lib.rs b/crates/uv-tool/src/lib.rs index 65f88429a..df78f3644 100644 --- a/crates/uv-tool/src/lib.rs +++ b/crates/uv-tool/src/lib.rs @@ -97,7 +97,7 @@ impl InstalledTools { #[allow(clippy::type_complexity)] pub fn tools(&self) -> Result)>, Error> { let mut tools = Vec::new(); - for directory in uv_fs::directories(self.root()) { + for directory in uv_fs::directories(self.root())? { let name = directory.file_name().unwrap().to_string_lossy().to_string(); let name = PackageName::from_str(&name)?; let path = directory.join("uv-receipt.toml"); diff --git a/crates/uv/src/commands/python/uninstall.rs b/crates/uv/src/commands/python/uninstall.rs index a1b339a6e..389b60071 100644 --- a/crates/uv/src/commands/python/uninstall.rs +++ b/crates/uv/src/commands/python/uninstall.rs @@ -37,7 +37,7 @@ pub(crate) async fn uninstall( do_uninstall(&installations, targets, all, printer, preview).await?; // Clean up any empty directories. - if uv_fs::directories(installations.root()).all(|path| uv_fs::is_temporary(&path)) { + if uv_fs::directories(installations.root())?.all(|path| uv_fs::is_temporary(&path)) { fs_err::tokio::remove_dir_all(&installations.root()).await?; if let Some(top_level) = installations.root().parent() { @@ -48,7 +48,7 @@ pub(crate) async fn uninstall( Err(err) => return Err(err.into()), } - if uv_fs::directories(top_level).all(|path| uv_fs::is_temporary(&path)) { + if uv_fs::directories(top_level)?.all(|path| uv_fs::is_temporary(&path)) { fs_err::tokio::remove_dir_all(top_level).await?; } } diff --git a/crates/uv/src/commands/tool/uninstall.rs b/crates/uv/src/commands/tool/uninstall.rs index c9577adbc..44c32891d 100644 --- a/crates/uv/src/commands/tool/uninstall.rs +++ b/crates/uv/src/commands/tool/uninstall.rs @@ -34,12 +34,12 @@ pub(crate) async fn uninstall(name: Vec, printer: Printer) -> Resul do_uninstall(&installed_tools, name, printer).await?; // Clean up any empty directories. - if uv_fs::directories(installed_tools.root()).all(|path| uv_fs::is_temporary(&path)) { + if uv_fs::directories(installed_tools.root())?.all(|path| uv_fs::is_temporary(&path)) { fs_err::tokio::remove_dir_all(&installed_tools.root()) .await .ignore_currently_being_deleted()?; if let Some(parent) = installed_tools.root().parent() { - if uv_fs::directories(parent).all(|path| uv_fs::is_temporary(&path)) { + if uv_fs::directories(parent)?.all(|path| uv_fs::is_temporary(&path)) { fs_err::tokio::remove_dir_all(parent) .await .ignore_currently_being_deleted()?;