Issue #3248909 by phenaproxima, tedbow: Do not allow destroying a stage during Apply

automatic_updates.services.yml

@@ -21,6 +21,7 @@ services:
       - '@file_system'
       - '@event_dispatcher'
       - '@tempstore.shared'
+      - '@datetime.time'
   automatic_updates.cron_updater:
     class: Drupal\automatic_updates\CronUpdater
     arguments:
@@ -33,6 +34,7 @@ services:
       - '@file_system'
       - '@event_dispatcher'
       - '@tempstore.shared'
+      - '@datetime.time'
   automatic_updates.staged_projects_validator:
     class: Drupal\automatic_updates\Validator\StagedProjectsValidator
     arguments:

package_manager/src/Stage.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\package_manager;
 
+use Drupal\Component\Datetime\TimeInterface;
 use Drupal\Component\FileSystem\FileSystem;
 use Drupal\Component\Utility\Crypt;
 use Drupal\Core\File\Exception\FileException;
@@ -55,6 +56,16 @@ class Stage {
    */
   protected const TEMPSTORE_METADATA_KEY = 'metadata';
 
+  /**
+   * The tempstore key under which to store the time that ::apply() was called.
+   *
+   * @var string
+   *
+   * @see ::apply()
+   * @see ::destroy()
+   */
+  private const TEMPSTORE_APPLY_TIME_KEY = 'apply_time';
+
   /**
    * The path locator service.
    *
@@ -104,6 +115,13 @@ class Stage {
    */
   protected $tempStore;
 
+  /**
+   * The time service.
+   *
+   * @var \Drupal\Component\Datetime\TimeInterface
+   */
+  protected $time;
+
   /**
    * The lock info for the stage.
    *
@@ -130,14 +148,17 @@ class Stage {
    *   The event dispatcher service.
    * @param \Drupal\Core\TempStore\SharedTempStoreFactory $shared_tempstore
    *   The shared tempstore factory.
+   * @param \Drupal\Component\Datetime\TimeInterface $time
+   *   The time service.
    */
-  public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore) {
+  public function __construct(PathLocator $path_locator, BeginnerInterface $beginner, StagerInterface $stager, CommitterInterface $committer, FileSystemInterface $file_system, EventDispatcherInterface $event_dispatcher, SharedTempStoreFactory $shared_tempstore, TimeInterface $time) {
     $this->pathLocator = $path_locator;
     $this->beginner = $beginner;
     $this->stager = $stager;
     $this->committer = $committer;
     $this->fileSystem = $file_system;
     $this->eventDispatcher = $event_dispatcher;
+    $this->time = $time;
     $this->tempStore = $shared_tempstore->get('package_manager_stage');
   }
 
@@ -224,7 +245,9 @@ class Stage {
     $stage_dir = $this->getStageDirectory();
 
     $event = new PreCreateEvent($this);
-    $this->dispatch($event);
+    // 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, $event->getExcludedPaths());
     $this->dispatch(new PostCreateEvent($this));
@@ -277,9 +300,16 @@ class Stage {
     $stage_dir = $this->getStageDirectory();
 
     $event = new PreApplyEvent($this);
-    $this->dispatch($event);
+    $this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime());
+    // If an error occurs while dispatching the event, ensure that ::destroy()
+    // doesn't think we're in the middle of applying the staged changes to the
+    // active directory.
+    $this->dispatch($event, function (): void {
+      $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
+    });
 
     $this->committer->commit($stage_dir, $active_dir, $event->getExcludedPaths());
+    $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
     $this->dispatch(new PostApplyEvent($this));
   }
 
@@ -290,14 +320,22 @@ class Stage {
    *   (optional) If TRUE, the staging area will be destroyed even if it is not
    *   owned by the current user or session. Defaults to FALSE.
    *
-   * @todo Do not allow the stage to be destroyed while it's being applied to
-   *   the active directory in https://www.drupal.org/i/3248909.
+   * @throws \Drupal\package_manager\Exception\StageException
+   *   If the staged changes are being applied to the active directory.
    */
   public function destroy(bool $force = FALSE): void {
     if (!$force) {
       $this->checkOwnership();
     }
 
+    // If we started applying staged changes to the active directory less than
+    // an hour ago, prevent the stage from being destroyed.
+    // @see :apply()
+    $apply_time = $this->tempStore->get(self::TEMPSTORE_APPLY_TIME_KEY);
+    if (isset($apply_time) && $this->time->getRequestTime() - $apply_time < 3600) {
+      throw new StageException('Cannot destroy the staging area while it is being applied to the active directory.');
+    }
+
     $this->dispatch(new PreDestroyEvent($this));
     // Delete all directories in parent staging directory.
     $parent_stage_dir = static::getStagingRoot();
@@ -332,13 +370,16 @@ class Stage {
    *
    * @param \Drupal\package_manager\Event\StageEvent $event
    *   The event object.
+   * @param callable $on_error
+   *   (optional) A callback function to call if an error occurs, before any
+   *   exceptions are thrown.
    *
    * @throws \Drupal\package_manager\Exception\StageValidationException
    *   If the event collects any validation errors.
    * @throws \Drupal\package_manager\Exception\StageException
    *   If any other sort of error occurs.
    */
-  protected function dispatch(StageEvent $event): void {
+  protected function dispatch(StageEvent $event, callable $on_error = NULL): void {
     try {
       $this->eventDispatcher->dispatch($event);
 
@@ -354,10 +395,8 @@ class Stage {
     }
 
     if (isset($error)) {
-      // If we won't be able to create the staging area, mark it as available.
-      // @see ::create()
-      if ($event instanceof PreCreateEvent) {
-        $this->markAsAvailable();
+      if ($on_error) {
+        $on_error();
       }
       throw $error;
     }

package_manager/tests/modules/package_manager_test_api/src/ApiController.php

@@ -42,6 +42,7 @@ class ApiController extends ControllerBase {
       $container->get('file_system'),
       $container->get('event_dispatcher'),
       $container->get('tempstore.shared'),
+      $container->get('datetime.time')
     );
     return new static($stage);
   }

package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php

@@ -44,6 +44,19 @@ class TestSubscriber implements EventSubscriberInterface {
     $this->state = $state;
   }
 
+  /**
+   * Sets whether a specific event will call exit().
+   *
+   * This is useful for simulating an unrecoverable (fatal) error when handling
+   * the given event.
+   *
+   * @param string $event
+   *   The event class.
+   */
+  public static function setExit(string $event): void {
+    \Drupal::state()->set(static::STATE_KEY . ".$event", 'exit');
+  }
+
   /**
    * Sets validation results for a specific event.
    *
@@ -102,6 +115,9 @@ class TestSubscriber implements EventSubscriberInterface {
     if ($results instanceof \Throwable) {
       throw $results;
     }
+    elseif ($results === 'exit') {
+      exit();
+    }
     /** @var \Drupal\package_manager\ValidationResult $result */
     foreach ($results as $result) {
       if ($result->getSeverity() === SystemManager::REQUIREMENT_ERROR) {

package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php

@@ -78,7 +78,8 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
       $this->container->get('package_manager.committer'),
       $this->container->get('file_system'),
       $this->container->get('event_dispatcher'),
-      $this->container->get('tempstore.shared')
+      $this->container->get('tempstore.shared'),
+      $this->container->get('datetime.time')
     );
   }
 
@@ -297,9 +298,9 @@ class TestStage extends Stage {
   /**
    * {@inheritdoc}
    */
-  protected function dispatch(StageEvent $event): void {
+  protected function dispatch(StageEvent $event, callable $on_error = NULL): void {
     try {
-      parent::dispatch($event);
+      parent::dispatch($event, $on_error);
     }
     catch (StageException $e) {
       // Attach the event object to the exception so that test code can verify

package_manager/tests/src/Kernel/StageTest.php

@@ -2,6 +2,11 @@
 
 namespace Drupal\Tests\package_manager\Kernel;
 
+use Drupal\Component\Datetime\Time;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\package_manager\Event\PreApplyEvent;
+use Drupal\package_manager\Exception\StageException;
+
 /**
  * @coversDefaultClass \Drupal\package_manager\Stage
  *
@@ -9,6 +14,16 @@ namespace Drupal\Tests\package_manager\Kernel;
  */
 class StageTest extends PackageManagerKernelTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
+  public function register(ContainerBuilder $container) {
+    parent::register($container);
+
+    $container->getDefinition('datetime.time')
+      ->setClass(TestTime::class);
+  }
+
   /**
    * @covers ::getStageDirectory
    */
@@ -27,4 +42,78 @@ class StageTest extends PackageManagerKernelTestBase {
     $this->createStage()->getStageDirectory();
   }
 
+  /**
+   * Data provider for ::testDestroyDuringApply().
+   *
+   * @return array[]
+   *   Sets of arguments to pass to the test method.
+   */
+  public function providerDestroyDuringApply(): array {
+    return [
+      'force destroy, not stale' => [TRUE, 1, TRUE],
+      'regular destroy, not stale' => [FALSE, 1, TRUE],
+      'force destroy, stale' => [TRUE, 7200, FALSE],
+      'regular destroy, stale' => [FALSE, 7200, FALSE],
+    ];
+  }
+
+  /**
+   * Tests destroying a stage while applying it.
+   *
+   * @param bool $force
+   *   Whether or not the stage should be force destroyed.
+   * @param int $time_offset
+   *   How many simulated seconds should have elapsed between the PreApplyEvent
+   *   being dispatched and the attempt to destroy the stage.
+   * @param bool $expect_exception
+   *   Whether or not destroying the stage will raise an exception.
+   *
+   * @dataProvider providerDestroyDuringApply
+   */
+  public function testDestroyDuringApply(bool $force, int $time_offset, bool $expect_exception): void {
+    $listener = function (PreApplyEvent $event) use ($force, $time_offset): void {
+      // Simulate that a certain amount of time has passed since we started
+      // applying staged changes. After a point, it should be possible to
+      // destroy the stage even if it hasn't finished.
+      TestTime::$offset = $time_offset;
+
+      // No real-life event subscriber should try to destroy the stage while
+      // handling another event. The only reason we're doing it here is to
+      // simulate an attempt to destroy the stage while it's being applied, for
+      // testing purposes.
+      $event->getStage()->destroy($force);
+    };
+    $this->container->get('event_dispatcher')
+      ->addListener(PreApplyEvent::class, $listener);
+
+    $stage = $this->createStage();
+    $stage->create();
+    if ($expect_exception) {
+      $this->expectException(StageException::class);
+      $this->expectExceptionMessage('Cannot destroy the staging area while it is being applied to the active directory.');
+    }
+    $stage->apply();
+  }
+
+}
+
+/**
+ * A test-only implementation of the time service.
+ */
+class TestTime extends Time {
+
+  /**
+   * An offset to add to the request time.
+   *
+   * @var int
+   */
+  public static $offset = 0;
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getRequestTime() {
+    return parent::getRequestTime() + static::$offset;
+  }
+
 }

src/Form/UpdateReady.php

@@ -11,6 +11,7 @@ use Drupal\Core\Form\FormBase;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\State\StateInterface;
+use Drupal\package_manager\Exception\StageException;
 use Drupal\package_manager\Exception\StageOwnershipException;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
@@ -193,9 +194,14 @@ class UpdateReady extends FormBase {
    * Cancels the in-progress update.
    */
   public function cancel(array &$form, FormStateInterface $form_state): void {
-    $this->updater->destroy();
-    $this->messenger()->addStatus($this->t('The update was successfully cancelled.'));
-    $form_state->setRedirect('automatic_updates.report_update');
+    try {
+      $this->updater->destroy();
+      $this->messenger()->addStatus($this->t('The update was successfully cancelled.'));
+      $form_state->setRedirect('automatic_updates.report_update');
+    }
+    catch (StageException $e) {
+      $this->messenger()->addError($e->getMessage());
+    }
   }
 
 }

src/Form/UpdaterForm.php

@@ -14,6 +14,7 @@ use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Link;
 use Drupal\Core\State\StateInterface;
 use Drupal\Core\Url;
+use Drupal\package_manager\Exception\StageException;
 use Drupal\package_manager\Exception\StageOwnershipException;
 use Drupal\system\SystemManager;
 use Drupal\update\UpdateManagerInterface;
@@ -276,8 +277,13 @@ class UpdaterForm extends FormBase {
    * Submit function to delete an existing in-progress update.
    */
   public function deleteExistingUpdate(): void {
-    $this->updater->destroy(TRUE);
-    $this->messenger()->addMessage($this->t("Staged update deleted"));
+    try {
+      $this->updater->destroy(TRUE);
+      $this->messenger()->addMessage($this->t("Staged update deleted"));
+    }
+    catch (StageException $e) {
+      $this->messenger()->addError($e->getMessage());
+    }
   }
 
   /**

src/Updater.php

@@ -91,9 +91,9 @@ class Updater extends Stage {
   /**
    * {@inheritdoc}
    */
-  protected function dispatch(StageEvent $event): void {
+  protected function dispatch(StageEvent $event, callable $on_error = NULL): void {
     try {
-      parent::dispatch($event);
+      parent::dispatch($event, $on_error);
     }
     catch (StageValidationException $e) {
       throw new UpdateException($e->getResults(), $e->getMessage() ?: "Unable to complete the update because of errors.", $e->getCode(), $e);

tests/src/Functional/UpdaterFormTest.php

@@ -3,6 +3,8 @@
 namespace Drupal\Tests\automatic_updates\Functional;
 
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
+use Drupal\automatic_updates_test\Datetime\TestTime;
+use Drupal\package_manager\Event\PreApplyEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\ValidationResult;
 use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1;
@@ -240,6 +242,7 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
    */
   public function testDeleteExistingUpdate(): void {
     $conflict_message = 'Cannot begin an update because another Composer operation is currently in progress.';
+    $cancelled_message = 'The update was successfully cancelled.';
 
     $assert_session = $this->assertSession();
     $page = $this->getSession()->getPage();
@@ -263,7 +266,7 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
     // Delete the existing update.
     $page->pressButton('Cancel update');
     $assert_session->addressEquals('/admin/reports/updates/automatic-update');
-    $assert_session->pageTextContains('The update was successfully cancelled.');
+    $assert_session->pageTextContains($cancelled_message);
     $assert_session->pageTextNotContains($conflict_message);
     // Ensure we can start another update after deleting the existing one.
     $page->pressButton('Update');
@@ -281,12 +284,60 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
     $this->drupalGet('/admin/reports/updates/automatic-update');
     $assert_session->pageTextContains($conflict_message);
     $assert_session->buttonNotExists('Update');
-    // We should be able to delete the previous update, and then we should be
-    // able to start a new one.
+    // We should be able to delete the previous update, then start a new one.
     $page->pressButton('Delete existing update');
     $assert_session->pageTextContains('Staged update deleted');
     $assert_session->pageTextNotContains($conflict_message);
-    $assert_session->buttonExists('Update');
+    $page->pressButton('Update');
+    $this->checkForMetaRefresh();
+    $this->assertUpdateReady();
+
+    // Stop execution during pre-apply. This should make Package Manager think
+    // the staged changes are being applied and raise an error if we try to
+    // cancel the update.
+    TestSubscriber1::setExit(PreApplyEvent::class);
+    $page->pressButton('Continue');
+    $this->checkForMetaRefresh();
+    $page->clickLink('the error page');
+    $page->pressButton('Cancel update');
+    // The exception should have been caught and displayed in the messages area.
+    $assert_session->statusCodeEquals(200);
+    $destroy_error = 'Cannot destroy the staging area while it is being applied to the active directory.';
+    $assert_session->pageTextContains($destroy_error);
+    $assert_session->pageTextNotContains($cancelled_message);
+
+    // We should get the same error if we log in as another user and try to
+    // delete the staged update.
+    $this->drupalLogin($this->rootUser);
+    $this->drupalGet('/admin/reports/updates/automatic-update');
+    $assert_session->pageTextContains($conflict_message);
+    $page->pressButton('Delete existing update');
+    $assert_session->statusCodeEquals(200);
+    $assert_session->pageTextContains($destroy_error);
+    $assert_session->pageTextNotContains('Staged update deleted');
+
+    // Two hours later, Package Manager should consider the stage to be stale,
+    // allowing the staged update to be deleted.
+    TestTime::setFakeTimeByOffset('+2 hours');
+    $this->getSession()->reload();
+    $assert_session->pageTextContains($conflict_message);
+    $page->pressButton('Delete existing update');
+    $assert_session->statusCodeEquals(200);
+    $assert_session->pageTextContains('Staged update deleted');
+
+    // If a legitimate error is raised during pre-apply, we should be able to
+    // delete the staged update right away.
+    $this->createTestValidationResults();
+    $results = $this->testResults['checker_1']['1 error'];
+    TestSubscriber1::setTestResult($results, PreApplyEvent::class);
+    $page->pressButton('Update');
+    $this->checkForMetaRefresh();
+    $this->assertUpdateReady();
+    $page->pressButton('Continue');
+    $this->checkForMetaRefresh();
+    $page->clickLink('the error page');
+    $page->pressButton('Cancel update');
+    $assert_session->pageTextContains($cancelled_message);
   }
 
   /**