diff --git a/scripts/check-gateway-watch-regression.mjs b/scripts/check-gateway-watch-regression.mjs index a8d59516ebdd..0dd7f864d464 100644 --- a/scripts/check-gateway-watch-regression.mjs +++ b/scripts/check-gateway-watch-regression.mjs @@ -20,6 +20,7 @@ const DEFAULTS = { readyTimeoutMs: 20_000, readySettleMs: 500, sigkillGraceMs: 10_000, + sigkillExitGraceMs: 2_000, cpuWarnMs: 1_000, cpuFailMs: 8_000, distRuntimeFileGrowthMax: 200, @@ -69,6 +70,9 @@ function parseArgs(argv) { case "--sigkill-grace-ms": options.sigkillGraceMs = Number(readValue()); break; + case "--sigkill-exit-grace-ms": + options.sigkillExitGraceMs = Number(readValue()); + break; case "--cpu-warn-ms": options.cpuWarnMs = Number(readValue()); break; @@ -467,10 +471,6 @@ async function runTimedWatch(options, outputDir) { stderr += String(chunk); }); - const exitPromise = new Promise((resolve) => { - child.on("exit", (code, signal) => resolve({ code, signal })); - }); - let watchPid = null; for (let attempt = 0; attempt < 50; attempt += 1) { if (fs.existsSync(pidFilePath)) { @@ -491,30 +491,7 @@ async function runTimedWatch(options, outputDir) { await sleep(options.windowMs); const idleCpuEndMs = watchPid ? readProcessTreeCpuMs(watchPid) : null; - if (watchPid) { - try { - process.kill(watchPid, "SIGTERM"); - } catch { - // ignore - } - } - - const gracefulExit = await Promise.race([ - exitPromise, - sleep(options.sigkillGraceMs).then(() => null), - ]); - - if (gracefulExit === null) { - if (watchPid) { - try { - process.kill(watchPid, "SIGKILL"); - } catch { - // ignore - } - } - } - - const exit = (await exitPromise) ?? { code: null, signal: null }; + const exit = await stopTimedWatchChild(child, watchPid, options); fs.writeFileSync(stdoutPath, stdout, "utf8"); fs.writeFileSync(stderrPath, stderr, "utf8"); const timing = fs.existsSync(timeFilePath) @@ -535,6 +512,56 @@ async function runTimedWatch(options, outputDir) { }; } +export async function stopTimedWatchChild(child, watchPid, options, deps = {}) { + const killProcess = deps.killProcess ?? ((pid, signal) => process.kill(pid, signal)); + const currentExit = () => + child.exitCode !== null || child.signalCode !== null + ? { code: child.exitCode, signal: child.signalCode } + : null; + const exited = new Promise((resolve) => { + child.once("exit", (code, signal) => resolve({ code, signal })); + }); + const waitForExit = async (ms) => + currentExit() ?? (await Promise.race([exited, sleep(ms).then(() => null)])); + const signalWatchProcess = (signal) => { + if (!watchPid) { + return; + } + try { + killProcess(watchPid, signal); + } catch { + // ignore + } + }; + + const existingExit = currentExit(); + if (existingExit) { + return existingExit; + } + + signalWatchProcess("SIGTERM"); + const gracefulExit = await waitForExit(options.sigkillGraceMs); + if (gracefulExit) { + return gracefulExit; + } + + signalWatchProcess("SIGKILL"); + const killedExit = await waitForExit(options.sigkillExitGraceMs ?? DEFAULTS.sigkillExitGraceMs); + if (killedExit) { + return killedExit; + } + + releaseUnsettledWatchChild(child); + return { code: null, signal: "SIGKILL" }; +} + +function releaseUnsettledWatchChild(child) { + child.stdin?.destroy?.(); + child.stdout?.destroy?.(); + child.stderr?.destroy?.(); + child.unref?.(); +} + function parsePathFile(filePath) { return fs .readFileSync(filePath, "utf8") diff --git a/test/scripts/check-gateway-watch-regression.test.ts b/test/scripts/check-gateway-watch-regression.test.ts index e0ba1f709c46..015d37d3d993 100644 --- a/test/scripts/check-gateway-watch-regression.test.ts +++ b/test/scripts/check-gateway-watch-regression.test.ts @@ -1,10 +1,12 @@ +import { EventEmitter } from "node:events"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { hasGatewayReadyLog, shouldRefreshBuildStampForRestoredArtifacts, + stopTimedWatchChild, writeBuildAndRuntimePostBuildStamps, } from "../../scripts/check-gateway-watch-regression.mjs"; import { @@ -57,4 +59,38 @@ describe("check-gateway-watch-regression", () => { fs.rmSync(rootDir, { recursive: true, force: true }); } }); + + it("bounds teardown when the watch process ignores termination signals", async () => { + const child = new EventEmitter() as EventEmitter & { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + stderr: { destroy: ReturnType }; + stdin: { destroy: ReturnType }; + stdout: { destroy: ReturnType }; + unref: ReturnType; + }; + child.exitCode = null; + child.signalCode = null; + child.stderr = { destroy: vi.fn() }; + child.stdin = { destroy: vi.fn() }; + child.stdout = { destroy: vi.fn() }; + child.unref = vi.fn(); + const killProcess = vi.fn(); + + await expect( + stopTimedWatchChild( + child, + 1234, + { sigkillExitGraceMs: 1, sigkillGraceMs: 1 }, + { killProcess }, + ), + ).resolves.toEqual({ code: null, signal: "SIGKILL" }); + + expect(killProcess).toHaveBeenNthCalledWith(1, 1234, "SIGTERM"); + expect(killProcess).toHaveBeenNthCalledWith(2, 1234, "SIGKILL"); + expect(child.stdin.destroy).toHaveBeenCalledOnce(); + expect(child.stdout.destroy).toHaveBeenCalledOnce(); + expect(child.stderr.destroy).toHaveBeenCalledOnce(); + expect(child.unref).toHaveBeenCalledOnce(); + }); });