[ZEPPELIN-6563] Collaborative mode can silently desynchronize paragraph text, and a later commit overwrites the server copy - #5413
Open
xhaktm00 wants to merge 1 commit into
Open
Conversation
patch_apply never throws: it drops a hunk whose context is gone, and fuzzy matching can place one at the wrong offset while still reporting success, so neither shape is visible to the receiver. A diverged client then commits its whole paragraph and the server stores it unconditionally, making that text authoritative for everyone. Senders now attach checksums of their text before and after the patch. A receiver that started from the same text but did not reach the same result knows it diverged and asks for that single paragraph again via GET_PARAGRAPH, instead of refetching the whole note. Commits carry the checksum of the text the client believed the server held, and the server keeps its own copy when they disagree. Both fields are optional, so clients that omit them behave as before. PatchParagraphSend and PatchParagraphReceived were mapped to the opposite directions, which left the receiving side without noteId or the new fields; they are swapped to match what is actually sent and received.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
In collaborative mode paragraph text is synchronised with diff-match-patch, and both the receiving client (
paragraph-base.ts) and the server (NotebookService.java) use only element[0]ofpatch_apply, discarding the per-hunk success array. A patch that cannot be applied does not throw, and it fails in two different shapes:patch_applyreturns the original text with[false], so the sender's edit silently disappears on that client[true]. If one user changesbravotobravo!while another has deleted that line, the second client ends up withalpha\n!charlie\nand a success flagThe second shape is why inspecting the success array is not enough, and the ticket already ruled out both that approach and
Match_Threshold = 0.A diverged paragraph does not stay local. Focusing and blurring calls
commitParagraph, which sends the client's whole paragraph, and the server stores it throughp.setText(text)without comparing it to what it holds, then broadcasts it to every connection. Whichever client commits first makes its own text authoritative for everyone.This PR keeps the existing patch mechanism and makes divergence detectable instead:
GET_PARAGRAPHmessage. The ticket rejected refetching the note because it recreates every editor while the user is typing, so recovery is per paragraph.broadcastParagraphalready sends a single paragraph, but a client had no way to ask for one; the response reuses the existingOP.PARAGRAPHhandler.Both fields are optional: clients that do not send them behave exactly as before.
Two things I deliberately left out of scope, and one I had to include:
patchApply(...)[0]is unchanged. The server holds a single authoritative copy and applies every patch in order, so it does not diverge the way a client with local edits does, and the commit check backs it up. I can add a warning there in a follow-up if you would prefer it for diagnostics.zeppelin-web/src/app/notebook/paragraph/paragraph.controller.jshas the same line, but with the new UI as default the classic one is served under/classicand has to be opened deliberately. Since the fields are optional it keeps working unchanged, and mixing the two UIs is safe apart from the classic client losing the check. Would you prefer I cover it here, or file a follow-up?PatchParagraphSendandPatchParagraphReceivedwere mapped to the opposite directions inmessage-data-type-map.interface.ts, which left the receiving side typed withoutnoteIdand without the new fields — the change did not compile until this was corrected, so the two are swapped to match what is actually sent and received. It is unrelated to the bug itself; happy to split it out if you would rather review it separately.Finally, this does not make the sync conflict-free. diff-match-patch locates a hunk by matching surrounding context, so a concurrent edit that removes that context can still make a patch land in the wrong place. Editors like Google Docs avoid this class of bug with Operational Transformation or CRDTs, where each edit carries an explicit position the server transforms against concurrent operations. Replacing Zeppelin's sync engine would be far larger than this ticket, so this PR takes the narrower path the ticket points at: make divergence detectable, and stop one client's diverged text from becoming everyone's.
What type of PR is it?
Bug Fix
Todos
GET_PARAGRAPHso a single paragraph can be resentWhat is the Jira issue?
How should this be tested?
New test
NotebookServiceTest#testUpdateParagraphChecksIsBasedOnCurrentServerTextcovers the commit check: a commit based on the current server text is stored, one based on text the server no longer holds is rejected and leaves the server copy untouched, and a commit without a checksum keeps the previous behaviour.Result:
Tests run: 7, Failures: 0, Errors: 0— this run is on top of the merged ZEPPELIN-6556, so the personalized-mode tests and the new check pass together. If a local Zeppelin server is running it holds the Lucene index lock and every test errors out insetUp, so stop it first or pointZEPPELIN_SEARCH_INDEX_PATHat a scratch directory.The checksum uses the same algorithm as
String.hashCode()so both sides compute the same value; I verified Java and TypeScript agree on empty strings, newlines, non-ASCII text, emoji (surrogate pairs) and long input.Manual: open the same note in two browser sessions and edit the same paragraph concurrently, with one session deleting a line while the other edits it. I confirmed the built bundle ships the new fields and that ordinary collaborative sync still works, but I could not reproduce the divergence by hand — on a local server the patch round trip is fast enough that the window never opens. This matches the ticket's note that existing e2e coverage does not exercise failed or misapplied patches, which is why the commit check is covered by unit tests instead.
Rejecting stale commit of paragraphin the server log marks a refused commit.Screenshots (if appropriate)
N/A
Questions: