From 37385ba3256eb229a72aa07a657147b8385885dc Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 11 Sep 2026 17:13:11 -0400 Subject: [PATCH 1/3] perf(files): avoid materializing directory listings during scans Process storage directory listings lazily instead of materializing the complete listing with `iterator_to_array()`. Defer transaction creation until the first entry or removal is found, while preserving rollback handling for lazy iterator failures and the existing recursion boundary. Assisted-by: GitHub Copilot:gpt-5.6-luna Signed-off-by: Josh --- lib/private/Files/Cache/Scanner.php | 190 ++++++++++++++++++---------- 1 file changed, 120 insertions(+), 70 deletions(-) diff --git a/lib/private/Files/Cache/Scanner.php b/lib/private/Files/Cache/Scanner.php index 0b71ef3539e3b..da426ee64fc31 100644 --- a/lib/private/Files/Cache/Scanner.php +++ b/lib/private/Files/Cache/Scanner.php @@ -443,96 +443,146 @@ protected function scanChildren(string $path, $recursive, int $reuse, int $folde } /** + * Process the current directory before recursively scanning child directories. + * + * Keeping this work in a separate method releases the current directory's local + * state before scanChildren() descends into the child queue. + * * @param bool|IScanner::SCAN_RECURSIVE_INCOMPLETE $recursive */ - private function handleChildren(string $path, $recursive, int $reuse, int $folderId, bool $lock, int|float &$size, bool &$etagChanged): array { - // we put this in its own function so it cleans up the memory before we start recursing + private function handleChildren( + string $path, + $recursive, + int $reuse, + int $folderId, + bool $lock, + int|float &$size, + bool &$etagChanged, + ): array { $existingChildren = $this->getExistingChildren($folderId); - $newChildren = iterator_to_array($this->storage->getDirectoryContent($path)); - - if (count($existingChildren) === 0 && count($newChildren) === 0) { - // no need to do a transaction - return []; - } - - if ($this->useTransactions) { - $this->connection->beginTransaction(); - } + $processingStarted = false; $exceptionOccurred = false; $childQueue = []; $newChildNames = []; - foreach ($newChildren as $fileMeta) { - $permissions = $fileMeta['scan_permissions'] ?? $fileMeta['permissions']; - if ($permissions === 0) { - continue; - } - $originalFile = $fileMeta['name']; - $file = trim(Filesystem::normalizePath($originalFile), '/'); - if (trim($originalFile, '/') !== $file) { - // encoding mismatch, might require compatibility wrapper - Server::get(LoggerInterface::class)->debug('Scanner: Skipping non-normalized file name "' . $originalFile . '" in path "' . $path . '".', ['app' => 'core']); - $this->emit('\OC\Files\Cache\Scanner', 'normalizedNameMismatch', [$path ? $path . '/' . $originalFile : $originalFile]); - // skip this entry - continue; - } - $newChildNames[] = $file; - $child = $path ? $path . '/' . $file : $file; - try { - $existingData = $existingChildren[$file] ?? false; - $data = $this->scanFile($child, $reuse, $folderId, $existingData, $lock, $fileMeta); - if ($data) { - if ($data['mimetype'] === 'httpd/unix-directory' && $recursive === self::SCAN_RECURSIVE) { - $childQueue[$child] = [$data['fileid'], $data['size']]; - } elseif ($data['mimetype'] === 'httpd/unix-directory' && $recursive === self::SCAN_RECURSIVE_INCOMPLETE && $data['size'] === -1) { - // only recurse into folders which aren't fully scanned - $childQueue[$child] = [$data['fileid'], $data['size']]; - } elseif ($data['size'] === -1) { - $size = -1; - } elseif ($size !== -1) { - $size += $data['size']; + try { + foreach ($this->storage->getDirectoryContent($path) as $fileMeta) { + // Avoid opening a transaction for an empty directory with no cached children. + if (!$processingStarted) { + if ($this->useTransactions) { + $this->connection->beginTransaction(); } + $processingStarted = true; + } + + $permissions = $fileMeta['scan_permissions'] ?? $fileMeta['permissions']; + if ($permissions === 0) { + continue; + } + + $originalFile = $fileMeta['name']; + $file = trim(Filesystem::normalizePath($originalFile), '/'); + + if (trim($originalFile, '/') !== $file) { + // Non-normalized names cannot be addressed consistently through the cache; may require compatibility wrapper. + Server::get(LoggerInterface::class)->debug( + 'Scanner: Skipping non-normalized file name "' . $originalFile . '" in path "' . $path . '".', + ['app' => 'core'] + ); + $this->emit( + '\OC\Files\Cache\Scanner', + 'normalizedNameMismatch', + [$path ? $path . '/' . $originalFile : $originalFile] + ); + continue; + } + + $newChildNames[] = $file; + $child = $path ? $path . '/' . $file : $file; + + try { + $existingData = $existingChildren[$file] ?? false; + $data = $this->scanFile($child, $reuse, $folderId, $existingData, $lock, $fileMeta); + + if ($data) { + if ( + $data['mimetype'] === 'httpd/unix-directory' + && $recursive === self::SCAN_RECURSIVE + ) { + $childQueue[$child] = [$data['fileid'], $data['size']]; + } elseif ( + $data['mimetype'] === 'httpd/unix-directory' + && $recursive === self::SCAN_RECURSIVE_INCOMPLETE + && $data['size'] === -1 + ) { + // In incomplete scans, recurse only into folders that still need scanning. + $childQueue[$child] = [$data['fileid'], $data['size']]; + } elseif ($data['size'] === -1) { + $size = -1; + } elseif ($size !== -1) { + $size += $data['size']; + } - if (isset($data['etag_changed']) && $data['etag_changed']) { - $etagChanged = true; + if (isset($data['etag_changed']) && $data['etag_changed']) { + $etagChanged = true; + } + } + } catch (Exception $ex) { + // A concurrent scanner may have inserted this entry already. + if ($this->useTransactions) { + $this->connection->rollback(); + $this->connection->beginTransaction(); + } + Server::get(LoggerInterface::class)->debug( + 'Exception while scanning file "' . $child . '"', + ['app' => 'core', 'exception' => $ex] + ); + $exceptionOccurred = true; + } catch (LockedException $e) { + if ($this->useTransactions) { + $this->connection->rollback(); } + throw $e; } - } catch (Exception $ex) { - // might happen if inserting duplicate while a scanning - // process is running in parallel - // log and ignore + } + + if (!$processingStarted && $existingChildren === []) { + return []; + } + + $removedChildren = \array_diff(array_keys($existingChildren), $newChildNames); + + // Removed cached children still require a transaction when the listing is empty. + if ($removedChildren !== [] && !$processingStarted) { if ($this->useTransactions) { - $this->connection->rollback(); $this->connection->beginTransaction(); } - Server::get(LoggerInterface::class)->debug('Exception while scanning file "' . $child . '"', [ - 'app' => 'core', - 'exception' => $ex, - ]); - $exceptionOccurred = true; - } catch (LockedException $e) { - if ($this->useTransactions) { - $this->connection->rollback(); - } - throw $e; + $processingStarted = true; } + + foreach ($removedChildren as $childName) { + $child = $path ? $path . '/' . $childName : $childName; + $this->removeFromCache((string)$child); + } + + if ($processingStarted && $this->useTransactions) { + $this->connection->commit(); + } + } catch (\Throwable $e) { + // Lazy directory listings can fail after the transaction has started. + if ($this->useTransactions && $this->connection->inTransaction()) { + $this->connection->rollBack(); + } + + throw $e; } - $removedChildren = \array_diff(array_keys($existingChildren), $newChildNames); - foreach ($removedChildren as $childName) { - $child = $path ? $path . '/' . $childName : $childName; - $this->removeFromCache((string)$child); - } - if ($this->useTransactions) { - $this->connection->commit(); - } + if ($exceptionOccurred) { - // It might happen that the parallel scan process has already - // inserted mimetypes but those weren't available yet inside the transaction - // To make sure to have the updated mime types in such cases, - // we reload them here + // Reload MIME types that may have been inserted by a concurrent scan. Server::get(IMimeTypeLoader::class)->reset(); } + return $childQueue; } From 576963db16e0cf977622179b85ea1bc0d56cf7ed Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 11 Sep 2026 17:32:40 -0400 Subject: [PATCH 2/3] test(files): cover lazy directory scanning Verify directory entries are processed incrementally and that failures while consuming the listing roll back any active transaction. Assisted-by: GitHub Copilot:gpt-5.6-luna Signed-off-by: Josh --- tests/lib/Files/Cache/ScannerTest.php | 79 ++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/tests/lib/Files/Cache/ScannerTest.php b/tests/lib/Files/Cache/ScannerTest.php index 90c059238e3bb..690c15aea35ee 100644 --- a/tests/lib/Files/Cache/ScannerTest.php +++ b/tests/lib/Files/Cache/ScannerTest.php @@ -19,10 +19,38 @@ use OCP\Server; use Test\TestCase; +final class LazyDirectoryStorage extends Temporary { + public bool $firstChildProcessed = false; + public bool $resumeBeforeFirstChildProcessed = false; + public bool $throwAfterFirstEntry = false; + + #[\Override] + public function getDirectoryContent(string $directory): \Traversable { + $first = true; + + foreach (parent::getDirectoryContent($directory) as $entry) { + yield $entry; + + if (!$first) { + continue; + } + + $first = false; + + if (!$this->firstChildProcessed) { + $this->resumedBeforeFirstChildProcessed = true; + } + + if ($this->throwAfterFirstEntry) { + throw new \RuntimeException('Directory listing failed after the first entry'); + } + } + } +} + /** * Class ScannerTest * - * * @package Test\Files\Cache */ #[\PHPUnit\Framework\Attributes\Group('DB')] @@ -125,6 +153,55 @@ public function testFolder(): void { $this->assertEquals($cachedDataFolder2['size'], $cachedDataText2['size']); } + public function testDirectoryContentIsConsumedLazily(): void { + $storage = new LazyDirectoryStorage(); + $storage->file_put_contents('file.txt', 'content'); + + $scanner = new Scanner($storage); + $scanner->listen( + '\OC\Files\Cache\Scanner', + 'scanFile', + function (string $path) use ($storage): void { + if ($path === 'file.txt') { + $storage->firstChildProcessed = true; + } + }, + ); + + $scanner->scan(''); + + $this->assertTrue($storage->firstChildProcessed); + $this->assertFalse($storage->resumedBeforeFirstChildProcessed); + $this->assertTrue($storage->getCache()->inCache('file.txt')); + } + + public function testDirectoryContentFailureRollsBackTransaction(): void { + $storage = new LazyDirectoryStorage(); + $storage->file_put_contents('file.txt', 'content'); + $storage->throwAfterFirstEntry = true; + + $scanner = new Scanner($storage); + $connection = Server::get(IDBConnection::class); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Directory listing failed after the first entry'); + + try { + $scanner->scan(''); + } finally { + $this->assertFalse($connection->inTransaction()); + } + } + + public function testEmptyDirectoryScanDoesNotLeaveTransactionOpen(): void { + $storage = new LazyDirectoryStorage([]); + $scanner = new Scanner($storage); + + $scanner->scan(''); + + $this->assertFalse(Server::get(IDBConnection::class)->inTransaction()); + } + public function testShallow(): void { $this->fillTestFolders(); From 4c063ff310884975ffe3766d13fc5d7151c1a824 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 11 Sep 2026 17:43:10 -0400 Subject: [PATCH 3/3] test(files): fix typo in ScannerTest property Signed-off-by: Josh --- tests/lib/Files/Cache/ScannerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lib/Files/Cache/ScannerTest.php b/tests/lib/Files/Cache/ScannerTest.php index 690c15aea35ee..f1bb3a99a762e 100644 --- a/tests/lib/Files/Cache/ScannerTest.php +++ b/tests/lib/Files/Cache/ScannerTest.php @@ -21,7 +21,7 @@ final class LazyDirectoryStorage extends Temporary { public bool $firstChildProcessed = false; - public bool $resumeBeforeFirstChildProcessed = false; + public bool $resumedBeforeFirstChildProcessed = false; public bool $throwAfterFirstEntry = false; #[\Override]