Skip to content

fix(sync): Break the sync loop that can overload a server (#1301) #1328

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

Merged
gabrielcld2 merged 3 commits into from
Oct 21, 2026
Merged

fix(sync): Break the sync loop that can overload a server (#1301) #1328

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
d017a48
fix(sync): Break the sync loop that can overload a server (#1301)
gabriel-detassigny Oct 19, 2026
ff83557
fix(sync): address PR review feedback on the loop guards and CAS retry
gabriel-detassigny Oct 20, 2026
9153887
fix(sync): don't count an intentional sync delay as a loop strike
gabriel-detassigny Oct 21, 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
86 changes: 79 additions & 7 deletions php/class-media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2787,15 +2787,63 @@ public function build_cached_meta( $post_id, $key, $single ) {
*/
public function update_post_meta( $post_id, $key, $data ) {

$meta = $this->get_post_meta( $post_id );
if ( ! isset( $meta[ $key ] ) ) {
$meta[ $key ] = '';
}
// The `_cloudinary` meta is a single serialized array shared by every sync type, so
// this is a read-modify-write: fetch the whole thing, change one key, write the whole
// thing back. Two of these running concurrently for the same attachment (e.g. a
// bulk-sync thread and an autosync thread both reaching it, see GitHub issue #1301) can
// race -- whichever writes last overwrites the other's change outright, silently. Use
// WP's compare-and-swap (the $prev_value arg) to detect that and retry against the
// now-current value instead of clobbering it.
$max_attempts = 3;
for ( $attempt = 0; $attempt < $max_attempts; $attempt++ ) {
if ( $attempt > 0 ) {
// Only before a retry, not the first attempt: the compare-and-swap below
// ($prev_value) is what catches a stale read, by failing against the DB's actual
// current value regardless of what's cached. Forcing a fresh read up front would
// cost every single meta write here -- and there are many per sync -- a full
// reload for no benefit, since nothing has failed yet to justify distrusting the
// cache.
wp_cache_delete( $post_id, 'post_meta' );
}

if ( $meta[ $key ] !== $data ) {
// Read before checking existence: when the row is missing, get_post_meta() below
// runs the `cloudinary_migrate_legacy_meta` filter, whose callbacks can create this
// row themselves as a side effect (e.g. legacy-plugin migration). Checking existence
// first would miss that and cost an extra, avoidable retry.
$meta = $this->get_post_meta( $post_id );

@utkarshcloudinary utkarshcloudinary Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: $exists is read before get_post_meta(). When the row is missing, get_post_meta() applies cloudinary_migrate_legacy_meta. Upgrade::migrate_legacy_meta() can write _cloudinary itself (lines 229 and 259). In that case, add_post_meta( ..., true ) fails and the first attempt is lost. The retry recovers, but if you swap the two lines, this extra attempt does not occur.

$exists = metadata_exists( 'post', $post_id, Sync::META_KEYS['cloudinary'] );
if ( isset( $meta[ $key ] ) && $meta[ $key ] === $data ) {
return true; // Already set to this value, including by a racing write.
}
$prev = $meta;
$meta[ $key ] = $data;

if ( $exists ) {
// Note: update_metadata() only applies $prev_value (and so only detects a race)
// when it's non-empty. If `_cloudinary` currently holds an empty array (e.g.
// right after meta_cleanup) or something that didn't come back as an array, this
// falls through to an unconditional write instead -- still correct, just unable
// to tell a race apart from an ordinary write in that specific case.
$result = update_post_meta( $post_id, Sync::META_KEYS['cloudinary'], $meta, $prev );

@utkarshcloudinary utkarshcloudinary Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: The CAS is disabled when $prev is empty. update_metadata() does if ( ! empty( $prev_value ) ) before it adds $prev_value to the WHERE. This occurs when the _cloudinary row exists but contains '' or an empty array, for example after meta_cleanup or delete_post_meta() removes the last key. In that case, the write is unconditional again. Also, get_post_meta() returns (array) $meta. If the stored value is not an array, $prev never matches the DB value, so all 3 attempts fail and the code uses the unconditional fallback. These are edge cases, but a short comment would help.

} else {
// No row yet: add it uniquely so a concurrent first write can't be silently
// dropped the way an unconditional update_post_meta() would allow.
$result = add_post_meta( $post_id, Sync::META_KEYS['cloudinary'], $meta, true );
}

if ( false !== $result ) {
return $result;
}
// Lost the race to a concurrent write; loop and retry against the new state.
}

// Still contended after retries: write unconditionally rather than looping forever.
// This can still lose a concurrent change, but only after genuinely sustained
// contention, not on the first overlap.
wp_cache_delete( $post_id, 'post_meta' );
$meta = $this->get_post_meta( $post_id );
$meta[ $key ] = $data;

return update_post_meta( $post_id, Sync::META_KEYS['cloudinary'], $meta );
}

Expand All @@ -2809,11 +2857,35 @@ public function update_post_meta( $post_id, $key, $data ) {
*/
public function delete_post_meta( $post_id, $key ) {

$meta = $this->get_post_meta( $post_id );
if ( isset( $meta[ $key ] ) ) {
// Same shared-blob read-modify-write as update_post_meta() above, so apply the same
// compare-and-swap retry against concurrent writers.
$max_attempts = 3;
for ( $attempt = 0; $attempt < $max_attempts; $attempt++ ) {
if ( $attempt > 0 ) {
// See the matching note in update_post_meta(): only before a retry, once the
// compare-and-swap below has actually failed against the DB's current value.
wp_cache_delete( $post_id, 'post_meta' );
}

$meta = $this->get_post_meta( $post_id );
if ( ! isset( $meta[ $key ] ) ) {
return true; // Already gone, including by a racing write.
}
$prev = $meta;
unset( $meta[ $key ] );

$result = update_post_meta( $post_id, Sync::META_KEYS['cloudinary'], $meta, $prev );
if ( false !== $result ) {
return $result;
}
// Lost the race to a concurrent write; loop and retry against the new state.
}

// Still contended after retries: write unconditionally rather than looping forever.
wp_cache_delete( $post_id, 'post_meta' );
$meta = $this->get_post_meta( $post_id );
unset( $meta[ $key ] );

return update_post_meta( $post_id, Sync::META_KEYS['cloudinary'], $meta );
}

Expand Down
79 changes: 64 additions & 15 deletions php/class-sync.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class Sync implements Setup, Assets {
'folder_sync' => '_folder_sync',
'last_oversize_check' => '_last_oversize_check',
'local_size' => '_cld_local_size',
'loop_count' => '_cld_loop_count',
'pending' => '_cloudinary_pending',
'plugin_version' => '_plugin_version',
'process_log' => '_cloudinary_process_log',
Expand Down Expand Up @@ -905,23 +906,71 @@ function ( $item, $key ) use ( $required_signature ) {
* @return string|null
*/
public function validate_sync_type( $type, $attachment_id ) {
// Validate that this sync type applied (for optional types like upgrade).
if ( false === $this->run_sync_method( $type, 'validate', $attachment_id ) ) {
// If invalid, save the new signature.
$this->set_signature_item( $attachment_id, $type );

$type = $this->get_sync_type( $attachment_id, false ); // Set cache to false to get the new signature.

// Check if this is a realtime process.
} elseif ( ! empty( $this->sync_base_struct[ $type ]['realtime'] ) ) {
$result = $this->run_sync_method( $type, 'sync', $attachment_id );
if ( ! empty( $result ) ) {
$this->log_sync_result( $attachment_id, $type, $result );
// Both branches below save a signature and then re-check get_sync_type() for what's
// next. If that save doesn't stick -- e.g. a concurrent request overwrites it before
// it's read back, see GitHub issue #1301 -- get_sync_type() keeps handing back this
// same $type and this method recurses on it forever, pinning a PHP worker and hammering
// the DB. Guard against that by tracking how deep the current, still-unresolved
// recursion chain for this attachment+type is -- NOT how many times this method has
// been entered overall. A long request can legitimately call this dozens of times for
// the same attachment+type without ever recursing (e.g. rendering sync status for many
// references to the same attachment); each of those returns before the next begins, so
// the depth count must drop back to zero between them and never accumulate.
static $depth = array();
$key = $attachment_id . ':' . $type;
$depth[ $key ] = isset( $depth[ $key ] ) ? $depth[ $key ] + 1 : 1;

try {
/**
* Filter the number of times a single sync type may recurse for the same attachment
* before it's treated as stuck in a loop.
*
* @hook cloudinary_sync_type_retry_limit
*
* @param int $limit The retry limit. Default 50.
* @param string $type The sync type being validated.
* @param int $attachment_id The attachment ID.
*
* @return int
*/
$limit = apply_filters( 'cloudinary_sync_type_retry_limit', 50, $type, $attachment_id );
if ( $depth[ $key ] > $limit ) {
update_post_meta(
$attachment_id,
self::META_KEYS['sync_error'],
sprintf(
/* translators: %s is the sync type that would not advance. */
__( 'Sync loop detected: the "%s" sync state could not be saved.', 'cloudinary' ),
$type
)
);

return null;
}
$type = $this->get_sync_type( $attachment_id, false ); // Set cache to false to get the new signature.
}

return $type;
// Validate that this sync type applied (for optional types like upgrade).
if ( false === $this->run_sync_method( $type, 'validate', $attachment_id ) ) {
// If invalid, save the new signature.
$this->set_signature_item( $attachment_id, $type );

$type = $this->get_sync_type( $attachment_id, false ); // Set cache to false to get the new signature.

// Check if this is a realtime process.
} elseif ( ! empty( $this->sync_base_struct[ $type ]['realtime'] ) ) {
$result = $this->run_sync_method( $type, 'sync', $attachment_id );
if ( ! empty( $result ) ) {
$this->log_sync_result( $attachment_id, $type, $result );
}
$type = $this->get_sync_type( $attachment_id, false ); // Set cache to false to get the new signature.
}

return $type;
} finally {
--$depth[ $key ];
if ( 0 >= $depth[ $key ] ) {
unset( $depth[ $key ] );
}
}
}

/**
Expand Down
53 changes: 52 additions & 1 deletion php/sync/class-push-sync.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,57 @@ public function process_assets( $attachments = array() ) {
// Flag attachment as being processed.
update_post_meta( $attachment_id, Sync::META_KEYS['syncing'], time() );
$stat[ $attachment_id ] = array();
$looped = false;
while ( $type = $this->sync->get_sync_type( $attachment_id, false ) ) { // phpcs:ignore WordPress.CodeAnalysis.AssignmentInCondition
// translators: variable is sync type.
$action_message = sprintf( __( 'Sync type: %s', 'cloudinary' ), $type );
do_action( '_cloudinary_queue_action', $action_message, $thread );
if ( isset( $stat[ $attachment_id ][ $type ] ) ) {
// Loop prevention.
// Loop prevention: the same sync type came up twice for this attachment in
// one pass, so its state isn't advancing. This can happen for a transient
// reason (e.g. a 'sync' callback that returns early without an error), so
// don't jump straight to a permanent error -- that would require a manual
// "clean up errors" step to recover from, where the attachment previously
// would have just been picked up again on the next run. Instead, count
// consecutive passes that ended this way across requests and only flag it
// once that's happened a few times in a row. See GitHub issue #1301.
$looped = true;

if ( $this->sync->can_sync( $attachment_id, $type ) ) {
$loop_count = (int) get_post_meta( $attachment_id, Sync::META_KEYS['loop_count'], true ) + 1;

/**
* Filter how many consecutive passes may end in a repeated, non-advancing
* sync type for the same attachment before it's flagged with a sync error.
*
* @hook cloudinary_sync_loop_strikes
*
* @param int $strikes The number of strikes allowed. Default 3.
* @param string $type The sync type that repeated.
* @param int $attachment_id The attachment ID.
*
* @return int
*/
$loop_limit = apply_filters( 'cloudinary_sync_loop_strikes', 3, $type, $attachment_id );
if ( $loop_count >= $loop_limit ) {
update_post_meta(
$attachment_id,
Sync::META_KEYS['sync_error'],
sprintf(
/* translators: %s is the sync type that would not advance. */
__( 'Sync loop detected: the "%s" sync state is not advancing.', 'cloudinary' ),
$type
)
);
delete_post_meta( $attachment_id, Sync::META_KEYS['loop_count'] );
} else {
update_post_meta( $attachment_id, Sync::META_KEYS['loop_count'], $loop_count );
}
}
// Else: can_sync() says this type isn't meant to run right now -- e.g.
// Storage's own delay before offloading to "Cloudinary only" storage -- so
// the repeat is an intentional wait, not evidence of being stuck. Don't count
// a strike for it; just stop this pass the same way the guard always has.
break;
}
$stat[ $attachment_id ][ $type ] = true;
Expand All @@ -288,6 +333,12 @@ public function process_assets( $attachments = array() ) {
}
}

if ( ! $looped ) {
// A full, successful pass: this attachment isn't stuck, so clear any count a
// previous, transient loop detection may have left behind.
delete_post_meta( $attachment_id, Sync::META_KEYS['loop_count'] );
}

Utils::clean_up_sync_meta( $attachment_id );
}

Expand Down
Loading
Loading