Skip to content

spawn: do not forward SIGPWR on Linux #31121

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

Closed
robobun wants to merge 1 commit into from
Closed

spawn: do not forward SIGPWR on Linux #31121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion src/jsc/bindings/c-bindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,9 +933,11 @@ static struct sigaction previous_actions[NSIG];
M(SIGIO);

#if OS(LINUX)
// SIGPWR is intentionally omitted: JavaScriptCore uses it to suspend and
// resume threads for garbage collection (see ThreadingPOSIX.cpp). Replacing
// that handler here would break GC and terminate the process with SIGPWR.
#define FOR_EACH_LINUX_ONLY_SIGNAL(M) \
M(SIGPOLL); \
M(SIGPWR); \
M(SIGSTKFLT);

#endif
Expand Down
24 changes: 24 additions & 0 deletions test/js/bun/util/open-in-editor-gc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isLinux } from "harness";

// On Linux, JavaScriptCore uses SIGPWR to suspend and resume threads during
// garbage collection. Bun.openInEditor() spawns the editor via bun.spawnSync
// on a detached background thread, which installs signal-forwarding handlers
// for the duration of the spawn. Those handlers must not replace JSC's SIGPWR
// handler, otherwise a concurrent GC will terminate the process with SIGPWR.
test.skipIf(!isLinux)("Bun.openInEditor concurrent with GC does not terminate the process with SIGPWR", () => {
const script = `
for (let k = 0; k < 50; k++) {
try { Bun.openInEditor("foo" + k); } catch {}
}
for (let i = 0; i < 200; i++) Bun.gc(true);
`;
const { exitCode, signalCode } = Bun.spawnSync({
cmd: [bunExe(), "-e", script],
env: { ...bunEnv, EDITOR: undefined, VISUAL: undefined },

Check warning on line 18 in test/js/bun/util/open-in-editor-gc.test.ts

View check run for this annotation

Claude / Claude Code Review

Test may spawn real editor processes despite unsetting EDITOR/VISUAL 

Unsetting `EDITOR`/`VISUAL` is not enough to make this test hermetic: `bunEnv` spreads `process.env` (including `PATH`), and when no `EDITOR`/`VISUAL` is set `EditorContext::detect_editor` falls through to `Editor::by_fallback`, which probes `PATH` for `code`/`subl`/`nvim`/`vim`/etc. On a Linux dev box with VS Code installed, running this test will spawn 50 detached `code fooN` processes (50 editor tabs); on CI it may leave orphaned `xdg-open` invocations. Add `PATH: "/dev/null"` to the env over
Comment on lines +16 to +18

@claude claude Bot May 20, 2026

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.

🟡 Unsetting EDITOR/VISUAL is not enough to make this test hermetic: bunEnv spreads process.env (including PATH), and when no EDITOR/VISUAL is set EditorContext::detect_editor falls through to Editor::by_fallback, which probes PATH for code/subl/nvim/vim/etc. On a Linux dev box with VS Code installed, running this test will spawn 50 detached code fooN processes (50 editor tabs); on CI it may leave orphaned xdg-open invocations. Add PATH: "/dev/null" to the env override (or pass { editor: "/nonexistent" } as the second arg to Bun.openInEditor) — the SIGPWR race is still exercised either way since the detached spawn thread still runs.

Extended reasoning...

What the bug is

The new regression test attempts to prevent Bun.openInEditor from launching a real editor by setting env: { ...bunEnv, EDITOR: undefined, VISUAL: undefined }. This is insufficient: when neither an explicit editor option nor EDITOR/VISUAL is present, Bun falls back to probing PATH for a list of common editors and will happily launch whichever one it finds — 50 times, on detached background threads.

Code path

  1. bunEnv is defined as { ...process.env, ... } (test/harness.ts:50-51) and never overrides PATH, so the spawned child inherits the host PATH.
  2. The inner script calls Bun.openInEditor("foo" + k) with no second argument, so open_in_editor (src/runtime/api/BunObject.rs:1084-1095) sees no explicit editor and calls edit.auto_detect_editor(env).
  3. detect_editor (src/runtime/cli/open.rs:619-716) finds self.name empty and EDITOR/VISUAL unset, so it falls through to Editor::by_fallback at line 701.
  4. by_fallback (open.rs:221-246) iterates DEFAULT_PREFERENCE_LIST = [Vscode, Sublime, Atom, Neovim, Webstorm, Intellij, Textmate, Vim] (open.rs:392) and calls by_path_for_editor, which runs which() against env.get(b"PATH").
  5. If a match is found, editor.open() spawns a detached std::thread that calls sync::spawn (the same Bun__registerSignalsForForwarding path this PR fixes) with the resolved binary as argv[0].

Why the existing guard doesn't help

The EDITOR: undefined, VISUAL: undefined override only short-circuits the first two branches of detect_editor (the by_name lookups for $EDITOR and $VISUAL). The third branch — by_fallback — is gated solely on PATH, which the test leaves untouched. On Linux, bin_path()'s hardcoded fallback locations are macOS-only (open.rs:424-437), so PATH is the only thing that matters.

Step-by-step example

On a Linux developer machine with VS Code installed (/usr/bin/code in PATH):

  1. bun test test/js/bun/util/open-in-editor-gc.test.ts spawns the child with PATH=/usr/bin:..., EDITOR and VISUAL unset.
  2. For k=0: detect_editor → by_fallback → which("code") resolves /usr/bin/code → Editor::Vscode, edit.path = "/usr/bin/code".
  3. editor.open() builds argv ["/usr/bin/code", "foo0"] (Vscode uses the binary directly, no xdg-open prefix — open.rs:286) and spawns it on a detached thread.
  4. Repeat 50× → 50 VS Code tabs open on the developer's desktop pointing at non-existent foo0..foo49.
  5. The child process exits; the spawned editor processes are orphaned (not reaped, not killed).

For vim/nvim the argv is prefixed with OPENER = b"xdg-open" (open.rs:20, 276-284), which on headless CI typically fails fast, so the impact there is lower — but it still spawns 50 processes per test run.

Impact

This is a test-hermeticity issue, not a correctness bug. The test itself will not fail or hang: the JS thread never joins the detached spawn threads, and auto_close swallows spawn errors via let _ = sync::spawn(...). The SIGPWR regression is still correctly exercised regardless of whether an editor is found (the detached thread still calls Bun__registerSignalsForForwarding either way). However:

  • On a developer machine with code/subl/idea in PATH, every run of this test pops 50 editor windows.
  • On CI with vim/nvim in PATH (very common on Linux images), it spawns 50 short-lived xdg-open processes per run, adding noise and orphans.

Fix

Add PATH: "/dev/null" (or PATH: "") to the env override:

env: { ...bunEnv, EDITOR: undefined, VISUAL: undefined, PATH: "/dev/null" },

Alternatively, pass an explicit non-existent editor so detect_editor never runs the fallback probe:

try { Bun.openInEditor("foo" + k, { editor: "/nonexistent" }); } catch {}

Either change keeps the regression coverage intact (the detached-thread spawnSync + concurrent GC race still fires) while making the test hermetic.

stdout: "ignore",
stderr: "ignore",
});
expect(signalCode).toBeUndefined();
expect(exitCode).toBe(0);
});
Loading