From 9744791cded93aaabdb050681e83eb4c05a5a58c Mon Sep 17 00:00:00 2001 From: Chris Huber Date: Wed, 14 Oct 2026 19:12:45 +0000 Subject: [PATCH] fix: collapse duplicate recurring routine chains on register Once two matching chains existed (e.g. a registration race where two concurrent requests both saw no pending action and both scheduled), register()'s idempotent path returned at the first match and never cancelled the others: every duplicate chain kept firing once per tick, forever (observed in production as paired jobs created in the same second for system-* routines). When several pending chains now match the routine's trigger, register() collapses them: it keeps the highest-id chain and cancels the rest, then returns true as before. The highest id is the only keeper guaranteed to sit at or above the routine's current watermark (Action Scheduler ids are a global increasing sequence and the watermark is always some chain's own first id), so the kept chain can never be killed by the generation fence. A collapse is not a schedule change: no new action is scheduled and no generation/watermark is minted, keeping the unchanged register() path read-only apart from the duplicate cancels. One-shot run_now enqueues are non-recurring and are never collapsed away. Existing duplicate chains self-heal on the next registration (every plugin boot re-registers), so no concurrent double-schedule lock is added around check-then-schedule - the collapse already bounds the race's damage to a single interval, without reintroducing the stuck-lock failure mode fixed for the reconcile lock in #580. Fixes #582. --- ...-agent-routine-action-scheduler-bridge.php | 51 ++++++- tests/routines-durability-smoke.php | 137 +++++++++++++++++- 2 files changed, 177 insertions(+), 11 deletions(-) diff --git a/src/Routines/class-wp-agent-routine-action-scheduler-bridge.php b/src/Routines/class-wp-agent-routine-action-scheduler-bridge.php index 926a9c8..92ea5a8 100644 --- a/src/Routines/class-wp-agent-routine-action-scheduler-bridge.php +++ b/src/Routines/class-wp-agent-routine-action-scheduler-bridge.php @@ -30,7 +30,12 @@ * request. `register()` only unschedules and reschedules when no * pending action exists or its recurrence (interval seconds / cron * expression) no longer matches the routine; an unchanged routine is a - * read-only no-op. + * read-only no-op. When *several* pending chains already match (e.g. + * left behind by a concurrent double-schedule race, where two requests + * both saw no pending action and both scheduled), the extra chains are + * collapsed: the newest (highest action id) chain is kept and every + * other matching chain is cancelled, so duplicate chains self-heal on + * the next registration instead of firing twice per tick forever. * - Generation + watermark fencing: a schedule change mints a fresh * generation and records the new chain's first action id as a * *watermark* in the routine's single option row @@ -217,8 +222,30 @@ public function register( WP_Agent_Routine $routine ): bool { $logical_args = array( 'routine_id' => $routine_id ); $existing = $this->pending_map()[ $routine_id ] ?? array(); - if ( null !== self::matching_action_id( $existing, $routine ) ) { - return true; // Unchanged: nothing to do. + $matching = self::matching_action_ids( $existing, $routine ); + if ( array() !== $matching ) { + // A matching chain already exists. If several do — e.g. a + // concurrent double-schedule race where two requests both saw + // no pending action and both scheduled — collapse the + // duplicates: keep exactly one chain and cancel the rest, so + // the routine stops firing once per duplicate chain per tick. + // The keeper is the *highest* action id: Action Scheduler ids + // are a global increasing sequence and the watermark is always + // some chain's own first id, so the newest chain is the only + // choice guaranteed to sit at or above the routine's current + // watermark (a keeper below the watermark would be cancelled + // by the generation fence at its first execution). One-shot + // `run_now` enqueues are not recurring and never match, so + // they are never collapsed away. + if ( count( $matching ) > 1 ) { + $keeper = max( $matching ); + foreach ( $matching as $action_id ) { + if ( $action_id !== $keeper ) { + $this->cancel( $action_id ); + } + } + } + return true; // Unchanged (or just collapsed): nothing to do. } if ( WP_Agent_Routine::TRIGGER_EXPRESSION === $routine->get_trigger_type() ) { @@ -484,21 +511,29 @@ private function forget_pending_action( int $action_id ): void { } /** - * The pending action, among those already known for a routine, whose - * recurrence matches the routine's current trigger — or null when none + * Every pending action, among those already known for a routine, whose + * recurrence matches the routine's current trigger — empty when none * does (including when the only pending actions are one-shot `run_now` * enqueues, which are never recurring and therefore never "the" * schedule). * + * A non-empty result with more than one element means duplicate + * chains are pending (a concurrent double-schedule race, or an older + * version's failure to collapse them): register() keeps the highest + * id and cancels the rest. + * * @param array $actions_by_id + * @return list Matching action ids, ascending. */ - private static function matching_action_id( array $actions_by_id, WP_Agent_Routine $routine ): ?int { + private static function matching_action_ids( array $actions_by_id, WP_Agent_Routine $routine ): array { + $matching = array(); foreach ( $actions_by_id as $action_id => $action ) { if ( self::recurrence_matches( $action, $routine ) ) { - return (int) $action_id; + $matching[] = (int) $action_id; } } - return null; + sort( $matching ); + return $matching; } /** diff --git a/tests/routines-durability-smoke.php b/tests/routines-durability-smoke.php index 6035f03..b1fd7e4 100644 --- a/tests/routines-durability-smoke.php +++ b/tests/routines-durability-smoke.php @@ -871,9 +871,140 @@ function smoke_hook_fired( string $hook ): bool { durable_assert( false, array_key_exists( 'agents_routine_action_generation_5', $GLOBALS['wpdb']->rows ), 'maybe_purge: the first call purges legacy rows' ); durable_assert( true, is_numeric( get_option( 'agents_routine_action_generation_purged', false ) ), 'maybe_purge: a marker option records that the purge ran' ); -$GLOBALS['wpdb']->rows['agents_routine_action_generation_6'] = 'gen-d'; -WP_Agent_Routine_Action_Scheduler_Bridge::maybe_purge_legacy_action_generation_options(); -durable_assert( true, array_key_exists( 'agents_routine_action_generation_6', $GLOBALS['wpdb']->rows ), 'maybe_purge: a second call is gated by the marker and does not run again' ); + $GLOBALS['wpdb']->rows['agents_routine_action_generation_6'] = 'gen-d'; + WP_Agent_Routine_Action_Scheduler_Bridge::maybe_purge_legacy_action_generation_options(); + durable_assert( true, array_key_exists( 'agents_routine_action_generation_6', $GLOBALS['wpdb']->rows ), 'maybe_purge: a second call is gated by the marker and does not run again' ); + +// --------------------------------------------------------------------------- +// 9. Duplicate-chain collapse (issue #582): two matching pending chains — +// the residue of a concurrent double-schedule race where two requests +// both saw no pending action and both scheduled — are collapsed to one +// on the next register() call, instead of firing once per chain per +// tick forever. +// --------------------------------------------------------------------------- + +// 9a. The observed production state: the watermark points at the LOWER +// chain (the racing request's watermark write won), so both chains run +// unfenced and the routine fires twice per tick. register() must +// collapse to the highest-id chain without scheduling anything or +// minting a new generation. +smoke_reset_state(); +$chain_low = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() + 300, + 300 +); +$chain_high = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() + 300, + 300 +); +update_option( + 'agents_routine_generation_dup', + array( 'generation' => 'race-gen', 'watermark' => $chain_low ) +); + +$as_next_before = $GLOBALS['smoke_as_next_id']; +$GLOBALS['smoke_as_cancel_calls'] = array(); +$collapse = WP_Agent_Routine_Registry::register( 'dup', array( 'agent' => 'commander', 'interval' => 300 ) ); +durable_assert( true, $collapse instanceof WP_Agent_Routine, 'collapse: registering a duplicated routine still succeeds' ); +durable_assert( array( $chain_low ), $GLOBALS['smoke_as_cancel_calls'], 'collapse: exactly the duplicate (lowest-id) chain is cancelled' ); +durable_assert( $as_next_before, $GLOBALS['smoke_as_next_id'], 'collapse: no new action is scheduled' ); +durable_assert( + array( 'generation' => 'race-gen', 'watermark' => $chain_low ), + get_option( 'agents_routine_generation_dup', array() ), + 'collapse: generation/watermark are untouched — a collapse is not a schedule change' +); +durable_assert( array( $chain_high ), array_keys( smoke_pending_rows() ), 'collapse: exactly one matching chain remains pending' ); + +// The kept chain is above even the stale-low watermark, so it runs +// unfenced and fires the routine callback exactly once. +$before = count( $GLOBALS['smoke_chat_ability']->calls ); +durable_assert( true, smoke_as_run( $chain_high ), 'collapse: the kept (highest-id) chain runs unfenced' ); +durable_assert( $before + 1, count( $GLOBALS['smoke_chat_ability']->calls ), 'collapse: the kept chain fires the routine callback' ); + +// After the collapse, a re-register is a pure read-only no-op again. +$GLOBALS['smoke_as_cancel_calls'] = array(); +$as_next_before = $GLOBALS['smoke_as_next_id']; +WP_Agent_Routine_Registry::register( 'dup', array( 'agent' => 'commander', 'interval' => 300 ) ); +durable_assert( array(), $GLOBALS['smoke_as_cancel_calls'], 'collapse: the follow-up register cancels nothing' ); +durable_assert( $as_next_before, $GLOBALS['smoke_as_next_id'], 'collapse: the follow-up register schedules nothing' ); + +// 9b. A worker already claimed the duplicate when the collapse landed: the +// claim raced the cancel, so the runner still tries the claimed +// action. The generation fence (its id is below the watermark — here +// the racing watermark write pointed at the higher chain) cancels it +// at before_execute, so it neither fires nor spawns a successor; the +// keeper is unaffected. +smoke_reset_state(); +$chain_low = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup-claimed' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() + 300, + 300 +); +$chain_high = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup-claimed' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() + 300, + 300 +); +update_option( + 'agents_routine_generation_dup-claimed', + array( 'generation' => 'race-gen-2', 'watermark' => $chain_high ) +); + +$claimed_row = $GLOBALS['smoke_as'][ $chain_low ]; +WP_Agent_Routine_Registry::register( 'dup-claimed', array( 'agent' => 'commander', 'interval' => 300 ) ); +durable_assert( array( $chain_low ), $GLOBALS['smoke_as_cancel_calls'], 'in-flight: the collapse cancels the duplicate chain by id' ); +durable_assert( array( $chain_high ), array_keys( smoke_pending_rows() ), 'in-flight: exactly the keeper remains pending' ); + +// Resurrect the claimed row as pending to model the claim winning the race. +$GLOBALS['smoke_as'][ $chain_low ] = $claimed_row; +$GLOBALS['smoke_as'][ $chain_low ]['status'] = ActionScheduler_Store::STATUS_PENDING; + +$before = count( $GLOBALS['smoke_chat_ability']->calls ); +durable_assert( false, smoke_as_run( $chain_low ), 'in-flight: the claimed duplicate is fenced at execution (id below the watermark)' ); +durable_assert( $before, count( $GLOBALS['smoke_chat_ability']->calls ), 'in-flight: the claimed duplicate does not fire the routine callback' ); +durable_assert( ActionScheduler_Store::STATUS_CANCELED, $GLOBALS['smoke_as'][ $chain_low ]['status'], 'in-flight: the claimed duplicate stays cancelled — no recurrence successor spawns' ); +durable_assert( true, smoke_as_run( $chain_high ), 'in-flight: the keeper runs normally' ); +durable_assert( $before + 1, count( $GLOBALS['smoke_chat_ability']->calls ), 'in-flight: the keeper fires the routine callback' ); + +// 9c. One-shot run_now enqueues are never recurring and must survive a +// collapse: only duplicate *recurring* chains are cancelled (also +// covers collapsing with no recorded watermark at all — pre-fence +// legacy chains). +smoke_reset_state(); +$chain_low = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup-one-shot' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() + 300, + 300 +); +$chain_high = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup-one-shot' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() + 300, + 300 +); +$one_shot = smoke_as_save( + WP_Agent_Routine_Action_Scheduler_Bridge::SCHEDULED_HOOK, + array( 'routine_id' => 'dup-one-shot' ), + WP_Agent_Routine_Action_Scheduler_Bridge::GROUP, + time() +); +WP_Agent_Routine_Registry::register( 'dup-one-shot', array( 'agent' => 'commander', 'interval' => 300 ) ); +durable_assert( array( $chain_low ), $GLOBALS['smoke_as_cancel_calls'], 'one-shot: only the duplicate recurring chain is cancelled' ); +durable_assert( array( $chain_high, $one_shot ), array_keys( smoke_pending_rows() ), 'one-shot: the pending run_now enqueue survives the collapse alongside the keeper' ); +durable_assert( false, array_key_exists( 'agents_routine_generation_dup-one-shot', $GLOBALS['smoke_options'] ), 'one-shot: no generation option is minted when there was none' ); // ---------------------------------------------------------------------------