diff --git a/apps/files/src/store/viewConfig.spec.ts b/apps/files/src/store/viewConfig.spec.ts new file mode 100644 index 0000000000000..093519b3e4320 --- /dev/null +++ b/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') + }) +}) diff --git a/apps/files/src/store/viewConfig.ts b/apps/files/src/store/viewConfig.ts index d624584e5dc14..8023098b9899a 100644 --- a/apps/files/src/store/viewConfig.ts +++ b/apps/files/src/store/viewConfig.ts @@ -49,6 +49,14 @@ export const useViewConfigStore = defineStore('viewconfig', () => { * @param value New value */ async function update(view: ViewId, key: string, value: string | number | boolean): Promise { + // 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, @@ -56,8 +64,6 @@ export const useViewConfigStore = defineStore('viewconfig', () => { key, }) } - - emit('files:view-config:updated', { view, key, value }) } /** diff --git a/apps/files_versions/lib/Storage.php b/apps/files_versions/lib/Storage.php index bd21c4f92b35e..c638316b7c30c 100644 --- a/apps/files_versions/lib/Storage.php +++ b/apps/files_versions/lib/Storage.php @@ -423,7 +423,16 @@ 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) { + // 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 diff --git a/apps/files_versions/lib/Versions/VersionManager.php b/apps/files_versions/lib/Versions/VersionManager.php index 2e91a063b7021..206a6b54b6fb6 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/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 { @@ -32,6 +34,8 @@ class VersionManager implements IVersionManager, IDeletableVersionBackend, INeed public function __construct( private IEventDispatcher $dispatcher, + private int $lockRetryBudgetMs = 10000, + private int $lockRetryInitialBackoffMs = 100, ) { } @@ -100,7 +104,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 => $this->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 +184,43 @@ 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 function retryOnLock(callable $callback): ?bool { + $waitedMs = 0; + $backoffMs = $this->lockRetryInitialBackoffMs; + for ($attempt = 1; ; $attempt++) { + try { + return $callback(); + } catch (LockedException $e) { + if ($waitedMs >= $this->lockRetryBudgetMs) { + 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. * diff --git a/apps/files_versions/tests/StorageTest.php b/apps/files_versions/tests/StorageTest.php index 35f60d4afe868..4e3c75b60820b 100644 --- a/apps/files_versions/tests/StorageTest.php +++ b/apps/files_versions/tests/StorageTest.php @@ -8,10 +8,13 @@ namespace OCA\files_versions\tests; +use OC\Files\View; use OCA\Files_Versions\Expiration; use OCA\Files_Versions\Storage; use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; +use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; use OCP\Server; use Test\TestCase; use Test\Traits\UserTrait; @@ -95,4 +98,42 @@ public function testExpireMaxAge(): void { $this->assertCount(0, Storage::getVersions('version_test', 'folder1/sub1/file3')); $this->assertCount(1, Storage::getVersions('version_test', 'folder2/file4')); } + + /** + * Regression test: when copyFileContents() fails to acquire the lock on the + * target file, the lock already held on the source file must be released + * before the exception propagates. Otherwise the source lock leaks and a + * retry (or any later access) collides with a lock nobody owns anymore. + */ + public function testCopyFileContentsReleasesSourceLockWhenTargetLockFails(): void { + $source = 'files/source.txt'; + $target = 'files/target.txt'; + $lockedException = new LockedException($target); + + $storage = $this->createMock(\OC\Files\Storage\Storage::class); + + $view = $this->createMock(View::class); + $view->method('resolvePath')->willReturn([$storage, 'internal']); + + // The source lock is acquired first and succeeds; acquiring the target + // lock then throws (e.g. a concurrent holder under load). + $view->expects($this->exactly(2)) + ->method('lockFile') + ->willReturnCallback(function (string $path) use ($target, $lockedException): void { + if ($path === $target) { + throw $lockedException; + } + }); + + // The already-held source lock must be released exactly once, and the + // never-acquired target lock must not be touched. + $view->expects($this->once()) + ->method('unlockFile') + ->with($source, ILockingProvider::LOCK_EXCLUSIVE); + + $this->expectExceptionObject($lockedException); + + $method = new \ReflectionMethod(Storage::class, 'copyFileContents'); + $method->invoke(null, $view, $source, $target); + } } diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php index e0e738bf81237..91d39930ede99 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/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'); diff --git a/apps/files_versions/tests/Versions/VersionManagerTest.php b/apps/files_versions/tests/Versions/VersionManagerTest.php index 4be522d5fa9a8..458341ad6aa13 100644 --- a/apps/files_versions/tests/Versions/VersionManagerTest.php +++ b/apps/files_versions/tests/Versions/VersionManagerTest.php @@ -16,6 +16,7 @@ use OCA\Files_Versions\Versions\VersionManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Storage\IStorage; +use OCP\Lock\LockedException; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -141,4 +142,54 @@ public function testRollbackFailure(): void { $this->assertFalse($manager->rollback($versionMock)); } + + public function testRollbackRetriesWhileFileIsTransientlyLocked(): void { + $versionMock = $this->createMock(IVersion::class); + $backendMock = $this->createMock(IVersionBackend::class); + $versionMock->method('getBackend')->willReturn($backendMock); + + // The live file is locked for the first two attempts (e.g. a concurrent + // read holds a shared lock) and then the lock clears. + $attempts = 0; + $backendMock->expects($this->exactly(3)) + ->method('rollback') + ->with($versionMock) + ->willReturnCallback(function () use (&$attempts): bool { + $attempts++; + if ($attempts < 3) { + throw new LockedException('files/foo.txt'); + } + return true; + }); + + $dispatcherMock = $this->createMock(IEventDispatcher::class); + $dispatcherMock->expects($this->once()) + ->method('dispatchTyped') + ->with($this->isInstanceOf(VersionRestoredEvent::class)); + + // Tiny retry budget/backoff so the test exercises the retry without waiting. + $manager = new VersionManager($dispatcherMock, 1000, 1); + + $this->assertTrue($manager->rollback($versionMock)); + $this->assertSame(3, $attempts); + } + + public function testRollbackGivesUpAfterRetryBudgetExhausted(): void { + $versionMock = $this->createMock(IVersion::class); + $backendMock = $this->createMock(IVersionBackend::class); + $versionMock->method('getBackend')->willReturn($backendMock); + + // The live file stays locked for the whole retry budget. + $backendMock->method('rollback') + ->with($versionMock) + ->willThrowException(new LockedException('files/foo.txt')); + + $dispatcherMock = $this->createMock(IEventDispatcher::class); + $dispatcherMock->expects($this->never())->method('dispatchTyped'); + + $manager = new VersionManager($dispatcherMock, 5, 1); + + $this->expectException(LockedException::class); + $manager->rollback($versionMock); + } } diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 33204e7699da1..baedfc6768156 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2209,6 +2209,7 @@ + diff --git a/cypress.config.ts b/cypress.config.ts index cec4bf5c8d6f7..f5c33e4c89bd6 100644 --- a/cypress.config.ts +++ b/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 = {} 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 diff --git a/cypress/e2e/files/FilesUtils.ts b/cypress/e2e/files/FilesUtils.ts index 2d7a28ac6d677..f004de108c228 100644 --- a/cypress/e2e/files/FilesUtils.ts +++ b/cypress/e2e/files/FilesUtils.ts @@ -62,6 +62,52 @@ export function getInlineActionEntryForFile(file: string, actionId: string) { return cy.get(`[data-cy-files-list-row-name="${CSS.escape(file)}"] [data-cy-files-list-row-action="${CSS.escape(actionId)}"]`) } +/** + * Open the actions menu of a file row and wait until it is displayed. + * + * Click exactly once, then wait: while the popover opens, the toggle + * already reports aria-expanded="true" although the menu is still hidden — + * the popover positions itself over several frames, which can take seconds + * on slow (CI) runners. Clicking again in that state toggles the menu + * closed and tangles the popover's show and hide transitions into a stuck, + * permanently invisible popover. Clicking is only safe while the toggle + * reports "closed", e.g. after an auto-close caused by a stray outside + * click. + * + * @param getActionButton query for the actions menu toggle of the row + */ +export function openActionsMenu(getActionButton: () => Cypress.Chainable>) { + // The menu open has two failure modes on slow runners, needing opposite + // responses: + // - The click is lost because the row's handler is not attached yet, so + // the toggle stays collapsed (aria-expanded="false"). We must click + // again. + // - The menu is opening but the popover is still positioning over a few + // frames (aria-expanded="true", not yet visible). Clicking again here + // would toggle it closed and tangle the show/hide transitions, so we + // must only wait. + // Poll accordingly until the menu is actually displayed. + const poll = (elapsed: number) => { + getActionButton().then(($toggle) => { + const menuId = $toggle.attr('aria-controls') + if (menuId && Cypress.$(`#${menuId}`).is(':visible')) { + return + } + if (elapsed >= 20000) { + throw new Error(`Actions menu did not open (aria-expanded=${$toggle.attr('aria-expanded')})`) + } + // Only (re)open while collapsed; never click a menu that is mid-open. + if ($toggle.attr('aria-expanded') !== 'true') { + cy.wrap($toggle).click({ force: true }) // force to avoid issues with overlaying file list header + } + // eslint-disable-next-line cypress/no-unnecessary-waiting -- give the popover a moment to open/position before re-checking + cy.wait(250) + poll(elapsed + 250) + }) + } + poll(0) +} + /** * * @param fileid @@ -70,8 +116,7 @@ export function getInlineActionEntryForFile(file: string, actionId: string) { export function triggerActionForFileId(fileid: number, actionId: string) { getActionButtonForFileId(fileid) .scrollIntoView() - getActionButtonForFileId(fileid) - .click({ force: true }) // force to avoid issues with overlaying file list header + openActionsMenu(() => getActionButtonForFileId(fileid)) getActionEntryForFileId(fileid, actionId) .find('button') .should('be.visible') @@ -86,8 +131,7 @@ export function triggerActionForFileId(fileid: number, actionId: string) { export function triggerActionForFile(filename: string, actionId: string) { getActionButtonForFile(filename) .scrollIntoView() - getActionButtonForFile(filename) - .click({ force: true }) // force to avoid issues with overlaying file list header + openActionsMenu(() => getActionButtonForFile(filename)) getActionEntryForFile(filename, actionId) .find('button') .should('be.visible') @@ -167,6 +211,49 @@ export function triggerSelectionAction(actionId: string) { .click() } +/** + * Inside the file picker, navigate to the home root and confirm the copy/move. + * + * On a slow runner two things race and must be handled deterministically: + * - The picker's current directory lags behind its confirm-button label: the + * button already reads the plain "Copy"/"Move" (root) label while the picker + * still shows the folder it opened in, so clicking it copies/moves into the + * wrong folder (deduplicated as "… (1)"). We therefore wait for the picker + * to actually reload its listing (its own PROPFIND) after clicking the + * "All files" breadcrumb, so its current directory really is the root before + * we confirm. + * - The confirm click itself can be dropped (its handler is not attached yet), + * so no request fires. We re-click until the resulting request is seen. + * + * @param verb the confirm action, 'Copy' or 'Move' + * @param requestAlias the intercept alias for the resulting DAV request + */ +function confirmPickerAtHomeRoot(verb: 'Copy' | 'Move', requestAlias: string) { + cy.intercept('PROPFIND', /\/(remote|public)\.php\/dav\/files\//).as('pickerNavigation') + cy.get('.breadcrumb') + .findByRole('button', { name: 'All files' }) + .should('be.visible') + .click() + // Wait for the picker to actually fetch the root listing, so its current + // directory (the copy/move target) is the root and not the folder it opened + // in — otherwise the confirm below would target the wrong folder. + cy.wait('@pickerNavigation') + + // The plain "Copy"/"Move" label only exists once the picker is at the root. + const confirmLabel = new RegExp(`^\\s*${verb}\\s*$`) + const clickUntilRequestFires = (attemptsLeft: number) => { + cy.contains('button', confirmLabel).should('be.visible').click() + // eslint-disable-next-line cypress/no-unnecessary-waiting -- give a dropped click a moment to be noticed + cy.wait(2000) + cy.get(`@${requestAlias}.all`).then((calls) => { + if (calls.length === 0 && attemptsLeft > 0) { + clickUntilRequestFires(attemptsLeft - 1) + } + }) + } + clickUntilRequestFires(4) +} + /** * * @param fileName @@ -181,13 +268,7 @@ export function moveFile(fileName: string, dirPath: string) { cy.intercept('MOVE', /\/(remote|public)\.php\/dav\/files\//).as('moveFile') if (dirPath === '/') { - // select home folder - cy.get('.breadcrumb') - .findByRole('button', { name: 'All files' }) - .should('be.visible') - .click() - // click move - cy.contains('button', 'Move').should('be.visible').click() + confirmPickerAtHomeRoot('Move', 'moveFile') } else if (dirPath === '.') { // click move cy.contains('button', 'Copy').should('be.visible').click() @@ -220,13 +301,7 @@ export function copyFile(fileName: string, dirPath: string) { cy.intercept('COPY', /\/(remote|public)\.php\/dav\/files\//).as('copyFile') if (dirPath === '/') { - // select home folder - cy.get('.breadcrumb') - .findByRole('button', { name: 'All files' }) - .should('be.visible') - .click() - // click copy - cy.contains('button', 'Copy').should('be.visible').click() + confirmPickerAtHomeRoot('Copy', 'copyFile') } else if (dirPath === '.') { // click copy cy.contains('button', 'Copy').should('be.visible').click() diff --git a/cypress/e2e/files_sharing/public-share/view_file-drop.cy.ts b/cypress/e2e/files_sharing/public-share/view_file-drop.cy.ts index c3c289774ccb2..5c81d011351bc 100644 --- a/cypress/e2e/files_sharing/public-share/view_file-drop.cy.ts +++ b/cypress/e2e/files_sharing/public-share/view_file-drop.cy.ts @@ -131,9 +131,19 @@ describe('files_sharing: Public share - File drop', { testIsolation: true }, () cy.wait('@uploadFile') - cy.findByRole('progressbar') - .should('be.visible') - .and((el) => { expect(Number.parseInt(el.attr('value') ?? '0')).be.gte(50) }) + // More than one progressbar can exist (upload picker and file drop + // view) and some of them stay hidden, so assert that any visible + // one reports the expected progress. + cy.findAllByRole('progressbar') + .should(($bars) => { + const visible = $bars.toArray().filter((el) => Cypress.$(el).is(':visible')) + const summary = $bars.toArray() + .map((el) => `${el.tagName}[value=${el.getAttribute('value')} visible=${Cypress.$(el).is(':visible')}]`) + .join(', ') + expect(visible.length, `visible progressbar (${summary})`).to.be.gte(1) + const values = visible.map((el) => Number.parseInt(el.getAttribute('value') ?? '0')) + expect(Math.max(...values), `upload progress (${summary})`).to.be.gte(50) + }) // continue second request .then(() => resolve(null)) diff --git a/cypress/e2e/files_versions/filesVersionsUtils.ts b/cypress/e2e/files_versions/filesVersionsUtils.ts index ae23dca409789..8173bc4e39886 100644 --- a/cypress/e2e/files_versions/filesVersionsUtils.ts +++ b/cypress/e2e/files_versions/filesVersionsUtils.ts @@ -7,18 +7,21 @@ import type { User } from '@nextcloud/e2e-test-server/cypress' import type { ShareSetting } from '../files_sharing/FilesSharingUtils.ts' import { basename } from '@nextcloud/paths' -import { triggerActionForFile } from '../files/FilesUtils.ts' +import { openActionsMenu, triggerActionForFile } from '../files/FilesUtils.ts' import { createShare } from '../files_sharing/FilesSharingUtils.ts' export function uploadThreeVersions(user: User, fileName: string) { - // A new version will not be created if the changes occur - // within less than one second of each other. - // eslint-disable-next-line cypress/no-unnecessary-waiting - cy.uploadContent(user, new Blob(['v1'], { type: 'text/plain' }), 'text/plain', `/${fileName}`) - .wait(1100) - .uploadContent(user, new Blob(['v2'], { type: 'text/plain' }), 'text/plain', `/${fileName}`) - .wait(1100) - .uploadContent(user, new Blob(['v3'], { type: 'text/plain' }), 'text/plain', `/${fileName}`) + // A version is identified by the file's modification time at second + // resolution (files_versions/.v), so two uploads that resolve to + // the same second collapse into a single version. Wall-clock spacing (cy.wait) + // is racy on slow runners: the mtime is set server-side at write time, so + // jitter can still floor two uploads into the same second. Pin an explicit, + // distinct mtime per upload instead (via X-OC-MTime) so exactly three versions + // are always created regardless of runner speed. + const baseMtime = Math.floor(Date.now() / 1000) - 5 + cy.uploadContent(user, new Blob(['v1'], { type: 'text/plain' }), 'text/plain', `/${fileName}`, baseMtime) + cy.uploadContent(user, new Blob(['v2'], { type: 'text/plain' }), 'text/plain', `/${fileName}`, baseMtime + 2) + cy.uploadContent(user, new Blob(['v3'], { type: 'text/plain' }), 'text/plain', `/${fileName}`, baseMtime + 4) cy.login(user) } @@ -39,15 +42,40 @@ export function openVersionsPanel(fileName: string) { cy.get('#tab-files_versions').should('be.visible', { timeout: 10000 }) } -export function toggleVersionMenu(index: number) { - cy.get('#tab-files_versions [data-files-versions-version]') +function getVersionMenuToggle(index: number) { + return cy.get('#tab-files_versions [data-files-versions-version]') .eq(index) .find('button') - .click() +} + +/** + * Open a version's actions menu. The version rows use the same NcActions menu + * as the file list, which on slow (CI) runners can drop the opening click or + * take several frames to display the popover — so open it with the shared + * robust helper instead of a single naive click. + * + * @param index the version row index + */ +export function openVersionMenu(index: number) { + openActionsMenu(() => getVersionMenuToggle(index)) +} + +/** + * Close a version's actions menu that was opened with openVersionMenu. Clicking + * the toggle while it is expanded collapses the popover. + * + * @param index the version row index + */ +export function closeVersionMenu(index: number) { + getVersionMenuToggle(index).then(($toggle) => { + if ($toggle.attr('aria-expanded') === 'true') { + cy.wrap($toggle).click({ force: true }) + } + }) } export function triggerVersionAction(index: number, actionName: string) { - toggleVersionMenu(index) + openVersionMenu(index) cy.get(`[data-cy-files-versions-version-action="${actionName}"]`).filter(':visible').click() } @@ -71,9 +99,11 @@ export function deleteVersion(index: number) { } export function doesNotHaveAction(index: number, actionName: string) { - toggleVersionMenu(index) + openVersionMenu(index) cy.get(`[data-cy-files-versions-version-action="${actionName}"]`).should('not.exist') - toggleVersionMenu(index) + // Close the menu again so its entries do not leak into the next assertion + // (the action query above is global). + closeVersionMenu(index) } export function assertVersionContent(index: number, expectedContent: string) { @@ -92,6 +122,34 @@ export function setupTestSharedFileFromUser(owner: User, randomFileName: string, cy.login(recipient) cy.visit('/apps/files') + // On a slow backend the freshly created share can be missing from the + // recipient's first directory listing: the mount cache is updated a + // moment after the share is committed, and the file list does not + // refetch on its own. Reload until the shared file shows up so + // downstream steps start from a stable state. + reloadUntilFileVisible(basename(randomFileName)) return cy.wrap(recipient) }) } + +/** + * Reload the current file list until the given file appears in it. + * + * @param fileName Name of the file expected in the current directory + * @param attemptsLeft Remaining reloads before giving up + */ +function reloadUntilFileVisible(fileName: string, attemptsLeft = 5) { + // The list has rendered once at least one row is present (a new user always + // has welcome.txt), so we can reliably tell "file missing" from "still loading". + cy.get('[data-cy-files-list-row-name]').should('have.length.at.least', 1) + cy.get('body').then(($body) => { + if ($body.find(`[data-cy-files-list-row-name="${CSS.escape(fileName)}"]`).length > 0) { + return + } + if (attemptsLeft === 0) { + throw new Error(`Shared file "${fileName}" never appeared in the recipient's file list after reloading`) + } + cy.reload() + reloadUntilFileVisible(fileName, attemptsLeft - 1) + }) +} diff --git a/cypress/support/e2e.ts b/cypress/support/e2e.ts index 30c1f008550e1..cc0b63287a4c0 100644 --- a/cypress/support/e2e.ts +++ b/cypress/support/e2e.ts @@ -26,3 +26,72 @@ Cypress.on('window:before:load', (win) => { } } }) + +// Optional CPU throttling to reproduce CI-like renderer slowness locally. +// Usage: CYPRESS_CPU_THROTTLE=8 npx cypress run ... +const cpuThrottle = Number(Cypress.env('CPU_THROTTLE')) +if (cpuThrottle > 1) { + beforeEach(() => { + Cypress.automation('remote:debugger:protocol', { + command: 'Emulation.setCPUThrottlingRate', + params: { rate: cpuThrottle }, + }) + }) +} + +// Repro-only: delay file-preview responses so the row re-render they trigger +// lands during the actions-menu open window (the flake we are chasing). +// Usage: CYPRESS_PREVIEW_DELAY=1500 npx cypress run ... +const previewDelay = Number(Cypress.env('PREVIEW_DELAY')) +if (previewDelay > 0) { + beforeEach(() => { + cy.intercept('GET', '**/core/preview?**', (req) => { + req.on('response', (res) => { + res.setDelay(previewDelay) + }) + }).as('delayedPreview') + }) + // Report to the terminal how many preview requests were intercepted/delayed, + // so we can confirm the repro knob is actually engaging. + afterEach(() => { + cy.get<{ length: number }>('@delayedPreview.all', { log: false }).then((calls) => { + cy.task('log', `[preview-delay] delayed ${calls?.length ?? 0} preview request(s) by ${previewDelay}ms`) + }) + }) +} + +// Repro-only: deterministically recreate the real flake mechanism — a file-row +// preview finishing loading re-renders the row and closes a just-opened actions +// menu. A MutationObserver watches for a row action toggle reporting +// aria-expanded="true" and, at that instant, forces the row's preview to +// reload so its @load handler fires a reactive re-render right on top of the +// opening menu. Usage: CYPRESS_FORCE_RERENDER=1 npx cypress run ... +if (Cypress.env('FORCE_RERENDER')) { + Cypress.on('window:before:load', (win) => { + const forceReloadPreviewForToggle = (toggle: Element) => { + const row = toggle.closest('[data-cy-files-list-row]') + const img = row?.querySelector('.files-list__row-icon-preview, img') + if (img?.src) { + const src = img.src + img.src = '' + // Reassign on the next frame so the browser refetches and re-fires @load + win.requestAnimationFrame(() => { + img.src = src.includes('?') ? `${src}&_r=${Date.now()}` : `${src}?_r=${Date.now()}` + }) + } + } + const observer = new win.MutationObserver((mutations) => { + for (const m of mutations) { + const target = m.target as Element + if (m.attributeName === 'aria-expanded' + && target.getAttribute('aria-expanded') === 'true' + && target.closest('[data-cy-files-list-row-actions]')) { + forceReloadPreviewForToggle(target) + } + } + }) + win.document.addEventListener('DOMContentLoaded', () => { + observer.observe(win.document.body, { attributes: true, subtree: true, attributeFilter: ['aria-expanded'] }) + }) + }) +} diff --git a/playwright.config.ts b/playwright.config.ts index bf88178fb95b4..01d4e7f18d494 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -9,9 +9,14 @@ export default defineConfig({ testDir: './tests/playwright/e2e', fullyParallel: true, forbidOnly: !!process.env.CI, - retries: process.env.CI ? 1 : 0, workers: process.env.CI ? 1 : undefined, reporter: process.env.CI ? [['blob'], ['dot'], ['github']] : 'html', + // // Higher timeouts are necessary to avoid + // // failures on CI runners with very high load + // timeout: process.env.CI ? 150_000 : 30_000, + // expect: { + // timeout: process.env.CI ? 25_000 : 5_000, + // }, use: { baseURL: 'http://localhost:8042/index.php/', trace: 'on-first-retry', diff --git a/tests/lib/Memcache/MemcachedTest.php b/tests/lib/Memcache/MemcachedTest.php index ec9513988fd12..be2c97fd3217d 100644 --- a/tests/lib/Memcache/MemcachedTest.php +++ b/tests/lib/Memcache/MemcachedTest.php @@ -30,6 +30,13 @@ public static function setUpBeforeClass(): void { protected function setUp(): void { parent::setUp(); $this->instance = new Memcached($this->getUniqueID()); + // Flush the shared \Memcached singleton so that its internal result + // code is reset to RES_SUCCESS before the first get() of each test. + // Without this, a prior test that ended with a successful set/get + // leaves RES_SUCCESS in the singleton, and if the underlying library + // then returns false+RES_SUCCESS for a missing key the get() wrapper + // cannot tell the difference from a stored false value. + $this->instance->clear(); } #[\Override] diff --git a/tests/playwright/support/utils/password-confirmation.ts b/tests/playwright/support/utils/password-confirmation.ts index 9524f1f5094f2..8da7ff7047f50 100644 --- a/tests/playwright/support/utils/password-confirmation.ts +++ b/tests/playwright/support/utils/password-confirmation.ts @@ -5,6 +5,8 @@ import type { Page } from '@playwright/test' +import { expect } from '@playwright/test' + /** * Handle the password confirmation dialog if it appears * @@ -14,21 +16,13 @@ import type { Page } from '@playwright/test' export async function handlePasswordConfirmation(page: Page, password = 'admin') { const dialog = page.locator('.modal-container:has-text("Authentication required")') - try { - // Check if the dialog exists within a short timeout - const dialogVisible = await dialog.isVisible({ timeout: 500 }).catch(() => false) - - if (dialogVisible) { - // Fill the password field + await expect(dialog).toBeVisible({ timeout: 500 }) + .then(async () => { await dialog.locator('input[type="password"]').fill(password) - - // Click the confirm button await dialog.getByRole('button', { name: 'Confirm' }).click() - - // Wait for the dialog to disappear - await dialog.waitFor({ state: 'hidden' }) - } - } catch { - // Dialog didn't appear, which is fine - some operations might not require confirmation - } + await expect(dialog).toBeHidden() + }) + .catch(() => { + // Dialog didn't appear — some operations don't require confirmation. + }) }