drush update-db returns exit code 0 on failure

[pfrenssen]

When I am executing database updates and one of the updates fails, then the batch process is aborted, but the cache is still rebuilt and the command returns a success message and exit code 0.

$ ./vendor/bin/drush updb
 ---------------- ---------------- ------------- --------------------------------------------- 
  Module           Update ID        Type          Description                                  
 ---------------- ---------------- ------------- --------------------------------------------- 
  joinup_migrate   add_user_73932   post-update   Add the missed 'simatosc' user (uid 73932).  
  joinup_migrate   disable_update   post-update   Disable the Update module.                   
  joinup_migrate   more_redirects   post-update   Add more specific redirects.                 
 ---------------- ---------------- ------------- --------------------------------------------- 

 Do you wish to run the specified pending updates? (yes/no) [yes]:
 > yes

 [error]  Drupal\Core\Database\DatabaseNotFoundException: SQLSTATE[HY000] [1049] Unknown database 'db_name' in Drupal\Core\Database\Driver\mysql\Connection::open() (line 153 of core/lib/Drupal/Core/Database/Driver/mysql/Connection.php). 
 [ok] Post updating joinup_migrate
 [success] Cache rebuild complete.
 [success] Finished performing updates.
$ echo $?
0

[weitzman]

Thanks for the quick testing. I may have been a bit over-aggresive with my most recent refactor. If you have time to debug it further, I'd be appreciative. Otherwise, I'll investigate soon.

[claudiu-cristea]

Let's work here #3095. Unfortunately I uncovered also another bug that makes the batch process ongoing and repeating the same updates until it reaches timeout

[pfrenssen]

I had looked at it yesterday and the error message is present deep in the batch process code. It gets passed in $proc['output'] in _drush_backend_invoke() but I don't know how to bubble up that error all the way to the top. The output gets parsed by drush_backend_parse_output(), maybe we should loop over the content and call drush_set_error() if there is an error message in the content?

The recent refactor did not cause this regression by the way, it also happens in 9.0.0-beta7 before the refactor.

[weitzman]

I wonder if we could work on the lines below. basically, there was an error if $data['error_status] = 1. That would replace the check for drush_get_error() and $data['context'] (I think).

Hopefully thats enough to get someone started.

drush/includes/batch.inc

Lines 141 to 145 in fc39fc4

	while (!$finished) {
	$data = drush_invoke_process('@self', $command, $args);
	$finished = drush_get_error() || !$data || (isset($data['context']['drush_batch_process_finished']) && $data['context']['drush_batch_process_finished'] == TRUE);
	}

[gaelg]

Edit: I first put another example, but it's another bug: #3127

[jonhattan]

It is confusing because involves several levels of nesting.

First, updatedb sets a batch in UpdateDBCommands::updateBatch() and delegate processing to drush_backend_batch_process(); that spawns a new process.
The batch operations callback and the finish callback are UpdateDBCommands::updateDoOne(), and ::updateFinish respectively. It would be clear to move the batch callbacks to a separate class.

The batch process itself is succesful, this is why error_status is 0 and it's ok. What we want is to capture the batch results and identify what means a failure in the context of updatedb command. See #3188