diff --git a/php/class-media.php b/php/class-media.php index cf4a3370b..cdfefb3f2 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -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 ); + $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 ); + } 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 +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 ); } diff --git a/php/class-sync.php b/php/class-sync.php index 30e73015d..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', @@ -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 ] ); + } + } } /** diff --git a/php/sync/class-push-sync.php b/php/sync/class-push-sync.php index b24761e71..673101c52 100644 --- a/php/sync/class-push-sync.php +++ b/php/sync/class-push-sync.php @@ -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; @@ -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 ); } 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 @@ +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..7235a6de0 --- /dev/null +++ b/tests/phpunit/tests/test-push-sync-outer-loop-guard.php @@ -0,0 +1,291 @@ +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; + } + + /** + * 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_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 ); + + $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 ], + 'The loop should have broken after the type repeated once, not kept running.' + ); + } + + /** + * 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 ) ); + } + + /** + * 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 + * 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' ); + $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(); + + $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..b98842a09 --- /dev/null +++ b/tests/phpunit/tests/test-sync-loop-guard.php @@ -0,0 +1,234 @@ +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 + * 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 ) ); + } + + /** + * 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.' + ); + } +}