From 5c20f5b9a2ac56855be1c70350788dc74cf31a01 Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Thu, 1 Feb 2024 09:31:12 +0100
Subject: [PATCH] fix(sharing): Avoid (dead)locking during orphan deletion

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---
 .../lib/DeleteOrphanedSharesJob.php           | 78 ++++++++++++++++---
 .../tests/DeleteOrphanedSharesJobTest.php     |  3 +-
 2 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/apps/files_sharing/lib/DeleteOrphanedSharesJob.php b/apps/files_sharing/lib/DeleteOrphanedSharesJob.php
index 6883bfd476225..6c6d5bfede550 100644
--- a/apps/files_sharing/lib/DeleteOrphanedSharesJob.php
+++ b/apps/files_sharing/lib/DeleteOrphanedSharesJob.php
@@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -24,24 +27,45 @@
  */
 namespace OCA\Files_Sharing;
 
+use OCP\AppFramework\Db\TTransactional;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\TimedJob;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
-use OCP\Server;
+use PDO;
 use Psr\Log\LoggerInterface;
+use function array_map;
 
 /**
  * Delete all share entries that have no matching entries in the file cache table.
  */
 class DeleteOrphanedSharesJob extends TimedJob {
+
+	use TTransactional;
+
+	private const CHUNK_SIZE = 1000;
+
+	private const INTERVAL = 24 * 60 * 60; // 1 day
+
+	private IDBConnection $db;
+
+	private LoggerInterface $logger;
+
 	/**
 	 * sets the correct interval for this timed job
 	 */
-	public function __construct(ITimeFactory $time) {
+	public function __construct(
+		ITimeFactory $time,
+		IDBConnection $db,
+		LoggerInterface $logger
+	) {
 		parent::__construct($time);
 
-		$this->setInterval(24 * 60 * 60); // 1 day
+		$this->db = $db;
+
+		$this->setInterval(self::INTERVAL); // 1 day
 		$this->setTimeSensitivity(self::TIME_INSENSITIVE);
+		$this->logger = $logger;
 	}
 
 	/**
@@ -50,15 +74,45 @@ public function __construct(ITimeFactory $time) {
 	 * @param array $argument unused argument
 	 */
 	public function run($argument) {
-		$connection = Server::get(IDBConnection::class);
-		$logger = Server::get(LoggerInterface::class);
-
-		$sql =
-			'DELETE FROM `*PREFIX*share` ' .
-			'WHERE `item_type` in (\'file\', \'folder\') ' .
-			'AND NOT EXISTS (SELECT `fileid` FROM `*PREFIX*filecache` WHERE `file_source` = `fileid`)';
+		$qbSelect = $this->db->getQueryBuilder();
+		$qbSelect->select('id')
+			->from('share', 's')
+			->leftJoin('s', 'filecache', 'fc', $qbSelect->expr()->eq('s.file_source', 'fc.fileid'))
+			->where($qbSelect->expr()->isNull('fc.fileid'))
+			->setMaxResults(self::CHUNK_SIZE);
+		$deleteQb = $this->db->getQueryBuilder();
+		$deleteQb->delete('share')
+			->where(
+				$deleteQb->expr()->in('id', $deleteQb->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY)
+			);
 
-		$deletedEntries = $connection->executeStatement($sql);
-		$logger->debug("$deletedEntries orphaned share(s) deleted", ['app' => 'DeleteOrphanedSharesJob']);
+		/**
+		 * Read a chunk of orphan rows and delete them. Continue as long as the
+		 * chunk is filled and time before the next cron run does not run out.
+		 *
+		 * Note: With isolation level READ COMMITTED, the database will allow
+		 * other transactions to delete rows between our SELECT and DELETE. In
+		 * that (unlikely) case, our DELETE will have fewer affected rows than
+		 * IDs passed for the WHERE IN. If this happens while processing a full
+		 * chunk, the logic below will stop prematurely.
+		 * Note: The queries below are optimized for low database locking. They
+		 * could be combined into one single DELETE with join or sub query, but
+		 * that has shown to (dead)lock often.
+		 */
+		$cutOff = $this->time->getTime() + self::INTERVAL;
+		do {
+			$deleted = $this->atomic(function () use ($qbSelect, $deleteQb) {
+				$result = $qbSelect->executeQuery();
+				$ids = array_map('intval', $result->fetchAll(PDO::FETCH_COLUMN));
+				$result->closeCursor();
+				$deleteQb->setParameter('ids', $ids, IQueryBuilder::PARAM_INT_ARRAY);
+				$deleted = $deleteQb->executeStatement();
+				$this->logger->debug("{deleted} orphaned share(s) deleted", [
+					'app' => 'DeleteOrphanedSharesJob',
+					'deleted' => $deleted,
+				]);
+				return $deleted;
+			}, $this->db);
+		} while ($deleted >= self::CHUNK_SIZE && $this->time->getTime() <= $cutOff);
 	}
 }
diff --git a/apps/files_sharing/tests/DeleteOrphanedSharesJobTest.php b/apps/files_sharing/tests/DeleteOrphanedSharesJobTest.php
index 3de40215f15b5..0a39246e03030 100644
--- a/apps/files_sharing/tests/DeleteOrphanedSharesJobTest.php
+++ b/apps/files_sharing/tests/DeleteOrphanedSharesJobTest.php
@@ -27,7 +27,6 @@
 namespace OCA\Files_Sharing\Tests;
 
 use OCA\Files_Sharing\DeleteOrphanedSharesJob;
-use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Share\IShare;
 
 /**
@@ -94,7 +93,7 @@ protected function setUp(): void {
 
 		\OC::registerShareHooks(\OC::$server->getSystemConfig());
 
-		$this->job = new DeleteOrphanedSharesJob(\OCP\Server::get(ITimeFactory::class));
+		$this->job = \OCP\Server::get(DeleteOrphanedSharesJob::class);
 	}
 
 	protected function tearDown(): void {