diff --git a/src/Database/Adapter/Mongo.php b/src/Database/Adapter/Mongo.php index e26e14a55..c52915946 100644 --- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ -479,7 +479,10 @@ public function createCollection(string $name, array $attributes = [], array $in } catch (MongoException $e) { $e = $this->processException($e); if ($e instanceof DuplicateException) { - return true; + if ($this->getSharedTables() || $name === Database::METADATA) { + return true; + } + throw $e; } // Client throws code-0 "Collection Exists" when its pre-check // finds the collection. In shared-tables/metadata context this diff --git a/src/Database/Database.php b/src/Database/Database.php index 760c1aeaf..9695708f5 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -1908,18 +1908,19 @@ public function createCollection(string $id, array $attributes = [], array $inde // tenants. A DuplicateException simply means the table already // exists for another tenant — not an orphan. } else { - // Metadata check (above) already verified collection is absent - // from metadata. A DuplicateException from the adapter means - // the collection exists only in physical schema — an orphan - // from a prior partial failure. Drop and recreate to ensure - // schema matches. + // The table exists and this process did not create it. It may + // belong to a peer that has not committed metadata yet, or it + // may be an orphan. Dropping it destroyed live collections + // during concurrent boot; attaching this caller's metadata to + // an unknown physical schema can invent columns that are not + // there. Leave the table and report Duplicate. Claiming the + // metadata row first is #939. try { - $this->adapter->deleteCollection($id); - } catch (NotFoundException) { - // Already removed by a concurrent reconciler. + $this->purgeCachedDocument(self::METADATA, $id); + } catch (\Throwable $cacheError) { + Console::warning('Warning: Failed to purge stale collection cache: ' . $cacheError->getMessage()); } - $this->adapter->createCollection($id, $attributes, $indexes); - $createdPhysicalTable = true; + throw new DuplicateException('Collection ' . $id . ' already exists', previous: $e); } } @@ -1929,6 +1930,16 @@ public function createCollection(string $id, array $attributes = [], array $inde try { $createdCollection = $this->silent(fn () => $this->createDocument(self::METADATA, $collection)); + } catch (DuplicateException $e) { + // A concurrent creator committed the metadata for this id first, so + // the physical table is the one its metadata describes. Rolling back + // here would drop a live collection out from under it. + try { + $this->purgeCachedDocument(self::METADATA, $id); + } catch (\Throwable $cacheError) { + Console::warning('Warning: Failed to purge stale collection cache: ' . $cacheError->getMessage()); + } + throw new DuplicateException('Collection ' . $id . ' already exists', previous: $e); } catch (\Throwable $e) { if ($createdPhysicalTable) { try { diff --git a/tests/e2e/Adapter/Scopes/CollectionTests.php b/tests/e2e/Adapter/Scopes/CollectionTests.php index 1cbebd1db..bcbfbe91a 100644 --- a/tests/e2e/Adapter/Scopes/CollectionTests.php +++ b/tests/e2e/Adapter/Scopes/CollectionTests.php @@ -3,6 +3,8 @@ namespace Tests\E2E\Adapter\Scopes; use Exception; +use Utopia\Cache\Adapter\None as NoneCache; +use Utopia\Cache\Cache; use Utopia\Database\Adapter\SQL; use Utopia\Database\Database; use Utopia\Database\Document; @@ -1842,4 +1844,145 @@ public function testCreateCollectionWithLongId(): void $this->assertTrue($database->deleteCollection($collection)); } + + /** + * Two processes reconciling the same schema race: one reads the collection + * as missing, a peer creates it and commits, and only then does the first + * process try to create it. The loser must not mistake the peer's table for + * an orphan and drop it. + */ + public function testCreateCollectionConcurrentlyKeepsPeerData(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + + $collection = 'concurrentCreate'; + + // A peer process: same database, its own cache, so its writes do not + // purge the negative cache entry this process is about to record. + $peer = (new Database($database->getAdapter(), new Cache(new NoneCache()))) + ->setAuthorization(self::$authorization); + + $this->assertTrue($database->getCollection($collection)->isEmpty()); + + $peer->createCollection($collection, [ + new Document([ + '$id' => ID::custom('name'), + 'type' => Database::VAR_STRING, + 'size' => 128, + 'required' => false, + ]), + ], permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + + $peer->createDocument($collection, new Document([ + '$id' => ID::custom('written'), + '$permissions' => [Permission::read(Role::any())], + 'name' => 'peer', + ])); + + try { + $database->createCollection($collection, [ + new Document([ + '$id' => ID::custom('name'), + 'type' => Database::VAR_STRING, + 'size' => 128, + 'required' => false, + ]), + ], permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + $this->fail('Expected DuplicateException for a collection a peer already created'); + } catch (DuplicateException) { + } + + $survivor = $peer->getDocument($collection, 'written'); + $this->assertSame('peer', $survivor->getAttribute('name'), 'Peer document was destroyed by the losing creator'); + + $metadata = $peer->getCollection($collection); + $this->assertFalse($metadata->isEmpty(), 'Peer collection metadata was destroyed by the losing creator'); + + // The loser's cache still held the collection as missing from the read + // it took before the peer committed, and the peer's purge cannot reach + // this instance. Losing the race has to clear it, or the collection + // stays invisible here until the entry expires. + $this->assertFalse($database->getCollection($collection)->isEmpty(), 'Losing creator kept a stale empty collection cached'); + $this->assertSame('peer', $database->getDocument($collection, 'written')->getAttribute('name')); + + $this->assertTrue($database->deleteCollection($collection)); + } + + /** + * A physical collection with no metadata is indistinguishable from a peer + * that has created the table and not yet committed its metadata row. + * createCollection must leave that table alone. + */ + public function testCreateCollectionDoesNotDropUncommittedPeerTable(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + + if ($database->getAdapter()->getSharedTables()) { + $this->expectNotToPerformAssertions(); + + return; + } + + $collection = 'preCommitCreate'; + $name = new Document([ + '$id' => ID::custom('name'), + 'type' => Database::VAR_STRING, + 'size' => 128, + 'required' => false, + ]); + + $database->getAdapter()->createCollection($collection, [$name], []); + + $schema = new Document([ + '$id' => $collection, + '$collection' => Database::METADATA, + 'name' => $collection, + 'attributes' => [$name], + 'indexes' => [], + 'documentSecurity' => true, + '$permissions' => [ + Permission::read(Role::any()), + Permission::create(Role::any()), + Permission::update(Role::any()), + Permission::delete(Role::any()), + ], + ]); + + $database->getAdapter()->createDocument($schema, new Document([ + '$id' => ID::custom('written'), + '$permissions' => [Permission::read(Role::any())], + 'name' => 'peer', + ])); + + try { + $database->createCollection($collection, [$name], permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + } catch (DuplicateException) { + // SQL adapters report the existing table as Duplicate. Mongo's + // createCollection is idempotent, so this process continues and + // claims metadata. Either way the physical collection must stay. + } + + $this->assertSame( + 'peer', + $database->getAdapter()->getDocument($schema, 'written')->getAttribute('name'), + 'Physical collection was dropped while metadata was still uncommitted' + ); + + try { + $database->deleteCollection($collection); + } catch (\Throwable) { + $database->getAdapter()->deleteCollection($collection); + } + } } diff --git a/tests/unit/CreateCollectionRaceTest.php b/tests/unit/CreateCollectionRaceTest.php new file mode 100644 index 000000000..6a4393802 --- /dev/null +++ b/tests/unit/CreateCollectionRaceTest.php @@ -0,0 +1,121 @@ +setDatabase('utopiaTests') + ->setNamespace('create_race_' . uniqid()); + $database->getAuthorization()->addRole(Role::any()->toString()); + $database->create(); + + $collection = 'preCommitCreate'; + $name = new Document([ + '$id' => ID::custom('name'), + 'type' => Database::VAR_STRING, + 'size' => 128, + 'required' => false, + ]); + + $adapter->createCollection($collection, [$name], []); + + $schema = new Document([ + '$id' => $collection, + '$collection' => Database::METADATA, + 'name' => $collection, + 'attributes' => [$name], + 'indexes' => [], + 'documentSecurity' => true, + '$permissions' => [ + Permission::read(Role::any()), + Permission::create(Role::any()), + Permission::update(Role::any()), + Permission::delete(Role::any()), + ], + ]); + + $adapter->createDocument($schema, new Document([ + '$id' => ID::custom('written'), + '$permissions' => [Permission::read(Role::any())], + 'name' => 'peer', + ])); + + try { + $database->createCollection($collection, [$name], permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + $this->fail('Expected DuplicateException for an existing physical collection'); + } catch (DuplicateException) { + } + + $this->assertSame( + 'peer', + $adapter->getDocument($schema, 'written')->getAttribute('name'), + 'Physical collection was dropped while metadata was still uncommitted' + ); + } + + public function testCreateCollectionStillReportsDuplicateWhenCachePurgeFails(): void + { + $cacheAdapter = new class () extends CacheMemory { + public bool $failPurge = false; + + public function purge(string $key, string $hash = ''): bool + { + if ($this->failPurge) { + throw new \RuntimeException('cache backend unavailable'); + } + + return parent::purge($key, $hash); + } + }; + + $adapter = new DatabaseMemory(); + $database = new Database($adapter, new Cache($cacheAdapter)); + $database + ->setDatabase('utopiaTests') + ->setNamespace('create_race_purge_' . uniqid()); + $database->getAuthorization()->addRole(Role::any()->toString()); + $database->create(); + + $collection = 'preCommitCreatePurgeFail'; + $name = new Document([ + '$id' => ID::custom('name'), + 'type' => Database::VAR_STRING, + 'size' => 128, + 'required' => false, + ]); + + $adapter->createCollection($collection, [$name], []); + + $cacheAdapter->failPurge = true; + + try { + $database->createCollection($collection, [$name], permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + $this->fail('Expected DuplicateException even when cache purge fails'); + } catch (DuplicateException $exception) { + $this->assertSame('Collection ' . $collection . ' already exists', $exception->getMessage()); + $this->assertInstanceOf(DuplicateException::class, $exception->getPrevious()); + } + } +}