diff --git a/src/Database/Database.php b/src/Database/Database.php index 1a3a2adf0..37b903d13 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -4840,16 +4840,26 @@ 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) { $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, [ @@ -4916,8 +4926,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')); + } +}