merge PR #5671 via eval/pr-5671: fix(bash): drained piped output before timeout return
This commit is contained in:
@@ -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::<String>();
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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 <url>`" 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)).
|
||||
|
||||
@@ -70,6 +70,9 @@ const shellSessionsInUse = new Set<string>();
|
||||
*/
|
||||
const retainedShells = new Set<Shell>();
|
||||
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<void> {
|
||||
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;
|
||||
|
||||
@@ -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<piNatives.ShellRunResult>();
|
||||
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", () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user