From 984e0586248af520be95d8dcaff7ae46a53a4769 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sat, 20 Jun 2026 12:20:12 +0200 Subject: [PATCH] fix(e2e): reap signaled PTY command trees --- scripts/e2e/lib/run-with-pty.mjs | 133 ++++++++++++++++++++++---- test/scripts/e2e-run-with-pty.test.ts | 30 ++++-- 2 files changed, 137 insertions(+), 26 deletions(-) diff --git a/scripts/e2e/lib/run-with-pty.mjs b/scripts/e2e/lib/run-with-pty.mjs index 2842ad577d6..14360fe256a 100644 --- a/scripts/e2e/lib/run-with-pty.mjs +++ b/scripts/e2e/lib/run-with-pty.mjs @@ -1,8 +1,8 @@ #!/usr/bin/env node -// Runs an E2E command under a pseudo-terminal. +import { spawnSync } from "node:child_process"; import fs from "node:fs"; import process from "node:process"; -import { spawn } from "@lydell/node-pty"; +import { spawn as spawnPty } from "@lydell/node-pty"; import { readPositiveIntEnv } from "./env-limits.mjs"; const [logPath, command, ...args] = process.argv.slice(2); @@ -17,6 +17,9 @@ if (!logPath || !command) { let exiting = false; let forwardedSignal = null; let forceKillTimer = null; +let terminationDrainTimer = null; +let terminationPids = []; +let pendingExitCode = null; let logFailed = false; const outputLimitMarker = `\n[run-with-pty output truncated after ${OUTPUT_MAX_BYTES} bytes]\n`; const outputState = { @@ -25,7 +28,7 @@ const outputState = { }; const log = fs.createWriteStream(logPath, { flags: "w" }); -const pty = spawn(command, args, { +const pty = spawnPty(command, args, { name: process.env.TERM || "xterm-256color", cols: readPositiveIntEnv("COLUMNS", 120), rows: readPositiveIntEnv("LINES", 40), @@ -43,11 +46,7 @@ log.on("error", (error) => { process.exit(1); } if (!exiting) { - pty.kill("SIGTERM"); - forceKillTimer ??= setTimeout(() => { - pty.kill("SIGKILL"); - }, FORCE_KILL_MS); - forceKillTimer.unref?.(); + terminatePtyTree("SIGTERM"); } }); @@ -86,18 +85,23 @@ pty.onData((data) => { pty.onExit(({ exitCode, signal }) => { exiting = true; - clearTimeout(forceKillTimer); + if (terminationPids.length === 0) { + clearTerminationTimers(); + } if (logFailed) { - process.exit(1); + exitWhenTerminationDrains(1); + return; } log.end(() => { if (forwardedSignal) { - process.exit(signalExitCode(forwardedSignal)); + exitWhenTerminationDrains(signalExitCode(forwardedSignal)); + return; } if (typeof exitCode === "number") { - process.exit(exitCode); + exitWhenTerminationDrains(exitCode); + return; } - process.exit(signal ? 128 + signal : 1); + exitWhenTerminationDrains(signal ? 128 + signal : 1); }); }); @@ -109,15 +113,108 @@ for (const signal of ["SIGHUP", "SIGINT", "SIGTERM"]) { process.on(signal, () => { if (!exiting) { forwardedSignal ??= signal; - pty.kill(signal); - forceKillTimer ??= setTimeout(() => { - pty.kill("SIGKILL"); - }, FORCE_KILL_MS); - forceKillTimer.unref?.(); + terminatePtyTree(signal); } }); } +function terminatePtyTree(signal) { + // node-pty kill() targets only pty.pid on Unix; wrapper-owned shutdowns + // keep the captured child tree alive until ignored descendants drain. + if (terminationPids.length === 0) { + terminationPids = collectPtyProcessTreePids(); + } + signalPtyProcessTree(signal); + forceKillTimer ??= setTimeout(() => { + signalPtyProcessTree("SIGKILL"); + }, FORCE_KILL_MS); + forceKillTimer.unref?.(); +} + +function exitWhenTerminationDrains(exitCode) { + pendingExitCode = exitCode; + if (processTreeIsAlive(terminationPids)) { + terminationDrainTimer ??= setInterval(finishIfTerminationDrained, 25); + return; + } + finishIfTerminationDrained(); +} + +function finishIfTerminationDrained() { + if (processTreeIsAlive(terminationPids)) { + return; + } + clearTerminationTimers(); + process.exit(pendingExitCode ?? 1); +} + +function clearTerminationTimers() { + if (forceKillTimer) { + clearTimeout(forceKillTimer); + forceKillTimer = null; + } + if (terminationDrainTimer) { + clearInterval(terminationDrainTimer); + terminationDrainTimer = null; + } +} + +function collectPtyProcessTreePids() { + if (process.platform === "win32" || typeof pty.pid !== "number") { + return typeof pty.pid === "number" ? [pty.pid] : []; + } + const ps = spawnSync("ps", ["-axo", "pid=,ppid="], { encoding: "utf8" }); + if (ps.status !== 0) { + return [pty.pid]; + } + const childrenByParent = new Map(); + for (const line of ps.stdout.split("\n")) { + const match = line.trim().match(/^(\d+)\s+(\d+)$/u); + if (!match) { + continue; + } + const pid = Number(match[1]); + const ppid = Number(match[2]); + const siblings = childrenByParent.get(ppid) ?? []; + siblings.push(pid); + childrenByParent.set(ppid, siblings); + } + const pids = [pty.pid]; + for (const parentPid of pids) { + for (const pid of childrenByParent.get(parentPid) ?? []) { + pids.push(pid); + } + } + return [...new Set(pids)]; +} + +function processTreeIsAlive(pids) { + return pids.some((pid) => { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return error?.code === "EPERM"; + } + }); +} + +function signalPtyProcessTree(signal) { + if (process.platform === "win32" || terminationPids.length === 0) { + pty.kill(signal); + return; + } + for (const pid of terminationPids.toReversed()) { + try { + process.kill(pid, signal); + } catch (error) { + if (error?.code !== "ESRCH") { + throw error; + } + } + } +} + function signalExitCode(signal) { switch (signal) { case "SIGHUP": diff --git a/test/scripts/e2e-run-with-pty.test.ts b/test/scripts/e2e-run-with-pty.test.ts index 1ddf9c67574..9580d50f2e0 100644 --- a/test/scripts/e2e-run-with-pty.test.ts +++ b/test/scripts/e2e-run-with-pty.test.ts @@ -1,5 +1,6 @@ // E2E Run With Pty tests cover e2e run with pty script behavior. import { spawn } from "node:child_process"; +import { existsSync } from "node:fs"; import { mkdtemp, readFile, rm } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -127,15 +128,23 @@ describe("run-with-pty", () => { posixIt("escalates forwarded termination signals for PTY commands that ignore them", async () => { const tempRoot = await mkdtemp(path.join(os.tmpdir(), "openclaw-run-with-pty-")); const logPath = path.join(tempRoot, "pty.log"); + const descendantPidPath = path.join(tempRoot, "descendant.pid"); + let descendantPid: number | null = null; + const probeCode = ` +const { spawn } = require("node:child_process"); +const fs = require("node:fs"); +process.on("SIGTERM", () => process.exit(0)); +const descendant = spawn(process.execPath, [ + "-e", + "process.on('SIGTERM',()=>{});process.on('SIGHUP',()=>{});setInterval(()=>{},1000);", +], { stdio: "ignore" }); +fs.writeFileSync(${JSON.stringify(descendantPidPath)}, String(descendant.pid)); +console.log("ready"); +setInterval(() => {}, 1000); +`; const child = spawn( process.execPath, - [ - scriptPath, - logPath, - process.execPath, - "-e", - "process.on('SIGTERM', () => {}); console.log('ready'); setInterval(() => {}, 1000);", - ], + [scriptPath, logPath, process.execPath, "-e", probeCode], { env: { ...process.env, @@ -156,7 +165,8 @@ describe("run-with-pty", () => { }); try { - await waitFor(() => stdout.text().includes("ready")); + await waitFor(() => stdout.text().includes("ready") && existsSync(descendantPidPath)); + descendantPid = Number(await readFile(descendantPidPath, "utf8")); child.kill("SIGTERM"); const result = await waitForClose(child, 5_000); const log = await readFile(logPath, "utf8"); @@ -164,7 +174,11 @@ describe("run-with-pty", () => { expect(result).toEqual({ code: 143, signal: null }); expect(stderr.text()).toBe(""); expect(log).toContain("ready"); + expect(isProcessAlive(descendantPid)).toBe(false); } finally { + if (descendantPid && isProcessAlive(descendantPid)) { + process.kill(descendantPid, "SIGKILL"); + } if (child.pid && isProcessAlive(child.pid)) { child.kill("SIGKILL"); }