diff --git a/__tests__/unit-tests/fixtures/class-transition-failure-events.php b/__tests__/unit-tests/fixtures/class-transition-failure-events.php new file mode 100644 index 0000000..d0ecf8a --- /dev/null +++ b/__tests__/unit-tests/fixtures/class-transition-failure-events.php @@ -0,0 +1,21 @@ +transition_error = $transition_error; + } + + protected function transition_event( Event $event ) { + return $this->transition_error; + } +} diff --git a/__tests__/unit-tests/test-events.php b/__tests__/unit-tests/test-events.php index 8c0f5c6..2009a0f 100644 --- a/__tests__/unit-tests/test-events.php +++ b/__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; diff --git a/includes/class-events.php b/includes/class-events.php index 38508ba..1efdc95 100644 --- a/includes/class-events.php +++ b/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 ); }