If an error occurs during an attended process, delete the stage on behalf of the user

I think the problem that we want to solve in this issue is that if there are any errors during PreCreateEvent/PostCreateEvent or PreRequireEvent/PostRequireEvent then we want to destroy the stage in UpdateStage for the user. If that's right then why shouldn’t we do it in StageBase so that the same thing will not have to be repeated for Project Browser @tedbow, @phenaproxima? Actually @Wim Leers and I were discussing about this issue and he suggested this.

@Wim Leers I don't think I had any reason for implementing it this way, I just followed the same way I implemented this in #3277034: Unhandled Composer Stager exceptions leave the update process in an indeterminate state in which @tedbow while reviewing initially requested changes in UpdateStage( Updater) https://git.drupalcode.org/project/automatic_updates/-/merge_requests/63... but later in https://git.drupalcode.org/project/automatic_updates/-/merge_requests/74... he suggested we should do this in separate issue and that is the current issue.

Discussed with @tedbow and @Wim Leers and decided that we should destroy the stage in the StageBase only. I reverted the changes in UpdateStage and added the logic to destroy the stage in StageBase itself.

\Drupal\Tests\package_manager\Kernel\LockFileValidatorTest::testLockFileChanged and \Drupal\Tests\package_manager\Kernel\LockFileValidatorTest::testLockFileDeleted is on Pre-require event while asserting status check results because now we are deleting stage on error during Pre-require event in StageBase see

    $on_error = $destroy_on_failure ? [$this, 'destroy'] : NULL;

    $this->dispatch(new PreRequireEvent($this, $runtime, $dev), $on_error);

and we pass the same stage while checking the status check results in test see

$stage = $this->assertResults([$result], $event_class);
// A status check should agree that there is an error here.
$this->assertStatusCheckResults([$result], $stage);

but now that stage is not available the error is not added. I think this behaviour is right as already during pre-require event we have shown the error and destroy the stage so we should have error added during status-check event after that.

So for now I think instead of

$stage = $this->assertResults([$result], $event_class);
// A status check should agree that there is an error here.
$this->assertStatusCheckResults([$result], $stage);

we should do

    $stage = $this->assertResults([$result], $event_class);
    $this->assertStatusCheckResults($event_class === PreRequireEvent::class
      // A status check should not surface this error, because the stage has
      // automatically been destroyed.
      ? []
      // A status check should agree that there is an error here.
      : [$result],
      $stage);

because this method test pre-apply event also during which we don't destroy the stage and we should see error during status check event.

Also while debugging this I found some inconsistencies handling composer stager exceptions in \Drupal\package_manager\StageBase::require and \Drupal\package_manager\StageBase::apply i.e. require() does

      try {
        $this->stager->stage($command, $active_dir, $stage_dir, NULL, $timeout);
      }
      catch (\Throwable $e) {
        // If the caught exception isn't InvalidArgumentException or
        // PreconditionException, a Composer operation was actually attempted,
        // and failed. The stage should therefore be destroyed, because it's in
        // an indeterminate and possibly unrecoverable state.
        assert(!$e instanceof InvalidArgumentException, sprintf("%s is calling Composer Stager with invalid arguments: this indicates a bug in your stage. The message was: %s.", get_class(), $e->getMessage()));
        assert(!$e instanceof InvalidArgumentException, sprintf("A precondition wasn't fulfilled while calling Composer Stager in %s: this indicates a bug in your stage. The message was: %s.", get_class(), $e->getMessage()));
        if ($destroy_on_failure) {
          $this->destroy();
        }
        elseif (!$e instanceof InvalidArgumentException && !$e instanceof PreconditionException) {
          $this->destroy();
        }
        $this->rethrowAsStageException($e);
      }
    };

while apply() does

    try {
      $this->committer->commit($stage_dir, $active_dir, $paths_to_exclude, NULL, $timeout);
    }
    catch (InvalidArgumentException | PreconditionException $e) {
      // The commit operation has not started yet, so we can clear the failure
      // marker.
      $this->failureMarker->clear();
      $this->rethrowAsStageException($e);
    }
    catch (\Throwable $throwable) {
      // The commit operation may have failed midway through, and the site code
      // is in an indeterminate state. Release the flag which says we're still
      // applying, because in this situation, the site owner should probably
      // restore everything from a backup.
      $this->setNotApplying()();
      // Update the marker file with the information from the throwable.
      $this->failureMarker->write($this, $this->getFailureMarkerMessage(), $throwable);
      throw new ApplyFailedException($this, $this->failureMarker->getMessage(), $throwable->getCode(), $throwable);
    }

. The inconsistency is that the require() catches all throwable exceptions and then catch it checks for the two classes( InvalidArgumentException and PreconditionException) while in apply() there are two catches one for the two classes and other for the all throwable exceptions. So tagging this issue as Needs architectural review.

I think we can make require() and apply() consistent in this issue. So, updating the issue summary.

Created #3371076: [PP-1] There is an error shown along with 'Update complete!' on update page if there is an error. as per discussion with @tedbow during scrum.

I think we still need to destroy stage in \Drupal\automatic_updates\BatchProcessor::stage() for the case if $this->stager->stage($command, $active_dir, $stage_dir, NULL, $timeout); fails in \Drupal\package_manager\StageBase::require() with InvalidArgumentException or PreconditionException where we don't destroy stage.

Created #3372940: Tell the user to try resolve the issue if possible before continuing if there is an error in PreApply as per #21

testUpdateStoppedByEventSubscriber also showed me that if an error happens in PreApply the user is also redirected back to UpdateReady but they have the option to "Continue" or "Cancel Update". In this case I think it is reasonable not to delete the stage automatically because the validation error might be something they could fix. But we may want to look into updating \Drupal\automatic_updates\BatchProcessor::finishCommit to tell the user they should try to resolve this issue if possible before continuing. That can be another issue.

The test is already there i.e. \Drupal\Tests\automatic_updates\Functional\UpdateErrorTest::testUpdateStoppedByEventSubscriber which is updated accordingly.

Updated the issue summary.