diff --git a/UPGRADE.md b/UPGRADE.md index 361737de..3c63b022 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -145,6 +145,27 @@ changement existe. `hasSideEffectForSlot()` étant désormais au port, ce cas re Une exécution qui a déjà écrit un marqueur de version garde le sien : `versionForChangeId()` est consulté en premier, et rien de ce commit ne le touche. +### Le profileur ne s'enregistre plus hors debug + +**Qui est concerné** : une application qui tirait `durable.execution_trace` du conteneur en +production, ou qui injectait `WorkflowExecutionObserverInterface` en s'attendant à la trace. + +Le collecteur, sa trace, son écouteur de remise à zéro et son middleware Messenger n'étaient posés +sous aucune condition. L'observateur qu'ils installent est injecté dans `ExecutionRuntime`, +`ExecutionEngine` et `ActivityMessageProcessor` : il passait donc sur le chemin chaud de chaque +exécution en production, pour alimenter une page que personne n'y sert. Et sa trace n'était vidée +que par un écouteur `kernel.request`, que `messenger:consume` ne déclenche jamais — un worker +l'accumulait tant qu'il vivait. + +Hors `kernel.debug`, `WorkflowExecutionObserverInterface` pointe désormais +`Gplanchat\Durable\Debug\NullWorkflowExecutionObserver`. Le contrat d'observation est intact ; +c'est son implémentation qui ne fait plus rien. En debug, rien ne change, sinon que la trace porte +un tag `kernel.reset` et se vide donc aussi entre deux messages d'un worker. + +Une application qui veut observer les exécutions en production n'a pas à ressusciter le profileur : +elle implémente `WorkflowExecutionObserverInterface` et aliase l'interface sur son propre service — +ce que le profileur faisait, en moins cher et sans accumuler une timeline pour l'écran de personne. + ## 0.1.0-alpha8 ### The divergence guard compares the payload too diff --git a/phpstan.neon b/phpstan.neon index 548ef88d..71f732e5 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -63,6 +63,13 @@ parameters: # de StubMethodsExtensionTest qui le diront. - identifier: phpstanApi.interface path: src/DurablePhpstan/Reflection/SchedulingMethodReflection.php + # `json_encode` appelle le `jsonSerialize()` de la valeur qu'on lui passe : du code + # applicatif, qui peut lever. PHPStan modélise la fonction comme incapable de lever, et + # déclare donc le `catch` mort. Il ne l'est pas — `RecordedDetailsStorableTest` le + # contredit en exécution, et sans ce `catch` l'exception remonte jusqu'à `collect()`, + # c'est-à-dire `kernel.response`, et emporte la requête. + - identifier: catch.neverThrown + path: src/Durable/Observation/RecordedDetails.php # DurableBundleTestTrait is consumed by application tests, not by library code - identifier: trait.unused path: src/DurableBundle/Testing/DurableBundleTestTrait.php diff --git a/src/Durable/Debug/NullWorkflowExecutionObserver.php b/src/Durable/Debug/NullWorkflowExecutionObserver.php new file mode 100644 index 00000000..24aa29ea --- /dev/null +++ b/src/Durable/Debug/NullWorkflowExecutionObserver.php @@ -0,0 +1,33 @@ + $short, 'recordedAt' => $recordedAt?->format(\DateTimeInterface::ATOM), - 'payload' => $event->payload(), + // The same barrier as the profiler: the command reads a production journal, + // and a payload that refuses encoding would bring down the very diagnosis + // one came for. + 'payload' => RecordedDetails::storable($event->payload()), ]; } } diff --git a/src/DurableBundle/DataCollector/DurableDataCollector.php b/src/DurableBundle/DataCollector/DurableDataCollector.php index 583da880..ccb1afcb 100644 --- a/src/DurableBundle/DataCollector/DurableDataCollector.php +++ b/src/DurableBundle/DataCollector/DurableDataCollector.php @@ -27,6 +27,7 @@ use Gplanchat\Durable\Event\WorkflowExecutionFailed; use Gplanchat\Durable\Event\WorkflowSignalReceived; use Gplanchat\Durable\Event\WorkflowUpdateHandled; +use Gplanchat\Durable\Observation\RecordedDetails; use Gplanchat\Durable\Store\EventStoreInterface; use Gplanchat\Durable\Store\WorkflowMetadataStore; use Symfony\Component\HttpFoundation\Request; @@ -112,6 +113,14 @@ public function collect(Request $request, Response $response, ?\Throwable $excep $grouped, ), ]; + + // The barrier, at the one place `$this->data` is built. It applies **key by key**: a + // pathological payload makes its own panel disappear, not the whole collector, which is + // what the blanket barrier did not guarantee, `$this->data` being typed + // `array|Data` chez le parent. + foreach ($this->data as $cle => $valeur) { + $this->data[$cle] = RecordedDetails::storable($valeur); + } } /** diff --git a/src/DurableBundle/DependencyInjection/DurableExtension.php b/src/DurableBundle/DependencyInjection/DurableExtension.php index 24d5852c..da30a208 100644 --- a/src/DurableBundle/DependencyInjection/DurableExtension.php +++ b/src/DurableBundle/DependencyInjection/DurableExtension.php @@ -44,6 +44,7 @@ use Gplanchat\Durable\Bundle\SchemaListener\DurableSchemaListener; use Gplanchat\Durable\Bundle\Transport\MessengerActivityTransport; use Gplanchat\Durable\Bundle\Transport\MessengerWorkflowTimerDispatcher; +use Gplanchat\Durable\Debug\NullWorkflowExecutionObserver; use Gplanchat\Durable\Debug\WorkflowExecutionObserverInterface; use Gplanchat\Durable\Handler\FireWorkflowTimersHandler; use Gplanchat\Durable\Handler\ResumeWorkflowHandler; @@ -76,6 +77,7 @@ use Gplanchat\Durable\Worker\ActivityMessageProcessor; use Gplanchat\Durable\Workflow\WorkflowDefinitionLoader; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\DependencyInjection\Extension\Extension; use Symfony\Component\DependencyInjection\Reference; use Temporal\Api\Workflowservice\V1\WorkflowServiceClient; @@ -95,7 +97,15 @@ public function load(array $configs, ContainerBuilder $container): void $asyncChildMessenger = (bool) ($config['child_workflow']['async_messenger'] ?? false); $container->setParameter('durable.child_workflow_async_messenger', $asyncChildMessenger); - $this->registerProfiler($container); + // A synthetic container, an extension test for instance, does not have this parameter; + // that does not make it production, hence the default to debug. + $debug = !$container->hasParameter('kernel.debug') || (bool) $container->getParameter('kernel.debug'); + + if ($debug) { + $this->registerProfiler($container); + } else { + $this->registerNullObserver($container); + } $this->registerChildWorkflowParentLinkStore($container); $this->registerWorkflowDefinitionLoader($container); $this->registerEventStore($container, $config); @@ -677,7 +687,11 @@ private function registerWorkflowMessengerServices(ContainerBuilder $container, new Reference(WorkflowClientInterface::class), new Reference(WorkflowMetadataStore::class), new Reference(WorkflowDefinitionLoader::class), - new Reference('durable.execution_trace'), + // Le profileur n'existe qu'en debug depuis ce correctif, et le constructeur + // target declares the dependency `?DurableExecutionTrace $executionTrace = null`. + // A bare reference would fail the production container's compilation as soon as + // a `temporal.dsn` is configured. + new Reference('durable.execution_trace', ContainerInterface::NULL_ON_INVALID_REFERENCE), ]) ->setPublic(true) ; @@ -760,16 +774,60 @@ private function registerCommands(ContainerBuilder $container, array $config): v ; } - private function registerProfiler(ContainerBuilder $container): void + /** + * The profiler is not neutral plumbing: its observer is injected into + * `ExecutionRuntime`, `ExecutionEngine` et `ActivityMessageProcessor`, donc il passe sur le + * hot path of every execution, and its trace is emptied only by a `kernel.request` listener, + * which `messenger:consume` never fires. + * + * Hors debug, on n'en enregistre donc rien du tout et l'observation retombe sur un objet nul. + * FrameworkBundle does the same for its own collectors, loaded from separate files under a + * condition. + */ + private function registerNullObserver(ContainerBuilder $container): void { - $container->register('durable.execution_trace', DurableExecutionTrace::class) + $container->register('durable.execution_observer.null', NullWorkflowExecutionObserver::class) + ->setPublic(false) + ; + + self::aliaserObservateur($container, 'durable.execution_observer.null'); + } + + /** + * Aliases the observation interface, **without overwriting what the application already + * declared**. + * + * `UPGRADE.md` invites an application that wants to observe its executions in production to + * implement the contract and alias the interface onto its own service. The definitions in the + * application's `services.yaml` already exist when the extension loads, since + * `MergeExtensionConfigurationPass` runs at compilation, after the configuration is loaded, so + * an unconditional `setAlias()` erased that alias and the escape hatch did not work. + */ + private static function aliaserObservateur(ContainerBuilder $container, string $service): void + { + if ($container->hasAlias(WorkflowExecutionObserverInterface::class) + || $container->hasDefinition(WorkflowExecutionObserverInterface::class) + ) { + return; + } + + $container->setAlias(WorkflowExecutionObserverInterface::class, $service) ->setPublic(true) ; + } - $container->setAlias(WorkflowExecutionObserverInterface::class, 'durable.execution_trace') + private function registerProfiler(ContainerBuilder $container): void + { + $container->register('durable.execution_trace', DurableExecutionTrace::class) + // `ResetDurableProfilerListener` ne borne que le cas HTTP. Dans un worker il n'y a pas + // request, and it is `services_resetter`, so this tag, that empties the trace between + // deux messages. Sans lui, un `messenger:consume` accumule la timeline tant qu'il vit. + ->addTag('kernel.reset', ['method' => 'reset']) ->setPublic(true) ; + self::aliaserObservateur($container, 'durable.execution_trace'); + $container->register(ResetDurableProfilerListener::class) ->setArguments([new Reference('durable.execution_trace')]) ->addTag('kernel.event_subscriber') diff --git a/tests/unit/Durable/Observation/RecordedDetailsStorableTest.php b/tests/unit/Durable/Observation/RecordedDetailsStorableTest.php new file mode 100644 index 00000000..fdb89f14 --- /dev/null +++ b/tests/unit/Durable/Observation/RecordedDetailsStorableTest.php @@ -0,0 +1,101 @@ + $piege])); + } + + /** + * Au-delà de 512 niveaux, `json_decode` rend `null` là où l'encodage avait produit du texte. + * La valeur disparaît — c'est assumé — mais l'appelant doit pouvoir ranger le résultat dans + * une propriété typée sans lever, d'où l'application clé par clé côté collecteur. + */ + public function testUneImbricationPlusProfondeQueJsonNeLeTientRendNull(): void + { + $profond = 'fond'; + for ($i = 0; $i < 600; ++$i) { + $profond = [$profond]; + } + + self::assertNull(RecordedDetails::storable($profond)); + } + + /** + * Les bornes de la frise se déclarent `float`. Sans `JSON_PRESERVE_ZERO_FRACTION`, une durée + * de trois secondes tout rondes revient en `int` et le type déclaré ment. + */ + public function testUnFlottantDeValeurEntiereResteUnFlottant(): void + { + $storable = RecordedDetails::storable(['spanSec' => 3.0, 'tMin' => 0.0]); + + self::assertIsArray($storable); + self::assertIsFloat($storable['spanSec']); + self::assertIsFloat($storable['tMin']); + } + + #[DataProvider('chargesUtilesOrdinaires')] + public function testCeQuiEtaitLisibleLeResteALIdentique(mixed $valeur): void + { + self::assertSame($valeur, RecordedDetails::storable($valeur)); + } + + /** + * @return iterable + */ + public static function chargesUtilesOrdinaires(): iterable + { + yield 'chaîne' => ['bonjour']; + yield 'entier' => [42]; + yield 'flottant' => [1.5]; + yield 'booléen' => [true]; + yield 'null' => [null]; + yield 'liste' => [[1, 2, 3]]; + yield 'tableau associatif' => [['a' => 1, 'b' => ['c' => 'd']]]; + } + + /** + * Une référence récursive, elle, survit : `JSON_PARTIAL_OUTPUT_ON_ERROR` la coupe et rend le + * reste. Le cas est ici pour qu'on cesse de le croire cassé. + */ + public function testUneReferenceRecursiveEstTronqueeEtNonPerdue(): void + { + $objet = new \stdClass(); + $objet->nom = 'boucle'; + $objet->soi = $objet; + + $storable = RecordedDetails::storable(['payload' => $objet]); + + self::assertIsArray($storable); + self::assertSame('boucle', $storable['payload']['nom']); + } +} diff --git a/tests/unit/DurableBundle/DependencyInjection/DurableProfilerWiringTest.php b/tests/unit/DurableBundle/DependencyInjection/DurableProfilerWiringTest.php new file mode 100644 index 00000000..d6a0163e --- /dev/null +++ b/tests/unit/DurableBundle/DependencyInjection/DurableProfilerWiringTest.php @@ -0,0 +1,210 @@ +load(debug: true)->getDefinition('durable.execution_trace'); + + self::assertArrayHasKey( + 'kernel.reset', + $definition->getTags(), + "sans ce tag, services_resetter ignore la trace et un worker l'accumule sans borne", + ); + self::assertSame( + 'reset', + $definition->getTag('kernel.reset')[0]['method'] ?? null, + 'et le resetter a besoin du nom de la méthode', + ); + } + + public function testHorsDebugAucunCollecteurNEstEnregistre(): void + { + $container = $this->load(debug: false); + + self::assertFalse( + $container->has('durable.execution_trace'), + 'la trace de profil n\'a rien à faire en production', + ); + + foreach ($container->getDefinitions() as $id => $definition) { + self::assertArrayNotHasKey( + 'data_collector', + $definition->getTags(), + \sprintf('%s ne doit pas collecter hors debug', $id), + ); + } + } + + /** + * Le contrat d'observation reste satisfait : les trois services du chemin chaud le reçoivent + * en injection, et un conteneur qui ne le fournirait pas ne compilerait plus. + */ + public function testHorsDebugLObservateurEstUnObjetNul(): void + { + $container = $this->load(debug: false); + + self::assertTrue($container->hasAlias(WorkflowExecutionObserverInterface::class)); + + $target = (string) $container->getAlias(WorkflowExecutionObserverInterface::class); + self::assertSame( + \Gplanchat\Durable\Debug\NullWorkflowExecutionObserver::class, + $container->getDefinition($target)->getClass(), + ); + } + + public function testEnDebugLObservateurEstBienLaTrace(): void + { + $container = $this->load(debug: true); + + self::assertSame( + 'durable.execution_trace', + (string) $container->getAlias(WorkflowExecutionObserverInterface::class), + ); + } + + /** + * Retirer le profileur de la production ne suffit pas : il faut que plus rien ne le réclame. + * + * `TemporalWorkflowResumeDispatcher` recevait `durable.execution_trace` par une référence nue. + * Le service n'étant plus enregistré hors debug, le conteneur d'une application de production + * configurée en Temporal natif ne compilait plus — et aucun test ne le voyait, tous chargeant + * une configuration vide, donc sans jamais construire cette branche. + */ + public function testHorsDebugUneApplicationTemporaleCompileEncore(): void + { + $container = $this->load(debug: false, config: ['temporal' => ['dsn' => self::DSN]]); + + $arguments = $container->getDefinition(WorkflowResumeDispatcher::class)->getArguments(); + $trace = $arguments[3] ?? null; + + self::assertInstanceOf(Reference::class, $trace); + self::assertSame('durable.execution_trace', (string) $trace); + self::assertSame( + ContainerInterface::NULL_ON_INVALID_REFERENCE, + $trace->getInvalidBehavior(), + 'une référence nue vers un service absent hors debug fait échouer la compilation', + ); + + self::assertNotContains( + 'durable.execution_trace', + self::servicesManquants($container), + 'le conteneur de production ne doit plus réclamer un service que le debug seul enregistre', + ); + } + + public function testEnDebugLeMemeConteneurRecoitLaVraieTrace(): void + { + $container = $this->load(debug: true, config: ['temporal' => ['dsn' => self::DSN]]); + + self::assertTrue($container->has('durable.execution_trace')); + self::assertSame( + 'durable.execution_trace', + (string) $container->getDefinition(WorkflowResumeDispatcher::class)->getArgument(3), + ); + } + + /** + * `UPGRADE.md` prescrit cette échappatoire aux applications qui veulent observer en + * production : implémenter le contrat, aliaser l'interface. Elle ne fonctionnait pas — le + * `setAlias()` de l'extension écrasait celui de l'application, dont les définitions sont + * pourtant déjà là quand l'extension se charge. + */ + #[DataProvider('environnements')] + public function testUnAliasDeLApplicationNEstPasEcrase(bool $debug): void + { + $container = new ContainerBuilder(); + $container->setParameter('kernel.debug', $debug); + $container->register('app.observateur', \stdClass::class); + $container->setAlias(WorkflowExecutionObserverInterface::class, 'app.observateur'); + + (new DurableExtension())->load([[]], $container); + + self::assertSame( + 'app.observateur', + (string) $container->getAlias(WorkflowExecutionObserverInterface::class), + ); + } + + /** + * @return iterable + */ + public static function environnements(): iterable + { + yield 'debug' => [true]; + yield 'production' => [false]; + } + + /** + * Les identifiants qu'un conteneur réclame sans les avoir. + * + * Le bundle seul ne compile pas : il référence légitimement des services que FrameworkBundle + * fournit (`messenger.default_bus`, …). On déclare donc chaque manquant en synthétique et on + * recommence, jusqu'à ce que la passe amont passe — ce qui reste est la liste exacte de ce + * que le bundle attend de l'extérieur. Un service **à nous** dans cette liste est un bug. + * + * @return list + */ + private static function servicesManquants(ContainerBuilder $container): array + { + $manquants = []; + $passe = new CheckExceptionOnInvalidReferenceBehaviorPass(); + + for ($i = 0; $i < 100; ++$i) { + try { + $passe->process($container); + + return $manquants; + } catch (ServiceNotFoundException $e) { + $id = $e->getId(); + if (null === $id || \in_array($id, $manquants, true)) { + throw $e; + } + $manquants[] = $id; + $container->register($id, \stdClass::class)->setSynthetic(true); + } + } + + self::fail('la passe de vérification ne converge pas'); + } + + /** + * @param array $config + */ + private function load(bool $debug, array $config = []): ContainerBuilder + { + $container = new ContainerBuilder(); + $container->setParameter('kernel.debug', $debug); + (new DurableExtension())->load([$config], $container); + + return $container; + } +} diff --git a/tests/unit/DurableBundle/DurableDataCollectorSerialisationTest.php b/tests/unit/DurableBundle/DurableDataCollectorSerialisationTest.php new file mode 100644 index 00000000..d1ed2055 --- /dev/null +++ b/tests/unit/DurableBundle/DurableDataCollectorSerialisationTest.php @@ -0,0 +1,121 @@ + + */ + public static function valeursQuiNeSeSerialisentPas(): iterable + { + yield 'closure' => [static fn(): int => 1]; + yield 'ressource' => [fopen('php://memory', 'rb')]; + yield 'objet anonyme portant une closure' => [new class { + public \Closure $callback; + + public function __construct() + { + $this->callback = static fn(): int => 1; + } + }]; + } + + #[DataProvider('valeursQuiNeSeSerialisentPas')] + public function testLeProfilResteStockableQuoiQueLaChargeUtilePorte(mixed $valeur): void + { + $collector = self::collectorAyantObserve(['commande' => 'X-1', 'hostile' => $valeur]); + + $serialise = serialize($collector); + + self::assertIsString($serialise); + self::assertInstanceOf(DurableDataCollector::class, unserialize($serialise)); + } + + /** + * Le reste de la charge utile est ce que l'exploitant est venu lire ; une valeur qui ne se + * rend pas ne doit pas l'emporter avec elle. + */ + public function testCeQuiEstLisibleDansLaChargeUtileEstConserve(): void + { + $collector = self::collectorAyantObserve([ + 'commande' => 'X-1', + 'montant' => 1250, + 'hostile' => static fn(): int => 1, + ]); + + $rendu = json_encode(unserialize(serialize($collector))->getTimeline()); + + self::assertStringContainsString('X-1', (string) $rendu); + self::assertStringContainsString('1250', (string) $rendu); + } + + /** + * Le journal peut porter des octets qui ne sont pas du texte valide. `json_encode` rend alors + * `false`, et le gabarit affiche un vide là où il y avait une charge utile. + */ + public function testUneChargeUtileBinaireResteAffichable(): void + { + $collector = self::collectorAyantObserve(['blob' => "\xB1\x31\xFE"]); + + $rendu = json_encode(unserialize(serialize($collector))->getTimeline()); + + self::assertIsString($rendu, 'une charge utile binaire ne doit pas rendre le panneau vide'); + } + + /** + * La barrière ne doit rien déformer de ce qui passait déjà : le gabarit lit des clés précises, + * et un aller-retour JSON qui transformerait une liste en objet les casserait en silence. + */ + public function testUneChargeUtileOrdinaireTraverseSansEtreDeformee(): void + { + $payload = [ + 'commande' => 'X-1', + 'montant' => 1250, + 'remise' => 0.15, + 'urgent' => false, + 'lignes' => ['a', 'b'], + 'client' => ['id' => 7, 'nom' => 'Dupont'], + 'note' => null, + ]; + + $timeline = self::collectorAyantObserve($payload)->getTimeline(); + + self::assertSame($payload, $timeline[0]['payload'] ?? null); + } + + /** + * @param array $payload + */ + private static function collectorAyantObserve(array $payload): DurableDataCollector + { + $trace = new DurableExecutionTrace(); + $trace->onWorkflowDispatchRequested('exec-1', 'Commande', $payload, false, 'async'); + + $collector = new DurableDataCollector($trace, new InMemoryWorkflowMetadataStore(), new InMemoryEventStore()); + $collector->collect(new Request(), new Response()); + + return $collector; + } +}