From ee4802802843235da403516da868cd5aafe23eaf Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Mon, 1 Jun 2026 19:40:39 +0200 Subject: [PATCH] fix(dev): clean tui pty watch children --- scripts/dev/tui-pty-test-watch.ts | 129 +++++++++++++++++++++--- test/scripts/dev-tooling-safety.test.ts | 67 +++++++++++- 2 files changed, 181 insertions(+), 15 deletions(-) diff --git a/scripts/dev/tui-pty-test-watch.ts b/scripts/dev/tui-pty-test-watch.ts index a53e8a3e5d2..a0d3390d63f 100644 --- a/scripts/dev/tui-pty-test-watch.ts +++ b/scripts/dev/tui-pty-test-watch.ts @@ -2,6 +2,7 @@ import { spawn } from "node:child_process"; import { mkdir, readFile, writeFile } from "node:fs/promises"; import { createRequire } from "node:module"; import path from "node:path"; +import { pathToFileURL } from "node:url"; type Options = { altScreen: boolean; @@ -20,6 +21,24 @@ const MODE_TEST_FILES = { const MIRROR_TERMINAL_QUERIES = ["\x1b[?u", "\x1b[16t"]; const DEFAULT_PTY_COLS = 100; const DEFAULT_PTY_ROWS = 30; +const CHILD_SIGTERM_GRACE_MS = 500; +const CHILD_SIGKILL_GRACE_MS = 5_000; + +type KillableChild = { + pid?: number; + kill(signal: NodeJS.Signals): boolean; +}; + +type ChildStopper = { + cancel: () => void; + stop: () => void; +}; + +type SignalChild = (child: KillableChild, signal: NodeJS.Signals) => void; + +function unrefTimer(timer: ReturnType): void { + (timer as { unref?: () => void }).unref?.(); +} function readOption(args: string[], name: string): string | undefined { const idx = args.indexOf(name); @@ -72,6 +91,64 @@ function currentTerminalDimension(value: number | undefined, fallback: number): return String(value && value > 0 ? value : fallback); } +function signalChildProcessTree(child: KillableChild, signal: NodeJS.Signals): void { + if (process.platform !== "win32" && typeof child.pid === "number") { + try { + process.kill(-child.pid, signal); + return; + } catch { + // Non-detached fallback or already-exited group; direct child signaling is + // still useful on platforms without process groups. + } + } + child.kill(signal); +} + +function createChildStopper( + child: KillableChild, + options: { + signalChild?: SignalChild; + sigtermGraceMs?: number; + sigkillGraceMs?: number; + } = {}, +): ChildStopper { + const signalChild = options.signalChild ?? signalChildProcessTree; + const sigtermGraceMs = options.sigtermGraceMs ?? CHILD_SIGTERM_GRACE_MS; + const sigkillGraceMs = options.sigkillGraceMs ?? CHILD_SIGKILL_GRACE_MS; + let stopping = false; + let termTimer: ReturnType | undefined; + let killTimer: ReturnType | undefined; + + const cancel = () => { + if (termTimer) { + clearTimeout(termTimer); + termTimer = undefined; + } + if (killTimer) { + clearTimeout(killTimer); + killTimer = undefined; + } + }; + + const stop = () => { + if (stopping) { + return; + } + stopping = true; + signalChild(child, "SIGINT"); + termTimer = setTimeout(() => { + signalChild(child, "SIGTERM"); + killTimer = setTimeout(() => { + signalChild(child, "SIGKILL"); + }, sigkillGraceMs); + unrefTimer(killTimer); + }, sigtermGraceMs); + unrefTimer(termTimer); + }; + + return { cancel, stop }; +} + async function createMirrorFile(mirrorPath: string): Promise { await mkdir(path.dirname(mirrorPath), { recursive: true }); await writeFile(mirrorPath, "", "utf8"); @@ -108,6 +185,7 @@ async function main(): Promise { ], { cwd: process.cwd(), + detached: process.platform !== "win32", env: { ...process.env, OPENCLAW_TUI_PTY_MIRROR_PATH: options.mirrorPath, @@ -172,10 +250,8 @@ async function main(): Promise { } }; - const stopChild = () => { - child.kill("SIGINT"); - setTimeout(() => child.kill("SIGTERM"), 500).unref(); - }; + const childStopper = createChildStopper(child); + const stopChild = childStopper.stop; const ignoredInput = (chunk: Buffer) => { if (chunk.includes(0x03)) { @@ -238,12 +314,20 @@ async function main(): Promise { childStderr += chunk.toString("utf8"); }); - let childExit: { code: number | null; signal: NodeJS.Signals | null } | null = null; - child.on("exit", (code, signal) => { - childExit = { code, signal }; + type ChildExit = { code: number | null; signal: NodeJS.Signals | null }; + let childExit: ChildExit | null = null; + const childFinished = new Promise((resolve) => { + child.once("exit", (code, signal) => { + childExit = { code, signal }; + childStopper.cancel(); + resolve(childExit); + }); }); - process.once("SIGINT", stopChild); + const parentSignals: NodeJS.Signals[] = ["SIGINT", "SIGTERM", "SIGHUP"]; + for (const signal of parentSignals) { + process.once(signal, stopChild); + } try { for (;;) { @@ -265,6 +349,12 @@ async function main(): Promise { writeMirrorChunk(result.chunk); } } finally { + if (!childExit) { + stopChild(); + } + for (const signal of parentSignals) { + process.off(signal, stopChild); + } await drainParentInput(); restoreInput(); if (useAltScreen) { @@ -273,6 +363,10 @@ async function main(): Promise { restoreScreen(); } + if (!childExit) { + childExit = await childFinished; + } + if (childStdout) { process.stdout.write(childStdout); } @@ -288,9 +382,16 @@ async function main(): Promise { } } -main().catch((error: unknown) => { - process.stderr.write( - `${error instanceof Error ? error.stack || error.message : String(error)}\n`, - ); - process.exit(1); -}); +if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) { + main().catch((error: unknown) => { + process.stderr.write( + `${error instanceof Error ? error.stack || error.message : String(error)}\n`, + ); + process.exit(1); + }); +} + +export const testing = { + createChildStopper, + signalChildProcessTree, +}; diff --git a/test/scripts/dev-tooling-safety.test.ts b/test/scripts/dev-tooling-safety.test.ts index 4dc99fbece6..d9126a60b54 100644 --- a/test/scripts/dev-tooling-safety.test.ts +++ b/test/scripts/dev-tooling-safety.test.ts @@ -2,11 +2,12 @@ import { EventEmitter } from "node:events"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { testing as promptProbeTesting } from "../../scripts/anthropic-prompt-probe.ts"; import { testing as claudeUsageTesting } from "../../scripts/debug-claude-usage.ts"; import { testing as discordSmokeTesting } from "../../scripts/dev/discord-acp-plain-language-smoke.ts"; import { testing as realtimeSmokeTesting } from "../../scripts/dev/realtime-talk-live-smoke.ts"; +import { testing as tuiPtyWatchTesting } from "../../scripts/dev/tui-pty-test-watch.ts"; import { maskIdentifier, parseBooleanEnv, @@ -16,6 +17,10 @@ import { redactJsonValueForDevToolLog, } from "../../scripts/lib/dev-tooling-safety.ts"; +afterEach(() => { + vi.useRealTimers(); +}); + describe("dev tooling safety helpers", () => { it("redacts secrets before truncating script log previews", () => { const token = "sk-test1234567890abcdefghijklmnop"; // pragma: allowlist secret @@ -193,6 +198,66 @@ describe("script-specific dev tooling hardening", () => { expect(calls).toBe(1); }); + it("escalates stalled TUI PTY watch children after interrupt cleanup", async () => { + vi.useFakeTimers(); + const signals: NodeJS.Signals[] = []; + const stopper = tuiPtyWatchTesting.createChildStopper( + { kill: () => true }, + { + signalChild(_child, signal: NodeJS.Signals): void { + signals.push(signal); + }, + sigkillGraceMs: 20, + sigtermGraceMs: 10, + }, + ); + + stopper.stop(); + expect(signals).toEqual(["SIGINT"]); + + await vi.advanceTimersByTimeAsync(10); + expect(signals).toEqual(["SIGINT", "SIGTERM"]); + + await vi.advanceTimersByTimeAsync(20); + expect(signals).toEqual(["SIGINT", "SIGTERM", "SIGKILL"]); + }); + + it.runIf(process.platform !== "win32")( + "signals the TUI PTY watch process group before falling back to the child", + () => { + const kill = vi.spyOn(process, "kill").mockReturnValue(true); + const childKill = vi.fn(() => true); + + try { + tuiPtyWatchTesting.signalChildProcessTree({ pid: 123, kill: childKill }, "SIGTERM"); + expect(kill).toHaveBeenCalledWith(-123, "SIGTERM"); + expect(childKill).not.toHaveBeenCalled(); + } finally { + kill.mockRestore(); + } + }, + ); + + it.runIf(process.platform !== "win32")( + "falls back to direct TUI PTY watch child signaling when the process group is gone", + () => { + const kill = vi.spyOn(process, "kill").mockImplementation(() => { + const error = new Error("missing process group") as NodeJS.ErrnoException; + error.code = "ESRCH"; + throw error; + }); + const childKill = vi.fn(() => true); + + try { + tuiPtyWatchTesting.signalChildProcessTree({ pid: 123, kill: childKill }, "SIGTERM"); + expect(kill).toHaveBeenCalledWith(-123, "SIGTERM"); + expect(childKill).toHaveBeenCalledWith("SIGTERM"); + } finally { + kill.mockRestore(); + } + }, + ); + it("aborts stalled OpenAI realtime smoke fetches at the request timeout", async () => { let signal: AbortSignal | undefined; const request = realtimeSmokeTesting.createOpenAIClientSecret("test-key", {