laravel / cashier-stripe

Laravel Cashier provides an expressive, fluent interface to Stripe's subscription billing services.
https://laravel.com/docs/billing
MIT License
2.38k stars 673 forks source link

Order of webhooks from Stripe may not be consistent causing subscription to be in incorrect state #1201

Closed hailwood closed 3 years ago

hailwood commented 3 years ago

Description:

From the stripe documentation:

Order of events

Stripe does not guarantee delivery of events in the order in which they are generated. For example, creating a subscription might generate the following events:

  • customer.subscription.created
  • invoice.created
  • invoice.paid
  • charge.created (if there’s a charge)

Your endpoint should not expect delivery of these events in this order and should handle this accordingly. You can also use the API to fetch any missing objects (e.g., you can fetch the invoice, charge, and subscription objects using the information from invoice.paid if you happen to receive this event first).

This actually just bit me - image

With the end result being - the subscription was marked as "incomplete" in the database as that was the 'latest' status we'd received.

Steps To Reproduce:

Solution

Record the last "event time" from stripe which would be the created value image if the subscription/customer (based off the webhook) has a newer "stripe event time" then discard the event.

driesvints commented 3 years ago

Heya, thanks for reporting.

I'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, I'll try to reproduce the issue.

Thanks!

Muneeb-Ahmed-Khan commented 3 years ago

This also happened with me, it is not an issue with cashier. but it is stripe that send webbooks out of order. Can you please confirm whether you are using cashier webbook handler or spatie webhook handler ?

hailwood commented 3 years ago

Hi @driesvints,

As @Muneeb-Ahmed-Khan says it's not exactly a "cashier" issue so much as it is Stripe, and it's inconsistent - most of the time Stripe will send the webhooks in the right order, but sometimes they're not so it's not really something we can provide a reproduction for in code as the only way to reliably reproduce it is to resend the earlier webhook from the stripe dashboard if they have sent in the correct order.

@Muneeb-Ahmed-Khan yeah just Cashier's inbuilt webhooks.

driesvints commented 3 years ago

This is for a Stripe Checkout session, correct?

driesvints commented 3 years ago

So, what the best solution here would be is to store webhooks in a database table, then use a scheduled command through the console kernel to handle webhooks every minute or so (or even faster). In the scheduled command you can then order incoming webhooks by timestamp and handle them correctly.

But since this is such unlikely to happen and I don't want to overcomplicate Cashier's codebase I'm gonna table this for Cashier for now. You can solve it in your own app if you like in the way I described above.

Muneeb-Ahmed-Khan commented 3 years ago

This is for a Stripe Checkout session, correct?

Yes usually happens when subscription is created via stripe checkout.

@hailwood The best way to handle this is to run customer.subscription.created code inside customer.subscription.updated first, what that would ensure that if subscription is updated first it would created subscription and then run the update code and when the created webbook is called the subscription would already be there so it would go to waste. I am sharing picture from my code so you can copy it :)

image

In above picture there are two blocks in customer.subscription.updated file. The first block is the handleCustomerSubscriptionCreated function's code and it is inserted at top to handleCustomerSubscriptionUpdated function.

Both handleCustomerSubscriptionCreated and handleCustomerSubscriptionUpdated are located at vendor\laravel\cashier\src\Http\Controllers\WebhookController.php if you want to take a look at them. Both the blocks are minimized but they contain standard code of the cashier's function. The trick is to place created code first in updated.

This ensures that if updated is called first then it would create a subscription. then handle the update functionality. Please thoroughly test it before pushing it to production, this fixed the error for me but your case might conflict so test it first but i think it's the correct patch :)

driesvints commented 3 years ago

The best way to handle this is to run customer.subscription.created code inside customer.subscription.updated first

This isn't always wanted. What if an updated event arrives after a deleted event? The customer would be re-created.

driesvints commented 3 years ago

I'm gonna re-open this to look into at some point since more people are experiencing this. But we won't be actively developing this feature right now.

anderscodes commented 3 years ago

Just to note, we are experiencing this same issue, not very frequently but at least once a month with our subscriptions. I found a solution here that sounds like the best option, simply ignore the webhook and retrieve the most up to date subscription data from stripe. Let me know if I can be of any assistance here.

driesvints commented 3 years ago

Thanks @anderscodes. That would indeed be an interesting path to take. But I wonder if it conflicts with many webhooks sent around the same time for the same subscription?

rabol commented 3 years ago

just had the same issue, and the result is that the subscription is not even in my DB

In test mode, I tried to use different 'cards', to see how it was handled

and at the end I used a 3D security card: 4000 0025 0000 3155, payment accepted, subscription is in Stripe, but nothing locally

JhumanJ commented 3 years ago

Hey, same issue here. Did the dirtiest fix I've ever done:

<?php

namespace App\Http\Controllers\Webhook;

use Laravel\Cashier\Http\Controllers\WebhookController;

class StripeController extends WebhookController
{

    public function handleCustomerSubscriptionUpdated(array $payload)
    {
        sleep(1);
        return parent::handleCustomerSubscriptionUpdated($payload); 
    }

}
rabol commented 3 years ago

I think the first thing is to figure if it is a Stripe or Cashier issue

Over the last few days I have done some heavy testing on a local app and have been using the stripe cli

first off, the stripe cli is not 100% stable - it needs restart once in a while

Until now I have always seen a customer.subscription.created and then very quickly after customer.subscription.updated request coming in - could it be that the two calls block each other ?

2021-07-26 10:49:09   --> customer.subscription.created [evt_1JHPUmHKKxxxxxxxxxxxxx]
2021-07-26 10:49:09   --> invoice.paid [evt_1JHPUnHKKCzjeDxxxxxxxxxxx]
2021-07-26 10:49:09   --> customer.subscription.updated [evt_1JHPUmHKxxxxxxxxxxx]

as you can see the requests time more or less equal

hailwood commented 3 years ago

@rabol it's definitely a Stripe issue, as seen in the screenshot provided in the initial issue the updated event is sent before the created event (events are listed newest (top), oldest (bottom)). That was not using a cli.

However Stripe specifically states the order may not be consistent, so dealing with the ordering is a Cashier level issue.

driesvints commented 3 years ago

@hailwood this is a technicality that we should solve in Cashier, nothing that Stripe can help. What's basically happening is that Stripe operates so fast that its webhooks are sent out at almost the same time down to the second or even closer. The web server receiving the webhook and handling it cannot react fast enough to execute the code of the created webhook and therefor the data isn't available when the updated webhook is received asynchronously.

Note that the way webhooks (or any event based system for that matter) work is that it's expected that webhooks/events are sent as soon as they're handled. The asynchronously nature of them arriving at different times (Network/IO) or not being handled fast enough (system resources) are a technicality that the sender can't account for.

I actually very much like @JhumanJ's dead simple solution. That one second gap could be enough to let the created webhook arrive in time and be processed first. Can everyone here maybe try that one out on their apps to see if that solves your issues? We can then re-evaluate within a few weeks if it was enough to solve all issues here.

JhumanJ commented 3 years ago

I’ve had this issue since before cashier had official support for checkout (I manually added the PR code back then). I used the sleep hack in 2 apps in production, and never had the issue again!

driesvints commented 3 years ago

I've sent in a PR with your solution @JhumanJ. Thank you for suggesting that. We'll see how it goes.

https://github.com/laravel/cashier-stripe/pull/1227

macsj200 commented 3 years ago

@JhumanJ thank you for sharing this suggestion! I'm unfamiliar with PHP's approach to asynchronous execution, do you know if your proposed solution would work with something like NodeJS as well?

Node doesn't have a builtin sleep() function so I was thinking of building a solution on top of the Promise API + setTimeout().

hailwood commented 3 years ago

@macsj200 that's really going to depend on how the node project is setup. If it's similar to Cashier's setup then sure you could do something like

function(req, res) {
    setTimeout(() => handleUpdatedWebhook(req, res), 1000);
}
rabol commented 3 years ago

I don't like the 'blocking' of the request by using sleep()

I created 2 jobs, one for the creating and one for updating the subscription then I implemented my version of handleCustomerSubscriptionCreated and handleCustomerSubscriptionUpdated

In handleCustomerSubscriptionUpdated, i check if the subscription is created, if not, then I re-submit the job. I did create a retry count so that the job does not go into a endless loop

macsj200 commented 3 years ago

Just to note, we are experiencing this same issue, not very frequently but at least once a month with our subscriptions. I found a solution here that sounds like the best option, simply ignore the webhook and retrieve the most up to date subscription data from stripe. Let me know if I can be of any assistance here.

I ended up using this solution, thanks!

driesvints commented 3 years ago

Hey all, I've noticed that this was still happening and have tried an alternate solution. Let me know if that looks good: https://github.com/laravel/cashier-stripe/pull/1243

bharatdangar2 commented 2 years ago

At the time of checkout success page, can we sync the database entry with Stripe using:

optional($user->subscription('main'))->syncStripeStatus();

https://stackoverflow.com/a/69031618/2752871

Techworker commented 2 years ago

It's an old and solved task, yet the following might help someone.

Please make sure you don't run any (possibly slow) sync code in the Cashier handlers. I had an Observer running on Subscription::created which broke the idempotency checks/mechanism integrated in Cashier.

Nice self made race condition.

Conclusion: Only do stuff in Cashier-Model related Observers when you can offload them to a queue. If it's not possible, find some other solution. Never block Cashier Webhook handling.

SebastianBotez commented 1 year ago

After spending 12 hours debugging, I tested the solution with sleep(1), I tried to find a way to schedule them and so on and no fix. For everyone having this problem in stripe checkout, let me tell you what I encountered. Because of the webhooks order, sometimes my subscriptions were active, sometimes were past_due. This happened because stripe sends the customer.subscription.updated event 2 times when creating or updating a subscription.

  1. First event comes with a status of past_due
  2. Second event comes with a status of active

The problem is that sometimes the event with the final state as active is coming first and the past_due event comes last. The fix that I figured out is listening to this type of event and when it occurs, make a call in stripe to retrieve the subscription. By doing this you will not take in consideration the payload, and you are checking the real deal directly in stripe. You can do this like so in App\Listeners\StripeEventListener: *Note that my Billable model is not User, it's License but you get the point

if ($event->payload['type'] === 'customer.subscription.updated') {
   $localSubscription = Subscription::where('license_id', $event->payload['data']['object']['metadata']['license_id'])->first();
   $stripe = Cashier::stripe();
   $stripeSubscription = $stripe->subscriptions->retrieve($localSubscription->stripe_id);

   $localSubscription->update(['stripe_status' => $stripeSubscription->status]);
}