From 53950bcbe88caad571af24a554f728a72c3a5022 Mon Sep 17 00:00:00 2001 From: yoshi49535 Date: Fri, 8 May 2026 17:00:20 +0900 Subject: [PATCH 1/4] fix: gracefulShutdown awaits async cleanup after server.close drain (v0.0.4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-existing handler ran cleanup synchronously *before* server.close, which silently dropped any Promise returned by cleanup. The process then exited before async resource releases (Redis disconnects, DB pools, file handles) could complete. New shape: 1. SIGTERM/SIGINT triggers handler 2. server.close() drains in-flight requests 3. server.closeAllConnections() drains keep-alive sockets (Node >= 18.2.0) 4. await cleanup?.() — caught and logged on rejection, does not abort exit 5. process.exit(0) Signature widening: cleanup type goes from () => void to () => void | Promise. This is structurally backward-compatible — every previous caller's cleanup shape still satisfies the new union. Cleanup errors are caught with console.error("gracefulShutdown: cleanup error", err) and do NOT abort process.exit(0). The contract is "best-effort cleanup, then guaranteed exit" — operators relying on hung-cleanup-blocks-exit semantics had no such guarantee in the previous implementation either (because cleanup was synchronous, exceptions would have crashed the handler before exit). Tests: 4 new RED → GREEN tests (call order, async await, error logging, closeAllConnections invocation) + 2 existing tests updated to be async and mock closeAllConnections. All 27 tests pass; lint, typecheck, and build all green. Spec: .claude/superpowers/specs/2026-05-05-d5-builder-context-lifecycle.md (cross-repo gracefulShutdown semantics fix section, lines 475-520). Cross-repo coordination: This release is a publish prerequisite for o3co/auth.provider v0.5.2 — the standalone template's gracefulShutdown(server, () => handle.dispose()) call was already passing an async cleanup that the previous sync impl silently dropped; v0.0.4 properly awaits it before process.exit. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 65 +++++++++++++++ src/__tests__/shutdown.test.mts | 142 +++++++++++++++++++++++++++++++- src/shutdown.mts | 13 ++- 3 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..d1a5e6a --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,65 @@ +# Changelog + +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). + +## [0.0.4] — Unreleased + +### Changed + +- **`gracefulShutdown(server, cleanup)` now awaits an async `cleanup` + callback after `server.close()` drains in-flight requests.** The + `cleanup` parameter type is widened from `() => void` to + `() => void | Promise` (backward-compatible at the call site — + any function that previously satisfied `() => void` still satisfies + the new union). Cleanup errors are caught individually with + `console.error("gracefulShutdown: cleanup error", err)` and do not + abort `process.exit(0)`. + + Previously, `cleanup` ran synchronously *before* `server.close()`, + which meant any returned `Promise` was dropped on the floor — the + process exited before async resource releases (Redis disconnects, + database pools, file handles) could complete. Now the order is: + + 1. SIGTERM/SIGINT triggers the handler + 2. `server.close()` drains in-flight requests + 3. `server.closeAllConnections()` drains keep-alive sockets (Node.js + >= 18.2.0) + 4. `await cleanup?.()` (caught + logged on rejection) + 5. `process.exit(0)` + + This is a behavior change. Operators relying on the old fire-and-forget + ordering — where `cleanup` started before `server.close()` and ran + concurrently with request draining — will now experience strictly + sequential, awaited cleanup. In practice, the new ordering is what + most operators *expected* the old code to do, so the migration impact + is typically zero. + +### Added + +- **`server.closeAllConnections()` is invoked alongside `server.close()`** + to drain keep-alive sockets that the latter alone leaves hanging on + modern Express / HTTP/1.1 deployments. Requires Node.js >= 18.2.0 + (already an effective floor for this package's consumers). + +### Migration + +The signature widening from `() => void` to `() => void | Promise` +is structurally backward-compatible: every previous caller's `cleanup` +shape still satisfies the new type. No code change is required at call +sites unless the caller explicitly wants to take advantage of awaited +async cleanup (e.g., switching from +`() => { redis.disconnect(); }` to +`async () => { await redis.disconnect(); }`). + +For callers in `o3co/auth.provider` `templates/standalone/src/app.mts`: +the existing call `gracefulShutdown(server, () => handle.dispose())` was +already passing a function returning a `Promise` that the old code was +silently dropping. After this release, `handle.dispose()` is properly +awaited before `process.exit(0)` runs. + +## [0.0.3] — 2026-04-18 + +- CI / publishing infrastructure: OIDC Trusted Publishing, dependabot, + pnpm store cache, idempotent release workflow, badges. diff --git a/src/__tests__/shutdown.test.mts b/src/__tests__/shutdown.test.mts index 969fce8..5b8b8c1 100644 --- a/src/__tests__/shutdown.test.mts +++ b/src/__tests__/shutdown.test.mts @@ -2,6 +2,11 @@ import { createServer } from "node:http"; import { afterEach, describe, expect, it, vi } from "vitest"; import { gracefulShutdown } from "../shutdown.mjs"; +const flushMicrotasks = async (): Promise => { + await new Promise((r) => setImmediate(r)); + await new Promise((r) => setImmediate(r)); +}; + describe("gracefulShutdown", () => { const listeners: Map void> = new Map(); @@ -29,13 +34,141 @@ describe("gracefulShutdown", () => { onSpy.mockRestore(); }); - it("calls cleanup function when handler is invoked", () => { + it("invokes cleanup inside server.close callback (after in-flight requests drain)", async () => { + const order: string[] = []; + const cleanup = vi.fn(() => { + order.push("cleanup"); + }); + const server = createServer(); + const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { + order.push("close"); + cb?.(); + return server; + }); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { + order.push("exit"); + return undefined as never; + }); + const onSpy = vi.spyOn(process, "on"); + + gracefulShutdown(server, cleanup); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + handler(); + await flushMicrotasks(); + + expect(order).toEqual(["close", "cleanup", "exit"]); + + closeSpy.mockRestore(); + closeAllSpy.mockRestore(); + exitSpy.mockRestore(); + onSpy.mockRestore(); + }); + + it("awaits an async cleanup before exiting", async () => { + let cleanupResolved = false; + const cleanup = vi.fn(async () => { + await new Promise((r) => setTimeout(r, 30)); + cleanupResolved = true; + }); + const server = createServer(); + const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { + cb?.(); + return server; + }); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + let exitObservedCleanupResolved = false; + const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { + exitObservedCleanupResolved = cleanupResolved; + return undefined as never; + }); + const onSpy = vi.spyOn(process, "on"); + + gracefulShutdown(server, cleanup); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + handler(); + await new Promise((r) => setTimeout(r, 60)); + + expect(cleanup).toHaveBeenCalledOnce(); + expect(cleanupResolved).toBe(true); + expect(exitSpy).toHaveBeenCalledWith(0); + expect(exitObservedCleanupResolved).toBe(true); + + closeSpy.mockRestore(); + closeAllSpy.mockRestore(); + exitSpy.mockRestore(); + onSpy.mockRestore(); + }); + + it("logs cleanup errors via console.error and still exits", async () => { + const cleanupErr = new Error("cleanup boom"); + const cleanup = vi.fn(async () => { + throw cleanupErr; + }); + const server = createServer(); + const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { + cb?.(); + return server; + }); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const onSpy = vi.spyOn(process, "on"); + + gracefulShutdown(server, cleanup); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + handler(); + await flushMicrotasks(); + + expect(errorSpy).toHaveBeenCalledWith("gracefulShutdown: cleanup error", cleanupErr); + expect(exitSpy).toHaveBeenCalledWith(0); + + closeSpy.mockRestore(); + closeAllSpy.mockRestore(); + exitSpy.mockRestore(); + errorSpy.mockRestore(); + onSpy.mockRestore(); + }); + + it("calls server.closeAllConnections to drain keepalive sockets", async () => { + const server = createServer(); + const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { + cb?.(); + return server; + }); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); + const onSpy = vi.spyOn(process, "on"); + + gracefulShutdown(server); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + handler(); + await flushMicrotasks(); + + expect(closeAllSpy).toHaveBeenCalledOnce(); + + closeSpy.mockRestore(); + closeAllSpy.mockRestore(); + exitSpy.mockRestore(); + onSpy.mockRestore(); + }); + + it("calls cleanup function when handler is invoked", async () => { const cleanup = vi.fn(); const server = createServer(); const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { cb?.(); return server; }); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -46,22 +179,25 @@ describe("gracefulShutdown", () => { listeners.set("SIGTERM", handler); handler(); + await flushMicrotasks(); expect(cleanup).toHaveBeenCalledOnce(); expect(closeSpy).toHaveBeenCalledOnce(); expect(exitSpy).toHaveBeenCalledWith(0); closeSpy.mockRestore(); + closeAllSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); - it("works without cleanup function", () => { + it("works without cleanup function", async () => { const server = createServer(); const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { cb?.(); return server; }); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -72,11 +208,13 @@ describe("gracefulShutdown", () => { listeners.set("SIGTERM", handler); handler(); + await flushMicrotasks(); expect(closeSpy).toHaveBeenCalledOnce(); expect(exitSpy).toHaveBeenCalledWith(0); closeSpy.mockRestore(); + closeAllSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); diff --git a/src/shutdown.mts b/src/shutdown.mts index f2db9f1..233bf02 100644 --- a/src/shutdown.mts +++ b/src/shutdown.mts @@ -1,9 +1,16 @@ import type { Server } from "node:http"; -export function gracefulShutdown(server: Server, cleanup?: () => void): void { +export function gracefulShutdown(server: Server, cleanup?: () => void | Promise): void { const handler = (): void => { - cleanup?.(); - server.close(() => process.exit(0)); + server.close(async () => { + try { + await cleanup?.(); + } catch (err) { + console.error("gracefulShutdown: cleanup error", err); + } + process.exit(0); + }); + server.closeAllConnections(); }; process.on("SIGTERM", handler); process.on("SIGINT", handler); From 3641ba7bb9d8e99b47071bfdb944a7b0d12a6561 Mon Sep 17 00:00:00 2001 From: yoshi49535 Date: Fri, 8 May 2026 17:09:47 +0900 Subject: [PATCH 2/4] fixup: address Round 1 review (closeIdleConnections + CHANGELOG + engines) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 multi-agent-review converged on a Critical issue + 2 Important items. This fixup addresses C1+C2+C3+C4+C6. C1 (Critical, Codex P1 + Claude I-2 convergence) — replace `server.closeAllConnections()` with `server.closeIdleConnections()`. Per Node.js docs: closeAllConnections — "Closes all established HTTP(S) connections... including active connections connected to this server which are sending a request or waiting for a response." closeIdleConnections — "Closes all connections... which are not sending a request or waiting for a response." The original spec called for `closeAllConnections`, but that destroys in-flight request connections too — exactly the regression a graceful shutdown is meant to avoid. `closeIdleConnections` is the correct keep-alive-reaping primitive. (Note: Node.js >= 19.0.0 reaps idle keep-alives in `server.close` itself, so the explicit call is harmless there and necessary on 18.x.) C2 (Important, Claude I-1) — fix CHANGELOG v0.0.3 release date 2026-04-18 → 2026-04-14 (verified against tag commit df1b2ed). C3 (Important, Claude I-2 paragraph 1) — rewrite the CHANGELOG "new shape" narrative so it reflects the actual concurrent execution: `server.close(...)` is registered with an async callback while `server.closeIdleConnections()` runs synchronously in parallel. The previous numbered-list framing implied sequential 1→2→3→4→5 which mismatched the implementation. C4 (Minor, Claude M-1) — declare `engines.node >= 18.19.0` in package.json. Matches the auth.provider consumer floor, fails fast on pre-18.2 installers where `closeIdleConnections` is undefined. C6 (Minor, Claude M-3) — rename "calls cleanup function when handler is invoked" test to "works with a synchronous cleanup function" so it clearly differentiates from the new "invokes cleanup inside server.close callback (after in-flight requests drain)" ordering test, and rename "calls server.closeAllConnections to drain keepalive sockets" to "calls server.closeIdleConnections to drain idle keep-alive sockets without aborting in-flight requests" for the same clarity reason. Deferred (per user-confirmed scope): - C5 M-2 JSDoc — pre-existing absence; out of scope for this PR - C7 M-4 SIGINT behavioral test — structural argument suffices (same handler reference is registered for both signals) - C8 M-5 cleanup-hang timeout — would be a separate contract decision, out of scope Verification: 27/27 tests pass, typecheck/lint/build all green. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 46 +++++++++++++++++++++------------ package.json | 3 +++ src/__tests__/shutdown.test.mts | 16 ++++++------ src/shutdown.mts | 2 +- 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1a5e6a..d3eeb12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,35 +13,49 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). `cleanup` parameter type is widened from `() => void` to `() => void | Promise` (backward-compatible at the call site — any function that previously satisfied `() => void` still satisfies - the new union). Cleanup errors are caught individually with + the new union). Cleanup errors are caught with `console.error("gracefulShutdown: cleanup error", err)` and do not abort `process.exit(0)`. Previously, `cleanup` ran synchronously *before* `server.close()`, which meant any returned `Promise` was dropped on the floor — the process exited before async resource releases (Redis disconnects, - database pools, file handles) could complete. Now the order is: + database pools, file handles) could complete. The new shape is: - 1. SIGTERM/SIGINT triggers the handler - 2. `server.close()` drains in-flight requests - 3. `server.closeAllConnections()` drains keep-alive sockets (Node.js - >= 18.2.0) - 4. `await cleanup?.()` (caught + logged on rejection) - 5. `process.exit(0)` + 1. SIGTERM/SIGINT triggers the handler. + 2. `server.close()` is registered with an async callback that fires + once all sockets are closed; in parallel, + `server.closeIdleConnections()` runs synchronously to release + idle keep-alive sockets so the close callback can fire promptly. + 3. Inside the close callback, `await cleanup?.()` runs (caught + + logged on rejection), then `process.exit(0)`. + + `server.closeIdleConnections()` deliberately leaves connections that + are mid-request alone — those drain naturally through `server.close`, + preserving the graceful-shutdown contract for in-flight requests. + (Note: starting with Node.js 19.0.0, `server.close` reaps idle + keep-alive connections on its own; the explicit + `closeIdleConnections()` call is harmless on 19+ and necessary on + 18.x.) This is a behavior change. Operators relying on the old fire-and-forget ordering — where `cleanup` started before `server.close()` and ran concurrently with request draining — will now experience strictly - sequential, awaited cleanup. In practice, the new ordering is what - most operators *expected* the old code to do, so the migration impact - is typically zero. + sequential, awaited cleanup after the request drain. In practice, the + new ordering is what most operators *expected* the old code to do, so + the migration impact is typically zero. ### Added -- **`server.closeAllConnections()` is invoked alongside `server.close()`** - to drain keep-alive sockets that the latter alone leaves hanging on - modern Express / HTTP/1.1 deployments. Requires Node.js >= 18.2.0 - (already an effective floor for this package's consumers). +- **`server.closeIdleConnections()` is invoked in parallel with + `server.close()`** so the close callback can fire promptly on Node 18.x + without waiting for keep-alive timeouts. Active in-flight requests are + not affected — only idle keep-alive connections are released. Requires + Node.js >= 18.2.0; `package.json` now declares + `engines.node >= 18.19.0` which already satisfies this. +- **`engines.node >= 18.19.0`** declared in `package.json` to match the + consumer floor (`auth.provider`) and to fail fast for installers on + pre-18.2 Node where `closeIdleConnections` is undefined. ### Migration @@ -59,7 +73,7 @@ already passing a function returning a `Promise` that the old code was silently dropping. After this release, `handle.dispose()` is properly awaited before `process.exit(0)` runs. -## [0.0.3] — 2026-04-18 +## [0.0.3] — 2026-04-14 - CI / publishing infrastructure: OIDC Trusted Publishing, dependabot, pnpm store cache, idempotent release workflow, badges. diff --git a/package.json b/package.json index 829090d..f404ba8 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,9 @@ "description": "Shared utilities for auth.* services", "version": "0.0.0", "license": "Apache-2.0", + "engines": { + "node": ">=18.19.0" + }, "repository": { "type": "git", "url": "https://github.com/o3co/auth.utils.git" diff --git a/src/__tests__/shutdown.test.mts b/src/__tests__/shutdown.test.mts index 5b8b8c1..c8c934a 100644 --- a/src/__tests__/shutdown.test.mts +++ b/src/__tests__/shutdown.test.mts @@ -45,7 +45,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { order.push("exit"); return undefined as never; @@ -78,7 +78,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); let exitObservedCleanupResolved = false; const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { exitObservedCleanupResolved = cleanupResolved; @@ -114,7 +114,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const onSpy = vi.spyOn(process, "on"); @@ -136,13 +136,13 @@ describe("gracefulShutdown", () => { onSpy.mockRestore(); }); - it("calls server.closeAllConnections to drain keepalive sockets", async () => { + it("calls server.closeIdleConnections to drain idle keep-alive sockets without aborting in-flight requests", async () => { const server = createServer(); const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -161,14 +161,14 @@ describe("gracefulShutdown", () => { onSpy.mockRestore(); }); - it("calls cleanup function when handler is invoked", async () => { + it("works with a synchronous cleanup function", async () => { const cleanup = vi.fn(); const server = createServer(); const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -197,7 +197,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); diff --git a/src/shutdown.mts b/src/shutdown.mts index 233bf02..14acf50 100644 --- a/src/shutdown.mts +++ b/src/shutdown.mts @@ -10,7 +10,7 @@ export function gracefulShutdown(server: Server, cleanup?: () => void | Promise< } process.exit(0); }); - server.closeAllConnections(); + server.closeIdleConnections(); }; process.on("SIGTERM", handler); process.on("SIGINT", handler); From 27ca9851c0ed719b68b67c857795711ab602ce0e Mon Sep 17 00:00:00 2001 From: yoshi49535 Date: Fri, 8 May 2026 17:17:40 +0900 Subject: [PATCH 3/4] fixup: address Round 2 Minor (CHANGELOG wording + var rename + regression defense) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 multi-agent-review (Codex 0 / Claude 3 Minor) — apply all 3. M-1 (Claude) — CHANGELOG line 50 reworded from "in parallel with server.close()" to "synchronously after server.close() registers its callback" so the Added section header matches the lifecycle described in the Changed section above (which already gets it right). M-2 (Claude) — add a negative `expect(closeAllSpy).not.toHaveBeenCalled()` assertion to the closeIdleConnections drain test. Defends against a future contributor flipping back to the pre-Round 1 spec premise where closeAllConnections aborts in-flight requests; the test suite now fails loudly instead of passing silently if that regression is reintroduced. M-3 (Claude) — rename test variable closeAllSpy → closeIdleSpy in the 6 sites that mock closeIdleConnections. The old name was a leftover from the closeAllConnections shape and read inconsistently with the production API. Verification: 27/27 tests pass, lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 12 ++++++------ src/__tests__/shutdown.test.mts | 29 +++++++++++++++++------------ 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3eeb12..902e58b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,12 +47,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Added -- **`server.closeIdleConnections()` is invoked in parallel with - `server.close()`** so the close callback can fire promptly on Node 18.x - without waiting for keep-alive timeouts. Active in-flight requests are - not affected — only idle keep-alive connections are released. Requires - Node.js >= 18.2.0; `package.json` now declares - `engines.node >= 18.19.0` which already satisfies this. +- **`server.closeIdleConnections()` is invoked synchronously after + `server.close()` registers its callback** so the close callback can + fire promptly on Node 18.x without waiting for keep-alive timeouts. + Active in-flight requests are not affected — only idle keep-alive + connections are released. Requires Node.js >= 18.2.0; `package.json` + now declares `engines.node >= 18.19.0` which already satisfies this. - **`engines.node >= 18.19.0`** declared in `package.json` to match the consumer floor (`auth.provider`) and to fail fast for installers on pre-18.2 Node where `closeIdleConnections` is undefined. diff --git a/src/__tests__/shutdown.test.mts b/src/__tests__/shutdown.test.mts index c8c934a..9890a5e 100644 --- a/src/__tests__/shutdown.test.mts +++ b/src/__tests__/shutdown.test.mts @@ -45,7 +45,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { order.push("exit"); return undefined as never; @@ -62,7 +62,7 @@ describe("gracefulShutdown", () => { expect(order).toEqual(["close", "cleanup", "exit"]); closeSpy.mockRestore(); - closeAllSpy.mockRestore(); + closeIdleSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); @@ -78,7 +78,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); let exitObservedCleanupResolved = false; const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { exitObservedCleanupResolved = cleanupResolved; @@ -99,7 +99,7 @@ describe("gracefulShutdown", () => { expect(exitObservedCleanupResolved).toBe(true); closeSpy.mockRestore(); - closeAllSpy.mockRestore(); + closeIdleSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); @@ -114,7 +114,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const onSpy = vi.spyOn(process, "on"); @@ -130,7 +130,7 @@ describe("gracefulShutdown", () => { expect(exitSpy).toHaveBeenCalledWith(0); closeSpy.mockRestore(); - closeAllSpy.mockRestore(); + closeIdleSpy.mockRestore(); exitSpy.mockRestore(); errorSpy.mockRestore(); onSpy.mockRestore(); @@ -142,7 +142,8 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeAllSpy = vi.spyOn(server, "closeAllConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -153,9 +154,13 @@ describe("gracefulShutdown", () => { handler(); await flushMicrotasks(); - expect(closeAllSpy).toHaveBeenCalledOnce(); + expect(closeIdleSpy).toHaveBeenCalledOnce(); + // Regression defense: closeAllConnections aborts in-flight requests. + // Pre-Round 1 spec premise was wrong; never reintroduce. + expect(closeAllSpy).not.toHaveBeenCalled(); closeSpy.mockRestore(); + closeIdleSpy.mockRestore(); closeAllSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); @@ -168,7 +173,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -186,7 +191,7 @@ describe("gracefulShutdown", () => { expect(exitSpy).toHaveBeenCalledWith(0); closeSpy.mockRestore(); - closeAllSpy.mockRestore(); + closeIdleSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); @@ -197,7 +202,7 @@ describe("gracefulShutdown", () => { cb?.(); return server; }); - const closeAllSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -214,7 +219,7 @@ describe("gracefulShutdown", () => { expect(exitSpy).toHaveBeenCalledWith(0); closeSpy.mockRestore(); - closeAllSpy.mockRestore(); + closeIdleSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); From 8d073f90ebcefad7e73edd16b4a5c978766426ca Mon Sep 17 00:00:00 2001 From: yoshi49535 Date: Fri, 8 May 2026 17:36:29 +0900 Subject: [PATCH 4/4] fixup: address Round 3 Copilot (T3 idempotency guard + T4/T10 rename) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 Copilot review: 10 threads. 7 stale (already addressed by Round 1 fixup 3641ba7 — Copilot's first review was on the pre-fixup commit). 3 new: T3 (Important) — idempotent guard for repeated signal delivery. RED test "is idempotent under repeated signal delivery" added first (asserts 3 handler() calls → cleanup/close/closeIdle/exit each invoked exactly once). Pre-impl: failed with "called 3 times". Post-impl: passes. Implementation: a closure-local `shuttingDown` flag short-circuits the second handler() onward, plus `process.removeListener` for both SIGTERM and SIGINT on first entry. Operators sending multiple SIGTERMs (k8s pre-SIGKILL retry, Ctrl+C-spam) no longer cause duplicated cleanup or multiple process.exit(0). (As a side benefit, removeListener also fixes a pre-existing test-order-pollution flake where the previous "works without cleanup function" test occasionally observed a stale handler on the real process due to listeners not being torn down between tests.) T4 + T10 (Minor, dupe across two Copilot reviews) — rename test helper `flushMicrotasks` → `flushEventLoop`. The helper uses `setImmediate`, which queues macrotasks (event-loop turns), not microtasks. Pure naming clarity. Stale 7 (resolved as already-addressed): - T1, T7: closeAllConnections feature-detect → covered by R1's closeIdleConnections + engines.node - T2, T8: closeAllConnections aborts in-flight → R1 fixed it - T5, T9: CHANGELOG order mismatch → R1 rewrote CHANGELOG narrative - T6: engines.node not declared → R1 added engines.node ≥ 18.19.0 Verification: 28/28 tests pass (27 prior + 1 new RED→GREEN), lint clean, typecheck clean, build clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 7 +++++ src/__tests__/shutdown.test.mts | 46 ++++++++++++++++++++++++++++----- src/shutdown.mts | 5 ++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 902e58b..f75d275 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - **`engines.node >= 18.19.0`** declared in `package.json` to match the consumer floor (`auth.provider`) and to fail fast for installers on pre-18.2 Node where `closeIdleConnections` is undefined. +- **Idempotent under repeated signal delivery.** A `shuttingDown` guard + in the handler returns early on the second SIGTERM / SIGINT, and the + signal listeners are removed on first invocation. Operators that send + multiple SIGTERMs (k8s sending repeated TERM before falling back to + SIGKILL, or operators pressing Ctrl+C several times) no longer cause + duplicated `cleanup()` invocations or duplicate `process.exit(0)` + calls. ### Migration diff --git a/src/__tests__/shutdown.test.mts b/src/__tests__/shutdown.test.mts index 9890a5e..90fe180 100644 --- a/src/__tests__/shutdown.test.mts +++ b/src/__tests__/shutdown.test.mts @@ -2,7 +2,7 @@ import { createServer } from "node:http"; import { afterEach, describe, expect, it, vi } from "vitest"; import { gracefulShutdown } from "../shutdown.mjs"; -const flushMicrotasks = async (): Promise => { +const flushEventLoop = async (): Promise => { await new Promise((r) => setImmediate(r)); await new Promise((r) => setImmediate(r)); }; @@ -57,7 +57,7 @@ describe("gracefulShutdown", () => { const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; listeners.set("SIGTERM", handler); handler(); - await flushMicrotasks(); + await flushEventLoop(); expect(order).toEqual(["close", "cleanup", "exit"]); @@ -124,7 +124,7 @@ describe("gracefulShutdown", () => { const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; listeners.set("SIGTERM", handler); handler(); - await flushMicrotasks(); + await flushEventLoop(); expect(errorSpy).toHaveBeenCalledWith("gracefulShutdown: cleanup error", cleanupErr); expect(exitSpy).toHaveBeenCalledWith(0); @@ -152,7 +152,7 @@ describe("gracefulShutdown", () => { const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; listeners.set("SIGTERM", handler); handler(); - await flushMicrotasks(); + await flushEventLoop(); expect(closeIdleSpy).toHaveBeenCalledOnce(); // Regression defense: closeAllConnections aborts in-flight requests. @@ -184,7 +184,7 @@ describe("gracefulShutdown", () => { listeners.set("SIGTERM", handler); handler(); - await flushMicrotasks(); + await flushEventLoop(); expect(cleanup).toHaveBeenCalledOnce(); expect(closeSpy).toHaveBeenCalledOnce(); @@ -196,6 +196,40 @@ describe("gracefulShutdown", () => { onSpy.mockRestore(); }); + it("is idempotent under repeated signal delivery (multi-SIGTERM safe)", async () => { + const cleanup = vi.fn(); + const server = createServer(); + const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { + cb?.(); + return server; + }); + const closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); + const onSpy = vi.spyOn(process, "on"); + + gracefulShutdown(server, cleanup); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + // Simulate three rapid SIGTERM deliveries (k8s will SIGKILL after grace, + // but operators may also Ctrl+C several times in a row). + handler(); + handler(); + handler(); + await flushEventLoop(); + + expect(cleanup).toHaveBeenCalledOnce(); + expect(closeSpy).toHaveBeenCalledOnce(); + expect(closeIdleSpy).toHaveBeenCalledOnce(); + expect(exitSpy).toHaveBeenCalledOnce(); + expect(exitSpy).toHaveBeenCalledWith(0); + + closeSpy.mockRestore(); + closeIdleSpy.mockRestore(); + exitSpy.mockRestore(); + onSpy.mockRestore(); + }); + it("works without cleanup function", async () => { const server = createServer(); const closeSpy = vi.spyOn(server, "close").mockImplementation((cb) => { @@ -213,7 +247,7 @@ describe("gracefulShutdown", () => { listeners.set("SIGTERM", handler); handler(); - await flushMicrotasks(); + await flushEventLoop(); expect(closeSpy).toHaveBeenCalledOnce(); expect(exitSpy).toHaveBeenCalledWith(0); diff --git a/src/shutdown.mts b/src/shutdown.mts index 14acf50..3072a9a 100644 --- a/src/shutdown.mts +++ b/src/shutdown.mts @@ -1,7 +1,12 @@ import type { Server } from "node:http"; export function gracefulShutdown(server: Server, cleanup?: () => void | Promise): void { + let shuttingDown = false; const handler = (): void => { + if (shuttingDown) return; + shuttingDown = true; + process.removeListener("SIGTERM", handler); + process.removeListener("SIGINT", handler); server.close(async () => { try { await cleanup?.();