-
Notifications
You must be signed in to change notification settings - Fork 55
Fix: CLOUD-3N2A #898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix: CLOUD-3N2A #898
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -436,12 +436,13 @@ public function withTransaction(callable $callback): mixed | |
| try { | ||
| $this->rollbackTransaction(); | ||
| } catch (\Throwable $rollback) { | ||
| $this->inTransaction = 0; | ||
|
|
||
| if ($attempts < $retries) { | ||
| \usleep($sleep * ($attempts + 1)); | ||
| continue; | ||
|
Comment on lines
+439
to
443
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This path resets |
||
| } | ||
|
|
||
| $this->inTransaction = 0; | ||
| throw $rollback; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,11 +80,16 @@ 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 { | ||
| $this->getPDO()->prepare('ROLLBACK')->execute(); | ||
| } | ||
| } catch (PDOException) { | ||
| // Defensive cleanup — if there is nothing to roll back | ||
| // (stale persistent connection, implicit commit, etc.) | ||
| // we can safely ignore the failure and proceed. | ||
| } | ||
|
Comment on lines
+83
to
93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The defensive rollback handling in both ArtifactsRepro: focused PHP harness using the real SQL adapter path with a fake PDO call trace
Repro: execution output showing rollback error swallowed and beginTransaction called
|
||
|
|
||
| $this->getPDO()->beginTransaction(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| <?php | ||
|
|
||
| namespace Tests\Unit; | ||
|
|
||
| use PDO; | ||
| use PDOException; | ||
| use PDOStatement; | ||
| use PHPUnit\Framework\TestCase; | ||
| use Utopia\Database\Adapter\MariaDB; | ||
| use Utopia\Database\Exception\Transaction as TransactionException; | ||
|
|
||
| class TransactionTest extends TestCase | ||
| { | ||
| /** | ||
| * Regression: when PDO::inTransaction() returns true but the server-side | ||
| * transaction is already gone (stale persistent connection, implicit | ||
| * commit, etc.), the defensive rollBack() in startTransaction() must not | ||
| * propagate "There is no active transaction" — it should silently recover | ||
| * and begin a fresh transaction. | ||
| */ | ||
| public function testStartTransactionHandlesStaleInTransactionFlag(): void | ||
| { | ||
| $pdoMock = $this->createMock(PDO::class); | ||
|
|
||
| // PDO thinks a transaction is active, but rollBack() disagrees | ||
| $pdoMock->method('inTransaction') | ||
| ->willReturn(true); | ||
|
|
||
| $pdoMock->method('rollBack') | ||
| ->willThrowException(new PDOException('There is no active transaction')); | ||
|
|
||
| $pdoMock->expects($this->once()) | ||
| ->method('beginTransaction') | ||
| ->willReturn(true); | ||
|
|
||
| $adapter = new MariaDB($pdoMock); | ||
|
|
||
| $result = $adapter->startTransaction(); | ||
|
|
||
| $this->assertTrue($result); | ||
| } | ||
|
|
||
| /** | ||
| * Regression: when rollbackTransaction() fails during a withTransaction() | ||
| * retry, the inTransaction counter must be reset to 0 before the next | ||
| * attempt so startTransaction() takes the fresh-transaction path. | ||
| */ | ||
| public function testWithTransactionResetsCounterOnRollbackFailureDuringRetry(): void | ||
| { | ||
| $pdoMock = $this->createMock(PDO::class); | ||
| $stmtMock = $this->createMock(PDOStatement::class); | ||
| $stmtMock->method('execute')->willReturn(true); | ||
|
|
||
| $callCount = 0; | ||
|
|
||
| // First call: beginTransaction succeeds, then callback fails, | ||
| // then rollBack also fails (simulating connection issue). | ||
| // Second call: fresh beginTransaction succeeds, callback succeeds. | ||
| $pdoMock->method('inTransaction') | ||
| ->willReturnCallback(function () use (&$callCount) { | ||
| // After the first failed rollback, PDO no longer thinks | ||
| // it's in a transaction. | ||
| return false; | ||
| }); | ||
|
|
||
| $pdoMock->method('prepare') | ||
| ->willReturn($stmtMock); | ||
|
|
||
| $pdoMock->method('beginTransaction') | ||
| ->willReturn(true); | ||
|
|
||
| $pdoMock->method('commit') | ||
| ->willReturn(true); | ||
|
|
||
| // rollBack fails the first time (simulating stale transaction) | ||
| $pdoMock->method('rollBack') | ||
| ->willThrowException(new PDOException('There is no active transaction')); | ||
|
|
||
| $adapter = new MariaDB($pdoMock); | ||
|
|
||
| $result = $adapter->withTransaction(function () use (&$callCount) { | ||
| $callCount++; | ||
| if ($callCount === 1) { | ||
| throw new \RuntimeException('Transient error'); | ||
| } | ||
| return 'success'; | ||
| }); | ||
|
|
||
| $this->assertSame('success', $result); | ||
| $this->assertSame(2, $callCount); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reset also runs when the failing rollback belongs to an inner savepoint. In a nested
withTransaction(), a transient error in the inner callback can make the savepoint rollback fail, resetinTransactionto0, and then retry the inner callback as a new top-level transaction. That retry can issue the top-level cleanup rollback and abort the outer transaction, while the outer callback continues and later commits or no-ops with invalid state. This can commit inner work independently and lose the outer transaction's atomicity.