module loader: coalesce concurrent embedder fetches of the same module - #33417
module loader: coalesce concurrent embedder fetches of the same module#33417robobun wants to merge 5 commits into
Conversation
JSC's dynamic-import path calls moduleLoaderFetch without first registering a module registry entry, so its in-flight dedupe has nothing to match until the first fetch settles. Every importer that arrived before then started its own fetch: three concurrent import()s of one specifier ran a plugin's onLoad three times (and re-transpiled the file three times), with the first result winning the registry. Keep the in-flight embedder fetch promise per (specifier, fetch type, host-defined import type) and hand the same promise to later callers until it settles. The synchronous fetch path (require(esm)) is untouched and still re-issues its own fetch.
|
Updated 3:42 AM PT - Jul 6th, 2026
❌ @robobun, your commit bb23ac5 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33417That installs a local version of the PR into your bun-33417 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 1 minute 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)
WalkthroughThis PR adds in-flight module fetch coalescing in ChangesIn-flight module fetch coalescing
Possibly related PRs
Suggested labels: jsc, module-loader, tests Suggested reviewers: Jarred-Sumner, Electroid 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/resolve/concurrent-dynamic-import.test.ts`:
- Around line 65-244: These subprocess-spawning tests are still declared with
plain test() even though they all use runFixture/Bun.spawn and tempDir
filesystem I/O, so they should be converted to concurrent execution. Update each
test declaration in concurrent-dynamic-import.test.ts to use test.concurrent (or
wrap the group in describe.concurrent if appropriate) while keeping the existing
assertions and fixtures unchanged.
- Around line 214-244: The test is asserting the `bun test` banner on the wrong
stream; in this fixture the runner output is written to `stderr`, while `stdout`
may be empty. Update the `runFixture` assertion in `a settled fetch stops being
shared once the module registry owns it` to check the `stderr` value alongside
the existing pass/fail expectations, using the `stdout`, `stderr`, and
`exitCode` variables from the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4aa1c80d-1e64-4d25-9526-0266d01abdba
📒 Files selected for processing (4)
src/jsc/bindings/ModuleLoader.cppsrc/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.htest/js/bun/resolve/concurrent-dynamic-import.test.ts
|
Thanks — went through all three. One applied, one declined with evidence, and the issue suggestion doesn't hold up.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/bun/resolve/concurrent-dynamic-import.test.ts (1)
214-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winComment contradicts actual
bun testsubprocess output routing.The new comment states
bun testprints its banner to stdout and everything else to stderr. This is the opposite of documented/observed behavior: whenbun testis run as a subprocess, the pass/fail summary and banner are written to stderr, while stdout remains empty. A prior review on this same test flagged exactly this stream mismatch (asserting the banner on the wrong stream), and that comment was not marked as addressed. If the assertion after this comment still checksstdoutfor the pass/fail text, it will be incorrect/flaky.🔍 Supporting evidence
A confirmed GitHub issue on this exact behavior:
"proc.stderr.toString() // result of \"bun test\"...All tests passed" / "proc.stdout.toString() // empty".Please verify the assertion checks
stderr(notstdout) for the pass/fail banner in this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/bun/resolve/concurrent-dynamic-import.test.ts` around lines 214 - 245, The subprocess stream assertion in the concurrent dynamic import test is checking the wrong output stream for the bun test banner. Update the test around runFixture in concurrent-dynamic-import.test.ts so the pass/fail summary and banner are asserted on stderr, not stdout, and keep stdout only for any non-test output; use the existing stderr/stdout checks in the test body as the place to correct this stream expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/js/bun/resolve/concurrent-dynamic-import.test.ts`:
- Around line 214-245: The subprocess stream assertion in the concurrent dynamic
import test is checking the wrong output stream for the bun test banner. Update
the test around runFixture in concurrent-dynamic-import.test.ts so the pass/fail
summary and banner are asserted on stderr, not stdout, and keep stdout only for
any non-test output; use the existing stderr/stdout checks in the test body as
the place to correct this stream expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55eaa4cb-6506-43b9-a26e-5c2c0fa05f81
📒 Files selected for processing (1)
test/js/bun/resolve/concurrent-dynamic-import.test.ts
|
The re-review re-raises the The banner and the summary go to different streams, which is what the finding conflates: The test asserts No code change for this one. The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Both points addressed. Dead
|
Clearing the map on reload cannot detach the settle reactions already attached to the promises it held. A fetch still in flight across a reload would settle later and remove its key from the map, evicting the entry a post-reload fetch had registered for the same specifier. Put the generation in the key so the stale reaction removes a key that is no longer live. Costs a few characters on a string the fetch already builds.
Stale settle reaction across
|
| build | retained promises |
|---|---|
| this branch | delta: 1600 |
same branch, my src/ changes reverted to the base commit |
delta: 1600 |
Identical. The 4-per-module is entirely JSC's registry entry (m_fetchPromise + m_modulePromise + m_loadPromise, plus the import chain) — the map retains nothing once a fetch settles, which is also what the mock.module test asserts behaviorally. So this PR has no steady-state memory footprint and cannot be the cause.
v8-heap-snapshot.test.ts JSON.parses a whole V8 heap snapshot and materializes every node and edge as JS objects; it is the kind of test that gets OOM-killed under parallel load. This push re-runs CI, which should give a clean read on that lane.
There was a problem hiding this comment.
The generation counter in bb23ac5 closes the --hot stale-reaction window I flagged — nice fix. I'm deferring rather than approving because this is core module-loader C++ (JSC promise reactions, GC-visited state on ZigGlobalObject, interaction with require(esm)'s sync queue and hot reload), which is worth a human pass even with all nits addressed.
Extended reasoning...
Overview
This PR adds an in-flight fetch coalescing cache to Zig::GlobalObject so that N concurrent import() calls of the same specifier share one embedder fetch instead of each re-running the loader/transpiler. Touches ZigGlobalObject.{h,cpp} (new JSMap lazy property, key builder, settle reaction, PromiseFunctions enumerator, reload() clear + generation counter), ModuleLoader.cpp (comment-only), and adds 8 subprocess tests to concurrent-dynamic-import.test.ts.
Since my last review the author addressed all three of my prior comments: test.concurrent (c837440), the dead if (!cached) guard (4951432), and the --hot stale-reaction eviction race — fixed in bb23ac5 by prefixing the fetch key with a generation counter that clearInFlightModuleFetches() bumps, so a pre-reload settle reaction removes a key that no post-reload entry can occupy. That's the exact hardening I suggested and it reads correctly.
Security risks
None identified. The cache is per-GlobalObject (per worker/isolate), keyed on internal module-registry inputs, and only affects whether a redundant fetch is issued — not what any importer receives. No user-controlled data reaches a new trust boundary.
Level of scrutiny
High. moduleLoaderFetch is on the hot path for every dynamic import; the change wires a GC-visited JSMap into ZigGlobalObject, attaches promise reactions via performPromiseThenWithContext, and interacts with hot reload, mock.module(), and the vm.m_synchronousModuleQueue branch that require(esm) takes. The design (key triple mirrors the module registry key, entry dropped on settle before the loader's own reaction runs, sync path deliberately bypassed) is well-reasoned and the test matrix is thorough, but this is exactly the class of change where a maintainer who owns the module loader should confirm the layering and the interaction with #33184/#32999 mentioned in the description.
Other factors
Test coverage is strong (9 tests: virtual + file namespaces, failure fanout, per-key isolation, both import-attribute distinctions, require(esm) race guardrail, mock.module invalidation), the author ran a GC-stress pass under collectContinuously + ASAN, and CodeRabbit's actionable feedback was applied. No bugs were found by the bug hunter this round. I'm not approving solely because module-loader semantics changes are outside the "simple/mechanical" bar for auto-approval, not because of any outstanding concern with the code.
Status: diff is green, one CI lane is stuck on infraBuild 68790 at The one failure is Same agent ( The one test-level failure from the previous build was a flake, and I checked rather than assumedBuild 68760 also had Before writing it off I measured the only way this PR could plausibly bloat a heap snapshot (the in-flight map retaining its promises). 400 specifiers × 3 concurrent importers,
Identical — the 4-per-module is entirely JSC's own registry entry ( Where the PR stands
Ready for review whenever someone has a moment. |
Final CI result for
|
1 participant
Concurrent dynamic
import()s of the same specifier run the module loader once per importer instead of once per module. ABun.pluginonLoadhandler fires N times, and a plain file is transpiled N times; the first result to settle wins the module registry, so the other loads' side effects happen but their output is thrown away.Reproduction
Not specific to virtual modules. The same three concurrent
import("./mod.mjs")calls run a file-namespaceonLoadthree times, and without a plugin they transpile the file three times.Cause
JSModuleLoader::loadModule's dynamic-import overload dedupes an in-flight load by looking up the module registry entry and reusingentry->ensureFetchPromise(). But the entry for a top-level dynamic import is not created until the first fetch settles, inside theModuleLoadTopSettledmicrotask (provideFetch→ensureRegistered). Until thengetRegisteredMayBeNull()returns null, so every importer that arrives in that window falls through tofetch()→GlobalObject::moduleLoaderFetch→ a fresh embedder fetch.Bun__onFulfillAsyncModulealready carried a comment describing this; the FIXME there asked for exactly the behavior this PR implements.The static-import (graph) path does not have the bug: it registers the entry and pipes the fetch promise into it before returning.
Fix
GlobalObject::moduleLoaderFetchkeeps the in-flight embedder fetch promise and hands the same promise to later callers until it settles. The cache:(specifier, ScriptFetchParameters::Type, hostDefinedImportType), the same triple the module registry keys on, so it can never merge two fetches the registry treats as distinct modules (import(p)vsimport(p, { with: { type: "text" } }), andimport(p)vsimport(p, { with: { unknown: "x" } }), stay separate);provideFetchtakes over from there, somock.module()and hot reload still see a clean slate;Zig::GlobalObject(one per worker / per--isolateglobal) as aJSMap, so the GC visits it with the rest of the global's members;GlobalObject::reload()alongside the module registry andrequireMap.Only the async branch changes. The
vm.m_synchronousModuleQueuebranch thatrequire(esm)takes is untouched: it still re-issues its own synchronous fetch, which is what keepsrequire("./x.mjs")working while animport("./x.mjs")of the same file is mid-transpile.Verification
bun bd test test/js/bun/resolve/concurrent-dynamic-import.test.ts— 9 pass. Four of them fail on the unfixed debug build (load: 3whereload: 1is expected). New coverage:onLoad, and the failing-loader path (all three importers reject, loader ran once)require(esm)racing a dynamic import of the same module still resolves synchronouslymock.module()after a settled fetch still wins, proving the in-flight entry was releasedAlso green:
test/js/bun/plugin/,test/js/bun/test/mock/,test/js/node/module/,test/js/bun/worker/,test/cli/hot/, andtest/js/bun/resolve/(the 3 failures there,load the same file 2000 times,load the same empty JS file 2000 timesandrequire(esm) sync queue is a GC root, time out identically onmainin this container).GC stress
300 distinct virtual modules × 4 concurrent importers each, under
BUN_JSC_collectContinuously=1on a debug+ASAN build: exactly 300 loads, every module resolves to its own contents, no sanitizer reports.Relationship to the other open module-loader PRs
Independent of #33184 (
require(esm)racing a static import): that bug needs a registry entry already atFetching, and this PR does not create one — the coalescing happens below the registry. Therequire(esm)race test added here passes onmaintoo and is there as a guardrail.The key triple matches where #32999 takes the registry's own key, so the two compose.