diff --git a/.github/workflows/windows-smoke.yml b/.github/workflows/windows-smoke.yml index aed6d0a668..bc2c5d9f86 100644 --- a/.github/workflows/windows-smoke.yml +++ b/.github/workflows/windows-smoke.yml @@ -3,7 +3,8 @@ # See sync-manifest.yaml target_owned_files. # # Runs Windows-specific tests that cannot be validated on Linux/macOS CI. -# Covers: CLI .cmd shim resolution, ProcessLivenessProbe platform guard. +# Covers: CLI .cmd shim resolution, ProcessLivenessProbe platform guard, +# and ManagedRunner parent/descendant process-tree cleanup. name: Windows Smoke @@ -48,5 +49,10 @@ jobs: node --test packages/api/test/cli-spawn-win.test.js node --test packages/api/test/process-liveness-probe.test.js node --test packages/api/test/pick-directory.test.js + - name: ManagedRunner Windows process-tree cleanup + run: >- + node --test + packages/api/test/managed-runner-process-tree.test.js + packages/api/test/managed-runner-windows.test.js - name: Desktop config behavioral tests (PowerShell) run: node --test desktop/generate-desktop-config-behavior.test.js diff --git a/packages/api/src/infrastructure/managed-runner-process-tree.ts b/packages/api/src/infrastructure/managed-runner-process-tree.ts new file mode 100644 index 0000000000..a53aecd7bb --- /dev/null +++ b/packages/api/src/infrastructure/managed-runner-process-tree.ts @@ -0,0 +1,172 @@ +import { spawn } from 'node:child_process'; + +/** Keep the OS helper bounded below ManagedRunner's 5s SIGKILL grace period. */ +export const TASKKILL_TIMEOUT_MS = 2_000; + +export interface TaskkillChildProcess { + once(event: 'error', listener: (error: Error) => void): this; + once(event: 'close', listener: (code: number | null, signal: NodeJS.Signals | null) => void): this; + kill(): boolean; +} + +export interface TaskkillSpawnOptions { + shell: false; + stdio: 'ignore'; + windowsHide: true; +} + +export interface BoundedTimerHandle { + unref?(): void; +} + +export interface ProcessTreeKillEscalation { + cancel(): void; +} + +export interface ProcessTreeKillEscalationDeps { + scheduleTimeout(callback: () => void, delayMs: number): BoundedTimerHandle; + cancelTimeout(handle: BoundedTimerHandle): void; +} + +export interface WindowsProcessTreeTerminationDeps { + spawnTaskkill(command: string, args: string[], options: TaskkillSpawnOptions): TaskkillChildProcess; + scheduleTimeout(callback: () => void, delayMs: number): BoundedTimerHandle; + cancelTimeout(handle: BoundedTimerHandle): void; +} + +export type WindowsProcessTreeTerminationResult = + | { + status: 'completed'; + exitCode: 0; + signal: null; + } + | { + status: 'failed'; + exitCode: number | null; + signal: NodeJS.Signals | null; + error?: Error; + } + | { + status: 'timed_out'; + exitCode: null; + signal: null; + error?: Error; + }; + +const DEFAULT_DEPS: WindowsProcessTreeTerminationDeps = { + spawnTaskkill: (command, args, options) => spawn(command, args, options), + scheduleTimeout: (callback, delayMs) => setTimeout(callback, delayMs), + cancelTimeout: (handle) => clearTimeout(handle as ReturnType), +}; + +const DEFAULT_ESCALATION_DEPS: ProcessTreeKillEscalationDeps = { + scheduleTimeout: DEFAULT_DEPS.scheduleTimeout, + cancelTimeout: DEFAULT_DEPS.cancelTimeout, +}; + +export function buildTaskkillArgs(pid: number, signal: NodeJS.Signals): string[] { + const args = ['/PID', String(pid), '/T']; + if (signal === 'SIGKILL') args.push('/F'); + return args; +} + +/** + * Arm one force-kill fallback for a graceful process-tree termination attempt. + * + * A completed Windows `taskkill /T` authoritatively terminated that PID's tree, + * so retaining a later `/F /T` would risk targeting a reused PID. Failed or + * timed-out helpers retain the fallback. Unix callers pass `null` and preserve + * their existing SIGTERM → grace → SIGKILL behavior. + */ +export function armProcessTreeKillEscalation( + gracefulTermination: Promise | null, + forceKill: () => void, + graceMs: number, + deps: ProcessTreeKillEscalationDeps = DEFAULT_ESCALATION_DEPS, +): ProcessTreeKillEscalation { + let active = true; + let timer: BoundedTimerHandle; + + const cancel = (): void => { + if (!active) return; + active = false; + deps.cancelTimeout(timer); + }; + + timer = deps.scheduleTimeout(() => { + if (!active) return; + active = false; + forceKill(); + }, graceMs); + + if (gracefulTermination) { + void gracefulTermination.then((result) => { + if (result.status === 'completed') cancel(); + }); + } + + return { cancel }; +} + +/** + * Terminate one Windows process tree without blocking the API event loop. + * + * This promise never rejects: spawn errors, non-zero exits, and a bounded helper + * timeout are returned as observable outcomes for ManagedRunner to log. + */ +export function terminateWindowsProcessTree( + pid: number, + signal: NodeJS.Signals, + deps: WindowsProcessTreeTerminationDeps = DEFAULT_DEPS, +): Promise { + const args = buildTaskkillArgs(pid, signal); + let child: TaskkillChildProcess; + try { + child = deps.spawnTaskkill('taskkill', args, { + shell: false, + stdio: 'ignore', + windowsHide: true, + }); + } catch (error) { + return Promise.resolve({ + status: 'failed', + exitCode: null, + signal: null, + error: error instanceof Error ? error : new Error(String(error)), + }); + } + + return new Promise((resolve) => { + let settled = false; + let timer: BoundedTimerHandle | null = null; + + const finish = (result: WindowsProcessTreeTerminationResult): void => { + if (settled) return; + settled = true; + if (timer) deps.cancelTimeout(timer); + resolve(result); + }; + + child.once('error', (error) => { + finish({ status: 'failed', exitCode: null, signal: null, error }); + }); + child.once('close', (exitCode, closeSignal) => { + if (exitCode === 0) { + finish({ status: 'completed', exitCode: 0, signal: null }); + return; + } + finish({ status: 'failed', exitCode, signal: closeSignal }); + }); + + timer = deps.scheduleTimeout(() => { + let error: Error | undefined; + try { + child.kill(); + } catch (cause) { + error = cause instanceof Error ? cause : new Error(String(cause)); + } + finish({ status: 'timed_out', exitCode: null, signal: null, ...(error ? { error } : {}) }); + }, TASKKILL_TIMEOUT_MS); + timer.unref?.(); + }); +} diff --git a/packages/api/src/infrastructure/managed-runner.ts b/packages/api/src/infrastructure/managed-runner.ts index 05e8003d15..a6c1bcca95 100644 --- a/packages/api/src/infrastructure/managed-runner.ts +++ b/packages/api/src/infrastructure/managed-runner.ts @@ -4,12 +4,8 @@ * Spawns a shell command, captures combined stdout+stderr output to a temp log file, * and returns a structured result when the command exits, times out, or is cancelled. * - * Design decisions (per f167-phase-p-wakewhen.md plan): - * - Shell mode: `spawn(command, { shell: true })` — commands are shell expressions - * - Output: combined stdout+stderr piped to temp file; last 50 lines returned - * - Timeout: SIGTERM → 5s grace → SIGKILL - * - Single-use: each ManagedRunner instance handles one command lifecycle - * - Log cleanup: temp file deleted after result is captured + * Shell expressions run once per instance; output is captured to a temporary log. + * Termination follows SIGTERM → 5s grace → SIGKILL and returns the last 50 lines. * * State machine: * IDLE → RUNNING → {COMPLETED | TIMED_OUT | CANCELLED} → (log cleaned up) @@ -21,6 +17,12 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { createModuleLogger } from './logger.js'; import { buildManagedRunnerEnvironment } from './managed-runner-environment.js'; +import { + armProcessTreeKillEscalation, + type ProcessTreeKillEscalation, + terminateWindowsProcessTree, + type WindowsProcessTreeTerminationResult, +} from './managed-runner-process-tree.js'; const log = createModuleLogger('managed-runner'); @@ -60,7 +62,7 @@ export class ManagedRunner { private _logPath: string | null = null; private _child: ChildProcess | null = null; private _timeoutTimer: ReturnType | null = null; - private _killTimer: ReturnType | null = null; + private _killEscalation: ProcessTreeKillEscalation | null = null; /** P2-3 fix: rolling tail buffer keeps last TAIL_LINES regardless of log file truncation */ private _rollingTail: string[] = []; @@ -101,8 +103,7 @@ export class ManagedRunner { return new Promise((resolve) => { const logStream = createWriteStream(this._logPath!, { flags: 'w' }); - // P1-2 fix: detached=true creates a new process group so we can kill - // the entire tree (shell + children) via process.kill(-pid, signal). + // A detached child leads the process group used for tree termination. const child = spawn(command, { shell: true, cwd, @@ -176,22 +177,11 @@ export class ManagedRunner { if (this._state !== 'running') return; log.info({ pid: this._pid, command, timeoutMs }, 'ManagedRunner: timeout reached, sending SIGTERM'); this._state = 'timed_out'; - // P1-2 fix: kill the process GROUP (shell + children), not just the shell PID. - this._killProcessGroup('SIGTERM'); - - // P1-4 fix (cloud R2): always attempt SIGKILL after grace period in timed_out state. - // With detached=true, the shell may exit first (firing the 'exit' event) while child - // processes in the group survive SIGTERM. _killProcessGroup(-pid, SIGKILL) is idempotent - // (ESRCH caught if group is already dead). - this._killTimer = setTimeout(() => { - log.warn({ pid: this._pid }, 'ManagedRunner: SIGKILL after grace period'); - this._killProcessGroup('SIGKILL'); - }, KILL_GRACE_MS); + const gracefulTermination = this._killProcessGroup('SIGTERM'); + this._armKillEscalation(gracefulTermination, 'ManagedRunner: SIGKILL after grace period'); }, timeoutMs); - // P2-9 fix (cloud R4): use 'close' instead of 'exit'. Node can emit 'exit' - // before child stdout/stderr streams have fully drained — using 'close' ensures - // all piped data has been consumed before we read the rolling tail. + // `close` waits for stdout/stderr to drain before the tail is read. child.on('close', (code, signal) => { // P2-3 fix: flush any remaining partial line to rolling tail if (_rollingPartialLine) { @@ -201,20 +191,13 @@ export class ManagedRunner { } _rollingPartialLine = ''; } - // P1-4 fix (cloud R2): selective timer clearing. - // Always clear timeout timer (process already exited, no need). - // Keep _killTimer alive in timed_out/cancelled state — with detached=true, - // the shell may exit while child processes in the group survive SIGTERM. - // The SIGKILL escalation must still fire to clean up the group. + // Keep escalation alive when descendants may outlive the shell. if (this._timeoutTimer) { clearTimeout(this._timeoutTimer); this._timeoutTimer = null; } if (this._state !== 'timed_out' && this._state !== 'cancelled') { - if (this._killTimer) { - clearTimeout(this._killTimer); - this._killTimer = null; - } + this._clearKillEscalation(); } const durationMs = Date.now() - startTime; @@ -270,39 +253,60 @@ export class ManagedRunner { }); } - /** - * Cancel the running process. SIGTERM → 5s grace → SIGKILL. - * No-op if not running. - */ + /** Cancel the running process. SIGTERM → 5s grace → SIGKILL. */ cancel(): void { if (this._state !== 'running' || !this._child) return; log.info({ pid: this._pid }, 'ManagedRunner: cancel requested, sending SIGTERM'); this._state = 'cancelled'; this._clearTimers(); - // P1-2 fix: kill the process GROUP, not just the shell PID - this._killProcessGroup('SIGTERM'); - - // P1-4 fix (cloud R2): always attempt SIGKILL after cancel grace period. - // With detached=true, shell may exit while children in the group survive. - this._killTimer = setTimeout(() => { - log.warn({ pid: this._pid }, 'ManagedRunner: SIGKILL after cancel grace period'); - this._killProcessGroup('SIGKILL'); - }, KILL_GRACE_MS); + const gracefulTermination = this._killProcessGroup('SIGTERM'); + this._armKillEscalation(gracefulTermination, 'ManagedRunner: SIGKILL after cancel grace period'); } - /** - * P1-2 fix: kill the entire process group (shell + child processes). - * With detached=true, child.pid IS the process group leader. - * process.kill(-pid, signal) sends the signal to all processes in the group. - */ - private _killProcessGroup(signal: NodeJS.Signals): void { - if (!this._pid) return; + /** Terminate the detached process group (Unix) or process tree (Windows). */ + private _killProcessGroup(signal: NodeJS.Signals): Promise | null { + if (!this._pid) return null; + + if (process.platform === 'win32') { + const pid = this._pid; + const termination = terminateWindowsProcessTree(pid, signal); + void termination.then((result) => { + if (result.status === 'completed') { + log.debug({ pid, signal }, 'ManagedRunner: taskkill completed'); + return; + } + log.warn({ pid, signal, result }, `ManagedRunner: taskkill ${result.status}`); + }); + return termination; + } + try { process.kill(-this._pid, signal); - } catch { + } catch (err) { // Process group may already be gone — that's fine + log.debug({ pid: this._pid, signal, err }, 'ManagedRunner: kill process group failed (may already be gone)'); } + return null; + } + + private _armKillEscalation( + gracefulTermination: Promise | null, + warningMessage: string, + ): void { + this._killEscalation = armProcessTreeKillEscalation( + gracefulTermination, + () => { + log.warn({ pid: this._pid }, warningMessage); + this._killProcessGroup('SIGKILL'); + }, + KILL_GRACE_MS, + ); + } + + private _clearKillEscalation(): void { + this._killEscalation?.cancel(); + this._killEscalation = null; } private _clearTimers(): void { @@ -310,10 +314,7 @@ export class ManagedRunner { clearTimeout(this._timeoutTimer); this._timeoutTimer = null; } - if (this._killTimer) { - clearTimeout(this._killTimer); - this._killTimer = null; - } + this._clearKillEscalation(); } private _readTailOutput(): string { diff --git a/packages/api/test/managed-runner-process-tree.test.js b/packages/api/test/managed-runner-process-tree.test.js new file mode 100644 index 0000000000..acf8d1909b --- /dev/null +++ b/packages/api/test/managed-runner-process-tree.test.js @@ -0,0 +1,198 @@ +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; +import { test } from 'node:test'; + +const { TASKKILL_TIMEOUT_MS, armProcessTreeKillEscalation, buildTaskkillArgs, terminateWindowsProcessTree } = + await import('../dist/infrastructure/managed-runner-process-tree.js'); + +function createHarness() { + const child = new EventEmitter(); + let killCalls = 0; + child.kill = () => { + killCalls += 1; + return true; + }; + + const spawnCalls = []; + let timeoutCallback = null; + let timeoutDelay = null; + let clearCalls = 0; + let unrefCalls = 0; + + return { + child, + deps: { + spawnTaskkill(command, args, options) { + spawnCalls.push({ command, args, options }); + return child; + }, + scheduleTimeout(callback, delayMs) { + timeoutCallback = callback; + timeoutDelay = delayMs; + return { + unref() { + unrefCalls += 1; + }, + }; + }, + cancelTimeout() { + clearCalls += 1; + }, + }, + fireTimeout() { + assert.ok(timeoutCallback, 'timeout callback should be registered'); + timeoutCallback(); + }, + observed() { + return { spawnCalls, timeoutDelay, clearCalls, unrefCalls, killCalls }; + }, + }; +} + +function createEscalationHarness() { + const timerHandle = {}; + let timeoutCallback = null; + let timeoutDelay = null; + let timerActive = false; + let cancelCalls = 0; + let forceKillCalls = 0; + + return { + deps: { + scheduleTimeout(callback, delayMs) { + timeoutCallback = callback; + timeoutDelay = delayMs; + timerActive = true; + return timerHandle; + }, + cancelTimeout(handle) { + assert.strictEqual(handle, timerHandle); + cancelCalls += 1; + timerActive = false; + }, + }, + fireTimeout() { + assert.ok(timeoutCallback, 'escalation callback should be registered'); + if (timerActive) timeoutCallback(); + }, + forceKill() { + forceKillCalls += 1; + }, + observed() { + return { timeoutDelay, cancelCalls, forceKillCalls }; + }, + }; +} + +test('taskkill args preserve graceful and forced process-tree semantics', () => { + assert.deepStrictEqual(buildTaskkillArgs(4321, 'SIGTERM'), ['/PID', '4321', '/T']); + assert.deepStrictEqual(buildTaskkillArgs(4321, 'SIGKILL'), ['/PID', '4321', '/T', '/F']); +}); + +test('Windows tree termination is asynchronous and reports successful completion', async () => { + const harness = createHarness(); + + const resultPromise = terminateWindowsProcessTree(4321, 'SIGTERM', harness.deps); + let settled = false; + void resultPromise.then(() => { + settled = true; + }); + await Promise.resolve(); + + assert.strictEqual(settled, false, 'termination must not synchronously block for taskkill'); + assert.deepStrictEqual(harness.observed().spawnCalls, [ + { + command: 'taskkill', + args: ['/PID', '4321', '/T'], + options: { shell: false, stdio: 'ignore', windowsHide: true }, + }, + ]); + assert.strictEqual(harness.observed().timeoutDelay, TASKKILL_TIMEOUT_MS); + assert.strictEqual(harness.observed().unrefCalls, 1); + + harness.child.emit('close', 0, null); + assert.deepStrictEqual(await resultPromise, { + status: 'completed', + exitCode: 0, + signal: null, + }); + assert.strictEqual(harness.observed().clearCalls, 1); +}); + +test('Windows tree termination reports taskkill spawn failure', async () => { + const harness = createHarness(); + const resultPromise = terminateWindowsProcessTree(4321, 'SIGTERM', harness.deps); + const error = new Error('taskkill unavailable'); + + harness.child.emit('error', error); + + assert.deepStrictEqual(await resultPromise, { + status: 'failed', + exitCode: null, + signal: null, + error, + }); + assert.strictEqual(harness.observed().clearCalls, 1); +}); + +test('Windows tree termination reports non-zero taskkill exit', async () => { + const harness = createHarness(); + const resultPromise = terminateWindowsProcessTree(4321, 'SIGTERM', harness.deps); + + harness.child.emit('close', 128, null); + + assert.deepStrictEqual(await resultPromise, { + status: 'failed', + exitCode: 128, + signal: null, + }); +}); + +test('Windows tree termination times out deterministically and stops taskkill', async () => { + const harness = createHarness(); + const resultPromise = terminateWindowsProcessTree(4321, 'SIGKILL', harness.deps); + + harness.fireTimeout(); + + assert.deepStrictEqual(await resultPromise, { + status: 'timed_out', + exitCode: null, + signal: null, + }); + assert.strictEqual(harness.observed().killCalls, 1); +}); + +test('completed graceful tree termination cancels its pending force escalation', async () => { + const harness = createEscalationHarness(); + armProcessTreeKillEscalation( + Promise.resolve({ status: 'completed', exitCode: 0, signal: null }), + () => harness.forceKill(), + 5_000, + harness.deps, + ); + + await Promise.resolve(); + + assert.deepStrictEqual(harness.observed(), { + timeoutDelay: 5_000, + cancelCalls: 1, + forceKillCalls: 0, + }); + harness.fireTimeout(); + assert.strictEqual(harness.observed().forceKillCalls, 0); +}); + +test('failed or timed-out graceful tree termination preserves force escalation', async () => { + const outcomes = [{ status: 'failed' }, { status: 'timed_out' }]; + + for (const outcome of outcomes) { + const harness = createEscalationHarness(); + armProcessTreeKillEscalation(Promise.resolve(outcome), () => harness.forceKill(), 5_000, harness.deps); + + await Promise.resolve(); + + assert.strictEqual(harness.observed().cancelCalls, 0, `${outcome.status} must retain escalation`); + harness.fireTimeout(); + assert.strictEqual(harness.observed().forceKillCalls, 1, `${outcome.status} must force-kill after grace`); + } +}); diff --git a/packages/api/test/managed-runner-windows.test.js b/packages/api/test/managed-runner-windows.test.js new file mode 100644 index 0000000000..e69b6cc2a9 --- /dev/null +++ b/packages/api/test/managed-runner-windows.test.js @@ -0,0 +1,117 @@ +import assert from 'node:assert/strict'; +import { execFile } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { test } from 'node:test'; +import { promisify } from 'node:util'; + +const { ManagedRunner } = await import('../dist/infrastructure/managed-runner.js'); +const execFileAsync = promisify(execFile); +const windowsOnly = { skip: process.platform !== 'win32', timeout: 30_000 }; + +function processExists(pid) { + try { + process.kill(pid, 0); + return true; + } catch (error) { + if (error?.code === 'ESRCH') return false; + if (error?.code === 'EPERM') return true; + throw error; + } +} + +async function waitFor(getValue, description, timeoutMs = 10_000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const value = getValue(); + if (value) return value; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + throw new Error(`timed out waiting for ${description}`); +} + +async function forceCleanup(pid) { + if (!pid || !processExists(pid)) return; + await execFileAsync('taskkill', ['/PID', String(pid), '/T', '/F'], { + timeout: 5_000, + windowsHide: true, + }).catch(() => undefined); +} + +function createProcessTreeFixture() { + const fixtureDir = mkdtempSync(join(tmpdir(), 'managed-runner-windows-')); + const pidPath = join(fixtureDir, 'descendant.pid'); + const scriptPath = join(fixtureDir, 'parent.mjs'); + writeFileSync( + scriptPath, + [ + "import { spawn } from 'node:child_process';", + "import { writeFileSync } from 'node:fs';", + "const descendant = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { stdio: 'ignore' });", + 'writeFileSync(process.argv[2], String(descendant.pid));', + 'setInterval(() => {}, 1000);', + ].join('\n'), + ); + const command = `"${process.execPath}" "${scriptPath}" "${pidPath}"`; + return { fixtureDir, pidPath, command }; +} + +async function readDescendantPid(pidPath) { + return waitFor(() => { + if (!existsSync(pidPath)) return null; + const pid = Number.parseInt(readFileSync(pidPath, 'utf8'), 10); + return Number.isInteger(pid) && pid > 0 ? pid : null; + }, 'descendant pid file'); +} + +async function assertDescendantGone(pid) { + await waitFor(() => !processExists(pid), `descendant process ${pid} to exit`); + assert.strictEqual(processExists(pid), false, `descendant process ${pid} should be gone`); +} + +test('Windows cancel terminates the real parent and descendant process tree', windowsOnly, async () => { + const fixture = createProcessTreeFixture(); + const runner = new ManagedRunner(); + let descendantPid = null; + + try { + const resultPromise = runner.launch(fixture.command, { timeoutMs: 60_000 }); + descendantPid = await readDescendantPid(fixture.pidPath); + assert.strictEqual(processExists(descendantPid), true); + + runner.cancel(); + + const result = await resultPromise; + assert.strictEqual(result.exitCode, null); + assert.strictEqual(result.timedOut, false); + assert.strictEqual(runner.state, 'cancelled'); + await assertDescendantGone(descendantPid); + } finally { + runner.cancel(); + await forceCleanup(descendantPid); + rmSync(fixture.fixtureDir, { recursive: true, force: true }); + } +}); + +test('Windows timeout terminates the real parent and descendant process tree', windowsOnly, async () => { + const fixture = createProcessTreeFixture(); + const runner = new ManagedRunner(); + let descendantPid = null; + + try { + const resultPromise = runner.launch(fixture.command, { timeoutMs: 1_000 }); + descendantPid = await readDescendantPid(fixture.pidPath); + assert.strictEqual(processExists(descendantPid), true); + + const result = await resultPromise; + assert.strictEqual(result.exitCode, null); + assert.strictEqual(result.timedOut, true); + assert.strictEqual(runner.state, 'timed_out'); + await assertDescendantGone(descendantPid); + } finally { + runner.cancel(); + await forceCleanup(descendantPid); + rmSync(fixture.fixtureDir, { recursive: true, force: true }); + } +});