install: bind a multi-path package's edges from the bun.lock row that prints the binding - #39347
install: bind a multi-path package's edges from the bun.lock row that prints the binding#39347robobun wants to merge 1 commit into
Conversation
…he binding A package printed at several paths in bun.lock has one set of edges, which the loader bound once per printed row, each row overwriting the previous one. Rows can find different copies of a peer, so the binding depended on which row the writer happened to print last: a `*` peer whose own target is printed at the root from the root-level row was rebound to whatever version a deeper row walked into, and a copy recorded next to one of the rows was only read back when that row came last. find_resolution now reports where it found the entry, and the package rows loop ranks each row's find: an entry in the package's own path, then the root entry (for edges the hoister keeps, which is the only other place it puts an edge's own target), then anything walked into. A row only overwrites a binding with at least as strong a find, so equal finds keep the last row as before, and a package printed once is unaffected. Optional peers rank every walked find the same, since the hoister rebinds them to whatever each placement dedupes onto.
|
Warning Review limit reached
Next review available in: 48 seconds Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (2)
Comment |
|
Status: ready for review. Reproduced with the released build (1.4.0-canary.1) on the two shapes now in The CodeRabbit comment above is a rate-limit notice, nothing to address in it. |
![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. The change is small and well-argued, but it adjusts how the bun.lock loader picks which row binds a deduped package's peer edges — a subtle semantics change whose correctness rests on the stated invariants of Tree::hoist_dependency and the optional-peer Rebind path. A maintainer familiar with the hoister should confirm the ranking (own-path > root > enclosing, with optional peers demoting root to walked) matches those invariants.
What was reviewed:
find_resolution_impl's newFoundAttracking —own_pathis set only on the first probe andoffset == 0correctly identifies the top-level entry; the two otherfind_resolutioncallers only inspectErr, so the widened return type is harmless.bound_bysizing and indexing — sized todependencies.len(), indexed bydep_idwhich is always in range;Unboundas the lowest rank means single-row packages behave exactly as before.- The derived
OrdonRowEvidence— variant order givesUnbound < Walked < WalkedToRoot < OwnEntry, and equal ranks keep last-row-wins as described. - Tests: the end-to-end
*-peer case, the both-orders own-entry case, and the optional-peer case that pins whyFoundAt::Rootmust rank asWalkedfor optional peers.
Extended reasoning...
Overview
The PR changes src/install/lockfile/bun.lock.rs (~60 net lines) and test/cli/install/bun-lock.test.ts (~190 net new test lines plus a refactor hoisting serveRegistry/installWithOwnCache to module scope). PkgMap::find_resolution now returns where the hit was found (FoundAt::OwnPath/EnclosingPath/Root), and the for row in pkg_rows loop in parse_into_binary_lockfile keeps a per-edge RowEvidence so that when the same package's edges are bound from several printed rows, a later row only overwrites the binding if its find is at least as strong. Optional peers demote Root to Walked because the hoister rebinds them per placement.
Security risks
None. This is pure lockfile-loading semantics over data bun itself wrote; no new parsing of untrusted input, no allocation on untrusted sizes, no filesystem or network changes.
Level of scrutiny
High. The lockfile loader runs on every bun install with an existing bun.lock, and a wrong ranking would silently rebind peer edges to a different package version — showing up as spurious --frozen-lockfile rejections or, with the isolated linker, packages linked against the wrong peer. The PR description's correctness argument leans entirely on how Tree::hoist_dependency places the package an edge is bound to (own path or root, never an enclosing path in between) and on HoistDependencyResult::Rebind for optional peers. That reasoning reads correctly to me and the third test pins the optional-peer carve-out, but someone who owns the hoister should confirm it.
Other factors
- Mechanically the change is clean: the two other
find_resolutioncall sites (lines ~1134, ~1376) only pattern-matchErr, so the tuple return type doesn't touch them;bound_byis sized to the fulldependenciesslice and indexed by the samedep_idused elsewhere; the#[derive(Ord)]variant ordering matches the documented ranking. - The test refactor is a pure hoist of existing helpers; the moved
serveRegistryandinstallWithOwnCacheare byte-equivalent to the previous nested versions modulo themanifestsparameter. - The new tests cover both parent orderings for the own-entry case (the failing order and the previously-passing one), the end-to-end
*-peer shape against a real registry, and the negative case that would regress if optional peers usedWalkedToRoot. The PR description also lists a broad set of install suites re-run under the debug build. - The acknowledged not-covered case (a
*peer of a package printed both inside and outside a bundled dependency) keeps the old last-row-wins behavior, which is no worse than main.
|
For whoever confirms the two invariants the review above points at, the code they come from (line numbers as of aec33f5, which this branch does not touch):
The bundled-subtree case is the one mentioned in the PR body as not covered: for the edges this change ranks, the walk is unbounded, so a find at a bundle root ranks as |
1 participant
Problem
bun.lockhas one set of edges, butparse_into_binary_lockfilebinds them once per printed row and every row overwrites the previous one (src/install/lockfile/bun.lock.rs, thefor row in pkg_rowsloop:pkg_map.getreturns the same package for each row becauseappend_package_dedupededupes equal resolutions, andmap_dep_to_pkgassigns unconditionally). Rows walk up from different paths, so for a peer they can find different copies, and the edge ended up on whichever row the writer printed last (rows are printed by depth, then path).*peer:bun installof a project where the peer's dependent is hoisted to the root and also nested under a parent writes a file with the edge's own target at the root (peer-target= 1.0.0, held by nothing else) and the parent's other version next to the nested printing. Loading binds the edge from the root row to 1.0.0, then from the deeper row to 1.1.0. Nothing holds 1.0.0 any more, sobun install --frozen-lockfileon the file bun just wrote fails witherror: lockfile had changes, but lockfile is frozen, and a plain install rewrites it (root entry flipped to 1.1.0, nested entry dropped). With the isolated linker a reinstall relinks the root-level dependent to the store entry keyed by the deeper copy (star-peer@1.0.0+<hash>changes) instead of the one the writing install linked.<parent>/dup/no-deps, what the hoister prints when it cannot dedupe the edge onto anything above): read back only when that parent sorts after the other one; otherwise the other row's walk to the root's copy wins and the re-save drops the record. On main this shape needs an optional peer (ranged required peers are bound by version); install: keep a peer nothing in bun.lock satisfies where the file records it #38892 makes it the normal shape for a required peer nothing in the file satisfies, and its "two paths" test pins the one order that works.Fix
PkgMap::find_resolutionalso returns where the hit was (FoundAt: the package's own path, an enclosing path, or the top-level entry). The package rows loop keeps a per-edgeRowEvidenceand a row only overwrites a binding when its find ranks at least as high: own entry > top-level entry (for edges other than optional peers) > anything else. Equal ranks keep the last row, so a package printed once binds exactly as before; edges bound by version (resolve_peer_dep_version_based) are unchanged, every row writes the same id.Tree::hoist_dependencyputs the package an edge is bound to in exactly two places, nested in the dependent's own path (a copy above blocks it) or at the root (nothing above holds the name). A copy at an enclosing path in between was placed for some other package's edge, and a row only walks into it because the edge deduped onto it (a peer the copy satisfies), which says nothing about the binding. So when rows disagree, the own entry is the binding and the root entry is the only walked copy that can be; a file whose rows agree is unaffected whatever the ranks. A file that was stable in every order before is still stable: the pick is always one of the rows' finds.HoistDependencyResult::Rebind, install: keep optional peer bindings when cleaning the lockfile #37426), both in the hoist at the end of the load and again in the save. Preferring the root copy for them makes the load's tree differ from the save's in a file a fresh install writes (root holds an out-of-range copy, the last-sorted parent holds an in-range one): the load dedupes the first placement onto the root copy, the second placement rebinds the edge, and the save then nests the rebound copy under the first placement, so--frozen-lockfilefails. Checked by building that variant; the third test below pins it. Own entries still win for optional peers: an entry in the dependent's own path is never a copy another placement walks into.*peer of a package printed both inside a bundled dependency and outside it. The hoist root of a bundled subtree is the bundle root, which the unbounded walk those edges use does not tell apart from an enclosing path, so such a find keeps ranking as a walk, i.e. last row wins as before.test/cli/install/bun-lock.test.ts,describe("loading bun.lock binds the edges of a package printed at several paths"): the*shape end to end against an in-process registry (fresh install, remove the root dependency on the peer,--frozen-lockfilemust accept the file bun wrote and install 1.0.0 at the root and 1.1.0 next tomid); the recorded copy under either parent, hand-written (both orders must re-save to the same entries, pass--frozen-lockfile --dry-runand be a fixed point); and an optional peer with nothing recorded (must keep passing--frozen-lockfile --dry-run). With the released build the first fails on the frozen install and the record-under-a-parentcase fails (record dropped); the other two pass by construction. The unmet-peer tests' registry helper moved to module scope for the first test, otherwise unchanged.cargo clippy -p bun_installandrustfmt --checkclean.Background
bun.locklists packages by their path in the hoistednode_moduleslayout ("a","a/b"). A package is printed at several paths when another version of it holds the slot above some of its dependents: each such dependent gets the package printed again next to it, but the loader dedupes the rows onto one package, which has one dependency list. The file has no field for what an edge resolved to; loading a non-workspace package's edge probes<row path>/<name>, then each enclosing path, then<name>at the top level (find_resolution), except for required peers with a range, which are bound by version (install: fix stale global-store links on disable and peer edge drift across bun.lock loads #32182).Tree::hoist_dependencybuilds the tree, a peer dedupes onto a copy found above it when the copy satisfies the range (or is a root dependency); only when it cannot is the package the edge is bound to printed next to the dependent, and when no copy exists above at all it is printed at the root. For an optional peer the dedupe also moves the binding to that copy (Rebind, install: keep optional peer bindings when cleaning the lockfile #37426); required peers keep their binding through hoists and through the clean that precedes a save (Package::clone), and a required peer edge keeps its target in the file even when nothing else depends on it, which is how a version ends up held only by such an edge after the dependency that brought it in leavespackage.json.--frozen-lockfilebuilds the tree from the loaded bindings, builds it again from a cleaned copy of the lockfile, and rejects the file when the two trees differ (Lockfile::eql);--lockfile-onlyprints the second tree.--frozen-lockfile --dry-runperforms that comparison without fetching anything, which is what the hand-written tests use.