diff --git a/Sources/KeyPathApp/Info.plist b/Sources/KeyPathApp/Info.plist index 5dbafacfc..3c7b2321b 100644 --- a/Sources/KeyPathApp/Info.plist +++ b/Sources/KeyPathApp/Info.plist @@ -11,7 +11,7 @@ CFBundleDisplayName KeyPath CFBundleVersion - 11 + 12 CFBundleShortVersionString 1.0.1 CFBundlePackageType diff --git a/Sources/KeyPathAppKit/CLI/SystemFacade.swift b/Sources/KeyPathAppKit/CLI/SystemFacade.swift index d47bdad37..52f788595 100644 --- a/Sources/KeyPathAppKit/CLI/SystemFacade.swift +++ b/Sources/KeyPathAppKit/CLI/SystemFacade.swift @@ -21,13 +21,13 @@ public struct SystemFacade: Sendable { public init() { self.init( startServiceOperation: { - try await HelperManager.shared.startKanataService() + try await KanataDaemonService.shared.start() }, stopServiceOperation: { - try await HelperManager.shared.stopKanataService() + try await KanataDaemonService.shared.stop() }, restartServiceOperation: { - try await HelperManager.shared.restartKanataService() + try await KanataDaemonService.shared.restart() }, runtimeCacheInvalidator: { await MainActor.run { @@ -110,8 +110,8 @@ public struct SystemFacade: Sendable { try await restartServiceOperation() await runtimeCacheInvalidator() - // The helper performs one launchctl kickstart -k against the fixed - // launchd job. Verify the final healthy runtime, not a transient gap. + // The lifecycle operation verifies registration plus process/TCP + // readiness. Independently verify the facade's final runtime view. return await waitForRuntime(timeoutSeconds: runtimeTransitionTimeoutSeconds) { snapshot in snapshot.isRunning && snapshot.isResponding } diff --git a/Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift b/Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift index 549ec9289..ed39d0e75 100644 --- a/Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift +++ b/Sources/KeyPathAppKit/Services/Kanata/KanataDaemonService.swift @@ -1,14 +1,21 @@ import Foundation import KeyPathCore import KeyPathDaemonLifecycle +import KeyPathInstallationWizard import ServiceManagement /// Errors related to recovery-daemon operations. enum KanataDaemonServiceError: LocalizedError, Equatable { + case approvalRequired + case startFailed(reason: String) case stopFailed(reason: String) var errorDescription: String? { switch self { + case .approvalRequired: + "Starting Kanata requires approval in System Settings." + case let .startFailed(reason): + "Failed to start Kanata service: \(reason)" case let .stopFailed(reason): "Failed to stop Kanata service: \(reason)" } @@ -47,6 +54,9 @@ final class KanataDaemonService { // whatever is listening on the machine. DEBUG-only โ€” production always probes. #if DEBUG nonisolated(unsafe) static var tcpProbeOverride: ((Int, Int) -> Bool)? + nonisolated(unsafe) static var processRunningOverride: (() async -> Bool)? + nonisolated(unsafe) static var runningPostconditionOverride: (() async -> Bool)? + nonisolated(unsafe) static var privilegedStopOverride: (() async throws -> Void)? #endif // MARK: - Internal Dependencies (Hidden from consumers) @@ -67,7 +77,9 @@ final class KanataDaemonService { case unknown var isRunning: Bool { - if case .running = self { return true } + if case .running = self { + return true + } return false } @@ -103,14 +115,105 @@ final class KanataDaemonService { // Drop the centralized status cache so the next read re-fetches. await SystemStateProvider.shared.invalidateSMAppServiceStatus(plistName: Constants.daemonPlistName) } catch { - if TestEnvironment.isRunningTests { - AppLogger.shared.log("๐Ÿงช [KanataDaemonService] Ignoring unregister error in tests: \(error)") - return - } throw KanataDaemonServiceError.stopFailed(reason: error.localizedDescription) } } + private func registerDaemon() async throws { + let service = makeSMService() + do { + try service.register() + await SystemStateProvider.shared.invalidateSMAppServiceStatus(plistName: Constants.daemonPlistName) + } catch { + throw KanataDaemonServiceError.startFailed(reason: error.localizedDescription) + } + } + + private func currentRegistrationStatus() async -> SMAppService.Status { + await SystemStateProvider.shared + .freshSMAppServiceStatus(for: Constants.daemonPlistName) + } + + private func processIsRunning() async -> Bool { + #if DEBUG + if let override = Self.processRunningOverride { + return await override() + } + #endif + + await pidCache.invalidateCache() + let processState = await detectProcessState() + return processState.isRunning + } + + private func waitForRunningPostcondition( + maxAttempts: Int = 80, + delayMilliseconds: Int = 250 + ) async -> Bool { + #if DEBUG + if let override = Self.runningPostconditionOverride { + return await override() + } + #endif + + for attempt in 1 ... maxAttempts { + if await processIsRunning() { + let tcpPort = PreferencesService.shared.tcpServerPort + let tcpAlive = await SystemStateProvider.shared + .isTCPPortResponding(port: tcpPort, timeoutMs: 300) + if tcpAlive { + return true + } + } + if attempt < maxAttempts { + try? await Task.sleep(for: .milliseconds(delayMilliseconds)) + } + } + return false + } + + private enum StoppedPostcondition: Equatable { + case satisfied + case registrationPresent + case processRunning + } + + private func waitForStoppedPostcondition( + // The launchd plist allows a five-second exit timeout. Give normal + // SMAppService shutdown slightly longer before escalating to the helper. + maxAttempts: Int = 60, + delayMilliseconds: Int = 100 + ) async -> StoppedPostcondition { + // Registration is correctness-critical but SMAppService.status is slow + // synchronous IPC. Fetch it once per unregister phase, then poll only the + // inexpensive launchd-backed process evidence while removal settles. + let status = await currentRegistrationStatus() + guard status == .notRegistered || status == .notFound else { + return .registrationPresent + } + + for attempt in 1 ... maxAttempts { + if await !processIsRunning() { + return .satisfied + } + if attempt < maxAttempts { + try? await Task.sleep(for: .milliseconds(delayMilliseconds)) + } + } + return .processRunning + } + + private func forceStopStaleDaemon() async throws { + #if DEBUG + if let override = Self.privilegedStopOverride { + try await override() + return + } + #endif + + try await PrivilegeBroker().stopKanataDaemonService() + } + private func detectProcessState() async -> ProcessSnapshot { if let daemonPID = await pidCache.getCachedPID() { return ProcessSnapshot(isRunning: true, pid: Int(daemonPID)) @@ -141,37 +244,110 @@ final class KanataDaemonService { // MARK: - Public API + /// Start the service through its owning SMAppService registration. + func start() async throws { + AppLogger.shared.log("โ–ถ๏ธ [KanataDaemonService] Start requested") + + let finalRegistrationStatus: SMAppService.Status + switch await currentRegistrationStatus() { + case .enabled: + finalRegistrationStatus = .enabled + case .requiresApproval: + throw KanataDaemonServiceError.approvalRequired + case .notRegistered, .notFound: + // Registration can race with an IPC error. The fresh status below + // is authoritative, so do not fail before observing the result. + try? await registerDaemon() + finalRegistrationStatus = await currentRegistrationStatus() + @unknown default: + throw KanataDaemonServiceError.startFailed(reason: "Unknown SMAppService registration state") + } + + switch finalRegistrationStatus { + case .enabled: + break + case .requiresApproval: + throw KanataDaemonServiceError.approvalRequired + case .notRegistered, .notFound: + throw KanataDaemonServiceError.startFailed(reason: "Registration did not persist") + @unknown default: + throw KanataDaemonServiceError.startFailed(reason: "Unknown SMAppService registration state") + } + + guard await waitForRunningPostcondition() else { + throw KanataDaemonServiceError.startFailed( + reason: "Service registered but did not reach process and TCP readiness" + ) + } + + await pidCache.invalidateCache() + lastObservedState = await refreshStatus() + AppLogger.shared.info("โœ… [KanataDaemonService] Started successfully") + } + /// Stop the service func stop() async throws { AppLogger.shared.log("๐Ÿ›‘ [KanataDaemonService] Stop requested") - try await unregisterDaemon() - - // Verify cleanup - try? await Task.sleep(for: .milliseconds(200)) // 0.2s - try? PIDFileManager.removePID() - await pidCache.invalidateCache() - let refreshedStatus = await refreshStatus() + // Treat the mutation result as evidence, not the verdict. A transient + // SMAppService error can race with a successful state change; the real + // registration/process postcondition below decides whether to retry. + try? await unregisterDaemon() + + // SMAppService removes its launchd job asynchronously. Across an app + // replacement, macOS can leave the prior bundle's registration alive + // after the first unregister request. Verify the real postcondition, + // retry the owning API once, then use the existing privileged bootout + // path only if the stale job still survives. + var stoppedPostcondition = await waitForStoppedPostcondition() + if stoppedPostcondition == .registrationPresent { + AppLogger.shared.warn( + "โš ๏ธ [KanataDaemonService] Job survived unregister; retrying SMAppService removal" + ) + try? await unregisterDaemon() + stoppedPostcondition = await waitForStoppedPostcondition() + } - if case .running = refreshedStatus { - if TestEnvironment.isRunningTests { - AppLogger.shared.log("๐Ÿงช [KanataDaemonService] Test environment stop fallback - marking service as stopped") - lastObservedState = .stopped - } else { - // If still running, it might be a zombie or external process - AppLogger.shared.warn("โš ๏ธ [KanataDaemonService] Service still running after stop request") - throw KanataDaemonServiceError.stopFailed(reason: "Process failed to terminate") + if stoppedPostcondition != .satisfied { + AppLogger.shared.warn( + "โš ๏ธ [KanataDaemonService] Stale job survived SMAppService retry; using privileged cleanup" + ) + // Like SMAppService mutations, helper delivery can report an error + // after changing state. The final postcondition remains authoritative. + try? await forceStopStaleDaemon() + + // A bootout can stop the process, but only the owning API can remove + // a still-enabled registration and prevent a later KeepAlive respawn. + if stoppedPostcondition == .registrationPresent { + try? await unregisterDaemon() } + stoppedPostcondition = await waitForStoppedPostcondition(maxAttempts: 20) } - if lastObservedState != .stopped { - AppLogger.shared.log("โ„น๏ธ [KanataDaemonService] Forcing state to stopped after successful stop") - lastObservedState = .stopped + guard stoppedPostcondition == .satisfied else { + throw KanataDaemonServiceError.stopFailed( + reason: "Service remained registered or running after stale-job cleanup" + ) } + try? PIDFileManager.removePID() + await pidCache.invalidateCache() + lastObservedState = .stopped + AppLogger.shared.info("โœ… [KanataDaemonService] Stopped successfully") } + /// Restart by removing the SMAppService registration before registering it + /// again. This is the supported way to stop a loaded KeepAlive job; disabling + /// or signaling the launchd label alone does not suppress an existing job's + /// KeepAlive respawn. + func restart() async throws { + AppLogger.shared.log("๐Ÿ”„ [KanataDaemonService] Restart requested") + try await stop() + try await start() + AppLogger.shared.info("โœ… [KanataDaemonService] Restart requested successfully") + } + /// Returns whether the internal recovery daemon is currently active. func isDaemonRunning() async -> Bool { let status = await refreshStatus() diff --git a/Tests/KeyPathTests/CLI/CLIServiceTests.swift b/Tests/KeyPathTests/CLI/CLIServiceTests.swift index 9763e9e66..a9847a1b9 100644 --- a/Tests/KeyPathTests/CLI/CLIServiceTests.swift +++ b/Tests/KeyPathTests/CLI/CLIServiceTests.swift @@ -28,7 +28,7 @@ final class CLIServiceTests: XCTestCase { // MARK: - service lifecycle - func testStopServiceReturnsFalseWhenPrivilegedHelperFails() async { + func testStopServiceReturnsFalseWhenLifecycleOperationFails() async { let facade = SystemFacade( stopServiceOperation: { throw ServiceOperationError.failed }, runtimeSnapshotProvider: { Self.runtimeSnapshot(running: true, responding: true) }, @@ -41,7 +41,7 @@ final class CLIServiceTests: XCTestCase { XCTAssertFalse(stopped) } - func testStopServiceWaitsForStoppedRuntimeAfterHelperSuccess() async { + func testStopServiceWaitsForStoppedRuntimeAfterLifecycleSuccess() async { let snapshots = RuntimeSnapshotSequence([ Self.runtimeSnapshot(running: true, responding: true), Self.runtimeSnapshot(running: false, responding: false) @@ -62,7 +62,7 @@ final class CLIServiceTests: XCTestCase { XCTAssertEqual(stopCount, 1) } - func testRestartServiceDoesNotReportSuccessWhenPrivilegedHelperFails() async { + func testRestartServiceDoesNotReportSuccessWhenLifecycleOperationFails() async { let operations = ServiceOperationRecorder() let facade = SystemFacade( restartServiceOperation: { @@ -81,7 +81,7 @@ final class CLIServiceTests: XCTestCase { XCTAssertEqual(restartCount, 1) } - func testRestartServiceWaitsForHealthyRuntimeAfterHelperSuccess() async { + func testRestartServiceWaitsForHealthyRuntimeAfterLifecycleSuccess() async { let snapshots = RuntimeSnapshotSequence([ Self.runtimeSnapshot(running: false, responding: false), Self.runtimeSnapshot(running: true, responding: true) @@ -117,7 +117,7 @@ final class CLIServiceTests: XCTestCase { XCTAssertFalse(started) } - func testStartServiceWaitsForHealthyRuntimeAfterHelperSuccess() async { + func testStartServiceWaitsForHealthyRuntimeAfterLifecycleSuccess() async { let snapshots = RuntimeSnapshotSequence([ Self.runtimeSnapshot(running: false, responding: false), Self.runtimeSnapshot(running: true, responding: true) diff --git a/Tests/KeyPathTests/Services/KanataDaemonServiceIntegrationTests.swift b/Tests/KeyPathTests/Services/KanataDaemonServiceIntegrationTests.swift index 25c92126c..d090b0a3c 100644 --- a/Tests/KeyPathTests/Services/KanataDaemonServiceIntegrationTests.swift +++ b/Tests/KeyPathTests/Services/KanataDaemonServiceIntegrationTests.swift @@ -5,9 +5,18 @@ import ServiceManagement /// Mock implementation of SMAppServiceProtocol for testing private class MockSMAppService: SMAppServiceProtocol, @unchecked Sendable { + enum MockError: Error { + case registerFailed + case unregisterFailed + } + var status: SMAppService.Status var registerCalled = false var unregisterCalled = false + var calls: [String] = [] + var statusesAfterUnregister: [SMAppService.Status] = [] + var failingUnregisterCalls: Set = [] + var statusBeforeRegisterError: SMAppService.Status? init(status: SMAppService.Status = .notRegistered) { self.status = status @@ -15,6 +24,11 @@ private class MockSMAppService: SMAppServiceProtocol, @unchecked Sendable { func register() throws { registerCalled = true + calls.append("register") + if let statusBeforeRegisterError { + status = statusBeforeRegisterError + throw MockError.registerFailed + } // Simulate successful registration transition if status == .notRegistered || status == .notFound { status = .enabled @@ -23,7 +37,13 @@ private class MockSMAppService: SMAppServiceProtocol, @unchecked Sendable { func unregister() async throws { unregisterCalled = true - status = .notRegistered + calls.append("unregister") + if failingUnregisterCalls.contains(calls.count(where: { $0 == "unregister" })) { + throw MockError.unregisterFailed + } + status = statusesAfterUnregister.isEmpty + ? .notRegistered + : statusesAfterUnregister.removeFirst() } } @@ -38,14 +58,18 @@ final class KanataDaemonServiceIntegrationTests: KeyPathAsyncTestCase { /// Point the centralized status provider (#853) at the same status the service's /// factory would report, with a zero TTL so each refresh re-reads. `evaluateStatus` /// now sources status from the provider rather than the service's own factory. - private func useStatus(_ status: SMAppService.Status) { - KanataDaemonService.smServiceFactory = { _ in MockSMAppService(status: status) } + private func useService(_ service: MockSMAppService) { + KanataDaemonService.smServiceFactory = { _ in service } SMAppServiceStatusProvider.shared = SMAppServiceStatusProvider( cacheTTL: 0, - serviceFactory: { _ in MockSMAppService(status: status) } + serviceFactory: { _ in service } ) } + private func useStatus(_ status: SMAppService.Status) { + useService(MockSMAppService(status: status)) + } + override func setUp() async throws { try await super.setUp() @@ -58,6 +82,8 @@ final class KanataDaemonServiceIntegrationTests: KeyPathAsyncTestCase { // runner is a dev Mac with a real kanata listening on the default port, which // would otherwise make the probe succeed and contaminate these status tests. KanataDaemonService.tcpProbeOverride = { _, _ in false } + KanataDaemonService.processRunningOverride = { false } + KanataDaemonService.runningPostconditionOverride = { true } // 2. Create Service under test service = KanataDaemonService() @@ -67,6 +93,9 @@ final class KanataDaemonServiceIntegrationTests: KeyPathAsyncTestCase { KanataDaemonService.smServiceFactory = originalFactory SMAppServiceStatusProvider.shared = originalStatusProvider KanataDaemonService.tcpProbeOverride = nil + KanataDaemonService.processRunningOverride = nil + KanataDaemonService.runningPostconditionOverride = nil + KanataDaemonService.privilegedStopOverride = nil service = nil try await super.tearDown() } @@ -88,6 +117,166 @@ final class KanataDaemonServiceIntegrationTests: KeyPathAsyncTestCase { } } + func testStartServiceRegisters() async throws { + let mock = MockSMAppService(status: .notRegistered) + useService(mock) + service = KanataDaemonService() + + try await service.start() + + XCTAssertTrue(mock.registerCalled) + XCTAssertEqual(mock.calls, ["register"]) + } + + func testStartVerifiesFinalStateWhenRegisterThrowsAfterEnabling() async throws { + let mock = MockSMAppService(status: .notRegistered) + mock.statusBeforeRegisterError = .enabled + useService(mock) + service = KanataDaemonService() + + try await service.start() + + XCTAssertEqual(mock.calls, ["register"]) + XCTAssertEqual(mock.status, .enabled) + } + + func testRestartServiceUnregistersBeforeRegistering() async throws { + let mock = MockSMAppService(status: .enabled) + useService(mock) + service = KanataDaemonService() + + try await service.restart() + + XCTAssertTrue(mock.unregisterCalled) + XCTAssertTrue(mock.registerCalled) + XCTAssertEqual(mock.calls, ["unregister", "register"]) + } + + func testStartServiceFailsExplicitlyWhenApprovalIsRequired() async { + let mock = MockSMAppService(status: .requiresApproval) + useService(mock) + service = KanataDaemonService() + + do { + try await service.start() + XCTFail("Expected approval-required failure") + } catch let error as KanataDaemonServiceError { + XCTAssertEqual(error, .approvalRequired) + } catch { + XCTFail("Unexpected error: \(error)") + } + + XCTAssertFalse(mock.registerCalled) + } + + func testStartServiceFailsWhenRegisteredRuntimeDoesNotBecomeReady() async { + let mock = MockSMAppService(status: .notRegistered) + useService(mock) + KanataDaemonService.runningPostconditionOverride = { false } + service = KanataDaemonService() + + do { + try await service.start() + XCTFail("Expected runtime-readiness failure") + } catch let error as KanataDaemonServiceError { + guard case let .startFailed(reason) = error else { + return XCTFail("Expected startFailed, got \(error)") + } + XCTAssertTrue(reason.contains("process and TCP readiness")) + } catch { + XCTFail("Unexpected error: \(error)") + } + } + + func testStopRetriesUnregisterThenUsesPrivilegedFallbackForStaleJob() async throws { + let mock = MockSMAppService(status: .enabled) + mock.statusesAfterUnregister = [.enabled, .enabled, .notRegistered] + useService(mock) + + var privilegedStopCalls = 0 + KanataDaemonService.privilegedStopOverride = { + privilegedStopCalls += 1 + } + service = KanataDaemonService() + + try await service.stop() + + XCTAssertEqual(mock.calls, ["unregister", "unregister", "unregister"]) + XCTAssertEqual(privilegedStopCalls, 1) + XCTAssertEqual(mock.status, .notRegistered) + } + + func testStopUsesPrivilegedFallbackWithoutRedundantUnregisterWhenOnlyProcessLingers() async throws { + let mock = MockSMAppService(status: .enabled) + useService(mock) + + var processRunning = true + KanataDaemonService.processRunningOverride = { processRunning } + var privilegedStopCalls = 0 + KanataDaemonService.privilegedStopOverride = { + privilegedStopCalls += 1 + processRunning = false + } + service = KanataDaemonService() + + try await service.stop() + + XCTAssertEqual(mock.calls, ["unregister"]) + XCTAssertEqual(privilegedStopCalls, 1) + XCTAssertEqual(mock.status, .notRegistered) + } + + func testStopVerifiesFinalStateWhenPostFallbackUnregisterThrows() async throws { + let mock = MockSMAppService(status: .enabled) + mock.statusesAfterUnregister = [.enabled, .enabled] + mock.failingUnregisterCalls = [3] + useService(mock) + + KanataDaemonService.privilegedStopOverride = { + mock.status = .notRegistered + } + service = KanataDaemonService() + + try await service.stop() + + XCTAssertEqual(mock.calls, ["unregister", "unregister", "unregister"]) + XCTAssertEqual(mock.status, .notRegistered) + } + + func testStopRetriesWhenInitialUnregisterThrows() async throws { + let mock = MockSMAppService(status: .enabled) + mock.failingUnregisterCalls = [1] + useService(mock) + + var privilegedStopCalls = 0 + KanataDaemonService.privilegedStopOverride = { + privilegedStopCalls += 1 + } + service = KanataDaemonService() + + try await service.stop() + + XCTAssertEqual(mock.calls, ["unregister", "unregister"]) + XCTAssertEqual(privilegedStopCalls, 0) + XCTAssertEqual(mock.status, .notRegistered) + } + + func testStopVerifiesFinalStateWhenPrivilegedCleanupThrows() async throws { + let mock = MockSMAppService(status: .enabled) + mock.statusesAfterUnregister = [.enabled, .enabled] + useService(mock) + + KanataDaemonService.privilegedStopOverride = { + throw MockSMAppService.MockError.unregisterFailed + } + service = KanataDaemonService() + + try await service.stop() + + XCTAssertEqual(mock.calls, ["unregister", "unregister", "unregister"]) + XCTAssertEqual(mock.status, .notRegistered) + } + func testStatusRefresh_ShouldDetectChanges() async { // Given: Initial unknown state diff --git a/docs/bugs/cli-service-control-helper-bypass.md b/docs/bugs/cli-service-control-helper-bypass.md index 6fa6ad2f5..3c468192f 100644 --- a/docs/bugs/cli-service-control-helper-bypass.md +++ b/docs/bugs/cli-service-control-helper-bypass.md @@ -57,3 +57,19 @@ contract advanced to 1.3.2 so installations cannot retain either earlier behavio reporting the helper as fresh. A lifecycle lint test preserves the required inspect-before-disable-before-signal and enable-before-kickstart ordering; installed-app acceptance verifies the real launchd transition. + +The next acceptance run proved that even direct PID signaling is insufficient: launchd respawns +an already loaded KeepAlive job after `launchctl disable`. The CLI lifecycle facade now uses the +same `SMAppService` register/unregister ownership path as the KeyPath UI. Stop unregisters the +daemon and verifies both registration and process removal, start registers it, and restart +performs those operations in order. If macOS leaves a stale job across an app replacement, stop +retries the owning API once before using the existing helper-backed privileged cleanup. The +helper remains the privilege boundary for operations that require root, but it no longer tries +to emulate the owning app's SMAppService lifecycle with launchctl signals. + +Lifecycle verification keeps synchronous SMAppService status IPC out of polling loops: stop +reads registration status once per bounded cleanup phase and polls launchd process evidence +between phases. It allows normal shutdown slightly longer than launchd's five-second exit +timeout before escalating to privileged cleanup. Start reports success only after registration +is enabled and both process and TCP readiness are proven; pending approval and failed launch +readiness are explicit failures.