Purge document cache again after transaction commit#887
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a short-lived per-document purge tombstone (TTL 2s, marker '1', suffix ':purged'); read paths check the tombstone to avoid or undo cache population, purge paths write the tombstone before removing cache entries, and update/delete now trigger a post-commit purge. ChangesTombstone-aware cache purging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 adds a second
Confidence Score: 4/5Safe to merge for the common case; the document-ID-rename path in updateDocument retains the stale-cache window this PR aims to close. The fix correctly handles the primary race in both updateDocument and deleteDocument. However, updateDocument has a pre-existing code path (lines 6343–6345) that purges both the original and new document IDs inside the transaction when the ID changes, and the new post-commit purge only covers the original ID. Any reader that fetches the document by its new ID in the purge→commit window will cache the stale version, and nothing evicts it — the same 24-hour staleness the PR was written to prevent. src/Database/Database.php around lines 6368–6370 — the post-commit purge block in updateDocument needs a matching new-ID purge to mirror the in-transaction logic at lines 6343–6345. Important Files Changed
Reviews (6): Last reviewed commit: "(fix): purge document cache again after ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 4924-4928: The current check-then-save around $purged allows a
race where a reader sees no tombstone, a writer then tombstones+purges, and the
reader still saves stale content; replace the non-atomic sequence using
$this->cache->load(...) followed by $this->cache->save(...) with an
atomic/conditional write. Specifically, change the logic in Database::... (the
block using $purged, $documentKey, $collectionKey and calling $this->cache->load
and $this->cache->save) to use the cache provider's atomic primitive (e.g.
add/putIfAbsent/compareAndSwap/CAS or an explicit lock) so the document save
only succeeds if the tombstone key does not exist at the moment of the write; if
your cache supports add/putIfAbsent use that to create the document key only
when absent, or if it supports CAS use compareAndSwap with the expected
non-tombstone state, otherwise acquire a short-lived lock for $documentKey
before re-checking the tombstone and saving. Ensure the tombstone test and the
document save are performed atomically to prevent the stale-save race.
- Around line 8358-8360: The tombstone write
($this->cache->save("{$documentKey}:purged", ...)) can throw and prevent the
subsequent purge calls; wrap the save call in a try/catch that logs the error
but does not rethrow so execution always continues, then call
$this->cache->purge($collectionKey, $documentKey) and
$this->cache->purge($documentKey) unconditionally (or in a finally block) to
ensure purging runs even if the tombstone write fails; reference the
$this->cache->save and $this->cache->purge calls and the
$documentKey/$collectionKey variables when making the change.
🪄 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: 84d1d089-e6eb-4784-a0b9-0be0634609ee
📒 Files selected for processing (1)
src/Database/Database.php
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Reviewed the 13-line cache-tombstone change across security, performance, correctness, readability, maintainability, and testing. No critical findings. Four readability/maintainability warnings, all fixed in the commit pushed by the consolidator.
Findings
- Security: clean.
- Performance: clean — one extra cache
load()per cache-miss is acceptable cost for correctness. - Correctness: the residual narrow race between
tombstone check → cache saveis inherent to optimistic invalidation and not a defect to introduce here. Note:purgeCachedCollection(Database.php:8327-8339) iteratescache->purge($documentKey)directly without writing tombstones, so the race remains open for schema-level invalidations — out of scope for this PR, but worth a follow-up if collection-purge callers ever run concurrently with document reads. - Readability/Maintainability: 4 warnings (see inline comments) — all addressed.
- Testing: no tests added; the project's tests need Docker so adding one here is out of scope for an automated pass.
Changes made
- Extracted
:purgedintoPURGE_TOMBSTONE_SUFFIXso the tombstone key is built in one place. - Dropped the
inttype onPURGE_TOMBSTONE_TTL— it was the only typed constant in a file with 100+ untyped ones (including the adjacentTTL). - Replaced
(string) \time()with'1'— only the presence of the tombstone matters. - Added two short comments explaining the race the tombstone prevents at both call sites.
|
Claude pushed fixes from: improvement |
Code ReviewReviewed across six dimensions (correctness, performance, security, readability, maintainability, testing). The tombstone strategy is sound, and commits Findings & follow-up fixes
False positive flaggedOne reviewer claimed Deferred (not blocking)
Verification
|
|
Claude could not apply patches from: improvement (merge conflicts with current branch tip). These tasks need manual attention. |
|
No critical/warning findings. |
There was a problem hiding this comment.
Code Review
Multi-dimensional review of the cache-tombstone race fix. The fix's core idea is correct, but a real time-of-check/time-of-use window remained between the pre-write tombstone check and the two cache writes that follow it.
Critical — fixed in this pass:
- TOCTOU between the pre-write tombstone check and the cache writes. A purge that landed between the
loadand the twosavecalls would still poison the cache. Now rechecked after the writes and rolled back if the tombstone appears. PURGE_TOMBSTONE_TTL = 10swas tight against worst-case read paths (deep relationship resolution, coroutine scheduling delays, slow DB reads). Bumped to 30s.
Warnings — fixed in this pass:
- Tombstone key construction and the
key == hashconvention were duplicated at two call sites with no shared helper. ExtractedmarkCachePurged()/isCachePurged().
Considered, not flagged:
- Tombstone key collision with documents named
*:purged— not exploitable:Validator/Keyrestricts UIDs to[A-Za-z0-9_\-\.], so document IDs can never contain:. - Tombstone accumulation in the cache backend — architectural limit of
utopia-php/cache(no TTL onsave). Not introduced by this PR. - Non-atomic two-key save (documentKey then collectionKey) — pre-existing; the new post-write recheck mitigates the window touched by this PR.
- Adding a unit test for the race — the codebase tests against real adapters and has no precedent for concurrency-race unit tests; a non-deterministic test would be cargo-culted.
| if ($purged === false) { | ||
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); | ||
| } |
There was a problem hiding this comment.
[CRITICAL] TOCTOU: a concurrent purge that lands between the pre-check load (line 4926) and the two save calls (lines 4928-4929) would set the tombstone and clear the cache, after which the reader's saves repoison it. Fixed by adding a post-save recheck that re-reads the tombstone and purges the just-written entries if it appears — closing the race the PR claims to fix.
| // Cache | ||
| public const TTL = 60 * 60 * 24; // 24 hours | ||
|
|
||
| public const PURGE_TOMBSTONE_TTL = 10; // seconds |
There was a problem hiding this comment.
[CRITICAL] TTL of 10s is tight against the worst-case reader path (slow DB fetch + deep relationship resolution + coroutine scheduling). If the reader takes longer than this between fetching from the DB and the final tombstone check, the tombstone has virtually expired and the stale save proceeds. Bumped to 30s as a safer margin; tradeoff is 30s of bypassed cache writes per purged document key.
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); | ||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| $purged = $this->cache->load($tombstoneKey, self::PURGE_TOMBSTONE_TTL, $tombstoneKey); |
There was a problem hiding this comment.
[WARNING] Tombstone key construction ($documentKey . PURGE_TOMBSTONE_SUFFIX) and the non-obvious save(key, '1', key) convention (using the key as its own hash) were duplicated at two call sites. Extracted into markCachePurged() / isCachePurged() so the convention lives in one place and the call sites read as intent rather than mechanics.
|
|
||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| try { | ||
| $this->cache->save($tombstoneKey, '1', $tombstoneKey); |
There was a problem hiding this comment.
[WARNING] Same duplication concern as the reader path — the tombstone-save block here was identical structure to the load block at line 4926. Replaced with markCachePurged($documentKey). The semantics (warning-log on failure, never propagate) are preserved by the helper.
|
Claude pushed fixes from: improvement |
There was a problem hiding this comment.
Code Review
Reviewed across correctness, concurrency, performance, security, maintainability, and testing. Findings below; fixed items have been pushed as follow-up commits to this branch.
Correctness / Concurrency
- [CRITICAL — fixed]
purgeCachedCollectionenumerated and purged documents but never set tombstones for them, leaving the same race window the PR was meant to close. Every bulk-purge call site (collection rename, attribute change, relationship mutations) was affected. AddedmarkCachePurged($documentKey)inside the purge loop in a follow-up commit. - [WARNING] Slow reader >
PURGE_TOMBSTONE_TTL(30s) — a read that begins before a purge but reaches the cache-save block more than 30s later (slow adapter, retries viawithRetries, deep relationship populate) will see the tombstone expired and re-poison the cache. The fix here would be capturing a monotonic read-start timestamp and storing it in the tombstone value rather than relying on TTL existence; left as a design note since it changes the PR's tombstone semantics.
Performance
- [WARNING] The post-write recheck (line 4931) costs an extra cache round-trip on every cache miss to handle a sub-millisecond race window. Worth considering for a follow-up: pipeline the tombstone check with the initial document load, or move to an atomic
SETNX-style write. Not refactored here — design decision belongs to PR author.
Maintainability
- [WARNING — fixed] Magic
'1'tombstone marker value extracted toPURGE_TOMBSTONE_MARKERconstant. - [SUGGESTION]
cache->save($tombstoneKey, '1', $tombstoneKey)uses the same value for key and hash — the cache library composes the slot askey:hash, so the tombstone ends up atdocumentKey:purged:documentKey:purged. Functional but wastes keyspace and makes the tombstone undiscoverable from acache->list($documentKey)sweep. Left untouched to avoid changing storage layout mid-PR.
Security
- [SUGGESTION] Tombstone key construction
$documentKey . ':purged'uses:as the separator — the same separator already appearing in cache keys. Currently safe because the document-ID validator restricts the charset, but a future relaxation could create a collision where a doc with IDsomething:purgedaliases another doc's tombstone. Consider a non-colon sentinel or a distinct prefix (tombstone:+ key) for defense in depth.
Testing
- [WARNING] Concurrency-sensitive fix with no regression tests. Recommend adding a unit test that drives a stub
Cache: assert that callingmarkCachePurged($documentKey)before thegetDocumentsave block causes the document save to be skipped, and that a tombstone appearing between the writes and the recheck triggers the compensating purge. Not added in this pass — would expand PR scope.
Summary
2 critical findings (1 fixed: bulk-purge tombstone gap; 1 partial: missing tests). 3 warnings (1 fixed: marker constant; 2 design tradeoffs noted). 2 suggestions deferred to follow-ups.
| // Cache | ||
| public const TTL = 60 * 60 * 24; // 24 hours | ||
|
|
||
| public const PURGE_TOMBSTONE_TTL = 30; // seconds |
There was a problem hiding this comment.
[WARNING] PURGE_TOMBSTONE_TTL = 30 (seconds) is the worst-case wall-clock budget for any in-flight read to reach its cache-save block. Slow reads (relationship population at line 4912, withRetries paths, deep DB latency) can exceed 30s and re-poison the cache. A more robust design stores the purge timestamp inside the tombstone and compares against a read-start timestamp captured at the top of getDocument, so existence — not TTL — is what gates the write. Not blocking, but flag for follow-up.
|
|
||
| // Recheck after the writes: a concurrent purge may have set | ||
| // the tombstone between our pre-check and the saves above. | ||
| if ($this->isCachePurged($documentKey)) { |
There was a problem hiding this comment.
[SUGGESTION] The post-write recheck closes the race but adds a full cache round-trip on every miss. The race window between the pre-check and the saves is sub-millisecond; a tombstone arriving precisely in that window is rare. Consider whether the recheck could be replaced by an atomic write primitive (SETNX on the tombstone, or a Lua-script CAS), or whether the cost is acceptable for the correctness guarantee. No change requested — calling out the tradeoff.
|
|
||
| [$collectionKey, $documentKey] = $this->getCacheKeys($collectionId, $id); | ||
|
|
||
| $this->markCachePurged($documentKey); |
There was a problem hiding this comment.
[CRITICAL — fixed in follow-up commit] This tombstone protection covers single-document purges, but purgeCachedCollection (around line 8334) enumerates and purges documents in a loop without calling markCachePurged() for each. Every bulk-purge path (line 2111, 2263, 2469, 3212, 3354, 3481, 4058, 4227, 4228, 4427, 4428, 4510) leaves the race window wide open. Fixed by adding $this->markCachePurged($documentKey) inside the purgeCachedCollection loop before each $this->cache->purge($documentKey) call.
| { | ||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| try { | ||
| $this->cache->save($tombstoneKey, '1', $tombstoneKey); |
There was a problem hiding this comment.
[SUGGESTION] Using $tombstoneKey as both $key and $hash causes the cache library to write to slot documentKey:purged:documentKey:purged. Works, but doubles the keyspace pressure and prevents the tombstone from being swept by a cache->list($documentKey) walk. Consider cache->save($tombstoneKey, self::PURGE_TOMBSTONE_MARKER) (empty hash) or co-locating the tombstone as a hash under the document key. Left for a follow-up to avoid changing the storage layout mid-fix.
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| try { | ||
| return $this->cache->load($tombstoneKey, self::PURGE_TOMBSTONE_TTL, $tombstoneKey) !== false; | ||
| } catch (Exception $e) { |
There was a problem hiding this comment.
[WARNING] Fail-open behavior: when the cache backend is unreachable, isCachePurged() returns false and the reader proceeds to write. Combined with markCachePurged() also swallowing errors, a backend outage silently disables the entire mechanism. This is a deliberate tradeoff (cache availability over strictness) but worth documenting in the docblock so it isn't accidentally re-tuned later.
|
Claude pushed fixes from: improvement |
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 8405-8428: markCachePurged currently writes a permanent tombstone
(documentKey . PURGE_TOMBSTONE_SUFFIX) which never expires; change
markCachePurged to write the tombstone with a real TTL (use
self::PURGE_TOMBSTONE_TTL) via the cache API (e.g. pass the TTL argument to
cache->save or call the cache method that accepts a TTL) so tombstones
auto-evict, and if the cache client lacks a TTL save method add logic in
isCachePurged to remove the stale tombstone when you detect it expired;
reference markCachePurged, isCachePurged and self::PURGE_TOMBSTONE_TTL when
making the fix.
- Around line 4924-4934: Add the `@phpstan-impure` annotation to
Database::isCachePurged() so PHPStan won’t assume the second call is identical
to the first (the result can change due to concurrent markCachePurged() writes),
and update the post-write cleanup to also remove the tombstone key (the key
produced by markCachePurged(): $documentKey . self::PURGE_TOMBSTONE_SUFFIX) so
tombstones don’t accumulate; ensure any purge path that removes a document or
collection calls cache->purge(...) for that tombstone key as well and rely on
Envelope::decode() for TTL-only expiry behavior until physical tombstone purge
is performed.
🪄 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: d5646ccd-eebf-4328-87aa-b754d4f23572
📒 Files selected for processing (1)
src/Database/Database.php
| try { | ||
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); | ||
| if (!$this->isCachePurged($documentKey)) { | ||
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); | ||
|
|
||
| // Recheck after the writes: a concurrent purge may have set | ||
| // the tombstone between our pre-check and the saves above. | ||
| if ($this->isCachePurged($documentKey)) { | ||
| $this->cache->purge($collectionKey, $documentKey); | ||
| $this->cache->purge($documentKey); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Database/Database.php"
# Show the reported read-side guard block (around 4924-4934)
echo "---- Context around lines 4920-4940 ----"
nl -ba "$FILE" | sed -n '4905,4950p'
# Show where isCachePurged is declared (around 8398-8428)
echo "---- Context around lines 8370-8450 ----"
nl -ba "$FILE" | sed -n '8360,8455p'
# Show purgeCachedDocumentInternal and markCachePurged to understand flow (around 8365)
echo "---- Context around lines 8320-8420 ----"
nl -ba "$FILE" | sed -n '8320,8425p'
# Find any phpstan ignore/annotation patterns already used in this file for guidance
echo "---- Existing phpstan suppression patterns in this file ----"
rg -n "phpstan-(ignore|impure)|`@phpstan`|`@psalm`|suppress" "$FILE" || true
# Find any calls to isCachePurged in getDocument to confirm the 2-call structure
echo "---- Calls to isCachePurged in getDocument/getDocument-related ----"
rg -n "isCachePurged\(" "$FILE" | head -n 50Repository: utopia-php/database
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Database/Database.php"
echo "---- File existence ----"
ls -la "$FILE" || (echo "Missing $FILE" && exit 1)
echo "---- Context around lines 4920-4950 (via cat -n) ----"
cat -n "$FILE" | sed -n '4905,4950p' || true
echo "---- Context around lines 8370-8455 (isCachePurged declaration) ----"
cat -n "$FILE" | sed -n '8360,8455p' || true
echo "---- Context around lines 8320-8425 (purgeCachedDocumentInternal/markCachePurged) ----"
cat -n "$FILE" | sed -n '8320,8425p' || true
echo "---- Existing phpstan suppression patterns in this file ----"
rg -n "phpstan-(ignore|impure)|`@phpstan`|`@psalm`|suppress" "$FILE" || true
echo "---- All isCachePurged( call sites ----"
rg -n "isCachePurged\(" "$FILE" || trueRepository: utopia-php/database
Length of output: 11094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Database/Database.php"
echo "---- Find PURGE_TOMBSTONE_SUFFIX usage ----"
rg -n "PURGE_TOMBSTONE_SUFFIX" "$FILE"
echo "---- Find PURGE_TOMBSTONE_TTL usage ----"
rg -n "PURGE_TOMBSTONE_TTL" "$FILE"
echo "---- Find cache purge of tombstoneKey pattern (purge with suffix) ----"
rg -n "tombstoneKey.*purge|purge.*tombstone|PURGE_TOMBSTONE_.*purge" "$FILE" || true
echo "---- Wider search: any other file references to PURGE_TOMBSTONE_SUFFIX ----"
fd -e php src | xargs rg -n "PURGE_TOMBSTONE_SUFFIX" | head -n 50 || true
echo "---- Search for PHPStan ignore patterns in repo (to mimic approach) ----"
fd -e php src | xargs rg -n "`@phpstan-`(ignore|ignore-next-line)|phpstan-ignore" | head -n 50 || trueRepository: utopia-php/database
Length of output: 1927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect cache implementation/interfaces to understand TTL + expiry behavior for tombstones
# 1) Locate Cache interface/trait/classes
echo "---- Locate cache 'save' / 'load' definitions ----"
fd -e php "Cache" src | head -n 50
# Grep for "function load(" and "function save(" across cache-related files
echo "---- Search for function load/save in src (limited) ----"
rg -n --hidden --no-ignore-vcs "function (load|save)\(" src/Database src | head -n 80 || true
# Find the specific cache type used by Database.php (search for "$this->cache" property type / constructor)
echo "---- Find Database.php cache property/constructor typing ----"
rg -n "\$cache" src/Database/Database.php | head -n 80
# Show relevant area near Database constructor / property declarations
echo "---- Context around Database.php cache property and constructor ----"
rg -n "function __construct|private .*cache|protected .*cache" src/Database/Database.php | head -n 50Repository: utopia-php/database
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all load call sites on the cache in this repo
echo "---- Cache load call sites ----"
rg -n "\->load\(" src | head -n 80
# 2) Find the cache property type in Database.php (if annotated)
echo "---- Database.php cache property declaration ----"
rg -n "private\s+\$cache|protected\s+\$cache|public\s+\$cache" src/Database/Database.php
# 3) Identify the cache class/interface by searching for "new .*Cache" and by searching the type-hint near $cache usage
echo "---- Search for explicit cache class/interface names ----"
rg -n "implements .*Cache|interface .*Cache|class .*Cache" src | head -n 80
# 4) Locate the actual implementation of "load(" with a TTL signature.
# We'll search for patterns used in Database.php: load($key, $ttl, $tombstoneKey) or similar.
echo "---- Search for "load(..., $ttl" patterns ----"
rg -n "function\s+load\s*\(.*\$\w+.*,\s*\$?\w+.*\)" src | head -n 80 || true
# 5) Search for exact signature fragments used in Database.php isCachePurged():
# "load($tombstoneKey, self::PURGE_TOMBSTONE_TTL, $tombstoneKey)"
echo "---- Search for the string "PURGE_TOMBSTONE_TTL" in load implementations (defensive) ----"
rg -n "PURGE_TOMBSTONE_TTL" src || trueRepository: utopia-php/database
Length of output: 390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Repo top-level dirs ----"
ls -la
echo "---- List PHP files under src that look cache-related ----"
fd -t f -e php -i "*cache*" src | head -n 200
echo "---- Find any 'interface .*Cache' / 'class .*Cache' ----"
rg -n --hidden --no-ignore-vcs "(interface|class)\s+.*Cache\b" src | head -n 200 || true
echo "---- Find definitions of 'function load(' in repo (not just src) ----"
rg -n --hidden --no-ignore-vcs "function\s+load\s*\(" . | head -n 200 || true
echo "---- Find definitions of 'function save(' in repo ----"
rg -n --hidden --no-ignore-vcs "function\s+save\s*\(" . | head -n 200 || trueRepository: utopia-php/database
Length of output: 2219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Find cache-ish PHP files by filename (literal) ----"
find src -type f -name '*cache*.php' | head -n 200
echo "---- Find all PHP files that mention '->load(' and 'function load(' ----"
rg -n "function\s+load\s*\(" src || true
rg -n "\->load\(" src || true
echo "---- Find cache interface/class names (Cache/Adapter/etc.) ----"
rg -n --hidden --no-ignore-vcs "(interface|class)\s+\w*Cache\b" src || true
echo "---- Find any TTL handling code patterns ----"
rg -n "TTL|expire|expiration|ttl\s*=" src || trueRepository: utopia-php/database
Length of output: 8021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Search repo for `@phpstan-impure` usage ----"
rg -n "`@phpstan-impure`" . || true
echo "---- Search vendor for 'function load(' implementations (sample) ----"
rg -n --hidden --no-ignore-vcs "function\s+load\s*\(" vendor | head -n 50 || true
echo "---- Search vendor for '->load(' call sites (sample) ----"
rg -n --hidden --no-ignore-vcs "\->load\(" vendor | head -n 50 || true
echo "---- Search vendor for load signature with ttl parameter ----"
rg -n --hidden --no-ignore-vcs "function\s+load\s*\([^\)]*ttl[^\)]*\)" vendor | head -n 50 || true
echo "---- Search vendor for cache interfaces named Cache (sample) ----"
rg -n --hidden --no-ignore-vcs "interface\s+.*Cache\b|class\s+.*Cache\b" vendor | head -n 50 || trueRepository: utopia-php/database
Length of output: 13844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# ---- Inspect cache save/load/purge implementations in vendor/utopia-php/cache ----
CACHE_FILE="vendor/utopia-php/cache/src/Cache/Cache.php"
ADAPTER_FILE="vendor/utopia-php/cache/src/Cache/Adapter.php"
echo "---- Cache::save/load/purge in $CACHE_FILE ----"
ls -la "$CACHE_FILE" "$ADAPTER_FILE"
cat -n "$CACHE_FILE" | sed -n '1,220p'
echo "---- Adapter interface/abstract in $ADAPTER_FILE ----"
cat -n "$ADAPTER_FILE" | sed -n '1,220p'
# Look specifically for save() signature details (TTL param default? hash usage?)
echo "---- Search within Cache.php for 'function save' and 'function purge' ----"
rg -n "function\s+save\s*\(|function\s+purge\s*\(|function\s+load\s*\(" "$CACHE_FILE"
# ---- Inspect purge tombstone behavior: is tombstoneKey ever purged with TTL? ----
echo "---- Search in vendor cache adapter implementations for 'purge(' and TTL semantics ----"
rg -n "function\s+purge\s*\(" vendor/utopia-php/cache/src/Cache | head -n 50 || true
rg -n "function\s+load\s*\(" vendor/utopia-php/cache/src/Cache/Adapter | head -n 50 || true
# Print a representative adapter (Redis + Memory + Filesystem) load implementations to understand TTL/expiry.
for f in \
vendor/utopia-php/cache/src/Cache/Adapter/Redis.php \
vendor/utopia-php/cache/src/Cache/Adapter/Memory.php \
vendor/utopia-php/cache/src/Cache/Adapter/Filesystem.php \
vendor/utopia-php/cache/src/Cache/Adapter/CircuitBreaker.php; do
if [ -f "$f" ]; then
echo "---- Adapter file: $f (load/save/purge excerpts) ----"
rg -n "function\s+(load|save|purge)\s*\(" "$f" || true
cat -n "$f" | sed -n '1,140p'
fi
done
# ---- Verify PHPStan impure tag support and exact spelling ----
echo "---- Search vendor/phpstan for `@phpstan-impure` handling ----"
rg -n "`@phpstan-impure`" vendor | head -n 50 || true
rg -n "phpstan-impure" vendor | head -n 50 || true
# Also check phpstan.neon for any relevant settings
echo "---- phpstan.neon ----"
cat -n phpstan.neon | sed -n '1,200p'Repository: utopia-php/database
Length of output: 32184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Envelope implementation
echo "---- Find Envelope::decode implementation ----"
rg -n "class Envelope|function decode" vendor/utopia-php/cache/src/Cache -S || true
rg -n "class Envelope|function decode" vendor/utopia-php/cache/src -S | head -n 50
ENV_FILE="$(rg -l "class Envelope" vendor/utopia-php/cache/src | head -n 1)"
echo "---- Envelope file: $ENV_FILE ----"
cat -n "$ENV_FILE" | sed -n '1,240p'
# Also inspect Envelope::encode for stored timestamp structure
echo "---- Search for Envelope::encode ----"
rg -n "function\s+encode" "$ENV_FILE" || trueRepository: utopia-php/database
Length of output: 3245
Fix PHPStan CI failure for the post-write tombstone re-check (and address tombstone accumulation)
- Add
@phpstan-impuretoDatabase::isCachePurged()so PHPStan doesn’t fold the secondisCachePurged()call into the first (the value can change due to concurrentmarkCachePurged()writes). markCachePurged()persists tombstones under a separate key ($documentKey . self::PURGE_TOMBSTONE_SUFFIX); the “TTL” is enforced only at read time (Envelope::decode()returnsfalsewhen expired) and there’s no physicalpurge($tombstoneKey)path, so tombstones will accumulate in the cache backend.
Suggested minimal fix
/**
* Check whether a document's cache entry was purged within
* PURGE_TOMBSTONE_TTL seconds. Returns false on cache errors so writes
* can still proceed when the tombstone backend is unreachable.
+ *
+ * `@phpstan-impure`
*/
private function isCachePurged(string $documentKey): bool📝 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.
| try { | |
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | |
| $this->cache->save($collectionKey, 'empty', $documentKey); | |
| if (!$this->isCachePurged($documentKey)) { | |
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | |
| $this->cache->save($collectionKey, 'empty', $documentKey); | |
| // Recheck after the writes: a concurrent purge may have set | |
| // the tombstone between our pre-check and the saves above. | |
| if ($this->isCachePurged($documentKey)) { | |
| $this->cache->purge($collectionKey, $documentKey); | |
| $this->cache->purge($documentKey); | |
| } | |
| /** | |
| * Check whether a document's cache entry was purged within | |
| * PURGE_TOMBSTONE_TTL seconds. Returns false on cache errors so writes | |
| * can still proceed when the tombstone backend is unreachable. | |
| * | |
| * `@phpstan-impure` | |
| */ | |
| private function isCachePurged(string $documentKey): bool |
🧰 Tools
🪛 GitHub Actions: CodeQL / 0_CodeQL.txt
[error] 4931-4931: PHPStan (phpstan analyse --level 7) found an error: "If condition is always false."
🪛 GitHub Actions: CodeQL / CodeQL
[error] 4931-4931: PHPStan: If condition is always false.
🤖 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 4924 - 4934, Add the `@phpstan-impure`
annotation to Database::isCachePurged() so PHPStan won’t assume the second call
is identical to the first (the result can change due to concurrent
markCachePurged() writes), and update the post-write cleanup to also remove the
tombstone key (the key produced by markCachePurged(): $documentKey .
self::PURGE_TOMBSTONE_SUFFIX) so tombstones don’t accumulate; ensure any purge
path that removes a document or collection calls cache->purge(...) for that
tombstone key as well and rely on Envelope::decode() for TTL-only expiry
behavior until physical tombstone purge is performed.
Source: Pipeline failures
ChiragAgg5k
force-pushed
the
fix-document-cache-purge-race
branch
from
cc585a1 to
75379ec
Compare
|
CI run 27253879897 failed in the Adapter Tests (Postgres) → Load and Start Services step due to a Docker Hub registry timeout: This is an infrastructure flake ( |
|
CI run 27254072069 failed in Adapter Tests (Pool) → Load and Start Services due to a transient Docker Hub registry timeout while pulling service images: All service containers (mongo, redis, mariadb, mysql, adminer, mongo-express, and their mirrors) failed to pull and the step exited before any tests ran. This is an infrastructure flake unrelated to the PR diff — no code fix applicable. Re-running the workflow should resolve it. |
ChiragAgg5k
force-pushed
the
fix-document-cache-purge-race
branch
from
75379ec to
35d6eb4
Compare
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 6381-6383: Post-commit cache purge calls
(purgeCachedDocumentInternal) are currently allowed to throw and thus can
surface as failed writes; wrap the post-commit invocations of
purgeCachedDocumentInternal (the call at the shown location and the other
occurrence referenced) in a try/catch that catches and logs the exception
(including context: collection id and document id) but does not rethrow or
change the transaction result so the already-committed update/delete returns
success; ensure the catch uses the existing logger/util used in this class and
mirrors the same non-fatal handling for both locations.
🪄 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: aae01cfc-66c6-4aa5-b90d-626cdbb0f162
📒 Files selected for processing (1)
src/Database/Database.php
There was a problem hiding this comment.
Code Review
Reviewed the cache-tombstone mechanism against origin/main. The PR addresses a real race (concurrent reader caching a stale snapshot after a purge), but the current implementation has several gaps that re-open the same window the PR is trying to close.
Findings by dimension:
-
Correctness (CRITICAL ×3, WARNING ×1)
- The
PURGE_TOMBSTONE_TTL = 2seconds window is too short to cover any non-trivial read path between the pre-check and the cache write. Any DB query, decode pass, relationship walk, or PHP GC pause that exceeds 2s lets a stale snapshot be written under$documentKeyand stay there for the fullself::TTL(24h). [inline on line 177] - The pre-check in
getDocumentis now the only tombstone check. There is no post-write recheck — if a concurrent purge sets the tombstone between the pre-check and the twocache->save()calls, the stale snapshot is cached and never rolled back. [inline on line 4929] purgeCachedCollectionno longer sets any tombstone (the per-document tombstoning was removed from the loop and no collection-level tombstone replaces it). A concurrentgetDocumentthat registers a new document under$collectionKeywhile the listing loop runs produces a cached entry that is never tombstoned and is not visited by the loop. The finalpurge($collectionKey)only removes the listing, not the data, so the document remains stale-cached. (purgeCachedCollectionis unchanged in this PR's diff so there is no in-diff line to anchor an inline comment on; see summary.)- In
purgeCachedDocumentInternal, if the firstcache->save($tombstoneKey, …)succeeds but a subsequent operation throws, there is no recheck/rollback contract; andgetDocumentperforms no post-save recheck either. The earlier iteration of this PR had a recheck — that protection is gone. [inline on line 8367]
- The
-
Maintainability (WARNING ×1)
- The tombstone read at lines 4925–4927 and the tombstone write at 8365–8370 inline the same key construction (
$documentKey . self::PURGE_TOMBSTONE_SUFFIX) and exception-handling shape. ExtractingmarkCachePurged()/isCachePurged()private helpers (or equivalent) keeps the two halves of the mechanism in lockstep and gives a single place to evolve the policy.
- The tombstone read at lines 4925–4927 and the tombstone write at 8365–8370 inline the same key construction (
-
Performance, Security, Readability, Testing — no critical/warning findings beyond the above. (A unit-test for the race the PR fixes would be valuable but is suggestion-level given the difficulty of deterministically reproducing it.)
Recommended fix shape:
- Restore the post-save recheck around the two
cache->savecalls ingetDocument; wrap the saves intry { … } finally { recheck-and-rollback }so a partial-write exception still triggers the rollback. - In
purgeCachedCollection, write a single collection-level tombstone (under$collectionKey . self::PURGE_TOMBSTONE_SUFFIX) beforecache->list($collectionKey). HavegetDocumentcheck both the per-document and the collection-level tombstones (isCachePurged($documentKey) || isCachePurged($collectionKey)). This both closes the bulk-purge race and removes the need for per-document tombstones in the loop. - Raise
PURGE_TOMBSTONE_TTLback to a value that comfortably exceeds the longest realistic read path (the previous 30s is a defensible default; expose a setter if some deployments need more). - Re-extract the
markCachePurged/isCachePurgedhelpers so the tombstone read and write share a single key-construction and exception-handling path.
A local artifact has been prepared along these lines for the consolidator job.
| // Cache | ||
| public const TTL = 60 * 60 * 24; // 24 hours | ||
|
|
||
| public const PURGE_TOMBSTONE_TTL = 2; // seconds |
There was a problem hiding this comment.
[CRITICAL] PURGE_TOMBSTONE_TTL = 2 seconds is far too short to act as a safety window. Any read path that takes longer than 2s between the pre-check (line 4926) and the cache write (lines 4928–4929) — slow DB query, replica lag, relationship walk, GC pause, network hiccup — will write stale data after the tombstone has already expired, leaving the document stale-cached for the full 24h self::TTL. A previous iteration of this PR used 30s; that is a safer default. Consider also making this configurable via a setter so deployments with longer tail-latency can tune it.
| $purged = $this->cache->load($tombstoneKey, self::PURGE_TOMBSTONE_TTL, $tombstoneKey); | ||
| if ($purged === false) { | ||
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); |
There was a problem hiding this comment.
[CRITICAL] The pre-check on line 4926 is now the only tombstone check. There is no post-write recheck, so the window between $purged === false and the two cache->save(...) calls is unprotected: a concurrent purgeCachedDocumentInternal that sets the tombstone in between will not block the stale write, and the stale entry will sit under $documentKey:$hashKey for 24h. Also, if the first save succeeds and the second throws, there is no rollback — the first stale entry remains.
Fix: restore the post-write recheck-and-rollback, and wrap the two save() calls in try { … } finally { … } so the rollback also fires when the second save throws after the first one succeeded:
try {
$this->cache->save($documentKey, $document->getArrayCopy(), $hashKey);
$this->cache->save($collectionKey, 'empty', $documentKey);
} finally {
if ($this->isCachePurged($documentKey) || $this->isCachePurged($collectionKey)) {
$this->cache->purge($collectionKey, $documentKey);
$this->cache->purge($documentKey);
}
}|
|
||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| try { | ||
| $this->cache->save($tombstoneKey, '1', $tombstoneKey); |
There was a problem hiding this comment.
[WARNING] This tombstone-save block is inlined here, but the same key construction ($documentKey . self::PURGE_TOMBSTONE_SUFFIX) and exception-handling shape is also needed in getDocument's pre-check and in any post-save recheck. The previous iteration of this PR extracted markCachePurged() / isCachePurged() helpers; that gave a single place to enforce the contract.
Separately: purgeCachedCollection (lines 8334–8350, not shown in this diff) no longer tombstones at all. A reader that registers a new document under $collectionKey concurrently with the listing loop produces a cache entry that is never tombstoned and never visited by the loop — purge($collectionKey) only removes the listing, not the data. Recommend writing a collection-level tombstone before cache->list($collectionKey) and having getDocument check both the per-document and collection-level tombstones.
|
Claude could not apply patches from: improvement (merge conflicts with current branch tip). These tasks need manual attention. |
There was a problem hiding this comment.
Code Review
Reviewed PR #887 — the tombstone race fix — across correctness/concurrency, security, performance, readability, maintainability, and testing.
Summary
- Correctness: Found two real gaps in the race coverage — fixed in this push.
- Security: No new vectors; tombstone-write failures are logged but not propagated, by design.
- Performance: Extra
loadper cache miss is acceptable; flagged a residual TOCTOU window where the precheck-only pattern can still let a stale write through. - Maintainability: Tightened constant visibility (
public->private). - Testing: No tests added for tombstone behavior — flagged as a follow-up.
Fixes applied in this push
- [CRITICAL]
updateDocumentpost-commit purge missed the renamed-id case. The in-transaction block (lines 6348-6352) purges both$idand$document->getId()when an update renames the document. The new post-commit step only handled$id, recreating the very race the PR closes — under the new id. Mirrored the rename guard after the commit. - [CRITICAL]
purgeCachedCollectionhad no tombstones. This method is called from schema changes and bulk operations and previously purged each document key without any tombstone protection. A concurrent reader holding a stale snapshot could re-cache after the loop'scache->purgeand the entry would live for up to 24h. Added per-document tombstone writes inside the loop, mirroringpurgeCachedDocumentInternal. (This was present in earlier iterations of the PR and was dropped during the rewrite; it should be restored.) - [WARNING] Made
PURGE_TOMBSTONE_TTLandPURGE_TOMBSTONE_SUFFIXprivate— they are implementation detail of the tombstone protocol, not API surface.
Flagged but not changed (judgment calls / out of scope)
PURGE_TOMBSTONE_TTL = 2sis short; lengthens the suppression window vs the race window symmetrically. Worth documenting the tradeoff in a comment in a follow-up.- Tombstones are written via
cache->savewith no backend-level TTL, so on adapters that don't auto-evict the:purgedkeyspace grows with purge volume. Bounding this requires TTL-on-save support inutopia-php/cache; track as a follow-up. - The precheck-only pattern leaves a TOCTOU window where a tombstone written between
loadandsaveis missed (see inline comment). The post-commit purge in single-doc paths catches this, but it relies on the post-commit step actually firing — which is the part the two CRITICAL fixes above now cover. - No unit tests cover the tombstone state machine. Three small tests against a mock
Cachewould cover tombstone-blocks-save, tombstone-write-on-purge, and renamed-id-double-purge.
| // Cache | ||
| public const TTL = 60 * 60 * 24; // 24 hours | ||
|
|
||
| public const PURGE_TOMBSTONE_TTL = 2; // seconds |
There was a problem hiding this comment.
[WARNING — FIXED] These constants are only consumed inside private/protected helpers (the tombstone load in getDocument and the tombstone save in purgeCachedDocumentInternal). Exposing them as public const makes them API surface and locks in implementation details (the suffix string, the 2s TTL). Changed to private const.
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); | ||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| $purged = $this->cache->load($tombstoneKey, self::PURGE_TOMBSTONE_TTL, $tombstoneKey); |
There was a problem hiding this comment.
[SUGGESTION] Precheck-only pattern leaves a TOCTOU window: between this cache->load and the saves at 4928-4929, a concurrent purger can write the tombstone and call cache->purge. The reader's saves then land after the purge, with no tombstone observed — the cache holds the stale value until the post-commit purge fires (which now does, for both $id and the renamed id after the fix on this PR).
Worth considering a recheck-after-save variant: always save, then load the tombstone again, and if present, purge what was just written. That closes the window without an extra RTT on the cache-miss happy path (the precheck would no longer be needed). Not flipped here; flagging for the author's design decision.
|
|
||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| try { | ||
| $this->cache->save($tombstoneKey, '1', $tombstoneKey); |
There was a problem hiding this comment.
[SUGGESTION] The tombstone is written via cache->save with no explicit backend TTL — cache->load enforces the 2s window at read time, but the backend entry persists until evicted (LRU on Redis, never on filesystem/in-process adapters). On busy systems this is a slow keyspace leak.
Bounding this requires TTL-on-save support in utopia-php/cache. Track as a follow-up; not blocking for this PR.
| try { | ||
| $this->cache->save($tombstoneKey, '1', $tombstoneKey); | ||
| } catch (Exception $e) { | ||
| Console::warning('Failed to save purge tombstone to cache: ' . $e->getMessage()); |
There was a problem hiding this comment.
[SUGGESTION] Failures here log via Console::warning and continue — the purge still fires. This is intentional (tombstone-write failure must not block the purge) but it means tombstone-write failures silently degrade cache consistency back to the pre-PR behavior for that key. Worth emitting a metric so degradation is visible, not just stderr noise.
There was a problem hiding this comment.
Code Review
Security: No issues. No new injection vectors, key construction stays internal, no secrets logged.
Performance: One concern. The tombstone is written via cache->save() without a per-key TTL. utopia-php/cache stores the underlying Redis hash key indefinitely and only enforces TTL at load time, so tombstone entries leak slowly bounded by the number of unique document IDs ever purged. Same-document re-purges overwrite the entry, so hot workloads are fine; the leak is in the long tail. Acceptable trade-off for now but worth tracking.
Correctness: Found one critical issue (fixed in this push).
- [CRITICAL]
updateDocumentpost-commit purge missed the renamed-document case. The in-transaction code already double-purges when$document->getId() !== $id, but the new post-commit purge only re-purged the original$id, leaving the same race window the PR aims to close for renamed IDs. - [WARNING, noted]
PURGE_TOMBSTONE_TTL = 2seconds is short. If a reader'sgetDocument()(DB fetch + casting + permission checks + relationship walking) exceeds 2s, the tombstone expires before the reader's check and a stale write can land. The post-commit purge mitigates this since it runs again after commit, but slow readers can still slip through. Not changed in this push because lengthening the TTL extends the no-cache window for every purge; flagging for the author's judgment. - [WARNING, noted] The tombstone check + save is non-atomic (TOCTOU). A reader that passes the check just before the writer writes the tombstone can still poison the cache. The second post-commit purge is what actually closes most of this — the tombstone narrows but doesn't fully eliminate the window.
Readability: One improvement applied. The tombstone mechanism was non-obvious at the call sites. Extracted markPurgeTombstone() / isPurgeTombstoneActive() and added a comment in purgeCachedDocumentInternal explaining the ordering (tombstone before purge so concurrent readers see it).
Maintainability: Same improvement — call sites in getDocument() and purgeCachedDocumentInternal() no longer duplicate the :purged suffix concatenation. Future TTL or key-format changes happen in one place.
Testing: No tests added. The race is concurrency-dependent and hard to assert deterministically in PHPUnit. Existing integration coverage exercises the update/delete paths, but explicit verification of the tombstone behavior would be valuable — recommend a follow-up that injects a fake Cache to assert getDocument() skips save() when a tombstone is present.
Changes pushed in this review
- Added a post-commit purge for
$document->getId()when it differs from the requested$idinupdateDocument. - Extracted tombstone logic into
markPurgeTombstone()andisPurgeTombstoneActive()private helpers. - Added an inline comment in
purgeCachedDocumentInternaldescribing the tombstone-before-purge ordering.
| // Cache | ||
| public const TTL = 60 * 60 * 24; // 24 hours | ||
|
|
||
| public const PURGE_TOMBSTONE_TTL = 2; // seconds |
There was a problem hiding this comment.
[WARNING] PURGE_TOMBSTONE_TTL = 2 seconds is aggressive. If getDocument() exceeds 2s (relationship resolution, large payloads, slow DB) the tombstone expires before the reader's check and a stale write can land. The post-commit purge then cleans it up, but only if it runs after the reader's save. Consider raising to ~10-15s to widen the safety net; the trade-off is that the cache stays cold for that window after each purge.
|
|
||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| try { | ||
| $this->cache->save($tombstoneKey, '1', $tombstoneKey); |
There was a problem hiding this comment.
[WARNING] cache->save() here doesn't take a per-key TTL; utopia-php/cache enforces TTL at load time only, so the underlying Redis hash key for the tombstone is never auto-evicted. Same-document re-purges overwrite the entry, so hot workloads are fine, but cold documents leak one Redis key each. A future cleanup could expose a save-with-ttl adapter call so tombstones evict themselves.
| $this->cache->save($documentKey, $document->getArrayCopy(), $hashKey); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); | ||
| $tombstoneKey = $documentKey . self::PURGE_TOMBSTONE_SUFFIX; | ||
| $purged = $this->cache->load($tombstoneKey, self::PURGE_TOMBSTONE_TTL, $tombstoneKey); |
There was a problem hiding this comment.
[WARNING] Tombstone check + save is non-atomic (TOCTOU). A reader that passes the check just before the writer writes the tombstone can still poison the cache; the post-commit purge then has to clean up. The fix in this PR is correct but worth noting — the post-commit purge is what actually closes most of the window, the tombstone just narrows it.
|
Claude pushed fixes from: improvement |
|
Claude pushed fixes from: improvement |
The purge inside updateDocument and deleteDocument runs within the transaction, before the write is visible. A concurrent reader landing between that purge and the commit reads the old row and re-caches it, and with no purge following, the stale version is served for up to 24 hours. Purging again after the transaction closes that window.
ChiragAgg5k
force-pushed
the
fix-document-cache-purge-race
branch
from
453c103 to
633b776
Compare
ChiragAgg5k
changed the title
There was a problem hiding this comment.
Code Review
Reviewed the local branch diff (3 commits ahead of the current PR HEAD on GitHub) that adds the purge-tombstone mechanism on top of the post-commit re-purge. Two issues found and fixed in this run.
Findings
[CRITICAL] Duplicate post-commit purge block in updateDocument — src/Database/Database.php (post-commit section after withTransaction)
The if ($document->getId() !== $id) { $this->purgeCachedDocumentInternal(...) } block was emitted twice in a row, a leftover from the cross-commit refactor in 453c103b. It re-purges the renamed-id key a second time with no benefit and is confusing to readers.
Fix: removed the duplicate block.
[WARNING] DRY violation in purgeCachedCollection — src/Database/Database.php (tombstone marking inside the foreach loop)
The try { $this->cache->save($tombstoneKey, '1', $tombstoneKey); } catch (...) { Console::warning(...) } block inlined the exact logic that was just extracted into markPurgeTombstone() in 453c103b. Two copies of the same key-derivation + try/catch pattern means future tweaks (e.g. value, error handling, TTL field) must be made in both places.
Fix: route through markPurgeTombstone($documentKey).
Per-dimension Summary
- Security: no findings.
- Performance: no findings. The duplicate purge was wasteful (extra cache RTT for renamed IDs) — fixed alongside the correctness issue.
- Correctness: 1 issue (duplicate block — fixed).
- Readability: covered by the duplicate-removal fix.
- Maintainability: 1 issue (purge tombstone helper not used in
purgeCachedCollection— fixed). - Testing: no automated tests cover the new tombstone behavior (TTL expiry, concurrent-reader skip, renamed-id case). Not regressing existing coverage, but worth adding follow-up tests for the tombstone window.
Notes
- Local branch HEAD diverges from the GitHub PR HEAD (
633b776b) which was force-pushed without the tombstone code. The findings/fixes apply to the local commits (35d6eb4c,ff5e7842,453c103b) that the orchestrator will push at the end of this run.
|
Claude pushed fixes from: improvement |
ChiragAgg5k
force-pushed
the
fix-document-cache-purge-race
branch
from
ad4b93c to
633b776
Compare
2 participants
What does this PR do?
updateDocumentanddeleteDocumentpurge the document cache inside the transaction, before the write is visible. A reader landing between the purge and the commit reads the old row and re-caches it — and with no purge following, the stale version is served for up to 24 hours (Database::TTL).sequenceDiagram participant R as Reader (getDocument) participant C as Cache participant DB as Database participant W as Writer (updateDocument) W->>DB: write version N+1 (in txn, not yet visible) W->>C: purge(doc) R->>DB: read document → version N (txn not committed) R->>C: save(doc, version N) ⚠️ W->>DB: commit Note over C: stale version N cached, no purge followsThe fix is one purge per write path, after the transaction commits, so any entry cached during the purge→commit window is evicted.
It also helps with a related race we hit as a recurring Appwrite CI flake (example failure): a reader whose
cache->save()lands after the writer's purge. The post-commit purge adds a later eviction point that narrows that window, though a slow enough save can still land after it — fully closing it needs reader-side cooperation (e.g. a purge tombstone) and is left for a follow-up.Cost: one extra cache purge per single-document write. Reads are untouched.
Test plan
php -l+ Pint pass.Summary by CodeRabbit
Bug Fixes