From 88ab61dac9d1f15298862670c7730e9f28105823 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Thu, 13 Aug 2026 16:19:16 -0700 Subject: [PATCH] install: build git cache folders in a staging dir and treat .bun-tag as the cache-hit marker Git checkouts (@G@) and bare mirrors (.git) were cloned straight into their final cache name, so an install killed mid-way left a folder that every later install trusted: an empty checkout resolved as an empty package and a half-cloned mirror failed every later fetch. Both are now built under a temporary sibling in the cache dir and renamed into place once complete, the same way tarball extraction already works. Cache-hit probes for unpatched entries go through one helper: npm folders must contain package.json, git checkouts the .bun-tag written last. This replaces the bare directory probes in determine_preinstall_state and the hoisted/isolated installers (and the hoisted installer's in-place mutation of the shared folder-name buffer). Since the tag is now the marker, checkout() replaces anything a repository checked in under that name and fails instead of publishing a folder it could never hit. --- src/install/PackageInstall.rs | 83 ++---- .../PackageManagerDirectories.rs | 23 ++ .../PackageManager/PackageManagerLifecycle.rs | 10 +- src/install/isolated_install.rs | 24 +- src/install/repository.rs | 259 ++++++++++++------ test/cli/install/bun-install.test.ts | 73 ++++- 6 files changed, 298 insertions(+), 174 deletions(-) diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index 43521627fdef..8cc7a2056efe 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -2278,68 +2278,27 @@ impl<'a> PackageInstall<'a> { let state = manager.get_preinstall_state(package_id); match state { crate::PreinstallState::Done => false, - _ => 'brk: { - if self.patch.is_none() { - let exists = match resolution_tag { - resolution::Tag::Npm => 'package_json_exists: { - // SAFETY: `buf` and `self.cache_dir_subpath` both derive from the - // same thread-local `cached_package_folder_name_buf` raw pointer - // (the debug_assert below checks the subpath aliases this buffer), - // so there is no cross-thread access. No other `&mut` into the - // buffer is created while `buf` is live, and the only writes are - // at indices >= `subpath_len` — past the subpath's contents — with - // the NUL terminator restored by the scopeguard before the borrow - // ends. - let buf: &mut [u8] = unsafe { - (*crate::package_manager::cached_package_folder_name_buf()) - .as_mut_slice() - }; - - debug_assert!(bun_core::is_slice_in_buffer( - self.cache_dir_subpath.as_bytes(), - buf - )); - - let subpath_len = - strings::without_trailing_slash(self.cache_dir_subpath.as_bytes()) - .len(); - buf[subpath_len] = SEP; - // SAFETY: p points into the long-lived cached_package_folder_name_buf; - // subpath_len is in bounds (was the prior NUL position). - let _restore = - scopeguard::guard(buf.as_mut_ptr(), move |p: *mut u8| unsafe { - *p.add(subpath_len) = 0; - }); - buf[subpath_len + 1..subpath_len + 1 + b"package.json\0".len()] - .copy_from_slice(b"package.json\0"); - // SAFETY: NUL written above. - let subpath = - ZStr::from_buf(&buf[..], subpath_len + 1 + b"package.json".len()); - break 'package_json_exists sys::exists_at(self.cache_dir, subpath); - } - _ => sys::directory_exists_at(self.cache_dir, self.cache_dir_subpath) - .unwrap_or(false), - }; - if exists { - manager.set_preinstall_state(package_id, crate::PreinstallState::Done); - } - break 'brk !exists; - } - let idx = strings::last_index_of(self.cache_dir_subpath.as_bytes(), b"_patch_hash=") - .unwrap_or_else(|| { - panic!("Patched dependency cache dir subpath does not have the \"_patch_hash=HASH\" suffix. This is a bug, please file a GitHub issue.") - }); - let cache_dir_subpath_without_patch_hash = - &self.cache_dir_subpath.as_bytes()[..idx]; - // Use a stack PathBuffer (no shared state). - let mut join_buf = PathBuffer::uninit(); - join_buf[..cache_dir_subpath_without_patch_hash.len()] - .copy_from_slice(cache_dir_subpath_without_patch_hash); - join_buf[cache_dir_subpath_without_patch_hash.len()] = 0; - // SAFETY: NUL written above. - let subpath = - ZStr::from_buf(&join_buf[..], cache_dir_subpath_without_patch_hash.len()); - let exists = sys::directory_exists_at(self.cache_dir, subpath).unwrap_or(false); + _ => { + let exists = if self.patch.is_none() { + crate::package_manager::directories::is_package_in_cache_at( + self.cache_dir, + self.cache_dir_subpath, + resolution_tag, + ) + } else { + let idx = + strings::last_index_of(self.cache_dir_subpath.as_bytes(), b"_patch_hash=") + .unwrap_or_else(|| { + panic!("Patched dependency cache dir subpath does not have the \"_patch_hash=HASH\" suffix. This is a bug, please file a GitHub issue.") + }); + let non_patched = + bun_core::ZBox::from_bytes(&self.cache_dir_subpath.as_bytes()[..idx]); + crate::package_manager::directories::is_package_in_cache_at( + self.cache_dir, + &non_patched, + resolution_tag, + ) + }; if exists { manager.set_preinstall_state(package_id, crate::PreinstallState::Done); } diff --git a/src/install/PackageManager/PackageManagerDirectories.rs b/src/install/PackageManager/PackageManagerDirectories.rs index 950f3ef74b54..5a2097bab505 100644 --- a/src/install/PackageManager/PackageManagerDirectories.rs +++ b/src/install/PackageManager/PackageManagerDirectories.rs @@ -754,6 +754,29 @@ pub fn is_folder_in_cache(this: &mut PackageManager, folder_path: &ZStr) -> bool sys::directory_exists_at(get_cache_directory(this), folder_path).unwrap_or(false) } +/// Cache hit for an unpatched entry: npm folders must contain `package.json`, git checkouts the `.bun-tag` written last. +pub fn is_package_in_cache_at(cache_dir: Fd, folder_path: &ZStr, tag: ResolutionTag) -> bool { + let marker: &[u8] = match tag { + ResolutionTag::Npm => b"package.json", + ResolutionTag::Git => b".bun-tag", + _ => return sys::directory_exists_at(cache_dir, folder_path).unwrap_or(false), + }; + let mut buf = PathBuffer::uninit(); + let marker_path = path::resolve_path::join_z_buf::( + &mut buf.0, + &[folder_path.as_bytes(), marker], + ); + sys::exists_at(cache_dir, marker_path) +} + +pub fn is_package_in_cache( + this: &mut PackageManager, + folder_path: &ZStr, + tag: ResolutionTag, +) -> bool { + is_package_in_cache_at(get_cache_directory(this), folder_path, tag) +} + // ─────────────────────────── global directories ─────────────────────────────── pub fn setup_global_dir(manager: &mut PackageManager, ctx: &Command::Context) -> Result<(), Error> { diff --git a/src/install/PackageManager/PackageManagerLifecycle.rs b/src/install/PackageManager/PackageManagerLifecycle.rs index b1845b30b495..63730f9876a4 100644 --- a/src/install/PackageManager/PackageManagerLifecycle.rs +++ b/src/install/PackageManager/PackageManagerLifecycle.rs @@ -158,7 +158,12 @@ impl PackageManager { return PreinstallState::Extract; } - if directories::is_folder_in_cache(self, folder_path) { + let in_cache = if patch_hash.is_some() { + directories::is_folder_in_cache(self, folder_path) + } else { + directories::is_package_in_cache(self, folder_path, pkg.resolution.tag) + }; + if in_cache { self.set_preinstall_state(pkg.meta.id, PreinstallState::Done); return PreinstallState::Done; } @@ -181,7 +186,8 @@ impl PackageManager { }); // Owned NUL-terminated copy. let non_patched_path = ZBox::from_bytes(&folder_path.as_bytes()[..idx]); - if directories::is_folder_in_cache(self, &non_patched_path) { + if directories::is_package_in_cache(self, &non_patched_path, pkg.resolution.tag) + { self.set_preinstall_state(pkg.meta.id, PreinstallState::ApplyPatch); // yay step 1 is already done for us return PreinstallState::ApplyPatch; diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index b406dd64f6cf..d8bf8c7f6fa2 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2348,25 +2348,11 @@ pub(crate) fn install_isolated_packages( install::PreinstallState::Done => false, _ => 'missing_from_cache: { if matches!(patch_info, installer::PatchInfo::None) { - let exists = match pkg_res_tag { - ResolutionTag::Npm => { - // Reshaped for borrowck — capture length - // instead of `save()` so the path stays unborrowed. - let cache_dir_path_save = pkg_cache_dir_subpath.len(); - pkg_cache_dir_subpath.append(b"package.json").assume_ok(); - let exists = sys::exists_at( - cache_dir, - pkg_cache_dir_subpath.slice_z(), - ); - pkg_cache_dir_subpath.set_length(cache_dir_path_save); - exists - } - _ => sys::directory_exists_at( - cache_dir, - pkg_cache_dir_subpath.slice_z(), - ) - .unwrap_or(false), - }; + let exists = package_manager::directories::is_package_in_cache_at( + cache_dir, + pkg_cache_dir_subpath.slice_z(), + pkg_res_tag, + ); if exists { installer.manager_mut().set_preinstall_state( pkg_id, diff --git a/src/install/repository.rs b/src/install/repository.rs index 3e2e9cb3152a..f1271f2119c9 100644 --- a/src/install/repository.rs +++ b/src/install/repository.rs @@ -421,6 +421,77 @@ fn exec(env: &bun_dotenv::Map, argv: &[&[u8]]) -> Result, Error> { Err(crate::Error::InstallFailed) } +/// A cache folder is built under a temporary sibling name and renamed onto `folder_name` once complete. +struct CacheStaging { + cache_dir: bun_sys::Fd, + tmp_name_buf: [u8; 64], + tmp_name_len: usize, +} + +impl CacheStaging { + fn new(cache_dir: bun_sys::Fd) -> Result { + let mut tmp_name_buf = [0u8; 64]; + let tmp_name_len = + Path::fs::FileSystem::tmpname(b"tmp", &mut tmp_name_buf, bun_core::fast_random()) + .map_err(|_| crate::Error::Sys(bun_errno::SystemErrno::ENOSPC))? + .len(); + Ok(Self { + cache_dir, + tmp_name_buf, + tmp_name_len, + }) + } + + fn tmp_name(&self) -> &[u8] { + &self.tmp_name_buf[..self.tmp_name_len] + } + + fn tmp_path(&self) -> &'static [u8] { + Path::resolve_path::join_abs_string::( + &PackageManager::get().cache_directory_path, + &[self.tmp_name()], + ) + } + + fn discard(&self) { + let _ = bun_sys::Dir::borrow(&self.cache_dir).delete_tree(self.tmp_name()); + } + + fn publish( + self, + log: &mut bun_ast::Log, + name: &[u8], + folder_name: &[u8], + ) -> Result { + let renamed = bun_sys::renameat_concurrently_a( + self.cache_dir, + self.tmp_name(), + self.cache_dir, + folder_name, + bun_sys::RenameatConcurrentlyOptions { + move_fallback: false, + }, + ); + // After an exchange the temporary name holds the folder that was replaced. + self.discard(); + if let Err(err) = renamed { + log.add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!( + "moving \"{}\" to cache dir failed: {}", + BStr::new(name), + err + ), + ); + return Err(crate::Error::InstallFailed); + } + bun_sys::Dir::borrow(&self.cache_dir) + .open_at(folder_name) + .map_err(Error::from) + } +} + impl RepositoryExt for Repository { fn parse_append_git(input: &[u8], buf: &mut StringBuf<'_>) -> Result { let mut remain = input; @@ -737,11 +808,7 @@ impl RepositoryExt for Repository { return Err(not_found.into()); } - let target = Path::resolve_path::join_abs_string::( - &PackageManager::get().cache_directory_path, - &[folder_name.as_bytes()], - ); - + let staging = CacheStaging::new(cache_dir)?; if let Err(err) = exec( env, &[ @@ -752,9 +819,10 @@ impl RepositoryExt for Repository { b"--quiet", b"--bare", url, - target, + staging.tmp_path(), ], ) { + staging.discard(); if err == crate::Error::RepositoryNotFound || attempt > 1 { log.add_error_fmt( None, @@ -765,9 +833,7 @@ impl RepositoryExt for Repository { return Err(err); } - bun_sys::Dir::borrow(&cache_dir) - .open_dir_z(folder_name) - .map_err(Into::into) + staging.publish(log, name, folder_name.as_bytes()) } } } @@ -874,97 +940,114 @@ impl RepositoryExt for Repository { ) .as_bytes(); - let package_dir = match bun_sys::Dir::borrow(&cache_dir) - .open_at(folder_name) - .map_err(Error::from) - { - Ok(d) => d, - Err(not_found) => 'brk: { - if not_found != crate::Error::Sys(bun_errno::SystemErrno::ENOENT) { - return Err(not_found); + let package_dir = 'brk: { + match bun_sys::Dir::borrow(&cache_dir).open_at(folder_name) { + Ok(dir) => { + if bun_sys::exists_at(dir.fd(), bun_core::zstr!(".bun-tag")) { + break 'brk dir; + } + dir.close(); } + Err(err) if err.get_errno() == bun_sys::E::ENOENT => {} + Err(err) => return Err(err.into()), + } + let repo_path = bun_sys::get_fd_path( + repo_dir, + // Per-field accessor — disjoint from `folder_name_buf` + // borrow above. See `TlBufs` accessor doc. + TlBufs::final_path_buf(), + )?; - let target = Path::resolve_path::join_abs_string::( - &PackageManager::get().cache_directory_path, - &[folder_name], + let staging = CacheStaging::new(cache_dir)?; + if let Err(err) = exec( + env, + &[ + b"git", + b"clone", + b"-c", + b"core.longpaths=true", + b"--quiet", + b"--no-checkout", + repo_path, + staging.tmp_path(), + ], + ) { + staging.discard(); + log.add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!("\"git clone\" for \"{}\" failed", BStr::new(name)), ); + return Err(err); + } - let repo_path = bun_sys::get_fd_path( - repo_dir, - // Per-field accessor — disjoint from `folder_name_buf` - // borrow above. See `TlBufs` accessor doc. - TlBufs::final_path_buf(), - )?; - - if let Err(err) = exec( - env, - &[ - b"git", - b"clone", - b"-c", - b"core.longpaths=true", - b"--quiet", - b"--no-checkout", - repo_path, - target, - ], - ) { - log.add_error_fmt( - None, - bun_ast::Loc::EMPTY, - format_args!("\"git clone\" for \"{}\" failed", BStr::new(name)), - ); - return Err(err); - } - - let folder = Path::resolve_path::join_abs_string::( - &PackageManager::get().cache_directory_path, - &[folder_name], + if let Err(err) = exec( + env, + // `is_safe_resolved_tag` above rejects a leading `-`, so + // `resolved` cannot be parsed as a git option. + &[ + b"git", + b"-C", + staging.tmp_path(), + b"checkout", + b"--quiet", + resolved, + ], + ) { + staging.discard(); + log.add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!("\"git checkout\" for \"{}\" failed", BStr::new(name)), ); + return Err(err); + } + { + let dir = match bun_sys::Dir::borrow(&cache_dir).open_at(staging.tmp_name()) { + Ok(dir) => dir, + Err(err) => { + staging.discard(); + return Err(err.into()); + } + }; + let _ = dir.delete_tree(b".git"); + // Unlinks a `node_modules` link only; directories are kept (bundleDependencies). + let _ = dir.delete_file_z(bun_core::zstr!("node_modules")); - if let Err(err) = exec( - env, - // `is_safe_resolved_tag` above rejects a leading `-`, so - // `resolved` cannot be parsed as a git option. - &[b"git", b"-C", folder, b"checkout", b"--quiet", resolved], - ) { + // `.bun-tag` is the cache-hit marker, so anything the repository checked in under that name is replaced. + let _ = dir.delete_file_z(bun_core::zstr!(".bun-tag")); + let tagged = bun_sys::File::openat( + dir.fd(), + bun_core::zstr!(".bun-tag"), + bun_sys::O::WRONLY + | bun_sys::O::CREAT + | bun_sys::O::EXCL + | if cfg!(windows) { + 0 + } else { + bun_sys::O::NOFOLLOW + }, + 0o664, + ) + .and_then(|f| f.write_all(resolved)); + // Windows cannot rename a directory with an open handle inside it. + dir.close(); + if let Err(err) = tagged { + staging.discard(); log.add_error_fmt( None, bun_ast::Loc::EMPTY, - format_args!("\"git checkout\" for \"{}\" failed", BStr::new(name)), + format_args!( + "writing \".bun-tag\" for \"{}\" failed: {}", + BStr::new(name), + BStr::new(err.name()) + ), ); - return Err(err); - } - let dir = bun_sys::Dir::borrow(&cache_dir) - .open_at(folder_name) - .map_err(Error::from)?; - let _ = dir.delete_tree(b".git"); - // Unlinks a `node_modules` link only; directories are kept (bundleDependencies). - let _ = dir.delete_file_z(bun_core::zstr!("node_modules")); - - if !resolved.is_empty() { - if bun_sys::File::openat( - dir.fd(), - bun_core::zstr!(".bun-tag"), - bun_sys::O::WRONLY - | bun_sys::O::CREAT - | bun_sys::O::TRUNC - | if cfg!(windows) { - 0 - } else { - bun_sys::O::NOFOLLOW - }, - 0o664, - ) - .and_then(|f| f.write_all(resolved)) - .is_err() - { - let _ = dir.delete_file_z(bun_core::zstr!(".bun-tag")); - } + return Err(crate::Error::InstallFailed); } - - break 'brk dir; } + + staging.publish(log, name, folder_name)? }; let (json_file, json_buf) = diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index e35753d1be20..d2bc50cce667 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -5379,9 +5379,11 @@ describe.concurrent("bun-install", () => { expect(err).toContain("Saved lockfile"); expect(out).toContain("1 package installed"); expect(readFileSync(target, "utf8")).toBe("original\n"); - expect(await readdirSorted(join(ctx.package_dir, "node_modules", ".cache", `@G@${sha}`))).toEqual( - isWindows ? [".bun-tag", "package.json"] : ["package.json"], - ); + expect(await readdirSorted(join(ctx.package_dir, "node_modules", ".cache", `@G@${sha}`))).toEqual([ + ".bun-tag", + "package.json", + ]); + expect(await file(join(ctx.package_dir, "node_modules", ".cache", `@G@${sha}`, ".bun-tag")).text()).toBe(sha); expect(await file(join(ctx.package_dir, "node_modules", "has-bun-tag", "package.json")).json()).toEqual({ name: "has-bun-tag", version: "1.0.0", @@ -5391,6 +5393,71 @@ describe.concurrent("bun-install", () => { }); }); + it("git checkout cache folders appear only once complete and are hit only when tagged", async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, dummyRegistryForContext(ctx, urls)); + using dir = tempDir("git-dep-checkout-fails", { + "work/package.json": JSON.stringify({ name: "checkout-fails", version: "1.0.0" }), + }); + const sha = await createDumbHttpGitRepo(String(dir), {}); + const treeSha = await git(join(String(dir), "work"), ["rev-parse", "HEAD^{tree}"]); + using server = serveDirectory(String(dir)); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { "checkout-fails": `git+http://localhost:${server.port}/repo.git` }, + }), + ); + async function install() { + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stdin: "ignore", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { out, err, exitCode }; + } + const cache = join(ctx.package_dir, "node_modules", ".cache"); + + expect(await install()).toMatchObject({ exitCode: 0 }); + expect(await readdirSorted(join(cache, `@G@${sha}`))).toEqual([".bun-tag", "package.json"]); + const mirror = (await readdirSorted(cache)).find(entry => entry.endsWith(".git"))!; + + // `git log` during resolution does not need the tree object, but `git checkout` cannot unpack without it. + const treeObject = join(cache, mirror, "objects", treeSha.slice(0, 2), treeSha.slice(2)); + const treeBytes = await file(treeObject).bytes(); + await rm(treeObject); + await rm(join(cache, `@G@${sha}`), { recursive: true }); + await rm(join(ctx.package_dir, "node_modules", "checkout-fails"), { recursive: true }); + const failed = await install(); + expect(failed.err).toContain('"git checkout" for "checkout-fails" failed'); + expect(failed.exitCode).not.toBe(0); + expect(await readdirSorted(cache)).toEqual([mirror]); + + await write(treeObject, treeBytes); + expect(await install()).toMatchObject({ exitCode: 0 }); + expect(await readdirSorted(join(cache, `@G@${sha}`))).toEqual([".bun-tag", "package.json"]); + + // A folder at the cache name without `.bun-tag` (left by older versions) is not a cache hit. + await rm(join(cache, `@G@${sha}`), { recursive: true }); + await mkdir(join(cache, `@G@${sha}`)); + await rm(join(ctx.package_dir, "node_modules", "checkout-fails"), { recursive: true }); + expect(await install()).toMatchObject({ exitCode: 0 }); + expect(await readdirSorted(join(cache, `@G@${sha}`))).toEqual([".bun-tag", "package.json"]); + expect(await readdirSorted(join(ctx.package_dir, "node_modules", "checkout-fails"))).toEqual([ + ".bun-tag", + "package.json", + ]); + expect(urls).toBeEmpty(); + }); + }); + it("should fail on invalid Git URL", async () => { await withContext(defaultOpts, async ctx => { const urls: string[] = [];