Skip to content

fix(db-postgres): release the client checked out during connect - #17831

Open
meikocho1 wants to merge 1 commit into
payloadcms:mainfrom
meikocho1:fix/db-postgres-release-bootstrap-client
Open

fix(db-postgres): release the client checked out during connect#17831
meikocho1 wants to merge 1 commit into
payloadcms:mainfrom
meikocho1:fix/db-postgres-release-bootstrap-client

Conversation

@meikocho1

Copy link
Copy Markdown

What?

@payloadcms/db-postgres checks out one client from the pool during connect and never releases it. This releases it, and adds a regression test asserting the pool has no checked-out clients after connecting.

Why?

connectWithReconnect acquires a client, attaches an error listener to it, and returns without calling release():

// packages/db-postgres/src/connect.ts
result = await pool.connect()
...
result.prependListener('error', (err) => { /* reconnect on ECONNRESET */ })
// returns here — the client is never released

A checked-out client is never handed back to the pool, so it is pinned for the lifetime of the process. Two consequences:

  1. pool.end() never drains. It waits for every checked-out client to come back, so it hangs. The reporter of db-postgres: payload.destroy leaves one checked-out pg client, causing pool.end() timeout #15674 measured exactly this — one client outstanding before and after payload.destroy(), then a pool.end() timeout:

    after-init            { idle: 8, total: 9, waiting: 0 }
    after-payload-destroy { idle: 8, total: 9, waiting: 0 }
    pool.end timeout      { elapsedMs: 3006, timeoutMs: 3000 }
    
  2. One connection is permanently unavailable for queries, on the primary pool and on every read-replica pool, since connect runs connectWithReconnect for each of them.

This also matters for payload jobs:run, where payload.destroy() is called specifically to "close database connections after running jobs so process can exit cleanly" (packages/payload/src/bin/index.ts).

Why not end the pool in destroy instead? That was my first thought, and it is wrong. reload() in packages/payload/src/index.ts calls payload.db.destroy() on every hot reload and then reconnects, and connect guards its pool creation with if (!this.pool) — so the pool is deliberately reused across hot reloads. Ending it in destroy would leave dev with a pool that has been shut down. The leak is in the checkout, so that is where it is fixed.

await pool.connect() itself is load-bearing and stays: connect relies on the error it throws to detect a missing database and branch into createDatabase(). Only the checkout is released; the error listener stays attached to the client instance, so the ECONNRESET reconnect path is unchanged.

How?

One result.release() after the listener is attached, plus a test in test/database/int.spec.ts asserting pool.totalCount - pool.idleCount === 0 once the adapter is connected — scoped to the adapters that use pg.Pool.

Verified against the Postgres suite (pnpm test:int:postgres database):

  • Without the fix, the new test fails with expected 1 to be +0 — one client outstanding, matching the reported measurement exactly.
  • With the fix it passes, and the full suite is green: 200 passed, 36 skipped.

Fixes #15674
Fixes #11727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant