fix(transaction): tolerate desynced cleanup rollback on startTransaction

[loks0n]

Problem

In production (Appwrite Cloud, nyc3) we see a high-volume, persistent error on databases.updateDocument / tablesDB.updateRow:

Utopia\Database\Exception\Transaction: Failed to start transaction: There is no active transaction
  at src/Database/Adapter/SQL.php:96  (startTransaction)
  caused by PDOException: There is no active transaction  (PDO::rollBack)

Root cause

A pooled connection can report a transaction it no longer holds after a reconnect.

When a connection drops mid-transaction (e.g. MySQL group-replication failover / member expulsion / wait_timeout), Swoole's PDOProxy reconnects and builds a fresh underlying PDO, but does not reset its own inTransaction counter (and only decrements it on a successful commit/rollback). The proxy then reports an open transaction the real connection no longer has:

public function inTransaction(): bool { return $this->inTransaction > 0; } // stale counter

SQL::startTransaction() trusts that and runs its pre-begin cleanup:

if ($this->getPDO()->inTransaction()) { // true (stale)
    $this->getPDO()->rollBack();         // real PDO has no txn -> throws "There is no active transaction"
}

The PDOException is rewrapped as Failed to start transaction: ... and aborts an otherwise-valid start. Because the pool reclaims (rather than destroys) the connection on error, the desynced connection stays in rotation and every subsequent transaction on it fails the same way — explaining the sustained, "ongoing" error volume.

Note: the utopia-native Utopia\Database\PDO wrapper is not affected — it reads the real connection's inTransaction() state, so it self-heals via withTransaction's retry loop. This only bites when wrapped by a proxy that tracks transaction state independently (Swoole PDOProxy).

Fix

Make the pre-begin cleanup rollback best-effort: swallow a PDOException from it and continue to beginTransaction() on the (clean, reconnected) connection. The start succeeds instead of throwing, which also breaks the poison-pill loop.

This does not mask genuine start failures — if the connection is truly unusable, the subsequent beginTransaction() still throws and is surfaced through the existing outer catch.

Tests

Added tests/unit/SQLTransactionTest.php which mocks a PDO whose inTransaction() returns true while rollBack() throws There is no active transaction (the exact desync). It reproduces the production error without this change and passes with it.

• ✅ tests/unit (341 tests) green
• ✅ Pint
• ✅ PHPStan level 7

Follow-up (out of scope)

To eliminate the residual swallowed exception per transaction-start on a desynced proxy, a pool-level improvement could call PDOProxy::reset() on reclaim, or destroy() connections that error mid-transaction.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved transaction startup to better handle desynchronized rollback states and continue cleanly after cleanup failures.
  • Ensures transaction start failures (e.g., when a begin operation cannot start) are surfaced with the correct error instead of being masked.

• Tests

  • Added unit test coverage for the failure path when cleanup succeeds but the transaction begin fails after a desynced rollback.
  • Extended existing coverage for recovery behavior.

[coderabbitai]

Warning

Review limit reached

@abnegate, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 54 minutes and 34 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cb5b4f43-b0c0-4e40-9735-45a3c075e41b

📥 Commits

Reviewing files that changed from the base of the PR and between d9ed545 and 240b957.

📒 Files selected for processing (3)

• src/Database/Adapter/Postgres.php
• src/Database/Adapter/SQL.php
• tests/unit/SQLTransactionTest.php

📝 Walkthrough

Walkthrough

SQL::startTransaction() now wraps the pre-transaction rollback cleanup in a try/catch that swallows PDOException, branching between rollBack() and a raw ROLLBACK statement depending on PDO's reported state. Unit tests verify both recovery from desynced rollback and that subsequent beginTransaction() failures are not masked by cleanup exceptions.

Changes

PDO Desync Recovery in startTransaction()

Layer / File(s)	Summary
Cleanup try/catch in startTransaction() src/Database/Adapter/SQL.php	startTransaction() wraps the inTransaction===0 rollback step in a try/catch(PDOException), attempting rollBack() if PDO reports an active transaction or falling back to a raw ROLLBACK statement, swallowing any exception so the fresh beginTransaction() always proceeds.
Test coverage for recovery and exception transparency tests/unit/SQLTransactionTest.php	Two PHPUnit tests validate cleanup behavior: the first mocks desynced PDO state where inTransaction() is true but rollBack() throws and asserts recovery succeeds; the second configures beginTransaction() to throw after rollback failure and asserts that this exception is raised (not masked by the rollback exception).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• utopia-php/database#677: Modifies SQL::startTransaction() transaction-state cleanup logic, directly overlapping with the rollback/PDOException recovery path changed here.
• utopia-php/database#748: Adjusts inTransaction counter resets on rollback/reconnect failures in SQL.php, related to the same desync-recovery concern addressed in this PR.

Suggested reviewers

• abnegate

Poem

🐇 A rollback threw, but I did not fret,
I caught the exception and swallowed it yet.
With a fresh beginTransaction() in store,
The desync that haunted me troubles no more.
Yet if begin itself should fail to start,
I'll surface that error—I'll do my part. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: making startTransaction() tolerate (handle gracefully) desynced cleanup rollback failures.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/start-transaction-desynced-rollback

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

Makes transaction startup tolerate stale pooled-connection transaction state during pre-begin cleanup.

Treats cleanup rollback failures as best-effort in the base SQL adapter and the Postgres override.

Adds unit coverage for desynced rollback recovery and for surfacing real begin-transaction failures.

Confidence Score: 5/5

The transaction startup change is narrowly scoped and covered by focused unit tests for both recovery and real begin-failure behavior.

The modified adapters preserve existing error surfacing for actual begin failures while tolerating only cleanup rollback exceptions, matching the described pooled-connection desync scenario.

No files require follow-up attention from this review.

T-Rex Logs

What T-Rex did

• Executed the base run: php trex_sql_transaction_recovery.php, which exited 127 with /bin/sh: php: not found.
• Executed the head run: php trex_sql_transaction_recovery.php, which also exited 127 with /bin/sh: php: not found.
• Tried to install PHP CLI via apt-get, but the operation could not proceed due to permission restrictions (UID 1001 cannot write to /var/lib/apt/lists/partial).
• Compared the SQL begin failure logs; the before and after artifacts show PHP CLI is absent and the failure blocks execution.
• Checked out repositories and observed: base checkout succeeded but lacked the head test file; head checkout succeeded and included the focused PHPUnit tests, but PHP execution still failed due to php: not found.

_{Ran code and verified through T-Rex}

_{Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/start-trans..." | Re-trigger Greptile}