From 217bb5712cd4d1c7caf04ba273241a5dabbb66cb Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Wed, 21 Oct 2026 02:01:35 +0300 Subject: [PATCH 1/7] fix: always pass a string cache token to the Optimole SDK (#1189) Individual cache tokens are stored as the API returned them, which can be a number. get_active_cache_booster() passed that value straight to Optimole::image(), whose cache buster is typed as a string, so image URL replacement threw a TypeError and broke the request. Cast the token to a string, and fall back to an empty token for non-scalar values. Closes #1187 Claude-Session: https://claude.ai/code/session_01WPwFxvA5VPq8sd4EzsNFFA Co-authored-by: Claude Opus 5.5 (1M context) --- inc/url_replacer.php | 9 +++++++-- tests/test-replacer.php | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/inc/url_replacer.php b/inc/url_replacer.php index af827f3c..e1095c9d 100644 --- a/inc/url_replacer.php +++ b/inc/url_replacer.php @@ -324,13 +324,18 @@ private function normalize_image( $url, $original_url, $args, $is_uploaded = fal /** * Get the active cache booster. * + * Individual tokens are stored as the API returned them, which can be a number, + * so the token is cast to the string the SDK expects. + * * @param string $url The URL. - * @param string $main_cache_buster The default value. + * @param mixed $main_cache_buster The default value. * * @return string */ public static function get_active_cache_booster( $url, $main_cache_buster ) { - return ( get_transient( Optml_Settings::INDIVIDUAL_CACHE_TOKENS_KEY ) ?: [] )[ crc32( wp_basename( $url ) ) ] ?? $main_cache_buster; + $token = ( get_transient( Optml_Settings::INDIVIDUAL_CACHE_TOKENS_KEY ) ?: [] )[ crc32( wp_basename( $url ) ) ] ?? $main_cache_buster; + + return is_scalar( $token ) ? (string) $token : ''; } /** * Throw error on object clone diff --git a/tests/test-replacer.php b/tests/test-replacer.php index e42be6a3..231e4407 100644 --- a/tests/test-replacer.php +++ b/tests/test-replacer.php @@ -849,4 +849,35 @@ public function test_dam_flag_removal() { $this->assertStringNotContainsString('dam:1', $replaced_content); $this->assertStringContainsString('q:mauto', $replaced_content); } -} \ No newline at end of file + + /** + * Individual cache tokens are stored as the API returned them, which can be a number. + * The SDK only accepts a string cache buster. + */ + public function test_build_url_with_numeric_individual_cache_token() { + $url = 'http://example.org/wp-content/uploads/2018/05/brands.png'; + set_transient( Optml_Settings::INDIVIDUAL_CACHE_TOKENS_KEY, [ crc32( wp_basename( $url ) ) => 12345 ], HOUR_IN_SECONDS ); + + $result = Optml_Url_Replacer::instance()->build_url( $url ); + + delete_transient( Optml_Settings::INDIVIDUAL_CACHE_TOKENS_KEY ); + + $this->assertStringContainsString( '/cb:12345/', $result ); + } + + /** + * The active cache token is always a string, whatever type was stored. + */ + public function test_get_active_cache_booster_returns_string() { + $url = 'http://example.org/wp-content/uploads/2018/05/brands.png'; + + $this->assertSame( 'eFRn.20eff', Optml_Url_Replacer::get_active_cache_booster( $url, 'eFRn.20eff' ) ); + $this->assertSame( '987', Optml_Url_Replacer::get_active_cache_booster( $url, 987 ) ); + $this->assertSame( '', Optml_Url_Replacer::get_active_cache_booster( $url, null ) ); + + set_transient( Optml_Settings::INDIVIDUAL_CACHE_TOKENS_KEY, [ crc32( wp_basename( $url ) ) => 12345 ], HOUR_IN_SECONDS ); + $this->assertSame( '12345', Optml_Url_Replacer::get_active_cache_booster( $url, 'eFRn.20eff' ) ); + $this->assertSame( 'eFRn.20eff', Optml_Url_Replacer::get_active_cache_booster( 'http://example.org/other.png', 'eFRn.20eff' ) ); + delete_transient( Optml_Settings::INDIVIDUAL_CACHE_TOKENS_KEY ); + } +} From e29c89489228704df53e57c9772bc695396e129d Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Thu, 22 Oct 2026 00:01:46 +0300 Subject: [PATCH 2/7] Don't take a data: placeholder in a lazy-load attribute as the image URL (#1190) * fix: don't take a data: placeholder in a lazy-load attribute as the image URL The image regex matches the `src=` at the end of any `*-src=` attribute, so in `` it captured the placeholder. The tag replacer then ran against `data:image/gif`: no profiler srcset or dimensions, and a data-opt-id shared by every image with that placeholder. A lazy-load attribute whose value starts with data: is now skipped, so the real src is used. A data: value in a plain src is still matched, so images owned by another lazy-load script are handled as before. Fixes #1186 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WPwFxvA5VPq8sd4EzsNFFA * test: cover data: placeholders without a src and their lazy-load result Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WPwFxvA5VPq8sd4EzsNFFA --------- Co-authored-by: Claude Opus 5.5 (1M context) --- inc/manager.php | 4 ++- tests/test-dimension-replacement.php | 11 ++++++++ tests/test-lazyload.php | 30 ++++++++++++++++++++++ tests/test-replacer.php | 38 ++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/inc/manager.php b/inc/manager.php index 587fe756..e855de25 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -698,7 +698,9 @@ public static function parse_images_from_html( $content ) { if ( OPTML_DEBUG ) { do_action( 'optml_log', 'Content to parse images from: ' . $content ); } - $regex = '/(?:]+?href=["|\'](?P[^\s]+?)["|\'][^>]*?>\s*)?(?P(?:\s*)?]*?\s?(?:' . implode( '|', array_merge( [ 'src' ], Optml_Tag_Replacer::possible_src_attributes() ) ) . ')=["\'\\\\]*?(?P[' . Optml_Config::$chars . ']{10,}).*?>(?:\s*<\/noscript\s*>)?){1}(?:\s*<\/a>)?/ismu'; + // A lazy-load attribute (e.g. data-src) holding a data: placeholder is skipped, so the real src is used. + // A data: src is still matched: the image then belongs to another lazy-load script. + $regex = '/(?:]+?href=["|\'](?P[^\s]+?)["|\'][^>]*?>\s*)?(?P(?:\s*)?]*?(?:\ssrc=["\'\\\\]*?|\s?(?:' . implode( '|', array_merge( [ 'src' ], Optml_Tag_Replacer::possible_src_attributes() ) ) . ')=["\'\\\\]*?(?!data:))(?P[' . Optml_Config::$chars . ']{10,}).*?>(?:\s*<\/noscript\s*>)?){1}(?:\s*<\/a>)?/ismu'; if ( preg_match_all( $regex, $content, $images, PREG_OFFSET_CAPTURE ) ) { if ( OPTML_DEBUG ) { diff --git a/tests/test-dimension-replacement.php b/tests/test-dimension-replacement.php index 32640ad4..6d389b03 100644 --- a/tests/test-dimension-replacement.php +++ b/tests/test-dimension-replacement.php @@ -117,6 +117,17 @@ public function test_existing_attributes_preserved() { } + /** + * A data: placeholder in data-src before src does not stop the real image from being processed. + */ + public function test_dimensions_injected_when_data_src_placeholder_comes_first() { + $html_input = 'Test image'; + + $result = $this->tag_replacer->process_image_tags( $html_input, $this->extract_images_from_html( $html_input ) ); + + $this->assertMatchesRegularExpression( '/width="1920".*height="1080".*data-opt-id=' . $this->tag_replacer->get_id_by_url( self::IMAGE_URL ) . '/', $result ); + } + /** * Helper method to extract images from HTML for testing. * Uses the actual manager method to ensure correct structure. diff --git a/tests/test-lazyload.php b/tests/test-lazyload.php index db132d0a..a7bded32 100644 --- a/tests/test-lazyload.php +++ b/tests/test-lazyload.php @@ -553,4 +553,34 @@ public function test_dam_lazyloading_no_wh_attributes() { $this->assertStringContainsString( $svg, $replaced_content ); remove_filter('optml_lazyload_images_skip','__return_zero'); } + + /** + * A tag whose data-src holds a data: placeholder is lazy-loaded from its real URL. + */ + public function test_lazyload_tag_with_data_uri_data_src() { + $settings = new Optml_Settings(); + $settings->update( 'skip_lazyload_images', 0 ); + // The skip limit is cached in a static on first use, so an earlier test may have cached another value. + $skip_limit = new ReflectionProperty( Optml_Lazyload_Replacer::class, 'skip_lazyload_images' ); + $skip_limit->setAccessible( true ); + $skip_limit->setValue( null, null ); + Optml_Tag_Replacer::$lazyload_skipped_images = 0; + Optml_Manager::instance()->init(); + + $placeholder = 'data:image/gif;base64,R0lGODlhAQABAIAAAP///////yH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=='; + $tags = [ + 'Test', + 'Test', + ]; + + foreach ( $tags as $tag ) { + $replaced_content = Optml_Manager::instance()->replace_content( $tag ); + + $this->assertStringContainsString( 'data-opt-src="https://test123.i.optimole.com/', $replaced_content, $tag ); + $this->assertStringContainsString( '/https://example.com/wp-content/uploads/2026/10/photo.jpg"', $replaced_content, $tag ); + $this->assertStringContainsString( 'data-src="' . $placeholder . '"', $replaced_content, $tag ); + } + + $skip_limit->setValue( null, null ); + } } diff --git a/tests/test-replacer.php b/tests/test-replacer.php index 231e4407..701acfbc 100644 --- a/tests/test-replacer.php +++ b/tests/test-replacer.php @@ -850,6 +850,44 @@ public function test_dam_flag_removal() { $this->assertStringContainsString('q:mauto', $replaced_content); } + /** + * A data: placeholder in a lazy-load attribute before src is not taken as the image URL. + */ + public function test_parse_images_skips_data_uri_in_lazy_attribute() { + $placeholder = 'data:image/gif;base64,R0lGODlhAQABAIAAAP///////yH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=='; + $real_src = 'https://example.com/wp-content/uploads/2026/10/photo.jpg'; + + $tags = [ + 'Test', + "Test", + 'Test', + 'Test', + '', + ]; + + foreach ( $tags as $tag ) { + $images = Optml_Manager::parse_images_from_html( $tag ); + $this->assertSame( [ $real_src ], $images['img_url'], $tag ); + $this->assertSame( [ $tag ], $images['img_tag'], $tag ); + } + + $placeholder_only = 'Test'; + $this->assertSame( [], Optml_Manager::parse_images_from_html( $placeholder_only ) ); + + $placeholder_and_lazy_src = 'Test'; + $this->assertSame( [ $real_src ], Optml_Manager::parse_images_from_html( $placeholder_and_lazy_src )['img_url'] ); + } + + /** + * A data: src is still matched, so an image owned by another lazy-load script is handled as before. + */ + public function test_parse_images_keeps_data_uri_src() { + $tag = ''; + $images = Optml_Manager::parse_images_from_html( $tag ); + + $this->assertSame( [ 'data:image/gif' ], $images['img_url'] ); + } + /** * Individual cache tokens are stored as the API returned them, which can be a number. * The SDK only accepts a string cache buster. From bf17ac96954107dab45f43522b75fde530bcb88a Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Thu, 22 Oct 2026 00:01:52 +0300 Subject: [PATCH 3/7] fix: stop retrying a failing transfer step forever (#1194) * fix: stop retrying a failing transfer step forever When a background transfer step threw (an API error, a lookup that keeps failing), it was rescheduled 10 seconds later with the same step, with no limit. The transfer stayed "in progress" in the dashboard and never finished. The failed attempts of a step are now counted; after 5 in a row the chain moves on to the next step, so the transfer can reach its end and the images left behind can be retried. A successful step clears the count. Refs #1108 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Keep step failures per transfer in an option and don't report success after skips Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Fix the finish branch comment Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p --------- Co-authored-by: Claude --- inc/media_offload.php | 125 ++++++++++++++++++++++++++++++++++++++- tests/test-media.php | 133 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+), 2 deletions(-) diff --git a/inc/media_offload.php b/inc/media_offload.php index c08ab51d..397478b6 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -67,6 +67,17 @@ class Optml_Media_Offload extends Optml_App_Replacer { */ const TRANSFER_LOCK_TTL = 600; + /** + * Option counting the failed attempts of the current transfer step, and the steps skipped so far. + * It is an option rather than a transient so an object cache eviction can't reset the count. + */ + const STEP_FAILURES_OPTION = 'optml_transfer_step_failures'; + + /** + * How many times a failing transfer step is retried before the chain moves on. + */ + const MAX_STEP_RETRIES = 5; + /** * Flag used inside wp_get_attachment url filter. * @@ -1988,6 +1999,7 @@ public static function move_images( $action, $refresh ) { $lock_token = self::acquire_transfer_lock( $action ); if ( false !== $lock_token ) { + delete_option( self::STEP_FAILURES_OPTION ); $total = ceil( $count / $batch ); self::schedule_action( time(), @@ -2274,9 +2286,16 @@ public function start_processing_images( $action, $batch, $page, $total, $step, self::$instance->settings->update( $option, 'disabled' ); - self::$instance->settings->update( 'show_offload_finish_notice', $type ); + // A transfer that skipped failing steps left images behind, so it does not report success. + $skipped = self::get_skipped_steps( $lock_token ); + delete_option( self::STEP_FAILURES_OPTION ); + if ( $skipped > 0 ) { + self::$instance->logger->add_log( $type, $skipped . ' step(s) were skipped after failing ' . self::MAX_STEP_RETRIES . ' times. Start the transfer again to move the images left behind.' ); + } else { + self::$instance->settings->update( 'show_offload_finish_notice', $type ); + } - // Transfer completed successfully: release the lock. + // Transfer finished: release the lock. self::release_transfer_lock( $lock_token ); return; @@ -2322,12 +2341,22 @@ public function start_processing_images( $action, $batch, $page, $total, $step, $lock_token, ] ); + self::clear_step_failures( $lock_token ); } catch ( Exception $e ) { // Reschedule the cron to run again after a delay. Sometimes memory limit is exausted. // This is a retryable error, so the lock is kept rather than released. $delay_in_seconds = 10; self::$instance->logger->add_log( $type, $e->getMessage() ); + // A step that keeps failing would be retried every few seconds forever, with the transfer + // shown as running. After a few attempts the chain moves on, so the transfer can finish + // and the images left behind can be retried. + if ( self::count_step_failure( $lock_token, $action, $step ) >= self::MAX_STEP_RETRIES ) { + self::record_skipped_step( $lock_token ); + self::$instance->logger->add_log( $type, 'Step ' . $step . ' failed ' . self::MAX_STEP_RETRIES . ' times, moving on to the next one.' ); + $step = $step + 1; + } + self::schedule_action( time() + $delay_in_seconds, 'optml_start_processing_images', @@ -2345,6 +2374,98 @@ public function start_processing_images( $action, $batch, $page, $total, $step, } } + /** + * Read the step failures record of a transfer chain. + * + * @param string $lock_token The transfer lock token of the chain. + * + * @return array{token: string, action: string, step: int, count: int, skipped: int} The record, empty for another chain. + */ + private static function get_step_failures( string $lock_token ): array { + $failures = get_option( self::STEP_FAILURES_OPTION ); + $record = [ + 'token' => $lock_token, + 'action' => '', + 'step' => 0, + 'count' => 0, + 'skipped' => 0, + ]; + if ( ! is_array( $failures ) || ! isset( $failures['token'] ) || $failures['token'] !== $lock_token ) { + return $record; + } + + return [ + 'token' => $lock_token, + 'action' => isset( $failures['action'] ) ? (string) $failures['action'] : '', + 'step' => isset( $failures['step'] ) ? (int) $failures['step'] : 0, + 'count' => isset( $failures['count'] ) ? (int) $failures['count'] : 0, + 'skipped' => isset( $failures['skipped'] ) ? (int) $failures['skipped'] : 0, + ]; + } + + /** + * Count a failed attempt of a transfer step. + * + * @param string $lock_token The transfer lock token of the chain. + * @param string $action The transfer action. + * @param int $step The step that failed. + * + * @return int How many times in a row this step has failed. + */ + private static function count_step_failure( string $lock_token, string $action, int $step ): int { + $record = self::get_step_failures( $lock_token ); + if ( $record['action'] !== $action || $record['step'] !== $step ) { + $record['count'] = 0; + } + $record['action'] = $action; + $record['step'] = $step; + ++$record['count']; + update_option( self::STEP_FAILURES_OPTION, $record, false ); + + return $record['count']; + } + + /** + * Remember that the chain skipped a step that kept failing. + * + * @param string $lock_token The transfer lock token of the chain. + * + * @return void + */ + private static function record_skipped_step( string $lock_token ): void { + $record = self::get_step_failures( $lock_token ); + $record['count'] = 0; + ++$record['skipped']; + update_option( self::STEP_FAILURES_OPTION, $record, false ); + } + + /** + * Clear the failed attempts after a step succeeds, keeping the count of skipped steps. + * + * @param string $lock_token The transfer lock token of the chain. + * + * @return void + */ + private static function clear_step_failures( string $lock_token ): void { + $record = self::get_step_failures( $lock_token ); + if ( 0 === $record['count'] ) { + return; + } + $record['count'] = 0; + update_option( self::STEP_FAILURES_OPTION, $record, false ); + } + + /** + * How many steps the chain skipped after they kept failing. + * + * @param string $lock_token The transfer lock token of the chain. + * + * @return int + */ + public static function get_skipped_steps( string $lock_token ): int { + return self::get_step_failures( $lock_token )['skipped']; + } + /** * Alter attachment image src for offloaded images. * diff --git a/tests/test-media.php b/tests/test-media.php index f8a9eaa1..85605bea 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -543,6 +543,139 @@ public function test_background_step_does_nothing_while_another_run_moves_images $this->assert_moving_images_lock_is_free(); } + /** + * A step that throws every time is retried a few times, then the chain moves on to the next step + * instead of retrying it every few seconds forever. + */ + public function test_failing_step_moves_on_after_retries(): void { + ( new Optml_Settings() )->update( 'rollback_status', 'enabled' ); + delete_option( Optml_Media_Offload::STEP_FAILURES_OPTION ); + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + $fail = function () { + throw new Exception( 'cloud lookup failed' ); + }; + add_action( 'pre_get_posts', $fail ); + + $steps = []; + for ( $attempt = 1; $attempt <= Optml_Media_Offload::MAX_STEP_RETRIES; $attempt++ ) { + _set_cron_array( [] ); + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 1, 5, 1, $token ); + $steps[] = $this->scheduled_transfer_step(); + } + remove_action( 'pre_get_posts', $fail ); + + $this->assertSame( array_merge( array_fill( 0, Optml_Media_Offload::MAX_STEP_RETRIES - 1, 1 ), [ 2 ] ), $steps ); + $this->assertSame( 1, Optml_Media_Offload::get_skipped_steps( $token ) ); + $this->assertSame( 0, get_option( Optml_Media_Offload::STEP_FAILURES_OPTION )['count'] ); + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + } + + /** + * A step that succeeds clears the failures of the one before it, but the skipped steps are kept. + */ + public function test_successful_step_clears_failures(): void { + ( new Optml_Settings() )->update( 'rollback_status', 'enabled' ); + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + update_option( Optml_Media_Offload::STEP_FAILURES_OPTION, $this->step_failures( $token, Optml_Media_Offload::MAX_STEP_RETRIES - 1, 1 ), false ); + + _set_cron_array( [] ); + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 1, 5, 1, $token ); + + $this->assertSame( 2, $this->scheduled_transfer_step() ); + $this->assertSame( 0, get_option( Optml_Media_Offload::STEP_FAILURES_OPTION )['count'] ); + $this->assertSame( 1, Optml_Media_Offload::get_skipped_steps( $token ) ); + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + } + + /** + * Failures left by an earlier transfer don't count toward the retries of a new one. + */ + public function test_failures_of_another_transfer_are_ignored(): void { + ( new Optml_Settings() )->update( 'rollback_status', 'enabled' ); + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + update_option( Optml_Media_Offload::STEP_FAILURES_OPTION, $this->step_failures( 'old-transfer', Optml_Media_Offload::MAX_STEP_RETRIES - 1, 2 ), false ); + $fail = function () { + throw new Exception( 'cloud lookup failed' ); + }; + add_action( 'pre_get_posts', $fail ); + + _set_cron_array( [] ); + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 1, 5, 1, $token ); + remove_action( 'pre_get_posts', $fail ); + + $this->assertSame( 1, $this->scheduled_transfer_step() ); + $this->assertSame( 1, get_option( Optml_Media_Offload::STEP_FAILURES_OPTION )['count'] ); + $this->assertSame( 0, Optml_Media_Offload::get_skipped_steps( $token ) ); + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + } + + /** + * A transfer that skipped steps does not show the "Transfer Complete" notice. + */ + public function test_transfer_with_skipped_steps_does_not_report_success(): void { + $settings = new Optml_Settings(); + $settings->update( 'rollback_status', 'enabled' ); + $settings->update( 'show_offload_finish_notice', '' ); + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + update_option( Optml_Media_Offload::STEP_FAILURES_OPTION, $this->step_failures( $token, 0, 1 ), false ); + + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 6, 5, 6, $token ); + + $settings = new Optml_Settings(); + $this->assertSame( 'disabled', $settings->get( 'rollback_status' ) ); + $this->assertSame( '', $settings->get( 'show_offload_finish_notice' ) ); + $this->assertFalse( get_option( Optml_Media_Offload::STEP_FAILURES_OPTION ) ); + } + + /** + * A transfer that ran every step shows the "Transfer Complete" notice. + */ + public function test_transfer_without_skipped_steps_reports_success(): void { + $settings = new Optml_Settings(); + $settings->update( 'rollback_status', 'enabled' ); + $settings->update( 'show_offload_finish_notice', '' ); + $token = Optml_Media_Offload::acquire_transfer_lock( 'rollback_images' ); + update_option( Optml_Media_Offload::STEP_FAILURES_OPTION, $this->step_failures( 'old-transfer', 0, 3 ), false ); + + Optml_Media_Offload::instance()->start_processing_images( 'rollback_images', 10, 6, 5, 6, $token ); + + $this->assertSame( 'rollback', ( new Optml_Settings() )->get( 'show_offload_finish_notice' ) ); + } + + /** + * Build a step failures record. + * + * @param string $token The transfer lock token. + * @param int $count The failed attempts of step 1. + * @param int $skipped The skipped steps. + * + * @return array{token: string, action: string, step: int, count: int, skipped: int} + */ + private function step_failures( string $token, int $count, int $skipped ): array { + return [ + 'token' => $token, + 'action' => 'rollback_images', + 'step' => 1, + 'count' => $count, + 'skipped' => $skipped, + ]; + } + + /** + * @return int|null The step of the background transfer step waiting in WP-Cron. + */ + private function scheduled_transfer_step(): ?int { + foreach ( (array) _get_cron_array() as $hooks ) { + if ( isset( $hooks['optml_start_processing_images'] ) ) { + $event = reset( $hooks['optml_start_processing_images'] ); + + return (int) $event['args'][4]; + } + } + + return null; + } + /** * @return int The background transfer steps waiting in WP-Cron. */ From 5a1421f7d1399332e09a787f38f080e93ca53854 Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Thu, 22 Oct 2026 01:01:35 +0300 Subject: [PATCH 4/7] feat: let an image keep its own sizes attribute (#1193) * feat: let an image keep its own sizes attribute The page profiler adds its measured breakpoints to an image's sizes attribute unless the value contains calc() or a similar formula. Sites with hand-written sizes had no supported way to keep them. An image with data-optml-keep-sizes now keeps its existing sizes attribute (srcset candidates are still added). The optml_keep_existing_sizes filter does the same site-wide without changing the markup. Refs #1115 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Clarify keep-sizes log, FAQ and tests Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Check the attribute keeps sizes before the filter overrides it Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p --------- Co-authored-by: Claude --- README.md | 8 ++++++ inc/tag_replacer.php | 40 ++++++++++++++++++++++++++--- readme.txt | 8 ++++++ tests/test-srcset.php | 59 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 76601f34..8da99532 100644 --- a/README.md +++ b/README.md @@ -345,3 +345,11 @@ add_filter('optml_dont_replace_url', function( $old, $url ) { }, 10, 2); + += Can I keep the sizes attribute I wrote for an image ? = + +Yes. Add the `data-optml-keep-sizes` attribute to the image tag and the page profiler won't add its own entries to the image's `sizes` attribute. Images lazy loaded by Optimole don't keep `srcset` or `sizes`, since the lazy loader doesn't need them. To keep the existing `sizes` attribute on every image, add this snippet: + + +add_filter( 'optml_keep_existing_sizes', '__return_true' ); + diff --git a/inc/tag_replacer.php b/inc/tag_replacer.php index 3330edb2..0df7d581 100644 --- a/inc/tag_replacer.php +++ b/inc/tag_replacer.php @@ -13,6 +13,11 @@ final class Optml_Tag_Replacer extends Optml_App_Replacer { use Optml_Validator; use Optml_Dam_Offload_Utils; + /** + * Attribute an image uses to keep its own sizes attribute. + */ + const KEEP_SIZES_ATTRIBUTE = 'data-optml-keep-sizes'; + /** * Cached object instance. * @@ -550,11 +555,16 @@ public function add_missing_srcset_attributes( $tag, $missing_srcsets, $new_url, $tag = $this->insert_after_attribute( $tag, 'src', $srcset_attr ); } - // Handle sizes attribute - skip if existing sizes contains calc() or complex formulas + // Handle sizes attribute - skip if the image asks to keep its existing sizes, or they contain calc() or complex formulas if ( ! empty( $new_sizes_entries ) ) { $should_skip_sizes = false; - if ( $has_existing_sizes ) { + if ( $has_existing_sizes && $this->keeps_existing_sizes( $tag ) ) { + $should_skip_sizes = true; + if ( OPTML_DEBUG ) { + do_action( 'optml_log', 'Keeping the existing sizes attribute: ' . $new_url ); + } + } elseif ( $has_existing_sizes ) { // Check if existing sizes contains calc() or complex formulas $sizes = $this->find_quoted_attribute( $tag, 'sizes' ); if ( null !== $sizes ) { @@ -596,9 +606,9 @@ function ( $a, $b ) { } } } else { - // Log that sizes were skipped due to complex formulas + // Log that sizes were skipped (kept on request, or complex formula) if ( OPTML_DEBUG ) { - do_action( 'optml_log', 'Skipped adding sizes due to complex formula detection' ); + do_action( 'optml_log', 'Skipped adding sizes' ); } } } @@ -834,6 +844,28 @@ private function has_attribute( string $tag, string $name ): bool { return in_array( $name, array_column( $this->get_attributes( $tag ) ?? [], 'name' ), true ); } + /** + * Check whether an image's own sizes attribute must be left as it is. + * + * An image opts out with the data-optml-keep-sizes attribute. The optml_keep_existing_sizes filter + * does it for every image, or for the ones it picks, with no change to the markup. + * + * @param string $tag The image tag. + * + * @return bool True if the existing sizes attribute is kept. + */ + private function keeps_existing_sizes( string $tag ): bool { + $keep = $this->has_attribute( $tag, self::KEEP_SIZES_ATTRIBUTE ); + + /** + * Filters whether Optimole leaves an image's existing sizes attribute unchanged. + * + * @param bool $keep Whether the existing sizes attribute is kept. True when the image has data-optml-keep-sizes. + * @param string $tag The image tag. + */ + return (bool) apply_filters( 'optml_keep_existing_sizes', $keep, $tag ); + } + /** * Check whether the tag's src or srcset is a data: URI placeholder. * diff --git a/readme.txt b/readme.txt index 89ca3fab..da7c72b0 100755 --- a/readme.txt +++ b/readme.txt @@ -345,3 +345,11 @@ add_filter('optml_dont_replace_url', function( $old, $url ) { }, 10, 2); + += Can I keep the sizes attribute I wrote for an image ? = + +Yes. Add the `data-optml-keep-sizes` attribute to the image tag and the page profiler won't add its own entries to the image's `sizes` attribute. Images lazy loaded by Optimole don't keep `srcset` or `sizes`, since the lazy loader doesn't need them. To keep the existing `sizes` attribute on every image, add this snippet: + + +add_filter( 'optml_keep_existing_sizes', '__return_true' ); + diff --git a/tests/test-srcset.php b/tests/test-srcset.php index 5b90d7d6..3f8c6b3d 100644 --- a/tests/test-srcset.php +++ b/tests/test-srcset.php @@ -168,6 +168,65 @@ public function test_add_missing_srcset_attributes_with_existing_sizes() { $this->assertStringContainsString( '(max-width: 768px) 400px', $result ); } + /** + * An image with data-optml-keep-sizes keeps its hand-written sizes, and still gets the new srcset entries. + */ + public function test_keep_sizes_attribute_preserves_existing_sizes() { + $tag_replacer = new Optml_Tag_Replacer(); + + $tag = 'Test'; + $missing_srcsets = [ + [ 'w' => 200, 'h' => 150, 'd' => 1, 's' => '200w', 'b' => 320 ], + [ 'w' => 400, 'h' => 300, 'd' => 1, 's' => '400w', 'b' => 768 ], + ]; + + $result = $tag_replacer->add_missing_srcset_attributes( $tag, $missing_srcsets, 'https://example.com/image.jpg', false ); + + $this->assertStringContainsString( 'sizes="(max-width: 480px) 300px"', $result ); + $this->assertStringNotContainsString( '(max-width: 320px) 200px', $result ); + $this->assertStringContainsString( '200w', $result ); + } + + /** + * The optml_keep_existing_sizes filter keeps existing sizes site-wide, with no change to the markup. + */ + public function test_keep_existing_sizes_filter() { + $tag_replacer = new Optml_Tag_Replacer(); + + $tag = 'Test'; + $missing_srcsets = [ + [ 'w' => 200, 'h' => 150, 'd' => 1, 's' => '200w', 'b' => 320 ], + ]; + + add_filter( 'optml_keep_existing_sizes', '__return_true' ); + $result = $tag_replacer->add_missing_srcset_attributes( $tag, $missing_srcsets, 'https://example.com/image.jpg', false ); + remove_filter( 'optml_keep_existing_sizes', '__return_true' ); + + $this->assertStringContainsString( 'sizes="(max-width: 480px) 300px"', $result ); + $this->assertStringNotContainsString( '(max-width: 320px) 200px', $result ); + } + + /** + * A filter returning false overrides the attribute, so the existing sizes are enhanced again. + */ + public function test_keep_existing_sizes_filter_can_override_attribute() { + $tag_replacer = new Optml_Tag_Replacer(); + + $tag = 'Test'; + $missing_srcsets = [ + [ 'w' => 200, 'h' => 150, 'd' => 1, 's' => '200w', 'b' => 320 ], + ]; + + $kept = $tag_replacer->add_missing_srcset_attributes( $tag, $missing_srcsets, 'https://example.com/image.jpg', false ); + $this->assertStringNotContainsString( '(max-width: 320px) 200px', $kept ); + + add_filter( 'optml_keep_existing_sizes', '__return_false' ); + $result = $tag_replacer->add_missing_srcset_attributes( $tag, $missing_srcsets, 'https://example.com/image.jpg', false ); + remove_filter( 'optml_keep_existing_sizes', '__return_false' ); + + $this->assertStringContainsString( '(max-width: 320px) 200px', $result ); + } + /** * Test retina images setting respect */ From 8e4a54b30e07c7ed47773ba6abb9c702c890f339 Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Thu, 22 Oct 2026 11:02:53 +0300 Subject: [PATCH 5/7] fix: bound the in-memory attachment ID cache (#1192) * fix: bound the in-memory attachment ID cache The request-local map in Optml_Attachment_Cache kept every distinct URL looked up during a request, including misses, with no limit. It now keeps at most 2000 URLs and drops the older half when full; dropped URLs are still served from the persistent cache (transient or object cache). Refs #1139 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Cover the cache-miss path in the memory cap test and fix the value type Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p --------- Co-authored-by: Claude --- inc/attachment_cache.php | 28 +++++++++-- tests/test-attachment-cache.php | 82 +++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 tests/test-attachment-cache.php diff --git a/inc/attachment_cache.php b/inc/attachment_cache.php index 64fa4a62..392caa9c 100644 --- a/inc/attachment_cache.php +++ b/inc/attachment_cache.php @@ -5,6 +5,13 @@ */ class Optml_Attachment_Cache { const CACHE_GROUP = 'om_att'; + /** + * Most URLs kept in the in-memory map during one request. + * + * A page with many distinct image URLs would otherwise grow the map for the whole request. + * When it is full, the older half is dropped; those URLs are still in the persistent cache. + */ + const MAX_MEMORY_ENTRIES = 2000; /** * Local cache map. * @@ -32,10 +39,10 @@ public static function get_cached_attachment_id( $url ) { return self::$cache_map[ $cache_key ]; } - $value = wp_using_ext_object_cache() + $value = wp_using_ext_object_cache() ? wp_cache_get( $cache_key, self::CACHE_GROUP ) : get_transient( self::CACHE_GROUP . $cache_key ); - self::$cache_map[ $cache_key ] = $value; + self::remember( $cache_key, $value ); return $value; } @@ -51,7 +58,7 @@ public static function get_cached_attachment_id( $url ) { public static function set_cached_attachment_id( $url, $id ) { $cache_key = self::get_cache_key( $url ); // We cache also in memory to avoid calling DB every time when not using Object Cache. - self::$cache_map[ $cache_key ] = $id; + self::remember( $cache_key, $id ); // If the ID is not found we cache for 10 minutes, otherwise for a week. // We try to reduce the cache time when is not found to // avoid caching for situation when this might be temporary. @@ -61,6 +68,21 @@ public static function set_cached_attachment_id( $url, $id ) { : set_transient( self::CACHE_GROUP . $cache_key, $id, $expiration ); } + /** + * Keep a value in the in-memory map, dropping the older half when it is full. + * + * @param string $cache_key The cache key. + * @param int|numeric-string|false $value The attachment ID (a numeric string when read back from a transient), or false when it is not cached. + * + * @return void + */ + private static function remember( $cache_key, $value ) { + if ( ! isset( self::$cache_map[ $cache_key ] ) && count( self::$cache_map ) >= self::MAX_MEMORY_ENTRIES ) { + self::$cache_map = array_slice( self::$cache_map, (int) ( self::MAX_MEMORY_ENTRIES / 2 ), null, true ); + } + self::$cache_map[ $cache_key ] = $value; + } + /** * Generate cache key for URL. * diff --git a/tests/test-attachment-cache.php b/tests/test-attachment-cache.php new file mode 100644 index 00000000..67ce1f29 --- /dev/null +++ b/tests/test-attachment-cache.php @@ -0,0 +1,82 @@ + + */ + private function memory_map() { + $property = new ReflectionProperty( Optml_Attachment_Cache::class, 'cache_map' ); + $property->setAccessible( true ); + + return $property->getValue(); + } + + /** + * A page with many distinct uncached image URLs must not grow the in-memory map for the whole request. + */ + public function test_memory_map_is_bounded_on_cache_misses() { + $limit = Optml_Attachment_Cache::MAX_MEMORY_ENTRIES; + + for ( $i = 0; $i <= $limit; $i++ ) { + Optml_Attachment_Cache::get_cached_attachment_id( 'https://example.org/wp-content/uploads/miss-' . $i . '.jpg' ); + } + + $this->assertCount( (int) ( $limit / 2 ) + 1, $this->memory_map() ); + } + + /** + * The newest URLs stay in memory when the older half is dropped, and the dropped ones are + * still found in the persistent cache. + */ + public function test_dropped_entries_are_still_found_in_the_persistent_cache() { + $limit = Optml_Attachment_Cache::MAX_MEMORY_ENTRIES; + + for ( $i = 0; $i <= $limit; $i++ ) { + Optml_Attachment_Cache::set_cached_attachment_id( 'https://example.org/wp-content/uploads/image-' . $i . '.jpg', $i + 1 ); + } + + $map = $this->memory_map(); + $this->assertCount( (int) ( $limit / 2 ) + 1, $map ); + $this->assertContains( $limit + 1, $map ); + $this->assertNotContains( 1, $map ); + + $this->assertSame( 1, (int) Optml_Attachment_Cache::get_cached_attachment_id( 'https://example.org/wp-content/uploads/image-0.jpg' ) ); + } + + /** + * Updating a URL that is already in memory never drops other entries. + */ + public function test_updating_a_known_url_keeps_the_map() { + $limit = Optml_Attachment_Cache::MAX_MEMORY_ENTRIES; + + for ( $i = 0; $i < $limit; $i++ ) { + Optml_Attachment_Cache::set_cached_attachment_id( 'https://example.org/wp-content/uploads/image-' . $i . '.jpg', $i + 1 ); + } + Optml_Attachment_Cache::set_cached_attachment_id( 'https://example.org/wp-content/uploads/image-0.jpg', 99 ); + + $this->assertCount( $limit, $this->memory_map() ); + $this->assertSame( 99, Optml_Attachment_Cache::get_cached_attachment_id( 'https://example.org/wp-content/uploads/image-0.jpg' ) ); + } +} From bf8f1148242a022b4d82697ee080d000f5b99fff Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Thu, 22 Oct 2026 12:02:08 +0300 Subject: [PATCH 6/7] fix: skip page processing after a fatal error to stop "Commands out of sync" logs (#1196) * fix: pass the page through unprocessed after a fatal error A fatal error that hits while MySQL is still sending rows leaves the connection out of sync, so every profiler and cache read at shutdown logged "Commands out of sync". Skip processing in that case. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * test: cover both fatal-error pass-through paths Read the last PHP error through a private seam so tests can simulate a fatal error. New buffer tests check that the default in-handler mode and the shutdown-capture mode both pass the page through unprocessed; each fails when its guard is removed. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * docs: give the error reader seam a precise callable type Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p --------- Co-authored-by: Claude --- inc/manager.php | 44 +++++++++++++ tests/test-fatal-error-passthrough.php | 87 ++++++++++++++++++++++++++ tests/test-zz-buffer.php | 65 +++++++++++++++++++ 3 files changed, 196 insertions(+) create mode 100644 tests/test-fatal-error-passthrough.php diff --git a/inc/manager.php b/inc/manager.php index e855de25..dc5dce2c 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -142,6 +142,13 @@ final class Optml_Manager { */ private static $ob_processed = false; + /** + * Reads the last PHP error; tests swap it to simulate a fatal error. + * + * @var callable(): (array{type: int, message: string, file: string, line: int}|null) + */ + private static $error_reader = 'error_get_last'; + /** * Class instance method. * @@ -985,6 +992,9 @@ public function handle_buffer_fallback( $content, $phase = 0 ) { if ( self::$ob_processed || $content === '' ) { return $content; } + if ( self::request_had_fatal_error() ) { + return $content; + } if ( ! $this->captures_at_shutdown() ) { try { return $this->replace_content( $content, self::is_ajax_request() ); @@ -1051,6 +1061,36 @@ public function close_final_buffer() { $this->capture_and_process_buffer( false ); } + /** + * Whether the request already died with a fatal error. + * + * A fatal error (memory exhausted, time limit) that hits while MySQL is still + * sending a query's rows leaves the connection out of sync, so every later + * query in the request fails with "Commands out of sync". The page is then + * passed through unprocessed, without the profiler and cache reads that would + * each log that error. + * + * @param array|null $error The last error, from error_get_last(). + * + * @return bool + */ + public static function is_fatal_error( $error ) { + if ( ! is_array( $error ) || ! isset( $error['type'] ) ) { + return false; + } + + return in_array( (int) $error['type'], [ E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR ], true ); + } + + /** + * Whether the current request already hit a fatal error. + * + * @return bool + */ + private static function request_had_fatal_error() { + return self::is_fatal_error( call_user_func( self::$error_reader ) ); + } + /** * Capture our buffer, process it outside the display-handler context and echo the result. * @@ -1087,6 +1127,10 @@ private function capture_and_process_buffer( $is_page = true ) { */ $html = apply_filters( 'optml_captured_page_html', $html ); } + if ( self::request_had_fatal_error() ) { + echo $html; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- full page HTML, passed through unprocessed. + return true; + } echo $this->replace_content( $html, self::is_ajax_request() ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- full page HTML, escaping would break the page. } return true; diff --git a/tests/test-fatal-error-passthrough.php b/tests/test-fatal-error-passthrough.php new file mode 100644 index 00000000..ef05c13e --- /dev/null +++ b/tests/test-fatal-error-passthrough.php @@ -0,0 +1,87 @@ +assertTrue( Optml_Manager::is_fatal_error( [ 'type' => $type, 'message' => 'Allowed memory size exhausted' ] ) ); + } + + /** + * @return array + */ + public function fatal_error_types(): array { + return [ + 'E_ERROR' => [ E_ERROR ], + 'E_PARSE' => [ E_PARSE ], + 'E_CORE_ERROR' => [ E_CORE_ERROR ], + 'E_COMPILE_ERROR' => [ E_COMPILE_ERROR ], + ]; + } + + /** + * Warnings, notices and deprecations don't stop the page from being processed. + * + * @dataProvider non_fatal_errors + * + * @param array|null $error The last error. + */ + public function test_other_errors_are_not_fatal( $error ): void { + $this->assertFalse( Optml_Manager::is_fatal_error( $error ) ); + } + + /** + * @return array|null}> + */ + public function non_fatal_errors(): array { + return [ + 'no error' => [ null ], + 'E_WARNING' => [ [ 'type' => E_WARNING ] ], + 'E_NOTICE' => [ [ 'type' => E_NOTICE ] ], + 'E_DEPRECATED' => [ [ 'type' => E_DEPRECATED ] ], + 'no type' => [ [ 'message' => 'x' ] ], + ]; + } + + /** + * A warning earlier in the request still lets the output handler process the page. + */ + public function test_output_handler_processes_page_after_a_warning(): void { + $settings = new Optml_Settings(); + $settings->update( + 'service_data', + [ + 'cdn_key' => 'test123', + 'cdn_secret' => '12345', + 'whitelist' => [ 'example.org' ], + ] + ); + $settings->update( 'lazyload', 'disabled' ); + Optml_Url_Replacer::instance()->init(); + Optml_Tag_Replacer::instance()->init(); + Optml_Manager::instance()->init(); + + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.PHP.DevelopmentFunctions.error_log_trigger_error -- sets error_get_last() to a warning. + @trigger_error( 'earlier warning', E_USER_WARNING ); + $html = ''; + + $result = Optml_Manager::instance()->handle_buffer_fallback( $html ); + + $this->assertStringContainsString( 'i.optimole.com', $result ); + } +} diff --git a/tests/test-zz-buffer.php b/tests/test-zz-buffer.php index a7f8e2fa..b6b595a8 100644 --- a/tests/test-zz-buffer.php +++ b/tests/test-zz-buffer.php @@ -56,6 +56,7 @@ public function tearDown(): void { } } $this->reset_buffer_state(); + $this->set_error_reader( 'error_get_last' ); remove_filter( 'optml_capture_at_shutdown', '__return_true' ); remove_filter( 'optml_capture_at_shutdown', '__return_false' ); parent::tearDown(); @@ -75,6 +76,70 @@ private function reset_buffer_state( $processed = false ) { } } + /** + * Swap how Optml_Manager reads the last PHP error. + * + * @param callable(): (array{type: int, message: string, file: string, line: int}|null) $reader Returns what error_get_last() would. + */ + private function set_error_reader( $reader ) { + $prop = new ReflectionProperty( Optml_Manager::class, 'error_reader' ); + $prop->setAccessible( true ); + $prop->setValue( null, $reader ); + } + + /** + * Simulate a request that already died with a fatal error. + */ + private function simulate_fatal_error() { + $this->set_error_reader( + function () { + return [ + 'type' => E_ERROR, + 'message' => 'Allowed memory size of 8388608 bytes exhausted', + 'file' => __FILE__, + 'line' => __LINE__, + ]; + } + ); + } + + /** + * After a fatal error the default in-handler mode passes the page through unprocessed. + */ + public function test_default_mode_passes_page_through_after_fatal_error() { + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $this->simulate_fatal_error(); + $manager->close_buffer(); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( self::IMG_TAGS, $out ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * After a fatal error the shutdown-capture mode echoes the page once, unprocessed, + * and late shutdown output is passed through too. + */ + public function test_capture_mode_passes_page_through_after_fatal_error() { + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $this->simulate_fatal_error(); + $manager->close_buffer(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( self::IMG_TAGS . self::IMG_TAGS, $out ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + /** * Callbacks on our filters may use output buffering without fataling. * From 863751c3c5edb9fb5025cd8554817f973aec1a97 Mon Sep 17 00:00:00 2001 From: Marius Cristea Date: Thu, 22 Oct 2026 13:03:05 +0300 Subject: [PATCH 7/7] fix: make the transfer lock atomic and clean up files left by interrupted rollbacks (#1191) * fix: make the transfer lock atomic and clean up files left by interrupted rollbacks Two requests that started a transfer at the same moment could both take the transfer lock, because it was read and then written. The lock is now stored with an insert that fails when the row already exists (wp_cache_add with a persistent object cache). A rollback that died after moving the restored file into uploads left that file behind, so the retry got a suffixed name (.1-1.webp) while references kept the old one. The moved file is now recorded on the attachment until the rollback finishes, the retry removes it first, and the attachment points to the restored file right after the move instead of after the sub-sizes. Refs #1136 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * fix: reuse the file left by an interrupted rollback instead of deleting it Review follow-up on the rollback cleanup: - the retry writes the downloaded image over the file the interrupted attempt left behind, instead of deleting it before the sideload, so a failed move never leaves the attachment pointing at a missing file; - the pending-file meta is also cleared when the retry ends in the branch for attachments whose local metadata was already saved; - the uploads-dir check normalizes both paths, so it works on Windows; - the lock expiry is written again after the insert; - native parameter and return types on the new helpers. Refs #1136 Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Write the restored file through a temporary copy and keep the lock expiry after a racing release Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p * Add native types to the failing stream test helper Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MEXWtYdE6e3ALYtqtP8j2p --------- Co-authored-by: Claude --- inc/media_offload.php | 179 +++++++++++++++++++++++++++---- tests/test-media.php | 244 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 405 insertions(+), 18 deletions(-) diff --git a/inc/media_offload.php b/inc/media_offload.php index 397478b6..c0080bcb 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -57,6 +57,11 @@ class Optml_Media_Offload extends Optml_App_Replacer { const POST_ROLLBACK_FLAG = 'optimole_rollback_post'; const RETRYABLE_META_COUNTER = '_optimole_retryable_errors'; + /** + * Post meta holding the file a rollback moved into uploads before the attachment was updated to it. + */ + const ROLLBACK_PENDING_FILE = '_optimole_rollback_pending_file'; + /** * Transient name for the transfer lock. */ @@ -885,6 +890,8 @@ public function rollback_and_update_images( $image_ids ) { if ( ! isset( $current_meta['file'] ) || ! self::is_uploaded_image( $current_meta['file'] ) ) { delete_post_meta( $id, self::META_KEYS['offloaded'] ); delete_post_meta( $id, self::OM_OFFLOADED_FLAG ); + // A rollback that died after WordPress saved the local metadata ends here on retry. + delete_post_meta( $id, self::ROLLBACK_PENDING_FILE ); ++$success_back; continue; } @@ -977,8 +984,21 @@ public function rollback_and_update_images( $image_ids ) { continue; } - // Move the temporary file into the uploads directory. - $results = wp_handle_sideload( $file, $overrides, get_the_date( 'Y/m', $id ) ); + if ( ! function_exists( 'update_attached_file' ) ) { + include_once ABSPATH . '/wp-admin/includes/post.php'; + } + if ( ! function_exists( 'update_attached_file' ) ) { + update_post_meta( $id, self::META_KEYS['rollback_error'], 'true' ); + continue; + } + + // A file left behind by an interrupted attempt is replaced in place: a new sideload would get a + // suffixed name, and deleting it first would leave the attachment without a file if the move fails. + $pending = self::get_pending_rollback_file( $id ); + $results = null !== $pending + ? self::replace_pending_rollback_file( $pending, $temp_file, $type ) + // Move the temporary file into the uploads directory. + : wp_handle_sideload( $file, $overrides, get_the_date( 'Y/m', $id ) ); if ( ! empty( $results['error'] ) ) { if ( OPTML_DEBUG_MEDIA ) { do_action( 'optml_log', ' wp_handle_sideload error' ); @@ -989,6 +1009,13 @@ public function rollback_and_update_images( $image_ids ) { continue; } + // Remember the moved file until the attachment points to it, so a retry can clean it up. + update_post_meta( $id, self::ROLLBACK_PENDING_FILE, $results['file'] ); + + // Point the attachment at the restored file right away: generating the sub-sizes is the + // slow part, and a request that dies there must not leave the stored path behind. + update_attached_file( $id, $results['file'] ); + if ( ! function_exists( 'wp_create_image_subsizes' ) ) { include_once ABSPATH . '/wp-admin/includes/image.php'; } @@ -1014,14 +1041,7 @@ public function rollback_and_update_images( $image_ids ) { wp_update_attachment_metadata( $id, $meta ); } - if ( ! function_exists( 'update_attached_file' ) ) { - include_once ABSPATH . '/wp-admin/includes/post.php'; - } - if ( ! function_exists( 'update_attached_file' ) ) { - update_post_meta( $id, self::META_KEYS['rollback_error'], 'true' ); - continue; - } - update_attached_file( $id, $results['file'] ); + delete_post_meta( $id, self::ROLLBACK_PENDING_FILE ); $duplicated_images = apply_filters( 'optml_offload_duplicated_images', [], $id ); if ( is_array( $duplicated_images ) && ! empty( $duplicated_images ) ) { @@ -1063,6 +1083,75 @@ public function rollback_and_update_images( $image_ids ) { return $success_back; } + /** + * Get the file an interrupted rollback of this attachment moved into uploads. + * + * Only an existing file inside the uploads directory is returned, so a corrupted meta value can + * never point the rollback at anything else. + * + * @param int $id The attachment ID. + * + * @return string|null The absolute path, or null when there is no usable file. + */ + public static function get_pending_rollback_file( int $id ): ?string { + $pending = get_post_meta( $id, self::ROLLBACK_PENDING_FILE, true ); + if ( empty( $pending ) || ! is_string( $pending ) ) { + return null; + } + + $uploads = wp_get_upload_dir(); + $base_dir = realpath( $uploads['basedir'] ); + $file = realpath( $pending ); + if ( false === $base_dir || false === $file || ! is_file( $file ) ) { + return null; + } + + $base_dir = trailingslashit( wp_normalize_path( $base_dir ) ); + if ( strpos( wp_normalize_path( $file ), $base_dir ) !== 0 ) { + return null; + } + + // The stored path, not the resolved one: WordPress stores attached files relative to the uploads path as configured. + return $pending; + } + + /** + * Put the downloaded image in place of the file an interrupted rollback left behind. + * + * The image is copied next to the old file first and then renamed over it, so a copy that fails + * partway (full disk, file size limit) leaves the old file intact. + * + * @param string $pending The file left behind, from {@see self::get_pending_rollback_file()}. + * @param string $temp_file The downloaded image. + * @param string $type The image MIME type. + * + * @return array{file?: string, url?: string, type?: string, error?: string} The same shape as wp_handle_sideload(). + */ + public static function replace_pending_rollback_file( string $pending, string $temp_file, string $type ): array { + $partial = $pending . '.optml-partial'; + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- a failed move is reported through the return value. + if ( ! @copy( $temp_file, $partial ) || ! @rename( $partial, $pending ) ) { + wp_delete_file( $partial ); + + return [ 'error' => 'The restored file could not be written to ' . $pending ]; + } + wp_delete_file( $temp_file ); + + // Same permissions as wp_handle_sideload() gives a new file. + $stat = stat( dirname( $pending ) ); + if ( false !== $stat ) { + chmod( $pending, $stat['mode'] & 0000666 ); + } + + $uploads = wp_get_upload_dir(); + + return [ + 'file' => $pending, + 'url' => str_replace( wp_normalize_path( $uploads['basedir'] ), $uploads['baseurl'], wp_normalize_path( $pending ) ), + 'type' => $type, + ]; + } + /** * Handle the bulk actions. * @@ -2081,25 +2170,79 @@ public static function is_scheduled( $hook ) { * @return string|false The lock token on success, false if another worker already holds the lock. */ public static function acquire_transfer_lock( $action ) { + // Reading the transient also removes it once it has expired, so an expired lock can be taken again. $lock = get_transient( self::TRANSFER_LOCK_TRANSIENT ); if ( false !== $lock ) { return false; } $token = wp_generate_uuid4(); + $value = [ + 'token' => $token, + 'action' => $action, + ]; - set_transient( - self::TRANSFER_LOCK_TRANSIENT, - [ - 'token' => $token, - 'action' => $action, - ], - self::TRANSFER_LOCK_TTL - ); + if ( ! self::add_transfer_lock( $value ) ) { + return false; + } return $token; } + /** + * Store the transfer lock only if no other request stored it first. + * + * A read followed by a write lets two requests that start at the same moment both take the lock. + * The write here fails when the lock already exists: wp_cache_add() with a persistent object + * cache, and an INSERT IGNORE on the transient row otherwise. Both use the same storage as + * set_transient(), so the rest of the lock code reads it as before. + * + * @param array{token: string, action: string} $value The lock value. + * + * @return bool True if this request stored the lock. + */ + private static function add_transfer_lock( array $value ): bool { + if ( wp_using_ext_object_cache() ) { + return wp_cache_add( self::TRANSFER_LOCK_TRANSIENT, $value, 'transient', self::TRANSFER_LOCK_TTL ); + } + + global $wpdb; + + // The expiry is written first, so a request that dies right after the insert cannot leave a + // lock that never expires. A request that loses the race only rewrites the winner's expiry to + // the same moment. + update_option( '_transient_timeout_' . self::TRANSFER_LOCK_TRANSIENT, time() + self::TRANSFER_LOCK_TTL, false ); + + $option = '_transient_' . self::TRANSFER_LOCK_TRANSIENT; + $added = $wpdb->query( + $wpdb->prepare( + "INSERT IGNORE INTO `{$wpdb->options}` (`option_name`, `option_value`, `autoload`) VALUES (%s, %s, %s)", + $option, + maybe_serialize( $value ), + 'no' + ) + ); + + // The read above cached the transient as missing; forget that so the next read sees the new row. + $notoptions = wp_cache_get( 'notoptions', 'options' ); + if ( is_array( $notoptions ) && isset( $notoptions[ $option ] ) ) { + unset( $notoptions[ $option ] ); + wp_cache_set( 'notoptions', $notoptions, 'options' ); + } + wp_cache_delete( $option, 'options' ); + + if ( 1 !== $added ) { + return false; + } + + // Written again, in case the previous holder's release removed it between the two writes. The + // cached copy is dropped first: it still holds the first write, so update_option() would skip the write. + wp_cache_delete( '_transient_timeout_' . self::TRANSFER_LOCK_TRANSIENT, 'options' ); + update_option( '_transient_timeout_' . self::TRANSFER_LOCK_TRANSIENT, time() + self::TRANSFER_LOCK_TTL, false ); + + return true; + } + /** * Renew the transfer lock if we still own it, extending its expiration. * diff --git a/tests/test-media.php b/tests/test-media.php index 85605bea..78d6f37a 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -824,6 +824,117 @@ public function test_image_rollback() { $this->assertStringContainsString( '-150x150', $image_meta['sizes']['thumbnail']['file'] ); } + /** + * A rollback that died after moving the file into uploads leaves that file behind. The retry + * must write over it, so the restored image keeps its name instead of getting a "-1" suffix. + */ + public function test_rollback_retry_replaces_file_left_by_interrupted_attempt() { + $attachment_id = self::$sample_attachement; + $uploads = wp_upload_dir( get_the_date( 'Y/m', $attachment_id ) ); + $orphan = $uploads['path'] . '/' . self::$files[0] . '.jpg'; + wp_mkdir_p( $uploads['path'] ); + file_put_contents( $orphan, 'left by an interrupted rollback' ); + update_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, $orphan ); + + Optml_Media_Offload::instance()->rollback_images( 100 ); + + $attached = get_attached_file( $attachment_id ); + $this->assertSame( $orphan, $attached ); + $this->assertTrue( file_exists( $attached ) ); + $this->assertNotSame( 'left by an interrupted rollback', file_get_contents( $attached ) ); + $this->assertEmpty( get_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, true ) ); + } + + /** + * A request that dies while the sub-sizes are generated has already pointed the attachment at + * the restored file, and remembers that file so a retry can clean it up. + */ + public function test_rollback_dying_during_subsizes_keeps_attachment_on_restored_file() { + // Another file already uses the name, so the restored file gets a suffixed one. + $uploads = wp_upload_dir( get_the_date( 'Y/m', self::$sample_attachement ) ); + $taken = $uploads['path'] . '/' . self::$files[0] . '.jpg'; + wp_mkdir_p( $uploads['path'] ); + file_put_contents( $taken, 'another file' ); + + $died = function () { + throw new RuntimeException( 'request died' ); + }; + add_filter( 'intermediate_image_sizes_advanced', $died ); + + try { + Optml_Media_Offload::instance()->rollback_and_update_images( [ self::$sample_attachement ] ); + $this->fail( 'The rollback was expected to stop.' ); + } catch ( RuntimeException $e ) { + $this->assertSame( 'request died', $e->getMessage() ); + } finally { + remove_filter( 'intermediate_image_sizes_advanced', $died ); + } + + $restored = get_post_meta( self::$sample_attachement, Optml_Media_Offload::ROLLBACK_PENDING_FILE, true ); + $this->assertNotEmpty( $restored ); + $this->assertTrue( file_exists( $restored ) ); + $this->assertNotSame( $taken, $restored ); + $this->assertSame( $restored, get_attached_file( self::$sample_attachement, true ) ); + unlink( $taken ); + } + + /** + * Only a file inside the uploads directory is used, whatever the meta says. + */ + public function test_pending_rollback_file_outside_uploads_is_ignored() { + $attachment_id = self::factory()->post->create( [ 'post_type' => 'attachment' ] ); + $outside = get_temp_dir() . 'optml-outside-uploads.jpg'; + file_put_contents( $outside, 'not ours' ); + update_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, $outside ); + + $this->assertNull( Optml_Media_Offload::get_pending_rollback_file( $attachment_id ) ); + $this->assertSame( 'not ours', file_get_contents( $outside ) ); + unlink( $outside ); + } + + /** + * A retry whose copy fails partway (full disk, file size limit) keeps the file the attachment points to. + */ + public function test_failed_copy_keeps_the_pending_file() { + if ( ! in_array( 'optml-failing-read', stream_get_wrappers(), true ) ) { + stream_wrapper_register( 'optml-failing-read', Optml_Test_Failing_Read_Stream::class ); + } + $uploads = wp_get_upload_dir(); + $pending = $uploads['basedir'] . '/optml-pending-copy.jpg'; + file_put_contents( $pending, 'restored image' ); + + $result = Optml_Media_Offload::replace_pending_rollback_file( $pending, 'optml-failing-read://download', 'image/jpeg' ); + + $this->assertArrayHasKey( 'error', $result ); + $this->assertSame( 'restored image', file_get_contents( $pending ) ); + $this->assertFileDoesNotExist( $pending . '.optml-partial' ); + unlink( $pending ); + } + + /** + * A retry that ends in the branch for attachments whose local metadata was already saved + * clears the pending file too. + */ + public function test_retry_after_local_metadata_was_saved_clears_pending_file() { + $attachment_id = self::factory()->post->create( [ 'post_type' => 'attachment' ] ); + wp_update_attachment_metadata( $attachment_id, [ 'file' => '2026/10/restored.jpg' ] ); + update_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, '/some/where/restored.jpg' ); + + Optml_Media_Offload::instance()->rollback_and_update_images( [ $attachment_id ] ); + + $this->assertEmpty( get_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, true ) ); + } + + /** + * A rollback that completes leaves no pending file behind, so a later retry deletes nothing. + */ + public function test_completed_rollback_clears_pending_file() { + Optml_Media_Offload::instance()->rollback_images( 100 ); + + $this->assertTrue( file_exists( get_attached_file( self::$sample_attachement ) ) ); + $this->assertEmpty( get_post_meta( self::$sample_attachement, Optml_Media_Offload::ROLLBACK_PENDING_FILE, true ) ); + } + public function test_rollback_retry_clears_errors() { // Use a dedicated attachment with fully controlled meta so the assertions do @@ -1122,6 +1233,63 @@ public function test_acquire_transfer_lock_blocks_concurrent_start() { delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); } + /** + * Two requests that both read the lock as free must not both take it: the one whose write comes + * second gets false, and the lock keeps the first one's token and an expiry. + */ + public function test_acquire_transfer_lock_fails_when_another_request_took_it_after_the_read() { + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + global $wpdb; + + // This request reads the lock as missing, then another request stores it before our write. + $this->assertFalse( get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ) ); + $wpdb->insert( + $wpdb->options, + [ + 'option_name' => '_transient_' . Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT, + 'option_value' => maybe_serialize( [ 'token' => 'other-request', 'action' => 'offload_images' ] ), + 'autoload' => 'no', + ] + ); + + $this->assertFalse( Optml_Media_Offload::acquire_transfer_lock( 'offload_images' ) ); + + $lock = get_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + $this->assertSame( 'other-request', $lock['token'] ); + $this->assertGreaterThan( time(), (int) get_option( '_transient_timeout_' . Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ) ); + + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + } + + /** + * Another request's release can delete the expiry row between our first write and the insert. + * The lock we then store must still get an expiry, or a dead worker would block transfers forever. + */ + public function test_acquire_transfer_lock_restores_expiry_removed_before_the_insert() { + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + global $wpdb; + $timeout = '_transient_timeout_' . Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT; + + $release = function ( $query ) use ( &$release, $wpdb, $timeout ) { + if ( false !== strpos( $query, 'INSERT IGNORE' ) ) { + remove_filter( 'query', $release ); + $wpdb->delete( $wpdb->options, [ 'option_name' => $timeout ] ); + } + + return $query; + }; + add_filter( 'query', $release ); + $token = Optml_Media_Offload::acquire_transfer_lock( 'offload_images' ); + remove_filter( 'query', $release ); + + $this->assertNotFalse( $token ); + $stored = $wpdb->get_var( $wpdb->prepare( "SELECT option_value FROM {$wpdb->options} WHERE option_name = %s", $timeout ) ); + $this->assertNotNull( $stored ); + $this->assertGreaterThan( time(), (int) $stored ); + + delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); + } + /** * A lock left behind by a worker that died mid-batch (fatal error, killed process) must * eventually become reclaimable, otherwise a crashed transfer would be stuck forever. @@ -1358,3 +1526,79 @@ public function transfer_action_provider(): array { ]; } } + +/** + * A stream that returns some bytes, then fails, like a copy that runs out of disk space partway. + */ +class Optml_Test_Failing_Read_Stream { + /** + * Stream context, set by PHP. + * + * @var resource|null + */ + public $context; + + /** + * Reads done so far. + * + * @var int + */ + private int $reads = 0; + + /** + * Open the stream. + * + * @param string $path The URL. + * @param string $mode The mode. + * @param int $options The options. + * @param string|null $opened_path The opened path. + * + * @return bool + */ + public function stream_open( string $path, string $mode, int $options, ?string &$opened_path ): bool { + return true; + } + + /** + * Return a few bytes the first time, then fail. + * + * @param int $count The bytes asked for. + * + * @return string|false + */ + public function stream_read( int $count ) { + ++$this->reads; + + return 1 === $this->reads ? 'partial' : false; + } + + /** + * The stream never reaches its end. + * + * @return bool + */ + public function stream_eof(): bool { + return false; + } + + /** + * Stream stats. + * + * @return array + */ + public function stream_stat(): array { + return []; + } + + /** + * URL stats. + * + * @param string $path The URL. + * @param int $flags The flags. + * + * @return array + */ + public function url_stat( string $path, int $flags ): array { + return [ 'size' => 100 ]; + } +}