diff --git a/documentation/audit/00-synthese.md b/documentation/audit/00-synthese.md new file mode 100644 index 00000000..b1cc261c --- /dev/null +++ b/documentation/audit/00-synthese.md @@ -0,0 +1,265 @@ +# Audit Durable — cœur, Symfony, Sylius + +20 grilles de relecture indépendantes, chacune portant sur un axe distinct — un domaine +d'expertise du monde Symfony, Sylius ou API Platform. Chaque constat est ancré sur un +`fichier:ligne` vérifié et, quand la règle vient de l'amont, sur la source amont correspondante. +Les vingt rapports détaillés sont dans ce répertoire, un fichier par axe. + +Périmètre : `src/Durable` (207 f., 16 300 l.), `src/DurableBundle` (23 f., 3 600 l.), +`src/DurablePlugin` (9 f., 550 l.), `src/Bridge/Dbal`, la partie écrite à la main de +`src/Bridge/Temporal` (94 f. sur 774 — les 680 fichiers protobuf générés sont exclus), la +documentation et les deux bancs d'essai. 151 constats bruts, dédupliqués ci-dessous. + +--- + +## Verdict + +Le projet est **solide là où on l'attendrait fragile, et fragile là où on l'attendrait solide.** + +Ce qui tient : la désérialisation du journal (aucun `unserialize`, liste blanche `match` fermée — +un journal falsifié n'instancie rien d'arbitraire), l'immuabilité (69 `readonly class`, 29/29 +événements `final readonly`, zéro setter), la matrice CI (6.4 LTS × 7.4 LTS × plancher et tête de la +ligne 8, en `lowest` et `highest`), le plancher PHP 8.2 réellement tenu, la procédure de migration +Rector, et l'exactitude de la documentation au niveau des exemples (51 FQCN cités, tous résolvent). + +Ce qui ne tient pas se concentre sur **quatre lignes de faille** : + +1. **Le rejeu déterministe a un trou et un coût.** Un `sideEffect()` qui rend `null` est ré-exécuté + à chaque passe — la garantie même que la primitive existe pour offrir. Et toute passe de rejeu + est quadratique. +2. **Le bundle est un prototype de câblage.** 48 services publics sur 60, 819 lignes d'extension + procédurale, le profileur actif en production, des middlewares imposés à tous les bus de + l'application, et deux ponts référencés en dur sans être déclarés nulle part. +3. **Le journal DBAL est invisible de Doctrine.** `doctrine:migrations:diff` génère des + `DROP TABLE` sur les tables du journal. +4. **Le plugin Sylius est un bundle Symfony qui porte le nom « plugin ».** Ni `type: sylius-plugin`, + ni dépendance Sylius déclarée, ni traduction, ni préfixe admin configurable. + +Trois constats sont contredits par les faits et méritent d'être notés comme tels : il n'y a **pas** +de faille de désérialisation, **pas** de collision avec `symfony/workflow`, et la route admin en dur +n'est **pas** un trou d'authentification (`IsGranted` refuse toujours) — c'est un défaut de +portabilité. + +--- + +## Bloquants + +### B1 — `sideEffect()` se rejoue pour de vrai quand la valeur enregistrée est `null` +`src/Durable/ExecutionContext.php:316` · `src/Durable/Port/WorkflowHistorySourceInterface.php:86` +**Deux grilles indépendantes (Fibers/rejeu, contrats) y arrivent par des chemins différents.** + +`findSideEffectForSlot(): mixed` rend `null` aussi bien pour « aucun slot enregistré » que pour +« slot enregistré valant `null` ». Le consommateur teste `null !== $replayResult` : il conclut « non +enregistré », **ré-exécute la closure non déterministe** et appende un `SideEffectRecorded` de plus. +À chaque passe : effet de bord rejoué, valeur potentiellement différente, journal qui croît d'un +événement par replay. Les trois méthodes sœurs du même port (`findActivitySlotResult`, +`findChildWorkflowForSlot`, `findNexusOperationSlotResult`) enveloppent justement leur résultat dans +une forme `array{result: mixed, …}` pour éviter exactement cela — l'incohérence est interne au +contrat. Temporal encode la présence d'un side effect par l'existence du marqueur, jamais par sa +valeur. + +*Correctif* : `findSideEffectForSlot(int $slot): ?array` de forme `array{result: mixed}|null`, ou un +`hasSideEffectForSlot(int): bool` au port. La présence d'un slot ne doit jamais être déduite de sa +valeur. + +### B2 — Le profileur est câblé en production et sa trace n'est jamais réinitialisée en worker +`src/DurableBundle/DependencyInjection/DurableExtension.php:96`, `:744-748` · +`src/DurableBundle/EventListener/ResetDurableProfilerListener.php:24` +**Trois grilles indépendantes (DX, profiler, HTTP).** + +`registerProfiler()` est appelé sans condition, en premier dans `load()`, et pose le tag +`data_collector` quels que soient `kernel.debug` et la présence du WebProfilerBundle. Le même +service `durable.execution_trace` est aliasé sur `WorkflowExecutionObserverInterface` et injecté +dans `ExecutionRuntime` (`:479`), `ExecutionEngine` (`:550`) et `ActivityMessageProcessor` (`:716`) : +il instrumente l'exécution en production. Il n'a **pas** de tag `kernel.reset` et n'est vidé que par +un listener `kernel.request` — dans `messenger:consume`, où il n'y a pas de requête, la trace grossit +sans borne. FrameworkBundle charge ses collecteurs depuis des fichiers séparés +(`cache_debug.php`, `debug_prod.php`) sous condition. + +*Correctif* : isoler la plomberie de profil dans un `config/debug.php` chargé si `%kernel.debug%`, +poser `kernel.reset`, et servir un observateur nul hors debug. + +### B3 — Le DataCollector ne `cloneVar` jamais : un payload non sérialisable casse tout le profil +`src/DurableBundle/DataCollector/DurableDataCollector.php:643`, `:95`, `:842` + +`'payload' => $event->payload()` entre brut dans `$this->data`, et `__serialize()` renvoie le tableau +tel quel. Un objet non sérialisable dans une charge utile de workflow — une `Closure`, une +connexion, un `PDO` — casse la sérialisation du profil **entier**, pas seulement du panneau Durable. +`cloneVar()` existe précisément pour ça. + +### B4 — `doctrine:migrations:diff` génère des `DROP TABLE` sur le journal +`src/Bridge/Dbal/Schema/DurableSchema.php:56`, `:15` · +`src/DurableBundle/DependencyInjection/DurableExtension.php:143` + +Les quatre tables du pont ne sont déclarées à Doctrine par aucun `configureSchema` ni écouteur +`postGenerateSchema` — **alors qu'un docblock du fichier l'affirme**. Elles sont donc inconnues de +`doctrine:schema:update` comme des migrations, qui les voient comme des tables orphelines. Le +`DoctrineTransport` de Messenger, cité en modèle par le code lui-même, résout exactement ce problème. + +### B5 — Le premier workflow du guide ne s'enregistre pas +`documentation/user/getting-started/_index.md:183` · `documentation/user/activities/_index.md:33` + +Le tutoriel pose `#[AsActivity]` sur l'implémentation, alors que `DurableBundle::build()` +n'autoconfigure que `#[AsActivityHandler]`. Le lecteur suit le guide et rien ne se branche. + +### B6 — Le parcours d'arrivée ne mène jamais à un résultat visible +`documentation/user/getting-started/_index.md:243` + +Le guide s'arrête sur un `dispatchNewWorkflowRun()` qui rend `void`, sans jamais indiquer de +consommateur pour le profil in-memory qu'il prescrit. `durable:sample`, qui draine tout seul, n'est +cité nulle part. + +--- + +## Exposer l'état en API (chantier `durable-apiplatform`) + +La question posée était : un `ProviderInterface` / `ProcessorInterface` API Platform peut-il se poser +sur les contrats actuels ? **Non pour les opérations d'item, oui pour les collections.** + +- **Bloquant** — `WorkflowRunCatalogInterface` n'a aucune lecture d'un run isolé : `readHistory()` + prend une `WorkflowRunDescription`, pas un identifiant. `GET /workflow_runs/{id}` est + inimplémentable, et les `@id` de collection pointeraient dans le vide. + (`src/Durable/Port/WorkflowRunCatalogInterface.php:34-57`) +- **Bloquant** — côté écriture, `dispatchNewWorkflowRun()` rend `void` ; l'identifiant est inventé + par l'appelant et devient le **workflowId** sur Temporal (donc un `groupId`) alors qu'il *est* le + `runId` sur DBAL. Une IRI `Location:` résoudrait sur un backend et pas sur l'autre. + (`src/Durable/Port/WorkflowResumeDispatcher.php:25`, `src/Bridge/Temporal/WorkflowClient.php:53-63`) +- **Majeur** — aucun backend ne donne de total : plafond à `PartialPaginatorInterface`, et le curseur + opaque est incompatible avec les liens `page` d'Hydra. +- **Sain, contre l'hypothèse de départ** — la pagination par clé existe et est bien faite + (`LIMIT n+1`), et il n'y a **pas** de couplage au backend : DTO `readonly` et suite de conformité. + Les tableaux bruts ne sont que dans `RunDashboard`, modèle taillé pour Twig. + +--- + +## Majeurs — cœur + +| # | Constat | Ancre | Grilles | +|---|---------|-------|---------| +| M1 | **Le rejeu est quadratique.** `EventStoreHistorySource` ouvre son propre `readStream()` complet dans chacune de ses 15 méthodes, sans mémoïsation ; `activity()` en déclenche trois par activité. Sous DBAL : une requête SQL **et** un mapping par ligne à chaque balayage — des centaines d'allers-retours base par tâche. Les SDK Temporal construisent l'état en une seule passe ordonnée. | `Store/EventStoreHistorySource.php:44` | 2 | +| M2 | **`version()` ignore `$minSupported`.** Retirer une vieille branche fait basculer silencieusement un run en vol sur la neuve. | `ExecutionContext.php:211-217` | 1 | +| M3 | **Un continue-as-new coupe la chaîne** : métadonnées supprimées, identifiant neuf, aucun lien — l'exécution est irrécupérable. | `Handler/ResumeWorkflowHandler.php:89-93` | 1 | +| M4 | **Les `finally` du workflow s'exécutent à chaque passe.** Le fiber abandonné à chaque suspension est détruit, donc PHP déroule sa pile (vérifié sur 8.2.33 contre la RFC Fibers). Un `finally` qui attend prend `FiberError`, levé hors de tout `WorkflowLifecycleInterface`. | `Worker/WorkflowFiberDriver.php:89-91` | 1 | +| M5 | **Une valeur de suspension non reconnue abandonne le run en silence** : `break`, aucun rappel de cycle de vie, exécution « non complétée » que rien ne reprogramme. Le mode de panne le plus coûteux à diagnostiquer du moteur, atteint par un `break`. | `Worker/WorkflowFiberDriver.php:63-65` | 1 | +| M6 | **La raison de l'attente est calculée à chaque suspension puis jetée** dans le chemin Messenger de production ; seul `InMemoryWorkflowRunner` la lit. | `Handler/ResumeWorkflowHandler.php:70-72` | 1 | +| M7 | **Les objets valeur ne franchissent pas les frontières.** `ExecutionId` : 0 type-hint contre 152 `string $executionId`. `WorkflowHistorySourceInterface` rend cinq `array{…}` de forme alors que sa propre docstring et une tâche cochée du change `value-objects-through-ports` annoncent des `Duration`. | `Port/WorkflowHistorySourceInterface.php:10,23,58` | 3 | +| M8 | **Les invariants ne sont pas validés** : un seul `throw` de validation sur ~60 types constructibles. Les objets sont gelés sans jamais avoir été vérifiés. | `Awaitable/QuorumAwaitable.php:37` | 1 | +| M9 | **Aucune interface marqueur d'exception** : impossible d'attraper « une erreur Durable » — 16 classes, hiérarchie plate, contre la convention Symfony. | `Exception/` (16 classes) | 1 | +| M10 | **`WorkflowCommandBufferInterface` : 5 méthodes sur 14** ne sont honorables que par un backend (3 corps vides côté Temporal, 2 qui lèvent côté journal) — et l'interface le documente comme intentionnel. | `Port/WorkflowCommandBufferInterface.php:35` | 1 | +| M11 | **`NullEventStore` n'est pas un objet nul** mais un bouche-trou de signature : `readStream()` rendant `[]` est indistinguable d'une exécution neuve, donc le workflow rejouerait ses activités au lieu d'échouer. | `Store/NullEventStore.php:44` | 1 | + +## Majeurs — bundle Symfony + +| # | Constat | Ancre | Grilles | +|---|---------|-------|---------| +| M12 | **48 `setPublic(true)` sur 60 services** (contre 13 privés), alias de décorateurs internes compris. Chaque service public échappe à l'inlining et à `RemoveUnusedDefinitionsPass`, et devient une promesse de compatibilité implicite. | `DependencyInjection/DurableExtension.php:157` (+47) | 2 | +| M13 | **Les middlewares s'insèrent en tête de _tous_ les bus** de l'application, sans opt-out : le verrou DBAL et le middleware de profil s'appliquent au bus de commandes métier d'un utilisateur qui n'a rien demandé. DoctrineBundle définit son middleware et laisse l'application l'ajouter. | `Compiler/RegisterDurableMiddlewarePass.php:38-57` | 2 | +| M14 | **Deux ponts référencés en dur, déclarés nulle part.** L'extension importe 16 classes du pont Temporal et 7 du pont DBAL ; le `composer.json` du bundle ne les mentionne pas, pas même en `suggest`. Le conteneur compile, puis fatal « class not found » au premier appel. Le paquet cœur, lui, le fait correctement. | `DurableBundle/composer.json:16-29` vs `DurableExtension.php:7-29` | 4 | +| M15 | **Le cache warmer est un no-op silencieux** : il écrit dans un pool d'exécution avec un TTL d'une heure au lieu du répertoire de build, et le pool par défaut est `null`. Aggravé par `hasDefinition()` qui rend `false` sur un **alias** — `Psr\Cache\CacheItemPoolInterface` en est un : l'utilisateur configure un pool, rien ne tourne, rien ne le signale. | `CacheWarmer/ActivityContractCacheWarmer.php:23-29` · `DurableExtension.php:503` | 4 | +| M16 | **Le conteneur instancie tous les gestionnaires d'activité pour en exécuter un seul** (`addMethodCall` + `Reference` au lieu d'un `ServiceLocator`) — vérifié dans le conteneur compilé du banc. | `Compiler/ActivityHandlerPass.php:75` | 1 | +| M17 | **`#[AsWorkflow]` existe mais n'est pas autoconfiguré.** Trois attributs sur quatre le sont ; les workflows se taguent à la main en YAML, par répertoire. L'incohérence est dans le bundle, pas dans l'application. | `DurableBundle.php:24-49` | 1 | +| M18 | **Le choix du backend n'existe pas comme nœud de config** : il est réparti sur `event_store.type`, `temporal.dsn` et `temporal.journal`, et la seule combinaison interdite est gardée par une `\LogicException` nue — donc sans chemin de configuration. Le garde est de plus **asymétrique** : `dbal` + DSN refuse dur, `in_memory` + DSN passe en silence. `mailer` fait la même exclusion en `->validate()->thenInvalid()`. | `DurableExtension.php:137-139`, `:424` | 1 | +| M19 | **`lock.factory` référencé sans vérifier que `framework.lock` est configuré**, et liste de backends fermée sans point d'extension. | `DurableExtension.php:162-166` | 1 | +| M20 | **Extension procédurale de 819 lignes**, sans `Resources/config`, identifiants en FQCN au lieu de `durable.*` — l'ordre d'appel des 18 méthodes de `load()` est devenu la structure du conteneur. | `DurableExtension.php:86-115` | 1 | + +## Majeurs — Messenger et transports + +| # | Constat | Ancre | +|---|---------|-------| +| M21 | **`retry_strategy`, `failure_transport` et `--limit` sont de la configuration morte.** Les trois transports Temporal receive-only exécutent le travail dans `get()` et retournent `[]` : aucune enveloppe n'atteint jamais le `Worker`. | `Bridge/Temporal/Messenger/TemporalJournalTransport.php:29-34` (+2) | +| M22 | **Les erreurs gRPC sortent brutes** de la boucle du `Worker`, hors du contrat `TransportException`. | `TemporalJournalTransport.php:31` | +| M23 | **Le verrou de reprise est pris aussi au dispatch**, faute de garde `ReceivedStamp` : une requête HTTP peut bloquer jusqu'à 300 s. | `Bridge/Dbal/Messenger/SingleResumeLockMiddleware.php:41-42` | +| M24 | **`MessengerActivityTransport` acquitte avant traitement** et retient une enveloppe sans jamais la rendre. | `DurableBundle/Transport/MessengerActivityTransport.php:59-73` | +| M25 | **Le retry des activités est réimplémenté à côté de Messenger**, sans jamais utiliser `UnrecoverableMessageHandlingException`. | `Durable/Worker/ActivityMessageProcessor.php:138-195` | + +## Majeurs — plugin Sylius + +| # | Constat | Ancre | Grilles | +|---|---------|-------|---------| +| M26 | **La route fige `/admin/` au lieu de `%sylius_admin.path_name%`.** Changer `SYLIUS_ADMIN_ROUTING_PATH_NAME` fait basculer la page sous le pare-feu boutique. Ce n'est **pas** un trou d'authentification — `IsGranted` refuse toujours — c'est un défaut de portabilité. Tous les plugins amont (Refund, Adyen) importent leurs routes admin avec ce préfixe. | `DurablePlugin/Resources/config/routes.yaml:2` | **4** | +| M27 | **C'est un bundle Symfony qui porte le nom « plugin »** : `type: symfony-bundle`, ni `SyliusPluginTrait`, ni `getPath()`, arborescence `Resources/` de Sylius 1 là où le squelette 2.0 pose `config/`, `templates/`, `translations/` à la racine. Et le `composer.json` **ne déclare aucune dépendance Sylius**, alors que son unique gabarit étend `@SyliusAdmin/shared/layout/base.html.twig`. | `DurablePlugin/composer.json:15`, `:33-35` | 2 | +| M28 | **Rien n'est traduisible** : zéro `|trans` dans tout le paquet, libellé de menu et 270 lignes de gabarit en anglais littéral, aucun `translations/`. | `EventListener/AdminMenuListener.php:37` | 2 | +| M29 | **Le châssis d'admin est reconstruit à la main** au lieu d'être composé par le hook `sylius_admin.common.index`, et **la liste est rendue à la main** sans lien « page précédente » alors que `sylius/grid-bundle` documente un `DataProviderInterface` non-Doctrine. La friction curseur/`Pagerfanta` est réelle mais n'est argumentée ni dans DUR049 ni dans la proposition. | `Resources/views/admin/dashboard/index.html.twig:61-66`, `:136-168` | 1 | +| M30 | **L'entrée de menu contourne le contrat Sylius** par `object` + `method_exists`, et est déclarée par `uri` — elle n'est donc jamais marquée active. | `EventListener/AdminMenuListener.php:15-41` | 2 | + +## Majeurs — outillage, paquets, tests, documentation + +| # | Constat | Ancre | +|---|---------|-------| +| M31 | **Le pont Temporal revendique le PSR-4 `Temporal\Api\`** et écrase silencieusement celui que `temporal/sdk` tire via `roadrunner-php/roadrunner-api-dto`, sans `conflict` pour l'interdire. | `Bridge/Temporal/composer.json:31` | +| M32 | **`self.version` sur les onze liens inter-paquets rend la ligne alpha publiée non installable** (`minimum-stability` est root-only). Recoupe ce qui avait déjà été constaté à l'installation. | `DurableBundle/composer.json:18` (+10) | +| M33 | **`gplanchat/durable-magento` est publié par splitsh mais absent du graphe racine**, et aucun `composer validate` en CI : les manifestes des paquets publiés sont structurellement invérifiables — trois sont effectivement faux. | `bin/splitsh-publish.sh:39` vs `composer.json:11-48` | +| M34 | **La baseline Psalm est morte à 87 %**, et `findUnusedBaselineEntry` est explicitement éteint. PHPStan est au niveau 5 sur 10 : les quinze formes de tableaux déclarées dans les ports ne sont **pas** vérifiées (niveau 6 minimum), et les casts `(string) $payload[…]` sur du `mixed` à la frontière de désérialisation passent sans signalement. Nuance mesurée : la baseline **ne masque aucun vrai bug** (25 entrées réelles sur 196, toutes des invariants), et l'écart niveau 5 → 8 vaut 67 diagnostics, dont 13 au seul niveau 6. | `psalm.xml:8`, `:42` · `phpstan.neon:5` | +| M35 | **Aucun adaptateur Temporal ne joue de suite de conformité DUR041.** Trois sont concernés — les deux stores d'événements et le catalogue de runs —, pour deux ports ; les trois autres backends jouent les quatre suites. L'ADR l'annonçait pourtant au présent (« they run this tier there », « Temporal's read-through store gets checked for the first time »), et le docbloc du cœur reprenait le même énoncé. *Repris depuis : DUR041 et le docbloc portent l'état réel.* | `Testing/EventStoreReplayConformanceTestCase.php:24` vs `adr/DUR041…md:5`, `:115` | +| M36 | **La conformité SQL n'est prouvée que contre SQLite en mémoire**, un moteur dont le dépôt a déjà écrit qu'il avalait deux fautes DBAL. | `tests/unit/Bridge/Dbal/DbalEventStoreConformanceTest.php:21` | +| M37 | **Le milieu de la pyramide est quasi vide** : 134 classes unitaires, 6 d'intégration sans infrastructure, 19 gated sur un serveur Temporal — l'étage déclaré par DUR010 manque. Behat : douze dépendances et 100 lignes de config pour **zéro scénario projet et zéro job CI**. | `phpunit.xml:35` · `sylius/features/` | +| M38 | **La référence de configuration se dit exhaustive et ignore le nœud `dbal` en entier** (quatre `table_name`, la valeur `dbal` de trois énumérations), et se trompe sur le défaut d'`activity_transport.type`. | `documentation/user/configuration/_index.md:8` | +| M39 | **Le README du pont Temporal documente 2 des 4 `purpose`** que la fabrique accepte — les deux manquants étant ceux que le guide utilisateur fait écrire. | `Bridge/Temporal/README.md:17` vs `TemporalTransportFactory.php:70,80,90` | +| M40 | **Navigation documentaire plate** : 17 sections sœurs, la référence avant le tutoriel (`packages` weight 5 vs `getting-started` 10), l'explication en huitième position, **aucun glossaire** — « curseur » est dans l'argumentaire du README et défini nulle part, « Nexus » apparaît une fois sans définition. | `documentation/user/packages/_index.md:3` | +| M41 | **Le banc Symfony : un GET démarre un workflow et l'exécute intégralement dans la requête**, avec un `catch \Throwable` qui court-circuite `kernel.exception`. | `symfony/src/Controller/SamplesWorkflowController.php:33`, `:49-72` | + +--- + +## Ce qui est sain — et vérifié comme tel + +- **Désérialisation du journal** : zéro `unserialize`, zéro `eval`. `EventDataMapper` est une liste + blanche `match` fermée, `WorkflowRegistry` n'appelle qu'un type préenregistré. Un journal falsifié + n'instancie rien d'arbitraire. +- **Immuabilité** : 69 `readonly class`, 222 propriétés `readonly`, 29/29 événements + `final readonly`, zéro setter, zéro classe non-`final`. +- **PHP 8.2+** : plancher réellement tenu, aucune syntaxe 8.3/8.4 hors code généré. 10 enums, + `match(true)`, types de propriété partout. +- **Matrice CI** : 6.4 LTS × 7.4 LTS × plancher et tête de la ligne 8, en `lowest` et `highest`, + PHPStan à chaque entrée, chaque ligne portant la raison d'être du bord qu'elle mesure. +- **Migrations BC** : le jeu Rector `durable-upgrade` couvre les treize renommages d'alpha8 et + `UPGRADE.md` documente la seule suppression que Rector ne peut pas exprimer. +- **Aucune collision avec `symfony/workflow`** : racine `durable` vs `framework.workflows`, tag + `durable.workflow` vs `workflow`, zéro collision de nom court. La collision est purement lexicale. +- **Isolation du backend annoncée par DUR037** : elle tient — zéro mention de `Bridge\`, `Temporal`, + `Dbal` ou `grpc` dans le plugin. +- **Documentation** : les 51 FQCN cités résolvent, aucun exemple PHP ne casse, DUR006/030/037/038/041 + existent et disent bien ce qu'on leur fait dire. +- **Sécurité du dashboard** : doublement protégé, `#[IsGranted]` **et** `access_control`. +- **Ordre des passes de compilation** : correct et vérifié contre `PassConfig`, avec justification + écrite de chaque priorité. Position de la pile de middlewares correcte (au-dessus de + `doctrine_transaction` et `handle_message`). +- **Twig du collector** : pas de `|raw`, `json_encode` ré-échappé par l'autoescape, états vides + traités. +- **`Attribute/`, `Query/`, `EventDataMapper`** : corrects, cibles justes, `class-string` là où il + faut, formes de retour exactes. +- **Métadonnées Composer** : `license`/`description`/`authors`/`keywords`/`type` partout, aucun + `@dev` qui fuit hors racine, contraintes tierces en `^`, `branch-alias` cohérent avec les tags. +- **Modèle de suspension** : un seul `Fiber::suspend()` dans tout le cœur, un pilote unique, et la + livraison d'annulation par `Fiber::throw()` au point de suspension est conforme à la RFC. +- **Conventions de test** : la règle « jamais de `run()` ni `fail()` comme méthode d'aide » est + tenue ; l'ordre de tableau est traité explicitement. + +--- + +## Les vingt axes + +| Axe | Ce qu'il couvre | Constats | +|---|---|---| +| [Cohérence Symfony](01-coherence-symfony.md) | minimalisme de la surface publique, DX d'installation | 8 | +| [PHP moderne, Fibers](02-php-moderne-fibers.md) | rejeu et coût du chemin chaud | 8 | +| [Contrats et typage](03-contrats-typage.md) | `Port/`, `Store/`, `Query/`, `Mapping/`, `Attribute/` | 8 | +| [État d'API](04-etat-api.md) | pagination, modèle de lecture HTTP, exposer les runs en API | 8 | +| [Conteneur et performance](05-conteneur-performance.md) | paresse des services, coût au boot et au rejeu | 8 | +| [Configuration](06-configuration.md) | validation, messages d'erreur, normalisation | 8 | +| [Compatibilité, analyse statique](07-compatibilite-statique.md) | dépréciations, matrice CI, analyse statique | 6 | +| [Console et sécurité](08-console-securite.md) | signature, sortie, codes de retour | 8 | +| [Routing et HTTP](09-routing-http.md) | routes, listeners kernel, sémantique des réponses | 5 | +| [API publique](10-api-publique.md) | immutabilité, nommage, objets valeur, invariants | 8 | +| [Vocabulaire et exploitation](11-vocabulaire-exploitation.md) | cohabitation Symfony, versionnage, ergonomie d'exploitation | 6 | +| [Interop et Doctrine](12-interop-doctrine.md) | points d'extension, Doctrine, ordre de chargement | 8 | +| [Documentation](13-documentation.md) | conventions des composants récents, doc contre code | 8 | +| [Passes et profiler](14-passes-profiler.md) | collecte, sérialisation des données collectées | 8 | +| [Messenger](15-messenger.md) | transports, middleware, stamps, retry | 8 | +| [Prise en main](16-prise-en-main.md) | structure documentaire, chemin du premier succès | 8 | +| [Composer et publication](17-composer-publication.md) | métadonnées, contraintes, publication depuis un monorepo | 8 | +| [Plugin Sylius](18-plugin-sylius.md) | conformité au squelette officiel, compatibilité Sylius 2 | 8 | +| [Tests](19-tests.md) | Behat, pyramide, isolation et déterminisme | 6 | +| [Back-office Sylius](20-admin-sylius.md) | layout, grilles de ressources, menu, Twig, accessibilité | 8 | + +Chaque rapport détaille ses constats avec, pour chacun, la référence amont qui fonde la règle et +le correctif proposé. diff --git a/documentation/audit/01-coherence-symfony.md b/documentation/audit/01-coherence-symfony.md new file mode 100644 index 00000000..0b0909dd --- /dev/null +++ b/documentation/audit/01-coherence-symfony.md @@ -0,0 +1,67 @@ +# Cohérence Symfony — minimalisme de la surface publique, DX d'installation + +## Synthèse + +Le bundle boote sans configuration — chaque nœud de `Configuration.php` porte `addDefaultsIfNotSet()` ou une valeur par défaut — mais le guide de démarrage et l'app d'exemple font écrire **environ 33 lignes de YAML réparties sur trois fichiers** (`durable.yaml` 18 l., `messenger.yaml` 12 l., `services.yaml` 3 l.) pour un premier workflow, dont 10 lignes qui ne font que répéter les défauts du bundle. La surface publique est le point le plus éloigné des conventions : 48 des 60 services enregistrés par l'extension sont `setPublic(true)`, là où le Bundle Best Practices demande le privé par défaut avec alias depuis l'interface. La structure de l'extension s'écarte aussi de l'amont : 819 lignes de PHP procédural, aucun fichier `Resources/config/*.php`, des identifiants de service en FQCN au lieu du préfixe d'alias. Deux mécanismes redoublent ce que Symfony sait déjà faire : la liste `activity_contracts.contracts` réénumère à la main ce que les balises `durable.activity_handler` portent déjà, et le collecteur de profil est câblé inconditionnellement, son observateur passant sur le chemin chaud de production. Le point sain : l'autoconfiguration par attribut de `AsActivityHandler` / `AsNexusServiceHandler` est idiomatique. + +## Constats + +### C1 — Presque tout le conteneur du bundle est public +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:157` (premier de 48 ; ex. aussi `:481`, `:553`, `:769`) +- **Gravité** : majeur +- **Constat** : `grep -c 'setPublic(true)'` sur l'extension donne 48, contre 13 `setPublic(false)`, pour 60 `->register(` — soit ~80 % de la plomberie interne exposée à `$container->get()`. Les alias eux-mêmes sont rendus publics (`:159`, `:212`, `:235`, `:748`), y compris pour des décorateurs internes (`durable.event_store.dbal.projecting`, `durable.workflow_metadata_store.in_memory.projecting`) que rien dans l'application n'a de raison de tirer du conteneur. Chaque service public est un point d'entrée que le compilateur ne peut plus retirer ni inliner, et une promesse de compatibilité implicite. +- **Amont** : https://symfony.com/doc/current/bundles/best_practices.html — « services not meant to be used by the application directly, should be defined as private. For public services, aliases should be created from the interface/class to the service id. » +- **Correctif** : passer tout en privé par défaut, ne garder public que les quelques services réellement tirés du conteneur (workers Messenger, catalogue lu par le tableau de bord) et exposer le reste par alias d'interface autowirable ; les services de debug se préfixent d'un point (`.durable.…`). + +### C2 — Le profil est câblé en production, et son observateur est sur le chemin chaud +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:96` (appel), `:763-774` (tag `data_collector`) +- **Gravité** : majeur +- **Constat** : `registerProfiler()` est appelé sans condition, en premier dans `load()`, et pose le tag `data_collector` quel que soit `kernel.debug` ou la présence du WebProfilerBundle. Le même service, `durable.execution_trace`, est aliasé sur `WorkflowExecutionObserverInterface` et injecté dans `ExecutionRuntime` (`:479`), `ExecutionEngine` (`:550`) et `ActivityMessageProcessor` (`:716`) — il instrumente donc l'exécution en prod, pas seulement en debug. Le profileur pèse par ailleurs ~1 350 des 3 643 lignes du bundle (`DataCollector/DurableDataCollector.php` 856 l. + `Profiler/` 499 l.), soit plus que l'extension elle-même. `ResetDurableProfilerListener` borne la croissance ; le reproche porte sur le câblage inconditionnel, pas sur une fuite. +- **Amont** : `symfony/vendor/symfony/framework-bundle/DependencyInjection/FrameworkExtension.php` charge `collectors.php` et `*_debug.php` sous condition (`Resources/config/cache_debug.php`, `debug_prod.php`, `http_client_debug.php`, `form_debug.php` sont des fichiers séparés du chemin nominal). +- **Correctif** : isoler la plomberie de profil dans un `Resources/config/debug.php` chargé seulement si `%kernel.debug%`, et faire de l'observateur un `NullWorkflowExecutionObserver` hors debug. + +### C3 — `#[AsWorkflow]` existe mais n'est pas autoconfiguré : les workflows se déclarent en YAML +- **Fichier** : `src/DurableBundle/DurableBundle.php:24-49` (trois attributs autoconfigurés), `src/Durable/Attribute/AsWorkflow.php:8` +- **Gravité** : majeur +- **Constat** : `build()` enregistre `registerAttributeForAutoconfiguration()` pour `AsActivityHandler`, `AsNexusServiceHandler` et `FulfilsNexusOperation`, mais pas pour `AsWorkflow`, qui existe pourtant dans le cœur et que `WorkflowDefinitionLoader` lit déjà (`src/Durable/Workflow/WorkflowDefinitionLoader.php:94`). Conséquence : l'utilisateur doit poser la balise à la main par répertoire (`symfony/config/services.yaml:23-29`, deux blocs `resource:` + `tags: [durable.workflow]`), ce que le guide documente comme l'étape normale. Trois briques sur quatre se déclarent par attribut, la quatrième par convention de dossier — l'incohérence est dans le bundle, pas dans l'application. +- **Amont** : https://symfony.com/doc/current/bundles/best_practices.html (registre des services par balise) et le mécanisme `registerAttributeForAutoconfiguration` déjà employé aux lignes 24-49 du même fichier. +- **Correctif** : ajouter un quatrième `registerAttributeForAutoconfiguration(AsWorkflow::class, …)` posant `durable.workflow` ; `WorkflowPass` n'a rien à changer, et les blocs `resource:` du `services.yaml` de l'application disparaissent. + +### C4 — Extension procédurale de 819 lignes, sans `Resources/config`, avec des identifiants en FQCN +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:86-115` (l'aiguillage), `src/DurableBundle/Resources/` (ne contient que `views/`) +- **Gravité** : majeur +- **Constat** : toute la définition des services est écrite en PHP impératif dans l'extension (60 `->register(`, 18 `setAlias`), sans aucun fichier de configuration de services ; les identifiants sont les FQCN du cœur (`Gplanchat\Durable\ExecutionEngine`, `:542`) plutôt que des identifiants préfixés `durable.*`. Le prix se voit à `registerDbalRunCatalog()` (`:217-223`) et `registerInMemoryRunCatalog()` (`:276-280`), qui doivent déplacer une définition déjà posée et en supprimer une autre parce que l'ordre d'appel des 18 méthodes de `load()` est devenu la structure du conteneur. +- **Amont** : https://symfony.com/doc/current/bundles/best_practices.html — « If the bundle defines services, they must be prefixed with the bundle alias instead of using fully qualified class names » ; « all services should be defined explicitly » dans `config/`. Cf. `symfony/vendor/symfony/framework-bundle/Resources/config/*.php` (≈40 fichiers) et `symfony/vendor/doctrine/doctrine-bundle/config/messenger.php`. https://symfony.com/doc/current/bundles/extension.html recommande `AbstractBundle` + `loadExtension()` + `$container->import()` pour tout nouveau bundle. +- **Correctif** : sortir le squelette invariant dans `config/services.php` (`ContainerConfigurator`, ids `durable.*`, `alias()` d'interfaces), ne garder dans le code que les branches réellement conditionnelles (dbal / temporal / messenger), et basculer sur `AbstractBundle`, ce qui fusionne `DurableBundle` + `DurableExtension` + `Configuration`. + +### C5 — Le bundle s'insère de force en tête de **tous** les bus Messenger de l'application +- **Fichier** : `src/DurableBundle/DependencyInjection/Compiler/RegisterDurableMiddlewarePass.php:38-57` +- **Gravité** : majeur +- **Constat** : la passe itère `findTaggedServiceIds('messenger.bus')` et réécrit le paramètre `.middleware` de chaque bus, y compris ceux que l'application a définis pour son propre compte, en insérant les middlewares Durable en position 0 (ou 1 derrière `traceable`). Il n'y a ni option de configuration ni liste de bus : le verrou DBAL et le middleware de profil s'appliquent au bus de commandes métier d'un utilisateur qui n'a jamais rien demandé. Le raisonnement du docblock (« il n'existe pas de balise `messenger.middleware` ») est exact, mais la conclusion — s'installer partout — n'est pas celle de l'amont. +- **Amont** : `symfony/vendor/doctrine/doctrine-bundle/config/messenger.php:18` définit `messenger.middleware.doctrine_transaction` et laisse l'application l'ajouter dans `framework.messenger.buses..middleware`. Cf. https://symfony.com/doc/current/messenger.html#middleware. +- **Correctif** : publier les middlewares sous des identifiants stables (`durable.messenger.middleware.single_resume_lock`, `…workflow_run_dispatch_profiler`) et documenter leur ajout ; à défaut, ajouter un nœud `durable.messenger.buses: []` limitant la passe aux bus nommés. + +### C6 — `activity_contracts.contracts` réénumère à la main ce que le conteneur sait déjà +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:59-63`, consommé en `DurableExtension.php:516-531` +- **Gravité** : mineur +- **Constat** : le préchauffage du cache exige la liste des interfaces de contrat dans le YAML applicatif (`symfony/config/packages/durable.yaml:19-23`, quatre FQCN écrits à la main). Ces mêmes FQCN sont déjà connus du conteneur : `ActivityHandlerPass.php:44` lit `$tag['contract']` sur chaque service balisé `durable.activity_handler`, balise posée automatiquement par l'attribut. Une interface ajoutée sans être reportée dans le YAML n'est simplement pas préchauffée, sans avertissement. +- **Amont** : https://symfony.com/doc/current/bundles/best_practices.html — la configuration sert « what changes per environment », pas à redire un inventaire dérivable ; c'est le rôle d'une passe de compilation (cf. `MessengerPass` de FrameworkBundle qui découvre les handlers par balise). +- **Correctif** : faire produire la liste par `ActivityHandlerPass` (ou une passe dédiée) et injecter le résultat dans le cache warmer ; garder le nœud `contracts` uniquement comme complément pour les contrats sans gestionnaire enregistré. + +### C7 — Le pool de cache configuré est abandonné en silence s'il n'est pas une `Definition` +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:503-505` +- **Gravité** : mineur +- **Constat** : `$container->hasDefinition($cacheId) ? new Reference($cacheId) : null` retourne `false` pour un **alias** (`Psr\Cache\CacheItemPoolInterface` est un alias vers `cache.app` dans `Resources/config/cache.php:268`) et pour tout service défini après le chargement des extensions, par exemple dans le `services.yaml` de l'application. L'utilisateur écrit `activity_contracts.cache: mon.pool`, le résolveur tourne sans cache, et rien ne le signale. +- **Amont** : `symfony/vendor/symfony/framework-bundle/Resources/config/cache.php:268` (`->alias(CacheItemPoolInterface::class, 'cache.app')`) ; la convention Symfony est de passer une `Reference` et de laisser `CheckExceptionOnInvalidReferenceBehaviorPass` valider à la compilation. +- **Correctif** : passer inconditionnellement `new Reference($cacheId)` quand la valeur est non nulle — l'erreur devient une erreur de compilation explicite au lieu d'une dégradation muette. + +### C8 — Aucune recette Flex : le « hello workflow » se paie en ~35 lignes de YAML écrites à la main +- **Fichier** : `documentation/user/getting-started/_index.md:52-69` (durable.yaml), `:86-97` (messenger.yaml), `:141-143` (services.yaml) ; défauts dans `src/DurableBundle/DependencyInjection/Configuration.php:16-87` +- **Gravité** : mineur +- **Constat** : Flex enregistre bien le bundle sans recette (auto-génération depuis le namespace PSR-4, `symfony/vendor/symfony/flex/src/Flex.php:706` + `SymfonyBundle.php:64-92`), mais s'arrête là. Or les nœuds `event_store`, `temporal`, `workflow_metadata` et `child_workflow.parent_link_store` du bloc « Minimal Symfony configuration » ne font que réécrire les valeurs déjà posées par `addDefaultsIfNotSet()` — 10 des 18 lignes de `durable.yaml` sont redondantes. Reste le vrai coût : les 12 lignes de transports et de routage Messenger, qu'aucune recette ne pose et qu'aucun `prepend()` ne fournit (`DurableExtension` n'implémente pas `PrependExtensionInterface`). +- **Amont** : https://symfony.com/doc/current/setup/flex.html (recettes et `config/packages/*.yaml` livrés) ; https://symfony.com/doc/current/bundles/prepend_extension.html pour la configuration d'une extension tierce depuis un bundle. +- **Correctif** : soumettre une recette à `symfony/recipes-contrib` livrant un `config/packages/durable.yaml` minimal, et implémenter `prepend()` pour poser le routage `framework.messenger.routing` des cinq messages internes — l'application ne garderait que les DSN de transports. + +## Point sain + +L'autoconfiguration par attribut (`DurableBundle.php:24-49`) et le pilotage des priorités de passes de compilation (`:52-60`, avec justification écrite de chaque priorité) suivent l'usage idiomatique de `registerAttributeForAutoconfiguration` et de `PassConfig`. diff --git a/documentation/audit/02-php-moderne-fibers.md b/documentation/audit/02-php-moderne-fibers.md new file mode 100644 index 00000000..3b3abb9f --- /dev/null +++ b/documentation/audit/02-php-moderne-fibers.md @@ -0,0 +1,62 @@ +# PHP moderne et Fibers — rejeu et coût du chemin chaud + +## Synthèse +Le modèle de suspension est globalement juste : un seul `Fiber::suspend()` dans tout le cœur (`ExecutionRuntime:68`), un pilote unique (`WorkflowFiberDriver`), et la livraison d'annulation par `Fiber::throw()` au point de suspension est conforme à la sémantique de la RFC. Deux angles morts subsistent côté Fibers : le fiber abandonné à chaque suspension est *détruit*, ce qui exécute les blocs `finally` du workflow à chaque passe, et une valeur de suspension inattendue fait sortir le pilote sans aucun rappel de cycle de vie. Le rejeu déterministe est bien gardé (slots positionnels, garde de divergence, versioning déduit), sauf sur `sideEffect()` dont le sentinelle de « non enregistré » est `null` — donc un effet de bord qui rend `null` se rejoue pour de vrai à chaque replay. Le coût du chemin chaud est le point le plus lourd : `EventStoreHistorySource` relit tout le flux à chaque interrogation, sans aucune mémoïsation, ce qui rend une passe de rejeu quadratique et, sous DBAL, quadratique en requêtes SQL. Sur l'axe PHP 8.2+ le périmètre est sain : 69 `readonly class`, 222 propriétés `readonly`, 10 `enum`, `match(true)`, types de propriété partout — rien à signaler. + +## Constats + +### C1 — `sideEffect()` se rejoue pour de vrai quand la valeur enregistrée est `null` +- **Fichier** : `src/Durable/ExecutionContext.php:316` (avec `src/Durable/Store/EventStoreHistorySource.php:223-236`) +- **Gravité** : bloquant +- **Constat** : `findSideEffectForSlot()` rend `null` aussi bien pour « pas de slot enregistré » que pour « slot enregistré valant `null` ». Le test `if (null !== $replayResult)` ligne 316 conclut donc « non enregistré », **ré-exécute la closure non déterministe** et appende un `SideEffectRecorded` de plus (`ExecutionContext:323`). À chaque passe de rejeu : effet de bord rejoué, valeur potentiellement différente, et journal qui croît d'un événement par passe. C'est exactement la propriété que `sideEffect()` existe pour garantir. Le trou est déjà écrit dans le docblock de `hasRecordedWorkAhead()` (`ExecutionContext:243-246`) pour le versioning, mais pas corrigé à sa source. +- **Amont** : Temporal encode la présence d'un side effect par l'existence du marqueur, jamais par sa valeur — `MarkerRecorded` / `SideEffectMarkerName` (https://github.com/temporalio/sdk-go/blob/master/internal/internal_event_handlers.go, `SideEffectMarkerName`) ; la doc citée par `SideEffectRecorded` elle-même (https://docs.temporal.io/develop/php/side-effects) pose que la closure n'est pas rejouée. +- **Correctif** : ajouter `hasSideEffectForSlot(int): bool` au port, ou faire rendre à `findSideEffectForSlot()` un `?array{value: mixed}` — la présence du slot ne doit jamais être déduite de la valeur. + +### C2 — Le rejeu est quadratique : `EventStoreHistorySource` relit tout le flux à chaque interrogation +- **Fichier** : `src/Durable/Store/EventStoreHistorySource.php:44` (15 appels à `readStream()` dans le fichier, aucun champ de cache) +- **Gravité** : majeur +- **Constat** : chaque méthode du port ouvre son propre `readStream()` sur la totalité du flux. `ExecutionContext::activity()` en déclenche trois par activité (`activityNameForSlot`, `findActivitySlotResult`, `findScheduledActivityId`) et `findActivitySlotResult()` alloue cinq tableaux à chaque appel pour n'en lire qu'un index. `ExecutionContext::countRecordedMessages()` (`:460`) boucle sur `messageAt($n)`, chacun étant lui-même un balayage complet. Un rejeu de N opérations coûte donc O(N²) événements désérialisés. Sous `DbalEventStore::readStreamWithRecordedAt()` (`src/Bridge/Dbal/Store/DbalEventStore.php:58`) chaque balayage est **une requête SQL + un `EventDataMapper::toDomainEvent()` par ligne** : des centaines d'allers-retours base par tâche de workflow. +- **Amont** : les SDK Temporal construisent l'état de rejeu en **une seule passe ordonnée** sur l'historique (state machine alimentée par `HandleHistoryEvent`, https://github.com/temporalio/sdk-go/blob/master/internal/internal_task_handlers.go) ; la source d'historique n'y est jamais ré-interrogée par slot. +- **Correctif** : matérialiser le flux une fois par instance de `EventStoreHistorySource` (elle est déjà liée à une exécution et à une passe) dans des index par slot construits en une passe, et servir les 15 accesseurs depuis ces index. + +### C3 — Le fiber abandonné à chaque suspension exécute les `finally` du workflow +- **Fichier** : `src/Durable/Worker/WorkflowFiberDriver.php:89-91` +- **Gravité** : majeur +- **Constat** : sur commande nouvelle, le pilote appelle `onSuspended()` puis `return null` ; la variable locale `$fiber` meurt et PHP détruit un fiber suspendu en **déroulant sa pile**. Vérifié sur PHP 8.2.33 : un `try { await(...) } finally { … }` dans le code workflow voit son `finally` s'exécuter à chaque passe de suspension, et un `finally` qui attend quoi que ce soit prend `FiberError: Cannot suspend in a force-closed fiber` — levé depuis la destruction, donc hors de `dispatchThrowable()` (`:114`) et hors de tout `WorkflowLifecycleInterface`. Aucun ADR ni test ne couvre le cas (`grep finally` vide dans DUR003/DUR027) ; `documentation/user/comparison/_index.md:392` ne mentionne que le cas destructeur. +- **Amont** : RFC Fibers, section Fiber lifecycle — « Fibers that are not finished (do not complete execution) are destroyed similarly to unfinished generators, executing any pending `finally` blocks » et « `Fiber::suspend()` may not be invoked in a force-closed fiber » (https://wiki.php.net/rfc/fibers). +- **Correctif** : écrire l'invariant dans DUR003 (« un `finally` autour d'un `await()` s'exécute une fois par passe, jamais une fois par exécution ») et l'outiller — `src/DurablePhpstan/` a déjà la place pour une règle refusant `try/finally` autour d'un `await()` dans une méthode `#[WorkflowMethod]`. + +### C4 — Une valeur de suspension non reconnue abandonne le run en silence +- **Fichier** : `src/Durable/Worker/WorkflowFiberDriver.php:63-65` +- **Gravité** : majeur +- **Constat** : si le fiber suspend avec autre chose qu'un `Awaitable` — code tiers, bibliothèque à fibers appelée depuis une activité en ligne, `Fiber::suspend()` direct que DUR022 interdit sans le vérifier — le `break` sort de la boucle, `isTerminated()` est faux ligne 104, et `run()` rend `null` après n'avoir appelé que `onBeforeRun()`. Ni `onSuspended`, ni `onFailed`, ni `onCompleted` : l'exécution reste « non complétée » dans le `WorkflowMetadataStore` et rien ne la reprogramme. C'est le mode de panne le plus coûteux à diagnostiquer de ce moteur, et il est atteint par un `break`. +- **Amont** : non sourcé (règle interne : `documentation/adr/DUR022-workflow-class-interface-and-workflow-environment.md:21` interdit `Fiber::suspend()` en code workflow, sans garde à l'exécution). +- **Correctif** : remplacer le `break` par `$this->dispatchThrowable($executionId, new WorkflowTaskFailure(...))` nommant la valeur reçue — un run doit toujours sortir du pilote par un rappel de cycle de vie. + +### C5 — `ExecutionRuntime` attend par défaut en tournant à vide, sans budget +- **Fichier** : `src/Durable/ExecutionRuntime.php:51` et `:79-82` +- **Gravité** : mineur +- **Constat** : `$distributed` vaut `false` par défaut, donc `new ExecutionRuntime($store, $transport, $executor)` (cas de `tests/integration/Durable/Messenger/MessengerActivityTransportTest.php:41`) prend le chemin `while (!$awaitable->isSettled()) { drain(); checkTimers(); }` : aucune pause, aucun budget, et `checkTimers()` relit tout le flux à chaque tour. Une attente de 30 s brûle un cœur pendant 30 s ; une condition qui attend un signal ne se règle jamais et la boucle ne se termine pas. `runUntilIdle()` (`:196`), lui, a bien un budget — l'asymétrie est fortuite. +- **Amont** : `Symfony\Component\Messenger\Worker` dort (option `sleep`, 1 000 000 µs par défaut) dès qu'aucun message n'est traité, précisément pour ne pas tourner à vide (https://github.com/symfony/messenger/blob/7.2/Worker.php). +- **Correctif** : faire de `distributed = true` le défaut, et border le drain synchrone du même budget que `runUntilIdle()` avec un `usleep()` quand ni activité ni minuteur n'a progressé. + +### C6 — Quatre balayages complets du flux par tic de minuterie, et le tic se réarme +- **Fichier** : `src/Durable/Handler/FireWorkflowTimersHandler.php:44-46` et `:57` +- **Gravité** : mineur +- **Constat** : une passe fait `countTimerCompleted()` (balayage), `checkTimers()` (balayage, `ExecutionRuntime:93`), `countTimerCompleted()` (balayage) puis `TimerWakeDelayCalculator` (balayage) — quatre lectures intégrales de l'historique. Quand aucun minuteur n'a tiré, le handler se redispatche (`:64`), donc le coût se répète à chaque réveil, sur un historique qui ne fait que grandir. +- **Amont** : non sourcé (constat de coût direct sur `DbalEventStore::readStream`, une requête SQL par balayage). +- **Correctif** : lire le flux une fois dans `__invoke()` et passer la liste d'événements aux trois calculs, qui n'ont besoin que des `Timer*` — ou faire rendre à `checkTimers()` le nombre de minuteurs réglés, ce qui supprime les deux `countTimerCompleted()`. + +### C7 — Réflexion allouée sur le chemin chaud de chaque `await()` +- **Fichier** : `src/Durable/WorkflowEnvironment.php:174` et `:193` (avec `src/Durable/Awaitable/ConditionAwaitable.php:49`) +- **Gravité** : mineur +- **Constat** : `applyMessagesUntil()` ligne 193 appelle `describeCondition()` seulement pour tester la présence d'une condition, mais `ConditionAwaitable::describe()` construit une `ReflectionFunction` et formate une chaîne `sprintf` à chaque fois, résultat jeté. Ligne 174, `self::describe($awaitable)` est évalué en argument à chaque `await()` borné — donc une `ReflectionClass` dans la branche `default` — alors que la chaîne ne sert qu'à composer le message de `DeadlineExceededException`, sur le seul chemin d'échéance dépassée. +- **Amont** : non sourcé (règle générale : la réflexion ne sert pas à un prédicat qui a une réponse structurelle). +- **Correctif** : ajouter `AwaitableInspector::hasCondition(): bool` (même traversée, pas de réflexion) pour la ligne 193, et passer la description en `\Closure` paresseuse — ou l'awaitable lui-même — à `awaitUnderDeadline()`. + +### C8 — Le test de contexte fiber n'est pas une identité +- **Fichier** : `src/Durable/ExecutionRuntime.php:67` +- **Gravité** : remarque +- **Constat** : `if (null !== \Fiber::getCurrent())` répond « oui » depuis n'importe quel fiber, pas seulement celui que `WorkflowFiberDriver` pilote. Si du code s'exécute dans un fiber imbriqué — un client HTTP à fibers appelé depuis une activité drainée en ligne, `revolt/event-loop` chargé par une dépendance — le `Fiber::suspend()` rend la main au pilote du fiber *intérieur*, et la ligne 71 lit `getResult()` sur un awaitable non réglé : `RuntimeException('Awaitable is not settled')` très loin de sa cause. +- **Amont** : `Revolt\EventLoop\Internal\DriverSuspension::suspend()` compare l'identité du fiber créateur et refuse autrement — `if ($fiber !== \Fiber::getCurrent()) { throw new \Error('Must not call suspend() from another fiber'); }` (https://github.com/revoltphp/event-loop/blob/main/src/EventLoop/Internal/DriverSuspension.php). +- **Correctif** : faire porter au runtime (ou au contexte) une `\WeakReference` sur le fiber ouvert par `WorkflowFiberDriver::run()` et comparer par identité, avec un message explicite quand un fiber étranger appelle `await()`. diff --git a/documentation/audit/03-contrats-typage.md b/documentation/audit/03-contrats-typage.md new file mode 100644 index 00000000..32a4c6fa --- /dev/null +++ b/documentation/audit/03-contrats-typage.md @@ -0,0 +1,183 @@ +# Contrats d'interface et typage — `Port/`, `Store/`, `Query/`, `Mapping/`, `Attribute/` + +## Synthèse +Les quatorze fichiers de `Port/` forment un ensemble globalement cohérent : douze interfaces y sont +réellement doubles (une implémentation journal, une implémentation Temporal), les docblocs y sont +denses et justifient les choix, et `WorkflowRunCatalogInterface` montre ce que le dépôt sait faire de +mieux — un port qui rend des objets de valeur (`WorkflowRunPage`, `WorkflowRunEvent`) plutôt que des +tableaux. Les défauts se concentrent ailleurs : un contrat de rejeu qui confond « rien d'enregistré » +et « null enregistré », un port de commandes dont un tiers des méthodes n'est honorable que par un +seul des deux backends, une interface à implémentation unique que personne n'injecte, et un +`NullEventStore` qui n'est pas un objet nul mais un bouche-trou de signature. Côté typage, les formes +de tableaux sont abondamment déclarées en PHPDoc mais `phpstan.neon` est réglé sur `level: 5`, palier +auquel ni les types de valeurs d'itérables ni les `mixed` ne sont vérifiés : ces annotations sont de +la documentation, pas des contraintes. Sur deux axes le périmètre est sain — les attributs de +`Attribute/` sont corrects et typés `class-string` là où il faut, et `Query/` comme +`Mapping/EventDataMapper` déclarent des formes de retour exactes et cohérentes avec ce qu'ils +produisent. + +## Constats + +### C1 — `findSideEffectForSlot()` confond « pas enregistré » et « enregistré à `null` » +- **Fichier** : `src/Durable/Port/WorkflowHistorySourceInterface.php:86` +- **Gravité** : majeur +- **Constat** : la méthode rend `mixed` et documente « null if not yet recorded » ; les deux + implémentations rendent bien `null` dans les deux cas + (`src/Durable/Store/EventStoreHistorySource.php:229` puis `:235`, + `src/Bridge/Temporal/Worker/TemporalExecutionHistory.php:567`). Le consommateur teste + `null !== $replayResult` (`src/Durable/ExecutionContext.php:316`) : une closure `sideEffect()` qui + rend `null` est donc **ré-exécutée à chaque rejeu**, et `recordSideEffect()` + (`src/Durable/Store/EventStoreCommandBuffer.php:97`) réémet un `SideEffectRecorded` à chaque passe. + J'ai vérifié que ces ajouts atterrissent en fin de flux, donc l'indexation des autres créneaux + reste alignée ; ce qui reste est la perte de déterminisme sur une closure non déterministe et une + croissance non bornée du journal — sur Temporal, un `RECORD_MARKER` de plus par tâche de workflow. + Les trois méthodes sœurs du même port (`findActivitySlotResult`, `findChildWorkflowForSlot`, + `findNexusOperationSlotResult`, lignes 25, 93, 60) enveloppent justement leur résultat dans une + forme `array{result: mixed, ...}` pour éviter exactement cela : l'incohérence est interne au + contrat. +- **Amont** : non sourcé — défaut démontrable depuis le code et ses deux implémentations, pas une + violation de convention amont. +- **Correctif** : aligner la signature sur ses sœurs, `findSideEffectForSlot(int $slot): ?array` avec + la forme `array{result: mixed}|null`, et adapter le test de `ExecutionContext::sideEffect()`. + +### C2 — `WorkflowCommandBufferInterface` : 5 méthodes sur 14 ne sont honorables que par un backend +- **Fichier** : `src/Durable/Port/WorkflowCommandBufferInterface.php:35` +- **Gravité** : majeur +- **Constat** : `recordUpdateHandled`, `completeChildWorkflow` et `failChildWorkflow` ont un corps + vide côté Temporal (`src/Bridge/Temporal/Worker/TemporalWorkflowCommandBuffer.php:310`, `:356`, + `:362`), et symétriquement `scheduleNexusOperation` et `cancelNexusOperation` lèvent + `NexusUnsupportedByBackendException` côté journal + (`src/Durable/Store/EventStoreCommandBuffer.php:209` et `:214`). Un tiers du contrat est donc + inapplicable selon l'implémentation, et l'interface le documente elle-même comme intentionnel + (`:69`, `:98`, `:128`) — ce qui en fait un contrat déclaré non honorable plutôt qu'un oubli. +- **Amont** : `api-platform/core` sépare l'état en deux contrats d'une seule méthode, + `src/State/ProviderInterface.php` et `src/State/ProcessorInterface.php`, plutôt qu'en un contrat + large dont chaque implémentation ne remplirait qu'une partie + (https://github.com/api-platform/core/blob/main/src/State/ProviderInterface.php). +- **Correctif** : extraire les capacités facultatives en interfaces séparées + (`NexusCommandBufferInterface`, `InlineChildWorkflowRecorderInterface`) que l'appelant teste par + `instanceof`, au lieu d'un `@throws` documentant qu'une implémentation conforme peut refuser. + +### C3 — `WorkflowBackendInterface` : une seule implémentation, aucun consommateur, et un cycle +- **Fichier** : `src/Durable/Port/WorkflowBackendInterface.php:17` +- **Gravité** : majeur +- **Constat** : recensement sur `src/` — la seule implémentation est + `src/Durable/Port/LocalWorkflowBackend.php:18`, et le seul site qui mentionne l'interface est son + enregistrement DI (`src/DurableBundle/DependencyInjection/DurableExtension.php:607`) ; aucune + classe ne la reçoit en injection. Le backend Temporal, que le docbloc annonce comme motif + (`:11-12`), ne l'implémente pas : il passe par `WorkflowTaskRunner`, d'une tout autre forme. En + outre `LocalWorkflowBackend.php:7` importe `Gplanchat\Durable\ExecutionEngine`, qui importe + lui-même `Gplanchat\Durable\Port\ChildWorkflowRunnerInterface` + (`src/Durable/ExecutionEngine.php:10`) : le namespace censé isoler le cœur des backends dépend du + moteur concret. +- **Amont** : `symfony/contracts` ne publie que des interfaces et des traits sans dépendance vers les + composants qui les implémentent (https://github.com/symfony/contracts) — la direction de + dépendance va de l'implémentation vers le contrat, jamais l'inverse. +- **Correctif** : supprimer `WorkflowBackendInterface` et `LocalWorkflowBackend` — `ExecutionEngine` + est déjà la surface publique de démarrage — ou, si le port doit vivre, déplacer + `LocalWorkflowBackend` hors de `Port/` et le faire implémenter par le pont Temporal. + +### C4 — `NullEventStore` n'est pas un objet nul, c'est un bouche-trou de signature +- **Fichier** : `src/Durable/Store/NullEventStore.php:44` +- **Gravité** : majeur +- **Constat** : son propre docbloc l'annonce — « used when an EventStoreInterface is required by + signature but the call site operates in distributed mode … where EventStoreInterface methods are + never actually called ». Son unique usage + (`src/Bridge/Temporal/Worker/WorkflowTaskRunner.php:45`) le passe à `ExecutionRuntime` à côté d'un + `NoopActivityTransport()` : c'est le constructeur d'`ExecutionRuntime` qui sur-spécifie ses + dépendances pour la moitié des backends supportés. Si l'hypothèse « jamais appelées » venait à + tomber, `readStream()` rendant `[]` est indistinguable d'une exécution neuve, et le workflow + rejouerait ses activités au lieu d'échouer. Les deux autres `Null*` du périmètre — + `Port/NullWorkflowTimerDispatcher.php:11` et `Port/NullWorkflowResumeDispatcher.php:10` — sont en + revanche des objets nuls corrects : ne rien faire y est un comportement réel pour un hôte mono- + processus, et les deux le disent. +- **Amont** : `Psr\Log\NullLogger` est l'objet nul de référence parce que journaliser est advisory ; + un store qui *est* la garantie de durabilité ne rentre pas dans ce moule + (https://www.php-fig.org/psr/psr-3/). +- **Correctif** : rendre la dépendance `EventStoreInterface` d'`ExecutionRuntime` explicitement + nullable pour le chemin Temporal, ou faire lever `NullEventStore` sur toute méthode plutôt que + rendre un flux vide silencieux. + +### C5 — `level: 5` : les formes de tableaux déclarées dans les ports ne sont pas vérifiées +- **Fichier** : `phpstan.neon:5` +- **Gravité** : majeur +- **Constat** : les ports déclarent une quinzaine de formes précises — `array{result: mixed, failed: + \Throwable|null}|null` (`Port/WorkflowHistorySourceInterface.php:23`), `array{position: int, kind: + 'signal'|'update', …}` (`:128`), `array{workflowType: string, payload: array, + completed?: bool}|null` (`Store/WorkflowMetadataStore.php:94`) — mais les types de valeurs + d'itérables ne sont contrôlés qu'à partir du niveau 6 et les `mixed` stricts qu'au niveau 9. Les + casts `(string) $payload['activityId']` sur du `mixed` dans + `src/Durable/Mapping/EventDataMapper.php:84,91,94,98` passent donc sans signalement, alors que + c'est précisément la frontière de désérialisation où une forme fausse doit être rejetée. Le + `completed?: bool` optionnel de `WorkflowMetadataStore::get()` recouvre par ailleurs + `hasActiveWorkflowMetadata()` (`:101`) sans que rien n'exprime laquelle des deux fait foi. +- **Amont** : niveaux PHPStan et types documentés + (https://phpstan.org/user-guide/rule-levels, https://phpstan.org/writing-php-code/phpdoc-types) ; + `api-platform/core` double ses `@param` d'un `@psalm-param array{…}` pour que la forme soit + effectivement contrainte. +- **Correctif** : monter `src/Durable` au niveau 6 puis 8 avec une baseline, et remplacer les formes + de retour des ports par des objets de valeur readonly comme le fait déjà + `WorkflowRunCatalogInterface`. + +### C6 — `WorkflowHistorySourceInterface` : `scheduledAt` annoncé en `Duration`, déclaré `float`, toujours `0.0`, jamais lu +- **Fichier** : `src/Durable/Port/WorkflowHistorySourceInterface.php:74` +- **Gravité** : mineur +- **Constat** : le docbloc de classe affirme (`:10`) que « Recorded timings are returned as + `Duration` » et prévient les implémentations tierces du changement, mais `findTimerSlotResult()` + déclare toujours `array{id: string, scheduledAt: float, failed: \Throwable|null}|null`. + `EventStoreHistorySource.php:195` et `:205` y écrivent `0.0` en dur, et une recherche sur + `['scheduledAt']` dans `src/` ne trouve aucun lecteur de cette clé. Une clé de forme que personne + ne produit ni ne consomme reste néanmoins obligatoire pour toute implémentation tierce. +- **Amont** : non sourcé — contradiction interne entre le docbloc de classe et la signature, vérifiée + sur les deux implémentations. +- **Correctif** : retirer `scheduledAt` de la forme de retour, ou la typer `Duration` et la faire + réellement porter la durée enregistrée dans les deux implémentations. + +### C7 — Suffixe `Interface` : trois exceptions dans un ensemble qui l'applique partout ailleurs +- **Fichier** : `src/Durable/Port/WorkflowResumeDispatcher.php:12` +- **Gravité** : mineur +- **Constat** : `WorkflowResumeDispatcher` (4 implémentations), `WorkflowTimerDispatcher` + (`src/Durable/Port/WorkflowTimerDispatcher.php:22`, 3 implémentations) et `WorkflowMetadataStore` + (`src/Durable/Store/WorkflowMetadataStore.php:81`) sont des interfaces sans suffixe, quand les + douze autres de `Port/` et les deux autres de `Store/` l'ont. Le coût est concret : + `NullWorkflowResumeDispatcher implements WorkflowResumeDispatcher` se lit comme une extension de + classe. +- **Amont** : standards de code Symfony, « Suffix interfaces with `Interface` » + (https://symfony.com/doc/current/contributing/code/standards.html). +- **Correctif** : renommer les trois en `*Interface` avec un alias `class_alias` déprécié le temps + d'une version majeure, conformément à la règle du dépôt sur les BC. + +### C8 — Trois docblocs orphelins : la documentation s'attache à la mauvaise méthode +- **Fichier** : `src/Durable/Port/WorkflowCommandBufferInterface.php:107` +- **Gravité** : mineur +- **Constat** : trois sites empilent deux docblocs consécutifs ; PHP n'associe que le dernier, le + premier est perdu et sa méthode se retrouve sans documentation. Ici le docbloc « Records workflow + failure (COMMAND_TYPE_FAIL_WORKFLOW_EXECUTION) » précède celui de `recordVersion()` alors qu'il + décrit `failWorkflow()` (`:118`) ; même schéma à `:120` où la doc de `cancelActivity()` (`:147`) + précède celle de `scheduleNexusOperation()` ; et à + `src/Durable/Port/WorkflowLifecycleInterface.php:42`, où la doc d'`onCancelled()` (`:59`) précède + celle d'`onCancellationDelivered()`. Les IDE et tout outil lisant `getDocComment()` affichent donc + la mauvaise sémantique sur trois méthodes d'annulation et d'échec. +- **Amont** : non sourcé — comportement de `ReflectionMethod::getDocComment()`, qui ne retourne que + le commentaire immédiatement précédent. +- **Correctif** : déplacer chacun des trois docblocs orphelins au-dessus de la méthode qu'il décrit. + +## Axes sains +- **`Attribute/`** : les douze attributs sont corrects — cibles `TARGET_CLASS`/`TARGET_METHOD` + justes, `IS_REPEATABLE` sur le seul qui en a besoin (`FulfilsNexusOperation.php:215`), et + `class-string` sur les trois paramètres qui portent un contrat. Deux détails de cohérence non + bloquants : la moitié sont `final readonly class` et l'autre `final class` à propriétés + `readonly` ; et seuls `AsSignalMethod`/`AsUpdateMethod` acceptent `\BackedEnum|string` là où + `AsQueryMethod`/`AsActivityMethod` exigent `string`. +- **`Query/`** : `WorkflowQueryEvaluator` et sa façade `WorkflowQueryRunner` déclarent des `list<…>` + exacts et cohérents avec ce qu'ils construisent ; rien à redire. +- **`Mapping/EventDataMapper`** : les cinq types `NexusOperation*` absents du `match` de + `toDomainEvent()` sont une exclusion écrite et couverte par un point d'extension de conformité + (`src/Durable/Testing/EventStoreConformanceTestCase.php:363`), pas un oubli. Le seul reproche est + que « événement journalisable » n'est exprimé que par ce tableau protégé, et pas au niveau du type. +- **Recensement des implémentations** : sur les douze interfaces de `Port/`, dix en ont au moins deux + réelles. Les exceptions sont `WorkflowBackendInterface` (constat C3), + `ParentChildWorkflowCoordinatorInterface` (une seule, `src/Durable/ParentChildWorkflowCoordinator.php`, + mais injectée par `ExecutionEngine.php:11` — abstraction défendable) et + `DeclaredActivityFailureInterface` (aucune dans `src/`, ce qui est normal : c'est un point + d'extension utilisateur, consommé par cinq classes du cœur). diff --git a/documentation/audit/04-etat-api.md b/documentation/audit/04-etat-api.md new file mode 100644 index 00000000..9c8c8592 --- /dev/null +++ b/documentation/audit/04-etat-api.md @@ -0,0 +1,62 @@ +# État d'API — pagination, modèle de lecture HTTP, exposer les runs en API + +## Synthèse +Réponse à la question directrice : **non pour les opérations d'item, oui pour les collections moyennant un adaptateur mince**. Le port `WorkflowRunCatalogInterface` n'offre aucune lecture d'un run isolé — un `Get` sur `/workflow_runs/{id}` n'est pas implémentable sans dérouler toutes les pages — et le côté écriture rend `void`, si bien qu'un `ProcessorInterface` n'a pas d'IRI à mettre dans son `Location:`. La pagination, elle, existe et est bien faite (par clé, curseur opaque, `n+1` pour trancher la dernière page) : ce qui manque n'est pas la pagination mais un **total**, qu'aucun des deux backends ne sait donner, ce qui plafonne à `PartialPaginatorInterface` et interdit les liens `page` d'Hydra. Sur un axe le périmètre est sain et il faut le dire : **il n'y a pas de couplage au backend** — le port est propre, les faits sont des DTO `readonly`, l'opacité du curseur est contractuelle et une suite de conformité (`src/Durable/Testing/WorkflowRunCatalogConformanceTestCase.php`) l'atteste ; l'hypothèse « retours en tableaux bruts » est fausse au port et vraie seulement au modèle de vue `RunDashboard`. + +## Constats + +### C1 — Aucune lecture d'un run isolé : l'opération `Get` est inimplémentable +- **Fichier** : `src/Durable/Port/WorkflowRunCatalogInterface.php:34-57` ; `src/Durable/Observation/RunDashboard.php:145-154` +- **Gravité** : bloquant +- **Constat** : Le port n'expose que `listRuns()`, `readHistory()` et `checkHealth()`, et `readHistory()` prend une `WorkflowRunDescription` complète — pas un identifiant — parce que Temporal exige `groupId` + `runId`. `RunDashboard::pick()` ne cherche le run sélectionné que dans la page courante et retombe silencieusement sur `$runs[0]` quand il n'y est pas : le défaut existe déjà côté tableau de bord. Un provider d'item ne peut donc rien faire de `$uriVariables['runId']` sinon paginer jusqu'à le trouver, et les `@id` de la collection pointeraient vers une opération qui n'existe pas. +- **Amont** : `ApiPlatform\State\ProviderInterface::provide(Operation $operation, array $uriVariables = [], array $context = []): object|array|null` — https://github.com/api-platform/core/blob/main/src/State/ProviderInterface.php : l'identifiant arrive en `uriVariables` et le provider doit rendre l'objet. +- **Correctif** : Ajouter au port un `findRun(string $runId, ?string $groupId = null): ?WorkflowRunDescription` — `DescribeWorkflowExecution` côté Temporal, `SELECT … WHERE execution_id = ?` côté DBAL — et faire passer `RunDashboard::pick()` par ce chemin plutôt que par le balayage de page. + +### C2 — Le côté écriture ne rend pas l'identité de la ressource, et n'en rend pas la même selon le backend +- **Fichier** : `src/Durable/Port/WorkflowResumeDispatcher.php:25` ; `src/Bridge/Temporal/Port/TemporalWorkflowResumeDispatcher.php:38-52` ; `src/Bridge/Temporal/WorkflowClient.php:53-63` +- **Gravité** : bloquant +- **Constat** : `dispatchNewWorkflowRun()` rend `void` et l'appelant invente lui-même l'identifiant (`(string) Uuid::v4()`, `symfony/src/Durable/DurableSampleWorkflowRunner.php:49`). Côté Temporal cet identifiant devient le **workflowId** — `startAsync()` le rend tel quel (`WorkflowClient.php:59-62`) et le run id assigné par le serveur est perdu — or le catalogue expose ce workflowId comme `groupId` et le run id comme `runId` (`TemporalWorkflowRunCatalog.php:146-160`). Côté DBAL, le même identifiant *est* le `runId`. Un processor qui répond `202 + Location` écrirait donc une IRI qui résout sur un backend et pas sur l'autre. +- **Amont** : `ApiPlatform\State\ProcessorInterface` ; `documentation/ost/OST003-php-ecosystem-integrations.md:87-101`, qui pose ce processor comme « la même classe sur les deux frameworks » et le premier paquet à écrire. +- **Correctif** : Faire rendre à `dispatchNewWorkflowRun()` un handle (`runId` + `groupId`) plutôt que `void`, et définir une fois pour toutes l'identité de ressource — `groupId ?? runId` — pour que l'IRI émise à l'écriture soit celle que `findRun()` (C1) accepte en lecture. + +### C3 — Aucun total : `PaginatorInterface` est hors d'atteinte, `TraversablePaginator` inutilisable +- **Fichier** : `src/Durable/Observation/WorkflowRunPage.php:17-25` ; `src/Bridge/Dbal/Store/DbalWorkflowRunCatalog.php:64-77` +- **Gravité** : majeur +- **Constat** : `WorkflowRunPage` porte les runs et un `nextCursor`, et rien d'autre. Le backend DBAL tranche l'existence d'une suite en lisant `LIMIT $limit + 1` — un choix délibéré et correct, mais qui exclut de connaître le total ; `ListWorkflowExecutions` de Temporal n'en rend pas non plus. `hydra:totalItems` et le lien `last` sont donc structurellement impossibles. +- **Amont** : https://api-platform.com/docs/core/pagination — « If you are using custom state providers […] you will need to return an instance of `PartialPaginatorInterface` or `PaginatorInterface` » ; `ApiPlatform\State\Pagination\TraversablePaginator::__construct(\Traversable $traversable, float $currentPage, float $itemsPerPage, float $totalItems)` exige un total, et implémente `PaginatorInterface`, pas seulement la variante partielle. +- **Correctif** : Écrire à la main un `PartialPaginatorInterface` (une vingtaine de lignes : `IteratorAggregate` + `count()` sur `$page->runs`) au-dessus de `WorkflowRunPage`, et activer `paginationPartial: true` sur l'opération pour que l'absence de `last` soit un contrat annoncé et non une régression. + +### C4 — Le curseur est opaque, `PartialPaginatorInterface` demande un numéro de page +- **Fichier** : `src/Durable/Observation/WorkflowRunPage.php:9-16` ; `src/Bridge/Temporal/Store/TemporalWorkflowRunCatalog.php:52-54,85` +- **Gravité** : majeur +- **Constat** : Le contrat impose que `nextCursor` soit opaque et rendu tel quel au même catalogue — jeton de page Temporal encodé en base64 d'un côté, `started_at`+`execution_id` de l'autre. Or `PartialPaginatorInterface::getCurrentPage(): float` est un **numéro** de page, et c'est lui qui alimente les liens `?page=N` d'Hydra : un provider ne peut pas les honorer, puisqu'il ne sait pas sauter à la page N. +- **Amont** : https://github.com/api-platform/core/blob/main/src/State/Pagination/PartialPaginatorInterface.php (`getCurrentPage(): float`, `getItemsPerPage(): float`) ; https://api-platform.com/docs/core/pagination — la pagination par curseur (`paginationViaCursor`) n'existe que pour Doctrine ORM/ODM et Elasticsearch, et exige un `RangeFilter` + un `OrderFilter` sur la propriété du curseur. +- **Correctif** : Ne pas passer par la pagination d'Hydra : déclarer `cursor` en paramètre de requête explicite sur l'opération, forcer `paginationPartial`, et émettre le lien `next` soi-même à partir de `$page->nextCursor`. + +### C5 — Un seul filtre, et l'ordre est gelé par le curseur +- **Fichier** : `src/Durable/Port/WorkflowRunCatalogInterface.php:34` ; `src/Bridge/Dbal/Store/DbalWorkflowRunCatalog.php:55-68,132-134` +- **Gravité** : majeur +- **Constat** : `listRuns()` n'accepte qu'un `WorkflowRunStatus` : pas de nom de workflow, pas de fenêtre temporelle, pas de tri. Le curseur DBAL encode `started_at` et `execution_id` et la requête est figée en `ORDER BY started_at DESC, execution_id ASC` — un `OrderFilter` invaliderait tous les curseurs déjà émis. Un `SearchFilter`/`DateFilter` n'aurait donc rien où s'accrocher, et un provider ne pourrait qu'ignorer `$context['filters']`. +- **Amont** : `ApiPlatform\Doctrine\Orm\State\CollectionProvider` — https://github.com/api-platform/core/blob/main/src/Doctrine/Orm/State/CollectionProvider.php : les filtres n'y sont pas appliqués par le provider mais par les extensions sur le `QueryBuilder`, mécanisme dont un port non-Doctrine ne dispose pas et qu'il doit donc traduire lui-même. +- **Correctif** : Introduire un objet de critère (`workflowName`, intervalle, statut, tri) passé à `listRuns()` — plutôt qu'empiler des paramètres nullables — en documentant que le tri fait partie de la clé du curseur et qu'un curseur d'un autre tri est refusé. + +### C6 — Le modèle de lecture existant est un contrat Twig, pas une ressource sérialisable +- **Fichier** : `src/Durable/Observation/RunDashboard.php:36-45,120-140` +- **Gravité** : majeur +- **Constat** : `build()` rend un `array{backend: array, runs: list>, kpis: …, pagination: …, selectedRun: …}` dont les clés `startedAt`, `endedAt`, `groupId` sont **conditionnellement absentes** — règle assumée et juste pour un gabarit, mais qui interdit un schéma OpenAPI stable et une hydratation client. La dégradation en tableaux est ici, pas au port : `WorkflowRunPage`, `WorkflowRunDescription` et `BackendHealth` sont des `final readonly class` correctes. +- **Amont** : https://api-platform.com/docs/guides/custom-pagination — le provider rend des objets de ressource, la normalisation est faite par le sérialiseur à partir de propriétés typées, pas d'un tableau à clés optionnelles. +- **Correctif** : Brancher le provider directement sur `WorkflowRunCatalogInterface` et rendre les `WorkflowRunDescription` (ou une ressource qui les enveloppe), en laissant `RunDashboard` à ce pour quoi il a été écrit — les gabarits Sylius et Magento. + +### C7 — `details` est du vocabulaire de backend : pas de schéma décrivable +- **Fichier** : `src/Durable/Observation/WorkflowRunEvent.php:22-25,61` +- **Gravité** : mineur +- **Constat** : `$details` est un `array` dont le docblock déclare explicitement que le contenu est celui du backend et non celui du composant, la normalisation étant remise à plus tard. Un gabarit s'en accommode ; un schéma OpenAPI ne peut le décrire que comme un objet libre, et un client d'API ne peut donc s'adosser à aucune clé. +- **Amont** : non sourcé (constat interne au dépôt ; c'est la conséquence directe du docblock, pas une règle amont). +- **Correctif** : Garder le brut mais le sortir du contrat : n'exposer comme propriétés typées que `sequence`, `recordedAt`, `kind`, `label`, `actionKey`, `started`, `failed`, et publier `details` en objet libre déclaré comme tel plutôt qu'en propriété promise. + +### C8 — Une sonde de santé par requête de collection +- **Fichier** : `src/Durable/Observation/RunDashboard.php:62-80` +- **Gravité** : mineur +- **Constat** : `build()` appelle `checkHealth()` inconditionnellement avant toute liste, ce qui vaut un aller-retour gRPC ou SQL supplémentaire par appel. Le compromis est bon pour une page consultée à la main — mieux vaut une sonde qu'un tableau vide et serein — mais sur une collection HTTP appelée en boucle il double le trafic vers le backend. +- **Amont** : non sourcé (arbitrage de coût, pas une règle API Platform). +- **Correctif** : Exposer la santé comme sa propre ressource (`GET /durable/backend_health`, provider dédié) et ne pas la faire porter par le provider de collection. diff --git a/documentation/audit/05-conteneur-performance.md b/documentation/audit/05-conteneur-performance.md new file mode 100644 index 00000000..5360500c --- /dev/null +++ b/documentation/audit/05-conteneur-performance.md @@ -0,0 +1,62 @@ +# Compilation du conteneur — paresse des services, coût au boot et au rejeu + +## Synthèse +Le paquet cœur est propre sur l'axe des dépendances cachées : `grep` ne trouve aucun `use` hors `Gplanchat\` en dehors de `Psr\Cache` et de PHPUnit (cantonné à `Testing/`), donc `php >=8.2` + `psr/cache` décrivent bien ce que le cœur charge. Le reste du périmètre porte deux coûts structurels : le conteneur construit **tous** les gestionnaires d'activité pour en exécuter un seul, et il expose 48 services publics là où la convention Symfony depuis 4.0 est le privé par défaut. Sur le chemin de rejeu, il n'y a pas d'`array_merge` en boucle — l'axe est sain — mais `EventStoreHistorySource` relit le journal entier à chaque consultation de slot, ce qui donne un rejeu en O(commandes × longueur du journal) et, sur le backend DBAL, un `SELECT` complet par consultation. Le cache warmer, enfin, écrit dans un pool d'exécution avec un TTL d'une heure au lieu du répertoire de build : ce qu'il produit n'est ni invalidable par `cache:clear` ni préchargeable, et il est un no-op silencieux dans la configuration par défaut. + +## Constats + +### C1 — Le conteneur instancie tous les gestionnaires d'activité pour en exécuter un seul +- **Fichier** : `src/DurableBundle/DependencyInjection/Compiler/ActivityHandlerPass.php:75` (et `src/Durable/RegistryActivityExecutor.php:12`, `src/DurableBundle/DependencyInjection/DurableExtension.php:600`) +- **Gravité** : majeur +- **Constat** : la passe pose un `addMethodCall('register', [$activityName, [new Reference($invokerId), '__invoke']])` par méthode d'activité ; l'argument étant un `callable` réel, le conteneur doit construire chaque invocateur — donc chaque service gestionnaire et tout son graphe — au moment où `ActivityExecutor` naît. Le conteneur compilé du banc le montre : `symfony/var/cache/dev/ContainerFHx7IVm/getActivityExecutorService.php` construit onze `…ActivityHandler` en ligne avant de rendre l'exécuteur. `NexusHandlerPass.php:110` a exactement la même forme sur `NexusOperationRegistry`. +- **Amont** : `symfony/console/DependencyInjection/AddConsoleCommandPass.php:149-160` — `ServiceLocatorTagPass::register($container, $lazyCommandRefs)` + carte `nom => id`, précisément pour que seule la commande appelée soit instanciée ; `symfony/dependency-injection/Compiler/ServiceLocatorTagPass.php`. +- **Correctif** : passer à `RegistryActivityExecutor` un `ServiceLocator` (via `ServiceLocatorTagPass::register()`) plus la carte `nom d'activité => id de service`, et résoudre dans `execute()`. Même changement pour `NexusOperationRegistry`. + +### C2 — Le rejeu relit le journal entier à chaque consultation de slot +- **Fichier** : `src/Durable/Store/EventStoreHistorySource.php:52` (et 104, 116, 158, 175, 211, 226, 241, 252, 279, 295, 329) +- **Gravité** : majeur +- **Constat** : les douze méthodes du port font chacune un `foreach ($this->eventStore->readStream(...))` complet, sans mémoïsation. `ExecutionContext::activity()` en déclenche trois à quatre par activité (`ExecutionContext.php:103`, `:111`, `:116`, `:274`), et `countRecordedMessages()` (`ExecutionContext.php:463`) boucle `messageAt($n)` — un balayage complet par message. Le rejeu est donc en O(commandes × longueur du journal), et sur DBAL chaque balayage est un `SELECT … WHERE execution_id = ? ORDER BY id` distinct (`src/Bridge/Dbal/Store/DbalEventStore.php:58-66`) : un workflow de 100 activités émet plusieurs centaines de lectures intégrales du même flux. +- **Amont** : le principe — mémoïser derrière un adaptateur mémoire ce qu'on relit sur un chemin chaud — est celui de `symfony/validator/Mapping/Factory/LazyLoadingMetadataFactory.php` (cache `ArrayAdapter` interposé). Pas de règle Symfony directement transposable ici : **constat non sourcé** sur la forme exacte du correctif. +- **Correctif** : matérialiser le flux une fois par instance d'`EventStoreHistorySource` (le rejeu porte sur un journal figé) et dériver les index par type de slot en une passe, plutôt qu'un balayage par question. + +### C3 — Le cache warmer écrit dans un pool d'exécution, avec un TTL, et non dans le répertoire de build +- **Fichier** : `src/DurableBundle/CacheWarmer/ActivityContractCacheWarmer.php:23-29` (et `src/Durable/Activity/ActivityContractResolver.php:19`) +- **Gravité** : majeur +- **Constat** : `warmUp()` ignore `$buildDir`, pousse dans un pool PSR-6 quelconque et rend `[]` — donc aucune classe à précharger. Le résolveur pose un `expiresAfter(3600)` sur une clé qui ne contient que le nom de classe : le fruit du warmup expire une heure après le déploiement, et sur un pool partagé entre déploiements (Redis) une métadonnée de contrat modifiée reste servie jusqu'à une heure, car rien ne lie la clé à une ressource de code. +- **Amont** : `symfony/framework-bundle/CacheWarmer/AbstractPhpFileCacheWarmer.php:35-56` — `doWarmUp()` rend `false` sans `$buildDir`, le résultat va dans un `PhpArrayAdapter` du répertoire de build, sans TTL, et `warmUp()` rend la liste des classes à précharger. +- **Correctif** : calquer `AbstractPhpFileCacheWarmer` — écrire un `PhpArrayAdapter` sous `$buildDir` (donc effacé par `cache:clear`), supprimer le TTL, rendre la liste de préchargement. + +### C4 — 48 `setPublic(true)` : ni inlining, ni suppression des définitions inutilisées +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:157` (et 47 autres occurrences dans le même fichier) +- **Gravité** : majeur +- **Constat** : presque tout ce que l'extension enregistre est public, alias compris. Le conteneur compilé du banc garde 17 services `Gplanchat\…` publics et 3 alias publics. Un service public n'est ni inliné ni retiré, et son identifiant devient une API que l'on ne peut plus renommer sans BC. +- **Amont** : `symfony/dependency-injection/Compiler/InlineServiceDefinitionsPass.php:199` (`if ($definition->isPublic()` → pas d'inlining) et `Compiler/RemoveUnusedDefinitionsPass.php:41,47` (public → conservé) ; c'est la raison du passage au privé par défaut en Symfony 4.0. +- **Correctif** : privé par défaut ; ne laisser public que ce qu'un test d'intégration ou une commande récupère vraiment par `getContainer()->get()`, et faire passer le reste par l'autowiring ou un `ServiceLocator` de test. + +### C5 — Le pool de cache des contrats est nul par défaut, et un pool déclaré en alias est silencieusement ignoré +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:503` (et `Configuration.php:58`) +- **Gravité** : mineur +- **Constat** : `activity_contracts.cache` vaut `null` par défaut, donc `ActivityContractResolver` fonctionne sans cache et le warmer de C3, s'il est configuré, ne conserve rien — le warmup est un no-op complet. Et le garde est `hasDefinition($cacheId)` : un pool désigné par un alias (le cas d'un `cache.pool` remappé) donne `false` et le cache est abandonné sans un mot. +- **Amont** : `symfony/dependency-injection/ContainerBuilder::has()` couvre définitions **et** alias, là où `hasDefinition()` ne voit que les définitions ; le `framework.cache.pools` de FrameworkBundle produit des services adressés par identifiant de pool. +- **Correctif** : `$container->has($cacheId)` plutôt que `hasDefinition()`, et lever si `cache` est configuré mais introuvable au lieu de retomber sur `null` ; par défaut, pointer sur `cache.system`. + +### C6 — Le journal DBAL exécute du DDL depuis le chemin de lecture, sans interrupteur +- **Fichier** : `src/Bridge/Dbal/Store/DbalEventStore.php:60` (et `src/Bridge/Dbal/Schema/DurableSchema.php:34-52`) +- **Gravité** : mineur +- **Constat** : `readStream()`, `readStreamWithRecordedAt()` et `countEventsInStream()` appellent `schema->ensure()`, qui au premier appel de chaque processus interroge le `SchemaManager` sur quatre tables puis émet les `CREATE TABLE` manquants. La configuration (`Configuration.php:18-25`) n'offre aucun moyen de le couper : chaque worker paie ce va-et-vient au boot, et la production mute son schéma depuis le chemin d'exécution. +- **Amont** : `symfony/messenger`, transport Doctrine — option `auto_setup` (défaut `true`, documentée comme à désactiver en production, la table étant alors créée par une migration). +- **Correctif** : ajouter un `dbal.auto_setup` (défaut `true`, `false` recommandé en production) qui court-circuite `ensure()`, le schéma étant déjà déclaré via `configureSchema`. + +### C7 — Le drain synchrone tourne à vide en relisant tout le journal à chaque tour +- **Fichier** : `src/Durable/ExecutionRuntime.php:79-82` (et `:93`) +- **Gravité** : mineur +- **Constat** : `while (!$awaitable->isSettled()) { drainActivityQueueOnce(); checkTimers(); }` ne cède jamais la main et n'attend jamais ; `checkTimers()` relit le flux entier à chaque tour, et `drainActivityQueueOnce()` appelle `ActivityEventJournal::lastTerminalOutcome()` qui en relit un autre. Une attente sur minuteur non échu occupe donc un cœur à 100 % à rejouer le journal jusqu'à l'échéance. Le chemin ne concerne que `distributed=false` — le défaut du constructeur (`:51`), ce que le harness de test et `InMemoryWorkflowRunner` empruntent — l'extension posant `true` (`DurableExtension.php:478`). +- **Amont** : constat non sourcé (règle algorithmique, pas une convention Symfony). +- **Correctif** : calculer le prochain échéancier depuis `TimerWakeDelayCalculator` et dormir jusque-là quand la file d'activités est vide, plutôt que de boucler. + +### C8 — Le bundle exige `symfony/uid`, qu'il n'utilise pas, et référence deux ponts qu'il ne déclare même pas en `suggest` +- **Fichier** : `src/DurableBundle/composer.json:28` +- **Gravité** : mineur +- **Constat** : aucun `Symfony\Component\Uid` n'apparaît dans `src/DurableBundle/` ni `src/Durable/` — le cœur génère ses UUID v7 en PHP pur (`src/Durable/Uuid/NativeUuidV7Generator.php`). À l'inverse, `DurableExtension.php:7-29` importe une vingtaine de classes de `Gplanchat\Bridge\Temporal\*`, `Gplanchat\Bridge\Dbal\*` et `Temporal\Api\Workflowservice\V1\WorkflowServiceClient`, absentes du `require` **et** du `suggest` du bundle (le cœur, lui, les suggère). +- **Amont** : convention Composer/Symfony — un paquet déclare ce qu'il charge ; `symfony/framework-bundle` liste en `suggest` chaque composant que son extension sait câbler. +- **Correctif** : retirer `symfony/uid` du `require` ; ajouter `gplanchat/durable-bridge-temporal` et `gplanchat/durable-bridge-dbal` en `suggest`, avec la mention que les blocs de configuration correspondants les exigent. diff --git a/documentation/audit/06-configuration.md b/documentation/audit/06-configuration.md new file mode 100644 index 00000000..4f0ab235 --- /dev/null +++ b/documentation/audit/06-configuration.md @@ -0,0 +1,155 @@ +# Sémantique de l'arbre de configuration — validation, messages d'erreur, normalisation + +## Synthèse + +L'arbre `durable` est court, entièrement typé et couvre son périmètre : les trois nœuds `type` +(`event_store`, `workflow_metadata`, `child_workflow.parent_link_store`) et `activity_transport.type` +sont de vrais `enumNode()` à `values()` explicites, et chaque nœud a une valeur par défaut — la +question « des scalaires qui devraient être des enums » ne se pose donc que pour les identifiants de +service et pour `temporal.dsn`. Le défaut central est ailleurs : le choix du backend n'est pas porté +par un nœud mais réparti sur trois (`event_store.type`, `temporal.dsn`, `temporal.journal`), et la +seule combinaison interdite est gardée depuis l'extension par une `\LogicException` nue plutôt que +par `->validate()->thenInvalid()`, ce qui prive le message de son chemin de configuration. Les +identifiants de service qui arrivent de la config (pool PSR-6, transport Messenger, connexion DBAL, +`lock_factory`) ne sont ni validés ni rapportés par leur chemin : le pool est avalé en silence, les +autres échouent avec un message qui ne nomme jamais l'option fautive. Enfin `temporal.dsn` est un +scalaire libre dont la seule validation vit dans une fabrique appelée à l'exécution — le dépôt +documente lui-même le symptôme dans `sylius/config/packages/durable.yaml`. + +## Constats + +### C1 — La seule contrainte inter-nœuds est une `\LogicException`, pas un `thenInvalid()` + +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:137-139` +- **Gravité** : majeur +- **Constat** : le couple interdit `event_store.type: dbal` + Temporal natif est refusé depuis + `registerDbalStores()` par un `throw new \LogicException(...)`. La contrainte est pourtant + purement configurationnelle : elle se calcule sur `$config` seul, sans jamais consulter le + conteneur. Le message obtenu n'est donc préfixé d'aucun chemin (`durable.event_store.type`), n'est + pas une `InvalidConfigurationException`, et échappe à ce que `lint:container` sait rapporter. +- **Amont** : `vendor/symfony/framework-bundle/DependencyInjection/Configuration.php:2332-2336` — + `mailer` exprime exactement ce genre d'exclusion mutuelle en `->validate()->ifTrue(...) + ->thenInvalid('"dsn" and "transports" cannot be used together.')`. Le pendant en extension + (`FrameworkExtension.php:2586-2587`, `failure_transport`) n'est employé que là où le contrôle a + besoin de l'état du conteneur — ce qui n'est pas le cas ici. +- **Correctif** : déplacer le test dans `Configuration::getConfigTreeBuilder()`, en `->validate()` + sur le nœud racine, avec `->thenInvalid()` : le texte actuel est bon, il lui manque seulement le + chemin que `thenInvalid()` ajoute gratuitement. + +### C2 — Le garde du choix de backend n'est posé que sur une branche sur deux + +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:424` (et `:432`) +- **Gravité** : majeur +- **Constat** : déclarer un journal que `temporal.dsn` vient ensuite recouvrir provoque un refus dur + si ce journal est `dbal` (`:137`), et pas un mot s'il est `in_memory` : l'alias + `EventStoreInterface` est réécrit vers `durable.event_store.temporal` sans que la valeur écrite + dans `event_store.type` soit jamais confrontée à `temporal.dsn`. C'est la même classe d'erreur + utilisateur, traitée deux fois différemment ; `symfony/config/packages/durable.yaml:27-30` + illustre le cas — `type: in_memory` y coexiste avec un DSN sans que rien ne le signale. +- **Amont** : `vendor/symfony/framework-bundle/DependencyInjection/Configuration.php:2332-2336` — + la règle amont est qu'une exclusion mutuelle se déclare une fois, sur le nœud parent, pour toutes + ses combinaisons, et non branche par branche à l'endroit du câblage. +- **Correctif** : faire porter le choix par un seul nœud — un `enumNode('backend')` à trois valeurs + (`in_memory`/`dbal`/`temporal`), ou à défaut un `->validate()` unique sur `durable` qui refuse + tout `event_store.type` explicite en présence d'un `temporal.dsn` avec `journal: true`. + +### C3 — `temporal.dsn` est un scalaire libre, validé seulement à l'instanciation du service + +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:36-39` +- **Gravité** : majeur +- **Constat** : le nœud n'a ni `cannotBeEmpty()`, ni contrainte de schéma. La seule vérification est + `TemporalConnection::fromDsn()` (`src/Bridge/Temporal/TemporalConnection.php:96-98`), posée en + `setFactory()` (`DurableExtension.php:339-341`) donc exécutée à l'instanciation. Une faute de + frappe produit `InvalidArgumentException: Invalid temporal:// DSN`, sans chemin de configuration + et à un moment arbitraire ; `sylius/config/packages/durable.yaml:17-21` décrit précisément ce + symptôme observé sur `doctrine:schema:create`. +- **Amont** : `vendor/symfony/dependency-injection/Compiler/ValidateEnvPlaceholdersPass.php:29` + (`TYPE_FIXTURES = ['array' => [], 'bool' => false, ..., 'string' => '']`) — les placeholders + `%env()%` sont remplacés par une chaîne vide avant le passage dans l'arbre, donc une validation + déclarative sur ce nœud ne casse pas les configurations pilotées par variable d'environnement. +- **Correctif** : ajouter sur le nœud un `->validate()->ifTrue(fn ($v) => null !== $v && '' !== $v + && !preg_match('#^temporal(-journal|-application)?://#i', $v))->thenInvalid(...)` ; l'erreur + devient `durable.temporal.dsn` à la compilation, et la fabrique reste le filet de sécurité. + +### C4 — Les identifiants de service venus de la config ne sont ni validés ni rapportés par leur chemin + +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:503-505` +- **Gravité** : majeur +- **Constat** : `activity_contracts.cache` passe par `$container->hasDefinition($cacheId)` — un pool + mal orthographié, ou simplement déclaré comme *alias*, retombe silencieusement sur `null` et le + cache de contrats disparaît sans un mot. Le contrôle dépend en outre de l'ordre des bundles : + `cache.app` n'existe que parce que `symfony/config/bundles.php:4` place FrameworkBundle avant + `DurableBundle` (`:6`). Les trois autres identifiants — `messenger.transport.` + (`:452-455`), `dbal.connection` (`:141`), `dbal.lock_factory` (`:163`) — partent en `Reference` + brute : l'échec arrive bien à la compilation, mais le message parle d'un service introuvable et ne + nomme jamais l'option Durable qui l'a produit. +- **Amont** : `vendor/symfony/framework-bundle/DependencyInjection/FrameworkExtension.php:2586-2587` + — quand un identifiant vient de la configuration, FrameworkBundle lève en nommant le concept de + configuration (« the failure transport "%s" is not a valid transport or service id »), il ne + dégrade jamais en silence. +- **Correctif** : remplacer `hasDefinition()` par `has()` et lever au lieu d'ignorer ; pour les + trois autres, encadrer la `Reference` d'un test `$container->has()` avec un message citant + `durable.activity_transport.transport_name` / `durable.dbal.connection`. Ajouter `cannotBeEmpty()` + sur ces scalaires (`Configuration.php:22,23,51`), une chaîne vide donnant aujourd'hui un + `Reference('')`. + +### C5 — `activity_transport.table_name` est un nœud fantôme + +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:50` +- **Gravité** : mineur +- **Constat** : le nœud est déclaré avec le défaut `durable_activity_outbox` et exposé jusque dans le + fichier de référence du banc (`symfony/config/reference.php:708`), mais aucun code ne le lit : + `registerActivityTransport()` (`DurableExtension.php:438-464`) n'utilise que `type` et + `transport_name`, et `registerCommands()` (`:721-724`) que ces deux-là également. Il apparaît dans + `config:dump-reference`, invite à une valeur, et la jette. +- **Amont** : non sourcé (règle générale : l'arbre de configuration est la surface publique du + bundle, `config:dump-reference` en est la documentation). +- **Correctif** : supprimer le nœud — ou, si une table d'outbox est prévue, le brancher ; laisser un + nœud documenté sans effet est le pire des deux. + +### C6 — Les `->info()` sont sur les nœuds descriptifs, pas sur ceux qui gouvernent le câblage + +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:29` +- **Gravité** : mineur +- **Constat** : `dbal` (`:20`) et `temporal.journal` (`:42`) portent des `info()` de plusieurs + phrases, tandis que les nœuds dont dépend réellement le montage n'en ont aucun : + `event_store.type` (`:29`), `activity_transport.type` (`:49`), `activity_transport.transport_name` + (`:51`), `max_activity_retries` (`:54`), `child_workflow.async_messenger` (`:69`), + `workflow_metadata.type` (`:82`). `max_activity_retries` n'a par ailleurs pas de `->min(0)`. +- **Amont** : `vendor/symfony/framework-bundle/DependencyInjection/Configuration.php:1766` et + `:1793-1797` — messenger documente chaque option scalaire et borne systématiquement ses entiers + (`->integerNode('max_retries')->defaultValue(3)->min(0)`). +- **Correctif** : une ligne d'`info()` par nœud restant, et `->min(0)` sur `max_activity_retries`. + +### C7 — Aucune normalisation de forme courte + +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:26-53` +- **Gravité** : mineur +- **Constat** : chaque section n'accepte que sa forme longue. `durable: { event_store: dbal }` ou + `durable: { temporal: 'temporal://localhost:7233' }` sont rejetés, alors que ce sont les deux + écritures qu'un utilisateur tente d'abord, l'un de ces nœuds n'ayant qu'une seule clé signifiante. + Aucun `beforeNormalization()` ni `acceptAndWrap()` n'apparaît dans le fichier. +- **Amont** : `vendor/symfony/framework-bundle/DependencyInjection/Configuration.php:1759-1765` — + le prototype de `messenger.transports` déclare `->acceptAndWrap(['string'], 'dsn')`, ce qui permet + d'écrire un transport comme une simple chaîne de DSN (`Symfony\Component\Config\Definition\ + Builder\ArrayNodeDefinition::acceptAndWrap()`, `vendor/symfony/config/.../ArrayNodeDefinition.php:95`). +- **Correctif** : `->acceptAndWrap(['string'], 'dsn')` sur `temporal` et + `->acceptAndWrap(['string'], 'type')` sur `event_store`, `workflow_metadata`, + `activity_transport` et `parent_link_store`. + +### C8 — Deux extensions sans `Configuration` : leur clé de config est acceptée puis ignorée + +- **Fichier** : `src/Bridge/Temporal/DependencyInjection/TemporalBridgeExtension.php:14-18` +- **Gravité** : remarque +- **Constat** : `TemporalBridgeExtension::load()` et `DurablePluginExtension::load()` + (`src/DurablePlugin/DependencyInjection/DurablePluginExtension.php:17-21`) reçoivent `$configs` + et ne l'utilisent pas ; aucune classe `Configuration` n'accompagne ces extensions. Écrire + `temporal_bridge: { foo: bar }` ou `durable_plugin: { ... }` dans un `config/packages/` ne + produit donc ni erreur ni effet, et `config:dump-reference temporal_bridge` ne rend rien. + `TemporalBridgeExtension::getAlias()` (`:20-23`) réexprime par ailleurs l'alias que la convention + de nommage déduit déjà de la classe. +- **Amont** : `vendor/symfony/dependency-injection/Compiler/ValidateEnvPlaceholdersPass.php:59-66` + — la passe ne valide une extension que si elle expose une `Configuration` ; sans elle, la section + est hors de toute vérification. +- **Correctif** : si ces bundles n'ont réellement aucune option, c'est acceptable ; sinon, ajouter + une `Configuration` minimale (ne serait-ce que vide) pour que toute clé inconnue soit rejetée. diff --git a/documentation/audit/07-compatibilite-statique.md b/documentation/audit/07-compatibilite-statique.md new file mode 100644 index 00000000..b6a9a648 --- /dev/null +++ b/documentation/audit/07-compatibilite-statique.md @@ -0,0 +1,55 @@ +# Compatibilité PHP/Symfony — dépréciations, matrice CI, analyse statique + +## Synthèse +La matrice CI est la partie la plus solide du périmètre : elle croise 6.4 LTS, 7.4 LTS, le plancher de la ligne 8 et sa tête, en `lowest` et en `highest`, avec PHPStan à chaque entrée, et chaque ligne porte la raison d'être du bord qu'elle mesure. Le plancher `php >=8.2` est tenu dans le code : hors code généré, aucune fonction ni syntaxe 8.3/8.4 (`json_validate`, constantes typées d'interface, hooks, visibilité asymétrique) n'apparaît — les 12 `#[\Override]` sont le seul emprunt à 8.3, et ils sont inertes sur 8.2. Sur les dépréciations : zéro `@deprecated`, zéro `trigger_deprecation`, aucune dépendance à `symfony/deprecation-contracts` dans tout le dépôt — la migration passe par le jeu Rector `durable-upgrade` (13 renommages d'alpha8) et par UPGRADE.md pour la seule suppression que Rector ne peut pas exprimer, ce qui tient avant 1.0 mais n'offre aucun avertissement à l'exécution. Ce qui ne tient pas, c'est la couche déclarative en dessous : aucun `composer validate`, aucune résolution isolée d'un paquet satellite dans les cinq workflows, donc les `composer.json` des paquets publiés sont structurellement invérifiables — et trois d'entre eux sont effectivement faux. Enfin la configuration des analyseurs a pourri : la baseline Psalm est morte à 87 %, et l'option qui le signalerait est explicitement éteinte. + +## Constats + +### C1 — Le bundle câble deux ponts qu'il ne déclare ni en `require`, ni en `require-dev`, ni en `suggest` +- **Fichier** : `src/DurableBundle/composer.json:16-29` ; `src/DurableBundle/DependencyInjection/DurableExtension.php:7-29` +- **Gravité** : majeur +- **Constat** : `DurableExtension` importe en dur 7 classes de `gplanchat/durable-bridge-dbal` (lignes 7-13) et 16 de `gplanchat/durable-bridge-temporal` (lignes 14-29), plus `Temporal\Api\Workflowservice\V1\WorkflowServiceClient` (ligne 79) ; le `composer.json` du bundle ne mentionne aucun des deux paquets, pas même en `suggest`. Le câblage est protégé par la configuration (`registerEventStore`, ligne 338) et `Foo::class` ne déclenche aucun autochargement, donc rien ne casse à l'installation : le conteneur compile, puis fatal « class not found » au premier appel, sans aucun signal au moment de `composer require`. Le paquet cœur, lui, fait la déclaration correctement — `src/Durable/composer.json:26-27` suggère les deux mêmes ponts — l'incohérence est entre paquets frères du même dépôt. +- **Amont** : `symfony/symfony:src/Symfony/Bundle/FrameworkBundle/composer.json` — FrameworkBundle référence en dur des dizaines de composants qu'il ne requiert pas et les déclare tous en `require-dev` (pour que son propre CI analyse le câblage) plus `conflict` sur les versions incâblables. Côté vérification, `symfony/symfony:.github/workflows/unit-tests.yml` (mode *low-deps*) entre dans chaque répertoire de composant et l'y résout contre des dépôts `path` locaux : une déclaration manquante y échoue. +- **Correctif** : ajouter les deux ponts en `require-dev` + `suggest` du bundle (jamais en `require` : cela imposerait `ext-grpc` à tout consommateur), et ajouter à `ci.yml` un job qui, pour chaque `src/*/composer.json`, fait `composer validate --strict` puis `maglnet/composer-require-checker` — le même job ferme C1, C3 et C4. Corollaire de la même cause : `find src -name 'phpstan*.neon'` ne rend rien, le niveau PHPStan n'est déclaré qu'une fois, à la racine ; après split, aucun satellite ne peut s'analyser seul. + +### C2 — La baseline Psalm est morte à 87 %, et l'option qui le dirait est éteinte +- **Fichier** : `psalm.xml:8` (`findUnusedBaselineEntry="false"`) ; `psalm.xml:42` ; `psalm-baseline.xml` +- **Gravité** : majeur +- **Constat** : la baseline compte 196 entrées sur 71 fichiers, dont 171 (87 %) sont des `MissingOverrideAttribute` — un diagnostic déjà supprimé globalement à `psalm.xml:42` — et dont trois fichiers n'existent même plus (`src/Durable/Awaitable/CancellingAnyAwaitable.php`, `src/DurableBundle/DependencyInjection/Compiler/RegisterWorkflowDispatchProfilerMiddlewarePass.php`, `src/DurablePlugin/Dashboard/TemporalEventsDashboardDataProvider.php`). Chronologie vérifiée en `git log` : baseline créée le 2026-03-29, `findUnusedBaselineEntry="false"` posé le 2026-04-06 (commit `e06eebbf`, « resolve all CI failures »), suppression globale le lendemain (`659c2e83`), puis baseline réécrite deux fois en août sans qu'une seule des 171 entrées tombe. Réponse à la question posée : elle ne masque **pas** de vrai problème — les 25 entrées réelles ont été relues une à une, et les trois plus inquiétantes (`InvalidThrow` sur `AwaitableAdapter.php:27`, `InvalidArrayOffset` sur `ActivitySpy.php:136`, les `InvalidPropertyAssignmentValue`) sont des invariants que Psalm ne sait pas suivre, pas des bugs. +- **Amont** : documentation Psalm, `findUnusedBaselineEntry` — « Emits `UnusedBaselineEntry` when a baseline entry is not being used to suppress an issue » () ; la maintenance attendue est `psalm --update-baseline`, qui « will remove fixed issues » (). +- **Correctif** : régénérer la baseline (`psalm --set-baseline`, elle retombe à ~25 entrées) et remettre `findUnusedBaselineEntry="true"`, pour qu'une entrée devenue inutile fasse échouer le job plutôt que de s'accumuler ; trancher au passage `psalm.xml:41`, qui justifie la suppression par « le projet cible PHP 8.2+ » alors que 12 `#[\Override]` sont écrits (`DurableDataCollector`, `DurableExecutionTrace`, `ResetDurableProfilerListener`, `LocalWorkflowBackend`, `TemporalReadThroughEventStore`) — la règle est à moitié appliquée et son motif affiché est faux. + +### C3 — Deux dépendances dures jamais utilisées, imposées à tous les consommateurs +- **Fichier** : `src/DurablePlugin/composer.json:20` ; `src/DurableBundle/composer.json:28` +- **Gravité** : majeur +- **Constat** : `knplabs/knp-menu: ^3.5` est en `require` dur du plugin, mais aucune ligne de `src/DurablePlugin/` ne référence le namespace `Knp\` — `AdminMenuListener.php:17-27` évite délibérément de typer contre KnpMenu et duck-type l'événement par `method_exists()`. Symétriquement, `symfony/uid` est en `require` dur du bundle alors qu'aucun `use Symfony\Component\Uid` n'existe : le bundle génère ses identifiants avec son propre `Gplanchat\Durable\Uuid\NativeUuidV7Generator` (`DurableExtension.php:538`). Deux paquets tirés dans le graphe de chaque consommateur pour rien. +- **Amont** : `composer validate --strict` ne les voit pas — c'est `icanhazstring/composer-unused` qui les détecte ; côté convention, FrameworkBundle (cf. C1) ne met en `require` que ce que son code ne peut pas ne pas charger. +- **Correctif** : retirer les deux lignes ; si KnpMenu redevient un jour typé dans le listener, le remettre en `suggest`, comme le fait déjà `sylius/admin-bundle` à la ligne 34 du même fichier. + +### C4 — Trois dépendances réellement utilisées ne sont déclarées qu'en transitif, en `require-dev` ou en `suggest` +- **Fichier** : `src/Bridge/Illuminate/composer.json:19-20` ; `src/DurablePlugin/composer.json:29-31` ; `src/DurableBundle/Testing/DurableBundleTestTrait.php:16` +- **Gravité** : mineur +- **Constat** : le pont Illuminate étend `Illuminate\Support\ServiceProvider` (`DurableIlluminateServiceProvider.php:7`) et utilise `Illuminate\Support\Facades\Schema` (`Migrations/0001_01_01_000000_create_durable_tables.php:7`), mais ne requiert que `illuminate/database` et `illuminate/contracts` — `illuminate/support` n'arrive que par le transitif. Le plugin instancie `Twig\Environment` en code de production avec `twig/twig` en `require-dev` seulement, transitif de `symfony/twig-bundle`. Le bundle utilise `PHPUnit\Framework\Assert` dans un trait publié en ne le déclarant qu'en `suggest`, sans aucune section `require-dev` : dans le dépôt satellite, ce fichier n'est analysable par personne. +- **Amont** : `symfony/symfony:.github/workflows/unit-tests.yml` — le mode *low-deps* résout chaque composant isolément contre des dépôts `path`, précisément pour que le transitif ne couvre plus la déclaration manquante. +- **Correctif** : déclarer `illuminate/support` et `twig/twig` en `require`, `phpunit/phpunit` en `require-dev` du bundle ; le job `composer-require-checker` de C1 les aurait tous les trois. + +### C5 — PHPStan au niveau 5 sur 10 et Psalm au niveau 4 : 67 diagnostics entre le niveau tenu et le niveau 8 +- **Fichier** : `phpstan.neon:5` (`level: 5`) ; `psalm.xml:3` (`errorLevel="4"`) ; `psalm.xml:38` +- **Gravité** : mineur +- **Constat** : mesuré, pas supposé — `phpstan analyse -c phpstan.neon --level=N` rend 0 erreur au niveau 5 configuré, 13 au niveau 6, 60 au niveau 7 et 67 au niveau 8, dont 23 dans `src/Durable` et 23 dans `src/Bridge` (chiffres corrigés des 2 `class.notFound` sur `Illuminate\Cache\{ArrayStore,NullStore}` dus à un `vendor/` local sans `illuminate/cache`, présent lui dans `composer.lock`). Côté Psalm, `errorLevel="4"` rétrograde tous les diagnostics `Possibly*` en information non bloquante et `PossiblyInvalidMethodCall` est en plus supprimé (`psalm.xml:38`) : aucun des deux outils ne bloque donc sur un déréférencement possiblement nul. Aucun bug avéré dans les 67 — les deux `method.nonObject` de `src/Durable/ChildWorkflowRunner.php:60-61` et le `throw.notThrowable` de `src/Durable/Awaitable/AwaitableAdapter.php:27` sont des invariants posés au constructeur, mais ce sont exactement ceux qu'un remaniement casse en silence, et le palier tractable est le 6 : 13 erreurs, toutes des types manquants sur l'API publique contre laquelle les consommateurs écrivent. +- **Amont** : niveaux PHPStan — 6 « report missing typehints », 8 « report calling methods and accessing properties on nullable types », maximum 10 depuis PHPStan 2.0 () ; niveaux Psalm — 1 le plus strict, 4 « demotes all Possibly* issues to non-blocking information » (). +- **Correctif** : passer `phpstan.neon` au niveau 6 tout de suite (13 erreurs, pas de baseline nécessaire), puis au 8 avec un `phpstan-baseline.neon` ou trois `assert()` narrant les invariants de `ChildWorkflowRunner` et `Deferred` ; abaisser `psalm.xml` à `errorLevel="3"` une fois la baseline de C2 régénérée. + +### C6 — Les deux analyseurs ne couvrent pas le même périmètre, et le CI annonce un périmètre qu'aucun des deux ne couvre +- **Fichier** : `.github/workflows/ci.yml:469` et `:472` ; `phpstan.neon:6-15` ; `psalm.xml:12-19` +- **Gravité** : mineur +- **Constat** : les deux étapes du job `static-analysis` s'intitulent « PHPStan (src/, tests/) » et « Psalm (src/, tests/) » alors que ni `phpstan.neon:6-15` ni `psalm.xml:12-19` ne listent `tests/` — aucun test du dépôt n'est analysé. Entre les deux outils, PHPStan couvre `src/DurablePhpstan` et `src/DurableRector` (lignes 14-15), Psalm non ; et `src/DurableDemoContracts` (6 fichiers, autochargé par la racine) n'est dans aucune des quatre configurations, Magento comprise. +- **Amont** : non sourcé — cohérence interne entre `ci.yml`, `phpstan.neon` et `psalm.xml`. +- **Correctif** : renommer les deux étapes en « (src/) », et soit aligner `psalm.xml` sur les neuf chemins de `phpstan.neon` plus `src/DurableDemoContracts`, soit y écrire pourquoi il n'en couvre que sept — le fichier documente déjà chacune de ses autres exclusions. + +### C7 — La ligne PHPUnit publiée par le cœur n'est plus corrigée en amont, et c'est celle que la matrice PHP 8.5 exerce +- **Fichier** : `src/Durable/composer.json:21` (`"phpunit/phpunit": "^11.0"`) ; `.github/workflows/ci.yml:16` +- **Gravité** : mineur +- **Constat** : le job `qa` tourne sur PHP 8.2 à 8.5 (ligne 16) contre `phpunit/phpunit 11.5.56` résolu par `composer.lock`, une ligne dont le support de correction est clos depuis le 2026-02-06 ; sa contrainte `php: >=8.2` étant ouverte, elle s'installe sur 8.5 sans que quiconque ait validé le couple. Le même `^11.0` est la contrainte `require-dev` du paquet publié `gplanchat/durable`, qui expédie des suites de conformité réutilisables (`Testing\EventStoreConformanceTestCase`) : elles ne sont donc jamais exercées contre PHPUnit 12 ou 13, que les consommateurs sur PHP 8.3+ utilisent déjà. +- **Amont** : — PHPUnit 11 « End of Bugfix Support: February 6, 2026 » ; PHPUnit 12 exige PHP ≥ 8.3, PHPUnit 13 PHP ≥ 8.4. +- **Correctif** : élargir la contrainte à `^11.5 || ^12.0 || ^13.0` dans les six `composer.json` qui la portent (racine, `Durable`, `DurablePhpstan`, `DurableRector` en `^11.0` ; `DurablePlugin`, `DurableModule` en `^11.5`), et ajouter à la matrice `qa` une entrée `{php: 8.4, phpunit: highest}` sur le modèle des matrices Symfony et Laravel déjà en place. diff --git a/documentation/audit/08-console-securite.md b/documentation/audit/08-console-securite.md new file mode 100644 index 00000000..7952f4ef --- /dev/null +++ b/documentation/audit/08-console-securite.md @@ -0,0 +1,65 @@ +# Composant Console et surface de sécurité — signature, sortie, codes de retour + +## Synthèse +Le périmètre console tient en deux commandes : `durable:execution:diagnose` (bundle Symfony) et `durable:worker` (module Magento) ; `bin/` ne contient que des scripts shell d'opérateur, pas de point d'entrée PHP. Les deux commandes sont correctement construites (injection par constructeur, `configure()` déclaratif, retours `Command::SUCCESS/FAILURE`), mais aucune ne traite sa sortie ni ses entrées selon les conventions du composant : la sortie JSON traverse le formateur et perd du contenu, l'erreur part sur stdout, et l'option `--role` lève une exception hors de la hiérarchie `Console\Exception`. La commande `durable:worker` est une boucle longue sans `SignalableCommandInterface` alors que son propre docblock la confie à systemd/supervisor. **Sur l'axe désérialisation, le périmètre est sain** : aucun `unserialize()`, aucun `eval()`, aucun `Closure::fromCallable()` dans `src/Durable`, `src/DurableBundle`, `src/Bridge/Dbal` ; `EventDataMapper::toDomainEvent()` est une liste blanche `match` fermée par un `default => throw`, et `WorkflowRegistry` ne résout qu'un type préenregistré — un journal falsifié ne peut donc pas instancier une classe arbitraire. Le dashboard Sylius est protégé par `#[IsGranted]` **et** par l'`access_control` Sylius, mais son chemin est écrit en dur. + +## Constats + +### C1 — La sortie `--json` traverse le formateur et perd du contenu +- **Fichier** : `src/DurableBundle/Command/DiagnoseExecutionCommand.php:88` +- **Gravité** : majeur +- **Constat** : `$output->writeln(json_encode($payload, ...))` écrit en mode `OUTPUT_NORMAL`, donc `OutputFormatter::format()` consomme toute balise reconnue présente dans les données. Reproduit sur `vendor/symfony/console` du dépôt : l'entrée `{"note":"client secret y"}` ressort `{"note":"client secret<\/error> y<\/> "}` — le contenu d'une charge utile d'activité est silencieusement amputé, et décoration activée ce sont des séquences ANSI qui sont injectées dans les valeurs JSON. Le même défaut touche la sortie humaine ligne 129-135, qui passe `truncateJson()` dans `$io->writeln(sprintf(...))`. +- **Amont** : `vendor/symfony/console/Descriptor/Descriptor.php:47` — les descripteurs JSON/XML de Symfony écrivent leur contenu avec `OutputInterface::OUTPUT_RAW` (`vendor/symfony/console/Output/OutputInterface.php:33`) précisément pour cela ; `OutputFormatter::escape()` (`vendor/symfony/console/Formatter/OutputFormatter.php:41`) couvre le cas de la sortie humaine. +- **Correctif** : passer `OutputInterface::OUTPUT_RAW` en troisième argument de `write()` pour la branche `--json`, et envelopper les extraits de charge utile dans `OutputFormatter::escape()` pour la branche humaine. + +### C2 — Le worker est une boucle longue sans gestion de signal +- **Fichier** : `src/DurableModule/Console/Command/RunWorkerCommand.php:114-120` (classe déclarée ligne 40) +- **Gravité** : majeur +- **Constat** : la boucle ne sort que par `--max-tasks` ou `--time-limit` ; la classe n'implémente pas `SignalableCommandInterface`, donc un `SIGTERM` de superviseur tue le processus au milieu d'un `$tick()` — exactement le scénario que le docblock de la classe revendique (« un superviseur redémarre »). Sans `--max-tasks` ni `--time-limit`, la boucle est infinie et n'a aucun point d'arrêt propre. +- **Amont** : `vendor/symfony/messenger/Command/ConsumeMessagesCommand.php:43` (`implements SignalableCommandInterface`), `:317` `getSubscribedSignals()`, `:322` `handleSignal()` — le worker de référence de l'écosystème finit son message courant puis s'arrête ; interface fournie par `vendor/symfony/console/Command/SignalableCommandInterface.php`. +- **Correctif** : implémenter `SignalableCommandInterface` en s'abonnant à `SIGTERM`/`SIGINT`, poser un drapeau d'arrêt lu en tête de boucle, et retourner `Command::SUCCESS` après le tour en cours. + +### C3 — Le chemin du dashboard écrit en dur ignore le préfixe admin configurable de Sylius +- **Fichier** : `src/DurablePlugin/Resources/config/routes.yaml:2` (`path: /admin/durable/dashboard`) +- **Gravité** : mineur +- **Constat** : Sylius rend son préfixe d'administration configurable par variable d'environnement ; la route du greffon le fige à `/admin`. Sur une boutique dont le préfixe a été changé, la page sort du pare-feu `admin` et du `access_control` correspondant, retombe dans le pare-feu `shop` (contexte de session différent) et devient inaccessible à un administrateur pourtant authentifié. Aucune fuite : `#[IsGranted('ROLE_ADMINISTRATION_ACCESS')]` (`src/DurablePlugin/Controller/AdminDashboardController.php:23`) refuse quand même. +- **Amont** : `sylius/vendor/sylius/sylius/src/Sylius/Bundle/AdminBundle/Resources/config/app/config.yml:9-10` — `sylius_admin.path_name: '%env(resolve:SYLIUS_ADMIN_ROUTING_PATH_NAME)%'` puis `sylius.security.admin_regex: "^/%sylius_admin.path_name%"`, référencé par `sylius/config/packages/security.yaml:19` et `:120`. +- **Correctif** : préfixer la route par `%sylius_admin.path_name%` (ou documenter le greffon comme à importer sous le préfixe admin de l'application) plutôt que par `/admin` littéral. + +### C4 — Validation d'option faite dans `execute()` et exception hors hiérarchie Console +- **Fichier** : `src/DurableModule/Console/Command/RunWorkerCommand.php:95` +- **Gravité** : mineur +- **Constat** : un `--role` inconnu lève un `\InvalidArgumentException` global depuis `execute()`. Le composant dispose d'une hiérarchie dédiée dont l'application sait qu'elle signale une erreur d'usage et non un plantage applicatif ; ici l'exploitant reçoit une trace d'exception applicative pour une faute de frappe. Le `default` du `match` est de plus atteint après que la commande a déjà été acceptée par le `InputDefinition`. +- **Amont** : `vendor/symfony/console/Exception/InvalidOptionException.php` et `vendor/symfony/console/Exception/ExceptionInterface.php` — les erreurs de valeur d'option du composant passent toutes par là (cf. `Input::validate()`). +- **Correctif** : lever `Symfony\Component\Console\Exception\InvalidOptionException` et déplacer le contrôle dans `initialize()`, avant tout effet de bord. + +### C5 — L'erreur part sur stdout, et un identifiant inconnu sort en code 0 +- **Fichier** : `src/DurableBundle/Command/DiagnoseExecutionCommand.php:45` (et `:97-98`, `:141`) +- **Gravité** : mineur +- **Constat** : le refus d'un `executionId` vide est écrit avec `$output->writeln()`, donc sur stdout — le canal que `--json` réserve aux données. Par ailleurs un identifiant totalement inconnu (aucune métadonnée, flux vide) produit un `warning` et retourne `Command::SUCCESS`, ce qui rend la commande inutilisable comme sonde dans un script. +- **Amont** : `vendor/symfony/console/Style/SymfonyStyle.php:360` — `getErrorStyle()` existe pour router les messages d'erreur vers `ConsoleOutputInterface::getErrorOutput()`. +- **Correctif** : écrire les erreurs via `$io->getErrorStyle()`, et retourner `Command::FAILURE` quand ni métadonnées ni événements n'existent pour l'identifiant demandé. + +### C6 — `--limit` accepte n'importe quelle valeur sans le dire +- **Fichier** : `src/DurableBundle/Command/DiagnoseExecutionCommand.php:50` +- **Gravité** : mineur +- **Constat** : `max(0, (int) $input->getOption('limit'))` transforme `--limit=abc` et `--limit=-5` en `0`. La commande s'exécute alors sans échantillon d'événements et sans signaler que l'option a été ignorée — un diagnostic silencieusement amputé est pire qu'un refus. +- **Amont** : non sourcé (aucune règle amont explicite ; `messenger:consume --limit` ne valide pas davantage sa valeur). +- **Correctif** : refuser une valeur non numérique ou négative avec `InvalidOptionException`, en rappelant la valeur reçue. + +### C7 — TLS désactivé par défaut sur la connexion gRPC, sans mTLS possible +- **Fichier** : `src/Bridge/Temporal/WorkflowServiceClientFactory.php:23-27` +- **Gravité** : mineur +- **Constat** : le canal est construit avec `ChannelCredentials::createInsecure()` sauf si le DSN porte `?tls=1` (`src/Bridge/Temporal/TemporalConnection.php:108`), et `createSsl()` est appelé sans argument — aucune façon de présenter un certificat client, une CA ou un `serverName`. Une grappe distante ne peut donc être jointe qu'en clair ou en TLS simple. À noter en sens inverse : le DSN est marqué `#[\SensitiveParameter]` (`TemporalConnection.php:92`), ce qui l'exclut des traces. +- **Amont** : https://docs.temporal.io/self-hosted-guide/security — « Temporal supports Mutual Transport Layer Security (mTLS) […] between application processes and a Temporal Service » ; le `serverName` y est recommandé contre l'usurpation. +- **Correctif** : accepter des paramètres de DSN pour la CA, le certificat et la clé client, et les passer à `ChannelCredentials::createSsl($rootCerts, $privateKey, $certChain)`. + +### C8 — Les charges utiles d'activité sont recopiées brutes dans le profiler et dans le diagnostic +- **Fichier** : `src/DurableBundle/DataCollector/DurableDataCollector.php:643` et `src/DurableBundle/Command/DiagnoseExecutionCommand.php:69` +- **Gravité** : remarque +- **Constat** : `'payload' => $event->payload()` verse la charge utile complète d'un événement dans le profil sérialisé, et `--json` la reverse intégralement sur stdout. Rien n'expurge un jeton, un mot de passe ou une donnée personnelle qu'une activité aurait reçu en argument ; le profil persiste sur disque dans `var/cache`. Aucun `Logger` n'est injecté dans le cœur ni dans le bundle, le journal applicatif est donc hors de cause. +- **Amont** : non sourcé (Symfony n'impose pas d'expurgation dans les collecteurs ; le précédent le plus proche est `#[\SensitiveParameter]`, déjà employé ailleurs dans le dépôt). +- **Correctif** : offrir une liste de clés à masquer (configuration du bundle) appliquée dans `summarizePayload()` et dans l'échantillon du diagnostic, ou n'exposer la charge utile complète que sous `-vv`. + +## Note de couverture +Aucun test du greffon Sylius n'exerce le contrôle d'accès : `src/DurablePlugin/tests/` ne contient aucune assertion sur `IsGranted`, un 403 ou un rôle. La suppression accidentelle de l'attribut ligne 23 du contrôleur ne serait rattrapée par rien. diff --git a/documentation/audit/09-routing-http.md b/documentation/audit/09-routing-http.md new file mode 100644 index 00000000..78ebb8d1 --- /dev/null +++ b/documentation/audit/09-routing-http.md @@ -0,0 +1,113 @@ +# Routing, HTTP, HttpKernel — routes, listeners kernel, sémantique des réponses + +## Synthèse + +Le périmètre HTTP est petit — une route de plugin, un listener kernel, cinq routes de bancs +d'essai — et trois des cinq questions posées s'y répondent par « sain » : les `requirements` ne +manquent nulle part de façon dangereuse (le seul paramètre, `{id}` en +`symfony/src/Controller/SamplesWorkflowController.php:33`, retombe sur le `[^/]+` par défaut de +Symfony et `SampleWorkflowCatalog::findById()` répond 404 sur inconnu) ; le seul listener kernel +du dépôt filtre bien `isMainRequest()` et se place à 1024, au-dessus de tous les écouteurs +`kernel.request` du cœur (le plus haut, `ValidateRequestListener`, est à 256) ; et le plugin +n'impose aucune route inconditionnelle — `routes.yaml` n'est chargé que si l'application l'importe +(`sylius/config/routes/durable_plugin.yaml`), `sylius/admin-bundle` reste en `suggest` tandis que +`symfony/security-bundle` est une dépendance dure, donc `#[IsGranted]` ne peut pas devenir un +no-op silencieux. Le recensement des listeners est clos : deux tags seulement dans tout `src/`, +dont un seul sur un événement kernel. + +Les deux questions restantes tombent mal. Le cycle de vie de la trace d'exécution est borné par la +requête HTTP **et par elle seule**, alors que le produit tourne principalement dans un +`messenger:consume` sans requête : l'état fuit d'un message à l'autre, sans borne. Et la route +admin du plugin fige `/admin/` là où tout Sylius — cœur et plugins amont — passe par +`%sylius_admin.path_name%`, ce qui décroche la page du pare-feu admin dès que ce nom change. + +## Constats + +### C1 — La trace d'exécution n'est remise à zéro que par une requête HTTP, jamais dans le worker + +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:744` (et `:479`, `:550`, `:716`) ; `src/DurableBundle/EventListener/ResetDurableProfilerListener.php:24` +- **Gravité** : bloquant +- **Constat** : `durable.execution_trace` est enregistré sans tag `kernel.reset`, et son unique + remise à zéro est le listener `KernelEvents::REQUEST`. Or l'alias + `WorkflowExecutionObserverInterface` est injecté dans `ExecutionRuntime` (:479), + `ExecutionEngine` (:550) et `ActivityMessageProcessor` (:716) — exactement les services qui + tournent dans `messenger:consume`, où aucun `kernel.request` n'a jamais lieu. Chaque run et + chaque activité empile une entrée dans `DurableExecutionTrace::$timeline` (payload compris) pour + toute la vie du process. +- **Amont** : `symfony/vendor/symfony/http-kernel/DependencyInjection/ResettableServicePass.php:34` + (seuls les services taggés `kernel.reset` entrent dans `services_resetter`) et + `symfony/vendor/symfony/messenger/EventListener/ResetServicesListener.php:33` + (`WorkerRunningEvent` → `servicesResetter->reset()` entre deux messages). `Profiler::reset()` + (`symfony/vendor/symfony/http-kernel/Profiler/Profiler.php:167`) ne rattrape rien : il appelle + `DurableDataCollector::reset()`, qui ne vide que `$this->data`, pas la trace — et le profiler + est absent en production. +- **Correctif** : ajouter `->addTag('kernel.reset', ['method' => 'reset'])` sur + `durable.execution_trace` (la méthode `reset()` existe déjà) ; le listener `kernel.request` + devient alors redondant et peut disparaître. + +### C2 — La route admin du plugin fige `/admin/` au lieu de `%sylius_admin.path_name%` + +- **Fichier** : `src/DurablePlugin/Resources/config/routes.yaml:2` ; `sylius/config/routes/durable_plugin.yaml:1` +- **Gravité** : majeur +- **Constat** : le chemin est écrit en dur `/admin/durable/dashboard`, et l'import applicatif ne + pose aucun `prefix`. Le nom de chemin admin de Sylius est un paramètre piloté par + l'environnement, `admin` n'étant que sa valeur par défaut ; le pare-feu `admin` et la règle + `access_control` en `ROLE_ADMINISTRATION_ACCESS` se calent tous deux sur + `%sylius.security.admin_regex%`, dérivé du même paramètre. Avec + `SYLIUS_ADMIN_ROUTING_PATH_NAME=backoffice`, la page reste à `/admin/…`, donc hors du pare-feu + admin et hors de l'`access_control` admin ; le `shop_regex` + (`^/(?!%sylius_admin.path_name%|api/.*|api$|media/.*)[^/]++`) la capture alors, et elle est + servie sous le pare-feu boutique avec le provider client. Seul `#[IsGranted]` sur le contrôleur + fait encore barrage. +- **Amont** : `sylius/vendor/sylius/sylius/src/Sylius/Bundle/AdminBundle/Resources/config/app/config.yml:8-10` + (`env(SYLIUS_ADMIN_ROUTING_PATH_NAME): admin`, `sylius.security.admin_regex: "^/%sylius_admin.path_name%"`) ; + `sylius/vendor/sylius/sylius/src/Sylius/Bundle/ShopBundle/Resources/config/app/config.yml:13`. + Convention amont côté plugins : `sylius/vendor/sylius/refund-plugin/config/routes.yaml:1-3` et + `sylius/vendor/sylius/adyen-plugin/config/routes.yaml:8-10` importent tous deux un + `config/routes/admin.yaml` sous `prefix: '/%sylius_admin.path_name%'`. +- **Correctif** : réduire le chemin à `/durable/dashboard` dans le fichier du plugin, et importer + ce fichier avec `prefix: '/%sylius_admin.path_name%'` (idéalement en le scindant en + `Resources/config/routes/admin.yaml`, comme les plugins amont). Le nom de route + `gplanchat_durable_plugin_admin_dashboard` est correct et sans risque de collision, il ne bouge + pas. + +### C3 — Un GET démarre un workflow + +- **Fichier** : `symfony/src/Controller/SamplesWorkflowController.php:33` +- **Gravité** : majeur +- **Constat** : `durable_samples_run` est déclarée `methods: ['GET']` mais son corps démarre une + exécution — `dispatchWorkflowRun()` (:83) ou `runAndSettle()` (:72). Une méthode déclarée sûre + produit ici un effet de bord persistant : un préchargement de lien, un crawler, un scanner ou un + simple rechargement de page déclenchent des runs. La banc d'essai étant l'exemple de référence + que les intégrateurs recopient, le motif se propage. +- **Amont** : RFC 9110 §9.2.1 (méthodes sûres : GET/HEAD ne doivent pas être perçues comme + demandant une action de modification d'état). +- **Correctif** : passer la route en `methods: ['POST']` avec un formulaire à jeton CSRF côté + gabarit, et répondre par une redirection 303 vers une route GET de résultat (PRG). + +### C4 — Le workflow s'exécute de bout en bout à l'intérieur de la requête HTTP + +- **Fichier** : `symfony/src/Controller/SamplesWorkflowController.php:49-72` +- **Gravité** : majeur +- **Constat** : `$waitForResult` vaut `true` par défaut (le paramètre `wait` doit être passé + explicitement pour l'inverser), et la branche par défaut appelle `runAndSettle()` : le run + complet, activités comprises, se déroule dans le process web pendant que la requête est tenue + ouverte. Le cycle de vie du workflow n'est donc pas borné par la requête, il est *confondu* avec + elle — un `max_execution_time` ou un timeout de reverse-proxy coupe alors le run là où il en est. + C'est l'inverse de la promesse du produit, sur la page qui sert à le démontrer. +- **Amont** : non sourcé (argument de conception, pas de règle amont). +- **Correctif** : inverser la valeur par défaut (`dispatchWorkflowRun()` par défaut, `wait=1` en + option de démonstration explicite) et rediriger vers une page d'état qui interroge le journal. + +### C5 — Le `catch \Throwable` court-circuite `kernel.exception` + +- **Fichier** : `symfony/src/Controller/SamplesWorkflowController.php:84-88` +- **Gravité** : mineur +- **Constat** : toute exception est attrapée dans le contrôleur et convertie en un rendu Twig avec + `new Response('', Response::HTTP_INTERNAL_SERVER_ERROR)`. Aucun `KernelEvents::EXCEPTION` n'est + donc dispatché : ni le `ErrorListener` du HttpKernel, ni le logger, ni le panneau « Exception » + du profiler ne voient jamais l'échec, et `$e->getMessage()` part dans la page rendue. +- **Amont** : non sourcé (comportement de `symfony/http-kernel` : `kernel.exception` n'est émis que + pour les exceptions qui remontent hors du contrôleur). +- **Correctif** : laisser remonter l'exception (ou la ré-emballer en `HttpException`) et confier la + page d'erreur au mécanisme d'erreur du framework. diff --git a/documentation/audit/10-api-publique.md b/documentation/audit/10-api-publique.md new file mode 100644 index 00000000..1cbbdee4 --- /dev/null +++ b/documentation/audit/10-api-publique.md @@ -0,0 +1,179 @@ +# Conception d'API publique — immutabilité, nommage, objets valeur, invariants + +## Synthèse + +Sur l'immutabilité, le périmètre est irréprochable et se mesure : aucun setter, aucun `with*()`, aucune +propriété publique mutable dans les huit dossiers, **aucune classe non `final`**, les 29 événements sont +`final readonly`, et les awaitables ne sont mutables que là où la sémantique de promesse l'exige. Mais +la seconde moitié de la question — « avec leurs invariants validés à la construction » — reçoit une +réponse inverse : un seul `throw` de validation existe sur une soixantaine de types constructibles. Le +même écart se rejoue sur les objets valeur : `ExecutionId` existe, est correct, et n'apparaît en +type-hint nulle part face à 152 signatures `string $executionId` — et sur `WorkflowHistorySourceInterface` +une tâche de migration cochée `[x]` en août décrit un retour `Duration` que le code ne rend pas. La +hiérarchie d'exceptions est plate : 16 classes, aucune interface marqueur, donc aucun `catch` par paquet. +Le nommage est cohérent à deux contre un, et rien du périmètre n'est marqué `@internal`. + +## Constats + +### C1 — Les invariants ne sont pas validés à la construction : un seul `throw` sur ~60 types + +- **Fichier** : `src/Durable/Awaitable/QuorumAwaitable.php:37` (le seul), `src/Durable/Attribute/AsActivity.php:10`, + `src/Durable/Event/ExecutionStarted.php:11`, `src/Durable/Failure/FailureEnvelope.php:14` +- **Gravité** : majeur +- **Constat** : `grep -rn 'throw new \\?Invalid|InvalidArgument|LogicException|Assert::'` sur les huit + dossiers ne retourne **qu'une** occurrence, `QuorumAwaitable`, dont le constructeur refuse un quorum + inatteignable avec un message qui nomme la panne évitée. Partout ailleurs les constructeurs acceptent + tout : `new AsActivity('')`, `new ExecutionStarted('')`, `new FailureEnvelope('', '')`, + `new AsSignalMethod('')` construisent sans broncher. Le `readonly` gèle donc un état qui n'a jamais + été vérifié — l'objet est immuable, pas valide, et l'erreur ressort ailleurs, plus tard. +- **Amont** : `webmozart/assert`, README — « efficient assertions to test the input and output of your + methods », avec l'exemple canonique d'un `Assert::greaterThan()` **dans le constructeur** + (https://github.com/webmozarts/assert#readme) ; c'est exactement la forme de `QuorumAwaitable:37`. +- **Correctif** : garder au moins l'identifiant et le nom non vides sur les types de frontière + (attributs, événements, `FailureEnvelope`, `ExecutionId`) — `QuorumAwaitable` prouve que le projet sait + écrire le garde, il n'est simplement écrit qu'une fois. + +### C2 — `ExecutionId` est un objet valeur mort : les ports transportent des `string` + +- **Fichier** : `src/Durable/Port/WorkflowResumeDispatcher.php:18`, `src/Durable/Event/Event.php:9`, + `src/Durable/ExecutionId.php:18` +- **Gravité** : majeur +- **Constat** : comptage sur `src/Durable` — `152` occurrences de `string $executionId`, `0` occurrence + de `ExecutionId` en type de paramètre ou de retour, et un seul appel dans tout `src/` + (`Handler/ResumeWorkflowHandler.php:90`), immédiatement déballé par `->toString()`. Le VO ne protège + donc rien : `dispatchResume($workflowType)` compile, et `ExecutionId::fromString('')` est accepté. + Aucun change en cours dans `openspec/changes/` ne retype cet identifiant — ce n'est pas une migration + à mi-parcours. +- **Amont** : l'ADR du projet, `documentation/adr/DUR031-value-objects-across-ports-and-wire-ownership.md` + (« Value objects cross the ports … Invariants did not travel ») ; côté amont, + `Symfony\Component\Uid\Uuid::fromString()` lève `InvalidArgumentException` sur une valeur non conforme + (https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Uid/Uuid.php). +- **Correctif** : soit valider le format dans `fromString()` et retyper les ports + (`WorkflowResumeDispatcher`, `WorkflowTimerDispatcher`, `WorkflowLifecycleInterface`, + `Event::executionId()`), soit supprimer `ExecutionId` — un VO qu'aucune signature n'exige coûte de la + lecture sans rendre de garantie. + +### C3 — `WorkflowHistorySourceInterface` : la migration vers les VO est cochée, le code n'a pas bougé + +- **Fichier** : `src/Durable/Port/WorkflowHistorySourceInterface.php:10` (la promesse), `:23`, `:58`, + `:74`, `:91`, `:128` (les retours réels) +- **Gravité** : majeur +- **Constat** : la docstring de classe annonce « Recorded timings are returned as `Duration` … + Third-party implementations written against the previous `float` return must adapt. See ADR DUR031 », + et `openspec/changes/archive/2026-08-26-value-objects-through-ports/tasks.md:11` coche `[x] 2.4 + WorkflowHistorySourceInterface::findTimerSlotResult() returns a Duration`. Le code rend toujours + `array{id: string, scheduledAt: float, failed: \Throwable|null}`, plus quatre autres tableaux de forme + (`{result, failed}` ×2, `{childExecutionId, result, failed}`, `{position, kind, name, payload}`). Le + côté écriture du même port documente pourtant sa propre migration réussie + (`WorkflowCommandBufferInterface:27-29`) : seule la moitié lecture est restée en arrière, avec une + docstring qui affirme le contraire. +- **Amont** : ADR DUR031, section « Value objects cross the ports », qui nomme explicitement les **deux** + interfaces et annonce le BC break aux implémenteurs tiers ; change archivé du 2026-08-26, tâche 2.4. +- **Correctif** : introduire des **DTO de résultat côté lecture** (`TimerSlotResult`, + `ActivitySlotResult`, `RecordedMessage`) — l'alternative que l'ADR rejette est « a command DTO per port + method », côté écriture, ce qui ne couvre pas ce cas ; à défaut, décocher la tâche et corriger la + docstring, qui aujourd'hui ment sur le contrat. + +### C4 — Aucune interface marqueur d'exception : on ne peut pas attraper « une erreur Durable » + +- **Fichier** : `src/Durable/Exception/DurableActivityFailedException.php:11` (et les 15 autres) +- **Gravité** : majeur +- **Constat** : les 16 exceptions du paquet étendent directement `\RuntimeException` (14) ou `\Exception` + (2) ; aucune `Gplanchat\Durable\Exception\ExceptionInterface` n'existe. La seule interface d'exception + du projet, `Port\DeclaredActivityFailureInterface extends \Throwable`, sert un autre rôle — elle décrit + une panne **métier** qu'une activité déclare pour la rejouer, pas les pannes du moteur. Un intégrateur + doit donc énumérer 16 classes pour distinguer une panne Durable d'un `\RuntimeException` de son propre + code, et l'énumération se périme au premier ajout. +- **Amont** : convention de composant Symfony — chaque composant publie une interface marqueur vide + étendant `\Throwable`, p. ex. `Messenger/Exception/ExceptionInterface.php` + (https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Messenger/Exception/ExceptionInterface.php). +- **Correctif** : ajouter `Exception\ExceptionInterface extends \Throwable` et la faire implémenter par + les 16 classes (non cassant : on n'ajoute qu'un parent), avec une seconde interface pour séparer les + exceptions de flux (`ContinueAsNewRequested`, `ChildWorkflowStartDeferred`) des vraies pannes. + +### C5 — `FailureEnvelope` duck-type `toHistoryPayload()` alors qu'une interface existe deux lignes plus haut + +- **Fichier** : `src/Durable/Failure/FailureEnvelope.php:38` +- **Gravité** : majeur +- **Constat** : `fromThrowable()` teste d'abord `$e instanceof DeclaredActivityFailureInterface`, puis + retombe sur `method_exists($e, 'toHistoryPayload')`. Ce second point d'extension n'a **aucun + implémenteur** dans le dépôt (`grep -rn toHistoryPayload src/` → 2 occurrences, toutes deux dans ce + fichier), aucune interface, aucune documentation. C'est un contrat public invisible : personne ne peut + le découvrir, et personne ne peut le retirer sans risquer de casser un tiers qui l'aurait deviné. +- **Amont** : promesse de rétrocompatibilité Symfony — ce qui n'est ni publié comme interface ni marqué + `@internal` est réputé public et figé (https://symfony.com/doc/current/contributing/code/bc.html) ; la + voie normale est une interface, ce que le projet fait déjà avec `DeclaredActivityFailureInterface`. +- **Correctif** : supprimer la branche `method_exists` (aucun appelant), ou la promouvoir en + `HistoryPayloadAwareInterface` documentée à côté de `DeclaredActivityFailureInterface`. + +### C6 — Nommage : suffixe `Interface` appliqué à 2 contre 1, deux exceptions homonymes, un `@template` inerte + +- **Fichier** : `src/Durable/Port/WorkflowResumeDispatcher.php:12`, + `src/Durable/Port/WorkflowTimerDispatcher.php:22`, + `src/Durable/Exception/WorkflowCancelledException.php:13`, + `src/Durable/Exception/WorkflowCancelledFailure.php:19`, `src/Durable/Awaitable/Awaitable.php:21` +- **Gravité** : mineur +- **Constat** : la prémisse de la question posée est fausse — `WorkflowResumeDispatcher` et + `WorkflowTimerDispatcher` **sont** des interfaces (`interface` en ligne 12 et 22), dont les + implémentations no-op `NullWorkflowResumeDispatcher` / `NullWorkflowTimerDispatcher` cohabitent dans le + même dossier ; le seul écart est le suffixe, appliqué à 18 interfaces du cœur contre 9 sans, dont ces + deux-là sur les 10 de `Port/`. Deux autres frottements de nommage : `WorkflowCancelledException` et + `WorkflowCancelledFailure` ne diffèrent que par le suffixe, portent les deux mêmes propriétés + (`$executionId`, `$reason`) et désignent des choses opposées — terminaison côté moteur contre signal + levé **dans le fiber** au point d'attente ; et `Awaitable` déclare `@template TValue` sans que + `getResult(): mixed` porte `@return TValue`, ce qui rend le générique inerte chez l'appelant. +- **Amont** : standards de code Symfony, « Suffix interfaces with `Interface` » + (https://symfony.com/doc/current/contributing/code/standards.html). Le suffixe `Failure` des exceptions + Temporal (`WorkflowCancelledFailure`, « Équivalent du `CanceledFailure` Temporal ») est un choix + d'interopérabilité assumé, pas un écart — ce n'est pas lui qui pose problème, c'est la collision. +- **Correctif** : renommer `WorkflowCancelledFailure` en `WorkflowCancellationRequestedFailure` (ce que + dit déjà son propre message), aligner les deux ports en `*Interface`, annoter `@return TValue` — les + deux renommages passant par une règle Rector, selon la règle projet « tout BC a sa procédure de + migration ». + +### C7 — `UuidGeneratorInterface` promet un identifiant monotone que l'implémentation ne produit pas + +- **Fichier** : `src/Durable/Uuid/UuidGeneratorInterface.php:16`, + `src/Durable/Uuid/NativeUuidV7Generator.php:19-28` +- **Gravité** : mineur +- **Constat** : la docstring du port dit « UUID v7 or equivalent **monotonic** UUID ». + `NativeUuidV7Generator` remplit `rand_a` (12 bits) avec `random_bytes()` pur, sans compteur ni + sous-milliseconde : deux identifiants générés dans la même milliseconde s'ordonnent au hasard. Le + contrat du port promet donc une garantie que sa seule implémentation ne tient pas. +- **Amont** : `Symfony\Component\Uid\UuidV7` incrémente `rand_a` d'un delta 24 bits dérivé d'un hachage + quand l'horodatage n'a pas changé — « Within the same ms, we increment the rand part by a random + 24-bit number » (https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Uid/UuidV7.php) ; + c'est cette mécanique supplémentaire qui rend un v7 monotone, elle est absente ici. +- **Correctif** : soit implémenter le compteur monotone sur `rand_a`, soit retirer le mot « monotonic » + de la docstring du port — un contrat qu'on ne tient pas est pire qu'un contrat absent. + +### C8 — Zéro `@internal` sur tout le périmètre : la surface publique est plus large que voulu + +- **Fichier** : `src/Durable/Awaitable/AwaitableInspector.php:10`, + `src/Durable/Awaitable/AwaitableCancellation.php:18`, + `src/Durable/Failure/ActivityFailureEventFactory.php:15` +- **Gravité** : mineur +- **Constat** : `grep -rl "@internal"` ne retourne **aucun** fichier dans les huit dossiers audités, et 6 + seulement dans tout `src/Durable` (207 fichiers). Or `AwaitableInspector` se décrit lui-même comme + « prédicats structurels … partagés par les points qui décident du réveil » — un détail + d'implémentation du moteur —, et `ActivityFailureEventFactory` fabrique des événements de journal que + personne hors du moteur n'a de raison de construire. Sans marquage, ces classes tombent sous la + garantie de rétrocompatibilité au même titre que `WorkflowEnvironment`. +- **Amont** : promesse de rétrocompatibilité Symfony — « code marked with the `@internal` tags are + excluded from our Backward Compatibility promise » + (https://symfony.com/doc/current/contributing/code/bc.html). +- **Correctif** : marquer `@internal` les utilitaires moteur (`AwaitableInspector`, + `AwaitableCancellation`, `ActivityFailureEventFactory`) **avant** le premier tag stable — après, c'est + un BC break qui demande sa propre procédure de migration. + +## Points sains, dits en une ligne chacun + +- **Mutabilité** : aucun setter ni `with*()` dans les huit dossiers, et aucune propriété publique + mutable — la question « readonly ou setters ? » est tranchée du bon côté. +- **`final`** : **aucune classe non `final`** dans le périmètre ; la moitié « quelles classes devraient + être `final` ? » de la question est déjà close, seule la moitié `@internal` (C8) reste ouverte. +- **Attributs** : les 12 sont `final` et immuables, à une hésitation de style près entre `final class` + + `public readonly $x` (7) et `final readonly class` + `public $x` (5), sans conséquence. +- **`array` dans les API** : le `array $payload` des activités et des signaux est une exception + **explicitement décidée** par l'ADR DUR031 (« It is caller data, genuinely untyped ») — ce n'est pas un + écart, et C3 ne vise que les enveloppes moteur. diff --git a/documentation/audit/11-vocabulaire-exploitation.md b/documentation/audit/11-vocabulaire-exploitation.md new file mode 100644 index 00000000..b6954e60 --- /dev/null +++ b/documentation/audit/11-vocabulaire-exploitation.md @@ -0,0 +1,49 @@ +# Vocabulaire du domaine — cohabitation Symfony, versionnage, ergonomie d'exploitation + +## Synthèse + +La cohabitation avec `symfony/workflow` est **saine et vérifiée** : racine de configuration `durable` (`Configuration.php:14`) contre `framework.workflows`, tag `durable.workflow` (`WorkflowPass.php:24`) contre `workflow` / `workflow.workflow`, identifiants de service en FQCN contre `workflow.NAME` / `state_machine.NAME`, et zéro collision de nom court (`Workflow`, `WorkflowInterface`, `Registry`, `Definition`, `Transition`, `Marking`, `StateMachine` : aucun fichier). La collision est purement lexicale et n'atteint ni le conteneur, ni l'autowiring, ni la configuration. Le vocabulaire aligné sur Temporal est globalement fidèle et les écarts assumés sont documentés à l'endroit où ils se produisent (`WorkflowRunDescription.php:14-17`, `WorkflowRunCatalogInterface.php:40`) — mais l'identité du *type* de workflow dérive sur quatre noms sans que rien ne le dise. Le mot « task » est sain sur tout le périmètre : il n'apparaît que dans `TaskQueue` et dans les événements `ActivityTask*`, aux deux sens que Temporal leur donne, jamais comme synonyme de « travail » en général. Sur l'axe Nexus, l'observation est exemplaire : `WorkflowRunEventKind::Nexus` existe précisément pour qu'un exploitant ne cherche pas la panne dans son propre système (`WorkflowRunEventKind.php:36-41`). Les deux faiblesses réelles sont ailleurs : la question « pourquoi celui-ci n'avance pas » a une réponse **calculée puis jetée** dans le chemin de production, et le versionnage en vol ne respecte pas la borne basse de son propre contrat. + +## Constats + +### C1 — `version()` accepte `$minSupported` et ne s'en sert jamais +- **Fichier** : `src/Durable/ExecutionContext.php:211-217` +- **Gravité** : majeur +- **Constat** : `version(string $changeId, int $minSupported, int $maxSupported)` retourne `$recorded` dès qu'un marqueur existe, sans jamais comparer à `$minSupported` ; le paramètre n'est lu nulle part dans le corps. Le geste que le versionnage sert à couvrir — retirer l'ancienne branche une fois les runs anciens éteints, en montant la borne basse — devient donc silencieux : un run en vol resté sur la version retirée reçoit son ancien numéro, le `if` correspondant n'existe plus dans le code, et l'exécution part sur la branche neuve. C'est exactement la divergence que `DUR042` interdit, obtenue sans aucun signal. +- **Amont** : https://docs.temporal.io/develop/go/versioning — « If an older version of the Workflow Execution history is replayed on this code, it fails because the minimum expected version is 1 » ; la borne basse est un contrat de rejet, pas une indication. +- **Correctif** : lever une exception dédiée quand `$recorded < $minSupported` (message nommant `$changeId`, la version enregistrée et la borne), plutôt que de rendre un numéro que le code appelant ne sait plus interpréter. + +### C2 — la raison de l'attente est calculée à chaque suspension, puis jetée dans le chemin distribué +- **Fichier** : `src/Durable/Handler/ResumeWorkflowHandler.php:70-72` +- **Gravité** : majeur +- **Constat** : `EventStoreWorkflowLifecycle.php:115` remplit `WorkflowSuspendedException::$waitingOn` avec `AwaitableInspector::describeCondition($pending)` — la condition nommée sur laquelle l'exécution bute. Le seul lecteur de `waitingOn()` est `InMemoryWorkflowRunner.php:96,115`, le runner en processus : le handler de reprise Messenger, qui est le chemin de production, ne lit que `waitingOnTimer()` et laisse tomber la chaîne. La réponse à « pourquoi celui-ci n'avance pas » est donc recalculée à chaque suspension et jetée aussitôt. +- **Amont** : `src/Bridge/Temporal/Api/Workflowservice/V1/DescribeWorkflowExecutionResponse.php:138,160,182,236` — le contrat Temporal expose `getPendingActivities()`, `getPendingChildren()`, `getPendingWorkflowTask()`, `getPendingNexusOperations()` pour cette question précise ; côté Durable rien d'équivalent n'est offert à l'exploitant. +- **Correctif** : ajouter une méthode `onWorkflowSuspended(string $executionId, ?string $waitingOn, bool $waitingOnTimer)` à `WorkflowExecutionObserverInterface` (`src/Durable/Debug/WorkflowExecutionObserverInterface.php:10-29`, qui n'a aujourd'hui que `onWorkflowRun` et `onActivityExecuted`) et l'appeler depuis `ResumeWorkflowHandler`. + +### C3 — un continue-as-new coupe la chaîne : métadonnées supprimées, nouvel id, aucun lien +- **Fichier** : `src/Durable/Handler/ResumeWorkflowHandler.php:89-93` +- **Gravité** : majeur +- **Constat** : la reprise supprime la ligne de métadonnées de l'ancien `executionId` (l. 89), génère un id neuf (l. 90) et le dispatche (l. 93) sans jamais enregistrer de lien entre les deux. `WorkflowContinuedAsNew` (`src/Durable/Event/WorkflowContinuedAsNew.php:19-24`) porte `nextWorkflowType`, `nextPayload`, `continuationMetadata` — mais pas de `nextExecutionId`, et il est de toute façon écrit avant que l'id neuf n'existe. Aucun champ `nextExecutionId` / `previousExecutionId` n'existant nulle part dans `src/Durable/` ni `src/Bridge/Dbal/`, un exploitant qui tient l'identifiant d'origine trouve `durable:execution:diagnose` sans métadonnées (« run inline sans dispatch, ou ID inconnu ») et n'a aucun moyen de suivre la continuation. +- **Amont** : https://docs.temporal.io/workflow-execution — le Continue-As-New forme une « Workflow Execution Chain » : nouveau Run Id, **Workflow Id conservé**, précisément pour que la chaîne reste parcourable. +- **Correctif** : ajouter `nextExecutionId` à `WorkflowContinuedAsNew` (id généré avant l'append) et l'exposer dans le diagnostic et dans `WorkflowRunDescription::$groupId` côté DBAL. + +### C4 — le diagnostic montre les *premiers* événements, alors que le blocage est au bout +- **Fichier** : `src/DurableBundle/Command/DiagnoseExecutionCommand.php:64` +- **Gravité** : mineur +- **Constat** : `if (\count($sample) < $limit)` retient les `--limit` premiers événements du flux, et la sortie l'annonce (« Premiers événements (max %d) », l. 127). Un workflow bloqué depuis des jours a un flux long dont seule la queue explique l'arrêt ; avec le défaut de 30, la commande affiche le démarrage et coupe exactement ce qu'on est venu voir. Elle n'affiche par ailleurs aucun minuteur en attente, alors que `TimerWakeDelayCalculator::millisecondsUntilNextTimerDue()` (`src/Durable/Timer/TimerWakeDelayCalculator.php:26`) calcule déjà « prochaine échéance » à partir du seul `EventStoreInterface` dont la commande dispose. +- **Amont** : non sourcé (jugement d'ergonomie d'exploitation). +- **Correctif** : garder une fenêtre glissante des `limit` derniers événements (ou une option `--tail`), et ajouter une section « en attente » alimentée par `TimerWakeDelayCalculator` et par les `NexusOperationScheduled` sans terminal. + +### C5 — quatre noms pour l'identité d'un type de workflow, sans note nulle part +- **Fichier** : `src/Durable/Workflow/WorkflowDefinitionLoader.php:53` +- **Gravité** : mineur +- **Constat** : le même concept circule sous `workflowType` (métadonnées, `ResumeWorkflowHandler.php:57`), `lookupKey` (variable locale, même ligne), `alias` (`aliasForTemporalInterop()`, dont le corps se contente d'appeler `workflowTypeForClass()`) et `workflowName` (`WorkflowRunDescription.php:22`). Contrairement au triplet `executionId` / `runId` / `groupId`, qui est un choix assumé et documenté à l'endroit du passage (`WorkflowRunDescription.php:14-17`, `WorkflowRunCatalogInterface.php:40`), cette dérive-là n'est expliquée nulle part et se lit comme quatre choses. +- **Amont** : https://docs.temporal.io/workflow-definition — « A Workflow Type is a name that maps to a Workflow Definition » : un seul terme amont pour ce que ces quatre noms désignent. Que deux d'entre eux portent déjà la même valeur se lit dans le dépôt même (`WorkflowDefinitionLoader.php:53-60`). +- **Correctif** : conserver `workflowType` dans le cœur et le pont, renommer `WorkflowRunDescription::$workflowName` en `$workflowType`, et supprimer `aliasForTemporalInterop()` au profit d'un appel direct puisqu'il ne fait rien d'autre. + +### C6 — l'outil de migration annonce que le versionnage n'existe pas, alors qu'il est livré +- **Fichier** : `src/DurableRector/Rector/UnmigratableTemporalCallRector.php:51` +- **Gravité** : mineur +- **Constat** : la table `REASONS` associe `getVersion` à « no equivalent yet — workflow versioning is an open change, and a run that reached this marker cannot migrate before it lands ». Or `WorkflowEnvironment::version()` (`src/Durable/WorkflowEnvironment.php:446`) expose la signature exacte de `Workflow::getVersion()`, `ExecutionContext::version()` l'implémente (l. 211), `Versioning/ChangePoint` fournit les constantes, et le cas est couvert par la suite de conformité (`EventStoreReplayConformanceTestCase.php:180`). Un utilisateur qui lance le Rector renonce à migrer pour une fonctionnalité présente. +- **Amont** : parité de signature avec `workflow.GetVersion(ctx, changeID, minSupported, maxSupported)` — https://docs.temporal.io/develop/go/versioning ; la correspondance existe donc bel et bien. +- **Correctif** : sortir `getVersion` de `REASONS` et l'ajouter à `REWRITABLE` (l. 46-49) avec la réserve de C1 tant que `$minSupported` n'est pas honoré. diff --git a/documentation/audit/12-interop-doctrine.md b/documentation/audit/12-interop-doctrine.md new file mode 100644 index 00000000..0728f7ba --- /dev/null +++ b/documentation/audit/12-interop-doctrine.md @@ -0,0 +1,77 @@ +# Interopérabilité entre bundles — points d'extension, Doctrine, ordre de chargement + +## Synthèse +Le pont DBAL prend bien une `Connection` **injectée** par identifiant de service configurable +(`durable.dbal.connection`, défaut `doctrine.dbal.default_connection`) : c'est la bonne moitié du +contrat, et elle place de fait le journal **dans** la transaction applicative ouverte — un fait +jamais énoncé ni dans DUR030 ni dans le README. L'autre moitié manque entièrement : les quatre +tables ne sont déclarées à Doctrine par **aucun** `configureSchema` ni écouteur `postGenerateSchema`, +alors que le docblock affirme le contraire, si bien que `doctrine:schema:update` et +`doctrine:migrations:diff` les voient comme des tables étrangères à supprimer. Côté conteneur, +l'ordre des passes est correct et vérifiable (les priorités 100/50/0 correspondent à +`PassConfig::$beforeOptimizationPasses`), mais le bundle référence en dur des classes de paquets +qu'il ne déclare ni en `require` ni en `suggest`, et un service `lock.factory` qui n'existe que si +`framework.lock` est configuré, sans garde ni message. Les points d'extension offerts à un tiers se +réduisent à l'écrasement d'alias : la liste des backends est un `enumNode` fermé. + +## Constats + +### C1 — Les tables du journal sont invisibles de l'outillage Doctrine, et le docblock prétend l'inverse +- **Fichier** : `src/Bridge/Dbal/Schema/DurableSchema.php:56` (et `:15`), `src/DurableBundle/DependencyInjection/DurableExtension.php:143` +- **Gravité** : bloquant +- **Constat** : le docblock de `addToSchema()` annonce « branché aussi sur `configureSchema` côté bundle » ; `grep -rn "configureSchema\|SchemaListener\|ToolEvents\|postGenerateSchema\|doctrine.event_listener" src/` ne renvoie que cette ligne de commentaire. Aucun service n'est tagué `doctrine.event_listener`, aucun `schema_filter` n'est prependé. Sur un banc comme `sylius/`, qui configure `doctrine_migrations` (`sylius/config/packages/doctrine_migrations.yaml:5`) et l'ORM (`sylius/config/packages/doctrine.yaml`), un `doctrine:migrations:diff` produira donc `DROP TABLE durable_events`, `durable_workflow_metadata`, `durable_workflow_runs`, `durable_child_workflow_parent_link` — le journal d'exécution effacé par une migration générée. +- **Amont** : `symfony/vendor/symfony/doctrine-messenger/Transport/DoctrineTransport.php:89` (`configureSchema(Schema, DbalConnection, \Closure $isSameDatabase)`) et `symfony/vendor/symfony/doctrine-messenger/Transport/Connection.php:357` ; le câblage est dans `symfony/vendor/doctrine/doctrine-bundle/config/messenger.php:56` — `doctrine.orm.messenger.doctrine_schema_listener` tagué `doctrine.event_listener` sur `postGenerateSchema` et `onSchemaCreateTable`. Même schéma pour `symfony/lock` : `symfony/vendor/doctrine/doctrine-bundle/config/orm.php:224` (`LockStoreSchemaListener`) et pour le cache PDO et le token provider remember-me. +- **Correctif** : enregistrer, sous garde `class_exists(Doctrine\ORM\Tools\ToolEvents::class)`, un listener tagué `doctrine.event_listener` sur `postGenerateSchema` qui appelle `DurableSchema::addToSchema()` en filtrant sur la connexion visée (motif `AbstractSchemaListener::getIsSameDatabaseChecker()`), et corriger le docblock `:56` tant que ce n'est pas fait. + +### C2 — Le DDL s'exécute paresseusement sur la connexion applicative, sans garde de transaction ni interrupteur +- **Fichier** : `src/Bridge/Dbal/Schema/DurableSchema.php:34-53` +- **Gravité** : majeur +- **Constat** : `ensure()` est appelé en tête de **chaque** méthode des stores (`DbalEventStore.php:37,60,81`, `DbalWorkflowRunProjection.php:38,75`, `DbalWorkflowRunCatalog.php:44`…) et émet `CREATE TABLE` sur la connexion injectée, quel que soit l'état transactionnel. Comme cette connexion est celle de l'application (`DurableExtension.php:141`), un premier `append()` déclenché à l'intérieur d'une transaction métier — cas normal sous `messenger.middleware.doctrine_transaction` ou dans une requête Sylius — provoque sur MySQL un commit implicite de la transaction en cours. Il n'existe par ailleurs aucun équivalent de `auto_setup: false` pour désactiver la création paresseuse en production. +- **Amont** : `symfony/vendor/symfony/doctrine-messenger/Transport/Connection.php:426` et `:441` — le transport que DUR030 (`documentation/adr/DUR030-…​.md:81-82`) et `DurableSchema.php:15` disent suivre refuse explicitement l'auto-setup dans ce cas : `if (!$this->autoSetup || $this->driverConnection->isTransactionActive()) { throw $e; }`. Le motif amont est de plus **réactif** (rattraper `TableNotFoundException`), pas proactif à chaque appel. +- **Correctif** : ajouter une option `auto_setup` (défaut `true`) et court-circuiter `ensure()` si `$connection->isTransactionActive()`, ou basculer sur le motif amont — laisser passer la requête et ne créer qu'après `TableNotFoundException`. + +### C3 — Le bundle référence des classes de paquets qu'il ne déclare pas +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:7-13`, `src/DurableBundle/composer.json` +- **Gravité** : majeur +- **Constat** : l'extension importe et enregistre `Gplanchat\Bridge\Dbal\{Schema\DurableSchema, Store\*, Messenger\SingleResumeLockMiddleware}` (lignes 7-13, enregistrés en 143-182), mais `src/DurableBundle/composer.json` ne mentionne ni `gplanchat/durable-bridge-dbal`, ni `doctrine/dbal`, ni `symfony/lock` — pas même en `suggest`, alors que trois autres entrées `suggest` existent. Aucune garde `class_exists()` n'accompagne `registerDbalStores()`. Poser `event_store.type: dbal` sans le paquet donne une erreur de réflexion sur une classe absente, loin de la ligne de configuration fautive. Le même reproche vaut pour les imports `Gplanchat\Bridge\Temporal\*` (lignes 14-29), hors périmètre ici. +- **Amont** : convention Symfony des bundles-ponts, appliquée par DoctrineBundle lui-même : `symfony/vendor/doctrine/doctrine-bundle/src/DoctrineBundle.php:80-85` teste `class_exists(RegisterDatePointTypePass::class)` avant d'enregistrer la passe qui en dépend, et `:60` teste `$container->hasExtension('security')` avant de toucher à l'extension voisine. +- **Correctif** : ajouter `gplanchat/durable-bridge-dbal` en `suggest` du bundle et faire échouer `registerDbalStores()` sur `class_exists(DurableSchema::class)` avec le message « installez `gplanchat/durable-bridge-dbal` pour `event_store.type: dbal` ». + +### C4 — `lock.factory` est référencé sans vérifier que `framework.lock` est configuré +- **Fichier** : `src/DurableBundle/DependencyInjection/DurableExtension.php:162-166`, `src/DurableBundle/DependencyInjection/Configuration.php:23` +- **Gravité** : majeur +- **Constat** : `SingleResumeLockMiddleware` reçoit `new Reference($config['dbal']['lock_factory'])`, défaut `lock.factory`. Ce service n'existe que si l'application a activé `framework.lock` — le banc `symfony/` n'a pas de `config/packages/lock.yaml`, seul `sylius/config/packages/lock.yaml` en pose un. Sans lui, `event_store.type: dbal` fait échouer la compilation sur un « service inexistant », alors que le verrou est décrit par DUR030 (`:65-76`) et le README comme la seule garde contre le rejeu concurrent. Aucun message n'oriente vers `framework.lock`. +- **Amont** : `symfony/vendor/doctrine/doctrine-bundle/src/DoctrineBundle.php:47-57` — DoctrineBundle supprime `doctrine.orm.listeners.pdo_session_handler_schema_listener` quand `session.handler` est absent plutôt que de laisser une référence pendante ; c'est la convention pour une dépendance optionnelle sur un service du FrameworkBundle. +- **Correctif** : garder l'enregistrement derrière `$container->has($config['dbal']['lock_factory'])` et lever une `InvalidConfigurationException` explicite (« `durable.event_store.type: dbal` requiert `framework.lock` — configurez un store partagé, pas un store par processus »). + +### C5 — Les middlewares Durable sont insérés en tête de *tous* les bus de l'application +- **Fichier** : `src/DurableBundle/DependencyInjection/Compiler/RegisterDurableMiddlewarePass.php:38-57` +- **Gravité** : mineur +- **Constat** : la passe itère `findTaggedServiceIds('messenger.bus')` et insère ses middlewares dans chaque `%busId%.middleware`, sans configuration pour restreindre la liste. Dans une application Sylius, cela injecte le verrou de reprise dans les bus de commande et d'événement de Sylius et de tout autre bundle. Le middleware s'esquive sur les messages non-Durable (`SingleResumeLockMiddleware.php:36-39`), donc l'effet est aujourd'hui bénin, mais un bundle tiers modifie ici la pile d'un bus qui ne lui appartient pas, sans possibilité pour l'intégrateur de s'y opposer. +- **Amont** : la configuration amont des piles est **par bus** — `framework.messenger.buses..middleware` (`symfony/vendor/symfony/framework-bundle/DependencyInjection/Configuration.php`, nœud `buses`) ; le commentaire de la passe (`:13-20`) constate à juste titre qu'aucune balise `messenger.middleware` n'existe en amont, mais en déduit un élargissement à tous les bus. +- **Correctif** : ajouter un nœud `durable.messenger.buses` (défaut : le ou les bus effectivement utilisés par Durable) et n'insérer que dans ceux-là. + +### C6 — `ensure()` marque le schéma « vérifié » avant d'exécuter le DDL +- **Fichier** : `src/Bridge/Dbal/Schema/DurableSchema.php:36-52` +- **Gravité** : mineur +- **Constat** : `$this->ensured = true;` est posé en ligne 39, avant la boucle `executeStatement()` des lignes 50-52. Si un `CREATE TABLE` échoue — droits insuffisants, course entre deux workers qui démarrent ensemble —, tous les appels suivants du processus considèrent le schéma prêt et échouent ensuite en `TableNotFoundException`, sans jamais retenter. Le cas de course est réel ici puisque la création est déclenchée par la première écriture de n'importe quel worker. +- **Amont** : `symfony/vendor/symfony/doctrine-messenger/Transport/Connection.php:426-448` — la garde amont est portée par le rattrapage de `TableNotFoundException` autour de chaque requête, ce qui rend l'échec de setup naturellement retentable. +- **Correctif** : déplacer l'affectation après la boucle, ou rattraper l'exception de création en revérifiant l'existence de la table (une course perdue n'est pas une erreur). + +### C7 — Un transport `temporal://` mal configuré casse `doctrine:schema:create`, et le contournement est dans la configuration du banc +- **Fichier** : `sylius/config/packages/durable.yaml:17-21`, `sylius/config/packages/messenger.yaml:25`, `src/Bridge/Temporal/Messenger/TemporalTransportFactory.php:42-44` +- **Gravité** : mineur +- **Constat** : `createTransport()` construit `TemporalConnection::fromDsn($dsn)` immédiatement et lève `InvalidArgumentException('Invalid temporal:// DSN…')` (`src/Bridge/Temporal/TemporalConnection.php:97`). Or l'écouteur de schéma de Doctrine reçoit un `tagged_iterator('messenger.receiver')` et doit **instancier** chaque transport pour tester son type : un transport `temporal://` dont l'env var est vide fait donc échouer `doctrine:schema:create`, avec un message qui ne parle ni de Doctrine ni de Messenger. La configuration du banc documente le contournement (ne déclarer le transport que dans les profils `demo*`) au lieu que le pont soit tolérant. +- **Amont** : `symfony/vendor/doctrine/doctrine-bundle/config/messenger.php:55-60` (`MessengerTransportDoctrineSchemaListener` construit sur `tagged_iterator('messenger.receiver')`) et `symfony/vendor/symfony/doctrine-bridge/SchemaListener/MessengerTransportDoctrineSchemaListener.php:34-47` (`foreach ($this->transports as $transport)`, puis `instanceof DoctrineTransport`). +- **Correctif** : différer la résolution du DSN dans les transports Temporal (construire la connexion au premier `get()`/`send()` plutôt qu'en fabrique), pour qu'un transport non utilisé soit instanciable sans DSN valide. + +### C8 — Points d'extension : liste de backends fermée et un nom de table non configurable +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:29`, `src/DurableBundle/DependencyInjection/DurableExtension.php:143-149` +- **Gravité** : mineur +- **Constat** : `enumNode('type')->values(['in_memory', 'dbal'])` ferme l'ensemble des backends : un bundle tiers qui voudrait fournir un `EventStoreInterface` (MongoDB, Elasticsearch…) ne peut pas s'annoncer par la configuration, il doit écraser l'alias `EventStoreInterface::class` dans une passe. Les services sont bien publics et aliasés sur l'interface, donc décorables, mais il n'existe **aucune** balise offerte à un tiers hormis `durable.activity_handler`, `durable.nexus_handler` et `durable.messenger.middleware` — rien côté stockage ni catalogue. Accessoirement, `durable.dbal.schema` est enregistré avec quatre arguments alors que `DurableSchema::__construct()` en accepte cinq : `durable_workflow_runs` est le seul nom de table qui ne peut pas être changé, ce que le README ne signale pas. +- **Amont** : non sourcé pour la fermeture de l'enum (choix de conception, pas de règle amont) ; pour la table non configurable, `symfony/vendor/symfony/doctrine-messenger/Transport/Connection.php:57` expose `table_name` comme toute autre option. +- **Correctif** : accepter en plus du couple `in_memory`/`dbal` un `service:` pointant un identifiant fourni par l'application, et passer le nom de la table `runs` comme les trois autres. + +--- + +**Sains sur leur axe** : l'ordre des passes de compilation est correct et documenté — `ActivityHandlerPass` et `NexusHandlerPass` à 50 passent bien après `AttributeAutoconfigurationPass` (priorité 100 dans `symfony/vendor/symfony/dependency-injection/Compiler/PassConfig.php:43-49`) et avant les passes à 0 ; `RegisterDurableMiddlewarePass` à 10 passe avant `MessengerPass` (priorité 0) qui lit `%busId%.middleware` ; `DurableTemporalTransportFactoryPass` en `TYPE_BEFORE_REMOVING` passe après l'autowiring. L'exclusion mutuelle `event_store.type: dbal` / `temporal.dsn` (`DurableExtension.php:137-139`) est vérifiée à la compilation avec un message actionnable — c'est le bon endroit. diff --git a/documentation/audit/13-documentation.md b/documentation/audit/13-documentation.md new file mode 100644 index 00000000..4d17f958 --- /dev/null +++ b/documentation/audit/13-documentation.md @@ -0,0 +1,81 @@ +# Exactitude de la documentation — conventions des composants récents, doc contre code + +## Synthèse + +Le périmètre est sain sur l'axe le plus dur : un balayage des 51 FQCN `Gplanchat\…` cités dans les +README, `UPGRADE.md` et `documentation/user/` ne trouve **aucun exemple PHP qui ne résoudrait pas** — +les 9 noms absents de `src/` sont tous des anciens noms, correctement présentés comme tels dans +`UPGRADE.md`. Les cinq ADR cités (DUR006, DUR030, DUR037, DUR038, DUR041) existent tous et disent +bien ce que les README leur font dire. Ce qui décroche est ailleurs : la **référence de +configuration**, qui se réclame exhaustive, ignore un nœud entier du `Configuration.php` et se +trompe sur un défaut ; le README du pont Temporal documente deux des quatre `purpose` que le code +accepte — les deux manquants étant précisément ceux que le guide utilisateur fait écrire. Le reste +tient de la métadonnée : un tableau racine en retard d'un paquet publié, une contrainte Symfony +recopiée à la main qui ment sur le `composer.json`, et quatre surfaces publiques en français sans +contrepartie anglaise. + +## Constats + +### C1 — La référence de configuration se dit exhaustive et ignore le nœud `dbal` en entier + +- **Fichier** : `documentation/user/configuration/_index.md:8` (« every key accepted by `DurableBundle` ») contre `src/DurableBundle/DependencyInjection/Configuration.php:18-25,30,50,74,83` +- **Gravité** : majeur +- **Constat** : Le nœud `durable.dbal` (`connection`, `lock_factory`) est absent de la page — ni exemple, ni section. Les quatre clés `table_name` (`event_store`, `workflow_metadata`, `activity_transport`, `child_workflow.parent_link_store`) ne sont documentées nulle part. Les trois `enumNode('type')` acceptent `['in_memory', 'dbal']` (lignes 29, 73, 82) alors que les tableaux de la page (`:45`, `:93`, `:146`) ne listent que `in_memory` — la page se contredit elle-même en `:58`, où sa prose parle de `event_store.type: dbal`. Enfin `:103` annonce `messenger` comme défaut de `activity_transport.type`, quand `Configuration.php:49` fait `->defaultValue('in_memory')`. +- **Amont** : le code lui-même (`Configuration.php`) ; c'est la sortie de `bin/console config:dump-reference durable` qui fait foi, comme pour tout bundle Symfony. +- **Correctif** : régénérer la page depuis `config:dump-reference` plutôt que de la maintenir à la main, ou au minimum ajouter la section `dbal`, les quatre `table_name`, la valeur `dbal` dans les trois énumérations et corriger le défaut de `activity_transport.type`. + +### C2 — Le README du pont Temporal documente deux des quatre `purpose` que la fabrique accepte + +- **Fichier** : `src/Bridge/Temporal/README.md:17` et `:29-31` contre `src/Bridge/Temporal/Messenger/TemporalTransportFactory.php:70,80,90` +- **Gravité** : majeur +- **Constat** : Le README annonce « **`options.purpose`** (`journal` \| `application`) » et sa table *Components* ne liste que `TemporalJournalTransport` et `TemporalApplicationTransport`. La fabrique en accepte quatre — son propre message d'erreur ligne 90 dit « expected journal, application, activity_worker, or nexus_worker » — et les deux manquants sont ceux que la documentation utilisateur fait écrire : `purpose: activity_worker` (`documentation/user/getting-started/_index.md:128`, `documentation/user/backends/_index.md:158`) et `purpose: nexus_worker` (`documentation/user/nexus/_index.md:185`). +- **Amont** : le code du paquet (`TemporalTransportFactory.php:70-90`) ; c'est la seule page d'accueil Packagist du satellite `durable-bridge-temporal`. +- **Correctif** : ajouter `activity_worker` et `nexus_worker` à la phrase d'invariant et les deux transports correspondants à la table *Components*. + +### C3 — `src/Durable/README.md` renvoie à un ADR sans rapport et annonce une extension Psalm qui n'existe pas + +- **Fichier** : `src/Durable/README.md:29` +- **Gravité** : majeur +- **Constat** : La ligne dit « Composer `suggest` lists optional PHPStan / **Psalm** extensions (see **DUR012** in the ADR index) ». Le `suggest` de `src/Durable/composer.json` ne cite qu'une extension, `gplanchat/durable-phpstan` ; **aucun paquet d'extension Psalm n'existe dans ce dépôt** — `SPLITS` n'en publie pas, et il n'y a pas de répertoire pour lui. (Psalm, lui, y est bien présent en tant qu'analyseur : `psalm.xml`, `psalm-baseline.xml`, `psalm-magento.xml`, et deux passages dans `.github/workflows/ci.yml`. C'est l'*extension* que la ligne annonce qui n'existe pas, pas l'outil.) Et DUR012 est *API client layer and repository adapters* — aucune mention de PHPStan ni de Psalm ; l'ADR qui fonde réellement l'extension est DUR038, celui que le `suggest` cite lui-même. +- **Amont** : `documentation/adr/DUR012-api-client-and-repository-adapter-layers.md` et `documentation/INDEX.md:31`. +- **Correctif** : remplacer la ligne par un renvoi à DUR038 et supprimer la mention Psalm — c'est le README du paquet phare, la première page que voit un lecteur de Packagist. + +### C4 — Le tableau des paquets du README racine est en retard d'un paquet publié + +- **Fichier** : `README.md:9-21` +- **Gravité** : majeur +- **Constat** : Le tableau liste huit paquets ; `gplanchat/durable-magento` (`src/DurableModule/`) n'y est pas, alors qu'il est *splitté et publié* (`bin/splitsh-publish.sh:39`), qu'il a son `composer.json`, son `LICENSE` et son README, et qu'il est documenté sur deux sections du guide (`documentation/user/packages/_index.md:20,288`). Même staleness plus bas : `README.md:71` ne renvoie qu'à 3 des 11 README de paquets, et `UPGRADE.md` n'est lié depuis nulle part dans le README racine. *(L'absence de `src/DurableDemoContracts/` est en revanche assumée et documentée par son propre README `:8-19` — pas un défaut.)* +- **Amont** : `bin/splitsh-publish.sh:39`, `documentation/user/packages/_index.md:288`. +- **Correctif** : ajouter la ligne `gplanchat/durable-magento` → `src/DurableModule/`, compléter la liste des README de paquets, et lier `UPGRADE.md` depuis la section *Documentation*. + +### C5 — Quatre surfaces publiques sont en français sans contrepartie anglaise, dont trois pages Packagist + +- **Fichier** : `UPGRADE.md:1`, `src/DurablePhpstan/README.md:1`, `src/DurableDemoContracts/README.md:1`, `src/DurableRector/README.md:34` +- **Gravité** : majeur +- **Constat** : Le guide utilisateur est délibérément bilingue (paires Hugo `_index.md` / `_index.fr.md`) — ce n'est pas le sujet. Le sujet est que ces quatre fichiers-là sont en français **sans version anglaise**, alors que les huit autres README de paquets, le README racine et la langue par défaut du site sont en anglais. Deux d'entre eux (`durable-phpstan`, `durable-rector`) sont la seule page d'accueil de satellites Packagist installables séparément, et `src/DurableRector/README.md` bascule de langue en son milieu (intro anglaise, section `## Monter de version à l'intérieur de Durable`). +- **Amont** : non sourcé côté convention amont — l'argument est interne : incohérence avec les huit README frères et avec la langue par défaut du site publié. +- **Correctif** : traduire ces quatre surfaces en anglais et, si le français doit être conservé, le faire sous la même mécanique `.fr.md` que le guide utilisateur. + +### C6 — La contrainte Symfony du README du bundle contredit son propre `composer.json` + +- **Fichier** : `src/DurableBundle/README.md:19` contre `src/DurableBundle/composer.json` +- **Gravité** : mineur +- **Constat** : Le README annonce « Symfony **6.4 || 7.4** ». Le `composer.json` du même répertoire contraint tous ses composants Symfony en `^6.4 || ^7.0 || ^8.0` — la 8.x est donc supportée et n'est pas annoncée, et « 7.4 » n'est pas une borne que la contrainte exprime. Le README du plugin, pour la même pile, écrit correctement « Symfony 6.4, 7.x or 8.x » (`src/DurablePlugin/README.md:64`) : les deux README du même dépôt se contredisent. +- **Amont** : `src/DurableBundle/composer.json` (source de vérité de l'installation). +- **Correctif** : aligner sur « Symfony 6.4, 7.x or 8.x », ou renvoyer au `composer.json` sans recopier la contrainte. + +### C7 — Cinq des sept `->info()` du bundle sont en français, et c'est ce qu'imprime `config:dump-reference` + +- **Fichier** : `src/DurableBundle/DependencyInjection/Configuration.php:20,22,23,38,42` (français) contre `:58,61` (anglais) +- **Gravité** : mineur +- **Constat** : Ces chaînes ne sont pas des commentaires : elles sont la documentation que Symfony rend à l'utilisateur via `bin/console config:dump-reference durable` et `debug:config`. Cinq sont en français, deux en anglais, dans le même arbre — un développeur qui déroule la référence lit deux langues. +- **Amont** : `symfony/symfony`, `src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php` — toutes les chaînes `->info()` du bundle de référence sont en anglais (vérifié sur la branche 7.4). +- **Correctif** : passer les cinq chaînes en anglais ; c'est aussi ce qui permettra de dériver la page de C1 automatiquement. + +### C8 — Trous structurels dans les README de paquets, et un `composer.json` sans mots-clés + +- **Fichier** : `src/Bridge/Dbal/README.md` (aucun `composer require`), `src/Durable/README.md`, `src/Bridge/Illuminate/README.md`, `src/DurableLaravel/README.md`, `src/DurableRector/README.md` (aucune section *Requirements*), `src/DurableDemoContracts/composer.json` +- **Gravité** : mineur +- **Constat** : `src/Bridge/Dbal/README.md` a *Requirements*, *Components*, *Configuration* et *Concurrency* mais ne dit jamais `composer require gplanchat/durable-bridge-dbal` — la seule commande d'installation manque au paquet le plus documenté. Symétriquement, quatre README ont la commande sans section *Requirements*. Côté métadonnées, les dix paquets publiés partagent une colonne vertébrale de mots-clés (`durable-execution`, `workflow`, `orchestration`, `long-running`) ; `gplanchat/durable-demo-contracts` est le seul sans aucun `keywords` — cohérent avec son non-publication, mais c'est la seule irrégularité de l'ensemble, qui est par ailleurs homogène. +- **Amont** : `symfony/symfony`, `src/Symfony/Component/Scheduler/README.md` — structure canonique d'un composant récent : titre, une phrase de description, puis *Resources* (Documentation / Contributing / Report issues). Les README Durable ont l'équivalent (encadré miroir + *Documentation* + *License*) ; ce sont l'installation et les prérequis qui manquent par endroits. +- **Correctif** : ajouter la ligne `composer require` au README DBAL et une section *Requirements* de trois lignes (PHP, dépendances majeures) aux quatre autres ; laisser `durable-demo-contracts` sans mots-clés est défendable puisqu'il n'est pas publié. diff --git a/documentation/audit/14-passes-profiler.md b/documentation/audit/14-passes-profiler.md new file mode 100644 index 00000000..924066bd --- /dev/null +++ b/documentation/audit/14-passes-profiler.md @@ -0,0 +1,62 @@ +# Compiler passes et profiler — collecte, sérialisation des données collectées + +## Synthèse +Le périmètre est fonctionnel mais s'écarte de deux conventions structurantes de `symfony/http-kernel` : les données collectées ne passent jamais par `cloneVar()`, et la trace qui les alimente tourne dans tous les environnements sans être réinitialisable par le `services_resetter`. Le premier point rend le profiler dépendant de la sérialisabilité des charges utiles applicatives ; le second fait grandir un tableau en mémoire pour la durée de vie d'un worker `messenger:consume`, en production. Les cinq passes de compilation sont correctement ordonnées (priorité 50 après `ResolveInstanceofConditionalsPass`, priorité 10 avant `MessengerPass`) et `NexusHandlerPass` est exemplaire sur le refus au démarrage, mais aucune n'utilise le second argument `$throwOnAbstract` de `findTaggedServiceIds()` et `ActivityHandlerPass` avale en silence une balise mal formée là où sa jumelle Nexus lève. Le template Twig est sain sur les deux axes demandés : aucun `|raw`, aucun `