install: rebind ranged peers whose target the saved tree drops - #38768
install: rebind ranged peers whose target the saved tree drops#38768robobun wants to merge 1 commit into
Conversation
A ranged peer edge bound to a package that every other edge ignores is deduped by the hoister onto the version of that name hoisted above it, so the package is never placed in the tree bun.lock prints. Loading the lockfile then binds the edge by version over the packages that were printed and picks a different target than the install that wrote the file linked, which re-keys every isolated-store entry whose peer set contains the edge on the next install. pnpm-lock.yaml migrations hit this through pnpm's auto-installed peers; adding a pinned version of a package that was previously only auto-installed for a peer hits it too. After hoisting, rebind such edges with the loader's own scan restricted to the packages the print will contain. The rebound edges are not part of the tree, so the printed lockfile is unchanged; their old targets are dropped by the next clean.
|
Warning Review limit reached
Next review available in: 7 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 (4)
Comment |
|
Status: ready for review. Every lane that exercises the package manager is green; the red lanes in CI are unrelated to this diff (see below). Reproduced on main ( With this branch the migrating install links 1644 packages, the second install prints CI (build 96949, finished): 169 of 179 jobs passed. The nine failed jobs are eight lanes (Linux and one darwin shard) on |
|
Updated 10:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 8077154 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 38768That installs a local version of the PR into your bun-38768 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes Lockfile::resolve — the hoist-and-print path every install runs — and adds a new rebind pass over buffers.resolutions, a human look from someone with package manager context would be worthwhile.
What was reviewed:
resolve_peer_dep_version_based_amongrefactor: with|_| trueit is behavior-preserving for all in-bounds candidates; the fallbackfirstnow skips out-of-bounds ids (defensive, arguably more correct).rebind_peers_to_printed_packagesinvariant: a target not inprintedimplies its dep_id is not inhoisted_dependencies, so mutatingresolutions[i]cannot invalidate the tree.AllocErrorfromDynamicBitSet::init_emptypropagates throughSubtreeErrorviaoom_from_alloc!; all fiveresolve()callers already handle it.- Both new tests are hermetic (Verdaccio), assert store contents / bun.lock bytes /
(no changes), and the PR confirms they fail on the pre-fix build.
Extended reasoning...
Overview
This PR fixes a lockfile round-trip inconsistency where the install that writes bun.lock links a different peer resolution than every install that reads it. The fix adds rebind_peers_to_printed_packages after hoisting in Lockfile::resolve (src/install/lockfile.rs:1263), which rebinds peer edges whose target was deduped out of the printed tree to the version a reload would pick. resolve_peer_dep_version_based is refactored into a filtered variant resolve_peer_dep_version_based_among so the writing install and the reload run the same scan over the same candidate set. Two tests cover the pnpm-migration shape and the pure-bun shape.
Security risks
None. This is package resolution bookkeeping over data already in the in-memory lockfile; no untrusted input parsing, no filesystem writes, no network.
Level of scrutiny
High. Lockfile::resolve runs on every path that prints a lockfile (bun.lock load, pnpm/yarn/npm migration, Cloner), and the isolated linker keys store entries on the resolutions this pass mutates. A wrong rebind would silently change what is installed. The PR description is unusually thorough (mechanism traced to a single edge on shadcn-ui, invariants stated and verified against the code), and the tests assert the exact observable outcome (store hash entries, bun.lock bytes, (no changes)), which raises confidence — but the domain reasoning ("every rebound edge was deduped, so the tree stays valid"; "the printed set is exactly root/workspaces plus hoisted_dependencies targets") is the kind of claim a maintainer who owns this code should confirm.
Other factors
- The
firstfallback inresolve_peer_dep_version_based_amongchanged fromcandidates.first()to the first in-bounds, accepted candidate. With the identity filter this only differs whenpackage_indexholds an id ≥pkg_resolutions.len(), which shouldn't happen (index is built from packages), so the change is defensive rather than behavioral. - Borrow structure in
rebind_peers_to_printed_packagesis sound:packages.items_resolution()and theBuffersfields are disjoint;printedis fixed before the mutating loop and rebinds only go from not-printed to printed, so iteration is stable. - The author ran the full install/migration/lockfile suite locally and verified the shadcn-ui repro end-to-end; no CODEOWNERS or prior reviewer comments on the timeline.
|
For whoever picks this up, the two claims the automated review asked to have confirmed, with where they come from:
The rebind is also a one-way move from a non-printed target to a printed one, so it never un-prints anything and a second pass would find nothing to do; the shadcn-ui migration writes a byte-identical |
### Problem - `bun install --frozen-lockfile` fails with `error: lockfile had changes, but lockfile is frozen` on a `bun.lock` whose tree has not changed, as soon as the file contains an entry that nothing depends on and that entry is not the last one in the file. Field case: activepieces/activepieces at HEAD, whose committed (1.3-written) lockfile 1.3.14 accepts; with #38853 applied, main still rejects it. - `Lockfile::eql` (`src/install/lockfile.rs`) compares the cleaned lockfile's tree with the tree the loaded lockfile was hoisted into at load time. It took a `cut_off_pkg_id` and left out every placement bound to a package id at or past it, on both sides. The callers pass the cleaned lockfile's package count. On the cleaned side that is a no-op. On the loaded side it does two things: it catches edges that resolving rebound to packages appended after load (those ids are past the loaded count; the loaded tree still places them, so leaving them out makes the counts differ; `bun update --frozen-lockfile` moving a transitive dependency relies on this, `bun-update-transitive.test.ts`), and, as soon as the clean dropped an entry, it also leaves out the loaded side's own last packages, whose ids are past the cleaned count but below the loaded one. That second effect is the bug: a real placement disappears from the comparison and the counts differ. - In activepieces the dropped entry is `react-dom@18.3.1`: the file nests it under `react-json-view` for a peer range (`^15 || ^16 || ^17`) nothing in the file satisfies, and since #32182 the loader binds that edge by version, falling back to the first candidate (`react-dom@19.2.5` at the root), so the nested copy is referenced by nothing and the clean drops it (4069 -> 4068 packages). The file's last entry, `mdast-util-find-and-replace@1.1.1` (id 4068), is placed twice; both placements were left out on the loaded side. The two hoists otherwise agree placement for placement (trace in the details below). ### Fix - The boundary on the loaded side is now the loaded lockfile's own package count at load time, which `mark_loaded_packages` already records for the resolver (`loaded_package_count`); the two callers in `install_with_manager.rs` pass `lockfile_before_clean.loaded_package_count`. A placement bound past it was rebound to a package appended by this install, so `eql` now returns false on it directly instead of leaving it out and relying on the counts. The cleaned side is compared whole. The debug-build determinism check in `save_lockfile` compares a lockfile with itself and keeps passing its own count. - Correct because the two things the old boundary mixed up are now separate: a rebound edge is a changed resolution and fails, and a package the clean dropped only matters if it was placed in the loaded tree, in which case its placements are now counted on the loaded side and missing on the cleaned side, which still fails. An entry that was in no tree no longer affects the result. - Visible consequence outside the frozen check: the same comparison decides whether a non-frozen install re-saves, so a no-op `bun install` on a lockfile whose only difference is an entry outside the tree no longer rewrites the file just to drop it; it is dropped the next time anything else causes a save. Installs with a package.json diff still save regardless (`had_any_diffs`). - Test: `test/cli/install/bun-lock.test.ts`, "--frozen-lockfile accepts a bun.lock with an entry nothing depends on, wherever it is listed". A hand-written lockfile with `no-deps` (depended on) and `a-dep` (not depended on) in both orders; on main the order with `a-dep` first fails with the error above, with this change both orders install `no-deps` only and leave the file untouched. The rebound case stays covered by the existing `bun update --frozen-lockfile` test in `bun-update-transitive.test.ts`, which failed on the first version of this PR (it dropped the loaded-side check entirely) and passes now. - With this change plus #38853, the committed lockfiles of activepieces, opencode, eliza and supermemory pass `--frozen-lockfile` with the debug build (activepieces still logs `4069 -> 4068` and passes); hono and remotion still pass, and a further 72 small repos with a committed `bun.lock` behave identically on 1.3.14, main and the two changes combined (65 pass everywhere, 7 are already out of date everywhere). Suites run with the debug build: `bun-lock`, `bun-update-transitive`, `bun-update`, `bun-add`, `bun-remove`, `migration/migrate`, `frozen-lockfile-pruned`, `frozen-lockfile-missing-workspace`, `lockfile-only`, `bun-lockb`, `bun-workspaces`, `isolated-install`, `bun-dedupe`, `bun-prune`, `hoist`, `catalogs`, `overrides`, `nested-overrides`, `lockfile-version-2`, `bun-install-registry`. - Not changed here: the loader binding an out-of-range peer to a different version than the entry the file nests next to the dependent (the producer of the unreferenced entry above). That rewrites such 1.3 lockfiles on the next non-frozen install and changes which copy the dependent gets; it is the same family as #38767 / #38768 / #38837 and is tracked separately. ### Background - `bun.lock` stores the hoisted tree as keys: `a/b` means `b` is installed in `a`'s `node_modules`. Loading the file creates one package per distinct entry (ids in file order), binds every dependency edge, and hoists the result into an in-memory tree. `mark_loaded_packages` then records the package count; anything resolving appends afterwards (a newer version `bun update` picked, a package added to package.json) gets a higher id, and edges may be rebound to those packages in place, while the tree built at load time is not rebuilt. - `clean_with_logger` builds a fresh lockfile by walking from the root (ids in walk order), so packages no edge reaches are dropped, and hoists it again. `--frozen-lockfile` compares that tree with the load-time tree using `Lockfile::eql`: placements are listed as (path, package), sorted, and compared pairwise on name, resolution, bins and scripts. The same comparison decides after a normal install whether the lockfile is saved again. `bun.lockb` uses a hash over the package list instead (`packages_len_before_install` still feeds that). <details> <summary>Hoist trace on activepieces (temporary instrumentation, not part of the change)</summary> Every decision the hoister made for `mdast-util-find-and-replace`, first while loading the file (ids 2628/4068), then during the clean (ids 1344/2474). The two runs are identical; only the comparison differed. ``` hoist dep_id=10546 pkg_id=2628 parent_pkg_id=2640 tree_node=858 -> Placement(ancestor) hoist dep_id=10499 pkg_id=2628 parent_pkg_id=2631 tree_node=1120 -> Hoisted(dedupe) hoist dep_id=16465 pkg_id=4068 parent_pkg_id=4043 tree_node=1222 -> Placement(own node) hoist dep_id=16465 pkg_id=4068 parent_pkg_id=4043 tree_node=1227 -> Placement(own node) hoist dep_id=3661 pkg_id=1344 parent_pkg_id=1347 tree_node=858 -> Placement(ancestor) hoist dep_id=3651 pkg_id=1344 parent_pkg_id=1343 tree_node=1120 -> Hoisted(dedupe) hoist dep_id=7043 pkg_id=2474 parent_pkg_id=2473 tree_node=1222 -> Placement(own node) hoist dep_id=7043 pkg_id=2474 parent_pkg_id=2473 tree_node=1227 -> Placement(own node) ``` Placement diff reported by an instrumented `eql` (raw `hoisted_dependencies` lengths were equal; only the filtered lists differed, by exactly the two placements of loaded id 4068): ``` only in cleaned: @tryfabric/martian/remark-gfm/mdast-util-gfm/mdast-util-gfm-autolink-literal :: mdast-util-find-and-replace@1.1.1 only in cleaned: slackify-markdown/remark-gfm/mdast-util-gfm/mdast-util-gfm-autolink-literal :: mdast-util-find-and-replace@1.1.1 ``` </details> <details> <summary>First version of this PR</summary> The first push removed the boundary altogether, on the reasoning that nothing appends packages during the install step any more. That is true of the step after the clean, but resolving before the clean still appends and rebinds, and the loaded tree is not rebuilt, so `bun update --frozen-lockfile` on a stale lockfile was accepted (`bun-update-transitive.test.ts` caught it in CI). The current version keeps that check and only moves the boundary to the loaded side's own count. </details> <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end -->
|
Cross-reference from #38892: it removes the nothing-satisfies fallback ( |
2 participants
Problem
bun installon a pnpm workspace (shadcn-ui/ui atshadcn@4.18.0, isolated linker) migratespnpm-lock.yamland installs. The nextbun installon the untouched checkout prints24 packages installedinstead of(no changes),node_modules/.bungrows by 28 entries such aseslint@9.26.0+e85e2cbf7faa9a92next to the existingeslint@9.26.0+3be8db87ac283cf5, andbun.lockis byte-identical throughout. The third install is a no-op.@hono/node-server@1.19.9's peerhono@^4is bound tohono@4.12.23by the migration and tohono@4.11.7after reloadingbun.lock.honoleaks as a transitive peer through@modelcontextprotocol/sdk@1.25.3intoeslint@9.26.0and from there into every eslint plugin, so the one edge re-keys 24 store entries.hono@4.12.23is a peer pnpm auto-installed; the peer edge is its only incoming edge. The migration binds the edge by version (resolve_peer_like_bun_lock,src/install/pnpm.rs:131) and picks 4.12.23 as the highest satisfying version. When the tree is hoisted,Tree::hoist_dependency(src/install/lockfile/Tree.rs:1025) findshono@4.11.7hoisted at the root, sees that it satisfies^4, and dedupes the edge onto it, so 4.12.23 is placed in no tree.save_from_binaryprints packages from the tree, so 4.12.23 is not inbun.lock. On reload,resolve_peer_dep_version_based(src/install/lockfile/bun.lock.rs:3352) scans the printed packages and binds the edge to 4.11.7.clean_with_loggerkeeps 4.12.23 alive for the writing install because a required peer edge points at it, so the migrating install links a binding that its ownbun.lockcannot reproduce.bun.lock, and the install that wrote thatbun.locklinks the old version while every later install links the pinned one.b7a043103) binds this edge to pnpm's recorded choice and round-trips on this repo. The non-migration shape predates it.Fix
Lockfile::resolve, the hoist that every lockfile goes through before it is printed, now callsrebind_peers_to_printed_packagesafter hoisting. It marks the packages the print will contain (root and workspaces, plus the target of every placed dependency inhoisted_dependencies) and, for each peer edge whose target is not among them, reruns the loader's scan restricted to those packages and rebinds the edge to the result.resolve_peer_dep_version_baseditself with a candidate filter (resolve_peer_dep_version_based_among), so the writing install computes the binding with the same function and the same candidate set the reload will use. Edges the loader does not bind by version (optional peers,*ranges, overridden names) returnNonefrom that function and are left alone, as before.bun.lockare unchanged (verified on shadcn-ui: the migratedbun.lockis byte-identical with and without this change). The dropped package has no incoming edges left and is removed by the nextclean_with_logger, which on the migration path runs before anything is installed or written.bun.lockis one bitset and one pass over the dependency buffer; the targets of such a lockfile are normally all printed, so nothing is scanned.eslint@9.26.0+e85e2cbf7faa9a92style entries directly, that is, the entries a reload produces; the+3be8db87ac283cf5entries keyed onhono@4.12.23are never created.test/cli/install/migration/pnpm-lock-v9.test.ts, "the migrating install links the peer version its bun.lock reloads with the isolated linker": a hand-writtenpnpm-lock.yamlwherepeer-deps-fixed'sno-deps@^1.0.0was auto-installed asno-deps@1.1.0whileone-deppinsno-deps@1.0.1. Fails before (the migrating install links 1.1.0), passes after (links 1.0.1, second install reports no changes, store listing unchanged).test/cli/install/isolated-install.test.ts, "a ranged peer whose auto-installed version drops out of the saved tree is rebound before linking": the non-migration shape above. Fails before (the install that drops 1.1.0 frombun.lockstill links the+7ff199101204a65dentry), passes after.shadcn@4.18.0: the migrating install now links 1644 packages, the second install printsChecked 1665 installs across 1868 packages (no changes),node_modules/.bunis identical across the two, andrm -rf node_modules && bun install --frozen-lockfileproduces the same store as the migrating install.*peers after their siblings, not on arrival order #37713 makes*peers version-bound too, after which this rebind covers them as well; install: let the hoister bind optional peers when loading bun.lock #37925 changes how optional peers are bound on load, which this change does not touch.Background
bun.lockrecords packages as a hoistednode_modulestree: each entry is keyed by the path a package would have in a hoisted install.Lockfile::resolvebuilds that tree (buffers.trees/buffers.hoisted_dependencies) from the in-memory graph, andsave_from_binaryprints it. A package that ends up in no tree node is not in the file.Tree::hoist_dependency). For regular dependencies a different version is placed nested instead, which is why only peer targets can disappear from the print.bun.lockhas no field for what a peer edge resolved to. Since install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182 the loader binds required, non-*peer edges by version: it scans the packages of that name present in the file, highest first, and takes the first one satisfying the range (resolve_peer_dep_version_based). This mirrors the fresh resolver, and the pnpm migration calls the same function, but both only agree with a later reload if every package they consider is actually printed.node_modules/.bun) keys a store entry asname@version+<hash of the peer packages it resolved>, and a package's peer set includes the peers of its dependencies that nothing below it satisfies. Changing one peer binding therefore re-keys the entry of every package the peer leaks through, which is how one edge becomes 24 relinked packages.clean_with_loggerrebuilds the lockfile with only the packages reachable from the root before installing and saving. Required peer edges count as reachability (a peer bun auto-installed has no other edge), which is why the dropped target survives the clean on the install that wrote the file, and why rebinding the edge is what lets the next clean remove it.Resolution diff on shadcn-ui/ui between the migrating install and the reload
Dumping every
package :: dependency -> resolutionedge right before the isolated store is built on both installs and diffing gives a single line:pnpm-lock.yamlhashono@4.11.7as a dependency of@modelcontextprotocol/sdk@1.26.0andhono@4.12.23only as the peer resolution in@hono/node-server@1.19.9(hono@4.12.23), the variant reached from@modelcontextprotocol/sdk@1.25.3, which eslint depends on. The migratedbun.lockcontains onlyhono@4.11.7, before and after this change.Store entries added by the second install before the change (28): two extra
eslint@9.26.0+...variants,@typescript-eslint/{eslint-plugin,parser,type-utils,utils}at 8.46.2 and 8.49.0/8.54.0,eslint-config-next@15.5.11and@16.0.0,eslint-plugin-import,eslint-import-resolver-typescript,eslint-module-utils,eslint-plugin-react,eslint-plugin-react-hooks@5.2.0and@7.0.1,eslint-plugin-jsx-a11y,eslint-config-prettier,eslint-config-turbo,eslint-plugin-turbo,typescript-eslint,@eslint-community/eslint-utils,@modelcontextprotocol/sdk@1.25.3. All of them havehonoin their transitive peer set.