install: keep optional peer bindings when cleaning the lockfile - #37426
Conversation
Loading bun.lock binds every optional peer to the package next to it before hoisting, but since #35681 Package::clone dropped those bindings again, so the tree built while cleaning could differ from the tree built while loading. For a graph where the peer target's subtree competes with another package for a hoisted slot, --frozen-lockfile then failed on an unchanged project, including on the lockfile bun had just written, and a re-save moved packages around (test/bun.lock: jsbn). Bind the slot after the clone queue drains instead, and only when a non-peer edge cloned the target, so a target held only by peer slots is still dropped. Hoist a second time when a peer was bound by a package placed after its dependent, so a fresh install writes the same layout a reload builds.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (31)
WalkthroughChangesOptional peer hoisting
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:36 AM PT - Aug 11th, 2026
✅ @robobun, your commit 5755a5d2215128a8473e1c6c1d80041c64511503 passed in 🧪 To try this PR locally: bunx bun-pr 37426That installs a local version of the PR into your bun-37426 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked that the optional-peer branch in Package::clone leaving *resolution unset is safe — the slot is pre-filled with invalid_package_id by extend_from_fn at Package.rs:531, and Cloner::flush only overwrites it when the target was cloned. Also confirmed hoist() has no callers outside resolve()/filter(), both updated for the new Result<bool, _> return.
Extended reasoning...
This PR reworks how optional peer bindings survive the lockfile clean and adds a conditional second hoist pass in Lockfile::resolve. It touches core package-manager hoisting logic and embeds a design choice (keep the recorded binding vs. re-derive everywhere) that affects lockfile stability across Bun versions, so a human should look at it. The one inline finding is cosmetic and effectively unreachable; I verified the two non-obvious safety points above so a later pass need not re-derive them.
|
On the two PRs flagged above: #37350 fixes one shape of the same load-vs-clean mismatch (a bundled dependency's optional peer) on the load side, by stopping the parser's path walk at the bundle root. This PR fixes the mismatch itself, so #37350's test also passes on this branch without its #35571 is about workspace hoist order, but it ran into this same mismatch on its repro and carries a side hunk that makes the parser skip binding optional peers when loading |
…oist until settled A carried-over binding is only kept if the bound package is still the one placed next to the dependent. When another version of it already holds that slot and the peer range accepts it, the dependent dedupes onto it, so repoint the binding there as well: that is what a reload binds from the saved tree, and the isolated linker keys the dependent's store entry by it, so the install that wrote bun.lock and the next install from it now link the same entry. Binding one peer can move a dependent and put a target within reach of a second peer the previous pass could not bind, so resolve() hoists until a pass binds nothing late instead of exactly twice. Each extra pass fills at least one more slot and none is ever emptied, so it ends.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Pushed a second round after reviewing the first version more carefully (description updated to match):
#37350's test still passes on this branch (checked again after these changes), and |
There was a problem hiding this comment.
I reviewed the changes since my earlier note (the Rebind variant, the loop-until-settled in resolve(), and the deferred optional_peers binding in Cloner::flush) and didn't find any bugs. Given this reworks hoist convergence for optional peers and is one of three mutually-exclusive approaches in flight (#35571, #37350), a maintainer should sign off on the direction.
What was reviewed:
- Termination of the
while self.hoist(...) {}loop —late_bound_optional_peeris only set onResolveReplace, which fills a previously-invalid slot, and no path clears one, so it's bounded by the optional-peer count. Rebindis gated onMETHOD == Resolvablesofilter()(install-time tree) still returnsHoistedand doesn't mutate resolutions.Cloner::flushbinds deferred peers only when the target survived the clone (mapping < max_package_id), so #35681's remove behavior is preserved.Package::clonenow constructsPendingResolutionbefore the optional-peer check but the*old_resolution >= max_package_idguard runs first, soold_resolutionis always a valid index intomapping.
Extended reasoning...
Overview
This PR touches the lockfile clean/hoist pipeline in src/install/lockfile.rs, lockfile/Package.rs, and lockfile/Tree.rs, plus ~200 lines of new tests in bun-lock.test.ts and a set of Verdaccio registry fixtures. The core change: instead of clearing optional-peer resolution slots during Package::clone (as #35681 introduced), defer them to a new Cloner::optional_peers list and bind them in Cloner::flush after the clone queue drains — but only if the target package was independently kept alive. Lockfile::resolve now loops hoist() until a pass binds no optional peer late, and a new HoistDependencyResult::Rebind variant updates the in-memory binding when a bound optional peer dedupes onto a different version already holding the slot.
Security risks
None. This is package-manager resolution bookkeeping over data already parsed and validated by bun.lock.rs. No new untrusted-input parsing, no filesystem paths derived from new fields, no network. The registry fixtures are local test data.
Level of scrutiny
High. The hoist algorithm is what determines the on-disk node_modules layout and bun.lock contents for every install; a bug here silently corrupts lockfiles or breaks --frozen-lockfile in CI for real projects. The change also introduces a fixed-point loop whose termination depends on a monotonicity invariant ("no pass empties a slot") that is argued in prose rather than enforced by the type system. The reasoning is careful and the tests are strong (each pinned to a specific failure mode without the fix, including the isolated-linker store-entry consistency check), but this is exactly the kind of subtle graph-algorithm change where a maintainer who owns src/install/ should confirm the approach.
Other factors
- Two other open PRs address overlapping symptoms with conflicting designs: #35571 clears the load-side binding (opposite direction, forces a one-time lockfile rewrite), and #37350 bounds one specific load-side walk. The author has already laid out the reconciliation in the thread, but choosing between "keep the binding" and "stop binding on load" is a design call.
- My earlier inline note (duplicated
Invalid dependency namediagnostic across hoist passes) was acknowledged as cosmetic-only and intentionally left; that's reasonable and now moot to re-raise. - The comment-cop bot flags were addressed in 5755a5d (comments trimmed to one-liners).
- Test coverage is thorough: fresh install → frozen → lockfile-only round-trips, both historical placements accepted under
--frozen-lockfile, the multi-pass shape-2 fixture, and the isolated-linker rebind check. The #35681 regression tests remain in the file and are stated to still pass.
3 participants
Repro
On a pristine checkout of main, with the released canary (1.4.0-canary.1, 9008ae7):
The re-save flips the top-level
jsbnfrom 1.1.0 to 0.1.1, dropsecc-jsbn/jsbnandsshpk/jsbn, and addsip-address/jsbn. Re-saving does not help:--frozen-lockfilerejects the re-saved lockfile too, so a project with this shape cannot pass--frozen-lockfileat all. 1.3.14 accepts both layouts. Any project with an optional peer whose target is reached through a deeper dependency, plus a version conflict somewhere under that target, hits this (heremongodbhas an optional peer onsocks,socks -> ip-address -> jsbn@1.1.0, andsshpkwantsjsbn@0.1.1). The new registry fixtures are a small version of the same graph, and the same three commands fail on them the same way.Cause
Loading
bun.lockbinds every optional peer to the package next to it (the path walk inbun.lock.rs) and then hoists. Since #35681,Package::clonewritesinvalid_package_idinto optional peer slots while cleaning, so the hoist inCloner::flushruns with those edges unbound.Hoisting is breadth-first, and the two runs queue the peer target's subtree at different times: with the edge bound, the target is placed when its dependent is (
socksfrommongodb, at depth 1) and its subtree is walked right after; unbound, the target is only placed when the first real edge reaches it (socks-proxy-agent, several levels down) viaResolveReplace. Whatever conflicts under that subtree (jsbn) is hoisted differently, so the tree built at load time and the tree built by the clean differ.--frozen-lockfilecompares exactly those two trees (Lockfile::eql), which is why it fails on an unchanged project and keeps failing after a re-save: the file is written from the clean's tree, but loading it binds the peers again and builds the other one. The same thing makes an unrelatedbun addrewrite these entries. #35681 described this as a one-time rewrite; it is not, because the load side still binds the slots.Fix
Three pieces, all in the lockfile tree builder:
Clean keeps the bindings.
Package::clonedefers optional peer slots toCloner::optional_peersinstead of clearing them;Cloner::flushbinds them once the clone queue has drained, and only when the target was actually cloned, i.e. some non-peer edge still reaches it. A target held only by peer slots is still never cloned, so what install: drop packages held only by optional-peer resolution slots from bun.lock #35681 fixed (bun removeleaving the package behind) stays fixed and its tests still pass. For a surviving target this is what 1.3.x did (Package.clonein Zig copied every slot), so lockfiles written by 1.3.x are fixed points:test/bun.lockpasses--frozen-lockfileand re-saves byte-identically with this branch, without touching the file.A kept binding has to be the one the tree expresses. The hoister places a bound optional peer like any dependency, except that when another version of the target already holds the slot next to the dependent and the peer range accepts it, the dependent dedupes onto that version. In that case the binding now moves to that version too (
HoistDependencyResult::Rebind, in the saved tree only). That is what node resolution finds, what the path walk binds on the next load, and what the isolated linker keys the dependent's store entry by, so the install that writesbun.lockand the next install from it link the same entry. Without this, a carried-over binding could survive in memory for one run while the saved tree said otherwise (the isolated linker would then link against one version and a reinstall against the other). Required peers are untouched: they keep the resolver's version, whichbun.lock.rsreproduces by version since install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182. So the rule for an optional peer is unchanged from main: it is bound to whatever ends up next to it. The carry-over only matters when the bound package is still the one placed there, and then it keeps its place instead of being re-derived.Fresh installs settle in the same pass structure a reload has. When a peer is bound late in a hoist pass (
ResolveReplace), the pass is repeated, until a pass binds nothing late. The last pass built the tree from bindings it had up front, which is what a reload does, so a fresh install writes the same file a reload would write instead of one that converges on the next re-save. Binding one peer can move a dependent and put a target within reach of a second peer that the previous pass could not bind (shape 2 of the fixtures: the target is hidden behind a bundled copy of itself until the first binding hoists its dependent out), which is why this is a loop rather than one extra pass. Each pass that repeats filled at least one empty slot and no pass empties one, so it ends after at most one pass per optional peer; in practice lockfiles loaded from disk take one pass and fresh installs one or two.filter(the install-time tree) runs after this and stays single-pass.Why keep the binding rather than also stop binding at load time (the other way to make the two sides agree, which #35571 carries as a side change)? Re-deriving everywhere rewrites every existing lockfile with this shape once, failing
--frozen-lockfilein CI on the bun upgrade (that approach had to regenerate thenext-pageslockfile snapshots; this branch regenerates nothing), and it rebinds a peer to a different version of its target on unrelatedbun adds. The binding recorded in the lockfile is a resolution like any other; keeping it while the target exists and the tree can express it is what the lockfile is for, and it is what 1.3.x did. Lockfiles written by builds with #35681 (the other placement) are accepted as well and converge on their next re-save; the parameterized test covers both placements. #37350 (bundled dependency's optional peer bound across the bundle root at load) is a different shape of the load/clean mismatch; its test passes on this branch too, and it is still worth landing on top because it stops that edge from being bound at all.Verification
Registry fixtures
optional-peer-hoist-*(generated bycreate-optional-peer-hoist-packages.ts, which documents both shapes; every tarball is a singlepackage.json, plus the bundled copy intarget@3.0.0). Tests added totest/cli/install/bun-lock.test.ts, each with what fails without the fix:--frozen-lockfilepasses and--lockfile-onlyre-saves byte-identically (before:error: lockfile had changes, but lockfile is frozenon the file the install just wrote)tail@2.0.0at the root) and re-saves identically (before, and with a fixed two passes:tail@1.0.0at the root and the first re-save rewrites it)--frozen-lockfileaccepts a hand-writtenbun.lockin either placement (both rejected before)providerkeepsconsumerbound totarget@1.0.0,target@2.0.0nests underprovider,--frozen-lockfilepasses, re-save identical (before: rebound to 2.0.0 and the existing entries re-hoisted)provideraliased to sort first, under the isolated linker:target@2.0.0takes the root, and the store entryconsumeris linked to by the writing install is the one a--frozen-lockfilereinstall from scratch links to as well (fails with piece 1 alone: the writing install still links thetarget@1.0.0variant)The four tests from #35681 still pass. Also run with the debug build:
bun-lockb,migrate-bun-lockb-v2,hoist,lockfile-only,lockfile-version-2,isolated-install(including the #32182 peer stability tests),bun-remove,bun-add,bun-update,bun-workspaces,overrides,catalogs,public-hoist-pattern,config-version,bun-pm,bun-pm-why,bun-install-registry,bun-installand themigration/suites (lockfile snapshots unchanged). The only failures are tests needing bitbucket/gitlab/external network access, which fail identically with the released build in this environment.cd test && bun bd install --frozen-lockfilepasses on the committedtest/bun.lock, and--lockfile-onlyleaves it unchanged.