From 5b701d59c7c7f4960156ec361d35991db0957f2e Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 3 Nov 2026 17:28:04 +0100 Subject: [PATCH 1/6] fix(sync): reset outdated document when the file changed outside of Nextcloud Writes that bypass Nextcloud, e.g. directly on external storage, do not trigger the NodeWritten listeners, so a stored document and its yjs state were reused when the file was opened again, and the editor showed outdated content. When opening a session, compare the stored checksum with the current file. If it differs and the document has no unsaved changes, reset the document so the client loads the current file content. Documents with unsaved changes are kept so the conflict is raised on the next save. Refs: #9236 Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Service/DocumentService.php | 16 ++++++-- tests/unit/Service/DocumentServiceTest.php | 48 ++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 0532c291be3..1f7106be054 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -116,10 +116,20 @@ public function isSaveFromText(): bool { * @throws Exception */ public function getOrCreateDocument(File $file): Document { + $checksum = self::computeCheckSum($file->getContent()); $document = $this->getDocument($file->getId()); if ($document !== null) { - $this->logger->info('Keep previous document of ' . $file->getId()); - return $document; + if ($document->getChecksum() === $checksum || $this->hasUnsavedChanges($document)) { + $this->logger->info('Keep previous document of ' . $file->getId()); + return $document; + } + // The file was changed without passing through Nextcloud, e.g. directly on external storage. + $this->logger->info('Reset outdated document of ' . $file->getId()); + try { + $this->resetDocument($file->getId()); + } catch (DocumentHasUnsavedChangesException) { + return $document; + } } if (!$this->ensureDocumentsFolder()) { @@ -133,7 +143,7 @@ public function getOrCreateDocument(File $file): Document { $document->setLastSavedVersionTime($file->getMTime()); $document->setLastSavedVersionEtag($file->getEtag()); $document->setBaseVersionEtag(uniqid()); - $document->setChecksum(self::computeCheckSum($file->getContent())); + $document->setChecksum($checksum); try { /** @var Document $document */ $document = $this->documentMapper->insert($document); diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php index 0a317b0d3f8..2e0adc402f1 100644 --- a/tests/unit/Service/DocumentServiceTest.php +++ b/tests/unit/Service/DocumentServiceTest.php @@ -191,4 +191,52 @@ public function testGetFileForSessionFromShareOfOtherFile() { $this->expectException(\InvalidArgumentException::class); $this->documentService->getFileForSession($session, 'shareToken'); } + + private function createDocumentWithContent(string $content): Document { + $document = new Document(); + $document->setId(42); + $document->setLastSavedVersion(3); + $document->setChecksum(DocumentService::computeCheckSum($content)); + return $document; + } + + private function createFile(string $content): File { + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(42); + $file->method('getContent')->willReturn($content); + $file->method('getMTime')->willReturn(1000); + $file->method('getEtag')->willReturn('etag'); + return $file; + } + + public function testKeepsDocumentWhenFileIsUnchanged(): void { + $loaded = $this->createDocumentWithContent('content'); + $this->documentMapper->method('find')->willReturn($loaded); + $this->documentMapper->expects(self::never())->method('delete'); + $this->documentMapper->expects(self::never())->method('insert'); + + self::assertSame($loaded, $this->documentService->getOrCreateDocument($this->createFile('content'))); + } + + public function testKeepsDocumentWithUnsavedChangesWhenFileChangedOutside(): void { + $loaded = $this->createDocumentWithContent('content'); + $this->documentMapper->method('find')->willReturn($loaded); + $this->setpMapper->method('getLatestVersion')->willReturn(4); + $this->documentMapper->expects(self::never())->method('delete'); + $this->documentMapper->expects(self::never())->method('insert'); + + self::assertSame($loaded, $this->documentService->getOrCreateDocument($this->createFile('changed outside'))); + } + + public function testResetsDocumentWhenFileChangedOutside(): void { + $loaded = $this->createDocumentWithContent('content'); + $this->documentMapper->method('find')->willReturn($loaded); + $this->setpMapper->method('getLatestVersion')->willReturn(3); + $this->documentMapper->expects(self::once())->method('delete')->with($loaded); + $this->documentMapper->expects(self::once())->method('insert')->willReturnArgument(0); + + $document = $this->documentService->getOrCreateDocument($this->createFile('changed outside')); + self::assertNotSame($loaded, $document); + self::assertSame(DocumentService::computeCheckSum('changed outside'), $document->getChecksum()); + } } From 3454cefec9f7de1c01b69f1aabe35c03d0e18a6e Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 3 Nov 2026 17:28:53 +0100 Subject: [PATCH 2/6] fix(sync): show conflict dialog when saving fails with 409 Since conflicts are no longer reported by the sync endpoint, a save rejected because the file changed outside of Text returned 409 with the outside content, but SaveService ignored it. The editor kept retrying silently and the changes were never saved. Emit a SAVE_COLLISION error so the conflict resolution dialog is shown. Refs: #9236 Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- src/services/SaveService.ts | 14 ++++++- src/tests/services/SaveService.spec.ts | 51 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 src/tests/services/SaveService.spec.ts diff --git a/src/services/SaveService.ts b/src/services/SaveService.ts index 14c6430a73f..4786e1a75e6 100644 --- a/src/services/SaveService.ts +++ b/src/services/SaveService.ts @@ -93,12 +93,24 @@ class SaveService { } catch (e) { logger.error('Failed to save document.', { error: e }) const response = ( - e as { response?: { status?: number; data?: { error?: string } } } + e as { + response?: { + status?: number + data?: { error?: string; outsideChange?: string } + } + } ).response if (response?.status === 403) { // Document is now read-only; permissionChange from sync will update the UI return } + if (response?.status === 409) { + this.bus.emit('error', { + type: ERROR_TYPE.SAVE_COLLISION, + data: { outsideChange: response.data?.outsideChange }, + }) + return + } if (response?.status === 412) { this.bus.emit('error', { type: ERROR_TYPE.LOAD_ERROR, diff --git a/src/tests/services/SaveService.spec.ts b/src/tests/services/SaveService.spec.ts new file mode 100644 index 00000000000..561e5cd9ea4 --- /dev/null +++ b/src/tests/services/SaveService.spec.ts @@ -0,0 +1,51 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { Connection } from '../../composables/useConnection.ts' + +import { describe, expect, it, vi } from 'vitest' +import { ref, shallowRef } from 'vue' +import * as saveApi from '../../apis/save.ts' +import { SaveService } from '../../services/SaveService.ts' +import { ERROR_TYPE } from '../../services/SyncService.ts' + +vi.mock('../../apis/save.ts') + +const connection = shallowRef({ + documentId: 123, + sessionId: 345, + sessionToken: 'sessionToken', + filePath: './', + baseVersionEtag: 'etag', +}) +function getSaveData() { + return { + version: 1, + autosaveContent: 'content', + documentState: 'state', + } +} + +describe('SaveService', () => { + it('emits a save collision when the file was changed outside', async () => { + vi.mocked(saveApi.save).mockRejectedValue({ + response: { status: 409, data: { outsideChange: 'outside content' } }, + }) + const saveService = new SaveService({ + connection, + document: ref(undefined), + getSaveData, + }) + const onError = vi.fn() + saveService.bus.on('error', onError) + + await saveService.save() + + expect(onError).toHaveBeenCalledWith({ + type: ERROR_TYPE.SAVE_COLLISION, + data: { outsideChange: 'outside content' }, + }) + }) +}) From 5c54c07a98edbb7edaf388e11102dcdacd8b9b2b Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 3 Nov 2026 18:21:19 +0100 Subject: [PATCH 3/6] test(playwright): test conflict resolution on outside changes Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- playwright/e2e/outside-change.spec.ts | 96 ++++++++++++++++++++++++++ playwright/support/fixtures/User.ts | 18 ++++- src/tests/services/SaveService.spec.ts | 2 + 3 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 playwright/e2e/outside-change.spec.ts diff --git a/playwright/e2e/outside-change.spec.ts b/playwright/e2e/outside-change.spec.ts new file mode 100644 index 00000000000..2e316b42259 --- /dev/null +++ b/playwright/e2e/outside-change.spec.ts @@ -0,0 +1,96 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { expect, mergeTests } from '@playwright/test' +import { test as editorTest } from '../support/fixtures/editor' +import { test as randomUserTest } from '../support/fixtures/random-user' +import { test as uploadFileTest } from '../support/fixtures/upload-file' + +const test = mergeTests(editorTest, randomUserTest, uploadFileTest) + +test.beforeEach(async ({ open }) => { + await open() +}) + +const resolutionVariants = [ + { + source: 'local', + buttonName: /Keep my local changes and overwrite the remote version/, + headingName: 'Hello world', + }, + { + source: 'server', + buttonName: /Discard my local changes and use the remote version/, + headingName: 'Good bye', + }, +] + +resolutionVariants.forEach(({ source, buttonName, headingName }) => { + test(`[conflict] saving after the file was changed outside of Nextcloud - resolve with ${source} version`, async ({ + container, + editor, + file, + page, + reader, + user, + }) => { + await expect(editor.el).toBeVisible() + const pushPromise = page.waitForRequest(/push/) + await editor.typeHeading('Hello world') + await pushPromise + await user.writeFileOutsideNextcloud({ + name: file.name, + content: '## Good bye', + }) + // Clicking the save indicator would force the save and skip the conflict check. + await page.keyboard.press('ControlOrMeta+s') + + // Verify both versions are shown + await expect(reader.getHeading({ name: 'Good bye' })).toBeVisible({ + timeout: 10_000, + }) + await expect(editor.getHeading({ name: 'Hello world' })).toBeVisible() + + // Resolve conflict + await container.getButton({ name: buttonName }).click() + await expect(reader.el).not.toBeVisible() + await expect(editor.getHeading({ name: headingName })).toBeVisible() + await expect(async () => { + expect(await file.getContent()).toContain(headingName) + }).toPass() + }) +}) + +test('reopening after the file was changed outside of Nextcloud shows the new content', async ({ + close, + container, + editor, + file, + page, + reader, + user, +}) => { + await expect(editor.el).toBeVisible() + const pushPromise = page.waitForRequest(/push/) + await editor.typeHeading('Hello world') + await pushPromise + await editor.saveIndicator.click() + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) + await close() + + await user.writeFileOutsideNextcloud({ name: file.name, content: '## Good bye' }) + await file.open() + + await expect(editor.getHeading({ name: 'Good bye' })).toBeVisible({ + timeout: 10_000, + }) + await expect(editor.getHeading({ name: 'Hello world' })).not.toBeVisible() + await expect(reader.el).not.toBeVisible() + await expect( + container.getButton({ + name: /Keep my local changes and overwrite the remote version/, + }), + ).not.toBeVisible() +}) diff --git a/playwright/support/fixtures/User.ts b/playwright/support/fixtures/User.ts index f12fa2e504e..6b5fd1643b3 100644 --- a/playwright/support/fixtures/User.ts +++ b/playwright/support/fixtures/User.ts @@ -3,8 +3,10 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { type Page } from '@playwright/test' import type { User as Account } from '@nextcloud/e2e-test-server' +import type { Page } from '@playwright/test' + +import { runExec } from '@nextcloud/e2e-test-server/docker' import { uploadFile } from './Node' export class User { @@ -23,4 +25,18 @@ export class User { return uploadFile({ ...upload, owner: this }) } + /** + * Write a file directly to the user's storage on the server, bypassing Nextcloud + * like a change on external storage would. + * + * @param options options for the write + * @param options.name Name of the file + * @param options.content File content + */ + async writeFileOutsideNextcloud({ name, content }: { name: string, content: string }) { + await runExec( + ['sh', '-c', 'printf "%s" "$CONTENT" > "data/$OWNER/files/$NAME"'], + { env: [`CONTENT=${content}`, `OWNER=${this.account.userId}`, `NAME=${name}`] }, + ) + } } diff --git a/src/tests/services/SaveService.spec.ts b/src/tests/services/SaveService.spec.ts index 561e5cd9ea4..30260057c4b 100644 --- a/src/tests/services/SaveService.spec.ts +++ b/src/tests/services/SaveService.spec.ts @@ -20,6 +20,8 @@ const connection = shallowRef({ filePath: './', baseVersionEtag: 'etag', }) +/** + */ function getSaveData() { return { version: 1, From df30d085e270f0bb3d19052d9cd8f906167e3984 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 4 Nov 2026 18:19:00 +0100 Subject: [PATCH 4/6] fix(sync): detect outside changes on save even if cached etag/mtime are unchanged On stable34, assertNoOutsideConflict() skipped the checksum comparison when the file cache's etag and mtime matched the document. Files changed directly on storage (e.g. external storage, filesystem_check_changes=0) keep their cached etag/mtime, so the change went undetected and was overwritten on save. Always compare the checksum, as stable35 does. Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Service/DocumentService.php | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 1f7106be054..43533caafb1 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -350,24 +350,18 @@ public function assertNoOutsideConflict(Document $document, File $file, bool $fo return; } - $fileMtime = $file->getMtime(); - $fileEtag = $file->getEtag(); - - if ($lastEtag === $fileEtag && $lastMTime === $fileMtime) { - return; - } - - $storedChecksum = $document->getChecksum(); - $fileContent = $file->getContent(); - $fileChecksum = self::computeCheckSum($fileContent); - - if ($storedChecksum !== $fileChecksum) { + $fileChecksum = self::computeCheckSum($file->getContent()); + if ($document->getChecksum() !== $fileChecksum) { throw new DocumentSaveConflictException('File changed in the meantime from outside'); } - $document->setLastSavedVersionTime($fileMtime); - $document->setLastSavedVersionEtag($fileEtag); - $this->documentMapper->update($document); + $fileMtime = $file->getMtime(); + $fileEtag = $file->getEtag(); + if ($lastEtag !== $fileEtag || $lastMTime !== $fileMtime) { + $document->setLastSavedVersionTime($fileMtime); + $document->setLastSavedVersionEtag($fileEtag); + $this->documentMapper->update($document); + } } /** From 3d61dc2cc8df18a53054d88a166abeee5a6ed64f Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 4 Nov 2026 21:43:19 +0100 Subject: [PATCH 5/6] ci(playwright): don't fail fast playwright jobs Signed-off-by: Jonas --- .github/workflows/playwright.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index 551325b62a5..7bd5beb4241 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -15,6 +15,7 @@ jobs: timeout-minutes: 60 runs-on: ubuntu-latest strategy: + fail-fast: false matrix: shardIndex: [1, 2, 3] shardTotal: [3] From a0ae7b6927df1626d1b852f1b0cc3cc249ed3ded Mon Sep 17 00:00:00 2001 From: Jonas Date: Mon, 29 Jun 2026 21:15:27 +0200 Subject: [PATCH 6/6] test(playwright): use shared CDP session in offline fixture Signed-off-by: Jonas --- playwright/support/fixtures/offline.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/playwright/support/fixtures/offline.ts b/playwright/support/fixtures/offline.ts index 0163aa9673b..276570bf6db 100644 --- a/playwright/support/fixtures/offline.ts +++ b/playwright/support/fixtures/offline.ts @@ -33,14 +33,16 @@ const setClientOffline = async (client: CDPSession): Promise => { /** * setOffline will turn the network off for the rest of the test and then on again. */ -export const test = base.extend({ - setOffline: async ({ context, page }, use) => { +export const test = base.extend({ + _cdpClient: async ({ context, page }, use) => { const client = await context.newCDPSession(page) - await use (() => setClientOffline(client)) - await setClientOnline(client) + await use(client) }, - setOnline: async ({ context, page }, use) => { - const client = await context.newCDPSession(page) - await use (() => setClientOnline(client)) + setOffline: async ({ _cdpClient }, use) => { + await use(() => setClientOffline(_cdpClient)) + await setClientOnline(_cdpClient) + }, + setOnline: async ({ _cdpClient }, use) => { + await use(() => setClientOnline(_cdpClient)) }, })