From 83da2469d6e065d9f76f0f1fc9506659a491e2c7 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:09:37 +0300 Subject: [PATCH 01/10] fix: survive a failed action that another process already handled Action Scheduler's `mark_failure()` throws "Unidentified action" when its UPDATE changes no row. That happens when another process deleted the action, and also when an overlapping cleaner already marked it failed: WP-Cron's queue run takes no lock, only the async runner does. Two callers let that exception escape and end the request with a fatal: the queue cleaner loop and the runner's action error path. The bundled copy is patched at `composer install` so both callers skip that action and continue, the same guard `delete_actions()` already uses. `composer-exit-on-patch-failure` makes a future Action Scheduler bump fail loudly instead of dropping the fix. Upstream 4.2.0 still throws; see woocommerce/action-scheduler#970. Refs: #1369 Co-Authored-By: Claude Fable 5.1 --- .distignore | 2 + composer.json | 14 +- composer.lock | 50 ++++- .../action-scheduler-1369-mark-failures.patch | 31 ++++ tests/test-action-scheduler-mark-failure.php | 173 ++++++++++++++++++ 5 files changed, 267 insertions(+), 3 deletions(-) create mode 100644 patches/action-scheduler-1369-mark-failures.patch create mode 100644 tests/test-action-scheduler-mark-failure.php diff --git a/.distignore b/.distignore index 30b35e1c3..94f2659f6 100755 --- a/.distignore +++ b/.distignore @@ -35,3 +35,5 @@ skills classes/Visualizer/Gutenberg/src classes/Visualizer/ChartBuilder/src classes/Visualizer/D3Renderer/src +patches +vendor/cweagans diff --git a/composer.json b/composer.json index 1153b42df..bbdb4cb0b 100644 --- a/composer.json +++ b/composer.json @@ -24,7 +24,8 @@ "codeinwp/themeisle-sdk": "^3.3", "neitanod/forceutf8": "~2.0", "openspout/openspout": "^3.7", - "woocommerce/action-scheduler": "^3.8" + "woocommerce/action-scheduler": "^3.8", + "cweagans/composer-patches": "^1.7" }, "autoload": { "files": [ @@ -45,7 +46,8 @@ "php": "7.4" }, "allow-plugins": { - "dealerdirect/phpcodesniffer-composer-installer": true + "dealerdirect/phpcodesniffer-composer-installer": true, + "cweagans/composer-patches": true } }, "require-dev": { @@ -55,5 +57,13 @@ "phpstan/phpstan": "^2.1", "szepeviktor/phpstan-wordpress": "^2.0", "yoast/phpunit-polyfills": "^4.0" + }, + "extra": { + "patches": { + "woocommerce/action-scheduler": { + "Tolerate an action already failed or deleted by another process when marking it failed (#1369, upstream #970)": "patches/action-scheduler-1369-mark-failures.patch" + } + }, + "composer-exit-on-patch-failure": true } } diff --git a/composer.lock b/composer.lock index 79c20cde6..a9269aa05 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "881b5f99c0b72f47e79eb09bdac7fbea", + "content-hash": "54649940158be9c9d52224064f0f3be3", "packages": [ { "name": "codeinwp/themeisle-sdk", @@ -47,6 +47,54 @@ }, "time": "2026-08-24T15:59:27+00:00" }, + { + "name": "cweagans/composer-patches", + "version": "1.7.3", + "source": { + "type": "git", + "url": "https://github.com/cweagans/composer-patches.git", + "reference": "e190d4466fe2b103a55467dfa83fc2fecfcaf2db" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/cweagans/composer-patches/zipball/e190d4466fe2b103a55467dfa83fc2fecfcaf2db", + "reference": "e190d4466fe2b103a55467dfa83fc2fecfcaf2db", + "shasum": "" + }, + "require": { + "composer-plugin-api": "^1.0 || ^2.0", + "php": ">=5.3.0" + }, + "require-dev": { + "composer/composer": "~1.0 || ~2.0", + "phpunit/phpunit": "~4.6" + }, + "type": "composer-plugin", + "extra": { + "class": "cweagans\\Composer\\Patches" + }, + "autoload": { + "psr-4": { + "cweagans\\Composer\\": "src" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "BSD-3-Clause" + ], + "authors": [ + { + "name": "Cameron Eagans", + "email": "me@cweagans.net" + } + ], + "description": "Provides a way to patch Composer packages.", + "support": { + "issues": "https://github.com/cweagans/composer-patches/issues", + "source": "https://github.com/cweagans/composer-patches/tree/1.7.3" + }, + "time": "2022-12-20T22:53:13+00:00" + }, { "name": "neitanod/forceutf8", "version": "v2.0.4", diff --git a/patches/action-scheduler-1369-mark-failures.patch b/patches/action-scheduler-1369-mark-failures.patch new file mode 100644 index 000000000..b884d3579 --- /dev/null +++ b/patches/action-scheduler-1369-mark-failures.patch @@ -0,0 +1,31 @@ +--- a/classes/ActionScheduler_QueueCleaner.php ++++ b/classes/ActionScheduler_QueueCleaner.php +@@ -222,7 +222,12 @@ + ); + + foreach ( $actions_to_reset as $action_id ) { +- $this->store->mark_failure( $action_id ); ++ try { ++ $this->store->mark_failure( $action_id ); ++ } catch ( Exception $e ) { ++ // Deleted or changed by another process between the query and the update: nothing left to mark. ++ continue; ++ } + do_action( 'action_scheduler_failed_action', $action_id, $timeout ); + } + } +--- a/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php ++++ b/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php +@@ -132,7 +132,11 @@ + */ + private function handle_action_error( $action_id, $e, $context, $valid_action ) { + if ( $valid_action ) { +- $this->store->mark_failure( $action_id ); ++ try { ++ $this->store->mark_failure( $action_id ); ++ } catch ( Exception $ignored ) { ++ // Already marked failed by the queue cleaner, or deleted by another process: nothing left to mark. ++ } + /** + * Runs when action execution fails. + * diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php new file mode 100644 index 000000000..dce264e29 --- /dev/null +++ b/tests/test-action-scheduler-mark-failure.php @@ -0,0 +1,173 @@ +markTestSkipped( 'Action Scheduler is not loaded.' ); + } + + $loaded = wp_normalize_path( ActionScheduler::plugin_path( '' ) ); + $ours = wp_normalize_path( VISUALIZER_ABSPATH . '/vendor/woocommerce/action-scheduler' ); + if ( 0 !== strpos( $loaded, $ours ) ) { + $this->markTestSkipped( 'Another Action Scheduler copy is loaded: ' . $loaded ); + } + + $this->store = new ActionScheduler_DBStore(); + $this->store->init(); + } + + /** + * Insert a stale in-progress action (last attempt two hours ago). + * + * @return int Action id. + */ + private function seed_stale_running_action() { + global $wpdb; + $gmt = gmdate( 'Y-m-d H:i:s', time() - 2 * HOUR_IN_SECONDS ); + $wpdb->insert( + $wpdb->actionscheduler_actions, + array( + 'hook' => 'visualizer_schedule_refresh_db', + 'status' => ActionScheduler_Store::STATUS_RUNNING, + 'scheduled_date_gmt' => $gmt, + 'scheduled_date_local' => $gmt, + 'args' => '[]', + 'schedule' => '', + 'group_id' => 0, + 'attempts' => 1, + 'last_attempt_gmt' => $gmt, + 'last_attempt_local' => $gmt, + 'claim_id' => 0, + ) + ); + return (int) $wpdb->insert_id; + } + + /** + * Run the cleaner's mark_failures() while "another process" acts on one + * action the moment the cleaner issues its UPDATE for it. + * + * @param int $action_id Action the other process touches. + * @param callable $other_process Runs once, right before that UPDATE. + */ + private function mark_failures_racing( $action_id, callable $other_process ) { + global $wpdb; + $table = $wpdb->actionscheduler_actions; + $filter = static function ( $sql ) use ( $action_id, $table, $other_process ) { + if ( 0 === stripos( ltrim( $sql ), 'UPDATE' ) && false !== strpos( $sql, $table ) && preg_match( '/action_id` = (\d+)/', $sql, $m ) && (int) $m[1] === $action_id ) { + $other_process(); + } + return $sql; + }; + add_filter( 'query', $filter ); + try { + ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); + } finally { + remove_filter( 'query', $filter ); + } + } + + /** + * Status column of one action, or null when the row is gone. + * + * @param int $action_id Action id. + * @return string|null + */ + private function status_of( $action_id ) { + global $wpdb; + return $wpdb->get_var( $wpdb->prepare( "SELECT status FROM {$wpdb->actionscheduler_actions} WHERE action_id = %d", $action_id ) ); + } + + /** + * Cleanup keeps going when an action vanishes between its query and its update. + */ + public function test_mark_failures_survives_an_action_deleted_by_another_process() { + global $wpdb; + $vanishing = $this->seed_stale_running_action(); + $survivor = $this->seed_stale_running_action(); + + $this->mark_failures_racing( + $vanishing, + static function () use ( $wpdb, $vanishing ) { + $wpdb->delete( $wpdb->actionscheduler_actions, array( 'action_id' => $vanishing ) ); + } + ); + + $this->assertNull( $this->status_of( $vanishing ), 'the concurrently deleted action stays gone' ); + $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $survivor ), 'cleanup continues and marks the remaining stale action failed' ); + } + + /** + * An overlapping cleaner marked it first. The UPDATE then changes nothing, + * MySQL reports zero rows, and the store throws as if the row were gone. + */ + public function test_mark_failures_survives_an_action_already_failed_by_an_overlapping_cleaner() { + global $wpdb; + $raced = $this->seed_stale_running_action(); + $survivor = $this->seed_stale_running_action(); + + $this->mark_failures_racing( + $raced, + static function () use ( $wpdb, $raced ) { + $wpdb->update( $wpdb->actionscheduler_actions, array( 'status' => ActionScheduler_Store::STATUS_FAILED ), array( 'action_id' => $raced ) ); + } + ); + + $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $raced ) ); + $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $survivor ), 'cleanup continues past the action the other cleaner already handled' ); + } + + /** + * Same hole on the processing path (the trace in upstream #970): a long + * action gets marked failed by the cleaner while it runs, then throws; + * marking it failed again changes no row. + */ + public function test_process_action_survives_marking_an_already_failed_action() { + global $wpdb; + $hook = 'visualizer_test_throwing_action'; + $action_id = $this->store->save_action( new ActionScheduler_Action( $hook, array(), new ActionScheduler_SimpleSchedule( as_get_datetime_object( '-1 minute' ) ) ) ); + + add_action( + $hook, + static function () use ( $wpdb, $action_id ) { + $wpdb->update( $wpdb->actionscheduler_actions, array( 'status' => ActionScheduler_Store::STATUS_FAILED ), array( 'action_id' => $action_id ) ); + throw new RuntimeException( 'refresh failed' ); + } + ); + + ( new ActionScheduler_QueueRunner( $this->store ) )->process_action( $action_id, 'test' ); + + $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $action_id ) ); + } +} From 87c183cbca8f18ef70d049729de375c2bb3c6a3b Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:38:14 +0300 Subject: [PATCH 02/10] test: keep WordPress update checks out of AJAX tests for any file order The framework snapshots hooks at the first test of the run and restores that snapshot after every test. WP_Ajax_UnitTestCase removes the `_maybe_update_*` admin_init hooks once per class, which only holds when an AJAX class runs first. A test file that sorts before test-ajax.php put the hooks back into the snapshot, so every later AJAX test called api.wordpress.org and failed on the response. Remove the hooks in the bootstrap so the order of test files does not matter. Co-Authored-By: Claude Fable 5.1 --- tests/bootstrap.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 230071c9a..ef7f6d385 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -32,6 +32,14 @@ function _manually_load_plugin() { tests_add_filter( 'muplugins_loaded', '_manually_load_plugin' ); // Start up the WP testing environment. require $_tests_dir . '/includes/bootstrap.php'; + +// The framework snapshots hooks at the first test and restores that snapshot +// after every test. WP_Ajax_UnitTestCase removes these once per class, which +// only holds when an AJAX class runs first. Remove them here so no test file +// order makes AJAX tests call api.wordpress.org. +remove_action( 'admin_init', '_maybe_update_core' ); +remove_action( 'admin_init', '_maybe_update_plugins' ); +remove_action( 'admin_init', '_maybe_update_themes' ); activate_plugin( 'visualizer/index.php' ); global $current_user; $current_user = new WP_User( 1 ); From 8bffd5bdff7deb3cd73aebbcb47bc0761ef63e6e Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:51:22 +0300 Subject: [PATCH 03/10] fix: let real database errors surface when marking an action failed The guard caught every Exception, so a database error inside `mark_failure()` was swallowed together with the race it targets. The store throws the same InvalidArgumentException for both; only `$wpdb->last_error` tells them apart. Catch that type only and rethrow when the database reported an error. Tests: one-shot guard in the query interceptor, so an injected UPDATE cannot re-enter it; a pattern that accepts quoted ids; a test that breaks the UPDATE and expects the exception to surface. Co-Authored-By: Claude Fable 5.1 --- .../action-scheduler-1369-mark-failures.patch | 28 +++++-- tests/test-action-scheduler-mark-failure.php | 74 +++++++++++++++++-- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/patches/action-scheduler-1369-mark-failures.patch b/patches/action-scheduler-1369-mark-failures.patch index b884d3579..08e250494 100644 --- a/patches/action-scheduler-1369-mark-failures.patch +++ b/patches/action-scheduler-1369-mark-failures.patch @@ -1,14 +1,25 @@ --- a/classes/ActionScheduler_QueueCleaner.php +++ b/classes/ActionScheduler_QueueCleaner.php -@@ -222,7 +222,12 @@ +@@ -204,6 +204,7 @@ + * @param int $time_limit The number of seconds to allow an action to run before it is considered to have failed. Default 300 (5 minutes). + */ + public function mark_failures( $time_limit = 300 ) { ++ global $wpdb; + $timeout = apply_filters( 'action_scheduler_failure_period', $time_limit ); + + if ( $timeout < 0 ) { +@@ -222,7 +223,15 @@ ); foreach ( $actions_to_reset as $action_id ) { - $this->store->mark_failure( $action_id ); + try { + $this->store->mark_failure( $action_id ); -+ } catch ( Exception $e ) { -+ // Deleted or changed by another process between the query and the update: nothing left to mark. ++ } catch ( InvalidArgumentException $e ) { ++ if ( ! empty( $wpdb->last_error ) ) { ++ throw $e; ++ } ++ // No database error: another process deleted or already failed this action. + continue; + } do_action( 'action_scheduler_failed_action', $action_id, $timeout ); @@ -16,15 +27,20 @@ } --- a/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php +++ b/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php -@@ -132,7 +132,11 @@ +@@ -131,8 +131,16 @@ + * @return void */ private function handle_action_error( $action_id, $e, $context, $valid_action ) { ++ global $wpdb; if ( $valid_action ) { - $this->store->mark_failure( $action_id ); + try { + $this->store->mark_failure( $action_id ); -+ } catch ( Exception $ignored ) { -+ // Already marked failed by the queue cleaner, or deleted by another process: nothing left to mark. ++ } catch ( InvalidArgumentException $mark_failure_error ) { ++ if ( ! empty( $wpdb->last_error ) ) { ++ throw $mark_failure_error; ++ } ++ // No database error: the cleaner already marked it failed, or another process deleted it. + } /** * Runs when action execution fails. diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index dce264e29..3b8a6877c 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -82,20 +82,55 @@ private function seed_stale_running_action() { * @param callable $other_process Runs once, right before that UPDATE. */ private function mark_failures_racing( $action_id, callable $other_process ) { - global $wpdb; - $table = $wpdb->actionscheduler_actions; - $filter = static function ( $sql ) use ( $action_id, $table, $other_process ) { - if ( 0 === stripos( ltrim( $sql ), 'UPDATE' ) && false !== strpos( $sql, $table ) && preg_match( '/action_id` = (\d+)/', $sql, $m ) && (int) $m[1] === $action_id ) { + $this->intercept_mark_failure_update( + $action_id, + static function ( $sql ) use ( $other_process ) { $other_process(); + return $sql; + } + ); + ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); + } + + /** + * Run `$intercept` once, on the UPDATE that marks `$action_id` failed, and + * use its return value as the SQL to execute. Removed after the test. + * + * @param int $action_id Action whose UPDATE is intercepted. + * @param callable $intercept Receives the SQL, returns the SQL to run. + */ + private function intercept_mark_failure_update( $action_id, callable $intercept ) { + global $wpdb; + $table = $wpdb->actionscheduler_actions; + $done = false; + // Queries issued inside $intercept re-enter this filter: run it once only. + $filter = static function ( $sql ) use ( $action_id, $table, $intercept, &$done ) { + if ( ! $done && 0 === stripos( ltrim( $sql ), 'UPDATE' ) && false !== strpos( $sql, $table ) && preg_match( '/action_id`?\s*=\s*\'?(\d+)/', $sql, $m ) && (int) $m[1] === $action_id ) { + $done = true; + return $intercept( $sql ); } return $sql; }; add_filter( 'query', $filter ); - try { - ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); - } finally { + $this->filters_to_remove[] = $filter; + } + + /** + * Query filters added by intercept_mark_failure_update(). + * + * @var callable[] + */ + private $filters_to_remove = array(); + + /** + * Remove the query filters even when a test throws. + */ + public function tear_down() { + foreach ( $this->filters_to_remove as $filter ) { remove_filter( 'query', $filter ); } + $this->filters_to_remove = array(); + parent::tear_down(); } /** @@ -170,4 +205,29 @@ static function () use ( $wpdb, $action_id ) { $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $action_id ) ); } + + /** + * The guard is for the race only. A real database error while marking the + * action failed must still surface, as it did before the patch. + */ + public function test_mark_failures_still_throws_on_a_database_error() { + global $wpdb; + $stale = $this->seed_stale_running_action(); + + // Break the UPDATE itself: the store gets `false`, not zero rows. + $this->intercept_mark_failure_update( + $stale, + static function ( $sql ) use ( $wpdb ) { + return str_replace( $wpdb->actionscheduler_actions, 'no_such_table', $sql ); + } + ); + $wpdb->suppress_errors( true ); + + $this->expectException( InvalidArgumentException::class ); + try { + ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); + } finally { + $wpdb->suppress_errors( false ); + } + } } From 69af78784d171b6d1a3b377b0669464370b7ba0f Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:01:45 +0300 Subject: [PATCH 04/10] build: refresh composer.lock content hash composer.json changed after the lock was written, so `composer validate` failed and every install warned that the lock file was stale. Co-Authored-By: Claude Fable 5.1 --- composer.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.lock b/composer.lock index a9269aa05..7b8ddc3b5 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "54649940158be9c9d52224064f0f3be3", + "content-hash": "28731c3ebcd9422db328f73b52e0883e", "packages": [ { "name": "codeinwp/themeisle-sdk", From ded4c25a4f1a31927916ef0220330bbe75d54352 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:10:56 +0300 Subject: [PATCH 05/10] test: restore the previous suppress_errors() setting Co-Authored-By: Claude Fable 5.1 --- tests/test-action-scheduler-mark-failure.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index 3b8a6877c..63e57ed6e 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -221,13 +221,13 @@ static function ( $sql ) use ( $wpdb ) { return str_replace( $wpdb->actionscheduler_actions, 'no_such_table', $sql ); } ); - $wpdb->suppress_errors( true ); + $suppressed = $wpdb->suppress_errors( true ); $this->expectException( InvalidArgumentException::class ); try { ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); } finally { - $wpdb->suppress_errors( false ); + $wpdb->suppress_errors( $suppressed ); } } } From 577ab24e272305c173ee3f848e0af354ab42d7b9 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:22:28 +0300 Subject: [PATCH 06/10] test: name the SQL predicate; note why last_error is trustworthy in the guard Co-Authored-By: Claude Fable 5.1 --- .../action-scheduler-1369-mark-failures.patch | 6 +++-- tests/test-action-scheduler-mark-failure.php | 26 +++++++++++++++---- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/patches/action-scheduler-1369-mark-failures.patch b/patches/action-scheduler-1369-mark-failures.patch index 08e250494..8ba57953e 100644 --- a/patches/action-scheduler-1369-mark-failures.patch +++ b/patches/action-scheduler-1369-mark-failures.patch @@ -8,7 +8,7 @@ $timeout = apply_filters( 'action_scheduler_failure_period', $time_limit ); if ( $timeout < 0 ) { -@@ -222,7 +223,15 @@ +@@ -222,7 +223,16 @@ ); foreach ( $actions_to_reset as $action_id ) { @@ -16,6 +16,7 @@ + try { + $this->store->mark_failure( $action_id ); + } catch ( InvalidArgumentException $e ) { ++ // last_error belongs to that UPDATE: wpdb::query() clears it before each statement. + if ( ! empty( $wpdb->last_error ) ) { + throw $e; + } @@ -27,7 +28,7 @@ } --- a/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php +++ b/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php -@@ -131,8 +131,16 @@ +@@ -131,8 +131,17 @@ * @return void */ private function handle_action_error( $action_id, $e, $context, $valid_action ) { @@ -37,6 +38,7 @@ + try { + $this->store->mark_failure( $action_id ); + } catch ( InvalidArgumentException $mark_failure_error ) { ++ // last_error belongs to that UPDATE: wpdb::query() clears it before each statement. + if ( ! empty( $wpdb->last_error ) ) { + throw $mark_failure_error; + } diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index 63e57ed6e..29be6bf02 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -104,17 +104,33 @@ private function intercept_mark_failure_update( $action_id, callable $intercept $table = $wpdb->actionscheduler_actions; $done = false; // Queries issued inside $intercept re-enter this filter: run it once only. - $filter = static function ( $sql ) use ( $action_id, $table, $intercept, &$done ) { - if ( ! $done && 0 === stripos( ltrim( $sql ), 'UPDATE' ) && false !== strpos( $sql, $table ) && preg_match( '/action_id`?\s*=\s*\'?(\d+)/', $sql, $m ) && (int) $m[1] === $action_id ) { - $done = true; - return $intercept( $sql ); + $filter = function ( $sql ) use ( $action_id, $table, $intercept, &$done ) { + if ( $done || ! $this->is_update_of_action( $sql, $table, $action_id ) ) { + return $sql; } - return $sql; + $done = true; + return $intercept( $sql ); }; add_filter( 'query', $filter ); $this->filters_to_remove[] = $filter; } + /** + * Whether `$sql` is an UPDATE of `$table` scoped to `$action_id`. + * Matches the SQL `wpdb::update()` builds with or without backticks and quotes. + * + * @param string $sql SQL about to run. + * @param string $table Actions table name. + * @param int $action_id Action id. + * @return bool + */ + private function is_update_of_action( $sql, $table, $action_id ) { + if ( 0 !== stripos( ltrim( $sql ), 'UPDATE' ) || false === strpos( $sql, $table ) ) { + return false; + } + return preg_match( '/action_id`?\s*=\s*\'?(\d+)/', $sql, $m ) && (int) $m[1] === $action_id; + } + /** * Query filters added by intercept_mark_failure_update(). * From 7b06c8ab658c7acb2ab0b62a7d90973fcddebdfc Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:48:16 +0300 Subject: [PATCH 07/10] test: seed through the store API; cover the runner path with a deleted action Co-Authored-By: Claude Fable 5.1 --- tests/test-action-scheduler-mark-failure.php | 52 +++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index 29be6bf02..47c097332 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -33,7 +33,7 @@ class Test_Visualizer_Action_Scheduler_Mark_Failure extends WP_UnitTestCase { public function set_up() { parent::set_up(); - if ( ! class_exists( 'ActionScheduler_QueueCleaner' ) || ! defined( 'VISUALIZER_ABSPATH' ) ) { + if ( ! class_exists( 'ActionScheduler' ) || ! class_exists( 'ActionScheduler_QueueCleaner' ) || ! defined( 'VISUALIZER_ABSPATH' ) ) { $this->markTestSkipped( 'Action Scheduler is not loaded.' ); } @@ -48,30 +48,25 @@ public function set_up() { } /** - * Insert a stale in-progress action (last attempt two hours ago). + * Save an action, then make it a stale in-progress one (last attempt two hours ago). * + * @param string $hook Action hook. * @return int Action id. */ - private function seed_stale_running_action() { + private function seed_stale_running_action( $hook = 'visualizer_schedule_refresh_db' ) { global $wpdb; - $gmt = gmdate( 'Y-m-d H:i:s', time() - 2 * HOUR_IN_SECONDS ); - $wpdb->insert( + $action_id = $this->store->save_action( new ActionScheduler_Action( $hook, array(), new ActionScheduler_SimpleSchedule( as_get_datetime_object( '-2 hours' ) ) ) ); + $gmt = gmdate( 'Y-m-d H:i:s', time() - 2 * HOUR_IN_SECONDS ); + $wpdb->update( $wpdb->actionscheduler_actions, array( - 'hook' => 'visualizer_schedule_refresh_db', - 'status' => ActionScheduler_Store::STATUS_RUNNING, - 'scheduled_date_gmt' => $gmt, - 'scheduled_date_local' => $gmt, - 'args' => '[]', - 'schedule' => '', - 'group_id' => 0, - 'attempts' => 1, - 'last_attempt_gmt' => $gmt, - 'last_attempt_local' => $gmt, - 'claim_id' => 0, - ) + 'status' => ActionScheduler_Store::STATUS_RUNNING, + 'last_attempt_gmt' => $gmt, + 'last_attempt_local' => $gmt, + ), + array( 'action_id' => $action_id ) ); - return (int) $wpdb->insert_id; + return (int) $action_id; } /** @@ -246,4 +241,25 @@ static function ( $sql ) use ( $wpdb ) { $wpdb->suppress_errors( $suppressed ); } } + + /** + * Runner path, deletion variant: the action is removed while it runs, then it throws. + */ + public function test_process_action_survives_marking_a_deleted_action() { + global $wpdb; + $hook = 'visualizer_test_throwing_action'; + $action_id = $this->store->save_action( new ActionScheduler_Action( $hook, array(), new ActionScheduler_SimpleSchedule( as_get_datetime_object( '-1 minute' ) ) ) ); + + add_action( + $hook, + static function () use ( $wpdb, $action_id ) { + $wpdb->delete( $wpdb->actionscheduler_actions, array( 'action_id' => $action_id ) ); + throw new RuntimeException( 'refresh failed' ); + } + ); + + ( new ActionScheduler_QueueRunner( $this->store ) )->process_action( $action_id, 'test' ); + + $this->assertNull( $this->status_of( $action_id ), 'the deleted action stays gone and the run survives' ); + } } From 2154cf6905438d7f937431977ac83e4752903b6a Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:01:20 +0300 Subject: [PATCH 08/10] fix: tolerate zero changed rows inside the store instead of at each caller `wpdb::update()` returns false on a database error and 0 when no row changed. `ActionScheduler_DBStore::mark_failure()` now throws only on false, so the race (deleted or already failed by another process) is handled once for every caller and no caller reads `$wpdb->last_error`. One hunk replaces the two caller guards. Co-Authored-By: Claude Fable 5.1 --- .../action-scheduler-1369-mark-failures.patch | 57 ++++--------------- tests/test-action-scheduler-mark-failure.php | 6 +- 2 files changed, 13 insertions(+), 50 deletions(-) diff --git a/patches/action-scheduler-1369-mark-failures.patch b/patches/action-scheduler-1369-mark-failures.patch index 8ba57953e..303764508 100644 --- a/patches/action-scheduler-1369-mark-failures.patch +++ b/patches/action-scheduler-1369-mark-failures.patch @@ -1,49 +1,12 @@ ---- a/classes/ActionScheduler_QueueCleaner.php -+++ b/classes/ActionScheduler_QueueCleaner.php -@@ -204,6 +204,7 @@ - * @param int $time_limit The number of seconds to allow an action to run before it is considered to have failed. Default 300 (5 minutes). - */ - public function mark_failures( $time_limit = 300 ) { -+ global $wpdb; - $timeout = apply_filters( 'action_scheduler_failure_period', $time_limit ); - - if ( $timeout < 0 ) { -@@ -222,7 +223,16 @@ +--- a/classes/data-stores/ActionScheduler_DBStore.php ++++ b/classes/data-stores/ActionScheduler_DBStore.php +@@ -1227,7 +1227,8 @@ + array( '%s' ), + array( '%d' ) ); - - foreach ( $actions_to_reset as $action_id ) { -- $this->store->mark_failure( $action_id ); -+ try { -+ $this->store->mark_failure( $action_id ); -+ } catch ( InvalidArgumentException $e ) { -+ // last_error belongs to that UPDATE: wpdb::query() clears it before each statement. -+ if ( ! empty( $wpdb->last_error ) ) { -+ throw $e; -+ } -+ // No database error: another process deleted or already failed this action. -+ continue; -+ } - do_action( 'action_scheduler_failed_action', $action_id, $timeout ); +- if ( empty( $updated ) ) { ++ // Zero rows means another process deleted the action or already marked it failed: nothing left to mark. ++ if ( false === $updated ) { + /* translators: %s is the action ID */ + throw new \InvalidArgumentException( sprintf( __( 'Unidentified action %s: we were unable to mark this action as having failed. It may may have been deleted by another process.', 'action-scheduler' ), $action_id ) ); } - } ---- a/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php -+++ b/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php -@@ -131,8 +131,17 @@ - * @return void - */ - private function handle_action_error( $action_id, $e, $context, $valid_action ) { -+ global $wpdb; - if ( $valid_action ) { -- $this->store->mark_failure( $action_id ); -+ try { -+ $this->store->mark_failure( $action_id ); -+ } catch ( InvalidArgumentException $mark_failure_error ) { -+ // last_error belongs to that UPDATE: wpdb::query() clears it before each statement. -+ if ( ! empty( $wpdb->last_error ) ) { -+ throw $mark_failure_error; -+ } -+ // No database error: the cleaner already marked it failed, or another process deleted it. -+ } - /** - * Runs when action execution fails. - * diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index 47c097332..43a3f8913 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -6,8 +6,8 @@ * "Unidentified action" whenever its UPDATE changes no row: the action was * deleted, or an overlapping cleaner already marked it failed (WP-Cron and the * async runner can overlap; only the async runner takes a lock). Unguarded, - * the whole queue run dies. The bundled copy is patched so both callers, the - * queue cleaner loop and the runner's error path, skip that action and go on. + * the whole queue run dies. The bundled copy is patched so the store treats + * zero changed rows as already handled and throws only on a database error. * * @package visualizer * @subpackage Tests @@ -15,7 +15,7 @@ */ /** - * Both mark_failure() callers in the bundled Action Scheduler. + * mark_failure() through both of its runtime callers in the bundled Action Scheduler. */ class Test_Visualizer_Action_Scheduler_Mark_Failure extends WP_UnitTestCase { From 4dd67635f80a3b72263c0514878a923a23e070a3 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:24:22 +0300 Subject: [PATCH 09/10] test: intercept only the UPDATE that sets status to failed Co-Authored-By: Claude Fable 5.1 --- tests/test-action-scheduler-mark-failure.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index 43a3f8913..e21c5493f 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -111,7 +111,7 @@ private function intercept_mark_failure_update( $action_id, callable $intercept } /** - * Whether `$sql` is an UPDATE of `$table` scoped to `$action_id`. + * Whether `$sql` is the UPDATE that marks `$action_id` in `$table` failed. * Matches the SQL `wpdb::update()` builds with or without backticks and quotes. * * @param string $sql SQL about to run. @@ -123,6 +123,9 @@ private function is_update_of_action( $sql, $table, $action_id ) { if ( 0 !== stripos( ltrim( $sql ), 'UPDATE' ) || false === strpos( $sql, $table ) ) { return false; } + if ( ! preg_match( '/status`?\s*=\s*\'' . ActionScheduler_Store::STATUS_FAILED . '\'/', $sql ) ) { + return false; + } return preg_match( '/action_id`?\s*=\s*\'?(\d+)/', $sql, $m ) && (int) $m[1] === $action_id; } From 60b3ff8328b1265410cc6fbd15fde5a2439b72dc Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:30:11 +0300 Subject: [PATCH 10/10] fix: replace the vendor patch with a store class of our own Patching the bundled Action Scheduler needed a Composer plugin, a patch file kept in the repo, and a re-roll on every dependency bump. Action Scheduler resolves its store through `action_scheduler_store_class`. Visualizer now answers that filter with a subclass of the database store that tolerates zero changed rows in `mark_failure()`, and still throws when the UPDATE itself failed. The library stays untouched, so a version bump needs no work, and the fix applies to whichever copy of Action Scheduler is loaded. Other stores, including another plugin's, are left alone. Refs: #1369 Co-Authored-By: Claude Opus 5 (1M context) --- .distignore | 2 - classes/Visualizer/ActionScheduler/Store.php | 46 +++++ composer.json | 14 +- composer.lock | 50 +---- index.php | 17 ++ .../action-scheduler-1369-mark-failures.patch | 12 -- phpstan.neon | 1 + tests/test-action-scheduler-mark-failure.php | 190 ++++++++---------- 8 files changed, 147 insertions(+), 185 deletions(-) create mode 100644 classes/Visualizer/ActionScheduler/Store.php delete mode 100644 patches/action-scheduler-1369-mark-failures.patch diff --git a/.distignore b/.distignore index 94f2659f6..30b35e1c3 100755 --- a/.distignore +++ b/.distignore @@ -35,5 +35,3 @@ skills classes/Visualizer/Gutenberg/src classes/Visualizer/ChartBuilder/src classes/Visualizer/D3Renderer/src -patches -vendor/cweagans diff --git a/classes/Visualizer/ActionScheduler/Store.php b/classes/Visualizer/ActionScheduler/Store.php new file mode 100644 index 000000000..851c898fa --- /dev/null +++ b/classes/Visualizer/ActionScheduler/Store.php @@ -0,0 +1,46 @@ +last_error ) ) { + throw $e; + } + } + } +} diff --git a/composer.json b/composer.json index bbdb4cb0b..1153b42df 100644 --- a/composer.json +++ b/composer.json @@ -24,8 +24,7 @@ "codeinwp/themeisle-sdk": "^3.3", "neitanod/forceutf8": "~2.0", "openspout/openspout": "^3.7", - "woocommerce/action-scheduler": "^3.8", - "cweagans/composer-patches": "^1.7" + "woocommerce/action-scheduler": "^3.8" }, "autoload": { "files": [ @@ -46,8 +45,7 @@ "php": "7.4" }, "allow-plugins": { - "dealerdirect/phpcodesniffer-composer-installer": true, - "cweagans/composer-patches": true + "dealerdirect/phpcodesniffer-composer-installer": true } }, "require-dev": { @@ -57,13 +55,5 @@ "phpstan/phpstan": "^2.1", "szepeviktor/phpstan-wordpress": "^2.0", "yoast/phpunit-polyfills": "^4.0" - }, - "extra": { - "patches": { - "woocommerce/action-scheduler": { - "Tolerate an action already failed or deleted by another process when marking it failed (#1369, upstream #970)": "patches/action-scheduler-1369-mark-failures.patch" - } - }, - "composer-exit-on-patch-failure": true } } diff --git a/composer.lock b/composer.lock index 7b8ddc3b5..79c20cde6 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "28731c3ebcd9422db328f73b52e0883e", + "content-hash": "881b5f99c0b72f47e79eb09bdac7fbea", "packages": [ { "name": "codeinwp/themeisle-sdk", @@ -47,54 +47,6 @@ }, "time": "2026-08-24T15:59:27+00:00" }, - { - "name": "cweagans/composer-patches", - "version": "1.7.3", - "source": { - "type": "git", - "url": "https://github.com/cweagans/composer-patches.git", - "reference": "e190d4466fe2b103a55467dfa83fc2fecfcaf2db" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/cweagans/composer-patches/zipball/e190d4466fe2b103a55467dfa83fc2fecfcaf2db", - "reference": "e190d4466fe2b103a55467dfa83fc2fecfcaf2db", - "shasum": "" - }, - "require": { - "composer-plugin-api": "^1.0 || ^2.0", - "php": ">=5.3.0" - }, - "require-dev": { - "composer/composer": "~1.0 || ~2.0", - "phpunit/phpunit": "~4.6" - }, - "type": "composer-plugin", - "extra": { - "class": "cweagans\\Composer\\Patches" - }, - "autoload": { - "psr-4": { - "cweagans\\Composer\\": "src" - } - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "BSD-3-Clause" - ], - "authors": [ - { - "name": "Cameron Eagans", - "email": "me@cweagans.net" - } - ], - "description": "Provides a way to patch Composer packages.", - "support": { - "issues": "https://github.com/cweagans/composer-patches/issues", - "source": "https://github.com/cweagans/composer-patches/tree/1.7.3" - }, - "time": "2022-12-20T22:53:13+00:00" - }, { "name": "neitanod/forceutf8", "version": "v2.0.4", diff --git a/index.php b/index.php index b25f24fbd..14b92c48f 100644 --- a/index.php +++ b/index.php @@ -156,6 +156,9 @@ function () { require_once $action_scheduler_file; } + // After Action Scheduler's own data controller, which sets the class at 100. + add_filter( 'action_scheduler_store_class', 'visualizer_action_scheduler_store_class', 200 ); + add_filter( 'themeisle_sdk_products', 'visualizer_register_sdk', 10, 1 ); add_filter( 'pirate_parrot_log', 'visualizer_register_parrot', 10, 1 ); add_filter( @@ -232,6 +235,20 @@ function visualizer_can_use_action_scheduler() { return isset( $wpdb ) && is_callable( array( $wpdb, 'db_server_info' ) ); } +/** + * Use a store that survives a lost race when marking an action failed. + * + * Only replaces Action Scheduler's own database store. Another plugin's store + * and the legacy post store, which does not have the problem, are left alone. + * + * @param string $class_name Store class Action Scheduler resolved. + * + * @return string + */ +function visualizer_action_scheduler_store_class( $class_name ) { + return 'ActionScheduler_DBStore' === $class_name ? 'Visualizer_ActionScheduler_Store' : $class_name; +} + /** * Registers with the SDK * diff --git a/patches/action-scheduler-1369-mark-failures.patch b/patches/action-scheduler-1369-mark-failures.patch deleted file mode 100644 index 303764508..000000000 --- a/patches/action-scheduler-1369-mark-failures.patch +++ /dev/null @@ -1,12 +0,0 @@ ---- a/classes/data-stores/ActionScheduler_DBStore.php -+++ b/classes/data-stores/ActionScheduler_DBStore.php -@@ -1227,7 +1227,8 @@ - array( '%s' ), - array( '%d' ) - ); -- if ( empty( $updated ) ) { -+ // Zero rows means another process deleted the action or already marked it failed: nothing left to mark. -+ if ( false === $updated ) { - /* translators: %s is the action ID */ - throw new \InvalidArgumentException( sprintf( __( 'Unidentified action %s: we were unable to mark this action as having failed. It may may have been deleted by another process.', 'action-scheduler' ), $action_id ) ); - } diff --git a/phpstan.neon b/phpstan.neon index 021d02592..477bdeff4 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -12,6 +12,7 @@ parameters: - %currentWorkingDirectory%/vendor/neitanod/forceutf8 - %currentWorkingDirectory%/vendor/openspout/openspout - %currentWorkingDirectory%/vendor/codeinwp/themeisle-sdk + - %currentWorkingDirectory%/vendor/woocommerce/action-scheduler excludePaths: - classes/Visualizer/Gutenberg/build (?) - classes/Visualizer/GutenChartBuilder/build (?) diff --git a/tests/test-action-scheduler-mark-failure.php b/tests/test-action-scheduler-mark-failure.php index e21c5493f..53d7da017 100644 --- a/tests/test-action-scheduler-mark-failure.php +++ b/tests/test-action-scheduler-mark-failure.php @@ -6,8 +6,8 @@ * "Unidentified action" whenever its UPDATE changes no row: the action was * deleted, or an overlapping cleaner already marked it failed (WP-Cron and the * async runner can overlap; only the async runner takes a lock). Unguarded, - * the whole queue run dies. The bundled copy is patched so the store treats - * zero changed rows as already handled and throws only on a database error. + * the whole queue run dies. Visualizer_ActionScheduler_Store tolerates that and + * still reports a database error. * * @package visualizer * @subpackage Tests @@ -15,38 +15,49 @@ */ /** - * mark_failure() through both of its runtime callers in the bundled Action Scheduler. + * The replacement store, and the two callers that mark actions failed. */ class Test_Visualizer_Action_Scheduler_Mark_Failure extends WP_UnitTestCase { /** - * The database store. The bug lives there; a fresh test site may still be - * on the legacy post store or the hybrid migration store. + * Store under test. * - * @var ActionScheduler_DBStore + * @var Visualizer_ActionScheduler_Store */ private $store; /** - * Skip when the bundled library is not the one loaded (another plugin's copy won). + * Query filters added during a test. + * + * @var callable[] + */ + private $filters_to_remove = array(); + + /** + * Skip when Action Scheduler is not loaded. */ public function set_up() { parent::set_up(); - if ( ! class_exists( 'ActionScheduler' ) || ! class_exists( 'ActionScheduler_QueueCleaner' ) || ! defined( 'VISUALIZER_ABSPATH' ) ) { + if ( ! class_exists( 'ActionScheduler_DBStore' ) ) { $this->markTestSkipped( 'Action Scheduler is not loaded.' ); } - $loaded = wp_normalize_path( ActionScheduler::plugin_path( '' ) ); - $ours = wp_normalize_path( VISUALIZER_ABSPATH . '/vendor/woocommerce/action-scheduler' ); - if ( 0 !== strpos( $loaded, $ours ) ) { - $this->markTestSkipped( 'Another Action Scheduler copy is loaded: ' . $loaded ); - } - - $this->store = new ActionScheduler_DBStore(); + $this->store = new Visualizer_ActionScheduler_Store(); $this->store->init(); } + /** + * Remove the query filters even when a test throws. + */ + public function tear_down() { + foreach ( $this->filters_to_remove as $filter ) { + remove_filter( 'query', $filter ); + } + $this->filters_to_remove = array(); + parent::tear_down(); + } + /** * Save an action, then make it a stale in-progress one (last attempt two hours ago). * @@ -70,26 +81,19 @@ private function seed_stale_running_action( $hook = 'visualizer_schedule_refresh } /** - * Run the cleaner's mark_failures() while "another process" acts on one - * action the moment the cleaner issues its UPDATE for it. + * Status column of one action, or null when the row is gone. * - * @param int $action_id Action the other process touches. - * @param callable $other_process Runs once, right before that UPDATE. + * @param int $action_id Action id. + * @return string|null */ - private function mark_failures_racing( $action_id, callable $other_process ) { - $this->intercept_mark_failure_update( - $action_id, - static function ( $sql ) use ( $other_process ) { - $other_process(); - return $sql; - } - ); - ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); + private function status_of( $action_id ) { + global $wpdb; + return $wpdb->get_var( $wpdb->prepare( "SELECT status FROM {$wpdb->actionscheduler_actions} WHERE action_id = %d", $action_id ) ); } /** * Run `$intercept` once, on the UPDATE that marks `$action_id` failed, and - * use its return value as the SQL to execute. Removed after the test. + * use its return value as the SQL to execute. * * @param int $action_id Action whose UPDATE is intercepted. * @param callable $intercept Receives the SQL, returns the SQL to run. @@ -100,7 +104,7 @@ private function intercept_mark_failure_update( $action_id, callable $intercept $done = false; // Queries issued inside $intercept re-enter this filter: run it once only. $filter = function ( $sql ) use ( $action_id, $table, $intercept, &$done ) { - if ( $done || ! $this->is_update_of_action( $sql, $table, $action_id ) ) { + if ( $done || ! $this->is_mark_failed_update( $sql, $table, $action_id ) ) { return $sql; } $done = true; @@ -119,7 +123,7 @@ private function intercept_mark_failure_update( $action_id, callable $intercept * @param int $action_id Action id. * @return bool */ - private function is_update_of_action( $sql, $table, $action_id ) { + private function is_mark_failed_update( $sql, $table, $action_id ) { if ( 0 !== stripos( ltrim( $sql ), 'UPDATE' ) || false === strpos( $sql, $table ) ) { return false; } @@ -130,107 +134,50 @@ private function is_update_of_action( $sql, $table, $action_id ) { } /** - * Query filters added by intercept_mark_failure_update(). - * - * @var callable[] - */ - private $filters_to_remove = array(); - - /** - * Remove the query filters even when a test throws. - */ - public function tear_down() { - foreach ( $this->filters_to_remove as $filter ) { - remove_filter( 'query', $filter ); - } - $this->filters_to_remove = array(); - parent::tear_down(); - } - - /** - * Status column of one action, or null when the row is gone. - * - * @param int $action_id Action id. - * @return string|null - */ - private function status_of( $action_id ) { - global $wpdb; - return $wpdb->get_var( $wpdb->prepare( "SELECT status FROM {$wpdb->actionscheduler_actions} WHERE action_id = %d", $action_id ) ); - } - - /** - * Cleanup keeps going when an action vanishes between its query and its update. + * Visualizer replaces Action Scheduler's own database store, and nothing else. */ - public function test_mark_failures_survives_an_action_deleted_by_another_process() { - global $wpdb; - $vanishing = $this->seed_stale_running_action(); - $survivor = $this->seed_stale_running_action(); - - $this->mark_failures_racing( - $vanishing, - static function () use ( $wpdb, $vanishing ) { - $wpdb->delete( $wpdb->actionscheduler_actions, array( 'action_id' => $vanishing ) ); - } - ); - - $this->assertNull( $this->status_of( $vanishing ), 'the concurrently deleted action stays gone' ); - $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $survivor ), 'cleanup continues and marks the remaining stale action failed' ); + public function test_filter_replaces_only_the_default_database_store() { + $this->assertSame( 'Visualizer_ActionScheduler_Store', visualizer_action_scheduler_store_class( 'ActionScheduler_DBStore' ) ); + $this->assertSame( 'Another_Plugin_Store', visualizer_action_scheduler_store_class( 'Another_Plugin_Store' ) ); + $this->assertSame( 'ActionScheduler_HybridStore', visualizer_action_scheduler_store_class( 'ActionScheduler_HybridStore' ) ); } /** - * An overlapping cleaner marked it first. The UPDATE then changes nothing, - * MySQL reports zero rows, and the store throws as if the row were gone. + * Another process deleted the action: nothing left to mark. */ - public function test_mark_failures_survives_an_action_already_failed_by_an_overlapping_cleaner() { + public function test_mark_failure_tolerates_a_deleted_action() { global $wpdb; - $raced = $this->seed_stale_running_action(); - $survivor = $this->seed_stale_running_action(); + $action_id = $this->seed_stale_running_action(); + $wpdb->delete( $wpdb->actionscheduler_actions, array( 'action_id' => $action_id ) ); - $this->mark_failures_racing( - $raced, - static function () use ( $wpdb, $raced ) { - $wpdb->update( $wpdb->actionscheduler_actions, array( 'status' => ActionScheduler_Store::STATUS_FAILED ), array( 'action_id' => $raced ) ); - } - ); + $this->store->mark_failure( $action_id ); - $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $raced ) ); - $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $survivor ), 'cleanup continues past the action the other cleaner already handled' ); + $this->assertNull( $this->status_of( $action_id ) ); } /** - * Same hole on the processing path (the trace in upstream #970): a long - * action gets marked failed by the cleaner while it runs, then throws; - * marking it failed again changes no row. + * An overlapping cleaner already marked it failed: the UPDATE changes nothing. */ - public function test_process_action_survives_marking_an_already_failed_action() { + public function test_mark_failure_tolerates_an_already_failed_action() { global $wpdb; - $hook = 'visualizer_test_throwing_action'; - $action_id = $this->store->save_action( new ActionScheduler_Action( $hook, array(), new ActionScheduler_SimpleSchedule( as_get_datetime_object( '-1 minute' ) ) ) ); + $action_id = $this->seed_stale_running_action(); + $wpdb->update( $wpdb->actionscheduler_actions, array( 'status' => ActionScheduler_Store::STATUS_FAILED ), array( 'action_id' => $action_id ) ); - add_action( - $hook, - static function () use ( $wpdb, $action_id ) { - $wpdb->update( $wpdb->actionscheduler_actions, array( 'status' => ActionScheduler_Store::STATUS_FAILED ), array( 'action_id' => $action_id ) ); - throw new RuntimeException( 'refresh failed' ); - } - ); - - ( new ActionScheduler_QueueRunner( $this->store ) )->process_action( $action_id, 'test' ); + $this->store->mark_failure( $action_id ); $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $action_id ) ); } /** - * The guard is for the race only. A real database error while marking the - * action failed must still surface, as it did before the patch. + * A real database error still surfaces. */ - public function test_mark_failures_still_throws_on_a_database_error() { + public function test_mark_failure_still_throws_on_a_database_error() { global $wpdb; - $stale = $this->seed_stale_running_action(); + $action_id = $this->seed_stale_running_action(); // Break the UPDATE itself: the store gets `false`, not zero rows. $this->intercept_mark_failure_update( - $stale, + $action_id, static function ( $sql ) use ( $wpdb ) { return str_replace( $wpdb->actionscheduler_actions, 'no_such_table', $sql ); } @@ -239,14 +186,37 @@ static function ( $sql ) use ( $wpdb ) { $this->expectException( InvalidArgumentException::class ); try { - ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); + $this->store->mark_failure( $action_id ); } finally { $wpdb->suppress_errors( $suppressed ); } } /** - * Runner path, deletion variant: the action is removed while it runs, then it throws. + * Queue cleanup keeps going when an action vanishes between its query and its update. + */ + public function test_mark_failures_continues_past_an_action_deleted_by_another_process() { + global $wpdb; + $vanishing = $this->seed_stale_running_action(); + $survivor = $this->seed_stale_running_action(); + + $this->intercept_mark_failure_update( + $vanishing, + static function ( $sql ) use ( $wpdb, $vanishing ) { + $wpdb->delete( $wpdb->actionscheduler_actions, array( 'action_id' => $vanishing ) ); + return $sql; + } + ); + + ( new ActionScheduler_QueueCleaner( $this->store ) )->mark_failures( 60 ); + + $this->assertNull( $this->status_of( $vanishing ), 'the concurrently deleted action stays gone' ); + $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $this->status_of( $survivor ), 'cleanup continues and marks the remaining stale action failed' ); + } + + /** + * Runner path (the trace in upstream #970): the action is deleted while it + * runs, then it throws, and the runner marks it failed. */ public function test_process_action_survives_marking_a_deleted_action() { global $wpdb;