feat(database): use cache lease in getDocument to fix read-after-write staleness#904
feat(database): use cache lease in getDocument to fix read-after-write staleness#904abnegate wants to merge 1 commit into
Conversation
…write staleness getDocument() re-populates the cache after a miss. Under concurrency a reader that fetched a row just before a concurrent updateDocument purge could write that now-stale row back into the cache *after* the purge ran, so later reads served stale data until the next write. Capture the cache generation before the adapter read and persist the result via saveWithLease(): the write only lands if no purge advanced the generation in the meantime, otherwise it is rejected and the next read re-fetches. forUpdate reads still bypass the cache entirely. Requires utopia-php/cache's Leasable capability (getGeneration/saveWithLease, utopia-php/cache#75); non-leasable cache adapters fall back to an unconditional save, so behaviour is unchanged for them. NOTE: composer is temporarily pinned to the cache feature branch so CI can resolve the new methods; revert to the released ^3.1 once cache#75 ships. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesGeneration-aware cache writes in getDocument()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 |
![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
🧹 Nitpick comments (1)
src/Database/Database.php (1)
4940-4941: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winOnly update the collection cache index after a successful lease save.
saveWithLease()returnsfalsewhen a concurrent purge invalidates the generation, but Line 4941 still writes the collection reference. Gate that follow-up write on!== falseso a rejected lease does not repopulate cache bookkeeping after the purge.Proposed fix
- $this->cache->saveWithLease($documentKey, $document->getArrayCopy(), $hashKey, $generation); - $this->cache->save($collectionKey, 'empty', $documentKey); + $saved = $this->cache->saveWithLease($documentKey, $document->getArrayCopy(), $hashKey, $generation); + if ($saved !== false) { + $this->cache->save($collectionKey, 'empty', $documentKey); + }🤖 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 4940 - 4941, The cache update for the collection index on line 4941 executes unconditionally, but it should only execute when the preceding saveWithLease() call on line 4940 succeeds. Capture the return value of saveWithLease() into a variable and wrap the subsequent cache->save() call in a condition that only executes when saveWithLease() does not return false, ensuring that rejected leases due to concurrent purges do not repopulate the collection cache bookkeeping.
🤖 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 `@composer.json`:
- Line 49: The "utopia-php/cache" dependency on line 49 is pinned to a moving
development branch (dev-feat/leasable-cache as 3.1.0) which creates
non-deterministic builds since the branch can move over time. Replace the branch
alias with a specific commit SHA to lock the exact version being used. Once the
stable 3.1 release is published on Packagist, update the dependency to use the
version constraint "^3.1" instead of the commit hash.
In `@src/Database/Database.php`:
- Around line 4882-4886: Wrap the $generation assignment that calls
getGeneration() on line 4886 in a try/catch block to match the error handling
pattern used in load() and saveWithLease() methods. Initialize $generation to
null in the catch block so that when getGeneration() throws during a cache
outage, the code gracefully falls back to database operations. Then, before
passing $generation to saveWithLease(), add a check to ensure $generation !==
null so that the cache save operation is only executed when generation was
successfully captured.
---
Nitpick comments:
In `@src/Database/Database.php`:
- Around line 4940-4941: The cache update for the collection index on line 4941
executes unconditionally, but it should only execute when the preceding
saveWithLease() call on line 4940 succeeds. Capture the return value of
saveWithLease() into a variable and wrap the subsequent cache->save() call in a
condition that only executes when saveWithLease() does not return false,
ensuring that rejected leases due to concurrent purges do not repopulate the
collection cache bookkeeping.
🪄 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: 4201ccf9-ef4a-4312-9faa-88734fc4abc2
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
composer.jsonsrc/Database/Database.php
| "utopia-php/validators": "0.2.*", | ||
| "utopia-php/console": "0.1.*", | ||
| "utopia-php/cache": "^3.0", | ||
| "utopia-php/cache": "dev-feat/leasable-cache as 3.1.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current constraint in composer.json:"
jq -r '.require["utopia-php/cache"]' composer.json
echo
echo "Check if stable 3.1 versions are available on Packagist:"
curl -s https://repo.packagist.org/p2/utopia-php/cache.json \
| jq -r '.packages["utopia-php/cache"][].version' \
| grep -E '^3\.1(\.|$)' || true
echo
echo "If no stable 3.1 appears above, keep temporary branch but pin an explicit commit SHA."Repository: utopia-php/database
Length of output: 278
Pin this dependency to a commit hash until 3.1 releases.
Line 49 references a moving branch (dev-feat/leasable-cache as 3.1.0). Since no stable 3.1 release exists on Packagist yet, downstream consumers of this library could resolve different commits over time, introducing non-deterministic builds. Add an explicit commit SHA to stabilize the dependency:
- "utopia-php/cache": "dev-feat/leasable-cache as 3.1.0",
+ "utopia-php/cache": "dev-feat/leasable-cache#<commit-sha> as 3.1.0",Switch to ^3.1 once the stable version is released.
🤖 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 `@composer.json` at line 49, The "utopia-php/cache" dependency on line 49 is
pinned to a moving development branch (dev-feat/leasable-cache as 3.1.0) which
creates non-deterministic builds since the branch can move over time. Replace
the branch alias with a specific commit SHA to lock the exact version being
used. Once the stable 3.1 release is published on Packagist, update the
dependency to use the version constraint "^3.1" instead of the commit hash.
| // Capture the cache generation BEFORE reading the row. If a concurrent | ||
| // updateDocument purges (and so advances the generation) while we read, | ||
| // saveWithLease() below rejects this now-stale value instead of | ||
| // re-poisoning the cache. See Cache\Feature\Leasable. | ||
| $generation = $forUpdate ? '0' : $this->cache->getGeneration($documentKey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and locate the specific lines
wc -l src/Database/Database.phpRepository: utopia-php/database
Length of output: 95
🏁 Script executed:
# Read the lines mentioned in the review: 4882-4887 for the generation capture
sed -n '4880,4890p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 547
🏁 Script executed:
# Read the load() error handling section: 4848-4852
sed -n '4845,4860p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 802
🏁 Script executed:
# Read the saveWithLease() section: around line 4940
sed -n '4930,4950p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 996
🏁 Script executed:
# Also check the broader context to understand $forUpdate and the full flow
sed -n '4840,4860p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 995
🏁 Script executed:
# Find the Cache class and examine getGeneration() method
fd -t f 'Cache\.php' src/Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for getGeneration method definition
rg 'function getGeneration' -A 10 src/Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Also check if there's a Leasable feature that might affect this
fd -t f 'Leasable' src/Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Search for Cache class definition
rg 'class Cache' src/Repository: utopia-php/database
Length of output: 45
🏁 Script executed:
# Look for the Leasable feature mentioned in the comment
rg 'Leasable' src/ -A 5 -B 5Repository: utopia-php/database
Length of output: 833
🏁 Script executed:
# Search for getGeneration more broadly
rg 'getGeneration' src/ -B 3 -A 3Repository: utopia-php/database
Length of output: 621
🏁 Script executed:
# Get a broader view of the cache save logic and how $generation is used
sed -n '4930,4950p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 996
🏁 Script executed:
# Check if there's a composer.json to see the cache dependency
cat composer.json 2>/dev/null || echo "No composer.json in root"Repository: utopia-php/database
Length of output: 2491
🏁 Script executed:
# Look at the full function signature around the generation capture
sed -n '4820,4895p' src/Database/Database.phpRepository: utopia-php/database
Length of output: 3187
🏁 Script executed:
# Look at the function name and any other cache initialization
sed -n '4700,4750p' src/Database/Database.php | head -40Repository: utopia-php/database
Length of output: 1330
🏁 Script executed:
# Search for the function definition to understand the signature
rg 'public function getDocument' -A 15 src/Database/Database.php | head -30Repository: utopia-php/database
Length of output: 932
🏁 Script executed:
# Verify the composer.json shows cache dependency and version
grep -A 1 "utopia-php/cache" composer.jsonRepository: utopia-php/database
Length of output: 232
Wrap getGeneration() in try/catch to match cache operation error handling.
Line 4886 calls getGeneration() without error handling, while load() (lines 4848–4852) and saveWithLease() (lines 4940–4945) are both wrapped in try/catch blocks as best-effort cache operations. If getGeneration() throws during a cache outage, the document read fails before the database fallback occurs, degrading availability.
Gate the cache save on successful generation capture by wrapping the call in try/catch and initializing to null, then checking $generation !== null before passing it to saveWithLease().
Proposed fix
- $generation = $forUpdate ? '0' : $this->cache->getGeneration($documentKey);
+ $generation = null;
+ if (!$forUpdate) {
+ try {
+ $generation = $this->cache->getGeneration($documentKey);
+ } catch (Exception $e) {
+ Console::warning('Warning: Failed to get document cache generation: ' . $e->getMessage());
+ }
+ }Then gate the later save:
- if (!$forUpdate && empty($relationships)) {
+ if (!$forUpdate && empty($relationships) && $generation !== null) {🤖 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 4882 - 4886, Wrap the $generation
assignment that calls getGeneration() on line 4886 in a try/catch block to match
the error handling pattern used in load() and saveWithLease() methods.
Initialize $generation to null in the catch block so that when getGeneration()
throws during a cache outage, the code gracefully falls back to database
operations. Then, before passing $generation to saveWithLease(), add a check to
ensure $generation !== null so that the cache save operation is only executed
when generation was successfully captured.
Greptile SummaryThis PR addresses a read-after-write cache staleness bug in
Confidence Score: 3/5Not safe to merge: the cache pin must be replaced with a stable tag, and an unguarded cache call can turn transient backend failures into broken document reads. The src/Database/Database.php (unguarded getGeneration call) and composer.json (branch pin that must be reverted before merge) Important Files Changed
Reviews (1): Last reviewed commit: "feat(database): use the cache lease in g..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Capture the cache generation BEFORE reading the row. If a concurrent | ||
| // updateDocument purges (and so advances the generation) while we read, | ||
| // saveWithLease() below rejects this now-stale value instead of | ||
| // re-poisoning the cache. See Cache\Feature\Leasable. | ||
| $generation = $forUpdate ? '0' : $this->cache->getGeneration($documentKey); |
There was a problem hiding this comment.
getGeneration() is called outside any try/catch block. Every other cache interaction in this method (load() on line 4849, saveWithLease()/save() on lines 4940–4941) is guarded. If the cache backend is temporarily unavailable (e.g., Redis connection error) and getGeneration() throws, the exception propagates out of getDocument() entirely — a regression from the pre-PR behaviour where cache failures were logged and reads fell through to the database.
| // Capture the cache generation BEFORE reading the row. If a concurrent | |
| // updateDocument purges (and so advances the generation) while we read, | |
| // saveWithLease() below rejects this now-stale value instead of | |
| // re-poisoning the cache. See Cache\Feature\Leasable. | |
| $generation = $forUpdate ? '0' : $this->cache->getGeneration($documentKey); | |
| // Capture the cache generation BEFORE reading the row. If a concurrent | |
| // updateDocument purges (and so advances the generation) while we read, | |
| // saveWithLease() below rejects this now-stale value instead of | |
| // re-poisoning the cache. See Cache\Feature\Leasable. | |
| $generation = '0'; | |
| if (!$forUpdate) { | |
| try { | |
| $generation = $this->cache->getGeneration($documentKey); | |
| } catch (Exception $e) { | |
| Console::warning('Warning: Failed to get cache generation: ' . $e->getMessage()); | |
| } | |
| } |
| $this->cache->saveWithLease($documentKey, $document->getArrayCopy(), $hashKey, $generation); | ||
| $this->cache->save($collectionKey, 'empty', $documentKey); |
There was a problem hiding this comment.
saveWithLease() is rejected (CAS mismatch — the stale-data case this PR targets), it returns false rather than throwing, so the subsequent save($collectionKey, …) still executes and registers the document key in the collection's invalidation index even though the document itself was never written to the cache. On the next collection-level purge this creates a phantom entry that is purged harmlessly, but it makes the tracking index transiently inconsistent. Gating the collection-key save on a truthy return from saveWithLease keeps the two in sync.
| $this->cache->saveWithLease($documentKey, $document->getArrayCopy(), $hashKey, $generation); | |
| $this->cache->save($collectionKey, 'empty', $documentKey); | |
| if ($this->cache->saveWithLease($documentKey, $document->getArrayCopy(), $hashKey, $generation)) { | |
| $this->cache->save($collectionKey, 'empty', $documentKey); | |
| } |
| @@ -40,7 +46,7 @@ | |||
| "ext-redis": "*", | |||
| "utopia-php/validators": "0.2.*", | |||
There was a problem hiding this comment.
main — composer.json still points at dev-feat/leasable-cache as 3.1.0. As noted in the PR description, this needs to be reverted to ^3.1 (or whatever tag utopia-php/cache#75 ships as) before this PR can merge, otherwise any environment running composer install will pull from an unversioned feature branch rather than a stable release.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Problem
getDocument()re-populates the cache after a miss. Under concurrency, a reader that fetched a row from the database just before a concurrentupdateDocument()purge can write that now-stale row back into the cache after the purge runs — so subsequent reads serve stale data until the next write. The post-commit purge can't prevent it: the stale reader'ssave()lands after the purge.Observed downstream as an intermittent read-after-write failure (a just-disabled project service still appearing enabled ~1.7% of the time under concurrent load).
Fix
Capture the cache generation before the adapter read, and persist via
saveWithLease():If a purge advanced the generation while we were reading, the write is rejected (CAS) and the next read re-fetches — instead of caching stale data.
forUpdatereads still bypass the cache entirely.Non-leasable cache adapters fall back to an unconditional
save()via theCachefacade, so behaviour is unchanged for them.Dependency / ordering
Requires the
Leasablecapability added in utopia-php/cache#75 (getGeneration()/saveWithLease()).composer.jsonis temporarily pinned to the cache feature branch (dev-feat/leasable-cache) so CI can resolve the new methods. Revert to the released^3.1once cache#75 is merged and tagged, then this can merge.Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores