DO NOT REVIEW - chore(cypress): Try to analyze failures

apps/files/src/store/viewConfig.spec.ts

@@ -0,0 +1,87 @@
+/*!
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import axios from '@nextcloud/axios'
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, test, vi } from 'vitest'
+import { useViewConfigStore } from './viewConfig.ts'
+
+vi.mock('@nextcloud/auth', () => ({
+	getCurrentUser: () => ({ uid: 'test', displayName: 'Test' }),
+}))
+vi.mock('@nextcloud/axios', () => ({
+	default: {
+		put: vi.fn(),
+	},
+}))
+vi.mock('@nextcloud/router', () => ({
+	generateUrl: (url: string) => url,
+}))
+vi.mock('@nextcloud/initial-state', () => ({
+	loadState: () => ({}),
+}))
+
+describe('View config store keeps sorting state consistent under slow requests', () => {
+	// Captured resolvers for the pending PUT requests, so a test can decide when
+	// (and in which order) the "server" responds.
+	let putResolvers: Array<() => void>
+
+	beforeEach(() => {
+		setActivePinia(createPinia())
+		putResolvers = []
+		vi.mocked(axios.put).mockImplementation(() => new Promise((resolve) => {
+			putResolvers.push(() => resolve({ data: {} } as never))
+		}))
+	})
+
+	test('reflects a sorting change locally without waiting for the server round-trip', () => {
+		const store = useViewConfigStore()
+
+		store.setSortingBy('size', 'files')
+
+		// The PUT requests are still pending, yet the local config is already
+		// updated so the header and file list can react immediately.
+		expect(putResolvers.length).toBeGreaterThan(0)
+		expect(store.getConfig('files')).toMatchObject({
+			sorting_mode: 'size',
+			sorting_direction: 'asc',
+		})
+	})
+
+	test('consecutive direction toggles read the freshly updated local state', () => {
+		const store = useViewConfigStore()
+
+		// Rapid header clicks: sort by size (asc), then flip twice. Each toggle
+		// must read the direction the previous action just wrote locally, even
+		// though none of the PUTs have resolved yet.
+		store.setSortingBy('size', 'files')
+		expect(store.getConfig('files').sorting_direction).toBe('asc')
+
+		store.toggleSortingDirection('files')
+		expect(store.getConfig('files').sorting_direction).toBe('desc')
+
+		store.toggleSortingDirection('files')
+		expect(store.getConfig('files').sorting_direction).toBe('asc')
+	})
+
+	test('an out-of-order server response does not clobber the local state', async () => {
+		const store = useViewConfigStore()
+
+		store.setSortingBy('size', 'files')
+		store.toggleSortingDirection('files')
+		store.toggleSortingDirection('files')
+
+		expect(store.getConfig('files').sorting_direction).toBe('asc')
+
+		// Let the queued PUTs resolve in reverse order: since the local store is
+		// no longer driven by request resolution, the latest value must survive.
+		for (const resolve of [...putResolvers].reverse()) {
+			resolve()
+		}
+		await Promise.resolve()
+
+		expect(store.getConfig('files').sorting_direction).toBe('asc')
+	})
+})

apps/files/src/store/viewConfig.ts

@@ -49,15 +49,21 @@ export const useViewConfigStore = defineStore('viewconfig', () => {
 	 * @param value New value
 	 */
 	async function update(view: ViewId, key: string, value: string | number | boolean): Promise<void> {
+		// Update the local store synchronously first, in call order, so the UI
+		// reflects the change immediately. Emitting only after the awaited server
+		// round-trip means rapid updates (e.g. flipping the sort direction several
+		// times in a row) issue concurrent PUTs that may resolve out of order,
+		// leaving the local state on whichever request finished last rather than
+		// the user's most recent action.
+		emit('files:view-config:updated', { view, key, value })
+
 		if (getCurrentUser() !== null) {
 			await axios.put(generateUrl('/apps/files/api/v1/views'), {
 				value,
 				view,
 				key,
 			})
 		}
-
-		emit('files:view-config:updated', { view, key, value })
 	}
 
 	/**

apps/files_versions/lib/Storage.php

@@ -423,7 +423,30 @@ private static function copyFileContents($view, $path1, $path2) {
 		[$storage2, $internalPath2] = $view->resolvePath($path2);
 
 		$view->lockFile($path1, ILockingProvider::LOCK_EXCLUSIVE);
-		$view->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE);
+		try {
+			$view->lockFile($path2, ILockingProvider::LOCK_EXCLUSIVE);
+		} catch (\Throwable $e) {
+			// DIAGNOSTIC: dump all currently-held file locks to identify the
+			// concurrent holder blocking the restore (lock: -1 exclusive, >0 shared).
+			try {
+				$db = \OCP\Server::get(\OCP\IDBConnection::class);
+				$q = $db->getQueryBuilder();
+				$q->select('key', 'lock')->from('file_locks')
+					->where($q->expr()->neq('lock', $q->createNamedParameter(0, \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_INT)));
+				$held = $q->executeQuery()->fetchAll();
+				\OCP\Server::get(\Psr\Log\LoggerInterface::class)->error(
+					'[restore-lock-diag] target=' . $internalPath2 . ' heldLocks=' . json_encode($held),
+					['app' => 'files_versions'],
+				);
+			} catch (\Throwable $ignore) {
+			}
+			// Acquiring the second lock can fail (e.g. the target file is
+			// transiently locked by a concurrent job under load). Release the
+			// first lock we already hold so a retry does not collide with a
+			// leaked lock.
+			$view->unlockFile($path1, ILockingProvider::LOCK_EXCLUSIVE);
+			throw $e;
+		}
 
 		try {
 			// TODO add a proper way of overwriting a file while maintaining file ids

apps/files_versions/lib/Versions/VersionManager.php

@@ -22,8 +22,10 @@
 use OCP\Files\Node;
 use OCP\Files\Storage\IStorage;
 use OCP\IUser;
+use OCP\Lock\LockedException;
 use OCP\Lock\ManuallyLockedException;
 use OCP\Server;
+use Psr\Log\LoggerInterface;
 
 class VersionManager implements IVersionManager, IDeletableVersionBackend, INeedSyncVersionBackend, IMetadataVersionBackend {
 
@@ -100,7 +102,7 @@ public function createVersion(IUser $user, FileInfo $file) {
 	#[\Override]
 	public function rollback(IVersion $version) {
 		$backend = $version->getBackend();
-		$result = self::handleAppLocks(fn (): ?bool => $backend->rollback($version));
+		$result = self::handleAppLocks(fn (): ?bool => self::retryOnLock(fn (): ?bool => $backend->rollback($version)));
 		// rollback doesn't have a return type yet and some implementations don't return anything
 		if ($result === null || $result === true) {
 			$this->dispatcher->dispatchTyped(new VersionRestoredEvent($version));
@@ -180,6 +182,45 @@ public function setMetadataValue(Node $node, int $revision, string $key, string
 		}
 	}
 
+	/**
+	 * Retry the rollback while the target file is transiently locked.
+	 *
+	 * Restoring a version needs an exclusive lock on the live file. Under load,
+	 * a concurrent operation on the same file — e.g. the versions expiration
+	 * job — can hold a lock on it, which makes the exclusive lock fail with a
+	 * LockedException and the whole restore return HTTP 500. That lock is not
+	 * held by our own request (so it is released independently of us) and is
+	 * short lived, so we keep retrying with a growing backoff until it clears
+	 * or we exceed a generous overall budget.
+	 *
+	 * @param callable $callback function performing the rollback
+	 * @return bool|null
+	 * @throws LockedException if the file stays locked for the whole budget
+	 */
+	private static function retryOnLock(callable $callback): ?bool {
+		// Total time we are willing to wait for a concurrent lock to clear.
+		$budgetMs = 15000;
+		$waitedMs = 0;
+		$backoffMs = 100;
+		for ($attempt = 1; ; $attempt++) {
+			try {
+				return $callback();
+			} catch (LockedException $e) {
+				if ($waitedMs >= $budgetMs) {
+					throw $e;
+				}
+				Server::get(LoggerInterface::class)->debug(
+					'Version rollback hit a locked file, retrying (attempt {attempt}, waited {waited}ms)',
+					['attempt' => $attempt, 'waited' => $waitedMs, 'app' => 'files_versions', 'exception' => $e],
+				);
+				usleep($backoffMs * 1000);
+				$waitedMs += $backoffMs;
+				// Grow the backoff but cap it so we keep probing regularly.
+				$backoffMs = min($backoffMs * 2, 1000);
+			}
+		}
+	}
+
 	/**
 	 * Catch ManuallyLockedException and retry in app context if possible.
 	 *

apps/files_versions/tests/VersioningTest.php

@@ -27,6 +27,8 @@
 use OCP\IConfig;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Lock\ILockingProvider;
+use OCP\Lock\LockedException;
 use OCP\Server;
 use OCP\Share\IShare;
 use OCP\User\Exceptions\UserNotFoundException;
@@ -763,6 +765,56 @@ function ($p) use (&$params): void {
 		);
 	}
 
+	/**
+	 * Restoring a version needs an exclusive lock on the live file. When another
+	 * request holds a shared (read) lock on that file at the same time — most
+	 * notably the versions sidebar generating a preview of the file via
+	 * OCA\Files_Versions\Controller\PreviewController — the exclusive lock cannot
+	 * be acquired and the restore throws a LockedException (surfacing as an HTTP
+	 * 500). This test reproduces that collision deterministically.
+	 */
+	public function testRestoreFailsWhileFileIsReadLocked(): void {
+		$filePath = self::TEST_VERSIONS_USER . '/files/sub/test.txt';
+		$this->rootView->mkdir(self::TEST_VERSIONS_USER . '/files/sub');
+		$this->rootView->file_put_contents($filePath, 'test file');
+		$fileInfo = $this->rootView->getFileInfo($filePath);
+
+		// Create a single older version to restore to.
+		$t2 = time() - 60 * 60 * 24 * 14;
+		$v2 = self::USERS_VERSIONS_ROOT . '/sub/test.txt.v' . $t2;
+		$this->rootView->mkdir(self::USERS_VERSIONS_ROOT . '/sub');
+		$this->rootView->file_put_contents($v2, 'version2');
+		$fileInfoV2 = $this->rootView->getFileInfo($v2);
+		$versionEntity = new VersionEntity();
+		$versionEntity->setFileId($fileInfo->getId());
+		$versionEntity->setTimestamp($t2);
+		$versionEntity->setSize($fileInfoV2->getSize());
+		$versionEntity->setMimetype($this->mimeTypeLoader->getId($fileInfoV2->getMimetype()));
+		$versionEntity->setMetadata([]);
+		$this->versionsMapper->insert($versionEntity);
+
+		// Simulate a concurrent version-preview read holding a shared lock.
+		$userView = new View('/' . self::TEST_VERSIONS_USER . '/files');
+		$userView->lockFile('/sub/test.txt', ILockingProvider::LOCK_SHARED);
+
+		// The low-level restore takes an exclusive lock on the live file, which
+		// must fail while the shared lock is held — this is the bug behind the 500.
+		$threw = false;
+		try {
+			Storage::rollback('/sub/test.txt', $t2, $this->user1);
+		} catch (LockedException $e) {
+			$threw = true;
+		}
+		$this->assertTrue($threw, 'Restore should fail with LockedException while the file is read-locked');
+		$this->assertEquals('test file', $this->rootView->file_get_contents($filePath), 'File must be unchanged after the failed restore');
+
+		// Once the read lock is released, the very same restore succeeds — proving
+		// the shared lock is the sole cause of the failure.
+		$userView->unlockFile('/sub/test.txt', ILockingProvider::LOCK_SHARED);
+		$this->assertTrue(Storage::rollback('/sub/test.txt', $t2, $this->user1));
+		$this->assertEquals('version2', $this->rootView->file_get_contents($filePath));
+	}
+
 	private function doTestRestore(): void {
 		$filePath = self::TEST_VERSIONS_USER . '/files/sub/test.txt';
 		$this->rootView->file_put_contents($filePath, 'test file');

build/psalm-baseline.xml

@@ -2209,6 +2209,7 @@
       <code><![CDATA[unlink]]></code>
       <code><![CDATA[unlockFile]]></code>
       <code><![CDATA[unlockFile]]></code>
+      <code><![CDATA[unlockFile]]></code>
     </InternalMethod>
   </file>
   <file src="apps/files_versions/lib/Versions/LegacyVersionsBackend.php">

cypress.config.ts

@@ -26,11 +26,7 @@ export default defineConfig({
 	viewportHeight: 720,
 
 	// Tries again when in run mode (cypress run) e.g. on CI
-	retries: {
-		runMode: 5,
-		// do not retry in `cypress open`
-		openMode: 0,
-	},
+	retries: 0,
 
 	// Needed to trigger `after:run` events with cypress open
 	experimentalInteractiveRunEvents: true,
@@ -58,6 +54,14 @@ export default defineConfig({
 		// Disable session isolation
 		testIsolation: false,
 
+		// The CI runners (and the local 0.2-CPU repro throttle) are slow enough
+		// that the default 4s command timeout is too short for e.g. a folder
+		// listing to re-fetch and render after a navigation. Raise it globally
+		// (to match requestTimeout) rather than sprinkling per-command timeouts.
+		// Retries do not help here: a retry re-runs the whole test on the same
+		// slow runner.
+		// defaultCommandTimeout: 30000,
+
 		requestTimeout: 30000,
 
 		// We've imported your old cypress plugins here.
@@ -71,6 +75,13 @@ export default defineConfig({
 			// because Cypress.env() and other options are local to the current spec file.
 			const data: Record<string, unknown> = {}
 			on('task', {
+				// Print a message to the Node/terminal stdout (browser console.log
+				// is not piped to the cypress run output in headless mode).
+				log(message) {
+					// eslint-disable-next-line no-console
+					console.log(message)
+					return null
+				},
 				setVariable({ key, value }) {
 					data[key] = value
 					return null