From 3f94733c5f372fb717324d55cbf0a5a774643868 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 1 Sep 2026 09:05:27 +0200 Subject: [PATCH 1/4] Revert "fix(sync): do not mistake own save requests for outside changes" This reverts commit a4eeb24d08288d619ab63eaa6bab81736e54077e. Signed-off-by: Max --- lib/Service/DocumentService.php | 12 +- tests/unit/Service/DocumentServiceTest.php | 150 --------------------- 2 files changed, 1 insertion(+), 161 deletions(-) delete mode 100644 tests/unit/Service/DocumentServiceTest.php diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 0f8ca17cf02..d4bcdf01747 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -307,7 +307,6 @@ public function getSteps(int $documentId, int $lastVersion): array { /** * @throws DocumentSaveConflictException - * @throws DoesNotExistException * @throws InvalidPathException * @throws NotFoundException */ @@ -332,16 +331,7 @@ public function assertNoOutsideConflict(Document $document, File $file, bool $fo $fileChecksum = self::computeCheckSum($fileContent); if ($storedChecksum !== $fileChecksum) { - // $document was loaded at the start of the request. - // A save request handled in the meantime is not reflected in it - // and would be mistaken for an outside change. - // Reload the document to compare against the latest saved state. - $document = $this->documentMapper->find($documentId); - if ($document->getChecksum() !== $fileChecksum) { - throw new DocumentSaveConflictException('File changed in the meantime from outside'); - } - // The save request already stored the latest version info. - return; + throw new DocumentSaveConflictException('File changed in the meantime from outside'); } $document->setLastSavedVersionTime($fileMtime); diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php deleted file mode 100644 index bf22c06aa68..00000000000 --- a/tests/unit/Service/DocumentServiceTest.php +++ /dev/null @@ -1,150 +0,0 @@ -documentMapper = $this->createMock(DocumentMapper::class); - $this->fileService = $this->createMock(FileService::class); - $this->cache = $this->createMock(ICache::class); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->method('createDistributed')->willReturn($this->cache); - $request = $this->createMock(IRequest::class); - $request->method('getParam')->willReturn(null); - - $this->fileService->method('isReadOnly')->willReturn(false); - - $this->documentService = new DocumentService( - $this->documentMapper, - $this->fileService, - $this->createMock(StepMapper::class), - $this->createMock(SessionMapper::class), - $this->createMock(IAppData::class), - 'admin', - $this->createMock(IRootFolder::class), - $cacheFactory, - $this->createMock(LoggerInterface::class), - $this->createMock(LockService::class), - $request, - $this->createMock(IManager::class), - $this->createMock(IUserMountCache::class), - $this->createMock(IConfig::class), - ); - } - - private function createDocument(string $etag, int $mtime, string $content): Document { - $document = new Document(); - $document->setId(123); - $document->setLastSavedVersionEtag($etag); - $document->setLastSavedVersionTime($mtime); - $document->setChecksum(DocumentService::computeCheckSum($content)); - return $document; - } - - private function mockFile(string $etag, int $mtime, string $content): File { - $file = $this->createMock(File::class); - $file->method('getEtag')->willReturn($etag); - $file->method('getMtime')->willReturn($mtime); - $file->method('getContent')->willReturn($content); - return $file; - } - - public function testNoConflictWhenVersionInfoMatches(): void { - $document = $this->createDocument('etag1', 1000, 'content'); - $file = $this->mockFile('etag1', 1000, 'content'); - - $this->documentMapper->expects(self::never())->method('find'); - $this->documentMapper->expects(self::never())->method('update'); - - $this->documentService->assertNoOutsideConflict($document, $file); - } - - public function testRefreshesVersionInfoWhenContentMatches(): void { - $document = $this->createDocument('etag1', 1000, 'content'); - $file = $this->mockFile('etag2', 2000, 'content'); - - $this->documentMapper->expects(self::never())->method('find'); - $this->documentMapper->expects(self::once()) - ->method('update') - ->with($document); - - $this->documentService->assertNoOutsideConflict($document, $file); - self::assertSame('etag2', $document->getLastSavedVersionEtag()); - self::assertSame(2000, $document->getLastSavedVersionTime()); - } - - public function testNoConflictWhenOwnSaveFinishedInTheMeantime(): void { - // Loaded at the start of the request - stale by now. - $document = $this->createDocument('etag1', 1000, 'old content'); - // A save request updated the file in the meantime ... - $file = $this->mockFile('etag2', 2000, 'new content'); - // ... and stored the new version info in the document. - $freshDocument = $this->createDocument('etag2', 2000, 'new content'); - - $this->documentMapper->expects(self::once()) - ->method('find') - ->with(123) - ->willReturn($freshDocument); - $this->documentMapper->expects(self::never())->method('update'); - - $this->documentService->assertNoOutsideConflict($document, $file); - } - - public function testConflictWhenFileChangedFromOutside(): void { - $document = $this->createDocument('etag1', 1000, 'old content'); - $file = $this->mockFile('etag2', 2000, 'outside content'); - // The latest saved state does not match the file either. - $freshDocument = $this->createDocument('etag1', 1000, 'old content'); - - $this->documentMapper->expects(self::once()) - ->method('find') - ->with(123) - ->willReturn($freshDocument); - - $this->expectException(DocumentSaveConflictException::class); - $this->documentService->assertNoOutsideConflict($document, $file); - } - - public function testNoConflictWhileSaveLockIsHeld(): void { - $document = $this->createDocument('etag1', 1000, 'old content'); - $file = $this->mockFile('etag2', 2000, 'new content'); - - $this->cache->method('get') - ->with('document-save-lock-123') - ->willReturn(true); - $this->documentMapper->expects(self::never())->method('find'); - $this->documentMapper->expects(self::never())->method('update'); - - $this->documentService->assertNoOutsideConflict($document, $file); - } -} From 9dc54e73eaef53c07075398298683daca4a37c67 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 1 Sep 2026 09:08:06 +0200 Subject: [PATCH 2/4] fix(sync): do not check for conflicts during sync The document may be in the middle of getting saved. When the file has been updated but the metadata in the documents table has not it may seem like the file was overwritten even though text is saving the file. This does not indicate an actual conflict. Avoid triggering the conflict screen in that race condition. Signed-off-by: Max --- lib/Service/ApiService.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 015bcccfd1e..c75b149bbfd 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -175,7 +175,6 @@ public function sync(Session $session, Document $document, int $version = 0, ?st // ensure file is still present and accessible $file = $this->fileService->getFileForSession($session, $shareToken); $result['readOnly'] = $this->fileService->isReadOnly($file, $shareToken); - $this->documentService->assertNoOutsideConflict($document, $file); } catch (NotPermittedException|NotFoundException|InvalidPathException $e) { $this->logger->info($e->getMessage(), ['exception' => $e]); return new DataResponse([ @@ -186,16 +185,9 @@ public function sync(Session $session, Document $document, int $version = 0, ?st return new DataResponse([ 'message' => 'Document no longer exists' ], Http::STATUS_NOT_FOUND); - } catch (DocumentSaveConflictException) { - try { - /** @psalm-suppress PossiblyUndefinedVariable */ - $result['outsideChange'] = $file->getContent(); - } catch (LockedException) { - // Ignore locked exception since it might happen due to an autosave action happening at the same time - } } - return new DataResponse($result, isset($result['outsideChange']) ? Http::STATUS_CONFLICT : Http::STATUS_OK); + return new DataResponse($result, Http::STATUS_OK); } public function save(Session $session, Document $document, int $version, string $autosaveContent, string $documentState, bool $force = false, bool $manualSave = false, ?string $shareToken = null): DataResponse { From 023d17d2c95420cac3f0ab66b85b5993bba90002 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 1 Sep 2026 19:01:04 +0200 Subject: [PATCH 3/4] fix(save): extend ttl for document save lock to 1 Minute We observed save requests that took longer than 30 seconds. The lock needs to outlive those in order to prevent superfluous 409 responses. Signed-off-by: Max --- lib/Service/DocumentService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index d4bcdf01747..2dbf91248c4 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -402,7 +402,7 @@ public function autosave(Document $document, File $file, int $version, string $a return $document; } - $this->cache->set('document-save-lock-' . $documentId, true, 10); + $this->cache->set('document-save-lock-' . $documentId, true, 60); try { $this->lockService->runInScope($file, function () use ($file, $autoSaveDocument, $documentState): void { $this->saveFromText = true; From d331c8f93a6f46c19eea89d7df027ffdef26cf3d Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 2 Sep 2026 04:27:43 +0200 Subject: [PATCH 4/4] fix(save): autosave only every ten seconds Abort autosave if another save is still under way. We observed save requests taking up to 30 seconds. In this timespan autosave requests would still be processed as the lastSavedTime in the database has not been updated yet. Signed-off-by: Max --- lib/Service/DocumentService.php | 5 + tests/unit/Service/DocumentServiceTest.php | 137 +++++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 tests/unit/Service/DocumentServiceTest.php diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 2dbf91248c4..91e510b08b4 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -364,6 +364,11 @@ public function autosave(Document $document, File $file, int $version, string $a $this->assertNoOutsideConflict($document, $file, $force); + // Abort autosave if already saving. + if ($this->cache->get('document-save-lock-' . $documentId) && $manualSave === false) { + return $document; + } + // Do not save if newer version already saved // Note that $version is the version of the steps the client has fetched. // It may have added steps on top of that - so if the versions match we still save. diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php new file mode 100644 index 00000000000..6fb5497c215 --- /dev/null +++ b/tests/unit/Service/DocumentServiceTest.php @@ -0,0 +1,137 @@ +documentMapper = $this->createMock(DocumentMapper::class); + $this->fileService = $this->createMock(FileService::class); + $this->cache = $this->createMock(ICache::class); + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('createDistributed')->willReturn($this->cache); + $request = $this->createMock(IRequest::class); + $request->method('getParam')->willReturn(null); + + $this->fileService->method('isReadOnly')->willReturn(false); + + $this->documentService = new DocumentService( + $this->documentMapper, + $this->fileService, + $this->createMock(StepMapper::class), + $this->createMock(SessionMapper::class), + $this->createMock(IAppData::class), + 'admin', + $this->createMock(IRootFolder::class), + $cacheFactory, + $this->createMock(LoggerInterface::class), + $this->createMock(LockService::class), + $request, + $this->createMock(IManager::class), + $this->createMock(IUserMountCache::class), + $this->createMock(IConfig::class), + ); + } + + private function createDocument(string $etag, int $mtime, string $content): Document { + $document = new Document(); + $document->setId(123); + $document->setLastSavedVersionEtag($etag); + $document->setLastSavedVersionTime($mtime); + $document->setChecksum(DocumentService::computeCheckSum($content)); + return $document; + } + + private function mockFile(string $etag, int $mtime, string $content): File { + $file = $this->createMock(File::class); + $file->method('getEtag')->willReturn($etag); + $file->method('getMtime')->willReturn($mtime); + $file->method('getContent')->willReturn($content); + return $file; + } + + public function testNoConflictWhenVersionInfoMatches(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $file = $this->mockFile('etag1', 1000, 'content'); + + $this->documentMapper->expects(self::never())->method('update'); + $this->documentService->assertNoOutsideConflict($document, $file); + } + + public function testRefreshesVersionInfoWhenContentMatches(): void { + $document = $this->createDocument('etag1', 1000, 'content'); + $file = $this->mockFile('etag2', 2000, 'content'); + + $this->documentMapper->expects(self::never())->method('find'); + $this->documentMapper->expects(self::once()) + ->method('update') + ->with($document); + + $this->documentService->assertNoOutsideConflict($document, $file); + self::assertSame('etag2', $document->getLastSavedVersionEtag()); + self::assertSame(2000, $document->getLastSavedVersionTime()); + } + + public function testConflictWhenFileChangedFromOutside(): void { + $document = $this->createDocument('etag1', 1000, 'old content'); + $file = $this->mockFile('etag2', 2000, 'outside content'); + + $this->expectException(DocumentSaveConflictException::class); + $this->documentService->assertNoOutsideConflict($document, $file); + } + + public function testNoConflictWhileSaveLockIsHeld(): void { + $document = $this->createDocument('etag1', 1000, 'old content'); + $file = $this->mockFile('etag2', 2000, 'new content'); + + $this->cache->method('get') + ->with('document-save-lock-123') + ->willReturn(true); + $this->documentMapper->expects(self::never())->method('find'); + $this->documentMapper->expects(self::never())->method('update'); + + $this->documentService->assertNoOutsideConflict($document, $file); + } + + public function testNoAutosavingWhileSaveIsUnderWay(): void { + $document = $this->createDocument('etag1', 1000, 'new content'); + $file = $this->mockFile('etag1', 1000, 'old content'); + $this->cache->method('get') + ->with('document-save-lock-123') + ->willReturn(true); + $this->documentMapper->expects(self::never())->method('update'); + + $result = $this->documentService->autosave($document, $file, 1234, 'new content', 'doc state'); + self::assertSame($result, $document); + } + +}