Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
174 stars 69 forks source link

Better handle multiple delete WC Pay subscription requests #3696

Closed james-allan closed 2 years ago

james-allan commented 2 years ago

Describe the bug

Slack thread: p1642825331219600-slack-CGGCLBN58

When a subscription is cancelled on the WC side, or the invoice_upcoming webhook handling code determiners that the subscription needs to be cancelled, we send a request to cancel the subscription on Stripe's end. This results in a delete request.

Currently, it seems possible to send multiple delete requests for the same subscription which Stripe will respond with a 404.

To Reproduce

  1. Disable the WC Subscriptions plugin and enable WC Pay subscriptions.
  2. Purchase a subscription product.
  3. Fully cancel the subscription immediately.
  4. Add the following code snippet to your site replacing the 123 with your subscription ID.
add_action( 'shutdown', function() {
    $subscription = wcs_get_subscription( 123 );

    if ( $subscription ) {
        do_action( 'woocommerce_subscription_status_cancelled', $subscription );
    }
} );

Actual behavior

I haven't been able to replicate this issue locally without using the code snippet. The error logs suggest that there was likely some loop or error condition that caused this site to spam the server with these subscription cancel requests because of the sheer number of requests (700+) over the space of 7 minutes (1-2 delete requests a second). That's not a manual interaction that would cause this.

Screen Shot 2022-01-24 at 12 11 56 pm

Expected behavior

We should avoid spamming the server in this way. I'm not 100% what the right approach is here.

james-allan commented 2 years ago

It was suggested on P2 (pdjTHR-wm-p2) that we should use the already existing Rate_Limit_Factory on server to limit the number of requests that get proxied to Stripe.

It looks very straightforward, however, we can use current uses of it (like Apple_Pay_Domains_Controller) as inspiration for how this can be used.

james-allan commented 2 years ago

Closing this issue here. Unless we intend on doing something client-side as well we can just address this issue on the server (1571-gh-Automattic/woocommerce-payments-server).

Feel free to re-open if we intend to add rate limiting on client-side as well.

zmaglica commented 2 years ago

Hi @james-allan . Is this issue still relevant? Based on your comment, I noticed that you want to close the issue, but it is still open.

james-allan commented 2 years ago

Oh thanks @zmaglica Yeah I meant to close it.