Audit and optimize ActionScheduler usages

[marcinbot]

See p7bje6-4BB-p2

Using ActionScheduler is not free. Spawning unnecessary actions can affect the site's performance.

Here's a list of improvements from a quick search for schedule_job across the code:

• We issue a bunch of actions after an order is processed. All of those could be rolled into one, which will take an order and perform the actions in bulk:
  • wcpay_update_customer_with_order_data Removed: see paJDYF-6T1-p2
  • wcpay_update_saved_payment_method
  • wcpay_track_new_order and wcpay_track_update_order
  • In addtion, the above 3 points could be moved to WC_Payments_Order_Service
  • wcpay_add_fee_breakdown_to_order_notes
  • In the p2 post above there was also a suggestion of maintaining a list of orders needing attention and running a recurring action processing them in batches.
  • There was also a suggestion to use the shutdown action hook (already used here and here) and avoid the ActionScheduler altogether, which would be preferable
• wcpay_instant_deposit_reminder seems to be an action used properly
• Failed webhook processing, we could try to process them all at once or batch them in bulks of, let's say, 5 (an arbitrary number):
  • wcpay_webhook_process_event shouldn't be issued per single webhook
• Failed webhook fetching (see below)
  • wcpay_webhook_fetch_events
  • This one's quite tricky as it hooks into the account update action, which it maybe shouldn't. In which case some backend work is also required, where we add a new endpoint.
  • Seems to be run every time an account is refreshed, which heavily depends on the logic there, which is unrelated to webhook processing.
  • Should be run using a scheduled recurring action instead (NOT a one-off action requeuing itself)
  • Feel free to break this out into its own issue/task

The above list covers all the async actions from the time of writing, but there might be more in the future. In general:

• We shouldn't issue more than one job if the actions can be batched and processed inside one job
• We should use recurring actions more rather than inventing our own mechanisms using one-off actions

cc @Automattic/gamma

[htdat]

Regarding wcpay_webhook_fetch_events action, I could provide some insights into it:

• It’s not true that this action is run for every account refresh. Instead, we hook into the account refresh, and check the has_more_failed_events flag from the account data to decide whether or not we add this action to ActionScheduler. https://github.com/Automattic/woocommerce-payments/blob/73ffcfda5d6e4e6de52226644a68f18b0ff37716/includes/class-wc-payments-webhook-reliability-service.php#L76-L84

• In other words, this action is only scheduled if the server (through account data refresh) gives a signal that there are failed webhooks stored on the server.

• IMO, this way is pretty optimized already given the fact that only a small set of WCPay sites have the problem with delivering webhook. If we use scheduled recurring action for all sites, it's not efficient at all in this specific case. @RadoslavGeorgiev and I discussed this internally with some data at the moment of implementing this mechanism p1645601664622659/1645086273.563649-slack-C01BPL3ALGP

[marcinbot]

Regarding wcpay_webhook_fetch_events action, I could provide some insights into it:

@htdat I think these are all fair points with regard to this issue which is about optimizing the AS usage. I have updated the description to remove it.

However, one could still argue that the accounts endpoint is not a great place for such flags (and we have more of those than just the failed webhooks, so this is not a criticism of the webhook mechanism). The endpoint represents more static data that can be cached. Hooking into it, or reading dynamic flags is introducing some side effects that might grow difficult to keep track of.

There's also a context of how often the failed webhooks are re-fetched. The account data can be refreshed as rarely as once a day in some cases, and that might not be great for processing webhooks in a timely manner.

Still, both of these are separate issues, so I'm happy to remove that action from the list here :)

[htdat]

@marcinbot - I agree about those two points, and we can consider improving them :)

[dmallory42]

Another relevant report was raised in #6662 around the usage of as_unschedule_action:

The WC_Payments_Action_Scheduler_Service::schedule_job() can get called dozens of times during the process of creating an order. Each time the callback is hit, calling as_unschedule_action() is adding quite a bit of overhead. This handling was added in #1295.

https://github.com/Automattic/woocommerce-payments/blob/db6e4446b9f246897c116c56fee90475fb2e29d0/includes/class-wc-payments-action-scheduler-service.php#L147-L153

Do we need to unschedule and reschedule the job, or can we simply call as_has_scheduled_action() instead?

as_has_scheduled_action() doesn't require that the query for the scheduled action be ordered by the date, so it doesn't have the same query issues with requiring a filesort that the normal query to get the next action does.

Mentioning it here just in case it gets lost between the two issues.