Cleanup of batches is never performed, add batch cleanup after execution of batch

+1, I just found batches from 2017 in my db.

I also think that there should be warning for every batch job running in browser not to close tab/window. I mean it's same as upgrading your phone or computer and it also says not to shut down the device before it's finished. I think it's even possible to alert user with js and block closing the window until user agrees. Sometimes you might forget a long batch job and press update button on your computer which wants to then close browser.

Tagging "Bug Smash Initiative"

Setting to NW for the reroll.

Actually do it this time.

Re-roll patch created, Please review.

Re-roll patch #27 with the branch 9.2.x.

Fixed "Custom Command Fail" issues for 9.3.x.

Removed out of date "Needs reroll" tag

Doesn't apply. The following is from a light read of the changes.

1. +++ b/core/lib/Drupal/Core/Queue/BatchQueueInterface.php
   @@ -0,0 +1,18 @@
   + * Batch API queue which is not part of QueueInterface.
   

   This is confusing to me. It is not part of QueueInterface but it extends from QueueInterface.

2. +++ b/core/modules/system/tests/modules/error_service_test/src/Logger/TestLog.php
   @@ -11,8 +11,16 @@
   +   * An array of arrays with three items: "level", "context" and "message".
   

   What are items? Are they keys? Maybe one summary line and then another paragraph to add more description?

3. +++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
   @@ -87,4 +90,38 @@ public function testBatchProgressMessages() {
   +   * Tests that batch queue was wrongly implemented.
   

   Probably should be 'Tests an incorrectly implemented batch queue.

   Add another comment here or in the code to explain what 'incorrectly implemented' means.

4. +++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
   @@ -87,4 +90,38 @@ public function testBatchProgressMessages() {
   +    $message = sprintf('Batch queue "%s" is not implements "%s" interface.', DatabaseQueue::class, BatchQueueInterface::class);
   

   s/is not implements/does not implement/

5. +++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
   @@ -87,4 +90,38 @@ public function testBatchProgressMessages() {
   +    foreach ($logger::$entries as $i => $entry) {
   

   $i is not used.

6. +++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
   @@ -87,4 +90,38 @@ public function testBatchProgressMessages() {
   +    $this->assertTrue($found, sprintf('Log entry "%s" about wrong queue implementation was found.', $message));
   

   Assertions shouldn't be using the message field. Instead use a command to explain what $found means.

Re-rolled the patch in #31 and addressed the changes specified in #33.4 and a part of #33.3.

I think the change mentioned in #33.5 is not needed as $i is being used in the following line:

+++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
@@ -87,4 +90,38 @@ public function testBatchProgressMessages() {
+        unset($logger::$entries[$i]);

Retaining status as "Needs work" to address the rest of the changes i.e., #33.1, #33.2, #33.3 (second part of it) and #33.6.

Thank you!

Reroll patch from the latest 9.4.x and another one for the 9.3.x.

This issue mixes 2 things, can we split up the BatchQueueInterface to a separate issue, since that's a feature request. And use this issue to simply fix the cleanup that was forgotten in #1998250: Move batch to pluggable storage?

Link to the commit where the cleanup was forgotten: https://git.drupalcode.org/project/drupal/-/commit/a2f15f28ebca3e589df6d...

If I'm right, we just need to add \Drupal::service('batch.storage')->cleanup(); where the db_delete query was removed

@daften : Agreed, created a new follow-up issue #3333713: add BatchQueueInterface for adding BatchQueueInterface.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This could use test coverage

Rerolled patch from #36 against 9.5.4. No other changes.

Patch rerolled for the 10.2.x branch

Here's another consequence of this bug. We discovered that we have 16000+ old batch jobs left in the queue table. _drush_batch_worker() invokes Drupal\Core\Queue\DataBaseQueue::claimItem(), which generates a query similar to this:

SELECT * FROM queue q WHERE name='drupal_batch:123301:0' ORDER BY item_id ASC LIMIT 0, 1;

This query now takes about 6 seconds to execute!

Now, during an upgrade, drush commands such as locale:check and locale:upgrade takes forever to execute because of this (they repeatedly add new batch jobs). Even enabling a new module will have this effect, since it will invoke the locale module to look for translations.

Rebased against head and moved changes to OOPS hooks.

MR and patch seem very different, was some stuff missed?

@smustgrave issue is rescoped check #40, patch rerolls are not correct. MR 3248 is relevant.

Then the needs tests tag still seems relevant hear so moving back to NW thanks

Leaving here a simpler patch to cleanup batches on system_cron()

Re-rolled #48 against 11.3.

Fixed the patch.