From 0e9651b4e3abb14f0428c6b6523a2d54f7798b9f Mon Sep 17 00:00:00 2001 From: Andres Contreras Date: Thu, 24 Sep 2026 12:45:58 -0700 Subject: [PATCH 1/3] fix(context,cli): a manifest naming a deleted class no longer stops the two commands that repair it, and firefly:cache says what it dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EagerSingletonsPass has skipped a definition whose class no longer exists since 26.09.1, and it was not enough: the guard protects the one pass it is written in. The deleted class was still in the manifest ContainerRegistrar received, so wireInterfaces() bound the interface it implemented — and tagged it as an implementation — to a class autoloading could not find. The throw therefore came out of make() for a perfectly live abstract while resolving a bean nobody had touched, and named a file the developer had already deleted. Three more places read a class straight off the same manifest with nothing between them and make(): RegisterBeanPostProcessorsPass at phase 700 (before eager singletons), InfrastructureStartPass, and RegisterEventListenersPass, whose listener closure throws on the first dispatch rather than at boot. composer dump-autoload could not recover it either, because package:discover boots the application too, so the only way out was rm bootstrap/cache/firefly/*.php, then composer dump-autoload, then firefly:cache, in that order. The check now lives at the one door every definition comes through. While firefly:cache or firefly:clear is the running command — AppScan::repairing(), deliberately wider than regenerating() and kept separate from it — BeanDefinitionRegistry::add() drops a definition whose class cannot be found and records it in a StaleDefinitionReport, so the registrar and all four passes see a manifest that agrees with what is on disk. CacheCommand prints one line naming what was dropped; the manifest it writes no longer mentions the class, so the next run is an ordinary clean one. Under every other command nothing changes, and that asymmetry is the more important half. A class that has gone missing in a process about to serve traffic is a broken deployment, not a stale cache, and dropping the definition there would hand the application an interface quietly rebound to whichever implementation survived, with nothing said anywhere. Only a MISSING class is ever tolerated: a class that exists and cannot be constructed still fails fast. The registry's new constructor arguments both default to the previous behaviour, so an existing `new BeanDefinitionRegistry` filters nothing and reports nothing. Tests. StaleDefinitionFilterTest pins the door itself, including the default that keeps a stale definition under any other command. StaleManifestWiringTest runs the four consumer paths the way boot runs them and carries two controls: a boot that is not repairing still throws "Target class [...] does not exist", and a class that exists and cannot be built still fails fast in a repair boot. StaleManifestRecoveryTest is the end-to-end case — a real source tree whose manifests are compiled by a SUBPROCESS (a scanner class_exists()es, and PHP never forgets a declared class, so an in-process compile would make the test pass for the wrong reason), the #[Primary] implementation then deleted from disk, firefly:cache exiting 0 and naming the entry, firefly:clear booting past the same manifest, and the same manifest still failing loudly under migrate. Docs. docs/cli.md and chapter 13 of both manuscripts carry the recovery, the console line it prints and the reason only these two commands drop anything; the CacheCommand listing under the provenance guard is re-quoted from the file. --- CHANGELOG.md | 47 ++++ book/src-es/13-cli-cache.md | 27 ++ book/src/13-cli-cache.md | 27 ++ docs/cli.md | 29 +++ .../FireflyAutoConfigureServiceProvider.php | 15 +- packages/cli/src/Command/CacheCommand.php | 39 +++ .../tests/Cache/StaleManifestRecoveryTest.php | 193 ++++++++++++++ packages/cli/tests/Support/StaleApp.php | 229 +++++++++++++++++ .../src/Definition/BeanDefinitionRegistry.php | 57 +++++ .../src/Definition/StaleDefinitionReport.php | 51 ++++ packages/context/src/Scan/AppScan.php | 30 ++- .../Definition/StaleDefinitionFilterTest.php | 120 +++++++++ .../tests/Pass/StaleManifestWiringTest.php | 241 ++++++++++++++++++ packages/context/tests/Scan/AppScanTest.php | 40 +++ tests/DocsProseIsRealTest.php | 5 +- 15 files changed, 1146 insertions(+), 4 deletions(-) create mode 100644 packages/cli/tests/Cache/StaleManifestRecoveryTest.php create mode 100644 packages/cli/tests/Support/StaleApp.php create mode 100644 packages/context/src/Definition/StaleDefinitionReport.php create mode 100644 packages/context/tests/Definition/StaleDefinitionFilterTest.php create mode 100644 packages/context/tests/Pass/StaleManifestWiringTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 7832849f..cb2aad96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,53 @@ All notable changes to LaraFly are documented here. This project uses CalVer (`Y ## [Unreleased] +One defect, reported by the same application that reported the `26.09.4` list, and it is the one a developer +hits on an ordinary afternoon: delete a `#[Component]`, forget to recompile, and neither of the two commands +that exist to repair the compiled cache can run any more. Nothing here changes what a served request does. + +### Fixed + +- **`packages/context` + `packages/container` — deleting a `#[Component]` no longer makes `firefly:cache` and + `firefly:clear` unrunnable, and `firefly:cache` says which entry it dropped.** `EagerSingletonsPass` has + skipped a definition whose class no longer exists since `26.09.1`, and it was not enough, because the guard + protects the one pass it is written in. The deleted class was still in the manifest `ContainerRegistrar` + received, so `wireInterfaces()` bound the interface it implemented — and tagged it as an implementation — to + a class autoloading could not find: the throw came out of `make()` for a perfectly live abstract, while + resolving a bean nobody had touched, and named a file the developer had already deleted. Three more places + read a class straight off the same manifest with nothing between them and `make()`: + `RegisterBeanPostProcessorsPass` (phase 700, *before* eager singletons), `InfrastructureStartPass`, and + `RegisterEventListenersPass`, whose listener closure throws on the first dispatch rather than at boot. + `composer dump-autoload` could not recover it either — `package:discover` boots the application too — so the + only way out was `rm bootstrap/cache/firefly/*.php`, then `composer dump-autoload`, then `firefly:cache`, in + that order: a three-step incantation a developer simply had to know. + + The check now lives at the one door every definition comes through. While `firefly:cache` or `firefly:clear` + is the running command, `BeanDefinitionRegistry::add()` drops a definition whose class cannot be found and + records it, so the registrar and all four passes see a manifest that agrees with what is on disk, and + `firefly:cache` prints one extra line naming what it dropped: + `firefly:cache — skipped 1 stale manifest entry naming a class that no longer exists: App\Security\ControlPlaneJwksProvider`. + The manifest it then writes no longer mentions the class, so the next run is an ordinary clean one. + + **Under every other command nothing changes**, and that asymmetry is deliberate rather than cautious: a + class that has gone missing in a process about to serve traffic is not a stale cache but a broken deployment + — a truncated artifact, a classmap built from a different tree — and dropping the definition there would + hand the application an interface quietly rebound to whichever implementation happened to survive, with + nothing said anywhere. Only a MISSING class is ever tolerated: a class that exists and cannot be constructed + still fails fast, in a repair command as much as anywhere else. + +### Added + +- **`packages/context` — `Firefly\Context\Scan\AppScan::repairing()`**, true while `firefly:cache` *or* + `firefly:clear` is the running command. Deliberately wider than `regenerating()` and kept separate from it: + `regenerating()` also decides whether a capability reads its compiled artifact or re-scans, which is a + question `firefly:clear` — which reads nothing and writes nothing — has no business answering. +- **`packages/context` — `Firefly\Context\Definition\StaleDefinitionReport`**, the append-only record of + what a repair boot dropped, bound as a container singleton by `FireflyAutoConfigureServiceProvider` and read + by `firefly:cache`. Shaped like `ConditionEvaluationReport`, for the same reason: reporting through a logger + would put a `psr/log` edge on the boot engine, which neither `firefly/context` nor `firefly/container` + carries. `BeanDefinitionRegistry` takes it, and the drop flag, as constructor arguments that both default to + the previous behaviour — an existing `new BeanDefinitionRegistry` filters nothing and reports nothing. + ## [26.09.4] - 2026-09-23 The gaps a second real application had to work around, closed in the framework instead. Every entry below diff --git a/book/src-es/13-cli-cache.md b/book/src-es/13-cli-cache.md index bfcee205..4bd2a047 100644 --- a/book/src-es/13-cli-cache.md +++ b/book/src-es/13-cli-cache.md @@ -40,8 +40,11 @@ final class CacheCommand extends Command $dir, )); + $this->reportStaleEntries(); + return self::SUCCESS; } + // … } ``` @@ -296,6 +299,30 @@ Seguro de ejecutar en cualquier momento: el siguiente arranque simplemente recur --- +## Cuando la caché nombra una clase que borraste + +Ambos comandos anteriores tienen que arrancar la aplicación antes de poder hacer su trabajo — `firefly:cache` no puede escribir los manifiestos sin arrancar primero la aplicación cuyos manifiestos son, y `firefly:clear` no puede borrar un directorio por el que no ha arrancado. Esa circularidad se vuelve incómoda en cuanto la caché está *equivocada*, y la forma habitual de equivocarla es borrar un `#[Component]` y no recompilar: `component.php` sigue nombrando la clase, y ningún autoloader la encuentra. + +Ese estado no se podía recuperar con ningún comando. La clase borrada seguía en el manifiesto que leía `ContainerRegistrar`, así que `wireInterfaces()` ligaba la interfaz que implementaba — y la etiquetaba como implementación — a una clase que no estaba. El fallo aparecía por tanto lejos de su causa: `Target class [App\Security\ControlPlaneJwksProvider] does not exist` salía al resolver un bean que nadie había tocado y que se limita a inyectar esa interfaz. `EagerSingletonsPass` omite una definición cuya clase ya no existe, y no servía de nada, porque la excepción nunca se lanzaba sobre la definición borrada. `composer dump-autoload` tampoco ayudaba — su `package:discover` arranca la aplicación también. La recuperación era borrar `bootstrap/cache/firefly/*.php` a mano, luego `composer dump-autoload`, y luego `firefly:cache`, en ese orden. + +`Firefly\Context\Scan\AppScan::repairing()` es la costura que lo cierra: mientras el comando en ejecución sea `firefly:cache` o `firefly:clear`, `BeanDefinitionRegistry` — la única puerta por la que entra toda definición — descarta una definición cuya clase no se encuentra, de modo que ni el registrar, ni el paso de bean post-processors, ni el de ciclo de vida, ni el de listeners llegan a verla. `firefly:cache` dice entonces qué entradas descartó: + +``` +php artisan firefly:cache +``` + +``` +firefly:cache — wrote 14 manifest(s) + 3 proxy(ies) to bootstrap/cache/firefly +firefly:cache — skipped 1 stale manifest entry naming a class that no longer exists: App\Security\ControlPlaneJwksProvider +``` + +El manifiesto que escribe ya no menciona la clase, así que la segunda ejecución es una ejecución limpia normal y la línea desaparece. + +!!! warning "Solo esos dos comandos descartan algo" + Bajo cualquier otro comando el manifiesto se toma tal cual, y una clase que ha desaparecido sigue deteniendo el arranque con el error del propio contenedor. Ahí esa es la respuesta correcta: una clase ausente en un proceso que está a punto de servir tráfico no es una caché rancia, es un despliegue roto — un artefacto truncado, un classmap construido desde otro árbol — y descartar la definición le entregaría a la aplicación una interfaz religada en silencio a la implementación que sobreviviera. Una respuesta equivocada de la que nadie se entera es peor que el fallo de arranque. Y solo se tolera una clase AUSENTE: una clase que existe y no se puede construir sigue fallando rápido, tanto en un comando de reparación como en cualquier otro sitio. + +--- + ## Actuator-sobre-CLI: `firefly:about`, `firefly:routes`, `firefly:health`, `firefly:metrics` El Capítulo 11 construyó una superficie de gestión alcanzable sobre HTTP. Estos cuatro comandos renderizan los **mismos** endpoints en el terminal, en-proceso — no se hace ninguna petición HTTP, y ninguno de ellos reimplementa lógica alguna del actuator: diff --git a/book/src/13-cli-cache.md b/book/src/13-cli-cache.md index 91969697..14ceaf20 100644 --- a/book/src/13-cli-cache.md +++ b/book/src/13-cli-cache.md @@ -40,8 +40,11 @@ final class CacheCommand extends Command $dir, )); + $this->reportStaleEntries(); + return self::SUCCESS; } + // … } ``` @@ -296,6 +299,30 @@ Safe to run at any time: the very next boot simply falls back to the in-process --- +## When the cache names a class you deleted + +Both commands above have to boot the application before they can do their work — `firefly:cache` cannot write the manifests without first booting the application whose manifests they are, and `firefly:clear` cannot delete a directory it has not booted past. That circularity turns uncomfortable the moment the cache is *wrong*, and the ordinary way to make it wrong is to delete a `#[Component]` and not recompile: `component.php` still names the class, and no autoloader can find it. + +That state used to be unrecoverable by any single command. The deleted class was still in the manifest `ContainerRegistrar` read, so `wireInterfaces()` bound the interface it implemented — and tagged it as an implementation — to a class that was not there. The failure therefore surfaced nowhere near its cause: `Target class [App\Security\ControlPlaneJwksProvider] does not exist` came out of resolving a bean nobody had touched, one that merely injects that interface. `EagerSingletonsPass` skips a definition whose class is gone, and it did not help, because the throw was never raised on the deleted definition. `composer dump-autoload` could not help either — its `package:discover` boots the application too. The recovery was to delete `bootstrap/cache/firefly/*.php` by hand, then `composer dump-autoload`, then `firefly:cache`, in that order. + +`Firefly\Context\Scan\AppScan::repairing()` is the seam that closes it: while `firefly:cache` or `firefly:clear` is the running command, `BeanDefinitionRegistry` — the one door every definition enters through — drops a definition whose class cannot be found, so the registrar, the bean-post-processor pass, the lifecycle pass and the event-listener pass never see it. `firefly:cache` then names what it dropped: + +``` +php artisan firefly:cache +``` + +``` +firefly:cache — wrote 14 manifest(s) + 3 proxy(ies) to bootstrap/cache/firefly +firefly:cache — skipped 1 stale manifest entry naming a class that no longer exists: App\Security\ControlPlaneJwksProvider +``` + +The manifest it writes no longer mentions the class, so the second run is an ordinary clean one and the line goes away. + +!!! warning "Only those two commands drop anything" + Under every other command the manifest is trusted exactly as it was, and a class that has gone missing still stops the boot with the container's own error. That is the right answer there: a missing class in a process about to serve traffic is not a stale cache, it is a broken deployment — a truncated artifact, a classmap built from a different tree — and dropping the definition would hand the application an interface quietly rebound to whichever implementation survived. A wrong answer nobody is told about is worse than the boot failure. And only a MISSING class is ever tolerated: a class that exists and cannot be constructed still fails fast, in a repair command as much as anywhere else. + +--- + ## Actuator-over-CLI: `firefly:about`, `firefly:routes`, `firefly:health`, `firefly:metrics` Chapter 11 built a management surface reachable over HTTP. These four commands render the **same** endpoints at the terminal, in-process — no HTTP request is made, and none of them reimplement any actuator logic: diff --git a/docs/cli.md b/docs/cli.md index 43b20cf4..0a5eb64d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -94,6 +94,35 @@ rather than run unprotected. Compiling is still worth it — reflection-free boot is the point of `firefly:cache` — but it is now an optimisation rather than a correctness requirement. +### When the cache names a class you deleted + +Deleting a `#[Component]` and forgetting to recompile leaves `bootstrap/cache/firefly/component.php` naming a class +no autoloader can find. That used to be unrecoverable by any single command: the deleted class was still in the +manifest the container registrar read, so the interface it implemented was bound to a class that was not there, and +`firefly:cache` and `firefly:clear` — each of which has to boot the application before it can rewrite or delete the +manifest — both died with `Target class [...] does not exist`, pointing at a file you had already deleted, from +inside a bean you had not touched. + +Those two commands now drop a manifest entry whose class cannot be found, and `firefly:cache` says which one: + +``` +php artisan firefly:cache +``` + +``` +firefly:cache — wrote 14 manifest(s) + 3 proxy(ies) to bootstrap/cache/firefly +firefly:cache — skipped 1 stale manifest entry naming a class that no longer exists: App\Security\ControlPlaneJwksProvider +``` + +The manifest it writes no longer mentions the class, so the next run is an ordinary clean one and the line goes away. + +**Only these two commands drop anything, and that is deliberate.** Under every other command the manifest is trusted +exactly as before, and a missing class still stops the boot. A class that has gone missing in a process about to +serve traffic is not a stale cache — it is a broken deployment, a truncated artifact or a classmap built from a +different tree — and quietly dropping the definition there would rebind the interface to whichever implementation +happened to survive, with nothing said anywhere. Only a MISSING class is ever tolerated: a class that exists and +cannot be constructed still fails fast, in a repair command as much as anywhere else. + ## `firefly:clear` ``` diff --git a/packages/autoconfigure/src/FireflyAutoConfigureServiceProvider.php b/packages/autoconfigure/src/FireflyAutoConfigureServiceProvider.php index 2944cab1..f2475b2b 100644 --- a/packages/autoconfigure/src/FireflyAutoConfigureServiceProvider.php +++ b/packages/autoconfigure/src/FireflyAutoConfigureServiceProvider.php @@ -21,6 +21,7 @@ use Firefly\Context\Condition\ConditionEvaluator; use Firefly\Context\Definition\BeanDefinitionRegistry; use Firefly\Context\Definition\DefinitionSource; +use Firefly\Context\Definition\StaleDefinitionReport; use Firefly\Context\Pass\ConditionPassOnePass; use Firefly\Context\Pass\ConditionPassTwoPass; use Firefly\Context\Pass\ContextRefreshedPass; @@ -115,9 +116,21 @@ private function bindBootContextAndKernel(): void $config = $this->config(); $profiles = (new ProfileResolver)->resolve(); + // The record of what a repair boot dropped, and the ONLY thing that can name it afterwards. + // + // Bound on the container even though BootContext deliberately is not, and the two are not in + // tension: BootContext carries live boot machinery (the registry, the evaluator, the container + // itself) that must never be reachable from a served request, while this is an append-only list + // of class names that can only be non-empty in a `firefly:cache` / `firefly:clear` process — a + // console process that serves nothing and exits. firefly/cli's CacheCommand resolves it to print + // the one line a developer needs; ActuatorRouteRegistrar binds ConditionEvaluationReport the same + // way, for the same reason. + $stale = new StaleDefinitionReport; + $this->app->instance(StaleDefinitionReport::class, $stale); + $bootContext = new BootContext( container: $container, - definitions: new BeanDefinitionRegistry, + definitions: new BeanDefinitionRegistry($stale, AppScan::repairing()), config: $config, profiles: $profiles, conditions: new ConditionEvaluator($config, $profiles), diff --git a/packages/cli/src/Command/CacheCommand.php b/packages/cli/src/Command/CacheCommand.php index 26e8791d..9509e8f1 100644 --- a/packages/cli/src/Command/CacheCommand.php +++ b/packages/cli/src/Command/CacheCommand.php @@ -6,6 +6,7 @@ use Firefly\Cli\Cache\FireflyCachePaths; use Firefly\Cli\Cache\ManifestCacheWriter; +use Firefly\Context\Definition\StaleDefinitionReport; use Illuminate\Console\Command; final class CacheCommand extends Command @@ -36,6 +37,44 @@ public function handle(): int $dir, )); + $this->reportStaleEntries(); + return self::SUCCESS; } + + /** + * Names the entries the boot that led here dropped, because it booted from a manifest that had gone + * stale and they named classes autoloading could not find. + * + * Without this line the run is indistinguishable from a clean one — same "wrote N manifest(s)", same + * exit 0 — and a developer would have no way to tell that the application they just booted was wired + * differently from the file it was wired from. The classes named here are the ones the manifest just + * written no longer mentions, so the message doubles as confirmation that the deletion took effect. + * + * A warning rather than info: nothing is wrong any more, but a dropped entry is still a fact worth + * seeing in a wall of green. Guarded by bound() because the command has to keep working in a process + * where FireflyAutoConfigureServiceProvider never registered — Lumen, a bare container harness — and + * an unreported skip is a far smaller failure than a command that cannot run. + */ + private function reportStaleEntries(): void + { + if (! $this->laravel->bound(StaleDefinitionReport::class)) { + return; + } + + /** @var StaleDefinitionReport $stale */ + $stale = $this->laravel->make(StaleDefinitionReport::class); + if ($stale->isEmpty()) { + return; + } + + $classes = $stale->classes(); + + $this->warn(sprintf( + 'firefly:cache — skipped %d stale manifest %s naming a class that no longer exists: %s', + count($classes), + count($classes) === 1 ? 'entry' : 'entries', + implode(', ', $classes), + )); + } } diff --git a/packages/cli/tests/Cache/StaleManifestRecoveryTest.php b/packages/cli/tests/Cache/StaleManifestRecoveryTest.php new file mode 100644 index 00000000..5157059a --- /dev/null +++ b/packages/cli/tests/Cache/StaleManifestRecoveryTest.php @@ -0,0 +1,193 @@ + + */ +function staleAppConfig(string $src, string $cache): array +{ + return [ + 'firefly' => [ + 'cache' => [ + 'path' => $cache, + 'component_manifest' => $cache.'/'.FireflyCachePaths::COMPONENT, + 'context_manifest' => $cache.'/'.FireflyCachePaths::CONTEXT, + ], + 'scan' => ['paths' => [StaleApp::NAMESPACE => $src]], + ], + ]; +} + +/** + * Runs $body with $argv installed as the process's command line, and puts back whatever was there. + * + * `$_SERVER['argv']` is what AppScan reads to decide whether this boot is one of the two that repair the + * compiled cache, and a test process's own command line is `pest`. The same swap UncachedMethodSecurityTest + * makes for the strict-method-security stand-down, for the same reason. + * + * @template TReturn + * + * @param list $argv + * @param Closure(): TReturn $body + * @return TReturn + */ +function withStaleArgv(array $argv, Closure $body): mixed +{ + $original = $_SERVER['argv'] ?? null; + $_SERVER['argv'] = $argv; + + try { + return $body(); + } finally { + $original === null ? array_key_exists('argv', $_SERVER) && ($_SERVER['argv'] = []) : $_SERVER['argv'] = $original; + } +} + +/** + * Every class named by the compiled component manifest in $cache. + * + * @return list + */ +function staleManifestClasses(string $cache): array +{ + return array_map( + static fn (ComponentDescriptor $c): string => $c->class, + ComponentManifest::load($cache.'/'.FireflyCachePaths::COMPONENT)->components, + ); +} + +/** + * Boots the stale app and runs the real CacheCommand against it. + * + * @return array{0: int, 1: string} + */ +function runStaleCacheCommand(Application $app): array +{ + $command = new CacheCommand; + $command->setLaravel($app); + + $output = new BufferedOutput; + $exit = $command->run(new ArrayInput([]), $output); + + return [$exit, $output->fetch()]; +} + +beforeEach(function () { + StaleApp::restoreStaleCache(); +}); + +it('recovers from a manifest naming a deleted class, and names the entry it skipped', function () { + ['src' => $src, 'cache' => $cache] = StaleApp::prepare(); + + // The precondition the whole test rests on: the compiled manifest still names the class, and the class + // really is gone from this process — not stubbed absent, deleted from the tree it was compiled from. + expect(class_exists(StaleApp::GHOST))->toBeFalse() + ->and(staleManifestClasses($cache))->toContain(StaleApp::GHOST); + + [$exit, $rendered] = withStaleArgv(['artisan', 'firefly:cache'], function () use ($src, $cache): array { + // Booting at all is half the assertion: before this change the boot below threw + // BindingResolutionException from EagerSingletonsPass and the command never ran. + $app = fireflyApplication(config: staleAppConfig($src, $cache), needs: ['cache']); + + return runStaleCacheCommand($app); + }); + + // One command, exit 0, and a line that names the file the developer deleted. + expect($exit)->toBe(0) + ->and($rendered)->toContain('skipped 1 stale manifest entry') + ->and($rendered)->toContain(StaleApp::GHOST); + + // …and the manifest it wrote no longer mentions it, so the second run is an ordinary clean one. + expect(staleManifestClasses($cache)) + ->not->toContain(StaleApp::GHOST) + ->toContain(StaleApp::SURVIVOR); +}); + +it('says nothing about stale entries once the manifest is clean', function () { + ['src' => $src, 'cache' => $cache] = StaleApp::prepare(); + + $rendered = withStaleArgv(['artisan', 'firefly:cache'], function () use ($src, $cache): string { + $app = fireflyApplication(config: staleAppConfig($src, $cache), needs: ['cache']); + runStaleCacheCommand($app); + + // A SECOND boot, now over the manifest the first run wrote. + $second = fireflyApplication(config: staleAppConfig($src, $cache), needs: ['cache']); + + /** @var string */ + return runStaleCacheCommand($second)[1]; + }); + + expect($rendered)->toContain('wrote') + ->and($rendered)->not->toContain('skipped'); +}); + +/* + | THE CONTROL, and the reason the skip is scoped to the two commands that repair the cache. + | + | A class that has gone missing in a process that is about to serve traffic is not a stale cache: it is a + | broken deployment — a truncated artifact, a classmap built from a different tree — and dropping the + | definition there would hand the application an interface quietly rebound to whichever implementation + | survived, with nothing said. A wrong answer nobody is told about is far worse than the boot failure this + | change removes, so under every other command the boot fails exactly as loudly as it always did. + */ +it('still fails loudly when the same stale manifest is booted under any other command', function () { + ['src' => $src, 'cache' => $cache] = StaleApp::prepare(); + + withStaleArgv(['artisan', 'migrate'], function () use ($src, $cache): void { + expect(fn () => fireflyApplication(config: staleAppConfig($src, $cache), needs: ['cache'])) + ->toThrow(BindingResolutionException::class, 'Target class ['.StaleApp::GHOST.'] does not exist.'); + }); +}); + +/* + | The other half of the escape hatch. EagerSingletonsPass's docblock has named BOTH firefly:cache and + | firefly:clear as the commands a stale manifest must not be able to block, and until AppScan grew + | repairing() the claim was only true of one of them: firefly:clear boots the application too, so a + | manifest naming a deleted class stopped the command whose entire job is to throw that manifest away. + */ +it('lets firefly:clear boot past the same stale manifest and remove the cache', function () { + ['src' => $src, 'cache' => $cache] = StaleApp::prepare(); + + $exit = withStaleArgv(['artisan', 'firefly:clear'], function () use ($src, $cache): int { + $app = fireflyApplication(config: staleAppConfig($src, $cache), needs: ['cache']); + + $command = new ClearCommand; + $command->setLaravel($app); + + return $command->run(new ArrayInput([]), new BufferedOutput); + }); + + expect($exit)->toBe(0) + ->and(is_dir($cache))->toBeFalse(); +}); diff --git a/packages/cli/tests/Support/StaleApp.php b/packages/cli/tests/Support/StaleApp.php new file mode 100644 index 00000000..711d5553 --- /dev/null +++ b/packages/cli/tests/Support/StaleApp.php @@ -0,0 +1,229 @@ + self::$root.'/src', 'cache' => self::$root.'/cache']; + } + + /** A pristine copy of the STALE manifests, so each test boots from the same artifacts firefly:cache rewrites. */ + public static function restoreStaleCache(): void + { + ['cache' => $cache] = self::prepare(); + + // firefly:clear deletes the whole directory, so the restore has to be able to put it back. + if (! is_dir($cache)) { + mkdir($cache, 0o755, true); + } + + foreach (glob((string) self::$root.'/stale/*.php') ?: [] as $file) { + copy($file, $cache.'/'.basename($file)); + } + } + + private static function register(): void + { + if (self::$registered) { + return; + } + + self::$registered = true; + + spl_autoload_register(static function (string $class): void { + if (! str_starts_with($class, self::NAMESPACE) || self::$root === null) { + return; + } + + $file = self::$root.'/src/'.str_replace('\\', '/', substr($class, strlen(self::NAMESPACE))).'.php'; + if (is_file($file)) { + require $file; + } + }); + } + + private static function write(): void + { + $src = (string) self::$root.'/src'; + mkdir($src, 0o755, true); + + file_put_contents($src.'/StaleContract.php', <<<'PHP' + write(['FireflyStaleFixture\\' => $src], %s); + + PHP, + var_export($autoload, true), + var_export($root.'/src', true), + var_export($root.'/cache', true), + )); + + $output = []; + $status = 0; + exec(escapeshellarg(PHP_BINARY).' '.escapeshellarg($script).' 2>&1', $output, $status); + + if ($status !== 0) { + throw new RuntimeException("compiling the stale fixture failed: \n".implode("\n", $output)); + } + + // Keep the pristine stale copy: the command under test rewrites the cache directory, and every test + // in the file has to start from the same broken artifacts. + mkdir($root.'/stale', 0o755, true); + foreach (glob($root.'/cache/*.php') ?: [] as $file) { + copy($file, $root.'/stale/'.basename($file)); + } + } +} diff --git a/packages/context/src/Definition/BeanDefinitionRegistry.php b/packages/context/src/Definition/BeanDefinitionRegistry.php index 28bab432..e20a3a4e 100644 --- a/packages/context/src/Definition/BeanDefinitionRegistry.php +++ b/packages/context/src/Definition/BeanDefinitionRegistry.php @@ -14,17 +14,74 @@ * MUST NOT touch the container — the definition stage and the instance stage are deliberately * split so that exactly one condition-filtered ComponentManifest is ever handed to the container * registrar (FlushDefinitionsPass). + * + * IT IS ALSO THE ONE DOOR EVERY DEFINITION COMES THROUGH, which is why the repair-boot filter below + * lives here rather than at a consumer. EagerSingletonsPass has skipped a definition whose class no + * longer exists since 26.09.1, and that guard was still not enough to let `firefly:cache` repair a + * stale manifest: it protects the ONE pass it is written in, and a deleted #[Component] that implemented + * an interface breaks the boot somewhere else entirely. ContainerRegistrar::wireInterfaces() binds the + * INTERFACE to the missing class and tags it as an implementation, so the throw comes out of resolving a + * bean that still exists and injects that contract — `Target class [...] does not exist` for a perfectly + * live abstract, with the deleted class named nowhere near the failure. Three more consumers read a class + * straight off the manifest with nothing between them and `make()`: RegisterBeanPostProcessorsPass + * (phase 700, BEFORE eager singletons), InfrastructureStartPass, and RegisterEventListenersPass, whose + * listener closure throws on the first dispatch rather than at boot. + * + * Filtering at the door closes all five at once, and leaves each consumer's own guard as depth rather + * than as the only thing standing between a developer and `rm -rf bootstrap/cache/firefly`. */ final class BeanDefinitionRegistry { /** @var list */ private array $definitions = []; + /** + * @param StaleDefinitionReport $stale where a dropped definition is recorded so the running command can name it + * @param bool $dropMissingClasses ONLY a repair boot passes true — see add() + */ + public function __construct( + private readonly StaleDefinitionReport $stale = new StaleDefinitionReport, + private readonly bool $dropMissingClasses = false, + ) {} + + /** + * Adds a definition — unless this is a repair boot and the definition names a class autoloading + * cannot find, in which case it is dropped and recorded instead. + * + * WHY THE DROP IS SCOPED TO A REPAIR BOOT, and not applied to every boot. A compiled manifest that + * names a class no longer on disk describes an application that has already moved on, and during + * `firefly:cache` or `firefly:clear` that is the whole point: the command exists to replace or delete + * the manifest it is booting from, it serves no request, and the fresh manifest it writes will not + * contain the entry at all. Refusing to boot there is refusing to be repaired — which is exactly the + * hole a developer fell into, and why the recovery was a three-step incantation (`rm` the cache + * directory, `composer dump-autoload`, `firefly:cache`) rather than the one command that advertises + * itself as the fix. + * + * Under EVERY OTHER command the definition is kept, and the boot fails exactly as loudly as it did + * before. That asymmetry is deliberate and is the more important half of this change. A missing class + * in a served process is not a stale cache, it is a broken deployment — a truncated artifact, a + * classmap built from a different tree — and silently dropping the definition there would let the + * application serve traffic with an interface quietly rebound to whichever implementation survived, + * or with a BeanPostProcessor that never ran. A wrong answer nobody is told about is far worse than + * the boot failure this change was written to remove. + */ public function add(BeanDefinition $definition): void { + if ($this->dropMissingClasses && ! class_exists($definition->class())) { + $this->stale->record($definition->class()); + + return; + } + $this->definitions[] = $definition; } + /** The classes this registry dropped, for whoever is in a position to report them. */ + public function stale(): StaleDefinitionReport + { + return $this->stale; + } + public function remove(string $class): void { $this->definitions = array_values(array_filter( diff --git a/packages/context/src/Definition/StaleDefinitionReport.php b/packages/context/src/Definition/StaleDefinitionReport.php new file mode 100644 index 00000000..2aa59661 --- /dev/null +++ b/packages/context/src/Definition/StaleDefinitionReport.php @@ -0,0 +1,51 @@ + */ + private array $classes = []; + + public function record(string $class): void + { + if (in_array($class, $this->classes, true)) { + return; + } + + $this->classes[] = $class; + } + + /** @return list */ + public function classes(): array + { + return $this->classes; + } + + public function isEmpty(): bool + { + return $this->classes === []; + } +} diff --git a/packages/context/src/Scan/AppScan.php b/packages/context/src/Scan/AppScan.php index 002a82e3..747a774d 100644 --- a/packages/context/src/Scan/AppScan.php +++ b/packages/context/src/Scan/AppScan.php @@ -39,6 +39,9 @@ final class AppScan /** The console command that regenerates every artefact below; see self::regenerating(). */ public const string REGENERATE_COMMAND = 'firefly:cache'; + /** The console command that deletes every artefact below; see self::repairing(). */ + public const string CLEAR_COMMAND = 'firefly:clear'; + public const string COMPONENT = 'component.php'; public const string CONTEXT = 'context.php'; @@ -128,6 +131,31 @@ public static function cachedFile(Container $app, string $basename): ?string * Lumen, where the helper does not exist at all. */ public static function regenerating(): bool + { + return self::running(self::REGENERATE_COMMAND); + } + + /** + * True while one of the two commands that REPAIR the compiled cache is the command being run. + * + * `firefly:cache` rewrites those files and `firefly:clear` deletes them, and each can only do its job + * by first booting the application whose files it is about to replace or remove. So a boot in one of + * these two processes is the one boot that must not be stopped by what is on disk — it is on its way + * to fix exactly that. EagerSingletonsPass's own docblock names both commands as the escape hatch its + * guard restores; this is the seam that makes the claim true for the other four places a compiled + * class name reaches `make()` (see BeanDefinitionRegistry::add()). + * + * Deliberately WIDER than regenerating() and kept separate from it rather than folded in: + * regenerating() also decides whether a capability reads its compiled artefact or re-scans, and + * `firefly:clear` — which reads nothing and writes nothing — has no business changing that. + */ + public static function repairing(): bool + { + return self::running(self::REGENERATE_COMMAND) || self::running(self::CLEAR_COMMAND); + } + + /** See regenerating() for why `$_SERVER['argv']` is read rather than the Laravel helper. */ + private static function running(string $command): bool { if (PHP_SAPI !== 'cli' && PHP_SAPI !== 'phpdbg') { return false; @@ -136,7 +164,7 @@ public static function regenerating(): bool /** @var mixed $argv */ $argv = $_SERVER['argv'] ?? null; - return is_array($argv) && in_array(self::REGENERATE_COMMAND, $argv, true); + return is_array($argv) && in_array($command, $argv, true); } public static function dir(Container $app): string diff --git a/packages/context/tests/Definition/StaleDefinitionFilterTest.php b/packages/context/tests/Definition/StaleDefinitionFilterTest.php new file mode 100644 index 00000000..44071240 --- /dev/null +++ b/packages/context/tests/Definition/StaleDefinitionFilterTest.php @@ -0,0 +1,120 @@ +add(staleFilterDefinition('App\\Deleted\\GoneProvider')); + $registry->add(staleFilterDefinition(StaleFilterLiveFixture::class)); + + expect(array_map( + static fn (BeanDefinition $d): string => $d->class(), + $registry->all(), + ))->toBe([StaleFilterLiveFixture::class]) + ->and($report->classes())->toBe(['App\\Deleted\\GoneProvider']) + ->and($report->isEmpty())->toBeFalse(); +}); + +// The dropped entry must not reach the container registrar either — toComponentManifest() is what +// FlushDefinitionsPass hands over, and wireInterfaces() binding a contract to a missing class is the +// crash this whole change exists to remove. +it('never puts a dropped definition into the ComponentManifest it hands the registrar', function () { + $registry = new BeanDefinitionRegistry(new StaleDefinitionReport, dropMissingClasses: true); + + $registry->add(staleFilterDefinition('App\\Deleted\\GoneProvider')); + $registry->add(staleFilterDefinition(StaleFilterLiveFixture::class)); + + expect(array_map( + static fn (ComponentDescriptor $d): string => $d->class, + $registry->toComponentManifest()->components, + ))->toBe([StaleFilterLiveFixture::class]); +}); + +// A repair boot is not a licence to lose beans that are perfectly fine. Nothing else changes. +it('keeps every definition whose class does exist', function () { + $report = new StaleDefinitionReport; + $registry = new BeanDefinitionRegistry($report, dropMissingClasses: true); + + $registry->add(staleFilterDefinition(StaleFilterLiveFixture::class)); + + expect($registry->all())->toHaveCount(1) + ->and($report->isEmpty())->toBeTrue(); +}); + +/* + | The default, which is what every boot that is NOT firefly:cache or firefly:clear gets: the manifest is + | trusted exactly as it was before this change. A missing class in a served process means the deployment + | is wrong — a truncated artifact, a classmap built from another tree — and dropping the definition there + | would let the application serve traffic with an interface quietly rebound to whichever implementation + | survived. That wrong answer, given in silence, is worse than the boot failure being removed. + */ +it('keeps a definition whose class is missing when this is not a repair boot', function () { + $report = new StaleDefinitionReport; + $registry = new BeanDefinitionRegistry($report); + + $registry->add(staleFilterDefinition('App\\Deleted\\GoneProvider')); + + expect($registry->all())->toHaveCount(1) + ->and($report->isEmpty())->toBeTrue(); +}); + +// The same class can reach the registry as a user definition and again from an auto-configuration. The +// developer wants the list of files to stop worrying about, not a tally of registry writes. +it('names a dropped class once however many definitions carried it', function () { + $report = new StaleDefinitionReport; + $registry = new BeanDefinitionRegistry($report, dropMissingClasses: true); + + $registry->add(staleFilterDefinition('App\\Deleted\\GoneProvider')); + $registry->add(staleFilterDefinition('App\\Deleted\\GoneProvider')); + + expect($report->classes())->toBe(['App\\Deleted\\GoneProvider']); +}); + +it('reports nothing at all when the manifest is clean', function () { + $report = new StaleDefinitionReport; + $registry = new BeanDefinitionRegistry($report, dropMissingClasses: true); + + $registry->add(staleFilterDefinition(StaleFilterLiveFixture::class)); + + expect($report->isEmpty())->toBeTrue() + ->and($report->classes())->toBe([]); +}); diff --git a/packages/context/tests/Pass/StaleManifestWiringTest.php b/packages/context/tests/Pass/StaleManifestWiringTest.php new file mode 100644 index 00000000..7de93eed --- /dev/null +++ b/packages/context/tests/Pass/StaleManifestWiringTest.php @@ -0,0 +1,241 @@ + $interfaces + */ +function staleWiringDefinition(string $class, array $interfaces = [], bool $primary = false, int $order = 0): BeanDefinition +{ + return new BeanDefinition(new ComponentDescriptor( + class: $class, + stereotype: 'component', + name: null, + scope: Scope::Singleton, + primary: $primary, + order: $order, + qualifier: null, + interfaces: $interfaces, + beans: [], + )); +} + +/** + * A BootContext over a registry that either filters (a repair boot) or does not (everything else). + * + * @param list $definitions + */ +function staleWiringContext(array $definitions, bool $repairing, ?StaleDefinitionReport $report = null): BootContext +{ + $registry = new BeanDefinitionRegistry($report ?? new StaleDefinitionReport, dropMissingClasses: $repairing); + foreach ($definitions as $definition) { + $registry->add($definition); + } + + $config = new Config(new Repository([])); + $profiles = new Profiles(['default']); + + return new BootContext( + container: new Container, + definitions: $registry, + config: $config, + profiles: $profiles, + conditions: new ConditionEvaluator($config, $profiles), + report: new ConditionEvaluationReport, + contextManifest: new ContextManifest([]), + ); +} + +it('binds the contract to the implementation that survived, not to the one that was deleted', function () { + $report = new StaleDefinitionReport; + + $context = staleWiringContext([ + // #[Primary], which is precisely why the stale entry used to win the interface binding. + staleWiringDefinition(STALE_WIRING_GHOST, [StaleWiringContract::class], primary: true), + staleWiringDefinition(StaleWiringSurvivor::class, [StaleWiringContract::class]), + staleWiringDefinition(StaleWiringConsumer::class), + ], repairing: true, report: $report); + + (new FlushDefinitionsPass)->run($context); + + // The whole failure, in one line: this used to throw + // BindingResolutionException("Target class [App\Deleted\ControlPlaneJwksProvider] does not exist.") + // while resolving a consumer that was never deleted and never changed. + (new EagerSingletonsPass)->run($context); + + /** @var StaleWiringConsumer $consumer */ + $consumer = $context->container->make(StaleWiringConsumer::class); + + expect($consumer->contract)->toBeInstanceOf(StaleWiringSurvivor::class) + ->and($context->container->make(StaleWiringContract::class))->toBeInstanceOf(StaleWiringSurvivor::class) + ->and($report->classes())->toBe([STALE_WIRING_GHOST]); +}); + +// getAll()/tagged() resolve every tagged implementation, so a ghost left in the tag list throws there too — +// on a contract whose other implementations are all perfectly healthy. +it('leaves the deleted implementation out of the contract tag', function () { + $context = staleWiringContext([ + staleWiringDefinition(STALE_WIRING_GHOST, [StaleWiringContract::class]), + staleWiringDefinition(StaleWiringSurvivor::class, [StaleWiringContract::class]), + ], repairing: true); + + (new FlushDefinitionsPass)->run($context); + + $tagged = iterator_to_array($context->container->tagged('firefly.contract.'.StaleWiringContract::class)); + + expect($tagged)->toHaveCount(1) + ->and($tagged[0])->toBeInstanceOf(StaleWiringSurvivor::class); +}); + +// Phase 700, BEFORE eager singletons: a deleted BeanPostProcessor took the boot down earlier than the guard +// EagerSingletonsPass carries could ever run. +it('registers the surviving bean post-processors past a deleted one', function () { + $context = staleWiringContext([ + staleWiringDefinition(STALE_WIRING_GHOST_PROCESSOR, [BeanPostProcessor::class]), + staleWiringDefinition(StaleWiringProcessor::class, [BeanPostProcessor::class]), + staleWiringDefinition(StaleWiringSurvivor::class), + ], repairing: true); + + (new FlushDefinitionsPass)->run($context); + (new RegisterBeanPostProcessorsPass)->run($context); + + expect($context->container->make(StaleWiringSurvivor::class))->toBeInstanceOf(StaleWiringSurvivor::class); +}); + +it('starts the surviving lifecycles past a deleted one', function () { + StaleWiringLifecycle::$started = false; + + $context = staleWiringContext([ + staleWiringDefinition(STALE_WIRING_GHOST_LIFECYCLE, [Lifecycle::class]), + staleWiringDefinition(StaleWiringLifecycle::class, [Lifecycle::class]), + ], repairing: true); + + (new FlushDefinitionsPass)->run($context); + (new InfrastructureStartPass)->run($context); + + expect(StaleWiringLifecycle::$started)->toBeTrue(); +}); + +/* + | THE CONTROL, and the reason the filter is scoped to a repair boot rather than applied to every one. + | + | Under any other command the manifest is trusted exactly as it was before this change, and a class that + | has gone missing still takes the boot down with the container's own error. That is the right answer + | there: a missing class in a served process is a broken deployment — a truncated artifact, a classmap + | built from a different tree — and dropping the definition would let the application serve traffic with + | this contract quietly rebound to whichever implementation happened to survive. A wrong answer nobody is + | told about is far worse than the boot failure this change removes. + */ +it('still fails loudly under any command that is not repairing the cache', function () { + $context = staleWiringContext([ + staleWiringDefinition(STALE_WIRING_GHOST, [StaleWiringContract::class], primary: true), + staleWiringDefinition(StaleWiringSurvivor::class, [StaleWiringContract::class]), + staleWiringDefinition(StaleWiringConsumer::class), + ], repairing: false); + + (new FlushDefinitionsPass)->run($context); + + expect(fn () => (new EagerSingletonsPass)->run($context)) + ->toThrow(BindingResolutionException::class, 'Target class ['.STALE_WIRING_GHOST.'] does not exist.'); +}); + +// Only a MISSING class is ever tolerated, in a repair boot as much as anywhere else. A class that exists and +// genuinely cannot be built is a defect in code that is still there, and failing fast at boot is right for it. +it('still fails fast in a repair boot when a class that DOES exist cannot be constructed', function () { + $context = staleWiringContext([ + staleWiringDefinition(StaleWiringConsumer::class), + ], repairing: true); + + (new FlushDefinitionsPass)->run($context); + + expect(fn () => (new EagerSingletonsPass)->run($context)) + ->toThrow(BindingResolutionException::class); +}); diff --git a/packages/context/tests/Scan/AppScanTest.php b/packages/context/tests/Scan/AppScanTest.php index 6de4fb9f..8b6c7197 100644 --- a/packages/context/tests/Scan/AppScanTest.php +++ b/packages/context/tests/Scan/AppScanTest.php @@ -126,3 +126,43 @@ function appScanContainer(array $firefly = []): Container @rmdir($dir); } }); + +/* + | `firefly:clear` deletes the compiled cache, and it can only do that by first booting the application + | whose cache it is about to remove. EagerSingletonsPass's docblock names BOTH commands as the escape + | hatch its guard restores, and the claim was only ever true of one of them: the definition registry's + | stale-entry filter (BeanDefinitionRegistry::add) reads THIS seam, so a manifest naming a deleted class + | no longer stops the command that exists to throw that manifest away. + | + | It stays separate from regenerating(), which also decides whether a capability reads its compiled + | artefact or re-scans — a question `firefly:clear` has no business answering. + */ +it('reports that it is repairing while either firefly:cache or firefly:clear is the running command', function (array $argv, bool $expected) { + $original = $_SERVER['argv'] ?? null; + $_SERVER['argv'] = $argv; + + try { + expect(AppScan::repairing())->toBe($expected); + } finally { + $original === null ? array_key_exists('argv', $_SERVER) && ($_SERVER['argv'] = []) : $_SERVER['argv'] = $original; + } +})->with([ + 'firefly:cache' => [['artisan', 'firefly:cache'], true], + 'firefly:clear' => [['artisan', 'firefly:clear'], true], + 'with options first' => [['artisan', '--no-ansi', 'firefly:clear'], true], + 'another command' => [['artisan', 'migrate'], false], + 'a lookalike' => [['artisan', 'firefly:clear-all'], false], + 'no command' => [['artisan'], false], +]); + +it('keeps firefly:clear out of regenerating(), so a clear never changes which artefacts are read', function () { + $original = $_SERVER['argv'] ?? null; + $_SERVER['argv'] = ['artisan', 'firefly:clear']; + + try { + expect(AppScan::regenerating())->toBeFalse() + ->and(AppScan::repairing())->toBeTrue(); + } finally { + $original === null ? array_key_exists('argv', $_SERVER) && ($_SERVER['argv'] = []) : $_SERVER['argv'] = $original; + } +}); diff --git a/tests/DocsProseIsRealTest.php b/tests/DocsProseIsRealTest.php index 37e38e90..27ab3b1a 100644 --- a/tests/DocsProseIsRealTest.php +++ b/tests/DocsProseIsRealTest.php @@ -1519,13 +1519,14 @@ function fireflyOAuth2InstallProse(array $client, array $server): array } } - // docs/cli.md, both tutorials, and the DI and CQRS chapters in both languages; the pair count in chapters + // docs/cli.md, both tutorials, and the DI and CQRS chapters in both languages, plus the stale-manifest + // recovery block that docs/cli.md and chapter 13 of each manuscript print; the pair count in chapters // 3, 7 (twice), 8 and 9 of each manuscript; the artifact tree in chapter 13 of each. The step count is // stated twice per manuscript — chapter 13's opening objective and the line over the artifact tree — and // the recap row once. These canaries are what turns a REWORDING red: a sentence that stops matching stops // being checked, silently, and that is precisely how "(11 steps, 12 artifacts)" survived the correction // of the paragraph 277 lines above it. - expect($consoleLines)->toBe(7) + expect($consoleLines)->toBe(10) ->and($pairClaims)->toBe(10) ->and($artifactClaims)->toBe(2) ->and($stepClaims)->toBe(4) From 465476bc8639e8eb38c5a02d81229ca01d67e20e Mon Sep 17 00:00:00 2001 From: Andres Contreras Date: Thu, 24 Sep 2026 12:57:44 -0700 Subject: [PATCH 2/3] feat(eda-kafka)!: every record dead-lettered to a .DLT carries the three provenance headers PyFly writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING: KafkaConsumerClient::deadLetter() takes the whole ReceivedEnvelope and a reason; deadLetterRaw() is gone. LaraFly wrote a dead-letter record with no headers at all, and PyFly writes three. dworkers runs both frameworks against shared topics, so one .DLT held two kinds of record: PyFly's, which says why it died and where it came from, and LaraFly's, which was raw bytes with no provenance whatsoever. Whoever drained that topic could not tell a LaraFly poison record from a replayed payload, and the offset needed to go back and look at the original was not there to be read. grep -rn 'x-dlt' packages/ returned nothing. The port had two dead-letter methods — one taking an EventEnvelope for an exhausted retry, one taking raw bytes for a poison record — and neither was handed the provenance it would have needed. They are now one method taking the record itself, because the record already carries the bytes or the envelope, the topic it was read from, and the broker handle its offset hangs off; the only thing this layer owes the DLT is WHY, which is the third argument. RdKafkaConsumerClient stamps x-dlt-reason, x-dlt-source-topic and x-dlt-source-offset, spelled exactly as PyFly spells them (src/pyfly/eda/adapters/kafka.py), because a topic both frameworks publish to is only readable if one `kcat -C -t .DLT -f '%h'` explains every record on it. The reason is the throw's short class name — PyFly's type(exc).__name__ — or RetriesExhausted for a record that decoded perfectly well and then ran out of retries, a case PyFly does not have because it deliberately does not dead-letter handler failures. A header the record cannot answer is left out rather than written empty: an x-dlt-source-offset of '' reads as an offset. producev() rather than produce(), which cannot carry headers; it has been in ext-rdkafka since 3.1, well below the librdkafka >= 1.5.3 this package already suggests. RabbitMQ needs none of this and gets none. The broker itself stamps x-death — source queue, exchange, reason and count — on everything its x-dead-letter-exchange routes, and the framework never republishes a message there to have an opinion about. Postgres keeps the row. Tests. dltHeaders() and dltPayload() are public and static for the same reason received() is: they are the halves of deadLetter() that owe nothing to ext-rdkafka, so the header set, the omission of a header the record cannot answer, and the raw-bytes-verbatim rule are all unit-asserted with no extension installed. KafkaEventConsumerTest pins both reasons through the consumer. The ext+broker-gated KafkaRoundTripTest reads the three headers back off the real DLT record, which is the only place producev() is exercised at all. --- CHANGELOG.md | 63 +++++++++++---- docs/modules/eda-brokers.md | 28 ++++++- packages/eda-kafka/README.md | 4 + .../eda-kafka/src/KafkaConsumerClient.php | 22 +++-- packages/eda-kafka/src/KafkaEventConsumer.php | 43 +++++++--- .../eda-kafka/src/RdKafkaConsumerClient.php | 80 +++++++++++++++++-- .../Fixtures/FakeKafkaConsumerClient.php | 15 +--- .../tests/Integration/KafkaRoundTripTest.php | 15 +++- .../tests/KafkaEventConsumerTest.php | 62 ++++++++++++-- 9 files changed, 269 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb2aad96..343fa939 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,53 @@ All notable changes to LaraFly are documented here. This project uses CalVer (`Y ## [Unreleased] -One defect, reported by the same application that reported the `26.09.4` list, and it is the one a developer -hits on an ordinary afternoon: delete a `#[Component]`, forget to recompile, and neither of the two commands -that exist to repair the compiled cache can run any more. Nothing here changes what a served request does. +Two things the same application found, one on a developer's machine and one on a shared Kafka topic. Delete a +`#[Component]`, forget to recompile, and neither of the two commands that exist to repair the compiled cache +could run any more. And a `.DLT` that LaraFly and PyFly both publish to held two kinds of record: +PyFly's, which says why it died and where it came from, and LaraFly's, which was raw bytes with no provenance +at all. Nothing here changes what a served request does. + +### BREAKING + +- **`packages/eda-kafka` — `KafkaConsumerClient::deadLetter()` takes the whole record and a reason, and + `deadLetterRaw()` is gone.** The port had two dead-letter methods — one taking an `EventEnvelope` for an + exhausted retry, one taking raw bytes for a poison record — and both wrote a record with **no headers at + all**. That is what a shared dead-letter topic cannot afford. dworkers runs LaraFly and PyFly against the + same topics, and PyFly has stamped `x-dlt-reason`, `x-dlt-source-topic` and `x-dlt-source-offset` on every + record it dead-letters since `v26.09.06`, so whoever drained a `.DLT` could not tell a LaraFly poison + record from a replayed payload, and the offset needed to go back and look at the original was not there. + `grep -rn 'x-dlt' packages/` returned nothing. The two methods are now one — + `deadLetter(ReceivedEnvelope $received, string $dltTopic, string $reason)` — because the record already + carries everything the provenance needs (the bytes or the envelope, the topic it was read from, and the + broker handle its offset hangs off), and only the consumer knows *why*. **Migration:** an application that + implements `KafkaConsumerClient` itself — a test double, or a client wrapping a different Kafka extension — + replaces its two methods with the one. Nothing else: `KafkaEventConsumer` is the only caller, and an + application that merely *uses* the Kafka adapter sees no API change, only three headers it did not have. + +### Added + +- **`packages/eda-kafka` — every record LaraFly dead-letters to a `.DLT` carries `x-dlt-reason`, + `x-dlt-source-topic` and `x-dlt-source-offset`.** The names and the reason's spelling are PyFly's, exactly: + the short class name of the throw that refused the bytes (`SerializationException`, `JsonException`, …, + which is PyFly's `type(exc).__name__`), or `RetriesExhausted` for a record that decoded perfectly well and + then ran out of retries — a case PyFly does not have, because it deliberately does not dead-letter handler + failures. The source topic is the one the record was **consumed** from, not the DLT and not the envelope's + declared destination. A header the record cannot answer is left out rather than written empty, because an + empty offset reads as an offset. Written with `producev()` (ext-rdkafka ≥ 3.1, well below the + `librdkafka >= 1.5.3` this package already suggests); the poison path still re-produces the **raw bytes + verbatim**, so a fixed producer can replay them byte for byte. RabbitMQ needs none of this and gets none: + the broker itself stamps `x-death` on everything its `x-dead-letter-exchange` routes, and the framework + never republishes a message there to have an opinion about. +- **`packages/context` — `Firefly\Context\Scan\AppScan::repairing()`**, true while `firefly:cache` *or* + `firefly:clear` is the running command. Deliberately wider than `regenerating()` and kept separate from it: + `regenerating()` also decides whether a capability reads its compiled artifact or re-scans, which is a + question `firefly:clear` — which reads nothing and writes nothing — has no business answering. +- **`packages/context` — `Firefly\Context\Definition\StaleDefinitionReport`**, the append-only record of + what a repair boot dropped, bound as a container singleton by `FireflyAutoConfigureServiceProvider` and read + by `firefly:cache`. Shaped like `ConditionEvaluationReport`, for the same reason: reporting through a logger + would put a `psr/log` edge on the boot engine, which neither `firefly/context` nor `firefly/container` + carries. `BeanDefinitionRegistry` takes it, and the drop flag, as constructor arguments that both default to + the previous behaviour — an existing `new BeanDefinitionRegistry` filters nothing and reports nothing. ### Fixed @@ -38,19 +82,6 @@ that exist to repair the compiled cache can run any more. Nothing here changes w nothing said anywhere. Only a MISSING class is ever tolerated: a class that exists and cannot be constructed still fails fast, in a repair command as much as anywhere else. -### Added - -- **`packages/context` — `Firefly\Context\Scan\AppScan::repairing()`**, true while `firefly:cache` *or* - `firefly:clear` is the running command. Deliberately wider than `regenerating()` and kept separate from it: - `regenerating()` also decides whether a capability reads its compiled artifact or re-scans, which is a - question `firefly:clear` — which reads nothing and writes nothing — has no business answering. -- **`packages/context` — `Firefly\Context\Definition\StaleDefinitionReport`**, the append-only record of - what a repair boot dropped, bound as a container singleton by `FireflyAutoConfigureServiceProvider` and read - by `firefly:cache`. Shaped like `ConditionEvaluationReport`, for the same reason: reporting through a logger - would put a `psr/log` edge on the boot engine, which neither `firefly/context` nor `firefly/container` - carries. `BeanDefinitionRegistry` takes it, and the drop flag, as constructor arguments that both default to - the previous behaviour — an existing `new BeanDefinitionRegistry` filters nothing and reports nothing. - ## [26.09.4] - 2026-09-23 The gaps a second real application had to work around, closed in the framework instead. Every entry below diff --git a/docs/modules/eda-brokers.md b/docs/modules/eda-brokers.md index f8fa40a7..418266b6 100644 --- a/docs/modules/eda-brokers.md +++ b/docs/modules/eda-brokers.md @@ -19,7 +19,7 @@ messages back off the wire — something M9's `start()`/`stop()` never needed. |---|---|---|---|---| | `firefly/eda-rabbitmq` | `php-amqplib/php-amqplib` (AMQP 0-9-1) | `"exchange/routingKey"` (or a bare routing key against the default exchange) | Broker-native DLX (dead-letter exchange) | `rabbitmq` | | `firefly/eda-postgres` | `ext-pdo_pgsql` (via Laravel's `ConnectionInterface`) | a destination string stored on the outbox row | Outbox row `status='FAILED'` | `postgres` | -| `firefly/eda-kafka` | `ext-rdkafka` (**OPTIONAL**) | a topic name | Dead-letter topic `.DLT` | `kafka` | +| `firefly/eda-kafka` | `ext-rdkafka` (**OPTIONAL**) | a topic name | Dead-letter topic `.DLT`, with `x-dlt-*` provenance headers | `kafka` | All three implement the same `EventPublisher::publish(string $destination, string $eventType, array $payload, array $headers = []): void` signature `firefly/eda` defines, and all three ship an @@ -276,8 +276,9 @@ decode throws, hands the loop a *poison* `ReceivedEnvelope` (`envelope` null, `r `failure`, `destination`). `JsonSerializer` itself checks every member's *type*, not just the six keys, and refuses each mismatch as one `SerializationException`; the `Throwable` catch is the backstop for any other `Serializer`. The loop never offers it to the sink, logs the destination and the -failure, and calls `nack(requeue: false)` — Kafka produces the raw bytes to `.DLT` and commits the -offset, RabbitMQ's queue routes it to its DLX — and polls the next record. It used to throw out of `poll()`, +failure, and calls `nack(requeue: false)` — Kafka produces the raw bytes to `.DLT`, with the three +provenance headers under [DLQ surface per broker](#dlq-surface-per-broker), and commits the offset; RabbitMQ's +queue routes it to its DLX — and polls the next record. It used to throw out of `poll()`, outside every catch in the process, and a supervisor restarted the worker onto the same offset for ever. `poll()` itself stays outside the loop's try on purpose: what can still throw from it is the transport (a lost connection, an auth refusal), and for that there is no record in hand to nack — the honest answer is to let @@ -307,7 +308,26 @@ Each broker's native surface, reached via an explicit `nack(requeue: false)`: application code copies the message anywhere. - **Kafka**: there is no broker-native DLX equivalent, so `KafkaEventConsumer::nack(requeue: false)` re-produces the envelope to a **dead-letter topic** named `".DLT"`, then commits the - original offset so the exhausted record is never redelivered from its original topic. + original offset so the exhausted record is never redelivered from its original topic. A poison record + (one whose bytes no `Serializer` would decode) is re-produced from its **raw bytes, verbatim**, so a + fixed producer can replay them byte for byte. + + Every record LaraFly writes to a `.DLT` carries three headers, so whoever drains that topic can tell a + dead-lettered record from a replayed payload and can find the original in the log: + + | Header | Value | + |---|---| + | `x-dlt-reason` | the short class name of the throw that refused the bytes (`SerializationException`, `JsonException`, …), or `RetriesExhausted` for a record that decoded and then ran out of retries | + | `x-dlt-source-topic` | the topic the record was **consumed** from — not the DLT, and not the envelope's declared destination | + | `x-dlt-source-offset` | the offset it sat at | + + The names and the reason's spelling are PyFly's, exactly: a `.DLT` that both frameworks publish to + is only readable if one `kcat -C -t .DLT -f '%h'` explains every record on it. A header the record + cannot answer is left out rather than written empty, because an empty offset reads as an offset. + + RabbitMQ needs none of this and gets none: the broker itself stamps `x-death` — source queue, exchange, + reason and count — on every message its `x-dead-letter-exchange` routes, and the framework never + republishes a message there to have an opinion about. Postgres keeps the row. - **Postgres**: there is no separate DLQ store at all — an exhausted outbox row is simply marked `status='FAILED'` (with `error_message` and `failed_at` populated) in place, on the same `firefly_eda_outbox` table. Query for `status='FAILED'` rows to inspect or reprocess them. diff --git a/packages/eda-kafka/README.md b/packages/eda-kafka/README.md index c2f0e6ab..38c8e0f7 100644 --- a/packages/eda-kafka/README.md +++ b/packages/eda-kafka/README.md @@ -9,6 +9,10 @@ precedent), gated behind `#[ConditionalOnProperty('firefly.eda.provider', having dead-letter topic (`.DLT`) for exhausted retries and a `KafkaHealthIndicator` feeding `/actuator/health`. +Every record it writes to a `.DLT` carries `x-dlt-reason`, `x-dlt-source-topic` and `x-dlt-source-offset` — +the same three headers, spelled the same way, that PyFly stamps on the records it dead-letters, so a topic +both frameworks publish to is readable from one `kcat -C -t .DLT -f '%h'`. + See [EDA Brokers](../../docs/modules/eda-brokers.md) for the full adapter reference. Apache-2.0 © Firefly Software Solutions Inc. diff --git a/packages/eda-kafka/src/KafkaConsumerClient.php b/packages/eda-kafka/src/KafkaConsumerClient.php index fbfcc73b..ba93ee44 100644 --- a/packages/eda-kafka/src/KafkaConsumerClient.php +++ b/packages/eda-kafka/src/KafkaConsumerClient.php @@ -5,7 +5,6 @@ namespace Firefly\Eda\Kafka; use Firefly\Eda\Consumer\ReceivedEnvelope; -use Firefly\Eda\EventEnvelope; /** * The minimal, ext-AGNOSTIC seam KafkaEventConsumer programs to — exactly the librdkafka operations the consumer @@ -18,8 +17,14 @@ * consume() returns a mapped ?ReceivedEnvelope (null = "nothing this tick"): the RD_KAFKA_RESP_ERR_* `match` that * decides message-vs-nothing lives inside RdKafkaConsumerClient because those constants are ext-only. commit()'s * $deliveryTag is the broker-native handle carried on the ReceivedEnvelope (the rdkafka Message for the real adapter); - * deadLetter() re-produces $envelope to the already-computed $dltTopic string; deadLetterRaw() does the same for the - * bytes of a POISON record (ReceivedEnvelope::poison()), which has no envelope to re-encode. + * deadLetter() re-produces one record to the already-computed $dltTopic string. + * + * deadLetter() TAKES THE WHOLE RECORD, and it used to take the pieces — an EventEnvelope for an exhausted retry, and + * a second method taking raw bytes for a poison record. Both wrote a record with NO headers at all, which is what a + * `.DLT` cannot afford: whoever drains that topic has to be able to tell a dead-lettered record from a replayed + * payload, and to find the offset it came from. The record carries everything that answer needs — the bytes or the + * envelope, the topic it was read from, and the broker handle the offset hangs off — so the port hands it over whole + * rather than making each adapter re-derive provenance it was never given. */ interface KafkaConsumerClient { @@ -31,10 +36,13 @@ public function consume(int $timeoutMs): ?ReceivedEnvelope; public function commit(mixed $deliveryTag): void; - public function deadLetter(EventEnvelope $envelope, string $dltTopic): void; - - /** Re-produce RAW bytes that could not be decoded to $dltTopic — verbatim, so a fixed producer can replay them. */ - public function deadLetterRaw(string $raw, string $dltTopic): void; + /** + * Re-produce one record to $dltTopic with $reason recorded on it. + * + * A POISON record (ReceivedEnvelope::poison()) is re-produced from its RAW bytes, verbatim, so a fixed producer + * can replay them byte for byte; any other record is re-encoded from its envelope. + */ + public function deadLetter(ReceivedEnvelope $received, string $dltTopic, string $reason): void; public function close(): void; } diff --git a/packages/eda-kafka/src/KafkaEventConsumer.php b/packages/eda-kafka/src/KafkaEventConsumer.php index 691769ad..15e7a541 100644 --- a/packages/eda-kafka/src/KafkaEventConsumer.php +++ b/packages/eda-kafka/src/KafkaEventConsumer.php @@ -34,6 +34,11 @@ * offset — so the exhausted record is never redelivered from its original topic, mirroring RabbitMqEventConsumer's DLX * routing outcome with a topic instead of an exchange. The DLT topic is derived from the broker-agnostic * EventEnvelope::$destination (the topic the publisher targeted), keeping this layer free of `\RdKafka\Message`. + * + * WHAT THIS LAYER OWES THE DLT is the REASON, and only the reason. The record already knows the bytes, the topic it + * was read from and the broker handle its offset hangs off, so the client can stamp those itself; why the record is + * being dead-lettered is a distinction only this method makes — a body the serializer refused, or a body that decoded + * and then ran out of retries — and reasonFor() is where it is named. */ final class KafkaEventConsumer implements EventConsumer { @@ -83,18 +88,38 @@ public function nack(ReceivedEnvelope $received, bool $requeue = true): void $envelope = $received->envelope; - if ($envelope === null) { - // A poison record: the RAW bytes go to the DLT of the topic they were read from, verbatim, and the - // offset is committed so the loop never reads them again. The topic comes from the record because - // there is no envelope to read a destination off. - $this->client->deadLetterRaw((string) $received->raw, ($received->destination ?? 'unknown').$this->deadLetterSuffix); - $this->client->commit($received->deliveryTag); + // A poison record has no envelope to read a destination off, so the DLT is derived from the topic the + // record was READ from; anything else keeps deriving it from the destination the publisher targeted. + $origin = $envelope !== null ? $envelope->destination : ($received->destination ?? 'unknown'); + + $this->client->deadLetter($received, $origin.$this->deadLetterSuffix, self::reasonFor($received)); + $this->client->commit($received->deliveryTag); + } - return; + /** + * Why this record is being dead-lettered, in the words the record itself supplies. + * + * A poison record answers with the SHORT class name of the throw that refused its bytes, which is exactly what + * PyFly writes into the same header on the same topics (`type(exc).__name__`): a `.DLT` that both + * frameworks publish to is only readable if `JsonException` means the same thing whichever side wrote it. + * A record that decoded perfectly well and then ran out of retries has no throw to name — the transport never + * saw one, the listener's error strategy did — so it gets the one word that is true of every record on that + * path, and is deliberately NOT a stand-in exception name that would read as a decode failure. + */ + public const string REASON_RETRIES_EXHAUSTED = 'RetriesExhausted'; + + private static function reasonFor(ReceivedEnvelope $received): string + { + $failure = $received->failure; + + if ($failure === null) { + return self::REASON_RETRIES_EXHAUSTED; } - $this->client->deadLetter($envelope, $envelope->destination.$this->deadLetterSuffix); - $this->client->commit($received->deliveryTag); + $class = $failure::class; + $separator = strrpos($class, '\\'); + + return $separator === false ? $class : substr($class, $separator + 1); } public function stop(): void diff --git a/packages/eda-kafka/src/RdKafkaConsumerClient.php b/packages/eda-kafka/src/RdKafkaConsumerClient.php index 894a30ad..51c292ac 100644 --- a/packages/eda-kafka/src/RdKafkaConsumerClient.php +++ b/packages/eda-kafka/src/RdKafkaConsumerClient.php @@ -5,7 +5,6 @@ namespace Firefly\Eda\Kafka; use Firefly\Eda\Consumer\ReceivedEnvelope; -use Firefly\Eda\EventEnvelope; use Firefly\Eda\Serializer; use RdKafka\KafkaConsumer; use RdKafka\Message; @@ -100,24 +99,89 @@ public function commit(mixed $deliveryTag): void $this->consumer()->commit($deliveryTag); } - public function deadLetter(EventEnvelope $envelope, string $dltTopic): void + /** + * Why the record died. PyFly writes the exception's own short class name here (its `type(exc).__name__`), and + * this side spells the header and the value the same way, because a `.DLT` both frameworks write to is + * only readable if one `kcat -f '%h'` explains every record on it. + */ + public const string REASON_HEADER = 'x-dlt-reason'; + + /** The topic the record was CONSUMED from — not the DLT, and not the envelope's declared destination. */ + public const string SOURCE_TOPIC_HEADER = 'x-dlt-source-topic'; + + /** The offset the record sat at, which is the only way back to it in the log. */ + public const string SOURCE_OFFSET_HEADER = 'x-dlt-source-offset'; + + /** + * Re-produces the record to $dltTopic WITH the three provenance headers. + * + * It used to write the bytes and nothing else. dworkers runs LaraFly and PyFly against shared topics, and PyFly + * has stamped `x-dlt-reason`, `x-dlt-source-topic` and `x-dlt-source-offset` on every record it dead-letters + * since `v26.09.06` — so one `.DLT` held two kinds of record: PyFly's, which says why it died and where + * it came from, and this one's, which was raw bytes with no provenance at all. Whoever drained that topic could + * not tell a LaraFly poison record from a replayed payload, and the offset needed to go back and look at the + * original was simply not there. The header names and the reason's spelling are PyFly's, exactly, because + * parity across the two ends of a shared topic is the whole point of writing them. + * + * producev() rather than produce(): headers are the only thing this method gained, and produce() cannot carry + * them. It has been in ext-rdkafka since 3.1 (librdkafka 0.11), well below the `librdkafka >= 1.5.3` this + * package already suggests. + */ + public function deadLetter(ReceivedEnvelope $received, string $dltTopic, string $reason): void { $producer = $this->producer(); $topic = $producer->newTopic($dltTopic); // RD_KAFKA_PARTITION_UA (-1) = librdkafka picks the partition; a dead-lettered record carries no partition // key, so there is no "correct" partition to preserve. - $topic->produce(RD_KAFKA_PARTITION_UA, 0, $this->serializer->serialize($envelope)); + $topic->producev( + RD_KAFKA_PARTITION_UA, + 0, + self::dltPayload($received, $this->serializer), + null, + self::dltHeaders($received, $reason), + ); $producer->flush(2000); } - public function deadLetterRaw(string $raw, string $dltTopic): void + /** + * The bytes the DLT record carries: a poison record's RAW bytes verbatim — so a fixed producer can replay them + * byte for byte, which a re-encoded approximation could not — and the re-serialised envelope for anything else. + * + * Public and static for the same reason received() is: it is the half of deadLetter() that owes nothing to + * ext-rdkafka, and it is the half worth asserting on. + */ + public static function dltPayload(ReceivedEnvelope $received, Serializer $serializer): string { - $producer = $this->producer(); - $topic = $producer->newTopic($dltTopic); + $envelope = $received->envelope; - $topic->produce(RD_KAFKA_PARTITION_UA, 0, $raw); - $producer->flush(2000); + return $envelope === null ? (string) $received->raw : $serializer->serialize($envelope); + } + + /** + * The three provenance headers, from the record alone. + * + * The offset is read off the delivery tag — the rdkafka Message for the real adapter — through property_exists() + * rather than an instanceof, so this stays callable, and testable, on a machine with no ext-rdkafka at all (the + * received() precedent). A header whose value the record cannot answer is LEFT OUT rather than written empty: an + * `x-dlt-source-offset` of `''` reads as an offset, and sends whoever is draining the topic looking for it. + * + * @return array + */ + public static function dltHeaders(ReceivedEnvelope $received, string $reason): array + { + $headers = [self::REASON_HEADER => $reason]; + + if ($received->destination !== null && $received->destination !== '') { + $headers[self::SOURCE_TOPIC_HEADER] = $received->destination; + } + + $tag = $received->deliveryTag; + if (is_object($tag) && property_exists($tag, 'offset') && is_int($tag->offset)) { + $headers[self::SOURCE_OFFSET_HEADER] = (string) $tag->offset; + } + + return $headers; } public function close(): void diff --git a/packages/eda-kafka/tests/Fixtures/FakeKafkaConsumerClient.php b/packages/eda-kafka/tests/Fixtures/FakeKafkaConsumerClient.php index 8a98ded0..d41a04be 100644 --- a/packages/eda-kafka/tests/Fixtures/FakeKafkaConsumerClient.php +++ b/packages/eda-kafka/tests/Fixtures/FakeKafkaConsumerClient.php @@ -5,7 +5,6 @@ namespace Firefly\Eda\Kafka\Tests\Fixtures; use Firefly\Eda\Consumer\ReceivedEnvelope; -use Firefly\Eda\EventEnvelope; use Firefly\Eda\Kafka\KafkaConsumerClient; /** @@ -25,12 +24,9 @@ final class FakeKafkaConsumerClient implements KafkaConsumerClient /** @var list every delivery tag commit() was called with, in order */ public array $committed = []; - /** @var list every [envelope, dltTopic] deadLetter() was called with */ + /** @var list every [record, dltTopic, reason] deadLetter() got */ public array $deadLettered = []; - /** @var list every [raw, dltTopic] deadLetterRaw() was called with */ - public array $deadLetteredRaw = []; - public int $closeCalls = 0; public function subscribe(array $topics): void @@ -48,14 +44,9 @@ public function commit(mixed $deliveryTag): void $this->committed[] = $deliveryTag; } - public function deadLetter(EventEnvelope $envelope, string $dltTopic): void - { - $this->deadLettered[] = [$envelope, $dltTopic]; - } - - public function deadLetterRaw(string $raw, string $dltTopic): void + public function deadLetter(ReceivedEnvelope $received, string $dltTopic, string $reason): void { - $this->deadLetteredRaw[] = [$raw, $dltTopic]; + $this->deadLettered[] = [$received, $dltTopic, $reason]; } public function close(): void diff --git a/packages/eda-kafka/tests/Integration/KafkaRoundTripTest.php b/packages/eda-kafka/tests/Integration/KafkaRoundTripTest.php index 25bfebab..77187d2b 100644 --- a/packages/eda-kafka/tests/Integration/KafkaRoundTripTest.php +++ b/packages/eda-kafka/tests/Integration/KafkaRoundTripTest.php @@ -28,7 +28,9 @@ * must see nothing — Kafka's own redelivery would prove the commit did NOT happen. * * Test 2 proves nack(requeue: false) is Kafka's DEAD-LETTER TOPIC path (no broker-native DLX, unlike RabbitMQ): a - * forced dead-letter re-produces the envelope to ".DLT", verified by consuming that topic directly. + * forced dead-letter re-produces the envelope to ".DLT", verified by consuming that topic directly — with the + * `x-dlt-reason` / `x-dlt-source-topic` / `x-dlt-source-offset` headers PyFly writes on the same topics, which only a + * real broker can prove, because producev() is the one call in this package that no unit test can reach. */ it('round-trips publish -> consume -> handler -> commit against a real Kafka, and commit() prevents redelivery', function () { $brokers = (string) getenv('FIREFLY_KAFKA_BROKERS'); @@ -122,6 +124,17 @@ } expect($dltReceived->envelope->payload['id'] ?? null)->toBe(99); + + // AND THE PROVENANCE, on the real broker, which is the only place the producev() call is exercised at all: the + // three headers PyFly stamps on the records it dead-letters onto these same topics. Read off the rdkafka Message + // the record carries as its delivery tag — the DLT consumer's own poll() decodes the body, not the headers. + $tag = $dltReceived->deliveryTag; + /** @var array $headers */ + $headers = is_object($tag) && property_exists($tag, 'headers') && is_array($tag->headers) ? $tag->headers : []; + + expect($headers['x-dlt-reason'] ?? null)->toBe(KafkaEventConsumer::REASON_RETRIES_EXHAUSTED) + ->and($headers['x-dlt-source-topic'] ?? null)->toBe($topic) + ->and($headers['x-dlt-source-offset'] ?? null)->toMatch('/^\d+$/'); })->skip( ! extension_loaded('rdkafka') || getenv('FIREFLY_KAFKA_BROKERS') === false, 'requires ext-rdkafka + FIREFLY_KAFKA_BROKERS', diff --git a/packages/eda-kafka/tests/KafkaEventConsumerTest.php b/packages/eda-kafka/tests/KafkaEventConsumerTest.php index 42279d7e..221534f1 100644 --- a/packages/eda-kafka/tests/KafkaEventConsumerTest.php +++ b/packages/eda-kafka/tests/KafkaEventConsumerTest.php @@ -57,26 +57,76 @@ function receivedFor(string $eventType = 'order.created', string $destination = $client = new FakeKafkaConsumerClient; $consumer = new KafkaEventConsumer($client); $envelope = new EventEnvelope('order.created', 'order.events', ['id' => 42]); + $received = new ReceivedEnvelope($envelope, 'tag-5'); - $consumer->nack(new ReceivedEnvelope($envelope, 'tag-5'), false); + $consumer->nack($received, false); - expect($client->deadLettered)->toBe([[$envelope, 'order.events.DLT']]) + // A record that decoded perfectly well and then ran out of retries names no exception, because the transport + // never saw one — the listener's error strategy did. + expect($client->deadLettered)->toBe([[$received, 'order.events.DLT', KafkaEventConsumer::REASON_RETRIES_EXHAUSTED]]) ->and($client->committed)->toBe(['tag-5']); }); -it('dead-letters a POISON record\'s raw bytes to ".DLT" then commits, so the loop never re-reads it', function () { +it('dead-letters a POISON record to ".DLT" naming the throw that refused it, then commits', function () { $client = new FakeKafkaConsumerClient; $consumer = new KafkaEventConsumer($client); $poison = ReceivedEnvelope::poison('{"eventId": 1, "no": "shape"}', 'tag-3', new RuntimeException('shape'), 'order.events'); $consumer->nack($poison, false); - // The RAW bytes, not a re-encoded approximation, so a fixed producer can be replayed byte for byte. - expect($client->deadLetteredRaw)->toBe([['{"eventId": 1, "no": "shape"}', 'order.events.DLT']]) - ->and($client->deadLettered)->toBe([]) + // The whole record goes over, so the client still has the RAW bytes (never a re-encoded approximation) AND the + // provenance the DLT needs. The reason is the throw's SHORT class name — `type(exc).__name__` on the PyFly side + // of the same topic. + expect($client->deadLettered)->toBe([[$poison, 'order.events.DLT', 'RuntimeException']]) + ->and($client->deadLettered[0][0]->raw)->toBe('{"eventId": 1, "no": "shape"}') ->and($client->committed)->toBe(['tag-3']); }); +/* + | LARAFLY WROTE A DEAD-LETTER RECORD WITH NO HEADERS AT ALL, AND PYFLY WROTE THREE. + | + | dworkers runs both frameworks against shared topics, so one `.DLT` held two kinds of record: PyFly's, which + | says why it died and where it came from, and LaraFly's, which was raw bytes with no provenance whatsoever. Whoever + | drained that topic could not tell a LaraFly poison record from a replayed payload, and the offset needed to go back + | and look at the original was not there to be read. `grep -rn 'x-dlt' packages/` returned nothing. + | + | The header names and the reason's spelling are PyFly's, exactly (src/pyfly/eda/adapters/kafka.py), because a topic + | both frameworks publish to is only readable if one `kcat -C -t .DLT -f '%h'` explains every record on it. + */ +it('stamps the three provenance headers PyFly stamps, spelled the same way', function () { + $received = new ReceivedEnvelope( + new EventEnvelope('order.created', 'order.events', ['id' => 1]), + // The delivery tag is the rdkafka Message in production; this stands in for it, which is the point of + // reading the offset off a property rather than off an instanceof. + (object) ['offset' => 4207], + destination: 'order.events', + ); + + expect(RdKafkaConsumerClient::dltHeaders($received, 'JsonException'))->toBe([ + 'x-dlt-reason' => 'JsonException', + 'x-dlt-source-topic' => 'order.events', + 'x-dlt-source-offset' => '4207', + ]); +}); + +// An `x-dlt-source-offset` of '' reads as an offset and sends whoever is draining the topic looking for it. A header +// the record cannot answer is left out instead. +it('leaves out a provenance header the record cannot answer rather than writing it empty', function () { + $headers = RdKafkaConsumerClient::dltHeaders(ReceivedEnvelope::poison('bytes', 'a-string-tag', new RuntimeException('x')), 'RuntimeException'); + + expect($headers)->toBe(['x-dlt-reason' => 'RuntimeException']); +}); + +it('re-produces a poison record byte for byte and re-encodes anything else', function () { + $serializer = new JsonSerializer; + $envelope = new EventEnvelope('order.created', 'order.events', ['id' => 7]); + + expect(RdKafkaConsumerClient::dltPayload(ReceivedEnvelope::poison('{"not":"an envelope"}', 'tag', new RuntimeException('x')), $serializer)) + ->toBe('{"not":"an envelope"}') + ->and(RdKafkaConsumerClient::dltPayload(new ReceivedEnvelope($envelope, 'tag'), $serializer)) + ->toBe($serializer->serialize($envelope)); +}); + /* * THE DESERIALISATION IS INSIDE THE RECORD, NOT AROUND THE POLL. RdKafkaConsumerClient::consume() deserialised the * payload on the way out, outside every try/catch in the process, so one malformed body killed the worker. From 18a6c094f705a6d3ec2f628b488ab7347d014b92 Mon Sep 17 00:00:00 2001 From: Andres Contreras Date: Thu, 24 Sep 2026 13:00:57 -0700 Subject: [PATCH 3/3] docs(context): EagerSingletonsPass points at the boundary filter that made its own escape-hatch claim true The guard's comment has always said firefly:cache and firefly:clear are the two commands its skip restores, and the claim was only ever true of the pass it is written in: a deleted #[Component] that implemented an interface breaks the boot in ContainerRegistrar instead, and the throw comes out of a bean that still exists. Say where the check actually lives now, and what this one is still for. --- packages/context/src/Pass/EagerSingletonsPass.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/context/src/Pass/EagerSingletonsPass.php b/packages/context/src/Pass/EagerSingletonsPass.php index 1a0dce6e..c25c7112 100644 --- a/packages/context/src/Pass/EagerSingletonsPass.php +++ b/packages/context/src/Pass/EagerSingletonsPass.php @@ -79,6 +79,16 @@ public function run(BootContext $context): void // constructor, an unsatisfiable dependency, the registrar's own NoUniqueBeanDefinition guard — // still propagates, because those are real defects in code that does exist and failing fast at // boot is exactly right for them. + // + // THIS GUARD IS NO LONGER THE ONLY THING STANDING BETWEEN A DEVELOPER AND `rm -rf + // bootstrap/cache/firefly`, and the escape hatch this comment claims was never true while it + // was. It protects the pass it is written in; a deleted #[Component] that implemented an + // interface breaks the boot somewhere else entirely, because ContainerRegistrar binds the + // INTERFACE to the missing class and the throw comes out of resolving a bean that still + // exists. BeanDefinitionRegistry::add() now drops a stale definition at the door while + // firefly:cache or firefly:clear is running, so the manifest this pass reads has none in it. + // What remains here is depth: under every other command the definition is still present, and + // skipping it is still the only defensible answer for THIS pass. $context->container->make($abstract); } }