From 64469efa2974aa87e54f50050e782ce348c84b4a Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 1 Feb 2024 09:31:12 +0100 Subject: [PATCH 1/2] fix(sharing): Avoid (dead)locking during orphan deletion Signed-off-by: Christoph Wurst [skip ci] --- .../lib/DeleteOrphanedSharesJob.php | 26 +++++++++++++++++-- .../tests/DeleteOrphanedSharesJobTest.php | 3 +-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/DeleteOrphanedSharesJob.php b/apps/files_sharing/lib/DeleteOrphanedSharesJob.php index a9452cb3dcc53..0763711114a25 100644 --- a/apps/files_sharing/lib/DeleteOrphanedSharesJob.php +++ b/apps/files_sharing/lib/DeleteOrphanedSharesJob.php @@ -1,4 +1,7 @@ setInterval(24 * 60 * 60); // 1 day + $this->db = $db; + + $this->setInterval(self::INTERVAL); // 1 day $this->setTimeSensitivity(self::TIME_INSENSITIVE); + $this->logger = $logger; } /** 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 { From 1a50eb42e857fc835322e314b72de34e456eb2e4 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Wed, 29 May 2024 10:21:58 +0200 Subject: [PATCH 2/2] Fix backport Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- .../lib/DeleteOrphanedSharesJob.php | 53 +++++++++++++++---- 1 file changed, 44 insertions(+), 9 deletions(-) diff --git a/apps/files_sharing/lib/DeleteOrphanedSharesJob.php b/apps/files_sharing/lib/DeleteOrphanedSharesJob.php index 0763711114a25..6c6d5bfede550 100644 --- a/apps/files_sharing/lib/DeleteOrphanedSharesJob.php +++ b/apps/files_sharing/lib/DeleteOrphanedSharesJob.php @@ -30,6 +30,11 @@ use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\IDBConnection; +use PDO; +use Psr\Log\LoggerInterface; +use function array_map; /** * Delete all share entries that have no matching entries in the file cache table. @@ -69,15 +74,45 @@ public function __construct( * @param array $argument unused argument */ public function run($argument) { - $connection = \OC::$server->getDatabaseConnection(); - $logger = \OC::$server->getLogger(); + $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) + ); - $sql = - 'DELETE FROM `*PREFIX*share` ' . - 'WHERE `item_type` in (\'file\', \'folder\') ' . - 'AND NOT EXISTS (SELECT `fileid` FROM `*PREFIX*filecache` WHERE `file_source` = `fileid`)'; - - $deletedEntries = $connection->executeUpdate($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); } }