From 3921ba845fa1271681f674e8d56f6120b35507ae Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 11 Jul 2026 12:24:49 +0000 Subject: [PATCH 1/7] install: don't leak package extraction temp directories into $TMPDIR When two bun install processes raced the same cache entry, the loser's rename fell back to RENAME_EXCHANGE, which swapped its temp directory with the winner's cache entry and left the swapped-out tree stranded in the temp dir forever. On long-lived machines this accumulated a full extracted package copy per raced install (reported at ~25 GB over 40 hours of CI). Deleting the swapped-out tree after the exchange is not safe either: the winner may still hold an open fd into that tree while hardlinking it into node_modules, and deleting it under the winner fails its install with ENOENT. Instead, treat the publish race as first writer wins: a new keep_existing_destination option on renameat_concurrently keeps the existing (equivalent) destination and deletes the caller's private temp copy, which no other process can be reading. Also clean up the temp directory on extraction and patch-apply error paths, which previously leaked a partially extracted tree per failed attempt. Fixes #33977 --- src/install/PackageManager/patchPackage.rs | 7 +- src/install/extract_tarball.rs | 18 +- src/install/patch_install.rs | 8 +- src/sys/lib.rs | 74 +++++- .../bun-install-tempdir-cleanup.test.ts | 219 ++++++++++++++++++ 5 files changed, 317 insertions(+), 9 deletions(-) create mode 100644 test/cli/install/bun-install-tempdir-cleanup.test.ts diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index 04dfd2bc0100..48819662409c 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -371,6 +371,7 @@ pub fn do_patch_commit( random_tempdir.as_bytes(), sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) .is_err() @@ -426,6 +427,7 @@ pub fn do_patch_commit( patch_tag_tmpname.as_bytes(), sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) { bun_core::warn!( @@ -458,7 +460,7 @@ pub fn do_patch_commit( random_tempdir.as_bytes(), new_folder_handle.fd, b"node_modules", - sys::RenameOptions { move_fallback: true }, + sys::RenameOptions { move_fallback: true, ..Default::default() }, ) { bun_core::warn!("failed renaming nested node_modules folder, this may cause issues: {}", e); } @@ -470,7 +472,7 @@ pub fn do_patch_commit( patch_tag_tmpname.as_bytes(), new_folder_handle.fd, patch_tag, - sys::RenameOptions { move_fallback: true }, + sys::RenameOptions { move_fallback: true, ..Default::default() }, ) { bun_core::warn!("failed renaming the bun patch tag, this may cause issues: {}", e); } @@ -621,6 +623,7 @@ pub fn do_patch_commit( path_in_patches_dir, sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) { Output::err(e, "failed renaming patch file to patches dir", ()); diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index dca3b11ca75d..6c29e1d35ed7 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -258,6 +258,11 @@ impl ExtractTarball { let mut resolved: &'static [u8] = b""; let tmpname = FileSystem::tmpname(tmpname_suffix, &mut tmpname_buf.0, bun_core::fast_random())?; + // Delete the temp dir if extraction fails before it's renamed into the + // cache; defused on success. + let tmpdir_cleanup = scopeguard::guard((), |()| { + let _ = Dir::borrow(&self.temp_dir).delete_tree(tmpname.as_bytes()); + }); { let extract_destination = match bun_sys::make_path::make_open_path( tmpdir, @@ -468,7 +473,11 @@ impl ExtractTarball { } } - self.move_to_cache_directory(log, tmpname, name, basename, resolved) + let result = self.move_to_cache_directory(log, tmpname, name, basename, resolved); + if result.is_ok() { + scopeguard::ScopeGuard::into_inner(tmpdir_cleanup); + } + result } /// Rename the freshly-extracted temp directory into the cache, read @@ -683,9 +692,9 @@ impl ExtractTarball { // // By: // 1. Rename from temporary directory to cache directory and fail if it already exists - // 2a. If the rename fails, swap the cache directory with the temporary directory version - // 2b. Delete the temporary directory version ONLY if we're not using a provided temporary directory - // 3. If rename still fails, fallback to racily deleting the cache directory version and then renaming the temporary directory version again. + // 2. If the rename fails because the destination exists, a concurrent install + // published an equivalent copy first: keep it and delete the temporary + // directory version (`keep_existing_destination`). // if create_subdir { @@ -701,6 +710,7 @@ impl ExtractTarball { folder_name, sys::RenameatConcurrentlyOptions { move_fallback: true, + keep_existing_destination: true, }, ) { log.add_error_fmt( diff --git a/src/install/patch_install.rs b/src/install/patch_install.rs index 8f3b3012f57e..a7f4a102c97e 100644 --- a/src/install/patch_install.rs +++ b/src/install/patch_install.rs @@ -452,6 +452,12 @@ impl PatchTask { let system_tmpdir = self.tempdir; + // Delete the temp dir on failure; after a successful rename into the + // cache this is a no-op. + scopeguard::defer! { + let _ = sys::Dir::borrow(&system_tmpdir).delete_tree(tempdir_name.as_bytes()); + } + let pkg_name = patch.pkgname; let dummy_node_modules = crate::package_installer::NodeModulesFolder { @@ -606,7 +612,7 @@ impl PatchTask { cache_dir_subpath_z, sys::RenameOptions { move_fallback: true, - ..Default::default() + keep_existing_destination: true, }, ) { log.add_error_fmt_opts( diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 68546dbdcd82..0cf0f3ff6e30 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9225,7 +9225,13 @@ pub fn exists(path: &[u8]) -> bool { /// retries; on EXDEV falls back to the slow open+copy path. Only opens the /// source inside the EXDEV branch. pub fn move_file_z(from_dir: Fd, filename: &ZStr, to_dir: Fd, destination: &ZStr) -> Maybe<()> { - match renameat_concurrently_without_fallback(from_dir, filename, to_dir, destination) { + match renameat_concurrently_without_fallback( + from_dir, + filename, + to_dir, + destination, + Default::default(), + ) { Ok(()) => Ok(()), // allow over-writing an empty directory Err(e) if e.get_errno() == E::EISDIR => { @@ -9302,6 +9308,12 @@ pub fn renameat_z(from_dir: impl AsFd, from: &ZStr, to_dir: impl AsFd, to: &ZStr #[derive(Default, Clone, Copy)] pub struct RenameatConcurrentlyOptions { pub move_fallback: bool, + /// If the destination already exists (a concurrent process published an + /// equivalent tree first, e.g. a shared package cache entry), keep it and + /// delete the source instead of atomically replacing it. Replacing would + /// yank entries out from under processes still reading the existing tree, + /// and swapping it into the source leaks the temp directory (#33977). + pub keep_existing_destination: bool, } /// Alias: `bun_install` call sites spell this `RenameOptions`. pub type RenameOptions = RenameatConcurrentlyOptions; @@ -9328,7 +9340,7 @@ pub fn renameat_concurrently( to: &ZStr, opts: RenameatConcurrentlyOptions, ) -> Maybe<()> { - match renameat_concurrently_without_fallback(from_dir_fd, from, to_dir_fd, to) { + match renameat_concurrently_without_fallback(from_dir_fd, from, to_dir_fd, to, opts) { Ok(()) => Ok(()), Err(e) => { if opts.move_fallback && e.get_errno() == E::EXDEV { @@ -9348,6 +9360,7 @@ pub fn renameat_concurrently_without_fallback( from: &ZStr, to_dir_fd: Fd, to: &ZStr, + opts: RenameatConcurrentlyOptions, ) -> Maybe<()> { 'attempt: { { @@ -9373,6 +9386,18 @@ pub fn renameat_concurrently_without_fallback( Ok(()) => break 'attempt, }; + if opts.keep_existing_destination && matches!(err.get_errno(), E::EEXIST | E::ENOTEMPTY) + { + // Another process won the race; its tree is equivalent to + // ours and may already have readers, so drop our copy. + if from_dir_fd.is_valid() { + let _ = Dir::borrow(&from_dir_fd).delete_tree(from.as_bytes()); + } else { + let _ = delete_tree_absolute(from.as_bytes()); + } + break 'attempt; + } + // Windows doesn't have any equivalent of renameat with swap #[cfg(not(windows))] { @@ -9742,6 +9767,7 @@ mod owned_handle_tests { b"sub", RenameatConcurrentlyOptions { move_fallback: true, + ..Default::default() }, ) .expect("rename"); @@ -9757,6 +9783,50 @@ mod owned_handle_tests { let _ = close(root); let _ = Dir::open(&tmp).map(|d| d.delete_tree(b".")); } + + /// With `keep_existing_destination`, losing the publish race keeps the + /// destination untouched and deletes the source instead of swapping the + /// two (which stranded the swapped-out tree in the temp dir, #33977). + #[test] + fn renameat_concurrently_keep_existing_destination() { + let _g = crate::file::tests::FD_TEST_LOCK.lock(); + let mut tmp = std::env::temp_dir().as_os_str().as_encoded_bytes().to_vec(); + tmp.extend_from_slice(b"/bun_sys_renameat_keep_test"); + let _ = open_dir_at(Fd::cwd(), &tmp).map(close); + let _ = mkdir_recursive_at(Fd::cwd(), &tmp); + let root = open_dir_at(Fd::cwd(), &tmp).expect("open root"); + let _ = mkdir_recursive_at(root, b"from/sub"); + let _ = mkdir_recursive_at(root, b"to/sub"); + let to_dir = open_dir_at(root, b"to").expect("open to"); + File::write_file(root, ZStr::from_static(b"to/sub/winner\0"), b"").expect("marker"); + + renameat_concurrently_a( + root, + b"from/sub", + to_dir, + b"sub", + RenameatConcurrentlyOptions { + move_fallback: true, + keep_existing_destination: true, + }, + ) + .expect("rename"); + + // The existing destination survives with its contents... + assert!( + exists_at(to_dir, ZStr::from_static(b"sub/winner\0")), + "existing destination was replaced" + ); + // ...and the source was cleaned up rather than left (or swapped) behind. + assert!( + !exists_at(root, ZStr::from_static(b"from/sub\0")), + "source left behind" + ); + + let _ = close(to_dir); + let _ = close(root); + let _ = Dir::open(&tmp).map(|d| d.delete_tree(b".")); + } } #[cfg(all(test, windows))] diff --git a/test/cli/install/bun-install-tempdir-cleanup.test.ts b/test/cli/install/bun-install-tempdir-cleanup.test.ts new file mode 100644 index 000000000000..97e7ae93079d --- /dev/null +++ b/test/cli/install/bun-install-tempdir-cleanup.test.ts @@ -0,0 +1,219 @@ +// https://github.com/oven-sh/bun/issues/33977 +// `bun install` must not leave package-extraction temp directories behind in +// the install temp dir ($BUN_TMPDIR / $TMPDIR): neither when two concurrent +// installs race the same cache entry (the RENAME_EXCHANGE fallback used to +// strand the loser's copy), nor when extraction or patching fails partway. + +import { expect, setDefaultTimeout, test } from "bun:test"; +import { rm } from "node:fs/promises"; +import { bunEnv, bunExe, readdirSorted, tempDir } from "harness"; +import { createHash } from "node:crypto"; +import { gzipSync } from "node:zlib"; +import { join } from "node:path"; + +setDefaultTimeout(1000 * 60 * 5); + +// --------------------------------------------------------------------------- +// Minimal in-process tarball + registry helpers (no binary fixtures). +// --------------------------------------------------------------------------- + +function octal(n: number, width: number): string { + return n.toString(8).padStart(width - 1, "0") + "\0"; +} + +function tarHeader(name: string, size: number, type: "0" | "5"): Buffer { + const buf = Buffer.alloc(512, 0); + buf.write(name, 0, 100, "utf8"); + buf.write(octal(0o644, 8), 100); // mode + buf.write(octal(0, 8), 108); // uid + buf.write(octal(0, 8), 116); // gid + buf.write(octal(size, 12), 124); // size + buf.write(octal(0, 12), 136); // mtime + buf.fill(" ", 148, 156); // checksum placeholder + buf.write(type, 156); + buf.write("ustar\0", 257); + buf.write("00", 263); + let sum = 0; + for (let i = 0; i < 512; i++) sum += buf[i]; + buf.write(octal(sum, 8), 148); + return buf; +} + +function pad512(len: number): Buffer { + return Buffer.alloc((512 - (len % 512)) % 512, 0); +} + +function buildTarball(entries: { path: string; body: Buffer }[]): { tgz: Buffer; integrity: string } { + const blocks: Buffer[] = []; + for (const { path, body } of entries) { + blocks.push(tarHeader(`package/${path}`, body.length, "0"), body, pad512(body.length)); + } + blocks.push(Buffer.alloc(1024, 0)); // end-of-archive + const tgz = gzipSync(Buffer.concat(blocks)); + return { tgz, integrity: "sha512-" + createHash("sha512").update(tgz).digest("base64") }; +} + +// A package with enough files that extraction takes long enough for two +// concurrent installs to reliably race the same cache entry. +function makePackageTarball(name: string, fileCount: number) { + const entries = [{ path: "package.json", body: Buffer.from(JSON.stringify({ name, version: "1.0.0" }) + "\n") }]; + for (let i = 0; i < fileCount; i++) { + // Incompressible content so gzip can't collapse the files away. + const body = Buffer.alloc(1024); + let seed = createHash("sha256").update(`${name}-${i}`).digest(); + for (let off = 0; off < body.length; off += 32) { + seed.copy(body, off); + seed = createHash("sha256").update(seed).digest(); + } + entries.push({ path: `files/f${i}.bin`, body }); + } + return buildTarball(entries); +} + +// Serves packuments at / and tarballs at //-/-1.0.0.tgz. +function makeRegistry(packages: Record) { + const server = Bun.serve({ + port: 0, + fetch(req) { + const { pathname } = new URL(req.url); + for (const [name, { tgz, integrity }] of Object.entries(packages)) { + if (pathname === `/${name}`) { + return Response.json({ + name, + "dist-tags": { latest: "1.0.0" }, + versions: { + "1.0.0": { + name, + version: "1.0.0", + dist: { + integrity, + tarball: `${server.url}${name}/-/${name}-1.0.0.tgz`, + }, + }, + }, + }); + } + if (pathname === `/${name}/-/${name}-1.0.0.tgz`) { + return new Response(tgz); + } + } + return new Response("not found", { status: 404 }); + }, + }); + return server; +} + +async function runInstall(cwd: string, cacheDir: string, tmpDir: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "install", "--no-save", "--linker=hoisted"], + cwd, + env: { + ...bunEnv, + BUN_INSTALL_CACHE_DIR: cacheDir, + BUN_TMPDIR: tmpDir, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +test("concurrent installs sharing a cache do not leak temp directories", async () => { + const packageCount = 8; + const packages: Record = {}; + for (let i = 0; i < packageCount; i++) { + packages[`leaky-pkg-${i}`] = makePackageTarball(`leaky-pkg-${i}`, 150); + } + using server = makeRegistry(packages); + + const dependencies = Object.fromEntries(Object.keys(packages).map(name => [name, "1.0.0"])); + const files: Record = { "tmp/.keep": "", "cache/.keep": "" }; + for (const proj of ["proj1", "proj2"]) { + files[`${proj}/package.json`] = JSON.stringify({ name: proj, version: "1.0.0", dependencies }); + files[`${proj}/bunfig.toml`] = `[install]\nregistry = "${server.url}"\n`; + } + using dir = tempDir("tempdir-leak", files); + const tmpDir = join(String(dir), "tmp"); + const cacheDir = join(String(dir), "cache"); + + for (let iteration = 0; iteration < 5; iteration++) { + // Evict the cache so both installs extract (and race) every package again. + await Promise.all([ + rm(cacheDir, { recursive: true, force: true }), + rm(join(String(dir), "proj1", "node_modules"), { recursive: true, force: true }), + rm(join(String(dir), "proj2", "node_modules"), { recursive: true, force: true }), + ]); + + const [r1, r2] = await Promise.all([ + runInstall(join(String(dir), "proj1"), cacheDir, tmpDir), + runInstall(join(String(dir), "proj2"), cacheDir, tmpDir), + ]); + expect({ stderr: r1.stderr, exitCode: r1.exitCode }).toMatchObject({ exitCode: 0 }); + expect({ stderr: r2.stderr, exitCode: r2.exitCode }).toMatchObject({ exitCode: 0 }); + } + + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); +}); + +test("a tarball that fails to extract does not leak its temp directory", async () => { + // Valid integrity (computed over the bytes) but not a gzip stream, so the + // failure happens during extraction, after the temp dir was created. + const tgz = Buffer.from("this is definitely not a gzipped tarball"); + using server = makeRegistry({ + "corrupt-pkg": { tgz, integrity: "sha512-" + createHash("sha512").update(tgz).digest("base64") }, + }); + + using dir = tempDir("tempdir-leak-corrupt", { + "proj/package.json": JSON.stringify({ + name: "proj", + version: "1.0.0", + dependencies: { "corrupt-pkg": "1.0.0" }, + }), + "proj/bunfig.toml": `[install]\nregistry = "${server.url}"\n`, + "tmp/.keep": "", + "cache/.keep": "", + }); + const tmpDir = join(String(dir), "tmp"); + + const { stderr, exitCode } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); + expect(stderr).toContain("corrupt-pkg"); + expect(exitCode).not.toBe(0); + + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); +}); + +test("a patch that fails to apply does not leak its temp directory", async () => { + using server = makeRegistry({ "patched-pkg": makePackageTarball("patched-pkg", 3) }); + + // Parses fine, but targets a file the package doesn't contain. + const patch = [ + "diff --git a/missing.txt b/missing.txt", + "index 0000000..1111111 100644", + "--- a/missing.txt", + "+++ b/missing.txt", + "@@ -1 +1 @@", + "-old", + "+new", + "", + ].join("\n"); + + using dir = tempDir("tempdir-leak-patch", { + "proj/package.json": JSON.stringify({ + name: "proj", + version: "1.0.0", + dependencies: { "patched-pkg": "1.0.0" }, + patchedDependencies: { "patched-pkg@1.0.0": "patches/patched-pkg.patch" }, + }), + "proj/patches/patched-pkg.patch": patch, + "proj/bunfig.toml": `[install]\nregistry = "${server.url}"\n`, + "tmp/.keep": "", + "cache/.keep": "", + }); + const tmpDir = join(String(dir), "tmp"); + + const { stderr } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); + expect(stderr).toContain("failed applying patch file"); + + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); +}); From 79dd00aa3c4d1735007fb05e275515f6c0c69c43 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 11 Jul 2026 12:27:14 +0000 Subject: [PATCH 2/7] [autofix.ci] apply automated fixes --- test/cli/install/bun-install-tempdir-cleanup.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cli/install/bun-install-tempdir-cleanup.test.ts b/test/cli/install/bun-install-tempdir-cleanup.test.ts index 97e7ae93079d..c87a0a5d2ac4 100644 --- a/test/cli/install/bun-install-tempdir-cleanup.test.ts +++ b/test/cli/install/bun-install-tempdir-cleanup.test.ts @@ -5,11 +5,11 @@ // strand the loser's copy), nor when extraction or patching fails partway. import { expect, setDefaultTimeout, test } from "bun:test"; -import { rm } from "node:fs/promises"; import { bunEnv, bunExe, readdirSorted, tempDir } from "harness"; import { createHash } from "node:crypto"; -import { gzipSync } from "node:zlib"; +import { rm } from "node:fs/promises"; import { join } from "node:path"; +import { gzipSync } from "node:zlib"; setDefaultTimeout(1000 * 60 * 5); From 120895b078fabf04c7f2880bfc5be60c9f7244af Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 11 Jul 2026 12:46:08 +0000 Subject: [PATCH 3/7] install: keep existing cache entry on Windows publish collision The Windows arm of move_to_cache_directory handled a destination that already exists by renaming it out of the cache and deleting it, then retrying its own move. When the existing entry was published by a concurrent install that is still copying from it, this yanks the tree out from under that process and fails it with ENOENT (#28062). Apply the same first-writer-wins rule as the POSIX path: if the destination directory exists, delete our own temp copy and use the existing entry. This deletes the rename-out-and-delete fallback block. Also address review feedback on the tests: run them concurrently, assert the patch failure exit code, and move exit-code assertions after filesystem checks. --- src/install/extract_tarball.rs | 41 ++++++------------- .../bun-install-tempdir-cleanup.test.ts | 11 ++--- 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index 6c29e1d35ed7..65d208b8fcc9 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -618,39 +618,22 @@ impl ExtractTarball { | 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( + // A concurrent install sharing the cache published + // this entry first. Keep it (it may already have + // readers) and drop our equivalent copy instead of + // renaming it out from under them. + if sys::directory_exists_at_w( 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()); - } + path_to_use, + ) + .unwrap_or(false) + { + let _ = tmpdir.delete_tree(tmpname.as_bytes()); + break; } + retries += 1; // 10ms, 20ms, 40ms, 80ms — long enough // for a concurrent close to land, diff --git a/test/cli/install/bun-install-tempdir-cleanup.test.ts b/test/cli/install/bun-install-tempdir-cleanup.test.ts index c87a0a5d2ac4..3df53a159a0f 100644 --- a/test/cli/install/bun-install-tempdir-cleanup.test.ts +++ b/test/cli/install/bun-install-tempdir-cleanup.test.ts @@ -119,7 +119,7 @@ async function runInstall(cwd: string, cacheDir: string, tmpDir: string) { return { stdout, stderr, exitCode }; } -test("concurrent installs sharing a cache do not leak temp directories", async () => { +test.concurrent("concurrent installs sharing a cache do not leak temp directories", async () => { const packageCount = 8; const packages: Record = {}; for (let i = 0; i < packageCount; i++) { @@ -156,7 +156,7 @@ test("concurrent installs sharing a cache do not leak temp directories", async ( expect(await readdirSorted(tmpDir)).toEqual([".keep"]); }); -test("a tarball that fails to extract does not leak its temp directory", async () => { +test.concurrent("a tarball that fails to extract does not leak its temp directory", async () => { // Valid integrity (computed over the bytes) but not a gzip stream, so the // failure happens during extraction, after the temp dir was created. const tgz = Buffer.from("this is definitely not a gzipped tarball"); @@ -178,12 +178,12 @@ test("a tarball that fails to extract does not leak its temp directory", async ( const { stderr, exitCode } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); expect(stderr).toContain("corrupt-pkg"); - expect(exitCode).not.toBe(0); expect(await readdirSorted(tmpDir)).toEqual([".keep"]); + expect(exitCode).not.toBe(0); }); -test("a patch that fails to apply does not leak its temp directory", async () => { +test.concurrent("a patch that fails to apply does not leak its temp directory", async () => { using server = makeRegistry({ "patched-pkg": makePackageTarball("patched-pkg", 3) }); // Parses fine, but targets a file the package doesn't contain. @@ -212,8 +212,9 @@ test("a patch that fails to apply does not leak its temp directory", async () => }); const tmpDir = join(String(dir), "tmp"); - const { stderr } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); + const { stderr, exitCode } = await runInstall(join(String(dir), "proj"), join(String(dir), "cache"), tmpDir); expect(stderr).toContain("failed applying patch file"); expect(await readdirSorted(tmpDir)).toEqual([".keep"]); + expect(exitCode).not.toBe(0); }); From f55cf7e01c196115721f1664c0f7cb67cc49ccce Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 11 Jul 2026 12:50:24 +0000 Subject: [PATCH 4/7] sys: honor keep_existing_destination on the racy fallback path When the filesystem lacks RENAME_NOREPLACE (EOPNOTSUPP), the errno does not say whether the destination exists, and the fallback deleted it unconditionally before renaming. With keep_existing_destination set, check for the destination first and keep it, deleting the source instead, so the option's contract holds on those filesystems too. --- src/sys/lib.rs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 0cf0f3ff6e30..09fa19581ce8 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9362,6 +9362,13 @@ pub fn renameat_concurrently_without_fallback( to: &ZStr, opts: RenameatConcurrentlyOptions, ) -> Maybe<()> { + let delete_source = || { + if from_dir_fd.is_valid() { + let _ = Dir::borrow(&from_dir_fd).delete_tree(from.as_bytes()); + } else { + let _ = delete_tree_absolute(from.as_bytes()); + } + }; 'attempt: { { // Happy path: the folder doesn't exist in the cache dir, so we can @@ -9390,11 +9397,7 @@ pub fn renameat_concurrently_without_fallback( { // Another process won the race; its tree is equivalent to // ours and may already have readers, so drop our copy. - if from_dir_fd.is_valid() { - let _ = Dir::borrow(&from_dir_fd).delete_tree(from.as_bytes()); - } else { - let _ = delete_tree_absolute(from.as_bytes()); - } + delete_source(); break 'attempt; } @@ -9426,6 +9429,20 @@ pub fn renameat_concurrently_without_fallback( } // sad path: let's try to delete the folder and then rename it + if opts.keep_existing_destination { + // The errno didn't tell us whether the destination exists (e.g. + // EOPNOTSUPP when the filesystem lacks RENAME_NOREPLACE); check + // before deleting a tree another process may be reading. + let destination_exists = if to_dir_fd.is_valid() { + exists_at(to_dir_fd, to) + } else { + exists_z(to) + }; + if destination_exists { + delete_source(); + break 'attempt; + } + } if to_dir_fd.is_valid() { let _ = Dir::borrow(&to_dir_fd).delete_tree(to.as_bytes()); } else { From 9943a49a233c3bc1bd29cc53c6e4d76e9ab6f915 Mon Sep 17 00:00:00 2001 From: robobun Date: Sat, 11 Jul 2026 13:38:46 +0000 Subject: [PATCH 5/7] install: replace invalid npm cache entries instead of keeping them keep_existing_destination assumed an existing cache entry is always an equivalent copy from a concurrent install, but package_missing_from_cache treats an npm entry without package.json as absent and re-extracts to heal it. Keeping such an entry broke that heal (caught by bun-install-registry.test.ts on the Windows lanes). Delete an existing npm entry that is missing package.json before publishing the fresh copy; no process reads from an entry that fails the cache check. Also make the keep_existing_destination fallback existence check directory-aware: Windows exists_at returns true only for files, so the guard never fired for directory destinations there. --- src/install/extract_tarball.rs | 21 +++++++++ src/sys/lib.rs | 11 ++--- .../bun-install-tempdir-cleanup.test.ts | 43 ++++++++++++++++++- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index 65d208b8fcc9..68b94b60feef 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -553,6 +553,27 @@ impl ExtractTarball { } let cache_dir = Dir::borrow(&self.cache_dir); + // An existing npm cache entry without package.json is invalid (the + // same check `package_missing_from_cache` uses), so no concurrent + // install reads from it. Delete it so the fresh copy below replaces + // it instead of being kept as an equivalent existing destination. + if self.resolution.tag == ResolutionTag::Npm { + let mut folder_name_z_buf = PathBuffer::uninit(); + folder_name_z_buf[..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()); + if sys::directory_exists_at(cache_dir.fd(), folder_name_z).unwrap_or(false) { + let mut json_buf = PathBuffer::uninit(); + let json_z = path::resolve_path::join_z_buf::( + &mut json_buf.0, + &[folder_name, b"package.json"], + ); + if !sys::exists_at(cache_dir.fd(), json_z) { + let _ = cache_dir.delete_tree(folder_name); + } + } + } + // e.g. @next // if it's a namespace package, we need to make sure the @name folder exists let create_subdir = basename.len() != name.len() && !self.resolution.tag.is_git(); diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 09fa19581ce8..225e48b3920e 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9432,13 +9432,14 @@ pub fn renameat_concurrently_without_fallback( if opts.keep_existing_destination { // The errno didn't tell us whether the destination exists (e.g. // EOPNOTSUPP when the filesystem lacks RENAME_NOREPLACE); check - // before deleting a tree another process may be reading. - let destination_exists = if to_dir_fd.is_valid() { - exists_at(to_dir_fd, to) + // before deleting a tree another process may be reading. Windows + // `exists_at` is file-only, so ask for the directory explicitly. + let dir_fd = if to_dir_fd.is_valid() { + to_dir_fd } else { - exists_z(to) + Fd::cwd() }; - if destination_exists { + if directory_exists_at(dir_fd, to).unwrap_or(false) { delete_source(); break 'attempt; } diff --git a/test/cli/install/bun-install-tempdir-cleanup.test.ts b/test/cli/install/bun-install-tempdir-cleanup.test.ts index 3df53a159a0f..1fbf5e964b96 100644 --- a/test/cli/install/bun-install-tempdir-cleanup.test.ts +++ b/test/cli/install/bun-install-tempdir-cleanup.test.ts @@ -103,9 +103,9 @@ function makeRegistry(packages: Record { + using server = makeRegistry({ "heal-pkg": makePackageTarball("heal-pkg", 3) }); + + using dir = tempDir("tempdir-leak-heal", { + "proj/package.json": JSON.stringify({ + name: "proj", + version: "1.0.0", + dependencies: { "heal-pkg": "1.0.0" }, + }), + "proj/bunfig.toml": `[install]\nregistry = "${server.url}"\n`, + "tmp/.keep": "", + "cache/.keep": "", + }); + const tmpDir = join(String(dir), "tmp"); + const cacheDir = join(String(dir), "cache"); + const proj = join(String(dir), "proj"); + + // Save a lockfile: with one, the reinstall below skips re-resolution and + // relies on the package.json-exists cache check that triggers re-extract. + const first = await runInstall(proj, cacheDir, tmpDir, []); + expect(first.exitCode).toBe(0); + + const [cacheEntry] = (await readdirSorted(cacheDir)).filter(name => name.startsWith("heal-pkg@")); + expect(cacheEntry).toBeDefined(); + + // A cache entry without package.json is invalid; a reinstall whose + // node_modules copy also fails verification must replace it, not keep it. + await Promise.all([ + rm(join(cacheDir, cacheEntry, "package.json")), + rm(join(proj, "node_modules", "heal-pkg", "package.json")), + ]); + + const second = await runInstall(proj, cacheDir, tmpDir, []); + expect(await readdirSorted(join(cacheDir, cacheEntry))).toContain("package.json"); + expect(await readdirSorted(join(proj, "node_modules", "heal-pkg"))).toContain("package.json"); + expect(await readdirSorted(tmpDir)).toEqual([".keep"]); + expect(second.exitCode).toBe(0); +}); From 61c7ea43e5216e3d8205872a3f8085209712ac05 Mon Sep 17 00:00:00 2001 From: robobun Date: Sat, 11 Jul 2026 14:06:48 +0000 Subject: [PATCH 6/7] sys: make source-cleanup assertion directory-aware in rename test Windows exists_at is file-only, so the assertion was vacuous there. --- src/sys/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 225e48b3920e..1ad81854d7ed 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9835,9 +9835,10 @@ mod owned_handle_tests { exists_at(to_dir, ZStr::from_static(b"sub/winner\0")), "existing destination was replaced" ); - // ...and the source was cleaned up rather than left (or swapped) behind. + // ...and the source was cleaned up rather than left (or swapped) + // behind. Windows `exists_at` is file-only, so check the directory. assert!( - !exists_at(root, ZStr::from_static(b"from/sub\0")), + !directory_exists_at(root, ZStr::from_static(b"from/sub\0")).unwrap_or(false), "source left behind" ); From c0b8c8d59ba5b53fc4b474fd9104f8a2bc320132 Mon Sep 17 00:00:00 2001 From: robobun Date: Sat, 11 Jul 2026 16:06:49 +0000 Subject: [PATCH 7/7] ci: retrigger