install: refresh URL/local tarballs under --force and explicit re-add/update#31868
install: refresh URL/local tarballs under --force and explicit re-add/update#31868robobun wants to merge 1 commit into
Conversation
WalkthroughImplement ChangesForce-refresh URL-keyed tarballs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
|
Updated 11:51 AM PT - Jun 10th, 2026
❌ @robobun, your commit b410712 has 8 failures in
🧪 To try this PR locally: bunx bun-pr 31868That installs a local version of the PR into your bun-31868 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
f472bcb to
e1561b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/PackageManager/runTasks.rs`:
- Around line 1163-1183: The integrity write is performed before the final
package mapping is finalized, so move the logic that persists
task.data_extract().integrity (and sets Enable::FORCE_SAVE_LOCKFILE) to after
process_extracted_tarball_package has been called and package_id has been
possibly retargeted; specifically, remove the block that writes to
manager.lockfile.packages.items_meta_mut()[package_id as usize].integrity and
instead, after process_extracted_tarball_package(...) confirms the final
package_id and new_integrity.tag.is_supported(), assign the integrity into
manager.lockfile.packages.items_meta_mut()[package_id as usize].integrity and
call manager.options.enable.set(Enable::FORCE_SAVE_LOCKFILE, true).
In `@test/regression/issue/31864.test.ts`:
- Around line 10-17: Remove the global 5-minute override by deleting the
setDefaultTimeout(1000 * 60 * 5) call in the test (do not replace it elsewhere);
rely on Bun’s default test timeout and keep the fixture small so the test fails
fast instead of stalling—ensure no other file-wide timeout overrides
(setDefaultTimeout) remain in this test file.
- Around line 61-138: Add a sibling test that covers the local-file path
variant: create two tarballs via buildTarball("VERSION_ONE") and
buildTarball("VERSION_TWO"), write v1.tgz to a local "./pkg.tgz" inside the
tempDir used for the test (instead of using Bun.serve/serveV2), run bun install
to populate node_modules, overwrite the same "./pkg.tgz" file with v2 bytes,
then run bun install --force (using the same spawnOpts and
BUN_INSTALL_CACHE_DIR) and assert the installed index file (installedIndex) now
contains "VERSION_TWO" and bun.lock contains v2.integrity (and not
v1.integrity); use the same helpers and assertions as the HTTP test so the
LocalTarball code path is exercised the same way.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1df1589d-511f-4993-8816-49201f1d7fdb
📒 Files selected for processing (7)
src/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/runTasks.rssrc/install/isolated_install.rssrc/install/resolution.rstest/regression/issue/31864.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/regression/issue/31864.test.ts:1-8— PerCLAUDE.mdandtest/CLAUDE.md,test/regression/issue/${issueNumber}.test.tsis reserved for true regressions (worked in a previous release, then broke) — and as the PR's own Cause section explains, URL/local tarball refresh under--forcenever worked because the cache key is the URL/path hash, so this is a missing capability rather than a regression. Consider folding this test into one of the existing URL-tarball suites the PR already references (test/cli/install/bun-install.test.tsorbun-install-tarball-integrity.test.ts).Extended reasoning...
What
The new test is added at
test/regression/issue/31864.test.ts, but the project's test-placement convention says that directory is reserved for true regressions, and this bug is not one.The convention
Both guideline files state the rule explicitly and identically:
CLAUDE.md:66— "Exception:test/regression/issue/${issueNumber}.test.tsis reserved for bugs that have a GitHub issue number and are true regressions (worked in a previous release, then broke). An issue number alone is not enough — if the behavior was never correct, it's not a regression and the test belongs in the existing file for that module."test/CLAUDE.md:153— "/test/regression/issue/${issueNumber}.test.tsis only for bugs that have a GitHub issue number and are true regressions (worked in a previous release, then broke). An issue number alone does not qualify — if it was never correct, put the test in the module's existing test file instead."
So the gating question is not "does this have an issue number?" but "did this ever work in a shipped release and then break?"
Why #31864 is not a regression — step by step
- The PR's Cause section says URL/local tarballs are cached under a folder named from
cached_tarball_folder_name(the URL/path hash), not the content hash. - The cache gate (
PackageInstall::package_missing_from_cacheand the isolated installer's inline equivalent) only checked whether that folder exists, with no--forcehandling. This is the design as it has always existed, not a recently introduced break. - Therefore, when the bytes behind the same URL change, the cache key is identical and
--forcestructurally could never have refreshed the tarball — the capability did not exist until this PR adds it (allow_force_refresh+is_tarball_cache_keyed_by_url). - The PR also notes a second structural blocker: even a naive re-download would have failed
IntegrityCheckFailedagainst the lockfile pin. There is no prior release in which this path worked.
There is no "worked in version X, broke in version Y" claim anywhere in the issue or PR. This is a missing feature/capability being added for the first time, which is exactly the case the guideline calls out as not belonging in
test/regression/issue/.Where it should go
The PR's own Verification section already names the natural homes:
test/cli/install/bun-install.test.ts— already covers URL/local tarball installs.test/cli/install/bun-install-tarball-integrity.test.ts— already covers the "tarball URL content changes" integrity path for the non---forcecase, which is the direct sibling of what this test exercises.
Either is a better fit than a new standalone regression-issue file.
Impact / fix
No functional impact — purely test organization. Fix: move the
describe.each(...)block (and its tar/server helpers, if not already available there) into one of the two existing suites above and deletetest/regression/issue/31864.test.ts. The header comment can keep thehttps://github.com/oven-sh/bun/issues/31864reference so the issue link is preserved. -
🟡
test/regression/issue/31864.test.ts:61-62— This test only exercises theRemoteTarballpath (viaBun.serve), but the fix also touches theLocalTarballpath — specifically the integrity-drop inenqueue_tarball_for_reading, which is only reached forfile:tarball dependencies. Consider adding a sibling case that writes the.tgzto disk, depends on it viafile:, swaps the bytes, and reinstalls with--force; right now deleting theenqueue_tarball_for_readinghunk would not break any test in this PR.Extended reasoning...
What's missing
The PR fixes
--forcerefresh for bothRemoteTarballandLocalTarball—is_tarball_cache_keyed_by_url()returnstruefor both, and the diff adds symmetric integrity-drop logic in two enqueue paths:generate_network_task_for_tarball(runTasks.rs) — used byRemoteTarballenqueue_tarball_for_reading(PackageManagerEnqueue.rs:258-267) — used byLocalTarball
The new regression test, however, only drives the
RemoteTarballpath: it spins up aBun.serveserver and depends onhttp://127.0.0.1:${port}/my-url-pkg.tgz. There is no test that depends on afile:./pkg.tgzon disk, swaps the bytes, and asserts--forcere-reads it.Why
enqueue_tarball_for_readingis uncoveredenqueue_tarball_for_readingis called exclusively from theResolutionTag::LocalTarballarms —PackageInstaller.rs:1562for the hoisted installer andisolated_install.rs:2542for the isolated installer. The HTTP-served tarball in this test resolves asRemoteTarballand goes throughgenerate_network_task_for_tarballinstead, so it never touches the new code inenqueue_tarball_for_reading.Step-by-step proof
- Revert only the
PackageManagerEnqueue.rshunk (restorelet integrity = this.lockfile.packages.items_meta()[package_id as usize].integrity;). - Run
test/regression/issue/31864.test.ts. - The test installs
http://127.0.0.1:PORT/my-url-pkg.tgz→Tag::RemoteTarball. - On
--force,package_missing_from_cachereturnstrue(covered hunk), and the download is enqueued viagenerate_network_task_for_tarball(covered hunk), which drops the integrity. enqueue_tarball_for_readingis never called — its reverted body is dead for this test.- Test passes despite the partial revert.
So one of the four load-bearing clauses in this PR has no covering test.
Project guidelines
CLAUDE.md's "Tests reviewers reject" section (distilled from merged-PR review feedback) calls this out explicitly:
Confirm deleting each load-bearing clause of your fix breaks at least one test.
Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix.Both apply here:
LocalTarballis the sibling entry point, and theenqueue_tarball_for_readingclause is deletable without test failure.Suggested fix
Wrap the test body in a second
describe.each(['remote', 'local'])(or add a siblingit) that, for the local variant:- Writes
v1.tgzinto the temp dir and setsdependencies: { "my-url-pkg": "file:./pkg.tgz" }. - Asserts
VERSION_ONEafter the first install. - Overwrites
pkg.tgzwithv2.tgzbytes (same path) and runsbun install --force. - Asserts
VERSION_TWOis installed andbun.lockcarriesv2.integrity.
This reuses the existing
buildTarballhelper and the hoisted/isolated matrix already in place.Severity
This is a test-coverage gap, not a production correctness bug — the
LocalTarballchange is symmetric with theRemoteTarballchange and looks correct by inspection. Filing as a nit, but it's the kind of gap the repo's own review checklist flags.
e1561b7 to
a798a5a
Compare
a798a5a to
6de9881
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-install-tarball-integrity.test.ts`:
- Around line 713-750: Extract the duplicated tar helper functions octal,
tarHeader, and pad512 to file-level (top of the test file, after imports and
before setDefaultTimeout) so both test suites reuse the same implementations;
remove the duplicate definitions in both places and keep each test's
buildTarball variants local (the marker-based one in the
"bun-install-tarball-integrity.test.ts" suite and the generic Buffer-based one
in the other suite), and update any calls to reference the single top-level
helpers (no API changes to tarHeader/pad512/octal).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5662ae7c-46e9-46da-9a63-25ea50a3cea9
📒 Files selected for processing (7)
src/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/runTasks.rssrc/install/isolated_install.rssrc/install/resolution.rstest/cli/install/bun-install-tarball-integrity.test.ts
6de9881 to
e95082e
Compare
fdc1be7 to
b9ef741
Compare
|
Status after the re-add/update conformance push (b410712): build 61771 is green on every lane except one. The only failed job is One pre-existing test in the file this PR touches, The earlier repo-wide |
b9ef741 to
a007c47
Compare
|
@robobun if you check, the behavior in bun 1.3 is actually that it checks with normal bun install even without force...please conform to this |
…/update URL and local tarballs are cached under a folder named from the URL/path hash (cached_tarball_folder_name), not from the tarball content. When the bytes behind the same URL/path changed, `bun install --force` copied the stale extraction into node_modules and never re-downloaded, so the code never updated. A forced re-download would also have tripped the lockfile-pinned integrity, which is why the only known workaround was clearing the cache and the lockfile by hand. Explicitly re-adding the URL (`bun i <url>`) also no longer refreshed it: Bun 1.3 did, as a side effect of the duplicate-package.json-entry bug fixed for #30499, and users rely on that re-add-to-refresh behavior. For these content-mutable tarballs only (npm/git/github are content-addressed by version/commit): - treat the package as missing from the cache under --force, and also when the dependency was explicitly named on the command line (matched against update_requests), so a download/read task is enqueued (package_missing_from_cache, and the isolated installer's inline equivalent). Gated on the initial install-phase pass, on the preinstall state not already being Done, and on no tarball task for the same URL having been enqueued this run (bun update re-downloads during the resolve phase; re-enqueueing would push a callback into the already-drained task and the package would silently never install). - drop the lockfile-pinned integrity when building the ExtractTarball so the fresh bytes are accepted and the hash is recomputed from them. - persist the recomputed hash over the stale one after extraction, for both the hoisted and isolated installers. Tests in bun-install-tarball-integrity.test.ts cover both linkers for --force (url + local + first-install-no-lockfile), re-add via `bun i <url>`, and `bun update <name>`; they fail on the unfixed build. Fixes #31864
a007c47 to
b410712
Compare
|
@VillainsRule You are right, thanks for pushing on this. I tested Bun v1.3.14 against a local server that swaps the tarball bytes behind a fixed URL:
The 1.4 canary lost the re-add refresh as a side effect of fixing #30499 (each This PR now restores both behaviors explicitly: |
What
bun install <url> --forcedid not refresh a URL tarball, and neither did explicitly re-adding the same URL (bun i <url>) even though Bun 1.3 refreshed it in that case. Reinstalling after the tarball at the URL changed kept serving the old code, and the only known workaround was clearing the cache and the lockfile by hand (bun pm cache rm, then deletenode_modulesandbun.lock).Fixes #31864.
Repro
Serve a tarball at a fixed URL, install it, change the bytes served at that same URL, then reinstall with
--force(or re-add the same URL).Cause
URL and local tarballs are cached under a folder named from the URL/path hash (
cached_tarball_folder_name), not from the tarball content. The cache gate that decides whether to download (PackageInstall::package_missing_from_cache, and the isolated installer's inline equivalent) only checks whether that folder exists, with no--forcehandling. When the bytes behind the same URL/path change, the folder name is identical, so--forcereinstalls from the stale extraction and never re-downloads.A naive re-download would then hit a second problem:
ExtractTarball::runverifies the fresh bytes against the integrity pinned in the lockfile and fails withIntegrityCheckFailed, which is the error the reporter saw after clearing only the cache.On Bun 1.3,
bun i <url>(re-adding the URL, no--force) did refresh the tarball, but only as a side effect of the duplicate-package.json-entry bug fixed for #30499: each re-add appended a second, URL-keyed dependency whose name hash missed the lockfile dedup and forced a fresh download. Fixing #30499 removed the accidental refresh along with the duplicate entries. Users rely on re-add-to-refresh (see the issue thread), so this PR restores that behavior through an explicit mechanism instead.npm/git/github resolutions are content-addressed by version/commit, so they are unaffected and keep hitting the cache.
Fix
For content-mutable tarballs only (
RemoteTarball/LocalTarball, identified byTag::is_tarball_cache_keyed_by_url):--force, and also when the dependency was explicitly named on the command line (bun i <url>,bun update <name>; matched viaUpdateRequest::matchesagainstupdate_requests), so a download/read task is enqueued. Applies to both the hoisted gate (package_missing_from_cache) and the isolated installer's inline gate, on the initial install-phase pass only.Done, or a tarball task for the same URL is already intask_queue(the resolve phase re-downloads it whenbun updateinvalidates the resolution). Without these guards the install phase re-enqueues into the already-drained task and the package silently never installs.ExtractTarball(generate_network_task_for_tarballfor remote,enqueue_tarball_for_readingfor local), so the fresh bytes are accepted and the hash is recomputed from them.Plain
bun install(no package named, no--force) is unchanged and still installs from the cache, matching Bun 1.3.Verification
The
tarball --force refreshtests intest/cli/install/bun-install-tarball-integrity.test.tsserve a tarball over a localBun.serve(port: 0, isolatedBUN_INSTALL_CACHE_DIR), swap the bytes at the same URL/path, and assert the installed file is the new content, the tarball was actually re-requested, and the lockfile integrity was updated. Cases covered for both the hoisted and isolated linkers:bun install --forcewith a changed remote tarballbun install --forcewith a changed local tarball at the same path--forceas the very first install with no lockfile (exercises the re-enqueue guard)bun i <url>re-add with no--force(the Bun 1.3 behavior from the issue thread), including asserting package.json does not grow a duplicate entry (bun install {tarball url}will add it multiple times to package.json #30499)bun update <name>of a URL tarball dep refreshing node_modulesAll fail on the unfixed build. The rest of the tarball-integrity suite (including
should fail integrity check when tarball URL content changesfor the plain-install path) still passes, confirming normal integrity verification is unchanged. Verified against Bun v1.3.14 that plainbun installstaying on the cached extraction matches 1.3, and that re-add/update refreshing matches 1.3.rust:check-allpasses on all targets.