From ea208ee0d69f08241f7525352e9affd060ff34e9 Mon Sep 17 00:00:00 2001 From: Max Date: Mon, 30 Oct 2023 20:31:10 +0100 Subject: [PATCH 1/2] fix(sync): prevent race condition by relying on autoincrement Prevent a possible race condition when two clients add steps at the same time. See #4600. Rely on the autoincrementing id in order to provide a canonical order that steps can be retrieved in. When two clients push steps at the same time the entries receive destinct ids that increment. So if another client fetches steps in between it will see the smaller id as the version of the fetched step and fetch the other step later on. Transition: In the future we can drop the version column entirely but currently there are still steps stored in the database that make use of the old column. So we need to transition away from that. In order to find entries that are newer than version x we select those that have both a version and an id larger than x. Entries of the new format are newer than any entry of the old format. So we set their version to the largest possible value. This way they will always fulfill the version condition and the condition on the id is more strict and therefore effective. For the old format the version will be smaller than the id as it's incremented per document while the id is unique accross documents. Therefore the version condition is the more strict one and effective. The only scenario where the version might be larger than the id would be if there's very few documents in the database and they have had a lot of steps stored in single database entries. Signed-off-by: Max Signed-off-by: Jonas --- cypress/e2e/api/SessionApi.spec.js | 14 +++++++------- lib/Db/Step.php | 15 +++++++++++++-- lib/Db/StepMapper.php | 19 +++++++++---------- lib/Service/DocumentService.php | 9 ++++----- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/cypress/e2e/api/SessionApi.spec.js b/cypress/e2e/api/SessionApi.spec.js index 36d8f94e1e5..82b7879f398 100644 --- 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 index b2c1ff1aa63..9558178c033 100644 --- a/lib/Db/Step.php +++ b/lib/Db/Step.php @@ -37,9 +37,17 @@ * @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 value for BIGINT. + */ + public const VERSION_STORED_IN_ID = 18446744073709551615; + public $id = null; protected string $data = ''; - protected int $version = 0; + protected int $version = self::VERSION_STORED_IN_ID; protected int $sessionId = 0; protected int $documentId = 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 index 11b7d59f894..8279f7d044b 100644 --- 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,11 +54,11 @@ 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(); @@ -69,7 +66,7 @@ public function getLatestVersion(int $documentId): ?int { 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 index 0d74c898010..7cf14929f48 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -259,15 +259,14 @@ 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 = $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) { From 308d64069f8903a113985d7d71d027b127a3b0ce Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 31 Oct 2023 12:32:47 +0100 Subject: [PATCH 2/2] fix(Step): Use largest possible 32-bit int value for transition The value used before (largest possible MySQL BIGINT value) was too big for PHP int. Since we still support 32-bit platforms on Nextcloud, let's stick to the largest possible 32-bit PHP integer value. Besides, setting the value as default for `Step::version` doesn't work as `QBMapper->insert()` doesn't recognize the `version` field as changed in that case. So let's default to `0` again and set it using `Step->setVersion()` later. Signed-off-by: Jonas --- lib/Db/Step.php | 6 +++--- lib/Service/DocumentService.php | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/Db/Step.php b/lib/Db/Step.php index 9558178c033..52377fcd144 100644 --- a/lib/Db/Step.php +++ b/lib/Db/Step.php @@ -41,13 +41,13 @@ 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 value for BIGINT. + * used the version field, use the largest possible 32-bit integer value. */ - public const VERSION_STORED_IN_ID = 18446744073709551615; + public const VERSION_STORED_IN_ID = 2147483647; public $id = null; protected string $data = ''; - protected int $version = self::VERSION_STORED_IN_ID; + protected int $version = 0; protected int $sessionId = 0; protected int $documentId = 0; diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 7cf14929f48..2998112e86c 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -263,6 +263,7 @@ private function insertSteps(Document $document, Session $session, array $steps) $step->setData($stepsJson); $step->setSessionId($session->getId()); $step->setDocumentId($document->getId()); + $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");