diff --git a/CHANGELOG.md b/CHANGELOG.md index a6b643c..6402c4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.6.1] - 2026-08-30 + +### Fixed +- **Essential modules never reached a queued job.** `HttpPipeline` passed its + essentials into `OnDemandLoader`; `WorkerLoop` built its loader with none, and + `Kernel::materialize()` had no way to hand them over. So a module the project + declared app-wide in `proj.json` `"essentials"` was app-wide for requests and + **absent from every job** — and for an essential that rebinds a port per scope + (tenancy rebinding `DatabasePort`) the failure is silent rather than loud: the + binding still resolves, just to the wrong connection. The worker now registers + essentials into every job container and seeds their domains into the job's + graph, so their transitive `requires[]` come with them — the same two steps + `LoadStage` performs for a request. A job whose class the manifest does not + know now also gets a container rather than the bare `CoreContainer`, since + "essential" means every unit of work; an application that declares no + essentials keeps its exact previous behaviour, including that fallback. + The class→domain mapping both surfaces need moved to + `DependencyGraphCalculator::domainsFor()`; a private copy in each pipeline is + how they drifted apart in the first place. +- **`APP_DEBUG` meant two different things in one file.** `ErrorStage::isDebug()` + parsed the value with `FILTER_VALIDATE_BOOL` while `publicError()` compared it + `=== 'true'`. With `APP_DEBUG=1` the HTML debug page — stack trace and source + excerpt — was served to anything sending `Accept: text/html`, while every JSON + response still masked its message as "An internal error occurred.". One flag, + two behaviours, and the more revealing of the two was the one that engaged. + There is now one `isDebug()`, used by both; `FILTER_VALIDATE_BOOL` is the + surviving parse because it is what every other kernel flag uses + (`HttpPipeline::flag()`), so `1`, `on`, `yes` and `true` mean the same thing + throughout. It also now reads through `env()` rather than `$_ENV`/`getenv()`: + the environment loader deliberately skips `putenv()`, so `getenv()` is not the + source of truth for a `.env` value. **Note the direction of the change** — with + `APP_DEBUG=1` the JSON path now reveals exception messages, which is what the + flag was asked for; `APP_DEBUG` unset or falsy masks them exactly as before. + ## [1.6.0] - 2026-08-29 ### Added diff --git a/src/Kernel/Kernel.php b/src/Kernel/Kernel.php index d94ddfe..7f386fa 100644 --- a/src/Kernel/Kernel.php +++ b/src/Kernel/Kernel.php @@ -539,6 +539,11 @@ private function materialize(RuntimeMode $mode): void $errorPipeline, $this->workerPipe, $this->workerSecret ?? (string) (env('JOB_SIGNING_SECRET') ?: ''), + // Essentials reach the worker for the same reason they reach HTTP: a + // job is a request with no HTTP in front of it, and a module the + // project declared app-wide should not quietly stop existing at the + // queue boundary. + essentialModules: $this->essentialModules, ); // Configuration compiled by CompileConfigManifestStage during build(). diff --git a/src/Kernel/Loading/DependencyGraphCalculator.php b/src/Kernel/Loading/DependencyGraphCalculator.php index 7917cb7..2e8edfa 100644 --- a/src/Kernel/Loading/DependencyGraphCalculator.php +++ b/src/Kernel/Loading/DependencyGraphCalculator.php @@ -12,6 +12,43 @@ public function __construct( private readonly array $manifest ) {} + /** + * The solves domains of a set of provider classes, read from a compiled + * service manifest. + * + * Essential modules are configured as CLASSES but seeded into a graph as + * DOMAINS, and resolving them through the graph (rather than only registering + * them standalone) is what brings their transitive requires[] with them — an + * essential like Tenancy gets its Database dependency bound instead of failing + * with an unbound contract. + * + * Lives here, not on a pipeline, because BOTH the HTTP and worker surfaces + * need the same mapping and a private copy in each is how they drifted apart + * in the first place. A class absent from the manifest (not in withModules) + * is skipped — OnDemandLoader still registers it standalone. + * + * @param array{services?: array>} $manifest + * @param list $moduleClasses + * @return list + */ + public static function domainsFor(array $manifest, array $moduleClasses): array + { + if ($moduleClasses === []) { + return []; + } + + $wanted = array_flip($moduleClasses); + $domains = []; + + foreach ($manifest['services'] ?? [] as $domain => $entry) { + if (isset($wanted[$entry['module'] ?? ''])) { + $domains[] = $domain; + } + } + + return $domains; + } + /** * Resolve the dependency graph for a service, optionally seeding extra * module domains the matched route declared in its own requires[]. diff --git a/src/Kernel/Pipelines/Http/HttpPipeline.php b/src/Kernel/Pipelines/Http/HttpPipeline.php index 483c6ab..6fab708 100644 --- a/src/Kernel/Pipelines/Http/HttpPipeline.php +++ b/src/Kernel/Pipelines/Http/HttpPipeline.php @@ -129,7 +129,11 @@ private function buildStages(): array new SecurityStage($this->gateway), ...$this->resolveHook('after.security'), new ResolveStage($this->matcher, self::flag('ROUTE_METHOD_NOT_ALLOWED', false)), - new LoadStage($this->calculator, $this->loader, self::essentialDomains($manifest, $this->essentialModules)), + new LoadStage( + $this->calculator, + $this->loader, + DependencyGraphCalculator::domainsFor($manifest, $this->essentialModules), + ), ...$this->resolveHook('after.load'), new RouteFilterStage($this->filters, $this->core), new ExecuteStage(), @@ -263,31 +267,4 @@ private static function policy(): string }; } - /** - * Map the essential module classes to their solves domains via the compiled - * service manifest, so LoadStage can seed them (and thus their transitive - * requires) into every request's dependency graph. An essential not present - * in the manifest (not in withModules) is skipped — OnDemandLoader still - * registers it standalone, preserving the previous behaviour. - * - * @param array{services?: array>} $manifest - * @param list $essentialModules - * @return list - */ - private static function essentialDomains(array $manifest, array $essentialModules): array - { - if ($essentialModules === []) { - return []; - } - - $wanted = array_flip($essentialModules); - $domains = []; - foreach ($manifest['services'] ?? [] as $domain => $entry) { - if (isset($wanted[$entry['module'] ?? ''])) { - $domains[] = $domain; - } - } - - return $domains; - } } diff --git a/src/Kernel/Pipelines/Http/Stages/ErrorStage.php b/src/Kernel/Pipelines/Http/Stages/ErrorStage.php index 19d2162..02ac907 100644 --- a/src/Kernel/Pipelines/Http/Stages/ErrorStage.php +++ b/src/Kernel/Pipelines/Http/Stages/ErrorStage.php @@ -58,9 +58,32 @@ private function buildErrorResponse(\Throwable $e, Request $request): Response return Response::json(['error' => $body], $status); } + /** + * Whether the application is in debug mode — the ONE place this is decided. + * + * It used to be decided twice, and differently: this method parsed the value + * with FILTER_VALIDATE_BOOL while publicError() compared it `=== 'true'`. So + * APP_DEBUG=1 enabled the HTML debug page — stack trace and source excerpt — + * for anything sending `Accept: text/html`, while every JSON response still + * masked its message as "An internal error occurred.". One flag, two + * behaviours, and the more revealing of the two was the one that turned on. + * + * FILTER_VALIDATE_BOOL is the surviving parse because it is what every other + * kernel flag uses (see HttpPipeline::flag()), so `1`, `on`, `yes` and `true` + * all mean the same thing across the kernel. + * + * Read through env(), not $_ENV/getenv(): the environment loader deliberately + * skips putenv(), so getenv() is not the source of truth for a .env value. + */ private function isDebug(): bool { - return filter_var($_ENV['APP_DEBUG'] ?? getenv('APP_DEBUG') ?: 'false', FILTER_VALIDATE_BOOL); + $value = \function_exists('env') ? env('APP_DEBUG') : ($_ENV['APP_DEBUG'] ?? null); + + if ($value === null || $value === '') { + return false; + } + + return filter_var($value, FILTER_VALIDATE_BOOL, FILTER_NULL_ON_FAILURE) ?? false; } /** API surface convention — same prefix the CSRF layer exempts. */ @@ -101,7 +124,7 @@ private function resolveHttpCode(\Throwable $e): int /** @return array */ private function publicError(\Throwable $e): array { - $debug = ($_ENV['APP_DEBUG'] ?? getenv('APP_DEBUG') ?? 'false') === 'true'; + $debug = $this->isDebug(); if ($e instanceof ValidationException) { return ['code' => 'validation_failed', 'message' => $e->getMessage(), 'fields' => $e->errors]; diff --git a/src/Kernel/Pipelines/Worker/WorkerLoop.php b/src/Kernel/Pipelines/Worker/WorkerLoop.php index 41dd5a0..28fd185 100644 --- a/src/Kernel/Pipelines/Worker/WorkerLoop.php +++ b/src/Kernel/Pipelines/Worker/WorkerLoop.php @@ -45,6 +45,14 @@ final class WorkerLoop private ?DependencyGraphCalculator $calculator = null; private ?OnDemandLoader $loader = null; + /** + * Solves domains of $essentialModules, mapped through the service manifest + * once (alongside the calculator) rather than per job. + * + * @var list + */ + private array $essentialDomains = []; + /** * Per-job retry strategies, built once from the compiled manifest. * @@ -73,6 +81,20 @@ public function __construct( private readonly string $signingSecret = '', /** Backoff applied by release() when a job declares no retry of its own. */ private readonly RetryStrategyContract $retry = new ExponentialRetryStrategy(), + /** + * Provider classes marked ESSENTIAL — registered into every job's + * container regardless of the job's own dependency graph, exactly as + * HttpPipeline does for every request. + * + * Without this the two surfaces disagreed about what "essential" means: + * a module declared essential in proj.json was app-wide for requests and + * absent from every job. For an essential that rebinds a port per scope + * (tenancy rebinding DatabasePort) the failure is silent rather than + * loud — the binding still resolves, just to the wrong connection. + * + * @var list> + */ + private readonly array $essentialModules = [], ) { } @@ -401,28 +423,49 @@ private function process(JobPayload $payload): JobResult } /** - * Build a request-scoped ModuleContainer for the job's domain. - * Returns null if the job's domain cannot be resolved — falls back to CoreContainer. + * Build a request-scoped ModuleContainer for the job's domain, plus every + * essential module — the same world an HTTP request is given. + * + * Returns null only when there is nothing to build: no job domain AND no + * essentials. That preserves the historical CoreContainer fallback for an + * application that declares no essentials, while an application that does + * declare them gets them on a job whose class the manifest does not know — + * which is exactly the case "essential" is supposed to cover. + * * Worker jobs always receive a guest Identity (no HTTP request context). */ private function resolveContainer(string $jobName): ?ModuleContainer { // First real job: load the manifests now (not at construction time). $this->jobManifest ??= $this->loadManifest('job-manifest.php', []); - $this->calculator ??= new DependencyGraphCalculator( - $this->loadManifest('service-manifest.php', ['services' => []]) - ); - $this->loader ??= new OnDemandLoader($this->core); + + if ($this->calculator === null) { + $manifest = $this->loadManifest('service-manifest.php', ['services' => []]); + $this->calculator = new DependencyGraphCalculator($manifest); + // Essentials are configured as CLASSES and seeded as DOMAINS; the + // mapping needs the service manifest, so it is resolved once here + // rather than on every job. + $this->essentialDomains = DependencyGraphCalculator::domainsFor($manifest, $this->essentialModules); + } + + // Passing the essentials to the LOADER registers them; seeding their + // DOMAINS into the graph below additionally brings their transitive + // requires[]. LoadStage does both, for the same reason. + $this->loader ??= new OnDemandLoader($this->core, $this->essentialModules); $entry = $this->jobManifest[$jobName] ?? null; $domain = $entry['solves'] ?? null; - if ($domain === null) { + $targets = $domain !== null + ? [$domain, ...$this->essentialDomains] + : $this->essentialDomains; + + if ($targets === []) { return null; } try { - $graph = $this->calculator->resolve($domain); + $graph = $this->calculator->resolve($targets[0], array_slice($targets, 1)); return $this->loader->loadWithIdentity($graph, null); // guest Identity for worker context } catch (\Throwable) { return null; // fall back to CoreContainer if graph resolution fails diff --git a/tests/Unit/Kernel/Pipelines/Http/ErrorStageDebugTest.php b/tests/Unit/Kernel/Pipelines/Http/ErrorStageDebugTest.php new file mode 100644 index 0000000..57dfa4b --- /dev/null +++ b/tests/Unit/Kernel/Pipelines/Http/ErrorStageDebugTest.php @@ -0,0 +1,145 @@ + */ + private array $previousEnv = []; + + protected function setUp(): void + { + $this->previousEnv = [ + 'env' => $_ENV['APP_DEBUG'] ?? null, + 'server' => $_SERVER['APP_DEBUG'] ?? null, + ]; + } + + protected function tearDown(): void + { + foreach (['env' => '_ENV', 'server' => '_SERVER'] as $key => $global) { + if ($this->previousEnv[$key] === null) { + unset($GLOBALS[$global]['APP_DEBUG']); + } else { + $GLOBALS[$global]['APP_DEBUG'] = $this->previousEnv[$key]; + } + } + } + + /** + * Every spelling FILTER_VALIDATE_BOOL accepts must reach the JSON body, not + * just the literal string 'true'. + */ + public function test_every_truthy_spelling_reveals_the_message(): void + { + foreach (['1', 'true', 'on', 'yes', 'TRUE'] as $spelling) { + $body = $this->jsonErrorBody($spelling); + + self::assertSame( + self::MESSAGE, + $body['error']['message'], + "APP_DEBUG={$spelling} must reveal the message on the JSON path, " + . 'as it already does on the HTML debug page', + ); + } + } + + public function test_debug_off_still_masks_the_message(): void + { + foreach (['0', 'false', 'off', 'no', ''] as $spelling) { + $body = $this->jsonErrorBody($spelling); + + self::assertSame( + 'An internal error occurred.', + $body['error']['message'], + "APP_DEBUG={$spelling} must not leak the message", + ); + } + } + + public function test_an_absent_flag_masks_the_message(): void + { + unset($_ENV['APP_DEBUG'], $_SERVER['APP_DEBUG']); + + $body = $this->decode($this->dispatch()); + + self::assertSame('An internal error occurred.', $body['error']['message']); + } + + public function test_the_error_envelope_keeps_its_shape(): void + { + $response = $this->dispatch('1'); + $body = $this->decode($response); + + self::assertSame(500, $response->status()); + self::assertArrayHasKey('code', $body['error']); + self::assertArrayHasKey('requestId', $body['error']); + self::assertSame('trace-42', $body['error']['requestId']); + } + + // ── harness ───────────────────────────────────────────────────────────── + + /** @return array */ + private function jsonErrorBody(string $appDebug): array + { + return $this->decode($this->dispatch($appDebug)); + } + + /** + * Drive the stage over a throwing $next on an /api path, which forces the + * structured JSON body rather than the HTML debug page. + */ + private function dispatch(?string $appDebug = null): Response + { + if ($appDebug === null) { + unset($_ENV['APP_DEBUG'], $_SERVER['APP_DEBUG']); + } else { + $_ENV['APP_DEBUG'] = $appDebug; + $_SERVER['APP_DEBUG'] = $appDebug; + } + + $request = Request::create('/api/invoices', 'GET') + ->withAttribute('correlation_id', 'trace-42'); + + // notifiers([]) has no fallback, so consume() writes nothing to disk. + $stage = new ErrorStage(ErrorPipeline::notifiers([])); + + return $stage->handle($request, static function (): Response { + throw new \RuntimeException(self::MESSAGE); + }); + } + + /** @return array */ + private function decode(Response $response): array + { + /** @var array $decoded */ + $decoded = json_decode((string) $response->getContent(), true); + + return $decoded; + } +} diff --git a/tests/Unit/Kernel/Pipelines/Worker/WorkerLoopEssentialsTest.php b/tests/Unit/Kernel/Pipelines/Worker/WorkerLoopEssentialsTest.php new file mode 100644 index 0000000..5f8afce --- /dev/null +++ b/tests/Unit/Kernel/Pipelines/Worker/WorkerLoopEssentialsTest.php @@ -0,0 +1,267 @@ +root = sys_get_temp_dir() . '/hkm-worker-essentials-' . bin2hex(random_bytes(6)); + mkdir($this->root . '/var/cache/manifests', 0775, true); + + $this->previousBase = Paths::base(); + $this->previousProject = Paths::project(); + Paths::setBase($this->root); + Paths::setProject($this->root); + + EssentialFixtureProvider::$registered = 0; + EssentialSpyJob::$handled = 0; + } + + protected function tearDown(): void + { + Paths::setBase((string) $this->previousBase); + Paths::setProject($this->previousProject); + + foreach (glob($this->root . '/var/cache/manifests/*') ?: [] as $file) { + @unlink($file); + } + @rmdir($this->root . '/var/cache/manifests'); + @rmdir($this->root . '/var/cache'); + @rmdir($this->root . '/var'); + @rmdir($this->root); + } + + // ── The fix ───────────────────────────────────────────────────────────── + + public function test_an_essential_module_registers_for_a_queued_job(): void + { + $this->writeManifests(); + + $this->runOnce([EssentialFixtureProvider::class]); + + self::assertSame(1, EssentialSpyJob::$handled, 'the job must still run'); + self::assertSame( + 1, + EssentialFixtureProvider::$registered, + 'an essential module must register into the job container, as it does for every request', + ); + } + + public function test_an_essential_registers_even_when_the_job_has_no_manifest_entry(): void + { + // A job the manifest does not know used to fall back to the CoreContainer + // with no module container at all. "Essential" means every unit of work, + // including this one. + $this->writeManifests(withJobEntry: false); + + $this->runOnce([EssentialFixtureProvider::class]); + + self::assertSame(1, EssentialSpyJob::$handled); + self::assertSame(1, EssentialFixtureProvider::$registered); + } + + // ── And it stays opt-in ───────────────────────────────────────────────── + + public function test_a_module_that_is_not_essential_does_not_register(): void + { + // The loop must not register every module in the service manifest — only + // the job's own graph plus what the project declared essential. + $this->writeManifests(); + + $this->runOnce(essentials: []); + + self::assertSame(1, EssentialSpyJob::$handled); + self::assertSame(0, EssentialFixtureProvider::$registered); + } + + // ── harness ───────────────────────────────────────────────────────────── + + /** + * @param list> $essentials + */ + private function runOnce(array $essentials): void + { + $core = new CoreContainer(); + $queue = new EssentialQueue([self::payload()]); + $core->instance(QueuePort::class, $queue); + + $loop = new WorkerLoop( + $core, + ErrorPipeline::notifiers([]), + new WorkerPipeline(), + '', + essentialModules: $essentials, + ); + + $loop->run(maxIterations: 1); + } + + /** + * A service manifest naming both the job's own domain and the essential's, + * plus (optionally) the job manifest entry that maps the job to its domain. + */ + private function writeManifests(bool $withJobEntry = true): void + { + $this->writeManifest('service-manifest.php', [ + 'services' => [ + 'job.testing' => [ + 'module' => JobFixtureProvider::class, + 'requires' => [], + ], + 'essential.testing' => [ + 'module' => EssentialFixtureProvider::class, + 'requires' => [], + ], + ], + ]); + + $this->writeManifest('job-manifest.php', $withJobEntry ? [ + EssentialSpyJob::class => [ + 'handler' => EssentialSpyJob::class, + 'queue' => 'default', + 'module' => JobFixtureProvider::class, + 'solves' => 'job.testing', + ], + ] : []); + } + + /** @param array $data */ + private function writeManifest(string $file, array $data): void + { + file_put_contents( + $this->root . '/var/cache/manifests/' . $file, + ' */ + public function requires(): array { return []; } + + /** @return list */ + public function exposes(): array { return []; } + + public function register(ModuleContainer $container): void + { + self::$registered++; + } + + public function boot(HttpPipeline $http, CliPipeline $cli, WorkerPipeline $worker, EventBus $events): void + { + } +} + +/** The module that owns the job's own domain. */ +final class JobFixtureProvider implements ModuleContract +{ + public function solves(): string { return 'job.testing'; } + + /** @return list */ + public function requires(): array { return []; } + + /** @return list */ + public function exposes(): array { return []; } + + public function register(ModuleContainer $container): void + { + } + + public function boot(HttpPipeline $http, CliPipeline $cli, WorkerPipeline $worker, EventBus $events): void + { + } +} + +final class EssentialSpyJob implements JobContract +{ + public static int $handled = 0; + + public function handle(JobPayload $payload): JobResult + { + self::$handled++; + + return JobResult::success(); + } + + public function failed(JobPayload $payload, \Throwable $e): void + { + } +} + +/** Minimal QueuePort that yields a fixed list of payloads once. */ +final class EssentialQueue implements QueuePort +{ + /** @var list */ + public array $calls = []; + + /** @param list $pending */ + public function __construct(private array $pending = []) {} + + public function push(string $jobClass, array $payload, string $queue = 'default', int $delay = 0): string { return 'id'; } + public function later(int $seconds, string $jobClass, array $payload, string $queue = 'default'): string { return 'id'; } + public function size(string $queue = 'default'): int { return count($this->pending); } + + public function pop(string $queue = 'default'): ?JobPayload + { + return array_shift($this->pending); + } + + public function ack(JobPayload $payload): void { $this->calls[] = 'ack'; } + public function release(JobPayload $payload, int $delay = 0): void { $this->calls[] = 'release'; } + public function fail(JobPayload $payload, ?\Throwable $reason = null): void { $this->calls[] = 'fail'; } +}