Multiple recurring orders get created if cron is badly configured

There is no code that protects against requeueing, which is probably what you're seeing.

The assumed flow is this:
1) Cron runs, queues a number of expired orders
2) Advanced Queue runs, processes the queued orders (closing and renewing them)
3) Cron runs, queues the next number of expired orders.

The question here is, why doesn't #2 happen as expected on your setup.
Are you running Advanced Queue as a daemon (drush/console), or via cron?

Unfortunately, I won't have time to investigate any additional bugs in the upcoming few weeks. You'll have to chase this one yourself for now.

What was the misconfiguration?

This matches what I'm seeing. I created a test subscription product with a 1 hour interval. If I run cron five times in a row, one right after the other, I'll get 5 charges one after the other. Each time I run cron, it should determine whether the subscription actually needs to be charged again. I should be able to run cron as many times as I want within the hour and only get the one valid charge.

I'm looking into this, too.

I changed to 1.x-dev and it seems to be honoring my billing schedule. I haven't diffed it to see what changed, but I *think* it may be working now.

Hi there, i'm getting this exact same issue as well. I set the billing schedule to daily, but every time cron runs it generates a new order and bills the charge to the user account.

This occurred when i was using the latest alphas/betas for each module, as well as the dev versions.

Currently I'm using this and it persists:

Drupal 8.5.3
commerce 2.6
commerce_license 2.x-dev
commerce_recurring 1.x-dev
recurring_period 1.x-dev
advancedqueue 1.0.0-beta3

Is it possible that something is misconfigured on my end? Would it help to provide my specific configuration?
What can I look in to to debug this or details can I get to help troubleshoot this?

franksj, are things still working for you? Could you share your specific versions and settings?

Thanks Almare. Do you use this on a production site and confirm what's working for your visitors?

Any advice on testing that things are working? I need to be sure subscribers won't be charged charged every time cron runs, but testing once a month isn't realistic either...

My products are monthly recurring and they're working correctly. As stated before, with hourly intervals, they will charge every time cron is run.

I think I was able to create a failing test. If we cron multiple times before the next billing cycle should kick off, then we create multiple orders.

In my case, I have a monthly and yearly subscription periods. If I set cron to run once a day, I'll create 30 orders for the single monthly subscription.

If someone uses ultimatecron, they can have the commerce recurring run on a faster schedule than the advanced queue.

The other thing I found (not directly related) is that if the order workflow gets changed from the default of recurring, then the transitioning to 'mark_paid' will fail and next time the order will be found as still in draft (but already paid) and will re-process as a new payment.

I've opened #2984223: RecurringOrderManager assumes workflow, resulting in failures and duplicate payments.

I wonder if we combine the fail test here w/ #2988470: Don't initiate payment of an already paid recurring order?

Here's the fixes from #2988470-2: Don't initiate payment of an already paid recurring order combined in to see if that fixes things. No interdiff, because it is simply the patch from over there merged in here.

Nope, we cannot just merge those two things. Here we "do the right thing" and check if we've already queued an order before re-queueing another.

Should the database query instead be a method available from advanced queue? Not to hold up this patch but maybe in the future.

The test could probably benefit from a comment making it a little clearer why we are running the cron twice back to back, otherwise someone might think it is an error and remove later. At a quick glance it looks like a typo

I would /much/ prefer an API to confirm I'm not able to queue something multiple times. Something in advanced queue. Ideally something added to BackendInterface that I can optionally call to confirm I'm just about to duplicate something I've already queued.

For now, here's an updated comment.

I came to realize that the previous tests weren't testing as much as I thought. Here's some improved tests.

Here, we test a couple scenarios where cron was disabled or the advancedqueue cron job is out of sync with commerce_recurring.

My project just got stung by the re-queueing problem. As a hotfix, the README should mention that cron timings have to be set up very carefully!

EDIT: nope, I wasn't thinking this through. closeOrder() will get called when a failed close is retried for dunning, so at that point, the order state will be needs_payment. Also, this bug also affects renewOrder()!

As a cleaner interim fix than #34, what having either RecurringOrderClose or RecurringOrderManager::closeOrder() simply ignore an order that is not in draft state?

Though I don't understand why this code allows for an order that's not in draft. When would that correctly happen?

  public function closeOrder(OrderInterface $order) {
    if ($order->getState()->value == 'draft') {
      $transition = $order->getState()->getWorkflow()->getTransition('place');
      $order->getState()->applyTransition($transition);
      $order->save();
    }

Reroll of the patch in #34, after removal of create() in #2952020: Remove static create() method from Cron class.

Here's a patch that relies on fixing Advanced Queue to support unique jobs: #2918866: Add support for unique jobs.

Tests won't pass because a) the AQ patch is needed, and b) #3017259: branch tests are failing

The changes I made to the test are much simpler than in the previous patch. I think all we need to test is that if our cron runs multiple times before AQ's cron runs, then no further jobs are queued.

In particular, some of the code in the tests in the previous patch confuses me:

1. +++ b/tests/src/Kernel/CronTest.php
   @@ -63,22 +64,103 @@ class CronTest extends RecurringKernelTestBase {
   +      // Construct Cron this way so we can properly inject our rewound time.
   +      $cron = new Cron($this->container->get('entity_type.manager'), $this->container->get('datetime.time'), $this->container->get('database'));
   +      $cron->run();
   +      advancedqueue_cron();
   

   I don't know why we need to recreated the Cron service each time. Existing test code just does:

       $this->rewindTime(strtotime('2017-02-24 19:00'));
       $this->container->get('commerce_recurring.cron')->run();
   

   so getting our Cron service from the container again seems to do the job. (I'm assuming the container destroys any services that rely on the time service when a new time service gets set on it, so that when you get our Cron again, it gets the time service injected fresh.)

2. +++ b/tests/src/Kernel/CronTest.php
   @@ -63,22 +64,103 @@ class CronTest extends RecurringKernelTestBase {
   +    // Now simulate that cron didn't run for a few days and is playing catchup.
   

   +++ b/tests/src/Kernel/CronTest.php
   @@ -63,22 +64,103 @@ class CronTest extends RecurringKernelTestBase {
   +    // Now simulate that commerce recurring and advancedqueue got out of sync.
   

I don't quite understand how these two scenarios work, or what they're testing.

At any rate, I don't think we need them: I think we can boil it down to ensuring that once a job it queued, another identical job won't get queued again.

Patch #40 no longer applies.

Marked #2998501: Duplicates commerce recurring ('commerce_recurring_order_close', 'commerce_recurring_order_renew') queues. as duplicate.

Re-roll of patch in #40

@Almare Still affects me with latest dev as recent as today.

Will try some of these patches and get involved.

I know there's some overlap in maintainership across the two modules required to get this fixed, but should just check the queue items before creating a matching one? The tricky part, I guess, is that advancedqueue can have different back-ends. Just a thought since this can cause big issues for people (I've been there)

Just want to add that this is still an issue.
Drupal 8.9
commerce_recurring 1.0@beta5
Anxiously waiting for this to stabilize. I will test out some of these patches.

The other patches are quite old.
2931290-48.patch is my solution for 1.0@beta5.

Updated patch.

I rerolled #43, the approach that depends on a patch to advanced queue.

Merged head into the issue fork, so the generated patch applies.

I have (for testing):

• Hourly recurring orders
• Crontab configured to run every 5 minutes with curl hitting the website cron hash URL
• Drupal auto-cron settings configured to run once an hour

Without patching, orders are being re-charged on every cron run, ie at 5 minute intervals.

The patch at #43 here, in conjunction with the unique jobs patch #58 against Advanced Queue at #2918866: Add support for unique jobs, does appear to fix the issue.

I had some confusion due to cron sleeping overnight (mac laptop), then the missed recurring orders being run one at a time on every subsequent 5-minute cron run until caught up, then hourly recurring orders resuming after that.

Related core issue could help here, it also needs opinions and ideas

I think I have run into this issue, where although cron was working on a correct schedule there was a job failing that was preventing the queue from clearing this then resulted in multiple charge attempts being made once the queue was unblocked.

Raising this to critical, we should never trigger multiple charges no matter the configuration.

I like the queue unique approach, but in addition, I think we should put some defensive code in the Recurring Order Manager that checks that it needs to charge before applying charges. This may be a separate issue, but I think billing multiple times is something that should be avoided at all costs.

RecurringOrderManager now has strong defenses against the same order being paid multiple times.

I wonder if RecurringOrderManager::createOrder() should refuse to create an order if that would create two orders for the same subscription with the same billing period. Simple to enforce.

But better done in a seperate issue.

@JeremyFrench @longwave can you confirm multiple orders were created for the same billing period, or just payment attempts?

Ultimately we need something to prevent multiple orders and double charges at any cost.

We need to capture these scenarios in automated tests of possible.

Also the merge request here will not get in unless the advanced queue patch gets in. It has no updates since almost a year ago.

@recidive it looks like both orders and payments. I've seen #2988470 which looks to stop multiple payment per order, but not multiple orders being created.

I think we can place the order (from Draft to Needs Payment). Anyway, we are doing the same from RecurringOrderManager::closeOrder()

    if ($order_state == 'draft') {
      $order->getState()->applyTransitionById('place');
      $order->save();
    }

Test are broken https://www.drupal.org/project/commerce_recurring/issues/2931290. I think the error with the transition is related to https://www.drupal.org/node/3220507

IMHO it makes sense to place the order (from Draft to Needs Payment) when we move the recurring order to the queue. The order will not be added to the queue again because we check the state from Drupal\commerce_recurring\Cron::enqueueOrders().

    $order_ids = $order_storage->getQuery()
      ->condition('type', 'recurring')
      ->condition('state', 'draft')
      ->condition('billing_period.ends', $this->time->getRequestTime(), '<=')
      ->accessCheck(FALSE)
      ->execute();

+1 for changing the state of the order during cron processing. I my case there are hundreds of orders every week and number of active subscriptions is also more than 500+. So in addition it will be nice to also limit the number of entities (orders in enqueueOrders and subscriptions in enqueueSubscriptions methods) to prevent OOM problem, because loading 500 orders can be a problem. Then if the state is changed and for example 50 orders were processed in first run, the next time the other 50 orders will be processed, because the first 50 already have another state and the query result will be different. Alternatively maybe some batch processing should be implemented here? Not sure that it is easily possible with cron operations.

>So in addition it will be nice to also limit the number of entities (orders in enqueueOrders and subscriptions in enqueueSubscriptions methods) to prevent OOM problem

I expanded #65 to limit the number of orders in `enqueueOrders` loaded at once to 50. I did not touch `enqueueSubscriptions` as subscriptions are not loaded, only their id's are returned from entity query.

I am not sure that batch fix in #70 will fix Timeout problems. I think it is better to limit on query level and just leave the other orders for another cron run. Something like this:

    $order_ids = $order_storage->getQuery()
      ->condition('type', 'recurring')
      ->condition('state', 'draft')
      ->condition('billing_period.ends', $this->time->getRequestTime(), '<=')
      ->accessCheck(FALSE)
      ->range(0, 50)
      ->execute();

No, this only prevents OOM issues. But can we then guarantee that all items are being queued? What if between 2 cron runs more than 50 items are being marked as `draft`?

I my case there are hundreds of orders every week and number of active subscriptions is also more than 500+. So in addition it will be nice to also limit the number of entities (orders in enqueueOrders and subscriptions in enqueueSubscriptions methods) to prevent OOM problem

This seems like a valid bug, but also scope creep. Please open a seperate issue for this and post a patch for that there, unless there's a reason it must be entangled with the issue being worked on here.

Issues get committed faster when they're kept tightly focused. Patches are vastly easier to review when small.

Lets make it Compatible with advancedqueue 1.0 also this related issue is now almost merged https://www.drupal.org/project/advancedqueue/issues/2918866

The changes will be UNIQUE_OVERWRITE_DUPLICATES instead of UNIQUE_OVERWRITE in
src/Plugin/AdvancedQueue/JobType/RecurringOrderRenew.php
src/Plugin/AdvancedQueue/JobType/RecurringOrderClose.php

hm I'd say "UNIQUE_DONT_OVERWRITE" as we don't want to queue duplicate jobs.

Actually in the latest MR on #2918866: Add support for unique jobs the annotation is allow_duplicates = false

Marking this as needs work, we should favor leveraging the Advancedqueue support for unique jobs instead...

Patch with the latest changes in MR
It should applied together with the patch for advancedqueue in #2918866: Add support for unique jobs

I've updated the patch to work with PHP 7.x

@phannphong: Could you upload an interdiff? Also FYI Commerce core itself has been requiring PHP8+ for a long time already.

So I think the Advancedqueue patch isn't going to fully help here...
I wish we had flags on the orders (but as actual fields that we could use to detect whether the recurring order was already queued for closing / renewal).

We could use the order's data for that, but then we'd still keep querying for the order IDS on each cron run, so this isn't ideal... Perhaps better than nothing... Or we could maybe use the "locked" flag for this?

We lock the order once it's queued for closing and renewal, the advantage of this solution is that we'd stop refreshing the order. Since it was supposedly queued for closing and renewal, it shouldn't be refreshed in subsequent order loads?

Could anyone confirm whether the attached patch helps? We'd then skip relying on Advancedqueue for that...

Hmm. We are allowing users with failing payments to put the recurring order back through checkout to update card details and pay it immediately.

We can unlock it before checkout but if the user abandons the process that might present a problem at next queueing query (every 15 minutes for us).

We can unlock it before checkout but if the user abandons the process that might present a problem at next queueing query (every 15 minutes for us).

A job will be requeued in this case yes, but I don't think that is a problem? Because the initial job would be marked as a failure, so technically a "new" job would be requeued and it'd also fail.

We have other options...

1. Set data flag on orders (e.g: $order->setData('commerce_recurring_order_close_queued', TRUE), the problem is orders would still be queried for "nothing" and would then be filtered out in the loop
2. Add a boolean field? Not sure how we'd name it? Just "queued"? "queued_for_closing"?

In any case, I don't think relying on Advancedqueue is the answer to this problem anymore. I'd favor an option allow ingus to filter out orders already queued at the query level (i.e. the extra boolean field).

Because the initial job would be marked as a failure

No it's still in dunning retry, ie needs_payment.

@John Pitcairn: I was referring to the Advancedqueue job state, not the order state? Not really sure what you're reporting here then? Because if the order has the "needs_payment" state, then it won't be re-queued the next time? Since we're only queuing draft orders?

OK so the state would prevent a requeue for us.

What else does a locked order imply though? What is that flag actually for?

I'd worry about further development deciding to assume only locked orders are in the queue. I also think there was some reason we don't save the order prior to putting it into checkout, which we'd have to to unlock it.

If the orders are already getting loaded and looped through then filtering on order data seems reasonable. If not and we are only querying for IDs then it probably should be a field for performance?

Sorry cross post ... wouldn't a boolean (or lock) to indicate queueing need an update hook to set that for already queued orders? Is that feasible?

wouldn't a boolean (or lock) to indicate queueing need an update hook to set that for already queued orders?

A locked order for example isn't refreshed on load. And if you're allowing your customers to resume checkout, you'd have to unlock them.
The data flag would work as I said, but we'd first query them for nothing.
Also, we could eventually skip saving the orders and just perform a direct update query to the DB (which would be more performant).

Using lock for this makes me nervous, as per #88. It seems like an insufficiently robust solution, potentially with unexpected consequences or interactions with custom code, and generally increasing the WTF of an already complex mechanism.

What about #62:

I wonder if RecurringOrderManager::createOrder() should refuse to create an order if that would create two orders for the same subscription with the same billing period. Simple to enforce.

Then we wouldn't be relying on AdvancedQueue. We could still use it's mechanism to avoid duplicates, but we'd not be reliant on it.

@jonathanshaw: How is that related? This is about ensuring we don't queue twice the job for renewing / closing the recurring order for the current billing cycle.
In other words, there is code invoked and multiple duplicate queue jobs created before we even reach RecurringOrderManager::createOrder().
Whatever fix we pick has to happen from the cron service and should prevent the jobs from being queued more than once. We can pick the data flags, this is the easiest solution but it means we can't filter out orders that shouldn't be renewed / closed at the query level, which would be better performance wise.

My thinking was the duplicate queue jobs would be harmless if RecurringOrderManager::createOrder() had the check I suggested.
I agree the order data seems like a natural place to store a flag.
Regarding performance, I wonder if commerce will one day change the data field to json and then we will be able to query it, something core is working on supporting #3343634: Add "json" as core data type to Schema and Database API

Probably silly idea: add a "queued" state?

Probably silly idea: add a "queued" state?

Not a good idea, the workflow can be changed.

Maybe we'll just go with the data flag... And in theory, if the queue is properly processed, we shouldn't be loading the same orders over and over...

How about we proceed with the unique queue support here, as it seems desirable even if not a complete solution. And do the data flag in a seperate issue?

The "unique" job support that was introduced isn't really helping us...
It wouldn't prevent a job to be queued if it failed...

See #3460188: In some cases the duplicated jobs are created.

Shall we, at the very least, also include a test to ensure no duplicates are created? I read carefully through this issue comments and it seems to me like #83 is the closest to the perfect test.

Performance issues are being discussed, especially in #69 - #73 with advice to work on them in a separate issue and not include in the scope here. I am a bit unsure about the latest proposal in #102 as it may close the door to making performance improvements in the future. This is a known limitation of the proposed approach as mentioned several times between #89 and #102.

This is a big pain, especially for merchants handling all their recurring payments on a frequent schedule (ex. hourly) or all at the same time (ex. first day of the month).

We could still go with the solution proposed in #102 if we plan to transition data property into json field as mentioned in #97 but given we're probably talking months if not years here I would rather opt for boolean field.

#83 is leveraging the Advancedqueue patch, but once again, I'm no longer convinced relying on Advancedqueue to fix duplicates is the answer.
Also, we're collecting subscriptions by passing the order to the RecurringOrderManager, so looks like we can't easily avoid loading the orders without rewriting the cron job completely.

Additionally, #102 is a pragmatic approach to an issue that was opened 7 years ago in an attempt to not drag this further and revisit this later since we can't really find an agreement on a "perfect" solution.

I think the boolean makes more sense too, but it seems there were reservations about this approach too.

I was only thinking about the subset of the test from #83, not the Unique AQ feature implementation. That being said I am not strongly opposed to implementing it as a safety measure, in addition to your proposed solution. It doesn't have to be done here and now though, could be done in a follow-up. That would address some concerns raised between #59 - #62

In regards to the boolean field, the only concern I see is in #93 and it's about setting a default value. I think this concern is valid whether we use lock, data, or boolean field. So if we don't change the query aspect of cron for now and leave it for a follow-up we may still be better, in a future-proof way, with boolean than data param. At least that's my opinion.

Anyways. combining #83 with #102 I was able to see that testing for running the cron twice (before running AQ cron) fixes the issue. I can still think of some scenarios where this can help - ex multiple instances of cron running at the same time but that's partially mitigated by cron lock.

Here are test results:

zaporylie@drupal-commerce-web:/var/www/html$ ./vendor/bin/phpunit  web/modules/contrib/commerce_recurring/tests/src/Kernel/CronTest.php 
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.8
Configuration: /var/www/html/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

.....F                                                              6 / 6 (100%)

Time: 00:10.476, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\commerce_recurring\Kernel\CronTest::testJobDuplicates
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'queued' => 2
+    'queued' => '4'
 )

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
/var/www/html/web/modules/contrib/commerce_recurring/tests/src/Kernel/CronTest.php:323
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

FAILURES!
Tests: 6, Assertions: 243, Failures: 1.

after the patch in #102

zaporylie@drupal-commerce-web:/var/www/html/web/modules/contrib/commerce_recurring$ cd -
/var/www/html
zaporylie@drupal-commerce-web:/var/www/html$ ./vendor/bin/phpunit  web/modules/contrib/commerce_recurring/tests/src/Kernel/CronTest.php 
PHPUnit 9.6.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.8
Configuration: /var/www/html/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

......                                                              6 / 6 (100%)

Time: 00:10.358, Memory: 4.00 MB

OK (6 tests, 243 assertions)

In regards to the boolean field, the only concern I see is in #93 and it's about setting a default value. I think this concern is valid whether we use lock, data, or boolean field.

Regardless of the picked solution, we can't properly backfill I mea, it is ok to me if a "queued" boolean or whatever name we picked has 0 even if the recurring order is placed, what really matters to me is fixing the actual issue and making sure we no longer queue the same recurring order for closing / renewal.

Do you think we should also change the method of loading the Order entity to OrderStorage::loadForUpdate to avoid race conditions? It kinda makes sense in this scenario and would further fail-safe proof the queuing mechanism. I am basing this on the fact that this issue was initially triggered by how differently people approach setting/splitting cron. That probably is a scope creep and a material for a followup though :)

Using loadForUpdate() makes sense IMO, I don't really consider this a scope creep. The method didn't previously exist, so why not using it to make this even more robust.

MR 31 is based on what we concluded so far:
- the commerce_recurring_queued boolean field is created - on install or via hook_update_N for existing websites. The field will be set to TRUE via cron
- Kernel test was introduced to ensure duplication issue is gone
- commerce requirement is bumped to account for orders now being loaded with OrderStorage::loadForUpdate to ensure the order is not being updated with 2 (or more) processes.

Some issues that we can work on in follow-up issues given we now have a param we can query against:
- adjust the order query to avoid orders that are already queued from being processed
- set a limit on the query to avoid processing more than reasonable number of orders at the same time

MR looks good to me, but let's see what other people think about it.

- adjust the order query to avoid orders that are already queued from being processed

I think we should do that already no? We can probably add a notExists()? Or maybe an OR condition group on notExists() + commerce_recurring_queued != 1.

I think:

- set a limit on the query to avoid processing more than reasonable number of orders at the same time

should be addressed in a followup issue yes.

Marking this as RTBC still, as we have tests and we've implemented the approach I favored (i.e. the boolean).
Perhaps we can update the test that was added to confirm the job types that were queued?

Basically, ensure that we have one "commerce_recurring_order_close" and another job for renewing the order "commerce_recurring_order_renew".

Perhaps we can update the test that was added to confirm the job types that were queued?

Added, although we already were testing this in CronTest::testActive(). Nevertheless, I think it makes sense to have an explicit test here and I added something I should have added before - asserting commerce_recurring_queued boolean value - 0 before cron runs, 1 after cron runs.

Went ahead and merged the request, let's move forward and address what's remaining as followup issues like suggested.

I added 2 child issues for followups.