Skip to content

feat(database): use cache lease in getDocument to fix read-after-write staleness #904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abnegate wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −13
Open

feat(database): use cache lease in getDocument to fix read-after-write staleness #904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@
"check": "./vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G",
"coverage": "./vendor/bin/coverage-check ./tmp/clover.xml 90"
},
"repositories": [
{
"type": "vcs",
"url": "https://github.com/utopia-php/cache"
}
],
"require": {
"php": ">=8.4",
"ext-pdo": "*",
Expand All @@ -40,7 +46,7 @@
"ext-redis": "*",
"utopia-php/validators": "0.2.*",

@greptile-apps greptile-apps Bot Jun 22, 2026

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.

P1 Temporary branch pin must not reach 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!

"utopia-php/console": "0.1.*",
"utopia-php/cache": "^3.0",
"utopia-php/cache": "dev-feat/leasable-cache as 3.1.0",

@coderabbitai coderabbitai Bot Jun 22, 2026

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.

⚠️ Potential issue | 🟠 Major

🧩 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.

"utopia-php/pools": "1.*",
"utopia-php/mongo": "1.*"
},
Expand Down
46 changes: 35 additions & 11 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4879,6 +4879,12 @@ public function getDocument(string $collection, string $id, array $queries = [],
return $document;
}

// 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);
Comment on lines +4882 to +4886

@coderabbitai coderabbitai Bot Jun 22, 2026

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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check the file structure and locate the specific lines
wc -l src/Database/Database.php

Repository: 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.php

Repository: utopia-php/database

Length of output: 547

🏁 Script executed:

# Read the load() error handling section: 4848-4852
sed -n '4845,4860p' src/Database/Database.php

Repository: utopia-php/database

Length of output: 802

🏁 Script executed:

# Read the saveWithLease() section: around line 4940
sed -n '4930,4950p' src/Database/Database.php

Repository: 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.php

Repository: 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 5

Repository: utopia-php/database

Length of output: 833

🏁 Script executed:

# Search for getGeneration more broadly
rg 'getGeneration' src/ -B 3 -A 3

Repository: 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.php

Repository: 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.php

Repository: 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 -40

Repository: 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 -30

Repository: 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.json

Repository: 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.

Comment on lines +4882 to +4886

@greptile-apps greptile-apps Bot Jun 22, 2026

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.

P1 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.

Suggested change
// 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());
}
}


$document = $this->adapter->getDocument(
$collection,
$id,
Expand Down Expand Up @@ -4931,7 +4937,7 @@ public function getDocument(string $collection, string $id, array $queries = [],
// caching the pre-commit row would poison the cache for other readers.
if (!$forUpdate && empty($relationships)) {
try {
$this->cache->save($documentKey, $document->getArrayCopy(), $hashKey);
$this->cache->saveWithLease($documentKey, $document->getArrayCopy(), $hashKey, $generation);
$this->cache->save($collectionKey, 'empty', $documentKey);
Comment on lines +4940 to 4941

@greptile-apps greptile-apps Bot Jun 22, 2026

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.

P2 When 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.

Suggested change
$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);
}

} catch (Exception $e) {
Console::warning('Failed to save document to cache: ' . $e->getMessage());
Expand Down
Loading