JSModuleLoader: a top-level load stores its failure into the entry it loaded - #474
Conversation
![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 didn't find any bugs. Because it changes error-storage semantics across several async boundaries in the module loader — a state machine whose invariants are subtle and where #472 just fixed the crash flavor of the same window — a human look from someone familiar with the loader would still be worthwhile.
What was reviewed:
topLevelLoadEntry()falls back toensureRegistered()when no entry is recorded, so withUSE(BUN_JSC_ADDITIONS)off the three call sites behave exactly as before.setEntry()goes throughWriteBarrier::set(vm, this, entry), andm_entryis already visited inModuleLoadingContext::visitChildrenImpl, so the detached entry stays live until the context is collected.- The extra
ensureRegistered()inmoduleLoadTopSettledafterprovideFetch()returns the same entryprovideFetch()just registered/found (both key on(specifier, type)), and the second-overload lookup is taken beforehostLoadImportedModule()runs host code. specifier/typelocals inmoduleLoadTopSettled(rejected branch) andmoduleLoadStoreErrorare still read fromcontext->moduleRequest()for theisEvaluationError()check, so replacing the entry lookup doesn't change error classification.
Extended reasoning...
Overview
This PR fixes a race in JSModuleLoader where a top-level load whose registry entry was removed via removeEntry() (Bun's delete require.cache[], mock.module()) while in flight would, on failure, store its error under the key again — either into a fresh entry or into a replacement load's entry — permanently poisoning subsequent imports. The fix records the specific ModuleRegistryEntry each top-level ModuleLoadingContext attaches to (setEntry()) and has the three error-storage sites (moduleLoadTopSettled rejected branch, moduleLoadTopRejected, moduleLoadStoreError) store into that entry via a new topLevelLoadEntry() helper instead of looking the key up again. Five files touched: JSMicrotask.cpp, JSModuleLoader.cpp/.h, ModuleLoadingContext.cpp/.h.
Security risks
None identified. This is internal module-loader bookkeeping; no user-controlled input is newly interpreted, no auth/crypto/permissions surface. The GC interaction (holding a detached entry alive via WriteBarrier) is handled correctly — m_entry was already a visited WriteBarrier field, and setEntry() uses the proper barrier.
Level of scrutiny
High. Module loading is a critical runtime code path exercised by every import() and require() in Bun. The change is small in line count but its correctness depends on invariants about which entry a load is "attached to" across multiple microtask hops, and on the ordering between provideFetch(), hostLoadImportedModule()'s host hooks, and the error-storage microtasks. The PR description argues these invariants very carefully and the author verified against five targeted tests plus controls, but this is exactly the kind of state-machine change where a second pair of eyes from someone who knows the loader is valuable.
Other factors
- Stacked on #472 (which fixed the crash flavor of the same window); the diff here is only the delta.
- All behavior changes are gated on
USE(BUN_JSC_ADDITIONS); thetopLevelLoadEntry()helper compiles for both configurations but with the flag off is a pure refactor of the previousensureRegistered()call. - The PR description is unusually thorough — five reproduction cases, local verification methodology, explicit cost accounting, and a note about a related-but-unfixed window. This raises confidence but doesn't substitute for a reviewer who can independently confirm the loader invariants.
- No prior review comments on this PR.
b5b5142 to
a668d5d
Compare
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it combines an upstream cherry-pick with fork-specific changes to the module loader's error-storage semantics — a race-sensitive path where removeEntry() interacts with in-flight loads — a human look would still be worthwhile.
Checked: removeFailedFetchEntry takes cellLock() before mutating m_moduleMap, matching the concurrent-GC pattern in ensureRegistered/removeEntry. setEntry() uses WriteBarrier::set and m_entry was already visited by visitChildrenImpl, so the newly-held detached entry stays live. The dropped ThrowScope& params on moduleLoadTopRejected/moduleLoadStoreError are no longer read in either build configuration. The ensureRegistered call added in moduleLoadTopSettled's fulfilled branch runs right after provideFetch(), so it returns the same entry rather than allocating a second one.
Extended reasoning...
Overview
This PR modifies five files in JavaScriptCore's module loader: JSMicrotask.cpp (three microtask handlers: moduleLoadTopSettled, moduleLoadTopRejected, moduleLoadStoreError), JSModuleLoader.cpp/h (adds removeFailedFetchEntry, promotes getRegisteredMayBeNull to public, records the registry entry into the second loadModule overload's context), and ModuleLoadingContext.cpp/h (adds setEntry() under USE(BUN_JSC_ADDITIONS)). Commit 1 is a cherry-pick of upstream 319474@main that stops caching fetch/network errors in the module map. Commit 2 is the fork residual: top-level loads now record which ModuleRegistryEntry they attached to and store their failure into that entry rather than into whatever the key currently maps to, which matters when Bun's removeEntry() (delete require.cache[], mock.module()) drops an entry mid-load.
Security risks
None identified. There is no user-controlled input parsing, auth, or crypto here. The change is internal state management for module registry entries. The GC-visible mutations (m_moduleMap.remove, m_entry.set) follow the existing locking/write-barrier patterns.
Level of scrutiny
High. The module loader is a core JSC subsystem, and this change specifically targets a race window between removeEntry() and in-flight top-level loads. The correctness argument depends on the ordering of provideFetch(), ensureRegistered(), hostLoadImportedModule(), and the two error-storing microtasks across both the USE(BUN_JSC_ADDITIONS) and non-Bun builds. Commit 1 also changes observable behavior for all Bun users (failed fetches are re-fetched on the next import() rather than replaying the cached error), and the removal of the fork's #262 branch relies on the upstream rewrite subsuming it. These are design-bearing changes to a critical path that a maintainer familiar with #262/#472 and the Bun-side test PR should confirm.
Other factors
The PR description is very thorough — five reproduction cases, per-commit fix attribution, local debug-asan verification against #472's engine, and a compile check with USE_BUN_JSC_ADDITIONS=0. The bug hunting system found nothing. I verified the concurrent-GC lock discipline in removeFailedFetchEntry, that m_entry is already covered by visitChildrenImpl, and that dropping the ThrowScope& parameters is safe in both build configurations. Nonetheless, the combination of an upstream cherry-pick that changes caching policy, a fork-specific overlay on top of it, and the inherent subtlety of the removeEntry-while-loading race puts this outside what I'd approve without a human maintainer's look.
Preview Builds
|
a092ae2 to
86b6c4d
Compare
a668d5d to
4875ff6
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks error storage in the module loader's top-level load path (a cherry-pick plus fork-specific race handling around removeEntry()), a human look would still be worthwhile.
What was reviewed:
removeFailedFetchEntrytakescellLock()before mutatingm_moduleMap, matching the patternvisitChildrenImplandremoveEntryrequire.setEntryuses a proper write barrier andm_entryis already visited invisitChildrenImpl, so the detached entry stays live until the context is collected.- The dropped
ThrowScope¶meters onmoduleLoadTopRejected/moduleLoadStoreErrorare safe: the new bodies no longer call anything that can throw (ensureRegistered/error()are gone). - The
ensureRegisteredcall afterprovideFetchinmoduleLoadTopSettledlooks up the same entryprovideFetchjust registered, sosetEntryrecords the right one.
Extended reasoning...
Overview
This PR touches five files in JavaScriptCore's module loader: JSMicrotask.cpp, JSModuleLoader.{h,cpp}, and ModuleLoadingContext.{h,cpp}. It has two parts: (1) a cherry-pick of upstream WebKit 319474@main that stops caching fetch/network errors in the module map (adds removeFailedFetchEntry, promotes getRegisteredMayBeNull to public, and rewrites the error branches of moduleLoadTopSettled/moduleLoadTopRejected/moduleLoadStoreError to look up rather than register entries), and (2) a Bun-specific residual under USE(BUN_JSC_ADDITIONS) that has each top-level ModuleLoadingContext record the specific ModuleRegistryEntry its load is attached to via a new setEntry(), so that when the load fails the error is stored into that entry rather than into whatever entry the key resolves to at failure time.
Security risks
None identified. This is internal module-loading bookkeeping — no untrusted-input parsing, auth, or boundary crossing. The GC-facing pieces (write barrier in setEntry, cellLock() around the m_moduleMap.remove in removeFailedFetchEntry) follow the existing patterns in the file, and m_entry was already visited by ModuleLoadingContext::visitChildrenImpl.
Level of scrutiny
High. The module loader is a core, hot runtime path and this change alters error-caching semantics under a race that only exists because Bun's fork adds removeEntry(). The reasoning is subtle (which entry a key resolves to at each microtask boundary, when a context has vs. lacks an entry, how a rejected fetch differs from a rejected evaluation) and the correctness argument spans several functions across two files. The cherry-pick portion is upstream-verified, but the fork residual layers a different lookup strategy on top of it and removes the fork's earlier #262 branch on the grounds that upstream's rewrite subsumes it. This is exactly the kind of change where a maintainer familiar with the loader's history (#258, #262, #472) should confirm the interactions the description enumerates.
Other factors
The PR description is unusually thorough and documents eight specific test scenarios in oven-sh/bun#39711 that were verified against a locally patched engine, including which commit fixes each and two control cases. The preview build passed. No prior human reviews are on the PR. The change is stacked on #472 and interacts with #258's pending re-keying, which the author notes. Given the depth of the semantic change and the number of cross-PR interactions, deferring to a human reviewer is the right call even with no bugs found.
…472 preview at b7f217b4)
…472 preview at b7f217b4)
…472 preview at b7f217b4)
86b6c4d to
50b13c0
Compare
4875ff6 to
0bddaf4
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes core module-loader error-caching semantics across microtask boundaries and interacts with the fork-only removeEntry() race window, a human look would still be worthwhile.
What was reviewed:
- Cherry-pick fidelity:
moduleLoadTopSettled's rejected branch,moduleLoadTopRejected,moduleLoadStoreError,removeFailedFetchEntry, and theloadModuleFetchFailed drop match upstream 319474@main's shape. - GC safety of
setEntry():m_entryis aWriteBarrieralready visited byvisitChildrenImpl, andremoveFailedFetchEntrytakescellLock()before mutatingm_moduleMap. - The
ensureRegisteredcall added inmoduleLoadTopSettledruns afterprovideFetch(), so it returns the same entry rather than creating a stray one. - Dropped
ThrowScope¶ms onmoduleLoadTopRejected/moduleLoadStoreError— the remaining bodies no longer call anything that needs exception checking.
Extended reasoning...
Overview
This PR touches five files in JSC's runtime module loader: JSMicrotask.cpp, JSModuleLoader.{h,cpp}, and ModuleLoadingContext.{h,cpp}. It is two commits: (1) a cherry-pick of upstream WebKit 319474@main ("Don't cache HTTP/network errors in the module map"), which rewrites how moduleLoadTopSettled/moduleLoadTopRejected/moduleLoadStoreError register errors and adds removeFailedFetchEntry(); (2) a Bun-specific residual under USE(BUN_JSC_ADDITIONS) that records the ModuleRegistryEntry a top-level load attaches to on its ModuleLoadingContext (setEntry()), so that when the load fails its error is stored into that entry rather than whatever entry the key currently maps to. This closes a race where removeEntry() (via delete require.cache[], mock.module(), plugins) drops an entry mid-load and a replacement load's entry then wrongly inherits the removed load's error.
Security risks
None identified. The change is confined to error-caching bookkeeping in the module registry. There is no new parsing, no user-controlled input handling beyond what already existed, and no auth/crypto/permissions surface. The GC-facing additions use the existing WriteBarrier<ModuleRegistryEntry> m_entry field (already visited) and take cellLock() where the map is mutated, matching the surrounding pattern.
Level of scrutiny
High. This is core engine code on the hot path of every ES module import in Bun. The correctness argument depends on subtle ordering across microtasks (fetch → moduleLoadTopSettled → second loadModule overload → moduleLoadStoreError/moduleLoadTopRejected), on the fork-only removeEntry() semantics, and on the interaction with #472 (this PR's base). The upstream cherry-pick also changes user-visible behavior for Bun (failed fetches are now retried on the next import() instead of replaying a cached error), which is a semantic shift worth a maintainer's sign-off even though it aligns with the HTML spec.
Other factors
The PR description is unusually thorough — it enumerates five reproduction shapes in oven-sh/bun#39711, states which commit fixes each, and reports local verification against Bun's resolve/plugin/mock/module test suites plus the concurrent-GC registry tests. The bug-hunting pass found nothing. The added ensureRegistered in moduleLoadTopSettled immediately follows provideFetch() (which itself calls ensureRegistered), so it cannot create a divergent entry; the second loadModule overload reads the entry before hostLoadImportedModule() runs host code, per its comment. Despite all this, the change is neither simple nor mechanical, it is stacked on another open PR, and it alters error-caching policy that every module load depends on — so I'm deferring rather than approving.
…472 preview at aea1f010)
50b13c0 to
22dfd70
Compare
0bddaf4 to
ff62d80
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it changes error-storage semantics in the module loader's microtask chain and interacts with the fork's removeEntry() race window, a human reviewer familiar with the loader should also take a look.
What was reviewed:
- The cherry-picked
getRegisteredMayBeNull/removeFailedFetchEntrypaths against upstream 319474@main's shape. setEntry()GC safety —m_entryis already visited inModuleLoadingContext::visitChildrenImpl, and the setter uses a write barrier.removeFailedFetchEntry()locking — takescellLock()before mutatingm_moduleMap, and guards against the entry no longer being the map's value.- Unused-variable fallout in the
!USE(BUN_JSC_ADDITIONS)branch ofmoduleLoadStoreError—specifier/typeare still consumed bygetRegisteredMayBeNullthere.
Extended reasoning...
Overview
The PR touches five files in JSC's module loader: JSMicrotask.cpp (three microtask handlers rewritten), JSModuleLoader.{h,cpp} (new removeFailedFetchEntry(), getRegisteredMayBeNull() made public, loadModule overloads adjusted), and ModuleLoadingContext.{h,cpp} (new setEntry() mutator). It combines a cherry-pick of upstream 319474@main (stop caching fetch/network errors in the module map) with a fork-specific residual under USE(BUN_JSC_ADDITIONS) that makes the two top-level load contexts remember the specific ModuleRegistryEntry they attached to, so that a later failure is stored into that entry rather than whatever the key resolves to after a concurrent removeEntry().
Security risks
No direct security surface — this is internal error-propagation bookkeeping. The GC-relevant additions (WriteBarrier<ModuleRegistryEntry> m_entry mutation via setEntry()) reuse an existing visited field with a proper write barrier, and removeFailedFetchEntry() takes the cell lock before mutating m_moduleMap, matching the pattern used by ensureRegistered() and removeEntry(). I did not spot UAF or missing-barrier concerns.
Level of scrutiny
High. The module loader's microtask chain is race-sensitive and load-bearing for every ES module import in Bun. The change alters when and where errors are cached (an observable behavioral change: failed fetches now retry on the next import()), removes the fork's #262 branch, and threads entry identity through two contexts that previously left it null. The reasoning in the PR description is careful and the reproduction matrix is detailed, but the correctness argument hinges on ordering between provideFetch(), ensureRegistered(), removeEntry(), and the microtask that later reads context->entry() — exactly the kind of invariant a maintainer who owns this code should sign off on.
Other factors
- Stacked on #472; the base PR should land first or together.
- The upstream half will also arrive via a future upgrade merge, so a reviewer should confirm the cherry-pick matches upstream to avoid a future conflict.
- No prior human or bot reviews on the thread; this is the first pass.
- Tests exist in the companion Bun PR (oven-sh/bun#39711) rather than in this repo, so verification depends on that external suite.
…472 preview at c148a12d)
22dfd70 to
daf9f5f
Compare
ff62d80 to
a0e12a5
Compare
robobun
changed the title
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes error-storage semantics in the module loader's top-level load path — subtle ordering between in-flight loads, removeEntry(), and detached-entry lifetime — a human look would still be worthwhile.
What was reviewed:
setEntry()usesWriteBarrier::setcorrectly andm_entryis already visited invisitChildrenImpl, so the detached entry stays live until the context dies.moduleLoadTopSettled'sensureRegisteredruns immediately afterprovideFetch()with no host code in between, so it records the same entryprovideFetchregistered.specifier/typeremain used inmoduleLoadStoreError's Bun path (forisEvaluationError); inmoduleLoadTopRejectedthey are correctly scoped to the#elsebranch.- The second
loadModuleoverload reads the entry beforehostLoadImportedModule()(which can run hostresolve()), matching the comment's intent.
Extended reasoning...
Overview
This PR changes how top-level module loads store their failure. Previously, moduleLoadTopRejected and moduleLoadStoreError looked up the registry entry by key at the moment of failure via getRegisteredMayBeNull(key). Under Bun's removeEntry() (delete require.cache[], mock.module()), that key may by then point to nothing or to a replacement load's entry, so a removed load's error could poison an unrelated replacement. The fix records the entry the load actually attaches to on its ModuleLoadingContext (via a new setEntry()) at the point of registration, and the error sites store into that recorded entry instead. All changes are gated behind USE(BUN_JSC_ADDITIONS).
Files touched: JSMicrotask.cpp (the two error-storing microtask handlers and the moduleLoadTopSettled registration point), JSModuleLoader.cpp (second loadModule overload records the entry before hostLoadImportedModule), ModuleLoadingContext.{h,cpp} (new setEntry method), and a comment update in JSModuleLoader.h's removeEntry().
Security risks
None identified. This is internal engine bookkeeping for module-loader error caching; no auth, crypto, or external input handling. The GC-lifetime change (a context keeps a detached entry alive until the load settles) matches what per-dependency contexts already do, and m_entry was already a visited WriteBarrier field.
Level of scrutiny
High. This is core JSC module-loader infrastructure with subtle invariants about microtask ordering, registry mutation during in-flight loads, and GC visitation. It is stacked on #472 and interacts with upstream 319474@main. The author has done thorough verification (eight targeted tests in oven-sh/bun#39711, debug-asan builds, concurrent-GC registry tests, and a compile check with the Bun flags off), and the reasoning in the description is precise, but the change is neither mechanical nor obvious — it depends on understanding exactly when each microtask fires relative to removeEntry() and replacement loads.
Other factors
- The change is small and well-commented, and
m_entryalready existed on the context (it was only unused for the top-level create() overload), so no new GC plumbing was required. - I checked that
ensureRegisteredinmoduleLoadTopSettledcannot see a different entry thanprovideFetch()registered (no yield or host callout between them), thatsetEntryfires the write barrier, and that thespecifier/typelocals remain correctly scoped/used under both preprocessor branches. - No prior human review or comments on the PR; only the preview-build bot has posted.
- Given the criticality of the module loader and the subtlety of the concurrent-removal window this fixes, I'm deferring rather than approving.
…472 preview at cb61607f)
…472 preview at 1cb96a7b)
319c644 to
084e853
Compare
5e159d0 to
3a8db01
Compare
…472 preview at 76882271)
084e853 to
18461ab
Compare
3a8db01 to
42ea219
Compare
…472 preview at 2da33d53)
18461ab to
3580958
Compare
42ea219 to
6a94e34
Compare
…472 preview at 030d206f)
030d206 to
d3deb97
Compare
6a94e34 to
2cd2d81
Compare
…472 preview at 0bb01ed5)
f7cb3cb to
6a17c6a
Compare
b38262c to
0e81736
Compare
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
| // The entry the key holds now, registered by provideFetch() just above or | ||
| // by provideModule() before the load, is the one the loadModule below | ||
| // loads; moduleLoadTopRejected stores into it. | ||
| context->setEntry(vm, globalObject->moduleLoader()->ensureRegistered(globalObject, specifier, type)); |
There was a problem hiding this comment.
🔴 ensureRegistered() re-registers a removed provideModule() key with a fresh Status::New entry, tripping the hostLoadImportedModule assert
Extended reasoning...
In the Bun provideModule() path, the outer context's entry is now recorded via ensureRegistered(specifier, type) after the !inherits<AbstractModuleRecord>() guard closes but before the inner loadModule call. Unlike the sibling read this PR adds at JSModuleLoader.cpp:852 (getRegisteredMayBeNull + null-check at :860-861), ensureRegistered() creates and inserts a brand-new Status::New entry when the key is absent. That happens whenever an embedder-preregistered entry (provideModule → status Fetched, so ensureFetchPromise fulfilled the fetch promise with the record) is removeEntry()'d in the window between the first loadModule overload scheduling ModuleLoadTopSettled and this microtask running. The freshly-inserted New entry is then found by hostLoadImportedModule (either via the symbol fast path at JSModuleLoader.cpp:674-675 or via the resolved-key lookup at :721) and hits ASSERT(mapEntry->status() != ModuleRegistryEntry::Status::New) at JSModuleLoader.cpp:727. On the base branch nothing was registered here, so hostLoadImportedModule fell through to the…
Verification: normal — the new ensureRegistered() call can insert a Status::New entry that the immediately-following hostLoadImportedModule then finds and asserts against; the base branch inserted nothing at this point. Trigger (provideModule path with removal during the fetch window): 1. Bun pre-registers key K via provideModule() (entry status Fetched, has a record) — the PR body itself cites this…
cfccea9 to
eaacac2
Compare
0e81736 to
ed2d1c8
Compare
… loaded moduleLoadTopRejected and moduleLoadStoreError look the entry up by key when a top-level load (import(), require(esm), the entry point) fails. The host can remove that entry while the load is in flight (removeEntry(): delete require.cache[], mock.module(), plugin modules), and a replacement load of the key can register a new entry before the removed load fails. The key lookup then stores the removed load's error into the replacement's entry, and every later import() of the key rejects with it although the replacement loaded fine. A load whose fetch was rejected could likewise store its rejection into an entry that a concurrent load of the key had registered in the meantime. The two top-level ModuleLoadingContexts now record the entry their load attaches to: moduleLoadTopSettled records the entry provideFetch() registered, and the second loadModule overload records the entry it links and evaluates. The two error sites store into that entry, the way the per-entry steps (moduleLoadStep, moduleRegistryFetchSettled) already store into the entry they hold. While the entry is registered this is the entry the key lookup found, so nothing changes. Once it is removed, the error stays with the detached entry. A load that failed at its fetch has no entry and stores nothing; moduleLoadTopSettled registers nothing for such a load since 319474@main. Under USE(BUN_JSC_ADDITIONS): only removeEntry() and clearAll() can make the key's entry differ from the load's.
ed2d1c8 to
56b8426
Compare
…472 preview at ceb9f90f)
…472 preview at ceb9f90f)
1 participant
Stacked on #472 (its branch is the base, so the diff here is only this commit). This PR's preview build contains both.
Upstream 319474@main ("Don't cache HTTP/network errors in the module map") arrived on main with the 8c4fd56 upgrade (#503). This PR used to carry it as a cherry-pick; it is now the fork residual on top of it.
Problem
removeEntry()(delete require.cache[],mock.module(), plugin modules) can remove an entry while a top-level load of it is in flight. When that load then fails,moduleLoadTopRejectedandmoduleLoadStoreError(JSMicrotask.cpp) store its error into whatever entry the key holds at that moment,getRegisteredMayBeNull(key). After a removal that is nothing, or the entry of a replacement load that registered the key in the meantime. In the second case the replacement, a module that loaded fine, rejects every laterimport()with the removed load's error. Before 319474@main it was worse (ensureRegistered(key), so the key was registered again with the error); JSModuleLoader: a removed registry entry's in-flight load must not re-cache its record #472 fixes the crash flavor of this window.JSModuleLoader::loadModule()answers every later load of a key whose entry holds an error fromentry->error(), so one wrongly stored error is permanent.Fix
ModuleLoadingContexts record the entry their load attaches to (setEntry()):moduleLoadTopSettledrecords the entryprovideFetch()registered, the secondloadModuleoverload records the entry it links and evaluates. The two error sites store into that entry.moduleLoadStep,moduleRegistryFetchSettled) already store into the entry they hold. While the entry stays registered it is the one the key lookup finds, so nothing changes. Once removed, the error stays with the detached entry and the key belongs to whatever replaced it. A load that failed at its fetch has no entry and stores nothing, which is 319474@main's policy.USE(BUN_JSC_ADDITIONS): onlyremoveEntry()andclearAll()can make the key's entry differ from the load's.a replacement load finished first, which needs this commit. Notes below.Background
ModuleRegistryEntry: promises, record, one error.removeEntry()is a fork addition, so upstream's key lookup and this entry are the same thing there.loadModule(specifier): it fetches, thenmoduleLoadTopSettledregisters the key (provideFetch()) and calls the second overload, which loads, links and evaluates the entry. Each overload carries aModuleLoadingContextthrough its microtasks. Those two contexts leftm_entryempty until now; the per-dependency ones (hostLoadImportedModule) always held theirs.Notes
Reproductions (Bun 1.4.0 and a Bun debug build against #472's preview; tests in oven-sh/bun#39711):
import()of a module whose dependency an asynconLoadholds back,delete require.cache[module], release it, the module throws while it evaluates. Nextimport(): rejected with the old error, the plugin is not asked again (moduleLoadTopRejected). Fixed by 319474@main.onLoadthrows (moduleLoadStoreErrorandmoduleLoadTopRejected). Fixed by 319474@main.import()is awaited before the gate opens. A thirdimport()is rejected with the removed load's error although the replacement loaded; with 319474@main alone the replacement's entry is the onegetRegisteredMayBeNull()finds. Fixed by this commit only.onLoadis pending registers it asFetching; animport()issued now shares that fetch.delete require.cache[], then theonLoadthrows. Nextimport(): rejected with the old error (moduleLoadTopSettled's rejected branch). Fixed by 319474@main, which stores nothing for a rejected fetch; this commit keeps it that way (the context has no entry).require()of a top-level-await module (Bun pre-registers the entry and the module keeps evaluating),delete require.cache[],import()it again, then the first run rejects. Before 319474@main the replacementimport()never settled: the first run'smoduleLoadStoreErrorregistered the key before the replacement'smoduleLoadTopSettled, soprovideFetch()found anEvaluationFailedentry and did nothing. 319474@main fixes the observed order; this commit also covers the order where the replacement registers first.import()andrequire(), also after a module that imports it failed), and a pluginonLoadthat rejected is called again by the nextimport(). These pin 319474@main's policy as Bun sees it.Local verification was done by recompiling the three affected unified-source bundles into the
autobuild-preview-pr-472-4ee15452linux debug-asan tarball, once with the upstream change alone (as a cherry-pick, identical to what the upgrade brought) and once with this commit on top, and linking a Bun debug build against each. The pass/fail split above comes from that; the pass-after was then repeated on the published preview builds of this PR. The files also compile withUSE_BUN_JSC_ADDITIONSandUSE_BUN_EVENT_LOOPset to 0. Against the full change, Bun'stest/js/bun/resolve/,test/js/bun/plugin/,test/js/bun/test/mock/,test/js/node/module/(including the registry concurrent-GC tests),test/cli/run/,test/cli/test/andtest/cli/test/isolation.test.tspass except for tests that fail the same way on the unmodified engine on a debug-asan build.Cost: one registry lookup per top-level load in
moduleLoadTopSettledand one in the secondloadModuleoverload, plus a write barrier each.m_entryalready existed on the context and is already visited; a context keeps a removed entry alive until its load settles, as the per-dependency contexts already did.Not changed:
moduleLoadTopSettledstill registers a fulfilled fetch by key (provideFetch()), so a removal during the fetch itself re-registers the pre-removal source; that is upstream's registration point and independent of the error sites. The FIXME in Bun'sBun__onFulfillAsyncModule(register before fetching) is where that would become consistent. #258 touches the same functions (it re-keys the lookups on the attribute string); the two lookups added here would take the request's parameters under it.