From 7a6699818314effdbe4d1bc88033d72481326666 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:44:48 +0000 Subject: [PATCH] install: cache file: tarballs under the integrity the lockfile pins A file: tarball's cache entry was named after the hash of its resolution string, which is the path as written in package.json ("pkg.tgz"). That path names a different tarball in every project sharing the cache, and an install from a lockfile never reads the tarball itself, so whichever project extracted last had its tarball installed into every other project depending on the same relative path, with the lockfile's integrity never consulted. Name the entry after the integrity recorded for the package instead (@T@sha512-@@@1), the same way git checkouts are named after their commit. Extraction names the entry after the integrity it verified (or computed, on first install), so an entry is only ever reused for the bytes the lockfile pins, and different tarballs at the same path coexist. A lockfile that predates tarball integrity cannot name an entry; such a package is extracted, and both installers record the computed integrity before the waiting installs run (the hoisted installer already did; the isolated one now receives the extraction result and does the same), so the lockfile is re-saved with it and the next install is a cache hit. bun patch and patch --commit report a missing integrity instead of diffing or copying against a folder that cannot be named. URL tarballs are unchanged. --- src/install/PackageInstall.rs | 5 + src/install/PackageInstaller.rs | 8 +- src/install/PackageManager.rs | 3 +- .../PackageManagerDirectories.rs | 66 ++++++++- .../PackageManager/PackageManagerLifecycle.rs | 5 +- src/install/PackageManager/patchPackage.rs | 13 +- src/install/PackageManager/runTasks.rs | 14 +- src/install/TarballStream.rs | 16 +-- src/install/extract_tarball.rs | 64 +++++---- src/install/integrity.rs | 23 +++- src/install/isolated_install.rs | 54 ++++---- src/install/isolated_install/Installer.rs | 63 ++++++++- src/install/patch_install.rs | 4 +- .../bun-install-tarball-integrity.test.ts | 126 ++++++++++++++++++ 14 files changed, 374 insertions(+), 90 deletions(-) diff --git a/src/install/PackageInstall.rs b/src/install/PackageInstall.rs index 6adf994d1ad9..0e890e476c6c 100644 --- a/src/install/PackageInstall.rs +++ b/src/install/PackageInstall.rs @@ -2248,6 +2248,11 @@ impl<'a> PackageInstall<'a> { package_id: PackageID, resolution_tag: resolution::Tag, ) -> bool { + // No entry can be named yet (a local tarball whose integrity the + // lockfile does not record); extracting it is what names one. + if self.cache_dir_subpath.is_empty() { + return true; + } let state = manager.get_preinstall_state(package_id); match state { crate::PreinstallState::Done => false, diff --git a/src/install/PackageInstaller.rs b/src/install/PackageInstaller.rs index 6c96867f593b..4c59436f26a3 100644 --- a/src/install/PackageInstaller.rs +++ b/src/install/PackageInstaller.rs @@ -1117,7 +1117,8 @@ impl<'a> PackageInstaller<'a> { // If a newly computed integrity hash is available (e.g. for a GitHub // tarball) and the lockfile doesn't already have one, persist it so - // the lockfile gets re-saved with the hash. + // the lockfile gets re-saved with the hash. Must happen before the + // callbacks below: a local tarball's cache entry is named after it. if data.integrity.tag.is_supported() { let pkg_metas = self.lockfile_mut().packages.items_meta_mut(); if !pkg_metas[package_id as usize].integrity.tag.is_supported() { @@ -1523,9 +1524,8 @@ impl<'a> PackageInstaller<'a> { } } resolution::Tag::LocalTarball => { - installer.cache_dir_subpath = package_manager::cached_tarball_folder_name( - self.manager_mut(), - *resolution.local_tarball(), + installer.cache_dir_subpath = package_manager::cached_local_tarball_folder_name( + &self.metas[package_id as usize].integrity, patch_contents_hash, ); installer.cache_dir = package_manager::get_cache_directory(self.manager_mut()); diff --git a/src/install/PackageManager.rs b/src/install/PackageManager.rs index e1524675ffb9..4c4f8e44a3ec 100644 --- a/src/install/PackageManager.rs +++ b/src/install/PackageManager.rs @@ -203,7 +203,8 @@ use directories::attempt_to_create_package_json_and_open; pub use directories::{ attempt_to_create_package_json, cached_git_folder_name, cached_git_folder_name_print, cached_git_folder_name_print_auto, cached_github_folder_name, cached_github_folder_name_print, - cached_github_folder_name_print_auto, cached_npm_package_folder_name, + cached_github_folder_name_print_auto, cached_local_tarball_folder_name, + cached_local_tarball_folder_name_print, cached_npm_package_folder_name, cached_npm_package_folder_name_print, cached_npm_package_folder_print_basename, cached_tarball_folder_name, cached_tarball_folder_name_print, compute_cache_dir_and_subpath, fetch_cache_directory_path, get_cache_directory, get_cache_directory_and_abs_path, diff --git a/src/install/PackageManager/PackageManagerDirectories.rs b/src/install/PackageManager/PackageManagerDirectories.rs index 4861484c4b21..8b4c09be09ca 100644 --- a/src/install/PackageManager/PackageManagerDirectories.rs +++ b/src/install/PackageManager/PackageManagerDirectories.rs @@ -11,7 +11,7 @@ use bun_core::{Global, Output, ZBox, env_var, fmt as bun_fmt}; use bun_dotenv::Loader as DotEnvLoader; use bun_install::lockfile::{Format as LockfileFormat, LoadResult, Lockfile}; use bun_install::resolution::Tag as ResolutionTag; -use bun_install::{PackageID, Resolution}; +use bun_install::{Integrity, PackageID, Resolution}; use bun_paths::{self as path, AbsPath, PathBuffer, SEP}; use bun_semver::{self as Semver, String as SemverString}; #[cfg(windows)] @@ -487,6 +487,14 @@ impl<'a> ByteCursor<'a> { self.put(bun_fmt::u64_hex_var_lower(&mut tmp, n)); } + /// Two lower-hex digits per byte. + #[inline(always)] + fn put_hex_bytes(&mut self, bytes: &[u8]) { + let end = self.at + bytes.len() * 2; + bun_fmt::bytes_to_hex_lower(bytes, &mut self.buf[self.at..end]); + self.at = end; + } + /// `@@@{d}` when set. #[inline(always)] fn put_cache_version(&mut self, v: Option) { @@ -725,6 +733,8 @@ pub fn cached_npm_package_folder_print_basename<'a>( w.finish_z() } +/// `@T@@@@1`, for URL tarballs. `file:` tarballs use +/// `cached_local_tarball_folder_name_print`. pub fn cached_tarball_folder_name_print<'a>( buf: &'a mut [u8], url: &[u8], @@ -750,6 +760,43 @@ pub fn cached_tarball_folder_name( ) } +/// `@T@sha512-@@@1`. +/// +/// A `file:` tarball's resolution is the path as written in package.json +/// (`pkg.tgz`), which names a different tarball in every project sharing the +/// cache, so unlike a URL tarball it is cached under the integrity bun.lock +/// pins for it: the entry is only reused for the bytes it was extracted from. +/// +/// Empty when the integrity is not known yet (lockfile written before tarball +/// integrity was recorded). Callers treat that like a cache miss: extracting the +/// tarball computes the integrity, and the install callbacks record it in the +/// lockfile before installing from the entry named after it. +pub fn cached_local_tarball_folder_name_print<'a>( + buf: &'a mut [u8], + integrity: &Integrity, + patch_hash: Option, +) -> &'a ZStr { + let Some(algorithm) = integrity.tag.name() else { + return ZStr::EMPTY; + }; + let digest = integrity.slice(); + let mut w = ByteCursor::new(buf); + w.put(b"@T@"); + w.put(algorithm.as_bytes()); + w.put_byte(b'-'); + w.put_hex_bytes(&digest[..digest.len().min(16)]); + w.put_cache_version(Some(CacheVersion::CURRENT)); + w.put_patch_hash(patch_hash); + w.finish_z() +} + +pub fn cached_local_tarball_folder_name( + integrity: &Integrity, + patch_hash: Option, +) -> &'static ZStr { + cached_local_tarball_folder_name_print(cached_package_folder_name_buf(), integrity, patch_hash) +} + 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) } @@ -947,6 +994,7 @@ pub fn compute_cache_dir_and_subpath<'a>( manager: &mut PackageManager, pkg_name: &[u8], resolution: &Resolution, + integrity: &Integrity, folder_path_buf: &'a mut PathBuffer, patch_hash: Option, ) -> CacheDirAndSubpath<'a> { @@ -986,8 +1034,20 @@ pub fn compute_cache_dir_and_subpath<'a>( cache_dir = Fd::cwd(); } ResolutionTag::LocalTarball => { - let tarball = *resolution.local_tarball(); - cache_dir_subpath = cached_tarball_folder_name(manager, tarball, patch_hash); + cache_dir_subpath = cached_local_tarball_folder_name(integrity, patch_hash); + if cache_dir_subpath.is_empty() { + Output::err_generic( + "the lockfile does not record an integrity for {}@{}, run bun install first", + ( + bun_fmt::s(name), + resolution.fmt( + manager.lockfile.buffers.string_bytes.as_slice(), + bun_fmt::PathSep::Posix, + ), + ), + ); + Global::exit(1); + } cache_dir = get_cache_directory(manager); } ResolutionTag::RemoteTarball => { diff --git a/src/install/PackageManager/PackageManagerLifecycle.rs b/src/install/PackageManager/PackageManagerLifecycle.rs index 63730f9876a4..ede34ff115b8 100644 --- a/src/install/PackageManager/PackageManagerLifecycle.rs +++ b/src/install/PackageManager/PackageManagerLifecycle.rs @@ -140,9 +140,8 @@ impl PackageManager { patch_hash, ) } - ResolutionTag::LocalTarball => directories::cached_tarball_folder_name( - self, - *pkg.resolution.local_tarball(), + ResolutionTag::LocalTarball => directories::cached_local_tarball_folder_name( + &pkg.meta.integrity, patch_hash, ), ResolutionTag::RemoteTarball => directories::cached_tarball_folder_name( diff --git a/src/install/PackageManager/patchPackage.rs b/src/install/PackageManager/patchPackage.rs index 49029f7cd401..8709f93b8596 100644 --- a/src/install/PackageManager/patchPackage.rs +++ b/src/install/PackageManager/patchPackage.rs @@ -270,8 +270,14 @@ pub fn do_patch_commit( // `compute_cache_dir_and_subpath` resolves `pkg.resolution`'s strings against `manager.lockfile`. manager.lockfile = lockfile; let name = manager.lockfile.str(&pkg.name).to_vec(); - let cache_result = - compute_cache_dir_and_subpath(manager, &name, &pkg.resolution, &mut folder_path_buf, None); + let cache_result = compute_cache_dir_and_subpath( + manager, + &name, + &pkg.resolution, + &pkg.meta.integrity, + &mut folder_path_buf, + None, + ); let cache_dir: Fd = cache_result.cache_dir; let cache_dir_subpath: &ZStr = cache_result.cache_dir_subpath; let changes_dir: &[u8] = &changes_dir; @@ -858,6 +864,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> { manager, &name, &actual_package.resolution, + &actual_package.meta.integrity, &mut folder_path_buf, existing_patchfile_hash, ); @@ -914,10 +921,12 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> { }; let pkg_resolution = pkg.resolution; + let pkg_integrity = pkg.meta.integrity; let cache_result = compute_cache_dir_and_subpath( manager, &pkg_name, &pkg_resolution, + &pkg_integrity, &mut folder_path_buf, existing_patchfile_hash, ); diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 05c3c87d1002..dd674fb65f00 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -110,7 +110,11 @@ pub trait RunTasksCallbacks { ) { unreachable!() } - fn on_extract_store_installer(_ctx: &mut Self::Ctx, _task_id: Task::Id) { + fn on_extract_store_installer( + _ctx: &mut Self::Ctx, + _task_id: Task::Id, + _data: &bun_install::ExtractData, + ) { unreachable!() } @@ -1146,7 +1150,7 @@ pub fn run_tasks( log_level, ); } else if C::IS_STORE_INSTALLER { - C::on_extract_store_installer(extract_ctx, task.id); + C::on_extract_store_installer(extract_ctx, task.id, task.data_extract()); } else { unreachable!("unexpected context type"); } @@ -1475,7 +1479,11 @@ pub fn run_tasks( log_level, ); } else if C::IS_STORE_INSTALLER { - C::on_extract_store_installer(extract_ctx, task.id); + C::on_extract_store_installer( + extract_ctx, + task.id, + task.data_git_checkout(), + ); } else { unreachable!("unexpected context type"); } diff --git a/src/install/TarballStream.rs b/src/install/TarballStream.rs index 7ae9f698469b..b30138842632 100644 --- a/src/install/TarballStream.rs +++ b/src/install/TarballStream.rs @@ -1153,12 +1153,14 @@ impl TarballStream { let (name, basename) = tarball.name_and_basename(); + let integrity = tarball.lockfile_integrity(|| self.hasher.final_()); let mut result = match tarball.move_to_cache_directory( &mut (*task).log, self.tmpname.as_zstr(), name, basename, self.resolved_github_dirname, + &integrity, ) { Ok(r) => r, Err(err) => { @@ -1167,19 +1169,7 @@ impl TarballStream { return; } }; - - match tarball.resolution.tag { - ResolutionTag::Github - | ResolutionTag::RemoteTarball - | ResolutionTag::LocalTarball => { - if tarball.integrity.tag.is_supported() { - result.integrity = tarball.integrity; - } else { - result.integrity = self.hasher.final_(); - } - } - _ => {} - } + result.integrity = integrity; if PackageManager::verbose_install() { bun_core::pretty_errorln!( diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index b8d3a8b091b5..c1dcf39f1492 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -57,28 +57,30 @@ impl ExtractTarball { return Err(crate::Error::IntegrityCheckFailed); } } - let mut result = self.extract(log, bytes)?; + let integrity = self.lockfile_integrity(|| Integrity::for_bytes(bytes)); + let mut result = self.extract(log, bytes, &integrity)?; + result.integrity = integrity; + Ok(result) + } - // Compute and store SHA-512 integrity hash for GitHub / URL / local tarballs - // so the lockfile can pin the exact tarball content. On subsequent installs - // the hash stored in the lockfile is forwarded via this.integrity and verified - // above, preventing a compromised server from silently swapping the tarball. + /// The integrity the lockfile records for a GitHub / URL / local tarball, so + /// later installs verify the same bytes (see `run`). That is the value the + /// lockfile already pins (verified before extraction), or else `compute` from + /// the bytes on the first install. A local tarball's cache entry is named + /// after it (`move_to_cache_directory`), which is why it is settled before + /// extracting. Unknown for npm packages, whose integrity comes from the + /// registry manifest. + pub(crate) fn lockfile_integrity(&self, compute: impl FnOnce() -> Integrity) -> Integrity { match self.resolution.tag { ResolutionTag::Github | ResolutionTag::RemoteTarball | ResolutionTag::LocalTarball => { if self.integrity.tag.is_supported() { - // Re-installing with an existing lockfile: integrity was already - // verified above, propagate the known value to ExtractData so that - // the lockfile keeps it on re-serialisation. - result.integrity = self.integrity; + self.integrity } else { - // First install (no integrity in the lockfile yet): compute it. - result.integrity = Integrity::for_bytes(bytes); + compute() } } - _ => {} + _ => Integrity::default(), } - - Ok(result) } } @@ -225,7 +227,12 @@ impl ExtractTarball { (name, basename) } - fn extract(&self, log: &mut bun_ast::Log, tgz_bytes: &[u8]) -> Result { + fn extract( + &self, + log: &mut bun_ast::Log, + tgz_bytes: &[u8], + integrity: &Integrity, + ) -> Result { let _tracer = bun_core::perf::trace("ExtractTarball.extract"); let tmpdir = Dir::borrow(&self.temp_dir); @@ -439,12 +446,16 @@ impl ExtractTarball { } } - self.move_to_cache_directory(log, tmpname, name, basename, resolved) + self.move_to_cache_directory(log, tmpname, name, basename, resolved, integrity) } /// Rename the freshly-extracted temp directory into the cache, read /// `package.json` if required, and build the `ExtractData` result. Shared /// between the buffered and streaming extraction paths. + /// + /// `resolved` (GitHub) and `integrity` (local tarballs, see + /// `lockfile_integrity`) name the cache entry for the resolutions whose + /// entries are keyed by content rather than by the resolution string. pub(crate) fn move_to_cache_directory( &self, log: &mut bun_ast::Log, @@ -452,6 +463,7 @@ impl ExtractTarball { name: &[u8], basename: &[u8], resolved: &[u8], + integrity: &Integrity, ) -> Result { let package_manager = self.package_manager.get(); @@ -500,14 +512,18 @@ impl ExtractTarball { ) .as_bytes() } - ResolutionTag::LocalTarball | ResolutionTag::RemoteTarball => { - directories::cached_tarball_folder_name_print( - &mut bufs.folder_name_buf, - self.url.slice(), - None, - ) - .as_bytes() - } + ResolutionTag::LocalTarball => directories::cached_local_tarball_folder_name_print( + &mut bufs.folder_name_buf, + integrity, + None, + ) + .as_bytes(), + ResolutionTag::RemoteTarball => directories::cached_tarball_folder_name_print( + &mut bufs.folder_name_buf, + self.url.slice(), + None, + ) + .as_bytes(), _ => unreachable!(), }; if folder_name.is_empty() || (folder_name.len() == 1 && folder_name[0] == b'/') { diff --git a/src/install/integrity.rs b/src/install/integrity.rs index e5ae223d335e..12e459448358 100644 --- a/src/install/integrity.rs +++ b/src/install/integrity.rs @@ -243,13 +243,11 @@ impl Integrity { impl fmt::Display for Integrity { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.tag { - Tag::SHA1 => f.write_str("sha1-")?, - Tag::SHA256 => f.write_str("sha256-")?, - Tag::SHA384 => f.write_str("sha384-")?, - Tag::SHA512 => f.write_str("sha512-")?, - _ => return Ok(()), - } + let Some(algorithm) = self.tag.name() else { + return Ok(()); + }; + f.write_str(algorithm)?; + f.write_str("-")?; let mut base64_buf = [0u8; 512]; let bytes = self.slice(); @@ -293,6 +291,17 @@ impl Tag { self.0 >= Tag::SHA1.0 && self.0 <= Tag::SHA512.0 } + /// The algorithm part of the SRI string (`sha512` in `sha512-...`); `None` for `UNKNOWN`. + pub(crate) fn name(self) -> Option<&'static str> { + Some(match self { + Tag::SHA1 => "sha1", + Tag::SHA256 => "sha256", + Tag::SHA384 => "sha384", + Tag::SHA512 => "sha512", + _ => return None, + }) + } + pub(crate) fn parse(buf: &[u8]) -> (Tag, usize) { let Some(i) = strings::index_of_char(&buf[0..buf.len().min(7)], b'-') else { return (Tag::UNKNOWN, 0); diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index d89105ac2ead..eae844bddb45 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -143,8 +143,12 @@ impl<'a> run_tasks::RunTasksCallbacks for StoreRunTasksCallbacks<'a> { const HAS_ON_PACKAGE_DOWNLOAD_ERROR: bool = true; const IS_STORE_INSTALLER: bool = true; - fn on_extract_store_installer(ctx: &mut Self::Ctx, task_id: Task::Id) { - ctx.on_package_extracted(task_id); + fn on_extract_store_installer( + ctx: &mut Self::Ctx, + task_id: Task::Id, + data: &install::ExtractData, + ) { + ctx.on_package_extracted(task_id, data); } fn on_package_download_error_store( @@ -2334,11 +2338,12 @@ pub(crate) fn install_isolated_packages( pkg_res.github(), None, ), - ResolutionTag::LocalTarball => package_manager::cached_tarball_folder_name( - installer.manager(), - *pkg_res.local_tarball(), - None, - ), + ResolutionTag::LocalTarball => { + package_manager::cached_local_tarball_folder_name( + &pkgs.items_meta()[pkg_id as usize].integrity, + None, + ) + } ResolutionTag::RemoteTarball => { package_manager::cached_tarball_folder_name( installer.manager(), @@ -2353,23 +2358,26 @@ pub(crate) fn install_isolated_packages( installer.manager_mut().get_cache_directory_and_abs_path(); let _ = &cache_dir_path; // dropped at scope exit - let missing_from_cache = match installer.manager().get_preinstall_state(pkg_id) - { - install::PreinstallState::Done => false, - _ => { - let exists = package_manager::directories::is_package_in_cache_at( - cache_dir, - cache_subpath_z, - pkg_res_tag, - ); - if exists { - installer - .manager_mut() - .set_preinstall_state(pkg_id, install::PreinstallState::Done); + // An empty name is a local tarball whose integrity the lockfile + // does not record; extracting it is what names its entry. + let missing_from_cache = cache_subpath_z.is_empty() + || match installer.manager().get_preinstall_state(pkg_id) { + install::PreinstallState::Done => false, + _ => { + let exists = package_manager::directories::is_package_in_cache_at( + cache_dir, + cache_subpath_z, + pkg_res_tag, + ); + if exists { + installer.manager_mut().set_preinstall_state( + pkg_id, + install::PreinstallState::Done, + ); + } + !exists } - !exists - } - }; + }; if !missing_from_cache { if let installer::PatchInfo::Patch(patch) = &patch_info { diff --git a/src/install/isolated_install/Installer.rs b/src/install/isolated_install/Installer.rs index ce346a116b37..12383a8c66a8 100644 --- a/src/install/isolated_install/Installer.rs +++ b/src/install/isolated_install/Installer.rs @@ -36,7 +36,7 @@ use super::symlinker::{self, Symlinker}; use crate::bun_fs; use crate::lockfile_real::package::PackageColumns as _; use crate::package_manager_real::directories; -use crate::package_manager_real::package_manager_options::Do; +use crate::package_manager_real::package_manager_options::{Do, Enable}; /// The enum lives at module level in `crate::resolution`. type ResolutionTag = resolution::Tag; @@ -70,7 +70,9 @@ pub struct Installer<'a> { /// pool and each task derefs this field; a `&'a mut` would assert /// exclusivity every concurrent task violates. Mutated only for /// `lockfile.trusted_dependencies` (under `trusted_dependencies_mutex`, - /// narrowed via `addr_of_mut!`). Never null. Read via `lockfile()`. + /// narrowed via `addr_of_mut!`) and, on the main thread, a not-yet-started + /// package's `meta.integrity` (`record_extracted_integrity`, one row via + /// the raw column pointer). Never null. Read via `lockfile()`. pub lockfile: *mut Lockfile, pub(crate) summary: InstallSummary, @@ -181,7 +183,11 @@ impl<'a> Installer<'a> { self.start_task(entry_id); } - pub(crate) fn on_package_extracted(&mut self, task_id: crate::package_manager_task::Id) { + pub(crate) fn on_package_extracted( + &mut self, + task_id: crate::package_manager_task::Id, + data: &install::ExtractData, + ) { if let Some(removed) = self.manager_mut().task_queue.remove(&task_id) { let store = self.store; @@ -205,6 +211,11 @@ impl<'a> Installer<'a> { let node_id = entry_node_ids[entry_id.get() as usize]; let pkg_id = node_pkg_ids[node_id.get() as usize]; + + if data.integrity.tag.is_supported() { + self.record_extracted_integrity(pkg_id, &data.integrity); + } + let pkg_name = pkg_names[pkg_id as usize]; let pkg_name_hash = pkg_name_hashes[pkg_id as usize]; let pkg_res = &pkg_resolutions[pkg_id as usize]; @@ -230,6 +241,42 @@ impl<'a> Installer<'a> { } } + /// Main thread, before the package's tasks start. The counterpart of the + /// integrity write-back in `PackageInstaller::install_enqueued_packages_after_extraction`: + /// a lockfile written before tarball integrity was recorded has none for the + /// package, so the extraction computed it. A local tarball's cache entry is + /// named after it (`cached_local_tarball_folder_name`), so the tasks need it + /// in place, and the lockfile is re-saved with it. + fn record_extracted_integrity(&mut self, pkg_id: PackageID, integrity: &install::Integrity) { + let pkgs = self.lockfile().packages.slice(); + assert!((pkg_id as usize) < pkgs.len()); + // SAFETY: `items_raw` carries the column's root provenance, so this + // field access needs no `&mut Lockfile`. Only a package's own tasks + // read its `integrity` (to name the cache entry), and none of this + // package's tasks have started: every entry of it waited on the + // extraction being reported. Tasks of other packages running on the + // pool hold `&[Meta]` over the column but only touch other bytes. + // `pkg_id` is in bounds per the assert above. + let recorded = unsafe { + core::ptr::addr_of_mut!( + (*pkgs + .items_raw::<"meta", package::Meta>() + .add(pkg_id as usize)) + .integrity + ) + }; + // SAFETY: see above. + if unsafe { (*recorded).tag.is_supported() } { + return; + } + // SAFETY: see above. + unsafe { recorded.write(*integrity) }; + self.manager_mut() + .options + .enable + .set(Enable::FORCE_SAVE_LOCKFILE, true); + } + /// Called from main thread when a tarball download or extraction fails. /// Without this, the upfront pending-task slot for each waiting entry is /// never released and the install loop blocks forever on @@ -1132,9 +1179,13 @@ impl Task { patch_info.contents_hash(), ), ResolutionTag::LocalTarball => { - directories::cached_tarball_folder_name( - manager, - *pkg_res.local_tarball(), + // Recorded by the lockfile or by + // `on_package_extracted` before this task started. + debug_assert!( + pkg_metas[pkg_id as usize].integrity.tag.is_supported() + ); + directories::cached_local_tarball_folder_name( + &pkg_metas[pkg_id as usize].integrity, patch_info.contents_hash(), ) } diff --git a/src/install/patch_install.rs b/src/install/patch_install.rs index c5162af3567d..cffe4e4470e0 100644 --- a/src/install/patch_install.rs +++ b/src/install/patch_install.rs @@ -760,16 +760,18 @@ impl PatchTask { let pkg_name_slice = pkg_name .slice(&pkg_manager.lockfile.buffers.string_bytes) .to_vec(); - // `Resolution` is `Copy`; copy out so the lockfile borrow ends + // `Resolution` and `Integrity` are `Copy`; copy out so the lockfile borrow ends // before `compute_cache_dir_and_subpath` reborrows `pkg_manager` mutably. let resolution_clone: Resolution = pkg_manager.lockfile.packages.items_resolution()[pkg_id as usize]; + let integrity = pkg_manager.lockfile.packages.items_meta()[pkg_id as usize].integrity; let mut folder_path_buf = PathBuffer::uninit(); let stuff = package_manager::compute_cache_dir_and_subpath( pkg_manager, &pkg_name_slice, &resolution_clone, + &integrity, &mut folder_path_buf, Some(patch_hash), ); diff --git a/test/cli/install/bun-install-tarball-integrity.test.ts b/test/cli/install/bun-install-tarball-integrity.test.ts index 422352e6a667..35e208458ea2 100644 --- a/test/cli/install/bun-install-tarball-integrity.test.ts +++ b/test/cli/install/bun-install-tarball-integrity.test.ts @@ -493,6 +493,132 @@ describe.concurrent("tarball integrity", () => { }); }); +// A `file:` tarball's resolution is the path written in package.json (`pkg.tgz`), +// which names a different tarball in every project sharing the cache, and an +// install from a lockfile never reads the tarball itself. So the cache entry is +// keyed by the integrity the lockfile pins, not by that path: two projects (or two +// branches of one project) with different tarballs at the same path get separate +// entries, and a lockfile that does not pin an integrity cannot name an entry and +// has the tarball read instead. +describe.concurrent.each(["hoisted", "isolated"] as const)("local tarball cache entries (%s)", linker => { + async function tarball(exported: string) { + const tgz = await new Bun.Archive( + { + "package/package.json": JSON.stringify({ name: "pkg", version: "1.0.0" }), + "package/index.js": `module.exports = ${JSON.stringify(exported)};\n`, + }, + { compress: "gzip" }, + ).bytes(); + const digest = createHash("sha512").update(tgz).digest(); + return { + file: Buffer.from(tgz), + integrity: "sha512-" + digest.toString("base64"), + cacheEntry: `@T@sha512-${digest.subarray(0, 16).toString("hex")}@@@1`, + }; + } + + function project(tgz: Buffer, extraFiles: Record = {}) { + return { + "package.json": JSON.stringify({ name: "app", dependencies: { pkg: "file:pkg.tgz" } }), + "pkg.tgz": tgz, + "bunfig.toml": Bun.TOML.stringify({ install: { linker } }), + ...extraFiles, + }; + } + + // The projects under `root` share `root/cache`. + async function install(root: string, name: string, ...args: string[]) { + await using proc = spawn({ + cmd: [bunExe(), "install", ...args], + cwd: join(root, name), + env: { ...env, BUN_INSTALL_CACHE_DIR: join(root, "cache") }, + stdout: "ignore", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(stderr).not.toContain("error:"); + expect(exitCode).toBe(0); + } + + const installedExport = (root: string, name: string) => + file(join(root, name, "node_modules", "pkg", "index.js")).text(); + const lockfile = (root: string, name: string) => file(join(root, name, "bun.lock")).text(); + const reinstall = (root: string, name: string, ...args: string[]) => + rm(join(root, name, "node_modules"), { recursive: true }).then(() => install(root, name, ...args)); + const cacheEntries = async (root: string) => + (await readdirSorted(join(root, "cache"))).filter(entry => entry.startsWith("@T@")); + + it("keeps the tarballs of two projects that both depend on file:pkg.tgz apart", async () => { + const one = await tarball("one"); + const two = await tarball("two"); + using root = tempDir("local-tarball-two-projects", { one: project(one.file), two: project(two.file) }); + + await install(String(root), "one"); + await install(String(root), "two"); + expect(await lockfile(String(root), "one")).toContain(one.integrity); + + await reinstall(String(root), "one"); + expect(await installedExport(String(root), "one")).toBe('module.exports = "one";\n'); + expect(await installedExport(String(root), "two")).toBe('module.exports = "two";\n'); + expect(await cacheEntries(String(root))).toEqual([one.cacheEntry, two.cacheEntry].sort()); + }); + + it("installs the tarball the lockfile pins after another build of it was extracted from the same path", async () => { + const v1 = await tarball("v1"); + const v2 = await tarball("v2"); + using root = tempDir("local-tarball-two-builds", { app: project(v1.file) }); + const app = join(String(root), "app"); + + await install(String(root), "app"); + const v1Lockfile = await lockfile(String(root), "app"); + expect(v1Lockfile).toContain(v1.integrity); + + // Like checking out a branch that ships v2: the rebuilt tarball is resolved and extracted. + await writeFile(join(app, "pkg.tgz"), v2.file); + await rm(join(app, "bun.lock")); + await reinstall(String(root), "app"); + expect(await installedExport(String(root), "app")).toBe('module.exports = "v2";\n'); + expect(await lockfile(String(root), "app")).toContain(v2.integrity); + + // Back on the v1 branch, pkg.tgz and bun.lock are v1's again. + await writeFile(join(app, "pkg.tgz"), v1.file); + await writeFile(join(app, "bun.lock"), v1Lockfile); + await reinstall(String(root), "app"); + expect(await installedExport(String(root), "app")).toBe('module.exports = "v1";\n'); + expect(await cacheEntries(String(root))).toEqual([v1.cacheEntry, v2.cacheEntry].sort()); + }); + + it("reads the tarball when the lockfile does not record its integrity, then records it", async () => { + const other = await tarball("other"); + const mine = await tarball("mine"); + using root = tempDir("local-tarball-unpinned", { + other: project(other.file), + app: project(mine.file, { + // Written before bun recorded the integrity of tarball packages. + "bun.lock": JSON.stringify({ + lockfileVersion: 1, + configVersion: 1, + workspaces: { "": { name: "app", dependencies: { pkg: "file:pkg.tgz" } } }, + packages: { pkg: ["pkg@pkg.tgz", {}] }, + }), + }), + }); + + await install(String(root), "other"); + await install(String(root), "app"); + expect(await installedExport(String(root), "app")).toBe('module.exports = "mine";\n'); + expect(await lockfile(String(root), "app")).toContain(mine.integrity); + expect(await cacheEntries(String(root))).toEqual([mine.cacheEntry, other.cacheEntry].sort()); + + // The recorded integrity names the entry that extraction created: this + // install is served from it without reading pkg.tgz (which would no longer + // match the pin). + await writeFile(join(String(root), "app", "pkg.tgz"), other.file); + await reinstall(String(root), "app"); + expect(await installedExport(String(root), "app")).toBe('module.exports = "mine";\n'); + }); +}); + describe.concurrent.each(["hoisted", "isolated"] as const)("tarball integrity mismatch (%s)", linker => { // Regression test for #29646 — with the isolated linker, a SHA-512 mismatch // during the resolve-phase tarball extract left `task_queue` /