This issue is automatically created based on existing pull request: #41173: Fix infinite loop and inert batch size in the persistent expired-quote cleanup cron
Description (*)
The persistent_clear_expired cron job (Magento\Persistent\Observer\ClearExpiredCronJobObserver → Magento\Persistent\Model\CleanExpiredPersistentQuotes) cleans up expired persistent quotes in batches (default batchSize 500, set in Magento_Persistent/etc/di.xml). It has three related defects:
-
Unbounded loop. In processStoreQuotes(), $lastProcessedId is only advanced when quoteRepository->delete($quote) succeeds:
$this->quoteRepository->delete($quote);
$lastProcessedId = (int)$quote->getId();
getExpiredPersistentQuotes() selects main_table.entity_id > $lastProcessedId. If every delete in a batch throws (for example, a foreign key from another table blocking the delete), the cursor never advances, the next iteration re-selects the identical batch, and while (true) never terminates. This PR moves the assignment above the try, unconditional, so the cursor always advances past a row once it has been visited, whether or not the delete succeeded.
-
batchSize has no effect on the query. In ExpiredPersistentQuotesCollection::getExpiredPersistentQuotes(), setOrder() and setPageSize() are applied to $additionalQuotes, but that collection's Select is only cloned to build a sub-select of matching entity IDs. The collection actually returned and iterated ($quotes) receives only a WHERE ... IN (...) and neither an ORDER BY nor a LIMIT:
$quotes->getSelect()->where('main_table.entity_id IN (' . $selectQuoteIds . ')');Every batch therefore loads the store's entire expired-quote backlog in one query, regardless of $batchSize. This PR adds the order and limit to the collection that is returned:
$quotes->getSelect()
->where('main_table.entity_id IN (' . $selectQuoteIds . ')')
->order('main_table.entity_id ' . Select::SQL_ASC)
->limit($batchSize);(The LIMIT can't live inside the IN (...) union sub-select on MySQL, which is why it has to sit on the outer collection.)
These first two fixes must ship together, and the ORDER BY is not optional. With a real LIMIT and an unconditional cursor but no deterministic order, $lastProcessedId becomes the last row visited in an arbitrary batch rather than the true maximum, and rows between the two are silently skipped forever. Today that can't happen only because the missing LIMIT makes every batch the whole remaining set.
-
Log volume. The catch block logs (string)$e, which stringifies the whole exception including its stack trace, once per failed row. Changed to $e->getMessage().
Related Pull Requests
None.
Fixed Issues (if relevant)
N/A — no existing GitHub issue; reproducible directly on 2.4-develop (see manual testing scenarios).
Manual testing scenarios (*)
This reproduces with core code only; no third-party extension is required. The general condition is anything that makes QuoteRepository::delete() throw for the highest-id expired persistent quote in a store — here, a foreign key from a custom table referencing quote.entity_id with ON DELETE NO ACTION.
-
Enable persistence (Stores → Configuration → Customers → Persistent Shopping Cart) and create a quote that the cron will select as expired: is_persistent = 1, updated_at older than the configured persistence lifetime, and matching the customer_log conditions in getExpiredPersistentQuotes() (logged in and out, or logged in with an expired session). Note its entity_id, e.g. 123.
-
Create a table with a blocking foreign key to that quote, and a row referencing it:
CREATE TABLE quote_reference_test (
entity_id INT UNSIGNED NOT NULL AUTO_INCREMENT,
quote_id INT UNSIGNED NOT NULL,
PRIMARY KEY (entity_id),
CONSTRAINT FK_QUOTE_REFERENCE_TEST_QUOTE
FOREIGN KEY (quote_id) REFERENCES quote (entity_id)
ON DELETE NO ACTION ON UPDATE CASCADE
) ENGINE=InnoDB;
INSERT INTO quote_reference_test (quote_id) VALUES (123);
-
Run the cron job (bin/magento cron:run --group default, or invoke CleanExpiredPersistentQuotes::execute($websiteId) directly).
- Before:
quoteRepository->delete() throws an integrity-constraint violation for quote 123 on every pass. $lastProcessedId never advances past its value from before quote 123, so getExpiredPersistentQuotes() re-selects the same batch (quote 123 and everything after it) every iteration. The job never terminates — it has to be killed — and the log fills with a full stack trace per iteration.
- After:
$lastProcessedId advances past quote 123 regardless of the delete failing, the loop makes progress through the rest of the batch and terminates once each store's expired quotes are exhausted, quote 123 is the only one left behind (by design, since it's genuinely undeletable), and the log records one single-line message for it, not one stack trace per iteration.
-
To confirm the LIMIT/ORDER BY specifically: with several dozen expired persistent quotes present and a query logger (or Xdebug breakpoint) on getExpiredPersistentQuotes(), compare the emitted SQL before and after — before, there is no LIMIT clause and the full expired set is fetched in one query; after, the query includes ORDER BY main_table.entity_id ASC LIMIT <batchSize>.
-
Clean up: DROP TABLE quote_reference_test;
Questions or comments
ExpiredPersistentQuotesCollectionTest::testGetExpiredPersistentQuotes mocks the collection's Select object for the returned collection but doesn't stub where()/order()/limit() on it (harmless before this change, since the original code never used that return value). This PR adds the missing stubs so the existing test continues to pass against the new order()/limit() calls; happy to adjust if maintainers prefer a different test shape.
Contribution checklist (*)
This issue is automatically created based on existing pull request: #41173: Fix infinite loop and inert batch size in the persistent expired-quote cleanup cron
Description (*)
The
persistent_clear_expiredcron job (Magento\Persistent\Observer\ClearExpiredCronJobObserver→Magento\Persistent\Model\CleanExpiredPersistentQuotes) cleans up expired persistent quotes in batches (defaultbatchSize500, set inMagento_Persistent/etc/di.xml). It has three related defects:Unbounded loop. In
processStoreQuotes(),$lastProcessedIdis only advanced whenquoteRepository->delete($quote)succeeds:getExpiredPersistentQuotes()selectsmain_table.entity_id > $lastProcessedId. If every delete in a batch throws (for example, a foreign key from another table blocking the delete), the cursor never advances, the next iteration re-selects the identical batch, andwhile (true)never terminates. This PR moves the assignment above thetry, unconditional, so the cursor always advances past a row once it has been visited, whether or not the delete succeeded.batchSizehas no effect on the query. InExpiredPersistentQuotesCollection::getExpiredPersistentQuotes(),setOrder()andsetPageSize()are applied to$additionalQuotes, but that collection'sSelectis only cloned to build a sub-select of matching entity IDs. The collection actually returned and iterated ($quotes) receives only aWHERE ... IN (...)and neither anORDER BYnor aLIMIT:Every batch therefore loads the store's entire expired-quote backlog in one query, regardless of
$batchSize. This PR adds the order and limit to the collection that is returned:(The
LIMITcan't live inside theIN (...)union sub-select on MySQL, which is why it has to sit on the outer collection.)These first two fixes must ship together, and the
ORDER BYis not optional. With a realLIMITand an unconditional cursor but no deterministic order,$lastProcessedIdbecomes the last row visited in an arbitrary batch rather than the true maximum, and rows between the two are silently skipped forever. Today that can't happen only because the missingLIMITmakes every batch the whole remaining set.Log volume. The catch block logs
(string)$e, which stringifies the whole exception including its stack trace, once per failed row. Changed to$e->getMessage().Related Pull Requests
None.
Fixed Issues (if relevant)
N/A — no existing GitHub issue; reproducible directly on
2.4-develop(see manual testing scenarios).Manual testing scenarios (*)
This reproduces with core code only; no third-party extension is required. The general condition is anything that makes
QuoteRepository::delete()throw for the highest-id expired persistent quote in a store — here, a foreign key from a custom table referencingquote.entity_idwithON DELETE NO ACTION.Enable persistence (Stores → Configuration → Customers → Persistent Shopping Cart) and create a quote that the cron will select as expired:
is_persistent = 1,updated_atolder than the configured persistence lifetime, and matching thecustomer_logconditions ingetExpiredPersistentQuotes()(logged in and out, or logged in with an expired session). Note itsentity_id, e.g.123.Create a table with a blocking foreign key to that quote, and a row referencing it:
Run the cron job (
bin/magento cron:run --group default, or invokeCleanExpiredPersistentQuotes::execute($websiteId)directly).quoteRepository->delete()throws an integrity-constraint violation for quote 123 on every pass.$lastProcessedIdnever advances past its value from before quote 123, sogetExpiredPersistentQuotes()re-selects the same batch (quote 123 and everything after it) every iteration. The job never terminates — it has to be killed — and the log fills with a full stack trace per iteration.$lastProcessedIdadvances past quote 123 regardless of the delete failing, the loop makes progress through the rest of the batch and terminates once each store's expired quotes are exhausted, quote 123 is the only one left behind (by design, since it's genuinely undeletable), and the log records one single-line message for it, not one stack trace per iteration.To confirm the
LIMIT/ORDER BYspecifically: with several dozen expired persistent quotes present and a query logger (or Xdebug breakpoint) ongetExpiredPersistentQuotes(), compare the emitted SQL before and after — before, there is noLIMITclause and the full expired set is fetched in one query; after, the query includesORDER BY main_table.entity_id ASC LIMIT <batchSize>.Clean up:
DROP TABLE quote_reference_test;Questions or comments
ExpiredPersistentQuotesCollectionTest::testGetExpiredPersistentQuotesmocks the collection'sSelectobject for the returned collection but doesn't stubwhere()/order()/limit()on it (harmless before this change, since the original code never used that return value). This PR adds the missing stubs so the existing test continues to pass against the neworder()/limit()calls; happy to adjust if maintainers prefer a different test shape.Contribution checklist (*)
CleanExpiredPersistentQuotesTestandExpiredPersistentQuotesCollectionTestcover the changed methods; the latter is updated for the neworder()/limit()calls.php -lon all changed files and by manual trace of the existing unit tests against the diff.