From bcda19ca9fd1797a8d769e159fa1701959118119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Planchat?= Date: Thu, 3 Sep 2026 23:28:26 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(coeur):=20un=20effet=20de=20bord=20enre?= =?UTF-8?q?gistr=C3=A9=20est=20un=20=C3=A9tat,=20pas=20une=20valeur=20de?= =?UTF-8?q?=20retour?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `findSideEffectForSlot()` rend `mixed` et signalait « rien d'enregistré » par `null`. Une closure `sideEffect()` qui rend légitimement `null` était donc indistinguable d'un slot vide : elle était ré-exécutée à chaque passe de rejeu, et le journal grossissait d'un `SideEffectRecorded` par passe — c'est-à-dire exactement la garantie que la primitive existe pour offrir. `hasSideEffectForSlot(): bool` rejoint le port. La présence d'un slot ne se déduit plus de la valeur qu'il porte. La séparation existait déjà sur ce même port pour les minuteurs, où `isTimerSettled()` répond l'état et `findTimerSlotResult()` la valeur ; les effets de bord s'alignent dessus. Le test a délimité le défaut plus finement que le rapport d'audit : sur les cinq valeurs qui se confondent avec l'absence — null, false, 0, chaîne vide, liste vide — seule `null` échouait, la comparaison étant un `!==` strict. Les quatre autres restent dans le jeu de données comme filet : un correctif écrit avec `??` ou `isset()` rouvrirait le trou pour toutes. Les slots restaient alignés malgré le défaut, les réémissions atterrissant en fin de flux ; un cas le fixe pour que ça ne dépende plus d'un heureux hasard. Rupture pour qui implémente le port, c'est-à-dire pour qui écrit un backend ; aucune application appelante n'est touchée. Rector ne peut rien : la réponse dépend de la façon dont l'adaptateur range ses slots, et lui en faire deviner une produirait un adaptateur qui compile et ment. UPGRADE.md donne les deux formes attendues, et dit pourquoi `array_key_exists()` et jamais `isset()`. Suite unit : 1089 tests contre 1073 sur main, mêmes 4 erreurs d'environnement (illuminate/cache absent du poste, la CI l'installe par sa matrice). PHPStan : mêmes 2 erreurs pré-existantes, aucune nouvelle. Refs: B1 de documentation/audit/ Co-Authored-By: Claude Opus 5 (1M context) --- UPGRADE.md | 52 +++++++ .../Worker/TemporalExecutionHistory.php | 5 + src/Durable/ExecutionContext.php | 8 +- .../Port/WorkflowHistorySourceInterface.php | 18 ++- src/Durable/Store/EventStoreHistorySource.php | 15 ++ .../Durable/SideEffectSlotPresenceTest.php | 130 ++++++++++++++++++ 6 files changed, 224 insertions(+), 4 deletions(-) create mode 100644 tests/unit/Durable/SideEffectSlotPresenceTest.php diff --git a/UPGRADE.md b/UPGRADE.md index 074f7922..fe1ac396 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -22,6 +22,58 @@ vendor/bin/rector process src Le set est **cumulatif** : le passer une fois rattrape toutes les versions franchies d'un coup. Il ne contient que ce que Rector sait faire sans deviner ; tout le reste est écrit à la main ci-dessous. +## Non publié + +### `WorkflowHistorySourceInterface` gagne `hasSideEffectForSlot()` + +**Qui est concerné** : uniquement qui **implémente** `WorkflowHistorySourceInterface` — c'est-à-dire +qui écrit un backend. Une application qui appelle `sideEffect()` n'a rien à changer ; elle gagne le +correctif sans rien faire. + +**Ce qui était cassé.** `findSideEffectForSlot()` rend `mixed` et signalait « rien d'enregistré » par +`null`. Une closure qui rend légitimement `null` était donc indistinguable d'un slot vide : elle +était **ré-exécutée à chaque passe de rejeu**, et le journal grossissait d'un `SideEffectRecorded` +par passe. C'est la garantie même que `sideEffect()` existe pour offrir. Les valeurs `false`, `0`, +`''` et `[]` n'étaient pas touchées — la comparaison était un `!==` strict. + +**Ce qu'il faut écrire.** Une méthode qui répond *le slot existe-t-il*, sans regarder ce qu'il porte. +Rector ne peut rien ici : la réponse dépend de la façon dont votre backend range ses slots, et lui +en faire deviner une produirait un adaptateur qui compile et ment. Les deux implémentations livrées +donnent les deux formes attendues. + +Sur un journal parcouru : + +```php +public function hasSideEffectForSlot(int $slot): bool +{ + $index = 0; + foreach ($this->eventStore->readStream($this->executionId) as $event) { + if ($event instanceof SideEffectRecorded) { + if ($index === $slot) { + return true; + } + ++$index; + } + } + + return false; +} +``` + +Sur un tableau indexé par slot — et c'est `array_key_exists()`, jamais `isset()`, qui rouvrirait +exactement le trou que ce correctif ferme : + +```php +public function hasSideEffectForSlot(int $slot): bool +{ + return \array_key_exists($slot, $this->sideEffects); +} +``` + +`findSideEffectForSlot()` ne change pas de signature et garde son comportement : elle rend la valeur, +et rend `null` aussi bien pour un slot absent que pour un slot portant `null`. C'est désormais écrit +dans son contrat, et c'est `hasSideEffectForSlot()` qui décide s'il faut exécuter la closure. + ## 0.1.0-alpha8 ### Laravel refuse au démarrage un workflow dont les noms de paramètres divergent du contrat diff --git a/src/Bridge/Temporal/Worker/TemporalExecutionHistory.php b/src/Bridge/Temporal/Worker/TemporalExecutionHistory.php index 44fc1390..492dcb14 100644 --- a/src/Bridge/Temporal/Worker/TemporalExecutionHistory.php +++ b/src/Bridge/Temporal/Worker/TemporalExecutionHistory.php @@ -562,6 +562,11 @@ public function findScheduledTimerId(int $slot): ?string return $this->scheduledTimerIds[$slot] ?? null; } + public function hasSideEffectForSlot(int $slot): bool + { + return \array_key_exists($slot, $this->sideEffects); + } + public function findSideEffectForSlot(int $slot): mixed { return $this->sideEffects[$slot] ?? null; diff --git a/src/Durable/ExecutionContext.php b/src/Durable/ExecutionContext.php index 25cfadd0..0ff52dd3 100644 --- a/src/Durable/ExecutionContext.php +++ b/src/Durable/ExecutionContext.php @@ -311,10 +311,12 @@ private function refuseDivergence(string $slotKind, int $slotIndex, ?string $rec public function sideEffect(\Closure $closure): Awaitable { $slotIndex = $this->sideEffectSlotIndex++; - $replayResult = $this->historySource->findSideEffectForSlot($slotIndex); $deferred = new \Gplanchat\Durable\Awaitable\Deferred(); - if (null !== $replayResult) { - $deferred->resolve($replayResult); + + // La présence du slot, jamais la valeur qu'il porte : une closure qui rend `null` a bel et + // bien été exécutée, et la relire est exactement ce que `sideEffect()` promet. + if ($this->historySource->hasSideEffectForSlot($slotIndex)) { + $deferred->resolve($this->historySource->findSideEffectForSlot($slotIndex)); return $deferred->awaitable(); } diff --git a/src/Durable/Port/WorkflowHistorySourceInterface.php b/src/Durable/Port/WorkflowHistorySourceInterface.php index 5f1d292d..4c666bd1 100644 --- a/src/Durable/Port/WorkflowHistorySourceInterface.php +++ b/src/Durable/Port/WorkflowHistorySourceInterface.php @@ -81,7 +81,23 @@ public function findTimerSlotResult(int $slot): ?array; public function findScheduledTimerId(int $slot): ?string; /** - * Returns the recorded side effect result at slot N, or null if not yet recorded. + * Whether slot N holds a recorded side effect. + * + * Presence is a fact about the history; the recorded value is data. They must be asked + * separately, because a side effect legitimately records `null` — and inferring "not recorded" + * from a `null` result re-runs a non-deterministic closure on every replay and appends a + * `SideEffectRecorded` per pass, which is the one guarantee `sideEffect()` exists to give. + * + * The same separation already exists on this port for timers, where {@see isTimerSettled()} + * answers the state and {@see findTimerSlotResult()} the value. + */ + public function hasSideEffectForSlot(int $slot): bool; + + /** + * Returns the recorded side effect result at slot N. + * + * Returns `null` both for a slot that recorded `null` and for a slot that recorded nothing; + * callers deciding whether to run a closure MUST ask {@see hasSideEffectForSlot()} first. */ public function findSideEffectForSlot(int $slot): mixed; diff --git a/src/Durable/Store/EventStoreHistorySource.php b/src/Durable/Store/EventStoreHistorySource.php index b13c566c..5aefc249 100644 --- a/src/Durable/Store/EventStoreHistorySource.php +++ b/src/Durable/Store/EventStoreHistorySource.php @@ -220,6 +220,21 @@ public function findScheduledTimerId(int $slot): ?string return null; } + public function hasSideEffectForSlot(int $slot): bool + { + $index = 0; + foreach ($this->eventStore->readStream($this->executionId) as $event) { + if ($event instanceof SideEffectRecorded) { + if ($index === $slot) { + return true; + } + ++$index; + } + } + + return false; + } + public function findSideEffectForSlot(int $slot): mixed { $index = 0; diff --git a/tests/unit/Durable/SideEffectSlotPresenceTest.php b/tests/unit/Durable/SideEffectSlotPresenceTest.php new file mode 100644 index 00000000..057f35be --- /dev/null +++ b/tests/unit/Durable/SideEffectSlotPresenceTest.php @@ -0,0 +1,130 @@ + + */ + public static function valeursQuiSeConfondentAvecLAbsence(): iterable + { + yield 'null' => [null]; + yield 'false' => [false]; + yield 'zéro' => [0]; + yield 'chaîne vide' => ['']; + yield 'liste vide' => [[]]; + } + + #[DataProvider('valeursQuiSeConfondentAvecLAbsence')] + public function testUneClosureQuiRendUneValeurFausseNeTourneQuUneFois(mixed $valeur): void + { + $store = new InMemoryEventStore(); + $appels = 0; + + $workflow = static function (WorkflowEnvironment $wf) use ($valeur, &$appels): mixed { + return $wf->sideEffect(static function () use ($valeur, &$appels): mixed { + ++$appels; + + return $valeur; + }); + }; + + self::executer($store, 'exec-1', $workflow); + self::assertSame(1, $appels, 'la première passe exécute la closure'); + + self::executer($store, 'exec-1', $workflow); + self::assertSame(1, $appels, 'la passe de rejeu doit relire le résultat, pas le recalculer'); + } + + #[DataProvider('valeursQuiSeConfondentAvecLAbsence')] + public function testLeJournalNeGrossitPasDUnEvenementParPasse(mixed $valeur): void + { + $store = new InMemoryEventStore(); + + $workflow = static fn(WorkflowEnvironment $wf): mixed => $wf->sideEffect(static fn(): mixed => $valeur); + + self::executer($store, 'exec-1', $workflow); + self::executer($store, 'exec-1', $workflow); + self::executer($store, 'exec-1', $workflow); + + self::assertSame( + 1, + self::compterEffetsDeBord($store, 'exec-1'), + 'trois passes sur une seule instruction sideEffect() doivent laisser un seul événement', + ); + } + + #[DataProvider('valeursQuiSeConfondentAvecLAbsence')] + public function testLaValeurRelueEstLaValeurEnregistree(mixed $valeur): void + { + $store = new InMemoryEventStore(); + + $workflow = static fn(WorkflowEnvironment $wf): mixed => $wf->sideEffect(static fn(): mixed => $valeur); + + self::assertSame($valeur, self::executer($store, 'exec-1', $workflow)); + self::assertSame($valeur, self::executer($store, 'exec-1', $workflow), 'le rejeu rend la même valeur'); + } + + /** + * Les slots restent alignés : un effet de bord « faux » ne doit pas décaler celui d'après. + */ + public function testUnEffetDeBordFauxNeDecalePasLeSlotSuivant(): void + { + $store = new InMemoryEventStore(); + + $workflow = static fn(WorkflowEnvironment $wf): array => [ + 'premier' => $wf->sideEffect(static fn(): mixed => null), + 'second' => $wf->sideEffect(static fn(): string => 'après'), + ]; + + self::assertSame(['premier' => null, 'second' => 'après'], self::executer($store, 'exec-1', $workflow)); + self::assertSame(['premier' => null, 'second' => 'après'], self::executer($store, 'exec-1', $workflow)); + self::assertSame(2, self::compterEffetsDeBord($store, 'exec-1')); + } + + private static function executer(InMemoryEventStore $store, string $executionId, \Closure $workflow): mixed + { + $runner = new InMemoryWorkflowRunner( + $store, + new InMemoryActivityTransport(), + new RegistryActivityExecutor(), + 0, + new WorkflowRegistry(), + ); + + return $runner->run($executionId, $workflow); + } + + private static function compterEffetsDeBord(InMemoryEventStore $store, string $executionId): int + { + $total = 0; + foreach ($store->readStream($executionId) as $event) { + if ($event instanceof SideEffectRecorded) { + ++$total; + } + } + + return $total; + } +} From b387107ac56e192889d4840f9782f54f884a8335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9gory=20Planchat?= Date: Fri, 4 Sep 2026 01:56:29 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(durable):=20fermer=20le=20m=C3=AAme=20t?= =?UTF-8?q?rou=20chez=20l'appelant=20fr=C3=A8re,=20et=20corriger=20deux=20?= =?UTF-8?q?docblocs=20qui=20mentaient?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit La relecture croisée n'a bloqué cette branche sur aucun axe — douze l'ont retouchée, toutes sur la même remarque : le correctif est juste et il s'est arrêté au symptôme cité. **`hasRecordedWorkAhead()` gardait exactement le trou que la branche venait d'outiller.** C'est le signal « en train de rejouer » dont `version()` dépend. Il interrogeait les quatre types de slot qui savent dire leur présence et laissait les effets de bord de côté — et son docbloc l'assumait en écrivant que `findSideEffectForSlot()` rend `mixed`, donc qu'on ne peut pas distinguer « rien ici » de « ici, la valeur null ». Quinze lignes plus loin, la même branche ajoutait `hasSideEffectForSlot()` au port, qui répond précisément à cette question. Conséquence, et elle est pire que le trou d'origine : une exécution dont le travail restant devant elle n'est fait que d'effets de bord était vue comme arrivée au bout de son historique. Elle prenait la branche **neuve** en plein rejeu et y écrivait son marqueur de version, dans une histoire écrite avant que le point de changement existe. `testUnTravailRestantFaitDEffetsDeBord- RetientLAncienneVersion` rejoue le cas : sans ce commit, la seconde passe rend `3` là où elle doit rendre `ChangePoint::DEFAULT_VERSION`. **Le docbloc du port invoquait un précédent qui n'existe pas.** `{@see isTimerSettled()}` ne figure pas parmi les dix-huit méthodes de `WorkflowHistorySourceInterface`. La séparation dont il voulait se réclamer est réelle mais porte d'autres noms — `findScheduledTimerId()` pour l'état, `findTimerSlotResult()` pour la valeur — et elle vaut aussi pour les activités, workflows enfants et opérations Nexus, dont les trois méthodes sœurs enveloppent leur résultat dans un `array{result: mixed, ...}` pour cette raison exacte. C'est cet argument-là qui tient. `UPGRADE.md` gagne la note correspondante : le changement de `version()` ne demande rien à écrire, mais il change un comportement observable et une application a le droit de savoir lequel. Le formateur est repassé — c'est ce qui rendait la CS rouge sur les quatre versions de PHP. Restent ouverts, hors périmètre : il n'existe aucune suite de conformance que les implémenteurs de `WorkflowHistorySourceInterface` doivent passer, donc rien ne verra le prochain ajout au port ; et la moitié Temporal du correctif reste vérifiée par la seule lecture. Co-Authored-By: Claude Opus 5 (1M context) --- UPGRADE.md | 20 ++++++++++ src/Durable/ExecutionContext.php | 13 ++++--- .../Port/WorkflowHistorySourceInterface.php | 6 ++- .../Durable/SideEffectSlotPresenceTest.php | 39 ++++++++++++++++++- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index fe1ac396..10b1cd99 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -74,6 +74,26 @@ public function hasSideEffectForSlot(int $slot): bool et rend `null` aussi bien pour un slot absent que pour un slot portant `null`. C'est désormais écrit dans son contrat, et c'est `hasSideEffectForSlot()` qui décide s'il faut exécuter la closure. + +### `version()` cesse de basculer une exécution en vol + +**Qui est concerné** : toute application qui appelle `version()`. Rien à écrire ; le comportement +change, en mieux, et il faut savoir en quoi. + +`version()` décide de rendre l'ancien comportement quand l'exécution est encore en train de +rejouer. Ce signal se déduisait des quatre types de slot qui savent dire leur présence — activité, +minuteur, workflow enfant, opération Nexus — et laissait les effets de bord de côté, pour la raison +même que le correctif ci-dessus vient de lever : leur présence ne se lisait pas sans lire leur +valeur. + +Conséquence : une exécution dont le travail restant devant elle n'était fait que d'effets de bord +était vue comme arrivée au bout de son historique. Elle prenait la branche **neuve** au milieu d'un +rejeu et y écrivait son marqueur de version — dans une histoire écrite avant que le point de +changement existe. `hasSideEffectForSlot()` étant désormais au port, ce cas rejoint les autres. + +Une exécution qui a déjà écrit un marqueur de version garde le sien : `versionForChangeId()` est +consulté en premier, et rien de ce commit ne le touche. + ## 0.1.0-alpha8 ### Laravel refuse au démarrage un workflow dont les noms de paramètres divergent du contrat diff --git a/src/Durable/ExecutionContext.php b/src/Durable/ExecutionContext.php index 0ff52dd3..4255a7d2 100644 --- a/src/Durable/ExecutionContext.php +++ b/src/Durable/ExecutionContext.php @@ -239,17 +239,20 @@ public function version(string $changeId, int $minSupported, int $maxSupported): * Déduit, donc déterministe : deux replays de la même histoire répondent pareil, ce qui est la * seule propriété dont le versioning a besoin. * - * Les effets de bord ne sont pas consultés : `findSideEffectForSlot()` rend `mixed`, et une - * valeur enregistrée peut légitimement être `null` — on ne peut pas distinguer « rien ici » de - * « ici, la valeur null ». Un workflow dont le seul travail avant un point de changement est - * un effet de bord sera donc traité comme neuf. C'est le trou, il est étroit, et il est écrit. + * Les effets de bord comptent comme les autres depuis que le port sait dire leur présence + * sans passer par leur valeur. Ils ne le pouvaient pas tant que `findSideEffectForSlot()` + * rendait `mixed` : une valeur enregistrée peut légitimement être `null`, et « rien ici » ne + * s'y distinguait pas de « ici, la valeur null ». Un workflow dont le seul travail avant un + * point de changement était un effet de bord basculait alors sur la branche neuve, en plein + * rejeu — le trou est fermé avec celui de `sideEffect()`, dont il était la même cause. */ private function hasRecordedWorkAhead(): bool { return null !== $this->historySource->findScheduledActivityId($this->activitySlotIndex) || null !== $this->historySource->findScheduledTimerId($this->timerSlotIndex) || null !== $this->historySource->findScheduledChildExecutionId($this->childWorkflowSlotIndex) - || null !== $this->historySource->findScheduledNexusOperation($this->nexusOperationSlotIndex); + || null !== $this->historySource->findScheduledNexusOperation($this->nexusOperationSlotIndex) + || $this->historySource->hasSideEffectForSlot($this->sideEffectSlotIndex); } /** diff --git a/src/Durable/Port/WorkflowHistorySourceInterface.php b/src/Durable/Port/WorkflowHistorySourceInterface.php index 4c666bd1..192faf79 100644 --- a/src/Durable/Port/WorkflowHistorySourceInterface.php +++ b/src/Durable/Port/WorkflowHistorySourceInterface.php @@ -88,8 +88,10 @@ public function findScheduledTimerId(int $slot): ?string; * from a `null` result re-runs a non-deterministic closure on every replay and appends a * `SideEffectRecorded` per pass, which is the one guarantee `sideEffect()` exists to give. * - * The same separation already exists on this port for timers, where {@see isTimerSettled()} - * answers the state and {@see findTimerSlotResult()} the value. + * The same separation already exists on this port for timers, where + * {@see findScheduledTimerId()} answers the state and {@see findTimerSlotResult()} the value, + * and for activities, child workflows and Nexus operations, whose three sibling methods wrap + * their result in an `array{result: mixed, ...}` for exactly this reason. */ public function hasSideEffectForSlot(int $slot): bool; diff --git a/tests/unit/Durable/SideEffectSlotPresenceTest.php b/tests/unit/Durable/SideEffectSlotPresenceTest.php index 057f35be..5c523079 100644 --- a/tests/unit/Durable/SideEffectSlotPresenceTest.php +++ b/tests/unit/Durable/SideEffectSlotPresenceTest.php @@ -5,12 +5,13 @@ namespace Tests\Unit\Durable; use Gplanchat\Durable\Event\SideEffectRecorded; +use Gplanchat\Durable\InMemoryWorkflowRunner; use Gplanchat\Durable\RegistryActivityExecutor; use Gplanchat\Durable\Store\InMemoryEventStore; use Gplanchat\Durable\Transport\InMemoryActivityTransport; +use Gplanchat\Durable\Versioning\ChangePoint; use Gplanchat\Durable\WorkflowEnvironment; use Gplanchat\Durable\WorkflowRegistry; -use Gplanchat\Durable\InMemoryWorkflowRunner; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -127,4 +128,40 @@ private static function compterEffetsDeBord(InMemoryEventStore $store, string $e return $total; } + + /** + * L'appelant frère. `version()` demande « suis-je en train de rejouer ? » à + * `hasRecordedWorkAhead()`, qui interrogeait les quatre autres types de slot et pas les effets + * de bord, faute de pouvoir en lire la présence sans en lire la valeur. + * + * Une exécution dont le travail restant devant elle n'est fait que d'effets de bord était donc + * vue comme arrivée au bout de son historique. Elle prenait la branche neuve **en plein + * rejeu**, et écrivait son marqueur de version au milieu d'une histoire écrite avant que le + * point de changement existe — ce que `version()` est précisément là pour empêcher. + */ + public function testUnTravailRestantFaitDEffetsDeBordRetientLAncienneVersion(): void + { + $store = new InMemoryEventStore(); + + // Le code d'avant : deux effets de bord, aucun point de changement. + $avant = static fn(WorkflowEnvironment $wf): array => [ + 'premier' => $wf->sideEffect(static fn(): mixed => null), + 'second' => $wf->sideEffect(static fn(): string => 'après'), + ]; + self::executer($store, 'exec-1', $avant); + + // Le code d'après, sur la même exécution : un point de changement s'est glissé entre les + // deux effets de bord, et le second est encore devant. + $apres = static fn(WorkflowEnvironment $wf): array => [ + 'premier' => $wf->sideEffect(static fn(): mixed => null), + 'version' => $wf->version('changement-1', 1, 3), + 'second' => $wf->sideEffect(static fn(): string => 'après'), + ]; + + self::assertSame( + ChangePoint::DEFAULT_VERSION, + self::executer($store, 'exec-1', $apres)['version'], + 'une exécution en vol garde l\'ancien comportement tant qu\'il lui reste du journal à rejouer', + ); + } }