[stable35] fix(filecache): announce every removed entry so metadata is cleaned up

apps/files/lib/Command/DeleteOrphanedFiles.php

@@ -18,12 +18,13 @@
 use OCP\IDBConnection;
 
 /**
- * Delete all file entries that have no matching entries in the storage table.
+ * Delete all file entries that have no matching entries in the storage table,
+ * and the rows keyed by file id that have no matching file entry.
  */
 #[AsCommand(
 	name: 'files:cleanup',
 	description: 'Clean up orphaned filecache and mount entries',
-	help: 'Deletes orphaned filecache and mount entries (those without an existing storage).',
+	help: 'Deletes orphaned filecache and mount entries (those without an existing storage), and filecache_extended and file metadata entries without a filecache entry.',
 )]
 class DeleteOrphanedFiles {
 	public const int CHUNK_SIZE = 200;
@@ -38,23 +39,22 @@ public function __invoke(
 		#[Option(name: 'skip-filecache-extended', description: 'don\'t remove orphaned entries from filecache_extended')]
 		bool $skipFilecacheExtended = false,
 	): ExitCode {
-		$fileIdsByStorage = [];
-
 		$deletedStorages = array_diff($this->getReferencedStorages(), $this->getExistingStorages());
 
-		$deleteExtended = !$skipFilecacheExtended;
-		if ($deleteExtended) {
-			$fileIdsByStorage = $this->getFileIdsForStorages($deletedStorages);
-		}
-
 		$deletedEntries = $this->cleanupOrphanedFileCache($deletedStorages);
 		$output->writeln("$deletedEntries orphaned file cache entries deleted");
 
-		if ($deleteExtended) {
-			$deletedFileCacheExtended = $this->cleanupOrphanedFileCacheExtended($fileIdsByStorage);
+		if (!$skipFilecacheExtended) {
+			$deletedFileCacheExtended = $this->cleanupEntriesWithoutFileCache('filecache_extended', 'fileid');
 			$output->writeln("$deletedFileCacheExtended orphaned file cache extended entries deleted");
 		}
 
+		$deletedMetadata = $this->cleanupEntriesWithoutFileCache('files_metadata', 'file_id');
+		$output->writeln("$deletedMetadata orphaned file metadata entries deleted");
+
+		$deletedMetadataIndex = $this->cleanupEntriesWithoutFileCache('files_metadata_index', 'file_id');
+		$output->writeln("$deletedMetadataIndex orphaned file metadata index entries deleted");
+
 		$deletedMounts = $this->cleanupOrphanedMounts();
 		$output->writeln("$deletedMounts orphaned mount entries deleted");
 
@@ -78,28 +78,6 @@ private function getExistingStorages(): array {
 		return $query->executeQuery()->fetchFirstColumn();
 	}
 
-	/**
-	 * @param int[] $storageIds
-	 * @return array<int, int[]>
-	 */
-	private function getFileIdsForStorages(array $storageIds): array {
-		$query = $this->connection->getQueryBuilder();
-		$query->select('storage', 'fileid')
-			->from('filecache')
-			->where($query->expr()->in('storage', $query->createParameter('storage_ids')));
-
-		$result = [];
-		$storageIdChunks = array_chunk($storageIds, self::CHUNK_SIZE);
-		foreach ($storageIdChunks as $storageIdChunk) {
-			$query->setParameter('storage_ids', $storageIdChunk, IQueryBuilder::PARAM_INT_ARRAY);
-			$chunk = $query->executeQuery()->fetchAllAssociative();
-			foreach ($chunk as $row) {
-				$result[$row['storage']][] = $row['fileid'];
-			}
-		}
-		return $result;
-	}
-
 	private function cleanupOrphanedFileCache(array $deletedStorages): int {
 		$deletedEntries = 0;
 
@@ -116,27 +94,38 @@ private function cleanupOrphanedFileCache(array $deletedStorages): int {
 		return $deletedEntries;
 	}
 
-	/**
-	 * @param array<int, int[]> $fileIdsByStorage
-	 * @return int
-	 */
-	private function cleanupOrphanedFileCacheExtended(array $fileIdsByStorage): int {
+	private function cleanupEntriesWithoutFileCache(string $table, string $fileIdColumn): int {
 		$deletedEntries = 0;
+		$lastFileId = 0;
+
+		while (true) {
+			$query = $this->connection->getQueryBuilder();
+			$query->select($fileIdColumn)
+				->from($table)
+				->where($query->expr()->gt($fileIdColumn, $query->createNamedParameter($lastFileId, IQueryBuilder::PARAM_INT)))
+				->orderBy($fileIdColumn)
+				->setMaxResults(IQueryBuilder::MAX_IN_PARAMETERS)
+				->runAcrossAllShards();
+			$fileIds = array_unique(array_map(intval(...), $query->executeQuery()->fetchFirstColumn()));
+			if ($fileIds === []) {
+				return $deletedEntries;
+			}
 
-		$deleteQuery = $this->connection->getQueryBuilder();
-		$deleteQuery->delete('filecache_extended')
-			->where($deleteQuery->expr()->in('fileid', $deleteQuery->createParameter('file_ids')));
-
-		foreach ($fileIdsByStorage as $storageId => $fileIds) {
-			$deleteQuery->hintShardKey('storage', $storageId, true);
-			$fileChunks = array_chunk($fileIds, self::CHUNK_SIZE);
-			foreach ($fileChunks as $fileChunk) {
-				$deleteQuery->setParameter('file_ids', $fileChunk, IQueryBuilder::PARAM_INT_ARRAY);
-				$deletedEntries += $deleteQuery->executeStatement();
+			$query = $this->connection->getQueryBuilder();
+			$query->select('fileid')
+				->from('filecache')
+				->where($query->expr()->in('fileid', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)));
+			$missingFileIds = array_diff($fileIds, $query->executeQuery()->fetchFirstColumn());
+
+			if ($missingFileIds !== []) {
+				$query = $this->connection->getQueryBuilder();
+				$query->delete($table)
+					->where($query->expr()->in($fileIdColumn, $query->createNamedParameter($missingFileIds, IQueryBuilder::PARAM_INT_ARRAY)));
+				$deletedEntries += $query->executeStatement();
 			}
-		}
 
-		return $deletedEntries;
+			$lastFileId = max($fileIds);
+		}
 	}
 
 	private function cleanupOrphanedMounts(): int {

apps/files/tests/Command/DeleteOrphanedFilesTest.php

@@ -9,11 +9,15 @@
 
 namespace OCA\Files\Tests\Command;
 
+use OC\Files\Storage\Temporary;
 use OC\Files\View;
 use OCA\Files\Command\DeleteOrphanedFiles;
 use OCP\Console\IOutput;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\Files\Cache\ICacheEntry;
 use OCP\Files\IRootFolder;
 use OCP\Files\StorageNotAvailableException;
+use OCP\FilesMetadata\IFilesMetadataManager;
 use OCP\IDBConnection;
 use OCP\IUserManager;
 use OCP\Server;
@@ -73,6 +77,31 @@ protected function getMountsCount(int $storageId): int {
 		return (int)$query->executeQuery()->fetchOne();
 	}
 
+	/**
+	 * @param list<int> $fileIds
+	 */
+	protected function countRows(string $table, string $column, array $fileIds): int {
+		// selecting rows instead of COUNT(*), which a sharded query answers once per shard
+		$query = $this->connection->getQueryBuilder();
+		$query->select($column)
+			->from($table)
+			->where($query->expr()->in($column, $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)));
+		return count($query->executeQuery()->fetchFirstColumn());
+	}
+
+	/**
+	 * @param list<string> $calls
+	 */
+	protected function expectOutput(IOutput&\PHPUnit\Framework\MockObject\MockObject $output, array $calls): void {
+		$output
+			->expects($this->exactly(count($calls)))
+			->method('writeln')
+			->willReturnCallback(function (string $message) use (&$calls): void {
+				$expected = array_shift($calls);
+				$this->assertSame($expected, $message);
+			});
+	}
+
 	/**
 	 * Test clearing orphaned files
 	 */
@@ -102,6 +131,16 @@ public function testClearFiles(): void {
 		$this->assertCount(1, $this->getFile($fileInfo->getId()), 'Asserts that file is still available');
 		$this->assertEquals(1, $this->getMountsCount($numericStorageId), 'Asserts that mount is still available');
 
+		$qb = $this->connection->getQueryBuilder();
+		$storageFileIds = array_map('intval', $qb->select('fileid')
+			->from('filecache')
+			->where($qb->expr()->eq('storage', $qb->createNamedParameter($numericStorageId, IQueryBuilder::PARAM_INT)))
+			->executeQuery()
+			->fetchFirstColumn());
+		$extendedEntries = $this->countRows('filecache_extended', 'fileid', $storageFileIds);
+		$metadataEntries = $this->countRows('files_metadata', 'file_id', $storageFileIds);
+		$metadataIndexEntries = $this->countRows('files_metadata_index', 'file_id', $storageFileIds);
+
 		$qb = $this->connection->getQueryBuilder();
 		$deletedRows = $qb->delete('storages')
 			->where($qb->expr()->eq('id', $qb->createNamedParameter($storageId)))
@@ -110,18 +149,13 @@ public function testClearFiles(): void {
 		$this->assertSame(1, $deletedRows, 'Asserts that storage got deleted');
 
 		// parent folder, `files`, ´test` and `welcome.txt` => 4 elements
-		$calls = [
+		$this->expectOutput($output, [
 			'3 orphaned file cache entries deleted',
-			'0 orphaned file cache extended entries deleted',
+			"$extendedEntries orphaned file cache extended entries deleted",
+			"$metadataEntries orphaned file metadata entries deleted",
+			"$metadataIndexEntries orphaned file metadata index entries deleted",
 			'1 orphaned mount entries deleted',
-		];
-		$output
-			->expects($this->exactly(3))
-			->method('writeln')
-			->willReturnCallback(function (string $message) use (&$calls): void {
-				$expected = array_shift($calls);
-				$this->assertSame($expected, $message);
-			});
+		]);
 
 		($this->command)($output);
 
@@ -136,4 +170,49 @@ public function testClearFiles(): void {
 		} catch (StorageNotAvailableException $e) {
 		}
 	}
+
+	public function testClearEntriesWithoutFileCacheEntry(): void {
+		// remove orphans left behind by other tests so that the counts below only cover this test
+		($this->command)($this->createMock(IOutput::class));
+
+		$storage = new Temporary([]);
+		$cache = $storage->getCache();
+		$cache->put('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
+		$data = ['size' => 1, 'mtime' => 1, 'mimetype' => 'text/plain', 'upload_time' => 25];
+		$orphanId = $cache->put('orphan.txt', $data);
+		$keptId = $cache->put('kept.txt', $data);
+
+		$metadataManager = Server::get(IFilesMetadataManager::class);
+		foreach ([$orphanId, $keptId] as $fileId) {
+			$metadata = $metadataManager->getMetadata($fileId, true);
+			$metadata->setString('test-key', 'value', true);
+			$metadataManager->saveMetadata($metadata);
+		}
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->delete('filecache')
+			->where($qb->expr()->eq('fileid', $qb->createNamedParameter($orphanId, IQueryBuilder::PARAM_INT)))
+			->executeStatement();
+
+		$output = $this->createMock(IOutput::class);
+		$this->expectOutput($output, [
+			'0 orphaned file cache entries deleted',
+			'1 orphaned file cache extended entries deleted',
+			'1 orphaned file metadata entries deleted',
+			'1 orphaned file metadata index entries deleted',
+			'0 orphaned mount entries deleted',
+		]);
+
+		($this->command)($output);
+
+		$this->assertSame(0, $this->countRows('filecache_extended', 'fileid', [$orphanId]));
+		$this->assertSame(0, $this->countRows('files_metadata', 'file_id', [$orphanId]));
+		$this->assertSame(0, $this->countRows('files_metadata_index', 'file_id', [$orphanId]));
+
+		$this->assertSame(1, $this->countRows('filecache_extended', 'fileid', [$keptId]));
+		$this->assertSame(1, $this->countRows('files_metadata', 'file_id', [$keptId]));
+		$this->assertSame(1, $this->countRows('files_metadata_index', 'file_id', [$keptId]));
+
+		$cache->clear();
+	}
 }

lib/private/Files/Cache/Cache.php

@@ -608,7 +608,9 @@ public function remove($file) {
 				$this->removeChildren($entry);
 			}
 
-			$this->eventDispatcher->dispatchTyped(new CacheEntryRemovedEvent($this->storage, $entry->getPath(), $entry->getId(), $this->getNumericStorageId()));
+			$event = new CacheEntryRemovedEvent($this->storage, $entry->getPath(), $entry->getId(), $this->getNumericStorageId());
+			$this->eventDispatcher->dispatchTyped($event);
+			$this->eventDispatcher->dispatchTyped(new CacheEntriesRemovedEvent([$event]));
 		}
 	}
 
@@ -679,8 +681,8 @@ private function removeChildren(ICacheEntry $entry) {
 			$query->executeStatement();
 		}
 
-		$cacheEntryRemovedEvents = [];
-		foreach (array_chunk(array_combine($deletedIds, $deletedPaths), IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) {
+		foreach (array_chunk(array_combine($deletedIds, $deletedPaths), IQueryBuilder::MAX_IN_PARAMETERS, true) as $chunk) {
+			$cacheEntryRemovedEvents = [];
 			/** @var array<int, string> $chunk */
 			foreach ($chunk as $fileId => $filePath) {
 				$cacheEntryRemovedEvents[] = new CacheEntryRemovedEvent(
@@ -900,10 +902,7 @@ private function getChildIds(int $storageId, string $path): array {
 	 * remove all entries for files that are stored on the storage from the cache
 	 */
 	public function clear() {
-		$query = $this->getQueryBuilder();
-		$query->delete('filecache')
-			->whereStorageId($this->getNumericStorageId());
-		$query->executeStatement();
+		Storage::removeFileCacheEntries($this->getNumericStorageId());
 
 		$query = $this->connection->getQueryBuilder();
 		$query->delete('storages')

lib/private/Files/Cache/Storage.php

@@ -13,6 +13,7 @@
 use OC\DB\Exceptions\DbalException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Files\Storage\IStorage;
+use OCP\FilesMetadata\IFilesMetadataManager;
 use OCP\IDBConnection;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
@@ -170,14 +171,11 @@ public static function cleanByMountId(int $mountId): void {
 			$query->select('storage_id')
 				->from('mounts')
 				->where($query->expr()->eq('mount_id', $query->createNamedParameter($mountId, IQueryBuilder::PARAM_INT)));
-			$storageIds = $query->executeQuery()->fetchFirstColumn();
-			$storageIds = array_unique($storageIds);
+			$storageIds = array_unique(array_map(intval(...), $query->executeQuery()->fetchFirstColumn()));
 
-			$query = $db->getQueryBuilder();
-			$query->delete('filecache')
-				->where($query->expr()->in('storage', $query->createNamedParameter($storageIds, IQueryBuilder::PARAM_INT_ARRAY)))
-				->runAcrossAllShards()
-				->executeStatement();
+			foreach ($storageIds as $storageId) {
+				self::removeFileCacheEntries($storageId);
+			}
 
 			$query = $db->getQueryBuilder();
 			$query->delete('storages')
@@ -195,4 +193,38 @@ public static function cleanByMountId(int $mountId): void {
 			throw $exception;
 		}
 	}
+
+	/**
+	 * Remove the filecache entries of a storage together with their filecache_extended and metadata rows
+	 */
+	public static function removeFileCacheEntries(int $numericStorageId): void {
+		$db = Server::get(IDBConnection::class);
+		$metadataManager = Server::get(IFilesMetadataManager::class);
+
+		while (true) {
+			$query = $db->getQueryBuilder();
+			$query->select('fileid')
+				->from('filecache')
+				->where($query->expr()->eq('storage', $query->createNamedParameter($numericStorageId, IQueryBuilder::PARAM_INT)))
+				->setMaxResults(IQueryBuilder::MAX_IN_PARAMETERS);
+			$fileIds = array_map(intval(...), $query->executeQuery()->fetchFirstColumn());
+			if ($fileIds === []) {
+				return;
+			}
+
+			$query = $db->getQueryBuilder();
+			$query->delete('filecache_extended')
+				->where($query->expr()->in('fileid', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)))
+				->hintShardKey('storage', $numericStorageId)
+				->executeStatement();
+
+			$metadataManager->deleteMetadataForFiles($numericStorageId, $fileIds);
+
+			$query = $db->getQueryBuilder();
+			$query->delete('filecache')
+				->where($query->expr()->eq('storage', $query->createNamedParameter($numericStorageId, IQueryBuilder::PARAM_INT)))
+				->andWhere($query->expr()->in('fileid', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)))
+				->executeStatement();
+		}
+	}
 }