fix(deps-restorer): retry Windows staging swaps (#14366)

Hoisted installs replace a package by deleting the existing target and renaming
a fully prepared staging directory into its place. Windows antivirus and indexer
processes can briefly hold either path open, causing `PermissionDenied` or
`ResourceBusy` even though the operation succeeds moments later.

Extract the existing retrying rename into shared filesystem helpers and use the
same bounded retry policy for recursive removal. Apply both helpers to the
hoisted staging swap while retaining immediate failure for non-transient errors
and for all equivalent Unix errors. Cap the final backoff at the remaining retry
budget and check the deadline before starting another filesystem attempt.

Fixes pnpm/pnpm#14349.

.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).

pnpm/crates/deps-restorer/src/import_indexed_dir.rs

@@ -680,7 +680,7 @@ fn stage_and_swap<Reporter: self::Reporter>(
     //    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<Reporter: self::Reporter>(
     //    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.

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.

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();

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};
 

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<Func, Value, Classify>(
+    operation: Func,
+    is_transient: Classify,
+) -> io::Result<Value>
+where
+    Func: FnMut() -> io::Result<Value>,
+    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<Elapsed, Sleep> {
+    budget: Duration,
+    elapsed: Elapsed,
+    sleep: Sleep,
+}
+
+#[cfg(any(windows, test))]
+fn retry_fs_operation_with_timing<Func, Value, Classify, Elapsed, Sleep>(
+    mut operation: Func,
+    is_transient: Classify,
+    mut timing: RetryTiming<Elapsed, Sleep>,
+) -> io::Result<Value>
+where
+    Func: FnMut() -> io::Result<Value>,
+    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;

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");
+}