diff --git a/inc/media_offload.php b/inc/media_offload.php index c08ab51d..d2cb694a 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. */ @@ -874,6 +879,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; } @@ -966,8 +973,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' ); @@ -978,6 +998,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'; } @@ -1003,14 +1030,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 ) ) { @@ -1052,6 +1072,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. * @@ -2069,25 +2158,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 f8a9eaa1..ea69f1ee 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -691,6 +691,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 @@ -989,6 +1100,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. @@ -1225,3 +1393,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 ]; + } +}