From e409b6a8a5be3132c664cd4364ec8d9124590658 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Tue, 3 Nov 2026 04:31:20 -0800 Subject: [PATCH 1/9] Stop orphaned session workers and verify none survive uninstall On 2026-11-03 an uninstall on the Air reported success while two --session-capabilities probes kept running from the trashed bundle, with an owner PID that no longer existed. Workers: only the runtime's 5 ms tick checked --session-owner, and the tick exists only once start() has finished. Capability probes never checked it, so a probe that hung outlived its owner indefinitely. SessionOwnerWatch now polls the owner on a background queue in both modes; a probe exits with _exit when its owner dies or after 15s, and a runtime worker hands off to an orderly finish on its run loop once it may hold keys or a Caps mapping. Parent: the probe was signalled only after the 2s wait loop ran out. A cancelled wait threw past the SIGTERM. Every abandoned probe is now signalled. Uninstall: prepare stops only the worker the live app tracks. SessionWorkerSweep lists this UID's KeyPath session workers from the kernel process table (any bundle, including the Trash), SIGTERMs them, kills probes that ignore it, and never kills a runtime worker. The coordinator sweeps before removing anything and again before reporting success. Co-Authored-By: Claude Opus 5.5 --- ...eLifecycleCoordinator+SessionRuntime.swift | 16 +- .../SessionUninstallCoordinator.swift | 18 +++ .../SessionRuntime/SessionOwnerWatch.swift | 61 ++++++++ .../SessionRuntime/SessionRuntimeWorker.swift | 30 +++- .../SessionRuntime/SessionWorkerSweep.swift | 137 ++++++++++++++++++ .../WizardProtocolConformances.swift | 8 + .../SessionOwnerWatchTests.swift | 75 ++++++++++ .../SessionWorkerSweepTests.swift | 130 +++++++++++++++++ .../SessionUninstallCoordinatorTests.swift | 44 ++++++ 9 files changed, 511 insertions(+), 8 deletions(-) create mode 100644 Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift create mode 100644 Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift create mode 100644 Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift create mode 100644 Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift diff --git a/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift b/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift index be6d7afd0..5112f4256 100644 --- a/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift +++ b/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift @@ -51,20 +51,26 @@ extension ServiceLifecycleCoordinator { do { let (application, url, nonce) = try await launchSessionProcess(capabilitiesOnly: true) defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + var reported = false + // A timeout, a cancelled wait, or an error all abandon the probe; + // signal it on every one of them, not only after the timeout. + // NSWorkspace may return an application without a valid PID (e.g. -1). + // Never let that become a process-group/broadcast signal target. + // Signal only the independently launched process owned by this call. + defer { + let probePID = application.processIdentifier + if !reported, probePID > 0, !application.isTerminated { kill(probePID, SIGTERM) } + } for _ in 0 ..< 40 { if let report = Self.readSessionReport(url), report.state == .capabilities, report.isCurrent(nonce: nonce, pid: application.processIdentifier, uid: getuid(), now: Date()) { + reported = true return Self.permissionSet(report) } try await Task.sleep(for: .milliseconds(50)) } - // NSWorkspace may return an application without a valid PID (e.g. -1). - // Never let that become a process-group/broadcast signal target. - // Signal only the independently launched process owned by this call. - let probePID = application.processIdentifier - if probePID > 0, !application.isTerminated { kill(probePID, SIGTERM) } } catch { AppLogger.shared.warn("Session permission report unavailable") } diff --git a/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift b/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift index 62052d5b6..d68744b15 100644 --- a/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift +++ b/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift @@ -12,6 +12,7 @@ final class SessionUninstallCoordinator: WizardUninstalling { private let resume: () async -> Void private let trash: (URL) throws -> Void private let removeLoginItem: () async throws -> Void + private let stopSessionWorkers: () async -> Bool private let files = FileManager.default init( @@ -22,6 +23,7 @@ final class SessionUninstallCoordinator: WizardUninstalling { prepare: @escaping () async -> Bool, resume: @escaping () async -> Void, removeLoginItem: @escaping () async throws -> Void = {}, + stopSessionWorkers: @escaping () async -> Bool = { true }, trash: @escaping (URL) throws -> Void = { try FileManager.default.trashItem(at: $0, resultingItemURL: nil) } ) { self.home = home @@ -32,6 +34,7 @@ final class SessionUninstallCoordinator: WizardUninstalling { self.resume = resume self.trash = trash self.removeLoginItem = removeLoginItem + self.stopSessionWorkers = stopSessionWorkers } func performUninstall(deleteConfig: Bool, removeVirtualHID: Bool, allowAdminFallback: Bool) async -> WizardUninstallResult { @@ -60,6 +63,13 @@ final class SessionUninstallCoordinator: WizardUninstalling { await resume() return WizardUninstallResult(success: false, failureReason: "Keyboard cleanup could not be verified. Nothing was removed. Try again after releasing all keys.") } + // prepare stops only the worker the live app tracks. Orphans of an + // earlier app process, or probes it abandoned, would otherwise keep + // running from the trashed bundle. + guard await stopSessionWorkers() else { + await resume() + return WizardUninstallResult(success: false, failureReason: "A KeyPath keyboard process did not stop. Nothing was removed. Restart your Mac, then try again.") + } let backup = home.appendingPathComponent("Downloads/KeyPath-Uninstall-Backup-\(UUID().uuidString)", isDirectory: true) do { @@ -100,6 +110,14 @@ final class SessionUninstallCoordinator: WizardUninstalling { guard !files.fileExists(atPath: app.path) else { throw CocoaError(.fileWriteUnknown) } + // Success means nothing is left running, so check again at the end. + guard await stopSessionWorkers() else { + return WizardUninstallResult( + success: false, + failureReason: "KeyPath was removed, but a KeyPath keyboard process is still running. Restart your Mac to stop it. Your backup is at \(backup.path).", + logs: ["Backup saved to \(backup.path)"] + ) + } return WizardUninstallResult(success: true, logs: ["Backup saved to \(backup.path)"]) } catch { await resume() diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift new file mode 100644 index 000000000..293cb5c09 --- /dev/null +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift @@ -0,0 +1,61 @@ +import Foundation +import KeyPathCore + +/// Retires a worker whose owning app is gone. It polls on a background queue, +/// so a worker wedged before its main-loop tick exists still exits: on +/// 2026-11-03 two capability probes outlived their owner by hours this way. +final class SessionOwnerWatch: @unchecked Sendable { + enum Verdict: Equatable, Sendable { + case ownerExited + case deadlineExceeded + } + + /// Owner death wins over the deadline; a worker without an owner PID is + /// bounded only by its deadline. + nonisolated static func verdict(ownerPID: Int32, ownerAlive: Bool, elapsed: TimeInterval, + deadline: TimeInterval?) -> Verdict? + { + if ownerPID > 0, !ownerAlive { return .ownerExited } + if let deadline, elapsed >= deadline { return .deadlineExceeded } + return nil + } + + private let lock = NSLock() + private var handler: @Sendable (Verdict) -> Void + private let timer: DispatchSourceTimer + + /// The resumed timer retains itself, and its handler retains the watch, + /// until the first verdict cancels it. + init( + ownerPID: Int32, deadline: TimeInterval?, interval: TimeInterval = 0.25, + isAlive: @escaping @Sendable (Int32) -> Bool = { SystemStateProvider.isProcessAlive(pid: $0) }, + uptime: @escaping @Sendable () -> TimeInterval = { ProcessInfo.processInfo.systemUptime }, + queue: DispatchQueue = DispatchQueue(label: "com.keypath.session-owner-watch", qos: .utility), + handler: @escaping @Sendable (Verdict) -> Void + ) { + self.handler = handler + timer = DispatchSource.makeTimerSource(queue: queue) + let started = uptime() + // The first check runs immediately: the owner may already be gone. + timer.schedule(deadline: .now(), repeating: interval) + timer.setEventHandler { [self] in + guard let verdict = Self.verdict(ownerPID: ownerPID, ownerAlive: isAlive(ownerPID), + elapsed: uptime() - started, deadline: deadline) + else { return } + timer.cancel() + lock.lock() + let current = self.handler + lock.unlock() + current(verdict) + } + timer.resume() + } + + /// The runtime hands off from a hard exit to an orderly shutdown once it + /// may own outputs or a Caps mapping. + func setHandler(_ handler: @escaping @Sendable (Verdict) -> Void) { + lock.lock() + self.handler = handler + lock.unlock() + } +} diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift index 8cef8e6ad..7fb6a2d42 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift @@ -103,11 +103,18 @@ public final class SessionRuntimeWorker { let path = argument("--session-report"), let nonce = argument("--session-nonce"), UUID(uuidString: nonce) != nil else { exit(64) } + let ownerPID = argument("--session-owner").flatMap(Int32.init) ?? 0 + let probeOnly = arguments.contains("--session-capabilities") + // Until the runtime can own outputs, nothing needs releasing: a dead + // owner, or a probe still running after its parent stopped waiting, + // ends the process at once. _exit cannot block on a wedged main thread. + let ownerWatch = SessionOwnerWatch(ownerPID: ownerPID, + deadline: probeOnly ? capabilityProbeDeadline : nil) { _ in _exit(0) } - if arguments.contains("--session-capabilities") { + if probeOnly { await writeCapabilityReport( reportURL: URL(fileURLWithPath: path), nonce: nonce, - ownerPID: argument("--session-owner").flatMap(Int32.init) ?? 0, + ownerPID: ownerPID, port: argument("--session-port").flatMap(UInt16.init) ?? 0, snapshotProvider: { await SystemStateProvider.shared.currentProcessSessionCapabilitySnapshot() @@ -118,7 +125,7 @@ public final class SessionRuntimeWorker { let capabilities = await SystemStateProvider.shared.currentProcessPermissionCapabilities() let worker = SessionRuntimeWorker( reportURL: URL(fileURLWithPath: path), nonce: nonce, - ownerPID: argument("--session-owner").flatMap(Int32.init) ?? 0, + ownerPID: ownerPID, port: argument("--session-port").flatMap(UInt16.init) ?? 0, capabilities: capabilities ) @@ -131,12 +138,29 @@ public final class SessionRuntimeWorker { worker.experimentalRawCapabilities = SystemStateProvider.shared.currentProcessRawPermissionCapabilities() worker.experimentalRawPosting = await SystemStateProvider.shared.currentProcessRawEventPostingStatus() #endif + // From here the worker may hold keys or a Caps mapping, so a dead owner + // retires it on its run loop, which releases them before exiting. + ownerWatch.setHandler { [weak worker] _ in + CFRunLoopPerformBlock(CFRunLoopGetMain(), CFRunLoopMode.commonModes.rawValue) { + MainActor.assumeIsolated { worker?.retire(reason: "owner-exited") } + } + CFRunLoopWakeUp(CFRunLoopGetMain()) + } worker.start(configPath: config) // Keep the worker and loaded bridge alive through process termination; // dropping a runtime cannot stop its detached TCP listener threads. worker.runLoop() } + /// The parent waits about two seconds for a capability report and then + /// abandons the probe; this bounds a probe the parent could not signal. + static let capabilityProbeDeadline: TimeInterval = 15 + + private func retire(reason: String) { + guard !finished else { return } + finish(.stopped, reason: reason) + } + /// Capability mode samples passive APIs and writes evidence only. In particular, /// this path never enters start(), creates an event tap, or loads configuration. static func writeCapabilityReport( diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift new file mode 100644 index 000000000..6f29e338e --- /dev/null +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift @@ -0,0 +1,137 @@ +import Darwin +import Foundation + +/// One KeyPath session worker found in this user's process table. +struct SessionWorkerProcess: Equatable, Sendable { + enum Mode: Equatable, Sendable { + case runtime + case capabilities + } + + let pid: pid_t + let mode: Mode + let ownerPID: pid_t? + + /// `arguments[0]` is the executable path. Matches any KeyPath bundle, + /// including one already moved to the Trash, but never the app itself. + static func parse(pid: pid_t, arguments: [String]) -> SessionWorkerProcess? { + guard let executable = arguments.first, executable.hasSuffix(".app/Contents/MacOS/KeyPath") else { return nil } + let mode: Mode + if arguments.contains("--session-runtime") { + mode = .runtime + } else if arguments.contains("--session-capabilities") { + mode = .capabilities + } else { + return nil + } + let owner = arguments.firstIndex(of: "--session-owner") + .flatMap { arguments.index(after: $0) < arguments.endIndex ? arguments[arguments.index(after: $0)] : nil } + .flatMap(pid_t.init) + return SessionWorkerProcess(pid: pid, mode: mode, ownerPID: owner) + } +} + +/// Stops every KeyPath session worker this user runs, not only the one the +/// live app tracks: orphans of an app that exited, and probes it abandoned. +/// Uninstall uses it so success means none remain (see AGENTS.md postconditions). +struct SessionWorkerSweep: Sendable { + struct Outcome: Equatable, Sendable { + let found: [SessionWorkerProcess] + let remaining: [SessionWorkerProcess] + var succeeded: Bool { remaining.isEmpty } + } + + var list: @Sendable () -> [SessionWorkerProcess] + var signal: @Sendable (pid_t, Int32) -> Void + var sleep: @Sendable (Duration) async -> Void + + static let live = SessionWorkerSweep( + list: { currentUserSessionWorkers() }, + signal: { pid, number in _ = kill(pid, number) }, + sleep: { try? await Task.sleep(for: $0) } + ) + + /// A runtime worker gets SIGTERM only: its shutdown releases held keys and + /// restores Caps mappings, and killing it would strand them. A capability + /// probe owns nothing, so one still alive after `probeGrace` is killed. + func stopAll(probeGrace: Duration = .seconds(2), runtimeGrace: Duration = .seconds(10), + poll: Duration = .milliseconds(100)) async -> Outcome + { + let found = list() + guard !found.isEmpty else { return Outcome(found: [], remaining: []) } + for worker in found { + signal(worker.pid, SIGTERM) + } + var waited = Duration.zero + var killedProbes = false + var remaining = found + while !remaining.isEmpty, waited < runtimeGrace { + await sleep(poll) + waited += poll + remaining = list() + if !killedProbes, waited >= probeGrace { + killedProbes = true + for worker in remaining where worker.mode == .capabilities { + signal(worker.pid, SIGKILL) + } + } + } + return Outcome(found: found, remaining: remaining) + } + + /// Reads the kernel process table directly: no pgrep, and only this UID. + static func currentUserSessionWorkers() -> [SessionWorkerProcess] { + var mib: [Int32] = [CTL_KERN, KERN_PROC, KERN_PROC_UID, Int32(bitPattern: getuid())] + var size = 0 + guard sysctl(&mib, 4, nil, &size, nil, 0) == 0, size > 0 else { return [] } + // Headroom for processes started between the two calls. + size += 32 * MemoryLayout.stride + var processes = [kinfo_proc](repeating: kinfo_proc(), count: size / MemoryLayout.stride) + guard sysctl(&mib, 4, &processes, &size, nil, 0) == 0 else { return [] } + let count = size / MemoryLayout.stride + let me = getpid() + return processes.prefix(count).compactMap { process in + let pid = process.kp_proc.p_pid + guard pid > 0, pid != me, let arguments = arguments(of: pid) else { return nil } + return SessionWorkerProcess.parse(pid: pid, arguments: arguments) + } + } + + /// KERN_PROCARGS2 is argc, the executable path, padding, then argv. + /// Returns the executable path followed by argv[1...]. + static func arguments(of pid: pid_t) -> [String]? { + var argumentMax: Int32 = 0 + var argumentMaxSize = MemoryLayout.size + var maxMib: [Int32] = [CTL_KERN, KERN_ARGMAX] + guard sysctl(&maxMib, 2, &argumentMax, &argumentMaxSize, nil, 0) == 0, argumentMax > 0 else { return nil } + var buffer = [UInt8](repeating: 0, count: Int(argumentMax)) + var size = buffer.count + var mib: [Int32] = [CTL_KERN, KERN_PROCARGS2, pid] + guard sysctl(&mib, 3, &buffer, &size, nil, 0) == 0 else { return nil } + return parseProcArgs(Array(buffer.prefix(size))) + } + + static func parseProcArgs(_ bytes: [UInt8]) -> [String]? { + let intSize = MemoryLayout.size + guard bytes.count > intSize else { return nil } + let argc = bytes.prefix(intSize).withUnsafeBytes { Int($0.loadUnaligned(as: Int32.self)) } + guard argc > 0 else { return nil } + var index = intSize + func nextString() -> String? { + guard index < bytes.count, let end = bytes[index...].firstIndex(of: 0) else { return nil } + let value = String(decoding: bytes[index ..< end], as: UTF8.self) + index = end + 1 + return value + } + guard let executable = nextString() else { return nil } + while index < bytes.count, bytes[index] == 0 { + index += 1 + } + var argv: [String] = [] + for _ in 0 ..< argc { + guard let argument = nextString() else { break } + argv.append(argument) + } + return [executable] + argv.dropFirst() + } +} diff --git a/Sources/KeyPathAppKit/WizardProtocolConformances.swift b/Sources/KeyPathAppKit/WizardProtocolConformances.swift index 7ccdd9e0a..9f9bc276e 100644 --- a/Sources/KeyPathAppKit/WizardProtocolConformances.swift +++ b/Sources/KeyPathAppKit/WizardProtocolConformances.swift @@ -163,6 +163,14 @@ public func configureWizardDependencies(runtimeCoordinator: RuntimeCoordinator) runtimeCoordinator.startConfigFileWatching() }, removeLoginItem: { try await LaunchAtLoginController.shared.setEnabled(false) + }, stopSessionWorkers: { + let outcome = await SessionWorkerSweep.live.stopAll() + if !outcome.found.isEmpty { + AppLogger.shared.log( + "[Uninstall] Session workers found=\(outcome.found.map(\.pid)) remaining=\(outcome.remaining.map(\.pid))" + ) + } + return outcome.succeeded }) } WizardDependencies.executePrivilegedBatch = { batch in diff --git a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift new file mode 100644 index 000000000..752e58ee2 --- /dev/null +++ b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift @@ -0,0 +1,75 @@ +import Foundation +@testable import KeyPathAppKit +import XCTest + +final class SessionOwnerWatchTests: XCTestCase { + func testVerdictRetiresWhenOwnerIsGone() { + XCTAssertEqual(SessionOwnerWatch.verdict(ownerPID: 30378, ownerAlive: false, elapsed: 0, deadline: nil), .ownerExited) + XCTAssertNil(SessionOwnerWatch.verdict(ownerPID: 30378, ownerAlive: true, elapsed: 3600, deadline: nil)) + } + + func testOwnerExitWinsOverDeadline() { + XCTAssertEqual(SessionOwnerWatch.verdict(ownerPID: 7, ownerAlive: false, elapsed: 20, deadline: 15), .ownerExited) + } + + func testDeadlineBoundsAProbeWhoseOwnerIsAlive() { + XCTAssertNil(SessionOwnerWatch.verdict(ownerPID: 7, ownerAlive: true, elapsed: 14.9, deadline: 15)) + XCTAssertEqual(SessionOwnerWatch.verdict(ownerPID: 7, ownerAlive: true, elapsed: 15, deadline: 15), .deadlineExceeded) + } + + /// A missing --session-owner is not a dead owner; only the deadline applies. + func testMissingOwnerIsNotTreatedAsExited() { + XCTAssertNil(SessionOwnerWatch.verdict(ownerPID: 0, ownerAlive: false, elapsed: 1, deadline: nil)) + XCTAssertEqual(SessionOwnerWatch.verdict(ownerPID: 0, ownerAlive: false, elapsed: 16, deadline: 15), .deadlineExceeded) + } + + func testWatchFiresOnceWhenOwnerDiesWithoutTheMainThread() { + let alive = Locked(true) + let fired = expectation(description: "Owner exit observed") + let calls = Locked(0) + let watch = SessionOwnerWatch(ownerPID: 30378, deadline: nil, interval: 0.01, + isAlive: { _ in alive.value }) { verdict in + XCTAssertFalse(Thread.isMainThread) + XCTAssertEqual(verdict, .ownerExited) + calls.update { $0 += 1 } + fired.fulfill() + } + alive.update { $0 = false } + wait(for: [fired], timeout: 2) + // A cancelled timer never delivers a second verdict. + Thread.sleep(forTimeInterval: 0.1) + XCTAssertEqual(calls.value, 1) + withExtendedLifetime(watch) {} + } + + func testReplacedHandlerReceivesTheVerdict() { + let alive = Locked(true) + let fired = expectation(description: "Replacement handler called") + let watch = SessionOwnerWatch(ownerPID: 30378, deadline: nil, interval: 0.01, + isAlive: { _ in alive.value }) { _ in XCTFail("Startup handler must be replaced") } + watch.setHandler { _ in fired.fulfill() } + alive.update { $0 = false } + wait(for: [fired], timeout: 2) + } + + func testWatchEndsAProbeAtItsDeadline() { + let now = Locked(100.0) + let fired = expectation(description: "Deadline observed") + let watch = SessionOwnerWatch(ownerPID: 30378, deadline: 15, interval: 0.01, + isAlive: { _ in true }, uptime: { now.value }) { verdict in + XCTAssertEqual(verdict, .deadlineExceeded) + fired.fulfill() + } + now.update { $0 = 115 } + wait(for: [fired], timeout: 2) + withExtendedLifetime(watch) {} + } +} + +private final class Locked: @unchecked Sendable { + private let lock = NSLock() + private var stored: Value + init(_ value: Value) { stored = value } + var value: Value { lock.lock(); defer { lock.unlock() }; return stored } + func update(_ body: (inout Value) -> Void) { lock.lock(); body(&stored); lock.unlock() } +} diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift new file mode 100644 index 000000000..1f4a65f9b --- /dev/null +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -0,0 +1,130 @@ +import Darwin +import Foundation +@testable import KeyPathAppKit +import XCTest + +final class SessionWorkerSweepTests: XCTestCase { + private static let trashedExecutable = "/Users/someone/.Trash/KeyPath.app/Contents/MacOS/KeyPath" + + // MARK: - Recognizing workers + + /// The 2026-11-03 orphans: capability probes from a trashed bundle whose owner had exited. + func testParsesAnOrphanedProbeFromATrashedBundle() { + let worker = SessionWorkerProcess.parse(pid: 13284, arguments: [ + Self.trashedExecutable, "--session-capabilities", "--session-report", "/tmp/r.json", + "--session-nonce", UUID().uuidString, "--session-owner", "30378", "--session-port", "37001", + "--session-config", "/Users/someone/.config/keypath/keypath.kbd", + ]) + XCTAssertEqual(worker, SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378)) + } + + func testParsesARuntimeWorker() { + let worker = SessionWorkerProcess.parse(pid: 42, arguments: [ + "/Applications/KeyPath.app/Contents/MacOS/KeyPath", "--session-runtime", "--session-owner", "41", + ]) + XCTAssertEqual(worker, SessionWorkerProcess(pid: 42, mode: .runtime, ownerPID: 41)) + } + + func testIgnoresTheAppItselfAndOtherProcesses() { + XCTAssertNil(SessionWorkerProcess.parse(pid: 1, arguments: ["/Applications/KeyPath.app/Contents/MacOS/KeyPath"])) + XCTAssertNil(SessionWorkerProcess.parse(pid: 2, arguments: ["/usr/bin/grep", "--session-runtime"])) + XCTAssertNil(SessionWorkerProcess.parse(pid: 3, arguments: [ + "/Applications/KeyPath.app/Contents/MacOS/keypath-cli", "--session-runtime", + ])) + XCTAssertNil(SessionWorkerProcess.parse(pid: 4, arguments: [])) + } + + func testMissingOwnerValueParsesWithoutOwner() { + let worker = SessionWorkerProcess.parse(pid: 5, arguments: [Self.trashedExecutable, "--session-runtime", "--session-owner"]) + XCTAssertEqual(worker, SessionWorkerProcess(pid: 5, mode: .runtime, ownerPID: nil)) + } + + func testParsesKernelArgumentLayout() { + var bytes = withUnsafeBytes(of: Int32(3)) { Array($0) } + for string in [Self.trashedExecutable, "", "", "KeyPath", "--session-capabilities", "--session-owner"] { + bytes += Array(string.utf8) + [0] + } + bytes += Array("KEYPATH=1".utf8) + [0] + XCTAssertEqual(SessionWorkerSweep.parseProcArgs(bytes), + [Self.trashedExecutable, "--session-capabilities", "--session-owner"]) + XCTAssertNil(SessionWorkerSweep.parseProcArgs([1, 0])) + } + + // MARK: - Stopping workers + + private final class FakeProcesses: @unchecked Sendable { + private let lock = NSLock() + private var running: [SessionWorkerProcess] + /// Workers that exit on SIGTERM; others ignore it. + private let exitOnTerm: Set + private(set) var signals: [(pid_t, Int32)] = [] + + init(_ running: [SessionWorkerProcess], exitOnTerm: Set) { + self.running = running + self.exitOnTerm = exitOnTerm + } + + var sweep: SessionWorkerSweep { + SessionWorkerSweep( + list: { self.locked { self.running } }, + signal: { pid, number in + self.locked { + self.signals.append((pid, number)) + if number == SIGKILL || self.exitOnTerm.contains(pid) { self.running.removeAll { $0.pid == pid } } + } + }, + sleep: { _ in } + ) + } + + func locked(_ body: () -> T) -> T { lock.lock(); defer { lock.unlock() }; return body() } + func received(_ number: Int32) -> [pid_t] { locked { signals.filter { $0.1 == number }.map(\.0) } } + } + + func testNoWorkersSucceedsWithoutSignalling() async { + let fake = FakeProcesses([], exitOnTerm: []) + let outcome = await fake.sweep.stopAll() + XCTAssertTrue(outcome.succeeded) + XCTAssertTrue(fake.signals.isEmpty) + } + + /// Every worker is stopped, not only the one the live app tracks. + func testStopsEveryWorkerIncludingOrphans() async { + let tracked = SessionWorkerProcess(pid: 100, mode: .runtime, ownerPID: 99) + let orphanA = SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378) + let orphanB = SessionWorkerProcess(pid: 29722, mode: .capabilities, ownerPID: 30378) + let fake = FakeProcesses([tracked, orphanA, orphanB], exitOnTerm: [100, 13284, 29722]) + let outcome = await fake.sweep.stopAll() + XCTAssertTrue(outcome.succeeded) + XCTAssertEqual(Set(fake.received(SIGTERM)), [100, 13284, 29722]) + XCTAssertTrue(fake.received(SIGKILL).isEmpty) + } + + func testKillsAWedgedProbeAfterItsGrace() async { + let probe = SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378) + let fake = FakeProcesses([probe], exitOnTerm: []) + let outcome = await fake.sweep.stopAll(probeGrace: .milliseconds(200), runtimeGrace: .seconds(1), + poll: .milliseconds(100)) + XCTAssertTrue(outcome.succeeded) + XCTAssertEqual(fake.received(SIGKILL), [13284]) + } + + /// A runtime worker may hold a Caps mapping; it is never killed, and a + /// worker that ignores SIGTERM is reported, not hidden. + func testNeverKillsARuntimeWorkerAndReportsItRemaining() async { + let runtime = SessionWorkerProcess(pid: 100, mode: .runtime, ownerPID: 99) + let fake = FakeProcesses([runtime], exitOnTerm: []) + let outcome = await fake.sweep.stopAll(probeGrace: .milliseconds(100), runtimeGrace: .milliseconds(500), + poll: .milliseconds(100)) + XCTAssertFalse(outcome.succeeded) + XCTAssertEqual(outcome.remaining, [runtime]) + XCTAssertTrue(fake.received(SIGKILL).isEmpty) + } + + /// Reads the real process table without signalling anything: this test + /// process is not a KeyPath worker, so it must never list itself. + func testLiveListingNeverIncludesThisProcess() { + XCTAssertFalse(SessionWorkerSweep.currentUserSessionWorkers().contains { $0.pid == getpid() }) + XCTAssertNotNil(SessionWorkerSweep.arguments(of: getpid())) + } +} diff --git a/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift b/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift index 7a89643f7..de32cdf3b 100644 --- a/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift +++ b/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift @@ -88,6 +88,50 @@ final class SessionUninstallCoordinatorTests: XCTestCase { XCTAssertTrue(FileManager.default.fileExists(atPath: root.appendingPathComponent(".config/keypath/keypath.kbd").path)) } + func testWorkersThatDoNotStopRemoveNothing() async { + var resumed = false + let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, + prepare: { true }, resume: { resumed = true }, + stopSessionWorkers: { false }, + trash: { _ in XCTFail("Must not trash while a worker runs") }) + let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) + XCTAssertFalse(result.success) + XCTAssertTrue(resumed) + XCTAssertTrue(result.failureReason?.contains("Nothing was removed") == true) + XCTAssertTrue(FileManager.default.fileExists(atPath: root.appendingPathComponent(".config/keypath/keypath.kbd").path)) + XCTAssertTrue(FileManager.default.fileExists(atPath: app.path)) + } + + /// Success is reported only after a final sweep finds no worker left. + func testWorkerFoundAfterRemovalIsNotReportedAsSuccess() async { + var sweeps = 0 + let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, + prepare: { true }, resume: {}, + stopSessionWorkers: { sweeps += 1; return sweeps == 1 }, + trash: { try FileManager.default.removeItem(at: $0) }) + let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) + XCTAssertEqual(sweeps, 2) + XCTAssertFalse(result.success) + XCTAssertTrue(result.failureReason?.contains("still running") == true) + XCTAssertTrue(result.failureReason?.contains("Your backup is at") == true) + } + + func testSuccessfulUninstallSweepsWorkersBeforeAndAfterRemoval() async { + var sweepsBeforeTrash = 0 + var sweeps = 0 + let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, + prepare: { true }, resume: { XCTFail("Unexpected failure") }, + stopSessionWorkers: { sweeps += 1; return true }, + trash: { url in + sweepsBeforeTrash = sweeps + try FileManager.default.removeItem(at: url) + }) + let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) + XCTAssertTrue(result.success, result.failureReason ?? "") + XCTAssertEqual(sweepsBeforeTrash, 1) + XCTAssertEqual(sweeps, 2) + } + func testBackupFailurePreservesSettingsAndApp() async throws { try write("Downloads", "not a directory") let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, From 6337e05e866ad1e8f1f64b62138141746d3b56c2 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Tue, 3 Nov 2026 04:52:59 -0800 Subject: [PATCH 2/9] Address review: unknown is not success, scope and bound the sweep - An unreadable process table, or a KeyPath process whose arguments cannot be read, now makes the sweep unverified rather than empty. The table read retries on ENOMEM. - Only processes whose p_comm is KeyPath pay for an argument read, with one shared buffer; graces are measured by wall clock. - Workers that appear mid-sweep are signalled, and any probe still present after the probe grace is killed. - Uninstall stops workers of the bundle being removed and orphans of any bundle, so a separate install's live worker is left alone. - An orphaned runtime worker whose run loop never runs its retirement exits after 30s instead of living forever. - The probe wait is extracted as awaitCapabilityReport, with tests for the cancelled-wait regression. Co-Authored-By: Claude Opus 5.5 --- ...eLifecycleCoordinator+SessionRuntime.swift | 55 +++++--- .../SessionRuntime/SessionRuntimeWorker.swift | 9 +- .../SessionRuntime/SessionWorkerSweep.swift | 131 +++++++++++++----- .../WizardProtocolConformances.swift | 2 +- .../SessionCapabilityProbeWaitTests.swift | 49 +++++++ .../SessionWorkerSweepTests.swift | 118 ++++++++++++++-- 6 files changed, 292 insertions(+), 72 deletions(-) create mode 100644 Tests/KeyPathTests/SessionRuntime/SessionCapabilityProbeWaitTests.swift diff --git a/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift b/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift index 5112f4256..c2ade0c4a 100644 --- a/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift +++ b/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift @@ -51,32 +51,49 @@ extension ServiceLifecycleCoordinator { do { let (application, url, nonce) = try await launchSessionProcess(capabilitiesOnly: true) defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } - var reported = false - // A timeout, a cancelled wait, or an error all abandon the probe; - // signal it on every one of them, not only after the timeout. - // NSWorkspace may return an application without a valid PID (e.g. -1). - // Never let that become a process-group/broadcast signal target. - // Signal only the independently launched process owned by this call. - defer { - let probePID = application.processIdentifier - if !reported, probePID > 0, !application.isTerminated { kill(probePID, SIGTERM) } - } - for _ in 0 ..< 40 { - if let report = Self.readSessionReport(url), - report.state == .capabilities, - report.isCurrent(nonce: nonce, pid: application.processIdentifier, uid: getuid(), now: Date()) - { - reported = true - return Self.permissionSet(report) + let report = try await Self.awaitCapabilityReport( + read: { + guard let report = Self.readSessionReport(url), report.state == .capabilities, + report.isCurrent(nonce: nonce, pid: application.processIdentifier, uid: getuid(), now: Date()) + else { return nil } + return report + }, + sleep: { try await Task.sleep(for: .milliseconds(50)) }, + abandon: { + // NSWorkspace may return an application without a valid PID (e.g. -1). + // Never let that become a process-group/broadcast signal target. + // Signal only the independently launched process owned by this call. + let probePID = application.processIdentifier + if probePID > 0, !application.isTerminated { kill(probePID, SIGTERM) } } - try await Task.sleep(for: .milliseconds(50)) - } + ) + if let report { return Self.permissionSet(report) } } catch { AppLogger.shared.warn("Session permission report unavailable") } return nil } + /// Polls about two seconds for the probe's report. A timeout, a cancelled + /// wait, or a thrown error all abandon the probe, and every one of them + /// signals it: on 2026-11-03 a cancelled wait skipped the signal and two + /// probes outlived their app. + static func awaitCapabilityReport( + attempts: Int = 40, read: () -> SessionRuntimeReport?, + sleep: () async throws -> Void, abandon: () -> Void + ) async throws -> SessionRuntimeReport? { + var reported = false + defer { if !reported { abandon() } } + for _ in 0 ..< attempts { + if let report = read() { + reported = true + return report + } + try await sleep() + } + return nil + } + private func isLegacyDaemonRunning() async -> Bool { #if DEBUG if let probe = testSessionLegacyDaemonRunning { return await probe() } diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift index 7fb6a2d42..fb676e12e 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift @@ -139,12 +139,15 @@ public final class SessionRuntimeWorker { worker.experimentalRawPosting = await SystemStateProvider.shared.currentProcessRawEventPostingStatus() #endif // From here the worker may hold keys or a Caps mapping, so a dead owner - // retires it on its run loop, which releases them before exiting. + // retires it on its run loop, which releases them before exiting. A + // run loop that never gets there would keep an orphan forever; after + // the grace it exits anyway, and the Caps journal survives for recovery. ownerWatch.setHandler { [weak worker] _ in CFRunLoopPerformBlock(CFRunLoopGetMain(), CFRunLoopMode.commonModes.rawValue) { MainActor.assumeIsolated { worker?.retire(reason: "owner-exited") } } CFRunLoopWakeUp(CFRunLoopGetMain()) + DispatchQueue.global(qos: .utility).asyncAfter(deadline: .now() + ownerExitShutdownGrace) { _exit(1) } } worker.start(configPath: config) // Keep the worker and loaded bridge alive through process termination; @@ -154,7 +157,9 @@ public final class SessionRuntimeWorker { /// The parent waits about two seconds for a capability report and then /// abandons the probe; this bounds a probe the parent could not signal. - static let capabilityProbeDeadline: TimeInterval = 15 + nonisolated static let capabilityProbeDeadline: TimeInterval = 15 + /// How long an orphaned runtime gets to release outputs on its run loop. + nonisolated static let ownerExitShutdownGrace: TimeInterval = 30 private func retire(reason: String) { guard !finished else { return } diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift index 6f29e338e..a56fafac4 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift @@ -1,5 +1,6 @@ import Darwin import Foundation +import KeyPathCore /// One KeyPath session worker found in this user's process table. struct SessionWorkerProcess: Equatable, Sendable { @@ -11,6 +12,7 @@ struct SessionWorkerProcess: Equatable, Sendable { let pid: pid_t let mode: Mode let ownerPID: pid_t? + var executable = "" /// `arguments[0]` is the executable path. Matches any KeyPath bundle, /// including one already moved to the Trash, but never the app itself. @@ -27,7 +29,15 @@ struct SessionWorkerProcess: Equatable, Sendable { let owner = arguments.firstIndex(of: "--session-owner") .flatMap { arguments.index(after: $0) < arguments.endIndex ? arguments[arguments.index(after: $0)] : nil } .flatMap(pid_t.init) - return SessionWorkerProcess(pid: pid, mode: mode, ownerPID: owner) + return SessionWorkerProcess(pid: pid, mode: mode, ownerPID: owner, executable: executable) + } + + /// Uninstall stops workers of the bundle it removes, and orphans of any + /// bundle; a separate KeyPath install whose app is alive keeps its worker. + func belongsToUninstall(of app: URL, ownerAlive: (pid_t) -> Bool) -> Bool { + if executable.hasPrefix(app.standardizedFileURL.path + "/") { return true } + guard let ownerPID, ownerPID > 0 else { return true } + return !ownerAlive(ownerPID) } } @@ -38,73 +48,118 @@ struct SessionWorkerSweep: Sendable { struct Outcome: Equatable, Sendable { let found: [SessionWorkerProcess] let remaining: [SessionWorkerProcess] - var succeeded: Bool { remaining.isEmpty } + /// False when the process table could not be read: absence unproven. + var verified = true + var succeeded: Bool { verified && remaining.isEmpty } } - var list: @Sendable () -> [SessionWorkerProcess] + /// nil means the process table could not be read, never "no workers". + var list: @Sendable () -> [SessionWorkerProcess]? var signal: @Sendable (pid_t, Int32) -> Void var sleep: @Sendable (Duration) async -> Void + var uptime: @Sendable () -> TimeInterval = { ProcessInfo.processInfo.systemUptime } - static let live = SessionWorkerSweep( - list: { currentUserSessionWorkers() }, - signal: { pid, number in _ = kill(pid, number) }, - sleep: { try? await Task.sleep(for: $0) } - ) + static func live(uninstalling app: URL) -> SessionWorkerSweep { + SessionWorkerSweep( + list: { + currentUserSessionWorkers()?.filter { + $0.belongsToUninstall(of: app) { SystemStateProvider.isProcessAlive(pid: $0) } + } + }, + signal: { pid, number in _ = kill(pid, number) }, + sleep: { try? await Task.sleep(for: $0) } + ) + } /// A runtime worker gets SIGTERM only: its shutdown releases held keys and /// restores Caps mappings, and killing it would strand them. A capability /// probe owns nothing, so one still alive after `probeGrace` is killed. - func stopAll(probeGrace: Duration = .seconds(2), runtimeGrace: Duration = .seconds(10), + /// Workers that appear mid-sweep are signalled too. Graces are wall-clock. + func stopAll(probeGrace: TimeInterval = 2, runtimeGrace: TimeInterval = 10, poll: Duration = .milliseconds(100)) async -> Outcome { - let found = list() + guard let found = list() else { return Outcome(found: [], remaining: [], verified: false) } guard !found.isEmpty else { return Outcome(found: [], remaining: []) } - for worker in found { - signal(worker.pid, SIGTERM) - } - var waited = Duration.zero - var killedProbes = false + let started = uptime() + var termed: Set = [] + var killed: Set = [] + var everFound = found var remaining = found - while !remaining.isEmpty, waited < runtimeGrace { - await sleep(poll) - waited += poll - remaining = list() - if !killedProbes, waited >= probeGrace { - killedProbes = true - for worker in remaining where worker.mode == .capabilities { + while true { + let elapsed = uptime() - started + for worker in remaining where !termed.contains(worker.pid) { + termed.insert(worker.pid) + signal(worker.pid, SIGTERM) + } + if elapsed >= probeGrace { + for worker in remaining where worker.mode == .capabilities && !killed.contains(worker.pid) { + killed.insert(worker.pid) signal(worker.pid, SIGKILL) } } + guard !remaining.isEmpty, elapsed < runtimeGrace else { break } + await sleep(poll) + guard let listed = list() else { return Outcome(found: everFound, remaining: remaining, verified: false) } + remaining = listed + everFound += listed.filter { worker in !everFound.contains { $0.pid == worker.pid } } } - return Outcome(found: found, remaining: remaining) + return Outcome(found: everFound, remaining: remaining) } /// Reads the kernel process table directly: no pgrep, and only this UID. - static func currentUserSessionWorkers() -> [SessionWorkerProcess] { + static func currentUserSessionWorkers() -> [SessionWorkerProcess]? { var mib: [Int32] = [CTL_KERN, KERN_PROC, KERN_PROC_UID, Int32(bitPattern: getuid())] + var processes: [kinfo_proc] = [] var size = 0 - guard sysctl(&mib, 4, nil, &size, nil, 0) == 0, size > 0 else { return [] } - // Headroom for processes started between the two calls. - size += 32 * MemoryLayout.stride - var processes = [kinfo_proc](repeating: kinfo_proc(), count: size / MemoryLayout.stride) - guard sysctl(&mib, 4, &processes, &size, nil, 0) == 0 else { return [] } + // Processes started between sizing and reading make the read ENOMEM. + for _ in 0 ..< 4 { + guard sysctl(&mib, 4, nil, &size, nil, 0) == 0, size > 0 else { return nil } + size += 32 * MemoryLayout.stride + processes = [kinfo_proc](repeating: kinfo_proc(), count: size / MemoryLayout.stride) + if sysctl(&mib, 4, &processes, &size, nil, 0) == 0 { break } + guard errno == ENOMEM else { return nil } + processes = [] + } + guard !processes.isEmpty else { return nil } let count = size / MemoryLayout.stride let me = getpid() - return processes.prefix(count).compactMap { process in + // One buffer for every process: kern.argmax is 1 MiB. + guard let argumentMax = argumentMax() else { return nil } + var buffer = [UInt8](repeating: 0, count: argumentMax) + var workers: [SessionWorkerProcess] = [] + for process in processes.prefix(count) { let pid = process.kp_proc.p_pid - guard pid > 0, pid != me, let arguments = arguments(of: pid) else { return nil } - return SessionWorkerProcess.parse(pid: pid, arguments: arguments) + // Only KeyPath processes pay for an argument read. A zombie has no + // arguments and runs nothing. + guard pid > 0, pid != me, Int32(process.kp_proc.p_stat) != SZOMB, + command(of: process) == "KeyPath" else { continue } + guard let arguments = arguments(of: pid, buffer: &buffer) else { + // Gone between the two reads, or unreadable: unreadable is unknown. + if kill(pid, 0) == 0 || errno == EPERM { return nil } + continue + } + if let worker = SessionWorkerProcess.parse(pid: pid, arguments: arguments) { workers.append(worker) } } + return workers } - /// KERN_PROCARGS2 is argc, the executable path, padding, then argv. - /// Returns the executable path followed by argv[1...]. - static func arguments(of pid: pid_t) -> [String]? { + private static func command(of process: kinfo_proc) -> String { + withUnsafeBytes(of: process.kp_proc.p_comm) { raw in + String(decoding: raw.prefix { $0 != 0 }, as: UTF8.self) + } + } + + static func argumentMax() -> Int? { var argumentMax: Int32 = 0 var argumentMaxSize = MemoryLayout.size - var maxMib: [Int32] = [CTL_KERN, KERN_ARGMAX] - guard sysctl(&maxMib, 2, &argumentMax, &argumentMaxSize, nil, 0) == 0, argumentMax > 0 else { return nil } - var buffer = [UInt8](repeating: 0, count: Int(argumentMax)) + var mib: [Int32] = [CTL_KERN, KERN_ARGMAX] + guard sysctl(&mib, 2, &argumentMax, &argumentMaxSize, nil, 0) == 0, argumentMax > 0 else { return nil } + return Int(argumentMax) + } + + /// KERN_PROCARGS2 is argc, the executable path, padding, then argv. + /// Returns the executable path followed by argv[1...]. + static func arguments(of pid: pid_t, buffer: inout [UInt8]) -> [String]? { var size = buffer.count var mib: [Int32] = [CTL_KERN, KERN_PROCARGS2, pid] guard sysctl(&mib, 3, &buffer, &size, nil, 0) == 0 else { return nil } diff --git a/Sources/KeyPathAppKit/WizardProtocolConformances.swift b/Sources/KeyPathAppKit/WizardProtocolConformances.swift index 9f9bc276e..0a269b756 100644 --- a/Sources/KeyPathAppKit/WizardProtocolConformances.swift +++ b/Sources/KeyPathAppKit/WizardProtocolConformances.swift @@ -164,7 +164,7 @@ public func configureWizardDependencies(runtimeCoordinator: RuntimeCoordinator) }, removeLoginItem: { try await LaunchAtLoginController.shared.setEnabled(false) }, stopSessionWorkers: { - let outcome = await SessionWorkerSweep.live.stopAll() + let outcome = await SessionWorkerSweep.live(uninstalling: Bundle.main.bundleURL).stopAll() if !outcome.found.isEmpty { AppLogger.shared.log( "[Uninstall] Session workers found=\(outcome.found.map(\.pid)) remaining=\(outcome.remaining.map(\.pid))" diff --git a/Tests/KeyPathTests/SessionRuntime/SessionCapabilityProbeWaitTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionCapabilityProbeWaitTests.swift new file mode 100644 index 000000000..a66a147ad --- /dev/null +++ b/Tests/KeyPathTests/SessionRuntime/SessionCapabilityProbeWaitTests.swift @@ -0,0 +1,49 @@ +import Foundation +@testable import KeyPathAppKit +import KeyPathCore +import XCTest + +/// The 2026-11-03 regression: a cancelled wait skipped the signal, so the +/// probe outlived the app. Every abandoned wait must signal the probe. +@MainActor +final class SessionCapabilityProbeWaitTests: XCTestCase { + private struct Cancelled: Error {} + + private func report() -> SessionRuntimeReport { + SessionRuntimeReport(nonce: "probe", pid: 123, uid: 502, state: .capabilities, + accessibility: true, effectiveInputAccess: true, tapActive: false, + tcpPort: 0, inputCount: 0, outputCount: 0) + } + + func testCancelledWaitSignalsTheProbe() async { + var abandoned = 0 + do { + _ = try await ServiceLifecycleCoordinator.awaitCapabilityReport( + read: { nil }, sleep: { throw Cancelled() }, abandon: { abandoned += 1 } + ) + XCTFail("The cancellation must propagate") + } catch {} + XCTAssertEqual(abandoned, 1) + } + + func testTimeoutSignalsTheProbeOnce() async throws { + var abandoned = 0 + var reads = 0 + let result = try await ServiceLifecycleCoordinator.awaitCapabilityReport( + attempts: 3, read: { reads += 1; return nil }, sleep: {}, abandon: { abandoned += 1 } + ) + XCTAssertNil(result) + XCTAssertEqual(reads, 3) + XCTAssertEqual(abandoned, 1) + } + + /// A probe that reported exits on its own and is not signalled. + func testReportedProbeIsNotSignalled() async throws { + var reads = 0 + let result = try await ServiceLifecycleCoordinator.awaitCapabilityReport( + read: { reads += 1; return reads == 2 ? self.report() : nil }, sleep: {}, + abandon: { XCTFail("A reported probe must not be signalled") } + ) + XCTAssertEqual(result?.nonce, "probe") + } +} diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index 1f4a65f9b..178cbc7ee 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -15,14 +15,15 @@ final class SessionWorkerSweepTests: XCTestCase { "--session-nonce", UUID().uuidString, "--session-owner", "30378", "--session-port", "37001", "--session-config", "/Users/someone/.config/keypath/keypath.kbd", ]) - XCTAssertEqual(worker, SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378)) + XCTAssertEqual(worker, SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378, executable: Self.trashedExecutable)) } func testParsesARuntimeWorker() { let worker = SessionWorkerProcess.parse(pid: 42, arguments: [ "/Applications/KeyPath.app/Contents/MacOS/KeyPath", "--session-runtime", "--session-owner", "41", ]) - XCTAssertEqual(worker, SessionWorkerProcess(pid: 42, mode: .runtime, ownerPID: 41)) + XCTAssertEqual(worker?.mode, .runtime) + XCTAssertEqual(worker?.ownerPID, 41) } func testIgnoresTheAppItselfAndOtherProcesses() { @@ -36,7 +37,8 @@ final class SessionWorkerSweepTests: XCTestCase { func testMissingOwnerValueParsesWithoutOwner() { let worker = SessionWorkerProcess.parse(pid: 5, arguments: [Self.trashedExecutable, "--session-runtime", "--session-owner"]) - XCTAssertEqual(worker, SessionWorkerProcess(pid: 5, mode: .runtime, ownerPID: nil)) + XCTAssertEqual(worker?.mode, .runtime) + XCTAssertNil(worker?.ownerPID) } func testParsesKernelArgumentLayout() { @@ -64,16 +66,28 @@ final class SessionWorkerSweepTests: XCTestCase { self.exitOnTerm = exitOnTerm } + /// Each sleep advances a fake clock, so graces are wall-clock bounded. + let clock = Locked(0.0) + var arrivals: [Int: SessionWorkerProcess] = [:] + private var polls = 0 + var sweep: SessionWorkerSweep { SessionWorkerSweep( - list: { self.locked { self.running } }, + list: { self.locked { + if let late = self.arrivals[self.polls] { self.running.append(late) } + self.polls += 1 + return Optional(self.running) + } }, signal: { pid, number in self.locked { self.signals.append((pid, number)) if number == SIGKILL || self.exitOnTerm.contains(pid) { self.running.removeAll { $0.pid == pid } } } }, - sleep: { _ in } + sleep: { duration in + self.clock.update { $0 += Double(duration.components.seconds) + Double(duration.components.attoseconds) / 1e18 } + }, + uptime: { self.clock.value } ) } @@ -103,8 +117,7 @@ final class SessionWorkerSweepTests: XCTestCase { func testKillsAWedgedProbeAfterItsGrace() async { let probe = SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378) let fake = FakeProcesses([probe], exitOnTerm: []) - let outcome = await fake.sweep.stopAll(probeGrace: .milliseconds(200), runtimeGrace: .seconds(1), - poll: .milliseconds(100)) + let outcome = await fake.sweep.stopAll(probeGrace: 0.2, runtimeGrace: 1, poll: .milliseconds(100)) XCTAssertTrue(outcome.succeeded) XCTAssertEqual(fake.received(SIGKILL), [13284]) } @@ -114,17 +127,98 @@ final class SessionWorkerSweepTests: XCTestCase { func testNeverKillsARuntimeWorkerAndReportsItRemaining() async { let runtime = SessionWorkerProcess(pid: 100, mode: .runtime, ownerPID: 99) let fake = FakeProcesses([runtime], exitOnTerm: []) - let outcome = await fake.sweep.stopAll(probeGrace: .milliseconds(100), runtimeGrace: .milliseconds(500), - poll: .milliseconds(100)) + let outcome = await fake.sweep.stopAll(probeGrace: 0.1, runtimeGrace: 0.5, poll: .milliseconds(100)) XCTAssertFalse(outcome.succeeded) XCTAssertEqual(outcome.remaining, [runtime]) XCTAssertTrue(fake.received(SIGKILL).isEmpty) } + /// A worker that appears after the first listing is signalled, and a late + /// probe is still killed once the probe grace has passed. + func testSignalsWorkersThatAppearMidSweep() async { + let first = SessionWorkerProcess(pid: 100, mode: .runtime, ownerPID: 99) + let late = SessionWorkerProcess(pid: 200, mode: .capabilities, ownerPID: 99) + let fake = FakeProcesses([first], exitOnTerm: [100]) + // Listed on the first poll, after the initial SIGTERM went out. + fake.arrivals[1] = late + let outcome = await fake.sweep.stopAll(probeGrace: 0.2, runtimeGrace: 2, poll: .milliseconds(100)) + XCTAssertTrue(outcome.succeeded) + XCTAssertEqual(Set(fake.received(SIGTERM)), [100, 200]) + XCTAssertEqual(fake.received(SIGKILL), [200]) + XCTAssertEqual(Set(outcome.found.map(\.pid)), [100, 200]) + } + + func testUninstallScopeIsTheRemovedBundleAndOrphans() { + let app = URL(fileURLWithPath: "/Applications/KeyPath.app") + let own = SessionWorkerProcess(pid: 1, mode: .runtime, ownerPID: 10, + executable: "/Applications/KeyPath.app/Contents/MacOS/KeyPath") + let otherLive = SessionWorkerProcess(pid: 2, mode: .runtime, ownerPID: 20, + executable: "/Users/me/dev/KeyPath.app/Contents/MacOS/KeyPath") + let orphan = SessionWorkerProcess(pid: 3, mode: .capabilities, ownerPID: 30378, + executable: Self.trashedExecutable) + let ownerless = SessionWorkerProcess(pid: 4, mode: .capabilities, ownerPID: nil, + executable: Self.trashedExecutable) + let alive: (pid_t) -> Bool = { $0 != 30378 } + XCTAssertTrue(own.belongsToUninstall(of: app, ownerAlive: alive)) + XCTAssertFalse(otherLive.belongsToUninstall(of: app, ownerAlive: alive)) + XCTAssertTrue(orphan.belongsToUninstall(of: app, ownerAlive: alive)) + XCTAssertTrue(ownerless.belongsToUninstall(of: app, ownerAlive: alive)) + // A sibling bundle whose name starts with the removed one is not inside it. + let sibling = SessionWorkerProcess(pid: 5, mode: .runtime, ownerPID: 20, + executable: "/Applications/KeyPath.app.old/Contents/MacOS/KeyPath") + XCTAssertFalse(sibling.belongsToUninstall(of: app, ownerAlive: alive)) + } + /// Reads the real process table without signalling anything: this test /// process is not a KeyPath worker, so it must never list itself. - func testLiveListingNeverIncludesThisProcess() { - XCTAssertFalse(SessionWorkerSweep.currentUserSessionWorkers().contains { $0.pid == getpid() }) - XCTAssertNotNil(SessionWorkerSweep.arguments(of: getpid())) + func testLiveListingNeverIncludesThisProcess() throws { + let workers = try XCTUnwrap(SessionWorkerSweep.currentUserSessionWorkers(), "Process table must be readable") + XCTAssertFalse(workers.contains { $0.pid == getpid() }) + var buffer = try [UInt8](repeating: 0, count: XCTUnwrap(SessionWorkerSweep.argumentMax())) + let arguments = try XCTUnwrap(SessionWorkerSweep.arguments(of: getpid(), buffer: &buffer)) + // The kernel reports the executable path, which need not equal argv[0]. + XCTAssertTrue(arguments.first?.hasPrefix("/") == true) + XCTAssertEqual(Array(arguments.dropFirst()), Array(ProcessInfo.processInfo.arguments.dropFirst())) + } + + /// An unreadable process table proves nothing, so it is not success. + func testUnreadableProcessTableIsNotSuccess() async { + let unreadable = SessionWorkerSweep(list: { nil }, signal: { _, _ in XCTFail("Nothing to signal") }, sleep: { _ in }) + let outcome = await unreadable.stopAll() + XCTAssertFalse(outcome.succeeded) + XCTAssertFalse(outcome.verified) + } + + func testTableThatBecomesUnreadableMidSweepIsNotSuccess() async { + let reads = Locked(0) + let probe = SessionWorkerProcess(pid: 13284, mode: .capabilities, ownerPID: 30378) + let sweep = SessionWorkerSweep(list: { + reads.update { $0 += 1 } + return reads.value == 1 ? [probe] : nil + }, signal: { _, _ in }, sleep: { _ in }) + let outcome = await sweep.stopAll() + XCTAssertFalse(outcome.succeeded) + XCTAssertEqual(outcome.remaining, [probe]) } + + func testMalformedKernelArgumentsDoNotCrash() { + // argc larger than the strings present: returns what is there. + var bytes = withUnsafeBytes(of: Int32(5)) { Array($0) } + bytes += Array(Self.trashedExecutable.utf8) + [0] + Array("KeyPath".utf8) + [0] + XCTAssertEqual(SessionWorkerSweep.parseProcArgs(bytes), [Self.trashedExecutable]) + // No terminating NUL after the executable path. + let unterminated = withUnsafeBytes(of: Int32(1)) { Array($0) } + Array("/x".utf8) + XCTAssertNil(SessionWorkerSweep.parseProcArgs(unterminated)) + // Non-UTF8 bytes decode with replacement rather than failing. + let invalid = withUnsafeBytes(of: Int32(1)) { Array($0) } + [0xFF, 0xFE, 0] + [0x41, 0] + XCTAssertEqual(SessionWorkerSweep.parseProcArgs(invalid)?.count, 1) + } +} + +private final class Locked: @unchecked Sendable { + private let lock = NSLock() + private var stored: Value + init(_ value: Value) { stored = value } + var value: Value { lock.lock(); defer { lock.unlock() }; return stored } + func update(_ body: (inout Value) -> Void) { lock.lock(); body(&stored); lock.unlock() } } From 672071ee46ae08e54e3e87f24961dc61ac613b25 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Wed, 4 Nov 2026 20:18:18 -0800 Subject: [PATCH 3/9] Address second review: confirm owners, separate unknown, cancel hard exit - An orphan's owner counts as alive only if that PID is a KeyPath app running the worker's own executable. A reused owner PID no longer hides an orphan from uninstall. - Uninstall distinguishes a worker that did not stop from a process table it could not read, with its own message for each, and logs every sweep including whether it was verified. - A process that exits or becomes a zombie mid-read is skipped, and one unreadable listing is retried before the sweep is unverified. - The 30s owner-exit hard exit is a cancellable work item: an orderly shutdown cancels it when it begins, and nothing is armed if one is already under way. - The final sweep's failure path documents why it does not resume. - Launch arguments are built in one place, and a test pins them to the sweep's parser. Co-Authored-By: Claude Opus 5.5 --- ...eLifecycleCoordinator+SessionRuntime.swift | 23 +++++-- .../SessionUninstallCoordinator.swift | 30 +++++++-- .../SessionRuntime/SessionOwnerWatch.swift | 34 ++++++++++ .../SessionRuntime/SessionRuntimeWorker.swift | 5 +- .../SessionRuntime/SessionWorkerSweep.swift | 61 +++++++++++++----- .../WizardProtocolConformances.swift | 10 ++- .../SessionOwnerWatchTests.swift | 27 ++++++++ .../SessionWorkerSweepTests.swift | 62 ++++++++++++++++--- .../SessionUninstallCoordinatorTests.swift | 33 +++++++++- 9 files changed, 242 insertions(+), 43 deletions(-) diff --git a/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift b/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift index c2ade0c4a..953e9b98d 100644 --- a/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift +++ b/Sources/KeyPathAppKit/Managers/ServiceLifecycleCoordinator+SessionRuntime.swift @@ -74,6 +74,19 @@ extension ServiceLifecycleCoordinator { return nil } + /// The worker's argv. Uninstall's sweep recognizes workers by these flags, + /// so a test pins them to SessionWorkerProcess.parse. + nonisolated static func sessionLaunchArguments( + capabilitiesOnly: Bool, reportPath: String, nonce: String, ownerPID: Int32, port: Int, configPath: String + ) -> [String] { + [ + capabilitiesOnly ? "--session-capabilities" : "--session-runtime", + "--session-report", reportPath, "--session-nonce", nonce, + "--session-owner", String(ownerPID), "--session-port", String(port), + "--session-config", configPath + ] + } + /// Polls about two seconds for the probe's report. A timeout, a cancelled /// wait, or a thrown error all abandon the probe, and every one of them /// signals it: on 2026-11-03 a cancelled wait skipped the signal and two @@ -812,12 +825,10 @@ extension ServiceLifecycleCoordinator { configuration.activates = false configuration.hides = true configuration.addsToRecentItems = false - configuration.arguments = [ - capabilitiesOnly ? "--session-capabilities" : "--session-runtime", - "--session-report", url.path, "--session-nonce", nonce, - "--session-owner", String(getpid()), "--session-port", String(KeyPathConstants.Networking.defaultTCPPort), - "--session-config", KeyPathConstants.Config.mainConfigPath - ] + configuration.arguments = Self.sessionLaunchArguments( + capabilitiesOnly: capabilitiesOnly, reportPath: url.path, nonce: nonce, ownerPID: getpid(), + port: KeyPathConstants.Networking.defaultTCPPort, configPath: KeyPathConstants.Config.mainConfigPath + ) sessionCapsIntermediate = nil if !capabilitiesOnly { sessionCapsPaused = nil diff --git a/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift b/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift index d68744b15..7c551f52b 100644 --- a/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift +++ b/Sources/KeyPathAppKit/Managers/SessionUninstallCoordinator.swift @@ -1,6 +1,14 @@ import AppKit import KeyPathWizardCore +/// What a sweep of this user's KeyPath session workers proved. +enum SessionWorkerSweepResult: Equatable, Sendable { + case noneRunning + case stillRunning + /// The process table could not be read, so absence is unproven. + case unverified +} + /// Removes only the current user's driverless installation. No privileged cleanup. @MainActor final class SessionUninstallCoordinator: WizardUninstalling { @@ -12,7 +20,7 @@ final class SessionUninstallCoordinator: WizardUninstalling { private let resume: () async -> Void private let trash: (URL) throws -> Void private let removeLoginItem: () async throws -> Void - private let stopSessionWorkers: () async -> Bool + private let stopSessionWorkers: () async -> SessionWorkerSweepResult private let files = FileManager.default init( @@ -23,7 +31,7 @@ final class SessionUninstallCoordinator: WizardUninstalling { prepare: @escaping () async -> Bool, resume: @escaping () async -> Void, removeLoginItem: @escaping () async throws -> Void = {}, - stopSessionWorkers: @escaping () async -> Bool = { true }, + stopSessionWorkers: @escaping () async -> SessionWorkerSweepResult = { .noneRunning }, trash: @escaping (URL) throws -> Void = { try FileManager.default.trashItem(at: $0, resultingItemURL: nil) } ) { self.home = home @@ -66,9 +74,15 @@ final class SessionUninstallCoordinator: WizardUninstalling { // prepare stops only the worker the live app tracks. Orphans of an // earlier app process, or probes it abandoned, would otherwise keep // running from the trashed bundle. - guard await stopSessionWorkers() else { + switch await stopSessionWorkers() { + case .noneRunning: + break + case .stillRunning: await resume() return WizardUninstallResult(success: false, failureReason: "A KeyPath keyboard process did not stop. Nothing was removed. Restart your Mac, then try again.") + case .unverified: + await resume() + return WizardUninstallResult(success: false, failureReason: "KeyPath could not check which of its processes are running. Nothing was removed. Try again.") } let backup = home.appendingPathComponent("Downloads/KeyPath-Uninstall-Backup-\(UUID().uuidString)", isDirectory: true) @@ -111,10 +125,16 @@ final class SessionUninstallCoordinator: WizardUninstalling { throw CocoaError(.fileWriteUnknown) } // Success means nothing is left running, so check again at the end. - guard await stopSessionWorkers() else { + // No resume() here, unlike the paths above: the app and its settings + // are gone, so there is nothing to resume watching or starting. + let finalSweep = await stopSessionWorkers() + guard finalSweep == .noneRunning else { + let state = finalSweep == .stillRunning + ? "a KeyPath keyboard process is still running. Restart your Mac to stop it." + : "KeyPath could not check that its processes stopped. Restart your Mac to be sure." return WizardUninstallResult( success: false, - failureReason: "KeyPath was removed, but a KeyPath keyboard process is still running. Restart your Mac to stop it. Your backup is at \(backup.path).", + failureReason: "KeyPath was removed, but \(state) Your backup is at \(backup.path).", logs: ["Backup saved to \(backup.path)"] ) } diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift index 293cb5c09..57f162021 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift @@ -59,3 +59,37 @@ final class SessionOwnerWatch: @unchecked Sendable { lock.unlock() } } + +/// Bounds an owner-exit retirement that the worker's run loop never runs. An +/// orderly shutdown cancels it when it begins, so a slow Caps restore or key +/// release is never cut off; the HID join it waits on stays unbounded. +final class SessionShutdownFallback: @unchecked Sendable { + static let shared = SessionShutdownFallback() + + private let lock = NSLock() + private var shutdownBegan = false + private var pending: DispatchWorkItem? + + /// Arms `action` after `grace`. Returns false, arming nothing, when an + /// orderly shutdown already began. + @discardableResult + func arm(after grace: TimeInterval, queue: DispatchQueue = .global(qos: .utility), + action: @escaping @Sendable () -> Void) -> Bool + { + lock.lock() + defer { lock.unlock() } + guard !shutdownBegan, pending == nil else { return !shutdownBegan } + let item = DispatchWorkItem(block: action) + pending = item + queue.asyncAfter(deadline: .now() + grace, execute: item) + return true + } + + func shutdownBegins() { + lock.lock() + shutdownBegan = true + pending?.cancel() + pending = nil + lock.unlock() + } +} diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift index fb676e12e..dfea5584a 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift @@ -142,12 +142,13 @@ public final class SessionRuntimeWorker { // retires it on its run loop, which releases them before exiting. A // run loop that never gets there would keep an orphan forever; after // the grace it exits anyway, and the Caps journal survives for recovery. + // A shutdown already under way, or one this starts, cancels that exit. ownerWatch.setHandler { [weak worker] _ in + guard SessionShutdownFallback.shared.arm(after: ownerExitShutdownGrace, action: { _exit(1) }) else { return } CFRunLoopPerformBlock(CFRunLoopGetMain(), CFRunLoopMode.commonModes.rawValue) { MainActor.assumeIsolated { worker?.retire(reason: "owner-exited") } } CFRunLoopWakeUp(CFRunLoopGetMain()) - DispatchQueue.global(qos: .utility).asyncAfter(deadline: .now() + ownerExitShutdownGrace) { _exit(1) } } worker.start(configPath: config) // Keep the worker and loaded bridge alive through process termination; @@ -1183,6 +1184,8 @@ public final class SessionRuntimeWorker { acknowledge: (() -> Void)? = nil ) -> Never { finished = true + // The orderly path owns shutdown from here; no hard exit may cut it off. + SessionShutdownFallback.shared.shutdownBegins() // Every exit records why, so a stop with no failure is still explainable. AppLogger.shared.log("🛑 [SessionWorker] Finishing: state=\(state) reason=\(reason ?? "none")") AppLogger.shared.flushBuffer() diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift index a56fafac4..dd41594c7 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift @@ -1,6 +1,5 @@ import Darwin import Foundation -import KeyPathCore /// One KeyPath session worker found in this user's process table. struct SessionWorkerProcess: Equatable, Sendable { @@ -34,10 +33,13 @@ struct SessionWorkerProcess: Equatable, Sendable { /// Uninstall stops workers of the bundle it removes, and orphans of any /// bundle; a separate KeyPath install whose app is alive keeps its worker. - func belongsToUninstall(of app: URL, ownerAlive: (pid_t) -> Bool) -> Bool { + /// The owner counts as alive only if that PID is a KeyPath app running the + /// worker's own executable: orphans outlive owners by hours, long enough + /// for the PID to be reused by an unrelated process. + func belongsToUninstall(of app: URL, appExecutable: (pid_t) -> String?) -> Bool { if executable.hasPrefix(app.standardizedFileURL.path + "/") { return true } guard let ownerPID, ownerPID > 0 else { return true } - return !ownerAlive(ownerPID) + return appExecutable(ownerPID) != executable } } @@ -51,6 +53,10 @@ struct SessionWorkerSweep: Sendable { /// False when the process table could not be read: absence unproven. var verified = true var succeeded: Bool { verified && remaining.isEmpty } + var result: SessionWorkerSweepResult { + guard verified else { return .unverified } + return remaining.isEmpty ? .noneRunning : .stillRunning + } } /// nil means the process table could not be read, never "no workers". @@ -62,9 +68,8 @@ struct SessionWorkerSweep: Sendable { static func live(uninstalling app: URL) -> SessionWorkerSweep { SessionWorkerSweep( list: { - currentUserSessionWorkers()?.filter { - $0.belongsToUninstall(of: app) { SystemStateProvider.isProcessAlive(pid: $0) } - } + guard let table = currentUserKeyPathProcesses() else { return nil } + return table.workers.filter { $0.belongsToUninstall(of: app) { table.apps[$0] } } }, signal: { pid, number in _ = kill(pid, number) }, sleep: { try? await Task.sleep(for: $0) } @@ -78,7 +83,9 @@ struct SessionWorkerSweep: Sendable { func stopAll(probeGrace: TimeInterval = 2, runtimeGrace: TimeInterval = 10, poll: Duration = .milliseconds(100)) async -> Outcome { - guard let found = list() else { return Outcome(found: [], remaining: [], verified: false) } + // One unreadable listing may be a process exiting mid-read; two is unknown. + func read() -> [SessionWorkerProcess]? { list() ?? list() } + guard let found = read() else { return Outcome(found: [], remaining: [], verified: false) } guard !found.isEmpty else { return Outcome(found: [], remaining: []) } let started = uptime() var termed: Set = [] @@ -99,15 +106,26 @@ struct SessionWorkerSweep: Sendable { } guard !remaining.isEmpty, elapsed < runtimeGrace else { break } await sleep(poll) - guard let listed = list() else { return Outcome(found: everFound, remaining: remaining, verified: false) } + guard let listed = read() else { return Outcome(found: everFound, remaining: remaining, verified: false) } remaining = listed everFound += listed.filter { worker in !everFound.contains { $0.pid == worker.pid } } } return Outcome(found: everFound, remaining: remaining) } - /// Reads the kernel process table directly: no pgrep, and only this UID. + /// This UID's KeyPath processes: session workers, and the apps (any other + /// KeyPath process) by PID with their executable path. + struct ProcessTable: Equatable, Sendable { + var workers: [SessionWorkerProcess] = [] + var apps: [pid_t: String] = [:] + } + static func currentUserSessionWorkers() -> [SessionWorkerProcess]? { + currentUserKeyPathProcesses()?.workers + } + + /// Reads the kernel process table directly: no pgrep, and only this UID. + static func currentUserKeyPathProcesses() -> ProcessTable? { var mib: [Int32] = [CTL_KERN, KERN_PROC, KERN_PROC_UID, Int32(bitPattern: getuid())] var processes: [kinfo_proc] = [] var size = 0 @@ -126,7 +144,7 @@ struct SessionWorkerSweep: Sendable { // One buffer for every process: kern.argmax is 1 MiB. guard let argumentMax = argumentMax() else { return nil } var buffer = [UInt8](repeating: 0, count: argumentMax) - var workers: [SessionWorkerProcess] = [] + var table = ProcessTable() for process in processes.prefix(count) { let pid = process.kp_proc.p_pid // Only KeyPath processes pay for an argument read. A zombie has no @@ -134,13 +152,28 @@ struct SessionWorkerSweep: Sendable { guard pid > 0, pid != me, Int32(process.kp_proc.p_stat) != SZOMB, command(of: process) == "KeyPath" else { continue } guard let arguments = arguments(of: pid, buffer: &buffer) else { - // Gone between the two reads, or unreadable: unreadable is unknown. - if kill(pid, 0) == 0 || errno == EPERM { return nil } + // Exited or became a zombie between the two reads: it runs + // nothing. A live process whose arguments cannot be read is unknown. + if isRunning(pid) { return nil } continue } - if let worker = SessionWorkerProcess.parse(pid: pid, arguments: arguments) { workers.append(worker) } + if let worker = SessionWorkerProcess.parse(pid: pid, arguments: arguments) { + table.workers.append(worker) + } else if let executable = arguments.first { + table.apps[pid] = executable + } } - return workers + return table + } + + /// True while the PID exists and is not a zombie. Signalling a zombie + /// succeeds, so kill(pid, 0) cannot tell. + private static func isRunning(_ pid: pid_t) -> Bool { + var mib: [Int32] = [CTL_KERN, KERN_PROC, KERN_PROC_PID, pid] + var process = kinfo_proc() + var size = MemoryLayout.stride + guard sysctl(&mib, 4, &process, &size, nil, 0) == 0, size > 0 else { return false } + return Int32(process.kp_proc.p_stat) != SZOMB } private static func command(of process: kinfo_proc) -> String { diff --git a/Sources/KeyPathAppKit/WizardProtocolConformances.swift b/Sources/KeyPathAppKit/WizardProtocolConformances.swift index 0a269b756..bc5cb5776 100644 --- a/Sources/KeyPathAppKit/WizardProtocolConformances.swift +++ b/Sources/KeyPathAppKit/WizardProtocolConformances.swift @@ -165,12 +165,10 @@ public func configureWizardDependencies(runtimeCoordinator: RuntimeCoordinator) try await LaunchAtLoginController.shared.setEnabled(false) }, stopSessionWorkers: { let outcome = await SessionWorkerSweep.live(uninstalling: Bundle.main.bundleURL).stopAll() - if !outcome.found.isEmpty { - AppLogger.shared.log( - "[Uninstall] Session workers found=\(outcome.found.map(\.pid)) remaining=\(outcome.remaining.map(\.pid))" - ) - } - return outcome.succeeded + AppLogger.shared.log( + "[Uninstall] Session worker sweep verified=\(outcome.verified) found=\(outcome.found.map(\.pid)) remaining=\(outcome.remaining.map(\.pid))" + ) + return outcome.result }) } WizardDependencies.executePrivilegedBatch = { batch in diff --git a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift index 752e58ee2..a07a18362 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift @@ -66,6 +66,33 @@ final class SessionOwnerWatchTests: XCTestCase { } } +final class SessionShutdownFallbackTests: XCTestCase { + func testOrderlyShutdownCancelsTheHardExit() { + let fallback = SessionShutdownFallback() + let fired = expectation(description: "Hard exit must not run") + fired.isInverted = true + XCTAssertTrue(fallback.arm(after: 0.05) { fired.fulfill() }) + fallback.shutdownBegins() + wait(for: [fired], timeout: 0.3) + } + + /// An owner exit during a shutdown already under way arms nothing. + func testNothingIsArmedOnceShutdownBegan() { + let fallback = SessionShutdownFallback() + fallback.shutdownBegins() + XCTAssertFalse(fallback.arm(after: 0) { XCTFail("Must not run") }) + } + + func testArmedFallbackRunsWhenNoShutdownStarts() { + let fallback = SessionShutdownFallback() + let fired = expectation(description: "Hard exit runs") + XCTAssertTrue(fallback.arm(after: 0.01) { fired.fulfill() }) + // A second owner verdict does not arm a second exit. + XCTAssertTrue(fallback.arm(after: 0.01) { XCTFail("Armed twice") }) + wait(for: [fired], timeout: 2) + } +} + private final class Locked: @unchecked Sendable { private let lock = NSLock() private var stored: Value diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index 178cbc7ee..bdf0cf94a 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -150,23 +150,69 @@ final class SessionWorkerSweepTests: XCTestCase { func testUninstallScopeIsTheRemovedBundleAndOrphans() { let app = URL(fileURLWithPath: "/Applications/KeyPath.app") + let dev = "/Users/me/dev/KeyPath.app/Contents/MacOS/KeyPath" let own = SessionWorkerProcess(pid: 1, mode: .runtime, ownerPID: 10, executable: "/Applications/KeyPath.app/Contents/MacOS/KeyPath") - let otherLive = SessionWorkerProcess(pid: 2, mode: .runtime, ownerPID: 20, - executable: "/Users/me/dev/KeyPath.app/Contents/MacOS/KeyPath") + let otherLive = SessionWorkerProcess(pid: 2, mode: .runtime, ownerPID: 20, executable: dev) let orphan = SessionWorkerProcess(pid: 3, mode: .capabilities, ownerPID: 30378, executable: Self.trashedExecutable) let ownerless = SessionWorkerProcess(pid: 4, mode: .capabilities, ownerPID: nil, executable: Self.trashedExecutable) - let alive: (pid_t) -> Bool = { $0 != 30378 } - XCTAssertTrue(own.belongsToUninstall(of: app, ownerAlive: alive)) - XCTAssertFalse(otherLive.belongsToUninstall(of: app, ownerAlive: alive)) - XCTAssertTrue(orphan.belongsToUninstall(of: app, ownerAlive: alive)) - XCTAssertTrue(ownerless.belongsToUninstall(of: app, ownerAlive: alive)) + let apps: [pid_t: String] = [10: "/Applications/KeyPath.app/Contents/MacOS/KeyPath", 20: dev] + XCTAssertTrue(own.belongsToUninstall(of: app) { apps[$0] }) + XCTAssertFalse(otherLive.belongsToUninstall(of: app) { apps[$0] }) + XCTAssertTrue(orphan.belongsToUninstall(of: app) { apps[$0] }) + XCTAssertTrue(ownerless.belongsToUninstall(of: app) { apps[$0] }) // A sibling bundle whose name starts with the removed one is not inside it. let sibling = SessionWorkerProcess(pid: 5, mode: .runtime, ownerPID: 20, executable: "/Applications/KeyPath.app.old/Contents/MacOS/KeyPath") - XCTAssertFalse(sibling.belongsToUninstall(of: app, ownerAlive: alive)) + XCTAssertTrue(sibling.belongsToUninstall(of: app) { apps[$0] }, "Owner 20 runs a different bundle") + } + + /// The 2026-11-03 orphans outlived their owner by hours. A reused owner + /// PID now belonging to an unrelated process, or to another KeyPath + /// bundle, does not make the orphan look owned. + func testReusedOwnerPIDDoesNotHideAnOrphan() { + let app = URL(fileURLWithPath: "/Applications/KeyPath.app") + let orphan = SessionWorkerProcess(pid: 13284, mode: .runtime, ownerPID: 30378, + executable: Self.trashedExecutable) + // PID 30378 is now some non-KeyPath process: not in the app table at all. + XCTAssertTrue(orphan.belongsToUninstall(of: app) { _ in nil }) + // PID 30378 is now a KeyPath app from a different bundle. + XCTAssertTrue(orphan.belongsToUninstall(of: app) { _ in "/Users/me/dev/KeyPath.app/Contents/MacOS/KeyPath" }) + } + + /// The parser recognizes exactly the argv the app launches workers with. + func testParsesTheRealLaunchArguments() { + for probe in [true, false] { + let arguments = ServiceLifecycleCoordinator.sessionLaunchArguments( + capabilitiesOnly: probe, reportPath: "/tmp/r/report.json", nonce: UUID().uuidString, + ownerPID: 30378, port: 37001, configPath: "/Users/me/.config/keypath/keypath.kbd" + ) + let worker = SessionWorkerProcess.parse(pid: 1, arguments: [Self.trashedExecutable] + arguments) + XCTAssertEqual(worker?.mode, probe ? .capabilities : .runtime) + XCTAssertEqual(worker?.ownerPID, 30378) + } + } + + /// One unreadable listing is retried; a process exiting mid-read must not + /// make the whole sweep unverified. + func testSingleUnreadableListingIsRetried() async { + let reads = Locked(0) + let sweep = SessionWorkerSweep(list: { + reads.update { $0 += 1 } + return reads.value == 1 ? nil : [] + }, signal: { _, _ in XCTFail("Nothing to signal") }, sleep: { _ in }) + let outcome = await sweep.stopAll() + XCTAssertTrue(outcome.succeeded) + XCTAssertEqual(outcome.result, .noneRunning) + } + + func testOutcomeResultSeparatesRunningFromUnverified() { + let worker = SessionWorkerProcess(pid: 1, mode: .runtime, ownerPID: 2) + XCTAssertEqual(SessionWorkerSweep.Outcome(found: [worker], remaining: []).result, .noneRunning) + XCTAssertEqual(SessionWorkerSweep.Outcome(found: [worker], remaining: [worker]).result, .stillRunning) + XCTAssertEqual(SessionWorkerSweep.Outcome(found: [], remaining: [], verified: false).result, .unverified) } /// Reads the real process table without signalling anything: this test diff --git a/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift b/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift index de32cdf3b..a8fbfa104 100644 --- a/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift +++ b/Tests/KeyPathTests/Uninstall/SessionUninstallCoordinatorTests.swift @@ -92,7 +92,7 @@ final class SessionUninstallCoordinatorTests: XCTestCase { var resumed = false let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, prepare: { true }, resume: { resumed = true }, - stopSessionWorkers: { false }, + stopSessionWorkers: { .stillRunning }, trash: { _ in XCTFail("Must not trash while a worker runs") }) let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) XCTAssertFalse(result.success) @@ -102,12 +102,39 @@ final class SessionUninstallCoordinatorTests: XCTestCase { XCTAssertTrue(FileManager.default.fileExists(atPath: app.path)) } + /// An unreadable process table is not reported as a running process. + func testUnverifiedSweepRemovesNothingAndSaysWhy() async { + var resumed = false + let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, + prepare: { true }, resume: { resumed = true }, + stopSessionWorkers: { .unverified }, + trash: { _ in XCTFail("Must not trash an unverified state") }) + let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) + XCTAssertFalse(result.success) + XCTAssertTrue(resumed) + XCTAssertTrue(result.failureReason?.contains("could not check") == true) + XCTAssertFalse(result.failureReason?.contains("did not stop") == true) + } + + /// The final sweep runs after the app and settings are gone, so a failure + /// there must not resume watching or starting anything. + func testFinalSweepFailureDoesNotResume() async { + var sweeps = 0 + let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, + prepare: { true }, resume: { XCTFail("Nothing is left to resume") }, + stopSessionWorkers: { sweeps += 1; return sweeps == 1 ? .noneRunning : .unverified }, + trash: { try FileManager.default.removeItem(at: $0) }) + let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) + XCTAssertFalse(result.success) + XCTAssertTrue(result.failureReason?.contains("could not check") == true) + } + /// Success is reported only after a final sweep finds no worker left. func testWorkerFoundAfterRemovalIsNotReportedAsSuccess() async { var sweeps = 0 let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, prepare: { true }, resume: {}, - stopSessionWorkers: { sweeps += 1; return sweeps == 1 }, + stopSessionWorkers: { sweeps += 1; return sweeps == 1 ? .noneRunning : .stillRunning }, trash: { try FileManager.default.removeItem(at: $0) }) let result = await coordinator.performUninstall(deleteConfig: true, removeVirtualHID: false, allowAdminFallback: false) XCTAssertEqual(sweeps, 2) @@ -121,7 +148,7 @@ final class SessionUninstallCoordinatorTests: XCTestCase { var sweeps = 0 let coordinator = SessionUninstallCoordinator(home: root, app: app, defaults: defaults, domain: domain, prepare: { true }, resume: { XCTFail("Unexpected failure") }, - stopSessionWorkers: { sweeps += 1; return true }, + stopSessionWorkers: { sweeps += 1; return .noneRunning }, trash: { url in sweepsBeforeTrash = sweeps try FileManager.default.removeItem(at: url) From 854853f28f70f4bf78a7c462c9504edd9f7eeb58 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Wed, 4 Nov 2026 20:30:51 -0800 Subject: [PATCH 4/9] Pin the bundle-boundary rule and stop a cancelled sweep - The sibling-bundle test now fails if the trailing "/" in the bundle prefix check is dropped: its owner is live and matching, so only the boundary rule can exclude it. - stopAll returns unverified on cancellation instead of re-reading the process table at full speed for the whole grace. - The process-table read never claims more bytes than its buffer holds. - A runtime worker's owner backstop polls every 2s with leeway; its 5 ms tick already covers the live case. - SessionShutdownFallback.arm documents what its result means. Co-Authored-By: Claude Opus 5.5 --- .../SessionRuntime/SessionOwnerWatch.swift | 7 +++--- .../SessionRuntime/SessionRuntimeWorker.swift | 5 +++- .../SessionRuntime/SessionWorkerSweep.swift | 5 ++++ .../SessionWorkerSweepTests.swift | 24 +++++++++++++++---- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift index 57f162021..7124d7ff7 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift @@ -37,7 +37,7 @@ final class SessionOwnerWatch: @unchecked Sendable { timer = DispatchSource.makeTimerSource(queue: queue) let started = uptime() // The first check runs immediately: the owner may already be gone. - timer.schedule(deadline: .now(), repeating: interval) + timer.schedule(deadline: .now(), repeating: interval, leeway: .milliseconds(Int(interval * 250))) timer.setEventHandler { [self] in guard let verdict = Self.verdict(ownerPID: ownerPID, ownerAlive: isAlive(ownerPID), elapsed: uptime() - started, deadline: deadline) @@ -70,8 +70,9 @@ final class SessionShutdownFallback: @unchecked Sendable { private var shutdownBegan = false private var pending: DispatchWorkItem? - /// Arms `action` after `grace`. Returns false, arming nothing, when an - /// orderly shutdown already began. + /// Arms `action` after `grace`, once. Returns whether an owner-exit + /// retirement should proceed: false when an orderly shutdown already began, + /// true otherwise, including when an earlier call already armed it. @discardableResult func arm(after grace: TimeInterval, queue: DispatchQueue = .global(qos: .utility), action: @escaping @Sendable () -> Void) -> Bool diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift index dfea5584a..0747ba1d8 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionRuntimeWorker.swift @@ -108,8 +108,11 @@ public final class SessionRuntimeWorker { // Until the runtime can own outputs, nothing needs releasing: a dead // owner, or a probe still running after its parent stopped waiting, // ends the process at once. _exit cannot block on a wedged main thread. + // A runtime's tick already checks the owner every 5 ms; this backstop + // for a wedged run loop polls slowly, since the 30 s grace dominates. let ownerWatch = SessionOwnerWatch(ownerPID: ownerPID, - deadline: probeOnly ? capabilityProbeDeadline : nil) { _ in _exit(0) } + deadline: probeOnly ? capabilityProbeDeadline : nil, + interval: probeOnly ? 0.25 : 2) { _ in _exit(0) } if probeOnly { await writeCapabilityReport( diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift index dd41594c7..df7c117a3 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift @@ -93,6 +93,9 @@ struct SessionWorkerSweep: Sendable { var everFound = found var remaining = found while true { + // A cancelled sleep returns at once; without this the loop would + // re-read the process table as fast as it can for the whole grace. + guard !Task.isCancelled else { return Outcome(found: everFound, remaining: remaining, verified: false) } let elapsed = uptime() - started for worker in remaining where !termed.contains(worker.pid) { termed.insert(worker.pid) @@ -134,6 +137,8 @@ struct SessionWorkerSweep: Sendable { guard sysctl(&mib, 4, nil, &size, nil, 0) == 0, size > 0 else { return nil } size += 32 * MemoryLayout.stride processes = [kinfo_proc](repeating: kinfo_proc(), count: size / MemoryLayout.stride) + // Never claim more bytes than the array holds. + size = processes.count * MemoryLayout.stride if sysctl(&mib, 4, &processes, &size, nil, 0) == 0 { break } guard errno == ENOMEM else { return nil } processes = [] diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index bdf0cf94a..9e59e101e 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -163,10 +163,12 @@ final class SessionWorkerSweepTests: XCTestCase { XCTAssertFalse(otherLive.belongsToUninstall(of: app) { apps[$0] }) XCTAssertTrue(orphan.belongsToUninstall(of: app) { apps[$0] }) XCTAssertTrue(ownerless.belongsToUninstall(of: app) { apps[$0] }) - // A sibling bundle whose name starts with the removed one is not inside it. - let sibling = SessionWorkerProcess(pid: 5, mode: .runtime, ownerPID: 20, - executable: "/Applications/KeyPath.app.old/Contents/MacOS/KeyPath") - XCTAssertTrue(sibling.belongsToUninstall(of: app) { apps[$0] }, "Owner 20 runs a different bundle") + // A sibling bundle whose name starts with the removed one is not inside + // it: its live, matching owner keeps its worker. + let siblingExecutable = "/Applications/KeyPath.app.old/Contents/MacOS/KeyPath" + let sibling = SessionWorkerProcess(pid: 5, mode: .runtime, ownerPID: 21, executable: siblingExecutable) + XCTAssertFalse(sibling.belongsToUninstall(of: app) { [21: siblingExecutable][$0] }, + "A sibling bundle is not inside the removed one") } /// The 2026-11-03 orphans outlived their owner by hours. A reused owner @@ -208,6 +210,20 @@ final class SessionWorkerSweepTests: XCTestCase { XCTAssertEqual(outcome.result, .noneRunning) } + /// A cancelled sweep stops at once instead of re-reading the table for + /// the whole grace, and claims nothing it did not verify. + func testCancelledSweepStopsUnverified() async { + let reads = Locked(0) + let runtime = SessionWorkerProcess(pid: 100, mode: .runtime, ownerPID: 99) + let sweep = SessionWorkerSweep(list: { reads.update { $0 += 1 }; return [runtime] }, + signal: { _, _ in }, sleep: { _ in }) + let task = Task { await sweep.stopAll() } + task.cancel() + let outcome = await task.value + XCTAssertEqual(outcome.result, .unverified) + XCTAssertLessThanOrEqual(reads.value, 2) + } + func testOutcomeResultSeparatesRunningFromUnverified() { let worker = SessionWorkerProcess(pid: 1, mode: .runtime, ownerPID: 2) XCTAssertEqual(SessionWorkerSweep.Outcome(found: [worker], remaining: []).result, .noneRunning) From 30dbefd977cf538cd09edd6316f54db518a2a299 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Thu, 5 Nov 2026 21:34:35 -0800 Subject: [PATCH 5/9] Format the PR's files with the pinned SwiftFormat 0.61.1 Co-Authored-By: Claude Opus 5.5 --- .../SessionRuntime/SessionWorkerSweep.swift | 11 ++++++--- .../SessionOwnerWatchTests.swift | 20 ++++++++++++---- .../SessionWorkerSweepTests.swift | 23 +++++++++++++++---- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift index df7c117a3..ad54d2b24 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift @@ -52,7 +52,10 @@ struct SessionWorkerSweep: Sendable { let remaining: [SessionWorkerProcess] /// False when the process table could not be read: absence unproven. var verified = true - var succeeded: Bool { verified && remaining.isEmpty } + var succeeded: Bool { + verified && remaining.isEmpty + } + var result: SessionWorkerSweepResult { guard verified else { return .unverified } return remaining.isEmpty ? .noneRunning : .stillRunning @@ -83,8 +86,10 @@ struct SessionWorkerSweep: Sendable { func stopAll(probeGrace: TimeInterval = 2, runtimeGrace: TimeInterval = 10, poll: Duration = .milliseconds(100)) async -> Outcome { - // One unreadable listing may be a process exiting mid-read; two is unknown. - func read() -> [SessionWorkerProcess]? { list() ?? list() } + /// One unreadable listing may be a process exiting mid-read; two is unknown. + func read() -> [SessionWorkerProcess]? { + list() ?? list() + } guard let found = read() else { return Outcome(found: [], remaining: [], verified: false) } guard !found.isEmpty else { return Outcome(found: [], remaining: []) } let started = uptime() diff --git a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift index a07a18362..d73db8564 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift @@ -28,7 +28,8 @@ final class SessionOwnerWatchTests: XCTestCase { let fired = expectation(description: "Owner exit observed") let calls = Locked(0) let watch = SessionOwnerWatch(ownerPID: 30378, deadline: nil, interval: 0.01, - isAlive: { _ in alive.value }) { verdict in + isAlive: { _ in alive.value }) + { verdict in XCTAssertFalse(Thread.isMainThread) XCTAssertEqual(verdict, .ownerExited) calls.update { $0 += 1 } @@ -56,7 +57,8 @@ final class SessionOwnerWatchTests: XCTestCase { let now = Locked(100.0) let fired = expectation(description: "Deadline observed") let watch = SessionOwnerWatch(ownerPID: 30378, deadline: 15, interval: 0.01, - isAlive: { _ in true }, uptime: { now.value }) { verdict in + isAlive: { _ in true }, uptime: { now.value }) + { verdict in XCTAssertEqual(verdict, .deadlineExceeded) fired.fulfill() } @@ -96,7 +98,15 @@ final class SessionShutdownFallbackTests: XCTestCase { private final class Locked: @unchecked Sendable { private let lock = NSLock() private var stored: Value - init(_ value: Value) { stored = value } - var value: Value { lock.lock(); defer { lock.unlock() }; return stored } - func update(_ body: (inout Value) -> Void) { lock.lock(); body(&stored); lock.unlock() } + init(_ value: Value) { + stored = value + } + + var value: Value { + lock.lock(); defer { lock.unlock() }; return stored + } + + func update(_ body: (inout Value) -> Void) { + lock.lock(); body(&stored); lock.unlock() + } } diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index 9e59e101e..f1667f003 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -91,8 +91,13 @@ final class SessionWorkerSweepTests: XCTestCase { ) } - func locked(_ body: () -> T) -> T { lock.lock(); defer { lock.unlock() }; return body() } - func received(_ number: Int32) -> [pid_t] { locked { signals.filter { $0.1 == number }.map(\.0) } } + func locked(_ body: () -> T) -> T { + lock.lock(); defer { lock.unlock() }; return body() + } + + func received(_ number: Int32) -> [pid_t] { + locked { signals.filter { $0.1 == number }.map(\.0) } + } } func testNoWorkersSucceedsWithoutSignalling() async { @@ -280,7 +285,15 @@ final class SessionWorkerSweepTests: XCTestCase { private final class Locked: @unchecked Sendable { private let lock = NSLock() private var stored: Value - init(_ value: Value) { stored = value } - var value: Value { lock.lock(); defer { lock.unlock() }; return stored } - func update(_ body: (inout Value) -> Void) { lock.lock(); body(&stored); lock.unlock() } + init(_ value: Value) { + stored = value + } + + var value: Value { + lock.lock(); defer { lock.unlock() }; return stored + } + + func update(_ body: (inout Value) -> Void) { + lock.lock(); body(&stored); lock.unlock() + } } From 4a7623eb6c2978a36cc0d139cf80f0c12c875fd9 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Thu, 5 Nov 2026 21:44:09 -0800 Subject: [PATCH 6/9] Deliver owner verdicts under the lock and drop the test sleep The watch copied its handler, released the lock and then called it, so an owner exit during startup could run the hard-exit handler after setHandler had installed the orderly one, skipping Caps restoration. It now delivers under the lock, and setHandler waits for a verdict in flight. The duplicate-callback test drains the watch's own serial queue instead of sleeping, which the no-sleep lint rejects. Co-Authored-By: Claude Opus 5.5 --- .../SessionRuntime/SessionOwnerWatch.swift | 10 ++++-- .../SessionOwnerWatchTests.swift | 35 +++++++++++++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift index 7124d7ff7..bd0d77b53 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionOwnerWatch.swift @@ -43,16 +43,20 @@ final class SessionOwnerWatch: @unchecked Sendable { elapsed: uptime() - started, deadline: deadline) else { return } timer.cancel() + // Deliver under the lock: once setHandler returns, a verdict + // already in flight has either run the old handler (and exited) + // or will run the new one, never the old one late. Both handlers + // return at once, so holding the lock cannot stall setHandler. lock.lock() - let current = self.handler + self.handler(verdict) lock.unlock() - current(verdict) } timer.resume() } /// The runtime hands off from a hard exit to an orderly shutdown once it - /// may own outputs or a Caps mapping. + /// may own outputs or a Caps mapping. Blocks while a verdict is being + /// delivered, so the old handler cannot run after this returns. func setHandler(_ handler: @escaping @Sendable (Verdict) -> Void) { lock.lock() self.handler = handler diff --git a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift index d73db8564..63bc2f704 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionOwnerWatchTests.swift @@ -27,8 +27,9 @@ final class SessionOwnerWatchTests: XCTestCase { let alive = Locked(true) let fired = expectation(description: "Owner exit observed") let calls = Locked(0) + let queue = DispatchQueue(label: "owner-watch-test") let watch = SessionOwnerWatch(ownerPID: 30378, deadline: nil, interval: 0.01, - isAlive: { _ in alive.value }) + isAlive: { _ in alive.value }, queue: queue) { verdict in XCTAssertFalse(Thread.isMainThread) XCTAssertEqual(verdict, .ownerExited) @@ -37,12 +38,40 @@ final class SessionOwnerWatchTests: XCTestCase { } alive.update { $0 = false } wait(for: [fired], timeout: 2) - // A cancelled timer never delivers a second verdict. - Thread.sleep(forTimeInterval: 0.1) + // The verdict cancels the timer on its own serial queue, so once that + // queue drains no further tick can be pending. + queue.sync {} XCTAssertEqual(calls.value, 1) withExtendedLifetime(watch) {} } + /// setHandler waits for a verdict being delivered, so the startup + /// hard-exit handler can never run after the runtime handler is installed. + func testSetHandlerWaitsForAVerdictInFlight() { + let alive = Locked(true) + let inOldHandler = DispatchSemaphore(value: 0) + let releaseOldHandler = DispatchSemaphore(value: 0) + let order = Locked<[String]>([]) + let watch = SessionOwnerWatch(ownerPID: 30378, deadline: nil, interval: 0.01, + isAlive: { _ in alive.value }) + { _ in + order.update { $0.append("old") } + inOldHandler.signal() + releaseOldHandler.wait() + } + alive.update { $0 = false } + XCTAssertEqual(inOldHandler.wait(timeout: .now() + 2), .success) + let swapped = expectation(description: "setHandler returned") + DispatchQueue.global().async { + watch.setHandler { _ in order.update { $0.append("new") } } + order.update { $0.append("swapped") } + swapped.fulfill() + } + releaseOldHandler.signal() + wait(for: [swapped], timeout: 2) + XCTAssertEqual(order.value, ["old", "swapped"]) + } + func testReplacedHandlerReceivesTheVerdict() { let alive = Locked(true) let fired = expectation(description: "Replacement handler called") From 4973b85554917bdd00fb3070a8336f39ff0a849d Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Thu, 5 Nov 2026 21:45:52 -0800 Subject: [PATCH 7/9] Check only the shape of the live argv read Test runners may rewrite argv, so comparing it with ProcessInfo.arguments could flake. Co-Authored-By: Claude Opus 5.5 --- .../KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index f1667f003..ab656a5d4 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -243,9 +243,9 @@ final class SessionWorkerSweepTests: XCTestCase { XCTAssertFalse(workers.contains { $0.pid == getpid() }) var buffer = try [UInt8](repeating: 0, count: XCTUnwrap(SessionWorkerSweep.argumentMax())) let arguments = try XCTUnwrap(SessionWorkerSweep.arguments(of: getpid(), buffer: &buffer)) - // The kernel reports the executable path, which need not equal argv[0]. + // The kernel reports the executable path, which need not equal argv[0], + // and test runners may rewrite argv, so only check the shape. XCTAssertTrue(arguments.first?.hasPrefix("/") == true) - XCTAssertEqual(Array(arguments.dropFirst()), Array(ProcessInfo.processInfo.arguments.dropFirst())) } /// An unreadable process table proves nothing, so it is not success. From fcacc87e19eb8e5b0a45a26c4ecc5e21fb1af0b7 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Thu, 5 Nov 2026 21:48:34 -0800 Subject: [PATCH 8/9] Match the removed bundle by its resolved path too The kernel reports a worker's resolved executable path, while the bundle URL may reach it through a symlink. Co-Authored-By: Claude Opus 5.5 --- .../SessionRuntime/SessionWorkerSweep.swift | 5 ++++- .../SessionRuntime/SessionWorkerSweepTests.swift | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift index ad54d2b24..e6c44c241 100644 --- a/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift +++ b/Sources/KeyPathAppKit/Services/SessionRuntime/SessionWorkerSweep.swift @@ -37,7 +37,10 @@ struct SessionWorkerProcess: Equatable, Sendable { /// worker's own executable: orphans outlive owners by hours, long enough /// for the PID to be reused by an unrelated process. func belongsToUninstall(of app: URL, appExecutable: (pid_t) -> String?) -> Bool { - if executable.hasPrefix(app.standardizedFileURL.path + "/") { return true } + // The kernel reports a resolved path; the bundle URL may reach it + // through a symlink. + let bundles = Set([app.standardizedFileURL.path, app.resolvingSymlinksInPath().path]) + if bundles.contains(where: { executable.hasPrefix($0 + "/") }) { return true } guard let ownerPID, ownerPID > 0 else { return true } return appExecutable(ownerPID) != executable } diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index ab656a5d4..8d3c23d0a 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -176,6 +176,20 @@ final class SessionWorkerSweepTests: XCTestCase { "A sibling bundle is not inside the removed one") } + /// A bundle reached through a symlink still claims its workers, which the + /// kernel reports at the resolved path, even when a matching owner is live. + func testSymlinkedBundleClaimsItsResolvedWorkers() throws { + let root = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + defer { try? FileManager.default.removeItem(at: root) } + let real = root.appendingPathComponent("real/KeyPath.app") + try FileManager.default.createDirectory(at: real, withIntermediateDirectories: true) + let link = root.appendingPathComponent("link") + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: real.deletingLastPathComponent()) + let executable = real.resolvingSymlinksInPath().path + "/Contents/MacOS/KeyPath" + let worker = SessionWorkerProcess(pid: 1, mode: .runtime, ownerPID: 10, executable: executable) + XCTAssertTrue(worker.belongsToUninstall(of: link.appendingPathComponent("KeyPath.app")) { _ in executable }) + } + /// The 2026-11-03 orphans outlived their owner by hours. A reused owner /// PID now belonging to an unrelated process, or to another KeyPath /// bundle, does not make the orphan look owned. From 5c4ed71a52e7092870b4908257f93c88321c24d8 Mon Sep 17 00:00:00 2001 From: Micah Alpern Date: Thu, 5 Nov 2026 21:49:57 -0800 Subject: [PATCH 9/9] Read the fake's signal log under its lock Co-Authored-By: Claude Opus 5.5 --- Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift index 8d3c23d0a..ee25f8e5b 100644 --- a/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift +++ b/Tests/KeyPathTests/SessionRuntime/SessionWorkerSweepTests.swift @@ -104,7 +104,7 @@ final class SessionWorkerSweepTests: XCTestCase { let fake = FakeProcesses([], exitOnTerm: []) let outcome = await fake.sweep.stopAll() XCTAssertTrue(outcome.succeeded) - XCTAssertTrue(fake.signals.isEmpty) + XCTAssertTrue(fake.locked { fake.signals.isEmpty }) } /// Every worker is stopped, not only the one the live app tracks.