install: reuse lockfile-resolved git/github/tarball packages when re-resolving - #37142
install: reuse lockfile-resolved git/github/tarball packages when re-resolving#37142robobun wants to merge 5 commits into
Conversation
…resolving When a lockfile-dirtying change re-enqueues a git:, github:, or tarball dependency (for example any dependency edit in a workspace member, which re-parses the member's whole dependency list), the enqueue looked the package up under the dependency's real package name. That name comes from the extracted package's package.json and is still empty on a freshly parsed dependency, so the in-memory lookup hashed an empty string, always missed the package index (which is keyed by real package names), and scheduled a redundant download/clone of a package the loaded lockfile had already resolved. The extract then appended a transient duplicate package that the lockfile clean pruned again. Fall back to the dependency's alias when the real name is not known yet. The resolution equality check inside get_package_id already matches loaded packages correctly; only the name-hash index key was wrong. Besides the redundant network traffic, the stale resolve-phase task this left behind is what armed the isolated-linker patchedDependencies deadlock addressed by #37136.
|
Warning Review limit reached
Next review available in: 33 minutes 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 (1)
WalkthroughChangesThe install path now handles empty real package names, targeted updates, and isolated-store Git checkout scheduling. Tests cover lockfile reuse, overrides, cold-cache Git installs, and explicit remote dependency updates. ChangesDependency resolution and update handling
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on released bun (workspace + github dep; any member package.json edit triggers a redundant codeload download per install, verified with a request-counting server and BUN_DEBUG_PackageManager=1 showing the empty-name lookup). The fix is in three parts (see the PR body): the alias fallback in enqueue_dependency_with_main_and_success_fn, a direct checkout enqueue in the isolated store's clone-completion path (review caught that the old path relied on the lookup missing, which would have deadlocked cold-cache isolated git installs), and a bun-update gate so update targets still re-fetch (verified A/B against released bun). Four tests in test/cli/install/bun-lock.test.ts cover the re-download fix (fail on unfixed bun), the cold-cache isolated reinstall, the override-by-declared-name change, and update parity. Local suites green: bun-lock 21, isolated-install 62, bun-workspaces 63, overrides 7, patch 49, bun-add 54, bun-update 6, bun-lockb 6. Waiting on CI. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-lock.test.ts`:
- Around line 1122-1125: Extend the test assertions after the existing gh-dep
verification to read node_modules/td-dep/index.js and assert its expected module
contents. Keep the assertion tied to the remote tarball package identified by
the td-dep lockfile entry, verifying extraction and linking rather than only
download and lockfile state.
- Line 1131: Update the “re-resolving reuses a git package from the lockfile
instead of re-fetching” test to detect whether the external Git executable is
available and skip the test when it is not, providing a clear skip reason. Keep
the test’s existing behavior unchanged when Git is available.
🪄 Autofix
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: 32f6f44f-615a-4e14-96c4-b29e812ac141
📒 Files selected for processing (2)
src/install/PackageManager/PackageManagerEnqueue.rstest/cli/install/bun-lock.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…pdate targets Review of the alias-fallback fix surfaced two paths that depended on the old always-miss lookup: - The isolated store's clone-completion re-ran the dependency resolver and relied on the in-memory lookup missing to fall through to the checkout enqueue. Once the lookup could hit, a cold-cache isolated install of a lockfile-resolved git dependency bound the dependency, never scheduled the checkout, and hung forever. The GitClone completion now has a store-installer branch that enqueues each waiter's checkout directly from its lockfile resolution, the same shape as the hoisted-installer branch and the clone-failure drain. - bun update targets would rebind to the lockfile package on their first enqueue and never re-fetch. Mirror the npm arm's should_update gate: update targets skip the bind on the fresh (alias-named) enqueue only; the post-extract re-enqueue binds as before. Also mirror the alias fallback in resolve_peer_dep_version_based so it keeps reproducing the fresh resolver's choice, assert the extracted tarball package's contents in the reuse test, and pin two behaviors with new tests: an override keyed by a github dependency's declared name applies without downloading the tarball, and bun update re-fetches github and remote tarball dependencies.
|
Updated 9:58 AM PT - Aug 7th, 2026
✅ @robobun, your commit 06f8a1f3849b8ab640b654a4b82e31533c9d8d91 passed in 🧪 To try this PR locally: bunx bun-pr 37142That installs a local version of the PR into your bun-37142 --bun |
There was a problem hiding this comment.
No issues found; my earlier nit on resolve_peer_dep_version_based was addressed. Deferring to a human because this touches core install resolution/task scheduling — the is_update_target gate and the new IS_STORE_INSTALLER clone-completion branch in runTasks.rs change control flow in a load-bearing path and warrant a maintainer's eyes.
Reviewed: the alias fallback and its use of name_hash in the git/github/tarball arms; that is_update_target mirrors the existing should_update split-borrow pattern at get_or_put_resolved_package_with_find_result; that the new store-installer GitClone branch mirrors the existing failed-clone waiter drain (checkout_id derivation, has_created_network_task dedupe); and that the four new tests are hermetic (local servers, pinned cache dir) and cover reuse, override short-circuit, bun update re-fetch, and the isolated cold-cache clone→checkout path.
Extended reasoning...
Overview
Four files: PackageManagerEnqueue.rs adds an alias fallback when dependency.realname() is empty (so lockfile-loaded git/github/tarball packages can be found by name-hash on re-resolve) plus an is_update_target() helper that gates the early-bind so bun update <dep> still re-fetches; runTasks.rs adds a ~50-line else if C::IS_STORE_INSTALLER branch in the GitClone success path that enqueues checkouts directly from lockfile resolutions (needed because the alias fallback would otherwise cause the resolve-phase re-enqueue to bind and return without scheduling the checkout, starving the parked store entry); bun.lock.rs applies the same empty-realname fallback to resolve_peer_dep_version_based (my earlier nit); bun-lock.test.ts adds four request-counting tests.
Security risks
None identified. This is resolution/caching behavior — no new inputs, no auth/crypto/permissions surface. The change makes the resolver more likely to reuse a lockfile entry (fewer network fetches), gated by get_package_id's existing resolution-equality check.
Level of scrutiny
High. enqueue_dependency_with_main_and_success_fn and the run_tasks GitClone arm are core to bun install correctness. The runTasks change replaces what the isolated installer previously did on clone completion (fell through to the "Resolving!" process_dependency_list_for_ctx path) with a direct checkout enqueue driven by clone-id waiters — a real design change, not a bugfix-in-place. is_update_target differs from the npm arm's should_update in one intentional way (matches by update_requests[].name_hash rather than updating_packages, since the latter only records npm/dist-tag deps); that's called out in the comment and has a covering test.
Other factors
- The
is_update_targetunsafe split-borrow (&*(*this_ptr).lockfilealongside&mut *this_ptr) copies the exact pattern already used atget_or_put_resolved_package_with_find_result(~line 2014), so it is not net-new risk. - The new store-installer branch closely mirrors the pre-existing failed-clone waiter-drain logic in the same function (same
Dependency-waiter walk, sameTask::Id::for_git_checkoutderivation, samehas_created_network_taskdedupe). - Tests are hermetic (local
Bun.servefor github/tarball, bare-repo dumb-HTTP for git, per-testBUN_INSTALL_CACHE_DIR) and assert both no-redownload on dirty-lockfile re-resolve and re-fetch onbun update <dep>, plus the isolated-linker cold-cache path that exercises the new runTasks branch. - Outstanding bot noise: comment-cop flagged several multi-line comments; the author already responded that they were trimmed to short invariant notes, and the current diff bears that out. CodeRabbit's "skipIf git unavailable" suggestion is minor — many existing install tests assume git.
- My earlier inline nit (mirror the fallback in
resolve_peer_dep_version_based) was addressed.
Given the scope (new control-flow branch in the install task state machine + a new gate on lockfile binding), this should get a maintainer review rather than an auto-approval.
…Y comments The comment named manager.root_dependency_list (no such field) and manager.workspace_package_json_cache (not accessed); the function reads manager.root_package_id and manager.workspace_name_hash. The disjointness claim was and remains correct. Fixes both the new instance and the pre-existing one it was copied from.
There was a problem hiding this comment.
No bugs found and both prior nits (the resolve_peer_dep_version_based mirroring and the inaccurate SAFETY comments) are now addressed, but this touches core package-manager resolution and adds a new isolated-store git checkout scheduling path — worth a human look.
What was reviewed:
- The alias fallback for empty
realname()and its interaction with overrides /package_indexkeying. is_update_targetagainst the pre-existingshould_updategate at line ~2016 — same shape (root-dep + catalog +update_requestsname-hash match), and only applied on the provisional-name enqueue so the post-extract re-enqueue still binds.- The new
IS_STORE_INSTALLERGitClone-success arm inrunTasks.rs— mirrors the existing failure-arm waiter drain and theIS_PACKAGE_INSTALLERcheckout enqueue;has_created_network_taskstill dedupes. - Tests are hermetic (local servers, pinned cache dir) and cover the negative (
bun updatestill re-fetches) and the isolated cold-cache reinstall.
Extended reasoning...
Overview
Four files: ~90 net Rust lines across PackageManagerEnqueue.rs (alias fallback + is_update_target gate on the git/github/tarball lookup arms, plus a SAFETY-comment correction), runTasks.rs (new IS_STORE_INSTALLER branch in the GitClone success arm that enqueues checkouts directly from lockfile resolutions instead of routing through the dependency resolver), and bun.lock.rs (5-line mirroring of the alias fallback in resolve_peer_dep_version_based). ~340 lines of new tests in bun-lock.test.ts.
Security risks
None identified. The change narrows when a network task is created (reuses an in-memory package the lockfile already loaded) and does not relax any integrity or origin check. The is_update_target gate ensures explicit bun update requests still re-fetch. The new unsafe blocks are lifetime-detach reads of string_bytes under the same invariant the sibling IS_PACKAGE_INSTALLER arm already relies on, and a tag-checked union read.
Level of scrutiny
High. This is the package manager's resolve/install task graph — the very control flow this PR fixes was load-bearing for the isolated linker (the old code depended on the lookup missing to schedule the checkout), and the fix compensates by adding a dedicated scheduling arm. That kind of coupling is exactly where a maintainer's review is valuable: they can confirm the isolated store's enqueue_git_for_checkout contract (waiters parked under the clone id, entry contexts under the checkout id) and whether the if let Some(waiters) = ...remove(&task.id) branch correctly replaces the previous fall-through to process_dependency_list_for_ctx in every store-installer scenario.
Other factors
- Both nits I raised on earlier revisions were addressed (peer-dep mirroring in 0f3dde5, SAFETY comment field names in 06f8a1f); all comment-cop and CodeRabbit threads are resolved.
- Test coverage is good: request-counting servers assert no re-download, a cold-cache isolated reinstall covers the new
runTasks.rsbranch, and abun updatetest guards against the gate over-applying. Tests follow harness conventions (port: 0,bunEnvspread,await using, pinnedBUN_INSTALL_CACHE_DIR). - The
is_update_targethelper duplicates the shape of the inlineshould_updateblock rather than sharing it — reasonable given the different call site, but a maintainer may prefer one implementation. - CI build #90162 was in progress at the time of the last timeline update.
1 participant
Symptom
Whenever
bun installre-resolves (any package.json change that dirties the lockfile and re-enqueues the dependency, most commonly any dependency edit in a workspace member), everygithub:,git:, and remote-tarball dependency the loaded lockfile had already resolved is downloaded again: a fresh codeload tarball request, a git fetch against the remote, or a tarball re-download, despite a warm cache and an unchanged resolution. The extract then appends a transient duplicate package to the in-memory lockfile which the lockfile clean prunes again, sobun.lockstays correct and the cost is the redundant network round trip on every such install.Repro (observable with a request-counting server standing in for the GitHub API):
"gh-dep": "github:owner/repo#sha".bun install: one tarball request.bun installagain: a second tarball request for the unchanged github dependency, every time.With
BUN_DEBUG_PackageManager=1the second install shows the lookup failing with an empty name:Cause
enqueue_dependency_with_main_and_success_fncomputes the lookup name asdependency.realname(), and forGit/Github/Tarballversions that is the package name from the extracted package's package.json, which is only filled in after the first extract completes (runTasks.rs) and is empty on a freshly parsed dependency. The git/github/tarball arms then calllockfile.get_package_id(name_hash, None, &res)withname_hash = hash(""), whilepackage_indexkeys packages by their real names, so the lookup always missed and a resolve-phase network task was created. (Theresolutionparameter does not help these arms either: the common workspace path re-parses the member and enqueues its dependency list with invalid resolution slots.)Fix
When
realname()is empty, fall back to the dependency's alias for the name-hash lookup. For these dependency types the alias is the package's name everywhere else in the tree until the extract teaches us otherwise (the network task's provisionalPackagealready usesdependency.name), and in practice it equals the real package name, soget_package_idnow finds the loaded package and the resolution equality check (which already matched correctly) binds the dependency to it with no network task. Aliased dependencies whose alias differs from the extracted name keep the previous behavior (download, then dedupe on the re-enqueue after extract).A side effect worth noting: the stale resolve-phase task entry this codepath left behind is what armed the isolated-linker patchedDependencies deadlock fixed in #37136 (see that PR's Cause section). The two fixes are complementary: this one stops the resolve phase from creating the redundant task at all, #37136 stops the install phase from re-requesting tarballs the cache already has.
Verification
Two new tests in
test/cli/install/bun-lock.test.ts, both against local servers (fake codeload tarball server; bare git repo over dumb HTTP):re-resolving reuses github and remote tarball packages from the lockfile instead of re-downloading: install, dirty the member, install again, assert the download counters did not move. On unfixed bun both counters go 1 -> 2.re-resolving reuses a git package from the lockfile instead of re-fetching: same shape for a sha-pinnedgit+httpdependency; on unfixed bun the second install makes 2 extra requests.Ran locally with the debug build:
test/cli/install/bun-lock.test.ts: 19 passtest/cli/install/isolated-install.test.ts: 62 passtest/cli/install/bun-workspaces.test.ts: 63 passtest/cli/install/overrides.test.ts: 7 passtest/cli/install/bun-install-patch.test.ts+bun-patch.test.ts: 49 passtest/cli/install/bun-add.test.ts: 54 pass,bun-update.test.ts: 6 pass,bun-lockb.test.ts: 6 passtest/cli/install/bun-install.test.ts -t GitHub: 10 pass (1 unrelated failure from a blocked external host in the sandbox, identical on released bun)Also verified manually that the isolated-linker patched-github-dependency scenario from #37136 completes with this change alone (the resolve phase no longer creates the task entry that parked the install-phase callbacks).
Branch-ref git dependencies (
#mainor no committish) still re-fetch on re-resolution: the text lockfile stores the resolved sha in the committish position, so the loaded resolution can't be matched against a branch-ref parse. That lossiness is a separate pre-existing issue and out of scope here.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-lock.test.ts