[stable34] fix: don't silently drop outside changes (e.g. from external storage)

.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]

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);
@@ -340,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);
+		}
 	}
 
 	/**

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()
+})

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}`] },
+		)
+	}
 }

playwright/support/fixtures/offline.ts

@@ -33,14 +33,16 @@ const setClientOffline = async (client: CDPSession): Promise<void> => {
 /**
  * setOffline will turn the network off for the rest of the test and then on again.
  */
-export const test = base.extend<OfflineFixture>({
-	setOffline: async ({ context, page }, use) => {
+export const test = base.extend<OfflineFixture & { _cdpClient: CDPSession }>({
+	_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))
 	},
 })

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,

src/tests/services/SaveService.spec.ts

@@ -0,0 +1,53 @@
+/**
+ * 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<Connection | undefined>({
+	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' },
+		})
+	})
+})

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());
+	}
 }