openInEditor: don't run spawnSync's process-wide signal forwarding on detached editor threads#31297
openInEditor: don't run spawnSync's process-wide signal forwarding on detached editor threads#31297robobun 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 30 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)
WalkthroughThe PR refactors ChangesopenInEditor Process Spawning Refactor
Possibly related issues
Possibly related PRs
🚥 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 76-79: The test is using Buffer inside a try/catch which hides
failures from the initial coercion step in open_in_editor; change the call in
the loop to pass a guaranteed-valid string path (e.g., "/dev/null" or
"C:\\Windows\\System32\\drivers\\etc\\hosts" depending on platform) to
open_in_editor/openInEditor so the pre-spawn coercion runs and any unexpected
throws surface, and remove the empty catch so that failures that occur before
Editor::open() are not swallowed; update the loop that calls
Bun.openInEditor(Buffer) to call Bun.openInEditor(validPathString) and allow
exceptions to propagate.
🪄 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: 257e542e-e569-4089-a22b-d030b0995d1c
📒 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 |
|
On the possible-duplicate note: #31195 (open) targets the same underlying problem reported in #31194, via a different approach — it adds a This PR instead takes the detached editor helper off the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
robobun
force-pushed
the
farm/6d382bbf/openineditor-plain-spawn
branch
from
39c2537 to
6d35553
Compare
There was a problem hiding this comment.
Thanks for addressing the comment cleanup — no further issues from my side. Not auto-approving since this and #31195 take different approaches to the same fix and the author flagged that as a maintainer call; the spawn/signal-handling path change is also worth a human sign-off.
Extended reasoning...
Overview
This PR changes auto_close in src/runtime/cli/open.rs — the function that runs on a detached std::thread per Bun.openInEditor() call — to use bun_core::spawn_sync_inherit (a minimal posix_spawn + waitpid / std::process::Command helper) instead of sync::spawn (the full bun.spawnSync machinery). The net diff is ~28 lines removed and ~6 added in the Rust source, plus a new Linux-only stress test in test/js/bun/util/open-in-editor-gc.test.ts and a few comment updates. The change restores parity with the original Zig std.process.Child.spawn() + wait() and, as a side effect, removes a per-call MiniEventLoop leak on Windows that was documented in the deleted FIXME.
Security risks
None identified. The change removes process-global signal-handler manipulation from a detached thread rather than adding any. The argv passed to the new helper is constructed exactly as before (reconstructed from SpawnedEditorContext), and the editor binary path still comes from the same detection logic; no new external input is introduced. The xdg-open opener is now resolved via PATH (matching the Zig original) rather than as a cwd-relative path, which is if anything a minor improvement.
Level of scrutiny
Medium. The diff is small and is a simplification, but it sits at the intersection of process spawning, detached threads, and process-wide signal state — an area where this repo has already seen three related fuzzer fingerprints (#31183, #30956, this one). More importantly, there is an open competing PR (#31195) that fixes the same issue by adding a forward_signals option to sync::spawn and keeping the editor helper on the spawnSync path. The author explicitly noted in the thread that the choice between the two approaches is a maintainer decision. That is a design call I should not make on their behalf.
Other factors
All prior review feedback has been addressed: the CodeRabbit suggestion to pass a plain string path in the new test was applied (commit 318f5cb / "/dev/null"), and my earlier nit about three stale comments pointing back at bun.spawnSync was cleaned up (commit 6d35553). The bug-hunting system found no issues. The new test is acknowledged by the author as exercising the crash scenario rather than failing deterministically pre-fix (the underlying race is timing-dependent), which is consistent with the existing test in the same file but means CI coverage is best-effort. I verified bun_core::spawn_sync_inherit exists and is a thin posix_spawn + waitpid wrapper with no signal-handler side effects. Given the competing-approach decision and the signal-handling subject matter, deferring to a human reviewer.
|
CI triage for build 60248: the only failure was |
|
Status update: the retriggered build (#60262) is failing again while still in progress. The previous build's only failure was I've used my one CI retrigger, so I'll leave this as-is rather than pushing more empty commits. The branch is rebased on current main, conflict-free, and all review feedback is addressed — once the bunx upstream breakage clears (or the test is fixed on main), a fresh build should go green. |
Bun.openInEditor spawns its editor on a detached thread. Routing that spawn through bun.spawnSync installed process-wide signal-forwarding handlers (Bun__registerSignalsForForwarding) and mutated process globals that are only safe on the main thread, so concurrent openInEditor calls raced that state against JSC's SIGPWR-based GC thread suspension on Linux. Switch the detached thread to bun_core::spawn_sync_inherit — a plain spawn + waitpid with inherited stdio, matching the original std.process.Child behaviour — so no process-global signal state is touched off the main thread.
robobun
force-pushed
the
farm/6d382bbf/openineditor-plain-spawn
branch
from
4ed3d07 to
e07dfde
Compare
1 participant
Fuzzer fingerprint:
8723bd9fcfb9e59fWhat the fuzzer found
A flaky crash (process terminated by signal 30 /
SIGPWR, Linux) on a script whose core is:The recursion unwinds through tens of thousands of frames and calls
Bun.openInEditor()at every depth, so the process ends up running tens of thousands of short-lived "Open Editor" threads while the GC is active.SIGPWRis the signal JSC uses for GC/scavenger thread suspend/resume on Linux, so dying with it means the process-wide signal state was corrupted while a suspend was in flight.This is the third fuzzer fingerprint in this family (#31183 fixed
4e72ca85863f92faby droppingSIGPWRfrom the spawnSync forwarding list; #30956 is open for11551e051c08db2c, hardening the register/unregister pair itself).Root cause
Editor::open()runs the actual editor spawn on a detachedstd::thread(auto_close). The Zig original usedstd.process.Child.spawn()+wait()there — a plain spawn with inherited stdio. The Rust port routed it throughsync::spawn(the fullbun.spawnSyncmachinery), which:Bun__registerSignalsForForwarding/Bun__unregisterSignalsForForwarding, an unsynchronized process-globalprevious_actions[]table withSA_RESETHANDhandlers), andBun__currentSyncPID, crash-handler reset, job-control/no-orphans state)all of which are documented as main-thread-only (
c-bindings.cpp: "We only ever use bun.spawnSync on the main thread."). WithBun.openInEditorthat machinery runs concurrently on thousands of detached threads, racing each other and the GC's signal-based thread suspension.Fix
Make
auto_closeusebun_core::spawn_sync_inherit— a minimal spawn +waitpidwith inherited stdio — instead ofsync::spawn. That restores the originalstd.process.Childsemantics: the detached editor threads no longer touch any process-wide signal state, so there is nothing left for them to race against GC suspension.Side effects of the switch:
FIXME(windows-leak)in the removed block is resolved: the Windows path ofspawn_sync_inheritusesCreateProcessWviastd::process::Command, so noMiniEventLoopis leaked per editor open.xdg-openopener (used for vim/emacs/neovim on Linux) is now resolved viaPATHlike the Zig original, instead of being exec'd as a cwd-relative path.Test
Added a Linux-only case to
test/js/bun/util/open-in-editor-gc.test.tsthat mirrors the fuzzer scenario: 1500Bun.openInEditor()calls with no detectable editor, interleaved with allocation and GC, asserting the child exits cleanly with no signal.Caveat: the underlying failure is a timing-dependent race that I could not reproduce deterministically on an unpatched build (the fuzzer itself marked the crash flaky), so the test exercises the crash scenario rather than failing reliably before the fix. The existing test in that file (from #31183) has the same characteristic and still passes.
Fixes #31194