From bdb391d1d513faf060e1ba694bf22fd449c42f5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Planchat?= Date: Fri, 4 Sep 2026 00:32:19 +0200 Subject: [PATCH] =?UTF-8?q?fix(bundle):=20d=C3=A9clarer=20et=20v=C3=A9rifi?= =?UTF-8?q?er=20ce=20que=20le=20bundle=20utilise=20r=C3=A9ellement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trois formes du même défaut : le bundle accepte ce qu'il ne peut pas tenir, et rien ne le dit avant l'incident. **Le pool de cache configuré était jeté en silence.** `hasDefinition($cacheId)` répond faux pour un alias — et `Psr\Cache\CacheItemPoolInterface` en est un — comme pour une définition posée par une extension qui tourne après celle-ci. L'exploitant écrivait `activity_contracts.cache: mon.pool`, rien n'était câblé, rien ne le signalait. La référence est désormais posée sans condition : un pool qui n'existe pas devient une erreur de compilation, que le conteneur sait nommer. **Et le résolveur re-réfléchissait à chaque appel.** Le pool vaut `null` par défaut, donc le cache warmer ne réchauffait rien et chaque appel d'activité refaisait la réflexion sur le contrat. Ces métadonnées dérivent des attributs, donc du code : elles ne peuvent pas changer tant que le processus vit. Une mémoire par instance les sert, et court-circuite aussi le pool — sur un Redis, c'était un aller-retour réseau par appel d'activité. **`lock.factory` manquant ne disait pas quoi configurer.** Le conteneur échouait déjà, sur un « service inexistant » qui nomme `lock.factory` et laisse chercher. Une passe le dit : quelle section pose ce service, et pourquoi elle n'est pas optionnelle ici — sans verrou, deux workers rejouent le même journal en même temps. Vérifié dans une passe et non dans l'extension : au chargement des extensions, celle qui pose `lock.factory` n'a pas forcément tourné. **Les deux ponts entrent dans `suggest`.** L'extension importe seize classes du pont Temporal et sept du pont DBAL ; un `composer require` du seul bundle donnait un conteneur qui compile et un fatal au premier appel. `DurableMiddlewareReachesTheBusTest` montait un conteneur avec le verrou DBAL et sans fabrique. La passe le refuse maintenant, à raison : une application dans cet état ne démarrerait pas. Le conteneur du test pose donc `lock.factory`, comme il posait déjà `messenger.bus.default` pour la même raison. Suite unit : 1081 tests contre 1073 sur main, mêmes 4 erreurs d'environnement. PHPStan : 2 erreurs, la base de main. Refs: M14, M15 et M19 de documentation/audit/ Co-Authored-By: Claude Opus 5 (1M context) --- .../Activity/ActivityContractResolver.php | 20 ++- .../Compiler/RequireLockFactoryPass.php | 54 ++++++++ .../DependencyInjection/DurableExtension.php | 9 +- src/DurableBundle/DurableBundle.php | 5 + src/DurableBundle/composer.json | 4 +- .../ActivityContractResolverMemoTest.php | 92 ++++++++++++++ .../DurableMiddlewareReachesTheBusTest.php | 5 + .../DurableDeclaredWiringTest.php | 117 ++++++++++++++++++ 8 files changed, 300 insertions(+), 6 deletions(-) create mode 100644 src/DurableBundle/DependencyInjection/Compiler/RequireLockFactoryPass.php create mode 100644 tests/unit/Durable/Activity/ActivityContractResolverMemoTest.php create mode 100644 tests/unit/DurableBundle/DependencyInjection/DurableDeclaredWiringTest.php diff --git a/src/Durable/Activity/ActivityContractResolver.php b/src/Durable/Activity/ActivityContractResolver.php index ca21c590..893fc566 100644 --- a/src/Durable/Activity/ActivityContractResolver.php +++ b/src/Durable/Activity/ActivityContractResolver.php @@ -18,6 +18,18 @@ final class ActivityContractResolver private const CACHE_PREFIX = 'durable.activity_contract.'; private const CACHE_TTL = 3600; + /** + * Les métadonnées déjà résolues dans ce processus. + * + * Elles dérivent des attributs, donc du code : elles ne peuvent pas changer tant que le + * processus vit. Sans cette mémoire, un résolveur sans pool — et le pool est `null` par défaut — + * refait la réflexion à chaque appel d'activité, et un résolveur avec pool refait un + * aller-retour au pool, qui sur un Redis est un aller-retour réseau. + * + * @var array> + */ + private array $resolved = []; + public function __construct( private readonly ?CacheItemPoolInterface $cache = null, ) {} @@ -29,12 +41,16 @@ public function __construct( */ public function resolveActivityMethods(string $contractClass): array { + if (isset($this->resolved[$contractClass])) { + return $this->resolved[$contractClass]; + } + $cacheKey = self::CACHE_PREFIX . str_replace('\\', '_', $contractClass); if (null !== $this->cache) { $item = $this->cache->getItem($cacheKey); if ($item->isHit()) { - return $item->get(); + return $this->resolved[$contractClass] = $item->get(); } } @@ -47,7 +63,7 @@ public function resolveActivityMethods(string $contractClass): array $this->cache->save($item); } - return $result; + return $this->resolved[$contractClass] = $result; } /** diff --git a/src/DurableBundle/DependencyInjection/Compiler/RequireLockFactoryPass.php b/src/DurableBundle/DependencyInjection/Compiler/RequireLockFactoryPass.php new file mode 100644 index 00000000..f0046a2b --- /dev/null +++ b/src/DurableBundle/DependencyInjection/Compiler/RequireLockFactoryPass.php @@ -0,0 +1,54 @@ +hasDefinition(self::LOCK_SERVICE)) { + return; + } + + // Le service peut avoir été redéfini sans argument par l'application ; on retombe alors sur + // le nom conventionnel plutôt que d'échouer sur la lecture de l'argument. + $arguments = $container->getDefinition(self::LOCK_SERVICE)->getArguments(); + $factory = (string) ($arguments[0] ?? 'lock.factory'); + + if ($container->has($factory)) { + return; + } + + throw new \LogicException(\sprintf( + 'durable: le backend DBAL sérialise les reprises d\'une même exécution avec un verrou, ' + . 'et le service "%s" qui le fournit n\'existe pas. Activez le composant Lock — ' + . '`framework.lock: true` dans config/packages/framework.yaml, ou une entrée `framework.lock.resources` ' + . 'pointant un magasin partagé entre vos processus — ou nommez votre propre fabrique dans ' + . '`durable.dbal.lock_factory`. Sans verrou, deux workers rejouent le même journal en même temps.', + $factory, + )); + } +} diff --git a/src/DurableBundle/DependencyInjection/DurableExtension.php b/src/DurableBundle/DependencyInjection/DurableExtension.php index f7aba215..ebd67568 100644 --- a/src/DurableBundle/DependencyInjection/DurableExtension.php +++ b/src/DurableBundle/DependencyInjection/DurableExtension.php @@ -500,9 +500,12 @@ private function registerActivityContractResolver(ContainerBuilder $container, a { $activityConfig = $config['activity_contracts'] ?? []; $cacheId = $activityConfig['cache'] ?? null; - $cacheRef = null !== $cacheId && $container->hasDefinition($cacheId) - ? new Reference($cacheId) - : null; + // Pas de `hasDefinition()` ici : un alias n'en est pas une — `Psr\Cache\CacheItemPoolInterface` + // en est un — et une définition posée par une extension qui tourne après celle-ci n'en est + // pas encore une. Le test rendait donc faux pour des configurations parfaitement valides, et + // le pool demandé était jeté sans un mot. Référencer sans condition rend l'erreur au + // compilateur, qui sait dire quel service manque. + $cacheRef = null !== $cacheId ? new Reference($cacheId) : null; $container->register(ActivityContractResolver::class, ActivityContractResolver::class) ->setArguments([$cacheRef]) diff --git a/src/DurableBundle/DurableBundle.php b/src/DurableBundle/DurableBundle.php index 2c9feaca..c66b83c9 100644 --- a/src/DurableBundle/DurableBundle.php +++ b/src/DurableBundle/DurableBundle.php @@ -11,6 +11,7 @@ use Gplanchat\Durable\Bundle\DependencyInjection\Compiler\DurableTemporalTransportFactoryPass; use Gplanchat\Durable\Bundle\DependencyInjection\Compiler\NexusHandlerPass; use Gplanchat\Durable\Bundle\DependencyInjection\Compiler\RegisterDurableMiddlewarePass; +use Gplanchat\Durable\Bundle\DependencyInjection\Compiler\RequireLockFactoryPass; use Gplanchat\Durable\Bundle\DependencyInjection\Compiler\WorkflowPass; use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\Compiler\PassConfig; @@ -56,6 +57,10 @@ static function (ChildDefinition $definition, FulfilsNexusOperation $attribute, $container->addCompilerPass(new ActivityHandlerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 50); // Même priorité, même raison : après l'autoconfiguration par attribut, avant les passes à 0. $container->addCompilerPass(new NexusHandlerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 50); + // Après l'enregistrement des services DBAL, avant que le conteneur ne se plaigne d'un + // service inexistant : le message de la passe dit quoi configurer, pas seulement quoi manque. + $container->addCompilerPass(new RequireLockFactoryPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, 20); + // Après tous les passes d'autowiring : injecte TemporalActivityWorker dans TemporalTransportFactory. $container->addCompilerPass(new DurableTemporalTransportFactoryPass(), PassConfig::TYPE_BEFORE_REMOVING); } diff --git a/src/DurableBundle/composer.json b/src/DurableBundle/composer.json index bfff38ff..62b7cb32 100644 --- a/src/DurableBundle/composer.json +++ b/src/DurableBundle/composer.json @@ -30,7 +30,9 @@ "suggest": { "phpunit/phpunit": "Required by the shipped test helper (Testing\\DurableBundleTestTrait)", "symfony/framework-bundle": "Required by consumers of the shipped test helper (Testing\\DurableBundleTestTrait), whose static::getContainer() resolves against a KernelTestCase", - "symfony/web-profiler-bundle": "Web Debug Toolbar and profiler: Durable panel (workflows / activities)" + "symfony/web-profiler-bundle": "Web Debug Toolbar and profiler: Durable panel (workflows / activities)", + "gplanchat/durable-bridge-temporal": "Temporal backend: DurableExtension wires its client, stores and worker transports when durable.temporal.dsn is set", + "gplanchat/durable-bridge-dbal": "SQL backend: DurableExtension wires its journal, stores and resume lock when event_store.type is dbal" }, "autoload": { "psr-4": { diff --git a/tests/unit/Durable/Activity/ActivityContractResolverMemoTest.php b/tests/unit/Durable/Activity/ActivityContractResolverMemoTest.php new file mode 100644 index 00000000..5df6331f --- /dev/null +++ b/tests/unit/Durable/Activity/ActivityContractResolverMemoTest.php @@ -0,0 +1,92 @@ + */ + private array $values = []; + + public function getItem(string $key): CacheItemInterface + { + ++$this->getItemCalls; + $values = &$this->values; + + return new class($key, $values) implements CacheItemInterface { + /** @param array $values */ + public function __construct(private string $key, private array &$values) {} + public function getKey(): string { return $this->key; } + public function get(): mixed { return $this->values[$this->key] ?? null; } + public function isHit(): bool { return \array_key_exists($this->key, $this->values); } + public function set(mixed $value): static { $this->values[$this->key] = $value; return $this; } + public function expiresAt(?\DateTimeInterface $expiration): static { return $this; } + public function expiresAfter(\DateInterval|int|null $time): static { return $this; } + }; + } + + public function getItems(array $keys = []): iterable { return []; } + public function hasItem(string $key): bool { return false; } + public function clear(): bool { return true; } + public function deleteItem(string $key): bool { return true; } + public function deleteItems(array $keys): bool { return true; } + public function save(CacheItemInterface $item): bool { return true; } + public function saveDeferred(CacheItemInterface $item): bool { return true; } + public function commit(): bool { return true; } + }; + + $resolver = new ActivityContractResolver($pool); + + $premier = $resolver->resolveActivityMethods(MemoContract::class); + $appresLePremier = $pool->getItemCalls; + $second = $resolver->resolveActivityMethods(MemoContract::class); + + self::assertSame($premier, $second); + self::assertSame( + $appresLePremier, + $pool->getItemCalls, + 'une donnée dérivée du code ne change pas dans le processus : le second appel doit être servi de mémoire', + ); + } + + public function testSansPoolLeResultatResteLeMeme(): void + { + $resolver = new ActivityContractResolver(); + + self::assertSame( + ['faire' => 'memo.faire'], + $resolver->resolveActivityMethods(MemoContract::class), + ); + self::assertSame( + ['faire' => 'memo.faire'], + $resolver->resolveActivityMethods(MemoContract::class), + ); + } +} diff --git a/tests/unit/DurableBundle/DependencyInjection/Compiler/DurableMiddlewareReachesTheBusTest.php b/tests/unit/DurableBundle/DependencyInjection/Compiler/DurableMiddlewareReachesTheBusTest.php index 9527e569..53b278f7 100644 --- a/tests/unit/DurableBundle/DependencyInjection/Compiler/DurableMiddlewareReachesTheBusTest.php +++ b/tests/unit/DurableBundle/DependencyInjection/Compiler/DurableMiddlewareReachesTheBusTest.php @@ -74,6 +74,11 @@ private function middlewareOfBusAfterCompilation(array $config, array $existing $container->register('messenger.bus.default')->addTag('messenger.bus'); $container->setParameter('messenger.bus.default.middleware', $existing); + // Et ce que FrameworkExtension pose quand `framework.lock` est configuré. Le backend DBAL + // l'exige : `RequireLockFactoryPass` refuse désormais un conteneur qui monte le verrou de + // reprise sans fabrique, parce qu'une application dans cet état ne démarrerait pas non plus. + $container->register('lock.factory', \stdClass::class); + (new DurableBundle())->build($container); foreach ($container->getCompilerPassConfig()->getBeforeOptimizationPasses() as $pass) { $pass->process($container); diff --git a/tests/unit/DurableBundle/DependencyInjection/DurableDeclaredWiringTest.php b/tests/unit/DurableBundle/DependencyInjection/DurableDeclaredWiringTest.php new file mode 100644 index 00000000..9fb43822 --- /dev/null +++ b/tests/unit/DurableBundle/DependencyInjection/DurableDeclaredWiringTest.php @@ -0,0 +1,117 @@ +load(['activity_contracts' => ['cache' => 'mon.pool.declare.plus.tard']]); + + $argument = $container->getDefinition(ActivityContractResolver::class)->getArgument(0); + + self::assertInstanceOf( + Reference::class, + $argument, + "le pool configuré doit être référencé ; s'il n'existe pas, c'est une erreur de compilation, pas un silence", + ); + self::assertSame('mon.pool.declare.plus.tard', (string) $argument); + } + + public function testSansPoolConfigureLeResolveurNEnRecoitAucun(): void + { + $container = $this->load([]); + + self::assertNull($container->getDefinition(ActivityContractResolver::class)->getArgument(0)); + } + + /** + * Sans `framework.lock`, `lock.factory` n'existe pas et le conteneur échoue — mais sur un + * « service inexistant » qui ne dit pas quoi configurer. Le verrou est obligatoire sur DBAL : + * sans lui, deux workers rejouent le même journal en même temps. + */ + public function testLAbsenceDeLockFactoryDitQuoiConfigurer(): void + { + $container = new ContainerBuilder(); + $container->setDefinition('durable.dbal.single_resume_lock', new Definition(\stdClass::class)) + ->setArguments([new Reference('lock.factory')]); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessageMatches('/framework\.lock/'); + + (new RequireLockFactoryPass())->process($container); + } + + public function testAvecLockFactoryLaPasseLaisseFaire(): void + { + $container = new ContainerBuilder(); + $container->setDefinition('durable.dbal.single_resume_lock', new Definition(\stdClass::class)); + $container->setDefinition('lock.factory', new Definition(\stdClass::class)); + + (new RequireLockFactoryPass())->process($container); + + self::assertTrue($container->hasDefinition('durable.dbal.single_resume_lock')); + } + + public function testSansBackendDbalLaPasseNeDitRien(): void + { + $container = new ContainerBuilder(); + + (new RequireLockFactoryPass())->process($container); + + self::assertFalse($container->hasDefinition('lock.factory')); + } + + /** + * L'extension importe des classes des deux ponts. Un `composer require` du seul bundle donne + * alors un conteneur qui compile et un fatal « class not found » au premier appel. + */ + public function testLesDeuxPontsSontDeclaresEnSuggest(): void + { + $manifest = json_decode( + (string) file_get_contents(__DIR__ . '/../../../../src/DurableBundle/composer.json'), + true, + ); + + self::assertIsArray($manifest); + $suggest = $manifest['suggest'] ?? []; + + foreach (['gplanchat/durable-bridge-temporal', 'gplanchat/durable-bridge-dbal'] as $bridge) { + self::assertArrayHasKey($bridge, $suggest, $bridge . ' est câblé en dur par DurableExtension'); + } + } + + /** + * @param array $config + */ + private function load(array $config): ContainerBuilder + { + $container = new ContainerBuilder(); + (new DurableExtension())->load([$config], $container); + + return $container; + } +}