Merge pull request #57360 from nextcloud/fix/trashbin-atomic-cache

fix(trashbin): keep cache and db consistent

apps/files_trashbin/lib/Trashbin.php

@@ -299,18 +299,65 @@ class Trashbin implements IEventListener {
 
 		$configuredTrashbinSize = static::getConfiguredTrashbinSize($owner);
 		if ($configuredTrashbinSize >= 0 && $sourceInfo->getSize() >= $configuredTrashbinSize) {
+			$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
 			return false;
 		}
 
-		try {
-			$moveSuccessful = true;
+		// there is still a possibility that the file has been deleted by a remote user
+		$deletedBy = self::overwriteDeletedBy($user);
 
+		$query = Server::get(IDBConnection::class)->getQueryBuilder();
+		$query->insert('files_trash')
+			->setValue('id', $query->createNamedParameter($filename))
+			->setValue('timestamp', $query->createNamedParameter($timestamp))
+			->setValue('location', $query->createNamedParameter($location))
+			->setValue('user', $query->createNamedParameter($owner))
+			->setValue('deleted_by', $query->createNamedParameter($deletedBy));
+		$inserted = false;
+		try {
+			$inserted = ($query->executeStatement() === 1);
+		} catch (\Throwable $e) {
+			Server::get(LoggerInterface::class)->error(
+				'trash bin database insert failed',
+				[
+					'app' => 'files_trashbin',
+					'exception' => $e,
+					'user' => $owner,
+					'filename' => $filename,
+					'timestamp' => $timestamp,
+				]
+			);
+		}
+		if (!$inserted) {
+			Server::get(LoggerInterface::class)->error(
+				'trash bin database couldn\'t be updated, skipping trash move',
+				[
+					'app' => 'files_trashbin',
+					'user' => $owner,
+					'filename' => $filename,
+					'timestamp' => $timestamp,
+				]
+			);
+			$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+			return false;
+		}
+
+		$moveSuccessful = true;
+		try {
 			$inCache = $sourceStorage->getCache()->inCache($sourceInternalPath);
 			$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
 			if ($inCache) {
 				$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
+			} else {
+				$sizeDifference = $sourceInfo->getSize();
+				if ($sizeDifference < 0) {
+					$sizeDifference = null;
+				} else {
+					$sizeDifference = (int)$sizeDifference;
+				}
+				$trashStorage->getUpdater()->update($trashInternalPath, null, $sizeDifference);
 			}
-		} catch (CopyRecursiveException $e) {
+		} catch (\Exception $e) {
 			$moveSuccessful = false;
 			if ($trashStorage->file_exists($trashInternalPath)) {
 				$trashStorage->unlink($trashInternalPath);
@@ -331,24 +378,31 @@ class Trashbin implements IEventListener {
 			} else {
 				$trashStorage->getUpdater()->remove($trashInternalPath);
 			}
-			return false;
+			$moveSuccessful = false;
+		}
+
+		if (!$moveSuccessful) {
+			Server::get(LoggerInterface::class)->error(
+				'trash move failed, removing trash metadata and payload',
+				[
+					'app' => 'files_trashbin',
+					'user' => $owner,
+					'filename' => $filename,
+					'timestamp' => $timestamp,
+				]
+			);
+			self::deleteTrashRow($user, $filename, $timestamp);
+			if ($trashStorage->file_exists($trashInternalPath)) {
+				if ($trashStorage->is_dir($trashInternalPath)) {
+					$trashStorage->rmdir($trashInternalPath);
+				} else {
+					$trashStorage->unlink($trashInternalPath);
+				}
+			}
+			$trashStorage->getUpdater()->remove($trashInternalPath);
 		}
 
 		if ($moveSuccessful) {
-			// there is still a possibility that the file has been deleted by a remote user
-			$deletedBy = self::overwriteDeletedBy($user);
-
-			$query = Server::get(IDBConnection::class)->getQueryBuilder();
-			$query->insert('files_trash')
-				->setValue('id', $query->createNamedParameter($filename))
-				->setValue('timestamp', $query->createNamedParameter($timestamp))
-				->setValue('location', $query->createNamedParameter($location))
-				->setValue('user', $query->createNamedParameter($owner))
-				->setValue('deleted_by', $query->createNamedParameter($deletedBy));
-			$result = $query->executeStatement();
-			if (!$result) {
-				Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated', ['app' => 'files_trashbin']);
-			}
 			Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path),
 				'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]);
 
@@ -545,12 +599,7 @@ class Trashbin implements IEventListener {
 			self::restoreVersions($view, $file, $filename, $uniqueFilename, $location, $timestamp);
 
 			if ($timestamp) {
-				$query = Server::get(IDBConnection::class)->getQueryBuilder();
-				$query->delete('files_trash')
-					->where($query->expr()->eq('user', $query->createNamedParameter($user)))
-					->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
-					->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
-				$query->executeStatement();
+				self::deleteTrashRow($user, $filename, $timestamp);
 			}
 
 			return true;
@@ -689,13 +738,6 @@ class Trashbin implements IEventListener {
 		$size = 0;
 
 		if ($timestamp) {
-			$query = Server::get(IDBConnection::class)->getQueryBuilder();
-			$query->delete('files_trash')
-				->where($query->expr()->eq('user', $query->createNamedParameter($user)))
-				->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
-				->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
-			$query->executeStatement();
-
 			$file = static::getTrashFilename($filename, $timestamp);
 		} else {
 			$file = $filename;
@@ -706,6 +748,9 @@ class Trashbin implements IEventListener {
 		try {
 			$node = $userRoot->get('/files_trashbin/files/' . $file);
 		} catch (NotFoundException $e) {
+			if ($timestamp) {
+				self::deleteTrashRow($user, $filename, $timestamp);
+			}
 			return $size;
 		}
 
@@ -719,9 +764,22 @@ class Trashbin implements IEventListener {
 		$node->delete();
 		self::emitTrashbinPostDelete('/files_trashbin/files/' . $file);
 
+		if ($timestamp) {
+			self::deleteTrashRow($user, $filename, $timestamp);
+		}
+
 		return $size;
 	}
 
+	private static function deleteTrashRow(string $user, string $filename, int $timestamp): void {
+		$query = Server::get(IDBConnection::class)->getQueryBuilder();
+		$query->delete('files_trash')
+			->where($query->expr()->eq('user', $query->createNamedParameter($user)))
+			->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
+			->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
+		$query->executeStatement();
+	}
+
 	/**
 	 * @param string $file
 	 * @param string $filename

apps/files_trashbin/tests/StorageTest.php

@@ -6,16 +6,21 @@ declare(strict_types=1);
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
+
 namespace OCA\Files_Trashbin\Tests;
 
+use OC\Files\Cache\Updater;
 use OC\Files\Filesystem;
+use OC\Files\ObjectStore\ObjectStoreStorage;
 use OC\Files\Storage\Common;
+use OC\Files\Storage\Local;
 use OC\Files\Storage\Temporary;
 use OC\Files\View;
 use OCA\Files_Trashbin\AppInfo\Application;
 use OCA\Files_Trashbin\Events\MoveToTrashEvent;
 use OCA\Files_Trashbin\Storage;
 use OCA\Files_Trashbin\Trash\ITrashManager;
+use OCA\Files_Trashbin\Trashbin;
 use OCP\AppFramework\Bootstrap\IBootContext;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Constants;
@@ -118,6 +123,84 @@ class StorageTest extends \Test\TestCase {
 		$this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.')));
 	}
 
+	public function testTrashEntryCreatedWhenSourceNotInCache(): void {
+		$this->userView->file_put_contents('uncached.txt', 'foo');
+
+		[$storage, $internalPath] = $this->userView->resolvePath('uncached.txt');
+		if ($storage->instanceOfStorage(ObjectStoreStorage::class)) {
+			$this->markTestSkipped('object store always has the file in cache');
+		}
+		$cache = $storage->getCache();
+		$cache->remove($internalPath);
+		$this->assertFalse($cache->inCache($internalPath));
+
+		$this->userView->unlink('uncached.txt');
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
+		$this->assertCount(1, $results);
+		$name = $results[0]->getName();
+		$this->assertEquals('uncached.txt', substr($name, 0, strrpos($name, '.')));
+
+		[$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/' . $name);
+		$this->assertTrue($trashStorage->getCache()->inCache($trashInternalPath));
+	}
+
+	public function testTrashEntryNotCreatedWhenDeleteFailed(): void {
+		$storage2 = $this->getMockBuilder(Temporary::class)
+			->setConstructorArgs([])
+			->onlyMethods(['unlink', 'instanceOfStorage'])
+			->getMock();
+		$storage2->method('unlink')
+			->willReturn(false);
+
+		// disable same-storage move optimization
+		$storage2->method('instanceOfStorage')
+			->willReturnCallback(fn (string $class) => ($class !== Local::class) && (new Temporary([]))->instanceOfStorage($class));
+
+
+		Filesystem::mount($storage2, [], $this->user . '/files/substorage');
+		$this->userView->file_put_contents('substorage/test.txt', 'foo');
+
+		$this->assertFalse($this->userView->unlink('substorage/test.txt'));
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
+		$this->assertEmpty($results);
+
+		$trashData = Trashbin::getExtraData($this->user);
+		$this->assertEmpty($trashData);
+	}
+
+	public function testTrashEntryNotCreatedWhenCacheRowFailed(): void {
+		$trashStorage = $this->getMockBuilder(Temporary::class)
+			->setConstructorArgs([])
+			->onlyMethods(['getUpdater'])
+			->getMock();
+		$updater = $this->getMockBuilder(Updater::class)
+			->setConstructorArgs([$trashStorage])
+			->onlyMethods(['renameFromStorage'])
+			->getMock();
+		$trashStorage->method('getUpdater')
+			->willReturn($updater);
+		$updater->method('renameFromStorage')
+			->willThrowException(new \Exception());
+
+		Filesystem::mount($trashStorage, [], $this->user . '/files_trashbin');
+		$this->userView->file_put_contents('test.txt', 'foo');
+
+		try {
+			$this->assertFalse($this->userView->unlink('test.txt'));
+			$this->fail();
+		} catch (\Exception) {
+			// expected
+		}
+
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
+		$this->assertEmpty($results);
+
+		$trashData = Trashbin::getExtraData($this->user);
+		$this->assertEmpty($trashData);
+	}
+
 	/**
 	 * Test that deleting a folder puts it into the trashbin.
 	 */