fix(memory): bound qmd taskkill cleanup (#109265)

* fix(memory): bound qmd taskkill cleanup * test(memory): satisfy qmd process lint
2026-07-21 02:06:43 +00:00 · 2026-07-18 06:11:14 -07:00
parent 1cddec35f4
commit ad3b8e0b6f
2 changed files with 216 additions and 33 deletions
@@ -17,14 +17,12 @@ import {
 import { MAX_SAFE_TIMEOUT_DELAY_MS } from "../../../gateway-client/src/timeouts.js";

 const spawnMock = vi.hoisted(() => vi.fn());
-const spawnSyncMock = vi.hoisted(() => vi.fn());

 vi.mock("node:child_process", async () => {
  const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
  return {
    ...actual,
    spawn: spawnMock,
-    spawnSync: spawnSyncMock,
  };
 });

@@ -41,19 +39,33 @@ function createMockChild(params: { pid?: number } = {}) {
    pid?: number;
    stdout: EventEmitter & { setEncoding: ReturnType<typeof vi.fn> };
    stderr: EventEmitter & { setEncoding: ReturnType<typeof vi.fn> };
+    exitCode: number | null;
+    signalCode: NodeJS.Signals | null;
    kill: ReturnType<typeof vi.fn>;
+    unref: ReturnType<typeof vi.fn>;
    closeWith: (code?: number | null, signal?: NodeJS.Signals | null) => void;
  };
  child.pid = params.pid;
  child.stdout = Object.assign(new EventEmitter(), { setEncoding: vi.fn() });
  child.stderr = Object.assign(new EventEmitter(), { setEncoding: vi.fn() });
+  child.exitCode = null;
+  child.signalCode = null;
  child.kill = vi.fn();
+  child.unref = vi.fn();
  child.closeWith = (code: number | null = 0, signal: NodeJS.Signals | null = null) => {
+    child.exitCode = code;
+    child.signalCode = signal;
    child.emit("close", code, signal);
  };
  return child;
 }

+function createClosingTaskkillChild(code = 0) {
+  const child = createMockChild();
+  queueMicrotask(() => child.closeWith(code));
+  return child;
+}
+
 let fixtureRoot = "";
 let tempDir = "";
 let platformSpy: MockInstance<() => NodeJS.Platform> | null = null;
@@ -90,6 +102,8 @@ beforeEach(async () => {
  await fs.mkdir(tempDir, { recursive: true });
  process.env.SystemRoot = "C:\\Windows";
  delete process.env.WINDIR;
+  spawnMock.mockReset();
+  spawnMock.mockImplementation(() => createClosingTaskkillChild());
 });

 afterEach(() => {
@@ -100,8 +114,6 @@ afterEach(() => {
  restoreEnvValue("WINDIR", originalWindir);
  platformSpy?.mockReturnValue("win32");
  spawnMock.mockReset();
-  spawnSyncMock.mockReset();
-  spawnSyncMock.mockReturnValue({ status: 0 });
  tempDir = "";
 });

@@ -191,7 +203,7 @@ describe("checkQmdBinaryAvailability", () => {
    await expect(
      checkQmdBinaryAvailability({ command: "qmd", env: process.env, cwd: tempDir }),
    ).resolves.toEqual({ available: true });
-    expect(spawnSyncMock).toHaveBeenCalledWith(taskkillPath, ["/PID", String(child.pid), "/T"], {
+    expect(spawnMock).toHaveBeenNthCalledWith(2, taskkillPath, ["/PID", String(child.pid), "/T"], {
      stdio: "ignore",
      windowsHide: true,
    });
@@ -204,21 +216,100 @@ describe("checkQmdBinaryAvailability", () => {
      queueMicrotask(() => child.emit("spawn"));
      return child;
    });
-    spawnSyncMock.mockReset();
-    spawnSyncMock.mockReturnValueOnce({ status: 1 }).mockReturnValueOnce({ status: 0 });
+    spawnMock.mockImplementationOnce(() => createClosingTaskkillChild(1));
+
+    await expect(
+      checkQmdBinaryAvailability({ command: "qmd", env: process.env, cwd: tempDir }),
+    ).resolves.toEqual({ available: true });
+    await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(3));
+
+    expect(spawnMock).toHaveBeenNthCalledWith(2, taskkillPath, ["/PID", "12345", "/T"], {
+      stdio: "ignore",
+      windowsHide: true,
+    });
+    expect(spawnMock).toHaveBeenNthCalledWith(3, taskkillPath, ["/PID", "12345", "/T", "/F"], {
+      stdio: "ignore",
+      windowsHide: true,
+    });
+    expect(child.kill).not.toHaveBeenCalled();
+  });
+
+  it("keeps the event loop responsive and does not retry a pid after taskkill times out", async () => {
+    vi.useFakeTimers();
+    const child = createMockChild({ pid: 12346 });
+    const taskkill = createMockChild();
+    spawnMock.mockImplementationOnce(() => {
+      queueMicrotask(() => child.emit("spawn"));
+      return child;
+    });
+    spawnMock.mockReturnValueOnce(taskkill);

    await expect(
      checkQmdBinaryAvailability({ command: "qmd", env: process.env, cwd: tempDir }),
    ).resolves.toEqual({ available: true });

-    expect(spawnSyncMock).toHaveBeenNthCalledWith(1, taskkillPath, ["/PID", "12345", "/T"], {
+    const heartbeat = vi.fn();
+    setTimeout(heartbeat, 1);
+    await vi.advanceTimersByTimeAsync(1);
+    expect(heartbeat).toHaveBeenCalledOnce();
+    expect(taskkill.kill).not.toHaveBeenCalled();
+    expect(taskkill.unref).not.toHaveBeenCalled();
+
+    await vi.advanceTimersByTimeAsync(4_999);
+    expect(spawnMock).toHaveBeenCalledTimes(2);
+    expect(spawnMock).toHaveBeenNthCalledWith(2, taskkillPath, ["/PID", "12346", "/T"], {
      stdio: "ignore",
      windowsHide: true,
    });
-    expect(spawnSyncMock).toHaveBeenNthCalledWith(2, taskkillPath, ["/PID", "12345", "/T", "/F"], {
-      stdio: "ignore",
-      windowsHide: true,
+    expect(taskkill.kill).toHaveBeenCalledWith("SIGKILL");
+    expect(taskkill.unref).toHaveBeenCalledOnce();
+    expect(child.kill).toHaveBeenCalledWith();
+  });
+
+  it("keeps a timed-out taskkill result when kill synchronously emits an error", async () => {
+    vi.useFakeTimers();
+    const child = createMockChild({ pid: 12347 });
+    const taskkill = createMockChild();
+    taskkill.kill.mockImplementationOnce(() => {
+      taskkill.emit("error", new Error("taskkill kill failed"));
+      return true;
    });
+    spawnMock.mockImplementationOnce(() => {
+      queueMicrotask(() => child.emit("spawn"));
+      return child;
+    });
+    spawnMock.mockReturnValueOnce(taskkill);
+
+    await expect(
+      checkQmdBinaryAvailability({ command: "qmd", env: process.env, cwd: tempDir }),
+    ).resolves.toEqual({ available: true });
+
+    await vi.advanceTimersByTimeAsync(5_000);
+    expect(spawnMock).toHaveBeenCalledTimes(2);
+    expect(taskkill.kill).toHaveBeenCalledWith("SIGKILL");
+    expect(child.kill).toHaveBeenCalledWith();
+  });
+
+  it("does not retry taskkill after the qmd child exits", async () => {
+    const child = createMockChild({ pid: 12348 });
+    const taskkill = createMockChild();
+    spawnMock.mockImplementationOnce(() => {
+      queueMicrotask(() => child.emit("spawn"));
+      return child;
+    });
+    spawnMock.mockReturnValueOnce(taskkill);
+
+    await expect(
+      checkQmdBinaryAvailability({ command: "qmd", env: process.env, cwd: tempDir }),
+    ).resolves.toEqual({ available: true });
+
+    child.closeWith(0);
+    taskkill.closeWith(1);
+    await new Promise<void>((resolve) => {
+      setImmediate(resolve);
+    });
+
+    expect(spawnMock).toHaveBeenCalledTimes(2);
    expect(child.kill).not.toHaveBeenCalled();
  });

@@ -252,7 +343,7 @@ describe("checkQmdBinaryAvailability", () => {
    const child = createMockChild();
    const err = Object.assign(new Error("spawn qmd ENOENT"), { code: "ENOENT" });
    spawnMock.mockImplementationOnce(() => {
-      queueMicrotask(() => child.emit("close"));
+      queueMicrotask(() => child.closeWith());
      queueMicrotask(() => child.emit("error", err));
      return child;
    });
@@ -520,13 +611,44 @@ describe("runCliCommand", () => {

    await expect(pending).rejects.toThrow("qmd query test timed out after 1ms");

-    expect(spawnSyncMock).toHaveBeenCalledWith(taskkillPath, ["/PID", "12346", "/T", "/F"], {
+    expect(spawnMock).toHaveBeenNthCalledWith(2, taskkillPath, ["/PID", "12346", "/T", "/F"], {
      stdio: "ignore",
      windowsHide: true,
    });
    expect(child.kill).not.toHaveBeenCalledWith("SIGKILL");
  });

+  it("falls back to direct SIGKILL when Windows taskkill times out", async () => {
+    vi.useFakeTimers();
+    const child = createMockChild({ pid: 12347 });
+    const taskkill = createMockChild();
+    spawnMock.mockReturnValueOnce(child);
+    spawnMock.mockReturnValueOnce(taskkill);
+
+    const pending = runCliCommand({
+      commandSummary: "qmd query test",
+      spawnInvocation: { command: "qmd", argv: ["query", "test", "--json"] },
+      env: process.env,
+      cwd: tempDir,
+      maxOutputChars: 10_000,
+      timeoutMs: 1,
+    });
+
+    const timeoutAssertion = expect(pending).rejects.toThrow("qmd query test timed out after 1ms");
+    await vi.advanceTimersByTimeAsync(1);
+    await timeoutAssertion;
+    expect(child.kill).not.toHaveBeenCalledWith("SIGKILL");
+
+    await vi.advanceTimersByTimeAsync(5_000);
+    expect(spawnMock).toHaveBeenNthCalledWith(2, taskkillPath, ["/PID", "12347", "/T", "/F"], {
+      stdio: "ignore",
+      windowsHide: true,
+    });
+    expect(taskkill.kill).toHaveBeenCalledWith("SIGKILL");
+    expect(taskkill.unref).toHaveBeenCalledOnce();
+    expect(child.kill).toHaveBeenCalledWith("SIGKILL");
+  });
+
  it.each(["stdout", "stderr"] as const)(
    "rejects when %s stream emits an error",
    async (streamName) => {
@@ -1,5 +1,5 @@
 // Memory Host SDK module implements qmd process behavior.
-import { spawn, spawnSync } from "node:child_process";
+import { spawn } from "node:child_process";
 import { statSync } from "node:fs";
 import path from "node:path";
 import { resolveSafeTimeoutDelayMs } from "../../../gateway-client/src/timeouts.js";
@@ -14,10 +14,15 @@ type CliSpawnInvocation = {

 type QmdChildProcess = {
  pid?: number;
+  exitCode: number | null;
+  signalCode: NodeJS.Signals | null;
  kill: (signal?: NodeJS.Signals) => boolean;
 };

+type WindowsTaskkillResult = "success" | "failure" | "timed-out";
+
 const DEFAULT_WINDOWS_SYSTEM_ROOT = "C:\\Windows";
+const WINDOWS_TASKKILL_TIMEOUT_MS = 5_000;

 export type QmdBinaryUnavailableReason = "binary" | "workspace-cwd";

@@ -104,7 +109,7 @@ export async function checkQmdBinaryAvailability(params: {
    });
    const timeoutMs = resolveSafeTimeoutDelayMs(params.timeoutMs ?? 2_000, { minMs: 0 });
    const timer = setTimeout(() => {
-      signalQmdProcessTree(child, "SIGKILL");
+      void signalQmdProcessTree(child, "SIGKILL");
      finish({
        available: false,
        reason: "binary",
@@ -117,7 +122,7 @@ export async function checkQmdBinaryAvailability(params: {
    });
    child.once("spawn", () => {
      didSpawn = true;
-      signalQmdProcessTree(child);
+      void signalQmdProcessTree(child);
      finish({ available: true });
    });
    child.once("close", () => {
@@ -217,14 +222,14 @@ export async function runCliCommand(params: {
      params.timeoutMs === undefined ? undefined : resolveSafeTimeoutDelayMs(params.timeoutMs);
    const timer = timeoutMs
      ? setTimeout(() => {
-          signalQmdProcessTree(child, "SIGKILL");
+          void signalQmdProcessTree(child, "SIGKILL");
          settle(() =>
            reject(new Error(`${params.commandSummary} timed out after ${timeoutMs}ms`)),
          );
        }, timeoutMs)
      : null;
    const onAbort = () => {
-      signalQmdProcessTree(child, "SIGKILL");
+      void signalQmdProcessTree(child, "SIGKILL");
      settle(() => reject(abortReason(signal, params.commandSummary)));
    };
    function settle(run: () => void): void {
@@ -261,7 +266,7 @@ export async function runCliCommand(params: {
        if (settled) {
          return;
        }
-        signalQmdProcessTree(child, "SIGKILL");
+        void signalQmdProcessTree(child, "SIGKILL");
        settle(() =>
          reject(
            new Error(`${params.commandSummary} ${streamName} error: ${error.message}`, {
@@ -356,7 +361,57 @@ function resolveWindowsTaskkillPath(env: Record<string, string | undefined> = pr
  return path.win32.join(systemRoot, "System32", "taskkill.exe");
 }

-function signalQmdProcessTree(child: QmdChildProcess, signal?: NodeJS.Signals): void {
+function runWindowsTaskkill(params: {
+  taskkillPath: string;
+  args: string[];
+}): Promise<WindowsTaskkillResult> {
+  return new Promise((resolve) => {
+    let taskkill: ReturnType<typeof spawn>;
+    try {
+      taskkill = spawn(params.taskkillPath, params.args, {
+        stdio: "ignore",
+        windowsHide: true,
+      });
+    } catch {
+      resolve("failure");
+      return;
+    }
+
+    let settled = false;
+    const finish = (result: WindowsTaskkillResult) => {
+      if (settled) {
+        return;
+      }
+      settled = true;
+      clearTimeout(timeout);
+      resolve(result);
+    };
+    taskkill.once("error", () => finish("failure"));
+    taskkill.once("close", (code) => finish(code === 0 ? "success" : "failure"));
+    const timeout = setTimeout(() => {
+      // Fix the timeout result before kill() can synchronously emit an error.
+      finish("timed-out");
+      try {
+        taskkill.kill("SIGKILL");
+      } catch {
+        // The retained qmd child handle remains the final fallback below.
+      }
+      taskkill.unref();
+      // Do not wait for a stalled system utility after its deadline. Its late
+      // events stay guarded by finish(), while qmd cleanup uses the child handle.
+    }, WINDOWS_TASKKILL_TIMEOUT_MS);
+    timeout.unref?.();
+  });
+}
+
+function isQmdChildAlive(child: QmdChildProcess): boolean {
+  return child.exitCode === null && child.signalCode === null;
+}
+
+async function signalQmdProcessTree(
+  child: QmdChildProcess,
+  signal?: NodeJS.Signals,
+): Promise<void> {
  if (shouldUseQmdProcessGroup() && typeof child.pid === "number") {
    try {
      if (signal === undefined) {
@@ -369,30 +424,36 @@ function signalQmdProcessTree(child: QmdChildProcess, signal?: NodeJS.Signals):
      // Fall back to the direct child if the process group already disappeared.
    }
  }
-  if (!shouldUseQmdProcessGroup() && typeof child.pid === "number") {
+  if (!shouldUseQmdProcessGroup() && typeof child.pid === "number" && isQmdChildAlive(child)) {
    const taskkillPath = resolveWindowsTaskkillPath();
    const args = ["/PID", String(child.pid), "/T"];
    if (signal === "SIGKILL") {
      args.push("/F");
    }
-    const result = spawnSync(taskkillPath, args, { stdio: "ignore", windowsHide: true });
-    if (!result.error && result.status === 0) {
+    const result = await runWindowsTaskkill({ taskkillPath, args });
+    if (result === "success") {
      return;
    }
-    if (signal !== "SIGKILL") {
-      const forceResult = spawnSync(taskkillPath, [...args, "/F"], {
-        stdio: "ignore",
-        windowsHide: true,
-      });
-      if (!forceResult.error && forceResult.status === 0) {
+    // taskkill /T requires a live root PID. Retrying an exited, reusable PID
+    // can target an unrelated process tree.
+    if (signal !== "SIGKILL" && result !== "timed-out" && isQmdChildAlive(child)) {
+      const forceResult = await runWindowsTaskkill({ taskkillPath, args: [...args, "/F"] });
+      if (forceResult === "success") {
        return;
      }
    }
  }
-  if (signal === undefined) {
-    child.kill();
-  } else {
-    child.kill(signal);
+  if (!isQmdChildAlive(child)) {
+    return;
+  }
+  try {
+    if (signal === undefined) {
+      child.kill();
+    } else {
+      child.kill(signal);
+    }
+  } catch {
+    // The child may already have exited while asynchronous tree cleanup ran.
  }
 }