JSC: propagate async context through PromiseFinallyAwaitJob - #268
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
WalkthroughChangesPromise Promise finally async context
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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.
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 a few lines above it, 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 schedule points in promiseFinallyReactionJob, and install/restore it in the PromiseFinallyAwaitJob case, mirroring PromiseFinallyReactionJob. The synchronous promiseFinallyAwaitJob calls remain inside the phase-1 install window and are unaffected.
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.
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.
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.
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.
1 participant
Problem
When a
.finally()callback returns a thenable, the follow-up that propagates the original fulfilment (or rejects with the thenable's reason) runs asInternalMicrotask::PromiseFinallyAwaitJob. UnlikePromiseFinallyReactionJoba few lines above it, this case did not install the async context, and the phase-1 handler did not capture it on the reaction it schedules. So in Bun:Fix
Capture the active async context alongside the reaction at the two schedule points in
promiseFinallyReactionJob(performPromiseThenWithInternalMicrotaskandcreateResolvingFunctionsWithInternalMicrotask), and install/restore it in thePromiseFinallyAwaitJobcase, mirroringPromiseFinallyReactionJob. The synchronouspromiseFinallyAwaitJobcalls remain inside the phase-1 install window and are unaffected.Context
This was originally the second commit of this PR, alongside the
AsyncFunctionResumesettle-ordering fix. That half landed independently as #295, so this PR is rebased to carry only the remainingPromiseFinallyAwaitJobfix.Audit
Of the internal-microtask cases that can settle a promise with a Bun async context active:
PromiseReactionJobAsyncFunctionResumeAsyncGeneratorYieldAwaited,AsyncGeneratorBodyCallNormal/Return,AsyncGeneratorAwaitReturnContinuationPromiseFinallyReactionJob(phase 1)PromiseFinallyAwaitJob(phase 2)Verification
Consumed by oven-sh/bun#31721, whose
async-context/async-context-unhandled-rejection-async-fn.jsfixture runs the example above (and its throw-in-finally, async-generator, and await-throw siblings) against both Bun and Node.