From 4eb90bf874a2e2ce6be1ca118efd5f12126c0043 Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Thu, 2 Jul 2026 17:25:50 +0900 Subject: [PATCH 1/2] fix(bash): bound ACP terminal lifecycle with abort/timeout ACP terminal RPCs in `BashTool.execute` were awaited without abort or timeout guards, so a hung `createTerminal`, `kill`, or `currentOutput` call could keep the tool alive past its configured timeout. The ACP polling loop also recreated `exitPromise`/`abortedP` `.then()` closures on every 250 ms tick, leaking memory during long-running commands. This change moves abort/timeout setup before `createTerminal`, races it against the timeout/abort signals, hoists the exit/abort racers outside the loop, tracks `lastPolledOutput`, and bounds kill and final-output waits with fallbacks; late-resolving `createTerminal` handles are killed and released. Verified with `bun run check:types` and `bun test test/bash-acp-terminal.test.ts` in `packages/coding-agent` (6 pass, 0 fail). Closes #4241 --- packages/coding-agent/CHANGELOG.md | 5 + packages/coding-agent/src/tools/bash.ts | 287 +++++++++++------- .../test/bash-acp-terminal.test.ts | 118 ++++++- 3 files changed, 289 insertions(+), 121 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 11098023c..7f3fabfd1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### Fixed + +- Fix ACP terminal hang/leak by bounding createTerminal and RPC awaits with abort/timeout and cleaning up late-resolving terminals ([#4241](https://github.com/can1357/oh-my-pi/issues/4241)) + + ## [16.3.0] - 2026-07-02 ### Added diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index f90e3535b..f6191aff2 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -16,7 +16,11 @@ import { InternalUrlRouter } from "../internal-urls"; import { truncateToVisualLines } from "../modes/components/visual-truncate"; import { highlightCode, type Theme } from "../modes/theme/theme"; import bashDescription from "../prompts/tools/bash.md" with { type: "text" }; -import type { ClientBridgeTerminalExitStatus, ClientBridgeTerminalOutput } from "../session/client-bridge"; +import type { + ClientBridgeTerminalExitStatus, + ClientBridgeTerminalHandle, + ClientBridgeTerminalOutput, +} from "../session/client-bridge"; import { DEFAULT_MAX_BYTES, enforceInlineByteCap, streamTailUpdates, TailBuffer } from "../session/streaming-output"; import { renderStatusLine } from "../tui"; import { CachedOutputBlock, markFramedBlockComponent, outputBlockContentWidth } from "../tui/output-block"; @@ -841,41 +845,49 @@ export class BashTool implements AgentTool ({ name, value: value as string })) - : undefined, - outputByteLimit: DEFAULT_MAX_BYTES, - }); - - // Emit partial update so the editor can embed the live terminal card. - onUpdate?.({ content: [], details: { terminalId: handle.terminalId } }); - - const exitPromise = handle.waitForExit(); - let exitStatus!: ClientBridgeTerminalExitStatus; - - type BridgeRaceResult = - | { kind: "exit"; status: ClientBridgeTerminalExitStatus } - | { kind: "poll" } - | { kind: "timeout" } - | { kind: "aborted" }; - - // Set up abort listener before entering the poll loop. The listener - // kicks off `handle.kill()` synchronously so a `session/cancel` - // arriving mid-poll terminates the remote command immediately, - // instead of waiting for the next `currentOutput()` to return. + const killGraceMs = 1000; + const outputSnapshotGraceMs = 2000; + const timeoutPromise = Bun.sleep(timeoutMs).then(() => ({ kind: "timeout" as const })); const { promise: abortedP, resolve: resolveAborted } = Promise.withResolvers(); + let handle: ClientBridgeTerminalHandle | undefined; let killStarted = false; const fireKill = (): Promise => { if (killStarted) return Promise.resolve(); + const currentHandle = handle; + if (!currentHandle) return Promise.resolve(); killStarted = true; - return handle.kill().catch((error: unknown) => { - logger.warn("ACP terminal kill failed", { terminalId: handle.terminalId, error }); + return currentHandle.kill().catch((error: unknown) => { + logger.warn("ACP terminal kill failed", { terminalId: currentHandle.terminalId, error }); }); }; + const cleanupLateCreate = (createP: Promise): void => { + void createP + .then(async lateHandle => { + try { + await lateHandle.kill(); + } catch (error) { + logger.warn("ACP terminal kill failed", { terminalId: lateHandle.terminalId, error }); + } + try { + await lateHandle.release(); + } catch (error) { + logger.warn("ACP terminal release failed", { terminalId: lateHandle.terminalId, error }); + } + }) + .catch((error: unknown) => { + logger.warn("ACP terminal create failed after cancellation", { error }); + }); + }; const onAbortSignal = () => { resolveAborted(); void fireKill(); @@ -883,92 +895,150 @@ export class BashTool implements AgentTool ({ name, value: value as string })) + : undefined, + outputByteLimit: DEFAULT_MAX_BYTES, + }); + const createRaced = await Promise.race([ + createP.then(createdHandle => ({ kind: "created" as const, handle: createdHandle })), + timeoutPromise, + abortedP.then(() => ({ kind: "aborted" as const })), + ]); + if (createRaced.kind === "aborted" || signal?.aborted) { + cleanupLateCreate(createP); + throw new ToolAbortError("Command aborted"); + } + if (createRaced.kind === "timeout") { + cleanupLateCreate(createP); + const timedOutResult: BashInteractiveResult = { + output: "", + exitCode: undefined, + cancelled: false, + timedOut: true, + truncated: false, + totalLines: 0, + totalBytes: 0, + outputLines: 0, + outputBytes: 0, + }; + return this.#buildCompletedResult(timedOutResult, timeoutSec, { + requestedTimeoutSec, + notices: pendingNotices, + wallTimeMs: performance.now() - bridgeWallTimeStart, + }); + } + + handle = createRaced.handle; + + // Emit partial update so the editor can embed the live terminal card. + onUpdate?.({ content: [], details: { terminalId: handle.terminalId } }); + + const exitPromise = handle.waitForExit(); + let exitStatus!: ClientBridgeTerminalExitStatus; + + type BridgeRaceResult = + | { kind: "exit"; status: ClientBridgeTerminalExitStatus } + | { kind: "poll" } + | { kind: "timeout" } + | { kind: "aborted" }; + + const exitRacer = exitPromise.then(status => ({ kind: "exit" as const, status })); + const abortRacer = abortedP.then(() => ({ kind: "aborted" as const })); + const abortPollRacer = abortedP.then(() => undefined as ClientBridgeTerminalOutput | undefined); + let lastPolledOutput: ClientBridgeTerminalOutput = { output: "", truncated: false }; + + // Poll until the process exits, times out, or the caller aborts. + for (;;) { + const racers: Array> = [ + exitRacer, + timeoutPromise, + Bun.sleep(250).then(() => ({ kind: "poll" as const })), + ]; + if (signal) { + racers.push(abortRacer); + } + const raced = await Promise.race(racers); + + if (raced.kind === "aborted" || signal?.aborted) { + await Promise.race([fireKill(), Bun.sleep(killGraceMs)]); throw new ToolAbortError("Command aborted"); } - const timeoutPromise = Bun.sleep(timeoutMs).then(() => ({ kind: "timeout" as const })); - // Poll until the process exits, times out, or the caller aborts. - for (;;) { - const racers: Array> = [ - exitPromise.then(s => ({ kind: "exit" as const, status: s })), - timeoutPromise, - Bun.sleep(250).then(() => ({ kind: "poll" as const })), - ]; - if (signal) { - racers.push(abortedP.then(() => ({ kind: "aborted" as const }))); - } - const raced = await Promise.race(racers); - - if (raced.kind === "aborted" || signal?.aborted) { - await fireKill(); - throw new ToolAbortError("Command aborted"); - } - - if (raced.kind === "timeout") { - // Kill before reading final output so a slow `terminal/output` - // RPC cannot let a timed-out command keep running past the - // enforced timeout. The handle stays valid post-kill so the - // buffered output is still readable. - await fireKill(); - let current = { output: "", truncated: false }; - try { - current = await handle.currentOutput(); - } catch (error) { - logger.warn("ACP terminal final output read failed", { - terminalId: handle.terminalId, - error, - }); - } - const timedOutResult: BashInteractiveResult = { - output: current.output, - exitCode: undefined, - cancelled: false, - timedOut: true, - truncated: current.truncated, - totalLines: current.output.length > 0 ? current.output.split("\n").length : 0, - totalBytes: current.output.length, - outputLines: current.output.length > 0 ? current.output.split("\n").length : 0, - outputBytes: current.output.length, - }; - return this.#buildCompletedResult(timedOutResult, timeoutSec, { - requestedTimeoutSec, - notices: pendingNotices, + if (raced.kind === "timeout") { + // Kill before reading final output so a slow `terminal/output` + // RPC cannot let a timed-out command keep running past the + // enforced timeout. The handle stays valid post-kill so the + // buffered output is still readable. + await Promise.race([fireKill(), Bun.sleep(killGraceMs)]); + let current = lastPolledOutput; + try { + current = await Promise.race([ + handle.currentOutput(), + Bun.sleep(outputSnapshotGraceMs).then(() => lastPolledOutput), + ]); + } catch (error) { + logger.warn("ACP terminal final output read failed", { terminalId: handle.terminalId, - wallTimeMs: performance.now() - bridgeWallTimeStart, + error, }); } - - if (raced.kind === "exit") { - exitStatus = raced.status; - break; - } - - // Poll tick: push current output so agent-loop transcript stays consistent. - // Race the read against abort so a stuck `terminal/output` RPC does not - // delay cancellation. - const pollOutput = await Promise.race([ - handle.currentOutput(), - abortedP.then(() => undefined as ClientBridgeTerminalOutput | undefined), - ]); - if (pollOutput === undefined) { - // Abort fired during the poll-tick read; let the next loop iteration - // observe `signal?.aborted` and exit via the abort branch. - continue; - } - onUpdate?.({ - content: [{ type: "text", text: pollOutput.output }], - details: { terminalId: handle.terminalId }, + const timedOutResult: BashInteractiveResult = { + output: current.output, + exitCode: undefined, + cancelled: false, + timedOut: true, + truncated: current.truncated, + totalLines: current.output.length > 0 ? current.output.split("\n").length : 0, + totalBytes: current.output.length, + outputLines: current.output.length > 0 ? current.output.split("\n").length : 0, + outputBytes: current.output.length, + }; + return this.#buildCompletedResult(timedOutResult, timeoutSec, { + requestedTimeoutSec, + notices: pendingNotices, + terminalId: handle.terminalId, + wallTimeMs: performance.now() - bridgeWallTimeStart, }); } - } finally { - signal?.removeEventListener("abort", onAbortSignal); + + if (raced.kind === "exit") { + exitStatus = raced.status; + break; + } + + // Poll tick: push current output so agent-loop transcript stays consistent. + // Race the read against abort so a stuck `terminal/output` RPC does not + // delay cancellation. + const pollOutput = await Promise.race([handle.currentOutput(), abortPollRacer]); + if (pollOutput === undefined) { + // Abort fired during the poll-tick read; let the next loop iteration + // observe `signal?.aborted` and exit via the abort branch. + continue; + } + lastPolledOutput = pollOutput; + onUpdate?.({ + content: [{ type: "text", text: pollOutput.output }], + details: { terminalId: handle.terminalId }, + }); } // Fetch final output; the terminal is released in the outer finally. - const finalOutput = await handle.currentOutput(); + let finalOutput = lastPolledOutput; + try { + finalOutput = await Promise.race([ + handle.currentOutput(), + Bun.sleep(outputSnapshotGraceMs).then(() => lastPolledOutput), + ]); + } catch (error) { + logger.warn("ACP terminal final output read failed", { + terminalId: handle.terminalId, + error, + }); + } // Map exit status: null exitCode with a signal → treat as signal kill (137). const rawExitCode = exitStatus.exitCode; @@ -1001,10 +1071,13 @@ export class BashTool implements AgentTool { expect(releaseSpy).toHaveBeenCalledTimes(1); }); - it("releases the client terminal when final output retrieval fails", async () => { + it("does not allocate a client terminal when the signal is already aborted before createTerminal", async () => { + const handle: ClientBridgeTerminalHandle = { + terminalId: "term-should-not-create", + waitForExit: async () => ({ exitCode: 0, signal: null }), + currentOutput: async () => ({ output: "should not be reached", truncated: false }), + kill: async () => {}, + release: async () => {}, + }; + const bridge: ClientBridge = { + capabilities: { terminal: true }, + createTerminal: async () => handle, + }; + const createSpy = spyOn(bridge, "createTerminal"); + + const controller = new AbortController(); + controller.abort(); + + const tool = new BashTool(makeSession(bridge)); + + await expect(tool.execute("call-pre-abort", { command: "echo hi" }, controller.signal)).rejects.toThrow( + /Command aborted/, + ); + + expect(createSpy).toHaveBeenCalledTimes(0); + }); + + it("resolves using the last polled output when final output retrieval fails", async () => { + const pendingExit = Promise.withResolvers<{ exitCode: number | null; signal: string | null }>(); + let currentOutputCalls = 0; const handle: ClientBridgeTerminalHandle = { terminalId: "term-output-failure", - waitForExit: async () => ({ exitCode: 0, signal: null }), + waitForExit: async () => pendingExit.promise, currentOutput: async () => { + currentOutputCalls++; + if (currentOutputCalls === 1) { + // first poll loop iteration + setTimeout(() => pendingExit.resolve({ exitCode: 0, signal: null }), 0); + return { output: "polled text", truncated: false }; + } throw new Error("client output unavailable"); }, kill: async () => {}, @@ -98,9 +132,11 @@ describe("BashTool ACP terminal routing", () => { const tool = new BashTool(makeSession(bridge)); - await expect(tool.execute("call-output-failure", { command: "echo hi" })).rejects.toThrow( - /client output unavailable/, - ); + const result = await tool.execute("call-output-failure", { command: "echo hi" }); + + const text = result.content.find(c => c.type === "text"); + expect(text?.text).toContain("polled text"); // proves fallback + expect(result.isError).toBeUndefined(); expect(releaseSpy).toHaveBeenCalledTimes(1); }); @@ -128,12 +164,18 @@ describe("BashTool ACP terminal routing", () => { expect(releaseSpy).toHaveBeenCalledTimes(1); }); - it("kills and releases the client terminal when the command times out", async () => { + it("kills and releases the client terminal when the caller aborts", async () => { const pendingExit = Promise.withResolvers<{ exitCode: number | null; signal: string | null }>(); + const controller = new AbortController(); + const handle: ClientBridgeTerminalHandle = { - terminalId: "term-timeout", + terminalId: "term-abort", waitForExit: async () => pendingExit.promise, - currentOutput: async () => ({ output: "", truncated: false }), + currentOutput: async () => { + // Trigger abort during the poll loop, ensuring handle is assigned + controller.abort(); + return { output: "partial", truncated: false }; + }, kill: async () => {}, release: async () => {}, }; @@ -144,16 +186,64 @@ describe("BashTool ACP terminal routing", () => { const killSpy = spyOn(handle, "kill"); const releaseSpy = spyOn(handle, "release"); - spyOn(Bun, "sleep").mockImplementation(async () => {}); - const tool = new BashTool(makeSession(bridge)); - await expect(tool.execute("call-timeout", { command: "sleep 60", timeout: 1 })).rejects.toThrow( - /Command timed out after 1 seconds/, - ); + const executePromise = tool.execute("call-abort", { command: "sleep 60" }, controller.signal); + + await expect(executePromise).rejects.toThrow(/Command aborted/); + expect(killSpy).toHaveBeenCalledTimes(1); + expect(releaseSpy).toHaveBeenCalledTimes(1); + }); + + it("kills and releases the client terminal when the command times out", async () => { + const sleepSpy = spyOn(Bun, "sleep"); + let resolveTimeout!: () => void; + const fakeTimeoutPromise = new Promise(resolve => { + resolveTimeout = resolve; + }); + + sleepSpy.mockImplementation(ms => { + if (ms === 1000 && sleepSpy.mock.calls.length === 1) { + return fakeTimeoutPromise; + } + return Promise.resolve(); + }); + + const pendingExit = Promise.withResolvers<{ exitCode: number | null; signal: string | null }>(); + let killCalls = 0; + let currentOutputAfterKill = 0; + + const handle: ClientBridgeTerminalHandle = { + terminalId: "term-timeout", + waitForExit: async () => pendingExit.promise, + currentOutput: async () => { + // Trigger the timeout during the first poll + if (killCalls === 0) { + resolveTimeout(); + } else { + currentOutputAfterKill++; + } + return { output: "timeout output", truncated: false }; + }, + kill: async () => { + killCalls++; + }, + release: async () => {}, + }; + const bridge: ClientBridge = { + capabilities: { terminal: true }, + createTerminal: async () => handle, + }; + const killSpy = spyOn(handle, "kill"); + const releaseSpy = spyOn(handle, "release"); + + const tool = new BashTool(makeSession(bridge)); + const executePromise = tool.execute("call-timeout", { command: "sleep 60", timeout: 1 }); + + await expect(executePromise).rejects.toThrow(/Command timed out after 1 seconds/); expect(killSpy).toHaveBeenCalledTimes(1); expect(releaseSpy).toHaveBeenCalledTimes(1); - pendingExit.resolve({ exitCode: null, signal: "TERM" }); + expect(currentOutputAfterKill).toBeGreaterThan(0); }); }); From 9bc94b7b26ad402946888b2140c7939db8ae03d2 Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Thu, 2 Jul 2026 19:08:20 +0900 Subject: [PATCH 2/2] fix: address cubic review feedback (iteration 5) Cancel the ACP terminal timeout timer on completion: the hoisted Bun.sleep(timeoutMs) left a live ref'd timer for the full command timeout after every fast command, accumulating timers and delaying process shutdown in SDK/headless use. Switched to a cleared setTimeout, and rewrote the timeout regression test against real time instead of mocking Bun.sleep (the mock coupled the test to timer implementation and starved the event loop). --- packages/coding-agent/src/tools/bash.ts | 9 +++++++- .../test/bash-acp-terminal.test.ts | 23 ++++--------------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index f6191aff2..9c2a82836 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -857,7 +857,13 @@ export class BashTool implements AgentTool ({ kind: "timeout" as const })); + // Cancellable timeout: a bare Bun.sleep(timeoutMs) would leave a live, + // ref'd timer for the full command timeout after fast completions — + // accumulating timers and delaying process shutdown in SDK/headless use. + const { promise: timeoutPromise, resolve: resolveTimeout } = Promise.withResolvers<{ + kind: "timeout"; + }>(); + const timeoutTimer = setTimeout(() => resolveTimeout({ kind: "timeout" }), timeoutMs); const { promise: abortedP, resolve: resolveAborted } = Promise.withResolvers(); let handle: ClientBridgeTerminalHandle | undefined; let killStarted = false; @@ -1071,6 +1077,7 @@ export class BashTool implements AgentTool { }); it("kills and releases the client terminal when the command times out", async () => { - const sleepSpy = spyOn(Bun, "sleep"); - let resolveTimeout!: () => void; - const fakeTimeoutPromise = new Promise(resolve => { - resolveTimeout = resolve; - }); - - sleepSpy.mockImplementation(ms => { - if (ms === 1000 && sleepSpy.mock.calls.length === 1) { - return fakeTimeoutPromise; - } - return Promise.resolve(); - }); - + // Real 1s timeout — no Bun.sleep/setTimeout mocking. Mocking the timer + // implementation couples the test to how the timeout is scheduled and + // starves the event loop when the implementation changes. const pendingExit = Promise.withResolvers<{ exitCode: number | null; signal: string | null }>(); let killCalls = 0; let currentOutputAfterKill = 0; @@ -217,12 +207,7 @@ describe("BashTool ACP terminal routing", () => { terminalId: "term-timeout", waitForExit: async () => pendingExit.promise, currentOutput: async () => { - // Trigger the timeout during the first poll - if (killCalls === 0) { - resolveTimeout(); - } else { - currentOutputAfterKill++; - } + if (killCalls > 0) currentOutputAfterKill++; return { output: "timeout output", truncated: false }; }, kill: async () => {