Clo 4084 fix disable rate limiting on cache failure 02#846
Open
davidjeddy wants to merge 6 commits intomainfrom
Open
Clo 4084 fix disable rate limiting on cache failure 02#846davidjeddy wants to merge 6 commits intomainfrom
davidjeddy wants to merge 6 commits intomainfrom
Conversation
…in a try/catch that logs a error but does not terminate execution.
Contributor
📝 WalkthroughWalkthroughImplements fail‑open cache handling: adds cache read/purge failure events, moves cache purging out of DB transactions for update/delete, wraps cache operations and event listener invocations in try/catch, introduces a retry-or-warn helper, and documents these changes in changelog/README and E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant DB as Database
participant Cache as Cache
participant Events as EventSystem
Note over App,Events: Normal update flow (cache healthy)
App->>DB: updateDocument()
DB->>DB: Transaction begin
DB->>DB: Update row
DB->>DB: Transaction commit
DB->>Cache: purgeCachedDocument()
Cache-->>DB: Success
DB->>Events: Emit EVENT_DOCUMENT_UPDATE
Events-->>App: Operation complete
Note over App,Events: Fail‑open update flow (cache broken)
App->>DB: updateDocument()
DB->>DB: Transaction begin
DB->>DB: Update row
DB->>DB: Transaction commit
DB->>Cache: purgeCachedDocument()
Cache--xDB: throws/throws Throwable
DB->>DB: catch Throwable
DB->>Events: Emit EVENT_CACHE_PURGE_FAILURE
Events->>DB: listener throws
DB->>DB: catch & log listener error
DB-->>App: Operation complete (DB persisted)
sequenceDiagram
actor App as Application
participant DB as Database
participant Cache as Cache
participant Retry as RetryHelper
participant Events as EventSystem
App->>DB: getDocument(id)
DB->>Cache: load(id)
alt Cache load succeeds
Cache-->>DB: Document
DB-->>App: Document
else Cache load throws
Cache--xDB: throws/throws Throwable
DB->>Retry: withRetriesOrWarn(load operation)
Retry->>Cache: retry load...
Retry-->>DB: final failure
DB->>DB: log warning
DB->>Events: Emit EVENT_CACHE_READ_FAILURE
Events->>DB: listener throws?
DB->>DB: catch & log listener error
DB->>DB: Read from DB fallback
DB-->>App: Document (from DB)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR implements a fail-open cache pattern for Key observations:
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4884
to
4891
| } catch (\Throwable $e) { | ||
| Console::warning('Failed to save document to cache: ' . $e->getMessage()); | ||
| try { | ||
| $this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e); | ||
| } catch (\Throwable $innerException) { | ||
| Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
EVENT_CACHE_PURGE_FAILURE misused for a cache write (save) failure
A cache save() failure is semantically distinct from a cache purge() failure, but both are mapped to EVENT_CACHE_PURGE_FAILURE. A developer who registers a listener for EVENT_CACHE_PURGE_FAILURE to react to eviction failures would unexpectedly receive events triggered by a failed cache-write during a read operation (getDocument's repopulation path).
This naming mismatch also causes the test testCacheReadFailureEmitsReadFailureEvent to be unpassable (see related test comment), since with a fully-broken cache the save path fires EVENT_CACHE_PURGE_FAILURE which the test asserts will not be fired.
Consider introducing a separate EVENT_CACHE_WRITE_FAILURE constant (or reusing EVENT_CACHE_READ_FAILURE) for the save-on-read failure path to give callers precise, actionable signals.
// Current (misleading):
} catch (\Throwable $e) {
Console::warning('Failed to save document to cache: ' . $e->getMessage());
try {
$this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e);
} catch (\Throwable $innerException) {
Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage());
}
}
// Suggested fix - use (or add) a write-failure event:
} catch (\Throwable $e) {
Console::warning('Failed to save document to cache: ' . $e->getMessage());
try {
$this->trigger(self::EVENT_CACHE_WRITE_FAILURE, $e);
} catch (\Throwable $innerException) {
Console::error('Cache unavailable: EVENT_CACHE_WRITE_FAILURE listener threw: ' . $innerException->getMessage());
}
}
Comment on lines
+8339
to
+8356
| try { | ||
| $result = $this->purgeCachedDocumentInternal($collectionId, $id); | ||
| } catch (\Throwable $e) { | ||
| Console::warning('Failed to purge document cache: ' . $e->getMessage()); | ||
| try { | ||
| $this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e); | ||
| } catch (\Throwable $innerException) { | ||
| Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage()); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| if ($id !== null) { | ||
| $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ | ||
| '$id' => $id, | ||
| '$collection' => $collectionId | ||
| ])); | ||
| try { | ||
| $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ | ||
| '$id' => $id, | ||
| '$collection' => $collectionId | ||
| ])); |
There was a problem hiding this comment.
catch block can never be reached
purgeCachedDocumentInternal now catches all \Throwable internally (both cache->purge() calls are wrapped) and never throws. As a result, the outer try/catch that wraps its call here will never execute. The EVENT_CACHE_PURGE_FAILURE trigger and the return false path are unreachable dead code.
The @throws Exception PHPDoc on purgeCachedDocumentInternal should also be removed, and this outer catch block can be simplified to a straight call:
$result = $this->purgeCachedDocumentInternal($collectionId, $id);(The duplicate event emission that would have occurred if both the inner and the outer catches fired is also avoided this way.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
2243-2253:⚠️ Potential issue | 🟠 MajorDon't emit
EVENT_DOCUMENT_PURGEon failed invalidations.
purgeCachedDocumentInternal()now reports failures viafalse, butwithRetriesOrWarn()only retries thrown exceptions and these paths still dispatch the success event unconditionally. On a broken cache backend, listeners will observe a purge that never happened.Suggested fix
-private function withRetriesOrWarn(callable $operation): void +private function withRetriesOrWarn(callable $operation): bool { try { - $this->withRetries($operation); + $this->withRetries(function () use ($operation) { + if ($operation() === false) { + throw new \RuntimeException('Cache invalidation did not complete'); + } + }); + return true; } catch (\Throwable $e) { Console::warning('Failed to purge cache after retries: ' . $e->getMessage()); try { $this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e); } catch (\Throwable $innerException) { Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage()); } + return false; } }- if ($id !== null) { + if ($result && $id !== null) { try { $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ '$id' => $id, '$collection' => $collectionId ])); @@ - $this->withRetriesOrWarn(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); - try { - $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ - '$id' => $collection->getId(), - '$collection' => self::METADATA - ])); - } catch (\Throwable $e) { - // Ignore - } + if ($this->withRetriesOrWarn(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId()))) { + try { + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $collection->getId(), + '$collection' => self::METADATA + ])); + } catch (\Throwable $innerException) { + Console::error('Cache unavailable: EVENT_DOCUMENT_PURGE listener threw: ' . $innerException->getMessage()); + } + }Also applies to: 2447-2457, 3164-3175, 3306-3317, 8297-8362, 10160-10172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Database.php` around lines 2243 - 2253, The code currently calls withRetriesOrWarn(fn => purgeCachedCollection(...)) and withRetriesOrWarn(fn => purgeCachedDocumentInternal(...)) but always emits EVENT_DOCUMENT_PURGE regardless of whether purgeCachedDocumentInternal returned false (it only reports failure via false, not exception). Change these sites (e.g., the block using purgeCachedCollection and purgeCachedDocumentInternal that then calls trigger(self::EVENT_DOCUMENT_PURGE, ...)) to capture the boolean return values from withRetriesOrWarn for both purgeCachedCollection and purgeCachedDocumentInternal, and only call $this->trigger(self::EVENT_DOCUMENT_PURGE, ...) when the relevant invalidation(s) returned true (i.e., the purge actually succeeded); do this for all similar occurrences (also at the other listed blocks) so listeners are only notified on successful invalidations.
🧹 Nitpick comments (1)
README.md (1)
901-917: Use a single Docker command style in testing docs.Line 901/Line 917 use
docker-compose, while Line 905/Line 909/Line 913 usedocker compose. Standardize to one style to avoid setup friction for contributors.Suggested doc tweak
-`docker-compose up --detach` +`docker compose up --detach` ... -`docker-compose down` +`docker compose down`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 901 - 917, The README contains mixed Docker command styles (`docker-compose up --detach` and `docker-compose down` vs `docker compose exec ...`) which should be standardized; update all occurrences to use a single style (choose either the modern `docker compose` or the legacy `docker-compose`) across the E2E, Resources, Unit, and Teardown sections so the commands `docker compose exec tests vendor/bin/phpunit /usr/src/code/tests/e2e`, `docker compose exec tests vendor/bin/phpunit /usr/src/code/tests/resources`, `docker compose exec tests vendor/bin/phpunit /usr/src/code/tests/unit`, and the up/down commands are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Database.php`:
- Around line 8252-8275: If any purge of individual document keys ($documentKeys
/ $documentKey via $this->cache->purge) fails, treat that as a collection-level
failure: stop the loop, do not proceed to purge $collectionKey, trigger
self::EVENT_CACHE_PURGE_FAILURE with the original exception (using
$this->trigger) and return false; ensure existing catch blocks still log via
Console::warning and Console::error when listener throws (preserve those
messages) but after catching a Throwable from a document purge you must abort
further purges and return false so the later $this->cache->purge($collectionKey)
and a true result are not executed.
In `@tests/e2e/Adapter/MariaDBTest.php`:
- Around line 156-162: After creating the document with
$database->createDocument(...) you must restore the real working cache and
re-query the database to ensure the insert persisted: restore the cache
implementation used elsewhere in the test, call the retrieval method (e.g.
$database->getDocument($collection, 'new') or the project's equivalent) to fetch
the document with ID 'new', then assert the fetched Document is not empty and
that getAttribute('title') equals 'created' (mirroring the update/delete
persistence checks). Ensure you reference the same ID used in creation
(ID::custom('new')) and use the same $collection and $database variables.
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 679-692: The final check only ensures the updated value is gone
and can miss that the write/delete never committed; after restarting Redis use
the existing waitForRedis() helper to wait for readiness, clear the document
cache (purge the in-memory/document cache used by the test harness), then call
getDocument('testRedisFallback', 'doc1') and assert ->isEmpty() to verify the DB
row was actually removed/updated on the persistent store rather than just
missing from cache; use the same identifiers updateDocument, deleteDocument,
getDocument, waitForRedis and the test's cache-purging utility to implement this
sequence.
---
Outside diff comments:
In `@src/Database/Database.php`:
- Around line 2243-2253: The code currently calls withRetriesOrWarn(fn =>
purgeCachedCollection(...)) and withRetriesOrWarn(fn =>
purgeCachedDocumentInternal(...)) but always emits EVENT_DOCUMENT_PURGE
regardless of whether purgeCachedDocumentInternal returned false (it only
reports failure via false, not exception). Change these sites (e.g., the block
using purgeCachedCollection and purgeCachedDocumentInternal that then calls
trigger(self::EVENT_DOCUMENT_PURGE, ...)) to capture the boolean return values
from withRetriesOrWarn for both purgeCachedCollection and
purgeCachedDocumentInternal, and only call
$this->trigger(self::EVENT_DOCUMENT_PURGE, ...) when the relevant
invalidation(s) returned true (i.e., the purge actually succeeded); do this for
all similar occurrences (also at the other listed blocks) so listeners are only
notified on successful invalidations.
---
Nitpick comments:
In `@README.md`:
- Around line 901-917: The README contains mixed Docker command styles
(`docker-compose up --detach` and `docker-compose down` vs `docker compose exec
...`) which should be standardized; update all occurrences to use a single style
(choose either the modern `docker compose` or the legacy `docker-compose`)
across the E2E, Resources, Unit, and Teardown sections so the commands `docker
compose exec tests vendor/bin/phpunit /usr/src/code/tests/e2e`, `docker compose
exec tests vendor/bin/phpunit /usr/src/code/tests/resources`, `docker compose
exec tests vendor/bin/phpunit /usr/src/code/tests/unit`, and the up/down
commands are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b2c83e0-372c-4588-90b5-42e3167f6a01
⛔ Files ignored due to path filters (1)
dev/xdebug.iniis excluded by!dev/**
📒 Files selected for processing (6)
CHANGES.mdREADME.mdbin/cli.phpsrc/Database/Database.phptests/e2e/Adapter/MariaDBTest.phptests/e2e/Adapter/Scopes/GeneralTests.php
Comment on lines
8252
to
+8275
| foreach ($documentKeys as $documentKey) { | ||
| $this->cache->purge($documentKey); | ||
| try { | ||
| $this->cache->purge($documentKey); | ||
| } catch (\Throwable $e) { | ||
| Console::warning('Failed to purge document cache key: ' . $e->getMessage()); | ||
| try { | ||
| $this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e); | ||
| } catch (\Throwable $innerException) { | ||
| Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $this->cache->purge($collectionKey); | ||
| try { | ||
| $this->cache->purge($collectionKey); | ||
| } catch (\Throwable $e) { | ||
| Console::warning('Failed to purge collection cache key: ' . $e->getMessage()); | ||
| try { | ||
| $this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e); | ||
| } catch (\Throwable $innerException) { | ||
| Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage()); | ||
| } | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Treat any child-key purge failure as a collection purge failure.
Line 8253 logs and continues when a $documentKey purge fails, but Line 8266 still removes $collectionKey and the method returns true. That can leave stale document entries reachable by direct cache lookup while deleting the only index later collection purges can use to find them.
Suggested fix
public function purgeCachedCollection(string $collectionId): bool
{
[$collectionKey] = $this->getCacheKeys($collectionId);
+ $success = true;
try {
$documentKeys = $this->cache->list($collectionKey);
} catch (\Throwable $e) {
Console::warning('Failed to list collection cache keys: ' . $e->getMessage());
@@
foreach ($documentKeys as $documentKey) {
try {
$this->cache->purge($documentKey);
} catch (\Throwable $e) {
Console::warning('Failed to purge document cache key: ' . $e->getMessage());
try {
$this->trigger(self::EVENT_CACHE_PURGE_FAILURE, $e);
} catch (\Throwable $innerException) {
Console::error('Cache unavailable: EVENT_CACHE_PURGE_FAILURE listener threw: ' . $innerException->getMessage());
}
+ $success = false;
}
}
+
+ if (!$success) {
+ return false;
+ }
try {
$this->cache->purge($collectionKey);
} catch (\Throwable $e) {
Console::warning('Failed to purge collection cache key: ' . $e->getMessage());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Database.php` around lines 8252 - 8275, If any purge of
individual document keys ($documentKeys / $documentKey via $this->cache->purge)
fails, treat that as a collection-level failure: stop the loop, do not proceed
to purge $collectionKey, trigger self::EVENT_CACHE_PURGE_FAILURE with the
original exception (using $this->trigger) and return false; ensure existing
catch blocks still log via Console::warning and Console::error when listener
throws (preserve those messages) but after catching a Throwable from a document
purge you must abort further purges and return false so the later
$this->cache->purge($collectionKey) and a true result are not executed.
Comment on lines
+156
to
+162
| // createDocument must persist to the database even if the cache save fails. | ||
| $doc = $database->createDocument($collection, new Document([ | ||
| '$id' => ID::custom('new'), | ||
| 'title' => 'created', | ||
| ])); | ||
| $this->assertFalse($doc->isEmpty()); | ||
| $this->assertEquals('created', $doc->getAttribute('title')); |
Contributor
There was a problem hiding this comment.
Re-read the new document after restoring the real cache.
Right now this only proves createDocument() returned a Document. If the insert is still rolled back on a cache write failure, the test would pass anyway. Mirror the update/delete persistence checks by restoring the working cache and fetching new back from the database.
🧪 Suggested tightening
$doc = $database->createDocument($collection, new Document([
'$id' => ID::custom('new'),
'title' => 'created',
]));
$this->assertFalse($doc->isEmpty());
$this->assertEquals('created', $doc->getAttribute('title'));
+
+ $database->setCache($originalCache);
+ $persisted = $database->getDocument($collection, 'new');
+ $this->assertFalse($persisted->isEmpty());
+ $this->assertEquals('created', $persisted->getAttribute('title'));📝 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
| // createDocument must persist to the database even if the cache save fails. | |
| $doc = $database->createDocument($collection, new Document([ | |
| '$id' => ID::custom('new'), | |
| 'title' => 'created', | |
| ])); | |
| $this->assertFalse($doc->isEmpty()); | |
| $this->assertEquals('created', $doc->getAttribute('title')); | |
| // createDocument must persist to the database even if the cache save fails. | |
| $doc = $database->createDocument($collection, new Document([ | |
| '$id' => ID::custom('new'), | |
| 'title' => 'created', | |
| ])); | |
| $this->assertFalse($doc->isEmpty()); | |
| $this->assertEquals('created', $doc->getAttribute('title')); | |
| $database->setCache($originalCache); | |
| $persisted = $database->getDocument($collection, 'new'); | |
| $this->assertFalse($persisted->isEmpty()); | |
| $this->assertEquals('created', $persisted->getAttribute('title')); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/MariaDBTest.php` around lines 156 - 162, After creating the
document with $database->createDocument(...) you must restore the real working
cache and re-query the database to ensure the insert persisted: restore the
cache implementation used elsewhere in the test, call the retrieval method (e.g.
$database->getDocument($collection, 'new') or the project's equivalent) to fetch
the document with ID 'new', then assert the fetched Document is not empty and
that getAttribute('title') equals 'created' (mirroring the update/delete
persistence checks). Ensure you reference the same ID used in creation
(ID::custom('new')) and use the same $collection and $database variables.
Comment on lines
+679
to
+692
| // Writes must succeed even when the cache is unavailable (fail-open) | ||
| $updated = $database->updateDocument('testRedisFallback', 'doc1', new Document([ | ||
| 'string' => 'text📝 updated', | ||
| ])); | ||
| $this->assertEquals('text📝 updated', $updated->getAttribute('string')); | ||
|
|
||
| try { | ||
| $database->deleteDocument('testRedisFallback', 'doc1'); | ||
| $this->fail('Failed to throw exception'); | ||
| } catch (\Throwable $e) { | ||
| $this->assertEquals('Redis server redis:6379 went away', $e->getMessage()); | ||
| } | ||
| $deleted = $database->deleteDocument('testRedisFallback', 'doc1'); | ||
| $this->assertTrue($deleted); | ||
|
|
||
| // Bring backup Redis | ||
| Console::execute('docker ps -a --filter "name=utopia-redis" --format "{{.Names}}" | xargs -r docker start', "", $stdout, $stderr); | ||
| sleep(5); | ||
|
|
||
| $this->assertCount(1, $database->find('testRedisFallback', [Query::equal('string', ['text📝'])])); | ||
| $this->assertCount(0, $database->find('testRedisFallback', [Query::equal('string', ['text📝 updated'])])); |
Contributor
There was a problem hiding this comment.
The post-restart check can still pass when the write/delete never committed.
assertCount(0, ... 'text📝 updated') only proves that value is gone. It still passes if the row survives with the original 'text📝'. Since this trait already has waitForRedis(), wait for readiness, purge the document cache, and assert getDocument('testRedisFallback', 'doc1')->isEmpty() so the final DB state is actually verified.
🧪 Suggested tightening
- // Bring backup Redis
+ // Bring back Redis
Console::execute('docker ps -a --filter "name=utopia-redis" --format "{{.Names}}" | xargs -r docker start', "", $stdout, $stderr);
- sleep(5);
-
- $this->assertCount(0, $database->find('testRedisFallback', [Query::equal('string', ['text📝 updated'])]));
+ $this->waitForRedis();
+ $database->purgeCachedDocument('testRedisFallback', 'doc1');
+ $this->assertTrue($database->getDocument('testRedisFallback', 'doc1')->isEmpty());📝 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
| // Writes must succeed even when the cache is unavailable (fail-open) | |
| $updated = $database->updateDocument('testRedisFallback', 'doc1', new Document([ | |
| 'string' => 'text📝 updated', | |
| ])); | |
| $this->assertEquals('text📝 updated', $updated->getAttribute('string')); | |
| try { | |
| $database->deleteDocument('testRedisFallback', 'doc1'); | |
| $this->fail('Failed to throw exception'); | |
| } catch (\Throwable $e) { | |
| $this->assertEquals('Redis server redis:6379 went away', $e->getMessage()); | |
| } | |
| $deleted = $database->deleteDocument('testRedisFallback', 'doc1'); | |
| $this->assertTrue($deleted); | |
| // Bring backup Redis | |
| Console::execute('docker ps -a --filter "name=utopia-redis" --format "{{.Names}}" | xargs -r docker start', "", $stdout, $stderr); | |
| sleep(5); | |
| $this->assertCount(1, $database->find('testRedisFallback', [Query::equal('string', ['text📝'])])); | |
| $this->assertCount(0, $database->find('testRedisFallback', [Query::equal('string', ['text📝 updated'])])); | |
| // Writes must succeed even when the cache is unavailable (fail-open) | |
| $updated = $database->updateDocument('testRedisFallback', 'doc1', new Document([ | |
| 'string' => 'text📝 updated', | |
| ])); | |
| $this->assertEquals('text📝 updated', $updated->getAttribute('string')); | |
| $deleted = $database->deleteDocument('testRedisFallback', 'doc1'); | |
| $this->assertTrue($deleted); | |
| // Bring back Redis | |
| Console::execute('docker ps -a --filter "name=utopia-redis" --format "{{.Names}}" | xargs -r docker start', "", $stdout, $stderr); | |
| $this->waitForRedis(); | |
| $database->purgeCachedDocument('testRedisFallback', 'doc1'); | |
| $this->assertTrue($database->getDocument('testRedisFallback', 'doc1')->isEmpty()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Adapter/Scopes/GeneralTests.php` around lines 679 - 692, The final
check only ensures the updated value is gone and can miss that the write/delete
never committed; after restarting Redis use the existing waitForRedis() helper
to wait for readiness, clear the document cache (purge the in-memory/document
cache used by the test harness), then call getDocument('testRedisFallback',
'doc1') and assert ->isEmpty() to verify the DB row was actually removed/updated
on the persistent store rather than just missing from cache; use the same
identifiers updateDocument, deleteDocument, getDocument, waitForRedis and the
test's cache-purging utility to implement this sequence.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/Adapter/MariaDBTest.php (1)
169-188: Consider adding persistence verification or noting coverage split.This test verifies no exception is thrown during
updateDocumentwith a broken cache, but doesn't verify the data was actually persisted. WhiletestUpdateDocumentPersistsDespiteBrokenCachePurgecovers persistence verification separately, it may be cleaner to either consolidate the tests or add a brief comment explaining the coverage split.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/MariaDBTest.php` around lines 169 - 188, The test testCacheFailOpenOnUpdate currently only asserts no exception from Database::updateDocument when cache is broken but doesn't verify persistence; either add a follow-up persistence check by retrieving the document (e.g., call Database::getDocument or readDocument for the same collection and id 'seed') and assert the title is 'updated', or add a short inline comment referencing the separate test testUpdateDocumentPersistsDespiteBrokenCachePurge that covers persistence to make the coverage intent explicit; update the test body around updateDocument and the finally block to include the retrieval/assertion or the clarifying comment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/Adapter/MariaDBTest.php`:
- Around line 169-188: The test testCacheFailOpenOnUpdate currently only asserts
no exception from Database::updateDocument when cache is broken but doesn't
verify persistence; either add a follow-up persistence check by retrieving the
document (e.g., call Database::getDocument or readDocument for the same
collection and id 'seed') and assert the title is 'updated', or add a short
inline comment referencing the separate test
testUpdateDocumentPersistsDespiteBrokenCachePurge that covers persistence to
make the coverage intent explicit; update the test body around updateDocument
and the finally block to include the retrieval/assertion or the clarifying
comment accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3e29782-472c-4fe9-b98e-12b99e8a4fbc
📒 Files selected for processing (2)
CHANGES.mdtests/e2e/Adapter/MariaDBTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
All notable changes to this project will be documented in this file.
The format is based on Keep a Changelog,
and this project adheres to Semantic Versioning.
Unreleased
6.0.0 - 2026-03-25
Added
Database::EVENT_CACHE_READ_FAILUREevent constant, emitted when a cache load fails during a document read. Previously these failures incorrectly emittedEVENT_CACHE_PURGE_FAILURE, making it impossible to distinguish a read-side cache miss from a write-side purge failure.Changed
updateDocumentis now performed outside the database transaction. Previously a cache failure inside the transaction would roll back the committed write; now the DB write is always committed first and the cache is invalidated afterward (half-open / fail-open pattern).deleteDocumentfollows the same transactional ordering fix: the row is deleted inside the transaction and the cache entry is evicted only after the transaction commits.EVENT_CACHE_PURGE_FAILURE,EVENT_CACHE_READ_FAILURE,EVENT_DOCUMENT_PURGE) are now wrapped in an innertry/catch. A listener that throws no longer propagates the exception up to the caller — the error is logged viaConsole::errorand execution continues.Fixed
getDocumentno longer emitsEVENT_CACHE_PURGE_FAILUREwhen the cache is unavailable for a read. It now correctly emitsEVENT_CACHE_READ_FAILURE, so callers that distinguish the two events receive the right signal.updateDocumentordeleteDocumentto surface an exception to the caller. Both operations are now fully fail-open with respect to the cache layer.EVENT_CACHE_PURGE_FAILUREorEVENT_CACHE_READ_FAILURElistener can no longer abort an in-progress database operation.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests