drizzle-team / drizzle-team/drizzle-orm

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

Open
#6,114 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
35.8k
Forks
1.6k
Avg merge
2d 7h
Merged PRs (30d)
4

Description

### Report hasn't been filed before.

- [x] I have verified that the bug I'm about to report hasn't been filed before.

### What version of `drizzle-orm` are you using?

1.0.0-rc.4 (the same code ships in 0.45.2)

### What version of `drizzle-kit` are you using?

0.31.9

### Other packages

pg 8.23.0, pg-pool 3.14.0

### Describe the Bug

`NodePgSession.transaction()` in the node-postgres driver has two defects in how it releases the pooled client. The defects are invisible in default configurations. They become deterministic when pg's client-side `query_timeout` is set. A dead pooled connection at checkout also triggers them.

```js
// node-postgres/session.js (1.0.0-rc.4; identical shape in 0.45.2)
const session = isPool ? new NodePgSession(await this.client.connect(), ...) : this;
const tx = new NodePgTransaction(...);
await tx.execute(sql`begin...`); // (1) OUTSIDE the try/finally
try {
const result = await transaction(tx);
await tx.execute(sql`commit`);
return result;
} catch (error) {
await tx.execute(sql`rollback`);
throw error;
} finally {
if (isPool) session.client.release(); // (2) always WITHOUT an error argument
}
```

**Defect 1 — a rejected BEGIN leaks the pool slot.** The `await tx.execute(sql`begin`)` line runs before the `try` block. If this line rejects, the `finally` block never runs. As a result, `release()` is never called. The client stays checked out for the life of the process. Enough occurrences exhaust the pool, and there is no recovery.

**Defect 2 — a failed transaction returns a broken client to the pool.** `release()` is always called without an error argument. pg destroys the stream on a query timeout only in pipeline mode (`pg/lib/client.js`, near line 723). In the default mode, the timed-out statement still runs on the server. The client's `activeQuery` is still set, and `_queryable` stays `true`. Because of this, the eviction check in pg-pool (`index.js`, near line 392) 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 they run inside the leftover transaction.

### Reproduction

The reproduction is schema-free. It runs against any reachable Postgres:

```ts
import { drizzle } from "drizzle-orm/node-postgres";
import { sql } from "drizzle-orm";
import pg from "pg";

const pool = new pg.Pool({
connectionString: process.env.DATABASE_URL,
max: 1,
query_timeout: 300,
connectionTimeoutMillis: 5_000,
});
const db = drizzle({ client: pool });

// 1. A transaction whose statement exceeds query_timeout rejects, as expected.
await db
.transaction(async (tx) => {
await tx.execute(sql`select pg_sleep(2)`);
})
.catch(() => {});

// 2. The max:1 pool is now broken. This unrelated query fails with
// "Query read timeout", because it inherited the client whose pg_sleep
// still runs on the server. With defect 1 (a BEGIN that times out on a
// dead pooled connection), it hangs until connectionTimeoutMillis instead.
const after = await db.execute(sql`select 1 as ok`);
console.log(after.rows); // never reached on 1.0.0-rc.4 / 0.45.2
```

Observed: step 2 fails with `Query read timeout`.
Expected: the pool removes the client of the failed transaction, and step 2 runs on a fresh connection.

### Suggested fix

Move `BEGIN` into the `try` block. Release with the error, unless a later `ROLLBACK` succeeds. A server that answers `ROLLBACK` has shown that the connection is healthy. An application-level error therefore still releases the client clean, and connection reuse is kept.

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

We run this change as a pnpm patch in production. The reproduction above fails on vanilla 1.0.0-rc.4 and passes with the patch. The open reports #4824 and #928 show similar symptoms.

Contributor guide

Open the contributing guide

Research direction

Start with NodePgSession.transaction in node-postgres/session.js and run the schema-free reproduction against PostgreSQL. Compare the transaction cleanup flow with pg/lib/client.js and pg-pool index.js, especially the referenced areas near lines 723 and 392. Done means a rejected BEGIN does not leak a pooled client and a failed transaction does not return a broken client to the pool.

Written by the indexing model from the issue text.

Assessment

Tech stack
nodejs, postgresql, typescript
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.