Implement SKIP LOCKED during action claiming

.github/workflows/pr-unit-tests.yml

@@ -3,16 +3,17 @@ on:
   pull_request
 jobs:
   test:
-    name: PHP ${{ matrix.php }} WP ${{ matrix.wp }} MU ${{ matrix.multisite }}
+    name: PHP ${{ matrix.php }} WP ${{ matrix.wp }} MU ${{ matrix.multisite }} DB ${{ matrix.db }}
     timeout-minutes: 15
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
       matrix:
         # We test against the earliest and latest PHP versions for each major supported version.
         php: [ '7.1', '7.4', '8.0', '8.3' ]
-        wp: [ '6.4', '6.5', '6.6', 'latest', 'nightly' ]
+        wp: [ '6.5', '6.6', 'latest', 'nightly' ]
         multisite: [ '0', '1' ]
+        db: [ 'mysql:5.6', 'mysql:8.1', 'mariadb:10.4', 'mariadb:10.6']
         exclude:
           # WordPress 6.6+ requires PHP 7.2+
           - php: 7.1
@@ -23,7 +24,7 @@ jobs:
             wp: nightly
     services:
       database:
-        image: mysql:5.6
+        image: ${{ matrix.db }}
         env:
           MYSQL_ROOT_PASSWORD: root
         ports:
@@ -85,7 +86,12 @@ jobs:
         run: sudo apt-get update && sudo apt-get install -y subversion
 
       - name: Init DB and WP
-        run: ./tests/bin/install.sh woo_test root root 127.0.0.1 ${{ matrix.wp }}
+        run: |
+          # Use mysql_native_password when using PHP < 7.4 and MySQL >= 8.0
+          if [ "$(php -r 'echo version_compare(PHP_VERSION, "7.4", "<");')" -eq 1 ] && [ "${{ matrix.db }}" == "mysql:8.1" ]; then
+            mysql -uroot -proot -h127.0.0.1 -e "ALTER USER 'root'@'%' IDENTIFIED WITH mysql_native_password BY 'root'; FLUSH PRIVILEGES;"
+          fi
+          ./tests/bin/install.sh woo_test root root 127.0.0.1 ${{ matrix.wp }}
 
       - name: Run tests
         run: |

classes/ActionScheduler_DataController.php

@@ -162,10 +162,19 @@ public static function free_memory() {
 			return;
 		}
 
-		$wp_object_cache->group_ops      = array();
-		$wp_object_cache->stats          = array();
-		$wp_object_cache->memcache_debug = array();
-		$wp_object_cache->cache          = array();
+		// Not all drop-ins support these props, however, there may be existing installations that rely on these being cleared.
+		if ( property_exists( $wp_object_cache, 'group_ops' ) ) {
+			$wp_object_cache->group_ops = array();
+		}
+		if ( property_exists( $wp_object_cache, 'stats' ) ) {
+			$wp_object_cache->stats = array();
+		}
+		if ( property_exists( $wp_object_cache, 'memcache_debug' ) ) {
+			$wp_object_cache->memcache_debug = array();
+		}
+		if ( property_exists( $wp_object_cache, 'cache' ) ) {
+			$wp_object_cache->cache = array();
+		}
 
 		if ( is_callable( array( $wp_object_cache, '__remoteset' ) ) ) {
 			call_user_func( array( $wp_object_cache, '__remoteset' ) ); // important!

classes/data-stores/ActionScheduler_DBStore.php

@@ -931,16 +931,8 @@ protected function claim_actions( $claim_id, $limit, ?DateTime $before_date = nu
 		 * @var \wpdb $wpdb
 		 */
 		global $wpdb;
-
 		$now  = as_get_datetime_object();
 		$date = is_null( $before_date ) ? $now : clone $before_date;
-		// can't use $wpdb->update() because of the <= condition.
-		$update = "UPDATE {$wpdb->actionscheduler_actions} SET claim_id=%d, last_attempt_gmt=%s, last_attempt_local=%s";
-		$params = array(
-			$claim_id,
-			$now->format( 'Y-m-d H:i:s' ),
-			current_time( 'mysql' ),
-		);
 
 		// Set claim filters.
 		if ( ! empty( $hooks ) ) {
@@ -954,14 +946,16 @@ protected function claim_actions( $claim_id, $limit, ?DateTime $before_date = nu
 			$group = $this->get_claim_filter( 'group' );
 		}
 
-		$where    = 'WHERE claim_id = 0 AND scheduled_date_gmt <= %s AND status=%s';
-		$params[] = $date->format( 'Y-m-d H:i:s' );
-		$params[] = self::STATUS_PENDING;
+		$where        = 'WHERE claim_id = 0 AND scheduled_date_gmt <= %s AND status=%s';
+		$where_params = array(
+			$date->format( 'Y-m-d H:i:s' ),
+			self::STATUS_PENDING,
+		);
 
 		if ( ! empty( $hooks ) ) {
 			$placeholders = array_fill( 0, count( $hooks ), '%s' );
-			$where       .= ' AND hook IN (' . join( ', ', $placeholders ) . ')';
-			$params       = array_merge( $params, array_values( $hooks ) );
+			$where        .= ' AND hook IN (' . join( ', ', $placeholders ) . ')';
+			$where_params = array_merge( $where_params, array_values( $hooks ) );
 		}
 
 		$group_operator = 'IN';
@@ -996,23 +990,32 @@ protected function claim_actions( $claim_id, $limit, ?DateTime $before_date = nu
 		/**
 		 * Sets the order-by clause used in the action claim query.
 		 *
-		 * @since 3.4.0
-		 * @since 3.8.3 Made $claim_id and $hooks available.
-		 *
 		 * @param string $order_by_sql
 		 * @param string $claim_id Claim Id.
-		 * @param array  $hooks Hooks to filter for.
+		 * @param array  $hooks    Hooks to filter for.
+		 *
+		 * @since 3.8.3 Made $claim_id and $hooks available.
+		 * @since 3.4.0
 		 */
-		$order    = apply_filters( 'action_scheduler_claim_actions_order_by', 'ORDER BY priority ASC, attempts ASC, scheduled_date_gmt ASC, action_id ASC', $claim_id, $hooks );
-		$params[] = $limit;
+		$order       = apply_filters( 'action_scheduler_claim_actions_order_by', 'ORDER BY priority ASC, attempts ASC, scheduled_date_gmt ASC, action_id ASC', $claim_id, $hooks );
+		$skip_locked = $this->db_supports_skip_locked() ? ' SKIP LOCKED' : '';
+
+		// Selecting the action_ids that we plan to claim, while skipping any locked rows to avoid deadlocking.
+		$select_sql = $wpdb->prepare( "SELECT action_id from {$wpdb->actionscheduler_actions} {$where} {$order} LIMIT %d FOR UPDATE{$skip_locked}", array_merge( $where_params, array( $limit ) ) );
 
-		$sql           = $wpdb->prepare( "{$update} {$where} {$order} LIMIT %d", $params ); // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared, WordPress.DB.PreparedSQLPlaceholders
-		$rows_affected = $wpdb->query( $sql ); // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared, WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching
+		// Now place it into an UPDATE statement by joining the result sets, allowing for the SKIP LOCKED behavior to take effect.
+		$update_sql    = "UPDATE {$wpdb->actionscheduler_actions} t1 JOIN ( $select_sql ) t2 ON t1.action_id = t2.action_id SET claim_id=%d, last_attempt_gmt=%s, last_attempt_local=%s";
+		$update_params = array(
+			$claim_id,
+			$now->format( 'Y-m-d H:i:s' ),
+			current_time( 'mysql' ),
+		);
+
+		$rows_affected = $wpdb->query( $wpdb->prepare( $update_sql, $update_params ) );
 		if ( false === $rows_affected ) {
 			$error = empty( $wpdb->last_error )
 				? _x( 'unknown', 'database error', 'action-scheduler' )
 				: $wpdb->last_error;
-
 			throw new \RuntimeException(
 				sprintf(
 					/* translators: %s database error. */
@@ -1025,6 +1028,43 @@ protected function claim_actions( $claim_id, $limit, ?DateTime $before_date = nu
 		return (int) $rows_affected;
 	}
 
+	/**
+	 * Determines whether the database supports using SKIP LOCKED. This logic mimicks the $wpdb::has_cap() logic.
+	 *
+	 * SKIP_LOCKED support was added to MariaDB in 10.6.0 and to MySQL in 8.0.1
+	 *
+	 * @return bool
+	 */
+	private function db_supports_skip_locked() {
+		global $wpdb;
+		$db_version     = $wpdb->db_version();
+		$db_server_info = $wpdb->db_server_info();
+		$is_mariadb     = ( false !== strpos( $db_server_info, 'MariaDB' ) );
+
+		if ( $is_mariadb &&
+		     '5.5.5' === $db_version &&
+		     PHP_VERSION_ID < 80016 // PHP 8.0.15 or older.
+		) {
+			/*
+			 * Account for MariaDB version being prefixed with '5.5.5-' on older PHP versions.
+			 */
+			$db_server_info = preg_replace( '/^5\.5\.5-(.*)/', '$1', $db_server_info );
+			$db_version     = preg_replace( '/[^0-9.].*/', '', $db_server_info );
+		}
+
+		$is_supported = ( $is_mariadb && version_compare( $db_version, '10.6.0', '>=' ) ) ||
+		                ( ! $is_mariadb && version_compare( $db_version, '8.0.1', '>=' ) );
+
+		/**
+		 * Filter whether the database supports the SKIP LOCKED modifier for queries.
+		 *
+		 * @param bool $is_supported Whether SKIP LOCKED is supported.
+		 *
+		 * @since 3.9.3
+		 */
+		return apply_filters( 'action_scheduler_db_supports_skip_locked', $is_supported );
+	}
+
 	/**
 	 * Get the number of active claims.
 	 *

classes/schema/ActionScheduler_StoreSchema.php

@@ -20,7 +20,7 @@ class ActionScheduler_StoreSchema extends ActionScheduler_Abstract_Schema {
 	 *
 	 * @var int
 	 */
-	protected $schema_version = 7;
+	protected $schema_version = 8;
 
 	/**
 	 * Construct.
@@ -80,7 +80,9 @@ protected function get_table_definition( $table ) {
 				        KEY args (args($max_index_length)),
 				        KEY group_id (group_id),
 				        KEY last_attempt_gmt (last_attempt_gmt),
-				        KEY `claim_id_status_scheduled_date_gmt` (`claim_id`, `status`, `scheduled_date_gmt`)
+				        KEY `claim_id_status_priority_scheduled_date_gmt` (`claim_id`,`status`,`priority`,`scheduled_date_gmt`),
+				        KEY `status_last_attempt_gmt` (`status`,`last_attempt_gmt`),
+				        KEY `status_claim_id` (`status`,`claim_id`)
 				        ) $charset_collate";
 
 			case self::CLAIMS_TABLE:

tests/phpunit/jobstore/ActionScheduler_DBStore_Test.php

@@ -8,6 +8,15 @@
  */
 class ActionScheduler_DBStore_Test extends AbstractStoreTest {
 
+	/**
+	 * Saved instance of wpdb to restore in cases where we replace the global instance with a mock.
+	 *
+	 * @see $this->test_db_supports_skip_locked()
+	 *
+	 * @var \wpdb
+	 */
+	private $original_wpdb;
+
 	public function setUp() {
 		global $wpdb;
 
@@ -17,6 +26,20 @@ public function setUp() {
 		parent::setUp();
 	}
 
+	/**
+	 * Restore the original wpdb global instance for tests that have replaced it.
+	 *
+	 * @return void
+	 */
+	public function tearDown() {
+		global $wpdb;
+		if ( null !== $this->original_wpdb ) {
+			$wpdb                = $this->original_wpdb;
+			$this->original_wpdb = null;
+		}
+		parent::tearDown();
+	}
+
 	/**
 	 * Get data store for tests.
 	 *
@@ -719,4 +742,73 @@ public function test_child_actions_are_processed_in_correct_order() {
 
 		$this->assertEquals( range( 1, 20 ), $actual_order, 'Once claimed, scheduled actions are executed in the expected order, including if "child actions" are scheduled from within another action.' );
 	}
+
+	/**
+	 * @param bool   $expected_result
+	 * @param string $db_server_info
+	 *
+	 * @return void
+	 *
+	 * @dataProvider db_supports_skip_locked_provider
+	 */
+	public function test_db_supports_skip_locked( bool $expected_result, string $db_server_info ) {
+		global $wpdb;
+
+		// Stash the original since we're overwriting it with a partial mock. Self::tear_down() will restore this.
+		$this->original_wpdb = $wpdb;
+
+		$wpdb = $this->getMockBuilder( get_class( $wpdb ) )
+		             ->setMethods( [ 'db_server_info' ] )
+		             ->disableOriginalConstructor()
+		             ->getMock();
+
+		$wpdb->method( 'db_server_info' )->willReturn( $db_server_info );
+
+		$reflection = new \ReflectionClass( ActionScheduler_DBStore::class );
+		$method     = $reflection->getMethod( 'db_supports_skip_locked' );
+		$method->setAccessible( true );
+		$db_store = new ActionScheduler_DBStore();
+		$this->assertSame( $expected_result, $method->invoke( $db_store ) );
+	}
+
+	/**
+	 * Data Provider for ::test_db_supports_skip_locked().
+	 *
+	 * @return array[]
+	 */
+	public static function db_supports_skip_locked_provider(): array {
+		// PHP <= 8.0.15 didn't strip the 5.5.5- prefix for MariaDB.
+		$maria_db_prefix = PHP_VERSION_ID < 80016 ? '5.5.5-' : '';
+
+		return array(
+			'MySQL 5.6.1 does not support skip locked'    => array(
+				false,
+				'5.6.1'
+			),
+			'MySQL 8.0.0 does not support skip locked'    => array(
+				false,
+				'8.0.0'
+			),
+			'MySQL 8.0.1 does support skip locked'        => array(
+				true,
+				'8.0.1'
+			),
+			'MySQL 8.4.4 does support skip locked'        => array(
+				true,
+				'8.4.4'
+			),
+			'MariaDB 10.5.0 does not support skip locked' => array(
+				false,
+				$maria_db_prefix . '10.5.0-MariaDB'
+			),
+			'MariaDB 10.6.0 does support skip locked'     => array(
+				true,
+				$maria_db_prefix . '10.6.0-MariaDB'
+			),
+			'MariaDB 11.5.0 does support skip locked'     => array(
+				true,
+				$maria_db_prefix . '11.5.0-MariaDB'
+			),
+		);
+	}
 }

tests/phpunit/procedural_api/procedural_api_Test.php

@@ -449,7 +449,7 @@ public function test_as_recover_from_incorrect_schema() {
 		$this->assertContains( 'Caught exception while enqueuing action "hook_18": Error saving action', $logged_errors );
 		$this->assertContains( 'Caught exception while enqueuing action "hook_19": Error saving action', $logged_errors );
 		$this->assertContains( 'Caught exception while enqueuing action "hook_20": Error saving action', $logged_errors );
-		$this->assertContains( "Unknown column 'priority' in 'field list'", $logged_errors );
+		$this->assertContains( "Unknown column 'priority' in ", $logged_errors );
 
 		// recreate the priority column.
 		$wpdb->query( "ALTER TABLE {$wpdb->actionscheduler_actions} ADD COLUMN priority tinyint(10) UNSIGNED NOT NULL DEFAULT 10" );