From be787434e5637a98831af744c3ed6f9e0d9d6e37 Mon Sep 17 00:00:00 2001 From: Martin Cajiao Date: Wed, 10 Jun 2026 19:36:03 -0600 Subject: [PATCH 1/4] fix(core): never let a failing output chunk block the shell exit result The exit result of a PTY execution is gated on the output processing chain: finalize() - the only path that resolves the result - ran exclusively through Promise.race(processingChain, abortFired) with no rejection handling. A chunk that threw anywhere in the rendering pipeline poisoned the chain, the race rejected, and finalize() never ran. The tool call then stayed `executing` forever and the UI kept reporting the shell as awaiting input after the process had already exited (#25166); the global unhandledRejection handler logs and continues, so this manifested as a silent hang rather than a crash. - Settle every output chunk even when it throws (try/catch around the chunk executor, try/finally around the terminal write callback), logging instead of poisoning the chain. - Treat a rejected chain as drained and run finalize() on both race outcomes. - Make finalize() idempotent and throw-proof: failures while rendering or serializing the final buffer degrade the captured output instead of skipping completeWithResult(). - Guard the deferred (debounced) render: it runs in a timer outside any caller try/catch, where a throw becomes an uncaught exception that kills the CLI. --- .../services/shellExecutionService.test.ts | 41 ++++ .../src/services/shellExecutionService.ts | 196 ++++++++++++------ 2 files changed, 177 insertions(+), 60 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 378c9c3fec5..a91d366ed1d 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -1247,6 +1247,47 @@ describe('ShellExecutionService', () => { expect(destroySpy).toHaveBeenCalled(); }); }); + + describe('Exit finalization resilience', () => { + // Regression tests for the "shell stuck in Awaiting input after the + // command completes" family (#25166): the exit result is gated on the + // output processing chain, so a chunk that throws anywhere in the + // rendering pipeline must never leave the execution unresolved. + + it('resolves the result even if rendering throws while processing output', async () => { + mockSerializeTerminalToObject.mockImplementation(() => { + throw new Error('serializer boom'); + }); + + const { result } = await simulateExecution('echo hello', (pty) => { + pty.onData.mock.calls[0][0]('hello\r\n'); + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + expect(result.output.trim()).toBe('hello'); + // The serialized snapshot is lost, but the execution must not hang. + expect(result.ansiOutput).toBeUndefined(); + expect(mockDebugLogger.warn).toHaveBeenCalled(); + }); + + it('resolves the result even if a chunk throws before reaching the terminal', async () => { + mockIsBinary.mockImplementation(() => { + throw new Error('sniff boom'); + }); + + const { result } = await simulateExecution('echo hello', (pty) => { + pty.onData.mock.calls[0][0]('hello\r\n'); + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(result.exitCode).toBe(0); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Error while processing shell output chunk'), + expect.any(Error), + ); + }); + }); }); describe('ShellExecutionService child_process fallback', () => { diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 1e8bff00e69..df8225a203f 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -1153,8 +1153,18 @@ export class ShellExecutionService { } renderTimeout = setTimeout(() => { - renderFn(); - renderTimeout = null; + // A deferred render runs outside any caller's try/catch; a throw + // here would surface as an uncaught exception and kill the CLI. + try { + renderFn(); + } catch (err) { + debugLogger.warn( + `Deferred render failed for shell execution (pid ${ptyPid}):`, + err, + ); + } finally { + renderTimeout = null; + } }, 68); }; @@ -1168,54 +1178,75 @@ export class ShellExecutionService { processingChain = processingChain.then( () => new Promise((resolveChunk) => { - if (!decoder) { - decoder = new TextDecoder('utf-8'); - } - - if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) { - sniffChunks.push(data); - } else if (!isStreamingRawContent) { - binaryBytesReceived += data.length; - } - - if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) { - const sniffBuffer = Buffer.concat(sniffChunks); - sniffedBytes = sniffBuffer.length; - - if (isBinary(sniffBuffer, 512, true)) { - isStreamingRawContent = false; - binaryBytesReceived = sniffBuffer.length; - const event: ShellOutputEvent = { type: 'binary_detected' }; - onOutputEvent(event); - ExecutionLifecycleService.emitEvent(ptyPid, event); + // A chunk that throws must settle rather than poison the + // chain: finalize() races against this chain on exit, and an + // unsettled link would block the exit result forever. + try { + if (!decoder) { + decoder = new TextDecoder('utf-8'); } - } - if (isStreamingRawContent) { - const decodedChunk = decoder.decode(data, { stream: true }); - if (decodedChunk.length === 0) { - resolveChunk(); - return; + if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) { + sniffChunks.push(data); + } else if (!isStreamingRawContent) { + binaryBytesReceived += data.length; } - if (ShellExecutionService.backgroundLogPids.has(ptyPid)) { - ShellExecutionService.syncBackgroundLog(ptyPid, decodedChunk); + if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) { + const sniffBuffer = Buffer.concat(sniffChunks); + sniffedBytes = sniffBuffer.length; + + if (isBinary(sniffBuffer, 512, true)) { + isStreamingRawContent = false; + binaryBytesReceived = sniffBuffer.length; + const event: ShellOutputEvent = { type: 'binary_detected' }; + onOutputEvent(event); + ExecutionLifecycleService.emitEvent(ptyPid, event); + } } - isWriting = true; - headlessTerminal.write(decodedChunk, () => { - render(); - isWriting = false; + if (isStreamingRawContent) { + const decodedChunk = decoder.decode(data, { stream: true }); + if (decodedChunk.length === 0) { + resolveChunk(); + return; + } + + if (ShellExecutionService.backgroundLogPids.has(ptyPid)) { + ShellExecutionService.syncBackgroundLog( + ptyPid, + decodedChunk, + ); + } + + isWriting = true; + headlessTerminal.write(decodedChunk, () => { + // A throw inside render() must not leave this chunk + // unsettled: the exit result is gated on the chain + // draining. + try { + render(); + } finally { + isWriting = false; + resolveChunk(); + } + }); + } else { + const totalBytes = binaryBytesReceived; + const event: ShellOutputEvent = { + type: 'binary_progress', + bytesReceived: totalBytes, + }; + onOutputEvent(event); + ExecutionLifecycleService.emitEvent(ptyPid, event); resolveChunk(); - }); - } else { - const totalBytes = binaryBytesReceived; - const event: ShellOutputEvent = { - type: 'binary_progress', - bytesReceived: totalBytes, - }; - onOutputEvent(event); - ExecutionLifecycleService.emitEvent(ptyPid, event); + } + } catch (err) { + debugLogger.warn( + `Error while processing shell output chunk (pid ${ptyPid}):`, + err, + ); + isWriting = false; resolveChunk(); } }), @@ -1237,8 +1268,27 @@ export class ShellExecutionService { // its buffer contents, then disposed to free memory. ShellExecutionService.destroyPtyProcess(ptyProcess); + // finalize() is the only path that resolves the execution result, + // so it must run exactly once on every exit, no matter how the + // drain race below settles. + let finalized = false; + const finalize = () => { - render(true); + if (finalized) { + return; + } + finalized = true; + // Nothing below may prevent the result from reaching the caller: + // a failure in the rendering pipeline must degrade output, not + // hang the execution. + try { + render(true); + } catch (err) { + debugLogger.warn( + `Final render failed for shell execution (pid ${ptyPid}):`, + err, + ); + } cmdCleanup?.(); const event: ShellOutputEvent = { @@ -1259,17 +1309,33 @@ export class ShellExecutionService { } onOutputEvent(event); - const endLine = headlessTerminal.buffer.active.length; - const startLine = Math.max( - 0, - endLine - (shellExecutionConfig.maxSerializedLines ?? 2000), - ); - const ansiOutputSnapshot = serializeTerminalToObject( - headlessTerminal, - startLine, - endLine, - ); - const finalOutput = getFullBufferText(headlessTerminal); + let ansiOutputSnapshot: AnsiOutput | undefined; + try { + const endLine = headlessTerminal.buffer.active.length; + const startLine = Math.max( + 0, + endLine - (shellExecutionConfig.maxSerializedLines ?? 2000), + ); + ansiOutputSnapshot = serializeTerminalToObject( + headlessTerminal, + startLine, + endLine, + ); + } catch (err) { + debugLogger.warn( + `Failed to serialize final shell output (pid ${ptyPid}):`, + err, + ); + } + let finalOutput = ''; + try { + finalOutput = getFullBufferText(headlessTerminal); + } catch (err) { + debugLogger.warn( + `Failed to extract final shell output (pid ${ptyPid}):`, + err, + ); + } // Dispose the headless terminal to free scrollback buffers. // This must happen after getFullBufferText() extracts the output. @@ -1302,7 +1368,12 @@ export class ShellExecutionService { return; } - const processingComplete = processingChain.then(() => 'processed'); + // A rejected chunk counts as drained: the exit result must never + // be blocked on the rendering pipeline failing. + const processingComplete = processingChain.then( + () => 'processed' as const, + () => 'processed' as const, + ); const abortFired = new Promise<'aborted'>((res) => { if (abortSignal.aborted) { res('aborted'); @@ -1313,10 +1384,15 @@ export class ShellExecutionService { }); }); - // eslint-disable-next-line @typescript-eslint/no-floating-promises - Promise.race([processingComplete, abortFired]).then(() => { - finalize(); - }); + + Promise.race([processingComplete, abortFired]).then( + () => { + finalize(); + }, + () => { + finalize(); + }, + ); }, ); From baa9995160491c5880d7db27ac9b477990371f12 Mon Sep 17 00:00:00 2001 From: Martin Cajiao Date: Wed, 10 Jun 2026 19:39:24 -0600 Subject: [PATCH 2/4] fix(core): bound the post-exit output drain with an idle watchdog Even with every chunk settling, the exit result still waits on the output chain draining through the headless terminal, and a write callback that is never invoked (xterm swallows callbacks on disposed or paused terminals; Windows ConPTY keeps flushing data after exit while the PTY is destroyed immediately) used to leave the execution unresolved forever - the visible symptom of #25166. After exit, an idle watchdog now polls drain progress: every settled chunk resets the window, so a slow but advancing drain (large final bursts against a 300k-line scrollback) is never cut short, and only a genuinely stuck chain trips it. When it fires, the execution finalizes with the output buffered so far and logs a warning so field reports can confirm which vector was hit. The interval is unref d and cleared by finalize(). --- .../shellExecutionService.drain.test.ts | 265 ++++++++++++++++++ .../src/services/shellExecutionService.ts | 53 +++- 2 files changed, 315 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/services/shellExecutionService.drain.test.ts diff --git a/packages/core/src/services/shellExecutionService.drain.test.ts b/packages/core/src/services/shellExecutionService.drain.test.ts new file mode 100644 index 00000000000..99e8431ff96 --- /dev/null +++ b/packages/core/src/services/shellExecutionService.drain.test.ts @@ -0,0 +1,265 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Tests for the post-exit output drain watchdog (#25166). + * + * These tests mock the headless terminal so the xterm write callback is + * under test control, which is how the stuck state reproduces: the exit + * result is gated on the output processing chain, and a write callback + * that never fires used to leave the execution unresolved forever, with + * the UI stuck showing the shell as awaiting input. + */ + +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; + +import { + ShellExecutionService, + DRAIN_STALL_TIMEOUT_MS, + type ShellExecutionConfig, +} from './shellExecutionService.js'; +import { NoopSandboxManager } from './sandboxManager.js'; +import { ExecutionLifecycleService } from './executionLifecycleService.js'; + +// Hoisted Mocks +const mockPtySpawn = vi.hoisted(() => vi.fn()); +const mockIsBinary = vi.hoisted(() => vi.fn()); +const mockPlatform = vi.hoisted(() => vi.fn()); +const mockHomedir = vi.hoisted(() => vi.fn()); +const mockGetPty = vi.hoisted(() => vi.fn()); +const mockSerializeTerminalToObject = vi.hoisted(() => vi.fn()); +const mockResolveExecutable = vi.hoisted(() => vi.fn()); +const mockDebugLogger = vi.hoisted(() => ({ + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +/** + * Controllable headless terminal: write callbacks are queued instead of + * fired, so each test decides when (or whether) a chunk finishes draining. + */ +const terminalState = vi.hoisted(() => ({ + pendingWriteCallbacks: [] as Array<() => void>, +})); + +vi.mock('@xterm/headless', () => { + class MockTerminal { + buffer = { + active: { + viewportY: 0, + baseY: 0, + cursorY: 0, + length: 0, + getLine: () => undefined, + }, + }; + scrollToTop = vi.fn(); + onScroll = vi.fn(); + resize = vi.fn(); + scrollLines = vi.fn(); + dispose = vi.fn(); + write = vi.fn((_data: string, cb?: () => void) => { + if (cb) { + terminalState.pendingWriteCallbacks.push(cb); + } + }); + } + return { + default: { Terminal: MockTerminal }, + Terminal: MockTerminal, + }; +}); + +vi.mock('../config/storage.js', () => ({ + Storage: { + getGlobalTempDir: vi.fn().mockReturnValue('/mock/temp'), + }, +})); +vi.mock('../utils/debugLogger.js', () => ({ + debugLogger: mockDebugLogger, +})); +vi.mock('../utils/shell-utils.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + resolveExecutable: mockResolveExecutable, + }; +}); +vi.mock('../utils/textUtils.js', () => ({ + isBinary: mockIsBinary, +})); +vi.mock('node:os', () => ({ + default: { + platform: mockPlatform, + homedir: mockHomedir, + constants: { signals: { SIGTERM: 15, SIGKILL: 9 } }, + }, + platform: mockPlatform, + homedir: mockHomedir, + constants: { signals: { SIGTERM: 15, SIGKILL: 9 } }, +})); +vi.mock('../utils/getPty.js', () => ({ + getPty: mockGetPty, +})); +vi.mock('../utils/terminalSerializer.js', () => ({ + serializeTerminalToObject: ( + _terminal: unknown, + ...args: [number | undefined, number | undefined] + ) => mockSerializeTerminalToObject(...args), + convertColorToHex: () => '#000000', + ColorMode: { DEFAULT: 0, PALETTE: 1, RGB: 2 }, +})); + +const shellExecutionConfig: ShellExecutionConfig = { + sessionId: 'default', + terminalWidth: 80, + terminalHeight: 24, + pager: 'cat', + showColor: false, + disableDynamicLineTrimming: true, + sanitizationConfig: { + enableEnvironmentVariableRedaction: false, + allowedEnvironmentVariables: [], + blockedEnvironmentVariables: [], + }, + sandboxManager: new NoopSandboxManager(), +}; + +describe('ShellExecutionService drain watchdog', () => { + let mockPtyProcess: { + pid: number; + kill: ReturnType; + onData: ReturnType; + onExit: ReturnType; + write: ReturnType; + resize: ReturnType; + destroy: ReturnType; + }; + + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + ExecutionLifecycleService.resetForTest(); + ShellExecutionService.resetForTest(); + terminalState.pendingWriteCallbacks.length = 0; + mockSerializeTerminalToObject.mockReturnValue([]); + mockIsBinary.mockReturnValue(false); + mockPlatform.mockReturnValue('linux'); + mockResolveExecutable.mockImplementation((exe: string) => exe); + mockGetPty.mockResolvedValue({ + module: { spawn: mockPtySpawn }, + name: 'mock-pty', + }); + + mockPtyProcess = { + pid: 12345, + kill: vi.fn(), + onData: vi.fn(), + onExit: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + destroy: vi.fn(), + }; + mockPtySpawn.mockReturnValue(mockPtyProcess); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + const startExecution = async (command: string) => { + const handle = await ShellExecutionService.execute( + command, + '/test/dir', + vi.fn(), + new AbortController().signal, + true, + shellExecutionConfig, + ); + // Let the microtask that registers onData/onExit run. + await vi.advanceTimersByTimeAsync(0); + return handle; + }; + + const emitData = (chunk: string) => { + mockPtyProcess.onData.mock.calls[0][0](chunk); + }; + + const emitExit = (exitCode = 0) => { + mockPtyProcess.onExit.mock.calls[0][0]({ exitCode, signal: null }); + }; + + it('finalizes the execution when a write callback is never invoked', async () => { + const handle = await startExecution('echo hello'); + + emitData('hello\r\n'); // its write callback is intentionally never fired + emitExit(0); + + let resolved = false; + const resultPromise = handle.result.then((r) => { + resolved = true; + return r; + }); + + // Before the watchdog window elapses the execution is still pending. + await vi.advanceTimersByTimeAsync(DRAIN_STALL_TIMEOUT_MS / 2); + expect(resolved).toBe(false); + + await vi.advanceTimersByTimeAsync(DRAIN_STALL_TIMEOUT_MS); + const result = await resultPromise; + + expect(resolved).toBe(true); + expect(result.exitCode).toBe(0); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('drain stalled'), + ); + }); + + it('does not cut short a slow drain that keeps making progress', async () => { + const handle = await startExecution('big-output'); + + emitData('chunk-1'); + emitData('chunk-2'); + emitData('chunk-3'); + emitExit(0); + + let resolved = false; + void handle.result.then(() => { + resolved = true; + }); + + // Each chunk settles slower than the poll cadence but faster than the + // stall window; total drain time exceeds the window. Progress must + // keep the watchdog from firing. + const step = DRAIN_STALL_TIMEOUT_MS * 0.75; + for (let i = 0; i < 3; i++) { + await vi.advanceTimersByTimeAsync(step); + expect(resolved).toBe(false); + terminalState.pendingWriteCallbacks.shift()?.(); + await vi.advanceTimersByTimeAsync(0); + } + + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(mockDebugLogger.warn).not.toHaveBeenCalledWith( + expect.stringContaining('drain stalled'), + ); + }); + + it('does not start a watchdog when there is nothing left to drain', async () => { + const handle = await startExecution('true'); + + emitExit(0); + await vi.advanceTimersByTimeAsync(0); + + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(mockDebugLogger.warn).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index df8225a203f..50aa44e5f29 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -65,6 +65,15 @@ export const GEMINI_CLI_IDENTIFICATION_ENV_VAR_VALUE = '1'; // we capture significant output from long-running commands. export const SCROLLBACK_LIMIT = 300000; +// After the process exits, the result still waits for queued output to +// drain through the headless terminal. That drain is bounded by an idle +// watchdog: if no chunk settles for a full window, the chain is considered +// stuck (e.g. a swallowed terminal write callback) and the execution +// finalizes with the output buffered so far. Progress resets the window, +// so a slow but advancing drain is never cut short. +export const DRAIN_STALL_TIMEOUT_MS = 2000; +const DRAIN_STALL_POLL_MS = 250; + const BASH_SHOPT_OPTIONS = 'promptvars nullglob extglob nocaseglob dotglob'; const BASH_SHOPT_GUARD = `shopt -u ${BASH_SHOPT_OPTIONS};`; @@ -1050,6 +1059,14 @@ export class ShellExecutionService { const error: Error | null = null; let exited = false; + // Updated every time an output chunk settles so the post-exit drain + // watchdog only fires when the chain is stuck, never while it is + // slow but advancing. + let lastDrainActivityAt = Date.now(); + const markDrainActivity = () => { + lastDrainActivityAt = Date.now(); + }; + let isStreamingRawContent = true; const MAX_SNIFF_SIZE = 4096; let sniffedBytes = 0; @@ -1251,6 +1268,9 @@ export class ShellExecutionService { } }), ); + // Feed the post-exit drain watchdog: every settled chunk is + // progress, whether it drained cleanly or failed. + void processingChain.then(markDrainActivity, markDrainActivity); }; ptyProcess.onData((data: string) => { @@ -1272,12 +1292,17 @@ export class ShellExecutionService { // so it must run exactly once on every exit, no matter how the // drain race below settles. let finalized = false; + let drainWatchdog: NodeJS.Timeout | undefined; const finalize = () => { if (finalized) { return; } finalized = true; + if (drainWatchdog) { + clearInterval(drainWatchdog); + drainWatchdog = undefined; + } // Nothing below may prevent the result from reaching the caller: // a failure in the rendering pipeline must degrade output, not // hang the execution. @@ -1384,9 +1409,31 @@ export class ShellExecutionService { }); }); - - Promise.race([processingComplete, abortFired]).then( - () => { + // Bound the drain with an idle watchdog: if no chunk settles for + // a full window after exit, the chain is stuck and the exit + // result must win. finalize() clears the interval. + markDrainActivity(); + const drainStalled = new Promise<'drain-stalled'>((res) => { + drainWatchdog = setInterval(() => { + if (Date.now() - lastDrainActivityAt >= DRAIN_STALL_TIMEOUT_MS) { + res('drain-stalled'); + } + }, DRAIN_STALL_POLL_MS); + drainWatchdog.unref?.(); + }); + + void Promise.race([ + processingComplete, + abortFired, + drainStalled, + ]).then( + (outcome) => { + if (outcome === 'drain-stalled') { + debugLogger.warn( + `Shell output drain stalled after exit (pid ${ptyPid}); ` + + `finalizing with the output buffered so far.`, + ); + } finalize(); }, () => { From 5a0083ba2a0344cdfa3f8445ae813a6f36b209f8 Mon Sep 17 00:00:00 2001 From: Martin Cajiao Date: Wed, 10 Jun 2026 19:57:26 -0600 Subject: [PATCH 3/4] fix(core): use a monotonic clock for the drain watchdog Addresses review feedback: Date.now() is wall-clock time, so an NTP adjustment or VM migration could fire the stall watchdog prematurely (clock jumps forward) or delay it (clock jumps backward). performance.now() is monotonic and immune to clock adjustments. The wall-clock Date.now() uses for history timestamps are untouched. --- packages/core/src/services/shellExecutionService.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 50aa44e5f29..1f9e0c42182 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -1061,10 +1061,11 @@ export class ShellExecutionService { // Updated every time an output chunk settles so the post-exit drain // watchdog only fires when the chain is stuck, never while it is - // slow but advancing. - let lastDrainActivityAt = Date.now(); + // slow but advancing. Uses the monotonic clock: wall-clock (Date.now) + // adjustments, e.g. NTP, must not fire or delay the watchdog. + let lastDrainActivityAt = performance.now(); const markDrainActivity = () => { - lastDrainActivityAt = Date.now(); + lastDrainActivityAt = performance.now(); }; let isStreamingRawContent = true; @@ -1415,7 +1416,10 @@ export class ShellExecutionService { markDrainActivity(); const drainStalled = new Promise<'drain-stalled'>((res) => { drainWatchdog = setInterval(() => { - if (Date.now() - lastDrainActivityAt >= DRAIN_STALL_TIMEOUT_MS) { + if ( + performance.now() - lastDrainActivityAt >= + DRAIN_STALL_TIMEOUT_MS + ) { res('drain-stalled'); } }, DRAIN_STALL_POLL_MS); From 4677496dcca8ef668da930a9024c4da3dc95a4b8 Mon Sep 17 00:00:00 2001 From: Martin Cajiao Date: Thu, 11 Jun 2026 00:24:33 -0600 Subject: [PATCH 4/4] fix(core): bound the child_process fallback wait for stdio close The fallback path finalized only on close, which waits for the stdio pipes to end. A grandchild that inherits the pipes and outlives the shell (common on Windows) keeps close from ever firing even though exit was emitted - the same stuck-result family as #25166, through the non-PTY path. From exit, the wait for close is now bounded the same way the PTY drain is: stream activity resets an idle window so the trailing flush is captured, and a hard cap (POST_EXIT_DRAIN_CAP_MS) covers grandchildren that keep writing indefinitely so they cannot hold the exit result hostage. handleExit is idempotent and clears the watchdog; a prompt close still wins with no behavior change on the happy path. --- .../services/shellExecutionService.test.ts | 80 +++++++++++++++++++ .../src/services/shellExecutionService.ts | 52 ++++++++++++ 2 files changed, 132 insertions(+) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index a91d366ed1d..9ec7909e712 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -19,6 +19,7 @@ import type { Readable } from 'node:stream'; import { type ChildProcess } from 'node:child_process'; import { ShellExecutionService, + DRAIN_STALL_TIMEOUT_MS, type ShellOutputEvent, type ShellExecutionConfig, } from './shellExecutionService.js'; @@ -1343,6 +1344,85 @@ describe('ShellExecutionService child_process fallback', () => { return { result, handle, abortController }; }; + describe('Post-exit drain watchdog', () => { + // 'close' waits for the stdio pipes to end; a grandchild that + // inherited them (common on Windows) keeps 'close' from ever firing + // even though the shell already exited — the same stuck-result family + // as #25166, through the fallback path. + + afterEach(() => { + vi.useRealTimers(); + }); + + const startExecution = async () => { + const handle = await ShellExecutionService.execute( + 'spawns-a-daemon', + '/test/dir', + onOutputEventMock, + new AbortController().signal, + true, + shellExecutionConfig, + ); + await vi.advanceTimersByTimeAsync(0); + return handle; + }; + + it('finalizes when close never fires after exit', async () => { + vi.useFakeTimers(); + const handle = await startExecution(); + + mockChildProcess.stdout?.emit('data', Buffer.from('hello\n')); + mockChildProcess.emit('exit', 0, null); + // No 'close': the pipes are held open by a grandchild. + + await vi.advanceTimersByTimeAsync(DRAIN_STALL_TIMEOUT_MS + 500); + const result = await handle.result; + + expect(result.exitCode).toBe(0); + expect(result.output).toContain('hello'); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('stdio drain stalled'), + ); + }); + + it('caps the wait when a grandchild keeps writing through the pipes', async () => { + vi.useFakeTimers(); + const handle = await startExecution(); + + mockChildProcess.emit('exit', 0, null); + + let resolved = false; + void handle.result.then(() => { + resolved = true; + }); + + // Keep the streams busy: the idle window never elapses, but the + // hard cap (10s) must still end the wait. + for (let i = 0; i < 9; i++) { + await vi.advanceTimersByTimeAsync(1000); + mockChildProcess.stdout?.emit('data', Buffer.from(`tick ${i}\n`)); + } + expect(resolved).toBe(false); + + await vi.advanceTimersByTimeAsync(1500); + const result = await handle.result; + expect(result.exitCode).toBe(0); + }); + + it('lets a prompt close win without logging a warning', async () => { + vi.useFakeTimers(); + const handle = await startExecution(); + + mockChildProcess.emit('exit', 0, null); + mockChildProcess.emit('close', 0, null); + await vi.advanceTimersByTimeAsync(DRAIN_STALL_TIMEOUT_MS * 2); + + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(mockDebugLogger.warn).not.toHaveBeenCalled(); + }); + }); + describe('Successful Execution', () => { it('should execute a command and capture stdout and stderr', async () => { const { result, handle } = await simulateExecution('ls -l', (cp) => { diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 1f9e0c42182..5e30ead856b 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -74,6 +74,13 @@ export const SCROLLBACK_LIMIT = 300000; export const DRAIN_STALL_TIMEOUT_MS = 2000; const DRAIN_STALL_POLL_MS = 250; +// In the child_process fallback the post-exit wait is for the stdio +// 'close' event, and data may keep arriving from a live grandchild that +// inherited the pipes rather than from a finite queue — so on top of the +// idle window there is a hard cap: a process that keeps writing through +// inherited pipes must not hold the exit result hostage indefinitely. +export const POST_EXIT_DRAIN_CAP_MS = 10_000; + const BASH_SHOPT_OPTIONS = 'promptvars nullglob extglob nocaseglob dotglob'; const BASH_SHOPT_GUARD = `shopt -u ${BASH_SHOPT_OPTIONS};`; @@ -664,12 +671,22 @@ export class ShellExecutionService { let stderrDecoder: TextDecoder | null = null; let error: Error | null = null; let exited = false; + let finalized = false; + let closeWatchdog: NodeJS.Timeout | undefined; + // Updated on every output chunk so the post-exit watchdog only fires + // when the streams go silent, not while a grandchild is still + // writing through the inherited pipes. + let lastDrainActivityAt = performance.now(); + const markDrainActivity = () => { + lastDrainActivityAt = performance.now(); + }; let isStreamingRawContent = true; const MAX_SNIFF_SIZE = 4096; let sniffedBytes = 0; const handleOutput = (data: Buffer, stream: 'stdout' | 'stderr') => { + markDrainActivity(); if (!stdoutDecoder || !stderrDecoder) { stdoutDecoder = new TextDecoder('utf-8'); stderrDecoder = new TextDecoder('utf-8'); @@ -743,6 +760,14 @@ export class ShellExecutionService { code: number | null, signal: NodeJS.Signals | null, ) => { + if (finalized) { + return; + } + finalized = true; + if (closeWatchdog) { + clearInterval(closeWatchdog); + closeWatchdog = undefined; + } cleanup(); cmdCleanup?.(); @@ -826,6 +851,33 @@ export class ShellExecutionService { handleExit(code, signal); }); + // 'close' waits for the stdio streams to end, which never happens if + // a grandchild inherited the pipes and outlives the shell (common on + // Windows). 'exit' still fires, so from there the wait for 'close' + // is bounded: stream activity resets an idle window (to capture the + // trailing flush), and a hard cap covers grandchildren that keep + // writing indefinitely. + child.on('exit', (code, signal) => { + if (finalized || closeWatchdog) { + return; + } + const exitedAt = performance.now(); + markDrainActivity(); + closeWatchdog = setInterval(() => { + const now = performance.now(); + const stalled = now - lastDrainActivityAt >= DRAIN_STALL_TIMEOUT_MS; + const capped = now - exitedAt >= POST_EXIT_DRAIN_CAP_MS; + if (stalled || capped) { + debugLogger.warn( + `Shell stdio drain stalled after exit (pid ${child.pid}); ` + + `finalizing with the output received so far.`, + ); + handleExit(code, signal); + } + }, DRAIN_STALL_POLL_MS); + closeWatchdog.unref?.(); + }); + function cleanup() { exited = true; abortSignal.removeEventListener('abort', abortHandler);