From 9e3bc007698e1c94bdfd0edbd53de0f81fd111de Mon Sep 17 00:00:00 2001 From: selul Date: Fri, 9 Oct 2026 19:15:06 +0300 Subject: [PATCH 1/6] fix: let WP-CLI continue a rollback started from the dashboard "Transfer back from Optimole" turns offload_media off when it starts the rollback, and `wp optimole media rollback_images` refused to run whenever offload_media was off. A rollback may now run while one is in progress (is_offload_enabled(), as the Abilities API already does); moving images to Optimole still needs offload_media on. The new Optml_Settings::can_transfer_images() holds that rule so it can be tested. The command also stopped a rollback when the offload limit flag was set; the limit only concerns uploads, so it now stops the offload only. Co-Authored-By: Claude Opus 5.5 --- inc/cli/cli_media.php | 5 +++-- inc/settings.php | 17 +++++++++++++++++ tests/test-media.php | 28 ++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/inc/cli/cli_media.php b/inc/cli/cli_media.php index 8aa31e12..fdc771e5 100644 --- a/inc/cli/cli_media.php +++ b/inc/cli/cli_media.php @@ -43,7 +43,7 @@ private function update_images_template( $action ) { 'limit_exceeded' => __( 'You have reached the maximum offloading limit of images. To increase the offload limit and for more information, contact our support.', 'optimole-wp' ), ]; $settings = new Optml_Settings(); - if ( $settings->get( 'offload_media' ) === 'disabled' ) { + if ( ! $settings->can_transfer_images( $action ) ) { return \WP_CLI::error( __( 'You need to have the offload_media option enabled in order to use this command', 'optimole-wp' ) ); } @@ -88,7 +88,8 @@ private function update_images_template( $action ) { $action === 'rollback' ? Optml_Media_Offload::instance()->rollback_images( $batch ) : Optml_Media_Offload::instance()->upload_images( $batch ); } - if ( Optml_Media_Offload::instance()->settings->is_offload_limit_reached() ) { + // The offload limit stops uploads only: moving images back lowers the usage. + if ( $action === 'offload' && Optml_Media_Offload::instance()->settings->is_offload_limit_reached() ) { WP_CLI::error( $strings['limit_exceeded'], true ); } diff --git a/inc/settings.php b/inc/settings.php index 1707afff..2ed70887 100644 --- a/inc/settings.php +++ b/inc/settings.php @@ -881,6 +881,23 @@ public function is_offload_enabled() { return $this->get( 'offload_media' ) === 'enabled' || $this->get( 'rollback_status' ) !== 'disabled'; } + /** + * Whether a bulk image transfer can run. Moving images to Optimole needs offload_media on. + * Moving them back is also allowed while a rollback is in progress: "Transfer back" in the + * dashboard turns offload_media off when it starts the rollback. + * + * @param string $action The transfer, 'offload' or 'rollback'. + * + * @return bool + */ + public function can_transfer_images( $action ) { + if ( 'rollback' === $action ) { + return $this->is_offload_enabled(); + } + + return $this->get( 'offload_media' ) !== 'disabled'; + } + /** * Get cloud sites whitelist for current domain only. * diff --git a/tests/test-media.php b/tests/test-media.php index 529b5373..14b8eb6b 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -345,6 +345,34 @@ public function test_image_sync() { $this->assertStringContainsString( '/w:300/h:200/q:mauto/process:' . self::$sample_attachement .'/id:579c7f7707ce87caa65fdf50c238a117', $image_medium_size[0] ); } + /** + * "Transfer back" in the dashboard turns offload_media off and starts the rollback; WP-CLI + * `wp optimole media rollback_images` must still be able to continue it. + * + * @dataProvider transfer_gate_provider + */ + public function test_can_transfer_images( $offload_media, $rollback_status, $action, $expected ) { + $settings = new Optml_Settings(); + $settings->update( 'offload_media', $offload_media ); + $settings->update( 'rollback_status', $rollback_status ); + + $this->assertSame( $expected, $settings->can_transfer_images( $action ) ); + } + + /** + * @return array + */ + public function transfer_gate_provider() { + return [ + 'offload with offload on' => [ 'enabled', 'disabled', 'offload', true ], + 'rollback with offload on' => [ 'enabled', 'disabled', 'rollback', true ], + 'rollback while a rollback runs' => [ 'disabled', 'enabled', 'rollback', true ], + 'offload while a rollback runs' => [ 'disabled', 'enabled', 'offload', false ], + 'rollback with offload off and idle' => [ 'disabled', 'disabled', 'rollback', false ], + 'offload with offload off and idle' => [ 'disabled', 'disabled', 'offload', false ], + ]; + } + public function test_image_rollback() { Optml_Media_Offload::instance()->rollback_images( 100 ); From 13afcb5560661d3e37bdf132fc5591e49d21be8d Mon Sep 17 00:00:00 2001 From: selul Date: Sun, 11 Oct 2026 15:52:39 +0300 Subject: [PATCH 2/6] test: run the WP-CLI media command under PHPUnit The tests only covered Optml_Settings::can_transfer_images(), so the command could go back to its old checks unnoticed. A small WP-CLI stand-in (tests/wp-cli-stub.php) lets the real command run: it continues a rollback started from the dashboard, is not stopped by the offload limit flag, still stops an offload when the account is at its limit, and is refused when nothing was offloaded. Co-Authored-By: Claude Opus 5.5 --- tests/test-media.php | 74 ++++++++++++++++++++++++++++++++++++++++++- tests/wp-cli-stub.php | 62 ++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/wp-cli-stub.php diff --git a/tests/test-media.php b/tests/test-media.php index 14b8eb6b..4e7ff487 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -29,6 +29,12 @@ class Test_Media extends WP_UnitTestCase { private static $sample_attachement_upper_case; private static $sample_attachment_scaled; public static $error_mark = false; + /** + * Offloaded images the mocked account reports, out of an offload limit of 5000. + * + * @var int + */ + public static $offloaded_images = 50; /** * Mock the api calls and remote images requests */ @@ -119,7 +125,7 @@ public function filter_pre_http_request( $false, $r, $url ) { 'data' => array( 'cdn_key' => 'cdn_key', 'cdn_secret' => 'cdn_secret', - 'offloaded_images' => 50, + 'offloaded_images' => self::$offloaded_images, 'offload_limit' => 5000, ), )), @@ -373,6 +379,72 @@ public function transfer_gate_provider() { ]; } + /** + * Run a `wp optimole media` command with the WP-CLI stand-ins. + * + * @param string $command The command method, 'rollback_images' or 'move_to_optimole'. + * @return string[] The lines the command printed. + */ + private function run_cli_media_command( $command ) { + require_once __DIR__ . '/wp-cli-stub.php'; + WP_CLI::$lines = []; + + ( new Optml_Cli_Media() )->$command(); + + return WP_CLI::$lines; + } + + public function test_cli_rollback_continues_a_rollback_started_from_the_dashboard() { + // "Transfer back from Optimole": offload_media goes off, the rollback is in progress. + $settings = new Optml_Settings(); + $settings->update( 'offload_media', 'disabled' ); + $settings->update( 'rollback_status', 'enabled' ); + + $lines = $this->run_cli_media_command( 'rollback_images' ); + + $this->assertContains( 'All images have been uploaded to your media library', $lines ); + $this->assertStringNotContainsString( 'id:', wp_get_attachment_metadata( self::$sample_attachement )['file'] ); + $this->assertFileExists( get_attached_file( self::$sample_attachement ) ); + } + + public function test_cli_rollback_is_not_stopped_by_the_offload_limit() { + $settings = new Optml_Settings(); + $settings->update( 'offload_limit_reached', 'enabled' ); + + $lines = $this->run_cli_media_command( 'rollback_images' ); + + $this->assertContains( 'All images have been uploaded to your media library', $lines ); + $this->assertStringNotContainsString( 'id:', wp_get_attachment_metadata( self::$sample_attachement )['file'] ); + } + + public function test_cli_offload_stops_at_the_offload_limit() { + // Bring the images back first, so there is something to offload; the account is at its limit. + Optml_Media_Offload::instance()->rollback_images( 100 ); + self::$offloaded_images = 5000; + require_once __DIR__ . '/wp-cli-stub.php'; + + try { + $this->run_cli_media_command( 'move_to_optimole' ); + $this->fail( 'The offload was not stopped at the offload limit.' ); + } catch ( Optml_Test_WP_CLI_Error $error ) { + $this->assertStringContainsString( 'maximum offloading limit', $error->getMessage() ); + } finally { + self::$offloaded_images = 50; + } + } + + public function test_cli_rollback_is_refused_when_nothing_was_offloaded() { + $settings = new Optml_Settings(); + $settings->update( 'offload_media', 'disabled' ); + $settings->update( 'rollback_status', 'disabled' ); + require_once __DIR__ . '/wp-cli-stub.php'; + + $this->expectException( Optml_Test_WP_CLI_Error::class ); + $this->expectExceptionMessage( 'You need to have the offload_media option enabled' ); + + $this->run_cli_media_command( 'rollback_images' ); + } + public function test_image_rollback() { Optml_Media_Offload::instance()->rollback_images( 100 ); diff --git a/tests/wp-cli-stub.php b/tests/wp-cli-stub.php new file mode 100644 index 00000000..06f65332 --- /dev/null +++ b/tests/wp-cli-stub.php @@ -0,0 +1,62 @@ + Date: Sun, 11 Oct 2026 18:00:49 +0300 Subject: [PATCH 3/6] fix: do not let the WP-CLI transfer overlap a running background transfer With the gate open, `wp optimole media rollback_images` could move the same images as the dashboard's background chain at the same time, and restore them twice. The background chain takes the transfer lock when it is scheduled, and maybe_reschedule() takes it again on every request while a transfer is in progress, also where cron never runs the chain. So the command cannot simply wait for the lock: it would never run on a site with a stuck cron. The lock now records when a step ran (on renew). The command is refused while a background step ran within the lock's lifetime, and otherwise takes the lock over: a background step that fires later cannot renew it and stops, and nothing is rescheduled while the command holds it. The command renews the lock per batch and releases it on every exit. Co-Authored-By: Claude Opus 5.5 --- inc/cli/cli_media.php | 25 +++++++++++++++--- inc/media_offload.php | 37 ++++++++++++++++++++++++++ tests/test-media.php | 60 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 4 deletions(-) diff --git a/inc/cli/cli_media.php b/inc/cli/cli_media.php index fdc771e5..e2c7cc2c 100644 --- a/inc/cli/cli_media.php +++ b/inc/cli/cli_media.php @@ -41,25 +41,35 @@ private function update_images_template( $action ) { 'success' => __( 'All images have been uploaded to your media library', 'optimole-wp' ), ], 'limit_exceeded' => __( 'You have reached the maximum offloading limit of images. To increase the offload limit and for more information, contact our support.', 'optimole-wp' ), + 'transfer_running' => __( 'Another image transfer is running. Wait for it to finish, then run this command again.', 'optimole-wp' ), ]; $settings = new Optml_Settings(); if ( ! $settings->can_transfer_images( $action ) ) { return \WP_CLI::error( __( 'You need to have the offload_media option enabled in order to use this command', 'optimole-wp' ) ); } + $number_of_images_for = 'offload_images'; + if ( $action === 'rollback' ) { + $number_of_images_for = 'rollback_images'; + } + + // One run moves images at a time: this command replaces a background transfer that is only + // scheduled (for example where cron is stuck), and waits for one that is running. + $lock_token = Optml_Media_Offload::take_over_transfer_lock( $number_of_images_for ); + if ( false === $lock_token ) { + return \WP_CLI::error( $strings['transfer_running'] ); + } + Optml_Media_Offload::clear_offload_errors_meta(); if ( $action === 'rollback' ) { Optml_Media_Offload::clear_rollback_errors_meta(); } WP_CLI::line( $strings[ $action ]['info'] ); - $number_of_images_for = 'offload_images'; - if ( $action === 'rollback' ) { - $number_of_images_for = 'rollback_images'; - } $number_of_images = Optml_Media_Offload::number_of_images_and_pages( $number_of_images_for ); Optml_Media_Offload::record_process_meta( $number_of_images ); $batch = 5; if ( $number_of_images === 0 ) { + Optml_Media_Offload::release_transfer_lock( $lock_token ); WP_CLI::success( $strings[ $action ]['success'] ); return; @@ -90,13 +100,20 @@ private function update_images_template( $action ) { // The offload limit stops uploads only: moving images back lowers the usage. if ( $action === 'offload' && Optml_Media_Offload::instance()->settings->is_offload_limit_reached() ) { + Optml_Media_Offload::release_transfer_lock( $lock_token ); WP_CLI::error( $strings['limit_exceeded'], true ); } + // Keep the lock while images are moved. It is lost when another run took it over. + if ( ! Optml_Media_Offload::renew_transfer_lock( $lock_token, $number_of_images_for ) ) { + WP_CLI::error( $strings['transfer_running'], true ); + } + $progress->tick(); ++$tick; } $progress->finish(); + Optml_Media_Offload::release_transfer_lock( $lock_token ); WP_CLI::line( $strings[ $action ]['success'] ); } } diff --git a/inc/media_offload.php b/inc/media_offload.php index cd66df44..44e20e3a 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -2031,11 +2031,13 @@ public static function renew_transfer_lock( $token, $action ) { return false; } + // The time tells a transfer that is running from one that is only scheduled, see take_over_transfer_lock(). set_transient( self::TRANSFER_LOCK_TRANSIENT, [ 'token' => $token, 'action' => $action, + 'time' => time(), ], self::TRANSFER_LOCK_TTL ); @@ -2043,6 +2045,41 @@ public static function renew_transfer_lock( $token, $action ) { return true; } + /** + * Take the transfer lock for a run in the foreground (WP-CLI), which replaces the background one. + * + * The background chain takes the lock when it is scheduled, and is scheduled again on every + * request while a transfer is in progress, also where cron never runs it. So a held lock alone + * does not mean images are being moved: only a lock renewed by a step that ran does. While that + * is the case, this fails. Otherwise the lock gets a new token; a background step that fires + * later cannot renew it and stops, and nothing is scheduled again while the lock is held. + * + * @param string $action The transfer action ('offload_images'|'rollback_images'). + * + * @return string|false The lock token, or false while another run is moving images. + */ + public static function take_over_transfer_lock( $action ) { + $lock = get_transient( self::TRANSFER_LOCK_TRANSIENT ); + + if ( is_array( $lock ) && isset( $lock['time'] ) ) { + return false; + } + + $token = wp_generate_uuid4(); + + set_transient( + self::TRANSFER_LOCK_TRANSIENT, + [ + 'token' => $token, + 'action' => $action, + 'time' => time(), + ], + self::TRANSFER_LOCK_TTL + ); + + return $token; + } + /** * Release the transfer lock if we still own it, allowing another worker to acquire it. * diff --git a/tests/test-media.php b/tests/test-media.php index 4e7ff487..aebd57fc 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -431,6 +431,66 @@ public function test_cli_offload_stops_at_the_offload_limit() { } finally { self::$offloaded_images = 50; } + + $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ), 'The command released the transfer lock.' ); + } + + /** + * The dashboard's background transfer is moving images: a step ran and renewed the lock. The + * command must not move the same images at the same time. + */ + public function test_cli_rollback_waits_for_a_background_transfer_that_is_running() { + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + Optml_Media_Offload::renew_transfer_lock( $token, 'rollback_images' ); + $offloaded = Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ); + require_once __DIR__ . '/wp-cli-stub.php'; + + try { + $this->run_cli_media_command( 'rollback_images' ); + $this->fail( 'The command ran while a background transfer was running.' ); + } catch ( Optml_Test_WP_CLI_Error $error ) { + $this->assertStringContainsString( 'Another image transfer is running', $error->getMessage() ); + } + + $this->assertGreaterThan( 0, $offloaded ); + $this->assertSame( $offloaded, Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ), 'The command moved no image.' ); + $this->assertTrue( Optml_Media_Offload::renew_transfer_lock( $token, 'rollback_images' ), 'The background transfer keeps its lock.' ); + } + + /** + * Where cron is stuck, the background transfer is scheduled (and holds the lock) but never + * runs. The command replaces it, and releases the lock when it is done. + */ + public function test_cli_rollback_replaces_a_background_transfer_that_never_ran() { + $scheduled = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + + $lines = $this->run_cli_media_command( 'rollback_images' ); + + $this->assertContains( 'All images have been uploaded to your media library', $lines ); + $this->assertStringNotContainsString( 'id:', wp_get_attachment_metadata( self::$sample_attachement )['file'] ); + $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ) ); + $this->assertFalse( Optml_Media_Offload::renew_transfer_lock( $scheduled, 'rollback_images' ) ); + } + + public function test_cli_releases_the_transfer_lock_when_there_is_nothing_to_move() { + // Every image is offloaded already. + $lines = $this->run_cli_media_command( 'move_to_optimole' ); + + $this->assertContains( 'All images have been uploaded to Optimole Cloud', $lines ); + $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ) ); + } + + public function test_take_over_transfer_lock_stops_a_scheduled_background_step() { + $scheduled = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + + $foreground = Optml_Media_Offload::take_over_transfer_lock( 'rollback_images' ); + + $this->assertIsString( $foreground ); + $this->assertNotSame( $scheduled, $foreground ); + // The background step that fires later does not own the lock anymore, and a second run waits. + $this->assertFalse( Optml_Media_Offload::renew_transfer_lock( $scheduled, 'rollback_images' ) ); + $this->assertFalse( Optml_Media_Offload::take_over_transfer_lock( 'rollback_images' ) ); + $this->assertFalse( Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ) ); } public function test_cli_rollback_is_refused_when_nothing_was_offloaded() { From 99891d97c81eb3d5407da7775dabb8ba73695d4f Mon Sep 17 00:00:00 2001 From: selul Date: Sun, 11 Oct 2026 19:01:02 +0300 Subject: [PATCH 4/6] fix: use a database advisory lock so only one run moves images The takeover of the transfer lock was a read-then-write: a background step could renew the lock between the two, and both runs then moved the same images. The transfer lock (a transient) goes back to what it was: it says which chain owns a transfer, and is taken when the chain is scheduled. A second lock now says who is moving images at this moment: a MySQL advisory lock (GET_LOCK), which is atomic and freed by the database when the request ends, also after a crash. A background step takes it for its batch and does nothing when it cannot; the WP-CLI command takes it for its whole run and stops with a message when it cannot. Where the database has no advisory locks, the lock is granted and transfers work as before. Co-Authored-By: Claude Opus 5.5 --- inc/cli/cli_media.php | 27 ++++----- inc/media_offload.php | 74 +++++++++++++++--------- tests/test-media.php | 129 +++++++++++++++++++++++++++++++++--------- 3 files changed, 161 insertions(+), 69 deletions(-) diff --git a/inc/cli/cli_media.php b/inc/cli/cli_media.php index e2c7cc2c..2c82fea3 100644 --- a/inc/cli/cli_media.php +++ b/inc/cli/cli_media.php @@ -48,15 +48,9 @@ private function update_images_template( $action ) { return \WP_CLI::error( __( 'You need to have the offload_media option enabled in order to use this command', 'optimole-wp' ) ); } - $number_of_images_for = 'offload_images'; - if ( $action === 'rollback' ) { - $number_of_images_for = 'rollback_images'; - } - - // One run moves images at a time: this command replaces a background transfer that is only - // scheduled (for example where cron is stuck), and waits for one that is running. - $lock_token = Optml_Media_Offload::take_over_transfer_lock( $number_of_images_for ); - if ( false === $lock_token ) { + // One run moves images at a time. A background step that is moving images right now holds this + // lock; while this command holds it, the background steps do nothing. + if ( ! Optml_Media_Offload::acquire_moving_images_lock() ) { return \WP_CLI::error( $strings['transfer_running'] ); } @@ -65,11 +59,15 @@ private function update_images_template( $action ) { Optml_Media_Offload::clear_rollback_errors_meta(); } WP_CLI::line( $strings[ $action ]['info'] ); + $number_of_images_for = 'offload_images'; + if ( $action === 'rollback' ) { + $number_of_images_for = 'rollback_images'; + } $number_of_images = Optml_Media_Offload::number_of_images_and_pages( $number_of_images_for ); Optml_Media_Offload::record_process_meta( $number_of_images ); $batch = 5; if ( $number_of_images === 0 ) { - Optml_Media_Offload::release_transfer_lock( $lock_token ); + Optml_Media_Offload::release_moving_images_lock(); WP_CLI::success( $strings[ $action ]['success'] ); return; @@ -100,20 +98,15 @@ private function update_images_template( $action ) { // The offload limit stops uploads only: moving images back lowers the usage. if ( $action === 'offload' && Optml_Media_Offload::instance()->settings->is_offload_limit_reached() ) { - Optml_Media_Offload::release_transfer_lock( $lock_token ); + Optml_Media_Offload::release_moving_images_lock(); WP_CLI::error( $strings['limit_exceeded'], true ); } - // Keep the lock while images are moved. It is lost when another run took it over. - if ( ! Optml_Media_Offload::renew_transfer_lock( $lock_token, $number_of_images_for ) ) { - WP_CLI::error( $strings['transfer_running'], true ); - } - $progress->tick(); ++$tick; } $progress->finish(); - Optml_Media_Offload::release_transfer_lock( $lock_token ); + Optml_Media_Offload::release_moving_images_lock(); WP_CLI::line( $strings[ $action ]['success'] ); } } diff --git a/inc/media_offload.php b/inc/media_offload.php index 44e20e3a..ff0020cc 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -2031,13 +2031,11 @@ public static function renew_transfer_lock( $token, $action ) { return false; } - // The time tells a transfer that is running from one that is only scheduled, see take_over_transfer_lock(). set_transient( self::TRANSFER_LOCK_TRANSIENT, [ 'token' => $token, 'action' => $action, - 'time' => time(), ], self::TRANSFER_LOCK_TTL ); @@ -2046,38 +2044,52 @@ public static function renew_transfer_lock( $token, $action ) { } /** - * Take the transfer lock for a run in the foreground (WP-CLI), which replaces the background one. + * Take the lock of the run that is moving images right now, without waiting. * - * The background chain takes the lock when it is scheduled, and is scheduled again on every - * request while a transfer is in progress, also where cron never runs it. So a held lock alone - * does not mean images are being moved: only a lock renewed by a step that ran does. While that - * is the case, this fails. Otherwise the lock gets a new token; a background step that fires - * later cannot renew it and stops, and nothing is scheduled again while the lock is held. + * The transfer lock above says which chain owns a transfer. It is taken when a chain is scheduled, + * and again on every request while a transfer is in progress, so it cannot tell whether images are + * being moved at this moment. This one is a database advisory lock: taking it is atomic, and the + * database frees it when the request ends, also after a crash. A background step holds it for its + * batch, and WP-CLI for its whole run, so the two never move the same images at the same time. * - * @param string $action The transfer action ('offload_images'|'rollback_images'). + * @return bool False while another run holds it. True when it was taken, and also where the + * database has no advisory locks, so transfers work there as before. + */ + public static function acquire_moving_images_lock(): bool { + global $wpdb; + + $suppress = $wpdb->suppress_errors(); + $result = $wpdb->get_var( $wpdb->prepare( 'SELECT GET_LOCK( %s, 0 )', self::get_moving_images_lock_name() ) ); + $wpdb->suppress_errors( $suppress ); + + return null === $result || 1 === (int) $result; + } + + /** + * Release the lock taken with {@see self::acquire_moving_images_lock()}. * - * @return string|false The lock token, or false while another run is moving images. + * @return void */ - public static function take_over_transfer_lock( $action ) { - $lock = get_transient( self::TRANSFER_LOCK_TRANSIENT ); + public static function release_moving_images_lock(): void { + global $wpdb; - if ( is_array( $lock ) && isset( $lock['time'] ) ) { - return false; - } + $suppress = $wpdb->suppress_errors(); + $wpdb->get_var( $wpdb->prepare( 'SELECT RELEASE_LOCK( %s )', self::get_moving_images_lock_name() ) ); + $wpdb->suppress_errors( $suppress ); + } - $token = wp_generate_uuid4(); + /** + * Name of the advisory lock. Such locks are shared by every database on the server, so the name + * holds the database and the table prefix (which is per site on a network). + * + * @return string + */ + public static function get_moving_images_lock_name(): string { + global $wpdb; - set_transient( - self::TRANSFER_LOCK_TRANSIENT, - [ - 'token' => $token, - 'action' => $action, - 'time' => time(), - ], - self::TRANSFER_LOCK_TTL - ); + $database = defined( 'DB_NAME' ) ? DB_NAME : ''; - return $token; + return 'optml_moving_images_' . md5( $database . $wpdb->prefix ); } /** @@ -2188,6 +2200,14 @@ public function start_processing_images( $action, $batch, $page, $total, $step, return; } + // Only one run moves images at a time. WP-CLI holds this lock while its transfer runs: it does the + // work of this step, and the chain is scheduled again once the transfer lock expires. + if ( ! self::acquire_moving_images_lock() ) { + self::$instance->logger->add_log( $type, 'Another run is moving images, this step is skipped.' ); + + return; + } + set_time_limit( 0 ); try { @@ -2238,6 +2258,8 @@ public function start_processing_images( $action, $batch, $page, $total, $step, $lock_token, ] ); + } finally { + self::release_moving_images_lock(); } } diff --git a/tests/test-media.php b/tests/test-media.php index aebd57fc..6b9025d5 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -432,65 +432,142 @@ public function test_cli_offload_stops_at_the_offload_limit() { self::$offloaded_images = 50; } - $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ), 'The command released the transfer lock.' ); + $this->assert_moving_images_lock_is_free(); } /** - * The dashboard's background transfer is moving images: a step ran and renewed the lock. The - * command must not move the same images at the same time. + * A second database connection: for advisory locks, another process. + * + * @return wpdb + */ + private function other_connection(): wpdb { + return new wpdb( DB_USER, DB_PASSWORD, DB_NAME, DB_HOST ); + } + + /** + * Hold or release the "moving images" lock from another connection. + * + * @param wpdb $connection The other connection. + * @param bool $hold True to take the lock, false to release it. + * @return void */ - public function test_cli_rollback_waits_for_a_background_transfer_that_is_running() { - $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); - Optml_Media_Offload::renew_transfer_lock( $token, 'rollback_images' ); + private function moving_images_lock_elsewhere( wpdb $connection, bool $hold ): void { + $query = $hold ? 'SELECT GET_LOCK( %s, 0 )' : 'SELECT RELEASE_LOCK( %s )'; + + $this->assertSame( '1', $connection->get_var( $connection->prepare( $query, Optml_Media_Offload::get_moving_images_lock_name() ) ) ); + } + + /** + * @return void + */ + private function assert_moving_images_lock_is_free(): void { + global $wpdb; + + $this->assertSame( '1', $wpdb->get_var( $wpdb->prepare( 'SELECT IS_FREE_LOCK( %s )', Optml_Media_Offload::get_moving_images_lock_name() ) ), 'The "moving images" lock was not released.' ); + } + + /** + * A background step is moving images at this moment. The command must not move the same images + * at the same time; it runs once that step is done. + */ + public function test_cli_rollback_waits_for_a_run_that_is_moving_images(): void { + $other = $this->other_connection(); $offloaded = Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ); + $this->moving_images_lock_elsewhere( $other, true ); require_once __DIR__ . '/wp-cli-stub.php'; try { $this->run_cli_media_command( 'rollback_images' ); - $this->fail( 'The command ran while a background transfer was running.' ); + $this->fail( 'The command ran while another run was moving images.' ); } catch ( Optml_Test_WP_CLI_Error $error ) { $this->assertStringContainsString( 'Another image transfer is running', $error->getMessage() ); } $this->assertGreaterThan( 0, $offloaded ); $this->assertSame( $offloaded, Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ), 'The command moved no image.' ); - $this->assertTrue( Optml_Media_Offload::renew_transfer_lock( $token, 'rollback_images' ), 'The background transfer keeps its lock.' ); + + $this->moving_images_lock_elsewhere( $other, false ); + $lines = $this->run_cli_media_command( 'rollback_images' ); + + $this->assertContains( 'All images have been uploaded to your media library', $lines ); + $this->assertSame( 0, Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ) ); + } + + /** + * While the command (another connection here) moves the images, a background step that fires + * does nothing: it moves no image and does not continue the chain. Once the lock is free, the + * step runs and schedules the next one. + */ + public function test_background_step_does_nothing_while_another_run_moves_images(): void { + ( new Optml_Settings() )->update( 'rollback_status', 'enabled' ); + $other = $this->other_connection(); + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + $offloaded = Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ); + $this->moving_images_lock_elsewhere( $other, true ); + + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 1, 5, 1, $token ); + + $this->assertGreaterThan( 0, $offloaded ); + $this->assertSame( $offloaded, Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ), 'The step moved no image.' ); + $this->assertSame( 0, $this->count_scheduled_transfer_steps() ); + + $this->moving_images_lock_elsewhere( $other, false ); + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 1, 5, 1, $token ); + + $this->assertSame( 1, $this->count_scheduled_transfer_steps() ); + $this->assert_moving_images_lock_is_free(); } /** - * Where cron is stuck, the background transfer is scheduled (and holds the lock) but never - * runs. The command replaces it, and releases the lock when it is done. + * @return int The background transfer steps waiting in WP-Cron. */ - public function test_cli_rollback_replaces_a_background_transfer_that_never_ran() { + private function count_scheduled_transfer_steps(): int { + $count = 0; + + foreach ( (array) _get_cron_array() as $hooks ) { + $count += isset( $hooks['optml_start_processing_images'] ) ? count( $hooks['optml_start_processing_images'] ) : 0; + } + + return $count; + } + + /** + * Where cron is stuck, the background transfer is scheduled and holds the transfer lock, but no + * step runs. The command does the work, and leaves the transfer lock to the chain. + */ + public function test_cli_rollback_runs_while_a_background_transfer_is_only_scheduled(): void { $scheduled = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); $lines = $this->run_cli_media_command( 'rollback_images' ); $this->assertContains( 'All images have been uploaded to your media library', $lines ); - $this->assertStringNotContainsString( 'id:', wp_get_attachment_metadata( self::$sample_attachement )['file'] ); - $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ) ); - $this->assertFalse( Optml_Media_Offload::renew_transfer_lock( $scheduled, 'rollback_images' ) ); + $this->assertSame( 0, Optml_Media_Offload::number_of_images_and_pages( 'rollback_images' ) ); + $this->assert_moving_images_lock_is_free(); + $this->assertTrue( Optml_Media_Offload::renew_transfer_lock( $scheduled, 'rollback_images' ) ); } - public function test_cli_releases_the_transfer_lock_when_there_is_nothing_to_move() { + public function test_cli_releases_the_moving_images_lock_when_there_is_nothing_to_move(): void { // Every image is offloaded already. $lines = $this->run_cli_media_command( 'move_to_optimole' ); $this->assertContains( 'All images have been uploaded to Optimole Cloud', $lines ); - $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ) ); + $this->assert_moving_images_lock_is_free(); } - public function test_take_over_transfer_lock_stops_a_scheduled_background_step() { - $scheduled = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); - - $foreground = Optml_Media_Offload::take_over_transfer_lock( 'rollback_images' ); + /** + * A database without advisory locks (the query fails): transfers run as before. + */ + public function test_moving_images_lock_is_granted_where_the_database_has_no_advisory_locks(): void { + $break_get_lock = static function ( string $query ): string { + return false !== strpos( $query, 'GET_LOCK' ) ? 'SELECT optml_no_such_function()' : $query; + }; + add_filter( 'query', $break_get_lock ); - $this->assertIsString( $foreground ); - $this->assertNotSame( $scheduled, $foreground ); - // The background step that fires later does not own the lock anymore, and a second run waits. - $this->assertFalse( Optml_Media_Offload::renew_transfer_lock( $scheduled, 'rollback_images' ) ); - $this->assertFalse( Optml_Media_Offload::take_over_transfer_lock( 'rollback_images' ) ); - $this->assertFalse( Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ) ); + try { + $this->assertTrue( Optml_Media_Offload::acquire_moving_images_lock() ); + } finally { + remove_filter( 'query', $break_get_lock ); + } } public function test_cli_rollback_is_refused_when_nothing_was_offloaded() { From afd49c2098235d867188398688feac3c21754ac9 Mon Sep 17 00:00:00 2001 From: selul Date: Sun, 11 Oct 2026 19:41:46 +0300 Subject: [PATCH 5/6] fix: take the moving-images lock on the primary and release it at shutdown With several database servers (HyperDB, LudicrousDB) the lock query could go to a different server in each run, so two runs could both get the lock. Reads are now sent to the primary before the lock is asked. A fatal error skips the cleanup of the run. On a persistent connection the lock would then outlive the request, so it is also released on shutdown. Co-Authored-By: Claude Opus 5.5 --- inc/media_offload.php | 10 ++++++ tests/test-media.php | 72 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/inc/media_offload.php b/inc/media_offload.php index ff0020cc..f8368dba 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -2058,10 +2058,20 @@ public static function renew_transfer_lock( $token, $action ) { public static function acquire_moving_images_lock(): bool { global $wpdb; + // The lock lives on one database server. Where reads are spread over several (HyperDB, + // LudicrousDB), every run has to ask the same one: the primary. + if ( method_exists( $wpdb, 'send_reads_to_masters' ) ) { + $wpdb->send_reads_to_masters(); + } + $suppress = $wpdb->suppress_errors(); $result = $wpdb->get_var( $wpdb->prepare( 'SELECT GET_LOCK( %s, 0 )', self::get_moving_images_lock_name() ) ); $wpdb->suppress_errors( $suppress ); + // A fatal error skips the cleanup of the caller. On a persistent connection the lock would then + // outlive the request, so it is also released when the request shuts down. + add_action( 'shutdown', [ __CLASS__, 'release_moving_images_lock' ] ); + return null === $result || 1 === (int) $result; } diff --git a/tests/test-media.php b/tests/test-media.php index 6b9025d5..52fd6198 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -554,6 +554,78 @@ public function test_cli_releases_the_moving_images_lock_when_there_is_nothing_t $this->assert_moving_images_lock_is_free(); } + /** + * A fatal error skips the cleanup of the run. The lock is then released when the request shuts + * down, which matters on a persistent connection. + */ + public function test_moving_images_lock_is_also_released_at_shutdown(): void { + $this->assertTrue( Optml_Media_Offload::acquire_moving_images_lock() ); + + $this->assertNotFalse( has_action( 'shutdown', [ Optml_Media_Offload::class, 'release_moving_images_lock' ] ) ); + + Optml_Media_Offload::release_moving_images_lock(); + $this->assert_moving_images_lock_is_free(); + } + + /** + * With several database servers the lock must be asked from the primary by every run. HyperDB + * and LudicrousDB route the following reads there with send_reads_to_masters(). + */ + public function test_moving_images_lock_is_taken_on_the_primary_database_server(): void { + global $wpdb; + + $original = $wpdb; + $wpdb = new class( $original ) { + /** + * @var string[] The calls, in order. + */ + public $calls = []; + + /** + * @var string + */ + public $prefix; + + /** + * @var wpdb + */ + private $db; + + public function __construct( wpdb $db ) { + $this->db = $db; + $this->prefix = $db->prefix; + } + + public function send_reads_to_masters(): void { + $this->calls[] = 'send_reads_to_masters'; + } + + public function suppress_errors( $suppress = true ) { + return $this->db->suppress_errors( $suppress ); + } + + public function prepare( $query, ...$args ) { + return $this->db->prepare( $query, ...$args ); + } + + public function get_var( $query ) { + $this->calls[] = false !== strpos( $query, 'GET_LOCK' ) ? 'GET_LOCK' : 'query'; + + return $this->db->get_var( $query ); + } + }; + + try { + $this->assertTrue( Optml_Media_Offload::acquire_moving_images_lock() ); + $calls = $wpdb->calls; + } finally { + $wpdb = $original; + Optml_Media_Offload::release_moving_images_lock(); + } + + $this->assertSame( [ 'send_reads_to_masters', 'GET_LOCK' ], $calls ); + } + /** * A database without advisory locks (the query fails): transfers run as before. */ From 0e438a7d51ecccebdee13ca94c49dc06b85bc985 Mon Sep 17 00:00:00 2001 From: selul Date: Sun, 11 Oct 2026 20:11:44 +0300 Subject: [PATCH 6/6] fix: do not add a translatable string for the WP-CLI lock message The message of the refused command is plain English, like the other WP-CLI messages of the plugin, so the fix adds no string to translate. Co-Authored-By: Claude Opus 5.5 --- inc/cli/cli_media.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/inc/cli/cli_media.php b/inc/cli/cli_media.php index 2c82fea3..4033c744 100644 --- a/inc/cli/cli_media.php +++ b/inc/cli/cli_media.php @@ -41,7 +41,6 @@ private function update_images_template( $action ) { 'success' => __( 'All images have been uploaded to your media library', 'optimole-wp' ), ], 'limit_exceeded' => __( 'You have reached the maximum offloading limit of images. To increase the offload limit and for more information, contact our support.', 'optimole-wp' ), - 'transfer_running' => __( 'Another image transfer is running. Wait for it to finish, then run this command again.', 'optimole-wp' ), ]; $settings = new Optml_Settings(); if ( ! $settings->can_transfer_images( $action ) ) { @@ -51,7 +50,7 @@ private function update_images_template( $action ) { // One run moves images at a time. A background step that is moving images right now holds this // lock; while this command holds it, the background steps do nothing. if ( ! Optml_Media_Offload::acquire_moving_images_lock() ) { - return \WP_CLI::error( $strings['transfer_running'] ); + return \WP_CLI::error( 'Another image transfer is running. Wait for it to finish, then run this command again.' ); } Optml_Media_Offload::clear_offload_errors_meta();