Reject transaction work after its connection closes - #1215
Open
ecsbeats wants to merge 2 commits into
Open
Conversation
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 #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.mjsin the repository root. It needs Node 22 and a PostgreSQL login that can terminate its own other sessions.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-reproWait for
pg_isreadyto report that the server is accepting connections before running the script. On upstream411429e, 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: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:
tests/test.js:20assumes a Node stack format; the same failure occurs on unmodified upstream.