fix(cron): do not abort cleanup job when attachments cannot be deleted

lib/Cron/Cleanup.php

@@ -15,6 +15,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 {
@@ -41,7 +42,12 @@ protected function run($argument): void {
 				continue;
 			}
 			$this->setupManager->tearDown();
-			$this->attachmentService->cleanupAttachments($document->getContextId());
+			try {
+				$this->attachmentService->cleanupAttachments($document->getContextId());
+			} 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->getContextId() . ', deleting is not permitted', ['exception' => $e]);
+			}
 			$this->documentService->markAttachmentCleanup($document);
 		}
 

tests/unit/CleanupTest.php

@@ -19,6 +19,7 @@
 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;
@@ -34,6 +35,7 @@ class CleanupTest extends \PHPUnit\Framework\TestCase {
 	private SessionService $sessionService;
 	private IRootFolder $rootFolder;
 	private ISetupManager $setupManager;
+	private LoggerInterface $logger;
 
 	public function setUp(): void {
 		$this->document = Document::fromRow([
@@ -57,13 +59,14 @@ public function setUp(): void {
 		);
 		$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->createMock(LoggerInterface::class), $this->setupManager, ) extends Cleanup {
+		$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);
 			}
@@ -115,4 +118,19 @@ public function testFailedCleanupRemainsPending(): void {
 
 		$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([]);
+	}
 }