[stable34] perf(cleanup): Optimize cleanup job

appinfo/info.xml

@@ -1,6 +1,6 @@
 <?xml version="1.0"?>
 <!--
-  - SPDX-FileCopyrightText: 2019-2024 Nextcloud GmbH and Nextcloud contributors
+  - SPDX-FileCopyrightText: 2019-2026 Nextcloud GmbH and Nextcloud contributors
   - SPDX-License-Identifier: AGPL-3.0-or-later
 -->
 <info xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"

composer/composer/autoload_classmap.php

@@ -68,6 +68,7 @@
     'OCA\\Text\\Migration\\Version070000Date20250925110024' => $baseDir . '/../lib/Migration/Version070000Date20250925110024.php',
     'OCA\\Text\\Migration\\Version080000Date20260331132113' => $baseDir . '/../lib/Migration/Version080000Date20260331132113.php',
     'OCA\\Text\\Migration\\Version080000Date20261020120000' => $baseDir . '/../lib/Migration/Version080000Date20261020120000.php',
+    'OCA\\Text\\Migration\\Version100000Date20261011000000' => $baseDir . '/../lib/Migration/Version100000Date20261011000000.php',
     'OCA\\Text\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php',
     'OCA\\Text\\Service\\AiTagService' => $baseDir . '/../lib/Service/AiTagService.php',
     'OCA\\Text\\Service\\ApiService' => $baseDir . '/../lib/Service/ApiService.php',

composer/composer/autoload_static.php

@@ -83,6 +83,7 @@ class ComposerStaticInitText
         'OCA\\Text\\Migration\\Version070000Date20250925110024' => __DIR__ . '/..' . '/../lib/Migration/Version070000Date20250925110024.php',
         'OCA\\Text\\Migration\\Version080000Date20260331132113' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20260331132113.php',
         'OCA\\Text\\Migration\\Version080000Date20261020120000' => __DIR__ . '/..' . '/../lib/Migration/Version080000Date20261020120000.php',
+        'OCA\\Text\\Migration\\Version100000Date20261011000000' => __DIR__ . '/..' . '/../lib/Migration/Version100000Date20261011000000.php',
         'OCA\\Text\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php',
         'OCA\\Text\\Service\\AiTagService' => __DIR__ . '/..' . '/../lib/Service/AiTagService.php',
         'OCA\\Text\\Service\\ApiService' => __DIR__ . '/..' . '/../lib/Service/ApiService.php',

lib/Cron/Cleanup.php

@@ -17,6 +17,7 @@
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\TimedJob;
 use OCP\Files\ISetupManager;
+use OCP\Files\NotPermittedException;
 use Psr\Log\LoggerInterface;
 
 class Cleanup extends TimedJob {
@@ -37,9 +38,19 @@ public function __construct(
 	 */
 	protected function run($argument): void {
 		$this->logger->debug('Run cleanup job for text documents');
-		foreach ($this->documentService->getAllWithNoActiveSession() as $document) {
+		foreach ($this->documentService->getAllPendingAttachmentCleanup() as $document) {
+			if ($this->documentService->hasUnsavedChanges($document)) {
+				$this->documentService->markAttachmentCleanup($document);
+				continue;
+			}
 			$this->setupManager->tearDown();
-			$this->attachmentService->cleanupAttachments($document->getId());
+			try {
+				$this->attachmentService->cleanupAttachments($document->getId());
+			} catch (NotPermittedException $e) {
+				// Retrying will not help until the document is saved again, so mark it as cleaned up anyway
+				$this->logger->warning('Could not clean up attachments of file ' . $document->getId() . ', deleting is not permitted', ['exception' => $e]);
+			}
+			$this->documentService->markAttachmentCleanup($document);
 		}
 
 		$this->logger->debug('Run cleanup job for text sessions');

lib/Db/Document.php

@@ -33,6 +33,7 @@ class Document extends Entity implements \JsonSerializable {
 	protected int $lastSavedVersion = 0;
 	protected int $initialVersion = 0;
 	protected int $lastSavedVersionTime = 0;
+	protected int $lastAttachmentCleanup = 0;
 	protected string $lastSavedVersionEtag = '';
 	protected string $baseVersionEtag = '';
 	protected ?string $checksum = null;
@@ -41,10 +42,19 @@ public function __construct() {
 		$this->addType('currentVersion', 'integer');
 		$this->addType('lastSavedVersion', 'integer');
 		$this->addType('lastSavedVersionTime', 'integer');
+		$this->addType('lastAttachmentCleanup', 'integer');
 		$this->addType('initialVersion', 'integer');
 		$this->addType('checksum', 'string');
 	}
 
+	public function getLastAttachmentCleanup(): int {
+		return $this->lastAttachmentCleanup;
+	}
+
+	public function setLastAttachmentCleanup(int $time): void {
+		$this->setter('lastAttachmentCleanup', [$time]);
+	}
+
 	public function jsonSerialize(): array {
 		return [
 			'id' => $this->getId(),

lib/Db/DocumentMapper.php

@@ -54,12 +54,13 @@ public function findAll(): Generator {
 		}
 	}
 
-	public function findAllWithNoActiveSessions(): Generator {
+	public function findAllPendingAttachmentCleanup(): Generator {
 		$qb = $this->db->getQueryBuilder();
 		$result = $qb->select('d.*')
 			->from($this->getTableName(), 'd')
 			->leftJoin('d', 'text_sessions', 's', $qb->expr()->eq('s.document_id', 'd.id'))
 			->where($qb->expr()->isNull('s.id'))
+			->andWhere($qb->expr()->gt('d.last_saved_version_time', 'd.last_attachment_cleanup'))
 			->executeQuery();
 		try {
 			while ($row = $result->fetch()) {

lib/Migration/Version100000Date20261011000000.php

@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Text\Migration;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\DB\Types;
+use OCP\Migration\Attributes\AddColumn;
+use OCP\Migration\Attributes\ColumnType;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+
+#[AddColumn(table: 'text_documents', name: 'last_attachment_cleanup', type: ColumnType::BIGINT, description: 'Last saved timestamp processed by attachment cleanup')]
+class Version100000Date20261011000000 extends SimpleMigrationStep {
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
+		$schema = $schemaClosure();
+		$table = $schema->getTable('text_documents');
+		if (!$table->hasColumn('last_attachment_cleanup')) {
+			$table->addColumn('last_attachment_cleanup', Types::BIGINT, [
+				'notnull' => true,
+				'default' => 0,
+				'unsigned' => true,
+			]);
+			return $schema;
+		}
+
+		return null;
+	}
+}

lib/Service/DocumentService.php

@@ -487,8 +487,13 @@ public function getAll(): \Generator {
 		return $this->documentMapper->findAll();
 	}
 
-	public function getAllWithNoActiveSession(): \Generator {
-		return $this->documentMapper->findAllWithNoActiveSessions();
+	public function getAllPendingAttachmentCleanup(): \Generator {
+		return $this->documentMapper->findAllPendingAttachmentCleanup();
+	}
+
+	public function markAttachmentCleanup(Document $document): void {
+		$document->setLastAttachmentCleanup($document->getLastSavedVersionTime());
+		$this->documentMapper->update($document);
 	}
 
 	/**

tests/unit/CleanupTest.php

@@ -0,0 +1,132 @@
+<?php
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Text\Tests;
+
+use OCA\Text\Cron\Cleanup;
+use OCA\Text\Db\Document;
+use OCA\Text\Service\AttachmentService;
+use OCA\Text\Service\DocumentService;
+use OCA\Text\Service\SessionService;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\Files\IFilenameValidator;
+use OCP\Files\IMimeTypeDetector;
+use OCP\Files\IRootFolder;
+use OCP\Files\ISetupManager;
+use OCP\Files\Node;
+use OCP\Files\NotPermittedException;
+use OCP\FilesMetadata\IFilesMetadataManager;
+use OCP\IPreview;
+use OCP\ISession;
+use OCP\IURLGenerator;
+use OCP\Share\IManager;
+use Psr\Log\LoggerInterface;
+
+class CleanupTest extends \PHPUnit\Framework\TestCase {
+	private Cleanup $cleanup;
+	private Document $document;
+	private DocumentService $documentService;
+	private AttachmentService $attachmentService;
+	private SessionService $sessionService;
+	private IRootFolder $rootFolder;
+	private ISetupManager $setupManager;
+	private LoggerInterface $logger;
+
+	public function setUp(): void {
+		$this->document = Document::fromRow([
+			'id' => 123,
+			'last_saved_version_time' => 1000,
+		]);
+		$this->documentService = $this->createMock(DocumentService::class);
+		$this->rootFolder = $this->createMock(IRootFolder::class);
+		$this->attachmentService = new AttachmentService(
+			$this->rootFolder,
+			$this->createMock(IManager::class),
+			$this->createMock(IPreview::class),
+			$this->createMock(IMimeTypeDetector::class),
+			$this->createMock(IURLGenerator::class),
+			$this->createMock(IFilenameValidator::class),
+			$this->createMock(IFilesMetadataManager::class),
+			$this->createMock(ISession::class),
+		);
+		$this->sessionService = $this->createMock(SessionService::class);
+		$this->setupManager = $this->createMock(ISetupManager::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->documentService->expects(self::never())->method('resetDocument');
+		$this->documentService->expects(self::once())
+			->method('getAllPendingAttachmentCleanup')
+			->willReturn((function (): \Generator {
+				yield $this->document;
+			})());
+		$this->cleanup = new class($this->createMock(ITimeFactory::class), $this->sessionService, $this->documentService, $this->attachmentService, $this->logger, $this->setupManager, ) extends Cleanup {
+			public function run($argument): void {
+				parent::run($argument);
+			}
+		};
+	}
+
+	public function testMarksCleanupOnlyAfterAttachmentsAreCleaned(): void {
+		$this->documentService->expects(self::once())
+			->method('hasUnsavedChanges')->with($this->document)->willReturn(false);
+		$this->setupManager->expects(self::once())->method('tearDown');
+		$cleaned = false;
+		$this->rootFolder->expects(self::once())
+			->method('getFirstNodeById')->with(123)
+			->willReturnCallback(function () use (&$cleaned): ?Node {
+				$cleaned = true;
+				return null;
+			});
+		$this->documentService->expects(self::once())
+			->method('markAttachmentCleanup')->with($this->document)
+			->willReturnCallback(function () use (&$cleaned): void {
+				self::assertTrue($cleaned);
+			});
+		$this->sessionService->expects(self::once())->method('removeInactiveSessionsWithoutSteps');
+		$this->sessionService->expects(self::once())->method('removeOldSessions');
+		$this->sessionService->expects(self::once())->method('removeOrphanedSteps');
+		$this->documentService->expects(self::once())->method('cleanupOldDocumentsFolders');
+
+		$this->cleanup->run([]);
+	}
+
+	public function testMarksUnsavedChangesWithoutCleanup(): void {
+		$this->documentService->expects(self::once())
+			->method('hasUnsavedChanges')->with($this->document)->willReturn(true);
+		$this->setupManager->expects(self::never())->method('tearDown');
+		$this->rootFolder->expects(self::never())->method('getFirstNodeById');
+		$this->documentService->expects(self::once())->method('markAttachmentCleanup')->with($this->document);
+
+		$this->cleanup->run([]);
+	}
+
+	public function testFailedCleanupRemainsPending(): void {
+		$this->documentService->method('hasUnsavedChanges')->willReturn(false);
+		$this->rootFolder->expects(self::once())
+			->method('getFirstNodeById')->with(123)
+			->willThrowException(new \RuntimeException('Cleanup failed'));
+		$this->documentService->expects(self::never())->method('markAttachmentCleanup');
+		$this->expectException(\RuntimeException::class);
+		$this->expectExceptionMessage('Cleanup failed');
+
+		$this->cleanup->run([]);
+	}
+
+	public function testNotPermittedCleanupIsMarkedAndJobContinues(): void {
+		$this->documentService->method('hasUnsavedChanges')->willReturn(false);
+		$this->rootFolder->expects(self::once())
+			->method('getFirstNodeById')->with(123)
+			->willThrowException(new NotPermittedException());
+		$this->logger->expects(self::once())->method('warning');
+		$this->documentService->expects(self::once())->method('markAttachmentCleanup')->with($this->document);
+		$this->sessionService->expects(self::once())->method('removeInactiveSessionsWithoutSteps');
+		$this->sessionService->expects(self::once())->method('removeOldSessions');
+		$this->sessionService->expects(self::once())->method('removeOrphanedSteps');
+		$this->documentService->expects(self::once())->method('cleanupOldDocumentsFolders');
+
+		$this->cleanup->run([]);
+	}
+}

tests/unit/Db/DocumentMapperTest.php

@@ -0,0 +1,78 @@
+<?php
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\Text\Db;
+
+use OCP\Server;
+
+/**
+ * @group DB
+ */
+class DocumentMapperTest extends \Test\TestCase {
+	private DocumentMapper $documentMapper;
+	private SessionMapper $sessionMapper;
+	private Document $document;
+
+	public function setUp(): void {
+		parent::setUp();
+		$this->documentMapper = Server::get(DocumentMapper::class);
+		$this->sessionMapper = Server::get(SessionMapper::class);
+		$this->document = $this->documentMapper->insert(Document::fromParams([
+			'id' => 900001,
+			'lastSavedVersionTime' => 1000,
+			'lastSavedVersionEtag' => 'saved-etag',
+			'baseVersionEtag' => 'base-etag',
+		]));
+	}
+
+	public function tearDown(): void {
+		$this->sessionMapper->deleteByDocumentId($this->document->getId());
+		$this->documentMapper->delete($this->document);
+		parent::tearDown();
+	}
+
+	private function pendingDocumentIds(): array {
+		return array_map(
+			static fn (Document $document): int => $document->getId(),
+			iterator_to_array($this->documentMapper->findAllPendingAttachmentCleanup()),
+		);
+	}
+
+	public function testUnprocessedDocumentsArePending(): void {
+		self::assertSame(0, $this->documentMapper->find($this->document->getId())->getLastAttachmentCleanup());
+		self::assertContains($this->document->getId(), $this->pendingDocumentIds());
+	}
+
+	public function testDocumentsWithSessionsAreExcluded(): void {
+		$session = $this->sessionMapper->insert(Session::fromParams([
+			'documentId' => $this->document->getId(),
+			'userId' => 'admin',
+			'token' => uniqid(),
+			'lastContact' => time(),
+		]));
+		self::assertNotContains($this->document->getId(), $this->pendingDocumentIds());
+		$this->sessionMapper->delete($session);
+		self::assertContains($this->document->getId(), $this->pendingDocumentIds());
+	}
+
+	public function testCleanedDocumentsAreExcludedUntilSavedAgain(): void {
+		$this->document->setLastAttachmentCleanup(1000);
+		$this->documentMapper->update($this->document);
+		self::assertNotContains($this->document->getId(), $this->pendingDocumentIds());
+
+		$this->document->setLastSavedVersionTime(1001);
+		$this->documentMapper->update($this->document);
+		self::assertContains($this->document->getId(), $this->pendingDocumentIds());
+	}
+
+	public function testOlderSavedTimestampsAreExcluded(): void {
+		$this->document->setLastAttachmentCleanup(1001);
+		$this->documentMapper->update($this->document);
+
+		self::assertNotContains($this->document->getId(), $this->pendingDocumentIds());
+	}
+}