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); } } 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; }