Skip to content

Commit ed30190

Browse files
abnegateclaude
andcommitted
fix(documents): stop a cache blip from failing a write it cannot make stale
A Redis reply that misses the 0.1s read timeout was turning stored writes into 500s. In production every still-firing group fails after the commit: purgeWrittenDocuments (CLOUD-3R97), the definition purge in the epoch publish (CLOUD-3R95) and the epoch save itself (CLOUD-3R8D). Throwing there cannot undo the write; it only reports a stored write as failed and makes queue workers repeat it. Every step after the commit already leaves the cache fail-closed when it cannot be confirmed, so it is now retried and then logged instead of thrown. Purge events still fire and their listener failures still reach the caller. Before the commit, a document purge that outlasts its retries used to refuse the write. It now blocks the collection's epoch instead, the marker #1042 showed is the one that guarantees no stale read; only when that fails too is the write refused, as during a real outage. The finish purge is retried only while #finished is unchanged, so a purge that timed out but landed is never counted twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent e1c599d commit ed30190

11 files changed

Lines changed: 256 additions & 146 deletions

‎src/Database/Database.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3323,7 +3323,7 @@ private function cleanup(
33233323
/**
33243324
* Persist metadata with automatic rollback on failure
33253325
*
3326-
* A failure raised after the metadata write committed (its cache invalidation or events), or a commit that
3326+
* A failure raised after the metadata write committed (its events), or a commit that
33273327
* could not be confirmed, is rethrown unchanged and rolls nothing back: the definition it reports on may be stored.
33283328
*
33293329
* @param callable|null $rollbackOperation Cleanup operation to run if persistence fails (null if no cleanup needed)

‎src/Database/Trait/Documents.php‎

Lines changed: 100 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Utopia\Database\Attribute;
1717
use Utopia\Database\Cache\Epoch;
1818
use Utopia\Database\Cache\Owners;
19+
use Utopia\Database\Cache\Registration;
1920
use Utopia\Database\Cache\Scope;
2021
use Utopia\Database\Capability;
2122
use Utopia\Database\Change;
@@ -3545,7 +3546,9 @@ private function purgeCommittedCollection(string $collection): void
35453546
}
35463547

35473548
/**
3548-
* Purge a document's cache slot, and once more after the open invalidation scope ends.
3549+
* Purge a document's cache slot, and once more after the open invalidation scope ends. A write whose purge
3550+
* cannot be confirmed blocks its collection's epoch instead, before it commits, so the document cached
3551+
* before it is never served again; only when that fails too is the write refused.
35493552
*
35503553
* @throws Exception
35513554
*/
@@ -3575,7 +3578,20 @@ protected function purgeCachedDocumentInternal(string $collectionId, ?string $id
35753578
);
35763579
}
35773580

3578-
$this->withRetries(fn () => $this->cache->purge($documentKey));
3581+
$scoped = $collectionId !== self::METADATA && isset($this->documentCacheMutations[$context]);
3582+
try {
3583+
if ($scoped && isset($this->documentCacheMutations[$context][$collectionKey])) {
3584+
$this->cache->purge($documentKey);
3585+
} else {
3586+
$this->withRetries(fn () => $this->cache->purge($documentKey));
3587+
}
3588+
} catch (Throwable $error) {
3589+
if (! $scoped) {
3590+
throw $error;
3591+
}
3592+
3593+
$this->advanceDocumentCacheEpoch($collectionKey, $this->getDefinitionCacheKey($collectionId));
3594+
}
35793595

35803596
return true;
35813597
}
@@ -3619,7 +3635,7 @@ private function purgeCachedDefinitions(): void
36193635
* Purge the documents a transaction wrote once it has committed or rolled back. A reader
36203636
* outside the transaction may have cached the pre-commit row after the purge inside it, so
36213637
* when this purge fails the collection's epoch is retired instead, which no such fill survives.
3622-
* A collection definition has no epoch of its own: its purge is tried once more.
3638+
* A collection definition has no epoch of its own, so its failed purge is the one thrown.
36233639
*
36243640
* @param array<string, string> $documents Collection keys by document key
36253641
*/
@@ -3628,20 +3644,29 @@ protected function purgeWrittenDocuments(array $documents): void
36283644
$definitions = $this->documentCacheDefinitions[$this->getEventContext()] ?? [];
36293645
$failure = null;
36303646
$retired = [];
3647+
$reachable = true;
36313648
foreach ($documents as $documentKey => $collectionKey) {
3649+
if (isset($retired[$collectionKey])) {
3650+
continue;
3651+
}
36323652
try {
3633-
$this->cache->purge($documentKey);
3653+
if ($reachable) {
3654+
$this->withRetries(fn () => $this->cache->purge($documentKey));
3655+
} else {
3656+
$this->cache->purge($documentKey);
3657+
}
36343658
} catch (Throwable $error) {
3635-
$failure ??= $error;
3659+
$reachable = false;
3660+
if (! isset($definitions[$collectionKey])) {
3661+
$failure ??= $error;
3662+
3663+
continue;
3664+
}
36363665
try {
3637-
if (! isset($definitions[$collectionKey])) {
3638-
$this->cache->purge($documentKey);
3639-
} elseif (! isset($retired[$collectionKey])) {
3640-
$retired[$collectionKey] = true;
3641-
$this->advanceDocumentCacheEpoch($collectionKey, $definitions[$collectionKey]);
3642-
}
3666+
$this->advanceDocumentCacheEpoch($collectionKey, $definitions[$collectionKey]);
3667+
$retired[$collectionKey] = true;
36433668
} catch (Throwable) {
3644-
// The purge failure below reaches the caller either way.
3669+
$failure ??= $error;
36453670
}
36463671
}
36473672
}
@@ -3838,14 +3863,7 @@ private function activateDocumentCacheEpoch(string $collectionKey, string $token
38383863
if ($owner !== false && $owner !== null && $owner !== $token) {
38393864
throw new RuntimeException("Invalid document cache owner '{$token}' for '{$collectionKey}'");
38403865
}
3841-
$owned = $owner === $token;
3842-
if ($owned && ! $this->cache->purge($registration->key, $registration->field)) {
3843-
$owner = $this->cache->load($registration->key, self::TTL, $registration->field);
3844-
if ($owner !== false && $owner !== null) {
3845-
throw new RuntimeException("Failed to release document cache owner '{$token}' for '{$collectionKey}'");
3846-
}
3847-
$owned = false;
3848-
}
3866+
$owned = $owner === $token && $this->releaseDocumentCacheOwner($registration, $collectionKey, $token);
38493867

38503868
$startedKey = $collectionKey.'#started';
38513869
$finishedKey = $collectionKey.'#finished';
@@ -3874,19 +3892,7 @@ private function activateDocumentCacheEpoch(string $collectionKey, string $token
38743892
return;
38753893
}
38763894

3877-
$this->cache->purge($finishedKey);
3878-
$nextFinished = $this->cache->getGeneration($finishedKey);
3879-
3880-
// A cache flush restarts generations, so an unchanged #finished
3881-
// only proves this purge was lost while the epoch read with it is
3882-
// still in place; publish the new epoch after this check, not before.
3883-
if (
3884-
$nextFinished === $finished
3885-
&& \is_string($epoch)
3886-
&& $this->cache->load($epochKey, self::DOCUMENT_CACHE_PERMANENT) === $epoch
3887-
) {
3888-
throw new RuntimeException("Failed to finish document cache invalidation '{$epochKey}'");
3889-
}
3895+
$nextFinished = $this->finishDocumentCacheWrite($collectionKey, $finished, $epoch);
38903896

38913897
$nextStarted = $this->cache->getGeneration($startedKey);
38923898
if ($nextStarted === $nextFinished) {
@@ -3912,11 +3918,68 @@ private function publishDocumentCacheEpoch(string $collectionKey, string $starte
39123918
{
39133919
$epochKey = $collectionKey.'#epoch';
39143920
$epoch = self::DOCUMENT_CACHE_ACTIVE_PREFIX.\bin2hex(\random_bytes(16)).self::DOCUMENT_CACHE_SEPARATOR.$started;
3915-
if ($this->cache->save($epochKey, $epoch) === false) {
3916-
throw new RuntimeException("Failed to activate document cache epoch '{$epochKey}'");
3917-
}
3921+
$this->withRetries(function () use ($epochKey, $epoch): void {
3922+
if ($this->cache->save($epochKey, $epoch) === false) {
3923+
throw new RuntimeException("Failed to activate document cache epoch '{$epochKey}'");
3924+
}
3925+
});
39183926

3919-
$this->purgeCachedDefinition($definitionKey);
3927+
$this->withRetries(fn () => $this->purgeCachedDefinition($definitionKey));
3928+
}
3929+
3930+
/**
3931+
* Release this write's registration, and report whether it was still this write's to release. A purge
3932+
* that cannot be confirmed is retried while the registration is still in place.
3933+
*/
3934+
private function releaseDocumentCacheOwner(Registration $registration, string $collectionKey, string $token): bool
3935+
{
3936+
$released = true;
3937+
$this->withRetries(function () use ($registration, $collectionKey, $token, &$released): void {
3938+
if ($this->cache->purge($registration->key, $registration->field)) {
3939+
return;
3940+
}
3941+
3942+
$owner = $this->cache->load($registration->key, self::TTL, $registration->field);
3943+
if ($owner !== false && $owner !== null) {
3944+
throw new RuntimeException("Failed to release document cache owner '{$token}' for '{$collectionKey}'");
3945+
}
3946+
$released = false;
3947+
});
3948+
3949+
return $released;
3950+
}
3951+
3952+
/**
3953+
* Count this write as finished and return the `#finished` generation after it. A cache flush restarts
3954+
* generations, so an unchanged `#finished` only proves the purge was lost while the epoch read with it
3955+
* is still in place; the purge is retried only then, so a purge that did land is never counted twice.
3956+
*/
3957+
private function finishDocumentCacheWrite(string $collectionKey, string $finished, mixed $epoch): string
3958+
{
3959+
$finishedKey = $collectionKey.'#finished';
3960+
$epochKey = $collectionKey.'#epoch';
3961+
$next = $finished;
3962+
$attempted = false;
3963+
$this->withRetries(function () use ($finishedKey, $epochKey, $finished, $epoch, &$next, &$attempted): void {
3964+
if ($attempted) {
3965+
$next = $this->cache->getGeneration($finishedKey);
3966+
}
3967+
if ($next === $finished) {
3968+
$attempted = true;
3969+
$this->cache->purge($finishedKey);
3970+
}
3971+
3972+
$next = $this->cache->getGeneration($finishedKey);
3973+
if (
3974+
$next === $finished
3975+
&& \is_string($epoch)
3976+
&& $this->cache->load($epochKey, self::DOCUMENT_CACHE_PERMANENT) === $epoch
3977+
) {
3978+
throw new RuntimeException("Failed to finish document cache invalidation '{$epochKey}'");
3979+
}
3980+
});
3981+
3982+
return $next;
39203983
}
39213984

39223985
/**

‎src/Database/Trait/Transactions.php‎

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -294,26 +294,9 @@ private function withInvalidationScope(callable $callback): mixed
294294
$this->documentPurgeEvents[$context],
295295
);
296296

297-
$failure = null;
298-
try {
299-
$this->purgeWrittenDocuments($documents);
300-
} catch (Throwable $error) {
301-
$failure = $error;
302-
}
303-
try {
304-
$this->activateDocumentInvalidation($documentTokens);
305-
} catch (Throwable $error) {
306-
$failure ??= $error;
307-
}
308-
try {
309-
$this->activateInvalidation($queryTokens);
310-
} catch (Throwable $error) {
311-
$failure ??= $error;
312-
}
313-
314-
$announcement = $this->announceDocumentPurges($purgeEvents);
315-
$failure ??= $announcement;
297+
$this->invalidateCommitted($documents, $documentTokens, $queryTokens);
316298

299+
$failure = $this->announceDocumentPurges($purgeEvents);
317300
if ($failure !== null) {
318301
$this->committedFailures ??= new WeakMap();
319302
$this->committedFailures[$failure] = true;
@@ -327,6 +310,32 @@ private function withInvalidationScope(callable $callback): mixed
327310
return $result;
328311
}
329312

313+
/**
314+
* Invalidate the cache for writes that have committed. The writes are stored whatever the cache answers, and each
315+
* step that cannot be confirmed leaves the cache fail-closed: a tombstone left in place lapses into a new epoch,
316+
* never back to the old one. A failure is logged, as throwing it would report a stored write as failed.
317+
*
318+
* @param array<string, string> $documents Collection keys by document key
319+
* @param array<string, string> $documentTokens
320+
* @param array<string, string> $queryTokens
321+
*/
322+
private function invalidateCommitted(array $documents, array $documentTokens, array $queryTokens): void
323+
{
324+
$steps = [
325+
fn () => $this->purgeWrittenDocuments($documents),
326+
fn () => $this->activateDocumentInvalidation($documentTokens),
327+
fn () => $this->activateInvalidation($queryTokens),
328+
];
329+
330+
foreach ($steps as $step) {
331+
try {
332+
$step();
333+
} catch (Throwable $error) {
334+
Console::warning('Warning: Failed to invalidate the cache after a commit: '.$error->getMessage());
335+
}
336+
}
337+
}
338+
330339
private function queueDefinitionRefill(string $definitionKey, string $field, string $id): void
331340
{
332341
$tenant = $this->adapter->getTenant();
@@ -370,7 +379,7 @@ private function cacheTransactionDefinitions(array $refills): void
370379

371380
/**
372381
* Whether the error was raised after its outermost transaction committed: the writes it
373-
* reports on are stored, and only the invalidation or the events after the commit failed.
382+
* reports on are stored, and only the events after the commit failed.
374383
*/
375384
private function failedAfterCommit(Throwable $error): bool
376385
{

‎tests/unit/Cache/DatabaseTest.php‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -616,12 +616,8 @@ public function testActivationFailureAfterCommitLeavesSharedEpochBlocked(): void
616616
);
617617
$cache->failActivations();
618618

619-
try {
620-
$writer->updateDocument('users', 'existing', new Document(['name' => 'updated']));
621-
$this->fail('Post-commit query-cache activation failure was not propagated');
622-
} catch (\RuntimeException $exception) {
623-
$this->assertStringContainsString('activate query cache', $exception->getMessage());
624-
}
619+
$updated = $writer->updateDocument('users', 'existing', new Document(['name' => 'updated']));
620+
$this->assertSame('updated', $updated->getAttribute('name'), 'A stored write must not be reported as failed because its query-cache activation failed');
625621

626622
$readerAdapter->observeFinds('users');
627623
$this->assertSame(

‎tests/unit/Cache/UnavailableCache.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ final class UnavailableCache implements CacheAdapter, Leasable
2121

2222
private int $failingPurges = 0;
2323

24+
private int $failingSaves = 0;
25+
2426
public function __construct()
2527
{
2628
$this->cache = new RedisLeasableCache();
@@ -44,6 +46,14 @@ public function failNextPurges(int $count): void
4446
$this->failingPurges = $count;
4547
}
4648

49+
/**
50+
* A blip on writes: the next saves report false, later ones are stored.
51+
*/
52+
public function failNextSaves(int $count): void
53+
{
54+
$this->failingSaves = $count;
55+
}
56+
4757
public function recover(): void
4858
{
4959
$this->open = false;
@@ -77,6 +87,12 @@ public function load(string $key, int $ttl, string $hash = ''): mixed
7787
#[\Override]
7888
public function save(string $key, array|string $data, string $hash = '', int $ttl = 0): bool|string|array
7989
{
90+
if ($this->failingSaves > 0) {
91+
$this->failingSaves--;
92+
93+
return false;
94+
}
95+
8096
return $this->writable() ? $this->cache->save($key, $data, $hash) : false;
8197
}
8298

‎tests/unit/CoreMinorsTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ public function testCreateIndexKeepsItsIndexWhenTheInvalidationAfterTheCommitFai
337337

338338
$this->assertSame(0, $deletes, 'An index whose definition committed must not be rolled back');
339339
$this->assertSame(1, $writes, 'A write that committed must not be repeated');
340-
$this->assertSame($failures[0] ?? null, $error, 'The failure after the commit must reach the caller as it was raised');
340+
$this->assertNull($error, 'A stored write must not be reported as failed because the invalidation after its commit failed');
341341
$this->assertSame(['by_count'], $this->indexKeys($database, 'logs'));
342342
$this->assertTrue($this->hasSchemaIndex($database, 'logs', 'by_count'), 'The committed index must still exist');
343343
}
@@ -423,7 +423,7 @@ public function testCreateRelationshipKeepsItsWorkWhenTheInvalidationAfterTheCom
423423
$this->assertSame(['_index_profile'], $this->indexKeys($database, 'accounts'), 'The two-way index must still be created');
424424
$this->assertTrue($this->hasSchemaIndex($database, 'profiles', '_index_account'));
425425
$this->assertTrue($this->hasSchemaIndex($database, 'accounts', '_index_profile'));
426-
$this->assertSame($failures[0] ?? null, $error, 'The failure after the commit must reach the caller as it was raised');
426+
$this->assertNull($error, 'A stored write must not be reported as failed because the invalidation after its commit failed');
427427
}
428428

429429
/**

‎tests/unit/CreateCollectionCleanupTest.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,9 @@ public function commitTransaction(): bool
9898
$adapter->afterCommit = null;
9999
$cache->failing = false;
100100

101-
$this->assertNotNull($error, 'A failed invalidation after the commit must reach the caller');
101+
$this->assertNull($error, 'A stored collection must not be reported as failed because the invalidation after its commit failed');
102102
$this->assertTrue($adapter->collectionExists('cleanup', 'logs'), 'A collection whose definition committed must keep its table');
103103
$this->assertSame('logs', $database->getCollection('logs')->getId());
104-
$this->assertSame($failure, $error, 'The failure after the commit must reach the caller as it was raised');
105104

106105
$database->createDocument('logs', new Document(['message' => 'kept']));
107106
$this->assertSame(1, $database->count('logs'));

0 commit comments

Comments
 (0)