Merge pull request #52825 from nextcloud/artonge/feat/files_trashbin_parallel_expire_job

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,
+		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 @@ class ExpireTrash extends TimedJob {
 			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 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;
+	}
 }

apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php

@@ -9,12 +9,15 @@ declare(strict_types=1);
 
 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;
@@ -26,6 +29,8 @@ class ExpireTrashTest extends TestCase {
 	private IJobList&MockObject $jobList;
 	private LoggerInterface&MockObject $logger;
 	private ITimeFactory&MockObject $time;
+	private SetupManager&MockObject $setupManager;
+	private ILockingProvider&MockObject $lockingProvider;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -35,6 +40,8 @@ class ExpireTrashTest extends TestCase {
 		$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')
@@ -47,25 +54,41 @@ class ExpireTrashTest extends TestCase {
 	}
 
 	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);
 	}
 }

build/psalm-baseline.xml

@@ -1754,19 +1754,6 @@
       <code><![CDATA[moveMount]]></code>
     </UndefinedMethod>
   </file>
-  <file src="apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php">
-    <DeprecatedClass>
-      <code><![CDATA[\OC_Util::setupFS($user)]]></code>
-      <code><![CDATA[\OC_Util::tearDownFS()]]></code>
-      <code><![CDATA[\OC_Util::tearDownFS()]]></code>
-      <code><![CDATA[\OC_Util::tearDownFS()]]></code>
-    </DeprecatedClass>
-    <DeprecatedMethod>
-      <code><![CDATA[\OC_Util::tearDownFS()]]></code>
-      <code><![CDATA[\OC_Util::tearDownFS()]]></code>
-      <code><![CDATA[\OC_Util::tearDownFS()]]></code>
-    </DeprecatedMethod>
-  </file>
   <file src="apps/files_trashbin/lib/Command/CleanUp.php">
     <DeprecatedClass>
       <code><![CDATA[\OC_Util::setupFS($uid)]]></code>

lib/private/User/Manager.php

@@ -634,7 +634,7 @@ class Manager extends PublicEmitter implements IUserManager {
 	}
 
 	/**
-	 * 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.
 	 *
@@ -813,19 +813,19 @@ class Manager extends PublicEmitter implements IUserManager {
 		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<IUser> 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) {
@@ -836,6 +836,6 @@ class Manager extends PublicEmitter implements IUserManager {
 					}
 				}
 			}
-		} while (count($userIds) === $limit);
+		} while (count($userIds) === $batchSize && $limit !== 0);
 	}
 }

lib/public/IUserManager.php

@@ -239,8 +239,9 @@ interface IUserManager {
 	 * 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<IUser> 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;
 }