spawnSync: make signal-forwarding register/unregister safe for concurrent callers - #30956
spawnSync: make signal-forwarding register/unregister safe for concurrent callers#30956robobun wants to merge 14 commits into
Conversation
|
Updated 10:04 PM PT - Aug 15th, 2026
⏳ @robobun, your commit be93485 is still building in
|
WalkthroughRemoved SIGPWR from the Linux-only signal-forwarding macro used by spawnSync, added process-wide mutex and reference-counting around signal handler register/unregister, and added a Linux regression test that stresses overlapping spawns and GC to confirm the SIGPWR handler remains intact. ChangesSignal Forwarding Fix
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/c-bindings.cpp (1)
936-953:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSignal-forwarding globals are still unsafe under overlapping
spawnSynccalls.Removing
SIGPWRfixes this specific crash path, but the forwarding mechanism still relies on unsynchronized process-global state (previous_actions,Bun__currentSyncPID,Bun__pendingSignalToSend). With concurrentspawnSyncwindows, handlers/PID state can still be clobbered and restored incorrectly for other forwarded signals.Also applies to: 915-916, 979-1015
🤖 Prompt for 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. In `@src/jsc/bindings/c-bindings.cpp` around lines 936 - 953, The signal-forwarding globals (previous_actions, Bun__currentSyncPID, Bun__pendingSignalToSend) used by spawnSync are not safe for overlapping calls; serialize access and make the handler logic robust: introduce a single shared mutex (e.g., spawnSyncSignalMutex) and lock it around any code that installs/restores signal handlers and mutates previous_actions, Bun__currentSyncPID, and Bun__pendingSignalToSend inside the spawnSync implementation and its helper functions, and change the signal handler to verify the PID against an atomic Bun__currentSyncPID before acting (so stray signals are ignored); this ensures handlers/state cannot be clobbered by concurrent spawnSync calls and restores handlers only when the caller that set them is still the owner.
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/c-bindings.cpp`:
- Around line 936-953: The signal-forwarding globals (previous_actions,
Bun__currentSyncPID, Bun__pendingSignalToSend) used by spawnSync are not safe
for overlapping calls; serialize access and make the handler logic robust:
introduce a single shared mutex (e.g., spawnSyncSignalMutex) and lock it around
any code that installs/restores signal handlers and mutates previous_actions,
Bun__currentSyncPID, and Bun__pendingSignalToSend inside the spawnSync
implementation and its helper functions, and change the signal handler to verify
the PID against an atomic Bun__currentSyncPID before acting (so stray signals
are ignored); this ensures handlers/state cannot be clobbered by concurrent
spawnSync calls and restores handlers only when the caller that set them is
still the owner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3b75814-6478-48d0-97e4-fbcdfd357797
📒 Files selected for processing (2)
src/jsc/bindings/c-bindings.cpptest/js/bun/spawn/spawnSync-sigpwr-gc.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts:54— nit:expect(stderr).not.toContain("AddressSanitizer")is the negative crash-marker pattern that rootCLAUDE.md:126says to NEVER write ("These tests will never fail in CI"). The positive assertions onsignalCode,stdout === "ok", andexitCode === 0already fail if the process dies via SIGPWR or ASAN aborts, so this line adds no signal — just drop it.Extended reasoning...
What the issue is
test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts:54adds:expect(stderr).not.toContain("AddressSanitizer");
This is a negative assertion on a crash-marker string in subprocess output. The repo's root
CLAUDE.md(line 126) explicitly forbids this pattern:NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI.
"AddressSanitizer"is exactly "or similar" — it's a crash-marker string just like"panic"or"uncaught exception".Why this assertion adds no signal — step by step
Consider the two CI configurations:
-
Non-ASAN build (the common case in CI). AddressSanitizer is not compiled in, so
stderrcan never contain"AddressSanitizer"regardless of whether the bug is present. The assertion is vacuously true and cannot fail — this is precisely what the CLAUDE.md rule warns about. -
ASAN build, bug regresses. If the SIGPWR handler gets clobbered and the process is terminated by signal 30, the child does not exit cleanly. In that case:
proc.signalCodeis"SIGPWR", notnull→ line 53expect(proc.signalCode).toBeNull()fails.- The child never reaches
console.log("ok")→ line 55expect(stdout.trim()).toBe("ok")fails. exitCodeis non-zero / null → line 56expect(exitCode).toBe(0)fails.
And if ASAN itself reports a memory error and aborts, it does so with a non-zero exit code (and typically
SIGABRT), so the same three positive assertions catch it before the stderr check matters.
In every scenario where
not.toContain("AddressSanitizer")would fail, at least one of the surrounding positive assertions has already failed. There is no execution path where this line is the sole failing assertion.Why the existing assertions are sufficient
The test already follows the recommended structure: it asserts on
signalCode === null,stdout.trim() === "ok", andexitCode === 0. These are positive assertions on observable process state and they fully cover the regression being tested (process terminated by SIGPWR during GC). The stderr check is pure belt-and-suspenders that violates a documented "NEVER" rule.Fix
Just delete line 54. Replacing it with
expect(stderr).toBe("")(the CLAUDE.md-preferred form) is also an option but may be too strict here — 64 concurrentopenInEditorcalls against a fakecodebinary could plausibly emit incidental noise on stderr without indicating failure, so dropping the line entirely is the safer choice.This is a test-hygiene/convention nit, not a functional bug in the SIGPWR fix itself.
-
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/spawnSync-sigpwr-gc.test.ts`:
- Around line 41-49: Change the subprocess spawn options to pipe stderr (replace
stderr: "inherit" with stderr: "pipe"), capture proc.stderr.text() alongside
proc.stdout.text() and proc.exited (e.g. await Promise.all([... ,
proc.stderr.text(), proc.exited])), store that value in a stderr variable, and
insert the CI-friendly assertion if (exitCode !== 0) { expect(stderr).toBe("");
} immediately before expect(exitCode).toBe(0); keep the existing checks on
proc.signalCode and stdout.trim() as-is.
🪄 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: bea85ffa-50b0-4816-90cc-048b3b916ad6
📒 Files selected for processing (1)
test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts
| stdout: "pipe", | ||
| stderr: "inherit", | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| expect(proc.signalCode).toBeNull(); | ||
| expect(stdout.trim()).toBe("ok"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Pipe stderr for better CI failure diagnostics.
With stderr: "inherit", you cannot programmatically assert stderr contents or surface them in the test diff on failure. Per repo conventions for subprocess tests using bunEnv, prefer piping stderr and asserting it's empty (after filtering ASAN noise if needed).
♻️ Suggested change
stdout: "pipe",
- stderr: "inherit",
+ stderr: "pipe",
});
- const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+ const [stdout, stderr, exitCode] = await Promise.all([
+ proc.stdout.text(),
+ proc.stderr.text(),
+ proc.exited,
+ ]);
expect(proc.signalCode).toBeNull();
expect(stdout.trim()).toBe("ok");
+ if (exitCode !== 0) {
+ expect(stderr).toBe("");
+ }
expect(exitCode).toBe(0);Based on learnings: "In oven-sh/bun Jest/Bun test files under test/js/ that spawn subprocesses using bunEnv from the harness module, it's safe and intentional to assert expect(stderr).toBe("") unconditionally" and "write if (exitCode !== 0) { expect(stderr).toBe(""); } immediately before expect(exitCode).toBe(0)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stdout: "pipe", | |
| stderr: "inherit", | |
| }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| expect(proc.signalCode).toBeNull(); | |
| expect(stdout.trim()).toBe("ok"); | |
| expect(exitCode).toBe(0); | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([ | |
| proc.stdout.text(), | |
| proc.stderr.text(), | |
| proc.exited, | |
| ]); | |
| expect(proc.signalCode).toBeNull(); | |
| expect(stdout.trim()).toBe("ok"); | |
| if (exitCode !== 0) { | |
| expect(stderr).toBe(""); | |
| } | |
| expect(exitCode).toBe(0); |
🤖 Prompt for 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.
In `@test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts` around lines 41 - 49, Change
the subprocess spawn options to pipe stderr (replace stderr: "inherit" with
stderr: "pipe"), capture proc.stderr.text() alongside proc.stdout.text() and
proc.exited (e.g. await Promise.all([... , proc.stderr.text(), proc.exited])),
store that value in a stderr variable, and insert the CI-friendly assertion if
(exitCode !== 0) { expect(stderr).toBe(""); } immediately before
expect(exitCode).toBe(0); keep the existing checks on proc.signalCode and
stdout.trim() as-is.
There was a problem hiding this comment.
LGTM — one-line removal of SIGPWR from the Linux forwarding list, clearly correct given JSC's use of SIGPWR for GC thread suspension.
Extended reasoning...
Overview
This PR removes M(SIGPWR) from the FOR_EACH_LINUX_ONLY_SIGNAL macro in src/jsc/bindings/c-bindings.cpp (one deleted line plus a 3-line explanatory comment) and adds a Linux-only regression test in test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts that stresses concurrent openInEditor → spawnSync calls against GC to verify the SIGPWR handler is no longer clobbered.
Security risks
None. The change is purely subtractive — it stops spawnSync from replacing the process-wide SIGPWR handler on Linux. There is no new attack surface, no input handling, and no auth/crypto/permissions involvement. The test only manipulates a temp dir, a symlink to sleep, and PATH for a child Bun process.
Level of scrutiny
Low-to-moderate. Signal-handler registration is process-global state, so changes here deserve a careful read, but this particular change is a strict reduction in scope: one fewer signal is touched. The rationale (JSC uses SIGPWR for GC thread suspend/resume in WTF/wtf/posix/ThreadingPOSIX.cpp) is correct and well-documented, and the npm list this was copied from targets V8 where SIGPWR is unused — so dropping it has no user-visible downside while fixing a real fuzzer-found crash.
Other factors
My two earlier inline comments were explicitly flagged as pre-existing issues (SIGPOLL/SIGIOT aliasing, and the stale main-thread-only comment / broader previous_actions[] race). The author verified the aliasing observation and reasonably scoped both out to a follow-up to keep this PR focused on the fuzzer crash; neither blocks this change. CodeRabbit's stderr-piping suggestion is a test-diagnostics nit, not a correctness issue. No bugs were found by the bug-hunting system on this revision. CI had failures on an earlier commit and a follow-up commit (test: drop negative crash-marker assertion) was pushed; the source change itself is sound regardless.
|
CI status: 68/69 checks pass. The one failure is The new Already re-rolled once for unrelated flakes on build 55633 ( |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/c-bindings.cpp (1)
1016-1022:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly clear the forwarded PID on the outermost unregister.
Line 1018 resets
Bun__currentSyncPIDbefore the depth check. If twospawnSynccalls overlap, the first inner unregister leavessignalForwardingDepth > 0but disables forwarding for the still-active outer call, so a later signal in that window never reaches the child.Suggested fix
extern "C" void Bun__unregisterSignalsForForwarding() { - Bun__currentSyncPID = 0; - std::lock_guard<std::mutex> lock(signalForwardingLock); if (--signalForwardingDepth != 0) return; + + Bun__currentSyncPID = 0; `#define` UNREGISTER_SIGNAL(SIG) \ if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \ }🤖 Prompt for 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. In `@src/jsc/bindings/c-bindings.cpp` around lines 1016 - 1022, The code resets Bun__currentSyncPID before checking signalForwardingDepth, which clears the forwarded PID prematurely for nested spawnSync calls; modify Bun__unregisterSignalsForForwarding so it acquires signalForwardingLock, decrements signalForwardingDepth, and only when signalForwardingDepth reaches 0 does it clear Bun__currentSyncPID and return—leave Bun__currentSyncPID unchanged when depth stays > 0 to ensure outer calls still forward signals.
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/c-bindings.cpp`:
- Around line 1016-1022: The code resets Bun__currentSyncPID before checking
signalForwardingDepth, which clears the forwarded PID prematurely for nested
spawnSync calls; modify Bun__unregisterSignalsForForwarding so it acquires
signalForwardingLock, decrements signalForwardingDepth, and only when
signalForwardingDepth reaches 0 does it clear Bun__currentSyncPID and
return—leave Bun__currentSyncPID unchanged when depth stays > 0 to ensure outer
calls still forward signals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 465df2fa-68f7-42bb-8adf-a435cfd0251b
📒 Files selected for processing (1)
src/jsc/bindings/c-bindings.cpp
|
CI on 7e2c283 (the reentrancy fix): 73/74 checks pass. The one failure is No test failures on this build; only auto-retried flakes in the warning annotations ( Already used my one retrigger on build 55649; not pushing another. The expired darwin-14-aarch64 job just needs a retry. |
|
Also fixes fuzzer fingerprint |
7e2c283 to
db5ce58
Compare
robobun
changed the title
| Bun__currentSyncPID = 0; | ||
|
|
||
| std::lock_guard<std::mutex> lock(signalForwardingLock); | ||
| if (--signalForwardingDepth != 0) | ||
| return; |
There was a problem hiding this comment.
🟡 Minor: Bun__currentSyncPID = 0; runs before the new lock + --signalForwardingDepth != 0 early-return, so an inner unregister (e.g. an openInEditor thread finishing while a main-thread spawnSync is still waiting) zeroes the outer caller's forwarding PID even though the depth guard is meant to make inner calls no-ops. You've already noted Bun__currentSyncPID remains a shared singleton, and moving this line below the guard trades a zeroed PID for a potentially stale (reaped) one in the opposite interleaving — so probably worth a one-line comment on why the reset is intentionally outside the guard rather than a code change.
Extended reasoning...
What this is
Bun__unregisterSignalsForForwarding() now does:
Bun__currentSyncPID = 0; // line 1019 — unconditional
std::lock_guard<std::mutex> lock(signalForwardingLock);
if (--signalForwardingDepth != 0)
return; // inner caller: no-opThe depth counter added in db5ce58 is meant to make nested register/unregister pairs no-ops so only the outermost pair touches process-wide state. But the PID reset on line 1019 sits above both the lock and the depth check, so every caller — inner or outer — zeroes Bun__currentSyncPID.
Concrete walk-through (the ordering where this bites)
Bun.openInEditordetached thread:Bun__currentSyncPID.store(0)(process.rs:3138) →register()(depth 0→1, installs handlers) → spawn editor →store(editor_pid)→ block inwaitpid.- Main thread
Bun.spawnSync:store(0)→register()(depth 1→2, early-return) → spawn child →store(child_pid)(process.rs:3152) → block inwaitpid. - Editor exits; detached thread's
SignalForwardingguard drops →Bun__unregisterSignalsForForwarding()→ line 1019 setsBun__currentSyncPID = 0→ depth 2→1 ≠ 0 → return. - User presses Ctrl-C. The forwarding lambda sees
Bun__currentSyncPID == 0and stashes the signal inBun__pendingSignalToSendinstead ofkill(child_pid, SIGINT). WithSA_RESETHAND, the disposition is nowSIG_DFL, so a second Ctrl-C kills bun without the child ever receiving the signal.
If line 1019 were below the depth check, step 3 would leave child_pid in place and step 4 would forward correctly.
Why this isn't already prevented
The new comment at lines 912-916 says "only the outermost pair touches process-wide signal dispositions" — and that's accurate for the sigaction calls and previous_actions[]. But Bun__currentSyncPID is also process-wide state read by the forwarding lambda, and it's reset outside the guard. Nothing else protects it: both Rust (process.rs:3138/3152) and Zig (process.zig:2396/2409) callers write it directly without checking depth.
Addressing the counter-argument
There's a reasonable case that the current placement is intentional. In the opposite interleaving (main-thread spawnSync registers first, openInEditor registers second), the inner caller has already overwritten the PID with 0 then editor_pid on its register path — so by the time the inner unregister runs, the outer PID is gone regardless. Moving line 1019 below the guard would then leave Bun__currentSyncPID == editor_pid (a freshly-reaped PID), and a subsequent signal would kill() a PID the kernel may have recycled. Zeroing it (current behavior) is the safer failure mode for that ordering: the signal is stashed rather than mis-delivered.
So neither placement is correct for all interleavings — that's the acknowledged singleton limitation, and a real fix needs per-caller PID tracking (out of scope here). The point of this comment is narrower: the placement looks like an oversight relative to the depth guard added two lines below it, and there is at least one realistic ordering (steps 1-4 above) where it discards a still-valid PID that moving it would preserve.
Suggested action
Given the stale-PID tradeoff, I'd lean toward leaving the code as-is and adding a one-line comment above line 1019 noting that the reset is deliberately outside the depth guard (zero is safer than a possibly-reaped inner PID). Alternatively, drop line 1019 entirely — every caller already does store(0) immediately before register() (process.rs:3138, process.zig:2396), so the outermost unregister doesn't need it, and removing it fixes the step-3 ordering above without introducing the stale-PID case.
There was a problem hiding this comment.
Yes — the reset is intentionally outside the depth guard. With overlapping callers the PID slot is already a last-writer-wins singleton, so by the time an inner unregister runs, the slot usually holds the inner (now-reaped) child anyway. Zeroing it makes the handler fall back to the pending-signal path instead of kill()ing a possibly-recycled PID, which is the safer failure mode. A real fix is per-caller PID tracking, which is out of scope here. Leaving the code as-is; happy to add the one-line comment if a maintainer prefers it inline.
|
CI on db5ce58 (rebased onto main): 67 pass, 3 fail, 4 pending. The failing jobs ( The changes in this PR (signal-forwarding mutex/depth guard, SIGIOT/SIGPOLL alias removal, regression test) passed everywhere they run, including Already used my one retrigger earlier; not pushing another. The 8254 OOM flake needs either a job retry or a fix to that test's memory footprint (separate issue). |
|
Triage note (sweep of open PRs whose tests pass on main): this PR's test passes on current main (94898e0), but it checks the SIGPWR symptom, which 832edbc (#31183) fixed by dropping SIGPWR from the list, so it no longer distinguishes main from this branch. Of the rest of this PR, the SIGIOT/SIGPOLL alias removal also landed separately in c0245e8 (#36711); the mutex + depth counter around |
JSC uses SIGPWR for GC thread suspend/resume on Linux. spawnSync's signal-forwarding helper installed a SA_RESETHAND handler for SIGPWR and saved the previous handler into a process-global array with no synchronization. Bun.openInEditor spawns a detached thread per call that runs spawnSync; concurrent calls raced on the previous-handler table and left SIGPWR at SIG_DFL, so the next GC suspend terminated the process with signal 30.
Bun.openInEditor spawns detached threads that call spawnSync, so register/unregister can run concurrently. Guard previous_actions[] with a mutex and depth counter so only the outermost pair installs and restores handlers. Also drop SIGIOT/SIGPOLL from the list since they alias SIGABRT/SIGIO and the second sigaction() was overwriting the saved handler with our own.
db5ce58 to
ed97b02
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // bun.spawnSync is primarily used from the main thread (e.g. `bun run`), but | ||
| // Bun.openInEditor spawns detached threads that also go through this path. | ||
| // The depth counter + lock below keep previous_actions[] from being corrupted | ||
| // by overlapping register/unregister calls; only the outermost pair touches | ||
| // process-wide signal dispositions. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Rebased onto main. Main has since absorbed two pieces that were previously in this PR: the SIGPWR removal (#31183) and the SIGIOT/SIGPOLL alias cleanup (#36711), so the conflict resolution was just dropping my copies of those in favor of main's. What's left here is the mutex/depth guard around register/unregister and the concurrency regression test. Also trimmed the block comment down to one line per the comment-cop note. After rebase: |
The previous test only checked the process survived GC, which main already guarantees since SIGPWR is no longer forwarded. Install a SIGUSR2 listener, burst 64 overlapping editor spawns, then deliver SIGUSR2 and check the listener still runs. Without the depth guard the unregister burst leaves SIGUSR2 at SIG_DFL and the child is killed.
reset_on_posix() reinstalls SIGABRT/SIGTRAP, both of which are in the forwarded set, so an inner caller dropping its guard was replacing the forwarding handler while an outer spawnSync was still waiting. Have Bun__unregisterSignalsForForwarding report whether it restored the table and gate the crash-handler reset on that.
| /// Returns true when this was the outermost call and the previous | ||
| /// dispositions were restored. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Waiting a fixed 500ms for 64 detached threads to unregister could time out on slow CI, and a single delivery during the burst goes to the forwarding handler. Send inside the poll loop instead.
…ister The outermost unregister already restores SIGABRT/SIGTRAP from previous_actions[], and the other crash signals are never touched, so reset_on_posix() here re-armed nothing. It also ran after the lock was released and could overwrite a forwarding handler a fresh register() had just installed on another thread. Removing it makes the bool return from the previous commit unnecessary.
| //! and `bun_threading` — none of which depend back on this crate, so no | ||
| //! cycle. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
This is the pre-existing crate doc listing bun_spawn's dependencies; the only change here is removing bun_crash_handler from that list since c1d46cb drops the dependency. Leaving the rest as-is.
These comments cited the Bun__currentSyncPID note, which now says the opposite; the tracker is actually gated on pdeathsig::is_arming_thread.
| // All state is process-global; this is only reached from the thread that | ||
| // armed the parent-death watchdog (pdeathsig::is_arming_thread), so no locking. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Pre-existing header comment; the edit here only corrects a cross-reference that this PR made stale (it pointed at the Bun__currentSyncPID note, which now says the opposite). It documents why the file has no locking, which matters more now that the sibling signal-forwarding code does.
| // thread-safe per C++11 [stmt.dcl]. Only the arming thread gets here | ||
| // anyway, but this keeps the binary's static-init section clean. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Trimmed to one line in 289bb83; the threading clause was redundant with the file header.
There was a problem hiding this comment.
All of my earlier feedback has been addressed (WTF::Lock swap, dropping the redundant reset_on_posix() and the residual outermost race it created, the SIGUSR2 re-delivery loop, the NoOrphansTracker cross-reference, and the 3000ms poll ceiling in be93485). This run's bug hunt found nothing new. Because the net change swaps process-wide signal dispositions under a new lock and removes the crash-handler re-arm from SignalForwarding::drop, a maintainer sign-off would still be worthwhile.
What was reviewed:
- Depth-guarded
WTF::LockaroundBun__registerSignalsForForwarding/unregister— only the outermost pair touchesprevious_actions[];Bun__currentSyncPID = 0intentionally stays outside (zeroing is safer than kill() on a possibly-recycled PID, per the earlier thread). reset_on_posix()removal — SIGSEGV/ILL/BUS/FPE are not inFOR_EACH_SIGNAL, and SIGABRT/SIGTRAP are restored fromprevious_actions[]by the outermost unregister, so nothing is left un-rearmed;bun_spawnlosing thebun_crash_handlerdep is consistent.- New Linux-only test polls SIGUSR2 delivery until observed (no fixed sleep), bails on zero spawns, and its inner deadline now fits under the default test timeout.
Extended reasoning...
Overview
The PR guards the process-global previous_actions[] signal table in c-bindings.cpp with a WTF::Lock + depth counter so overlapping spawnSync callers (notably Bun.openInEditor's detached threads) can't corrupt each other's saved dispositions. It also drops bun_crash_handler::reset_on_posix() from SignalForwarding::drop (and the bun_crash_handler dep from bun_spawn), updates two stale cross-reference comments in NoOrphansTracker.cpp, and adds a Linux-only regression test.
Security risks
No new attack surface. The forwarded-signal set is unchanged; the change only serializes access to existing process-global state. Removing reset_on_posix() was verified redundant: the four crash-only signals (SEGV/ILL/BUS/FPE) are never in the forwarded set so their handlers are never disturbed, and SIGABRT/SIGTRAP are restored from previous_actions[] by the outermost unregister — confirmed by robobun's SigCgt diff and my own trace in the earlier review.
Level of scrutiny
High. This is process-wide POSIX signal handling that runs on detached threads and interacts with the crash handler. I've reviewed it across six rounds and each concern (inner-caller reset_on_posix stomp, outermost-vs-fresh-register race after lock release, std::mutex vs WTF::Lock, test flake from single-shot SIGUSR2, NoOrphansTracker comment drift, inner-deadline ≥ outer-timeout) was fixed. The remaining known limitation — Bun__currentSyncPID is a last-writer-wins singleton — was explicitly discussed and is out of scope here; zeroing it before the depth check is the safer failure mode.
Other factors
A maintainer (Jarred-Sumner) directed the reentrancy-guard shape early on. The test was verified 5/5 fail-without / 5/5 pass-with by swapping in main's source files. The final code delta is ~15 substantive lines. Given the subject matter (signal disposition management + crash-handler interaction), I'm deferring rather than approving so a human can confirm the reset_on_posix() removal and the depth-guard placement.
There was a problem hiding this comment.
I reviewed this PR and didn't find any new issues — all prior review feedback (WTF::Lock swap, reset_on_posix() removal, NoOrphansTracker cross-reference, test poll-loop and deadline) has been addressed. Because it changes process-wide signal disposition handling and removes the crash-handler re-arm from SignalForwarding::Drop, a human look would still be worthwhile.
What was reviewed:
- Depth-counted
WTF::Lockguard aroundprevious_actions[]— only the outermost pair installs/restores; inner callers early-return. reset_on_posix()removal: verified SIGSEGV/ILL/BUS/FPE are not inFOR_EACH_SIGNALand SIGABRT/SIGTRAP are restored fromprevious_actions[], so nothing is left un-armed; also closes the post-lock race with a fresh register.Bun__currentSyncPID = 0staying above the guard was examined and left as the safer failure mode (zero vs. possibly-recycled inner PID); acknowledged singleton limitation.- New Linux-only test polls with re-delivery inside a 3s deadline; verified fail-without-fix / pass-with-fix per the PR notes.
Extended reasoning...
Overview
This PR guards Bun__registerSignalsForForwarding / Bun__unregisterSignalsForForwarding in src/jsc/bindings/c-bindings.cpp with a WTF::Lock + depth counter so overlapping spawnSync callers (notably Bun.openInEditor's detached threads) don't corrupt the process-global previous_actions[NSIG] table. It also removes bun_crash_handler::reset_on_posix() from SignalForwarding::Drop (and drops the bun_crash_handler dep from bun_spawn), updates two stale cross-reference comments in NoOrphansTracker.cpp, and adds a Linux-only regression test to open-in-editor-gc.test.ts. Cargo.lock reflects the removed dependency.
Security risks
None identified. The change narrows a race that could leave forwarded signals at SIG_DFL; it does not add new attack surface, parse untrusted input, or touch auth/crypto. The signal set is unchanged from main (SIGPWR was already excluded via #31183; SIGIOT/SIGPOLL alias cleanup landed via #36711).
Level of scrutiny
High. Process-wide sigaction state is shared across every thread and every subsystem (JSC's GC suspend, the crash handler, TTY restore, user process.on listeners). The PR removes a call that was added deliberately in 2024 — the analysis that it's now redundant (crash-handler signals either not forwarded, or restored from previous_actions[]) is sound, and the author verified /proc/self/status SigCgt is identical before/after, but this is exactly the kind of deletion REVIEW.md flags for a maintainer to confirm ("before deleting odd-looking code, git-blame why it was written").
Other factors
This PR has been through six rounds of automated review, each addressed. The remaining known limitation — Bun__currentSyncPID is still a last-writer-wins singleton across concurrent callers — was discussed on 2026-05-23 and intentionally left out of scope (zeroing before the depth check is the safer failure mode vs. a possibly-recycled PID). The only human input so far was a "@robobun fix" that requested the reentrancy guard; a maintainer has not yet signed off on the final shape (in particular the reset_on_posix() removal and the singleton-PID scope decision). Several github-actions comment-cop threads remain technically unresolved but are pre-existing crate/file header docs, not new workarounds. Given the criticality of the code path, deferring to a human rather than shadow-approving.
2 participants
Fuzzer fingerprint:
11551e051c08db2cWhat
Bun__registerSignalsForForwarding()/Bun__unregisterSignalsForForwarding()with aWTF::Lockand depth counter so only the outermost register/unregister pair installs and restores process-wide signal handlers.crash_handler::reset_on_posix()call fromSignalForwarding'sDrop. The outermost unregister already restores SIGABRT/SIGTRAP fromprevious_actions[]and the remaining crash signals are never touched by the forwarding set, so it re-armed nothing; it also ran after the lock was released and could overwrite a forwarding handler a freshregister()on another thread had just installed.bun_spawnno longer depends onbun_crash_handler.Why
The signal-forwarding helpers assume they run on the main thread only (
previous_actions[]is an unsynchronized process-global), butBun.openInEditorrunsspawnSyncon detached threads. With a burst of overlapping calls, the first register saves the real handler and every later one saves our own forwarding handler; on the way out the first unregister zeroes the table and every later one installs that zeroed entry, so forwarded signals (SIGINT,SIGTERM,SIGUSR2, ...) end up atSIG_DFLand any handler installed before the burst (Bun's own or a user'sprocess.on(...)) is gone.The worst symptom of this race, losing JSC's
SIGPWRGC suspend handler and dying with signal 30, was fixed on main by removingSIGPWRfrom the list (#31183, same fuzzer crash family). TheSIGIOT/SIGPOLLalias cleanup that was briefly in this PR landed via #36711. What remains here is the reentrancy guard requested in review.Test
Added to
test/js/bun/util/open-in-editor-gc.test.ts(Linux-only): install aprocess.on("SIGUSR2")listener, burst 64 overlappingBun.openInEditorcalls againstsleep, then keep deliveringSIGUSR2until the listener runs or a deadline passes.Verified by swapping in main's copies of the touched source files: without the guard the child is killed by
SIGUSR2(5/5 runs); with it the listener fires (5/5 runs)./proc/self/statusSigCgtis identical before and after aspawnSyncwith the crash-handler reset removed, andtest/regression/issue/14799.test.ts(#36711) still passes.no test proof · iteration 9 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/open-in-editor-gc.test.ts