From 594871c5898d852ed7fdedb2044fa308300e91a7 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:45:37 +0300 Subject: [PATCH 01/17] fix: keep the DB refresh scheduled when Action Scheduler creation fails as_schedule_recurring_action() returns 0 instead of throwing when it cannot store an action. The activation path ignored that return, cleared the WP-Cron fallback anyway, and left the refresh hook with no trigger at all. Nothing recovered it: the recovery hook stood down whenever Action Scheduler was usable, and the migration was gated on the WP-Cron event that had just been deleted, so even a reactivation could fail the same way. Clear the fallback only after re-querying Action Scheduler for the action. Turn the recovery hook into a plain "is anything going to fire this hook" check, which also covers the legacy WP-Cron migration, so the duplicate scheduling code in Visualizer_Module_Upgrade is no longer needed. Two consequences of running that check on every request: - Re-arm WP-Cron only when the event is missing or its interval changed. Re-arming unconditionally pinned it to a past timestamp on every request, so the refresh became due on every cron spawn. - Schedule the action as unique. Action Scheduler creates the next recurrence only after the current one completes, so a concurrent request can arrive while nothing is pending and add a duplicate. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 45 ++++--- classes/Visualizer/Module/Upgrade.php | 42 ------- tests/test-schedule-refresh-db.php | 166 ++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 56 deletions(-) create mode 100644 tests/test-schedule-refresh-db.php diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 6ce006595..553abc64c 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -498,37 +498,54 @@ private function schedule_refresh_db_action(): void { ) { $next = as_next_scheduled_action( $hook, array(), $group ); if ( false === $next ) { - as_schedule_recurring_action( $timestamp, $interval, $hook, array(), $group ); + // Unique: this runs on every request, and Action Scheduler only creates the next + // recurrence once the current one completes, so a concurrent request can arrive + // while nothing is pending. Uniqueness is enforced in the insert itself. + as_schedule_recurring_action( $timestamp, $interval, $hook, array(), $group, true ); + } + + // as_schedule_recurring_action() returns 0 and stores nothing when creation fails, + // so drop the WP-Cron fallback only once the action is really there to replace it. + if ( false !== as_next_scheduled_action( $hook, array(), $group ) ) { + wp_clear_scheduled_hook( $hook ); + return; } - wp_clear_scheduled_hook( $hook ); - return; } - wp_clear_scheduled_hook( $hook ); - wp_schedule_event( $timestamp, $interval_key, $hook ); + // Re-arm only what is missing or stale: this runs on every request while Action + // Scheduler keeps refusing, and resetting a live event would keep the refresh due. + $event = wp_get_scheduled_event( $hook ); + if ( ! $event || $event->schedule !== $interval_key ) { + wp_clear_scheduled_hook( $hook ); + wp_schedule_event( $timestamp, $interval_key, $hook ); + } } /** - * Keep the DB refresh scheduled when Action Scheduler is not available. + * Keep the DB refresh scheduled on whichever scheduler the site can use. * - * The migration to Action Scheduler clears the WP-Cron event, so a site that - * already migrated and then lost the library would have nothing left running - * the refresh. Re-arms WP-Cron in that case; no-op whenever the library is up. + * This is the only way back: once the refresh hook has no trigger, nothing but a + * reactivation used to restore it, and reactivation can fail the same way. Also covers + * a site that lost Action Scheduler, and a legacy WP-Cron event that never migrated. + * + * ponytail: two indexed queries per request; cache in a transient if a profile ever complains. */ public function maybe_reschedule_refresh_db(): void { + $hook = 'visualizer_schedule_refresh_db'; + if ( visualizer_can_use_action_scheduler() && function_exists( 'as_next_scheduled_action' ) && function_exists( 'as_schedule_recurring_action' ) ) { - return; + $scheduled = false !== as_next_scheduled_action( $hook, array(), 'visualizer' ); + } else { + $scheduled = (bool) wp_next_scheduled( $hook ); } - if ( wp_next_scheduled( 'visualizer_schedule_refresh_db' ) ) { - return; + if ( ! $scheduled ) { + $this->schedule_refresh_db_action(); } - - $this->schedule_refresh_db_action(); } /** diff --git a/classes/Visualizer/Module/Upgrade.php b/classes/Visualizer/Module/Upgrade.php index 1989d63b0..1dcf3b9e3 100644 --- a/classes/Visualizer/Module/Upgrade.php +++ b/classes/Visualizer/Module/Upgrade.php @@ -24,11 +24,6 @@ public static function upgrade() { $upgraded = true; } - if ( wp_next_scheduled( 'visualizer_schedule_refresh_db' ) ) { - self::migrate_action_scheduler(); - $upgraded = true; - } - if ( ! $upgraded ) { return; } @@ -79,41 +74,4 @@ private static function makeAllTableChartsTabular() { ); // phpcs:enable WordPress.DB.PreparedSQL.NotPrepared } - - /** - * Migrate recurring WP-Cron jobs to Action Scheduler. - */ - private static function migrate_action_scheduler(): void { - if ( ! function_exists( 'as_schedule_recurring_action' ) || ! function_exists( 'as_next_scheduled_action' ) ) { - return; - } - - $hook = 'visualizer_schedule_refresh_db'; - $group = 'visualizer'; - $interval_key = apply_filters( 'visualizer_chart_schedule_interval', 'visualizer_ten_minutes' ); - $interval = self::get_schedule_interval_seconds( $interval_key ); - $timestamp = strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS; - - $next = as_next_scheduled_action( $hook, array(), $group ); - if ( false === $next ) { - as_schedule_recurring_action( $timestamp, $interval, $hook, array(), $group ); - } - - wp_clear_scheduled_hook( $hook ); - } - - /** - * Resolve a cron schedule key to seconds. - * - * @param string $interval_key Cron schedule key. - * @return int Interval in seconds. - */ - private static function get_schedule_interval_seconds( $interval_key ) { - $schedules = wp_get_schedules(); - if ( isset( $schedules[ $interval_key ]['interval'] ) ) { - return (int) $schedules[ $interval_key ]['interval']; - } - - return 600; - } } diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php new file mode 100644 index 000000000..b0173f580 --- /dev/null +++ b/tests/test-schedule-refresh-db.php @@ -0,0 +1,166 @@ +assertTrue( function_exists( 'as_schedule_recurring_action' ), 'Action Scheduler must be loaded' ); + $this->assertTrue( ActionScheduler::is_initialized(), 'Action Scheduler must be initialized' ); + } + + /** + * A refused Action Scheduler creation must leave the WP-Cron fallback in place. + */ + public function test_activation_keeps_a_trigger_when_action_scheduler_creation_fails() { + add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + $this->lifecycle( 'activate' ); + remove_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + + $this->assertTrue( $this->has_trigger(), 'activation must not leave the refresh hook with no trigger at all' ); + } + + /** + * A site that already lost both schedulers must recover on an ordinary request. + */ + public function test_recovery_restores_a_trigger_when_both_schedulers_are_empty() { + $this->assertFalse( $this->has_trigger(), 'precondition: nothing is scheduled' ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + + $this->assertTrue( $this->has_trigger(), 'a missing refresh trigger must be restored without another activation' ); + } + + /** + * A legacy WP-Cron event must move onto Action Scheduler and stop firing twice. + */ + public function test_legacy_wp_cron_event_migrates_to_action_scheduler() { + wp_schedule_event( time(), 'visualizer_ten_minutes', self::HOOK ); + $this->assertNotFalse( wp_next_scheduled( self::HOOK ), 'precondition: a legacy WP-Cron event exists' ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + + $this->assertNotFalse( as_next_scheduled_action( self::HOOK, array(), self::GROUP ), 'the refresh must move onto Action Scheduler' ); + $this->assertFalse( wp_next_scheduled( self::HOOK ), 'the superseded WP-Cron event must not survive the migration' ); + } + + /** + * While Action Scheduler keeps refusing, later requests must leave the fallback alone. + * + * Re-arming it on every request pins the event to a past timestamp, so the refresh runs + * on every cron spawn instead of every ten minutes. + */ + public function test_recovery_does_not_drag_a_live_wp_cron_event_back_into_the_past() { + add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + $this->assertNotFalse( wp_next_scheduled( self::HOOK ), 'precondition: the fallback is armed' ); + + // mimic WP-Cron having run the event and rescheduled it forward. + wp_clear_scheduled_hook( self::HOOK ); + $future = time() + 600; + wp_schedule_event( $future, 'visualizer_ten_minutes', self::HOOK ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + remove_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + + $this->assertSame( $future, wp_next_scheduled( self::HOOK ), 'a later request must not make the refresh due again' ); + } + + /** + * A concurrent request must not be able to create a second recurring action. + * + * Action Scheduler creates the next recurrence only after the current one completes, so + * every interval there is a moment with nothing pending. A visitor arriving in that + * window used to add a duplicate, and duplicates never go away on their own. + */ + public function test_recovery_does_not_create_a_second_action_when_a_concurrent_request_wins_the_race() { + $done = false; + // Stand in for another request that schedules between our lookup and our write. + $racer = function ( $pre, $timestamp, $interval, $hook, $args, $group, $priority, $unique ) use ( &$done ) { + if ( ! $done ) { + $done = true; + as_schedule_recurring_action( $timestamp, $interval, $hook, $args, $group, $unique ); + } + return null; + }; + add_filter( 'pre_as_schedule_recurring_action', $racer, 10, 9 ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + remove_filter( 'pre_as_schedule_recurring_action', $racer, 10 ); + + $this->assertTrue( $done, 'precondition: the race was actually simulated' ); + $this->assertCount( 1, $this->pending_actions(), 'a lost race must not leave the refresh scheduled twice' ); + } + + /** + * Every pending refresh action. + * + * @return array + */ + private function pending_actions(): array { + return as_get_scheduled_actions( + array( + 'hook' => self::HOOK, + 'group' => self::GROUP, + 'status' => ActionScheduler_Store::STATUS_PENDING, + ), + 'ids' + ); + } + + /** + * The registered setup module. + * + * @return Visualizer_Module_Setup + */ + private function setup_module(): Visualizer_Module_Setup { + return Visualizer_Plugin::instance()->getModule( Visualizer_Module_Setup::NAME ); + } +} From 38786b19cc9555fd135e10f73ea0cf89e2263cef Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:03:15 +0300 Subject: [PATCH 02/17] fix: clear a WP-Cron event left beside the Action Scheduler action Both schedulers fire visualizer_schedule_refresh_db, so a site holding an action and an event refreshes twice per interval. The recovery check counted the action alone as settled and left the event in place. Treat the refresh as settled only when Action Scheduler holds the action and no event fires the same hook beside it. Also from review: reuse the first lookup instead of querying Action Scheduler again when the action already exists, and name the hook and the group once instead of repeating the literals. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 35 ++++++++++++++++++++--------- tests/test-schedule-refresh-db.php | 20 ++++++++++++++++- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 553abc64c..50059b62e 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -31,6 +31,16 @@ class Visualizer_Module_Setup extends Visualizer_Module { const NAME = __CLASS__; + /** + * Hook that refreshes database charts. + */ + const REFRESH_DB_HOOK = 'visualizer_schedule_refresh_db'; + + /** + * Action Scheduler group that owns the refresh. + */ + const REFRESH_DB_GROUP = 'visualizer'; + /** * Constructor. * @@ -44,7 +54,7 @@ public function __construct( Visualizer_Plugin $plugin ) { register_activation_hook( VISUALIZER_BASEFILE, array( $this, 'activate' ) ); register_deactivation_hook( VISUALIZER_BASEFILE, array( $this, 'deactivate' ) ); - $this->_addAction( 'visualizer_schedule_refresh_db', 'refreshDbChart' ); + $this->_addAction( self::REFRESH_DB_HOOK, 'refreshDbChart' ); $this->_addAction( 'init', 'maybe_reschedule_refresh_db' ); $this->_addFilter( 'visualizer_schedule_refresh_chart', 'refresh_db_for_chart', 10, 3 ); @@ -485,8 +495,8 @@ public function custom_cron_schedules( $schedules ) { * Schedule the recurring DB refresh action. */ private function schedule_refresh_db_action(): void { - $hook = 'visualizer_schedule_refresh_db'; - $group = 'visualizer'; + $hook = self::REFRESH_DB_HOOK; + $group = self::REFRESH_DB_GROUP; $interval_key = apply_filters( 'visualizer_chart_schedule_interval', 'visualizer_ten_minutes' ); $interval = $this->get_schedule_interval_seconds( $interval_key ); $timestamp = strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS; @@ -502,11 +512,13 @@ private function schedule_refresh_db_action(): void { // recurrence once the current one completes, so a concurrent request can arrive // while nothing is pending. Uniqueness is enforced in the insert itself. as_schedule_recurring_action( $timestamp, $interval, $hook, array(), $group, true ); + + // The call returns 0 and stores nothing when creation fails, so ask the store. + $next = as_next_scheduled_action( $hook, array(), $group ); } - // as_schedule_recurring_action() returns 0 and stores nothing when creation fails, - // so drop the WP-Cron fallback only once the action is really there to replace it. - if ( false !== as_next_scheduled_action( $hook, array(), $group ) ) { + // Drop the WP-Cron fallback only once the action is there to replace it. + if ( false !== $next ) { wp_clear_scheduled_hook( $hook ); return; } @@ -531,14 +543,17 @@ private function schedule_refresh_db_action(): void { * ponytail: two indexed queries per request; cache in a transient if a profile ever complains. */ public function maybe_reschedule_refresh_db(): void { - $hook = 'visualizer_schedule_refresh_db'; + $hook = self::REFRESH_DB_HOOK; if ( visualizer_can_use_action_scheduler() && function_exists( 'as_next_scheduled_action' ) && function_exists( 'as_schedule_recurring_action' ) ) { - $scheduled = false !== as_next_scheduled_action( $hook, array(), 'visualizer' ); + // Settled only once Action Scheduler holds the action and no WP-Cron event fires + // the same hook beside it; a site keeping both refreshes twice per interval. + $scheduled = false !== as_next_scheduled_action( $hook, array(), self::REFRESH_DB_GROUP ) + && ! wp_next_scheduled( $hook ); } else { $scheduled = (bool) wp_next_scheduled( $hook ); } @@ -552,8 +567,8 @@ public function maybe_reschedule_refresh_db(): void { * Unschedule the recurring DB refresh action. */ private function unschedule_refresh_db_action(): void { - $hook = 'visualizer_schedule_refresh_db'; - $group = 'visualizer'; + $hook = self::REFRESH_DB_HOOK; + $group = self::REFRESH_DB_GROUP; if ( function_exists( 'as_unschedule_all_actions' ) ) { as_unschedule_all_actions( $hook, array(), $group ); } diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index b0173f580..dab2ebb2a 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -130,7 +130,7 @@ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_ } return null; }; - add_filter( 'pre_as_schedule_recurring_action', $racer, 10, 9 ); + add_filter( 'pre_as_schedule_recurring_action', $racer, 10, 8 ); $this->setup_module()->maybe_reschedule_refresh_db(); remove_filter( 'pre_as_schedule_recurring_action', $racer, 10 ); @@ -139,6 +139,24 @@ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_ $this->assertCount( 1, $this->pending_actions(), 'a lost race must not leave the refresh scheduled twice' ); } + /** + * A WP-Cron event left beside an Action Scheduler action must go. + * + * Both schedulers fire the same hook, so a site that keeps both refreshes twice per + * interval. A lost race between the fallback and a concurrent request can leave that pair. + */ + public function test_recovery_removes_a_wp_cron_event_left_beside_an_action_scheduler_action() { + as_schedule_recurring_action( time(), 600, self::HOOK, array(), self::GROUP, true ); + wp_schedule_event( time(), 'visualizer_ten_minutes', self::HOOK ); + $this->assertNotFalse( as_next_scheduled_action( self::HOOK, array(), self::GROUP ), 'precondition: Action Scheduler owns the refresh' ); + $this->assertNotFalse( wp_next_scheduled( self::HOOK ), 'precondition: a WP-Cron event sits beside it' ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + + $this->assertFalse( wp_next_scheduled( self::HOOK ), 'the refresh must not stay scheduled on both systems' ); + $this->assertNotFalse( as_next_scheduled_action( self::HOOK, array(), self::GROUP ), 'the Action Scheduler action must survive' ); + } + /** * Every pending refresh action. * From 22ccbba4ef8ae568a9d3266ed30b06b69f2cc027 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:14:49 +0300 Subject: [PATCH 03/17] test: skip instead of fatal when Action Scheduler is not loaded index.php loads Action Scheduler only when visualizer_can_use_action_scheduler() passes, so a host without wpdb::db_server_info() has none. set_up() called the library unconditionally, which ended the whole suite with a fatal before any test could report. Skip the file instead. Also corrects the WP-Cron comment: the check is missing or different interval, not a stale timestamp. A timestamp criterion would pin the event to the past again, which is what the check exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 5 +++-- tests/test-schedule-refresh-db.php | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 50059b62e..e317cb7d8 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -524,8 +524,9 @@ private function schedule_refresh_db_action(): void { } } - // Re-arm only what is missing or stale: this runs on every request while Action - // Scheduler keeps refusing, and resetting a live event would keep the refresh due. + // Re-arm only when the event is missing or set to a different interval. This runs on + // every request while Action Scheduler keeps refusing, and re-arming a live event + // would pin it to a past timestamp and make the refresh due on every cron spawn. $event = wp_get_scheduled_event( $hook ); if ( ! $event || $event->schedule !== $interval_key ) { wp_clear_scheduled_hook( $hook ); diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index dab2ebb2a..78cb70c9d 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -24,6 +24,13 @@ class Test_Visualizer_Schedule_Refresh_Db extends WP_UnitTestCase { */ public function set_up() { parent::set_up(); + + // index.php loads Action Scheduler only when visualizer_can_use_action_scheduler() + // passes, so skip rather than fatal on a host that cannot run it. + if ( ! function_exists( 'as_unschedule_all_actions' ) ) { + $this->markTestSkipped( 'Action Scheduler is not loaded on this environment.' ); + } + as_unschedule_all_actions( self::HOOK, array(), self::GROUP ); wp_clear_scheduled_hook( self::HOOK ); } From afa7686eea1d9b6d80b99822d230ada59e4d9dd4 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:34:41 +0300 Subject: [PATCH 04/17] fix: recover the refresh chain a killed run ends, and throttle the check Hands-on investigation on the reporting site corrected the premise of #1384. Reactivation did restore the action; the customer was reading WP Crontrol, where the hook correctly no longer appears after the Action Scheduler migration. The real defect is that a run killed mid flight ends the recurring chain for good: Action Scheduler creates the next occurrence inside schedule_next_instance(), which a host kill, fatal or timeout never reaches, and the queue cleaner then marks the action failed with no successor. The per-request check already recovers this. Add the two pieces it was missing: - Hook action_scheduler_ensure_recurring_actions, Action Scheduler's own daily assurance hook, as a floor under the per-request check for sites that serve few requests. - Throttle the per-request check to one run per 300s, recorded in an autoloaded option so a settled site spends no query on it. Action Scheduler needs the same 300s to mark a killed run failed, so a shorter window cannot recover one any sooner. The daily hook ignores the window. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 40 ++++++++++++++--- tests/test-schedule-refresh-db.php | 69 ++++++++++++++++++++++++++++- uninstall.php | 1 + 3 files changed, 102 insertions(+), 8 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index e317cb7d8..27bab4976 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -41,6 +41,17 @@ class Visualizer_Module_Setup extends Visualizer_Module { */ const REFRESH_DB_GROUP = 'visualizer'; + /** + * When the refresh trigger was last checked. + */ + const REFRESH_DB_CHECK_OPTION = 'visualizer-refresh-db-checked'; + + /** + * How long a check stays good for. Action Scheduler needs the same 300s to mark a + * killed run failed, so a shorter window cannot recover one any sooner. + */ + const REFRESH_DB_CHECK_WINDOW = 300; + /** * Constructor. * @@ -56,6 +67,7 @@ public function __construct( Visualizer_Plugin $plugin ) { register_deactivation_hook( VISUALIZER_BASEFILE, array( $this, 'deactivate' ) ); $this->_addAction( self::REFRESH_DB_HOOK, 'refreshDbChart' ); $this->_addAction( 'init', 'maybe_reschedule_refresh_db' ); + $this->_addAction( 'action_scheduler_ensure_recurring_actions', 'ensure_refresh_db_action' ); $this->_addFilter( 'visualizer_schedule_refresh_chart', 'refresh_db_for_chart', 10, 3 ); $this->_addAction( 'admin_init', 'adminInit' ); @@ -535,15 +547,31 @@ private function schedule_refresh_db_action(): void { } /** - * Keep the DB refresh scheduled on whichever scheduler the site can use. + * Check once per window that something still fires the refresh. * - * This is the only way back: once the refresh hook has no trigger, nothing but a - * reactivation used to restore it, and reactivation can fail the same way. Also covers - * a site that lost Action Scheduler, and a legacy WP-Cron event that never migrated. - * - * ponytail: two indexed queries per request; cache in a transient if a profile ever complains. + * Hooked to `init`, so it runs on every request. The timestamp is autoloaded and costs + * no query, and the daily `action_scheduler_ensure_recurring_actions` hook is the floor + * under it on a site that serves few requests. */ public function maybe_reschedule_refresh_db(): void { + $checked = (int) get_option( self::REFRESH_DB_CHECK_OPTION, 0 ); + if ( time() - $checked < self::REFRESH_DB_CHECK_WINDOW ) { + return; + } + + update_option( self::REFRESH_DB_CHECK_OPTION, time(), true ); + $this->ensure_refresh_db_action(); + } + + /** + * Keep the DB refresh scheduled on whichever scheduler the site can use. + * + * This is the only way back. Action Scheduler creates the next occurrence of a recurring + * action inside schedule_next_instance(), which a killed run never reaches, so the chain + * ends there and nothing but a reactivation used to restore it. Also covers a site that + * lost Action Scheduler, and a legacy WP-Cron event that never migrated. + */ + public function ensure_refresh_db_action(): void { $hook = self::REFRESH_DB_HOOK; if ( diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 78cb70c9d..f598896ef 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -33,6 +33,9 @@ public function set_up() { as_unschedule_all_actions( self::HOOK, array(), self::GROUP ); wp_clear_scheduled_hook( self::HOOK ); + + // the bootstrap activates the plugin, so `init` has already opened a check window. + delete_option( Visualizer_Module_Setup::REFRESH_DB_CHECK_OPTION ); } /** @@ -106,7 +109,7 @@ public function test_legacy_wp_cron_event_migrates_to_action_scheduler() { public function test_recovery_does_not_drag_a_live_wp_cron_event_back_into_the_past() { add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); - $this->setup_module()->maybe_reschedule_refresh_db(); + $this->setup_module()->ensure_refresh_db_action(); $this->assertNotFalse( wp_next_scheduled( self::HOOK ), 'precondition: the fallback is armed' ); // mimic WP-Cron having run the event and rescheduled it forward. @@ -114,7 +117,7 @@ public function test_recovery_does_not_drag_a_live_wp_cron_event_back_into_the_p $future = time() + 600; wp_schedule_event( $future, 'visualizer_ten_minutes', self::HOOK ); - $this->setup_module()->maybe_reschedule_refresh_db(); + $this->setup_module()->ensure_refresh_db_action(); remove_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); $this->assertSame( $future, wp_next_scheduled( self::HOOK ), 'a later request must not make the refresh due again' ); @@ -164,6 +167,68 @@ public function test_recovery_removes_a_wp_cron_event_left_beside_an_action_sche $this->assertNotFalse( as_next_scheduled_action( self::HOOK, array(), self::GROUP ), 'the Action Scheduler action must survive' ); } + /** + * A run killed mid flight must not end the recurring chain. + * + * Action Scheduler creates the next occurrence inside schedule_next_instance(), which a + * host kill, fatal or timeout never reaches. The queue cleaner then marks the action + * failed, and nothing succeeds it. This is the scenario confirmed on the reporting site. + */ + public function test_a_killed_run_does_not_end_the_recurring_chain() { + as_schedule_recurring_action( time(), 600, self::HOOK, array(), self::GROUP, true ); + $pending = $this->pending_actions(); + $action_id = reset( $pending ); + + // what ActionScheduler_QueueCleaner::mark_failures() does to a run that never returned. + $store = ActionScheduler::store(); + $store->log_execution( $action_id ); + $store->mark_failure( $action_id ); + + $this->assertSame( ActionScheduler_Store::STATUS_FAILED, $store->get_status( $action_id ), 'precondition: the run was killed' ); + $this->assertFalse( $this->has_trigger(), 'precondition: nothing succeeds the killed run' ); + + $this->setup_module()->maybe_reschedule_refresh_db(); + + $this->assertCount( 1, $this->pending_actions(), 'a killed run must get a successor' ); + } + + /** + * Action Scheduler's own daily assurance hook must restore a missing action. + * + * This is the floor under the per-request check, and it runs even on a site that serves + * no admin requests for a while. + */ + public function test_the_daily_action_scheduler_hook_restores_a_missing_action() { + $this->assertFalse( $this->has_trigger(), 'precondition: nothing is scheduled' ); + + do_action( 'action_scheduler_ensure_recurring_actions' ); + + $this->assertTrue( $this->has_trigger(), 'the daily assurance hook must restore the refresh' ); + } + + /** + * The per-request check stands down inside its window, and the daily hook does not. + * + * The check runs on `init`, so it must not query Action Scheduler on every request of a + * settled site. The daily assurance hook ignores the window and is the floor. + */ + public function test_the_per_request_check_is_throttled_and_the_daily_hook_is_the_floor() { + $module = $this->setup_module(); + + $module->maybe_reschedule_refresh_db(); + $this->assertTrue( $this->has_trigger(), 'precondition: the first request scheduled the refresh' ); + + // the chain dies again, inside the window the first request opened. + as_unschedule_all_actions( self::HOOK, array(), self::GROUP ); + $module->maybe_reschedule_refresh_db(); + + $this->assertFalse( $this->has_trigger(), 'inside the window the per-request check must stand down' ); + + do_action( 'action_scheduler_ensure_recurring_actions' ); + + $this->assertTrue( $this->has_trigger(), 'the daily assurance hook must repair it whatever the window says' ); + } + /** * Every pending refresh action. * diff --git a/uninstall.php b/uninstall.php index 5da4a28f5..b6ab35d5a 100644 --- a/uninstall.php +++ b/uninstall.php @@ -27,3 +27,4 @@ // clean up after ourselves, that's a good plugin! delete_option( 'visualizer_fresh_install' ); delete_option( 'visualizer_wizard_data' ); +delete_option( 'visualizer-refresh-db-checked' ); From c8cf5121d7df41bcb7443a1d1c029ac9bd64b4d6 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:47:52 +0300 Subject: [PATCH 05/17] fix: start a recovered run now, and stop rewriting an autoloaded option Two findings from review. The start time is local midnight derived from gmt_offset. West of UTC that midnight has not arrived yet, so a recovered run was parked up to twelve hours ahead and the charts stayed stale for the rest of the day. Fall back to the previous midnight when the computed one is still in the future. The throttle recorded its timestamp in an autoloaded option, so every window rewrote the alloptions blob and invalidated it for every process. A transient with the window as its expiry is not autoloaded, expires on its own, and keeps the read cached. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 22 ++++++++++++++-------- tests/test-schedule-refresh-db.php | 21 ++++++++++++++++++++- uninstall.php | 2 +- 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 27bab4976..90e3107e3 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -42,9 +42,9 @@ class Visualizer_Module_Setup extends Visualizer_Module { const REFRESH_DB_GROUP = 'visualizer'; /** - * When the refresh trigger was last checked. + * Marks the refresh trigger as checked recently. */ - const REFRESH_DB_CHECK_OPTION = 'visualizer-refresh-db-checked'; + const REFRESH_DB_CHECK_TRANSIENT = 'visualizer-refresh-db-checked'; /** * How long a check stays good for. Action Scheduler needs the same 300s to mark a @@ -513,6 +513,12 @@ private function schedule_refresh_db_action(): void { $interval = $this->get_schedule_interval_seconds( $interval_key ); $timestamp = strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS; + // West of UTC that midnight has not arrived yet. Start from the one before it, so a + // recovered run is due immediately instead of later in the day. + if ( $timestamp > time() ) { + $timestamp -= DAY_IN_SECONDS; + } + if ( visualizer_can_use_action_scheduler() && function_exists( 'as_next_scheduled_action' ) @@ -549,17 +555,17 @@ private function schedule_refresh_db_action(): void { /** * Check once per window that something still fires the refresh. * - * Hooked to `init`, so it runs on every request. The timestamp is autoloaded and costs - * no query, and the daily `action_scheduler_ensure_recurring_actions` hook is the floor - * under it on a site that serves few requests. + * Hooked to `init`, so it runs on every request. A transient keeps that to one cached + * read instead of two Action Scheduler queries, and it expires on its own rather than + * rewriting the autoloaded options blob every window. The daily + * `action_scheduler_ensure_recurring_actions` hook is the floor under it. */ public function maybe_reschedule_refresh_db(): void { - $checked = (int) get_option( self::REFRESH_DB_CHECK_OPTION, 0 ); - if ( time() - $checked < self::REFRESH_DB_CHECK_WINDOW ) { + if ( get_transient( self::REFRESH_DB_CHECK_TRANSIENT ) ) { return; } - update_option( self::REFRESH_DB_CHECK_OPTION, time(), true ); + set_transient( self::REFRESH_DB_CHECK_TRANSIENT, 1, self::REFRESH_DB_CHECK_WINDOW ); $this->ensure_refresh_db_action(); } diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index f598896ef..656819b4f 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -35,7 +35,7 @@ public function set_up() { wp_clear_scheduled_hook( self::HOOK ); // the bootstrap activates the plugin, so `init` has already opened a check window. - delete_option( Visualizer_Module_Setup::REFRESH_DB_CHECK_OPTION ); + delete_transient( Visualizer_Module_Setup::REFRESH_DB_CHECK_TRANSIENT ); } /** @@ -229,6 +229,25 @@ public function test_the_per_request_check_is_throttled_and_the_daily_hook_is_th $this->assertTrue( $this->has_trigger(), 'the daily assurance hook must repair it whatever the window says' ); } + /** + * Recovery must make the refresh due now, not at a midnight that has not happened yet. + * + * The start time is local midnight derived from `gmt_offset`. West of UTC that midnight + * can still be ahead of us, which would park the recovered run hours into the future and + * leave the charts stale for the rest of the day. + */ + public function test_recovery_does_not_park_the_next_run_in_the_future() { + // Far enough west that the computed midnight is ahead of us whatever the time of + // day. WordPress does not clamp gmt_offset, so this stays deterministic. + $hours_into_utc_day = ( time() - strtotime( 'midnight' ) ) / HOUR_IN_SECONDS; + update_option( 'gmt_offset', - ( $hours_into_utc_day + 1 ) ); + + $this->setup_module()->ensure_refresh_db_action(); + + $next = as_next_scheduled_action( self::HOOK, array(), self::GROUP ); + $this->assertLessThanOrEqual( time(), $next, 'the recovered refresh must be due now, not hours from now' ); + } + /** * Every pending refresh action. * diff --git a/uninstall.php b/uninstall.php index b6ab35d5a..f9c8b7e29 100644 --- a/uninstall.php +++ b/uninstall.php @@ -27,4 +27,4 @@ // clean up after ourselves, that's a good plugin! delete_option( 'visualizer_fresh_install' ); delete_option( 'visualizer_wizard_data' ); -delete_option( 'visualizer-refresh-db-checked' ); +delete_transient( 'visualizer-refresh-db-checked' ); From 3ea7339e7df1970ec0a34571b9d3929527495717 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:59:44 +0300 Subject: [PATCH 06/17] test: prove the filter argument order instead of assuming it Review has now questioned the pre_as_schedule_recurring_action argument order twice. The racer records the two arguments it receives and the test asserts their types, so a swap fails loudly instead of silently forwarding the wrong value. Types separate them whatever value the code under test passes. Also from review: guard set_up() on the ActionScheduler classes the tests call statics on, not only the functions, and scope the gmt_offset change to a filter the framework restores. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test-schedule-refresh-db.php | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 656819b4f..e64cedb70 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -27,7 +27,11 @@ public function set_up() { // index.php loads Action Scheduler only when visualizer_can_use_action_scheduler() // passes, so skip rather than fatal on a host that cannot run it. - if ( ! function_exists( 'as_unschedule_all_actions' ) ) { + if ( + ! class_exists( 'ActionScheduler' ) + || ! class_exists( 'ActionScheduler_Store' ) + || ! function_exists( 'as_unschedule_all_actions' ) + ) { $this->markTestSkipped( 'Action Scheduler is not loaded on this environment.' ); } @@ -132,11 +136,17 @@ public function test_recovery_does_not_drag_a_live_wp_cron_event_back_into_the_p */ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_request_wins_the_race() { $done = false; + $seen = array(); // Stand in for another request that schedules between our lookup and our write. - $racer = function ( $pre, $timestamp, $interval, $hook, $args, $group, $priority, $unique ) use ( &$done ) { + // Action Scheduler passes $priority before $unique, see its functions.php:165. + $racer = function ( $pre, $timestamp, $interval, $hook, $args, $group, $priority, $unique ) use ( &$done, &$seen ) { if ( ! $done ) { $done = true; - as_schedule_recurring_action( $timestamp, $interval, $hook, $args, $group, $unique ); + $seen = array( + 'priority' => $priority, + 'unique' => $unique, + ); + as_schedule_recurring_action( $timestamp, $interval, $hook, $args, $group, $unique, $priority ); } return null; }; @@ -145,6 +155,11 @@ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_ $this->setup_module()->maybe_reschedule_refresh_db(); remove_filter( 'pre_as_schedule_recurring_action', $racer, 10 ); + // The simulation only reproduces the race if the racer read the real arguments, so + // pin the order here rather than trusting the signature. Types alone separate the + // two, whatever value the code under test passes for $unique. + $this->assertIsInt( $seen['priority'], 'the filter must pass $priority before $unique' ); + $this->assertIsBool( $seen['unique'], 'the filter must pass $priority before $unique' ); $this->assertTrue( $done, 'precondition: the race was actually simulated' ); $this->assertCount( 1, $this->pending_actions(), 'a lost race must not leave the refresh scheduled twice' ); } @@ -240,7 +255,12 @@ public function test_recovery_does_not_park_the_next_run_in_the_future() { // Far enough west that the computed midnight is ahead of us whatever the time of // day. WordPress does not clamp gmt_offset, so this stays deterministic. $hours_into_utc_day = ( time() - strtotime( 'midnight' ) ) / HOUR_IN_SECONDS; - update_option( 'gmt_offset', - ( $hours_into_utc_day + 1 ) ); + add_filter( + 'pre_option_gmt_offset', + function () use ( $hours_into_utc_day ) { + return - ( $hours_into_utc_day + 1 ); + } + ); $this->setup_module()->ensure_refresh_db_action(); From d69c3388dfe6d36839b65d231c0111fd27fff11b Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:17:19 +0300 Subject: [PATCH 07/17] test: do not depend on plugin_basename() for the lifecycle hook name plugin_basename() resolves differently depending on where the plugin directory is loaded from, so the hook name register_activation_hook() used is not stable across environments. Loading the plugin from outside WP_PLUGIN_DIR registers the full path as the hook name while the test computes the short one, and the activation test then fires a hook nothing listens to and passes vacuously or fails. Call the callback the hook points at instead. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test-schedule-refresh-db.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index e64cedb70..4da899968 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -53,12 +53,16 @@ private function has_trigger(): bool { } /** - * Fire a plugin lifecycle hook the way WordPress does. + * Run what WordPress runs for a plugin lifecycle event. + * + * Calls the callback rather than firing the hook: plugin_basename() resolves differently + * depending on where the plugin directory is loaded from, so the hook name is not stable + * across environments. * * @param string $action Either `activate` or `deactivate`. */ private function lifecycle( string $action ) { - do_action( $action . '_' . plugin_basename( VISUALIZER_BASEFILE ), false ); + $this->setup_module()->$action( false ); } /** From 29adb15198874a5c769502f4e51cb45e2e95a632 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:29:23 +0300 Subject: [PATCH 08/17] test: say why the Action Scheduler precondition fails instead of skipping A red build on this assertion needs to point somewhere. Absent is skipped in set_up() because some hosts cannot run Action Scheduler; loaded but not initialized means the load order broke, and every other test in the class would then assert nothing. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test-schedule-refresh-db.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 4da899968..af7857d5e 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -67,10 +67,19 @@ private function lifecycle( string $action ) { /** * Precondition: Action Scheduler is usable, otherwise the rest proves nothing. + * + * This one fails rather than skips, on purpose. set_up() skips the class when Action + * Scheduler is absent, which is an environment this plugin supports. Present but not + * initialized is not one: the library initializes on `init` at priority 1, so reaching a + * test without it means the load order broke, and every as_* call in this class would + * quietly return false and assert nothing. */ public function test_action_scheduler_is_available() { $this->assertTrue( function_exists( 'as_schedule_recurring_action' ), 'Action Scheduler must be loaded' ); - $this->assertTrue( ActionScheduler::is_initialized(), 'Action Scheduler must be initialized' ); + $this->assertTrue( + ActionScheduler::is_initialized(), + 'Action Scheduler is loaded but its data store is not initialized, so every other test in this class would assert nothing. Check that it is loaded before init.' + ); } /** From fb1f58d81c1fa995167c13395139f01c8a7b5e58 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:43:03 +0300 Subject: [PATCH 09/17] fix: do not drop the refresh on an unknown interval or a fractional offset Two findings from review, both on the WP-Cron fallback. wp_schedule_event() refuses a schedule WP-Cron does not know, and the fallback cleared the existing event before asking, so a visualizer_chart_schedule_interval filter returning an unregistered key left the refresh with nothing and the throttle then held the retry off. Fall back to the plugin's own schedule when the filtered key is not registered, which is what get_schedule_interval_seconds() already does for the interval itself. gmt_offset is a number rather than an integer, so the start time could carry a fraction. WP-Cron keys its array by that value, and PHP reports losing precision from wp-includes/cron.php. Cast the start time to an integer. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 13 ++++++- tests/test-schedule-refresh-db.php | 58 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 90e3107e3..45b4f0f49 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -510,8 +510,17 @@ private function schedule_refresh_db_action(): void { $hook = self::REFRESH_DB_HOOK; $group = self::REFRESH_DB_GROUP; $interval_key = apply_filters( 'visualizer_chart_schedule_interval', 'visualizer_ten_minutes' ); - $interval = $this->get_schedule_interval_seconds( $interval_key ); - $timestamp = strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS; + + // wp_schedule_event() refuses a schedule WP-Cron does not know, and the fallback below + // clears the old event before it asks, so an unknown key would drop the refresh. + if ( ! isset( wp_get_schedules()[ $interval_key ] ) ) { + $interval_key = 'visualizer_ten_minutes'; + } + + $interval = $this->get_schedule_interval_seconds( $interval_key ); + // gmt_offset is a number, not an integer, so the product can carry a fraction that + // WP-Cron would then lose when it keys its array by this value. + $timestamp = (int) ( strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ); // West of UTC that midnight has not arrived yet. Start from the one before it, so a // recovered run is due immediately instead of later in the day. diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index af7857d5e..bfddff7a3 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -281,6 +281,64 @@ function () use ( $hours_into_utc_day ) { $this->assertLessThanOrEqual( time(), $next, 'the recovered refresh must be due now, not hours from now' ); } + /** + * A filtered interval key WP-Cron does not know must not drop the trigger. + * + * wp_schedule_event() returns false for an unregistered schedule, and the fallback used + * to clear the old event first, so the refresh was left with nothing and the throttle + * then held the retry off. get_schedule_interval_seconds() already copes with this. + */ + public function test_an_unknown_interval_key_still_leaves_a_trigger() { + add_filter( + 'visualizer_chart_schedule_interval', + static function () { + return 'not_a_registered_schedule'; + } + ); + // force the WP-Cron fallback, which is the path that takes $interval_key. + add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + + $this->setup_module()->ensure_refresh_db_action(); + + $this->assertTrue( $this->has_trigger(), 'an unknown interval key must not leave the refresh with no trigger' ); + } + + /** + * The start time must be a whole second, whatever gmt_offset holds. + * + * gmt_offset is a number, not an integer, so multiplying it makes the start time a float. + * WP-Cron keys its array by that value and PHP then reports losing precision. + */ + public function test_a_fractional_offset_does_not_schedule_a_fractional_timestamp() { + add_filter( + 'pre_option_gmt_offset', + static function () { + return 5.0001; + } + ); + // force the WP-Cron fallback, which is where the value becomes an array key. + add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + + $lost = array(); + set_error_handler( + static function ( $errno, $errstr ) use ( &$lost ) { + if ( false !== strpos( $errstr, 'loses precision' ) ) { + $lost[] = $errstr; + } + return true; + }, + E_DEPRECATED + ); + + try { + $this->setup_module()->ensure_refresh_db_action(); + } finally { + restore_error_handler(); + } + + $this->assertSame( array(), $lost, 'the start time must be a whole second' ); + } + /** * Every pending refresh action. * From 091ffcfb008478feb554085b8bcaf909a2d9544f Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:45:51 +0300 Subject: [PATCH 10/17] docs: note that tear_down restores hooks, so test filters need no removal Reviewers have read the missing remove_filter() calls as a leak four times across this PR and its pro counterpart. WP_UnitTestCase_Base backs up $wp_filter in set_up() and restores it wholesale in tear_down(). Co-Authored-By: Claude Opus 5 (1M context) --- tests/test-schedule-refresh-db.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index bfddff7a3..51bb4003a 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -13,6 +13,11 @@ * Action Scheduler returns 0 instead of throwing when it cannot create an action, so the * plugin used to drop the WP-Cron fallback for an action that was never stored, leaving * nothing scheduled and no way back. + * + * A filter added inside a test needs no removal. WP_UnitTestCase_Base::set_up() backs up + * $wp_filter and tear_down() restores it wholesale, so a filter cannot reach the next test. + * The tests here that do call remove_filter() call it mid test, because they still assert + * afterwards and need the filter gone first. */ class Test_Visualizer_Schedule_Refresh_Db extends WP_UnitTestCase { From 8b32e018db7d3f9bf6894601d83d501f4a4c762f Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Mon, 21 Sep 2026 16:58:42 +0300 Subject: [PATCH 11/17] fix: record the check window only when a trigger exists The window skips work that is already done, so recording it after an attempt that established no trigger left the site without one until the window expired, with the daily assurance hook a day away. ensure_refresh_db_action() now reports whether the refresh ended up scheduled, and the caller caches only that. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 37 ++++++++++++++++++++--------- tests/test-schedule-refresh-db.php | 24 +++++++++++++++++++ 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 45b4f0f49..078799965 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -574,8 +574,11 @@ public function maybe_reschedule_refresh_db(): void { return; } - set_transient( self::REFRESH_DB_CHECK_TRANSIENT, 1, self::REFRESH_DB_CHECK_WINDOW ); - $this->ensure_refresh_db_action(); + // Only skip the check while there is something to skip it for. An attempt that + // established no trigger is retried on the next request, not after the window. + if ( $this->ensure_refresh_db_action() ) { + set_transient( self::REFRESH_DB_CHECK_TRANSIENT, 1, self::REFRESH_DB_CHECK_WINDOW ); + } } /** @@ -586,7 +589,25 @@ public function maybe_reschedule_refresh_db(): void { * ends there and nothing but a reactivation used to restore it. Also covers a site that * lost Action Scheduler, and a legacy WP-Cron event that never migrated. */ - public function ensure_refresh_db_action(): void { + public function ensure_refresh_db_action(): bool { + if ( $this->refresh_db_is_scheduled() ) { + return true; + } + + $this->schedule_refresh_db_action(); + + return $this->refresh_db_is_scheduled(); + } + + /** + * Whether something will fire the refresh hook again. + * + * Settled only once Action Scheduler holds the action and no WP-Cron event fires the same + * hook beside it; a site keeping both refreshes twice per interval. + * + * @return bool + */ + private function refresh_db_is_scheduled(): bool { $hook = self::REFRESH_DB_HOOK; if ( @@ -594,17 +615,11 @@ public function ensure_refresh_db_action(): void { && function_exists( 'as_next_scheduled_action' ) && function_exists( 'as_schedule_recurring_action' ) ) { - // Settled only once Action Scheduler holds the action and no WP-Cron event fires - // the same hook beside it; a site keeping both refreshes twice per interval. - $scheduled = false !== as_next_scheduled_action( $hook, array(), self::REFRESH_DB_GROUP ) + return false !== as_next_scheduled_action( $hook, array(), self::REFRESH_DB_GROUP ) && ! wp_next_scheduled( $hook ); - } else { - $scheduled = (bool) wp_next_scheduled( $hook ); } - if ( ! $scheduled ) { - $this->schedule_refresh_db_action(); - } + return (bool) wp_next_scheduled( $hook ); } /** diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 51bb4003a..96456bfbc 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -344,6 +344,30 @@ static function ( $errno, $errstr ) use ( &$lost ) { $this->assertSame( array(), $lost, 'the start time must be a whole second' ); } + /** + * A check that scheduled nothing must be retried, not cached. + * + * The window exists to skip work that is already done. Recording it after an attempt that + * established no trigger leaves the site without one until the window expires. + */ + public function test_a_failed_check_is_retried_on_the_next_request() { + $module = $this->setup_module(); + + // refuse both schedulers. + add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + add_filter( 'schedule_event', '__return_false' ); + + $module->maybe_reschedule_refresh_db(); + $this->assertFalse( $this->has_trigger(), 'precondition: nothing could be scheduled' ); + + remove_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + remove_filter( 'schedule_event', '__return_false' ); + + $module->maybe_reschedule_refresh_db(); + + $this->assertTrue( $this->has_trigger(), 'a check that scheduled nothing must be retried on the next request' ); + } + /** * Every pending refresh action. * From 842a76c0ee292e9d0a9d1959863599eb6682c5ef Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Tue, 22 Sep 2026 09:42:05 +0300 Subject: [PATCH 12/17] fix: treat a live WP-Cron fallback as scheduled for the check window Where Action Scheduler is present but refuses, the fallback is the trigger, and the previous commit reported that as unscheduled. The window was then never recorded, so every request repeated the whole check and the refused insert. Split the two questions: settled means the refresh sits on the scheduler this site should use, which is what decides whether to act; having a trigger means something will fire the hook at all, which is what decides whether to cache. ensure_refresh_db_action() goes back to returning nothing. Co-Authored-By: Claude Opus 5 (1M context) --- classes/Visualizer/Module/Setup.php | 41 +++++++++++++++++++++-------- tests/test-schedule-refresh-db.php | 26 ++++++++++++++++++ 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 078799965..4e9c84f50 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -574,9 +574,11 @@ public function maybe_reschedule_refresh_db(): void { return; } + $this->ensure_refresh_db_action(); + // Only skip the check while there is something to skip it for. An attempt that // established no trigger is retried on the next request, not after the window. - if ( $this->ensure_refresh_db_action() ) { + if ( $this->has_refresh_db_trigger() ) { set_transient( self::REFRESH_DB_CHECK_TRANSIENT, 1, self::REFRESH_DB_CHECK_WINDOW ); } } @@ -589,25 +591,22 @@ public function maybe_reschedule_refresh_db(): void { * ends there and nothing but a reactivation used to restore it. Also covers a site that * lost Action Scheduler, and a legacy WP-Cron event that never migrated. */ - public function ensure_refresh_db_action(): bool { - if ( $this->refresh_db_is_scheduled() ) { - return true; + public function ensure_refresh_db_action(): void { + if ( ! $this->refresh_db_is_settled() ) { + $this->schedule_refresh_db_action(); } - - $this->schedule_refresh_db_action(); - - return $this->refresh_db_is_scheduled(); } /** - * Whether something will fire the refresh hook again. + * Whether the refresh sits on the scheduler this site should be using. * * Settled only once Action Scheduler holds the action and no WP-Cron event fires the same - * hook beside it; a site keeping both refreshes twice per interval. + * hook beside it; a site keeping both refreshes twice per interval. Being unsettled is a + * reason to act, not a sign that nothing runs. * * @return bool */ - private function refresh_db_is_scheduled(): bool { + private function refresh_db_is_settled(): bool { $hook = self::REFRESH_DB_HOOK; if ( @@ -622,6 +621,26 @@ private function refresh_db_is_scheduled(): bool { return (bool) wp_next_scheduled( $hook ); } + /** + * Whether anything at all will fire the refresh hook again. + * + * Where Action Scheduler is present but refuses, the WP-Cron fallback is the trigger, and + * a site with one is not in trouble even though it is not settled. + * + * @return bool + */ + private function has_refresh_db_trigger(): bool { + $hook = self::REFRESH_DB_HOOK; + + if ( visualizer_can_use_action_scheduler() && function_exists( 'as_next_scheduled_action' ) ) { + if ( false !== as_next_scheduled_action( $hook, array(), self::REFRESH_DB_GROUP ) ) { + return true; + } + } + + return (bool) wp_next_scheduled( $hook ); + } + /** * Unschedule the recurring DB refresh action. */ diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 96456bfbc..b4971c78f 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -368,6 +368,32 @@ public function test_a_failed_check_is_retried_on_the_next_request() { $this->assertTrue( $this->has_trigger(), 'a check that scheduled nothing must be retried on the next request' ); } + /** + * A live WP-Cron fallback counts as scheduled, so the window still applies. + * + * Where Action Scheduler is present but refuses, the fallback is the trigger. Reporting + * that as unscheduled makes every request retry the whole check and the refused insert. + */ + public function test_a_wp_cron_fallback_is_not_re_attempted_on_every_request() { + $attempts = 0; + $refuse = function () use ( &$attempts ) { + ++$attempts; + return 0; + }; + add_filter( 'pre_as_schedule_recurring_action', $refuse ); + + $module = $this->setup_module(); + $module->maybe_reschedule_refresh_db(); + + $this->assertNotFalse( wp_next_scheduled( self::HOOK ), 'precondition: the fallback is armed' ); + $after_first = $attempts; + + $module->maybe_reschedule_refresh_db(); + remove_filter( 'pre_as_schedule_recurring_action', $refuse ); + + $this->assertSame( $after_first, $attempts, 'a live fallback must not be re-attempted on the next request' ); + } + /** * Every pending refresh action. * From f81420797cbdb08e54184d6b42e7e5290c5b6977 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:03:03 +0300 Subject: [PATCH 13/17] refactor: read the cron schedules once when scheduling the refresh The key check and the interval lookup each called wp_get_schedules(). Read it once and derive both from the same array, which also makes the one-line helper that did the second lookup unnecessary. Co-Authored-By: Claude Fable 5.1 --- classes/Visualizer/Module/Setup.php | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 4e9c84f50..9145112d3 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -509,15 +509,17 @@ public function custom_cron_schedules( $schedules ) { private function schedule_refresh_db_action(): void { $hook = self::REFRESH_DB_HOOK; $group = self::REFRESH_DB_GROUP; + $schedules = wp_get_schedules(); $interval_key = apply_filters( 'visualizer_chart_schedule_interval', 'visualizer_ten_minutes' ); // wp_schedule_event() refuses a schedule WP-Cron does not know, and the fallback below // clears the old event before it asks, so an unknown key would drop the refresh. - if ( ! isset( wp_get_schedules()[ $interval_key ] ) ) { + if ( ! isset( $schedules[ $interval_key ]['interval'] ) ) { $interval_key = 'visualizer_ten_minutes'; } - $interval = $this->get_schedule_interval_seconds( $interval_key ); + // The plugin registers that schedule itself; the literal only covers a filter removing it. + $interval = isset( $schedules[ $interval_key ]['interval'] ) ? (int) $schedules[ $interval_key ]['interval'] : 600; // gmt_offset is a number, not an integer, so the product can carry a fraction that // WP-Cron would then lose when it keys its array by this value. $timestamp = (int) ( strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ); @@ -652,19 +654,4 @@ private function unschedule_refresh_db_action(): void { } wp_clear_scheduled_hook( $hook ); } - - /** - * Resolve a cron schedule key to seconds. - * - * @param string $interval_key Cron schedule key. - * @return int Interval in seconds. - */ - private function get_schedule_interval_seconds( $interval_key ) { - $schedules = wp_get_schedules(); - if ( isset( $schedules[ $interval_key ]['interval'] ) ) { - return (int) $schedules[ $interval_key ]['interval']; - } - - return 600; - } } From dbd3176777ca15cab592d6ca021a434a54717ba6 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:16:55 +0300 Subject: [PATCH 14/17] fix: keep the old WP-Cron event until its replacement is scheduled Changing interval cleared the old event and then scheduled the new one, so a refused schedule left the refresh with nothing. That is the bug this PR fixes on the Action Scheduler path, repeated on the WP-Cron one. Schedule first and remove the old event by its own timestamp afterwards. wp_clear_scheduled_hook() would remove the new event too, since WP-Cron does not dedupe recurring events for a hook. A matching timestamp means the write already replaced the old entry in place, so nothing is removed in that case. Co-Authored-By: Claude Fable 5.1 --- classes/Visualizer/Module/Setup.php | 17 ++++++++++++++--- tests/test-schedule-refresh-db.php | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 9145112d3..2a191e895 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -557,9 +557,20 @@ private function schedule_refresh_db_action(): void { // every request while Action Scheduler keeps refusing, and re-arming a live event // would pin it to a past timestamp and make the refresh due on every cron spawn. $event = wp_get_scheduled_event( $hook ); - if ( ! $event || $event->schedule !== $interval_key ) { - wp_clear_scheduled_hook( $hook ); - wp_schedule_event( $timestamp, $interval_key, $hook ); + if ( $event && $event->schedule === $interval_key ) { + return; + } + + // Schedule the replacement before touching the old event, so a refused schedule leaves + // the old one running. wp_clear_scheduled_hook() would take the new one with it, so + // remove the old event by its own timestamp. The same timestamp means the write above + // already replaced it in place. + if ( false === wp_schedule_event( $timestamp, $interval_key, $hook ) ) { + return; + } + + if ( $event && $event->timestamp !== $timestamp ) { + wp_unschedule_event( $event->timestamp, $hook, $event->args ); } } diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index b4971c78f..8c8a90b76 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -394,6 +394,25 @@ public function test_a_wp_cron_fallback_is_not_re_attempted_on_every_request() { $this->assertSame( $after_first, $attempts, 'a live fallback must not be re-attempted on the next request' ); } + /** + * A refused replacement must not take the old WP-Cron event with it. + * + * Changing interval used to clear the old event and then schedule the new one, so a + * refused schedule left nothing. Same principle as the Action Scheduler path: nothing is + * removed until what replaces it exists. + */ + public function test_a_refused_reschedule_keeps_the_old_wp_cron_event() { + // an event on another interval, with nothing to migrate it to. + wp_schedule_event( time() + HOUR_IN_SECONDS, 'hourly', self::HOOK ); + add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); + // WP-Cron refuses the replacement. + add_filter( 'schedule_event', '__return_false' ); + + $this->setup_module()->ensure_refresh_db_action(); + + $this->assertNotFalse( wp_next_scheduled( self::HOOK ), 'the old event must survive a refused replacement' ); + } + /** * Every pending refresh action. * From ede25f055800ef68b5add5381efccf0c24acc313 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:18:28 +0300 Subject: [PATCH 15/17] fix: unschedule the old refresh event without passing its arguments The refresh is scheduled without arguments, so the old event has none to match. Keeps this in step with the Pro plugin, where PHPStan objected to passing the typed-as-array field where a list is expected. Co-Authored-By: Claude Fable 5.1 --- classes/Visualizer/Module/Setup.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 2a191e895..335fc3fc4 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -569,8 +569,9 @@ private function schedule_refresh_db_action(): void { return; } + // The refresh is scheduled without arguments, so the old event has none to match. if ( $event && $event->timestamp !== $timestamp ) { - wp_unschedule_event( $event->timestamp, $hook, $event->args ); + wp_unschedule_event( $event->timestamp, $hook ); } } From da6fa8febd7b3df0fa4d2a0a382ea2d821b581cb Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Tue, 22 Sep 2026 14:23:34 +0300 Subject: [PATCH 16/17] docs: trim the comments Requested in review: keep the one line that says why, drop the paragraphs. Co-Authored-By: Claude Fable 5.1 --- classes/Visualizer/Module/Setup.php | 61 +++++------------- tests/test-schedule-refresh-db.php | 98 ++++++----------------------- 2 files changed, 35 insertions(+), 124 deletions(-) diff --git a/classes/Visualizer/Module/Setup.php b/classes/Visualizer/Module/Setup.php index 335fc3fc4..00a3fbcae 100644 --- a/classes/Visualizer/Module/Setup.php +++ b/classes/Visualizer/Module/Setup.php @@ -47,8 +47,7 @@ class Visualizer_Module_Setup extends Visualizer_Module { const REFRESH_DB_CHECK_TRANSIENT = 'visualizer-refresh-db-checked'; /** - * How long a check stays good for. Action Scheduler needs the same 300s to mark a - * killed run failed, so a shorter window cannot recover one any sooner. + * Seconds a check stays valid; matches Action Scheduler's timeout for a killed run. */ const REFRESH_DB_CHECK_WINDOW = 300; @@ -512,20 +511,16 @@ private function schedule_refresh_db_action(): void { $schedules = wp_get_schedules(); $interval_key = apply_filters( 'visualizer_chart_schedule_interval', 'visualizer_ten_minutes' ); - // wp_schedule_event() refuses a schedule WP-Cron does not know, and the fallback below - // clears the old event before it asks, so an unknown key would drop the refresh. + // wp_schedule_event() refuses an unregistered schedule. if ( ! isset( $schedules[ $interval_key ]['interval'] ) ) { $interval_key = 'visualizer_ten_minutes'; } - // The plugin registers that schedule itself; the literal only covers a filter removing it. $interval = isset( $schedules[ $interval_key ]['interval'] ) ? (int) $schedules[ $interval_key ]['interval'] : 600; - // gmt_offset is a number, not an integer, so the product can carry a fraction that - // WP-Cron would then lose when it keys its array by this value. + // gmt_offset can be fractional, and WP-Cron keys its array by this value. $timestamp = (int) ( strtotime( 'midnight' ) - get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ); - // West of UTC that midnight has not arrived yet. Start from the one before it, so a - // recovered run is due immediately instead of later in the day. + // West of UTC that midnight is still ahead; start from the previous one. if ( $timestamp > time() ) { $timestamp -= DAY_IN_SECONDS; } @@ -537,51 +532,40 @@ private function schedule_refresh_db_action(): void { ) { $next = as_next_scheduled_action( $hook, array(), $group ); if ( false === $next ) { - // Unique: this runs on every request, and Action Scheduler only creates the next - // recurrence once the current one completes, so a concurrent request can arrive - // while nothing is pending. Uniqueness is enforced in the insert itself. + // Unique: a concurrent request can arrive while nothing is pending. as_schedule_recurring_action( $timestamp, $interval, $hook, array(), $group, true ); - // The call returns 0 and stores nothing when creation fails, so ask the store. + // Returns 0 on failure, so ask the store. $next = as_next_scheduled_action( $hook, array(), $group ); } - // Drop the WP-Cron fallback only once the action is there to replace it. + // Drop the WP-Cron fallback only once the action exists. if ( false !== $next ) { wp_clear_scheduled_hook( $hook ); return; } } - // Re-arm only when the event is missing or set to a different interval. This runs on - // every request while Action Scheduler keeps refusing, and re-arming a live event - // would pin it to a past timestamp and make the refresh due on every cron spawn. + // Re-arming a live event would pin it to a past timestamp and keep it due. $event = wp_get_scheduled_event( $hook ); if ( $event && $event->schedule === $interval_key ) { return; } - // Schedule the replacement before touching the old event, so a refused schedule leaves - // the old one running. wp_clear_scheduled_hook() would take the new one with it, so - // remove the old event by its own timestamp. The same timestamp means the write above - // already replaced it in place. + // Schedule first so a refused replacement keeps the old event, then remove the old one + // by its timestamp: wp_clear_scheduled_hook() would take the new one too. if ( false === wp_schedule_event( $timestamp, $interval_key, $hook ) ) { return; } - // The refresh is scheduled without arguments, so the old event has none to match. + // A matching timestamp was already overwritten in place. if ( $event && $event->timestamp !== $timestamp ) { wp_unschedule_event( $event->timestamp, $hook ); } } /** - * Check once per window that something still fires the refresh. - * - * Hooked to `init`, so it runs on every request. A transient keeps that to one cached - * read instead of two Action Scheduler queries, and it expires on its own rather than - * rewriting the autoloaded options blob every window. The daily - * `action_scheduler_ensure_recurring_actions` hook is the floor under it. + * Check once per window, on init, that something still fires the refresh. */ public function maybe_reschedule_refresh_db(): void { if ( get_transient( self::REFRESH_DB_CHECK_TRANSIENT ) ) { @@ -590,20 +574,16 @@ public function maybe_reschedule_refresh_db(): void { $this->ensure_refresh_db_action(); - // Only skip the check while there is something to skip it for. An attempt that - // established no trigger is retried on the next request, not after the window. + // Cache only a check that left a trigger; a failed one retries next request. if ( $this->has_refresh_db_trigger() ) { set_transient( self::REFRESH_DB_CHECK_TRANSIENT, 1, self::REFRESH_DB_CHECK_WINDOW ); } } /** - * Keep the DB refresh scheduled on whichever scheduler the site can use. + * Keep the DB refresh scheduled. * - * This is the only way back. Action Scheduler creates the next occurrence of a recurring - * action inside schedule_next_instance(), which a killed run never reaches, so the chain - * ends there and nothing but a reactivation used to restore it. Also covers a site that - * lost Action Scheduler, and a legacy WP-Cron event that never migrated. + * A killed run never reaches schedule_next_instance(), so Action Scheduler's chain ends there. */ public function ensure_refresh_db_action(): void { if ( ! $this->refresh_db_is_settled() ) { @@ -612,11 +592,7 @@ public function ensure_refresh_db_action(): void { } /** - * Whether the refresh sits on the scheduler this site should be using. - * - * Settled only once Action Scheduler holds the action and no WP-Cron event fires the same - * hook beside it; a site keeping both refreshes twice per interval. Being unsettled is a - * reason to act, not a sign that nothing runs. + * Whether the refresh is on Action Scheduler with no WP-Cron event beside it. * * @return bool */ @@ -636,10 +612,7 @@ private function refresh_db_is_settled(): bool { } /** - * Whether anything at all will fire the refresh hook again. - * - * Where Action Scheduler is present but refuses, the WP-Cron fallback is the trigger, and - * a site with one is not in trouble even though it is not settled. + * Whether anything will fire the refresh hook again. * * @return bool */ diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 8c8a90b76..997e889cb 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -10,14 +10,7 @@ /** * Database charts refresh only while `visualizer_schedule_refresh_db` has a live trigger. * - * Action Scheduler returns 0 instead of throwing when it cannot create an action, so the - * plugin used to drop the WP-Cron fallback for an action that was never stored, leaving - * nothing scheduled and no way back. - * - * A filter added inside a test needs no removal. WP_UnitTestCase_Base::set_up() backs up - * $wp_filter and tear_down() restores it wholesale, so a filter cannot reach the next test. - * The tests here that do call remove_filter() call it mid test, because they still assert - * afterwards and need the filter gone first. + * Filters added inside a test need no removal: tear_down() restores $wp_filter wholesale. */ class Test_Visualizer_Schedule_Refresh_Db extends WP_UnitTestCase { @@ -30,8 +23,7 @@ class Test_Visualizer_Schedule_Refresh_Db extends WP_UnitTestCase { public function set_up() { parent::set_up(); - // index.php loads Action Scheduler only when visualizer_can_use_action_scheduler() - // passes, so skip rather than fatal on a host that cannot run it. + // Skip rather than fatal where index.php did not load Action Scheduler. if ( ! class_exists( 'ActionScheduler' ) || ! class_exists( 'ActionScheduler_Store' ) @@ -43,7 +35,7 @@ public function set_up() { as_unschedule_all_actions( self::HOOK, array(), self::GROUP ); wp_clear_scheduled_hook( self::HOOK ); - // the bootstrap activates the plugin, so `init` has already opened a check window. + // the bootstrap's init already opened a check window. delete_transient( Visualizer_Module_Setup::REFRESH_DB_CHECK_TRANSIENT ); } @@ -58,11 +50,7 @@ private function has_trigger(): bool { } /** - * Run what WordPress runs for a plugin lifecycle event. - * - * Calls the callback rather than firing the hook: plugin_basename() resolves differently - * depending on where the plugin directory is loaded from, so the hook name is not stable - * across environments. + * Call the lifecycle callback directly; plugin_basename() is not stable across environments. * * @param string $action Either `activate` or `deactivate`. */ @@ -71,19 +59,13 @@ private function lifecycle( string $action ) { } /** - * Precondition: Action Scheduler is usable, otherwise the rest proves nothing. - * - * This one fails rather than skips, on purpose. set_up() skips the class when Action - * Scheduler is absent, which is an environment this plugin supports. Present but not - * initialized is not one: the library initializes on `init` at priority 1, so reaching a - * test without it means the load order broke, and every as_* call in this class would - * quietly return false and assert nothing. + * Precondition. Fails rather than skips: loaded but uninitialized means the load order broke. */ public function test_action_scheduler_is_available() { $this->assertTrue( function_exists( 'as_schedule_recurring_action' ), 'Action Scheduler must be loaded' ); $this->assertTrue( ActionScheduler::is_initialized(), - 'Action Scheduler is loaded but its data store is not initialized, so every other test in this class would assert nothing. Check that it is loaded before init.' + 'Action Scheduler is loaded but not initialized; the other tests would assert nothing.' ); } @@ -124,9 +106,6 @@ public function test_legacy_wp_cron_event_migrates_to_action_scheduler() { /** * While Action Scheduler keeps refusing, later requests must leave the fallback alone. - * - * Re-arming it on every request pins the event to a past timestamp, so the refresh runs - * on every cron spawn instead of every ten minutes. */ public function test_recovery_does_not_drag_a_live_wp_cron_event_back_into_the_past() { add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); @@ -147,16 +126,12 @@ public function test_recovery_does_not_drag_a_live_wp_cron_event_back_into_the_p /** * A concurrent request must not be able to create a second recurring action. - * - * Action Scheduler creates the next recurrence only after the current one completes, so - * every interval there is a moment with nothing pending. A visitor arriving in that - * window used to add a duplicate, and duplicates never go away on their own. */ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_request_wins_the_race() { $done = false; $seen = array(); - // Stand in for another request that schedules between our lookup and our write. - // Action Scheduler passes $priority before $unique, see its functions.php:165. + // Another request scheduling between our lookup and our write. Action Scheduler + // passes $priority before $unique, see its functions.php:165. $racer = function ( $pre, $timestamp, $interval, $hook, $args, $group, $priority, $unique ) use ( &$done, &$seen ) { if ( ! $done ) { $done = true; @@ -173,9 +148,7 @@ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_ $this->setup_module()->maybe_reschedule_refresh_db(); remove_filter( 'pre_as_schedule_recurring_action', $racer, 10 ); - // The simulation only reproduces the race if the racer read the real arguments, so - // pin the order here rather than trusting the signature. Types alone separate the - // two, whatever value the code under test passes for $unique. + // Pin the argument order by type, whatever value the code passes for $unique. $this->assertIsInt( $seen['priority'], 'the filter must pass $priority before $unique' ); $this->assertIsBool( $seen['unique'], 'the filter must pass $priority before $unique' ); $this->assertTrue( $done, 'precondition: the race was actually simulated' ); @@ -183,10 +156,7 @@ public function test_recovery_does_not_create_a_second_action_when_a_concurrent_ } /** - * A WP-Cron event left beside an Action Scheduler action must go. - * - * Both schedulers fire the same hook, so a site that keeps both refreshes twice per - * interval. A lost race between the fallback and a concurrent request can leave that pair. + * A WP-Cron event left beside an Action Scheduler action must go; both fire the hook. */ public function test_recovery_removes_a_wp_cron_event_left_beside_an_action_scheduler_action() { as_schedule_recurring_action( time(), 600, self::HOOK, array(), self::GROUP, true ); @@ -201,18 +171,14 @@ public function test_recovery_removes_a_wp_cron_event_left_beside_an_action_sche } /** - * A run killed mid flight must not end the recurring chain. - * - * Action Scheduler creates the next occurrence inside schedule_next_instance(), which a - * host kill, fatal or timeout never reaches. The queue cleaner then marks the action - * failed, and nothing succeeds it. This is the scenario confirmed on the reporting site. + * A run killed mid flight must not end the recurring chain (the scenario on the reporting site). */ public function test_a_killed_run_does_not_end_the_recurring_chain() { as_schedule_recurring_action( time(), 600, self::HOOK, array(), self::GROUP, true ); $pending = $this->pending_actions(); $action_id = reset( $pending ); - // what ActionScheduler_QueueCleaner::mark_failures() does to a run that never returned. + // what the queue cleaner does to a run that never returned. $store = ActionScheduler::store(); $store->log_execution( $action_id ); $store->mark_failure( $action_id ); @@ -226,10 +192,7 @@ public function test_a_killed_run_does_not_end_the_recurring_chain() { } /** - * Action Scheduler's own daily assurance hook must restore a missing action. - * - * This is the floor under the per-request check, and it runs even on a site that serves - * no admin requests for a while. + * Action Scheduler's daily assurance hook must restore a missing action. */ public function test_the_daily_action_scheduler_hook_restores_a_missing_action() { $this->assertFalse( $this->has_trigger(), 'precondition: nothing is scheduled' ); @@ -240,10 +203,7 @@ public function test_the_daily_action_scheduler_hook_restores_a_missing_action() } /** - * The per-request check stands down inside its window, and the daily hook does not. - * - * The check runs on `init`, so it must not query Action Scheduler on every request of a - * settled site. The daily assurance hook ignores the window and is the floor. + * The per-request check stands down inside its window; the daily hook does not. */ public function test_the_per_request_check_is_throttled_and_the_daily_hook_is_the_floor() { $module = $this->setup_module(); @@ -264,14 +224,9 @@ public function test_the_per_request_check_is_throttled_and_the_daily_hook_is_th /** * Recovery must make the refresh due now, not at a midnight that has not happened yet. - * - * The start time is local midnight derived from `gmt_offset`. West of UTC that midnight - * can still be ahead of us, which would park the recovered run hours into the future and - * leave the charts stale for the rest of the day. */ public function test_recovery_does_not_park_the_next_run_in_the_future() { - // Far enough west that the computed midnight is ahead of us whatever the time of - // day. WordPress does not clamp gmt_offset, so this stays deterministic. + // Far enough west that the computed midnight is always ahead of now. $hours_into_utc_day = ( time() - strtotime( 'midnight' ) ) / HOUR_IN_SECONDS; add_filter( 'pre_option_gmt_offset', @@ -288,10 +243,6 @@ function () use ( $hours_into_utc_day ) { /** * A filtered interval key WP-Cron does not know must not drop the trigger. - * - * wp_schedule_event() returns false for an unregistered schedule, and the fallback used - * to clear the old event first, so the refresh was left with nothing and the throttle - * then held the retry off. get_schedule_interval_seconds() already copes with this. */ public function test_an_unknown_interval_key_still_leaves_a_trigger() { add_filter( @@ -300,7 +251,7 @@ static function () { return 'not_a_registered_schedule'; } ); - // force the WP-Cron fallback, which is the path that takes $interval_key. + // force the WP-Cron fallback. add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); $this->setup_module()->ensure_refresh_db_action(); @@ -310,9 +261,6 @@ static function () { /** * The start time must be a whole second, whatever gmt_offset holds. - * - * gmt_offset is a number, not an integer, so multiplying it makes the start time a float. - * WP-Cron keys its array by that value and PHP then reports losing precision. */ public function test_a_fractional_offset_does_not_schedule_a_fractional_timestamp() { add_filter( @@ -321,7 +269,7 @@ static function () { return 5.0001; } ); - // force the WP-Cron fallback, which is where the value becomes an array key. + // force the WP-Cron fallback. add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); $lost = array(); @@ -346,9 +294,6 @@ static function ( $errno, $errstr ) use ( &$lost ) { /** * A check that scheduled nothing must be retried, not cached. - * - * The window exists to skip work that is already done. Recording it after an attempt that - * established no trigger leaves the site without one until the window expires. */ public function test_a_failed_check_is_retried_on_the_next_request() { $module = $this->setup_module(); @@ -370,9 +315,6 @@ public function test_a_failed_check_is_retried_on_the_next_request() { /** * A live WP-Cron fallback counts as scheduled, so the window still applies. - * - * Where Action Scheduler is present but refuses, the fallback is the trigger. Reporting - * that as unscheduled makes every request retry the whole check and the refused insert. */ public function test_a_wp_cron_fallback_is_not_re_attempted_on_every_request() { $attempts = 0; @@ -396,13 +338,9 @@ public function test_a_wp_cron_fallback_is_not_re_attempted_on_every_request() { /** * A refused replacement must not take the old WP-Cron event with it. - * - * Changing interval used to clear the old event and then schedule the new one, so a - * refused schedule left nothing. Same principle as the Action Scheduler path: nothing is - * removed until what replaces it exists. */ public function test_a_refused_reschedule_keeps_the_old_wp_cron_event() { - // an event on another interval, with nothing to migrate it to. + // an event on another interval. wp_schedule_event( time() + HOUR_IN_SECONDS, 'hourly', self::HOOK ); add_filter( 'pre_as_schedule_recurring_action', '__return_zero' ); // WP-Cron refuses the replacement. From 02d963078853a5556a3d87bea30658e22518a992 Mon Sep 17 00:00:00 2001 From: Alexia Soare <108459992+Alexia-Soare@users.noreply.github.com> Date: Fri, 25 Sep 2026 09:27:05 +0300 Subject: [PATCH 17/17] test: type the pending action IDs as numeric strings Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test-schedule-refresh-db.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-schedule-refresh-db.php b/tests/test-schedule-refresh-db.php index 997e889cb..d7fc8e037 100644 --- a/tests/test-schedule-refresh-db.php +++ b/tests/test-schedule-refresh-db.php @@ -354,7 +354,7 @@ public function test_a_refused_reschedule_keeps_the_old_wp_cron_event() { /** * Every pending refresh action. * - * @return array + * @return list */ private function pending_actions(): array { return as_get_scheduled_actions(