fix: retry helper re-register while job is missing

CHANGELOG.md

@@ -34,7 +34,7 @@ All notable changes to `secure-agent` are documented here. The format follows
 - Menu bar: the popover opens in front of fullscreen apps on every Space.
 - Console: the polling fallback keeps refreshing every 2 s, posture shows stale after two minutes without a refresh, and review changes refresh the console.
 - Console: an older worktree read no longer overwrites a newer one, and markup escapes single quotes.
-- File telemetry starts when the collector appears after launch, restarts its helper from the new build after an update, its repair no longer leaves the helper unregistered, the app re-registers a helper launchd dropped when the app was replaced in place, and a restarted daemon resumes the spool where its last run stopped instead of losing the backlog (counting the spool as lost when that position rotated away while it was stopped), and retries within milliseconds when the event bus is full instead of falling behind heavy file activity; an oversized spool line is skipped and counted as lost instead of dropping the unread tail.
+- File telemetry starts when the collector appears after launch, restarts its helper from the new build after an update, its repair no longer leaves the helper unregistered, the app re-registers a helper launchd dropped when the app was replaced in place, retrying while the job stays missing, and a restarted daemon resumes the spool where its last run stopped instead of losing the backlog (counting the spool as lost when that position rotated away while it was stopped), and retries within milliseconds when the event bus is full instead of falling behind heavy file activity; an oversized spool line is skipped and counted as lost instead of dropping the unread tail.
 - Doctor: session identity counts sessions started since boot, so old unnamed sessions no longer fail it; spool loss no longer also fails Collectors as a silent eslogger; Storage passes once its recovered write failures are 10 minutes old or marked seen.
 - Probes: `lsof` and `launchctl` calls time out instead of hanging.
 - Retention: prune failures show in write health, and plugin actions keep 50,000 rows like the tool calls they mirror, so a day of agent work no longer fails Doctor's retention check.

menubar/Sources/SecureAgentMenubar/SetupManager.swift

@@ -127,6 +127,11 @@ public final class SetupManager: ObservableObject {
     /// and when the last one started.
     private(set) var esRepairProbe: Task<Void, Never>?
     private var esRepairProbeStarted: Date?
+    /// Seconds between launchd probes; tests shorten it.
+    var esRepairProbeInterval: TimeInterval = 30
+    /// Re-registers this launch for an enabled service launchd held no job
+    /// for (bounded by `ESAutopilot.maxMissingJobRepairs`).
+    private var esMissingJobRepairs = 0
     /// The 1 s poll while a System Settings switch is pending.
     private var esPollTask: Task<Void, Never>?
     /// Telemetry Doctor results, in check order.
@@ -723,13 +728,16 @@ public final class SetupManager: ObservableObject {
         }
     }
 
-    /// Re-registers once per launch when the service is enabled but launchd
-    /// will not start its job. The launchd probe runs off the main actor, one
-    /// at a time, at most every 30 s.
+    /// Re-registers when the service is enabled but launchd will not start its
+    /// job: once per launch for a refused or stale job, and up to
+    /// `ESAutopilot.maxMissingJobRepairs` times while launchd holds no job,
+    /// since this launch's own registration can leave it that way. The
+    /// launchd probe runs off the main actor, one at a time, at most every
+    /// `esRepairProbeInterval`.
     private func repairStuckESJob() {
-        guard esLocationAllowed, esRepairProbe == nil, !esRegisterAttemptedThisLaunch, !esUserDisabled,
-              esServiceStatus == .enabled,
-              esRepairProbeStarted.map({ Date().timeIntervalSince($0) >= 30 }) ?? true else { return }
+        guard esLocationAllowed, esRepairProbe == nil, !esUserDisabled, esServiceStatus == .enabled,
+              !esRegisterAttemptedThisLaunch || esMissingJobRepairs < ESAutopilot.maxMissingJobRepairs,
+              esRepairProbeStarted.map({ Date().timeIntervalSince($0) >= esRepairProbeInterval }) ?? true else { return }
         esRepairProbeStarted = Date()
         let probe = esLaunchdProbe
         let staleProbe = esHelperStaleProbe
@@ -739,13 +747,17 @@ public final class SetupManager: ObservableObject {
                 return (job, staleProbe(job))
             }.value
             guard let self else { return }
-            self.esRepairProbe = nil
             if ESAutopilot.needsReregister(serviceStatus: self.esService.status,
                                            userDisabled: self.esUserDisabled,
                                            attemptedThisLaunch: self.esRegisterAttemptedThisLaunch,
-                                           launchd: job, helperStale: stale) {
+                                           launchd: job, helperStale: stale,
+                                           missingJobRepairs: self.esMissingJobRepairs) {
+                if job == .notFound { self.esMissingJobRepairs += 1 }
                 await self.reregisterESService()
             }
+            // Cleared after the re-register: the refresh it runs must not
+            // start a second probe while this one is still repairing.
+            self.esRepairProbe = nil
         }
     }
 

menubar/Sources/SecureAgentMenubar/TelemetryDoctor.swift

@@ -35,26 +35,30 @@ enum ESAutopilot {
         }
     }
 
+    /// Re-registers per launch for an enabled service launchd holds no job
+    /// for: the registration that replaces a job still being torn down (an
+    /// app replaced in place, its old helper running) can leave none, and the
+    /// next one loads it.
+    static let maxMissingJobRepairs = 3
+
     /// Whether to unregister and register the service again: it is enabled
-    /// and not removed by the user, and either its running process predates
-    /// the helper binary on disk, launchd holds no job for it (an app bundle
-    /// replaced in place: launchd drops the job when the bundle it was
-    /// registered from moves away, while the registration still reads
-    /// enabled), or launchd holds its job with no pid and last refused to
-    /// spawn it (EX_CONFIG, e.g. a registration bound to a bundle path that no
-    /// longer exists). Once per launch.
+    /// and not removed by the user, and either launchd holds no job for it
+    /// (an app bundle replaced in place: launchd drops the job when the
+    /// bundle it was registered from moves away, while the registration still
+    /// reads enabled), up to `maxMissingJobRepairs` times a launch whatever
+    /// this launch already tried; or, once per launch, its running process
+    /// predates the helper binary on disk, or launchd holds its job with no
+    /// pid and last refused to spawn it (EX_CONFIG, e.g. a registration bound
+    /// to a bundle path that no longer exists).
     static func needsReregister(serviceStatus: SMAppService.Status, userDisabled: Bool,
-                                attemptedThisLaunch: Bool, launchd: LaunchdProbe, helperStale: Bool) -> Bool {
-        guard !userDisabled, !attemptedThisLaunch, serviceStatus == .enabled else { return false }
+                                attemptedThisLaunch: Bool, launchd: LaunchdProbe, helperStale: Bool,
+                                missingJobRepairs: Int = 0) -> Bool {
+        guard !userDisabled, serviceStatus == .enabled else { return false }
+        if launchd == .notFound { return missingJobRepairs < maxMissingJobRepairs }
+        guard !attemptedThisLaunch else { return false }
         if helperStale { return true }
-        switch launchd {
-        case .notFound:
-            return true
-        case let .loaded(_, pid, lastExit):
-            return pid == nil && TelemetryDoctor.exitCode(lastExit) == TelemetryDoctor.exConfig
-        case .unreadable:
-            return false
-        }
+        guard case let .loaded(_, pid, lastExit) = launchd, pid == nil else { return false }
+        return TelemetryDoctor.exitCode(lastExit) == TelemetryDoctor.exConfig
     }
 
     /// Opened by `secure-agent telemetry repair`: re-register the service now,

menubar/Tests/SecureAgentMenubarTests/ESAutopilotTests.swift

@@ -162,7 +162,7 @@ final class ESAutopilotTests: XCTestCase {
             (.enabled, false, false, .loaded(state: "waiting", pid: nil, lastExitCode: "(never exited)"), false, false, "never exited"),
             (.requiresApproval, false, false, refused, false, false, "not enabled"),
             (.enabled, false, false, .notFound, false, true, "enabled with no job: the app bundle was replaced in place"),
-            (.enabled, false, true, .notFound, false, false, "no job, already attempted this launch"),
+            (.enabled, false, true, .notFound, false, true, "no job after this launch's own attempt: retried"),
             (.enabled, true, false, .notFound, false, false, "no job, removed by the user"),
             (.requiresApproval, false, false, .notFound, false, false, "no job, not enabled"),
             (.enabled, false, false, .unreadable, false, false, "unreadable job"),
@@ -176,6 +176,44 @@ final class ESAutopilotTests: XCTestCase {
                                                        attemptedThisLaunch: attempted, launchd: job,
                                                        helperStale: stale), want, why)
         }
+        XCTAssertFalse(ESAutopilot.needsReregister(serviceStatus: .enabled, userDisabled: false, attemptedThisLaunch: true,
+                                                   launchd: .notFound, helperStale: false,
+                                                   missingJobRepairs: ESAutopilot.maxMissingJobRepairs),
+                       "a missing job is re-registered a bounded number of times a launch")
+    }
+
+    /// An app replaced in place leaves the service enabled with no launchd
+    /// job, and the launch's own re-register of the old helper can leave it
+    /// that way: the repair keeps re-registering while the job stays missing,
+    /// up to its bound, and leaves a loaded job alone.
+    @MainActor func testRefreshReregistersAMissingJobUntilLaunchdHoldsIt() async throws {
+        try XCTSkipIf(FileManager.default.fileExists(atPath: SetupManager.legacyESPlistPath),
+                      "an old helper on this Mac makes the stage legacyInstalled")
+        let service = FakeESService()
+        service.status = .enabled
+        service.registerStatus = .enabled
+        let job = JobBox(.notFound)
+        let defaults = try XCTUnwrap(UserDefaults(suiteName: "ESAutopilotTests.\(UUID().uuidString)"))
+        let setup = SetupManager(esService: service, defaults: defaults, configurationHome: temporaryHome(), plistPresent: { true }, openPane: { _ in },
+                                 launchdProbe: { job.value }, helperStaleProbe: { _ in false },
+                                 appURL: installedCopy)
+        setup.esRepairProbeInterval = 0
+        let probeOnce = { () async in
+            setup.refreshESState()
+            if let probe = setup.esRepairProbe { await probe.value }
+        }
+
+        await probeOnce()
+        await probeOnce()
+        XCTAssertEqual(service.registerCount, 2, "re-registered on each probe that finds no job")
+
+        job.value = .loaded(state: "running", pid: 600, lastExitCode: nil)
+        await probeOnce()
+        XCTAssertEqual(service.registerCount, 2, "a loaded job is left alone")
+
+        job.value = .notFound
+        for _ in 0..<5 { await probeOnce() }
+        XCTAssertEqual(service.registerCount, ESAutopilot.maxMissingJobRepairs, "bounded per launch")
     }
 
     func testOnlyTheExactRepairURLTriggers() throws {
@@ -299,12 +337,25 @@ final class ESAutopilotTests: XCTestCase {
 
 private let installedCopy = URL(fileURLWithPath: AppIdentity.installedAppPath)
 
+/// The launchd job a test's probe reports; the probe reads it off the main actor.
+final class JobBox: @unchecked Sendable {
+    private let lock = NSLock()
+    private var job: LaunchdProbe
+    init(_ job: LaunchdProbe) { self.job = job }
+    var value: LaunchdProbe {
+        get { lock.lock(); defer { lock.unlock() }; return job }
+        set { lock.lock(); job = newValue; lock.unlock() }
+    }
+}
+
 /// Stands in for SMAppService: register moves to requiresApproval, as a
 /// first registration does on macOS.
 final class FakeESService: ESServiceControl {
     var registerCount = 0
     var unregisterCount = 0
     var registerError: Error?
+    /// The status a register leaves: an approved service stays `.enabled`.
+    var registerStatus: SMAppService.Status = .requiresApproval
     /// Reads of `status` that still answer `.enabled` after an unregister, as
     /// macOS applies it late; a register before they run out fails.
     var unregisterLag = 0
@@ -326,7 +377,7 @@ final class FakeESService: ESServiceControl {
         registerCount += 1
         if lagLeft > 0 { throw NSError(domain: "SMAppServiceErrorDomain", code: 1) }
         if let registerError { throw registerError }
-        stored = .requiresApproval
+        stored = registerStatus
     }
 
     func unregister() throws {