Atomic option locking

classes/ActionScheduler_OptionLock.php

@@ -24,7 +24,37 @@ class ActionScheduler_OptionLock extends ActionScheduler_Lock {
 	 * @bool True if lock value has changed, false if not or if set failed.
 	 */
 	public function set( $lock_type ) {
-		return update_option( $this->get_key( $lock_type ), time() + $this->get_duration( $lock_type ) );
+		global $wpdb;
+
+		$lock_key            = $this->get_key( $lock_type );
+		$existing_lock_value = $this->get_existing_lock( $lock_type );
+		$new_lock_value      = $this->new_lock_value( $lock_type );
+
+		// The lock may not exist yet, or may have been deleted.
+		if ( empty( $existing_lock_value ) ) {
+			return (bool) $wpdb->insert(
+				$wpdb->options,
+				array(
+					'option_name'  => $lock_key,
+					'option_value' => $new_lock_value,
+					'autoload'     => 'no',
+				)
+			);
+		}
+
+		if ( $this->get_expiration_from( $existing_lock_value ) >= time() ) {
+			return false;
+		}
+
+		// Otherwise, try to obtain the lock.
+		return (bool) $wpdb->update(
+			$wpdb->options,
+			array( 'option_value' => $new_lock_value ),
+			array(
+				'option_name'  => $lock_key,
+				'option_value' => $existing_lock_value,
+			)
+		);
 	}
 
 	/**
@@ -34,7 +64,30 @@ public function set( $lock_type ) {
 	 * @return bool|int False if no lock is set, otherwise the timestamp for when the lock is set to expire.
 	 */
 	public function get_expiration( $lock_type ) {
-		return get_option( $this->get_key( $lock_type ) );
+		return $this->get_expiration_from( $this->get_existing_lock( $lock_type ) );
+	}
+
+	/**
+	 * Given the lock string, derives the lock expiration timestamp (or false if it cannot be determined).
+	 *
+	 * @param string $lock_value String containing a timestamp, or pipe-separated combination of unique value and timestamp.
+	 *
+	 * @return false|int
+	 */
+	private function get_expiration_from( $lock_value ) {
+		$lock_string = explode( '|', $lock_value );
+
+		// Old style lock?
+		if ( count( $lock_string ) === 1 && is_numeric( $lock_string[0] ) ) {
+			return (int) $lock_string[0];
+		}
+
+		// New style lock?
+		if ( count( $lock_string ) === 2 && is_numeric( $lock_string[1] ) ) {
+			return (int) $lock_string[1];
+		}
+
+		return false;
 	}
 
 	/**
@@ -46,4 +99,37 @@ public function get_expiration( $lock_type ) {
 	protected function get_key( $lock_type ) {
 		return sprintf( 'action_scheduler_lock_%s', $lock_type );
 	}
+
+	/**
+	 * Supplies the existing lock value, or an empty string if not set.
+	 *
+	 * @param string $lock_type A string to identify different lock types.
+	 *
+	 * @return string
+	 */
+	private function get_existing_lock( $lock_type ) {
+		global $wpdb;
+
+		// Now grab the existing lock value, if there is one.
+		return (string) $wpdb->get_var(
+			$wpdb->prepare(
+				"SELECT option_value FROM $wpdb->options WHERE option_name = %s",
+				$this->get_key( $lock_type )
+			)
+		);
+	}
+
+	/**
+	 * Supplies a lock value consisting of a unique value and the current timestamp, which are separated by a pipe
+	 * character.
+	 *
+	 * Example: (string) "649de012e6b262.09774912|1688068114"
+	 *
+	 * @param string $lock_type A string to identify different lock types.
+	 *
+	 * @return string
+	 */
+	private function new_lock_value( $lock_type ) {
+		return uniqid( '', true ) . '|' . ( time() + $this->get_duration( $lock_type ) );
+	}
 }

classes/ActionScheduler_QueueRunner.php

@@ -103,9 +103,12 @@ public function unhook_dispatch_async_request() {
 	 * should dispatch a request to process pending actions.
 	 */
 	public function maybe_dispatch_async_request() {
-		if ( is_admin() && ! ActionScheduler::lock()->is_locked( 'async-request-runner' ) ) {
-			// Only start an async queue at most once every 60 seconds
-			ActionScheduler::lock()->set( 'async-request-runner' );
+		// Only start an async queue at most once every 60 seconds.
+		if (
+			is_admin()
+			&& ! ActionScheduler::lock()->is_locked( 'async-request-runner' )
+			&& ActionScheduler::lock()->set( 'async-request-runner' )
+		) {
 			$this->async_request->maybe_dispatch();
 		}
 	}

classes/abstracts/ActionScheduler_Lock.php

@@ -26,6 +26,8 @@ public function is_locked( $lock_type ) {
 	/**
 	 * Set a lock.
 	 *
+	 * To prevent race conditions, implementations should avoid setting the lock if the lock is already held.
+	 *
 	 * @param string $lock_type A string to identify different lock types.
 	 * @return bool
 	 */

tests/phpunit/lock/ActionScheduler_OptionLock_Test.php

@@ -42,4 +42,32 @@ public function test_get_expiration() {
 		$this->assertGreaterThan( $current_time, $expiration );
 		$this->assertLessThan( $current_time + MINUTE_IN_SECONDS + 1, $expiration );
 	}
+
+	/**
+	 * A call to `ActionScheduler::lock()->set()` should fail if the lock is already held (ie, by another process).
+	 *
+	 * @return void
+	 */
+	public function test_lock_resists_race_conditions() {
+		$lock = ActionScheduler::lock();
+		$type = md5( wp_rand() );
+
+		// Approximate conditions in which a concurrently executing request manages to set (and obtain) the lock
+		// immediately before the current request can do so.
+		$simulate_concurrent_claim = function ( $query ) use ( $lock, $type ) {
+			static $executed = false;
+
+			if ( ! $executed && false !== strpos( $query, 'action_scheduler_lock_' ) && 0 === strpos( $query, 'UPDATE' ) ) {
+				$executed = true;
+				$lock->set( $type );
+			}
+
+			return $query;
+		};
+
+		add_filter( 'query', $simulate_concurrent_claim );
+		$this->assertFalse( $lock->is_locked( $type ), 'Initially, the lock is not held' );
+		$this->assertFalse( $lock->set( $type ), 'The lock was not obtained, because another process already claimed it.' );
+		remove_filter( 'query', $simulate_concurrent_claim );
+	}
 }