Multiple recurring orders get created if cron is badly configured

Lets make it Compatible with advancedqueue 1.0 also this related issue is now almost merged https://www.drupal.org/project/advancedqueue/issues/2918866 ✨ Add support for unique jobs Fixed

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

Actually in the latest MR on ✨ Add support for unique jobs Fixed 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 ✨ Add support for unique jobs Fixed

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 if 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. Ideally the fix would allow us to filter out orders already queued at the query level.

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 queue jobs 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 ✨ Add "json" as core data type to Schema and Database API Needs work

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 🐛 In some cases the duplicated jobs are created Active .

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.

jsacksick → credited vetal4ik → .

I added 2 child issues for followups.

Automatically closed - issue fixed for 2 weeks with no activity.