Merge pull request #4938 from nextcloud/fix/4600-prevent-race-condition

fix(sync): prevent race condition by relying on autoincrement
nextcloud · Nov 8, 2023 · c49d129 · c49d129
2 parents 2bad2df + 3a7fdbd
commit c49d129
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 23 deletions.
diff --git a/cypress/e2e/api/SessionApi.spec.js b/cypress/e2e/api/SessionApi.spec.js
@@ -115,7 +115,7 @@ describe('The session Api', function() {
 					const version = 0
 					cy.pushSteps({ connection, steps, version })
 						.its('version')
-						.should('eql', 1)
+						.should('be.at.least', 1)
 					cy.syncSteps(connection)
 						.its('steps[0].data')
 						.should('eql', steps)
@@ -173,7 +173,7 @@ describe('The session Api', function() {
 		it('saves', function() {
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
-				.should('eql', 1)
+				.should('be.at.least', 1)
 			cy.save(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true })
 			cy.downloadFile(filePath)
 				.its('data')
@@ -184,7 +184,7 @@ describe('The session Api', function() {
 			const documentState = 'Base64 encoded string'
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
-				.should('eql', 1)
+				.should('be.at.least', 1)
 			cy.save(connection, {
 				version: 1,
 				autosaveContent: '# Heading 1',
@@ -247,7 +247,7 @@ describe('The session Api', function() {
 		it('saves public', function() {
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
-				.should('eql', 1)
+				.should('be.at.least', 1)
 			cy.save(connection, { version: 1, autosaveContent: '# Heading 1', manualSave: true })
 			cy.login(user)
 			cy.prepareSessionApi()
@@ -260,7 +260,7 @@ describe('The session Api', function() {
 			const documentState = 'Base64 encoded string'
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
-				.should('eql', 1)
+				.should('be.at.least', 1)
 			cy.save(connection, {
 				version: 1,
 				autosaveContent: '# Heading 1',
@@ -331,7 +331,7 @@ describe('The session Api', function() {
 			let joining
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
-				.should('eql', 1)
+				.should('be.at.least', 1)
 			cy.createTextSession(undefined, { filePath: '', shareToken })
 				.then(con => {
 					joining = con
@@ -348,7 +348,7 @@ describe('The session Api', function() {
 			cy.log('Initial user pushes steps')
 			cy.pushSteps({ connection, steps: [messages.update], version })
 				.its('version')
-				.should('eql', 1)
+				.should('be.at.least', 1)
 			cy.log('Other user creates session')
 			cy.createTextSession(undefined, { filePath: '', shareToken })
 				.then(con => {

diff --git a/lib/Db/Step.php b/lib/Db/Step.php
@@ -37,6 +37,14 @@
  * @method setDocumentId(int $documentId): void
  */
 class Step extends Entity implements JsonSerializable {
+
+	/*
+	 * Transition: We now use the auto-incrementing id as the version.
+	 * To ensure that new steps always have a larger version than those that
+	 * used the version field, use the largest possible 32-bit integer value.
+	 */
+	public const VERSION_STORED_IN_ID = 2147483647;
+
 	public $id = null;
 	protected string $data = '';
 	protected int $version = 0;
@@ -55,10 +63,13 @@ public function jsonSerialize(): array {
 		if (\json_last_error() !== JSON_ERROR_NONE) {
 			throw new \InvalidArgumentException('Failed to parse step data');
 		}
+		$version = $this->getVersion() === self::VERSION_STORED_IN_ID
+			? $this->getId()
+			: $this->getVersion();
 		return [
 			'id' => $this->getId(),
 			'data' => $jsonData,
-			'version' => $this->getVersion(),
+			'version' => $version,
 			'sessionId' => $this->getSessionId()
 		];
 	}

diff --git a/lib/Db/StepMapper.php b/lib/Db/StepMapper.php
@@ -36,19 +36,16 @@ public function __construct(IDBConnection $db) {
 	/**
 	 * @return Step[]
 	 */
-	public function find(int $documentId, int $fromVersion, int $lastAckedVersion = null): array {
+	public function find(int $documentId, int $fromVersion): array {
 		/* @var $qb IQueryBuilder */
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
 			->from($this->getTableName())
 			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
-			->andWhere($qb->expr()->gt('version', $qb->createNamedParameter($fromVersion)));
-		if ($lastAckedVersion) {
-			$qb->andWhere($qb->expr()->lte('version', $qb->createNamedParameter($lastAckedVersion)));
-		}
+			->andWhere($qb->expr()->gt('version', $qb->createNamedParameter($fromVersion)))
+			->andWhere($qb->expr()->gt('id', $qb->createNamedParameter($fromVersion)));
 		$qb
 			->setMaxResults(1000)
-			->orderBy('version')
 			->orderBy('id');
 
 		return $this->findEntities($qb);
@@ -57,19 +54,19 @@ public function find(int $documentId, int $fromVersion, int $lastAckedVersion =
 	public function getLatestVersion(int $documentId): ?int {
 		/* @var $qb IQueryBuilder */
 		$qb = $this->db->getQueryBuilder();
-		$result = $qb->select('version')
+		$result = $qb->select('id')
 			->from($this->getTableName())
 			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
 			->setMaxResults(1)
-			->orderBy('version', 'DESC')
+			->orderBy('id', 'DESC')
 			->executeQuery();
 
 		$data = $result->fetch();
 		if ($data === false) {
 			return null;
 		}
 
-		return $data['version'];
+		return $data['id'];
 	}
 
 	public function deleteAll(int $documentId): void {
@@ -79,11 +76,12 @@ public function deleteAll(int $documentId): void {
 			->executeStatement();
 	}
 
+	// not in use right now
 	public function deleteBeforeVersion(int $documentId, int $version): int {
 		$qb = $this->db->getQueryBuilder();
 		return $qb->delete($this->getTableName())
 			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
-			->andWhere($qb->expr()->lte('version', $qb->createNamedParameter($version)))
+			->andWhere($qb->expr()->lte('id', $qb->createNamedParameter($version)))
 			->executeStatement();
 	}
 
@@ -92,6 +90,7 @@ public function deleteAfterVersion(int $documentId, int $version): int {
 		return $qb->delete($this->getTableName())
 			->where($qb->expr()->eq('document_id', $qb->createNamedParameter($documentId)))
 			->andWhere($qb->expr()->gt('version', $qb->createNamedParameter($version)))
+			->andWhere($qb->expr()->gt('id', $qb->createNamedParameter($version)))
 			->executeStatement();
 	}
 }
diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php
@@ -259,15 +259,15 @@ private function insertSteps(Document $document, Session $session, array $steps)
 		try {
 			$stepsJson = json_encode($steps, JSON_THROW_ON_ERROR);
 			$stepsVersion = $this->stepMapper->getLatestVersion($document->getId());
-			$newVersion = $stepsVersion + count($steps);
-			$this->logger->debug("Adding steps to " . $document->getId() . ": bumping version from $stepsVersion to $newVersion");
-			$this->cache->set('document-version-' . $document->getId(), $newVersion);
 			$step = new Step();
 			$step->setData($stepsJson);
 			$step->setSessionId($session->getId());
 			$step->setDocumentId($document->getId());
-			$step->setVersion($newVersion);
-			$this->stepMapper->insert($step);
+			$step->setVersion(Step::VERSION_STORED_IN_ID);
+			$step = $this->stepMapper->insert($step);
+			$newVersion = $step->getId();
+			$this->logger->debug("Adding steps to " . $document->getId() . ": bumping version from $stepsVersion to $newVersion");
+			$this->cache->set('document-version-' . $document->getId(), $newVersion);
 			// TODO write steps to cache for quicker reading
 			return $newVersion;
 		} catch (\Throwable $e) {