From 023e640a7ee3b32b1da4de52c4d3804e749e6390 Mon Sep 17 00:00:00 2001 From: Jannes Stubbemann Date: Tue, 8 Sep 2026 17:56:39 +0200 Subject: [PATCH] fix(db): reap idle pool connections, name the pool, and end it on shutdown (#12956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The server keeps one postgres.js pool (`packages/db/src/client.ts`, `createDb`) for every query it runs. #10795 made the pool tunable from the environment, but the defaults stayed at the driver defaults: an idle connection never closes, the pool reports itself as `postgres.js`, and no code path ever calls `sql.end()`. > - On a hosted Paperclip deployment the server entered a restart loop (a bundled plugin failure that #12953 describes made every run fail, and the pool saturated). Each generation opened its ten connections, died, and left the backends open on the PostgreSQL side until TCP keepalive reaped them hours later. After about 20 generations the backends exceeded `max_connections`, and every later boot died on its first bootstrap query with `sorry, too many clients already`, before `server.listen()`. The loop could not heal itself. #9555 describes the same shape on a launchd-supervised self-hosted install. > - Three properties of the pool combine to make this possible: idle connections are never reaped, the pool is never ended on any exit path, and an operator cannot even find the leaked backends in `pg_stat_activity` because they carry the generic driver name. > - This pull request gives the pool a 60 second idle timeout and the `paperclip` application name by default, exposes `max_lifetime` and `application_name` through the same `DATABASE_*` environment contract that #10795 introduced, and ends the pool on the orderly SIGINT/SIGTERM path and on the fail-loud startup path. > - The benefit is that a restarting or crash-looping server releases its backends instead of accumulating them, and an operator can see and count Paperclip's connections. ## Linked Issues or Issue Description - Refs #9555 — database connection pool leak causes an infinite restart loop under load. This PR closes the "pool never ends, idle connections never close" part of that report. - Refs #12953 — hosted outage report. The pool exhaustion is the second half of that incident; the first half (a stuck sandbox provider plugin) has its own PR. - Related prior PRs: #9597 and #8780 both propose hard-coded `idle_timeout` / `max_lifetime` values in `createDb`. Both predate #10795 (merged), which made these options environment-driven; this PR builds on the merged shape and adds the shutdown `end()` that neither covers. #4006 and #7481 are closed earlier attempts in the same area. ## What Changed - `packages/db/src/client.ts` - New `resolveDatabaseClientOptions()` applies Paperclip defaults on top of the environment: `idleTimeoutSeconds` defaults to 60 (`DEFAULT_DATABASE_IDLE_TIMEOUT_SECONDS`) and `applicationName` to `paperclip` (`DEFAULT_DATABASE_APPLICATION_NAME`). `createDb` uses it for both the environment path and explicit options. - `DATABASE_IDLE_TIMEOUT_SECONDS` now accepts `0` to restore the driver default (keep idle connections open). Negative or non-integer values still throw. - New environment variables: `DATABASE_MAX_LIFETIME_SECONDS` (positive integer, maps to `max_lifetime`) and `DATABASE_APPLICATION_NAME` (non-empty string, maps to `connection.application_name`). - `postgresJsOptions()` maps the two new options. - `server/src/shutdown.ts` - `finalizeServerShutdown` gains two optional ordered steps: `closeHttpListener` runs first, before the application services stop; `closeDatabase` runs after the application services and before the embedded PostgreSQL stop. A failure in either is logged and does not stop the teardown. Final order: listener → application services → database pool → embedded PostgreSQL → instrumentation → Sentry. - New `closeHttpListenerForShutdown()`: stops accepting requests, closes idle keep-alive sockets, waits up to 5 s for open connections, then closes whatever is left. Requests still in flight are drained while every service is available, and none can reach a route after `sql.end()`, on the signal path and the programmatic path alike (the programmatic path's later `server.close` finds the listener closed and skips). - `server/src/app.ts`: the app shutdown hook (`shutdownAppServices`) now stops the plugin job scheduler, whose tick queries the database, so a programmatic `shutdown()` leaves no timer running against the ended pool. - `server/src/index.ts` - `startServer()` is now a thin wrapper around the boot sequence. When the boot sequence throws after the pool exists, the wrapper ends the pool (and the separate migration pool, when configured) before it rethrows. This covers the `process.exit(1)` path in the main module and the CLI `paperclip run` path alike. - The orderly shutdown passes the same `closeDatabaseClients` to `finalizeServerShutdown`. - `endDatabaseClient` tolerates a client without `$client` (test doubles) and uses a 5 second end timeout. - Docs: `docs/deploy/database.md` gets a "Connection Pool Settings" table with every `DATABASE_*` pool variable, its default and its effect; `doc/DATABASE.md` lists the two new variables. - Tests - `packages/db/src/client-options.test.ts`: parsing of the new variables, `0` for the idle timeout, rejection of malformed values, driver option mapping, and the `resolveDatabaseClientOptions` defaults. - `packages/db/src/client.test.ts` (embedded PostgreSQL): `createDb(url)` reports `application_name = paperclip` for its own backend, and a pool with `idleTimeoutSeconds: 1` has zero backends in `pg_stat_activity` after the timeout. - `server/src/shutdown.test.ts`: the listener closes before the application services, and the database close runs between the application services and the embedded PostgreSQL stop; a failing database close is logged while the teardown still finishes; `closeHttpListenerForShutdown` closes idle sockets and resolves on close, force-closes after the grace period, and is a no-op when the listener was never bound. ## Verification - `pnpm --filter @paperclipai/db typecheck` — passes (`check:migrations` + `tsc --noEmit`). - `cd server && pnpm typecheck` — passes. - `cd packages/db && pnpm exec vitest run src/client-options.test.ts src/client.test.ts src/client-teardown-registry.test.ts` — 9 + 18 + 3 tests pass (the `client.test.ts` cases need embedded PostgreSQL; the new one waits up to 10 s for the idle reap and passed in about 3 s). - `cd server && pnpm exec vitest run src/shutdown.test.ts src/__tests__/server-startup-feedback-export.test.ts src/__tests__/bootstrap-claim-routes.test.ts` — 34 + 11 tests pass. The startup-feedback suite exercises `startServer()` with a mocked `createDb`, which is why `endDatabaseClient` tolerates a client without `$client`. - Manual check for a reviewer: start the server against any PostgreSQL, then run `SELECT application_name, state, count(*) FROM pg_stat_activity GROUP BY 1, 2;`. Paperclip's backends now show `paperclip`. Leave the server idle for more than 60 s and the idle backends disappear. Send SIGTERM and the backends close before the process exits. ## Risks - Behavior change with no environment set: idle pooled connections now close after 60 s. The next query after an idle period pays a reconnect (single-digit milliseconds on a local socket). postgres.js reconnects transparently. Set `DATABASE_IDLE_TIMEOUT_SECONDS=0` to keep the previous behavior. - `application_name` changes from `postgres.js` to `paperclip`. Anything that filtered `pg_stat_activity` on the old name would need an update; nothing in this repo does. - The HTTP listener now closes at the start of the final teardown (after the heartbeat run drain, which still needs the API for running agents). The pool close runs after the application services. A late query from a timer that survived the service shutdown would fail with a driver "connection ended" error instead of running; the known database-backed timer (the plugin job scheduler) is now stopped in the service shutdown. - The listener drain adds at most 5 s to a shutdown while long-lived connections (for example WebSocket clients) are open; after that they are closed forcibly. - `startServer()` is split into a wrapper and the boot sequence. The exported signature and return type are unchanged. - No migration, no schema change. ## Model Used - Claude Fable 5.1 (`claude-fable-5-1`) via Claude Code, extended thinking, tool use (file edits, shell, test runs). The change was produced with the model and reviewed by the submitting human. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_014t3bi2beVNVVHAxK36dmXm --------- Co-authored-by: Claude Fable 5.1 --- doc/DATABASE.md | 4 +- docs/deploy/database.md | 17 ++++- packages/db/src/client-options.test.ts | 71 +++++++++++++++++++- packages/db/src/client.test.ts | 44 ++++++++++++ packages/db/src/client.ts | 74 +++++++++++++++++++-- server/src/app.ts | 3 + server/src/index.ts | 50 ++++++++++++++ server/src/shutdown.test.ts | 92 ++++++++++++++++++++++++++ server/src/shutdown.ts | 88 ++++++++++++++++++++++++ 9 files changed, 433 insertions(+), 10 deletions(-) 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 {