From 2c67e40ac167964af081e07a9966d48806827a05 Mon Sep 17 00:00:00 2001 From: Michael Pretty Date: Mon, 17 Mar 2025 14:23:06 -0400 Subject: [PATCH 1/3] Only release claims on pending actions. --- .../data-stores/ActionScheduler_DBStore.php | 11 +++++-- .../ActionScheduler_HybridStore.php | 2 +- .../ActionScheduler_wpPostStore.php | 29 +++++++++++++++---- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/classes/data-stores/ActionScheduler_DBStore.php b/classes/data-stores/ActionScheduler_DBStore.php index 25a883ab..5a5e01e9 100644 --- a/classes/data-stores/ActionScheduler_DBStore.php +++ b/classes/data-stores/ActionScheduler_DBStore.php @@ -1094,7 +1094,7 @@ public function find_actions_by_claim_id( $claim_id ) { } /** - * Release actions from a claim and delete the claim. + * Release pending actions from a claim and delete the claim. * * @param ActionScheduler_ActionClaim $claim Claim object. * @throws \RuntimeException When unable to release actions from claim. @@ -1107,14 +1107,21 @@ public function release_claim( ActionScheduler_ActionClaim $claim ) { */ global $wpdb; + if ( 0 === intval( $claim->get_id() ) ) { + // Verify that the claim_id is valid before attempting to release it. + return; + } + /** * Deadlock warning: This function modifies actions to release them from claims that have been processed. Earlier, we used to it in a atomic query, i.e. we would update all actions belonging to a particular claim_id with claim_id = 0. * While this was functionally correct, it would cause deadlock, since this update query will hold a lock on the claim_id_.. index on the action table. * This allowed the possibility of a race condition, where the claimer query is also running at the same time, then the claimer query will also try to acquire a lock on the claim_id_.. index, and in this case if claim release query has already progressed to the point of acquiring the lock, but have not updated yet, it would cause a deadlock. * * We resolve this by getting all the actions_id that we want to release claim from in a separate query, and then releasing the claim on each of them. This way, our lock is acquired on the action_id index instead of the claim_id index. Note that the lock on claim_id will still be acquired, but it will only when we actually make the update, rather than when we select the actions. + * + * We only release pending actions in order for them to be claimed by another process. */ - $action_ids = $wpdb->get_col( $wpdb->prepare( "SELECT action_id FROM {$wpdb->actionscheduler_actions} WHERE claim_id = %d", $claim->get_id() ) ); + $action_ids = $wpdb->get_col( $wpdb->prepare( "SELECT action_id FROM {$wpdb->actionscheduler_actions} WHERE claim_id = %d AND status = %s", $claim->get_id(), self::STATUS_PENDING ) ); $row_updates = 0; if ( count( $action_ids ) > 0 ) { diff --git a/classes/data-stores/ActionScheduler_HybridStore.php b/classes/data-stores/ActionScheduler_HybridStore.php index c0845da4..40520cc8 100644 --- a/classes/data-stores/ActionScheduler_HybridStore.php +++ b/classes/data-stores/ActionScheduler_HybridStore.php @@ -432,7 +432,7 @@ public function get_claim_id( $action_id ) { } /** - * Release a claim in the table data store. + * Release a claim in the table data store on any pending actions. * * @param ActionScheduler_ActionClaim $claim Claim object. */ diff --git a/classes/data-stores/ActionScheduler_wpPostStore.php b/classes/data-stores/ActionScheduler_wpPostStore.php index a503c183..8f6375a6 100644 --- a/classes/data-stores/ActionScheduler_wpPostStore.php +++ b/classes/data-stores/ActionScheduler_wpPostStore.php @@ -791,18 +791,13 @@ public function find_actions_by_claim_id( $claim_id ) { } /** - * Release claim. + * Release pending actions from a claim. * * @param ActionScheduler_ActionClaim $claim Claim object to release. * @return void * @throws RuntimeException When the claim is not unlocked. */ public function release_claim( ActionScheduler_ActionClaim $claim ) { - $action_ids = $this->find_actions_by_claim_id( $claim->get_id() ); - if ( empty( $action_ids ) ) { - return; // nothing to do. - } - $action_id_string = implode( ',', array_map( 'intval', $action_ids ) ); /** * Global wpdb object. * @@ -810,6 +805,28 @@ public function release_claim( ActionScheduler_ActionClaim $claim ) { */ global $wpdb; + $claim_id = $claim->get_id(); + if ( trim( $claim_id ) === '' ) { + // Verify that the claim_id is valid before attempting to release it. + return; + } + + // Only attempt to release pending actions to be claimed again. Running and complete actions are no longer relevant outside of admin/analytics. + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching + $action_ids = $wpdb->get_col( + $wpdb->prepare( + "SELECT ID, post_date_gmt FROM {$wpdb->posts} WHERE post_type = %s AND post_password = %s AND post_status = %s", + self::POST_TYPE, + $claim_id, + self::STATUS_PENDING + ) + ); + + if ( empty( $action_ids ) ) { + return; // nothing to do. + } + $action_id_string = implode( ',', array_map( 'intval', $action_ids ) ); + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching $result = $wpdb->query( $wpdb->prepare( From 7623cf53078bad4bb53fbcb82a63c2c1de7ed2cf Mon Sep 17 00:00:00 2001 From: Michael Pretty Date: Wed, 16 Apr 2025 13:07:48 -0400 Subject: [PATCH 2/3] Remove `claimed` parameter from query in ActionScheduler_wcSystemStatus::get_action_status_date() as it doesn't necessarily make sense to filter out claimed actions. --- classes/ActionScheduler_wcSystemStatus.php | 1 - 1 file changed, 1 deletion(-) diff --git a/classes/ActionScheduler_wcSystemStatus.php b/classes/ActionScheduler_wcSystemStatus.php index bca63e71..cd6ca940 100644 --- a/classes/ActionScheduler_wcSystemStatus.php +++ b/classes/ActionScheduler_wcSystemStatus.php @@ -76,7 +76,6 @@ protected function get_action_status_date( $status, $date_type = 'oldest' ) { $action = $this->store->query_actions( array( - 'claimed' => false, 'status' => $status, 'per_page' => 1, 'order' => $order, From 2a1a9a0f99761dc57831836e15b7b636041c5af7 Mon Sep 17 00:00:00 2001 From: Michael Pretty Date: Wed, 16 Apr 2025 13:12:17 -0400 Subject: [PATCH 3/3] Remove `claimed` parameter from query in System_Command::get_action_status_date() as it doesn't necessarily make sense to filter out claimed actions. --- classes/WP_CLI/System_Command.php | 1 - 1 file changed, 1 deletion(-) diff --git a/classes/WP_CLI/System_Command.php b/classes/WP_CLI/System_Command.php index a936a634..c7c3adaf 100644 --- a/classes/WP_CLI/System_Command.php +++ b/classes/WP_CLI/System_Command.php @@ -262,7 +262,6 @@ protected function get_action_status_date( $status, $date_type = 'oldest' ) { $order = 'oldest' === $date_type ? 'ASC' : 'DESC'; $args = array( - 'claimed' => false, 'status' => $status, 'per_page' => 1, 'order' => $order,