RTC: Fix race condition on room creation which can cause a split update log

[danluu]

What?

This is part of a series of bug reports and PRs from an AI fuzzing project. See #77532 for more details on that. I'm filing a few more of these after a discussion with @alecgeatches on how the fuzzer found a bug that I hadn't file that ended up taking some developer time to chase down and repro.

first-room-storage-live-repro.mp4

first-room-storage-document-edit-repro.mp4

BEGIN AI GENERATED TEXT

The post-meta sync storage backend creates one wp_sync_storage post per sync room and uses md5( $room ) as the post slug. On first access to a new room, two concurrent writers can both miss the existing-storage lookup and both call wp_insert_post() with the same slug. WordPress keeps slugs unique, so one request receives the canonical slug and the other receives a suffixed slug such as <hash>-2.

There is one trunk/product bug covered by this fix:

First-writer split storage bug. The original storage creation path trusted the inserted post ID without checking whether WordPress kept the exact room-hash slug. If a request received a suffixed post, it could acknowledge awareness or sync updates written to the suffixed lineage. A fresh storage instance later looks up only the canonical room hash, so those acknowledged first-access writes are unreachable. Source: get_storage_post_id() creates and caches the storage post. Repro/test: test_first_access_race_does_not_split_room_storage().

Branch Layout

The PR branch danluu/fix-rtc-room-split intentionally contains only the PHPUnit regression tests and the storage fix. This explanation is kept on the separate danluu/rtc-room-split-explanation branch so it does not appear in the PR diff.

There is no top-level Playwright/e2e test in the PR branch.

Reproduction Layers

PHPUnit storage repro

phpunit/tests/collaboration/wpSyncPostMetaStorage.php::test_first_access_race_does_not_split_room_storage

This test installs a wp_insert_post_data filter that injects a competing wp_insert_post() exactly between the storage layer's miss and its insert. It then asserts:

• only one published wp_sync_storage lineage exists for the room hash;
• the surviving lineage uses the exact room hash slug;
• a fresh WP_Sync_Post_Meta_Storage instance can read the acknowledged update.

On the vulnerable implementation, the test fails because WordPress creates both <hash> and <hash>-2, and the update can be written to the suffixed post.

Browser/video repro

The local browser/video harness uses editor-visible presence to expose the split. It creates a draft post, enables a test-only race injector for the post's sync room, opens the post as admin, and delays the initial empty sync polls until the admin's presence payload contains visible collaborator metadata. The first meaningful admin presence poll then travels through the production /wp-sync/v1/updates endpoint and triggers the storage race.

After that acknowledged first write, the test pauses admin polling so the normal retry loop cannot immediately heal the symptom. A collaborator opens the same post and polls the canonical room. The test then holds the collaborator's next poll and releases the admin poll. On the vulnerable implementation, the admin screen learns that the collaborator joined, but the collaborator screen still has no Collaborators list button, because the admin's acknowledged first presence write was written to the suffixed storage lineage.

The deterministic browser harness still drives the production editor and sync REST path. The artificial parts are the scheduler that forces the otherwise timing-dependent first-writer race and the short admin-poll pause that keeps the user-visible presence loss observable long enough for the assertion and video. The diagnostic REST endpoint verifies the supporting storage invariant. The browser/video harness is not part of the PR branch.

Annotated videos for this bug:

• /Users/danluu/conductor/workspaces/gutenberg-v1/providence/.context/rtc-issue-08/first-room-storage-live-repro.mp4
• /Users/danluu/conductor/workspaces/gutenberg-v1/providence/.context/rtc-issue-08/first-room-storage-document-edit-repro.mp4

Observed vulnerable-branch result:

Expected lineage_count: 1
Received lineage_count: 2
Admin screen shows collaborator presence
Collaborator screen has no Collaborators list button

Real-vs-False-Positive Analysis

Oracle validity

The oracle is valid. A sync room must have one reachable storage lineage because fresh readers find storage by the canonical room hash. If an acknowledged write lands in a suffixed lineage, it is not reachable through normal get_storage_post_id() lookup in a new request.

Generated shape validity

The generated room shape is valid. The low-level repro uses postType/post:<id>:first-access-race, which matches the production REST room pattern. The browser repro uses the normal editor room postType/post:<id>.

Helper misuse

The failing path does not rely on private test-only storage calls for the browser repro. It opens normal editor sessions and lets the production sync endpoint call WP_Sync_Post_Meta_Storage. The PHPUnit repro uses the storage class directly to minimize the race window, but the same get-or-insert code is used by production REST requests.

Environment contamination

The tests reset the static room cache before fresh-reader checks. The room hash includes the test post ID and a unique suffix, and lineage counting filters by post type, publish status, exact room-hash slug, and suffixed room-hash slugs. The failure is not caused by stale cache state or older storage rows.

Race-injection-only behavior

The injection forces an interleaving that can occur with two concurrent HTTP requests first accessing a new room: both miss lookup, both insert, and WordPress uniquifies one slug. The injected writer uses wp_insert_post() with the same post type, status, title, and slug as production storage creation.

Known-fixed bug masking

The issue is independent of previously fixed cursor and compaction races. Those bugs affect update retrieval and deletion after storage already exists. This bug happens before the room has canonical storage and can lose the very first awareness or sync update into a suffixed lineage.

Fix Plan

The storage layer must treat the exact room-hash slug as the only canonical storage lineage:

• After inserting a storage post, resolve the canonical exact-slug post before returning or caching a post ID.
• If the inserted post is the only candidate and no exact slug exists, promote that post to the exact room-hash slug before using it.
• If a suffixed duplicate is observed while resolving the race, merge its metadata into the exact-slug post.
• Do not delete a suffixed storage post if moving its postmeta rows fails.
• Cache only the resolved canonical post ID.
• Cover the first-access race in PHPUnit.

The PR branch intentionally does not include this explanation, the browser-only diagnostic mu-plugin, or an e2e repro harness.

END AI GENERATED TEXT

Use of AI Tools

The code here is all AI generated. As noted above, the bug finding came from an AI fuzzing project.

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: danluu <danluu@git.wordpress.org>
Co-authored-by: dmsnell <dmsnell@git.wordpress.org>
Co-authored-by: peterwilsoncc <peterwilsoncc@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

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

[dmsnell]

I haven’t followed the post-cache issues too closely, but I wonder if this is going to be problematic as well.

I would expect that promoting a non-canonical storage meta will occur rarely (vs. multiple times per second as was occurring in trunk for edits), but I would like some double-checking on this to make sure it would not cause thrashing on the site with other posts.

@peterwilsoncc this PR might seem a bit random, but are you able to speak to the use of clean_post_cache() here? the situation is that two sessions opened a post and due to a race condition, created two separate wp_sync_storage rows. this code is resolving that race.

[peterwilsoncc]

I think this can use wp_update_post()

[dmsnell]

thanks @peterwilsoncc — I’ve made that update in a871f1c, but I wonder if there’s important gains running through all the additional machinery in get_posts(). given that this is going to be occurring frequently during an edit session.

I’ll leave it and we can see later if it’s problematic. if this code disappears in favor of a table then I suppose this is good enough.

[dmsnell]

I think the concern may be minor here, but I wanted to ask if this OR clause is hiding a needless table scan. I asked an LLM to analyze it and it suggested that a UNION instead could introduce two index-scans to replace a single table scan.

Here is MariaDB’s EXPLAIN for the two versions:

MariaDB [wordpress]> EXPLAIN SELECT ID FROM wp_posts WHERE post_type = "wp_sync_storage" AND post_status = "publish" AND ( post_name = "abc123" OR post_name LIKE "abc123-" ) ORDER BY ID ASC;
+------+-------------+----------+-------+-----------------------------------------------+---------+---------+------+------+-------------+
| id   | select_type | table    | type  | possible_keys                                 | key     | key_len | ref  | rows | Extra       |
+------+-------------+----------+-------+-----------------------------------------------+---------+---------+------+------+-------------+
|    1 | SIMPLE      | wp_posts | index | post_name,type_status_date,type_status_author | PRIMARY | 8       | NULL | 35   | Using where |
+------+-------------+----------+-------+-----------------------------------------------+---------+---------+------+------+-------------+
1 row in set (0.003 sec)

MariaDB [wordpress]> EXPLAIN SELECT ID FROM wp_posts WHERE post_type = "wp_sync_storage" AND post_status = "publish" AND post_name = "abc123" UNION SELECT ID FROM wp_posts WHERE post_type = "wp_sync_storage" AND post_status = "publish" AND post_name LIKE "abc123-" ORDER BY ID ASC;
+------+--------------+------------+-------+-----------------------------------------------+-----------+---------+-------+------+----------------+
| id   | select_type  | table      | type  | possible_keys                                 | key       | key_len | ref   | rows | Extra          |
+------+--------------+------------+-------+-----------------------------------------------+-----------+---------+-------+------+----------------+
|    1 | PRIMARY      | wp_posts   | ref   | post_name,type_status_date,type_status_author | post_name | 766     | const | 1    | Using where    |
|    2 | UNION        | wp_posts   | range | post_name,type_status_date,type_status_author | post_name | 766     | NULL  | 1    | Using where    |
| NULL | UNION RESULT | <union1,2> | ALL   | NULL                                          | NULL      | NULL    | NULL  | NULL | Using filesort |
+------+--------------+------------+-------+-----------------------------------------------+-----------+---------+-------+------+----------------+
3 rows in set (0.002 sec)

Granted, for my little test site with two posts both are going to be roughly equivalent, but if we hit a site with thousands of posts, I wonder if this could introduce noticeable DB overhead.

Not being sure how to interpret the Using filesort I also asked about eliminating that by sorting in PHP and there was no clear guidance other than to say that when the expected set is small (which it is here) there’s no practical difference.

[peterwilsoncc]

@danluu Are you able to provide more information about how the race condition is being forced, so I can do so locally.

The reason I am asking is that I'd like to know how real world this is, the additional work being done will slow down requests which would make any race condition more likely to occur.

@dmsnell The clear_post_cache call is correct as so get_posts() queries up-to-date data on follow up calls.

[peterwilsoncc]

I've added some first view notes inline but my main question is in the comment above re: reproduction steps.

I know from conversations with Matt that he is pretty keen on avoiding the post/post_meta approach so I'd really prefer to focus our energies there rather than on code that's unlikely to make a WordPress release.

As the updates are added to a row identified by the room name, the race condition isn't able to occur with the improved storage structure.

Aside: while post_meta has been the prompt for Core-64696 (don't read all of it), using posts for rooms is an abuse of both the posts and posts_meta tables in my view.

[peterwilsoncc]

This may result in two copies of awareness post_meta stored against a single post ID.

[dmsnell]

do you have the mechanism for how this would create the duplicates? we did some investigation into meta_id, but trying to prevent that ended up adding considerable complexity and was addressing a secondary effect of this PR, which itself is preventing duplicate posts at a more frequent rate.

I think that even in the presence of new risk here, my assessment is that this is significantly lowering the overall risk. that is, we’re not introducing the possibility of creating duplicate awareness posts; we’re starting in a system that already duplicates them and we’re removing one common way it does that.

[peterwilsoncc]

This can use the get_posts API.

$posts = get_posts(
				array(
					'post_type'      => self::POST_TYPE,
					'posts_per_page' => 1,
					'post_status'    => 'publish',
					'name'           => $room_hash,
					'fields'         => 'ids',
					'orderby'        => 'ID',
					'order'          => 'ASC',
				)
			);

[dmsnell]

thanks @peterwilsoncc — I’ve made that update in a871f1c, but I wonder if there’s important gains running through all the additional machinery in get_posts(). given that this is going to be occurring frequently during an edit session.

I’ll leave it and we can see later if it’s problematic. if this code disappears in favor of a table then I suppose this is good enough.

[dmsnell]

oops I changed the code to get_posts() but then I didn’t change the response handler, introducing a bug. @t-hamano caught it and resolved it in #78053

[peterwilsoncc]

I think this can use wp_update_post()

[danluu]

@peterwilsoncc Thanks for taking a look! Specifically on the question of the repro, the checked in e2e test forced a race by creating a wp_insert_post_data filter that does a wp_insert_post (with a check to prevent infinite recursion).

For a less artificial (but non-deterministic) repro, this script attempts to demonstrate the race: https://github.com/danluu/gutenberg/blob/danluu/rtc-room-split-explanation/docs/explanations/fuzzer-bugs/rtc-issue-08-normal-rest-repro.sh. The script creates concurrent draft posts across users and then checks if there was a room split. It works on my machine fairly consistently, but it may be less consistent in a different environment.

[peterwilsoncc]

For a less artificial (but non-deterministic) repro, this script attempts to demonstrate the race: https://github.com/danluu/gutenberg/blob/danluu/rtc-room-split-explanation/docs/explanations/fuzzer-bugs/rtc-issue-08-normal-rest-repro.sh. The script creates concurrent draft posts across users and then checks if there was a room split. It works on my machine fairly consistently, but it may be less consistent in a different environment.

Thanks for the script @danluu -- while feedback comes in from hosts on the performance of the various options (see the call for testing on make/hosting), I'm going to compare the effects across the various options. There's a good chance something will be needed but the exact shape is yet to be determined.

[dmsnell]

@peterwilsoncc

I know from conversations with Matt that he is pretty keen on avoiding the post/post_meta approach so I'd really prefer to focus our energies there rather than on code that's unlikely to make a WordPress release.

I might have missed the news in the flurry over the past week, but I thought we are still shipping the post meta approach for 7.0?

If so, the goal is to get this code in for the release, as a bug fix to the RTC functionality introduced in 7.0. The idea being that it would be best to risk duplicating some effort now to prevent people from getting corrupted documents when they try out the new hallmark feature of the release.

[peterwilsoncc]

No such comment has been posted on the core ticket so I have been working on the basis that we’d be proceeding with the table given that was why Matt delayed the release. If you’re hearing otherwise are you able to ask someone to post on the core ticket to the effect and modify the milestone accordingly

…

I might have missed the news in the flurry over the past week, but I thought we are still shipping the post meta approach for 7.0?

[dmsnell]

I'd really prefer to focus our energies there rather than on code that's unlikely to make a WordPress release.

@peterwilsoncc thank you for your insight here and your helpful review on this PR. I hear that feedback, but I want to provide a bit more context:

• this work isn’t taking focus away from other parts of the system. @danluu and I have been doing bug sprints on RTC and are going to be continuing to do through up through the release. we’re tackling whichever parts of the system present the bugs and this one just happened to be in the post_meta part.
• if we end up with a table in 7.0 then I would hope that work we do on post_meta won’t end up mattering that much. we can throw it all away, and this code can be deleted. as it stands, this is currently in the RC so I’d rather we have a more-working RC even if we re-engineer it than a broken RC. hopefully as we update the code, these additional tests and verifications will guide our transition to a table.

I just want to say that as far as we can guarantee, we will be monitoring this code and happy to handle any necessary follow-ups or reverts. as we stack up detected bugs and fixes though, it’s becoming harder and harder to address them because we’re finding the same bugs repeatedly.

[dmsnell]

There could be some follow-up work on this, but it appears to solve a real problem. I’ll handle the backport.

If anything serious turns up we can revert.

[Mamaduka]

The unit tests are failing on trunk after this was merged. Details - https://github.com/WordPress/gutenberg/actions/runs/25459792903/job/74699025309

[t-hamano]

These unit tests should be fixed here: #78053

Furthermore, let's backport this to wp/7.0 to maintain consistency.

[github-actions]

I just cherry-picked this PR to the wp/7.0 branch to get it included in the next release: 888812a