Fix round: guard the parse, bound the future, name the real tab

Covers the queue assignment the second heartbeat exists to prove, stops an
unreadable heartbeat value from taking the whole readiness page down with it,
rejects a heartbeat dated into the future, and points both checks at the tab
that actually restarts the scheduler and provisioning worker.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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<int, Check> */
     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));
     }
 }

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();
+});