From 2459522efb4342207200b4bca5d6c565174c1b27 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 8 Aug 2026 05:55:36 +0000 Subject: [PATCH 1/5] Fix CommonJS modules silently skipped when require() loads an ESM graph 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. --- src/jsc/bindings/JSCommonJSModule.cpp | 77 ++++++++++++------- src/jsc/bindings/ZigGlobalObject.cpp | 42 +++++++++- .../require-esm-nested-cjs-sibling.test.ts | 74 ++++++++++++++++++ 3 files changed, 166 insertions(+), 27 deletions(-) create mode 100644 test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts diff --git a/src/jsc/bindings/JSCommonJSModule.cpp b/src/jsc/bindings/JSCommonJSModule.cpp index f849adacd02c..5c58cf2057fd 100644 --- a/src/jsc/bindings/JSCommonJSModule.cpp +++ b/src/jsc/bindings/JSCommonJSModule.cpp @@ -63,6 +63,8 @@ #include #include #include "ModuleLoader.h" +#include +#include #include #include @@ -1582,35 +1584,58 @@ static JSC::SourceCode commonJSModuleSyntheticSourceCode(const SourceOrigin& sou JSValue entry = globalObject->requireMap()->get(globalObject, keyValue); RETURN_IF_EXCEPTION(scope, {}); - if (entry) { - if (auto* moduleObject = dynamicDowncast(entry)) { - if (!moduleObject->hasEvaluated) { - evaluateCommonJSModuleOnce( - vm, - globalObject, - moduleObject, - moduleObject->m_dirname.get(), - moduleObject->m_filename.get()); - if (auto exception = scope.exception()) { - if (vm.hasPendingTerminationException()) [[unlikely]] - return; - (void)scope.tryClearException(); - - // On error, remove the module from the require map - // so that it can be re-evaluated on the next require. - globalObject->requireMap()->remove(globalObject, moduleObject->filename()); - RETURN_IF_EXCEPTION(scope, {}); - - scope.throwException(globalObject, exception); - return; - } + // JSMap::get returns undefined (a truthy JSValue) for a + // missing key. An entry that vanished between + // createCommonJSModule() and this makeModule step means an + // earlier evaluation attempt failed and was evicted (below); + // producing an empty module here would silently skip the + // module's side effects and swallow that error. Rethrow the + // original error when the registry kept it. + if (entry.isUndefinedOrNull()) [[unlikely]] { + if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(moduleKey)) { + JSValue error = registryEntry->error(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + if (error) { + scope.throwException(globalObject, error); + return; } + } + throwException(globalObject, scope, createError(globalObject, makeString("Module \""_s, StringView(moduleKey.string()), "\" was removed from the require cache while it was being loaded."_s))); + return; + } - moduleObject->toSyntheticSource(globalObject, moduleKey, exportNames, exportValues); - RETURN_IF_EXCEPTION(scope, {}); + if (auto* moduleObject = dynamicDowncast(entry)) { + if (!moduleObject->hasEvaluated) { + evaluateCommonJSModuleOnce( + vm, + globalObject, + moduleObject, + moduleObject->m_dirname.get(), + moduleObject->m_filename.get()); + if (auto exception = scope.exception()) { + if (vm.hasPendingTerminationException()) [[unlikely]] + return; + (void)scope.tryClearException(); + + // On error, remove the module from the require map + // so that it can be re-evaluated on the next require. + globalObject->requireMap()->remove(globalObject, moduleObject->filename()); + RETURN_IF_EXCEPTION(scope, {}); + + // The rejection this throw feeds can be dropped when + // its reaction is stranded on an outer synchronous + // module queue; keep the error reachable for a + // replayed makeModule of this same module (above). + if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(moduleKey)) + registryEntry->setEvaluationError(globalObject, exception->value()); + + scope.throwException(globalObject, exception); + return; + } } - } else { - // require map was cleared of the entry + + moduleObject->toSyntheticSource(globalObject, moduleKey, exportNames, exportValues); + RETURN_IF_EXCEPTION(scope, {}); } }, sourceOrigin, diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 04724225b631..552937d23191 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -825,7 +825,47 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob } } - JSPromise* promise = loader->loadModuleSync(globalObject, key, nullptr, nullptr); + // Inlined JSModuleLoader::loadModuleSync with one extra step: when this + // module's registry entry is mid-fetch (a surrounding ESM graph load + // already started fetching it), the reactions that would settle its fetch + // promise are queued on the *outer* drain's synchronous module queue, + // which this nested load cannot reach. loadModule() would reuse that + // pending fetch promise, the load would never complete, and require() + // would throw a spurious "async module" TypeError. Re-issue the fetch + // synchronously and settle the entry's fetch promise while our queue is + // current, so the whole chain drains here (mirrors the synchronous-replay + // path in JSModuleLoader::hostLoadImportedModule for dependency edges). + JSC::VM::SynchronousModuleQueue syncQueue; + syncQueue.prev = vm.m_synchronousModuleQueue; + vm.m_synchronousModuleQueue = &syncQueue; + + if (auto* entry = loader->registryEntry(key)) { + if (entry->status() == JSC::ModuleRegistryEntry::Status::Fetching) { + // Guarantees the fetch-settled reaction is attached to the fetch + // promise (a no-op for entries hostLoadImportedModule created). + entry->ensureModulePromise(globalObject); + JSPromise* fetchPromise = entry->ensureFetchPromise(globalObject); + if (!scope.exception() && fetchPromise->status() == JSPromise::Status::Pending) { + JSPromise* fetched = loader->fetch(globalObject, JSC::jsString(vm, keyString), nullptr, nullptr); + if (!scope.exception()) { + // The async path pipeFrom()'d this promise, which set the + // first-resolving-function flag, so the guarded + // fulfill()/reject() would no-op; settle it directly. + if (fetched->status() == JSPromise::Status::Fulfilled) + fetchPromise->fulfillPromise(vm, fetched->result()); + else if (fetched->status() == JSPromise::Status::Rejected) + fetchPromise->rejectPromise(vm, fetched->result()); + } + } + } + } + + JSPromise* promise = nullptr; + if (!scope.exception()) + promise = loader->loadModule(globalObject, key, nullptr, nullptr, { JSC::ModuleLoadFlag::Evaluate }); + if (!scope.exception()) + JSC::JSModuleLoader::drainSynchronousModuleQueue(globalObject); + vm.m_synchronousModuleQueue = syncQueue.prev; RETURN_IF_EXCEPTION(scope, {}); switch (promise->status()) { diff --git a/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts new file mode 100644 index 000000000000..c094147e9eca --- /dev/null +++ b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts @@ -0,0 +1,74 @@ +// When a CommonJS entry require()s an ESM graph, the whole graph loads on the +// loader's synchronous module queue. A CJS module inside that graph has its +// body evaluated mid-load, and that body can require() an ESM sibling whose +// registry entry is mid-fetch: the reactions that would settle it sit on the +// *outer* drain's queue, which the nested synchronous load cannot reach. +// +// That used to have two faces: +// 1. require() of the in-flight sibling threw a spurious +// `require() async module "..." is unsupported` TypeError even though the +// sibling has no top-level await. +// 2. The TypeError aborted the CJS module's body, which evicted it from the +// require cache; a replayed makeModule then found no cache entry and +// silently produced an *empty* module, so the CJS module's top-level code +// never ran at all: no throw, exit 0, one module of the graph skipped. +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +const graph = { + "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");`, +}; + +test.concurrent("require() of an ESM entry evaluates every CommonJS module in the graph", async () => { + using dir = tempDir("require-esm-nested-cjs", graph); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + // Same evaluation order Node prints. Before the fix, f.cjs was silently + // skipped: "N=6 h,e,c,b,d,a" and no "f.cjs evaluated" line. + expect(stdout).toBe("f.cjs evaluated\nN=7 h,f,e,c,b,d,a\n"); + expect(exitCode).toBe(0); +}); + +test.concurrent("a CommonJS module throwing inside a require()d ESM graph surfaces the error", async () => { + using dir = tempDir("require-esm-nested-cjs-throw", { + ...graph, + "f.cjs": `require("./h.mjs"); +throw new Error("boom from f.cjs");`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Before the fix this printed "N=6 h,e,c,b,d,a" and exited 0: the throw was + // swallowed along with the module. + expect(stderr).toContain("boom from f.cjs"); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); +}); From 19696e9ecf77513bb114c2494ac3c7fb2971a7cd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 8 Aug 2026 06:32:53 +0000 Subject: [PATCH 2/5] ci: retrigger From 830664e4045f51e675b9a4a907036c5140daddde Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 8 Aug 2026 06:34:58 +0000 Subject: [PATCH 3/5] Tighten loader comments --- src/jsc/bindings/JSCommonJSModule.cpp | 18 +++++++----------- src/jsc/bindings/ZigGlobalObject.cpp | 24 +++++++++--------------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/src/jsc/bindings/JSCommonJSModule.cpp b/src/jsc/bindings/JSCommonJSModule.cpp index 5c58cf2057fd..6611d09e6418 100644 --- a/src/jsc/bindings/JSCommonJSModule.cpp +++ b/src/jsc/bindings/JSCommonJSModule.cpp @@ -1584,13 +1584,10 @@ static JSC::SourceCode commonJSModuleSyntheticSourceCode(const SourceOrigin& sou JSValue entry = globalObject->requireMap()->get(globalObject, keyValue); RETURN_IF_EXCEPTION(scope, {}); - // JSMap::get returns undefined (a truthy JSValue) for a - // missing key. An entry that vanished between - // createCommonJSModule() and this makeModule step means an - // earlier evaluation attempt failed and was evicted (below); - // producing an empty module here would silently skip the - // module's side effects and swallow that error. Rethrow the - // original error when the registry kept it. + // JSMap::get returns undefined, which is truthy as a JSValue, + // for a missing key. A vanished entry means an evaluation + // attempt failed and was evicted (below); an empty module here + // would silently drop the module's side effects and its error. if (entry.isUndefinedOrNull()) [[unlikely]] { if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(moduleKey)) { JSValue error = registryEntry->error(globalObject); @@ -1622,10 +1619,9 @@ static JSC::SourceCode commonJSModuleSyntheticSourceCode(const SourceOrigin& sou globalObject->requireMap()->remove(globalObject, moduleObject->filename()); RETURN_IF_EXCEPTION(scope, {}); - // The rejection this throw feeds can be dropped when - // its reaction is stranded on an outer synchronous - // module queue; keep the error reachable for a - // replayed makeModule of this same module (above). + // Keep the error reachable for a replayed makeModule + // (above); this throw's rejection can strand on an + // outer synchronous module queue and get dropped. if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(moduleKey)) registryEntry->setEvaluationError(globalObject, exception->value()); diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 552937d23191..5719e1b28a23 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -825,32 +825,26 @@ JSC_DEFINE_HOST_FUNCTION(functionEsmLoadSync, (JSC::JSGlobalObject * lexicalGlob } } - // Inlined JSModuleLoader::loadModuleSync with one extra step: when this - // module's registry entry is mid-fetch (a surrounding ESM graph load - // already started fetching it), the reactions that would settle its fetch - // promise are queued on the *outer* drain's synchronous module queue, - // which this nested load cannot reach. loadModule() would reuse that - // pending fetch promise, the load would never complete, and require() - // would throw a spurious "async module" TypeError. Re-issue the fetch - // synchronously and settle the entry's fetch promise while our queue is - // current, so the whole chain drains here (mirrors the synchronous-replay - // path in JSModuleLoader::hostLoadImportedModule for dependency edges). + // Inlined JSModuleLoader::loadModuleSync plus one extra step. A mid-fetch + // entry's settle reactions live on the *outer* drain's synchronous module + // queue, which this nested load cannot reach, so reusing its pending fetch + // promise would misreport the module as async. Re-fetch and settle the + // entry while our queue is current, mirroring hostLoadImportedModule's + // synchronous-replay path for dependency edges. JSC::VM::SynchronousModuleQueue syncQueue; syncQueue.prev = vm.m_synchronousModuleQueue; vm.m_synchronousModuleQueue = &syncQueue; if (auto* entry = loader->registryEntry(key)) { if (entry->status() == JSC::ModuleRegistryEntry::Status::Fetching) { - // Guarantees the fetch-settled reaction is attached to the fetch - // promise (a no-op for entries hostLoadImportedModule created). + // Attaches the fetch-settled reaction if nothing has yet. entry->ensureModulePromise(globalObject); JSPromise* fetchPromise = entry->ensureFetchPromise(globalObject); if (!scope.exception() && fetchPromise->status() == JSPromise::Status::Pending) { JSPromise* fetched = loader->fetch(globalObject, JSC::jsString(vm, keyString), nullptr, nullptr); if (!scope.exception()) { - // The async path pipeFrom()'d this promise, which set the - // first-resolving-function flag, so the guarded - // fulfill()/reject() would no-op; settle it directly. + // pipeFrom() already claimed the resolving-function flag, so + // the guarded fulfill()/reject() would no-op; settle directly. if (fetched->status() == JSPromise::Status::Fulfilled) fetchPromise->fulfillPromise(vm, fetched->result()); else if (fetched->status() == JSPromise::Status::Rejected) From b13214c9d5c6cb08735c79b78ed1d604a929e4f2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 8 Aug 2026 07:03:49 +0000 Subject: [PATCH 4/5] Record evaluation errors from JSCommonJSModule::load eviction too 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. --- src/jsc/bindings/JSCommonJSModule.cpp | 10 +++++++ .../require-esm-nested-cjs-sibling.test.ts | 26 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/jsc/bindings/JSCommonJSModule.cpp b/src/jsc/bindings/JSCommonJSModule.cpp index 6611d09e6418..51224ac9ed66 100644 --- a/src/jsc/bindings/JSCommonJSModule.cpp +++ b/src/jsc/bindings/JSCommonJSModule.cpp @@ -269,6 +269,16 @@ bool JSCommonJSModule::load(JSC::VM& vm, Zig::GlobalObject* globalObject) bool wasRemoved = globalObject->requireMap()->remove(globalObject, this->filename()); ASSERT(wasRemoved); + // Keep the error reachable for a replayed makeModule of this module's + // ESM synthetic source, matching commonJSModuleSyntheticSourceCode's + // error path. Plain CJS require never registers with the module + // loader, so this is a no-op there. + auto filenameString = this->filename().toWTFString(globalObject); + if (scope.exception()) [[unlikely]] + (void)scope.tryClearException(); + else if (auto* registryEntry = globalObject->moduleLoader()->registryEntry(JSC::Identifier::fromString(vm, filenameString))) + registryEntry->setEvaluationError(globalObject, exception->value()); + scope.throwException(globalObject, exception); return false; } diff --git a/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts index c094147e9eca..a383cb88f4f1 100644 --- a/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts +++ b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts @@ -72,3 +72,29 @@ throw new Error("boom from f.cjs");`, expect(stdout).toBe(""); expect(exitCode).toBe(1); }); + +test.concurrent("a caught require() of a throwing CJS sibling still fails its import edge with the original error", async () => { + using dir = tempDir("require-esm-nested-cjs-caught", { + "main.cjs": `require("./a.mjs");`, + "a.mjs": `import "./e.mjs"; +console.log("a evaluated");`, + "e.mjs": `import "./g.cjs"; +import "./f.cjs";`, + // g.cjs evaluates f.cjs first (both are in the require cache before either + // runs) and swallows the throw; the import edge of f.cjs must still reject + // with f's real error, not succeed with an empty module. + "g.cjs": `try { require("./f.cjs"); } catch {}`, + "f.cjs": `throw new Error("boom from f.cjs");`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Before the fix: "a evaluated" and exit 0, with f.cjs silently skipped. + expect(stderr).toContain("boom from f.cjs"); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); +}); From 5a03dbedf4b2dfac10aa2323d83972c2e6b57f51 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 07:06:21 +0000 Subject: [PATCH 5/5] [autofix.ci] apply automated fixes --- .../require-esm-nested-cjs-sibling.test.ts | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts index a383cb88f4f1..164b5da61b05 100644 --- a/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts +++ b/test/js/bun/resolve/require-esm-nested-cjs-sibling.test.ts @@ -73,28 +73,31 @@ throw new Error("boom from f.cjs");`, expect(exitCode).toBe(1); }); -test.concurrent("a caught require() of a throwing CJS sibling still fails its import edge with the original error", async () => { - using dir = tempDir("require-esm-nested-cjs-caught", { - "main.cjs": `require("./a.mjs");`, - "a.mjs": `import "./e.mjs"; +test.concurrent( + "a caught require() of a throwing CJS sibling still fails its import edge with the original error", + async () => { + using dir = tempDir("require-esm-nested-cjs-caught", { + "main.cjs": `require("./a.mjs");`, + "a.mjs": `import "./e.mjs"; console.log("a evaluated");`, - "e.mjs": `import "./g.cjs"; + "e.mjs": `import "./g.cjs"; import "./f.cjs";`, - // g.cjs evaluates f.cjs first (both are in the require cache before either - // runs) and swallows the throw; the import edge of f.cjs must still reject - // with f's real error, not succeed with an empty module. - "g.cjs": `try { require("./f.cjs"); } catch {}`, - "f.cjs": `throw new Error("boom from f.cjs");`, - }); - await using proc = Bun.spawn({ - cmd: [bunExe(), "main.cjs"], - env: bunEnv, - cwd: String(dir), - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - // Before the fix: "a evaluated" and exit 0, with f.cjs silently skipped. - expect(stderr).toContain("boom from f.cjs"); - expect(stdout).toBe(""); - expect(exitCode).toBe(1); -}); + // g.cjs evaluates f.cjs first (both are in the require cache before either + // runs) and swallows the throw; the import edge of f.cjs must still reject + // with f's real error, not succeed with an empty module. + "g.cjs": `try { require("./f.cjs"); } catch {}`, + "f.cjs": `throw new Error("boom from f.cjs");`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Before the fix: "a evaluated" and exit 0, with f.cjs silently skipped. + expect(stderr).toContain("boom from f.cjs"); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); + }, +);