Skip to content

ai slop #36955

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.

Sign up for GitHub

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

Closed
robobun wants to merge 18 commits into from
Closed

ai slop #36955

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8d50ddf
install: tolerate concurrent installs racing on the same destination …
robobun Aug 5, 2026
b582f96
test: concurrent installs into the same destination tolerate racing h…
robobun Aug 5, 2026
7ee45be
install: keep the existing cache entry when a concurrent extract move…
robobun Aug 5, 2026
422ac38
install: retry transient ENOENT when a peer renames the destination d…
robobun Aug 5, 2026
f27a19b
test: cover cold-cache concurrent extraction in the racing installs test
robobun Aug 5, 2026
897335a
install: survive destination renames from concurrent installs on POSIX
robobun Aug 5, 2026
377f91e
[autofix.ci] apply automated fixes
autofix-ci[bot] Aug 5, 2026
b464718
Tighten comments
robobun Aug 5, 2026
6eb3a5a
install: probe package.json for tarball cache entries; assert file co…
robobun Aug 5, 2026
a3ff14f
install: retry racing destination-dir opens and stage the POSIX backoff
robobun Aug 5, 2026
c99bfd9
install: accept an already-identical link in the isolated linker
robobun Aug 5, 2026
9da7429
Allow unused mut on the Windows arm
robobun Aug 5, 2026
51da66d
install: recover from EINVAL for an unlinked destination dirfd on macOS
robobun Aug 5, 2026
06c4eae
install: symlink backend EEXIST retry must link to the cache target, …
robobun Aug 5, 2026
0907aac
install: give the macOS clonefile copy loop the same destination-rena…
robobun Aug 5, 2026
f6eadd8
[autofix.ci] apply automated fixes
autofix-ci[bot] Aug 5, 2026
d4b9960
install: propagate non-transient destination re-open errors
robobun Aug 5, 2026
9e99ab2
[autofix.ci] apply automated fixes
autofix-ci[bot] Aug 5, 2026
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
156 changes: 126 additions & 30 deletions src/install/PackageInstall.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,6 @@
}

fn run(&mut self) -> Option<crate::Error> {
use bun_sys::windows;
// Read scalar fields before borrowing `bytes` so no `&mut self` reborrow
// overlaps the slice borrows below.
let src_len = self.src_len;
Expand All @@ -626,6 +625,41 @@
let dest_len = dest.len() - 1;
debug_assert_eq!(dest[dest_len], 0);

// Peers installing the same package cause transient EBUSY (file
// briefly held open) and ENOENT (destination dir renamed aside by
// uninstall-before-install); retry with the same backoff
// extract_tarball uses for cache moves.
Comment thread
robobun marked this conversation as resolved.
const MAX_RETRIES: u32 = 4;
let mut retries: u32 = 0;
loop {
let err = match Self::link_or_copy(src, dest, dest_len, basename) {
None => return None,
Some(err) => err,
};
let transient = matches!(
err,
crate::Error::Sys(bun_errno::SystemErrno::EBUSY)
| crate::Error::Sys(bun_errno::SystemErrno::ENOENT)
);
if !transient || retries == MAX_RETRIES {
return Some(err);
}
retries += 1;
std::thread::sleep(std::time::Duration::from_millis(10u64 << (retries - 1)));
}
}

/// One attempt at materializing `dest` as a hard link of `src`, falling
/// back to a copy. A `dest` that already is the same file object as `src`
/// (a peer linked it first) is success, not a conflict.
Comment thread
robobun marked this conversation as resolved.
fn link_or_copy(
src: &[u16],
dest: &mut [u16],
dest_len: usize,
basename: usize,
) -> Option<crate::Error> {
use bun_sys::windows;

// `windows::CreateHardLinkW` is the safe wrapper (logs + Option<&mut SA>).
if windows::CreateHardLinkW(dest.as_ptr(), src.as_ptr(), None) != 0 {
return None;
Expand All @@ -635,13 +669,8 @@
windows::Win32Error::ALREADY_EXISTS
| windows::Win32Error::FILE_EXISTS
| windows::Win32Error::CANNOT_MAKE => {
// Race condition: this shouldn't happen
if cfg!(debug_assertions) {
bun_output::scoped_log!(
install,
"CreateHardLinkW returned EEXIST, this shouldn't happen: {}",
bun_core::fmt::fmt_path_u16(&dest[..dest_len], Default::default())
);
if windows::same_file_w(dest.as_ptr(), src.as_ptr()) {
return None;
}
// SAFETY: FFI — dest is a valid NUL-terminated u16 buffer.
unsafe { windows::DeleteFileW(dest.as_ptr()) };
Expand All @@ -662,6 +691,12 @@
return None;
}

// If the DeleteFileW above failed, dest may still be the link we
// want; CopyFileW onto it would copy the file over itself and fail.
Comment thread
robobun marked this conversation as resolved.
if windows::same_file_w(dest.as_ptr(), src.as_ptr()) {
return None;
}

if PackageManager::verbose_install() {
bun_core::run_once! {{
bun_core::warn!(
Expand Down Expand Up @@ -1582,12 +1617,24 @@
#[cfg(not(windows))]
type WinOffset = ();

#[cfg(windows)]
type PosixDirRef<'b> = ();
#[cfg(not(windows))]
type PosixDirRef<'b> = &'b Dir;
#[cfg(windows)]
type PosixZStrRef<'b> = ();
#[cfg(not(windows))]
type PosixZStrRef<'b> = &'b ZStr;

// Two overlapping slices into the same buffer (`head` is the whole
// buffer, `to_copy_into` is its tail) would be two live aliasing
// `&mut [u16]`, which is UB — pass head buffer + tail offset and
// reslice inside.
// reslice inside. `destbase`/`destpath` (posix) re-open
// `destination_dir` if a peer renames it away.
Comment thread
robobun marked this conversation as resolved.
fn copy(
destination_dir: &Dir,
destbase: PosixDirRef<'_>,
destpath: PosixZStrRef<'_>,
destination_dir: &mut Dir,
walker: &mut Walker,
to_copy_into1_offset: WinOffset,
head1: WinSlice<'_>,
Expand All @@ -1598,7 +1645,7 @@
#[cfg(not(windows))]
let _ = (to_copy_into1_offset, head1, to_copy_into2_offset, head2);
#[cfg(windows)]
let _ = destination_dir;
let _ = (destination_dir, destbase, destpath);
#[cfg(windows)]
let queue = HardLinkWindowsInstallTask::init_queue();
// on Windows, tasks already pushed to `queue` are running on
Expand Down Expand Up @@ -1626,7 +1673,7 @@
match entry.kind {
EntryKind::Directory => {
let _ = bun_sys::MakePath::make_path::<OSPathChar>(
destination_dir,
&*destination_dir,
entry.path.as_bytes(),
);
}
Expand All @@ -1644,24 +1691,69 @@
}
}

if let Err(err) = sys::linkat(
entry.dir,
entry.basename,
destination_dir.fd(),
entry.path,
) {
if err.get_errno() == sys::E::EEXIST {
let _ = sys::unlinkat(destination_dir, entry.path);
sys::linkat(
entry.dir,
entry.basename,
destination_dir.fd(),
entry.path,
)
.map_err(map_linkat_err)?;
} else {
// Peers installing the same package cause
// EEXIST (one already linked this file; same
// inode is success) and ENOENT (destination dir
// renamed aside by uninstall-before-install).
// Fix up and retry immediately; no sleeps on the
// install thread.
Comment thread
robobun marked this conversation as resolved.
Outdated
const MAX_RETRIES: u32 = 8;
let mut retries: u32 = 0;
loop {
let err = match sys::linkat(
entry.dir,
entry.basename,
destination_dir.fd(),
entry.path,
) {
Ok(()) => break,
Comment thread
robobun marked this conversation as resolved.
Err(err) => err,
};
match err.get_errno() {
sys::E::EEXIST => {
if let (Ok(src), Ok(dest)) = (
sys::fstatat(entry.dir, entry.basename),
sys::fstatat(&*destination_dir, entry.path),
) {
if src.st_dev == dest.st_dev
&& src.st_ino == dest.st_ino
{
break;
}
}
let _ = sys::unlinkat(&*destination_dir, entry.path);
}

Check notice on line 1725 in src/install/PackageInstall.rs

View check run for this annotation

Claude / Claude Code Review

install_with_symlink EEXIST retry passes wrong target, creating self-referential symlinks 

Pre-existing, not touched by this PR — surfacing under the same 'fix the whole class' rule as the Hardlinker.rs note: `install_with_symlink`'s POSIX EEXIST retry (PackageInstall.rs:1948) passes `entry.basename` as the symlink target instead of `target`, so the retry creates `<dest>/<entry.path>` → `<basename>`, which resolves to itself (ELOOP on read). Any `bun install --backend symlink` where a destination file already exists — the concurrent-install scenario this PR handles for hardlink — inst
Comment thread
robobun marked this conversation as resolved.
sys::E::ENOENT => {
// The held fd may be an unlinked dir
// (peer renamed it aside and deleted
// it); re-open by path.
Comment thread
robobun marked this conversation as resolved.
if let Ok(reopened) = destbase.make_open_path(
destpath.as_bytes(),
OpenDirOptions {
iterate: true,
..Default::default()
},
) {
*destination_dir = reopened;
}
let entry_dirname = bun_paths::resolve_path::dirname::<
bun_paths::platform::Auto,
>(
entry.path.as_bytes()
);
if !entry_dirname.is_empty() {
let _ = bun_sys::MakePath::make_path::<OSPathChar>(
&*destination_dir,
entry_dirname,
);
}
}

Check warning on line 1750 in src/install/PackageInstall.rs

View check run for this annotation

Claude / Claude Code Review

install_with_copyfile (hardlink's direct EXDEV fallback) still Global::crash()s on the same rename-away race 

🟡 Same-class sibling: `install_with_copyfile` — the direct EXDEV fallback from `install_with_hardlink` on this same dispatch (L2572→2611) — has the identical stale-dirfd race and still ends in `Global::crash()` (POSIX L1496-1534, Windows L1433-1473) instead of getting the reopen-from-`dest_dir` recovery added here. Pre-existing and narrower (needs cache and node_modules on different filesystems, or explicit `--backend copyfile`), so not blocking; consider giving copyfile the same ENOENT recovery
Comment thread
robobun marked this conversation as resolved.
Outdated
_ => return Err(map_linkat_err(err)),
}
if retries == MAX_RETRIES {
return Err(map_linkat_err(err));
}
retries += 1;
}

real_file_count += 1;
Expand Down Expand Up @@ -1727,7 +1819,9 @@

#[cfg(windows)]
let result = copy(
&state.subdir,
(),
(),
&mut state.subdir,
state.walker.as_mut().unwrap(),
state.to_copy_buf_off,
&mut state.buf[..],
Expand All @@ -1736,7 +1830,9 @@
);
#[cfg(not(windows))]
let result = copy(
&state.subdir,
dest_dir,
self.destination_dir_subpath,
&mut state.subdir,
state.walker.as_mut().unwrap(),
(),
(),
Expand Down
123 changes: 76 additions & 47 deletions src/install/extract_tarball.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,59 +573,88 @@ impl ExtractTarball {
true,
) {
bun_sys::Result::Err(err) => {
if retries < MAX_RETRIES {
match err.get_errno() {
sys::Errno::NOTEMPTY
let collided = matches!(
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());
}
| sys::Errno::EXIST
);
if collided {
// before we attempt to touch the destination, let's close the source dir.
let _ = sys::close(dir_to_move);

// An existing destination is a complete entry
// from a concurrent install (entries only
// appear via atomic rename): keep it rather
// than deleting files a peer is copying out of
// the cache. Incomplete leftovers are replaced
// (npm/tarball entries always have a
// package.json; git deps may not).
Comment thread
robobun marked this conversation as resolved.
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());
let keep_existing = match self.resolution.tag {
ResolutionTag::Npm
| ResolutionTag::LocalTarball
| ResolutionTag::RemoteTarball => {
let mut pkg_json_buf = PathBuffer::uninit();
let pkg_json = path::resolve_path::join_z_buf::<
path::platform::Auto,
>(
&mut pkg_json_buf.0,
&[folder_name, b"package.json"],
);
sys::exists_at(Fd::from_std_dir(cache_dir), pkg_json)
}
_ => sys::directory_exists_at(
Fd::from_std_dir(cache_dir),
folder_name_z,
)
.unwrap_or(false),
};
if keep_existing {
let _ = tmpdir.delete_tree(tmpname.as_bytes());
break;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if retries < MAX_RETRIES {
// Replace it: rename it into the temp dir
// first (harder for an application to
// reach mid-delete), then delete it there.
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);
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;
}
_ => {}
retries += 1;
// 10ms, 20ms, 40ms, 80ms — long enough
// for a concurrent close to land,
// short enough to not slow a legit
// failure noticeably.
Comment thread
robobun marked this conversation as resolved.
std::thread::sleep(std::time::Duration::from_millis(
10u64 << (retries - 1),
));
continue;
}
} else {
let _ = sys::close(dir_to_move);
}
let _ = sys::close(dir_to_move);
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
Expand Down
Loading
Loading