From ed30190ea74d76bddec4f9fb930ef945149f082e Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 22 Oct 2026 13:57:49 +1300 Subject: [PATCH] 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 --- src/Database/Database.php | 2 +- src/Database/Trait/Documents.php | 137 +++++++++++++----- src/Database/Trait/Transactions.php | 49 ++++--- tests/unit/Cache/DatabaseTest.php | 8 +- tests/unit/Cache/UnavailableCache.php | 16 ++ tests/unit/CoreMinorsTest.php | 4 +- tests/unit/CreateCollectionCleanupTest.php | 3 +- tests/unit/Documents/CacheUnavailableTest.php | 68 ++++++++- .../unit/Documents/DocumentCacheEpochTest.php | 111 +++++--------- .../DocumentCacheInvalidationTest.php | 2 +- tests/unit/Event/DocumentPurgeTest.php | 2 +- 11 files changed, 256 insertions(+), 146 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 5f8d66de2..12d85006b 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -3323,7 +3323,7 @@ private function cleanup( /** * Persist metadata with automatic rollback on failure * - * A failure raised after the metadata write committed (its cache invalidation or events), or a commit that + * A failure raised after the metadata write committed (its events), or a commit that * could not be confirmed, is rethrown unchanged and rolls nothing back: the definition it reports on may be stored. * * @param callable|null $rollbackOperation Cleanup operation to run if persistence fails (null if no cleanup needed) diff --git a/src/Database/Trait/Documents.php b/src/Database/Trait/Documents.php index e37fcda84..c73dbef17 100644 --- a/src/Database/Trait/Documents.php +++ b/src/Database/Trait/Documents.php @@ -16,6 +16,7 @@ use Utopia\Database\Attribute; use Utopia\Database\Cache\Epoch; use Utopia\Database\Cache\Owners; +use Utopia\Database\Cache\Registration; use Utopia\Database\Cache\Scope; use Utopia\Database\Capability; use Utopia\Database\Change; @@ -3545,7 +3546,9 @@ private function purgeCommittedCollection(string $collection): void } /** - * Purge a document's cache slot, and once more after the open invalidation scope ends. + * Purge a document's cache slot, and once more after the open invalidation scope ends. A write whose purge + * cannot be confirmed blocks its collection's epoch instead, before it commits, so the document cached + * before it is never served again; only when that fails too is the write refused. * * @throws Exception */ @@ -3575,7 +3578,20 @@ protected function purgeCachedDocumentInternal(string $collectionId, ?string $id ); } - $this->withRetries(fn () => $this->cache->purge($documentKey)); + $scoped = $collectionId !== self::METADATA && isset($this->documentCacheMutations[$context]); + try { + if ($scoped && isset($this->documentCacheMutations[$context][$collectionKey])) { + $this->cache->purge($documentKey); + } else { + $this->withRetries(fn () => $this->cache->purge($documentKey)); + } + } catch (Throwable $error) { + if (! $scoped) { + throw $error; + } + + $this->advanceDocumentCacheEpoch($collectionKey, $this->getDefinitionCacheKey($collectionId)); + } return true; } @@ -3619,7 +3635,7 @@ private function purgeCachedDefinitions(): void * Purge the documents a transaction wrote once it has committed or rolled back. A reader * outside the transaction may have cached the pre-commit row after the purge inside it, so * when this purge fails the collection's epoch is retired instead, which no such fill survives. - * A collection definition has no epoch of its own: its purge is tried once more. + * A collection definition has no epoch of its own, so its failed purge is the one thrown. * * @param array $documents Collection keys by document key */ @@ -3628,20 +3644,29 @@ protected function purgeWrittenDocuments(array $documents): void $definitions = $this->documentCacheDefinitions[$this->getEventContext()] ?? []; $failure = null; $retired = []; + $reachable = true; foreach ($documents as $documentKey => $collectionKey) { + if (isset($retired[$collectionKey])) { + continue; + } try { - $this->cache->purge($documentKey); + if ($reachable) { + $this->withRetries(fn () => $this->cache->purge($documentKey)); + } else { + $this->cache->purge($documentKey); + } } catch (Throwable $error) { - $failure ??= $error; + $reachable = false; + if (! isset($definitions[$collectionKey])) { + $failure ??= $error; + + continue; + } try { - if (! isset($definitions[$collectionKey])) { - $this->cache->purge($documentKey); - } elseif (! isset($retired[$collectionKey])) { - $retired[$collectionKey] = true; - $this->advanceDocumentCacheEpoch($collectionKey, $definitions[$collectionKey]); - } + $this->advanceDocumentCacheEpoch($collectionKey, $definitions[$collectionKey]); + $retired[$collectionKey] = true; } catch (Throwable) { - // The purge failure below reaches the caller either way. + $failure ??= $error; } } } @@ -3838,14 +3863,7 @@ private function activateDocumentCacheEpoch(string $collectionKey, string $token if ($owner !== false && $owner !== null && $owner !== $token) { throw new RuntimeException("Invalid document cache owner '{$token}' for '{$collectionKey}'"); } - $owned = $owner === $token; - if ($owned && ! $this->cache->purge($registration->key, $registration->field)) { - $owner = $this->cache->load($registration->key, self::TTL, $registration->field); - if ($owner !== false && $owner !== null) { - throw new RuntimeException("Failed to release document cache owner '{$token}' for '{$collectionKey}'"); - } - $owned = false; - } + $owned = $owner === $token && $this->releaseDocumentCacheOwner($registration, $collectionKey, $token); $startedKey = $collectionKey.'#started'; $finishedKey = $collectionKey.'#finished'; @@ -3874,19 +3892,7 @@ private function activateDocumentCacheEpoch(string $collectionKey, string $token return; } - $this->cache->purge($finishedKey); - $nextFinished = $this->cache->getGeneration($finishedKey); - - // A cache flush restarts generations, so an unchanged #finished - // only proves this purge was lost while the epoch read with it is - // still in place; publish the new epoch after this check, not before. - if ( - $nextFinished === $finished - && \is_string($epoch) - && $this->cache->load($epochKey, self::DOCUMENT_CACHE_PERMANENT) === $epoch - ) { - throw new RuntimeException("Failed to finish document cache invalidation '{$epochKey}'"); - } + $nextFinished = $this->finishDocumentCacheWrite($collectionKey, $finished, $epoch); $nextStarted = $this->cache->getGeneration($startedKey); if ($nextStarted === $nextFinished) { @@ -3912,11 +3918,68 @@ private function publishDocumentCacheEpoch(string $collectionKey, string $starte { $epochKey = $collectionKey.'#epoch'; $epoch = self::DOCUMENT_CACHE_ACTIVE_PREFIX.\bin2hex(\random_bytes(16)).self::DOCUMENT_CACHE_SEPARATOR.$started; - if ($this->cache->save($epochKey, $epoch) === false) { - throw new RuntimeException("Failed to activate document cache epoch '{$epochKey}'"); - } + $this->withRetries(function () use ($epochKey, $epoch): void { + if ($this->cache->save($epochKey, $epoch) === false) { + throw new RuntimeException("Failed to activate document cache epoch '{$epochKey}'"); + } + }); - $this->purgeCachedDefinition($definitionKey); + $this->withRetries(fn () => $this->purgeCachedDefinition($definitionKey)); + } + + /** + * Release this write's registration, and report whether it was still this write's to release. A purge + * that cannot be confirmed is retried while the registration is still in place. + */ + private function releaseDocumentCacheOwner(Registration $registration, string $collectionKey, string $token): bool + { + $released = true; + $this->withRetries(function () use ($registration, $collectionKey, $token, &$released): void { + if ($this->cache->purge($registration->key, $registration->field)) { + return; + } + + $owner = $this->cache->load($registration->key, self::TTL, $registration->field); + if ($owner !== false && $owner !== null) { + throw new RuntimeException("Failed to release document cache owner '{$token}' for '{$collectionKey}'"); + } + $released = false; + }); + + return $released; + } + + /** + * Count this write as finished and return the `#finished` generation after it. A cache flush restarts + * generations, so an unchanged `#finished` only proves the purge was lost while the epoch read with it + * is still in place; the purge is retried only then, so a purge that did land is never counted twice. + */ + private function finishDocumentCacheWrite(string $collectionKey, string $finished, mixed $epoch): string + { + $finishedKey = $collectionKey.'#finished'; + $epochKey = $collectionKey.'#epoch'; + $next = $finished; + $attempted = false; + $this->withRetries(function () use ($finishedKey, $epochKey, $finished, $epoch, &$next, &$attempted): void { + if ($attempted) { + $next = $this->cache->getGeneration($finishedKey); + } + if ($next === $finished) { + $attempted = true; + $this->cache->purge($finishedKey); + } + + $next = $this->cache->getGeneration($finishedKey); + if ( + $next === $finished + && \is_string($epoch) + && $this->cache->load($epochKey, self::DOCUMENT_CACHE_PERMANENT) === $epoch + ) { + throw new RuntimeException("Failed to finish document cache invalidation '{$epochKey}'"); + } + }); + + return $next; } /** diff --git a/src/Database/Trait/Transactions.php b/src/Database/Trait/Transactions.php index da8eefdea..a1ec7861c 100644 --- a/src/Database/Trait/Transactions.php +++ b/src/Database/Trait/Transactions.php @@ -294,26 +294,9 @@ private function withInvalidationScope(callable $callback): mixed $this->documentPurgeEvents[$context], ); - $failure = null; - try { - $this->purgeWrittenDocuments($documents); - } catch (Throwable $error) { - $failure = $error; - } - try { - $this->activateDocumentInvalidation($documentTokens); - } catch (Throwable $error) { - $failure ??= $error; - } - try { - $this->activateInvalidation($queryTokens); - } catch (Throwable $error) { - $failure ??= $error; - } - - $announcement = $this->announceDocumentPurges($purgeEvents); - $failure ??= $announcement; + $this->invalidateCommitted($documents, $documentTokens, $queryTokens); + $failure = $this->announceDocumentPurges($purgeEvents); if ($failure !== null) { $this->committedFailures ??= new WeakMap(); $this->committedFailures[$failure] = true; @@ -327,6 +310,32 @@ private function withInvalidationScope(callable $callback): mixed return $result; } + /** + * Invalidate the cache for writes that have committed. The writes are stored whatever the cache answers, and each + * step that cannot be confirmed leaves the cache fail-closed: a tombstone left in place lapses into a new epoch, + * never back to the old one. A failure is logged, as throwing it would report a stored write as failed. + * + * @param array $documents Collection keys by document key + * @param array $documentTokens + * @param array $queryTokens + */ + private function invalidateCommitted(array $documents, array $documentTokens, array $queryTokens): void + { + $steps = [ + fn () => $this->purgeWrittenDocuments($documents), + fn () => $this->activateDocumentInvalidation($documentTokens), + fn () => $this->activateInvalidation($queryTokens), + ]; + + foreach ($steps as $step) { + try { + $step(); + } catch (Throwable $error) { + Console::warning('Warning: Failed to invalidate the cache after a commit: '.$error->getMessage()); + } + } + } + private function queueDefinitionRefill(string $definitionKey, string $field, string $id): void { $tenant = $this->adapter->getTenant(); @@ -370,7 +379,7 @@ private function cacheTransactionDefinitions(array $refills): void /** * Whether the error was raised after its outermost transaction committed: the writes it - * reports on are stored, and only the invalidation or the events after the commit failed. + * reports on are stored, and only the events after the commit failed. */ private function failedAfterCommit(Throwable $error): bool { diff --git a/tests/unit/Cache/DatabaseTest.php b/tests/unit/Cache/DatabaseTest.php index e14d097e4..e20c01bcd 100644 --- a/tests/unit/Cache/DatabaseTest.php +++ b/tests/unit/Cache/DatabaseTest.php @@ -616,12 +616,8 @@ public function testActivationFailureAfterCommitLeavesSharedEpochBlocked(): void ); $cache->failActivations(); - try { - $writer->updateDocument('users', 'existing', new Document(['name' => 'updated'])); - $this->fail('Post-commit query-cache activation failure was not propagated'); - } catch (\RuntimeException $exception) { - $this->assertStringContainsString('activate query cache', $exception->getMessage()); - } + $updated = $writer->updateDocument('users', 'existing', new Document(['name' => 'updated'])); + $this->assertSame('updated', $updated->getAttribute('name'), 'A stored write must not be reported as failed because its query-cache activation failed'); $readerAdapter->observeFinds('users'); $this->assertSame( diff --git a/tests/unit/Cache/UnavailableCache.php b/tests/unit/Cache/UnavailableCache.php index c84a599ec..c207cb840 100644 --- a/tests/unit/Cache/UnavailableCache.php +++ b/tests/unit/Cache/UnavailableCache.php @@ -21,6 +21,8 @@ final class UnavailableCache implements CacheAdapter, Leasable private int $failingPurges = 0; + private int $failingSaves = 0; + public function __construct() { $this->cache = new RedisLeasableCache(); @@ -44,6 +46,14 @@ public function failNextPurges(int $count): void $this->failingPurges = $count; } + /** + * A blip on writes: the next saves report false, later ones are stored. + */ + public function failNextSaves(int $count): void + { + $this->failingSaves = $count; + } + public function recover(): void { $this->open = false; @@ -77,6 +87,12 @@ public function load(string $key, int $ttl, string $hash = ''): mixed #[\Override] public function save(string $key, array|string $data, string $hash = '', int $ttl = 0): bool|string|array { + if ($this->failingSaves > 0) { + $this->failingSaves--; + + return false; + } + return $this->writable() ? $this->cache->save($key, $data, $hash) : false; } diff --git a/tests/unit/CoreMinorsTest.php b/tests/unit/CoreMinorsTest.php index 871de5a6b..31b1d9589 100644 --- a/tests/unit/CoreMinorsTest.php +++ b/tests/unit/CoreMinorsTest.php @@ -337,7 +337,7 @@ public function testCreateIndexKeepsItsIndexWhenTheInvalidationAfterTheCommitFai $this->assertSame(0, $deletes, 'An index whose definition committed must not be rolled back'); $this->assertSame(1, $writes, 'A write that committed must not be repeated'); - $this->assertSame($failures[0] ?? null, $error, 'The failure after the commit must reach the caller as it was raised'); + $this->assertNull($error, 'A stored write must not be reported as failed because the invalidation after its commit failed'); $this->assertSame(['by_count'], $this->indexKeys($database, 'logs')); $this->assertTrue($this->hasSchemaIndex($database, 'logs', 'by_count'), 'The committed index must still exist'); } @@ -423,7 +423,7 @@ public function testCreateRelationshipKeepsItsWorkWhenTheInvalidationAfterTheCom $this->assertSame(['_index_profile'], $this->indexKeys($database, 'accounts'), 'The two-way index must still be created'); $this->assertTrue($this->hasSchemaIndex($database, 'profiles', '_index_account')); $this->assertTrue($this->hasSchemaIndex($database, 'accounts', '_index_profile')); - $this->assertSame($failures[0] ?? null, $error, 'The failure after the commit must reach the caller as it was raised'); + $this->assertNull($error, 'A stored write must not be reported as failed because the invalidation after its commit failed'); } /** diff --git a/tests/unit/CreateCollectionCleanupTest.php b/tests/unit/CreateCollectionCleanupTest.php index e66535753..2764ec843 100644 --- a/tests/unit/CreateCollectionCleanupTest.php +++ b/tests/unit/CreateCollectionCleanupTest.php @@ -98,10 +98,9 @@ public function commitTransaction(): bool $adapter->afterCommit = null; $cache->failing = false; - $this->assertNotNull($error, 'A failed invalidation after the commit must reach the caller'); + $this->assertNull($error, 'A stored collection must not be reported as failed because the invalidation after its commit failed'); $this->assertTrue($adapter->collectionExists('cleanup', 'logs'), 'A collection whose definition committed must keep its table'); $this->assertSame('logs', $database->getCollection('logs')->getId()); - $this->assertSame($failure, $error, 'The failure after the commit must reach the caller as it was raised'); $database->createDocument('logs', new Document(['message' => 'kept'])); $this->assertSame(1, $database->count('logs')); diff --git a/tests/unit/Documents/CacheUnavailableTest.php b/tests/unit/Documents/CacheUnavailableTest.php index 262bdc0fa..b0bd57f3f 100644 --- a/tests/unit/Documents/CacheUnavailableTest.php +++ b/tests/unit/Documents/CacheUnavailableTest.php @@ -104,6 +104,32 @@ public static function blips(): array ]; } + /** + * A purge that outlasts its retries used to refuse the write. Blocking the collection's epoch before the commit + * keeps the document cached before the write from being served, so the write commits instead. + * + * @param Closure(Database): mixed $write + */ + #[DataProvider('singleWrites')] + public function testAWritePastThePurgeRetriesCommitsBehindATombstone(Closure $write): void + { + $cache = new UnavailableCache(); + $database = $this->createDatabase($cache); + $cache->failNextPurges(3); + + $failure = $this->attempt(fn (): mixed => $write($database)); + + $this->assertNull($failure, 'A write the cache can still block must not be refused'); + $this->assertSame('updated', $this->stored($database)); + $this->assertSame('updated', $this->cached($database), 'The document must not be served from the cache as it was before the write'); + } + + /** @return array */ + public static function singleWrites(): array + { + return \array_diff_key(self::blips(), ['batch update' => true]); + } + public function testAPurgeFailingAfterTheCommitRetiresTheCollectionInstead(): void { $cache = new UnavailableCache(); @@ -111,13 +137,53 @@ public function testAPurgeFailingAfterTheCommitRetiresTheCollectionInstead(): vo $database = $this->createDatabase($cache, $adapter); $adapter->afterCommit = $cache->failWrites(...); - $this->attempt(fn (): Document => $database->updateDocument('webhooks', 'hook', new Document(['name' => 'updated']))); + $failure = $this->attempt(fn (): Document => $database->updateDocument('webhooks', 'hook', new Document(['name' => 'updated']))); $cache->recover(); + $this->assertNull($failure, 'A stored write must not be reported as failed because its cache invalidation failed'); $this->assertSame('updated', $this->stored($database)); $this->assertSame('updated', $this->cached($database), 'A committed write whose purge failed must not leave the old document cached'); } + /** + * Publishing the epoch after a batch write commits can miss a Redis timeout. The write is stored and its + * tombstone stays in place, so it must report success while readers keep bypassing the cache. + */ + public function testACommittedWriteWhoseEpochCannotBePublishedStaysFailClosed(): void + { + $cache = new UnavailableCache(); + $adapter = new CommitHookMemory(); + $database = $this->createDatabase($cache, $adapter); + $adapter->afterCommit = fn () => $cache->failNextSaves(3); + + $failure = $this->attempt(fn (): int => $database->updateDocuments( + 'webhooks', + new Document(['name' => 'updated']), + [Query::equal('$id', ['hook'])], + )); + + $this->assertNull($failure, 'A stored write must not be reported as failed because its epoch could not be published'); + $this->assertSame('updated', $this->stored($database)); + $this->assertSame('updated', $this->cached($database)); + $this->assertSame('updated', $this->cached($database), 'A refill while the tombstone stands must not cache the old document'); + } + + public function testAnEpochPublishBlipAfterTheCommitIsRetried(): void + { + $cache = new UnavailableCache(); + $adapter = new CommitHookMemory(); + $database = $this->createDatabase($cache, $adapter); + $adapter->afterCommit = fn () => $cache->failNextSaves(2); + + $database->updateDocuments('webhooks', new Document(['name' => 'updated']), [Query::equal('$id', ['hook'])]); + + $collectionKey = $database->getCacheBaseKeys('webhooks')[0]; + $epoch = $database->getCache()->load($collectionKey.'#epoch', Database::TTL); + $this->assertIsString($epoch); + $this->assertStringStartsNotWith('blocked', $epoch, 'A blip shorter than the retries must still publish a fresh epoch'); + $this->assertSame('updated', $this->cached($database)); + } + public function testPurgeCachedCollectionReportsACacheThatCannotConfirmIt(): void { $cache = new UnavailableCache(); diff --git a/tests/unit/Documents/DocumentCacheEpochTest.php b/tests/unit/Documents/DocumentCacheEpochTest.php index 1152e0ef8..eb8382051 100644 --- a/tests/unit/Documents/DocumentCacheEpochTest.php +++ b/tests/unit/Documents/DocumentCacheEpochTest.php @@ -98,12 +98,7 @@ public function testActivationFailureAfterCommitLeavesTheEpochBlocked(): void $this->assertSame('original', $database->getDocument('webhooks', 'hook')->getAttribute('name')); $cache->failActivations(); - try { - $this->renameDocument($database, 'webhooks', 'hook', 'updated'); - $this->fail('Document cache activation failure was not propagated'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('activate document cache epoch', $error->getMessage()); - } + $this->assertSame(1, $this->renameDocument($database, 'webhooks', 'hook', 'updated'), 'A stored write must not be reported as failed because its cache activation failed'); $this->assertSame('updated', $database->getDocument('webhooks', 'hook')->getAttribute('name')); UncachedTwin::of($database)->updateDocument('webhooks', 'hook', new Document(['name' => 'changed'])); @@ -134,15 +129,10 @@ public function testActivationFailureDoesNotStrandOtherCollectionEpochs(): void $this->assertSame('original', $database->getDocument('logs', 'log')->getAttribute('name')); $cache->failActivations('collection:webhooks#epoch'); - try { - $database->withTransaction(function () use ($database): void { - $this->renameDocument($database, 'webhooks', 'hook', 'updated'); - $this->renameDocument($database, 'logs', 'log', 'updated'); - }); - $this->fail('Document cache activation failure was not propagated'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('activate document cache epoch', $error->getMessage()); - } + $database->withTransaction(function () use ($database): void { + $this->renameDocument($database, 'webhooks', 'hook', 'updated'); + $this->renameDocument($database, 'logs', 'log', 'updated'); + }); $this->assertSame('updated', $database->getDocument('webhooks', 'hook')->getAttribute('name')); UncachedTwin::of($database)->updateDocument('webhooks', 'hook', new Document(['name' => 'changed'])); @@ -207,7 +197,7 @@ public function testCacheFlushAfterActivationReadsGenerationsDoesNotFailTheCommi $this->assertNull($database->findCollection('webhooks')); } - public function testActivationPurgeFailureStillPropagates(): void + public function testALostFinishAfterCommitDoesNotFailTheWrite(): void { $cache = new FlushDuringActivationMemory(); $database = $this->createDatabaseWithCache($cache); @@ -218,12 +208,7 @@ public function testActivationPurgeFailureStillPropagates(): void $this->assertTrue($cache->flush()); $cache->failDuringActivation(); - try { - $this->renameDocument($database, 'webhooks', 'hook', 'updated'); - $this->fail('Document cache activation purge failure was not propagated'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('finish document cache invalidation', $error->getMessage()); - } + $this->assertSame(1, $this->renameDocument($database, 'webhooks', 'hook', 'updated'), 'A stored write must not be reported as failed because its cache activation failed'); $this->assertSame('updated', $database->getDocument('webhooks', 'hook')->getAttribute('name')); } @@ -292,7 +277,7 @@ public function testOverlappingWritesSucceedOnACacheWithoutFields(): void $this->assertSame('updated', $writer->getDocument('webhooks', 'hook')->getAttribute('name')); } - public function testActivationRejectsACorruptedOwnerRegistration(): void + public function testACorruptedOwnerRegistrationDoesNotFailTheCommittedWrite(): void { $cache = new RedisLeasableCache(); $database = $this->createDatabaseWithCache($cache); @@ -302,12 +287,7 @@ public function testActivationRejectsACorruptedOwnerRegistration(): void ])); $cache->corruptFieldWrites(); - try { - $this->renameDocument($database, 'webhooks', 'hook', 'updated'); - $this->fail('A corrupted document cache owner registration was accepted'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('Invalid document cache owner', $error->getMessage()); - } + $this->assertSame(1, $this->renameDocument($database, 'webhooks', 'hook', 'updated'), 'A stored write must not be reported as failed because its cache activation failed'); $cache->corruptFieldWrites(false); $this->assertSame('updated', $database->getDocument('webhooks', 'hook')->getAttribute('name')); @@ -315,7 +295,7 @@ public function testActivationRejectsACorruptedOwnerRegistration(): void $this->assertSame('changed', $database->getDocument('webhooks', 'hook')->getAttribute('name'), 'A collection whose activation was refused stays uncached'); } - public function testActivationPropagatesAnOwnerReleaseFailure(): void + public function testAnOwnerReleaseFailureDoesNotFailTheCommittedWrite(): void { $cache = new RedisLeasableCache(); $database = $this->createDatabaseWithCache($cache); @@ -325,12 +305,7 @@ public function testActivationPropagatesAnOwnerReleaseFailure(): void ])); $cache->failFieldPurges(); - try { - $this->renameDocument($database, 'webhooks', 'hook', 'updated'); - $this->fail('A document cache owner release failure was not propagated'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('Failed to release document cache owner', $error->getMessage()); - } + $this->assertSame(1, $this->renameDocument($database, 'webhooks', 'hook', 'updated'), 'A stored write must not be reported as failed because its cache activation failed'); $this->assertSame('updated', $database->getDocument('webhooks', 'hook')->getAttribute('name')); UncachedTwin::of($database)->updateDocument('webhooks', 'hook', new Document(['name' => 'changed'])); @@ -427,12 +402,7 @@ public function testALostActivationDoesNotKeepTheCollectionUncached(): void $database = $this->createCountedDatabase($adapter, $cache); $cache->failActivations(); - try { - $this->renameDocument($database, 'webhooks', 'hook', 'updated'); - $this->fail('The activation did not fail'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('activate document cache epoch', $error->getMessage()); - } + $this->renameDocument($database, 'webhooks', 'hook', 'updated'); $this->assertSame('updated', $database->getDocument('webhooks', 'hook')->getAttribute('name')); UncachedTwin::of($database)->updateDocument('webhooks', 'hook', new Document(['name' => 'changed'])); @@ -454,12 +424,7 @@ public function testAWriteAfterAnAbandonedWriteReenablesTheCollection(): void $database = $this->createCountedDatabase($adapter, $cache); $cache->abandonNextWrite(); - try { - $this->renameDocument($database, 'webhooks', 'hook', 'abandoned'); - $this->fail('The abandoned write released its registration'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('release document cache owner', $error->getMessage()); - } + $this->renameDocument($database, 'webhooks', 'hook', 'abandoned'); $this->renameDocument($database, 'webhooks', 'hook', 'second'); $this->assertSame('second', $database->getDocument('webhooks', 'hook')->getAttribute('name')); @@ -484,17 +449,12 @@ public function testAWriterYoungerThanTheTimeoutKeepsItsBlockUntilItsActivation( try { $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); - try { - $writer->withTransaction(function () use ($writer, $reader, $cache): void { - $this->renameDocument($writer, 'users', 'user', 'updated'); - $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); - $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); - $cache->failActivations(); - }); - $this->fail('The activation did not fail'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('activate document cache epoch', $error->getMessage()); - } + $writer->withTransaction(function () use ($writer, $reader, $cache): void { + $this->renameDocument($writer, 'users', 'user', 'updated'); + $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); + $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); + $cache->failActivations(); + }); $this->assertSame('updated', $reader->getDocument('users', 'user')->getAttribute('name'), 'A reader must not cache what it read while a write younger than the writer timeout was in flight'); } finally { @@ -531,18 +491,13 @@ public function testAWriteFinishingWhileAnotherIsInFlightLeavesTheCollectionBloc try { $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); - try { - $writer->withTransaction(function () use ($writer, $reader, $cache): void { - $this->renameDocument($writer, 'users', 'user', 'updated'); - $reader->purgeCachedCollection('users'); - $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); - $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); - $cache->abandonNextWrite(); - }); - $this->fail('The abandoned write released its registration'); - } catch (\RuntimeException $error) { - $this->assertStringContainsString('release document cache owner', $error->getMessage()); - } + $writer->withTransaction(function () use ($writer, $reader, $cache): void { + $this->renameDocument($writer, 'users', 'user', 'updated'); + $reader->purgeCachedCollection('users'); + $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); + $this->assertSame('original', $reader->getDocument('users', 'user')->getAttribute('name')); + $cache->abandonNextWrite(); + }); $this->assertSame('updated', $reader->getDocument('users', 'user')->getAttribute('name'), 'A write that finishes while another is in flight must not re-enable the collection'); } finally { @@ -896,6 +851,8 @@ final class AbandoningCache implements CacheAdapter, Leasable private bool $abandoning = false; + private string $abandoned = ''; + public function __construct() { $this->cache = new RedisLeasableCache(); @@ -934,10 +891,14 @@ public function list(string $key): array #[\Override] public function purge(string $key, string $hash = ''): bool { - if ($this->abandoning && $hash !== '' && \str_ends_with($key, '#owners')) { - $this->abandoning = false; - - return false; + if ($hash !== '' && \str_ends_with($key, '#owners')) { + if ($this->abandoning) { + $this->abandoning = false; + $this->abandoned = $hash; + } + if ($hash === $this->abandoned) { + return false; + } } return $this->cache->purge($key, $hash); diff --git a/tests/unit/Documents/DocumentCacheInvalidationTest.php b/tests/unit/Documents/DocumentCacheInvalidationTest.php index 771f81fd9..edc4aa627 100644 --- a/tests/unit/Documents/DocumentCacheInvalidationTest.php +++ b/tests/unit/Documents/DocumentCacheInvalidationTest.php @@ -243,7 +243,7 @@ public function testAFailedPostCommitPurgeNeverServesAPreCommitFill(): void $failure = $error->getMessage(); } - $this->assertSame(self::PURGE_FAILURE, $failure, 'A failed purge after the commit must reach the caller'); + $this->assertNull($failure, 'A stored write must not be reported as failed because its purge after the commit failed'); $this->assertSame('hook', $duringCommit, 'A reader outside the transaction reads the committed row and caches it'); $this->assertSame('renamed', $reader->getDocument('webhooks', 'hook')->getAttribute('name'), 'A fill made before the commit must never be served after it, even when the purge after the commit fails'); diff --git a/tests/unit/Event/DocumentPurgeTest.php b/tests/unit/Event/DocumentPurgeTest.php index 3e633cb51..031dcd491 100644 --- a/tests/unit/Event/DocumentPurgeTest.php +++ b/tests/unit/Event/DocumentPurgeTest.php @@ -609,7 +609,7 @@ public function commitTransaction(): bool $cache->failing = true; }; - $this->assertSame($failure, $this->failureOf(static fn (): mixed => $write($database))); + $this->assertNull($this->failureOf(static fn (): mixed => $write($database)), 'A stored write must not be reported as failed because the invalidation after its commit failed'); $this->assertSame(['posts/first'], $this->purged($recorder)); }