From d017a4846fe455768574df007fa01dd956c4bee3 Mon Sep 17 00:00:00 2001
From: Gabriel de Tassigny <gabriel.detassigny@xwp.co>
Date: Mon, 19 Oct 2026 13:53:24 +0200
Subject: [PATCH 1/3] fix(sync): Break the sync loop that can overload a server
 (#1301)

Two independent fixes for a scenario where an attachment's sync
state stops advancing and the plugin ends up repeatedly reprocessing
it, overloading the server with internal /cloudinary/v1/queue
requests:

1. Sync::validate_sync_type() recurses into get_sync_type() after
   saving a signature. If that save doesn't stick, the recursion
   never terminates, pinning a PHP worker indefinitely. Add a bounded
   retry counter (filterable via cloudinary_sync_type_retry_limit)
   that flags a sync error once a type clearly isn't resolving,
   instead of recursing forever.

2. Push_Sync::process_assets() already had an outer loop guard for
   when the same sync type repeats in one pass, but it only broke the
   loop without flagging the attachment. That left it unsynced,
   unqueued and un-errored, so the next build_queue() run picked it
   straight back up, repeating the same stuck request. Flag a sync
   error there too, so build_queue()'s own `_cld_error NOT EXISTS`
   check excludes it afterwards.

Root cause: Media::update_post_meta() / delete_post_meta() do a
non-atomic read-modify-write of the whole `_cloudinary` meta blob.
Two of these racing for the same attachment -- e.g. a bulk-sync
thread and an autosync thread both reaching it, which the existing
add_to_thread_queue() "steal" logic allows -- can silently lose one
side's write entirely, with nothing logged. Reproduced locally with
two concurrent writers and no filter needed. Harden both methods with
a compare-and-swap retry (via update_post_meta()'s $prev_value arg),
invalidating the primed post-meta cache between attempts so a retry
actually sees a concurrent write instead of replaying a stale read.

Covered by three new PHPUnit suites: the retry-limit circuit breaker,
the outer loop guard flagging + excluding the attachment from the
next queue build, and the meta race (on first creation, on an
existing row, and for delete_post_meta()).
---
 php/class-media.php                           |  73 ++++++-
 php/class-sync.php                            |  37 ++++
 php/sync/class-push-sync.php                  |  10 +-
 .../tests/test-media-concurrent-meta.php      | 179 ++++++++++++++++++
 .../tests/test-push-sync-outer-loop-guard.php | 152 +++++++++++++++
 tests/phpunit/tests/test-sync-loop-guard.php  | 169 +++++++++++++++++
 6 files changed, 611 insertions(+), 9 deletions(-)
 create mode 100644 tests/phpunit/tests/test-media-concurrent-meta.php
 create mode 100644 tests/phpunit/tests/test-push-sync-outer-loop-guard.php
 create mode 100644 tests/phpunit/tests/test-sync-loop-guard.php

diff --git a/php/class-media.php b/php/class-media.php
index cf4a3370b..03fcc4ac8 100644
--- a/php/class-media.php
+++ b/php/class-media.php
@@ -2787,15 +2787,50 @@ 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 ] = '';
-		}
-
-		if ( $meta[ $key ] !== $data ) {
+		// 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++ ) {
+			// WP primes and caches ALL of a post's meta on first access, so a plain re-read
+			// here would just replay this same process's first (possibly already stale) read on
+			// every retry, never seeing what a concurrent process wrote in the meantime. Drop it
+			// so each attempt re-queries the DB.
+			wp_cache_delete( $post_id, 'post_meta' );
+
+			$exists = metadata_exists( 'post', $post_id, Sync::META_KEYS['cloudinary'] );
+			$meta   = $this->get_post_meta( $post_id );
+			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 ) {
+				$result = update_post_meta( $post_id, Sync::META_KEYS['cloudinary'], $meta, $prev );
+			} 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 );
 	}
 
@@ -2809,11 +2844,33 @@ 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++ ) {
+			// See the matching note in update_post_meta(): drop the cached meta so this
+			// actually re-reads the DB instead of replaying an earlier, possibly stale read.
+			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 );
 	}
 
diff --git a/php/class-sync.php b/php/class-sync.php
index 30e73015d..878736c98 100644
--- a/php/class-sync.php
+++ b/php/class-sync.php
@@ -905,6 +905,43 @@ function ( $item, $key ) use ( $required_signature ) {
 	 * @return string|null
 	 */
 	public function validate_sync_type( $type, $attachment_id ) {
+		// 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. Count re-entries per attachment+type and bail out to a sync error once it's
+		// clearly not resolving, rather than let the recursion run unbounded.
+		static $attempts  = array();
+		$key              = $attachment_id . ':' . $type;
+		$attempts[ $key ] = isset( $attempts[ $key ] ) ? $attempts[ $key ] + 1 : 1;
+
+		/**
+		 * Filter the number of times a single sync type may be re-validated for the same
+		 * attachment within one request 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 ( $attempts[ $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;
+		}
+
 		// 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.
diff --git a/php/sync/class-push-sync.php b/php/sync/class-push-sync.php
index b24761e71..f4d4b19a7 100644
--- a/php/sync/class-push-sync.php
+++ b/php/sync/class-push-sync.php
@@ -278,7 +278,15 @@ public function process_assets( $attachments = array() ) {
 				$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. Flag it instead of leaving it
+					// unsynced, unqueued and un-errored -- which would just queue it right back
+					// up on the next build_queue() run. See GitHub issue #1301.
+					update_post_meta(
+						$attachment_id,
+						Sync::META_KEYS['sync_error'],
+						__( 'Sync loop detected: sync state is not advancing.', 'cloudinary' )
+					);
 					break;
 				}
 				$stat[ $attachment_id ][ $type ] = true;
diff --git a/tests/phpunit/tests/test-media-concurrent-meta.php b/tests/phpunit/tests/test-media-concurrent-meta.php
new file mode 100644
index 000000000..06427a877
--- /dev/null
+++ b/tests/phpunit/tests/test-media-concurrent-meta.php
@@ -0,0 +1,179 @@
+<?php
+/**
+ * Tests for Cloudinary\Media::update_post_meta() / delete_post_meta().
+ *
+ * These read the whole `_cloudinary` meta blob, change one key, and write the whole blob back.
+ * Two of these running concurrently for the same attachment -- e.g. a bulk-sync thread and an
+ * autosync thread both reaching it -- can race: whichever finishes writing last overwrites the
+ * other's change outright, with nothing logged. That silently stalled signature is what GitHub
+ * issue #1301 reported, so these confirm the compare-and-swap retry actually merges a concurrent
+ * write instead of clobbering it.
+ *
+ * @package Cloudinary
+ */
+
+use Cloudinary\Media;
+
+/**
+ * Covers the concurrency handling in Media::update_post_meta() and delete_post_meta().
+ */
+class Test_Media_Concurrent_Meta extends WP_UnitTestCase {
+
+	/**
+	 * A real attachment to write `_cloudinary` meta against.
+	 *
+	 * @var int
+	 */
+	protected static $attachment_id;
+
+	/**
+	 * Create a real attachment once for all tests.
+	 *
+	 * @param WP_UnitTest_Factory $factory The test factory.
+	 *
+	 * @return void
+	 */
+	public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
+		self::$attachment_id = $factory->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
+	}
+
+	/**
+	 * Start each test with no `_cloudinary` meta, so "the row doesn't exist yet" and "the row
+	 * already exists" are both exercised from a clean, known state.
+	 *
+	 * @return void
+	 */
+	public function set_up() {
+		parent::set_up();
+		delete_post_meta( self::$attachment_id, '_cloudinary' );
+	}
+
+	/**
+	 * @return Media
+	 */
+	protected function get_media() {
+		return \Cloudinary\get_plugin_instance()->get_component( 'media' );
+	}
+
+	/**
+	 * Simulates a second process's write landing in the DB in between this call's read and its
+	 * own write, by performing it from inside a filter that fires at exactly that point.
+	 *
+	 * @param string $hook          The filter to inject the concurrent write at.
+	 * @param string $concurrent_key The meta sub-key the "other process" writes.
+	 * @param mixed  $concurrent_val The value the "other process" writes.
+	 *
+	 * @return void
+	 */
+	protected function inject_concurrent_write( $hook, $concurrent_key, $concurrent_val ) {
+		$attachment_id = self::$attachment_id;
+		$did_run       = false;
+
+		add_filter(
+			$hook,
+			function ( $check, $object_id, $meta_key ) use ( $attachment_id, $concurrent_key, $concurrent_val, &$did_run ) {
+				if ( ! $did_run && $object_id === $attachment_id && '_cloudinary' === $meta_key ) {
+					$did_run = true;
+					wp_cache_delete( $attachment_id, 'post_meta' );
+					$meta                     = get_post_meta( $attachment_id, '_cloudinary', true );
+					$meta                     = is_array( $meta ) ? $meta : array();
+					$meta[ $concurrent_key ]  = $concurrent_val;
+					if ( metadata_exists( 'post', $attachment_id, '_cloudinary' ) ) {
+						update_post_meta( $attachment_id, '_cloudinary', $meta );
+					} else {
+						add_post_meta( $attachment_id, '_cloudinary', $meta, true );
+					}
+				}
+
+				return $check;
+			},
+			10,
+			3
+		);
+	}
+
+	/**
+	 * Writing a brand new key, with no concurrent writer, behaves exactly as before.
+	 *
+	 * @return void
+	 */
+	public function test_writes_normally_with_no_concurrent_writer() {
+		$media = $this->get_media();
+
+		$this->assertNotFalse( $media->update_post_meta( self::$attachment_id, 'public_id', 'solo-value' ) );
+		$this->assertSame( 'solo-value', $media->get_post_meta( self::$attachment_id, 'public_id', true ) );
+	}
+
+	/**
+	 * The race this issue reports: the `_cloudinary` row doesn't exist yet, and a concurrent
+	 * process creates it (with a different key) in the gap between this call's read and its
+	 * write. Before the fix, the write landed via a plain update_post_meta() with no compare-and-
+	 * swap, so it was free to overwrite the concurrent row outright. It should instead retry and
+	 * merge, so both keys survive.
+	 *
+	 * @return void
+	 */
+	public function test_merges_a_concurrent_write_on_first_creation() {
+		$this->inject_concurrent_write( 'add_post_metadata', 'version', 'from-concurrent-writer' );
+
+		$media = $this->get_media();
+		$media->update_post_meta( self::$attachment_id, 'public_id', 'from-this-writer' );
+
+		$meta = $media->get_post_meta( self::$attachment_id );
+		$this->assertSame( 'from-this-writer', $meta['public_id'] );
+		$this->assertSame( 'from-concurrent-writer', $meta['version'] );
+	}
+
+	/**
+	 * Same race, but against an attachment that's already partway synced, so the `_cloudinary`
+	 * row already exists and the write goes through update_post_meta()'s compare-and-swap
+	 * ($prev_value) path instead of add_post_meta()'s.
+	 *
+	 * @return void
+	 */
+	public function test_merges_a_concurrent_write_on_an_existing_row() {
+		$media = $this->get_media();
+		$media->update_post_meta( self::$attachment_id, 'folder_sync', true );
+
+		$this->inject_concurrent_write( 'update_post_metadata', 'remote_size', '1024' );
+
+		$media->update_post_meta( self::$attachment_id, 'public_id', 'from-this-writer' );
+
+		$meta = $media->get_post_meta( self::$attachment_id );
+		$this->assertSame( 'from-this-writer', $meta['public_id'] );
+		$this->assertSame( '1024', $meta['remote_size'] );
+		$this->assertTrue( $meta['folder_sync'] );
+	}
+
+	/**
+	 * delete_post_meta() reads, removes the key and writes the same shared blob back, so it has
+	 * the identical race -- confirm it also merges rather than clobbering.
+	 *
+	 * @return void
+	 */
+	public function test_delete_merges_a_concurrent_write() {
+		$media = $this->get_media();
+		$media->update_post_meta( self::$attachment_id, 'to_delete', 'gone-soon' );
+
+		$this->inject_concurrent_write( 'update_post_metadata', 'remote_size', '2048' );
+
+		$media->delete_post_meta( self::$attachment_id, 'to_delete' );
+
+		$meta = $media->get_post_meta( self::$attachment_id );
+		$this->assertArrayNotHasKey( 'to_delete', $meta );
+		$this->assertSame( '2048', $meta['remote_size'] );
+	}
+
+	/**
+	 * Writing the same value that a concurrent writer just set is a no-op, not a conflict --
+	 * confirms the early-exit path doesn't needlessly retry or error.
+	 *
+	 * @return void
+	 */
+	public function test_no_retry_needed_when_value_already_matches() {
+		$media = $this->get_media();
+		$media->update_post_meta( self::$attachment_id, 'public_id', 'already-set' );
+
+		$this->assertTrue( $media->update_post_meta( self::$attachment_id, 'public_id', 'already-set' ) );
+	}
+}
diff --git a/tests/phpunit/tests/test-push-sync-outer-loop-guard.php b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
new file mode 100644
index 000000000..ab3d7fdb1
--- /dev/null
+++ b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
@@ -0,0 +1,152 @@
+<?php
+/**
+ * Tests for the outer loop guard in Cloudinary\Sync\Push_Sync::process_assets().
+ *
+ * process_assets() loops over whatever sync type get_sync_type() hands back and runs it. If the
+ * same type comes back a second time in that same pass, its state isn't advancing, so the loop
+ * breaks. Before this fix that break left the attachment unsynced, unqueued and un-errored --
+ * build_queue() would just pick it straight back up on the next run, repeating the same stuck
+ * request forever (GitHub issue #1301). These confirm the break now also flags a sync error, so
+ * build_queue()'s own `_cld_error NOT EXISTS` check excludes it afterwards.
+ *
+ * @package Cloudinary
+ */
+
+use Cloudinary\Connect;
+use Cloudinary\Sync;
+
+/**
+ * Covers the loop-prevention branch in Push_Sync::process_assets().
+ */
+class Test_Push_Sync_Outer_Loop_Guard extends WP_UnitTestCase {
+
+	/**
+	 * A real, deliverable attachment to run process_assets() against.
+	 *
+	 * @var int
+	 */
+	protected static $attachment_id;
+
+	/**
+	 * @param WP_UnitTest_Factory $factory The test factory.
+	 *
+	 * @return void
+	 */
+	public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
+		self::$attachment_id = $factory->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
+	}
+
+	/**
+	 * register_sync_type() writes directly into the Sync component singleton, which outlives any
+	 * one test, and nothing in the plugin ever needs to remove a registered type -- so the lowest-
+	 * priority 'test_stuck_type' registered below would otherwise leak into every other test in
+	 * the same PHPUnit run (and always win, being the lowest priority), including Test_Sync_Loop_
+	 * Guard's. Reflection is the only way at sync_base_struct from the outside to undo it.
+	 *
+	 * @return void
+	 */
+	public function tear_down() {
+		$sync       = \Cloudinary\get_plugin_instance()->get_component( 'sync' );
+		$reflection = new ReflectionProperty( $sync, 'sync_base_struct' );
+		$reflection->setAccessible( true );
+		$struct = $reflection->getValue( $sync );
+		unset( $struct['test_stuck_type'] );
+		$reflection->setValue( $sync, $struct );
+		$sync->setup_sync_types();
+
+		parent::tear_down();
+	}
+
+	/**
+	 * Registers a sync type that never resolves -- its 'sync' callback runs but never saves a
+	 * matching signature -- so get_sync_type() keeps handing it back. Given the lowest possible
+	 * priority, it is always the type process_assets() picks, regardless of every real type's own
+	 * state, isolating the outer loop's own behaviour from the rest of the sync system (which is
+	 * covered separately in Test_Sync_Loop_Guard).
+	 *
+	 * This also does the manager wiring that normally only happens once the site is "connected"
+	 * -- which the PHPUnit bootstrap never is -- so that process_assets() and build_queue() have
+	 * what they read directly ($managers['media'] etc.). It fakes that with a syntactically valid
+	 * but make-believe `cloudinary_url`, with the usage-stats transient pre-seeded so
+	 * Connect::setup()'s usage_stats() finds it non-empty and skips the one live API call it would
+	 * otherwise make; everything downstream (Media's, Sync's and Delivery's own setup()) then runs
+	 * as it would on a real connected site, without reaching the network itself.
+	 *
+	 * @return \Cloudinary\Sync
+	 */
+	protected function register_never_resolving_sync_type() {
+		$plugin  = \Cloudinary\get_plugin_instance();
+		$connect = $plugin->get_component( 'connect' );
+
+		$plugin->settings->set_param( 'connected', true );
+		set_transient( Connect::META_KEYS['usage'], array( 'placeholder' => true ), HOUR_IN_SECONDS );
+		$plugin->settings->set_value( 'cloudinary_url', 'cloudinary://123456789012345:abcdefghijklmnopqrstuvwxyzABCDEF@test-cloud' );
+		$connect->setup();
+
+		do_action( 'cloudinary_connected', $plugin ); // Delivery::init() etc.
+
+		$plugin->get_component( 'media' )->setup();
+		$plugin->get_component( 'sync' )->setup();
+		$plugin->get_component( 'delivery' )->setup();
+
+		$sync = $plugin->get_component( 'sync' );
+		$sync->register_assets();
+
+		$sync->register_sync_type(
+			'test_stuck_type',
+			array(
+				'asset_state' => 0,
+				'generate'    => '__return_true',
+				'validate'    => '__return_true',
+				'priority'    => -1,
+				'sync'        => '__return_null', // Runs, but never saves a signature.
+				'realtime'    => false,
+			)
+		);
+		$sync->setup_sync_types();
+
+		return $sync;
+	}
+
+	/**
+	 * @return void
+	 */
+	public function test_flags_a_sync_error_when_a_type_repeats() {
+		$this->register_never_resolving_sync_type();
+
+		$push = \Cloudinary\get_plugin_instance()->get_component( 'sync' )->managers['push'];
+		$stat = $push->process_assets( self::$attachment_id );
+
+		$error = get_post_meta( self::$attachment_id, Sync::META_KEYS['sync_error'], true );
+		$this->assertNotEmpty( $error, 'Loop prevention should flag a sync error.' );
+
+		$this->assertCount(
+			1,
+			$stat[ self::$attachment_id ],
+			'The loop should have broken after the type repeated once, not kept running.'
+		);
+	}
+
+	/**
+	 * Once errored, build_queue()'s own meta_query (`_cld_error` NOT EXISTS) is what keeps a
+	 * stuck attachment from being queued again -- confirm the flag this guard sets actually
+	 * satisfies that, end to end.
+	 *
+	 * @return void
+	 */
+	public function test_flagged_attachment_is_excluded_from_the_next_queue_build() {
+		$this->register_never_resolving_sync_type();
+
+		$sync = \Cloudinary\get_plugin_instance()->get_component( 'sync' );
+		$sync->managers['push']->process_assets( self::$attachment_id );
+
+		$queue = $sync->managers['queue'];
+		$queue->build_queue();
+
+		$this->assertSame(
+			0,
+			$queue->get_last_built_total(),
+			'The flagged attachment should not be re-queued.'
+		);
+	}
+}
diff --git a/tests/phpunit/tests/test-sync-loop-guard.php b/tests/phpunit/tests/test-sync-loop-guard.php
new file mode 100644
index 000000000..a343fb909
--- /dev/null
+++ b/tests/phpunit/tests/test-sync-loop-guard.php
@@ -0,0 +1,169 @@
+<?php
+/**
+ * Tests for the recursion guard in Cloudinary\Sync::validate_sync_type().
+ *
+ * get_sync_type() and validate_sync_type() recurse into each other: a sync type is run (or its
+ * signature saved), then get_sync_type() is re-checked for what's next. If that save doesn't
+ * stick -- e.g. a filter blocks the `_cloudinary` meta write, or a concurrent request overwrites
+ * it first, see GitHub issue #1301 -- get_sync_type() keeps handing back the same type and this
+ * recurses without end, pinning a PHP worker and hammering the DB with the same queue request
+ * over and over. These confirm it instead bails out to a sync error once a type clearly isn't
+ * resolving.
+ *
+ * @package Cloudinary
+ */
+
+use Cloudinary\Connect;
+use Cloudinary\Sync;
+
+/**
+ * Covers the retry-limit circuit breaker in Sync::validate_sync_type().
+ */
+class Test_Sync_Loop_Guard extends WP_UnitTestCase {
+
+	/**
+	 * A real, deliverable attachment -- 'delivery' is the first sync type checked for any
+	 * attachment, is realtime, and (unlike 'file') does no network calls, so it is exactly the
+	 * type the live incident got stuck recursing on.
+	 *
+	 * A fresh one per test, not a shared static: validate_sync_type()'s retry counter is a
+	 * function-static, so it persists for this whole PHPUnit process. Reusing one attachment
+	 * across test methods would let one test's count carry over into the next.
+	 *
+	 * @var int
+	 */
+	protected $attachment_id;
+
+	/**
+	 * @return void
+	 */
+	public function set_up() {
+		parent::set_up();
+		$this->attachment_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
+	}
+
+	/**
+	 * Sync-type registration (incl. 'delivery') and every component's manager wiring --
+	 * Sync::$managers['media'], Media::$sync, Delivery::$sync, and so on -- only happen once the
+	 * site is "connected". The PHPUnit bootstrap never connects to a real Cloudinary account, so
+	 * this fakes one: a syntactically valid but make-believe `cloudinary_url`, with the usage-
+	 * stats transient pre-seeded so Connect::setup()'s usage_stats() finds it non-empty and skips
+	 * the one live API call it would otherwise make. Everything downstream of that (Media's,
+	 * Sync's and Delivery's own setup()) then runs exactly as it would on a real connected site,
+	 * without reaching the network itself.
+	 *
+	 * @return Sync
+	 */
+	protected function get_sync() {
+		$plugin  = \Cloudinary\get_plugin_instance();
+		$connect = $plugin->get_component( 'connect' );
+
+		$plugin->settings->set_param( 'connected', true );
+		set_transient( Connect::META_KEYS['usage'], array( 'placeholder' => true ), HOUR_IN_SECONDS );
+		$plugin->settings->set_value( 'cloudinary_url', 'cloudinary://123456789012345:abcdefghijklmnopqrstuvwxyzABCDEF@test-cloud' );
+		$connect->setup();
+
+		do_action( 'cloudinary_connected', $plugin ); // Delivery::init() etc.
+
+		$plugin->get_component( 'media' )->setup();
+		$plugin->get_component( 'sync' )->setup();
+		$plugin->get_component( 'delivery' )->setup();
+
+		$sync = $plugin->get_component( 'sync' );
+		$sync->register_assets(); // Populates sync_base_struct / sync_types (incl. 'delivery').
+
+		return $sync;
+	}
+
+	/**
+	 * Blocks every write to `_cloudinary` meta for this attachment, the same way the issue's own
+	 * reproduction filter does, so no sync type's signature can ever be saved.
+	 *
+	 * @return void
+	 */
+	protected function block_cloudinary_meta_writes() {
+		$attachment_id = $this->attachment_id;
+
+		add_filter(
+			'update_post_metadata',
+			function ( $check, $object_id, $meta_key ) use ( $attachment_id ) {
+				if ( $object_id === $attachment_id && '_cloudinary' === $meta_key ) {
+					return true; // Pretend the write succeeded; don't actually let it through.
+				}
+
+				return $check;
+			},
+			10,
+			3
+		);
+		add_filter(
+			'add_post_metadata',
+			function ( $check, $object_id, $meta_key ) use ( $attachment_id ) {
+				if ( $object_id === $attachment_id && '_cloudinary' === $meta_key ) {
+					return true;
+				}
+
+				return $check;
+			},
+			10,
+			3
+		);
+	}
+
+	/**
+	 * With a low retry limit and the write blocked, get_sync_type() must return (not recurse
+	 * forever) and the attachment must come out flagged with a sync error naming the stuck type.
+	 *
+	 * @return void
+	 */
+	public function test_stops_and_flags_a_sync_error_once_the_retry_limit_is_hit() {
+		$this->block_cloudinary_meta_writes();
+		add_filter(
+			'cloudinary_sync_type_retry_limit',
+			function () {
+				return 5;
+			}
+		);
+
+		$sync = $this->get_sync();
+		$type = $sync->get_sync_type( $this->attachment_id, false );
+
+		$this->assertNull( $type, 'get_sync_type() should give up rather than recurse forever.' );
+
+		$error = get_post_meta( $this->attachment_id, Sync::META_KEYS['sync_error'], true );
+		$this->assertNotEmpty( $error );
+		$this->assertStringContainsString( 'delivery', $error );
+	}
+
+	/**
+	 * Once flagged, get_sync_type()'s own existing short-circuit for errored attachments must
+	 * take over on the next call, rather than this re-entering (and re-counting against) the
+	 * retry logic.
+	 *
+	 * @return void
+	 */
+	public function test_errored_attachment_short_circuits_on_the_next_call() {
+		$this->block_cloudinary_meta_writes();
+		add_filter( 'cloudinary_sync_type_retry_limit', fn() => 5 );
+
+		$sync = $this->get_sync();
+		$sync->get_sync_type( $this->attachment_id, false );
+
+		$this->assertNull( $sync->get_sync_type( $this->attachment_id, false ) );
+	}
+
+	/**
+	 * A healthy attachment -- no write ever blocked -- must resolve 'delivery' in well under the
+	 * retry limit, so the breaker can never be mistaken for normal operation.
+	 *
+	 * @return void
+	 */
+	public function test_does_not_trip_during_normal_resolution() {
+		add_filter( 'cloudinary_sync_type_retry_limit', fn() => 5 );
+
+		$sync = $this->get_sync();
+		$sync->get_sync_type( $this->attachment_id, false );
+
+		$this->assertEmpty( get_post_meta( $this->attachment_id, Sync::META_KEYS['sync_error'], true ) );
+	}
+}

From ff835575fd9a15beb736b7c56d9e7c662a5fde14 Mon Sep 17 00:00:00 2001
From: Gabriel de Tassigny <gabriel.detassigny@xwp.co>
Date: Tue, 20 Oct 2026 09:53:12 +0200
Subject: [PATCH 2/3] fix(sync): address PR review feedback on the loop guards
 and CAS retry

Addresses @utkarshcloudinary's review on #1328:

- blocker: Sync::validate_sync_type()'s retry counter counted every
  call for an attachment+type, not recursion depth, so a type that
  resolves immediately but is legitimately checked many times across
  a long request (status rendered for several references to the same
  attachment, media library pages, etc.) could falsely trip the
  breaker after 50 *total* calls. Switch to a depth counter that
  increments on entry and decrements on every return via `finally`,
  so only an actual, still-unresolved recursive chain accumulates.

- warning: Push_Sync::process_assets()'s outer loop guard flagged a
  permanent `_cld_error` the first time a type repeated in one pass,
  removing the self-healing the plugin had before this PR (the
  attachment would just get reprocessed on the next queue build). A
  single repeat can be transient. Track consecutive loop detections
  across passes in postmeta and only flag the error once that reaches
  a (filterable) limit, clearing it again on any pass that completes
  without repeating. Also names the stuck $type in the message, same
  as the other guard.

- warning: Media::update_post_meta()/delete_post_meta() dropped the
  primed post-meta cache on every attempt, including the first, where
  it isn't needed -- the compare-and-swap ($prev_value) already
  catches a stale read by failing against the DB's actual current
  value. Only invalidate the cache before a retry, once the CAS has
  actually failed.

- nit: reordered the existence check after the read in
  update_post_meta(), since a cold cache's `cloudinary_migrate_legacy_meta`
  filter can itself create the row as a side effect; checking existence
  first could miss that and cost an avoidable extra retry.

- nit: documented that update_metadata()'s CAS is skipped when $prev
  is empty (e.g. right after meta_cleanup) or not an array, so that
  specific transition falls through to an unconditional write.

Test changes:
- test-sync-loop-guard.php: added a regression test confirming 20
  independent, non-recursing calls for a stable pending type never
  trip the breaker (4x the test's own retry limit), matching the
  blocker above. Added matching sync_base_struct teardown so its
  custom type doesn't leak into other tests.
- test-push-sync-outer-loop-guard.php: updated for the 3-strike
  behaviour -- a single repeat no longer errors, the error only lands
  (naming the type) once the limit is reached, and a successful pass
  resets the strike count.

Re-verified manually against the live, connected wp-env instance:
the original issue's exact repro still resolves via the inner guard
in one pass; a genuine two-process race on a fresh attachment still
merges both writes; and a normal upload still fully syncs.
---
 php/class-media.php                           | 33 +++++--
 php/class-sync.php                            | 98 +++++++++++--------
 php/sync/class-push-sync.php                  | 52 ++++++++--
 .../tests/test-push-sync-outer-loop-guard.php | 90 +++++++++++++++--
 tests/phpunit/tests/test-sync-loop-guard.php  | 71 +++++++++++++-
 5 files changed, 274 insertions(+), 70 deletions(-)

diff --git a/php/class-media.php b/php/class-media.php
index 03fcc4ac8..cdfefb3f2 100644
--- a/php/class-media.php
+++ b/php/class-media.php
@@ -2796,14 +2796,22 @@ public function update_post_meta( $post_id, $key, $data ) {
 		// now-current value instead of clobbering it.
 		$max_attempts = 3;
 		for ( $attempt = 0; $attempt < $max_attempts; $attempt++ ) {
-			// WP primes and caches ALL of a post's meta on first access, so a plain re-read
-			// here would just replay this same process's first (possibly already stale) read on
-			// every retry, never seeing what a concurrent process wrote in the meantime. Drop it
-			// so each attempt re-queries the DB.
-			wp_cache_delete( $post_id, 'post_meta' );
+			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' );
+			}
 
-			$exists = metadata_exists( 'post', $post_id, Sync::META_KEYS['cloudinary'] );
+			// 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 );
+			$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.
 			}
@@ -2811,6 +2819,11 @@ public function update_post_meta( $post_id, $key, $data ) {
 			$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 );
 			} else {
 				// No row yet: add it uniquely so a concurrent first write can't be silently
@@ -2848,9 +2861,11 @@ public function delete_post_meta( $post_id, $key ) {
 		// compare-and-swap retry against concurrent writers.
 		$max_attempts = 3;
 		for ( $attempt = 0; $attempt < $max_attempts; $attempt++ ) {
-			// See the matching note in update_post_meta(): drop the cached meta so this
-			// actually re-reads the DB instead of replaying an earlier, possibly stale read.
-			wp_cache_delete( $post_id, 'post_meta' );
+			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 ] ) ) {
diff --git a/php/class-sync.php b/php/class-sync.php
index 878736c98..98e16008a 100644
--- a/php/class-sync.php
+++ b/php/class-sync.php
@@ -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',
@@ -909,56 +910,67 @@ public function validate_sync_type( $type, $attachment_id ) {
 		// 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. Count re-entries per attachment+type and bail out to a sync error once it's
-		// clearly not resolving, rather than let the recursion run unbounded.
-		static $attempts  = array();
-		$key              = $attachment_id . ':' . $type;
-		$attempts[ $key ] = isset( $attempts[ $key ] ) ? $attempts[ $key ] + 1 : 1;
+		// 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
+					)
+				);
 
-		/**
-		 * Filter the number of times a single sync type may be re-validated for the same
-		 * attachment within one request 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 ( $attempts[ $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;
+			}
 
-			return null;
-		}
+			// 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 );
 
-		// 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.
 
-			$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.
+			}
 
-			// 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 );
+			return $type;
+		} finally {
+			--$depth[ $key ];
+			if ( 0 >= $depth[ $key ] ) {
+				unset( $depth[ $key ] );
 			}
-			$type = $this->get_sync_type( $attachment_id, false ); // Set cache to false to get the new signature.
 		}
-
-		return $type;
 	}
 
 	/**
diff --git a/php/sync/class-push-sync.php b/php/sync/class-push-sync.php
index f4d4b19a7..f7ca50246 100644
--- a/php/sync/class-push-sync.php
+++ b/php/sync/class-push-sync.php
@@ -273,20 +273,50 @@ 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: the same sync type came up twice for this attachment in
-					// one pass, so its state isn't advancing. Flag it instead of leaving it
-					// unsynced, unqueued and un-errored -- which would just queue it right back
-					// up on the next build_queue() run. See GitHub issue #1301.
-					update_post_meta(
-						$attachment_id,
-						Sync::META_KEYS['sync_error'],
-						__( 'Sync loop detected: sync state is not advancing.', 'cloudinary' )
-					);
+					// 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;
+					$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 );
+					}
 					break;
 				}
 				$stat[ $attachment_id ][ $type ] = true;
@@ -296,6 +326,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 );
 		}
 
diff --git a/tests/phpunit/tests/test-push-sync-outer-loop-guard.php b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
index ab3d7fdb1..050dc678c 100644
--- a/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
+++ b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
@@ -6,8 +6,14 @@
  * same type comes back a second time in that same pass, its state isn't advancing, so the loop
  * breaks. Before this fix that break left the attachment unsynced, unqueued and un-errored --
  * build_queue() would just pick it straight back up on the next run, repeating the same stuck
- * request forever (GitHub issue #1301). These confirm the break now also flags a sync error, so
- * build_queue()'s own `_cld_error NOT EXISTS` check excludes it afterwards.
+ * request forever (GitHub issue #1301).
+ *
+ * A single repeat can be a transient hiccup rather than a genuinely stuck attachment, so the fix
+ * doesn't flag a sync error the first time: it counts consecutive passes that ended this way
+ * (persisted in postmeta, across requests) and only errors once that count reaches a limit,
+ * clearing it again the moment a pass completes without repeating. These cover both ends of that:
+ * early repeats that still self-heal, and a sync error once the limit is actually reached -- at
+ * which point build_queue()'s own `_cld_error NOT EXISTS` check excludes the attachment.
  *
  * @package Cloudinary
  */
@@ -109,17 +115,20 @@ protected function register_never_resolving_sync_type() {
 	}
 
 	/**
+	 * A single repeated pass is not enough to flag a permanent error: the loop still breaks (so
+	 * the request doesn't hang), but the attachment is left to be picked up again, the same as
+	 * before this fix, since one repeat alone doesn't prove it's genuinely stuck.
+	 *
 	 * @return void
 	 */
-	public function test_flags_a_sync_error_when_a_type_repeats() {
+	public function test_a_single_repeat_does_not_flag_a_sync_error() {
 		$this->register_never_resolving_sync_type();
 
 		$push = \Cloudinary\get_plugin_instance()->get_component( 'sync' )->managers['push'];
 		$stat = $push->process_assets( self::$attachment_id );
 
-		$error = get_post_meta( self::$attachment_id, Sync::META_KEYS['sync_error'], true );
-		$this->assertNotEmpty( $error, 'Loop prevention should flag a sync error.' );
-
+		$this->assertEmpty( get_post_meta( self::$attachment_id, Sync::META_KEYS['sync_error'], true ) );
+		$this->assertSame( '1', get_post_meta( self::$attachment_id, Sync::META_KEYS['loop_count'], true ) );
 		$this->assertCount(
 			1,
 			$stat[ self::$attachment_id ],
@@ -127,6 +136,70 @@ public function test_flags_a_sync_error_when_a_type_repeats() {
 		);
 	}
 
+	/**
+	 * Once the same repeat has happened on enough consecutive passes (the default is 3, matching
+	 * the filter's default), it's no longer a plausible one-off: flag the sync error, naming the
+	 * type that wouldn't advance.
+	 *
+	 * @return void
+	 */
+	public function test_flags_a_sync_error_once_the_strike_limit_is_reached() {
+		$this->register_never_resolving_sync_type();
+		$push = \Cloudinary\get_plugin_instance()->get_component( 'sync' )->managers['push'];
+
+		$push->process_assets( self::$attachment_id );
+		$push->process_assets( self::$attachment_id );
+		$this->assertEmpty(
+			get_post_meta( self::$attachment_id, Sync::META_KEYS['sync_error'], true ),
+			'Two strikes should not have flagged it yet.'
+		);
+
+		$push->process_assets( self::$attachment_id );
+
+		$error = get_post_meta( self::$attachment_id, Sync::META_KEYS['sync_error'], true );
+		$this->assertNotEmpty( $error, 'The third strike should flag a sync error.' );
+		$this->assertStringContainsString( 'test_stuck_type', $error );
+		$this->assertEmpty(
+			get_post_meta( self::$attachment_id, Sync::META_KEYS['loop_count'], true ),
+			'The strike count should be cleared once it has done its job.'
+		);
+	}
+
+	/**
+	 * A pass that completes without repeating -- the attachment recovering on its own after an
+	 * earlier, transient repeat -- must clear the strike count, not leave it to accumulate with
+	 * unrelated future repeats.
+	 *
+	 * @return void
+	 */
+	public function test_a_successful_pass_resets_the_strike_count() {
+		$sync = $this->register_never_resolving_sync_type();
+		$push = $sync->managers['push'];
+
+		$push->process_assets( self::$attachment_id );
+		$this->assertSame( '1', get_post_meta( self::$attachment_id, Sync::META_KEYS['loop_count'], true ) );
+
+		// Make the type resolve this time, instead of repeating.
+		$sync->register_sync_type(
+			'test_stuck_type',
+			array(
+				'asset_state' => 0,
+				'generate'    => '__return_true',
+				'validate'    => '__return_true',
+				'priority'    => -1,
+				'sync'        => function ( $attachment_id ) use ( $sync ) {
+					$sync->set_signature_item( $attachment_id, 'test_stuck_type' );
+				},
+				'realtime'    => false,
+			)
+		);
+		$sync->setup_sync_types();
+
+		$push->process_assets( self::$attachment_id );
+
+		$this->assertEmpty( get_post_meta( self::$attachment_id, Sync::META_KEYS['loop_count'], true ) );
+	}
+
 	/**
 	 * Once errored, build_queue()'s own meta_query (`_cld_error` NOT EXISTS) is what keeps a
 	 * stuck attachment from being queued again -- confirm the flag this guard sets actually
@@ -138,7 +211,10 @@ public function test_flagged_attachment_is_excluded_from_the_next_queue_build()
 		$this->register_never_resolving_sync_type();
 
 		$sync = \Cloudinary\get_plugin_instance()->get_component( 'sync' );
-		$sync->managers['push']->process_assets( self::$attachment_id );
+		$push = $sync->managers['push'];
+		$push->process_assets( self::$attachment_id );
+		$push->process_assets( self::$attachment_id );
+		$push->process_assets( self::$attachment_id );
 
 		$queue = $sync->managers['queue'];
 		$queue->build_queue();
diff --git a/tests/phpunit/tests/test-sync-loop-guard.php b/tests/phpunit/tests/test-sync-loop-guard.php
index a343fb909..b98842a09 100644
--- a/tests/phpunit/tests/test-sync-loop-guard.php
+++ b/tests/phpunit/tests/test-sync-loop-guard.php
@@ -10,6 +10,11 @@
  * over and over. These confirm it instead bails out to a sync error once a type clearly isn't
  * resolving.
  *
+ * The guard counts how deep the current, still-unresolved recursion chain is, not how many times
+ * validate_sync_type() has been entered overall, since a type that resolves immediately (no
+ * recursion) can legitimately be checked many times across a long request without ever being
+ * stuck -- confirmed below alongside the stuck case.
+ *
  * @package Cloudinary
  */
 
@@ -26,9 +31,9 @@ class Test_Sync_Loop_Guard extends WP_UnitTestCase {
 	 * attachment, is realtime, and (unlike 'file') does no network calls, so it is exactly the
 	 * type the live incident got stuck recursing on.
 	 *
-	 * A fresh one per test, not a shared static: validate_sync_type()'s retry counter is a
-	 * function-static, so it persists for this whole PHPUnit process. Reusing one attachment
-	 * across test methods would let one test's count carry over into the next.
+	 * A fresh one per test, not a shared static, for isolation: validate_sync_type()'s counter is
+	 * a function-static keyed by attachment ID, so a fresh ID per test rules out one test's calls
+	 * being counted against another's, on top of the counter's own per-chain reset.
 	 *
 	 * @var int
 	 */
@@ -42,6 +47,26 @@ public function set_up() {
 		$this->attachment_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
 	}
 
+	/**
+	 * register_sync_type() (used below for 'test_pending_type') writes directly into the Sync
+	 * component singleton, which outlives any one test -- see the matching note in
+	 * Test_Push_Sync_Outer_Loop_Guard, whose own custom type this would otherwise collide with if
+	 * both ran in the same PHPUnit process.
+	 *
+	 * @return void
+	 */
+	public function tear_down() {
+		$sync       = \Cloudinary\get_plugin_instance()->get_component( 'sync' );
+		$reflection = new ReflectionProperty( $sync, 'sync_base_struct' );
+		$reflection->setAccessible( true );
+		$struct = $reflection->getValue( $sync );
+		unset( $struct['test_pending_type'] );
+		$reflection->setValue( $sync, $struct );
+		$sync->setup_sync_types();
+
+		parent::tear_down();
+	}
+
 	/**
 	 * Sync-type registration (incl. 'delivery') and every component's manager wiring --
 	 * Sync::$managers['media'], Media::$sync, Delivery::$sync, and so on -- only happen once the
@@ -166,4 +191,44 @@ public function test_does_not_trip_during_normal_resolution() {
 
 		$this->assertEmpty( get_post_meta( $this->attachment_id, Sync::META_KEYS['sync_error'], true ) );
 	}
+
+	/**
+	 * A type that's simply pending -- not stuck, just not synced yet, and not realtime so nothing
+	 * recurses -- can legitimately be checked many times in one long request (status rendered for
+	 * several references to the same attachment, the process_assets() loop itself, and so on).
+	 * None of those calls recurse into one another, so the guard must never mistake their total
+	 * count for one unresolved chain, however many of them there are.
+	 *
+	 * @return void
+	 */
+	public function test_many_independent_calls_for_a_stable_pending_type_do_not_trip_the_guard() {
+		$sync = $this->get_sync();
+		add_filter( 'cloudinary_sync_type_retry_limit', fn() => 5 );
+
+		// A type that never resolves but also never recurses: validate() is true and it isn't
+		// realtime, so validate_sync_type() returns it directly every time, same as a type simply
+		// waiting its turn. Lowest priority so it's always what get_sync_type() picks.
+		$sync->register_sync_type(
+			'test_pending_type',
+			array(
+				'asset_state' => 0,
+				'generate'    => '__return_true',
+				'validate'    => '__return_true',
+				'priority'    => -1,
+				'sync'        => '__return_null',
+				'realtime'    => false,
+			)
+		);
+		$sync->setup_sync_types();
+
+		for ( $i = 0; $i < 20; $i++ ) {
+			$type = $sync->get_sync_type( $this->attachment_id, false );
+			$this->assertSame( 'test_pending_type', $type );
+		}
+
+		$this->assertEmpty(
+			get_post_meta( $this->attachment_id, Sync::META_KEYS['sync_error'], true ),
+			'20 independent, non-recursing calls (4x the 5-call limit) must not trip the guard.'
+		);
+	}
 }

From 9153887c881d786023e632036b588a8e7a76f31c Mon Sep 17 00:00:00 2001
From: Gabriel de Tassigny <gabriel.detassigny@xwp.co>
Date: Wed, 21 Oct 2026 14:48:11 +0200
Subject: [PATCH 3/3] fix(sync): don't count an intentional sync delay as a
 loop strike

Addresses review comment on #1328 (discussion_r4323662606):

Storage's "Cloudinary only" offload mode deliberately delays removing
the local file for a minute after upload (DELETE_DELAY). While that
delay is active, Storage::validate() still returns true (the local
file exists) and 'storage' isn't realtime, so validate_sync_type()
hands it straight back without recursing; Storage::sync() then
returns early via can_sync()'s delay_cld_only() filter, without
saving a matching signature. process_assets() sees 'storage' repeat
in the same pass -- an entirely expected, intentional wait, not a
stuck attachment.

Auto sync can run process_assets() on the same attachment several
times within that one-minute window, so the strike counter added in
the previous commit could reach its limit purely from this normal
behaviour and permanently error out a healthy attachment.

Skip the strike (but still break out of the pass, same as always)
when can_sync() says the repeated type isn't meant to run right now.
This generalizes beyond storage: any sync type gated the same way
behind can_sync() gets the same treatment.

Added a test that reproduces the real scenario -- 'cld' offload, a
freshly-set delay, process_assets() called three times -- and
confirms no sync error is set. Verified it fails without the fix
(reverted the can_sync() check locally, confirmed the new test
catches it, then restored the fix) before adding it.
---
 php/sync/class-push-sync.php                  | 65 ++++++++++---------
 .../tests/test-push-sync-outer-loop-guard.php | 63 ++++++++++++++++++
 2 files changed, 99 insertions(+), 29 deletions(-)

diff --git a/php/sync/class-push-sync.php b/php/sync/class-push-sync.php
index f7ca50246..673101c52 100644
--- a/php/sync/class-push-sync.php
+++ b/php/sync/class-push-sync.php
@@ -287,36 +287,43 @@ public function process_assets( $attachments = array() ) {
 					// 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;
-					$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 );
+					$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;
diff --git a/tests/phpunit/tests/test-push-sync-outer-loop-guard.php b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
index 050dc678c..7235a6de0 100644
--- a/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
+++ b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php
@@ -200,6 +200,69 @@ public function test_a_successful_pass_resets_the_strike_count() {
 		$this->assertEmpty( get_post_meta( self::$attachment_id, Sync::META_KEYS['loop_count'], true ) );
 	}
 
+	/**
+	 * A real sync type can repeat for a reason that isn't being stuck at all: Storage's "Cloudinary
+	 * only" offload mode deliberately delays removing the local file for a minute after upload, so
+	 * its own sync() returns early (via can_sync()'s delay_cld_only() filter) without saving a
+	 * matching signature -- get_sync_type() keeps handing 'storage' back for as long as that delay
+	 * lasts. Auto sync can easily run process_assets() on the same attachment three times within
+	 * that one minute, so without this check those three ordinary, expected repeats would
+	 * permanently error out a perfectly healthy attachment.
+	 *
+	 * @return void
+	 */
+	public function test_an_intentional_sync_delay_does_not_count_as_a_strike() {
+		$plugin  = \Cloudinary\get_plugin_instance();
+		$connect = $plugin->get_component( 'connect' );
+
+		$plugin->settings->set_param( 'connected', true );
+		set_transient( Connect::META_KEYS['usage'], array( 'placeholder' => true ), HOUR_IN_SECONDS );
+		$plugin->settings->set_value( 'cloudinary_url', 'cloudinary://123456789012345:abcdefghijklmnopqrstuvwxyzABCDEF@test-cloud' );
+		$plugin->settings->set_value( 'offload', 'cld' );
+		$connect->setup();
+
+		do_action( 'cloudinary_connected', $plugin ); // Delivery::init() etc.
+
+		$plugin->get_component( 'media' )->setup();
+		$sync = $plugin->get_component( 'sync' );
+		$sync->setup();
+		$plugin->get_component( 'delivery' )->setup();
+		// Registers 'storage' and 'size' plus the delay_cld_only() filter, reading the 'cld'
+		// offload mode just set above. Must run before register_assets() below, which is what
+		// actually rebuilds $sync_types to include whatever is registered by this point.
+		$plugin->get_component( 'storage' )->setup();
+
+		$sync->register_assets();
+
+		// Act as if the attachment is already fully synced for everything except storage/size --
+		// matching the real scenario this guards against, where auto sync has already uploaded a
+		// new attachment and the offload delay is the only thing left pending. $sync_types is
+		// protected, so read it the same way the other tests in this file reach sync_base_struct.
+		update_post_meta( self::$attachment_id, '_public_id', 'fake-public-id' );
+		$reflection = new ReflectionProperty( $sync, 'sync_types' );
+		$reflection->setAccessible( true );
+		foreach ( array_keys( $reflection->getValue( $sync ) ) as $type ) {
+			if ( in_array( $type, array( 'storage', 'size' ), true ) ) {
+				continue;
+			}
+			$sync->set_signature_item( self::$attachment_id, $type );
+		}
+
+		// A delay that was just set, so delay_cld_only() blocks the actual offload for the next
+		// minute -- an intentional wait, not a stuck attachment.
+		update_post_meta( self::$attachment_id, Sync::META_KEYS['delay'], time() );
+
+		$push = $sync->managers['push'];
+		$push->process_assets( self::$attachment_id );
+		$push->process_assets( self::$attachment_id );
+		$push->process_assets( self::$attachment_id );
+
+		$this->assertEmpty(
+			get_post_meta( self::$attachment_id, Sync::META_KEYS['sync_error'], true ),
+			'An intentional wait on the offload delay must never be counted as a stuck loop.'
+		);
+	}
+
 	/**
 	 * Once errored, build_queue()'s own meta_query (`_cld_error` NOT EXISTS) is what keeps a
 	 * stuck attachment from being queued again -- confirm the flag this guard sets actually