diff --git a/scripts/e2e/telegram-user-credential-io.ts b/scripts/e2e/telegram-user-credential-io.ts index df35832dfaaa..8b1e13ec02a2 100644 --- a/scripts/e2e/telegram-user-credential-io.ts +++ b/scripts/e2e/telegram-user-credential-io.ts @@ -21,6 +21,33 @@ type RunCommandOptions = { const DEFAULT_OUTPUT_LIMIT = 128 * 1024; const DEFAULT_FETCH_BODY_LIMIT = 1024 * 1024; const KILL_GRACE_MS = 5_000; +const SIGNAL_EXIT_CODES = { + SIGHUP: 129, + SIGINT: 130, + SIGTERM: 143, +}; +const ACTIVE_CHILD_TREE_KILLERS = new Set<(signal: NodeJS.Signals) => void>(); +let forwardedSignalExitCode: number | undefined; +let forwardedSignalForceKillTimer: NodeJS.Timeout | undefined; + +for (const signal of Object.keys(SIGNAL_EXIT_CODES) as Array) { + process.on(signal, () => { + forwardedSignalExitCode ??= SIGNAL_EXIT_CODES[signal]; + if (ACTIVE_CHILD_TREE_KILLERS.size === 0) { + process.exit(forwardedSignalExitCode); + } + const activeKillers = Array.from(ACTIVE_CHILD_TREE_KILLERS); + for (const killChildTree of activeKillers) { + killChildTree(signal); + } + forwardedSignalForceKillTimer ??= setTimeout(() => { + for (const killChildTree of activeKillers) { + killChildTree("SIGKILL"); + } + process.exit(forwardedSignalExitCode); + }, KILL_GRACE_MS); + }); +} function timeoutError(message: string) { return Object.assign(new Error(message), { code: "ETIMEDOUT" }); @@ -70,15 +97,19 @@ export function runCommand( options: RunCommandOptions, ) { return new Promise((resolve, reject) => { + const useProcessGroup = process.platform !== "win32"; const child = spawn(command, args, { cwd, stdio: ["ignore", "pipe", "pipe"], + detached: useProcessGroup, }); + const activeChildTree = registerActiveChildProcessTree(child); const outputLimit = options.outputLimit ?? DEFAULT_OUTPUT_LIMIT; let stdout = ""; let stderr = ""; let settled = false; let killTimer: NodeJS.Timeout | undefined; + let pendingTimeoutReject: (() => void) | undefined; let timedOutError: Error | undefined; const timeoutMs = Math.max(1, options.timeoutMs); const timeoutKillGraceMs = Math.max(0, options.timeoutKillGraceMs ?? KILL_GRACE_MS); @@ -94,6 +125,7 @@ export function runCommand( } settled = true; clearTimers(); + activeChildTree.unregister(); reject(error); }; const timeout: NodeJS.Timeout = setTimeout(() => { @@ -103,11 +135,12 @@ export function runCommand( timedOutError = timeoutError( `${command} ${args.join(" ")} timed out after ${timeoutMs}ms\n${stdout}${stderr}`, ); - child.kill("SIGTERM"); + activeChildTree.killChildTree("SIGTERM"); killTimer = setTimeout(() => { - child.kill("SIGKILL"); + killTimer = undefined; + activeChildTree.killChildTree("SIGKILL"); + pendingTimeoutReject?.(); }, timeoutKillGraceMs); - killTimer.unref?.(); }, timeoutMs); timeout.unref?.(); @@ -122,8 +155,26 @@ export function runCommand( if (settled) { return; } + if (forwardedSignalExitCode !== undefined) { + activeChildTree.unregister(); + return; + } + if (timedOutError && killTimer && childProcessTreeMayStillExist(child)) { + const error = timedOutError; + pendingTimeoutReject = () => { + if (settled) { + return; + } + settled = true; + clearTimers(); + activeChildTree.unregister(); + reject(error); + }; + return; + } settled = true; clearTimers(); + activeChildTree.unregister(); if (timedOutError) { reject(timedOutError); return; @@ -138,6 +189,41 @@ export function runCommand( }); } +function signalChildProcessTree(child: ReturnType, signal: NodeJS.Signals) { + if (process.platform !== "win32" && child.pid) { + try { + process.kill(-child.pid, signal); + return; + } catch { + // The process group can disappear between timeout and cleanup. + } + } + child.kill(signal); +} + +function childProcessTreeMayStillExist(child: ReturnType) { + if (process.platform === "win32" || !child.pid) { + return false; + } + try { + process.kill(-child.pid, 0); + return true; + } catch { + return false; + } +} + +function registerActiveChildProcessTree(child: ReturnType) { + const killChildTree = (signal: NodeJS.Signals) => signalChildProcessTree(child, signal); + ACTIVE_CHILD_TREE_KILLERS.add(killChildTree); + return { + killChildTree, + unregister: () => { + ACTIVE_CHILD_TREE_KILLERS.delete(killChildTree); + }, + }; +} + export async function fetchJsonWithTimeout(params: FetchJsonParams) { const timeoutMs = Math.max(1, params.timeoutMs); const maxBodyBytes = resolveFetchBodyLimit(params.maxBodyBytes); diff --git a/test/scripts/telegram-user-credential.test.ts b/test/scripts/telegram-user-credential.test.ts index 15bc32de0e59..804a0dae8c1f 100644 --- a/test/scripts/telegram-user-credential.test.ts +++ b/test/scripts/telegram-user-credential.test.ts @@ -1,4 +1,4 @@ -import { existsSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path, { win32 } from "node:path"; @@ -18,6 +18,41 @@ function makeTempDir(prefix: string) { return dir; } +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function waitForFile(filePath: string, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (existsSync(filePath)) { + return; + } + await new Promise((resolve) => { + setTimeout(resolve, 25); + }); + } + throw new Error(`timeout waiting for ${filePath}`); +} + +async function waitForDead(pid: number, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (!isProcessAlive(pid)) { + return; + } + await new Promise((resolve) => { + setTimeout(resolve, 25); + }); + } + throw new Error(`process still alive: ${pid}`); +} + afterEach(() => { for (const dir of tempDirs.splice(0)) { rmSync(dir, { force: true, recursive: true }); @@ -112,6 +147,43 @@ setInterval(() => {}, 1000); }, ); + it.runIf(process.platform !== "win32")( + "kills timed-out child process groups", + async () => { + const dir = makeTempDir("openclaw-telegram-credential-tree-timeout-"); + const childPidPath = path.join(dir, "child.pid"); + let childPid: number | undefined; + + try { + const childScript = "process.on('SIGTERM', () => {}); setInterval(() => {}, 1000);"; + const parentScript = [ + "const { spawn } = require('node:child_process');", + "const fs = require('node:fs');", + `const child = spawn(process.execPath, ['-e', ${JSON.stringify(childScript)}], { stdio: 'ignore' });`, + `fs.writeFileSync(${JSON.stringify(childPidPath)}, String(child.pid));`, + "setInterval(() => {}, 1000);", + ].join(""); + + const runPromise = runCommand(process.execPath, ["-e", parentScript], dir, { + timeoutKillGraceMs: 25, + timeoutMs: 100, + }); + await waitForFile(childPidPath, 2_000); + childPid = Number.parseInt(readFileSync(childPidPath, "utf8"), 10); + + await expect(runPromise).rejects.toMatchObject({ + code: "ETIMEDOUT", + message: expect.stringContaining("timed out after 100ms"), + }); + await waitForDead(childPid, 2_000); + } finally { + if (childPid !== undefined && isProcessAlive(childPid)) { + process.kill(childPid, "SIGKILL"); + } + } + }, + ); + it("aborts broker fetches that never return", async () => { let signal: AbortSignal | undefined; await expect(