Skip to content

[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
apache:masterfrom
xhaktm00:ZEPPELIN-6563
Open

[ZEPPELIN-6563] Collaborative mode can silently desynchronize paragraph text, and a later commit overwrites the server copy#5413
xhaktm00 wants to merge 1 commit into
apache:masterfrom
xhaktm00:ZEPPELIN-6563

Conversation

@xhaktm00

@xhaktm00 xhaktm00 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

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] of patch_apply, discarding the per-hunk success array. A patch that cannot be applied does not throw, and it fails in two different shapes:

  • dropped — with no matching context, patch_apply returns the original text with [false], so the sender's edit silently disappears on that client
  • misapplied — fuzzy matching can place a hunk at the wrong offset and still report [true]. If one user changes bravo to bravo! while another has deleted that line, the second client ends up with alpha\n!charlie\n and a success flag

The 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 through p.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:

  • Detection — senders 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. When the base checksums differ the receiver is simply editing concurrently, so no verdict is made and the current behaviour is kept; this is what keeps ordinary concurrent editing from being flagged. Sending the text itself would defeat the purpose of patching — the two checksums add about 37 bytes to a ~160 byte patch message.
  • Recovery — a diverged receiver asks for that one paragraph again through a new GET_PARAGRAPH message. The ticket rejected refetching the note because it recreates every editor while the user is typing, so recovery is per paragraph. broadcastParagraph already sends a single paragraph, but a client had no way to ask for one; the response reuses the existing OP.PARAGRAPH handler.
  • Containment — a commit carries the checksum of the text the client believed the server held. When it does not match, the server keeps its own copy and sends it back rather than storing text that may be diverged.

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:

  • The server-side 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.
  • The classic UI is unchanged. zeppelin-web/src/app/notebook/paragraph/paragraph.controller.js has the same line, but with the new UI as default the classic one is served under /classic and 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?
  • PatchParagraphSend and PatchParagraphReceived were mapped to the opposite directions in message-data-type-map.interface.ts, which left the receiving side typed without noteId and 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

  • Send before/after checksums with each patch
  • Verify the patched result on the receiver and request the paragraph again when it diverged
  • Add GET_PARAGRAPH so a single paragraph can be resent
  • Reject a commit whose base no longer matches the server text
  • Add unit tests for the commit check

What is the Jira issue?

How should this be tested?

New test NotebookServiceTest#testUpdateParagraphChecksIsBasedOnCurrentServerText covers 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.

./mvnw -pl zeppelin-server test -Dtest=NotebookServiceTest -DfailIfNoTests=false

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 in setUp, so stop it first or point ZEPPELIN_SEARCH_INDEX_PATH at 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 paragraph in the server log marks a refused commit.

Screenshots (if appropriate)

N/A

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No — the new fields are optional, and the new op is only sent after a detected divergence
  • Does this needs documentation? No

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.
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.

1 participant