nextcloud / text

📑 Collaborative document editing using Markdown
GNU Affero General Public License v3.0
542 stars 87 forks source link

Sync issues with missing Yjs steps/structs #4600

Closed mejo- closed 6 months ago

mejo- commented 1 year ago

Describe the bug In a test session with four participants we were able to create a situation were changes of two participants were no longer applied to the document of the two others.

Some first insights after analysing the HAR file of three participants

max-nextcloud commented 10 months ago

I think we are missing a mechanism for resyncing clients once they get out of sync. yjs-websocket provides this in https://github.com/yjs/y-websocket/blob/master/src/y-websocket.js#L300-L311 - but that only works with a server that understands the y-js protocol and can answer the sync type 1 messages from the client.

All a shared resync with multiple clients would take would be:

  1. One client (A) sends its full current yjs state incl. history and a matching sync type 1 message.
  2. These are distributed to all clients.
  3. All clients reply to the sync type 1 if they have additional steps
  4. These replies are again distributed to all clients.

At the same time this sync scales fairly well:

  1. grows linear with the compressed document size (some 10k)
  2. grows linear with the number of clients (n) and the compressed document size ( some 10k per client)
  3. will be small as it's only the diff to 1. - so at most the compressed steps since the last resync
  4. grows with n^2 - times the small diff.

For this to scale nicely it's important that only one client performs 1. Otherwise everything will be multiplied by n.

Since we already have an autosave request that is send by one client every 30 seconds (?) I propose the following: a) in addition to the autosave content the saving client sends the content of 1. b) the server adds these two messages to the distributed steps even though one of them is a sync type 1 which we normally drop.

This should be enough to provide the resync mechanism. We could rely on it and simplify other parts of the sync:

c) When autosaving we don't need to upload the doc state anymore as that's encoded in the first message in 1. d) When connecting we don't need to distribute the doc state anymore. e) During new connections / when receiving a sync type 1 message from a client the server can (re)send all messages since the last autosave including the last resync.

mejo- commented 10 months ago

@max-nextcloud: how would 1. be triggered? In other words: when exactly would client (A) send the full current yjs state? When it thinks it's out of sync? Or when it's requested by some other party to do so?

As far as I understand, you would make the client do 1. along with the autosave every 30 seconds. How would you ensure that only one client does so? And how do we decide which client is responsible to do it? What if this client becomes unresponsive?

Apart from these questions, your approach sounds sensible to me.

mejo- commented 10 months ago

This morning, @juliushaertl and me went through the code and identified two more potential causes for problems:

  1. There's a possibility for a race condition in DocumentService->addStep(). First we get $stepsVersion via StepMapper->getLatestVersion(), then we do some more stuff, then we write back the steps via StepMapper->insert(), calculating $newVersion from the formerly retrieved $stepsVersion. If another client writes steps in the same moment, the new calculated steps versions might conflict: https://github.com/nextcloud/text/blob/aad168b24a3218a877c1ab76aae9d8d551627e9b/lib/Service/DocumentService.php#L265-L274 We should change this to be atomic, by calculating the new version within the same query that writes them. E.g. something like:
    INSERT INTO text_steps (version, [...]) SUM(SELECT MAX(version) FROM text_steps + $newStepCount), ...
  2. In DocumentService->addStep() we fetch all steps if there's queries in pushes of type YJS_SYNC_MESSAGE_STEP1: https://github.com/nextcloud/text/blob/aad168b24a3218a877c1ab76aae9d8d551627e9b/lib/Service/DocumentService.php#L232-L234 But this only fetches the first 1000 steps due to setMaxResults() in StepMapper->find(): https://github.com/nextcloud/text/blob/aad168b24a3218a877c1ab76aae9d8d551627e9b/lib/Db/StepMapper.php#L50 This should be changed to return all steps.

We're not sure whether any of these two causes the problems people experience, but they should be fixed nevertheless. @max-nextcloud and @juliushaertl if you agree, I could prepare patches for these two problems.

juliushaertl commented 10 months ago

Regarding the y.js based resyncing mechanism, we were wondering earlier when this would actually be triggered. So far I think that y.js would only send the SyncStep1 on:

Now my main question to consider implementing the above resync handling would be if we can find a resoning on why the existing methods to get steps to the clients could miss steps:

max-nextcloud commented 10 months ago

In DocumentService->addStep() we fetch all steps if there's queries in pushes of type YJS_SYNC_MESSAGE_STEP1:

If i remember correctly we never used them on the cient side. So while i agree that we should fetch all steps for now this should not matter. Plus the whole approach of resending the entire history seems inefficient.

max-nextcloud commented 10 months ago

I could prepare patches for these two problems.

A patch for 1. seems like a good idea to eliminate race conditions. With 2. as i said i'd check if this is actually being used before investing time in something that I hope will be replaced anyway.

juliushaertl commented 10 months ago

I could prepare patches for these two problems.

A patch for 1. seems like a good idea to eliminate race conditions. With 2. as i said i'd check if this is actually being used before investing time in something that I hope will be replaced anyway.

Maybe we can have a quick logging patch to at least see an indication if someone runs into the issue

max-nextcloud commented 10 months ago

As far as I understand, you would make the client do 1. along with the autosave every 30 seconds. How would you ensure that only one client does so? And how do we decide which client is responsible to do it? What if this client becomes unresponsive?

I had another look how autosave actually works. The autosave function inside SyncService is debounced so it's executed at most every 30 seconds. It's triggered from Editor.vue whenever there are local changes (inside on onStateChange).

My understanding now is that every client will trigger the autosave and the server will just ignore them for a while to avoid saving all of the time. But I have not looked into the server side code here yet. This approach seems doable for the resync as well. All clients trigger a resync at most x seconds and the server only distributes one of them every x seconds. There may be better times to trigger a resync than the onStateChange handler though. Ideally it would happen if the client notices y.js updates it cannot apply due to missing steps - however that would require some upstream changes as far as I understand ( https://github.com/yjs/yjs/issues/550 ). Another option might be to trigger the resync whenever a client reconnects. That would cover the laptop reopening pretty well.

max-nextcloud commented 10 months ago

Attempts to reproduce this - ideally in a reliable way

Failed

### Oct 27th - two clients against local dev server I tried to reproduce this problem on a local instance. So far i failed - but also tried with the server and both clients running on the same machine: * turned the network off on one client - everything recovered fine. * stopped one client with a js breakpoint - everything recovered fine. * closed the laptop lid - send it to sleep - everything recovered fine. The laptop sleep obviously affected both clients and the server. Will try with different machines next.
### Oct 28th - three clients against local dev and remote server, 12h sleep Tried some more things to reproduce this. This time with two authenticated users and one guest against the local dev server and cloud.nextcloud.com. Still failing. Either I just did not run into the problem or disconnecting and reconnecting network is not enough and neither is sending the laptop to sleep. I had the laptop sleep over night but the editing session was still working and in sync afterwards.

Other bugs found / reproduced

MrRinkana commented 10 months ago

I can confirm that there seems to be a way for people connecting to get out of sync. I had one document being edited by 5 people and a 6th only got a old version of the document, even if he refreshed the page (text 3.8.0).

Unfortunately I have not found a way to reproduce the issue consistently.

max-nextcloud commented 9 months ago

We have a fix for the one scenario that we could reproduce reliably. It will ship with the next releases - that is 28.0.2, 27.1.6 and 26.0.11.

I'm curious to see if it also fixes the harder to reproduce cases. I'll keep this issue open until we got some feedback.

MrRinkana commented 6 months ago

I tested this a little bit now by creating a .md file and sharing it publicly. I then opened 8 different connections to the file trough the use of Firefox containers (same computer, different sets of cookes etc) and one from my phone (different ip, latencies) of which one was logged in (and owned the file).

I'm on 28.0.2

In summary it seems to work much better!

I did however manage to get the file out of sync, namely I managed to create a table (managed to make it happen twice) that did not appear on all clients, while other changes (after the creation of the table) did. Reloading fixes the sync.

First time only my phone didn't see the table, nor changes to text in it, but could both edit and se other edits. On the clients that saw the table, the edits from the phone appeared normally above or under the table, so the file managed it well! Reloading introduced the table.

Second time I made a new table on one tab (client), added text into one square from another tab, waited a bit then opened the file again from one tab that had closed the file (file owner). The file owner did not see the tab. Three more tabs did not see the table, but the two that edited it and one more did. Reloading the page fixed the issue, and it seemed like the other tabs fixed themselves without needing to reload after that. See picture: Screenshot_20240223_124436-edit

I did not manage to find a consistent trigger. I just played around in the file for about 30 mins, triggering it twice. I suspect a test with multiple users with different latencies/ips would be a good idea, as in my case only the phone had different ip and latency. I unfortunately will not be able to test it in such a way for a while.

No errors where logged (with log level 2) in the Nextcloud log.

mejo- commented 6 months ago

Thanks everyone for your input here and @MrRinkana for trying to further hunt this down. Given the feedback of users who suffered from this issue we expect this to be fixed with most recent Nextcloud 27 and 28 releases.

@MrRinkana could you please try whether you're able to create an out of sync state by adding tables and open a new issue about it if so?