From b607b5398b9c505445b756b79bb131c88dca52f5 Mon Sep 17 00:00:00 2001 From: Malbolged <116412117+malbolged@users.noreply.github.com> Date: Fri, 17 Jul 2026 02:05:39 +0100 Subject: [PATCH 1/2] fix(pg-pool): honor connectionTimeoutMillis and don't leak timed-out connections When connectionTimeoutMillis fired while a new connection was still being established, the connect callback's success path never checked whether the timeout had already fired. If the connection finished establishing just after the deadline, the pool would resolve the checkout successfully - silently violating connectionTimeoutMillis - and retain a connection it had already given up on. The timeout callback also tore down the local socket with stream.destroy() instead of terminating the connection cleanly. This changes newClient() to: - reject the checkout at the deadline and remove the client from _clients immediately, so a connect that hangs (or never calls back) can neither delay the timeout nor leak into _clients - terminate the half-open connection cleanly (connection.end() sends a proper Terminate) instead of destroying the local socket - silence the dying client's 'error' events before ending it, so they don't surface as an unhandled 'error' (which can crash the process) - discard, rather than hand back, a connection that finishes establishing after the timeout already rejected the checkout Adds a deterministic regression test (a 200ms connect against a 100ms timeout) that fails on the previous behavior and passes with this change. Fixes #3543 --- packages/pg-pool/index.js | 32 +++++++++++++++++---- packages/pg-pool/test/connection-timeout.js | 29 +++++++++++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/packages/pg-pool/index.js b/packages/pg-pool/index.js index ab514fa88..ca9ebb4ad 100644 --- a/packages/pg-pool/index.js +++ b/packages/pg-pool/index.js @@ -249,14 +249,26 @@ class Pool extends EventEmitter { let timeoutHit = false if (this.options.connectionTimeoutMillis) { 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) @@ -282,6 +294,14 @@ class Pool extends EventEmitter { 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 { this.log('new client connected') diff --git a/packages/pg-pool/test/connection-timeout.js b/packages/pg-pool/test/connection-timeout.js index c4fd1832b..c4ae87dd7 100644 --- a/packages/pg-pool/test/connection-timeout.js +++ b/packages/pg-pool/test/connection-timeout.js @@ -227,6 +227,35 @@ describe('connection timeout', () => { }) }) + it('does not retain a connection that establishes after the timeout (#3543)', (done) => { + const Client = require('pg').Client + const orgConnect = Client.prototype.connect + + // Force the first connection to finish establishing only *after* the + // connection timeout has already fired (200ms > 100ms). + let first = true + Client.prototype.connect = function (cb) { + if (first) { + first = false + return setTimeout(() => orgConnect.call(this, cb), 200) + } + return orgConnect.call(this, cb) + } + + const pool = new Pool({ connectionTimeoutMillis: 100, max: 1 }) + pool.connect((err, client) => { + Client.prototype.connect = orgConnect + // The checkout must fail with a timeout rather than silently resolving + // ~200ms later, and the timed-out connection must not be retained. + expect(err).to.be.an(Error) + expect(err.message).to.contain('timeout') + expect(client).to.be(undefined) + expect(pool.totalCount).to.equal(0) + expect(pool.idleCount).to.equal(0) + pool.end(done) + }) + }) + it('should connect if timeout is passed, but native client in connected state', (done) => { const Client = require('pg').native.Client From 8c94fea54b308f27799919aff06d5410ad030700 Mon Sep 17 00:00:00 2001 From: Malbolged <116412117+malbolged@users.noreply.github.com> Date: Fri, 17 Jul 2026 04:16:51 +0100 Subject: [PATCH 2/2] fix(pg-pool): don't time out a native client that is already connected The previous commit hoisted `timeoutHit = true` and the checkout rejection above the `if (client.connection) / else if (!client.isConnected())` check, which dropped an intentional behavior: a client with no connection object that already reports `isConnected()` (the native client) was never timed out, because it has connected and simply hasn't invoked its connect callback yet. Restore that exemption by returning early from the timeout callback in that case, which fixes `should connect if timeout is passed, but native client in connected state`. --- packages/pg-pool/index.js | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/pg-pool/index.js b/packages/pg-pool/index.js index ca9ebb4ad..e3cf6dec8 100644 --- a/packages/pg-pool/index.js +++ b/packages/pg-pool/index.js @@ -249,26 +249,24 @@ class Pool extends EventEmitter { let timeoutHit = false if (this.options.connectionTimeoutMillis) { tid = setTimeout(() => { + // the native client reports itself connected before its connect callback fires + if (!client.connection && client.isConnected()) { + return + } + 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) { client.connection.end() - } else if (!client.isConnected()) { + } else { client.end() } }, this.options.connectionTimeoutMillis) @@ -295,9 +293,6 @@ class Pool extends EventEmitter { 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', () => {})