[Real-time collaboration] Fix race condition in compaction#75835
[Real-time collaboration] Fix race condition in compaction#75835maxschmeling wants to merge 3 commits intotrunkfrom
Conversation
maxschmeling
added
[Type] Bug
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the real-time collaboration sync provider's compaction mechanism. The original "delete-all-then-re-add" approach had a critical race condition window where concurrent operations could lose messages or incorrectly report no messages.
Changes:
- Switched from deleting all updates and re-adding newer ones to a targeted deletion approach that only removes updates older than the cursor
- Eliminated the race condition by avoiding the complete deletion of all sync updates
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This avoids removing all, and ensures that only the targeted updates are deleted. | ||
| */ | ||
| if ( ! delete_post_meta( $post_id, self::SYNC_UPDATE_META_KEY, $envelope ) ) { | ||
| // stop deleting and return false if an update fails to delete |
There was a problem hiding this comment.
Comment should start with a capital letter to match the style convention used throughout this file. All other inline comments in this file begin with a capital letter.
| // stop deleting and return false if an update fails to delete | |
| // Stop deleting and return false if an update fails to delete |
| foreach ( $all_updates as $envelope ) { | ||
| if ( $add_result && $envelope['timestamp'] >= $cursor ) { | ||
| $add_result = (bool) add_post_meta( $post_id, self::SYNC_UPDATE_META_KEY, $envelope, false ); | ||
| if ( isset( $envelope['timestamp'] ) && $envelope['timestamp'] < $cursor ) { |
There was a problem hiding this comment.
The check for isset($envelope['timestamp']) is redundant. The get_all_updates() method called at line 308 already filters out any updates that don't have a timestamp field (see lines 116-121), ensuring all returned envelopes have both 'timestamp' and 'value' keys with timestamp being an integer. This check can be simplified to just $envelope['timestamp'] < $cursor.
| if ( isset( $envelope['timestamp'] ) && $envelope['timestamp'] < $cursor ) { | |
| if ( $envelope['timestamp'] < $cursor ) { |
| * Use delete_post_meta with serialized value to ensure only this specific update is removed. | ||
| * This avoids removing all, and ensures that only the targeted updates are deleted. | ||
| */ | ||
| if ( ! delete_post_meta( $post_id, self::SYNC_UPDATE_META_KEY, $envelope ) ) { |
There was a problem hiding this comment.
Passing $meta_value results in a meta_value query, which is not performant since that column is not indexed. (On some hosts the first n characters are indexed, but these values will likely exceed reasonable values of n.)
Perhaps we do need to collect and pass meta_ids.
| } | ||
|
|
||
| return $add_result; | ||
| return true; |
There was a problem hiding this comment.
This is still not failing if delete_post_meta fails
|
Nice, I found this issue separately and started working on an idea to build on top of @westonruter PR here. The idea is to use meta_id for ordering, and scope delete requests to the rows lower than the max meta_id was at the time of the fetch. I have a branch in progress here that shows the concept: https://github.com/WordPress/wordpress-develop/compare/trunk...mindctrl:wordpress-develop:try/post-meta-refactor?expand=1. There's also a race condition in |
What?
This fixes a race condition with the default sync provider compaction.
Why?
The original approach of deleting all post meta and adding back newer updates resulted in a couple different potential race conditions where messages could get completely lost or incorrectly show no messages by mistake.
How?
This change just switches from a "delete all then re-add" strategy to a "targeted deletion" approach. With post meta we can't delete in one
DELETEquery, so we have to calldelete_post_metafor every entry. This is not efficient, but is much safer.Testing Instructions
Testing the race condition directly is more difficult.