Fix CommonJS modules silently skipped when require() loads an ESM graph by robobun · Pull Request #37187 · oven-sh/bun

robobun · 2026-08-08T05:55:51Z

Problem

A synchronous require() of an ES module whose registry entry is mid-fetch (a surrounding graph load already started it) threw TypeError: require() async module "..." is unsupported. use "await import()" instead. with no top-level await anywhere. Deterministic doors: require() of an ESM entry whose CJS member requires an in-flight sibling, and an ESM entry whose first import is a CJS file that requires a sibling of the entry.
Worse face: when that TypeError aborted a CJS body mid-graph, the module was evicted from the require cache, and the replayed makeModule in commonJSModuleSyntheticSourceCode (JSCommonJSModule.cpp) built an empty module from the missing entry (JSMap::get returns undefined, truthy as a JSValue). The module's top-level code never ran, its error vanished, exit 0.

Fix

functionEsmLoadSync (ZigGlobalObject.cpp): when the target entry is Fetching with a pending fetch promise, re-issue the fetch synchronously and settle that promise on the nested synchronous module queue before loading, mirroring the replay hostLoadImportedModule does for dependency edges.
The synthetic-source generator throws on a missing require-cache entry instead of fabricating an empty module, and rethrows the recorded evaluation error. Both eviction sites (the generator and JSCommonJSModule::load) record that error on the registry entry.
Verified: test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts (6 tests, all fail on 1.4.2, pass here). Also test/js/bun/resolve, test/js/node/module, test/js/bun/test/mock, bundler_splitting.

Background

require(esm) loads a graph without yielding: VM::m_synchronousModuleQueue diverts the loader's promise reactions to a queue the caller drains inline. Nested requires push nested queues.
A CJS module inside an ESM graph is a synthetic module whose generate step runs the CJS body, so user code (and nested require()) runs while the outer graph is still loading.
An entry created by the outer load has its settle reactions on the outer queue. A nested drain cannot reach them, so reusing its pending fetch promise never completes.

Notes

Original 8-file reproduction (CommonJS entry):

# main.cjs
require("./a.mjs");
# a.mjs
import "./b.mjs"; import "./d.mjs"; import "./e.mjs";
(globalThis.T ??= []).push("a");
console.log("N=" + globalThis.T.length + " " + globalThis.T.join(","));
# b.mjs:  import "./c.cjs"; (globalThis.T ??= []).push("b");
# c.cjs:  require("./e.mjs"); (globalThis.T ??= []).push("c");
# d.mjs:  import "./f.cjs"; (globalThis.T ??= []).push("d");
# e.mjs:  import "./f.cjs"; import "./h.mjs"; (globalThis.T ??= []).push("e");
# f.cjs:  require("./h.mjs"); (globalThis.T ??= []).push("f"); console.log("f.cjs evaluated");
# h.mjs:  (globalThis.T ??= []).push("h");

bun main.cjs  ->  N=6 h,e,c,b,d,a                      (f.cjs never evaluated, 10/10)
node main.cjs ->  f.cjs evaluated / N=7 h,f,e,c,b,d,a

With f.cjs ending in throw new Error("boom"), stock bun still printed N=6 and exited 0. With this change it exits 1 with boom from f.cjs and the right stack, as Node does.

Traced sequence on a debug build: f.cjs's body runs during makeModule and requires h.mjs, which is mid-fetch with its reactions on the outer queue. The nested load reused the pending fetch promise, stayed pending, and was reported as an async module. The TypeError aborted f's body, the generator evicted f from the require cache and rethrew into a reaction that was itself stranded and dropped. A later nested load (c.cjs requiring e.mjs) replayed makeModule for f, found undefined in the require cache, passed the if (entry) check, and produced an empty record. e and d linked against it and the graph "succeeded".

ESM-entry shapes verified against this branch (stock 1.4.2 rates in parentheses), now tests 4 to 6 in the file:

main.mjs: import "./req.cjs"; import d from "./p.mjs" where req.cjs does require("./p.mjs"): ok true, one module record, r.default === d (stock: TypeError 5/5). The esm-first permutation and the permutation with an extra sibling between: 30/30 and 40/40 ok (stock: 0/60 and 3/100 here).
Diamond entry -> a.mjs -> c.mjs, entry -> b.cjs -> require(c.mjs): 40/40 B-req-ok (stock: 42/50 B-req-ERR).
TLA siblings (p1,p2 import t.mjs with await 0) plus k.cjs -> require(s.mjs) without TLA: 40/40 K-req-ok, TLA modules still evaluate after (stock: 17/60 K-req-ERR).
In the diamond and TLA shapes the CJS sibling still evaluates ahead of earlier-listed ESM siblings (B C ... A where Node prints C A B ...). That ordering difference is pre-existing and separate; the tests assert membership, not order.

Other checks: .js-extension flavor of the 8-file graph, NODE_COMPILE_CACHE cold and warm, BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1 bun main.cjs all print N=7. BUN_JSC_validateExceptionChecks=1 over both new paths reports no violations. The debug-only ASSERTION FAILED: module->loadedModules().size() <= loadedModulesCountBefore + 1 on some of these graphs (for example the 8-file graph run as BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1 bun a.mjs) is pre-existing, unchanged here, and addressed by oven-sh/WebKit#396.

Related PRs: #37185 fixes the sibling door where the require target goes through fetchCommonJSModule with already-transpiled source (different file, no conflict); #33184, which pre-settled the same way in functionEsmLoadSync for the concurrent-import race in #33180, was closed into #37185. #33184 alone fixed the silent skip but not the swallowed error. The concurrent-import race test from #33184 passes with this change.

A CommonJS module evaluated mid-load inside a require()d ESM graph can itself require() an ESM sibling whose registry entry is mid-fetch. The reactions that would settle that entry sit on the outer drain's synchronous module queue, which the nested load cannot reach, so the load stayed pending and require() threw a spurious 'require() async module' TypeError. That aborted the CJS body, evicted it from the require cache, and a replayed makeModule then built an empty module from the missing cache entry: the module's top-level code never ran, the error was swallowed, and the process exited 0. esmLoadSync now re-issues the fetch synchronously and settles the entry's fetch promise on its own queue before loading, mirroring the synchronous-replay path hostLoadImportedModule already has for dependency edges. The synthetic module generator no longer fabricates an empty module when the require cache entry is gone (JSMap::get returns undefined, which is truthy as a JSValue, so the old missing-entry branch was unreachable); it rethrows the recorded evaluation error instead.

coderabbitai · 2026-08-08T05:55:58Z

Warning

Review limit reached

Run on-demand review

_{On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file.}

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: f19a61bf-865f-493b-921c-2b53164ff20e

📥 Commits

Reviewing files that changed from the base of the PR and between 62838e1 and 90cdf57.

📒 Files selected for processing (3)

src/jsc/bindings/JSCommonJSModule.cpp
src/jsc/bindings/ZigGlobalObject.cpp
test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts

Walkthrough

The module loader now preserves CommonJS evaluation errors, handles missing cache entries, and manages synchronous ESM queues. Regression tests cover nested CommonJS evaluation order and error propagation.

Changes

Module loading

Layer / File(s)	Summary
CommonJS evaluation error recovery `src/jsc/bindings/JSCommonJSModule.cpp`, `test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts`	CommonJS loading records evaluation errors, handles missing cache entries, and rethrows preserved failures. Tests verify nested error propagation.
Synchronous ESM queue processing `src/jsc/bindings/ZigGlobalObject.cpp`, `test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts`	Synchronous ESM loading re-fetches in-progress entries, settles pending promises, evaluates the module graph, drains the queue, and restores the previous queue. Tests verify evaluation order and successful process results.

Possibly related PRs

oven-sh/bun#35971: Both PRs modify CommonJS and ESM module evaluation paths.
oven-sh/bun#36397: Both PRs preserve and rethrow module evaluation failures.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the primary fix: preventing CommonJS modules from being silently skipped when require() loads an ESM graph.
Description check	✅ Passed	The description explains the problem, fix, technical background, regression coverage, and verification results. It does not use the exact template headings, but it provides the required information an…

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

claude

Beyond the inline nit, I also checked: vm.m_synchronousModuleQueue = syncQueue.prev is restored unconditionally on every path through the new block (all fallible calls are gated on !scope.exception(), and the restore precedes RETURN_IF_EXCEPTION); the new if (error) check on registryEntry->error() does not repeat the JSValue-truthiness bug this PR fixes — ModuleRegistryEntry::error returns an empty JSValue when unset, not jsUndefined(); and setEvaluationError before scope.throwException in the eviction path does not need an intervening exception check (it's a WriteBarrier store, no ThrowScope).

Extended reasoning...

The change touches the core ESM/CJS interop path in the module loader (synchronous module queue nesting, promise reaction ordering, registry entry state) and textually overlaps with two other open PRs (#33184, #37185) in functionEsmLoadSync. That interaction is subtle enough that a human should confirm the pre-settle approach and the coordination with the sibling PRs before merge; the inline nit is debug-validator-only and does not affect release behavior.

robobun · 2026-08-08T06:11:00Z

Addressed the review: the one inline note (exception check between the two ensure* calls) is resolved in-thread with details. Short version: in this fork those helpers are JSPromise::create plus performPromiseThenWithInternalMicrotask, no ThrowScope and no JS entry, matching the same back-to-back shape hostLoadImportedModule uses; and a debug run with BUN_JSC_validateExceptionChecks=1 over both new paths (the repro graph and the throwing-f.cjs variant) reports zero violations, so the code stays as is. No source changes from this review.

github-actions · 2026-08-08T06:12:46Z

This PR may be a duplicate of:

module loader: fix false "require() async module" for a module racing a concurrent import #33184 - Same root cause and same function (functionEsmLoadSync): a registry entry stuck at Status::Fetching cannot be settled from a nested sync load, so a non-async module is misreported as require() async module ... is unsupported, and both fix it with the same synchronous re-fetch + pre-settle of the entry's fetch promise.
Fix spurious "require() async module" TypeError when require() hits an ESM module the import graph is still fetching #37185 - Same spurious require() async module TypeError for a mid-flight, non-async ESM sibling, fixed with the same pre-settle-the-fetch-promise mechanic at the sibling entry point fetchCommonJSModule in ModuleLoader.cpp.

🤖 Generated with Claude Code

robobun · 2026-08-08T06:13:04Z

Not a duplicate, though the overlap is real and is called out in the PR body ("Relation to open PRs in this area").

Summary of the relationship, verified by running each change against this PR's repros:

module loader: fix false "require() async module" for a module racing a concurrent import #33184 applies the same pre-settle technique in the same function for the concurrent-import race (Bun 1.3.14 regression: require() async module error for jose via jwks-rsa/firebase-admin (works fine on 1.3.11) #33180). It does fix this PR's silent-skip repro, but not the swallowed-error half: with module loader: fix false "require() async module" for a module racing a concurrent import #33184 alone, a CommonJS module that throws inside a require()d ESM graph still evaluates to an empty module and the process exits 0 with the error lost. That half lives in the synthetic module generator (JSCommonJSModule.cpp) and is unique to this PR. The two functionEsmLoadSync hunks conflict textually; whichever PR lands second rebases that hunk, and this PR's generator half is unaffected by the order.
Fix spurious "require() async module" TypeError when require() hits an ESM module the import graph is still fetching #37185 fixes a different entry point (fetchCommonJSModule in ModuleLoader.cpp, for require targets that arrive with source already transpiled). Different file, no conflict, complementary door into the same underlying gap.

Conversely, this PR's tests fail on both of those branches' behavior for the error-swallow case, so neither supersedes it.

robobun · 2026-08-08T06:35:09Z

^{Updated 5:16 PM PT - Sep 7th, 2026}

❌ @Jarred-Sumner, your commit 90cdf57 has 4 failures in Build #112195 (All Failures):

test/cli/run/require-cache.test.ts - code 1 on 🐧 13 x64-asan
test/js/workerd/html-rewriter-leak.test.ts - code 1: SyntaxError: JSON Parse error: Unexpected EOF on 🐧 13 x64-asan
test/js/node/vm/sourcetextmodule-leak.test.ts - code 1 on 🐧 13 x64-asan
test/js/node/test/parallel/test-crypto-dh-leak.js - code 1 on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 37187

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

bun-37187 --bun

A CJS sibling evaluated via require() from another CJS module's body (JSCommonJSModule::load) that throws was evicted from the require cache without recording its error on the module's registry entry, so the replayed makeModule for its import edge surfaced the generic removed- from-cache message instead of the user's error. Mirror the generator path's setEvaluationError there; plain CJS require has no registry entry, so this is a no-op outside ESM graphs.

robobun · 2026-08-08T07:22:21Z

CI status: the diff is green across lanes. The only red job in the last two runs (builds 90533 and 90541) is test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts on the x64-asan lane, a pre-existing intermittent abort that test was added to hunt (its header documents the SIGABRT as asan-lane-only and predating this branch; it exercises worker terminate plus builtin require, none of which touches this PR's file-module loading paths). It has been reported for main-side triage. The other entries passed on retry.

Ready for review. Summary of what landed since the PR opened: the original two fixes, plus a reviewer-found sibling (JSCommonJSModule::load eviction now records the evaluation error the same way the generator path does, so a try/catch-swallowed require() of a throwing CJS sibling fails its import edge with the real error). Three tests, each verified to fail without its fix.

robobun · 2026-08-13T01:45:09Z

Heads-up from consolidating the require() async module PRs: #33184 was closed in favour of #37185, which settles the mid-fetch entry from fetchCommonJSModule rather than from functionEsmLoadSync. On a build of current main plus #37185 (be872ba plus its ModuleLoader.cpp change), the first test in this PR (require() of an ESM entry evaluates every CommonJS module in the graph) passes, while the other two (error propagation through the require cache) still fail, so the JSCommonJSModule.cpp half here is independent and this PR stays open for it. Once #37185 lands, the functionEsmLoadSync hunk here should be droppable; it will conflict textually in the meantime.

…-esm-nested-cjs-skip

Three more doors into the same gap, all from an ESM entry rather than a require()d one: a CJS dependency listed first that require()s an ESM sibling of the entry (deterministic TypeError before), a diamond where a CJS sibling require()s what an ESM sibling imports, and the same with top-level-await siblings elsewhere in the graph (both timing-dependent before, roughly 4/5 and 1/4 of runs). All pass with this branch.

robobun · 2026-09-07T06:47:11Z

Pushed e6833fa: merged current main (the branch had drifted ~1000 commits; JSModuleLoader::fetch gained a referrer parameter, now passed), and added three ESM-entry shapes of the same gap as tests, from fuzz-ledger reports that still reproduce on 1.4.2 and canary:

import "./req.cjs"; import d from "./p.mjs" where req.cjs require()s p.mjs: deterministic TypeError on 1.4.2, ok true with one module record here (require() and import see the same namespace).
Diamond (a CJS sibling require()s what an ESM sibling imports): 1.4.2 fails about 4 in 5 runs, 40/40 here.
The same with top-level-await siblings elsewhere in the graph, requiring a non-TLA module: 1.4.2 fails about 1 in 4, 40/40 here, and the TLA modules still evaluate afterwards.

All six tests in the file fail on 1.4.2 for the reported reason and pass on this branch. PR body rewritten to the current state; details and rates are in its Notes.

claude

Code review found no issues

No high-confidence issues detected in this change.

claude

Code review found no issues

No high-confidence issues detected in this change.

robobun assigned Jarred-Sumner Aug 8, 2026

github-actions Bot added the claude label Aug 8, 2026

robobun mentioned this pull request Aug 8, 2026

module loader: fix false "require() async module" for a module racing a concurrent import #33184

Closed

claude Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread src/jsc/bindings/ZigGlobalObject.cpp

ci: retrigger

19696e9