Cache empty getDocument results#924
Conversation
📝 WalkthroughWalkthrough
ChangesNegative cache lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Cache
Client->>Database: Read missing document
Database->>Cache: Save empty marker
Cache-->>Database: Empty marker stored
Client->>Database: Create document
Database->>Cache: Purge document marker
Cache-->>Database: Marker invalidated
Client->>Database: Read created document
Database-->>Client: Return document
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 caches missing document lookups to avoid repeated adapter reads. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "run tests" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Database/Database.php`:
- Around line 8460-8483: Update purgeCreatedDocumentCache() to wrap both the
tenant-scoped withTenant() path and direct purgeCachedDocumentInternal() call in
exception handling, logging cache purge failures with Console::warning while
allowing the committed createDocument/createDocuments operation to succeed. Keep
the existing tenant resolution and purge behavior unchanged; do not refactor the
duplicated tenant wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 231a6f04-a567-4008-9e9e-2e0a5df8d4b9
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
src/Database/Database.phptests/e2e/Adapter/Scopes/DocumentTests.php
| /** | ||
| * Invalidate any cache entry for a freshly created document. | ||
| * | ||
| * A read of a not-yet-created id caches a negative ("not found") marker; | ||
| * once the id is inserted that marker must be cleared so the document | ||
| * becomes visible immediately. Resolves the correct tenant first when | ||
| * tenant-per-document is enabled, so the purge targets the right key. | ||
| * | ||
| * @param Document $collection | ||
| * @param Document $document | ||
| * @return void | ||
| * @throws Exception | ||
| */ | ||
| private function purgeCreatedDocumentCache(Document $collection, Document $document): void | ||
| { | ||
| if ($this->getSharedTables() && $this->getTenantPerDocument()) { | ||
| $this->withTenant($document->getTenant(), function () use ($collection, $document) { | ||
| $this->purgeCachedDocumentInternal($collection->getId(), $document->getId()); | ||
| }); | ||
| } else { | ||
| $this->purgeCachedDocumentInternal($collection->getId(), $document->getId()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Cache purge failure can fail a successful createDocument()/createDocuments().
purgeCreatedDocumentCache() calls purgeCachedDocumentInternal() (via withTenant() in tenant-per-document mode) with no exception handling anywhere in the chain. If the cache backend throws, this propagates straight out of createDocument() (Line 5726-5729) and createDocuments() (Line 5856-5858) after the row is already committed — a durable write gets reported as a failure, and in the batch path the exception aborts all remaining documents in the chunk without going through the onError callback.
This same PR already establishes the correct pattern for cache-consistency operations a few lines earlier: every cache call inside getDocument()'s negative-cache write path (Line 4935-4946) is wrapped in try/catch with a Console::warning, precisely because a best-effort cache operation must never fail the primary database operation.
🐛 Proposed fix: guard the purge like the read-path already does
private function purgeCreatedDocumentCache(Document $collection, Document $document): void
{
try {
if ($this->getSharedTables() && $this->getTenantPerDocument()) {
$this->withTenant($document->getTenant(), function () use ($collection, $document) {
$this->purgeCachedDocumentInternal($collection->getId(), $document->getId());
});
} else {
$this->purgeCachedDocumentInternal($collection->getId(), $document->getId());
}
} catch (Exception $e) {
Console::warning('Failed to purge empty document cache: ' . $e->getMessage());
}
}Separately (optional): this tenant-wrapping shape is now duplicated a third time (also in upsertDocumentsWithIncrease and deleteDocuments); a small shared helper (e.g. withDocumentTenant(Document $doc, callable $cb)) would remove the repetition, but that's a nice-to-have, not blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Database/Database.php` around lines 8460 - 8483, Update
purgeCreatedDocumentCache() to wrap both the tenant-scoped withTenant() path and
direct purgeCachedDocumentInternal() call in exception handling, logging cache
purge failures with Console::warning while allowing the committed
createDocument/createDocuments operation to succeed. Keep the existing tenant
resolution and purge behavior unchanged; do not refactor the duplicated tenant
wrapper.
1 participant
Summary by CodeRabbit
Performance
Bug Fixes
Tests