diff --git a/app/Support/Readiness/OperationChecks.php b/app/Support/Readiness/OperationChecks.php index 8d01d4e..93ef7ee 100644 --- a/app/Support/Readiness/OperationChecks.php +++ b/app/Support/Readiness/OperationChecks.php @@ -30,6 +30,19 @@ final class OperationChecks */ private const STALE_AFTER_MINUTES = 5; + /** + * Fix-Runde (Befund 3): a heartbeat this far into the future is as + * untrustworthy as one from the past — a clock skewed between the web, + * scheduler and worker containers, or a corrupted value that still + * happens to parse. Without an upper bound, `now()->addHour()` written + * once by a confused clock would read as healthy forever, since it would + * never age past STALE_AFTER_MINUTES from a `greaterThan()`-only check. + * One minute is generous slack for the same reason five is above: + * everything that writes these timestamps runs on the same host clock, + * so more than a minute of "future" is a fault, not jitter. + */ + private const FUTURE_TOLERANCE_MINUTES = 1; + /** @return array */ public static function all(): array { @@ -40,7 +53,14 @@ final class OperationChecks severity: Check::SEVERITY_BLOCKING, label: __('readiness.operation.scheduler'), breaks: __('readiness.operation.scheduler_breaks'), - tab: 'integrations', + // Fix-Runde (Befund 4): 'integrations' is not a member of + // Integrations::TABS (['services', 'platform', 'env']) — a + // link to it would go nowhere. 'env' is where saveEnv() lives, + // and saveEnv() is the actual remedy: it restarts the + // scheduler and queue-provisioning workers (see its own + // docblock), which is exactly what an operator would do about + // either check in this group. + tab: 'env', satisfied: self::isFresh('heartbeat.scheduler'), ), new Check( @@ -49,7 +69,7 @@ final class OperationChecks severity: Check::SEVERITY_BLOCKING, label: __('readiness.operation.queue_provisioning'), breaks: __('readiness.operation.queue_provisioning_breaks'), - tab: 'integrations', + tab: 'env', // Same fix as above; same remedy (saveEnv()). // Written by RecordProvisioningHeartbeat, which the scheduler // enqueues but only the worker actually runs. A fresh // timestamp here is proof of a running worker, not just of a @@ -67,12 +87,34 @@ final class OperationChecks return false; } + // Fix-Runde (Befund 2): reproduced live — a garbled value in the + // heartbeat row (never written by RecordProvisioningHeartbeat or the + // scheduler call themselves, but reachable by anyone editing + // app_settings directly) made Carbon::parse() throw, which propagated + // out of Readiness::all() and took the ENTIRE readiness page down + // with it, not just this one check. A page whose only job is to + // report calmly what is missing must not join the outage itself. An + // unreadable value is treated exactly like a missing one: not + // satisfied, nothing thrown. + try { + $parsed = Carbon::parse($recordedAt); + } catch (\Throwable) { + return false; + } + // Not diffInMinutes(): App\Models\Incident::durationMinutes() already // documents that it is SIGNED, and `now()->diffInMinutes($recordedAt)` // with $recordedAt in the past returns a NEGATIVE number — which // would compare as "less than 5" for a heartbeat six hours stale, and // report a dead worker as healthy. A direct threshold comparison has // no sign to get backwards. - return Carbon::parse($recordedAt)->greaterThan(now()->subMinutes(self::STALE_AFTER_MINUTES)); + // + // Bounded on both sides (Befund 3): stale in the past beyond + // STALE_AFTER_MINUTES, but ALSO not more than FUTURE_TOLERANCE_MINUTES + // ahead of now — otherwise a heartbeat from a skewed clock would read + // as healthy indefinitely, since "in the future" always satisfies a + // one-sided "not older than five minutes" comparison. + return $parsed->greaterThan(now()->subMinutes(self::STALE_AFTER_MINUTES)) + && $parsed->lessThanOrEqualTo(now()->addMinutes(self::FUTURE_TOLERANCE_MINUTES)); } } diff --git a/tests/Feature/Readiness/HeartbeatTest.php b/tests/Feature/Readiness/HeartbeatTest.php index 5635697..b5da3e1 100644 --- a/tests/Feature/Readiness/HeartbeatTest.php +++ b/tests/Feature/Readiness/HeartbeatTest.php @@ -4,6 +4,7 @@ use App\Provisioning\Jobs\RecordProvisioningHeartbeat; use App\Support\Readiness; use App\Support\Readiness\Check; use App\Support\Settings; +use Illuminate\Support\Facades\Queue; function operationCheck(string $key): ?Check { @@ -48,3 +49,48 @@ it('records the worker heartbeat when the job runs', function () { expect(operationCheck('operation.queue_provisioning')->satisfied)->toBeTrue(); }); + +/** + * Fix-Runde (Befund 1): calling ->handle() directly, as every test above + * does, proves only that the job writes its value — never that it is the + * PROVISIONING worker doing it. That one property is the entire reason a + * second heartbeat exists at all: if this job ever migrated onto the default + * queue, this check would keep reporting a live provisioning worker while the + * queue that actually matters sat unattended — the exact silent failure this + * task was built to catch. + */ +it('is pushed onto the provisioning queue, not the default one', function () { + Queue::fake(); + + RecordProvisioningHeartbeat::dispatch(); + + Queue::assertPushedOn('provisioning', RecordProvisioningHeartbeat::class); +}); + +/** + * Fix-Runde (Befund 2): reproduced live by the reviewer — a garbled value in + * the heartbeat row made Carbon::parse() throw, which propagated straight out + * of Readiness::all(). Not just this one check failing: the whole readiness + * page, whose only purpose is to calmly report what is missing, going down + * with it. Unreachable through any real write path today, which is the + * argument FOR guarding it cheaply, not against. + */ +it('treats an unreadable heartbeat as not satisfied, without taking the whole page down', function () { + Settings::set('heartbeat.scheduler', 'not-a-timestamp'); + + $checks = Readiness::all(); + + expect(collect($checks)->firstWhere('key', 'operation.scheduler')->satisfied)->toBeFalse(); +}); + +/** + * Fix-Runde (Befund 3): isFresh() had no upper bound — a heartbeat with a + * timestamp in the future (a clock skewed between containers, or a corrupted + * value that still happens to parse) would report permanent health for as + * long as nobody noticed the clock was wrong. + */ +it('does not treat a heartbeat from the future as healthy', function () { + Settings::set('heartbeat.scheduler', now()->addHour()->toIso8601String()); + + expect(operationCheck('operation.scheduler')->satisfied)->toBeFalse(); +});