From 9d2d682674117456394f46e70bb11caa50dc6622 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 19:40:32 +0000 Subject: [PATCH 1/4] 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 --- inc/media_offload.php | 121 ++++++++++++++++++++++++++++++++++++------ tests/test-media.php | 104 ++++++++++++++++++++++++++++++++++++ 2 files changed, 209 insertions(+), 16 deletions(-) diff --git a/inc/media_offload.php b/inc/media_offload.php index c08ab51d..a2899247 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. */ @@ -966,6 +971,17 @@ public function rollback_and_update_images( $image_ids ) { continue; } + 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 would make WordPress pick a suffixed name. + self::delete_pending_rollback_file( $id ); + // Move the temporary file into the uploads directory. $results = wp_handle_sideload( $file, $overrides, get_the_date( 'Y/m', $id ) ); if ( ! empty( $results['error'] ) ) { @@ -978,6 +994,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 +1026,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 +1068,34 @@ public function rollback_and_update_images( $image_ids ) { return $success_back; } + /** + * Delete the file an interrupted rollback of this attachment moved into uploads. + * + * The file is only removed when it is inside the uploads directory, so a corrupted meta value + * can never delete anything else. + * + * @param int $id The attachment ID. + * + * @return void + */ + public static function delete_pending_rollback_file( $id ) { + $pending = get_post_meta( $id, self::ROLLBACK_PENDING_FILE, true ); + if ( empty( $pending ) || ! is_string( $pending ) ) { + return; + } + delete_post_meta( $id, self::ROLLBACK_PENDING_FILE ); + + $uploads = wp_get_upload_dir(); + $base_dir = realpath( $uploads['basedir'] ); + $file = realpath( $pending ); + if ( false === $base_dir || false === $file || ! is_file( $file ) || strpos( $file, trailingslashit( $base_dir ) ) !== 0 ) { + return; + } + + wp_delete_file( $file ); + self::$instance->logger->add_log( Optml_Logger::LOG_TYPE_ROLLBACK, 'Image ID: ' . $id . ' had a file left by an interrupted rollback, it was removed before retrying.' ); + } + /** * Handle the bulk actions. * @@ -2069,25 +2113,70 @@ 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 ) { + 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' ); + + return 1 === $added; + } + /** * 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..9dcb2957 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -691,6 +691,86 @@ 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 remove it, so the restored image keeps its name instead of getting a "-1" suffix. + */ + public function test_rollback_retry_removes_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 removed, whatever the meta says. + */ + public function test_pending_rollback_file_outside_uploads_is_kept() { + $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 ); + + Optml_Media_Offload::delete_pending_rollback_file( $attachment_id ); + + $this->assertTrue( file_exists( $outside ) ); + $this->assertEmpty( get_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, true ) ); + unlink( $outside ); + } + + /** + * 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 @@ -993,6 +1073,30 @@ public function test_acquire_transfer_lock_blocks_concurrent_start() { * 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. */ + 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 ); + } + public function test_acquire_transfer_lock_reclaims_expired_lock() { delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); From b37c91d4091e6f9d9a5179ac83280e477d9fdb5a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 20:34:41 +0000 Subject: [PATCH 2/4] 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 --- inc/media_offload.php | 84 +++++++++++++++++++++++++++++++++---------- tests/test-media.php | 34 +++++++++++++----- 2 files changed, 91 insertions(+), 27 deletions(-) diff --git a/inc/media_offload.php b/inc/media_offload.php index a2899247..ac44e73e 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -879,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; } @@ -979,11 +981,13 @@ public function rollback_and_update_images( $image_ids ) { continue; } - // A file left behind by an interrupted attempt would make WordPress pick a suffixed name. - self::delete_pending_rollback_file( $id ); - - // Move the temporary file into the uploads directory. - $results = wp_handle_sideload( $file, $overrides, get_the_date( 'Y/m', $id ) ); + // 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' ); @@ -1069,31 +1073,68 @@ public function rollback_and_update_images( $image_ids ) { } /** - * Delete the file an interrupted rollback of this attachment moved into uploads. + * Get the file an interrupted rollback of this attachment moved into uploads. * - * The file is only removed when it is inside the uploads directory, so a corrupted meta value - * can never delete anything else. + * 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 void + * @return string|null The absolute path, or null when there is no usable file. */ - public static function delete_pending_rollback_file( $id ) { + 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; + return null; } - delete_post_meta( $id, self::ROLLBACK_PENDING_FILE ); $uploads = wp_get_upload_dir(); $base_dir = realpath( $uploads['basedir'] ); $file = realpath( $pending ); - if ( false === $base_dir || false === $file || ! is_file( $file ) || strpos( $file, trailingslashit( $base_dir ) ) !== 0 ) { - return; + 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 old file is only overwritten once the new one is in place, so a failed move keeps it. + * + * @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 { + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- a failed move is reported through the return value. + if ( ! @copy( $temp_file, $pending ) ) { + 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 ); } - wp_delete_file( $file ); - self::$instance->logger->add_log( Optml_Logger::LOG_TYPE_ROLLBACK, 'Image ID: ' . $id . ' had a file left by an interrupted rollback, it was removed before retrying.' ); + $uploads = wp_get_upload_dir(); + + return [ + 'file' => $pending, + 'url' => str_replace( wp_normalize_path( $uploads['basedir'] ), $uploads['baseurl'], wp_normalize_path( $pending ) ), + 'type' => $type, + ]; } /** @@ -2144,7 +2185,7 @@ public static function acquire_transfer_lock( $action ) { * * @return bool True if this request stored the lock. */ - private static function add_transfer_lock( array $value ) { + 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 ); } @@ -2174,7 +2215,14 @@ private static function add_transfer_lock( array $value ) { } wp_cache_delete( $option, 'options' ); - return 1 === $added; + if ( 1 !== $added ) { + return false; + } + + // Written again, in case the previous holder's release removed it between the two writes. + update_option( '_transient_timeout_' . self::TRANSFER_LOCK_TRANSIENT, time() + self::TRANSFER_LOCK_TTL, false ); + + return true; } /** diff --git a/tests/test-media.php b/tests/test-media.php index 9dcb2957..a896fecb 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -693,9 +693,9 @@ public function test_image_rollback() { /** * A rollback that died after moving the file into uploads leaves that file behind. The retry - * must remove it, so the restored image keeps its name instead of getting a "-1" suffix. + * must write over it, so the restored image keeps its name instead of getting a "-1" suffix. */ - public function test_rollback_retry_removes_file_left_by_interrupted_attempt() { + 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'; @@ -746,19 +746,31 @@ public function test_rollback_dying_during_subsizes_keeps_attachment_on_restored } /** - * Only a file inside the uploads directory is removed, whatever the meta says. + * Only a file inside the uploads directory is used, whatever the meta says. */ - public function test_pending_rollback_file_outside_uploads_is_kept() { + 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 ); - Optml_Media_Offload::delete_pending_rollback_file( $attachment_id ); + $this->assertNull( Optml_Media_Offload::get_pending_rollback_file( $attachment_id ) ); + $this->assertSame( 'not ours', file_get_contents( $outside ) ); + unlink( $outside ); + } + + /** + * 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->assertTrue( file_exists( $outside ) ); $this->assertEmpty( get_post_meta( $attachment_id, Optml_Media_Offload::ROLLBACK_PENDING_FILE, true ) ); - unlink( $outside ); } /** @@ -1070,8 +1082,8 @@ public function test_acquire_transfer_lock_blocks_concurrent_start() { } /** - * 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. + * 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 ); @@ -1097,6 +1109,10 @@ public function test_acquire_transfer_lock_fails_when_another_request_took_it_af 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. + */ public function test_acquire_transfer_lock_reclaims_expired_lock() { delete_transient( Optml_Media_Offload::TRANSFER_LOCK_TRANSIENT ); From 7fc4911ed9f494faff55cb6bdf051a0cc5d10418 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 21:31:18 +0000 Subject: [PATCH 3/4] 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 --- inc/media_offload.php | 12 +++- tests/test-media.php | 124 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/inc/media_offload.php b/inc/media_offload.php index ac44e73e..d2cb694a 100644 --- a/inc/media_offload.php +++ b/inc/media_offload.php @@ -1107,7 +1107,8 @@ public static function get_pending_rollback_file( int $id ): ?string { /** * Put the downloaded image in place of the file an interrupted rollback left behind. * - * The old file is only overwritten once the new one is in place, so a failed move keeps it. + * 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. @@ -1116,8 +1117,11 @@ public static function get_pending_rollback_file( int $id ): ?string { * @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, $pending ) ) { + 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 ); @@ -2219,7 +2223,9 @@ private static function add_transfer_lock( array $value ): bool { return false; } - // Written again, in case the previous holder's release removed it between the two writes. + // 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; diff --git a/tests/test-media.php b/tests/test-media.php index a896fecb..a0ff8fcf 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -759,6 +759,25 @@ public function test_pending_rollback_file_outside_uploads_is_ignored() { 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. @@ -1109,6 +1128,35 @@ public function test_acquire_transfer_lock_fails_when_another_request_took_it_af 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. @@ -1345,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 $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( $path, $mode, $options, &$opened_path ) { + 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( $count ) { + ++$this->reads; + + return 1 === $this->reads ? 'partial' : false; + } + + /** + * The stream never reaches its end. + * + * @return bool + */ + public function stream_eof() { + return false; + } + + /** + * Stream stats. + * + * @return array + */ + public function stream_stat() { + return []; + } + + /** + * URL stats. + * + * @param string $path The URL. + * @param int $flags The flags. + * + * @return array + */ + public function url_stat( $path, $flags ) { + return [ 'size' => 100 ]; + } +} From 0de6178295fa540aa8e08bcdcafc05197f22c789 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 21:59:31 +0000 Subject: [PATCH 4/4] 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 --- tests/test-media.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test-media.php b/tests/test-media.php index a0ff8fcf..ea69f1ee 100644 --- a/tests/test-media.php +++ b/tests/test-media.php @@ -1410,7 +1410,7 @@ class Optml_Test_Failing_Read_Stream { * * @var int */ - private $reads = 0; + private int $reads = 0; /** * Open the stream. @@ -1422,7 +1422,7 @@ class Optml_Test_Failing_Read_Stream { * * @return bool */ - public function stream_open( $path, $mode, $options, &$opened_path ) { + public function stream_open( string $path, string $mode, int $options, ?string &$opened_path ): bool { return true; } @@ -1433,7 +1433,7 @@ public function stream_open( $path, $mode, $options, &$opened_path ) { * * @return string|false */ - public function stream_read( $count ) { + public function stream_read( int $count ) { ++$this->reads; return 1 === $this->reads ? 'partial' : false; @@ -1444,7 +1444,7 @@ public function stream_read( $count ) { * * @return bool */ - public function stream_eof() { + public function stream_eof(): bool { return false; } @@ -1453,7 +1453,7 @@ public function stream_eof() { * * @return array */ - public function stream_stat() { + public function stream_stat(): array { return []; } @@ -1465,7 +1465,7 @@ public function stream_stat() { * * @return array */ - public function url_stat( $path, $flags ) { + public function url_stat( string $path, int $flags ): array { return [ 'size' => 100 ]; } }