Skip to content

fix(postgres): end a transaction block left open on a pooled connection - #4409

Open
jmortlock wants to merge 1 commit into
transact-rs:mainfrom
jmortlock:fix/pg-release-dirty-transaction
Open

jmortlock wants to merge 1 commit into
transact-rs:mainfrom
jmortlock:fix/pg-release-dirty-transaction

Conversation

@jmortlock

Copy link
Copy Markdown

Follow-up to #4393 / #4394, covering the half of that bug those did not reach.

The problem

Floating::return_to_pool validates a connection on release with Connection::ping, and the Postgres impl is a bare wait_until_ready:

async fn ping(&mut self) -> Result<(), Error> {
    self.write_sync();
    self.wait_until_ready().await
}

That drains the ReadyForQuery but never looks at its transaction-status byte. A session sitting inside a transaction block is therefore reported healthy and handed to the next borrower, whose statements silently run inside it and hold its locks.

Two shapes reach that point with a client-side transaction_depth of zero, so neither drop guard has queued a ROLLBACK:

  1. A future cancelled while BEGIN's round trip is in flightPostgres: cancelling Pool::begin leaves the connection in a transaction and poisons it for the pool #4393, fixed at the source by fix(postgres): roll back a transaction cancelled during BEGIN #4394.
  2. A statement that fails inside a block opened by a multi-statement raw_sql, e.g. BEGIN; SELECT 1/0;. The block is opened and aborted within a single simple-query message, so the depth is never raised at all and fix(postgres): roll back a transaction cancelled during BEGIN #4394 does not apply.

The second shape does not self-heal. The next borrower's BEGIN fails because the block is already aborted, which leaves the depth at zero again, so nothing queues a rollback on that release either, and the ping waves it through once more. Every checkout of that connection fails with 25P02 until max_lifetime recycles it.

Both shapes also occur with BEGIN READ ONLY, where the damage surfaces not as 25P02 but as a write failing with cannot execute … in a read-only transaction at some unrelated call site that merely drew the poisoned connection.

The change

Check the server-reported status in ping and roll back when it disagrees with the client's view.

This costs no extra round trip. wait_until_ready has just refreshed transaction_status from the server's own ReadyForQuery, so the check is a field read, and the ROLLBACK is only sent on a connection that is actually dirty — a clean release is byte-for-byte what it is today, with no WARNING: there is no transaction in progress in the server log.

Fixing it in ping rather than in return_to_pool keeps it in the driver, where the status byte lives, and covers every caller of ping rather than the pool alone.

The transaction_depth == 0 gate is load-bearing. ping is public API and a caller may be holding a transaction deliberately across one; a non-zero depth means the client knows about the block and owns its lifetime, so it is left strictly alone. Only a block the client has no record of is ended here. That also means there is never a savepoint to restore to, hence a plain ROLLBACK rather than rollback_ansi_transaction_sql.

Also: PgConnection::transaction_status

Exposed as public API, with Clone/Copy/PartialEq/Eq on TransactionStatus.

Connection::is_in_transaction reports get_transaction_depth(self) != 0 — the client-side depth, which is precisely the value that is wrong in both shapes above. The server's view was not reachable from outside the crate at all: in_transaction() is pub(crate), the field is private, and while message::ready_for_query does pub use TransactionStatus, mod message itself is private in lib.rs, so the type could not even be named. This lets pool users make the same distinction themselves in an after_release hook at no round-trip cost.

Note in_transaction() is deliberately left as-is: it treats Error as not in a transaction, which is correct for its caller in begin but wrong for a release check. The new accessor returns the raw status so callers choose their own predicate.

Tests

it_does_not_return_a_connection_inside_a_transaction_to_the_pool — on a one-connection pool, so the same session is guaranteed back. Covers shape 2 (deterministic, no timing), asserting the precondition that the client-side depth stays zero, then the leaked BEGIN READ ONLY case, ending with the write that fails in production. It fails on main with:

assertion `left == right` failed: connection was returned to the pool still inside a transaction block
  left: Error
 right: Idle

it_does_not_roll_back_a_transaction_the_caller_owns — pins the depth gate, so the guard cannot be "simplified" into rolling back a live transaction underneath its owner.

Verified against a real server (Postgres 17, the tests/postgres/setup.sql fixtures). All 9 Postgres test targets pass, 270 tests, including it_rolls_back_a_transaction_cancelled_during_begin from #4394 and the it_can_work_with_failed_transactions / it_can_work_with_nested_transactions / it_can_fail_and_recover family. cargo clippy -p sqlx-postgres and cargo fmt --all --check are clean, and the 150 sqlx-postgres unit tests pass.

Note on scope

This makes #4394 belt-and-braces rather than redundant: #4394 stops the cancelled-BEGIN case from ever leaking, and this stops any connection from returning to the pool dirty, whatever the cause. I would suggest keeping both.

Happy to split the transaction_status accessor into its own PR if you would rather review the fix alone.

`Floating::return_to_pool` validates a connection on release with
`Connection::ping`, and the Postgres impl was a bare `wait_until_ready`: it
drains the `ReadyForQuery` but never looks at its transaction-status byte. A
session sitting inside a transaction block is therefore reported healthy and
handed to the next borrower, whose statements run inside it and hold its locks.

Two shapes reach that point with a client-side `transaction_depth` of zero, so
neither drop guard has queued a `ROLLBACK`:

- A future cancelled while `BEGIN`'s round trip is in flight (transact-rs#4393). transact-rs#4394
  fixed this one at the source by claiming the depth before the await.
- A statement that fails inside a block opened by a multi-statement `raw_sql`,
  where the block is opened and aborted within a single simple-query message
  and the depth is never raised at all. This one does not self-heal: the next
  borrower's `BEGIN` fails because the block is already aborted, leaving the
  depth at zero again, so every subsequent checkout fails with 25P02 until
  `max_lifetime` recycles the connection.

Check the server-reported status in `ping` and roll back when it disagrees.
The status byte is already decoded into `PgConnection::transaction_status` on
every `ReadyForQuery`, so the check itself is free, and the `ROLLBACK` is only
sent on a connection that is actually dirty.

The check is gated on `transaction_depth == 0`. `ping` is public API and a
caller may be holding a transaction deliberately; a non-zero depth means the
client knows about the block and owns its lifetime, so it is left alone. Only a
block the client has no record of is ended here -- which also means there is
never a savepoint to restore to, hence a plain `ROLLBACK`.

Also exposes `PgConnection::transaction_status`, so pool users can make the
same distinction themselves. `Connection::is_in_transaction` reports the
client-side depth, which is precisely the value that is wrong in both shapes
above, and the server's view was not reachable from outside the crate.

Tests: `it_does_not_return_a_connection_inside_a_transaction_to_the_pool`
covers both shapes plus the leaked `BEGIN READ ONLY` case, where the damage
surfaces as a failing write rather than 25P02, and
`it_does_not_roll_back_a_transaction_the_caller_owns` pins the depth guard.
The first fails on main with `left: Error, right: Idle`.
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.

1 participant