From c9350a52410ba6b1222228bb582d78467f055322 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Thu, 10 Sep 2026 18:50:10 +0200 Subject: [PATCH] fix(files): clean up filecache companion rows of removed storages cleanByMountId() deleted the filecache rows of an unmounted storage but left their filecache_extended and file metadata rows behind. It now goes through Storage::removeFileCacheEntries(), the same path clear() uses. files:cleanup also removes filecache_extended, files_metadata and files_metadata_index rows whose file is no longer in the filecache, so rows orphaned before this fix are cleaned up too. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- .../files/lib/Command/DeleteOrphanedFiles.php | 89 ++++++++--------- .../tests/Command/DeleteOrphanedFilesTest.php | 99 +++++++++++++++++-- lib/private/Files/Cache/Cache.php | 28 +----- lib/private/Files/Cache/Storage.php | 46 +++++++-- tests/lib/Files/Cache/StorageTest.php | 67 +++++++++++++ 5 files changed, 235 insertions(+), 94 deletions(-) create mode 100644 tests/lib/Files/Cache/StorageTest.php diff --git a/apps/files/lib/Command/DeleteOrphanedFiles.php b/apps/files/lib/Command/DeleteOrphanedFiles.php index 0563584d75be2..be97f3bd88fa0 100644 --- a/apps/files/lib/Command/DeleteOrphanedFiles.php +++ b/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 - */ - 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 $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 { diff --git a/apps/files/tests/Command/DeleteOrphanedFilesTest.php b/apps/files/tests/Command/DeleteOrphanedFilesTest.php index 4745669cebeec..b51d683157d34 100644 --- a/apps/files/tests/Command/DeleteOrphanedFilesTest.php +++ b/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 $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 $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(); + } } diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index bee5c996d8932..689972cde834e 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -901,33 +901,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() { - $storageId = $this->getNumericStorageId(); - - while (true) { - $query = $this->getQueryBuilder(); - $query->select('fileid') - ->from('filecache') - ->whereStorageId($storageId) - ->setMaxResults(IQueryBuilder::MAX_IN_PARAMETERS); - $fileIds = array_map(intval(...), $query->executeQuery()->fetchFirstColumn()); - if ($fileIds === []) { - break; - } - - $query = $this->getQueryBuilder(); - $query->delete('filecache_extended') - ->where($query->expr()->in('fileid', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))) - ->hintShardKey('storage', $storageId); - $query->executeStatement(); - - $this->metadataManager->deleteMetadataForFiles($storageId, $fileIds); - - $query = $this->getQueryBuilder(); - $query->delete('filecache') - ->whereStorageId($storageId) - ->andWhere($query->expr()->in('fileid', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); - $query->executeStatement(); - } + Storage::removeFileCacheEntries($this->getNumericStorageId()); $query = $this->connection->getQueryBuilder(); $query->delete('storages') diff --git a/lib/private/Files/Cache/Storage.php b/lib/private/Files/Cache/Storage.php index 4b9a074db5b73..7f8869d69bbb1 100644 --- a/lib/private/Files/Cache/Storage.php +++ b/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(); + } + } } diff --git a/tests/lib/Files/Cache/StorageTest.php b/tests/lib/Files/Cache/StorageTest.php new file mode 100644 index 0000000000000..7d8dfe98b8ff0 --- /dev/null +++ b/tests/lib/Files/Cache/StorageTest.php @@ -0,0 +1,67 @@ +getCache(); + $rootId = $cache->put('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]); + $fileId = $cache->put('foo.txt', ['size' => 1, 'mtime' => 1, 'mimetype' => 'text/plain', 'upload_time' => 25]); + + $metadataManager = Server::get(IFilesMetadataManager::class); + $metadata = $metadataManager->getMetadata($fileId, true); + $metadata->setString('test-key', 'value', true); + $metadataManager->saveMetadata($metadata); + + $mountId = random_int(100000000, 999999999); + $mountPoint = '/' . $this->getUniqueID('user') . '/files/mount/'; + $qb = Server::get(IDBConnection::class)->getQueryBuilder(); + $qb->insert('mounts') + ->values([ + 'storage_id' => $qb->createNamedParameter($cache->getNumericStorageId(), IQueryBuilder::PARAM_INT), + 'root_id' => $qb->createNamedParameter($rootId, IQueryBuilder::PARAM_INT), + 'user_id' => $qb->createNamedParameter('test'), + 'mount_point' => $qb->createNamedParameter($mountPoint), + 'mount_point_hash' => $qb->createNamedParameter(hash('xxh128', $mountPoint)), + 'mount_id' => $qb->createNamedParameter($mountId, IQueryBuilder::PARAM_INT), + ]) + ->executeStatement(); + + $this->assertSame(1, $this->countRows('filecache_extended', 'fileid', $fileId)); + $this->assertSame(1, $this->countRows('files_metadata', 'file_id', $fileId)); + $this->assertSame(1, $this->countRows('files_metadata_index', 'file_id', $fileId)); + + Storage::cleanByMountId($mountId); + + $this->assertSame(0, $this->countRows('filecache', 'fileid', $fileId)); + $this->assertSame(0, $this->countRows('filecache_extended', 'fileid', $fileId)); + $this->assertSame(0, $this->countRows('files_metadata', 'file_id', $fileId)); + $this->assertSame(0, $this->countRows('files_metadata_index', 'file_id', $fileId)); + } + + private function countRows(string $table, string $column, int $fileId): int { + $qb = Server::get(IDBConnection::class)->getQueryBuilder(); + $qb->select($qb->func()->count()) + ->from($table) + ->where($qb->expr()->eq($column, $qb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT))); + return (int)$qb->executeQuery()->fetchOne(); + } +}