From 05ffa8dc3926ad5675ec3e92a9650c7a04b029b0 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 21 May 2025 11:18:39 +0200 Subject: [PATCH 1/2] feat: Support limit argument in `getSeenUsers` Signed-off-by: Louis Chemineau --- lib/private/User/Manager.php | 26 +++++++++++++------------- lib/public/IUserManager.php | 3 ++- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 4915e55497a87..e48304183bda8 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -607,7 +607,7 @@ public function callForSeenUsers(\Closure $callback) { } /** - * Getting all userIds that have a listLogin value requires checking the + * Getting all userIds that have a lastLogin value requires checking the * value in php because on oracle you cannot use a clob in a where clause, * preventing us from doing a not null or length(value) > 0 check. * @@ -780,19 +780,19 @@ public function getDisplayNameCache(): DisplayNameCache { return $this->displayNameCache; } - /** - * Gets the list of users sorted by lastLogin, from most recent to least recent - * - * @param int $offset from which offset to fetch - * @return \Iterator list of user IDs - * @since 30.0.0 - */ - public function getSeenUsers(int $offset = 0): \Iterator { - $limit = 1000; + public function getSeenUsers(int $offset = 0, ?int $limit = null): \Iterator { + $maxBatchSize = 1000; do { - $userIds = $this->getSeenUserIds($limit, $offset); - $offset += $limit; + if ($limit !== null) { + $batchSize = min($limit, $maxBatchSize); + $limit -= $batchSize; + } else { + $batchSize = $maxBatchSize; + } + + $userIds = $this->getSeenUserIds($batchSize, $offset); + $offset += $batchSize; foreach ($userIds as $userId) { foreach ($this->backends as $backend) { @@ -803,6 +803,6 @@ public function getSeenUsers(int $offset = 0): \Iterator { } } } - } while (count($userIds) === $limit); + } while (count($userIds) === $batchSize && $limit !== 0); } } diff --git a/lib/public/IUserManager.php b/lib/public/IUserManager.php index 75549a953e3ac..cd86d81cf6d0e 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -228,8 +228,9 @@ public function getLastLoggedInUsers(?int $limit = null, int $offset = 0, string * The offset argument allows the caller to continue the iteration at a specific offset. * * @param int $offset from which offset to fetch + * @param int|null $limit maximum number of records to fetch * @return \Iterator list of IUser object * @since 32.0.0 */ - public function getSeenUsers(int $offset = 0): \Iterator; + public function getSeenUsers(int $offset = 0, ?int $limit = null): \Iterator; } From 1f464b22a73883c6252eef4ea5156b04ffa40023 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Fri, 5 Sep 2025 12:46:52 +0200 Subject: [PATCH 2/2] feat(files_trashbin): Refactor expire background job to support parallel run - Follow-up of #51600 The original PR introduced the possibility to continue an `ExpireTrash` job by saving the offset. This was to prevent having to start over the whole user list when the job crashed or was killed. But on big instances, one process is not enough to go through all the users in a timely manner. Supporting parallel run allows covering more ground faster. This PR introduced this possibility. We are now storing the offset right away to allow another parallel job to pick up the task at that point. We are arbitrarily cutting the user list in chunk of 10 to not drastically overflow the 30 minutes time limit. Signed-off-by: Louis Chemineau --- .../lib/BackgroundJob/ExpireTrash.php | 109 +++++++++++++----- .../tests/BackgroundJob/ExpireTrashTest.php | 42 +++++-- 2 files changed, 113 insertions(+), 38 deletions(-) diff --git a/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php b/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php index 35e4d91e53940..8d3dad4a17a02 100644 --- a/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php +++ b/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php @@ -7,32 +7,42 @@ */ namespace OCA\Files_Trashbin\BackgroundJob; +use OC\Files\SetupManager; +use OC\Files\View; +use OCA\Files_Trashbin\AppInfo\Application; use OCA\Files_Trashbin\Expiration; use OCA\Files_Trashbin\Helper; use OCA\Files_Trashbin\Trashbin; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; use OCP\IAppConfig; +use OCP\IUser; use OCP\IUserManager; +use OCP\Lock\ILockingProvider; use Psr\Log\LoggerInterface; class ExpireTrash extends TimedJob { + public const TOGGLE_CONFIG_KEY_NAME = 'background_job_expire_trash'; + public const OFFSET_CONFIG_KEY_NAME = 'background_job_expire_trash_offset'; + private const THIRTY_MINUTES = 30 * 60; + private const USER_BATCH_SIZE = 10; public function __construct( private IAppConfig $appConfig, private IUserManager $userManager, private Expiration $expiration, private LoggerInterface $logger, - ITimeFactory $time + private SetupManager $setupManager, + private ILockingProvider $lockingProvider, + ITimeFactory $time, ) { parent::__construct($time); - // Run once per 30 minutes - $this->setInterval(60 * 30); + $this->setInterval(self::THIRTY_MINUTES); } protected function run($argument) { - $backgroundJob = $this->appConfig->getValueString('files_trashbin', 'background_job_expire_trash', 'yes'); - if ($backgroundJob === 'no') { + $backgroundJob = $this->appConfig->getValueBool(Application::APP_ID, self::TOGGLE_CONFIG_KEY_NAME, true); + if (!$backgroundJob) { return; } @@ -41,48 +51,89 @@ protected function run($argument) { return; } - $stopTime = time() + 60 * 30; // Stops after 30 minutes. - $offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0); - $users = $this->userManager->getSeenUsers($offset); + $startTime = time(); - foreach ($users as $user) { - try { + // Process users in batches of 10, but don't run for more than 30 minutes + while (time() < $startTime + self::THIRTY_MINUTES) { + $offset = $this->getNextOffset(); + $users = $this->userManager->getSeenUsers($offset, self::USER_BATCH_SIZE); + $count = 0; + + foreach ($users as $user) { $uid = $user->getUID(); - if (!$this->setupFS($uid)) { - continue; + $count++; + + try { + if ($this->setupFS($user)) { + $dirContent = Helper::getTrashFiles('/', $uid, 'mtime'); + Trashbin::deleteExpiredFiles($dirContent, $uid); + } + } catch (\Throwable $e) { + $this->logger->error('Error while expiring trashbin for user ' . $uid, ['exception' => $e]); + } finally { + $this->setupManager->tearDown(); } - $dirContent = Helper::getTrashFiles('/', $uid, 'mtime'); - Trashbin::deleteExpiredFiles($dirContent, $uid); - } catch (\Throwable $e) { - $this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]); } - $offset++; - - if ($stopTime < time()) { - $this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset); - \OC_Util::tearDownFS(); - return; + // If the last batch was not full it means that we reached the end of the user list. + if ($count < self::USER_BATCH_SIZE) { + $this->resetOffset(); } } - - $this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0); - \OC_Util::tearDownFS(); } /** * Act on behalf on trash item owner */ - protected function setupFS(string $user): bool { - \OC_Util::tearDownFS(); - \OC_Util::setupFS($user); + protected function setupFS(IUser $user): bool { + $this->setupManager->setupForUser($user); // Check if this user has a trashbin directory - $view = new \OC\Files\View('/' . $user); + $view = new View('/' . $user->getUID()); if (!$view->is_dir('/files_trashbin/files')) { return false; } return true; } + + private function getNextOffset(): int { + return $this->runMutexOperation(function () { + $this->appConfig->clearCache(); + + $offset = $this->appConfig->getValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, 0); + $this->appConfig->setValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, $offset + self::USER_BATCH_SIZE); + + return $offset; + }); + + } + + private function resetOffset() { + $this->runMutexOperation(function () { + $this->appConfig->setValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, 0); + }); + } + + private function runMutexOperation($operation): mixed { + $acquired = false; + + while ($acquired === false) { + try { + $this->lockingProvider->acquireLock(self::OFFSET_CONFIG_KEY_NAME, ILockingProvider::LOCK_EXCLUSIVE, 'Expire trashbin background job offset'); + $acquired = true; + } catch (\OCP\Lock\LockedException $e) { + // wait a bit and try again + usleep(100000); + } + } + + try { + $result = $operation(); + } finally { + $this->lockingProvider->releaseLock(self::OFFSET_CONFIG_KEY_NAME, ILockingProvider::LOCK_EXCLUSIVE); + } + + return $result; + } } diff --git a/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php b/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php index 2a860851e9967..af68aeea2e09b 100644 --- a/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php +++ b/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php @@ -8,12 +8,15 @@ namespace OCA\Files_Trashbin\Tests\BackgroundJob; +use OC\Files\SetupManager; +use OCA\Files_Trashbin\AppInfo\Application; use OCA\Files_Trashbin\BackgroundJob\ExpireTrash; use OCA\Files_Trashbin\Expiration; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJobList; use OCP\IAppConfig; use OCP\IUserManager; +use OCP\Lock\ILockingProvider; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -37,6 +40,9 @@ class ExpireTrashTest extends TestCase { /** @var ITimeFactory&MockObject */ private $time; + private SetupManager&MockObject $setupManager; + private ILockingProvider&MockObject $lockingProvider; + protected function setUp(): void { parent::setUp(); @@ -45,6 +51,8 @@ protected function setUp(): void { $this->expiration = $this->createMock(Expiration::class); $this->jobList = $this->createMock(IJobList::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->setupManager = $this->createMock(SetupManager::class); + $this->lockingProvider = $this->createMock(ILockingProvider::class); $this->time = $this->createMock(ITimeFactory::class); $this->time->method('getTime') @@ -57,25 +65,41 @@ protected function setUp(): void { } public function testConstructAndRun(): void { - $this->appConfig->method('getValueString') - ->with('files_trashbin', 'background_job_expire_trash', 'yes') - ->willReturn('yes'); + $this->appConfig->method('getValueBool') + ->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true) + ->willReturn(true); $this->appConfig->method('getValueInt') - ->with('files_trashbin', 'background_job_expire_trash_offset', 0) + ->with(Application::APP_ID, ExpireTrash::OFFSET_CONFIG_KEY_NAME, 0) ->willReturn(0); - $job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time); + $job = new ExpireTrash( + $this->appConfig, + $this->userManager, + $this->expiration, + $this->logger, + $this->setupManager, + $this->lockingProvider, + $this->time, + ); $job->start($this->jobList); } public function testBackgroundJobDeactivated(): void { - $this->appConfig->method('getValueString') - ->with('files_trashbin', 'background_job_expire_trash', 'yes') - ->willReturn('no'); + $this->appConfig->method('getValueBool') + ->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true) + ->willReturn(false); $this->expiration->expects($this->never()) ->method('getMaxAgeAsTimestamp'); - $job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time); + $job = new ExpireTrash( + $this->appConfig, + $this->userManager, + $this->expiration, + $this->logger, + $this->setupManager, + $this->lockingProvider, + $this->time, + ); $job->start($this->jobList); } }