From c713349304153b916851b16240b5a8cd07430656 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 20 Jul 2026 09:28:54 -0400 Subject: [PATCH 1/4] fix(view): release cache refresh locks on errors Signed-off-by: Josh --- lib/private/Files/View.php | 74 ++++++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 23 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 712b40b2380cb..8fcba6441cd59 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1383,48 +1383,76 @@ private function getUserObjectForOwner(string $ownerId): IUser { } /** - * Get file info from cache + * Get cached metadata for a storage path. * - * If the file is not in cached it will be scanned - * If the file has changed on storage the cache will be updated + * Scans the path if it is not cached, or refreshes cached metadata when it has + * changed on storage. If refreshing cached metadata requires a lock that cannot + * be acquired, the existing cached metadata is returned instead. * - * @param Storage $storage - * @param string $internalPath - * @param string $relativePath - * @return ICacheEntry|bool + * @param Storage $storage Storage containing the path + * @param string $internalPath Path relative to the storage root + * @param string $relativePath Path relative to this view + * @return ICacheEntry|false Cached metadata, or false if the path does not + * exist or cannot be scanned due to a lock */ private function getCacheEntry($storage, $internalPath, $relativePath) { $cache = $storage->getCache($internalPath); $data = $cache->get($internalPath); $watcher = $storage->getWatcher($internalPath); - try { - // if the file is not in the cache or needs to be updated, trigger the scanner and reload the data - if (!$data || (isset($data['size']) && $data['size'] === -1)) { - if (!$storage->file_exists($internalPath)) { - return false; - } - // don't need to get a lock here since the scanner does it's own locking + if (!$data || (isset($data['size']) && $data['size'] === -1)) { + // Populate missing or incomplete cache entries. The scanner handles locking. + if (!$storage->file_exists($internalPath)) { + return false; + } + + try { $scanner = $storage->getScanner($internalPath); $scanner->scan($internalPath, Scanner::SCAN_SHALLOW); $data = $cache->get($internalPath); - } elseif (!Scanner::isPartialFile($internalPath) && $watcher->needsUpdate($internalPath, $data)) { + } catch (LockedException $e) { + // If the path cannot be scanned because it is locked, return the existing cache result. + } + } elseif (!Scanner::isPartialFile($internalPath) && $watcher->needsUpdate($internalPath, $data)) { + // Refresh metadata when storage has changed and propagate changes. Handle locking. + try { $this->lockFile($relativePath, ILockingProvider::LOCK_SHARED); - $cacheDataBefore = $data instanceof CacheEntry ? $data->getData() : false; + } catch (LockedException $e) { + // Refreshing cache metadata is best-effort; use the existing cache entry. + return $data; + } + + $refreshException = null; + try { + $cachedDataBeforeUpdate = $data instanceof CacheEntry ? $data->getData() : false; $watcher->update($internalPath, $data); $data = $cache->get($internalPath); - $cacheDataAfter = $data instanceof CacheEntry ? $data->getData() : false; + $cachedDataAfterUpdate = $data instanceof CacheEntry ? $data->getData() : false; - // Only propagate mtime change to parent folders if the scanner actually changed the cached metadata, - // to avoid updating folder mtimes on every read for backends that conservatively report directories as updated (e.g. S3) - if ($cacheDataAfter !== $cacheDataBefore) { + // Propagate only actual metadata changes, avoiding mtime updates on every + // read for backends that conservatively report directories as updated (e.g. S3). + if ($cachedDataAfterUpdate !== $cachedDataBeforeUpdate) { $storage->getPropagator()->propagateChange($internalPath, time()); $data = $cache->get($internalPath); } - $this->unlockFile($relativePath, ILockingProvider::LOCK_SHARED); + } catch (\Throwable $e) { + $refreshException = $e; + throw $e; + } finally { + try { + $this->unlockFile($relativePath, ILockingProvider::LOCK_SHARED); + } catch (\Throwable $unlockException) { + if ($refreshException !== null) { + $this->logger->error('Failed to release cache refresh lock', [ + 'app' => 'core', + 'path' => $relativePath, + 'exception' => $unlockException, + ]); + } else { + throw $unlockException; + } + } } - } catch (LockedException $e) { - // if the file is locked we just use the old cache info } return $data; From 289a8982c7854366ca924b724ed3ca073020b4db Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 20 Jul 2026 11:48:11 -0400 Subject: [PATCH 2/4] chore(view): add runtime typing to getCacheEntry() Signed-off-by: Josh --- lib/private/Files/View.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 8fcba6441cd59..99247d2fc9f6f 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1392,10 +1392,13 @@ private function getUserObjectForOwner(string $ownerId): IUser { * @param Storage $storage Storage containing the path * @param string $internalPath Path relative to the storage root * @param string $relativePath Path relative to this view - * @return ICacheEntry|false Cached metadata, or false if the path does not - * exist or cannot be scanned due to a lock + * @return ICacheEntry|false Cached metadata, or false if the path does not exist or cannot be scanned due to a lock */ - private function getCacheEntry($storage, $internalPath, $relativePath) { + private function getCacheEntry( + Storage $storage, + string $internalPath, + string $relativePath, + ): ICacheEntry|false { $cache = $storage->getCache($internalPath); $data = $cache->get($internalPath); $watcher = $storage->getWatcher($internalPath); From ecbd6aebc87defeb7c4fa0a024afc651429a084c Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 20 Jul 2026 17:18:24 -0400 Subject: [PATCH 3/4] test(view): cover cache refresh lock cleanup Assisted-by: GitHub Copilot:GPT-5 Signed-off-by: Josh --- tests/lib/Files/ViewTest.php | 95 ++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index c21bf6fc40c9d..ed4047e2a1286 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -21,6 +21,9 @@ use OCA\Files_Trashbin\Trash\ITrashManager; use OCP\Cache\CappedMemoryCache; use OCP\Constants; +use OCP\Files\Cache\ICacheEntry; +use OCP\Files\Cache\IPropagator; +use OCP\Files\Cache\IWatcher; use OCP\Files\Config\IMountProvider; use OCP\Files\Config\IMountProviderCollection; use OCP\Files\Config\IUserMountCache; @@ -91,6 +94,49 @@ public function hasUpdated(string $path, int $time): bool { } } +class TemporaryWatcherUpdateThrows extends TemporaryAlwaysUpdated { + public function getWatcher(string $path = '', ?IStorage $storage = null): IWatcher { + return new class(parent::getWatcher($path, $storage)) implements IWatcher { + public function __construct( + private readonly IWatcher $watcher, + ) { + } + + public function setPolicy($policy): void { + $this->watcher->setPolicy($policy); + } + + public function setCheckFilter(?string $filter): void { + $this->watcher->setCheckFilter($filter); + } + + public function getPolicy() { + return $this->watcher->getPolicy(); + } + + public function checkUpdate($path, $cachedEntry = null) { + return $this->watcher->checkUpdate($path, $cachedEntry); + } + + public function update($path, $cachedData): void { + throw new \RuntimeException('Simulated watcher update failure'); + } + + public function needsUpdate($path, $cachedData): bool { + return $this->watcher->needsUpdate($path, $cachedData); + } + + public function cleanFolder($path): void { + $this->watcher->cleanFolder($path); + } + + public function onUpdate(callable $callback): void { + $this->watcher->onUpdate($callback); + } + }; + } +} + class TestEventHandler { public function umount() { } @@ -492,6 +538,55 @@ public function testWatcherDoesNotPropagateWhenStorageMtimeUnchanged(): void { ); } + public function testWatcherRefreshFailureReleasesFileAndParentLocks(): void { + $storage = $this->getTestStorage(true, TemporaryWatcherUpdateThrows::class); + Filesystem::mount($storage, [], '/' . self::$user . '/files'); + + $view = new View('/' . self::$user . '/files'); + $storage->getWatcher()->setPolicy(Watcher::CHECK_ALWAYS); + + try { + $view->getFileInfo('folder/bar.txt'); + $this->fail('Expected the watcher update failure to be rethrown'); + } catch (\RuntimeException $e) { + $this->assertSame('Simulated watcher update failure', $e->getMessage()); + } + + $this->assertFalse( + $this->isFileLocked($view, 'folder/bar.txt', ILockingProvider::LOCK_SHARED), + 'The refreshed file lock must be released after a watcher failure', + ); + $this->assertFalse( + $this->isFileLocked($view, 'folder', ILockingProvider::LOCK_SHARED), + 'The parent refresh lock must be released after a watcher failure', + ); + $this->assertFalse( + $this->isFileLocked($view, '/', ILockingProvider::LOCK_SHARED), + 'The root refresh lock must be released after a watcher failure', + ); + } + + public function testWatcherRefreshUsesCachedEntryWhenRefreshLockCannotBeAcquired(): void { + $storage = $this->getTestStorage(true, TemporaryAlwaysUpdated::class); + Filesystem::mount($storage, [], '/' . self::$user . '/files'); + + $view = new View('/' . self::$user . '/files'); + $storage->getWatcher()->setPolicy(Watcher::CHECK_ALWAYS); + + $cachedEntry = $storage->getCache()->get('folder/bar.txt'); + $this->assertInstanceOf(ICacheEntry::class, $cachedEntry); + + $view->lockFile('folder/bar.txt', ILockingProvider::LOCK_EXCLUSIVE); + try { + $info = $view->getFileInfo('folder/bar.txt'); + + $this->assertNotFalse($info); + $this->assertSame($cachedEntry->getId(), $info->getId()); + } finally { + $view->unlockFile('folder/bar.txt', ILockingProvider::LOCK_EXCLUSIVE); + } + } + public function testCopyBetweenStorageNoCross(): void { $storage1 = $this->getTestStorage(true, TemporaryNoCross::class); $storage2 = $this->getTestStorage(true, TemporaryNoCross::class); From eafc18dfa886f75a2fbf50662bb7073aa36e30e9 Mon Sep 17 00:00:00 2001 From: Josh Date: Tue, 21 Jul 2026 10:55:34 -0400 Subject: [PATCH 4/4] chore(view): drop unused IPropagator import in ViewTest Signed-off-by: Josh --- tests/lib/Files/ViewTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index ed4047e2a1286..f9849ab736b4e 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -22,7 +22,6 @@ use OCP\Cache\CappedMemoryCache; use OCP\Constants; use OCP\Files\Cache\ICacheEntry; -use OCP\Files\Cache\IPropagator; use OCP\Files\Cache\IWatcher; use OCP\Files\Config\IMountProvider; use OCP\Files\Config\IMountProviderCollection;