Fix postgres connection pool transaction leak by timcase · Pull Request #224 · GothenburgBitFactory/taskchampion-sync-server

timcase · 2026-07-06T22:54:31Z

Problem

The Postgres backend permanently takes down the server after any concurrent or error-producing request, requiring a process restart to recover.

Two bugs combine to cause this:

Bug 1 — No rollback on drop

Storage::txn() issues BEGIN immediately on connection checkout, but Txn had no Drop implementation. When a transaction was dropped without calling commit() — which happens on every read-only return path (get_child_version, get_snapshot) and every error path — the connection returns to the bb8 pool with its Postgres transaction still open or aborted.

On the next request, that connection is checked out and a new BEGIN is issued inside the existing transaction. If the previous transaction was aborted (e.g. by a serialization failure), every subsequent statement on that connection returns ERROR: current transaction is aborted, commands ignored until end of transaction block. This cascades across all pooled connections until the server is dead.

Bug 2 — Unnecessary SERIALIZABLE isolation

Under concurrent load, SERIALIZABLE transactions fail with SQLSTATE 40001. The application already implements optimistic concurrency via a compare-and-swap UPDATE ... WHERE latest_version_id = $expected in add_version. SERIALIZABLE isolation is redundant — and under Bug 1, each failure leaves an aborted connection in the pool.

Fix

1. impl Drop for Txn (postgres/src/lib.rs)

When Txn is dropped with a connection still held, spawn a Tokio task to issue ROLLBACK before the PooledConnection returns to the pool. ROLLBACK succeeds whether the transaction is idle-open or aborted, returning a clean connection in both cases.

2. Explicit commit() on all return paths (core/src/server.rs)

Read-only methods (get_child_version, get_snapshot) and early-exit paths in add_version and add_snapshot now call txn.commit().await? explicitly. The Drop rollback is a safety net for unexpected error paths, not the primary cleanup mechanism.

3. SERIALIZABLE → READ COMMITTED (postgres/src/lib.rs)

The application-level CAS in add_version already provides the necessary isolation guarantee. Different client_id values have completely non-overlapping rows. Dropping to READ COMMITTED eliminates serialization failures without any loss of correctness.

4. add_version return type (core/src/storage.rs, all backends)

Changed StorageTxn::add_version from -> Result<()> to -> Result<Option<Uuid>>. On a CAS miss, the backend re-reads and returns Ok(Some(current_latest_version_id)), allowing the server layer to surface a proper ExpectedParentVersion conflict response instead of HTTP 500.

Tests

Two regression tests are added to postgres/src/lib.rs using the existing with_db harness:

test_dropped_transaction_leaks_into_next_request — verifies that a dropped (uncommitted) transaction does not make its writes visible to the next transaction on the same pooled connection
test_aborted_transaction_poisons_pool — verifies that a connection left in an aborted state does not prevent subsequent independent requests from succeeding

A Docker-based reproducer script (repro/run-postgres-bug-repro.sh) is included to demonstrate the failure on unpatched code and confirm the fix.

Storage::txn() issues BEGIN immediately on connection checkout but several code paths dropped Txn without committing or rolling back, returning open or aborted transactions to the bb8 pool. The next request inheriting an aborted connection would fail with "current transaction is aborted" and cascade across the pool, requiring a process restart to recover. Three fixes: 1. Implement Drop for postgres Txn: spawn a task to ROLLBACK before the PooledConnection is returned to the pool, ensuring both idle and aborted transactions are cleaned up on any code path that does not call commit(). 2. Call txn.commit() on every normal return path in server.rs, including the read-only methods (get_child_version, get_snapshot) and early returns in add_version and add_snapshot that performed reads but skipped commit. 3. Change postgres isolation level from SERIALIZABLE to READ COMMITTED. The application already uses a compare-and-swap UPDATE on clients.latest_version_id, which serializes same-client writes correctly without SERIALIZABLE. Change StorageTxn::add_version to return Ok(Some(current_latest)) on CAS failure instead of anyhow::bail!, so the caller can surface a clean 409 conflict response rather than a 500. Includes two regression tests in postgres/tests/transaction_leak.rs and a docker-based reproduction script in repro/ that demonstrate both failure modes against an unpatched server and pass after the fix is applied.

djmitche

This looks good -- thanks for the fix!

It seems like the core issue is a bug in bb8_postgres, but I suppose that's debatable, and this fix seems solid!

djmitche · 2026-07-18T20:57:34Z

+        // If the transaction was not committed, the pooled connection still holds an open (or
+        // aborted) transaction. Roll it back before the connection returns to the pool — otherwise
+        // it poisons the next request that checks it out. The connection is owned by the spawned
+        // task and is not released to bb8 until ROLLBACK completes.


I've been using sqlx recently, and it has a close_on_drop method on connections that basically indicates they shouldn't be re-used. I don't see a similar thing for bb8. The bb8_postgres crate does seem to do some basic checks on connections, but it really only checks Client::is_closed and a no-op query, so I assume those do not detect this bad-transaction state. I don't see an actual close method!

From what I understand, bb8 uses the connection's Drop method to add the connection back to the pool. This new impl Drop for Txn moves the connection into the task, so the connection Drop doesn't occur until the ROLLBACK is complete. I suspect that if the ROLLBACK fails then the bad connection would still end up back in the pool, but maybe that only occurs in actual server-failure scenarios. And it doesn't appear there's a way to address this in bb8?

djmitche · 2026-07-18T21:09:55Z

+    /// next transaction. On the unpatched backend the dropped transaction stays
+    /// open on the pooled connection; the next `txn()` issues `BEGIN` inside it
+    /// and the uncommitted version becomes visible (dirty read).
+    #[tokio::test]


Comments referring to "unpatched" implicitly referencing this PR are confusing when read outside the context of that PR. Could you rephrase this so that it states the expectation, indicating that this tests the effectiveness of the Drop implementation?

djmitche · 2026-07-18T21:10:21Z

+    /// on its connection. Without a rollback-on-drop the aborted connection returns
+    /// to the pool and poisons every subsequent request. On the unpatched backend
+    /// the next independent `txn()` or read fails with "current transaction is
+    /// aborted, commands ignored until end of transaction block".


Similar here.

djmitche · 2026-07-18T21:12:47Z

I would expect that the "normal" runs of the tests would detect this sort of failure? I don't think this script needs to be checked in, even if it was useful during PR development.

djmitche · 2026-07-18T21:14:29Z

+                // If the client presented a non-nil parent, their chain is from
+                // a previous server. Return 409 with NIL_VERSION_ID so the
+                // client library knows to push from scratch, ensuring the chain
+                // is rooted at nil on this server.
+                if parent_version_id != NIL_VERSION_ID {
+                    let mut rb = HttpResponse::Conflict();
+                    rb.append_header((PARENT_VERSION_ID_HEADER, NIL_VERSION_ID.to_string()));
+                    return Ok(rb.finish());
+                }


This seems off-topic for this PR?

It's by design that we don't do this -- a replica may not have the child of the nil version, due to having already sent those versions to another server. This stanza would mean that a server can only be used for a new task DB, and replicas can never migrate from e.g., local sync to a server.

djmitche · 2026-07-18T21:15:09Z

-    async fn test_auto_add_client() {
+    async fn test_auto_add_client_nil_parent() {
+        // When auto-creating a client and the parent is nil, the version is
+        // accepted and the chain is correctly rooted.


This test change also shouldn't be needed.

djmitche · 2026-07-18T21:21:40Z

        };

        if let Some(client) = self.guard.clients.get_mut(&self.client_id) {
            client.latest_version_id = version_id;


I feel like this should also compare, although since the type is only used for testing I suppose there's no great reason to do so. As-is, this method never returns Ok(Some(_)).

For completeness, please add the comparison here and a quick test for it in this module.

djmitche · 2026-08-15T13:46:08Z

@timcase were you able to finish this up?

timcase force-pushed the fix/postgres-transaction-leak branch from 46cceba to bf177ea Compare July 6, 2026 23:00

djmitche reviewed Jul 18, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix postgres connection pool transaction leak - #224

Fix postgres connection pool transaction leak#224
timcase wants to merge 1 commit into
GothenburgBitFactory:mainfrom
timcase:fix/postgres-transaction-leak

timcase commented Jul 6, 2026

Uh oh!

djmitche left a comment

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche Jul 18, 2026

Uh oh!

djmitche commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

timcase commented Jul 6, 2026

Problem

Fix

Tests

Uh oh!

djmitche left a comment

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

djmitche commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants