diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..f75d275 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,86 @@ +# 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 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. The new shape is: + + 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 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.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. +- **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 + +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-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 969fce8..90fe180 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 flushEventLoop = 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,146 @@ 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 closeIdleSpy = vi.spyOn(server, "closeIdleConnections").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 flushEventLoop(); + + expect(order).toEqual(["close", "cleanup", "exit"]); + + closeSpy.mockRestore(); + closeIdleSpy.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 closeIdleSpy = vi.spyOn(server, "closeIdleConnections").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(); + closeIdleSpy.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 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"); + + gracefulShutdown(server, cleanup); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + handler(); + await flushEventLoop(); + + expect(errorSpy).toHaveBeenCalledWith("gracefulShutdown: cleanup error", cleanupErr); + expect(exitSpy).toHaveBeenCalledWith(0); + + closeSpy.mockRestore(); + closeIdleSpy.mockRestore(); + exitSpy.mockRestore(); + errorSpy.mockRestore(); + onSpy.mockRestore(); + }); + + 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 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"); + + gracefulShutdown(server); + + const handler = onSpy.mock.calls.find(([event]) => event === "SIGTERM")?.[1] as () => void; + listeners.set("SIGTERM", handler); + handler(); + await flushEventLoop(); + + 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(); + }); + + 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 closeIdleSpy = vi.spyOn(server, "closeIdleConnections").mockImplementation(() => server); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); const onSpy = vi.spyOn(process, "on"); @@ -46,22 +184,59 @@ describe("gracefulShutdown", () => { listeners.set("SIGTERM", handler); handler(); + await flushEventLoop(); + + expect(cleanup).toHaveBeenCalledOnce(); + expect(closeSpy).toHaveBeenCalledOnce(); + expect(exitSpy).toHaveBeenCalledWith(0); + + closeSpy.mockRestore(); + closeIdleSpy.mockRestore(); + exitSpy.mockRestore(); + 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", () => { + it("works without cleanup function", async () => { 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"); @@ -72,11 +247,13 @@ describe("gracefulShutdown", () => { listeners.set("SIGTERM", handler); handler(); + await flushEventLoop(); expect(closeSpy).toHaveBeenCalledOnce(); expect(exitSpy).toHaveBeenCalledWith(0); closeSpy.mockRestore(); + closeIdleSpy.mockRestore(); exitSpy.mockRestore(); onSpy.mockRestore(); }); diff --git a/src/shutdown.mts b/src/shutdown.mts index f2db9f1..3072a9a 100644 --- a/src/shutdown.mts +++ b/src/shutdown.mts @@ -1,9 +1,21 @@ import type { Server } from "node:http"; -export function gracefulShutdown(server: Server, cleanup?: () => void): void { +export function gracefulShutdown(server: Server, cleanup?: () => void | Promise): void { + let shuttingDown = false; const handler = (): void => { - cleanup?.(); - server.close(() => process.exit(0)); + if (shuttingDown) return; + shuttingDown = true; + process.removeListener("SIGTERM", handler); + process.removeListener("SIGINT", handler); + server.close(async () => { + try { + await cleanup?.(); + } catch (err) { + console.error("gracefulShutdown: cleanup error", err); + } + process.exit(0); + }); + server.closeIdleConnections(); }; process.on("SIGTERM", handler); process.on("SIGINT", handler);