Bun.openInEditor: throw when no editor is detected instead of spawning an empty command - #31310
Bun.openInEditor: throw when no editor is detected instead of spawning an empty command#31310robobun wants to merge 2 commits into
Conversation
…g an empty command detect_editor caches "nothing found" as Editor::None, but open_in_editor only treated Option::None as failure, so with no detectable editor every call silently spawned a detached "Open Editor" thread plus a child process with an empty argv[0] that could never exec. Treat Editor::None as "not found" so the existing "Failed to auto-detect editor" / "Could not find editor" errors fire instead.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 19 minutes and 31 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)
Comment |
|
Updated 11:00 PM PT - May 23rd, 2026
❌ @robobun, your commit 32febea has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31310That installs a local version of the PR into your bun-31310 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
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 — correctly maps the Some(Editor::None) sentinel to the existing "no editor" error paths, with a regression test.
Extended reasoning...
Overview
This PR touches two files: a 3-line logic change in src/runtime/api/BunObject.rs (open_in_editor) and a new regression test in test/js/bun/util/open-in-editor-gc.test.ts. The change adds .filter(|e| *e != Editor::None) at three points where an Option<Editor> is consumed, so that the cached "nothing found" sentinel is treated the same as Option::None and routes into the pre-existing Could not find editor … / Failed to auto-detect editor throws instead of falling through to spawn an empty command.
I verified the root cause in the codebase: EditorContext::detect_editor ends with self.editor = Some(Editor::None); (src/runtime/cli/open.rs:646) when no editor is resolvable, and auto_detect_editor only re-detects when self.editor.is_none(). So the prior code's match edit.editor { Some(e) => e, None => throw } could never reach the throw once detection had run — it would proceed with Editor::None and an empty edit.path, spawning a doomed child per call. The filter is the minimal, obvious fix and matches the PR description exactly.
Security risks
None. Bun.openInEditor is a developer-convenience API; this change strictly reduces process spawning by replacing an empty-argv fork with a thrown JS error. No new inputs are accepted, no auth/crypto/permissions code is touched.
Level of scrutiny
Low. The diff is mechanical (three identical .filter insertions on Option<Editor>), the sentinel semantics are confirmed in open.rs, and the only behavioral change is that previously-silent failure cases now throw the error message the code already intended to throw. The accompanying test pins the behavior (50/50 throws, exact error message, clean exit) in an isolated child process with an empty PATH and no EDITOR/VISUAL.
Other factors
No CODEOWNERS cover these paths, no bugs were flagged by the bug-hunting system, and there are no outstanding human review comments. The existing Bun.openInEditor does not break GC signal handling test (explicit absolute editor path) is unaffected since absolute paths resolve to Editor::Other/a real variant, never Editor::None.
There was a problem hiding this comment.
LGTM — minimal fix that makes the existing-but-unreachable "Failed to auto-detect editor" error actually fire instead of spawning an empty command.
Extended reasoning...
Overview
This PR adds .filter(|e| *e != Editor::None) at three points in open_in_editor (src/runtime/api/BunObject.rs) so that the Editor::None sentinel — which EditorContext::detect_editor writes at src/runtime/cli/open.rs:646 when nothing is found — is treated the same as Option::None. Previously the "Could not find editor …" and "Failed to auto-detect editor" branches were dead code because detect_editor always returns Some(_), so the function fell through to editor.open() with an empty edit.path and spawned a doomed child per call. A new Linux-only test spawns a child bun with an empty PATH and no EDITOR/VISUAL, calls Bun.openInEditor 50× and asserts every call throws.
Security risks
None. The change strictly removes a process spawn (empty argv[0]) in favor of throwing a JS error. No new inputs are interpreted, no paths or commands are constructed differently for the success cases (absolute paths and real detected editors are unchanged).
Level of scrutiny
Low-to-medium. The diff is three one-line filters plus a test; I traced both the first-call path (edit.editor == None → auto_detect_editor runs → sets Some(Editor::None) → filtered → throw) and the cached path (edit.editor == Some(Editor::None) → filtered → auto_detect_editor is a no-op via its is_none() guard → filtered again → throw), and the explicit-{ editor: "…" } path now correctly restores prev and throws "Could not find editor" when detection yields Editor::None. Editor::Other (used for unrecognized absolute paths) is untouched, so user-supplied editor binaries still work.
Other factors
The single CI failure (webview-chrome.test.ts core dump) is unrelated to this change. The bot-flagged "duplicate" PRs (#31299/#31297/#31195) target the orthogonal signal-forwarding side of #31194; this PR is complementary and the description explicitly leaves #31194 open. No CODEOWNERS cover these files, no human review comments are outstanding, and the bug-hunting system found nothing.
|
CI status: the code change is green on every job that exercised it; the two failing runs are unrelated, pre-existing flakes:
Neither suite touches |
|
Closing: the same fix landed in #37210. #37210 (merged 2026-08-09) treats Verified on current main (bdb7382): |
1 participant
What does this PR do?
Fixes a Fuzzilli-found flaky crash (fingerprint
ad4892a7a5610032, TERMSIG 30 / "Power failure" on Linux) in theBun.openInEditorpath — the same family as #31183.Root cause
EditorContext::detect_editorcaches "nothing found" as the sentinelEditor::None(with an empty binary path), butBun.openInEditoronly treatedOption::Noneas "no editor". So when no editor is detectable (no bunfigeditor, noEDITOR/VISUAL, nothing inPATH— i.e. the fuzzing container), the intended"Failed to auto-detect editor"error never fired. Instead, every call silently spawned a detached "Open Editor" thread plus a forked child with an emptyargv[0]that can never exec.The fuzzer sample exploits this: it recurses until stack overflow and calls
Bun.openInEditoronce per unwound frame, so a single script run creates tens of thousands of detached threads, each running the synchronous spawn machinery (process-global signal-forwarding register/unregister, vfork + wait) concurrently, followed byBun.gc(true). On Linux, JSC and the libpas scavenger suspend threads with SIGPWR during exactly this kind of thread/allocator churn (verified: forcing SIGPWR toSIG_DFLand running this workload dies immediately with "Power failure"), and the storm of concurrent, main-thread-only spawnSync signal bookkeeping is what lets the flaky SIGPWR termination show up. #31183 removed SIGPWR from the forwarding list; this removes the pointless spawn storm that hammers it.Fix
Treat
Editor::Noneas "not found" inBun.openInEditor:"Failed to auto-detect editor"instead of spawning an empty command per call{ editor: "code" }(or another known name) that isn't installed → throws"Could not find editor \"code\""instead of falling through to the empty spawnRelated (not closed by this PR): #31194 tracks making the detached editor helper skip the foreground spawnSync signal-forwarding entirely (via a
forward_signalsspawn option). This PR only eliminates the no-editor spawn storm; when a real editor is configured, the helper thread still usessync::spawnas before, so that issue remains open.How did you verify your code works?
test/js/bun/util/open-in-editor-gc.test.tsrunsBun.openInEditor50× in a child bun with editor detection guaranteed to fail and asserts every call throws (fails on the previous behavior:threw: 0, silent: 50; passes with this change).Bun.openInEditor does not break GC signal handlingtest (from Don't forward SIGPWR in spawnSync signal handling #31183, uses an explicit editor path) still passes.Failed to auto-detect editor) and no longer spawns any threads/processes; previously it spawned one detached thread + doomed child per unwound frame.