fix(bash): returned on stalled cancellation

Raced bash execution against the JavaScript abort signal and timeout so the tool returns even when native shell cleanup does not settle.

Added regression coverage for native cleanup stalls on ESC abort and timeout.

Fixes #1347
This commit is contained in:
roboomp
2026-05-25 18:03:09 +00:00
parent 3b072a10b9
commit 14c7ddf7d0
3 changed files with 90 additions and 14 deletions
+1
View File
@@ -5,6 +5,7 @@
### Fixed
- Fixed clipboard image paste (Ctrl+V) silently failing on WSL2 by routing image reads through a `powershell.exe` bridge when WSL interop is detected, since `arboard` returns `ContentNotAvailable` under WSLg ([#1280](https://github.com/can1357/oh-my-pi/issues/1280))
- Fixed `bash` tool timeout and ESC cancellation getting stuck when native shell cleanup stalls; the JavaScript-side deadline now returns the tool result on schedule while native cleanup continues in the background ([#1347](https://github.com/can1357/oh-my-pi/issues/1347))
## [15.2.4] - 2026-05-22
### Breaking Changes
+25 -14
View File
@@ -48,8 +48,6 @@ export interface BashResult {
artifactId?: string;
}
const HARD_TIMEOUT_GRACE_MS = 5_000;
const shellSessions = new Map<string, Shell>();
const brokenShellSessions = new Set<string>();
@@ -106,8 +104,9 @@ export async function executeBash(command: string, options?: BashExecutorOptions
// sink.push() is synchronous — buffer management, counters, and onChunk
// all run inline. File writes (artifact path) are handled asynchronously
// inside the sink. No promise chain needed.
let acceptingChunks = true;
const enqueueChunk = (chunk: string) => {
sink.push(chunk);
if (acceptingChunks) sink.push(chunk);
};
if (options?.signal?.aborted) {
@@ -144,21 +143,22 @@ export async function executeBash(command: string, options?: BashExecutorOptions
void shellSession.abort();
}
};
const abortDeferred = Promise.withResolvers<"abort">();
const abortHandler = () => {
abortCurrentExecution();
abortDeferred.resolve("abort");
};
if (userSignal) {
userSignal.addEventListener("abort", abortHandler, { once: true });
}
let hardTimeoutTimer: NodeJS.Timeout | undefined;
const hardTimeoutDeferred = Promise.withResolvers<"hard-timeout">();
let timeoutTimer: NodeJS.Timeout | undefined;
const timeoutDeferred = Promise.withResolvers<"timeout">();
const baseTimeoutMs = Math.max(1_000, options?.timeout ?? 300_000);
const hardTimeoutMs = baseTimeoutMs + HARD_TIMEOUT_GRACE_MS;
hardTimeoutTimer = setTimeout(() => {
timeoutTimer = setTimeout(() => {
abortCurrentExecution();
hardTimeoutDeferred.resolve("hard-timeout");
}, hardTimeoutMs);
timeoutDeferred.resolve("timeout");
}, baseTimeoutMs);
let resetSession = false;
@@ -198,10 +198,13 @@ export async function executeBash(command: string, options?: BashExecutorOptions
const winner = await Promise.race([
runPromise.then(result => ({ kind: "result" as const, result })),
hardTimeoutDeferred.promise.then(() => ({ kind: "hard-timeout" as const })),
timeoutDeferred.promise.then(kind => ({ kind })),
abortDeferred.promise.then(kind => ({ kind })),
]);
if (winner.kind === "hard-timeout") {
if (winner.kind === "timeout" || winner.kind === "abort") {
acceptingChunks = false;
void runPromise.catch(() => undefined);
if (shellSession) {
resetSession = true;
// Fall back to one-shot execution for the rest of the process once
@@ -211,9 +214,17 @@ export async function executeBash(command: string, options?: BashExecutorOptions
return {
exitCode: undefined,
cancelled: true,
...(await sink.dump(`Command exceeded hard timeout after ${Math.round(hardTimeoutMs / 1000)} seconds`)),
...(await sink.dump(
winner.kind === "timeout"
? `Command timed out after ${Math.round(baseTimeoutMs / 1000)} seconds`
: "Command cancelled",
)),
};
}
if (timeoutTimer) {
clearTimeout(timeoutTimer);
timeoutTimer = undefined;
}
// Handle timeout
if (winner.result.timedOut) {
@@ -268,8 +279,8 @@ export async function executeBash(command: string, options?: BashExecutorOptions
resetSession = true;
throw err;
} finally {
if (hardTimeoutTimer) {
clearTimeout(hardTimeoutTimer);
if (timeoutTimer) {
clearTimeout(timeoutTimer);
}
if (userSignal) {
userSignal.removeEventListener("abort", abortHandler);
@@ -6,6 +6,7 @@ import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config
import { 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 * as piNatives from "@oh-my-pi/pi-natives";
// Matches the schema default for `tools.artifactHeadBytes` (20 KB) used by
// OutputSink when bash-executor pulls settings via resolveOutputSinkHeadBytes.
@@ -164,6 +165,69 @@ describe("executeBash", () => {
expect(result.output).toContain("Command cancelled");
});
it("returns promptly when native abort cleanup stalls", async () => {
if (process.platform === "win32") {
return;
}
vi.spyOn(piNatives.Shell.prototype, "run").mockImplementation((_options, onChunk) => {
onChunk?.(null, "started\n");
return new Promise(() => {});
});
const abortSpy = vi.spyOn(piNatives.Shell.prototype, "abort").mockResolvedValue();
const controller = new AbortController();
const promise = executeBash("sleep 10", {
cwd: tempDir,
timeout: 5000,
signal: controller.signal,
sessionKey: "hung-native-abort",
});
await Bun.sleep(50);
controller.abort();
const raced = await Promise.race([
promise.then(result => ({ type: "result" as const, result })),
Bun.sleep(750).then(() => ({ type: "timeout" as const })),
]);
expect(raced.type).toBe("result");
if (raced.type === "result") {
expect(raced.result.cancelled).toBe(true);
expect(raced.result.output).toContain("Command cancelled");
}
expect(abortSpy).toHaveBeenCalled();
});
it("returns at the JavaScript timeout when native timeout cleanup stalls", async () => {
if (process.platform === "win32") {
return;
}
vi.spyOn(piNatives.Shell.prototype, "run").mockImplementation((_options, onChunk) => {
onChunk?.(null, "started\n");
return new Promise(() => {});
});
const abortSpy = vi.spyOn(piNatives.Shell.prototype, "abort").mockResolvedValue();
const promise = executeBash("sleep 10", {
cwd: tempDir,
timeout: 1000,
sessionKey: "hung-native-timeout",
});
const raced = await Promise.race([
promise.then(result => ({ type: "result" as const, result })),
Bun.sleep(1500).then(() => ({ type: "timeout" as const })),
]);
expect(raced.type).toBe("result");
if (raced.type === "result") {
expect(raced.result.cancelled).toBe(true);
expect(raced.result.output).toContain("Command timed out after 1 seconds");
}
expect(abortSpy).toHaveBeenCalled();
});
it("aborts before follow-up output", async () => {
if (process.platform === "win32") {
return;