Skip to content
Open
Show file tree
Hide file tree
Changes from 11 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
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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 @@
#if OS(LINUX) || OS(DARWIN) || OS(FREEBSD)
#include <signal.h>
#include <pthread.h>
#include <wtf/Lock.h>

// Note: We only ever use bun.spawnSync on the main thread.
// Bun.openInEditor runs spawnSync on detached threads, so register/unregister can overlap.

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

View check run for this annotation

Claude / Claude Code Review

Stale NoOrphansTracker.cpp cross-references now point at contradicting Bun__currentSyncPID comment

nit: this rewrites the `Bun__currentSyncPID` comment from "only ever … on the main thread" to "runs spawnSync on detached threads, so register/unregister can overlap", but `NoOrphansTracker.cpp:29-30` and `:68-69` still say "spawnSync is single-threaded by design (see Bun__currentSyncPID), so no locking" — following that cross-reference now lands on the opposite claim. NoOrphansTracker's actual invariant still holds (it's gated on `is_arming_thread()` at process.rs:3013-3014, not on this comment
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 WTF::Lock signalForwardingLock;
static int signalForwardingDepth = 0;

// 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__registerSignalsForForwarding()
{
WTF::Locker<WTF::Lock> locker(signalForwardingLock);
if (signalForwardingDepth++ != 0)
return;

Bun__pendingSignalToSend = 0;
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
Expand Down Expand Up @@ -1032,6 +1039,10 @@
{
Bun__currentSyncPID = 0;

WTF::Locker<WTF::Lock> locker(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.


#define UNREGISTER_SIGNAL(SIG) \
if (sigaction(SIG, &previous_actions[SIG], NULL) == -1) { \
}
Expand Down
1 change: 0 additions & 1 deletion src/spawn/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ scopeguard.workspace = true
bun_analytics.workspace = true
bun_dispatch.workspace = true
bun_core.workspace = true
bun_crash_handler.workspace = true
bun_event_loop.workspace = true
bun_io.workspace = true
bun_output.workspace = true
Expand Down
4 changes: 2 additions & 2 deletions src/spawn/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
//! `bun_runtime` re-exports them. The only non-leaf dependencies are
//! `bun_io` (`FilePoll`/`KeepAlive`/`EventLoopCtx`), `bun_ptr`
//! (`ThreadSafeRefCount`), `bun_io` (`BufferedWriter`), `bun_event_loop`,
//! `bun_threading`, and `bun_crash_handler` — none of which depend back on
//! this crate, so no cycle.
//! and `bun_threading` — none of which depend back on this crate, so no
//! cycle.
Comment on lines +12 to +13

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

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.

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.


use core::ffi::c_char;

Expand Down
4 changes: 1 addition & 3 deletions src/spawn/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2853,8 +2853,7 @@ mod spawn_process_body {
};

/// RAII guard around `Bun__registerSignalsForForwarding`: registers on
/// construction, unregisters and restores the crash-handler signal
/// disposition on drop.
/// construction, unregisters on drop.
#[cfg(unix)]
struct SignalForwarding;
#[cfg(unix)]
Expand All @@ -2869,7 +2868,6 @@ mod spawn_process_body {
impl Drop for SignalForwarding {
fn drop(&mut self) {
Bun__unregisterSignalsForForwarding();
bun_crash_handler::reset_on_posix();
}
Comment thread
claude[bot] marked this conversation as resolved.
}

Expand Down
51 changes: 51 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,54 @@ test.skipIf(!isLinux)("Bun.openInEditor does not break GC signal handling", asyn

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); }

// The editor children each run for 100ms, so by now the burst has
// started and the forwarding handler owns SIGUSR2; the original handler
// only comes back once the last thread unregisters. Keep delivering
// until it does (or the table was corrupted and it never does).
await Bun.sleep(50);
const deadline = Date.now() + 5000;
while (!fired && Date.now() < deadline) {
process.kill(process.pid, "SIGUSR2");
await Bun.sleep(20);
}
Comment thread
claude[bot] marked this conversation as resolved.
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