From cbe4a2615636194f11c0bd44a74edd7bd6af0433 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 6 Feb 2026 12:09:18 +0100 Subject: [PATCH 1/2] fix(propagator): Improve lock behavior of propagator Fix possible dead locks when running the propagator caused by two requests updating the same amount rows in transactions. - Lock rows always in the same deterministic order by sorting the path_hash first - On all database outside of sqlite, also do first a SELECT FOR UPDATE to lock all the rows used in batch UPDATE calls, afterward to decrease the risk of two requests trying to lock the same rows Signed-off-by: Carl Schwan (cherry picked from commit cae742d18243020fb3f3615883bf1fd1c6e3a524) --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DB/QueryBuilder/ExtendedQueryBuilder.php | 6 + lib/private/DB/QueryBuilder/QueryBuilder.php | 10 + lib/private/Files/Cache/Propagator.php | 191 ++++++++++-------- .../QueryBuilder/ConflictResolutionMode.php | 26 +++ lib/public/DB/QueryBuilder/IQueryBuilder.php | 8 + tests/lib/Files/Cache/PropagatorTest.php | 3 +- 8 files changed, 165 insertions(+), 81 deletions(-) create mode 100644 lib/public/DB/QueryBuilder/ConflictResolutionMode.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index b7209606baf32..aa548470e07d0 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -311,6 +311,7 @@ 'OCP\\DB\\IPreparedStatement' => $baseDir . '/lib/public/DB/IPreparedStatement.php', 'OCP\\DB\\IResult' => $baseDir . '/lib/public/DB/IResult.php', 'OCP\\DB\\ISchemaWrapper' => $baseDir . '/lib/public/DB/ISchemaWrapper.php', + 'OCP\\DB\\QueryBuilder\\ConflictResolutionMode' => $baseDir . '/lib/public/DB/QueryBuilder/ConflictResolutionMode.php', 'OCP\\DB\\QueryBuilder\\ICompositeExpression' => $baseDir . '/lib/public/DB/QueryBuilder/ICompositeExpression.php', 'OCP\\DB\\QueryBuilder\\IExpressionBuilder' => $baseDir . '/lib/public/DB/QueryBuilder/IExpressionBuilder.php', 'OCP\\DB\\QueryBuilder\\IFunctionBuilder' => $baseDir . '/lib/public/DB/QueryBuilder/IFunctionBuilder.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 7409cdf224d11..75bcc8d69de9a 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -352,6 +352,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\DB\\IPreparedStatement' => __DIR__ . '/../../..' . '/lib/public/DB/IPreparedStatement.php', 'OCP\\DB\\IResult' => __DIR__ . '/../../..' . '/lib/public/DB/IResult.php', 'OCP\\DB\\ISchemaWrapper' => __DIR__ . '/../../..' . '/lib/public/DB/ISchemaWrapper.php', + 'OCP\\DB\\QueryBuilder\\ConflictResolutionMode' => __DIR__ . '/../../..' . '/lib/public/DB/QueryBuilder/ConflictResolutionMode.php', 'OCP\\DB\\QueryBuilder\\ICompositeExpression' => __DIR__ . '/../../..' . '/lib/public/DB/QueryBuilder/ICompositeExpression.php', 'OCP\\DB\\QueryBuilder\\IExpressionBuilder' => __DIR__ . '/../../..' . '/lib/public/DB/QueryBuilder/IExpressionBuilder.php', 'OCP\\DB\\QueryBuilder\\IFunctionBuilder' => __DIR__ . '/../../..' . '/lib/public/DB/QueryBuilder/IFunctionBuilder.php', diff --git a/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php b/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php index f928132a123a8..c017412cd180b 100644 --- a/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php +++ b/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php @@ -10,6 +10,7 @@ use OC\DB\Exceptions\DbalException; use OCP\DB\IResult; +use OCP\DB\QueryBuilder\ConflictResolutionMode; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -306,4 +307,9 @@ public function getOutputColumns(): array { public function prefixTableName(string $table): string { return $this->builder->prefixTableName($table); } + + public function forUpdate(ConflictResolutionMode $conflictResolutionMode = ConflictResolutionMode::Ordinary): self { + $this->builder->forUpdate($conflictResolutionMode); + return $this; + } } diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index ea93e57d80d40..9fac22bb181e2 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -20,12 +20,14 @@ use OC\DB\QueryBuilder\FunctionBuilder\SqliteFunctionBuilder; use OC\SystemConfig; use OCP\DB\IResult; +use OCP\DB\QueryBuilder\ConflictResolutionMode; use OCP\DB\QueryBuilder\ICompositeExpression; use OCP\DB\QueryBuilder\ILiteral; use OCP\DB\QueryBuilder\IParameter; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\DB\QueryBuilder\IQueryFunction; use OCP\IDBConnection; +use Override; use Psr\Log\LoggerInterface; class QueryBuilder implements IQueryBuilder { @@ -1404,4 +1406,12 @@ public function runAcrossAllShards(): self { return $this; } + #[Override] + public function forUpdate(ConflictResolutionMode $conflictResolutionMode = ConflictResolutionMode::Ordinary): self { + match ($conflictResolutionMode) { + ConflictResolutionMode::Ordinary => $this->queryBuilder->forUpdate(), + ConflictResolutionMode::SkipLocked => $this->queryBuilder->forUpdate(\Doctrine\DBAL\Query\ForUpdate\ConflictResolutionMode::SKIP_LOCKED), + }; + return $this; + } } diff --git a/lib/private/Files/Cache/Propagator.php b/lib/private/Files/Cache/Propagator.php index a6ba87896f4de..a3400bdb04b24 100644 --- a/lib/private/Files/Cache/Propagator.php +++ b/lib/private/Files/Cache/Propagator.php @@ -1,5 +1,7 @@ storage = $storage; - $this->connection = $connection; - $this->ignore = $ignore; + public function __construct( + protected readonly IStorage $storage, + private readonly IDBConnection $connection, + private readonly array $ignore = [], + ) { $this->clock = Server::get(ClockInterface::class); } - /** - * @param string $internalPath - * @param int $time - * @param int $sizeDifference number of bytes the file has grown - */ + #[Override] public function propagateChange($internalPath, $time, $sizeDifference = 0) { // Do not propagate changes in ignored paths foreach ($this->ignore as $ignore) { @@ -63,9 +45,9 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) { } } - $time = min((int)$time, $this->clock->now()->getTimestamp()); + $time = min($time, $this->clock->now()->getTimestamp()); - $storageId = $this->storage->getStorageCache()->getNumericId(); + $storageId = $this->storage->getCache()->getNumericStorageId(); $parents = $this->getParents($internalPath); @@ -77,6 +59,7 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) { } $parentHashes = array_map('md5', $parents); + sort($parentHashes); // Ensure rows are always locked in the same order $etag = uniqid(); // since we give all folders the same etag we don't ask the storage for the etag $builder = $this->connection->getQueryBuilder(); @@ -137,7 +120,10 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) { } } - protected function getParents($path) { + /** + * @return string[] + */ + protected function getParents(string $path): array { $parts = explode('/', $path); $parent = ''; $parents = []; @@ -148,19 +134,12 @@ protected function getParents($path) { return $parents; } - /** - * Mark the beginning of a propagation batch - * - * Note that not all cache setups support propagation in which case this will be a noop - * - * Batching for cache setups that do support it has to be explicit since the cache state is not fully consistent - * before the batch is committed. - */ - public function beginBatch() { + #[Override] + public function beginBatch(): void { $this->inBatch = true; } - private function addToBatch($internalPath, $time, $sizeDifference) { + private function addToBatch(string $internalPath, int $time, int $sizeDifference): void { if (!isset($this->batch[$internalPath])) { $this->batch[$internalPath] = [ 'hash' => md5($internalPath), @@ -175,49 +154,103 @@ private function addToBatch($internalPath, $time, $sizeDifference) { } } - /** - * Commit the active propagation batch - */ - public function commitBatch() { + #[Override] + public function commitBatch(): void { if (!$this->inBatch) { throw new \BadMethodCallException('Not in batch'); } $this->inBatch = false; - $this->connection->beginTransaction(); - - $query = $this->connection->getQueryBuilder(); - $storageId = (int)$this->storage->getStorageCache()->getNumericId(); - - $query->update('filecache') - ->set('mtime', $query->func()->greatest('mtime', $query->createParameter('time'))) - ->set('etag', $query->expr()->literal(uniqid())) - ->where($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) - ->andWhere($query->expr()->eq('path_hash', $query->createParameter('hash'))); - - $sizeQuery = $this->connection->getQueryBuilder(); - $sizeQuery->update('filecache') - ->set('size', $sizeQuery->func()->add('size', $sizeQuery->createParameter('size'))) - ->where($query->expr()->eq('storage', $sizeQuery->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) - ->andWhere($query->expr()->eq('path_hash', $sizeQuery->createParameter('hash'))) - ->andWhere($sizeQuery->expr()->gt('size', $sizeQuery->createNamedParameter(-1, IQueryBuilder::PARAM_INT))); - - foreach ($this->batch as $item) { - $query->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT); - $query->setParameter('hash', $item['hash']); - - $query->executeStatement(); - - if ($item['size']) { - $sizeQuery->setParameter('size', $item['size'], IQueryBuilder::PARAM_INT); - $sizeQuery->setParameter('hash', $item['hash']); - - $sizeQuery->executeStatement(); + // Ensure rows are always locked in the same order + uasort($this->batch, static fn (array $a, array $b) => $a['hash'] <=> $b['hash']); + + try { + $this->connection->beginTransaction(); + + $storageId = $this->storage->getCache()->getNumericStorageId(); + + if ($this->connection->getDatabaseProvider() !== IDBConnection::PLATFORM_SQLITE) { + // Lock the rows before updating then with a SELECT FOR UPDATE + // The select also allow us to fetch the fileid and then use these in the UPDATE + // queries as a faster lookup than the path_hash + $hashes = array_map(static fn (array $a): string => $a['hash'], $this->batch); + + foreach (array_chunk($hashes, 1000) as $hashesChunk) { + $query = $this->connection->getQueryBuilder(); + $result = $query->select('fileid', 'path', 'path_hash', 'size') + ->from('filecache') + ->where($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) + ->andWhere($query->expr()->in('path_hash', $query->createNamedParameter($hashesChunk, IQueryBuilder::PARAM_STR_ARRAY))) + ->orderBy('path_hash') + ->forUpdate() + ->executeQuery(); + + $query = $this->connection->getQueryBuilder(); + $query->update('filecache') + ->set('mtime', $query->func()->greatest('mtime', $query->createParameter('time'))) + ->set('etag', $query->expr()->literal(uniqid())) + ->where($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) + ->andWhere($query->expr()->eq('fileid', $query->createParameter('fileid'))); + + $queryWithSize = $this->connection->getQueryBuilder(); + $queryWithSize->update('filecache') + ->set('mtime', $queryWithSize->func()->greatest('mtime', $queryWithSize->createParameter('time'))) + ->set('etag', $queryWithSize->expr()->literal(uniqid())) + ->set('size', $queryWithSize->func()->add('size', $queryWithSize->createParameter('size'))) + ->where($queryWithSize->expr()->eq('storage', $queryWithSize->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) + ->andWhere($queryWithSize->expr()->eq('fileid', $queryWithSize->createParameter('fileid'))); + + while ($row = $result->fetchAssociative()) { + $item = $this->batch[$row['path']]; + if ($item['size'] && $row['size'] > -1) { + $queryWithSize->setParameter('fileid', $row['fileid'], IQueryBuilder::PARAM_INT) + ->setParameter('size', $item['size'], IQueryBuilder::PARAM_INT) + ->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT) + ->executeStatement(); + } else { + $query->setParameter('fileid', $row['fileid'], IQueryBuilder::PARAM_INT) + ->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT) + ->executeStatement(); + } + } + } + } else { + // No FOR UPDATE support in Sqlite, but instead the whole table is locked + $query = $this->connection->getQueryBuilder(); + $query->update('filecache') + ->set('mtime', $query->func()->greatest('mtime', $query->createParameter('time'))) + ->set('etag', $query->expr()->literal(uniqid())) + ->where($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) + ->andWhere($query->expr()->eq('path_hash', $query->createParameter('hash'))); + + $queryWithSize = $this->connection->getQueryBuilder(); + $queryWithSize->update('filecache') + ->set('mtime', $queryWithSize->func()->greatest('mtime', $queryWithSize->createParameter('time'))) + ->set('etag', $queryWithSize->expr()->literal(uniqid())) + ->set('size', $queryWithSize->func()->add('size', $queryWithSize->createParameter('size'))) + ->where($queryWithSize->expr()->eq('storage', $queryWithSize->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) + ->andWhere($queryWithSize->expr()->eq('path_hash', $queryWithSize->createParameter('hash'))); + + foreach ($this->batch as $item) { + if ($item['size']) { + $queryWithSize->setParameter('hash', $item['hash'], IQueryBuilder::PARAM_STR) + ->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT) + ->setParameter('size', $item['size'], IQueryBuilder::PARAM_INT) + ->executeStatement(); + } else { + $query->setParameter('hash', $item['hash'], IQueryBuilder::PARAM_STR) + ->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT) + ->executeStatement(); + } + } } - } - $this->batch = []; + $this->batch = []; - $this->connection->commit(); + $this->connection->commit(); + } catch (\Exception $e) { + $this->connection->rollback(); + throw $e; + } } } diff --git a/lib/public/DB/QueryBuilder/ConflictResolutionMode.php b/lib/public/DB/QueryBuilder/ConflictResolutionMode.php new file mode 100644 index 0000000000000..56a59b39099c4 --- /dev/null +++ b/lib/public/DB/QueryBuilder/ConflictResolutionMode.php @@ -0,0 +1,26 @@ + Date: Fri, 6 Feb 2026 16:13:44 +0100 Subject: [PATCH 2/2] fix(propagator): Lock rows also in propagateChange Signed-off-by: Carl Schwan (cherry picked from commit 3d031b03f3cb18ba84648e0eb95c79c5d6d18a36) --- lib/private/Files/Cache/Propagator.php | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/private/Files/Cache/Propagator.php b/lib/private/Files/Cache/Propagator.php index a3400bdb04b24..c1337051fe20f 100644 --- a/lib/private/Files/Cache/Propagator.php +++ b/lib/private/Files/Cache/Propagator.php @@ -11,6 +11,7 @@ use OC\DB\Exceptions\DbalException; use OC\Files\Storage\Wrapper\Encryption; +use OCP\DB\QueryBuilder\ILiteral; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Cache\IPropagator; use OCP\Files\Storage\IReliableEtagStorage; @@ -63,9 +64,7 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) { $etag = uniqid(); // since we give all folders the same etag we don't ask the storage for the etag $builder = $this->connection->getQueryBuilder(); - $hashParams = array_map(function ($hash) use ($builder) { - return $builder->expr()->literal($hash); - }, $parentHashes); + $hashParams = array_map(static fn (string $hash): ILiteral => $builder->expr()->literal($hash), $parentHashes); $builder->update('filecache') ->set('mtime', $builder->func()->greatest('mtime', $builder->createNamedParameter($time, IQueryBuilder::PARAM_INT))) @@ -106,9 +105,27 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) { for ($i = 0; $i < self::MAX_RETRIES; $i++) { try { - $builder->executeStatement(); + if ($this->connection->getDatabaseProvider() !== IDBConnection::PLATFORM_SQLITE) { + $this->connection->beginTransaction(); + // Lock all the rows first with a SELECT FOR UPDATE ordered by path_hash + $forUpdate = $this->connection->getQueryBuilder(); + $forUpdate->select('fileid') + ->from('filecache') + ->where($forUpdate->expr()->eq('storage', $forUpdate->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) + ->andWhere($forUpdate->expr()->in('path_hash', $hashParams)) + ->orderBy('path_hash') + ->forUpdate() + ->executeQuery(); + $builder->executeStatement(); + $this->connection->commit(); + } else { + $builder->executeStatement(); + } break; } catch (DbalException $e) { + if ($this->connection->getDatabaseProvider() !== IDBConnection::PLATFORM_SQLITE) { + $this->connection->rollBack(); + } if (!$e->isRetryable()) { throw $e; }