diff --git a/CHANGELOG.md b/CHANGELOG.md index 7832849f..343fa939 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,84 @@ All notable changes to LaraFly are documented here. This project uses CalVer (`Y ## [Unreleased] +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 + +- **`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. + ## [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/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/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/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); } } 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/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. 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)