Skip to content

fix: purge collection cache after commit in createRelationship#906

Open
premtsd-code wants to merge 1 commit into
mainfrom
fix/relationship-index-stale-metadata-read
Open

fix: purge collection cache after commit in createRelationship#906
premtsd-code wants to merge 1 commit into
mainfrom
fix/relationship-index-stale-metadata-read

Conversation

@premtsd-code

@premtsd-code premtsd-code commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a flaky failure when creating a two-way relationship:

Failed to create relationship indexes: Invalid index attribute "..." not found

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:

  1. appends the new relationship column to the in-memory $collection / $relatedCollection,
  2. persists both via updateDocument(self::METADATA, ...) inside its own outer withTransaction(),
  3. creates the relationship's FK index via 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 when updateDocument() 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:

outer withTransaction      startTransaction   count 0->1   real BEGIN
  updateDocument
    inner withTransaction   startTransaction   count 1->2   no BEGIN
      write the new column                                  (uncommitted)
    inner commitTransaction count 2->1   NOT a real commit, just a decrement
    purgeCachedDocumentInternal()  <- fires HERE, count==1, txn STILL OPEN  (pre-commit)
outer commitTransaction    count 1->0   the REAL commit
  // no purge between here and createIndex
createIndex() re-reads -> may hit a stale, pre-commit copy

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, so createIndex()'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 purge updateDocument() 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

  • 4 lines added in createRelationship; no signature or behavior change on any existing path.
  • Only runs once per relationship creation; negligible cost.
  • Residual: genuine DB read-replica lag is out of scope (same residual the existing post-commit purges accept; not present on single-DB CI).

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

  • Bug Fixes
    • Improved cache invalidation after creating relationships so related data is refreshed only after the transaction fully commits.
    • Prevents stale metadata from appearing when changes occur within nested transactions.

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.
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

In Database::createRelationship(), four withRetries() cache purge calls are added after the outer transaction commits. These clear the cached document and collection metadata for both the source collection and the related collection, addressing stale cache entries that could persist when updateDocument's internal purge fires inside a nested transaction before the real commit.

Changes

Post-commit cache invalidation in createRelationship

Layer / File(s) Summary
Post-commit cache purge for both collections
src/Database/Database.php
After the relationship creation transaction commits, four retry-wrapped cache purge calls are added to evict the document and collection metadata caches for both $collection and $relatedCollection, replacing potentially stale entries written during the inner transaction.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐇 The cache was stale, a ghostly lie,
Left behind when commits were nigh.
But now with retries, purge calls four,
Fresh data lives — stale no more!
hop hop the rabbit clears the store. 🗑️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the main fix: re-purging collection cache after commit in createRelationship.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/relationship-index-stale-metadata-read

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes a concurrency-triggered cache race in createRelationship where updateDocument's post-commit purge fired pre-commit (due to transaction nesting), leaving a window for a concurrent reader to re-cache stale collection metadata. createIndex then validated attributes against this stale copy, causing the intermittent "Invalid index attribute not found" error.

  • Adds four purgeCachedCollection / purgeCachedDocumentInternal calls for both $collection and $relatedCollection, placed after the outer withTransaction completes and before createIndex is called — exactly mirroring the post-commit purge pattern already used at lines 2260–2261, 3351–3352, and 3478.
  • The change is isolated to DDL code, adds no new parameters, and is consistent with the existing retry + purge idiom throughout the file.

Confidence Score: 5/5

The 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

Filename Overview
src/Database/Database.php Adds four post-commit cache purges in createRelationship, after the outer withTransaction but before createIndex, to prevent a read-after-write cache race that could surface stale collection metadata missing the new relationship column.

Reviews (1): Last reviewed commit: "fix: purge collection cache after commit..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0dfae8c and d63786e.

📒 Files selected for processing (1)
  • src/Database/Database.php

Comment thread src/Database/Database.php
Comment on lines +3816 to +3822
// 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()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Suggested change
// 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant