diff --git a/.changeset/retry-windows-staging-swaps.md b/.changeset/retry-windows-staging-swaps.md new file mode 100644 index 0000000000..cb03efc72e --- /dev/null +++ b/.changeset/retry-windows-staging-swaps.md @@ -0,0 +1,6 @@ +--- +"pacquet": patch +--- + +Retry transient Windows file-lock errors while replacing hoisted packages during installation. +This fixes [pnpm/pnpm#14349](https://github.com/pnpm/pnpm/issues/14349). diff --git a/pnpm/crates/deps-restorer/src/import_indexed_dir.rs b/pnpm/crates/deps-restorer/src/import_indexed_dir.rs index e341ee4900..35eaf13b7f 100644 --- a/pnpm/crates/deps-restorer/src/import_indexed_dir.rs +++ b/pnpm/crates/deps-restorer/src/import_indexed_dir.rs @@ -680,7 +680,7 @@ fn stage_and_swap( // the staged tree and any merge backup hold the preserved data. // Try to move it back into place before bailing, and retain // those temporary paths if restoration can't run. - if let Err(error) = fs::remove_dir_all(dir_path) { + if let Err(error) = pnpm_fs::remove_dir_all_with_retry(dir_path) { finalize_stage_cleanup_after_failure( &preserved_modules, &stage, @@ -697,7 +697,7 @@ fn stage_and_swap( // rename fails, recreate // `dir_path` so the rescued `node_modules/` has somewhere to // land. - if let Err(error) = fs::rename(&stage, dir_path) { + if let Err(error) = pnpm_fs::rename_with_retry(&stage, dir_path) { // `create_dir_all` is the gate: without `dir_path`, the rescue // rename has no destination. Treat its failure as "rescue // can't run" and leak the staging directory below. diff --git a/pnpm/crates/fs/src/ensure_file.rs b/pnpm/crates/fs/src/ensure_file.rs index d76cdc8075..5c4589da41 100644 --- a/pnpm/crates/fs/src/ensure_file.rs +++ b/pnpm/crates/fs/src/ensure_file.rs @@ -1,3 +1,4 @@ +use crate::rename_with_retry; use derive_more::{Display, Error}; use miette::Diagnostic; use std::{ @@ -9,9 +10,11 @@ use std::{ Mutex, atomic::{AtomicU64, Ordering}, }, - time::{Duration, Instant}, }; +#[cfg(unix)] +use std::time::Duration; + /// POSIX `EMFILE` — process has hit `RLIMIT_NOFILE`. Hardcoded /// instead of pulling in `libc` for a single integer that's been /// stable across every Unix since 4.2BSD. @@ -469,84 +472,6 @@ pub fn create_exclusive_temp_file( }) } -/// Total budget for retrying a rename that keeps hitting transient -/// errors. -const RENAME_RETRY_BUDGET: Duration = Duration::from_mins(1); - -/// Cap on per-iteration sleep — the backoff grows by 10 ms each loop -/// and stops growing at 100 ms. -const RENAME_RETRY_BACKOFF_CAP: Duration = Duration::from_millis(100); - -/// `fs::rename` with the one retry family that actually hits pacquet -/// in practice: Windows Defender (and other Windows antivirus / file- -/// indexer tooling) momentarily holding the destination open, which -/// makes the rename fail with `ERROR_ACCESS_DENIED` / -/// `ERROR_SHARING_VIOLATION`. These surface through Rust's -/// `io::ErrorKind` as `PermissionDenied` or `ResourceBusy`, and they -/// clear as soon as the scan completes — a short sleep + retry -/// recovers. Mirrors the `EPERM|EACCES|EBUSY` arm of -/// `rename-overwrite`'s `renameOverwriteSync` (see zkochan/packages/ -/// rename-overwrite/index.js): 60-second total budget, 10 ms backoff -/// step, 100 ms cap. -/// -/// Other retry arms from `rename-overwrite` (`ENOTEMPTY`/`EEXIST`/ -/// `ENOTDIR` swap-rename, `ENOENT` mkdir-and-recurse, `EXDEV` copy- -/// and-delete) don't apply to this call site: temp and target share -/// the CAS shard dir (already pre-created by `StoreDir::init`), both -/// are files not directories, and pacquet's CAS readers -/// (`link_file` → `fs::hard_link` / `reflink_copy`) don't keep file -/// handles on the target, so there's no "parallel reader sees a gap" -/// concern that would motivate swap-rename. -pub fn rename_with_retry(src: &Path, dst: &Path) -> io::Result<()> { - let mut backoff = Duration::ZERO; - let start = Instant::now(); - - loop { - match fs::rename(src, dst) { - Ok(()) => return Ok(()), - Err(error) => { - if !is_transient_rename_error(&error) || start.elapsed() >= RENAME_RETRY_BUDGET { - return Err(error); - } - if !backoff.is_zero() { - std::thread::sleep(backoff); - } - backoff = (backoff + Duration::from_millis(10)).min(RENAME_RETRY_BACKOFF_CAP); - } - } - } -} - -/// Classify a `rename` error as transient-retry-worthy. -/// -/// On Windows, AV / indexer interference briefly holds the -/// destination open and surfaces as `ERROR_ACCESS_DENIED` (→ -/// `PermissionDenied`) or `ERROR_SHARING_VIOLATION` (→ -/// `ResourceBusy`, Rust 1.84+ mapping). Both clear on their own -/// within tens-to-hundreds of ms, which is exactly what the retry -/// loop is for. -/// -/// On Unix, `rename` returning `EACCES`/`EPERM` is essentially -/// always a permanent permission issue (non-writable directory, -/// sticky-bit conflict, `AppArmor` deny) — retrying for 60 s just -/// stretches out the failure. `EBUSY` on Unix also tends to be -/// permanent (mount-point conflicts). So on non-Windows the -/// classifier is disabled and any `rename` error propagates -/// immediately. -fn is_transient_rename_error( - #[cfg_attr(not(windows), allow(unused, reason = "only inspected in the Windows branch below"))] - error: &io::Error, -) -> bool { - #[cfg(windows)] - { - matches!(error.kind(), io::ErrorKind::PermissionDenied | io::ErrorKind::ResourceBusy) - } - #[cfg(not(windows))] - { - false - } -} - /// Build a unique temp path inside `dir`, of the form /// `{base}{pid}{counter}` per [`create_exclusive_temp_file`]'s /// uniqueness contract. diff --git a/pnpm/crates/fs/src/ensure_file/tests.rs b/pnpm/crates/fs/src/ensure_file/tests.rs index f5c2718a93..b51431a46c 100644 --- a/pnpm/crates/fs/src/ensure_file/tests.rs +++ b/pnpm/crates/fs/src/ensure_file/tests.rs @@ -1,6 +1,6 @@ use super::{ - EnsureFileError, create_exclusive_temp_file, ensure_file, file_equals_bytes, - is_transient_rename_error, rename_with_retry, strip_dash_suffix, temp_path_in, + EnsureFileError, create_exclusive_temp_file, ensure_file, file_equals_bytes, strip_dash_suffix, + temp_path_in, }; use std::{fs, io, path::Path}; use tempfile::tempdir; @@ -119,54 +119,6 @@ fn create_exclusive_temp_file_yields_distinct_open_files() { assert_eq!(fs::read(&path_a).unwrap(), b"payload"); } -/// Windows AV / indexer interference surfaces as -/// `PermissionDenied` or `ResourceBusy` and must trigger the -/// retry loop there. On non-Windows those codes are essentially -/// always permanent (permission / mount-point issues), so the -/// classifier must return `false` to avoid pathologically -/// spinning for 60 s on a misconfigured store dir. Any other -/// kind must propagate immediately on every platform. -#[test] -fn transient_rename_error_classifier() { - let permission_denied = io::Error::from(io::ErrorKind::PermissionDenied); - let resource_busy = io::Error::from(io::ErrorKind::ResourceBusy); - - #[cfg(windows)] - { - assert!(is_transient_rename_error(&permission_denied)); - assert!(is_transient_rename_error(&resource_busy)); - } - #[cfg(not(windows))] - { - assert!( - !is_transient_rename_error(&permission_denied), - "Unix PermissionDenied is permanent, must not retry", - ); - assert!( - !is_transient_rename_error(&resource_busy), - "Unix ResourceBusy is effectively permanent, must not retry", - ); - } - - // Non-transient kinds must never trigger the retry loop on - // any platform — a regression classifying e.g. `NotFound` as - // transient would spin for 60 s on a legitimately missing - // source. - for kind in [ - io::ErrorKind::NotFound, - io::ErrorKind::AlreadyExists, - io::ErrorKind::InvalidInput, - io::ErrorKind::InvalidData, - io::ErrorKind::Unsupported, - io::ErrorKind::Other, - ] { - assert!( - !is_transient_rename_error(&io::Error::from(kind)), - "{kind:?} must not be classified as transient", - ); - } -} - /// A symlink at the target path — which on Unix returns `EEXIST` /// from `open(O_CREAT|O_EXCL)` just like a regular file would — /// must be scrubbed and replaced with a real regular file even @@ -210,25 +162,6 @@ fn dangling_symlink_at_cas_path_is_scrubbed_to_a_regular_file() { assert_eq!(fs::read(&cas_path).unwrap(), b"fresh"); } -/// Happy-path rename (no transient errors) moves the payload -/// atomically and removes the source. Correctness only — we -/// deliberately don't assert a wall-clock bound because rename -/// latency on loaded CI / slow filesystems can exceed any -/// reasonable timing threshold without the retry path actually -/// being taken. -#[test] -fn rename_with_retry_succeeds_when_no_error() { - let tmp = tempdir().unwrap(); - let src = tmp.path().join("src"); - let dst = tmp.path().join("dst"); - fs::write(&src, b"payload").unwrap(); - - rename_with_retry(&src, &dst).expect("rename should succeed"); - - assert_eq!(fs::read(&dst).unwrap(), b"payload"); - assert!(!src.exists(), "source should be gone after rename"); -} - #[test] fn file_equals_bytes_classifies_match_mismatch_and_length_mismatch() { let tmp = tempdir().unwrap(); diff --git a/pnpm/crates/fs/src/lib.rs b/pnpm/crates/fs/src/lib.rs index 1547a89597..4c6971f293 100644 --- a/pnpm/crates/fs/src/lib.rs +++ b/pnpm/crates/fs/src/lib.rs @@ -5,6 +5,7 @@ mod lexical_normalize; mod realpath_missing; mod relative_path; mod remove_dirent; +mod retry; mod symlink_dir; mod write_atomic; @@ -15,6 +16,7 @@ pub use lexical_normalize::lexical_normalize; pub use realpath_missing::realpath_missing; pub use relative_path::relative_path; pub use remove_dirent::remove_dirent; +pub use retry::{remove_dir_all_with_retry, rename_with_retry}; pub use symlink_dir::*; pub use write_atomic::{write_atomic, write_atomic_private}; diff --git a/pnpm/crates/fs/src/retry.rs b/pnpm/crates/fs/src/retry.rs new file mode 100644 index 0000000000..1976f3c54c --- /dev/null +++ b/pnpm/crates/fs/src/retry.rs @@ -0,0 +1,117 @@ +use std::{fs, io, path::Path}; + +#[cfg(any(windows, test))] +use std::time::{Duration, Instant}; + +#[cfg(any(windows, test))] +const RETRY_BUDGET: Duration = Duration::from_mins(1); +#[cfg(any(windows, test))] +const RETRY_BACKOFF_CAP: Duration = Duration::from_millis(100); + +/// Rename a filesystem entry, retrying transient Windows file-lock errors. +pub fn rename_with_retry(src: &Path, dst: &Path) -> io::Result<()> { + #[cfg(windows)] + { + retry_fs_operation(|| fs::rename(src, dst), is_transient_file_lock_error) + } + #[cfg(not(windows))] + { + fs::rename(src, dst) + } +} + +/// Remove a directory tree, retrying transient Windows file-lock errors. +pub fn remove_dir_all_with_retry(path: &Path) -> io::Result<()> { + #[cfg(windows)] + { + retry_fs_operation(|| fs::remove_dir_all(path), is_transient_file_lock_error) + } + #[cfg(not(windows))] + { + fs::remove_dir_all(path) + } +} + +#[cfg(any(windows, test))] +fn retry_fs_operation( + operation: Func, + is_transient: Classify, +) -> io::Result +where + Func: FnMut() -> io::Result, + Classify: Fn(&io::Error) -> bool, +{ + let start = Instant::now(); + retry_fs_operation_with_timing( + operation, + is_transient, + RetryTiming { + budget: RETRY_BUDGET, + elapsed: || start.elapsed(), + sleep: std::thread::sleep, + }, + ) +} + +#[cfg(any(windows, test))] +struct RetryTiming { + budget: Duration, + elapsed: Elapsed, + sleep: Sleep, +} + +#[cfg(any(windows, test))] +fn retry_fs_operation_with_timing( + mut operation: Func, + is_transient: Classify, + mut timing: RetryTiming, +) -> io::Result +where + Func: FnMut() -> io::Result, + Classify: Fn(&io::Error) -> bool, + Elapsed: FnMut() -> Duration, + Sleep: FnMut(Duration), +{ + let mut backoff = Duration::ZERO; + + loop { + match operation() { + Ok(value) => return Ok(value), + Err(error) => { + if !is_transient(&error) || (timing.elapsed)() >= timing.budget { + return Err(error); + } + let remaining = timing.budget.saturating_sub((timing.elapsed)()); + let delay = backoff.min(remaining); + if !delay.is_zero() { + (timing.sleep)(delay); + } + if (timing.elapsed)() >= timing.budget { + return Err(error); + } + backoff = (backoff + Duration::from_millis(10)).min(RETRY_BACKOFF_CAP); + } + } + } +} + +#[cfg(any(windows, test))] +fn is_transient_file_lock_error( + #[cfg_attr(not(windows), allow(unused, reason = "only inspected on Windows"))] + error: &io::Error, +) -> bool { + // Antivirus and indexer scans can briefly hold a Windows path open. The equivalent error + // kinds on Unix usually mean a permanent permissions or mount-point problem, so retrying + // them there would only delay the failure. + #[cfg(windows)] + { + matches!(error.kind(), io::ErrorKind::PermissionDenied | io::ErrorKind::ResourceBusy) + } + #[cfg(not(windows))] + { + false + } +} + +#[cfg(test)] +mod tests; diff --git a/pnpm/crates/fs/src/retry/tests.rs b/pnpm/crates/fs/src/retry/tests.rs new file mode 100644 index 0000000000..474f83cba2 --- /dev/null +++ b/pnpm/crates/fs/src/retry/tests.rs @@ -0,0 +1,112 @@ +use super::{ + RetryTiming, is_transient_file_lock_error, remove_dir_all_with_retry, rename_with_retry, + retry_fs_operation, retry_fs_operation_with_timing, +}; +use std::{cell::Cell, fs, io, time::Duration}; +use tempfile::tempdir; + +#[test] +fn retries_transient_errors_until_the_operation_succeeds() { + let attempts = Cell::new(0); + + let result = retry_fs_operation( + || { + let attempt = attempts.get(); + attempts.set(attempt + 1); + if attempt < 2 { + Err(io::Error::from(io::ErrorKind::PermissionDenied)) + } else { + Ok("done") + } + }, + |error| error.kind() == io::ErrorKind::PermissionDenied, + ); + + assert_eq!(result.unwrap(), "done"); + assert_eq!(attempts.get(), 3); +} + +#[test] +fn propagates_non_transient_errors_without_retrying() { + let attempts = Cell::new(0); + + let result: io::Result<()> = retry_fs_operation( + || { + attempts.set(attempts.get() + 1); + Err(io::Error::from(io::ErrorKind::NotFound)) + }, + |_| false, + ); + + assert_eq!(result.unwrap_err().kind(), io::ErrorKind::NotFound); + assert_eq!(attempts.get(), 1); +} + +#[test] +fn stops_retrying_at_the_budget_deadline() { + let attempts = Cell::new(0); + let elapsed = Cell::new(Duration::ZERO); + let budget = Duration::from_millis(15); + + let result: io::Result<()> = retry_fs_operation_with_timing( + || { + let attempt = attempts.get() + 1; + attempts.set(attempt); + Err(io::Error::other(format!("attempt {attempt}"))) + }, + |_| true, + RetryTiming { + budget, + elapsed: || elapsed.get(), + sleep: |delay| elapsed.set(elapsed.get() + delay), + }, + ); + + assert_eq!(result.unwrap_err().to_string(), "attempt 3"); + assert_eq!(attempts.get(), 3); + assert_eq!(elapsed.get(), budget); +} + +#[test] +fn transient_file_lock_error_classifier_is_windows_specific() { + for kind in [io::ErrorKind::PermissionDenied, io::ErrorKind::ResourceBusy] { + let error = io::Error::from(kind); + assert_eq!(is_transient_file_lock_error(&error), cfg!(windows), "{kind:?}"); + } + + for kind in [ + io::ErrorKind::NotFound, + io::ErrorKind::AlreadyExists, + io::ErrorKind::InvalidInput, + io::ErrorKind::InvalidData, + io::ErrorKind::Unsupported, + io::ErrorKind::Other, + ] { + assert!(!is_transient_file_lock_error(&io::Error::from(kind)), "{kind:?}"); + } +} + +#[test] +fn rename_with_retry_moves_the_entry() { + let root = tempdir().unwrap(); + let src = root.path().join("src"); + let dst = root.path().join("dst"); + fs::write(&src, b"payload").unwrap(); + + rename_with_retry(&src, &dst).expect("rename should succeed"); + + assert_eq!(fs::read(&dst).unwrap(), b"payload"); + assert!(!src.exists(), "source should be gone after rename"); +} + +#[test] +fn remove_dir_all_with_retry_removes_the_tree() { + let root = tempdir().unwrap(); + let target = root.path().join("target"); + fs::create_dir_all(target.join("nested")).unwrap(); + fs::write(target.join("nested/file"), b"payload").unwrap(); + + remove_dir_all_with_retry(&target).expect("remove should succeed"); + + assert!(!target.exists(), "directory tree should be gone after removal"); +}