Commit ebb0e8f7a14 for woocommerce
commit ebb0e8f7a140776f8607a388085b916431bf6a77
Author: Adam Grzybkowski <agrzybkowski@outlook.com>
Date: Mon Jun 8 20:45:05 2026 +0200
Fix push notification safety net not canceled on delivery (#65574)
* Fix push notification safety net not canceled on delivery
schedule_safety_net() queued the Action Scheduler safety-net action with positional args while cancel_safety_net() matched with associative args, so the exact-args match never succeeded and the stale action fired ~60s later (and could duplicate pushes on the retry path). Add Notification::get_safety_net_args() as the single source of truth for the match args (identity-keyed; StockNotification appends event_type) and use it in schedule, dedupe, and cancel. Rework the cancel tests to seed via the real schedule path and add StockNotification coverage.
* Add changelog entry for safety-net cancel fix
diff --git a/plugins/woocommerce/changelog/fix-push-notification-safety-net-cancel b/plugins/woocommerce/changelog/fix-push-notification-safety-net-cancel
new file mode 100644
index 00000000000..ad10758f99b
--- /dev/null
+++ b/plugins/woocommerce/changelog/fix-push-notification-safety-net-cancel
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Push notifications: cancel the safety-net Action Scheduler action on delivery by matching schedule and cancel on a single identity-keyed args shape, so stale actions no longer fire ~60s later (and the retry path can no longer produce duplicate pushes).
diff --git a/plugins/woocommerce/src/Internal/PushNotifications/Notifications/Notification.php b/plugins/woocommerce/src/Internal/PushNotifications/Notifications/Notification.php
index bfe2b0352f9..d898a4a75c4 100644
--- a/plugins/woocommerce/src/Internal/PushNotifications/Notifications/Notification.php
+++ b/plugins/woocommerce/src/Internal/PushNotifications/Notifications/Notification.php
@@ -172,6 +172,28 @@ abstract class Notification {
return $this->resource_id;
}
+ /**
+ * Canonical positional ActionScheduler arguments for the safety-net job.
+ *
+ * Single source of truth shared by the scheduler (and its dedupe guard) and
+ * the cancel path so the serialized args always match. Action Scheduler
+ * matches the stored args by exact equality, so any divergence between the
+ * schedule-side and cancel-side shapes silently breaks cancellation.
+ *
+ * The args are keyed on the notification's *identity* — the minimal data
+ * needed to uniquely identify and reconstruct the notification — mirroring
+ * {@see self::get_identifier()}. Volatile payload fields (e.g. a stock
+ * snapshot captured at trigger time) must not be included: they are not part
+ * of the identity and may differ between schedule and cancel.
+ *
+ * @return array<int, mixed>
+ *
+ * @since 10.9.0
+ */
+ public function get_safety_net_args(): array {
+ return array( $this->get_type(), $this->get_resource_id() );
+ }
+
/**
* Decide whether this notification should be delivered to a user given
* their stored preference value for {@see static::get_type()}.
diff --git a/plugins/woocommerce/src/Internal/PushNotifications/Notifications/StockNotification.php b/plugins/woocommerce/src/Internal/PushNotifications/Notifications/StockNotification.php
index 6d6eb3bb0eb..6335b2d1e42 100644
--- a/plugins/woocommerce/src/Internal/PushNotifications/Notifications/StockNotification.php
+++ b/plugins/woocommerce/src/Internal/PushNotifications/Notifications/StockNotification.php
@@ -152,6 +152,32 @@ class StockNotification extends Notification {
);
}
+ /**
+ * {@inheritDoc}
+ *
+ * Appends `event_type` because it is part of this notification's identity
+ * (see {@see self::get_identifier()}): the same product can have distinct
+ * low_stock / out_of_stock / on_backorder safety nets pending at once, and
+ * the callback needs it to reconstruct the correct subtype.
+ *
+ * `stock_quantity_at_trigger` is deliberately omitted — it is volatile
+ * payload data, not identity, and does not round-trip through every cancel
+ * path, so including it in the match key would risk breaking cancellation.
+ * The safety-net fallback message reads current product stock when it is
+ * absent (see {@see self::build_message()}).
+ *
+ * @return array<int, mixed>
+ *
+ * @since 10.9.0
+ */
+ public function get_safety_net_args(): array {
+ return array(
+ $this->get_type(),
+ $this->get_resource_id(),
+ array( 'event_type' => $this->event_type ),
+ );
+ }
+
/**
* {@inheritDoc}
*
diff --git a/plugins/woocommerce/src/Internal/PushNotifications/Services/NotificationProcessor.php b/plugins/woocommerce/src/Internal/PushNotifications/Services/NotificationProcessor.php
index 07d553f2a64..41c8b41e3f6 100644
--- a/plugins/woocommerce/src/Internal/PushNotifications/Services/NotificationProcessor.php
+++ b/plugins/woocommerce/src/Internal/PushNotifications/Services/NotificationProcessor.php
@@ -246,12 +246,12 @@ class NotificationProcessor {
* @since 10.9.0
*/
private function cancel_safety_net( Notification $notification ): void {
+ // Must match the shape PendingNotificationStore::schedule_safety_net() used;
+ // both derive the args from Notification::get_safety_net_args() so the
+ // exact-equality match Action Scheduler performs succeeds.
as_unschedule_all_actions(
self::SAFETY_NET_HOOK,
- array(
- 'type' => $notification->get_type(),
- 'resource_id' => $notification->get_resource_id(),
- ),
+ $notification->get_safety_net_args(),
self::ACTION_SCHEDULER_GROUP
);
}
diff --git a/plugins/woocommerce/src/Internal/PushNotifications/Services/PendingNotificationStore.php b/plugins/woocommerce/src/Internal/PushNotifications/Services/PendingNotificationStore.php
index 227dac2038d..f73ba5d372c 100644
--- a/plugins/woocommerce/src/Internal/PushNotifications/Services/PendingNotificationStore.php
+++ b/plugins/woocommerce/src/Internal/PushNotifications/Services/PendingNotificationStore.php
@@ -120,16 +120,10 @@ class PendingNotificationStore {
* @since 10.7.0
*/
private function schedule_safety_net( Notification $notification ): void {
- $data = $notification->to_array();
- $type = $data['type'];
- $resource_id = $data['resource_id'];
- unset( $data['type'], $data['resource_id'] );
-
- // Pass `type` and `resource_id` positionally and bundle any subclass-specific
- // extras (event_type, stock_quantity_at_trigger, etc.) into a single array
- // argument so the safety-net callback signature stays stable as new
- // notification subclasses add fields to to_array().
- $args = array( $type, $resource_id, $data );
+ // Canonical, identity-keyed args shared with NotificationProcessor::cancel_safety_net().
+ // Action Scheduler matches stored args by exact equality, so both sides must derive
+ // them from the same place; see Notification::get_safety_net_args().
+ $args = $notification->get_safety_net_args();
if ( as_has_scheduled_action( NotificationProcessor::SAFETY_NET_HOOK, $args, NotificationProcessor::ACTION_SCHEDULER_GROUP ) ) {
return;
diff --git a/plugins/woocommerce/tests/php/src/Internal/PushNotifications/Services/NotificationProcessorTest.php b/plugins/woocommerce/tests/php/src/Internal/PushNotifications/Services/NotificationProcessorTest.php
index 1e8f03abe49..94b04ce501e 100644
--- a/plugins/woocommerce/tests/php/src/Internal/PushNotifications/Services/NotificationProcessorTest.php
+++ b/plugins/woocommerce/tests/php/src/Internal/PushNotifications/Services/NotificationProcessorTest.php
@@ -5,6 +5,7 @@ declare( strict_types = 1 );
namespace Automattic\WooCommerce\Tests\Internal\PushNotifications\Services;
use Automattic\WooCommerce\Internal\PushNotifications\DataStores\PushTokensDataStore;
+use Automattic\WooCommerce\Internal\PushNotifications\Dispatchers\InternalNotificationDispatcher;
use Automattic\WooCommerce\Internal\PushNotifications\Dispatchers\WpcomNotificationDispatcher;
use Automattic\WooCommerce\Internal\PushNotifications\Entities\PushToken;
use Automattic\WooCommerce\Internal\PushNotifications\Notifications\NewOrderNotification;
@@ -15,6 +16,7 @@ use Automattic\WooCommerce\Internal\PushNotifications\PushNotifications;
use Automattic\WooCommerce\Internal\PushNotifications\Services\NotificationPreferencesService;
use Automattic\WooCommerce\Internal\PushNotifications\Services\NotificationProcessor;
use Automattic\WooCommerce\Internal\PushNotifications\Services\NotificationRetryHandler;
+use Automattic\WooCommerce\Internal\PushNotifications\Services\PendingNotificationStore;
use Automattic\WooCommerce\RestApi\UnitTests\LoggerSpyTrait;
use WC_Helper_Product;
use WC_Unit_Test_Case;
@@ -615,28 +617,21 @@ class NotificationProcessorTest extends WC_Unit_Test_Case {
$notification = new NewOrderNotification( $this->order_id );
- as_schedule_single_action(
- time() + NotificationProcessor::SAFETY_NET_DELAY,
- NotificationProcessor::SAFETY_NET_HOOK,
- array(
- 'type' => $notification->get_type(),
- 'resource_id' => $this->order_id,
- ),
- NotificationProcessor::ACTION_SCHEDULER_GROUP
+ // Seed the precondition through the REAL schedule path so the test setup
+ // and production use the same arg shape; if the schedule shape ever drifts
+ // from the cancel shape again, this assertion fails in CI.
+ $this->schedule_safety_net( $notification );
+ $this->assertTrue(
+ $this->is_safety_net_pending( $notification ),
+ 'Safety net should be pending before processing.'
);
$this->sut->process( $notification );
- $scheduled = as_next_scheduled_action(
- NotificationProcessor::SAFETY_NET_HOOK,
- array(
- 'type' => 'store_order',
- 'resource_id' => $this->order_id,
- ),
- NotificationProcessor::ACTION_SCHEDULER_GROUP
+ $this->assertFalse(
+ $this->is_safety_net_pending( $notification ),
+ 'Safety net should be cancelled after successful send.'
);
-
- $this->assertFalse( $scheduled, 'Safety net should be cancelled after successful send.' );
}
/**
@@ -652,28 +647,94 @@ class NotificationProcessorTest extends WC_Unit_Test_Case {
$notification = new NewOrderNotification( $this->order_id );
- as_schedule_single_action(
- time() + NotificationProcessor::SAFETY_NET_DELAY,
- NotificationProcessor::SAFETY_NET_HOOK,
- array(
- 'type' => $notification->get_type(),
- 'resource_id' => $this->order_id,
- ),
- NotificationProcessor::ACTION_SCHEDULER_GROUP
+ $this->schedule_safety_net( $notification );
+ $this->assertTrue(
+ $this->is_safety_net_pending( $notification ),
+ 'Safety net should be pending before processing.'
);
$this->sut->process( $notification );
- $scheduled = as_next_scheduled_action(
- NotificationProcessor::SAFETY_NET_HOOK,
+ $this->assertFalse(
+ $this->is_safety_net_pending( $notification ),
+ 'Safety net should be cancelled when retry is scheduled.'
+ );
+ }
+
+ /**
+ * Locks the schedule/cancel arg-shape contract for the StockNotification
+ * subclass, whose identity (and therefore safety-net match key) includes
+ * `event_type`. Schedules via the real path, then cancels using a
+ * notification reconstructed from the serialized payload — exactly what the
+ * loopback controller does on the primary success path — to prove the
+ * round-tripped args still match.
+ *
+ * @testdox Should cancel a stock notification's safety net using a reconstructed notification.
+ */
+ public function test_process_cancels_safety_net_for_stock_notification(): void {
+ $product = WC_Helper_Product::create_simple_product(
+ true,
array(
- 'type' => 'store_order',
- 'resource_id' => $this->order_id,
- ),
- NotificationProcessor::ACTION_SCHEDULER_GROUP
+ 'manage_stock' => true,
+ 'stock_quantity' => 0,
+ )
+ );
+
+ $this->dispatcher->method( 'dispatch' )->willReturn(
+ array(
+ 'success' => true,
+ 'retry_after' => null,
+ )
+ );
+
+ $notification = new StockNotification( $product->get_id(), StockNotification::EVENT_OUT_OF_STOCK );
+
+ $this->schedule_safety_net( $notification );
+ $this->assertTrue(
+ $this->is_safety_net_pending( $notification ),
+ 'Stock safety net should be pending before processing.'
);
- $this->assertFalse( $scheduled, 'Safety net should be cancelled when retry is scheduled.' );
+ // Reconstruct from the serialized payload, mirroring the loopback controller.
+ $reconstructed = Notification::from_array( $notification->to_array() );
+ $this->sut->process( $reconstructed );
+
+ $this->assertFalse(
+ $this->is_safety_net_pending( $notification ),
+ 'Stock safety net should be cancelled after successful send.'
+ );
+ }
+
+ /**
+ * Schedules a safety-net action through the real
+ * {@see PendingNotificationStore::schedule_safety_net()} so tests exercise
+ * the production schedule shape rather than a hand-built fixture.
+ *
+ * @param Notification $notification The notification to schedule a safety net for.
+ * @return void
+ */
+ private function schedule_safety_net( Notification $notification ): void {
+ $store = new PendingNotificationStore();
+ $store->init( $this->createMock( InternalNotificationDispatcher::class ) );
+
+ $method = new \ReflectionMethod( PendingNotificationStore::class, 'schedule_safety_net' );
+ $method->setAccessible( true );
+ $method->invoke( $store, $notification );
+ }
+
+ /**
+ * Whether a safety-net action is currently scheduled for a notification,
+ * matched on its canonical args.
+ *
+ * @param Notification $notification The notification to check.
+ * @return bool
+ */
+ private function is_safety_net_pending( Notification $notification ): bool {
+ return as_has_scheduled_action(
+ NotificationProcessor::SAFETY_NET_HOOK,
+ $notification->get_safety_net_args(),
+ NotificationProcessor::ACTION_SCHEDULER_GROUP
+ );
}
/**