diff --git a/clients/cli/__tests__/load-summary.test.ts b/clients/cli/__tests__/load-summary.test.ts index 7c417a58f0..770e35c297 100644 --- a/clients/cli/__tests__/load-summary.test.ts +++ b/clients/cli/__tests__/load-summary.test.ts @@ -43,19 +43,34 @@ import { * Deliver a signal to the listener `withLoadInterrupt` added, and to nothing * else: `process.emit` would also reach the test worker's own SIGINT/SIGTERM * handlers, which end the worker. Returns false while no new listener exists. + * + * Calls the RAW listener, so a `process.once` one (the re-raise absorber, + * #2973) removes itself as a real delivery would; `process.listeners` hands + * back the unwrapped function, and calling that would leave it registered. */ function signalLoadRun( signal: "SIGINT" | "SIGTERM", existing: NodeJS.SignalsListener[], ): boolean { const added = process - .listeners(signal) - .find((listener) => !existing.includes(listener)); + .rawListeners(signal) + .find((raw) => !existing.includes(unwrapOnce(raw))); if (!added) return false; - (added as (s: NodeJS.Signals) => void)(signal); + (added as NodeJS.SignalsListener)(signal); return true; } +/** + * The function a `process.once` wrapper calls, or the listener itself. + * `rawListeners` is typed `Function[]`, hence the single narrowing cast. + */ +function unwrapOnce(raw: object): NodeJS.SignalsListener { + const listener = raw as NodeJS.SignalsListener & { + listener?: NodeJS.SignalsListener; + }; + return listener.listener ?? listener; +} + describe("success and failure latency (#2836)", () => { it("times successes apart from failures and counts ok throughput", async () => { const clock = fakeClock(); @@ -585,6 +600,8 @@ describe("withLoadInterrupt (#2834)", () => { const run = withLoadInterrupt(untilAborted); expect(process.listenerCount("SIGTERM")).toBe(before + 1); expect(signalLoadRun("SIGTERM", existing)).toBe(true); + // `when-exit`'s re-raise, landing while the run is still pending. + expect(signalLoadRun("SIGTERM", existing)).toBe(true); await expect(run).resolves.toEqual({ reason: "interrupted", detail: "received SIGTERM", @@ -632,6 +649,8 @@ describe("withLoadInterrupt (#2834)", () => { expect(exit).toHaveBeenCalledWith(EXIT_CODES.INTERRUPTED); finish(); await run; + // The SIGINT re-raise never came, so deliver it to the absorber (#2973). + expect(signalLoadRun("SIGINT", ints)).toBe(true); }); it("exits the real process by default on the second signal", async () => { @@ -651,6 +670,28 @@ describe("withLoadInterrupt (#2834)", () => { expect(exit).toHaveBeenCalledWith(130); finish(); await run; + // The SIGINT re-raise never came, so deliver it to the absorber (#2973). + expect(signalLoadRun("SIGINT", ints)).toBe(true); + }); + + it("absorbs a re-raise still in flight when the run ends (#2973)", async () => { + const exit = vi.fn(); + const existing = process.listeners("SIGINT"); + const before = existing.length; + const terms = process.listenerCount("SIGTERM"); + // The run settles on the abort, before the re-raise has arrived. + const run = withLoadInterrupt(untilAborted, { quiet: true, exit }); + signalLoadRun("SIGINT", existing); + await run; + // One listener is left, for the first signal only, so the pending copy + // never meets Node's default disposition (which would end the process by + // the signal rather than with 130). + expect(process.listenerCount("SIGINT")).toBe(before + 1); + expect(process.listenerCount("SIGTERM")).toBe(terms); + // The copy arrives — at any time, there is no window — and is swallowed. + expect(signalLoadRun("SIGINT", existing)).toBe(true); + expect(exit).not.toHaveBeenCalled(); + expect(process.listenerCount("SIGINT")).toBe(before); }); it("removes its listeners when the run throws", async () => { @@ -723,6 +764,9 @@ describe("--load summary fixes (CLI)", () => { () => expect(signalLoadRun("SIGINT", existing)).toBe(true), { timeout: 10_000, interval: 200 }, ); + // `when-exit`'s re-raise, as a real process would receive it; without + // it the listener that absorbs a late copy (#2973) is left behind. + expect(signalLoadRun("SIGINT", existing)).toBe(true); const result = await run; expect(result.exitCode).toBe(EXIT_CODES.INTERRUPTED); const lines = result.stdout.trim().split("\n"); diff --git a/clients/cli/src/handlers/load-interrupt.ts b/clients/cli/src/handlers/load-interrupt.ts index 82431dca6b..a5d63961ad 100644 --- a/clients/cli/src/handlers/load-interrupt.ts +++ b/clients/cli/src/handlers/load-interrupt.ts @@ -33,7 +33,22 @@ import type { * * The listeners exist only while `run` is pending, so a signal before or after * the run keeps Node's default behaviour, and an in-process caller (the - * launcher, the test runner) is left with no stray listener. + * launcher, the test runner) is left with no stray listener — with one + * exception, below. + * + * ⚠️ **The re-raise can still be in flight when the run ends (#2973).** A + * signal a process sends itself is not handled synchronously: the kernel may + * hand it to any thread, and on a loaded machine that thread can wait several + * milliseconds to be scheduled — longer than the in-flight calls take to + * settle. Removing the last listener restores the default disposition, so a + * copy still pending at that moment killed the process by the signal, after + * the summary was out but before it could exit 130. So when the run ends + * without the copy having arrived, a one-shot listener is left to absorb it. + * Its removal is driven by the copy itself rather than by a timer, which is + * what makes it deterministic: however late the copy lands, it is swallowed, + * and the default disposition returns only after it. When nothing re-raises + * (a caller that invokes the listener directly), that one no-op listener is + * the stray left behind; it swallows one later signal and removes itself. */ /** How long after the first signal a repeat of it is treated as a re-raise. */ export const REPEAT_GRACE_MS = 500; @@ -52,10 +67,13 @@ export async function withLoadInterrupt( const exit = options.exit ?? ((code: number) => process.exit(code)); const now = options.now ?? (() => performance.now()); let first: { signal: NodeJS.Signals; at: number } | undefined; + // Whether the dependency's re-raise of `first` has been seen yet (#2973). + let echoed = false; const onSignal = (signal: NodeJS.Signals): void => { if (first !== undefined) { // A dependency's re-raise of the first signal, not a second press. if (signal === first.signal && now() - first.at < REPEAT_GRACE_MS) { + echoed = true; return; } exit(EXIT_CODES.INTERRUPTED); @@ -78,11 +96,21 @@ export async function withLoadInterrupt( try { return await run(controller.signal); } finally { + // Added BEFORE the `off` calls, so the signal never has zero listeners: + // the pending copy is delivered on whichever thread the kernel picks, not + // between two lines of this one, so even a synchronous gap with the + // default disposition restored would be a window it could land in. + if (first !== undefined && !echoed) { + process.once(first.signal, absorbReRaise); + } process.off("SIGINT", onSignal); process.off("SIGTERM", onSignal); } } +/** Swallows the in-flight re-raise of the first signal (#2973). */ +function absorbReRaise(): void {} + /** * After the summary is printed: exit 130 (`interrupted`) when the run was * cut short by Ctrl+C / SIGTERM, the conventional status for a process ended