RTC: Fix title divergence between users on page refresh after title update#77666
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
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. |
0ff6ab3 to
573b567
Compare
|
@danluu This fix looks good! I just wanted to note that the blast radius on this is a bit smaller than the description looks like. I spent some time trying to reproduce in a local post, but had no luck: manual-title-change-failure.movHowever, the end-to-end test replicates every time so I knew that I was missing something. I also tried some variations like using data APIs directly like command-title-change-failure.movAfter some more testing, I found this was only possible if the post is created via API like the reproduction steps. The reason this reproduces is that when a user creates a post via the UI, the fixed logic doesn't run because there's already an existing doc stored with the post and we instead return in a different section of code. Either way, even if the reproduction is tricky this is still a valid bug for some scenarios and I'm glad to have it fixed! Just adding the context above for why the reproduction is difficult, and that this applies to posts created outside of the UI (e.g. via |
What?
This is part of a series of bug reports and PRs from an AI fuzzing project. See #77532 for more details on that.
This PR fixes an issue where users see different titles after a page refresh. See the following videos for what happens before and after the fix:
rtc-title-reload-bug.mp4
rtc-title-reload-fixed.mp4
BEGIN AI GENERATED TEXT
If two collaborators are editing the same post, an unsaved title edit can sync to both users and then be undone for one collaborator when the other collaborator reloads. The collaboration room remains connected: later block edits and later fresh title edits still sync. The lost value is specifically the already-synced, unsaved title value.
The PR branch for review is
try/rtc-title-reload-pr. It has the requested commit shape:dcb258e2f7f01dc64c39820782c5b743a8127d660ff6ab371a02bacfaa57b83e0e19da9e834f3930User-visible Behavior
Expected behavior:
Actual behavior:
The strongest reproduction regresses the non-reloading collaborator, which rules out a simple local page-state explanation.
Confirmed Mechanism
The bug is caused by the CRDT persistence save path replaying a stale REST save response back into the live sync document.
The failure sequence is:
packages/core-data/src/resolvers.jscallssaveEntityRecordso__unstablePrePersistcan write_crdt_document.titleis the stale saved title.packages/core-data/src/actions.jsapplies the full save response to the sync manager withLOCAL_UNDO_IGNORED_ORIGINand{ isSave: true }.This matches the deeper instrumentation:
Repros And Checks
There are three committed repro levels on the PR branch.
The lowest-level repro is in
packages/core-data/src/test/actions.js. The test namedpreserves the live sync title when a CRDT persistence save returns stale post fieldsmodels the sync document as already holding the unsaved title, then runssaveEntityRecordwith a stale REST save response. On the buggy path, the stale title is replayed into the sync document.The middle-level repro is in
packages/core-data/src/test/resolvers.js. The test namedpersistCRDTDoc does not replay a stale save response into the sync documentwires the realpersistCRDTDocresolver callback into the realsaveEntityRecordaction. This reproduces the exact internal path used during reload/bootstrap, without needing a browser.The top-level product repro is the browser test
test/e2e/specs/editor/collaboration/collaboration-title-reload.spec.ts. It uses two real editor pages, a real title edit, a real reload, core-data entity resolution, the sync manager, REST save response handling, and the collaboration provider.To rerun the browser repro on the PR branch:
To see the repros fail before the fix, check out the tests commit and run either the unit repros or the browser repro:
To rerun just the lower-level repros on the PR branch:
The audit branch also contains a broader diagnostic repro suite in
collaboration-title-reload-repro.spec.ts. That file is intentionally not part of the PR branch; it contains exploratory checks that are useful for investigation but too broad for the small PR.Environment Used
7.1-alpha-6226023.0.11642980d599,RTC: Fix "Connection Lost" dialog when too many entities are loaded (#77631)v20.19.010.8.2WP_BASE_URL=http://localhost:8889Why This Is Not A False Positive
The bug does not depend on fuzz-only hooks, injected faults, or mocked browser state. It reproduces in normal Playwright browser sessions with two real editor pages.
The bug also survives deeper checks:
One subtle non-bug is worth separating: the saved post record remaining on the initial title before explicit save is normal. The production bug is that the stale saved title is replayed into the live CRDT document during an automatic CRDT persistence save.
Introduction Analysis
The strongest direct introduction candidate is
ea2cfb87be4, from PR #75841,RTC: Fix entity save call / initial persistence.That change altered the CRDT persistence callback from saving only edited fields to resolving the full edited entity record and calling
saveEntityRecord. The change was made so CRDT persistence could trigger even when there were no ordinary unsaved entity edits. That solved the initial-persistence problem, but it also meant an automatic CRDT persistence save could now receive a full stale REST response and feed that response into the generic synced-save path.Relevant history:
saveEditedEntityRecord.Y.Text. It is a prerequisite for this title-specific symptom, but it is not the most direct stale-save-response introduction point.Fix Plan And Rationale
The fix should not add sleeps, retries, or reload-specific special cases. The failure is deterministic once the automatic CRDT persistence save replays a stale full REST response.
The proposed fix is the one in the PR branch:
saveEntityRecordbehavior unchanged by default.__unstableSkipSyncUpdateoption tosaveEntityRecord.{ isSave: true }.persistCRDTDoc, where the save exists to persist_crdt_documentand should not make stale REST fields authoritative.The rationale is that the sync manager still needs to mark the document as saved, but it must not treat the REST response title/content fields as new collaborative edits for this internal persistence save. Passing
{}preserves the save marker path without applying stale post fields.Minimum verification for the fix:
Verification run on the PR branch:
END AI GENERATED TEXT
Use of AI Tools
The bug finding as well as the fix are AI generated. So far, of the manually verified (verified by humans) bugs the fuzzer has found, we have 2 real bugs and 0 false positives (a number of bugs have not been checked by someone who's familiar with Gutenberg). I'm not very familiar with Gutenberg and am not a good judge of whether or not something is a false positive, but the divergence between one user's title and the other seems like something that shouldn't happen?