From a9e6e4e67ee1878b8ade02c915af9db45342bcef Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 19:57:59 +0000 Subject: [PATCH] fix(bash): aborted isolated shells on cancellation Route overlapping executions through owned Shell instances so timeout and interrupt paths can explicitly abort native child-process cleanup. Fixes #5389 --- packages/coding-agent/CHANGELOG.md | 4 + .../coding-agent/src/exec/bash-executor.ts | 73 ++++++++----------- .../coding-agent/test/bash-executor.test.ts | 53 +++++++++++++- 3 files changed, 83 insertions(+), 47 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index da3550531..277fb8316 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed overlapping Bash timeout and interrupt cleanup to explicitly abort isolated shells instead of leaving child processes running ([#5389](https://github.com/can1357/oh-my-pi/issues/5389)). + ## [16.5.0] - 2026-07-13 ### Breaking Changes diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index 505feca96..44d398d3c 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -4,7 +4,7 @@ * Uses brush-core via native bindings for shell execution. */ import { ExponentialYield } from "@oh-my-pi/pi-agent-core/utils/yield"; -import { executeShell, type MinimizerOptions, Shell, type ShellRunResult } from "@oh-my-pi/pi-natives"; +import { type MinimizerOptions, Shell, type ShellRunResult } from "@oh-my-pi/pi-natives"; import { isExecutable, type ShellConfig } from "@oh-my-pi/pi-utils/procmgr"; import { Settings, type ShellMinimizerSettings } from "../config/settings"; import { OutputSink } from "../session/streaming-output"; @@ -250,6 +250,11 @@ export async function executeBash(command: string, options?: BashExecutorOptions }; } + const shellOptions = { + sessionEnv: shellEnv, + snapshotPath: snapshotPath ?? undefined, + minimizer, + }; const sessionKey = buildSessionKey(shell, prefix, snapshotPath, shellEnv, options?.sessionKey, minimizer); const persistentSessionBroken = brokenShellSessions.has(sessionKey); if (persistentSessionBroken) { @@ -264,13 +269,10 @@ export async function executeBash(command: string, options?: BashExecutorOptions const sessionBusy = shellSessionsInUse.has(sessionKey); let shellSession = persistentSessionBroken || sessionBusy ? undefined : shellSessions.get(sessionKey); if (!shellSession && !persistentSessionBroken && !sessionBusy) { - shellSession = new Shell({ - sessionEnv: shellEnv, - snapshotPath: snapshotPath ?? undefined, - minimizer, - }); + shellSession = new Shell(shellOptions); shellSessions.set(sessionKey, shellSession); } + const executionShell = shellSession ?? new Shell(shellOptions); const ownsPersistentSession = shellSession !== undefined; if (ownsPersistentSession) { shellSessionsInUse.add(sessionKey); @@ -278,13 +280,15 @@ export async function executeBash(command: string, options?: BashExecutorOptions const userSignal = options?.signal; const runAbortController = new AbortController(); let abortCleanupPromise: Promise | undefined; + const abortShell = (): Promise => { + abortCleanupPromise ??= executionShell.abort().catch(() => undefined); + return abortCleanupPromise; + }; const abortCurrentExecution = () => { if (!runAbortController.signal.aborted) { runAbortController.abort(); } - if (shellSession && !abortCleanupPromise) { - abortCleanupPromise = shellSession.abort().catch(() => undefined); - } + void abortShell(); }; const abortDeferred = Promise.withResolvers<"abort">(); const abortHandler = () => { @@ -317,38 +321,20 @@ export async function executeBash(command: string, options?: BashExecutorOptions let resetSession = false; try { - const runPromise = shellSession - ? shellSession.run( - { - command: finalCommand, - cwd: commandCwd, - env: commandEnv, - timeoutMs: nativeTimeoutMs, - signal: runAbortController.signal, - }, - (err, chunk) => { - if (!err) { - enqueueChunk(chunk); - } - }, - ) - : executeShell( - { - command: finalCommand, - cwd: commandCwd, - env: commandEnv, - sessionEnv: shellEnv, - snapshotPath: snapshotPath ?? undefined, - minimizer, - timeoutMs: nativeTimeoutMs, - signal: runAbortController.signal, - }, - (err, chunk) => { - if (!err) { - enqueueChunk(chunk); - } - }, - ); + const runPromise = executionShell.run( + { + command: finalCommand, + cwd: commandCwd, + env: commandEnv, + timeoutMs: nativeTimeoutMs, + signal: runAbortController.signal, + }, + (err, chunk) => { + if (!err) { + enqueueChunk(chunk); + } + }, + ); const ey = new ExponentialYield(); const winner = await ey.race< @@ -361,11 +347,12 @@ export async function executeBash(command: string, options?: BashExecutorOptions if (winner.kind === "timeout" || winner.kind === "abort") { acceptingChunks = false; + const cleanupPromise = abortShell(); if (shellSession) { resetSession = true; - quarantineShellSession(sessionKey, runPromise, abortCleanupPromise); + quarantineShellSession(sessionKey, runPromise, cleanupPromise); } else { - void runPromise.catch(() => undefined); + void Promise.allSettled([runPromise, cleanupPromise]); } return { exitCode: undefined, diff --git a/packages/coding-agent/test/bash-executor.test.ts b/packages/coding-agent/test/bash-executor.test.ts index f428d3ea9..ab7aea794 100644 --- a/packages/coding-agent/test/bash-executor.test.ts +++ b/packages/coding-agent/test/bash-executor.test.ts @@ -6,7 +6,7 @@ import { resetSettingsForTest, Settings, type ShellMinimizerSettings } from "@oh import { buildMinimizerOptions, executeBash } from "@oh-my-pi/pi-coding-agent/exec/bash-executor"; import { DEFAULT_MAX_BYTES } from "@oh-my-pi/pi-coding-agent/session/streaming-output"; import * as shellSnapshot from "@oh-my-pi/pi-coding-agent/utils/shell-snapshot"; -import type { Shell } from "@oh-my-pi/pi-natives"; +import type { Shell, ShellRunResult } from "@oh-my-pi/pi-natives"; import * as piNatives from "@oh-my-pi/pi-natives"; import { removeSyncWithRetries } from "@oh-my-pi/pi-utils"; @@ -475,7 +475,7 @@ exit 64 sessionKey: "hung-native-abort", }); expect(next.output.trim()).toBe("next"); - expect(runCalls).toBe(1); + expect(runCalls).toBe(2); }); it("restores persistent sessions after native abort cleanup settles", async () => { @@ -520,7 +520,7 @@ exit 64 expect(next.output.trim()).toBe("still_persistent"); }); - it("does not abort the native signal when the JavaScript timeout fallback returns streamed output", async () => { + it("aborts the shell without aborting its native signal when the JavaScript timeout fallback wins", async () => { // Compress the JS-side fallback timer (floored at 1000ms in the source) so // the safety-net fires deterministically without a real 1s wait. Only long // timers are shrunk — fs/subprocess setup keeps real scheduling — and the @@ -554,7 +554,52 @@ exit 64 expect(result.output).toContain("Command timed out after 1 seconds"); expect(nativeSignal).toBeDefined(); expect(nativeSignal?.aborted).toBe(false); - expect(abortSpy).not.toHaveBeenCalled(); + expect(abortSpy).toHaveBeenCalledTimes(1); + }); + + it("explicitly aborts an overlapping one-shot shell when timeout cleanup stalls", async () => { + const realSetTimeout = globalThis.setTimeout; + vi.spyOn(globalThis, "setTimeout").mockImplementation(((handler: () => void, ms?: number, ...rest: unknown[]) => + realSetTimeout( + handler, + typeof ms === "number" && ms >= 1000 ? 5 : ms, + ...rest, + )) as typeof globalThis.setTimeout); + + const ownerResult = Promise.withResolvers(); + const isolatedResult = Promise.withResolvers(); + const ownerDispatched = Promise.withResolvers(); + const isolatedDispatched = Promise.withResolvers(); + vi.spyOn(piNatives.Shell.prototype, "run").mockImplementation(options => { + if (options.command === "owner") { + ownerDispatched.resolve(); + return ownerResult.promise; + } + isolatedDispatched.resolve(); + return isolatedResult.promise; + }); + const abortSpy = vi.spyOn(piNatives.Shell.prototype, "abort").mockResolvedValue(); + + const owner = executeBash("owner", { + cwd: tempDir, + timeout: 0, + sessionKey: "stalled-one-shot-timeout", + }); + await ownerDispatched.promise; + const overlapping = executeBash("isolated", { + cwd: tempDir, + timeout: 1000, + sessionKey: "stalled-one-shot-timeout", + }); + await isolatedDispatched.promise; + + const result = await overlapping; + expect(result.cancelled).toBe(true); + expect(abortSpy).toHaveBeenCalledTimes(1); + + isolatedResult.resolve({ exitCode: undefined, cancelled: true, timedOut: true }); + ownerResult.resolve({ exitCode: 0, cancelled: false, timedOut: false }); + await owner; }); it("aborts before follow-up output", async () => {