From d20948bec2d9de879448fe9070dfd442045892e4 Mon Sep 17 00:00:00 2001 From: Tomasz Kramkowski Date: Mon, 15 Dec 2025 14:05:05 +0000 Subject: [PATCH] Support creating lock files on ExFAT on MacOS (#17115) ## Summary Fix #16859 by falling back to simply creating the lock file and then attempting to apply permissions in cases where the temporary lockfile cannot be renamed without overwriting (persist_noclobber) due to lack of underlying support from the filesystem. I've also improved the error handling. ## Test Plan Manually on MacOS with an ExFAT partition. ~~~ bash session $ hdiutil create -size 1g -fs ExFAT -volname EXFATDISK exfat.dmg $ hdiutil attach exfat.dmg $ cd /Volumes/EXFATDISK $ uv init --bare --cache-dir build/uv/cache -v ~~~ --- Cargo.lock | 2 +- crates/uv-bin-install/Cargo.toml | 1 - crates/uv-bin-install/src/lib.rs | 5 +- crates/uv-cache/Cargo.toml | 1 + crates/uv-cache/src/lib.rs | 37 ++++++++---- crates/uv-client/src/error.rs | 4 +- crates/uv-distribution/src/error.rs | 5 +- crates/uv-fs/src/locked_file.rs | 92 ++++++++++++++++++++++++----- 8 files changed, 110 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9bb2813f1..a3c673041 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5613,7 +5613,6 @@ dependencies = [ "uv-client", "uv-distribution-filename", "uv-extract", - "uv-fs", "uv-pep440", "uv-platform", "uv-redacted", @@ -5721,6 +5720,7 @@ dependencies = [ "same-file", "serde", "tempfile", + "thiserror 2.0.17", "tracing", "uv-cache-info", "uv-cache-key", diff --git a/crates/uv-bin-install/Cargo.toml b/crates/uv-bin-install/Cargo.toml index 7d9487abd..f0241f480 100644 --- a/crates/uv-bin-install/Cargo.toml +++ b/crates/uv-bin-install/Cargo.toml @@ -20,7 +20,6 @@ uv-cache = { workspace = true } uv-client = { workspace = true } uv-distribution-filename = { workspace = true } uv-extract = { workspace = true } -uv-fs = { workspace = true } uv-pep440 = { workspace = true } uv-platform = { workspace = true } uv-redacted = { workspace = true } diff --git a/crates/uv-bin-install/src/lib.rs b/crates/uv-bin-install/src/lib.rs index 1a26b0a56..dd3107aa3 100644 --- a/crates/uv-bin-install/src/lib.rs +++ b/crates/uv-bin-install/src/lib.rs @@ -19,10 +19,9 @@ use tracing::debug; use url::Url; use uv_distribution_filename::SourceDistExtension; -use uv_cache::{Cache, CacheBucket, CacheEntry}; +use uv_cache::{Cache, CacheBucket, CacheEntry, Error as CacheError}; use uv_client::{BaseClient, is_transient_network_error}; use uv_extract::{Error as ExtractError, stream}; -use uv_fs::LockedFileError; use uv_pep440::Version; use uv_platform::Platform; use uv_redacted::DisplaySafeUrl; @@ -137,7 +136,7 @@ pub enum Error { Io(#[from] std::io::Error), #[error(transparent)] - LockedFile(#[from] LockedFileError), + Cache(#[from] CacheError), #[error("Failed to detect platform")] Platform(#[from] uv_platform::Error), diff --git a/crates/uv-cache/Cargo.toml b/crates/uv-cache/Cargo.toml index 5df57fbab..681bd520e 100644 --- a/crates/uv-cache/Cargo.toml +++ b/crates/uv-cache/Cargo.toml @@ -34,5 +34,6 @@ rustc-hash = { workspace = true } same-file = { workspace = true } serde = { workspace = true, features = ["derive"] } tempfile = { workspace = true } +thiserror = { workspace = true } tracing = { workspace = true } walkdir = { workspace = true } diff --git a/crates/uv-cache/src/lib.rs b/crates/uv-cache/src/lib.rs index b38f15f36..d67118cd5 100644 --- a/crates/uv-cache/src/lib.rs +++ b/crates/uv-cache/src/lib.rs @@ -35,6 +35,17 @@ mod wheel; /// Must be kept in-sync with the version in [`CacheBucket::to_str`]. pub const ARCHIVE_VERSION: u8 = 0; +/// Error locking a cache entry or shard +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error(transparent)] + Io(#[from] io::Error), + #[error("Could not make the path absolute")] + Absolute(#[source] io::Error), + #[error("Could not acquire lock")] + Acquire(#[from] LockedFileError), +} + /// A [`CacheEntry`] which may or may not exist yet. #[derive(Debug, Clone)] pub struct CacheEntry(PathBuf); @@ -80,14 +91,14 @@ impl CacheEntry { } /// Acquire the [`CacheEntry`] as an exclusive lock. - pub async fn lock(&self) -> Result { + pub async fn lock(&self) -> Result { fs_err::create_dir_all(self.dir())?; - LockedFile::acquire( + Ok(LockedFile::acquire( self.path(), LockedFileMode::Exclusive, self.path().display(), ) - .await + .await?) } } @@ -114,14 +125,14 @@ impl CacheShard { } /// Acquire the cache entry as an exclusive lock. - pub async fn lock(&self) -> Result { + pub async fn lock(&self) -> Result { fs_err::create_dir_all(self.as_ref())?; - LockedFile::acquire( + Ok(LockedFile::acquire( self.join(".lock"), LockedFileMode::Exclusive, self.display(), ) - .await + .await?) } /// Return the [`CacheShard`] as a [`PathBuf`]. @@ -391,7 +402,7 @@ impl Cache { } /// Populate the cache scaffold. - fn create_base_files(root: &PathBuf) -> Result<(), io::Error> { + fn create_base_files(root: &PathBuf) -> io::Result<()> { // Create the cache directory, if it doesn't exist. fs_err::create_dir_all(root)?; @@ -441,7 +452,7 @@ impl Cache { } /// Initialize the [`Cache`]. - pub async fn init(self) -> Result { + pub async fn init(self) -> Result { let root = &self.root; Self::create_base_files(root)?; @@ -466,18 +477,18 @@ impl Cache { ); None } - Err(err) => return Err(err), + Err(err) => return Err(err.into()), }; Ok(Self { - root: std::path::absolute(root)?, + root: std::path::absolute(root).map_err(Error::Absolute)?, lock_file, ..self }) } /// Initialize the [`Cache`], assuming that there are no other uv processes running. - pub fn init_no_wait(self) -> Result, io::Error> { + pub fn init_no_wait(self) -> Result, Error> { let root = &self.root; Self::create_base_files(root)?; @@ -491,7 +502,7 @@ impl Cache { return Ok(None); }; Ok(Some(Self { - root: std::path::absolute(root)?, + root: std::path::absolute(root).map_err(Error::Absolute)?, lock_file: Some(Arc::new(lock_file)), ..self })) @@ -531,7 +542,7 @@ impl Cache { /// Remove a package from the cache. /// /// Returns the number of entries removed from the cache. - pub fn remove(&self, name: &PackageName) -> Result { + pub fn remove(&self, name: &PackageName) -> io::Result { // Collect the set of referenced archives. let references = self.find_archive_references()?; diff --git a/crates/uv-client/src/error.rs b/crates/uv-client/src/error.rs index 21180dc20..858a7a6e7 100644 --- a/crates/uv-client/src/error.rs +++ b/crates/uv-client/src/error.rs @@ -5,8 +5,8 @@ use std::fmt::{Display, Formatter}; use std::ops::Deref; use std::path::PathBuf; +use uv_cache::Error as CacheError; use uv_distribution_filename::{WheelFilename, WheelFilenameError}; -use uv_fs::LockedFileError; use uv_normalize::PackageName; use uv_redacted::DisplaySafeUrl; @@ -339,7 +339,7 @@ pub enum ErrorKind { CacheWrite(#[source] std::io::Error), #[error("Failed to acquire lock on the client cache")] - CacheLock(#[source] LockedFileError), + CacheLock(#[source] CacheError), #[error(transparent)] Io(std::io::Error), diff --git a/crates/uv-distribution/src/error.rs b/crates/uv-distribution/src/error.rs index 172074dd0..4961a5694 100644 --- a/crates/uv-distribution/src/error.rs +++ b/crates/uv-distribution/src/error.rs @@ -5,10 +5,11 @@ use tokio::task::JoinError; use zip::result::ZipError; use crate::metadata::MetadataError; +use uv_cache::Error as CacheError; use uv_client::WrappedReqwestError; use uv_distribution_filename::{WheelFilename, WheelFilenameError}; use uv_distribution_types::{InstalledDist, InstalledDistError, IsBuildBackendError}; -use uv_fs::{LockedFileError, Simplified}; +use uv_fs::Simplified; use uv_git::GitError; use uv_normalize::PackageName; use uv_pep440::{Version, VersionSpecifiers}; @@ -42,7 +43,7 @@ pub enum Error { #[error("Failed to write to the distribution cache")] CacheWrite(#[source] std::io::Error), #[error("Failed to acquire lock on the distribution cache")] - CacheLock(#[source] LockedFileError), + CacheLock(#[source] CacheError), #[error("Failed to deserialize cache entry")] CacheDecode(#[from] rmp_serde::decode::Error), #[error("Failed to serialize cache entry")] diff --git a/crates/uv-fs/src/locked_file.rs b/crates/uv-fs/src/locked_file.rs index 3d9e6ea95..46a913dd2 100644 --- a/crates/uv-fs/src/locked_file.rs +++ b/crates/uv-fs/src/locked_file.rs @@ -1,3 +1,4 @@ +use std::convert::Into; use std::fmt::Display; use std::path::{Path, PathBuf}; use std::sync::LazyLock; @@ -59,10 +60,18 @@ pub enum LockedFileError { source: io::Error, }, #[error(transparent)] - Io(#[from] io::Error), - #[error(transparent)] #[cfg(feature = "tokio")] JoinError(#[from] tokio::task::JoinError), + #[error("Could not create temporary file")] + CreateTemporary(#[source] io::Error), + #[error("Could not persist temporary file `{}`", path.user_display())] + PersistTemporary { + path: PathBuf, + #[source] + source: io::Error, + }, + #[error(transparent)] + Io(#[from] io::Error), } impl LockedFileError { @@ -72,6 +81,8 @@ impl LockedFileError { #[cfg(feature = "tokio")] Self::JoinError(_) => None, Self::Lock { source, .. } => Some(source), + Self::CreateTemporary(err) => Some(err), + Self::PersistTemporary { source, .. } => Some(source), Self::Io(err) => Some(err), } } @@ -201,7 +212,7 @@ impl LockedFile { mode: LockedFileMode, resource: impl Display, ) -> Result { - let file = Self::create(path)?; + let file = Self::create(&path)?; let resource = resource.to_string(); Self::lock_file(file, mode, &resource).await } @@ -222,10 +233,25 @@ impl LockedFile { } #[cfg(unix)] - fn create(path: impl AsRef) -> Result { - use std::os::unix::fs::PermissionsExt; + fn create(path: impl AsRef) -> Result { + use rustix::io::Errno; + #[allow(clippy::disallowed_types)] + use std::{fs::File, os::unix::fs::PermissionsExt}; use tempfile::NamedTempFile; + /// The permissions the lockfile should end up with + const DESIRED_MODE: u32 = 0o666; + + #[allow(clippy::disallowed_types)] + fn try_set_permissions(file: &File, path: &Path) { + if let Err(err) = file.set_permissions(std::fs::Permissions::from_mode(DESIRED_MODE)) { + warn!( + "Failed to set permissions on temporary file `{path}`: {err}", + path = path.user_display() + ); + } + } + // If path already exists, return it. if let Ok(file) = fs_err::OpenOptions::new() .read(true) @@ -238,16 +264,12 @@ impl LockedFile { // Otherwise, create a temporary file with 666 permissions. We must set // permissions _after_ creating the file, to override the `umask`. let file = if let Some(parent) = path.as_ref().parent() { - NamedTempFile::new_in(parent)? + NamedTempFile::new_in(parent) } else { - NamedTempFile::new()? - }; - if let Err(err) = file - .as_file() - .set_permissions(std::fs::Permissions::from_mode(0o666)) - { - warn!("Failed to set permissions on temporary file: {err}"); + NamedTempFile::new() } + .map_err(LockedFileError::CreateTemporary)?; + try_set_permissions(file.as_file(), file.path()); // Try to move the file to path, but if path exists now, just open path match file.persist_noclobber(path.as_ref()) { @@ -258,20 +280,60 @@ impl LockedFile { .read(true) .write(true) .open(path.as_ref()) + .map_err(Into::into) + } else if matches!( + Errno::from_io_error(&err.error), + Some(Errno::NOTSUP | Errno::INVAL) + ) { + // Fallback in case `persist_noclobber`, which uses `renameat2` or + // `renameatx_np` under the hood, is not supported by the FS. Linux reports this + // with `EINVAL` and MacOS with `ENOTSUP`. For these reasons and many others, + // there isn't an ErrorKind we can use here, and in fact on MacOS `ENOTSUP` gets + // mapped to `ErrorKind::Other` + + // There is a race here where another process has just created the file, and we + // try to open it and get permission errors because the other process hasn't set + // the permission bits yet. This will lead to a transient failure, but unlike + // alternative approaches it won't ever lead to a situation where two processes + // are locking two different files. Also, since `persist_noclobber` is more + // likely to not be supported on special filesystems which don't have permission + // bits, it's less likely to ever matter. + let file = fs_err::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(path.as_ref())?; + + // We don't want to `try_set_permissions` in cases where another user's process + // has already created the lockfile and changed its permissions because we might + // not have permission to change the permissions which would produce a confusing + // warning. + if file + .metadata() + .is_ok_and(|metadata| metadata.permissions().mode() != DESIRED_MODE) + { + try_set_permissions(file.file(), path.as_ref()); + } + Ok(file) } else { - Err(err.error) + let temp_path = err.file.into_temp_path(); + Err(LockedFileError::PersistTemporary { + path: >::as_ref(&temp_path).to_path_buf(), + source: err.error, + }) } } } } #[cfg(not(unix))] - fn create(path: impl AsRef) -> std::io::Result { + fn create(path: impl AsRef) -> Result { fs_err::OpenOptions::new() .read(true) .write(true) .create(true) .open(path.as_ref()) + .map_err(Into::into) } }