Skip to content

fix: retry deadlocks with exponential backoff and jitter #914

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.

Sign up for GitHub

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

Open
claudear wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: retry deadlocks with exponential backoff and jitter #914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions src/Database/Adapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,9 @@ public function withTransaction(callable $callback): mixed
{
$sleep = 50_000; // 50 milliseconds
$retries = 2;
$deadlockRetries = 5;

for ($attempts = 0; $attempts <= $retries; $attempts++) {
for ($attempts = 0; $attempts <= $deadlockRetries; $attempts++) {
try {
$this->startTransaction();
$result = $callback();
Expand All @@ -413,8 +414,9 @@ public function withTransaction(callable $callback): mixed
try {
$this->rollbackTransaction();
} catch (\Throwable $rollback) {
if ($attempts < $retries) {
\usleep($sleep * ($attempts + 1));
$maxRetries = self::isDeadlock($action) ? $deadlockRetries : $retries;
if ($attempts < $maxRetries) {
\usleep(self::retrySleep($sleep, $attempts));
Comment on lines +417 to +419

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Base rollback-failure retries on the rollback error.

Line 417 gives the deadlock retry budget when $action is a deadlock even if $rollback is a non-deadlock cleanup failure. That can re-enter the callback after an unconfirmed rollback state.

Proposed fix 
-                    $maxRetries = self::isDeadlock($action) ? $deadlockRetries : $retries;
+                    $maxRetries = self::isDeadlock($rollback) ? $deadlockRetries : $retries;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$maxRetries = self::isDeadlock($action) ? $deadlockRetries : $retries;
if ($attempts < $maxRetries) {
\usleep(self::retrySleep($sleep, $attempts));
$maxRetries = self::isDeadlock($rollback) ? $deadlockRetries : $retries;
if ($attempts < $maxRetries) {
\usleep(self::retrySleep($sleep, $attempts));
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Database/Adapter.php` around lines 417 - 419, The retry logic in
Adapter::runRetryCallback is using the original $action to choose $maxRetries,
which can wrongly apply deadlock retries even when the rollback failure itself
is not a deadlock. Update the retry budget calculation so it is based on the
rollback error state (the $rollback exception/error) rather than $action, and
keep the retry/sleep flow in this block consistent with the existing retry
helper methods like isDeadlock() and retrySleep().

continue;
}

Expand All @@ -434,8 +436,9 @@ public function withTransaction(callable $callback): mixed
throw $action;
}

if ($attempts < $retries) {
\usleep($sleep * ($attempts + 1));
$maxRetries = self::isDeadlock($action) ? $deadlockRetries : $retries;
if ($attempts < $maxRetries) {
\usleep(self::retrySleep($sleep, $attempts));
continue;
}

Expand All @@ -447,6 +450,27 @@ public function withTransaction(callable $callback): mixed
throw new TransactionException('Failed to execute transaction');
}

/**
* Check if a throwable is a database deadlock error.
*/
protected static function isDeadlock(\Throwable $e): bool
{
if ($e instanceof \PDOException && (string)$e->getCode() === '40001') {
return true;
}

return \str_contains($e->getMessage(), 'Deadlock found when trying to get lock');
Comment on lines +456 to +462

@greptile-apps greptile-apps Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 isDeadlock message fallback matches any \Throwable

The second condition — str_contains($e->getMessage(), 'Deadlock found when trying to get lock') — applies to every \Throwable, not just PDOException. An application-level exception whose message happens to contain that string (e.g. wrapping or logging a previous deadlock) would silently receive 5 retries instead of 2. Limiting the message check to PDOException (parallel to the SQLSTATE check above it) would make the detection more precise.

Suggested change
protected static function isDeadlock(\Throwable $e): bool
{
if ($e instanceof \PDOException && (string)$e->getCode() === '40001') {
return true;
}
return \str_contains($e->getMessage(), 'Deadlock found when trying to get lock');
if ($e instanceof \PDOException && (string)$e->getCode() === '40001') {
return true;
}
return $e instanceof \PDOException && \str_contains($e->getMessage(), 'Deadlock found when trying to get lock');

Comment on lines +456 to +462

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

PostgreSQL SQLSTATE code for deadlock_detected and serialization_failure

💡 Result:

In PostgreSQL, the SQLSTATE error codes for deadlock_detected and serialization_failure are as follows: - serialization_failure: 40001 [1][2][3] - deadlock_detected: 40P01 [4][1][2][3] These codes are part of Class 40 — Transaction Rollback, and applications are typically expected to retry transactions that encounter these specific errors [1][2][3].

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Adapter.php outline =="
ast-grep outline src/Database/Adapter.php --view expanded || true

echo
echo "== Relevant slice =="
sed -n '420,500p' src/Database/Adapter.php

echo
echo "== isDeadlock usages =="
rg -n "isDeadlock\(" src/Database -S

echo
echo "== adapter subclasses / drivers =="
fd -t f 'Adapter.php' src/Database -x printf '%s\n' {}
fd -t f -g '*Adapter.php' src/Database | sed -n '1,80p'

Repository: utopia-php/database

Length of output: 3202

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== PostgreSQL-related files =="
git ls-files | rg -n 'Postgre|pgsql|pg_'

echo
echo "== PostgreSQL adapter / driver references =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' -i 'pgsql|postgres|40P01|deadlock detected|serialization failure' src test . || true

echo
echo "== Adapter subclasses =="
fd -t f 'Adapter.php' src | sed -n '1,120p'

Repository: utopia-php/database

Length of output: 9686

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '380,460p' src/Database/Adapter.php

echo
echo "== SQLTransactionTest slice =="
sed -n '1,220p' tests/unit/SQLTransactionTest.php

Repository: utopia-php/database

Length of output: 8774

Add PostgreSQL deadlock SQLSTATE 40P01 to the shared retry helper.
40001 covers serialization failures; PostgreSQL deadlocks use 40P01, so those transactions still fall back to the generic 2-retry path.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Database/Adapter.php` around lines 456 - 462, The shared deadlock retry
helper in Adapter::isDeadlock only recognizes SQLSTATE 40001 and the message
fallback, so PostgreSQL deadlocks are missed. Update isDeadlock to also treat
PDOException instances with SQLSTATE 40P01 as deadlocks, alongside the existing
40001 check and message-based match, so the retry path catches PostgreSQL
deadlock failures too.

}

/**
* Calculate retry sleep with exponential backoff and jitter.
*/
protected static function retrySleep(int $baseSleep, int $attempt): int
{
$delay = $baseSleep * (2 ** $attempt);
return $delay + \random_int(0, $delay);
}

/**
* Apply a transformation to a query before an event occurs
*
Expand Down
165 changes: 165 additions & 0 deletions tests/unit/SQLTransactionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
<?php

namespace Tests\Unit;

use PDOException;
use PHPUnit\Framework\TestCase;
use ReflectionMethod;
use Utopia\Database\Adapter\MySQL;
use Utopia\Database\Adapter\Postgres;
use Utopia\Database\Adapter\SQL;
use Utopia\Database\Exception\Transaction as TransactionException;

final class SQLTransactionTest extends TestCase
{
/**
* A deadlock (SQLSTATE 40001) inside withTransaction should be retried
* more aggressively than a generic exception (which gets only 2 retries).
*/
public function testWithTransactionRetriesDeadlockMoreThanGenericError(): void
{
$pdo = $this->getMockBuilder(\PDO::class)
->disableOriginalConstructor()
->getMock();

$pdo->method('inTransaction')->willReturn(true);
$pdo->method('beginTransaction')->willReturn(true);
$pdo->method('rollBack')->willReturn(true);

$adapter = new MySQL($pdo);

// First, verify a generic RuntimeException gets only 3 attempts (initial + 2 retries)
$genericAttempts = 0;
try {
$adapter->withTransaction(function () use (&$genericAttempts) {
$genericAttempts++;
throw new \RuntimeException('Generic failure');
});
} catch (\RuntimeException) {
}
$this->assertEquals(3, $genericAttempts, 'Generic exception should get 3 attempts (initial + 2 retries)');

// Now verify a deadlock PDOException gets more attempts
$deadlockAttempts = 0;
try {
$adapter->withTransaction(function () use (&$deadlockAttempts) {
$deadlockAttempts++;
throw new PDOException(
'SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction',
'40001'
);
});
} catch (PDOException) {
}
$this->assertGreaterThan(
$genericAttempts,
$deadlockAttempts,
'Deadlock should get more retry attempts than a generic exception'
);
$this->assertEquals(6, $deadlockAttempts, 'Deadlock should get 6 attempts (initial + 5 retries)');
}

/**
* When a deadlock resolves on a retry, the transaction should succeed.
*/
public function testWithTransactionSucceedsAfterTransientDeadlock(): void
{
$pdo = $this->getMockBuilder(\PDO::class)
->disableOriginalConstructor()
->getMock();

$pdo->method('inTransaction')->willReturn(true);
$pdo->method('beginTransaction')->willReturn(true);
$pdo->method('rollBack')->willReturn(true);
$pdo->method('commit')->willReturn(true);

$adapter = new MySQL($pdo);

$attempts = 0;
$result = $adapter->withTransaction(function () use (&$attempts) {
$attempts++;
if ($attempts < 3) {
throw new PDOException(
'SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction',
'40001'
);
}
return 'success';
});

$this->assertEquals('success', $result);
$this->assertEquals(3, $attempts, 'Should succeed on the 3rd attempt after 2 deadlocks');
}

/**
* 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
{
$method = new ReflectionMethod(Postgres::class, 'startTransaction');
$this->assertSame(SQL::class, $method->getDeclaringClass()->getName());

$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());
}
Comment on lines +101 to +164

@greptile-apps greptile-apps Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Three startTransaction tests verify behaviour that doesn't exist

testStartTransactionRecoversFromDesyncedRollback, testStartTransactionDoesNotMaskBeginFailureAfterDesyncedRollback, and testPostgresStartTransactionRecoversFromDesyncedRollback all set up a scenario where PDO::rollBack() throws PDOException('There is no active transaction'), then expect startTransaction() to swallow that specific error and proceed to call beginTransaction(). But SQL::startTransaction() wraps every PDOException in a single try/catch block covering both rollBack() and beginTransaction(). When rollBack() throws, execution jumps straight to the catch and re-throws as TransactionException('Failed to start transaction: There is no active transaction') — beginTransaction() is never reached.

Concretely: the two "recover" tests will fail because startTransaction() throws instead of returning true, and testStartTransactionDoesNotMaskBeginFailureAfterDesyncedRollback will fail because the actual exception message is 'There is no active transaction', not 'Connection lost'. The implementation change needed to make these pass (selectively ignoring "no active transaction" rollback errors and retrying beginTransaction) is not present in this PR.

Comment on lines +101 to +164

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

Align these tests with the actual startTransaction implementation.

The supplied SQL::startTransaction() snippet wraps the rollback PDOException immediately, so Lines 117 and 140 won’t reach the expected paths. The supplied Postgres snippet also declares its own startTransaction(), so Line 146 would not resolve to SQL::class. Update the production implementations in the same PR or adjust these assertions.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/SQLTransactionTest.php` around lines 101 - 164, The tests for
SQL::startTransaction and Postgres::startTransaction do not match the current
implementation behavior. Update the production methods so
SQL::startTransaction() can recover from a desynced rollback before calling
beginTransaction(), and ensure Postgres delegates to SQL::startTransaction()
rather than declaring its own override. If the implementation is intended to
stay as-is, adjust the assertions in these tests to expect the rollback
PDOException path and the actual declaring class returned by ReflectionMethod.

}