diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 5cc203929e7..699f585e286 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -17,8 +17,14 @@ 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 { ShellExecutionService } from './shellExecutionService.js'; +import type { + ShellAbortReason, + ShellOutputEvent, +} from './shellExecutionService.js'; +import { + getShellAbortReasonKind, + ShellExecutionService, +} from './shellExecutionService.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; const { Terminal } = pkg; @@ -225,8 +231,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(); @@ -608,6 +618,192 @@ 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', + ); + }); + + 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-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 + // 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 + // 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', () => { @@ -943,6 +1139,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); }); @@ -1139,6 +1351,168 @@ 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('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-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 + // 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(); @@ -1335,8 +1709,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(); @@ -1356,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); }); @@ -1424,3 +1817,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 8d7e0bb3bab..4b3e2700a1d 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -21,9 +21,71 @@ 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; + +/** + * 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. + * - 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 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, +): ShellAbortReason['kind'] { + if ( + reason !== null && + typeof reason === 'object' && + Object.prototype.hasOwnProperty.call(reason, 'kind') + ) { + 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. + } + } + return 'cancel'; +} /** * On Windows with PowerShell, prefix the command with a statement that forces @@ -37,6 +99,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 +130,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. */ @@ -510,27 +597,154 @@ 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); + }; - const abortHandler = async () => { - 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'); + 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 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() + // 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. + // + // 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 { + 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', + }); + }; + + 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 kind = getShellAbortReasonKind(abortSignal.reason); + switch (kind) { + case 'background': + performBackgroundPromote(); + return; + case 'cancel': + 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; + await performCancelKill(); + return _exhaustive; } } }; @@ -541,12 +755,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; @@ -661,11 +870,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; } @@ -786,30 +1002,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); }); @@ -818,7 +1050,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; @@ -826,9 +1058,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); @@ -897,25 +1130,216 @@ export class ShellExecutionService { }, ); - const abortHandler = async () => { - if (ptyProcess.pid && !exited) { - if (os.platform() === 'win32') { - ptyProcess.kill(); + 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`), + // 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 { + // @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.removeListener('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).`, + ); + } + + const finalBuffer = Buffer.concat(outputChunks); + let snapshot = ''; + try { + // 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 { - 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(); - } + snapshot = serializeTerminalToText(headlessTerminal) ?? ''; + } + } catch (serErr) { + // 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 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, + 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 performCancelKill = async (): Promise => { + if (!ptyProcess.pid || exited) return; + 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 kind = getShellAbortReasonKind(abortSignal.reason); + switch (kind) { + case 'background': + await performBackgroundPromote(); + return; + case 'cancel': + 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; + await performCancelKill(); + return _exhaustive; } } };