drizzle-team / drizzle-team/drizzle-orm
[BUG]: node-postgres transaction() leaks the client on a rejected BEGIN and returns broken clients to the pool
- 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
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