Merge pull request #63376 from nextcloud/fix/preview-keep-local-file-on-race

fix(preview): keep the local file when losing the insert race

Author: CarlSchwan

lib/private/Preview/Generator.php

@@ -594,7 +594,7 @@ public function savePreview(Preview $previewEntry, IImage $preview): Preview {
 				throw $e;
 			}
 
-			$this->storageFactory->deletePreview($previewEntry);
+			$this->storageFactory->deleteUnreferencedPreview($previewEntry);
 
 			$this->logger->debug('Generating a preview but one already exists.', ['exception' => $e]);
 

lib/private/Preview/Storage/IPreviewStorage.php

@@ -35,6 +35,19 @@ public function readPreview(Preview $preview): mixed;
 	 */
 	public function deletePreview(Preview $preview): void;
 
+	/**
+	 * Delete the stored data of a preview that never got a database row, for
+	 * example after losing the insert race on the unique constraint.
+	 *
+	 * Only backends that key their storage by the preview id hold data that
+	 * belongs to this entity alone. The local storage derives its path from
+	 * the preview specification, so the file is the very same one the winner
+	 * of the race is now referencing and has to be kept.
+	 *
+	 * @throws NotPermittedException
+	 */
+	public function deleteUnreferencedPreview(Preview $preview): void;
+
 	/**
 	 * Migration helper
 	 *

lib/private/Preview/Storage/LocalPreviewStorage.php

@@ -69,6 +69,13 @@ public function deletePreview(Preview $preview): void {
 		}
 	}
 
+	#[Override]
+	public function deleteUnreferencedPreview(Preview $preview): void {
+		// constructPath() keys on the file id and the preview specification, not
+		// on the preview id, so this file is shared with the preview that won the
+		// race. Deleting it would leave that one with a row but no file.
+	}
+
 	public function getRootFolder(): string {
 		return $this->config->getSystemValueString('datadirectory', OC::$SERVERROOT . '/data');
 	}

lib/private/Preview/Storage/ObjectStorePreviewStorage.php

@@ -167,6 +167,13 @@ private function getObjectStore(string $objectStoreName, array $config): IObject
 		return $this->objectStoreCache[$objectStoreName][$bucketName];
 	}
 
+	#[Override]
+	public function deleteUnreferencedPreview(Preview $preview): void {
+		// The urn embeds the preview id, so this object belongs to this entity
+		// alone and can be removed safely.
+		$this->deletePreview($preview);
+	}
+
 	public function getUrn(Preview $preview, array $config): string {
 		if ($preview->getOldFileId()) {
 			return ($config['arguments']['objectPrefix'] ?? 'urn:oid:') . $preview->getOldFileId();

lib/private/Preview/Storage/StorageFactory.php

@@ -37,6 +37,11 @@ public function deletePreview(Preview $preview): void {
 		$this->getBackend()->deletePreview($preview);
 	}
 
+	#[Override]
+	public function deleteUnreferencedPreview(Preview $preview): void {
+		$this->getBackend()->deleteUnreferencedPreview($preview);
+	}
+
 	private function getBackend(): IPreviewStorage {
 		if ($this->backend) {
 			return $this->backend;

tests/lib/Preview/Storage/LocalPreviewStorageTest.php

@@ -9,6 +9,7 @@
 
 namespace Test\Preview\Storage;
 
+use OC\Preview\Db\Preview;
 use OC\Preview\Db\PreviewMapper;
 use OC\Preview\Storage\LocalPreviewStorage;
 use OCP\DB\Exception as DBException;
@@ -19,6 +20,7 @@
 use OCP\Files\IMimeTypeDetector;
 use OCP\Files\IMimeTypeLoader;
 use OCP\Files\IRootFolder;
+use OCP\Files\NotFoundException;
 use OCP\IAppConfig;
 use OCP\IConfig;
 use OCP\IDBConnection;
@@ -287,4 +289,43 @@ public function testScanFetchesAllFilecacheRows(): void {
 
 		$this->assertSame(3, $count);
 	}
+
+	private function makePreview(int $fileId = self::FILE_ID): Preview {
+		$preview = new Preview();
+		$preview->setFileId($fileId);
+		$preview->setWidth(1024);
+		$preview->setHeight(768);
+		$preview->setCropped(false);
+		$preview->setMax(true);
+		$preview->setMimetype('image/jpeg');
+
+		return $preview;
+	}
+
+	/**
+	 * Losing the insert race must not remove the local file: the path is derived
+	 * from the preview specification, so it is the same file the preview that
+	 * won the race now points at.
+	 */
+	public function testDeleteUnreferencedPreviewKeepsTheSharedFile(): void {
+		$preview = $this->makePreview();
+		$this->storage->writePreview($preview, 'preview data');
+
+		$this->storage->deleteUnreferencedPreview($preview);
+
+		$this->assertSame('preview data', stream_get_contents($this->storage->readPreview($preview)));
+	}
+
+	/**
+	 * Deleting a preview for real still has to remove the file.
+	 */
+	public function testDeletePreviewRemovesTheFile(): void {
+		$preview = $this->makePreview();
+		$this->storage->writePreview($preview, 'preview data');
+
+		$this->storage->deletePreview($preview);
+
+		$this->expectException(NotFoundException::class);
+		$this->storage->readPreview($preview);
+	}
 }