RTC: add regression tests for data corruption bug due to cursor scope issue#77662
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @danluu! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
9426641 to
cdc44aa
Compare
alecgeatches
left a comment
There was a problem hiding this comment.
Good additional coverage! Merging.
What?
Follow up to #77532, #77658
This is part of an AI fuzzing project which has uncovered a number of real bugs (#77532, and also #77631 independently of @alecgeatches finding the repro for that bug) and has not yet had a false positive (I'm sure there will be some in the future).
The fuzzer found another data corruption bug that's independent from #77532 but is also fixed by #77658. The PR for #77658 was created by a subagent narrowly looking at #77532, so it does not include regression tests for this bug. This PR has two purposes:
Why?
Here's a video of the issue:
core-file-cursor-scope-real-user-annotated.mp4
Codex's description of the bug:
BEGIN AI GENERATED TEXT
The RTC block-merge write path throws away rich-text field scope and keeps only
one bare cursor offset. If a block update changes a rich-text field that is not
the selected field, RTC can reuse the selected field's cursor while diffing the
wrong rich-text value.
The confirmed high-level repro uses the bundled
core/fileblock:Downloadbutton text.Open in new tab.Replace -> Open Media Library.The author sees the new file name correctly as
manual, but the collaboratorsees the corrupted file name
manuaa.This is a real user-visible RTC bug in a bundled core block. It is not just a
synthetic custom-block platform issue.
Root Cause
WPBlockSelectionpreserves field scope:clientIdattributeKeyFor the
core/filerepro, the selected field isdownloadButtonTextand theselection offset is
2.The RTC write path collapses that scoped selection to just a number before
block merging:
applyPostChangesToCRDTDoc()readschanges.selection?.selectionStart?.offset ?? nullmergeCrdtBlocks()receives only that bare numberWhen the later File block replacement changes
fileNamefromgammatomanual, the merge code still has the olddownloadButtonTextcursor offset.It no longer knows that the cursor belongs to
downloadButtonText, so it usesthat offset while diffing
fileName. That corrupts the collaborator'sfileNametomanuaa.The selected field in the repro is plain text, so offset
2is already a validHTML index for
downloadButtonText. The failure is not the offset-space bug.The corruption occurs because a cursor from one field is reused for another
field.
Confirmed User Repro
The confirmed bundled-core browser repro is committed here:
core/filePlaywright reproThe user-visible sequence is:
name is
gammaand whose button text isDownload.Downloadbutton text.2insideDownload.Open in new tab.Replace -> Open Media Library.manual.Expected result on both editors:
Actual result:
The Playwright test also saves the draft after the visible corruption and
checks that the collaborator remains corrupted in that same live session.
Why The Intermediate Update Matters
The one-step file replacement path did not reliably reproduce the bug by
itself. The confirmed repro needs the extra user action while the caret remains
inside
downloadButtonText.That extra action is not synthetic. It is a normal File block control:
Clicking it updates the block's
textLinkTargetwhile the editor selection isstill scoped to
downloadButtonTextat offset2. The following filereplacement changes the other rich-text field,
fileName, while that stalebare offset is still available to RTC. Because RTC has already dropped
attributeKey, it cannot tell that the cursor does not belong tofileName.Pre-Fix Production Path
The confirmed browser path is:
core/fileblock is edited in the browser.downloadButtonText.Open in new tabsends a block update while preserving thatselection.
fileName.editEntityRecord()forwardsblocksplusselection.SyncManager.update()applies the change to the live Yjs document.applyPostChangesToCRDTDoc()dropsattributeKeyand keeps onlyoffset.mergeCrdtBlocks()reuses that offset while mergingfileName.fileNamecontent.Evidence And Regression Coverage
The regression tests cover multiple levels of the production write path. On the
pre-fix implementation, these tests reproduce the corruption; with the scoped
cursor fix, they pass:
mergeCrdtBlocks()regressionapplyPostChangesToCRDTDoc()regressionSyncManager.update()regressioncore/filePlaywright regression with real browser UI interactionsThe lower-level tests use a two-rich-text block to isolate the cursor-scope
failure and include negative controls:
nullcursor hintscross-field cursor reuse
the wrong coordinate space
Those controls are also linked directly:
nullcursor controlThe browser repro then confirms the same class of bug in a bundled core block
with normal user interactions. The author and collaborator diverge only after
the RTC merge path runs.
Re-Running The Repros
Run these commands from the repo root with Node 20 active.
mergeCrdtBlocks()regressionnpm run test:unit -- --runInBand \ packages/core-data/src/utils/test/rtc-rich-text-cursor-scope.test.js \ --testNamePattern="mergeCrdtBlocks"applyPostChangesToCRDTDoc()regressionnpm run test:unit -- --runInBand \ packages/core-data/src/utils/test/rtc-rich-text-cursor-scope.test.js \ --testNamePattern="applyPostChangesToCRDTDoc"SyncManager.update()regressionnpm run test:unit -- --runInBand \ packages/core-data/src/utils/test/rtc-rich-text-cursor-scope.test.js \ --testNamePattern="SyncManager.update"custom-block browser regression with programmatic insertion
npm run wp-env-test start WP_BASE_URL=http://localhost:8889 \ npm run test:e2e -- \ test/e2e/specs/editor/collaboration/collaboration-rich-text-cursor-scope.spec.ts \ --project=chromium \ --grep="keeps collaborator rich text correct$"custom-block browser regression with the real inserter and typing interactions
npm run wp-env-test start WP_BASE_URL=http://localhost:8889 \ npm run test:e2e -- \ test/e2e/specs/editor/collaboration/collaboration-rich-text-cursor-scope.spec.ts \ --project=chromium \ --grep="real inserter and typing interactions"core/filebrowser regression with real browser UI interactionsnpm run wp-env-test start WP_BASE_URL=http://localhost:8889 \ npm run test:e2e -- \ test/e2e/specs/editor/collaboration/collaboration-rich-text-cursor-scope.spec.ts \ --project=chromium \ --grep="core/file"The full committed cursor-scope test suite is:
How The Bug Was Introduced
The current top-level-field bug was introduced on January 14, 2026 by commit
30c040ca841("Real-time collaboration: Use alternative diff in quill-delta,provide incremental text updates") from
PR #73699.
That change:
changes.selection?.selectionStart?.offset ?? nullinto
mergeCrdtBlocks()Later, on April 2, 2026, commit
09a21c64b5b("RTC: Fix core/table cell merging") from
PR #76913 generalized
the same cursor plumbing into schema-aware array and object merges. That did
not create the top-level
core/filebug shown here, but it widened the samescope-loss problem to nested rich-text structures.
Impact Bounds
What is currently proven:
saveDraft()in that same sessionWhat is not currently proven:
core/filecorruption survives a fresh reloadThat narrows severity, but it does not make the bug a false positive. The bug
causes real in-session collaborator-visible corruption.
END AI GENERATED TEXT
Note that the above was with respect to this branch in my fork, not this PR.
Use of AI Tools
As noted above, this is an AI generated PR coming out of an AI fuzzing project. We've had no false positives so far, but of course the false positive rate isn't zero and I expect false positives at some point. In this case, since this is adding regression tests, a false positive would just mean that we're adding some tests for a bug that was somehow not real. While this can happen when a fuzz test uncovers a bug with respect to a function call, this particular bug was reproduced with real user actions inside playwright and should not be a false positive.