ModuleLoader: record a non-Error fetch rejection as a fetch failure

[robobun]

What

A top-level dynamic import() whose embedder fetch rejects with a value that is not an ErrorInstance poisons its module registry entry. Every later importer of the same file that goes through hostLoadImportedModule (a static import, or a dynamic import inside another module's graph load) then hangs forever.

Bun hits this constantly: a single-message transpile failure rejects the fetch with a BuildMessage, which is a plain JSDestructibleObject. No import attributes, no concurrency, no unusual input required:

// bad.ts contains exactly `import {` (one parser error -> a single BuildMessage)
// other.ts contains `import "./bad.ts"`
await import("./bad.ts").catch(() => {});
await import("./other.ts"); // never settles

On Bun 1.4.0 the second import() never resolves or rejects and the process never exits (timeout 10 bun run.mjs exits 124).

Why

moduleLoadTopSettled's rejected branch (JSMicrotask.cpp) gates the entire fetch/evaluation-error classification on dynamicDowncast<ErrorInstance>(errorValue):

if (auto* error = dynamicDowncast<ErrorInstance>(errorValue)) {
    auto failure = JSModuleLoader::getErrorInfo(globalObject, error);
    if (failure.isEvaluationError(specifier, type))
        entry->setEvaluationError(globalObject, error);
    else
        entry->setFetchError(globalObject, error);
}
intermediatePromise->reject(vm, errorValue);

For a non-ErrorInstance value, nothing is recorded. moduleLoadTopRejected then runs, finds entry->fetchError() null, and calls entry->setEvaluationError(errorValue).

That leaves the entry at EvaluationFailed with no fetch, module, or load promise: the top-level rejection path never reaches the inner loadModule/hostLoadImportedModule that would have attached them. The next hostLoadImportedModule of the same key finds a non-New entry with no fetchError() and no loadPromise(), skips the re-fetch because the status is not New, and calls ensureModulePromise(), which creates a fresh pending fetchPromise. Nothing ever settles it.

Fix

Add the else branch: a non-ErrorInstance rejection value is still a fetch failure, so record it with setFetchError.

This is safe by construction. moduleLoadTopSettled reacts to the promise JSModuleLoader::loadModule passes it, which is always a fetch result: either a fresh fetch() or a cached entry->ensureFetchPromise(). The only thing that rejects an entry's fetch promise is setFetchError, and provideFetch only fulfills it, so every rejection that reaches this microtask is a fetch failure. The existing isEvaluationError check is about ErrorInstances that carry stage metadata from attachErrorInfo; a value that cannot carry that metadata cannot be an evaluation error here.

It also matches the sibling path. The nested-import reaction, moduleRegistryFetchSettled, already calls setFetchError unconditionally and only gates the optional attachErrorInfo metadata on the value being an ErrorInstance:

JSValue errorValue = arguments[1];
if (auto* error = dynamicDowncast<ErrorInstance>(errorValue))
    JSModuleLoader::attachErrorInfo(globalObject, error, ...);
entry->setFetchError(globalObject, errorValue);

With the entry at FetchFailed, moduleLoadTopRejected rejects with the cached fetchError() instead of calling setEvaluationError, and later importers take hostLoadImportedModule's fetchError() short circuit and reject instead of parking on a promise nothing settles.

Not a duplicate of #258

#258 fixes a different bug in the same function (the module map key collapsing every host-defined import attribute onto one entry). It threads the attribute into the ensureRegistered call here but keeps the ErrorInstance gate, and this hang reproduces with no import attributes at all, so the two are independent. The diffs apply cleanly together.

Verification

Built JSCOnly (debug, asserts, ASAN) from this branch and linked it into a Bun debug build.

• The repro above: both import()s reject with the BuildMessage, the process exits 0.
• Without the fix the second import() never settles (10s timeout).
• A Bun regression test is in the companion oven-sh/bun PR (linked once its preview artifact exists); it fails with a 5000ms test timeout on released Bun and passes on this build.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 64ef46f5-dae9-4ae2-bf7f-b3873a923a6c

📥 Commits

Reviewing files that changed from the base of the PR and between 0d9ee56 and 661592f.

📒 Files selected for processing (1)

• Source/JavaScriptCore/runtime/JSMicrotask.cpp

---

Walkthrough

This change extends the rejection-handling logic in moduleLoadTopSettled within JSMicrotask.cpp. A new else branch handles cases where the load rejection value is not an ErrorInstance, recording it as the module entry's fetch error instead of leaving it unhandled.

Changes

Module Fetch Error Categorization

Layer / File(s)	Summary
Non-ErrorInstance rejection handling Source/JavaScriptCore/runtime/JSMicrotask.cpp	Adds an else branch in moduleLoadTopSettled to set the module entry's fetch error via entry->setFetchError(...) when the rejection value is not an ErrorInstance, supplementing the existing ErrorInstance classification logic.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is substantive, but it does not follow the required template and omits the Bugzilla link, reviewed-by line, and changed-path details.	Rewrite the description to match the template: bug title, Bugzilla link, Reviewed by line, brief fix explanation, and bullet list of changed paths/functions.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: non-Error fetch rejections are recorded as fetch failures.
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.

---

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

[robobun]

The companion Bun PR is oven-sh/bun#33149: it pins WEBKIT_VERSION at autobuild-preview-pr-262-661592f9 and adds the regression test described above (test/js/bun/resolve/concurrent-dynamic-import.test.ts, a 5000ms timeout on released Bun, passes with this branch).

[claude]

LGTM — minimal else-branch that brings moduleLoadTopSettled's non-ErrorInstance rejection handling into parity with moduleRegistryFetchSettled.

Extended reasoning...

Overview

This PR adds a single else branch (5 lines including comment) to moduleLoadTopSettled in Source/JavaScriptCore/runtime/JSMicrotask.cpp. When the embedder fetch promise rejects with a value that is not an ErrorInstance, the code previously recorded nothing on the registry entry; the new branch calls entry->setFetchError(globalObject, errorValue) so the entry transitions to FetchFailed instead of being left in a state that later causes hostLoadImportedModule to park forever on a fresh pending promise.

Security risks

None. This is internal module-loader error-state bookkeeping. The change only affects which ModuleRegistryEntry status/error slot a rejection value is stored in; it does not introduce new data flow, parsing, allocation, or trust boundaries.

Level of scrutiny

Low. The change is mechanical and mirrors the existing sibling reaction moduleRegistryFetchSettled (lines ~908–913), which already calls setFetchError unconditionally on the JSValue and only gates the optional attachErrorInfo metadata on ErrorInstance. setFetchError's signature is (JSGlobalObject*, JSValue), so passing the raw errorValue is type-correct, and moduleLoadTopRejected already has a non-ErrorInstance branch when reading entry->fetchError() back (line ~1164–1165), so the downstream path handles the value shape.

Other factors

The PR description gives a precise causal chain (non-Error rejection → no fetchError recorded → moduleLoadTopRejected falls through to setEvaluationError → entry at EvaluationFailed with no fetch/load promise → next importer's ensureModulePromise() creates a never-settled promise) that I verified against the surrounding code in JSMicrotask.cpp and ModuleRegistryEntry.cpp. The author built JSCOnly debug+ASAN and confirmed the repro and fix, with a companion Bun regression test. The bug-hunting system found no issues, and there are no prior reviewer comments on the timeline.

[github-actions]

Preview Builds

Commit	Release	Date
661592f9	autobuild-preview-pr-262-661592f9	2026-06-30 20:36:16 UTC