Conversation
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-<digest prefix>@@@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.
|
Warning Review limit reached
Next review available in: 20 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
Comment |
|
Status: reproduced and fixed.
|
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt found no issues. Because it changes the shared install-cache key model for file: tarballs across 14 files, threads ExtractData through both installers, and adds an unsafe raw-pointer write into the lockfile's meta column while pool tasks hold &[Meta] views (record_extracted_integrity), a human look would still be worthwhile.
What was reviewed:
- Cache-entry naming:
cached_local_tarball_folder_name_printand every LocalTarball call site now key on integrity; empty name is treated as a miss inpackage_missing_from_cache, the isolated probe, anddetermine_preinstall_state(which already handled it). - Integrity write-back: hoisted path already did this; the new isolated
record_extracted_integritymirrors it and setsFORCE_SAVE_LOCKFILE, which--frozen-lockfilestill supersedes at save time (install_with_manager.rs:2223). Integrity::Displayrefactor viaTag::name()— output unchanged (sha512-<b64>==).compute_cache_dir_and_subpathnow hard-exits on missing integrity for LocalTarball instead of returning an empty subpath intobun patch.
Extended reasoning...
Overview
The PR fixes a real correctness bug: local file: tarballs were cached under a hash of their relative path (e.g. pkg.tgz), so any two projects — or two branches of one project — that used the same relative path but different tarball bytes shared and overwrote one cache entry. The fix keys the entry on the tarball's integrity (@T@sha512-<first 16 digest bytes hex>@@@1), the same content-addressed model already used for git commits. It touches 14 files: the folder-name printers, both installers (hoisted and isolated), the buffered and streaming extractors, bun patch / bun patch --commit, the RunTasksCallbacks trait signature, and adds six new integration tests across both linkers.
Security risks
Low. The change makes the cache more content-addressed — an entry can only be reused for the exact bytes the lockfile pinned, which is strictly tighter than before. Sixteen bytes (128 bits) of SHA-512 is well beyond collision range for a local-machine cache. No new untrusted-input parsing; the integrity value is either read from the project's own lockfile or computed from bytes on disk.
Level of scrutiny
High. This is core package-manager cache logic that every bun install with a file: tarball hits, and it involves:
- A design choice (integrity-keyed for local tarballs, URL-keyed for remote) a maintainer should ratify.
- New
unsafeinInstaller::record_extracted_integritythat writes oneMetarow viaitems_rawwhile other packages' pool tasks concurrently hold&[Meta]over the same column. The SAFETY comment argues those tasks only touch other rows' bytes and this package's tasks haven't started; that argument reads correctly to me but deserves a second pair of eyes on the Stacked-Borrows and thread-affinity claims. - A new
Global::exit(1)insidecompute_cache_dir_and_subpath(reachable frombun patch) — the PR argues the normal flow always installs first so integrity is present, but a human should confirm no other caller can hit it unexpectedly.
Other factors
The PR is exceptionally well-documented and thoroughly tested (six new tests covering both linkers, cross-project collisions, branch switching, and legacy lockfiles without integrity), and the author ran the surrounding suites. --frozen-lockfile behavior was checked: record_extracted_integrity sets FORCE_SAVE_LOCKFILE, but save_lockfile_only (install_with_manager.rs:2223) short-circuits on frozen_lockfile() regardless, matching the description. No prior human reviews on the timeline. Given the scope and the concurrency argument, deferring rather than approving.
|
On the three points raised above, for whoever picks this up: Why local tarballs are keyed differently from URL tarballs. The cache is keyed by resolution identity everywhere else (
|
Problem
bun.lockpresent, afile:pkg.tgzdependency is installed from whatever the shared install cache holds under@T@<hash of "pkg.tgz">@@@1. Two projects that both depend onfile:pkg.tgzbut ship different tarballs overwrite each other's entry, and the next lockfile install of one project gets the other project's tarball; the same happens to one project switching between branches whosepkg.tgzdiffers.bun installreports1 package installed, and the integrity inbun.lockis never compared against anything.cached_tarball_folder_name_print(src/install/PackageManager/PackageManagerDirectories.rs) hashes the resolution string, and a local tarball's resolution is the path as written in package.json (relative to the project, or to the workspace), which is not an identity across projects. Only extraction verifies the integrity, and a lockfile install only extracts when the entry is missing (PackageInstall::package_missing_from_cache, and the isolated installer's probe inisolated_install.rs).Fix
@T@sha512-<first 16 digest bytes as hex>@@@1(cached_local_tarball_folder_name), so the name equalssha512sum pkg.tgz | cut -c1-32. URL tarballs keep their URL-keyed names.ExtractTarball::lockfile_integrity, passed intomove_to_cache_directorylike the GitHubresolvedname), which is also the value written to the lockfile. So an entry is only reused for the bytes the lockfile pins, different tarballs at the same path coexist, and identical tarballs in different projects share one entry. This is the same model as git checkouts, which are cached under their commit rather than under the URL they were requested by.package_missing_from_cache, the isolated probe;determine_preinstall_statealready treated an empty name this way). The hoisted installer already recorded a computed integrity before re-running the waiting installs; the isolated installer now receives the extraction result (on_extract_store_installer) and does the same (Installer::record_extracted_integrity), so the entry can be named, the lockfile is re-saved with the integrity, and the next install is a cache hit. Under--frozen-lockfilesuch a lockfile keeps re-extracting the tarball and is left untouched.compute_cache_dir_and_subpath(bun patch,bun patch --commit) takes the package's integrity and reports a missing one instead of diffing or copying against the cache root. In the normal flow it is present, becausebun patchinstalls first and that install records and saves it.Integrity::Tag::name()is shared between the folder name andDisplay; theDisplayoutput is unchanged.test/cli/install/bun-install-tarball-integrity.test.ts,local tarball cache entriesfor both linkers: two projects sharing a cache keep their tarballs apart, a project reinstalling from a lockfile after another build of the tarball was extracted from the same path gets the pinned build, and a lockfile without an integrity installs from the tarball, records the integrity, and is then served from the entry it recorded. All six fail on the unfixed build at the installed-contents assertion.bun-patch.test.ts(includes thefile:tarball patch flow, which exercises the patched..._patch_hash=entries),bun-install-patch.test.ts,bun-lock.test.ts, the tarball tests ofbun-install.test.ts(except the one fetching from the public internet, which fails identically without this change),bun-add.test.tsandisolated-install.test.ts, and thefile:tarball migration tests inmigrate.test.tsandpnpm-lock-v9.test.ts. Manually:--frozen-lockfilewith a lockfile lacking the integrity (correct contents, lockfile untouched),bun patchplus--commitstarting from such a lockfile, workspace-relativefile:tarballs with both linkers, two aliases of one tarball, andbun add ./pkg.tgz.cargo clippy -p bun_installis clean.@T@<path hash>entries are simply no longer looked up for local tarballs; each local tarball is extracted once more on its next install.Background
~/.bun/install/cache(orBUN_INSTALL_CACHE_DIR) is shared by every project on the machine. Each package is extracted once into a folder there (name@version@@@1for npm,@G@<commit>for git,@T@...for tarballs) and installs copy, hardlink or clone out of it; a cache hit is a directory probe, and the tarball bytes are verified only when the folder is created.@@@1is the cache layout version.file:and URL tarballs bun hashes the tarball on first extraction and storessha512-...as the third element of the package'sbun.lockentry (["pkg@pkg.tgz", {}, "sha512-..."]); later extractions are verified against it. Lockfiles written before this was recorded, and some migrated lockfiles, have no integrity for these packages.PackageInstaller::install_enqueued_packages_after_extractionfor the hoisted linker,Installer::on_package_extractedfor the isolated one). The extraction result (ExtractData) carries the integrity that was verified or computed.&Lockfileviews, so the lockfile is only ever written through narrowed raw pointers from the main thread.record_extracted_integritywrites one row through the column's raw pointer (items_raw) before any task of that package starts.Reproduction (no network)
Before: the last line prints
two(the cache holds one@T@85f77fa09e18a07f@@@1, last written by project two). After:one, and the cache holds one@T@sha512-...@@@1entry per tarball.