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 <[email protected]>

[skip ci]

Author: artonge

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php

@@ -32,6 +32,7 @@
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\TimedJob;
 use OCP\IAppConfig;
+use OCP\IUser;
 use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
 
@@ -45,8 +46,7 @@ public function __construct(
 		ITimeFactory $time
 	) {
 		parent::__construct($time);
-		// Run once per 30 minutes
-		$this->setInterval(60 * 30);
+		$this->setInterval(self::THIRTY_MINUTES);
 	}
 
 	protected function run($argument) {
@@ -60,41 +60,44 @@ 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);
+		$stopTime = time() + self::THIRTY_MINUTES;
 
-		foreach ($users as $user) {
-			try {
+		do {
+			$this->appConfig->clearCache();
+			$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
+			$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset + 10);
+
+			$users = $this->userManager->getSeenUsers($offset, 10);
+			$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++;
+		} while (time() < $stopTime && $count === 10);
 
-			if ($stopTime < time()) {
-				$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset);
-				\OC_Util::tearDownFS();
-				return;
-			}
+		if ($count < 10) {
+			$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
 		}
-
-		$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);

apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php

@@ -25,6 +25,7 @@
 
 namespace OCA\Files_Trashbin\Tests\BackgroundJob;
 
+use OC\Files\SetupManager;
 use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
 use OCA\Files_Trashbin\Expiration;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -36,23 +37,14 @@
 use Test\TestCase;
 
 class ExpireTrashTest extends TestCase {
-	/** @var IAppConfig&MockObject */
-	private $appConfig;
 
-	/** @var IUserManager&MockObject */
-	private $userManager;
-
-	/** @var Expiration&MockObject */
-	private $expiration;
-
-	/** @var IJobList&MockObject */
-	private $jobList;
-
-	/** @var LoggerInterface&MockObject */
-	private $logger;
-
-	/** @var ITimeFactory&MockObject */
-	private $time;
+	private IAppConfig&MockObject $appConfig;
+	private IUserManager&MockObject $userManager;
+	private Expiration&MockObject $expiration;
+	private IJobList&MockObject $jobList;
+	private LoggerInterface&MockObject $logger;
+	private ITimeFactory&MockObject $time;
+	private SetupManager&MockObject $setupManager;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -62,6 +54,7 @@ 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->time = $this->createMock(ITimeFactory::class);
 		$this->time->method('getTime')
@@ -81,7 +74,7 @@ public function testConstructAndRun(): void {
 			->with('files_trashbin', 'background_job_expire_trash_offset', 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->time);
 		$job->start($this->jobList);
 	}
 
@@ -92,7 +85,7 @@ public function testBackgroundJobDeactivated(): void {
 		$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->time);
 		$job->start($this->jobList);
 	}
 }