From 94a600da1806388629f527d9a50fba5c4d835e3d Mon Sep 17 00:00:00 2001 From: whutzefengxie-ops Date: Tue, 4 Aug 2026 10:50:46 +0800 Subject: [PATCH 1/4] fix: add Windows platform support for process group killing in ManagedRunner Fixes #1284 ## Problem ManagedRunner._killProcessGroup uses Unix-style process.kill(-pid) syntax, which does not work on Windows. This causes child processes (sleep, cmd.exe) to survive after tests complete, leading to zombie processes. ## Root Cause - process.kill(-pid) is Unix syntax for killing a process group - On Windows, negative PIDs are invalid and ignored - Only the parent shell (cmd.exe) is killed, child processes survive - These zombie processes accumulate and can cause system instability ## Solution Add platform-specific process killing logic: - Windows: use 'taskkill /PID /T' to kill the entire process tree - /T flag terminates all child processes - /F flag added for SIGKILL equivalent (force termination) - Unix: keep existing process.kill(-pid) logic ## Testing - Verified on Windows 11: child processes are now killed correctly - TypeScript type check passes - Build succeeds ## Impact - Fixes zombie process leaks on Windows during test runs - No impact on Unix/macOS (existing logic preserved) - Related tests: F167 Phase P wakeWhen integration tests --- .../api/src/infrastructure/managed-runner.ts | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/api/src/infrastructure/managed-runner.ts b/packages/api/src/infrastructure/managed-runner.ts index 05e8003d15..4cf9e68ede 100644 --- a/packages/api/src/infrastructure/managed-runner.ts +++ b/packages/api/src/infrastructure/managed-runner.ts @@ -15,7 +15,7 @@ * IDLE → RUNNING → {COMPLETED | TIMED_OUT | CANCELLED} → (log cleaned up) */ -import { type ChildProcess, spawn } from 'node:child_process'; +import { type ChildProcess, execFileSync, spawn } from 'node:child_process'; import { createWriteStream, existsSync, mkdirSync, readFileSync, unlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -294,14 +294,32 @@ export class ManagedRunner { /** * 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. + * + * Platform-specific implementation: + * - Unix: process.kill(-pid, signal) sends signal to all processes in the group + * - Windows: taskkill /T kills the entire process tree */ private _killProcessGroup(signal: NodeJS.Signals): void { if (!this._pid) return; + try { - process.kill(-this._pid, signal); - } catch { + if (process.platform === 'win32') { + // Windows: use taskkill /T to kill the entire process tree + // /T = terminate all child processes + // /F = force termination (only for SIGKILL equivalent) + const args = signal === 'SIGKILL' + ? ['/PID', String(this._pid), '/T', '/F'] + : ['/PID', String(this._pid), '/T']; + execFileSync('taskkill', args, { stdio: 'ignore' }); + log.debug({ pid: this._pid, signal, args }, 'ManagedRunner: taskkill sent (Windows)'); + } else { + // Unix: use negative PID to kill the process group + process.kill(-this._pid, signal); + log.debug({ pid: this._pid, signal }, 'ManagedRunner: signal sent to process group (Unix)'); + } + } 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 dead)'); } } From dcc87b8288555a245d8e3a020a4d3c0388dad8e7 Mon Sep 17 00:00:00 2001 From: "CodexSol-GPT-5.6-sol" Date: Wed, 12 Aug 2026 12:52:22 -0700 Subject: [PATCH 2/4] fix(F167): bound Windows managed process cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: Windows cannot use Unix negative process-group PIDs, while a synchronous taskkill would block the API event loop and its SIGKILL recovery timer. Add an asynchronous bounded taskkill seam with observable outcomes plus deterministic and real-Windows descendant cleanup coverage. [小太阳·砚砚/GPT-5.6 Sol🐾] Thread-Context: threadId=thread_msqi08quzgvdbv5u catId=codex-sol --- .../managed-runner-process-tree.ts | 120 ++++++++++++++++ .../api/src/infrastructure/managed-runner.ts | 72 ++++------ .../test/managed-runner-process-tree.test.js | 129 ++++++++++++++++++ .../api/test/managed-runner-windows.test.js | 117 ++++++++++++++++ 4 files changed, 391 insertions(+), 47 deletions(-) create mode 100644 packages/api/src/infrastructure/managed-runner-process-tree.ts create mode 100644 packages/api/test/managed-runner-process-tree.test.js create mode 100644 packages/api/test/managed-runner-windows.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..2ac46425de --- /dev/null +++ b/packages/api/src/infrastructure/managed-runner-process-tree.ts @@ -0,0 +1,120 @@ +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 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), +}; + +export function buildTaskkillArgs(pid: number, signal: NodeJS.Signals): string[] { + const args = ['/PID', String(pid), '/T']; + if (signal === 'SIGKILL') args.push('/F'); + return args; +} + +/** + * 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 4cf9e68ede..56d21be250 100644 --- a/packages/api/src/infrastructure/managed-runner.ts +++ b/packages/api/src/infrastructure/managed-runner.ts @@ -4,23 +4,20 @@ * 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) */ -import { type ChildProcess, execFileSync, spawn } from 'node:child_process'; +import { type ChildProcess, spawn } from 'node:child_process'; import { createWriteStream, existsSync, mkdirSync, readFileSync, unlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { createModuleLogger } from './logger.js'; import { buildManagedRunnerEnvironment } from './managed-runner-environment.js'; +import { terminateWindowsProcessTree } from './managed-runner-process-tree.js'; const log = createModuleLogger('managed-runner'); @@ -101,8 +98,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, @@ -179,19 +175,14 @@ export class ManagedRunner { // 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). + // Escalate even if the shell exits before descendants. this._killTimer = setTimeout(() => { log.warn({ pid: this._pid }, 'ManagedRunner: SIGKILL after grace period'); this._killProcessGroup('SIGKILL'); }, KILL_GRACE_MS); }, 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,11 +192,7 @@ 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; @@ -280,46 +267,37 @@ export class ManagedRunner { 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 + // Terminate the whole process tree. 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. + // Escalate even if the shell exits before descendants. this._killTimer = setTimeout(() => { log.warn({ pid: this._pid }, 'ManagedRunner: SIGKILL after cancel grace period'); this._killProcessGroup('SIGKILL'); }, KILL_GRACE_MS); } - /** - * P1-2 fix: kill the entire process group (shell + child processes). - * With detached=true, child.pid IS the process group leader. - * - * Platform-specific implementation: - * - Unix: process.kill(-pid, signal) sends signal to all processes in the group - * - Windows: taskkill /T kills the entire process tree - */ + /** Terminate the detached process group (Unix) or process tree (Windows). */ private _killProcessGroup(signal: NodeJS.Signals): void { if (!this._pid) return; + if (process.platform === 'win32') { + const pid = this._pid; + void terminateWindowsProcessTree(pid, signal).then((result) => { + if (result.status === 'completed') { + log.debug({ pid, signal }, 'ManagedRunner: taskkill completed'); + return; + } + log.warn({ pid, signal, result }, `ManagedRunner: taskkill ${result.status}`); + }); + return; + } + try { - if (process.platform === 'win32') { - // Windows: use taskkill /T to kill the entire process tree - // /T = terminate all child processes - // /F = force termination (only for SIGKILL equivalent) - const args = signal === 'SIGKILL' - ? ['/PID', String(this._pid), '/T', '/F'] - : ['/PID', String(this._pid), '/T']; - execFileSync('taskkill', args, { stdio: 'ignore' }); - log.debug({ pid: this._pid, signal, args }, 'ManagedRunner: taskkill sent (Windows)'); - } else { - // Unix: use negative PID to kill the process group - process.kill(-this._pid, signal); - log.debug({ pid: this._pid, signal }, 'ManagedRunner: signal sent to process group (Unix)'); - } + process.kill(-this._pid, signal); } 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 dead)'); + log.debug({ pid: this._pid, signal, err }, 'ManagedRunner: kill process group failed (may already be gone)'); } } 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..04524c6fde --- /dev/null +++ b/packages/api/test/managed-runner-process-tree.test.js @@ -0,0 +1,129 @@ +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; +import { test } from 'node:test'; + +const { TASKKILL_TIMEOUT_MS, 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 }; + }, + }; +} + +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); +}); 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 }); + } +}); From f7060816de14f933c141e985a3c317fa21d0dbb5 Mon Sep 17 00:00:00 2001 From: "CodexSol-GPT-5.6-sol" <773678591@qq.com> Date: Wed, 12 Aug 2026 13:30:19 -0700 Subject: [PATCH 3/4] test(windows): run ManagedRunner tree cleanup regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: the PR changes Windows process-tree termination, so target-owned CI must execute the real parent-plus-descendant cancel and timeout regressions on Windows. Thread-Context: thread_msqi08quzgvdbv5u [小太阳·砚砚/GPT-5.6 Sol🐾] --- .github/workflows/windows-smoke.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 From 9df5fa3c9a7c1a75807972b59051dc4daa98cfe7 Mon Sep 17 00:00:00 2001 From: "CodexSol-GPT-5.6-sol" Date: Wed, 12 Aug 2026 14:08:35 -0700 Subject: [PATCH 4/4] fix(F167): cancel completed Windows kill escalation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: a successful taskkill /T made the later forced /F /T timer stale, allowing PID reuse to target an unrelated tree. Bind each escalation to its graceful attempt, cancel only completed Windows outcomes, and retain failure, timeout, and Unix fallback semantics. [小太阳·砚砚/GPT-5.6 Sol🐾] Thread-Context: threadId=thread_msqi08quzgvdbv5u catId=codex-sol --- .../managed-runner-process-tree.ts | 52 +++++++++++++ .../api/src/infrastructure/managed-runner.ts | 73 +++++++++--------- .../test/managed-runner-process-tree.test.js | 75 ++++++++++++++++++- 3 files changed, 163 insertions(+), 37 deletions(-) diff --git a/packages/api/src/infrastructure/managed-runner-process-tree.ts b/packages/api/src/infrastructure/managed-runner-process-tree.ts index 2ac46425de..a53aecd7bb 100644 --- a/packages/api/src/infrastructure/managed-runner-process-tree.ts +++ b/packages/api/src/infrastructure/managed-runner-process-tree.ts @@ -19,6 +19,15 @@ 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; @@ -50,12 +59,55 @@ const DEFAULT_DEPS: WindowsProcessTreeTerminationDeps = { 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. * diff --git a/packages/api/src/infrastructure/managed-runner.ts b/packages/api/src/infrastructure/managed-runner.ts index 56d21be250..a6c1bcca95 100644 --- a/packages/api/src/infrastructure/managed-runner.ts +++ b/packages/api/src/infrastructure/managed-runner.ts @@ -17,7 +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 { terminateWindowsProcessTree } from './managed-runner-process-tree.js'; +import { + armProcessTreeKillEscalation, + type ProcessTreeKillEscalation, + terminateWindowsProcessTree, + type WindowsProcessTreeTerminationResult, +} from './managed-runner-process-tree.js'; const log = createModuleLogger('managed-runner'); @@ -57,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[] = []; @@ -172,14 +177,8 @@ 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'); - - // Escalate even if the shell exits before descendants. - 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); // `close` waits for stdout/stderr to drain before the tail is read. @@ -198,10 +197,7 @@ export class ManagedRunner { 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; @@ -257,40 +253,32 @@ 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(); - // Terminate the whole process tree. - this._killProcessGroup('SIGTERM'); - - // Escalate even if the shell exits before descendants. - 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'); } /** Terminate the detached process group (Unix) or process tree (Windows). */ - private _killProcessGroup(signal: NodeJS.Signals): void { - if (!this._pid) return; + private _killProcessGroup(signal: NodeJS.Signals): Promise | null { + if (!this._pid) return null; if (process.platform === 'win32') { const pid = this._pid; - void terminateWindowsProcessTree(pid, signal).then((result) => { + 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; + return termination; } try { @@ -299,6 +287,26 @@ export class ManagedRunner { // 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 { @@ -306,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 index 04524c6fde..acf8d1909b 100644 --- a/packages/api/test/managed-runner-process-tree.test.js +++ b/packages/api/test/managed-runner-process-tree.test.js @@ -2,9 +2,8 @@ import assert from 'node:assert/strict'; import { EventEmitter } from 'node:events'; import { test } from 'node:test'; -const { TASKKILL_TIMEOUT_MS, buildTaskkillArgs, terminateWindowsProcessTree } = await import( - '../dist/infrastructure/managed-runner-process-tree.js' -); +const { TASKKILL_TIMEOUT_MS, armProcessTreeKillEscalation, buildTaskkillArgs, terminateWindowsProcessTree } = + await import('../dist/infrastructure/managed-runner-process-tree.js'); function createHarness() { const child = new EventEmitter(); @@ -50,6 +49,41 @@ function createHarness() { }; } +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']); @@ -127,3 +161,38 @@ test('Windows tree termination times out deterministically and stops taskkill', }); 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`); + } +});