Fix connection returned to pool without rollback after failed commit

[rkurka]

Summary

PooledConnection.commitTransaction() uses doOnSubscribe to clear the inTransaction flag, which fires before the database processes the commit. If the commit subsequently fails, the flag is already false. When close() is called, the safety-net rollback is skipped and the connection is returned to the pool with an open transaction.

Root cause

// Before (PooledConnection.java:117)
return Mono.from(this.connection.commitTransaction())
    .doOnSubscribe(ignore -> this.inTransaction = false);  // clears flag immediately on subscribe

doOnSubscribe fires at subscription time, not on successful completion. Compare with beginTransaction(), which correctly uses the pessimistic approach:

return Mono.from(this.connection.beginTransaction())
    .doOnSubscribe(ignore -> this.inTransaction = true)   // set true early (safe: unnecessary rollback is harmless)
    .doOnError(e -> this.inTransaction = false);           // revert on failure

For commitTransaction, the semantics are reversed: clearing the flag early is unsafe, because a failed commit leaves the transaction open.

Failure scenario

Step-by-step trace through the code:

1. beginTransaction() is called → doOnSubscribe sets inTransaction = true → DB executes BEGIN → onComplete. State: inTransaction == true

2. Application executes statements (e.g. INSERT INTO orders ...)

3. commitTransaction() is called → doOnSubscribe immediately sets inTransaction = false → DB attempts COMMIT → fails (serialization failure, deadlock, transient network error) → onError propagates to caller. State: inTransaction == false, but the transaction is still open on the database side

4. Caller handles the error and calls close() to return the connection to the pool

5. close() triggers the release chain:

   • Validation.validate(this, ValidationDepth.LOCAL) — checks if the TCP connection is alive. It is (the error was a logical failure, not a connection loss). Validation passes.
   • if (this.inTransaction) (line 74) — inTransaction is false. Rollback is skipped.
   • this.ref.release() — connection is returned to the pool

6. Next consumer acquires the same physical connection. The previous transaction is still open on the database. The new consumer's statements execute within the previous session's uncommitted transaction. A subsequent COMMIT commits both sessions' data; a ROLLBACK rolls back both.

Concrete example

User A: BEGIN
User A: INSERT INTO orders (amount) VALUES (10000)
User A: COMMIT  →  DB: "ERROR: could not serialize access"
                    ↑ doOnSubscribe already set inTransaction = false
User A: close() →  validation OK → no rollback → ref.release()

User B: pool.acquire() → gets the same connection
User B: INSERT INTO orders (amount) VALUES (5)
User B: COMMIT  →  DB commits BOTH inserts (10000 + 5)

User A's failed order (10,000) is silently committed alongside User B's order.

Fix

Change doOnSubscribe to doOnSuccess:

return Mono.from(this.connection.commitTransaction())
    .doOnSuccess(ignore -> this.inTransaction = false);

The flag is only cleared after the commit succeeds. On failure, inTransaction remains true, so close() issues a rollback before returning the connection to the pool.

Why this is safe

• Successful commit: doOnSuccess fires → inTransaction = false → close() skips rollback (correct, same behavior as before)
• Failed commit: doOnSuccess does not fire → inTransaction stays true → close() issues rollback → connection returned clean (fixed behavior)
• Comparison with beginTransaction(): beginTransaction uses doOnSubscribe to set true early, which is the correct pessimistic direction — an unnecessary rollback on close is harmless. For commitTransaction, the pessimistic direction is to keep the flag true until success.

Test plan

• Added failedCommitShouldRollbackOnClose test: begin → failed commit → close → verify rollback was called
• All 98 existing tests pass without modification

🤖 Generated with Claude Code