Skip to content

[Pg] fix: destroy pooled client when transaction() BEGIN or ROLLBACK fails - #6120

Open
SnowingFox wants to merge 1 commit into
drizzle-team:mainfrom
SnowingFox:fix/node-postgres-transaction-leak
Open

[Pg] fix: destroy pooled client when transaction() BEGIN or ROLLBACK fails#6120
SnowingFox wants to merge 1 commit into
drizzle-team:mainfrom
SnowingFox:fix/node-postgres-transaction-leak

Conversation

@SnowingFox

Copy link
Copy Markdown

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.

  1. 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.

  2. 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:

await tx.execute(sql`begin...`);
try { ... } finally { if (isPool) client.release(); }

After:

let releaseErr;
try {
  await tx.execute(sql`begin...`);
  const result = await transaction(tx);
  await tx.execute(sql`commit`);
  return result;
} catch (error) {
  releaseErr = error instanceof Error ? error : true;
  try { await tx.execute(sql`rollback`); releaseErr = undefined; } catch {}
  throw error;
} finally {
  if (isPool) client.release(releaseErr);
}

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.

@shi-rudo

Copy link
Copy Markdown

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 true fallback for thrown non-Error values is an improvement over our patch.

We saw no regressions in three days of production traffic.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: node-postgres transaction() leaks the client on a rejected BEGIN and returns broken clients to the pool

2 participants