Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
13 changes: 12 additions & 1 deletion src/jsc/bindings/c-bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -937,11 +937,14 @@ extern "C" int ffi_fileno(FILE* file)
#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.openInEditor runs spawnSync on detached threads, so register/unregister can overlap.
Comment thread
claude[bot] marked this conversation as resolved.
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;
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

// npm's signal list minus SIGIOT/SIGPOLL (aliases of SIGABRT/SIGIO; listing both would overwrite previous_actions[N]).
// https://github.com/npm/cli/blob/fefd509992a05c2dfddbe7bc46931c42f1da69d7/workspaces/arborist/lib/signals.js#L26-L57
Expand Down Expand Up @@ -1004,6 +1007,10 @@ extern "C" void Bun__sendPendingSignalIfNecessary()

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 Down Expand Up @@ -1032,6 +1039,10 @@ extern "C" void Bun__unregisterSignalsForForwarding()
{
Bun__currentSyncPID = 0;

std::lock_guard<std::mutex> lock(signalForwardingLock);
if (--signalForwardingDepth != 0)
return;
Comment on lines 1040 to +1044

Copy link
Copy Markdown
Contributor

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

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.

Comment thread
claude[bot] marked this conversation as resolved.
Outdated

#define UNREGISTER_SIGNAL(SIG) \
if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \
}
Expand Down
47 changes: 47 additions & 0 deletions test/js/bun/util/open-in-editor-gc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,50 @@

await Promise.all(runs);
});

// Each detached editor thread goes through bun.spawnSync's signal-forwarding
// register/unregister, which swaps every forwarded signal's disposition in a
// process-global table. With many threads overlapping, an unsynchronized
// table ends up restoring SIG_DFL (or the forwarding handler itself) instead
// of the handler that was installed before the burst, so a JS signal
// listener registered beforehand silently stops working or kills the process.
test.skipIf(!isLinux)("Bun.openInEditor bursts do not drop previously installed signal handlers", async () => {
const sleep = ["/usr/bin/sleep", "/bin/sleep"].find(p => existsSync(p));
expect(sleep).toBeDefined();

using dir = tempDir("open-in-editor-signal-table", {
"run.js": `
let fired = false;
process.on("SIGUSR2", () => { fired = true; });

let spawned = 0;
for (let i = 0; i < 64; i++) {
try { Bun.openInEditor("0.1", { editor: ${JSON.stringify(sleep)} }); spawned++; } catch {}
}
if (spawned === 0) { console.log("no editor threads spawned"); process.exit(2); }

// Let every detached thread finish its sleep and unregister.
await Bun.sleep(500);

Check failure on line 178 in test/js/bun/util/open-in-editor-gc.test.ts

View check run for this annotation

Claude / Claude Code Review

Fixed Bun.sleep(500) may not outlast 64 editor threads under debug/ASAN, causing false 'handler lost' failures

The fixed `await Bun.sleep(500)` may not outlast all 64 detached editor threads on debug/ASAN CI — if any thread has not yet unregistered, `signalForwardingDepth > 0` so the forwarding lambda (not the JS listener) receives the single SIGUSR2, and the test prints `handler lost` even though the fix works. The neighboring test in this file uses 1000ms for only 8 spawns; move `process.kill(process.pid, "SIGUSR2")` inside the poll loop so the signal is re-delivered until `fired` or the deadline, inst
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

process.kill(process.pid, "SIGUSR2");
const deadline = Date.now() + 2000;
while (!fired && Date.now() < deadline) await Bun.sleep(5);
console.log(fired ? "ok" : "handler lost");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "run.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout.trim()).toBe("ok");
expect(proc.signalCode).toBeNull();
expect(exitCode).toBe(0);
});
Loading