diff --git a/inc/cli/cli_media.php b/inc/cli/cli_media.php index 8aa31e12..4033c744 100644 --- a/inc/cli/cli_media.php +++ b/inc/cli/cli_media.php @@ -43,10 +43,16 @@ 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' ) ); } + // 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( 'Another image transfer is running. Wait for it to finish, then run this command again.' ); + } + Optml_Media_Offload::clear_offload_errors_meta(); if ( $action === 'rollback' ) { Optml_Media_Offload::clear_rollback_errors_meta(); @@ -60,6 +66,7 @@ private function update_images_template( $action ) { Optml_Media_Offload::record_process_meta( $number_of_images ); $batch = 5; if ( $number_of_images === 0 ) { + Optml_Media_Offload::release_moving_images_lock(); WP_CLI::success( $strings[ $action ]['success'] ); return; @@ -88,7 +95,9 @@ 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() ) { + Optml_Media_Offload::release_moving_images_lock(); WP_CLI::error( $strings['limit_exceeded'], true ); } @@ -96,6 +105,7 @@ private function update_images_template( $action ) { ++$tick; } $progress->finish(); + 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 cd66df44..f8368dba 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -2043,6 +2043,65 @@ public static function renew_transfer_lock( $token, $action ) { return true; } + /** + * Take the lock of the run that is moving images right now, without waiting. + * + * 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. + * + * @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; + + // 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; + } + + /** + * Release the lock taken with {@see self::acquire_moving_images_lock()}. + * + * @return void + */ + public static function release_moving_images_lock(): void { + global $wpdb; + + $suppress = $wpdb->suppress_errors(); + $wpdb->get_var( $wpdb->prepare( 'SELECT RELEASE_LOCK( %s )', self::get_moving_images_lock_name() ) ); + $wpdb->suppress_errors( $suppress ); + } + + /** + * 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; + + $database = defined( 'DB_NAME' ) ? DB_NAME : ''; + + return 'optml_moving_images_' . md5( $database . $wpdb->prefix ); + } + /** * Release the transfer lock if we still own it, allowing another worker to acquire it. * @@ -2151,6 +2210,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 { @@ -2201,6 +2268,8 @@ public function start_processing_images( $action, $batch, $page, $total, $step, $lock_token, ] ); + } finally { + self::release_moving_images_lock(); } } 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..52fd6198 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, ), )), @@ -345,6 +351,309 @@ 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 ], + ]; + } + + /** + * 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; + } + + $this->assert_moving_images_lock_is_free(); + } + + /** + * 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 + */ + 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 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->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(); + } + + /** + * @return int The background transfer steps waiting in WP-Cron. + */ + 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->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_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->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. + */ + 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 ); + + 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() { + $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 @@ +