diff --git a/libs/cua-driver/Package.swift b/libs/cua-driver/Package.swift index ce51c9652a..ee39eaaee7 100644 --- a/libs/cua-driver/Package.swift +++ b/libs/cua-driver/Package.swift @@ -39,5 +39,9 @@ let package = Package( name: "ZoomMathTests", dependencies: ["CuaDriverCore"] ), + .testTarget( + name: "FocusStealPreventerTests", + dependencies: ["CuaDriverCore"] + ), ] ) diff --git a/libs/cua-driver/Sources/CuaDriverCore/Focus/FocusGuard.swift b/libs/cua-driver/Sources/CuaDriverCore/Focus/FocusGuard.swift index a6fb5013d1..d14aa4d919 100644 --- a/libs/cua-driver/Sources/CuaDriverCore/Focus/FocusGuard.swift +++ b/libs/cua-driver/Sources/CuaDriverCore/Focus/FocusGuard.swift @@ -29,6 +29,17 @@ public actor FocusGuard { private let enforcer: SyntheticAppFocusEnforcer private let systemPreventer: SystemFocusStealPreventer? + /// Construct a guard with the three focus-suppression layers wired in. + /// + /// - Parameters: + /// - enablement: AX enablement assertion used to write synthetic + /// focus on the target window/element. + /// - enforcer: synthetic-focus enforcer that flips + /// `kAXEnhancedUserInterface` etc. for the duration of the body. + /// - systemPreventer: optional layer-3 reactive preventer. When + /// supplied, the guard arms a lease around the body so any + /// target self-activation triggered by the AX action is undone + /// before the next compositor frame. public init( enablement: AXEnablementAssertion, enforcer: SyntheticAppFocusEnforcer, @@ -84,15 +95,23 @@ public actor FocusGuard { // activation notification and immediately re-activates the prior // frontmost app. Only armed when the target isn't already // frontmost (no point suppressing self → self). - var suppressionHandle: SuppressionHandle? + // + // Lease form: ARC fires `deinit` on every exit path including the + // catch branch below. The lease replaces a previous bug-prone + // pattern of manually pairing begin/end across do/catch — if a + // future edit forgets one cleanup branch, the lease still + // releases when the local goes out of scope. + var suppressionLease: SuppressionLease? if let preventer = systemPreventer { let targetApp = NSRunningApplication(processIdentifier: pid) let isTargetFrontmost = targetApp?.isActive ?? false if !isTargetFrontmost, let frontmost = NSWorkspace.shared.frontmostApplication { - suppressionHandle = await preventer.beginSuppression( - targetPid: pid, restoreTo: frontmost + suppressionLease = await preventer.leaseSuppression( + targetPid: pid, + restoreTo: frontmost, + origin: "FocusGuard.withFocusSuppressed" ) } } @@ -100,27 +119,43 @@ public actor FocusGuard { do { let result = try await body() if let focusState { await enforcer.reenableActivation(focusState) } - if let handle = suppressionHandle { - try? await Task.sleep(nanoseconds: 50_000_000) // 50ms - await systemPreventer?.endSuppression(handle) + if let lease = suppressionLease { + // 50ms gives the target's reflex post-AXPress activation + // (Safari WebKit) time to fire before we tear down the + // observer that catches it. Explicit release awaits any + // pending reactivation tasks scheduled in that window. + try? await Task.sleep(nanoseconds: 50_000_000) + await lease.release() } return result } catch { if let focusState { await enforcer.reenableActivation(focusState) } - if let handle = suppressionHandle { - await systemPreventer?.endSuppression(handle) + if let lease = suppressionLease { + await lease.release() } throw error } + // If a future edit ever drops one of the explicit `release()` + // calls above, ARC fires the lease's `deinit` when this scope + // unwinds — the entry still gets released. Belt + suspenders. } // MARK: - Helpers } +/// Errors thrown by ``FocusGuard/withFocusSuppressed(pid:element:body:)``. public enum FocusGuardError: Error, CustomStringConvertible, Sendable { + /// The target window is minimized in the Dock; AX actions on it + /// would force-deminiaturize it (especially in Chrome). Caller must + /// either unminimize first or use a keyboard-input alternative + /// (`type_text_chars`, `press_key`) that does not have this side + /// effect. case windowMinimized(pid: pid_t) + /// Human-readable description of the error including the recovery + /// hint. `Tool.Content.text` propagates this directly to MCP + /// clients. public var description: String { switch self { case .windowMinimized(let pid): diff --git a/libs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swift b/libs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swift index e68d034a30..28522f758f 100644 --- a/libs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swift +++ b/libs/cua-driver/Sources/CuaDriverCore/Focus/SystemFocusStealPreventer.swift @@ -1,10 +1,19 @@ import AppKit import Foundation +import os /// An opaque handle returned by ``SystemFocusStealPreventer/beginSuppression``. /// Pass the same handle to ``SystemFocusStealPreventer/endSuppression`` to /// stop suppressing for that particular target; other concurrent suppressions /// stay active until their own handles are ended. +/// +/// **Prefer ``SystemFocusStealPreventer/withSuppression(targetPid:restoreTo:origin:body:)`` +/// over manual `begin`/`end` whenever the suppression's lifetime fits inside +/// a single async function** — the closure form is leak-proof by construction. +/// When the lifetime must span function boundaries (e.g. a snapshot taken +/// before an action and released after side-effect detection), prefer +/// ``SuppressionLease`` over raw handles — the lease releases the entry in +/// `deinit`, so ARC catches leaks that scope-bound defers cannot. public struct SuppressionHandle: Sendable, Hashable { fileprivate let id: UUID @@ -13,6 +22,78 @@ public struct SuppressionHandle: Sendable, Hashable { } } +/// Reference-typed lease for a focus suppression entry. Releases the entry +/// in `deinit`, which is ARC's strongest available guarantee that no exit +/// path — including thrown errors, task cancellation, or future call-site +/// regressions — can leak the underlying registration. +/// +/// Construct via ``SystemFocusStealPreventer/leaseSuppression(targetPid:restoreTo:origin:)``. +/// Call ``release()`` explicitly when you want to await pending reactivation +/// tasks; otherwise just drop the lease and ARC will fire a fire-and-forget +/// cleanup. `release()` is idempotent. +/// +/// This is the recommended API for the snapshot/detect pattern where the +/// suppression's lifetime must span function boundaries — the lease can be +/// stored in a struct and the cleanup is guaranteed by the language, not +/// by call-site discipline. +public final class SuppressionLease: @unchecked Sendable { + private let preventer: SystemFocusStealPreventer + private let handle: SuppressionHandle + /// `OSAllocatedUnfairLock` rather than `NSLock`+`var` because Swift 6 + /// bans `NSLock.lock()` from async contexts (the kernel-level priority- + /// inversion guarantees of `os_unfair_lock` mean the runtime can prove + /// the critical section is bounded). This is the platform-idiomatic + /// async-safe replacement for "lock + bool flag" patterns. macOS 13+, + /// and we target macOS 14, so it's freely available. + private let releasedFlag = OSAllocatedUnfairLock(initialState: false) + + /// The handle for the underlying entry. Useful for callers that want to + /// pass through the legacy ``SystemFocusStealPreventer/endSuppression(_:)`` + /// API; new code should prefer ``release()``. + public var rawHandle: SuppressionHandle { handle } + + fileprivate init(preventer: SystemFocusStealPreventer, handle: SuppressionHandle) { + self.preventer = preventer + self.handle = handle + } + + /// Release the lease and await any in-flight reactivation tasks. + /// Idempotent: calling more than once is a no-op. Concurrent calls are + /// race-safe — exactly one will perform the dispatcher remove, the + /// rest return early. + public func release() async { + // Atomic test-and-set. Returns the prior value; we proceed only + // when we were the first caller to flip false→true. + let alreadyReleased = releasedFlag.withLock { released in + let prior = released + released = true + return prior + } + if alreadyReleased { return } + await preventer.endSuppression(handle) + } + + deinit { + // ARC safety net: the holder dropped us without calling release(). + // Same atomic test-and-set as release(), but we can't await from a + // deinit so we hand the cleanup to a detached Task. Pending + // reactivation tasks scheduled by the observer are orphaned — + // they're harmless idempotent `activate(options: [])` calls. The + // deadline eviction in the dispatcher (layer 3) catches the same + // case in bounded time even if this Task is never scheduled, so + // we lose nothing by fire-and-forgetting here. + let alreadyReleased = releasedFlag.withLock { released in + let prior = released + released = true + return prior + } + if alreadyReleased { return } + let p = preventer + let h = handle + Task.detached { await p.endSuppression(h) } + } +} + /// Layer 3 of the focus-suppression stack. Reactively counters the /// "target app called `NSApp.activate(ignoringOtherApps:)` in its own /// `applicationDidFinishLaunching`" failure mode. @@ -48,10 +129,33 @@ public struct SuppressionHandle: Sendable, Hashable { /// `CGSRegisterConnectionNotifyProc` / kCPS notifications, which we /// deliberately do not take a dependency on. /// -/// Multiple concurrent suppressions are supported — each `beginSuppression` -/// call returns a distinct handle and adds an entry to the internal map. -/// The shared `NSWorkspace` observer is installed on the first suppression -/// and removed when the last handle is ended. +/// ## Lifetime safety +/// +/// The shared dispatcher applies four overlapping guarantees so that no +/// single bug can resurrect the v0.1.9 focus-trap regression where a +/// leaked wildcard entry hijacked every app activation in the OS for the +/// rest of the process's life: +/// +/// 1. **Closure scope (preferred)** — ``withSuppression(targetPid:restoreTo:origin:body:)`` +/// pairs begin/end with `defer`. No handle escapes the closure. +/// 2. **ARC scope** — ``leaseSuppression(targetPid:restoreTo:origin:)`` returns +/// a ``SuppressionLease`` that ends the entry in `deinit`. Catches any +/// control flow scope-defer cannot — thrown errors between begin and end, +/// task cancellation, future call-site regressions. +/// 3. **Wall-clock deadline** — every entry carries a ``maxLifetimeNs`` +/// expiry (default 5 s). The observer evicts expired entries on every +/// fire; a janitor task evicts during idle. **Worst-case leak duration is +/// bounded by ``maxLifetimeNs``, independent of every other layer.** +/// 4. **Observability** — every entry carries an ``origin`` tag and the +/// dispatcher logs a warning when active count crosses +/// ``warnActiveThreshold`` or when the deadline reaper fires. Future +/// leaks surface in `log show --process cua-driver` instead of silently +/// stealing focus. +/// +/// Multiple concurrent suppressions are supported — each registration adds +/// an entry to the internal map. The shared `NSWorkspace` observer is +/// installed on the first suppression and removed when the last entry is +/// gone (whether removed manually, by lease deinit, or by deadline). public actor SystemFocusStealPreventer { /// Delay between observing the target's self-activation and firing /// the restoring `activate(options: [])`. Tradeoff: @@ -74,35 +178,143 @@ public actor SystemFocusStealPreventer { /// several frames' worth of runloop turns inside /// `applicationDidFinishLaunching` BEFORE our demote reaches /// WindowServer — the activation notification itself is async. - /// Calculator still gets its window created (orthogonal path via - /// the `hides=YES` + `unhide()` dance). Chrome still gets its - /// URL handoff processed. Net: zero-delay demote is strictly - /// better. - private static let suppressionDelayNs: UInt64 = 0 + /// Calculator-with-no-window has been verified to be a separate + /// issue (`activates = false` swallows the initial window event) + /// and tuning this delay does not rescue it. + public static let suppressionDelayNs: UInt64 = 0 + + /// Wall-clock upper bound on a suppression entry's lifetime. The + /// dispatcher evicts entries older than this whenever the observer + /// fires or the janitor runs. Set well above the longest legitimate + /// click + detect window (≈1.3 s) so the safety net never trips + /// during normal operation, but tight enough that a runaway leak + /// recovers in seconds rather than the entire process lifetime. + /// + /// This bound is the layer-3 safety net that makes ``SuppressionLease`` + /// `deinit` and ``withSuppression`` `defer` mistakes recoverable. + public static let maxLifetimeNs: UInt64 = 5_000_000_000 // 5 s + + /// How often the janitor task wakes up during idle to evict expired + /// entries when no NSWorkspace activation events arrive. Cheap — + /// just a lock + dictionary scan. Keeps the worst-case eviction + /// latency at `maxLifetimeNs + janitorIntervalNs`. + public static let janitorIntervalNs: UInt64 = 1_000_000_000 // 1 s + + /// Active-entry count above which the dispatcher logs a warning to the + /// unified log. Legitimate workloads have at most ~2 concurrent + /// suppressions (one from `WindowChangeDetector.snapshot()`, one from + /// `LaunchAppTool`'s placeholder→pid swap). Anything above 2 is + /// suspicious; above this threshold it's almost certainly a leak. + public static let warnActiveThreshold: Int = 4 + + /// Default origin tag used when a caller doesn't supply one. Surfaces + /// in leak warnings as a fallback so we can still grep for the file. + fileprivate static let unknownOrigin = "" private let dispatcher: Dispatcher + private let janitorIntervalNs: UInt64 + private var janitorTask: Task? - public init() { - self.dispatcher = Dispatcher(suppressionDelayNs: Self.suppressionDelayNs) + /// Designated initializer. Production callers use the default values + /// for `maxLifetimeNs` / `janitorIntervalNs` / `warnActiveThreshold` + /// — those are the safety-net knobs and there's no good reason to + /// vary them in production. Tests pass tight values to verify the + /// layer-3 reaper deterministically. + /// + /// Actors don't support `convenience` inits (they have a flat init + /// model), so we expose one initializer with sensible defaults. + public init( + suppressionDelayNs: UInt64 = SystemFocusStealPreventer.suppressionDelayNs, + maxLifetimeNs: UInt64 = SystemFocusStealPreventer.maxLifetimeNs, + janitorIntervalNs: UInt64 = SystemFocusStealPreventer.janitorIntervalNs, + warnActiveThreshold: Int = SystemFocusStealPreventer.warnActiveThreshold + ) { + self.dispatcher = Dispatcher( + suppressionDelayNs: suppressionDelayNs, + maxLifetimeNs: maxLifetimeNs, + warnActiveThreshold: warnActiveThreshold + ) + self.janitorIntervalNs = janitorIntervalNs + } + + // MARK: - Closure-scoped (preferred) + + /// Run `body` while a suppression entry is active. The entry is + /// guaranteed to be released on every exit path — return, throw, task + /// cancellation. No handle escapes the closure, so callers cannot + /// forget to release. + /// + /// This is the strongest available API: the language enforces the + /// lifetime. Use it whenever the suppression fits inside a single + /// async function. + @discardableResult + public func withSuppression( + targetPid: pid_t, + restoreTo: NSRunningApplication, + origin: StaticString = #function, + body: @Sendable () async throws -> T + ) async rethrows -> T { + let handle = dispatcher.add( + targetPid: targetPid, restoreTo: restoreTo, origin: "\(origin)" + ) + startJanitorIfNeeded() + do { + let result = try await body() + await endSuppression(handle) + return result + } catch { + await endSuppression(handle) + throw error + } + } + + // MARK: - ARC-scoped + + /// Register a suppression and return a ``SuppressionLease`` that ends + /// it in `deinit`. Use this when the lifetime must span function + /// boundaries (e.g. snapshot/detect pattern) and a closure scope won't + /// work. ARC catches leaks that scope-defers cannot. + /// + /// The caller can call ``SuppressionLease/release()`` to await pending + /// reactivation tasks; if the caller simply drops the lease, ARC fires + /// a fire-and-forget cleanup. Either way the entry is released. + public func leaseSuppression( + targetPid: pid_t, + restoreTo: NSRunningApplication, + origin: StaticString = #function + ) -> SuppressionLease { + let handle = dispatcher.add( + targetPid: targetPid, restoreTo: restoreTo, origin: "\(origin)" + ) + startJanitorIfNeeded() + return SuppressionLease(preventer: self, handle: handle) } - /// Begin suppressing focus-steal events for `targetPid`. Any - /// `NSWorkspace.didActivateApplicationNotification` that fires while the - /// suppression is active and names `targetPid` as the newly-active app - /// schedules a delayed `restoreTo.activate(options: [])` on the main - /// actor to steal focus back onto whatever was frontmost before the - /// launch. + // MARK: - Manual (deprecated; kept for migration) + + /// Begin suppressing. Manual lifetime — caller is responsible for + /// matching ``endSuppression(_:)``. **Prefer ``withSuppression`` or + /// ``leaseSuppression`` over this manual API.** Direct begin/end pairs + /// are vulnerable to leaks across error and async boundaries; the + /// scoped APIs above make those leaks impossible. /// /// Returns a handle that must be passed to ``endSuppression(_:)`` to /// stop the suppression. Overlapping calls for different targets are - /// independent — each registers its own `(pid, restoreTo)` entry. + /// independent — each registers its own `(pid, restoreTo)` entry. The + /// underlying entry is also subject to the dispatcher's + /// ``maxLifetimeNs`` deadline, so a forgotten end will self-recover + /// in bounded time. + @available(*, deprecated, message: "Prefer withSuppression { … } (closure-scoped) or leaseSuppression() (ARC-scoped). Manual begin/end pairs are leak-prone across error and async boundaries.") @discardableResult public func beginSuppression( targetPid: pid_t, - restoreTo: NSRunningApplication + restoreTo: NSRunningApplication, + origin: StaticString = #function ) async -> SuppressionHandle { - let handle = SuppressionHandle() - dispatcher.add(handle: handle, targetPid: targetPid, restoreTo: restoreTo) + let handle = dispatcher.add( + targetPid: targetPid, restoreTo: restoreTo, origin: "\(origin)" + ) + startJanitorIfNeeded() return handle } @@ -120,6 +332,49 @@ public actor SystemFocusStealPreventer { _ = await task.value } } + + // MARK: - Diagnostics + + /// Number of currently-active suppression entries. Test/diagnostic-only. + public var activeCount: Int { + dispatcher.activeCount + } + + // MARK: - Janitor + + private func startJanitorIfNeeded() { + if janitorTask != nil { return } + let dispatcher = self.dispatcher + let interval = self.janitorIntervalNs + janitorTask = Task.detached(priority: .background) { [weak self] in + while !Task.isCancelled { + try? await Task.sleep(nanoseconds: interval) + let evicted = dispatcher.reapExpired() + for task in evicted { _ = await task.value } + // Idle shutdown: when the dispatcher has no entries and + // observer is torn down, stop the janitor. + if await self?.shouldStopJanitor() ?? true { break } + } + await self?.clearJanitor() + } + } + + /// Test-only: force a reap pass without waiting for the janitor or + /// an `NSWorkspace` activation. Production code should never call + /// this — eviction is automatic. Exposed for unit tests so the + /// layer-3 deadline contract can be verified deterministically. + public func _forceReapForTesting() async { + let pending = dispatcher.reapExpired() + for task in pending { _ = await task.value } + } + + private func shouldStopJanitor() -> Bool { + dispatcher.activeCount == 0 + } + + private func clearJanitor() { + janitorTask = nil + } } // MARK: - Dispatcher @@ -134,27 +389,88 @@ private final class Dispatcher: @unchecked Sendable { private struct Entry { let targetPid: pid_t let restoreTo: NSRunningApplication + let origin: String + /// Wall-clock deadline (mach_absolute_time-style monotonic ns). + /// Layer-3 safety net: when the observer fires or the janitor + /// runs, any entry with `now > deadline` is force-evicted. + let deadline: UInt64 } private let suppressionDelayNs: UInt64 + private let maxLifetimeNs: UInt64 + private let warnActiveThreshold: Int + private let lock = NSLock() private var entries: [UUID: Entry] = [:] private var pendingRestoreTasks: [Task] = [] private var observer: NSObjectProtocol? - init(suppressionDelayNs: UInt64) { + /// Unified-log subsystem. Routed through `os.Logger` so the messages + /// appear in `log show --process cua-driver` and `log stream`. We + /// don't take a swift-log dependency — `os.Logger` is free, builds + /// into Console.app, and is the right tool for "operator wants to + /// see what the driver did last Tuesday" diagnostics. + private let logger = Logger( + subsystem: "io.trycua.cua-driver", category: "FocusStealPreventer" + ) + + init(suppressionDelayNs: UInt64, maxLifetimeNs: UInt64, warnActiveThreshold: Int) { self.suppressionDelayNs = suppressionDelayNs + self.maxLifetimeNs = maxLifetimeNs + self.warnActiveThreshold = warnActiveThreshold + } + + var activeCount: Int { + lock.lock(); defer { lock.unlock() } + return entries.count } - func add(handle: SuppressionHandle, targetPid: pid_t, restoreTo: NSRunningApplication) { + /// Register a new entry and return its handle. Installs the shared + /// `NSWorkspace` observer if this is the first entry. Logs a warning + /// if the active count crosses the leak-suspicion threshold so future + /// regressions surface in the unified log instead of silently + /// stealing focus. + func add( + targetPid: pid_t, restoreTo: NSRunningApplication, origin: String + ) -> SuppressionHandle { + let handle = SuppressionHandle() + let deadline = monotonicNow() &+ maxLifetimeNs + lock.lock() - entries[handle.id] = Entry(targetPid: targetPid, restoreTo: restoreTo) + entries[handle.id] = Entry( + targetPid: targetPid, + restoreTo: restoreTo, + origin: origin, + deadline: deadline + ) + let count = entries.count let needsObserver = (observer == nil) + // Snapshot a description list while holding the lock so we can + // log without re-acquiring it. + let leakSuspect = count > warnActiveThreshold + let originList = leakSuspect ? entries.values.map(\.origin).sorted() : [] lock.unlock() if needsObserver { installObserver() } + + if leakSuspect { + // Surface, don't crash. A leak is a bug we want to fix; an + // assert in production breaks the user's automation. Log it + // loudly in the unified log instead — operators can grep for + // "FocusStealPreventer leak" and the origin list pinpoints + // the call sites holding the entries. + logger.warning( + """ + FocusStealPreventer leak suspect: \(count, privacy: .public) active \ + entries (threshold \(self.warnActiveThreshold, privacy: .public)). \ + Origins: \(originList.joined(separator: ", "), privacy: .public) + """ + ) + } + + return handle } /// Removes the entry for `handle` and returns any in-flight @@ -182,6 +498,56 @@ private final class Dispatcher: @unchecked Sendable { return pending } + /// Layer-3 safety net: scan for entries past their deadline and force- + /// evict them. Returns any pending reactivation tasks that the caller + /// can drain. + /// + /// Called from two places: (1) the janitor task on a timer, (2) the + /// activation observer on every fire. The observer-side reap is what + /// makes a leaked wildcard entry stop hijacking activations *before* + /// the next user app-switch — even if the janitor is starved. + @discardableResult + func reapExpired() -> [Task] { + let now = monotonicNow() + + lock.lock() + var evicted: [(UUID, Entry)] = [] + for (id, entry) in entries where now > entry.deadline { + evicted.append((id, entry)) + entries.removeValue(forKey: id) + } + let shouldRemoveObserver = entries.isEmpty && !evicted.isEmpty + let token = observer + if shouldRemoveObserver { + observer = nil + } + let pending = shouldRemoveObserver ? pendingRestoreTasks : [] + if shouldRemoveObserver { + pendingRestoreTasks = [] + } + lock.unlock() + + if shouldRemoveObserver, let token { + NSWorkspace.shared.notificationCenter.removeObserver(token) + } + + for (_, entry) in evicted { + // Errors, not warnings: deadline reap means a higher-layer + // guarantee (closure defer / lease deinit) failed. Surface + // loudly so the next operator pass can find it. + logger.error( + """ + FocusStealPreventer deadline reap: evicted entry origin=\ + \(entry.origin, privacy: .public) targetPid=\ + \(entry.targetPid, privacy: .public). This indicates a \ + missing release path; investigate the named origin. + """ + ) + } + + return pending + } + private func installObserver() { // queue: nil delivers the callback synchronously on the posting // thread. NSWorkspace posts on main, so the activation handler @@ -218,6 +584,12 @@ private final class Dispatcher: @unchecked Sendable { let activatedPid = app.processIdentifier + // Reap on every fire. Cheap (one dictionary scan) and bounds the + // worst-case leak duration to `maxLifetimeNs` — the leaked entry + // stops hijacking activations *before* this very fire schedules a + // restore task. + reapExpired() + lock.lock() // Match entries where: // - targetPid == activatedPid (specific target suppression), OR @@ -257,3 +629,15 @@ private final class Dispatcher: @unchecked Sendable { lock.unlock() } } + +// MARK: - Time + +/// Monotonic nanosecond clock for entry deadlines. Uses +/// `clock_gettime(CLOCK_MONOTONIC_RAW)` so jumps in wall time (sleep, +/// NTP slew) cannot accidentally expire entries early or extend leaks. +@inline(__always) +private func monotonicNow() -> UInt64 { + var ts = timespec() + clock_gettime(CLOCK_MONOTONIC_RAW, &ts) + return UInt64(ts.tv_sec) &* 1_000_000_000 &+ UInt64(ts.tv_nsec) +} diff --git a/libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift b/libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift index 5b66bd0f95..882e8c7208 100644 --- a/libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift +++ b/libs/cua-driver/Sources/CuaDriverServer/Tools/LaunchAppTool.swift @@ -3,7 +3,16 @@ import CuaDriverCore import Foundation import MCP +/// MCP tool that launches a macOS app in the background without stealing +/// focus from the current frontmost application. Pairs the LaunchServices +/// `activates = false` flag with the layer-3 `SystemFocusStealPreventer` so +/// targets that self-activate during `applicationDidFinishLaunching` are +/// also kept off-front. Configures `WEBKIT_INSPECTOR_SERVER` and +/// `--remote-debugging-port` for WKWebView / Electron remote inspection +/// when the corresponding ports are supplied. public enum LaunchAppTool { + /// MCP `ToolHandler` registration. Wires the JSON schema, argument + /// parser, and the body that performs the launch. public static let handler = ToolHandler( tool: Tool( name: "launch_app", @@ -196,19 +205,24 @@ public enum LaunchAppTool { NSWorkspace.shared.frontmostApplication } - // Arm the preventer speculatively with targetPid=0; we'll - // replace it below once we know the real pid. The preventer - // matches by pid on each activation notification, so a - // placeholder won't fire false positives. - var handle: SuppressionHandle? - if let priorFrontmost { - handle = + // Arm the preventer speculatively with a placeholder + // (targetPid=0 wildcard) BEFORE launch returns, so any + // activation the target emits while `AppLauncher.launch` + // is still running gets caught. Lease form: if `launch` + // throws, ARC fires `deinit` on the lease and the entry + // is released. We never have to thread a manual cleanup + // through the catch path. + let placeholderLease: SuppressionLease? = + if let priorFrontmost { await AppStateRegistry.systemFocusStealPreventer - .beginSuppression( - targetPid: 0, // placeholder; replaced after launch - restoreTo: priorFrontmost - ) - } + .leaseSuppression( + targetPid: 0, // placeholder; replaced after launch + restoreTo: priorFrontmost, + origin: "LaunchAppTool.placeholder" + ) + } else { + nil + } var additionalArguments: [String] = rawExtraArgs.compactMap { $0.stringValue } if let port = electronDebuggingPort { @@ -234,33 +248,37 @@ public enum LaunchAppTool { createsNewApplicationInstance: createsNewInstance ) - // Replace the placeholder pid with the real one so any - // activation notification the target emits from now on is - // caught. Observed activations that fired DURING the launch - // (synchronous `open`) will have been seen by the observer - // but not matched (pid=0 mismatch), so they pass through — - // this fix covers the PROCESS-ORDERING race (target emits - // activation AFTER `open` returns) which is the common - // case on real machines; the intra-`open` case is handled - // by the target's own reflex running asynchronously. + // Crossfade from placeholder (wildcard) to pid-specific + // suppression with NO suppression-free window in between. + // + // Earlier ordering was `release placeholder → arm + // pid-specific`, which left a brief window where a target + // self-activation could slip through. The dispatcher + // permits multiple concurrent entries — during the + // overlap below, any activation is matched by both the + // placeholder wildcard ("anything not priorFrontmost") + // and the pid-specific entry ("exactly info.pid"); the + // first match restores `priorFrontmost`, which is what + // we want. + // + // 500ms is enough for applicationDidFinishLaunching plus + // any reflex NSApp.activate to fire and get suppressed. let shouldSuppress = priorFrontmost != nil && priorFrontmost?.processIdentifier != info.pid - if shouldSuppress, let handle, let priorFrontmost { + if shouldSuppress, let priorFrontmost { await AppStateRegistry.systemFocusStealPreventer - .endSuppression(handle) - let reArmedHandle = - await AppStateRegistry.systemFocusStealPreventer - .beginSuppression( + .withSuppression( targetPid: info.pid, - restoreTo: priorFrontmost - ) - // 500ms is enough for applicationDidFinishLaunching - // plus any reflex NSApp.activate to fire and get - // suppressed. - try? await Task.sleep(nanoseconds: 500_000_000) - await AppStateRegistry.systemFocusStealPreventer - .endSuppression(reArmedHandle) + restoreTo: priorFrontmost, + origin: "LaunchAppTool.postLaunch" + ) { + // Pid-specific entry is now armed. NOW it's + // safe to drop the placeholder — the + // crossfade is complete. + await placeholderLease?.release() + try? await Task.sleep(nanoseconds: 500_000_000) + } // Belt-and-braces: if the target is STILL frontmost // after the suppression window (intra-`open` synchronous @@ -275,9 +293,15 @@ public enum LaunchAppTool { _ = priorFrontmost.activate(options: []) } } - } else if let handle { - await AppStateRegistry.systemFocusStealPreventer - .endSuppression(handle) + } else { + // No pid-specific phase needed (priorFrontmost was nil + // or matches the launched pid). The placeholder has + // done its job — catching any intra-launch activation + // — and can be released now. ARC would also catch this + // via deinit when `placeholderLease` goes out of scope, + // but explicit release awaits any pending reactivation + // Tasks before we leave the do-block. + await placeholderLease?.release() } var summary = "✅ Launched \(info.name) (pid \(info.pid)) in background." diff --git a/libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift b/libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift index 8c5644e767..afabe1fb33 100644 --- a/libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift +++ b/libs/cua-driver/Sources/CuaDriverServer/Tools/WindowChangeDetector.swift @@ -23,24 +23,43 @@ public enum WindowChangeDetector { /// A lightweight record for a newly-appeared window. public struct WindowEvent: Sendable { + /// CGWindowID of the window. Stable for the window's lifetime. public let windowId: Int + /// Process id of the app that owns the window. public let pid: Int32 + /// Owning app's localized name (e.g. "Safari"). public let appName: String + /// Window title at the moment of detection. May be empty. public let title: String } /// State captured before the action. + /// + /// Holds an ARC-managed ``SuppressionLease`` rather than a raw handle. + /// **This is the leak-proofing for the snapshot/detect pattern**: if a + /// caller drops the `Snapshot` without ever calling + /// ``WindowChangeDetector/detectChanges(snapshot:timeout:pollInterval:)`` + /// (e.g. an early-return error path between snapshot and detect), the + /// lease's `deinit` releases the underlying entry. The cleanup happens + /// by the language, not by call-site discipline. + /// + /// `Snapshot` is a struct, so a copy retains the lease reference. The + /// final copy going out of scope is what fires deinit; explicit + /// `detectChanges` releases it earlier and turns deinit into a no-op. public struct Snapshot: Sendable { /// CGWindowIDs of all visible layer-0 windows at snapshot time. public let windowIds: Set /// PID of the frontmost application at snapshot time, or nil. public let frontPid: Int32? - /// Wildcard suppression handle armed at snapshot time. + /// Wildcard suppression lease armed at snapshot time. /// Active from `snapshot()` through `detectChanges()` so that any /// app that self-activates as a side-effect of the action (e.g. /// Safari opening from UTM Gallery) is blocked before the first /// compositor frame, not just after we notice it in the poll loop. - internal let suppressionHandle: SuppressionHandle? + /// + /// ARC-managed: dropping the Snapshot without calling + /// `detectChanges` is safe — `SuppressionLease.deinit` releases. + internal let suppressionLease: SuppressionLease? } /// What changed after the action. @@ -80,6 +99,9 @@ public enum WindowChangeDetector { } } + /// Sentinel returned when the detection window elapses with no + /// new windows and no foreground change. Reused so callers can + /// `return .noChange` cheaply. public static let noChange = Changes(newWindows: [], foregroundChanged: false) } @@ -103,15 +125,23 @@ public enum WindowChangeDetector { } // Arm the wildcard suppressor immediately — before the action fires. - var suppressionHandle: SuppressionHandle? + // Lease form: ARC releases on Snapshot drop even if detectChanges() + // is never called (early-return error path between snapshot and + // detect). origin tag surfaces in the unified log if a leak warning + // is ever triggered. + var lease: SuppressionLease? if let pid = frontPid, let restoreTo = NSRunningApplication(processIdentifier: pid) { - suppressionHandle = await AppStateRegistry.systemFocusStealPreventer - .beginSuppression(targetPid: 0, restoreTo: restoreTo) + lease = await AppStateRegistry.systemFocusStealPreventer + .leaseSuppression( + targetPid: 0, + restoreTo: restoreTo, + origin: "WindowChangeDetector.snapshot" + ) } - return Snapshot(windowIds: ids, frontPid: frontPid, suppressionHandle: suppressionHandle) + return Snapshot(windowIds: ids, frontPid: frontPid, suppressionLease: lease) } /// Poll for up to `timeout` seconds for new windows or a foreground-app @@ -133,16 +163,21 @@ public enum WindowChangeDetector { timeout: TimeInterval = 1.0, pollInterval: Int = 50 ) async -> Changes { - // End the wildcard suppressor that was armed in snapshot() once - // we return — covers the full action + detection window. - defer { - if let handle = snapshot.suppressionHandle { - Task { await AppStateRegistry.systemFocusStealPreventer.endSuppression(handle) } - } - } - + // Single-exit refactor so the lease can be torn down with a + // direct `await` (not a detached Task) before returning. The + // earlier defer-with-Task.detached form let `detectChanges` + // return while the wildcard suppressor was still active — + // a stale lease bleeding into the next action's snapshot + // window. Awaiting in-line is the only way to make the + // detection boundary the lease teardown boundary. + // + // The lease's `deinit` safety net still applies if this call + // is somehow skipped (caller early-return between snapshot and + // detect): ARC fires deinit when the Snapshot copy goes out of + // scope and the entry is released. Belt + suspenders. + var result: Changes = .noChange let deadline = Date().addingTimeInterval(timeout) - while Date() < deadline { + pollLoop: while Date() < deadline { try? await Task.sleep(for: .milliseconds(pollInterval)) // --- New windows --- @@ -178,10 +213,23 @@ public enum WindowChangeDetector { } if !newEvents.isEmpty || foregroundChanged { - return Changes(newWindows: newEvents, foregroundChanged: foregroundChanged) + result = Changes( + newWindows: newEvents, foregroundChanged: foregroundChanged + ) + break pollLoop } } - return .noChange + + // Tear down the wildcard suppressor BEFORE returning. Direct + // `await` (not `Task { ... }`) so the dispatcher entry and any + // in-flight delayed reactivation Tasks are fully drained before + // the caller sees `result`. Without this, the next caller's + // snapshot() could observe the stale wildcard still firing on + // the next NSWorkspace activation. + if let lease = snapshot.suppressionLease { + await lease.release() + } + return result } /// Re-activate the previously-frontmost application, sending its window diff --git a/libs/cua-driver/Tests/FocusStealPreventerTests/FocusStealPreventerTests.swift b/libs/cua-driver/Tests/FocusStealPreventerTests/FocusStealPreventerTests.swift new file mode 100644 index 0000000000..cb0db4532b --- /dev/null +++ b/libs/cua-driver/Tests/FocusStealPreventerTests/FocusStealPreventerTests.swift @@ -0,0 +1,320 @@ +import AppKit +import XCTest +@testable import CuaDriverCore + +/// Unit tests for the four-layer leak-prevention design in +/// ``SystemFocusStealPreventer``. +/// +/// The contract under test is that **no caller error path can leave a +/// suppression entry alive in the dispatcher for longer than +/// ``SystemFocusStealPreventer/maxLifetimeNs``**, regardless of which +/// API surface they used. Each test exercises one layer of the design: +/// +/// 1. ``testWithSuppressionReleasesOnReturn`` / +/// ``testWithSuppressionReleasesOnThrow`` — closure scope (compiler +/// enforces release on every exit path). +/// 2. ``testLeaseReleasesOnExplicitCall`` / +/// ``testLeaseReleasesOnDeinit`` — ARC scope (deinit catches what +/// scope-defer cannot). +/// 3. ``testDeadlineReapsLeakedManualEntry`` — wall-clock deadline +/// (the safety net under everything else). +/// +/// We use `NSRunningApplication.current` for `restoreTo` so the tests +/// don't depend on any external app being frontmost. The dispatcher +/// just stores the reference — none of these tests fire the +/// `NSWorkspace.didActivateApplicationNotification` observer. +final class FocusStealPreventerTests: XCTestCase { + + private var selfApp: NSRunningApplication { NSRunningApplication.current } + + // MARK: - Layer 1: closure scope + + func testWithSuppressionReleasesOnReturn() async { + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + let __count1 = await preventer.activeCount + XCTAssertEqual(__count1, 0) + await preventer.withSuppression(targetPid: 0, restoreTo: selfApp) { + let inside = await preventer.activeCount + XCTAssertEqual(inside, 1, "entry must be live during body") + } + + let __count2 = await preventer.activeCount + + XCTAssertEqual(__count2, 0, + "withSuppression must release on normal return" + ) + } + + func testWithSuppressionReleasesOnThrow() async { + struct BodyError: Error {} + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + + do { + try await preventer.withSuppression(targetPid: 0, restoreTo: selfApp) { + throw BodyError() + } + XCTFail("expected throw") + } catch is BodyError { + // expected + } catch { + XCTFail("unexpected error: \(error)") + } + + let __count3 = await preventer.activeCount + + XCTAssertEqual(__count3, 0, + "withSuppression must release on thrown error" + ) + } + + // MARK: - Layer 2: ARC scope (lease) + + func testLeaseReleasesOnExplicitCall() async { + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + let lease = await preventer.leaseSuppression(targetPid: 0, restoreTo: selfApp) + let __count4 = await preventer.activeCount + XCTAssertEqual(__count4, 1) + await lease.release() + let __count5 = await preventer.activeCount + XCTAssertEqual(__count5, 0) + } + + func testLeaseReleaseIsIdempotent() async { + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + let lease = await preventer.leaseSuppression(targetPid: 0, restoreTo: selfApp) + + await lease.release() + await lease.release() // second call must be a no-op + + let __count6 = await preventer.activeCount + + XCTAssertEqual(__count6, 0) + } + + /// ARC fires `deinit` when the lease's last reference goes out of + /// scope. The deinit's cleanup is dispatched to a `Task.detached`, + /// so we have to poll for the active-count to drop. The poll + /// timeout (2 s) is generous compared to the dispatcher's release + /// latency (microseconds). If this test ever flakes, the design's + /// language-level guarantee has failed and the regression is + /// urgent. + func testLeaseReleasesOnDeinit() async throws { + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + + // Scope the lease so it deinits at the end of this block. + do { + let lease = await preventer.leaseSuppression(targetPid: 0, restoreTo: selfApp) + let __count7 = await preventer.activeCount + XCTAssertEqual(__count7, 1) + _ = lease + } + + try await waitForActiveCount(0, on: preventer, timeout: 2.0) + } + + // MARK: - Layer 3: wall-clock deadline (the safety net) + + /// **The crucial test.** Even when every higher layer fails — the + /// caller used the deprecated raw `beginSuppression`, threw away + /// the handle, and never called `endSuppression` — the dispatcher's + /// deadline reaper must evict the entry within `maxLifetimeNs`. + /// Anything else means the v0.1.9 focus-trap regression class is + /// still possible. + /// + /// Uses the test-seam initializer to set `maxLifetimeNs = 200 ms` + /// so the test runs fast. The eviction is triggered explicitly via + /// `_forceReapForTesting()`, which simulates the reap pass that + /// happens automatically inside the activation observer and the + /// janitor task. We don't rely on the janitor to prove the + /// contract — that would make the test wait the full janitor + /// interval and add CI flake risk. + func testDeadlineReapsLeakedManualEntry() async throws { + let testMaxLifetimeNs: UInt64 = 200_000_000 // 200 ms + let preventer = SystemFocusStealPreventer( + suppressionDelayNs: 0, + maxLifetimeNs: testMaxLifetimeNs, + janitorIntervalNs: 60_000_000_000, // disable janitor influence + warnActiveThreshold: 1000 + ) + + // Deprecated API on purpose: this test exists *because* the + // deprecated path remains a leak hazard for callers who haven't + // migrated. The deadline must protect them. + @available(*, deprecated) + func leakAnEntry() async { + _ = await preventer.beginSuppression(targetPid: 0, restoreTo: selfApp) + } + await leakAnEntry() + let __count8 = await preventer.activeCount + XCTAssertEqual(__count8, 1) + // Wait past the deadline. + try await Task.sleep(nanoseconds: testMaxLifetimeNs + 50_000_000) + + // Trigger the observer-side reap path directly. + await preventer._forceReapForTesting() + + let __count9 = await preventer.activeCount + + XCTAssertEqual(__count9, + 0, + "deadline reaper must evict expired entries even when the caller leaked the handle" + ) + } + + /// The deadline reaper must not evict still-live entries just because + /// they share a preventer with expired ones. + func testDeadlineReapsOnlyExpiredEntries() async throws { + let testMaxLifetimeNs: UInt64 = 200_000_000 + let preventer = SystemFocusStealPreventer( + suppressionDelayNs: 0, + maxLifetimeNs: testMaxLifetimeNs, + janitorIntervalNs: 60_000_000_000, + warnActiveThreshold: 1000 + ) + + @available(*, deprecated) + func makeOldHandle() async -> SuppressionHandle { + await preventer.beginSuppression(targetPid: 0, restoreTo: selfApp) + } + let oldHandle = await makeOldHandle() + // Wait so the first entry is past its deadline. + try await Task.sleep(nanoseconds: testMaxLifetimeNs + 50_000_000) + // Add a fresh entry — its deadline is `now + maxLifetimeNs`. + let freshLease = await preventer.leaseSuppression(targetPid: 0, restoreTo: selfApp) + + await preventer._forceReapForTesting() + + let __count10 = await preventer.activeCount + + XCTAssertEqual(__count10, 1, + "fresh entry must survive a reap pass that evicts only the expired one" + ) + // Cleanup. + await preventer.endSuppression(oldHandle) // already evicted; idempotent + await freshLease.release() + let __count11 = await preventer.activeCount + XCTAssertEqual(__count11, 0) + } + + // MARK: - Diagnostics + + /// `endSuppression` of an already-evicted handle must be idempotent. + /// Existing call sites (and the deprecated migration window) depend + /// on this — a forgotten handle that gets reaped should not crash + /// the eventual end call. + func testEndSuppressionAfterDeadlineIsNoOp() async throws { + let testMaxLifetimeNs: UInt64 = 100_000_000 + let preventer = SystemFocusStealPreventer( + suppressionDelayNs: 0, + maxLifetimeNs: testMaxLifetimeNs, + janitorIntervalNs: 60_000_000_000, + warnActiveThreshold: 1000 + ) + + @available(*, deprecated) + func beginAndForget() async -> SuppressionHandle { + await preventer.beginSuppression(targetPid: 0, restoreTo: selfApp) + } + let handle = await beginAndForget() + + try await Task.sleep(nanoseconds: testMaxLifetimeNs + 50_000_000) + await preventer._forceReapForTesting() + let __count12 = await preventer.activeCount + XCTAssertEqual(__count12, 0) + // Calling end on an already-reaped entry must not crash or + // resurrect anything. + await preventer.endSuppression(handle) + let __count13 = await preventer.activeCount + XCTAssertEqual(__count13, 0) + } + + // MARK: - Concurrency invariants (regression tests for CR feedback) + + /// Regression test for the Task-based defer that let `detectChanges` + /// return before the lease was actually torn down. A direct `await + /// lease.release()` must drain the dispatcher entry before the + /// caller proceeds — otherwise a stale wildcard suppressor could + /// bleed into the next caller's snapshot window. + /// + /// This test verifies the explicit `release()` semantics: the + /// post-await `activeCount` is observed by the same task that + /// awaited, with no scheduling gap that a `Task { await ... }` + /// detach would introduce. + func testExplicitReleaseDrainsBeforeReturning() async { + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + let lease = await preventer.leaseSuppression(targetPid: 0, restoreTo: selfApp) + let beforeRelease = await preventer.activeCount + XCTAssertEqual(beforeRelease, 1) + + await lease.release() + + // Crucial: the count is observed *immediately* after `release` + // returns, with no `try await Task.sleep` or polling. A + // detached release would not satisfy this assertion + // deterministically. + let immediatelyAfter = await preventer.activeCount + XCTAssertEqual( + immediatelyAfter, 0, + "explicit release() must drain the entry before returning, " + + "not hand cleanup to a detached Task" + ) + } + + /// Regression test for the LaunchAppTool placeholder→pid crossfade. + /// Two overlapping leases must coexist in the dispatcher (overlap is + /// the structural fix for the gap-window bug). The dispatcher's add + /// and remove must be independent so the crossfade has zero + /// suppression-free time. + func testCrossfadeOfTwoLeasesHasNoSuppressionGap() async { + let preventer = SystemFocusStealPreventer(suppressionDelayNs: 0) + let __c201 = await preventer.activeCount + XCTAssertEqual(__c201, 0) + // Phase 1: placeholder armed. + let placeholder = await preventer.leaseSuppression(targetPid: 0, restoreTo: selfApp) + let __c202 = await preventer.activeCount + XCTAssertEqual(__c202, 1) + // Phase 2: pid-specific armed BEFORE placeholder is released. + // This is the crossfade: both entries live concurrently. + let pidSpecific = await preventer.leaseSuppression( + targetPid: 1234, restoreTo: selfApp + ) + let duringOverlap = await preventer.activeCount + XCTAssertEqual( + duringOverlap, 2, + "dispatcher must support two concurrent leases — this is " + + "what makes the LaunchAppTool crossfade leak-free" + ) + + // Phase 3: drop the placeholder; the pid-specific entry survives. + await placeholder.release() + let afterPlaceholderDrop = await preventer.activeCount + XCTAssertEqual( + afterPlaceholderDrop, 1, + "releasing the placeholder must not affect the pid-specific entry" + ) + + // Phase 4: drop the pid-specific entry. Done. + await pidSpecific.release() + let __c203 = await preventer.activeCount + XCTAssertEqual(__c203, 0) + } + + // MARK: - Helpers + + /// Poll `activeCount` until it reaches `expected` or `timeout` + /// elapses. Used for tests where cleanup happens on a detached + /// Task and there's no better signal. + private func waitForActiveCount( + _ expected: Int, + on preventer: SystemFocusStealPreventer, + timeout: TimeInterval + ) async throws { + let deadline = Date().addingTimeInterval(timeout) + while Date() < deadline { + if await preventer.activeCount == expected { return } + try await Task.sleep(nanoseconds: 10_000_000) // 10 ms + } + let final = await preventer.activeCount + XCTFail("activeCount never reached \(expected); final=\(final)") + } +}