Skip to content

Commit f6eafa3

Browse files
thymikeethymikee
committed
fix(apple-runner): refresh a served macOS read against a relaunched pid
A read served in place resolved through `resolveAppWithoutActivation`, which returns the cached handle when the cache names the app — and a cached handle can record a pid an outside relaunch replaced. The activating route never answers from that residue because it runs `refreshCachedTargetIfProcessChanged` before its `targetNeedsActivation` read; the new arm bypassed the shared identity rule, so between an outside relaunch and the next read, a served read could address the dead process while claiming to observe the live one. Reachability: an `open`-bound session whose app is quit and reopened by the user leaves exactly this residue, and macOS `notRunningRefusal` never intercepts it. The fix runs the shared rule on the arm before resolution, so identity is settled once and both routes read the same live pid. Pinned by a host-lane test that plants the residue (live app, bogus cached pid) and asserts the served preparation refreshes without activating; canary: deleting the refresh call fails the test with the bogus pid still in place. Also drops the probe wrapper the arm no longer needs: a served read is one `state` query, and the activating route's second read cannot reuse the first because the refresh between them may have replaced the process the first answered for. The arm's and the predicate's comments are cut to the invariant they protect, per AGENTS.md; the decision record lives in #3338. Partial #3254
1 parent 74f3899 commit f6eafa3

2 files changed

Lines changed: 55 additions & 37 deletions

File tree

‎apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift‎

Lines changed: 19 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -536,19 +536,24 @@ extension RunnerTests {
536536
#else
537537
// The platform exception, written once: `SystemSurfaceHostRegistry` registers no hosts off iOS,
538538
// so nothing is ever served in place there and such a command keeps the activation route this
539-
// axis found it on. The macOS host never sends one of this arm's commands here: `alert` answers
540-
// through the macOS helper (`runAppleAlert`'s host split) and `action-button` is refused by the
541-
// owner's Action Button fact (`hasAppleActionButton`) before dispatch, so no macOS request can
542-
// be activated from this arm, and only the tvOS and visionOS runners reach it off iOS.
539+
// axis found it on. The host never sends either command here on macOS — `alert` answers through
540+
// the helper and `action-button` is refused by its owner fact before dispatch — so off iOS this
541+
// route is reached only by the tvOS and visionOS runners.
543542
return prepareActivatedTarget(command: command)
544543
#endif
545544
case .existingApp:
546545
// No request-dependent bypass here: it decides by querying the cached target's state, and a
547546
// command that may bring nothing forward has nothing for it to settle.
548547
#if os(macOS)
549-
if macReadMayBeServedInBackground(command) {
550-
// Served in place with no binding, the same shape the iOS `.presentedSurface` arm takes:
551-
// the next command that needs the app forward pays for its own activation.
548+
if let bundleId = command.appBundleId?.trimmedNonEmpty,
549+
macReadMayBeServedInBackground(
550+
command: command,
551+
targetState: XCUIApplication(bundleIdentifier: bundleId).state
552+
)
553+
{
554+
// Identity first, through the shared rule: a served read must answer the process a bound
555+
// read would have read, not a pid an outside relaunch replaced.
556+
refreshCachedTargetIfProcessChanged(bundleId: bundleId)
552557
return .context(ActiveCommandContext(app: resolveAppWithoutActivation(command: command)))
553558
}
554559
#endif
@@ -786,37 +791,14 @@ extension RunnerTests {
786791
return XCUIApplication(bundleIdentifier: bundleId)
787792
}
788793

789-
/// Whether a macOS `.existingApp` read may be served from the app exactly where it sits: the
790-
/// request names an app that is running but behind other windows. On a desktop the foreground
791-
/// repair is not a no-op for the user — it takes their frontmost app away for a command that only
792-
/// asked to look, which is what #3254 reports. The answer comes from `state`, which never launches,
793-
/// so a stopped or unknown app keeps the standing route untouched, including the launch that route
794-
/// performs; only the raise of an already-running app is in question here.
794+
/// A macOS `.existingApp` read is served in place only for `.runningBackground`: the one state
795+
/// whose tree answers while the app sits behind other windows. Every other state keeps the
796+
/// activating route, matching `targetNeedsActivation`'s macOS set, and serving in place books no
797+
/// activation fact or marker — the fact channel records only a repair performed, so a served
798+
/// read answers as an ordinary read (#3254; decision record in #3338).
795799
///
796-
/// Only the one state the host can answer a full tree from is served in place. Anything else —
797-
/// `.unknown`, `.notRunning`, and on non-macOS builds the suspended state, which the macOS SDK
798-
/// does not declare and the host lane therefore cannot name — stays on the activating route on
799-
/// purpose: an app that cannot promise an answerable tree should pay the repair that settles it
800-
/// rather than trade a focus steal for an empty read. This mirrors `targetNeedsActivation`'s
801-
/// macOS set, and `macAppCaptureNeedsRaise` agrees with it state for state: the suspended or
802-
/// unknown app that a read activates is the same app a window-level capture raises.
803-
///
804-
/// Serving in place books no activation fact and carries no substitute marker, decided with #3254
805-
/// rather than by omission: the activation channel records only a repair that was performed, and
806-
/// the observation payload's state belongs to the iOS observe-only contract, so an
807-
/// invented marker would put the same integer in two contracts with different meanings. The tree
808-
/// is live, not degraded — the measured rect drift was time, not foreground state — and nothing
809-
/// on the host acts on a "was background" fact today, so the answer travels as an ordinary read.
810-
/// `testMacReadOfABackgroundAppIsServedInBackgroundWithoutActivating` pins the silence so it stays
811-
/// owned: a future disclosure must change that assertion on purpose, not appear silently.
812-
@MainActor
813-
func macReadMayBeServedInBackground(_ command: Command) -> Bool {
814-
guard let bundleId = command.appBundleId?.trimmedNonEmpty else { return false }
815-
return macReadMayBeServedInBackground(command: command, targetState: XCUIApplication(bundleIdentifier: bundleId).state)
816-
}
817-
818-
/// The state split of `macReadMayBeServedInBackground`, separated from the probe so the host lane
819-
/// pins every state the answer branches on, the same way `macAppCaptureNeedsRaise` is pinned.
800+
/// The probe handle must be fresh, not cached: a cached handle can address a pid an outside
801+
/// relaunch replaced, whose `.state` would answer for the dead process.
820802
func macReadMayBeServedInBackground(command: Command, targetState: XCUIApplication.State) -> Bool {
821803
command.appBundleId?.trimmedNonEmpty != nil && targetState == .runningBackground
822804
}

‎apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandDispatchTests.swift‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -761,6 +761,42 @@ extension RunnerTests {
761761
}
762762
}
763763

764+
/// The identity rule the served read must go through (#3254 review): with the cache bound to a
765+
/// pid an outside relaunch replaced, a served read answers the process a bound read would have
766+
/// read, not the dead handle the cache holds. The bogus cached pid makes the wiring observable:
767+
/// the served preparation must leave the live pid behind, so deleting the
768+
/// `refreshCachedTargetIfProcessChanged` call from the arm leaves the bogus value and fails here.
769+
@MainActor
770+
func testMacBackgroundReadRefreshesACachedHandleFromADeadPid() throws {
771+
let target = try startMacTargetAndLoseTheForeground()
772+
let bundleId = Self.macBackgroundTargetBundleId
773+
defer {
774+
invalidateCachedTarget(reason: "unit_test_cleanup")
775+
pendingTargetActivation = nil
776+
target.terminate()
777+
}
778+
let command = try runnerCommandFixture(
779+
#"{"command":"snapshot","commandId":"read","appBundleId":"\#(bundleId)"}"#
780+
)
781+
// Plant the residue an outside relaunch leaves between two reads: the cache still names this
782+
// app and still holds a handle, but the pid it recorded is dead.
783+
mainOwned.app = target
784+
mainOwned.bundleId = bundleId
785+
mainOwned.processIdentifier = 999_999
786+
pendingTargetActivation = nil
787+
788+
guard case .context(let prepared) = prepareActiveCommandContext(command: command) else {
789+
return XCTFail("the read must be served")
790+
}
791+
XCTAssertEqual(prepared.app.state, .runningBackground, "still served in place")
792+
XCTAssertNil(pendingTargetActivation, "the refresh must not activate")
793+
XCTAssertNotEqual(
794+
mainOwned.processIdentifier,
795+
999_999,
796+
"a served read must run the shared identity refresh, not answer from a dead pid"
797+
)
798+
}
799+
764800
/// The scope limit that keeps this from being a blanket no-activate rule: an interaction against
765801
/// the same background app still comes forward through the same preparation, because on macOS the
766802
/// XCTest path drives events into the foreground window and a background click would land on

0 commit comments

Comments
 (0)