From 1baab3bef5bdd5d055bfdb67d366566476d98d6d Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 27 May 2026 03:36:54 +0200 Subject: [PATCH] fix(gateway): bound benchmark teardown waits --- scripts/bench-gateway-restart.ts | 52 +++++++++++++++++----- scripts/bench-gateway-startup.ts | 52 +++++++++++++++++----- test/scripts/bench-gateway-restart.test.ts | 36 +++++++++++++++ test/scripts/bench-gateway-startup.test.ts | 36 +++++++++++++++ 4 files changed, 156 insertions(+), 20 deletions(-) diff --git a/scripts/bench-gateway-restart.ts b/scripts/bench-gateway-restart.ts index 5a80e1ecd13..937ad3da8a0 100644 --- a/scripts/bench-gateway-restart.ts +++ b/scripts/bench-gateway-restart.ts @@ -173,6 +173,8 @@ const DEFAULT_TIMEOUT_MS = 30_000; const DEFAULT_POST_READY_DELAY_MS = 250; const DEFAULT_ENTRY = "dist/entry.js"; const RESTART_INTENT_FILENAME = "gateway-restart-intent.json"; +const TEARDOWN_GRACE_MS = 2_000; +const TEARDOWN_KILL_GRACE_MS = 1_000; const BASE_CONFIG = { browser: { enabled: false }, @@ -879,7 +881,10 @@ function writeRestartIntent(env: NodeJS.ProcessEnv, targetPid: number, reason: s } } -async function stopChild(child: ChildProcessWithoutNullStreams): Promise { +async function stopChild( + child: ChildProcessWithoutNullStreams, + options: { killGraceMs?: number; teardownGraceMs?: number } = {}, +): Promise { const currentExit = (): ChildExit | null => child.exitCode != null || child.signalCode != null ? { exitCode: child.exitCode, signal: child.signalCode } @@ -897,6 +902,8 @@ async function stopChild(child: ChildProcessWithoutNullStreams): Promise => + await Promise.race([exited, delay(ms).then(() => null)]); await new Promise((resolve) => setImmediate(resolve)); const queuedExit = observedExit ?? currentExit(); @@ -904,15 +911,39 @@ async function stopChild(child: ChildProcessWithoutNullStreams): Promise { - if (child.exitCode == null && child.signalCode == null) { - killProcessTree(child, "SIGKILL"); - } - return exited; - }); - const exit = await Promise.race([exited, timeout]); - return { ...exit, exitedBeforeTeardown: !sentTeardownSignal }; + const gracefulExit = await waitForExit(teardownGraceMs); + if (gracefulExit != null) { + return { ...gracefulExit, exitedBeforeTeardown: !sentTeardownSignal }; + } + + const postGraceExit = currentExit() ?? observedExit; + if (postGraceExit != null) { + return { ...postGraceExit, exitedBeforeTeardown: !sentTeardownSignal }; + } + if (!sentTeardownSignal) { + releaseUnsettledChild(child); + return { exitCode: null, exitedBeforeTeardown: true, signal: null }; + } + + killProcessTree(child, "SIGKILL"); + const killedExit = await waitForExit(killGraceMs); + const finalExit = killedExit ?? currentExit() ?? observedExit; + if (finalExit != null) { + return { ...finalExit, exitedBeforeTeardown: false }; + } + + releaseUnsettledChild(child); + return { exitCode: null, exitedBeforeTeardown: false, signal: "SIGKILL" }; +} + +function releaseUnsettledChild(child: ChildProcessWithoutNullStreams): void { + child.stdin.destroy(); + child.stdout.destroy(); + child.stderr.destroy(); + child.unref(); } function killProcessTree(child: ChildProcessWithoutNullStreams, signal: NodeJS.Signals): boolean { @@ -1559,7 +1590,8 @@ async function runGatewaySample(options: { const exit = await stopChild(child); clearInterval(rssTimer); sampleRss(); - await childExitPromise.catch(() => null); + // stopChild is the bounded teardown wait; the raw exit promise may never settle. + void childExitPromise.catch(() => null); flushOutputLineBuffers(outputBuffers, onLine, performance.now() - sampleStartAt, { flushPartial: true, }); diff --git a/scripts/bench-gateway-startup.ts b/scripts/bench-gateway-startup.ts index 4fe2e17314b..46c832cceb1 100644 --- a/scripts/bench-gateway-startup.ts +++ b/scripts/bench-gateway-startup.ts @@ -109,6 +109,8 @@ const DEFAULT_RUNS = 5; const DEFAULT_WARMUP = 1; const DEFAULT_TIMEOUT_MS = 30_000; const DEFAULT_ENTRY = "dist/entry.js"; +const TEARDOWN_GRACE_MS = 2_000; +const TEARDOWN_KILL_GRACE_MS = 1_000; const BASE_CONFIG = { browser: { enabled: false }, @@ -708,7 +710,10 @@ function sanitizedEnv( return env; } -async function stopChild(child: ChildProcessWithoutNullStreams): Promise { +async function stopChild( + child: ChildProcessWithoutNullStreams, + options: { killGraceMs?: number; teardownGraceMs?: number } = {}, +): Promise { const currentExit = (): ChildExit | null => child.exitCode != null || child.signalCode != null ? { exitCode: child.exitCode, signal: child.signalCode } @@ -726,6 +731,8 @@ async function stopChild(child: ChildProcessWithoutNullStreams): Promise => + await Promise.race([exited, delay(ms).then(() => null)]); await new Promise((resolve) => setImmediate(resolve)); const queuedExit = observedExit ?? currentExit(); @@ -733,15 +740,39 @@ async function stopChild(child: ChildProcessWithoutNullStreams): Promise { - if (child.exitCode == null && child.signalCode == null) { - killProcessTree(child, "SIGKILL"); - } - return exited; - }); - const exit = await Promise.race([exited, timeout]); - return { ...exit, exitedBeforeTeardown: !sentTeardownSignal }; + const gracefulExit = await waitForExit(teardownGraceMs); + if (gracefulExit != null) { + return { ...gracefulExit, exitedBeforeTeardown: !sentTeardownSignal }; + } + + const postGraceExit = currentExit() ?? observedExit; + if (postGraceExit != null) { + return { ...postGraceExit, exitedBeforeTeardown: !sentTeardownSignal }; + } + if (!sentTeardownSignal) { + releaseUnsettledChild(child); + return { exitCode: null, exitedBeforeTeardown: true, signal: null }; + } + + killProcessTree(child, "SIGKILL"); + const killedExit = await waitForExit(killGraceMs); + const finalExit = killedExit ?? currentExit() ?? observedExit; + if (finalExit != null) { + return { ...finalExit, exitedBeforeTeardown: false }; + } + + releaseUnsettledChild(child); + return { exitCode: null, exitedBeforeTeardown: false, signal: "SIGKILL" }; +} + +function releaseUnsettledChild(child: ChildProcessWithoutNullStreams): void { + child.stdin.destroy(); + child.stdout.destroy(); + child.stderr.destroy(); + child.unref(); } function killProcessTree(child: ChildProcessWithoutNullStreams, signal: NodeJS.Signals): boolean { @@ -1006,7 +1037,8 @@ async function runGatewaySample(options: { const exit = await stopChild(child); clearInterval(rssTimer); sampleRss(); - await childExitPromise.catch(() => null); + // stopChild is the bounded teardown wait; the raw exit promise may never settle. + void childExitPromise.catch(() => null); rmSync(root, { force: true, maxRetries: 3, recursive: true, retryDelay: 100 }); return { diff --git a/test/scripts/bench-gateway-restart.test.ts b/test/scripts/bench-gateway-restart.test.ts index 0d3822f51b5..6a9b1e5da14 100644 --- a/test/scripts/bench-gateway-restart.test.ts +++ b/test/scripts/bench-gateway-restart.test.ts @@ -261,6 +261,42 @@ node 1234 user 12u IPv4 0t0 TCP localhost:1234 expect(child.kill).toHaveBeenCalledWith("SIGTERM"); }); + it("bounds teardown when the child ignores termination signals", async () => { + const child = new EventEmitter() as EventEmitter & { + exitCode: number | null; + kill: ReturnType; + signalCode: NodeJS.Signals | null; + stderr: { destroy: ReturnType }; + stdin: { destroy: ReturnType }; + stdout: { destroy: ReturnType }; + unref: ReturnType; + }; + child.exitCode = null; + child.signalCode = null; + child.kill = vi.fn(() => true); + child.stderr = { destroy: vi.fn() }; + child.stdin = { destroy: vi.fn() }; + child.stdout = { destroy: vi.fn() }; + child.unref = vi.fn(); + + await expect( + testing.stopChild(child as unknown as Parameters[0], { + killGraceMs: 1, + teardownGraceMs: 1, + }), + ).resolves.toEqual({ + exitedBeforeTeardown: false, + exitCode: null, + signal: "SIGKILL", + }); + expect(child.kill).toHaveBeenNthCalledWith(1, "SIGTERM"); + expect(child.kill).toHaveBeenNthCalledWith(2, "SIGKILL"); + expect(child.stdin.destroy).toHaveBeenCalledOnce(); + expect(child.stdout.destroy).toHaveBeenCalledOnce(); + expect(child.stderr.destroy).toHaveBeenCalledOnce(); + expect(child.unref).toHaveBeenCalledOnce(); + }); + it("marks clean and signaled pre-teardown child exits as benchmark failures", () => { expect( testing.resolveSampleExitFailure({ diff --git a/test/scripts/bench-gateway-startup.test.ts b/test/scripts/bench-gateway-startup.test.ts index 1dd3828c8ea..1c8d60cb361 100644 --- a/test/scripts/bench-gateway-startup.test.ts +++ b/test/scripts/bench-gateway-startup.test.ts @@ -325,6 +325,42 @@ describe("gateway startup benchmark script", () => { expect(child.kill).toHaveBeenCalledWith("SIGTERM"); }); + it("bounds teardown when the child ignores termination signals", async () => { + const child = new EventEmitter() as EventEmitter & { + exitCode: number | null; + kill: ReturnType; + signalCode: NodeJS.Signals | null; + stderr: { destroy: ReturnType }; + stdin: { destroy: ReturnType }; + stdout: { destroy: ReturnType }; + unref: ReturnType; + }; + child.exitCode = null; + child.signalCode = null; + child.kill = vi.fn(() => true); + child.stderr = { destroy: vi.fn() }; + child.stdin = { destroy: vi.fn() }; + child.stdout = { destroy: vi.fn() }; + child.unref = vi.fn(); + + await expect( + testing.stopChild(child as unknown as Parameters[0], { + killGraceMs: 1, + teardownGraceMs: 1, + }), + ).resolves.toEqual({ + exitedBeforeTeardown: false, + exitCode: null, + signal: "SIGKILL", + }); + expect(child.kill).toHaveBeenNthCalledWith(1, "SIGTERM"); + expect(child.kill).toHaveBeenNthCalledWith(2, "SIGKILL"); + expect(child.stdin.destroy).toHaveBeenCalledOnce(); + expect(child.stdout.destroy).toHaveBeenCalledOnce(); + expect(child.stderr.destroy).toHaveBeenCalledOnce(); + expect(child.unref).toHaveBeenCalledOnce(); + }); + it("collects Count-suffixed startup trace metrics", () => { const startupTrace: Record = {};