Merge pull request #895 from utopia-php/fix/start-transaction-desynced-rollback

fix(transaction): tolerate desynced cleanup rollback on startTransaction

Author: abnegate

src/Database/Adapter/Postgres.php

@@ -38,11 +38,18 @@ public function startTransaction(): bool
     {
         try {
             if ($this->inTransaction === 0) {
-                if ($this->getPDO()->inTransaction()) {
-                    $this->getPDO()->rollBack();
-                } else {
-                    // If no active transaction, this has no effect.
-                    $this->getPDO()->prepare('ROLLBACK')->execute();
+                try {
+                    if ($this->getPDO()->inTransaction()) {
+                        $this->getPDO()->rollBack();
+                    } else {
+                        // If no active transaction, this has no effect.
+                        $this->getPDO()->prepare('ROLLBACK')->execute();
+                    }
+                } catch (PDOException) {
+                    // A pooled connection can report a transaction it no longer
+                    // holds after a reconnect (e.g. Swoole PDOProxy keeps its own
+                    // counter), making this cleanup rollback throw. It is best
+                    // effort; swallow it and begin a fresh transaction below.
                 }
 
                 $result = $this->getPDO()->beginTransaction();

src/Database/Adapter/SQL.php

@@ -80,11 +80,18 @@ public function startTransaction(): bool
     {
         try {
             if ($this->inTransaction === 0) {
-                if ($this->getPDO()->inTransaction()) {
-                    $this->getPDO()->rollBack();
-                } else {
-                    // If no active transaction, this has no effect.
-                    $this->getPDO()->prepare('ROLLBACK')->execute();
+                try {
+                    if ($this->getPDO()->inTransaction()) {
+                        $this->getPDO()->rollBack();
+                    } else {
+                        // If no active transaction, this has no effect.
+                        $this->getPDO()->prepare('ROLLBACK')->execute();
+                    }
+                } catch (PDOException) {
+                    // A pooled connection can report a transaction it no longer
+                    // holds after a reconnect (e.g. Swoole PDOProxy keeps its own
+                    // counter), making this cleanup rollback throw. It is best
+                    // effort; swallow it and begin a fresh transaction below.
                 }
 
                 $this->getPDO()->beginTransaction();

tests/unit/SQLTransactionTest.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Tests\Unit;
+
+use PDOException;
+use PHPUnit\Framework\TestCase;
+use Utopia\Database\Adapter\MySQL;
+use Utopia\Database\Adapter\Postgres;
+use Utopia\Database\Exception\Transaction as TransactionException;
+
+class SQLTransactionTest extends TestCase
+{
+    /**
+     * A pooled connection (e.g. Swoole PDOProxy) can keep its own transaction
+     * counter that survives a reconnect, so it reports an open transaction the
+     * underlying connection no longer holds. The cleanup rollBack() then throws
+     * "There is no active transaction". startTransaction() must swallow that and
+     * still begin a fresh transaction instead of surfacing it as a failure.
+     */
+    public function testStartTransactionRecoversFromDesyncedRollback(): void
+    {
+        $pdo = $this->getMockBuilder(\PDO::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $pdo->method('inTransaction')->willReturn(true);
+        $pdo->method('rollBack')->willThrowException(
+            new PDOException('There is no active transaction')
+        );
+        $pdo->expects($this->once())
+            ->method('beginTransaction')
+            ->willReturn(true);
+
+        $adapter = new MySQL($pdo);
+
+        $this->assertTrue($adapter->startTransaction());
+        $this->assertTrue($adapter->inTransaction());
+    }
+
+    public function testStartTransactionDoesNotMaskBeginFailureAfterDesyncedRollback(): void
+    {
+        $pdo = $this->getMockBuilder(\PDO::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $pdo->method('inTransaction')->willReturn(true);
+        $pdo->method('rollBack')->willThrowException(
+            new PDOException('There is no active transaction')
+        );
+        $pdo->expects($this->once())
+            ->method('beginTransaction')
+            ->willThrowException(new PDOException('Connection lost'));
+
+        $adapter = new MySQL($pdo);
+
+        $this->expectException(TransactionException::class);
+        $this->expectExceptionMessage('Failed to start transaction: Connection lost');
+
+        $adapter->startTransaction();
+    }
+
+    public function testPostgresStartTransactionRecoversFromDesyncedRollback(): void
+    {
+        $pdo = $this->getMockBuilder(\PDO::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $pdo->method('inTransaction')->willReturn(true);
+        $pdo->method('rollBack')->willThrowException(
+            new PDOException('There is no active transaction')
+        );
+        $pdo->expects($this->once())
+            ->method('beginTransaction')
+            ->willReturn(true);
+
+        $adapter = new Postgres($pdo);
+
+        $this->assertTrue($adapter->startTransaction());
+        $this->assertTrue($adapter->inTransaction());
+    }
+}