Fix sync errors after network issues #4474

mejo- · 2023-07-10T16:00:46Z

📝 Summary

Resolves: Issues when reconnecting #4340
Resolves: Lost content in collaborative session #4414

🚧 TODO

Look into Cypress test
Remove .only from Cypress test
remove disableBc: true from SyncServiceProvider.js

🏁 Checklist

Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
Sign-off message is added to all commits
Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
Documentation is not required

src/services/SyncServiceProvider.js

cypress · 2023-07-10T17:55:07Z

1 failed and 1 flaky tests on run #10957 ↗︎

1	147	2	0	1

Details:

Fix sync errors after network issues
Project: Text	Commit: `ce2b903fac`
Status: Failed	Duration: 03:34 💡
Started: Jul 11, 2023 5:03 PM	Ended: Jul 11, 2023 5:07 PM

cypress/e2e/sync.spec.js • 1 failed test

View Output Video

Test		Artifacts
Sync > recovers from a lost connection		`Output` `Screenshots`

cypress/e2e/directediting.spec.js • 1 flaky test

View Output Video

Test		Artifacts
direct editing > Open an existing plain text file, edit it		`Output` `Screenshots`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

max-nextcloud · 2023-07-11T11:02:50Z

I could not reproduce the cypress failure locally... set a crazy long wait time ™️ for the failed cypress statement to see if it's a matter of speed.

max-nextcloud · 2023-07-11T11:47:00Z

Ahh... now the SyncServiceProvider.spec is failing because the browser broadcast kicks in.

juliushaertl · 2023-07-11T14:47:31Z

Code looks good to me, also did a couple of tests using separate browsers and killing random connections, setting offline/online again, so far no obstacles found :) Very nice.

juliushaertl · 2023-07-11T14:48:28Z

One minor unrelated UI bug, the notice about connection issues seems to no longer be sticky, but unrelated to this change:

mejo- · 2023-07-11T14:58:18Z

/compile

mejo- · 2023-07-11T15:02:02Z

One minor unrelated UI bug, the notice about connection issues seems to no longer be sticky, but unrelated to this change:

Interesting, good catch. Let's open a new issue to track it: #4483

Signed-off-by: Max <max@nextcloud.com>

Also drop the `sending` logic from `WebsocketPolyfill`. It prevented resending the queue after an error. The queue was non empty then so `this.#initiateSending()` was not called. `SyncService` is responsible for throttling the network requests if needed. Signed-off-by: Max <max@nextcloud.com>

Signed-off-by: Jonas <jonas@freesources.org>

Signed-off-by: Max <max@nextcloud.com>

This way the calling function can catch all network errors and make sure to preserve the unsent steps in the queue. Signed-off-by: Max <max@nextcloud.com>

SyncService will resend failed steps after network has recovered. Remove obsolete `data.retry` flag when emitting error of `ERROR_TYPE.CONNECTION_FAILED`. Fixes: #4340 Signed-off-by: Jonas <jonas@freesources.org>

Also only wait for `@alive` once in `syncs even when initial state was present` to fix the test. Signed-off-by: Jonas <jonas@freesources.org>

Signed-off-by: Max <max@nextcloud.com>

This is required for Cypress tests where we want the broadcast channel synchronization to be disabled. If not passed, default to `true` in debug mode and `false` otherwise. Signed-off-by: Jonas <jonas@freesources.org>

Signed-off-by: Jonas <jonas@freesources.org>

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>

mejo- · 2023-07-11T17:08:41Z

/backport 6454257,c012aac34c0a47f9b3fae67878a5244a9ac12638,2a666d1ea5958abb26f99cc39cd9372f5c802bc5,6a4489e8cce0d73bcee115fe472738cef180270a,155bbb5fa5783ef08e3ac8a6d3106c236d5c606e,0329f46f61420d5d4fbcf1c7da02d3cb71712161,58fc762c4da53ec6275513c447275b835371b67b,fa5d293a2df74b4a6189c3c4cd9f3bed41d47981,d196ae0d1fd16eb2a5ba2c08e400c92e4c7adb0f,f0396923a4ed15ec104d7e9533ac3a053cea725b,236b07ef71353083d2bafaac86bbe81973d3116e to stable27

backportbot-nextcloud · 2023-07-11T17:09:48Z

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

mejo- · 2023-07-11T17:31:16Z

Manual backports:

stable27: [stable27] Fix sync errors after network issues #4486
stable26: [stable26] Fix sync errors after network issues #4487

mejo- added the 2. developing label Jul 10, 2023

mejo- added this to the Nextcloud 28 milestone Jul 10, 2023

mejo- requested a review from juliushaertl July 10, 2023 16:00

juliushaertl reviewed Jul 10, 2023

View reviewed changes

src/services/SyncServiceProvider.js Outdated Show resolved Hide resolved

mejo- force-pushed the test/sync-protocol branch from 0226261 to fc8b297 Compare July 11, 2023 06:44

mejo- force-pushed the test/sync-protocol branch 2 times, most recently from 2a49ed9 to 21f6f39 Compare July 11, 2023 11:35

mejo- force-pushed the test/sync-protocol branch 3 times, most recently from 1cda958 to 61524c2 Compare July 11, 2023 13:34

mejo- marked this pull request as ready for review July 11, 2023 13:42

max-nextcloud requested a review from juliushaertl July 11, 2023 13:43

max-nextcloud added bug Something isn't working backport-request bug: regression labels Jul 11, 2023

juliushaertl approved these changes Jul 11, 2023

View reviewed changes

mejo- mentioned this pull request Jul 11, 2023

Document status notice with connection issues is not sticky #4483

Closed

mejo- force-pushed the test/sync-protocol branch from 61524c2 to 92c86de Compare July 11, 2023 15:04

max-nextcloud and others added 5 commits July 11, 2023 19:01

test(sync): yjs sync via sync service

6454257

Signed-off-by: Max <max@nextcloud.com>

wip: test yjs chattiness

c012aac

Signed-off-by: Max <max@nextcloud.com>

test(syncservice): Apply Yjs steps in wrong order

6a4489e

Signed-off-by: Jonas <jonas@freesources.org>

test(sync): improve cypress tests for missing messages

155bbb5

Signed-off-by: Max <max@nextcloud.com>

max-nextcloud and others added 7 commits July 11, 2023 19:01

fix(sync): sending steps with interval in wrapper function

0329f46

This way the calling function can catch all network errors and make sure to preserve the unsent steps in the queue. Signed-off-by: Max <max@nextcloud.com>

fix(editor): Don't reconnect Editor on connection issues

58fc762

SyncService will resend failed steps after network has recovered. Remove obsolete `data.retry` flag when emitting error of `ERROR_TYPE.CONNECTION_FAILED`. Fixes: #4340 Signed-off-by: Jonas <jonas@freesources.org>

test(cypress): Fix code syntax to please eslint

fa5d293

Also only wait for `@alive` once in `syncs even when initial state was present` to fix the test. Signed-off-by: Jonas <jonas@freesources.org>

fix(test): wait longer for file could not be found message

d196ae0

Signed-off-by: Max <max@nextcloud.com>

fix(sync): Allow to pass disableBc to createSyncServiceProvider()

f039692

This is required for Cypress tests where we want the broadcast channel synchronization to be disabled. If not passed, default to `true` in debug mode and `false` otherwise. Signed-off-by: Jonas <jonas@freesources.org>

test(sync): cleanup Cypress tests

236b07e

Signed-off-by: Jonas <jonas@freesources.org>

chore(assets): Recompile assets

ce2b903

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>

mejo- force-pushed the test/sync-protocol branch from efe68c0 to ce2b903 Compare July 11, 2023 17:01

mejo- merged commit c81b0cd into main Jul 11, 2023
28 of 30 checks passed

delete-merged-branch bot deleted the test/sync-protocol branch July 11, 2023 17:08

This was referenced Jul 11, 2023

[stable27] Fix sync errors after network issues #4486

Merged

[stable26] Fix sync errors after network issues #4487

Merged

mejo- removed the backport-request label Jul 11, 2023

mejo- mentioned this pull request Jul 12, 2023

Lost edit after going offline and coming back online #1667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sync errors after network issues #4474

Fix sync errors after network issues #4474

mejo- commented Jul 10, 2023 •

edited

Loading

cypress bot commented Jul 10, 2023 •

edited

Loading

max-nextcloud commented Jul 11, 2023

max-nextcloud commented Jul 11, 2023

juliushaertl commented Jul 11, 2023

juliushaertl commented Jul 11, 2023

mejo- commented Jul 11, 2023

mejo- commented Jul 11, 2023

mejo- commented Jul 11, 2023

backportbot-nextcloud bot commented Jul 11, 2023

mejo- commented Jul 11, 2023

Fix sync errors after network issues #4474

Fix sync errors after network issues #4474

Conversation

mejo- commented Jul 10, 2023 • edited Loading

📝 Summary

🚧 TODO

🏁 Checklist

cypress bot commented Jul 10, 2023 • edited Loading

1 failed and 1 flaky tests on run #10957 ↗︎

max-nextcloud commented Jul 11, 2023

max-nextcloud commented Jul 11, 2023

juliushaertl commented Jul 11, 2023

juliushaertl commented Jul 11, 2023

mejo- commented Jul 11, 2023

mejo- commented Jul 11, 2023

mejo- commented Jul 11, 2023

backportbot-nextcloud bot commented Jul 11, 2023

mejo- commented Jul 11, 2023

mejo- commented Jul 10, 2023 •

edited

Loading

cypress bot commented Jul 10, 2023 •

edited

Loading