Only release claims on pending actions.

[prettyboymp]

Fixes: #1105, improves #1104

The current query for claiming actions can get really slow for a sites that process a ton of actions. Part of the cause to this is that all actions are reset back to claim_id 0 after a batch is processed. This increases the amount of time needed to query the pending actions that need to be claimed because the initial segment of the index used to claim actions is heavily seeded with 0 values causing the optimizer to frequently choose a less performant index.

This improves that by no longer resetting the claim_id of completed or running actions back to 0 when a claim is released. Since only pending actions are still processed, leaving these with their old shouldn't cause any side-effects, and if anything improves debugging potential issues since the original claim that processed any actions will still be present.

Testing Instructions:

1. Clear out any existing completed or failed actions that are already set to claim_id=0 or take note of the current count to verify the results of the changes later:

-- Remove Existing:
DELETE from wp_actionscheduler_actions where claim_id = 0 and status != 'pending';
-- Get the count:
SELECT count(*) from wp_actionscheduler_actions where claim_id = 0 and status != 'pending';

2. Generate actions to be processed. You can use the code below in a mu-plugins file and a wp-cli command wp generate_actions to create a bunch of pseudo actions to be processed.

/**
 * Actions used for testing.
 */
function schedule_random_action() {

	$foo         = rand( 0, 10000 );
	$priority    = rand( 0, 255 );
	$time_offset = rand( 1, 24 ) * HOUR_IN_SECONDS;

	return as_schedule_single_action( time() + $time_offset, 'as_performance_testing_action', array( 'foo' => $foo ), '', false, $priority );
}

// The action has to have a callback to avoid being marked as failed.
add_action( 'as_performance_testing_action', function() {
  // do nothing.
});


if ( class_exists( 'WP_CLI' ) ) {
	/**
	 * Generate 50K actions for testing.  These are no-op callbacks so that we're testing the load of scheduling, not the actions themselves.
	 *
	 * Example:
	 *  wp generate_actions
	 */
	WP_CLI::add_command(
		'generate_actions',
		function () {
			if ( ! did_action( 'action_scheduler_init' ) ) {
				WP_CLI::error( 'ActionScheduler is not initialized!!!' );
			}

			global $wpdb;
			WP_CLI::line( ' current actions: ' . $wpdb->get_var( "SELECT COUNT(*) FROM wp_actionscheduler_actions" ) );

			$num_to_create = 50000;

			for ( $i = 0; $i < $num_to_create; $i ++ ) {
				if ( ! schedule_random_action() ) {
					WP_CLI::warning( 'action not created' );
				}
			}
			WP_CLI::success( "Created $num_to_create actions" );
		}
	);
}

3. Open the admin and allow the queue to process over time.
4. Verify that no new non-pending actions have a claim_id = 0: SELECT count(*) from wp_actionscheduler_actions where claim_id = 0 and status != 'pending'.

Note that these changes will not effect actions that have already been completed or failed as those really no longer matter to the system and are only retained for a period of time for admin review. They will eventually get cleaned up via a job.

[prettyboymp]

What I'm not sure is the potential backward compatibility conflicts here. I don't know why existing extensions would need completed actions to get reset back to claim_id 0, but we can't be sure that is the case.

[joshuatf]

Actions on my end still appear to be reset to 0. Here's what I did:

1. Ran wp generate_actions which created a lot of as_performance_testing_action actions
2. Manually run the actions by visiting Tools->Actoin Schedule
3. Run the above SQL query and see actions are set to 0 after completion.

Are there any other steps I need to take or that I did wrong?

[prettyboymp]

Actions on my end still appear to be reset to 0. Here's what I did:

1. Ran wp generate_actions which created a lot of as_performance_testing_action actions
2. Manually run the actions by visiting Tools->Actoin Schedule
3. Run the above SQL query and see actions are set to 0 after completion.

Are there any other steps I need to take or that I did wrong?

@joshuatf , sorry, I made an assumption that the site wouldn't have any existing actions that were already failed or completed that were reset to claim_id 0. I've updated the instructions. Can you confirm whether those were new actions or not?

[joshuatf]

Hey @prettyboymp, sorry for the late review. This is happening on new actions, though it doesn't seem related to the area you touched.

The code you've added otherwise LGTM; I'm just not entirely sure where claims are being released in my case.

It might be good to get someone else more familiar with AS to take a glance at this and give final approval, especially around any potential backwards compatibility issues. cc @jorgeatorres

[joshuatf]

I can confirm that this array is empty and the below query to set the claim_id = 0 does not get run. However, my claims in the DB are still getting set to 0.

Is there anywhere else this might be happening or is it possible for multiple instances of AS to be running at once?

[jorgeatorres]

Note that actions processed via Tools > Scheduled Actions are not claimed/released. Is this how you're running the actions @joshuatf?

For the runner to properly stake a claim and then release the actions, you'd have to either let those actions run naturally or use a runner such as the CLI one (i.e. run wp action-scheduler run).

[jorgeatorres]

This looks good to me @prettyboymp and tests well. I like the performance improvements this can bring and also that by retaining the claim ID we've gained a bit on the auditing side, which is always useful.

I'm not too concerned about the backwards compatibility of this change in the sense that 3rd parties need completed actions to have claim_id = 0. In theory they shouldn't necessarily be querying the database directly.

That said, we do have a claimed field that can be used to query actions via as_get_scheduled_actions() (implemented as query_actions() in the data stores). We would be changing the behavior here as claimed = true wouldn't have returned completed actions prior to this change. Maybe we can "force" the query function to ignore non-pending actions when the claimed = true arg is provided.

FWIW, this arg is used in a few places, and it'd be good to check that we're not breaking anything in those places. In particular, the queue cleaner seems fine, but other integrations such as the one with WC's SSR might need some adjustments.

What do you think?

[prettyboymp]

@jorgeatorres, I updated the status reports to no longer include the claimed parameter since it doesn't really make sense to have them applied when searching for the oldest/newest of each action status.

That said, we do have a claimed field that can be used to query actions via as_get_scheduled_actions() (implemented as query_actions() in the data stores). We would be changing the behavior here as claimed = true wouldn't have returned completed actions prior to this change. Maybe we can "force" the query function to ignore non-pending actions when the claimed = true arg is provided.

I’m a bit hesitant to introduce forced manipulation of the status query argument for special cases where claimed is set, mainly because I’m not aware of any use cases that would be impacted by completed actions remaining in a claimed state—aside from scenarios explicitly trying to retrieve currently claimed actions. Even in those cases, unless the status parameter is also specified, the query could return non-pending actions while a claim is still active, since the claim is only released after all actions are processed.

I also reviewed all plugins on WooCommerce.com and didn’t find any instances where claimed was used without a specific status parameter, apart from what we’ve already accounted for within Action Scheduler itself.