Fix batch process race conditions by making ‘bid (batch id)’ auto-increment

I am trying to work out if the ID for non-progressive batches is completely redundant, or if it's 'used' and doesn't matter what it is, or if it matters somehow.

This logic in batch_set() does seem to rely on the ID regardless of if the batch is progressive or not.

    // Add the set to the batch.
    if (empty($batch['id'])) {
      // The batch is not running yet. Simply add the new set.
      $batch['sets'][] = $batch_set;
    }
    else {
      // The set is being added while the batch is running.
      _batch_append_set($batch, $batch_set);
    }

However, the ID is completely ignored in batch_progress() for non-progressive batches, so perhaps this is fine?

But... if it's fine for non-progressive batches to not have an ID at all, then can we get rid of this from the patch?

+++ b/core/includes/form.inc
@@ -897,9 +898,29 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
+    }
+    else {
+      $batch['id'] = rand();
+    }

Failures in #6

I think the failures in #6 are due to this code in form.inc

function _batch_populate_queue(&$batch, $set_id) {
  $batch_set = &$batch['sets'][$set_id];

  if (isset($batch_set['operations'])) {
    $batch_set += [
      'queue' => [
        'name' => 'drupal_batch:' . $batch['id'] . ':' . $set_id,
        'class' => $batch['progressive'] ? 'Drupal\Core\Queue\Batch' : 'Drupal\Core\Queue\BatchMemory',
      ],
    ];

i.e. the $batch['id'] is non existing at that point for non-progressive batches. Maybe 'drupal_batch:' . $batch['id'] ?? '' . ':' . $set_id, could just fix that?

Great idea splitting this out from the other issue.

1. +++ b/core/includes/form.inc
   @@ -897,9 +898,26 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
   +    if ($batch['progressive']) {
   +      try {
   +        $batch['id'] = \Drupal::service('batch.storage')->getId();
   +      }
   

   Are we concerned about the impact of not assigning an ID to non-progressive batches and whether or not code in custom or contrib might depending on this being set. Maybe it should be:
   $batch['id'] = $batch['progressive'] ? \Drupal::service('batch.storage')->getId() : '';

2. +++ b/core/lib/Drupal/Core/Batch/BatchStorage.php
   @@ -123,10 +123,36 @@ public function cleanup() {
   +  /**
   +   * Saves a batch.
   +   *
   +   * @param array $batch
   +   *   The array representing the batch to create.
   +   */
   +  protected function doCreate(array $batch) {
   +    $this->connection->update('batch')
   +      ->fields([
   +        'token' => $this->csrfToken->get($batch['id']),
   +        'batch' => serialize($batch),
   +      ])
   +      ->condition('bid', $batch['id'])
   +      ->execute();
   +  }
   

   A function called doCreate doing a database update seems really problematic.

3. +++ b/core/lib/Drupal/Core/Batch/BatchStorage.php
   @@ -138,23 +164,22 @@ public function create(array $batch) {
   +  protected function doGetId(): int {
   +    return $this->connection->insert('batch')
          ->fields([
   -        'bid' => $batch['id'],
            'timestamp' => REQUEST_TIME,
   -        'token' => $this->csrfToken->get($batch['id']),
   -        'batch' => serialize($batch),
   +        'token' => '',
   +        'batch' => NULL,
          ])
          ->execute();
      }
   

   And a function called doGet doing an insert is also interesting.

Maybe

doGetId() => doInsert()
doCreate() => doUpdate()

so the do*() reflect the actual database operations?

The following PHPStan baseline entry should be adjusted to count: 1

		-
			message: "#^Call to deprecated constant REQUEST_TIME\\: Deprecated in drupal\\:8\\.3\\.0 and is removed from drupal\\:11\\.0\\.0\\. Use \\\\Drupal\\:\\:time\\(\\)\\-\\>getRequestTime\\(\\); $#"
			count: 2
			path: lib/Drupal/Core/Batch/BatchStorage.php

Maybe

doGetId() => doInsert()
doCreate() => doUpdate()

so the do*() reflect the actual database operations?

It is a solution we can take. For me the methods doCreate() and doGetId() are 2 helper methods and their name should reflect that. I do get the point that we would create a doCreate() method that does an update query. Naming things is not always easy in IT.

Naming things is not always easy in IT.

https://dev.to/thinkster/the-hardest-thing-in-programming-384g

😊

@gidarai I will roll the next patch ok?

We need a test for the function system_update_10101().

Sure we need a test there. Can I get a review before getting into that?

Added the update path test.

Upgrade status looks good. As an assertion before the run, in this case try/catch which was a great solution btw.

Thanks - the PGSQL failure is real though, NW to get it sorted. On it.

The PgSql failure will be fixed by #3028706: New serial columns do not own sequences, so better wait on that one.

I was wrong in #28, #3028706: New serial columns do not own sequences is not fixing the pgsql issue, we still need to understand why after the update inserting to the table we got all the lastIDs to 0.

For the change to the users table with added something special for PostgreSQL databases. Maybe we should do the same here.

  // The new PostgreSQL sequence for the uid field needs to start with the last
  // used user ID + 1 and the sequence must be owned by uid field.
  // @todo https://drupal.org/i/3028706 implement a generic fix.
  if ($connection->driver() == 'pgsql') {
    $maximum_uid = $connection->query('SELECT MAX([uid]) FROM {users}')->fetchField();
    $seq = $connection->makeSequenceName('users', 'uid');
    $connection->query("ALTER SEQUENCE " . $seq . " RESTART WITH " . ($maximum_uid + 1) . " OWNED BY {users}.uid");
  }

From : https://git.drupalcode.org/project/drupal/-/blob/9.4.8/core/modules/user...

Thanks @daffie.

No I actually found out the problem to be a trickier one - in the update path test the call to changeField() is done by the SUT, and the test database connection then gets bogged, its lastId() method does not understand that the field is now a serial so it keeps returning 0. Must be some static caching somewhere.

#30 will be solved by #3028706: New serial columns do not own sequences in a consistent way, and it's not strictly necessary here (although I'd say it'd better to get that in before this).

Working on an update to the MR.

Let's see the level of bot happiness now.

The MR looks good.

I create a couple of remarks on the MR.

All my points have been addressed.
Testing has been added.
The MR is for me RTBC.
The CR needs to be updated. We are not using the keyvalue store as a replacement.

Updated CR. Now we probably will need another one for #2665216: Deprecate Drupal\Core\Database\Connection::nextId() and the {sequences} table and schema, but one step at a time.

All my points have been addressed.
Testing has been added.
The IS and the CR are in order.
For me it is RTBC.

The MR from @mondrake is the one to merge.

rebased

I was reading the change record and I don't understand why it discusses a postponed issue. Oh, I see this is one change record for two issues. That is not a good idea because the two issue may get committed to different versions.

The CR should also be in clear English, see Technical writing (English) and Write a change record for a Drupal core issue.

I am tagging this for change record updates.

I have updated the CR.
Back to RTBC.

I checked the change record. I think it's fine for it to reference the postponed issue - we explicitly split this issue out of that one, but they're closely related and it's useful context.

Committed/pushed to 10.1.x, thanks!

I had to regenerate the phpstan baseline on commit

FYI I've been having issues with running updates on 10.1

>  [error]  TypeError: ArrayObject::__construct(): Argument #1 ($array) must be of type array, bool given in ArrayObject->__construct() (line 15 of /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.php) #0 /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.php(15): ArrayObject->__construct(false)
> #1 /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/UnstructuredListData.php(21): Consolidation\OutputFormatters\StructuredData\AbstractListData->__construct(false)
> #2 /var/www/html/vendor/drush/drush/src/Commands/core/UpdateDBCommands.php(132): Consolidation\OutputFormatters\StructuredData\UnstructuredListData->__construct(false)
> #3 [internal function]: Drush\Commands\core\UpdateDBCommands->process('3', Array)
> #4 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
> #5 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #6 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #7 /var/www/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(390): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #8 /var/www/html/web/vendor/symfony/console/Command/Command.php(312): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #9 /var/www/html/web/vendor/symfony/console/Application.php(1040): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #10 /var/www/html/web/vendor/symfony/console/Application.php(314): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #11 /var/www/html/web/vendor/symfony/console/Application.php(168): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #12 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(124): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #13 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(51): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
> #14 /var/www/html/vendor/drush/drush/drush.php(77): Drush\Runtime\Runtime->run(Array)
> #15 /var/www/html/vendor/drush/drush/drush(4): require('/var/www/html/v...')
> #16 /var/www/html/vendor/bin/drush(120): include('/var/www/html/v...')
> #17 {main}. 
> TypeError: ArrayObject::__construct(): Argument #1 ($array) must be of type array, bool given in /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.php on line 15 #0 /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.php(15): ArrayObject->__construct(false)
> #1 /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/UnstructuredListData.php(21): Consolidation\OutputFormatters\StructuredData\AbstractListData->__construct(false)
> #2 /var/www/html/vendor/drush/drush/src/Commands/core/UpdateDBCommands.php(132): Consolidation\OutputFormatters\StructuredData\UnstructuredListData->__construct(false)
> #3 [internal function]: Drush\Commands\core\UpdateDBCommands->process('3', Array)
> #4 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
> #5 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #6 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #7 /var/www/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(390): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #8 /var/www/html/web/vendor/symfony/console/Command/Command.php(312): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #9 /var/www/html/web/vendor/symfony/console/Application.php(1040): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #10 /var/www/html/web/vendor/symfony/console/Application.php(314): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #11 /var/www/html/web/vendor/symfony/console/Application.php(168): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #12 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(124): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #13 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(51): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
> #14 /var/www/html/vendor/drush/drush/drush.php(77): Drush\Runtime\Runtime->run(Array)
> #15 /var/www/html/vendor/drush/drush/drush(4): require('/var/www/html/v...')
> #16 /var/www/html/vendor/bin/drush(120): include('/var/www/html/v...')
> #17 {main}
>  [warning] Drush command terminated abnormally.

In ProcessBase.php line 171:
                                                                                                                                                                                                                                       
  Unable to decode output into JSON: Syntax error                                                                                                                                                                                      
                                                                                                                                                                                                                                       
  TypeError: ArrayObject::__construct(): Argument #1 ($array) must be of type array, bool given in ArrayObject->__construct() (line 15 of /var/www/html/vendor/consolidation/output-formatters/src/StructuredData/AbstractListData.ph  
  p).                                                   

Reversing this patch I was able to run the updates.

Re-opening this but not rolling back just yet since it's only in 10.1

I think we need to open a ticket against drush, and depending what happens there, maybe it's an easy fix in drush, or if not we might need to roll back until the drush ticket is ready.

Filed issue for drush as it needs BC too https://github.com/drush-ops/drush/issues/5494

So reverted the changes in core/lib/Drupal/Core/Batch/BatchStorage.php and things appear to be working again.

I also ran into the issue but with a different error. @catch asked me to post the output in here as well.

i am able to reproduce the error consistently. i set up a new install of 10.1.x-dev on ddev with php 8.2 and mariadb 10.5. after that i apply the patch from #2492171-386: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc). after that `drush updatedb`is executed as shown in the gist (created one since the output is too long for the issue here):

https://gist.github.com/rpkoller/ec980e0bd95d071102bf006a7a19f6d3

Since this can be reproduced consistently by @rkoller, I've gone ahead and reverted this. Batch has logic for ::ensureTableExists() etc., but either the patch is breaking that, or it's leading to a situation where existing breakage is being exposed.

per @catch's suggestion i've tested the following now:

1. install 10.1.x-dev
2. went to /admin/modules/update and clicked check manually
3. then went into phpmyadmin and checked if the batch table was in place and it was
4. applied the patch from #2492171-368: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc)
5. went to update.php and ran the update => it ran through without any errors

i did an extra test while writing up this comment

1. install 10.1.x-dev
2. go into phpadmin -> the batch table is missing

It means insert into batch skipping ensureTable

No drush expert - but if I understand correctly batches in drush are run via it's own batch.inc code, i.e. code in Drupal's form.inc is not executed when the update runs via drush. That would explain why the table doesn't exist - in the new code that is ensured in BatchStorage::getid() which drush doesn't execute.

IMHO it would be easier to fix drush if this were in.

EDIT: commented upstream too https://github.com/drush-ops/drush/issues/5494#issuecomment-1484207843

I don't think this is a drush problem, or not exclusively - if you do a clean install, add an update (via a patch that adds one or manually), then go straight to update.php before executing any other batches, you'll hit this.

ah right - quite an edge case though. We can fix here but the drush problem will remain, more likely to happen when drush is used in test scripts.

second thinking: if we install anew a code base with this issue included, getId() should be ensuring the table, no? so what’s failing?

even to start update drush will need to get batch::ID using old method and probably will fail after running update

For me the drush fix is quite clear, I will try to post a PR on github. To test it, though, it would be better to have this in Drupal core. What's still unclear to me is how comes you can end up with a new install (10.1.x including the patch here), call update.php and fail because the table is missing.

Filed https://github.com/drush-ops/drush/pull/5509 for downstream drush

https://github.com/drush-ops/drush/pull/5509 downstream is making progress, but testing it without the change here in the codebase would be somehow missing sense.

Can I suggest to get this back into core, so we can test drush and address the edge case of #58 separately?

https://github.com/drush-ops/drush/pull/5509 was merged downstream.

At least the new auto-increment should get initial value from current `sequences` value to allow finish batches already running/created

#66 I do not see why that would be needed. AFAICS the important thing is to get a unique ID for the new ones.

The fix for Drush has landed. See: https://github.com/drush-ops/drush/pull/5509

Looks like the fix hasn't landed in drush 11 yet?

Maybe update can use "known ensureTable()" from core and fix it as well if no override applied and table exists?

#69 no, because there's nothing to fix there at the moment while this is not committed. So we're in a catch 22 I'm afraid...

OK I think the answer here is to commit this to 11.x (10.2.x) - that will give it time to percolate through drush versions and updates, and more time to flush out any remaining issues too. I fixed the deprecation version on commit.

Committed/pushed to 11.x, thanks!

Added the Drush PR fix in the IS, for the record.

Coming here via https://github.com/drush-ops/drush/issues/5797.

The drush PR that was merged seems incorrect and is not consistent with what core is doing.

The commit here has a specific fallback logic for the update path that does a manual INSERT query. The merge request against drush does not have that, it falls back to \Drupal::database()->nextId(); but does not actually insert the row, so drush never actually persists the batch.

I've proposed a new PR that implements the same logic as core does here.

Just for the record, the PR from Berdir was merged to Drush 12.4.3 https://github.com/drush-ops/drush/releases/tag/12.4.3 and I can confirm that it works with Drupal 10.2.0-rc1. In my use case module on installation does batch processing. Now it works fine with Drush 12.4.3.