mirror of
https://github.com/OpenRouterTeam/spawn.git
synced 2026-05-13 07:10:46 +00:00
fix: use spawnSync for script execution to eliminate fd 0 competition (#1942)
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 <noreply@anthropic.com>
This commit is contained in:
parent
954f3b4893
commit
bdcde7bfc4
4 changed files with 45 additions and 63 deletions
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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> {
|
||||
): 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<string, string | undefined>): Promise<void> {
|
||||
return new Promise<void>((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<string, string | undefined>): 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<void> {
|
||||
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 ───────────────────────────────────────────────────────────────────────
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue