From 7e47b8216820571638bd12e955987c664c397a44 Mon Sep 17 00:00:00 2001
From: Max <max@nextcloud.com>
Date: Sat, 4 Mar 2023 09:06:26 +0100
Subject: [PATCH] fix: load fresh session if none are remaining

When creating a new session
check for all existing sessions
to take those into account
that are only preserved because of unsaved changes.

It there still aren't any sessions
make sure to load a fresh editing session
even if there are unsaved steps.
These steps most likely originate from a race condition
when closing the last editing session and pushing steps at the same time.

We were seeing empty editing sessions in production
while the markdown files had some content.

This was happening because there were some steps still around
even though all sessions had been closed.

When the last session was closed
steps were pushed at the same time.
The push passed the session check before the session was cleared.
The session data and the steps were cleared
and only then the new step was added.

This left the database in an inconsistent state:
* just a few late steps instead of a full history
* no sessions these steps would belong to

Instead of trying to prevent this race condition
detect this state and recover from it.

Sessions are sometimes aborted without close request in the middle of editing.
In this case all steps are present in the database
and the session also remains.

The session will not be cleaned up during `resetDocument`.
Instead `resetDocument` throws `DocumentHasUnsavedChagesException`.

We can detect this scenario because even `removeInactiveSessions`
also takes remaining steps into account and leaves sessions with steps alone.
Therefore sessions will remain in `apiService->create(...)`
and `freshSession` won't be set.

Steps are only cleaned up when the last active session is closed.
Even if a few steps manage to sneak in after the clean up
the session will be removed
and so `remainingSessions` will be empty during `create`.
Therefore `freshSession` will be set and the content will be send out.
The leftovers won't be cleaned up until the file has been saved again.
But that should be fine. They will not impact the editing.

Signed-off-by: Max <max@nextcloud.com>
---
 cypress/e2e/SessionApi.spec.js | 11 +++++++++++
 cypress/support/sessions.js    | 11 +++++++++++
 lib/Service/ApiService.php     |  6 +++---
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/cypress/e2e/SessionApi.spec.js b/cypress/e2e/SessionApi.spec.js
index 9287360df77..c06c932ecb5 100644
--- a/cypress/e2e/SessionApi.spec.js
+++ b/cypress/e2e/SessionApi.spec.js
@@ -349,5 +349,16 @@ describe('The session Api', function() {
 				})
 		})
 
+		// Failed with a probability of ~ 50% initially
+		it('ignores steps stored after close cleaned up', function() {
+			cy.pushAndClose({ connection, steps: [messages.update], version })
+			cy.createTextSession(undefined, { filePath: '', shareToken })
+				.then(con => {
+					connection = con
+				})
+				.its('state.documentSource')
+				.should('eql', '## Hello world\n')
+		})
+
 	})
 })
diff --git a/cypress/support/sessions.js b/cypress/support/sessions.js
index 3774e384d1d..2f243bb20a7 100644
--- a/cypress/support/sessions.js
+++ b/cypress/support/sessions.js
@@ -51,3 +51,14 @@ Cypress.Commands.add('syncSteps', (connection, options = { version: 0 }) => {
 	return connection.sync(options)
 		.then(response => response.data)
 })
+
+// Used to test for race conditions between the last push and the close request
+Cypress.Commands.add('pushAndClose', ({ connection, steps, version, awareness = '' }) => {
+	cy.log('Race between push and close')
+		.then(() => {
+			const push = connection.push({ steps, version, awareness })
+				.catch(e => e) // handle 403 gracefully
+			const close = connection.close()
+			return Promise.all([push, close])
+		})
+})
diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php
index 176267b10ab..b85c8069d96 100644
--- a/lib/Service/ApiService.php
+++ b/lib/Service/ApiService.php
@@ -113,12 +113,12 @@ public function create($fileId = null, $filePath = null, $token = null, $guestNa
 			$readOnly = $this->documentService->isReadOnly($file, $token);
 
 			$this->sessionService->removeInactiveSessions($file->getId());
-			$activeSessions = $this->sessionService->getActiveSessions($file->getId());
+			$remainingSessions = $this->sessionService->getAllSessions($file->getId());
 			$freshSession = false;
-			if ($forceRecreate || count($activeSessions) === 0) {
+			if ($forceRecreate || count($remainingSessions) === 0) {
+				$freshSession = true;
 				try {
 					$this->documentService->resetDocument($file->getId(), $forceRecreate);
-					$freshSession = true;
 				} catch (DocumentHasUnsavedChangesException $e) {
 				}
 			}