Skip to content

RTC: Ensure a single canonical update log for collaborative edits. #11660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
dmsnell wants to merge 3 commits into
base: trunk
Choose a base branch
from
Draft

RTC: Ensure a single canonical update log for collaborative edits. #11660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7045bed
RTC: Ensure a single canonical update log for collaborative edits.
Apr 27, 2026
b4139bb
Updates from final merged PR.
dmsnell May 6, 2026
0f43ace
Merge remote-tracking branch 'upstream/trunk' into rtc/avoid-duplicat…
dmsnell May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 168 additions & 7 deletions src/wp-includes/collaboration/class-wp-sync-post-meta-storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
// Use direct database operation to avoid cache invalidation performed by
// post meta functions (`wp_cache_set_posts_last_changed()` and direct
// `wp_cache_delete()` calls).
return (bool) $wpdb->insert(
$result = $wpdb->insert(
$wpdb->postmeta,
array(
'post_id' => $post_id,
Expand All @@ -95,6 +95,13 @@
),
array( '%d', '%s', '%s' )
);

if ( $result ) {
Copy link
Copy Markdown

@apermo apermo Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While technically fine, how would you feel about a strict comparison? insert() return int|false.

What about

Suggested change
if ( $result ) {
if ( is_int( $result ) ) {

Copy link
Copy Markdown
Member Author

@dmsnell dmsnell May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically this change would introduce a bug, I believe, in the case that the insert inserted zero rows. that is, false and 0 are equivalent for this check.

we could use if ( is_int( $result ) && $result > 0 ) or just if ( $result > 0 ) but the former seems verbose and the latter seems no different in essence than this truthy check.

thoughts?

$room_hash = md5( $room );
self::$storage_post_ids[ $room_hash ] = $this->merge_duplicate_storage_posts( $room_hash, $post_id );

Check warning on line 101 in src/wp-includes/collaboration/class-wp-sync-post-meta-storage.php

View workflow job for this annotation

GitHub Actions / PHP static analysis / Run PHP static analysis 

Call to an undefined method WP_Sync_Post_Meta_Storage::merge_duplicate_storage_posts().
}

return (bool) $result;
}

/**
Expand Down Expand Up @@ -153,7 +160,8 @@
public function set_awareness_state( string $room, array $awareness ): bool {
global $wpdb;

$post_id = $this->get_storage_post_id( $room );
$room_hash = md5( $room );
$post_id = $this->get_storage_post_id( $room );
if ( null === $post_id ) {
return false;
}
Expand All @@ -174,16 +182,22 @@
);

if ( $meta_id ) {
return (bool) $wpdb->update(
$result = $wpdb->update(
$wpdb->postmeta,
array( 'meta_value' => wp_json_encode( $awareness ) ),
array( 'meta_id' => $meta_id ),
array( '%s' ),
array( '%d' )
);

if ( false !== $result ) {
self::$storage_post_ids[ $room_hash ] = $this->merge_duplicate_storage_posts( $room_hash, $post_id );

Check warning on line 194 in src/wp-includes/collaboration/class-wp-sync-post-meta-storage.php

View workflow job for this annotation

GitHub Actions / PHP static analysis / Run PHP static analysis 

Call to an undefined method WP_Sync_Post_Meta_Storage::merge_duplicate_storage_posts().
}
Comment on lines +193 to +195
Copy link
Copy Markdown

@apermo apermo Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you actually did what i suggested above, but why the negative check instead of the positive test?

Copy link
Copy Markdown
Member Author

@dmsnell dmsnell May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong on this, but I believe that the difference is in creating vs. updating a row in the database. we need to know that there’s an appropriate post before we call merge_duplicate_storage_posts(). it’s okay if something fails and the editor has to retry, but we need to be careful to avoid corrupting the sync meta on the server.

from the code perspective, a 0 from $wpdb->insert() means there’s no row in the database to merge, but a 0 from $wpdb->update() might mean the post was already in the state it needs to be in.


return false !== $result;
}

return (bool) $wpdb->insert(
$result = $wpdb->insert(
$wpdb->postmeta,
array(
'post_id' => $post_id,
Expand All @@ -192,6 +206,12 @@
),
array( '%d', '%s', '%s' )
);

if ( $result ) {
self::$storage_post_ids[ $room_hash ] = $this->merge_duplicate_storage_posts( $room_hash, $post_id );

Check warning on line 211 in src/wp-includes/collaboration/class-wp-sync-post-meta-storage.php

View workflow job for this annotation

GitHub Actions / PHP static analysis / Run PHP static analysis 

Call to an undefined method WP_Sync_Post_Meta_Storage::merge_duplicate_storage_posts().
}
Comment on lines +210 to +212
Copy link
Copy Markdown

@apermo apermo Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, same argument/case

Copy link
Copy Markdown
Member Author

@dmsnell dmsnell May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same explanation 😄


return (bool) $result;
}

/**
Expand Down Expand Up @@ -256,14 +276,155 @@
)
);

if ( is_int( $post_id ) ) {
self::$storage_post_ids[ $room_hash ] = $post_id;
return $post_id;
if ( is_int( $post_id ) && $post_id > 0 ) {
$canonical_post_id = $this->resolve_canonical_storage_post_id_after_insert( $room_hash, $post_id );
if ( null === $canonical_post_id ) {
return null;
}

self::$storage_post_ids[ $room_hash ] = $canonical_post_id;
return $canonical_post_id;
}

return null;
}

/**
* Resolves the canonical room storage post after inserting a new post.
*
* Two concurrent first writers can both miss the lookup above and create
* storage posts for the same room hash. Depending on the exact interleaving,
* WordPress may create either a duplicate exact slug or a suffixed slug.
* When this request receives a non-canonical post, redirect it to the
* canonical storage before any sync or awareness data is written.
*
* @since 7.0.0
*
* @param string $room_hash MD5 hash of the room identifier.
* @param int $inserted_post_id Post ID returned by wp_insert_post().
* @return int|null Canonical storage post ID.
*/
private function resolve_canonical_storage_post_id_after_insert( string $room_hash, int $inserted_post_id ): ?int {
$canonical_post_id = $this->find_canonical_storage_post_id( $room_hash );
if ( null === $canonical_post_id ) {
$canonical_post_id = $this->promote_storage_post_to_canonical_slug( $room_hash, $inserted_post_id );
}

if ( null === $canonical_post_id ) {
Copy link
Copy Markdown

@apermo apermo Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still consider checking for intval() instead for null, but in these cases the case is not as strong as above.

Copy link
Copy Markdown
Member Author

@dmsnell dmsnell May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. hm. the functions returning the $canonical_post_id are both typed to return int|null (unlike $wpdb->insert() etc…)

there should be no reasonable way to get to this point if it’s neither null nor int (strict-mode aside), so I think in this case it’s more or less equivalent, no?

the intval() emphasizes what it expects and the null-check emphasizes the fact that a post id might not exist.

wp_delete_post( $inserted_post_id, true );
return null;
}

if ( $inserted_post_id !== $canonical_post_id ) {
/*
* This request just created a duplicate empty storage post because
* another first writer won the exact-slug race. Delete only that
* just-created empty post and write this request's data to canonical
* storage.
*
* Do not merge or delete older duplicate storage posts here. A stale
* request may already hold a duplicate post ID, and MySQL advisory
* locks/raw transactions are not a reliable cross-server fence under
* HyperDB or database proxies. Future historical repair should be
* bounded and idempotent, or run out of band with primary-pinned
* verification and a grace period before deleting duplicates.
*/
wp_delete_post( $inserted_post_id, true );
}

return $canonical_post_id;
}

/**
* Finds the canonical storage post for a room hash.
*
* The canonical post is the oldest published storage post with the exact
* room hash slug. Suffixed slugs are repair candidates, not canonical.
*
* @since 7.0.0
*
* @param string $room_hash MD5 hash of the room identifier.
* @return int|null Canonical storage post ID.
*/
private function find_canonical_storage_post_id( string $room_hash ): ?int {
Copy link
Copy Markdown
Contributor

@t-hamano t-hamano May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function appears to always return null. I have submitted additional fixes in Gutenberg.

WordPress/gutenberg#78053

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

return is_numeric( $post_id ) ? (int) $post_id : null;
}

/**
* Promotes a storage post to the canonical room slug.
*
* @since 7.0.0
*
* @param string $room_hash MD5 hash of the room identifier.
* @param int $post_id Post ID to promote.
* @return int|null Promoted post ID on success.
*/
private function promote_storage_post_to_canonical_slug( string $room_hash, int $post_id ): ?int {
global $wpdb;

/*
* @todo Could this be replaced by {@see wp_update_post()}? Could we experience
* a race with other posts having a different post type or post status?
*/
$result = $wpdb->update(
$wpdb->posts,
array( 'post_name' => $room_hash ),
array(
'ID' => $post_id,
'post_type' => self::POST_TYPE,
'post_status' => 'publish',
),
array( '%s' ),
array( '%d', '%s', '%s' )
);

if ( false === $result ) {
return null;
}

clean_post_cache( $post_id );
return $post_id;
}

/**
* Lists storage posts belonging to a room hash, including suffixed duplicates.
*
* @since 7.0.0
*
* @param string $room_hash MD5 hash of the room identifier.
* @return array<int> Storage post IDs.
*/
private function get_storage_post_ids_for_room_hash( string $room_hash ): array {
global $wpdb;

$post_ids = $wpdb->get_col(
$wpdb->prepare(
"SELECT ID FROM {$wpdb->posts}
WHERE post_type = %s
AND post_status = 'publish'
AND ( post_name = %s OR post_name LIKE %s )
ORDER BY ID ASC",
self::POST_TYPE,
$room_hash,
$wpdb->esc_like( $room_hash . '-' ) . '%'
)
);

return array_map( 'intval', $post_ids );
}

/**
* Gets the number of updates stored for a given room.
*
Expand Down
Loading
Loading