fix(events): prevent callbacks after failed transitions

__tests__/unit-tests/fixtures/class-transition-failure-events.php

@@ -0,0 +1,21 @@
+<?php
+
+namespace Automattic\WP\Cron_Control\Tests;
+
+use Automattic\WP\Cron_Control\Event;
+use Automattic\WP\Cron_Control\Events;
+use WP_Error;
+
+class Transition_Failure_Events extends Events {
+	private $transition_error;
+
+	protected function class_init() {}
+
+	public function set_transition_error( WP_Error $transition_error ): void {
+		$this->transition_error = $transition_error;
+	}
+
+	protected function transition_event( Event $event ) {
+		return $this->transition_error;
+	}
+}

__tests__/unit-tests/test-events.php

@@ -4,6 +4,10 @@
 
 use Automattic\WP\Cron_Control\Events;
 use Automattic\WP\Cron_Control\Event;
+use Automattic\WP\Cron_Control\Lock;
+use WP_Error;
+
+require_once __DIR__ . '/fixtures/class-transition-failure-events.php';
 
 class Events_Tests extends \WP_UnitTestCase {
 	public function setUp(): void {
@@ -85,6 +89,57 @@ public function test_flatten_wp_events_array() {
 		// Could maybe test more here, but honestly feels like it would couple too closely to the implementation itself.
 	}
 
+	public function test_run_event_does_not_run_a_callback_after_a_failed_complete() {
+		$this->assert_transition_failure_prevents_callback( array() );
+	}
+
+	public function test_run_event_does_not_run_a_callback_after_a_failed_reschedule() {
+		$this->assert_transition_failure_prevents_callback(
+			array(
+				'schedule' => 'hourly',
+				'interval' => HOUR_IN_SECONDS,
+			)
+		);
+	}
+
+	public function test_run_event_releases_only_the_action_lock_for_an_internal_event_after_a_failed_transition() {
+		$this->assert_transition_failure_prevents_callback(
+			array(
+				'action' => 'a8c_cron_control_purge_completed_events',
+			)
+		);
+	}
+
+	private function assert_transition_failure_prevents_callback( array $event_data ) {
+		$called = 0;
+		$action = $event_data['action'] ?? 'test_failed_transition_' . wp_generate_uuid4();
+		add_action(
+			$action,
+			function () use ( &$called ) {
+				++$called;
+			}
+		);
+
+		$event_data['action']     = $action;
+		$event_data['timestamp'] = time();
+		$event                    = Utils::create_test_event( $event_data );
+		$events                   = Transition_Failure_Events::instance();
+		$error                    = new WP_Error( 'cron-control:event:failed-update' );
+		$events->set_transition_error( $error );
+		Lock::prime_lock( Events::LOCK );
+
+		if ( $event->is_internal() ) {
+			Lock::check_lock( Events::LOCK, 1 );
+		}
+
+		$result = $events->run_event( $event->get_timestamp(), md5( $event->get_action() ), $event->get_instance() );
+
+		$this->assertSame( $error, $result );
+		$this->assertSame( 0, $called );
+		$this->assertSame( $event->is_internal() ? 1 : 0, Lock::get_lock_value( Events::LOCK ) );
+		$this->assertSame( 0, Lock::get_lock_value( $events->get_lock_key_for_event_action( $event ) ) );
+	}
+
 	private function create_test_events() {
 		$time_one = 1400000000;
 		$time_two = 1500000000;

includes/class-events.php

@@ -256,10 +256,11 @@ public function run_event( $timestamp, $action, $instance, $force = false ) {
 		}
 
 		// Core reschedules/conpletes an event before running it, so we respect that.
-		if ( $event->is_recurring() ) {
-			$event->reschedule();
-		} else {
-			$event->complete();
+		$transition_result = $this->transition_event( $event );
+
+		if ( is_wp_error( $transition_result ) ) {
+			$this->release_event_locks( $event, $force );
+			return $transition_result;
 		}
 
 		try {
@@ -280,13 +281,7 @@ public function run_event( $timestamp, $action, $instance, $force = false ) {
 		}
 
 		// Free locks for the next event, unless they weren't set to begin with.
-		if ( ! $force ) {
-			// If we got this far, there's no uncaught error to handle.
-			$this->running_event = null;
-			remove_action( 'shutdown', array( $this, 'do_lock_cleanup_on_shutdown' ) );
-
-			$this->do_lock_cleanup( $event );
-		}
+		$this->release_event_locks( $event, $force );
 
 		// Callback didn't trigger a Throwable, indicating it succeeded.
 		if ( ! isset( $return ) ) {
@@ -300,6 +295,27 @@ public function run_event( $timestamp, $action, $instance, $force = false ) {
 		return $return;
 	}
 
+	private function release_event_locks( Event $event, bool $force ): void {
+		if ( $force ) {
+			return;
+		}
+
+		// If we got this far, there's no uncaught error to handle.
+		$this->running_event = null;
+		remove_action( 'shutdown', array( $this, 'do_lock_cleanup_on_shutdown' ) );
+		$this->do_lock_cleanup( $event );
+	}
+
+	/**
+	 * Persist the event state required before executing its callback.
+	 *
+	 * @param Event $event Event to transition.
+	 * @return true|WP_Error
+	 */
+	protected function transition_event( Event $event ) {
+		return $event->is_recurring() ? $event->reschedule() : $event->complete();
+	}
+
 	private function prime_event_action_lock( Event $event ): void {
 		Lock::prime_lock( $this->get_lock_key_for_event_action( $event ), JOB_LOCK_EXPIRY_IN_MINUTES * \MINUTE_IN_SECONDS );
 	}