From ef47859a5deb99af18da81af65b258c3ca86c4ac Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 11 Jun 2026 13:05:11 +1200 Subject: [PATCH 1/2] fix: bypass cache for locking document reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A forUpdate read is meant to observe the current row under lock, but it was served from the document cache when an entry existed. updateDocument merges its changes into that read and writes the result back, so a stale cache entry durably reverted attributes that were updated since the entry was cached — the lost-update behind flaky project config E2E tests. A locking read now always reads through to the adapter, and skips the cache save: it runs inside an open transaction, and caching the pre-commit row would poison the cache for other readers. Co-Authored-By: Claude Fable 5 --- src/Database/Database.php | 21 ++++-- tests/unit/ForUpdateCacheTest.php | 104 ++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 7 deletions(-) create mode 100644 tests/unit/ForUpdateCacheTest.php diff --git a/src/Database/Database.php b/src/Database/Database.php index 1a3a2adf0..158d94274 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -4840,11 +4840,16 @@ public function getDocument(string $collection, string $id, array $queries = [], $selections ); - try { - $cached = $this->cache->load($documentKey, self::TTL, $hashKey); - } catch (Exception $e) { - Console::warning('Warning: Failed to get document from cache: ' . $e->getMessage()); - $cached = null; + // A locking read must observe the current row, not a cached copy: + // updateDocument merges the changes into this read and writes the result + // back, so serving it from a stale cache would persist the staleness. + $cached = null; + if (!$forUpdate) { + try { + $cached = $this->cache->load($documentKey, self::TTL, $hashKey); + } catch (Exception $e) { + Console::warning('Warning: Failed to get document from cache: ' . $e->getMessage()); + } } if ($cached) { @@ -4916,8 +4921,10 @@ public function getDocument(string $collection, string $id, array $queries = [], fn ($attribute) => $attribute['type'] === Database::VAR_RELATIONSHIP ); - // Don't save to cache if it's part of a relationship - if (empty($relationships)) { + // Don't save to cache if it's part of a relationship, or if this is a + // locking read: a forUpdate read happens inside an open transaction, and + // 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->save($collectionKey, 'empty', $documentKey); diff --git a/tests/unit/ForUpdateCacheTest.php b/tests/unit/ForUpdateCacheTest.php new file mode 100644 index 000000000..aef2736b3 --- /dev/null +++ b/tests/unit/ForUpdateCacheTest.php @@ -0,0 +1,104 @@ +adapter = new DatabaseMemory(); + $this->database = new Database($this->adapter, new Cache(new CacheMemory())); + $this->database + ->setDatabase('utopiaTests') + ->setNamespace('for_update_' . \uniqid()); + + $this->database->create(); + $this->database->createCollection('projects'); + $this->database->createAttribute('projects', 'name', Database::VAR_STRING, 255, false); + $this->database->createAttribute('projects', 'description', Database::VAR_STRING, 255, false); + $this->database->createDocument('projects', new Document([ + '$id' => 'project', + '$permissions' => [ + Permission::read(Role::any()), + Permission::update(Role::any()), + ], + 'name' => 'stale', + 'description' => 'stale', + ])); + } + + /** + * Write to the row through the adapter, bypassing Database and therefore + * the cache purge — the cache now holds a stale copy, exactly as if a + * concurrent reader had re-cached the old row after a writer's purge. + */ + private function staleCache(string $attribute, string $value): void + { + $collection = $this->database->getCollection('projects'); + $document = $this->adapter->getDocument($collection, 'project'); + $document->setAttribute($attribute, $value); + $this->adapter->updateDocument($collection, 'project', $document, true); + } + + public function testForUpdateReadBypassesStaleCache(): void + { + $cached = $this->database->getDocument('projects', 'project'); + $this->assertSame('stale', $cached->getAttribute('name')); + + $this->staleCache('name', 'fresh'); + + $cached = $this->database->getDocument('projects', 'project'); + $this->assertSame('stale', $cached->getAttribute('name')); + + $fresh = $this->database->getDocument('projects', 'project', forUpdate: true); + $this->assertSame('fresh', $fresh->getAttribute('name')); + } + + public function testForUpdateReadDoesNotPopulateCache(): void + { + $this->database->getDocument('projects', 'project'); + $this->staleCache('name', 'fresh'); + + $this->database->getDocument('projects', 'project', forUpdate: true); + + // The locking read happens inside an open transaction; caching what it + // saw would publish a pre-commit row. The cached copy must be untouched. + $cached = $this->database->getDocument('projects', 'project'); + $this->assertSame('stale', $cached->getAttribute('name')); + } + + public function testUpdateDocumentDoesNotResurrectStaleCachedAttributes(): void + { + $this->database->getDocument('projects', 'project'); + $this->staleCache('name', 'fresh'); + + // updateDocument merges the changes into its locking read of the current + // row. Served from the stale cache, that merge would durably revert + // name to 'stale' — the lost-update behind flaky project config tests. + $this->database->updateDocument('projects', 'project', new Document([ + 'description' => 'updated', + ])); + + $collection = $this->database->getCollection('projects'); + $row = $this->adapter->getDocument($collection, 'project'); + $this->assertSame('fresh', $row->getAttribute('name')); + $this->assertSame('updated', $row->getAttribute('description')); + + $document = $this->database->getDocument('projects', 'project'); + $this->assertSame('fresh', $document->getAttribute('name')); + $this->assertSame('updated', $document->getAttribute('description')); + } +} From 8e59397fb0748053a53ae1114e7500fed67c574e Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 11 Jun 2026 13:33:34 +1200 Subject: [PATCH 2/2] fix: re-cast cache hits so cached and fresh reads compare equal The Redis cache envelope serializes through JSON, which collapses floats with zero fractions to ints. With forUpdate reads now coming from the adapter, updateDocument compared a caller document built from a cached read (int) against a freshly-typed old document (float): strict inequality flipped the no-op detection into requiring update permission on read-only documents (testEmptyTenant). Re-running casting on the cache-hit path restores type fidelity for both sources. Co-Authored-By: Claude Fable 5 --- src/Database/Database.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Database/Database.php b/src/Database/Database.php index 158d94274..37b903d13 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -4855,6 +4855,11 @@ public function getDocument(string $collection, string $id, array $queries = [], if ($cached) { $document = $this->createDocumentInstance($collection->getId(), $cached); + // JSON serialization in cache backends collapses floats with zero + // fractions to ints. Re-cast so cached and freshly-loaded documents + // compare equal under strict equality (e.g. in updateDocument). + $document = $this->casting($collection, $document); + if ($collection->getId() !== self::METADATA) { if (!$this->authorization->isValid(new Input(self::PERMISSION_READ, [