Merge pull request #3922 from nextcloud/backport/3908/stable26

[stable26] Avoid cleanup on close to not run into race conditions with other sessions on the same document

Author: juliusknorr

cypress/e2e/SessionApi.spec.js

@@ -350,7 +350,8 @@ describe('The session Api', function() {
 		})
 
 		// Failed with a probability of ~ 50% initially
-		it('ignores steps stored after close cleaned up', function() {
+		// Skipped for now since the behaviour chanced by not cleaning up the state on close/create
+		it.skip('ignores steps stored after close cleaned up', function() {
 			cy.pushAndClose({ connection, steps: [messages.update], version })
 			cy.createTextSession(undefined, { filePath: '', shareToken })
 				.then(con => {

cypress/e2e/sync.spec.js

@@ -95,14 +95,14 @@ describe('Sync', () => {
 			.should('include', 'after the lost connection')
 	})
 
-	it('passes the doc content from one session to the next', () => {
+	it.only('passes the doc content from one session to the next', () => {
 		cy.closeFile()
 		cy.intercept({ method: 'PUT', url: '**/apps/text/session/create' })
 			.as('create')
 		cy.openTestFile()
 		cy.wait('@create', { timeout: 10000 })
 			.its('response.body')
-			.should('have.property', 'content')
+			.should('have.property', 'documentState')
 			.should('not.be.empty')
 		cy.getContent().find('h2').should('contain', 'Hello world')
 		cy.getContent().find('li').should('contain', 'Saving the doc saves the doc state')

lib/Controller/PublicSessionController.php

@@ -66,8 +66,8 @@ protected function isPasswordProtected(): bool {
 	 * @NoAdminRequired
 	 * @PublicPage
 	 */
-	public function create(string $token, string $file = null, $guestName = null, bool $forceRecreate = false): DataResponse {
-		return $this->apiService->create(null, $file, $token, $guestName, $forceRecreate);
+	public function create(string $token, string $file = null, $guestName = null): DataResponse {
+		return $this->apiService->create(null, $file, $token, $guestName);
 	}
 
 	/**

lib/Controller/SessionController.php

@@ -53,8 +53,8 @@ public function __construct(string $appName, IRequest $request, ApiService $apiS
 	/**
 	 * @NoAdminRequired
 	 */
-	public function create(int $fileId = null, string $file = null, bool $forceRecreate = false): DataResponse {
-		return $this->apiService->create($fileId, $file, null, null, $forceRecreate);
+	public function create(int $fileId = null, string $file = null): DataResponse {
+		return $this->apiService->create($fileId, $file, null, null);
 	}
 
 	/**

lib/Cron/Cleanup.php

@@ -28,8 +28,6 @@
 
 namespace OCA\Text\Cron;
 
-use OCA\Text\Db\Session;
-use OCA\Text\Exception\DocumentHasUnsavedChangesException;
 use OCA\Text\Service\DocumentService;
 use OCA\Text\Service\AttachmentService;
 use OCA\Text\Service\SessionService;
@@ -57,29 +55,25 @@ public function __construct(ITimeFactory $time,
 	}
 
 	protected function run($argument) {
-		$this->logger->debug('Run cleanup job for text sessions');
-		$inactive = $this->sessionService->findAllInactive();
-		/** @var Session $session */
-		foreach ($inactive as $session) {
-			$activeSessions = $this->sessionService->getActiveSessions($session->getDocumentId());
-			if (count($activeSessions) > 0) {
+		$this->logger->debug('Run cleanup job for text documents');
+		$documents = $this->documentService->getAll();
+		foreach ($documents as $document) {
+			$allSessions = $this->sessionService->getAllSessions($document->getId());
+			if (count($allSessions) > 0) {
+				// Do not reset if there are any sessions left
+				// Inactive sessions will get removed further down and will trigger a reset next time
 				continue;
 			}
 
-			$this->documentService->unlock($session->getDocumentId());
-
-			try {
-				$this->logger->debug('Resetting document ' . $session->getDocumentId() . '');
-				$this->documentService->resetDocument($session->getDocumentId());
-				$this->attachmentService->cleanupAttachments($session->getDocumentId());
-				$this->logger->debug('Resetting document ' . $session->getDocumentId() . '');
-			} catch (DocumentHasUnsavedChangesException $e) {
-				$this->logger->info('Document ' . $session->getDocumentId() . ' has not been reset, as it has unsaved changes');
-			} catch (\Throwable $e) {
-				$this->logger->error('Document ' . $session->getDocumentId() . ' has not been reset, as an error occured', ['exception' => $e]);
+			if ($this->documentService->hasUnsavedChanges($document)) {
+				continue;
 			}
+
+			$this->documentService->resetDocument($document->getId());
 		}
-		$removedSessions = $this->sessionService->removeInactiveSessions(null);
+
+		$this->logger->debug('Run cleanup job for text sessions');
+		$removedSessions = $this->sessionService->removeInactiveSessionsWithoutSteps(null);
 		$this->logger->debug('Removed ' . $removedSessions . ' inactive sessions');
 	}
 }

lib/Db/DocumentMapper.php

@@ -54,4 +54,13 @@ public function find($documentId): Document {
 		}
 		return Document::fromRow($data);
 	}
+
+	public function findAll(): array {
+		$qb = $this->db->getQueryBuilder();
+		$result = $qb->select('*')
+			->from($this->getTableName())
+			->execute();
+
+		return $this->findEntities($qb);
+	}
 }

lib/Db/SessionMapper.php

@@ -99,7 +99,7 @@ public function findAllInactive() {
 		return $this->findEntities($qb);
 	}
 
-	public function deleteInactive($documentId = -1) {
+	public function deleteInactiveWithoutSteps($documentId = -1) {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('session_id')
 			->from('text_steps');

lib/Service/ApiService.php

@@ -31,7 +31,6 @@
 use OC\Files\Node\File;
 use OCA\Files_Sharing\SharedStorage;
 use OCA\Text\AppInfo\Application;
-use OCA\Text\Exception\DocumentHasUnsavedChangesException;
 use OCA\Text\Exception\DocumentSaveConflictException;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
@@ -72,7 +71,7 @@ public function __construct(IRequest $request,
 		$this->l10n = $l10n;
 	}
 
-	public function create($fileId = null, $filePath = null, $token = null, $guestName = null, bool $forceRecreate = false): DataResponse {
+	public function create($fileId = null, $filePath = null, $token = null, $guestName = null): DataResponse {
 		try {
 			/** @var File $file */
 			if ($token) {
@@ -112,18 +111,16 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
 
 			$readOnly = $this->documentService->isReadOnly($file, $token);
 
-			$this->sessionService->removeInactiveSessions($file->getId());
-			$remainingSessions = $this->sessionService->getAllSessions($file->getId());
-			$freshSession = false;
-			if ($forceRecreate || count($remainingSessions) === 0) {
-				$freshSession = true;
-				try {
-					$this->documentService->resetDocument($file->getId(), $forceRecreate);
-				} catch (DocumentHasUnsavedChangesException $e) {
-				}
-			}
+			$this->sessionService->removeInactiveSessionsWithoutSteps($file->getId());
+			$document = $this->documentService->getDocument($file);
+			$freshSession = $document === null;
 
-			$document = $this->documentService->createDocument($file);
+			if ($freshSession) {
+				$this->logger->info('Create new document of ' . $file->getId());
+				$document = $this->documentService->createDocument($file);
+			} else {
+				$this->logger->info('Keep previous document of ' . $file->getId());
+			}
 		} catch (Exception $e) {
 			$this->logger->error($e->getMessage(), ['exception' => $e]);
 			return new DataResponse('Failed to create the document session', 500);
@@ -132,20 +129,22 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
 		$session = $this->sessionService->initSession($document->getId(), $guestName);
 
 		if ($freshSession) {
-			$this->logger->debug('Starting a fresh session');
+			$this->logger->debug('Starting a fresh editing session for ' . $file->getId());
 			$documentState = null;
 			$content = $this->loadContent($file);
 		} else {
-			$this->logger->debug('Loading existing session');
+			$this->logger->debug('Loading existing session for ' . $file->getId());
 			$content = null;
 			try {
 				$stateFile = $this->documentService->getStateFile($document->getId());
 				$documentState = $stateFile->getContent();
 			} catch (NotFoundException $e) {
+				$this->logger->debug('State file not found for ' . $file->getId());
 				$documentState = ''; // no state saved yet.
 				// If there are no steps yet we might still need the content.
 				$steps = $this->documentService->getSteps($document->getId(), 0);
 				if (empty($steps)) {
+					$this->logger->debug('Empty steps, loading content for ' . $file->getId());
 					$content = $this->loadContent($file);
 				}
 			}
@@ -173,14 +172,10 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
 
 	public function close($documentId, $sessionId, $sessionToken): DataResponse {
 		$this->sessionService->closeSession($documentId, $sessionId, $sessionToken);
-		$this->sessionService->removeInactiveSessions($documentId);
+		$this->sessionService->removeInactiveSessionsWithoutSteps($documentId);
 		$activeSessions = $this->sessionService->getActiveSessions($documentId);
 		if (count($activeSessions) === 0) {
-			try {
-				$this->documentService->resetDocument($documentId);
-				$this->attachmentService->cleanupAttachments($documentId);
-			} catch (DocumentHasUnsavedChangesException $e) {
-			}
+			$this->documentService->unlock($documentId);
 		}
 		return new DataResponse([]);
 	}

lib/Service/DocumentService.php

@@ -112,6 +112,14 @@ public function __construct(DocumentMapper $documentMapper, StepMapper $stepMapp
 		}
 	}
 
+	public function getDocument(File $file): ?Document {
+		try {
+			return $this->documentMapper->find($file->getId());
+		} catch (DoesNotExistException|NotFoundException $e) {
+			return null;
+		}
+	}
+
 	/**
 	 * @param File $file
 	 * @return Entity
@@ -127,7 +135,7 @@ public function createDocument(File $file): Document {
 			// This way the user can still resolve conflicts in the editor view
 			$stepsVersion = $this->stepMapper->getLatestVersion($document->getId());
 			if ($stepsVersion && ($document->getLastSavedVersion() !== $stepsVersion)) {
-				$this->logger->debug('Unsaved steps but collission with file, continue collaborative editing');
+				$this->logger->debug('Unsaved steps, continue collaborative editing');
 				return $document;
 			}
 			return $document;
@@ -258,6 +266,7 @@ private function insertSteps($documentId, $sessionId, $steps, $version): int {
 			}
 			$stepsVersion = $this->stepMapper->getLatestVersion($document->getId());
 			$newVersion = $stepsVersion + count($steps);
+			$this->logger->debug("Adding steps to $documentId: bumping version from $stepsVersion to $newVersion");
 			$this->cache->set('document-version-' . $document->getId(), $newVersion);
 			$step = new Step();
 			$step->setData($stepsJson);
@@ -332,6 +341,22 @@ public function autosave(?File $file, int $documentId, int $version, ?string $au
 			return $document;
 		}
 
+		if (empty($autoaveDocument)) {
+			$this->logger->debug('Saving empty document', [
+				'requestVersion' => $version,
+				'requestAutosaveDocument' => $autoaveDocument,
+				'requestDocumentState' => $documentState,
+				'document' => $document->jsonSerialize(),
+				'fileSizeBeforeSave' => $file->getSize(),
+				'steps' => array_map(function (Step $step) {
+					return $step->jsonSerialize();
+				}, $this->stepMapper->find($documentId, 0)),
+				'sessions' => array_map(function (Session $session) {
+					return $session->jsonSerialize();
+				}, $this->sessionMapper->findAll($documentId))
+			]);
+		}
+
 		$this->cache->set('document-save-lock-' . $documentId, true, 10);
 		try {
 			$this->lockManager->runInScope(new LockContext(
@@ -344,46 +369,49 @@ public function autosave(?File $file, int $documentId, int $version, ?string $au
 					$this->writeDocumentState($file->getId(), $documentState);
 				}
 			});
+			$document->setLastSavedVersion($stepsVersion);
+			$document->setLastSavedVersionTime(time());
+			$document->setLastSavedVersionEtag($file->getEtag());
+			$this->documentMapper->update($document);
 		} catch (LockedException $e) {
 			// Ignore lock since it might occur when multiple people save at the same time
 			return $document;
+		} finally {
+			$this->cache->remove('document-save-lock-' . $documentId);
 		}
-		$document->setLastSavedVersion($stepsVersion);
-		$document->setLastSavedVersionTime(time());
-		$document->setLastSavedVersionEtag($file->getEtag());
-		$this->documentMapper->update($document);
-		$this->cache->remove('document-save-lock-' . $documentId);
 		return $document;
 	}
 
 	/**
-	 * @param $documentId
-	 * @param bool $force
 	 * @throws DocumentHasUnsavedChangesException
+	 * @throws Exception
+	 * @throws NotPermittedException
 	 */
 	public function resetDocument(int $documentId, bool $force = false): void {
 		try {
-			$this->unlock($documentId);
-
 			$document = $this->documentMapper->find($documentId);
-
-			if ($force || !$this->hasUnsavedChanges($document)) {
-				$this->stepMapper->deleteAll($documentId);
-				$this->sessionMapper->deleteByDocumentId($documentId);
-				$this->documentMapper->delete($document);
-
-				try {
-					$this->getStateFile($documentId)->delete();
-				} catch (NotFoundException $e) {
-				} catch (NotPermittedException $e) {
-				}
-			} elseif ($this->hasUnsavedChanges($document)) {
+			if (!$force && $this->hasUnsavedChanges($document)) {
+				$this->logger->debug('did not reset document for ' . $documentId);
 				throw new DocumentHasUnsavedChangesException('Did not reset document, as it has unsaved changes');
 			}
-		} catch (DoesNotExistException $e) {
+
+			$this->unlock($documentId);
+
+			$this->stepMapper->deleteAll($documentId);
+			$this->sessionMapper->deleteByDocumentId($documentId);
+			$this->documentMapper->delete($document);
+
+			$this->getStateFile($documentId)->delete();
+			$this->logger->debug('document reset for ' . $documentId);
+		} catch (DoesNotExistException|NotFoundException $e) {
+			// Ignore if document not found or state file not found
 		}
 	}
 
+	public function getAll() {
+		return $this->documentMapper->findAll();
+	}
+
 	/**
 	 * @param Session $session
 	 * @param $shareToken

lib/Service/SessionService.php

@@ -146,9 +146,9 @@ public function findAllInactive() {
 		return $this->sessionMapper->findAllInactive();
 	}
 
-	public function removeInactiveSessions($documentId = -1) {
+	public function removeInactiveSessionsWithoutSteps($documentId = -1) {
 		// No need to clear the cache here as we already set a TTL
-		return $this->sessionMapper->deleteInactive($documentId);
+		return $this->sessionMapper->deleteInactiveWithoutSteps($documentId);
 	}
 
 	/**