Issue #3277034 by kunal.sachdev, phenaproxima, tedbow, Wim Leers: Unhandled...

Issue #3277034 by kunal.sachdev, phenaproxima, tedbow, Wim Leers: Unhandled Composer Stager exceptions leave the update process in an indeterminate state

package_manager/src/Stage.php

@@ -256,12 +256,20 @@ class Stage implements LoggerAwareInterface {
    *   A list of paths that Composer Stager should ignore when creating the
    *   stage directory and applying staged changes to the active directory.
    *
+   * @throws \Drupal\package_manager\Exception\StageException
+   *   Thrown if an exception occurs while collecting ignored paths.
+   *
    * @see ::create()
    * @see ::apply()
    */
   protected function getIgnoredPaths(): array {
     $event = new CollectIgnoredPathsEvent($this);
-    $this->eventDispatcher->dispatch($event);
+    try {
+      $this->eventDispatcher->dispatch($event);
+    }
+    catch (\Throwable $e) {
+      $this->rethrowAsStageException($e);
+    }
     return $event->getAll();
   }
 
@@ -283,7 +291,9 @@ class Stage implements LoggerAwareInterface {
    *   as long as the stage needs to exist.
    *
    * @throws \Drupal\package_manager\Exception\StageException
-   *   Thrown if a stage directory already exists.
+   *   Thrown if a stage directory already exists, or if an error occurs while
+   *   creating the stage directory. In the latter situation, the stage
+   *   directory will be destroyed.
    *
    * @see ::claim()
    */
@@ -312,22 +322,32 @@ class Stage implements LoggerAwareInterface {
     $active_dir = $this->pathFactory->create($this->pathLocator->getProjectRoot());
     $stage_dir = $this->pathFactory->create($this->getStageDirectory());
 
-    try {
-      $ignored_paths = $this->getIgnoredPaths();
-    }
-    catch (\Throwable $throwable) {
-      throw new StageException($this, $throwable->getMessage(), $throwable->getCode(), $throwable);
-    }
-    $event = new PreCreateEvent($this, $ignored_paths);
+    $event = new PreCreateEvent($this, $this->getIgnoredPaths());
     // If an error occurs and we won't be able to create the stage, mark it as
     // available.
     $this->dispatch($event, [$this, 'markAsAvailable']);
 
-    $this->beginner->begin($active_dir, $stage_dir, new PathList($event->getExcludedPaths()), NULL, $timeout);
+    try {
+      $this->beginner->begin($active_dir, $stage_dir, new PathList($event->getExcludedPaths()), NULL, $timeout);
+    }
+    catch (\Throwable $error) {
+      $this->destroy();
+      $this->rethrowAsStageException($error);
+    }
     $this->dispatch(new PostCreateEvent($this));
     return $id;
   }
 
+  /**
+   * Wraps an exception in a StageException and re-throws it.
+   *
+   * @param \Throwable $e
+   *   The throwable to wrap.
+   */
+  private function rethrowAsStageException(\Throwable $e): never {
+    throw new StageException($this, $e->getMessage(), $e->getCode(), $e);
+  }
+
   /**
    * Adds or updates packages in the stage directory.
    *
@@ -340,33 +360,56 @@ class Stage implements LoggerAwareInterface {
    * @param int|null $timeout
    *   (optional) How long to allow the Composer operation to run before timing
    *   out, in seconds, or NULL to never time out. Defaults to 300 seconds.
+   *
+   * @throws \Drupal\package_manager\Exception\StageException
+   *   Thrown if the Composer operation cannot be started, or if an error occurs
+   *   during the operation. In the latter situation, the stage directory will
+   *   be destroyed.
    */
   public function require(array $runtime, array $dev = [], ?int $timeout = 300): void {
     $this->checkOwnership();
 
     $this->dispatch(new PreRequireEvent($this, $runtime, $dev));
-    $active_dir = $this->pathFactory->create($this->pathLocator->getProjectRoot());
-    $stage_dir = $this->pathFactory->create($this->getStageDirectory());
+
+    // A helper function to execute a command in the stage, destroying it if an
+    // exception occurs in the middle of a Composer operation.
+    $do_stage = function (array $command) use ($timeout): void {
+      $active_dir = $this->pathFactory->create($this->pathLocator->getProjectRoot());
+      $stage_dir = $this->pathFactory->create($this->getStageDirectory());
+
+      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.
+        if (!$e instanceof InvalidArgumentException && !$e instanceof PreconditionException) {
+          $this->destroy();
+        }
+        $this->rethrowAsStageException($e);
+      }
+    };
 
     // Change the runtime and dev requirements as needed, but don't update
     // the installed packages yet.
     if ($runtime) {
       self::validateRequirements($runtime);
       $command = array_merge(['require', '--no-update'], $runtime);
-      $this->stager->stage($command, $active_dir, $stage_dir, NULL, $timeout);
+      $do_stage($command);
     }
     if ($dev) {
       self::validateRequirements($dev);
       $command = array_merge(['require', '--dev', '--no-update'], $dev);
-      $this->stager->stage($command, $active_dir, $stage_dir, NULL, $timeout);
+      $do_stage($command);
     }
 
     // If constraints were changed, update those packages.
     if ($runtime || $dev) {
       $command = array_merge(['update', '--with-all-dependencies'], $runtime, $dev);
-      $this->stager->stage($command, $active_dir, $stage_dir, NULL, $timeout);
+      $do_stage($command);
     }
-
     $this->dispatch(new PostRequireEvent($this, $runtime, $dev));
   }
 
@@ -397,17 +440,10 @@ class Stage implements LoggerAwareInterface {
     $active_dir = $this->pathFactory->create($this->pathLocator->getProjectRoot());
     $stage_dir = $this->pathFactory->create($this->getStageDirectory());
 
-    try {
-      $ignored_paths = $this->getIgnoredPaths();
-    }
-    catch (\Throwable $throwable) {
-      throw new StageException($this, $throwable->getMessage(), $throwable->getCode(), $throwable);
-    }
-
     // If an error occurs while dispatching the events, ensure that ::destroy()
     // doesn't think we're in the middle of applying the staged changes to the
     // active directory.
-    $event = new PreApplyEvent($this, $ignored_paths);
+    $event = new PreApplyEvent($this, $this->getIgnoredPaths());
     $this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime());
     $this->dispatch($event, $this->setNotApplying());
 
@@ -426,7 +462,7 @@ class Stage implements LoggerAwareInterface {
       // The commit operation has not started yet, so we can clear the failure
       // marker.
       $this->failureMarker->clear();
-      throw new StageException($this, $e->getMessage(), $e->getCode(), $e);
+      $this->rethrowAsStageException($e);
     }
     catch (\Throwable $throwable) {
       // The commit operation may have failed midway through, and the site code

package_manager/tests/modules/package_manager_bypass/src/ComposerStagerExceptionTrait.php

+<?php
+
+declare(strict_types = 1);
+
+namespace Drupal\package_manager_bypass;
+
+/**
+ * Trait to make Composer Stager throw pre-determined exceptions in tests.
+ *
+ * @internal
+ */
+trait ComposerStagerExceptionTrait {
+
+  /**
+   * Sets an exception to be thrown.
+   *
+   * @param \Throwable $exception
+   *   The throwable.
+   */
+  public static function setException(\Throwable $exception): void {
+    \Drupal::state()->set(static::class . '-exception', $exception);
+  }
+
+  /**
+   * Throws the exception if set.
+   */
+  protected function throwExceptionIfSet(): void {
+    if ($exception = $this->state->get(static::class . '-exception')) {
+      throw $exception;
+    }
+  }
+
+}

package_manager/tests/modules/package_manager_bypass/src/LoggingBeginner.php

@@ -18,6 +18,7 @@ use PhpTuf\ComposerStager\Domain\Value\PathList\PathListInterface;
  */
 final class LoggingBeginner implements BeginnerInterface {
 
+  use ComposerStagerExceptionTrait;
   use LoggingDecoratorTrait;
 
   /**
@@ -45,6 +46,7 @@ final class LoggingBeginner implements BeginnerInterface {
    */
   public function begin(PathInterface $activeDir, PathInterface $stagingDir, ?PathListInterface $exclusions = NULL, ?ProcessOutputCallbackInterface $callback = NULL, ?int $timeout = ProcessRunnerInterface::DEFAULT_TIMEOUT): void {
     $this->saveInvocationArguments($activeDir, $stagingDir, $exclusions, $timeout);
+    $this->throwExceptionIfSet();
     $this->inner->begin($activeDir, $stagingDir, $exclusions, $callback, $timeout);
   }
 

package_manager/tests/modules/package_manager_bypass/src/LoggingCommitter.php

@@ -18,6 +18,7 @@ use PhpTuf\ComposerStager\Domain\Value\PathList\PathListInterface;
  */
 final class LoggingCommitter implements CommitterInterface {
 
+  use ComposerStagerExceptionTrait;
   use LoggingDecoratorTrait;
 
   /**
@@ -45,20 +46,8 @@ final class LoggingCommitter implements CommitterInterface {
    */
   public function commit(PathInterface $stagingDir, PathInterface $activeDir, ?PathListInterface $exclusions = NULL, ?ProcessOutputCallbackInterface $callback = NULL, ?int $timeout = ProcessRunnerInterface::DEFAULT_TIMEOUT): void {
     $this->saveInvocationArguments($stagingDir, $activeDir, $exclusions, $timeout);
-    if ($exception = $this->state->get(static::class . '-exception')) {
-      throw $exception;
-    }
+    $this->throwExceptionIfSet();
     $this->inner->commit($stagingDir, $activeDir, $exclusions, $callback, $timeout);
   }
 
-  /**
-   * Sets an exception to be thrown during ::commit().
-   *
-   * @param \Throwable $exception
-   *   The throwable.
-   */
-  public static function setException(\Throwable $exception): void {
-    \Drupal::state()->set(static::class . '-exception', $exception);
-  }
-
 }

package_manager/tests/modules/package_manager_bypass/src/NoOpStager.php

@@ -27,6 +27,7 @@ use PhpTuf\ComposerStager\Domain\Value\Path\PathInterface;
  */
 final class NoOpStager implements StagerInterface {
 
+  use ComposerStagerExceptionTrait;
   use LoggingDecoratorTrait;
 
   /**
@@ -44,6 +45,7 @@ final class NoOpStager implements StagerInterface {
    */
   public function stage(array $composerCommand, PathInterface $activeDir, PathInterface $stagingDir, ?ProcessOutputCallbackInterface $callback = NULL, ?int $timeout = ProcessRunnerInterface::DEFAULT_TIMEOUT): void {
     $this->saveInvocationArguments($composerCommand, $stagingDir, $timeout);
+    $this->throwExceptionIfSet();
 
     // If desired, simulate a change to the lock file (e.g., as a result of
     // running `composer update`).

package_manager/tests/src/Kernel/StageTest.php

@@ -17,7 +17,9 @@ use Drupal\package_manager\Event\StageEvent;
 use Drupal\package_manager\Exception\ApplyFailedException;
 use Drupal\package_manager\Exception\StageException;
 use Drupal\package_manager\Exception\StageFailureMarkerException;
+use Drupal\package_manager_bypass\LoggingBeginner;
 use Drupal\package_manager_bypass\LoggingCommitter;
+use Drupal\package_manager_bypass\NoOpStager;
 use PhpTuf\ComposerStager\Domain\Exception\InvalidArgumentException;
 use PhpTuf\ComposerStager\Domain\Exception\PreconditionException;
 use PhpTuf\ComposerStager\Domain\Service\Precondition\PreconditionInterface;
@@ -544,6 +546,57 @@ class StageTest extends PackageManagerKernelTestBase {
     $this->assertFalse($logger->hasRecord($warning_message, LogLevel::WARNING));
   }
 
+  /**
+   * Data provider for ::testFailureDuringComposerStagerOperations().
+   *
+   * @return array[]
+   *   The test cases.
+   */
+  public function providerFailureDuringComposerStagerOperations(): array {
+    return [
+      [LoggingBeginner::class],
+      [NoOpStager::class],
+      [LoggingCommitter::class],
+    ];
+  }
+
+  /**
+   * Tests when Composer Stager throws an exception during an operation.
+   *
+   * @param string $throwing_class
+   *   The fully qualified name of the Composer Stager class that should throw
+   *   an exception. It is expected to have a static ::setException() method,
+   *   provided by \Drupal\package_manager_bypass\ComposerStagerExceptionTrait.
+   *
+   * @dataProvider providerFailureDuringComposerStagerOperations
+   */
+  public function testFailureDuringComposerStagerOperations(string $throwing_class): void {
+    if ($throwing_class === LoggingCommitter::class) {
+      // If the committer throws an exception, Stage will always re-throw it
+      // with a predetermined failure message.
+      $expected_message = 'Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.';
+    }
+    else {
+      $expected_message = "$throwing_class is angry!";
+    }
+
+    $exception = new \Exception($expected_message, 1024);
+    $throwing_class::setException($exception);
+
+    $stage = $this->createStage();
+    try {
+      $stage->create();
+      $stage->require(['ext-json:*']);
+      $stage->apply();
+      $this->fail('Expected an exception to be thrown, but it was not.');
+    }
+    catch (StageException $e) {
+      $this->assertSame($expected_message, $e->getMessage());
+      $this->assertSame(1024, $e->getCode());
+      $this->assertSame($exception, $e->getPrevious());
+    }
+  }
+
   /**
    * Tests that ignored paths are collected before create and apply.
    */