From b7d4cd27857f4c15340604f0c546de32907c2592 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 24 May 2026 02:12:12 +0000 Subject: [PATCH 1/4] Spawn the editor from Bun.openInEditor without spawnSync's signal forwarding The detached editor thread went through sync::spawn (bun.spawnSync), whose signal-forwarding setup (Bun__registerSignalsForForwarding / previous_actions / Bun__currentSyncPID) is process-global and only safe on the main thread. Concurrent Bun.openInEditor calls raced on it and flipped the dispositions of unrelated signals process-wide. Use the minimal spawn+wait helper instead, matching the original std.process.Child behavior. --- src/runtime/cli/open.rs | 35 ++--------- test/js/bun/util/open-in-editor-gc.test.ts | 69 +++++++++++++++++++++- 2 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/runtime/cli/open.rs b/src/runtime/cli/open.rs index 26c0dac269cc..23f357cad5a7 100644 --- a/src/runtime/cli/open.rs +++ b/src/runtime/cli/open.rs @@ -7,8 +7,6 @@ use bun_paths::{self, MAX_PATH_BYTES, PathBuffer}; use bun_resolver::fs as Fs; use bun_which::which; -use crate::api::bun::process::sync; - // ────────────────────────────────────────────────────────────────────────── #[cfg(target_os = "macos")] @@ -415,33 +413,12 @@ fn auto_close(spawned: *mut SpawnedEditorContext) { argv[j] = unsafe { bun_core::ffi::slice(p, l) }; } - // FIXME(windows-leak): the sync::spawn path - // requires a `WindowsOptions.loop_`; `MiniEventLoop::init_global` heap-allocates a - // MiniEventLoop + uv_loop_t into a thread-local that is NEVER torn down. Because this - // runs on a fresh detached std::thread per `Editor::open()` call, every editor-open on - // Windows leaks one MiniEventLoop + uv_loop_t (+ DotEnv Loader/Map if env was null). - // Proper fix needs either (a) a MiniEventLoop teardown helper (none exists today), or - // (b) plumbing the caller's existing EventLoopHandle through SpawnedEditorContext - // (signature change to Editor::open + callers). Both are out-of-scope for this file. - let owned_argv: Vec> = argv[0..spawned.argc] - .iter() - .map(|s| s.to_vec().into_boxed_slice()) - .collect(); - let _ = sync::spawn(&sync::Options { - argv: owned_argv, - envp: None, - stderr: sync::SyncStdio::Inherit, - stdout: sync::SyncStdio::Inherit, - stdin: sync::SyncStdio::Inherit, - #[cfg(windows)] - windows: crate::api::bun::process::WindowsOptions { - loop_: bun_jsc::EventLoopHandle::init_mini(bun_event_loop::MiniEventLoop::init_global( - None, None, - )), - ..Default::default() - }, - ..Default::default() - }); + // Zig called `child_process.spawn()` then `.wait()` via std.process.Child. Use the + // minimal spawn+wait helper here, NOT `sync::spawn` (bun.spawnSync): spawnSync's + // signal-forwarding setup mutates process-wide state (`Bun__registerSignalsForForwarding`, + // `Bun__currentSyncPID`) that is only safe on the main thread, and this runs on a + // detached thread per `Editor::open()` call. + let _ = bun_core::util::spawn_sync_inherit(&argv[0..spawned.argc]); } // ────────────────────────────────────────────────────────────────────────── diff --git a/test/js/bun/util/open-in-editor-gc.test.ts b/test/js/bun/util/open-in-editor-gc.test.ts index d63a41ee700b..818f8e761e61 100644 --- a/test/js/bun/util/open-in-editor-gc.test.ts +++ b/test/js/bun/util/open-in-editor-gc.test.ts @@ -3,8 +3,75 @@ import { bunEnv, bunExe, isLinux, tempDir } from "harness"; import { existsSync, symlinkSync } from "node:fs"; import { join } from "node:path"; +// The detached editor thread spawned by Bun.openInEditor must not mutate process-wide +// signal state. It used to run bun.spawnSync's signal-forwarding setup, which is only +// safe on the main thread: concurrent openInEditor calls raced on the shared +// previous_actions[] array and flipped unrelated signal dispositions process-wide +// (installing a one-shot forwarding handler, then resetting them to SIG_DFL), which can +// get the process killed by a stray signal while the GC suspend signal (SIGPWR) is in +// flight. Sample the process's caught-signal mask (SigCgt in /proc/self/status) while +// hammering openInEditor and assert it never changes. +test.skipIf(!isLinux)("concurrent Bun.openInEditor calls do not touch process signal handlers", async () => { + const sleep = ["/usr/bin/sleep", "/bin/sleep"].find(p => existsSync(p)); + expect(sleep).toBeDefined(); + + using dir = tempDir("open-in-editor-signals", { + "storm.js": ` + const { readFileSync } = require("node:fs"); + const sleepBin = process.argv[2]; + function caughtMask() { + const status = readFileSync("/proc/self/status", "utf8"); + const line = status.split("\\n").find(l => l.startsWith("SigCgt:")); + return BigInt("0x" + line.slice("SigCgt:".length).trim()); + } + // Warm-up: let any lazy one-time handler installation happen before baselining. + try { Bun.openInEditor("0.05", { editor: sleepBin }); } catch {} + await Bun.sleep(150); + const baseline = caughtMask(); + let changed = 0n; + // Each call spawns a detached editor thread that runs \`sleep 0.15\` and waits for + // it, so dozens of editor threads overlap. + for (let i = 0; i < 64; i++) { + try { Bun.openInEditor("0.15", { editor: sleepBin }); } catch {} + if ((i & 7) === 0) changed |= baseline ^ caughtMask(); + } + // Keep sampling while the editor threads drain. + for (let i = 0; i < 150; i++) { + changed |= baseline ^ caughtMask(); + await Bun.sleep(5); + } + // Force GC (which uses SIGPWR on Linux to suspend/resume threads) to prove the + // process still survives it. + Bun.gc(true); + const changedSignals = []; + for (let sig = 1; sig <= 64; sig++) { + if (changed & (1n << BigInt(sig - 1))) changedSignals.push(sig); + } + console.log("CHANGED:" + JSON.stringify(changedSignals)); + console.log("ALIVE"); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "storm.js", sleep!], + 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(stdout).toContain("ALIVE"); + const changed = JSON.parse(stdout.match(/^CHANGED:(.*)$/m)![1]); + expect(changed).toEqual([]); + expect(stderr).toBe(""); + expect(proc.signalCode).toBeNull(); + expect(exitCode).toBe(0); +}); + // On Linux, JSC uses SIGPWR to suspend/resume threads for GC and the libpas -// scavenger. Bun.openInEditor spawns a detached thread that goes through +// scavenger. Bun.openInEditor spawns a detached thread that used to go through // bun.spawnSync, whose signal-forwarding setup must not touch SIGPWR or the // process is terminated the next time GC/scavenger fires. test.skipIf(!isLinux)("Bun.openInEditor does not break GC signal handling", async () => { From e7ed435abd0887ae10ebbce862742ee88d10a069 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 24 May 2026 02:25:54 +0000 Subject: [PATCH 2/4] test: assert CHANGED line is present before parsing it --- test/js/bun/util/open-in-editor-gc.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/js/bun/util/open-in-editor-gc.test.ts b/test/js/bun/util/open-in-editor-gc.test.ts index 818f8e761e61..73a6bbf351af 100644 --- a/test/js/bun/util/open-in-editor-gc.test.ts +++ b/test/js/bun/util/open-in-editor-gc.test.ts @@ -63,7 +63,9 @@ test.skipIf(!isLinux)("concurrent Bun.openInEditor calls do not touch process si const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stdout).toContain("ALIVE"); - const changed = JSON.parse(stdout.match(/^CHANGED:(.*)$/m)![1]); + const changedLine = stdout.match(/^CHANGED:(.*)$/m); + expect(changedLine).not.toBeNull(); + const changed = JSON.parse(changedLine![1]); expect(changed).toEqual([]); expect(stderr).toBe(""); expect(proc.signalCode).toBeNull(); From eef8bd42795a8d20c44f606d301ff519faaef138 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 24 May 2026 02:40:15 +0000 Subject: [PATCH 3/4] Update stale porting comments around the editor spawn thread --- src/runtime/cli/open.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/runtime/cli/open.rs b/src/runtime/cli/open.rs index 23f357cad5a7..9099e0e45dc9 100644 --- a/src/runtime/cli/open.rs +++ b/src/runtime/cli/open.rs @@ -287,6 +287,9 @@ impl Editor { } spawned.argc = i; + // Zig stored `std.process.Child.init(args_buf[0..i], default_allocator)` here and + // spawned a detached std.Thread to run it; the detached thread below does the + // spawn+wait itself via `spawn_sync_inherit` (see `auto_close`). let spawned_ptr = bun_core::heap::into_raw(spawned); // bun_threading has no detached-spawn helper; std::thread::spawn is used // and the JoinHandle is dropped, detaching the thread. From 02180da089bdf8007b89f0a1140c6e58bc9f46d9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 24 May 2026 03:18:48 +0000 Subject: [PATCH 4/4] ci: retrigger