From bdcde7bfc423f8e885a08fc20857049ca8b60241 Mon Sep 17 00:00:00 2001 From: Ahmed Abushagur Date: Wed, 25 Feb 2026 23:50:59 -0800 Subject: [PATCH] fix: use spawnSync for script execution to eliminate fd 0 competition (#1942) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cmdRun path (the main user flow) was still using async child_process.spawn for script execution. This left Bun's event loop running while SSH (a grandchild process inside the bash script) competed for fd 0 input bytes — causing intermittent keystroke loss. Switch spawnBash to use spawnSync, which blocks the event loop entirely and gives the child process exclusive terminal access. This matches what we already did for runInteractiveCommand in #1939. Also removes dead spawnCalls tracking code from cmdrun-happy-path test. Co-authored-by: Claude Opus 4.6 --- packages/cli/package.json | 2 +- .../src/__tests__/cmdrun-happy-path.test.ts | 8 --- .../commands-update-download.test.ts | 27 ++----- packages/cli/src/commands.ts | 71 ++++++++++--------- 4 files changed, 45 insertions(+), 63 deletions(-) diff --git a/packages/cli/package.json b/packages/cli/package.json index be9f2323..126f7daf 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@openrouter/spawn", - "version": "0.10.18", + "version": "0.10.19", "type": "module", "bin": { "spawn": "cli.js" diff --git a/packages/cli/src/__tests__/cmdrun-happy-path.test.ts b/packages/cli/src/__tests__/cmdrun-happy-path.test.ts index bff7ead1..e948eef2 100644 --- a/packages/cli/src/__tests__/cmdrun-happy-path.test.ts +++ b/packages/cli/src/__tests__/cmdrun-happy-path.test.ts @@ -68,13 +68,6 @@ const { cmdRun } = await import("../commands.js"); const VALID_SCRIPT = "#!/bin/bash\nset -eo pipefail\nexit 0"; -/** Track child_process.spawn calls to verify env vars passed to bash */ -let spawnCalls: Array<{ - command: string; - args: string[]; - options: any; -}> = []; - /** Track all fetch calls to verify download behavior */ let fetchCalls: Array<{ url: string; @@ -156,7 +149,6 @@ describe("cmdRun happy-path pipeline", () => { mockSpinnerStop.mockClear(); mockSpinnerMessage.mockClear(); fetchCalls = []; - spawnCalls = []; processExitSpy = spyOn(process, "exit").mockImplementation((_code?: number): never => { throw new Error("process.exit"); diff --git a/packages/cli/src/__tests__/commands-update-download.test.ts b/packages/cli/src/__tests__/commands-update-download.test.ts index 2025d747..5617e12e 100644 --- a/packages/cli/src/__tests__/commands-update-download.test.ts +++ b/packages/cli/src/__tests__/commands-update-download.test.ts @@ -56,29 +56,16 @@ mock.module("@clack/prompts", () => ({ // - execSync: used by performUpdate() to run curl|bash install — without this mock, // "should handle update failure gracefully" downloads the real install script from // the network, causing a 58s timeout under full-suite concurrency (CLAUDE.md violation). -// - spawn: used by spawnBash() to run downloaded scripts — mock must fire the "close" -// event immediately (code 0) so Promise-based callers resolve rather than hanging. +// - spawnSync: used by spawnBash() to run downloaded scripts — returns exit code 0 +// so callers see a successful execution. mock.module("node:child_process", () => ({ execSync: mock(() => {}), execFileSync: mock(() => {}), - spawn: mock(() => { - type Handler = (...args: unknown[]) => void; - const child = { - on: mock((event: string, cb: Handler) => { - if (event === "close") { - queueMicrotask(() => cb(0, null)); - } - return child; - }), - stdout: { - on: mock(() => {}), - }, - stderr: { - on: mock(() => {}), - }, - }; - return child; - }), + spawnSync: mock(() => ({ + status: 0, + signal: null, + error: null, + })), })); // Import commands after mock setup diff --git a/packages/cli/src/commands.ts b/packages/cli/src/commands.ts index 0332ec90..97e92883 100644 --- a/packages/cli/src/commands.ts +++ b/packages/cli/src/commands.ts @@ -3,7 +3,7 @@ import * as p from "@clack/prompts"; import pc from "picocolors"; import * as v from "valibot"; import { parseJsonWith, isString } from "@openrouter/spawn-shared"; -import { spawn } from "node:child_process"; +import { spawnSync } from "node:child_process"; import * as fs from "node:fs"; import * as path from "node:path"; import type { Manifest } from "./manifest.js"; @@ -1678,15 +1678,15 @@ function handleUserInterrupt(errMsg: string, dashboardUrl?: string): void { * and an interactive session, so retrying would create duplicate servers. * On SSH disconnect (exit 255), shows a reconnect hint instead. */ -async function runBashScript( +function runBashScript( script: string, prompt?: string, dashboardUrl?: string, debug?: boolean, spawnName?: string, -): Promise { +): string | undefined { try { - await runBash(script, prompt, debug, spawnName); + runBash(script, prompt, debug, spawnName); return undefined; // success } catch (err) { const errMsg = getErrorMessage(err); @@ -1748,42 +1748,45 @@ async function execScript( } } - const lastErr = await runBashScript(scriptContent, prompt, dashboardUrl, debug, spawnName); + const lastErr = runBashScript(scriptContent, prompt, dashboardUrl, debug, spawnName); if (lastErr) { reportScriptFailure(lastErr, cloud, agent, authHint, prompt, dashboardUrl, spawnName); } } -function spawnBash(script: string, env: Record): Promise { - return new Promise((resolve, reject) => { - const child = spawn( - "bash", - [ - "-c", - script, - ], - { - stdio: "inherit", - env, - }, - ); - child.on("close", (code: number | null, signal: NodeJS.Signals | null) => { - if (code === 0) { - resolve(); - } else if (code !== null) { - const msg = code === 130 ? "Script interrupted by user (Ctrl+C)" : `Script exited with code ${code}`; - reject(new Error(msg)); - } else { - // code is null when killed by a signal (SIGKILL, SIGTERM, etc.) - const sig = signal ?? "unknown signal"; - reject(new Error(`Script was killed by ${sig}`)); - } - }); - child.on("error", reject); - }); +function spawnBash(script: string, env: Record): void { + const result = spawnSync( + "bash", + [ + "-c", + script, + ], + { + stdio: "inherit", + env, + }, + ); + + if (result.error) { + throw result.error; + } + + const code = result.status; + const signal = result.signal; + + if (code === 0) { + return; + } + if (code !== null) { + const msg = code === 130 ? "Script interrupted by user (Ctrl+C)" : `Script exited with code ${code}`; + throw new Error(msg); + } + // code is null when killed by a signal (SIGKILL, SIGTERM, etc.) + const sig = signal ?? "unknown signal"; + throw new Error(`Script was killed by ${sig}`); } -function runBash(script: string, prompt?: string, debug?: boolean, spawnName?: string): Promise { +function runBash(script: string, prompt?: string, debug?: boolean, spawnName?: string): void { // SECURITY: Validate script content before execution validateScriptContent(script); @@ -1810,7 +1813,7 @@ function runBash(script: string, prompt?: string, debug?: boolean, spawnName?: s // gets a pristine file descriptor (prevents silent hangs / early exit) prepareStdinForHandoff(); - return spawnBash(script, env); + spawnBash(script, env); } // ── List ───────────────────────────────────────────────────────────────────────