From 06aa6563e4d51d6df5de6c9664c3a868f3f5bb2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Hou=C5=A1ka?= Date: Wed, 4 Nov 2026 23:47:16 +0100 Subject: [PATCH] fix(coding-agent): report the shell exit code when bash times out after the shell exited (#11813) Fixes #11613. ## Problem The bash tool waits for stdout/stderr to close, not only for the shell to exit (#5303). A background process that inherited those pipes and keeps writing keeps the tool waiting after the shell exited. When the timeout then hits, the tool reported only `Command timed out after N seconds`. Example, timeout 3s: ```bash bash -c 'while true; do echo tick; sleep 0.05; done' & sleep 1; echo boom >&2; exit 1 ``` The shell exits with code 1 after ~1s. The tool still times out at the deadline, because the loop is still writing to its output. Both facts are true: the command failed with code 1, and something it started was still running at the timeout. The old message dropped the first, so the model read a failed command as a slow one and retried with a longer timeout, which never helps here. ## Fix The timeout stays a timeout, since the tool did hit its deadline while background processes were still writing. When the shell had already exited before the deadline, the message now also says so: ``` Command timed out after 3 seconds. The shell exited with code 1 after 1.1 seconds, but background processes it started kept writing output until the timeout. ``` A shell still running at the deadline keeps the plain `Command timed out after N seconds`. The local backend throws a module-private `ShellTimeoutError` with the message `timeout:` as before, so custom `BashOperations` that throw `timeout:N` are unaffected. ## Behavior per platform - Linux and macOS: the shell runs in its own process group (`detached: true`). At the deadline `kill(-pgid)` still reaches the background processes after the shell exited, the pipes close, and the tool returns at the deadline. Tested on Linux; macOS uses the same mechanism and matches the macOS repros in the issue. - Windows: still not fully solved. At the deadline `taskkill /T /PID ` fails with `process not found`, because the shell pid is gone. Walking parent pids does not help: Git's `bin\bash.exe` is a launcher for `usr\bin\bash.exe`, and the orphaned loop's parent is that already-exited inner bash, not the pid pi holds. The tool keeps reading until the output pauses for 100ms. Measured with Git Bash: the message is now correct, but the tool returned after 14s instead of 3s; with a loop that never pauses it never returns. This PR only fixes the reporting. ## Same pattern elsewhere - `packages/durable/src/env/node.ts` (`NodeExecutionEnv.exec`): same code, same false `timeout:N` after the shell exited. Same Windows limitation. - `packages/coding-agent/src/core/exec.ts` (`pi.exec` for extensions): the timeout sends SIGTERM/SIGKILL to the direct child only, no process group. A descendant that keeps writing keeps the call waiting past the timeout on every platform, and the result has `killed: true` even when the child had already exited with its own code. - `packages/coding-agent/src/core/package-manager.ts` (capture commands for git/npm): waits for `close` and kills only the direct child on timeout, so a descendant holding the pipes makes the timeout ineffective on every platform, even when the descendant writes nothing. - `packages/mcp/src/transports/stdio.ts`: on Windows, closing a server that already exited skips taskkill, so children holding its pipes survive. The code documents this as a known limitation. Possible follow-up for Windows and the cases above: once the deadline passed, stop waiting for the pipes (destroy our ends). On Windows this also ends most writers, which die from a broken pipe (tested with Git Bash). ## Tests - `test/suite/regressions/11613-bash-timeout-after-shell-exit.test.ts` (Unix only; it would hang on Windows for the reason above): background writer after the shell exits with code 3, and a shell still running at the deadline. The first case fails without the fix. - Checked manually on Windows 11 with Git Bash through the built bundle: both messages are as above. - `npm run check`, `test/tools.test.ts`, the #5303 and #5208 regressions, `bash-close-hang-windows`, `powershell-tool`, `shell-kill-process-tree`. This PR description is AI-generated. --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/core/tools/bash.ts | 45 ++++++++++++++++--- ...1613-bash-timeout-after-shell-exit.test.ts | 43 ++++++++++++++++++ 3 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 packages/coding-agent/test/suite/regressions/11613-bash-timeout-after-shell-exit.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index de056596d..b9b6ea383 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,7 @@ ### Fixed +- Fixed the bash tool reporting only `Command timed out` when the command had already exited but a background process it started kept writing to its output until the timeout. The error now also reports the shell's exit code and when it exited ([#11613](https://github.com/earendil-works/pi/issues/11613)) - Images in steering and follow-up messages, including `prompt()` while streaming, are now resized like images in idle prompts instead of being sent and saved at full size ([#11738](https://github.com/earendil-works/pi/issues/11738)) - A failed token refresh of an MCP connection no longer registers an OAuth client or replaces the PKCE verifier of a sign-in in progress, which made that sign-in fail. MCP sign-in now refreshes the stored tokens with the stored client even when the loopback callback port changed, instead of registering a new client and opening the browser. Ctrl+C in `/mcp` and the `--timeout` of `pi mcp login` now also stop a sign-in while it waits up to 25 seconds for another pi process's token refresh, and a refresh lock that fails for another reason than that wait now fails the sign-in instead of opening the browser. - Fixed `pi mcp list` reporting MCP servers with `"auth": { "provider": "" }`, such as Radius, as needing sign-in although the provider was signed in: the shell command sent no token. It now sends the provider's `/login` token and refreshes it when it expired. For these servers the sign-in hint names `/login ` instead of `pi mcp login`, and `pi mcp login` and `pi mcp logout` point to `/login` and `/logout` instead of failing with "does not use OAuth" diff --git a/packages/coding-agent/src/core/tools/bash.ts b/packages/coding-agent/src/core/tools/bash.ts index 2084d6f7c..92de2d09b 100644 --- a/packages/coding-agent/src/core/tools/bash.ts +++ b/packages/coding-agent/src/core/tools/bash.ts @@ -92,6 +92,20 @@ export interface BashOperations { ) => Promise<{ exitCode: number | null }>; } +/** + * Timeout of the local shell backend. Its message keeps the `timeout:` form custom operations throw. + * `shellExit` is set when the shell itself exited before the deadline, so the tool was only still waiting + * because background processes kept writing to the inherited stdout/stderr pipes (#11613). + */ +class ShellTimeoutError extends Error { + readonly shellExit: { exitCode: number; afterSeconds: number } | undefined; + + constructor(timeout: number | undefined, shellExit: { exitCode: number; afterSeconds: number } | undefined) { + super(`timeout:${timeout}`); + this.shellExit = shellExit; + } +} + /** Shared process execution used by the built-in shell tools. */ export function createLocalShellOperations(shellName: string, resolveShellConfig: () => ShellConfig): BashOperations { return { @@ -120,7 +134,13 @@ export function createLocalShellOperations(shellName: string, resolveShellConfig child.stdin?.end(command); } if (child.pid) trackDetachedChildPid(child.pid); + const startedAt = performance.now(); + let shellExitedAfterMs: number | undefined; + child.once("exit", () => { + shellExitedAfterMs = performance.now() - startedAt; + }); let timedOut = false; + let exitedBeforeDeadlineAfterMs: number | undefined; let timeoutHandle: NodeJS.Timeout | undefined; const onAbort = () => { if (child.pid) killProcessTree(child.pid); @@ -131,6 +151,9 @@ export function createLocalShellOperations(shellName: string, resolveShellConfig if (timeoutMs !== undefined) { timeoutHandle = setTimeout(() => { timedOut = true; + exitedBeforeDeadlineAfterMs = shellExitedAfterMs; + // Also when the shell already exited: on Unix the group kill still reaches background + // processes holding the pipes. Windows cannot find them once the shell pid is gone. if (child.pid) killProcessTree(child.pid); }, timeoutMs); } @@ -148,13 +171,19 @@ export function createLocalShellOperations(shellName: string, resolveShellConfig if (signal?.aborted) { throw new Error("aborted"); } - if (timedOut) { - throw new Error(`timeout:${timeout}`); - } // A signal-killed shell has no exit code. Use the standard shell convention so // callers do not mistake the termination for a successful command. const signalCode = child.signalCode; - return { exitCode: exitCode ?? (signalCode ? 128 + (osConstants.signals[signalCode] ?? 0) : 1) }; + const resolvedExitCode = exitCode ?? (signalCode ? 128 + (osConstants.signals[signalCode] ?? 0) : 1); + if (timedOut) { + throw new ShellTimeoutError( + timeout, + exitedBeforeDeadlineAfterMs !== undefined + ? { exitCode: resolvedExitCode, afterSeconds: Math.round(exitedBeforeDeadlineAfterMs / 100) / 10 } + : undefined, + ); + } + return { exitCode: resolvedExitCode }; } finally { if (child.pid) untrackDetachedChildPid(child.pid); if (timeoutHandle) clearTimeout(timeoutHandle); @@ -375,7 +404,13 @@ export function createShellToolDefinition( } if (err instanceof Error && err.message.startsWith("timeout:")) { const timeoutSecs = err.message.split(":")[1]; - throw new Error(appendStatus(text, `Command timed out after ${timeoutSecs} seconds`)); + let status = `Command timed out after ${timeoutSecs} seconds`; + if (err instanceof ShellTimeoutError && err.shellExit) { + // Both facts matter: the command's real result, and that its background processes + // were still writing output when the timeout hit (#11613). + status += `. The shell exited with code ${err.shellExit.exitCode} after ${err.shellExit.afterSeconds} seconds, but background processes it started kept writing output until the timeout.`; + } + throw new Error(appendStatus(text, status)); } throw err; } diff --git a/packages/coding-agent/test/suite/regressions/11613-bash-timeout-after-shell-exit.test.ts b/packages/coding-agent/test/suite/regressions/11613-bash-timeout-after-shell-exit.test.ts new file mode 100644 index 000000000..8ea5596df --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/11613-bash-timeout-after-shell-exit.test.ts @@ -0,0 +1,43 @@ +import { tmpdir } from "node:os"; +import { describe, expect, it } from "vitest"; +import { createBashTool } from "../../../src/core/tools/bash.ts"; + +/** + * Regression test for https://github.com/earendil-works/pi/issues/11613 + * + * The bash tool waits for stdout/stderr to close, not only for the shell to exit (#5303). A background + * process that inherited those pipes and keeps writing keeps the tool waiting after the shell exited. + * When the timeout then hit, the tool reported only "Command timed out", so the shell's real exit code + * and the fact that it had already finished were lost. The tool now reports both. + * + * Unix only: on Windows the deadline cannot kill background processes once the shell pid is gone, so the + * tool keeps waiting until their output pauses; this test would hang there. + */ +describe.skipIf(process.platform === "win32")("issue #11613 bash timeout after the shell exited", () => { + const bashTool = createBashTool(tmpdir()); + + async function runExpectingTimeout(command: string, timeout: number): Promise { + const error = await bashTool.execute("test-11613", { command, timeout }).then( + () => undefined, + (err: unknown) => err, + ); + expect(error).toBeInstanceOf(Error); + return (error as Error).message; + } + + it("reports the shell's exit code when background processes kept writing until the timeout", async () => { + const message = await runExpectingTimeout( + "(while true; do echo tick; sleep 0.02; done) & sleep 0.2; echo boom >&2; exit 3", + 1, + ); + expect(message).toContain("boom"); + expect(message).toMatch( + /Command timed out after 1 seconds\. The shell exited with code 3 after 0\.\d seconds, but background processes it started kept writing output until the timeout\.$/, + ); + }); + + it("keeps the plain timeout message when the shell was still running at the deadline", async () => { + const message = await runExpectingTimeout("echo started; sleep 30", 0.3); + expect(message).toMatch(/started\s+Command timed out after 0\.3 seconds$/); + }); +});