Smashing a silent connection leak in node-postgres (pg-pool)

This is a submission for DEV's Summer Bug Smash: Clear the Lineup powered by Sentry.

I fixed a real, open performance/correctness bug in pg-pool (the connection pool that ships with
node-postgres, ~12.5k ⭐, downloaded tens of
millions of times a week), reproduced it against a real Postgres, hardened the fix with
Sentry and Google's Gemini in the loop, and added a regression test.

Issue: brianc/node-postgres#3543
PR: brianc/node-postgres#3711

Project Overview

node-postgres (pg) is the most widely used PostgreSQL client for Node.js. Almost everyone
who talks to Postgres from Node uses its built-in connection pool, pg-pool, directly or
through an ORM (Sequelize, TypeORM, Prisma's pg adapter, Knex…). When you set
connectionTimeoutMillis, you are telling the pool: "if getting me a connection takes longer
than this, give up and error." That guarantee is what this bug quietly breaks.

Bug Fix or Performance Improvement

Issue #3543 reports that when connectionTimeoutMillis fires while a new connection is being
established, a connection can still be opened on the Postgres side and is never cleaned up — the
pool keeps holding it, and with the native (pg-native) client the process can crash with exit
code 13.

The culprit is in Pool.newClient():

tid = setTimeout(() => {
  if (client.connection) {
    timeoutHit = true
    client.connection.stream.destroy()   // rip out the local socket
  } else if (!client.isConnected()) {
    timeoutHit = true
    client.end()
  }
}, this.options.connectionTimeoutMillis)

client.connect((err) => {
  if (tid) clearTimeout(tid)
  client.on('error', idleListener)
  if (err) { /* remove client, reject */ }
  else {
    // success path — never checks whether the timeout already fired
    return this._afterConnect(client, pendingItem, idleListener)
  }
})

Two things are wrong:

1. The success path never checks timeoutHit. If the connection finishes establishing right after the timeout has fired, client.connect(...) calls back with no error, so the pool happily returns/keeps a connection it had already "timed out" on.
2. stream.destroy() rips out the local socket instead of terminating the connection cleanly. The reporter's suggested one-liner (client.connection.end()) doesn't actually help on current pg — at the moment the timeout fires, client.connection is still falsy, so that branch never runs. (I verified this; more below.)

The net effect: connectionTimeoutMillis is silently violated — pool.connect() resolves
after the deadline instead of rejecting — and the timed-out connection is retained. In the
worst cases it deadlocks a max: 1 pool or crashes a pg-native process.

Code

The fix (packages/pg-pool/index.js):

       tid = setTimeout(() => {
+        this.log('ending client due to timeout')
+        timeoutHit = true
+        // Reject the checkout at the deadline and stop tracking this client
+        // immediately, so a connect that hangs (or never calls back) can neither
+        // delay the timeout past connectionTimeoutMillis nor leak into _clients.
+        this._clients = this._clients.filter((c) => c !== client)
+        if (!pendingItem.timedOut) {
+          pendingItem.timedOut = true
+          pendingItem.callback(new Error('Connection terminated due to connection timeout'), undefined, NOOP)
+        }
+        this._pulseQueue()
+        // Tear the half-open connection down cleanly (send a Terminate rather
+        // than ripping out the local socket) in the background, and silence the
+        // dying client's errors so they don't surface as an unhandled 'error'
+        // (which can crash the process, e.g. with pg-native).
+        client.removeAllListeners('error')
+        client.on('error', () => {})
         if (client.connection) {
-          this.log('ending client due to timeout')
-          timeoutHit = true
-          client.connection.stream.destroy()
+          client.connection.end()
         } else if (!client.isConnected()) {
-          this.log('ending client due to timeout')
-          timeoutHit = true
-          // force kill the node driver, and let libpq do its teardown
           client.end()
         }
       }, this.options.connectionTimeoutMillis)

         if (!pendingItem.timedOut) {
           pendingItem.callback(err, undefined, NOOP)
         }
+      } else if (timeoutHit) {
+        // Race: the connection finished establishing after the timeout already
+        // rejected the checkout and removed this client. Just close the
+        // late-completing connection cleanly so no backend is leaked.
+        this.log('client connected after timeout, discarding')
+        client.removeAllListeners('error')
+        client.on('error', () => {})
+        client.end()
       } else {

Plus a deterministic regression test in packages/pg-pool/test/connection-timeout.js.

My Improvements

I don't trust an open issue until I've reproduced it, so I stood up a throwaway PostgreSQL 18
cluster and built a harness around the reporter's trigger (delay the first real connect past
the timeout).

Rigorous leak measurement. Counting "idle backends with query_start IS NULL" is misleading —
it also catches a legitimately checked-out client that just hasn't run a query yet. So I tagged
the pool's connections with a unique application_name and compared what the pool thinks it
holds (pool.totalCount) against the backends Postgres actually holds for the pool:

variant	first pool.connect()	connect events	pool.totalCount	server backends	never-used backends
buggy	resolved after ~1150ms (timeout = 1000ms!)	2	2	2	1
reporter's 1-line fix	resolved after ~1150ms	2	2	2	1
my fix	rejected at ~1000ms	1	1	1	0

The headline defect on the pure-JS client is precisely this: the timeout is not honored and the
timed-out connection is retained. (The dramatic orphaned-backend / exit-13 crash the reporter
saw is specific to pg-native and proxied poolers like PgBouncer, which I couldn't fully
reproduce without a native build — so I'm careful to present those as reporter-observed, not
claimed.)

Testing. The project's own connection-timeout suite goes from 9 to 10 passing with my
change (the only failing test needs the native addon, which isn't built in my environment). My
new regression test — "does not retain a connection that establishes after the timeout" — is
deterministic (a 200ms connect vs a 100ms timeout): it fails on the original code (resolves
with no error) and passes on the fixed code (rejects, totalCount === 0).

Best Use of Sentry

The most interesting part of this bug is that, before the fix, the failure is invisible.
pool.connect() resolves successfully — there is no exception, no log, nothing for an
observability tool to catch — while connections quietly pile up on the database. So I wired a tiny
reproduction into @sentry/node to show the difference the fix makes to observability, not just
correctness.

Sentry auto-instrumented pg/pg-pool with zero effort from me, so each checkout shows up as a
db.pool.connect trace with nested pg.connect / pg.query spans. The two runs sit side by side
in the issue stream:

• Before the fix, there is no exception to catch. The best I can do is detect the violation
  myself and report it:

  [before-fix (buggy)] connectionTimeoutMillis (1000ms) exceeded (1154ms) but pool.connect()
RESOLVED — silent timeout; server backends=1, pool believes=1

• After the fix, the timeout is a first-class, monitorable exception:

  Connection terminated due to connection timeout

  tags: outcome: rejected, scenario: after-fix (fixed)
  context pool_timeout: connectionTimeoutMillis: 1000, elapsedMs: 1012, pool_totalCount: 0

That elapsedMs: 1012 against a 1000ms deadline is the whole fix in one number — the checkout now
fails at the deadline instead of resolving at ~1150ms, and pool_totalCount: 0 proves nothing
was retained. The root span drops from 1.16s (buggy) to 1.04s (fixed) for the same work.

The trace is my favourite artifact here. The before-fix waterfall shows a single
db.pool.connect root span lasting 1.16s — already longer than the 1000ms timeout it was
supposed to honor — and inside it, two pg.connect spans. That's the over-connecting rendered
as a picture: one checkout, two backends.

Seer, honestly

I ran Seer (Sentry's AI root-cause) on the timeout exception, and it independently landed on my
core finding without being told:

"pool.fixed.js correctly enforces the timeout (unlike pool.buggy.js which silently resolved at
~1100ms, violating the timeout contract)"

It reconstructed the whole sequence — connect exceeds connectionTimeoutMillis, setTimeout fires
at 1000ms, timeoutHit=true, the error gets wrapped — and even produced accurate reproduction
steps. That's a genuinely useful second opinion on a concurrency bug.

One detail it got wrong, and it's worth saying so: Seer reported the teardown as
client.connection.stream.destroy(). That's the stock pg-pool in node_modules — the fixed
file doesn't call stream.destroy() at all. It had reached for the nearest matching source rather
than the file the frames actually came from. It's a good reminder that AI root-cause is a strong
lead, not a verdict: the conclusion was right, one supporting detail wasn't, and only reading the
code told me which was which.

That's the real value story: the fix converts a silent, un-monitorable timeout violation into a
clean, captured, alertable error — one Seer can actually reason about.

Best Use of Google AI

I used Gemini (via Google AI Studio) as a reviewer at two points, and it materially improved
the fix.

First I asked it to locate the race and explain why the reporter's proposed one-liner is
insufficient. It correctly identified that "the else (success) path in the connect callback
does not check timeoutHit… this registers the timed-out client as active/idle in the pool,
leaking the backend connection."

Then I gave it my first draft of the fix and asked it to try to break it. It found three real
problems I had missed:

1. Deferred rejection — my draft only rejected once the connect callback fired, so the caller failed at deadline + socket-teardown time, not at the deadline.
2. Leak if the callback never fires — because cleanup lived in the connect callback, a hung connect would leak the client into _clients forever.
3. Unhandled 'error' crash risk — ending a still-connecting client emits an 'error'; if the pool has an error listener and the app doesn't, the process crashes. This is almost certainly the mechanism behind the reporter's pg-native exit-13.

I rewrote the fix to reject and remove the client immediately at the deadline, and to strip
error listeners + attach a silent catch-all before tearing the connection down in the background.
Then I re-ran the full test suite to confirm the hardened version still passes everything. (Full
prompt/response transcript included as a screenshot below.)

Using an AI to adversarially review my own patch — and verifying every suggestion against the
real test suite rather than trusting it — is what made this fix production-grade instead of just
"passing the repro."

(Screenshot to add: the AI Studio conversation, especially Gemini's critique of the draft fix.)

Wrapping up

• Issue: brianc/node-postgres#3543
• PR: brianc/node-postgres#3711
• Reproduced against real Postgres 18, fixed, regression-tested (suite 10/10 minus the native-only test), and hardened with Sentry + Gemini in the loop.

Thanks for reading — and thanks to the reporter (@nicholaswold) for a beautifully detailed issue.

Comments

[Frank]

How did you identify the silent connection leak in node-postgres, was it through monitoring or a specific test case? I'd love to swap ideas on debugging pg-pool issues.