Skip to content
7 changes: 7 additions & 0 deletions src/bun_core/env_var.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ new!(pub BUN_INSTALL_STREAMING_MIN_SIZE: unsigned, "BUN_INSTALL_STREAMING_MIN_SI
// thread schedules a drain; collapses the per-chunk thread-pool futex wake
// into roughly one per `threshold` bytes.
new!(pub BUN_INSTALL_STREAMING_DRAIN_THRESHOLD: unsigned, "BUN_INSTALL_STREAMING_DRAIN_THRESHOLD", { default: 256 * 1024 });
// How long (ms) `bun install` keeps retrying a rename into the cache on
// Windows while a scanner holds a file in the directory open (see
// `bun_install::cache_rename`). 5s covers the real-time scan of a
// multi-megabyte binary with margin (SQLite's equivalent retry waits 1.4s,
// graceful-fs 60s); it is also what a permanent failure such as an unwritable
// cache dir now costs per package before it is reported. 0 disables retrying.
Comment thread
robobun marked this conversation as resolved.
Outdated
new!(pub BUN_INSTALL_WINDOWS_RENAME_RETRY_MS: unsigned, "BUN_INSTALL_WINDOWS_RENAME_RETRY_MS", { default: 5_000 });
new!(pub BUN_NEEDS_PROC_SELF_WORKAROUND: boolean, "BUN_NEEDS_PROC_SELF_WORKAROUND", { default: false });
new!(pub BUN_OPTIONS: string, "BUN_OPTIONS", {});
new!(pub BUN_POSTGRES_SOCKET_MONITOR: string, "BUN_POSTGRES_SOCKET_MONITOR", {});
Expand Down
101 changes: 101 additions & 0 deletions src/install/cache_rename.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
//! Retry budget shared by the renames that publish a freshly built directory
//! into the install cache: an extracted tarball, a patched package, or a
//! global virtual store entry.
//!
//! On Windows, renaming a directory fails with `STATUS_ACCESS_DENIED` or
//! `STATUS_SHARING_VIOLATION` while any other process holds a handle without
//! `FILE_SHARE_DELETE` on a file inside it. Antivirus, the Search Indexer and
//! endpoint agents open freshly written files exactly that way, typically for
//! tens of milliseconds up to a few seconds, so the rename is retried until
//! `BUN_INSTALL_WINDOWS_RENAME_RETRY_MS` has elapsed. POSIX renames are not
//! affected by open handles and `EPERM`/`EACCES` are real permission failures
//! there, so nothing is ever retried off Windows.
Comment thread
robobun marked this conversation as resolved.
Outdated

use core::fmt;
use core::time::Duration;
use std::time::Instant;

use bun_sys as sys;

pub(crate) const ENV_VAR_NAME: &str = "BUN_INSTALL_WINDOWS_RENAME_RETRY_MS";

Check warning on line 20 in src/install/cache_rename.rs

View check run for this annotation

Claude / Claude Code Review

ENV_VAR_NAME string literal duplicates the canonical name in env_var.rs

`ENV_VAR_NAME` hard-codes `"BUN_INSTALL_WINDOWS_RENAME_RETRY_MS"` as a second string literal, but the env-var machinery already exposes the canonical name via `bun_core::env_var::BUN_INSTALL_WINDOWS_RENAME_RETRY_MS.key()` (see `HOME.key()` at create_command.rs:1812). This var was already renamed once during review (`WIN32_AV` → `WINDOWS_RENAME`, commit 127c5eb5); a future rename that misses this literal leaves the user-facing hint in `ExhaustedHint::fmt` naming a nonexistent variable. Secondaril

Check warning on line 20 in src/install/cache_rename.rs

View check run for this annotation

Claude / Claude Code Review

PR description stale again after 9ee07e05: env var and test file were renamed

The PR description is stale again after 9ee07e05: the **Fix** section names `BUN_INSTALL_WIN32_AV_RETRY_MS` but the env var is now `BUN_INSTALL_WINDOWS_RENAME_RETRY_MS` (env_var.rs:114, cache_rename.rs:20); the **Verification** section names `test/cli/install/bun-install-windows-locked-temp.test.ts` but the file is `bun-install-windows-rename-retry.test.ts`, describes a 500ms hold when `HOLD_MS = 2000`, and covers only the tarball rename when the fix now also covers `patch_install` and the isola
Comment thread
robobun marked this conversation as resolved.
Outdated
Comment thread
robobun marked this conversation as resolved.
Outdated

pub(crate) struct RenameRetry {
started: Instant,
budget: Duration,
/// Sleep before the next attempt; grows 10ms per attempt and caps at 100ms,
/// which is the schedule npm's `graceful-fs` uses for the same failure.
Comment thread
robobun marked this conversation as resolved.
Outdated
next_backoff: Duration,
exhausted: bool,
}

impl RenameRetry {
pub(crate) fn start() -> Self {
Self {
started: Instant::now(),
budget: Duration::from_millis(
bun_core::env_var::BUN_INSTALL_WINDOWS_RENAME_RETRY_MS
.get()
.unwrap_or(5_000),
),
next_backoff: Duration::ZERO,
exhausted: false,
}
}

/// Whether `err` is one of the errors Windows reports while another process
/// holds a handle inside the directory being renamed or at its destination.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn is_transient(err: &sys::Error) -> bool {
cfg!(windows)
&& matches!(
err.get_errno(),
sys::Errno::EPERM | sys::Errno::EACCES | sys::Errno::EBUSY
)
}

/// Called after a failed attempt. Sleeps and returns `true` while the budget
/// allows another attempt; returns `false` once it is spent.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn wait(&mut self) -> bool {
if self.started.elapsed() >= self.budget {
self.exhausted = true;
return false;
}
self.next_backoff =
(self.next_backoff + Duration::from_millis(10)).min(Duration::from_millis(100));
std::thread::sleep(self.next_backoff);
true
}

pub(crate) fn exhausted(&self) -> bool {
self.exhausted
}

/// Suffix for the error reported to the user once `wait()` has returned
/// `false`; displays as nothing otherwise.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn exhausted_hint(&self) -> ExhaustedHint {
ExhaustedHint {
waited: if self.exhausted {
Some(self.started.elapsed())
} else {
None
},
}
}
}

pub(crate) struct ExhaustedHint {
waited: Option<Duration>,
}

impl fmt::Display for ExhaustedHint {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.waited {
Some(waited) => write!(
f,
" (gave up after retrying for {}ms; another process is holding a file open in the directory. Set {} to wait longer)",
waited.as_millis(),
ENV_VAR_NAME,
),
None => Ok(()),
}
}
}
105 changes: 48 additions & 57 deletions src/install/extract_tarball.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ use bun_libarchive::{ArchiveAppender, ExtractOptions};
use bun_resolver::fs::FileSystem;
#[cfg(windows)]
use bun_sys::FdDirExt;

#[cfg(windows)]
use crate::cache_rename::RenameRetry;
type Error = crate::Error;

pub struct ExtractTarball {
Expand Down Expand Up @@ -522,12 +525,14 @@ impl ExtractTarball {
// Now that we've extracted the archive, we rename.
#[cfg(windows)]
{
// Windows EBUSY/SHARING_VIOLATION on `NtSetInformationFile` is
// transient when a concurrent process (another `bun install`
// sharing the cache, AV, the Search Indexer) is closing its
// handle to the destination. Back off briefly between retries.
const MAX_RETRIES: u32 = 4;
let mut retries: u32 = 0;
// The rename fails transiently when another process holds a
// handle into either directory: a concurrent `bun install`
// sharing the cache still has the destination open (EXIST /
// NOTEMPTY, or PERM since NTFS reports replacing a directory
// that way too), or a scanner has one of our freshly extracted
// files open (PERM / BUSY, see `cache_rename`). Both are
// retried against the same `RenameRetry` budget.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut retry = RenameRetry::start();
let mut path2_buf = WPathBuffer::uninit();
let path2 = strings::to_wpath_normalized(&mut path2_buf, folder_name);
if create_subdir {
Expand Down Expand Up @@ -573,65 +578,51 @@ impl ExtractTarball {
true,
) {
bun_sys::Result::Err(err) => {
if retries < MAX_RETRIES {
match err.get_errno() {
sys::Errno::NOTEMPTY
| sys::Errno::PERM
| sys::Errno::BUSY
| sys::Errno::EXIST => {
// before we attempt to delete the destination, let's close the source dir.
let _ = sys::close(dir_to_move);

// We tried to move the folder over
// but it didn't work!
// so instead of just simply deleting the folder
// we rename it back into the temp dir
// and then delete that temp dir
// The goal is to make it more difficult for an application to reach this folder
let mut tempdest_buf = PathBuffer::uninit();
tempdest_buf[0..tmpname.len()]
.copy_from_slice(tmpname.as_bytes());
tempdest_buf[tmpname.len()..][0..4]
.copy_from_slice(&[b't', b'm', b'p', 0]);
let tempdest =
ZStr::from_buf(&tempdest_buf, tmpname.len() + 3);
let mut folder_name_z_buf = PathBuffer::uninit();
folder_name_z_buf[0..folder_name.len()]
.copy_from_slice(folder_name);
folder_name_z_buf[folder_name.len()] = 0;
let folder_name_z =
ZStr::from_buf(&folder_name_z_buf, folder_name.len());
match sys::renameat(
Fd::from_std_dir(cache_dir),
folder_name_z,
Fd::from_std_dir(tmpdir),
tempdest,
) {
bun_sys::Result::Err(_) => {}
bun_sys::Result::Ok(_) => {
let _ = tmpdir.delete_tree(tempdest.as_bytes());
}
}
retries += 1;
// 10ms, 20ms, 40ms, 80ms — long enough
// for a concurrent close to land,
// short enough to not slow a legit
// failure noticeably.
std::thread::sleep(std::time::Duration::from_millis(
10u64 << (retries - 1),
));
continue;
// before we attempt to delete the destination, let's close the source dir.
let _ = sys::close(dir_to_move);

let retryable = RenameRetry::is_transient(&err)
|| matches!(
err.get_errno(),
sys::Errno::NOTEMPTY | sys::Errno::EXIST
);
if retryable && retry.wait() {
// We tried to move the folder over
// but it didn't work!
// so instead of just simply deleting the folder
// we rename it back into the temp dir
// and then delete that temp dir
// The goal is to make it more difficult for an application to reach this folder
Comment thread
robobun marked this conversation as resolved.
let mut tempdest_buf = PathBuffer::uninit();
tempdest_buf[0..tmpname.len()].copy_from_slice(tmpname.as_bytes());
tempdest_buf[tmpname.len()..][0..4]
.copy_from_slice(&[b't', b'm', b'p', 0]);
let tempdest = ZStr::from_buf(&tempdest_buf, tmpname.len() + 3);
let mut folder_name_z_buf = PathBuffer::uninit();
folder_name_z_buf[0..folder_name.len()].copy_from_slice(folder_name);
folder_name_z_buf[folder_name.len()] = 0;
let folder_name_z =
ZStr::from_buf(&folder_name_z_buf, folder_name.len());
match sys::renameat(
Fd::from_std_dir(cache_dir),
folder_name_z,
Fd::from_std_dir(tmpdir),
tempdest,
) {
bun_sys::Result::Err(_) => {}
bun_sys::Result::Ok(_) => {
let _ = tmpdir.delete_tree(tempdest.as_bytes());
}
_ => {}
}
continue;
}
let _ = sys::close(dir_to_move);
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!(
"moving \"{}\" to cache dir failed\n{}\n From: {}\n To: {}",
"moving \"{}\" to cache dir failed{}\n{}\n From: {}\n To: {}",
bun_fmt::s(name),
retry.exhausted_hint(),
err,
bun_fmt::s(tmpname.as_bytes()),
bun_fmt::s(folder_name),
Expand Down
Loading