Preserve AsyncLocalStorage context during dynamic import() module evaluation - #32695
Preserve AsyncLocalStorage context during dynamic import() module evaluation#32695robobun wants to merge 5 commits into
Conversation
A module loaded via dynamic import() from inside AsyncLocalStorage.run() evaluated its top-level code with no active store: getStore() returned undefined during module initialization, whereas Node returns the active store. JSC drives dynamic-import evaluation from an internal microtask (DynamicImportLoadSettled -> module->evaluate) that never restores m_asyncContextData, so the imported module's body ran with whatever context happened to be current at evaluation time (undefined). Capture the async context active at the import() call site in moduleLoaderImportModule, keyed by the resolved module key, and reinstate it around the module body in moduleLoaderEvaluate (both the regular and eval-entrypoint paths). The entry is dropped when the import settles synchronously (an already-evaluated module never re-evaluates) and consumed when the body evaluates, so it does not accumulate. This covers the module's synchronous top-level evaluation. A top-level-await module's post-await continuations resume through JSC's async-module machinery outside this hook; the context is restored before that path runs, which keeps it from misreading the wrapped async-context tuple on the current engine.
|
Updated 9:07 AM PT - Jun 25th, 2026
❌ @robobun, your commit 33c7b00 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32695That installs a local version of the PR into your bun-32695 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Thanks, but this PR does not fix #32694, and I confirmed it: the repro still crashes with this change applied. The crash is a type confusion in WebKit's async-module resume, not in the dynamic-import evaluation path this PR touches. Backtrace on a debug build of this branch: In This PR only reinstates the context around a dynamically-imported module's own evaluation ( #32694 is the same root cause as #32178 (the same |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR captures AsyncLocalStorage context at dynamic ChangesDynamic import async-context handling
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/ZigGlobalObject.cpp (1)
3614-3626: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftClear pending context entries when import rejects before evaluation.
Cleanup currently relies on either a non-pending result or
moduleLoaderEvaluate, but pending imports can reject during fetch/parse/dependency loading and never evaluate, leaving the captured store retained and available for a later same-key import. Also clean up on the synchronous exception path at Lines 3616-3617 and attach cleanup to the returned promise’s rejection/settlement path for pending imports that never reach evaluation.🤖 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 `@src/jsc/bindings/ZigGlobalObject.cpp` around lines 3614 - 3626, The pending async context entry is only being cleared for cached/non-pending imports, so rejected imports can still leave stale entries behind. Update the import flow in ZigGlobalObject::importModule around the JSC::importModule call to also remove the asyncContextKey on the synchronous exception path and ensure pending results clean up when the returned promise rejects or settles before reaching moduleLoaderEvaluate. Keep the cleanup tied to m_pendingDynamicImportAsyncContexts so same-key imports cannot reuse a retained store after a failed fetch/parse/dependency load.
🤖 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 `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3600-3607: The pending dynamic import tracking in
ZigGlobalObject::moduleLoaderImportWithDynamicImportContext should preserve the
first context for a module key instead of overwriting it with later concurrent
imports. Introduce a sentinel entry for “no context” so the initial import is
recorded even when asyncContext is undefined, and change the
m_pendingDynamicImportAsyncContexts update logic to only insert a key if it is
not already present. Keep the fix localized around asyncContextKey creation and
the map->set/get flow so the first import’s call-site state is what drives
evaluation.
- Around line 3591-3610: The virtual-module import path in
ZigGlobalObject::importModule bypasses the new AsyncLocalStorage capture block
because resolveVirtualModule(...) returns early from JSC::importModule(...).
Refactor that branch to reuse the shared async-context capture flow by setting
resolvedIdentifier and falling through to the existing import logic, or extract
the capture/cleanup into a helper and call it from both paths so plugin/virtual
modules preserve the caller’s context.
---
Outside diff comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3614-3626: The pending async context entry is only being cleared
for cached/non-pending imports, so rejected imports can still leave stale
entries behind. Update the import flow in ZigGlobalObject::importModule around
the JSC::importModule call to also remove the asyncContextKey on the synchronous
exception path and ensure pending results clean up when the returned promise
rejects or settles before reaching moduleLoaderEvaluate. Keep the cleanup tied
to m_pendingDynamicImportAsyncContexts so same-key imports cannot reuse a
retained store after a failed fetch/parse/dependency load.
🪄 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: 9fb2418f-e585-4774-a4ac-05734e16986e
📒 Files selected for processing (3)
src/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.htest/js/node/async_hooks/AsyncLocalStorage.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…mport failure Extract the capture into a helper and apply it to the plugin virtual-module import path as well, so dynamically imported virtual modules also evaluate with the caller's AsyncLocalStorage context. Remove the captured entry when an import never reaches evaluation: on the synchronous importModule exception path, and on async load failure via Bun__onFulfillAsyncModule's reject path. Previously only cached (synchronously settled) imports were cleaned up, so a pending import that later rejected (syntax/transpile error, failed dependency) retained the captured context for the lifetime of the VM. Concurrent imports of the same not-yet-loaded module keep last-writer-wins semantics, which self-heals a stale entry on the next import of that key. Adds a regression test for a failing dynamic import inside a run() scope.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3638-3644: The cleanup in ZigGlobalObject::moduleLoaderEvaluate
only drops the async context for the current fetch-failure key, so the root
dynamic import context can remain pinned on transitive failures. Update the
dynamic import rejection/failure path for resolvedIdentifier to also call
dropDynamicImportAsyncContext for the root asyncContextKey, not just the
Bun__onFulfillAsyncModule fetch-failure seam. Keep the existing pending-status
guard logic, but ensure the root import’s captured store is released when its
promise rejects before evaluation.
🪄 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: 554d0465-a5f4-4203-9580-5abc0f6fe421
📒 Files selected for processing (3)
src/jsc/bindings/ModuleLoader.cppsrc/jsc/bindings/ZigGlobalObject.cpptest/js/node/async_hooks/AsyncLocalStorage.test.ts
… without one A dynamic import whose own source fetches but then fails to link (a transitive dependency with a syntax/resolution error, an async plugin onLoad rejection) never reaches moduleLoaderEvaluate and has no Bun-side fetch-failure seam, so its captured-context entry was left in the map. A later import of that same key with no active context previously early-returned without touching the entry, so the stale context could be applied to that evaluation. Clear the entry for the key when capturing finds no active context. Combined with the existing overwrite on a contextful import, the map now always reflects the most recent import's call-site state, so a stale entry can never drive a later evaluation and is released on the next import of that key. Not done via a reaction on the returned import promise: attaching one marks the user's promise handled and would suppress (or, re-thrown, spuriously emit) the unhandled-rejection report for a fire-and-forget failed import().
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/ZigGlobalObject.cpp (1)
3486-3494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim new comments to the 3-line limit.
These added comments exceed the repository’s 3-line max. Please compress them or move extended rationale to the PR description.
As per coding guidelines, "Keep code comments to 3 lines max."
Also applies to: 3644-3648, 3758-3764
🤖 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 `@src/jsc/bindings/ZigGlobalObject.cpp` around lines 3486 - 3494, The newly added explanatory comments in ZigGlobalObject.cpp exceed the repository’s 3-line comment limit and need to be compressed. Shorten the comment blocks around the dynamic import async-context logic in the relevant areas, including the sections associated with evaluateModuleWithCapturedAsyncContext and the other noted comment blocks, while preserving only the essential summary and moving extra rationale to the PR description.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/ZigGlobalObject.cpp`:
- Around line 3486-3494: The newly added explanatory comments in
ZigGlobalObject.cpp exceed the repository’s 3-line comment limit and need to be
compressed. Shorten the comment blocks around the dynamic import async-context
logic in the relevant areas, including the sections associated with
evaluateModuleWithCapturedAsyncContext and the other noted comment blocks, while
preserving only the essential summary and moving extra rationale to the PR
description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 218b3cec-df32-470e-af3c-2b061d823fc5
📒 Files selected for processing (1)
src/jsc/bindings/ZigGlobalObject.cpp
The JSMap set/get/remove on m_pendingDynamicImportAsyncContexts each open an internal JSC ThrowScope, and every throw scope records a need-exception-check on return. Without a check before the next scope (JSC::importModule -> requestImportModule, or evaluateNonVirtual), the exception-check validator (BUN_JSC_validateExceptionChecks, enabled in the x64-asan CI lane) aborts with "unchecked exception", failing the new AsyncLocalStorage test and bun-server. Check the scope after each JSMap operation: - moduleLoaderImportModule: RETURN_IF_EXCEPTION after captureDynamicImportAsyncContext (which does the set), and assertNoException after the cached-module drop. - evaluateModuleWithCapturedAsyncContext: function-level scope, RETURN_IF_EXCEPTION after get/remove, RELEASE_AND_RETURN / release() around evaluateNonVirtual. Use get (returns jsUndefined() when absent) instead of has+get to avoid two lookups. - Bun__onFulfillAsyncModule: assertNoException after remove. These maps are JSString-keyed builtins, so the operations only throw on OOM; assertNoException is used where the op cannot allocate (remove/find), propagation where it can (set/get). The validator's simulated throws set the check flag but not a real exception, so capture still runs normally (verified with BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1).
|
Update after build 64634 finished: the diff is green; every CI red is on a test this PR does not touch (it only changes the dynamic-import hooks in
So the NAPI reds are systematic and unrelated rather than flaky; the diff itself is green. Could a maintainer confirm the Windows NAPI failures are pre-existing on main and merge, or advise if anything here is actually expected from this change? I have used my one re-roll. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Terrible implementation. If we do want this, it should be done in WebKit.
|
Understood, thanks. Agreed the engine is the right layer. The clean place is the dynamic-import pipeline itself: capture the active context at the Closing this. The proper fix belongs in oven-sh/WebKit; #32693 stays open to track it. |
2 participants
Fixes #32693
Repro
store.mjsindex.mjsimported.mjsCause
Since the module-loader rewrite, JSC drives dynamic-import evaluation from an internal microtask (
DynamicImportLoadSettled->module->evaluate) that never restoresm_asyncContextData. Unlikeawait, timers, andqueueMicrotask(whose reactions snapshot/restore the async-context slot), that path leaves the slot at whatever was current at evaluation time, so the imported module's top-level code runs with no active store.enterWith()andAsyncLocalStorage.snapshot()/.bind()do not help, because the slot is reset between microtask ticks before the deferred evaluation runs.Fix
Thread the context through the resolved module key, entirely in Bun's module hooks:
moduleLoaderImportModule: if an async context is active at theimport()call site, record it keyed by the resolved module key. Drop the entry again when the import settles synchronously (an already-evaluated/cached module never re-evaluates).moduleLoaderEvaluate(regular and eval-entrypoint paths): if the module being evaluated has a recorded context, reinstate it around the module body and restore it afterward, mirroring the existingAsyncContextFramecall pattern. The entry is consumed on evaluation, so the map does not accumulate.Node preserves this via V8's continuation-preserved embedder data.
Scope
This covers the imported module's synchronous top-level evaluation (the reported case). A top-level-await module's post-
awaitcontinuations resume through JSC's async-module machinery (AsyncModuleExecutionResume), which runs outside this hook; the context is restored before that path runs, so the synchronous prefix sees the store and the resume path does not misread the wrapped async-context tuple on the current WebKit pin (the type-confusion crash in #32178, addressed separately by #32184 / oven-sh/WebKit#252). Propagating context across a dynamically-imported TLA module's awaits can build on that engine-side work in a follow-up.Verification
Added to
test/js/node/async_hooks/AsyncLocalStorage.test.ts:run()inside it) observe the active store; an import with no active context evaluates with an undefined store (no leak into unrelated imports);Both fail on the released build (
eval:undefined) and pass with this change.test/js/node/async_hooks/and the dynamic-import/TLA suites intest/js/bun/resolve/pass.