Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Windows AV/EDR file locks during script installations #9543

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

124 changes: 118 additions & 6 deletions crates/uv-fs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,13 @@ pub fn copy_atomic_sync(from: impl AsRef<Path>, to: impl AsRef<Path>) -> std::io
Ok(())
}

fn backoff_file_move() -> backoff::ExponentialBackoff {
backoff::ExponentialBackoffBuilder::default()
.with_initial_interval(std::time::Duration::from_millis(10))
.with_max_elapsed_time(Some(std::time::Duration::from_secs(10)))
.build()
}

/// Rename a file, retrying (on Windows) if it fails due to transient operating system errors.
#[cfg(feature = "tokio")]
pub async fn rename_with_retry(
Expand All @@ -227,15 +234,11 @@ pub async fn rename_with_retry(
// This is most common for DLLs, and the common suggestion is to retry the operation with
// some backoff.
//
// See: <https://github.com/astral-sh/uv/issues/1491>
// See: <https://github.com/astral-sh/uv/issues/1491> & <https://github.com/astral-sh/uv/issues/9531>
let from = from.as_ref();
let to = to.as_ref();

let backoff = backoff::ExponentialBackoffBuilder::default()
.with_initial_interval(std::time::Duration::from_millis(10))
.with_max_elapsed_time(Some(std::time::Duration::from_secs(10)))
.build();

let backoff = backoff_file_move();
backoff::future::retry(backoff, || async move {
match fs_err::rename(from, to) {
Ok(()) => Ok(()),
Expand All @@ -257,6 +260,115 @@ pub async fn rename_with_retry(
}
}

/// Rename a file, retrying (on Windows) if it fails due to transient operating system errors, in a synchronous context.
pub fn rename_with_retry_sync(
from: impl AsRef<Path>,
to: impl AsRef<Path>,
) -> Result<(), std::io::Error> {
if cfg!(windows) {
// On Windows, antivirus software can lock files temporarily, making them inaccessible.
// This is most common for DLLs, and the common suggestion is to retry the operation with
// some backoff.
//
// See: <https://github.com/astral-sh/uv/issues/1491> & <https://github.com/astral-sh/uv/issues/9531>
let from = from.as_ref();
let to = to.as_ref();

let backoff = backoff_file_move();
backoff::retry(backoff, || match fs_err::rename(from, to) {
Ok(()) => Ok(()),
Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => {
warn!(
"Retrying rename from {} to {} due to transient error: {}",
from.display(),
to.display(),
err
);
Err(backoff::Error::transient(err))
}
Err(err) => Err(backoff::Error::permanent(err)),
})
.map_err(|err| {
std::io::Error::new(
std::io::ErrorKind::Other,
format!(
"Failed to rename {} to {}: {}",
from.display(),
to.display(),
err
),
)
})
} else {
fs_err::rename(from, to)
}
}

/// Persist a `NamedTempFile`, retrying (on Windows) if it fails due to transient operating system errors, in a synchronous context.
pub fn persist_with_retry_sync(
from: NamedTempFile,
to: impl AsRef<Path>,
) -> Result<(), std::io::Error> {
if cfg!(windows) {
// On Windows, antivirus software can lock files temporarily, making them inaccessible.
// This is most common for DLLs, and the common suggestion is to retry the operation with
// some backoff.
//
// See: <https://github.com/astral-sh/uv/issues/1491> & <https://github.com/astral-sh/uv/issues/9531>
let to = to.as_ref();

// the `NamedTempFile` `persist` method consumes `self`, and returns it back inside the Error in case of `PersistError`
// https://docs.rs/tempfile/latest/tempfile/struct.NamedTempFile.html#method.persist
// So we will update the `from` optional value in safe and borrow-checker friendly way every retry
// Allows us to use the NamedTempFile inside a FnMut closure used for backoff::retry
let mut from = Some(from);

let backoff = backoff_file_move();
let persisted = backoff::retry(backoff, move || {
if let Some(file) = from.take() {
file.persist(to).map_err(|err| {
let error_message = err.to_string();
warn!(
"Retrying to persist temporary file to {}: {}",
to.display(),
error_message
);

// Set back the NamedTempFile returned back by the Error
from = Some(err.file);

backoff::Error::transient(std::io::Error::new(
std::io::ErrorKind::Other,
format!(
"Failed to persist temporary file to {}: {}",
to.display(),
error_message
),
))
})
} else {
Err(backoff::Error::permanent(std::io::Error::new(
std::io::ErrorKind::Other,
format!(
"Failed to retrieve temporary file while trying to persist to {}",
to.display()
),
)))
}
});

match persisted {
Ok(_) => Ok(()),
Err(err) => Err(std::io::Error::new(
std::io::ErrorKind::Other,
format!("{err:?}"),
)),
}
} else {
fs_err::rename(from, to)
}
}

/// Iterate over the subdirectories of a directory.
///
/// If the directory does not exist, returns an empty iterator.
Expand Down
18 changes: 5 additions & 13 deletions crates/uv-install-wheel/src/wheel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use tracing::{instrument, warn};
use walkdir::WalkDir;

use uv_cache_info::CacheInfo;
use uv_fs::{relative_to, Simplified};
use uv_fs::{persist_with_retry_sync, relative_to, rename_with_retry_sync, Simplified};
use uv_normalize::PackageName;
use uv_pypi_types::DirectUrl;
use uv_shell::escape_posix_for_single_quotes;
Expand Down Expand Up @@ -424,16 +424,8 @@ fn install_script(

let mut target = uv_fs::tempfile_in(&layout.scheme.scripts)?;
let size_and_encoded_hash = copy_and_hash(&mut start.chain(script), &mut target)?;
target.persist(&script_absolute).map_err(|err| {
io::Error::new(
io::ErrorKind::Other,
format!(
"Failed to persist temporary file to {}: {}",
path.user_display(),
err.error
),
)
})?;

persist_with_retry_sync(target, &script_absolute)?;
fs::remove_file(&path)?;

// Make the script executable. We just created the file, so we can set permissions directly.
Expand Down Expand Up @@ -467,7 +459,7 @@ fn install_script(

if permissions.mode() & 0o111 == 0o111 {
// If the permissions are already executable, we don't need to change them.
fs::rename(&path, &script_absolute)?;
rename_with_retry_sync(&path, &script_absolute)?;
} else {
// If we have to modify the permissions, copy the file, since we might not own it.
warn!(
Expand All @@ -488,7 +480,7 @@ fn install_script(

#[cfg(not(unix))]
{
fs::rename(&path, &script_absolute)?;
rename_with_retry_sync(&path, &script_absolute)?;
}

None
Expand Down
Loading