fix(coding-agent): fix windows hanging when descendants inherit stdout/stderr handles (#2389)
This commit is contained in:
@@ -5,6 +5,7 @@
|
|||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
- Tests for session-selector-rename and tree-selector are now keybinding-agnostic, resetting editor keybindings to defaults before each test so user `keybindings.json` cannot cause failures ([#2360](https://github.com/badlogic/pi-mono/issues/2360))
|
- Tests for session-selector-rename and tree-selector are now keybinding-agnostic, resetting editor keybindings to defaults before each test so user `keybindings.json` cannot cause failures ([#2360](https://github.com/badlogic/pi-mono/issues/2360))
|
||||||
|
- Fixed Windows bash execution hanging for commands that spawn detached descendants inheriting stdout/stderr handles, which caused `agent-browser` and similar commands to spin forever.
|
||||||
|
|
||||||
## [0.60.0] - 2026-03-18
|
## [0.60.0] - 2026-03-18
|
||||||
|
|
||||||
|
|||||||
@@ -3,6 +3,7 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import { spawn } from "node:child_process";
|
import { spawn } from "node:child_process";
|
||||||
|
import { waitForChildProcess } from "../utils/child-process.js";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Options for executing shell commands.
|
* Options for executing shell commands.
|
||||||
@@ -85,20 +86,22 @@ export async function execCommand(
|
|||||||
stderr += data.toString();
|
stderr += data.toString();
|
||||||
});
|
});
|
||||||
|
|
||||||
proc.on("close", (code) => {
|
// Wait for process termination without hanging on inherited stdio handles
|
||||||
if (timeoutId) clearTimeout(timeoutId);
|
// held open by detached descendants.
|
||||||
if (options?.signal) {
|
waitForChildProcess(proc)
|
||||||
options.signal.removeEventListener("abort", killProcess);
|
.then((code) => {
|
||||||
}
|
if (timeoutId) clearTimeout(timeoutId);
|
||||||
resolve({ stdout, stderr, code: code ?? 0, killed });
|
if (options?.signal) {
|
||||||
});
|
options.signal.removeEventListener("abort", killProcess);
|
||||||
|
}
|
||||||
proc.on("error", (_err) => {
|
resolve({ stdout, stderr, code: code ?? 0, killed });
|
||||||
if (timeoutId) clearTimeout(timeoutId);
|
})
|
||||||
if (options?.signal) {
|
.catch((_err) => {
|
||||||
options.signal.removeEventListener("abort", killProcess);
|
if (timeoutId) clearTimeout(timeoutId);
|
||||||
}
|
if (options?.signal) {
|
||||||
resolve({ stdout, stderr, code: 1, killed });
|
options.signal.removeEventListener("abort", killProcess);
|
||||||
});
|
}
|
||||||
|
resolve({ stdout, stderr, code: 1, killed });
|
||||||
|
});
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import { join } from "node:path";
|
|||||||
import type { AgentTool } from "@mariozechner/pi-agent-core";
|
import type { AgentTool } from "@mariozechner/pi-agent-core";
|
||||||
import { type Static, Type } from "@sinclair/typebox";
|
import { type Static, Type } from "@sinclair/typebox";
|
||||||
import { spawn } from "child_process";
|
import { spawn } from "child_process";
|
||||||
|
import { waitForChildProcess } from "../../utils/child-process.js";
|
||||||
import { getShellConfig, getShellEnv, killProcessTree } from "../../utils/shell.js";
|
import { getShellConfig, getShellEnv, killProcessTree } from "../../utils/shell.js";
|
||||||
import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, type TruncationResult, truncateTail } from "./truncate.js";
|
import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, formatSize, type TruncationResult, truncateTail } from "./truncate.js";
|
||||||
|
|
||||||
@@ -98,13 +99,6 @@ export function createLocalBashOperations(): BashOperations {
|
|||||||
child.stderr.on("data", onData);
|
child.stderr.on("data", onData);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Handle shell spawn errors
|
|
||||||
child.on("error", (err) => {
|
|
||||||
if (timeoutHandle) clearTimeout(timeoutHandle);
|
|
||||||
if (signal) signal.removeEventListener("abort", onAbort);
|
|
||||||
reject(err);
|
|
||||||
});
|
|
||||||
|
|
||||||
// Handle abort signal - kill entire process tree
|
// Handle abort signal - kill entire process tree
|
||||||
const onAbort = () => {
|
const onAbort = () => {
|
||||||
if (child.pid) {
|
if (child.pid) {
|
||||||
@@ -120,23 +114,30 @@ export function createLocalBashOperations(): BashOperations {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Handle process exit
|
// Handle shell spawn errors and wait for the process to terminate without hanging
|
||||||
child.on("close", (code) => {
|
// on inherited stdio handles held by detached descendants.
|
||||||
if (timeoutHandle) clearTimeout(timeoutHandle);
|
waitForChildProcess(child)
|
||||||
if (signal) signal.removeEventListener("abort", onAbort);
|
.then((code) => {
|
||||||
|
if (timeoutHandle) clearTimeout(timeoutHandle);
|
||||||
|
if (signal) signal.removeEventListener("abort", onAbort);
|
||||||
|
|
||||||
if (signal?.aborted) {
|
if (signal?.aborted) {
|
||||||
reject(new Error("aborted"));
|
reject(new Error("aborted"));
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (timedOut) {
|
if (timedOut) {
|
||||||
reject(new Error(`timeout:${timeout}`));
|
reject(new Error(`timeout:${timeout}`));
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
resolve({ exitCode: code });
|
resolve({ exitCode: code });
|
||||||
});
|
})
|
||||||
|
.catch((err) => {
|
||||||
|
if (timeoutHandle) clearTimeout(timeoutHandle);
|
||||||
|
if (signal) signal.removeEventListener("abort", onAbort);
|
||||||
|
reject(err);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
86
packages/coding-agent/src/utils/child-process.ts
Normal file
86
packages/coding-agent/src/utils/child-process.ts
Normal file
@@ -0,0 +1,86 @@
|
|||||||
|
import type { ChildProcess } from "node:child_process";
|
||||||
|
|
||||||
|
const EXIT_STDIO_GRACE_MS = 100;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Wait for a child process to terminate without hanging on inherited stdio handles.
|
||||||
|
*
|
||||||
|
* On Windows, daemonized descendants can inherit the child's stdout/stderr pipe
|
||||||
|
* handles. In that case the child emits `exit`, but `close` can hang forever even
|
||||||
|
* though the original process is already gone. We wait briefly for stdio to end,
|
||||||
|
* then forcibly stop tracking the inherited handles.
|
||||||
|
*/
|
||||||
|
export function waitForChildProcess(child: ChildProcess): Promise<number | null> {
|
||||||
|
return new Promise((resolve, reject) => {
|
||||||
|
let settled = false;
|
||||||
|
let exited = false;
|
||||||
|
let exitCode: number | null = null;
|
||||||
|
let postExitTimer: NodeJS.Timeout | undefined;
|
||||||
|
let stdoutEnded = child.stdout === null;
|
||||||
|
let stderrEnded = child.stderr === null;
|
||||||
|
|
||||||
|
const cleanup = () => {
|
||||||
|
if (postExitTimer) {
|
||||||
|
clearTimeout(postExitTimer);
|
||||||
|
postExitTimer = undefined;
|
||||||
|
}
|
||||||
|
child.removeListener("error", onError);
|
||||||
|
child.removeListener("exit", onExit);
|
||||||
|
child.removeListener("close", onClose);
|
||||||
|
child.stdout?.removeListener("end", onStdoutEnd);
|
||||||
|
child.stderr?.removeListener("end", onStderrEnd);
|
||||||
|
};
|
||||||
|
|
||||||
|
const finalize = (code: number | null) => {
|
||||||
|
if (settled) return;
|
||||||
|
settled = true;
|
||||||
|
cleanup();
|
||||||
|
child.stdout?.destroy();
|
||||||
|
child.stderr?.destroy();
|
||||||
|
resolve(code);
|
||||||
|
};
|
||||||
|
|
||||||
|
const maybeFinalizeAfterExit = () => {
|
||||||
|
if (!exited || settled) return;
|
||||||
|
if (stdoutEnded && stderrEnded) {
|
||||||
|
finalize(exitCode);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const onStdoutEnd = () => {
|
||||||
|
stdoutEnded = true;
|
||||||
|
maybeFinalizeAfterExit();
|
||||||
|
};
|
||||||
|
|
||||||
|
const onStderrEnd = () => {
|
||||||
|
stderrEnded = true;
|
||||||
|
maybeFinalizeAfterExit();
|
||||||
|
};
|
||||||
|
|
||||||
|
const onError = (err: Error) => {
|
||||||
|
if (settled) return;
|
||||||
|
settled = true;
|
||||||
|
cleanup();
|
||||||
|
reject(err);
|
||||||
|
};
|
||||||
|
|
||||||
|
const onExit = (code: number | null) => {
|
||||||
|
exited = true;
|
||||||
|
exitCode = code;
|
||||||
|
maybeFinalizeAfterExit();
|
||||||
|
if (!settled) {
|
||||||
|
postExitTimer = setTimeout(() => finalize(code), EXIT_STDIO_GRACE_MS);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const onClose = (code: number | null) => {
|
||||||
|
finalize(code);
|
||||||
|
};
|
||||||
|
|
||||||
|
child.stdout?.once("end", onStdoutEnd);
|
||||||
|
child.stderr?.once("end", onStderrEnd);
|
||||||
|
child.once("error", onError);
|
||||||
|
child.once("exit", onExit);
|
||||||
|
child.once("close", onClose);
|
||||||
|
});
|
||||||
|
}
|
||||||
120
packages/coding-agent/test/bash-close-hang-windows.test.ts
Normal file
120
packages/coding-agent/test/bash-close-hang-windows.test.ts
Normal file
@@ -0,0 +1,120 @@
|
|||||||
|
import { execFileSync } from "node:child_process";
|
||||||
|
import { existsSync, mkdirSync, readFileSync, rmSync } from "node:fs";
|
||||||
|
import { tmpdir } from "node:os";
|
||||||
|
import { join } from "node:path";
|
||||||
|
import { afterEach, beforeEach, describe, expect, it } from "vitest";
|
||||||
|
import { executeBash } from "../src/core/bash-executor.js";
|
||||||
|
import { createBashTool } from "../src/core/tools/bash.js";
|
||||||
|
|
||||||
|
function toBashSingleQuotedArg(value: string): string {
|
||||||
|
return `'${value.replace(/\\/g, "/").replace(/'/g, `'"'"'`)}'`;
|
||||||
|
}
|
||||||
|
|
||||||
|
function createInheritedStdioCommand(pidFile: string): string {
|
||||||
|
const pidFileArg = toBashSingleQuotedArg(pidFile);
|
||||||
|
return (
|
||||||
|
'node -e "' +
|
||||||
|
"const fs=require('fs');" +
|
||||||
|
"const {spawn}=require('child_process');" +
|
||||||
|
"const child=spawn(process.execPath,['-e','setTimeout(()=>{},60000)'],{stdio:'inherit',detached:true});" +
|
||||||
|
"fs.writeFileSync(process.argv[1], String(child.pid));" +
|
||||||
|
"child.unref();" +
|
||||||
|
"console.log('child-exiting');" +
|
||||||
|
'" ' +
|
||||||
|
pidFileArg
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
function cleanupDetachedChild(pidFile: string): void {
|
||||||
|
if (!existsSync(pidFile)) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const pid = Number.parseInt(readFileSync(pidFile, "utf-8").trim(), 10);
|
||||||
|
if (Number.isFinite(pid) && pid > 0) {
|
||||||
|
try {
|
||||||
|
execFileSync("taskkill", ["/F", "/T", "/PID", String(pid)], { stdio: "ignore" });
|
||||||
|
} catch {
|
||||||
|
// Process may have already exited.
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
async function withTimeout<T>(promise: Promise<T>, ms: number, onTimeout: () => void): Promise<T> {
|
||||||
|
return new Promise<T>((resolve, reject) => {
|
||||||
|
const timeoutId = setTimeout(() => {
|
||||||
|
onTimeout();
|
||||||
|
reject(new Error(`Timed out after ${ms}ms`));
|
||||||
|
}, ms);
|
||||||
|
|
||||||
|
promise.then(
|
||||||
|
(value) => {
|
||||||
|
clearTimeout(timeoutId);
|
||||||
|
resolve(value);
|
||||||
|
},
|
||||||
|
(error: unknown) => {
|
||||||
|
clearTimeout(timeoutId);
|
||||||
|
reject(error);
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
function getTextOutput(result: { content?: Array<{ type: string; text?: string }> }): string {
|
||||||
|
return (
|
||||||
|
result.content
|
||||||
|
?.filter((block) => block.type === "text")
|
||||||
|
.map((block) => block.text ?? "")
|
||||||
|
.join("\n") ?? ""
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
describe.skipIf(process.platform !== "win32")("Windows child-process close handling", () => {
|
||||||
|
let testDir: string;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
testDir = join(tmpdir(), `coding-agent-bash-close-test-${Date.now()}-${Math.random().toString(36).slice(2)}`);
|
||||||
|
mkdirSync(testDir, { recursive: true });
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
rmSync(testDir, { recursive: true, force: true });
|
||||||
|
});
|
||||||
|
|
||||||
|
it("executeBash resolves after the shell exits even if inherited stdio handles stay open", async () => {
|
||||||
|
const pidFile = join(testDir, "executor-grandchild.pid");
|
||||||
|
const command = createInheritedStdioCommand(pidFile);
|
||||||
|
const controller = new AbortController();
|
||||||
|
|
||||||
|
try {
|
||||||
|
const result = await withTimeout(executeBash(command, { signal: controller.signal }), 3000, () => {
|
||||||
|
controller.abort();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result.output).toContain("child-exiting");
|
||||||
|
expect(result.exitCode).toBe(0);
|
||||||
|
expect(result.cancelled).toBe(false);
|
||||||
|
} finally {
|
||||||
|
controller.abort();
|
||||||
|
cleanupDetachedChild(pidFile);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("bash tool resolves after the shell exits even if inherited stdio handles stay open", async () => {
|
||||||
|
const pidFile = join(testDir, "tool-grandchild.pid");
|
||||||
|
const command = createInheritedStdioCommand(pidFile);
|
||||||
|
const controller = new AbortController();
|
||||||
|
const bashTool = createBashTool(testDir);
|
||||||
|
|
||||||
|
try {
|
||||||
|
const result = await withTimeout(bashTool.execute("test-call", { command }, controller.signal), 3000, () => {
|
||||||
|
controller.abort();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(getTextOutput(result)).toContain("child-exiting");
|
||||||
|
} finally {
|
||||||
|
controller.abort();
|
||||||
|
cleanupDetachedChild(pidFile);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user