JSC: propagate async context through PromiseFinallyAwaitJob and PromiseResolveWithoutHandlerJob - #268
JSC: propagate async context through PromiseFinallyAwaitJob and PromiseResolveWithoutHandlerJob#268robobun wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the reordering cleanly mirrors the existing PromiseReactionJob pattern — but since this changes observable async-context semantics on the async-function settle path, it's worth a quick human sanity check.
Extended reasoning...
Overview
This PR reorders two statements in each of the two terminal arms of InternalMicrotask::AsyncFunctionResume in JSMicrotask.cpp: it now calls promise->reject() / promise->resolve() before restoring the Bun async-context slot, instead of after. The net diff is moving scope.release(); promise->reject/resolve(...) above the asyncContextData->putInternalField(vm, 0, restoreAsyncContext) restore in both branches, plus a comment explaining the ordering.
Correctness
The new ordering is a direct copy of the already-established pattern in PromiseReactionJob a few dozen lines above (lines ~1855–1866), which explicitly documents the same invariant. asyncContextData is rooted via globalObject->m_asyncContextData and restoreAsyncContext is a stack JSValue, so there's no GC hazard from calling into reject()/resolve() before the restore. putInternalField doesn't need the throw scope, so scope.release() preceding it is fine — again identical to the reference pattern. The audit table in the PR description matches what I see in the file.
Security risks
None. This is a Bun-only (USE(BUN_JSC_ADDITIONS)) reordering of when an async-context slot is restored; no new inputs, no parsing, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. The diff is mechanically tiny and pattern-matched against existing code, and the bug-hunting pass found nothing. However, it lives on the hot path of async function completion in JavaScriptCore and intentionally changes what the embedder's promiseRejectionTracker (and thenable-resolution jobs) observe. That's a deliberate, user-visible semantic change to AsyncLocalStorage behavior across await, so I'd rather a human confirm the intended semantics than auto-approve.
Other factors
Verification is via the downstream oven-sh/bun#31721 fixture suite rather than an in-tree JSC test, and there are no prior reviewer comments on the PR. Given it's engine-runtime behavior rather than a config/typo change, deferring.
42d949c to
ee98a20
Compare
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 41 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesBun async contexts now propagate through promise reactions, thenable resolution, promise settlement, and Promise async context propagation
Merge Risk: ⚪ Minimal · up to The change propagates async context through promise settlement paths to align rejection handling with Node behavior; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly explains the problem, affected microtasks, implementation, related bugs, audit scope, and verification results. It does not include every legacy template field, such as a Bugzilla URL, reviewer line, or explicit file/function list, but the essential technical information is complete. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
There was a problem hiding this comment.
LGTM — small reorder that mirrors the established PromiseReactionJob pattern in the same file.
Extended reasoning...
Overview
This PR reorders two statements in each of the two terminal arms of InternalMicrotask::AsyncFunctionResume in Source/JavaScriptCore/runtime/JSMicrotask.cpp. Previously the async context was restored before calling promise->reject() / promise->resolve(); now the settle happens first and the context is restored afterwards. A comment is added referencing the identical ordering already used by PromiseReactionJob at line ~1855 in the same file.
Security risks
None. The change is a pure statement reorder inside #if USE(BUN_JSC_ADDITIONS) guards. putInternalField is a simple internal-field store with a write barrier and does not depend on the exception scope, so moving it after scope.release() and the settle call is safe — this is exactly what PromiseReactionJob already does a few dozen lines above (verified at JSMicrotask.cpp:1858-1866). No new user-controlled data flow, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. While this is core JSC async/await runtime code, the change is mechanical: it aligns two branches with an existing, documented, in-tree pattern rather than introducing new logic. The PR description includes a thorough audit of every putInternalField(vm, 0, restoreAsyncContext) site in the file confirming the other branches already follow the settle-then-restore ordering. The behavioural delta is narrow (what promiseRejectionTracker and thenable-resolution jobs observe as the current async context), and .then()/.catch() handler semantics are unaffected since performPromiseThen captures context at registration time.
Other factors
The change is Bun-specific (entirely inside USE(BUN_JSC_ADDITIONS)), is consumed and tested by oven-sh/bun#31721 with fixtures that fail before and pass after, and the multi-agent bug hunter found no issues. No prior human or bot review comments to address. Net diff is ~10 lines of reordering plus a 4-line explanatory comment.
Preview Builds
|
oven-sh/WebKit#268 settles the async function's promise before restoring the async context, so a function that fails after an await reports its rejection with its own context still installed. Un-skips the async-fn fixture, which now passes on bun and node alike. Pinned to the PR's preview build; re-pin to the autobuild tag of its merge commit once it lands on WebKit main.
ee98a20 to
48232e3
Compare
There was a problem hiding this comment.
I didn't find any issues, but this reorders exception-scope release and promise settlement in JSC's async function resume path, so it's worth a human sign-off.
What was reviewed:
- Confirmed the new ordering matches
PromiseReactionJobat JSMicrotask.cpp:1858-1867 (settle → restore). putInternalFieldis a plain write-barrier store (InternalFieldTuple.h:75) — safe afterscope.release()even with a pending exception fromreject()/resolve().- Termination-exception early-return and the await-continuation arm still restore context correctly.
Extended reasoning...
Overview
Two-branch reorder in runInternalMicrotask's AsyncFunctionResume case (Source/JavaScriptCore/runtime/JSMicrotask.cpp). In both terminal arms — the error reject arm and the State::Executing resolve arm — the Bun async-context restore (asyncContextData->putInternalField(vm, 0, restoreAsyncContext)) is moved from before scope.release(); promise->reject/resolve(...) to after it. A comment is added referencing the identical ordering already used in PromiseReactionJob.
Security risks
None. The change is a reorder of two existing statements inside #if USE(BUN_JSC_ADDITIONS) blocks; no new inputs, no auth/crypto/permission surface.
Level of scrutiny
Moderate-to-high. The diff is tiny and mechanically mirrors the established PromiseReactionJob pattern in the same function (lines 1858-1867 and 1875-1880), which already does scope.release(); settle; restore;. I verified putInternalField is a bare WriteBarrier::set with no exception checks, so calling it after scope.release() with a possibly-pending exception is safe — same as the reference case. That said, this is the JSC microtask/async-resume hot path, where ordering between ThrowScope::release(), promise settlement, the rejection tracker callback, and async-context slot mutation is subtle and observable to embedders. Per the approval guidelines for critical runtime code paths, I'm deferring rather than auto-approving.
Other factors
- The PR description includes a full audit table of every
restoreAsyncContextsite in the file; I spot-checked it and it's accurate. - Downstream tests exist in oven-sh/bun#31721 covering the
unhandledRejection+AsyncLocalStoragescenario. - No prior human or bot review on the thread (CodeRabbit was rate-limited).
- The bug hunting system found no issues.
oven-sh/WebKit#268 settles the async function's promise before restoring the async context, so a function that fails after an await reports its rejection with its own context still installed. Un-skips the async-fn fixture, which now passes on bun and node alike. Pinned to the PR's preview build (on top of WebKit 4895f45d, so it keeps the shared-allocator change from #34009). Re-pin to the autobuild tag of its merge commit once it lands on WebKit main.
…fixture cases The claim that these arms were already correct was wrong: a .finally() callback that returns a rejected thenable settles from PromiseFinallyAwaitJob, which did not carry the async context across, so the unhandledRejection handler observed undefined. Fixed alongside the AsyncFunctionResume ordering in oven-sh/WebKit#268. Add fixture cases for both .finally() shapes and two async-generator shapes. The file is back in the tracking test's todos until the WebKit pin picks up the PromiseFinallyAwaitJob fix.
There was a problem hiding this comment.
I didn't find any bugs, but this touches core JSC promise/async-context machinery and changes observable AsyncLocalStorage semantics, so it warrants a human look. Note that the diff also includes a second commit (PromiseFinallyAwaitJob context propagation) that isn't covered by the PR description — the audit table there marks PromiseFinallyReactionJob as "already correct", but this PR now modifies it.
What was reviewed:
AsyncFunctionResumereorder matches thePromiseReactionJobreference pattern exactly;putInternalFieldafterscope.release()is a plain write-barrier store, safe post-release.- New
InternalFieldTuplewrap/unwrap forPromiseFinallyAwaitJobfollows the same shape asAsyncGeneratorYieldAwaited/PromiseReactionJob; the synchronouspromiseFinallyAwaitJob(...)call sites keep passing rawcontext(no wrap needed since context is still installed). - Checked that
restoreAsyncContextis only read whenasyncContextDatais non-null in all new arms.
Extended reasoning...
Overview
Single-file change to Source/JavaScriptCore/runtime/JSMicrotask.cpp with two logically separate pieces:
-
AsyncFunctionResumesettle ordering (matches title/description): reorders the error-reject andExecuting-resolve arms to callpromise->reject()/promise->resolve()before restoring the Bun async-context slot, sopromiseRejectionTracker()and thenable-resolution jobs observe the async function's context. This is a two-line swap in each arm plus a comment, and mirrors the existingPromiseReactionJobordering line-for-line. -
PromiseFinallyAwaitJobcontext propagation (second commit0aef04ea, not in the PR description):promiseFinallyReactionJobnow captures the current async context into anInternalFieldTuplealongside the reaction context when scheduling phase 2, and thePromiseFinallyAwaitJobcase unwraps it and installs/restores aroundpromiseFinallyAwaitJob(...). This is net-new behavior, though it follows the sameInternalFieldTuple-wrap pattern already used byPromiseReactionJob,AsyncGeneratorYieldAwaited, andAsyncFromSyncIteratorContinue.
Security risks
None apparent. No parsing, no untrusted input handling, no auth/crypto. The change adjusts when a per-VM InternalFieldTuple slot is written and adds a heap allocation (InternalFieldTuple::create) on the .finally() scheduling path. The dynamicDowncast<InternalFieldTuple> discriminator is type-safe; the fallback uncheckedDowncast<JSSlimPromiseReaction> matches pre-existing behavior for the non-wrapped case.
Level of scrutiny
High. This is core JavaScriptCore promise/microtask machinery on the hot path of every await and .finally(). It intentionally changes what Bun's unhandledRejection handler and thenable-resolution jobs observe from AsyncLocalStorage, i.e., user-visible semantics. The change is small and pattern-following, but the surface it touches is exactly the kind where subtle ordering bugs (double-restore, leaked context, missed restore on early return) are easy to introduce and hard to detect. A maintainer familiar with the Bun async-context design should confirm the intended semantics — particularly for the second commit, which the PR description doesn't cover.
Other factors
- The PR description's audit table is stale: it lists
PromiseFinallyReactionJobas "already correct / unchanged", but the diff modifies it. The description should be updated or the second commit split out. putInternalFieldis a simpleWriteBarrier::set(checkedInternalFieldTuple.h) — safe to call afterscope.release(), consistent with all sibling arms.- The synchronous
promiseFinallyAwaitJob(globalObject, vm, result, context, ...)call sites inpromiseFinallyReactionJobcorrectly keep passing rawcontextrather thanscheduledContext, since the async context is still installed at that point (phase-1 wrapper restores it). - Downstream test coverage exists in oven-sh/bun#31721 per the description, but no in-tree JSC tests are added here.
- No prior human or bot reviews with substantive feedback on the timeline.
oven-sh/WebKit#268's second commit carries the async context through PromiseFinallyAwaitJob, so a .finally() callback that returns a rejected thenable now reports its unhandled rejection with the callback's store. Un-skips the async-fn fixture (now 7 cases, all passing on bun and node). Also makes the "unhandledRejection async context" block concurrent — 8 hermetic subprocess spawns, so there's no reason to run them sequentially.
0aef04e to
86efe05
Compare
robobun
changed the title
There was a problem hiding this comment.
I didn't find any issues — the capture/install/restore logic is a close mirror of the existing PromiseFinallyReactionJob case and the other InternalFieldTuple-wrapped microtask handlers in this file. Deferring to a human because this sits on the JSC promise microtask dispatch path.
What was reviewed:
- Confirmed the wrap/unwrap pattern matches phase 1 (
PromiseFinallyReactionJob) and the ~8 otherdynamicDowncast<InternalFieldTuple>sites inrunInternalMicrotask. - Verified the two schedule points now carry
scheduledContextwhile the synchronouspromiseFinallyAwaitJobcalls still receive the rawcontext(they run inside the phase-1 install window). - Checked that
scope.release()ordering and the post-callputInternalFieldrestore match the established pattern; non-Bun builds are unchanged behind#if USE(BUN_JSC_ADDITIONS).
Extended reasoning...
Overview
This PR modifies Source/JavaScriptCore/runtime/JSMicrotask.cpp to propagate Bun's async context (AsyncLocalStorage) through the PromiseFinallyAwaitJob internal microtask — the phase-2 job that runs when a .finally() callback returns a thenable. It has two pieces:
- In
promiseFinallyReactionJob(phase 1), before scheduling phase 2 viaperformPromiseThenWithInternalMicrotaskorcreateResolvingFunctionsWithInternalMicrotask, wrap theJSSlimPromiseReactioncontext in anInternalFieldTuplealongside the current async context (only when one is active). - In the
InternalMicrotask::PromiseFinallyAwaitJobcase ofrunInternalMicrotask, unwrap that tuple, install the captured async context intoglobalObject->m_asyncContextData, runpromiseFinallyAwaitJob, then restore the previous value.
Both pieces are guarded by #if USE(BUN_JSC_ADDITIONS); the non-Bun path is byte-for-byte unchanged.
Security risks
None identified. This is internal promise-reaction bookkeeping — no user-controlled input parsing, no auth/crypto/permissions surface. The uncheckedDowncast<JSSlimPromiseReaction> on the unwrapped field is safe because the only writer of that slot is the phase-1 code added in this same PR, which always stores a JSSlimPromiseReaction*. The InternalFieldTuple is GC-managed, so the captured context and async-context values are kept alive across the microtask boundary the same way they are in every other case in this file.
Level of scrutiny
High. runInternalMicrotask is on the hot path for every promise reaction in Bun, and mistakes in async-context install/restore can leak state across unrelated tasks. That said, the change is not novel logic — it is a near-verbatim copy of the PromiseFinallyReactionJob case immediately above it (JSMicrotask.cpp:2176-2214) and structurally identical to the async-generator and module cases at lines ~1940/2059/2088/2117/2146/2361. I compared each step (tuple detection, asyncContextData null-guard, restoreAsyncContext capture, scope.release() before the job call, restore after) and they line up exactly.
I also verified the four synchronous promiseFinallyAwaitJob(...) calls inside promiseFinallyReactionJob (non-object result, non-thenable, .then getter throw, non-callable .then) still pass the raw context, not scheduledContext — correct, since those run while phase 1's async context is still installed by the caller.
Other factors
- The PR description includes an audit table of all internal-microtask cases that settle promises under an async context, with this being the last gap after #295.
- Companion test coverage lives in oven-sh/bun#31721 (
async-context-unhandled-rejection-async-fn.js), validated against Node. - No prior human review comments to address; only bot summaries in the timeline.
Given the critical code path, I'm deferring rather than auto-approving, but I have no concrete concerns to raise.
|
Rebased onto main: the |
oven-sh/WebKit#268 settles the async function's promise before restoring the async context, so a function that fails after an await reports its rejection with its own context still installed. Un-skips the async-fn fixture, which now passes on bun and node alike. Pinned to the PR's preview build (on top of WebKit 4895f45d, so it keeps the shared-allocator change from #34009). Re-pin to the autobuild tag of its merge commit once it lands on WebKit main.
…fixture cases The claim that these arms were already correct was wrong: a .finally() callback that returns a rejected thenable settles from PromiseFinallyAwaitJob, which did not carry the async context across, so the unhandledRejection handler observed undefined. Fixed alongside the AsyncFunctionResume ordering in oven-sh/WebKit#268. Add fixture cases for both .finally() shapes and two async-generator shapes. The file is back in the tracking test's todos until the WebKit pin picks up the PromiseFinallyAwaitJob fix.
oven-sh/WebKit#268's second commit carries the async context through PromiseFinallyAwaitJob, so a .finally() callback that returns a rejected thenable now reports its unhandled rejection with the callback's store. Un-skips the async-fn fixture (now 7 cases, all passing on bun and node). Also makes the "unhandledRejection async context" block concurrent — 8 hermetic subprocess spawns, so there's no reason to run them sequentially.
86efe05 to
5f70edc
Compare
|
Rebased onto main again: #301 introduced |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
Source/JavaScriptCore/runtime/JSMicrotask.cpp (1)
803-860: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefer async-context wrapping until deferred work is needed.
wrapWithCurrentallocates anInternalFieldTuplewhenever a context is active, but synchronous, primitive, non-thenable, and non-callable results execute inline and never usescheduledContext. Capture it immediately before the two deferred scheduling calls to avoid an allocation on the common synchronous path.🤖 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 `@Source/JavaScriptCore/runtime/JSMicrotask.cpp` around lines 803 - 860, Move the AsyncContextSwapScope::wrapWithCurrent call out of the initial setup and defer it until just before the deferred PromiseFinallyAwaitJob scheduling paths: the fast JSPromise performPromiseThenWithInternalMicrotask call and createResolvingFunctionsWithInternalMicrotask. Keep synchronous, primitive, non-thenable, and non-callable branches using the original context directly, while passing the wrapped context only to deferred work.
🤖 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 `@Source/JavaScriptCore/runtime/JSMicrotask.cpp`:
- Around line 803-860: Move the AsyncContextSwapScope::wrapWithCurrent call out
of the initial setup and defer it until just before the deferred
PromiseFinallyAwaitJob scheduling paths: the fast JSPromise
performPromiseThenWithInternalMicrotask call and
createResolvingFunctionsWithInternalMicrotask. Keep synchronous, primitive,
non-thenable, and non-callable branches using the original context directly,
while passing the wrapped context only to deferred work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: afd1840e-84f7-4eb5-a62b-91526d29ce59
📒 Files selected for processing (1)
Source/JavaScriptCore/runtime/JSMicrotask.cpp
There was a problem hiding this comment.
I didn't find any issues — the change mechanically applies the same wrapWithCurrent / unwrapContextTuple + AsyncContextSwapScope pattern already used by PromiseFinallyReactionJob and the other async-generator cases in this file. Deferring to a human because this is core JSC promise-reaction machinery and part of a subtle async-context series (#278/#295/#301).
What was reviewed:
wrapWithCurrentreturnscontextunchanged when no async context is active, so the fast/non-ALS path and non-Bun builds (#elsebranch) are unaffected.unwrapContextTuplemutatescontextArgin place before theuncheckedDowncast<JSSlimPromiseReaction>, so the cast is still safe when a tuple was passed.- The synchronous
promiseFinallyAwaitJob(...)calls still pass the rawcontext— correct, since they run inside the phase-1 scope's install window.
Extended reasoning...
Overview
The PR touches a single file, Source/JavaScriptCore/runtime/JSMicrotask.cpp, adding ~19 lines across two locations:
- In
promiseFinallyReactionJob(phase 1), wrap theJSSlimPromiseReaction*context with the current Bun async context viaAsyncContextSwapScope::wrapWithCurrentbefore handing it to the two schedule points that enqueuePromiseFinallyAwaitJob(performPromiseThenWithInternalMicrotaskandcreateResolvingFunctionsWithInternalMicrotask). - In the
InternalMicrotask::PromiseFinallyAwaitJobdispatch case, unwrap that tuple withunwrapContextTupleand install the async context via an RAIIAsyncContextSwapScope, exactly as the adjacentPromiseFinallyReactionJobcase already does.
Both additions are guarded by #if USE(BUN_JSC_ADDITIONS); the #else branch keeps scheduledContext = context, so non-Bun builds see no behavioral or codegen change.
Security risks
None identified. This is internal promise-reaction plumbing with no user-controlled input beyond what already flows through the existing code paths. The added allocation (InternalFieldTuple::create inside wrapWithCurrent) is the same one already performed for the sibling reaction jobs and is GC-safe (all live values are on the stack or already stored in the reaction via setHandlerOrContext).
Level of scrutiny
This is core JSC promise-reaction runtime — every .finally() that returns a thenable flows through here — so it warrants a real look from someone who owns Bun's AsyncLocalStorage semantics. The change itself is mechanical (it copies the pattern from the case block ~20 lines above verbatim), but async-context propagation ordering has been subtle enough to need a multi-PR series (#278, #295, #301, and this one), so I'd rather a human confirm the settle-vs-restore ordering here matches the intended ALS semantics.
Other factors
- The non-Bun fast path is preserved:
wrapWithCurrentreturns the raw context when no async context is active, so no extra allocation on the common path. - The four synchronous
promiseFinallyAwaitJob(globalObject, vm, ..., context, ...)calls inpromiseFinallyReactionJobintentionally keep the rawcontextrather thanscheduledContext— they execute inside the phase-1AsyncContextSwapScope, so wrapping would be redundant. The PR description calls this out. unwrapContextTupletakesJSValue&and rewrites it to field 0 before returning field 1, so the subsequentuncheckedDowncast<JSSlimPromiseReaction>(contextArg)remains type-correct whether or not a tuple was passed.- Downstream test coverage exists in oven-sh/bun#31721.
- Bug hunting system found nothing.
oven-sh/WebKit#268 rebased onto WebKit main a8d15c1c and rewritten to use the AsyncContextSwapScope helper from #301, so it now matches every other microtask case (one wrapWithCurrent at the schedule point, one unwrapContextTuple + RAII scope in the case). Same behaviour, 19 lines instead of 51.
oven-sh/WebKit#268 settles the async function's promise before restoring the async context, so a function that fails after an await reports its rejection with its own context still installed. Un-skips the async-fn fixture, which now passes on bun and node alike. Pinned to the PR's preview build (on top of WebKit 4895f45d, so it keeps the shared-allocator change from #34009). Re-pin to the autobuild tag of its merge commit once it lands on WebKit main.
…fixture cases The claim that these arms were already correct was wrong: a .finally() callback that returns a rejected thenable settles from PromiseFinallyAwaitJob, which did not carry the async context across, so the unhandledRejection handler observed undefined. Fixed alongside the AsyncFunctionResume ordering in oven-sh/WebKit#268. Add fixture cases for both .finally() shapes and two async-generator shapes. The file is back in the tracking test's todos until the WebKit pin picks up the PromiseFinallyAwaitJob fix.
oven-sh/WebKit#268's second commit carries the async context through PromiseFinallyAwaitJob, so a .finally() callback that returns a rejected thenable now reports its unhandled rejection with the callback's store. Un-skips the async-fn fixture (now 7 cases, all passing on bun and node). Also makes the "unhandledRejection async context" block concurrent — 8 hermetic subprocess spawns, so there's no reason to run them sequentially.
5791471 to
4b44c71
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches core JSC promise-reaction and microtask dispatch — a hot, correctness-sensitive path — a human look would still be worthwhile.
Checked that unwrapContextTuple handles every shape now passed as arguments[2] to PromiseResolveWithoutHandlerJob (undefined, null, and InternalFieldTuple from all four call sites) and leaves contextArg correctly typed for the uncheckedDowncast<JSSlimPromiseReaction> in PromiseFinallyAwaitJob.
Checked that moving the AsyncContextSwapScope above the species check in PromiseResolveThenableJobFast is safe — the RAII scope is torn down by RELEASE_AND_RETURN, and the slow path's performPromiseThen now captures the intended context.
Checked that the triggerPromiseReactions change stays inside the existing USE(BUN_JSC_ADDITIONS) block and reaches the 3-arg queueMicrotask, matching what the job case reads.
Extended reasoning...
Overview
The PR threads Bun's async context (AsyncLocalStorage) through two internal microtasks that previously dropped it: PromiseFinallyAwaitJob (phase 2 of .finally() when the callback returns a thenable) and PromiseResolveWithoutHandlerJob (handler-less side of a .then() reaction). It touches JSMicrotask.cpp (promiseFinallyReactionJob, the PromiseResolveThenableJobFast / PromiseResolveWithoutHandlerJob / PromiseFinallyAwaitJob cases in runInternalMicrotask) and JSPromise.cpp (performPromiseThen, performPromiseThenWithContext, triggerPromiseReactions). Every functional change is under #if USE(BUN_JSC_ADDITIONS).
Security risks
None identified. The change carries an existing internal JSValue (either jsUndefined() or an engine-created InternalFieldTuple) through an extra microtask argument slot and installs it via the existing AsyncContextSwapScope RAII helper. No new user-controlled input reaches a downcast or allocation path; unwrapContextTuple uses dynamicDowncast and tolerates empty/non-tuple values.
Level of scrutiny
High. performPromiseThen, triggerPromiseReactions, and runInternalMicrotask sit on the hot path of every promise chain in the engine. The change is small (~50 lines net) and closely mirrors the already-merged patterns for PromiseReactionJob and PromiseFinallyReactionJob, and the no-context fast paths are preserved (wrapWithCurrent returns the input unchanged when no async context is active). But a subtle mistake here — a leaked context, a wrong tuple field, or a mistyped uncheckedDowncast — would be a runtime-wide correctness or crash issue, and the reordering of the AsyncContextSwapScope relative to the species-watchpoint check in PromiseResolveThenableJobFast is the kind of thing a maintainer familiar with the microtask contract should sign off on.
Other factors
The PR description includes a thorough audit table and end-to-end verification against Node.js and the Next.js reproduction, and preview builds have been produced across multiple rebases. There are no outstanding human review comments and no prior review from me. The bug-hunting pass found nothing. Given the criticality of the code path, deferring rather than auto-approving.
The unhandledRejection event is emitted from the end-of-tick drain, after the context that was live at rejection time has been unwound, so AsyncLocalStorage.getStore() observed undefined inside the listener. promiseRejectionTracker(Reject) now queues the promise wrapped in an AsyncContextFrame (via AsyncContextFrame::withAsyncContextIfNeeded) when a context is active; both Handle-path lookups unwrap through one helper. handleRejectedPromises installs the captured context around the dispatch (replaying undefined for contextless entries so a re-entrant drain does not leak the caller's store) and restores it afterwards. The microtask drains in unhandled_rejection() and the auto-GC after the dispatch run under a ClearedAsyncContextScope, as does the isBunTest early-return, since the propagation machinery assumes the ambient slot is undefined during a top-level drain. A throwing unhandledRejection listener is reported after the slot is cleared: EventEmitter gains an emit overload that returns the listener's throw instead of reporting it inside the installed window. As in Node, that throw also halts later listeners. This pins the rejection-time context (Node >= 24 / async-context-frame semantics), not the creation-time context Node 22's default ALS replays. Tests: dual-runtime fixtures under async-context/ (sync rejections in two stores, a contextless rejection, a timer rejection, a final in-context rejection plus a context-free poll that catches a leaked slot; and the async-fn shapes: await-throw, awaited native rejection, escaped async fn, two async-generator shapes, both .finally() shapes), bun-only tests for the rejection-time semantic, same-tick handling, the re-entrant drain, strict mode (keeps the context for uncaughtException but not the drain), the throwing listener with a persistent enterWith, and a spawned `bun test` run; and the #32554 regression test parametrized with AsyncLocalStorage so its in-flight scan sees frame-wrapped entries. The async-fn fixture stays in the tracking test's todos until WEBKIT_VERSION picks up oven-sh/WebKit#268, which carries the remaining JSC half (PromiseFinallyAwaitJob did not carry the async context across).
…tures - Every new comment in src/ is one line; the reasoning lives in the PR and commit messages. - The exception-returning emit overload delegates to fireEventListeners, which takes the optional out-parameter, instead of duplicating its body. - The dual-runtime test.each tables skip the node leg when node is absent. - Both fixtures fail on an unknown or duplicate delivery. The async-fn fixture's await-throw case resumes from a macrotask (the cross-tick shape) and the rest resume from a microtask; the one case that needs oven-sh/WebKit#268 moves to its own fixture so the other six are enforced now, and only that fixture is in the tracking test's todos.
oven-sh/WebKit#268, rebased onto current WebKit main, carries the async context through PromiseFinallyAwaitJob, so a .finally() callback that returns a rejected thenable now reports its rejection with the callback's store. The finally-thenable fixture passes on bun and node and comes out of the tracking test's todos. The preview's asset set matches main's current autobuild exactly (42 tarballs). Re-pin to the merge commit's autobuild before merging.
…dlerJob fix The previous preview (e3c620df) sat one WebKit commit behind main's pin, so it lacked the FTL FFI fix from oven-sh/WebKit#477 and ffi.test.js crashed on both Windows lanes. The new preview (33161987) is oven-sh/WebKit#268 on top of current WebKit main. oven-sh/WebKit#268 gained a second commit. A promise that adopts another promise's settlement without a handler of its own (p.then(f) with p rejecting, resolve(otherPromise), an async function returning a promise) is settled by PromiseResolveWithoutHandlerJob, and JSC dropped the context then() had captured on every path that queues that job. Those derived promises are the rejections Next.js 16 filters by reading its prerender store from the unhandledRejection listener (#39847); with the bun-side change alone that app still logged two rejections per request, with this preview it logs none, the same as node. async-context-unhandled-rejection-then-passthrough.js covers the four shapes on bun and node. All four observe a null store on the previous preview.
Main moved WEBKIT_VERSION to aea1f010 (oven-sh/WebKit#330, the buffer accessor intrinsic that #35343 registers). oven-sh/WebKit#268 is rebased onto that commit, so its preview carries everything main's pin has plus the two async context fixes.
|
Rebased onto 7688227, which oven-sh/bun main pins now. Both commits applied cleanly. |
|
Preview builds for this branch cannot complete at the moment: the GitHub-hosted |
|
Rebased onto 0bb01ed, which oven-sh/bun main pins now. Both commits applied cleanly. |
1 similar comment
|
Rebased onto 0bb01ed, which oven-sh/bun main pins now. Both commits applied cleanly. |
|
Rebased onto f5deafe, which oven-sh/bun main pins now. Both commits applied cleanly. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
When a .finally() callback returns a thenable, the follow-up that propagates the original fulfilment (or rejects with the thenable's reason) runs as InternalMicrotask::PromiseFinallyAwaitJob. Unlike PromiseFinallyReactionJob, this case did not install the async context, and the phase-1 handler did not capture it on the reaction it schedules, so an unhandled rejection originating here observed an undefined AsyncLocalStorage store. Capture the active async context alongside the reaction at the two schedule points in promiseFinallyReactionJob (wrapWithCurrent), and install/restore it in the PromiseFinallyAwaitJob case (unwrapContextTuple + AsyncContextSwapScope), mirroring PromiseFinallyReactionJob. The synchronous promiseFinallyAwaitJob calls remain inside the phase-1 install window and are unaffected.
A reaction registered with then() captures the async context, but when the settled side has no handler the derived promise is settled by PromiseResolveWithoutHandlerJob, and every path that queues that job dropped the context: triggerPromiseReactions for a JSFullPromiseReaction with an undefined side, the already-settled arms of performPromiseThen and performPromiseThenWithContext, and the PromiseResolveThenableJobFast reaction that makes one promise adopt another. The derived promise then rejected with no async context installed, so the rejection tracker (and Bun's unhandledRejection event) saw none, while the same shape with a handler, or through the non-fast thenable job, kept it. Pass the reaction's context as the job's third argument and install it around the settle, the way PromiseReactionJob does. The thenable fast path installs the captured context before the species check too, so the slow path's performPromiseThen captures the same context.
|
oven-sh/bun#31721 no longer pins a preview of this PR; it builds on bun main's pin and keeps the two fixtures that need this change as todos. So this PR is no longer on the bun PR's critical path, but the |
1 participant
Problem
Two internal microtasks settle a promise without the async context of the call that set them up, so in Bun the rejection tracker (and with it
process.on("unhandledRejection"), oven-sh/bun#31721) sees noAsyncLocalStoragestore for them. Node reports the store in both cases.PromiseFinallyAwaitJob. When a.finally()callback returns a thenable, the follow-up that propagates the original settlement (or rejects with the thenable's reason) runs as this job. UnlikePromiseFinallyReactionJoba few lines above it, the case did not install the async context, and phase 1 did not capture it on the reaction it schedules.PromiseResolveWithoutHandlerJob. A reaction registered withthen()captures the async context, but when the settled side has no handler the derived promise is settled by this job, and every path that queues it dropped the context:triggerPromiseReactionsfor aJSFullPromiseReactionwith an undefined side, the already-settled arms ofperformPromiseThenandperformPromiseThenWithContext, and thePromiseResolveThenableJobFastreaction that makes one promise adopt another. The same shape with a handler, or through the non-fast thenable job, kept the context.This is what makes Next.js 16 log
NEXT_PRERENDER_INTERRUPTEDrejections under Bun that it suppresses under Node (oven-sh/bun#39847): itsunhandledRejectionfilter reads the prerender store, and the rejections it has to filter are exactly these handler-less derived promises.Fix
.finally()captures the active async context alongside the reaction at its two schedule points (performPromiseThenWithInternalMicrotaskandcreateResolvingFunctionsWithInternalMicrotask), and thePromiseFinallyAwaitJobcase installs and restores it, mirroringPromiseFinallyReactionJob. The synchronouspromiseFinallyAwaitJobcalls stay inside the phase 1 window and are unaffected.PromiseResolveWithoutHandlerJobpasses the reaction's context as the job's third argument (it wasjsUndefined()), and the case installs it around the settle withAsyncContextSwapScope, the wayPromiseReactionJobdoes.PromiseResolveThenableJobFastinstalls the captured context before the species check, so its slow path (performPromiseThen) captures the same context, and captures it on the adopting reaction withwrapWithCurrent.undefined, so the paths are unchanged in that case.Interpreter::getAsyncStackTracealready unwraps the tuple shape the reactions now carry.Context
The
AsyncFunctionResumesettle ordering fix that this PR originally carried landed separately as #295.Audit
Internal microtask cases that settle a promise while a Bun async context can be active:
PromiseReactionJobAsyncFunctionResumeAsyncGeneratorYieldAwaited,AsyncGeneratorBodyCallNormal/Return,AsyncGeneratorAwaitReturnContinuationPromiseResolveThenableJob,PromiseResolveThenableJobWithInternalMicrotaskPromiseFinallyReactionJob(phase 1)PromiseFinallyAwaitJob(phase 2)PromiseResolveWithoutHandlerJob, andPromiseResolveThenableJobFastwhich registers itsettleInlineHandler)Verification
Consumed by oven-sh/bun#31721. Two of its fixtures run against both Bun and Node and need this change:
async-context-unhandled-rejection-finally-thenable.js(case 1) andasync-context-unhandled-rejection-then-passthrough.js(four legs of case 2: pending source, settled source, adoption throughresolve(), adoption through an async function's return). Both fail on Bun without this change and pass with it. The other async shapes (throw infinally, async generators,awaitthen throw) pass with or without it. Measured with the Next.js app from oven-sh/bun#39847 (3 requests): release bun logs 6unhandledRejection, bun with only the bun-side change still logs 6, bun on this PR's preview (autobuild-preview-pr-268-33161987) logs 0, the same as node.