diff --git a/crates/pi-natives/src/shell.rs b/crates/pi-natives/src/shell.rs index ccc0e07dd..4b80406f4 100644 --- a/crates/pi-natives/src/shell.rs +++ b/crates/pi-natives/src/shell.rs @@ -558,4 +558,31 @@ mod tests { .expect("shell run should return"); assert!(result.cancelled); } + + #[tokio::test(flavor = "multi_thread")] + async fn timeout_drains_pipeline_output_before_stopping_reader() { + let shell = CoreShell::new(None); + let (tx, rx) = flume::unbounded::(); + let result = shell + .run( + CoreShellRunOptions { + command: "yes x | tail -5".to_string(), + cwd: None, + env: None, + timeout_ms: Some(50), + }, + Some(tx), + CancelToken::new(Some(50)), + ) + .await + .expect("shell run"); + + let mut output = String::new(); + while let Ok(chunk) = rx.recv_async().await { + output.push_str(&chunk); + } + + assert!(result.timed_out); + assert_eq!(output.lines().filter(|line| *line == "x").count(), 5); + } } diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 60e3967c2..e30c98a6c 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -1141,11 +1141,16 @@ async fn run_shell_command_once( } } }); + // Let pipeline consumers flush output after cancellation kills their + // producers. The outer run cancellation remains bounded, and this delayed + // fallback still releases readers whose writers never close. + const CANCEL_READER_GRACE: Duration = Duration::from_millis(500); let cancel_bridge = tokio::spawn({ let cancel_token = cancel_token.clone(); let reader_cancel = reader_cancel.clone(); async move { cancel_token.cancelled().await; + time::sleep(CANCEL_READER_GRACE).await; reader_cancel.cancel(); } }); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8bc39746b..2f9d20ff2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -70,6 +70,10 @@ - Fixed ACP stdio EOF/EPIPE disconnects bypassing awaited session teardown and leaving in-flight tool calls pending in persisted rollouts ([#4788](https://github.com/can1357/oh-my-pi/issues/4788)). - Routed the print-mode assistant-error/aborted exit, RPC `pi.shutdown()` and stdin-EOF shutdowns, and the extension command-context `shutdown()` through the awaited, idempotent `session.dispose()` before `process.exit()`, so the bounded browser reaper (`releaseTabsForOwner`) always runs and OMP-owned Chromium no longer outlives the process ([#5643](https://github.com/can1357/oh-my-pi/issues/5643)). +### Fixed + +- Fixed Windows bash crashes when a piped command times out while flushing output; explicit-timeout watchdogs now wait for bounded native teardown instead of returning mid-drain. ([#5316](https://github.com/can1357/oh-my-pi/issues/5316)) + ### Removed - Fixed `/login` for paste-code providers (Codex, Anthropic, Gemini CLI, GitLab Duo, Antigravity, Devin) dropping the pasted fallback redirect URL: the login dialog captured focus but never mounted an input, and the "complete pairing with `/login `" tip pointed at the hidden, unfocused editor. The dialog now mounts a focused input for the manual code/URL paste ([#5339](https://github.com/can1357/oh-my-pi/issues/5339)). diff --git a/packages/coding-agent/src/exec/bash-executor.ts b/packages/coding-agent/src/exec/bash-executor.ts index 44d398d3c..d61f34d31 100644 --- a/packages/coding-agent/src/exec/bash-executor.ts +++ b/packages/coding-agent/src/exec/bash-executor.ts @@ -70,6 +70,9 @@ const shellSessionsInUse = new Set(); */ const retainedShells = new Set(); const RETAIN_REAP_INTERVAL_MS = 5_000; +// Native cancellation may spend two seconds unwinding the shell before its +// N-API chunk bridge drains. The JS watchdog must not race that teardown. +const NATIVE_TIMEOUT_FALLBACK_GRACE_MS = 5_000; async function retainShellWithLiveBackgroundJobs(shell: Shell): Promise { let live: number; @@ -306,16 +309,18 @@ export async function executeBash(command: string, options?: BashExecutorOptions const nativeTimeoutMs = requestedTimeoutMs !== undefined && requestedTimeoutMs > 0 ? requestedTimeoutMs : undefined; const nativeOwnsTimeout = nativeTimeoutMs !== undefined; if (deadlineTimeoutMs !== undefined) { + const fallbackTimeoutMs = nativeOwnsTimeout + ? deadlineTimeoutMs + NATIVE_TIMEOUT_FALLBACK_GRACE_MS + : deadlineTimeoutMs; timeoutTimer = setTimeout(() => { - // Explicit timeouts are already enforced inside pi-natives via - // `timeoutMs`. Do not also abort the JS AbortSignal here: on Windows, - // aborting that signal while a piped command is still forwarding output - // can terminate the Bun host before the native timeout result resolves. + // Explicit timeouts are enforced inside pi-natives via `timeoutMs`. + // Give native cancellation time to flush pipeline output and drain the + // N-API bridge before this result-only watchdog quarantines the run. if (!nativeOwnsTimeout) { abortCurrentExecution(); } timeoutDeferred.resolve("timeout"); - }, deadlineTimeoutMs); + }, fallbackTimeoutMs); } let resetSession = false; diff --git a/packages/coding-agent/test/bash-executor.test.ts b/packages/coding-agent/test/bash-executor.test.ts index ab7aea794..d2b7c96bf 100644 --- a/packages/coding-agent/test/bash-executor.test.ts +++ b/packages/coding-agent/test/bash-executor.test.ts @@ -1034,6 +1034,42 @@ exit 64 expect(result.output).toContain("Command cancelled"); await expectMarkerNeverWritten(marker, release); }); + it("waits for native timeout teardown to flush piped output", async () => { + const realSetTimeout = globalThis.setTimeout; + vi.spyOn(globalThis, "setTimeout").mockImplementation(((handler: () => void, ms?: number, ...rest: unknown[]) => + realSetTimeout( + handler, + ms === 1000 ? 5 : typeof ms === "number" && ms > 1000 ? 50 : ms, + ...rest, + )) as typeof globalThis.setTimeout); + + let nativeSignal: AbortSignal | undefined; + vi.spyOn(piNatives.Shell.prototype, "run").mockImplementation((options, onChunk) => { + if (options.signal instanceof AbortSignal) { + nativeSignal = options.signal; + } + const nativeResult = Promise.withResolvers(); + realSetTimeout(() => { + onChunk?.(null, "flushed-during-timeout\n"); + nativeResult.resolve({ exitCode: undefined, cancelled: false, timedOut: true }); + }, 20); + return nativeResult.promise; + }); + const abortSpy = vi.spyOn(piNatives.Shell.prototype, "abort").mockResolvedValue(); + + const result = await executeBash("producer | tail -5", { + cwd: tempDir, + timeout: 1000, + sessionKey: "native-timeout-flushes-pipeline", + }); + + expect(result.cancelled).toBe(true); + expect(result.output).toContain("flushed-during-timeout"); + expect(result.output).toContain("Command timed out after 1 seconds"); + expect(nativeSignal).toBeDefined(); + expect(nativeSignal?.aborted).toBe(false); + expect(abortSpy).not.toHaveBeenCalled(); + }); }); describe("executeBash :async: background retention", () => { diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 4e517c278..0dc3a3dc5 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -19,6 +19,9 @@ ### Fixed - Fixed an issue where Windows PTY callers were forced through shell command re-quoting by supporting direct executable and argument launching. +### Fixed + +- Fixed timed-out shell pipelines cancelling their output reader while the final stage was still flushing, which dropped captured output and could terminate Windows hosts during teardown. ([#5316](https://github.com/can1357/oh-my-pi/issues/5316)) ## [16.4.6] - 2026-07-12