node:vm: throw instead of crashing on a cyclic SourceTextModule linked inside the linker

[robobun]

Fixes #31623

Repro

Build a two-module node:vm SourceTextModule import cycle (A imports B, B imports A) and link + evaluate each module inside the linker callback as it's first encountered:

import * as vm from "node:vm";

const ctx = vm.createContext({ globalThis });
const sources = {
  a: `import { b } from "b"; export const a = "A"; export const ab = () => b;`,
  b: `import { a } from "a"; export const b = "B"; export const ba = () => a;`,
};
const built = new Map();

async function ensure(id) {
  const existing = built.get(id);
  if (existing) return existing;
  const m = new vm.SourceTextModule(sources[id], { context: ctx, identifier: id });
  built.set(id, m);
  await m.link(async spec => await ensure(spec));
  await m.evaluate();
  return m;
}

await ensure("a");

$ bun repro.ts
panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFF8

Under a debug/ASAN build it surfaces as the underlying JSC assert:

ASSERTION FAILED: iter != referrer->loadedModules().end()
vendor/WebKit/Source/JavaScriptCore/runtime/JSModuleLoader.cpp(591)

Cause

SourceTextModule instantiate() runs JSC's whole-graph record->link(), which walks every reachable record via innerModuleLinking and calls JSModuleLoader::getImportedModule() for each requested module. getImportedModule() looks the request up in the referrer's loadedModules(), which is only populated by setImportedModule() during that module's own native link().

In the link+evaluate-inside-the-linker pattern, module B finishes its [kLink] and calls instantiate() while module A is still suspended mid-link() (awaiting its own linker promise for B). A's setImportedModule() calls haven't run yet, so A's loadedModules() is empty. When B's record->link() descends into A and iterates A's requested modules, getImportedModule(A, "b") finds nothing → dereferences the end() iterator: an assert in debug, a read of 0xFFFFFFFFFFFFFFF8 (offset 8 past end()) in release.

Fix

Before instantiate() calls the whole-graph record->link(), pre-walk the dependency graph exactly the way innerModuleLinking does. If any request is missing from a record's loadedModules(), the graph isn't fully linked, so throw a catchable ERR_VM_MODULE_LINK_FAILURE with Node's message (request for '<specifier>' is not in cache) instead of letting JSC crash. A visited set keeps the walk finite on cycles.

The canonical cyclic pattern — link the whole graph first (linker just returns the module), then evaluate the root once — already populates every record's loadedModules() before any instantiate(), so it is unaffected. Same for acyclic incremental linking.

Verification

• bun repro.ts now throws ERR_VM_MODULE_LINK_FAILURE: request for 'b' is not in cache and exits 1 instead of segfaulting — matching Node.
• Canonical link-whole-graph-then-evaluate still resolves cyclic bindings (a.ab() === "B", b.ba() === "A").
• New tests in test/js/node/vm/vm.test.ts (node:vm SourceTextModule cyclic graph linking): the cyclic case fails (crashes) without the fix under both bun bd (the JSC assert) and the release build (SIGSEGV), and passes with it; self-import and canonical cases guard the working paths.
• Full test/js/node/vm/vm.test.ts, the SourceTextModule leak/GC suites, and the node-parallel test-vm-module-{link,errors,reevaluate,synthetic} tests all pass.

[robobun]

^{Updated 7:39 PM PT - May 30th, 2026}

✅ @robobun, your commit bc73a75eb05de6cfb2f651430a9c5c53dc0a9dbd passed in Build #59423! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 31624

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

bun-31624 --bun

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a9f548d-c943-461f-adb7-8f1538cb75b0

📥 Commits

Reviewing files that changed from the base of the PR and between c1b7f8a and bc73a75.

📒 Files selected for processing (1)

• src/jsc/bindings/NodeVMSourceTextModule.cpp

---

Walkthrough

NodeVMSourceTextModule now validates the entire module dependency graph exists in the loadedModules cache before calling record->link(), throwing ERR_VM_MODULE_LINK_FAILURE with a "request for '' is not in cache" message when a dependency is missing. Three regression tests cover cyclic import scenarios.

Changes

Cyclic SourceTextModule Safety

Layer / File(s)	Summary
Module graph pre-validation infrastructure src/jsc/bindings/NodeVMSourceTextModule.cpp	Added wtf/HashSet.h, implemented isModuleGraphLinked() to traverse requestedModules() and confirm each request exists in the corresponding record's loadedModules() while tracking visited records, and updated instantiate() to call the helper and throw ERR_VM_MODULE_LINK_FAILURE with a "request for '<specifier>' is not in cache" message if validation fails before calling record->link().
Cyclic module linking regression tests test/js/node/vm/vm.test.ts	Added a test suite with three cases that spawn a separate process to exercise cyclic SourceTextModule graphs: linking/evaluating inside the linker now throws a catchable ERR_VM_MODULE_LINK_FAILURE, a self-importing module links and evaluates safely, and link-then-evaluate flow works; tests assert empty stderr, exact stdout, and exit code 0.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: preventing a crash when a cyclic SourceTextModule is linked inside the linker callback by throwing an error instead.
Description check	✅ Passed	The PR description comprehensively addresses both required template sections: detailed explanation of the fix and verification of the code through repro testing, canonical patterns, and new test cases.
Linked Issues check	✅ Passed	The PR directly addresses issue #31623 by implementing the fix for the cyclic SourceTextModule crash: throws ERR_VM_MODULE_LINK_FAILURE instead of segfaulting, supports both incremental and canonical linking patterns, and includes comprehensive verification.
Out of Scope Changes check	✅ Passed	All code changes are directly within scope: the helper function isModuleGraphLinked() and pre-walk validation in instantiate() address the crash, and test additions verify both the fix and existing working patterns.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[claude]

LGTM — the iterative worklist rewrite in bc73a75 addresses the recursion-depth concern, and the pre-walk now cleanly converts the crash into a catchable ERR_VM_MODULE_LINK_FAILURE.

Extended reasoning...

Overview

This PR fixes a segfault in node:vm SourceTextModule when a cyclic module graph is linked + evaluated from inside the linker callback. The change adds a static isModuleGraphLinked() helper in src/jsc/bindings/NodeVMSourceTextModule.cpp that pre-walks the dependency graph (iterative worklist + visited set) and verifies every requestedModules() entry is present in the corresponding record's loadedModules() before handing off to JSC's record->link(). If a request is missing, instantiate() throws ERR_VM_MODULE_LINK_FAILURE with Node's "request for '' is not in cache" message instead of letting JSModuleLoader::getImportedModule() dereference an end() iterator. Three subprocess regression tests in test/js/node/vm/vm.test.ts cover the crash case, a self-import, and the canonical link-then-evaluate cyclic pattern.

Prior review

On the first revision I flagged that the recursive form of isModuleGraphLinked() could overflow the native stack on a very deep linear chain before innerModuleLinking's own isSafeToRecurse() guard could throw. The author rewrote it iteratively in bc73a75 (exactly the suggested shape) and verified a 50k-deep chain now surfaces a catchable RangeError rather than segfaulting. That thread is resolved and the current diff reflects the fix.

Security risks

None introduced. The change is purely defensive validation on the instantiate() path: it inspects already-built JSC module records (no user-callback invocation, no allocation that can trip GC mid-walk since HashSet/Vector of raw pointers are walked synchronously with no JS reentry) and either passes through unchanged or throws an error where the process previously crashed. It does not touch context isolation, sandbox boundaries, or evaluation semantics.

Level of scrutiny

Moderate. Native JSC-binding code in node:vm warrants care, but the change is narrow and additive: the happy path (isModuleGraphLinked returns true → fall through to the existing record->link()) is behaviorally identical to before. The ModuleMapKey { request.m_specifier.impl(), request.type() } lookup mirrors how link() populates loadedModules() via setImportedModule(globalObject, requested[i], ...), so false positives on well-formed graphs are guarded against — and the canonical-cyclic-pattern test exercises exactly that.

Other factors

• The bug-hunting system found no issues on the current revision.
• The PR description documents that vm.test.ts, the SourceTextModule leak/GC suites, and node-parallel test-vm-module-* all pass.
• No CODEOWNERS entries cover the touched paths.
• The added code is well-commented, including the rationale for the iterative form.