install: stop looping on a peer dependency no published version satisfies - #38851
Conversation
…fies In the peer pass, get_or_put_resolved_package returned Ok(None) for a peer whose manifest has no version in range. The caller reads Ok(None) as "manifest not loaded", reloads it from the manifest cache and retries, so an install with the manifest cached never returned; with a cold cache the edge was silently left unresolved instead. Defer with Ok(None) only outside the peer pass (as the find_result variant already does) and warn for the peer in the pass itself, leaving the edge unresolved like other unmet peers. bun.lock parsing now accepts an unresolved peer edge the same way it accepts an unresolved optional edge, so the lockfile written after such an install loads again and --frozen-lockfile passes.
|
Warning Review limit reached
Next review available in: 19 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: reproduced and fixed, waiting on CI. Reproduced on main (1.4.0 canary) and on 1.3.14 with a two-package local registry ( With this branch all three go away on both linkers; the new tests in |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate, though the overlap is real and is called out in the description. #33156 is about peers provided by |
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 5e8017e has some failures in 🧪 To try this PR locally: bunx bun-pr 38851That installs a local version of the PR into your bun-38851 --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 the automated bug hunt found nothing. The fix looks correct and thoroughly tested, but since it changes peer-resolution semantics and widens what bun.lock accepts on load, a human look at the policy choice (warn + leave unresolved vs. error) would still be worthwhile.
What was reviewed:
- The new
&& !install_peerguard matches the existing deferral atget_or_put_resolved_package_with_find_result(line 2141) — consistent. warn_unmet_peer_dependencyis only reached for required peers with nofail_fn, sobun add/update paths that pass afail_fnare unaffected.may_stay_unresolvedwidens lockfile acceptance to anyPEERedge;verify_resolutionsand the tree builders already skip unresolved peers, so the parser is being brought into agreement with what the writer already emits.- Tests cover both linkers, the cached-manifest loop, and the
--frozen-lockfileround-trip for registry, root, and workspace peers.
Extended reasoning...
Overview
Four files: PackageManagerEnqueue.rs (the loop fix + new warning helper), PackageManagerResolution.rs (comment-only), bun.lock.rs (three call sites now accept unbound peer edges via may_stay_unresolved), and ~180 lines of new tests in bun-lock.test.ts.
Security risks
None identified. The change loosens what bun.lock accepts on load (any PEER-flagged edge may lack a package entry, not just OPTIONAL ones), but this only affects whether the parser rejects the file — it does not cause anything extra to be fetched or installed. The unresolved edge stays unresolved after load, as it does after a fresh resolve.
Level of scrutiny
Medium-high. The install resolver is a dense, stateful subsystem where Ok(None) carries different meanings across passes, and the fix hinges on that distinction. The core one-line change (&& !install_peer) is well-justified by the existing precedent at line 2141 in get_or_put_resolved_package_with_find_result, and the PR description traces the loop mechanism precisely. Still, this is a behavioral/policy change to how required-but-unsatisfiable peers are handled (warn vs. error), and it changes lockfile-load acceptance. That is the kind of decision a maintainer should sign off on rather than an automated approval.
Other factors
The PR description is unusually thorough — it explains why the alternative of recording the peer as optionalPeers would not be stable under Package::Diff, why erroring would break existing pinned tests, and lists the full local test matrix run. Tests are strong: inline snapshots of the lockfile, request-count assertions proving the cache path is exercised, a spawn timeout that would catch the loop regressing, and both-linker coverage. No CODEOWNERS apply to the touched paths. Deferring only because the semantics change is not mechanical.
The CI runner sets BUN_INSTALL_CACHE_DIR per test file, which takes precedence over the bunfig cache setting, so the concurrent hoisted and isolated variants shared one cache and the second one never requested the tarball.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-reasoned and the tests are thorough, but it changes install semantics in a way that warrants a human look — the lockfile parser now tolerates any unresolved peer edge (not just optional ones), and the peer-pass error path now surfaces a new user-facing warning.
What was reviewed
- Traced the
!install_peerguard inget_or_put_resolved_package— it mirrors the existing deferral at line 2138 inget_or_put_resolved_package_with_find_result, and the fall-through correctly returnsNoMatchingVersion/DistTagNotFoundso the caller stops retrying. - Checked
Behavior::is_required()— it returns true for both required and optional peers (!OPTIONAL || PEER), so optional peers with no matching version will also hitwarn_unmet_peer_dependency; that reads as intentional but is a broader surface than the PR title implies. - Verified
may_stay_unresolvedcovers exactly the three call sites and thatintersects(OPTIONAL | PEER)is the right predicate for the writer's output. - Tests: hermetic in-process registry with
cache-control: max-age=300, per-project cache dir, both linkers, root/workspace variant, and a re-resolve step that would have looped without the fix.
Extended reasoning...
Overview
This PR fixes an infinite loop and an unloadable bun.lock when a required peerDependencies entry names a package that exists in the registry but whose range no published version satisfies (issue #26046). Three source changes:
PackageManagerEnqueue.rs—get_or_put_resolved_packagenow only defers withOk(None)onFindVersionError::NotFoundwhen!install_peer; in the peer pass it returns the error instead, ending the retry loop. The caller adds a peer-specific arm that warns instead of erroring forNoMatchingVersion/DistTagNotFound, plus a small#[cold]helper for the warning.bun.lock.rs— three sites inparse_into_binary_lockfilereplacedep.behavior.contains(Behavior::OPTIONAL)with a sharedmay_stay_unresolved(dep)that also accepts unboundPEERedges.PackageManagerResolution.rs— comment-only, replacing a TODO that said unresolved required peers should error with a pointer to the new warning.
Tests add a two-package in-process registry (hoisted + isolated linkers) that pins the warning text, an inline lockfile snapshot, a --frozen-lockfile round-trip, and a re-resolve with a warm manifest cache and zero registry requests.
Security risks
None identified. This is package-manager resolution and lockfile-parsing logic; no untrusted-input parsing, auth, or filesystem-path handling changes. The lockfile parser is loosened (unbound required-peer edges are now accepted), but the writer already emits lockfiles in that shape, and the edge stays unresolved rather than resolving to anything attacker-controlled.
Level of scrutiny
High. bun install is production-critical and this touches core peer-resolution flow, the meaning of Ok(None) from get_or_put_resolved_package in the peer pass, and lockfile-parsing tolerance. The change is small in line count but semantically significant: it introduces a new user-facing warning, loosens what the lockfile parser accepts, and interacts with the manifest cache. It also overlaps with open PR #33156 on the parser change, so a maintainer should decide sequencing.
Other factors
- The
!install_peerguard directly mirrors the pre-existing deferral inget_or_put_resolved_package_with_find_result(line 2138), which supports the claim that the two functions were meant to agree here. Behavior::is_required()is!is_optional()=!OPTIONAL || PEER, so optional peers also enter the new warning branch. That is probably fine (it is a warning, not an error) but is broader than the description states.- The tests are hermetic and follow harness conventions (
tempDir,port: 0, per-projectBUN_INSTALL_CACHE_DIR, spawn timeout as a hang guard,test.concurrent, both linkers viadescribe.each). - The comment-cop bot flagged four long comments; the author cut them in 5e8017e and those threads are resolved.
- CI (#97275) was still building at the last timeline update; no green result is recorded yet.
Given the semantic reach into install behavior and the overlap with #33156, this should get a human maintainer's eyes before merging.
|
One correction to the review above: optional peers cannot reach the new warning. |
…g duplicate package paths Squash of the reviewed commits, rebased on main. The bun.lock parser part of the original change landed separately in #38851 and is no longer in this diff; see the PR description for the full problem, cause and fix write-up.
…g duplicate package paths Squash of the reviewed commits, rebased on main. The bun.lock parser part of the original change landed separately in #38851 and is no longer in this diff; see the PR description for the full problem, cause and fix write-up.
…g duplicate package paths Squash of the reviewed commits, rebased on main. The bun.lock parser part of the original change landed separately in #38851 and is no longer in this diff. A self-contained workspace (#40014) is a hoisting barrier, so a package in its dependency closure does not take a root folder as its peer provider. See the PR description for the full write-up.
…g duplicate package paths Squash of the reviewed commits, rebased on main. The bun.lock parser part of the original change landed separately in #38851 and is no longer in this diff. A self-contained workspace (#40014) is a hoisting barrier, so a package in its dependency closure does not take a root folder as its peer provider. Peers a root folder could satisfy are decided only once the dependency graph is complete. See the PR description for the full write-up.
### Problem - `test/cli/install/bun-lock.test.ts` > "peer no published version satisfies" > "declared by a registry package" (from #38851) fails on main about every fourth build (build 104990). `expect(registry.requests).toEqual([])` at bun-lock.test.ts:1778 receives `["/peer-target"]`: the manifest cache has no usable entry for `peer-target`. It has two causes. #40073 covers the first, the `save_async` exit race. This PR fixes the second. - `Serializer::save` (src/install/npm.rs:1352) named the temporary file `<name hash>.npm-<milliseconds>` in the temporary directory every bun process shares. The four concurrent peer tests all save `peer-target`, and two saves in one millisecond opened one file. Windows, release build, four installs saving one package at once: 77 of 80 lost their entry. Linux uses `O_TMPFILE` today; #38916 starts using the name on Linux too, so this fix has to land first. ### Fix - `Serializer::save` names the file with `FileSystem::tmpname`, as `TarballStream` already does. Correct because the name no longer depends on anything two processes share. - Two tests in `bun-install-registry.test.ts`: four synchronized installs keep their own valid entries, and an install still caches its manifest when directories occupy the old temporary names. The second fails 3 of 3 on Windows without the fix and passes 5 of 5 with it. The test registry serves the tarball only after the cache entry exists, so neither test depends on the exit race. - Verified: `bun bd test test/cli/install/bun-install-registry.test.ts` (250 pass) on Linux, both new tests on Windows. ### Background - Manifest cache: each fetched manifest is serialized to `<cache dir>/<name hash>-<registry hash>.npm`. Within the response's `max-age`, a later install resolves from that file and makes no request. - `write_file` opens the temporary file with `O_CREAT | O_TRUNC`, writes, then renames it into the cache. Two processes on one name write one file. On macOS the second rename fails and the first cache holds the second's bytes, which `load_by_file` rejects. On Windows `rename_at_w` moves by handle, so the second rename moves the file out of the first cache again. - The exit race: `save_async` writes the entry from a thread pool task that `bun install` does not wait for, by design (#37203). The test change in #39190 works around it for the bun-lock test. The harness flag in #40073 removes it from every install test. <details><summary>Notes</summary> Probe (Windows, 16 vCPUs): one registry and one project per install, `dependencies: { "peer-target": "2.0.1" }`, each registry holds its manifest response until all four have been asked, then all respond at once. After exit, the `.npm` entry is read and checked for the install's own registry origin. | binary | temp dir | rounds | ok | missing | wrong registry | | --- | --- | --- | --- | --- | --- | | 1.4.1-canary.1 (release, unfixed) | shared `%TEMP%` | 20 | 3 | 60 | 17 | | 1.4.1-canary.1 (release, unfixed) | shared, responses not synchronized | 40 | 135 | 17 | 8 | | 1.4.1-canary.1 (release, unfixed) | one per install | 40 | 160 | 0 | 0 | | debug, unfixed | shared | 20 | 74 | 3 | 3 | | debug, this branch | shared | 20 | 80 | 0 | 0 | The unfixed debug build loses far fewer entries than the release build because its slower parse spreads the four saves over several milliseconds. That is also why the concurrent test detects the bug almost always on release lanes and only sometimes on a debug build. The directory test fails deterministically on a debug build (3 of 3 and, in an earlier shape of the test, 5 of 5 on Windows). Windows mechanism in detail: `open_file_at_windows` maps `O_CREAT | O_TRUNC` to `FILE_OVERWRITE_IF` with `FILE_SHARE_READ | WRITE | DELETE`, so every process opens and truncates the same file. `rename_at_w` (src/sys/windows/mod.rs:1933) opens the source by path and then moves it by handle. When two processes have opened the path before either moves it, both moves succeed: the second one relocates the file out of the first process's cache. That process sees a successful save and has no entry, which is the "no entry, no error" case in the probe. `FileSystem::tmpname` (src/resolver/lib.rs:198) formats `.<random ^ nanoseconds>-<counter>.<ext>`. The temporary directory probe in `PackageManagerDirectories.rs` and `TarballStream::open_destination` use it the same way. Test registry and the exit race: both new tests read the cache after the install exits. To keep them independent of the `save_async` exit race, the registry answers the tarball request only once a `.npm` entry exists in the project's cache (polling with a 5 second deadline, after which it answers anyway so a lost write ends as a failed assertion instead of a hang). The tarball is requested after the manifest is parsed, so the install cannot finish before its entry is on disk. Sequencing: #38916 changes the Linux `O_TMPFILE` path to link the file into the temporary directory under `tmp_path` before renaming it over an existing entry, which makes the name load-bearing on Linux. This PR should land before it. #39190 (the bun-lock warm-up loop) and #40073 (harness flag) handle the exit race; an earlier shape of this PR carried #39190's commit, which the self-review flagged as a duplicate of an open PR, so it was dropped. Gate: the fixed code path is not reachable on Linux today (`O_TMPFILE`), so the new tests pass on Linux with and without the fix. The fail-before proof is the Windows run above. Also run: `cargo clippy -p bun_install`, clean. </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-install-registry.test.ts, test/cli/install/bun-lock.test.ts <!-- robobun:evidence:end -->
Squash of the reviewed commits, rebased on main. Parts of the original change landed separately while it was in review and are no longer in this diff: the bun.lock parser change (#38851) and the tree dedupe of a peer bound to a folder package (#40564). A self-contained workspace (#40014) is a hoisting barrier, so a package in its dependency closure does not take a root folder as its peer provider. Peers a root folder could satisfy are decided only once the dependency graph is complete. See the PR description for the full write-up.
2 participants
Problem
A package declares a required
peerDependenciesentry whose package exists in the registry but whose range no published version satisfies, and nothing else in the tree provides it. Registry:a@2.1.0withpeerDependencies: { b: "^1.0.1" }, onlyb@2.0.1published; project depends ona. (Real-world shape, #26046:rolldown-plugin-solid-oxc's peersolid-jsx-oxc@*, whose only versions are prereleases.)bun installexits 0 with no diagnostic and leaves the peer edge unresolved.bun.lockit writes does not load back:bun install --frozen-lockfile/bun ci/bun pm lsfail witherror: Failed to resolve peer dependency 'b' for package 'a',InvalidLockfile: failed to parse lockfile, thenlockfile had changes, but lockfile is frozen. A peer declared by the root or a workspace package.json fails the same way (Failed to resolve root peer dependency 'b').bun install(which ignores the unparseable lockfile and re-resolves), or any install without a lockfile, never returns whileb's manifest is still fresh in the manifest cache: 100% CPU inwait_for_resolution->process_peer_dependency_list->enqueue_dependency_with_main->get_or_put_resolved_package.get_or_put_resolved_package(src/install/PackageManager/PackageManagerEnqueue.rs,FindVersionError::NotFoundpath) a peer returnedOk(None)even during the peer pass (install_peer). The caller,enqueue_dependency_with_main_and_success_fn, takesOk(None)to mean the manifest is not loaded; in the peer pass it drops the manifest's dedupe entry, loads the still-fresh manifest from the cache andcontinues into the same lookup, which returnsOk(None)again. With a cold cache the sameOk(None)pushes a callback onto a manifest task that has already completed, which is why the first install is silent.parse_into_binary_lockfile(src/install/lockfile/bun.lock.rs) only tolerate an unbound edge that carries theOPTIONALbit, while the resolver can leave a required peer unbound and the writer records such an edge without a package entry.Fix
get_or_put_resolved_packagedefers withOk(None)only when!install_peer, the conditionget_or_put_resolved_package_with_find_resultalready uses for the same deferral. In the peer pass it returnsNoMatchingVersion/DistTagNotFoundlike a regular dependency, so the loop ends.warn: No version matching "^1.0.1" found for peer dependency "b" (but package exists)) and leaves the edge unresolved. Failing the install instead would change established behavior:verify_resolutionsdeliberately accepts unresolved peers,test/cli/install/bun-install.test.ts("should handle installing the same peerDependency with the same version") pins exit 0 for a root peer with no matching version, andcatalogs.test.tsdocuments the same policy. A peer whose package 404s still errors as before.may_stay_unresolved, at the root, workspace and package sites). Nothing else is needed: the hoisted tree, the isolated linker,Lockfile::eqlandverify_resolutionsalready skip unresolved peers, and the writer already emits the declaration. The loaded state equals the freshly resolved state, so--frozen-lockfilepasses and a reload writes identical bytes, including for root and workspace peers. Recording such peers asoptionalPeersinstead (what the lockb and pnpm migrations do) would not be stable for those:Package::Diffcompares behavior bits, so package.json's plain peer edge would be re-resolved on every install andLockfile::eqlwould then see a different tree.file:peers and does not touch the loop or the warning.test/cli/install/bun-lock.test.ts, "peer no published version satisfies", for both linkers: the install warns,peer-targetis not installed, the lockfile matches an inline snapshot,--frozen-lockfilepasses and leaves it unchanged, and a second resolve with both manifests cached (the test registry sees zero requests) finishes and writes the same bytes; plus a root + workspace variant. Without the fix each step fails on its own: no warning; theInvalidLockfileerrors above; the re-resolve is killed by the spawn timeout (exit 143).bun-install-registry(242 pass),bun-lock,bun-install,isolated-install,catalogs,nested-overrides,frozen-lockfile-pruned,bun-lockbandmigration/locally. The only failures need public network access (bitbucket/gitlab/badssl), which this environment does not have.Fixes #26046
Background
peer_dependencies;wait_for_resolutionthen drains that queue withinstall_peer = true. In that pass a peer is first bound to a same-named package already in the tree (a satisfying one, or any one with the "incorrect peer dependency" warning) and otherwise installed from the registry.Ok(None)fromget_or_put_resolved_packagemeans "park it" in the first pass and "manifest not loaded yet" in the second, so in the second pass only a genuinely unloaded manifest may produce it.max-age; inside that window bun resolves from the cache without creating a network task, so the whole peer pass runs synchronously. That is why the loop needs a warm cache and why the test registry sendscache-control: max-age=300, as registry.npmjs.org does.bun.lockstores each package's declared dependency groups plus the resolved packages keyed by tree path. On load every declared edge is bound to a package entry; an edge that cannot be bound is a parse error unless it is allowed to stay unbound, which until now meant theOPTIONALbit (optional dependencies andpeerDependenciesMetaoptional peers).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