Skip to content

RTC: Use prepared queries instead of *_post_meta functions #11325

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

Open
chriszarate wants to merge 9 commits into
base: trunk
Choose a base branch
from
+793 −284
Open

RTC: Use prepared queries instead of *_post_meta functions #11325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b76450c
Use prepared queries instead of `*_post_meta` functions
chriszarate Mar 20, 2026
bfb5f12
Add unit tests to assert post meta cache is preserved
chriszarate Mar 20, 2026
a0f1fdd
Remove unnecessary unslashing and use JSON encoding
chriszarate Mar 21, 2026
0e73dcd
Consolidate post meta storage tests
chriszarate Mar 21, 2026
4a4596d
Ignore race condition and coalesce on latest row
chriszarate Mar 23, 2026
290a1dc
Add @global markers
chriszarate Mar 23, 2026
59251c0
Add @ticket markers
chriszarate Mar 23, 2026
5168269
Update comment
chriszarate Mar 23, 2026
a1b2cee
Add explicit order by statement to get_storage_post_id query
chriszarate Mar 23, 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
96 changes: 86 additions & 10 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 @@ -30,15 +30,15 @@ class WP_Sync_Post_Meta_Storage implements WP_Sync_Storage {
* @since 7.0.0
* @var string
*/
const AWARENESS_META_KEY = 'wp_sync_awareness';
const AWARENESS_META_KEY = 'wp_sync_awareness_state';

/**
* Meta key for sync updates.
*
* @since 7.0.0
* @var string
*/
const SYNC_UPDATE_META_KEY = 'wp_sync_update';
const SYNC_UPDATE_META_KEY = 'wp_sync_update_data';

/**
* Cache of cursors by room.
Expand Down Expand Up @@ -69,36 +69,68 @@ class WP_Sync_Post_Meta_Storage implements WP_Sync_Storage {
*
* @since 7.0.0
*
* @global wpdb $wpdb WordPress database abstraction object.
*
* @param string $room Room identifier.
* @param mixed $update Sync update.
* @return bool True on success, false on failure.
*/
public function add_update( string $room, $update ): bool {
global $wpdb;

$post_id = $this->get_storage_post_id( $room );
if ( null === $post_id ) {
return false;
}

$meta_id = add_post_meta( $post_id, self::SYNC_UPDATE_META_KEY, $update, false );

return (bool) $meta_id;
// 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(
$wpdb->postmeta,
array(
'post_id' => $post_id,
'meta_key' => self::SYNC_UPDATE_META_KEY,
'meta_value' => wp_json_encode( $update ),
),
array( '%d', '%s', '%s' )
);
}

/**
* Gets awareness state for a given room.
*
* @since 7.0.0
*
* @global wpdb $wpdb WordPress database abstraction object.
*
* @param string $room Room identifier.
* @return array<int, mixed> Awareness state.
*/
public function get_awareness_state( string $room ): array {
global $wpdb;

$post_id = $this->get_storage_post_id( $room );
if ( null === $post_id ) {
return array();
}

$awareness = get_post_meta( $post_id, self::AWARENESS_META_KEY, true );
// Use direct database operation to avoid updating the post meta cache.
// ORDER BY meta_id DESC ensures the latest row wins if duplicates exist
// from a past race condition in set_awareness_state().
$meta_value = $wpdb->get_var(
$wpdb->prepare(
"SELECT meta_value FROM $wpdb->postmeta WHERE post_id = %d AND meta_key = %s ORDER BY meta_id DESC LIMIT 1",
$post_id,
self::AWARENESS_META_KEY
)
);

if ( null === $meta_value ) {
return array();
}

$awareness = json_decode( $meta_value, true );

if ( ! is_array( $awareness ) ) {
return array();
Expand All @@ -112,19 +144,54 @@ public function get_awareness_state( string $room ): array {
*
* @since 7.0.0
*
* @global wpdb $wpdb WordPress database abstraction object.
*
* @param string $room Room identifier.
* @param array<int, mixed> $awareness Serializable awareness state.
* @return bool True on success, false on failure.
*/
public function set_awareness_state( string $room, array $awareness ): bool {
global $wpdb;

$post_id = $this->get_storage_post_id( $room );
if ( null === $post_id ) {
return false;
}

// update_post_meta returns false if the value is the same as the existing value.
update_post_meta( $post_id, wp_slash( self::AWARENESS_META_KEY ), wp_slash( $awareness ) );
return true;
// 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).
//
// If two concurrent requests both see no row and both INSERT, the
// duplicate is harmless: get_awareness_state() reads the latest row
// (ORDER BY meta_id DESC).
$meta_id = $wpdb->get_var(
$wpdb->prepare(
"SELECT meta_id FROM $wpdb->postmeta WHERE post_id = %d AND meta_key = %s ORDER BY meta_id DESC LIMIT 1",
$post_id,
self::AWARENESS_META_KEY
)
);

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

return (bool) $wpdb->insert(
$wpdb->postmeta,
array(
'post_id' => $post_id,
'meta_key' => self::AWARENESS_META_KEY,
'meta_value' => wp_json_encode( $awareness ),
),
array( '%d', '%s', '%s' )
);
Copy link
Contributor

@adamziel adamziel Mar 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

When this code paths is processed in two concurrent requests, we may end up with two post meta records.

Unfortunately, WordPress doesn't give us many options here. If we had a unique index, we could use INSERT INTO ON DUPLICATE KEY UPDATE, but we don't. If we always used InnoDB tables, we could use SELECT FOR UPDATE, but we don't.

The only solution I can think if are advisory locks, so something like:

global $wpdb;

$lock_name = "meta_{$post_id}_{$meta_key}";
$lock_acquired = $wpdb->get_var(
	$wpdb->prepare("SELECT GET_LOCK(%s, 5)", $lock_name)
);

if ($lock_acquired) {
	try {
		// Safe to do get_var/update/insert
	} finally {
	    $wpdb->query($wpdb->prepare("SELECT RELEASE_LOCK(%s)", $lock_name));
	}
}

I don't trust $wpdb->prepare(), but I also don't see any alternatives. insert() and update() use prepare() internally anyway and $wpdb the public API doesn't seem to provide any alternatives. I'd really love to expose actual prepared statements and deprecate all these string-stitching methods in 7.1 or 7.2 .

Copy link
Author

@chriszarate chriszarate Mar 23, 2026

Choose a reason for hiding this comment

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

Isn't this the same race condition that exists in update_post_meta? For single meta values, if the meta key does not exist, it delegates to add_post_meta. That function bails if the key does exist:

wordpress-develop/src/wp-includes/meta.php

Lines 265 to 267 in 0e73dcd

$meta_ids = $wpdb->get_col( $wpdb->prepare( "SELECT $id_column FROM $table WHERE meta_key = %s AND $column = %d", $meta_key, $object_id ) );
if ( empty( $meta_ids ) ) {
return add_metadata( $meta_type, $object_id, $raw_meta_key, $passed_value );

wordpress-develop/src/wp-includes/meta.php

Lines 95 to 102 in 0e73dcd

if ( $unique && $wpdb->get_var(
$wpdb->prepare(
"SELECT COUNT(*) FROM $table WHERE meta_key = %s AND $column = %d",
$meta_key,
$object_id
)
) ) {
return false;

At any rate, I can think of two alternate solutions:

  1. Accept the race condition and add ORDER BY meta_id DESC LIMIT 1 to the SELECT query. A duplicate row is created, but all peers will coalesce on the same row.
    • Incidentally, a similar race condition exists for the post creation in get_storage_post_id and is resolved similarly.
  2. Since we don't have a UNIQUE index to help us, abandon post meta and store the awareness state in the post itself (e.g., post_content).

@adamziel What do you think? Are advisory locks still a better solution?

Copy link
Author

@chriszarate chriszarate Mar 23, 2026

Choose a reason for hiding this comment

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

I've gone ahead and pushed up solution 1 in 4a4596d.

I haven't bothered to implement clean up since there should be at most one duplicate row per room.

Copy link
Author

@chriszarate chriszarate Mar 23, 2026

Choose a reason for hiding this comment

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

Incidentally, a similar race condition exists for the post creation in get_storage_post_id

Just FYI (since I brought it up), I don't think this race condition requires solving. With the polling provider, no sync updates are sent until another collaborator is detected. In other words, we are guaranteed at least two consecutive polling requests sent by the same user to "initialize" the room, so we should never hit this condition.

Copy link
Contributor

@adamziel adamziel Mar 23, 2026

Choose a reason for hiding this comment

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

@chriszarate Thank you for elaborating!

Isn't this the same race condition that exists in update_post_meta?

Sure. I'd have the same note if we've used update_post_meta here.

Accept the race condition and add ORDER BY meta_id DESC LIMIT 1 to the SELECT query. A duplicate row is created, but all peers will coalesce on the same row.

As long as some important piece of information isn't getting trapped in that second record, this sounds like a good solution. Thank you! And good inline commenting as well.

I haven't bothered to implement clean up since there should be at most one duplicate row per room.

I'm not sure I follow – I don't see any DELETE query for the awareness state so I think we're good?

Also, theoretically you can exploit this race condition to insert a 100 duplicates. It doesn't seem very likely, but it is possible. I think ignoring those duplicates as you did in 4a4596d is a smart idea.

With the polling provider, no sync updates are sent until another collaborator is detected.

While I understand that's what the current implementation intends to do, sometimes things don't go as planned. Accounting for unexpected duplicate meta keys early on may save hours or days of trying to reproduce a weird, non-deterministic bug report later.

Copy link
Author

@chriszarate chriszarate Mar 23, 2026

Choose a reason for hiding this comment

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

As long as some important piece of information isn't getting trapped in that second record, this sounds like a good solution.

Awareness state is not critical to sync behavior. If this race condition is hit, we lose the awareness state for a single peer but it will be restored on the next poll (and by that point the race condition no longer exists).

I'm not sure I follow – I don't see any DELETE query for the awareness state so I think we're good?

Oops. I initially implemented a cleanup function but removed it and forgot to update the comment. Fixed in 5168269.

Also, theoretically you can exploit this race condition to insert a 100 duplicates. It doesn't seem very likely, but it is possible.

The race condition can only be hit when the meta key does not yet exist, so it does seem pretty unlikely. Maybe a determined bad actor could squeeze out x - 1 duplicates, where x is the number of PHP workers?

While I understand that's what the current implementation intends to do, sometimes things don't go as planned. Accounting for unexpected duplicate meta keys early on may save hours or days of trying to reproduce a weird, non-deterministic bug report later.

Great advice. I'll think about ways to address that in a separate update.

}

/**
Expand Down Expand Up @@ -168,6 +235,8 @@ private function get_storage_post_id( string $room ): ?int {
'post_status' => 'publish',
'name' => $room_hash,
'fields' => 'ids',
'orderby' => 'ID',
'order' => 'ASC',
)
);

Expand Down Expand Up @@ -212,6 +281,8 @@ public function get_update_count( string $room ): int {
*
* @since 7.0.0
*
* @global wpdb $wpdb WordPress database abstraction object.
*
* @param string $room Room identifier.
* @param int $cursor Return updates after this cursor (meta_id).
* @return array<int, mixed> Sync updates.
Expand Down Expand Up @@ -261,7 +332,10 @@ public function get_updates_after_cursor( string $room, int $cursor ): array {

$updates = array();
foreach ( $rows as $row ) {
$updates[] = maybe_unserialize( $row->meta_value );
$decoded = json_decode( $row->meta_value, true );
if ( null !== $decoded ) {
$updates[] = $decoded;
}
}

return $updates;
Expand All @@ -272,6 +346,8 @@ public function get_updates_after_cursor( string $room, int $cursor ): array {
*
* @since 7.0.0
*
* @global wpdb $wpdb WordPress database abstraction object.
*
* @param string $room Room identifier.
* @param int $cursor Remove updates with meta_id < this cursor.
* @return bool True on success, false on failure.
Expand Down
Loading
Loading