Skip to content

fix: collapse duplicate recurring routine chains on register #583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chubes4 wants to merge 1 commit into from
Closed

fix: collapse duplicate recurring routine chains on register #583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions src/Routines/class-wp-agent-routine-action-scheduler-bridge.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() ) {
Expand Down Expand Up @@ -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<int,\ActionScheduler_Action> $actions_by_id
* @return list<int> 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;
}

/**
Expand Down
137 changes: 134 additions & 3 deletions tests/routines-durability-smoke.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' );

// ---------------------------------------------------------------------------

Expand Down
Loading