Bypass cache for locking document reads

[abnegate]

What does this PR do?

getDocument(..., forUpdate: true) served the document from the cache when an entry existed. A locking read is meant to observe the current row under lock — updateDocument merges its changes into that read and writes the merged result back, so a stale cache entry durably reverted attributes updated since the entry was cached:

1. Writer A updates serviceUsers = false, purges the cache.
2. A concurrent reader re-caches the old row (the reader-side race noted in Purge document cache again after transaction commit #887).
3. Writer B updates an unrelated attribute. Its forUpdate read is served from the poisoned cache, the merge resurrects serviceUsers = true, and the adapter writes it back — the staleness is now durable in the database, surviving every later purge.

This is the mechanism behind the recurring ProjectsConsoleClientTest::testGetProject CI flake in Appwrite (project services / auths / smtp map fields reverting after a 200 PATCH; failing ~50–70% of runs in appwrite-labs/cloud since the race window widened).

The fix: a forUpdate read always reads through to the adapter, and skips the cache save — it runs inside an open transaction, so caching what it saw would publish a pre-commit row to other readers (the same poisoning #887 closed for the write path's own purge).

Replaces the core of #885 without the unrelated scope that stalled it.

Test plan

• tests/unit/ForUpdateCacheTest.php — three regression tests, all failing before the fix:
  • a forUpdate read bypasses a stale cache entry,
  • a forUpdate read does not (re)populate the cache,
  • updateDocument does not resurrect stale cached attributes into the database (the durable lost-update).
• Full unit suite green (354 tests). MySQL adapter e2e Cache|ForUpdate and testUpdateDocument|testCreateDocument slices green locally (incl. testCacheFallback).
• Pint + PHPStan level 7 green.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Tightened caching so locking (for-update) reads bypass stale cache and don't return pre-commit values.
  • Avoided populating cache with transient/pre-commit state and prevented cache-related lost updates during concurrent document changes.

• Tests

  • Added unit tests to verify cache coherency around locking reads, cache non-population during transactions, and correct merge behavior on updates.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 05ae4b10-0b25-4744-9572-d82238b2de6b

📥 Commits

Reviewing files that changed from the base of the PR and between ef47859 and 8e59397.

📒 Files selected for processing (1)

• src/Database/Database.php

🚧 Files skipped from review as they are similar to previous changes (1)

• src/Database/Database.php

---

📝 Walkthrough

Walkthrough

Cache load/save in Database::getDocument() is conditioned on non-locking reads so that locking reads (forUpdate) bypass cache and do not populate it; cached rows are cast before use. New tests validate stale-cache bypass, no cache pollution from locking reads, and safe merges during updates.

Changes

Cache coherency during locking reads

Layer / File(s)	Summary
getDocument() cache coherency during locking reads src/Database/Database.php	Cache loading is skipped when $forUpdate is true ($cached initialized to null), cached rows are passed through casting() on hit, and cache saving requires !$forUpdate and no relationship attributes to avoid caching pre-commit rows.
Cache coherency test suite tests/unit/ForUpdateCacheTest.php	Adds tests with in-memory DB/cache and a staleCache() adapter helper that leaves a stale cached row; tests verify: (1) forUpdate reads return fresh data, (2) forUpdate reads do not populate/overwrite the cache, (3) updateDocument merges without resurrecting stale cached attributes.

Sequence Diagram

sequenceDiagram
  participant Reader
  participant getDocument
  participant Cache
  participant Database
  Reader->>getDocument: getDocument(id, forUpdate: true)
  alt forUpdate true
    getDocument->>Database: read fresh row (locking read)
    Database-->>getDocument: fresh row
    getDocument-->>Reader: return fresh row
    Note over Cache: Cache not loaded, not saved
  else forUpdate false
    getDocument->>Cache: cache->load(id)
    alt cache hit
      Cache-->>getDocument: cached row
      getDocument->>getDocument: casting(cached row)
    else cache miss
      getDocument->>Database: read fresh row
      Database-->>getDocument: fresh row
      getDocument->>Cache: cache->save(fresh row)  // only if no relationships
    end
    getDocument-->>Reader: return row
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Possibly related PRs

• utopia-php/database#887: Both PRs modify Database::getDocument() cache behavior to prevent caching pre-commit/pre-update document states during concurrent updates.

Suggested reviewers

• ArnabChatterjee20k

Poem

🐰 I hopped through rows both new and old,

ForUpdate kept the cache on hold,
No poisoned row can trick my nose,
Fresh reads win where locking goes,
Hooray — safe hops and nested prose!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Bypass cache for locking document reads' directly summarizes the main change: cache bypass for forUpdate=true reads. It matches the primary objective of the PR.
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/for-update-cache-bypass

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR fixes a durable lost-update bug where getDocument(..., forUpdate: true) was served from cache instead of the live row. Because updateDocument merges its partial input into that read before writing back, a stale cache entry could silently resurrect overwritten attribute values in the database. The fix is a targeted two-guard change that skips both the cache load and the cache save whenever forUpdate is set.

• Cache bypass on locking reads (Database.php ~line 4847): the cache->load call is now skipped entirely when $forUpdate is true, ensuring the adapter always returns the current, locked row.
• Cache write suppression (Database.php ~line 4932): the cache->save call is also guarded by !$forUpdate, preventing a pre-commit row from poisoning the shared cache for other readers.
• Bundled fix for float normalisation in the cache-hit path: $this->casting() is now applied to documents returned from cache, aligning their types with freshly-loaded documents (pre-existing JSON float→int collapse).

Confidence Score: 5/5

Safe to merge — the change is minimal, directly addresses a well-understood lost-update bug, and is covered by three dedicated regression tests that were failing before the fix.

Both changed lines in Database.php are straightforward guard additions that skip existing operations under a single, well-understood condition. The three regression tests in ForUpdateCacheTest cover the read-bypass, the write-suppress, and the end-to-end updateDocument scenario that originally caused the CI flake. No existing behaviour changes outside the forUpdate code path.

No files require special attention.

Important Files Changed

Filename	Overview
src/Database/Database.php	Guards the cache load and cache save behind !$forUpdate; also adds a casting() call in the cache-hit path to normalise JSON-serialised floats. Logic is correct and tightly scoped.
tests/unit/ForUpdateCacheTest.php	Three targeted regression tests: bypass stale cache, no cache re-population, and no durable lost-update via updateDocument. All three were failing before the fix.

_{Reviews (2): Last reviewed commit: "fix: re-cast cache hits so cached and fr..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

tests/unit/ForUpdateCacheTest.php (1)

70-81: 💤 Low value

Consider asserting the forUpdate read result for test completeness.

The test correctly verifies that the forUpdate read does not populate the cache (line 80 asserts the cached value remains 'stale'). For completeness, consider also asserting that the forUpdate read itself returned 'fresh', confirming it bypassed the cache on read as well.

📋 Suggested enhancement

-        $this->database->getDocument('projects', 'project', forUpdate: true);
+        $fresh = $this->database->getDocument('projects', 'project', forUpdate: true);
+        $this->assertSame('fresh', $fresh->getAttribute('name'));
 
         // The locking read happens inside an open transaction; caching what it
         // saw would publish a pre-commit row. The cached copy must be untouched.

🤖 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 `@tests/unit/ForUpdateCacheTest.php` around lines 70 - 81, In
testForUpdateReadDoesNotPopulateCache, capture the result of the forUpdate read
(the call to $this->database->getDocument('projects', 'project', forUpdate:
true)) into a variable and add an assertion that its name attribute equals
'fresh' (e.g. assertSame('fresh', $result->getAttribute('name'))), so the test
verifies the for-update read returned the fresh value while subsequent
non-forUpdate reads still return the stale cached value; keep the existing
staleCache('name','fresh') and final assertSame('stale',
$cached->getAttribute('name')) intact.

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

Nitpick comments:
In `@tests/unit/ForUpdateCacheTest.php`:
- Around line 70-81: In testForUpdateReadDoesNotPopulateCache, capture the
result of the forUpdate read (the call to
$this->database->getDocument('projects', 'project', forUpdate: true)) into a
variable and add an assertion that its name attribute equals 'fresh' (e.g.
assertSame('fresh', $result->getAttribute('name'))), so the test verifies the
for-update read returned the fresh value while subsequent non-forUpdate reads
still return the stale cached value; keep the existing
staleCache('name','fresh') and final assertSame('stale',
$cached->getAttribute('name')) intact.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a4068665-a760-46ad-85cb-c04f268fd4c6

📥 Commits

Reviewing files that changed from the base of the PR and between 2ae2e6c and ef47859.

📒 Files selected for processing (2)

• src/Database/Database.php
• tests/unit/ForUpdateCacheTest.php