Skip to content

process: install UnhandledPromiseRejectionWarning .stack as non-enumerable - #34292

Open
robobun wants to merge 2 commits into
mainfrom
farm/2df06bc1/unhandled-rejection-warning-stack-dontenum
Open

process: install UnhandledPromiseRejectionWarning .stack as non-enumerable#34292
robobun wants to merge 2 commits into
mainfrom
farm/2df06bc1/unhandled-rejection-warning-stack-dontenum

Conversation

@robobun

@robobun robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Repro

// bun --unhandled-rejections=warn repro.js
process.on('warning', w => {
  if (w.name === 'UnhandledPromiseRejectionWarning' && w instanceof Error) {
    const d = Object.getOwnPropertyDescriptor(w, 'stack');
    if (d) console.log({ keys: Object.keys(w), enumerable: d.enumerable });
  }
});
Promise.reject(new Error('boom'));
node --unhandled-rejections=warn repro.js   # { keys: [ 'name' ], enumerable: false }
bun  --unhandled-rejections=warn repro.js   # { keys: [ 'stack' ], enumerable: true }

Cause

Bun__promises__emitUnhandledRejectionWarning in src/jsc/bindings/BunProcess.cpp copies the rejection reason's .stack onto the warning ErrorInstance via warning->putDirect(vm, vm.propertyNames->stack, reasonStack). The 3-arg putDirect overload defaults to attributes 0, which is enumerable. Node assigns warning.stack = reason.stack, and V8's Error stack accessor keeps the property non-enumerable.

This was the one engine-installed .stack write on an ErrorInstance not covered by #34259, which fixed the same pattern at the FormatStackTraceForJS.cpp sites.

Fix

Pass JSC::PropertyAttribute::DontEnum on that putDirect, matching every other .stack write on an ErrorInstance in the bindings and ErrorInstance::materializeErrorInfoIfNeeded itself. The two other putDirect .stack writes with attributes 0 (BunProcess.cpp:2372, BunProcessReportObjectWindows.cpp:277) are left alone: they populate process.report.getReport().javascriptStack.stack, an enumerable array field on a plain object in Node as well.

Verification

New test in test/js/node/process/process.test.js spawns bun --unhandled-rejections=warn, picks the warning whose .stack is the reason's stack, and asserts enumerable: false, configurable: true, and that Object.keys / for..in / JSON.stringify all omit stack. Every asserted field was first checked against Node v26.3.0. The test does not assert data-vs-accessor shape (V8 keeps an accessor, JSC writes a data property); the observable enumerability behaviour is identical.

USE_SYSTEM_BUN=1 bun test process.test.js -t 'UnhandledPromiseRejectionWarning installs'  # fails (enumerable: true)
bun bd test process.test.js -t 'UnhandledPromiseRejectionWarning installs'                # passes

Also green: test/js/node/promise/reject-tostring.test.ts, test/js/node/v8/capture-stack-trace.test.js, the existing unhandledRejection tests in process.test.js, and BUN_JSC_validateExceptionChecks=1 over the warn path.


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

…rable

Bun__promises__emitUnhandledRejectionWarning copied the reason's stack onto
the warning ErrorInstance via the 3-arg putDirect overload, which defaults to
attributes 0 (enumerable). Node installs it non-enumerable, so Object.keys /
JSON.stringify / for..in / spread on the warning object diverged.

This was the one engine-installed .stack write on an ErrorInstance that #34259
did not cover.
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 12 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: 7b2e853b-08dc-4422-837e-5898b26f957e

📥 Commits

Reviewing files that changed from the base of the PR and between d93b4cc and dbafa9c.

📒 Files selected for processing (2)
  • src/jsc/bindings/BunProcess.cpp
  • test/js/node/process/process.test.js

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

@robobun

robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced with bun --unhandled-rejections=warn on a one-line script (Object.getOwnPropertyDescriptor(w, 'stack').enumerable is true, Node v26.3.0 gives false). Fix is the single putDirect attribute in Bun__promises__emitUnhandledRejectionWarning; new test in process.test.js fails on the released binary and passes with this change.

CI on builds 73577 and 73587: the new test passes on every lane. Remaining reds are unrelated to this diff (timer-heap-race.test.ts is a main break from #33131 being handled separately; test-net-connect-memleak.js is tagged pre-existing; bun-install-proxy / spawn / bun-jsc / server-sourcemap / test-fs-promises-file-handle-readFile / test-repl-close are tagged flaky). Ready for review.

@robobun

robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:52 PM PT - Jul 15th, 2026

@robobun, your commit dbafa9c has 1 failures in Build #73587 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34292

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

bun-34292 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — one-line attribute flag addition matching the pattern from #34259, with a targeted regression test.

What was reviewed:

  • Verified the DontEnum | 0 attribute matches every other .stack write on ErrorInstance in the bindings (FormatStackTraceForJS.cpp, NodeVM.cpp, BunProcess.cpp:2020) and the adjacent .name write on line 1320.
  • Confirmed the two intentionally-untouched putDirect(.stack, ..., 0) sites (BunProcess.cpp:2372, BunProcessReportObjectWindows.cpp:277) are on the process.report plain object, not an ErrorInstance, so leaving them enumerable is correct.
  • Test follows harness conventions (spawn with -e, await using, concurrent pipe drain) and is placed alongside the existing unhandledRejection tests.
Extended reasoning...

Overview

Single-character-class change: adds JSC::PropertyAttribute::DontEnum | 0 as the fourth argument to one putDirect call in Bun__promises__emitUnhandledRejectionWarning (src/jsc/bindings/BunProcess.cpp:1328). This makes the .stack property installed on the UnhandledPromiseRejectionWarning Error object non-enumerable, matching Node.js behavior. A 45-line regression test is added to test/js/node/process/process.test.js.

Security risks

None. This changes only a property attribute flag on a warning object; no input parsing, no auth, no allocation, no control-flow change.

Level of scrutiny

Low. This is a mechanical follow-up to #34259 (already merged as 8215078), which applied the identical fix to the FormatStackTraceForJS.cpp sites. A grep of putDirect.*propertyNames->stack across src/jsc/bindings/ confirms every ErrorInstance .stack write now uses DontEnum, and the two remaining attributes-0 writes are on the process.report.getReport().javascriptStack plain object where enumerability is intentional (as the PR description states and the code confirms — javascriptStack is a plain constructEmptyObject, not an ErrorInstance). The adjacent .name putDirect on line 1320 already uses the exact same DontEnum | 0 idiom, so this is purely bringing one line into consistency.

Other factors

The test is well-constructed: it spawns a subprocess with --unhandled-rejections=warn, filters to the specific warning whose .stack matches the reason's stack, and asserts enumerable: false / configurable: true plus that Object.keys, for..in, and JSON.stringify all omit stack. It correctly avoids asserting data-vs-accessor descriptor shape (a known JSC/V8 divergence). Pipes are drained concurrently, bunEnv/bunExe() are used, and it sits inside the existing describe.concurrent block next to related unhandledRejection tests. The PR description documents USE_SYSTEM_BUN=1 failure and bun bd pass. No outstanding reviewer comments; no prior claude[bot] reviews on this PR.

If a future regression causes no warning to match the filter, the fixture now
emits "null" instead of "undefined" so the assertion shows a readable
result: null vs {...} diff rather than a bare JSON.parse SyntaxError.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — one-token attribute addition matching the established pattern from #34259.

What was reviewed:

  • The putDirect change matches every other .stack write on ErrorInstance in the bindings (FormatStackTraceForJS.cpp, NodeVM.cpp, and the .name write two lines above).
  • Confirmed the two intentionally-excluded .stack writes (report objects) are on plain objects, not ErrorInstance — correct to leave alone.
  • Test's TDZ on reason is fine (handler runs async after the const initializes); the w.stack !== reason.stack filter correctly selects the second of the two emitted warnings.
Extended reasoning...

Overview

Single-line change in src/jsc/bindings/BunProcess.cpp: adds JSC::PropertyAttribute::DontEnum | 0 to the putDirect call that copies the rejection reason's .stack onto the UnhandledPromiseRejectionWarning ErrorInstance. Accompanied by a subprocess-spawning test in test/js/node/process/process.test.js that asserts the descriptor is non-enumerable and that Object.keys/for..in/JSON.stringify all omit stack.

Security risks

None. This changes only the enumerability attribute of a property on a warning object. No auth, crypto, permissions, untrusted input parsing, or memory management is touched.

Level of scrutiny

Low. This is a follow-up to #34259 (visible in recent commits) which fixed the same class of bug at other sites. The | 0 idiom and DontEnum attribute are used identically on line 1320 of the same function for .name, and at 7+ other .stack writes across FormatStackTraceForJS.cpp and NodeVM.cpp. There is no ambiguity in what the change does or why.

Other factors

  • The PR description explicitly enumerates the two other attributes-0 .stack writes (process.report.getReport().javascriptStack.stack) and correctly explains why they're excluded — those populate plain-object fields that Node also exposes as enumerable.
  • The test follows the file's conventions: await using on the spawn, concurrent drain of stdout/stderr/exited, combined-object assertion. It correctly handles the fact that two warnings fire (one with reasonStack as the message, one with the warning object) by filtering on w.stack === reason.stack.
  • PR author verified against Node v26.3.0 and confirmed USE_SYSTEM_BUN=1 fails / bun bd passes.
  • No prior reviewer comments to address; no CODEOWNERS concerns for a mechanical Node-compat attribute fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant