[Pg] fix: destroy pooled client when transaction() BEGIN or ROLLBACK fails - #6120
Open
SnowingFox wants to merge 1 commit into
Open
[Pg] fix: destroy pooled client when transaction() BEGIN or ROLLBACK fails#6120SnowingFox wants to merge 1 commit into
SnowingFox wants to merge 1 commit into
Conversation
1 task
|
Author of #6114 here. This change matches the fix that we run in production as a pnpm patch since 2026-08-09. The structure is the same: BEGIN inside the try block, and release with the error unless a later ROLLBACK succeeds. Our integration test for the defect fails on unpatched 1.0.0-rc.4 and passes with this change. The test uses a max:1 pool, causes a query timeout inside a transaction, and then runs one more query. On unpatched 1.0.0-rc.4, that last query fails with "Query read timeout". With this change, it succeeds. The We saw no regressions in three days of production traffic. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6114.
Problem
NodePgSession.transaction() in the node-postgres driver has two defects in how it releases a pooled client. They are invisible in default configurations, but become deterministic when the pg client-side query_timeout is set.
A rejected BEGIN leaks the pool slot. The await tx.execute(sql begin...) runs BEFORE the try/finally block. If it rejects, the finally block never runs and release() is never called, so the client stays checked out for the life of the process. Enough occurrences exhaust the pool with no recovery.
A failed transaction returns a broken client to the pool. release() is always called without an error argument. After a client-side query_timeout the statement still runs on the server, the client activeQuery is still set, and the pg-pool eviction check does not remove the client. The broken client returns to the idle pool, and the next checkout inherits it: its queries wait behind a statement that can never complete, or run inside the leftover transaction.
Reproduction (deterministic)
A max:1 pg.Pool with query_timeout: 300. A transaction that runs pg_sleep(2) rejects as expected, then a plain select 1 on the same pool fails with Query read timeout on the buggy code and passes once release(err) destroys the client. The reproduction is schema-free and runs against any reachable Postgres. It is added as an integration test.
Fix
Move BEGIN inside the try block, and release the client with the error unless a later ROLLBACK succeeds. A server that answers ROLLBACK has shown that the connection is healthy, so an application-level error still releases the client clean and connection reuse is kept.
Before:
After:
Test plan
Adds integration-tests/tests/pg/node-postgres-transaction.test.ts. The test requires a reachable Postgres: the repository CI provides one via the postgres:14 service at PG_CONNECTION_STRING (see .github/workflows/release-feature-branch.yaml, the other shard runs tests/pg/**/*.test.ts), and outside CI the test falls back to the createDockerDB() helper (Docker). The test fails on the current code with Query read timeout and passes with this fix.