Conversation
On Linux without pidfd, the waiter thread and a JS SIGCHLD listener each call sigaction(SIGCHLD) and replace the handler of the other. A listener added after the first spawn left the waiter thread without a wakeup, so proc.exited never resolved. A listener added before it never fired. The waiter thread's handler now also forwards the signal to the JS listeners. BunProcess.cpp reports each change of a signal disposition for JS listeners through Bun__onSignalDispositionChanged. For SIGCHLD, the waiter thread then installs its handler again and checks its children once.
|
Status: fix and test are in this PR (#42933). The diff is green. One red test in CI is a known break on main (see below). How I reproduced it
On 1.4.3 canary this prints The new block in CI (build 116575)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe change connects JavaScript SIGCHLD listener lifecycle events to Linux and Android child-process waiter handling. The waiter tracks listener state, forwards SIGCHLD notifications when required, and preserves child-exit processing. Tests cover waiter-thread and pidfd modes. ChangesSIGCHLD listener disposition flow
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The SIGCHLD listener and child-exit paths are covered across waiter-thread and pidfd modes, with no remaining actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
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, two other things were checked and ruled out: the wakeup handler running in bun install (no JS VM) is fine because Bun__onPosixSignal returns before touching the event loop when there is no main-thread VM; and the Worker-start race cannot leave forwardSignal as the final SIGCHLD handler, since HANDLES_SIGCHLD is stored before the waiter thread's sigaction, so whichever side installs last ends with wakeup in place.
Extended reasoning...
Three confirmed findings are posted inline (SA_NOCLDSTOP changing observable behavior for JS SIGCHLD listeners under the waiter thread, a window where a Worker-started waiter thread's wakeup runs before JS_LISTENS_FOR_SIGCHLD is set, and the generic disposition hook not being wired for crash signals). This note records only what else was examined: the no-VM path of Bun__onPosixSignal in src/jsc/PosixSignalHandle.rs (early return on get_main_thread_vm() == None, eventfd write in wake() is async-signal-safe), and the hang variant of the Worker race in src/spawn/process.rs, which is excluded by the ordering of the HANDLES_SIGCHLD store relative to the waiter thread's sigaction. The inline findings warrant a human look before merge.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/spawn/process.rs— Users on Linux without pidfd who add process.on("SIGCHLD") after merging stop receiving the signal when a child is stopped or continued; the same script on a pidfd kernel still receives it. The listener's handler is nowwakeup, which src/spawn/process.rs:1384 installs with SA_NOCLDSTOP, whileforwardSignalat src/jsc/bindings/BunProcess.cpp:1570 has only SA_RESTART. Fix: while JS_LISTENS_FOR_SIGCHLD is set, installwakeupwithout SA_NOCLDSTOP so JS listeners see the same SIGCHLD set on both paths; reload_handlers already reruns on every toggle so the flags can follow the flag, and the waiter thread's wait4 at process.rs:1231 uses WNOHANG only, so an extra stop wakeup is a harmless no-op pass.Extended reasoning...
Kernels without pidfd_open (pre-5.3, gVisor, older seccomp profiles) set the waiter thread flag at src/spawn_sys/spawn_process.rs:544. Every spawn then goes through the waiter thread. On the base branch, a listener added after the first spawn ran under forwardSignal (SA_RESTART, no SA_NOCLDSTOP), so a SIGSTOP/SIGCONT of a child delivered SIGCHLD to JS. After this change set_js_listens_for_sigchld at process.rs:1395 reruns reload_handlers, so the final handler is always wakeup with sa_flags SA_NOCLDSTOP at process.rs:1384. The kernel then does not generate SIGCHLD for a stopped or continued child at all, so neither wake() nor Bun__onPosixSignal runs. The listener silently never fires for stop/continue for this population, and the same script on a pidfd kernel does fire. The PR Notes name this as accepted, but the sizing is one line: SA_NOCLDSTOP is only there to spare the waiter thread a wakeup, and its wait4 pass without WUNTRACED already ignores stopped children. Population: every waiter-thread user with a SIGCHLD listener; rate: every child stop/continue. Remedy: set sa_flags to 0…
Verification: nit, acknowledged in diff: the PR description's "Not changed" notes state "with the waiter thread, a JS SIGCHLD listener does not fire when a child stops. With pidfd it does, because forwardSignal has no SA_NOCLDSTOP" — that statement matches the code. Trigger: Linux without usable pidfd_open (src/spawn_sys/spawn_process.rs:544 sets the waiter thread flag on… | nit; acknowledged in diff: the…
…position changes The listener flag is now stored from Bun__onSignalListenerCountChanged, which runs before BunProcess.cpp installs forwardSignal. The waiter thread's handler can replace forwardSignal at any time after that, and it then already forwards to the listener. While a JS listener exists, the waiter thread installs its handler without SA_NOCLDSTOP. The listener then also hears a stopped and a continued child, as it does without the waiter thread. A lock orders the flag read and the sigaction of the two threads that can install.
|
Changes after the review (127bab9, b89093b):
A test merge with #42911 is clean. With both, the flags are |
There was a problem hiding this comment.
I re-reviewed the two follow-up commits and found no bugs; the ordering race I raised on the previous push is addressed (the listener flag now moves into Bun__onSignalListenerCountChanged, which runs before either sigaction). Because this changes the process-wide SIGCHLD disposition shared by two owners across threads and is only exercisable on Linux without pidfd, a human look would still be worthwhile.
What was reviewed:
- Handler ordering across the four interleavings of
reload_handlers(waiter thread, underRELOAD_HANDLERS_LOCK) andonDidChangeListenersadd/remove on the main thread; the eventfdwake()after each reinstall covers exits delivered whileforwardSignal/SIG_DFLwas installed. wakeupremains async-signal-safe:bun_sys::writeon the eventfd plusBun__onPosixSignal, the same callforwardSignalalready makes; the flag reads are plain SeqCst atomics with no lock.- Non-Linux targets:
Bun__onSignalDispositionChangediscfg(unix)with a no-op body outside Linux/Android, matching the#if !OS(WINDOWS)C++ declaration and call sites; the kqueue and pidfd paths are untouched. - The fixture drains stdout with
proc.exitedconcurrently and asserts exact per-stage signal counts; theit.eachcases are serial on purpose so a hung fixture is killed on the test timeout.
Extended reasoning...
Overview
The PR lets the Linux/Android spawn waiter thread (the non-pidfd fallback) and process.on("SIGCHLD") share the single SIGCHLD disposition. src/spawn/process.rs gains two AtomicBool flags, a Guarded<()> lock around reload_handlers, and a split of the old handler into wake() plus wakeup() which forwards to Bun__onPosixSignal when a JS listener exists. src/jsc/PosixSignalHandle.rs mirrors the listener count into the waiter thread flag inside Bun__onSignalListenerCountChanged and adds Bun__onSignalDispositionChanged, which src/jsc/bindings/BunProcess.cpp calls after installing forwardSignal and after restoring the old disposition. A fixture and a four-case it.each matrix in test/js/bun/spawn/spawn.test.ts cover waiter thread vs pidfd and listener before/after the first spawn.
Since my previous review, commits 127bab9 and b89093b moved the JS_LISTENS_FOR_SIGCHLD store from the post-sigaction hook into Bun__onSignalListenerCountChanged, which runs before any disposition change; that closes the interleaving I flagged where a Worker-started waiter thread could install wakeup with the flag still false. The second commit only shortens comments.
Security risks
None specific to this change. It does not parse untrusted input or touch auth/crypto. The one hazard class is signal-handler safety: wakeup now calls Bun__onPosixSignal, which is the same function the existing forwardSignal handler already invokes, and bun_sys::write on an eventfd initialized before the thread starts. No allocation, locking, or logging in the handler.
Level of scrutiny
High. Process-wide signal dispositions with two native owners and a cross-thread install order are a classic source of rare hangs, and the buggy configuration (Linux without pidfd_open) is not reproducible on most developer machines except via BUN_FEATURE_FLAG_FORCE_WAITER_THREAD. I traced the four interleavings of the waiter thread's reload_handlers and the main thread's add/remove path and found each ends with wakeup installed and the current flag value, with the trailing wake() covering any exit delivered to the interim handler. The remaining residual risk is the kind a maintainer familiar with the waiter thread and the crash-handler signal ownership should weigh, which is why I deferred rather than approved.
Other factors
The bug hunt ran to a dry streak with no findings. bun_spawn already references other Bun__* symbols by extern "C", so the new link reference follows precedent. The PR description explicitly names the same-class sites left unfixed (crash-signal restore, SIGINT/vm) as out of scope, which I noted as pre-existing on the prior push and did not re-raise. The test matrix asserts exact counts on a normalized JSON stream and orders the exit-code assertion last, per harness conventions; the serial it.each is justified inline.
|
Updated 8:26 AM PT - Sep 16th, 2026
❌ @robobun, your commit b89093b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42933That installs a local version of the PR into your bun-42933 --bun |
Problem
pidfd_open(old kernel, gVisor, seccomp), aprocess.on("SIGCHLD")listener added after the firstBun.spawnmakesawait proc.exitednever resolve. A listener added before it never fires.sigaction(SIGCHLD)replace each other.WaiterThreadPosix::reload_handlers()(src/spawn/process.rs:1364) installswakeup, which wakes the waiter thread.installForwardSignalHandler(src/jsc/bindings/BunProcess.cpp:1558) installsforwardSignal, which only queues the signal for JS. Removal of the last listener restoresSIG_DFL(BunProcess.cpp:1675).Fix
wakeupalso callsBun__onPosixSignalwhile a JS SIGCHLD listener exists.Bun__onSignalListenerCountChangedstores that fact beforeBunProcess.cppchanges the disposition.BunProcess.cppcalls the newBun__onSignalDispositionChangedafter each such change. For SIGCHLD, the waiter thread then installswakeupagain and wakes once. While JS listens,wakeuphas noSA_NOCLDSTOP, likeforwardSignal.wait4for each child again, so no child exit is lost whilewakeupis not installed. Without the waiter thread, the calls only store a flag.test/js/bun/spawn/spawn.test.ts(new block, both waiter thread cases time out without the fix). Alsospawn-signal,spawn-kill-signal,spawnSync,process-signal-listener-count, andprocess.test.js -t signal.Background
pidfd_openis not available. It sleeps inpoll()on an eventfd. Its SIGCHLD handler,wakeup, writes the eventfd. Then the thread callswait4(WNOHANG)for each child.sigaction()call wins.Bun__onPosixSignalis async-signal-safe. It queues a signal number for theprocess.on(<signal>)listeners.Notes
Repro (
sigchld-listener.js, run withBUN_GARBAGE_COLLECTOR_LEVEL=1 BUN_FEATURE_FLAG_FORCE_WAITER_THREAD=1 bun sigchld-listener.js after):afterTIMEOUT: exited never resolved, signals 1exited 0, signals 1beforeexited 0, signals 0exited 0, signals 2Order of the calls. On each change of the SIGCHLD listener count,
BunProcess.cppdoes three things in this order:Bun__onSignalListenerCountChangedstoresJS_LISTENS_FOR_SIGCHLD. From here on,wakeupforwards to JS (or stops), whichever thread installs it.forwardSignalfor a first listener, theSIG_DFLcheck for the removal of the last one.Bun__onSignalDispositionChangedchecksHANDLES_SIGCHLD. If it is set, it installswakeupagain and writes the eventfd.The waiter thread sets
HANDLES_SIGCHLDbefore its ownsigaction. So whicheversigactionruns last, the final handler iswakeup:wakeupis the handler.BunProcess.cppinstalls last: the waiter thread'ssigactionran before it, soHANDLES_SIGCHLDis set and step 3 installswakeupagain.Between step 2 and step 3 a SIGCHLD goes to
forwardSignal(JS gets it) or toSIG_DFL(nobody listens any more). The waiter thread misses it. The eventfd write in step 3 covers that: the child is a zombie by then, and the nextwait4(WNOHANG)pass reaps it.reload_handlers()reads the flag to choosesa_flags, and two threads can be in it (the JS thread in step 3, the waiter thread at its start). A lock around the read and thesigactionmakes the last install use the last value of the flag. The signal handler never takes that lock.The test.
BUN_FEATURE_FLAG_FORCE_WAITER_THREAD=1withBUN_GARBAGE_COLLECTOR_LEVELset forces the waiter thread. The fixture spawnscatthree times, one at a time. It waits for an echo before it closes stdin, so the waiter thread has already calledwait4for the child and sleeps. Only SIGCHLD can report the exit. It waits forproc.exitedand, while it listens, for the listener count to reach the expected number. The second child also getsSIGSTOPandSIGCONT, and the listener must hear each one. It runs with the listener added before and after the first spawn, with the waiter thread and with pidfd. The tests are serial on purpose:bun testkills a process that a timed-out test left behind only for serial tests.Without the fix (debug build of main, and 1.4.3 canary): the two waiter thread cases time out, the two pidfd cases pass. With the fix: 4 pass. The full
spawn.test.tspasses on the debug build (148 pass, 0 fail), which includes its second run with the waiter thread.Stress probes (not in the PR). 300 children exit while the main thread toggles the listener: 45 of 45 runs complete with the fix, 5 of 5 hang without it. A Worker does the first 100 spawns while the main thread toggles the listener on a 0 ms interval: 40 of 40 complete with the fix, 3 of 3 hang without it.
Same class, not fixed here.
onDidChangeListenerstreats a signal disposition as owned by JS listeners alone: the first listener replaces the handler, and removal of the last one goes toSIG_DFL. Other native users of a signal have the same conflict.Bun__onSignalDispositionChangedis generic so that they can use it too, but this PR changes only SIGCHLD:process.on("SIGABRT", f)thenprocess.off("SIGABRT", f),/proc/self/statusshows SIGABRT no longer caught (same for SIGSEGV, SIGBUS, SIGTRAP), so the crash handler is gone for that signal. Not a hang. A fix needs a per-signal install inbun_crash_handler:reset_on_posix()installs all six signals, which would replace a listener on another crash signal. It also needs a release build to test, because ASAN builds do not install the crash handler.node:vmbreakOnSigint(Instability with vm.runInNewContext(, { breakOnSigint: true }) #31885). repl: interrupt a running evaluation with Ctrl+C #33411 and node:util: make setTraceSigInt actually trace SIGINT #36382 are open and add SIGINT cases to the same two branches ofonDidChangeListeners.watchModeStickySignal, node compat batch: callback-throw dispatch, Assert class + native deep-equality parity, Intl gate + URL/buffer fallout, compile cache, watch kill-signal, profilers (+98 tests) #34660). SIGCHLD does not reuse it. That mechanism keepsforwardSignalinstalled for one signal, set once at startup on the main thread, and does the native work insideBun__onPosixSignal. The waiter thread starts later, from any thread, and also inbun install, where there is no JS VM andBun__onPosixSignalreturns at once. So the waiter thread keeps its own handler.Not changed.
sa_flagsline ofwakeup. spawn: install the waiter thread's SIGCHLD handler with SA_RESTART #42911 addsSA_RESTARTthere. A test merge of the two branches is clean, and the result isSA_RESTARTalone while JS listens, the same flags asforwardSignal.signal(). Whenwakeupis the handler, it restoreswakeupwith the flags ofsignal(). The waiter thread then installswakeupwith its own flags.cfg(linux, android).Other checks.
cargo clippy -p bun_spawn -p bun_jsc.cargo check -p bun_jscforaarch64-apple-darwin,x86_64-unknown-freebsd,aarch64-linux-androidandx86_64-pc-windows-msvc.verify-binary.ts binaryreports nothing for the debug build.test/regression/issue/ctrl-c.test.tsfails 4 vite cases on a debug build in this container with and without the change (they time out at 5 s).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/bun/spawn/spawn.test.ts