UPE payments and webhooks have a race condition

[reykjalin]

Describe the bug

When processing payments made via UPE (the new checkout experience) we do some initial processing, set the order status to pending and then redirect to the order received page. Once the customer is on the order received page we finalize the payment in WC_Stripe_UPE_Payment_Gateway::process_order_for_confirmed_intent.

Independently from this we receive webhooks from Stripe and the payment_intent.success webhook calls WC_Stripe_Payment_Gateway::process_response where we call WC_Order::payment_complete if the intent status is succeeded.

Usually this isn't a problem, but when the webhook is processed before the customer loads the Order Received page we never finish processing the payment in the UPE gateway because the order status is wrong; it's been set to processing by the call to WC_Order::payment_complete when processing the webhook.

This usually doesn't cause a problem since the payment is completed and the order is marked as such. However, when it's necessary to save a card -- such as in the case of subscriptions -- the payment method ID is never saved to the order, causing subscription renewals to fail. This also makes it so that the card is never saved to the customer.

• Patch the issue Fix UPE payment webhook race condition #2540.
• Introduce a proper fix, perhaps by refactoring the UPE::process_payment method, perhaps by refactoring the webhook processing.

To Reproduce

Warning
You must be using UPE on your test store to reproduce the issue.

Warning
You must have webhooks set up correctly on your test store to reproduce the issue.

1. Set a breakpoint here and start your debugger.
2. Add a subscription product to your cart.
3. Go to the checkout and complete a payment successfully, e.g. with 4242 4242 4242 4242.
4. Let the debugger sit on the breakpoint until you're sure the webhook has been processed. Check your Stripe dashboard or Stripe CLI logs to be extra sure.
5. Once the webhook is processed, step through the code and note that you'll hit an early return here.
6. You can safely continue the debugger and stop it.
7. Now go check My Account → Payment Methods; you'll see the card you used isn't present.
8. Follow the Merchant - Renew subscription automatically critical flow; the renewal will fail.

Expected behavior

No race condition; the order is properly processed despite processing the webhook properly.

Warning
You must be using UPE on your test store to reproduce this simulated success.

Warning
You must have webhooks set up correctly on your test store to reproduce this simulated success.

You can simulate the proper behavior by following these steps:

1. Set a breakpoint here.
2. Add a subscription product to your cart.
3. Go to the checkout and complete a payment successfully, e.g. with 4242 4242 4242 4242.
4. Let the debugger sit on the breakpoint until you've been successfully redirected to the Order Received page.
5. Once you've been successfully redirected to the Order Received page, step through the code and note that you'll hit an early return here because of the current status of the order (processing, or completed).
6. You can safely continue the debugger and stop it.
7. Now go check My Account → Payment Methods; you'll see the card you used has been saved.
8. Follow the Merchant - Renew subscription automatically critical flow; the renewal will succeed.

Screenshots

N/A

Environment (please complete the following information):**

• WordPress Version: 6.1.1
• WooCommerce Version: 7.3.0
• Stripe Plugin Version: develop
• Browser [e.g. chrome, safari] and Version
• Any other plugins installed: WooCommerce, WooCommerce Subscriptions, Code Snippets.

Additional context

p1673550580661099-slack-C01BZUL57SQ

[reykjalin]

#2540 has a stop-gap fix for this issue but it's not ideal; just an easy way to get the fix out so we can patch the issue.

We should take some time to refactor the UPE::process_payment method such that we don't need to rely on more order meta to process things correctly. Alternatively, refactoring the webhook processing may be warranted.

I'll bump the priority on this issue down to high once #2540 has been merged.

[jrick1229]

Looks like we've seen this here: 5986498-zen
We were able to downgrade Stripe and the issue did not persist.

[reykjalin]

@jrick1229 what version of the plugin was the merchant using before the downgrade? And what version did you downgrade the merchant to?

I ask because this was patched in 7.1.0 and shouldn't be an issue. If it is there's something more going on we need to look into further.

[jrick1229]

That's interesting @reykjalin – I totally missed the merged PR, my bad. With that said, they were definitely using v7.1.0 and downgraded to the last major version available.

[jrick1229]

It looks like they just responded back and said it happened again, so definitely not related like I had hoped.

[ecairol]

@reykjalin I might have found a situation where a similar (or the same) race condition is happening between UPE payments and Webhooks.

Here are the details I have collected so far:

• Stripe shows a webhook with a 200 response but the Order never updates from Pending to Processing
• The issue has only happened with users that have a customer_id (not guests)
• The log Begin processing UPE redirect payment for order 13759 for the amount of 14.00 is not present on the Orders with the problem
• Instead, it logs this extra line: Could not find order via charge ID: ch_3N6kWyFeHJGKu6uz0S8roa2H
• The orders with the issue have the meta_key _stripe_upe_waiting_for_redirect with a value of `
• The orders with the issue are missing the meta_key _stripe_charge_captured and _stripe_upe_redirect_processed (I think this missing values are causing process_webhook_capture() to return out prematurely)
• The orders with the issue also don't have a _transaction_id
• There's no suspicious pattern in the data Stripe sends to the webhook
• The site is using version Version 7.4.0
• The last time the problem happened was on May 12

If you don't think it's related to this issue but you have some ideas of where should I take a look, I'd appreciate it.
Also let me know if I should open a new bug.

[ecairol]

Hi team!

The fix introduced on PR #2550 is causing the flow to return before $this->process_response( $charge, $order ); is executed, leaving the Order in a perpetual status of "Pending", where it should be "Processing".

I'm aware the actual solution involves refactoring the need for additional metadata, however, in the meantime, would something like this work?

// TODO: This is a stop-gap to fix a critical issue, see
// https://github.com/woocommerce/woocommerce-gateway-stripe/issues/2536. It would
// be better if we removed the need for additional meta data in favor of refactoring
// this part of the payment processing.
if ( $order->get_meta( '_stripe_upe_waiting_for_redirect' ) ?? false ) {
	
	$attempts      = 0;
	$still_waiting = true;
	while ( $attempts < 2 ) {
		sleep( 1 ); // Wait 1s for the race condition between UPE payments and webhooks to be resolved.
		$still_waiting = $order->get_meta( '_stripe_upe_waiting_for_redirect' ) ?? false;
		if ( ! $still_waiting ) {
			$charge = $this->get_latest_charge_from_intent( $intent );
			break;
		}
		$attempts++;
	}

	if ( $still_waiting ) {
		WC_Stripe_Logger::log( "Stripe UPE redirect took too much time for order $order_id" );
		return;
	}
}

do_action( 'wc_gateway_stripe_process_payment', $charge, $order );

// Process valid response.
$this->process_response( $charge, $order );
break;

I haven't tested the code above because I want to check with you first if this is a solution that you could potentially accept in a PR, or if it's a solution at all.

We are seeing this issue every couple of days on one of our sites.

From my point of view, waiting 1 or 2 extra seconds is better for customers than getting their order in a "Pending" state when they have already paid, because they need to contact the merchant, and the merchant has to do the manual validation and change the Order status manually.

Note: this issue happens only to customers that already have a user account, I suspect that makes the purchase process go quicker and there's more chance for the race condition to manifest.

Let me know your thoughts, if the code above is something that looks promising I can open the PR.

[xpurichan]

Hi @reykjalin we're eager to hear your thoughts on the above ⬆️ One of our Team 51 partners has been experiencing repeated issues with this and we would be glad to collaborate on a solution with you. Thanks!

[reykjalin]

@ecairol thank you for taking a look! We haven't had the time to look into this for a long time, but maybe @diegocurbelo and Quark can come back to this soon 🤔

@xpurichan unfortunately a complete fix here isn't all that simple, just like the discussion has revealed so far. The implementation is a bit flawed fundamentally and a significant bit of exploration and refactoring is needed to completely fix the issue. That doesn't mean there isn't any room for collaboration, but it does mean it's going to take some time to fix the issue.

@diegocurbelo and @woocommerce/quark are going to be working more on Stripe going forward so I'd have to defer to them on how this will be prioritized and worked on.

[ecairol]

Completely understood @reykjalin

What I wonder is if we can submit a PR with an updated patch while @woocommerce/quark works on a definite solution.

The two ideas that I have are:

1. Before return , do a sleep( 1 ); an try again. Or...
2. Execute a custom hook lik do_action( 'incomplete_ process_payment_intent_success', $charge) before the return; – that way the theme can evaluate and update the Order Status automatically.

Ideally, also a WC_Stripe_Logger::log() to make it easier for folks debugging this issue in the future .

We're more than glad to submit a PR with these suggestions but would like to hear the team's thoughts first.

[xpurichan]

Hi @reykjalin would you be able to share input on @ecairol ideas above? ⬆️

[ecairol]

For reference, I went ahead and opened a PR for option 2 above, which is the less invasive and would allow us to implement a fix on the theme: #2638

[crweiner]

Hi @reykjalin! We wanted to ping you to see if you had a chance to review @ecairol's PR #2638 above ⬆️ . Let us know how we can best assist, thank you!

[reykjalin]

Sorry it took me a long time to get to it! I've been really busy after stepping in as interim lead of Fractal while working through several SIRTs, and adding to that the fact that ownership of the Stripe plugin was transferred to Quark put this low on my prioritization list. I apologize for the delay!

I've replied to the PR and started a discussion in Slack with @diegocurbelo and Quark on how to proceed.

[crweiner]

Thank you very much @reykjalin! No worries at all and thank you for taking a look at the PR! @ecairol, I believe the Slack thread is at p1688179542591189-slack-CHG7MTCAF if it's easier to jump into there. 😄

It looks like @diegocurbelo has approved the changes so far too! 🎉

[ecairol]

Thanks for the ping @crweiner, and thank you @reykjalin and @diegocurbelo. I was AFK this week, but I'll merge the PR after the base branch is updated (it's running some checks now)

@crweiner Since this is going to be released on the product, I think we can go and patch the plugin ourselves, and then add a small fix at the theme level so that, whenever the issue happens, the Order status is changed automatically.

[toncijajic]

Since this is going to be released on the product, I think we can go and patch the plugin ourselves, and then add a small fix at the theme level so that, whenever the issue happens, the Order status is changed automatically.

@ecairol this sounds good, let us know once it's done.

[WillBrubaker]

I'm reasonably certain we've got a case of this in 6688022-zen. Pertinent bits from the debug log:

2023-08-16T09:45:31+00:00 DEBUG 
====Stripe Version: 7.5.0====
====Start Log====
Stripe PaymentIntent pi_3NfgFtAAQlwIAChE0XYrMLMP succeeded for order 13162
====End Log====


2023-08-16T09:45:31+00:00 DEBUG 
====Stripe Version: 7.5.0====
====Start Log====
Stripe UPE waiting for redirect. The status for order 13162 might need manual adjustment.
====End Log====

[ecairol]

@WillBrubaker yes, that seems to be the case!

This is what I added to functions.php to patch the problem:

function site_change_order_to_processing( $order ) {
    $order->update_status( 'processing', __( 'Manual changed to Processing because of Stripe Webhook race condition.', 'site' ), true );
}
add_action( 'wc_gateway_stripe_process_payment_intent_incomplete', 'site_change_order_to_processing' );

[dsmithweb-a8c]

7316233-zen

[WillBrubaker]

7379266-zen

[WillBrubaker]

https://wordpress.org/support/topic/orders-stuck-in-pending-payment-5

[a-danae]

Related GH issues:

• Potential duplicate webhook actions #2331
• Double Notes & Double Emails #2463

[a-danae]

Closing this issue as it should have been solved by #3331 and released in Stripe 8.6.1.