Skip to content

Commit 7fd133e

Browse files
authored
Merge pull request #927 from utopia-php/disable-timeout-retries
Exclude timeout exception
2 parents 0797ba6 + 3a51f51 commit 7fd133e

4 files changed

Lines changed: 186 additions & 12 deletions

File tree

src/Database/Adapter.php

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -438,16 +438,15 @@ public function withTransaction(callable $callback): mixed
438438
$this->commitTransaction();
439439
return $result;
440440
} catch (\Throwable $action) {
441+
$rollback = null;
441442
try {
442443
$this->rollbackTransaction();
443-
} catch (\Throwable $rollback) {
444-
if ($attempts < $retries) {
445-
\usleep($sleep * ($attempts + 1));
446-
continue;
447-
}
448-
444+
} catch (\Throwable $rollbackError) {
445+
// Not every adapter resets the depth counter when its
446+
// rollback throws (e.g. Redis), so reset it here to avoid
447+
// leaking transaction state onto the reused connection.
448+
$rollback = $rollbackError;
449449
$this->inTransaction = 0;
450-
throw $rollback;
451450
}
452451

453452
if (
@@ -456,7 +455,8 @@ public function withTransaction(callable $callback): mixed
456455
$action instanceof AuthorizationException ||
457456
$action instanceof RelationshipException ||
458457
$action instanceof ConflictException ||
459-
$action instanceof LimitException
458+
$action instanceof LimitException ||
459+
$action instanceof TimeoutException
460460
) {
461461
throw $action;
462462
}
@@ -466,7 +466,7 @@ public function withTransaction(callable $callback): mixed
466466
continue;
467467
}
468468

469-
throw $action;
469+
throw $rollback ?? $action;
470470
}
471471
}
472472

src/Database/Adapter/Mongo.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,8 @@ public function withTransaction(callable $callback): mixed
173173
$action instanceof AuthorizationException ||
174174
$action instanceof RelationshipException ||
175175
$action instanceof ConflictException ||
176-
$action instanceof LimitException
176+
$action instanceof LimitException ||
177+
$action instanceof TimeoutException
177178
) {
178179
throw $action;
179180
}

src/Database/Adapter/Redis.php

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4000,8 +4000,20 @@ public function rollbackTransaction(): bool
40004000
return false;
40014001
}
40024002

4003-
$this->rollbackJournal();
4004-
$this->inTransaction--;
4003+
try {
4004+
$this->rollbackJournal();
4005+
$this->inTransaction--;
4006+
} catch (\Throwable $e) {
4007+
// A failed rollback (mid-replay) leaves the transaction in an
4008+
// indeterminate state. Discard all pending journal state so the
4009+
// connection is clean for reuse. Both must be cleared together to
4010+
// preserve the count($journalStack) === inTransaction invariant:
4011+
// resetting only the counter would strand parent frames that later
4012+
// transactions merge into, growing the stack without bound.
4013+
$this->inTransaction = 0;
4014+
$this->journalStack = [];
4015+
throw $e;
4016+
}
40054017

40064018
return true;
40074019
}
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
<?php
2+
3+
namespace Tests\Unit;
4+
5+
use PHPUnit\Framework\TestCase;
6+
use Utopia\Database\Adapter\Memory as DatabaseMemory;
7+
use Utopia\Database\Adapter\Redis as RedisAdapter;
8+
use Utopia\Database\Exception\Duplicate as DuplicateException;
9+
use Utopia\Database\Exception\Timeout as TimeoutException;
10+
11+
/**
12+
* Covers the retry policy of Adapter::withTransaction(): which exceptions abort
13+
* immediately versus which are retried up to the built-in attempt budget.
14+
*/
15+
class TransactionRetryTest extends TestCase
16+
{
17+
private DatabaseMemory $adapter;
18+
19+
protected function setUp(): void
20+
{
21+
$this->adapter = new DatabaseMemory();
22+
}
23+
24+
/**
25+
* A statement timeout already spent the full timeout budget on this attempt;
26+
* retrying re-runs it for another full budget and amplifies lock convoys.
27+
* It must abort after a single attempt.
28+
*/
29+
public function testTimeoutIsNotRetried(): void
30+
{
31+
$attempts = 0;
32+
$thrown = null;
33+
34+
try {
35+
$this->adapter->withTransaction(function () use (&$attempts) {
36+
$attempts++;
37+
throw new TimeoutException('Query timed out');
38+
});
39+
} catch (TimeoutException $e) {
40+
$thrown = $e;
41+
}
42+
43+
$this->assertInstanceOf(TimeoutException::class, $thrown);
44+
$this->assertSame(1, $attempts);
45+
}
46+
47+
/**
48+
* A duplicate is deterministic, so it also aborts on the first attempt.
49+
* Anchors the timeout case against an existing no-retry exception.
50+
*/
51+
public function testDuplicateIsNotRetried(): void
52+
{
53+
$attempts = 0;
54+
$thrown = null;
55+
56+
try {
57+
$this->adapter->withTransaction(function () use (&$attempts) {
58+
$attempts++;
59+
throw new DuplicateException('Duplicate');
60+
});
61+
} catch (DuplicateException $e) {
62+
$thrown = $e;
63+
}
64+
65+
$this->assertInstanceOf(DuplicateException::class, $thrown);
66+
$this->assertSame(1, $attempts);
67+
}
68+
69+
/**
70+
* A transient/unknown failure is still retried across the full attempt
71+
* budget (3 attempts: initial + 2 retries) before the error propagates.
72+
*/
73+
public function testGenericFailureIsRetried(): void
74+
{
75+
$attempts = 0;
76+
$thrown = null;
77+
78+
try {
79+
$this->adapter->withTransaction(function () use (&$attempts) {
80+
$attempts++;
81+
throw new \RuntimeException('transient');
82+
});
83+
} catch (\RuntimeException $e) {
84+
$thrown = $e;
85+
}
86+
87+
$this->assertInstanceOf(\RuntimeException::class, $thrown);
88+
$this->assertSame(3, $attempts);
89+
}
90+
91+
/**
92+
* Rollback cleanup can itself fail (adapters throw a DatabaseException from
93+
* rollbackTransaction()). A non-retriable action must still abort after a
94+
* single attempt and propagate the original action, not be retried or
95+
* masked by the rollback error.
96+
*/
97+
public function testNonRetriableActionAbortsWhenRollbackFails(): void
98+
{
99+
$adapter = new class () extends DatabaseMemory {
100+
public function rollbackTransaction(): bool
101+
{
102+
throw new \RuntimeException('rollback failed');
103+
}
104+
};
105+
106+
$attempts = 0;
107+
$thrown = null;
108+
109+
try {
110+
$adapter->withTransaction(function () use (&$attempts) {
111+
$attempts++;
112+
throw new TimeoutException('Query timed out');
113+
});
114+
} catch (\Throwable $e) {
115+
$thrown = $e;
116+
}
117+
118+
$this->assertInstanceOf(TimeoutException::class, $thrown);
119+
$this->assertSame(1, $attempts);
120+
}
121+
122+
/**
123+
* A failed rollback in the Redis adapter must reset the depth counter and
124+
* the journal stack together. Resetting only the counter would strand the
125+
* parent frames, breaking the count($journalStack) === inTransaction
126+
* invariant and letting later transactions merge into a stale frame.
127+
*/
128+
public function testRedisRollbackFailureClearsJournalStack(): void
129+
{
130+
if (!\extension_loaded('redis')) {
131+
$this->markTestSkipped('redis extension not loaded');
132+
}
133+
134+
$adapter = new class (new \Redis()) extends RedisAdapter {
135+
protected function rollbackJournal(): void
136+
{
137+
throw new \RuntimeException('rollback replay failed');
138+
}
139+
};
140+
141+
$adapter->startTransaction();
142+
$adapter->startTransaction();
143+
144+
$thrown = null;
145+
try {
146+
$adapter->rollbackTransaction();
147+
} catch (\Throwable $e) {
148+
$thrown = $e;
149+
}
150+
151+
$this->assertInstanceOf(\RuntimeException::class, $thrown);
152+
153+
$inTransaction = new \ReflectionProperty(RedisAdapter::class, 'inTransaction');
154+
$inTransaction->setAccessible(true);
155+
$this->assertSame(0, $inTransaction->getValue($adapter));
156+
157+
$journalStack = new \ReflectionProperty(RedisAdapter::class, 'journalStack');
158+
$journalStack->setAccessible(true);
159+
$this->assertSame([], $journalStack->getValue($adapter));
160+
}
161+
}

0 commit comments

Comments
 (0)