innerModuleLoading: decide needsErrorReaction from the request key, not loadedModules size - #363
innerModuleLoading: decide needsErrorReaction from the request key, not loadedModules size#363robobun wants to merge 1 commit into
Conversation
…ot loadedModules size The size() == sizeBefore heuristic assumes that the only mutation hostLoadImportedModule can make to referrer.[[LoadedModules]] is adding the current request. Under load that invariant does not hold: a nested graph walk reached through FinishLoadingImportedModule can visit the same referrer (still status New) and synchronously populate a different request key first. size() grows by 1, so needsErrorReaction becomes false, but the current request is still pending; the ModuleGraphLoadingError reaction is never attached, and a later rejection of that load promise never reaches state.[[PromiseCapability]], leaving a dynamic import hanging or a module error swallowed. Check the request key directly instead of inferring from size(). The same lookup already runs two statements earlier for the 2.d.ii cache hit, so the cost is one extra HashMap::contains on the new-edge path. Drop the now-tautological assert.
|
Warning Review limit reached
Next review available in: 9 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
Preview Builds
|
2 participants
Problem
ASSERTION FAILED: needsErrorReaction != module->loadedModules().contains(ModuleMapKey { request.m_specifier.impl(), request.type() })inJSModuleLoader::innerModuleLoading(JSModuleLoader.cpp:893).Seen as a fleet escalation (5 hits) plus ~17% reproducible under 12-way parallel load with a package-resolve fuzzer scenario that dynamic-imports a graph over a freshly rewritten package tree while calling multiple resolver entry points. Debug/asserts builds abort; release builds do not crash.
Release consequence
innerModuleLoadingdecides whether to attach theModuleGraphLoadingErrorreaction to the promisehostLoadImportedModulereturned by checking whethermodule->loadedModules().size()grew across the call. When it grows, the code assumesFinishLoadingImportedModuleran synchronously with a normal completion for this request, so the edge is already accounted for and the error reaction can be skipped.That inference is wrong when the size growth came from a different
(specifier, type)key of the same referrer. A nested graph walk reached viaFinishLoadingImportedModule -> ContinueModuleLoading/ContinueDynamicImportcan visit this same referrer (stillstatus == New) and populate one of its other request keys synchronously.size()grows by exactly one, soneedsErrorReactionbecomesfalse, but the current request's load promise is still pending. If that promise later rejects,state.[[PromiseCapability]]never hears about it: the dynamic import hangs and the module error is swallowed.Fix
Replace the size heuristic with a direct key lookup:
needsErrorReaction = !module->loadedModules().contains(requestKey). This is exactly what the assertion already computed in debug builds, so it is provably equivalent when the invariant holds and correct when it does not. The sameHashMap::findalready runs two statements earlier for the 2.d.ii cache hit, so the cost is one extracontains()on the new-edge path.The
size() <= before + 1and theneedsErrorReaction != contains()asserts are removed: the first is no longer an invariant this code depends on, and the second is now a tautology.Verification
debug-local, ASAN on) against bun main at 4eb6f99c1.import-meta.test.js(32 tests) passes.A local-only deterministic reproduction proved intractable without the original fuzzer artifact (
repro/modulegraph-assert-1206/p-resolve-standalone.ts); every synchronous re-entrancy candidate path was guarded in a static read of the code, yet the assert fires under load. The fix is correct by construction for the described failure mode and carries no behavioral downside when the invariant holds.