Don't forward SIGPWR in spawnSync signal handling on Linux - #31104
Closed
robobun wants to merge 1 commit into
Closed
Don't forward SIGPWR in spawnSync signal handling on Linux#31104robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Claude / Claude Code Review
completed
May 19, 2026 in 18m 3s
Code review found 4 potential issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 2 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/bun/util/openInEditor-gc.test.ts:26-31 |
Test ignores stderr, leaving no diagnostics on unrelated exitCode failure |
| 🟣 Pre-existing | src/jsc/bindings/c-bindings.cpp:936-938 |
previous_actions[] race remains for other signals; main-thread-only comment now known false |
| 🟣 Pre-existing | src/jsc/bindings/c-bindings.cpp:939-941 |
SIGPOLL/SIGIO and SIGIOT/SIGABRT aliases self-overwrite previous_actions[] |
Annotations
Check warning on line 31 in test/js/bun/util/openInEditor-gc.test.ts
claude / Claude Code Review
Test ignores stderr, leaving no diagnostics on unrelated exitCode failure
nit: Consider piping `stderr` (and surfacing it before the `exitCode` assertion) instead of `"ignore"`. Per the repo convention in CLAUDE.md, spawned-process tests should print/assert output before asserting `exitCode` so that an unrelated failure (e.g. a debug assertion or future `openInEditor` regression) doesn't reduce to an opaque "expected 0, received 1" in CI. The `signalCode` assertion already covers the SIGPWR case nicely, but stderr-on-failure would help debug everything else.
Check notice on line 938 in src/jsc/bindings/c-bindings.cpp
claude / Claude Code Review
previous_actions[] race remains for other signals; main-thread-only comment now known false
Worth noting as a follow-up: the PR description correctly identifies that `Bun.openInEditor()` runs `sync::spawn` on detached threads, which means the "We only ever use bun.spawnSync on the main thread" comment at line 912 is now known to be false, and the underlying race on `previous_actions[]` + the unconditional `memset` still applies to every other signal in `FOR_EACH_SIGNAL` (e.g. the `SIGINT`/`SIGTERM` → `onExitSignal` handler that restores TTY state on Ctrl-C, or any user `process.on('SIG
Check notice on line 941 in src/jsc/bindings/c-bindings.cpp
claude / Claude Code Review
SIGPOLL/SIGIO and SIGIOT/SIGABRT aliases self-overwrite previous_actions[]
Pre-existing, but since this PR is auditing this exact list: `SIGPOLL` is an alias for `SIGIO` (29) on Linux, and `SIGIOT` is an alias for `SIGABRT` (6). Because `REGISTER_SIGNAL` runs twice for the same numeric signal, the second `sigaction()` saves the just-installed forwarder into `previous_actions[N]`, so unregister "restores" the forwarder instead of the real prior handler — same handler-clobbering class as the SIGPWR bug. Dropping `M(SIGPOLL)` here (and `M(SIGIOT)` from `FOR_EACH_POSIX_SIG
Loading