Fix spurious "require() async module" TypeError when a CJS module requires an ESM sibling mid-transpile - #37185
Fix spurious "require() async module" TypeError when a CJS module requires an ESM sibling mid-transpile#37185robobun wants to merge 1 commit into
Conversation
…entry A CommonJS module imported by an ESM graph has its body evaluated while the graph is still loading, and that body can require() an ESM sibling the graph already started fetching on the transpiler thread. The sibling's registry entry is then status Fetching with a pending fetch promise. fetchCommonJSModule re-transpiled the file synchronously but provideFetch() only accepts a New entry, so the fresh source was silently dropped, the synchronous load had nothing to drain, and require() threw a spurious TypeError: require() async module "..." is unsupported. use "await import()" instead. for a module with no top-level await. Intermittent on multi-file mixed ESM/CJS graphs (28/30 runs on the repro below on a loaded machine). Settle the entry's fetch promise with the just-transpiled source instead. The async transpiler's own result lands later on an already-settled promise and is dropped by the existing PromiseFulfillWithoutHandlerJob pending-target guard.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The overlap with #33184 is real but partial. Both address the same symptom (the false Differences:
Only one of the two should land. If maintainers prefer #33184's placement, the WebKit companion and the ordered test here still stand on their own; otherwise this PR supersedes it for the race. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it touches the JSC module-loader race path (directly settling a pipeFrom()'d fetch promise via fulfillPromise and relying on the later async-transpiler result being dropped), a human look from someone familiar with the C++ loader would still be worthwhile.
What was reviewed:
- All four
provideFetchcall sites infetchCommonJSModule/fetchCommonJSModuleNonBuiltinroute through the new helper; the fallthrough still callsprovideFetchwith theJSSourceCode*overload (matches ZigGlobalObject.cpp:741). - The isolation-cache and non-builtin sites now wrap
SourceCodeinJSSourceCode::create— consistent with what the fetch promise expects. - Checked the transitive mid-fetch (diamond) graph concern — ruled out; the entry-status guard applies per-specifier.
- Test spawns 12 concurrent subprocesses with a padded ESM sibling, drains stdout/stderr/exited concurrently, and asserts exact evaluation order; 30s timeout is justified for debug/ASAN.
Extended reasoning...
Overview
This PR fixes a race in src/jsc/bindings/ModuleLoader.cpp where a CJS module require()ing an ESM sibling that is already mid-fetch on the async transpiler thread would spuriously throw require() async module ... is unsupported. The fix introduces provideFetchForSyncLoad(), which detects a Fetching-status registry entry and settles its fetch promise directly with the just-transpiled source (via fulfillPromise, bypassing the first-resolving-function guard set by pipeFrom()), so the synchronous module queue can drain it. Four call sites are rewired; two now wrap raw SourceCode in JSSourceCode::create. A new test reproduces the race with a padded ESM file across 12 concurrent runs.
Security risks
None. This is internal module-loader plumbing; no user-controlled input reaches new parsing or allocation paths, and no auth/crypto/permissions surface is touched.
Level of scrutiny
High. The module loader is on every startup path, and the fix depends on non-obvious JSC internals: that fulfillPromise can safely override a pipeFrom-armed promise, that the FetchSettled reaction lands on the caller's synchronous queue, and that the async transpiler's later resolution is harmlessly dropped by PromiseFulfillWithoutHandlerJob's pending-target check. The PR also notes a related JSC-side assertion (oven-sh/WebKit#396) that covers adjacent doors of the same graph. These are correctness claims about vendored JSC behavior that merit a maintainer's eye.
Other factors
The bug-hunting system raised and refuted two candidates (30s test timeout — reasonable for 12 debug/ASAN spawns; transitive diamond-graph coverage — the fix is per-specifier). The test follows harness conventions (tempDir, bunEnv, concurrent pipe drain, exact-output assertion, exit code last). No prior human review comments. Given the subtlety of the JSC promise/registry interaction and its position in a critical hot path, deferring to human review rather than auto-approving.
|
For whoever takes the human look, the two contracts the review flags are both explicit in the vendored JSC:
CI is green across all lanes on Buildkite build 90527. |
2 participants
Symptom
On a mixed ESM/CJS graph where a CommonJS module
require()s an ESM module that an ancestor also imports statically,bun entry.mjsintermittently fails (28/30 runs on a loaded machine, 6/30 elsewhere) with:even though
e.mjshas no top-level await.6-file repro
Expected output (what Node prints):
e,d,c,b,a,entry.Cause
A CommonJS module imported by an ESM graph has its body evaluated while the graph is still loading (the loader's makeModule step runs the CJS body to learn its exports). That body can
require()an ESM sibling the graph already started fetching on the transpiler thread, so the sibling's registry entry is status Fetching with a pending fetch promise.fetchCommonJSModulere-transpiles the file synchronously, butprovideFetch()only accepts a New entry, so the fresh source was silently dropped. The synchronous require(esm) load then had nothing to drain, its load promise stayed pending, and the pending promise was misreported as an async module.Fix
Settle the entry's fetch promise directly with the just-transpiled source. The FetchSettled reaction lands on the synchronous module queue the caller drains, so the require(esm) load completes without yielding. The async transpiler's own result arrives later on an already-settled promise and is dropped by the existing
PromiseFulfillWithoutHandlerJobpending-target guard (the same tolerancehostLoadImportedModule's synchronous replay path relies on).Verification
test/js/bun/resolve/require-esm-in-flight-sibling.test.tspadse.mjsso its transpile reliably loses the race, runs 12 concurrent instances, and asserts the exact evaluation order. Fails on an unfixed build, passes with this change.test/js/bun/resolverequire/esm suites andtest/js/node/modulepass.Related
Fixes #33180 (the same race, hit in the wild through jose via jwks-rsa/firebase-admin). #33184 is an earlier PR for that issue using the same mechanic in
functionEsmLoadSync; see the comparison in the comments, only one of the two should land.The same graph has a second, pre-existing face: with
require()of the ESM entry (orBUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER=1, orNODE_COMPILE_CACHE), debug builds abort withASSERTION FAILED: module->loadedModules().size() <= loadedModulesCountBefore + 1inJSModuleLoader::innerModuleLoadingbecause nested synchronous loads can complete other edges of the referrer module during one host-load call, which also loses ModuleGraphLoadingError reactions on release builds. That is fixed on the JSC side in oven-sh/WebKit#396; tests for those doors follow with the WebKit version bump once it merges. Verified locally against a WebKit build with both changes: all doors printe,d,c,b,a,entry.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file