Skip to content

Commit ee48028

Browse files
vincentkocvincentkoc
committed
fix(dev): clean tui pty watch children
1 parent 3c32459 commit ee48028

2 files changed

Lines changed: 181 additions & 15 deletions

File tree

‎scripts/dev/tui-pty-test-watch.ts‎

Lines changed: 115 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { spawn } from "node:child_process";
22
import { mkdir, readFile, writeFile } from "node:fs/promises";
33
import { createRequire } from "node:module";
44
import path from "node:path";
5+
import { pathToFileURL } from "node:url";
56

67
type Options = {
78
altScreen: boolean;
@@ -20,6 +21,24 @@ const MODE_TEST_FILES = {
2021
const MIRROR_TERMINAL_QUERIES = ["\x1b[?u", "\x1b[16t"];
2122
const DEFAULT_PTY_COLS = 100;
2223
const DEFAULT_PTY_ROWS = 30;
24+
const CHILD_SIGTERM_GRACE_MS = 500;
25+
const CHILD_SIGKILL_GRACE_MS = 5_000;
26+
27+
type KillableChild = {
28+
pid?: number;
29+
kill(signal: NodeJS.Signals): boolean;
30+
};
31+
32+
type ChildStopper = {
33+
cancel: () => void;
34+
stop: () => void;
35+
};
36+
37+
type SignalChild = (child: KillableChild, signal: NodeJS.Signals) => void;
38+
39+
function unrefTimer(timer: ReturnType<typeof setTimeout>): void {
40+
(timer as { unref?: () => void }).unref?.();
41+
}
2342

2443
function readOption(args: string[], name: string): string | undefined {
2544
const idx = args.indexOf(name);
@@ -72,6 +91,64 @@ function currentTerminalDimension(value: number | undefined, fallback: number):
7291
return String(value && value > 0 ? value : fallback);
7392
}
7493

94+
function signalChildProcessTree(child: KillableChild, signal: NodeJS.Signals): void {
95+
if (process.platform !== "win32" && typeof child.pid === "number") {
96+
try {
97+
process.kill(-child.pid, signal);
98+
return;
99+
} catch {
100+
// Non-detached fallback or already-exited group; direct child signaling is
101+
// still useful on platforms without process groups.
102+
}
103+
}
104+
child.kill(signal);
105+
}
106+
107+
function createChildStopper(
108+
child: KillableChild,
109+
options: {
110+
signalChild?: SignalChild;
111+
sigtermGraceMs?: number;
112+
sigkillGraceMs?: number;
113+
} = {},
114+
): ChildStopper {
115+
const signalChild = options.signalChild ?? signalChildProcessTree;
116+
const sigtermGraceMs = options.sigtermGraceMs ?? CHILD_SIGTERM_GRACE_MS;
117+
const sigkillGraceMs = options.sigkillGraceMs ?? CHILD_SIGKILL_GRACE_MS;
118+
let stopping = false;
119+
let termTimer: ReturnType<typeof setTimeout> | undefined;
120+
let killTimer: ReturnType<typeof setTimeout> | undefined;
121+
122+
const cancel = () => {
123+
if (termTimer) {
124+
clearTimeout(termTimer);
125+
termTimer = undefined;
126+
}
127+
if (killTimer) {
128+
clearTimeout(killTimer);
129+
killTimer = undefined;
130+
}
131+
};
132+
133+
const stop = () => {
134+
if (stopping) {
135+
return;
136+
}
137+
stopping = true;
138+
signalChild(child, "SIGINT");
139+
termTimer = setTimeout(() => {
140+
signalChild(child, "SIGTERM");
141+
killTimer = setTimeout(() => {
142+
signalChild(child, "SIGKILL");
143+
}, sigkillGraceMs);
144+
unrefTimer(killTimer);
145+
}, sigtermGraceMs);
146+
unrefTimer(termTimer);
147+
};
148+
149+
return { cancel, stop };
150+
}
151+
75152
async function createMirrorFile(mirrorPath: string): Promise<void> {
76153
await mkdir(path.dirname(mirrorPath), { recursive: true });
77154
await writeFile(mirrorPath, "", "utf8");
@@ -108,6 +185,7 @@ async function main(): Promise<void> {
108185
],
109186
{
110187
cwd: process.cwd(),
188+
detached: process.platform !== "win32",
111189
env: {
112190
...process.env,
113191
OPENCLAW_TUI_PTY_MIRROR_PATH: options.mirrorPath,
@@ -172,10 +250,8 @@ async function main(): Promise<void> {
172250
}
173251
};
174252

175-
const stopChild = () => {
176-
child.kill("SIGINT");
177-
setTimeout(() => child.kill("SIGTERM"), 500).unref();
178-
};
253+
const childStopper = createChildStopper(child);
254+
const stopChild = childStopper.stop;
179255

180256
const ignoredInput = (chunk: Buffer) => {
181257
if (chunk.includes(0x03)) {
@@ -238,12 +314,20 @@ async function main(): Promise<void> {
238314
childStderr += chunk.toString("utf8");
239315
});
240316

241-
let childExit: { code: number | null; signal: NodeJS.Signals | null } | null = null;
242-
child.on("exit", (code, signal) => {
243-
childExit = { code, signal };
317+
type ChildExit = { code: number | null; signal: NodeJS.Signals | null };
318+
let childExit: ChildExit | null = null;
319+
const childFinished = new Promise<ChildExit>((resolve) => {
320+
child.once("exit", (code, signal) => {
321+
childExit = { code, signal };
322+
childStopper.cancel();
323+
resolve(childExit);
324+
});
244325
});
245326

246-
process.once("SIGINT", stopChild);
327+
const parentSignals: NodeJS.Signals[] = ["SIGINT", "SIGTERM", "SIGHUP"];
328+
for (const signal of parentSignals) {
329+
process.once(signal, stopChild);
330+
}
247331

248332
try {
249333
for (;;) {
@@ -265,6 +349,12 @@ async function main(): Promise<void> {
265349
writeMirrorChunk(result.chunk);
266350
}
267351
} finally {
352+
if (!childExit) {
353+
stopChild();
354+
}
355+
for (const signal of parentSignals) {
356+
process.off(signal, stopChild);
357+
}
268358
await drainParentInput();
269359
restoreInput();
270360
if (useAltScreen) {
@@ -273,6 +363,10 @@ async function main(): Promise<void> {
273363
restoreScreen();
274364
}
275365

366+
if (!childExit) {
367+
childExit = await childFinished;
368+
}
369+
276370
if (childStdout) {
277371
process.stdout.write(childStdout);
278372
}
@@ -288,9 +382,16 @@ async function main(): Promise<void> {
288382
}
289383
}
290384

291-
main().catch((error: unknown) => {
292-
process.stderr.write(
293-
`${error instanceof Error ? error.stack || error.message : String(error)}\n`,
294-
);
295-
process.exit(1);
296-
});
385+
if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) {
386+
main().catch((error: unknown) => {
387+
process.stderr.write(
388+
`${error instanceof Error ? error.stack || error.message : String(error)}\n`,
389+
);
390+
process.exit(1);
391+
});
392+
}
393+
394+
export const testing = {
395+
createChildStopper,
396+
signalChildProcessTree,
397+
};

‎test/scripts/dev-tooling-safety.test.ts‎

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,12 @@ import { EventEmitter } from "node:events";
22
import fs from "node:fs/promises";
33
import os from "node:os";
44
import path from "node:path";
5-
import { describe, expect, it } from "vitest";
5+
import { afterEach, describe, expect, it, vi } from "vitest";
66
import { testing as promptProbeTesting } from "../../scripts/anthropic-prompt-probe.ts";
77
import { testing as claudeUsageTesting } from "../../scripts/debug-claude-usage.ts";
88
import { testing as discordSmokeTesting } from "../../scripts/dev/discord-acp-plain-language-smoke.ts";
99
import { testing as realtimeSmokeTesting } from "../../scripts/dev/realtime-talk-live-smoke.ts";
10+
import { testing as tuiPtyWatchTesting } from "../../scripts/dev/tui-pty-test-watch.ts";
1011
import {
1112
maskIdentifier,
1213
parseBooleanEnv,
@@ -16,6 +17,10 @@ import {
1617
redactJsonValueForDevToolLog,
1718
} from "../../scripts/lib/dev-tooling-safety.ts";
1819

20+
afterEach(() => {
21+
vi.useRealTimers();
22+
});
23+
1924
describe("dev tooling safety helpers", () => {
2025
it("redacts secrets before truncating script log previews", () => {
2126
const token = "sk-test1234567890abcdefghijklmnop"; // pragma: allowlist secret
@@ -193,6 +198,66 @@ describe("script-specific dev tooling hardening", () => {
193198
expect(calls).toBe(1);
194199
});
195200

201+
it("escalates stalled TUI PTY watch children after interrupt cleanup", async () => {
202+
vi.useFakeTimers();
203+
const signals: NodeJS.Signals[] = [];
204+
const stopper = tuiPtyWatchTesting.createChildStopper(
205+
{ kill: () => true },
206+
{
207+
signalChild(_child, signal: NodeJS.Signals): void {
208+
signals.push(signal);
209+
},
210+
sigkillGraceMs: 20,
211+
sigtermGraceMs: 10,
212+
},
213+
);
214+
215+
stopper.stop();
216+
expect(signals).toEqual(["SIGINT"]);
217+
218+
await vi.advanceTimersByTimeAsync(10);
219+
expect(signals).toEqual(["SIGINT", "SIGTERM"]);
220+
221+
await vi.advanceTimersByTimeAsync(20);
222+
expect(signals).toEqual(["SIGINT", "SIGTERM", "SIGKILL"]);
223+
});
224+
225+
it.runIf(process.platform !== "win32")(
226+
"signals the TUI PTY watch process group before falling back to the child",
227+
() => {
228+
const kill = vi.spyOn(process, "kill").mockReturnValue(true);
229+
const childKill = vi.fn(() => true);
230+
231+
try {
232+
tuiPtyWatchTesting.signalChildProcessTree({ pid: 123, kill: childKill }, "SIGTERM");
233+
expect(kill).toHaveBeenCalledWith(-123, "SIGTERM");
234+
expect(childKill).not.toHaveBeenCalled();
235+
} finally {
236+
kill.mockRestore();
237+
}
238+
},
239+
);
240+
241+
it.runIf(process.platform !== "win32")(
242+
"falls back to direct TUI PTY watch child signaling when the process group is gone",
243+
() => {
244+
const kill = vi.spyOn(process, "kill").mockImplementation(() => {
245+
const error = new Error("missing process group") as NodeJS.ErrnoException;
246+
error.code = "ESRCH";
247+
throw error;
248+
});
249+
const childKill = vi.fn(() => true);
250+
251+
try {
252+
tuiPtyWatchTesting.signalChildProcessTree({ pid: 123, kill: childKill }, "SIGTERM");
253+
expect(kill).toHaveBeenCalledWith(-123, "SIGTERM");
254+
expect(childKill).toHaveBeenCalledWith("SIGTERM");
255+
} finally {
256+
kill.mockRestore();
257+
}
258+
},
259+
);
260+
196261
it("aborts stalled OpenAI realtime smoke fetches at the request timeout", async () => {
197262
let signal: AbortSignal | undefined;
198263
const request = realtimeSmokeTesting.createOpenAIClientSecret("test-key", {

0 commit comments

Comments
 (0)