[Bug Report] Server-Side Data Inconsistencies When Rapidly Creating Nodes in ShareDB

[TianBM]

Bug Report

Title

Server-Side Data Inconsistencies When Rapidly Creating Nodes in ShareDB

Description

I'm using ShareDB and sharedb-mongo to build an online mind-map app. I found that when creating nodes in a short time, the server-side accepts operations (ops) with incorrect data. This issue is caused by the client-side pushing and applying ops with the same operation object. If the op-apply method changes the operation object, it will push to the server-side with incorrect data. Modifying the push op method could potentially resolve this issue.

Steps to Reproduce

Steps to reproduce the behavior:

1. Push an insert node operation.
2. Push an insert node operation in the last node's children.
3. Re-push the insert node operation as in step 2.
4. Observe the server-side operations and data.

Expected Behavior

The server-side should accept and apply operations with correct data, even when multiple nodes are created rapidly.

Actual Behavior

The client-side pushes the operation after it is applied, resulting in the operation being changed and causing incorrect data to be sent to the server-side.

Screenshots

Environment

• ShareDB version: 3.3.2
• Node.js version: 18
• OTType: ot-tree

Additional Context

This issue is affecting the reliability of real-time collaborative features in the application. Implementing a solution to handle rapid consecutive operations more effectively on the client-side would be highly beneficial. For example, ensuring unique operation objects for each push operation may resolve this issue.

[alecgibson]

Thanks for the report! I'm not sure I fully understand the issue (I've not used the ot-tree type personally). Could you please provide a minimally-working reproduction, ideally with a failing test case.

[TianBM]

OK，I will provide more details tomorrow

[TianBM]

I posted a minimally-working project in https://github.com/TianBM/mini-ot

[alecgibson]

Hi thanks for this. I see you've included a hand-rolled OT type in here. Is this your own type? Is the bug with the type or with ShareDB?

Also I don't fully understand this type. Could you please write a failing test case, with some assertion on the expected final state.

[TianBM]

I found it on GitHub and noticed that json1 has the same issue. I’m writing a unit test for ShareDB to help identify and fix the problem. I believe that ShareDB should ensure the operation (op) isn't modified, even if some OT types may change the op object.

[alecgibson]

There are some json1 tests here already. Probably a good place to chuck a new test, and should hopefully give you a good starting point for a new test.

[TianBM]

yeah, I noticed that json0 has the clone op operation, which avoids using the same object for the push method and the apply method, so I would suggest that clone could be added before the sharedb's ottype.apply call to ensure that they don't affect each other

[TianBM]

I think this is a bug in ot-tree, but I also hope sharedb will consider my proposal to add the clone method before the ottype.apply call, to avoid the push method and the apply method affecting each other

[alecgibson]

I agree cloning would be safest, but it does come with performance and maintenance penalties.

According to the docs:

apply(snapshot, op) -> snapshot': Apply an operation to a document snapshot. Returns the changed snapshot. For performance, old document must not be used after this function call, so apply may reuse and return the current snapshot object.

...the snapshot actively isn't guaranteed immutable, but no mention is made of the op, which I personally would read as: the type should not mutate the op, and it's a concern of the type to clone the op if it needs to mutate it for internal work.

Further, I think if the type mutates the op, I'm pretty sure it breaks this property of transform:

Transform must conform to Transform Property 1. That is, apply(apply(snapshot, op1), transform(op2, op1, 'left')) == apply(apply(snapshot, op2), transform(op1, op2, 'right'))

If apply() mutates the ops, I'm pretty sure you can't guarantee Transform Property 1, so the type is broken.

Also, there's points in ShareDB where we directly call type.apply(), and I don't think it should be a concern of ShareDB to have to clone before every call to this.

[TianBM]

I agree that ops shouldn't be mutable, but there are certain cases where, for example, the op object is added directly to the snapshot, in which case the ot-type may have all the behaviors met, but is affected by the subsequent apply method because it's not cloned

[alecgibson]

That's true, but I'd still say that would probably fail Transform Property 1 I suspect. If a type is well-behaved, and knows that it is going to do this sort of thing, I'd say the onus is on the type to clone if it thinks it appropriate.

[TianBM]

I closed this issue, but I still think there is a problem: ot-type with all behaviors satisfied may cause wrong data because of the current sharedb writeup, I need some time to check out