diff --git a/doc/DATABASE.md b/doc/DATABASE.md index 340f6e7d27..5f1c0326d0 100644 --- a/doc/DATABASE.md +++ b/doc/DATABASE.md @@ -122,8 +122,10 @@ All of these are optional; when unset, the driver defaults apply and behavior is ```sh DATABASE_PREPARED_STATEMENTS=false # required for transaction-mode poolers; default: enabled DATABASE_POOL_MAX=25 # connection pool size; default: 10 -DATABASE_IDLE_TIMEOUT_SECONDS=60 # close idle pooled connections; default: keep open +DATABASE_IDLE_TIMEOUT_SECONDS=60 # close idle pooled connections; default: 60 (0 = keep open) DATABASE_CONNECT_TIMEOUT_SECONDS=10 # default: 30 +DATABASE_MAX_LIFETIME_SECONDS=1800 # recycle a pooled connection after this long; default: 30-60 min (random) +DATABASE_APPLICATION_NAME=paperclip # application_name in pg_stat_activity; default: paperclip ``` ### Push the schema diff --git a/docs/deploy/database.md b/docs/deploy/database.md index 0d4ad5731e..5f25489d8e 100644 --- a/docs/deploy/database.md +++ b/docs/deploy/database.md @@ -62,7 +62,22 @@ If using connection pooling (transaction mode), disable prepared statements via DATABASE_PREPARED_STATEMENTS=false ``` -Related optional client tuning (driver defaults apply when unset): `DATABASE_POOL_MAX`, `DATABASE_IDLE_TIMEOUT_SECONDS`, `DATABASE_CONNECT_TIMEOUT_SECONDS`. +Related optional client tuning: `DATABASE_POOL_MAX`, `DATABASE_IDLE_TIMEOUT_SECONDS`, `DATABASE_CONNECT_TIMEOUT_SECONDS`, `DATABASE_MAX_LIFETIME_SECONDS`, `DATABASE_APPLICATION_NAME`. Driver defaults apply when unset, except that idle pooled connections close after 60 seconds (`DATABASE_IDLE_TIMEOUT_SECONDS=0` keeps them open) and the pool reports `application_name=paperclip`. See [Connection pool settings](#connection-pool-settings). + +## Connection Pool Settings + +The server opens one postgres.js pool for its own queries (and a second one when `DATABASE_MIGRATION_URL` points at a different connection). Every setting is optional: + +| Variable | Default | Effect | +|----------|---------|--------| +| `DATABASE_POOL_MAX` | `10` (driver) | Maximum pooled connections. | +| `DATABASE_IDLE_TIMEOUT_SECONDS` | `60` | Close a pooled connection after this much idle time. `0` keeps idle connections open forever (the driver default). | +| `DATABASE_CONNECT_TIMEOUT_SECONDS` | `30` (driver) | Give up on a connection attempt after this long. | +| `DATABASE_MAX_LIFETIME_SECONDS` | 30–60 min, randomized (driver) | Recycle a pooled connection once it is this old. | +| `DATABASE_APPLICATION_NAME` | `paperclip` | Value of `application_name` in `pg_stat_activity`, so you can find Paperclip's backends: `SELECT * FROM pg_stat_activity WHERE application_name = 'paperclip';` | +| `DATABASE_PREPARED_STATEMENTS` | `true` (driver) | Set `false` behind a transaction-mode pooler (see above). | + +The server ends its pools during shutdown (SIGINT/SIGTERM) and when startup fails after the pool was opened, so a restarting server does not leave idle backends behind. Size `max_connections` on the PostgreSQL side for at least `DATABASE_POOL_MAX` per server process plus your other clients. ## Switching Between Modes diff --git a/packages/db/src/client-options.test.ts b/packages/db/src/client-options.test.ts index 267d14dc17..a6fbce0058 100644 --- a/packages/db/src/client-options.test.ts +++ b/packages/db/src/client-options.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from "vitest"; -import { databaseClientOptionsFromEnv, postgresJsOptions } from "./client.js"; +import { + DEFAULT_DATABASE_APPLICATION_NAME, + DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS, + databaseClientOptionsFromEnv, + postgresJsOptions, + resolveDatabaseClientOptions, +} from "./client.js"; describe("databaseClientOptionsFromEnv", () => { it("returns no options when nothing is set, preserving driver defaults", () => { @@ -29,8 +35,20 @@ describe("databaseClientOptionsFromEnv", () => { DATABASE_POOL_MAX: "25", DATABASE_IDLE_TIMEOUT_SECONDS: "60", DATABASE_CONNECT_TIMEOUT_SECONDS: "10", + DATABASE_MAX_LIFETIME_SECONDS: "1800", + DATABASE_APPLICATION_NAME: " paperclip-web ", }), - ).toEqual({ maxConnections: 25, idleTimeoutSeconds: 60, connectTimeoutSeconds: 10 }); + ).toEqual({ + maxConnections: 25, + idleTimeoutSeconds: 60, + connectTimeoutSeconds: 10, + maxLifetimeSeconds: 1800, + applicationName: "paperclip-web", + }); + }); + + it("accepts DATABASE_IDLE_TIMEOUT_SECONDS=0 as an explicit opt-out of idle reaping", () => { + expect(databaseClientOptionsFromEnv({ DATABASE_IDLE_TIMEOUT_SECONDS: "0" })).toEqual({ idleTimeoutSeconds: 0 }); }); it("rejects malformed values instead of silently ignoring them", () => { @@ -42,6 +60,18 @@ describe("databaseClientOptionsFromEnv", () => { expect(() => databaseClientOptionsFromEnv({ DATABASE_CONNECT_TIMEOUT_SECONDS: "1.5" })).toThrow( /DATABASE_CONNECT_TIMEOUT_SECONDS/, ); + expect(() => databaseClientOptionsFromEnv({ DATABASE_IDLE_TIMEOUT_SECONDS: "-1" })).toThrow( + /DATABASE_IDLE_TIMEOUT_SECONDS/, + ); + expect(() => databaseClientOptionsFromEnv({ DATABASE_IDLE_TIMEOUT_SECONDS: "abc" })).toThrow( + /DATABASE_IDLE_TIMEOUT_SECONDS/, + ); + expect(() => databaseClientOptionsFromEnv({ DATABASE_MAX_LIFETIME_SECONDS: "0" })).toThrow( + /DATABASE_MAX_LIFETIME_SECONDS/, + ); + expect(() => databaseClientOptionsFromEnv({ DATABASE_MAX_LIFETIME_SECONDS: "NaN" })).toThrow( + /DATABASE_MAX_LIFETIME_SECONDS/, + ); }); it("maps to postgres.js option names", () => { @@ -51,7 +81,42 @@ describe("databaseClientOptionsFromEnv", () => { maxConnections: 25, idleTimeoutSeconds: 60, connectTimeoutSeconds: 10, + maxLifetimeSeconds: 1800, + applicationName: "paperclip-web", }), - ).toEqual({ prepare: false, max: 25, idle_timeout: 60, connect_timeout: 10 }); + ).toEqual({ + prepare: false, + max: 25, + idle_timeout: 60, + connect_timeout: 10, + max_lifetime: 1800, + connection: { application_name: "paperclip-web" }, + }); + }); +}); + +describe("resolveDatabaseClientOptions", () => { + it("reaps idle connections and names the pool when the environment sets nothing", () => { + expect(resolveDatabaseClientOptions({})).toEqual({ + idleTimeoutSeconds: DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS, + applicationName: DEFAULT_DATABASE_APPLICATION_NAME, + }); + expect(postgresJsOptions(resolveDatabaseClientOptions(databaseClientOptionsFromEnv({})))).toEqual({ + idle_timeout: DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS, + connection: { application_name: DEFAULT_DATABASE_APPLICATION_NAME }, + }); + }); + + it("keeps every explicit value, including an idle timeout of 0", () => { + expect( + resolveDatabaseClientOptions({ + maxConnections: 3, + idleTimeoutSeconds: 0, + applicationName: "paperclip-cli", + }), + ).toEqual({ maxConnections: 3, idleTimeoutSeconds: 0, applicationName: "paperclip-cli" }); + expect(postgresJsOptions(resolveDatabaseClientOptions({ idleTimeoutSeconds: 0 }))).toMatchObject({ + idle_timeout: 0, + }); }); }); diff --git a/packages/db/src/client.test.ts b/packages/db/src/client.test.ts index d5e166890f..52a66676d2 100644 --- a/packages/db/src/client.test.ts +++ b/packages/db/src/client.test.ts @@ -3,7 +3,9 @@ import fs from "node:fs"; import { afterEach, describe, expect, it } from "vitest"; import postgres from "postgres"; import { + DEFAULT_DATABASE_APPLICATION_NAME, applyPendingMigrations, + createDb, inspectMigrations, resetPostgresDatabase, } from "./client.js"; @@ -89,6 +91,48 @@ if (!embeddedPostgresSupport.supported) { ); } +describeEmbeddedPostgres("createDb pool defaults", () => { + it("names its backends and closes them once idle", async () => { + const url = await createTempDatabase(); + const observer = postgres(url, { max: 1, onnotice: () => {} }); + cleanups.push(async () => { + await observer.end({ timeout: 1 }); + }); + + const backendsNamed = async (name: string) => { + const rows = await observer` + select count(*)::int as count from pg_stat_activity where application_name = ${name} + `; + return rows[0]?.count ?? 0; + }; + + const db = createDb(url); + cleanups.push(async () => { + await db.$client.end({ timeout: 1 }); + }); + const [self] = await db.$client`select application_name from pg_stat_activity where pid = pg_backend_pid()`; + expect(self?.application_name).toBe(DEFAULT_DATABASE_APPLICATION_NAME); + + const shortLived = createDb(url, { applicationName: "paperclip-idle-test", idleTimeoutSeconds: 1 }); + cleanups.push(async () => { + await shortLived.$client.end({ timeout: 1 }); + }); + await shortLived.$client`select 1`; + expect(await backendsNamed("paperclip-idle-test")).toBe(1); + + // The driver closes the idle connection after `idle_timeout`; without the + // option (the driver default) the backend would stay until the process + // exits. Wait past the timeout, then poll PostgreSQL's own view. + const deadline = Date.now() + 10_000; + let remaining = await backendsNamed("paperclip-idle-test"); + while (remaining > 0 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 250)); + remaining = await backendsNamed("paperclip-idle-test"); + } + expect(remaining).toBe(0); + }, 30_000); +}); + describeEmbeddedPostgres("resetPostgresDatabase", () => { it("recreates an existing database so stale tables are removed", async () => { const connectionString = await createTempDatabase(); diff --git a/packages/db/src/client.ts b/packages/db/src/client.ts index 75a988aa88..ed7de6716f 100644 --- a/packages/db/src/client.ts +++ b/packages/db/src/client.ts @@ -142,8 +142,32 @@ export interface DatabaseClientOptions { idleTimeoutSeconds?: number; /** postgres.js `connect_timeout` in seconds (driver default: 30). */ connectTimeoutSeconds?: number; + /** + * postgres.js `max_lifetime` in seconds. Bounds how long one pooled + * connection is reused before the client replaces it (driver default: a + * random value between 30 and 60 minutes). + */ + maxLifetimeSeconds?: number; + /** + * postgres.js `connection.application_name`, shown in + * `pg_stat_activity.application_name`. Lets an operator tell Paperclip's + * pool apart from other clients of the same database (driver default: + * `postgres.js`). + */ + applicationName?: string; } +/** + * Idle pooled connections close after this many seconds unless + * `DATABASE_IDLE_TIMEOUT_SECONDS` says otherwise. The driver default keeps an + * idle connection open forever, so a process that stops issuing queries still + * holds every backend it ever opened. Set `DATABASE_IDLE_TIMEOUT_SECONDS=0` + * to restore the driver default. + */ +export const DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS = 60; +/** `application_name` reported to PostgreSQL unless `DATABASE_APPLICATION_NAME` overrides it. */ +export const DEFAULT_DATABASE_APPLICATION_NAME = "paperclip"; + function envBoolean(env: NodeJS.ProcessEnv, name: string): boolean | undefined { const value = env[name]?.trim().toLowerCase(); if (value === undefined || value === "") return undefined; @@ -161,12 +185,28 @@ function envPositiveInteger(env: NodeJS.ProcessEnv, name: string): number | unde return Number.parseInt(value, 10); } +function envNonNegativeInteger(env: NodeJS.ProcessEnv, name: string): number | undefined { + const value = env[name]?.trim(); + if (value === undefined || value === "") return undefined; + if (!/^(?:0|[1-9]\d*)$/.test(value)) { + throw new Error(`${name} must be a non-negative integer, got: ${env[name]}`); + } + return Number.parseInt(value, 10); +} + +function envNonEmptyString(env: NodeJS.ProcessEnv, name: string): string | undefined { + const value = env[name]?.trim(); + if (value === undefined || value === "") return undefined; + return value; +} + /** * Database client tuning from the environment, so hosted deployments can * adapt to their connection topology (pooled endpoints, network latency) - * without editing source. Every variable is optional; when unset the - * driver defaults apply and behavior is identical to a bare - * `postgres(url)` — self-hosted setups need none of these. + * without editing source. Every variable is optional. This function returns + * only the values the environment sets; `resolveDatabaseClientOptions` adds + * Paperclip's own defaults on top, and the driver defaults apply to the rest + * — self-hosted setups need none of these. */ export function databaseClientOptionsFromEnv(env: NodeJS.ProcessEnv = process.env): DatabaseClientOptions { const options: DatabaseClientOptions = {}; @@ -174,24 +214,48 @@ export function databaseClientOptionsFromEnv(env: NodeJS.ProcessEnv = process.en if (prepare !== undefined) options.prepare = prepare; const maxConnections = envPositiveInteger(env, "DATABASE_POOL_MAX"); if (maxConnections !== undefined) options.maxConnections = maxConnections; - const idleTimeoutSeconds = envPositiveInteger(env, "DATABASE_IDLE_TIMEOUT_SECONDS"); + // `0` is allowed here: it disables idle reaping (the driver default). + const idleTimeoutSeconds = envNonNegativeInteger(env, "DATABASE_IDLE_TIMEOUT_SECONDS"); if (idleTimeoutSeconds !== undefined) options.idleTimeoutSeconds = idleTimeoutSeconds; const connectTimeoutSeconds = envPositiveInteger(env, "DATABASE_CONNECT_TIMEOUT_SECONDS"); if (connectTimeoutSeconds !== undefined) options.connectTimeoutSeconds = connectTimeoutSeconds; + const maxLifetimeSeconds = envPositiveInteger(env, "DATABASE_MAX_LIFETIME_SECONDS"); + if (maxLifetimeSeconds !== undefined) options.maxLifetimeSeconds = maxLifetimeSeconds; + const applicationName = envNonEmptyString(env, "DATABASE_APPLICATION_NAME"); + if (applicationName !== undefined) options.applicationName = applicationName; return options; } +/** + * Fills in Paperclip's defaults for the options the caller left unset: idle + * connections are reaped after `DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS`, and the + * pool identifies itself as `DEFAULT_DATABASE_APPLICATION_NAME`. Everything + * else stays at the driver default. An explicit value (including + * `idleTimeoutSeconds: 0`) always wins over the default. + */ +export function resolveDatabaseClientOptions(options: DatabaseClientOptions): DatabaseClientOptions { + return { + ...options, + idleTimeoutSeconds: options.idleTimeoutSeconds ?? DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS, + applicationName: options.applicationName ?? DEFAULT_DATABASE_APPLICATION_NAME, + }; +} + export function postgresJsOptions(options: DatabaseClientOptions): Record { const driverOptions: Record = {}; if (options.prepare !== undefined) driverOptions.prepare = options.prepare; if (options.maxConnections !== undefined) driverOptions.max = options.maxConnections; if (options.idleTimeoutSeconds !== undefined) driverOptions.idle_timeout = options.idleTimeoutSeconds; if (options.connectTimeoutSeconds !== undefined) driverOptions.connect_timeout = options.connectTimeoutSeconds; + if (options.maxLifetimeSeconds !== undefined) driverOptions.max_lifetime = options.maxLifetimeSeconds; + if (options.applicationName !== undefined) { + driverOptions.connection = { application_name: options.applicationName }; + } return driverOptions; } export function createDb(url: string, options?: DatabaseClientOptions) { - const resolved = options ?? databaseClientOptionsFromEnv(); + const resolved = resolveDatabaseClientOptions(options ?? databaseClientOptionsFromEnv()); const sql = postgres(url, postgresJsOptions(resolved)); const key = hostPortKeyOrNull(url); if (key) registerClient(key, sql); diff --git a/server/src/app.ts b/server/src/app.ts index 3ca53ecc80..2940a9d665 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -982,6 +982,9 @@ export async function createApp( scheduler.stop(); jobCoordinator.stop(); disableFeedbackExportFlushes(); + // The scheduler tick queries the database. Stop it here, inside the + // awaited teardown, so no tick runs after the caller ends the pool. + scheduler.stop(); if (importTransferSweepTimer) { clearInterval(importTransferSweepTimer); importTransferSweepTimer = null; diff --git a/server/src/index.ts b/server/src/index.ts index a4087526b3..13725d74a3 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -104,6 +104,7 @@ import { conflict } from "./errors.js"; import { ensureDecisionSigningSecret } from "./services/decision-signing.js"; import { createDecisionRetentionNotifyOriginAgent, createDecisionWakeOriginAgent } from "./services/decision-wakeup.js"; import { + closeHttpListenerForShutdown, coordinateHeartbeatSchedulerShutdown, drainRunExecutionFinalizersForShutdown, finalizeServerShutdown, @@ -158,7 +159,40 @@ export interface StartedServer { shutdown: (signal?: "SIGINT" | "SIGTERM") => Promise; } +// Set by the boot sequence once the primary pool exists. A boot that fails +// after that point (a bootstrap query that throws, for example) must end the +// pool before the caller exits: the driver keeps idle connections open until +// the process dies, and in a restart loop the leftover backends of every +// failed generation can exhaust `max_connections` before the next boot even +// gets a connection. +type StartupDatabaseTeardown = { close: (() => Promise) | null }; + +// Ends the pool behind a drizzle client. Tolerates a client without `$client` +// (test doubles) and never throws, so it is safe on every exit path. +async function endDatabaseClient(client: unknown, timeoutSeconds: number): Promise { + const sql = (client as { $client?: { end?: (options?: { timeout?: number }) => Promise } } | null) + ?.$client; + if (typeof sql?.end !== "function") return; + await sql.end({ timeout: timeoutSeconds }); +} + export async function startServer(): Promise { + const startupDatabase: StartupDatabaseTeardown = { close: null }; + try { + return await startServerWithDatabaseTeardown(startupDatabase); + } catch (error) { + if (startupDatabase.close) { + await startupDatabase.close().catch((closeError) => { + logger.error({ err: closeError }, "failed to close database clients after startup failure"); + }); + } + throw error; + } +} + +async function startServerWithDatabaseTeardown( + startupDatabase: StartupDatabaseTeardown, +): Promise { setStartupRecoveryPhase("starting"); warnIfUnsupportedNodeVersion(process.versions.node, (message) => logger.warn(message)); @@ -586,6 +620,15 @@ export async function startServer(): Promise { resolvedEmbeddedPostgresPort = port; startupDbInfo = { mode: "embedded-postgres", dataDir, port }; } + + // Ends every pool this process opened. Used by the orderly shutdown path + // (after the application services, before the embedded provider stops) and + // by the fail-loud startup path, so no exit leaves pooled backends behind. + const closeDatabaseClients = async () => { + const clients = pluginMigrationDb === db ? [db] : [db, pluginMigrationDb]; + await Promise.all(clients.map((client) => endDatabaseClient(client, 5))); + }; + startupDatabase.close = closeDatabaseClients; // A claimed warm-pool stack may restart while its provider environment still // names the pool host. Restore the signed, durable identity before Better @@ -1883,9 +1926,16 @@ export async function startServer(): Promise { // setup-token login session must stop and release its sandbox lease before // the database and the provider stop, so an orderly shutdown never leaves a // sandbox lease or confidential login state alive past the process exit. + // The HTTP listener closes first, while every service is still up, so a + // request in flight is drained against a working server and none reaches + // a route once the pool is gone; the programmatic close below then finds + // the listener already closed and skips. await finalizeServerShutdown({ signal, shutdownAppServices: appShutdown, + closeHttpListener: () => + closeHttpListenerForShutdown({ server, signal, log: logger }), + closeDatabase: closeDatabaseClients, stopEmbeddedPostgres, shutdownInstrumentation, shutdownSentry, diff --git a/server/src/shutdown.test.ts b/server/src/shutdown.test.ts index c3500b3a88..4a4c13bf12 100644 --- a/server/src/shutdown.test.ts +++ b/server/src/shutdown.test.ts @@ -1,6 +1,7 @@ import { EventEmitter } from "node:events"; import { describe, expect, it, vi } from "vitest"; import { + closeHttpListenerForShutdown, coordinateHeartbeatSchedulerShutdown, drainRunExecutionFinalizersForShutdown, finalizeServerShutdown, @@ -32,6 +33,12 @@ describe("finalizeServerShutdown", () => { await release.promise; order.push("appServices:settled"); }); + const closeHttpListener = vi.fn(async () => { + order.push("listener:close"); + }); + const closeDatabase = vi.fn(async () => { + order.push("database:close"); + }); const stopEmbeddedPostgres = vi.fn(async () => { order.push("postgres:stop"); }); @@ -46,6 +53,8 @@ describe("finalizeServerShutdown", () => { const finalize = finalizeServerShutdown({ signal: "SIGTERM", shutdownAppServices, + closeHttpListener, + closeDatabase, stopEmbeddedPostgres, shutdownInstrumentation, shutdownSentry, @@ -59,6 +68,10 @@ describe("finalizeServerShutdown", () => { // The cleanup is in flight. The database stop, the instrumentation flush, // and the process exit continuation must all wait for it to settle. await vi.waitFor(() => expect(shutdownAppServices).toHaveBeenCalledOnce()); + // The listener already closed: requests are drained while every service + // is still available, and nothing after this point can be reached. + expect(closeHttpListener).toHaveBeenCalledOnce(); + expect(closeDatabase).not.toHaveBeenCalled(); expect(stopEmbeddedPostgres).not.toHaveBeenCalled(); expect(shutdownInstrumentation).not.toHaveBeenCalled(); expect(exited).toBe(false); @@ -68,8 +81,10 @@ describe("finalizeServerShutdown", () => { expect(exited).toBe(true); expect(order).toEqual([ + "listener:close", "appServices:start", "appServices:settled", + "database:close", "postgres:stop", "instrumentation:flush", "sentry:flush", @@ -125,6 +140,35 @@ describe("finalizeServerShutdown", () => { expect(exited).toBe(true); }); + it("logs a failed database close and still stops the provider and exits", async () => { + const order: string[] = []; + const closeError = new Error("pool end timed out"); + const closeDatabase = vi.fn(async () => { + order.push("database:close"); + throw closeError; + }); + const stopEmbeddedPostgres = vi.fn(async () => { + order.push("postgres:stop"); + }); + const log = stubLogger(); + + await finalizeServerShutdown({ + signal: "SIGTERM", + shutdownAppServices: vi.fn(async () => undefined), + closeDatabase, + stopEmbeddedPostgres, + shutdownInstrumentation: vi.fn(async () => undefined), + shutdownSentry: vi.fn(async () => undefined), + log, + }); + + expect(order).toEqual(["database:close", "postgres:stop"]); + expect(log.error).toHaveBeenCalledWith( + expect.objectContaining({ err: closeError, signal: "SIGTERM" }), + "Database client shutdown failed", + ); + }); + it("skips the database stop when no embedded PostgreSQL runs in this process", async () => { const shutdownAppServices = vi.fn(async () => undefined); const shutdownInstrumentation = vi.fn(async () => undefined); @@ -146,6 +190,54 @@ describe("finalizeServerShutdown", () => { }); }); +describe("closeHttpListenerForShutdown", () => { + function fakeServer(input: { listening: boolean; closeDelayMs?: number | null }) { + const closeIdleConnections = vi.fn(); + const closeAllConnections = vi.fn(); + const close = vi.fn((callback?: (err?: Error) => void) => { + if (input.closeDelayMs === null) return; + setTimeout(() => callback?.(), input.closeDelayMs ?? 0); + }); + return { listening: input.listening, close, closeIdleConnections, closeAllConnections }; + } + + it("stops accepting, closes idle keep-alive sockets, and resolves once the listener closed", async () => { + const server = fakeServer({ listening: true, closeDelayMs: 0 }); + await expect( + closeHttpListenerForShutdown({ server, signal: "SIGTERM", timeoutMs: 1_000, log: stubLogger() }), + ).resolves.toBe("closed"); + expect(server.close).toHaveBeenCalledOnce(); + expect(server.closeIdleConnections).toHaveBeenCalledOnce(); + expect(server.closeAllConnections).not.toHaveBeenCalled(); + }); + + it("closes the remaining connections when the drain outlives the grace period", async () => { + vi.useFakeTimers(); + try { + const server = fakeServer({ listening: true, closeDelayMs: null }); + const log = stubLogger(); + const pending = closeHttpListenerForShutdown({ server, signal: "SIGINT", timeoutMs: 250, log }); + await vi.advanceTimersByTimeAsync(250); + await expect(pending).resolves.toBe("timed_out"); + expect(server.closeAllConnections).toHaveBeenCalledOnce(); + expect(log.info).toHaveBeenCalledWith( + expect.objectContaining({ timeoutMs: 250 }), + expect.stringContaining("timed out"), + ); + } finally { + vi.useRealTimers(); + } + }); + + it("does nothing when the listener was never bound", async () => { + const server = fakeServer({ listening: false }); + await expect( + closeHttpListenerForShutdown({ server, signal: "SIGTERM", log: stubLogger() }), + ).resolves.toBe("not_listening"); + expect(server.close).not.toHaveBeenCalled(); + }); +}); + describe("drainRunExecutionFinalizersForShutdown", () => { it("awaits bounded execution finalizers", async () => { const release = deferred(); diff --git a/server/src/shutdown.ts b/server/src/shutdown.ts index adffd4dc6d..a76501b9f3 100644 --- a/server/src/shutdown.ts +++ b/server/src/shutdown.ts @@ -36,6 +36,58 @@ export async function drainRunExecutionFinalizersForShutdown(input: { } } +type ShutdownHttpListener = { + listening: boolean; + close(callback?: (err?: Error) => void): unknown; + closeIdleConnections?: () => void; + closeAllConnections?: () => void; +}; + +/** + * Stops the HTTP listener from accepting new requests and waits, for at most + * `timeoutMs`, for the open connections to finish. Idle keep-alive sockets + * close at once; whatever is still open when the grace period ends is closed + * forcibly, so the teardown never hangs on a long-lived client. Call this + * before the database pool ends, so no request can reach a route after + * `sql.end()` and fail with a connection-ended error. + */ +export async function closeHttpListenerForShutdown(input: { + server: ShutdownHttpListener; + signal: "SIGINT" | "SIGTERM"; + timeoutMs?: number; + log: ShutdownLogger; +}): Promise<"closed" | "timed_out" | "not_listening"> { + if (!input.server.listening) return "not_listening"; + const timeoutMs = input.timeoutMs ?? 5_000; + let timer: NodeJS.Timeout | null = null; + try { + return await Promise.race([ + new Promise<"closed">((resolve) => { + input.server.close((err) => { + if (err && (err as NodeJS.ErrnoException).code !== "ERR_SERVER_NOT_RUNNING") { + input.log.error({ err, signal: input.signal }, "HTTP listener close failed"); + } + resolve("closed"); + }); + input.server.closeIdleConnections?.(); + }), + new Promise<"timed_out">((resolve) => { + timer = setTimeout(() => { + input.log.info( + { signal: input.signal, timeoutMs }, + "HTTP listener drain timed out; closing the remaining connections", + ); + input.server.closeAllConnections?.(); + resolve("timed_out"); + }, timeoutMs); + timer.unref?.(); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + /** * Runs the final, ordered teardown of the server. It awaits the application * service cleanup first, so a live setup-token login session stops and releases @@ -52,6 +104,20 @@ export async function drainRunExecutionFinalizersForShutdown(input: { export async function finalizeServerShutdown(input: { signal: "SIGINT" | "SIGTERM"; shutdownAppServices: (() => Promise) | undefined; + /** + * Stops the HTTP listener and drains its connections (see + * `closeHttpListenerForShutdown`). Runs first, while every application + * service is still available to the requests being drained, so no request + * runs against a half-dismantled service or an ended pool. + */ + closeHttpListener?: (() => Promise) | null; + /** + * Ends the server's PostgreSQL client pools. Runs after the application + * services (which still need the database) and before the embedded + * provider stops, so the backends close in order and none outlive the + * process. + */ + closeDatabase?: (() => Promise) | null; stopEmbeddedPostgres: (() => Promise) | null; shutdownInstrumentation: () => Promise; shutdownSentry: () => Promise; @@ -59,6 +125,16 @@ export async function finalizeServerShutdown(input: { }): Promise { const { signal } = input; + // Stop accepting requests and drain the open ones before any service goes + // away, so a request that is still in flight sees a fully working server. + if (input.closeHttpListener) { + try { + await input.closeHttpListener(); + } catch (err) { + input.log.error({ err, signal }, "HTTP listener shutdown failed"); + } + } + // Await the application service cleanup, so a live setup-token login session // releases its sandbox lease before the database and the provider stop. A // rejected cleanup stays durable for the reaper; it does not block the exit. @@ -68,6 +144,18 @@ export async function finalizeServerShutdown(input: { input.log.error({ err, signal }, "Application service shutdown failed"); } + // End the client pools once nothing needs them any more. Without this the + // process exit leaves the pooled backends to PostgreSQL's own TCP keepalive + // reaping, and a restart loop can pile up enough of them to hit + // `max_connections` before the next boot gets a connection. + if (input.closeDatabase) { + try { + await input.closeDatabase(); + } catch (err) { + input.log.error({ err, signal }, "Database client shutdown failed"); + } + } + if (input.stopEmbeddedPostgres) { input.log.info({ signal }, "Stopping embedded PostgreSQL"); try {