feat(taskprocessing): avoid generator cascade

Signed-off-by: Julien Veyssier <[email protected]>

core/Command/TaskProcessing/Cleanup.php

@@ -9,16 +9,25 @@
 namespace OC\Core\Command\TaskProcessing;
 
 use OC\Core\Command\Base;
+use OC\TaskProcessing\Db\TaskMapper;
 use OC\TaskProcessing\Manager;
+use OCP\Files\AppData\IAppDataFactory;
+use Psr\Log\LoggerInterface;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class Cleanup extends Base {
+	private \OCP\Files\IAppData $appData;
+
 	public function __construct(
 		protected Manager $taskProcessingManager,
+		private TaskMapper $taskMapper,
+		private LoggerInterface $logger,
+		IAppDataFactory $appDataFactory,
 	) {
 		parent::__construct();
+		$this->appData = $appDataFactory->get('core');
 	}
 
 	protected function configure() {
@@ -37,20 +46,48 @@ protected function configure() {
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$maxAgeSeconds = $input->getArgument('maxAgeSeconds') ?? Manager::MAX_TASK_AGE_SECONDS;
 		$output->writeln('<comment>Cleanup up tasks older than ' . $maxAgeSeconds . ' seconds and the related output files</comment>');
-		$cleanupResult = $this->taskProcessingManager->cleanupOldTasks($maxAgeSeconds);
-		foreach ($cleanupResult as $entry) {
-			if (isset($entry['task_id'], $entry['file_id'], $entry['file_name'])) {
-				$output->writeln("<info>\t - " . 'Deleted appData/core/TaskProcessing/' . $entry['file_name'] . ' (fileId: ' . $entry['file_id'] . ', taskId: ' . $entry['task_id'] . ')</info>');
-			} elseif (isset($entry['directory_name'])) {
-				$output->writeln("<info>\t - " . 'Deleted appData/core/' . $entry['directory_name'] . '/' . $entry['file_name'] . '</info>');
-			} elseif (isset($entry['deleted_task_count'])) {
-				$output->writeln("<comment>\t - " . 'Deleted ' . $entry['deleted_task_count'] . ' tasks from the database</comment>');
-			} elseif (isset($entry['deleted_task_id_list'])) {
-				foreach ($entry['deleted_task_id_list'] as $taskId) {
-					$output->writeln("<info>\t - " . 'Deleted task ' . $taskId . ' from the database</info>');
-				}
+
+		$taskIdsToCleanup = [];
+		try {
+			$fileCleanupGenerator = $this->taskProcessingManager->cleanupTaskProcessingTaskFiles($maxAgeSeconds);
+			foreach ($fileCleanupGenerator as $cleanedUpEntry) {
+				$output->writeln(
+					"<info>\t - " . 'Deleted appData/core/TaskProcessing/' . $cleanedUpEntry['file_name']
+					. ' (fileId: ' . $cleanedUpEntry['file_id'] . ', taskId: ' . $cleanedUpEntry['task_id'] . ')</info>'
+				);
 			}
+			$taskIdsToCleanup = $fileCleanupGenerator->getReturn();
+		} catch (\Exception $e) {
+			$this->logger->warning('Failed to delete stale task processing tasks files', ['exception' => $e]);
+			$output->writeln('<warning>Failed to delete stale task processing tasks files</warning>');
 		}
+		try {
+			$deletedTaskCount = $this->taskMapper->deleteOlderThan($maxAgeSeconds);
+			foreach ($taskIdsToCleanup as $taskId) {
+				$output->writeln("<info>\t - " . 'Deleted task ' . $taskId . ' from the database</info>');
+			}
+			$output->writeln("<comment>\t - " . 'Deleted ' . $deletedTaskCount . ' tasks from the database</comment>');
+		} catch (\OCP\DB\Exception $e) {
+			$this->logger->warning('Failed to delete stale task processing tasks', ['exception' => $e]);
+			$output->writeln('<warning>Failed to delete stale task processing tasks</warning>');
+		}
+		try {
+			$textToImageDeletedFileNames = $this->taskProcessingManager->clearFilesOlderThan($this->appData->getFolder('text2image'), $maxAgeSeconds);
+			foreach ($textToImageDeletedFileNames as $entry) {
+				$output->writeln("<info>\t - " . 'Deleted appData/core/text2image/' . $entry . '</info>');
+			}
+		} catch (\OCP\Files\NotFoundException $e) {
+			// noop
+		}
+		try {
+			$audioToTextDeletedFileNames = $this->taskProcessingManager->clearFilesOlderThan($this->appData->getFolder('audio2text'), $maxAgeSeconds);
+			foreach ($audioToTextDeletedFileNames as $entry) {
+				$output->writeln("<info>\t - " . 'Deleted appData/core/audio2text/' . $entry . '</info>');
+			}
+		} catch (\OCP\Files\NotFoundException $e) {
+			// noop
+		}
+
 		return 0;
 	}
 }

lib/private/TaskProcessing/Manager.php

@@ -1490,58 +1490,18 @@ public function extractFileIdsFromTask(Task $task): array {
 		return $ids;
 	}
 
-	/**
-	 * @param int $ageInSeconds
-	 * @return \Generator
-	 */
-	public function cleanupOldTasks(int $ageInSeconds = self::MAX_TASK_AGE_SECONDS): \Generator {
-		$taskIdsToCleanup = [];
-		try {
-			$fileCleanupGenerator = $this->cleanupTaskProcessingTaskFiles($ageInSeconds);
-			foreach ($fileCleanupGenerator as $cleanedUpEntry) {
-				yield $cleanedUpEntry;
-			}
-			$taskIdsToCleanup = $fileCleanupGenerator->getReturn();
-		} catch (\Exception $e) {
-			$this->logger->warning('Failed to delete stale task processing tasks files', ['exception' => $e]);
-		}
-		try {
-			$deletedTaskCount = $this->taskMapper->deleteOlderThan($ageInSeconds);
-			yield ['deleted_task_id_list' => $taskIdsToCleanup];
-			yield ['deleted_task_count' => $deletedTaskCount];
-		} catch (\OCP\DB\Exception $e) {
-			$this->logger->warning('Failed to delete stale task processing tasks', ['exception' => $e]);
-		}
-		try {
-			$textToImageDeletedFiles = $this->clearFilesOlderThan($this->appData->getFolder('text2image'), $ageInSeconds);
-			foreach ($textToImageDeletedFiles as $entry) {
-				yield $entry;
-			}
-		} catch (\OCP\Files\NotFoundException $e) {
-			// noop
-		}
-		try {
-			$audioToTextDeletedFiles = $this->clearFilesOlderThan($this->appData->getFolder('audio2text'), $ageInSeconds);
-			foreach ($audioToTextDeletedFiles as $entry) {
-				yield $entry;
-			}
-		} catch (\OCP\Files\NotFoundException $e) {
-			// noop
-		}
-	}
-
 	/**
 	 * @param ISimpleFolder $folder
 	 * @param int $ageInSeconds
 	 * @return \Generator
 	 */
-	private function clearFilesOlderThan(ISimpleFolder $folder, int $ageInSeconds): \Generator {
+	public function clearFilesOlderThan(ISimpleFolder $folder, int $ageInSeconds = self::MAX_TASK_AGE_SECONDS): \Generator {
 		foreach ($folder->getDirectoryListing() as $file) {
 			if ($file->getMTime() < time() - $ageInSeconds) {
 				try {
 					$fileName = $file->getName();
 					$file->delete();
-					yield ['directory_name' => $folder->getName(), 'file_name' => $fileName];
+					yield $fileName;
 				} catch (NotPermittedException $e) {
 					$this->logger->warning('Failed to delete a stale task processing file', ['exception' => $e]);
 				}
@@ -1558,7 +1518,7 @@ private function clearFilesOlderThan(ISimpleFolder $folder, int $ageInSeconds):
 	 * @throws \JsonException
 	 * @throws \OCP\Files\NotFoundException
 	 */
-	private function cleanupTaskProcessingTaskFiles(int $ageInSeconds): \Generator {
+	public function cleanupTaskProcessingTaskFiles(int $ageInSeconds = self::MAX_TASK_AGE_SECONDS): \Generator {
 		$taskIdsToCleanup = [];
 		foreach ($this->taskMapper->getTasksToCleanup($ageInSeconds) as $task) {
 			$taskIdsToCleanup[] = $task->getId();

lib/private/TaskProcessing/RemoveOldTasksBackgroundJob.php

@@ -6,25 +6,52 @@
  */
 namespace OC\TaskProcessing;
 
+use OC\TaskProcessing\Db\TaskMapper;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\TimedJob;
+use OCP\Files\AppData\IAppDataFactory;
+use Psr\Log\LoggerInterface;
 
 class RemoveOldTasksBackgroundJob extends TimedJob {
+	private \OCP\Files\IAppData $appData;
 
 	public function __construct(
 		ITimeFactory $timeFactory,
 		private Manager $taskProcessingManager,
+		private TaskMapper $taskMapper,
+		private LoggerInterface $logger,
+		IAppDataFactory $appDataFactory,
 	) {
 		parent::__construct($timeFactory);
 		$this->setInterval(60 * 60 * 24);
 		// can be deferred to maintenance window
 		$this->setTimeSensitivity(self::TIME_INSENSITIVE);
+		$this->appData = $appDataFactory->get('core');
 	}
 
 	/**
 	 * @inheritDoc
 	 */
 	protected function run($argument): void {
-		iterator_to_array($this->taskProcessingManager->cleanupOldTasks());
+		try {
+			iterator_to_array($this->taskProcessingManager->cleanupTaskProcessingTaskFiles());
+		} catch (\Exception $e) {
+			$this->logger->warning('Failed to delete stale task processing tasks files', ['exception' => $e]);
+		}
+		try {
+			$this->taskMapper->deleteOlderThan(Manager::MAX_TASK_AGE_SECONDS);
+		} catch (\OCP\DB\Exception $e) {
+			$this->logger->warning('Failed to delete stale task processing tasks', ['exception' => $e]);
+		}
+		try {
+			iterator_to_array($this->taskProcessingManager->clearFilesOlderThan($this->appData->getFolder('text2image')));
+		} catch (\OCP\Files\NotFoundException $e) {
+			// noop
+		}
+		try {
+			iterator_to_array($this->taskProcessingManager->clearFilesOlderThan($this->appData->getFolder('audio2text')));
+		} catch (\OCP\Files\NotFoundException $e) {
+			// noop
+		}
 	}
 }

tests/lib/TaskProcessing/TaskProcessingTest.php

@@ -943,6 +943,11 @@ public function testOldTasksShouldBeCleanedUp(): void {
 		$timeFactory->expects($this->any())->method('getDateTime')->willReturnCallback(fn () => $currentTime);
 		$timeFactory->expects($this->any())->method('getTime')->willReturnCallback(fn () => $currentTime->getTimestamp());
 
+		$this->taskMapper = new TaskMapper(
+			Server::get(IDBConnection::class),
+			$timeFactory,
+		);
+
 		$this->registrationContext->expects($this->any())->method('getTaskProcessingProviders')->willReturn([
 			new ServiceRegistration('test', SuccessfulSyncProvider::class)
 		]);
@@ -963,34 +968,14 @@ public function testOldTasksShouldBeCleanedUp(): void {
 
 		$task = $this->manager->getTask($task->getId());
 
-		$taskMapper = new TaskMapper(
-			Server::get(IDBConnection::class),
-			$timeFactory,
-		);
-		$manager = new Manager(
-			$this->appConfig,
-			$this->coordinator,
-			$this->serverContainer,
-			Server::get(LoggerInterface::class),
-			$taskMapper,
-			$this->jobList,
-			Server::get(IEventDispatcher::class),
-			Server::get(IAppDataFactory::class),
-			Server::get(IRootFolder::class),
-			Server::get(\OC\TextToImage\Manager::class),
-			$this->userMountCache,
-			Server::get(IClientService::class),
-			Server::get(IAppManager::class),
-			Server::get(IUserManager::class),
-			Server::get(IUserSession::class),
-			Server::get(ICacheFactory::class),
-		);
 		$currentTime = $currentTime->add(new \DateInterval('P1Y'));
 		// run background job
 		$bgJob = new RemoveOldTasksBackgroundJob(
 			$timeFactory,
-			// use a locally defined manager to make sure the taskMapper uses the mocked timeFactory
-			$manager,
+			$this->manager,
+			$this->taskMapper,
+			Server::get(LoggerInterface::class),
+			Server::get(IAppDataFactory::class),
 		);
 		$bgJob->setArgument([]);
 		$bgJob->start($this->jobList);