diff --git a/classes/ActionScheduler_QueueCleaner.php b/classes/ActionScheduler_QueueCleaner.php index d031fd68..3d3a587e 100644 --- a/classes/ActionScheduler_QueueCleaner.php +++ b/classes/ActionScheduler_QueueCleaner.php @@ -188,7 +188,7 @@ public function delete_old_actions() { * * @return array Actions deleted. */ - public function clean_actions( array $statuses_to_purge, DateTime $cutoff_date, $batch_size = null, $context = 'old' ) { + public function clean_actions( array $statuses_to_purge, DateTime $cutoff_date, $batch_size = null, $context = 'old', $date_type = 'modified' ) { $batch_size = ! is_null( $batch_size ) ? $batch_size : $this->batch_size; $cutoff = ! is_null( $cutoff_date ) ? $cutoff_date : as_get_datetime_object( $this->month_in_seconds . ' seconds ago' ); $lifespan = time() - $cutoff->getTimestamp(); @@ -198,6 +198,9 @@ public function clean_actions( array $statuses_to_purge, DateTime $cutoff_date, // For inline cleanup during a queue run, the batch size should remain unchanged to avoid increasing the process footprint. $is_scheduled_cleanup = doing_action( self::RUN_SCHEDULED_CLEANER_HOOK ) || doing_action( self::CONTINUE_SCHEDULED_CLEANER_HOOK ); + if ( $is_scheduled_cleanup ) { + $date_type = 'modified'; + } // 250 balances replication safety, backlog clearance speed, and claim slot duration on high-volume stores. $iteration_batch_size = $is_scheduled_cleanup ? max( 250, $batch_size ) : $batch_size; $iteration_unused_budget = 0; @@ -231,15 +234,19 @@ static function( $a, $b ) { $deleted_actions = array(); foreach ( $statuses_to_purge as $status ) { $iteration_execution_budget = $iteration_batch_size + $iteration_unused_budget; - $actions_to_delete = $this->store->query_actions( - array( - 'status' => $status, - 'modified' => $cutoff, - 'modified_compare' => '<=', - 'per_page' => $iteration_execution_budget, - 'orderby' => 'none', - ) + $query = array( + 'status' => $status, + 'per_page' => $iteration_execution_budget, + 'orderby' => 'none', ); + if ( 'date' === $date_type ) { + $query['date'] = $cutoff; + $query['date_compare'] = '<='; + } else { + $query['modified'] = $cutoff; + $query['modified_compare'] = '<='; + } + $actions_to_delete = $this->store->query_actions( $query ); $deleted_actions[] = $this->delete_actions( $actions_to_delete, $lifespan, $context ); $fetched_actions_count = count( $actions_to_delete ); diff --git a/classes/WP_CLI/ActionScheduler_WPCLI_Clean_Command.php b/classes/WP_CLI/ActionScheduler_WPCLI_Clean_Command.php index b0ac4e30..24772461 100644 --- a/classes/WP_CLI/ActionScheduler_WPCLI_Clean_Command.php +++ b/classes/WP_CLI/ActionScheduler_WPCLI_Clean_Command.php @@ -36,7 +36,7 @@ public function clean( $args, $assoc_args ) { $batches = absint( \WP_CLI\Utils\get_flag_value( $assoc_args, 'batches', 0 ) ); $status = explode( ',', WP_CLI\Utils\get_flag_value( $assoc_args, 'status', '' ) ); $status = array_filter( array_map( 'trim', $status ) ); - $before = \WP_CLI\Utils\get_flag_value( $assoc_args, 'before', '' ); + $before = \WP_CLI\Utils\get_flag_value( $assoc_args, 'before', '31 days ago' ); $sleep = \WP_CLI\Utils\get_flag_value( $assoc_args, 'pause', 0 ); $batches_completed = 0; @@ -58,7 +58,7 @@ public function clean( $args, $assoc_args ) { sleep( $sleep ); } - $deleted = count( $cleaner->clean_actions( $status, $lifespan, null, 'CLI' ) ); + $deleted = count( $cleaner->clean_actions( $status, $lifespan, null, 'CLI', 'date' ) ); if ( $deleted <= 0 ) { break; } diff --git a/classes/data-stores/ActionScheduler_DBLogger.php b/classes/data-stores/ActionScheduler_DBLogger.php index 805869d2..0faa6e1e 100644 --- a/classes/data-stores/ActionScheduler_DBLogger.php +++ b/classes/data-stores/ActionScheduler_DBLogger.php @@ -121,6 +121,11 @@ public function init() { * @param int $action_id Action ID. */ public function clear_deleted_action_logs( $action_id ) { + if ( defined( 'WP_CLI' ) && WP_CLI ) { + global $wpdb; + $wpdb->delete( $wpdb->actionscheduler_logs, array( 'action_id' => $action_id ), array( '%d' ) ); + return; + } $this->clear_deleted_action_logs_single_batch( $action_id, -1, 1, 4000 ); } diff --git a/classes/data-stores/ActionScheduler_DBStore.php b/classes/data-stores/ActionScheduler_DBStore.php index e634f9e4..eac0d334 100644 --- a/classes/data-stores/ActionScheduler_DBStore.php +++ b/classes/data-stores/ActionScheduler_DBStore.php @@ -872,12 +872,21 @@ public function delete_action( $action_id ) { */ global $wpdb; + $claim_id = (int) $wpdb->get_var( $wpdb->prepare( "SELECT claim_id FROM {$wpdb->actionscheduler_actions} WHERE action_id = %d", $action_id ) ); + $deleted = $wpdb->delete( $wpdb->actionscheduler_actions, array( 'action_id' => $action_id ), array( '%d' ) ); if ( empty( $deleted ) ) { /* translators: %s is the action ID */ throw new \InvalidArgumentException( sprintf( __( 'Unidentified action %s: we were unable to delete this action. It may may have been deleted by another process.', 'action-scheduler' ), $action_id ) ); } do_action( 'action_scheduler_deleted_action', $action_id ); + + if ( $claim_id > 0 ) { + $referenced = (int) $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(*) FROM {$wpdb->actionscheduler_actions} WHERE claim_id = %d", $claim_id ) ); + if ( 0 === $referenced ) { + $wpdb->delete( $wpdb->actionscheduler_claims, array( 'claim_id' => $claim_id ), array( '%d' ) ); + } + } } /** diff --git a/classes/data-stores/ActionScheduler_wpCommentLogger.php b/classes/data-stores/ActionScheduler_wpCommentLogger.php index a9d84d31..72fcb58b 100644 --- a/classes/data-stores/ActionScheduler_wpCommentLogger.php +++ b/classes/data-stores/ActionScheduler_wpCommentLogger.php @@ -7,6 +7,24 @@ class ActionScheduler_wpCommentLogger extends ActionScheduler_Logger { const AGENT = 'ActionScheduler'; const TYPE = 'action_log'; + public function clear_deleted_action_logs( $action_id ) { + global $wpdb; + $wpdb->query( + $wpdb->prepare( + "DELETE cm FROM {$wpdb->commentmeta} cm INNER JOIN {$wpdb->comments} c ON c.comment_ID = cm.comment_id WHERE c.comment_post_ID = %d AND c.comment_type = %s", + $action_id, + self::TYPE + ) + ); + $wpdb->query( + $wpdb->prepare( + "DELETE FROM {$wpdb->comments} WHERE comment_post_ID = %d AND comment_type = %s", + $action_id, + self::TYPE + ) + ); + } + /** * Create log entry. * @@ -255,6 +273,8 @@ public function init() { add_action( 'action_scheduler_after_process_queue', array( $this, 'enable_comment_counting' ), 10, 0 ); parent::init(); + add_action( 'action_scheduler_deleted_action', array( $this, 'clear_deleted_action_logs' ), 10, 1 ); + add_action( 'action_scheduler_canceled_corrupted_action', array( $this, 'clear_deleted_action_logs' ), 10, 1 ); add_action( 'pre_get_comments', array( $this, 'filter_comment_queries' ), 10, 1 ); add_action( 'wp_count_comments', array( $this, 'filter_comment_count' ), 20, 2 ); // run after WC_Comments::wp_count_comments() to make sure we exclude order notes and action logs. diff --git a/tests/phpunit/runner/ActionScheduler_QueueCleaner_Test.php b/tests/phpunit/runner/ActionScheduler_QueueCleaner_Test.php index cf62aa99..cb483fbf 100644 --- a/tests/phpunit/runner/ActionScheduler_QueueCleaner_Test.php +++ b/tests/phpunit/runner/ActionScheduler_QueueCleaner_Test.php @@ -586,4 +586,172 @@ static function( $query ) { remove_action( 'action_scheduler_run_actions_cleanup_hook', array( $cleaner, 'delete_old_actions' ) ); remove_action( 'action_scheduler_continue_actions_cleanup_hook', array( $cleaner, 'delete_old_actions' ) ); } + + /** + * Helper to create a scenario with two completed actions scheduled 32 days ago, + * where Action B has a recent last_attempt_gmt to simulate a retried action. + * + * @return array{store: ActionScheduler_Store, action_a_id: int, action_b_id: int} + */ + private function create_cli_scenario() { + $store = ActionScheduler::store(); + ActionScheduler_Callbacks::add_callbacks(); + $scheduled_date_old = as_get_datetime_object( '-32 days' ); + + $action_a = new ActionScheduler_Action( + ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, + array( md5( wp_rand() ) ), + new ActionScheduler_SimpleSchedule( $scheduled_date_old ) + ); + $action_a_id = $store->save_action( $action_a ); + $store->mark_complete( $action_a_id ); + $this->force_last_attempt_date( $action_a_id, $scheduled_date_old ); + + $action_b = new ActionScheduler_Action( + ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, + array( md5( wp_rand() ) ), + new ActionScheduler_SimpleSchedule( $scheduled_date_old ) + ); + $action_b_id = $store->save_action( $action_b ); + $store->mark_complete( $action_b_id ); + $this->force_last_attempt_date( $action_b_id, as_get_datetime_object( '-1 hour' ) ); + + return array( + 'store' => $store, + 'action_a_id' => $action_a_id, + 'action_b_id' => $action_b_id, + ); + } + + /** + * Regression test for #1107: clean_actions() with $date_type = 'date' deletes actions based on + * scheduled_date_gmt, so even actions with a recent last_attempt_gmt are correctly cleaned up. + */ + public function test_clean_actions_uses_scheduled_date_when_date_type_is_date() { + $scenario = $this->create_cli_scenario(); + $store = $scenario['store']; + $action_a_id = $scenario['action_a_id']; + $action_b_id = $scenario['action_b_id']; + + $cleaner = new ActionScheduler_QueueCleaner( $store ); + $cutoff_date = as_get_datetime_object( '-31 days' ); + $deleted = $cleaner->clean_actions( + array( ActionScheduler_Store::STATUS_COMPLETE ), + $cutoff_date, + null, + 'CLI', + 'date' + ); + + $this->assertContains( $action_a_id, $deleted, 'Action A (old modified) should be deleted.' ); + $this->assertContains( $action_b_id, $deleted, 'Action B (recent modified) SHOULD be deleted when using scheduled date.' ); + $this->assertCount( 2, $deleted, 'Both actions should be deleted when using date-based filtering.' ); + + $this->assertInstanceOf( ActionScheduler_NullAction::class, $store->fetch_action( $action_a_id ) ); + $this->assertInstanceOf( ActionScheduler_NullAction::class, $store->fetch_action( $action_b_id ) ); + } + + /** + * Regression test for #1107: clean_actions() with $date_type = 'modified' skips + * actions with a recent last_attempt_gmt. This demonstrates the original bug. + */ + public function test_clean_actions_with_modified_date_misses_recently_touched_actions() { + $scenario = $this->create_cli_scenario(); + $store = $scenario['store']; + $action_a_id = $scenario['action_a_id']; + $action_b_id = $scenario['action_b_id']; + + $cleaner = new ActionScheduler_QueueCleaner( $store ); + $cutoff_date = as_get_datetime_object( '-31 days' ); + $deleted = $cleaner->clean_actions( + array( ActionScheduler_Store::STATUS_COMPLETE ), + $cutoff_date, + null, + 'CLI', + 'modified' + ); + + $this->assertContains( $action_a_id, $deleted, 'Action A (old both dates) should be deleted.' ); + $this->assertNotContains( $action_b_id, $deleted, 'Action B (recent last_attempt) escapes deletion — this is the bug #1107.' ); + $this->assertCount( 1, $deleted, 'Only Action A is deleted. Action B escapes modified-based filtering.' ); + $this->assertNotInstanceOf( ActionScheduler_NullAction::class, $store->fetch_action( $action_b_id ) ); + } + + /** + * Scheduled (cron) cleanup always forces 'modified' date type, even if 'date' was requested externally. + */ + public function test_scheduled_cleanup_uses_modified_date() { + $store = ActionScheduler::store(); + ActionScheduler_Callbacks::add_callbacks(); + + $action = new ActionScheduler_Action( + ActionScheduler_Callbacks::HOOK_WITH_CALLBACK, + array( md5( wp_rand() ) ), + new ActionScheduler_SimpleSchedule( as_get_datetime_object( '-32 days' ) ) + ); + $action_id = $store->save_action( $action ); + $store->mark_complete( $action_id ); + $this->force_last_attempt_date( $action_id, as_get_datetime_object( '-5 minutes' ) ); + + $cleaner = new ActionScheduler_QueueCleaner( $store ); + $cleaner->register_cleaner_hooks(); + do_action( 'action_scheduler_run_actions_cleanup_hook' ); + + $fetched_action = $store->fetch_action( $action_id ); + $this->assertNotInstanceOf( ActionScheduler_NullAction::class, $fetched_action, 'Scheduled cleanup should NOT delete actions with recent last_attempt.' ); + $this->assertInstanceOf( ActionScheduler_FinishedAction::class, $fetched_action, 'The action should still exist as a FinishedAction.' ); + } + + /** + * Verify orphan claims are cleaned up when deleting an action. + */ + public function test_orphan_claim_cleaned_on_action_delete() { + $store = ActionScheduler::store(); + $schedule = new ActionScheduler_SimpleSchedule( as_get_datetime_object( '-32 days' ) ); + + $action_a_id = $store->save_action( new ActionScheduler_Action( 'hook_a', array(), $schedule ) ); + $action_b_id = $store->save_action( new ActionScheduler_Action( 'hook_b', array(), $schedule ) ); + + $claim = $store->stake_claim( 10 ); + $claim_id = $claim->get_id(); + + $store->delete_action( $action_a_id ); + + global $wpdb; + $claims_after_a = (int) $wpdb->get_var( + $wpdb->prepare( "SELECT COUNT(*) FROM {$wpdb->actionscheduler_claims} WHERE claim_id = %d", $claim_id ) + ); + $this->assertEquals( 1, $claims_after_a, 'Claim should still exist since action B references it.' ); + + $store->delete_action( $action_b_id ); + + $claims_after_b = (int) $wpdb->get_var( + $wpdb->prepare( "SELECT COUNT(*) FROM {$wpdb->actionscheduler_claims} WHERE claim_id = %d", $claim_id ) + ); + $this->assertEquals( 0, $claims_after_b, 'Orphan claim should be deleted when its last referencing action is deleted.' ); + } + + /** + * Force the last_attempt_gmt and last_attempt_local columns to a specific datetime. + * + * @param int $action_id Action ID. + * @param DateTime $date The date to set. + */ + private function force_last_attempt_date( $action_id, DateTime $date ) { + global $wpdb; + $date->setTimezone( new DateTimeZone( 'UTC' ) ); + $gmt = $date->format( 'Y-m-d H:i:s' ); + $local = get_date_from_gmt( $gmt ); + + $wpdb->update( + $wpdb->actionscheduler_actions, + array( + 'last_attempt_gmt' => $gmt, + 'last_attempt_local' => $local, + ), + array( 'action_id' => $action_id ), + array( '%s', '%s' ), + array( '%d' ) + ); + } }