From eb5f8855a2e15629e63a52ae53d9b6aa6424316c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:22:12 +0000 Subject: [PATCH 1/4] install: stop leaking the displaced cache folder when a tarball is re-extracted --- src/install/PackageManager/patchPackage.rs | 7 +- src/install/TarballStream.rs | 6 + src/install/extract_tarball.rs | 127 ++++++++---- src/install/repository.rs | 4 +- src/install/resolution.rs | 7 + src/sys/lib.rs | 87 ++++++++- .../bun-install-tarball-integrity.test.ts | 182 ++++++++++++------ 7 files changed, 320 insertions(+), 100 deletions(-) diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index 49029f7cd401..bd26a9b33852 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -344,6 +344,7 @@ pub fn do_patch_commit( random_tempdir.as_bytes(), sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) .is_err() @@ -399,6 +400,7 @@ pub fn do_patch_commit( patch_tag_tmpname.as_bytes(), sys::RenameOptions { move_fallback: true, + ..Default::default() }, ) { bun_core::warn!( @@ -431,7 +433,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); } @@ -443,7 +445,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); } @@ -588,6 +590,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/TarballStream.rs b/src/install/TarballStream.rs index 7ae9f698469b..bcf29466cf19 100644 --- a/src/install/TarballStream.rs +++ b/src/install/TarballStream.rs @@ -133,6 +133,8 @@ pub struct TarballStream { /// Owned copy of the temp-directory name. // `ZBox` is the owned NUL-terminated counterpart of `&ZStr`. tmpname: ZBox, + /// Captured in `init`, before the first byte is extracted. + cache_publish: CachePublish, /// Incremental SHA over the *compressed* bytes, matching /// `Integrity.verify` / `Integrity.forBytes` in the buffered path. @@ -230,6 +232,7 @@ impl TarballStream { }, compute_if_missing, ); + let cache_publish = tarball.cache_publish(); // bun.TrivialNew(@This()) → heap::alloc(Box::new(...)). Pointer is // recovered via `container_of` from the thread-pool callback and @@ -254,6 +257,7 @@ impl TarballStream { entry_final_offset: 0, dest: None, tmpname: ZBox::from_bytes(b""), + cache_publish, hasher, resolved_github_dirname: b"", want_first_dirname, @@ -1159,6 +1163,7 @@ impl TarballStream { name, basename, self.resolved_github_dirname, + self.cache_publish, ) { Ok(r) => r, Err(err) => { @@ -1452,5 +1457,6 @@ fn tokenize_rest_after_first(s: &[OSPathChar]) -> &[OSPathChar] { // Resolved Phase-B paths: Resolution::Tag is the real npm/git/tarball // discriminant; Data/Status live on PackageManagerTask. +use crate::extract_tarball::CachePublish; use crate::package_manager_task::{Data as TaskData, Status as TaskStatus}; use crate::resolution::Tag as ResolutionTag; diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index b8d3a8b091b5..53a8c19d8208 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -181,7 +181,49 @@ pub(crate) fn uses_streaming_extraction() -> bool { .unwrap_or(false) } +/// What `move_to_cache_directory` does when the cache already has a folder +/// under the name being published. Decided by `cache_publish()` before +/// extraction starts; by the time the extracted tree is renamed into place, +/// a folder that was there all along and one a concurrent install published +/// in the meantime look the same. +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum CachePublish { + /// Swap the fresh copy in and leave the one swapped out in the temp + /// directory: it may have been published by a concurrent install that is + /// still installing from it. npm and GitHub folders are published this way. + Replace, + /// The folder was already there when this tarball's task started, so this + /// is a re-extraction of the same tarball path or URL (no lockfile, or the + /// tarball behind it changed): swap the fresh copy in and delete the one + /// swapped out. + Supersede, + /// The folder was missing when this tarball's task started. If it exists + /// now, a concurrent install extracted the same tarball first: keep its + /// copy and delete ours, which nothing else can have opened. + KeepExisting, +} + impl ExtractTarball { + /// See [`CachePublish`]. Must run before anything is extracted. + pub(crate) fn cache_publish(&self) -> CachePublish { + if !self.resolution.tag.is_tarball() { + return CachePublish::Replace; + } + let name_taken = TL_BUFS.with_borrow_mut(|bufs| { + let folder_name = directories::cached_tarball_folder_name_print( + &mut bufs.folder_name_buf, + self.url.slice(), + None, + ); + sys::exists_at_type(self.cache_dir, folder_name).is_ok() + }); + if name_taken { + CachePublish::Supersede + } else { + CachePublish::KeepExisting + } + } + /// Derive the display name and a filesystem-safe basename for this /// package. Shared by the buffered `extract()` path below and the /// streaming extractor in `TarballStream.rs` so both pick identical @@ -256,6 +298,7 @@ impl ExtractTarball { let mut resolved: &'static [u8] = b""; let tmpname = FileSystem::tmpname(tmpname_suffix, &mut tmpname_buf.0, bun_core::fast_random())?; + let publish = self.cache_publish(); { let extract_destination = match bun_sys::make_path::make_open_path( tmpdir, @@ -439,7 +482,7 @@ impl ExtractTarball { } } - self.move_to_cache_directory(log, tmpname, name, basename, resolved) + self.move_to_cache_directory(log, tmpname, name, basename, resolved, publish) } /// Rename the freshly-extracted temp directory into the cache, read @@ -452,6 +495,7 @@ impl ExtractTarball { name: &[u8], basename: &[u8], resolved: &[u8], + publish: CachePublish, ) -> Result { let package_manager = self.package_manager.get(); @@ -521,7 +565,7 @@ impl ExtractTarball { // Now that we've extracted the archive, we rename. #[cfg(windows)] - { + let moved: Result<(), Error> = 'moved: { // 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 @@ -537,6 +581,10 @@ impl ExtractTarball { } let path_to_use = path2; + 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()); loop { let dir_to_move = match sys::open_dir_at_windows_a( @@ -562,7 +610,7 @@ impl ExtractTarball { bun_fmt::s(folder_name), ), ); - return Err(crate::Error::InstallFailed); + break 'moved Err(crate::Error::InstallFailed); } }; @@ -582,6 +630,16 @@ impl ExtractTarball { // before we attempt to delete the destination, let's close the source dir. let _ = sys::close(dir_to_move); + if publish == CachePublish::KeepExisting + && sys::directory_exists_at( + cache_dir.fd(), + folder_name_z, + ) + .unwrap_or(false) + { + break; + } + // We tried to move the folder over // but it didn't work! // so instead of just simply deleting the folder @@ -595,12 +653,6 @@ impl ExtractTarball { .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, @@ -637,7 +689,7 @@ impl ExtractTarball { bun_fmt::s(folder_name), ), ); - return Err(crate::Error::InstallFailed); + break 'moved Err(crate::Error::InstallFailed); } bun_sys::Result::Ok(_) => { let _ = sys::close(dir_to_move); @@ -646,47 +698,54 @@ impl ExtractTarball { break; } - } + Ok(()) + }; #[cfg(not(windows))] - { - // Attempt to gracefully handle duplicate concurrent `bun install` calls - // - // 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. - // - + let moved: Result<(), Error> = { if create_subdir { if let Some(folder) = bun_paths::Dirname::dirname(folder_name) { let _ = bun_sys::make_path::make_path(cache_dir, folder); } } - if let Err(err) = sys::renameat_concurrently_a( + // An existing folder is normally swapped out atomically (see + // `renameat_concurrently`); `KeepExisting` leaves it alone. + match sys::renameat_concurrently_a( tmpdir.fd(), tmpname.as_bytes(), cache_dir.fd(), folder_name, sys::RenameatConcurrentlyOptions { move_fallback: true, + keep_existing_destination: publish == CachePublish::KeepExisting, }, ) { - log.add_error_fmt( - None, - bun_ast::Loc::EMPTY, - format_args!( - "moving \"{}\" to cache dir failed: {}\n From: {}\n To: {}", - bun_fmt::s(name), - err, - bun_fmt::s(tmpname.as_bytes()), - bun_fmt::s(folder_name), - ), - ); - return Err(crate::Error::InstallFailed); + Ok(()) => Ok(()), + Err(err) => { + log.add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!( + "moving \"{}\" to cache dir failed: {}\n From: {}\n To: {}", + bun_fmt::s(name), + err, + bun_fmt::s(tmpname.as_bytes()), + bun_fmt::s(folder_name), + ), + ); + Err(crate::Error::InstallFailed) + } } + }; + + // Whatever is left under the temp name is either our own copy (the + // move failed, or a concurrent install published first) or the + // folder we superseded. `Replace` leaves a swapped-out folder + // alone, see `CachePublish`. + if moved.is_err() || publish != CachePublish::Replace { + let _ = tmpdir.delete_tree(tmpname.as_bytes()); } + moved?; // We return a resolved absolute absolute file path to the cache dir. // To get that directory, we open the directory again. diff --git a/src/install/repository.rs b/src/install/repository.rs index 7d86db59cd51..b3c51d36a1fa 100644 --- a/src/install/repository.rs +++ b/src/install/repository.rs @@ -468,9 +468,7 @@ impl CacheStaging { self.tmp_name(), self.cache_dir, folder_name, - bun_sys::RenameatConcurrentlyOptions { - move_fallback: false, - }, + bun_sys::RenameatConcurrentlyOptions::default(), ); // After an exchange the temporary name holds the folder that was replaced. self.discard(); diff --git a/src/install/resolution.rs b/src/install/resolution.rs index 5998e39bc87b..54aff25f2a52 100644 --- a/src/install/resolution.rs +++ b/src/install/resolution.rs @@ -965,6 +965,13 @@ impl Tag { self == Tag::Git || self == Tag::Github } + /// A tarball addressed by path or URL. Its cache folder is named after + /// that address, not the contents, so the same folder can hold different + /// versions of the tarball over time. + pub(crate) fn is_tarball(self) -> bool { + self == Tag::LocalTarball || self == Tag::RemoteTarball + } + pub(crate) fn can_enqueue_install_task(self) -> bool { self == Tag::Npm || self == Tag::LocalTarball diff --git a/src/sys/lib.rs b/src/sys/lib.rs index b4b8f766985e..2b3689ac5420 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9197,7 +9197,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, + RenameatConcurrentlyOptions::default(), + ) { Ok(()) => Ok(()), // allow over-writing an empty directory Err(e) if e.get_errno() == E::EISDIR => { @@ -9278,6 +9284,11 @@ 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, + /// When `to` already exists, leave it in place and delete `from` instead + /// of replacing it. For a shared cache this is the "another process + /// published the same thing first" outcome: the existing tree may already + /// have readers, while nothing else can have found `from` yet. + pub keep_existing_destination: bool, } /// Alias: `bun_install` call sites spell this `RenameOptions`. pub type RenameOptions = RenameatConcurrentlyOptions; @@ -9296,7 +9307,12 @@ pub(crate) fn move_file_z_slow_maybe( /// `renameatConcurrently`. Tries an atomic NOREPLACE rename, /// then EXCHANGE, then a racy delete-tree + rename. With `move_fallback` set, -/// an EXDEV result falls through to a slow open/copy. +/// an EXDEV result falls through to a slow open/copy. With +/// `keep_existing_destination` set, an existing `to` wins and `from` is +/// deleted instead. +/// +/// After a successful EXCHANGE, `from` names the tree that used to be at +/// `to`; callers that do not want to keep it have to delete it themselves. pub fn renameat_concurrently( from_dir_fd: Fd, from: &ZStr, @@ -9304,7 +9320,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 { @@ -9324,6 +9340,7 @@ pub(crate) fn renameat_concurrently_without_fallback( from: &ZStr, to_dir_fd: Fd, to: &ZStr, + opts: RenameatConcurrentlyOptions, ) -> Maybe<()> { 'attempt: { { @@ -9349,6 +9366,27 @@ pub(crate) fn renameat_concurrently_without_fallback( Ok(()) => break 'attempt, }; + // The errno alone does not say whether `to` exists (Windows and + // filesystems without RENAME_NOREPLACE fail differently), so look. + if opts.keep_existing_destination + && exists_at_type( + if to_dir_fd.is_valid() { + to_dir_fd + } else { + Fd::cwd() + }, + to, + ) + .is_ok() + { + 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))] { @@ -9711,6 +9749,7 @@ mod owned_handle_tests { b"sub", RenameatConcurrentlyOptions { move_fallback: true, + ..Default::default() }, ) .expect("rename"); @@ -9726,6 +9765,48 @@ mod owned_handle_tests { let _ = close(root); let _ = Dir::open(&tmp).map(|d| d.delete_tree(b".")); } + + #[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_existing_test"); + let _ = Dir::open(&tmp).map(|d| d.delete_tree(b".")); + 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"); + File::write_file(root, ZStr::from_static(b"from/sub/loser\0"), b"").expect("loser"); + File::write_file(root, ZStr::from_static(b"to/sub/winner\0"), b"").expect("winner"); + let to_dir = open_dir_at(root, b"to").expect("open to"); + + let opts = RenameatConcurrentlyOptions { + keep_existing_destination: true, + ..Default::default() + }; + + // Destination taken: it is kept as-is and the source goes away. + renameat_concurrently_a(root, b"from/sub", to_dir, b"sub", opts).expect("rename"); + assert!(matches!( + exists_at_type(root, ZStr::from_static(b"to/sub/winner\0")), + Ok(ExistsAtType::File) + )); + assert!(exists_at_type(root, ZStr::from_static(b"to/sub/loser\0")).is_err()); + assert!(exists_at_type(root, ZStr::from_static(b"from/sub\0")).is_err()); + + // Destination free: a plain rename. + let _ = mkdir_recursive_at(root, b"from/other"); + renameat_concurrently_a(root, b"from/other", to_dir, b"other", opts).expect("rename"); + assert!(matches!( + exists_at_type(root, ZStr::from_static(b"to/other\0")), + Ok(ExistsAtType::Directory) + )); + assert!(exists_at_type(root, ZStr::from_static(b"from/other\0")).is_err()); + + 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-tarball-integrity.test.ts b/test/cli/install/bun-install-tarball-integrity.test.ts index 422352e6a667..806b15a873c4 100644 --- a/test/cli/install/bun-install-tarball-integrity.test.ts +++ b/test/cli/install/bun-install-tarball-integrity.test.ts @@ -2,7 +2,7 @@ import { file, spawn } from "bun"; import { afterAll, beforeAll, describe, expect, it, setDefaultTimeout } from "bun:test"; import { rm, writeFile } from "fs/promises"; import { bunExe, bunEnv as env, readdirSorted, tempDir } from "harness"; -import { createHash } from "node:crypto"; +import { createHash, randomBytes } from "node:crypto"; import { gzipSync } from "node:zlib"; import { join } from "path"; import { @@ -39,6 +39,33 @@ async function withContext( // Default context options for most tests const defaultOpts = { linker: "hoisted" as const }; +/** A .tgz with the given files under the usual `package/` root. */ +function tarball(files: Record): Buffer { + function octal(n: number, width: number) { + return n.toString(8).padStart(width - 1, "0") + "\0"; + } + const blocks: Buffer[] = []; + for (const [name, body] of Object.entries(files)) { + const header = Buffer.alloc(512, 0); + header.write(`package/${name}`, 0, 100, "utf8"); + header.write(octal(0o644, 8), 100); + header.write(octal(0, 8), 108); + header.write(octal(0, 8), 116); + header.write(octal(body.length, 12), 124); + header.write(octal(0, 12), 136); + header.fill(" ", 148, 156); + header.write("0", 156); + header.write("ustar\0", 257); + header.write("00", 263); + let sum = 0; + for (let i = 0; i < 512; i++) sum += header[i]; + header.write(octal(sum, 8), 148); + blocks.push(header, body, Buffer.alloc((512 - (body.length % 512)) % 512, 0)); + } + blocks.push(Buffer.alloc(1024, 0)); + return gzipSync(Buffer.concat(blocks)); +} + describe.concurrent("tarball integrity", () => { it("should store integrity hash for tarball URL in text lockfile", async () => { await withContext(defaultOpts, async ctx => { @@ -506,37 +533,8 @@ describe.concurrent.each(["hoisted", "isolated"] as const)("tarball integrity mi // callback is the void `onPackageDownloadError = {}` — i.e. the branch the // fix in runTasks.zig now cleans up. it("should fail (not hang) when tarball bytes don't match manifest SHA-512", { timeout: 60_000 }, async () => { - function octal(n: number, width: number) { - return n.toString(8).padStart(width - 1, "0") + "\0"; - } - function tarHeader(name: string, size: number) { - const buf = Buffer.alloc(512, 0); - buf.write(name, 0, 100, "utf8"); - buf.write(octal(0o644, 8), 100); - buf.write(octal(0, 8), 108); - buf.write(octal(0, 8), 116); - buf.write(octal(size, 12), 124); - buf.write(octal(0, 12), 136); - buf.fill(" ", 148, 156); - buf.write("0", 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) { - return Buffer.alloc((512 - (len % 512)) % 512, 0); - } function buildTarball(body: Buffer) { - const tar = Buffer.concat([ - tarHeader("package/package.json", body.length), - body, - pad512(body.length), - Buffer.alloc(1024, 0), - ]); - const tgz = gzipSync(tar); + const tgz = tarball({ "package.json": body }); return { tgz, integrity: "sha512-" + createHash("sha512").update(tgz).digest("base64") }; } @@ -617,34 +615,8 @@ describe.concurrent.each(["hoisted", "isolated"] as const)("tarball integrity mi }); describe.concurrent("tarball integrity metadata forms", () => { - function octal(n: number, width: number) { - return n.toString(8).padStart(width - 1, "0") + "\0"; - } - function tarHeader(name: string, size: number) { - const buf = Buffer.alloc(512, 0); - buf.write(name, 0, 100, "utf8"); - buf.write(octal(0o644, 8), 100); - buf.write(octal(0, 8), 108); - buf.write(octal(0, 8), 116); - buf.write(octal(size, 12), 124); - buf.write(octal(0, 12), 136); - buf.fill(" ", 148, 156); - buf.write("0", 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 buildTarball(body: Buffer) { - const tar = Buffer.concat([ - tarHeader("package/package.json", body.length), - body, - Buffer.alloc((512 - (body.length % 512)) % 512, 0), - Buffer.alloc(1024, 0), - ]); - const tgz = gzipSync(tar); + const tgz = tarball({ "package.json": body }); return { tgz, sha512: "sha512-" + createHash("sha512").update(tgz).digest("base64"), @@ -854,3 +826,97 @@ describe.concurrent.each(["hoisted", "isolated"] as const)("tarball download fai }); }); }); + +// A `file:` or URL tarball is cached under a folder named after its path or +// URL, so installing without a lockfile extracts it again over the folder from +// the previous install. The fresh copy must win (the tarball may have been +// repacked) and the copy it displaces must not be left behind in the temp dir, +// which used to grow by one extracted tarball per install. +describe.concurrent("tarball re-extraction over an existing cache folder", () => { + function tarballOfVersion(version: string, padding: Record = {}) { + return tarball({ "package.json": Buffer.from(JSON.stringify({ name: "pkg", version }) + "\n"), ...padding }); + } + + function project(name: string, spec: string) { + return tempDir(name, { + "app/package.json": JSON.stringify({ name: "app", dependencies: { pkg: spec } }), + // The install extracts into tmp/ and renames into cache/; both are inside + // the test directory, so anything left in tmp/ after an install is a leak. + cache: {}, + tmp: {}, + }); + } + + /** `bun install` from scratch (no node_modules, no lockfile); returns the installed version of `pkg`. */ + async function installFresh(dir: string) { + const app = join(dir, "app"); + await Promise.all([ + rm(join(app, "node_modules"), { recursive: true, force: true }), + rm(join(app, "bun.lock"), { force: true }), + ]); + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: app, + env: { ...env, BUN_INSTALL_CACHE_DIR: join(dir, "cache"), BUN_TMPDIR: join(dir, "tmp") }, + stdout: "pipe", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect({ stderr, exitCode }).toMatchObject({ exitCode: 0 }); + const { version } = await file(join(app, "node_modules", "pkg", "package.json")).json(); + return version as string; + } + + async function cacheState(dir: string) { + const [tmp, cache] = await Promise.all([readdirSorted(join(dir, "tmp")), readdirSorted(join(dir, "cache"))]); + return { tmp, tarballFolders: cache.filter(name => name.startsWith("@T@")).length }; + } + + it("file: tarball", async () => { + using dir = project("tarball-reextract-file", "file:../pkg.tgz"); + await writeFile(join(String(dir), "pkg.tgz"), tarballOfVersion("1.0.0")); + + expect(await installFresh(String(dir))).toBe("1.0.0"); + expect(await installFresh(String(dir))).toBe("1.0.0"); + expect(await installFresh(String(dir))).toBe("1.0.0"); + expect(await cacheState(String(dir))).toEqual({ tmp: [], tarballFolders: 1 }); + + await writeFile(join(String(dir), "pkg.tgz"), tarballOfVersion("2.0.0")); + expect(await installFresh(String(dir))).toBe("2.0.0"); + expect(await cacheState(String(dir))).toEqual({ tmp: [], tarballFolders: 1 }); + }); + + it("tarball URL", async () => { + // Incompressible padding, served without a Content-Length in several + // chunks, makes the download take the streaming extractor rather than + // being buffered and extracted in one go. + const padding = { "blob.bin": randomBytes(256 * 1024) }; + let tgz = tarballOfVersion("1.0.0", padding); + await using server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch() { + let offset = 0; + return new Response( + new ReadableStream({ + pull(controller) { + if (offset >= tgz.length) return controller.close(); + controller.enqueue(tgz.subarray(offset, offset + 64 * 1024)); + offset += 64 * 1024; + }, + }), + ); + }, + }); + using dir = project("tarball-reextract-url", `${server.url}pkg.tgz`); + + expect(await installFresh(String(dir))).toBe("1.0.0"); + expect(await installFresh(String(dir))).toBe("1.0.0"); + expect(await installFresh(String(dir))).toBe("1.0.0"); + expect(await cacheState(String(dir))).toEqual({ tmp: [], tarballFolders: 1 }); + + tgz = tarballOfVersion("2.0.0", padding); + expect(await installFresh(String(dir))).toBe("2.0.0"); + expect(await cacheState(String(dir))).toEqual({ tmp: [], tarballFolders: 1 }); + }); +}); From f13531807423259837105542df3863577c6c7862 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:41:28 +0000 Subject: [PATCH 2/4] test: drain stdout in the re-extraction install helper --- test/cli/install/bun-install-tarball-integrity.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cli/install/bun-install-tarball-integrity.test.ts b/test/cli/install/bun-install-tarball-integrity.test.ts index 806b15a873c4..a2e91ed40e5e 100644 --- a/test/cli/install/bun-install-tarball-integrity.test.ts +++ b/test/cli/install/bun-install-tarball-integrity.test.ts @@ -861,8 +861,8 @@ describe.concurrent("tarball re-extraction over an existing cache folder", () => stdout: "pipe", stderr: "pipe", }); - const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); - expect({ stderr, exitCode }).toMatchObject({ exitCode: 0 }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toMatchObject({ exitCode: 0 }); const { version } = await file(join(app, "node_modules", "pkg", "package.json")).json(); return version as string; } From ab7e2691d4932a8e998e24e40b629e0be52aad82 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:46:29 +0000 Subject: [PATCH 3/4] install: shorten the CachePublish and rename option docs --- src/install/extract_tarball.rs | 33 +++++++++++---------------------- src/install/resolution.rs | 5 ++--- src/sys/lib.rs | 19 +++++++------------ 3 files changed, 20 insertions(+), 37 deletions(-) diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index 53a8c19d8208..8bfb08b097a7 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -181,30 +181,23 @@ pub(crate) fn uses_streaming_extraction() -> bool { .unwrap_or(false) } -/// What `move_to_cache_directory` does when the cache already has a folder -/// under the name being published. Decided by `cache_publish()` before -/// extraction starts; by the time the extracted tree is renamed into place, -/// a folder that was there all along and one a concurrent install published -/// in the meantime look the same. +/// How `move_to_cache_directory` treats a cache folder that already has the +/// name being published. Decided before extracting: afterwards a pre-existing +/// folder and one a concurrent install published meanwhile look the same. #[derive(Clone, Copy, PartialEq, Eq)] pub(crate) enum CachePublish { - /// Swap the fresh copy in and leave the one swapped out in the temp - /// directory: it may have been published by a concurrent install that is - /// still installing from it. npm and GitHub folders are published this way. + /// npm and GitHub: swap the fresh copy in. The swapped-out folder is left + /// in the temp dir, as a concurrent install may still be reading it. Replace, - /// The folder was already there when this tarball's task started, so this - /// is a re-extraction of the same tarball path or URL (no lockfile, or the - /// tarball behind it changed): swap the fresh copy in and delete the one - /// swapped out. + /// Tarball re-extracted over its own folder: swap the fresh copy in and + /// delete the swapped-out folder. Supersede, - /// The folder was missing when this tarball's task started. If it exists - /// now, a concurrent install extracted the same tarball first: keep its - /// copy and delete ours, which nothing else can have opened. + /// Tarball whose folder did not exist yet: if one exists by now, a + /// concurrent install extracted the same tarball, so keep it and drop ours. KeepExisting, } impl ExtractTarball { - /// See [`CachePublish`]. Must run before anything is extracted. pub(crate) fn cache_publish(&self) -> CachePublish { if !self.resolution.tag.is_tarball() { return CachePublish::Replace; @@ -708,8 +701,6 @@ impl ExtractTarball { } } - // An existing folder is normally swapped out atomically (see - // `renameat_concurrently`); `KeepExisting` leaves it alone. match sys::renameat_concurrently_a( tmpdir.fd(), tmpname.as_bytes(), @@ -738,10 +729,8 @@ impl ExtractTarball { } }; - // Whatever is left under the temp name is either our own copy (the - // move failed, or a concurrent install published first) or the - // folder we superseded. `Replace` leaves a swapped-out folder - // alone, see `CachePublish`. + // The temp name now holds our own copy (failed or lost), the + // superseded folder, or a `Replace` swap-out (see `CachePublish`). if moved.is_err() || publish != CachePublish::Replace { let _ = tmpdir.delete_tree(tmpname.as_bytes()); } diff --git a/src/install/resolution.rs b/src/install/resolution.rs index 54aff25f2a52..a4b54dde87ae 100644 --- a/src/install/resolution.rs +++ b/src/install/resolution.rs @@ -965,9 +965,8 @@ impl Tag { self == Tag::Git || self == Tag::Github } - /// A tarball addressed by path or URL. Its cache folder is named after - /// that address, not the contents, so the same folder can hold different - /// versions of the tarball over time. + /// Cached under a hash of the path or URL, so the folder's contents can + /// change from one extraction to the next. pub(crate) fn is_tarball(self) -> bool { self == Tag::LocalTarball || self == Tag::RemoteTarball } diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 2b3689ac5420..e01dcb10aa48 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -9284,10 +9284,8 @@ 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, - /// When `to` already exists, leave it in place and delete `from` instead - /// of replacing it. For a shared cache this is the "another process - /// published the same thing first" outcome: the existing tree may already - /// have readers, while nothing else can have found `from` yet. + /// If `to` already exists (another process published it first and may be + /// reading it), keep it and delete `from` instead of replacing it. pub keep_existing_destination: bool, } /// Alias: `bun_install` call sites spell this `RenameOptions`. @@ -9307,12 +9305,9 @@ pub(crate) fn move_file_z_slow_maybe( /// `renameatConcurrently`. Tries an atomic NOREPLACE rename, /// then EXCHANGE, then a racy delete-tree + rename. With `move_fallback` set, -/// an EXDEV result falls through to a slow open/copy. With -/// `keep_existing_destination` set, an existing `to` wins and `from` is -/// deleted instead. -/// -/// After a successful EXCHANGE, `from` names the tree that used to be at -/// `to`; callers that do not want to keep it have to delete it themselves. +/// an EXDEV result falls through to a slow open/copy; see +/// [`RenameatConcurrentlyOptions`] for `keep_existing_destination`. +/// After an EXCHANGE, `from` holds the tree that was at `to`. pub fn renameat_concurrently( from_dir_fd: Fd, from: &ZStr, @@ -9366,8 +9361,8 @@ pub(crate) fn renameat_concurrently_without_fallback( Ok(()) => break 'attempt, }; - // The errno alone does not say whether `to` exists (Windows and - // filesystems without RENAME_NOREPLACE fail differently), so look. + // Not keyed on the errno: Windows and filesystems without + // RENAME_NOREPLACE report an existing `to` differently. if opts.keep_existing_destination && exists_at_type( if to_dir_fd.is_valid() { From 0613908c0fe492f4d4bec745beea9547172f349e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:22:39 +0000 Subject: [PATCH 4/4] ci: retrigger