Reject stale cached snapshot after write#902
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFixes a cache coherency race in ChangesCache version marker for stale snapshot rejection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 Summary
Confidence Score: 4/5Merge safety is limited by several write paths that still purge cached documents without recording the marker needed to reject stale snapshots. The changed cache guard is well scoped, and focused runtime checks reproduced the marker gap for counter writes, bulk deletes, and upserts; other analogous paths remain code-supported but were not executable in this environment. src/Database/Database.php needs attention around bulk update/delete, counter, upsert, delete marker timestamp, and ID rename cache-marker handling.
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $this->purgeCachedDocumentInternal($collection->getId(), $id); | ||
| // Advance the version marker past any snapshot so a stale "exists" copy | ||
| // re-cached by a racing reader is rejected on subsequent reads. | ||
| $this->recordCachedDocumentVersion($collection->getId(), $id, DateTime::now()); |
There was a problem hiding this comment.
The delete path records DateTime::now() as the marker. For documents whose $updatedAt was preserved as a future timestamp, a stale pre-delete snapshot can have an $updatedAt greater than this marker. Because the read guard only rejects cached versions that are strictly older than the marker, that stale existing document can survive a delete and be served from cache.
| $this->purgeCachedDocumentInternal($collection->getId(), $id); | ||
| // Record the committed version so a stale snapshot re-cached by a racing | ||
| // reader is rejected on subsequent reads (see getDocument). | ||
| $this->recordCachedDocumentVersion($collection->getId(), $id, $document->getUpdatedAt()); |
There was a problem hiding this comment.
When updateDocument() changes a document ID, the transaction purges both the old and new keys, but the new post-commit purge and marker are only written for the old $id. The post-commit marker is the part that protects against a reader re-caching around the commit window, so a stale entry under the new ID can still be accepted by getDocument().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 6415-6417: The recordCachedDocumentVersion() call that prevents
stale cached snapshot resurrection is only applied in updateDocument() and
deleteDocument(), but the same race condition exists in updateDocuments(),
upsertDocuments(), increaseDocumentAttribute(), decreaseDocumentAttribute(), and
deleteDocuments(). Add a recordCachedDocumentVersion() call after each
successful document purge in all these methods to record the version marker
consistently across all document write paths, ensuring stale cached snapshots
are rejected regardless of which write method is used.
- Around line 4873-4876: The cache rejection logic in the comparison at
cacheVersionStamp($cached['$updatedAt']) only checks if cachedVersion is
strictly less than marker, but this fails when update writes preserve $updatedAt
(creating equality) or when deletes use DateTime::now() which can be older than
preserved values, allowing stale snapshots to remain cached. Replace the loose
comparison with a strictly monotonic cache version token that guarantees the
marker is always strictly newer than any pre-write snapshot, ensuring that the
condition properly rejects all stale cached data. This comparison issue exists
in multiple locations (also at lines 6415-6417 and 7807-7809) and should be
consistently addressed across all occurrences.
🪄 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: 67999732-e315-445e-8fa0-5c31f2ac084d
📒 Files selected for processing (3)
PR_BODY.mdsrc/Database/Database.phptests/unit/ForUpdateCacheTest.php
| if (\is_string($marker) && $marker !== '') { | ||
| $cachedVersion = $this->cacheVersionStamp($cached['$updatedAt']); | ||
| if ($cachedVersion !== null && (float) $cachedVersion < (float) $marker) { | ||
| $cached = null; |
There was a problem hiding this comment.
Make the cache marker strictly dominate stale snapshots.
The guard only rejects cachedVersion < marker, but update writes can preserve/tie $updatedAt, and deletes use DateTime::now() which can be older than a preserved/future $updatedAt. In those cases, a stale re-cached snapshot is not rejected. Use a truly monotonic cache-only version token, or otherwise guarantee the marker is strictly newer than any pre-write snapshot before relying on this comparison.
Also applies to: 6415-6417, 7807-7809
🤖 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 4873 - 4876, The cache rejection
logic in the comparison at cacheVersionStamp($cached['$updatedAt']) only checks
if cachedVersion is strictly less than marker, but this fails when update writes
preserve $updatedAt (creating equality) or when deletes use DateTime::now()
which can be older than preserved values, allowing stale snapshots to remain
cached. Replace the loose comparison with a strictly monotonic cache version
token that guarantees the marker is always strictly newer than any pre-write
snapshot, ensuring that the condition properly rejects all stale cached data.
This comparison issue exists in multiple locations (also at lines 6415-6417 and
7807-7809) and should be consistently addressed across all occurrences.
| // Record the committed version so a stale snapshot re-cached by a racing | ||
| // reader is rejected on subsequent reads (see getDocument). | ||
| $this->recordCachedDocumentVersion($collection->getId(), $id, $document->getUpdatedAt()); |
There was a problem hiding this comment.
Apply the version marker to every document write path.
This only covers updateDocument() and deleteDocument(). The same purge-only race remains in updateDocuments(), upsertDocuments(), increaseDocumentAttribute(), decreaseDocumentAttribute(), and deleteDocuments(), so those writes can still resurrect stale cached snapshots after their purge. Record the marker next to each successful post-commit document purge. This follows the PR objective to reject stale cached snapshots after writes.
Also applies to: 7807-7809
🤖 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 6415 - 6417, The
recordCachedDocumentVersion() call that prevents stale cached snapshot
resurrection is only applied in updateDocument() and deleteDocument(), but the
same race condition exists in updateDocuments(), upsertDocuments(),
increaseDocumentAttribute(), decreaseDocumentAttribute(), and deleteDocuments().
Add a recordCachedDocumentVersion() call after each successful document purge in
all these methods to record the version marker consistently across all document
write paths, ensuring stale cached snapshots are rejected regardless of which
write method is used.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests