diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 712b40b2380cb..99247d2fc9f6f 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -1383,48 +1383,79 @@ 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 - */ - private function getCacheEntry($storage, $internalPath, $relativePath) { + * @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 $storage, + string $internalPath, + string $relativePath, + ): ICacheEntry|false { $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; diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index c21bf6fc40c9d..f9849ab736b4e 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -21,6 +21,8 @@ use OCA\Files_Trashbin\Trash\ITrashManager; use OCP\Cache\CappedMemoryCache; use OCP\Constants; +use OCP\Files\Cache\ICacheEntry; +use OCP\Files\Cache\IWatcher; use OCP\Files\Config\IMountProvider; use OCP\Files\Config\IMountProviderCollection; use OCP\Files\Config\IUserMountCache; @@ -91,6 +93,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 +537,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);