From 6cbab376d7ab8f1ffc554f545c9ca955ae8d6610 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 5 May 2026 12:45:27 +0800 Subject: [PATCH] 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 5cc203929..827a20184 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 8d7e0bb3b..1d39bc196 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();