From f3778bc9c7c93add71c69ad98ff5e3d1554cb686 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 --- lib/composer/composer/LICENSE | 2 + 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 | 92 +++++++++++++++---- .../QueryBuilder/ConflictResolutionMode.php | 26 ++++++ lib/public/DB/QueryBuilder/IQueryBuilder.php | 8 ++ tests/lib/Files/Cache/PropagatorTest.php | 3 +- 9 files changed, 127 insertions(+), 22 deletions(-) create mode 100644 lib/public/DB/QueryBuilder/ConflictResolutionMode.php diff --git a/lib/composer/composer/LICENSE b/lib/composer/composer/LICENSE index 62ecfd8d004..f27399a042d 100644 --- a/lib/composer/composer/LICENSE +++ b/lib/composer/composer/LICENSE @@ -1,3 +1,4 @@ + Copyright (c) Nils Adermann, Jordi Boggiano Permission is hereby granted, free of charge, to any person obtaining a copy @@ -17,3 +18,4 @@ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 89f12a775b0..ce18c4275cc 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -319,6 +319,7 @@ return array( '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 b43d17d10cc..36e3c2a2276 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -360,6 +360,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 9230c55c28c..c7eda9b337c 100644 --- a/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php +++ b/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php @@ -9,6 +9,7 @@ declare(strict_types=1); namespace OC\DB\QueryBuilder; use OCP\DB\IResult; +use OCP\DB\QueryBuilder\ConflictResolutionMode; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -290,4 +291,9 @@ abstract class ExtendedQueryBuilder implements IQueryBuilder { 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 962ad48b1c4..faf196e241c 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -19,6 +19,7 @@ use OC\DB\QueryBuilder\FunctionBuilder\PgSqlFunctionBuilder; 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\IExpressionBuilder; use OCP\DB\QueryBuilder\IFunctionBuilder; @@ -27,6 +28,7 @@ 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 { @@ -1364,4 +1366,12 @@ class QueryBuilder implements IQueryBuilder { 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 151b15d7590..d75240aff56 100644 --- a/lib/private/Files/Cache/Propagator.php +++ b/lib/private/Files/Cache/Propagator.php @@ -59,6 +59,7 @@ class Propagator implements IPropagator { } $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(); @@ -160,36 +161,87 @@ class Propagator implements IPropagator { } $this->inBatch = false; + // 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(); - $query = $this->connection->getQueryBuilder(); $storageId = $this->storage->getCache()->getNumericStorageId(); - $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'))); + 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); - $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 (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(); - foreach ($this->batch as $item) { - $query->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT); - $query->setParameter('hash', $item['hash']); + $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'))); - $query->executeStatement(); + $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'))); - if ($item['size']) { - $sizeQuery->setParameter('size', $item['size'], IQueryBuilder::PARAM_INT); - $sizeQuery->setParameter('hash', $item['hash']); + 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'))); - $sizeQuery->executeStatement(); + $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(); + } } } diff --git a/lib/public/DB/QueryBuilder/ConflictResolutionMode.php b/lib/public/DB/QueryBuilder/ConflictResolutionMode.php new file mode 100644 index 00000000000..56a59b39099 --- /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 --- lib/private/Files/Cache/Propagator.php | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/private/Files/Cache/Propagator.php b/lib/private/Files/Cache/Propagator.php index d75240aff56..fdd926d89ac 100644 --- a/lib/private/Files/Cache/Propagator.php +++ b/lib/private/Files/Cache/Propagator.php @@ -11,6 +11,7 @@ namespace OC\Files\Cache; 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 @@ class Propagator implements IPropagator { $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,14 +105,31 @@ class Propagator implements IPropagator { 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; } - /** @var LoggerInterface $loggerInterface */ $loggerInterface = Server::get(LoggerInterface::class); $loggerInterface->warning('Retrying propagation query after retryable exception.', [ 'exception' => $e ]); }