Repository navigation
Conversation
The handler installed for process.on(<signal>) only pushes the signal onto the PosixSignalHandle ring for the JS thread. During a --watch reload that thread never ticks again (it is running the kill-signal listeners, or the watcher thread is the one exec'ing), so a SIGTERM that arrived before on_before_reload_process_posix reset the dispositions sat in the ring and was discarded by execve; the process came back up with the same pid instead of dying. Drain the ring after the reset and raise() each signal whose disposition is now SIG_DFL, so it takes the default action it would have taken had it arrived a moment later. dequeue() skips zeroed slots since the reloading thread may race the main thread's own drain.
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on the released 1.4.0 and on a debug build of main with a With this branch both variants die of the SIGTERM before the execve. The two new cases in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes lock-free ring-buffer semantics (relaxing dequeue from single-consumer to tolerate a racing second consumer via zero-skip, with a plain store on head) and adds raise() calls in the pre-execve signal-reset path, a human look would still be worthwhile.
What was reviewed:
- The
dequeuerace: two consumers can both read the samehead, both advance it; the loser's slot swap yields 0 and loops, so no signal is duplicated.headcan briefly regress, but only during the committed-to-execve window. - Re-raise loop: queue is fully drained into a
sigset_tbefore anyraise(), and only signals whose disposition is nowSIG_DFLare raised, so a re-raise cannot re-enterBun__onPosixSignaland refill the ring. Bun__takeQueuedPosixSignalnull-safety: returns 0 when no VM or nosignal_handler, so the C++whileloop terminates cleanly on the no-listener path.- Tests: both new cases await the observable condition (exit vs. second "started"), buffer stdout to line boundaries, use the shared
watchee/afterEachSIGKILL cleanup, and are self-limiting on leak.
Extended reasoning...
Overview
This PR fixes a race in bun --watch where a signal (e.g. SIGTERM) delivered to the process while a reload is under way is caught by the process.on() handler, queued onto the PosixSignalHandle ring for the JS thread, and then silently discarded by the execve because the JS thread never ticks again. The fix drains that ring in on_before_reload_process_posix (after dispositions are reset to SIG_DFL) and re-raises each queued signal so it takes its default action. Three files: src/jsc/PosixSignalHandle.rs (new FFI export Bun__takeQueuedPosixSignal, dequeue now loops past zeroed slots), src/jsc/bindings/c-bindings.cpp (drain + re-raise after the sigaction reset), and two new tests in test/cli/watch/watch.test.ts.
Security risks
None identified. The change is confined to the process's own signal handling immediately before it replaces its own image; no external input reaches the new code.
Level of scrutiny
High. This is signal-handling code interacting with a lock-free SPSC ring that is being relaxed to tolerate a second consumer. dequeue previously had a single consumer (the main thread); the new caller may run on the watcher thread concurrently with the main thread's drain. The zero-skip loop handles the lost-race case, but head is advanced with a plain store (not CAS), so an interleaving where consumer A advances past consumer B's read, then B stores an older head+1, can move head backwards. The PR's argument is that this only happens after the process is committed to execve, so the worst outcome is a signal being both dispatched to JS in the dying image and re-raised — which is acceptable. That reasoning looks sound to me, but it is exactly the kind of "correct because of the specific calling context" invariant a maintainer should sign off on.
The C++ side is careful: it collects all queued signals into a sigset_t before raising any (so a fatal raise doesn't strand later entries — though a fatal signal terminates anyway), bounds-checks 0 < s < NSIG, and re-checks the current disposition is SIG_DFL before raising (skipping SIGSEGV/SIGBUS/RT/sigThreadSuspendResume, which the reset loop leaves caught). The while (int s = ...) loop terminates on 0, which Bun__takeQueuedPosixSignal returns for empty/no-VM/no-handler.
Other factors
The PR description is unusually thorough, the tests deterministically place a SIGTERM in the ring at reload time (self-kill from inside the kill-signal listener, and self-kill before a busy-spin), and the author reports 10 consecutive debug-build passes plus the surrounding signal/watch test surface. The tests follow harness conventions (tempDir, bunEnv, line-buffered stdout, shared afterEach SIGKILL cleanup, bounded busy-spin so a leaked child self-terminates). No CODEOWNERS cover these paths. Deferring because the concurrency-model change to dequeue and the choice to raise() in the reload path are design decisions worth a maintainer's eyes, not because anything looks wrong.
…lers (#41177) ### Problem - Signal delivery to JS segfaults at address 0 in `JSC::Identifier::add` under `Bun__onSignalForJS` (Sentry BUN-4BS1: 314 events, macOS arm64, all with `spawn`). `signalNumberToNameMap->get(0)` returns a null `String`. - `PosixSignalHandle::enqueue` (`src/jsc/PosixSignalHandle.rs`) assumes one producer, but handlers run on several threads. Two handlers load one `tail`. The slower one stores it behind `head`, and `dequeue` then returns consumed slots, which hold 0. ### Fix - `bun_threading::SignalRing` packs `head` and `tail` in one atomic word. A producer claims its index with one CAS on that word. A slot holds 0 until its producer stores the signal, and `dequeue` stops there. - The handler no longer logs (`scoped_log!` is not async-signal-safe and deadlocked debug builds) or panics on a number outside 1..=255. - `Bun__onSignalForJS` returns false for a number with no name, like the Windows path. - Verified: `signal_ring.rs` unit tests pass under `cargo test` and `cargo miri test`, and fail 10 of 10 runs on the old ring. Also the `process`, `watch`, and `test-signal-*` suites. Self-reviewed: 8 concerns, 7 addressed (rejected "no user report": Sentry shows the crash). ### Background - `process.on("SIGxxx")` installs a handler that queues the number in the ring and wakes the loop. The loop calls `Bun__onSignalForJS` for each number. - The kernel runs a process-directed signal's handler on one thread: the main thread, unless it blocks the signal, as `bun-spawn.cpp:250` does during a spawn. A handler may only use atomics and async-signal-safe calls. - The ring is MPSC: many producers, one consumer. 0 marks an empty slot, since signal numbers start at 1. #38689 adds a second consumer that must also stop at 0. <details><summary>Notes</summary> Reproduction on Linux (release `bun 1.4.1`): 24 worker threads each calling `process.kill(process.pid, sig)` 100k times with SIGUSR1/SIGUSR2 crashed the process with `Segmentation fault at address 0x0` on the main thread in 2 of 10 runs. Fewer signals or one signal type did not reproduce in the runs tried, which matches a race that needs a handler on a non-main thread to be preempted for a while. A spawn-based JS fixture at test size never crashed the unfixed release binary, so it is not part of this PR. A `bun:internal-for-testing` probe test was removed at review: the Rust unit tests are the coverage. The same stress against the unfixed debug build deadlocked instead of crashing. ASAN backtrace of the stuck main thread: `Bun::forwardSignal` -> `Bun__onPosixSignal` -> `PosixSignalHandle::enqueue` -> `ScopedLogger::is_visible` -> `Once::call` (futex wait), nested inside another `forwardSignal` -> `enqueue` -> `is_visible` -> `evaluate_is_visible` -> `alloc_zeroed`. The outer handler found the ring full, entered the logger's `Once`, and the nested handler waited on that same `Once`. Why one atomic word: a first version kept `head` and `tail` separate and claimed `tail` with a CAS. Review found that a producer descheduled between its two loads judges fullness on a stale pair and drops a signal from a ring that is not full. `never_rejects_below_capacity` in `signal_ring.rs` caught that (5 rejects with at most 40 of 64 slots in use). The packed word makes the check exact. `signal_ring.rs` tests: `concurrent_producers_deliver_every_accepted_signal_once` (8 x 50k, ring allowed to fill), `never_rejects_below_capacity`, `fifo_across_both_wraps` (slot wrap and u16 wrap, fullness at exactly CAPACITY), `stops_at_a_claimed_slot_until_it_is_written`. Under Miri the stress uses 4 x 200 and the four tests take about 6 s. The old single-producer protocol, placed inside `SignalRing` with everything else unchanged: `concurrent_producers_deliver_every_accepted_signal_once` fails 10 of 10 runs, with 57,334 to 125,804 zero sentinels per run. `never_rejects_below_capacity` hangs in 10 of 10 runs (killed after 60 s), because lost entries keep its producers waiting. The removed JS probe failed 3 of 3 runs on the same protocol. Handler changes in `Bun__onPosixSignal`: the loop wakeup moved out of `enqueue` and runs only after a successful enqueue. A review finding (99755d2) replaced `u8::try_from(number).expect(..)`. A panic in a signal handler aborts the process, and a 0 in the ring would stop the consumer at that slot for good. A number outside 1..=255 is now dropped. Suites run with the fixed debug build: `test/internal/linear-fifo.test.ts`, `test/internal/source-lints/`, `test/js/node/process/process.test.js -t signal`, `test/js/node/process/process-signal-listener-count.test.ts`, `test/cli/watch/watch.test.ts`, `test-vm-sigint.js`, `test-vm-sigint-existing-handler.js`, `test-process-remove-all-signal-listeners.js`, `test-signal-args.js`, `test-signal-handler-remove-on-exit.js`, `test-signal-handler.js`, `test-signal-unregister.js`, `test-watch-mode-kill-signal-override.mjs`, plus the 24-thread stress that deadlocked the old debug build (3/3 clean). Why macOS: XNU picks the first thread that does not block the signal, and `bun-spawn.cpp` uses `fork()` there with all signals blocked on the calling thread for the whole handshake. Linux prefers the main thread unless it is blocked or already has that signal pending, so the same race is rarer. One Linux canary event exists (BUN-44YR). Related open PR: #38689 also edits `dequeue` for an exec-time consumer and skips 0 slots there. With this ring a 0 at `head` is a claim in progress, not garbage, so that consumer must stop at it too. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 11 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/internal/signal-ring.test.ts bun test v1.4.1 (a6c4cc2) test/internal/signal-ring.test.ts: # Unhandled error between tests ------------------------------- SyntaxError: Export named 'signalRingProbe' not found in module 'bun:internal-for-testing'. ------------------------------- 0 pass 1 fail 1 error Ran 1 test across 1 file. [2.53s] error: script "bd" exited with code 1 __F:-1:S:0 release without fix: BUILD FAILED (no junit output) bun test v1.4.1-canary.1 (a6c4cc2) test/internal/signal-ring.test.ts: # Unhandled error between tests ------------------------------- SyntaxError: Export named 'signalRingProbe' not found in module 'bun:internal-for-testing'. ------------------------------- 0 pass 1 fail 1 error Ran 1 test across 1 file. [83.00ms] __F:-1:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/internal/signal-ring.test.ts bun test v1.4.1 (a6c4cc2) test/internal/signal-ring.test.ts: (pass) every signal the ring accepts from 8 concurrent producers comes out once, never as 0 [196.77ms] 1 pass 0 fail 1 expect() calls Ran 1 test across 1 file. [2.19s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 34fefe3 features baseline 23 deps, 131 codegen, 1172 objects in 742ms ninja: Entering directory `/workspace/bun/build/release' [1/1244] install /workspace/bun bun install v1.4.1-canary.1 (a6c4cc2) Checked 26 installs across 63 packages (no changes) [8.00ms] [2/1244] install /workspace/bun/packages/bun-error bun install v1.4.1-canary.1 (a6c4cc2) Checked 1 install across 2 packages (no changes) [1.00ms] [3/1244] gen ErrorCode+*.h [4/1244] fetch zlib [zlib] up to date [5/1244] install /workspace/bun/src/node-fallbacks bun install v1.4.1-canary.1 (a6c4cc2) Checked 111 installs across 104 packages (no changes) [14.00ms] [6/1244] gen bindgenv2 [7/1244] fetch libjpeg-turbo [libjpeg-turbo] up to date [8/1217] gen .bind.ts → GeneratedBindings.cpp [9/1217] fetch tinycc [tinycc] up to date [10/1216] gen node-fallbacks/react-refresh.js Bundled 1 module in 11ms react-refresh.js 4.81 KB (entry point) [11/1216] gen bake.{client,server,error}.js -> bake.client.js, bake.serve ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/codegen/generate-js2native.ts | 1 + src/js/internal-for-testing.ts | 4 + src/jsc/PosixSignalHandle.rs | 96 ++------ src/jsc/VirtualMachine.rs | 2 + src/jsc/bindings/BunProcess.cpp | 6 +- src/runtime/lib.rs | 1 + src/runtime/signal_ring_testing.rs | 80 +++++++ src/threading/lib.rs | 2 + src/threading/signal_ring.rs | 261 +++++++++++++++++++++ test/internal/signal-ring.test.ts | 28 +++ .../source-lints/dead-code-escape-limits.json | 2 +- 11 files changed, 403 insertions(+), 80 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/codegen/generate-js2native.ts 0 0 8 src/js/internal-for-testing.ts 0 0 9 src/jsc/PosixSignalHandle.rs 6 10 16 src/jsc/VirtualMachine.rs 0 0 9 src/jsc/bindings/BunProcess.cpp 4 2 10 src/runtime/lib.rs 0 0 8 src/runtime/signal_ring_testing.rs 0 1 9 src/threading/lib.rs 1 2 8 src/threading/signal_ring.rs 1 2 9 test/internal/signal-ring.test.ts 0 2 8 test/internal/source-lints/dead-code-escape-limits.json 0 0 9 ``` </details> <!-- robobun:evidence:end -->
Problem
bun --watchprocess while it is restarting is silently dropped:kill <pid>(SIGTERM) during a restart leaves the process running on the new image, with the same pid.test/js/node/test/parallel/test-watch-mode-kill-signal-override.mjstiming out (debian 13 x64-asan lane of an unrelated PR, passed on retry): the parent sends SIGTERM the moment it sees__SIGINT received__, and the log then showsscript ready <same pid>again and the test waits forever.process.on(<signal>)(forwardSignalinsrc/jsc/bindings/BunProcess.cpp) only pushes the signal onto thePosixSignalHandlering for the JS thread to dispatch. Once a watch reload is under way the JS thread never ticks again:VirtualMachine::reloadruns the--watch-kill-signallisteners synchronously and goes straight intoreload_process(src/jsc/VirtualMachine.rs,src/runtime/node/node_process.rs), and when there are no kill-signal listeners the watcher thread callsreload_processitself (src/jsc/hot_reloader.rs).on_before_reload_process_posix(src/jsc/bindings/c-bindings.cpp) resets the dispositions to SIG_DFL, which only covers signals arriving after that point; anything already in the ring is discarded by the execve.Fix
on_before_reload_process_posixdrains the ring after the disposition reset andraise()s each queued signal whose disposition is now SIG_DFL. The ring is emptied before anything is raised, and signals the reset loop leaves caught (SIGSEGV/SIGBUS, JSC's suspend signal, RT signals) are skipped, so a re-raise cannot refill the ring.PosixSignalHandle.rsexportsBun__takeQueuedPosixSignalfor that, anddequeueskips zeroed slots: the reloading thread may be the watcher thread racing the main thread's own drain, and the loser of that race sees a slot the other side already emptied.test/cli/watch/watch.test.ts("a signal caught during a --watch reload is not lost", 2 tests): each makes the watched script send itself a SIGTERM it has a listener for, so the signal is deterministically sitting in the ring when the reload happens. One covers the JS-thread path (the kill-signal listener sends it, then callsprocess.exit()), the other the watcher-thread path (no kill-signal listener, JS thread spinning). Both fail on the current release (a second boot is observed) and pass with this change (the process dies of SIGTERM); 10 consecutive runs on the debug build pass.test/cli/watch/watch.test.ts,test/cli/hot/watch.test.ts, the three vendoredtest-watch-mode-kill-signal-*.mjs(override 8 times in a row, one boot each),process-signal-listener-count.test.ts, thesignaltests intest/js/node/process/process.test.js, and the vendoredtest-signal-*.js/test-process-remove-all-signal-listeners.js.Background
bun --watchrestarts byexecve-ing itself in the same process (node uses a separate watcher process that kills and respawns the child).reload_process(src/bun_core/util.rs) is the single exit for every restart: the watcher thread's immediate reload, the JS thread after emitting the kill-signal listeners, the grace timer that forces a reload around a wedged JS thread, and auto-reload on crash.--watch-kill-signal(default SIGTERM) names the signal node would send the child; bun emulates it by running that signal's JS listeners synchronously right before the execve, which is why the JS thread cannot dispatch anything else at that point.PosixSignalHandleis a lock-free ring written by the signal handler and drained by the main thread at the top of each event loop tick intoPosixSignalTasks; a caught signal lives there between delivery and its JS listener running. Pending (blocked, undelivered) signals survive an execve, but a signal that was already delivered to a handler does not.What this does not cover
A SIGTERM that arrives after the execve is handled by the new image normally (it dies, or its freshly registered listener runs); that is a different, much narrower window than the one fixed here, which spanned the whole
process.exit()-> compile-cache flush -> fd cleanup -> sigaction reset sequence on an ASAN build. The residual race in the vendored test would need the parent to react more slowly than a full bun re-exec and script boot.