Repository navigation
Conversation
…await stale `loadModuleSync` sets a stale module to `Pending` before it reads the record of the module. For a record with top-level await it then throws, and the module stayed `Pending` with no evaluation. Each later load took that module as it was, so `import()` resolved with `null`. `loadModuleSync` now sets the module back to `Stale` before it throws, and the next load evaluates it. #46017 made the same repair at the three other places where a load ends before the body of the module runs.
|
Updated 5:44 AM PT - Oct 26th, 2026
❌ @robobun, your commit 4585aae has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46970That installs a local version of the PR into your bun-46970 --bun |
|
Status: reproduced on main 449877f with the new test CI: the diff is green and ready for a maintainer review. Build 133041 passed 180 of 181 jobs, and PR: #46970 |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Stale record and retry behavior src/runtime/bake/hmr-module.ts, test/bake/dev/esm.test.ts |
loadModuleSync marks an existing module record stale before throwing for an async ESM module. The test verifies that a dynamic import fails, synchronous require() calls remain rejected, and a later dynamic import succeeds. |
Possibly related PRs
- oven-sh/bun#46615: Also changes stale-state handling before
AsyncImportErrorinloadModuleSync. - oven-sh/bun#46764: Changes HMR handling for async ESM evaluation, which relates to the top-level-await loading and retry behavior tested here.
Suggested reviewers: dylan-conway
Priority:
Merge Risk: 
4585a
-
Merge Risk: ⚪ Minimal · up to
4585aThe change addresses the failed-import retry without an identified merge-blocking issue.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the main change: resetting stale module state after a refused require() involving top-level await. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description explains the problem, the fix, and verification results. It does not use the exact template headings, but it provides the required information and additional relevant context.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the sync/async twin for the same class of bug: loadModuleAsync has no isAsync refusal path (it destructures load and ignores the flag), so there is no sibling site needing the same Stale reset. I also traced the state mod can be in at line 359 when non-null: only Pending (set from Stale at line 304; Error throws at 302 and every other state returns at 310), so the reset cannot clobber a Loaded or Error entry.
Extended reasoning...
The change is two lines in the Bake dev-server HMR module loader (src/runtime/bake/hmr-module.ts) resetting a Pending module to Stale before loadModuleSync throws AsyncImportError, plus one devTest in test/bake/dev/esm.test.ts covering the stale-then-require-then-import sequence. It touches no security-sensitive surface. The async loader has no equivalent refusal branch, and the only reachable prior state at the new line is Pending, so the fix is contained; the inline findings cover the adjacent pre-existing gap and test coverage of the guard.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/bake/hmr-module.ts— Users whose code require()s a top-level-await module while an import() of it is still in flight get an empty exports object instead of the top-level-await error the PR title promises. loadModuleSync returns a Pending module at src/runtime/bake/hmr-module.ts:310 without checking the record's isAsync flag, so require at :148 runs toCommonJS(null) and caches the empty result in mod.cjs. The new reset at :359 only covers the Stale-to-Pending path; the mid-evaluation Pending path is the same class of refused require() and is left returning silently wrong data. Fix: in loadModuleSync, when an existing module is Pending and its unloaded record has isAsync set, throw AsyncImportError instead of returning the module; add a test with a require() issued during the await.Why this was flagged
The trigger is a TLA module A whose loadModuleAsync evaluation is pending (state set to Pending at src/runtime/bake/hmr-module.ts:416 with Promise.all at :503 or the load promise at :526 not yet settled) while another module B calls require(A) through HMRModule.require at :145. loadModuleSync sees mod.state === State.Pending, takes the else branch at :306-310 and returns A. require at :148 computes toCommonJS(mod.exports) with exports still null, producing {__esModule: true} and caching it in mod.cjs until :530 clears it. B therefore reads undefined for every export and never sees the 'uses top-level await' error that the sync path throws at :360 for the same module one tick earlier or later. The base branch behaves the same, but this PR is advertised as fixing require() of a TLA module; the Pending-return branch is the sibling site that still hands back an unevaluated TLA module. Remedy: consult the isAsync flag of unloadedModuleRegistry[id] before returning a Pending module from loadModuleSync.
Verification: A top-level-await ESM module A is mid-evaluation (state Pending) when code calls require(A). loadModuleSync at src/runtime/bake/hmr-module.ts:300-311 returns a Pending mod at :310 before the isAsync check at :357. require at :148 then does toCommonJS(null), so the caller silently gets {__esModule: true}. Lines :301-311 are byte-identical on the base commit, so the base fails the same way.
…and a require() through an import The test now calls require() before any load of the module, and it calls require() of a module that imports the module with top-level await. Without the state reset the last two steps fail. Without the `if (mod)` guard the first two steps fail.
|
On the finding outside the diff:
|
There was a problem hiding this comment.
LGTM — the follow-up commit covers the first-ever require() and the transitive require() through via.ts that the earlier nit asked for, and the two-line fix is correct.
What was reviewed:
- Traced
loadModuleSyncfor a Stale TLA module: state goes Stale -> Pending at line 304, and the new reset at line 359 runs beforeAsyncImportErroris thrown, so the laterimport()re-evaluates instead of returning the Pending entry;importersis correctly not updated on the refused path. - Checked the sync/async twin:
loadModuleAsyncnever refuses TLA modules, so there is no sibling site needing the same reset. - Traced the test's six steps through
require()/loadModuleAsync:via.tsis left Stale by the existing catch at line 379 on both refused paths, so each assertion depends on the fix or theif (mod)guard and matches the exact message built at line 151.
Extended reasoning...
The change adds one guarded state reset in src/runtime/bake/hmr-module.ts (the dev-server HMR client/server module loader) and one devTest in test/bake/dev/esm.test.ts; it touches no auth, crypto, or input-parsing surface. The fix is two lines on a path that evaluates nothing, mirrors the existing Stale-reset pattern used at lines 337 and 379, and the test now exercises both the registry-entry and no-registry-entry branches plus the transitive importer case. The only prior reviewer comment (mine) was addressed by commit 5e6588c, no CODEOWNERS entry covers these files, and the hunt ran dry with no findings.
Problem
require()of a module with top-level await throwsCannot require "tla.ts" because "tla.ts" uses top-level await. If the module was stale, it staysPendingafter that throw and nothing evaluates it. A laterimport()resolves withnull.loadModuleSync(src/runtime/bake/hmr-module.ts:303,:357). It sets a stale module toPending, then throws for a record with top-level await and leaves the state.Fix
loadModuleSyncsets the module toStalebefore it throws. The next load evaluates the module.test/bake/dev/esm.test.ts(1 new test, fails on main: the last two of its six steps readnull). Alsohot.test.tsandbundle.test.ts.Background
hmr-module.tsis the module loader of the dev server, in the browser and in the bake server. It keeps oneHMRModulefor each file, with astate.Stalemeans that the next load evaluates the module again: after a hot update replaced it, or after an import of it threw before its body ran.Pendingmodule returns it as it is. That is right for an import cycle only.Downsides
Notes
require()of the module, for example in atryblock with animport()fallback. Thatimport()resolves withnull.replaceModuleswaits for a dispose callback. The reload loop marks the module stale again at its turn, so that case repairs itself.robobun/72a11806/hmr-wait-in-flight-tla, no PR). Its review asked for this line as a PR of its own. The other parts need a decision on the contract first. The question is in a comment on dev server: re-evaluate every module between a saved file and its accepting module #46764.require()before any load of the module, directly and through a module with no await that imports it. It repeats both calls after a failedimport(). Without the reset (main), the last two steps givenull is not an object (evaluating 'import_tla.tla'). Without theif (mod)guard, the first two steps giveundefined is not an object (evaluating 'mod.state = 1').require()of a module whose evaluation is in flight returns{}, as on main. A refusal there is point 1 of the question on dev server: re-evaluate every module between a saved file and its accepting module #46764.bake-codegen.ts --debug=false: client 52,885 to 52,901 B (as embedded, zstd 19: 17,695 to 17,701 B), server 10,908 to 10,924 B.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/esm.test.ts