diff --git a/packages/eval/src/__tests__/process-termination.test.ts b/packages/eval/src/__tests__/process-termination.test.ts new file mode 100644 index 0000000000..696e934c27 --- /dev/null +++ b/packages/eval/src/__tests__/process-termination.test.ts @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import type { ChildProcess } from 'node:child_process'; +import test from 'node:test'; +import { terminateProcess } from '../process-termination.js'; + +function childProcess(pid = 123): { + child: ChildProcess; + signals: Array; +} { + const signals: Array = []; + return { + child: { + pid, + exitCode: null, + signalCode: null, + kill: (signal?: NodeJS.Signals) => { + signals.push(signal); + return true; + }, + } as unknown as ChildProcess, + signals, + }; +} + +test('uses taskkill for first-stage Windows termination so descendants cannot outlive the root', async () => { + const { child, signals } = childProcess(); + const pids: number[] = []; + + assert.equal( + await terminateProcess(child, 'SIGTERM', { + platform: 'win32', + runTaskkill: async (pid) => { + pids.push(pid); + return true; + }, + }), + true, + ); + assert.deepEqual(pids, [123]); + assert.deepEqual(signals, []); +}); + +test('uses taskkill for forced Windows termination and preserves the process tree', async () => { + const { child, signals } = childProcess(456); + const pids: number[] = []; + + assert.equal( + await terminateProcess(child, 'SIGKILL', { + platform: 'win32', + runTaskkill: async (pid) => { + pids.push(pid); + return true; + }, + }), + true, + ); + assert.deepEqual(pids, [456]); + assert.deepEqual(signals, []); +}); + +test('falls back to the root process when Windows taskkill is unavailable', async () => { + const { child, signals } = childProcess(); + + assert.equal( + await terminateProcess(child, 'SIGKILL', { + platform: 'win32', + runTaskkill: async () => false, + }), + true, + ); + assert.deepEqual(signals, [undefined]); +}); + +test('preserves POSIX signal semantics', async () => { + const { child, signals } = childProcess(); + + assert.equal(await terminateProcess(child, 'SIGTERM', { platform: 'linux' }), true); + assert.deepEqual(signals, ['SIGTERM']); +}); + +test('does not signal a child that has already exited', async () => { + const { child, signals } = childProcess(); + Object.assign(child, { exitCode: 0 }); + + assert.equal(await terminateProcess(child, 'SIGKILL', { platform: 'win32' }), false); + assert.deepEqual(signals, []); +}); diff --git a/packages/eval/src/__tests__/process-termination.windows.test.ts b/packages/eval/src/__tests__/process-termination.windows.test.ts new file mode 100644 index 0000000000..4911b5d454 --- /dev/null +++ b/packages/eval/src/__tests__/process-termination.windows.test.ts @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { mkdtemp, readFile, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import test from 'node:test'; +import { terminateProcess } from '../process-termination.js'; + +const WINDOWS_PROCESS_SETTLEMENT_MS = 5_000; + +test('first-stage Windows termination removes a supervisor and its descendant', { + skip: process.platform !== 'win32', + timeout: 10_000, +}, async () => { + const root = await mkdtemp(join(tmpdir(), 'maka-eval-process-tree-')); + const workerPidPath = join(root, 'worker.pid'); + const supervisor = spawn( + process.execPath, + [ + '-e', + ` +const { spawn } = require('node:child_process'); +const { writeFileSync } = require('node:fs'); +const worker = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { + stdio: 'ignore', + windowsHide: true, +}); +writeFileSync(process.argv[1], String(worker.pid)); +setInterval(() => {}, 1000); +`, + workerPidPath, + ], + { stdio: 'ignore', windowsHide: true }, + ); + let workerPid: number | undefined; + try { + workerPid = await waitForWorkerPid(workerPidPath); + assert.equal(await terminateProcess(supervisor, 'SIGTERM'), true); + await waitForExit(supervisor); + await waitForProcessToExit(workerPid); + } finally { + if (supervisor.exitCode === null && supervisor.signalCode === null) { + await terminateProcess(supervisor, 'SIGKILL'); + } + if (workerPid !== undefined && isProcessAlive(workerPid)) { + try { + process.kill(workerPid); + } catch { + // The worker may exit between the liveness check and cleanup. + } + } + await rm(root, { recursive: true, force: true }); + } +}); + +async function waitForWorkerPid(path: string): Promise { + const deadline = Date.now() + WINDOWS_PROCESS_SETTLEMENT_MS; + while (Date.now() < deadline) { + try { + const pid = Number(await readFile(path, 'utf8')); + if (Number.isSafeInteger(pid) && pid > 0) return pid; + } catch { + // The supervisor has not written its worker PID yet. + } + await delay(20); + } + throw new Error('supervisor did not report its worker PID'); +} + +async function waitForExit(child: ReturnType): Promise { + const deadline = Date.now() + WINDOWS_PROCESS_SETTLEMENT_MS; + while (child.exitCode === null && child.signalCode === null && Date.now() < deadline) { + await delay(20); + } + assert.ok( + child.exitCode !== null || child.signalCode !== null, + 'supervisor did not exit after taskkill', + ); +} + +async function waitForProcessToExit(pid: number): Promise { + const deadline = Date.now() + WINDOWS_PROCESS_SETTLEMENT_MS; + while (isProcessAlive(pid) && Date.now() < deadline) await delay(20); + assert.equal(isProcessAlive(pid), false, `descendant ${pid} survived tree termination`); +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} diff --git a/packages/eval/src/harbor-external-subject.ts b/packages/eval/src/harbor-external-subject.ts index b4b58a59db..8a407c053f 100644 --- a/packages/eval/src/harbor-external-subject.ts +++ b/packages/eval/src/harbor-external-subject.ts @@ -39,6 +39,7 @@ import { type ProviderUsage as Usage, } from './provider-metering.js'; import { removeEvalWebTools } from './provider-web-tool-surface.js'; +import { terminateProcess } from './process-termination.js'; import { takeRelayResultToken, writeRelayResult } from './relay-result-frame.js'; const resultToken = takeRelayResultToken(); @@ -340,10 +341,10 @@ if (!systemRoot?.startsWith('/')) throw new Error('external subject system root let credentialPath: string | undefined; let child: ChildProcess | undefined; let stopped = false; -const stop = (signal: NodeJS.Signals) => { +const stop = (signal: 'SIGINT' | 'SIGTERM') => { stopped = true; removeCredential(); - child?.kill(signal); + void terminateProcess(child, signal); }; const terminate = () => stop('SIGTERM'); const interrupt = () => stop('SIGINT'); diff --git a/packages/eval/src/harness-executor.ts b/packages/eval/src/harness-executor.ts index 617e6a9164..e5e19fcd9e 100644 --- a/packages/eval/src/harness-executor.ts +++ b/packages/eval/src/harness-executor.ts @@ -48,6 +48,7 @@ import { type SubjectExecutionContext, } from './runner.js'; import type { EvalResult } from './result.js'; +import { terminateProcess } from './process-termination.js'; export type HarnessFramework = 'harbor' | 'pier'; type RelayTransportStage = 'ready' | 'execute' | 'receive' | 'decision'; @@ -1195,7 +1196,7 @@ async function waitForTrial(child: ChildProcess, wait: TrialWait): Promise { signal?.throwIfAborted(); await new Promise((resolvePromise, rejectPromise) => { - execFile( - command, - [...args], - { - env: environment, - cwd, - timeout: PREFLIGHT_TIMEOUT_MS, - killSignal: 'SIGKILL', - maxBuffer: PREFLIGHT_OUTPUT_LIMIT_BYTES, - encoding: 'utf8', - ...(signal ? { signal } : {}), - }, - (error, _stdout, stderr) => { - if (!error) { - resolvePromise(); - return; - } - rejectPromise(new Error(stderr.trim() || error.message)); - }, - ); + let settled = false; + let timeout: NodeJS.Timeout | undefined; + let child: ChildProcess | undefined; + const onAbort = () => { + void terminateProcess(child, 'SIGTERM'); + }; + const settle = (error?: Error, stderr = '') => { + if (settled) return; + settled = true; + if (timeout) clearTimeout(timeout); + signal?.removeEventListener('abort', onAbort); + if (!error) { + resolvePromise(); + return; + } + rejectPromise(new Error(stderr.trim() || error.message)); + }; + try { + child = execFile( + command, + [...args], + { + env: environment, + cwd, + killSignal: 'SIGTERM', + maxBuffer: PREFLIGHT_OUTPUT_LIMIT_BYTES, + encoding: 'utf8', + }, + (error, _stdout, stderr) => { + settle(error ?? undefined, stderr); + }, + ); + } catch (error) { + settle(error instanceof Error ? error : new Error(String(error))); + return; + } + timeout = setTimeout(() => { + void terminateProcess(child, 'SIGKILL'); + }, PREFLIGHT_TIMEOUT_MS); + if (signal) { + if (signal.aborted) onAbort(); + else signal.addEventListener('abort', onAbort, { once: true }); + } }); } diff --git a/packages/eval/src/process-termination.ts b/packages/eval/src/process-termination.ts new file mode 100644 index 0000000000..6a267a6b0e --- /dev/null +++ b/packages/eval/src/process-termination.ts @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { spawn, type ChildProcess } from 'node:child_process'; + +export type ProcessTerminationSignal = 'SIGINT' | 'SIGTERM' | 'SIGKILL'; + +const WINDOWS_TASKKILL_TIMEOUT_MS = 2_000; + +export interface ProcessTerminationOptions { + readonly platform?: NodeJS.Platform; + readonly runTaskkill?: (pid: number) => Promise; +} + +/** + * Terminates a child using the platform's process lifecycle semantics. + * Windows does not implement POSIX signals, so every termination request must + * include descendants. Otherwise the root can exit before forced escalation + * and leave its workers running. + */ +export function terminateProcess( + child: ChildProcess | undefined, + signal: ProcessTerminationSignal, + options: ProcessTerminationOptions = {}, +): Promise { + if (!child || child.exitCode !== null || child.signalCode !== null) return Promise.resolve(false); + + const platform = options.platform ?? process.platform; + if (platform === 'win32') { + const pid = child.pid; + if (typeof pid !== 'number' || !Number.isSafeInteger(pid) || pid <= 0) { + return killChild(child); + } + return (options.runTaskkill ?? killWindowsTree)(pid).then((killed) => + killed ? true : killChild(child), + ); + } + + return killChild(child, signal); +} + +function killChild(child: ChildProcess, signal?: ProcessTerminationSignal): Promise { + try { + // Omitting the signal is intentional on Windows: Node uses its native + // graceful termination path instead of trying to emulate a POSIX signal. + return Promise.resolve(signal === undefined ? child.kill() : child.kill(signal)); + } catch { + return Promise.resolve(false); + } +} + +function killWindowsTree(pid: number): Promise { + return new Promise((resolve) => { + let settled = false; + let timeout: NodeJS.Timeout | undefined; + const finish = (succeeded: boolean) => { + if (settled) return; + settled = true; + if (timeout) clearTimeout(timeout); + resolve(succeeded); + }; + try { + const killer = spawn('taskkill.exe', ['/PID', String(pid), '/T', '/F'], { + stdio: 'ignore', + windowsHide: true, + }); + killer.once('error', () => finish(false)); + killer.once('close', (code) => finish(code === 0)); + timeout = setTimeout(() => { + try { + killer.kill(); + } catch { + /* taskkill already exited */ + } + finish(false); + }, WINDOWS_TASKKILL_TIMEOUT_MS); + } catch { + finish(false); + } + }); +}