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

[gabrielcld2]

What this fixes

Fixes #1301. When an attachment's sync state stops advancing, the plugin could get stuck repeatedly reprocessing it, sending internal requests to /wp-json/cloudinary/v1/queue until the server runs out of PHP workers.

Root cause

The plugin keeps all of an attachment's sync progress in one saved record. Updating it means: read the current record, change one part, save it all back. If two sync passes do this for the same attachment close together in time — e.g. a bulk sync and the automatic background sync both reaching it — one can silently overwrite the other's change, with nothing logged. Whichever change is lost looks to the plugin like that step never happened, so it retries, loses the update again, and never progresses.

On top of that, the retry logic for a stuck step had no limit and never flagged the attachment as failed, so it looped indefinitely instead of giving up and reporting an error.

This PR makes the save safe to run from two processes at once, and makes the retry logic give up and flag a sync error after a reasonable number of attempts instead of looping forever.

How to reproduce

No special filter or conflicting plugin needed — just two sync passes landing on the same, freshly uploaded attachment close together (routine with auto sync + a bulk sync running at the same time). In PHP, against a never-synced attachment ID:

$media = \Cloudinary\get_plugin_instance()->get_component( 'media' );

// Process A: pause partway through saving one piece of sync data, simulating
// a slow request under load.
add_filter( 'cloudinary_migrate_legacy_meta', function () {
    sleep( 2 );
    return array();
} );
$media->update_post_meta( $id, 'public_id', 'from-process-a' );

// Process B (a separate, concurrent PHP process, started ~1s after A, no filter applied):
$media->update_post_meta( $id, 'version', 'from-process-b' );

Before this fix, once A finishes it silently overwrites B's change. After this fix, both survive.

Testing

Three new PHPUnit suites (retry limit, queue-level guard, concurrent writes). Also verified manually on a real connected site: the original report's exact repro now resolves in seconds with a clear error instead of hanging, the race above no longer loses a write, and normal syncing is unaffected.

[utkarshcloudinary]

Fix is correct left some inline feedback.

[utkarshcloudinary]

blocker: This counter counts calls to validate_sync_type() for the whole request, not the recursion depth of one chain. It never decrements and it never resets.

If an attachment has a pending type that is not realtime (for example options after a settings change), each normal get_sync_type() call gets to this point and returns the type without a write. filter_status(), Delivery (once per URL item), Upload_Sync, and the process_assets() loop all call it many times. In a long request such as WP-CLI sync, a media library render, or a page with many sizes of the same image, the 51st normal call writes _cld_error. After that, the attachment stays out of sync until someone clears the errors manually.

Count only re-entry in the current chain. Decrement the counter when the call returns:

static $depth = array();
$key          = $attachment_id . ':' . $type;
$depth[ $key ] = ( $depth[ $key ] ?? 0 ) + 1;
try {
    // existing guard + body
} finally {
    --$depth[ $key ];
    if ( 0 >= $depth[ $key ] ) {
        unset( $depth[ $key ] );
    }
}

This change also stops $attempts from growing without limit during long CLI runs. Also add a test that calls get_sync_type() more than limit times on a stable pending attachment, and asserts that no _cld_error is written. test_does_not_trip_during_normal_resolution makes only one call, so it does not find this. The docblock in test-sync-loop-guard.php says that the counter continues across tests, which shows the same problem.

[gabrielcld2]

Done. implemented the decrement on finally. ff83557

[utkarshcloudinary]

warning: This branch now writes a permanent _cld_error when the same type comes back twice in one pass. Before this change, the code only did break, and the next queue build tried the attachment again.

A type can repeat for transient reasons. Examples are a sync callback that returns early without a WP_Error, a third-party sync type, or the concurrent write this PR describes when the CAS fallback at class-media.php:2834 still loses. These attachments now need a manual "clean up errors" step. Before, they recovered by themselves.

Consider a small counter in meta across passes, for example 3 consecutive loop detections before the error is set. Or write a process log entry with log_sync_result() so the reason is visible, and let build_queue() skip the attachment for a cool-down period. As a minimum, include $type in the message, as the validate_sync_type() guard does, so support can tell which state stalled.

[gabrielcld2]

Done. Now it tracks consecutive loop detections in postmeta across passes and only sets _cld_error after 3 strike (by default. number configurable by filter).

[utkarshcloudinary]

warning: wp_cache_delete( $post_id, 'post_meta' ) runs on every write. It is not only for retries. This deletes the cache for all meta of the attachment. metadata_exists() then runs a full SELECT for the post meta again.

upload_asset() does about 5 update_post_meta() calls and then calls set_signature_item() several times. Thus one upload now does about 10 or more full meta reloads, and with a persistent object cache, the same number of cache deletes and sets. This affects the bulk-sync path that this PR wants to make lighter.

Read from the cache on the first attempt. Call wp_cache_delete() only before a retry, after the CAS failed. The CAS itself (the WHERE meta_value = $prev condition) finds stale reads, so a fresh read is not necessary on the first attempt.

[gabrielcld2]

Done. Moved the cache-bust to only run before a retry.

[utkarshcloudinary]

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.

[utkarshcloudinary]

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.

[utkarshcloudinary]

Looks good; Left minor feedback.

[utkarshcloudinary]

warning: With the "Cloudinary only" storage mode, the strike counter can set a permanent error on a healthy attachment.

• Storage::validate() saves the delay meta and returns true while the local file exists (class-storage.php:310-319).
• Storage::sync() calls can_sync( $attachment_id, 'storage' ). The delay_cld_only() filter returns false for DELETE_DELAY (60 seconds), so sync() returns at line 156 and does not update the signature. This delay is intentional.
• Thus storage comes back a second time in process_assets(), and the attachment gets one strike. Before this PR, the loop only stopped and the next run tried again. Now, 3 runs during the 60-second delay set _cld_error. Then build_queue() skips the attachment, and it stays errored after the delay ends until someone manually cleans up the errors.

Auto sync handles new uploads immediately, so 3 runs in 60 seconds is possible.

Suggested fix: do not count a strike when can_sync() returns false for the repeated type. A more general option is to let a sync type tell the loop that it is waiting, because third-party sync types can also wait on purpose.

Suggested test: set the cld offload mode with a recent delay value, call process_assets() 3 times, and assert that no _cld_error is set.