fix(scripts): wait for benchmark process groups

scripts/lib/gateway-bench-child.ts

@@ -3,6 +3,7 @@ import type { ChildProcessWithoutNullStreams } from "node:child_process";
 
 const TEARDOWN_GRACE_MS = 2_000;
 const TEARDOWN_KILL_GRACE_MS = 1_000;
+const EXIT_POLL_MS = 10;
 
 export type ChildExit = {
   exitCode: number | null;
@@ -23,43 +24,95 @@ export async function stopChild(
   child: ChildProcessWithoutNullStreams,
   options: { killGraceMs?: number; teardownGraceMs?: number } = {},
 ): Promise<StopChildResult> {
-  const currentExit = (): ChildExit | null =>
-    child.exitCode != null || child.signalCode != null
+  const teardownGraceMs = options.teardownGraceMs ?? TEARDOWN_GRACE_MS;
+  const killGraceMs = options.killGraceMs ?? TEARDOWN_KILL_GRACE_MS;
+  let observedExit: ChildExit | null = null;
+  const directExit = (): ChildExit | null =>
+    observedExit ??
+    (child.exitCode != null || child.signalCode != null
       ? { exitCode: child.exitCode, signal: child.signalCode }
-      : null;
+      : null);
+  const currentExit = (): ChildExit | null => {
+    const exit = directExit();
+    if (exit == null || isProcessTreeAlive(child)) {
+      return null;
+    }
+    return exit;
+  };
+  const waitForProcessTreeExit = async (ms: number): Promise<boolean> => {
+    const deadlineAt = Date.now() + ms;
+    while (Date.now() < deadlineAt) {
+      if (!isProcessTreeAlive(child)) {
+        return true;
+      }
+      await delay(Math.min(EXIT_POLL_MS, deadlineAt - Date.now()));
+    }
+    return !isProcessTreeAlive(child);
+  };
+  const cleanupExitedProcessTree = async (
+    exit: ChildExit,
+    exitedBeforeTeardown: boolean,
+  ): Promise<StopChildResult> => {
+    if (!isProcessTreeAlive(child)) {
+      return { ...exit, exitedBeforeTeardown };
+    }
+    const sentTeardownSignal = killProcessTree(child, "SIGTERM");
+    if (sentTeardownSignal) {
+      await waitForProcessTreeExit(teardownGraceMs);
+    }
+    if (sentTeardownSignal && isProcessTreeAlive(child)) {
+      killProcessTree(child, "SIGKILL");
+      await waitForProcessTreeExit(killGraceMs);
+    }
+    if (!sentTeardownSignal) {
+      releaseUnsettledChild(child);
+    }
+    return { ...exit, exitedBeforeTeardown };
+  };
 
-  const existingExit = currentExit();
+  const existingExit = directExit();
   if (existingExit != null) {
-    return { ...existingExit, exitedBeforeTeardown: true };
+    return await cleanupExitedProcessTree(existingExit, true);
   }
 
-  let observedExit: ChildExit | null = null;
   const exited = new Promise<ChildExit>((resolve) => {
     child.once("exit", (exitCode, signal) => {
       observedExit = { exitCode, signal };
       resolve(observedExit);
     });
   });
-  const waitForExit = async (ms: number): Promise<ChildExit | null> =>
-    await Promise.race([exited, delay(ms).then(() => null)]);
+  const waitForExit = async (ms: number): Promise<ChildExit | null> => {
+    const deadlineAt = Date.now() + ms;
+    while (Date.now() < deadlineAt) {
+      const waitMs = Math.min(EXIT_POLL_MS, deadlineAt - Date.now());
+      if (directExit() == null) {
+        await Promise.race([exited, delay(waitMs)]);
+      } else {
+        await delay(waitMs);
+      }
+      const exit = currentExit();
+      if (exit != null) {
+        return exit;
+      }
+    }
+    return currentExit();
+  };
 
   await new Promise<void>((resolve) => {
     setImmediate(resolve);
   });
-  const queuedExit = observedExit ?? currentExit();
+  const queuedExit = directExit();
   if (queuedExit != null) {
-    return { ...queuedExit, exitedBeforeTeardown: true };
+    return await cleanupExitedProcessTree(queuedExit, true);
   }
 
-  const teardownGraceMs = options.teardownGraceMs ?? TEARDOWN_GRACE_MS;
-  const killGraceMs = options.killGraceMs ?? TEARDOWN_KILL_GRACE_MS;
   const sentTeardownSignal = killProcessTree(child, "SIGTERM");
   const gracefulExit = await waitForExit(teardownGraceMs);
   if (gracefulExit != null) {
     return { ...gracefulExit, exitedBeforeTeardown: !sentTeardownSignal };
   }
 
-  const postGraceExit = currentExit() ?? observedExit;
+  const postGraceExit = currentExit();
   if (postGraceExit != null) {
     return { ...postGraceExit, exitedBeforeTeardown: !sentTeardownSignal };
   }
@@ -70,7 +123,7 @@ export async function stopChild(
 
   killProcessTree(child, "SIGKILL");
   const killedExit = await waitForExit(killGraceMs);
-  const finalExit = killedExit ?? currentExit() ?? observedExit;
+  const finalExit = killedExit ?? currentExit();
   if (finalExit != null) {
     return { ...finalExit, exitedBeforeTeardown: false };
   }
@@ -86,6 +139,23 @@ function releaseUnsettledChild(child: ChildProcessWithoutNullStreams): void {
   child.unref();
 }
 
+function isProcessTreeAlive(child: ChildProcessWithoutNullStreams): boolean {
+  if (process.platform === "win32" || child.pid === undefined) {
+    return false;
+  }
+  try {
+    process.kill(-child.pid, 0);
+    return true;
+  } catch (error) {
+    return isProcessStillExistsError(error);
+  }
+}
+
+function isProcessStillExistsError(error: unknown): boolean {
+  const code = (error as { code?: unknown }).code;
+  return code === "EPERM";
+}
+
 function killProcessTree(child: ChildProcessWithoutNullStreams, signal: NodeJS.Signals): boolean {
   if (process.platform !== "win32" && child.pid !== undefined) {
     try {

scripts/package-openclaw-for-docker.mjs

@@ -14,6 +14,8 @@ const DEFAULT_PACKAGE_INVENTORY_TIMEOUT_MS = 5 * 60 * 1000;
 const DEFAULT_PACKAGE_PACK_TIMEOUT_MS = 5 * 60 * 1000;
 const DEFAULT_PACKAGE_TARBALL_CHECK_TIMEOUT_MS = 5 * 60 * 1000;
 const DEFAULT_TIMEOUT_KILL_AFTER_MS = 5_000;
+const PROCESS_GROUP_EXIT_POLL_MS = 25;
+const POST_FORCE_KILL_WAIT_MS = 1_000;
 const DEFAULT_CAPTURED_STDOUT_MAX_BYTES = 1024 * 1024;
 const ACTIVE_CHILD_KILLERS = new Set();
 const SIGNAL_EXIT_CODES = {
@@ -208,6 +210,18 @@ function run(command, args, cwd, options = {}) {
         return error?.code === "EPERM";
       }
     };
+    const waitForProcessGroupExit = async (timeoutMs) => {
+      const deadlineAt = Date.now() + timeoutMs;
+      while (Date.now() < deadlineAt) {
+        if (!processGroupAlive()) {
+          return true;
+        }
+        await new Promise((resolvePoll) => {
+          setTimeout(resolvePoll, PROCESS_GROUP_EXIT_POLL_MS);
+        });
+      }
+      return !processGroupAlive();
+    };
     const terminateChild = () => {
       killChild("SIGTERM");
       forceKillTimeout = setTimeout(() => {
@@ -228,6 +242,16 @@ function run(command, args, cwd, options = {}) {
             terminateChild();
           }, options.timeoutMs);
     timeout?.unref?.();
+    const finishAfterTeardown = async (error, value = "") => {
+      if (processGroupAlive()) {
+        await waitForProcessGroupExit(options.killAfterMs ?? DEFAULT_TIMEOUT_KILL_AFTER_MS);
+      }
+      if (processGroupAlive()) {
+        killChild("SIGKILL");
+        await waitForProcessGroupExit(POST_FORCE_KILL_WAIT_MS);
+      }
+      finish(error, value);
+    };
     if (options.captureStdout) {
       child.stdout.on("data", (chunk) => {
         if (outputLimitExceeded) {
@@ -250,11 +274,13 @@ function run(command, args, cwd, options = {}) {
     child.on("error", (error) => finish(error));
     child.on("close", (status, signal) => {
       if (timedOut) {
-        finish(new Error(`${command} ${args.join(" ")} timed out after ${options.timeoutMs}ms`));
+        void finishAfterTeardown(
+          new Error(`${command} ${args.join(" ")} timed out after ${options.timeoutMs}ms`),
+        );
         return;
       }
       if (outputLimitExceeded) {
-        finish(
+        void finishAfterTeardown(
           new Error(
             `${command} ${args.join(" ")} exceeded captured stdout limit (${maxCapturedStdoutBytes} bytes)`,
           ),

test/scripts/bench-gateway-child-test-support.ts

@@ -100,4 +100,133 @@ export function registerStopChildBehaviorTests<TChild>(params: {
     expect(child.stderr.destroy).toHaveBeenCalledOnce();
     expect(child.unref).toHaveBeenCalledOnce();
   });
+
+  it.skipIf(process.platform === "win32")(
+    "preserves pre-teardown wrapper exits while cleaning the process group",
+    async () => {
+      const child = new EventEmitter() as EventEmitter & {
+        exitCode: number | null;
+        kill: ReturnType<typeof vi.fn>;
+        pid: number;
+        signalCode: NodeJS.Signals | null;
+        stderr: { destroy: ReturnType<typeof vi.fn> };
+        stdin: { destroy: ReturnType<typeof vi.fn> };
+        stdout: { destroy: ReturnType<typeof vi.fn> };
+        unref: ReturnType<typeof vi.fn>;
+      };
+      child.exitCode = null;
+      child.kill = vi.fn(() => true);
+      child.pid = 4444;
+      child.signalCode = null;
+      child.stderr = { destroy: vi.fn() };
+      child.stdin = { destroy: vi.fn() };
+      child.stdout = { destroy: vi.fn() };
+      child.unref = vi.fn();
+
+      let processGroupAlive = true;
+      const processKill = vi.spyOn(process, "kill").mockImplementation((pid, signal) => {
+        expect(pid).toBe(-child.pid);
+        if (signal === "SIGKILL") {
+          processGroupAlive = false;
+          return true;
+        }
+        if (signal === 0 && !processGroupAlive) {
+          throw Object.assign(new Error("gone"), { code: "ESRCH" });
+        }
+        return true;
+      });
+      try {
+        const stopped = params.stopChild(child as unknown as TChild, {
+          killGraceMs: 50,
+          teardownGraceMs: 1,
+        });
+        queueMicrotask(() => {
+          child.exitCode = 0;
+          child.emit("exit", 0, null);
+        });
+        await expect(
+          stopped,
+        ).resolves.toEqual({
+          exitedBeforeTeardown: true,
+          exitCode: 0,
+          signal: null,
+        });
+        expect(processKill).toHaveBeenCalledWith(-child.pid, "SIGTERM");
+        expect(processKill).toHaveBeenCalledWith(-child.pid, "SIGKILL");
+        expect(child.kill).not.toHaveBeenCalled();
+        expect(child.stdin.destroy).not.toHaveBeenCalled();
+        expect(child.stdout.destroy).not.toHaveBeenCalled();
+        expect(child.stderr.destroy).not.toHaveBeenCalled();
+        expect(child.unref).not.toHaveBeenCalled();
+      } finally {
+        processKill.mockRestore();
+      }
+    },
+  );
+
+  it.skipIf(process.platform === "win32")(
+    "waits for the process group after a teardown-triggered wrapper exit",
+    async () => {
+      const child = new EventEmitter() as EventEmitter & {
+        exitCode: number | null;
+        kill: ReturnType<typeof vi.fn>;
+        pid: number;
+        signalCode: NodeJS.Signals | null;
+        stderr: { destroy: ReturnType<typeof vi.fn> };
+        stdin: { destroy: ReturnType<typeof vi.fn> };
+        stdout: { destroy: ReturnType<typeof vi.fn> };
+        unref: ReturnType<typeof vi.fn>;
+      };
+      child.exitCode = null;
+      child.kill = vi.fn(() => true);
+      child.pid = 4445;
+      child.signalCode = null;
+      child.stderr = { destroy: vi.fn() };
+      child.stdin = { destroy: vi.fn() };
+      child.stdout = { destroy: vi.fn() };
+      child.unref = vi.fn();
+
+      let emittedExit = false;
+      let processGroupAlive = true;
+      const processKill = vi.spyOn(process, "kill").mockImplementation((pid, signal) => {
+        expect(pid).toBe(-child.pid);
+        if (signal === "SIGTERM" && !emittedExit) {
+          emittedExit = true;
+          queueMicrotask(() => {
+            child.exitCode = 0;
+            child.emit("exit", 0, null);
+          });
+        }
+        if (signal === "SIGKILL") {
+          processGroupAlive = false;
+          return true;
+        }
+        if (signal === 0 && !processGroupAlive) {
+          throw Object.assign(new Error("gone"), { code: "ESRCH" });
+        }
+        return true;
+      });
+      try {
+        await expect(
+          params.stopChild(child as unknown as TChild, {
+            killGraceMs: 50,
+            teardownGraceMs: 1,
+          }),
+        ).resolves.toEqual({
+          exitedBeforeTeardown: false,
+          exitCode: 0,
+          signal: null,
+        });
+        expect(processKill).toHaveBeenCalledWith(-child.pid, "SIGTERM");
+        expect(processKill).toHaveBeenCalledWith(-child.pid, "SIGKILL");
+        expect(child.kill).not.toHaveBeenCalled();
+        expect(child.stdin.destroy).not.toHaveBeenCalled();
+        expect(child.stdout.destroy).not.toHaveBeenCalled();
+        expect(child.stderr.destroy).not.toHaveBeenCalled();
+        expect(child.unref).not.toHaveBeenCalled();
+      } finally {
+        processKill.mockRestore();
+      }
+    },
+  );
 }