Merge pull request #4474 from nextcloud/test/sync-protocol

Fix sync errors after network issues

cypress/e2e/api/SyncServiceProvider.spec.js

@@ -0,0 +1,165 @@
+/*
+ * @copyright Copyright (c) 2023 Max <[email protected]>
+ *
+ * @author Max <[email protected]>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+import { randUser } from '../../utils/index.js'
+import { SyncService } from '../../../src/services/SyncService.js'
+import createSyncServiceProvider from '../../../src/services/SyncServiceProvider.js'
+import { Doc } from 'yjs'
+
+const user = randUser()
+
+describe('Sync service provider', function() {
+	let fileId
+
+	before(function() {
+		cy.createUser(user)
+		window.OC = {
+			config: { modRewriteWorking: false },
+			webroot: '',
+		}
+	})
+
+	beforeEach(function() {
+		cy.login(user)
+		cy.prepareSessionApi()
+		cy.uploadTestFile('test.md')
+			.then(id => {
+				fileId = id
+			})
+		cy.wrap(new Doc()).as('source')
+		cy.wrap(new Doc()).as('target')
+		cy.get('@source').then(source => createProvider(source)).as('sourceProvider')
+		cy.get('@target').then(target => createProvider(target)).as('targetProvider')
+	})
+
+	afterEach(function() {
+		this.sourceProvider?.destroy()
+		this.targetProvider?.destroy()
+	})
+
+	/**
+	 * @param {object} ydoc Yjs document
+	 */
+	function createProvider(ydoc) {
+		const syncService = new SyncService({
+			serialize: () => 'Serialized',
+			getDocumentState: () => null,
+		})
+		syncService.on('opened', () => syncService.startSync())
+		return createSyncServiceProvider({
+			ydoc,
+			syncService,
+			fileId,
+			initialSession: null,
+			disableBc: true,
+		})
+	}
+
+	it('recovers from a dropped message', function() {
+		const sourceMap = this.source.getMap()
+		const targetMap = this.target.getMap()
+		cy.intercept({ method: 'POST', url: '**/apps/text/session/push' })
+			.as('push')
+		cy.intercept({ method: 'POST', url: '**/apps/text/session/sync' })
+			.as('sync')
+		cy.wait('@push')
+		cy.then(() => {
+			sourceMap.set('keyA', 'valueA')
+			expect(targetMap.get('keyB')).to.be.eq(undefined)
+		})
+		cy.wait('@sync')
+		cy.wait('@sync')
+		// eslint-disable-next-line cypress/no-unnecessary-waiting
+		cy.wait(1000)
+		cy.then(() => {
+			expect(targetMap.get('keyA')).to.be.eq('valueA')
+		})
+		cy.intercept({
+			method: 'POST',
+			url: '**/apps/text/session/push',
+		}, req => {
+			if (req.body.steps) {
+				req.reply({ forceNetworkError: true })
+				req.alias = 'dead'
+			} else {
+				req.continue()
+			}
+		})
+		cy.then(() => {
+			sourceMap.set('keyB', 'valueB')
+			expect(targetMap.get('keyB')).to.be.eq(undefined)
+		})
+		cy.wait('@dead')
+		cy.then(() => {
+			expect(targetMap.get('keyB')).to.be.eq(undefined)
+		})
+		cy.intercept({
+			method: 'POST',
+			url: '**/apps/text/session/push',
+		}, req => {
+			if (req.body.steps) {
+				req.alias = 'alive'
+				req.continue()
+			} else {
+				req.continue()
+			}
+		})
+		cy.then(() => {
+			sourceMap.set('keyC', 'valueC')
+			expect(targetMap.get('keyB')).to.be.eq(undefined)
+		})
+		cy.wait('@alive')
+		// eslint-disable-next-line cypress/no-unnecessary-waiting
+		cy.wait(1000)
+		cy.then(() => {
+			expect(targetMap.get('keyC')).to.be.eq('valueC')
+			expect(targetMap.get('keyB')).to.be.eq('valueB')
+		})
+	})
+
+	/*
+	 * Counts the amount of push and sync requests in one minute.
+	 * Skipped per default, useful for comparison before/after changes to SyncProvider or PollingBackend.
+	 */
+	it.skip('is not too chatty', function() {
+		const sourceMap = this.source.getMap()
+		const targetMap = this.target.getMap()
+		cy.intercept({ method: 'POST', url: '**/apps/text/session/push' })
+			.as('push')
+		cy.intercept({ method: 'POST', url: '**/apps/text/session/sync' })
+			.as('sync')
+		cy.wait('@push')
+		cy.then(() => {
+			sourceMap.set('keyA', 'valueA')
+			expect(targetMap.get('keyB')).to.be.eq(undefined)
+		})
+		// eslint-disable-next-line cypress/no-unnecessary-waiting
+		cy.wait(60000)
+		cy.then(() => {
+			expect(targetMap.get('keyA')).to.be.eq('valueA')
+		})
+		// 2 clients push awareness updates every 15 seconds -> 2*5 = 10. Actual 15.
+		cy.get('@push.all').its('length').should('be.lessThan', 30)
+		// 2 clients sync fast first and then every 5 seconds -> 2*12 = 24. Actual 32.
+		cy.get('@sync.all').its('length').should('be.lessThan', 60)
+	})
+})

cypress/e2e/api/Yjs.spec.js

@@ -0,0 +1,61 @@
+/*
+ * @copyright Copyright (c) 2023 Jonas <[email protected]>
+ *
+ * @author Jonas <[email protected]>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+import { applyUpdate, Doc, encodeStateAsUpdate, encodeStateVector } from 'yjs'
+
+describe('Yjs', function() {
+	// Only tests that Yjs allows to apply steps in wrong order
+	it('applies step in wrong order', function() {
+		const source = new Doc()
+		const target = new Doc()
+		const sourceMap = source.getMap()
+		const targetMap = target.getMap()
+
+		target.on('afterTransaction', (tr, doc) => {
+			// console.log('afterTransaction', tr)
+		})
+
+		// Add keyA to source and apply to target
+		sourceMap.set('keyA', 'valueA')
+		const update0A = encodeStateAsUpdate(source)
+		const sourceVectorA = encodeStateVector(source)
+		applyUpdate(target, update0A)
+		expect(targetMap.get('keyA')).to.be.eq('valueA')
+
+		// Add keyB to source, don't apply to target yet
+		sourceMap.set('keyB', 'valueB')
+		const updateAB = encodeStateAsUpdate(source, sourceVectorA)
+		const sourceVectorB = encodeStateVector(source)
+
+		// Add keyC to source, apply to target
+		sourceMap.set('keyC', 'valueC')
+		const updateBC = encodeStateAsUpdate(source, sourceVectorB)
+		applyUpdate(target, updateBC)
+		expect(targetMap.get('keyB')).to.be.eq(undefined)
+		expect(targetMap.get('keyC')).to.be.eq(undefined)
+
+		// Apply keyB to target
+		applyUpdate(target, updateAB)
+		expect(targetMap.get('keyB')).to.be.eq('valueB')
+		expect(targetMap.get('keyC')).to.be.eq('valueC')
+	})
+})

cypress/e2e/sync.spec.js

@@ -77,7 +77,7 @@ describe('Sync', () => {
 			}
 		}).as('sessionRequests')
 		cy.wait('@dead', { timeout: 30000 })
-		cy.get('#editor-container .document-status', { timeout: 10000 })
+		cy.get('#editor-container .document-status', { timeout: 30000 })
 			.should('contain', 'File could not be loaded')
 			.then(() => {
 				count = 4

src/components/Editor.vue

@@ -573,11 +573,7 @@ export default {
 			}
 			if (type === ERROR_TYPE.CONNECTION_FAILED && !this.hasConnectionIssue) {
 				this.hasConnectionIssue = true
-				// FIXME: ideally we just try to reconnect in the service, so we don't loose steps
-				OC.Notification.showTemporary('Connection failed, reconnecting')
-				if (data.retry !== false) {
-					setTimeout(this.reconnect.bind(this), 5000)
-				}
+				OC.Notification.showTemporary('Connection failed.')
 			}
 			if (type === ERROR_TYPE.SOURCE_NOT_FOUND) {
 				this.hasConnectionIssue = true