Fix CommonJS modules silently skipped when require() loads an ESM graph

src/jsc/bindings/JSCommonJSModule.cpp

@@ -63,6 +63,8 @@
 #include <JavaScriptCore/DFGAbstractHeap.h>
 #include <JavaScriptCore/Completion.h>
 #include "ModuleLoader.h"
+#include <JavaScriptCore/JSModuleLoader.h>
+#include <JavaScriptCore/ModuleRegistryEntry.h>
 #include <JavaScriptCore/JSMap.h>
 
 #include <JavaScriptCore/JSMapInlines.h>
@@ -1582,35 +1584,58 @@
                 JSValue entry = globalObject->requireMap()->get(globalObject, keyValue);
                 RETURN_IF_EXCEPTION(scope, {});
 
-                if (entry) {
-                    if (auto* moduleObject = dynamicDowncast<JSCommonJSModule>(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<JSCommonJSModule>(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,

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()) {

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);
+});