Spawn the editor from Bun.openInEditor without spawnSync's signal forwarding - #31299
Spawn the editor from Bun.openInEditor without spawnSync's signal forwarding#31299robobun wants to merge 4 commits into
Conversation
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 59 minutes and 47 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR consolidates editor process spawning in ChangesEditor spawn refactoring and signal handling validation
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/js/bun/util/open-in-editor-gc.test.ts`:
- Around line 65-67: The test uses stdout.match(/^CHANGED:(.*)$/m) with a
non-null assertion which throws an unhelpful error if the fixture never prints
CHANGED:; replace the assertion with an explicit match variable (e.g. const
match = stdout.match(/^CHANGED:(.*)$/m)), assert match is truthy with a clear
error message mentioning “missing CHANGED: output” (or fail the test), then
parse JSON.parse(match[1]) into changed and continue the existing expect on
changed; update references to stdout, match, changed, and the regex
/^CHANGED:(.*)$/m in open-in-editor-gc.test.ts accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aed9a9c5-f556-4d56-9ba9-4e7b43139b49
📒 Files selected for processing (2)
src/runtime/cli/open.rstest/js/bun/util/open-in-editor-gc.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for cleaning up the stale porting TODOs as well.
Extended reasoning...
Overview
This PR fixes a Fuzzilli-found crash (#31194) where concurrent Bun.openInEditor() calls from detached threads raced on bun.spawnSync's process-global signal-forwarding state (Bun__registerSignalsForForwarding / previous_actions[]), corrupting signal dispositions and getting the process killed by SIGPWR during GC. The fix replaces ~30 lines of sync::spawn setup (including a Windows MiniEventLoop leak workaround) in auto_close() with a one-line call to the existing bun_core::util::spawn_sync_inherit helper — a plain posix_spawn + waitpid (CreateProcessW on Windows) that touches no process-wide signal state. This matches what the original Zig code did (std.process.Child.spawn() + .wait()). A new Linux regression test samples SigCgt in /proc/self/status while hammering openInEditor and asserts the caught-signal mask never changes.
Security risks
None. The change narrows the spawn surface: it removes signal-handler manipulation and a per-call event-loop allocation, replacing them with a minimal spawn+wait that's already used by bun init and the crash handler. No new inputs, no privilege changes, no new code paths.
Level of scrutiny
Moderate — process spawning is sensitive, but this is a strict simplification that swaps a misused heavyweight primitive for an established lightweight one (spawn_sync_inherit is already exercised by init_command.rs and crash_handler/lib.rs). I read spawn_sync_inherit end-to-end: on Linux it's posix_spawn_bun + an EINTR-safe waitpid loop with no sigaction calls; on Windows it's std::process::Command (CreateProcessW) with inherited stdio and no event loop, which also eliminates the documented MiniEventLoop leak. Argv handling is unchanged (same (ptr, len) reconstruction into &[u8] slices, which spawn_sync_inherit accepts via impl AsRef<[u8]>).
Other factors
Both prior review nits have been addressed: my comment about the stale TODO(port) recommending bun.spawnSync was fixed in d68be24 (the Editor::open() comment now points at spawn_sync_inherit, and the obsolete SpawnedEditorContext TODO was removed), and CodeRabbit's regex-guard nit was fixed in a479191. The new test is well-constructed (warm-up before baselining, overlapping editor threads via sleep 0.15, forced GC, exit/signal assertions) and the PR description confirms it fails on current canary with the expected 14-signal delta. The musl CI entries are scripts/build/ci.ts build-step failures on a479191, not test failures, and the only commit since (d68be24) is comment-only. No CODEOWNERS cover these files.
There was a problem hiding this comment.
LGTM — focused fix that swaps the main-thread-only sync::spawn for the lightweight spawn_sync_inherit helper on the detached editor thread, restoring parity with the original Zig behavior; both earlier review nits have been addressed.
Extended reasoning...
Overview
This PR changes src/runtime/cli/open.rs to stop calling sync::spawn (bun.spawnSync) from the detached editor thread spawned by Bun.openInEditor, replacing it with bun_core::util::spawn_sync_inherit — a plain posix_spawn/CreateProcessW + wait helper with inherited stdio. It also removes the now-unneeded sync import, the Windows MiniEventLoop::init_global workaround (and its FIXME describing the per-call leak), and updates two stale TODO(port) comments that previously recommended the unsafe approach. A new Linux-only regression test in test/js/bun/util/open-in-editor-gc.test.ts hammers openInEditor concurrently while sampling SigCgt from /proc/self/status and asserts the caught-signal mask never changes, then forces GC to confirm the process survives SIGPWR traffic. Net diff in open.rs is roughly −30 lines.
Security risks
None identified. The change removes process-global signal-handler mutation from a non-main thread (a thread-safety hazard) rather than adding any new attack surface. spawn_sync_inherit is an existing, established helper already used elsewhere (e.g., crash_handler's llvm-symbolizer spawn); on Linux/FreeBSD it routes through posix_spawn_bun with explicit dup2 actions for fds 0–2, on macOS it uses posix_spawnp, and on Windows it uses CreateProcessW directly. The argv passed in is the same editor-binary + file-path argv that was previously handed to sync::spawn, so there's no new injection vector. The editor binary is whatever the user configured (or auto-detected) — unchanged from before.
Level of scrutiny
Medium. Process spawning and signal handling are subtle areas, but this is a simplification that restores parity with the original Zig implementation (std.process.Child.spawn() + .wait() on a detached thread, no signal forwarding). I verified spawn_sync_inherit in src/bun_core/util.rs:5088 and confirmed it does no signal-forwarding registration and touches no process-global mutable state — it just builds a NUL-terminated argv, spawns, and waitpids. The PR description's diagnosis of the race on previous_actions[] / Bun__currentSyncPID is accurate and matches the "main thread only" comment in c-bindings.cpp. The behavior difference (no Ctrl-C forwarding to the editor child) matches the pre-port Zig behavior, so this is not a regression.
Other factors
- Both prior review nits were addressed in follow-up commits: my comment about the stale
TODO(port): … calls bun.spawnSyncguidance inEditor::open()was fixed in d68be24, and CodeRabbit's nit about guarding theCHANGED:regex match was fixed in a479191. - The new test is well-designed: it uses an absolute path to
sleepas the editor so dozens of editor threads overlap, includes a warm-up call before baselining to avoid false positives from lazy one-time handler installation, and asserts on the precise observable (SigCgt mask) rather than just "didn't crash". The PR author confirmed it fails on canary and passes with the fix. - The Windows side benefit (eliminating the per-call
MiniEventLoop+uv_loop_tleak documented in the removed FIXME) falls out for free sincespawn_sync_inheritusesCreateProcessWdirectly. - A bot flagged potential duplicate PRs (#31297, #31195) — that's a merge-coordination question for maintainers, not a correctness concern with this PR.
a1fb564 to
a5c5061
Compare
There was a problem hiding this comment.
LGTM — small, well-reasoned simplification that swaps the off-main-thread sync::spawn for the established spawn_sync_inherit helper, with a solid regression test; both prior nits are addressed.
Extended reasoning...
Overview
This PR changes auto_close() in src/runtime/cli/open.rs — the function that runs on a detached thread to spawn the user's editor for Bun.openInEditor(). It replaces ~25 lines of sync::spawn setup (including a Windows-specific MiniEventLoop::init_global that the deleted FIXME noted was leaking per call) with a single call to bun_core::util::spawn_sync_inherit, an existing helper already used by init_command.rs and crash_handler/lib.rs. It also adds a Linux regression test that samples SigCgt from /proc/self/status while hammering openInEditor and asserts the caught-signal mask never changes, plus updates the surrounding porting comments.
Security risks
None identified. The change removes process-wide signal-disposition mutation from a detached thread rather than adding any new privileged behavior. The spawned process is the user's configured editor, same as before; stdio inheritance and argv construction are unchanged. No auth, crypto, or permission surfaces are touched.
Level of scrutiny
Moderate — process spawning and signal handling are sensitive, but this diff is a strict simplification: it deletes the off-main-thread use of Bun__registerSignalsForForwarding (which c-bindings.cpp documents as main-thread-only) and falls back to a minimal posix_spawn+waitpid / CreateProcessW helper that's already in production use elsewhere in the codebase. This also restores parity with the original Zig implementation (std.process.Child.spawn() + .wait() with no signal forwarding). The new test directly asserts the invariant being fixed and the PR description shows it failing on canary and passing here.
Other factors
- Both prior inline nits (coderabbit's regex-match guard, my stale-TODO comment) were addressed in follow-up commits (05c6d94, e0bef05) and are marked resolved.
- No CODEOWNERS entry covers these files.
- The bug-hunting system found nothing in this run.
- The Windows per-call
MiniEventLoopleak called out in the deleted FIXME is fixed as a side effect sincespawn_sync_inheritusesCreateProcessWdirectly. - The PR description includes a detailed root-cause analysis (race on the shared
previous_actions[]array) and verification steps includingcargo check/clippyand the fuzzer repro.
|
CI status for the rebased head (
No code-path overlap between this PR (Bun.openInEditor's editor-thread spawn) and bunx, so I'm not retriggering further; this should go green once the Node-version bump lands on main. |
…warding 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.
a5c5061 to
02180da
Compare
There was a problem hiding this comment.
LGTM — focused fix swapping the editor thread's spawn to spawn_sync_inherit, with a solid SigCgt regression test; prior nits addressed.
Extended reasoning...
Overview
This PR changes one code path in src/runtime/cli/open.rs: the detached editor thread spawned by Bun.openInEditor now calls bun_core::util::spawn_sync_inherit(&argv) (plain posix_spawn/CreateProcessW + wait, inherited stdio) instead of sync::spawn (bun.spawnSync). The latter's Bun__registerSignalsForForwarding / Bun__unregisterSignalsForForwarding mutate process-global previous_actions[] and Bun__currentSyncPID and are documented as main-thread-only; running them from many concurrent detached threads raced and corrupted process-wide signal dispositions, leading to SIGPWR termination during GC. The diff also removes the now-unnecessary Windows MiniEventLoop::init_global per-call leak, updates the porting comments to point at the new helper, and adds a Linux regression test that samples SigCgt from /proc/self/status while hammering openInEditor and asserts the mask never changes.
Security risks
None. This is the editor-launch convenience API; no auth, crypto, permissions, or untrusted-input parsing is touched. The change strictly removes process-global signal mutation from a non-main thread.
Level of scrutiny
Low–medium. The substantive change is a single function-call swap to an existing, already-used helper (spawn_sync_inherit), restoring the behavior the original Zig implementation had (std.process.Child.spawn() + wait() with no signal forwarding). I verified spawn_sync_inherit in bun_core/util.rs: it's a thin posix_spawn(+PATH resolve)/posix_spawnp/CreateProcessW + waitpid wrapper with no global state. Bun.openInEditor is a developer-convenience path, not a hot runtime path.
Other factors
- Both prior inline comments (CodeRabbit's regex-guard nit and my stale-TODO nit) are addressed in commits e7ed435 and eef8bd4 and marked resolved; the diff reflects both fixes.
- The new test is well-constructed: it baselines
SigCgtafter a warm-up call, fires 64 overlappingsleep 0.15editor spawns, samples the mask throughout, forcesBun.gc(true), and asserts no signal bits flipped and the process is alive with exit 0. It demonstrably fails on canary and passes with the fix. - CI per the latest robobun summary is green everywhere except an unrelated
bunx.test.tsAngular/Node-version registry failure on Alpine that reproduces on main. - Duplicate-PR bot flagged #31297 / #31195 as overlapping fixes — a merge-coordination matter for maintainers, not a correctness concern with this diff.
|
Consolidated into #31297: same source change, and the signal dispositions check from this PR's test is folded in there (as a single kept-alive editor sampled via SigCgt, so it fails deterministically without the fix). This branch also had a conflict in the test file. |
1 participant
What does this PR do?
Fixes #31194
Fixes a Fuzzilli-found crash (fingerprint
1f89a105f06c0e7e): a script that callsBun.openInEditor()tens of thousands of times (via a recursive constructor that swallows the stack-overflow error and calls it on every unwound frame) gets the process killed by signal 30 (SIGPWR — the signal JSC uses on Linux to suspend/resume threads for GC) with no output.Each
Bun.openInEditor()call spawns a detached editor thread, and that thread ran the fullsync::spawn(bun.spawnSync) machinery. spawnSync's signal-forwarding setup is process-global and written for main-thread-only use (c-bindings.cpp: "Note: We only ever use bun.spawnSync on the main thread"):Bun__registerSignalsForForwarding()/Bun__unregisterSignalsForForwarding()install a one-shot (SA_RESETHAND) forwarding handler over ~14 signals and save/restore the previous dispositions through a shared, unsynchronizedprevious_actions[]array that getsmemsetto zero after each restore, plus the sharedBun__currentSyncPID.With many concurrent
openInEditorcalls, those register/unregister cycles race each other: restores frequently re-install from the zeroed array, so the process-wide dispositions of SIGHUP, SIGINT, SIGQUIT, SIGTRAP, SIGABRT, SIGUSR2, SIGALRM, SIGTERM, SIGSTKFLT, SIGXCPU, SIGXFSZ, SIGVTALRM, SIGIO and SIGSYS flip between "one-shot forwarding handler" andSIG_DFLfor the duration of the storm and can be left corrupted afterwards. Tracing a release build during a 256-call storm shows ~250sigaction(SIGHUP, forwarding handler w/ SA_RESETHAND)installs and ~247 restores of a zeroedsigaction(i.e.SIG_DFL), ending with the handler gone. That is the same failure class as #31183 (which dropped SIGPWR itself from the forwarding list after an earlier fuzzer crash): process-wide signal handling gets scrambled by the editor threads while GC's SIGPWR suspend/resume traffic is in flight.The fix makes the detached editor thread spawn the editor with the minimal
bun_core::util::spawn_sync_inherit(plain spawn +waitpid, inherited stdio) instead ofsync::spawn. That matches what the original implementation did (std.process.Child.spawn()+wait()on the detached thread — no signal forwarding, no job control, noBun__currentSyncPID), so nothing in theopenInEditorpath touches process-wide signal state anymore. It also removes the per-callMiniEventLoopleak on Windows that the old code's FIXME described, sincespawn_sync_inheritusesCreateProcessWdirectly there.Note: I could not reproduce the exact SIGPWR termination locally (it needs the fuzzer machine's timing/parallelism — the crash is deterministic there), but the process-wide disposition corruption from this code path reproduces reliably on current canary and is what this PR removes.
How did you verify your code works?
test/js/bun/util/open-in-editor-gc.test.ts: it hammersBun.openInEditorwith an absolute editor path pointing atsleepso dozens of editor threads overlap, samples the process's caught-signal mask (SigCgtin/proc/self/status) the whole time, and asserts it never changes, then forces GC and asserts the process is still alive.USE_SYSTEM_BUN=1 bun test test/js/bun/util/open-in-editor-gc.test.ts(bun 1.4.0-canary f161e03) fails: the mask changes for signals[1,2,3,5,6,12,14,15,16,24,25,26,29,31].bun bd test test/js/bun/util/open-in-editor-gc.test.tspasses repeatedly with this change; the pre-existing SIGPWR test in the same file still passes.Bun.openInEditor(Bun)calls interleaved withBun.gc) run to completion with exit 0 on the debug build with this change.cargo check -p bun_runtime/cargo clippy -p bun_runtimeare clean for the touched code.