SystemError: give async fs/dns/etc. errors a .stack string when there are no JS frames

[robobun]

What

Async node:fs errors (callback form and fs.promises consumed via .then()/.catch()) had err.stack === undefined. Every sync form of the same failing call had a normal stack, and Node gives every fs error a .stack string. Loggers that print ${err.stack} or console.error(err.stack) output the literal "undefined" for the most common error object in a Node app.

import fs from "node:fs";
const P = "/tmp/__nope_dir/deep/f";
try { fs.openSync(P, "r"); } catch (e) { console.log("openSync        ", typeof e.stack); }
await new Promise(r => fs.open(P, "r", e => { console.log("open(cb)        ", typeof e.stack); r(); }));
await new Promise(r => fs.readFile(P, e => { console.log("readFile(cb)    ", typeof e.stack); r(); }));
for (const m of ["readFile", "stat", "readdir", "unlink"]) {
  try { await fs.promises[m](P); } catch (e) { console.log(("promises." + m).padEnd(16), typeof e.stack); }
}
// node: all "string"    bun: sync "string", every async row "undefined"

Why

SystemError__toErrorInstance constructs the error from native code at the top of the event loop (the threadpool completion callback), where there are no JS frames on the stack. createError() captures an empty m_stackTrace, and ErrorInstance::materializeErrorInfoIfNeeded never installs a .stack own property when the trace is empty, so reading err.stack falls through to undefined.

Bun__attachAsyncStackFromPromise tries to recover async frames from the promise's await chain, but the callback-form wrappers (fs.open(path, cb) in src/js/node/fs.ts) attach the user's callback via .then(), which yields no JSAsyncFunctionGenerator to walk, so it bails with zero frames too.

Fix

In SystemError__toErrorInstance, when the freshly-created ErrorInstance's stack trace is empty, install the existing m_lazyStackCustomGetterSetter on .stack. That getter (errorInstanceLazyStackCustomGetter) formats whatever stackTrace() holds at access time:

• zero frames: "Error: <message>" (matches Node's header-only form for async fs errors)
• frames later attached by Bun__attachAsyncStackFromPromise (await case): full async trace
• Error.prepareStackTrace is called with an empty call-sites array, matching Node

The sync path is unaffected (always has JS frames; the new branch is not taken). Same for any other SystemError created off the event loop with no JS on the stack (dns, sockets).

Verification

before / after

Before:

open(cb) own props: [ "message", "code", "path", "syscall", "errno" ]
open(cb) .stack: undefined

After:

open(cb) own props: [ "message", "stack", "code", "path", "syscall", "errno" ]
open(cb) .stack: "Error: ENOENT: no such file or directory, open '/tmp/__nope_dir/deep/f'"

Node:

open(cb) own props: [ 'stack', 'message', 'errno', 'code', 'syscall', 'path' ]
open(cb) .stack: "Error: ENOENT: no such file or directory, open '/tmp/__nope_dir/deep/f'"

New tests in test/js/node/fs/fs.test.ts cover the callback form for 9 fs functions, fs.promises.* via both await and .then(), createReadStream's 'error' event, and Error.prepareStackTrace interaction. All fail on the released binary and pass with this change.

The underlying materializeErrorInfoIfNeeded behavior (empty trace => no .stack) lives in JavaScriptCore and also affects Error.stackTraceLimit = 0; new Error("x").stack; that is a separate WebKit-side change and is not addressed here.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3f6bf63d-9660-4c41-9ef2-f569ff9efeee

📥 Commits

Reviewing files that changed from the base of the PR and between ae4b17d and 32d79c6.

📒 Files selected for processing (2)

• src/jsc/bindings/bindings.cpp
• test/js/node/fs/fs.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 1:40 AM PT - Jul 25th, 2026}

❌ @robobun, your commit 32d79c6 has some failures in Build #80117 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 35515

That installs a local version of the PR into your bun-35515 executable, so you can run:

bun-35515 --bun

[claude]

I didn't find any bugs, but this touches JSC error/stack materialization internals and makes a layering choice (patching SystemError__toErrorInstance rather than the WebKit-side materializeErrorInfoIfNeeded root the description names), so a maintainer familiar with the stack-trace machinery should sign off.

What was reviewed:

• The new putDirectCustomAccessor call matches the existing pattern in FormatStackTraceForJS.cpp:806 (Error.captureStackTrace) — same accessor, same attributes.
• Interaction with Bun__attachAsyncStackFromPromise: installing the custom accessor does not set hasMaterializedErrorInfo, so later-attached async frames still land and the lazy getter picks them up on first read.
• Tests: subprocess isolation for the prepareStackTrace case, pipes drained concurrently, stderr/stdout asserted before exit code.

Extended reasoning...

Overview

The PR adds ~15 lines to SystemError__toErrorInstance in src/jsc/bindings/bindings.cpp: after createError() returns, if the resulting ErrorInstance has a null or empty stackTrace(), it installs the existing m_lazyStackCustomGetterSetter on .stack. This ensures async fs/dns/socket errors created from native code with no JS frames on the stack expose err.stack as a string (header-only or with later-attached async frames) instead of undefined. ~80 lines of new tests in test/js/node/fs/fs.test.ts cover 9 callback-form fs functions, fs.promises via await and via .then(), createReadStream's 'error' event, and Error.prepareStackTrace.

Security risks

None. This only affects the shape of the .stack property on error objects; no new inputs are parsed and no privileged operations are gated.

Level of scrutiny

Medium-high. The C++ change is small and reuses an established pattern verbatim (identical putDirectCustomAccessor call to errorConstructorFuncCaptureStackTrace at FormatStackTraceForJS.cpp:806). But it lives in the intersection of JSC's error-info materialization state machine, custom accessors, and Bun__attachAsyncStackFromPromise — an area where ordering (materialize vs. setStackFrames vs. lazy-getter) has caused ASSERTs before (see the comment at FormatStackTraceForJS.cpp:783-786). I traced the interaction: putDirectCustomAccessor does not flip m_errorInfoMaterialized, so Bun__attachAsyncStackFromPromise's guard at AsyncStackTrace.cpp:163 still passes and later-attached frames still work; the getter then reads whatever stackTrace() holds at access time. That looks correct, but a maintainer who owns this code should confirm.

Other factors

• Layering: the PR description explicitly notes the root cause is in JSC's materializeErrorInfoIfNeeded (which also affects Error.stackTraceLimit = 0; new Error().stack) and defers that to a separate WebKit change. Patching only SystemError__toErrorInstance is a deliberate scoping choice; per REVIEW.md's "fix at the layer that owns the invariant" guidance, a human should agree that this narrower fix is the right call for now.
• Whole-class coverage: only SystemError__toErrorInstance is patched. Other native error-creation paths that can run with no JS frames (e.g. the ErrorCode machinery) may exhibit the same undefined stack — a maintainer can decide whether those belong in this PR.
• Tests follow harness conventions (bunEnv, bunExe(), concurrent pipe drain, subprocess isolation for the global prepareStackTrace mutation) and assert specific error codes plus the header regex rather than just truthiness.

[claude]

I didn't find any issues, but this touches native JSC error-instance construction in SystemError__toErrorInstance, which every async fs/dns/socket error flows through — worth a quick human glance before landing.

What was reviewed:

• The new branch reuses m_lazyStackCustomGetterSetter exactly as errorConstructorFuncCaptureStackTrace already does (FormatStackTraceForJS.cpp:806); no exception check needed since putDirectCustomAccessor and stackTrace() are non-throwing.
• Confirmed the getter handles stackTrace() == nullptr and empty vectors, and self-replaces with a data property on first read, so repeated .stack access and later setStackFrames from Bun__attachAsyncStackFromPromise both work.
• Tests cover the variant matrix (9 callback fns, promises via await and .then(), stream 'error', prepareStackTrace in an isolated subprocess) and follow harness conventions.

Extended reasoning...

Overview

This PR fixes err.stack === undefined on async node:fs (and by extension dns/socket) errors created from native code when no JS frames are on the stack. It adds an 8-line branch to SystemError__toErrorInstance in src/jsc/bindings/bindings.cpp that installs the existing m_lazyStackCustomGetterSetter on .stack when the freshly-created ErrorInstance has an empty/null stack trace, plus ~80 lines of tests in test/js/node/fs/fs.test.ts.

Security risks

None. This is error-message formatting; no untrusted-input parsing, auth, or crypto is involved.

Level of scrutiny

Moderate. The C++ change is tiny and directly copies an established pattern — the identical putDirectCustomAccessor(... m_lazyStackCustomGetterSetter ...) call already exists at FormatStackTraceForJS.cpp:806 for Error.captureStackTrace. The new code is guarded by !trace || trace->isEmpty(), so the sync path (which always has JS frames) is untouched. errorInstanceLazyStackCustomGetter explicitly handles the null-trace case by building an empty Vector<StackFrame> and calling computeErrorInfoToJSValue, then replaces the accessor with a plain data property, so there is no re-entrancy or repeated-computation concern. None of the calls in the new block can throw, so no RETURN_IF_EXCEPTION is needed.

That said, SystemError__toErrorInstance is the constructor for essentially every syscall-derived error object in the runtime, and the lazy getter runs Error.prepareStackTrace (user code) at .stack access time rather than at construction. That is the same timing Node uses and the same behavior captureStackTrace already has in Bun, but it is a behavior change on a very widely-hit path, so a maintainer should confirm this is the layer they want the fix at (vs. the JSC-side materializeErrorInfoIfNeeded change the PR description mentions as future work).

Other factors

Test coverage is thorough per REVIEW.md's variant-matrix guidance: callback form for 9 fs functions, fs.promises via both await and .then(), createReadStream 'error' event, and Error.prepareStackTrace interaction isolated in a subprocess with all pipes drained via Promise.all. The nonexistent-path fixture is under tmpdir() and never created, so ENOENT is deterministic. The comment-cop bot's lint request was addressed in commit 32d79c6. CI build #80117 is still in flight, so there is no green signal yet.

[robobun]

CI status: the diff is green. The new async fs errors have a .stack string describe block in test/js/node/fs/fs.test.ts passed on every lane that ran it.

The red on builds #79991 and #80117 is unrelated infrastructure and known flakes:

• build-bun jobs timed out waiting on build-cpp jobs that stayed in scheduled and were never picked up by an agent (x64-asan/aarch64-musl on 79991; freebsd-x64/windows-x64 on 80117). The Rust side compiled and linked on every lane that ran; the C++ change compiled on all lanes that ran build-cpp.
• Pre-existing flakes, all marked [flaky] and none touching error construction or .stack: bun-install-registry.test.ts (hoisting, win-aarch64), html-rewriter-leak.test.ts (RSS threshold, x64-asan), proxy-stress-protocol.test.ts (ECONNRESET, x64-asan), vendor/elysia/test/response/stream.test.ts (x64-asan), no-orphans.test.ts (perl daemon timeout, darwin-x64), test-fastutf8stream-reopen.js, in-process-cron.test.ts.

Ready for review.

[robobun]

Superseded by #38074, which installs the same lazy .stack accessor here and also in the error-code constructor and the async-stack attach, with tests for each path. Closing in favor of that PR.