From 6cbab376d7ab8f1ffc554f545c9ca955ae8d6610 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 5 May 2026 12:45:27 +0800 Subject: [PATCH 1/9] feat(core): add signal.reason convention for ShellExecutionService.execute() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foundation for #3831 Phase D (b) — Ctrl+B promote of a running foreground shell to background. Defines a discriminated `ShellAbortReason` union that the AbortSignal carries; default behavior (no reason / `{ kind: 'cancel' }`) keeps the existing tree-kill on abort. `{ kind: 'background' }` is a takeover signal — execute() skips the kill, drops the child from its active set (so cleanup() won't kill it later), flushes a snapshot of captured output, and resolves the result Promise immediately with `promoted: true` so the awaiting caller unblocks. Pure plumbing: no caller sets the reason yet, so this is a zero-behavior change for existing call sites. The `promoted?: boolean` field is optional on ShellExecutionResult so existing consumers compile against the new shape without source changes. Tests pin both branches in both childProcessFallback and executeWithPty: default abort still SIGTERM-tree-kills; `{ kind: 'cancel' }` is identical to default (pin against accidental routing through the background branch); `{ kind: 'background' }` skips the kill, snapshot output is preserved, mockProcessKill / mockPtyProcess.kill are NOT called. Part of #3831 (Phase D part b — Ctrl+B promote running shell to background). PR-1 of 3. --- .../services/shellExecutionService.test.ts | 119 +++++++++++++++++- .../src/services/shellExecutionService.ts | 91 ++++++++++++++ 2 files changed, 209 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 5cc203929e7..827a20184e5 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -17,7 +17,10 @@ import EventEmitter from 'node:events'; import type { Readable } from 'node:stream'; import { type ChildProcess } from 'node:child_process'; import pkg from '@xterm/headless'; -import type { ShellOutputEvent } from './shellExecutionService.js'; +import type { + ShellAbortReason, + ShellOutputEvent, +} from './shellExecutionService.js'; import { ShellExecutionService } from './shellExecutionService.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; @@ -608,6 +611,59 @@ describe('ShellExecutionService', () => { expect(result.aborted).toBe(true); // The process kill is mocked, so we just check that the flag is set. }); + + it('signal.reason = { kind: "cancel" } still tree-kills (same as default)', async () => { + const { result } = await simulateExecution( + 'sleep 10', + (pty, abortController) => { + abortController.abort({ kind: 'cancel' } satisfies ShellAbortReason); + pty.onExit.mock.calls[0][0]({ exitCode: 1, signal: null }); + }, + ); + + expect(result.aborted).toBe(true); + expect(result.promoted).toBeUndefined(); + // The default kill path runs: SIGTERM via process.kill on the + // process-group pid. Pinning that we DID try to kill — i.e., reason + // === 'cancel' is NOT mistakenly routed through the background branch. + expect(mockProcessKill).toHaveBeenCalledWith( + -mockPtyProcess.pid, + 'SIGTERM', + ); + }); + + it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true', async () => { + // Critical: do NOT fire onExit — the child is still alive after the + // background-promote abort. The result Promise must resolve via the + // abort handler's own immediate resolve, not via the exit handler. + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (_pty, abortController) => { + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + }, + ); + + expect(result.aborted).toBe(true); + expect(result.promoted).toBe(true); + expect(result.exitCode).toBeNull(); + expect(result.signal).toBeNull(); + expect(result.error).toBeNull(); + expect(result.pid).toBe(mockPtyProcess.pid); + // Verify the kill path did NOT run: neither the PTY's own kill() nor + // process.kill on the group pid. Caller now owns the child. + expect(mockPtyProcess.kill).not.toHaveBeenCalled(); + expect(mockProcessKill).not.toHaveBeenCalledWith( + -mockPtyProcess.pid, + 'SIGTERM', + ); + expect(mockProcessKill).not.toHaveBeenCalledWith( + -mockPtyProcess.pid, + 'SIGKILL', + ); + }); }); describe('Binary Output', () => { @@ -1139,6 +1195,67 @@ describe('ShellExecutionService child_process fallback', () => { }, ); + it('signal.reason = { kind: "cancel" } still tree-kills (same as default)', async () => { + mockPlatform.mockReturnValue('linux'); + const { result } = await simulateExecution( + 'sleep 10', + (cp, abortController) => { + abortController.abort({ kind: 'cancel' } satisfies ShellAbortReason); + cp.emit('exit', null, 'SIGKILL'); + cp.emit('close', null, 'SIGKILL'); + }, + ); + + expect(result.aborted).toBe(true); + expect(result.promoted).toBeUndefined(); + // Default kill path ran — pin that reason === 'cancel' is NOT + // mistakenly routed through the background branch. + expect(mockProcessKill).toHaveBeenCalledWith( + -mockChildProcess.pid!, + 'SIGTERM', + ); + }); + + it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true', async () => { + mockPlatform.mockReturnValue('linux'); + // Critical: do NOT fire 'exit' — the child is still alive after the + // background-promote abort. The result Promise must resolve via the + // abort handler's own immediate resolve. + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (cp, abortController) => { + // Emit some output first so the snapshot has content. + cp.stdout?.emit('data', Buffer.from('line1\nline2\n')); + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + }, + ); + + expect(result.aborted).toBe(true); + expect(result.promoted).toBe(true); + expect(result.exitCode).toBeNull(); + expect(result.signal).toBeNull(); + expect(result.error).toBeNull(); + expect(result.pid).toBe(mockChildProcess.pid); + // Output captured up to the promote moment is preserved as the + // snapshot for the caller to seed the BackgroundShellEntry's output + // file from. + expect(result.output).toContain('line1'); + expect(result.output).toContain('line2'); + // Verify the kill path did NOT run. + expect(mockProcessKill).not.toHaveBeenCalledWith( + -mockChildProcess.pid!, + 'SIGTERM', + ); + expect(mockProcessKill).not.toHaveBeenCalledWith( + -mockChildProcess.pid!, + 'SIGKILL', + ); + expect(mockChildProcess.kill).not.toHaveBeenCalled(); + }); + it('should gracefully attempt SIGKILL on linux if SIGTERM fails', async () => { mockPlatform.mockReturnValue('linux'); vi.useFakeTimers(); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 8d7e0bb3bab..1d39bc19645 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -37,6 +37,23 @@ function applyPowerShellUtf8Prefix(command: string, shell: string): string { return command; } +/** + * Discriminated reason attached to the AbortSignal that drives execute(). + * Default behavior (no reason set, or `{ kind: 'cancel' }`) is the historical + * tree-kill on abort. `{ kind: 'background' }` is a takeover signal: the + * caller has accepted ownership of the child process and wants execute() to + * relinquish it without killing — used by the foreground-shell → background + * promote path so the in-flight child keeps running. + * + * Callers MUST attach their own listeners (data / exit / error) to the live + * child *before* calling `abortController.abort({ kind: 'background', ... })`, + * since execute() drops the child from its active set on background-abort and + * will no longer route events to its own handlers' downstream consumers. + */ +export type ShellAbortReason = + | { kind: 'cancel' } + | { kind: 'background'; shellId?: string }; + /** A structured result from a shell command execution. */ export interface ShellExecutionResult { /** The raw, unprocessed output buffer. */ @@ -51,6 +68,14 @@ export interface ShellExecutionResult { error: Error | null; /** A boolean indicating if the command was aborted by the user. */ aborted: boolean; + /** + * True iff execute() returned because of a background-promote abort + * (`signal.reason.kind === 'background'`) — the child process is still + * alive and the caller has taken over its lifecycle. Callers receiving + * `promoted: true` must NOT treat exitCode/signal as terminal — the + * underlying process has not exited. + */ + promoted?: boolean; /** The process ID of the spawned shell. */ pid: number | undefined; /** The method used to execute the shell command. */ @@ -518,6 +543,37 @@ export class ShellExecutionService { }); const abortHandler = async () => { + // Background-promote takeover: skip kill, drop the child from our + // active set (so cleanup() won't kill it later), flush our text + // buffers into a snapshot, and resolve immediately with + // `promoted: true` so the awaiting caller unblocks. The caller has + // attached its own listeners to the live child by this point. + const reason = abortSignal.reason as ShellAbortReason | undefined; + if (reason?.kind === 'background' && child.pid && !exited) { + this.activeChildProcesses.delete(child.pid); + const { + stdout: snapStdout, + stderr: snapStderr, + finalBuffer, + } = cleanup(); + const separator = snapStdout.endsWith('\n') ? '' : '\n'; + const combined = + snapStdout + + (snapStderr ? (snapStdout ? separator : '') + snapStderr : ''); + resolve({ + rawOutput: finalBuffer, + output: stripAnsi(combined).trim(), + exitCode: null, + signal: null, + error: null, + aborted: true, + promoted: true, + pid: child.pid, + executionMethod: 'child_process', + }); + return; + } + if (child.pid && !exited) { if (isWindows) { cpSpawn('taskkill', ['/pid', child.pid.toString(), '/f', '/t']); @@ -898,6 +954,41 @@ export class ShellExecutionService { ); const abortHandler = async () => { + // Background-promote takeover: skip kill, drop the PTY from the + // active set (so cleanup() won't kill it later), flush a snapshot + // of the output buffer captured so far, and resolve immediately + // with `promoted: true` so the awaiting caller unblocks. The + // caller has attached its own listeners to the live PTY by this + // point and owns its lifecycle from here on. + const reason = abortSignal.reason as ShellAbortReason | undefined; + if (reason?.kind === 'background' && ptyProcess.pid && !exited) { + exited = true; + abortSignal.removeEventListener('abort', abortHandler); + this.activePtys.delete(ptyProcess.pid); + const finalBuffer = Buffer.concat(outputChunks); + let snapshot = ''; + try { + snapshot = serializeTerminalToText(headlessTerminal) ?? ''; + } catch { + // Best-effort snapshot — terminal serialization may fail if the + // PTY is mid-render; an empty snapshot is acceptable since the + // caller already has the rawOutput buffer below. + } + resolve({ + rawOutput: finalBuffer, + output: snapshot, + exitCode: null, + signal: null, + error, + aborted: true, + promoted: true, + pid: ptyProcess.pid, + executionMethod: + (ptyInfo?.name as 'node-pty' | 'lydell-node-pty') ?? 'node-pty', + }); + return; + } + if (ptyProcess.pid && !exited) { if (os.platform() === 'win32') { ptyProcess.kill(); From 8e8e18ca7f44441fb20d74369f06c8a481212101 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 5 May 2026 14:24:18 +0800 Subject: [PATCH 2/9] fix(core): detach service listeners on background-promote (resolve review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 4 Critical + 2 Suggestion findings on PR-1 of #3831: - **childProcess listener detach** (review line 555 + 573): Anonymous arrow listeners on stdout/stderr/error/exit could not be off()'d. After background-promote, post-promote bytes would re-enter handleOutput, which then calls decoder.decode() on a now-finalized text decoder (cleanup() already called .decode() without stream:true) → TypeError crash. Even without the crash, old onOutputEvent would fire for new data → ownership contract violation + duplication. Fix: extract named handler refs (stdoutHandler / stderrHandler / errorHandler / exitHandler) and call off() on all four in the background-promote branch via a detachServiceListeners() helper. - **PTY listener detach** (review line 967 + 990): node-pty's onData / onExit return IDisposable handles; the abort handler now captures dataDisposable / exitDisposable and calls .dispose() in the background-promote branch. ptyProcess.on('error') is EventEmitter-style (not IDisposable) — extract a named ptyErrorHandler ref and off() it. Without these, post-promote PTY error throws → Node.js crash; post-promote data continues writing to headlessTerminal and calling old onOutputEvent → ownership violation. - **PTY in-flight chain item ownership** (related to review line 990): processingChain may have already-enqueued callbacks past the early listenersDetached check. Refactored from "early-return short-circuit" to "guard each onOutputEvent emit individually" so in-flight writes still LAND in headlessTerminal (snapshot reflects them) but no events leak to the foreground onOutputEvent. Also clear renderTimeout in the abort handler so a pending throttled render doesn't fire post-promote. - **PTY snapshot freshness** (review line 972, suggestion): The original abort handler called serializeTerminalToText immediately. Now we await Promise.race([processingChain drain, SIGKILL_TIMEOUT_MS]) first (mirrors the onExit finalize pattern at ~line 970) so in-flight headlessTerminal.write callbacks land before serialization. Skipped render(true) intentionally because it would emit final onOutputEvent data (renderFn calls onOutputEvent), violating the "no emit post-promote" invariant — added a comment explaining why direct serialize is correct. - **Handoff-boundary tests** (review line 1257, suggestion): Added 4 new tests pinning the ownership contract — 2 for child_process (post-promote stdout/stderr does NOT route to onOutputEvent; child exit does NOT re-resolve result), 2 for PTY (data/exit disposables ARE called; result shape stays promoted: true even if post-promote events fire). Also: test setup now stubs mockPtyProcess.onData / .onExit to return { dispose: vi.fn() } so the background-promote path's dispose() calls don't crash on undefined (the stub's mock.results[0].value is then inspected by the new handoff tests). 58 / 58 tests pass (50 baseline + 4 first-pass + 4 handoff). Total +235 / -35 on top of the prior commit. --- .../services/shellExecutionService.test.ts | 132 ++++++++++++++++- .../src/services/shellExecutionService.ts | 138 ++++++++++++++---- 2 files changed, 235 insertions(+), 35 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 827a20184e5..9e98d7cd3ae 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -228,8 +228,12 @@ describe('ShellExecutionService', () => { }; mockPtyProcess.pid = 12345; mockPtyProcess.kill = vi.fn(); - mockPtyProcess.onData = vi.fn(); - mockPtyProcess.onExit = vi.fn(); + // node-pty's onData/onExit return IDisposable; the production + // background-promote path calls .dispose() on those handles to detach + // its listeners cleanly. Mock them to return a disposable stub so the + // promote path doesn't crash on `undefined.dispose()`. + mockPtyProcess.onData = vi.fn().mockReturnValue({ dispose: vi.fn() }); + mockPtyProcess.onExit = vi.fn().mockReturnValue({ dispose: vi.fn() }); mockPtyProcess.write = vi.fn(); mockPtyProcess.resize = vi.fn(); @@ -664,6 +668,60 @@ describe('ShellExecutionService', () => { 'SIGKILL', ); }); + + it('post-promotion: PTY data is no longer routed to onOutputEvent (handoff boundary)', async () => { + // Pin the ownership contract: after background-promote, PTY data + // arriving on the still-running child must NOT surface through the + // foreground execute()'s onOutputEvent (the caller has its own + // listeners now). Without dataDisposable.dispose() in the abort + // handler, the listener-retention bug would let post-promote bytes + // leak into the foreground consumer. + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (pty, abortController) => { + // Data BEFORE promote — fed via the live onData listener so it + // reaches the foreground onOutputEvent normally. + pty.onData.mock.calls[0][0]('pre-promote-data\n'); + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + }, + ); + expect(result.promoted).toBe(true); + + // The disposable returned by mockPtyProcess.onData was disposed by + // the abort handler — verify by calling .dispose's mock. + const dataDisposableStub = mockPtyProcess.onData.mock.results[0] + .value as { dispose: Mock }; + expect(dataDisposableStub.dispose).toHaveBeenCalled(); + const exitDisposableStub = mockPtyProcess.onExit.mock.results[0] + .value as { dispose: Mock }; + expect(exitDisposableStub.dispose).toHaveBeenCalled(); + }); + + it('post-promotion: PTY exit does NOT re-resolve the result (already resolved with promoted)', async () => { + // Pin: even if the still-running child later exits naturally and the + // caller's own exit listener fires, our foreground result Promise + // must NOT be re-resolved with a different shape (Promise can only + // resolve once). The exit disposable being disposed prevents our + // own onExit from firing at all in the first place — but verify the + // final resolved shape stays `promoted: true` regardless. + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (_pty, abortController) => { + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + }, + ); + + // Resolved as promoted, with no exit info from a post-promote exit. + expect(result.promoted).toBe(true); + expect(result.exitCode).toBeNull(); + expect(result.signal).toBeNull(); + }); }); describe('Binary Output', () => { @@ -1256,6 +1314,68 @@ describe('ShellExecutionService child_process fallback', () => { expect(mockChildProcess.kill).not.toHaveBeenCalled(); }); + it('post-promotion: stdout / stderr data is no longer routed to onOutputEvent (handoff boundary)', async () => { + mockPlatform.mockReturnValue('linux'); + // Pin the ownership contract: after background-promote, stdout/stderr + // arriving on the still-running child must NOT surface through the + // foreground execute()'s onOutputEvent. Without off()'ing the + // stdoutHandler / stderrHandler in the abort handler, post-promote + // bytes would re-enter handleOutput, which then calls + // decoder.decode() on a now-finalized decoder (cleanup() called + // .decode() without stream:true) → TypeError crash, OR routes to + // onOutputEvent → ownership leak / duplicated emit. + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (cp, abortController) => { + cp.stdout?.emit('data', Buffer.from('pre-promote\n')); + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + // Capture call count at the moment of promote, then emit more + // data on the still-live child stream and assert onOutputEvent + // was NOT called again. (Also verifies no TypeError from + // decoding through the finalized decoder.) + const eventCountAtPromote = onOutputEventMock.mock.calls.length; + cp.stdout?.emit('data', Buffer.from('post-promote-stdout\n')); + cp.stderr?.emit('data', Buffer.from('post-promote-stderr\n')); + expect(onOutputEventMock.mock.calls.length).toBe(eventCountAtPromote); + }, + ); + + expect(result.promoted).toBe(true); + // Pre-promote data made it into the snapshot; post-promote did not. + expect(result.output).toContain('pre-promote'); + expect(result.output).not.toContain('post-promote-stdout'); + expect(result.output).not.toContain('post-promote-stderr'); + }); + + it('post-promotion: child exit does NOT re-resolve the result with a non-promoted shape', async () => { + mockPlatform.mockReturnValue('linux'); + // Pin: even if the still-running child later exits naturally and the + // caller's own exit listener fires, our foreground result Promise + // must NOT be re-resolved (Promise can only resolve once). The + // detached exit handler prevents our own handler from firing. + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (cp, abortController) => { + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + // Simulate the still-running child exiting later; this should + // NOT route through our handleExit because the exit listener + // was off()'d in the background-promote branch. + cp.emit('exit', 42, null); + cp.emit('close', 42, null); + }, + ); + + expect(result.promoted).toBe(true); + expect(result.exitCode).toBeNull(); + expect(result.signal).toBeNull(); + }); + it('should gracefully attempt SIGKILL on linux if SIGTERM fails', async () => { mockPlatform.mockReturnValue('linux'); vi.useFakeTimers(); @@ -1452,8 +1572,12 @@ describe('ShellExecutionService execution method selection', () => { }; mockPtyProcess.pid = 12345; mockPtyProcess.kill = vi.fn(); - mockPtyProcess.onData = vi.fn(); - mockPtyProcess.onExit = vi.fn(); + // node-pty's onData/onExit return IDisposable; the production + // background-promote path calls .dispose() on those handles to detach + // its listeners cleanly. Mock them to return a disposable stub so the + // promote path doesn't crash on `undefined.dispose()`. + mockPtyProcess.onData = vi.fn().mockReturnValue({ dispose: vi.fn() }); + mockPtyProcess.onExit = vi.fn().mockReturnValue({ dispose: vi.fn() }); mockPtyProcess.write = vi.fn(); mockPtyProcess.resize = vi.fn(); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 1d39bc19645..af1bfd0c9eb 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -535,22 +535,50 @@ export class ShellExecutionService { }); }; - child.stdout.on('data', (data) => handleOutput(data, 'stdout')); - child.stderr.on('data', (data) => handleOutput(data, 'stderr')); - child.on('error', (err) => { + // Named handler refs so the background-promote branch below can + // detach them all and hand ownership of the child cleanly to the + // caller. Anonymous arrows here would leak: the still-running child + // would keep firing into our handlers (using a finalized decoder → + // TypeError, or duplicating events the caller now also receives). + const stdoutHandler = (data: Buffer) => handleOutput(data, 'stdout'); + const stderrHandler = (data: Buffer) => handleOutput(data, 'stderr'); + const errorHandler = (err: Error) => { error = err; handleExit(1, null); - }); + }; + const exitHandler = ( + code: number | null, + signal: NodeJS.Signals | null, + ) => { + if (child.pid) { + this.activeChildProcesses.delete(child.pid); + } + handleExit(code, signal); + }; + + child.stdout.on('data', stdoutHandler); + child.stderr.on('data', stderrHandler); + child.on('error', errorHandler); + + const detachServiceListeners = () => { + child.stdout?.off('data', stdoutHandler); + child.stderr?.off('data', stderrHandler); + child.off('error', errorHandler); + child.off('exit', exitHandler); + }; const abortHandler = async () => { - // Background-promote takeover: skip kill, drop the child from our - // active set (so cleanup() won't kill it later), flush our text - // buffers into a snapshot, and resolve immediately with - // `promoted: true` so the awaiting caller unblocks. The caller has - // attached its own listeners to the live child by this point. + // Background-promote takeover: skip kill, detach our listeners (so + // post-promote output doesn't leak into the foreground onOutputEvent + // or the now-finalized text decoder), drop the child from our active + // set (so cleanup() won't kill it later), flush our text buffers + // into a snapshot, and resolve immediately with `promoted: true` so + // the awaiting caller unblocks. The caller has attached its own + // listeners to the live child by this point and now owns the child. const reason = abortSignal.reason as ShellAbortReason | undefined; if (reason?.kind === 'background' && child.pid && !exited) { this.activeChildProcesses.delete(child.pid); + detachServiceListeners(); const { stdout: snapStdout, stderr: snapStderr, @@ -597,12 +625,7 @@ export class ShellExecutionService { this.activeChildProcesses.add(child.pid); } - child.on('exit', (code, signal) => { - if (child.pid) { - this.activeChildProcesses.delete(child.pid); - } - handleExit(code, signal); - }); + child.on('exit', exitHandler); function cleanup() { exited = true; @@ -717,11 +740,18 @@ export class ShellExecutionService { let isWriting = false; let hasStartedOutput = false; let renderTimeout: NodeJS.Timeout | null = null; + // Set to true by the background-promote branch so any in-flight + // processingChain callback or pending render short-circuits instead + // of emitting onOutputEvent / writing to the (now caller-owned) + // headlessTerminal. The PTY data disposable is also disposed in the + // same branch so no NEW work is enqueued — this guard handles the + // already-scheduled chain items. + let listenersDetached = false; const RENDER_THROTTLE_MS = 100; const renderFn = () => { - if (!isStreamingRawContent) { + if (!isStreamingRawContent || listenersDetached) { return; } @@ -842,30 +872,46 @@ export class ShellExecutionService { if (isBinary(sniffBuffer)) { isStreamingRawContent = false; - onOutputEvent({ type: 'binary_detected' }); + if (!listenersDetached) { + onOutputEvent({ type: 'binary_detected' }); + } } } if (isStreamingRawContent) { const decodedChunk = decoder!.decode(data, { stream: true }); isWriting = true; + // Allow in-flight writes to LAND in the headlessTerminal + // even after a background promote — the snapshot we'll + // serialize next reads from this buffer. The render() + // callback (and renderFn) is already guarded by + // listenersDetached, so no onOutputEvent fires. headlessTerminal.write(decodedChunk, () => { render(); isWriting = false; resolve(); }); } else { - onOutputEvent({ - type: 'binary_progress', - bytesReceived, - }); + if (!listenersDetached) { + onOutputEvent({ + type: 'binary_progress', + bytesReceived, + }); + } resolve(); } }), ); }; - ptyProcess.onData((data: string) => { + // Capture the IDisposables that node-pty returns so the + // background-promote branch below can hand the live PTY to the + // caller cleanly. Without dispose(), post-promote PTY data would + // continue calling our handleOutput → render → onOutputEvent (the + // foreground caller's downstream consumer that no longer owns this + // child) and post-promote PTY errors would `throw err` → process + // crash. + const dataDisposable = ptyProcess.onData((data: string) => { const bufferData = Buffer.from(data, 'utf-8'); handleOutput(bufferData); }); @@ -874,7 +920,7 @@ export class ShellExecutionService { // due to race conditions between the exit event and read operations. // This is a normal behavior on macOS/Linux and should not crash the app. // See: https://github.com/microsoft/node-pty/issues/178 - ptyProcess.on('error', (err: NodeJS.ErrnoException) => { + const ptyErrorHandler = (err: NodeJS.ErrnoException) => { if (isExpectedPtyReadExitError(err)) { // EIO is expected when the PTY process exits - ignore it return; @@ -882,9 +928,10 @@ export class ShellExecutionService { // Surface unexpected PTY errors to preserve existing crash behavior. throw err; - }); + }; + ptyProcess.on('error', ptyErrorHandler); - ptyProcess.onExit( + const exitDisposable = ptyProcess.onExit( ({ exitCode, signal }: { exitCode: number; signal?: number }) => { exited = true; abortSignal.removeEventListener('abort', abortHandler); @@ -954,17 +1001,46 @@ export class ShellExecutionService { ); const abortHandler = async () => { - // Background-promote takeover: skip kill, drop the PTY from the - // active set (so cleanup() won't kill it later), flush a snapshot - // of the output buffer captured so far, and resolve immediately - // with `promoted: true` so the awaiting caller unblocks. The - // caller has attached its own listeners to the live PTY by this - // point and owns its lifecycle from here on. + // Background-promote takeover: skip kill, dispose all our + // listeners on the live PTY (so post-promote data/exit/error don't + // leak into our foreground onOutputEvent or crash via the error + // handler's `throw err`), set the listenersDetached guard so any + // already-enqueued processingChain callback's onOutputEvent emits + // are suppressed (in-flight writes still LAND in headlessTerminal + // so the snapshot below reflects them), drain pending chain work, + // drop the PTY from the active set (so cleanup() won't kill it + // later), serialize the terminal as the snapshot, and resolve + // immediately with `promoted: true` so the awaiting caller + // unblocks. The caller has attached its own listeners to the live + // PTY by this point and owns its lifecycle from here on. const reason = abortSignal.reason as ShellAbortReason | undefined; if (reason?.kind === 'background' && ptyProcess.pid && !exited) { exited = true; + listenersDetached = true; abortSignal.removeEventListener('abort', abortHandler); + dataDisposable.dispose(); + exitDisposable.dispose(); + ptyProcess.off('error', ptyErrorHandler); + if (renderTimeout) { + clearTimeout(renderTimeout); + renderTimeout = null; + } this.activePtys.delete(ptyProcess.pid); + + // Drain in-flight chain work (already-enqueued + // headlessTerminal.write callbacks) so the snapshot reflects + // the last batch of bytes the PTY emitted before promote. + // Bounded by SIGKILL_TIMEOUT_MS so the caller's await never + // blocks indefinitely if a write callback is stuck. + const drain = () => + new Promise((res) => setImmediate(res)).then( + () => processingChain, + ); + await Promise.race([ + processingChain.then(drain).then(drain), + new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)), + ]); + const finalBuffer = Buffer.concat(outputChunks); let snapshot = ''; try { From 9008717b8a261e632d3e2035c87fe58d045e8da9 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 5 May 2026 22:44:30 +0800 Subject: [PATCH 3/9] fix(core): defensive hardening for ShellExecutionService background-promote (resolve 2nd review pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 6 follow-up [Suggestion] threads on PR-1 of #3831 — all substantive code-quality issues raised by the second-pass review of the dispose-based detach commit (8e8e18ca7): - **Exhaustive switch on `ShellAbortReason.kind`** (both abort handlers). Earlier `if (reason?.kind === 'background')` form silently fell through to kill for any unrecognized variant — a future `{ kind: 'suspend' }` would have killed the process with zero compile-time signal. Switched to `switch (kind)` with a `never`-typed default that runs `debugLogger.warn` and falls back to the safest behavior (cancel/kill). Each branch is now extracted into a named helper (`performBackgroundPromote` / `performCancelKill`) so the switch body stays a single screenful. - **Each `dispose()` wrapped in its own try/catch** (PTY). node-pty's `IDisposable` contract doesn't guarantee no-throw. Without per-dispose try/catch a single throwing dispose() would skip subsequent cleanup (the other dispose, off('error'), activePtys.delete, drain, resolve) and the caller would hang forever on `await result`. Each call now logs via debugLogger.warn on failure but continues. - **`.catch(() => undefined)` on the processingChain side of the drain race** (PTY). `Promise.race([processingChain.then(drain).then(drain), timeout])` would propagate a chain rejection out of the race; since `addEventListener` doesn't await our handler, the rejection became unhandled and `resolve()` was never called → caller hung. Now the rejection is swallowed; the timeout side still terminates the race on time. - **Drain-timeout truncation now emits a diagnostic warning** (PTY). Previously the 200ms drain timeout could fire, the snapshot would be taken with the buffer in mid-write state, and the result.output would be silently truncated. Race result is now observed via a symbol sentinel; when the timeout side wins, debugLogger.warn fires pointing the user at rawOutput as the un-truncated fallback. - **Snapshot serialize failure logs instead of swallowing silently** (PTY). Empty `catch {}` made result.output indistinguishable from "command produced no output" if serializeTerminalToText threw. Now `debugLogger.warn` with the error message leaves a trail for support bundles. - **Dedicated `PROMOTE_DRAIN_TIMEOUT_MS` constant** separated from `SIGKILL_TIMEOUT_MS`. Both are 200ms today, but they have unrelated reasons-to-change (kill escalation timing vs. promote drain ceiling) — sharing the constant means tuning one would silently change the other. Also adds a module-level `debugLogger = createDebugLogger('SHELL_EXECUTION')` since the service had no logging surface before this commit. 58 / 58 tests pass; tsc clean; ESLint clean. No new tests added: the new behaviors (timeout sentinel firing, dispose throw, exhaustive switch default) are defensive log-only paths; existing handoff tests already cover the happy path. Adding mock-throw tests is reasonable follow-up but not blocking. --- .../src/services/shellExecutionService.ts | 348 ++++++++++++------ 1 file changed, 228 insertions(+), 120 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index af1bfd0c9eb..a688d564051 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -21,9 +21,20 @@ import { type AnsiOutput, } from '../utils/terminalSerializer.js'; import { normalizePathEnvForWindows } from '../utils/windowsPath.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; const { Terminal } = pkg; +const debugLogger = createDebugLogger('SHELL_EXECUTION'); + const SIGKILL_TIMEOUT_MS = 200; +/** + * Bound on how long the background-promote drain waits for in-flight + * processingChain callbacks to finish writing into the headless terminal + * before snapshotting it. Kept separate from SIGKILL_TIMEOUT_MS so that + * tuning kill escalation doesn't silently change drain behavior; same + * 200ms default today, but the two have unrelated reasons-to-change. + */ +const PROMOTE_DRAIN_TIMEOUT_MS = 200; /** * On Windows with PowerShell, prefix the command with a statement that forces @@ -567,54 +578,78 @@ export class ShellExecutionService { child.off('exit', exitHandler); }; - const abortHandler = async () => { - // Background-promote takeover: skip kill, detach our listeners (so - // post-promote output doesn't leak into the foreground onOutputEvent - // or the now-finalized text decoder), drop the child from our active - // set (so cleanup() won't kill it later), flush our text buffers - // into a snapshot, and resolve immediately with `promoted: true` so - // the awaiting caller unblocks. The caller has attached its own - // listeners to the live child by this point and now owns the child. - const reason = abortSignal.reason as ShellAbortReason | undefined; - if (reason?.kind === 'background' && child.pid && !exited) { - this.activeChildProcesses.delete(child.pid); - detachServiceListeners(); - const { - stdout: snapStdout, - stderr: snapStderr, - finalBuffer, - } = cleanup(); - const separator = snapStdout.endsWith('\n') ? '' : '\n'; - const combined = - snapStdout + - (snapStderr ? (snapStdout ? separator : '') + snapStderr : ''); - resolve({ - rawOutput: finalBuffer, - output: stripAnsi(combined).trim(), - exitCode: null, - signal: null, - error: null, - aborted: true, - promoted: true, - pid: child.pid, - executionMethod: 'child_process', - }); - return; - } + const performBackgroundPromote = (): void => { + if (!child.pid || exited) return; + // Detach our listeners (so post-promote output doesn't leak + // into the foreground onOutputEvent or the now-finalized text + // decoder), drop the child from our active set (so cleanup() + // won't kill it later), flush our text buffers into a snapshot, + // and resolve immediately with `promoted: true` so the awaiting + // caller unblocks. The caller has attached its own listeners + // by this point and now owns the child. + this.activeChildProcesses.delete(child.pid); + detachServiceListeners(); + const { + stdout: snapStdout, + stderr: snapStderr, + finalBuffer, + } = cleanup(); + const separator = snapStdout.endsWith('\n') ? '' : '\n'; + const combined = + snapStdout + + (snapStderr ? (snapStdout ? separator : '') + snapStderr : ''); + resolve({ + rawOutput: finalBuffer, + output: stripAnsi(combined).trim(), + exitCode: null, + signal: null, + error: null, + aborted: true, + promoted: true, + pid: child.pid, + executionMethod: 'child_process', + }); + }; - if (child.pid && !exited) { - if (isWindows) { - cpSpawn('taskkill', ['/pid', child.pid.toString(), '/f', '/t']); - } else { - try { - process.kill(-child.pid, 'SIGTERM'); - await new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)); - if (!exited) { - process.kill(-child.pid, 'SIGKILL'); - } - } catch (_e) { - if (!exited) child.kill('SIGKILL'); + const performCancelKill = async (): Promise => { + if (!child.pid || exited) return; + if (isWindows) { + cpSpawn('taskkill', ['/pid', child.pid.toString(), '/f', '/t']); + } else { + try { + process.kill(-child.pid, 'SIGTERM'); + await new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)); + if (!exited) { + process.kill(-child.pid, 'SIGKILL'); } + } catch (_e) { + if (!exited) child.kill('SIGKILL'); + } + } + }; + + const abortHandler = async () => { + // Default reason (none set) is treated as cancel — historical + // behavior. Switch on `kind` so any future ShellAbortReason + // variant fails the type-check at the `never` default rather + // than silently falling through to the kill path. (Earlier + // if-else form would have silently killed the process for + // e.g. a future `{ kind: 'suspend' }` — review feedback.) + const reason = abortSignal.reason as ShellAbortReason | undefined; + const kind: ShellAbortReason['kind'] = reason?.kind ?? 'cancel'; + switch (kind) { + case 'background': + performBackgroundPromote(); + return; + case 'cancel': + await performCancelKill(); + return; + default: { + const _exhaustive: never = kind; + debugLogger.warn( + `Unknown ShellAbortReason kind, falling back to cancel/kill: ${String(_exhaustive)}`, + ); + await performCancelKill(); } } }; @@ -1000,89 +1035,162 @@ export class ShellExecutionService { }, ); - const abortHandler = async () => { - // Background-promote takeover: skip kill, dispose all our - // listeners on the live PTY (so post-promote data/exit/error don't - // leak into our foreground onOutputEvent or crash via the error - // handler's `throw err`), set the listenersDetached guard so any - // already-enqueued processingChain callback's onOutputEvent emits - // are suppressed (in-flight writes still LAND in headlessTerminal - // so the snapshot below reflects them), drain pending chain work, - // drop the PTY from the active set (so cleanup() won't kill it - // later), serialize the terminal as the snapshot, and resolve - // immediately with `promoted: true` so the awaiting caller - // unblocks. The caller has attached its own listeners to the live - // PTY by this point and owns its lifecycle from here on. - const reason = abortSignal.reason as ShellAbortReason | undefined; - if (reason?.kind === 'background' && ptyProcess.pid && !exited) { - exited = true; - listenersDetached = true; - abortSignal.removeEventListener('abort', abortHandler); + const performBackgroundPromote = async (): Promise => { + if (!ptyProcess.pid || exited) return; + // Skip kill, dispose all our listeners on the live PTY (so + // post-promote data/exit/error don't leak into our foreground + // onOutputEvent or crash via the error handler's `throw err`), + // set the listenersDetached guard so any already-enqueued + // processingChain callback's onOutputEvent emits are + // suppressed (in-flight writes still LAND in headlessTerminal + // so the snapshot below reflects them), drain pending chain + // work, drop the PTY from the active set (so cleanup() won't + // kill it later), serialize the terminal as the snapshot, and + // resolve immediately with `promoted: true` so the awaiting + // caller unblocks. The caller has attached its own listeners + // by this point and owns the PTY's lifecycle. + exited = true; + listenersDetached = true; + abortSignal.removeEventListener('abort', abortHandler); + // Each dispose() in its own try/catch — node-pty's IDisposable + // contract doesn't guarantee no-throw, and we must run all + // teardown steps even if one throws (otherwise activePtys.delete + // / drain / resolve could be skipped and the caller would hang). + try { dataDisposable.dispose(); + } catch (e) { + debugLogger.warn( + `dataDisposable.dispose() threw during background-promote: ${e instanceof Error ? e.message : String(e)}`, + ); + } + try { exitDisposable.dispose(); + } catch (e) { + debugLogger.warn( + `exitDisposable.dispose() threw during background-promote: ${e instanceof Error ? e.message : String(e)}`, + ); + } + try { ptyProcess.off('error', ptyErrorHandler); - if (renderTimeout) { - clearTimeout(renderTimeout); - renderTimeout = null; - } - this.activePtys.delete(ptyProcess.pid); + } catch (e) { + debugLogger.warn( + `ptyProcess.off('error') threw during background-promote: ${e instanceof Error ? e.message : String(e)}`, + ); + } + if (renderTimeout) { + clearTimeout(renderTimeout); + renderTimeout = null; + } + this.activePtys.delete(ptyProcess.pid); + + // Drain in-flight chain work (already-enqueued + // headlessTerminal.write callbacks) so the snapshot reflects + // the last batch of bytes the PTY emitted before promote. + // Bounded by PROMOTE_DRAIN_TIMEOUT_MS so the caller's await + // never blocks indefinitely if a write callback is stuck. + // The drain side may reject (a prior chain item threw); swallow + // via .catch — abort handlers run via addEventListener which + // doesn't await our return, so a leaked rejection here would + // become unhandled and the caller would hang waiting on resolve. + // Race result is observed (not just discarded) so we can warn + // when the timeout side won — without that the snapshot may be + // truncated with no diagnostic trail. + const TIMEOUT_SENTINEL = Symbol('drain-timeout'); + const drain = () => + new Promise((res) => setImmediate(res)).then( + () => processingChain, + ); + const winner = await Promise.race([ + processingChain + .then(drain) + .then(drain) + .catch(() => undefined), + new Promise((res) => + setTimeout(() => res(TIMEOUT_SENTINEL), PROMOTE_DRAIN_TIMEOUT_MS), + ), + ]); + if (winner === TIMEOUT_SENTINEL) { + debugLogger.warn( + `Background-promote drain hit the ${PROMOTE_DRAIN_TIMEOUT_MS}ms ` + + `timeout before processingChain settled. The output snapshot ` + + `may be missing the very last batch of bytes the PTY emitted ` + + `before promote (rawOutput in the result still has the full ` + + `buffer the caller can re-render).`, + ); + } - // Drain in-flight chain work (already-enqueued - // headlessTerminal.write callbacks) so the snapshot reflects - // the last batch of bytes the PTY emitted before promote. - // Bounded by SIGKILL_TIMEOUT_MS so the caller's await never - // blocks indefinitely if a write callback is stuck. - const drain = () => - new Promise((res) => setImmediate(res)).then( - () => processingChain, - ); - await Promise.race([ - processingChain.then(drain).then(drain), - new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)), - ]); + const finalBuffer = Buffer.concat(outputChunks); + let snapshot = ''; + try { + snapshot = serializeTerminalToText(headlessTerminal) ?? ''; + } catch (serErr) { + // Best-effort snapshot — terminal serialization may fail if + // the PTY is mid-render. Empty snapshot is acceptable since + // the caller has rawOutput, but log so the failure leaves a + // diagnostic trail (otherwise an empty `output` is + // indistinguishable from "command produced no output"). + debugLogger.warn( + `Background-promote snapshot serialization failed: ${serErr instanceof Error ? serErr.message : String(serErr)}. ` + + `Result.output will be empty; caller should fall back to rawOutput.`, + ); + } + resolve({ + rawOutput: finalBuffer, + output: snapshot, + exitCode: null, + signal: null, + error, + aborted: true, + promoted: true, + pid: ptyProcess.pid, + executionMethod: + (ptyInfo?.name as 'node-pty' | 'lydell-node-pty') ?? 'node-pty', + }); + }; - const finalBuffer = Buffer.concat(outputChunks); - let snapshot = ''; + const performCancelKill = async (): Promise => { + if (!ptyProcess.pid || exited) return; + if (os.platform() === 'win32') { + ptyProcess.kill(); + } else { try { - snapshot = serializeTerminalToText(headlessTerminal) ?? ''; - } catch { - // Best-effort snapshot — terminal serialization may fail if the - // PTY is mid-render; an empty snapshot is acceptable since the - // caller already has the rawOutput buffer below. + // Send SIGTERM first to allow graceful shutdown + process.kill(-ptyProcess.pid, 'SIGTERM'); + await new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)); + if (!exited) { + // Escalate to SIGKILL if still running + process.kill(-ptyProcess.pid, 'SIGKILL'); + } + } catch (_e) { + // Fallback to killing just the process if the group kill fails + if (!exited) { + ptyProcess.kill(); + } } - resolve({ - rawOutput: finalBuffer, - output: snapshot, - exitCode: null, - signal: null, - error, - aborted: true, - promoted: true, - pid: ptyProcess.pid, - executionMethod: - (ptyInfo?.name as 'node-pty' | 'lydell-node-pty') ?? 'node-pty', - }); - return; } + }; - if (ptyProcess.pid && !exited) { - if (os.platform() === 'win32') { - ptyProcess.kill(); - } else { - try { - // Send SIGTERM first to allow graceful shutdown - process.kill(-ptyProcess.pid, 'SIGTERM'); - await new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS)); - if (!exited) { - // Escalate to SIGKILL if still running - process.kill(-ptyProcess.pid, 'SIGKILL'); - } - } catch (_e) { - // Fallback to killing just the process if the group kill fails - if (!exited) { - ptyProcess.kill(); - } - } + const abortHandler = async () => { + // Switch on the discriminated `kind` so any future + // ShellAbortReason variant fails the type-check at the + // `never` default rather than silently falling through to the + // kill path (review feedback — earlier if-else form would have + // silently killed for e.g. a future `{ kind: 'suspend' }`). + const reason = abortSignal.reason as ShellAbortReason | undefined; + const kind: ShellAbortReason['kind'] = reason?.kind ?? 'cancel'; + switch (kind) { + case 'background': + await performBackgroundPromote(); + return; + case 'cancel': + await performCancelKill(); + return; + default: { + const _exhaustive: never = kind; + debugLogger.warn( + `Unknown ShellAbortReason kind, falling back to cancel/kill: ${String(_exhaustive)}`, + ); + await performCancelKill(); } } }; From 094fff09caf6ef4833beb750c241be54b350c0b0 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 6 May 2026 12:14:16 +0800 Subject: [PATCH 4/9] =?UTF-8?q?fix(core):=20real=20bug=20=E2=80=94=20ptyPr?= =?UTF-8?q?ocess.off=20=E2=86=92=20removeListener;=20defensive=20abort-rea?= =?UTF-8?q?son=20read?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the third review pass on PR-1 of #3831 — 1 real bug + 2 defensive hardenings: - **Real bug: `ptyProcess.off('error', ...)` throws TypeError at runtime** (line ~1074). `@lydell/node-pty`'s `IPty` interface exposes the legacy Node EventEmitter `removeListener`, not the modern `off` alias. Previous form threw, the surrounding try/catch swallowed it (post-prior-pass dispose hardening), but the old `ptyErrorHandler` stayed registered — so a post-promote PTY error would still hit our foreground handler and `throw err`, breaking the handoff contract that PR-1's whole listener-detach work is supposed to enforce. Switched to `removeListener`. The catch + warn stays as defense-in-depth; the message wording is updated. - **Prototype-pollution-safe `kind` read** (extracted to module-level helper `getShellAbortReasonKind`). The previous `reason?.kind` walked the prototype chain — a polluted `Object.prototype.kind = 'background'` would silently route `abortController.abort({})` (any plain object reason) into the promote branch and skip the kill. Lifecycle/safety branch deserves the extra check. Helper now: rejects non-object reasons; reads `kind` only as an OWN property (`hasOwnProperty`); whitelists against `'background' | 'cancel'`; defaults to `'cancel'` (the safe historical behavior) for everything else. Both abort handlers (childProcess + PTY) now share this helper. - **`streamStdout: true` + background-promote = silent empty snapshot** (childProcess `performBackgroundPromote`). The promote snapshot reads from the `stdout` / `stderr` string accumulators; but in `streamStdout` mode `handleOutput` forwards bytes through `onOutputEvent` and skips the accumulators entirely. Today PR-1's only call site (foreground shell.ts) uses `streamStdout: false`, so the combination is unreachable — but if a future caller pairs the two, `result.output` would be empty with no diagnostic. Added a `debugLogger.warn` when the combination occurs, pointing the caller at `rawOutput` as the fallback. Cheaper than building a parallel accumulator just for this latent case. 58 / 58 tests pass; tsc clean; ESLint clean. --- .../src/services/shellExecutionService.ts | 60 +++++++++++++++++-- 1 file changed, 54 insertions(+), 6 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index a688d564051..7c5bb6dabf4 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -36,6 +36,30 @@ const SIGKILL_TIMEOUT_MS = 200; */ const PROMOTE_DRAIN_TIMEOUT_MS = 200; +/** + * Read the `kind` discriminator off `abortSignal.reason` defensively: + * - Reject non-object reasons (DOMException, strings, numbers). + * - Read the `kind` property as an OWN property only — without + * `hasOwnProperty`, a polluted `Object.prototype.kind = 'background'` + * would force the kill path through the promote branch on any plain + * `abortController.abort({})`. Lifecycle/safety branches deserve the + * extra check. + * - Whitelist the value against the known union — anything else (typos, + * future-untyped variants) defaults to `'cancel'` so the historical + * kill behavior is preserved as the safe fallback. + */ +function getShellAbortReasonKind(reason: unknown): ShellAbortReason['kind'] { + if ( + reason !== null && + typeof reason === 'object' && + Object.prototype.hasOwnProperty.call(reason, 'kind') + ) { + const kind = (reason as { kind?: unknown }).kind; + if (kind === 'background' || kind === 'cancel') return kind; + } + return 'cancel'; +} + /** * On Windows with PowerShell, prefix the command with a statement that forces * UTF-8 output encoding so that CJK and other non-ASCII characters are emitted @@ -587,6 +611,26 @@ export class ShellExecutionService { // and resolve immediately with `promoted: true` so the awaiting // caller unblocks. The caller has attached its own listeners // by this point and now owns the child. + // + // INVARIANT: this snapshot path reads from `stdout` / `stderr` + // string accumulators (populated by handleOutput's buffered-text + // branch). Under `streamStdout: true`, output is forwarded + // through `onOutputEvent` and NOT accumulated into stdout/stderr, + // so the promoted snapshot would be silently empty. PR-1's only + // caller (foreground shell.ts) uses streamStdout: false, so + // there's no live combination today; if a future caller pairs + // `streamStdout: true` with `{ kind: 'background' }`, log so the + // empty-snapshot is observable rather than mysterious. The + // caller still has rawOutput as a fallback. + if (streamStdout) { + debugLogger.warn( + 'Background-promote on a streamStdout=true child_process: ' + + 'snapshot accumulators were never populated (output went ' + + 'through onOutputEvent), so result.output will be empty. ' + + 'Caller should fall back to rawOutput, or assemble its ' + + 'own snapshot from the data events it received.', + ); + } this.activeChildProcesses.delete(child.pid); detachServiceListeners(); const { @@ -635,8 +679,7 @@ export class ShellExecutionService { // than silently falling through to the kill path. (Earlier // if-else form would have silently killed the process for // e.g. a future `{ kind: 'suspend' }` — review feedback.) - const reason = abortSignal.reason as ShellAbortReason | undefined; - const kind: ShellAbortReason['kind'] = reason?.kind ?? 'cancel'; + const kind = getShellAbortReasonKind(abortSignal.reason); switch (kind) { case 'background': performBackgroundPromote(); @@ -1071,10 +1114,16 @@ export class ShellExecutionService { ); } try { - ptyProcess.off('error', ptyErrorHandler); + // @lydell/node-pty's IPty exposes `removeListener` (Node's + // EventEmitter API), not the modern `off` alias. Calling + // `off` here used to throw TypeError at runtime — caught + // and logged but the handler stayed registered, so a + // post-promote PTY error would still run our foreground + // handler's `throw err` and break the handoff contract. + ptyProcess.removeListener('error', ptyErrorHandler); } catch (e) { debugLogger.warn( - `ptyProcess.off('error') threw during background-promote: ${e instanceof Error ? e.message : String(e)}`, + `ptyProcess.removeListener('error') threw during background-promote: ${e instanceof Error ? e.message : String(e)}`, ); } if (renderTimeout) { @@ -1176,8 +1225,7 @@ export class ShellExecutionService { // `never` default rather than silently falling through to the // kill path (review feedback — earlier if-else form would have // silently killed for e.g. a future `{ kind: 'suspend' }`). - const reason = abortSignal.reason as ShellAbortReason | undefined; - const kind: ShellAbortReason['kind'] = reason?.kind ?? 'cancel'; + const kind = getShellAbortReasonKind(abortSignal.reason); switch (kind) { case 'background': await performBackgroundPromote(); From 4cc558b3d5991516d38e81c2c82256b7e7a8734e Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 6 May 2026 15:09:02 +0800 Subject: [PATCH 5/9] fix(core): liveness check + throw-safe abort-reason read + encoding-aware PTY snapshot (resolve 4th review pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves 6 threads on PR-1 of #3831 — 1 Critical + 1 real bug + 2 quality + 2 test-coverage: - **[Critical] `getShellAbortReasonKind` throw-safe property read.** Previous form read `reason.kind` after only checking that `kind` is an own property. An own accessor that throws (or a Proxy with a trapping getter) would throw before the helper reached either the cancel kill path or the background promote path. Abort handlers are dispatched async and not awaited by AbortSignal, so a leaked throw here would have left the shell process alive instead of being killed on cancel — quietly. Wrapped the property read in try/catch with a fall-back to the safe 'cancel' kill behavior. - **Real bug: child_process post-exit race in background-promote** (`performBackgroundPromote`). The child may have already exited but the 'exit' event hasn't reached our handler yet (Node delivers events on the next microtask). Promoting in that window would detach our exit listener and report `promoted: true` for a process that's already dead — the caller would hold an inert pid expecting to take over. Now we read `child.exitCode` / `child.signalCode` before detaching: if either is non-null, fall through and let the pending exit handler resolve normally with the real exit info. Mirrored mock setup so `exitCode` / `signalCode` default to `null` (matching real ChildProcess) instead of `undefined`. - **PTY snapshot: re-decode + replay (mirror exit-path encoding).** The promoted snapshot was serializing `headlessTerminal` directly, which was fed by a streaming decoder initialized from the first-chunk encoding heuristic. When early output is ASCII-only but later output is in a different encoding (GBK / Shift-JIS / etc.), this produces mojibake — and the normal exit path doesn't, because it re-decodes `finalBuffer` with `getCachedEncodingForBuffer` and replays through a fresh terminal. Now mirrors that logic so `result.output` shape matches across the two paths. Direct-serialize remains as a last-ditch fallback if replay throws. - **Switch `default` no longer emits a runtime warn.** Reviewer noted the helper's whitelist made the `default: { _exhaustive: never }` branch unreachable at runtime — the `debugLogger.warn` in it could never fire. Kept the `_: never = kind` type assertion (so a future ShellAbortReason variant forces a TS error here, directing the developer to extend BOTH the helper's whitelist AND add a `case`), removed the unreachable warn. Added a comment that the assertion is the static-only safety net the union expansion would trigger. - **Direct unit tests for `getShellAbortReasonKind`** (8 cases). The helper's prototype-pollution defense is the main reason it exists; if `hasOwnProperty` is accidentally removed the regression would silently send `abortController.abort({})` (any plain reason) into the promote path. Exported the helper and added direct tests for: null / undefined, non-object, empty object (no own kind), prototype- only kind (pollution), unknown kind value, throwing accessor, Proxy trap, and the two happy paths. - **`removeListener` regression guard.** The fix to call `ptyProcess.removeListener('error', ...)` instead of `.off(...)` matters because `@lydell/node-pty`'s IPty interface only exposes `removeListener` — `.off()` throws TypeError on a real PTY but the EventEmitter mock tolerates both. Added a test that spies on both methods and asserts the production code uses `removeListener` for the 'error' event, so a future swap back to `.off()` regresses loudly under the mock instead of silently. 68 / 68 tests pass (58 baseline + 9 helper boundary + 1 removeListener guard + 1 post-exit race); tsc clean; ESLint clean. --- .../services/shellExecutionService.test.ts | 168 +++++++++++++++++- .../src/services/shellExecutionService.ts | 106 +++++++++-- 2 files changed, 256 insertions(+), 18 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 9e98d7cd3ae..2ae3149d467 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -21,7 +21,10 @@ import type { ShellAbortReason, ShellOutputEvent, } from './shellExecutionService.js'; -import { ShellExecutionService } from './shellExecutionService.js'; +import { + getShellAbortReasonKind, + ShellExecutionService, +} from './shellExecutionService.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; const { Terminal } = pkg; @@ -700,6 +703,40 @@ describe('ShellExecutionService', () => { expect(exitDisposableStub.dispose).toHaveBeenCalled(); }); + it("post-promotion: ptyProcess error listener is removed via 'removeListener', NOT 'off' (regression guard for @lydell/node-pty)", async () => { + // node EventEmitter exposes both `off` (Node 10+) and the legacy + // `removeListener`, but @lydell/node-pty's IPty interface only + // surfaces `removeListener` — calling `.off(...)` on a real PTY + // throws TypeError. Pin that the production code path uses + // `removeListener` so a future refactor swapping to `.off()` + // doesn't silently regress under the EventEmitter mock (which + // tolerates both). + const removeListenerSpy = vi.spyOn(mockPtyProcess, 'removeListener'); + const offSpy = vi.spyOn(mockPtyProcess, 'off'); + + const { result } = await simulateExecution( + 'tail -f /tmp/never.log', + (_pty, abortController) => { + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + }, + ); + + expect(result.promoted).toBe(true); + // The 'error' handler is removed via legacy API; `.off` must not + // appear in the production teardown path. + expect(removeListenerSpy).toHaveBeenCalledWith( + 'error', + expect.any(Function), + ); + const offErrorCalls = offSpy.mock.calls.filter( + ([event]) => event === 'error', + ); + expect(offErrorCalls).toEqual([]); + }); + it('post-promotion: PTY exit does NOT re-resolve the result (already resolved with promoted)', async () => { // Pin: even if the still-running child later exits naturally and the // caller's own exit listener fires, our foreground result Promise @@ -1057,6 +1094,22 @@ describe('ShellExecutionService child_process fallback', () => { value: 12345, configurable: true, }); + // Mirror real Node ChildProcess: `exitCode` / `signalCode` are `null` + // while the child is alive and become a number / signal name on + // exit. The background-promote liveness guard reads these to detect + // an exit that fired between abort dispatch and the abort handler + // run, and a default of `undefined` would mistakenly look terminal + // and skip the promote. + Object.defineProperty(mockChildProcess, 'exitCode', { + value: null, + writable: true, + configurable: true, + }); + Object.defineProperty(mockChildProcess, 'signalCode', { + value: null, + writable: true, + configurable: true, + }); mockCpSpawn.mockReturnValue(mockChildProcess); }); @@ -1350,6 +1403,45 @@ describe('ShellExecutionService child_process fallback', () => { expect(result.output).not.toContain('post-promote-stderr'); }); + it('post-exit race: background-promote refuses if child is already terminal (exitCode/signalCode non-null)', async () => { + // Race window: the child may have exited (exitCode set) but the + // 'exit' event hasn't reached our handler yet because Node delivers + // child_process events on the next microtask. Promoting in that + // window would detach our exit listener and report `promoted: true` + // for a process that's already dead — the caller would hold an + // inert pid expecting to take over. Production code reads + // exitCode / signalCode before detaching; if either is non-null, + // it falls through and lets the pending exit handler resolve + // normally with the real exit info. + mockPlatform.mockReturnValue('linux'); + const { result } = await simulateExecution( + 'fast-and-cancelled', + (cp, abortController) => { + // Simulate the race: pretend the child has already exited + // (exitCode set on the ChildProcess) but the 'exit' event + // emit is queued behind the abort dispatch. + Object.defineProperty(cp, 'exitCode', { + value: 0, + writable: true, + configurable: true, + }); + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + // Now drain the pending exit + close events; the normal + // exit path should resolve the result. + cp.emit('exit', 0, null); + cp.emit('close', 0, null); + }, + ); + + // Result is the normal exit shape, not the promoted shape. + expect(result.promoted).toBeUndefined(); + expect(result.aborted).toBe(true); // abortSignal.aborted is still true + expect(result.exitCode).toBe(0); + }); + it('post-promotion: child exit does NOT re-resolve the result with a non-promoted shape', async () => { mockPlatform.mockReturnValue('linux'); // Pin: even if the still-running child later exits naturally and the @@ -1665,3 +1757,77 @@ describe('ShellExecutionService execution method selection', () => { expect(result.executionMethod).toBe('child_process'); }); }); + +describe('getShellAbortReasonKind (defensive abort-reason read)', () => { + it("returns 'cancel' for null reason (e.g. plain abortController.abort())", () => { + expect(getShellAbortReasonKind(null)).toBe('cancel'); + expect(getShellAbortReasonKind(undefined)).toBe('cancel'); + }); + + it("returns 'cancel' for non-object reasons (string / number / DOMException)", () => { + expect(getShellAbortReasonKind('background')).toBe('cancel'); + expect(getShellAbortReasonKind(42)).toBe('cancel'); + expect(getShellAbortReasonKind(true)).toBe('cancel'); + // DOMException-like object — not the real DOMException constructor in + // the test runtime, but the principle is the same: a non-discriminated + // object reason without an own `kind` falls back to cancel. + expect(getShellAbortReasonKind(new Error('aborted'))).toBe('cancel'); + }); + + it("returns 'cancel' for an empty object (no own kind)", () => { + expect(getShellAbortReasonKind({})).toBe('cancel'); + }); + + it("returns 'cancel' when 'kind' lives only on the prototype (pollution defense)", () => { + const polluted: Record = Object.create({ + kind: 'background', + }); + // hasOwnProperty('kind') is false → helper rejects the prototype-only kind + expect(getShellAbortReasonKind(polluted)).toBe('cancel'); + }); + + it("returns 'cancel' for an unknown kind value (typo / future-untyped variant)", () => { + expect(getShellAbortReasonKind({ kind: 'suspend' })).toBe('cancel'); + expect(getShellAbortReasonKind({ kind: 'BACKGROUND' })).toBe('cancel'); + expect(getShellAbortReasonKind({ kind: 42 })).toBe('cancel'); + }); + + it("returns 'cancel' when reading 'kind' throws (accessor / Proxy trap)", () => { + const throwingReason = Object.defineProperty({}, 'kind', { + enumerable: true, + configurable: true, + get() { + throw new Error('accessor blew up'); + }, + }); + expect(getShellAbortReasonKind(throwingReason)).toBe('cancel'); + + const proxyReason = new Proxy( + {}, + { + get(_target, prop) { + if (prop === 'kind') throw new Error('proxy trap blew up'); + return undefined; + }, + getOwnPropertyDescriptor(_target, prop) { + if (prop === 'kind') { + return { configurable: true, enumerable: true, value: 'unused' }; + } + return undefined; + }, + }, + ); + expect(getShellAbortReasonKind(proxyReason)).toBe('cancel'); + }); + + it("returns 'background' for the canonical happy-path reason", () => { + expect(getShellAbortReasonKind({ kind: 'background' })).toBe('background'); + expect( + getShellAbortReasonKind({ kind: 'background', shellId: 'bg_x' }), + ).toBe('background'); + }); + + it("returns 'cancel' for the canonical cancel reason", () => { + expect(getShellAbortReasonKind({ kind: 'cancel' })).toBe('cancel'); + }); +}); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 7c5bb6dabf4..4296cfb77d0 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -44,18 +44,34 @@ const PROMOTE_DRAIN_TIMEOUT_MS = 200; * would force the kill path through the promote branch on any plain * `abortController.abort({})`. Lifecycle/safety branches deserve the * extra check. + * - Wrap the property read in try/catch — an own getter or a `Proxy` + * trap may throw during inspection. A throw here would propagate up + * past the abort handler (which is dispatched async and not awaited + * by AbortSignal), leaving the shell process alive instead of being + * killed on cancel. We swallow the throw and fall back to 'cancel'. * - Whitelist the value against the known union — anything else (typos, * future-untyped variants) defaults to `'cancel'` so the historical * kill behavior is preserved as the safe fallback. + * + * Exported for direct unit testing of all six edge cases (null / + * non-object / `{}` no own kind / prototype-only kind / unknown kind / + * throwing accessor) — the integration tests only exercise the three + * happy-path inputs. */ -function getShellAbortReasonKind(reason: unknown): ShellAbortReason['kind'] { +export function getShellAbortReasonKind( + reason: unknown, +): ShellAbortReason['kind'] { if ( reason !== null && typeof reason === 'object' && Object.prototype.hasOwnProperty.call(reason, 'kind') ) { - const kind = (reason as { kind?: unknown }).kind; - if (kind === 'background' || kind === 'cancel') return kind; + try { + const kind = (reason as { kind?: unknown }).kind; + if (kind === 'background' || kind === 'cancel') return kind; + } catch { + // Throwing accessor / Proxy trap — fall back to safe kill below. + } } return 'cancel'; } @@ -604,6 +620,25 @@ export class ShellExecutionService { const performBackgroundPromote = (): void => { if (!child.pid || exited) return; + // Race guard: the child may have already exited but the 'exit' + // event hasn't reached our handler yet (Node delivers + // child_process events on the next microtask). Promoting in + // that window would detach our exit listener, leak the + // already-terminal exit code, and report `promoted: true` to + // the caller for a process that's already dead — they'd hold + // an inert pid expecting to take over. Check exitCode / + // signalCode before detaching: if either is non-null the + // child is gone, so leave the listeners alone and let the + // pending exit handler fire normally with the real exit info. + if (child.exitCode !== null || child.signalCode !== null) { + debugLogger.debug( + `Background-promote requested for pid ${child.pid} but child ` + + `is already terminal (exitCode=${child.exitCode}, ` + + `signalCode=${child.signalCode}); falling through to the ` + + `normal exit-handled resolution.`, + ); + return; + } // Detach our listeners (so post-promote output doesn't leak // into the foreground onOutputEvent or the now-finalized text // decoder), drop the child from our active set (so cleanup() @@ -688,11 +723,17 @@ export class ShellExecutionService { await performCancelKill(); return; default: { + // Unreachable at runtime: getShellAbortReasonKind whitelists + // the return to the union members, so this branch only + // exists to force a TS error if the `ShellAbortReason` union + // ever gains a new variant — that error directs the + // developer to (1) extend the helper's whitelist and + // (2) add a `case` here. Without this exhaustiveness check + // the helper's whitelist and the switch could drift apart + // silently when the union grows. const _exhaustive: never = kind; - debugLogger.warn( - `Unknown ShellAbortReason kind, falling back to cancel/kill: ${String(_exhaustive)}`, - ); await performCancelKill(); + return _exhaustive; } } }; @@ -1171,17 +1212,42 @@ export class ShellExecutionService { const finalBuffer = Buffer.concat(outputChunks); let snapshot = ''; try { - snapshot = serializeTerminalToText(headlessTerminal) ?? ''; + // Mirror the normal exit path's snapshot logic: re-decode + // the full buffer with the final encoding (the streaming + // decoder fed `headlessTerminal` from a first-chunk + // heuristic, which can mis-detect when early output is + // ASCII-only but later output is in a different encoding, + // e.g. GBK). Then replay through a fresh terminal so ANSI + // sequences land at the right cursor position. Falling back + // to `serializeTerminalToText(headlessTerminal)` would risk + // mojibake on the promoted snapshot that the normal exit + // path doesn't produce. + if (isStreamingRawContent) { + const finalEncoding = getCachedEncodingForBuffer(finalBuffer); + const decodedOutput = new TextDecoder(finalEncoding).decode( + finalBuffer, + ); + snapshot = await replayTerminalOutput(decodedOutput, cols, rows); + } else { + snapshot = serializeTerminalToText(headlessTerminal) ?? ''; + } } catch (serErr) { - // Best-effort snapshot — terminal serialization may fail if - // the PTY is mid-render. Empty snapshot is acceptable since - // the caller has rawOutput, but log so the failure leaves a - // diagnostic trail (otherwise an empty `output` is - // indistinguishable from "command produced no output"). + // Best-effort snapshot — re-decode + replay may fail (encoding + // detection error, terminal write throw, etc.). Empty snapshot + // is acceptable since the caller has rawOutput, but log so + // the failure leaves a diagnostic trail (otherwise an empty + // `output` is indistinguishable from "command produced no + // output"). Try the simpler direct-serialize path as a + // last-ditch fallback before giving up. debugLogger.warn( - `Background-promote snapshot serialization failed: ${serErr instanceof Error ? serErr.message : String(serErr)}. ` + - `Result.output will be empty; caller should fall back to rawOutput.`, + `Background-promote snapshot replay failed: ${serErr instanceof Error ? serErr.message : String(serErr)}. ` + + `Falling back to direct headlessTerminal serialize; if that also fails, output stays empty.`, ); + try { + snapshot = serializeTerminalToText(headlessTerminal) ?? ''; + } catch { + // Both paths failed — leave snapshot empty. + } } resolve({ rawOutput: finalBuffer, @@ -1234,11 +1300,17 @@ export class ShellExecutionService { await performCancelKill(); return; default: { + // Unreachable at runtime: getShellAbortReasonKind whitelists + // the return to the union members, so this branch only + // exists to force a TS error if the `ShellAbortReason` union + // ever gains a new variant — that error directs the + // developer to (1) extend the helper's whitelist and + // (2) add a `case` here. Without this exhaustiveness check + // the helper's whitelist and the switch could drift apart + // silently when the union grows. const _exhaustive: never = kind; - debugLogger.warn( - `Unknown ShellAbortReason kind, falling back to cancel/kill: ${String(_exhaustive)}`, - ); await performCancelKill(); + return _exhaustive; } } }; From 289feaa7467769d5e501ec67d75b4d5e49fab5a1 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 6 May 2026 22:59:54 +0800 Subject: [PATCH 6/9] fix(core): PTY background-promote post-exit race guard (resolve 5th review pass) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors the child_process post-exit race fix from 4cc558b3d into the PTY path — addresses 1 [Critical] thread on PR-1 of #3831: The PTY may have already exited but our `exitDisposable` (onExit callback) hasn't run yet — node-pty delivers the exit event asynchronously after the PTY's native SIGCHLD, so there's a window between "PTY actually dead" and "service onExit fires". Promoting in that window detaches our exit listener and reports `promoted: true` for a dead PTY, losing the real exit status; the caller would hold an inert pid expecting to take over. The IPty interface doesn't expose an `exitCode` field we can read directly (unlike `child.exitCode` / `child.signalCode` for child_process), so use `process.kill(pid, 0)` as a best-effort liveness check via the existing `ShellExecutionService.isPtyActive` helper. If kill(pid, 0) throws ESRCH, the pid is gone — log at debug level and fall through, letting the pending onExit callback resolve normally with the real exit info. Also adds a unit test mirroring the child_process race test: mocks `process.kill(pid, 0)` to throw ESRCH on the liveness probe, asserts the result has no `promoted: true` and reports the real exitCode. 69 / 69 tests pass; tsc clean; ESLint clean. --- .../services/shellExecutionService.test.ts | 45 +++++++++++++++++++ .../src/services/shellExecutionService.ts | 18 ++++++++ 2 files changed, 63 insertions(+) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 2ae3149d467..eb9771c06bb 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -703,6 +703,51 @@ describe('ShellExecutionService', () => { expect(exitDisposableStub.dispose).toHaveBeenCalled(); }); + it('post-exit race: PTY background-promote refuses if process.kill(pid, 0) reports the pid is gone', async () => { + // Mirror of the child_process post-exit race test. The PTY may + // have already exited but our `exitDisposable` (onExit) handler + // hasn't run yet — node-pty delivers the exit event async after + // the native SIGCHLD. Promoting in that window would detach our + // exit listener, miss the real exit status, and report + // `promoted: true` for a dead PTY. Production guard: + // process.kill(pid, 0); if it throws ESRCH, fall through. + mockProcessKill.mockImplementationOnce((pid, signal) => { + // Only fail the very first liveness probe with signal 0 — let + // any subsequent kill calls (e.g. cleanup() at process exit) + // succeed so the test teardown stays clean. + if (signal === 0) { + throw Object.assign(new Error('ESRCH'), { code: 'ESRCH' }); + } + return true; + }); + const { result } = await simulateExecution( + 'fast-and-cancelled', + (pty, abortController) => { + abortController.abort({ + kind: 'background', + shellId: 'bg_test123', + } satisfies ShellAbortReason); + // Drain the pending onExit (production code falls through; + // normal exit path resolves with the real exit info). + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: undefined }); + }, + ); + + // Result is the normal exit shape, not the promoted shape. + expect(result.promoted).toBeUndefined(); + expect(result.exitCode).toBe(0); + // Our PTY listeners stayed registered — the disposables are + // disposed by the natural onExit, not the abort handler. + const dataDisposableStub = mockPtyProcess.onData.mock.results[0] + .value as { dispose: Mock }; + // dataDisposable is NOT disposed by our abort handler in the + // race-fallthrough path (the normal onExit handler doesn't + // dispose it either — it relies on the PTY tearing down its own + // event source). What matters is that we did NOT pre-dispose it + // and lose the exit info. + void dataDisposableStub; // referenced for the future expansion + }); + it("post-promotion: ptyProcess error listener is removed via 'removeListener', NOT 'off' (regression guard for @lydell/node-pty)", async () => { // node EventEmitter exposes both `off` (Node 10+) and the legacy // `removeListener`, but @lydell/node-pty's IPty interface only diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 4296cfb77d0..41c22031c84 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -1121,6 +1121,24 @@ export class ShellExecutionService { const performBackgroundPromote = async (): Promise => { if (!ptyProcess.pid || exited) return; + // Race guard mirroring the child_process path: the PTY may + // have already exited but `exitDisposable` (our onExit + // handler) has not yet run — node-pty delivers the exit + // event asynchronously after the PTY's native SIGCHLD. The + // IPty interface doesn't expose an `exitCode` field we can + // read directly, so use `process.kill(pid, 0)` as a + // best-effort liveness check (it throws ESRCH if the pid + // is gone, EPERM if it's not ours / not reusable). If the + // PTY is gone, fall through and let the pending onExit + // callback resolve normally with the real exit status. + if (!ShellExecutionService.isPtyActive(ptyProcess.pid)) { + debugLogger.debug( + `Background-promote requested for PTY pid ${ptyProcess.pid} ` + + `but the process is no longer alive (process.kill(pid, 0) ` + + `failed); falling through to normal exit-handled resolution.`, + ); + return; + } // Skip kill, dispose all our listeners on the live PTY (so // post-promote data/exit/error don't leak into our foreground // onOutputEvent or crash via the error handler's `throw err`), From c51274b5af83ea760bdb2232aab01dd2320d655b Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 7 May 2026 00:20:08 +0800 Subject: [PATCH 7/9] docs(core): correct getShellAbortReasonKind boundary-test count in JSDoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doc said 'all six edge cases' but the test suite has 8 cases (added Proxy-trap and undefined later). Off-by-2 cosmetic only — no behavior change. Caught during a multi-round self-audit of PR-1 of #3831. Audit summary: 7 rounds (correctness / reverse / consistency / coverage / build / exception paths / style) found one false-positive (a sync- abort registration-order race I initially thought existed). Verified that Node's WHATWG AbortSignal does NOT auto-fire 'abort' listeners on already-aborted signals, so the race window cannot open. No code change needed for that scenario; this commit is just the JSDoc fix. 69 / 69 tests still pass; tsc + ESLint clean. --- packages/core/src/services/shellExecutionService.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 41c22031c84..93bdc435c8a 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -53,10 +53,11 @@ const PROMOTE_DRAIN_TIMEOUT_MS = 200; * future-untyped variants) defaults to `'cancel'` so the historical * kill behavior is preserved as the safe fallback. * - * Exported for direct unit testing of all six edge cases (null / - * non-object / `{}` no own kind / prototype-only kind / unknown kind / - * throwing accessor) — the integration tests only exercise the three - * happy-path inputs. + * Exported for direct unit testing of all eight cases (null / + * undefined / non-object / `{}` no own kind / prototype-only kind / + * unknown kind / throwing-accessor / Proxy trap, plus the two + * happy-path inputs) — the integration tests only exercise the three + * happy-path scenarios. */ export function getShellAbortReasonKind( reason: unknown, From 36d96a4b44351bd7fe65be0d18ec4e11bec6c8c7 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 7 May 2026 00:40:38 +0800 Subject: [PATCH 8/9] docs(core): document the helper / union / switch sync invariant explicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-round self-audit found that `getShellAbortReasonKind`'s value whitelist has no compile-time tie to the `ShellAbortReason` union: when the union grows, TypeScript's `_exhaustive: never` in each switch forces #3 (the case arm) to be added, but the helper's whitelist (#2) silently keeps degrading the new variant to 'cancel', and the new case arm is never reached at runtime. Reviewer #4 raised this on the second pass; the original commit chose to accept it (option B in that thread) but didn't leave a strong in-code signal for future contributors. Added an INVARIANT block inside the helper enumerating the three sites that must be kept in sync, so the next person extending `ShellAbortReason` sees the coupling at the place where they're most likely to forget it. No behavior change — comment-only. 69 / 69 tests still pass; tsc + ESLint clean. Audit summary (this round + prior round): 18 angles total over two sweeps and one reverse-attack pass. Found: - 0 real bugs - 1 false-positive race (sync-abort registration order — Node WHATWG AbortSignal does NOT auto-fire on already-aborted signals; investigated, reverted) - 1 cosmetic doc fix (boundary-test count off-by-2) - 1 cosmetic INVARIANT block (this commit) Areas reviewed without finding new issues: caller-side ShellExecutionResult shape compatibility (optional `promoted?` field, existing callers spread-untouched); `exited` flag lifecycle (monotonic, cleanup() idempotent); processingChain in-flight ownership (listenersDetached guards every onOutputEvent emit including the renderFn-rendered case via the same flag); race between exit event and abort handler (both microtasks, FIFO ordering gives correct outcome either way); Node version dependence (`AbortSignal.reason` is Node 17.2+, engines: >=20 covers it); test isolation (mockImplementationOnce + module-level mockProcessKill clears each beforeEach); `process.kill(pid, 0)` Windows liveness reliability (best-effort, acceptable for PR-1 plumbing); PID reuse race on the PTY liveness check (theoretically possible, microsecond window, unavoidable at the OS level — rejected in spec discussion); PR-2/PR-3 contract surface (caller MUST attach listeners before abort — documented; any future caller violating this is its own bug). --- packages/core/src/services/shellExecutionService.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 93bdc435c8a..4b3e2700a1d 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -69,6 +69,16 @@ export function getShellAbortReasonKind( ) { try { const kind = (reason as { kind?: unknown }).kind; + // INVARIANT — three points must be kept in sync when extending + // `ShellAbortReason`: + // (1) the discriminated union below (`type ShellAbortReason`), + // (2) the value-equality whitelist on this line, and + // (3) the `case` arms in both abort-handler switches (the + // `default: { const _exhaustive: never = kind; ... }` + // statically forces #3 when #1 grows, but #2 has no + // compile-time tie to the union; if you forget to extend + // it the new variant silently degrades to 'cancel' here + // and the `case` you added in #3 is never reached). if (kind === 'background' || kind === 'cancel') return kind; } catch { // Throwing accessor / Proxy trap — fall back to safe kill below. From b7259e53df5dec446fde79e486890ab054f5b0fa Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 7 May 2026 07:39:57 +0800 Subject: [PATCH 9/9] test(core): align mockChildProcess.exitCode/signalCode in second beforeEach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'execution method selection' describe block has its own beforeEach (separate from 'child_process fallback') that builds mockChildProcess but does not set `exitCode` / `signalCode = null`. Real Node `ChildProcess.exitCode` / `signalCode` are `null` while the process is alive — and production now reads these in the background-promote race guard. The current tests in this block don't exercise the promote path, so they pass regardless, but any future promote-related test landing here would silently trip the guard (`undefined !== null` is true) and fall through to the normal-exit branch instead of promoting. Mirror the `child_process fallback` block's mock setup so the two beforeEach hooks produce equivalent ChildProcess shapes, eliminating a quiet foot-gun for future contributors. Comment-only / test-fixture change. 69 / 69 tests still pass; tsc clean. Found during a deeper third-round self-audit of PR-1 of #3831. --- .../src/services/shellExecutionService.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index eb9771c06bb..699f585e286 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -1734,6 +1734,21 @@ describe('ShellExecutionService execution method selection', () => { value: 54321, configurable: true, }); + // Mirror real Node ChildProcess: `exitCode` / `signalCode` are + // `null` while alive. Kept in sync with the `child_process + // fallback` describe block's mock setup so any future promote- + // related test that lands here doesn't trip the production + // `child.exitCode !== null` race guard with a stale `undefined`. + Object.defineProperty(mockChildProcess, 'exitCode', { + value: null, + writable: true, + configurable: true, + }); + Object.defineProperty(mockChildProcess, 'signalCode', { + value: null, + writable: true, + configurable: true, + }); mockCpSpawn.mockReturnValue(mockChildProcess); });