Skip to content

Commit 602bc0b

Browse files
vincentkocvincentkoc
committed
fix(bench): clean timed-out sample process groups
1 parent a1d278b commit 602bc0b

2 files changed

Lines changed: 184 additions & 22 deletions

File tree

‎scripts/bench-cli-startup.ts‎

Lines changed: 110 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ type CliOptions = {
105105
const DEFAULT_RUNS = 5;
106106
const DEFAULT_WARMUP = 1;
107107
const DEFAULT_TIMEOUT_MS = 30_000;
108+
const TIMEOUT_KILL_GRACE_MS = 1_000;
109+
const PROCESS_GROUP_EXIT_POLL_MS = 25;
108110
const DEFAULT_ENTRY = "openclaw.mjs";
109111
const MAX_RSS_MARKER = "__OPENCLAW_MAX_RSS_KB__=";
110112
const VALUE_FLAGS = new Set([
@@ -708,12 +710,16 @@ async function runSample(params: {
708710
let stderr = "";
709711
let settled = false;
710712
let timedOut = false;
713+
let forceKillAt: number | null = null;
714+
let forceKillTimer: ReturnType<typeof setTimeout> | null = null;
711715
const maxOutputLength = 32 * 1024 * 1024;
712716

713717
try {
714718
return await new Promise<Sample>((resolve) => {
719+
const useProcessGroup = process.platform !== "win32";
715720
const proc = spawn(process.execPath, nodeArgs, {
716721
cwd: process.cwd(),
722+
detached: useProcessGroup,
717723
env: {
718724
...process.env,
719725
HOME: runRoot,
@@ -733,6 +739,10 @@ async function runSample(params: {
733739
return;
734740
}
735741
settled = true;
742+
if (forceKillTimer) {
743+
clearTimeout(forceKillTimer);
744+
forceKillTimer = null;
745+
}
736746
const ms = Number(process.hrtime.bigint() - started) / 1e6;
737747
resolve({
738748
ms,
@@ -751,18 +761,11 @@ async function runSample(params: {
751761

752762
const timeout = setTimeout(() => {
753763
timedOut = true;
754-
try {
755-
proc.kill("SIGTERM");
756-
} catch {
757-
// Best-effort timeout cleanup.
758-
}
759-
setTimeout(() => {
760-
try {
761-
proc.kill("SIGKILL");
762-
} catch {
763-
// Best-effort timeout cleanup.
764-
}
765-
}, 1_000).unref?.();
764+
signalSampleProcess(proc, "SIGTERM", useProcessGroup);
765+
forceKillAt = Date.now() + TIMEOUT_KILL_GRACE_MS;
766+
forceKillTimer = setTimeout(() => {
767+
signalSampleProcess(proc, "SIGKILL", useProcessGroup);
768+
}, TIMEOUT_KILL_GRACE_MS).unref?.();
766769
}, params.timeoutMs);
767770
timeout.unref?.();
768771

@@ -790,23 +793,108 @@ async function runSample(params: {
790793
});
791794
proc.once("close", (code, signal) => {
792795
clearTimeout(timeout);
793-
finish({
794-
exitCode: code,
795-
signal,
796-
...(code === 0 && signal == null
797-
? {}
798-
: {
799-
stdoutTail: tailLines(stdout, 20),
800-
stderrTail: tailLines(stderr, 20),
801-
}),
802-
});
796+
const complete = () =>
797+
finish({
798+
exitCode: code,
799+
signal,
800+
...(code === 0 && signal == null
801+
? {}
802+
: {
803+
stdoutTail: tailLines(stdout, 20),
804+
stderrTail: tailLines(stderr, 20),
805+
}),
806+
});
807+
if (timedOut && isSampleProcessGroupAlive(proc, useProcessGroup)) {
808+
void finishAfterTimeoutCleanup({
809+
complete,
810+
forceKillAt,
811+
proc,
812+
useProcessGroup,
813+
});
814+
return;
815+
}
816+
complete();
803817
});
804818
});
805819
} finally {
806820
rmSync(runRoot, { recursive: true, force: true });
807821
}
808822
}
809823

824+
async function finishAfterTimeoutCleanup(params: {
825+
complete: () => void;
826+
forceKillAt: number | null;
827+
proc: ReturnType<typeof spawn>;
828+
useProcessGroup: boolean;
829+
}): Promise<void> {
830+
const graceRemainingMs =
831+
params.forceKillAt === null
832+
? TIMEOUT_KILL_GRACE_MS
833+
: Math.max(0, params.forceKillAt - Date.now());
834+
if (graceRemainingMs > 0) {
835+
await waitForSampleProcessGroupExit(params.proc, params.useProcessGroup, graceRemainingMs);
836+
}
837+
if (isSampleProcessGroupAlive(params.proc, params.useProcessGroup)) {
838+
signalSampleProcess(params.proc, "SIGKILL", params.useProcessGroup);
839+
}
840+
await waitForSampleProcessGroupExit(params.proc, params.useProcessGroup, TIMEOUT_KILL_GRACE_MS);
841+
params.complete();
842+
}
843+
844+
function signalSampleProcess(
845+
proc: ReturnType<typeof spawn>,
846+
signal: NodeJS.Signals,
847+
useProcessGroup: boolean,
848+
): void {
849+
if (!proc.pid) {
850+
return;
851+
}
852+
try {
853+
if (useProcessGroup) {
854+
process.kill(-proc.pid, signal);
855+
} else {
856+
proc.kill(signal);
857+
}
858+
} catch (error) {
859+
const code = (error as NodeJS.ErrnoException | undefined)?.code;
860+
if (code !== "ESRCH" && code !== "EPERM") {
861+
throw error;
862+
}
863+
}
864+
}
865+
866+
function isSampleProcessGroupAlive(
867+
proc: ReturnType<typeof spawn>,
868+
useProcessGroup: boolean,
869+
): boolean {
870+
if (!useProcessGroup || !proc.pid) {
871+
return false;
872+
}
873+
try {
874+
process.kill(-proc.pid, 0);
875+
return true;
876+
} catch (error) {
877+
return (error as NodeJS.ErrnoException | undefined)?.code === "EPERM";
878+
}
879+
}
880+
881+
async function waitForSampleProcessGroupExit(
882+
proc: ReturnType<typeof spawn>,
883+
useProcessGroup: boolean,
884+
timeoutMs: number,
885+
): Promise<boolean> {
886+
const deadlineAt = Date.now() + timeoutMs;
887+
while (Date.now() < deadlineAt) {
888+
if (!isSampleProcessGroupAlive(proc, useProcessGroup)) {
889+
return true;
890+
}
891+
await new Promise((resolvePoll) => {
892+
setTimeout(resolvePoll, PROCESS_GROUP_EXIT_POLL_MS);
893+
});
894+
}
895+
return !isSampleProcessGroupAlive(proc, useProcessGroup);
896+
}
897+
810898
async function runCase(params: {
811899
entry: string;
812900
commandCase: CommandCase;

‎test/scripts/bench-cli-startup.test.ts‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,15 @@ function withEnv<T>(env: Record<string, string | undefined>, callback: () => T):
2929
}
3030
}
3131

32+
function isProcessAlive(pid: number): boolean {
33+
try {
34+
process.kill(pid, 0);
35+
return true;
36+
} catch {
37+
return false;
38+
}
39+
}
40+
3241
describe("bench-cli-startup", () => {
3342
it("rejects unknown CLI options before running benchmarks", () => {
3443
expect(() => testing.validateCliArgs(["--wat"])).toThrow("Unknown argument: --wat");
@@ -49,6 +58,71 @@ describe("bench-cli-startup", () => {
4958
expect(result.stderr).not.toContain("\n at ");
5059
});
5160

61+
it.runIf(process.platform !== "win32")(
62+
"cleans timed-out benchmark process groups when the leader exits first",
63+
() => {
64+
const tempDirs = createTempDirTracker();
65+
const tmpDir = tempDirs.make("openclaw-cli-startup-timeout-group-");
66+
const entryPath = join(tmpDir, "entry.mjs");
67+
const childPidPath = join(tmpDir, "child.pid");
68+
let childPid = 0;
69+
try {
70+
writeFileSync(
71+
entryPath,
72+
[
73+
"import { spawn } from 'node:child_process';",
74+
"import { writeFileSync } from 'node:fs';",
75+
"process.on('SIGTERM', () => process.exit(0));",
76+
"const child = spawn(process.execPath, [",
77+
" '-e',",
78+
" \"process.on('SIGTERM',()=>{});setInterval(()=>{},1000);\",",
79+
"], { stdio: 'ignore' });",
80+
`writeFileSync(${JSON.stringify(childPidPath)}, String(child.pid));`,
81+
"setInterval(() => {}, 1000);",
82+
"",
83+
].join("\n"),
84+
"utf8",
85+
);
86+
87+
const result = spawnSync(
88+
process.execPath,
89+
[
90+
"--import",
91+
"tsx",
92+
"scripts/bench-cli-startup.ts",
93+
"--entry",
94+
entryPath,
95+
"--case",
96+
"version",
97+
"--runs",
98+
"1",
99+
"--warmup",
100+
"0",
101+
"--timeout-ms",
102+
"100",
103+
"--json",
104+
],
105+
{
106+
cwd: join(__dirname, "../.."),
107+
encoding: "utf8",
108+
timeout: 8_000,
109+
},
110+
);
111+
112+
childPid = Number(readFileSync(childPidPath, "utf8"));
113+
expect(result.status).toBe(1);
114+
expect(result.signal).toBeNull();
115+
expect(result.stderr).toContain("version sample 1: timed out");
116+
expect(isProcessAlive(childPid)).toBe(false);
117+
} finally {
118+
if (childPid && isProcessAlive(childPid)) {
119+
process.kill(childPid, "SIGKILL");
120+
}
121+
tempDirs.cleanup();
122+
}
123+
},
124+
);
125+
52126
it("writes compare-mode JSON output and creates parent directories", () => {
53127
const tempDirs = createTempDirTracker();
54128
const tmpDir = tempDirs.make("openclaw-cli-startup-compare-output-");

0 commit comments

Comments
 (0)