install: stop hoisting a dependency cycle forever - #38976
Conversation
The tree builder nests a dependency inside its dependent whenever the nearest node_modules holding that name has a different version. Around a cycle through two versions of the same packages (x@1 -> y@1 -> x@2 -> y@2 -> x@1 ...) every copy conflicts with the copy one level up, so copies were nested without end and `bun install` never returned while its memory grew. A package bundling a dependency that depends back on it looped as well: the bundled subtree's walk stops at the bundler's own node_modules without seeing the bundler one level up, so the bundler was copied into itself and the copy bundled the dependency again. process_subtree now leaves a copy of a package that is nested below another copy of the same package without a node_modules of its own (the point at which npm links back to the copy above), which bounds every path by the number of packages, and hoist_dependency resolves a dependency onto the package whose node_modules it would otherwise be placed in. The bun.lock loader bound a package's dependencies from each of its rows in turn, so the unexpanded copy, always the deepest row, rebound them to the versions it sits under and a clean checkout installed a smaller tree while --frozen-lockfile passed. Rows below another row of the same package are skipped now; the copy the tree was built from binds the package.
|
Warning Review limit reached
Next review available in: 16 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 (25)
Comment |
|
Updated 7:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 86a1bc6 has some failures in 🧪 To try this PR locally: bunx bun-pr 38976That installs a local version of the PR into your bun-38976 --bun |
|
Status: ready for review; CI is green on every lane that ran. Reproduced on the released bun (and on main) with a local registry: The new cases in Build 97864 (current head): 177 of 179 jobs passed, including this test on Linux, macOS x64 and both Windows targets. The remaining two are the |
![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}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the earlier Windows path-separator issue in installedPackageJsons was fixed in cb91604). Because this changes the hoister's termination condition and how bun.lock rows are bound on load — with an acknowledged layout change for non-cycle graphs where a package sits below a copy of itself — a human look from someone who owns src/install/lockfile/ would still be worthwhile.
What was reviewed
- The
process_subtreeancestor walk: starts atself.id(the parent tree, not the just-appended one), guardsdependency_idagainstresolutions.len()soROOT_DEP_IDis skipped, and comparesPackageID— matches npm's stop condition. - The bundled-root check in
hoist_dependency: gated on bothresolutions[dependency_id] == package_idand matchingname_hash, so it only fires when the dependency is the bundling package itself;package_id == invalid_package_idcannot reach it. PkgMap::is_below_copy_of: probes every/-prefix ofpkg_path; scoped-name half-prefixes (@scope) are absent from the map so they miss harmlessly, and the full path is excluded so a row does not match itself.- Tests: fresh install →
--frozen-lockfilereload →--lockfile-onlyre-print, across both linkers, workspaces, bundled, and optional-peer shapes; the reload half fails without thebun.lock.rschange per the author's report.
Extended reasoning...
Overview
This PR fixes an infinite loop / OOM in bun install's hoisting pass when the dependency graph contains a cycle through two versions of the same package names (e.g. x@1 → y@1 → x@2 → y@2 → x@1), and a related loop when a bundled dependency has a peer back on the package bundling it. Three code changes:
src/install/lockfile/Tree.rs,process_subtree: before appending a new tree for a package, walk the ancestor chain; if any ancestor tree'sdependency_idresolves to the samePackageID, return early. Every dependency of that package was placed while the higher copy was processed, so re-processing only re-nests the shadowing versions forever.src/install/lockfile/Tree.rs,hoist_dependency: after the walk stops at a bundled hoist root, if the dependency being placed is the bundling package itself (same resolvedPackageIDand samename_hashasthis.dependency_id), returnHoistedinstead of placing it into its ownnode_modules.src/install/lockfile/bun.lock.rs: newPkgMap::is_below_copy_ofand acontinuein the row-binding loop so a lockfile row that sits below another row of the same package does not rebind that package's dependencies to the shadowing versions on its path — the copy above binds them.
The rest is test fixtures (generated by create-hoist-cycle-packages.ts) and ~190 lines of tests in hoist.test.ts.
Security risks
None. The change narrows when the hoister recurses and when the loader rebinds; input is the already-resolved dependency graph / lockfile rows. No new parsing of untrusted data, no filesystem or network paths touched. is_below_copy_of slices pkg_path at / boundaries and looks the prefixes up in a map keyed by full package paths, so partial-segment matches are impossible by construction.
Level of scrutiny
High. process_subtree and hoist_dependency run on every bun install, and the row-binding loop runs on every lockfile load. The PR description explicitly notes a behaviour change for non-hanging graphs: a package nested below a copy of itself whose subtree previously converged now gets no node_modules of its own (matching npm), and its rows disappear from bun.lock on the next write. --frozen-lockfile is stated to be unaffected because it compares hoisted trees, but that and the interaction with #38848 (which touches the same bundle-root spot) are the kind of things a maintainer who owns this code should confirm.
Other factors
- The bug-hunting system found no issues on the current head. My earlier finding (Windows backslashes in
installedPackageJsons) was fixed in cb91604 and is verified in the diff. - The tests are strong: each shape does a fresh install, asserts the exact
bun.locktree, reinstalls from that lockfile with--frozen-lockfilein a clean dir, compares installedpackage.jsonsets, then--lockfile-onlyre-prints and byte-compares. The author reports all of them time out on main and still fail with only theTree.rshalf applied, so both halves are load-bearing. - The four remaining comment-cop flags were pushed back on by the author with reasons (invariant / cross-file coupling documentation rather than workaround justification); that's a style call for a human.
- The existing
folder:-cycle guard just above the new code inprocess_subtreeis now largely subsumed by the new general check (both compare ancestor resolution to the targetpkg_id), which a reviewer may want to note, though the folder guard also matches onname_hashand skips only the one dependency rather than the whole subtree.
2 participants
Problem
bun installnever returns on some dependency graphs: 100% CPU and memory growing without bound (about 1 GB after 3 s on a release build) until it is killed. Both linkers and--lockfile-onlyare affected because the hang is in the lockfile's hoisting pass (Lockfile::resolve->Lockfile::hoist->Tree::process_subtree), which runs before anything is written. Found by an install fuzzer (13% of its random graphs); most shapes also hang bun 1.3.14, one (a cycle closed through an optional peer) only hangs current main.x@1 -> y@1 -> x@2 -> y@2 -> x@1, with the project depending onx@1.src/install/lockfile/Tree.rs: whenhoist_dependencywalks up from a package and the first entry with the dependency's name it meets is a different version,process_subtreenests the dependency inside the package and queues that copy's own dependencies. Around a cycle through two versions of the same names every copy conflicts with the copy one level up, so each copy nests another one and the queue inLockfile::hoistnever drains. The only guard against this was forfolder:dependencies.node_modulesas its hoist root, and the walk stops there without looking at the folder one level up that holds the bundler itself. The bundler is copied into its ownnode_modules, the copy bundles the dependency again, and so on.Fix
process_subtreeno longer builds anode_modulesfor a copy of a package that is nested somewhere below another copy of the same package. Everything that package depends on was placed while the copy above was processed; laying it out again can only re-nest the versions the levels in between shadow, which is the step that never converges. This is the same condition at which npm's placer stops nesting (@npmcli/arboristplace-dep.js, which links back to the copy above at that point). Without a link the copy stays installed as is and its own dependencies resolve to whatever is on the path, so one edge of the cycle resolves to the other version; no finitenode_moduleslayout of such a graph avoids that. Every copy on a path now belongs to a different package, so the pass is bounded for any input.hoist_dependency, when it would place a dependency into anode_modulesbelonging to the very package the dependency resolves to (same name), treats it as already hoisted: that package is resolvable from everything inside its folder. This placement only ever happened in the bundled shape above, where it always led to the hang, so no terminating install changes.folder:guard inprocess_subtreestays.folder:dependencies never go through the walk, so that guard is what keeps a folder package from being copied at all when it is its own ancestor; the new check only decides whether a copy that was placed gets anode_modules.bun.lock.rs: the loader binds a package's dependencies by walking up from each of its rows, last row winning. The unexpanded copy is always the deepest row of its package and by construction walks to the shadowing versions, so a clean checkout loaded the written lockfile into a different graph, re-hoisted it to a smaller tree, and--frozen-lockfileaccepted it (the frozen check compares the loaded tree with itself after cleaning). Rows that sit below another row of the same package are now skipped when binding; the copy above, which the tree was built from, binds the package (PkgMap::is_below_copy_of, mirroring the check inprocess_subtree). Existing lockfiles have such rows only for a copy that was fully laid out, whose bindings agree with the copy above anyway.node_modules(the same layout npm produces); its rows under the copy disappear from bun.lock on the next write.--frozen-lockfileis unaffected since it compares hoisted trees, not rows.test/cli/install/hoist.test.ts: the plain two-version cycle under both linkers (also checking the installed layout), the same cycle entered at both versions from the root and a workspace, the bundled host/plugin shape under both linkers, and a cycle closed through an optional peer. Every test installs from scratch, then installs the resulting bun.lock in a new directory with--frozen-lockfileand compares the installed packages, then re-prints it with--lockfile-onlyand compares the text. All of them time out on main. With only theTree.rshalf applied they still fail: the plain cycle reloads to 2 of 6 packages and the optional-peer one to 6 of 13, with exit code 0. Fixtures come fromtest/cli/install/registry/packages/create-hoist-cycle-packages.ts.hoist_dependencyfor a different bug (a bundled subtree placing another version of one of the bundler's own dependencies into the bundler's folder); the check here only concerns the bundler itself and is independent of it.Background
Lockfile::hoistturns the resolved graph into thenode_moduleslayout. EachTreeis onenode_modulesfolder:tree.dependency_idis the dependency whose package owns the folder,tree.parentthe folder that package sits in. It is built breadth first; placing a dependency that has dependencies of its own queues aFillItem, andprocess_subtreelater creates that package's folder and places its dependencies one by one.hoist_dependency) walks up from the dependent's own folder. No entry with that name anywhere on the path: it goes to the top. The same package found: nothing to place. A different version found first: the dependency is nested in the dependent's own folder, because Node would otherwise resolve the name to that other version. The last rule is what a two-version cycle drives forever.packagesbynode_modulespath, so one package can appear under several keys; loading walks up from each key to rebind the package's dependencies and then hoists again, and--frozen-lockfilecompares that tree with the one a clean resolve produces.--lockfile-onlyalways rewrites the file, which is why the tests use it to see the tree a reload builds.bundleDependencies) ship inside the bundler's tarball. Their own dependencies are kept below the bundler by giving their subtree a hoist root at the bundler'snode_modules, which the walk never climbs past.