fix: route process.emit('exit'/'beforeExit') through user-visible emit#29196
fix: route process.emit('exit'/'beforeExit') through user-visible emit#29196
Conversation
Node dispatches the 'exit' and 'beforeExit' events by looking up the 'emit' property on the process object at call time, so user overrides of process.emit (e.g. signal-exit's monkey-patch) are honored. Bun was walking its internal EventEmitter listener list directly, bypassing any user-installed emit, leaving libraries like signal-exit silent during shutdown. Look up 'emit' on the process object in dispatchExitInternal and Process__dispatchOnBeforeExit and call it with process as 'this'. If a user replaced emit with a non-function, fall back to the internal emitter so lifecycle events still fire. Any thrown exception is reported through the global object's uncaught-exception path. Fixes #29194
|
Updated 1:24 PM PT - Apr 12th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 29196That installs a local version of the PR into your bun-29196 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughProcess lifecycle dispatch now invokes the runtime Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29194.test.ts`:
- Around line 73-74: Remove the flaky empty-stderr assertion by deleting the
expect(stderr).toBe("") line in the test (leave the exit code check
expect(exitCode).toBe(7) intact); this ensures the test no longer fails on
ASAN/CI spurious stderr output while still asserting the expected exitCode.
- Around line 107-108: Remove the empty-stderr assertion by deleting the
expect(stderr).toBe("") line in the test so ASAN warnings won't cause failures;
keep the existing expect(exitCode).toBe(0) check (i.e., locate the
expect(stderr).toBe("") statement near the exitCode assertion in the test and
remove it).
- Around line 41-42: Remove the brittle stderr assertion in the test so ASAN
warnings don't cause false failures: delete the expect(stderr).toBe("")
assertion in test/regression/issue/29194.test.ts (the line referencing stderr)
and rely solely on the exitCode assertion (expect(exitCode).toBe(0)) to detect
crashes/failures when spawning Bun subprocesses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 644e7d27-08bb-47d8-9ac8-bf58e9983bbd
📒 Files selected for processing (2)
src/bun.js/bindings/BunProcess.cpptest/regression/issue/29194.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/regression/issue/29194.test.ts (1)
37-37:⚠️ Potential issue | 🟡 MinorDrop empty-stderr assertions; they are flaky on ASAN CI.
expect(stderr).toBe("")can fail on ASAN due to known runtime warnings unrelated to regression correctness. Keep exit-code and stdout assertions as the signal instead.🔧 Proposed fix
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); + const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); @@ - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); + const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); @@ - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); + const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);Based on learnings: "In oven-sh/bun regression tests under
test/regression/issue/*.test.ts, avoid asserting that spawned Bun subprocessstderris empty... useexitCodeas the primary crash/failure signal."Also applies to: 65-65, 95-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29194.test.ts` at line 37, Remove the flaky empty-stderr assertions in test/regression/issue/29194.test.ts by deleting the expect(stderr).toBe("") checks (currently at the assertion around line where stderr is checked and the duplicates at the other two locations), and rely on existing exitCode and stdout assertions as the test signal; update any related comments or expectations in the same test function (the assertions surrounding stderr, stdout, and exitCode variables) so the test no longer asserts stderr equality to an empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29194.test.ts`:
- Around line 38-41: The test asserts process exitCode before validating stdout
which reduces diagnostic value; reorder the assertions so the
JSON.parse(stdout.trim()) and subsequent expect(seen).toContain("exit") and
expect(seen).toContain("beforeExit") run before expect(exitCode).toBe(0). Locate
the block using variables exitCode and stdout and the parsed seen array
(JSON.parse(stdout.trim())), move the stdout/seen assertions ahead of the
exitCode assertion, and apply the same reordering to the other occurrences
around the lines referencing exitCode and stdout (the similar blocks at the
other indicated positions).
---
Duplicate comments:
In `@test/regression/issue/29194.test.ts`:
- Line 37: Remove the flaky empty-stderr assertions in
test/regression/issue/29194.test.ts by deleting the expect(stderr).toBe("")
checks (currently at the assertion around line where stderr is checked and the
duplicates at the other two locations), and rely on existing exitCode and stdout
assertions as the test signal; update any related comments or expectations in
the same test function (the assertions surrounding stderr, stdout, and exitCode
variables) so the test no longer asserts stderr equality to an empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb02b920-f2ee-4fb0-b794-46a387345d50
📒 Files selected for processing (1)
test/regression/issue/29194.test.ts
ASAN CI lanes print warnings to stderr, so empty-stderr assertions race. Using stdout payload + exit code as the crash signal instead, with stdout checked first for clearer diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29194.test.ts`:
- Around line 17-95: Add a regression test that covers the fallback path when
process.emit is non-callable by creating a new test (similar to the existing
ones) that sets process.emit = null (or a non-function), then registers an
"exit" handler and triggers a natural shutdown or explicit process.exit, and
asserts the handler runs and the process exits with the expected code; this
ensures the runtime path in callProcessEmit that falls back to the internal
emitter is exercised and guarded against regressions (reference symbols:
process.emit, callProcessEmit, and the existing test names like "process.emit
override is invoked for explicit process.exit(code)").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 777b10f5-0ee9-470b-ab96-4d9e59f16ee8
📒 Files selected for processing (1)
test/regression/issue/29194.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…cessEmit DECLARE_CATCH_SCOPE isn't used anywhere else in this codebase and was causing CI cpp build lanes to fail. Rewrite callProcessEmit using the exact pattern from constructStdioWriteStream: - DECLARE_TOP_EXCEPTION_SCOPE - JSC::profiledCall with ProfilingReason::API - tryClearException(), not clearException() - skip reporting termination exceptions, which JSC expects to propagate
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BunProcess.cpp`:
- Around line 284-290: The fallback branch that calls process->wrapped().emit
when callData.type == JSC::CallData::Type::None can propagate a JS exception and
return early, skipping the same exception reporting and shutdown continuation
performed in the callable path; modify the fallback to mirror the callable path
by catching or checking for a pending exception after
process->wrapped().emit(...) and, if an exception exists, invoke
reportUncaughtExceptionAtEventLoop(...) with the exception and then continue to
call reallyExit(...) (or otherwise ensure shutdown proceeds) so both paths
behave identically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b50ed92-5707-455c-b83e-87f1bbf4b972
📒 Files selected for processing (1)
src/bun.js/bindings/BunProcess.cpp
…Emit When process.emit is replaced with a non-function, the fallback path walks internal listeners directly. If one throws, mirror the callable path and route the exception through reportUncaughtExceptionAtEventLoop so process.exit(code) doesn't short-circuit reallyExit().
There was a problem hiding this comment.
LGTM — all prior review concerns (termination-exception handling, fallback-path exception reporting, assertion ordering) have been addressed.
Extended reasoning...
Overview
Adds a callProcessEmit helper in BunProcess.cpp that dispatches exit and beforeExit lifecycle events through the runtime-visible process.emit property (looked up at call time) rather than walking the internal EventEmitter listener list directly. This makes Bun match Node.js behavior for libraries like signal-exit that monkey-patch process.emit.
Security risks
None. This is a pure behavioral correctness fix for event dispatch ordering; no auth, crypto, or permissions code is touched.
Level of scrutiny
The change touches dispatchExitInternal and Process__dispatchOnBeforeExit — process lifecycle code that every Bun program runs through at shutdown. That warrants care, but the implementation is straightforward: it follows the existing Process_functionEmitHelper pattern and all exception-handling now correctly uses DECLARE_TOP_EXCEPTION_SCOPE, vm.isTerminationException() gating, and tryClearException(), matching every other call site in the file.
Other factors
All comments from the previous review round have been fully addressed across commits ce075c9, 999a10a, and f172e6b. The fallback (non-callable process.emit) branch now has identical exception-report-and-clear logic as the callable path. Regression tests follow repo conventions (stdout assertions before exitCode, no empty-stderr assertions, bunExe/bunEnv from harness).
…verrides When process.emit isn't monkey-patched (the common case), dispatch lifecycle events through the internal EventEmitter directly instead of the JS-visible 'emit' binding. The binding goes through EventEmitter::emitForBindings, which gates on scriptExecutionContext() being alive — that is not guaranteed during natural shutdown, and the gate was silently dropping 'exit'/'beforeExit' listeners (including Node test harnesses' common.mustCall checks). Check if 'emit' is an own property of process to detect user override. Only the own-property case goes through the lookup-and-call path; otherwise we call process->wrapped().emit() directly, matching the pre-fix semantics for the default path.
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed.
Extended reasoning...
Overview
The PR adds a callProcessEmit helper in BunProcess.cpp that routes exit and beforeExit lifecycle events through the user-visible process.emit at call time (checking for an own-property override), matching Node.js semantics. The test file adds three child-process regression tests covering natural shutdown, explicit process.exit(code), and the signal-exit wrapper pattern.
Security risks
None. This is a compatibility fix with no auth, crypto, or permissions involvement.
Level of scrutiny
The C++ changes touch process shutdown, which is critical but well-contained. The helper follows established exception-handling patterns in the file (DECLARE_TOP_EXCEPTION_SCOPE, tryClearException(), termination-exception skip), and the fallback path is symmetric with the callable path. All edge cases identified during review were resolved.
Other factors
All inline comments from prior review rounds are marked resolved. The three commits since initial submission (999a10a, ce075c9, f172e6b) address every issue raised: ASAN-safe test assertions, stdout-before-exitCode ordering, proper termination-exception handling, and fallback-branch exception reporting. No bugs were found by the automated bug hunting system.
There was a problem hiding this comment.
LGTM — all concerns from the previous review have been addressed.
Extended reasoning...
Overview
The PR adds a callProcessEmit helper in BunProcess.cpp that routes exit and beforeExit dispatch through the JS-visible process.emit own-property if one exists, matching Node.js's behavior. Two functions are updated: dispatchExitInternal and Process__dispatchOnBeforeExit. A regression test file covers natural shutdown, explicit process.exit(code), and the signal-exit wrapper pattern.
Security risks
No security-sensitive code paths (auth, crypto, permissions) are touched. The new dispatch code uses getOwnPropertySlot on the process object — a standard JSC lookup — and does not expose any new attack surface.
Level of scrutiny
This touches C++ process-lifecycle code, which warrants careful scrutiny. Multiple rounds of review identified and fixed: (1) use of DECLARE_CATCH_SCOPE (wrong pattern) → switched to DECLARE_TOP_EXCEPTION_SCOPE; (2) unconditional clearException() without checking vm.isTerminationException → fixed with the termination-guard pattern used everywhere else in the file; (3) fallback branch missing exception reporting → both callable and non-callable paths are now symmetric. The final code state correctly matches the established patterns in BunProcess.cpp and ZigGlobalObject.cpp.
Other factors
The bug hunting system found no issues. Tests use bunExe/bunEnv from harness, assert stdout before exitCode, and omit the ASAN-sensitive expect(stderr).toBe('') assertions. The nextTick-queue drain after beforeExit is now unconditional, matching Node semantics. All inline review comments are resolved.
getOwnPropertySlot may return true for entries in the prototype's static property hash table (EventEmitter.prototype.emit), causing the non-override path to take the JS-binding route through emitForBindings — which drops listeners when scriptExecutionContext() is being torn down during shutdown. Use getDirect instead, which only reads instance storage. A value there can only come from 'process.emit = fn', which is the exact condition we want to detect for signal-exit-style overrides.
There was a problem hiding this comment.
LGTM — all review concerns addressed.
Extended reasoning...
Overview
Modifies src/bun.js/bindings/BunProcess.cpp to add a callProcessEmit helper that routes exit/beforeExit lifecycle events through the user-visible process.emit property (via getDirect to detect own-property overrides only), with a fallback to the internal emitter. Adds three regression tests in test/regression/issue/29194.test.ts.
Security risks
None. The change is scoped to process lifecycle event dispatch and doesn't touch auth, crypto, or permission code.
Level of scrutiny
Medium — this is a process shutdown code path, but the logic is straightforward. The use of getDirect (rather than get) to detect own-property overrides only is the correct approach and consistent with how Node.js behaves (honoring monkey-patches without routing through the prototype chain).
Other factors
All prior review comments have been resolved: DECLARE_TOP_EXCEPTION_SCOPE + tryClearException() + termination-exception guard are in place; both branches of callProcessEmit share a single exception handler; test assertion ordering and ASAN-unsafe expect(stderr).toBe("") assertions were fixed. The pre-existing processIsExiting data race is not introduced by this PR.
…rride Earlier refactors removed the hasEventListeners guard and reordered the putDirect(_exiting) call. Restore the exact original flow for the no-user-override case — the only behavioral change should be an additional user.emit dispatch path when process.emit is overridden. Split the helper into userEmitOverride (pure lookup) and callUserEmitOverride (call with exception handling), keeping the rest of dispatchExitInternal byte-identical to pre-fix semantics.
The hasEventListeners guard was previously gating the putDirect of process._exiting, so when no exit listeners were registered, the flag stayed false despite the process being in shutdown. Node sets it unconditionally. Move the assignment before all early returns. Reported by claude[bot] review on #29196.
The hasEventListeners guard was previously gating the putDirect of process._exiting, so when no exit listeners were registered, the flag stayed false despite the process being in shutdown. Node sets it unconditionally. Move the assignment before all early returns. Reported by claude[bot] review on #29196.
robobun
force-pushed
the
farm/a7624688/signal-exit-emit
branch
from
e3ccdf0 to
4015afb
Compare
beforeExit path uses the return value of emit() to decide whether to drain the nextTick queue, matching Node. Previously the user-override branch set fired=true unconditionally, causing the queue to drain even when the patched emit returned false. callUserEmitOverride now returns toBoolean of the JS result; the beforeExit caller uses it for the drain decision. The exit caller discards the value since it doesn't drain.
There was a problem hiding this comment.
LGTM — all prior review concerns have been addressed.
Extended reasoning...
Overview
This PR routes process.emit('exit') and process.emit('beforeExit') through the user-visible emit property (using getDirect) rather than directly invoking the internal EventEmitter listener list. It adds a userEmitOverride helper and a callUserEmitOverride helper in BunProcess.cpp, plus three regression tests in test/regression/issue/29194.test.ts.
Security risks
None. The change only affects how lifecycle events are dispatched during process shutdown and does not touch auth, crypto, or permissions.
Level of scrutiny
Moderate — BunProcess.cpp is runtime-critical, but the change is well-scoped to two call sites (dispatchExitInternal and Process__dispatchOnBeforeExit) with clear Node.js compatibility semantics.
Other factors
All four issues identified in earlier review rounds were fixed:
- Exception scope type and termination-exception guard (ce075c9)
- Fallback path exception reporting symmetry (f172e6b)
_exitingset unconditionally before any early return (4015afb)firedcorrectly derived from the override return value (23538f9)
The bug hunting system found no additional issues. Tests cover the three primary scenarios (natural shutdown, explicit process.exit(code), and signal-exit-style wrapper).
There was a problem hiding this comment.
LGTM — all prior review concerns addressed.
Extended reasoning...
Overview
This PR routes process.emit('exit') and process.emit('beforeExit') through the user-visible process.emit property at call time rather than walking the internal EventEmitter listener list directly. Two files changed: src/bun.js/bindings/BunProcess.cpp (new userEmitOverride/callUserEmitOverride helpers + updated dispatchExitInternal and Process__dispatchOnBeforeExit) and a new regression test file.
Security risks
No security-sensitive code paths. The change is purely about Node.js compatibility for event dispatch during shutdown — no auth, crypto, or permissions involvement.
Level of scrutiny
The change touches production-critical process lifecycle code (shutdown events), which warrants careful review. However, the implementation has gone through multiple rounds of iterative fixing and all substantive issues have been resolved: exception handling now uses DECLARE_TOP_EXCEPTION_SCOPE + tryClearException() with a termination-exception skip; the fallback (non-callable user emit) path has symmetric exception reporting; _exiting is set unconditionally before any early return; and callUserEmitOverride returns the JS truthy result so the beforeExit drain decision matches Node semantics.
Other factors
All inline comments from prior review rounds are resolved. The three regression tests cover natural shutdown, explicit process.exit(code), and the signal-exit monkey-patch pattern, with assertions in the correct order (stdout before exitCode). The remaining pre-existing data-race on the static bool is acknowledged out of scope and predates this PR. No new bugs found by the automated hunting system.
1 participant
Fixes #29194
Repro
Minimal demo without signal-exit, which monkey-patches
process.emit:Before: prints ``` (empty).
Node: prints
newListener,beforeExit,exit.Cause
Node dispatches the
exitandbeforeExitevents by looking upemiton the process object at call time, so user overrides are honored. Bun was walking its internalEventEmitterlistener list directly viaprocess->wrapped().emit(...), bypassing any user-installedprocess.emit. That leavessignal-exit(and any other lib that hooksprocess.emit) silent during shutdown.Call sites in
src/bun.js/bindings/BunProcess.cpp:dispatchExitInternal— called from natural-shutdown andprocess.exit(code)Process__dispatchOnBeforeExit— called when the event loop drainsFix
Extract a
callProcessEmithelper that does a standard propertygetfor"emit"on the process object, then invokes it withprocessasthis. This is the same patternProcess_functionEmitHelperalready uses for nextTick-routed events.exitandbeforeExitnow respect user overrides.Behavior if user set
process.emitto a non-function: fall back to the internal emitter so lifecycle events still fire. If the user emit throws, the exception is reported throughreportUncaughtExceptionAtEventLoop.The old
hasEventListenersearly-return indispatchExitInternalis dropped — with user-visible dispatch, a patchedemitshould be called even without a registered listener (matches Node)._exiting = trueis now set unconditionally, also matching Node.Verification
Test in
test/regression/issue/29194.test.tsspawns a child that monkey-patchesprocess.emit, attaches anexitlistener, and asserts the monkey-patch observed bothbeforeExitandexit. Covers:exitthrough patchedemitbeforeExitthrough patchedemitprocess.exit(code)dispatchesexitthrough patchedemitwith correct codeexit