error: give errors created with no JS frames a .stack property - #38074
error: give errors created with no JS frames a .stack property#38074robobun wants to merge 4 commits into
Conversation
JSC only defines .stack when the captured trace has at least one frame. Errors Bun builds while no JS is running (fs, dns, socket and fetch completions, ERR_* errors from event loop callbacks, thread pool jobs) capture none, so they had no .stack at all unless an async function happened to be awaiting the promise they were rejected with. Install the lazy .stack accessor that Error.captureStackTrace already uses on such errors, from the SystemError and error-code constructors and from the async stack attach when it recovers no frames. Reading it formats the usual "Name: message" line (through Error.prepareStackTrace when set), as V8 does for an error with no frames.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
Comment |
|
Reproduced on the released 1.4.0 binary and on main: errors Bun creates while no JS is running ( Fix is in this PR (#38074): every constructor native code builds an error through installs the lazy |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Those two are partial versions of this change rather than alternatives to it, and this PR is meant to replace both:
This PR covers all three sites through one shared helper. Both older PRs have been conflicting with main for a while; closing them in favor of this one. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; the earlier stdout-drain nit is now addressed. Because it changes how .stack is installed on every SystemError / ERR_* error / async-attach path via JSC ErrorInstance internals, a human look would still be worthwhile.
What was reviewed:
installLazyStackIfFrameless: null-vs-emptystackTrace()gating,getDirectidempotency check, and reuse of the existingm_lazyStackCustomGetterSetter— matches theError.captureStackTraceinstall path.uncheckedDowncast<ErrorInstance>insystemErrorToErrorInstance—JSC::createError(globalObject, errorType, ...)always returns anErrorInstance, so the unchecked cast is sound.Bun__attachAsyncStackFromPromiserestructuring — the frames-found path is unchanged (setStackFrames+ return); the fallback only runs whenlimit == 0or the walk found nothing.- New tests: hermetic (tempDir / ephemeral local port), restore
Error.stackTraceLimit/prepareStackTraceinfinally/afterEach, and the subprocess test now drains stdout.
Extended reasoning...
Overview
The PR adds a single helper, Bun::installLazyStackIfFrameless, in FormatStackTraceForJS.{cpp,h} and calls it from three native-error construction sites: systemErrorToErrorInstance (bindings.cpp), ErrorCodeCache::createError (ErrorCode.cpp), and the empty-frames fallback of Bun__attachAsyncStackFromPromise (AsyncStackTrace.cpp). The helper installs the pre-existing lazy stack CustomGetterSetter on an ErrorInstance when its captured frame vector is present-but-empty and no own stack exists yet. Four test files gain coverage; three pre-existing tests in promises.test.js are tightened from "string or undefined" to "starts with Error: message".
Security risks
None identified. The change only affects the shape of the .stack property on error objects (adding a header-line string where there was previously undefined). No untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. While the helper itself is small and reuses an existing accessor mechanism, it is wired into three very hot construction paths that produce essentially every native error in the runtime (fs, dns, fetch, sockets, redis, all ERR_* codes). It relies on subtle ErrorInstance internals — the distinction between a null m_stackTrace (deleted stackTraceLimit) and an empty one, and the interaction between the installed CustomAccessor and JSC's own materializeErrorInfoIfNeeded. The PR description explicitly notes this is a call-site workaround for behavior that ideally lives inside JSC's materialization; a maintainer should confirm that trade-off is acceptable.
Other factors
- My prior inline nit (undrained stdout in the
--unhandled-rejections=warnsubprocess test) was addressed in commit cc66d96; the test now drains all three of stdout/stderr/exited concurrently and asserts{ stdout: "", exitCode: 0 }. - The comment-cop bot flagged the new 2–4 line explanatory comments at each call site. Those read as concise "why" context (e.g. "Native code also builds these while no JS is running…"), not workaround justifications, but a maintainer may still want them trimmed.
- The bug-hunting system found nothing this run. I checked that the
AsyncStackTrace.cpprestructuring preserves the originalsetStackFramespath exactly when frames are recovered, that thegetDirectguard makes the constructor-time and attach-time calls compose without double-installing, and that theErrorCodeCache::createErrorcall site is only reached aftercreated_erroris confirmed non-null (the earlierif (!created_error)branch handles the exception case). - Test coverage is thorough (own/non-enumerable descriptor, lazy formatting,
prepareStackTracewith zero call sites, assignment before read,stackTraceLimit = 0vs. deleted, async-frame preservation, and the--unhandled-rejections=warnend-to-end), and existing weaker assertions were tightened rather than left permissive.
Given the breadth of affected error paths and the JSC-internals dependency, I'm deferring rather than approving.
|
Updated 5:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 442477c has some failures in 🧪 To try this PR locally: bunx bun-pr 38074That installs a local version of the PR into your bun-38074 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the earlier stdout-draining nit was addressed in cc66d96). Since it changes error-construction behavior across three native JSC binding sites and interacts with ErrorInstance materialization state, a human look would still be worthwhile.
What was reviewed:
installLazyStackIfFrameless: null-vs-empty trace guard,getDirectidempotence check, and reuse of the existingm_lazyStackCustomGetterSetteraccessor.- The three call sites:
uncheckedDowncast<ErrorInstance>insystemErrorToErrorInstanceis safe (JSC'screateError(_, ErrorType, _)always returns anErrorInstance);ErrorCodeCache::createErrorpasses theErrorInstance*directly; the attach-site fallback preserves the existingsetStackFramespath when frames are recovered. - Tests: cover callback delivery,
.then(),await,stackTraceLimit=0/ deleted,prepareStackTrace, and assignment; the tightenedpromises.test.jsassertions no longer acceptundefined.
Extended reasoning...
Overview
This PR adds Bun::installLazyStackIfFrameless in FormatStackTraceForJS.cpp and calls it from three native error-construction sites (systemErrorToErrorInstance in bindings.cpp, ErrorCodeCache::createError in ErrorCode.cpp, and the empty-frames fallback in Bun__attachAsyncStackFromPromise). When an ErrorInstance was created with an empty (but non-null) frame vector and has no own .stack yet, it installs the same lazy CustomGetterSetter that Error.captureStackTrace already uses, so .stack reads as "Name: message" and honors Error.prepareStackTrace. Four test files gain coverage for callback-delivered errors, .then() rejections, stackTraceLimit interactions, and the --unhandled-rejections=warn output.
Security risks
None identified. The change reads ErrorInstance::stackTrace() and does a getDirect / putDirectCustomAccessor on a freshly-constructed error; no user-controlled input reaches new parsing or allocation paths.
Level of scrutiny
This is native C++ in the JSC bindings layer, on paths every SystemError and every ERR_*-coded error passes through. It interacts with ErrorInstance's lazy-materialization state (m_stackTrace, hasMaterializedErrorInfo, the lazy-stack accessor) — an area with a history of subtle GC / desync bugs (see the neighboring captureStackTrace branch that avoids setStackFrames after materialization). While the helper itself is small and reuses an existing accessor, the blast radius (every native error) and the subtlety of the invariants argue for a maintainer's review rather than an auto-approve.
Other factors
- The change is well-tested: nine new tests in
capture-stack-trace.test.jscovering ownership/enumerability, lazy formatting,prepareStackTrace, assignment, the async-frame path being unchanged,stackTraceLimit=0, deletedstackTraceLimit, and the unhandled-rejection warning; three tightened assertions inpromises.test.js; and per-constructor coverage inpassword.test.tsandconnection-failures.test.ts. The PR description confirmsUSE_SYSTEM_BUN=1failure /bun bdpass for each. - I checked that
uncheckedDowncast<ErrorInstance>insystemErrorToErrorInstanceis safe: JSC'screateError(JSGlobalObject*, ErrorType, const String&)dispatches to constructors that always returnErrorInstances. - The remaining comment-cop bot flags on
FormatStackTraceForJS.{cpp,h}point at two-line comments that state non-obvious invariants (null vs. empty trace, V8's deleted-stackTraceLimitbehavior); after the trim in f904787 they read as appropriate, not paragraph-length justifications. - My earlier inline nit about draining stdout in the
--unhandled-rejections=warnsubprocess test was addressed in cc66d96 and the thread is resolved.
Hook the remaining constructors native code builds errors through (BunString/ZigString error family, AggregateError, S3Error, the SystemError info-object variant, node:crypto job errors, the WebSocket error event) instead of three hand-picked sites, and make the async stack attach leave errors with a null trace alone (Error.stackTraceLimit deleted, or a stack string already present), matching what the helper does. Bun.build now rejects through the async stack attach like the other native promise APIs.
There was a problem hiding this comment.
I reviewed this PR (including the expanded 442477c commit) and didn't find any bugs. Because it changes error construction across ~13 native constructor paths in core JSC bindings — and 442477c grew the scope well beyond the three sites described in the PR body — a human look at the overall approach would be worthwhile.
Checked: installLazyStackIfFrameless is idempotent (own-stack guard) so the constructor-site + attach-site calls compose; the AsyncStackTrace.cpp restructure preserves the frames-found path exactly and only falls through when limit == 0 or no frames were recovered; the JSValue overload's dynamicDowncast makes the createDOMException wrap a no-op when the result isn't an ErrorInstance; ordering in S3Error.cpp (accessor installed before .name is set) is fine because the getter reads name lazily.
The Windows bake/deinitialization.test.ts segfault in CI is in a file this PR doesn't touch.
Extended reasoning...
Overview
Adds Bun::installLazyStackIfFrameless (two overloads) in FormatStackTraceForJS.{cpp,h} and threads it through native error construction so errors built with no JS on the stack get an own, non-enumerable, lazily-formatted .stack (matching V8's "Name: message" header behavior). The latest commit (442477c) expanded the original three call sites to every native error constructor reachable from bindings.cpp (ZigString__to{,Type,Syntax,Range}ErrorInstance, ZigString__toDOMExceptionInstance, JSC__create{,Type,Range}Error, both AggregateError helpers, both SystemError paths), plus S3Error, WebSocket error events, and createCryptoError. It also restructures Bun__attachAsyncStackFromPromise to fall through to the helper when no async frames are recovered, and switches Bun.build()'s failure reject to reject_with_async_stack. Tests are added/tightened across seven files.
Security risks
None identified. No parsing of untrusted input, no auth/crypto logic changes (the CryptoUtil.cpp touch only adds the accessor to an already-constructed error object). getDirect and putDirectCustomAccessor don't run user JS, so no reentrancy on the install path.
Level of scrutiny
High. This is not a mechanical change: it touches the construction path of essentially every native error object in the runtime, sits in hand-written JSC binding C++, and the most recent commit roughly quadrupled the number of touched call sites relative to the PR description. The helper itself is small and well-guarded (null trace → skip; non-empty trace → skip; existing own stack → skip), and the lazy getter it installs is the same one Error.captureStackTrace already uses, so the mechanism is proven — but the breadth of application and the design choice (patch every construction site vs. fix JSC's materializeErrorInfoIfNeeded) warrant a maintainer's sign-off.
Other factors
- My earlier inline nit (undrained stdout pipe) was addressed in cc66d96.
- The two unresolved comment-cop threads on
FormatStackTraceForJS.{cpp,h}appear to be bot noise — the flagged comments are already one-liners after f904787. - Test coverage is thorough: property descriptor shape, lazy formatting,
prepareStackTracewith zero call sites,stackTraceLimit = 0vs deleted, the--unhandled-rejections=warnend-to-end, and per-constructor coverage (fs callback, fetch,Bun.buildAggregateError,Bun.password, redis, crypto sign job, WebSocket error event). - CI shows one failure (
test/bake/deinitialization.test.tssegfault on Windows x64) at f904787; that test is unrelated to anything this PR touches, and there's a newer commit (442477c) whose CI status isn't in the timeline yet.
1 participant
Problem
stackproperty at all:typeof err.stack === "undefined",Object.hasOwn(err, "stack") === false. Node always gives an error a.stackstring, at minimum theName: messageline. Code doingerr.stack.split("\n")throws, and--unhandled-rejections=warnprintsUnhandledPromiseRejectionWarning: [object Object]for these rejections because node's error-like check is "has an ownstack".fscallback andfs.promiseserror,Bun.file().text(),Bun.connect()(the rejection and theconnectErrorargument),fetch()network failures,node:dns,RedisClientconnection errors,Bun.password.verify(), theAggregateErrorfromBun.build()/Bun.Transpiler, theWebSocketerror event's.error,node:cryptojob errors passed to callbacks. The one exception was an errorawaited directly inside an async function, which got a string because anat async fframe was attached.ErrorInstance::materializeErrorInfoIfNeeded(vendoredErrorInstance.cpp:410) only defines.stackwhen the captured frame vector is non-empty. An error created from an event loop callback captures an empty vector, so nothing ever defines the property.Bun__attachAsyncStackFromPromise(src/jsc/bindings/AsyncStackTrace.cpp) fills the vector when an async function is awaiting the promise, but returned without touching the error for.then()/.catch(), combinators, top-level await, and callback or event delivery.Fix
Bun::installLazyStackIfFrameless(FormatStackTraceForJS.cpp): if an error's frame vector exists but is empty and it has no ownstackyet, install the lazystackaccessorError.captureStackTracealready uses. The first read formats the empty vector the normal way (soError.prepareStackTraceruns, with an empty call-site array) and replaces the accessor with a non-enumerable data property holdingName: message.ErrorInstancethrough:systemErrorToErrorInstanceandSystemError__toErrorInstanceWithInfoObject,ErrorCodeCache::createError(allERR_*errors), theZigString__to*ErrorInstance/JSC__create*Errorstring-to-error family (andZigString__toDOMExceptionInstance, for the codes that yield a real Error), bothcreateAggregateErrorbindings,S3Error__toErrorInstance,createCryptoError(node:crypto jobs), andWebSocket.cpp's error event. Constructors called during JS execution see a non-empty vector and return after one check, so synchronous errors are untouched.createOutOfMemoryErroris deliberately left out.Bun__attachAsyncStackFromPromisealso calls it when the await walk recovers nothing (covers any constructor not listed above). It now returns early for a null vector too, which is what the helper does: a null vector means the error was created withError.stackTraceLimitdeleted (V8 leaves.stackundefined in that case) or already holds a stack string; previously it attached frames to those, so a deleted limit behaved differently depending on whether something awaited the promise.Bun.build()rejects throughreject_with_async_stacklike the other native promise APIs (js_bundle_completion_task.rs), so an awaited failure lists the awaiting function..stack; V8 also exposes it as an accessor until first read; formatted lazily, so latername/messagechanges andError.prepareStackTraceare honored;Error.stackTraceLimit = 0yields the header line, a deletedError.stackTraceLimityields undefined, in both the.then()andawaitconsumption modes (test matrix below).console.log/ uncaught printing for them is byte-identical because the printer reads the frame vector, not the property..stackalone.USE(BUN_JSC_ADDITIONS)block ofErrorInstance::materializeErrorInfoIfNeeded(fn && m_stackTrace && !m_stackTrace->isEmpty()->fn && m_stackTrace; Bun's hook already formats an empty vector). That needs an oven-sh/WebKit change plus a pin bump, and its fail-before cannot be demonstrated from this repo alone, so this PR does it at Bun's constructors; once the engine change lands, the helper and its call sites can be deleted. Until then the only remaining gap isError.stackTraceLimit = 0; new Error(), which is constructed inside JSC.USE_SYSTEM_BUN=1and passes withbun bd test; the two tests below that pin unchanged behavior are marked):test/js/node/v8/capture-stack-trace.test.js, newdescribe: fs callback error (callback delivery): own + non-enumerable, lazy formatting,prepareStackTracewith zero call sites, assignment;fetch()via.then()gets the header, viaawaitstill gets the async frame (unchanged behavior);Bun.build()via.then()and viaawait;Error.stackTraceLimit = 0for a SystemError and anERR_*error; deleted limit on a sync error (unchanged behavior); the{10, 0, deleted} x {.then(), await}matrix onBun.password.verify()(plain constructor + attach);--unhandled-rejections=warnprints the error.test/js/node/fs/promises.test.js: the Promise-subclass / thenable /Promise.alltests used to acceptundefined, now require the header; new.catch()test.test/js/valkey/reliability/connection-failures.test.ts:ERR_REDIS_CONNECTION_CLOSEDvia the error-code constructor (plain reject, no attach); needs no server.test/js/web/websocket/error-event.test.ts:event.errorof a refused connection.test/js/node/crypto/crypto-sign-regression.test.ts:crypto.sign()callback error from a failing job.inspect-error.test.jshas two failures in a debug build that reproduce without this change.Background
ErrorInstancestores the frames captured at construction inm_stackTraceand only defines thestack/line/columnproperties on the first access to one of them (materializeErrorInfoIfNeeded), formatting through Bun's hook. With zero frames that code is skipped entirely, which is the bug.getStackTracereturns an empty vector when there was nothing to capture and a null one whenError.stackTraceLimitis deleted or not a number; errors rebuilt bystructuredClonealso have a null vector and carry a stack string instead.errorInstanceLazyStackCustomGetter(FormatStackTraceForJS.cpp) is aCustomGetterSetterBun installs as an ownstackproperty; reading it formats whatever frames the error holds at that moment andputDirects the result over itself. Until now onlyError.captureStackTraceinstalled it.Bun__attachAsyncStackFromPromise, which walks the promise's reaction chain looking for async functions awaiting it and stores those asat async fframes. Consumers that are not a directawait(.then(),Promise.all, top-level await, callbacks) give it nothing to find.code/syscall/path/errno);systemErrorToErrorInstanceturns it into a JS Error.ERR_*errors are node-style coded errors, all built byErrorCodeCache::createError. Message-only errors from Rust go through theZigString__to*ErrorInstance/JSC__create*Errorbindings.Probe: native errors with and without .stack (main before / after)
typeof e.stackbefore, on main: fs.promisesawait/.then()/.catch(),fs.readFile/open/statcallbacks,createReadStreamerror event,Bun.file().text()/.arrayBuffer(),Bun.connect(await,.then(),connectErrorargument, rejection afterconnectError),fetch(await,.catch()),dns.lookupcallback,dns.promises.lookup,dns.resolve4callback,RedisClient,Bun.password.verifyvia.then(),Bun.build(.then()and awaited),Bun.Transpiler.transform,WebSocketerror event,crypto.signcallback: allundefined. zlib callbacks,Bun.spawnENOENT,Bun.SQL,Bun.file().stream(), abortedfs.promises.readFile:string(created with JS on the stack).After: every row above is a
string; the awaitedBun.buildrow additionally has theat asyncframe.Bun.password.verify()rejection,.stackbyError.stackTraceLimitand consumer:.then()beforeawaitbefore.then()afterawaitafterEarlier revision
The first revision of this PR called the helper from three sites only (SystemError constructor,
ErrorCodeCache::createError, the attach fallback) and describedBun.build's AggregateError and the WebSocket error event as needing an engine change; both are built by Bun's own C++, so review of that revision led to hooking every constructor, the null-vector consistency fix in the attach, and theBun.build/WebSocket/crypto/ matrix tests. TheBun.passwordtests moved frompassword.test.tsinto the matrix.