install: re-resolve a peer row instead of binding it to a stale bun.lock leftover - #38901
install: re-resolve a peer row instead of binding it to a stale bun.lock leftover#38901robobun wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 9 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 (2)
Comment |
|
Status: ready for review (rebased onto main after #38851 landed; latest revision e2c732a). Reproduced on main (0def731) with a loopback registry serving peer-q 1.0.0 / 1.1.0 / 2.0.0 and a root CI: build 97953 (e2c732a) has every lane green except jobs that failed once and passed on retry (hook timeouts in two install files, a shell pipe test, napi node-gyp builds on Windows aarch64, cluster tests), none touching peer resolution; the Windows bunx hang from the first run is gone. Nothing further planned from my side; ready for a maintainer. |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither of those is closed by this PR, so I am not adding them to the description.
|
![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}
…ile leftover In the deferred peer pass, a peer row whose range no package of its name satisfies is bound to the highest same-kind package anyway, with the "incorrect peer dependency" warning. When that package was loaded from bun.lock and no non-peer row resolves to it any more, it is only there because a peer row previously installed it; binding the row back to it keeps the stale version and then, on the named update path, narrows the declared range down to it. Leave such packages out of the fallback so the row resolves afresh. Packages appended during the current resolve and packages some non-peer row still resolves to are bound as before. This is what `bun update -i`, `bun update <name>@<version>`, and a hand edit of a peerDependencies range followed by `bun install` run into, as well as a package whose replacement declares a different peer range.
…keep the highest-candidate fallback Only a root or workspace peerDependencies entry passes over the leftover it installed: a package's own peer rows take whatever copy the tree has, since a copy resolved for them alone is never placed, so for them the warning stays. The fallback stays the highest candidate or nothing, matching how loading bun.lock binds, instead of moving down to a lower candidate. When the entry passed over its leftover but nothing published matches its new range, bind the leftover after all rather than returning the "manifest not loaded" answer, which spins on a warm manifest cache and otherwise leaves the row unresolved.
fd4260f to
8510010
Compare
|
Pushed a reworked version (description updated to match):
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/install/PackageManager/PackageManagerEnqueue.rs:2482-2493— The nothing-published fallback binds tohighest_peer_candidateat the moment this row is drained, but a later workspace peer row on the same name can still hitwould_revive_leftover→ resolve afresh → append a higher version, so the bound id is no longercandidates.first()in the saved lockfile andresolve_peer_dep_version_basedflips the row on reload (one-time bun.lock rewrite /--frozen-lockfilefailure). This is the residual case of the resolver/loader-agreement class 09c526f fixed forexisting_peer_target; consider deferring the row until the other workspace peer rows onname_hashhave drained, or re-visiting rows bound via this fallback once the peer pass is done.Extended reasoning...
What the bug is
existing_peer_target's doc comment (line 2815-2818) says "the fallback has to stay the highest candidate or nothing" so thatresolve_peer_dep_version_based— which falls back tocandidates.first()unconditionally at bun.lock.rs:3402 — reproduces the resolver's choice on load. Commit 09c526f enforced that forexisting_peer_targetitself (the fallback is now the highest candidate orNone, never a lower one). But the second fallback added at line 2482-2493 ("Nothing published to resolve afresh: the entry that passed over its leftover keeps it") binds tohighest_peer_candidateat the moment that row is processed, and a later-drained workspace peer row on the same name can still resolve afresh and append a higher version — so what was highest at line 2483 is no longercandidates.first()in the saved lockfile.Why the later row still resolves afresh
would_revive_leftover(line 2917) filters out peer rows via!dep.behavior.is_peer(). So after the unpublished-range row binds the leftover viabind_existing_peer(false)at line 2486, a subsequent workspace peer row on the same name still sees the leftover as reached only by peer rows →would_revive_leftoverreturns true →existing_peer_targetreturnsNone→ the row falls through to the manifest and appends a fresh, higher version.Step-by-step proof
Setup: root has
peerDependencies: { react: "15.0.0" }(a yanked/typo'd version — nothing published matches). Workspace member W haspeerDependencies: { react: "^18" }. bun.lock carries a peer-only leftoverreact@17.0.2(installed for one of these rows in a previous run). Root's row is enqueued intopeer_dependenciesbefore W's.- Root's row drained.
existing_peer_target: 17.0.2 doesn't satisfy15.0.0; fallback highest is 17.0.2;would_revive_leftover(17.0.2)→ true (workspace row ✓,id < loaded_package_count✓, only peer rows resolve to it) → returnsNone. Manifest:find_best_version_with_filter("15.0.0")→NotFound. Line 2483:highest_peer_candidate→ 17.0.2 (only entry inpackage_index[react]at this moment).bind_existing_peer(false)→ warn,resolutions[root_row] = 17.0.2. - W's row drained.
existing_peer_target: 17.0.2 doesn't satisfy^18;would_revive_leftover(17.0.2)is still true — root's row is a peer row, filtered out by!dep.behavior.is_peer()at line 2917 → returnsNone. Manifest →react@18.xappended.resolutions[W_row] = 18.x. cleanand save. Root → 17.0.2 (peer), root → W → 18.x (peer). Both kept. Saved bun.lock:package_index[react] = [18.x, 17.0.2], highest first.- Reload.
resolve_peer_dep_version_basedfor root's15.0.0: neither 18.x nor 17.0.2 satisfies → fallback tocandidates.first() = 18.x. Root's row flips from 17.0.2 (resolver) to 18.x (loader). On the nextbun install,cleanfinds 17.0.2 unreachable and drops it; bun.lock is rewritten on a no-op install and--frozen-lockfilefails once.
Why not already prevented
The updated doc comment at bun.lock.rs:3320-3325 argues the divergence case "makes the edge resolve afresh to a package its range accepts, which is what gets saved and what the satisfies scan above finds." That covers the case where the row did resolve to a satisfying package. Line 2486 is precisely the case where it did not (nothing published matches), and the leftover it was bound back to is no longer
candidates.first()after step 2. Before this PR the same tree bound both rows to 17.0.2 (neither could skip the leftover), so only onereactwas saved and the loader trivially agreed — the divergence is new.Impact and severity
This is the residual case of the resolver/loader-agreement class the earlier review comment flagged and 09c526f fixed for
existing_peer_target, so per REVIEW.md's "fix the whole class in the same PR" it belongs here. That said, the trigger is narrow — a root/workspace peer entry whose range matches nothing published (typo/yanked), plus another root/workspace peer on the same name resolving higher than the leftover, plus a peer-only leftover in bun.lock, plus the unpublished row FIFO'd first — and the symptom is a one-time lockfile rewrite that then converges (arguably to a better state than pre-PR, where both rows were stuck on 17.0.2). Marking as a nit: worth addressing alongside the class, not worth blocking the fix for the much more common bug.Possible fix
At line 2486, only take the fallback when the highest cannot later be superseded — e.g. defer the row (re-
write_itemtopeer_dependencies) until every other workspace peer row onname_hashhas drained, or after the peer pass, re-visit rows bound via this fallback and rebind them to the then-currenthighest_peer_candidate. - Root's row drained.
…esolving afresh The tree dedupes every other peer row onto the copy a root row holds, whatever their ranges say, so a workspace entry that resolved its own copy would have it dropped when the tree is written and lose the warning with it. Count the root's rows, peer or not, as holding the copy; the entry then binds to it and warns as before. Covers the root-plus-member, root-and-member-rewritten, and members-only shapes.
…nly NotFound A version blocked by --minimum-release-age took the TooRecent arm before the NotFound site rebound the copy, turning the warn-and-keep of main into an error that dropped the package. Bind it once where the manifest lookup fails, ahead of both arms; the NotFound site goes back to what main has.
|
On the additional finding (unpublished root range plus a member entry on the same name): I ran that shape and a few neighbours. The real problem it uncovered was a step earlier than the reload: the member's copy was being resolved and then dropped when the tree was written, because What is left of the resolver/loader disagreement is an entry that nothing satisfies whose copy is outranked by another copy in the file. That is independent of this change: on main today, |
There was a problem hiding this comment.
I reviewed the latest revision (e2c732a) and found no new issues — the added test pins the root-edited/member-unchanged case from my earlier note, and the FindVersionResult::Err rebinding now covers both NotFound and TooRecent. Because this reworks the deferred-peer binding path and its agreement with resolve_peer_dep_version_based in the loader, a human look at the would_revive_leftover predicate and its interaction with Tree::hoist_dependency would still be worthwhile.
What was reviewed:
existing_peer_target/highest_peer_candidate/bind_existing_peeragainst the removedId/Idsarms — same satisfies scan, same tag check, same warning text; the only behavioural delta is thewould_revive_leftovergate.- The
FindVersionResult::Errrebinding runs before theTooRecentreturn and before theNotFoundfall-through, so both shapes keep main's warn-and-keep. would_revive_leftover'sowner == 0 || !is_peer()predicate againstTree::hoist_dependency's root-dedupe and the loader's highest-first fallback — the remaining resolver/loader disagreement is the pre-existing one #38892 addresses.resolution_satisfies_dependencyhad no other callers.
Extended reasoning...
Overview
The PR changes get_or_put_resolved_package's install_peer branch in src/install/PackageManager/PackageManagerEnqueue.rs: the duplicated Id/Ids match arms (~100 lines) are replaced by four helpers — existing_peer_target (satisfies scan then highest-or-nothing fallback), highest_peer_candidate (same-kind tag check on candidates[0]), bind_existing_peer (the warning + success_fn call, shared with the manifest-miss site), and would_revive_leftover (the new predicate: root/workspace row, id < loaded_package_count, no non-peer or root-owned row resolves to it). The FindVersionResult::Err arm now rebinds the passed-over leftover before returning either error. resolution_satisfies_dependency is deleted (no other callers). ~260 lines of tests in bun-update-transitive.test.ts cover the fixed paths and pin the shapes that stay as on main.
Security risks
None identified. This is dependency-resolution bookkeeping over data already parsed into the lockfile; no new parsing of untrusted input, no filesystem path handling, no network surface beyond what bun install already reaches.
Level of scrutiny
High. Peer resolution in bun install is a correctness-critical path where a wrong binding silently installs the wrong version or diverges from what --frozen-lockfile reproduces. Two earlier revisions of this PR each had a 🔴 finding (fallback-vs-loader disagreement; TooRecent bypassing the rebind), both fixed. The would_revive_leftover predicate is a heuristic over the whole buffers.resolutions table whose correctness depends on drain order (root before members), on Tree::hoist_dependency's root-dedupe rule, and on the loader binding the same highest candidate — invariants that live in three other files. That is exactly the shape a maintainer familiar with the resolver should sign off on.
Other factors
- The author addressed all three prior findings with code + pinning tests, and the PR description traces each design choice (highest-or-nothing, root-row block, dependency-owned peers left alone, #38892 handling the remaining loader gap) to a concrete failure mode.
- The one open note (🟡, root edited / member unchanged loses the warning for one install) is diagnostic-only, the on-disk result is better than main's, and the fix belongs at the pre-existing
Tree.rsTODO; the new test pins current behaviour without asserting stderr. - Test coverage is thorough (14 new cases, each ending in
--frozen-lockfileto check loader agreement), but the resolver/loader/tree interaction is subtle enough that I'm not confident a human reviewer wouldn't want to weigh in on the predicate's exact shape.
2 participants
Problem
bun update -ioffers a rootpeerDependenciesrow (peer-q peer 1.1.0 1.1.0 2.0.0, the Latest column is what selecting it promises). Enter then printswarn: incorrect peer dependency "peer-q@1.1.0", summarises(no changes), leaves bun.lock and node_modules on 1.1.0, and rewrites package.json from^1.0.0to^1.1.0. Same for a workspace row under-i -r, and for the in-range case (locked 1.0.0, Target 1.1.0: nothing moves, the warning prints, the range is written back as it was).-i:bun update peer-q@2.0.0on the same tree warns and writes^1.1.0,bun add peer-q@2.0.0warns and keeps 1.1.0 (fix(install): Resolve the requested version whenbun addtargets a peer dependency #30855's report), and editing the range to^2.0.0by hand and runningbun installkeeps 1.1.0 and warns.--latestworks only because it writes a dist-tag literal, which the fallback below never matches.get_or_put_resolved_package(src/install/PackageManager/PackageManagerEnqueue.rs, theinstall_peerbranch). In the deferred peer pass a peer row binds to a package of its name already in the lockfile; when none satisfies the range it binds to the highest one anyway and warns. With an existing bun.lock that candidate is the package this same entry installed last time: nothing else depends on it, it is only still there becausecleanhas not run yet.-iwrites the range into package.json and runsbun update <name>, the differ re-enqueues the row, the pass binds it back to the leftover, and the named path's write-back then rewrites the range to the version it ended up on.Fix
existing_peer_targetreplaces theId/Idsarms (same satisfies scan, same highest-candidate fallback, same kind check; the warning and binding move intobind_existing_peer, shared with the site below). The fallback is skipped whenwould_revive_leftover: the row is a root or workspace entry (Lockfile::is_workspace_dependency, already used by this function forfile:rows), the candidate was loaded from bun.lock (id < loaded_package_count), and the only rows still resolving to it are peer rows of workspace members or of dependencies. The row then resolves from the manifest like a peer nothing provides, the new package takes the entry's place in its node_modules, andcleandrops the leftover.devDependenciesentry next to the peer entry, a dependency's own copy), so the entry binds to it and warns as before. Any row of the root package.json, peer included, also blocks it:Tree::hoist_dependencydedupes every other peer row onto the root's copy whatever their ranges say, so a member entry that resolved its own copy would only have it dropped when the tree is written, and the warning with it (that is what the first revision did; "a member's rewritten entry keeps binding to the copy the root's own entry holds" pins it). Packages appended during the current resolve never qualify, so a fresh install binds exactly as before. Root rows are drained before member rows, so when root and member entries are rewritten together the root's new copy is there for the member to bind to.resolve_peer_dep_version_based); a row that resolved afresh is saved next to a package that satisfies it. (Raised in review; the first revision picked the first non-leftover candidate.) The remaining way resolver and loader can disagree, an entry that nothing satisfies whose copy is outranked by another copy in the file, exists on main independently of this change (it is reached today by a regular dependency pulling in a higher copy) and is what install: keep a peer nothing in bun.lock satisfies where the file records it #38892 fixes in the loader.--minimum-release-age, theFindVersionResult::Errarm in the peer pass binds the leftover after all, so both shapes keep behaving exactly as on main:warn: incorrect peer dependency, old copy kept. The first of them is what the Windows lanes oftest/cli/install/bunx.test.tsexercise:bun initinstalls the root peertypescript@^6, then the test runsbun add typescript@5.0.0, a version that was never published (5.0 shipped as 5.0.2); without the binding the first revision spun forever on a warm manifest cache (the loop install: stop looping on a peer dependency no published version satisfies #38851 has since fixed on main for the case with no copy at all; rebased onto it), and the age-gated shape turned the warning into an error. Every row that reaches that arm with this change also reached it on main.resolution_satisfies_dependencyhad no other caller and is deleted.bun update <name>/ barebun updateon an entry whose locked version still satisfies it (the satisfies scan binds it first; the "never re-resolved" tests inbun-update-transitive.test.tsstill pass), and-ion a peer row whose siblingdevDependenciesrow is left unselected.bun addtargets a peer dependency #30855 fixes thebun addinstance by skipping the fallback for every row whose name is an update request, tree-wide: afterbun add react@18, each package with an older react peer range would resolve its own react instead of binding to the root's and warning. The checks here leave those bound.test/cli/install/bun-update-transitive.test.ts. Fail on main, pass here:-iTarget row,-iLatest row,-i -rmember row,update <name>@<version>,installafter an edit, the two-copy case (entry rewritten past both its own 1.1.0 and a dependency's 1.0.0 resolves 2.0.0 instead of binding 1.0.0), root and member entries rewritten together, only the root's entry rewritten (its copy is installed and the unchanged member is deduped onto it, as for a dependency bump; the tree's missing warning there is a pre-existing TODO, filed separately, so that test does not assert on stderr), and a member entry rewritten next to a sibling's when the root declares nothing (each member gets its copy). Pass on both and pin what stays: an entry another group provides, a member entry under a root entry (binds the root's copy and warns), a dependency's own peer row with the two copies (binds the highest and warns),bun add <name>@<unpublished version>(keeps the copy and warns; times out without the rebinding), the same entry under--minimum-release-age(keeps the copy and warns; errors without it), two unprovided peers on a fresh install. Also run locally:bun-update-transitive,bun-lock(with install: stop looping on a peer dependency no published version satisfies #38851's cases),bun-update-lockfile-sync,bun-update/bun-add,bun-install-registry -t peer,bun-install,isolated-install/bun-workspaces/frozen-lockfile-pruned,bun-audit/nested-overrides/hoist/bun-dedupe/bun-prune/isolated-relink/test-dev-peer-dependency-priority,catalogs/bun-add-filter/update_interactive_*,migration/; the only failures need public network access or were 5s timeouts on a heavily loaded machine that pass alone. On Windows against registry.npmjs.org:bun init+bun add typescript@5.0.0warns and keeps 6.0.3 in under a second,bun add typescript@5.0.4installs 5.0.4, and the bunx test passes.Background
buffers.dependencies[i]) owned by a package;buffers.resolutions[i]is the package it resolves to; the root package.json is package 0.package_indexmaps a name to every package of that name in the lockfile, highest version first. On an install with an existing bun.lock all of the old packages stay in the lockfile while rows are re-resolved;Lockfile::cleanrebuilds it from the root at the end and drops whatever nothing reaches.loaded_package_countis how many packages came from bun.lock; packages resolved during the run are appended after them.peer_dependencies;wait_for_resolutionthen drains them withinstall_peer = true, which is the branch changed here. A peer nothing in the tree provides is installed from the registry at that point; root and workspace peer entries are installed this way too, which is how the leftover got there.bun update -iapplies a selection by writing the chosen ranges into package.json and then runningbun update <selected names>; that path re-resolves the named rows and writes each one's resolved version back into its range, which is where the^1.1.0came from.Probes on main (loopback registry with peer-q 1.0.0 / 1.1.0 / 2.0.0)
With the fix the first three print
^ peer-q 1.1.0 -> 2.0.0(install:+ peer-q@2.0.0), write^2.0.0, lock 2.0.0, andbun install --frozen-lockfileafterwards reports no changes. With a dev row on the same name, selecting both rows in-istill moves both to^2.0.0, selecting the dev row alone still leaves the peer entry untouched, and selecting the peer row alone still warns and keeps 1.1.0, all as on main.