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 <carlschwan@kde.org> (cherry picked from commit cae742d182)
2026-02-18 18:28:50 -05:00 · 2026-02-06 12:09:18 +01:00 · 2026-02-06 12:09:18 +01:00 · cbe4a26156
commit cbe4a26156
parent c3bd909779
8 changed files with 159 additions and 75 deletions
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@ -311,6 +311,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',
--- 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',
--- a/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php
+++ b/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php
@ -10,6 +10,7 @@ namespace OC\DB\QueryBuilder;

 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 @@ 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;
+	}
 }
--- a/lib/private/DB/QueryBuilder/QueryBuilder.php
+++ b/lib/private/DB/QueryBuilder/QueryBuilder.php
@ -20,12 +20,14 @@ 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\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 @@ 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;
+	}
 }
--- a/lib/private/Files/Cache/Propagator.php
+++ b/lib/private/Files/Cache/Propagator.php
@ -1,5 +1,7 @@
 <?php

+declare(strict_types=1);
+
 /**
 * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
 * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
@ -12,49 +14,29 @@ use OC\Files\Storage\Wrapper\Encryption;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Files\Cache\IPropagator;
 use OCP\Files\Storage\IReliableEtagStorage;
+use OCP\Files\Storage\IStorage;
 use OCP\IDBConnection;
 use OCP\Server;
+use Override;
 use Psr\Clock\ClockInterface;
 use Psr\Log\LoggerInterface;

-/**
- * Propagate etags and mtimes within the storage
- */
 class Propagator implements IPropagator {
 	public const MAX_RETRIES = 3;
-	private $inBatch = false;
-
-	private $batch = [];
-
-	/**
-	 * @var \OC\Files\Storage\Storage
-	 */
-	protected $storage;
-
-	/**
-	 * @var IDBConnection
-	 */
-	private $connection;
-
-	/**
-	 * @var array
-	 */
-	private $ignore = [];

+	private bool $inBatch = false;
+	private array $batch = [];
 	private ClockInterface $clock;

-	public function __construct(\OC\Files\Storage\Storage $storage, IDBConnection $connection, array $ignore = []) {
-		$this->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 @@ class Propagator implements IPropagator {
 			}
 		}

-		$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 @@ 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();
@ -137,7 +120,10 @@ class Propagator implements IPropagator {
 		}
 	}

-	protected function getParents($path) {
+	/**
+	 * @return string[]
+	 */
+	protected function getParents(string $path): array {
 		$parts = explode('/', $path);
 		$parent = '';
 		$parents = [];
@ -148,19 +134,12 @@ class Propagator implements IPropagator {
 		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 @@ class Propagator implements IPropagator {
 		}
 	}

-	/**
-	 * 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();
+		// Ensure rows are always locked in the same order
+		uasort($this->batch, static fn (array $a, array $b) => $a['hash'] <=> $b['hash']);

-		$query = $this->connection->getQueryBuilder();
-		$storageId = (int)$this->storage->getStorageCache()->getNumericId();
+		try {
+			$this->connection->beginTransaction();

-		$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')));
+			$storageId = $this->storage->getCache()->getNumericStorageId();

-		$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)));
+			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 ($this->batch as $item) {
-			$query->setParameter('time', $item['time'], IQueryBuilder::PARAM_INT);
-			$query->setParameter('hash', $item['hash']);
+				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->executeStatement();
+					$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')));

-			if ($item['size']) {
-				$sizeQuery->setParameter('size', $item['size'], IQueryBuilder::PARAM_INT);
-				$sizeQuery->setParameter('hash', $item['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('fileid', $queryWithSize->createParameter('fileid')));

-				$sizeQuery->executeStatement();
+					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->connection->commit();
+		} catch (\Exception $e) {
+			$this->connection->rollback();
+			throw $e;
 		}
-
-		$this->batch = [];
-
-		$this->connection->commit();
 	}
 }
--- a/lib/public/DB/QueryBuilder/ConflictResolutionMode.php
+++ b/lib/public/DB/QueryBuilder/ConflictResolutionMode.php
@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCP\DB\QueryBuilder;
+
+/**
+ * Conflict resolution mode for "FOR UPDATE" select queries.
+ *
+ * @since 34.0.0
+ */
+enum ConflictResolutionMode {
+	/**
+	 * Wait for the row to be unlocked.
+	 */
+	case Ordinary;
+	/**
+	 * Skip the row if it is locked.
+	 */
+	case SkipLocked;
+}
--- a/lib/public/DB/QueryBuilder/IQueryBuilder.php
+++ b/lib/public/DB/QueryBuilder/IQueryBuilder.php
@ -1108,4 +1108,12 @@ interface IQueryBuilder {
 	 * @since 30.0.0
 	 */
 	public function getOutputColumns(): array;
+
+	/**
+	 * Locks the queried rows for a subsequent update.
+	 *
+	 * @return $this
+	 * @since 34.0.0
+	 */
+	public function forUpdate(ConflictResolutionMode $conflictResolutionMode = ConflictResolutionMode::Ordinary): self;
 }
--- a/tests/lib/Files/Cache/PropagatorTest.php
+++ b/tests/lib/Files/Cache/PropagatorTest.php
@ -17,8 +17,7 @@ use Test\TestCase;
 * @group DB
 */
 class PropagatorTest extends TestCase {
-	/** @var IStorage */
-	private $storage;
+	private IStorage $storage;

 	protected function setUp(): void {
 		parent::setUp();