fix(sync): push local changes on reconnect #5279

max-nextcloud · 2024-01-17T19:46:40Z

When an editing session is interrupted
steps that were already pushed to the server
may be cleared alongside the sync service session.

Make sure to push all local state
that is not part of the document state
when (re-)connecting.

Todo

add y-protocols to package.json
keep .yjs file on server side even during session cleanup
try it out
Add some actual e2e tests

🏁 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

When an editing session is interrupted steps that were already pushed to the server may be cleared alongside the sync service session. Make sure to push all local state that is not part of the document state when (re-)connecting. Tests Yjs relies on a browser environment. Therefore we test it in a cypress test. Move these tests into component tests to separate them from 'normal' e2e tests. Signed-off-by: Max <max@nextcloud.com>

Even if all sessions have expired and been removed there may still be disconnected clients that hold state from the last editing session. When they reconnect they will send their yjs updates based on the old state they had. Preserve the yjs state accross editing sessions so updates send after a reconnect can still be processed. Signed-off-by: Max <max@nextcloud.com>

max-nextcloud · 2024-01-18T06:56:47Z

This will break how we currently implemented occ test:reset without the --force option:

the occ job will remove the last steps from the database and clear all sessions
the clients with the local changes will reconnect
they will fetch the .yjs file, compute their local update and redistribute it.

We could clear the .yjs file in the cleanup job. We would need to detect this and reload the editor when there is no yjs file during a reconnect.

cypress.config.js

juliusknorr · 2024-01-18T07:22:01Z

This will break how we currently implemented occ test:reset without the --force option

That seems reasonable. For a manual test it would be good to try this before/after:

Write a document
Set the browser to offline
Reset the document with occ
Join in another browser and do some edits
Reconnect the offline browser

src/components/Editor.vue

src/helpers/yjs.js

juliusknorr · 2024-01-18T07:26:51Z

src/components/Editor.vue

@@ -487,6 +487,11 @@ export default {
 		onLoaded({ documentSource, documentState }) {
 			if (documentState) {
 				applyDocumentState(this.$ydoc, documentState, this.$providers[0])
+				// distribute additional state that may exist locally


So we always distribute the local (possibly outdated) state? Can we ensure/proof somehow that this does not lead to unexpected reverting of newer changes from the server?

This state is yjs document state. So basically what we are distributing is our local knowledge of the history of the document. yjs will handle sorting those edits and turning them into a current state. In the end it will produce a consistent view for everyone - as long as all are on the same page. We need to share all local history so our future edits will be applied.

Let's say we worked together on a text. I got disconnected but made some changes before the editor noticed. Now I'm reconnecting.

If I distribute my local changes yjs will put all changes in an order and create a current content that may contain my or your changes depending on the order picked by yjs. There's no way of knowing what would be the correct order and we would have to live with that. At least we have a consistent view on the content.

If I keep connect without distributing my local changes I will receive your changes - the doc would probably look the same as above for me. Your view would stay the same as it was when i was disconnected and changes i make later on will not show for you.

If I drop all local changes and basically reload the editor we'd also be on the same page and my changes would be lost.

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

mejo-

Thanks a lot for looking into this ❤️

I tried to follow the logic of the code and your changes seem reasonable to me - also taking into account yours and Julius' comments. I didn't test myself and would give yours and Julius' assessment more weight than mine since I struggle to keep the big picture about yjs and document state in my head 🤯

max-nextcloud · 2024-01-18T11:10:24Z

That seems reasonable. For a manual test it would be good to try this before/after:

Before

Somewhat mixed results:

occ reset while editor is connected - editor turned empty - not even a menu bar
second attempt - editor showed reconnect button. continued editing on reconnect, after closing an opening doc was back to saved state
third attempt - disconnected editor right after occ run, showed reconnect button, connected other editor, editing based on last changes, reconnected first editor - reconnected editor can see changes from the other but not the other way round.

None of this seems so good that we would want to keep it ;).

After

~~Seeing lot's of 422 responses... Turns out I am mixing base64 encoded steps with Uint8Arrays in the queue. Will fix.~~ ✔️ fixed.
now when i run occ test:reset I can still reconnect with the previously connected client whose steps were removed
if I run occ test:reset --full I can reconnect and the document only shows the content of the last save.

So this seems fine.

Remove duplicate encoding for updateMessage Signed-off-by: Max <max@nextcloud.com>

juliusknorr · 2024-01-19T07:55:19Z

Sorry, I've had probably some confusion when testing yesterday. Did a reset now (probably with a bit more concentration) and it seems to work well. I couldn't see any trouble with changes being reverted.

juliusknorr · 2024-01-19T08:04:06Z

As the e2e test checkbox is still unchecked, any idea how you could test that? But would not consider that a blocker for the merge

juliusknorr · 2024-01-19T17:32:10Z

/backport to stable28

juliusknorr · 2024-01-19T17:32:21Z

/backport to stable27

juliusknorr · 2024-01-19T17:32:34Z

/backport to stable26

max-nextcloud force-pushed the fix/yjs-history branch 3 times, most recently from 0c782e2 to ad3aa52 Compare January 18, 2024 06:46

max-nextcloud added 2 commits January 18, 2024 07:52

max-nextcloud force-pushed the fix/yjs-history branch from ad3aa52 to fa145e8 Compare January 18, 2024 06:52

juliusknorr reviewed Jan 18, 2024

View reviewed changes

cypress.config.js Show resolved Hide resolved

juliusknorr added 2. developing bug Something isn't working labels Jan 18, 2024

juliusknorr added this to the Nextcloud 29 milestone Jan 18, 2024

juliusknorr reviewed Jan 18, 2024

View reviewed changes

src/components/Editor.vue Show resolved Hide resolved

juliusknorr reviewed Jan 18, 2024

View reviewed changes

src/helpers/yjs.js Show resolved Hide resolved

juliusknorr reviewed Jan 18, 2024

View reviewed changes

max-nextcloud force-pushed the fix/yjs-history branch 4 times, most recently from a80a79f to bf5d7f8 Compare January 18, 2024 09:09

test(cy): components in CI

a643da0

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

max-nextcloud force-pushed the fix/yjs-history branch from bf5d7f8 to a643da0 Compare January 18, 2024 09:27

max-nextcloud added 2 commits January 18, 2024 10:34

enh(log): debug log when sending updateMessage

31d0592

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

docs(js): add return type annotations

a7b6175

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

mejo- reviewed Jan 18, 2024

View reviewed changes

fix(sync): yjs messages are Uint8Arrays in the queue

89493af

Remove duplicate encoding for updateMessage Signed-off-by: Max <max@nextcloud.com>

max-nextcloud requested a review from juliusknorr January 18, 2024 11:35

juliusknorr approved these changes Jan 19, 2024

View reviewed changes

juliusknorr merged commit dd176f0 into main Jan 19, 2024
57 checks passed

juliusknorr deleted the fix/yjs-history branch January 19, 2024 17:31

backportbot bot added the backport-request label Jan 19, 2024

backportbot bot mentioned this pull request Jan 19, 2024

[stable28] fix(sync): push local changes on reconnect #5284

Merged

backportbot bot removed the backport-request label Jan 19, 2024

This was referenced Jan 19, 2024

[stable27] fix(sync): push local changes on reconnect #5285

Merged

[stable26] fix(sync): push local changes on reconnect #5286

Merged

blizzz mentioned this pull request Mar 5, 2024

29.0.0 beta 1 nextcloud/server#43991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sync): push local changes on reconnect #5279

fix(sync): push local changes on reconnect #5279

max-nextcloud commented Jan 17, 2024 •

edited

Loading

max-nextcloud commented Jan 18, 2024

juliusknorr commented Jan 18, 2024

juliusknorr Jan 18, 2024 •

edited

Loading

max-nextcloud Jan 18, 2024

mejo- left a comment

max-nextcloud commented Jan 18, 2024 •

edited

Loading

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

fix(sync): push local changes on reconnect #5279

fix(sync): push local changes on reconnect #5279

Conversation

max-nextcloud commented Jan 17, 2024 • edited Loading

Todo

🏁 Checklist

max-nextcloud commented Jan 18, 2024

juliusknorr commented Jan 18, 2024

juliusknorr Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

max-nextcloud Jan 18, 2024

Choose a reason for hiding this comment

mejo- left a comment

Choose a reason for hiding this comment

max-nextcloud commented Jan 18, 2024 • edited Loading

Before

After

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

juliusknorr commented Jan 19, 2024

max-nextcloud commented Jan 17, 2024 •

edited

Loading

juliusknorr Jan 18, 2024 •

edited

Loading

max-nextcloud commented Jan 18, 2024 •

edited

Loading