Skip to content

Commit 4e37635

Browse files
authored
fix(ios-runner): dispose a busy runner on close so open boots clean (#2552) (#2605)
* fix(ios-runner): dispose a busy runner on close so open boots clean (#2552) A runner with watchdog-abandoned main-thread work refuses every command until it drains or escalates to RUNNER_WEDGED. Plain close retained it and the next open reused it, so close recovered nothing. The runner now stamps its live main-thread occupancy on every successful response; the daemon mirrors it on the session and, at the close retention decision, disposes a runner reported busy instead of pooling it back. Killing the process is the only way to abort uncancellable XCTest work. * fix(ios-runner): tag the watchdog timeout, drain-clear busy, stop busy at close (#2552) Address review on #2552: - Runner tags the execution-watchdog timeout with the typed MAIN_THREAD_TIMEOUT code so the stalling command itself reports main-thread occupancy, not only a later RUNNER_BUSY refusal. - Host mirrors occupancy: set on RUNNER_BUSY or MAIN_THREAD_TIMEOUT, cleared by any other served reply (which reached the main thread and drained), left intact by transport failures. - Close that would retain a runner first stops it when occupied, awaited and lease-released via the new stopRunnerSessionIfBusy seam, before the retain-vs-stop decision. - Cover the production close route with lifecycle finalize tests and the Swift wire-code mapping. * refactor(ios-runner): own the close release decision in the runner module (#2552) Collapse the retain-vs-stop close decision into the module that owns the occupancy bit. Close now calls one intent, releaseRunnerOnClose(deviceId, { retain }), instead of asking stopRunnerSessionIfBusy for a boolean and re-branching in lifecycle. The runner module keeps warm reuse for an idle runner and stops one still draining, so the busy fact never crosses the package boundary, the fire-and-forget scheduling path is gone, and the close route stays awaited. * test(ios-runner): chain close release from a real MAIN_THREAD_TIMEOUT, pin daemon close by call arguments (#2552) The release-on-close test now trips MAIN_THREAD_TIMEOUT through executeRunnerCommandWithSession instead of setting runnerMainThreadBusy by hand, so it fails if the catch-side busy marking stops reaching close. The daemon mocks for releaseIosRunnerOnClose no longer restate the retain policy; the close tests assert the { retain } argument instead. The unrelated session-test-harness un-exports are reverted.
1 parent 5f6481c commit 4e37635

29 files changed

Lines changed: 655 additions & 56 deletions

‎.fallowrc.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@
9595
"exports": [
9696
"detachIosSimulatorRunnerSessionsForShutdown",
9797
"hasLiveIosRunnerSession",
98+
"releaseIosRunnerOnClose",
9899
"releaseSpeculativeIosRunnerSessionFor",
99100
"stopAllIosRunnerSessions"
100101
]
@@ -160,7 +161,6 @@
160161
"resolveRunnerAppBundleId",
161162
"detachIosSimulatorRunnerSessionsForShutdown",
162163
"getRunnerSessionSnapshot",
163-
"scheduleIosRunnerIdleStop",
164164
"stopIosRunnerSession",
165165
"stopAllIosRunnerSessions",
166166
"readStaleRunnerLease",

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -999,6 +999,14 @@ extension RunnerTests {
999999
case idle
10001000
case busy(abandonedForSeconds: TimeInterval)
10011001
case wedged(abandonedForSeconds: TimeInterval)
1002+
1003+
/// Whether the main thread is occupied by watchdog-abandoned work, for the occupancy stamp that
1004+
/// every successful response carries. Wedged is still occupied: it only differs in that a
1005+
/// restart, not waiting, is the cure.
1006+
var reportsMainThreadBusy: Bool {
1007+
if case .idle = self { return false }
1008+
return true
1009+
}
10021010
}
10031011

10041012
func currentMainThreadBusyState() -> MainThreadBusyState {

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,52 @@ extension RunnerTests {
183183
XCTAssertEqual(stamped.error?.message, "boom")
184184
}
185185

186+
func testStampingCurrentMainThreadBusyPreservesPayload() {
187+
let stamped = Response(ok: true, data: DataPayload(nodes: [], truncated: false))
188+
.stampingCurrentMainThreadBusy(true)
189+
190+
XCTAssertEqual(stamped.ok, true)
191+
XCTAssertEqual(stamped.data?.runnerMainThreadBusy, true)
192+
}
193+
194+
func testStampingCurrentMainThreadBusySkipsErrorResponses() {
195+
let response = Response(ok: false, error: ErrorPayload(code: "RUNNER_BUSY", message: "busy"))
196+
let stamped = response.stampingCurrentMainThreadBusy(true)
197+
198+
XCTAssertEqual(stamped.ok, false)
199+
XCTAssertNil(stamped.data)
200+
XCTAssertEqual(stamped.error?.code, "RUNNER_BUSY")
201+
}
202+
203+
func testMainThreadBusyStateReportsOccupancy() {
204+
XCTAssertFalse(MainThreadBusyState.idle.reportsMainThreadBusy)
205+
XCTAssertTrue(MainThreadBusyState.busy(abandonedForSeconds: 5).reportsMainThreadBusy)
206+
XCTAssertTrue(MainThreadBusyState.wedged(abandonedForSeconds: 200).reportsMainThreadBusy)
207+
XCTAssertEqual(
208+
Response(ok: true).stampingCurrentMainThreadBusy(false).data?.runnerMainThreadBusy, false)
209+
}
210+
211+
func testCommandFailedResponseTagsMainThreadTimeoutWithTypedCode() {
212+
let timeout = NSError(
213+
domain: RunnerErrorDomain.general,
214+
code: RunnerErrorCode.mainThreadExecutionTimedOut,
215+
userInfo: [NSLocalizedDescriptionKey: "main thread execution timed out"]
216+
)
217+
218+
let response = commandFailedResponse(from: timeout)
219+
220+
XCTAssertEqual(response.ok, false)
221+
XCTAssertEqual(response.error?.code, RunnerWireErrorCode.mainThreadTimeout)
222+
}
223+
224+
func testCommandFailedResponseKeepsGenericCodeForOtherErrors() {
225+
let other = NSError(domain: "SomeOtherDomain", code: 99, userInfo: nil)
226+
227+
let response = commandFailedResponse(from: other)
228+
229+
XCTAssertEqual(response.error?.code, "COMMAND_FAILED")
230+
}
231+
186232
func testJournalStoredResponseStaysUnstamped() throws {
187233
let journal = RunnerCommandJournal()
188234
let recordStart = runnerJournalCommand("recordStart", id: "record-start-anchor")

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,16 @@ extension Response {
229229
payload.currentUptimeMs = value
230230
return Response(ok: ok, data: payload, error: error)
231231
}
232+
233+
// The daemon reads this occupancy flag to decide whether a healthy response proves the runner
234+
// drained its watchdog-abandoned main-thread work. Only successful responses carry it; a refusal
235+
// is itself the busy signal and needs no stamp.
236+
func stampingCurrentMainThreadBusy(_ value: Bool) -> Response {
237+
guard ok else { return self }
238+
var payload = data ?? DataPayload()
239+
payload.runnerMainThreadBusy = value
240+
return Response(ok: ok, data: payload, error: error)
241+
}
232242
}
233243

234244
struct DataPayload: Codable {
@@ -276,6 +286,11 @@ struct DataPayload: Codable {
276286
var textEntryRoute: String?
277287
var runnerFatal: Bool?
278288
var runnerFatalReason: String?
289+
/// Whether main-thread XCTest work past the execution watchdog is still draining when this
290+
/// response is written. A private-AX snapshot can be served successfully while an abandoned tree
291+
/// crawl still grinds, so the healthy response must carry the live occupancy rather than let the
292+
/// daemon read `ok` as proof the runner drained (#2552).
293+
var runnerMainThreadBusy: Bool?
279294
var completedSteps: Int?
280295
var failedStepIndex: Int?
281296
var sequenceResults: [SequenceStepResult]?

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

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -152,14 +152,7 @@ extension RunnerTests {
152152
self.deliverCommandResult(
153153
command: command,
154154
result: (
155-
self.jsonResponse(
156-
status: 500,
157-
response: self.errorResponse(
158-
code: "COMMAND_FAILED",
159-
message: error.localizedDescription,
160-
hint: "Check the runner log for XCTest details, then retry after the app is foregrounded if this was a timeout or activation failure."
161-
)
162-
),
155+
self.jsonResponse(status: 500, response: self.commandFailedResponse(from: error)),
163156
false
164157
),
165158
completion: completion
@@ -274,7 +267,9 @@ extension RunnerTests {
274267
// rather than pairing a stale uptime with a much-later receipt time.
275268
let stamped =
276269
response.ok
277-
? response.stampingCurrentUptimeMs(ProcessInfo.processInfo.systemUptime * 1000)
270+
? response
271+
.stampingCurrentUptimeMs(ProcessInfo.processInfo.systemUptime * 1000)
272+
.stampingCurrentMainThreadBusy(currentMainThreadBusyState().reportsMainThreadBusy)
278273
: response
279274
let encoder = JSONEncoder()
280275
let body = (try? encoder.encode(stamped)).flatMap { String(data: $0, encoding: .utf8) } ?? "{}"
@@ -285,6 +280,32 @@ extension RunnerTests {
285280
Response(ok: false, error: ErrorPayload(code: code, message: message, hint: hint))
286281
}
287282

283+
/// Turns a thrown command error into its wire response. The execution-watchdog timeout keeps its
284+
/// own typed code so the daemon records the runner as main-thread-occupied from the stalling
285+
/// command itself, not only from a later `RUNNER_BUSY` refusal (#2552); every other throw stays the
286+
/// generic `COMMAND_FAILED`.
287+
func commandFailedResponse(from error: Error) -> Response {
288+
let nsError = error as NSError
289+
if nsError.domain == RunnerErrorDomain.general,
290+
nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut
291+
{
292+
return Response(
293+
ok: false,
294+
error: ErrorPayload(
295+
code: RunnerWireErrorCode.mainThreadTimeout,
296+
message: nsError.localizedDescription,
297+
hint:
298+
"The runner abandoned this command's main-thread work past its execution watchdog and it is still draining. Wait and retry, or use a screenshot and interact by coordinates."
299+
)
300+
)
301+
}
302+
return errorResponse(
303+
code: "COMMAND_FAILED",
304+
message: error.localizedDescription,
305+
hint: "Check the runner log for XCTest details, then retry after the app is foregrounded if this was a timeout or activation failure."
306+
)
307+
}
308+
288309
private func httpResponse(status: Int, body: String) -> Data {
289310
let headers = [
290311
"HTTP/1.1 \(status) OK",

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,13 @@ final class RunnerTests: XCTestCase {
2828
static let objcException = 1
2929
}
3030

31+
/// String codes the daemon keys behavior on. `RUNNER_BUSY` and `RUNNER_WEDGED` come from the busy
32+
/// gate; `MAIN_THREAD_TIMEOUT` is emitted by the transport when a command trips the execution
33+
/// watchdog, so the daemon can tell "the main thread is now occupied" from a generic failure.
34+
enum RunnerWireErrorCode {
35+
static let mainThreadTimeout = "MAIN_THREAD_TIMEOUT"
36+
}
37+
3138
static let springboardBundleId = "com.apple.springboard"
3239
// SpringBoard hosts blocking system modals on iOS/visionOS; tvOS (PineBoard/HeadBoard)
3340
// and macOS have no such host, so there is nothing to probe there.

‎docs/adr/0005-ios-runner-interaction-lifecycle.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,22 @@ whether the runner is alive and accepting new HTTP requests.
5656
Dead cached runner processes are invalidated without graceful `shutdown`. A process that already
5757
stopped cannot answer the shutdown request, so graceful cleanup only adds stale-listener delay.
5858

59+
The runner stamps its live main-thread occupancy (`runnerMainThreadBusy`) onto every successful
60+
response and tags the execution-watchdog timeout with the typed `MAIN_THREAD_TIMEOUT` code, so the
61+
daemon learns the main thread is occupied from the stalling command itself, not only from a later
62+
`RUNNER_BUSY` refusal. The daemon mirrors that occupancy on the `RunnerSession`: set on `RUNNER_BUSY`
63+
or `MAIN_THREAD_TIMEOUT`, cleared by any other served runner reply (which reached the main thread and
64+
therefore drained), and left intact by a transport failure or an unstamped recovered response. A
65+
healthy `ok` is never read as proof of drain because a private-AX snapshot can be served while an
66+
abandoned tree crawl still grinds on the XCTest main thread (#2552).
67+
68+
Close that would retain a runner for reuse first stops it when that occupancy is set, awaiting the
69+
stop so the lease is released before the next request, because a runner still finishing
70+
watchdog-abandoned work refuses every command until it drains or escalates to `RUNNER_WEDGED`; pooling
71+
it back to the next `open` hands the same stalled runner to the caller and `close` recovers nothing.
72+
Killing the process is the only way to abort uncancellable XCTest work. This is the `RUNNER_WEDGED`
73+
restart from #1105 applied at the close boundary rather than after the wedge threshold elapses.
74+
5975
When XCTest reports a root accessibility snapshot failure such as `kAXErrorIllegalArgument`, the
6076
runner treats the cached app target as suspect. Interactive snapshots fail closed to a truncated
6177
root-only payload instead of issuing more flat fallback queries against the same broken tree, and
@@ -84,6 +100,10 @@ If xcodebuild still exits for another reason, the next command detects the stale
84100
process/liveness checks and avoids the old 15-second graceful-shutdown wait. The remaining latency is
85101
fresh xcodebuild runner startup, not a stale transport stall.
86102

103+
A `close` then `open` on a runner still draining abandoned main-thread work now pays a fresh runner boot
104+
instead of inheriting the stalled one, so the wedge no longer survives the close/open cycle. Runners
105+
that were never reported busy keep the existing warm-reuse path unchanged.
106+
87107
The daemon no longer models a generic "recent success" cache as a runner-health signal. A proven
88108
healthy mutating response for the same app — recorded only after the `runnerFatal` check and only
89109
for allowlisted interactions — is now a real end-to-end liveness proof (HTTP listener through to the

‎packages/contracts/src/application-lifecycle-runtime.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,13 @@ export type AppleApplicationTools = Readonly<{
315315
device: DeviceInfo,
316316
execution: Readonly<{ requestId?: string }>,
317317
): Promise<boolean>;
318-
scheduleRunnerIdleStop(deviceId: string): void;
318+
/**
319+
* Releases this device's runner at session close. When `retain` is set and the runner is idle it
320+
* keeps warm reuse under an idle-stop timer; otherwise it stops now. A runner whose last exchange
321+
* reported main-thread work still draining is never retained, so a stalled process is not pooled
322+
* back out to the next `open` (#2552). Awaited so `close` returns only once the lease is gone.
323+
*/
324+
releaseRunnerOnClose(deviceId: string, options: Readonly<{ retain: boolean }>): Promise<void>;
319325
prepareRunner(
320326
device: DeviceInfo,
321327
input: PrepareAppleRunnerInput,

‎packages/platform-apple/src/core/runner-client.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ export const detachIosSimulatorRunnerSessionsForShutdown: AppleRunnerClient['det
3737
client.detachIosSimulatorRunnerSessionsForShutdown;
3838
export const getRunnerSessionSnapshot: AppleRunnerClient['getRunnerSessionSnapshot'] =
3939
client.getRunnerSessionSnapshot;
40-
export const scheduleIosRunnerIdleStop: AppleRunnerClient['scheduleIosRunnerIdleStop'] =
41-
client.scheduleIosRunnerIdleStop;
40+
export const releaseIosRunnerOnClose: AppleRunnerClient['releaseIosRunnerOnClose'] =
41+
client.releaseIosRunnerOnClose;
4242
export const stopIosRunnerSession: AppleRunnerClient['stopIosRunnerSession'] =
4343
client.stopIosRunnerSession;
4444
export const stopAllIosRunnerSessions: AppleRunnerClient['stopAllIosRunnerSessions'] =

‎packages/platform-apple/src/lifecycle.test.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,56 @@ test('discards a retained physical iOS runner when relaunch fails and preserves
244244
expect(notifyRunnerAppRelaunched).not.toHaveBeenCalled();
245245
});
246246

247+
test.each([true, false])(
248+
'close finalization delegates the runner release to the runner module with retain=%s (#2552)',
249+
async (retainRunner) => {
250+
const signal = new AbortController().signal;
251+
const baseHost = platformRuntimeHostFixture();
252+
const releaseRunnerOnClose = vi.fn(async () => {});
253+
const dismissCloseAlerts = vi.fn(async () => {});
254+
const host = {
255+
...baseHost,
256+
appleApplications: {
257+
...baseHost.appleApplications,
258+
releaseRunnerOnClose,
259+
dismissCloseAlerts,
260+
},
261+
} as unknown as PlatformRuntimeHost;
262+
const lifecycle = bindAppleApplicationLifecycle({ host, device, signal });
263+
264+
await lifecycle.finalizeApplicationClose({ surface: 'app', retainRunner, stateDir: '/tmp' });
265+
266+
expect(releaseRunnerOnClose).toHaveBeenCalledWith(device.id, { retain: retainRunner });
267+
expect(dismissCloseAlerts).toHaveBeenCalled();
268+
},
269+
);
270+
271+
test('daemon-shutdown finalization dismisses alerts and defers the runner release to the gateway (#2552)', async () => {
272+
const signal = new AbortController().signal;
273+
const baseHost = platformRuntimeHostFixture();
274+
const releaseRunnerOnClose = vi.fn(async () => {});
275+
const dismissCloseAlerts = vi.fn(async () => {});
276+
const host = {
277+
...baseHost,
278+
appleApplications: {
279+
...baseHost.appleApplications,
280+
releaseRunnerOnClose,
281+
dismissCloseAlerts,
282+
},
283+
} as unknown as PlatformRuntimeHost;
284+
const lifecycle = bindAppleApplicationLifecycle({ host, device, signal });
285+
286+
await lifecycle.finalizeApplicationClose({
287+
surface: 'app',
288+
retainRunner: true,
289+
stateDir: '/tmp',
290+
daemonShutdown: true,
291+
});
292+
293+
expect(releaseRunnerOnClose).not.toHaveBeenCalled();
294+
expect(dismissCloseAlerts).toHaveBeenCalled();
295+
});
296+
247297
test('prepare shares one startup budget across the Simulator boot and the runner preparation', async () => {
248298
vi.useFakeTimers();
249299
try {

0 commit comments

Comments
 (0)