install: do not download tarballs that the installers never place - #43122
Conversation
A fresh resolve starts the tarball download of every registry package it resolves. The installers place nothing below a dependency that they filter: a bundled dependency (the tarball of the parent ships it), a package for another platform, or a group that --production or --omit turns off. Those downloads were never used. `bun install` of npm@10.9.2 downloaded 174 tarballs to place one package. The first resolve of a package through such a dependency no longer creates the tarball task. No flag on the dependencies below says that they are not placed, so lockfile scratch state marks the dependencies of each package that is first reached this way. When a dependency that the installers do place resolves to the same package, the install phase downloads it like any other cache miss. Behavior::is_placed is the test that Tree and the tree printer already made, now in one place. The runtime auto-install keeps every download. It has no install phase, and with --install=force it loads a bundled dependency from the cache.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughChangesInstaller resolution now distinguishes placed and unplaced dependencies. Runtime auto-install enables resolve-triggered downloads while skipping the install phase. Registry tests cover bundled, platform-specific, omitted, patched, cached, and runtime scenarios. Dependency placement and resolution
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable regression risk remains from the reviewed changes. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation Issue Resolution Change bundled-dependency resolution so Bun does not enqueue or fetch the bundled dependency manifest from the configured registry. Add an automated regression test that uses a registry where the bundled package is absent and verifies that installation does not request that manifest or fail because of it. Full details: Out of Scope Changes checkExplanation The linked issue
Comment |
|
Status: review findings handled in 41198b3 and c60dbae, waiting for CI. PR: #43122 How I reproduced it, on main (b64b630) and on 1.4.3-canary.1:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/install/PackageManager/PackageManagerEnqueue.rs— Patched packages below a bundled, disabled or omitted dependency are still downloaded by a fresh resolve, depending on timing, so the description's claim that they no longer are does not hold. Theunplacedskip only gates thePreinstallState::Extractarm; theCalcPatchHasharm at PackageManagerEnqueue.rs:2491 still creates the task, and its after-state in patch_install.rs:301 re-runsdetermine_preinstall_stateand enqueues the tarball without anyunplacedcheck. Fix: make every resolve-phase tarball site honour the same placed/unplaced decision, e.g. carry it inEnqueueAfterStateor recompute it fromdep_idwith a shared helper, and skip the download at patch_install.rs:301 too.Extended reasoning...
Perf-only, no wrong install results; filed because it is the parallel arm of the change and the PR text says the opposite. In a fresh install, patched dependencies from package.json have
patchfile_hash_is_nulltrue (install_with_manager.rs:434). create_new_lockfile_and_enqueue pre-enqueues one calc-hash task per patch on the thread pool (install_with_manager.rs:1962-1966) and immediately calls enqueue_dependency_list (1968), which resolves synchronously any root dependency whose manifest is in the disk cache. resolve_pending_tasks then calls wait_for_calcing_patch_hashes (1993), whose is_done loop runs run_tasks (install_with_manager.rs:1045-1052), so manifests arriving meanwhile also resolve packages before the hashes land. For such a package determine_preinstall_state sees the null hash and returns CalcPatchHash (PackageManagerLifecycle.rs:107-110). The CalcPatchHash arm (PackageManagerEnqueue.rs:2491-2505) does not look atunplacedand builds the task with EnqueueAfterState{pkg_id, dependency_id, url}. When the hash task completes, patch_install.rs:287-354 resets the state,…Verification: nit — perf-only and the base already downloads these tarballs, but the PR's description claim ("A package with a patch that only exists inside a bundle is no longer downloaded either") does not hold, and REVIEW.md asks that parallel switch arms be covered in the same PR. Trigger: a fresh resolve where a
patchedDependenciesentry names a package that is first reached below a… | nit — the new…
The skip only covered the download arm. A package with a patch that resolves before the hash of its patch is known takes the CalcPatchHash arm, and the task that arm creates downloads the tarball when the hash lands. The ApplyPatch arm patched a cached copy that no install used. One guard arm now covers every state. The install phase fetches and patches the package when a placed dependency resolves to it. The tests install a third way, a fresh resolve that finds its manifests in the cache. That resolve runs before any patch hash is known, so the patched projects reach the CalcPatchHash arm every time. The offline install docs say which packages an install caches.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:09 AM PT - Sep 17th, 2026
✅ @robobun, your commit c60dbaec656961418f82ef6850f999ac191d94c8 passed in 🧪 To try this PR locally: bunx bun-pr 43122That installs a local version of the PR into your bun-43122 --bun |
|
Both review findings are handled in 41198b3. c60dbae shortens the code comments that were flagged. Patched packages. Confirmed. With the manifests in the cache, the resolve runs before the hash of the patch is known and takes the The tests now install each project a third way: a fresh resolve that finds its manifests in the cache. That phase reaches Cache contents. Answered in the review thread: documented in |
|
The two pre-merge warnings came from a false issue link. The PR notes named #27418 right after the words "does not fix", and GitHub read that as a closing keyword. A merge would have closed the issue. I reworded the notes. The PR has no closing reference now, and #27418 stays open. That issue is about manifest requests for bundled dependencies, which this PR does not change. |
…#44414) ### Problem - `bun update <dep> <peer>` exits 1 with `error: util@^1.0.0 failed to resolve` when `<peer>` is an auto-installed peer that nothing else depends on. Each name alone works. - `enqueue_peer_rows` (`src/install/update_transitive.rs:452`) calls `populate_manifest_cache` while dependencies wait for manifests. Its wait runs `run_tasks` in manifests-only mode, which skips their waiter lists (`runTasks.rs:662`, `:1070`). - The opposite mismatch aborts `bun install` over a yarn.lock with an unusable scope registry: `panic: infallible: task queued`. ### Fix - `run_tasks` takes the waiter list of a finished manifest request on every pass. A prefetch request has none. - `print_log` returns `InstallFailed` when the log it resets holds an error. `populate_manifest_cache` starts the progress bar before its wait (`panic: downloads_node active`). - Verified: `bun-update-transitive.test.ts` (18 new cases), `yarn-lock-migration.test.ts` (1). All 19 fail without the fix. Also the update, audit and migration suites. ### Background - A waiter is a dependency that waits for a manifest request. `task_queue` maps request ids to waiters. Only `run_tasks` retires requests. - `populate_manifest_cache` fetches manifests ahead of use (`bun outdated`, bare `bun update`, five more entrances). Its requests have no waiters. - Considered a reorder of the named update: it covers 1 of 7 entrances and keeps both skips and the abort. ### Downsides - A prefetch pass does one `task_queue` lookup per stored manifest: 15 more instructions, no insert, no allocation. - Queued dependencies now resolve inside the prefetch wait. Such a run can print no `Resolving dependencies` line. - After a failed download, `bun update --latest <name>` exits 1 and saves nothing. It exited 0 and saved bun.lock. <details><summary>Notes</summary> **Repro.** A loopback registry has `util@1.0.0` with `peerDependencies: { core: "^1.0.0" }` and `core@1.0.0`. The project depends on `util ^1.0.0`. `bun install`, then `bun update util core`: exit 1, `error: util@^1.0.0 failed to resolve`, nothing saved. The result is the same after `util@1.1.0` and `core@1.1.0` exist, and in either name order. The failure is in every release since 1.4.0 (#38333 added `enqueue_peer_rows`). **Why 1.4.2 passed one shape.** When `core` also depends on `util`, 1.4.2 moved both packages. The re-resolved peer made a new `core@1.1.0`, its dependency started a tarball download of a new `util`, and the extract arm of `run_tasks` ran the waiter list of the `util` manifest. #43122 removed that download. No revert is needed: the waiter was already dropped, and shapes without that download fail on 1.4.2 too. **Mechanism.** `bun update` does not read the manifest disk cache, so each queued dependency starts a manifest request and parks a `TaskCallbackContext::Dependency` in `task_queue` (`PackageManagerEnqueue.rs:1331`). `populate_manifest_cache` flushes and schedules every queued request and waits until the manager has no pending task. In that wait the old code stored the manifest and took `continue` before the `task_queue` take. `network_dedupe_map` keeps the request id, so nothing asks again. The dependency stays unresolved. **Forms that failed and now pass (each is a test).** Both name orders. A glob next to the peer and `*`. `--latest`. A transitive dependency named next to the peer. The peer named from a workspace member. The peer itself added to package.json. A dependency, an optional dependency, an override or a `file:` folder added to package.json before `bun update <peer>`. `--prefer-offline` with only the peer's manifest cached. A patched dependency named next to the peer. Two forms were silent on main. With an optional dependency added to package.json, `bun update <peer>` exits 0 and saves a bun.lock without it. `bun update <optional-dep> <peer>` exits 0, writes `"util": ""` to package.json and saves a bun.lock with no packages. **`print_log`.** It has two callers, `enqueue_peer_rows` and `plan_edges`. Both print and reset the log in the middle of an install, and `install_with_manager` reads `log.has_errors()` later to decide whether to save. With the waiters delivered inside the prefetch wait, an error of such a dependency reaches the first caller: a tarball 404 or an integrity failure for a dependency that the request does not name. The second caller has the same defect on main: `bun update --latest <name>` runs `plan_edges` after the first resolve wave, so a tarball 404 in that wave prints `error: GET ... - 404`, exits 0 and saves bun.lock and package.json, where `bun update <name>` exits 1 and saves nothing. Two tests pin both callers. Each fails without the guard. **Migration abort.** `Packages::All` returns at the first manifest request it cannot start, after it scheduled earlier ones. `fetch_necessary_package_metadata_after_yarn_or_pnpm_migration` drops that error. The install wait then completed a request with no `task_queue` entry and hit `.expect("infallible: task queued")`. Release build of the merge base: exit 134. This branch: exit 0, bun.lock written. **Progress bar.** `start_manifest_task` starts the bar only when it creates a request. When every name is cached or already requested by a queued dependency, the wait starts with no bar, and the first pass of `run_tasks` names the bar if a manifest download has completed by then. Real timing did not hit the window: 0 panics in 200 pty runs on each build. With the main thread paused for 400 ms before that first pass (gdb), a release build of main panics with `downloads_node active` in 5 of 5 runs, for the peer added to package.json and for `--prefer-offline` with only the peer's manifest cached. This branch: 0 of 25 runs. No test covers it, because it needs a pty and that pause. **Measurements (release builds of the merge base 4b02e10 and of this branch, unless noted).** - `run_tasks_erased`, pass of a normal install: parsed-manifest arm 35 -> 33 instructions from the manifest store to the `process_dependency_list_for_ctx` call, 304 arm 34 -> 33. The compare-and-branch on `manifests_only` is gone at both sites. Whole function: 6517 -> 6448 instructions. - `run_tasks_erased`: 33,102 -> 32,764 bytes. `populate_manifest_cache`: 3,724 -> 3,756 bytes. Release `.text`: 0 bytes delta (80,679,983 both). Stripped binary: 80,848,456 bytes both. These sizes are from the first push. The later `print_log` change adds one compare and one early return. - Prefetch pass with nothing queued (`bun outdated`, 30 dependencies, empty manifest cache): 30 `task_queue` lookups (0 on the merge base), 0 inserts, 0 waiter deliveries. After the manifest store the pass runs 20 instructions for each manifest, 12 of them in `HashMap::get_index`. The merge base runs 5. - Waiters (debug build, gdb): `bun update util core` 1 of 1 delivered once. With `core` depending on `util`: 2 of 2. `bun update '*'` over 40 direct dependencies and 1 peer: 40 of 40 delivered once, 41 manifest requests with no duplicate, 41 packages moved, `bun install --frozen-lockfile` passes. The take in the extract arm found 0 non-empty lists. - Requests of `bun update util core` with both packages newer: 1 `GET /util`, 1 `GET /core`, then the 2 tarballs. `bun update core nope`: 0 requests, same reject text. - stdout, stderr and requests of `bun update util` and of `bun update core`: 0 changed lines. - Event-loop waits entered (`AnyEventLoop::tick_raw`, debug builds, two runs each): `bun update util` 3 and 5 on both builds. `bun update core` 3 and 5 to 6 on the merge base, 3 and 6 on this branch. **Left for later.** - A waiter that is parked on a failed manifest request stays parked in both modes. - The prefetch pass still runs with `install_peer = true`. Only non-peer dependencies can be parked there today. - `MANIFESTS_ONLY` now only keeps the parsed-manifest arm from naming the progress bar. - #40284 rewrites the same two hunks and keeps both early exits. #43981 adds another `populate_manifest_cache` call inside an install, which this change makes safe in any call order. </details> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
2 participants
Problem
bun installwith no lockfile,bun add) downloads the tarball of every registry package it resolves. The installers place nothing below a dependency that they filter. That is a bundled dependency, a package for another platform, or a group that--productionor--omitturns off.bun installofnpm@10.9.2downloads 196 tarballs to place 1 package.get_or_put_resolved_package_with_find_result(src/install/PackageManager/PackageManagerEnqueue.rs:2438). It creates the tarball task for the first dependency that resolves a package, whatever that dependency is.Fix
Lockfile.scratchrecords the dependencies below.Behavior::is_placedis the testTree.rsalready made, now shared.test/cli/install/bun-install-registry.test.ts(23 new tests, 20 fail without the fix). Also 16 more install suites, see Notes.bun updateand--dry-runare left out, see Notes.Background
bundleDependenciesnames dependencies that a package ships inside its own tarball. bun still resolves them and records them in the lockfile.is_filtered_dependency_or_workspace,src/install/lockfile/Tree.rs).Notes
Issue #27418 stays open. This PR does not change it: bun still fetches the manifest of a bundled dependency, so a bundled dependency that the registry does not have still fails the install. Only tarball downloads change.
Numbers. Release builds of main (b64b630) and of this branch, cold cache, no lockfile, real registry, 10 interleaved runs each (20 for bun-lambda).
node_modulesis identical between the two builds in every run. bun-lambda's lockfile already varies from run to run on main (3 distinct lockfiles in 20 runs for each build).npm@10.9.2packages/bun-lambda--productionOn this link the wall time differences are inside the run-to-run noise (standard deviation 50 to 900 ms), except
--production. The rows with shared packages are the cost case: a package that is first resolved below a filtered dependency loses its resolve-phase download and the install phase fetches it.Not covered. Each of these still downloads tarballs that no install uses, on main and on this branch.
--filterleaves out. The workspace selection is not known during the resolve.bun update, when it resolves a new version below a package that came from the lockfile. The mark is only set for packages that this resolve creates.--dry-run. It has no install phase at all, so the right fix is to turn the prefetch off for it. That is a separate change.Why the runtime auto-install is exempt. With
--install=forcethe runtime loads a bundled dependency from the cache, not from the parent'snode_modules. The resolver has a branch that downloads a resolved package that is missing from the cache (src/resolver/resolver.rs, thePreinstallState::Extractarm), but it never runs: it compares the error withFileNotFound, andFrom<install::Error> for bun_core::Error(src/install/error.rs) maps theENOENTtoUnexpected. So a resolve is the only time the runtime downloads a package. Without the exemptionbun --install=force -p 'require("outer")'fails witherror: Unexpected while resolving package 'bd'.Equivalence of the feature sets. The installers test a dependency of a local package with
local_package_featuresand a dependency of a remote package withremote_package_features. The resolve does not know the owner, and tests withlocal_package_featuresonly. The two differ indev_dependenciesandworkspaces, and a remote package has neither kind of dependency (Features::NPM). The optional and peer flags are always set together.A package with a patch that the installers do not place gets no download and no patch task, whichever arm of the resolve it takes (
Extract,CalcPatchHash,ApplyPatch). Before, it was downloaded and patched in the cache, and never installed. If a placed dependency resolves to the same package, the install phase downloads it and applies the patch. The tests install each project a third way, a fresh resolve that finds its manifests in the cache. That resolve runs before the hash of any patch is known, so it reaches theCalcPatchHasharm every time.Cache contents. A fresh resolve no longer leaves the tarballs of filtered packages in the cache. An
--offlineinstall with the same platform and flags as the run that warmed the cache is not affected: the install phase of that run fetched everything it placed. A cache warmed with--productionor--omitno longer holds the groups that were left out. With--dry-runorBUN_CONFIG_SKIP_INSTALL_PACKAGES=1there is no install phase, so a package that is first resolved below a filtered dependency is not fetched at all. An install from a lockfile never cached more than it placed.docs/pm/cli/install.mdxnow states that contract.Other suites run with the debug build:
bun-prune,bun-dedupe,bun-audit,bun-lock,bun-update-transitive,bun-install-cpu-os,architecture-match,bun-lockb,bun-pm-licenses,bun-install-offline,bun-install-security-provider,bun-workspaces,bun-add,bun-install-patch,bun-patch,bun-install(13 failures, all of them need bitbucket, gitlab or another external host, and all of them also fail with the released build).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts