Repository navigation
Conversation
…de, without Error.prepareStackTrace Both flags built their stack with new Error().stack in internal/process/pre_execution. That is the value of the user's Error.prepareStackTrace, so a formatter that returned a non-string made process.exit() and every process.env operation throw. Node prints both traces from native code. - Bun::formatCurrentStackTrace() formats the current frames as the default error.stack does. It makes no Error and runs no JS. - process.reallyExit() prints the --trace-exit warning, as Node's Environment::Exit() does. The JS wrapper of process.exit() is gone. - --trace-env-js-stack prints ten frames from the native capture. - The warning of the async_hooks stubs reads its stack the same way.
|
Updated 12:28 PM PT - Oct 18th, 2026
✅ @robobun, your commit f57967531bab6a56ce92459f937b19f7f4f3d1ac passed in 🧪 To try this PR locally: bunx bun-pr 45933That installs a local version of the PR into your bun-45933 --bun |
|
Status Reproduced on main (and on bun 1.4.3) with the two commands in the PR body:
The fix and the tests are in this PR: #45933 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughEnvironment stack traces and async-hooks warnings now use a native current-stack formatter. The ChangesNative stack formatting and consumers
Native process exit tracing
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
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.
Beyond the inline findings, I also checked the new Bun__Process__writeExitTrace boundary for string ownership — Bun::toString on the local WTF::String and to_utf8() on the Rust side both borrow, so no ref is leaked or double-released — and the reallyExit exit-code change: static_cast<uint8_t>(toInt32 % 256) yields the same byte as the old uint8_t path for every int32, so only the printed value changed. The per-VM m_traceExitFrameLimit lives on the Process object, so each worker enables and clears its own.
Extended reasoning...
The change moves the --trace-exit and --trace-env-js-stack stack capture from JS into a new native helper in FormatStackTraceForJS.cpp, adds a frame-limit optional on Bun::Process, prints the exit warning from Process_functionReallyExit via a new Rust stderr export, and adds subprocess tests. It touches process exit and native stack walking under DisallowVMEntry but no auth, crypto, or injection surface. Two test-coverage nits were reported inline; a human look is still worthwhile given the native exit path and the formatStackTrace-with-null-error contract the helper relies on.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
test/js/node/process/process-trace-flags.test.ts— nit: Maintainers get no regression coverage for two Error states the change claims to fix for --trace-exit. The--trace-exitmatrix at test/js/node/process/process-trace-flags.test.ts:51-57 omits "Error.stackTraceLimit is an accessor" and "Error is frozen", which the--trace-env-js-stackmatrix at line 713 runs over all oferrorStates. Fix: run the exit matrix over every key oferrorStates(or state why two are excluded), so a future read or write ofError.stackTraceLimiton the exit path shows up as the accessor's stdout line.Why this was flagged
The PR description lists
Object.freeze(Error)and an accessor onError.stackTraceLimitas inputs that brokeprocess.exit()on the base branch. The new exit test at test/js/node/process/process-trace-flags.test.ts:51-57 lists only five of the sevenerrorStateskeys, leaving those two out, while the env test at line 713 usesObject.keys(errorStates). The accessor state logs "Error.stackTraceLimit was read" / "was written" to stdout, and the exit test assertsstdout: "", so including it is the only check thatprocess.exit()under--trace-exitno longer touchesError.stackTraceLimit. If a later change reintroduces anErrorread on the exit path, no test fails. No runtime behaviour differs from the base; this is a coverage gap for the sibling inputs the REVIEW.md variant-matrix rule asks for.Verification: nit. A future change makes the
--trace-exitpath read or writeError.stackTraceLimit— nothing in the suite would catch it for the two omitted states. The--trace-exitmatrix at test/js/node/process/process-trace-flags.test.ts:51-57 omits"Error.stackTraceLimit is an accessor"and"Error is frozen". On base the exit path never read or wroteError.stackTraceLimit.
|
About the two review notes on the tests:
|
Problem
--trace-exitand--trace-env-js-stackreadnew Error().stack(src/js/internal/process/pre_execution.ts:131,:248). IfError.prepareStackTracereturns a non-string,process.exit()and eachprocess.envoperation throwTypeError: new Error().stack.split is not a function.bun --trace-exit --stack-trace-limit=0 -e "process.exit(0)"exits 1 withTypeError: undefined is not an object.exitlisteners run, so its code can be wrong. No user reported this.Fix
Bun::formatCurrentStackTrace()formats the current frames as the defaulterror.stackdoes, with noErrorand no JS.--trace-env-js-stackand thenode:async_hooksstub warning use it.process.reallyExit()prints the--trace-exitwarning, as Node'sEnvironment::Exit()does. The JS wrapper is gone.test/js/node/process/process-trace-flags.test.ts,async_hooks.node.test.ts(29 cases fail on main), and the four vendoredtest-trace-*.jsfiles, unedited.Background
Error.prepareStackTraceis the V8 hook that formatserror.stack. It can return any value.process.exit()runs theexitlisteners, then calls the nativeprocess.reallyExit(), which ends the process.process.envProxy stays.Downsides
at exit (unknown).process.exit(-1)printscode 255(Node:-1): Bun keeps 8 bits of the exit code.process.reallyExit(), +2,799 B of text,sizeof(Bun::Process)136 to 144.Notes
Repro (main, with node v26.3.0 for reference)
No user reported this. Both flags are hidden from
--helpand are not in the docs. The defect was found in the code.The same mechanism breaks both flags on main for a formatter that throws and for a replaced
globalThis.Error. WithError.stackTraceLimit = 0,process.exit()throws and the env trace loses its frames.Object.freeze(Error)and an accessor onError.stackTraceLimitbreak--trace-env-js-stackonly, because only its printer wrote that property. A formatter that returns a string runs once for each traced operation, and its text replaces the frames.What Node does
--trace-exit:Environment::Exitprints underDisallowJavascriptExecutionScope.process.reallyExitcalls it with its argument. The frame limit is the--stack-trace-limitoption.--trace-env-js-stack:DumpJavaScriptBacktraceprints 10 frames.Changes to the
--trace-exitoutputexitlistener setsprocess.exitCode = 7code 0, before the listenercode 7, after the listenerprocess.exit(9)in anexitlistenerprocess.exit()at the stack limitprocess.reallyExit(3)code 3process.reallyExitprocess.exit("abc")(throwsERR_INVALID_ARG_TYPE)code abc--stack-trace-limit=0TypeError, exit status 1Error.stackTraceLimit = 50, thenrecurse(20)process.exit(-1)code -1code 255code -1process.exit(300)code 300code 44code 30012 of 14 fixtures match node v26.3.0 on warning order, exit code and frame names (main: 8 of 14). The two others are the last two rows. Bun keeps 8 bits of the exit code (
ExitHandler::exit_codeis au8), andprocess.exit()passes those toprocess.reallyExit().process.exitCodeand theexitevent report the same 255 and 44. The main branch printed the argument ofprocess.exit()before it was stored. A directprocess.reallyExit(256)printscode 256.Measurements (release builds of main fd539ca with and without this change, linux x64)
size, 106,677,292 to 106,680,091). The stripped file grows by one 4,096 B page. New and changed native symbols +3,502 B (nm -S). Bundledpre_execution.js7,692 B (was 8,474).process.reallyExit: 33 instructions beforeBun__Process__exit(was 29,objdump).setProcessExitCodeInner,Process_functionExitand theprocess.envgetters are byte-identical in size.sizeof(Bun::Process): 136 to 144 bytes. The GC cell stays at 144 B.Error.stackTraceLimitgets and sets: 0 and 0 (main throws aTypeErrorwhen the property is an accessor, node 0 and 0). Formatter calls per traced exit: 0 (was 1, node 0).bun:jscheapStats, JIT off)./proc/self/io). Writes to fd 2 per traced exit: 1 (was 1,gdbcatch syscall write, becausestraceis not installed).instructions:ufrom aperf_event_opencounter, two-N difference, becauseperfis not installed.--trace-exitoverhead over no flag for-e 'process.exit(0)': 1.09 M instructions (was 4.03 M). Median of 10 alternated runs, spread 1.068 M to 1.111 M.require('node:async_hooks')with no flag: main 494, this branch 494.Not in this PR
util.getCallSitesandisInsideNodeModules(internal/shared.ts, reached fromurl.parse()) still swapError.prepareStackTracearound a capture. They need call sites, not text. Read Error.prepareStackTrace from the Error constructor when a stack is formatted #45570 and BUN_JSC_immutableGlobals: one switch that makes every built-in immutable #45177 each give them a native capture.printEnvTracewithError.captureStackTrace, which still runs the user formatter, and lists--trace-exitas not covered. After this change that hunk and that entry are not needed.String.prototype.split,startsWithorRegExp.prototype.execstill makes a tracedprocess.envread throw, as on main (node: no). The env printer keeps its string and regex calls. This change removes only its use ofError.asyncframes thaterror.stackhas, inside the frame limit. Node prints synchronous frames only.error.stackitself can still passError.stackTraceLimitby the frames of an await chain.at exit (unknown): the formatter prints every host function frame asunknown.process.exit()called from anexitlistener returns to the listener (worker: run the 'exit' listeners before the worker's VM is unpublished #40132). The warning is printed once, for the first call that reachesprocess.reallyExit().process.reallyExitand print no warning (Node prints one).atlines. Node prints numbered frames under a header, and--trace-env-js-stackimplies--trace-envthere.bun --trace-exit file.wasmnow ends with a warning, because the WASI runner callsprocess.reallyExit(0).vmtimeoutwith a blocked stderr). The test needs a reader that leaves stderr full for longer than the timeout, so it waits on a timer. Checked by hand: main throwsERR_SCRIPT_EXECUTION_TIMEOUTand keeps running. This branch exits, as Node does.BUN_FEATURE_FLAG_VERBOSE_WARNINGS=1, thenode:async_hookswarning looks for a known caller in 10 frames. Main looked inError.stackTraceLimitframes.process.reallyExit("3")andprocess.reallyExit(3.7)exit 0 (Node: 3). TheisAnyInt()check inprocess.reallyExit()is as it was.Why a new test file
The 27 cases each start a process and share three helpers.
test/js/node/process/process.test.jsalready has about 190 tests in 3,300 lines. In a debug build on a 12-core machine, three of its cases timed out in a run of the whole file, so the new cases are not in that batch.Suites run with the debug build
test/js/node/process/process-trace-flags.test.ts,test/js/node/async_hooks/async_hooks.node.test.ts,test/js/node/trace_events/trace-events.test.ts,test/js/node/v8/capture-stack-trace.test.js, theexittests oftest/js/node/process/process.test.js, and 50 vendored Node tests forprocess.exit,process.env, worker exit,util.inspect,vmtimeouts and the trace flags.BUN_JSC_validateExceptionChecks=1reports nothing for both flags.cargo checkpasses forx86_64-pc-windows-msvcandaarch64-apple-darwin.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/async_hooks/async_hooks.node.test.ts