install: resolve a range onto an existing version only when every install has it by robobun · Pull Request #38832 · oven-sh/bun

robobun · 2026-08-15T02:37:03Z

Problem

A fresh bun install (no bun.lock) of {"a":"1.0.0","b":"1.0.0"} where a depends on z@1.0.0 and b on z@^1.0.0 (registry has z@1.0.0 and z@1.1.0) writes one of two lockfiles: z@1.0.0 alone, or z@1.0.0 plus b/z@1.1.0. Roughly a 1:2 split over 30 clean installs; same on 1.3.14 and with --linker isolated. Found by the resolver property fuzzer (fresh-install determinism), which hit it in about a third of generated graphs.
Cause: Lockfile::get_package_id (src/install/lockfile.rs) resolves a range onto a lower version of the package that already exists in the lockfile when that version was appended for an exact pin, or is of the same major as the range's best match. Whether a's z@1.0.0 exists yet when b's row is resolved depends on whose manifest the registry answered first. Holding a's manifest back forces the two-copy lockfile every time, holding b's forces the one-copy lockfile every time.
The same thing happens with two transitive ranges of one major (~1.0.0 next to ^1.0.0), and in the other direction: a range that the root's pin satisfies was instead resolved onto its best match whenever some sibling had appended that version first.
The Rust port's guard in this function (exact_pinned) already stopped the cross-major variant of this; the exact-pin and same-major exemptions it kept are the remaining order-dependent cases.
The exact-pin record itself has the same problem: exact_pinned was set by whichever row happened to append the version. With a -> z@1.1.0 next to b -> z@^1.0.0 (both 1.1.0), a range from another major resolved later (for example z@* in the subtree of a peer, which is installed after the regular rows) was absorbed onto 1.1.0 or not depending on whether a's or b's manifest came first.

Fix

get_package_id settles a range onto an existing lower version only when that version is present on every install, whatever order manifests arrive in:
- loaded from the lockfile (as before),
- appended for a root or workspace row (AppendedFor::direct, recorded per appended package by mark_appended_for), or
- appended before resolution last drained completely (settled_package_count, bumped by mark_settled_packages in wait_for_resolution once every regular row is resolved, before peers are installed).
The pin record (AppendedFor::pinned) is no longer just the appending row's: an exact regular row that resolves to a version which is not reusable yet records the pin too (mark_pinned_by_reuse), and the record is frozen once the version becomes reusable, because from then on rows are reading it. So by the time anything reads it, it says whether any regular row pinned that version, which is a property of the inputs. Peer rows do not count, whether they reuse or append, since whether a peer binds on sight or in the peer pass, and which of several peers on one version gets to append it, depend on arrival order. The one already-deterministic outcome this moves: a version appended in the peer pass for a root or workspace exact peerDependencies row (which is reusable right away, being direct) still absorbs same-major ranges from later peer-pass subtrees, but no longer cross-major ones.
The pinned / same-major policy for the versions that do qualify is otherwise the existing one, so outcomes that were already order independent (root pin absorbing a transitive range, root range absorbing a same-major transitive range, cross-major refusal, peer subtrees settling onto regular rows) are unchanged. Only the previously order-dependent cases change, and they now always get the result they already got whenever the range was processed first: each transitive range resolves to its own best match. bun dedupe still collapses such copies afterwards when the user wants that.
Qualifying versions are also preferred over a version some other transitive row appended even when the latter is the range's own best match (the third case above); the best match is otherwise only reused when it is exactly the version the range would append anyway, so the answer no longer depends on what a sibling did first.
Why this is order independent: direct rows are enqueued before any manifest is processed (workspaces sort first in the root's list), and each manifest's waiting rows are resolved FIFO, so for a given package name the direct rows always resolve before any transitive row; the lockfile's packages are fixed before resolution starts; and by the time mark_settled_packages runs nothing is in flight, so the set present at that point is itself a function of the inputs only.
Verified:
- test/cli/install/bun-lock.test.ts: ten graph shapes, each installed under arrival orders forced by holding one manifest until a request that can only follow the other parent's resolution (the tarball of what it resolved to). The four shapes above fail on main (two different lockfiles); the six preserved-behavior shapes pass on main and still pass, one of them pinning that an exact row landing on the root's range-resolved version does not change what later rows get (which is what freezing the record is for); all ten pass with the fix.
- bun-install-registry.test.ts 242/242 (the peer-subtree snapshot duplicate dependency in optionalDependencies maintains sort order is what the settled watermark keeps unchanged), bun-lock, bun-update-transitive, bun-dedupe, hoist, lockfile-only, isolated-install, bun-workspaces, catalogs, overrides, nested-overrides, test-dev-peer-dependency-priority, bun-update-lockfile-sync, bun-add, bun-update, minimum-release-age, bun-remove, bun-prune, bun-pm-why, the autoinstall tests and the migration suites pass with the debug build. bun-install.test.ts and migration/complex-workspace only fail their bitbucket/gitlab/off-box tests, which need network here.
Related: install: dedupe a transitive wide range onto a root/workspace range across majors #34336 and install: keep one copy of a package when one version satisfies every range #38770 extend which direct versions a range may settle on (cross-major); with this structure that is the pinned clause in get_package_id, so they compose, and whichever lands second rebases one hunk. install: resolve deferred * peers after their siblings, not on arrival order #37713 changes when * peers bind early; under this change that early binding only sees qualifying versions, so the two are independent. Deterministic dependency resolution: an ordered walk over a live tree #36476 (draft) replaces the resolver wholesale; this is the small fix for the property in the meantime.

Background

Resolution is arrival driven. The root's rows (and, through them, each workspace's rows) are enqueued first; every package that resolves pushes its own row list onto a queue that is drained afterwards; a row whose manifest is not in memory yet is put on that manifest's callback list and resolved, in list order, when the response lands. Regular rows are all resolved first; peers that were not bindable on sight are installed in a second pass.
get_package_id(name, range, best_match) is the step that decides whether a row reuses a package already in the lockfile or appends best_match (the highest version in the manifest that the range accepts). Reusing a version the range merely satisfies is what keeps one copy of a package in the tree; this PR only changes which existing versions are eligible for that.
loaded_package_count is the existing watermark separating packages loaded from bun.lock from ones appended in this run; settled_package_count is the same idea taken again once resolution has drained. is_workspace_dependency is the existing "is this row declared by the root or a workspace" predicate. is_reusable combines the two watermarks with the direct flag; "reusable" throughout means "a range that merely satisfies this version may resolve to it".

Probe: 10 shapes x forced arrival orders, main vs this branch

main (1.4.0 release and debug build of 4bf3f3645):
NONDETERMINISTIC (2 distinct lockfiles)   transitive exact pin vs transitive range (reported)
    a last: z=z@1.0.0, b/z=z@1.1.0
    b last: z=z@1.0.0
NONDETERMINISTIC (2 distinct lockfiles)   transitive same-major ranges
    a last: z=z@1.0.5, b/z=z@1.1.0
    b last: z=z@1.0.5
NONDETERMINISTIC (2 distinct lockfiles)   root pin wins over a sibling that appends the range's best match
    b last: z=z@1.0.0, b/z=z@1.1.0, c/z=z@1.1.0
    c last: z=z@1.0.0, c/z=z@1.1.0
deterministic   root exact pin still absorbs a transitive range            {"z":"z@1.0.0"}
deterministic   root same-major range still absorbs a transitive range     {"z":"z@1.0.5"}
deterministic   root range does not absorb a transitive range, other major {"z":"z@1.1.0","c/z":"z@2.0.0"}
deterministic   workspace exact pin absorbs a sibling workspace's star      {"z":"z@1.0.0"}
deterministic   workspace pin vs transitive range                          {"z":"z@1.0.0"}
deterministic   peer star between two transitive pins                      {"z":"z@1.0.0","b/z":"z@1.1.0"}
deterministic   peer range between two transitive pins                     {"z":"z@1.0.0","b/z":"z@1.0.5"}

this branch: all 10 deterministic; the 7 shapes that were deterministic on main
produce the same lockfile as on main, and the first 3 now always produce
    {"z":"z@1.0.0","b/z":"z@1.1.0"}
    {"z":"z@1.0.5","b/z":"z@1.1.0"}
    {"z":"z@1.0.0","c/z":"z@1.1.0"}

no test proof · iteration 1 · 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

…tall has it A fresh `bun install` resolved a transitive range onto whatever lower version of the package happened to be in the lockfile already, if that version was an exact pin or of the same major. Whether such a version exists when the range is resolved depends on which parent's manifest the registry returned first, so the same package.json produced different lockfiles from one install to the next. `get_package_id` now settles a range onto an existing lower version only when that version is present on every install regardless of arrival order: loaded from the lockfile, appended for a root or workspace row (those are always resolved before transitive rows of the same name), or appended before resolution last drained completely (the regular rows are all resolved before peers are installed). Such a version is also preferred over a version some other transitive row appended even when the latter is the range's own best match, since its existence is equally order dependent. Everything else resolves to its best match from the manifest, which is what the range resolved to whenever it was processed first. The exact-pin / same-major policy for the versions that do qualify is unchanged, so every outcome that was already order independent stays the same.

coderabbitai · 2026-08-15T02:37:12Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 16c3358e-9fd1-4a9c-9fe1-0faeb0b6e88a

📥 Commits

Reviewing files that changed from the base of the PR and between e9b5e63 and 16ad612.

📒 Files selected for processing (4)

src/install/PackageManager/PackageManagerEnqueue.rs
src/install/PackageManager/install_with_manager.rs
src/install/lockfile.rs
test/cli/install/bun-lock.test.ts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-08-15T02:37:14Z

^{Updated 11:05 PM PT - Aug 14th, 2026}

❌ @robobun, your commit 16ad612 has some failures in Build #97367 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 38832

That installs a local version of the PR into your bun-38832 executable, so you can run:

bun-38832 --bun

robobun · 2026-08-15T02:37:58Z

Status: reproduced with a mock registry that holds one packument back until the other parent's tarball is requested (a last gives z@1.0.0 + b/z@1.1.0, b last gives z@1.0.0 alone; same on the 1.4.0 release build and on main's debug build, hoisted and isolated). With this branch both orders write the same lockfile.

Current state (16ad612): both review findings are in (0d28e11, 16ad612), every review thread is answered, and the latest review pass reported nothing further. test/cli/install/bun-lock.test.ts has ten forced-order shapes, four of which fail on main.

CI for 16ad612 (build 97367): every lane that ran passed (177 jobs, no test failures); the build is marked failed only because its two darwin 14 aarch64 - test-bun jobs expired in the queue before an agent picked them up, which is currently happening to most builds on that lane (32 of the last 40 finished builds). Nothing in this diff is platform specific; the install suites passed on the other darwin lane. Those two jobs can be retried once the lane has capacity. The remaining decision is the policy direction (a transitive range gets its own best match unless a version every install has satisfies it; bun dedupe collapses the rest), which needs a maintainer.

… reusable Whether a version appended for a transitive row was recorded as pinned depended on which of the rows resolving to it happened to append it. Rows resolved after the version settles read that bit, so an exact row and a range row landing on the same version still gave two different answers to a later range from another major depending on arrival order. An exact regular row that resolves to an already-present version now records the pin as well, as long as the version is not reusable yet; once it is, rows are reading the bit and it stays as it is. Peer rows do not count, since whether they bind on sight or in the peer pass depends on arrival order.

The reuse path already skipped peer rows; the append path recorded them, so the pin on a version appended in the peer pass depended on which peer row got to it first.

claude

16ad612 addresses the peer-append asymmetry from my earlier note — pins now gates !behavior.is_peer() on both the reuse and append paths, so a peer row never stamps .pinned. No further issues found on this pass. Because this rewrites get_package_id's reuse rule and adds new resolver-phase state (settled_package_count, AppendedFor), a human look at the resolver semantics would still be worthwhile.

Checked: the is_reusable invariant against is_workspace_dependency and the FIFO ordering claim; that mark_settled_packages in wait_for_resolution runs between the regular drain and the peer pass (and again on the second wait_for_resolution in the --latest path); that the .eql() fallback still covers non-npm resolutions and should_update/suppress_peer_satisfies callers passing None; and that mark_pinned_by_reuse's freeze-once-reusable rules out the plain-OR regression the eighth test shape pins.

Extended reasoning...

Overview

This PR changes how Lockfile::get_package_id decides whether an npm range may resolve onto a version already in the lockfile, replacing the prior exact_pinned bitset with per-package AppendedFor { direct, pinned } state plus a settled_package_count watermark bumped between the regular and peer resolution passes. PackageManagerEnqueue.rs records pins on both the append and reuse paths (now gated on !behavior.is_peer() after 16ad612), install_with_manager.rs calls mark_settled_packages() in wait_for_resolution, and ~250 lines of new tests in bun-lock.test.ts force manifest arrival orders via a gated in-process registry across ten graph shapes.

Security risks

None identified. This is version-selection logic inside the resolver; no new inputs are parsed, no filesystem paths are derived from untrusted data, and the test registry is local (port: 0).

Level of scrutiny

High. get_package_id is on the hot path of every bun install and its output determines what ends up in bun.lock and node_modules. The change is a semantic rewrite of the reuse predicate with a subtle order-independence argument, and two earlier review passes each surfaced a real ordering hole (the .exact reuse gap fixed in 0d28e11, and the peer-append asymmetry fixed in 16ad612). The PR description also notes it will need a one-hunk rebase against #34336/#38770 and interacts with #37713 and the #36476 resolver rewrite — a maintainer familiar with those should confirm the intended reuse policy (each transitive range gets its own best match, bun dedupe collapses afterward) is the direction they want.

Other factors

The two prior findings from this reviewer were both addressed with follow-up commits and matching test shapes (shape 4 for the 🔴, shape 8 for the freeze rule ruling out a plain OR). The comment-cop bot still flags two doc comments on the latest push; the author has responded that these carry the invariant rather than justify a workaround, which seems reasonable but is a style call for a human. CI for 16ad612 is building (#97367). The test harness looks sound: it awaits a tarball request that can only follow the other parent's resolution, so the forced ordering is a real happens-before rather than a sleep.

robobun assigned Jarred-Sumner Aug 15, 2026

github-actions Bot added the claude label Aug 15, 2026

ci: retrigger

209a429