Conversation
…val order
A peer dependency whose manifest was already in memory when its parent
was processed got bound right away to whichever version of the package
happened to be in the lockfile at that instant. For every range except
`*` this was already suppressed so the peer is resolved in the deferred
peer pass, once all siblings have been appended. Extend that to `*`,
which is the range most exposed to arrival order since it is satisfied
by every sibling.
This is what made "hoisting > peers > it should hoist 1.0.1 when peer *"
flaky: on slow machines the a-dep manifest was processed before
peer-a-dep-star's, so the peer bound to a-dep@1.0.9 and that is what got
hoisted. The test only passed elsewhere because the peer normally ends
up on 1.0.10 and toContain("1.0.1") matches "1.0.10". Compare the exact
version now, expect 1.0.10 for `*`, and add a test that forces the slow
arrival order through a small proxy in front of the registry. The macOS
todo on the `^1.0.2` case covered the same class of flake and is removed.
WalkthroughChangesPeer dependency resolution
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:20 PM PT - Aug 11th, 2026
✅ @robobun, your commit 61dfaf9c8e7826243e7c4d1f6a65666b427eddae passed in 🧪 To try this PR locally: bunx bun-pr 37713That installs a local version of the PR into your bun-37713 --bun |
|
Status: fix pushed (resolver + bun.lock loader), waiting on CI. Reproduced both halves deterministically in
Supersedes #33972, which only marks the case todo. #36476 is the long term fix for arrival-order dependence in general. |
The text lockfile loader kept the same `*` exemption as the resolver, so a `*` peer edge was rebound to whatever the printed tree hoisted nearest to it, while the resolver binds it to the highest version in the tree. Whenever those differ the isolated linker keys a second store entry for the dependent on every warm install. Route `*` peers through the same version scan as every other peer range, matching the resolver.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/PackageManager/PackageManagerEnqueue.rs (1)
1986-2007: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReuse
suppress_peer_satisfiesin the fallthrough branch.Line 1986 and line 2005 compute the same predicate
behavior.is_peer() && !install_peer. The two sites must stay in sync: line 1986 disables the range match, and line 2005 defers the dependency. If one changes later, deferred peers bind by exact resolution but never defer, or defer without the exact-match attempt.♻️ Proposed refactor
- } else if behavior.is_peer() && !install_peer { + } else if suppress_peer_satisfies { return Ok(None); }🤖 Prompt for 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. In `@src/install/PackageManager/PackageManagerEnqueue.rs` around lines 1986 - 2007, Reuse the existing suppress_peer_satisfies variable in the fallthrough condition after get_package_id instead of recomputing behavior.is_peer() && !install_peer, keeping both range-matching and dependency-deferral paths governed by the same predicate.
🤖 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-registry.test.ts`:
- Around line 3735-3738: Remove the conditional console.log block around the
installed-output assertion, leaving the expect(out).not.toContain("package
installed") assertion unchanged.
---
Outside diff comments:
In `@src/install/PackageManager/PackageManagerEnqueue.rs`:
- Around line 1986-2007: Reuse the existing suppress_peer_satisfies variable in
the fallthrough condition after get_package_id instead of recomputing
behavior.is_peer() && !install_peer, keeping both range-matching and
dependency-deferral paths governed by the same predicate.
🪄 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: 1ddeee79-07c9-4061-ac0e-c428c8e3d5fd
📒 Files selected for processing (3)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/lockfile/bun.lock.rstest/cli/install/bun-install-registry.test.ts
There was a problem hiding this comment.
Thanks for addressing the mirrored is_deferred_peer exemption in 69fcc80 — the loader and resolver now agree, and the new bun.lock round-trip test covers exactly the store-entry re-keying I flagged. I re-reviewed and found no further issues. Since this changes the user-visible resolution of * peers (now deterministically the highest sibling in the tree rather than arrival-order-dependent) and the PR frames itself as a stopgap ahead of #36476, a maintainer sign-off on the intended semantics would still be worthwhile.
What was reviewed:
suppress_peer_satisfiesnow covers*— deferred peers fall through to theis_peer && !install_peerreturn and bind in phase 2's descending-index scan; the exact-match reuse path is preserved.- All three
is_deferred_peercall sites inbun.lock.rsnow route*peers throughresolve_peer_dep_version_based; optional-peer exemption kept, matchingprocess_subtree's hoisted-sibling binding. - Test tightening:
toContain("1.0.1")→ exact.toBe()viahoistedADepVersion(); the removedisMacOSimport has no remaining references; the proxy test releases each held response on a request bun only makes after the prior step (no timers), and the strayconsole.logis gone.
Extended reasoning...
Overview
Two-line logic change in each of two files, plus test hardening. PackageManagerEnqueue.rs drops the is_star() exemption from suppress_peer_satisfies, so a still-deferred * peer can only bind early on an exact eql(find_result) match and otherwise falls through to return Ok(None) at line 1997-1998, to be resolved in the install_peer pass by get_or_put_resolved_package's descending-index scan. bun.lock.rs drops the same exemption from is_deferred_peer, so the text-lockfile loader routes * peers through resolve_peer_dep_version_based (the loader-side replica of that scan) instead of the printed-tree path walk. The test file tightens the existing hoisting assertions from toContain to exact-version toBe, changes the peer * expectation from 1.0.1 to 1.0.10, removes the macOS todoIf on peer ^1.0.2, and adds two new tests: a proxy that forces the flaky arrival order, and a bun.lock/isolated-linker round-trip.
Security risks
None. No untrusted-input parsing, no allocation sizing, no auth/crypto. The change narrows an early-binding condition; the fall-through path already existed and handles every other peer range.
Level of scrutiny
High — this is bun install peer resolution, a critical path every user hits, and it changes what * peers resolve to. The old behavior was non-deterministic (the whole point of the PR: it flaked on Windows CI depending on which manifest arrived first), so any deterministic outcome is strictly better, but "highest sibling" vs. e.g. "lowest sibling" or "nearest hoisted" is a semantic choice a maintainer should confirm, especially since the PR itself calls this a stopgap ahead of #36476's rework.
Other factors
My previous review flagged that the resolver change would desync from is_deferred_peer in the lockfile loader; the author fixed that in 69fcc80 and added a dedicated test (peer * binds to the same version when bun.lock is loaded as when it was resolved) that fails on main with the isolated linker creating a duplicate store entry. The comment-cop and CodeRabbit nits (long comments, stray console.log) were addressed in 35fdb95 and 61dfaf9. All review threads are resolved. The proxy test is well-constructed — it uses Promise.withResolvers gates keyed on request paths bun only issues after the prior step completes, so it has no timers and no sleep. I checked that the removed isMacOS import has no other references in the file, and that all three is_deferred_peer call sites in bun.lock.rs benefit from the change. I did not find any other is_star() special-casing of peers in the install crate that would need the same treatment.
|
This same Two things that may save whoever rebases this some time:
Suites run locally with the rebased change (debug build): isolated-install, bun-install-registry, bun-lock, bun-lockb, bun-workspaces, catalogs, bun-dedupe, bun-prune, bun-pm-licenses, bun-pm-why, bun-update-transitive, frozen-lockfile-pruned and the pnpm migration suites, all green. |
Problem
bun-install-registry.test.tshoisting > peers > it should hoist 1.0.1 when peer *goes red on unrelated PRs, mostly on Windows (latest: build 92591, 4/4 attempts):Expected to contain: "1.0.1", receiveda-dep1.0.9.a-depapeer *edge binds to depends on registry response order. If thea-depmanifest is already in memory when the peer is seen, it binds at once to the highesta-depin the lockfile at that instant; otherwise it waits for the peer pass and gets the highest overall.*was exempted because thepeer *test was believed to need it. It does not: the test normally gets1.0.10, whichtoContain("1.0.1")also matches.bun.lockloader had the same*exemption, so a reloaded*peer can bind to a different version than the resolver chose. When it does, the isolated linker adds a second store entry for the dependent on every warm install (2 packages installedinstead of(no changes)). This part is already reachable on main.Fix
*exemption in the resolver: a deferred peer binds early only on an exact match and otherwise waits for the peer pass, like every other range. The result depends only on the full set of siblings, never on arrival order (1.0.10here).bun.lockloader, so a reload runs the same version scan as the resolver and the two agree.bun.lockwith a*peer bound lower rebinds once to the highest version, as non-*peers already do;--frozen-lockfilestill passes on it.1.0.9as CI) and passes 30/30 with it. A second new test fails on main for the reload case. Stopgap until Deterministic dependency resolution: an ordered walk over a live tree #36476; test(install): mark "peer *" hoisting case todoIf(isFlaky) #33972 can be closed in favour of this.Background
install_peerpass after every regular dependency has been appended.node_modules/a-depis whichevera-depthe first sorted root dependency resolved to. Herepeer-a-dep-starsorts first, so its binding is what the test reads.bun.lockrecords the tree, not peer bindings; on load each peer edge is re-derived, by walking the printed tree or by the resolver's version scan. The isolated linker keys store entries by the peer versions a package resolved to, so both derivations must agree or warm installs churn.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
Original description
Fixes the
bun-install-registry.test.tsflake that has been going red on unrelated PRs for weeks, mostly on the Windows lanes (latest: build 92591, 4/4 attempts):Cause
Root depends on
uses-a-dep-1..10(each pinsa-dep@1.0.N) andpeer-a-dep-star(peera-dep@*).peer-a-dep-starsorts first among the root dependencies, so whatever its peer resolves to is what gets hoisted tonode_modules/a-dep.How the peer resolves depends on what the registry answered first. If the
a-depmanifest is not in memory yet whenpeer-a-dep-staris processed, the peer is deferred and resolved in the peer pass after every sibling has been appended, where the descending package index gives1.0.10. If the manifest is already in memory (slow machine: thea-depmanifest, requested after the firstuses-a-dep-*response, lands before the remaining initial responses),get_or_put_resolved_package_with_find_resultbinds the peer immediately to the highesta-depthat exists in the lockfile at that instant. On the failing machines that was1.0.9, becauseuses-a-dep-10had not been processed yet.suppress_peer_satisfiesalready prevents exactly this for every other range (>=,^,~, exact), but*was explicitly exempted, with a comment saying thepeer *test depended on it. It does not: the test normally gets1.0.10and only passed becausetoContain("1.0.1")matches"1.0.10".The text lockfile loader (
is_deferred_peerinbun.lock.rs) carried the same*exemption: on reload, a*peer edge was rebound by the printed-tree path walk (nearest hoisteda-dep) instead of the version scan the resolver uses. Whenever the two differ, the isolated linker keys a second store entry for the dependent on every warm install. That part was already reachable on main, since the resolver usually deferred*peers too: withuses-a-dep-1,uses-a-dep-5and a workspace depending onpeer-a-dep-star, the secondbun install --linker isolatedreports2 packages installedand adds a secondpeer-a-dep-star@1.0.0+<hash>entry tonode_modules/.bun.Fix
PackageManagerEnqueue.rs: drop the*exemption, so a deferred peer only binds early on an exact match of the manifest's best version and otherwise waits for the peer pass like every other range. The result no longer depends on arrival order (1.0.10here).bun.lock.rs: drop the matching exemption inis_deferred_peer, so loadingbun.lockbinds*peers with the same version scan and agrees with the resolver.Loading a
bun.lockwritten before this change whose*peer had been bound to a lower version rebinds it to the highest one in the tree, the same one-time adjustment non-*peers already get;--frozen-lockfilestill passes on such a lockfile (checked with a lockfile produced by the forced arrival order below).This is a stopgap. #36476 reworks resolution so arrival order cannot matter anywhere; #33972 (marks the case todo) can be closed in favour of this.
Tests
hoistingtests compare the exacta-depversion instead oftoContain, andpeer *expects1.0.10.peer * hoists the same version no matter which manifest the registry answers first: a small proxy in front of verdaccio holdspeer-a-dep-star's manifest until ana-deptarball has been requested (so thea-depmanifest is loaded and1.0.1..1.0.9exist) and holdsuses-a-dep-10's manifest until thepeer-a-dep-startarball has been requested. Each release is triggered by a request bun only makes after the previous step, so there are no timers. Without the fix it fails with1.0.9on every run (20/20 locally, the same value CI reports); with it, 30/30 pass.peer * binds to the same version when bun.lock is loaded as when it was resolved: the isolated-linker scenario above with--save-text-lockfile. On main the second install prints2 packages installedinstead of(no changes); with the fix the store entry set is unchanged.todoIfon thepeer ^1.0.2case was for the same class of flake and is removed; that range has been covered bysuppress_peer_satisfiessince the rewrite.bun-install-registry.test.ts,bun-install.test.ts,bun-add.test.ts,bun-lock.test.ts,isolated-install.test.ts,bun-workspaces.test.ts,test-dev-peer-dependency-priority.test.ts, the pnpm/yarn migration suites andregression/issue/36577.test.tspass locally with the debug build (the only failures inbun-install.test.tsare the git/tarball tests that need network access and fail identically without this change).