From 25b185f398e6a1991c41f72e1a5d6ac6832a5e20 Mon Sep 17 00:00:00 2001 From: Duncan Ogilvie Date: Thu, 19 Mar 2026 08:34:52 +0100 Subject: [PATCH] fix(coding-agent): fix windows hanging when descendants inherit stdout/stderr handles (#2389) --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/core/exec.ts | 33 ++--- packages/coding-agent/src/core/tools/bash.ts | 43 ++++--- .../coding-agent/src/utils/child-process.ts | 86 +++++++++++++ .../test/bash-close-hang-windows.test.ts | 120 ++++++++++++++++++ 5 files changed, 247 insertions(+), 36 deletions(-) create mode 100644 packages/coding-agent/src/utils/child-process.ts create mode 100644 packages/coding-agent/test/bash-close-hang-windows.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 84ccb8b8..839c15bb 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### 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)) +- 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 diff --git a/packages/coding-agent/src/core/exec.ts b/packages/coding-agent/src/core/exec.ts index b7dd046c..40f0453d 100644 --- a/packages/coding-agent/src/core/exec.ts +++ b/packages/coding-agent/src/core/exec.ts @@ -3,6 +3,7 @@ */ import { spawn } from "node:child_process"; +import { waitForChildProcess } from "../utils/child-process.js"; /** * Options for executing shell commands. @@ -85,20 +86,22 @@ export async function execCommand( stderr += data.toString(); }); - proc.on("close", (code) => { - if (timeoutId) clearTimeout(timeoutId); - if (options?.signal) { - options.signal.removeEventListener("abort", killProcess); - } - resolve({ stdout, stderr, code: code ?? 0, killed }); - }); - - proc.on("error", (_err) => { - if (timeoutId) clearTimeout(timeoutId); - if (options?.signal) { - options.signal.removeEventListener("abort", killProcess); - } - resolve({ stdout, stderr, code: 1, killed }); - }); + // Wait for process termination without hanging on inherited stdio handles + // held open by detached descendants. + waitForChildProcess(proc) + .then((code) => { + if (timeoutId) clearTimeout(timeoutId); + if (options?.signal) { + options.signal.removeEventListener("abort", killProcess); + } + resolve({ stdout, stderr, code: code ?? 0, killed }); + }) + .catch((_err) => { + if (timeoutId) clearTimeout(timeoutId); + if (options?.signal) { + options.signal.removeEventListener("abort", killProcess); + } + resolve({ stdout, stderr, code: 1, killed }); + }); }); } diff --git a/packages/coding-agent/src/core/tools/bash.ts b/packages/coding-agent/src/core/tools/bash.ts index f9d948c2..6685efca 100644 --- a/packages/coding-agent/src/core/tools/bash.ts +++ b/packages/coding-agent/src/core/tools/bash.ts @@ -5,6 +5,7 @@ import { join } from "node:path"; import type { AgentTool } from "@mariozechner/pi-agent-core"; import { type Static, Type } from "@sinclair/typebox"; import { spawn } from "child_process"; +import { waitForChildProcess } from "../../utils/child-process.js"; import { getShellConfig, getShellEnv, killProcessTree } from "../../utils/shell.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); } - // 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 const onAbort = () => { if (child.pid) { @@ -120,23 +114,30 @@ export function createLocalBashOperations(): BashOperations { } } - // Handle process exit - child.on("close", (code) => { - if (timeoutHandle) clearTimeout(timeoutHandle); - if (signal) signal.removeEventListener("abort", onAbort); + // Handle shell spawn errors and wait for the process to terminate without hanging + // on inherited stdio handles held by detached descendants. + waitForChildProcess(child) + .then((code) => { + if (timeoutHandle) clearTimeout(timeoutHandle); + if (signal) signal.removeEventListener("abort", onAbort); - if (signal?.aborted) { - reject(new Error("aborted")); - return; - } + if (signal?.aborted) { + reject(new Error("aborted")); + return; + } - if (timedOut) { - reject(new Error(`timeout:${timeout}`)); - return; - } + if (timedOut) { + reject(new Error(`timeout:${timeout}`)); + return; + } - resolve({ exitCode: code }); - }); + resolve({ exitCode: code }); + }) + .catch((err) => { + if (timeoutHandle) clearTimeout(timeoutHandle); + if (signal) signal.removeEventListener("abort", onAbort); + reject(err); + }); }); }, }; diff --git a/packages/coding-agent/src/utils/child-process.ts b/packages/coding-agent/src/utils/child-process.ts new file mode 100644 index 00000000..118027e6 --- /dev/null +++ b/packages/coding-agent/src/utils/child-process.ts @@ -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 { + 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); + }); +} diff --git a/packages/coding-agent/test/bash-close-hang-windows.test.ts b/packages/coding-agent/test/bash-close-hang-windows.test.ts new file mode 100644 index 00000000..ef8c7922 --- /dev/null +++ b/packages/coding-agent/test/bash-close-hang-windows.test.ts @@ -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(promise: Promise, ms: number, onTimeout: () => void): Promise { + return new Promise((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); + } + }); +});