install: don't resolve a bundled dependency's optional peer past the bundle hoist root when loading bun.lock - #37350
Merged
Jarred-Sumner merged 4 commits intoAug 12, 2026
Conversation
… bundle hoist root when loading bun.lock The hoister never searches past a bundled dependency's hoist root when it re-derives optional peer edges, but the bun.lock parser's path walk went all the way to the root. Loading a lockfile where a bundled dependency declares an optional peer that is satisfiable from the root then produced a tree a fresh install would never generate, and --frozen-lockfile rejected a lockfile bun itself had just written. Bound the parser's path walk at the bundle hoist root for optional peer edges, matching Tree::hoist_dependency. Fixes #37346
Contributor
|
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 (1)
WalkthroughChangesBundled optional-peer resolution
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
Author
|
Updated 6:25 PM PT - Aug 10th, 2026
✅ @alii, your commit b0abf3816972d6ecc7c385ae68f75d1a88aeb2b2 passed in 🧪 To try this PR locally: bunx bun-pr 37350That installs a local version of the PR into your bun-37350 --bun |
Jarred-Sumner
deleted the
farm/f0091ae1/frozen-lockfile-bundled-optional-peer
branch
August 12, 2026 06:15
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 13, 2026
### Repro On a pristine checkout of main, with the released canary (1.4.0-canary.1, 9008ae7): ```sh cd test bun install --frozen-lockfile # error: lockfile had changes, but lockfile is frozen bun install --lockfile-only && git diff --stat bun.lock # re-hoists jsbn bun install --frozen-lockfile # still fails on the re-saved file ``` The re-save flips the top-level `jsbn` from 1.1.0 to 0.1.1, drops `ecc-jsbn/jsbn` and `sshpk/jsbn`, and adds `ip-address/jsbn`. Re-saving does not help: `--frozen-lockfile` rejects the re-saved lockfile too, so a project with this shape cannot pass `--frozen-lockfile` at 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 (here `mongodb` has an optional peer on `socks`, `socks -> ip-address -> jsbn@1.1.0`, and `sshpk` wants `jsbn@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.lock` binds every optional peer to the package next to it (the path walk in `bun.lock.rs`) and then hoists. Since #35681, `Package::clone` writes `invalid_package_id` into optional peer slots while cleaning, so the hoist in `Cloner::flush` runs 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 (`socks` from `mongodb`, 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) via `ResolveReplace`. 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-lockfile` compares 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 unrelated `bun add` rewrite 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: 1. **Clean keeps the bindings.** `Package::clone` defers optional peer slots to `Cloner::optional_peers` instead of clearing them; `Cloner::flush` binds 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 #35681 fixed (`bun remove` leaving the package behind) stays fixed and its tests still pass. For a surviving target this is what 1.3.x did (`Package.clone` in Zig copied every slot), so lockfiles written by 1.3.x are fixed points: `test/bun.lock` passes `--frozen-lockfile` and re-saves byte-identically with this branch, without touching the file. 2. **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 writes `bun.lock` and 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, which `bun.lock.rs` reproduces by version since #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. 3. **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-lockfile` in CI on the bun upgrade (that approach had to regenerate the `next-pages` lockfile snapshots; this branch regenerates nothing), and it rebinds a peer to a different version of its target on unrelated `bun add`s. 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 by `create-optional-peer-hoist-packages.ts`, which documents both shapes; every tarball is a single `package.json`, plus the bundled copy in `target@3.0.0`). Tests added to `test/cli/install/bun-lock.test.ts`, each with what fails without the fix: - fresh install, then `--frozen-lockfile` passes and `--lockfile-only` re-saves byte-identically (before: `error: lockfile had changes, but lockfile is frozen` on the file the install just wrote) - shape 2: a fresh install writes the settled layout (`tail@2.0.0` at the root) and re-saves identically (before, and with a fixed two passes: `tail@1.0.0` at the root and the first re-save rewrites it) - `--frozen-lockfile` accepts a hand-written `bun.lock` in either placement (both rejected before) - adding `provider` keeps `consumer` bound to `target@1.0.0`, `target@2.0.0` nests under `provider`, `--frozen-lockfile` passes, re-save identical (before: rebound to 2.0.0 and the existing entries re-hoisted) - the same with `provider` aliased to sort first, under the isolated linker: `target@2.0.0` takes the root, and the store entry `consumer` is linked to by the writing install is the one a `--frozen-lockfile` reinstall from scratch links to as well (fails with piece 1 alone: the writing install still links the `target@1.0.0` variant) 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-install` and the `migration/` 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-lockfile` passes on the committed `test/bun.lock`, and `--lockfile-only` leaves it unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #37346
Repro
{ "dependencies": { "cdk8s": "2.70.88", "constructs": "10.8.0", "debug": "3.2.7" } }cdk8sbundlesfollow-redirects, which declaresdebugas an optional peer. Adding a top-leveldebugmakes that peer satisfiable from the root and flips the frozen install from pass to fail, even though the lockfile is byte-identical across installs.Cause
The frozen-lockfile check compares the loaded lockfile against the cleaned one with
Lockfile::eql, which compares the hoisted trees.Package::clone(run byclean_with_logger) resets optional peer edges and letsTree::hoist_dependencyre-derive them, and that search is bounded at the bundle hoist root, so the bundled dependency's optional peer ondebugstays unresolved, exactly as in a fresh install (which is whybun.lockhas nocdk8s/debugentry).The bun.lock parser's
find_resolutionpath walk had no such bound: it walkedcdk8s/follow-redirects/debug->cdk8s/debug->debugand bound the edge to the rootdebug. The parse-side hoist then placeddebug(and its dependencyms) under the bundle hoist root, producing tree placements a fresh install never generates:so
eqlreturned false and the frozen install failed.Fix
Bound the parser's path walk at the bundle hoist root for optional peer edges: once a directory on the upward walk is a bundled package, only one more level (the tree node containing the bundled package) is searched, matching
Tree::hoist_dependency. The walk for other dependency kinds is unchanged, as is lockfile serialization.Verification
bundled-optional-peer(bundlesoptional-peer-deps, which has an optional peer onno-deps) and a test intest/cli/install/bun-lock.test.tsthat installs it together withno-depsand runs--frozen-lockfiletwice (with and withoutnode_modules). The test fails witherror: lockfile had changes, but lockfile is frozenwithout the src change and passes with it.test/cli/install/bun-lock.test.ts,lockfile-version-2.test.ts, and the bundled/peer suites inbun-install-registry.test.tspass.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-lock.test.ts