Merge nucleic/upbeat-opal-viper-cc8l into main

Sources/RunnerHost/Doctor.swift

@@ -92,7 +92,11 @@ public enum Doctor {
     ///    same verb the real download uses, because the presigned redirect
     ///    target is signed per method. Catches a version bump that no longer
     ///    has a darwin-arm64 asset.
-    /// 10. **Local Network privacy**. Passes when a subnet allowlist is set in
+    /// 10. **Guest SSH**, against whichever slot currently holds a DHCP lease —
+    ///     the one check that exercises host → vmnet → guest `sshd` → password
+    ///     auth end to end. Informational when no guest is up, since `doctor`
+    ///     will not boot one.
+    /// 11. **Local Network privacy**. Passes when a subnet allowlist is set in
     ///     `com.apple.network.local-network`; otherwise informational. On
     ///     macOS 15+ the first attempt to reach a guest over the NAT link can
     ///     be blocked by the Local Network permission prompt, which a
@@ -107,6 +111,7 @@ public enum Doctor {
         checks.append(checkLoginKeychain())
         checks.append(contentsOf: await checkGitea(config: config))
         checks.append(await checkRunnerDownloadURL(config: config))
+        checks.append(await checkGuestSSH(config: config))
         checks.append(localNetworkNote())
         return checks
     }
@@ -148,6 +153,7 @@ public enum Doctor {
         checks.append(checkLoginKeychain())
         checks.append(contentsOf: await checkGitea(config: loaded))
         checks.append(await checkRunnerDownloadURL(config: loaded))
+        checks.append(await checkGuestSSH(config: loaded))
         checks.append(contentsOf: checkTokenFilePermissions(config: loaded))
         checks.append(localNetworkNote())
         return checks
@@ -523,6 +529,129 @@ public enum Doctor {
         ]
     }
 
+    /// Whether a guest that is up right now actually accepts an SSH session.
+    ///
+    /// Every other check in this file inspects the host. This one exercises the
+    /// exact path the boot sequence depends on and that nothing else proves:
+    /// host → vmnet → guest `sshd` → password auth with `guest.username` /
+    /// `guest.password`. It is the difference between "the daemon never got a
+    /// runner online" and a named cause — wrong credentials, Local Network
+    /// privacy blocking the link, or a guest image whose Remote Login is off.
+    ///
+    /// Read-only with respect to host state: it uses the MACs already persisted
+    /// in `state.json` and never generates them, so running `doctor` on a fresh
+    /// host does not quietly create the slot address table.
+    ///
+    /// - Note: Informational when no guest is currently leased. `doctor` must not
+    ///   boot a VM — that costs minutes and a slot out of the host's hard cap of
+    ///   two — so with nothing running there is simply nothing to probe. To make
+    ///   this check meaningful, leave a guest up (`gitea-macos-runner vm boot`)
+    ///   and run `doctor` again.
+    public static func checkGuestSSH(config: RunnerConfig) async -> DoctorCheck {
+        let name = "guest ssh"
+
+        let macs: [String]
+        do {
+            macs = try VMStore(config: config).loadState().slotMACAddresses
+        } catch {
+            return DoctorCheck(
+                name: name,
+                result: .warn,
+                detail: "could not read host state: \(error)",
+                remediation: "check that \(config.storeDirectoryURL.path) is readable"
+            )
+        }
+
+        guard !macs.isEmpty else {
+            return DoctorCheck(
+                name: name,
+                result: .info,
+                detail: "no slot MAC addresses assigned yet; skipped",
+                remediation: nil
+            )
+        }
+
+        // Newest lease wins if a slot somehow holds more than one: that is the
+        // guest currently on the link.
+        let leases = DHCPLeaseParser.parseFile()
+        guard let (mac, lease) = macs.lazy
+            .compactMap({ mac in DHCPLeaseParser.lease(forMAC: mac, in: leases).map { (mac, $0) } })
+            .first
+        else {
+            return DoctorCheck(
+                name: name,
+                result: .info,
+                detail: "no guest currently holds a DHCP lease; skipped",
+                remediation: """
+                    this check only runs against a guest that is already up. To exercise the \
+                    host→guest SSH path, run `gitea-macos-runner vm boot --image default` and \
+                    then `doctor` again.
+                    """
+            )
+        }
+
+        // Short and fixed rather than derived from `scheduler.bootTimeoutSeconds`:
+        // this probes a guest that is already booted, so a slow answer is a
+        // finding, not something to wait fifteen minutes for.
+        do {
+            try await waitForSSH(
+                host: lease.ipAddress,
+                username: config.guest.username,
+                password: config.guest.password,
+                timeout: .seconds(20),
+                pollInterval: .seconds(2)
+            )
+            return DoctorCheck(
+                name: name,
+                result: .pass,
+                detail: "authenticated to \(config.guest.username)@\(lease.ipAddress) (\(mac))"
+            )
+        } catch let error as CoreError {
+            let detail = "\(config.guest.username)@\(lease.ipAddress) (\(mac)): \(error)"
+            switch error {
+            case .timeout:
+                // Nothing answered. bootpd leases last 24 hours and the slot MACs
+                // are persistent, so on any host that has ever run the daemon the
+                // most likely explanation is a lease outliving the guest that held
+                // it — not a broken host. Calling that `.fail` would make `doctor`
+                // cry wolf on a perfectly healthy idle machine.
+                return DoctorCheck(
+                    name: name,
+                    result: .warn,
+                    detail: detail,
+                    remediation: """
+                        most likely a stale lease: bootpd keeps leases for 24 hours, so this \
+                        address may belong to a guest that has already been torn down. If a \
+                        guest really is up at this address, the daemon cannot reach it either — \
+                        check that the host's Local Network permission is not dropping the \
+                        connection (see the "local network access" check).
+                        """
+                )
+            default:
+                // Authentication reached the guest and was refused: the guest is up
+                // and the credentials are wrong. Nothing about that improves on its
+                // own, and every boot will fail the same way.
+                return DoctorCheck(
+                    name: name,
+                    result: .fail,
+                    detail: detail,
+                    remediation: """
+                        the daemon authenticates over this exact path, so no boot can succeed \
+                        while it fails. Check that guest.username and guest.password match an \
+                        account in the guest image, and that Remote Login is enabled there.
+                        """
+                )
+            }
+        } catch {
+            return DoctorCheck(
+                name: name,
+                result: .warn,
+                detail: "\(config.guest.username)@\(lease.ipAddress) (\(mac)): \(error)",
+                remediation: nil
+            )
+        }
+    }
+
     /// The macOS 15+ Local Network permission note.
     ///
     /// Reports `.pass` when the host carries a subnet allowlist that actually

Sources/RunnerHost/Orchestrator.swift

@@ -50,7 +50,7 @@ public struct LiveVM: Sendable {
 ///
 /// `ensureFreeSpace` → `cloneImage(named:slotMAC:)` → ``VMInstance/start(options:)``
 /// → poll `/var/db/dhcpd_leases` for the slot MAC until `bootTimeout` →
-/// ``waitForSSH(host:port:username:password:timeout:pollInterval:)`` → write the
+/// ``waitForSSH(host:port:username:password:timeout:pollInterval:reportInterval:onAttemptFailure:)`` → write the
 /// registration token into a guest file with mode `0600` → over SSH:
 ///
 /// ```sh
@@ -105,6 +105,15 @@ public actor Orchestrator {
     /// The supervising task per slot: clone → boot → register → run → teardown.
     private var slotTasks: [Int: Task<Void, Never>] = [:]
 
+    /// Why a slot's supervising task was cancelled, left for that task to find.
+    ///
+    /// `Task.cancel()` carries no payload and `CancellationError` no detail, so
+    /// a lifecycle that catches one knows only *that* it was stopped. Everything
+    /// worth reading — "boot timeout: provisioning for 312s (limit 300s)" — is
+    /// known only to the canceller. Without this hand-off the operator sees
+    /// `reason=cancelled`, which names the mechanism and hides the cause.
+    private var slotCancelReasons: [Int: String] = [:]
+
     /// The "the guest stopped on its own" watcher per slot.
     private var deathWatchTasks: [Int: Task<Void, Never>] = [:]
 
@@ -252,6 +261,7 @@ public actor Orchestrator {
         // let them run their own teardown; whatever they miss we clean up below.
         let tasks = slotTasks
         slotTasks.removeAll()
+        for (slot, _) in tasks { slotCancelReasons[slot] = "shutting down" }
         for (_, task) in tasks { task.cancel() }
         for (_, task) in tasks { await task.value }
 
@@ -307,6 +317,9 @@ public actor Orchestrator {
                 // are about to kill, so it would otherwise clear that entry only
                 // after the boot had already been refused.
                 if let task = slotTasks.removeValue(forKey: slot) {
+                    // Before the cancel, not after: the task may reach its
+                    // `catch` the instant it is cancelled.
+                    slotCancelReasons[slot] = reason
                     task.cancel()
                     // Its own teardown runs to completion here, which also means
                     // it cannot race a successor booted later in this pass.
@@ -404,6 +417,9 @@ public actor Orchestrator {
 
         let generation = (slotGeneration[slot] ?? 0) + 1
         slotGeneration[slot] = generation
+        // Taken alongside the `Date()` handed to the planner, so the lifecycle
+        // can measure against the same deadline the planner will enforce.
+        let provisioningStarted = ContinuousClock.now
         state = SchedulerCore.markProvisioning(state: state, slot: slot, jobHint: jobHint, now: Date())
 
         logger.info(
@@ -417,7 +433,13 @@ public actor Orchestrator {
 
         slotTasks[slot] = Task { [weak self] in
             guard let self else { return }
-            await self.runSlotLifecycle(slot: slot, jobHint: jobHint, runnerName: runnerName, generation: generation)
+            await self.runSlotLifecycle(
+                slot: slot,
+                jobHint: jobHint,
+                runnerName: runnerName,
+                generation: generation,
+                provisioningStarted: provisioningStarted
+            )
         }
     }
 
@@ -425,11 +447,40 @@ public actor Orchestrator {
     ///
     /// Every failure path funnels into the same teardown, because a slot that is
     /// neither live nor idle is a slot leaked for the process's lifetime.
-    private func runSlotLifecycle(slot: Int, jobHint: Int64, runnerName: String, generation: Int) async {
+    ///
+    /// - Parameter provisioningStarted: When the *planner's* boot deadline began
+    ///   ticking — earlier than this function's own first instruction. Used to
+    ///   budget the readiness waits against the deadline that will actually be
+    ///   enforced rather than against a fresh copy of it.
+    private func runSlotLifecycle(
+        slot: Int,
+        jobHint: Int64,
+        runnerName: String,
+        generation: Int,
+        provisioningStarted: ContinuousClock.Instant
+    ) async {
         let bootTimeout = Duration.seconds(max(30, config.scheduler.bootTimeoutSeconds))
         let jobTimeout = Duration.seconds(max(60, config.scheduler.jobTimeoutMinutes * 60))
         var teardownReason = "job finished"
 
+        // Both readiness waits below are already supervised by the planner's
+        // boot deadline, which started ticking at `provisioningStarted` — before
+        // the clone, the boot and the DHCP lease had spent any of it. Handing
+        // either wait the full `bootTimeout` puts its deadline strictly *after*
+        // the planner's, so the planner always wins the race: this task is
+        // cancelled mid-wait and the specific error the wait was about to throw
+        // ("ssh on 192.168.65.233:22 after 41 attempts; last error: …") is
+        // discarded in favour of a bare cancellation. Budget from what is left
+        // and the wait gets to speak first.
+        func remainingBootBudget() -> Duration {
+            let spent = ContinuousClock.now - provisioningStarted
+            // Landing a little before the planner, so its next tick finds the
+            // slot already failing for a stated reason. The floor keeps an
+            // already-overrun budget from collapsing to zero attempts, which
+            // would trade one useless message for another.
+            return max(.seconds(15), bootTimeout - spent - .seconds(5))
+        }
+
         do {
             let mac = try store.macAddress(forSlot: slot, slotCount: slotCount)
             // Whatever lease this MAC already holds belongs to the *previous*
@@ -454,15 +505,40 @@ public actor Orchestrator {
                 await self?.vmStoppedUnexpectedly(slot: slot, generation: generation, reason: reason)
             }
 
-            let ip = try await waitForLease(mac: mac, timeout: bootTimeout, replacing: priorLease)
+            let ip = try await waitForLease(
+                mac: mac,
+                timeout: remainingBootBudget(),
+                replacing: priorLease
+            )
             live[slot]?.ipAddress = ip
             logger.info("guest leased address", metadata: ["slot": .stringConvertible(slot), "ip": .string(ip)])
 
+            // A `Logger` is a value type, so the callback below gets its own
+            // copy and never touches the actor — which is what lets it be a
+            // plain synchronous closure called from inside the poll loop.
+            let log = logger
             try await waitForSSH(
                 host: ip,
                 username: config.guest.username,
                 password: config.guest.password,
-                timeout: bootTimeout
+                timeout: remainingBootBudget(),
+                onAttemptFailure: { attempt in
+                    // A guest sharing a host with other Virtualization guests
+                    // can take minutes to start `sshd`. Without this the wait is
+                    // indistinguishable from a hang: the log goes quiet between
+                    // "guest leased address" and teardown, which is exactly the
+                    // window an operator most wants to see into.
+                    log.info(
+                        "waiting for guest ssh",
+                        metadata: [
+                            "slot": .stringConvertible(slot),
+                            "host": .string(ip),
+                            "attempt": .stringConvertible(attempt.attempt),
+                            "elapsed": .string("\(attempt.elapsed.components.seconds)s"),
+                            "error": .string(attempt.error),
+                        ]
+                    )
+                }
             )
 
             let token = try await registrationToken()
@@ -511,7 +587,9 @@ public actor Orchestrator {
                 )
             }
         } catch is CancellationError {
-            teardownReason = "cancelled"
+            // Whoever cancelled us knows why; `CancellationError` does not.
+            // Falling back to "cancelled" only when nobody left a note.
+            teardownReason = slotCancelReasons[slot] ?? "cancelled"
         } catch {
             teardownReason = "\(error)"
             logger.error(
@@ -539,6 +617,10 @@ public actor Orchestrator {
             state = SchedulerCore.releaseJob(state: state, jobID: jobHint)
         }
 
+        // A note left for a cancel that arrived after the lifecycle had already
+        // finished on its own would otherwise be read by the *next* occupant of
+        // this slot, mislabelling its teardown.
+        slotCancelReasons[slot] = nil
         slotTasks[slot] = nil
     }
 
@@ -551,6 +633,7 @@ public actor Orchestrator {
             "guest stopped unexpectedly",
             metadata: ["slot": .stringConvertible(slot), "reason": .string("\(reason)")]
         )
+        slotCancelReasons[slot] = "guest stopped: \(reason)"
         slotTasks[slot]?.cancel()
         await teardownSlot(slot, reason: "guest stopped: \(reason)")
     }