node:module: throw instead of crashing when a preload sets runMain to a non-callable, and report a throwing override

[robobun]

Problem

• A preload that assigns a non-function to require("module").runMain crashes when Bun goes to run the entry point: panic(main thread): Segmentation fault at address 0x4 in release builds (ASSERTION FAILED: Expected object to be callable but received 0 in debug builds). Repro: echo 'require("module").runMain = {}' > preload.cjs; bun -r ./preload.cjs main.js. A string (runMain = "x") crashes the same way. Node exits 1 with TypeError: ... runMain is not a function.
• runMain = undefined / null / 5 were silently ignored: the property still read back as the builtin and main ran. Node stores the value and then fails with the same TypeError.
• A callable override that throws (or a class, which throws when called without new) is reported only as Error occurred loading entry point: JSError, exit 1. The exception itself is never printed and process.on("uncaughtException") listeners are not consulted. Node prints the error / runs the listeners.
• The same two things happen when the preload comes from a Worker's preload option: a non-callable runMain there segfaults the whole process, and a throwing one closes the worker with exit code 1 but never fires its error event.
• Causes, at b7a0431:
  • setModuleRunMain (src/jsc/modules/NodeModuleModule.cpp:832) stores any cell into m_moduleRunMainFunction and drops non-cells; NodeModuleModule__callOverriddenRunMain (:824) downcasts the stored cell to JSObject and calls it without checking that it is callable.
  • reload_entry_point (src/jsc/VirtualMachine.rs:2705) maps any exception from that call to CrateError::JSError, and entry_point_load_failed (src/runtime/cli/run_command.rs:1653) prints only the error name.

Fix

• The assigned value now lives in a new WriteBarrier<JSC::Unknown> slot (m_moduleRunMainOverride), empty while the property holds the builtin. The setter stores any value there (assigning the builtin back clears it), the getter returns it, and m_moduleRunMainFunction only ever holds the builtin (typed LazyProperty<JSFunction> accordingly). The unused hasOverriddenModuleRunMain bool is removed; the slot being non-empty is that state.
• NodeModuleModule__callOverriddenRunMain calls the current value through JSC::call(..., "Module.runMain is not a function"_s), which throws that TypeError for anything without call data and otherwise propagates what the override throws. It now follows the zero-means-threw convention, and passes the Module object as this, which is how Node's bootstrap calls it (Module.runMain(main)); previously this was the global object.
• On the Rust side, an exception from that call becomes the entry point's promise: a fresh promise marked handled and rejected with the exception, stored as pending_internal_promise and returned, so the run command reports it exactly like an entry module that throws (prints it, or consults uncaughtException listeners, --hot keeps running), and a worker's startup code reports it as that worker's error event. evaluated_as_cjs is set so the origin passed to listeners is uncaughtException, matching Node, where this throw happens synchronously in the bootstrap. The success path (use the promise the override stored by calling the builtin, else wrap the return value) is unchanged, so an async override that rejects is still reported as a rejection with origin unhandledRejection, also as in Node.
• Why this is correct: it matches Node. Module.runMain there is a plain data property, so assignment always succeeds and reads back, and the failure happens when the bootstrap calls it, as an ordinary uncaught exception. Checking callability at call time (not in the setter) also keeps code working that assigns a placeholder and restores the function before main runs. Marking the rejected promise handled up front is what the module loader does for its own entry promises, so the failure is reported once, by the caller, and never also as an unhandled rejection.
• Verified with the new tests in test/js/node/module/node-module-module.test.js: non-callable values ({}, a string, undefined, null, 5) exit 1 with the TypeError and main does not run; a throwing function and a class report the actual error; both failure kinds reach process.on("uncaughtException") with origin uncaughtException; assigned values read back and restoring the builtin works; an override is called with Module as this and can call through to the builtin; a throwing and a non-callable override in a Worker's preload surface as that worker's error event with exit code 1. All 13 fail on the released build (the object/string rows crash the child) and pass with this change; the two existing Module.runMain tests still pass.
• Also ran with the debug build: test/cli/run/preload-test.test.js, test/config/bunfig/preload.test.ts, test/js/bun/resolve/bun-main-entry-point.test.ts, test/internal/source-lints/, and the repro scripts under BUN_JSC_validateExceptionChecks=1.
• node:module: throw instead of crashing when _resolveFilename is set to a non-callable #38089 fixes the same setter/call pattern for Module._resolveFilename and deliberately left runMain to this change because of the reporting half; the two touch adjacent lines of ZigGlobalObject.h and are otherwise independent.

Background

• -r / --require / bunfig preload scripts run before the entry point. require("module").runMain is a custom accessor on the node:module object; when a preload assigns it, the setter tells the VM (has_patched_run_main) and reload_entry_point then runs the entry point by calling the assigned value instead of loading bun:main itself, mirroring Node, whose bootstrap ends with Module.runMain(mainPath) so that preloads can wrap it. The builtin runMain loads the entry and hands its promise to the VM via setOverrideModuleRunMainPromise.
• reload_entry_point returns the entry point's promise to the run command (run_command.rs) or the worker startup code. If that promise is already rejected, they report the rejection through uncaught_exception, which prints the error or dispatches it to process.on("uncaughtException") listeners with an origin string chosen from entry_point_result.evaluated_as_cjs (uncaughtException for a synchronously thrown CJS entry, unhandledRejection otherwise). Returning Err instead goes to entry_point_load_failed, which only knows the error's name.
• JSC's promise rejection tracker reports a rejected promise as unhandled unless the promise was marked handled before it was rejected; Bun's module loader pre-marks the entry promises it returns, so the caller that inspects them is their only reporter. The new rejected promise follows the same rule.
• LazyProperty<T> is a create-on-first-use slot that can only hold a T* cell, which is why the old setter had to drop primitives. WriteBarrier<Unknown> is a GC-visited slot holding any JSValue; members declared in FOR_EACH_GLOBALOBJECT_GC_MEMBER are visited automatically. Error.prepareStackTrace on the same global object already uses this builtin-in-LazyProperty plus user-value-in-WriteBarrier arrangement.
• JSC::call(globalObject, callee, thisValue, args, message) looks up the callee's CallData (functions, bound functions and callable proxies qualify; primitives, plain objects and strings do not), throws a TypeError with message if there is none, and otherwise performs the call. Returning an empty EncodedJSValue with an exception pending is the convention from_js_host_call checks on the Rust side.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/module/node-module-module.test.js

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 22e362c4-7aa9-4653-8502-8eccc2c2a7aa

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and fb874b0.

📒 Files selected for processing (4)

• src/jsc/VirtualMachine.rs
• src/jsc/bindings/ZigGlobalObject.h
• src/jsc/modules/NodeModuleModule.cpp
• test/js/node/module/node-module-module.test.js

---

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

[robobun]

Status: fix is up for review.

Reproduced on the released build (1.4.0-canary.1, linux x64) with bun -r ./preload.cjs main.js:

• require("module").runMain = {} (or a string) in the preload: panic(main thread): Segmentation fault at address 0x4, exit 139.
• runMain = undefined / null / 5: ignored, main ran.
• a throwing override: only Error occurred loading entry point: JSError printed.

With this branch each case exits 1 with the real error (TypeError: Module.runMain is not a function, or the override's exception), and uncaughtException listeners receive it. Tests: the new Module.runMain ... cases in test/js/node/module/node-module-module.test.js (all fail on the released build, pass with bun bd test).

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks entry-point failure reporting across the C++/Rust FFI boundary and adds a new GC-visited slot on the global object, a human look would still be worthwhile.

Checked: the new WriteBarrier<Unknown> is declared in FOR_EACH_GLOBALOBJECT_GC_MEMBER so it's visited; the removed hasOverriddenModuleRunMain bool has no remaining references; JSInternalPromise is an alias for JSPromise so the Err arm's &mut JSPromise coerces to the annotated *mut JSInternalPromise. Verified the C++ signature switch to EncodedJSValue matches Rust's #[repr(transparent)] JSValue, and that from_js_host_call's zero-is-throw contract is satisfied by RETURN_IF_EXCEPTION(scope, {}). Confirmed reject(.., Err(JsError::Thrown)) takes the pending exception (left set by call_zero_is_throw) and set_handled() before reject prevents a duplicate unhandled-rejection report.

Extended reasoning...

Overview

Fixes a segfault when a preload assigns a non-callable to Module.runMain, and makes a throwing override report through uncaughtException instead of a bare JSError name. Touches four files: ZigGlobalObject.h (new WriteBarrier<Unknown> m_moduleRunMainOverride, retyped m_moduleRunMainFunction to LazyProperty<JSFunction>, dropped hasOverriddenModuleRunMain), NodeModuleModule.cpp (getter/setter now round-trip any JSValue via the new slot; callOverriddenRunMain uses JSC::call(..., message) with a throw scope and the Module object as this; signature moved to EncodedJSValue), VirtualMachine.rs (the Err path now creates a handled+rejected JSPromise and sets evaluated_as_cjs so the run command reports it via uncaught_exception with origin uncaughtException), and 11 new subprocess tests.

Security risks

None. This is Node-compat error handling for a preload's runMain override; no auth, network, filesystem, or untrusted-input parsing. The change turns a crash into a catchable error.

Level of scrutiny

Medium-high. The individual edits are small and follow the existing Error.prepareStackTrace pattern (builtin in a LazyProperty, user value in a WriteBarrier<Unknown>), but they sit on the entry-point bootstrap path across a C++↔Rust FFI boundary with GC-visited state and promise-rejection-tracker semantics. A regression here would affect every process that preloads a runMain wrapper.

Other factors

• Verified JSInternalPromise is a type alias for JSPromise (src/jsc/lib.rs:206), so the Err match arm's &mut JSPromise coerces to the annotated *mut JSInternalPromise and is a valid pending_internal_promise value; the caller (run_command.rs:1441) reads the promise's rejection through the AnyPromise/status_ptr path that dispatches on the shared JSPromise layout.
• Verified from_js_host_call = call_zero_is_throw, which leaves the exception pending and returns Err(JsError::Thrown); JSPromise::reject(.., Err(Thrown)) then try_take_exception()s it, so the exception is consumed exactly once.
• hasOverriddenModuleRunMain has no remaining references anywhere in src/; has_patched_run_main on the VM (set by Bun__VirtualMachine__setOverrideModuleRunMain) is what reload_entry_point still keys on, and the setter still calls it unconditionally.
• The C++ signature change from by-value JSValue to EncodedJSValue aligns with the Rust declaration (argv1: JSValue, #[repr(transparent)] over usize).
• Tests cover the full variant matrix (object/string crash cases, primitive silently-ignored cases, throwing function, class, uncaughtException listener + origin, read-back, restore-original, this === Module) and are subprocess-isolated with pipes drained concurrently.
• Deferring rather than approving because entry-point bootstrap + new GC-visited global-object state is the kind of change a maintainer should sign off on, not because I found anything wrong.

[robobun]

^{Updated 9:44 AM PT - Aug 13th, 2026}

✅ @robobun, your commit fb874b035c6505c0b573ff8084e32caa9f336771 passed in Build #94499! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38119

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

bun-38119 --bun

[claude]

I reviewed this PR and didn't find any bugs. It touches the entry-point load path in VirtualMachine::reload_entry_point and adds a GC-visited slot on ZigGlobalObject, so a human look would still be worthwhile.

What was reviewed:

• Verified m_moduleRunMainOverride is declared under FOR_EACH_GLOBALOBJECT_GC_MEMBER so it is visited, and hasOverriddenModuleRunMain has no remaining references.
• Checked the FFI shape: Rust JSValue is #[repr(transparent)] over the encoded word, matching the C++ side's switch to JSC::EncodedJSValue; from_js_host_call matches the new zero-is-throw contract.
• Confirmed JSInternalPromise aliases JSPromise, so the Err arm's JSPromise::create result unifies with the *mut JSInternalPromise binding; set_handled() is called before reject() as intended.
• Checked JSC::call(..., errorMessage) throws for non-callables and that RETURN_IF_EXCEPTION covers the call; the setter now stores every value (including primitives) and clears the override when the builtin is reassigned.

Extended reasoning...

Overview

This PR fixes a segfault when a preload assigns a non-callable to require("module").runMain, and makes a throwing override report its actual exception (and reach uncaughtException listeners) instead of a bare JSError. It touches four files: VirtualMachine.rs (entry-point load path), ZigGlobalObject.h (a new WriteBarrier<Unknown> slot for the override, retyping the builtin slot to LazyProperty<JSFunction>, and dropping the now-unused hasOverriddenModuleRunMain bool), NodeModuleModule.cpp (getter/setter/call rewritten around the new slot, using JSC::call with an error message and passing the Module object as this), and node-module-module.test.js (11 new subprocess/worker tests).

Security risks

None identified. The change validates a user-supplied override at call time and turns a crash into a catchable TypeError; no new inputs are trusted.

Level of scrutiny

Moderate-to-high. reload_entry_point is on the startup path of every bun run, and ZigGlobalObject.h adds GC-visited state. The changes follow the existing Error.prepareStackTrace pattern (builtin in a LazyProperty, user value in a WriteBarrier<Unknown>), and the C++ exception-scope discipline (DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION) is correct. The Rust side's new Err arm creates a promise, marks it handled, then rejects it — matching how the module loader pre-marks its own entry promises. I confirmed JSInternalPromise is a re-export of JSPromise (src/jsc/lib.rs), so the mixed types in the match unify.

Other factors

• The C++ argv1 parameter changed from JSValue (a class by value across extern "C") to JSC::EncodedJSValue, which now exactly matches the Rust extern's #[repr(transparent)] JSValue — a small ABI hardening.
• The call now passes the Module object as this instead of the global object; this is a behavioral change (matching Node) covered by a new test.
• The comment-cop bot left six automated notes about multi-line comments; those are style-linter noise rather than correctness issues, but the author may want to trim the longer one in the Err arm.
• Test coverage is thorough: non-callable variants, throwing function, class, uncaughtException origin, worker preloads, and round-trip read-back — all subprocess-isolated with pipes drained via Promise.all.

Given the critical-path surface (VM entry-point loading, a new GC-visited member on the global object), I'm deferring rather than auto-approving.

[claude]

I reviewed this PR and didn't find any bugs. Since it reworks error handling on the entry-point load path in VirtualMachine::reload_entry_point and adds a new GC-visited slot on ZigGlobalObject, a human look would still be worthwhile.

What was reviewed:

• hasOverriddenModuleRunMain has no remaining references; removal is safe.
• JSInternalPromise is a re-export of JSPromise (src/jsc/lib.rs), so JSPromise::create coercing to *mut JSInternalPromise in the Err arm type-checks and stores correctly in pending_internal_promise.
• from_js_host_call leaves the exception pending on Err(Thrown), and JSPromise::reject(.., Err(Thrown)) takes it via try_take_exception before rejecting — the exception is not lost or double-reported.
• m_moduleRunMainOverride is declared inside FOR_EACH_GLOBALOBJECT_GC_MEMBER, so it is visited automatically.

Extended reasoning...

Overview

This PR fixes a segfault when a preload assigns a non-callable to Module.runMain, and improves error reporting when a runMain override throws. It touches four files: ZigGlobalObject.h (adds WriteBarrier<Unknown> m_moduleRunMainOverride, retypes m_moduleRunMainFunction to LazyProperty<JSFunction>, removes the unused hasOverriddenModuleRunMain bool), NodeModuleModule.cpp (getter/setter now round-trip any JSValue through the new slot; callOverriddenRunMain uses JSC::call with a TypeError message and follows the zero-is-throw convention; this is now the Module object), VirtualMachine.rs (on exception, wraps it in a handled rejected promise stored as pending_internal_promise with evaluated_as_cjs = true so the run command / worker report it via the normal uncaught-exception path), and 13 new tests covering non-callables, throwing overrides, uncaughtException dispatch, Worker preloads, and value round-tripping.

Security risks

None identified. This is Node-compat error handling for a niche override hook (Module.runMain), only reached when a preload script explicitly reassigns it. No new user-controlled input reaches native parsing, and the change replaces an unchecked downcast+call with a type-checked JSC::call — strictly safer than before.

Level of scrutiny

Moderate-to-high. The C++ side is straightforward (mirrors the existing Error.prepareStackTrace builtin+override pattern on the same global object), but the Rust side alters control flow in reload_entry_point, which every bun run and every Worker startup goes through. The Err arm creates a JSPromise, marks it handled before rejecting, and stores it as the entry promise — I traced this against the callers (load_entry_point, hot-reload reporting, worker startup) and it looks consistent with how the existing paths store and inspect pending_internal_promise, but a maintainer familiar with that machinery should confirm the interaction with --hot and pending_internal_promise_reported_at.

Other factors

The comment-cop bot flagged verbose comments earlier; those were trimmed in fb874b0 and all threads are resolved. Test coverage is thorough (variant matrix over non-callable types, throwing function vs class, uncaughtException listener, Worker preload). CI build #94499 was still running at review time. The PR description notes BUN_JSC_validateExceptionChecks=1 was run against the repro scripts. No prior human review; no CODEOWNERS I could see for these paths.