#301 - ProcessManager: restore the parent process history after a failed process, and reset it between executions

CHANGELOG.md

@@ -18,6 +18,9 @@ Latest
 * [#282](https://github.com/cleverage/process-bundle/issues/282) Require `psr/container` (`ProcessHelpCommand`), `ext-xmlreader` (`XmlStreamReaderTask`) and `ext-libxml` (`XmlFile`, `XmlStreamReaderTask`, `XpathEvaluatorTransformer`), used directly; add `symfony/cache` (used by the tests) to the dev requirements.
 * [#268](https://github.com/cleverage/process-bundle/issues/268) Clean up composer.json: remove the unused `symfony/dotenv`, `symfony/runtime` and `symfony/error-handler` dependencies (applications relying on the bundle to install them must require them), bound `symfony/service-contracts` to `^2.5|^3`, update the package description.
 
+## Fixes
+* [#301](https://github.com/cleverage/process-bundle/issues/301) Fix ProcessManager: the history of the parent process is restored after a failed process too, so that the logs of a parent process going on after a failed sub-process (e.g. `process_executor` task with the `skip` error strategy) are no longer attributed to the sub-process (`process_id`, `process_code` and `process_context` added by the Monolog processors), and that no history of a failed process is left after its execution (`getProcessHistory()`). The `Critical process failure` log and the `cleverage_process.fail` event are still attributed to the failed process. `ProcessManager` implements `ResetInterface` (`kernel.reset` tag): its state is reset between two messages of a long-running worker. Add tests.
+
 v5.3
 -----
 

config/services/manager.yaml

@@ -11,6 +11,8 @@ services:
             - '@event_dispatcher'
             - !tagged_iterator cleverage_process.stop_checker
             - '@cleverage_process.registry.task'
+        tags:
+            - { name: kernel.reset, method: reset }
 
     CleverAge\ProcessBundle\Manager\ProcessManager:
         alias: cleverage_process.manager.process

src/Manager/ProcessManager.php

@@ -35,13 +35,14 @@
 use CleverAge\ProcessBundle\Registry\TaskRegistry;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+use Symfony\Contracts\Service\ResetInterface;
 
 /**
  * Execute processes.
  *
  * @final since 5.4
  */
-class ProcessManager
+class ProcessManager implements ResetInterface
 {
     protected const EXECUTE_PROCESS = 1;
 
@@ -92,6 +93,17 @@ public function __construct(
         $this->taskRegistry = $taskRegistry;
     }
 
+    /**
+     * Forget the state of the previous executions, e.g. between two messages of a long-running worker.
+     */
+    public function reset(): void
+    {
+        $this->processedIterables = [];
+        $this->processedBlockings = [];
+        $this->processHistory = null;
+        $this->taskConfiguration = null;
+    }
+
     public function getProcessHistory(): ?ProcessHistory
     {
         return $this->processHistory;
@@ -113,6 +125,7 @@ public function getTaskConfiguration(): ?TaskConfiguration
      */
     public function execute(string $processCode, mixed $input = null, array $context = []): mixed
     {
+        $parentProcessHistory = $this->processHistory;
         try {
             $this->eventDispatcher->dispatch(
                 new ProcessEvent($processCode, $input, $context),
@@ -137,6 +150,10 @@ public function execute(string $processCode, mixed $input = null, array $context
             );
 
             throw $error;
+        } finally {
+            // When the process has failed, doExecute() did not restore the history of the parent process (if any): the
+            // logs above are still attributed to the failed process, the next ones to the parent process
+            $this->processHistory = $parentProcessHistory;
         }
 
         return $result;

tests/DependencyInjection/CleverAgeProcessExtensionTest.php

@@ -180,4 +180,15 @@ public function testStopCheckersAreInjectedInTheProcessManager(): void
         self::assertInstanceOf(TaggedIteratorArgument::class, $argument);
         self::assertSame('cleverage_process.stop_checker', $argument->getTag());
     }
+
+    public function testTheProcessManagerIsResetBetweenExecutions(): void
+    {
+        $container = new ContainerBuilder();
+        (new CleverAgeProcessExtension())->load([], $container);
+
+        self::assertSame(
+            [['method' => 'reset']],
+            $container->getDefinition('cleverage_process.manager.process')->getTag('kernel.reset')
+        );
+    }
 }

tests/Manager/ProcessManagerProcessHistoryTest.php

@@ -0,0 +1,208 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Manager;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Event\ProcessEvent;
+use CleverAge\ProcessBundle\Exception\ProcessFailedException;
+use CleverAge\ProcessBundle\Logger\AbstractLogger;
+use CleverAge\ProcessBundle\Logger\ProcessLogger;
+use CleverAge\ProcessBundle\Logger\TaskLogger;
+use CleverAge\ProcessBundle\Manager\ProcessManager;
+use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Model\TaskInterface;
+use CleverAge\ProcessBundle\Registry\ProcessConfigurationRegistry;
+use CleverAge\ProcessBundle\Registry\TaskRegistry;
+use CleverAge\ProcessBundle\Task\ConstantOutputTask;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\EventDispatcher\EventDispatcher;
+
+/**
+ * The process history returned by ProcessManager::getProcessHistory() (used by the log processors) after a failure.
+ */
+#[\PHPUnit\Framework\Attributes\CoversClass(ProcessManager::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessFailedException::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskRegistry::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessEvent::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractLogger::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfigurationRegistry::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ConstantOutputTask::class)]
+class ProcessManagerProcessHistoryTest extends TestCase
+{
+    private Container $container;
+
+    private ProcessManager $processManager;
+
+    /**
+     * What the tasks saw of the process manager history while the process was running.
+     */
+    private mixed $observedHistory = null;
+
+    protected function setUp(): void
+    {
+        $this->container = new Container();
+        $this->container->set('test.constant', new ConstantOutputTask());
+        $this->container->set('test.failing', new class implements TaskInterface {
+            public function execute(ProcessState $state): void
+            {
+                throw new \LogicException('Something went wrong');
+            }
+        });
+        // Runs a failing sub-process and goes on, as the process_executor task with the skip error strategy
+        $this->container->set('test.sub_process', new class($this) implements TaskInterface {
+            public function __construct(
+                private readonly ProcessManagerProcessHistoryTest $test,
+            ) {
+            }
+
+            public function execute(ProcessState $state): void
+            {
+                $this->test->executeSubProcess();
+            }
+        });
+
+        $registry = new ProcessConfigurationRegistry(
+            [
+                'test.parent' => $this->createProcessConfiguration([
+                    'entry' => $this->createTaskConfiguration('@test.constant', ['output' => 'value'], ['sub_process']),
+                    'sub_process' => $this->createTaskConfiguration('@test.sub_process'),
+                ]),
+                'test.reset' => $this->createProcessConfiguration([
+                    'reset' => $this->createTaskConfiguration('@test.reset'),
+                ]),
+                'test.failing' => $this->createProcessConfiguration([
+                    'entry' => $this->createTaskConfiguration('@test.constant', ['output' => 'value'], ['fail']),
+                    'fail' => $this->createTaskConfiguration('@test.failing'),
+                ]),
+            ],
+            'stop'
+        );
+
+        $this->processManager = new ProcessManager(
+            $this->container,
+            new ProcessLogger(new NullLogger()),
+            new TaskLogger(new NullLogger()),
+            $registry,
+            new ContextualOptionResolver(),
+            new EventDispatcher(),
+            taskRegistry: new TaskRegistry($this->container),
+        );
+    }
+
+    public function executeSubProcess(): void
+    {
+        try {
+            $this->processManager->execute('test.failing');
+        } catch (ProcessFailedException) {
+        }
+        $this->observedHistory = $this->processManager->getProcessHistory()?->getProcessCode();
+    }
+
+    public function testTheParentHistoryIsRestoredAfterAFailedSubProcess(): void
+    {
+        $this->processManager->execute('test.parent');
+
+        self::assertSame('test.parent', $this->observedHistory);
+        self::assertNull($this->processManager->getProcessHistory());
+    }
+
+    public function testNoHistoryIsLeftAfterAFailedProcess(): void
+    {
+        try {
+            $this->processManager->execute('test.failing');
+            self::fail('The process should have failed');
+        } catch (ProcessFailedException) {
+        }
+
+        self::assertNull($this->processManager->getProcessHistory());
+    }
+
+    public function testResetForgetsTheCurrentExecution(): void
+    {
+        $this->container->set('test.reset', new class($this) implements TaskInterface {
+            public function __construct(
+                private readonly ProcessManagerProcessHistoryTest $test,
+            ) {
+            }
+
+            public function execute(ProcessState $state): void
+            {
+                $this->test->resetProcessManager();
+            }
+        });
+
+        $this->processManager->execute('test.reset');
+
+        self::assertSame(['test.reset', null], $this->observedHistory);
+    }
+
+    public function resetProcessManager(): void
+    {
+        $before = $this->processManager->getProcessHistory()?->getProcessCode();
+        $this->processManager->reset();
+        $this->observedHistory = [$before, $this->processManager->getProcessHistory()];
+        self::assertNull($this->processManager->getTaskConfiguration());
+    }
+
+    /**
+     * @param array<string, array<string, mixed>> $tasks
+     *
+     * @return array<string, mixed>
+     */
+    private function createProcessConfiguration(array $tasks): array
+    {
+        return [
+            'options' => [],
+            'entry_point' => null,
+            'end_point' => null,
+            'description' => '',
+            'help' => '',
+            'public' => true,
+            'tasks' => $tasks,
+        ];
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     * @param list<string>         $outputs
+     *
+     * @return array<string, mixed>
+     */
+    private function createTaskConfiguration(string $service, array $options = [], array $outputs = []): array
+    {
+        return [
+            'service' => $service,
+            'options' => $options,
+            'description' => '',
+            'help' => '',
+            'outputs' => $outputs,
+            'errors' => [],
+            'error_outputs' => [],
+            'error_strategy' => null,
+            'log_level' => null,
+        ];
+    }
+}

tests/Manager/ProcessManagerStopCheckerTest.php

@@ -63,6 +63,13 @@ class ProcessManagerStopCheckerTest extends TestCase
 
     private ?\Throwable $failedEventError = null;
 
+    private ?ProcessManager $processManager = null;
+
+    /**
+     * Whether the history of the process manager was failed when the "failed" event was dispatched.
+     */
+    private ?bool $failedEventHistoryIsFailed = null;
+
     public function testWithoutStopCheckerTheProcessRunsEntirely(): void
     {
         $this->createProcessManager([])->execute('test.process');
@@ -107,7 +114,9 @@ public function testStopRequestedDuringTheIteration(): void
 
         self::assertSame([1, 2], $this->recorded);
         self::assertSame($exception, $this->failedEventError);
-        self::assertTrue($processManager->getProcessHistory()?->isFailed());
+        self::assertTrue($this->failedEventHistoryIsFailed);
+        // The history of the failed process is not kept once it has ended
+        self::assertNull($processManager->getProcessHistory());
     }
 
     public function testAnyCheckerCanRequestAStop(): void
@@ -192,9 +201,10 @@ public function execute(ProcessState $state): void
         $eventDispatcher = new EventDispatcher();
         $eventDispatcher->addListener(ProcessEvent::EVENT_PROCESS_FAILED, function (ProcessEvent $event): void {
             $this->failedEventError = $event->getProcessError();
+            $this->failedEventHistoryIsFailed = $this->processManager?->getProcessHistory()?->isFailed();
         });
 
-        return new ProcessManager(
+        return $this->processManager = new ProcessManager(
             $container,
             new ProcessLogger(new NullLogger()),
             new TaskLogger(new NullLogger()),