Skip to content

Conversation

dmsnell
Copy link
Contributor

@dmsnell dmsnell commented Apr 22, 2020

Co-authored by @beaucollins

This fix finds itself in the midst of a struggle to find the root cause
of the so-called "ghost-writing" or "infinite-duplication" bug. Under
certain relatively-rare and hard-to-reproduce circumstances we find a
given change reapplied as quickly as the library can communicate with
the server. This tends to destroy entities when transforming the change
against itself.

In this patch we're applying two techniques to prevent cycles in our
change update flow: stop applying any patch whose end version is below
our ghost's version; and make sure we ackwnoledge a queued change if
the server sends it back as a duplicate change.. If we have a newer
ghost than the change then it implies that the change has already been
applied. Likewise, if we submit a patch the server has already received
then we should flush any local buffers holding on to it.

These two changes will prevent a subset of possible ways that we can
trigger ghost-writing but it's unclear how significant that subset is
relative to all cases. We know that in some cases the local client
library continues to send changes to the server without updating its
local sv and it does not appear like this change will address those
cases.

Co-authored by @beaucollins

This fix finds itself in the midst of a struggle to find the root cause
of the so-called "ghost-writing" or "infinite-duplication" bug. Under
certain relatively-rare and hard-to-reproduce circumstances we find a
given change reapplied as quickly as the library can communicate with
the server. This tends to destroy entities when transforming the change
against itself.

In this patch we're applying two techniques to prevent cycles in our
change update flow: stop applying any patch whose end version is below
our ghost's version; and make sure we ackwnoledge a queued change if
the server sends it back as a duplicate change.. If we have a newer
ghost than the change then it implies that the change has already been
applied. Likewise, if we submit a patch the server has already received
then we should flush any local buffers holding on to it.

These two changes will prevent a subset of possible ways that we can
trigger ghost-writing but it's unclear how significant that subset is
relative to all cases. We know that in some cases the local client
library continues to send changes to the server without updating its
local `sv` and it does not appear like this change will address those
cases.
Copy link

@codebykat codebykat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks reasonable but I'm not sure how to test it.

@dmsnell
Copy link
Contributor Author

dmsnell commented Apr 23, 2020

not sure how to test it.

So far it's just the test suite that tests it. At least with this change there's very little chance I think that any changes would negatively impact normal conditions.

We're looking for messages from the server that rarely happen so I don't know a good way to test other than via the synthetic construction in crossed_wires_test.js

@jleandroperez
Copy link

@dmsnell potentially a silly question, but... here I go anyways!

One thing I'm thinking about is that the seen changes collection is runtime only, whereas the storage (should be) persistent, right?

If that's the case, relaunching the browser should render the seenChanges failsafe 100% inoperable, right?

@dmsnell
Copy link
Contributor Author

dmsnell commented Apr 24, 2020

If that's the case, relaunching the browser should render the seenChanges failsafe 100% inoperable, right?

@jleandroperez that's right but a refresh should clear out the situation that produced the defect - a 409 Duplicate Change request matching a patch in the local queue. For other situations if (ghost.version >= patch.ev) should take care of it.

@dmsnell dmsnell merged commit 986ce2a into master Apr 24, 2020
@dmsnell dmsnell deleted the fix/stop-reapplying-applied-changes branch April 24, 2020 22:24
sandymcfadden added a commit that referenced this pull request Aug 31, 2021
Reverts the changes in PR #101 which was causing some syncing issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants