Repository navigation
fix: apply default signal disposition when no JS listeners are registered - #27311
coleleavitt wants to merge 4 commits into
Conversation
…ered When a signal (e.g. SIGHUP from terminal death) is delivered to Bun's event loop via Bun__onSignalForJS and no JavaScript listeners are registered via process.on(), the signal is silently swallowed. This causes processes to become orphaned zombies consuming memory indefinitely. Node.js behavior: when no listener exists for a signal, the default OS disposition takes effect (terminate for SIGHUP/SIGTERM/SIGINT). Bun should match this behavior. The fix checks listenerCount in Bun__onSignalForJS. If zero listeners exist, restore SIG_DFL via sigaction() and re-raise the signal so the OS terminates the process with the correct exit code (128 + signo). This is the root cause of orphaned opencode/Bun processes that persist after terminal sessions are closed, as reported in multiple issues. Refs: #1505, #7422
WalkthroughRestore default POSIX signal disposition and re-raise signals when no JS listeners exist (Windows unchanged). Unknown signals emit to bindings with an empty name and return. Existing JS-delivery path remains when listeners are present. Tests added to validate default-disposition and listener-managed signal behavior. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1143-1144: The early return after the `#endif` currently sits
outside the OS(WINDOWS) guard and silently swallows signals on Windows when
listenerCount == 0; to avoid future confusion, add a clear inline comment
immediately above the return in BunProcess.cpp (near the
listenerCount/emitForBindings logic) stating that this early return is
intentional on Windows, explaining that Windows cannot re-raise/restore handlers
the same way and that the behaviour intentionally preserves the old
emitForBindings(0) outcome, so maintainers understand why no handler
restore/re-raise is performed; reference the listenerCount variable and
emitForBindings call in the comment.
- Around line 1128-1135: The code currently converts signalNumber to a
signalName via signalNumberToNameMap and unconditionally creates an Identifier
(Identifier::fromString) which for unknown numbers yields an empty string and
causes listenerCount check to fall through to calling sigaction(signalNumber,
SIG_DFL) and raise(signalNumber); add a guard after computing signalName to
detect an empty/invalid WTF::String (or missing mapping) and, in that case,
avoid calling sigaction/raise and instead call the prior safe path
(emitForBindings or no-op) — i.e., only proceed to the listenerCount check and
potential sigaction/raise when signalName is non-empty/valid and maps to a known
signal; reference signalNumberToNameMap, Identifier::fromString,
process->wrapped().listenerCount, sigaction, and raise to locate the logic to
change.
|
Newest first ✅ 2f24c — Looks good! Reviewed 2 files across Previous reviews✅ 7fe2e — Looks good! Reviewed 2 files across 🟡 02b54 — 1 issue(s) found
✅ cf40e — Looks good! Reviewed 1 file in |
Address review feedback on #27311: - Skip SIG_DFL restore for SIGPIPE and SIGXFSZ (Node.js ignores these by default; they should never terminate the process) - Add Windows comment explaining the early return is intentional (no orphaned-process issue on Windows due to uv_signal_t) - Add tests for signal default disposition: SIGHUP/SIGTERM terminate when no listeners, SIGUSR1 handled with listener, and removing all listeners restores default disposition
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1139-1141: The check that references SIGXFSZ is unguarded and can
break builds on platforms where SIGXFSZ isn't defined; update the signal
filtering block that checks signalNumber (currently comparing against SIGPIPE
and SIGXFSZ) so that the comparison to SIGXFSZ is wrapped in an `#ifdef` SIGXFSZ
guard, keeping the SIGPIPE check unconditional; ensure you only reference
SIGXFSZ when the macro is defined to match the other uses in BunProcess.cpp.
In `@test/js/node/process/process-on.test.ts`:
- Around line 108-121: The test captures child.signalCode incorrectly by passing
it into Promise.all (it’s synchronous and read too early), so change each
affected Promise.all (the SIGHUP, SIGTERM, and "removing all listeners" test
blocks) to await the async values first (e.g., const [stdout, stderr, exitCode]
= await Promise.all([child.stdout.text(), child.stderr.text(), child.exited]);)
and then read the synchronous child.signalCode after awaiting child.exited
(e.g., const signalCode = child.signalCode;) before performing the if
(signalCode) / else assertions; update those three blocks to remove
child.signalCode from Promise.all and to read it post-exit.
---
Duplicate comments:
In `@src/bun.js/bindings/BunProcess.cpp`:
- Around line 1128-1135: The code calls signalNumberToNameMap->get(signalNumber)
and constructs an Identifier from the result, but when the map has no entry this
yields an empty Identifier so listenerCount==0 and the code incorrectly falls
through to resetting the handler and re-raising an unmapped signal; add a guard
after obtaining signalName (and before Identifier::fromString / listenerCount)
that checks if signalName.isEmpty() and if so use the safe pre-PR behavior (call
the emitForBindings fallback on process->wrapped() or otherwise no-op/return)
instead of calling sigaction/raise for an unknown signal; update the block
around signalName, Identifier::fromString(...), and
process->wrapped().listenerCount(...) to perform this emptiness check and early
return.
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/js/node/process/process-on.test.ts`:
- Line 108: The test currently awaits child.stdout.text(), child.stderr.text(),
and child.exited via Promise.all (the destructuring at the line with "const
[stdout, stderr, exitCode] = await Promise.all([...])" and similar instances at
the other two locations) but never uses stdout/stderr; either add a lightweight
assertion such as expecting stderr to be empty (e.g., expect(stderr).toBe(""))
to surface unexpected errors, or remove stdout and stderr from the Promise.all
and destructuring so you only await child.exited (or await child.exited and
capture only the output you assert), updating the calls that reference
stdout/stderr accordingly (look for the Promise.all usages around the three test
sites and modify them to assert or drop unused bindings).
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([child.stdout.text(), child.stderr.text(), child.exited]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
stdout and stderr are captured but never asserted.
In tests at lines 108, 139, and 201, stdout and stderr are read via Promise.all but neither is ever checked. If the intent is diagnostic (for debugging failed runs), consider adding a brief assertion like expect(stderr).toBe("") to surface unexpected error output. Otherwise, you can drop them from the destructuring to reduce noise.
🔧 Example: add stderr assertion or simplify
Option A — assert no unexpected stderr:
const [stdout, stderr, exitCode] = await Promise.all([child.stdout.text(), child.stderr.text(), child.exited]);
+ expect(stderr).toBe("");Option B — drop unused bindings:
- const [stdout, stderr, exitCode] = await Promise.all([child.stdout.text(), child.stderr.text(), child.exited]);
+ const exitCode = await child.exited;Also applies to: 139-139, 201-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/process/process-on.test.ts` at line 108, The test currently
awaits child.stdout.text(), child.stderr.text(), and child.exited via
Promise.all (the destructuring at the line with "const [stdout, stderr,
exitCode] = await Promise.all([...])" and similar instances at the other two
locations) but never uses stdout/stderr; either add a lightweight assertion such
as expecting stderr to be empty (e.g., expect(stderr).toBe("")) to surface
unexpected errors, or remove stdout and stderr from the Promise.all and
destructuring so you only await child.exited (or await child.exited and capture
only the output you assert), updating the calls that reference stdout/stderr
accordingly (look for the Promise.all usages around the three test sites and
modify them to assert or drop unused bindings).
…t during teardown When a signal (e.g. SIGINT from Ctrl+C) has no JS listeners, we restore SIG_DFL and re-raise it. However, Worker threads faulting during the resulting process teardown would trigger Bun's crash handler, which calls glibc backtrace() - a function that is not async-signal-safe. This caused segfaults in _Unwind_Find_FDE() when EH frame tables were partially freed. Fix: Before re-raising, restore terminal state via bun_restore_stdio() and reset SIGSEGV/SIGBUS/SIGILL/SIGFPE to SIG_DFL so teardown faults terminate cleanly instead of entering the crash handler. This matches the pattern used by raiseIgnoringPanicHandler() in Global.zig and onExitSignal() in c-bindings.cpp.
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 1162-1178: The crash handler teardown (bun_restore_stdio() and the
sigaction reset block for SIGSEGV/SIGBUS/SIGILL/SIGFPE) must only run when the
signal being processed has a terminating default disposition; add a check (e.g.,
helper is_terminating_default(signum)) and skip the teardown when the signal is
non-terminating (preserving existing filtering for SIGPIPE/SIGXFSZ). Wrap the
bun_restore_stdio() call and the block that sets reset.sa_handler = SIG_DFL and
calls sigaction(...) for SIGSEGV/SIGBUS/SIGILL/SIGFPE in this conditional so
crash handlers remain active if raise() would return for non-terminating
signals.
| // Restore terminal state before re-raising (matches onExitSignal behavior). | ||
| bun_restore_stdio(); | ||
|
|
||
| // Reset Bun's crash handler signals (SIGSEGV, SIGBUS, SIGILL, SIGFPE) to | ||
| // SIG_DFL before re-raising. Without this, Worker threads that encounter a | ||
| // fault during process teardown would trigger the crash handler's backtrace() | ||
| // call, which is not async-signal-safe and can segfault in _Unwind_Find_FDE(). | ||
| { | ||
| struct sigaction reset; | ||
| memset(&reset, 0, sizeof(reset)); | ||
| reset.sa_handler = SIG_DFL; | ||
| sigemptyset(&reset.sa_mask); | ||
| sigaction(SIGSEGV, &reset, nullptr); | ||
| sigaction(SIGBUS, &reset, nullptr); | ||
| sigaction(SIGILL, &reset, nullptr); | ||
| sigaction(SIGFPE, &reset, nullptr); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which signals have non-terminating default dispositions on Linux/macOS
# and could cause the crash handler reset to be permanent.
# Also check if Bun__onSignalForJS can be invoked independently of JS listener registration.
echo "=== Checking how Bun__onSignalForJS is called from Zig side ==="
rg -n "Bun__onSignalForJS\|onSignalForJS" --type-add 'zig:*.zig' --type zig -C 3
echo ""
echo "=== Checking how Bun__onPosixSignal routes signals ==="
rg -n "Bun__onPosixSignal\|onPosixSignal" --type-add 'zig:*.zig' --type zig -C 5
echo ""
echo "=== Checking existing crash handler setup patterns ==="
rg -n "SIGSEGV.*SIG_DFL\|sigaction.*SIGSEGV" --type cpp -C 2
echo ""
echo "=== Checking which signals Bun intercepts at the Zig level ==="
rg -n "sigaction\|signal_handler\|SignalHandler" --type-add 'zig:*.zig' --type zig -C 2 | head -80Repository: oven-sh/bun
Length of output: 294
🏁 Script executed:
#!/bin/bash
# First, find the BunProcess.cpp file and read the context around lines 1162-1178
echo "=== Finding BunProcess.cpp ==="
find . -name "BunProcess.cpp" -type f
echo ""
echo "=== Reading context around lines 1162-1178 of BunProcess.cpp ==="
wc -l src/bun.js/bindings/BunProcess.cpp
head -1200 src/bun.js/bindings/BunProcess.cpp | tail -100
echo ""
echo "=== Search for the function containing this code ==="
rg -n "Bun__onSignalForJS\|onSignalForJS" --type cpp src/bun.js/bindings/BunProcess.cpp -B 30 | head -50
echo ""
echo "=== Search broadly for signal in BunProcess.cpp ==="
rg -n "listenerCount\|signalNameIdentifier" --type cpp src/bun.js/bindings/BunProcess.cppRepository: oven-sh/bun
Length of output: 4021
🏁 Script executed:
#!/bin/bash
# Find where Bun__onSignalForJS is called from
echo "=== Searching for calls to Bun__onSignalForJS ==="
rg -n "Bun__onSignalForJS" --type cpp --type zig
echo ""
echo "=== Searching in bindings directory more broadly ==="
fd . src/bun.js/bindings -name "*Signal*" -o -name "*signal*"
echo ""
echo "=== Check if there's signal dispatch logic in Zig ==="
rg -n "onSignalForJS\|signalNumber" --type zig src/ -C 2 | head -100Repository: oven-sh/bun
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Read PosixSignalHandle.zig to see signal routing logic
echo "=== Reading PosixSignalHandle.zig around the call site ==="
wc -l src/bun.js/event_loop/PosixSignalHandle.zig
echo ""
echo "=== Context around line 91-95 ==="
sed -n '1,150p' src/bun.js/event_loop/PosixSignalHandle.zig
echo ""
echo "=== Check what signals are registered/handled ==="
rg -n "SIGWINCH\|SIGCHLD\|SIGCONT\|SIGURG\|SIGSEGV" src/bun.js/event_loop/PosixSignalHandle.zig
echo ""
echo "=== Check signal handler setup ==="
sed -n '150,250p' src/bun.js/event_loop/PosixSignalHandle.zigRepository: oven-sh/bun
Length of output: 4462
🏁 Script executed:
#!/bin/bash
# Find where signals are registered with sigaction using Bun__onPosixSignal
echo "=== Search for sigaction registration of signals ==="
rg -n "sigaction.*Bun__onPosixSignal\|Bun__onPosixSignal.*sigaction" --type cpp --type zig -B 5 -A 5
echo ""
echo "=== Search for signal handler registration ==="
rg -n "sa_handler.*Bun__onPosixSignal\|\.sa_handler.*Bun__onPosixSignal" --type cpp -B 3 -A 3
echo ""
echo "=== Search for signal setup in bindings ==="
rg -n "signal.*register\|setupSignal\|initSignal" --type cpp src/bun.js/bindings/ -C 3
echo ""
echo "=== Search for signal names setup to find which signals are handled ==="
rg -n "signalNames\[|SIGHUP\|SIGINT\|SIGTERM" --type cpp src/bun.js/bindings/BunProcess.cpp | head -40Repository: oven-sh/bun
Length of output: 2928
🏁 Script executed:
#!/bin/bash
# Search for where sigaction is actually called to register handlers
echo "=== Search for sigaction calls in C++ and Zig ==="
rg -n "sigaction" --type cpp --type zig -B 2 -A 2 | head -100
echo ""
echo "=== Check if signal handler registration filters signals ==="
rg -n "signal.*register\|for.*signal\|each.*signal" --type cpp src/bun.js/bindings/BunProcess.cpp -C 3 | head -80
echo ""
echo "=== Check main event loop initialization for signal setup ==="
fd . src/bun.js/event_loop -name "*.zig" -type f | head -10
echo ""
echo "=== Look for where Bun__onPosixSignal is registered ==="
rg -n "Bun__onPosixSignal" --type cpp --type zig -B 10 -A 5Repository: oven-sh/bun
Length of output: 13286
🏁 Script executed:
#!/bin/bash
# Find where process.on('signal') listeners are registered
echo "=== Search for where signal listeners are added dynamically ==="
rg -n "addEventListener\|on.*signal\|addListener" --type cpp src/bun.js/bindings/BunProcess.cpp -C 3 | head -100
echo ""
echo "=== Look for the onDidChangeListeners function that was mentioned ==="
sed -n '1409,1550p' src/bun.js/bindings/BunProcess.cpp
echo ""
echo "=== Check if there's signal registration when listener is added ==="
rg -n "forwardSignal\|sigaction" src/bun.js/bindings/BunProcess.cpp -C 3Repository: oven-sh/bun
Length of output: 5765
🏁 Script executed:
#!/bin/bash
# Continue reading onDidChangeListeners from where it ended
echo "=== Continue reading onDidChangeListeners around signal handler setup ==="
sed -n '1470,1600p' src/bun.js/bindings/BunProcess.cppRepository: oven-sh/bun
Length of output: 4762
Crash handler reset is unsafe for non-terminating signals.
When no JavaScript listeners are registered for a signal, the code unconditionally resets Bun's crash handler signals (SIGSEGV, SIGBUS, SIGILL, SIGFPE) to SIG_DFL before re-raising the signal. For signals with non-terminating default dispositions (e.g., SIGWINCH, SIGCHLD, SIGCONT, SIGURG), raise() returns instead of terminating the process, leaving the crash handlers permanently disabled. This can occur when:
- A user registers a listener for a non-terminating signal (
process.on('SIGWINCH', ...)), which registers the signal handler - The signal arrives and is enqueued
- The listener is removed, setting
listenerCountto 0 - The event loop processes the queued signal, triggering the crash handler reset code
raise()returns, and the process continues with disabled crash handlers- Any subsequent fault produces a raw crash without Bun's diagnostic handler
Currently only SIGPIPE and SIGXFSZ are filtered. The bun_restore_stdio() call and crash handler resets should be conditional on the signal actually having a terminating default disposition.
🛡️ Proposed fix: only run teardown for terminating signals
if (process->wrapped().listenerCount(signalNameIdentifier) == 0) {
`#if` !OS(WINDOWS)
// Node.js ignores SIGPIPE and SIGXFSZ by default (they should never
// terminate the process). Match that behavior here.
if (signalNumber == SIGPIPE
`#ifdef` SIGXFSZ
|| signalNumber == SIGXFSZ
`#endif`
) {
return;
}
- // Restore terminal state before re-raising (matches onExitSignal behavior).
- bun_restore_stdio();
-
- // Reset Bun's crash handler signals (SIGSEGV, SIGBUS, SIGILL, SIGFPE) to
- // SIG_DFL before re-raising. Without this, Worker threads that encounter a
- // fault during process teardown would trigger the crash handler's backtrace()
- // call, which is not async-signal-safe and can segfault in _Unwind_Find_FDE().
- {
- struct sigaction reset;
- memset(&reset, 0, sizeof(reset));
- reset.sa_handler = SIG_DFL;
- sigemptyset(&reset.sa_mask);
- sigaction(SIGSEGV, &reset, nullptr);
- sigaction(SIGBUS, &reset, nullptr);
- sigaction(SIGILL, &reset, nullptr);
- sigaction(SIGFPE, &reset, nullptr);
- }
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = SIG_DFL;
sigemptyset(&sa.sa_mask);
sigaction(signalNumber, &sa, nullptr);
+
+ // Only perform teardown (stdio restore, crash handler reset) for
+ // signals whose default disposition terminates the process.
+ // For non-terminating signals (ignore/continue), raise() returns
+ // and the process keeps running — we must not disable crash handlers.
+ bool isNonTerminating = false;
+#ifdef SIGWINCH
+ isNonTerminating |= (signalNumber == SIGWINCH);
+#endif
+#ifdef SIGCHLD
+ isNonTerminating |= (signalNumber == SIGCHLD);
+#endif
+#ifdef SIGCONT
+ isNonTerminating |= (signalNumber == SIGCONT);
+#endif
+#ifdef SIGURG
+ isNonTerminating |= (signalNumber == SIGURG);
+#endif
+ if (!isNonTerminating) {
+ bun_restore_stdio();
+ struct sigaction reset;
+ memset(&reset, 0, sizeof(reset));
+ reset.sa_handler = SIG_DFL;
+ sigemptyset(&reset.sa_mask);
+ sigaction(SIGSEGV, &reset, nullptr);
+ sigaction(SIGBUS, &reset, nullptr);
+ sigaction(SIGILL, &reset, nullptr);
+ sigaction(SIGFPE, &reset, nullptr);
+ }
+
raise(signalNumber);
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/BunProcess.cpp` around lines 1162 - 1178, The crash
handler teardown (bun_restore_stdio() and the sigaction reset block for
SIGSEGV/SIGBUS/SIGILL/SIGFPE) must only run when the signal being processed has
a terminating default disposition; add a check (e.g., helper
is_terminating_default(signum)) and skip the teardown when the signal is
non-terminating (preserving existing filtering for SIGPIPE/SIGXFSZ). Wrap the
bun_restore_stdio() call and the block that sets reset.sa_handler = SIG_DFL and
calls sigaction(...) for SIGSEGV/SIGBUS/SIGILL/SIGFPE in this conditional so
crash handlers remain active if raise() would return for non-terminating
signals.
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
Summary
When a signal (e.g.
SIGHUPfrom terminal death) is delivered to Bun's event loop viaBun__onSignalForJSand no JavaScript listeners are registered viaprocess.on(), the signal is silently swallowed. This causes processes to become orphaned zombies that consume memory indefinitely.Node.js behavior: when no listener exists for a signal, the default OS disposition takes effect (terminate for
SIGHUP/SIGTERM/SIGINT). Bun should match this.The Bug
In
BunProcess.cpp,Bun__onSignalForJS()unconditionally callsemitForBindings()on the process object. WhenlistenerCountis 0,emitForBindings()does nothing and the function returns — the signal is consumed without effect.Signal flow that triggers this:
sigaction()in several places (onDidChangeListeners,Bun__registerSignalsForForwardingduringspawnSync, etc.)forwardSignal/Bun__onPosixSignal) enqueues to ring bufferBun__onSignalForJSprocess.on()listener exists:emitForBindings()is a no-op → signal vanishesThe Fix
Check
listenerCountinBun__onSignalForJS. If zero listeners exist, restoreSIG_DFLviasigaction()and re-raise the signal so the OS terminates the process with the correct exit code (128 + signo). This matches Node.js'sSignalExit()pattern innode.cc.Reproduction
tmux kill-pane)SIGHUPfrom the kernel but continues running as an orphan (PPID=1)Real-world impact: On my system, 14 orphaned
opencode(Bun-based TUI) processes accumulated to 66 GB of RAM from repeated terminal session closures.Notes
process.on("SIGHUP", () => process.exit())insrc/js/internal/html.ts:363— this fix makes the workaround unnecessary for all Bun applications#if !OS(WINDOWS)guard ensures no behavioral change on Windows wheresigaction/raisearen't availablelistenerCount == 0so existing programs with explicit signal handlers are unaffectedRefs: #1505, #7422