Issue #3291147 by phenaproxima, tedbow, effulgentsia, heddn: Successfully...

Issue #3291147 by phenaproxima, tedbow, effulgentsia, heddn: Successfully applying an update puts the remainder of the request into an unreliable state

automatic_updates.module

@@ -88,6 +88,12 @@ function automatic_updates_module_implements_alter(&$implementations, $hook) {
     // own routes to avoid these messages while an update is in progress.
     unset($implementations['update']);
   }
+  if ($hook === 'cron') {
+    // Whatever mofo.
+    $hook = $implementations['automatic_updates'];
+    unset($implementations['automatic_updates']);
+    $implementations['automatic_updates'] = $hook;
+  }
 }
 
 /**

automatic_updates.routing.yml

@@ -27,6 +27,12 @@ automatic_updates.finish:
   options:
     _maintenance_access: TRUE
     _automatic_updates_readiness_messages: skip
+automatic_updates.cron.post_apply:
+  path: '/automatic-update/cron/post-apply/{stage_id}/{installed_version}/{target_version}/{key}'
+  defaults:
+    _controller: 'automatic_updates.cron_updater:handlePostApply'
+  requirements:
+    _access_system_cron: 'TRUE'
 # Links to our updater form appear in three different sets of local tasks. To ensure the breadcrumbs and paths are
 # consistent with the other local tasks in each set, we need two separate routes to the same form.
 automatic_updates.report_update:

automatic_updates.services.yml

@@ -33,6 +33,7 @@ services:
       - '@logger.factory'
       - '@plugin.manager.mail'
       - '@language_manager'
+      - '@state'
       - '@config.factory'
       - '@package_manager.path_locator'
       - '@package_manager.beginner'

automatic_updates_extensions/src/BatchProcessor.php

@@ -123,6 +123,25 @@ final class BatchProcessor {
     }
   }
 
+  /**
+   * Calls the updater's postApply() method.
+   *
+   * @param string $stage_id
+   *   The stage ID.
+   * @param array $context
+   *   The current context of the batch job.
+   *
+   * @see \Drupal\automatic_updates\Updater::postApply()
+   */
+  public static function postApply(string $stage_id, array &$context): void {
+    try {
+      static::getUpdater()->claim($stage_id)->postApply();
+    }
+    catch (\Throwable $e) {
+      static::handleException($e, $context);
+    }
+  }
+
   /**
    * Calls the updater's clean() method.
    *

automatic_updates_extensions/src/Form/UpdateReady.php

@@ -162,6 +162,7 @@ final class UpdateReady extends FormBase {
       ->setTitle($this->t('Apply updates'))
       ->setInitMessage($this->t('Preparing to apply updates'))
       ->addOperation([BatchProcessor::class, 'commit'], [$stage_id])
+      ->addOperation([BatchProcessor::class, 'postApply'], [$stage_id])
       ->addOperation([BatchProcessor::class, 'clean'], [$stage_id])
       ->setFinishCallback([BatchProcessor::class, 'finishCommit'])
       ->toArray();

automatic_updates_extensions/tests/modules/automatic_updates_extensions_test_api/src/ApiController.php

@@ -74,6 +74,7 @@ class ApiController extends ControllerBase {
     $this->extensionUpdater->begin($request->get('projects', []));
     $this->extensionUpdater->stage();
     $this->extensionUpdater->apply();
+    $this->extensionUpdater->postApply();
     $this->extensionUpdater->destroy();
 
     $dir = $this->pathLocator->getProjectRoot();

automatic_updates_extensions/tests/src/Kernel/AutomaticUpdatesExtensionsKernelTestBase.php

@@ -103,6 +103,7 @@ abstract class AutomaticUpdatesExtensionsKernelTestBase extends AutomaticUpdates
       $updater->begin($project_versions);
       $updater->stage();
       $updater->apply();
+      $updater->postApply();
       $updater->destroy();
 
       // If we did not get an exception, ensure we didn't expect any results.

config/schema/automatic_updates.schema.yml

@@ -5,6 +5,9 @@ automatic_updates.settings:
     cron:
       type: string
       label: 'Enable automatic updates during cron'
+    cron_port:
+      type: integer
+      label: 'Port to use for finalization sub-request'
     allow_core_minor_updates:
       type: boolean
       label: 'Allow minor level Drupal core updates'

package_manager/src/Stage.php

@@ -338,6 +338,15 @@ class Stage {
   /**
    * Applies staged changes to the active directory.
    *
+   * After the staged changes are applied, the current request should be
+   * terminated as soon as possible. This is because the code loaded into the
+   * PHP runtime may no longer match the code that is physically present in the
+   * active code base, which means that the current request is running in an
+   * unreliable, inconsistent environment. In the next request,
+   * ::postApply() should be called as early as possible after Drupal is
+   * fully bootstrapped, to rebuild the service container, flush caches, and
+   * dispatch the post-apply event.
+   *
    * @param int|null $timeout
    *   (optional) How long to allow the file copying operation to run before
    *   timing out, in seconds, or NULL to never time out. Defaults to 600
@@ -352,15 +361,31 @@ class Stage {
     // 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.
-    $release_apply = function (): void {
-      $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
-    };
-
     $event = new PreApplyEvent($this);
     $this->tempStore->set(self::TEMPSTORE_APPLY_TIME_KEY, $this->time->getRequestTime());
-    $this->dispatch($event, $release_apply);
+    $this->dispatch($event, $this->setNotApplying());
 
     $this->committer->commit($stage_dir, $active_dir, new PathList($event->getExcludedPaths()), NULL, $timeout);
+  }
+
+  /**
+   * Returns a closure that marks this stage as no longer being applied.
+   *
+   * @return \Closure
+   *   A closure that, when called, marks this stage as no longer in the process
+   *   of being applied to the active directory.
+   */
+  private function setNotApplying(): \Closure {
+    return function (): void {
+      $this->tempStore->delete(self::TEMPSTORE_APPLY_TIME_KEY);
+    };
+  }
+
+  /**
+   * Performs post-apply tasks.
+   */
+  public function postApply(): void {
+    $this->checkOwnership();
 
     // Rebuild the container and clear all caches, to ensure that new services
     // are picked up.
@@ -370,6 +395,7 @@ class Stage {
     // unlikely to call newly added code during the current request.
     $this->eventDispatcher = \Drupal::service('event_dispatcher');
 
+    $release_apply = $this->setNotApplying();
     $this->dispatch(new PostApplyEvent($this), $release_apply);
     $release_apply();
   }

package_manager/tests/modules/package_manager_test_api/package_manager_test_api.routing.yml

@@ -4,3 +4,9 @@ package_manager_test_api:
     _controller: 'Drupal\package_manager_test_api\ApiController::run'
   requirements:
     _access: 'TRUE'
+package_manager_test_api.finish:
+  path: '/package-manager-test-api/finish/{id}'
+  defaults:
+    _controller: 'Drupal\package_manager_test_api\ApiController::finish'
+  requirements:
+    _access: 'TRUE'

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

@@ -3,10 +3,12 @@
 namespace Drupal\package_manager_test_api;
 
 use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\Url;
 use Drupal\package_manager\PathLocator;
 use Drupal\package_manager\Stage;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\JsonResponse;
+use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 
 /**
@@ -63,10 +65,10 @@ class ApiController extends ControllerBase {
   }
 
   /**
-   * Runs a complete stage life cycle.
+   * Begins a stage life cycle.
    *
    * Creates a staging area, requires packages into it, applies changes to the
-   * active directory, and destroys the stage.
+   * active directory.
    *
    * @param \Symfony\Component\HttpFoundation\Request $request
    *   The request. The runtime and dev dependencies are expected to be in
@@ -75,18 +77,47 @@ class ApiController extends ControllerBase {
    *   contains an array of file paths, relative to the project root, whose
    *   contents should be returned in the response.
    *
-   * @return \Symfony\Component\HttpFoundation\JsonResponse
-   *   A JSON response containing an associative array of the contents of the
-   *   files listed in the 'files_to_return' request key. The array will be
-   *   keyed by path, relative to the project root.
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   *   A response that directs to the ::finish() method.
+   *
+   * @see ::finish()
    */
-  public function run(Request $request): JsonResponse {
-    $this->stage->create();
+  public function run(Request $request): RedirectResponse {
+    $id = $this->stage->create();
     $this->stage->require(
       $request->get('runtime', []),
       $request->get('dev', [])
     );
     $this->stage->apply();
+
+    $redirect_url = Url::fromRoute('package_manager_test_api.finish')
+      ->setRouteParameter('id', $id)
+      ->setOption('query', [
+        'files_to_return' => $request->get('files_to_return', []),
+      ])
+      ->setAbsolute()
+      ->toString();
+
+    return new RedirectResponse($redirect_url);
+  }
+
+  /**
+   * Performs post-apply tasks and destroys the stage.
+   *
+   * @param string $id
+   *   The stage ID.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request. There may be a 'files_to_return' key in either the query
+   *   string or request body which contains an array of file paths, relative to
+   *   the project root, whose contents should be returned in the response.
+   *
+   * @return \Symfony\Component\HttpFoundation\JsonResponse
+   *   A JSON response containing an associative array of the contents of the
+   *   files listed in the 'files_to_return' request key. The array will be
+   *   keyed by path, relative to the project root.
+   */
+  public function finish(string $id, Request $request): JsonResponse {
+    $this->stage->claim($id)->postApply();
     $this->stage->destroy();
 
     $dir = $this->pathLocator->getProjectRoot();

package_manager/tests/src/Build/PackageUpdateTest.php

@@ -100,10 +100,11 @@ class PackageUpdateTest extends TemplateProjectTestBase {
     $this->assertSame($expected_pre_apply_results, $results['pre']);
 
     $expected_post_apply_results = [
-      // Existing functions will still use the pre-update version.
-      'return value of existing global function' => 'pre-update-value',
-      // New functions that were added in .module files will not be available.
-      'new global function exists' => 'not exists',
+      // An existing functions will now return a new value.
+      'return value of existing global function' => 'post-update-value',
+      // New functions that were added in .module files will be available
+      // because the post-apply event is dispatched in a new request.
+      'new global function exists' => 'exists',
       // Definitions for existing routes should be updated.
       'path of changed route' => '/updated-module/changed/post',
       // Routes deleted from the updated module should not be available.
@@ -127,11 +128,11 @@ class PackageUpdateTest extends TemplateProjectTestBase {
       'value of updated_module.added_service' => 'New service, should not exist before update',
       // A block removed from the updated module should not be defined anymore.
       'updated_module_deleted_block block exists' => 'not exists',
-      // A block that was updated should have a changed definition, but an
-      // unchanged implementation.
+      // A block that was updated should have a changed definition and
+      // implementation.
       'updated_module_updated_block block exists' => 'exists',
       'updated_module_updated_block block label' => '1.1.0',
-      'updated_module_updated_block block output' => '1.0.0',
+      'updated_module_updated_block block output' => '1.1.0',
       // A block added to the module should be defined.
       'updated_module_added_block block exists' => 'exists',
       'updated_module_added_block block label' => 'Added block',
@@ -142,14 +143,13 @@ class PackageUpdateTest extends TemplateProjectTestBase {
       'updated_module_ignored_block block exists' => 'exists',
       'updated_module_ignored_block block label' => '1.1.0',
       'updated_module_ignored_block block output' => 'I was ignored before the update.',
-      // Existing class should be available.
+      // Existing class should still be available, and will be outputting its
+      // new value.
       'ChangedClass exists' => 'exists',
-      // Existing class will still use the pre-update version.
-      'value of ChangedClass' => 'Before Update',
-      // Classes loaded in pre-apply or before and deleted from the updated module should
-      // be available.
-      'LoadedAndDeletedClass exists' => 'exists',
-      'value of LoadedAndDeletedClass' => 'This class will be loaded and then deleted',
+      'value of ChangedClass' => 'After Update',
+      // Classes loaded in pre-apply, but deleted from the updated module,
+      // should be unavailable.
+      'LoadedAndDeletedClass exists' => 'not exists',
       // Classes not loaded before the apply operation and deleted from the updated module
       // should not be available.
       'DeletedClass exists' => 'not exists',

package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php

@@ -141,6 +141,7 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
       $stage->create();
       $stage->require(['drupal/core:9.8.1']);
       $stage->apply();
+      $stage->postApply();
       $stage->destroy();
 
       // If we did not get an exception, ensure we didn't expect any results.

package_manager/tests/src/Kernel/StageEventsTest.php

@@ -100,6 +100,7 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
     $this->stage->create();
     $this->stage->require(['ext-json:*']);
     $this->stage->apply();
+    $this->stage->postApply();
     $this->stage->destroy();
 
     $this->assertSame($this->events, [
@@ -154,6 +155,7 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
       $this->stage->create();
       $this->stage->require(['ext-json:*']);
       $this->stage->apply();
+      $this->stage->postApply();
       $this->stage->destroy();
 
       $this->fail('Expected \Drupal\package_manager\Exception\StageValidationException to be thrown.');

package_manager/tests/src/Kernel/StageOwnershipTest.php

@@ -114,6 +114,7 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
         ['vendor/lib:0.0.1'],
       ],
       'apply' => [],
+      'postApply' => [],
       'destroy' => [],
     ];
     foreach ($callbacks as $method => $arguments) {
@@ -179,6 +180,7 @@ class StageOwnershipTest extends PackageManagerKernelTestBase {
         ['vendor/lib:0.0.1'],
       ],
       'apply' => [],
+      'postApply' => [],
       'destroy' => [],
     ];
     foreach ($callbacks as $method => $arguments) {

package_manager/tests/src/Kernel/StageTest.php

@@ -192,6 +192,15 @@ class StageTest extends PackageManagerKernelTestBase {
       $this->expectExceptionMessage('Cannot destroy the staging area while it is being applied to the active directory.');
     }
     $stage->apply();
+
+    // If the stage was successfully destroyed by the event handler (i.e., the
+    // stage has been applying for too long and is therefore considered stale),
+    // the postApply() method should fail because the stage is not claimed.
+    if ($stage->isAvailable()) {
+      $this->expectException('LogicException');
+      $this->expectExceptionMessage('Stage must be claimed before performing any operations on it.');
+    }
+    $stage->postApply();
   }
 
   /**

src/BatchProcessor.php

@@ -122,6 +122,34 @@ class BatchProcessor {
   public static function commit(string $stage_id, array &$context): void {
     try {
       static::getUpdater()->claim($stage_id)->apply();
+      // The batch system does not allow any single request to run for longer
+      // than a second, so this will force the next operation to be done in a
+      // new request. This helps keep the running code in as consistent a state
+      // as possible.
+      // @see \Drupal\package_manager\Stage::apply()
+      // @see \Drupal\package_manager\Stage::postApply()
+      // @todo See if there's a better way to ensure the post-apply tasks run
+      //   in a new request in https://www.drupal.org/i/3293150.
+      sleep(1);
+    }
+    catch (\Throwable $e) {
+      static::handleException($e, $context);
+    }
+  }
+
+  /**
+   * Calls the updater's postApply() method.
+   *
+   * @param string $stage_id
+   *   The stage ID.
+   * @param array $context
+   *   The current context of the batch job.
+   *
+   * @see \Drupal\automatic_updates\Updater::postApply()
+   */
+  public static function postApply(string $stage_id, array &$context): void {
+    try {
+      static::getUpdater()->claim($stage_id)->postApply();
     }
     catch (\Throwable $e) {
       static::handleException($e, $context);

src/CronUpdater.php

@@ -6,7 +6,11 @@ use Drupal\automatic_updates_9_3_shim\ProjectRelease;
 use Drupal\Core\Language\LanguageManagerInterface;
 use Drupal\Core\Logger\LoggerChannelFactoryInterface;
 use Drupal\Core\Mail\MailManagerInterface;
+use Drupal\Core\State\StateInterface;
+use Drupal\Core\Url;
 use Drupal\package_manager\Exception\StageValidationException;
+use GuzzleHttp\Psr7\Uri as GuzzleUri;
+use Symfony\Component\HttpFoundation\Response;
 
 /**
  * Defines a service that updates via cron.
@@ -75,6 +79,13 @@ class CronUpdater extends Updater {
    */
   protected $languageManager;
 
+  /**
+   * The state service.
+   *
+   * @var \Drupal\Core\State\StateInterface
+   */
+  protected $state;
+
   /**
    * Constructs a CronUpdater object.
    *
@@ -86,15 +97,18 @@ class CronUpdater extends Updater {
    *   The mail manager service.
    * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    *   The language manager service.
+   * @param \Drupal\Core\State\StateInterface $state
+   *   The state service.
    * @param mixed ...$arguments
    *   Additional arguments to pass to the parent constructor.
    */
-  public function __construct(ReleaseChooser $release_chooser, LoggerChannelFactoryInterface $logger_factory, MailManagerInterface $mail_manager, LanguageManagerInterface $language_manager, ...$arguments) {
+  public function __construct(ReleaseChooser $release_chooser, LoggerChannelFactoryInterface $logger_factory, MailManagerInterface $mail_manager, LanguageManagerInterface $language_manager, StateInterface $state, ...$arguments) {
     parent::__construct(...$arguments);
     $this->releaseChooser = $release_chooser;
     $this->logger = $logger_factory->get('automatic_updates');
     $this->mailManager = $mail_manager;
     $this->languageManager = $language_manager;
+    $this->state = $state;
   }
 
   /**
@@ -162,9 +176,91 @@ class CronUpdater extends Updater {
     // stage regardless of whether the update succeeds.
     try {
       // @see ::begin()
-      parent::begin(['drupal' => $target_version], $timeout);
+      $stage_id = parent::begin(['drupal' => $target_version], $timeout);
       $this->stage();
       $this->apply();
+    }
+    catch (\Throwable $e) {
+      $this->logger->error($e->getMessage());
+
+      // If an error occurred during the pre-create event, the stage will be
+      // marked as available and we shouldn't try to destroy it, since the stage
+      // must be claimed in order to be destroyed.
+      if (!$this->isAvailable()) {
+        $this->destroy();
+      }
+      return;
+    }
+
+    // Perform a subrequest to run ::postApply(), which needs to be done in a
+    // separate request.
+    // @see parent::apply()
+    $url = Url::fromRoute('automatic_updates.cron.post_apply', [
+      'stage_id' => $stage_id,
+      'installed_version' => $installed_version,
+      'target_version' => $target_version,
+      'key' => $this->state->get('system.cron_key'),
+    ]);
+    $this->triggerPostApply($url);
+  }
+
+  /**
+   * Executes a subrequest to run post-apply tasks.
+   *
+   * @param \Drupal\Core\Url $url
+   *   The URL of the post-apply handler.
+   */
+  protected function triggerPostApply(Url $url): void {
+    $url = $url->setAbsolute()->toString();
+
+    // If we're using a single-threaded web server (e.g., the built-in PHP web
+    // server used in build tests), allow the post-apply request to be sent to
+    // an alternate port.
+    // @todo If using the built-in PHP web server, validate that this port is
+    //   set in https://www.drupal.org/i/3293146.
+    $port = $this->configFactory->get('automatic_updates.settings')
+      ->get('cron_port');
+    if ($port) {
+      $url = (string) (new GuzzleUri($url))->withPort($port);
+    }
+
+    // Use the bare cURL API to make the request, so that we're not relying on
+    // any third-party classes or other code which may have changed during the
+    // update.
+    $curl = curl_init($url);
+    curl_setopt($curl, CURLOPT_RETURNTRANSFER, TRUE);
+    $response = curl_exec($curl);
+    $status = curl_getinfo($curl, CURLINFO_RESPONSE_CODE);
+    if ($status !== 200) {
+      $this->logger->error('Post-apply tasks failed with output: %status %response', [
+        '%status' => $status,
+        '%response' => $response,
+      ]);
+    }
+    curl_close($curl);
+  }
+
+  /**
+   * Runs post-apply tasks.
+   *
+   * @param string $stage_id
+   *   The stage ID.
+   * @param string $installed_version
+   *   The version of Drupal core that started the update.
+   * @param string $target_version
+   *   The version of Drupal core to which we updated.
+   *
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   An empty 200 response if the post-apply tasks succeeded.
+   */
+  public function handlePostApply(string $stage_id, string $installed_version, string $target_version): Response {
+    $this->claim($stage_id);
+
+    // Run post-apply tasks in their own try-catch block so that, if anything
+    // raises an exception, we'll log it and proceed to destroy the stage as
+    // soon as possible (which is also what we do in ::performUpdate()).
+    try {
+      $this->postApply();
 
       $this->logger->info(
         'Drupal core has been updated from %previous_version to %target_version',
@@ -189,13 +285,6 @@ class CronUpdater extends Updater {
       $this->logger->error($e->getMessage());
     }
 
-    // If an error occurred during the pre-create event, the stage will be
-    // marked as available and we shouldn't try to destroy it, since the stage
-    // must be claimed in order to be destroyed.
-    if ($this->isAvailable()) {
-      return;
-    }
-
     // If any pre-destroy event subscribers raise validation errors, ensure they
     // are formatted and logged. But if any pre- or post-destroy event
     // subscribers throw another exception, don't bother catching it, since it
@@ -206,6 +295,8 @@ class CronUpdater extends Updater {
     catch (StageValidationException $e) {
       $this->logger->error($e->getMessage());
     }
+
+    return new Response();
   }
 
   /**

src/Form/UpdateReady.php

@@ -199,6 +199,7 @@ class UpdateReady extends FormBase {
       ->setTitle($this->t('Apply updates'))
       ->setInitMessage($this->t('Preparing to apply updates'))
       ->addOperation([BatchProcessor::class, 'commit'], [$stage_id])
+      ->addOperation([BatchProcessor::class, 'postApply'], [$stage_id])
       ->addOperation([BatchProcessor::class, 'clean'], [$stage_id])
       ->setFinishCallback([BatchProcessor::class, 'finishCommit'])
       ->toArray();

tests/modules/automatic_updates_test/src/TestController.php

@@ -32,6 +32,7 @@ class TestController extends ControllerBase {
       $updater->begin(['drupal' => $to_version]);
       $updater->stage();
       $updater->apply();
+      $updater->postApply();
       $updater->destroy();
 
       // The code base has been updated, but as far as the PHP runtime is