Merge pull request #51807 from nextcloud/backport/51600/stable30

[stable30] feat: Limit `ExpireTrash` job to 30 minutes

Author: AndyScherzinger

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php

@@ -11,36 +11,24 @@
 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 {
-	private IConfig $config;
-	private Expiration $expiration;
-	private IUserManager $userManager;
 
 	public function __construct(
-		IConfig $config,
-		IUserManager $userManager,
-		Expiration $expiration,
+		private IAppConfig $appConfig,
+		private IUserManager $userManager,
+		private Expiration $expiration,
 		ITimeFactory $time
 	) {
 		parent::__construct($time);
 		// Run once per 30 minutes
 		$this->setInterval(60 * 30);
-
-		$this->config = $config;
-		$this->userManager = $userManager;
-		$this->expiration = $expiration;
 	}
 
-	/**
-	 * @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;
 		}
@@ -50,15 +38,28 @@ protected function run($argument) {
 			return;
 		}
 
-		$this->userManager->callForSeenUsers(function (IUser $user) {
+		$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();
 	}
 

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

lib/private/User/Manager.php

@@ -620,30 +620,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);
+		}
 	}
 
 	/**
@@ -819,4 +803,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<IUser> 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);
+	}
 }

lib/public/IUserManager.php

@@ -221,4 +221,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<IUser> list of IUser object
+	 * @since 32.0.0
+	 */
+	public function getSeenUsers(int $offset = 0): \Iterator;
 }