fix(view): release cache refresh locks on errors

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;

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);