spawn: don't reinstall fault-signal handlers after sync-spawn signal forwarding - #30776
spawn: don't reinstall fault-signal handlers after sync-spawn signal forwarding#30776robobun wants to merge 10 commits into
Conversation
|
Updated 2:17 PM PT - Aug 15th, 2026
❌ @robobun, your commit 49e6487 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 30776That installs a local version of the PR into your bun-30776 --bun |
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. WalkthroughThis PR adds fault-signal handler probing to internal testing APIs, prevents signal-forwarding cleanup from resetting JSC fault handlers during sync-spawn, validates the fix with a POSIX regression test, and applies minor formatting and documentation tweaks. ChangesJSC Signal Handler Preservation
Unrelated Minor Updates
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/spawn/sync-spawn-preserves-jsc-signal-handlers.test.ts`:
- Around line 46-87: Replace the unmanaged tmpdirSync usage by creating the
empty git config inside a harness-managed tempDir: remove tmpdirSync and the
standalone writeFileSync call that creates emptyGitConfig, call tempDir(...) to
create a temp directory with an "empty.gitconfig" entry (using tempDir(..., {
"empty.gitconfig": "" })) and set emptyGitConfig to that generated path; update
env to use that emptyGitConfig and keep BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING
and other env vars unchanged so cleanup is automatic.
🪄 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: 59335f5a-b1d9-4ef1-8f28-bfce5e1e4d63
📒 Files selected for processing (13)
src/crash_handler/lib.rssrc/errno/lib.rssrc/js/internal-for-testing.tssrc/perf/tracy.rssrc/runtime/api/crash_handler_jsc.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/webview/ChromeProcess.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rstest/js/bun/spawn/sync-spawn-preserves-jsc-signal-handlers.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
All earlier nits are addressed and I didn't find anything new, but the core change alters process-wide fault-signal dispositions that gate both JSC's VMTraps and Bun's crash reporter (and pairs with a WebKit-side change), so it's worth a human sign-off.
Extended reasoning...
Overview
The functional core is a one-line deletion: removing bun_crash_handler::reset_on_posix() from SignalForwarding::drop (src/spawn/process.rs:2929). Everything else is scaffolding around it — a new testing-only getFaultSignalHandlers() probe in crash_handler_jsc.rs (read-only sigaction(sig, NULL, &out)), a regression test that compares fault-handler partitions before/after a sync-spawn, dropping the now-unused bun_crash_handler dep from bun_spawn, an unrelated JSON.stringify wrap of OVERLAY_CSS in bake-codegen, and a large volume of pure rustfmt #[cfg(...)] reflow across ~10 files.
Security risks
None identified. The new unsafe block is a read-only libc::sigaction query gated behind bun:internal-for-testing. No new inputs are parsed and no privilege boundaries are crossed.
Level of scrutiny
High. The deleted line governs which handler receives SIGSEGV/SIGBUS/SIGILL/SIGFPE after every CLI sync-spawn. The PR's argument — that jscSignalHandler already chains unhandled faults to Bun's crash handler, so reinstalling it on top is both redundant and destructive to VMTraps — is well-reasoned and matches the WTF source, but the correctness depends on JSC's signal-chaining contract holding on every platform/build config (release vs ASAN, glibc vs musl, macOS). The PR description itself notes debug-ASAN builds never exhibited the regression and that release CI is the real guard. There's also a companion WebKit PR (#233) reordering CommonData::invalidateLinkedCode(); someone should confirm whether this change is safe to land independently of that.
Other factors
All six of my earlier inline comments (stale dep, capacity hint, tempDir convention, stale cross-references, .zig spec divergence) have been addressed or explicitly declined with reasoning, and the threads are resolved. The bundled bake-codegen.ts change is logically separate from the signal fix but is small and self-explanatory. The rustfmt reflow is noise-only. No CODEOWNERS cover the touched paths.
|
CI status for maintainer review (head 49e6487, rebased onto current main) Build #98613 (current head, finished): 178 of 179 jobs passed. The one red job is a single Every other failure in #98613 and #98552 was a retried flake that passed on retry or alone ( The new For the reviewer: the chaining invariant the bot review asks about is verifiable at Companion WebKit change oven-sh/WebKit#233 is green and mergeable, awaiting review. |
dc83ab1 to
268a31e
Compare
There was a problem hiding this comment.
All earlier review feedback is addressed and I found nothing further, but the core change alters fault-signal (SIGSEGV/SIGBUS) disposition behaviour that JSC's VMTraps and Bun's crash reporter both depend on — worth a maintainer sign-off on the chaining argument and the companion WebKit change before merging.
Extended reasoning...
Overview
The functional core is a one-line deletion: bun_crash_handler::reset_on_posix() is removed from SignalForwarding::drop in src/spawn/process.rs. Supporting changes: a new getFaultSignalHandlers() testing probe in crash_handler_jsc.rs (read-only libc::sigaction query, gated behind bun:internal-for-testing), a comprehensive regression test, removal of the now-unused bun_crash_handler dep from bun_spawn, and some unrelated cleanup (JSON.stringify the OVERLAY_CSS define in bake-codegen.ts, two stale comment edits).
Security risks
No new attack surface. The new native binding only reads signal dispositions via sigaction(sig, NULL, &out) and is only reachable from bun:internal-for-testing. The deleted call was itself the thing that mutated process-global signal state; removing it strictly reduces side effects. No auth/crypto/permissions/input-handling code is touched.
Level of scrutiny
High. The change is small and the reasoning in the PR description and the new doc-comment is thorough and internally consistent, but its correctness rests on a non-local invariant: that WTF's jscSignalHandler chains unhandled SIGSEGV/SIGBUS/SIGILL/SIGFPE to the handler it displaced (Bun's crash reporter), so removing the explicit reinstall does not regress crash reporting. That holds per the JSC source the description cites, but it's exactly the kind of cross-repo assumption (and there is a companion oven-sh/WebKit#233) that a maintainer familiar with the JSC signal layer should confirm. A mistake here would either re-break VMTraps or silently lose crash reports — both hard to notice in CI since debug ASAN builds never exhibited the original bug.
Other factors
All five inline threads from earlier review rounds (mine and CodeRabbit's) are resolved with corresponding commits; the diff reflects every requested fix. The author explicitly declined to edit the frozen .zig spec sibling, with sound reasoning, and deferred the pre-existing SIGIOT/SIGPOLL aliasing bug to a follow-up — both reasonable. CI build #54775 reports all 72 lanes green including the new test; only the Buildkite aggregate status is in a confused state. The regression test is well-designed (compares handler equivalence classes across two processes to neutralise ASLR) and would catch a reintroduction.
268a31e to
38b52c1
Compare
There was a problem hiding this comment.
All my earlier nits are addressed and no new issues found — deferring final sign-off to a maintainer since this changes fault-signal disposition handling on the crash-reporter / JSC VMTraps boundary.
Extended reasoning...
Overview
The core functional change is a single deleted line: bun_crash_handler::reset_on_posix() is removed from SignalForwarding::drop in src/spawn/process.rs, so the CLI sync-spawn path no longer reinstalls Bun's crash handler on SIGSEGV/SIGBUS/SIGILL/SIGFPE after the signal-forwarding scope unwinds. Supporting changes: a new getFaultSignalHandlers() probe in crash_handler_jsc.rs (unsafe libc::sigaction query, testing-only), a thorough regression test driving bun test --changed to exercise the scope inside a live JSC VM, removal of the now-unused bun_crash_handler dep from bun_spawn, and a handful of unrelated comment/codegen cleanups (bake-codegen.ts OVERLAY_CSS stringify, stale cross-reference comments in json_lexer.rs / bun-build-api.test.ts).
Security risks
No injection, auth, or data-exposure surface. The change does affect process-wide signal dispositions and crash-reporting behaviour: the correctness argument is that WTF's jscSignalHandler already chains unhandled faults to Bun's crash handler (which it displaced at JSC init), so removing the reinstall preserves crash reporting while restoring signal-based VMTraps. That argument is well-documented in the PR body and the new doc-comment, but it hinges on JSC/WTF behaviour that lives outside this repo (and there's a companion WebKit PR #233). The new unsafe block in the testing probe is a read-only sigaction(sig, NULL, &out) query and is gated behind bun:internal-for-testing.
Level of scrutiny
High. Signal handling sits at the intersection of Bun's crash reporter, JSC's stop-the-world GC / Worker.terminate() / watchdog interruption, and ASAN — getting it wrong manifests as spurious "Segmentation fault in JIT code" crashes (the bug being fixed) or, in the other direction, lost crash reports. The fix itself is one line, but verifying that not reinstalling the handler is safe in every configuration (release vs debug-ASAN, with/without the WebKit companion change, across the various sync-spawn callers) is the kind of judgment call a maintainer who owns this area should make.
Other factors
All five of my earlier inline threads (unused dep, capacity hint, stale comments, .zig spec divergence, SIGIOT/SIGPOLL aliasing follow-up) are resolved; the author addressed each one or gave a reasoned decline. The bug-hunting system found nothing this round. CI is green on all 74 build/test lanes per the author's status comment, with two unrelated infra flakes. The PR also bundles a small unrelated bake-codegen.ts change (explicit JSON.stringify of OVERLAY_CSS) — benign, but another reason a human should glance at the full diff before merge.
b944f76 to
4aefdf6
Compare
There was a problem hiding this comment.
No further issues found — all my earlier nits are addressed. The core one-line deletion looks correct given the reasoning, but since it changes process-wide fault-signal disposition (and its correctness hinges on jscSignalHandler chaining unhandled faults back to Bun's crash handler), it'd be good to have a maintainer familiar with the JSC signal layer / crash_handler confirm before merge.
Extended reasoning...
Overview
The PR removes a single bun_crash_handler::reset_on_posix() call from SignalForwarding::drop in src/spawn/process.rs, drops the now-unused bun_crash_handler dep from bun_spawn, adds a getFaultSignalHandlers() internal-for-testing probe in crash_handler_jsc.rs, and adds a regression test that compares fault-signal handler shape with and without a preceding CLI sync-spawn. It also bundles two small unrelated cleanups: JSON.stringify-wrapping OVERLAY_CSS in bake-codegen.ts and dropping a stale comment cross-reference in bun-build-api.test.ts.
Security risks
None identified. The change removes a sigaction reinstallation rather than adding one; the new probe is read-only (sigaction(sig, NULL, &out)) and gated behind bun:internal-for-testing. No untrusted-input parsing, auth, or crypto paths are touched.
Level of scrutiny
High. The functional diff is tiny, but it changes the process-wide disposition of SIGSEGV/SIGBUS/SIGILL/SIGFPE for every CLI path that goes through the sync-spawn signal-forwarding scope (bun run, bunx, bun test --changed, bun create, …). Correctness of both crash reporting and signal-based VMTraps after this change rests on the claim that WTF's jscSignalHandler chains unhandled faults to whatever handler it displaced at SignalHandlers::finalize() time — which is a JSC/WTF-internal invariant a maintainer should confirm. The PR also notes debug ASAN builds never exhibited the regression (reset_on_posix() early-returns under ASAN), so the test guards the invariant by shape-comparison rather than reproducing the original crash; release CI is the real guard.
Other factors
All prior inline review comments (mine and CodeRabbit's) are resolved. The author's CI-status comment indicates the lanes exercising this code pass and remaining reds are unrelated known issues. There's a companion WebKit change (oven-sh/WebKit#233) touching the same VMTraps area, which further suggests someone with JSC-signal context should look at both together. The bundled bake-codegen.ts change is orthogonal but low-risk. Given the subtlety of signal-handler layering in a JIT runtime, I'm deferring rather than approving.
|
For whoever picks up the final review, the one thing the bot review asks to have confirmed (that crash reporting still works after this change) is verifiable in two places in the vendored WTF source:
So for a real segfault the delivery order is unchanged by this PR: The regression test exercises exactly that scope ( |
…forwarding SignalForwarding::drop (the CLI sync-spawn wrapper used by bun run <pkg-script>, bun test --changed's git probes, bunx, bun create, bun pm version, …) called crash_handler::reset_on_posix() after Bun__unregisterSignalsForForwarding(). The forwarding set never included SIGSEGV/SIGBUS/SIGILL/SIGFPE and the unregister call already restores every signal it touched from previous_actions[], so the reset was a no-op for its stated purpose — but it reinstalled Bun's crash handler on the four fault signals with oldact=NULL, overwriting WTF's jscSignalHandler that WTF::SignalHandlers::finalize() had layered on top at JSC init. jscSignalHandler is what makes signal-based VMTraps work: when JSC needs to interrupt DFG/FTL code for a stop-the-world GC, Worker.terminate(), or the watchdog, it patches a halt instruction over each invalidation point, the mutator takes SIGSEGV, jscSignalHandler looks the PC up in DFG::pcCodeBlockMap, jettisons the CodeBlock (patching the halt back to a jump to the OSR-exit thunk), and returns Handled. With Bun's crash handler in its place the halt delivers straight to the crash reporter instead, and the process dies with a spurious segfault in JIT code. Users have been working around this with BUN_JSC_usePollingTraps=1. Drop the reset_on_posix() call; jscSignalHandler already chains unhandled faults to the crash handler it displaced. Add a crash_handler.getFaultSignalHandlers() test probe and a test that captures the fault-signal dispositions in a fresh process and in a bun test --changed process (which runs git via the sync-spawn scope before loading tests) and asserts the handler partition is identical. ASAN builds never exhibited the regression because reset_on_posix() early-returns under ASAN; the test asserts the invariant for both.
…ir for gitconfig fixture
The minified overlay CSS begins with `*{box-sizing:...}`. Pre-#30679
codegen buns reject that in the JSON-lex pass before the `define`
auto-quote fallback can treat it as a string literal, so building from
source breaks whenever the bootstrap bun predates that commit. Quote it
explicitly like the `side` define already is.
Also: colocate the empty gitconfig fixture with the test's git repo
inside one harness-managed `tempDir` instead of a standalone
`tmpdirSync()`, per test conventions.
…handler js_bindings object from ENTRIES.len()
…uote comment Same as 0d0cc60 for json_lexer.rs — bake-codegen.ts now explicitly JSON.stringify()s OVERLAY_CSS, so the 'passes verbatim' clause is no longer true. The describe.each() inputs below are the concrete example.
std::mem::zeroed is on the workspace disallowed-methods list; the bun_core::ffi::zeroed helper is the sanctioned form for zero-init of C POD structs (same pattern as crash_handler and Global).
… siblings Main tightened every other host fn in this module to private; the unreachable_pub lint flags the one remaining pub.
4aefdf6 to
b658f64
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto main (da12867). One conflict, in Re-verified after rebase: the new test passes under |
The full explanation lives in the PR, commit message, and the test file header; the source comments only need to say what not to do and where to read why.
The regression test is the guard against re-adding the reset; the history is in the commit that removed it.
There was a problem hiding this comment.
All prior feedback is addressed (including the reset_on_posix visibility nit, fixed in 49e6487) and this run found no new issues. Deferring final sign-off to a maintainer given this changes process-wide fault-signal dispositions in the crash-handler/JSC-VMTraps layering — the correctness argument that crash reporting is preserved rests on WTF's jscSignalHandler chaining (Signals.cpp:538-544, 594-596), which is worth a human confirming.
What was reviewed:
SignalForwarding::dropdeletion vs.Bun__unregisterSignalsForForwarding's own snapshot restore — the removed call was redundant for the forwarding set and only clobbered SIGSEGV/SIGBUS/SIGILL/SIGFPE.- The new
getFaultSignalHandlersprobe: read-onlysigaction(sig, NULL, &out), gated tobun:internal-for-testing, no exception-scope gaps. - Regression test hermeticity (git env pinned via
GIT_CONFIG_NOSYSTEM/local gitconfig; ASLR handled by comparing partition shape, not addresses). bake-codegen.tsJSON.stringify(OVERLAY_CSS)change and the two stale cross-references it invalidated — both now updated.
Extended reasoning...
Overview
The functional change is a single deleted line in src/spawn/process.rs: SignalForwarding::drop no longer calls bun_crash_handler::reset_on_posix() after Bun__unregisterSignalsForForwarding(). The rest is cleanup (drop the now-unused bun_crash_handler dep from bun_spawn, make reset_on_posix module-private) plus test scaffolding: a new getFaultSignalHandlers() probe in crash_handler_jsc.rs exposed via bun:internal-for-testing, and a regression test that compares fault-signal dispositions before/after a bun test --changed sync-spawn. A tangential build-from-source fix in bake-codegen.ts (wrapping OVERLAY_CSS in JSON.stringify) rides along with two stale-comment updates.
Security risks
None identified. The new probe is a read-only sigaction(sig, NULL, &out) query, only reachable via bun:internal-for-testing (gated behind BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING). No user input reaches native code beyond that, and no signal dispositions are changed by the new code — the fix is purely a deletion.
Level of scrutiny
High. The one-line deletion is small, but its correctness depends on a non-local invariant: that WTF's jscSignalHandler (installed on SIGSEGV/SIGBUS at JSC init) chains unhandled faults to the handler it displaced — Bun's crash reporter. If that chaining ever changed, removing reset_on_posix() would silently break crash reporting after any CLI sync-spawn. The author has cited the exact WTF source lines (Signals.cpp:594-596 for save, 538-544 for chain), and the regression test guards the observable invariant, but this is process-wide signal-handler layering across the JSC/Bun boundary and there's a companion WebKit PR (#233) — a maintainer familiar with that stack should confirm.
Other factors
Three prior review rounds from me have all been addressed: the stale json_lexer.rs and bun-build-api.test.ts cross-references, the .zig porting-spec question (declined with a reasonable rationale — the doc-comment + test guard the divergence), and the reset_on_posix pub → private nit (fixed in 49e6487). The comment-cop bot's paragraph-comment complaints were all trimmed. CI on the last full build was green on lanes exercising this code; the reds were documented fleet-wide breakages unrelated to this diff. The test is well-constructed (hermetic git env, ASLR-tolerant shape comparison, asserts the baseline is non-SIG_DFL so it can't pass vacuously) though it can only guard release behaviour indirectly since debug-ASAN builds never exhibited the regression.
1 participant
What does this PR do?
SignalForwarding::drop— the RAII cleanup around the CLI sync-spawn path used bybun run <pkg-script>,bun test --changed's git probes,bunx,bun create,bun pm version, … — calledcrash_handler::reset_on_posix()afterBun__unregisterSignalsForForwarding().The signal-forwarding set (
FOR_EACH_SIGNALinc-bindings.cpp:SIGINT/SIGTERM/SIGHUP/SIGQUIT/SIGABRT/…) never includedSIGSEGV/SIGBUS/SIGILL/SIGFPE, and the unregister call already restores every signal it touched from a saved snapshot. So the reset was a no-op for its stated purpose — but it reinstalled Bun's crash handler on the four fault signals witholdact=NULL, overwriting WTF'sjscSignalHandlerthatWTF::SignalHandlers::finalize()had layered on top at JSC init.Why does it matter?
jscSignalHandleris what makes signal-based VMTraps work. When JSC needs to interrupt DFG/FTL code asynchronously — stop-the-world GC (NeedStopTheWorld),Worker.terminate()(NeedTermination), watchdog timeout — theVMTraps::SignalSenderthread patches a halt instruction (hlton x86,dc zva, xzron arm64) over each invalidation point in the running CodeBlock. The mutator hits it, the kernel deliversSIGSEGV, andjscSignalHandlerdispatches to the VMTraps lambda, which looks the PC up inDFG::pcCodeBlockMap, jettisons the CodeBlock (patching the halt back to a jump to the OSR-exit thunk), and returnsHandled. The mutator OSR-exits and reachesVMTraps::handleTraps().With Bun's crash handler installed in its place, the halt delivers
SIGSEGVstraight to the crash reporter, and the process dies with a spurious "Segmentation fault in JIT code" whose stack shows a half-jettisoned CodeBlock. Users have been working around this withBUN_JSC_usePollingTraps=1.How
reset_on_posix()call fromSignalForwarding::drop.jscSignalHandleralready chains unhandled faults to the handler it displaced (Bun's crash handler), so crash reporting is unchanged.bun:internal-for-testingprobe,crash_handler.getFaultSignalHandlers(), that returns the four currentsa_sigactionaddresses vialibc::sigaction.bun -eprocess and in abun test --changedprocess (the one cross-platform path where the sync-spawn scope runs inside a live JSC VM and JS continues afterward) and asserts they match.reset_on_posix()itself is now private tocrash_handler; with the spawn call gone,init()in the same module is its only caller.bun_spawnno longer depends onbun_crash_handler.bake-codegen.tspassesOVERLAY_CSSthroughJSON.stringifylike the neighbouringsidedefine, so the codegen step does not depend on the bootstrap bun having the define auto-quoting from JSON lexer: tokenize?/*/(/)sodefineauto-quote can recover #30679 (it failed to build with an older bootstrap bun otherwise). The bundler test comment that pointed at the raw form is updated to match.Testing note
Debug ASAN builds never exhibited the regression because
reset_on_posix()early-returns underENABLE_ASAN. The test asserts the invariant — fault handlers identical with and without a preceding sync spawn — which holds for both build types, so it guards the release behaviour even thoughbun bdalone cannot reproduce the original failure. Release CI will catch a regression.A companion change (oven-sh/WebKit#233) reorders
CommonData::invalidateLinkedCode()so the HLT is patched to a jump before its PC is removed frompcCodeBlockMap, closing a separate window wherecodeBlockForVMTrapPC()can returnnullptrfor a still-live HLT.no test proof · iteration 15 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts