From 275b32d5138c0221b425198a9b517f536bba0a0a Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 26 Mar 2025 21:03:53 +0100 Subject: [PATCH 1/3] feat: Implement getSeenUsers to iterate over users This method uses an iterator. This is lighter on resources and gives more control to the caller Signed-off-by: Louis Chemineau --- lib/private/User/Manager.php | 26 ++++++++++++++++++++++++++ lib/public/IUserManager.php | 11 +++++++++++ 2 files changed, 37 insertions(+) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 62a7b39be1685..cfe2d6dad0b33 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -827,4 +827,30 @@ private function verifyUid(string $uid, bool $checkDataDirectory = false): bool 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; + + do { + $userIds = $this->getSeenUserIds($limit, $offset); + $offset += $limit; + + foreach ($userIds as $userId) { + foreach ($this->backends as $backend) { + if ($backend->userExists($userId)) { + $user = $this->getUserObject($userId, $backend, false); + yield $user; + break; + } + } + } + } while (count($userIds) === $limit); + } } diff --git a/lib/public/IUserManager.php b/lib/public/IUserManager.php index 50eaa9c98b731..3fd3234818e2e 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -231,4 +231,15 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v * @since 30.0.0 */ public function getLastLoggedInUsers(?int $limit = null, int $offset = 0, string $search = ''): array; + + /** + * Gets the list of users. + * An iterator is returned allowing the caller to stop the iteration at any time. + * The offset argument allows the caller to continue the iteration at a specific offset. + * + * @param int $offset from which offset to fetch + * @return \Iterator list of IUser object + * @since 32.0.0 + */ + public function getSeenUsers(int $offset = 0): \Iterator; } From 22e02d1c1e7534ab2b91d2cc3f88cb6a0f7c3495 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 26 Mar 2025 20:59:11 +0100 Subject: [PATCH 2/3] chore: Refactor callForSeenUsers to use getSeenUsers Signed-off-by: Louis Chemineau --- lib/private/User/Manager.php | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index cfe2d6dad0b33..152fb08eeeb9a 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -628,30 +628,14 @@ public function countSeenUsers() { return $result; } - /** - * @param \Closure $callback - * @psalm-param \Closure(\OCP\IUser):?bool $callback - * @since 11.0.0 - */ public function callForSeenUsers(\Closure $callback) { - $limit = 1000; - $offset = 0; - do { - $userIds = $this->getSeenUserIds($limit, $offset); - $offset += $limit; - foreach ($userIds as $userId) { - foreach ($this->backends as $backend) { - if ($backend->userExists($userId)) { - $user = $this->getUserObject($userId, $backend, false); - $return = $callback($user); - if ($return === false) { - return; - } - break; - } - } + $users = $this->getSeenUsers(); + foreach ($users as $user) { + $return = $callback($user); + if ($return === false) { + return; } - } while (count($userIds) >= $limit); + } } /** From bd9a2eba760af44c4308e5b907b55ed6bdf8e77a Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 26 Mar 2025 21:01:47 +0100 Subject: [PATCH 3/3] feat: Limit trash expire job to 30 minutes And pick up where it left off, leveraging getSeenUsers. Signed-off-by: Louis Chemineau --- .../lib/BackgroundJob/ExpireTrash.php | 30 ++++++++++++------- .../tests/BackgroundJob/ExpireTrashTest.php | 27 +++++++++-------- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php b/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php index 4e1c1b95fde51..c587d463501ef 100644 --- a/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php +++ b/apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php @@ -12,13 +12,12 @@ use OCA\Files_Trashbin\Trashbin; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; -use OCP\IConfig; -use OCP\IUser; +use OCP\IAppConfig; use OCP\IUserManager; class ExpireTrash extends TimedJob { public function __construct( - private IConfig $config, + private IAppConfig $appConfig, private IUserManager $userManager, private Expiration $expiration, ITimeFactory $time, @@ -28,12 +27,8 @@ public function __construct( $this->setInterval(60 * 30); } - /** - * @param $argument - * @throws \Exception - */ protected function run($argument) { - $backgroundJob = $this->config->getAppValue('files_trashbin', 'background_job_expire_trash', 'yes'); + $backgroundJob = $this->appConfig->getValueString('files_trashbin', 'background_job_expire_trash', 'yes'); if ($backgroundJob === 'no') { return; } @@ -43,15 +38,28 @@ protected function run($argument) { return; } - $this->userManager->callForSeenUsers(function (IUser $user): void { + $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); + + foreach ($users as $user) { $uid = $user->getUID(); if (!$this->setupFS($uid)) { - return; + continue; } $dirContent = Helper::getTrashFiles('/', $uid, 'mtime'); Trashbin::deleteExpiredFiles($dirContent, $uid); - }); + $offset++; + + if ($stopTime < time()) { + $this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset); + \OC_Util::tearDownFS(); + return; + } + } + + $this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0); \OC_Util::tearDownFS(); } diff --git a/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php b/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php index b172f1f27154a..829770689833a 100644 --- a/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php +++ b/apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php @@ -11,31 +11,31 @@ use OCA\Files_Trashbin\Expiration; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJobList; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IUserManager; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class ExpireTrashTest extends TestCase { - /** @var IConfig|MockObject */ - private $config; + /** @var IAppConfig&MockObject */ + private $appConfig; - /** @var IUserManager|MockObject */ + /** @var IUserManager&MockObject */ private $userManager; - /** @var Expiration|MockObject */ + /** @var Expiration&MockObject */ private $expiration; - /** @var IJobList|MockObject */ + /** @var IJobList&MockObject */ private $jobList; - /** @var ITimeFactory|MockObject */ + /** @var ITimeFactory&MockObject */ private $time; protected function setUp(): void { parent::setUp(); - $this->config = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->userManager = $this->createMock(IUserManager::class); $this->expiration = $this->createMock(Expiration::class); $this->jobList = $this->createMock(IJobList::class); @@ -51,22 +51,25 @@ protected function setUp(): void { } public function testConstructAndRun(): void { - $this->config->method('getAppValue') + $this->appConfig->method('getValueString') ->with('files_trashbin', 'background_job_expire_trash', 'yes') ->willReturn('yes'); + $this->appConfig->method('getValueInt') + ->with('files_trashbin', 'background_job_expire_trash_offset', 0) + ->willReturn(0); - $job = new ExpireTrash($this->config, $this->userManager, $this->expiration, $this->time); + $job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->time); $job->start($this->jobList); } public function testBackgroundJobDeactivated(): void { - $this->config->method('getAppValue') + $this->appConfig->method('getValueString') ->with('files_trashbin', 'background_job_expire_trash', 'yes') ->willReturn('no'); $this->expiration->expects($this->never()) ->method('getMaxAgeAsTimestamp'); - $job = new ExpireTrash($this->config, $this->userManager, $this->expiration, $this->time); + $job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->time); $job->start($this->jobList); } }