Conversation
…ection A native rejection gets its async stack from a walk over the pending reactions of the rejected promise. The walk knew only the reaction layouts of JSC's promise fast paths. Off those paths JSC calls then(resolve, reject) with a resolving-function pair, and the walk followed the promise that then() returned, which nothing awaits. JSC leaves the fast paths for the rest of the realm once `then` is defined on Object.prototype, Promise.prototype.then is replaced, or Promise.prototype is frozen. Every fs.promises error then had no stack, because each fs.promises function is an async function that returns the native promise. The walk now continues at the promise a reject function rejects, or at the await it resumes, and falls back to the promise then() returned when nothing awaits that target. It also reads the promise out of the capability record that then() stores once the species watchpoint has fired.
|
Status: ready for review. How to reproduce. A release build is enough. import { readFile } from "node:fs/promises";
async function level1() { await readFile("/nonexistent-path/x"); }
async function probe(label) { try { await level1(); } catch (e) { console.log(label, e.stack); } }
await probe("before"); // Error: ENOENT ... at async level1 ... at async probe
Object.defineProperty(Object.prototype, "then", { configurable: true, get() {} });
delete Object.prototype.then;
await probe("after"); // undefined on 1.4.3-canary, the same frames on this branchThe same state in the test suite, two files in one process: 1.4.3-canary: 79 pass, 1 fail ( |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughNative Promise rejection tracing now resolves JSC rejection targets, follows reaction and derived-promise paths, prevents cyclic generator traversal, and adds coverage for multiple promise patterns and runtime contexts. ChangesAsync rejection tracing
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The native rejection tracing change has targeted coverage for its shared behavior and no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/bindings/AsyncStackTrace.cpp— A program that feeds an async function's own outcome back into a race it awaits gets a stack of ten identicalat asyncframes instead of one, with no deadlock required. The outer loop at AsyncStackTrace.cpp:233-238 has no visited set, and the new reject-function following at line 72-75 lets getAwaitingGenerator(returnPromise) land back on the same generator. Fix: stop the outer loop when getAwaitingGenerator returns a generator already appended (track visited generators or return promises), so a cycle yields each frame once.Extended reasoning...
The dismissal said this needs a deadlock-shaped program. It does not. Off the fast path, Promise.race registers the race promise's first-resolving reject function on each input, and rejectionTargetOf at line 72-75 follows it while the race promise is Pending. Program:
const { promise, resolve, reject } = Promise.withResolvers(); async function A() { await Promise.race([Bun.file(missing).text(), promise]); } A().then(resolve, reject);. This completes normally: N rejects, the race rejects, A throws, A's return promise rejects, reject settlespromise. Walk: N's reaction holds the race reject function. Line 74 sees the race promise Pending with the first-resolving flag clear. The race promise is awaited by A, so line 232 finds A and line 234 appends the frame. Line 236 takes A's return promise RA. RA's reaction isthen(resolve, reject); rejectHandlerOf at line 82-83 returns reject. Line 72-75 givespromise.promiseis an input of the race, so its reaction again holds the race reject function, which again leads to the race promise and to A. Line 233 only stops at results.size() <…Verification: nit — triggered when an async function's return promise is wired (via
then(resolve, reject)/catch(reject)with a resolving-function pair) into a promise that the same function is racing against, e.g. a fail-fast worker poolworker().catch(abort)whereworkerdoesawait Promise.race([nativeOp, abortPromise]). Mechanism, traced in /home/claude/bun/src/jsc/bindings/AsyncStackTrace.cpp:…
…the fixture The search shared one 32-hop budget between the walk along a reject function and the fallback walk, so a long chain behind the reject function could starve the promise that then() returned. Each walk now has its own limit, the number of walks is limited, and a search that the limit ends falls back to one walk without reject functions. A chain that leads back to a generator that is already in the stack is a cycle. The search now treats it as a dead end, so a fail-fast worker that races a native promise against its own abort promise gets one frame and not ten. On Windows a Bun.file().text() that rejects does not keep the process alive (#39787), so the fixture printed nothing there. It now uses Bun.file().stat().
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 12:32 PM PT - Sep 17th, 2026
✅ @robobun, your commit af78411988ff3095de4a1e01b62a19a3aaf02209 passed in 🧪 To try this PR locally: bunx bun-pr 43127That installs a local version of the PR into your bun-43127 --bun |
|
The review finding about repeated frames is confirmed and fixed in 75afa6d. Shape: The search now treats a generator that it already visited as a dead end and tries the fallbacks. The result is the same as on main: one |
There was a problem hiding this comment.
I reviewed the updated PR and didn't find any bugs; the per-walk hop budget and cycle guard added in the second commit address the shared-budget concern from my earlier pass. Because the new walk depends on JSC-internal layouts (JSFunctionWithFields resolving-function fields, JSSlimPromiseReaction::handlerOrContext, the capability record's promise slot) that cannot be checked against the vendored WebKit source from this checkout, a human familiar with the JSC promise implementation should still take a look.
What was reviewed:
- Allocation under
AssertNoGC:WTF::HashSet/WTF::Vectoruse fastMalloc,getConcurrently+getDirect(offset)avoid property-table materialization, and an accessor in thepromiseslot yields aGetterSettercell that fails the downcast rather than running user code. - Behavior preservation: the
FulfillHandlerarm is unchanged and the finalwalkReactions(start, nullptr)pass reduces to the base branch's chain-following, so no shape loses frames when reject-function targets do not pan out. - Termination: 32 hops per walk, at most 8 walks plus one plain walk, and
seenstops the outer loop on generator cycles; the fixture covers the 40-link chain, 10 chained reject targets, and theworker().catch(abort)cycle.
Extended reasoning...
Overview
The PR extends collectAsyncStackFramesFromPromise in /home/claude/bun/src/jsc/bindings/AsyncStackTrace.cpp so that a native rejection still gets at async frames after JSC has permanently left its promise fast paths (Object.prototype.then defined, Promise.prototype.then replaced, or Promise.prototype frozen). Three new lambdas recognize JSC's own native reject functions stored as reaction handlers and follow them to the promise or await record they would settle; derivedPromiseOf additionally reads the then() result out of a capability record. The single-pass walk became a bounded multi-walk search with fallbacks and a seen set. Tests add a spawned fixture (/home/claude/bun/test/js/bun/util/native-rejection-async-stack-fixture.js) exercising 14 consumption shapes across three triggers, each before/after and with/without AsyncLocalStorage, asserted exactly via toEqual in /home/claude/bun/test/js/bun/util/bun-file.test.ts.
Security risks
None specific: the code only reads engine-internal cells and never invokes user code. I checked the one place user-controlled objects are inspected (derivedPromiseOf reading a promise own property): it uses structure()->getConcurrently and getDirect(offset), so a getter placed there is returned as a GetterSetter cell and fails the JSPromise downcast instead of executing. All field reads are gated on dynamicDowncast.
Level of scrutiny
Moderate-to-high. The change runs under JSC::AssertNoGC, keeps raw JSPromise*/JSAsyncFunctionGenerator* in WTF containers (acceptable only because no JS-heap allocation occurs in the walk, which holds for fastMalloc-backed WTF::Vector/HashSet), and hard-codes assumptions about JSC internals: that calling either resolving function clears ResolvingOther/ResolvingWithInternalMicrotaskOther, that FirstResolvingPromise pairs with isFirstResolvingFunctionCalledFlag, and that the capability record stores its promise as an own data property named promise. This checkout has no vendored WebKit tree, so these could not be verified against source here; if any assumption is wrong the failure mode is benign (the walk hits a non-pending promise or a failed downcast and falls back), but a maintainer who knows the JSC promise rewrite should confirm them. There is no CODEOWNERS entry covering src/jsc/bindings.
Other factors
The earlier inline finding (shared 32-hop budget starving the fallback walk) is addressed: each walkReactions call now has its own hop counter, the driver caps walks at 8, and a final reject-function-free walk preserves the base behavior. The FulfillHandler arm is byte-for-byte the old logic, and the heap-reaction path still only inspects the head of the reaction list, as before. The bug-hunting run exited on a dry streak with no findings. The test asserts exact frame lists (not toContain), drains stdout/stderr concurrently, uses test.concurrent.each, and spawns a fresh process per trigger since the watchpoints cannot be reset; the fixture's main() has no catch, so any thrown error surfaces as non-empty stderr and a nonzero exit, which the assertions catch.
|
For the reviewer who checks the JSC layouts: these are the places in the pinned WebKit (
Each read is behind a checked downcast. If a layout changes, the walk finds nothing there and uses the plain |
This branch has not been deployed
1 participant
Problem
await fs.promises.readFile(missing)rejects withe.stack === undefined. Node prints every frame.thendefined onObject.prototype,Promise.prototype.thenreplaced,Promise.prototypefrozen (SESlockdown()).inner.then(resolve, reject)to resolve a promise with another promise. The walk insrc/jsc/bindings/AsyncStackTrace.cppfollows the promise thatthen()returned. Nothing awaits it.Fix
await. The walk continues there. If nothing awaits there, it uses the promise thatthen()returned.then()keeps that promise in a capability record, which the walk now reads.CaptureAsyncStackTracefollowsPromiseCapabilityDefaultResolvethe same way.test/js/bun/util/bun-file.test.ts(3 new tests, all fail on 1.4.3-canary). Also thefs, streams and stack-trace suites. Self-reviewed: 12 concerns raised, 12 addressed.Background
Bun.file, streams) has no JS call stack.Bun__attachAsyncStackFromPromisebuilds one from the async functions that await the rejected promise. It finds them through the pending reactions.fs.promisesfunction is an async function that returns the native promise. On the fast path one internal reaction links the two promises. The fast paths depend on two realm watchpoints, which never reset once fired.resolveorrejectof one promise. An internal field holds its promise or itsawaitcontext.Notes
How it was found. There is no user report.
test/js/web/streams/streams.test.jshas a test that defines and deletesObject.prototype.then. Run by hand in one process beforetest/js/node/fs/promises.test.js, it makeserrors from fs.promises include async stack framesfail (-t "multi-chunk|errors from fs.promises include async stack frames": 79 pass, 1 fail on 1.4.3-canary, 80 pass here). CI runs one file per process, so CI cannot see it.The test. The fixture runs 14 ways to consume a native rejection, before and after the trigger, each with and without
AsyncLocalStorage.run(). It runs in a child process because the trigger cannot be undone. The native promise isBun.file(missing).stat()and nottext(): on Windows atext()that rejects does not keep the process alive (#39787), so the fixture printed nothing there. Shapes with no frames on 1.4.3-canary:at asyncframepromiseSubclass,forwardingThenable,thenResolveReject,catchRejectObject.prototype.thenor a replacedPromise.prototype.thenfsPromises,asyncFunctionReturn,resolveWithPromise,race,failFastWorkerObject.freeze(Promise.prototype)On this branch every shape has its frames in every phase, on Linux and on Windows x64. SES
lockdown()was checked by hand:e.stack === undefinedon canary, full frames here.Frames that are new. The first row never had frames.
awaitof a Promise subclass or of a thenable that forwardsthen(onFulfilled, onRejected)to a native promise uses the third kind of resolving function (promiseResolvingFunctionRejectWithInternalMicrotask), which holds theawaitcontext.nativePromise.then(resolve, reject)and.catch(reject)with the functions fromPromise.withResolvers()use the same pair kind asPromise.race.No frame is lost. A first revision followed the resolving function only. It lost
await nativePromise.catch(reject)when only a callback consumes the deferred promise. The fallback keeps it (thenResolveRejectResult,catchRejectResult). Each walk has its own limit of 32 hops, so a long chain behind the reject function cannot use up the hops of the fallback (catchRejectResultLongChain). A search has at most 8 walks. If that limit ends a search, one more walk runs without reject functions, which is the search as it is on main (catchRejectResultManyTargets). A probe over 33 shapes, 7 realm states, with and withoutAsyncLocalStorage(462 cells), compared with 1.4.3-canary: 244 cells are equal, the others gain frames, none loses one.Cycles.
worker().catch(abort), whereworker()awaitsPromise.race([nativePromise, aborted]), is a cycle: the return promise ofworker()leads toaborted, andabortedleads back to the race thatworker()awaits. The outer loop had no guard for that. With the reject function followed, the stack wasat async workerten times. The search now treats a generator that it already visited as a dead end and tries the fallbacks, so the result is the same as on main: oneworkerframe, then the caller (failFastWorker).Still without frames.
thenpatch that wraps the callbacks (zone.js, async-listener, continuation-local-storage): the reaction holds the wrapper, not the resolving function. Checked: same result on canary and here.return awaitin thefs.promiseswrappers would coverfs.promisesfor these. That is a separate change.new Promise((resolve, reject) => nativePromise.then(resolve, reject)): the executor functions are JS closures.Promise.all,anyandallSettledon the fast path. Off the fast path JSC registers the reject function of thePromise.allpromise, so the walk follows it there.Interpreter::getAsyncStackTrace, not from this walk.reject_with_async_stack.No allocation. The walk runs under
AssertNoGC.JSObject::getDirect(vm, name)can materialize a property table, so the capability lookup usesStructure::getConcurrently. The fixture also passes withBUN_JSC_collectContinuously=1and withBUN_JSC_slowPathAllocsBetweenGCs=20.Other open PRs on this file. #43014 (
finally()) edits the same loop. This diff stays off the lines it touches. The two merge cleanly in either order, and the merged tree passes the tests of both. #38074 and #35685 also merge cleanly. The new test is inbun-file.test.tsand not inpromises.test.jsfor the same reason.Review. Two findings of the automated review are fixed in the second commit: the shared hop budget and the cycle above.
Self-review. The review asked for: the fallback above, the third resolving-function kind, a species-watchpoint trigger in the test, a narrower statement of who benefits, and no conflict with #43014. All are in. One is done differently than proposed: the review asked to stack this on #43014. The diff avoids its lines instead, so neither PR has to wait for the other.
[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file