RTC: Fix divergence in duplicate table rows when one collaborator edits one duplicate while another collaborator deletes the other duplicate#77723
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. |
What?
This is part of a series of issues and PRs created by a coding agent looking at the output of an AI generated fuzzer. See #77716 for the tracking issue.
Here's a video that demonstrates the issue described in the title of this PR:
duplicate-table-rows-updated-repro.mp4
BEGIN AI GENERATED TEXT
Two collaborators can end up with different visible table contents when a table
contains duplicate rows and one collaborator edits one duplicate while another
collaborator deletes the other duplicate.
The problem is in the CRDT merge for nested array-valued block attributes, not
in the table UI.
core/tablestores rows and cells as nested query attributes.The merge code preserves existing Yjs child objects by matching array entries by
serialized value. Duplicate rows are therefore ambiguous: two different logical
rows can have the same serialized value, so a row delete can be matched against
the wrong Yjs object while a concurrent edit remains attached to another row.
Current Repro
test/e2e/specs/editor/collaboration/collaboration-table-duplicates.spec.tsdocs/explanations/architecture/rtc-stock-repros/videos/duplicate-table-rows.mp4format, with both editor screens visible and a running annotated log. The
local artifact path used to update this branch was
/Users/danluu/dev/fuzz/gutenberg/artifacts/rtc-duplicate-table-rows-video/duplicate-table-rows-updated-repro.mp4.try/awareness-exceptionatd834aab9f47636f85c78e0d8912658155f7fdd18. That was not literallyorigin/trunk, but the table and CRDT implementation files relevant to thisrepro matched trunk at the time of recording.
Normal user-visible flow:
anchor,same,same.sametoedited-second-duplicate.anchor,edited-second-duplicate.The updated repro reads the rendered table cells, not raw block attributes, so
it checks what users actually see in each editor.
Observed vs Expected
Expected: both editors converge on two visible rows:
Observed in the updated repro video:
The users now see different versions of the same shared table.
Branch Contents
This explanation/repro branch intentionally does not contain the production fix.
It contains browser and lower-level repros, the collaboration fixture
stabilization needed by the browser repro, this explanation, and the MP4
artifact.
Changes besides the test:
test/e2e/specs/editor/collaboration/fixtures/collaboration-utils.tswaitsfor transport-level awareness in the current post room before driving the
repro. This avoids depending on the collaborator presence button rendering.
packages/core-data/src/utils/test/crdt-table-duplicates-repro.test.tsreproduces the same failure with only
mergeCrdtBlocks(), twoY.Docs, andYjs updates.
packages/core-data/src/utils/test/crdt.tsreproduces the same failurethrough the post-entity wrapper path:
applyPostChangesToCRDTDoc()andgetPostChangesFromCRDTDoc().docs/explanations/architecture/rtc-stock-repros/duplicate-table-rows.mddocuments the current failure, fix plan, and verification.
docs/explanations/architecture/rtc-stock-repros/videos/duplicate-table-rows.mp4is the updated annotated video artifact.
Lower-Level Repros
The bug can be reproduced below the browser at the core-data CRDT layers:
test/e2e/specs/editor/collaboration/collaboration-table-duplicates.spec.tsdrives two real editor sessions and asserts the rendered table cells.
packages/core-data/src/utils/test/crdt-table-duplicates-repro.test.tsskips WordPress, REST, Playwright, and the sync endpoint. It creates two
Y.Docs, seeds the same table, edits the later duplicate row in one doc,deletes the earlier duplicate row in the other doc, exchanges Yjs updates,
and expects both docs to converge.
packages/core-data/src/utils/test/crdt.tsnow runs the same scenariothrough
applyPostChangesToCRDTDoc()andgetPostChangesFromCRDTDoc().This verifies the post sync config path that collaboration uses before the
generic sync manager sees a document update.
There is no faithful standalone repro in the pure table-state helpers. Files
like
packages/block-library/src/table/state.jsonly transform one local tableattribute object at a time; they do not have Yjs state, collaborator snapshots,
or remote update ordering. The production fix still adds guardrail tests there
to ensure hidden row/cell identity survives local table operations, but the RTC
failure itself starts at the core-data CRDT merge layer.
There is also no table-specific standalone repro in
packages/sync. The syncmanager is intentionally generic and delegates all block/table interpretation to
the
SyncConfigfunctions supplied by core-data. A manager test that importedcore-data's table merge config would cross that package boundary while adding no
new table merge coverage beyond the post-wrapper repro above.
Root Cause
mergeYArray()treats equal array values as interchangeable. For a queryattribute like a table body, that means two distinct rows with identical cell
content can be matched by value instead of by logical identity.
Indexes are not enough once users make concurrent structural edits. A delete can
shift indexes while another user edits the row that used to sit after the
deleted row. Value matching is also not enough because the two duplicate rows
look identical before one of them is edited.
How It Was Introduced
The duplicate-row failure is rooted in #77164, with several earlier PRs making
the table path visible:
84019935998c, "Improve CRDT merge logic for post entities")created the post/block CRDT merge infrastructure in
core-data. It is thebase layer, not the table-specific regression.
80605517663/ac9073b15d3, "RTC: Fix CRDT serialization ofnested RichText attributes") made nested
RichTextDataserializerecursively, including rich text inside table cells.
85695dcffdc, "RTC: Fix RichTextData deserialization") added thematching recursive deserialization through schema query nodes, so table cell
content could round-trip back into runtime
RichTextData.09a21c64b5b, "RTC: Fix core/table cell merging") madecore/tablerows and cells schema-aware nested Yjs structures:array/query attributes became
Y.Array<Y.Map>, object/query attributesbecame
Y.Map, and nested cell content becameY.Text. This enabledin-place table cell merging. At this point, structural length changes still
rebuilt the affected array, which was less stable but did not infer row
identity from duplicate row values.
a6bfd3e5543, "RTC: Improve array attribute stability whenstructural changes occur") changed
mergeYArray()to preserve nested Yjschildren during inserts/deletes by using a left/right sweep and
areArrayElementsEqual(). That was intended to keep existing row/cellobjects stable across structural edits, but it matched query-array elements
by serialized value. For duplicate table rows, two different logical rows
can serialize to the same value (
same), so RTC: Improve array attribute stability when structural changes occur #77164 made those rowsinterchangeable to the diff algorithm. A concurrent delete of one duplicate
and edit of the other can therefore attach the edit/delete to different
logical rows and leave collaborators with divergent visible tables.
Fix Plan
The production fix lives on
try/rtc-duplicate-table-rows-stock-repro-pr-trunk.The safe plan is:
Y.Map. This gives table rows and cells identity even when their serialized
values are duplicates.
block attributes. When CRDT data is deserialized into block attributes, carry
the ID as an enumerable symbol property instead. Object spread preserves it,
but
JSON.stringify, REST payloads, and serialized block HTML do not exposeit.
convert the symbol ID back to the internal CRDT key so
mergeYArray()canmatch rows and cells by identity before falling back to value-based matching.
during cell edits and column insert/delete operations.
property deletion, so it neither creates false diffs nor gets stripped from
existing Y.Map children.
for both possible lower-level core-data repro paths, for identity
round-tripping without string-key leaks, and for the table state operations
that need to preserve symbol properties.
This avoids the main failure mode of a naive fix: leaking
__unstableSyncIdintoruntime block attributes, post content, REST-visible data, copied blocks, or
plugin-observable table attributes.
Verification
The browser repro command is:
The trunk-based PR branch was verified after the fix with:
Result on the fixed PR branch: the focused unit tests passed, the production
build completed, and the browser repro passed with both editors converging on
anchor,edited-second-duplicate.END AI GENERATED TEXT
Use of AI Tools
Everything here was AI generated except the text of this PR that is not in the AI generated text section of this PR.