Skip to content

Reject transaction work after its connection closes - #1215

Open
ecsbeats wants to merge 2 commits into
porsager:masterfrom
ecsbeats:fix/disconnected-transactions
Open

ecsbeats wants to merge 2 commits into
porsager:masterfrom
ecsbeats:fix/disconnected-transactions

Conversation

@ecsbeats

@ecsbeats ecsbeats commented Sep 9, 2026

Copy link
Copy Markdown

Fixes #1216.

If a backend disconnects while a sql.begin() callback is awaiting other work, begin() rejects but the callback keeps running. Once the pool reconnects, that callback can issue queries on the replacement connection. Returning or throwing from it can also commit or roll back a different transaction. Queries still queued inside the original transaction can remain pending indefinitely.

Keep the close error on the transaction, reject its queued queries, and reject subsequent queries through that transaction's handler, including automatic commit/rollback. Clear the connection's pending write and query/result/error state before reuse, too. Without that reset, the first query after reconnect can receive the old backend's fatal error.

The four regression tests cover queued queries, a query through the disconnected transaction, and late commit/rollback. The latter three share a fixture that pauses the original callback until a replacement transaction is open. No new dependencies or public API changes.

Related to #1204. This reproduction specifically covers a backend disconnect followed by connection reuse; I haven't verified that it explains the original report.

Reproduce

Save the following as repro.mjs in the repository root. It needs Node 22 and a PostgreSQL login that can terminate its own other sessions.

import assert from 'node:assert/strict'
import postgres from './src/index.js'

const pool = postgres({ max: 1 })
const admin = postgres({ max: 1 })
let resume
  , connected
const gate = new Promise(resolve => { resume = resolve })
const ready = new Promise(resolve => { connected = resolve })
const failed = pool.begin(async sql => {
  connected((await sql`select 1`).state.pid)
  await gate
}).catch(error => error)

try {
  await admin`select pg_terminate_backend(${ await ready }::int)`
  assert.equal((await failed).code, 'CONNECTION_CLOSED')
  await pool.begin(async sql => {
    const [{ id: before }] = await sql`select txid_current()::text as id`
    resume()
    await new Promise(resolve => setImmediate(resolve))
    const [{ id: after }] = await sql`select txid_current()::text as id`
    console.log({ before, after })
    assert.equal(after, before)
  })
} finally {
  resume()
  await Promise.all([pool.end({ timeout: 0 }), admin.end({ timeout: 0 })])
}

For a disposable local server:

docker run --rm -d --name postgres-disconnect-repro \
  -p 127.0.0.1:55432:5432 \
  -e POSTGRES_HOST_AUTH_METHOD=trust postgres:17
docker exec postgres-disconnect-repro pg_isready -U postgres
PGHOST=127.0.0.1 PGPORT=55432 PGUSER=postgres PGDATABASE=postgres node repro.mjs
docker stop postgres-disconnect-repro

Wait for pg_isready to report that the server is accepting connections before running the script. On upstream 411429e, the assertion fails because the transaction IDs differ: the old callback has committed the replacement transaction. On this branch they stay equal and the script exits successfully. I verified both outcomes.

Tests

The first commit, 0282301, adds only the regression tests; the second applies the fix. With the PostgreSQL setup from the existing test workflow in place:

git worktree add ../postgres-disconnect-before 0282301
(cd ../postgres-disconnect-before && PGUSER=postgres PGSOCKET=/var/run/postgresql npm run test:esm)
PGUSER=postgres PGSOCKET=/var/run/postgresql npm test
npm exec --yes --package=eslint@8.57.1 -- eslint src tests

The first run times out in Disconnect rejects queued transaction queries and allows reconnect. The fixed branch passes. The full suite needs both PostgreSQL servers from the workflow (ports 5432 and 5433), SSL, logical replication, prepared transactions, the PostgreSQL CLI tools, and Deno 1.x. Use disposable databases: the existing bootstrap changes server settings and recreates its test database and roles.

Validated against PostgreSQL 17.11:

  • Node 12.22.12 and 22.22.0: all 268 tests pass in both ESM and CommonJS.
  • Deno 1.46.3: all 268 tests pass.
  • Bun 1.4.2: the standalone reproduction and focused probes for all four failure cases pass. The full suite cannot register tests because tests/test.js:20 assumes a Node stack format; the same failure occurs on unmodified upstream.
  • ESLint passes.

@AlephNotation

Copy link
Copy Markdown

I've also recently gotten bitten by this one

@batalyse-gmbh

Copy link
Copy Markdown

Thanks for this PR. We tested it together with #1209: the two fix every crash case we could reproduce. We found one case that is still broken, and a one-line fix for it.

What we tested

  • postgres 3.4.9, Node 26, PostgreSQL 18.4 and 18.6 (same results on both)
  • max: 1, so the same connection is always reused

On 3.4.9 without the PRs

Scenario What happens
transaction_timeout or pg_terminate_backend during a query in sql.begin() Crash: the nextWrite TypeError from #1133. If you catch it, the connection never comes back: every reconnect ends in CONNECT_TIMEOUT.
idle_in_transaction_session_timeout, then another begin() reuses the connection The dead transaction's INSERT and COMMIT run inside the other transaction. That transaction throws, but its inserts stay committed (#1216).
Any kill during a query The next query fails with the old backend's 57P01 / 25P04 error.

A plain null check in nextWrite (as in #1168) is not enough. It turns the crash into the same endless CONNECT_TIMEOUT.

With #1209 + #1215

All three are fixed. ✅

Still broken: a connection reset inside begin()

Symptom: after an ECONNRESET (for example a failover), one query works. After that the connection hangs forever.

Cause:

  1. On a reset, the socket emits 'error' first and 'close' a moment later. Promise callbacks run in between.
  2. In that gap, begin() sends its rollback. The socket is still set and closedError is not set yet, so the rollback is added to sent.
  3. closed(true) only clears sent when !hadError, so the rollback stays there.
  4. After reconnecting, the connection treats that old rollback as in flight and waits for a reply that never comes.

Repro:

import net from "node:net";
import postgres from "postgres";

// A TCP proxy in front of Postgres, so we can reset the socket like a failover would.
const pairs = [];
const proxy = net.createServer((client) => {
  const upstream = net.connect(5432, "127.0.0.1");
  client.pipe(upstream).pipe(client);
  client.on("error", () => {});
  upstream.on("error", () => {});
  pairs.push([client, upstream]);
}).listen(0, "127.0.0.1");
await new Promise((r) => proxy.once("listening", r));

const sql = postgres({ host: "127.0.0.1", port: proxy.address().port, max: 1 /* , user, password */ });

await sql.begin(async (tx) => {
  setTimeout(() => pairs.forEach(([c, u]) => (u.destroy(), c.resetAndDestroy())), 500);
  await tx`select pg_sleep(3)`;
}).catch((e) => console.log("begin rejected:", e.code));

const ask = (tag) => sql`select ${tag}::text as tag`.then(([r]) => console.log(tag, "->", r.tag));
await ask("Q1");
await Promise.race([ask("Q2"), new Promise((r) => setTimeout(() => r(console.log("Q2 still pending after 3s")), 3000))]);
process.exit(0);

Output on 3.4.9, and on 3.4.9 + #1209 + #1215:

begin rejected: CONNECTION_CLOSED
Q1 -> Q1
Q2 still pending after 3s

Expected: Q2 -> Q2.

Fix

Always clear sent when the socket closes:

-    !hadError && (query || sent.length) && error(Errors.connection('CONNECTION_CLOSED', options, socket))
+    if (query || sent.length)
+      error(Errors.connection('CONNECTION_CLOSED', options, socket))

It has to be an if. The code has no semicolons, so a line starting with ( would join onto the return reconnect() above it.

We tested it on top of #1209 + #1215:

  • the repro above passes;
  • a plain query outside a transaction still gets ECONNRESET;
  • all the scenarios in the table still pass.

Written by Claude Opus

@cat-a-guerra

Copy link
Copy Markdown

We independently found and patched the same two issues in production (3.4.9): the stale chunk/nextWriteTimer that leaves reconnects ending in CONNECT_TIMEOUT, and the stale query/errorResponse that replays the old backend's 57P01 onto the next query. Our patch makes the same two resets, so +1 to this approach. We also saw sql.end() never resolve while the connection was in that stale state.

One path this PR doesn't reach: closed() returns early via if (initial) return reconnect() before the new reset lines run. So a connection killed during startup (the fetch_types query) still replays its 57P01 onto the first query, plus an unhandled rejection. Repro and a one-line fix in #1223. We confirmed this PR alone doesn't change the output there.

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.

Disconnected transaction handles can read another user's rows through RLS

4 participants