Skip to content

Commit a68693c

Browse files
authored
refactor(apple): give the foreground probe one result shape (#4267)
- SystemSurfaceForegroundProbe always returns { foreground, sessionId? }, so the snapshot route drops its three narrowings. - Test fakes without a runner session answer 'unknown', as the real probe does. - The native wedge test holds a read after the admitted one and checks it at a time far past the old abandon stamp, so it fails if the stamp is not cleared. - The lease handler uses errorMessage() for the teardown diagnostic. Follow-up to #4256 and #4257.
1 parent 0a3b018 commit a68693c

7 files changed

Lines changed: 44 additions & 31 deletions

File tree

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,25 @@ extension RunnerTests {
8282
RunnerSnapshotBridgeDrainForTest()
8383
let admitted = RunnerSnapshotBridgeReadForTest(request, 1000, { ["ok": true] }, 16)
8484
XCTAssertEqual(admitted["ok"] as? Bool, true)
85+
86+
let started = DispatchSemaphore(value: 0)
87+
let secondRelease = DispatchSemaphore(value: 0)
88+
let finished = expectation(description: "second read outlives its budget")
89+
DispatchQueue.global().async {
90+
let second = RunnerSnapshotBridgeReadForTest(request, 1000, {
91+
started.signal()
92+
secondRelease.wait()
93+
return ["ok": true]
94+
}, 100)
95+
XCTAssertEqual(second["error_code"] as? String, "bridge-read-abandoned")
96+
finished.fulfill()
97+
}
98+
started.wait()
99+
let secondBusy = RunnerSnapshotBridgeReadForTest(request, 100, { ["ok": true] }, 100)
100+
XCTAssertEqual(secondBusy["error_code"] as? String, "bridge-read-in-flight")
101+
wait(for: [finished], timeout: 5)
102+
secondRelease.signal()
103+
RunnerSnapshotBridgeDrainForTest()
85104
}
86105

87106
func testReaderDoesNotRefuseTheBoundsOfAFirstRead() throws {

‎packages/daemon/src/sessions/handlers/lease.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ async function heartbeatLease(
335335
phase: 'lease_provider_ended_session_teardown_failed',
336336
data: {
337337
leaseId: lease.leaseId,
338-
error: error instanceof Error ? error.message : String(error),
338+
error: errorMessage(error),
339339
},
340340
});
341341
}

‎packages/platform-apple/src/runner-snapshot-bridge.test.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,15 @@ type RunnerProcess = { pid: number; processStartTime: string };
3939
* foreground process, and `describe` reads it unless the host planned another process, or answers
4040
* the refusal `refuseDescribe` names. A describe given `holdNextDescribe` reads until that promise
4141
* settles, and, like the runner, it keeps reading after the daemon stops waiting: a describe that
42-
* arrives meanwhile is refused as `bridge-read-in-flight`. A describe that names surface hosts is
43-
* refused as `system-surface-host-unverified`, as a sandbox that refuses the process table does.
42+
* arrives meanwhile is refused as `bridge-read-in-flight`. With `refuseSurfaceHostCheck`, a describe that
43+
* names surface hosts is refused as `system-surface-host-unverified`, as a sandbox that refuses the
44+
* process table does.
4445
*/
4546
function bridgeRunner(state: {
4647
process: RunnerProcess;
4748
refuseDescribe?: string;
4849
refuseTarget?: string;
50+
refuseSurfaceHostCheck?: boolean;
4951
holdNextDescribe?: Promise<void>;
5052
}) {
5153
const sent: RunnerCommand[] = [];
@@ -86,7 +88,7 @@ function bridgeRunner(state: {
8688
) {
8789
return refusal('stale_target', 'target-process-changed');
8890
}
89-
if (request.surfaceHostExecutables)
91+
if (request.surfaceHostExecutables && state.refuseSurfaceHostCheck)
9092
return refusal('unsupported', 'system-surface-host-unverified');
9193
if (state.refuseDescribe) return refusal('application_not_responding', state.refuseDescribe);
9294
if (reading) return refusal('application_not_responding', 'bridge-read-in-flight');
@@ -142,7 +144,7 @@ function verbs(sent: readonly RunnerCommand[]): string[] {
142144
return sent.map((command) => command.snapshotBridge?.verb ?? command.command);
143145
}
144146

145-
/** The bridge route of a runner whose foreground read proves no system surface covers the app. */
147+
/** The bridge route of a runner with an optional foreground session. */
146148
function bridgeRoute(provider: AppleRunnerProvider, runnerSessionId?: string) {
147149
const bridge = createRunnerSnapshotBridge({ device: IOS_SIMULATOR, provider });
148150
const presented: SnapshotRuntimeAcquiredResult[] = [];
@@ -165,7 +167,9 @@ function bridgeRoute(provider: AppleRunnerProvider, runnerSessionId?: string) {
165167
...(runnerSessionId ? { sessionChanged: bridge.resetTargets } : {}),
166168
},
167169
async () =>
168-
runnerSessionId ? { foreground: 'background', sessionId: runnerSessionId } : 'background',
170+
runnerSessionId
171+
? { foreground: 'background', sessionId: runnerSessionId }
172+
: { foreground: 'unknown' },
169173
);
170174
const snapshot = (signal = new AbortController().signal) =>
171175
route.capture(
@@ -427,6 +431,7 @@ test('the runner runtime reads a remote Simulator through the bridge once its fo
427431

428432
test('a runner whose foreground read cannot answer asks the bridge read to check the surface hosts', async () => {
429433
const { provider, sent } = bridgeRunner({
434+
refuseSurfaceHostCheck: true,
430435
process: { pid: 41, processStartTime: '1700000000.000001' },
431436
});
432437
const { snapshot, fallbacks } = runtimeSnapshots(withHostStates(provider, undefined));
@@ -478,6 +483,7 @@ test('the foreground proof starts the runner when none is live, then reads only
478483
test('a runner whose session id cannot be read answers the foreground proof as unknown', async () => {
479484
registry.session = undefined;
480485
const { provider, sent } = bridgeRunner({
486+
refuseSurfaceHostCheck: true,
481487
process: { pid: 41, processStartTime: '1700000000.000001' },
482488
});
483489
const { snapshot, fallbacks } = runtimeSnapshots(withHostStates(provider, 'notRunning'));
@@ -650,7 +656,7 @@ test('a wedge before the runner id is known does not disable a newly identified
650656
targetFreshness: 'cached',
651657
sessionChanged: bridge.resetTargets,
652658
},
653-
async () => (sessionId ? { foreground: 'background', sessionId } : 'background'),
659+
async () => (sessionId ? { foreground: 'background', sessionId } : { foreground: 'unknown' }),
654660
);
655661
const snapshot = () =>
656662
route.capture(

‎packages/platform-apple/src/snapshot-route.test.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,12 @@ test.for([
132132
async ({ runner, host, acquired, checked, served }) => {
133133
const source = sourceReturning(host === 'present' ? surfaceHostPresent : bridgeAcquisition());
134134
const fallback = vi.fn(async () => runnerResult());
135-
const systemSurfaceForeground = vi.fn(async () =>
136-
runner === 'foreground'
137-
? ({ kind: 'foreground', host: presentSurface.host } as const)
138-
: runner,
139-
);
135+
const systemSurfaceForeground = vi.fn(async () => ({
136+
foreground:
137+
runner === 'foreground'
138+
? ({ kind: 'foreground', host: presentSurface.host } as const)
139+
: runner,
140+
}));
140141
const route = createAppleSnapshotRoute(bridgePresentingHost(), {
141142
source,
142143
resolveTarget: vi.fn(async () => target),

‎packages/platform-apple/src/snapshot-route.ts‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -211,10 +211,7 @@ function createSnapshotBridgeRouteCore<Target extends SnapshotBridgeRouteTarget>
211211
probeResult: Awaited<ReturnType<SystemSurfaceForegroundProbe>>,
212212
device: DeviceInfo,
213213
): typeof currentCircuit => {
214-
const probedSessionId =
215-
channel.sessionChanged && typeof probeResult === 'object' && 'sessionId' in probeResult
216-
? probeResult.sessionId
217-
: undefined;
214+
const probedSessionId = channel.sessionChanged ? probeResult.sessionId : undefined;
218215
if (probedSessionId === undefined) return startingCircuit;
219216
if (currentCircuit !== startingCircuit) {
220217
return probedSessionId === currentCircuit.sessionId ? currentCircuit : startingCircuit;
@@ -264,10 +261,7 @@ function createSnapshotBridgeRouteCore<Target extends SnapshotBridgeRouteTarget>
264261
const startingCircuit = currentCircuit;
265262
const probeResult = await readSurfaceForeground(device, input, signal, surfaceForeground);
266263
const circuit = circuitForProbe(startingCircuit, probeResult, device);
267-
const foreground =
268-
typeof probeResult === 'object' && 'sessionId' in probeResult
269-
? probeResult.foreground
270-
: probeResult;
264+
const { foreground } = probeResult;
271265
const captureBridgeDisabled = (target: SnapshotBridgeRouteTarget) =>
272266
isDisabledIn(circuit, target);
273267
if (foreground !== 'background' && foreground !== 'unknown') {
@@ -460,12 +454,7 @@ async function readSurfaceForeground(
460454
): Promise<Awaited<ReturnType<SystemSurfaceForegroundProbe>>> {
461455
const requestId = input.execution?.requestId;
462456
const foreground = await probe(device, { signal, ...(requestId ? { requestId } : {}) });
463-
if (
464-
foreground === 'background' ||
465-
(typeof foreground === 'object' &&
466-
'sessionId' in foreground &&
467-
foreground.foreground === 'background')
468-
) {
457+
if (foreground.foreground === 'background') {
469458
emitDiagnostic({
470459
level: 'debug',
471460
phase: 'ios_snapshot_route_surface_host_background',

‎packages/platform-apple/src/system-surface-foreground.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ test.for([
9696
] as const)('%s is unknown and sends no command', async ([, liveness]) => {
9797
runner.readRunnerSessionLiveness.mockReturnValue(liveness);
9898

99-
await expect(probe()).resolves.toBe('unknown');
99+
await expect(probe()).resolves.toEqual({ foreground: 'unknown' });
100100
expect(runner.runAppleRunnerCommand).not.toHaveBeenCalled();
101101
});
102102

‎packages/platform-apple/src/system-surface-foreground.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,7 @@ export type SystemSurfaceForeground =
2626
export type SystemSurfaceForegroundProbe = (
2727
device: DeviceInfo,
2828
input: Readonly<{ signal: AbortSignal; requestId?: string }>,
29-
) => Promise<
30-
SystemSurfaceForeground | Readonly<{ foreground: SystemSurfaceForeground; sessionId: string }>
31-
>;
29+
) => Promise<Readonly<{ foreground: SystemSurfaceForeground; sessionId?: string }>>;
3230

3331
/**
3432
* Bounds each read below the read-only resend window, so a runner busy draining abandoned XCTest
@@ -65,7 +63,7 @@ export function createSystemSurfaceForegroundProbe(
6563
): SystemSurfaceForegroundProbe {
6664
return async (device, input) => {
6765
const session = await readySession(device, input);
68-
if (!session) return 'unknown';
66+
if (!session) return { foreground: 'unknown' };
6967
// The reads go out together: each one is a runner round trip, which costs a network hop to a
7068
// remote Simulator. The answer reads them in registry order, so a decisive state settles it
7169
// without waiting for the hosts after it.

0 commit comments

Comments
 (0)