install: stop hoisting a dependency cycle forever

Draft
RO
25d ago
+617-2

Draft, do not merge. Three defects were reproduced on this branch. The first two were found in review and are regressions on installs that main gets right. All three exit 0, so no test or check reports them:

  1. A package nested below another copy of itself, with no cycle through the affected dependency, loses its own node_modules. Root depends on a@1, b@2, c@2, e@2; a@1 -> b@1, e@1; b@1 -> a@2; a@2 -> c@1, e@3; c@1 -> a@1. main installs 11 packages and the nested a@1 gets e@1. This branch installs 10 and the nested a@1 resolves e@3.
  2. A self-contained workspace (workspaces.selfContained, hoisted linker) with a dependency that depends back on the workspace. main creates apps/desktop/node_modules/desktop -> ... This branch does not.
  3. The lockfile this branch writes for a cycle is misread by released versions of bun. The copy that gets no node_modules is written as an ordinary packages row in a version 2 lockfile. bun 1.4.2 loads that file, binds the package from that deepest row, and installs 2 of the 6 packages of the x/y cycle, with exit code 0, also under --frozen-lockfile. The loader change in this PR only protects versions that contain it. A repository that commits such a lockfile breaks, without an error, for everyone on an older bun. I only tested this branch reading its own lockfile and did not consider older readers.

The hang fix itself works for the reported graphs. The cut in process_subtree is too coarse and the hoist-root check in hoist_dependency is too broad. The Fix section below describes the code as it is now, not the design that will ship.

Problem

  • bun install never 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-only are 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.
  • Smallest shape, no peers or workspaces needed: x@1 -> y@1 -> x@2 -> y@2 -> x@1, with the project depending on x@1.
  • Cause, src/install/lockfile/Tree.rs: when hoist_dependency walks up from a package and the first entry with the dependency's name it meets is a different version, process_subtree nests 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 in Lockfile::hoist never drains. The only guard against this was for folder: dependencies.
  • A second shape hangs for a different reason: a package that bundles a dependency which depends (or has a peer dependency) back on it, the other graph the fuzzer reduced to. A bundled dependency's subtree uses the bundler's own node_modules as 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 own node_modules, the copy bundles the dependency again, and so on.

Fix

  • process_subtree no longer builds a node_modules for 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/arborist place-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 finite node_modules layout 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 a node_modules belonging to the very package the dependency resolves to (same name), treats it as already hoisted: that package is resolvable from everything inside its folder. In the bundled shape this placement always led to the hang.
  • Since #40014 a self-contained workspace is a hoist root too, so this check also fires for a dependency inside such a workspace that depends back on the workspace. That is regression 2 above: main places a link there and terminates. An earlier version of this description said that two such shapes came out identical on main and on this branch. That comparison ran under the isolated linker, where the barrier does not apply, so it showed nothing.
  • The existing folder: guard in process_subtree stays. 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 a node_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-lockfile accepted 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 in process_subtree). Existing lockfiles have such rows only for a copy that was fully laid out, whose bindings agree with the copy above anyway.
  • Behaviour change outside the hang: a package nested below a copy of itself no longer gets its own node_modules, even when its dependencies would have converged. This is regression 1 above. It is not the layout npm produces: npm links the nested copy back to the first one, so that copy's dependencies still resolve correctly. --frozen-lockfile does not notice, since it compares hoisted trees.
  • Verified with 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-lockfile and compares the installed packages, then re-prints it with --lockfile-only and compares the text. All of them time out on main. With only the Tree.rs half 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 from test/cli/install/registry/packages/create-hoist-cycle-packages.ts.
  • #38848 changes the same stop-at-the-bundle-root spot in hoist_dependency for 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.
  • Both graphs the fuzzer reported (one with a workspace pinning a second version of a package, one closed through an optional peer) install and round-trip the same way against a local registry serving exactly those versions; the optional-peer fixture is a smaller version of the second one.

Background

  • Hoisting: Lockfile::hoist turns the resolved graph into the node_modules layout. Each Tree is one node_modules folder: tree.dependency_id is the dependency whose package owns the folder, tree.parent the folder that package sits in. It is built breadth first; placing a dependency that has dependencies of its own queues a FillItem, and process_subtree later creates that package's folder and places its dependencies one by one.
  • Placing a dependency (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.
  • The same builder runs once over the whole graph (the tree bun.lock is written from) and once with disabled and omitted dependencies filtered out (the tree that is installed). bun.lock keys packages by node_modules path, 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-lockfile compares that tree with the one a clean resolve produces. --lockfile-only always rewrites the file, which is why the tests use it to see the tree a reload builds.
  • Bundled dependencies (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's node_modules, which the walk never climbs past.

[human-review] gate passed · iteration 0 · 25 files touched

fails on main (without fix) 
ASAN without fix: 6 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/cli/install/hoist.test.ts
bun test v1.4.3 (367d939d9)

test/cli/install/hoist.test.ts:
(pass) should handle resolving optional peer from multiple instances of same package [285.35ms]
killed 2 dangling processes
(fail) a dependency cycle through two versions of the same packages is installed (hoisted linker) [5005.64ms]
  ^ this test timed out after 5000ms.
Verdaccio exited with code null and signal SIGTERM

# Unhandled error between tests
-------------------------------
52 |     env: bunEnv,
53 |     stdout: "pipe",
54 |     stderr: "pipe",
55 |   });
56 |   const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
57 |   expect({ args, stdout, stderr, exitCode }).toMatchObject({
                                                  ^
error: expect(received).toMatchObject(expected)

  {
-   "exitCode": 0,
+   "args": [],
+   "exitCode": 143,
    "stderr": StringNotContaining "error:",
+   "stdout": 
+ "bun install v1.4.3 (367d939d9)
+ "
+ ,
  }

- Expected  - 1
+ Received  + 6

      a
... (truncated)

release without fix: 6 FAILED
bun test v1.4.3-canary.1 (367d939d9)

test/cli/install/hoist.test.ts:
(pass) should handle resolving optional peer from multiple instances of same package [36.81ms]
killed 2 dangling processes
(fail) a dependency cycle through two versions of the same packages is installed (hoisted linker) [5000.77ms]
  ^ this test timed out after 5000ms.
Verdaccio exited with code null and signal SIGTERM
52 |     env: bunEnv,
53 |     stdout: "pipe",
54 |     stderr: "pipe",
55 |   });
56 |   const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
57 |   expect({ args, stdout, stderr, exitCode }).toMatchObject({
                                                  ^
error: expect(received).toMatchObject(expected)

  {
-   "exitCode": 0,
-   "stderr": StringNotContaining "error:",
+   "args": [],
+   "exitCode": 1,
+   "stderr": 
+ "Resolving dependencies
+ Resolved, downloaded and extracted [6]
+ error: ConnectionRefused downloading package manifest hoist-cycle-x
+ error: hoist-cycle-x@1.0.0 failed to resolve
+ "
+ ,
+   "stdout": 
+ "bun install v1.4.3-canary.1 (367d939d9)
+ "
+ ,
  }

- Expected  - 2
+ Received  + 13

      at ins
... (truncated)
passes on PR (with fix) 
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/cli/install/hoist.test.ts
bun test v1.4.3 (367d939d9)

test/cli/install/hoist.test.ts:
(pass) should handle resolving optional peer from multiple instances of same package [299.45ms]
(pass) a dependency cycle through two versions of the same packages is installed (hoisted linker) [654.36ms]
(pass) a dependency cycle through two versions of the same packages is installed (isolated linker) [666.05ms]
(pass) a cycle entered at both of its versions, from the root and from a workspace, is installed [479.50ms]
(pass) a bundled dependency with a peer on the package bundling it is installed (hoisted linker) [474.72ms]
(pass) a bundled dependency with a peer on the package bundling it is installed (isolated linker) [565.59ms]
(pass) a cycle closed through an optional peer is installed [636.81ms]

 7 pass
 0 fail
 44 expect() calls
Ran 7 tests across 1 file. [6.32s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 833ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/19] rustc bun_install 
[2/19] rustc bun_jsc 
[3/19] rustc bun_ast_jsc 
[4/19] rustc bun_sys_jsc 
[5/19] rustc bun_semver_jsc 
[6/19] rustc bun_bundler_jsc 
[7/19] rustc bun_patch_jsc 
[8/19] rustc bun_css_jsc 
[9/19] rustc bun_sourcemap_jsc 
[10/19] rustc bun_js_parser_jsc 
[11/19] rustc bun_install_jsc 
[12/19] rustc bun_http_jsc 
[13/19] rustc bun_sql_jsc 
[14/19] rustc bun_runtime 
[15/19] link bun-profile
ld.lld: warning: Linking two modules of different target triples: 'obj/unified/UnifiedSource-src_jsc_bindings-0.cpp.o' is 'x86_64-pc-linux-gnu' whereas '../../../../root/.bun/build-cache/webkit-299c5323879e79af-lto/lib/libJavaScriptCore.a(UnifiedSource-inspector-2.cpp.o at 102508510)' is 'x86_64-unknown-linux-gnu'


ld.lld: warning: Linking two modules of different target triples: 'obj/unified/UnifiedSource-src_jsc_bindings-1.cpp.o' is 'x86_64-pc-linux-gnu' whereas '../../../../root/.bun/build-cache/webkit-299c5323879e79af-lto/lib/libJavaScriptCore.a(UnifiedSource-inspector-2.cpp.o at 102508510)' i
... (truncated)
diff hotspot 
src/install/lockfile/Tree.rs                       |  28 +++
 src/install/lockfile/bun.lock.rs                   |  23 +++
 test/cli/install/hoist.test.ts                     | 191 ++++++++++++++++++++-
 .../packages/create-hoist-cycle-packages.ts        | 129 ++++++++++++++
 .../hoist-bundled-cycle-host-1.0.0.tgz             | Bin 0 -> 265 bytes
 .../packages/hoist-bundled-cycle-host/package.json |  25 +++
 .../hoist-bundled-cycle-plugin-1.0.0.tgz           | Bin 0 -> 208 bytes
 .../hoist-bundled-cycle-plugin/package.json        |  22 +++
 .../packages/hoist-cycle-x/hoist-cycle-x-1.0.0.tgz | Bin 0 -> 188 bytes
 .../packages/hoist-cycle-x/hoist-cycle-x-2.0.0.tgz | Bin 0 -> 188 bytes
 .../registry/packages/hoist-cycle-x/package.json   |  35 ++++
 .../packages/hoist-cycle-y/hoist-cycle-y-1.0.0.tgz | Bin 0 -> 192 bytes
 .../packages/hoist-cycle-y/hoist-cycle-y-2.0.0.tgz | Bin 0 -> 192 bytes
 .../registry/packages/hoist-cycle-y/package.json   |  35 ++++
 .../hoist-optional-peer-cycle-entry-1.0.0.tgz      | Bin 0 -> 205 bytes
 .../hoist-optional-peer-cycle-entry/package.json   |  22 +++
 .../hoist-optional-peer-cycle-x-1.0.0.tgz          | Bin 0 -> 209 bytes
 .../hoist-optional-peer-cycle-x-2.0.0.tgz          | Bin 0 -> 209 bytes
 .../hoist-optional-peer-cycle-x/package.json       |  37 ++++
 .../hoist-optional-peer-cycle-y-1.0.0.tgz          | Bin 0 -> 209 bytes
 .../hoist-optional-peer-cycle-y-2.0.0.tgz          | Bin 0 -> 232 bytes
 .../hoist-optional-peer-cycle-y/package.json       |  40 +++++
 .../hoist-optional-peer-cycle-z-1.0.0.tgz          | Bin 0 -> 205 bytes
 .../hoist-optional-peer-cycle-z-2.0.0.tgz          | Bin 0 -> 173 bytes
 .../hoist-optional-peer-cycle-z/package.json       |  32 ++++
 25 files changed, 617 insertions(+), 2 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file 
file                                                      reads  edits  tests
src/install/lockfile/Tree.rs                                 14     10     19
src/install/lockfile/bun.lock.rs                              9      4     19
test/cli/install/hoist.test.ts                                4      8     19
…nstall/registry/packages/create-hoist-cycle-packages.ts      0      4     20
…t-bundled-cycle-host/hoist-bundled-cycle-host-1.0.0.tgz      0      0     19
…registry/packages/hoist-bundled-cycle-host/package.json      0      0     19
…ndled-cycle-plugin/hoist-bundled-cycle-plugin-1.0.0.tgz      0      0     19
…gistry/packages/hoist-bundled-cycle-plugin/package.json      0      0     19
…registry/packages/hoist-cycle-x/hoist-cycle-x-1.0.0.tgz      0      0     20
…registry/packages/hoist-cycle-x/hoist-cycle-x-2.0.0.tgz      0      0     19
…li/install/registry/packages/hoist-cycle-x/package.json      0      0     19
…registry/packages/hoist-cycle-y/hoist-cycle-y-1.0.0.tgz      0      0     19
…registry/packages/hoist-cycle-y/hoist-cycle-y-2.0.0.tgz      0      0     19
…li/install/registry/packages/hoist-cycle-y/package.json      0      0     19
…r-cycle-entry/hoist-optional-peer-cycle-entry-1.0.0.tgz      0      0     19
…y/packages/hoist-optional-peer-cycle-entry/package.json      0      0     19
(+ 9 more files)