From 29b767ba865c8d9e76f6f51da53481dd69640983 Mon Sep 17 00:00:00 2001 From: sacru2red Date: Mon, 10 Aug 2026 12:40:37 +0900 Subject: [PATCH] fix(windows): retry the post-extraction rename blocked by A/V handles `fnm install` on Windows could fail with "Can't download the requested binary: Access is denied. (os error 5)" long after the download and the extraction had both succeeded. Windows denies a directory rename while any file inside the tree has an open handle that does not grant FILE_SHARE_DELETE, which is what a real-time A/V scanner holds on the freshly extracted node.exe. Retry both post-extraction renames with bounded backoff, and stop reporting post-download filesystem failures as download errors: - add fs_retry::rename_with_retry, backing off 10ms at a time up to 100ms within a budget from FNM_RENAME_RETRY_TIMEOUT_MS (default 5000, 0 disables). is_transient_lock is cfg(windows) and matches ERROR_ACCESS_DENIED, ERROR_SHARING_VIOLATION and ERROR_LOCK_VIOLATION; elsewhere it is always false, so behaviour is unchanged. - use it in downloader::install_node_dist and DirectoryPortal::teleport. - add downloader::Error::CantMoveIntoPlace for the three post-download filesystem steps and forward it transparently through commands::install instead of wrapping it in DownloadError. The new tests pin the condition without an A/V product installed by opening node.exe with share_mode(FILE_SHARE_READ). Refs #1583, #1193 Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/windows-rename-retry.md | 11 +++ src/commands/install.rs | 5 + src/directory_portal.rs | 2 +- src/downloader.rs | 15 ++- src/fs_retry.rs | 149 +++++++++++++++++++++++++++++ src/main.rs | 1 + 6 files changed, 178 insertions(+), 5 deletions(-) create mode 100644 .changeset/windows-rename-retry.md create mode 100644 src/fs_retry.rs diff --git a/.changeset/windows-rename-retry.md b/.changeset/windows-rename-retry.md new file mode 100644 index 000000000..bd0863275 --- /dev/null +++ b/.changeset/windows-rename-retry.md @@ -0,0 +1,11 @@ +--- +"fnm": patch +--- + +Retry the post-extraction directory rename on Windows when an A/V handle blocks it + +On Windows, `fnm install` could fail with `Can't download the requested binary: Access is denied. (os error 5)` even though the download and the extraction both succeeded. Windows denies a directory rename while any file inside the tree has an open handle that does not grant `FILE_SHARE_DELETE`, which is what a real-time A/V scanner holds on the freshly extracted `node.exe`. + +Both renames that follow extraction are now retried with bounded backoff. The budget defaults to 5000ms and can be tuned with `FNM_RENAME_RETRY_TIMEOUT_MS` (`0` disables retrying). Filesystem failures after the download also stopped reporting themselves as download errors. + +Refs #1583, #1193 diff --git a/src/commands/install.rs b/src/commands/install.rs index 6db7a81cc..b51984e80 100644 --- a/src/commands/install.rs +++ b/src/commands/install.rs @@ -154,6 +154,9 @@ impl Command for Install { Err(err @ DownloaderError::VersionAlreadyInstalled { .. }) => { outln!(config, Error, "{} {}", "warning:".bold().yellow(), err); } + Err(source @ DownloaderError::CantMoveIntoPlace { .. }) => { + Err(Error::CantMoveIntoPlace { source })?; + } Err(source) => Err(Error::DownloadError { source })?, Ok(()) => {} } @@ -226,6 +229,8 @@ pub enum Error { #[error("Can't download the requested binary: {}", source)] DownloadError { source: DownloaderError }, #[error(transparent)] + CantMoveIntoPlace { source: DownloaderError }, + #[error(transparent)] IoError { #[from] source: std::io::Error, diff --git a/src/directory_portal.rs b/src/directory_portal.rs index 5a3f85611..41fda84e4 100644 --- a/src/directory_portal.rs +++ b/src/directory_portal.rs @@ -28,7 +28,7 @@ impl> DirectoryPortal

{ self.temp_dir.path().display(), self.target.as_ref().display() ); - std::fs::rename(&self.temp_dir, &self.target)?; + crate::fs_retry::rename_with_retry(self.temp_dir.path(), self.target.as_ref())?; Ok(self.target) } } diff --git a/src/downloader.rs b/src/downloader.rs index f9f588752..4c2aa411e 100644 --- a/src/downloader.rs +++ b/src/downloader.rs @@ -33,6 +33,8 @@ pub enum Error { VersionNotFound { version: Version, arch: Arch }, #[error("Version already installed at {:?}", path)] VersionAlreadyInstalled { path: PathBuf }, + #[error("Can't move the extracted files into place: {}", source)] + CantMoveIntoPlace { source: std::io::Error }, } #[cfg(unix)] @@ -110,15 +112,20 @@ pub fn install_node_dist>( } debug!("Extraction completed"); - let installed_directory = std::fs::read_dir(&portal)? + let installed_directory = std::fs::read_dir(&portal) + .map_err(|source| Error::CantMoveIntoPlace { source })? .next() - .ok_or(Error::TarIsEmpty)??; + .ok_or(Error::TarIsEmpty)? + .map_err(|source| Error::CantMoveIntoPlace { source })?; let installed_directory = installed_directory.path(); let renamed_installation_dir = portal.join("installation"); - std::fs::rename(installed_directory, renamed_installation_dir)?; + crate::fs_retry::rename_with_retry(&installed_directory, &renamed_installation_dir) + .map_err(|source| Error::CantMoveIntoPlace { source })?; - portal.teleport()?; + portal + .teleport() + .map_err(|source| Error::CantMoveIntoPlace { source })?; return Ok(()); } diff --git a/src/fs_retry.rs b/src/fs_retry.rs new file mode 100644 index 000000000..9bd359b68 --- /dev/null +++ b/src/fs_retry.rs @@ -0,0 +1,149 @@ +use log::debug; +use std::path::Path; +use std::time::{Duration, Instant}; + +const DEFAULT_BUDGET_MS: u64 = 5_000; +const BACKOFF_STEP_MS: u64 = 10; +const MAX_BACKOFF_MS: u64 = 100; + +fn budget_from_env() -> Duration { + let millis = std::env::var("FNM_RENAME_RETRY_TIMEOUT_MS") + .ok() + .and_then(|value| value.parse::().ok()) + .unwrap_or(DEFAULT_BUDGET_MS); + Duration::from_millis(millis) +} + +/// On Windows a directory rename is denied while any file inside the tree has an open handle +/// that does not grant `FILE_SHARE_DELETE` -- which is what a real-time A/V scanner holds on a +/// freshly extracted `node.exe`. The scan releases it on its own, so these are transient. +#[cfg(windows)] +fn is_transient_lock(err: &std::io::Error) -> bool { + // ERROR_ACCESS_DENIED (5), ERROR_SHARING_VIOLATION (32), ERROR_LOCK_VIOLATION (33) + matches!(err.raw_os_error(), Some(5 | 32 | 33)) +} + +#[cfg(not(windows))] +fn is_transient_lock(_err: &std::io::Error) -> bool { + false +} + +/// `std::fs::rename`, retried for as long as it keeps failing with a transient lock. +/// +/// The budget is read from `FNM_RENAME_RETRY_TIMEOUT_MS` (milliseconds, `0` disables retrying). +/// No error is transient outside Windows, so everywhere else this is a plain `std::fs::rename`. +pub fn rename_with_retry(from: &Path, to: &Path) -> std::io::Result<()> { + rename_with_budget(from, to, budget_from_env()) +} + +/// Same as [`rename_with_retry`], but with an explicit budget so tests don't have to mutate +/// process-global environment variables. +fn rename_with_budget(from: &Path, to: &Path, budget: Duration) -> std::io::Result<()> { + let start = Instant::now(); + let mut backoff = Duration::ZERO; + + loop { + match std::fs::rename(from, to) { + Ok(()) => return Ok(()), + Err(err) => { + if !is_transient_lock(&err) || start.elapsed() >= budget { + return Err(err); + } + + debug!( + "Moving {} into {} is blocked ({}), retrying in {}ms", + from.display(), + to.display(), + err, + backoff.as_millis() + ); + + std::thread::sleep(backoff); + backoff = (backoff + Duration::from_millis(BACKOFF_STEP_MS)) + .min(Duration::from_millis(MAX_BACKOFF_MS)); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test_log::test] + fn renames_without_retrying_when_nothing_blocks() { + let root = tempfile::tempdir().expect("Can't generate a temp directory"); + let source = root.path().join("source"); + let target = root.path().join("target"); + std::fs::create_dir(&source).expect("Can't create the source directory"); + + rename_with_budget(&source, &target, Duration::ZERO) + .expect("An unblocked rename must not need the retry budget"); + + assert!(target.is_dir()); + } +} + +#[cfg(all(test, windows))] +mod windows_tests { + use super::*; + use std::fs::{File, OpenOptions}; + use std::os::windows::fs::OpenOptionsExt; + use std::path::PathBuf; + + /// `FILE_SHARE_READ` alone deliberately withholds `FILE_SHARE_DELETE`, which is what makes + /// the rename of the parent directory fail the way it fails behind an A/V scanner. + const FILE_SHARE_READ: u32 = 0x0000_0001; + + fn locked_tree() -> (tempfile::TempDir, PathBuf, File) { + let root = tempfile::tempdir().expect("Can't generate a temp directory"); + let source = root.path().join("source"); + std::fs::create_dir_all(source.join("inner")).expect("Can't create the source tree"); + + let victim = source.join("inner").join("node.exe"); + std::fs::write(&victim, b"stub").expect("Can't write the stub file"); + let handle = OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ) + .open(&victim) + .expect("Can't open the stub file with a scanner-like share mode"); + + (root, source, handle) + } + + #[test_log::test] + fn retries_until_the_handle_is_released() { + let (root, source, handle) = locked_tree(); + let target = root.path().join("target"); + + let releaser = std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(300)); + drop(handle); + }); + + rename_with_budget(&source, &target, Duration::from_secs(5)) + .expect("The retry budget should outlast a 300ms lock"); + + releaser.join().expect("Can't join the releasing thread"); + assert!(target.join("inner").join("node.exe").exists()); + } + + #[test_log::test] + fn does_not_retry_when_the_budget_is_zero() { + let (root, source, handle) = locked_tree(); + let target = root.path().join("target"); + + let err = rename_with_budget(&source, &target, Duration::ZERO) + .expect_err("A zero budget must return the original error immediately"); + + // Windows reports ERROR_ACCESS_DENIED (5) here; ERROR_SHARING_VIOLATION (32) is the + // same class of transient lock, so accept either. + assert!( + matches!(err.raw_os_error(), Some(5 | 32)), + "unexpected error: {err:?}" + ); + + drop(handle); + drop(root); + } +} diff --git a/src/main.rs b/src/main.rs index 6ab10eab5..3b75f3dc1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,6 +17,7 @@ mod current_version; mod directory_portal; mod downloader; mod fs; +mod fs_retry; mod http; mod installed_versions; mod lts;