diff --git a/CHANGELOG.md b/CHANGELOG.md index dfac53b7..20585412 100644 --- a/CHANGELOG.md +++ b/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 ----- diff --git a/config/services/manager.yaml b/config/services/manager.yaml index 1739b5f7..a1fe6cfd 100644 --- a/config/services/manager.yaml +++ b/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 diff --git a/src/Manager/ProcessManager.php b/src/Manager/ProcessManager.php index 0ebf3e51..9b02d16a 100644 --- a/src/Manager/ProcessManager.php +++ b/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; diff --git a/tests/DependencyInjection/CleverAgeProcessExtensionTest.php b/tests/DependencyInjection/CleverAgeProcessExtensionTest.php index fce0c464..a0f5d545 100644 --- a/tests/DependencyInjection/CleverAgeProcessExtensionTest.php +++ b/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') + ); + } } diff --git a/tests/Manager/ProcessManagerProcessHistoryTest.php b/tests/Manager/ProcessManagerProcessHistoryTest.php new file mode 100644 index 00000000..6891f5d7 --- /dev/null +++ b/tests/Manager/ProcessManagerProcessHistoryTest.php @@ -0,0 +1,208 @@ +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> $tasks + * + * @return array + */ + private function createProcessConfiguration(array $tasks): array + { + return [ + 'options' => [], + 'entry_point' => null, + 'end_point' => null, + 'description' => '', + 'help' => '', + 'public' => true, + 'tasks' => $tasks, + ]; + } + + /** + * @param array $options + * @param list $outputs + * + * @return array + */ + 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, + ]; + } +} diff --git a/tests/Manager/ProcessManagerStopCheckerTest.php b/tests/Manager/ProcessManagerStopCheckerTest.php index 927d0476..a1fea719 100644 --- a/tests/Manager/ProcessManagerStopCheckerTest.php +++ b/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()),