Skip to content

spawnSync: make signal-forwarding register/unregister safe for concurrent callers #30956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 5 commits into
base: main
Choose a base branch
from
+69 −3
Open

spawnSync: make signal-forwarding register/unregister safe for concurrent callers #30956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
062df75
spawnSync: do not forward SIGPWR on Linux
robobun May 18, 2026
df6ebae
[autofix.ci] apply automated fixes
autofix-ci[bot] May 18, 2026
9179fad
ci: retrigger
robobun May 18, 2026
75096d0
test: drop negative crash-marker assertion
robobun May 18, 2026
db5ce58
spawnSync: make signal-forwarding register/unregister reentrant
robobun May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/jsc/bindings/c-bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,14 +908,23 @@
#if OS(LINUX) || OS(DARWIN) || OS(FREEBSD)
#include <signal.h>
#include <pthread.h>
#include <mutex>

// Note: We only ever use bun.spawnSync on the main thread.
// 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.
extern "C" int64_t Bun__currentSyncPID = 0;
static int Bun__pendingSignalToSend = 0;
static struct sigaction previous_actions[NSIG];
static std::mutex signalForwardingLock;
static int signalForwardingDepth = 0;

// This list of signals is copied from npm.
// https://github.com/npm/cli/blob/fefd509992a05c2dfddbe7bc46931c42f1da69d7/workspaces/arborist/lib/signals.js#L26-L57
// SIGIOT is omitted because it aliases SIGABRT; registering both would
// overwrite previous_actions[SIGABRT] with our own handler.
#define FOR_EACH_POSIX_SIGNAL(M) \
M(SIGABRT); \
M(SIGALRM); \
Expand All @@ -929,16 +938,15 @@
M(SIGTRAP); \
M(SIGSYS); \
M(SIGQUIT); \
M(SIGIOT); \
M(SIGIO);

#if OS(LINUX)
// SIGPWR is intentionally excluded: JSC uses it for GC thread suspend/resume
// (see wtf/posix/ThreadingPOSIX.cpp). Overriding it here breaks GC and the
// SA_RESETHAND disposition leaves it at SIG_DFL after one delivery, which
// kills the process on the next collection.
// SIGPOLL is omitted because it aliases SIGIO (see SIGIOT note above).
#define FOR_EACH_LINUX_ONLY_SIGNAL(M) \
M(SIGPOLL); \
M(SIGSTKFLT);
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.

#endif
Expand Down Expand Up @@ -979,6 +987,10 @@

extern "C" void Bun__registerSignalsForForwarding()
{
std::lock_guard<std::mutex> lock(signalForwardingLock);
if (signalForwardingDepth++ != 0)
return;

Bun__pendingSignalToSend = 0;
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
Expand All @@ -1004,8 +1016,12 @@

extern "C" void Bun__unregisterSignalsForForwarding()
{
Bun__currentSyncPID = 0;

std::lock_guard<std::mutex> lock(signalForwardingLock);
if (--signalForwardingDepth != 0)
return;

Check warning on line 1023 in src/jsc/bindings/c-bindings.cpp

View check run for this annotation

Claude / Claude Code Review

Inner unregister zeros Bun__currentSyncPID before depth guard 

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
Comment on lines 1019 to +1023
Copy link
Copy Markdown
Contributor

@claude claude Bot May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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-op

The 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)

  1. Bun.openInEditor detached thread: Bun__currentSyncPID.store(0) (process.rs:3138) → register() (depth 0→1, installs handlers) → spawn editor → store(editor_pid) → block in waitpid.
  2. Main thread Bun.spawnSync: store(0) → register() (depth 1→2, early-return) → spawn child → store(child_pid) (process.rs:3152) → block in waitpid.
  3. Editor exits; detached thread's SignalForwarding guard drops → Bun__unregisterSignalsForForwarding() → line 1019 sets Bun__currentSyncPID = 0 → depth 2→1 ≠ 0 → return.
  4. User presses Ctrl-C. The forwarding lambda sees Bun__currentSyncPID == 0 and stashes the signal in Bun__pendingSignalToSend instead of kill(child_pid, SIGINT). With SA_RESETHAND, the disposition is now SIG_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.

Copy link
Copy Markdown
Collaborator Author

@robobun robobun May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#define UNREGISTER_SIGNAL(SIG) \
if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \
}
Expand Down
50 changes: 50 additions & 0 deletions test/js/bun/spawn/spawnSync-sigpwr-gc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { expect, test } from "bun:test";
import { symlinkSync } from "fs";
import { bunEnv, bunExe, isLinux, tempDir } from "harness";
import { join } from "path";

// Regression: spawnSync's signal-forwarding list included SIGPWR on Linux.
// JSC uses SIGPWR to suspend/resume threads for GC. Bun.openInEditor spawns a
// detached thread per call that runs spawnSync; concurrent calls race on the
// process-wide previous-handler table and can leave SIGPWR at SIG_DFL. The
// next GC suspend then terminates the process with signal 30.
test.skipIf(!isLinux)("spawnSync signal forwarding does not clobber JSC's SIGPWR handler", async () => {
const sleepBin = Bun.which("sleep");
expect(sleepBin).toBeTruthy();

using dir = tempDir("spawnSync-sigpwr", {
"run.js": `
for (let i = 0; i < 64; i++) {
try { Bun.openInEditor("0.2"); } catch {}
}
let junk = [];
for (let i = 0; i < 2000; i++) {
junk.push({ a: new Uint8Array(4096).fill(i), b: { c: i } });
}
for (let i = 0; i < 30; i++) Bun.gc(true);
console.log("ok");
`,
});
// Make `code` (first in the editor preference list) resolve to `sleep`, so
// each background spawnSync holds its signal-forwarding window open long
// enough for threads to overlap.
symlinkSync(sleepBin!, join(String(dir), "code"));

await using proc = Bun.spawn({
cmd: [bunExe(), join(String(dir), "run.js")],
env: {
...bunEnv,
PATH: String(dir),
EDITOR: "",
VISUAL: "",
},
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);
Comment on lines +41 to +49
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

});
Loading