Report a module's build errors to every later load of it (WebKit bump for oven-sh/WebKit#451)

src/jsc/bindings/JSCommonJSExtensions.cpp

@@ -264,6 +264,8 @@ JSC::EncodedJSValue builtinLoader(JSC::JSGlobalObject* globalObject, JSC::CallFr
     res.success = false;
     memset(&res.result, 0, sizeof res.result);
 
+    evictFetchFailedModuleRegistryEntry(global->moduleLoader(), JSC::Identifier::fromString(vm, specifierWtfString));
+
     JSValue result = fetchCommonJSModuleNonBuiltin<true>(
         global->bunVM(),
         vm,

src/jsc/bindings/ModuleLoader.cpp

@@ -655,6 +655,37 @@ void evaluateCommonJSCustomExtension(
     RETURN_IF_EXCEPTION(scope, );
 }
 
+// JSC settles every later load of a FetchFailed key with JSModuleLoader::duplicateError's
+// copy of the stored error, which keeps only its type and message (an AggregateError of
+// build errors loses `errors`). Such an entry holds no module record, so dropping it only
+// makes the next load fetch the module again, as Node does; link and evaluation failures
+// hold a record and stay cached as the spec requires.
+void evictFetchFailedModuleRegistryEntry(JSC::JSModuleLoader* moduleLoader, const JSC::Identifier& key)
+{
+    using Type = JSC::ScriptFetchParameters::Type;
+    // JavaScript first so that the common case, a loaded module, stops after one lookup.
+    static constexpr Type types[] = { Type::JavaScript, Type::None, Type::JSON, Type::WebAssembly, Type::HostDefined };
+
+    // removeEntry() drops every type variant of the key, so all of them must have failed.
+    auto& moduleMap = moduleLoader->moduleMap();
+    bool fetchFailed = false;
+    for (Type type : types) {
+        auto entry = moduleMap.get({ key.impl(), type });
+        if (!entry)
+            continue;
+        if (entry->status() != JSC::ModuleRegistryEntry::Status::FetchFailed)
+            return;
+        fetchFailed = true;
+    }
+    if (!fetchFailed)
+        return;
+
+    // JSModuleLoader::visitChildrenImpl iterates these maps on the GC thread
+    // under cellLock(); take the same lock so the removal can't race it.
+    WTF::Locker locker { moduleLoader->cellLock() };
+    moduleLoader->removeEntry(key);
+}
+
 JSValue fetchCommonJSModule(
     Zig::GlobalObject* globalObject,
     JSCommonJSModule* target,
@@ -675,6 +706,10 @@ JSValue fetchCommonJSModule(
 
     BunString specifier = Bun::toString(specifierWtfString);
 
+    // Before the virtual module branches too: their provideFetch() is a no-op on a failed entry.
+    auto moduleKey = JSC::Identifier::fromString(vm, specifierWtfString);
+    evictFetchFailedModuleRegistryEntry(globalObject->moduleLoader(), moduleKey);
+
     bool wasModuleMock = false;
 
     // When "bun test" is enabled, allow users to override builtin modules
@@ -788,7 +823,7 @@ JSValue fetchCommonJSModule(
     }
 
     bool hasAlreadyLoadedESMVersionSoWeShouldntTranspileItTwice = [&]() -> bool {
-        auto* entry = globalObject->moduleLoader()->registryEntry(JSC::Identifier::fromString(vm, specifierWtfString));
+        auto* entry = globalObject->moduleLoader()->registryEntry(moduleKey);
         return entry && entry->status() >= JSC::ModuleRegistryEntry::Status::Fetched;
     }();
 

src/jsc/bindings/ModuleLoader.h

@@ -16,6 +16,7 @@ class GlobalObject;
 
 namespace JSC {
 class JSPromise;
+class JSModuleLoader;
 }
 
 namespace Bun {
@@ -105,6 +106,10 @@ JSValue fetchCommonJSModule(
     BunString* referrer,
     BunString* typeAttribute);
 
+// Call right before the loader looks `key` up for a load, so that a fetch that
+// failed earlier is retried instead of replayed.
+void evictFetchFailedModuleRegistryEntry(JSC::JSModuleLoader* moduleLoader, const JSC::Identifier& key);
+
 template<bool isExtension>
 JSValue fetchCommonJSModuleNonBuiltin(
     void* bunVM,

src/jsc/bindings/ZigGlobalObject.cpp

@@ -3619,12 +3619,8 @@ extern "C" void JSC__JSGlobalObject__queueMicrotaskCallback(Zig::GlobalObject* g
     globalObject->vm().queueMicrotask(WTF::move(task));
 }
 
-JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject,
-    JSModuleLoader* loader, JSValue key,
-    JSValue referrer, RefPtr<JSC::ScriptFetcher>, bool)
+static JSC::Identifier resolveModuleSpecifier(Zig::GlobalObject* globalObject, JSValue key, JSValue referrer)
 {
-    Zig::GlobalObject* globalObject = static_cast<Zig::GlobalObject*>(jsGlobalObject);
-
     ErrorableString res;
     res.success = false;
 
@@ -3704,6 +3700,21 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject
     }
 }
 
+JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject,
+    JSModuleLoader* loader, JSValue key,
+    JSValue referrer, RefPtr<JSC::ScriptFetcher>, bool)
+{
+    Zig::GlobalObject* globalObject = static_cast<Zig::GlobalObject*>(jsGlobalObject);
+    auto scope = DECLARE_THROW_SCOPE(globalObject->vm());
+
+    JSC::Identifier resolved = resolveModuleSpecifier(globalObject, key, referrer);
+    RETURN_IF_EXCEPTION(scope, resolved);
+
+    // The only host hook that runs before the registry lookup for static imports as well as import().
+    Bun::evictFetchFailedModuleRegistryEntry(loader, resolved);
+    return resolved;
+}
+
 JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalObject,
     JSModuleLoader*,
     JSString* moduleNameValue,

test/js/bun/resolve/build-error.test.ts

@@ -1,3 +1,4 @@
+import { describe, expect, test } from "bun:test";
 import { bunEnv, bunExe, tempDir } from "harness";
 import { join } from "node:path";
 
@@ -88,3 +89,218 @@ test("BuildMessage finalize frees with the same allocator it was created with",
     Bun.gc(true);
   }
 });
+
+// A module whose build failed has no module record, but the module loader kept
+// its registry entry and settled every later load of it with a copy of the
+// stored error that only had the error's type and message: the second import()
+// (or require(), or another module importing it) got an AggregateError without
+// `errors`, which is also what made `bun test` crash on the second test file
+// importing a broken module (#36963). The entry is now dropped before the next
+// load, so the module is built again and, as in Node, every importer gets the
+// complete error of its own attempt, or the module once the file is fixed.
+describe.concurrent("loading a module again after it failed to build", () => {
+  // Three declarations of the same const produce exactly two build errors.
+  const twoBuildErrors = `const dup = 1; const dup = 2; const dup = 3;\n`;
+  // The stripped copy has the same name and message; `errors` tells them apart.
+  const shape = /* js */ `
+    const shape = e => ({
+      name: e.constructor.name,
+      message: e.message,
+      errors: e.errors ? e.errors.map(error => error.name) : null,
+    });
+  `;
+  const aggregateOfTwo = {
+    name: "AggregateError",
+    message: expect.stringMatching(/^2 errors building "/),
+    errors: ["BuildMessage", "BuildMessage"],
+  };
+
+  async function runEntry(files: Record<string, string>) {
+    using dir = tempDir("reload-failed-build", files);
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "entry.js"],
+      env: bunEnv,
+      cwd: String(dir),
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(stderr).toBe("");
+    expect(exitCode).toBe(0);
+    return JSON.parse(stdout);
+  }
+
+  test("import() and require() report the build errors on every load", async () => {
+    const result = await runEntry({
+      "bad.js": twoBuildErrors,
+      "entry.js": /* js */ `
+        ${shape}
+        const out = {};
+        out.firstImport = await import("./bad.js").then(() => "loaded", shape);
+        out.secondImport = await import("./bad.js").then(() => "loaded", shape);
+        try {
+          require("./bad.js");
+          out.require = "loaded";
+        } catch (e) {
+          out.require = shape(e);
+        }
+        console.log(JSON.stringify(out));
+      `,
+    });
+    expect(result).toEqual({ firstImport: aggregateOfTwo, secondImport: aggregateOfTwo, require: aggregateOfTwo });
+  });
+
+  test("every module importing it reports the build errors", async () => {
+    const result = await runEntry({
+      "bad.js": twoBuildErrors,
+      "a.js": `import "./bad.js";`,
+      "b.js": `import "./bad.js";`,
+      "entry.js": /* js */ `
+        ${shape}
+        const out = {};
+        out.a = await import("./a.js").then(() => "loaded", shape);
+        out.b = await import("./b.js").then(() => "loaded", shape);
+        console.log(JSON.stringify(out));
+      `,
+    });
+    expect(result).toEqual({ a: aggregateOfTwo, b: aggregateOfTwo });
+  });
+
+  test("bun test prints the build errors for every test file importing it", async () => {
+    using dir = tempDir("reload-failed-build-test", {
+      "bad.js": twoBuildErrors,
+      "a.test.js": `import "./bad.js";`,
+      "b.test.js": `import "./bad.js";`,
+    });
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "test", "./a.test.js", "./b.test.js"],
+      env: bunEnv,
+      cwd: String(dir),
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    const output = stdout + stderr;
+    expect(output.split('error: "dup" has already been declared')).toHaveLength(1 + 2 * 2);
+    expect(output).toContain("Ran 2 tests across 2 files.");
+    expect(exitCode).toBe(1);
+  });
+
+  test("the module is loaded once the file is fixed", async () => {
+    const result = await runEntry({
+      "via-import.js": twoBuildErrors,
+      "via-require.js": twoBuildErrors,
+      "via-extension.js": twoBuildErrors,
+      // Loads via-extension.js into this module object the way a hijacked
+      // require.extensions handler would, so require() of this file returns it.
+      "load-via-extension.cjs": `require.extensions[".js"](module, require.resolve("./via-extension.js"));`,
+      "entry.js": /* js */ `
+        ${shape}
+        import { writeFileSync } from "node:fs";
+        const fix = name => writeFileSync(import.meta.dir + "/" + name, "export const loadedBy = " + JSON.stringify(name) + ";");
+        const attempt = async load => {
+          try {
+            return (await load()).loadedBy;
+          } catch (e) {
+            return shape(e);
+          }
+        };
+        const out = {};
+
+        out.importBefore = await attempt(() => import("./via-import.js"));
+        fix("via-import.js");
+        out.importAfter = await attempt(() => import("./via-import.js"));
+
+        out.requireBefore = await attempt(() => import("./via-require.js"));
+        fix("via-require.js");
+        out.requireAfter = await attempt(() => require("./via-require.js"));
+
+        out.extensionBefore = await attempt(() => import("./via-extension.js"));
+        fix("via-extension.js");
+        out.extensionAfter = await attempt(() => require("./load-via-extension.cjs"));
+
+        console.log(JSON.stringify(out));
+      `,
+    });
+    expect(result).toEqual({
+      importBefore: aggregateOfTwo,
+      importAfter: "via-import.js",
+      requireBefore: aggregateOfTwo,
+      requireAfter: "via-require.js",
+      extensionBefore: aggregateOfTwo,
+      extensionAfter: "via-extension.js",
+    });
+  });
+
+  test("a failed build does not unload the same file imported with another type", async () => {
+    const result = await runEntry({
+      "bad.js": twoBuildErrors,
+      "entry.js": /* js */ `
+        ${shape}
+        const out = {};
+        out.firstImport = await import("./bad.js").then(() => "loaded", shape);
+        const firstText = await import("./bad.js", { with: { type: "text" } });
+        out.secondImport = await import("./bad.js").then(() => "loaded", shape);
+        const secondText = await import("./bad.js", { with: { type: "text" } });
+        out.textIsSameModule = firstText === secondText;
+        out.text = firstText.default;
+        console.log(JSON.stringify(out));
+      `,
+    });
+    expect(result).toEqual({
+      firstImport: aggregateOfTwo,
+      secondImport: aggregateOfTwo,
+      textIsSameModule: true,
+      text: twoBuildErrors,
+    });
+  });
+
+  test("a plugin module that failed to load is loaded again with its own error", async () => {
+    const result = await runEntry({
+      "entry.js": /* js */ `
+        let attempts = 0;
+        let thrown;
+        Bun.plugin({
+          name: "failing module",
+          setup(build) {
+            build.module("virtual:failing", async () => {
+              attempts++;
+              thrown = new Error("attempt " + attempts);
+              thrown.code = "E_ATTEMPT_" + attempts;
+              throw thrown;
+            });
+          },
+        });
+        const shape = e => ({ message: e.message, code: e.code ?? null, isThrownObject: e === thrown });
+        const out = {};
+        out.first = await import("virtual:failing").then(() => "loaded", shape);
+        out.second = await import("virtual:failing").then(() => "loaded", shape);
+        out.attempts = attempts;
+        console.log(JSON.stringify(out));
+      `,
+    });
+    expect(result).toEqual({
+      first: { message: "attempt 1", code: "E_ATTEMPT_1", isThrownObject: true },
+      second: { message: "attempt 2", code: "E_ATTEMPT_2", isThrownObject: true },
+      attempts: 2,
+    });
+  });
+
+  // Only a failed build is retried. A module that threw while evaluating is
+  // cached along with its error, as the spec requires.
+  test("a module that threw while evaluating is not evaluated again", async () => {
+    const result = await runEntry({
+      "throws.js": `globalThis.evaluations = (globalThis.evaluations ?? 0) + 1;\nthrow new Error("evaluation failed");`,
+      "entry.js": /* js */ `
+        const errors = [];
+        for (let i = 0; i < 2; i++) errors.push(await import("./throws.js").then(() => "loaded", e => e));
+        console.log(JSON.stringify({
+          messages: errors.map(e => e.message),
+          sameError: errors[0] === errors[1],
+          evaluations: globalThis.evaluations,
+        }));
+      `,
+    });
+    expect(result).toEqual({ messages: ["evaluation failed", "evaluation failed"], sameError: true, evaluations: 1 });
+  });
+});