diff --git a/packages/memory-host-sdk/src/host/qmd-process.test.ts b/packages/memory-host-sdk/src/host/qmd-process.test.ts index 4c735544dca..ed54a7230fd 100644 --- a/packages/memory-host-sdk/src/host/qmd-process.test.ts +++ b/packages/memory-host-sdk/src/host/qmd-process.test.ts @@ -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("node:child_process"); return { ...actual, spawn: spawnMock, - spawnSync: spawnSyncMock, }; }); @@ -41,19 +39,33 @@ function createMockChild(params: { pid?: number } = {}) { pid?: number; stdout: EventEmitter & { setEncoding: ReturnType }; stderr: EventEmitter & { setEncoding: ReturnType }; + exitCode: number | null; + signalCode: NodeJS.Signals | null; kill: ReturnType; + unref: ReturnType; 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((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) => { diff --git a/packages/memory-host-sdk/src/host/qmd-process.ts b/packages/memory-host-sdk/src/host/qmd-process.ts index 4c3c7ec486a..7a0936f6cad 100644 --- a/packages/memory-host-sdk/src/host/qmd-process.ts +++ b/packages/memory-host-sdk/src/host/qmd-process.ts @@ -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 = 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 { + return new Promise((resolve) => { + let taskkill: ReturnType; + 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 { 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. } }