Deterministic dependency resolution: an ordered walk over a live tree - #36476
Draft
dylan-conway wants to merge 14 commits into
Draft
dylan-conway wants to merge 14 commits into
dylan-conway wants to merge 14 commits into
Conversation
An `npm:real@range` dependency resolves the package `real` under its real name in the package index; the alias is only the name it installs under. A resolution-time map (`known_npm_aliases`), filled in as manifests were parsed, retargeted any other npm dependency edge that happened to share an alias's name onto the aliased package whenever the range was satisfiable. The outcome depended on which manifests had been parsed so far, and it coupled unrelated packages by name alone. Delete the map and its plumbing. A plain edge that shares an alias's name now resolves against the registry under its own name like any other dependency, and hoisting decides its placement (nesting it when the root's alias owns the slot). Dependency parsing is pure again: `parse`, `parse_with_tag`, and the resolver's `parse_dependency` hooks no longer take a mutable package-manager handle, and `parse_dependency` no longer needs a name hash. The lockfile-loading path and `from_npm` only ever read the manager, so those take a shared reference and two raw-pointer reborrows that existed to hand out a mutable one are gone. Two tests encoded the old retargeting and are updated: a workspace whose dependency shares the root alias's name resolves the real package, and a workspace's explicit alias now resolves to its own target instead of inheriting the root's.
dylan-conway
marked this pull request as ready for review
July 30, 2026 17:06
dylan-conway
marked this pull request as draft
July 30, 2026 17:13
Contributor
WalkthroughRemoved npm alias registry state from dependency parsing and PackageManager, updated lockfile and installation call paths, and revised workspace and unhoisted dependency installation tests. ChangesNPM alias parsing decoupling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/install/bun-install.test.ts (1)
3157-3231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a companion negative-path test.
This test covers the success case (a plain workspace dependency resolves independently of a root alias). The PR intentionally changes behavior so that a plain dependency whose name exists only as an alias target now fails with a 404 instead of being silently satisfied by the aliased package. A dedicated regression test asserting that failure mode would directly guard this documented breaking change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/install/bun-install.test.ts` around lines 3157 - 3231, Add a companion test alongside the existing workspace dependency test that defines a root npm alias whose target name is used only by a plain workspace dependency, then runs install and asserts the registry returns a 404 and installation fails. Keep the test focused on preventing the alias from satisfying the plain dependency, using the existing test helpers and context setup patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/cli/install/bun-install.test.ts`:
- Around line 3157-3231: Add a companion test alongside the existing workspace
dependency test that defines a root npm alias whose target name is used only by
a plain workspace dependency, then runs install and asserts the registry returns
a 404 and installation fails. Keep the test focused on preventing the alias from
satisfying the plain dependency, using the existing test helpers and context
setup patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c73fb017-e889-4255-aa92-da084e005d10
📒 Files selected for processing (26)
src/install/PackageManager.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/UpdateRequest.rssrc/install/PackageManager/install_with_manager.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/auto_installer.rssrc/install/dependency.rssrc/install/lockfile.rssrc/install/lockfile/Buffers.rssrc/install/lockfile/CatalogMap.rssrc/install/lockfile/OverrideMap.rssrc/install/lockfile/Package.rssrc/install/lockfile/bun.lock.rssrc/install/lockfile/bun.lockb.rssrc/install/migration.rssrc/install/pnpm.rssrc/install/yarn.rssrc/install_jsc/dependency_jsc.rssrc/install_jsc/update_request_jsc.rssrc/install_types/resolver_hooks.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/package_manager_command.rstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.ts
💤 Files with no reviewable changes (9)
- src/install_jsc/update_request_jsc.rs
- src/install_jsc/dependency_jsc.rs
- src/runtime/cli/bunx_command.rs
- src/install/PackageManager/updatePackageJSONAndInstall.rs
- src/install/pnpm.rs
- src/install/yarn.rs
- src/resolver/resolver.rs
- src/install/PackageManager.rs
- src/resolver/package_json.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I didn't find bugs, but this deliberately changes resolution semantics for npm-aliased dependencies (and diverges from npm), so a maintainer should sign off on the design.
What was reviewed:
- Traced every dropped
alias_hash/&mut PackageManagerparam throughparse/parse_with_tagcall sites — all are pure deletions with no orphaned reads. - Checked
NpmInfo.is_aliasconsumers (override gate inPackageManagerEnqueue.rs:658,PackageJSONEditor,bun.lock.rs) against the semantic change from "recorded in map" to "literal usednpm:". - Verified the
from_npmreshape inPackageManagerEnqueue.rsdrops the aliasing raw-pointer split cleanly andthis_ptris still declared before its remaining use. - Confirmed the two updated tests encode the new placement (independent fetch + nesting) rather than being weakened.
Extended reasoning...
Overview
This PR deletes the known_npm_aliases map from PackageManager and all its plumbing across 24 source files plus 2 test files. The map previously retargeted plain npm dependency edges onto same-named aliased packages during resolution. With it gone, dependency parsing (parse/parse_with_tag) is pure again — the alias_hash, &mut PackageManager, and NpmAliasRegistry trait parameters are removed from ~30 call sites, two unsafe raw-pointer reborrows in auto_installer.rs and PackageManagerEnqueue.rs are eliminated, and several &mut PackageManager params downgrade to &PackageManager or disappear entirely.
Security risks
None identified. This is internal resolution machinery; no untrusted-input parsing, path handling, or auth surface is touched.
Level of scrutiny
High — this is core package-manager resolution logic and a deliberate behavioral change. The PR description is explicit that (a) it diverges from npm's in-tree satisfaction behavior, and (b) a workspace that plainly depends on a name that only exists as an alias will now 404 against the registry. That's a user-visible change that could break existing installs, and per the repo review guidance, changing a Bun-native default vs. Node/npm compat is a maintainer-level call.
Other factors
- The mechanical side is clean: nearly every hunk is a parameter deletion or a call-site update, and the one non-trivial reshape (
get_or_put_resolved_package_with_find_result) correctly re-declaresthis_ptrafter the split-borrow so the debug-assert path still compiles. NpmInfo.is_aliaschanges meaning (no longer gated onalias_hash.is_some()). The author spot-checkedbun addoutput against release; the override-skip gate atPackageManagerEnqueue.rs:658and thebun.lockserialization consumer look consistent with the new definition, but I'd want a maintainer to confirm the intent there.- Two existing tests were rewritten to assert the new behavior (one renamed from "should get npm alias with matching version" to assert independent resolution). Per the review rules, weakening/repurposing existing tests needs explicit justification — the PR body states it, but a human should confirm this is the desired product direction.
- One test (
yarn-cli-repo) is noted as timing out locally on debug+ASAN and left for CI.
This was referenced Jul 31, 2026
No-Verification-Needed: incomplete WIP checkpoint, not a completed change
…inism # Conflicts: # src/install/PackageManager.rs # src/install/PackageManager/processDependencyList.rs # src/install/dependency.rs # src/install/lockfile.rs # src/install/lockfile/CatalogMap.rs # src/install/lockfile/Package.rs
Replace the arrival-driven resolver — where the outcome depended on the order network responses landed — with a deterministic cursor that builds the dependency tree as it resolves. One rule applies to every package (aliases, git, tarballs, workspaces, peers): an edge uses a same-named slot up the tree when that slot's version satisfies its range, otherwise it resolves its own best version and takes a new slot below any conflict. The walk stops at the first same-named slot, matching the runtime's own node_modules lookup, so a satisfied name is never fetched and a nearer non-satisfying slot is a genuine conflict. The cursor runs two passes: the first places every already-bound edge (the recorded tree, no I/O), the second sweeps every node deciding the unbound edges against that fully-placed tree. Manifest fetches run in parallel and speculatively, gated by the walk; failures are recorded as data on the name and reported per dependency at decision time rather than aborting the resolve. Peers are decided in the same pass at their own node, walking from the parent scope, deleting the deferred global peer phase whose outcome depended on arrival order. The loaded lockfile is authoritative, holes included: an edge the lockfile left unresolved and the manifest diff did not touch stays unresolved, so an in-sync install produces the exact same tree, and an out-of-sync install re-resolves only what the diff touched. A range edge no slot up-tree satisfies prefers the highest already-present satisfying version before the manifest, keeping the lockfile's settled choices stable; edges named in a `bun update` request resolve fresh instead. Fresh installs, incremental installs, add/remove/update/link, the runtime auto-installer, lockfile migration, and the manifest-only prefetch all run on the one resolve_graph driver. Both linkers and lockfile save/load are unchanged consumers of the resulting per-edge bindings, so the lockfile format does not change. Re-enables the two peer/hoisting tests that had been marked flaky on CI, which the deterministic order makes stable, and adds a test for a same-named workspace dependency satisfied by a root npm alias without requesting the shadowed name.
Collaborator
…esolver Address issues found in review of the dependency-resolution rewrite: - Guard against version-conflict dependency cycles (a@1 depending on a@2 depending on a@1) by refusing to re-place a package that already sits in the ancestor chain, matching npm. Previously such cycles nested nodes without bound. - Treat catalog references as update targets so a bare `bun update` moves them too, restoring the rule from #36379 that the rewrite dropped. - Chain a checkout for every dependency waiting on a git clone, each from its own recorded commit, instead of only the clone task's originating dependency. Fixes isolated-linker installs with several git deps on one repository, and drops the now-unused originating-dependency field. - Prefetch a node's manifests when its parent finishes deciding rather than when the cursor reaches the node: the whole frontier fetches in parallel while the parent's slots (including any alias) already exist, so a name a sibling slot satisfies is still never requested. - Skip the resolution passes entirely on an install whose manifest diff produced no work, so an in-sync install pays no resolution cost. - Give peers one conflict policy in every resolution path: a peer whose parent scope holds a different same-kind package binds to it with a warning instead of installing a second copy. This also stops dist-tag peers from warning when the scope already holds the tagged version. - On a failed tarball download in the callback-free task loop, reset the package back to Extract so a resolver still waiting on it re-checks, hits the recorded failure, and fails fast instead of polling an extraction that will never land. Adds a resolution-order test: a fake registry that parks each manifest response and releases them in a controlled order (forward, reversed, and seeded shuffles), asserting the resolved lockfile is byte-identical across every order.
Replace the single arrival-order test with a scenario framework: an in-memory fake registry that parks manifest responses and releases them in controlled orders (with real tarballs built by `bun pm pack` and cached by content), a project builder, and a lockfile parser. Every scenario pins the exact resolution it must produce and also runs under three release orders, asserting a byte-identical lockfile across all of them. Adds 23 scenarios across peers (ranged peers with many pins, satisfied and conflicting peers, auto-installed peers, unsatisfiable peers, shared providers, peer chains, dist-tag peers), hoisting and conflicts (name-order tie-breaking, diamonds, deep conflicts, exact/range dedupe, self-cycles), dist-tags and npm aliases (shadowing without requesting the shadowed name), workspaces and catalogs, and lockfile fidelity (in-sync reinstall, adding a dependency reuses a locked version). The exact-plus-range dedupe scenario resolves differently across release orders on the current release, so the suite includes at least one order-controlled nondeterminism regression. A cross-version dependency cycle is recorded as a disabled scenario: resolution completes but the hoister loops on it, which reproduces on released builds and belongs to a separate fix.
Address issues found in a further review of the resolution rewrite: - Mark a tarball's dedupe entry failed when building its network task errors (the entry is inserted before the fallible build), so a later edge on the same tarball fails fast instead of waiting on a task that will never run. - When chaining checkouts after a git clone, resolve the commit from the committish for a dependency whose lockfile carries no resolved sha (e.g. a non-GitHub git dep migrated from yarn.lock) rather than passing an empty one to checkout. - Route in-flight manifest fetches through the task's dedupe entry so an optional edge's fetch is upgraded to required once a required edge waits on it, and its failure errors instead of only warning. - For a dist-tag peer, check the parent scope for a same-named occupant before resolving: bind (with a warning only when it is not the tagged version) instead of creating the tagged package and firing its download first. - Prefetch a node's manifests when it is created rather than when its parent finishes, restoring whole-frontier pipelining; only names the owning node has yet to decide (the only ones that can still shadow a child) keep waiting for the owner, so a satisfied name is still never requested. Restore the pre-existing "should get npm alias with matching version" test verbatim and keep the alias-independence case alongside it as its own test, rather than mutating the original's inputs. Add eleven more resolution-order scenarios: optional dependencies and optional peers, root vs transitive devDependencies, overrides pinning a transitive version, deep chains, wide fan-in, three-way major conflicts, the root's version claiming the top level, and a root dist-tag over a conflicting nested range.
dylan-conway
changed the title
Address issues found in a further review of the resolution rewrite: - Compare a dist-tag peer's occupant against the version the resolver would actually pick by applying the same minimumReleaseAge / exclude filter, so no spurious incorrect-peer warning is printed for a filtered tag. - Register a manifest fetch's dedupe entry before copying the cached manifest, so a dedupe hit (a fetch already in flight) no longer pays for a deep copy of a possibly multi-megabyte manifest on every retry. - Track the owning node's undecided edge names in a count map instead of rebuilding a name list per placement, making the "can this name still shadow a child" check O(1) rather than O(D) per child edge. - Remove the peer-specific placement routine, now identical to the general one after the peer conflict policy was unified, and route peers through the same placement. - Give the dependency sorter one shared comparator used by the resolver, the tree builder, and the isolated linker instead of three copies. - Drop parameters that no longer carry anything and delegate the resolved package log line to the shared formatter. In the resolution-order test, use the file-level default timeout the other install suites use instead of a per-test timeout on every scenario.
This was referenced Aug 7, 2026
This was referenced Aug 15, 2026
This was referenced Sep 16, 2026
Open
This was referenced Sep 30, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rewrite
bun install's dependency resolution so its outcome no longer depends on the order the registry's responses arrive in.The problem
Resolution was arrival-driven: whichever manifest landed first could decide the shape of the tree. That is the root cause of the years-long flaky install tests (e.g.
hoisting > peersin the registry suite, retried in a large fraction of CI builds) — the samepackage.jsoncould produce different lockfiles depending on network timing. It also meant a name that only exists as an npm alias could be requested from the registry and 404, failing an otherwise-valid install.What changes
Resolution becomes an ordered walk over a live tree. A single cursor builds the dependency tree as it resolves, deciding one edge at a time in a fixed order (breadth-first from the root; within a package the existing dependency order — workspaces, dev, optional, prod, peers last — name-ascending within each). One rule governs every kind of package — aliases, git, tarballs, workspaces, plain registry packages, and peers alike:
The walk stops at the first same-named slot, exactly as node's runtime lookup does, so a nearer slot that doesn't satisfy is a genuine conflict (the edge nests below it) and a satisfied name is never fetched. The cursor runs two passes: the first places every edge the lockfile already bound (the recorded tree — no I/O); the second sweeps every node deciding the unbound edges against that fully-placed tree, so a newly added dependency resolves against the whole existing tree.
Manifest fetches run in parallel and speculatively, gated by the walk so a name a slot up-tree already satisfies is never requested. Failures are recorded as data on the name and reported per dependency when it is decided, rather than aborting resolution. Peers are decided in the same pass at their own node, walking from the parent scope (npm's model), which deletes the deferred peer phase whose outcome depended on arrival order. Version-conflict dependency cycles (
a@1⇄a@2) are bound rather than re-nested, as npm does.The loaded lockfile is treated as authoritative, holes included: bound edges are never re-decided, and an edge the lockfile left unresolved that the manifest diff did not touch stays unresolved. So an in-sync install produces the exact same tree, and an out-of-sync install re-resolves only what the diff touched. A range edge that no slot up-tree satisfies prefers the highest already-present satisfying version before consulting the manifest, keeping the lockfile's settled versions stable; edges named in a
bun updaterequest resolve fresh instead.Fresh installs, incremental installs,
add/remove/update/link, the runtime auto-installer, lockfile migration, and the manifest-only prefetch all run on the one driver. Both linkers and lockfile save/load are unchanged consumers of the resulting per-edge bindings, so the lockfile format does not change.Verification
bun-install, the registry suite, workspaces, add, remove, update, lock, lockb, link, pack, lifecycle, audit, pm, catalogs, and isolated-install all green. The failures that remain reproduce identically on the released binary (thebun patch --linker=isolatedgroup and one isolated-install ranged-peer test) and are unrelated to this change.bun-install-resolution-order.test.ts: a fake registry that parks each manifest response and releases them in a controlled order, with real tarballs extracting in between. It carries a suite of specific edge-case scenarios (peers, conflicts, cycles, aliases, dist-tags, workspaces, catalogs, overrides, optional deps, lockfile fidelity), each pinning the exact expected resolution and running under several release orders that must yield a byte-identical lockfile. At least one scenario resolves differently across release orders on the current release (an order-controlled nondeterminism regression); most others pin behavior the release already had, now guaranteed order-independent.Notes for review
"lodash": "npm:lodash-es@…"alias plus a transitive plainlodashrange) now bind to the one slot rather than fetching the second package. This is the intended consequence of the general rule and is called out for an explicit decision.incorrect peer dependencywarning instead of installing a second copy — one policy across range, dist-tag, and local peers.