fix: purge collection cache after commit in createRelationship#906
fix: purge collection cache after commit in createRelationship#906premtsd-code wants to merge 1 commit into
Conversation
The two updateDocument() metadata writes run inside createRelationship's outer withTransaction(). Because the adapter nests transactions with a counter and only commits at the outermost level, each updateDocument()'s own post-commit purge fires while the outer transaction is still open (pre-commit). A concurrent reader can then re-cache the pre-commit collection, missing the new relationship column, and the subsequent createIndex() re-read validates against that stale copy and fails with 'Invalid index attribute "..." not found' (flaky, shared-tables/concurrent only). Purge the collection caches again after the outer commit, before creating the relationship indexes, so the re-read sees the committed schema.
📝 WalkthroughWalkthroughIn ChangesPost-commit cache invalidation in
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 SummaryFixes a concurrency-triggered cache race in
Confidence Score: 5/5The change is a minimal, well-scoped addition to a DDL path that only runs once per relationship creation; it has no behavioral effect on any read or write path. Four cache purge calls are inserted after the real transaction commit and before createIndex, directly addressing the race window the PR describes. The fix matches identical patterns used in multiple other places in the same file, the purged cache types (collection key and METADATA document key) are both necessary and non-overlapping, and the placement is correct relative to the transaction boundary. No new failure modes are introduced. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "fix: purge collection cache after commit..." | Re-trigger Greptile |
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 3816-3822: The post-commit purge in Database::updateDocument() is
happening before the index-creation cleanup try/catch, so a failure from
withRetries() can leave committed metadata/columns without the required indexes.
Move the purge calls for the collection and relatedCollection into the same
cleanup scope as the createIndex recovery logic (around the existing try near
updateDocument) so any retry path can safely complete after duplicate metadata
is handled.
🪄 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: ee32e52a-dc7d-4ea4-ac75-5f54df146710
📒 Files selected for processing (1)
src/Database/Database.php
| // updateDocument's purge ran inside this outer transaction (nested commits don't | ||
| // commit), so it fired pre-commit and a stale collection can stay cached. Re-purge | ||
| // after the real commit, else createIndex below re-reads it without the new column. | ||
| $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId())); | ||
| $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); | ||
| $this->withRetries(fn () => $this->purgeCachedCollection($relatedCollection->getId())); | ||
| $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId())); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Put the post-commit purge inside the index-creation cleanup scope.
Line 3819 can throw after withRetries() exhausts, but this is before the try at Line 3828. That leaves committed relationship metadata/columns without creating the required indexes, and a retry will hit duplicate metadata instead of completing recovery.
Proposed fix
- // updateDocument's purge ran inside this outer transaction (nested commits don't
- // commit), so it fired pre-commit and a stale collection can stay cached. Re-purge
- // after the real commit, else createIndex below re-reads it without the new column.
- $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId()));
- $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId()));
- $this->withRetries(fn () => $this->purgeCachedCollection($relatedCollection->getId()));
- $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId()));
-
$indexKey = '_index_' . $id;
$twoWayIndexKey = '_index_' . $twoWayKey;
$indexesCreated = [];
try {
+ // updateDocument's purge ran inside this outer transaction (nested commits don't
+ // commit), so it fired pre-commit and a stale collection can stay cached. Re-purge
+ // after the real commit, else createIndex below re-reads it without the new column.
+ $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId()));
+ $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId()));
+ $this->withRetries(fn () => $this->purgeCachedCollection($relatedCollection->getId()));
+ $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId()));
+
switch ($type) {📝 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.
| // updateDocument's purge ran inside this outer transaction (nested commits don't | |
| // commit), so it fired pre-commit and a stale collection can stay cached. Re-purge | |
| // after the real commit, else createIndex below re-reads it without the new column. | |
| $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId())); | |
| $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); | |
| $this->withRetries(fn () => $this->purgeCachedCollection($relatedCollection->getId())); | |
| $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId())); | |
| $indexKey = '_index_' . $id; | |
| $twoWayIndexKey = '_index_' . $twoWayKey; | |
| $indexesCreated = []; | |
| try { | |
| // updateDocument's purge ran inside this outer transaction (nested commits don't | |
| // commit), so it fired pre-commit and a stale collection can stay cached. Re-purge | |
| // after the real commit, else createIndex below re-reads it without the new column. | |
| $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId())); | |
| $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); | |
| $this->withRetries(fn () => $this->purgeCachedCollection($relatedCollection->getId())); | |
| $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId())); | |
| switch ($type) { |
🤖 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 3816 - 3822, The post-commit purge in
Database::updateDocument() is happening before the index-creation cleanup
try/catch, so a failure from withRetries() can leave committed metadata/columns
without the required indexes. Move the purge calls for the collection and
relatedCollection into the same cleanup scope as the createIndex recovery logic
(around the existing try near updateDocument) so any retry path can safely
complete after duplicate metadata is handled.
Summary
Fixes a flaky failure when creating a two-way relationship:
It surfaces intermittently under concurrency + a shared cache (e.g. shared-tables migrations/restores), and effectively never single-threaded — the classic signature of a read-after-write cache race.
Root cause
createRelationship()does three things in order:$collection/$relatedCollection,updateDocument(self::METADATA, ...)inside its own outerwithTransaction(),createIndex(), which re-reads the collection (getCollection()) and validates the index attributes against that copy.updateDocument()already guards against stale caching — it purges again after commit (Database.php:6407, "Purge again after commit so readers cannot re-cache the pre-commit version"). But that guard only works whenupdateDocument()owns the transaction.Here it doesn't. The adapter nests transactions with a counter and only commits at the outermost level (
Adapter::commitTransaction: "If there is more than one active transaction, this decrement the transaction count... If the transaction count is 1, it will be committed"). So:Between the (pre-commit) purge and the outer commit, a concurrent reader can load the collection metadata, read the still-uncommitted (column-less) row from the DB, and re-cache it. The lease (
saveWithLease/getGeneration) doesn't catch this because no purge bumps the generation during that read. After the outer commit there is no purge, socreateIndex()'s re-read serves the stale copy → the new relationship column is missing → index validation throws.Fix
Purge the collection caches after the outer transaction commits and before
createIndex()— the post-commit purgeupdateDocument()intended to do but couldn't, because nesting deferred the real commit past it. This both (a) forces the re-read to miss the cache and read committed data, and (b) bumps the generation so any straggler stale write is rejected by the lease.This mirrors the existing post-commit purge pattern already used elsewhere in this file (e.g. lines 6406, 7795, and the metadata-mutation purges around 2260-2261).
Scope / risk
createRelationship; no signature or behavior change on any existing path.Testing
Covered by the existing relationship test suite (all relationship types still create their indexes correctly). The race itself is timing-dependent; this change removes the dependency on a post-commit re-read served from a potentially poisoned shared cache.
Summary by CodeRabbit