mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-20 01:01:53 +00:00
feat(core): add signal.reason convention for ShellExecutionService.execute()
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.
This commit is contained in:
Shaojin Wen
parent
0501c7165b
commit
6cbab376d7
2 changed files with 209 additions and 1 deletions
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue