From 94d23876d1f585d89653b884f5a26b265020f1a2 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 1 Sep 2026 09:08:06 +0200 Subject: [PATCH 1/3] 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 8bafbc93b74dfc6b5e090aa848fda5124dc5c190 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 1 Sep 2026 19:01:04 +0200 Subject: [PATCH 2/3] 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 a79b84d692fc07c212ae19391f7fd89442e032c5 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 2 Sep 2026 04:27:43 +0200 Subject: [PATCH 3/3] fix(save): autosave only every ten seconds 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); + } + +}