From 40da0ef0d439d00597d8e210309ffa3f678b5429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 12:11:01 +0100 Subject: [PATCH 01/17] fix(ios): let the command journal decide admission once per commandId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runner kept command lifecycle in two places: the transport's in-flight id set and the command journal. They were cleared at different moments, so a repeat of a commandId whose answer had already been delivered was admitted again and ran the command a second time — a second tap. Pruning could also evict a command that was still executing, which the daemon reads as notAccepted, and a late transition for a pruned id could invent an entry the runner never claimed. RunnerCommandJournal now answers each inbound request with one verdict: execute, join, or terminal. A terminal id is answered from the recorded outcome, then the recorded error, then COMMAND_ALREADY_FINISHED. Pruning drops only closed entries, and an update never opens a record again. The journal closes a commandId at the last fast RUNNER_BUSY or RUNNER_WEDGED refusal, so the read-only busy resend keeps its reused id (ADR 0005). The transport keeps only the waiter list. --- .../RunnerTests+CommandDispatch.swift | 4 + .../RunnerTests+CommandJournal.swift | 163 +++++++++++++++--- .../RunnerTests+Transport.swift | 135 ++++++++------- .../RunnerTests.swift | 10 +- .../RunnerTests+CommandJournalTests.swift | 89 ++++++++++ .../RunnerTests+TransportTests.swift | 129 +++++++++++--- .../0005-ios-runner-interaction-lifecycle.md | 22 +++ 7 files changed, 441 insertions(+), 111 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift index e49a86bfdd..ee76ba52af 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift @@ -189,6 +189,10 @@ extension RunnerTests { if let unavailable = runnerUnavailableResponse(command: command) { return unavailable } + // Past the last fast refusal, this command's work is going to the device, so its `commandId` is + // spent: a repeat of it is answered from the journal instead of running the device again. The + // read-only busy resend arrives before this point, never after it (ADR 0005). + commandJournal.markDispatched(command: command) if command.command == .snapshot { return try executeSnapshotDispatched(command: command) } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 18ac85605e..9a9e6b7b57 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -10,6 +10,14 @@ enum RunnerCommandLifecycleState: String { case failed } +extension RunnerCommandLifecycleState { + /// Whether execution has ended for an entry in this state. `notAccepted` is never written to an + /// entry — `status` synthesizes it for an id the journal does not hold — so it is not terminal. + var isTerminal: Bool { + self == .completed || self == .failed + } +} + struct RunnerCommandJournalEntry { let commandId: String let command: String @@ -17,8 +25,28 @@ struct RunnerCommandJournalEntry { var responseOk: Bool? var responseJson: String? var error: ErrorPayload? + /// Whether any of this command's work reached the device. The runner refuses a command whose main + /// thread is still draining abandoned work before running anything, so such a refusal left nothing + /// on the device and its terminal entry must not close the id: the read-only busy resend carries the + /// same `commandId` (ADR 0005). + var dispatched = false +} + +/// The journal's verdict on one command that carries a `commandId`. +enum RunnerCommandAdmission { + /// Nothing is running for this id and nothing has finished: the caller owns the one execution. + case execute + /// An execution is claimed and has not finished: the caller joins its outcome and runs nothing. + case join + /// Execution finished, and the work reached the device: the caller answers from the journal and + /// never executes this command again. + case terminal(outcome: Response) } +/// The runner's one command-lifecycle record: what it holds for a `commandId`, whether that id may run, +/// and what `status` answers for it. The transport asks this type before enqueuing anything, so one +/// `commandId` is executed at most once, and a command that already finished cannot be replayed onto the +/// device by a repeat that arrives after its answer was delivered. final class RunnerCommandJournal { private let lock = NSLock() private let maxEntries = 64 @@ -26,25 +54,47 @@ final class RunnerCommandJournal { private var entries: [String: RunnerCommandJournalEntry] = [:] private var order: [String] = [] - func accept(command: Command) { - guard let commandId = command.commandId?.trimmedNonEmpty else { return } + @discardableResult + func accept(command: Command) -> RunnerCommandAdmission { + guard let commandId = command.commandId?.trimmedNonEmpty else { return .execute } lock.lock() defer { lock.unlock() } - entries[commandId] = RunnerCommandJournalEntry( - commandId: commandId, - command: command.command.rawValue, - state: .accepted, + guard let entry = entries[commandId] else { + return claim(command: command, commandId: commandId) + } + if !entry.state.isTerminal { + return .join + } + // Nothing of this command ever reached the device, so the id carries no effect to protect and the + // send that reuses it is that command's own first execution. + guard entry.dispatched else { + return claim(command: command, commandId: commandId) + } + return .terminal(outcome: recordedOutcome(for: entry)) + } + + func start(command: Command) { + update( + command: command, + state: .started, responseOk: nil, responseJson: nil, - error: nil + error: nil, + marksDispatched: false ) - order.removeAll { $0 == commandId } - order.append(commandId) - pruneIfNeeded() } - func start(command: Command) { - update(command: command, state: .started, responseOk: nil, responseJson: nil, error: nil) + /// Records that this command's work was handed to the device, as opposed to being refused while it + /// was still queued. Until this point a refusal owes the caller a retry of the same `commandId`. + func markDispatched(command: Command) { + update( + command: command, + state: .started, + responseOk: nil, + responseJson: nil, + error: nil, + marksDispatched: true + ) } func finish(command: Command, response: Response) { @@ -53,7 +103,8 @@ final class RunnerCommandJournal { state: response.ok ? .completed : .failed, responseOk: response.ok, responseJson: encodeResponseJson(command: command, response: response), - error: response.error + error: response.error, + marksDispatched: false ) } @@ -63,10 +114,24 @@ final class RunnerCommandJournal { state: .failed, responseOk: nil, responseJson: nil, - error: ErrorPayload(message: error.localizedDescription) + error: ErrorPayload(message: error.localizedDescription), + marksDispatched: false ) } + /// The answer owed a send that joined an execution which has since closed: the outcome the runner + /// recorded, whether or not the work reached the device. Registering that send as a waiter instead + /// would leave it waiting on a delivery that already went out. `accept` answers a closed id on its own + /// terms, so this is narrower than admission. + func closedOutcome(for command: Command) -> Response? { + guard let commandId = command.commandId?.trimmedNonEmpty else { return nil } + lock.lock() + let entry = entries[commandId] + lock.unlock() + guard let entry, entry.state.isTerminal else { return nil } + return recordedOutcome(for: entry) + } + func status(normalizedCommandId commandId: String) -> DataPayload { lock.lock() let entry = entries[commandId] @@ -89,39 +154,87 @@ final class RunnerCommandJournal { ) } + /// Writes the claim for one execution. Callers hold the lock. + private func claim(command: Command, commandId: String) -> RunnerCommandAdmission { + entries[commandId] = RunnerCommandJournalEntry( + commandId: commandId, + command: command.command.rawValue, + state: .accepted, + responseOk: nil, + responseJson: nil, + error: nil + ) + order.removeAll { $0 == commandId } + order.append(commandId) + pruneIfNeeded() + return .execute + } + + /// Moves one claimed entry forward. A transition for an id the journal does not hold describes work it + /// never claimed, and one for an entry that already finished restates a closed outcome: neither may + /// open a record again. Re-opening one would report `accepted` or `started` for work that is not + /// running, and the daemon holds that verdict as an in-flight exchange on a runner it would otherwise + /// hand off. private func update( command: Command, state: RunnerCommandLifecycleState, responseOk: Bool?, responseJson: String?, - error: ErrorPayload? + error: ErrorPayload?, + marksDispatched: Bool ) { guard let commandId = command.commandId?.trimmedNonEmpty else { return } lock.lock() defer { lock.unlock() } - var entry = entries[commandId] ?? RunnerCommandJournalEntry( - commandId: commandId, - command: command.command.rawValue, - state: .accepted, - responseOk: nil, - responseJson: nil, - error: nil - ) + guard var entry = entries[commandId], !entry.state.isTerminal else { return } entry.state = state entry.responseOk = responseOk entry.responseJson = responseJson entry.error = error + if marksDispatched { + entry.dispatched = true + } entries[commandId] = entry order.removeAll { $0 == commandId } order.append(commandId) pruneIfNeeded() } + /// Drops the oldest closed entries. An entry whose execution is claimed but not finished is never + /// dropped, however deep the backlog: `status` answers `notAccepted` only for an id this journal does + /// not hold, and the daemon cannot read that as a safe terminal state — it would invalidate a session + /// whose runner is still executing the command it lost the answer to. private func pruneIfNeeded() { while order.count > maxEntries { - let removed = order.removeFirst() - entries.removeValue(forKey: removed) + guard let removable = order.first(where: { entries[$0]?.state.isTerminal == true }) else { + return + } + order.removeAll { $0 == removable } + entries.removeValue(forKey: removable) + } + } + + /// The answer owed a repeat of a finished `commandId`: the outcome the runner kept, then the failure it + /// kept, and only then a typed refusal for a command whose own classification retains no body at all. + private func recordedOutcome(for entry: RunnerCommandJournalEntry) -> Response { + if let responseJson = entry.responseJson, + let recorded = try? JSONDecoder().decode(Response.self, from: Data(responseJson.utf8)) + { + return recorded } + if let error = entry.error { + return Response(ok: false, error: error) + } + return Response( + ok: false, + error: ErrorPayload( + code: RunnerTests.RunnerWireErrorCode.commandAlreadyFinished, + message: + "The runner already finished \(entry.command) with id \(entry.commandId) and kept no response to replay.", + hint: + "Read the screen with snapshot before sending anything else: the finished command's effect is already on the device." + ) + ) } private func encodeResponseJson(command: Command, response: Response) -> String? { diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index 7deeea8b19..0e49061d78 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -203,46 +203,59 @@ extension RunnerTests { completion((jsonResponse(status: 200, response: response), false)) return } - // Re-sends of a still-executing commandId (the daemon's transport retry loop) attach to - // the in-flight execution and receive its response instead of piling a second execution - // onto the main queue behind it (#1105 capture pileup). - if attachToInFlightCommandIfNeeded(command: command, completion: completion) { - return - } - NSLog( - "AGENT_DEVICE_RUNNER_COMMAND_ACCEPTED command=%@ commandId=%@", - command.command.rawValue, - command.commandId ?? "" - ) - enqueueAccepted(command: command) { result in - switch result { - case .success(let response): - NSLog( - "AGENT_DEVICE_RUNNER_COMMAND_COMPLETED command=%@ commandId=%@ ok=%d", - command.command.rawValue, - command.commandId ?? "", - response.ok ? 1 : 0 - ) - self.deliverCommandResult( - command: command, - result: (self.jsonResponse(status: 200, response: response), command.command == .shutdown), - completion: completion - ) - case .failure(let error): - NSLog( - "AGENT_DEVICE_RUNNER_COMMAND_FAILED command=%@ commandId=%@ error=%@", - command.command.rawValue, - command.commandId ?? "", - String(describing: error) - ) - self.deliverCommandResult( - command: command, - result: ( - self.jsonResponse(status: 500, response: self.commandFailedResponse(from: error)), - false - ), - completion: completion - ) + // The journal decides whether this send may run the command at all. Reading that verdict here, at + // the one route an inbound request takes, is what keeps one `commandId` to one execution: a + // command that already finished is answered from the journal, so a repeat that arrives after its + // answer was delivered cannot drive the device a second time. + switch commandJournal.accept(command: command) { + case .join: + // Re-sends of a still-executing commandId (the daemon's transport retry loop) attach to the + // in-flight execution and receive its response instead of piling a second execution onto the + // main queue behind it (#1105 capture pileup). + attachToInFlightExecution(command: command, completion: completion) + case .terminal(let outcome): + NSLog( + "AGENT_DEVICE_RUNNER_COMMAND_ALREADY_FINISHED command=%@ commandId=%@", + command.command.rawValue, + command.commandId ?? "" + ) + completion((jsonResponse(status: 200, response: outcome), false)) + case .execute: + NSLog( + "AGENT_DEVICE_RUNNER_COMMAND_ACCEPTED command=%@ commandId=%@", + command.command.rawValue, + command.commandId ?? "" + ) + enqueueAdmittedExecution(command: command) { result in + switch result { + case .success(let response): + NSLog( + "AGENT_DEVICE_RUNNER_COMMAND_COMPLETED command=%@ commandId=%@ ok=%d", + command.command.rawValue, + command.commandId ?? "", + response.ok ? 1 : 0 + ) + self.deliverCommandResult( + command: command, + result: (self.jsonResponse(status: 200, response: response), command.command == .shutdown), + completion: completion + ) + case .failure(let error): + NSLog( + "AGENT_DEVICE_RUNNER_COMMAND_FAILED command=%@ commandId=%@ error=%@", + command.command.rawValue, + command.commandId ?? "", + String(describing: error) + ) + self.deliverCommandResult( + command: command, + result: ( + self.jsonResponse(status: 500, response: self.commandFailedResponse(from: error)), + false + ), + completion: completion + ) + } } } } catch { @@ -274,13 +287,13 @@ extension RunnerTests { } } - /// Journal-accepts `command` and executes it on `commandExecutionQueue`; `completion` runs on that - /// queue. - func enqueueAccepted( + /// Executes an already-admitted `command` on `commandExecutionQueue`; `completion` runs on that + /// queue. Admission is not this function's decision: the route asked the journal for the verdict + /// before calling it, so nothing here can re-admit a spent `commandId`. + func enqueueAdmittedExecution( command: Command, completion: @escaping @Sendable (Result) -> Void ) { - commandJournal.accept(command: command) commandExecutionQueue.async { completion(Result { try self.executeAccepted(command: command) }) } @@ -288,28 +301,29 @@ extension RunnerTests { // MARK: - In-Flight Command Coalescing - /// Returns true when this send duplicated a still-executing commandId and was attached as a - /// waiter of the in-flight execution. Otherwise marks the commandId in flight and returns - /// false so the caller enqueues the (single) execution. - func attachToInFlightCommandIfNeeded( + /// Attaches this send to the in-flight execution the journal just told it to join, so the execution's + /// own delivery answers it. The journal closes an entry before its delivery drains this waiter list, and + /// both take `inFlightCommandLock`, so checking here under that lock decides which answer this send is + /// owed exactly once: one that arrives after the entry closed answers from the journal instead of + /// waiting for a delivery that already went out. + func attachToInFlightExecution( command: Command, completion: @escaping @Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void - ) -> Bool { - guard let commandId = command.commandId?.trimmedNonEmpty else { return false } + ) { + guard let commandId = command.commandId?.trimmedNonEmpty else { return } inFlightCommandLock.lock() - if inFlightCommandIds.contains(commandId) { - inFlightCommandWaiters[commandId, default: []].append(completion) + if let outcome = commandJournal.closedOutcome(for: command) { inFlightCommandLock.unlock() - NSLog( - "AGENT_DEVICE_RUNNER_COMMAND_COALESCED command=%@ commandId=%@", - command.command.rawValue, - commandId - ) - return true + completion((jsonResponse(status: 200, response: outcome), false)) + return } - inFlightCommandIds.insert(commandId) + inFlightCommandWaiters[commandId, default: []].append(completion) inFlightCommandLock.unlock() - return false + NSLog( + "AGENT_DEVICE_RUNNER_COMMAND_COALESCED command=%@ commandId=%@", + command.command.rawValue, + commandId + ) } func deliverCommandResult( @@ -320,7 +334,6 @@ extension RunnerTests { var waiters: [@Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void] = [] if let commandId = command.commandId?.trimmedNonEmpty { inFlightCommandLock.lock() - inFlightCommandIds.remove(commandId) waiters = inFlightCommandWaiters.removeValue(forKey: commandId) ?? [] inFlightCommandLock.unlock() } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift index d152fa5cda..b99d68e6dd 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -44,6 +44,10 @@ final class RunnerTests: XCTestCase { static let noTextInputAtPoint = "NO_TEXT_INPUT_AT_POINT" static let textInputProbeUnavailable = "TEXT_INPUT_PROBE_UNAVAILABLE" static let alertButtonNotFound = "ALERT_BUTTON_NOT_FOUND" + /// Refuses a repeat of a `commandId` the runner already finished, when that command's own + /// classification keeps no response body to replay. Nothing ran for the repeat, so it discloses + /// dispatched: no, and its hint sends the caller to observe the screen instead of resending. + static let commandAlreadyFinished = "COMMAND_ALREADY_FINISHED" } static let springboardBundleId = "com.apple.springboard" @@ -104,10 +108,10 @@ final class RunnerTests: XCTestCase { var firstInteractionWindow: FirstInteractionWindow? var activeRecording: ScreenRecorder? let commandJournal = RunnerCommandJournal() - // Coalesces duplicate transport sends of the same commandId onto the single in-flight - // execution instead of enqueueing them again behind it (#1105 capture pileup). let inFlightCommandLock = NSLock() - var inFlightCommandIds: Set = [] + // Waits of a `commandId` whose execution is in flight, so the one execution answers every send that + // joined it (#1105 capture pileup). The journal owns whether an execution is in flight; this holds + // only the sends that attached to it. var inFlightCommandWaiters: [String: [@Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void]] = [:] // Tracks main-queue work abandoned by the execution watchdog (runMainThreadWork). While any is // outstanding the main thread is occupied: new main-thread commands fail fast as busy instead diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 44555b0b76..eabf32ea45 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -200,6 +200,95 @@ extension RunnerTests { XCTAssertNil(largeReadStatus.lifecycleResponseJson) } + /// The 64-entry window must never cost the runner the ability to answer "is this command still + /// running?". A queued or executing command is the one entry pruning may not drop: `status` reports + /// `notAccepted` for an id it does not hold, and the daemon cannot read that as a safe terminal + /// state, so a snapshot burst behind one slow command would push the daemon to invalidate the session + /// of a runner that is still executing that command. + func testJournalKeepsAnExecutingCommandQueryablePastTheRetentionWindow() throws { + let journal = RunnerCommandJournal() + let running = runnerJournalCommand("tap", id: "running-tap") + journal.accept(command: running) + journal.start(command: running) + + for index in 0..<64 { + let settled = runnerJournalCommand("readText", id: "settled-\(index)") + journal.accept(command: settled) + journal.finish( + command: settled, + response: Response(ok: true, data: DataPayload(text: "x")) + ) + } + + let status = journal.status(normalizedCommandId: "running-tap") + XCTAssertEqual( + status.lifecycleState, + RunnerCommandLifecycleState.started.rawValue, + "a command the runner is still executing must stay queryable however many settled entries arrive" + ) + } + + /// A transition for an id the journal no longer holds says nothing about a command the runner is + /// running: it has no record to advance. Re-opening one would report `started` for work nobody + /// enqueued, which the daemon holds as an in-flight exchange it must wait out or invalidate on. + func testJournalDoesNotInventAnEntryForAPrunedCommandId() throws { + let journal = RunnerCommandJournal() + let early = runnerJournalCommand("tap", id: "pruned-tap") + journal.accept(command: early) + journal.finish( + command: early, + response: Response(ok: true, data: DataPayload(message: "tapped")) + ) + for index in 0..<64 { + let settled = runnerJournalCommand("readText", id: "settled-\(index)") + journal.accept(command: settled) + journal.finish( + command: settled, + response: Response(ok: true, data: DataPayload(text: "x")) + ) + } + XCTAssertEqual( + journal.status(normalizedCommandId: "pruned-tap").lifecycleState, + RunnerCommandLifecycleState.notAccepted.rawValue + ) + + journal.start(command: early) + + XCTAssertEqual( + journal.status(normalizedCommandId: "pruned-tap").lifecycleState, + RunnerCommandLifecycleState.notAccepted.rawValue, + "a late transition for a pruned id must not report it as running" + ) + } + + /// The daemon resends a read-only command the runner refused with `RUNNER_BUSY`, under its busy + /// budget and with the same `commandId` (ADR 0005). A terminal record closes only ids whose execution + /// began, so a refusal that dispatched nothing has to leave the id executable once the runner is idle, + /// and must still travel as the journal failure the status recovery classifies as `RUNNER_BUSY`. + func testJournalAdmitsAgainACommandIdRefusedBeforeDispatch() throws { + let command = try runnerJournalCommand("recordStop", id: "busy-then-idle") + abandonedMainThreadWorkCount = 1 + abandonedMainThreadWorkSince = Date(timeIntervalSinceNow: -2) + defer { + abandonedMainThreadWorkCount = 0 + abandonedMainThreadWorkSince = nil + } + + let refused = try execute(command: command) + let refusalStatus = commandJournal.status(normalizedCommandId: "busy-then-idle") + abandonedMainThreadWorkCount = 0 + abandonedMainThreadWorkSince = nil + + XCTAssertEqual(refused.error?.code, "RUNNER_BUSY") + XCTAssertEqual(refusalStatus.lifecycleState, RunnerCommandLifecycleState.failed.rawValue) + XCTAssertEqual(refusalStatus.lifecycleErrorCode, "RUNNER_BUSY") + + let served = try execute(command: command) + + XCTAssertTrue(served.ok, "the refused commandId must run once the runner is idle, got \(String(describing: served.error?.code))") + XCTAssertEqual(served.data?.message, "recording already stopped") + } + func testCommandJournalKeepsErrorMetadataWhenResponseJsonIsDropped() { let journal = RunnerCommandJournal() let snapshot = runnerJournalCommand("snapshot", id: "snapshot-error") diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index 59ebfcca2f..d0a9ab83a2 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -472,58 +472,143 @@ extension RunnerTests { } func testDuplicateCommandIdCoalescesOntoInFlightExecution() throws { + // `scroll` keeps its response body in the journal, so the test can tell a replayed outcome from the + // typed refusal a bodyless command's repeat gets. let command = try JSONDecoder().decode( Command.self, - from: Data(#"{"command":"snapshot","commandId":"snapshot-coalesce"}"#.utf8) + from: Data(#"{"command":"scroll","commandId":"scroll-coalesce"}"#.utf8) ) final class Delivered { var primaryData: RunnerHttpResponse? var waiterData: RunnerHttpResponse? + var lateJoinerData: RunnerHttpResponse? } let delivered = Delivered() - defer { - inFlightCommandIds.removeAll() - inFlightCommandWaiters.removeAll() + defer { inFlightCommandWaiters.removeAll() } + + // The execute path hands the sending connection's completion to the execution's own delivery; the + // join path registers its completion as a waiter of that execution. + guard case .execute = commandJournal.accept(command: command) else { + return XCTFail("the first send of a commandId must be admitted for execution") + } + guard case .join = commandJournal.accept(command: command) else { + return XCTFail("a send of a commandId whose execution has not finished must join it") + } + attachToInFlightExecution(command: command) { result in + delivered.waiterData = result.response } - XCTAssertFalse( - attachToInFlightCommandIfNeeded(command: command) { result in - delivered.primaryData = result.response - } - ) - XCTAssertTrue( - attachToInFlightCommandIfNeeded(command: command) { result in - delivered.waiterData = result.response - } + commandJournal.start(command: command) + commandJournal.markDispatched(command: command) + commandJournal.finish( + command: command, + response: Response(ok: true, data: DataPayload(message: "captured")) ) - let result = RunnerHttpResponse(status: 200, body: "single-result") - deliverCommandResult( - command: command, - result: (result, false) - ) { result in + deliverCommandResult(command: command, result: (result, false)) { result in delivered.primaryData = result.response } XCTAssertEqual(delivered.primaryData, result) XCTAssertEqual(delivered.waiterData, result) - XCTAssertFalse(inFlightCommandIds.contains("snapshot-coalesce")) XCTAssertNil(inFlightCommandWaiters["snapshot-coalesce"]) + + // The execution has answered and closed. A send that arrives now is the finished command's repeat, + // and it must be answered from the journal instead of waiting for a delivery that already went out. + guard case .terminal(let outcome) = commandJournal.accept(command: command) else { + return XCTFail("a send of a finished commandId must be answered from the journal") + } + XCTAssertTrue(outcome.ok) + XCTAssertEqual(outcome.data?.message, "captured") + + let lateCommand = try JSONDecoder().decode( + Command.self, + from: Data(#"{"command":"snapshot","commandId":"snapshot-late-join"}"#.utf8) + ) + commandJournal.accept(command: lateCommand) + commandJournal.markDispatched(command: lateCommand) + commandJournal.finish( + command: lateCommand, + response: Response(ok: true, data: DataPayload(message: "captured late")) + ) + attachToInFlightExecution(command: lateCommand) { result in + delivered.lateJoinerData = result.response + } + XCTAssertNotNil(delivered.lateJoinerData, "a join after the answer was recorded must still be answered") + XCTAssertNil(inFlightCommandWaiters["snapshot-late-join"], "a journal answer must not register a waiter") } - /// Routes `command` through the transport's inline and queued paths. The calling test's main - /// thread serves the command's main-thread work while it waits. + /// The bug this repo cannot survive: a repeat of a `commandId` whose command already finished used to + /// be admitted again and drive the device a second time — a second tap. Sends go through the real + /// listener here, because admission is the transport's route, not any helper's. + /// + /// `recordStop` makes the second execution observable without recording anything: with no active + /// recording it answers "recording already stopped", so a re-execution would be seen as the recorder + /// the test installed being consumed instead of the recorded answer being replayed. + func testTransportNeverRepeatsAFinishedCommandsExecution() throws { + let listener = try startTransportListener() + defer { listener.cancel() } + let port = try XCTUnwrap(listener.port) + let client = URLSession(configuration: .ephemeral) + defer { client.invalidateAndCancel() } + let payload = #"{"command":"recordStop","commandId":"record-stop-twice"}"# + + activeRecording = nil + let first = try transportHttpResponse(client: client, port: port, payload: payload) + XCTAssertTrue(first.ok, String(describing: first.error)) + XCTAssertEqual(first.data?.message, "recording already stopped") + + // Prove nothing ran the second time: had it run, this recorder would be stopped and cleared. + let installed = ScreenRecorder(outputPath: "", fps: 30) + activeRecording = installed + defer { activeRecording = nil } + + let repeatSend = try transportHttpResponse(client: client, port: port, payload: payload) + + XCTAssertIdentical(activeRecording, installed, "a finished commandId must not execute again") + XCTAssertEqual(repeatSend.ok, true) + XCTAssertEqual( + repeatSend.data?.message, + "recording already stopped", + "the repeat must be answered with the outcome the runner recorded, got \(String(describing: repeatSend.data?.message))" + ) + XCTAssertEqual( + commandJournal.status(normalizedCommandId: "record-stop-twice").lifecycleState, + RunnerCommandLifecycleState.completed.rawValue + ) + } + + /// Routes `command` the way the transport routes it: an inline read, then the journal's verdict on its + /// `commandId`. A finished id comes back as the answer the runner recorded for it; the execute path + /// rethrows what the dispatch threw, which the wire path turns into its own response. Sends that need + /// the wire path itself, including coalescing and admission end to end, go through + /// `startTransportListener` instead. func execute(command: Command) throws -> Response { dispatchPrecondition(condition: .onQueue(.main)) if let response = inlineResponse(for: command) { return response } + switch commandJournal.accept(command: command) { + case .terminal(let outcome): + return outcome + case .join: + throw NSError( + domain: RunnerErrorDomain.general, + code: RunnerErrorCode.commandReturnedNoResponse, + userInfo: [ + NSLocalizedDescriptionKey: + "this seam does not coalesce onto an in-flight execution; send through the transport listener" + ] + ) + case .execute: + break + } final class ResultBox { var result: Result? } let box = ResultBox() let executed = XCTestExpectation(description: "\(command.command.rawValue) executed off main") - enqueueAccepted(command: command) { result in + enqueueAdmittedExecution(command: command) { result in box.result = result executed.fulfill() } diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index add1b40a31..2d0c1ff8ef 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -14,6 +14,8 @@ Accepted - Invalidate dead cached runner processes without graceful `shutdown`. - After `RUNNER_BUSY`, resend only read-only commands, under their busy-specific budget. - After `RUNNER_BUSY` on a mutating command, return the refusal to the caller without a resend. +- Let the runner's command journal decide once per `commandId` whether a send executes, joins the running execution, or is answered from the recorded outcome. +- Never execute a `commandId` twice, and never evict a journal entry whose execution has not finished. - If `close` would retain a runner whose main-thread occupancy is set, stop that runner first and await the stop. - After a root AX failure, clear the cached app target. Keep raw/strict errors distinct from interactive recovery payloads. @@ -114,6 +116,26 @@ This bypass is in `runner-lifecycle.ts`. The same trait already decides the startup-preflight skip above. Thus, it is the single place for a change to that classification. +`RunnerCommandJournal` owns command admission, and the transport asks it once per inbound request. +Its verdict is `execute`, `join`, or `terminal`: + +- `execute` claims the `commandId`, and that send owns the one execution. +- `join` means an execution is claimed and unfinished. The send attaches as a waiter of it, which keeps a transport retry from stacking a second execution behind the first (#1105). +- `terminal` means execution finished with effect on the device. The runner answers with the outcome it recorded, and the command never runs again. + +The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` refusal, not at accept. +A refusal before that point dispatched nothing, so the entry stays open and the read-only busy resend above can still run under its own `commandId`. +Answering a closed `commandId` uses the retained response, then the retained error, then the typed `COMMAND_ALREADY_FINISHED` refusal for a command whose own classification retains no body. +That refusal discloses `dispatched: no`: nothing ran for the repeat. + +Pruning drops only closed entries, so a queued or executing command always stays queryable. +`status` answers `notAccepted` only for an id the journal does not hold. +The daemon cannot read `notAccepted` as a safe terminal state, so evicting a running command would push it to invalidate a session whose runner is still executing that command. + +Rejected: gating re-admission on the runner's read-only classification. That would replay a finished `snapshot` after its caller already received the answer, which is not the same command's first execution. + +Rejected: keeping the transport's own in-flight id set beside the journal. Two records of one lifecycle drifted: the set was cleared when the answer was delivered while the journal still held the entry, so a repeat arriving in that window was admitted and executed again. + If `close` would retain a runner for reuse and that occupancy is set, `close` first stops the runner. `close` awaits the stop, so the lease is released before the next request. A runner that still finishes watchdog-abandoned work refuses every command sent to it until it drains or escalates to `RUNNER_WEDGED`. From 0166b7c8ef74e981a544b64ebb11ce2a03c4345e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 13:39:25 +0100 Subject: [PATCH 02/17] fix(ios): keep a pruned commandId spent and answer a joined send from one decision Pruning an entry kept the entry's admission fact with the body it shared, so a pruned id became executable again and a delayed resend could drive the device a second time. Pruning now leaves the id in a bounded closed set that answers admission with the typed refusal, and a pruned refusal keeps its id free for the busy resend (ADR 0005). The join verdict and registering the send for the execution's answer were two steps, so a close that landed between them left the send waiting on a delivery that had already gone out. They are one operation the journal's lock covers. --- .../RunnerTests+CommandJournal.swift | 89 +++++-- .../RunnerTests+Transport.swift | 52 ++-- .../RunnerTests+CommandJournalTests.swift | 78 ++++-- .../RunnerTests+TransportTests.swift | 227 +++++++++++++----- .../0005-ios-runner-interaction-lifecycle.md | 12 +- 5 files changed, 328 insertions(+), 130 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 9a9e6b7b57..2af7a4d970 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -36,7 +36,8 @@ struct RunnerCommandJournalEntry { enum RunnerCommandAdmission { /// Nothing is running for this id and nothing has finished: the caller owns the one execution. case execute - /// An execution is claimed and has not finished: the caller joins its outcome and runs nothing. + /// An execution is claimed and has not finished: the caller runs nothing and is answered by that + /// execution. `admit(command:registeringJoin:)` has already registered the send it gave this verdict. case join /// Execution finished, and the work reached the device: the caller answers from the journal and /// never executes this command again. @@ -50,19 +51,44 @@ enum RunnerCommandAdmission { final class RunnerCommandJournal { private let lock = NSLock() private let maxEntries = 64 + /// How many closed `commandId`s are remembered as bare ids once their entry is pruned. The window is + /// 64x the entry window because a marker holds no outcome body, only the id. + private let maxClosedCommandIds = 4096 private let maxResponseJsonBytes = 16 * 1024 private var entries: [String: RunnerCommandJournalEntry] = [:] private var order: [String] = [] + /// Ids whose dispatched execution closed and whose entry has since been pruned: the admission half of + /// what the entry recorded, kept without a body so bounded memory cannot make a spent id executable + /// again. It settles admission only. `status` still answers `notAccepted` for such an id, because the + /// outcome the entry held is gone and only the entry can report one. + private var closedCommandIds: Set = [] + private var closedOrder: [String] = [] + /// Settles one send, and when the answer is to join a running execution, registers that send for the + /// answer inside the same locked operation. No transition can close the entry between the two, which is + /// what keeps a joined send from waiting on a delivery that already went out. `register` runs on the + /// calling thread while the journal's lock is held: it may take a lock of its own, and must not call + /// back into this journal. The wire route passes one; a caller that only reads the verdict — a journal + /// test, or a probe that will not itself answer for the execution — leaves it unregistered and must not + /// treat a `.join` verdict as a send that will be answered. @discardableResult - func accept(command: Command) -> RunnerCommandAdmission { + func accept( + command: Command, + registeringJoin register: (String) -> Void = { _ in } + ) -> RunnerCommandAdmission { guard let commandId = command.commandId?.trimmedNonEmpty else { return .execute } lock.lock() defer { lock.unlock() } guard let entry = entries[commandId] else { + // The entry can be pruned; the fact it recorded is not, so an id that spent itself on the device + // never executes again. + if closedCommandIds.contains(commandId) { + return .terminal(outcome: alreadyFinishedRefusal(command: command, commandId: commandId, outcomeDropped: true)) + } return claim(command: command, commandId: commandId) } if !entry.state.isTerminal { + register(commandId) return .join } // Nothing of this command ever reached the device, so the id carries no effect to protect and the @@ -119,19 +145,6 @@ final class RunnerCommandJournal { ) } - /// The answer owed a send that joined an execution which has since closed: the outcome the runner - /// recorded, whether or not the work reached the device. Registering that send as a waiter instead - /// would leave it waiting on a delivery that already went out. `accept` answers a closed id on its own - /// terms, so this is narrower than admission. - func closedOutcome(for command: Command) -> Response? { - guard let commandId = command.commandId?.trimmedNonEmpty else { return nil } - lock.lock() - let entry = entries[commandId] - lock.unlock() - guard let entry, entry.state.isTerminal else { return nil } - return recordedOutcome(for: entry) - } - func status(normalizedCommandId commandId: String) -> DataPayload { lock.lock() let entry = entries[commandId] @@ -200,16 +213,26 @@ final class RunnerCommandJournal { pruneIfNeeded() } - /// Drops the oldest closed entries. An entry whose execution is claimed but not finished is never - /// dropped, however deep the backlog: `status` answers `notAccepted` only for an id this journal does - /// not hold, and the daemon cannot read that as a safe terminal state — it would invalidate a session - /// whose runner is still executing the command it lost the answer to. + /// Drops the oldest closed entries, keeping each one's id as the fact that settles admission for it. + /// An entry whose execution is claimed but not finished is never dropped, however deep the backlog: + /// `status` answers `notAccepted` only for an id this journal does not hold, and the daemon cannot read + /// that as a safe terminal state — it would invalidate a session whose runner is still executing the + /// command it lost the answer to. private func pruneIfNeeded() { while order.count > maxEntries { guard let removable = order.first(where: { entries[$0]?.state.isTerminal == true }) else { return } order.removeAll { $0 == removable } + // Only work that reached the device spends its id. A pruned refusal leaves the id free, which is + // what the busy resend of the same `commandId` depends on (ADR 0005). + if entries[removable]?.dispatched == true { + closedCommandIds.insert(removable) + closedOrder.append(removable) + while closedOrder.count > maxClosedCommandIds { + closedCommandIds.remove(closedOrder.removeFirst()) + } + } entries.removeValue(forKey: removable) } } @@ -225,12 +248,36 @@ final class RunnerCommandJournal { if let error = entry.error { return Response(ok: false, error: error) } - return Response( + return alreadyFinishedRefusal( + commandId: entry.commandId, + commandName: entry.command, + outcomeDropped: false + ) + } + + private func alreadyFinishedRefusal( + command: Command, + commandId: String, + outcomeDropped: Bool + ) -> Response { + alreadyFinishedRefusal( + commandId: commandId, + commandName: command.command.rawValue, + outcomeDropped: outcomeDropped + ) + } + + private func alreadyFinishedRefusal( + commandId: String, + commandName: String, + outcomeDropped: Bool + ) -> Response { + Response( ok: false, error: ErrorPayload( code: RunnerTests.RunnerWireErrorCode.commandAlreadyFinished, message: - "The runner already finished \(entry.command) with id \(entry.commandId) and kept no response to replay.", + "The runner already finished \(commandName) with id \(commandId) and kept no response to replay\(outcomeDropped ? ", because that record has since been dropped" : "").", hint: "Read the screen with snapshot before sending anything else: the finished command's effect is already on the device." ) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index 0e49061d78..c927e60fb9 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -203,16 +203,25 @@ extension RunnerTests { completion((jsonResponse(status: 200, response: response), false)) return } - // The journal decides whether this send may run the command at all. Reading that verdict here, at - // the one route an inbound request takes, is what keeps one `commandId` to one execution: a + // The journal decides here, at the one route an inbound request takes, whether this send may run + // the command at all, and when the answer is to join an execution it registers this send for that + // execution's answer in the same operation. That is what keeps one `commandId` to one execution: a // command that already finished is answered from the journal, so a repeat that arrives after its - // answer was delivered cannot drive the device a second time. - switch commandJournal.accept(command: command) { + // answer was delivered cannot drive the device a second time, and a send told to join can never be + // left behind by a delivery that went out first. + let admission = commandJournal.accept(command: command) { [weak self] commandId in + self?.registerJoiner(for: commandId, completion: completion) + } + switch admission { case .join: - // Re-sends of a still-executing commandId (the daemon's transport retry loop) attach to the - // in-flight execution and receive its response instead of piling a second execution onto the - // main queue behind it (#1105 capture pileup). - attachToInFlightExecution(command: command, completion: completion) + // Re-sends of a still-executing commandId (the daemon's transport retry loop) were registered + // onto the in-flight execution above and receive its response instead of piling a second + // execution onto the main queue behind it (#1105 capture pileup). + NSLog( + "AGENT_DEVICE_RUNNER_COMMAND_COALESCED command=%@ commandId=%@", + command.command.rawValue, + command.commandId ?? "" + ) case .terminal(let outcome): NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ALREADY_FINISHED command=%@ commandId=%@", @@ -301,31 +310,22 @@ extension RunnerTests { // MARK: - In-Flight Command Coalescing - /// Attaches this send to the in-flight execution the journal just told it to join, so the execution's - /// own delivery answers it. The journal closes an entry before its delivery drains this waiter list, and - /// both take `inFlightCommandLock`, so checking here under that lock decides which answer this send is - /// owed exactly once: one that arrives after the entry closed answers from the journal instead of - /// waiting for a delivery that already went out. - func attachToInFlightExecution( - command: Command, + /// Records this send as a waiter of the in-flight execution the journal just settled it onto, so the + /// execution's own delivery answers it. The journal calls this while it holds its own lock and after it + /// checked that the entry is still running, which is what makes the two indivisible: the transition that + /// closes an entry needs that lock, and one close drains this list only after it has closed the entry, so + /// no send can be recorded here for a delivery that already went out. Takes only + /// `inFlightCommandLock`, and never the journal's, so nothing here can re-enter the journal. + func registerJoiner( + for commandId: String, completion: @escaping @Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void ) { - guard let commandId = command.commandId?.trimmedNonEmpty else { return } inFlightCommandLock.lock() - if let outcome = commandJournal.closedOutcome(for: command) { - inFlightCommandLock.unlock() - completion((jsonResponse(status: 200, response: outcome), false)) - return - } inFlightCommandWaiters[commandId, default: []].append(completion) inFlightCommandLock.unlock() - NSLog( - "AGENT_DEVICE_RUNNER_COMMAND_COALESCED command=%@ commandId=%@", - command.command.rawValue, - commandId - ) } + /// Answers the send that owns this execution, and every send the journal settled onto it. func deliverCommandResult( command: Command, result: (response: RunnerHttpResponse, shouldFinish: Bool), diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index eabf32ea45..402aa9855f 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -200,6 +200,19 @@ extension RunnerTests { XCTAssertNil(largeReadStatus.lifecycleResponseJson) } + /// Fills the 64-entry retention window the way a busy session does: closed commands, oldest first. The + /// transport's own retention tests need the same pressure, so this is the one implementation. + func runnerJournalFinishOtherCommands(_ journal: RunnerCommandJournal, _ count: Int) { + for index in 0.. RunnerHttpResponse { + let executed = commandJournal.accept(command: command) + let joined = commandJournal.accept(command: command) { [weak self] commandId in + self?.registerJoiner(for: commandId) { result in + delivered.joinerData = result.response + } } - guard case .join = commandJournal.accept(command: command) else { - return XCTFail("a send of a commandId whose execution has not finished must join it") + if case .execute = executed {} else { + XCTFail("the first send of a commandId must be admitted for execution") } - attachToInFlightExecution(command: command) { result in - delivered.waiterData = result.response + if case .join = joined {} else { + XCTFail("a send of a commandId whose execution has not finished must join it") } commandJournal.start(command: command) commandJournal.markDispatched(command: command) - commandJournal.finish( - command: command, - response: Response(ok: true, data: DataPayload(message: "captured")) - ) - let result = RunnerHttpResponse(status: 200, body: "single-result") - deliverCommandResult(command: command, result: (result, false)) { result in + if let outcome { + commandJournal.finish(command: command, response: outcome) + } else { + commandJournal.fail(command: command, error: NSError(domain: "test", code: 7, userInfo: nil)) + } + let deliveredBytes = RunnerHttpResponse(status: 200, body: "one-delivery") + deliverCommandResult(command: command, result: (deliveredBytes, false)) { result in delivered.primaryData = result.response } + return deliveredBytes + } - XCTAssertEqual(delivered.primaryData, result) - XCTAssertEqual(delivered.waiterData, result) - XCTAssertNil(inFlightCommandWaiters["snapshot-coalesce"]) + func testDuplicateCommandIdCoalescesOntoInFlightExecution() throws { + // `scroll` keeps its response body in the journal, so the test can tell a replayed outcome from the + // typed refusal a bodyless command's repeat gets. + let command = try transportCommand("scroll", id: "scroll-coalesce") + let delivered = TransportJoiner() + defer { inFlightCommandWaiters.removeAll() } + + let delivery = runExecutionWithJoiner( + command, + into: delivered, + outcome: Response(ok: true, data: DataPayload(message: "captured")) + ) + + XCTAssertEqual(delivered.primaryData, delivery) + XCTAssertEqual(delivered.joinerData, delivery) + XCTAssertNil(inFlightCommandWaiters["scroll-coalesce"]) - // The execution has answered and closed. A send that arrives now is the finished command's repeat, - // and it must be answered from the journal instead of waiting for a delivery that already went out. - guard case .terminal(let outcome) = commandJournal.accept(command: command) else { + // The execution has answered and closed. A send that arrives now is the finished command's repeat, and + // the journal settles it with the recorded outcome instead of registering it onto a delivery that + // already went out. + var registeredLateSend = false + let lateVerdict = commandJournal.accept(command: command) { _ in + registeredLateSend = true + } + guard case .terminal(let outcome) = lateVerdict else { return XCTFail("a send of a finished commandId must be answered from the journal") } + XCTAssertFalse(registeredLateSend, "a send answered from the journal must not be registered") XCTAssertTrue(outcome.ok) XCTAssertEqual(outcome.data?.message, "captured") + XCTAssertNil(inFlightCommandWaiters["scroll-coalesce"], "a journal answer must not register a waiter") + } - let lateCommand = try JSONDecoder().decode( - Command.self, - from: Data(#"{"command":"snapshot","commandId":"snapshot-late-join"}"#.utf8) - ) - commandJournal.accept(command: lateCommand) - commandJournal.markDispatched(command: lateCommand) - commandJournal.finish( - command: lateCommand, - response: Response(ok: true, data: DataPayload(message: "captured late")) + /// The interleaving that used to leave a connection unanswered: the verdict said join, and the execution + /// closed and drained before the send was registered, so nothing was left to answer it — and pruning had + /// already hidden the outcome a late check would have read. Settling and registering are one operation + /// now, so the send is registered before the transition that closes the entry can run. + func testJournalAnswersAJoinerWhenTheCloseThatAnswersItAlsoPrunesTheEntry() throws { + // A window full of open commands makes the close that ends this command also prune it: the case where + // a send that registered afterwards used to find no entry and no delivery left. + let command = try transportCommand("scroll", id: "scroll-join-pruned") + let delivered = TransportJoiner() + defer { inFlightCommandWaiters.removeAll() } + try openCommandsFillingTheJournalWindow(64) + + let delivery = runExecutionWithJoiner( + command, + into: delivered, + outcome: Response(ok: true, data: DataPayload(message: "captured")) ) - attachToInFlightExecution(command: lateCommand) { result in - delivered.lateJoinerData = result.response + assertJournalWindowDropped("scroll-join-pruned") + + XCTAssertEqual(delivery, delivered.joinerData, "the close that pruned the entry must answer its joiner") + XCTAssertNil(inFlightCommandWaiters["scroll-join-pruned"], "answering must not leave a waiter behind") + } + + /// A command that fails closes the entry through the same transition, so its joiner is owed the same + /// answer. + func testJournalAnswersAJoinerWhenTheExecutionFails() throws { + let command = try transportCommand("scroll", id: "scroll-join-failed") + let delivered = TransportJoiner() + defer { inFlightCommandWaiters.removeAll() } + + let delivery = runExecutionWithJoiner(command, into: delivered, outcome: nil) + + XCTAssertEqual(delivery, delivered.joinerData, "the failure that closed the entry must answer its joiner") + XCTAssertNil(inFlightCommandWaiters["scroll-join-failed"]) + } + + /// One send's answers on the transport route: the execution's own delivery and the join's. + final class TransportJoiner { + var primaryData: RunnerHttpResponse? + var joinerData: RunnerHttpResponse? + } + + /// Fills the retention window with commands the journal holds as running, so a close that lands after + /// them has to prune. + private func openCommandsFillingTheJournalWindow(_ count: Int) throws { + for index in 0.. Response { + try transportHttpResponse( + client: client, + port: port, + payload: #"{"command":"recordStop","commandId":"\#(id)"}"# + ) + } + func installRecorderAsEvidence() -> ScreenRecorder { + let installed = ScreenRecorder(outputPath: "", fps: 30) + activeRecording = installed + return installed + } activeRecording = nil - let first = try transportHttpResponse(client: client, port: port, payload: payload) + let first = try recordStop("record-stop-twice") XCTAssertTrue(first.ok, String(describing: first.error)) XCTAssertEqual(first.data?.message, "recording already stopped") - // Prove nothing ran the second time: had it run, this recorder would be stopped and cleared. - let installed = ScreenRecorder(outputPath: "", fps: 30) - activeRecording = installed - defer { activeRecording = nil } - - let repeatSend = try transportHttpResponse(client: client, port: port, payload: payload) - + var installed = installRecorderAsEvidence() + let repeatSend = try recordStop("record-stop-twice") XCTAssertIdentical(activeRecording, installed, "a finished commandId must not execute again") XCTAssertEqual(repeatSend.ok, true) XCTAssertEqual( @@ -576,6 +651,32 @@ extension RunnerTests { commandJournal.status(normalizedCommandId: "record-stop-twice").lifecycleState, RunnerCommandLifecycleState.completed.rawValue ) + + // Now the same route for an id whose entry pruning has dropped: only the fact the journal keeps + // beside the entry can answer for it, and that fact owes the typed refusal. + activeRecording = nil + XCTAssertTrue(try recordStop("record-stop-pruned").ok) + runnerJournalFinishOtherCommands(commandJournal, 64) + assertJournalWindowDropped("record-stop-pruned") + installed = installRecorderAsEvidence() + defer { activeRecording = nil } + + let resend = try recordStop("record-stop-pruned") + + XCTAssertIdentical( + activeRecording, + installed, + "a pruned id whose work reached the device must not execute again" + ) + XCTAssertEqual(resend.ok, false) + XCTAssertEqual(resend.error?.code, RunnerWireErrorCode.commandAlreadyFinished) + } + + private func transportCommand(_ command: String, id: String) throws -> Command { + try JSONDecoder().decode( + Command.self, + from: Data(#"{"command":"\#(command)","commandId":"\#(id)"}"#.utf8) + ) } /// Routes `command` the way the transport routes it: an inline read, then the journal's verdict on its diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index 2d0c1ff8ef..487f4a93ea 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -14,8 +14,8 @@ Accepted - Invalidate dead cached runner processes without graceful `shutdown`. - After `RUNNER_BUSY`, resend only read-only commands, under their busy-specific budget. - After `RUNNER_BUSY` on a mutating command, return the refusal to the caller without a resend. -- Let the runner's command journal decide once per `commandId` whether a send executes, joins the running execution, or is answered from the recorded outcome. -- Never execute a `commandId` twice, and never evict a journal entry whose execution has not finished. +- Let the runner's command journal decide once per `commandId` whether a send executes, joins the running execution, or is answered from the recorded outcome, and register a joining send for that answer inside the same decision. +- Never execute a `commandId` twice, never evict a journal entry whose execution has not finished, and keep the fact that a spent `commandId` was spent after its entry is pruned. - If `close` would retain a runner whose main-thread occupancy is set, stop that runner first and await the stop. - After a root AX failure, clear the cached app target. Keep raw/strict errors distinct from interactive recovery payloads. @@ -120,17 +120,21 @@ Thus, it is the single place for a change to that classification. Its verdict is `execute`, `join`, or `terminal`: - `execute` claims the `commandId`, and that send owns the one execution. -- `join` means an execution is claimed and unfinished. The send attaches as a waiter of it, which keeps a transport retry from stacking a second execution behind the first (#1105). +- `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. - `terminal` means execution finished with effect on the device. The runner answers with the outcome it recorded, and the command never runs again. The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` refusal, not at accept. A refusal before that point dispatched nothing, so the entry stays open and the read-only busy resend above can still run under its own `commandId`. Answering a closed `commandId` uses the retained response, then the retained error, then the typed `COMMAND_ALREADY_FINISHED` refusal for a command whose own classification retains no body. -That refusal discloses `dispatched: no`: nothing ran for the repeat. +That refusal is a structured failure the daemon does not retry. Its dispatch disclosure stays `unknown`: the runner will not re-run the command, and only the runner knows whether an earlier send of that id reached the device. Pruning drops only closed entries, so a queued or executing command always stays queryable. +When a closed entry that dispatched is pruned, the journal keeps its id in a separate closed-id window of 4096 ids, 64x the entry window, because a bare id carries no outcome body. +A resend inside that window is answered with the typed refusal; `status` still answers `notAccepted` for it, because only the entry can report an outcome. +An id the runner refused before dispatch is left free, which is what the read-only busy resend depends on. `status` answers `notAccepted` only for an id the journal does not hold. The daemon cannot read `notAccepted` as a safe terminal state, so evicting a running command would push it to invalidate a session whose runner is still executing that command. +A watermark over issued ids was rejected: the daemon mints `commandId` as a random UUID per request, reuses one id across retries and across runner generations, and persists nothing, so no ordering exists to compare. Rejected: gating re-admission on the runner's read-only classification. That would replay a finished `snapshot` after its caller already received the answer, which is not the same command's first execution. From aacfb1e622a7d83f9e9208ca6afeef69bce4fad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 14:47:57 +0100 Subject: [PATCH 03/17] docs(ios): state the closed-id valve as a memory bound, not an admission boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The daemon holds no per-id instant after which it never resends a commandId: its resend budgets are attempt counts and per-exchange timeouts redrawn per attempt, and a restart-and-replay posts the same id after a replacement boot. Tieing marker retention to such a deadline would need a wire-contract change, so the journal keeps a spent id for the life of the runner process and the 4096 cap is named for what it is: a memory safety valve. The eviction site and ADR 0005 now state the residual bound, and one test pins both halves of the rule — refused for the whole window, oldest id released by exactly one close past it. --- .../RunnerTests+CommandJournal.swift | 20 ++++-- .../RunnerTests+CommandJournalTests.swift | 69 ++++++++++++++++--- .../0005-ios-runner-interaction-lifecycle.md | 6 +- .../apple-runner/src/runner-session-types.ts | 5 +- 4 files changed, 81 insertions(+), 19 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 2af7a4d970..b4550e4412 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -46,21 +46,24 @@ enum RunnerCommandAdmission { /// The runner's one command-lifecycle record: what it holds for a `commandId`, whether that id may run, /// and what `status` answers for it. The transport asks this type before enqueuing anything, so one -/// `commandId` is executed at most once, and a command that already finished cannot be replayed onto the -/// device by a repeat that arrives after its answer was delivered. +/// `commandId` is executed at most once in a runner process, and a command that already finished cannot be +/// replayed onto the device by a repeat that arrives after its answer was delivered. The one release is +/// `closedCommandIdSafetyValve`, and `pruneIfNeeded` states the bound it leaves. final class RunnerCommandJournal { private let lock = NSLock() private let maxEntries = 64 - /// How many closed `commandId`s are remembered as bare ids once their entry is pruned. The window is - /// 64x the entry window because a marker holds no outcome body, only the id. - private let maxClosedCommandIds = 4096 + /// Memory safety valve on closed-id retention, not an admission boundary: the journal keeps a closed + /// id as long as it runs, and this only stops a runner from growing without limit. It is 64x the entry + /// window because a marker holds no outcome body, only the id. Tests read it rather than repeat it. + static let closedCommandIdSafetyValve = 4096 private let maxResponseJsonBytes = 16 * 1024 private var entries: [String: RunnerCommandJournalEntry] = [:] private var order: [String] = [] /// Ids whose dispatched execution closed and whose entry has since been pruned: the admission half of /// what the entry recorded, kept without a body so bounded memory cannot make a spent id executable /// again. It settles admission only. `status` still answers `notAccepted` for such an id, because the - /// outcome the entry held is gone and only the entry can report one. + /// outcome the entry held is gone and only the entry can report one. Retention ends only at the safety + /// valve above: an id older than that can be admitted again, and `pruneIfNeeded` states that bound. private var closedCommandIds: Set = [] private var closedOrder: [String] = [] @@ -229,7 +232,10 @@ final class RunnerCommandJournal { if entries[removable]?.dispatched == true { closedCommandIds.insert(removable) closedOrder.append(removable) - while closedOrder.count > maxClosedCommandIds { + while closedOrder.count > Self.closedCommandIdSafetyValve { + // The safety valve is the one place a spent id returns to circulation: a resend more than 4096 + // completed commands later can re-execute, and the daemon resends only after a lost response + // within one request deadline. closedCommandIds.remove(closedOrder.removeFirst()) } } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 402aa9855f..dc74a4a3be 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -202,10 +202,19 @@ extension RunnerTests { /// Fills the 64-entry retention window the way a busy session does: closed commands, oldest first. The /// transport's own retention tests need the same pressure, so this is the one implementation. - func runnerJournalFinishOtherCommands(_ journal: RunnerCommandJournal, _ count: Int) { + /// `dispatched` decides whether that work reached the device, which is what spends an id. + func runnerJournalFinishOtherCommands( + _ journal: RunnerCommandJournal, + _ count: Int, + idPrefix: String = "settled", + dispatched: Bool = false + ) { for index in 0.. = new Set(['completed', 'failed']); export const RUNNER_IN_FLIGHT_LIFECYCLE_STATES: ReadonlySet = new Set([ From 6c232c9382b3a416c34739b523c4b806cee4778b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 16:14:38 +0100 Subject: [PATCH 04/17] fix(runner): read a journal refusal as silent about main-thread occupancy The daemon marked the runner drained on any structured failure it did not classify as occupancy. A repeat refused by the command journal is that shape: the journal answered from its record without executing, so it observed nothing about the main thread, and a refusal landing mid-drain was read as proof the abandoned work had drained. Retention and handoff act on that report, so a runner still grinding XCTest work became eligible for reuse. Occupancy is now one three-valued verdict from the classifier, keyed on the typed runner code, and the silent reading leaves the report standing. The runner keeps stamping failures as it did: a stamp on a failure would move every other failure route, not this one. --- .../0005-ios-runner-interaction-lifecycle.md | 1 + .../runner-error-classification.test.ts | 35 +++++++++++++++++++ .../__tests__/runner-session-close.test.ts | 35 +++++++++++++++++++ packages/apple-runner/src/runner-contract.ts | 8 +++++ .../src/runner-error-classification.ts | 35 +++++++++++++++---- packages/apple-runner/src/runner-exchange.ts | 15 ++++---- 6 files changed, 113 insertions(+), 16 deletions(-) diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index c7c6350bd8..c192eb938c 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -127,6 +127,7 @@ The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGE A refusal before that point dispatched nothing, so the entry stays open and the read-only busy resend above can still run under its own `commandId`. Answering a closed `commandId` uses the retained response, then the retained error, then the typed `COMMAND_ALREADY_FINISHED` refusal for a command whose own classification retains no body. That refusal is a structured failure the daemon does not retry. Its dispatch disclosure stays `unknown`: the runner will not re-run the command, and only the runner knows whether an earlier send of that id reached the device. +That refusal also reports nothing about the runner's main thread. The journal wrote it without executing, so the runner may be idle or mid-drain and the refusal cannot say which. The daemon's occupancy rule has three readings, occupied, drained, and this one, silent, and only the first two write `runnerMainThreadBusy`; a repeat refused while abandoned work drains must leave that report standing, because retention and handoff act on it. Pruning drops only closed entries, so a queued or executing command always stays queryable. When a closed entry that dispatched is pruned, the journal keeps its id in a separate closed-id window, because a bare id carries no outcome body. diff --git a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts index e4e715600e..33760651d2 100644 --- a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts +++ b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { + COMMAND_ALREADY_FINISHED_RUNNER_CODE, NO_TEXT_INPUT_AT_POINT_RUNNER_CODE, RUNNER_SCREEN_CAPTURE_REFUSAL_RUNNER_CODES, TEXT_INPUT_PROBE_UNAVAILABLE_RUNNER_CODE, @@ -13,6 +14,7 @@ import { isRetryableRunnerError, isRunnerBusyError, resolveRunnerFatalErrorReason, + runnerMainThreadOccupancyReport, shouldRebuildCachedRunnerArtifact, shouldRestartRunnerAfterReadinessPreflight, shouldRestartRunnerBeforeCommandSend, @@ -457,6 +459,39 @@ test('only the typed RUNNER_BUSY refusal reads as busy', () => { assert.equal(isRunnerBusyError(new Error('RUNNER_BUSY')), false); }); +// --- main-thread occupancy (runnerMainThreadOccupancyReport) --- + +/** The AppError `buildRunnerResponseError` publishes for one runner wire code: its own details plus + * the structured payload that makes the reply an answer. */ +function runnerReplyFailure(runnerCode: string): AppError { + const classified = classifyRunnerReportedError(runnerCode); + return new AppError(classified.code, 'runner error', { + ...classified.details, + runner: { ok: false, error: { code: runnerCode, message: 'runner error' } }, + }); +} + +// The literal is what the Swift runner encodes, so it is the contract and not the constant name: +// a rename on one side has to fail here rather than silently split the pair. +test('the journal refusal of a finished commandId reports nothing about main-thread occupancy', () => { + assert.equal(COMMAND_ALREADY_FINISHED_RUNNER_CODE, 'COMMAND_ALREADY_FINISHED'); + assert.equal( + runnerMainThreadOccupancyReport(runnerReplyFailure('COMMAND_ALREADY_FINISHED')), + 'silent', + ); + // Without the structured runner payload there is no answer at all, which is also silent. + assert.equal(runnerMainThreadOccupancyReport(commandFailed('boom')), 'silent'); +}); + +test('occupancy reads the busy refusal and the stalling timeout as occupied, and a served failure as drained', () => { + assert.equal(runnerMainThreadOccupancyReport(runnerReplyFailure('RUNNER_BUSY')), 'occupied'); + assert.equal( + runnerMainThreadOccupancyReport(runnerReplyFailure('MAIN_THREAD_TIMEOUT')), + 'occupied', + ); + assert.equal(runnerMainThreadOccupancyReport(runnerReplyFailure('ELEMENT_NOT_FOUND')), 'drained'); +}); + // --- runner that predates the listener token --- test('a runner that does not require the token is session-fatal and never retried or replayed', () => { diff --git a/packages/apple-runner/src/__tests__/runner-session-close.test.ts b/packages/apple-runner/src/__tests__/runner-session-close.test.ts index 9594a16577..4a8b6c9b49 100644 --- a/packages/apple-runner/src/__tests__/runner-session-close.test.ts +++ b/packages/apple-runner/src/__tests__/runner-session-close.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import { EventEmitter } from 'node:events'; import { beforeEach, test, vi } from 'vitest'; import type { ExecBackgroundResult } from '@agent-device/host-kit/command'; +import { AppError } from '@agent-device/kernel/errors'; import { IOS_SIMULATOR } from './device-fixtures.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import { @@ -287,6 +288,40 @@ test('a served non-busy error clears a stale busy report so close keeps a draine assert.equal(session.runnerMainThreadBusy, false); }); +test('a journal refusal of a finished commandId leaves a busy main-thread report intact', async () => { + // The repeat this PR exists to refuse: the runner answers `COMMAND_ALREADY_FINISHED` from its + // journal without executing, so it observed nothing about its main thread. Clearing the report + // here would let retention and handoff proceed on a runner still draining abandoned work. + const session = makeRunnerSession({ state: 'ready', runnerMainThreadBusy: true }); + mockWaitForRunner.mockResolvedValueOnce(runnerResponse({ uptimeMs: 42 })); + mockSendRunnerCommandOnce.mockResolvedValueOnce( + runnerError({ + code: 'COMMAND_ALREADY_FINISHED', + message: 'The runner already finished tap with id runner-x and kept no response to replay.', + }), + ); + + // The validator pins that THIS refusal answered: a preflight or transport rejection would leave + // the report intact for the wrong reason and pass the assertion anyway. + await assert.rejects( + () => + executeRunnerCommandWithSession( + IOS_SIMULATOR, + session, + { command: 'tap', x: 120, y: 240, appBundleId: 'com.example.demo' }, + '/tmp/runner.log', + 30_000, + ), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.runnerErrorCode, 'COMMAND_ALREADY_FINISHED'); + return true; + }, + ); + + assert.equal(session.runnerMainThreadBusy, true); +}); + test('close stops an unready generation and the next use starts fresh', async () => { const device = { ...IOS_SIMULATOR, id: 'runner-session-release-on-close-sim' }; const session = await ensureRunnerSession(device, {}); diff --git a/packages/apple-runner/src/runner-contract.ts b/packages/apple-runner/src/runner-contract.ts index 98d1106543..16920d7238 100644 --- a/packages/apple-runner/src/runner-contract.ts +++ b/packages/apple-runner/src/runner-contract.ts @@ -39,6 +39,14 @@ export const RUNNER_BUSY_RUNNER_CODE = 'RUNNER_BUSY'; */ export const MAIN_THREAD_TIMEOUT_RUNNER_CODE = 'MAIN_THREAD_TIMEOUT'; +/** + * The runner's own code for refusing a repeat of a `commandId` it already finished, when that + * command's classification keeps no response body to replay. The command journal answers this; no + * command ran for the repeat. It is therefore silent about the runner's main thread, and a caller + * must not read it as drained occupancy ({@link runnerMainThreadOccupancyReport}). + */ +export const COMMAND_ALREADY_FINISHED_RUNNER_CODE = 'COMMAND_ALREADY_FINISHED'; + /** The longest readiness preflight a ready runner session pays before a conservative command. */ export const RUNNER_READY_PREFLIGHT_TIMEOUT_MS = 1_000; diff --git a/packages/apple-runner/src/runner-error-classification.ts b/packages/apple-runner/src/runner-error-classification.ts index 5d32ac8a20..bbec27351d 100644 --- a/packages/apple-runner/src/runner-error-classification.ts +++ b/packages/apple-runner/src/runner-error-classification.ts @@ -11,6 +11,7 @@ import type { IosDeveloperModeState, } from './runner-physical-device.ts'; import { + COMMAND_ALREADY_FINISHED_RUNNER_CODE, MAIN_THREAD_TIMEOUT_RUNNER_CODE, RUNNER_BUSY_RUNNER_CODE, RUNNER_WEDGED_RUNNER_CODE, @@ -657,13 +658,33 @@ export function isRetryableRunnerError(err: unknown): boolean { } /** - * True when the runner reported its main thread occupied by watchdog-abandoned XCTest work, either - * as a fast refusal of this command (`RUNNER_BUSY`) or as the timeout the stalling command itself - * returns (`MAIN_THREAD_TIMEOUT`) (#2552). Both are diagnostic-only on the wire (`COMMAND_FAILED` + - * `details.runnerErrorCode`), so family policy reads the typed detail rather than the message. + * What one runner failure proves about the runner's main thread: it is still draining the + * watchdog-abandoned XCTest work, it has drained, or the failure says nothing either way. The third + * reading is not a fallback — it is what an unanswered transport failure reports, and what the + * journal's refusal of a finished `commandId` reports, because that answer was written without the + * main thread being involved at all. */ -export function isRunnerMainThreadOccupiedError(error: unknown): boolean { - return isRunnerBusyError(error) || isRunnerMainThreadTimeoutError(error); +export type RunnerMainThreadOccupancy = 'occupied' | 'drained' | 'silent'; + +/** + * The one verdict for what a runner failure reports about main-thread occupancy. + * + * `RUNNER_BUSY` is a fast refusal of this command while abandoned work drains, and + * `MAIN_THREAD_TIMEOUT` is the timeout the stalling command itself returns, so both report the + * thread occupied (#2552). Any other structured reply was served by a runner that reached the + * request, which is evidence the abandoned work drained. Two answers report nothing: a + * transport-shaped failure never reached the runner, and `COMMAND_ALREADY_FINISHED` is the journal + * answering from its record, with no execution and therefore no observation of the thread — a + * repeat refused mid-drain is exactly that case, and reading it as drained would hand off a runner + * that is still grinding. + */ +export function runnerMainThreadOccupancyReport(error: unknown): RunnerMainThreadOccupancy { + if (isRunnerBusyError(error) || isRunnerMainThreadTimeoutError(error)) return 'occupied'; + // A transport-shaped failure never reached the runner, so it reports nothing. + if (!isStructuredRunnerFailure(error)) return 'silent'; + // The journal answered from its own record, so no execution observed the main thread. + if (error.details?.runnerErrorCode === COMMAND_ALREADY_FINISHED_RUNNER_CODE) return 'silent'; + return 'drained'; } /** @@ -691,7 +712,7 @@ export function isRunnerBusyError(error: unknown): boolean { * payload, so the command's outcome is known and no lifecycle status probe is needed to recover it. * A transport-shaped failure (aborted body, malformed payload, refused connection) answered nothing. */ -export function isStructuredRunnerFailure(error: unknown): boolean { +export function isStructuredRunnerFailure(error: unknown): error is AppError { return error instanceof AppError && error.details?.runner !== undefined; } diff --git a/packages/apple-runner/src/runner-exchange.ts b/packages/apple-runner/src/runner-exchange.ts index dfbdcbc95f..3690cc02ae 100644 --- a/packages/apple-runner/src/runner-exchange.ts +++ b/packages/apple-runner/src/runner-exchange.ts @@ -19,8 +19,8 @@ import { import { resolveRunnerFatalErrorReason, isRunnerCommandProvablyUnwritten, - isRunnerMainThreadOccupiedError, isStructuredRunnerFailure, + runnerMainThreadOccupancyReport, } from './runner-error-classification.ts'; import { canSkipRunnerReadinessPreflightAfterHealthyMutation, @@ -230,14 +230,11 @@ function recordUnansweredRunnerExchange( } else { session.commandCharges.markAbandoned(runnerCommand.commandId); } - // A main-thread occupancy report (`RUNNER_BUSY`, or the `MAIN_THREAD_TIMEOUT` the stalling - // command itself returns) marks the runner still draining. Any OTHER structured runner reply was - // served off that abandoned work, so it has drained; a transport-shaped error answered nothing - // and leaves the report intact (#2552). - if (isRunnerMainThreadOccupiedError(error)) { - session.runnerMainThreadBusy = true; - } else if (answered) { - session.runnerMainThreadBusy = false; + // Whether the runner's main thread is still draining is one verdict from the classifier, and + // `silent` is the reading that changes nothing: whatever the last real answer reported stands. + const occupancy = runnerMainThreadOccupancyReport(error); + if (occupancy !== 'silent') { + session.runnerMainThreadBusy = occupancy === 'occupied'; } return answered; } From 30772859b099efa2833469bb179613cebb4089d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 16:14:46 +0100 Subject: [PATCH 05/17] chore(ios): name the journal's real selector and settle its review nits Review round 4 cleanups, none of them a behavior change. The admission doc names the selector the code has, accept(command:registeringJoin:), and so does ADR 0005. The COMMAND_ALREADY_FINISHED code comment now states the disclosure the daemon actually publishes, unknown. ADR 0005 defines terminal as finished execution rather than an effect on the device, says a pre-dispatch refusal closes its entry as failed while leaving the id reusable, says a runner restart starts a new journal, and scopes the 4096 bound to lost-response resends, since an id refused before dispatch never enters the window. The daemon comment keeps one clause for notAccepted, because the wire never distinguishes a still-marked pruned id. In tests: the busy-resend test drops a stray try and its read-only claim, the pruned-refusal test asserts its own prune precondition, the closed-id window test proves the valve releases exactly one id by still refusing the next-oldest, and the duplicated transport comment goes. The already-finished refusal now builds two whole messages rather than splicing a clause into another sentence. --- .../RunnerTests+CommandJournal.swift | 19 +++++++---- .../RunnerTests.swift | 5 +-- .../RunnerTests+CommandJournalTests.swift | 34 ++++++++++++++----- .../RunnerTests+TransportTests.swift | 3 -- .../0005-ios-runner-interaction-lifecycle.md | 6 ++-- .../apple-runner/src/runner-session-types.ts | 6 ++-- 6 files changed, 47 insertions(+), 26 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index b4550e4412..4228fa01aa 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -37,7 +37,7 @@ enum RunnerCommandAdmission { /// Nothing is running for this id and nothing has finished: the caller owns the one execution. case execute /// An execution is claimed and has not finished: the caller runs nothing and is answered by that - /// execution. `admit(command:registeringJoin:)` has already registered the send it gave this verdict. + /// execution. `accept(command:registeringJoin:)` has already registered the send it gave this verdict. case join /// Execution finished, and the work reached the device: the caller answers from the journal and /// never executes this command again. @@ -233,9 +233,9 @@ final class RunnerCommandJournal { closedCommandIds.insert(removable) closedOrder.append(removable) while closedOrder.count > Self.closedCommandIdSafetyValve { - // The safety valve is the one place a spent id returns to circulation: a resend more than 4096 - // completed commands later can re-execute, and the daemon resends only after a lost response - // within one request deadline. + // The safety valve is the one place a spent id returns to circulation: a resend more than + // 4096 completed dispatches later, within this runner process, can re-execute. A runner + // restart starts a new journal, which answers `notAccepted` for every id. closedCommandIds.remove(closedOrder.removeFirst()) } } @@ -278,12 +278,17 @@ final class RunnerCommandJournal { commandName: String, outcomeDropped: Bool ) -> Response { - Response( + // Two whole messages rather than one sentence spliced into another: which record is gone is the + // fact the caller acts on, so each variant has to read as a sentence on its own. + let reason = + outcomeDropped + ? "The runner already finished \(commandName) with id \(commandId). That command's recorded outcome has since been dropped from the journal, so there is no response to replay." + : "The runner already finished \(commandName) with id \(commandId) and kept no response to replay." + return Response( ok: false, error: ErrorPayload( code: RunnerTests.RunnerWireErrorCode.commandAlreadyFinished, - message: - "The runner already finished \(commandName) with id \(commandId) and kept no response to replay\(outcomeDropped ? ", because that record has since been dropped" : "").", + message: reason, hint: "Read the screen with snapshot before sending anything else: the finished command's effect is already on the device." ) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift index b99d68e6dd..6288557419 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -45,8 +45,9 @@ final class RunnerTests: XCTestCase { static let textInputProbeUnavailable = "TEXT_INPUT_PROBE_UNAVAILABLE" static let alertButtonNotFound = "ALERT_BUTTON_NOT_FOUND" /// Refuses a repeat of a `commandId` the runner already finished, when that command's own - /// classification keeps no response body to replay. Nothing ran for the repeat, so it discloses - /// dispatched: no, and its hint sends the caller to observe the screen instead of resending. + /// classification keeps no response body to replay. Nothing ran for the repeat, but the daemon's + /// dispatch disclosure stays `unknown` — only the runner knows whether an earlier send of that id + /// reached the device — and its hint sends the caller to observe the screen instead of resending. static let commandAlreadyFinished = "COMMAND_ALREADY_FINISHED" } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index dc74a4a3be..71da4d5a1e 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -307,8 +307,7 @@ extension RunnerTests { /// therefore exact, and these are both halves of it: a spent id is refused for the whole window, and the /// only id the valve releases is the oldest one, when a close fills the window past it. An id released /// there is one whose command completed more than `closedCommandIdSafetyValve` dispatched commands - /// earlier, which is the residual bound ADR 0005 states: the daemon resends only after a lost response, - /// within one request deadline, never that far behind. + /// earlier, within this runner process, which is the residual bound ADR 0005 states. func testJournalRefusesASpentIdAcrossTheClosedIdWindowAndReleasesOnlyTheOldest() throws { let journal = RunnerCommandJournal() let tap = runnerJournalCommand("tap", id: "spent-tap") @@ -331,9 +330,21 @@ extension RunnerTests { "the last close inside the window must still refuse the spent id" ) - // One more dispatched close fills the window, and the valve moves on by exactly one id. + // One more dispatched close fills the window, and the valve moves on by exactly one id. That id + // is checked first while asking about a neighbour cannot disturb it: admitting the released id + // below claims an entry, which pressures the window to evict one more. runnerJournalFinishOtherCommands(journal, 1, idPrefix: "valve", dispatched: true) + let nextOldest = runnerJournalCommand("readText", id: "window-0") + guard case .terminal(let stillRefused) = journal.accept(command: nextOldest) else { + return XCTFail("the valve must release the oldest closed id only, not the next-oldest") + } + XCTAssertEqual( + stillRefused.error?.code, + RunnerTests.RunnerWireErrorCode.commandAlreadyFinished, + "one close past the valve releases exactly one id" + ) + guard case .execute = journal.accept(command: tap) else { return XCTFail("the valve must release exactly the oldest closed id") } @@ -355,17 +366,24 @@ extension RunnerTests { ) runnerJournalFinishOtherCommands(journal, 64) + XCTAssertEqual( + journal.status(normalizedCommandId: "refused-before-dispatch").lifecycleState, + RunnerCommandLifecycleState.notAccepted.rawValue, + "the test must have pushed this refusal's own entry out of the window" + ) + guard case .execute = journal.accept(command: refused) else { return XCTFail("an id the runner refused before dispatch carries no effect and must stay executable") } } - /// The daemon resends a read-only command the runner refused with `RUNNER_BUSY`, under its busy - /// budget and with the same `commandId` (ADR 0005). A terminal record closes only ids whose execution - /// began, so a refusal that dispatched nothing has to leave the id executable once the runner is idle, - /// and must still travel as the journal failure the status recovery classifies as `RUNNER_BUSY`. + /// The journal closes only ids whose execution began, so a `RUNNER_BUSY` refusal that dispatched + /// nothing leaves the id executable once the runner is idle, and travels as the journal failure the + /// status recovery classifies as `RUNNER_BUSY`. Scoped to journal admission: the daemon's same-id + /// busy resend covers read-only commands (`index-busy-resend.test.ts`), and `recordStop` is here + /// for its observable no-op answer, not for its class. func testJournalAdmitsAgainACommandIdRefusedBeforeDispatch() throws { - let command = try runnerJournalCommand("recordStop", id: "busy-then-idle") + let command = runnerJournalCommand("recordStop", id: "busy-then-idle") abandonedMainThreadWorkCount = 1 abandonedMainThreadWorkSince = Date(timeIntervalSinceNow: -2) defer { diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index c62ef46766..aac61efb3e 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -612,9 +612,6 @@ extension RunnerTests { /// it answers "recording already stopped", so a re-execution shows up as the recorder this test installs /// being consumed instead of the recorded answer being replayed. func testTransportNeverRepeatsAFinishedCommandsExecution() throws { - // `recordStop` makes a second execution observable without recording anything: with no active - // recording it answers "recording already stopped", so a re-execution shows up as the recorder this - // test installs being consumed instead of the recorded answer being replayed. let listener = try startTransportListener() defer { listener.cancel() } let port = try XCTUnwrap(listener.port) diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index c192eb938c..40871e76de 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -121,10 +121,10 @@ Its verdict is `execute`, `join`, or `terminal`: - `execute` claims the `commandId`, and that send owns the one execution. - `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. -- `terminal` means execution finished with effect on the device. The runner answers with the outcome it recorded, and the command never runs again. +- `terminal` means execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` refusal, not at accept. -A refusal before that point dispatched nothing, so the entry stays open and the read-only busy resend above can still run under its own `commandId`. +A refusal before that point closed its entry as `failed` but dispatched nothing, so the `commandId` stays reusable and the read-only busy resend above can still run under it. Answering a closed `commandId` uses the retained response, then the retained error, then the typed `COMMAND_ALREADY_FINISHED` refusal for a command whose own classification retains no body. That refusal is a structured failure the daemon does not retry. Its dispatch disclosure stays `unknown`: the runner will not re-run the command, and only the runner knows whether an earlier send of that id reached the device. That refusal also reports nothing about the runner's main thread. The journal wrote it without executing, so the runner may be idle or mid-drain and the refusal cannot say which. The daemon's occupancy rule has three readings, occupied, drained, and this one, silent, and only the first two write `runnerMainThreadBusy`; a repeat refused while abandoned work drains must leave that report standing, because retention and handoff act on it. @@ -135,7 +135,7 @@ A resend inside that window is answered with the typed refusal; `status` still a An id the runner refused before dispatch is left free, which is what the read-only busy resend depends on. `status` answers `notAccepted` only for an id the journal does not hold. The daemon cannot read `notAccepted` as a safe terminal state, so evicting a running command would push it to invalidate a session whose runner is still executing that command. -The closed-id window holds 4096 ids, 64x the entry window, and it is a memory safety valve, not an admission boundary. Its residual bound, accepted here: within one runner process, a spent id is refused for the whole window, and the valve releases only the oldest id, so a resend arriving more than 4096 completed dispatches after a command can re-execute it. The daemon resends one `commandId` only after a lost response, within one request deadline, never that far behind. A journal that survives a runner restart is a new journal, and a fresh runner answers `notAccepted` for every id; a replay across a runner replacement is governed by charge settlement, not by this window. +The closed-id window holds 4096 ids, 64x the entry window, and it is a memory safety valve, not an admission boundary. Its residual bound, accepted here: within one runner process, a spent id is refused for the whole window, and the valve releases only the oldest id, so a resend arriving more than 4096 completed dispatches after a command can re-execute it. The resends this bound speaks of are lost-response resends of a dispatched command: a read-only `RUNNER_BUSY` resend reuses a `commandId` the runner refused before dispatch, and such an id never enters the window, so no chain of refusals can consume it. A runner restart starts a new journal, which answers `notAccepted` for every id; a replay across a runner replacement is governed by charge settlement, not by this window. A watermark over issued ids was rejected: the daemon mints `commandId` as a random UUID per request, reuses one id across retries and across runner generations, and persists nothing, so no ordering exists to compare. Eviction on a daemon resend deadline was rejected: the daemon holds no per-id "never resend after this instant" value — its resend budgets are attempt counts and per-exchange timeouts redrawn per attempt, and a restart-and-replay can legitimately post the same id minutes later — so a deadline-based rule would need a wire-contract change this change does not make. diff --git a/packages/apple-runner/src/runner-session-types.ts b/packages/apple-runner/src/runner-session-types.ts index b117ddf119..7b42300270 100644 --- a/packages/apple-runner/src/runner-session-types.ts +++ b/packages/apple-runner/src/runner-session-types.ts @@ -333,9 +333,9 @@ export class RunnerCommandAccounting { * * `completed` and `failed` close an entry — from the response's `ok` in `finish`, or a thrown error in * `fail` — so execution ended. `accepted` and `started` are written as execution opens, so they share - * one in-flight verdict. `notAccepted` is what `status` reports for an id the journal does not hold — - * one it never claimed, or one whose entry was pruned and whose closed-id record has since been released - * — which this daemon cannot read as terminal. + * one in-flight verdict. `notAccepted` is what `status` reports for an id the journal does not hold, + * whether that id was never claimed or its entry was pruned — the wire never draws that line, and + * this daemon cannot read either reading as terminal. */ const RUNNER_TERMINAL_LIFECYCLE_STATES: ReadonlySet = new Set(['completed', 'failed']); export const RUNNER_IN_FLIGHT_LIFECYCLE_STATES: ReadonlySet = new Set([ From 03ec2cc80024b86a62ebfd0fc0d74ddf48793af8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 17:57:03 +0100 Subject: [PATCH 06/17] fix(ios): answer a replayed command with what the live answer carried A repeat of a `commandId` the runner already finished is answered from the journal's record, so that answer has to carry what the answer the caller lost carried. Two things did not: a command that closed by throwing was recorded without its wire code, so a replayed `MAIN_THREAD_TIMEOUT` reached the daemon as an unclassified refusal and cleared the main-thread occupancy report at the moment the timed-out command was what occupied the thread; and a replayed `shutdown` answered without the flag that ends the runner's own test run, so the runner kept listening until disposal escalated to a signal. Both now derive from the same owner as the live answer: `RunnerTests.wireErrorCode(for:)` names the code the transport encodes and the journal records, and one `finishesRunner` flag decides the finish for every route a `shutdown` answer travels. The cross-language tie in `runner-swift-settlement-fixtures.ts` reads the Swift declaration so the daemon's occupancy verdict and the code the runner writes for it cannot drift apart silently. --- .../RunnerTests+CommandJournal.swift | 10 +- .../RunnerTests+Transport.swift | 28 +++--- .../RunnerTests.swift | 13 +++ .../RunnerTests+CommandJournalTests.swift | 35 +++++++ .../RunnerTests+TransportTests.swift | 96 +++++++++++++++++++ .../runner-error-classification.test.ts | 31 +++++- .../runner-swift-settlement-fixtures.ts | 61 ++++++++++++ .../src/runner-error-classification.ts | 4 +- 8 files changed, 261 insertions(+), 17 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 4228fa01aa..98d23119fd 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -137,13 +137,21 @@ final class RunnerCommandJournal { ) } + /// Records the failure that ended this command's execution. The typed wire code goes with it, the + /// same one the transport puts on the live reply for the same throw: a repeat of this `commandId` is + /// answered from this record, and a failure that arrives without its code reads to the daemon as an + /// ordinary served refusal — which for a `MAIN_THREAD_TIMEOUT` would report the main thread drained + /// when the command that just timed out is what occupied it. func fail(command: Command, error: Error) { update( command: command, state: .failed, responseOk: nil, responseJson: nil, - error: ErrorPayload(message: error.localizedDescription), + error: ErrorPayload( + code: RunnerTests.wireErrorCode(for: error), + message: error.localizedDescription + ), marksDispatched: false ) } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index c927e60fb9..5c6191def6 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -212,6 +212,11 @@ extension RunnerTests { let admission = commandJournal.accept(command: command) { [weak self] commandId in self?.registerJoiner(for: commandId, completion: completion) } + // A `shutdown` answer is what ends the runner's own XCTest run, whoever it is delivered to: a + // send that joined the execution receives this same flag through `deliverCommandResult`, and a + // send the journal answers receives it here. `finish()` guards its own expectation against being + // fulfilled twice, so more than one send asking to finish is harmless. + let finishesRunner = command.command == .shutdown switch admission { case .join: // Re-sends of a still-executing commandId (the daemon's transport retry loop) were registered @@ -228,7 +233,7 @@ extension RunnerTests { command.command.rawValue, command.commandId ?? "" ) - completion((jsonResponse(status: 200, response: outcome), false)) + completion((jsonResponse(status: 200, response: outcome), finishesRunner)) case .execute: NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ACCEPTED command=%@ commandId=%@", @@ -246,7 +251,7 @@ extension RunnerTests { ) self.deliverCommandResult( command: command, - result: (self.jsonResponse(status: 200, response: response), command.command == .shutdown), + result: (self.jsonResponse(status: 200, response: response), finishesRunner), completion: completion ) case .failure(let error): @@ -370,27 +375,24 @@ extension RunnerTests { Response(ok: false, error: ErrorPayload(code: code, message: message, hint: hint)) } - /// Turns a thrown command error into its wire response. The execution-watchdog timeout keeps its - /// own typed code so the daemon records the runner as main-thread-occupied from the stalling - /// command itself, not only from a later `RUNNER_BUSY` refusal (#2552); every other throw stays the - /// generic `COMMAND_FAILED`. + /// Turns a thrown command error into its wire response, under the one code mapping the journal's + /// failure record uses, so the answer a resend reads from the journal cannot disagree with the answer + /// the caller lost. func commandFailedResponse(from error: Error) -> Response { - let nsError = error as NSError - if nsError.domain == RunnerErrorDomain.general, - nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut - { + let code = RunnerTests.wireErrorCode(for: error) + if code == RunnerWireErrorCode.mainThreadTimeout { return Response( ok: false, error: ErrorPayload( - code: RunnerWireErrorCode.mainThreadTimeout, - message: nsError.localizedDescription, + code: code, + message: error.localizedDescription, hint: "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." ) ) } return errorResponse( - code: "COMMAND_FAILED", + code: code, message: error.localizedDescription, hint: "Check the runner log for XCTest details, then retry after the app is foregrounded if this was a timeout or activation failure." ) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift index 6288557419..ef2e48bbbd 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -51,6 +51,19 @@ final class RunnerTests: XCTestCase { static let commandAlreadyFinished = "COMMAND_ALREADY_FINISHED" } + /// The wire code a thrown command error travels under: the execution-watchdog timeout keeps its own + /// typed code so the daemon records the runner as main-thread-occupied from the stalling command + /// itself (#2552), and every other throw stays the generic `COMMAND_FAILED`. The transport encodes a + /// live failure with this, and the journal records a thrown failure with it, so a repeat answered from + /// the journal's record carries the same code as the answer the caller lost. + static func wireErrorCode(for error: Error) -> String { + let nsError = error as NSError + return nsError.domain == RunnerErrorDomain.general + && nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut + ? RunnerWireErrorCode.mainThreadTimeout + : "COMMAND_FAILED" + } + static let springboardBundleId = "com.apple.springboard" // SpringBoard hosts blocking system modals on iOS/visionOS; tvOS (PineBoard/HeadBoard) // and macOS have no such host, so there is nothing to probe there. diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 71da4d5a1e..002b185020 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -406,6 +406,41 @@ extension RunnerTests { XCTAssertEqual(served.data?.message, "recording already stopped") } + /// The recorded failure has to carry the same code the live answer carried, because the daemon reads + /// that code to decide whether the runner's main thread is still draining. A repeat of a command that + /// timed out on the execution watchdog is answered from this record, and a failure stored without its + /// code reaches the daemon as an ordinary served refusal: it would report the thread drained at the + /// exact moment the timed-out command occupied it. + func testJournalReplayOfAThrownFailureCarriesItsWireCode() throws { + let journal = RunnerCommandJournal() + let command = runnerJournalCommand("readText", id: "timed-out-then-repeated") + journal.accept(command: command) + journal.markDispatched(command: command) + journal.fail(command: command, error: Self.mainThreadExecutionTimeoutError()) + // Nothing of a timed-out command is retained to replay, so the recorded failure is the only answer + // a repeat can be given. + XCTAssertEqual( + journal.status(normalizedCommandId: "timed-out-then-repeated").lifecycleResponseJson, + nil + ) + + guard case .terminal(let replay) = journal.accept(command: command) else { + return XCTFail("a repeat of a finished, dispatched commandId is answered from the journal") + } + + XCTAssertEqual(replay.ok, false) + XCTAssertEqual( + replay.error?.code, + RunnerWireErrorCode.mainThreadTimeout, + "a replayed failure must carry the code the live answer carried, got \(String(describing: replay.error?.code))" + ) + XCTAssertEqual( + journal.status(normalizedCommandId: "timed-out-then-repeated").lifecycleErrorCode, + RunnerWireErrorCode.mainThreadTimeout, + "`status` recovery reads the same record and must see the code too" + ) + } + func testCommandJournalKeepsErrorMetadataWhenResponseJsonIsDropped() { let journal = RunnerCommandJournal() let snapshot = runnerJournalCommand("snapshot", id: "snapshot-error") diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index aac61efb3e..4a9bdc8412 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -676,6 +676,102 @@ extension RunnerTests { ) } + /// A command that closed through the throw path must be answered from the journal with the same code + /// the live answer carried. The daemon reads that code to decide whether the runner's main thread is + /// still draining the work this command abandoned: a replayed failure that arrives without it reads + /// as an ordinary served refusal, which reports the thread drained at the exact moment the timed-out + /// command occupies it. + /// + /// The alert route is the throw path this lane can drive on its own deadline: the resolution outlives + /// the 500 ms request, so the bounded dispatch throws the watchdog timeout, `executeAccepted` records + /// it, and the repeat is answered from that record. + /// + /// Repeats go through `execute(command:)`, which routes a send the way the transport does, so this + /// proves the wire path and not a helper's. + #if os(iOS) + @MainActor + func testTransportReplaysAThrownFailureWithItsWireCode() throws { + let resolutionExited = expectation(description: "blocked alert resolution exited") + let releaseResolution = DispatchSemaphore(value: 0) + mainOwned.app = springboard + mainOwned.bundleId = Self.springboardBundleId + alertResolutionOverrideForTesting = { _ in + _ = releaseResolution.wait(timeout: .now() + 2) + resolutionExited.fulfill() + return nil + } + defer { + releaseResolution.signal() + alertResolutionOverrideForTesting = nil + mainOwned.app = nil + mainOwned.bundleId = nil + } + let payload = + #"{"command":"alert","commandId":"alert-timed-out","appBundleId":"com.apple.springboard","action":"get","timeoutMs":500}"# + let command = try JSONDecoder().decode(Command.self, from: Data(payload.utf8)) + + XCTAssertThrowsError(try execute(command: command)) { error in + XCTAssertEqual((error as NSError).code, RunnerErrorCode.mainThreadExecutionTimedOut) + } + releaseResolution.signal() + wait(for: [resolutionExited], timeout: 2) + let drainDeadline = Date().addingTimeInterval(2) + while hasAbandonedMainThreadWork(), Date() < drainDeadline { + sleepFor(0.005) + } + + let replay = try execute(command: command) + + XCTAssertEqual(replay.ok, false) + XCTAssertEqual( + replay.error?.code, + RunnerWireErrorCode.mainThreadTimeout, + "a replayed failure must carry the code the live answer carried, got \(String(describing: replay.error?.code))" + ) + } + #endif + + /// A `shutdown` answer is what ends the runner's own XCTest run, so the send that receives it has to be + /// the one that ends the runner — including the send the journal answers. A resend whose first answer + /// the daemon lost is exactly the case the journal exists to serve: if that reply is not allowed to + /// finish, the runner keeps listening with no caller left to stop it, and disposal has to wait out its + /// timeout and escalate to a signal. + func testTransportFinishesTheRunnerOnAReplayOfAFinishedShutdown() throws { + let command = try transportCommand("shutdown", id: "shutdown-answered-twice") + // The state a resend finds: this shutdown ran, was marked dispatched, and closed with its answer. + commandJournal.accept(command: command) + commandJournal.markDispatched(command: command) + commandJournal.finish( + command: command, + response: Response(ok: true, data: DataPayload(message: "shutdown")) + ) + + let listener = try startTransportListener() + defer { listener.cancel() } + let port = try XCTUnwrap(listener.port) + let client = URLSession(configuration: .ephemeral) + defer { client.invalidateAndCancel() } + let finished = expectation(description: "the replayed shutdown asked the runner to finish") + // Cleared on the way out so a run that never finished cannot leak the expectation into the next + // test; the delivery that fulfils it clears it first. + defer { doneExpectation = nil } + doneExpectation = finished + + let reply = try transportHttpResponse( + client: client, + port: port, + payload: #"{"command":"shutdown","commandId":"shutdown-answered-twice"}"# + ) + + XCTAssertEqual(reply.ok, true) + XCTAssertEqual( + reply.data?.message, + "shutdown", + "the repeat is answered from the journal rather than executed again" + ) + wait(for: [finished], timeout: 2) + } + /// Routes `command` the way the transport routes it: an inline read, then the journal's verdict on its /// `commandId`. A finished id comes back as the answer the runner recorded for it; the execute path /// rethrows what the dispatch threw, which the wire path turns into its own response. Sends that need diff --git a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts index 33760651d2..8ead048356 100644 --- a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts +++ b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts @@ -3,6 +3,7 @@ import { test } from 'vitest'; import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { COMMAND_ALREADY_FINISHED_RUNNER_CODE, + MAIN_THREAD_TIMEOUT_RUNNER_CODE, NO_TEXT_INPUT_AT_POINT_RUNNER_CODE, RUNNER_SCREEN_CAPTURE_REFUSAL_RUNNER_CODES, TEXT_INPUT_PROBE_UNAVAILABLE_RUNNER_CODE, @@ -21,6 +22,7 @@ import { shouldRetryRunnerConnectError, } from '../runner-error-classification.ts'; import { runnerConnectFailure, unwrittenConnectRefusal } from './runner-session-fixtures.ts'; +import { requireSwiftThrownErrorWireCode } from './runner-swift-settlement-fixtures.ts'; const TAP = { command: 'tap' } as const; const SNAPSHOT = { command: 'snapshot' } as const; @@ -471,8 +473,8 @@ function runnerReplyFailure(runnerCode: string): AppError { }); } -// The literal is what the Swift runner encodes, so it is the contract and not the constant name: -// a rename on one side has to fail here rather than silently split the pair. +// Pins the daemon-side literal, in the repo's style for these codes. The cross-language tie — that the +// Swift runner really encodes this string — is the test below, which reads the Swift declaration. test('the journal refusal of a finished commandId reports nothing about main-thread occupancy', () => { assert.equal(COMMAND_ALREADY_FINISHED_RUNNER_CODE, 'COMMAND_ALREADY_FINISHED'); assert.equal( @@ -492,6 +494,31 @@ test('occupancy reads the busy refusal and the stalling timeout as occupied, and assert.equal(runnerMainThreadOccupancyReport(runnerReplyFailure('ELEMENT_NOT_FOUND')), 'drained'); }); +/** + * The occupancy verdict and the code the runner writes for it are one claim across two languages, so + * the Swift declaration is read here rather than restated: renaming + * `RunnerTests.RunnerWireErrorCode.mainThreadTimeout` splits the pair and fails this test. The same + * read ties the journal's failure record to that code. A repeat of a command that closed by throwing is + * answered from that record, and a record without the code reaches the daemon as an ordinary served + * refusal, which would report the main thread drained while the command that just timed out is what + * occupies it. + */ +test('the runner writes the occupancy code the daemon reads, on the live reply and the journal record', () => { + const swift = requireSwiftThrownErrorWireCode(); + + assert.equal( + swift.timeoutCode, + MAIN_THREAD_TIMEOUT_RUNNER_CODE, + 'the runner must encode the watchdog timeout under the code the daemon keys occupancy on', + ); + assert.equal( + swift.journalRecordsDerivedCode, + true, + 'the journal must record a thrown failure under the same code the transport replies with, so a ' + + 'replayed failure reports the same runner condition as the answer the caller lost', + ); +}); + // --- runner that predates the listener token --- test('a runner that does not require the token is session-fatal and never retried or replayed', () => { diff --git a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts index 05f2e91730..53ed4342cd 100644 --- a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts +++ b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts @@ -19,6 +19,10 @@ const SWIFT_JOURNAL = path.resolve( here, '../../../../apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift', ); +const SWIFT_RUNNER = path.resolve( + here, + '../../../../apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift', +); /** `inlineResponse(for:)`'s switch arms, up to the `default:` where queued commands begin. */ const INLINE_SWITCH = @@ -125,3 +129,60 @@ export function requireLifecycleSettlementRows( settlesCharge: expectations[lifecycleState]!, })); } + +/** The string values `RunnerWireErrorCode` declares, keyed by the Swift member name. */ +function readSwiftWireErrorCodes(): Record { + const swift = fs.readFileSync(SWIFT_RUNNER, 'utf8'); + const declaration = swift.match(/enum RunnerWireErrorCode \{([\s\S]*?)\n {2}\}/); + const body = declaration?.[1]; + assert.ok( + body !== undefined, + 'RunnerTests.swift must declare RunnerWireErrorCode with a body. The daemon keys behavior on ' + + 'these codes, so a runner that stops naming them in one place breaks the tie here first.', + ); + const codes: Record = {}; + for (const [, member, value] of body.matchAll(/static let (\w+) = "([A-Z0-9_]+)"/g)) { + codes[member!] = value!; + } + assert.ok( + Object.keys(codes).length > 0, + 'RunnerWireErrorCode declares no `static let NAME = "CODE"` row this reader can parse', + ); + return codes; +} + +/** + * The wire code the runner derives for one thrown error. Both readers of a failed command — the + * transport encoding a live reply, and the journal recording a thrown one so a repeat can be answered + * from it — must use this one declaration, or the answer a resend receives reports a different runner + * condition than the answer the caller lost. The daemon keys main-thread occupancy on the timeout code, + * so the pair this ties is genuinely cross-language: it reads the Swift side rather than a second copy + * of the same string in a TypeScript test. + */ +export function requireSwiftThrownErrorWireCode(): { + /** The Swift member whose value the daemon's occupancy verdict keys on. */ + timeoutCode: string; + /** Whether the journal's failure record derives its code from the same declaration. */ + journalRecordsDerivedCode: boolean; +} { + const codes = readSwiftWireErrorCodes(); + const timeoutCode = codes['mainThreadTimeout']; + assert.ok( + timeoutCode !== undefined, + 'RunnerWireErrorCode must declare mainThreadTimeout: the daemon reads this code to record the ' + + "runner's main thread as still draining, on the live reply and on a journal replay alike.", + ); + const journal = fs.readFileSync(SWIFT_JOURNAL, 'utf8'); + const recorded = journal.match( + /func fail\(command: Command, error: Error\)[\s\S]*?\n {2}\}/, + )?.[0]; + assert.ok( + recorded !== undefined, + 'RunnerCommandJournal must declare fail(command:error:) — the record a repeated commandId is ' + + 'answered from when the command closed by throwing.', + ); + return { + timeoutCode, + journalRecordsDerivedCode: /RunnerTests\.wireErrorCode\(for: error\)/.test(recorded), + }; +} diff --git a/packages/apple-runner/src/runner-error-classification.ts b/packages/apple-runner/src/runner-error-classification.ts index bbec27351d..a063b63b05 100644 --- a/packages/apple-runner/src/runner-error-classification.ts +++ b/packages/apple-runner/src/runner-error-classification.ts @@ -682,7 +682,9 @@ export function runnerMainThreadOccupancyReport(error: unknown): RunnerMainThrea if (isRunnerBusyError(error) || isRunnerMainThreadTimeoutError(error)) return 'occupied'; // A transport-shaped failure never reached the runner, so it reports nothing. if (!isStructuredRunnerFailure(error)) return 'silent'; - // The journal answered from its own record, so no execution observed the main thread. + // The journal answered from its own record, so no execution observed the main thread. This is the + // code the journal stores for a command whose own classification keeps no body; a recorded failure + // carries its real code and is read as the occupancy that code reports. if (error.details?.runnerErrorCode === COMMAND_ALREADY_FINISHED_RUNNER_CODE) return 'silent'; return 'drained'; } From 5a0d20cf9f47dbfe67015b7279deb2821c4418d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 19:10:46 +0100 Subject: [PATCH 07/17] fix(ios): let the journal decide when a replayed answer ends the runner `terminal` answers a repeat from the record the journal holds, so which answer ends the runner is that record's fact. Deriving it from the repeat's own request let a caller end the runner on some other command's answer: a spent `commandId` reused under the name `shutdown` received that end, while a recorded `shutdown` answer named as another command withheld it. The verdict now carries `answersShutdown`, read from the command the entry names, and ADR 0005 states the ownership. --- .../RunnerTests+CommandJournal.swift | 27 +++++++-- .../RunnerTests+Transport.swift | 15 +++-- .../RunnerTests+CommandJournalTests.swift | 58 +++++++++++++++++-- .../RunnerTests+TransportTests.swift | 44 +++++++++++++- .../0005-ios-runner-interaction-lifecycle.md | 2 +- 5 files changed, 126 insertions(+), 20 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 98d23119fd..bdf5056bc9 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -40,8 +40,11 @@ enum RunnerCommandAdmission { /// execution. `accept(command:registeringJoin:)` has already registered the send it gave this verdict. case join /// Execution finished, and the work reached the device: the caller answers from the journal and - /// never executes this command again. - case terminal(outcome: Response) + /// never executes this command again. `answersShutdown` says whether the answer being handed over is + /// the runner's own `shutdown` answer, which the caller owes the runner's end. The repeat's request + /// cannot answer that: a caller may reuse a spent `commandId` under another command name, and the + /// answer it receives stays the recorded command's. + case terminal(outcome: Response, answersShutdown: Bool) } /// The runner's one command-lifecycle record: what it holds for a `commandId`, whether that id may run, @@ -84,9 +87,15 @@ final class RunnerCommandJournal { defer { lock.unlock() } guard let entry = entries[commandId] else { // The entry can be pruned; the fact it recorded is not, so an id that spent itself on the device - // never executes again. + // never executes again. Only the entry named the command, so this refusal can name only what the + // repeat asked for. if closedCommandIds.contains(commandId) { - return .terminal(outcome: alreadyFinishedRefusal(command: command, commandId: commandId, outcomeDropped: true)) + // The record that named this id is gone, so all this refusal can promise is that the id is + // spent — it is no command's answer, and so never the runner's own. + return .terminal( + outcome: alreadyFinishedRefusal(command: command, commandId: commandId, outcomeDropped: true), + answersShutdown: false + ) } return claim(command: command, commandId: commandId) } @@ -99,7 +108,10 @@ final class RunnerCommandJournal { guard entry.dispatched else { return claim(command: command, commandId: commandId) } - return .terminal(outcome: recordedOutcome(for: entry)) + return .terminal( + outcome: recordedOutcome(for: entry), + answersShutdown: entry.command == CommandType.shutdown.rawValue + ) } func start(command: Command) { @@ -238,6 +250,11 @@ final class RunnerCommandJournal { // Only work that reached the device spends its id. A pruned refusal leaves the id free, which is // what the busy resend of the same `commandId` depends on (ADR 0005). if entries[removable]?.dispatched == true { + // The two closed-id structures stay one-to-one, and nothing here needs a duplicate check to keep + // them that way: an id enters `closedCommandIds` only in this branch, and an id whose entry this + // journal no longer holds can be claimed again only once the marker is gone — which the valve + // below does by removing the id from both structures together. So a second close of a released id + // finds an id in neither structure, and one still marked can never have an entry to prune. closedCommandIds.insert(removable) closedOrder.append(removable) while closedOrder.count > Self.closedCommandIdSafetyValve { diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index 5c6191def6..e188358e60 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -212,11 +212,6 @@ extension RunnerTests { let admission = commandJournal.accept(command: command) { [weak self] commandId in self?.registerJoiner(for: commandId, completion: completion) } - // A `shutdown` answer is what ends the runner's own XCTest run, whoever it is delivered to: a - // send that joined the execution receives this same flag through `deliverCommandResult`, and a - // send the journal answers receives it here. `finish()` guards its own expectation against being - // fulfilled twice, so more than one send asking to finish is harmless. - let finishesRunner = command.command == .shutdown switch admission { case .join: // Re-sends of a still-executing commandId (the daemon's transport retry loop) were registered @@ -227,13 +222,17 @@ extension RunnerTests { command.command.rawValue, command.commandId ?? "" ) - case .terminal(let outcome): + case .terminal(let outcome, let answersShutdown): + // Which answer ends the runner is the journal's fact, not this request's: a caller may reuse a + // spent `commandId` under another command name, and what it receives is still the recorded + // command's outcome. `finish()` guards its own expectation against being fulfilled twice, so a + // shutdown that several sends receive is harmless. NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ALREADY_FINISHED command=%@ commandId=%@", command.command.rawValue, command.commandId ?? "" ) - completion((jsonResponse(status: 200, response: outcome), finishesRunner)) + completion((jsonResponse(status: 200, response: outcome), answersShutdown)) case .execute: NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ACCEPTED command=%@ commandId=%@", @@ -251,7 +250,7 @@ extension RunnerTests { ) self.deliverCommandResult( command: command, - result: (self.jsonResponse(status: 200, response: response), finishesRunner), + result: (self.jsonResponse(status: 200, response: response), command.command == .shutdown), completion: completion ) case .failure(let error): diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 002b185020..bb96857c28 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -291,7 +291,7 @@ extension RunnerTests { runnerJournalCloseDispatchedCommand(journal, tap, message: "tapped") runnerJournalFinishOtherCommands(journal, 64) - guard case .terminal(let outcome) = journal.accept(command: tap) else { + guard case .terminal(let outcome, _) = journal.accept(command: tap) else { return XCTFail("a pruned id whose work reached the device must never be admitted again") } XCTAssertFalse(outcome.ok) @@ -321,7 +321,7 @@ extension RunnerTests { idPrefix: "window", dispatched: true ) - guard case .terminal(let refusal) = journal.accept(command: tap) else { + guard case .terminal(let refusal, _) = journal.accept(command: tap) else { return XCTFail("a spent id must stay refused for the whole closed-id window") } XCTAssertEqual( @@ -336,7 +336,7 @@ extension RunnerTests { runnerJournalFinishOtherCommands(journal, 1, idPrefix: "valve", dispatched: true) let nextOldest = runnerJournalCommand("readText", id: "window-0") - guard case .terminal(let stillRefused) = journal.accept(command: nextOldest) else { + guard case .terminal(let stillRefused, _) = journal.accept(command: nextOldest) else { return XCTFail("the valve must release the oldest closed id only, not the next-oldest") } XCTAssertEqual( @@ -406,6 +406,56 @@ extension RunnerTests { XCTAssertEqual(served.data?.message, "recording already stopped") } + /// Whether `accept` reports that the answer it hands over is the runner's own `shutdown` answer. + private func journalAnswersShutdown( + _ journal: RunnerCommandJournal, + command: Command + ) -> Bool { + switch journal.accept(command: command) { + case .terminal(_, let answersShutdown): + return answersShutdown + case .execute: + XCTFail("a finished, dispatched commandId must be answered from the journal") + return false + case .join: + XCTFail("this test leaves no execution in flight") + return false + } + } + + /// Which answer ends the runner is the journal's fact about the record, not the repeat's: a caller may + /// reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded + /// command's. Naming `shutdown` in the repeat must not end the runner on some other command's answer, + /// and naming another command must not withhold the end that a recorded `shutdown` answer owes. + func testJournalDecidesFromTheRecordWhetherAReplayEndsTheRunner() throws { + let journal = RunnerCommandJournal() + let tap = runnerJournalCommand("tap", id: "spent-then-named-shutdown") + journal.accept(command: tap) + journal.markDispatched(command: tap) + journal.finish(command: tap, response: Response(ok: true, data: DataPayload(message: "tapped"))) + + let shutdown = runnerJournalCommand("shutdown", id: "spent-then-named-shutdown") + XCTAssertEqual( + journalAnswersShutdown(journal, command: shutdown), + false, + "a recorded tap answer must not end the runner because the repeat named shutdown" + ) + + let shutdownId = "shutdown-then-named-tap" + let realShutdown = runnerJournalCommand("shutdown", id: shutdownId) + journal.accept(command: realShutdown) + journal.markDispatched(command: realShutdown) + journal.finish( + command: realShutdown, + response: Response(ok: true, data: DataPayload(message: "shutdown")) + ) + XCTAssertEqual( + journalAnswersShutdown(journal, command: runnerJournalCommand("tap", id: shutdownId)), + true, + "a recorded shutdown answer must still end the runner when the repeat names another command" + ) + } + /// The recorded failure has to carry the same code the live answer carried, because the daemon reads /// that code to decide whether the runner's main thread is still draining. A repeat of a command that /// timed out on the execution watchdog is answered from this record, and a failure stored without its @@ -424,7 +474,7 @@ extension RunnerTests { nil ) - guard case .terminal(let replay) = journal.accept(command: command) else { + guard case .terminal(let replay, _) = journal.accept(command: command) else { return XCTFail("a repeat of a finished, dispatched commandId is answered from the journal") } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index 4a9bdc8412..51317139ad 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -532,7 +532,7 @@ extension RunnerTests { let lateVerdict = commandJournal.accept(command: command) { _ in registeredLateSend = true } - guard case .terminal(let outcome) = lateVerdict else { + guard case .terminal(let outcome, _) = lateVerdict else { return XCTFail("a send of a finished commandId must be answered from the journal") } XCTAssertFalse(registeredLateSend, "a send answered from the journal must not be registered") @@ -772,6 +772,46 @@ extension RunnerTests { wait(for: [finished], timeout: 2) } + /// The mirror of the case above, and the reason the finish flag is the journal's fact: a caller that + /// reuses a spent `commandId` under the name `shutdown` receives the recorded answer of whatever that + /// id really ran. Ending the runner there would stop it on some other command's answer, so the resend + /// is answered and the runner keeps serving. + func testTransportDoesNotFinishOnAReplayOfAnotherCommandsRecordedAnswer() throws { + let spent = try transportCommand("readText", id: "spent-then-named-shutdown") + commandJournal.accept(command: spent) + commandJournal.markDispatched(command: spent) + commandJournal.finish( + command: spent, + response: Response(ok: true, data: DataPayload(message: "read")) + ) + + let listener = try startTransportListener() + defer { listener.cancel() } + let port = try XCTUnwrap(listener.port) + let client = URLSession(configuration: .ephemeral) + defer { client.invalidateAndCancel() } + // Fulfilled only by a delivery that wrongly asks the runner to finish, which is what this test + // rules out. `wait` with no expectation would not catch a finish that happens late. + let wronglyFinished = expectation(description: "a replay must not finish the runner") + wronglyFinished.isInverted = true + doneExpectation = wronglyFinished + defer { doneExpectation = nil } + + let reply = try transportHttpResponse( + client: client, + port: port, + payload: #"{"command":"shutdown","commandId":"spent-then-named-shutdown"}"# + ) + + XCTAssertEqual(reply.ok, true) + XCTAssertEqual( + reply.data?.message, + "read", + "the id's recorded answer is served, not a shutdown answer" + ) + wait(for: [wronglyFinished], timeout: 1) + } + /// Routes `command` the way the transport routes it: an inline read, then the journal's verdict on its /// `commandId`. A finished id comes back as the answer the runner recorded for it; the execute path /// rethrows what the dispatch threw, which the wire path turns into its own response. Sends that need @@ -783,7 +823,7 @@ extension RunnerTests { return response } switch commandJournal.accept(command: command) { - case .terminal(let outcome): + case .terminal(let outcome, _): return outcome case .join: throw NSError( diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index 40871e76de..f27cc83053 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -121,7 +121,7 @@ Its verdict is `execute`, `join`, or `terminal`: - `execute` claims the `commandId`, and that send owns the one execution. - `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. -- `terminal` means execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. +- `terminal` means execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. Whether that answer ends the runner is the journal's fact about the record it read, not the repeat's request: a caller may reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded command's. The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` refusal, not at accept. A refusal before that point closed its entry as `failed` but dispatched nothing, so the `commandId` stays reusable and the read-only busy resend above can still run under it. From b64801450a82745b36ee4ea3ba370bd68364a450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 20:05:43 +0100 Subject: [PATCH 08/17] fix(ios): withhold the runner's end from a replayed shutdown refusal The live path finishes the runner only on a `shutdown` that succeeded, so the journal's replay has to read the outcome before deciding. `terminal` now gates `answersShutdown` on the recorded answer being `ok`: replaying a refused `shutdown` kept asking the runner to finish, which would end a runner whose caller was still being told it stayed up. States the two ownership facts this round's review turned up, and neither needs a guard: only the `execute` verdict can reach `markDispatched`, so no busy refusal can follow the mark for the command it refuses; and the request's `close` is what ends the deadline bookkeeping, so the journal answer is the only writer for that connection. ADR 0005 records what `notAccepted` costs a lost charge. --- .../RunnerTests+CommandDispatch.swift | 5 +++-- .../RunnerTests+CommandJournal.swift | 14 +++++++----- .../RunnerTests+Transport.swift | 3 +++ .../RunnerTests+CommandJournalTests.swift | 22 +++++++++++++++++++ .../0005-ios-runner-interaction-lifecycle.md | 1 + 5 files changed, 38 insertions(+), 7 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift index ee76ba52af..8e3cd853c2 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift @@ -190,8 +190,9 @@ extension RunnerTests { return unavailable } // Past the last fast refusal, this command's work is going to the device, so its `commandId` is - // spent: a repeat of it is answered from the journal instead of running the device again. The - // read-only busy resend arrives before this point, never after it (ADR 0005). + // spent: a repeat of it is answered from the journal instead of running the device again. Only the + // `.execute` verdict reaches this line, so a repeat of a spent id is refused at the transport and no + // busy refusal here can follow the mark for the command it refuses (ADR 0005). commandJournal.markDispatched(command: command) if command.command == .snapshot { return try executeSnapshotDispatched(command: command) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index bdf5056bc9..3367c788be 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -41,9 +41,9 @@ enum RunnerCommandAdmission { case join /// Execution finished, and the work reached the device: the caller answers from the journal and /// never executes this command again. `answersShutdown` says whether the answer being handed over is - /// the runner's own `shutdown` answer, which the caller owes the runner's end. The repeat's request - /// cannot answer that: a caller may reuse a spent `commandId` under another command name, and the - /// answer it receives stays the recorded command's. + /// the runner's own successful `shutdown` answer, which the caller owes the runner's end. Neither the + /// repeat's request nor the command name alone can answer that: a caller may reuse a spent `commandId` + /// under another command name, and the live path withholds the finish from a `shutdown` that threw. case terminal(outcome: Response, answersShutdown: Bool) } @@ -108,9 +108,13 @@ final class RunnerCommandJournal { guard entry.dispatched else { return claim(command: command, commandId: commandId) } + let outcome = recordedOutcome(for: entry) + // Only a recorded `shutdown` that succeeded ends the runner. The live path withholds that finish from + // a `shutdown` that threw, so replaying its refusal must not stop a runner whose caller is still + // holding it for another command. return .terminal( - outcome: recordedOutcome(for: entry), - answersShutdown: entry.command == CommandType.shutdown.rawValue + outcome: outcome, + answersShutdown: entry.command == CommandType.shutdown.rawValue && outcome.ok ) } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index e188358e60..be0469aa9f 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -94,6 +94,9 @@ extension RunnerTests { receiveRequest(inbound, over: connection) return } + // Closing here ends the request's deadline bookkeeping: the armed work item refuses a closed inbound, + // so from this point the completion below is the only writer for this connection, whichever admission + // verdict the journal gives. inbound.close() let body = inbound.buffer.subdata(in: head.bodyOffset.. Date: Sun, 1 Nov 2026 21:27:50 +0100 Subject: [PATCH 09/17] fix(ios): keep a delivery in the generation its execution claimed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `commandId` the runner refuses before dispatch is claimed again, and the transport keyed its joiners by the id, so the delivery of that refusal could answer the sends that joined the execution which followed it — one execution's answer handed to another execution's callers. The journal now mints an identity per claim and hands it with the verdict, and the transport keys waiters by it. `markDispatched` sets only its own mark, so the retention order keeps recording claim order, and both it and the state transitions go through one locked mutation. The main-thread-timeout hint keys on the error the watchdog throws rather than on the code derived from it. ADR 0005 states the per-claim identity, and the classification test pins the published shape of `COMMAND_ALREADY_FINISHED` the way its siblings are pinned. --- .../RunnerTests+CommandJournal.swift | 136 +++++++++--------- .../RunnerTests+Transport.swift | 44 +++--- .../RunnerTests.swift | 10 +- .../RunnerTests+TransportTests.swift | 79 ++++++++-- .../0005-ios-runner-interaction-lifecycle.md | 2 +- .../runner-error-classification.test.ts | 19 ++- 6 files changed, 188 insertions(+), 102 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 3367c788be..415e4004bb 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -30,15 +30,23 @@ struct RunnerCommandJournalEntry { /// on the device and its terminal entry must not close the id: the read-only busy resend carries the /// same `commandId` (ADR 0005). var dispatched = false + /// Which execution of this `commandId` the record belongs to, minted when the id was claimed. The + /// transport answers joiners from it, so a delivery reaches only the sends that joined the execution that + /// closed. It has to outlive the entry's close: an id the runner refused before dispatch is claimed + /// again, and the sends of that new execution must not be answered by the delivery of the refusal that + /// freed the id. + let executionToken: Int } /// The journal's verdict on one command that carries a `commandId`. enum RunnerCommandAdmission { - /// Nothing is running for this id and nothing has finished: the caller owns the one execution. - case execute + /// Nothing is running for this id and nothing has finished: the caller owns the one execution, + /// identified by `executionToken` for as long as that claim answers for it. + case execute(executionToken: Int) /// An execution is claimed and has not finished: the caller runs nothing and is answered by that - /// execution. `accept(command:registeringJoin:)` has already registered the send it gave this verdict. - case join + /// execution, the one `executionToken` names. `accept(command:registeringJoin:)` has already + /// registered the send it gave this verdict. + case join(executionToken: Int) /// Execution finished, and the work reached the device: the caller answers from the journal and /// never executes this command again. `answersShutdown` says whether the answer being handed over is /// the runner's own successful `shutdown` answer, which the caller owes the runner's end. Neither the @@ -69,6 +77,9 @@ final class RunnerCommandJournal { /// valve above: an id older than that can be admitted again, and `pruneIfNeeded` states that bound. private var closedCommandIds: Set = [] private var closedOrder: [String] = [] + /// Mints the identity of each claim, so a delivery can tell the sends that joined its own execution from + /// the sends that joined a later one under the same `commandId`. + private var nextExecutionToken = 0 /// Settles one send, and when the answer is to join a running execution, registers that send for the /// answer inside the same locked operation. No transition can close the entry between the two, which is @@ -80,11 +91,14 @@ final class RunnerCommandJournal { @discardableResult func accept( command: Command, - registeringJoin register: (String) -> Void = { _ in } + registeringJoin register: (Int) -> Void = { _ in } ) -> RunnerCommandAdmission { - guard let commandId = command.commandId?.trimmedNonEmpty else { return .execute } lock.lock() defer { lock.unlock() } + guard let commandId = command.commandId?.trimmedNonEmpty else { + // No `commandId`, so nothing to settle and no other send can share this claim. + return .execute(executionToken: mintExecutionToken()) + } guard let entry = entries[commandId] else { // The entry can be pruned; the fact it recorded is not, so an id that spent itself on the device // never executes again. Only the entry named the command, so this refusal can name only what the @@ -100,8 +114,9 @@ final class RunnerCommandJournal { return claim(command: command, commandId: commandId) } if !entry.state.isTerminal { - register(commandId) - return .join + let executionToken = entry.executionToken + register(executionToken) + return .join(executionToken: executionToken) } // Nothing of this command ever reached the device, so the id carries no effect to protect and the // send that reuses it is that command's own first execution. @@ -118,39 +133,28 @@ final class RunnerCommandJournal { ) } + /// Mints a claim identity. Callers hold the journal's lock, except for the no-`commandId` route, which + /// needs no entry and only asks for a value no other claim carries. + private func mintExecutionToken() -> Int { + nextExecutionToken += 1 + return nextExecutionToken + } + func start(command: Command) { - update( - command: command, - state: .started, - responseOk: nil, - responseJson: nil, - error: nil, - marksDispatched: false - ) + transition(command: command, reorders: true) { entry in + entry.state = .started + } } /// Records that this command's work was handed to the device, as opposed to being refused while it /// was still queued. Until this point a refusal owes the caller a retry of the same `commandId`. - func markDispatched(command: Command) { - update( - command: command, - state: .started, - responseOk: nil, - responseJson: nil, - error: nil, - marksDispatched: true - ) - } - func finish(command: Command, response: Response) { - update( - command: command, - state: response.ok ? .completed : .failed, - responseOk: response.ok, - responseJson: encodeResponseJson(command: command, response: response), - error: response.error, - marksDispatched: false - ) + transition(command: command, reorders: true) { entry in + entry.state = response.ok ? .completed : .failed + entry.responseOk = response.ok + entry.responseJson = encodeResponseJson(command: command, response: response) + entry.error = response.error + } } /// Records the failure that ended this command's execution. The typed wire code goes with it, the @@ -159,17 +163,14 @@ final class RunnerCommandJournal { /// ordinary served refusal — which for a `MAIN_THREAD_TIMEOUT` would report the main thread drained /// when the command that just timed out is what occupied it. func fail(command: Command, error: Error) { - update( - command: command, - state: .failed, - responseOk: nil, - responseJson: nil, - error: ErrorPayload( - code: RunnerTests.wireErrorCode(for: error), - message: error.localizedDescription - ), - marksDispatched: false + let payload = ErrorPayload( + code: RunnerTests.wireErrorCode(for: error), + message: error.localizedDescription ) + transition(command: command, reorders: true) { entry in + entry.state = .failed + entry.error = payload + } } func status(normalizedCommandId commandId: String) -> DataPayload { @@ -196,45 +197,50 @@ final class RunnerCommandJournal { /// Writes the claim for one execution. Callers hold the lock. private func claim(command: Command, commandId: String) -> RunnerCommandAdmission { + let executionToken = mintExecutionToken() entries[commandId] = RunnerCommandJournalEntry( commandId: commandId, command: command.command.rawValue, state: .accepted, responseOk: nil, responseJson: nil, - error: nil + error: nil, + executionToken: executionToken ) order.removeAll { $0 == commandId } order.append(commandId) pruneIfNeeded() - return .execute + return .execute(executionToken: executionToken) } - /// Moves one claimed entry forward. A transition for an id the journal does not hold describes work it - /// never claimed, and one for an entry that already finished restates a closed outcome: neither may - /// open a record again. Re-opening one would report `accepted` or `started` for work that is not - /// running, and the daemon holds that verdict as an in-flight exchange on a runner it would otherwise - /// hand off. - private func update( + /// Records that this command's work was handed to the device, as opposed to being refused while it + /// was still queued. Until this point a refusal owes the caller a retry of the same `commandId`. Only + /// the mark moves: the entry keeps its place in the retention order, which is claim order, and the + /// state `start` already wrote is what `status` reports for a running command. + func markDispatched(command: Command) { + transition(command: command, reorders: false) { entry in + entry.dispatched = true + } + } + + /// Applies one transition to the record this command's id holds. A transition for an id the journal does + /// not hold describes work it never claimed, and one for an entry that already finished restates a + /// closed outcome: neither may open a record again. Re-opening one would report `accepted` or `started` + /// for work that is not running, and the daemon holds that verdict as an in-flight exchange on a runner + /// it would otherwise hand off. `reorders` is what settling does: an entry that finishes goes to the back + /// of the retention order, which records claim order. + private func transition( command: Command, - state: RunnerCommandLifecycleState, - responseOk: Bool?, - responseJson: String?, - error: ErrorPayload?, - marksDispatched: Bool + reorders: Bool, + _ mutate: (inout RunnerCommandJournalEntry) -> Void ) { guard let commandId = command.commandId?.trimmedNonEmpty else { return } lock.lock() defer { lock.unlock() } guard var entry = entries[commandId], !entry.state.isTerminal else { return } - entry.state = state - entry.responseOk = responseOk - entry.responseJson = responseJson - entry.error = error - if marksDispatched { - entry.dispatched = true - } + mutate(&entry) entries[commandId] = entry + guard reorders else { return } order.removeAll { $0 == commandId } order.append(commandId) pruneIfNeeded() diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index be0469aa9f..188cac0c6f 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -212,8 +212,8 @@ extension RunnerTests { // command that already finished is answered from the journal, so a repeat that arrives after its // answer was delivered cannot drive the device a second time, and a send told to join can never be // left behind by a delivery that went out first. - let admission = commandJournal.accept(command: command) { [weak self] commandId in - self?.registerJoiner(for: commandId, completion: completion) + let admission = commandJournal.accept(command: command) { [weak self] executionToken in + self?.registerJoiner(for: executionToken, completion: completion) } switch admission { case .join: @@ -236,7 +236,7 @@ extension RunnerTests { command.commandId ?? "" ) completion((jsonResponse(status: 200, response: outcome), answersShutdown)) - case .execute: + case .execute(let executionToken): NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ACCEPTED command=%@ commandId=%@", command.command.rawValue, @@ -252,7 +252,7 @@ extension RunnerTests { response.ok ? 1 : 0 ) self.deliverCommandResult( - command: command, + executionToken: executionToken, result: (self.jsonResponse(status: 200, response: response), command.command == .shutdown), completion: completion ) @@ -264,7 +264,7 @@ extension RunnerTests { String(describing: error) ) self.deliverCommandResult( - command: command, + executionToken: executionToken, result: ( self.jsonResponse(status: 500, response: self.commandFailedResponse(from: error)), false @@ -321,29 +321,30 @@ extension RunnerTests { /// execution's own delivery answers it. The journal calls this while it holds its own lock and after it /// checked that the entry is still running, which is what makes the two indivisible: the transition that /// closes an entry needs that lock, and one close drains this list only after it has closed the entry, so - /// no send can be recorded here for a delivery that already went out. Takes only - /// `inFlightCommandLock`, and never the journal's, so nothing here can re-enter the journal. + /// no send can be recorded here for a delivery that already went out. The key is the execution's identity + /// rather than the `commandId`, because a refused-before-dispatch id is claimed again and the sends of + /// that execution must not join the refusal that freed the id. Takes only `inFlightCommandLock`, and + /// never the journal's, so nothing here can re-enter the journal. func registerJoiner( - for commandId: String, + for executionToken: Int, completion: @escaping @Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void ) { inFlightCommandLock.lock() - inFlightCommandWaiters[commandId, default: []].append(completion) + inFlightCommandWaiters[executionToken, default: []].append(completion) inFlightCommandLock.unlock() } - /// Answers the send that owns this execution, and every send the journal settled onto it. + /// Answers the send that owns this execution, and every send the journal settled onto it. No send can + /// join after the close that runs before this delivery, so the list this drains is exactly the sends that + /// joined the execution whose work produced `result`. func deliverCommandResult( - command: Command, + executionToken: Int, result: (response: RunnerHttpResponse, shouldFinish: Bool), completion: ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void ) { - var waiters: [@Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void] = [] - if let commandId = command.commandId?.trimmedNonEmpty { - inFlightCommandLock.lock() - waiters = inFlightCommandWaiters.removeValue(forKey: commandId) ?? [] - inFlightCommandLock.unlock() - } + inFlightCommandLock.lock() + let waiters = inFlightCommandWaiters.removeValue(forKey: executionToken) ?? [] + inFlightCommandLock.unlock() completion(result) for waiter in waiters { waiter(result) @@ -382,7 +383,14 @@ extension RunnerTests { /// the caller lost. func commandFailedResponse(from error: Error) -> Response { let code = RunnerTests.wireErrorCode(for: error) - if code == RunnerWireErrorCode.mainThreadTimeout { + // The hint says the watchdog abandoned this command's work, so it goes only with the error the + // watchdog throws. The code is read from the one owner it derives from, but the branch keys on the + // error's own identity: a later code that `wireErrorCode(for:)` learns to derive must not pull an + // unrelated throw into this claim. + let nsError = error as NSError + if nsError.domain == RunnerErrorDomain.general, + nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut + { return Response( ok: false, error: ErrorPayload( diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift index ef2e48bbbd..c53285a775 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -123,10 +123,12 @@ final class RunnerTests: XCTestCase { var activeRecording: ScreenRecorder? let commandJournal = RunnerCommandJournal() let inFlightCommandLock = NSLock() - // Waits of a `commandId` whose execution is in flight, so the one execution answers every send that - // joined it (#1105 capture pileup). The journal owns whether an execution is in flight; this holds - // only the sends that attached to it. - var inFlightCommandWaiters: [String: [@Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void]] = [:] + /// Waits of an execution whose `commandId` is in flight, keyed by the execution's identity so the one + /// execution answers every send that joined it (#1105 capture pileup). The journal owns which execution + /// is in flight and mints the identity; this holds only the sends that attached to it. A `commandId` the + /// runner refused before dispatch is claimed again, and the key is what keeps that new execution's sends + /// from being answered by the delivery of the refusal that freed the id. + var inFlightCommandWaiters: [Int: [@Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void]] = [:] // Tracks main-queue work abandoned by the execution watchdog (runMainThreadWork). While any is // outstanding the main thread is occupied: new main-thread commands fail fast as busy instead // of queueing behind work that cannot be cancelled, capture plans skip XCTest-backed tiers, diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index 51317139ad..a01a0af858 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -361,7 +361,7 @@ extension RunnerTests { var waiterCount = 0 repeat { inFlightCommandLock.lock() - waiterCount = inFlightCommandWaiters["shared-shutdown"]?.count ?? 0 + waiterCount = inFlightCommandWaiters.values.reduce(0) { $0 + $1.count } inFlightCommandLock.unlock() if waiterCount == sockets.count - 1 { break } Thread.sleep(forTimeInterval: 0.01) @@ -482,17 +482,27 @@ extension RunnerTests { outcome: Response? ) -> RunnerHttpResponse { let executed = commandJournal.accept(command: command) - let joined = commandJournal.accept(command: command) { [weak self] commandId in - self?.registerJoiner(for: commandId) { result in + var executionToken = -1 + let joined = commandJournal.accept(command: command) { [weak self] token in + executionToken = token + self?.registerJoiner(for: token) { result in delivered.joinerData = result.response } } - if case .execute = executed {} else { + guard case .execute(let claimedToken) = executed else { XCTFail("the first send of a commandId must be admitted for execution") + return Data() } - if case .join = joined {} else { + guard case .join(let joinedToken) = joined else { XCTFail("a send of a commandId whose execution has not finished must join it") + return Data() } + XCTAssertEqual( + joinedToken, + executionToken, + "the joiner is registered for the execution the verdict names" + ) + XCTAssertEqual(claimedToken, joinedToken, "both sends speak of one execution") commandJournal.start(command: command) commandJournal.markDispatched(command: command) @@ -502,7 +512,7 @@ extension RunnerTests { commandJournal.fail(command: command, error: NSError(domain: "test", code: 7, userInfo: nil)) } let deliveredBytes = RunnerHttpResponse(status: 200, body: "one-delivery") - deliverCommandResult(command: command, result: (deliveredBytes, false)) { result in + deliverCommandResult(executionToken: claimedToken, result: (deliveredBytes, false)) { result in delivered.primaryData = result.response } return deliveredBytes @@ -523,7 +533,7 @@ extension RunnerTests { XCTAssertEqual(delivered.primaryData, delivery) XCTAssertEqual(delivered.joinerData, delivery) - XCTAssertNil(inFlightCommandWaiters["scroll-coalesce"]) + assertNoJoinersLeft("the close must drain every send that joined it") // The execution has answered and closed. A send that arrives now is the finished command's repeat, and // the journal settles it with the recorded outcome instead of registering it onto a delivery that @@ -538,7 +548,7 @@ extension RunnerTests { XCTAssertFalse(registeredLateSend, "a send answered from the journal must not be registered") XCTAssertTrue(outcome.ok) XCTAssertEqual(outcome.data?.message, "captured") - XCTAssertNil(inFlightCommandWaiters["scroll-coalesce"], "a journal answer must not register a waiter") + assertNoJoinersLeft("a journal answer must not register a waiter") } /// The interleaving that used to leave a connection unanswered: the verdict said join, and the execution @@ -561,7 +571,7 @@ extension RunnerTests { assertJournalWindowDropped("scroll-join-pruned") XCTAssertEqual(delivery, delivered.joinerData, "the close that pruned the entry must answer its joiner") - XCTAssertNil(inFlightCommandWaiters["scroll-join-pruned"], "answering must not leave a waiter behind") + assertNoJoinersLeft("answering must not leave a waiter behind") } /// A command that fails closes the entry through the same transition, so its joiner is owed the same @@ -574,7 +584,56 @@ extension RunnerTests { let delivery = runExecutionWithJoiner(command, into: delivered, outcome: nil) XCTAssertEqual(delivery, delivered.joinerData, "the failure that closed the entry must answer its joiner") - XCTAssertNil(inFlightCommandWaiters["scroll-join-failed"]) + assertNoJoinersLeft("a failed close must drain its joiner too") + } + + /// A `commandId` the runner refused before dispatch is claimed again, and the delivery of that refusal + /// must not answer the sends of the execution that followed it. The journal mints one identity per + /// claim and the transport keys its joiners by it, so the two generations cannot cross. + func testDeliveryOfARefusalNeverAnswersTheJoinersOfTheExecutionAfterIt() throws { + let command = try transportCommand("scroll", id: "scroll-refused-then-rerun") + defer { inFlightCommandWaiters.removeAll() } + + // Generation one: claimed, refused before dispatch, and its entry closed with `dispatched == false`. + guard case .execute(let firstToken) = commandJournal.accept(command: command) else { + return XCTFail("the first send must be admitted") + } + commandJournal.start(command: command) + commandJournal.finish(command: command, response: Response(ok: false, data: nil)) + + // Generation two: the same id, admitted again because the refusal left no effect to protect. A send + // joins this execution while it runs. + guard case .execute(let secondToken) = commandJournal.accept(command: command) else { + return XCTFail("a commandId refused before dispatch must be claimable again") + } + XCTAssertNotEqual(firstToken, secondToken, "each claim gets its own identity") + let second = TransportJoiner() + let joinVerdict = commandJournal.accept(command: command) { [weak self] token in + self?.registerJoiner(for: token) { result in + second.joinerData = result.response + } + } + guard case .join = joinVerdict else { + return XCTFail("a send of the running execution's id must join it") + } + + // The refused generation delivers its own answer. It must reach nobody from the next generation. + deliverCommandResult(executionToken: firstToken, result: (Data("refusal".utf8), false)) { _ in } + XCTAssertNil(second.joinerData, "a refusal must not answer the execution that replaced it") + + commandJournal.start(command: command) + commandJournal.markDispatched(command: command) + commandJournal.finish(command: command, response: Response(ok: true, data: DataPayload(message: "ran"))) + deliverCommandResult(executionToken: secondToken, result: (Data("ran".utf8), false)) { _ in } + XCTAssertEqual(second.joinerData, Data("ran".utf8), "the joiner is answered by its own execution") + } + + /// Asserts that no send is left waiting for a delivery, whichever execution it joined. + private func assertNoJoinersLeft(_ message: String) { + XCTAssertFalse( + inFlightCommandWaiters.values.contains { !$0.isEmpty }, + message + ) } /// One send's answers on the transport route: the execution's own delivery and the join's. diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index 3b0d8c4c2d..262ec9420d 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -120,7 +120,7 @@ Thus, it is the single place for a change to that classification. Its verdict is `execute`, `join`, or `terminal`: - `execute` claims the `commandId`, and that send owns the one execution. -- `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. +- `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. Each claim carries an execution identity the journal mints, and the transport keys its waiters by it: an id refused before dispatch is claimed again, and the delivery of that refusal must not answer the sends of the execution that follows. - `terminal` means execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. Whether that answer ends the runner is the journal's fact about the record it read, not the repeat's request: a caller may reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded command's. The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` refusal, not at accept. diff --git a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts index 8ead048356..e962af1e46 100644 --- a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts +++ b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts @@ -477,10 +477,21 @@ function runnerReplyFailure(runnerCode: string): AppError { // Swift runner really encodes this string — is the test below, which reads the Swift declaration. test('the journal refusal of a finished commandId reports nothing about main-thread occupancy', () => { assert.equal(COMMAND_ALREADY_FINISHED_RUNNER_CODE, 'COMMAND_ALREADY_FINISHED'); - assert.equal( - runnerMainThreadOccupancyReport(runnerReplyFailure('COMMAND_ALREADY_FINISHED')), - 'silent', - ); + // The published shape, not only the runner detail: this code travels under its own name the way + // `NO_TEXT_INPUT_AT_POINT` does, rather than staying `COMMAND_FAILED` like the diagnostic codes, + // because no caller policy reads it — the runner will not re-run the command, whatever an earlier + // send of that id did. + const refusal = classifyRunnerReportedError(COMMAND_ALREADY_FINISHED_RUNNER_CODE); + assert.equal(refusal.code, 'COMMAND_ALREADY_FINISHED'); + assert.equal(refusal.details.dispatched, 'unknown'); + assert.equal(refusal.details.retriable, undefined); + const refusalError = new AppError(refusal.code, 'already finished', { + ...refusal.details, + runner: { ok: false, error: { code: COMMAND_ALREADY_FINISHED_RUNNER_CODE, message: 'x' } }, + }); + assert.equal(isRetryableRunnerError(refusalError), false); + assert.equal(isRunnerBusyError(refusalError), false); + assert.equal(runnerMainThreadOccupancyReport(refusalError), 'silent'); // Without the structured runner payload there is no answer at all, which is also silent. assert.equal(runnerMainThreadOccupancyReport(commandFailed('boom')), 'silent'); }); From 79b44d60a6a372e4731d39622d46cb3a73b80a4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 22:20:32 +0100 Subject: [PATCH 10/17] fix(ios): reexecute finished reads without spending their ids --- .../RunnerTests+CommandDispatch.swift | 2 +- .../RunnerTests+CommandJournal.swift | 53 +++++++------- .../RunnerTests+Models.swift | 15 ++-- .../RunnerTests+Transport.swift | 35 +++------- .../RunnerTests.swift | 14 ++++ .../RunnerTests+CommandJournalTests.swift | 69 +++++++++++++++++-- .../RunnerTests+TransportTests.swift | 52 +++++++++++++- contracts/fixtures/dispatch-disclosure.json | 6 ++ .../0005-ios-runner-interaction-lifecycle.md | 14 ++-- .../index-dispatch-disclosure.test.ts | 1 + .../runner-swift-settlement-fixtures.ts | 29 ++++++-- website/docs/docs/commands.md | 2 + 12 files changed, 213 insertions(+), 79 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift index 8e3cd853c2..7f77331073 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift @@ -190,7 +190,7 @@ extension RunnerTests { return unavailable } // Past the last fast refusal, this command's work is going to the device, so its `commandId` is - // spent: a repeat of it is answered from the journal instead of running the device again. Only the + // spent for a non-read command. Finished reads can run again after a lost reply. Only the // `.execute` verdict reaches this line, so a repeat of a spent id is refused at the transport and no // busy refusal here can follow the mark for the command it refuses (ADR 0005). commandJournal.markDispatched(command: command) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index 415e4004bb..e93d121b37 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -36,18 +36,19 @@ struct RunnerCommandJournalEntry { /// again, and the sends of that new execution must not be answered by the delivery of the refusal that /// freed the id. let executionToken: Int + let isReadOnly: Bool } /// The journal's verdict on one command that carries a `commandId`. enum RunnerCommandAdmission { - /// Nothing is running for this id and nothing has finished: the caller owns the one execution, + /// The caller owns this execution, /// identified by `executionToken` for as long as that claim answers for it. case execute(executionToken: Int) /// An execution is claimed and has not finished: the caller runs nothing and is answered by that /// execution, the one `executionToken` names. `accept(command:registeringJoin:)` has already /// registered the send it gave this verdict. case join(executionToken: Int) - /// Execution finished, and the work reached the device: the caller answers from the journal and + /// Non-read execution finished after dispatch: the caller answers from the journal and /// never executes this command again. `answersShutdown` says whether the answer being handed over is /// the runner's own successful `shutdown` answer, which the caller owes the runner's end. Neither the /// repeat's request nor the command name alone can answer that: a caller may reuse a spent `commandId` @@ -57,14 +58,13 @@ enum RunnerCommandAdmission { /// The runner's one command-lifecycle record: what it holds for a `commandId`, whether that id may run, /// and what `status` answers for it. The transport asks this type before enqueuing anything, so one -/// `commandId` is executed at most once in a runner process, and a command that already finished cannot be -/// replayed onto the device by a repeat that arrives after its answer was delivered. The one release is -/// `closedCommandIdSafetyValve`, and `pruneIfNeeded` states the bound it leaves. +/// mutation id stays spent after execution, while a finished read can execute again after a lost reply. +/// `closedCommandIdSafetyValve` releases the oldest marker; `pruneIfNeeded` states its bound. final class RunnerCommandJournal { private let lock = NSLock() private let maxEntries = 64 /// Memory safety valve on closed-id retention, not an admission boundary: the journal keeps a closed - /// id as long as it runs, and this only stops a runner from growing without limit. It is 64x the entry + /// mutation id until its marker is evicted. It is 64x the entry /// window because a marker holds no outcome body, only the id. Tests read it rather than repeat it. static let closedCommandIdSafetyValve = 4096 private let maxResponseJsonBytes = 16 * 1024 @@ -118,9 +118,10 @@ final class RunnerCommandJournal { register(executionToken) return .join(executionToken: executionToken) } - // Nothing of this command ever reached the device, so the id carries no effect to protect and the - // send that reuses it is that command's own first execution. - guard entry.dispatched else { + // Reads execute again after finishing, including after a lost reply or a recorded failure. + // A pre-dispatch refusal also leaves the id reusable. Classification belongs to the recorded + // command: renaming a spent mutation to a read must not bypass its admission history. + guard entry.dispatched, !entry.isReadOnly else { return claim(command: command, commandId: commandId) } let outcome = recordedOutcome(for: entry) @@ -133,8 +134,7 @@ final class RunnerCommandJournal { ) } - /// Mints a claim identity. Callers hold the journal's lock, except for the no-`commandId` route, which - /// needs no entry and only asks for a value no other claim carries. + /// Callers hold the journal's lock, including for commands without an id. private func mintExecutionToken() -> Int { nextExecutionToken += 1 return nextExecutionToken @@ -146,8 +146,9 @@ final class RunnerCommandJournal { } } - /// Records that this command's work was handed to the device, as opposed to being refused while it - /// was still queued. Until this point a refusal owes the caller a retry of the same `commandId`. + /// Records the outcome that ended this command's execution: the state its `ok` implies, the response + /// body where the command's trait retains a replayable one, and its error. Whether the id this record + /// closes is spent is the separate `dispatched` mark `markDispatched` writes. func finish(command: Command, response: Response) { transition(command: command, reorders: true) { entry in entry.state = response.ok ? .completed : .failed @@ -157,16 +158,13 @@ final class RunnerCommandJournal { } } - /// Records the failure that ended this command's execution. The typed wire code goes with it, the - /// same one the transport puts on the live reply for the same throw: a repeat of this `commandId` is - /// answered from this record, and a failure that arrives without its code reads to the daemon as an - /// ordinary served refusal — which for a `MAIN_THREAD_TIMEOUT` would report the main thread drained - /// when the command that just timed out is what occupied it. + /// Records the failure that ended this command's execution. The typed wire code and the hint go with it, + /// the same error body the transport puts on the live reply for the same throw: a repeat of this + /// `commandId` is answered from this record, and a failure that arrives without its code reads to the + /// daemon as an ordinary served refusal — which for a `MAIN_THREAD_TIMEOUT` would report the main thread + /// drained when the command that just timed out is what occupied it. func fail(command: Command, error: Error) { - let payload = ErrorPayload( - code: RunnerTests.wireErrorCode(for: error), - message: error.localizedDescription - ) + let payload = RunnerTests.thrownErrorPayload(for: error) transition(command: command, reorders: true) { entry in entry.state = .failed entry.error = payload @@ -205,7 +203,8 @@ final class RunnerCommandJournal { responseOk: nil, responseJson: nil, error: nil, - executionToken: executionToken + executionToken: executionToken, + isReadOnly: command.traits.isReadOnly ) order.removeAll { $0 == commandId } order.append(commandId) @@ -257,9 +256,9 @@ final class RunnerCommandJournal { return } order.removeAll { $0 == removable } - // Only work that reached the device spends its id. A pruned refusal leaves the id free, which is - // what the busy resend of the same `commandId` depends on (ADR 0005). - if entries[removable]?.dispatched == true { + // Only dispatched non-read commands spend their ids. Reads and pre-dispatch refusals leave + // no marker, so resends can execute after the outcome is pruned (ADR 0005). + if let entry = entries[removable], entry.dispatched, !entry.isReadOnly { // The two closed-id structures stay one-to-one, and nothing here needs a duplicate check to keep // them that way: an id enters `closedCommandIds` only in this branch, and an id whose entry this // journal no longer holds can be claimed again only once the marker is gone — which the valve @@ -325,7 +324,7 @@ final class RunnerCommandJournal { code: RunnerTests.RunnerWireErrorCode.commandAlreadyFinished, message: reason, hint: - "Read the screen with snapshot before sending anything else: the finished command's effect is already on the device." + "Read the screen with snapshot before sending anything else: the finished command's work reached the device, so it may already have had its effect." ) ) } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift index 9d73aabea5..7f13e0fb81 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift @@ -108,19 +108,23 @@ struct CommandTraits { let convertsRecordedFailure: Bool /// Whether the journal may store this command's response JSON, subject to its byte limit. let retainsJournalResponseJson: Bool + /// Whether a finished command may execute again under the same id after a lost reply. + let isReadOnly: Bool init( isInteraction: Bool = false, retryOnSessionLoss: Bool = false, launchPolicy: CommandLaunchPolicy, convertsRecordedFailure: Bool = false, - retainsJournalResponseJson: Bool + retainsJournalResponseJson: Bool, + isReadOnly: Bool = false ) { self.isInteraction = isInteraction self.retryOnSessionLoss = retryOnSessionLoss self.launchPolicy = launchPolicy self.convertsRecordedFailure = convertsRecordedFailure self.retainsJournalResponseJson = retainsJournalResponseJson + self.isReadOnly = isReadOnly } } @@ -147,7 +151,8 @@ fileprivate extension CommandTraits { static func appRead(retainsJournalResponseJson: Bool) -> CommandTraits { CommandTraits( retryOnSessionLoss: true, launchPolicy: .existingApp, - retainsJournalResponseJson: retainsJournalResponseJson + retainsJournalResponseJson: retainsJournalResponseJson, + isReadOnly: true ) } @@ -166,7 +171,8 @@ fileprivate extension CommandTraits { static func runnerCaptureRead(retainsJournalResponseJson: Bool) -> CommandTraits { CommandTraits( retryOnSessionLoss: true, launchPolicy: .noApp, - retainsJournalResponseJson: retainsJournalResponseJson + retainsJournalResponseJson: retainsJournalResponseJson, + isReadOnly: true ) } @@ -191,7 +197,8 @@ fileprivate extension CommandTraits { static let presentedSurfaceQuery = CommandTraits( retryOnSessionLoss: true, launchPolicy: .presentedSurface, - retainsJournalResponseJson: true + retainsJournalResponseJson: true, + isReadOnly: true ) } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index 188cac0c6f..e531d4b4b8 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -230,6 +230,10 @@ extension RunnerTests { // spent `commandId` under another command name, and what it receives is still the recorded // command's outcome. `finish()` guards its own expectation against being fulfilled twice, so a // shutdown that several sends receive is harmless. + // `jsonResponse` stamps the outcome it is given, so a replayed success reports the main thread's + // occupancy NOW rather than the occupancy the recorded answer was written under — the stored body + // is kept unstamped for exactly this reason. That is what lets a replayed answer settle a stale + // busy report on the daemon side; the journal itself observed nothing. NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ALREADY_FINISHED command=%@ commandId=%@", command.command.rawValue, @@ -378,34 +382,11 @@ extension RunnerTests { Response(ok: false, error: ErrorPayload(code: code, message: message, hint: hint)) } - /// Turns a thrown command error into its wire response, under the one code mapping the journal's - /// failure record uses, so the answer a resend reads from the journal cannot disagree with the answer - /// the caller lost. + /// Turns a thrown command error into its wire response, under the one error body the journal's failure + /// record uses, so the answer a resend reads from the journal cannot disagree with the answer the caller + /// lost. func commandFailedResponse(from error: Error) -> Response { - let code = RunnerTests.wireErrorCode(for: error) - // The hint says the watchdog abandoned this command's work, so it goes only with the error the - // watchdog throws. The code is read from the one owner it derives from, but the branch keys on the - // error's own identity: a later code that `wireErrorCode(for:)` learns to derive must not pull an - // unrelated throw into this claim. - let nsError = error as NSError - if nsError.domain == RunnerErrorDomain.general, - nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut - { - return Response( - ok: false, - error: ErrorPayload( - code: code, - message: error.localizedDescription, - hint: - "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." - ) - ) - } - return errorResponse( - code: code, - message: error.localizedDescription, - hint: "Check the runner log for XCTest details, then retry after the app is foregrounded if this was a timeout or activation failure." - ) + Response(ok: false, error: RunnerTests.thrownErrorPayload(for: error)) } /// Present on every response so the daemon can refuse a runner build that predates the token. diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift index c53285a775..a329db5454 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -64,6 +64,20 @@ final class RunnerTests: XCTestCase { : "COMMAND_FAILED" } + /// A replayed failure must carry the same code and recovery hint as its live reply. + static func thrownErrorPayload(for error: Error) -> ErrorPayload { + let nsError = error as NSError + let abandoned = nsError.domain == RunnerErrorDomain.general + && nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut + return ErrorPayload( + code: wireErrorCode(for: error), + message: error.localizedDescription, + hint: abandoned + ? "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." + : "Check the runner log for XCTest details, then retry after the app is foregrounded if this was a timeout or activation failure." + ) + } + static let springboardBundleId = "com.apple.springboard" // SpringBoard hosts blocking system modals on iOS/visionOS; tvOS (PineBoard/HeadBoard) // and macOS have no such host, so there is nothing to probe there. diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 363e24631e..5fa4bebe67 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -210,7 +210,7 @@ extension RunnerTests { dispatched: Bool = false ) { for index in 0.. Command { let json = #"{"command":"\#(command)","commandId":"\#(id)"}"# return try! JSONDecoder().decode(Command.self, from: Data(json.utf8)) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index a01a0af858..c3b210ae3b 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -1,5 +1,8 @@ import XCTest import Network +#if os(iOS) +import UIKit +#endif #if AGENT_DEVICE_RUNNER_UNIT_TESTS extension RunnerTests { @@ -728,6 +731,51 @@ extension RunnerTests { XCTAssertEqual(resend.error?.code, RunnerWireErrorCode.commandAlreadyFinished) } + #if os(iOS) + @MainActor + func testTransportReplaysPasteboardWriteWithoutWritingAgain() throws { + let original = UIPasteboard.general.string + defer { UIPasteboard.general.string = original } + let listener = try startTransportListener() + defer { listener.cancel() } + let port = try XCTUnwrap(listener.port) + let client = URLSession(configuration: .ephemeral) + defer { client.invalidateAndCancel() } + let first = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"pasteboardWrite","commandId":"pasteboard-twice","text":"journal-first-write"}"# + ) + XCTAssertTrue(first.ok, String(describing: first.error)) + XCTAssertEqual(UIPasteboard.general.string, "journal-first-write") + let repeated = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"pasteboardWrite","commandId":"pasteboard-twice","text":"would-overwrite-on-reexecution"}"# + ) + XCTAssertTrue(repeated.ok, String(describing: repeated.error)) + XCTAssertEqual(UIPasteboard.general.string, "journal-first-write") + XCTAssertEqual(repeated.data?.runnerMainThreadBusy, false) + } + #endif + + @MainActor + func testTransportReexecutesAFinishedSnapshotWithTheSameId() throws { + app.launch() + defer { app.terminate() } + let listener = try startTransportListener() + defer { listener.cancel() } + let port = try XCTUnwrap(listener.port) + let client = URLSession(configuration: .ephemeral) + defer { client.invalidateAndCancel() } + let payload = #"{"command":"snapshot","commandId":"snapshot-twice","interactiveOnly":true}"# + + for _ in 0..<2 { + let reply = try transportHttpResponse(client: client, port: port, payload: payload) + XCTAssertTrue(reply.ok, String(describing: reply.error)) + let nodes = try XCTUnwrap(reply.data?.nodes) + XCTAssertFalse(nodes.isEmpty) + } + } + private func transportCommand(_ command: String, id: String) throws -> Command { try JSONDecoder().decode( Command.self, @@ -766,7 +814,7 @@ extension RunnerTests { mainOwned.bundleId = nil } let payload = - #"{"command":"alert","commandId":"alert-timed-out","appBundleId":"com.apple.springboard","action":"get","timeoutMs":500}"# + #"{"command":"alert","commandId":"alert-timed-out","appBundleId":"com.apple.springboard","action":"accept","timeoutMs":500}"# let command = try JSONDecoder().decode(Command.self, from: Data(payload.utf8)) XCTAssertThrowsError(try execute(command: command)) { error in @@ -836,7 +884,7 @@ extension RunnerTests { /// id really ran. Ending the runner there would stop it on some other command's answer, so the resend /// is answered and the runner keeps serving. func testTransportDoesNotFinishOnAReplayOfAnotherCommandsRecordedAnswer() throws { - let spent = try transportCommand("readText", id: "spent-then-named-shutdown") + let spent = try transportCommand("recordStart", id: "spent-then-named-shutdown") commandJournal.accept(command: spent) commandJournal.markDispatched(command: spent) commandJournal.finish( diff --git a/contracts/fixtures/dispatch-disclosure.json b/contracts/fixtures/dispatch-disclosure.json index b9eb364c46..a7ad1eaf76 100644 --- a/contracts/fixtures/dispatch-disclosure.json +++ b/contracts/fixtures/dispatch-disclosure.json @@ -271,6 +271,12 @@ "trigger": "structured runner reply with code UNSUPPORTED_OPERATION, which a synthesized gesture or an element tap also reports after it ran", "dispatched": "unknown" }, + { + "id": "ios-runner.reply.COMMAND_ALREADY_FINISHED", + "producer": "ios-runner", + "trigger": "structured runner reply with code COMMAND_ALREADY_FINISHED: the journal refused a repeat of a spent commandId and kept no outcome to replay, so it cannot say what that command did", + "dispatched": "unknown" + }, { "id": "ios-runner.reply.unlisted-code", "producer": "ios-runner", diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index 262ec9420d..7576b65829 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -14,8 +14,8 @@ Accepted - Invalidate dead cached runner processes without graceful `shutdown`. - After `RUNNER_BUSY`, resend only read-only commands, under their busy-specific budget. - After `RUNNER_BUSY` on a mutating command, return the refusal to the caller without a resend. -- Let the runner's command journal decide once per `commandId` whether a send executes, joins the running execution, or is answered from the recorded outcome, and register a joining send for that answer inside the same decision. -- Never execute a `commandId` twice within one runner process, never evict a journal entry whose execution has not finished, and keep the fact that a spent `commandId` was spent after its entry is pruned. The kept fact is released only by a memory safety valve; see the admission rule below for its bound. +- For each journaled send, let the runner's command journal decide whether it executes, joins the running execution, or is answered from the recorded outcome. Inside the same decision, register a joining send for that answer. +- Within the closed-id retention window, never execute a dispatched non-read `commandId` twice. Never evict a journal entry whose execution has not finished. After a dispatched non-read entry is pruned, keep its id in that window. The kept fact is released only by a memory safety valve; see the admission rule below for its bound. - If `close` would retain a runner whose main-thread occupancy is set, stop that runner first and await the stop. - After a root AX failure, clear the cached app target. Keep raw/strict errors distinct from interactive recovery payloads. @@ -116,21 +116,21 @@ This bypass is in `runner-lifecycle.ts`. The same trait already decides the startup-preflight skip above. Thus, it is the single place for a change to that classification. -`RunnerCommandJournal` owns command admission, and the transport asks it once per inbound request. +`RunnerCommandJournal` owns command admission, and the transport asks it once per journaled request. Inline `status` and `uptime` probes bypass admission. Its verdict is `execute`, `join`, or `terminal`: - `execute` claims the `commandId`, and that send owns the one execution. - `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. Each claim carries an execution identity the journal mints, and the transport keys its waiters by it: an id refused before dispatch is claimed again, and the delivery of that refusal must not answer the sends of the execution that follows. -- `terminal` means execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. Whether that answer ends the runner is the journal's fact about the record it read, not the repeat's request: a caller may reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded command's. +- `terminal` means a non-read execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. Whether that answer ends the runner is the journal's fact about the record it read, not the repeat's request: a caller may reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded command's. -The journal closes a `commandId` at the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` refusal, not at accept. +After the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` check, the journal marks dispatch. A finished non-read entry with that mark spends its `commandId`. A refusal before that point closed its entry as `failed` but dispatched nothing, so the `commandId` stays reusable and the read-only busy resend above can still run under it. Answering a closed `commandId` uses the retained response, then the retained error, then the typed `COMMAND_ALREADY_FINISHED` refusal for a command whose own classification retains no body. That refusal is a structured failure the daemon does not retry. Its dispatch disclosure stays `unknown`: the runner will not re-run the command, and only the runner knows whether an earlier send of that id reached the device. That refusal also reports nothing about the runner's main thread. The journal wrote it without executing, so the runner may be idle or mid-drain and the refusal cannot say which. The daemon's occupancy rule has three readings, occupied, drained, and this one, silent, and only the first two write `runnerMainThreadBusy`; a repeat refused while abandoned work drains must leave that report standing, because retention and handoff act on it. Pruning drops only closed entries, so a queued or executing command always stays queryable. -When a closed entry that dispatched is pruned, the journal keeps its id in a separate closed-id window, because a bare id carries no outcome body. +When a closed non-read entry that dispatched is pruned, the journal keeps its id in a separate closed-id window, because a bare id carries no outcome body. A resend inside that window is answered with the typed refusal; `status` still answers `notAccepted` for it, because only the entry can report an outcome. For a command whose answer the daemon lost, that reading keeps the charge: recovery cannot settle a charge on a state it cannot read as terminal, so it stops the runner instead of handing it on, which is the safe half of the trade the window makes. An id the runner refused before dispatch is left free, which is what the read-only busy resend depends on. @@ -140,7 +140,7 @@ The closed-id window holds 4096 ids, 64x the entry window, and it is a memory sa A watermark over issued ids was rejected: the daemon mints `commandId` as a random UUID per request, reuses one id across retries and across runner generations, and persists nothing, so no ordering exists to compare. Eviction on a daemon resend deadline was rejected: the daemon holds no per-id "never resend after this instant" value — its resend budgets are attempt counts and per-exchange timeouts redrawn per attempt, and a restart-and-replay can legitimately post the same id minutes later — so a deadline-based rule would need a wire-contract change this change does not make. -Rejected: gating re-admission on the runner's read-only classification. That would replay a finished `snapshot` after its caller already received the answer, which is not the same command's first execution. +`CommandTraits.isReadOnly` is declared by `appRead`, `runnerCaptureRead`, and `presentedSurfaceQuery`. It is independent of `retryOnSessionLoss` and response retention. When a read entry finishes, `accept` claims it again instead of answering from its recorded outcome, even if that outcome was a failure. Pruning a read leaves no closed-id marker. Reads therefore never spend their ids, and a lost-response resend of `snapshot` captures the screen again. Admission uses the recorded entry's trait, so renaming a spent mutation to a read cannot bypass its marker or retained outcome. Rejected: keeping the transport's own in-flight id set beside the journal. Two records of one lifecycle drifted: the set was cleared when the answer was delivered while the journal still held the entry, so a repeat arriving in that window was admitted and executed again. diff --git a/packages/apple-runner/src/__tests__/index-dispatch-disclosure.test.ts b/packages/apple-runner/src/__tests__/index-dispatch-disclosure.test.ts index 3db43476a1..016ff9eded 100644 --- a/packages/apple-runner/src/__tests__/index-dispatch-disclosure.test.ts +++ b/packages/apple-runner/src/__tests__/index-dispatch-disclosure.test.ts @@ -181,6 +181,7 @@ const DRIVERS: Record Promise> = { 'ios-runner.reply.ELEMENT_OFFSCREEN': () => replyFailure('ELEMENT_OFFSCREEN'), 'ios-runner.reply.AMBIGUOUS_MATCH': () => replyFailure('AMBIGUOUS_MATCH'), 'ios-runner.reply.MAIN_THREAD_TIMEOUT': () => replyFailure('MAIN_THREAD_TIMEOUT'), + 'ios-runner.reply.COMMAND_ALREADY_FINISHED': () => replyFailure('COMMAND_ALREADY_FINISHED'), 'ios-runner.reply.unlisted-code': () => replyFailure('XCTEST_RECORDED_FAILURE'), 'ios-runner.series.later-chunk-refused': pressSeriesRefusedOnSecondChunk, 'ios-runner.series.invalid-step-before-send': pressSeriesWithInvalidStep, diff --git a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts index 53ed4342cd..1d2928d99c 100644 --- a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts +++ b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts @@ -152,17 +152,17 @@ function readSwiftWireErrorCodes(): Record { } /** - * The wire code the runner derives for one thrown error. Both readers of a failed command — the + * The error body the runner derives for one thrown error. Both readers of a failed command — the * transport encoding a live reply, and the journal recording a thrown one so a repeat can be answered * from it — must use this one declaration, or the answer a resend receives reports a different runner - * condition than the answer the caller lost. The daemon keys main-thread occupancy on the timeout code, - * so the pair this ties is genuinely cross-language: it reads the Swift side rather than a second copy - * of the same string in a TypeScript test. + * condition, or carries less, than the answer the caller lost. The daemon keys main-thread occupancy on + * the timeout code, so the pair this ties is genuinely cross-language: it reads the Swift side rather + * than a second copy of the same string in a TypeScript test. */ export function requireSwiftThrownErrorWireCode(): { /** The Swift member whose value the daemon's occupancy verdict keys on. */ timeoutCode: string; - /** Whether the journal's failure record derives its code from the same declaration. */ + /** Whether the journal's failure record derives its error body from the same owner the transport uses. */ journalRecordsDerivedCode: boolean; } { const codes = readSwiftWireErrorCodes(); @@ -181,8 +181,25 @@ export function requireSwiftThrownErrorWireCode(): { 'RunnerCommandJournal must declare fail(command:error:) — the record a repeated commandId is ' + 'answered from when the command closed by throwing.', ); + const transport = fs.readFileSync(SWIFT_TRANSPORT, 'utf8'); + const live = transport.match( + /func commandFailedResponse\(from error: Error\)[\s\S]*?\n {2}\}/, + )?.[0]; + assert.ok( + live !== undefined, + 'RunnerTests+Transport.swift must declare commandFailedResponse(from:) — the live reply a ' + + 'failed command is answered with.', + ); + // One owner for the whole error body, not just the code: a record that kept the code but dropped the + // hint would send a caller to the runner log for a condition whose live answer told it to wait. + const owner = /\bRunnerTests\.thrownErrorPayload\(for: error\)/; + assert.ok( + owner.test(live), + 'commandFailedResponse(from:) must encode the live failure with RunnerTests.thrownErrorPayload(for:) ' + + '— the one declaration that pairs the code with the hint the caller acts on.', + ); return { timeoutCode, - journalRecordsDerivedCode: /RunnerTests\.wireErrorCode\(for: error\)/.test(recorded), + journalRecordsDerivedCode: owner.test(recorded), }; } diff --git a/website/docs/docs/commands.md b/website/docs/docs/commands.md index 12fbc94b45..525e13870a 100644 --- a/website/docs/docs/commands.md +++ b/website/docs/docs/commands.md @@ -539,6 +539,8 @@ agent-device wait 'id="field-phone" value="00 062 91 77"' ## Retry after a failed command +If an Apple runner returns `COMMAND_ALREADY_FINISHED`, it has already finished that command ID and cannot replay its outcome. Run `snapshot -i` to inspect the result before deciding whether to send another interaction. + When a command fails, read `error.details.dispatched` before you retry: - `no`: the action never reached the device. Check `details.reason` before retrying; a bounded From 0746c01811977f2e1ca5813705f11bb069828854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 22:44:42 +0100 Subject: [PATCH 11/17] fix(ios): preserve selector retries and pasteboard test state --- .../RunnerTests+Models.swift | 3 ++- .../RunnerTests+CommandJournalTests.swift | 4 +-- .../RunnerTests+TransportTests.swift | 27 +++++++++++++++++-- .../0005-ios-runner-interaction-lifecycle.md | 2 +- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift index 7f13e0fb81..d59c982eed 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift @@ -163,7 +163,8 @@ fileprivate extension CommandTraits { static let selectorResolution = CommandTraits( launchPolicy: .existingApp, convertsRecordedFailure: true, - retainsJournalResponseJson: true + retainsJournalResponseJson: true, + isReadOnly: true ) /// Reads the runner answers from its own capture and state, so preparation never brings an app diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 5fa4bebe67..3aa8cb53a4 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -637,7 +637,7 @@ extension RunnerTests { } func testJournalReexecutesFinishedReadsWithoutSpendingTheirIds() throws { - for name in ["snapshot", "screenshot", "readText", "findText", "gestureViewport", "appState", "status", "alert"] { + for name in ["snapshot", "screenshot", "readText", "findText", "gestureViewport", "appState", "status", "alert", "querySelector"] { for failed in [false, true] { let journal = RunnerCommandJournal() let command = runnerJournalCommand(name, id: "repeated-read") @@ -665,7 +665,7 @@ extension RunnerTests { } func testJournalDoesNotSpendPrunedReadIds() throws { - for name in ["snapshot", "screenshot", "readText", "alert"] { + for name in ["snapshot", "screenshot", "readText", "alert", "querySelector"] { let journal = RunnerCommandJournal() let read = runnerJournalCommand(name, id: "pruned-read") runnerJournalCloseDispatchedCommand(journal, read, message: "read") diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index c3b210ae3b..0708eba31f 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -734,8 +734,8 @@ extension RunnerTests { #if os(iOS) @MainActor func testTransportReplaysPasteboardWriteWithoutWritingAgain() throws { - let original = UIPasteboard.general.string - defer { UIPasteboard.general.string = original } + let originalItems = UIPasteboard.general.items + defer { UIPasteboard.general.items = originalItems } let listener = try startTransportListener() defer { listener.cancel() } let port = try XCTUnwrap(listener.port) @@ -776,6 +776,29 @@ extension RunnerTests { } } + @MainActor + func testTransportReexecutesASelectorReadWithTheSameId() throws { + app.launch() + defer { app.terminate() } + let listener = try startTransportListener() + defer { listener.cancel() } + let port = try XCTUnwrap(listener.port) + let client = URLSession(configuration: .ephemeral) + defer { client.invalidateAndCancel() } + let first = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"Agent Device Runner"}"# + ) + XCTAssertTrue(first.ok, String(describing: first.error)) + XCTAssertEqual(first.data?.found, true) + let repeated = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"missing-journal-selector"}"# + ) + XCTAssertTrue(repeated.ok, String(describing: repeated.error)) + XCTAssertEqual(repeated.data?.found, false, "a finished selector read must execute instead of replaying its retained answer") + } + private func transportCommand(_ command: String, id: String) throws -> Command { try JSONDecoder().decode( Command.self, diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index 7576b65829..a4bfdeb594 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -140,7 +140,7 @@ The closed-id window holds 4096 ids, 64x the entry window, and it is a memory sa A watermark over issued ids was rejected: the daemon mints `commandId` as a random UUID per request, reuses one id across retries and across runner generations, and persists nothing, so no ordering exists to compare. Eviction on a daemon resend deadline was rejected: the daemon holds no per-id "never resend after this instant" value — its resend budgets are attempt counts and per-exchange timeouts redrawn per attempt, and a restart-and-replay can legitimately post the same id minutes later — so a deadline-based rule would need a wire-contract change this change does not make. -`CommandTraits.isReadOnly` is declared by `appRead`, `runnerCaptureRead`, and `presentedSurfaceQuery`. It is independent of `retryOnSessionLoss` and response retention. When a read entry finishes, `accept` claims it again instead of answering from its recorded outcome, even if that outcome was a failure. Pruning a read leaves no closed-id marker. Reads therefore never spend their ids, and a lost-response resend of `snapshot` captures the screen again. Admission uses the recorded entry's trait, so renaming a spent mutation to a read cannot bypass its marker or retained outcome. +`CommandTraits.isReadOnly` is declared by `appRead`, `runnerCaptureRead`, `presentedSurfaceQuery`, and `selectorResolution`. It is independent of `retryOnSessionLoss` and response retention. When a read entry finishes, `accept` claims it again instead of answering from its recorded outcome, even if that outcome was a failure. Pruning a read leaves no closed-id marker. Reads therefore never spend their ids, and a lost-response resend of `snapshot` captures the screen again. Admission uses the recorded entry's trait, so renaming a spent mutation to a read cannot bypass its marker or retained outcome. Rejected: keeping the transport's own in-flight id set beside the journal. Two records of one lifecycle drifted: the set was cleared when the answer was delivered while the journal still held the entry, so a repeat arriving in that window was admitted and executed again. From 8928680ea41d19bcd55b86a40d07f2a29588d7c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 22:50:39 +0100 Subject: [PATCH 12/17] test(ios): share transport listener and client lifetimes --- .../RunnerTests+TransportTests.swift | 85 ++++++++++--------- 1 file changed, 43 insertions(+), 42 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index 0708eba31f..c34e894584 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -736,24 +736,21 @@ extension RunnerTests { func testTransportReplaysPasteboardWriteWithoutWritingAgain() throws { let originalItems = UIPasteboard.general.items defer { UIPasteboard.general.items = originalItems } - let listener = try startTransportListener() - defer { listener.cancel() } - let port = try XCTUnwrap(listener.port) - let client = URLSession(configuration: .ephemeral) - defer { client.invalidateAndCancel() } - let first = try transportHttpResponse( - client: client, port: port, - payload: #"{"command":"pasteboardWrite","commandId":"pasteboard-twice","text":"journal-first-write"}"# - ) - XCTAssertTrue(first.ok, String(describing: first.error)) - XCTAssertEqual(UIPasteboard.general.string, "journal-first-write") - let repeated = try transportHttpResponse( - client: client, port: port, - payload: #"{"command":"pasteboardWrite","commandId":"pasteboard-twice","text":"would-overwrite-on-reexecution"}"# - ) - XCTAssertTrue(repeated.ok, String(describing: repeated.error)) - XCTAssertEqual(UIPasteboard.general.string, "journal-first-write") - XCTAssertEqual(repeated.data?.runnerMainThreadBusy, false) + try withTransportClient { client, port in + let first = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"pasteboardWrite","commandId":"pasteboard-twice","text":"journal-first-write"}"# + ) + XCTAssertTrue(first.ok, String(describing: first.error)) + XCTAssertEqual(UIPasteboard.general.string, "journal-first-write") + let repeated = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"pasteboardWrite","commandId":"pasteboard-twice","text":"would-overwrite-on-reexecution"}"# + ) + XCTAssertTrue(repeated.ok, String(describing: repeated.error)) + XCTAssertEqual(UIPasteboard.general.string, "journal-first-write") + XCTAssertEqual(repeated.data?.runnerMainThreadBusy, false) + } } #endif @@ -761,18 +758,15 @@ extension RunnerTests { func testTransportReexecutesAFinishedSnapshotWithTheSameId() throws { app.launch() defer { app.terminate() } - let listener = try startTransportListener() - defer { listener.cancel() } - let port = try XCTUnwrap(listener.port) - let client = URLSession(configuration: .ephemeral) - defer { client.invalidateAndCancel() } - let payload = #"{"command":"snapshot","commandId":"snapshot-twice","interactiveOnly":true}"# - - for _ in 0..<2 { - let reply = try transportHttpResponse(client: client, port: port, payload: payload) - XCTAssertTrue(reply.ok, String(describing: reply.error)) - let nodes = try XCTUnwrap(reply.data?.nodes) - XCTAssertFalse(nodes.isEmpty) + try withTransportClient { client, port in + let payload = #"{"command":"snapshot","commandId":"snapshot-twice","interactiveOnly":true}"# + + for _ in 0..<2 { + let reply = try transportHttpResponse(client: client, port: port, payload: payload) + XCTAssertTrue(reply.ok, String(describing: reply.error)) + let nodes = try XCTUnwrap(reply.data?.nodes) + XCTAssertFalse(nodes.isEmpty) + } } } @@ -780,23 +774,30 @@ extension RunnerTests { func testTransportReexecutesASelectorReadWithTheSameId() throws { app.launch() defer { app.terminate() } + try withTransportClient { client, port in + let first = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"Agent Device Runner"}"# + ) + XCTAssertTrue(first.ok, String(describing: first.error)) + XCTAssertEqual(first.data?.found, true) + let repeated = try transportHttpResponse( + client: client, port: port, + payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"missing-journal-selector"}"# + ) + XCTAssertTrue(repeated.ok, String(describing: repeated.error)) + XCTAssertEqual(repeated.data?.found, false, "a finished selector read must execute instead of replaying its retained answer") + } + } + + @MainActor + private func withTransportClient(_ body: (URLSession, NWEndpoint.Port) throws -> Void) throws { let listener = try startTransportListener() defer { listener.cancel() } let port = try XCTUnwrap(listener.port) let client = URLSession(configuration: .ephemeral) defer { client.invalidateAndCancel() } - let first = try transportHttpResponse( - client: client, port: port, - payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"Agent Device Runner"}"# - ) - XCTAssertTrue(first.ok, String(describing: first.error)) - XCTAssertEqual(first.data?.found, true) - let repeated = try transportHttpResponse( - client: client, port: port, - payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"missing-journal-selector"}"# - ) - XCTAssertTrue(repeated.ok, String(describing: repeated.error)) - XCTAssertEqual(repeated.data?.found, false, "a finished selector read must execute instead of replaying its retained answer") + try body(client, port) } private func transportCommand(_ command: String, id: String) throws -> Command { From 66746032f4baa0c10d010646681f3a099f159fe8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 23:07:10 +0100 Subject: [PATCH 13/17] fix(ios): require explicit read traits and document runner refusals --- .changeset/ios-runner-journal-admission.md | 5 +++++ .../RunnerTests+Models.swift | 15 +++++++++------ .../AgentDeviceRunnerUITests/RunnerTests.swift | 15 +-------------- .../__tests__/runner-error-classification.test.ts | 5 +++++ .../__tests__/runner-swift-settlement-fixtures.ts | 8 ++++++++ 5 files changed, 28 insertions(+), 20 deletions(-) create mode 100644 .changeset/ios-runner-journal-admission.md diff --git a/.changeset/ios-runner-journal-admission.md b/.changeset/ios-runner-journal-admission.md new file mode 100644 index 0000000000..b44a511130 --- /dev/null +++ b/.changeset/ios-runner-journal-admission.md @@ -0,0 +1,5 @@ +--- +'agent-device': patch +--- + +Prevent repeated Apple runner interactions with the same command ID from executing twice while allowing read-only retries, including selector queries, to read again. If a repeat cannot replay the finished command's result, return `COMMAND_ALREADY_FINISHED` with `error.details.dispatched: unknown`. Run `snapshot -i` to inspect the screen before deciding whether to send another interaction. diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift index d59c982eed..0ec529dc58 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift @@ -117,7 +117,7 @@ struct CommandTraits { launchPolicy: CommandLaunchPolicy, convertsRecordedFailure: Bool = false, retainsJournalResponseJson: Bool, - isReadOnly: Bool = false + isReadOnly: Bool ) { self.isInteraction = isInteraction self.retryOnSessionLoss = retryOnSessionLoss @@ -135,7 +135,8 @@ fileprivate extension CommandTraits { isInteraction: true, launchPolicy: .mayLaunch, convertsRecordedFailure: true, - retainsJournalResponseJson: true + retainsJournalResponseJson: true, + isReadOnly: false ) /// Mutations the runner performs without the element-interaction preflight. NOTE: `mouseClick` @@ -143,7 +144,8 @@ fileprivate extension CommandTraits { /// bespoke macOS activation, so classifying it needs a macOS smoke check first (tracked as a /// follow-up). static let appMutation = CommandTraits( - launchPolicy: .mayLaunch, convertsRecordedFailure: true, retainsJournalResponseJson: true + launchPolicy: .mayLaunch, convertsRecordedFailure: true, retainsJournalResponseJson: true, + isReadOnly: false ) /// Reads of the session app: replayable after session invalidation, and refused rather than @@ -178,12 +180,12 @@ fileprivate extension CommandTraits { } /// The runner's own lifecycle: no session app is brought forward, and no mutation is proven. - static let runnerLifecycle = CommandTraits(launchPolicy: .noApp, retainsJournalResponseJson: true) + static let runnerLifecycle = CommandTraits(launchPolicy: .noApp, retainsJournalResponseJson: true, isReadOnly: false) /// Device state the runner sets from its own process, such as the pasteboard: no app is brought /// forward, since the state belongs to the device rather than to the session app, and no UI /// mutation is proven. - static let deviceState = CommandTraits(launchPolicy: .noApp, retainsJournalResponseJson: true) + static let deviceState = CommandTraits(launchPolicy: .noApp, retainsJournalResponseJson: true, isReadOnly: false) /// Commands hosted by the surface that already has focus, which no activation may cancel. A /// hardware press belongs to the system rather than to the session app, and an alert answers from @@ -191,7 +193,8 @@ fileprivate extension CommandTraits { static let presentedSurfaceMutation = CommandTraits( launchPolicy: .presentedSurface, convertsRecordedFailure: true, - retainsJournalResponseJson: true + retainsJournalResponseJson: true, + isReadOnly: false ) /// `alert get` changes nothing, so it is the one alert action that may be replayed. diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift index a329db5454..1a2709a017 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -51,26 +51,13 @@ final class RunnerTests: XCTestCase { static let commandAlreadyFinished = "COMMAND_ALREADY_FINISHED" } - /// The wire code a thrown command error travels under: the execution-watchdog timeout keeps its own - /// typed code so the daemon records the runner as main-thread-occupied from the stalling command - /// itself (#2552), and every other throw stays the generic `COMMAND_FAILED`. The transport encodes a - /// live failure with this, and the journal records a thrown failure with it, so a repeat answered from - /// the journal's record carries the same code as the answer the caller lost. - static func wireErrorCode(for error: Error) -> String { - let nsError = error as NSError - return nsError.domain == RunnerErrorDomain.general - && nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut - ? RunnerWireErrorCode.mainThreadTimeout - : "COMMAND_FAILED" - } - /// A replayed failure must carry the same code and recovery hint as its live reply. static func thrownErrorPayload(for error: Error) -> ErrorPayload { let nsError = error as NSError let abandoned = nsError.domain == RunnerErrorDomain.general && nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut return ErrorPayload( - code: wireErrorCode(for: error), + code: abandoned ? RunnerWireErrorCode.mainThreadTimeout : "COMMAND_FAILED", message: error.localizedDescription, hint: abandoned ? "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." diff --git a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts index e962af1e46..978a28bec3 100644 --- a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts +++ b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts @@ -522,6 +522,11 @@ test('the runner writes the occupancy code the daemon reads, on the live reply a MAIN_THREAD_TIMEOUT_RUNNER_CODE, 'the runner must encode the watchdog timeout under the code the daemon keys occupancy on', ); + assert.equal( + swift.finishedCode, + COMMAND_ALREADY_FINISHED_RUNNER_CODE, + 'the runner must encode its finished-id refusal under the code the daemon reads as silent', + ); assert.equal( swift.journalRecordsDerivedCode, true, diff --git a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts index 1d2928d99c..a65af465cb 100644 --- a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts +++ b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts @@ -162,6 +162,8 @@ function readSwiftWireErrorCodes(): Record { export function requireSwiftThrownErrorWireCode(): { /** The Swift member whose value the daemon's occupancy verdict keys on. */ timeoutCode: string; + /** The bodyless journal refusal whose occupancy report must stay silent. */ + finishedCode: string; /** Whether the journal's failure record derives its error body from the same owner the transport uses. */ journalRecordsDerivedCode: boolean; } { @@ -172,6 +174,11 @@ export function requireSwiftThrownErrorWireCode(): { 'RunnerWireErrorCode must declare mainThreadTimeout: the daemon reads this code to record the ' + "runner's main thread as still draining, on the live reply and on a journal replay alike.", ); + const finishedCode = codes['commandAlreadyFinished']; + assert.ok( + finishedCode !== undefined, + 'RunnerWireErrorCode must declare commandAlreadyFinished: the daemon reads this code as silent about main-thread occupancy.', + ); const journal = fs.readFileSync(SWIFT_JOURNAL, 'utf8'); const recorded = journal.match( /func fail\(command: Command, error: Error\)[\s\S]*?\n {2}\}/, @@ -200,6 +207,7 @@ export function requireSwiftThrownErrorWireCode(): { ); return { timeoutCode, + finishedCode, journalRecordsDerivedCode: owner.test(recorded), }; } From a108312d270502bca7ed757635e947f036a4fcd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 23:22:02 +0100 Subject: [PATCH 14/17] test(ios): assert every command read-only trait --- .../UnitTests/RunnerTests+ModelsTests.swift | 92 ++++++++++--------- 1 file changed, 48 insertions(+), 44 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift index 59cf06ba41..01934336ee 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift @@ -154,6 +154,7 @@ extension RunnerTests { let launchPolicy: CommandLaunchPolicy let convertsRecordedFailure: Bool let retainsJournalResponseJson: Bool + let isReadOnly: Bool } private func expectation( @@ -161,14 +162,16 @@ extension RunnerTests { retry: Bool, launch: CommandLaunchPolicy, converts: Bool, - retains: Bool + retains: Bool, + isReadOnly: Bool ) -> ExpectedTraits { ExpectedTraits( isInteraction: interaction, retryOnSessionLoss: retry, launchPolicy: launch, convertsRecordedFailure: converts, - retainsJournalResponseJson: retains + retainsJournalResponseJson: retains, + isReadOnly: isReadOnly ) } @@ -194,6 +197,7 @@ extension RunnerTests { expectation.retainsJournalResponseJson, "\(request) retainsJournalResponseJson" ) + XCTAssertEqual(traits.isReadOnly, expectation.isReadOnly, "\(request) isReadOnly") } /// The commands the merge-base classified read-only, copied from its `CommandType.traits` @@ -254,62 +258,62 @@ extension RunnerTests { /// a concrete launch case, so re-pointing a command at another policy fails that row. func testEveryCommandDeclaresEveryRunnerSideDecisionTogether() throws { let table: [(CommandType, ExpectedTraits)] = [ - (.tap, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.mouseClick, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.longPress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.drag, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.remotePress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.type, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.swipe, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.scroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.desktopScroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.findText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true)), + (.tap, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.mouseClick, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.longPress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.drag, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.remotePress, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.type, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.swipe, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.scroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.desktopScroll, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.findText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true, isReadOnly: true)), ( .querySelector, - expectation(interaction: false, retry: false, launch: .existingApp, converts: true, retains: true) + expectation(interaction: false, retry: false, launch: .existingApp, converts: true, retains: true, isReadOnly: true) ), - (.readText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true)), - (.snapshot, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: false)), - (.screenshot, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: false)), - (.backInApp, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.backSystem, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.home, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.rotate, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.appSwitcher, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), + (.readText, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true, isReadOnly: true)), + (.snapshot, expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: false, isReadOnly: true)), + (.screenshot, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: false, isReadOnly: true)), + (.backInApp, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.backSystem, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.home, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.rotate, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.appSwitcher, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), ( .actionButton, - expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true) + expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true, isReadOnly: false) ), ( .volumeUp, - expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true) + expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true, isReadOnly: false) ), ( .volumeDown, - expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true) + expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true, isReadOnly: false) ), - (.keyboardDismiss, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.keyboardReturn, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), + (.keyboardDismiss, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.keyboardReturn, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), ( .alert, - expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true) + expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true, isReadOnly: true) ), - (.sequence, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.gesture, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true)), + (.sequence, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.gesture, expectation(interaction: true, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), ( .gestureViewport, - expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true) + expectation(interaction: false, retry: true, launch: .existingApp, converts: false, retains: true, isReadOnly: true) ), - (.recordStart, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.recordStop, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)), - (.status, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: true)), - (.uptime, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)), - (.appState, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: true)), - (.pasteboardWrite, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)), - (.activate, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true)), - (.terminate, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)), - (.targetReset, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)), - (.shutdown, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true)) + (.recordStart, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.recordStop, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true, isReadOnly: false)), + (.status, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: true, isReadOnly: true)), + (.uptime, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true, isReadOnly: false)), + (.appState, expectation(interaction: false, retry: true, launch: .noApp, converts: false, retains: true, isReadOnly: true)), + (.pasteboardWrite, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true, isReadOnly: false)), + (.activate, expectation(interaction: false, retry: false, launch: .mayLaunch, converts: true, retains: true, isReadOnly: false)), + (.terminate, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true, isReadOnly: false)), + (.targetReset, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true, isReadOnly: false)), + (.shutdown, expectation(interaction: false, retry: false, launch: .noApp, converts: false, retains: true, isReadOnly: false)) ] for (type, rowExpectation) in table { let request = #"{"command":"\#(type.rawValue)"}"# @@ -332,15 +336,15 @@ extension RunnerTests { // The one payload-dependent command settles each fact per action: `get` changes nothing and may // be replayed, while `accept` and `dismiss` mutate and must not be. let alertCases: [(action: String?, expectation: ExpectedTraits)] = [ - (nil, expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true)), - ("get", expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true)), + (nil, expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true, isReadOnly: true)), + ("get", expectation(interaction: false, retry: true, launch: .presentedSurface, converts: false, retains: true, isReadOnly: true)), ( "accept", - expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true) + expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true, isReadOnly: false) ), ( "dismiss", - expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true) + expectation(interaction: false, retry: false, launch: .presentedSurface, converts: true, retains: true, isReadOnly: false) ) ] for alertCase in alertCases { From 099c30cfebfe1e9fb3d82f2444e2280b3e9e2f1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 23:50:56 +0100 Subject: [PATCH 15/17] test(ios): observe selector read admission without host labels --- .../RunnerTests+TransportTests.swift | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index c34e894584..8ed8832554 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -775,18 +775,45 @@ extension RunnerTests { app.launch() defer { app.terminate() } try withTransportClient { client, port in + let selector = "missing-journal-selector-\(UUID().uuidString)" + let payload = #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"\#(selector)"}"# let first = try transportHttpResponse( - client: client, port: port, - payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"Agent Device Runner"}"# + client: client, port: port, payload: payload ) XCTAssertTrue(first.ok, String(describing: first.error)) - XCTAssertEqual(first.data?.found, true) - let repeated = try transportHttpResponse( - client: client, port: port, - payload: #"{"command":"querySelector","commandId":"selector-twice","selectorKey":"label","selectorValue":"missing-journal-selector"}"# + XCTAssertEqual(first.data?.found, false) + XCTAssertEqual( + commandJournal.status(normalizedCommandId: "selector-twice").lifecycleState, + RunnerCommandLifecycleState.completed.rawValue + ) + + let queueOccupied = expectation(description: "command queue occupied before selector resend") + let releaseQueue = DispatchSemaphore(value: 0) + defer { releaseQueue.signal() } + commandExecutionQueue.async { + queueOccupied.fulfill() + releaseQueue.wait() + } + wait(for: [queueOccupied], timeout: 2) + + let repeatSend = transportHttpRequest(client: client, port: port, payload: payload) + let readmitted = XCTNSPredicateExpectation( + predicate: NSPredicate { _, _ in + self.commandJournal.status(normalizedCommandId: "selector-twice").lifecycleState + == RunnerCommandLifecycleState.accepted.rawValue + }, + object: nil ) + wait(for: [readmitted], timeout: 2) + releaseQueue.signal() + wait(for: [repeatSend.finished], timeout: 2) + let repeated = try repeatSend.response() XCTAssertTrue(repeated.ok, String(describing: repeated.error)) - XCTAssertEqual(repeated.data?.found, false, "a finished selector read must execute instead of replaying its retained answer") + XCTAssertEqual(repeated.data?.found, false) + XCTAssertEqual( + commandJournal.status(normalizedCommandId: "selector-twice").lifecycleState, + RunnerCommandLifecycleState.completed.rawValue + ) } } From b57eae73984a01b4dfb8f4164c6abd58275c9544 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 2 Nov 2026 07:50:06 +0100 Subject: [PATCH 16/17] refactor(ios): leave shutdown finish with the original delivery --- .../RunnerTests+CommandJournal.swift | 33 +-- .../RunnerTests+Transport.swift | 13 +- .../RunnerTests+CommandJournalTests.swift | 80 +----- .../UnitTests/RunnerTests+ModelsTests.swift | 43 +++- .../RunnerTests+TransportTests.swift | 97 +------ .../fixtures/runner-command-read-only.json | 239 ++++++++++++++++++ .../0005-ios-runner-interaction-lifecycle.md | 2 +- .../__tests__/runner-command-traits.test.ts | 25 +- .../runner-error-classification.test.ts | 17 +- .../runner-swift-settlement-fixtures.ts | 38 +-- 10 files changed, 320 insertions(+), 267 deletions(-) create mode 100644 contracts/fixtures/runner-command-read-only.json diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift index e93d121b37..53b50f0295 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift @@ -30,11 +30,7 @@ struct RunnerCommandJournalEntry { /// on the device and its terminal entry must not close the id: the read-only busy resend carries the /// same `commandId` (ADR 0005). var dispatched = false - /// Which execution of this `commandId` the record belongs to, minted when the id was claimed. The - /// transport answers joiners from it, so a delivery reaches only the sends that joined the execution that - /// closed. It has to outlive the entry's close: an id the runner refused before dispatch is claimed - /// again, and the sends of that new execution must not be answered by the delivery of the refusal that - /// freed the id. + /// Distinguishes joiners when the same id is claimed again before an earlier delivery finishes. let executionToken: Int let isReadOnly: Bool } @@ -48,12 +44,8 @@ enum RunnerCommandAdmission { /// execution, the one `executionToken` names. `accept(command:registeringJoin:)` has already /// registered the send it gave this verdict. case join(executionToken: Int) - /// Non-read execution finished after dispatch: the caller answers from the journal and - /// never executes this command again. `answersShutdown` says whether the answer being handed over is - /// the runner's own successful `shutdown` answer, which the caller owes the runner's end. Neither the - /// repeat's request nor the command name alone can answer that: a caller may reuse a spent `commandId` - /// under another command name, and the live path withholds the finish from a `shutdown` that threw. - case terminal(outcome: Response, answersShutdown: Bool) + /// Replays the recorded outcome without executing the command. + case terminal(outcome: Response) } /// The runner's one command-lifecycle record: what it holds for a `commandId`, whether that id may run, @@ -107,8 +99,7 @@ final class RunnerCommandJournal { // The record that named this id is gone, so all this refusal can promise is that the id is // spent — it is no command's answer, and so never the runner's own. return .terminal( - outcome: alreadyFinishedRefusal(command: command, commandId: commandId, outcomeDropped: true), - answersShutdown: false + outcome: alreadyFinishedRefusal(command: command, commandId: commandId, outcomeDropped: true) ) } return claim(command: command, commandId: commandId) @@ -124,14 +115,7 @@ final class RunnerCommandJournal { guard entry.dispatched, !entry.isReadOnly else { return claim(command: command, commandId: commandId) } - let outcome = recordedOutcome(for: entry) - // Only a recorded `shutdown` that succeeded ends the runner. The live path withholds that finish from - // a `shutdown` that threw, so replaying its refusal must not stop a runner whose caller is still - // holding it for another command. - return .terminal( - outcome: outcome, - answersShutdown: entry.command == CommandType.shutdown.rawValue && outcome.ok - ) + return .terminal(outcome: recordedOutcome(for: entry)) } /// Callers hold the journal's lock, including for commands without an id. @@ -259,11 +243,6 @@ final class RunnerCommandJournal { // Only dispatched non-read commands spend their ids. Reads and pre-dispatch refusals leave // no marker, so resends can execute after the outcome is pruned (ADR 0005). if let entry = entries[removable], entry.dispatched, !entry.isReadOnly { - // The two closed-id structures stay one-to-one, and nothing here needs a duplicate check to keep - // them that way: an id enters `closedCommandIds` only in this branch, and an id whose entry this - // journal no longer holds can be claimed again only once the marker is gone — which the valve - // below does by removing the id from both structures together. So a second close of a released id - // finds an id in neither structure, and one still marked can never have an entry to prune. closedCommandIds.insert(removable) closedOrder.append(removable) while closedOrder.count > Self.closedCommandIdSafetyValve { @@ -312,8 +291,6 @@ final class RunnerCommandJournal { commandName: String, outcomeDropped: Bool ) -> Response { - // Two whole messages rather than one sentence spliced into another: which record is gone is the - // fact the caller acts on, so each variant has to read as a sentence on its own. let reason = outcomeDropped ? "The runner already finished \(commandName) with id \(commandId). That command's recorded outcome has since been dropped from the journal, so there is no response to replay." diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index e531d4b4b8..403a92e616 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -225,21 +225,14 @@ extension RunnerTests { command.command.rawValue, command.commandId ?? "" ) - case .terminal(let outcome, let answersShutdown): - // Which answer ends the runner is the journal's fact, not this request's: a caller may reuse a - // spent `commandId` under another command name, and what it receives is still the recorded - // command's outcome. `finish()` guards its own expectation against being fulfilled twice, so a - // shutdown that several sends receive is harmless. - // `jsonResponse` stamps the outcome it is given, so a replayed success reports the main thread's - // occupancy NOW rather than the occupancy the recorded answer was written under — the stored body - // is kept unstamped for exactly this reason. That is what lets a replayed answer settle a stale - // busy report on the daemon side; the journal itself observed nothing. + case .terminal(let outcome): + // Replayed successes need current occupancy, so stamp them after reading the stored outcome. NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ALREADY_FINISHED command=%@ commandId=%@", command.command.rawValue, command.commandId ?? "" ) - completion((jsonResponse(status: 200, response: outcome), answersShutdown)) + completion((jsonResponse(status: 200, response: outcome), false)) case .execute(let executionToken): NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ACCEPTED command=%@ commandId=%@", diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 3aa8cb53a4..ac9892a664 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -291,7 +291,7 @@ extension RunnerTests { runnerJournalCloseDispatchedCommand(journal, tap, message: "tapped") runnerJournalFinishOtherCommands(journal, 64) - guard case .terminal(let outcome, _) = journal.accept(command: tap) else { + guard case .terminal(let outcome) = journal.accept(command: tap) else { return XCTFail("a pruned id whose work reached the device must never be admitted again") } XCTAssertFalse(outcome.ok) @@ -321,7 +321,7 @@ extension RunnerTests { idPrefix: "window", dispatched: true ) - guard case .terminal(let refusal, _) = journal.accept(command: tap) else { + guard case .terminal(let refusal) = journal.accept(command: tap) else { return XCTFail("a spent id must stay refused for the whole closed-id window") } XCTAssertEqual( @@ -336,7 +336,7 @@ extension RunnerTests { runnerJournalFinishOtherCommands(journal, 1, idPrefix: "valve", dispatched: true) let nextOldest = runnerJournalCommand("recordStop", id: "window-0") - guard case .terminal(let stillRefused, _) = journal.accept(command: nextOldest) else { + guard case .terminal(let stillRefused) = journal.accept(command: nextOldest) else { return XCTFail("the valve must release the oldest closed id only, not the next-oldest") } XCTAssertEqual( @@ -411,80 +411,16 @@ extension RunnerTests { XCTAssertEqual(served.data?.message, "recording already stopped") } - /// Whether `accept` reports that the answer it hands over is the runner's own `shutdown` answer. - private func journalAnswersShutdown( - _ journal: RunnerCommandJournal, - command: Command - ) -> Bool { - switch journal.accept(command: command) { - case .terminal(_, let answersShutdown): - return answersShutdown - case .execute: - XCTFail("a finished, dispatched commandId must be answered from the journal") - return false - case .join: - XCTFail("this test leaves no execution in flight") - return false - } - } - - /// Which answer ends the runner is the journal's fact about the record, not the repeat's: a caller may - /// reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded - /// command's. Naming `shutdown` in the repeat must not end the runner on some other command's answer, - /// and naming another command must not withhold the end that a recorded `shutdown` answer owes. - func testJournalDecidesFromTheRecordWhetherAReplayEndsTheRunner() throws { + func testJournalDoesNotReadmitASpentMutationRenamedAsARead() { let journal = RunnerCommandJournal() - let tap = runnerJournalCommand("tap", id: "spent-then-named-shutdown") + let tap = runnerJournalCommand("tap", id: "spent-mutation") journal.accept(command: tap) journal.markDispatched(command: tap) journal.finish(command: tap, response: Response(ok: true, data: DataPayload(message: "tapped"))) - - guard case .terminal = journal.accept(command: runnerJournalCommand("snapshot", id: "spent-then-named-shutdown")) else { + guard case .terminal(let outcome) = journal.accept(command: runnerJournalCommand("snapshot", id: "spent-mutation")) else { return XCTFail("renaming a spent mutation to a read must not bypass admission") } - - let shutdown = runnerJournalCommand("shutdown", id: "spent-then-named-shutdown") - XCTAssertEqual( - journalAnswersShutdown(journal, command: shutdown), - false, - "a recorded tap answer must not end the runner because the repeat named shutdown" - ) - - let shutdownId = "shutdown-then-named-tap" - let realShutdown = runnerJournalCommand("shutdown", id: shutdownId) - journal.accept(command: realShutdown) - journal.markDispatched(command: realShutdown) - journal.finish( - command: realShutdown, - response: Response(ok: true, data: DataPayload(message: "shutdown")) - ) - XCTAssertEqual( - journalAnswersShutdown(journal, command: runnerJournalCommand("tap", id: shutdownId)), - true, - "a recorded shutdown answer must still end the runner when the repeat names another command" - ) - } - - /// The live path withholds the finish from a `shutdown` that threw, so a replay of that refusal has to - /// withhold it too. A refused shutdown means the runner is still serving for its caller — ending it here - /// would tear down the runner the resend is still talking to, on an answer that said it stayed up. - func testJournalWithholdsTheFinishFromAReplayedFailedShutdown() throws { - let journal = RunnerCommandJournal() - let command = runnerJournalCommand("shutdown", id: "refused-shutdown") - journal.accept(command: command) - journal.markDispatched(command: command) - journal.fail(command: command, error: Self.mainThreadExecutionTimeoutError()) - - let replayed = journal.accept(command: command) - guard case .terminal(let outcome, let answersShutdown) = replayed else { - return XCTFail("a refused, dispatched shutdown must be answered from the journal") - } - XCTAssertEqual(outcome.ok, false, "the recorded refusal is what the repeat is answered with") - XCTAssertEqual( - answersShutdown, - false, - "a recorded shutdown refusal must not end the runner the caller still holds" - ) + XCTAssertEqual(outcome.data?.message, "tapped") } /// The recorded failure has to carry the same code the live answer carried, because the daemon reads @@ -505,7 +441,7 @@ extension RunnerTests { nil ) - guard case .terminal(let replay, _) = journal.accept(command: command) else { + guard case .terminal(let replay) = journal.accept(command: command) else { return XCTFail("a repeat of a finished, dispatched commandId is answered from the journal") } diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift index 01934336ee..7c603af668 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift @@ -48,6 +48,27 @@ extension RunnerTests { XCTAssertEqual(orphaned, [], "CommandType cases with no production request") } + func testQueuedReadOnlyTraitsMatchTheDaemonParityFixture() throws { + let rows = try XCTUnwrap( + runnerContractFixture("runner-command-read-only.json") as? [[String: Any]] + ) + var covered = Set() + for row in rows { + let request = try XCTUnwrap(row["command"] as? [String: Any]) + let name = String(describing: request) + let command = try decodeProductionRunnerRequest(request, name) + let readOnly = try XCTUnwrap(row["readOnly"] as? Bool, name) + XCTAssertNil(inlineResponse(for: command), name) + XCTAssertEqual(command.traits.isReadOnly, readOnly, name) + covered.insert(command.command) + } + let queued = try Set(CommandType.allCases.filter { type in + let command = try runnerCommandFixture(#"{"command":"\#(type.rawValue)"}"#) + return inlineResponse(for: command) == nil + }) + XCTAssertEqual(covered, queued, "every queued CommandType needs parity coverage") + } + func testEveryRunnerRequestFieldHasAProductionRequest() throws { let entries = try productionRunnerRequests() let requests = entries.map(\.request) @@ -81,6 +102,16 @@ extension RunnerTests { } private func productionRunnerRequests() throws -> [(name: String, request: [String: Any])] { + let entries = try XCTUnwrap(runnerContractFixture("runner-requests.json") as? [[String: Any]]) + return try entries.map { entry in + ( + name: try XCTUnwrap(entry["name"] as? String), + request: try XCTUnwrap(entry["request"] as? [String: Any], "\(entry["name"] ?? "?")") + ) + } + } + + private func runnerContractFixture(_ name: String) throws -> Any { let fixtureURL = URL(fileURLWithPath: #filePath) .deletingLastPathComponent() .deletingLastPathComponent() @@ -88,16 +119,8 @@ extension RunnerTests { .deletingLastPathComponent() .deletingLastPathComponent() .deletingLastPathComponent() - .appendingPathComponent("contracts/fixtures/runner-requests.json") - let entries = try XCTUnwrap( - JSONSerialization.jsonObject(with: Data(contentsOf: fixtureURL)) as? [[String: Any]] - ) - return try entries.map { entry in - ( - name: try XCTUnwrap(entry["name"] as? String), - request: try XCTUnwrap(entry["request"] as? [String: Any], "\(entry["name"] ?? "?")") - ) - } + .appendingPathComponent("contracts/fixtures/\(name)") + return try JSONSerialization.jsonObject(with: Data(contentsOf: fixtureURL)) } private func decodeProductionRunnerRequest(_ request: [String: Any], _ name: String) throws diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index 8ed8832554..eb5a90a89f 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift @@ -476,7 +476,7 @@ extension RunnerTests { /// Routes one `commandId` the way the transport does: an executing send, a second send the journal /// settles onto that execution, and the close with its delivery. `outcome` decides whether the execution - /// finished or failed. Returns the bytes that delivery carried, which is what both sends must be answered + /// finished or failed. Returns the response that both sends must be answered /// with. @discardableResult private func runExecutionWithJoiner( @@ -494,11 +494,11 @@ extension RunnerTests { } guard case .execute(let claimedToken) = executed else { XCTFail("the first send of a commandId must be admitted for execution") - return Data() + return RunnerHttpResponse(status: 500, body: "") } guard case .join(let joinedToken) = joined else { XCTFail("a send of a commandId whose execution has not finished must join it") - return Data() + return RunnerHttpResponse(status: 500, body: "") } XCTAssertEqual( joinedToken, @@ -545,7 +545,7 @@ extension RunnerTests { let lateVerdict = commandJournal.accept(command: command) { _ in registeredLateSend = true } - guard case .terminal(let outcome, _) = lateVerdict else { + guard case .terminal(let outcome) = lateVerdict else { return XCTFail("a send of a finished commandId must be answered from the journal") } XCTAssertFalse(registeredLateSend, "a send answered from the journal must not be registered") @@ -621,14 +621,14 @@ extension RunnerTests { } // The refused generation delivers its own answer. It must reach nobody from the next generation. - deliverCommandResult(executionToken: firstToken, result: (Data("refusal".utf8), false)) { _ in } + deliverCommandResult(executionToken: firstToken, result: (RunnerHttpResponse(status: 200, body: "refusal"), false)) { _ in } XCTAssertNil(second.joinerData, "a refusal must not answer the execution that replaced it") commandJournal.start(command: command) commandJournal.markDispatched(command: command) commandJournal.finish(command: command, response: Response(ok: true, data: DataPayload(message: "ran"))) - deliverCommandResult(executionToken: secondToken, result: (Data("ran".utf8), false)) { _ in } - XCTAssertEqual(second.joinerData, Data("ran".utf8), "the joiner is answered by its own execution") + deliverCommandResult(executionToken: secondToken, result: (RunnerHttpResponse(status: 200, body: "ran"), false)) { _ in } + XCTAssertEqual(second.joinerData, RunnerHttpResponse(status: 200, body: "ran"), "the joiner is answered by its own execution") } /// Asserts that no send is left waiting for a delivery, whichever execution it joined. @@ -889,87 +889,6 @@ extension RunnerTests { } #endif - /// A `shutdown` answer is what ends the runner's own XCTest run, so the send that receives it has to be - /// the one that ends the runner — including the send the journal answers. A resend whose first answer - /// the daemon lost is exactly the case the journal exists to serve: if that reply is not allowed to - /// finish, the runner keeps listening with no caller left to stop it, and disposal has to wait out its - /// timeout and escalate to a signal. - func testTransportFinishesTheRunnerOnAReplayOfAFinishedShutdown() throws { - let command = try transportCommand("shutdown", id: "shutdown-answered-twice") - // The state a resend finds: this shutdown ran, was marked dispatched, and closed with its answer. - commandJournal.accept(command: command) - commandJournal.markDispatched(command: command) - commandJournal.finish( - command: command, - response: Response(ok: true, data: DataPayload(message: "shutdown")) - ) - - let listener = try startTransportListener() - defer { listener.cancel() } - let port = try XCTUnwrap(listener.port) - let client = URLSession(configuration: .ephemeral) - defer { client.invalidateAndCancel() } - let finished = expectation(description: "the replayed shutdown asked the runner to finish") - // Cleared on the way out so a run that never finished cannot leak the expectation into the next - // test; the delivery that fulfils it clears it first. - defer { doneExpectation = nil } - doneExpectation = finished - - let reply = try transportHttpResponse( - client: client, - port: port, - payload: #"{"command":"shutdown","commandId":"shutdown-answered-twice"}"# - ) - - XCTAssertEqual(reply.ok, true) - XCTAssertEqual( - reply.data?.message, - "shutdown", - "the repeat is answered from the journal rather than executed again" - ) - wait(for: [finished], timeout: 2) - } - - /// The mirror of the case above, and the reason the finish flag is the journal's fact: a caller that - /// reuses a spent `commandId` under the name `shutdown` receives the recorded answer of whatever that - /// id really ran. Ending the runner there would stop it on some other command's answer, so the resend - /// is answered and the runner keeps serving. - func testTransportDoesNotFinishOnAReplayOfAnotherCommandsRecordedAnswer() throws { - let spent = try transportCommand("recordStart", id: "spent-then-named-shutdown") - commandJournal.accept(command: spent) - commandJournal.markDispatched(command: spent) - commandJournal.finish( - command: spent, - response: Response(ok: true, data: DataPayload(message: "read")) - ) - - let listener = try startTransportListener() - defer { listener.cancel() } - let port = try XCTUnwrap(listener.port) - let client = URLSession(configuration: .ephemeral) - defer { client.invalidateAndCancel() } - // Fulfilled only by a delivery that wrongly asks the runner to finish, which is what this test - // rules out. `wait` with no expectation would not catch a finish that happens late. - let wronglyFinished = expectation(description: "a replay must not finish the runner") - wronglyFinished.isInverted = true - doneExpectation = wronglyFinished - defer { doneExpectation = nil } - - let reply = try transportHttpResponse( - client: client, - port: port, - payload: #"{"command":"shutdown","commandId":"spent-then-named-shutdown"}"# - ) - - XCTAssertEqual(reply.ok, true) - XCTAssertEqual( - reply.data?.message, - "read", - "the id's recorded answer is served, not a shutdown answer" - ) - wait(for: [wronglyFinished], timeout: 1) - } - /// Routes `command` the way the transport routes it: an inline read, then the journal's verdict on its /// `commandId`. A finished id comes back as the answer the runner recorded for it; the execute path /// rethrows what the dispatch threw, which the wire path turns into its own response. Sends that need @@ -981,7 +900,7 @@ extension RunnerTests { return response } switch commandJournal.accept(command: command) { - case .terminal(let outcome, _): + case .terminal(let outcome): return outcome case .join: throw NSError( diff --git a/contracts/fixtures/runner-command-read-only.json b/contracts/fixtures/runner-command-read-only.json new file mode 100644 index 0000000000..a49de04a93 --- /dev/null +++ b/contracts/fixtures/runner-command-read-only.json @@ -0,0 +1,239 @@ +[ + { + "command": { + "command": "actionButton" + }, + "readOnly": false + }, + { + "command": { + "command": "activate" + }, + "readOnly": false + }, + { + "command": { + "command": "alert" + }, + "readOnly": true + }, + { + "command": { + "command": "appState" + }, + "readOnly": true + }, + { + "command": { + "command": "appSwitcher" + }, + "readOnly": false + }, + { + "command": { + "command": "backInApp" + }, + "readOnly": false + }, + { + "command": { + "command": "backSystem" + }, + "readOnly": false + }, + { + "command": { + "command": "desktopScroll" + }, + "readOnly": false + }, + { + "command": { + "command": "drag" + }, + "readOnly": false + }, + { + "command": { + "command": "findText" + }, + "readOnly": true + }, + { + "command": { + "command": "gesture" + }, + "readOnly": false + }, + { + "command": { + "command": "gestureViewport" + }, + "readOnly": true + }, + { + "command": { + "command": "home" + }, + "readOnly": false + }, + { + "command": { + "command": "keyboardDismiss" + }, + "readOnly": false + }, + { + "command": { + "command": "keyboardReturn" + }, + "readOnly": false + }, + { + "command": { + "command": "longPress" + }, + "readOnly": false + }, + { + "command": { + "command": "mouseClick" + }, + "readOnly": false + }, + { + "command": { + "command": "pasteboardWrite" + }, + "readOnly": false + }, + { + "command": { + "command": "querySelector" + }, + "readOnly": true + }, + { + "command": { + "command": "readText" + }, + "readOnly": true + }, + { + "command": { + "command": "recordStart" + }, + "readOnly": false + }, + { + "command": { + "command": "recordStop" + }, + "readOnly": false + }, + { + "command": { + "command": "remotePress" + }, + "readOnly": false + }, + { + "command": { + "command": "rotate" + }, + "readOnly": false + }, + { + "command": { + "command": "screenshot" + }, + "readOnly": true + }, + { + "command": { + "command": "scroll" + }, + "readOnly": false + }, + { + "command": { + "command": "sequence" + }, + "readOnly": false + }, + { + "command": { + "command": "shutdown" + }, + "readOnly": false + }, + { + "command": { + "command": "snapshot" + }, + "readOnly": true + }, + { + "command": { + "command": "swipe" + }, + "readOnly": false + }, + { + "command": { + "command": "tap" + }, + "readOnly": false + }, + { + "command": { + "command": "targetReset" + }, + "readOnly": false + }, + { + "command": { + "command": "terminate" + }, + "readOnly": false + }, + { + "command": { + "command": "type" + }, + "readOnly": false + }, + { + "command": { + "command": "volumeDown" + }, + "readOnly": false + }, + { + "command": { + "command": "volumeUp" + }, + "readOnly": false + }, + { + "command": { + "command": "alert", + "action": "get" + }, + "readOnly": true + }, + { + "command": { + "command": "alert", + "action": "accept" + }, + "readOnly": false + }, + { + "command": { + "command": "alert", + "action": "dismiss" + }, + "readOnly": false + } +] diff --git a/docs/adr/0005-ios-runner-interaction-lifecycle.md b/docs/adr/0005-ios-runner-interaction-lifecycle.md index a4bfdeb594..486b8fb7e5 100644 --- a/docs/adr/0005-ios-runner-interaction-lifecycle.md +++ b/docs/adr/0005-ios-runner-interaction-lifecycle.md @@ -121,7 +121,7 @@ Its verdict is `execute`, `join`, or `terminal`: - `execute` claims the `commandId`, and that send owns the one execution. - `join` means an execution is claimed and unfinished. The journal registers that send as a waiter of the execution in the same locked operation, which keeps a transport retry from stacking a second execution behind the first (#1105). Settling and registering are one operation because a close between them left the send waiting on a delivery that had already gone out. Each claim carries an execution identity the journal mints, and the transport keys its waiters by it: an id refused before dispatch is claimed again, and the delivery of that refusal must not answer the sends of the execution that follows. -- `terminal` means a non-read execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. Whether that answer ends the runner is the journal's fact about the record it read, not the repeat's request: a caller may reuse a spent `commandId` under another command name, and the outcome it receives stays the recorded command's. +- `terminal` means a non-read execution ended after dispatch. The runner answers with the outcome it recorded, which may be a failure, and the command never runs again. After the last fast `RUNNER_BUSY` or `RUNNER_WEDGED` check, the journal marks dispatch. A finished non-read entry with that mark spends its `commandId`. A refusal before that point closed its entry as `failed` but dispatched nothing, so the `commandId` stays reusable and the read-only busy resend above can still run under it. diff --git a/packages/apple-runner/src/__tests__/runner-command-traits.test.ts b/packages/apple-runner/src/__tests__/runner-command-traits.test.ts index 7a984d933a..3ad217cc7c 100644 --- a/packages/apple-runner/src/__tests__/runner-command-traits.test.ts +++ b/packages/apple-runner/src/__tests__/runner-command-traits.test.ts @@ -2,6 +2,7 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import { isDeepStrictEqual } from 'node:util'; import { test } from 'vitest'; +import { readRunnerRequestFixture } from '@agent-device/test-kit'; import type { RunnerCommand } from '../runner-contract.ts'; import { isReadOnlyRunnerCommand, @@ -9,6 +10,7 @@ import { RUNNER_COMMAND_TRAITS, type RunnerCommandTraits, } from '../runner-command-traits.ts'; +import { readSwiftInlineCommands } from './runner-swift-settlement-fixtures.ts'; const RUNNER_COMMANDS = Object.keys(RUNNER_COMMAND_TRAITS) as Array; @@ -68,10 +70,6 @@ test('runner command trait table pins lifecycle-sensitive command groups', () => }); test('alert actions match the native read-only golden table', () => { - // The fixture's `query` column records whether the alert request changes anything — `get` is the - // one action that is side-effect-free — and each side consumes it under its own name: `readOnly` - // for this daemon trait, retry eligibility for the Apple runner, which no longer classifies - // commands by read-only-ness at all. const cases = JSON.parse( fs.readFileSync( new URL('../../../../contracts/fixtures/alert-command-traits.json', import.meta.url), @@ -88,6 +86,25 @@ test('alert actions match the native read-only golden table', () => { } }); +test('queued command read-only traits match the Swift parity fixture', () => { + const cases = JSON.parse( + fs.readFileSync( + new URL('../../../../contracts/fixtures/runner-command-read-only.json', import.meta.url), + 'utf8', + ), + ) as Array<{ command: RunnerCommand; readOnly: boolean }>; + const inline = new Set(readSwiftInlineCommands()); + const queued = new Set( + readRunnerRequestFixture() + .map(({ request }) => String(request.command)) + .filter((command) => !inline.has(command)), + ); + assert.deepEqual(new Set(cases.map(({ command }) => command.command)), queued); + for (const { command, readOnly } of cases) { + assert.equal(readRunnerCommandTraits(command).readOnly, readOnly, JSON.stringify(command)); + } +}); + function commandsWithTraits(traits: RunnerCommandTraits): RunnerCommand['command'][] { return RUNNER_COMMANDS.filter( (command) => diff --git a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts index 978a28bec3..7c881cc936 100644 --- a/packages/apple-runner/src/__tests__/runner-error-classification.test.ts +++ b/packages/apple-runner/src/__tests__/runner-error-classification.test.ts @@ -505,16 +505,7 @@ test('occupancy reads the busy refusal and the stalling timeout as occupied, and assert.equal(runnerMainThreadOccupancyReport(runnerReplyFailure('ELEMENT_NOT_FOUND')), 'drained'); }); -/** - * The occupancy verdict and the code the runner writes for it are one claim across two languages, so - * the Swift declaration is read here rather than restated: renaming - * `RunnerTests.RunnerWireErrorCode.mainThreadTimeout` splits the pair and fails this test. The same - * read ties the journal's failure record to that code. A repeat of a command that closed by throwing is - * answered from that record, and a record without the code reaches the daemon as an ordinary served - * refusal, which would report the main thread drained while the command that just timed out is what - * occupies it. - */ -test('the runner writes the occupancy code the daemon reads, on the live reply and the journal record', () => { +test('the runner declares the wire codes the daemon reads for occupancy', () => { const swift = requireSwiftThrownErrorWireCode(); assert.equal( @@ -527,12 +518,6 @@ test('the runner writes the occupancy code the daemon reads, on the live reply a COMMAND_ALREADY_FINISHED_RUNNER_CODE, 'the runner must encode its finished-id refusal under the code the daemon reads as silent', ); - assert.equal( - swift.journalRecordsDerivedCode, - true, - 'the journal must record a thrown failure under the same code the transport replies with, so a ' + - 'replayed failure reports the same runner condition as the answer the caller lost', - ); }); // --- runner that predates the listener token --- diff --git a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts index a65af465cb..2fae53f42d 100644 --- a/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts +++ b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts @@ -151,21 +151,12 @@ function readSwiftWireErrorCodes(): Record { return codes; } -/** - * The error body the runner derives for one thrown error. Both readers of a failed command — the - * transport encoding a live reply, and the journal recording a thrown one so a repeat can be answered - * from it — must use this one declaration, or the answer a resend receives reports a different runner - * condition, or carries less, than the answer the caller lost. The daemon keys main-thread occupancy on - * the timeout code, so the pair this ties is genuinely cross-language: it reads the Swift side rather - * than a second copy of the same string in a TypeScript test. - */ +/** The Swift wire-code declarations the daemon uses for occupancy. */ export function requireSwiftThrownErrorWireCode(): { /** The Swift member whose value the daemon's occupancy verdict keys on. */ timeoutCode: string; /** The bodyless journal refusal whose occupancy report must stay silent. */ finishedCode: string; - /** Whether the journal's failure record derives its error body from the same owner the transport uses. */ - journalRecordsDerivedCode: boolean; } { const codes = readSwiftWireErrorCodes(); const timeoutCode = codes['mainThreadTimeout']; @@ -179,35 +170,8 @@ export function requireSwiftThrownErrorWireCode(): { finishedCode !== undefined, 'RunnerWireErrorCode must declare commandAlreadyFinished: the daemon reads this code as silent about main-thread occupancy.', ); - const journal = fs.readFileSync(SWIFT_JOURNAL, 'utf8'); - const recorded = journal.match( - /func fail\(command: Command, error: Error\)[\s\S]*?\n {2}\}/, - )?.[0]; - assert.ok( - recorded !== undefined, - 'RunnerCommandJournal must declare fail(command:error:) — the record a repeated commandId is ' + - 'answered from when the command closed by throwing.', - ); - const transport = fs.readFileSync(SWIFT_TRANSPORT, 'utf8'); - const live = transport.match( - /func commandFailedResponse\(from error: Error\)[\s\S]*?\n {2}\}/, - )?.[0]; - assert.ok( - live !== undefined, - 'RunnerTests+Transport.swift must declare commandFailedResponse(from:) — the live reply a ' + - 'failed command is answered with.', - ); - // One owner for the whole error body, not just the code: a record that kept the code but dropped the - // hint would send a caller to the runner log for a condition whose live answer told it to wait. - const owner = /\bRunnerTests\.thrownErrorPayload\(for: error\)/; - assert.ok( - owner.test(live), - 'commandFailedResponse(from:) must encode the live failure with RunnerTests.thrownErrorPayload(for:) ' + - '— the one declaration that pairs the code with the hint the caller acts on.', - ); return { timeoutCode, finishedCode, - journalRecordsDerivedCode: owner.test(recorded), }; } From 39a6304ccd587bd2430565a82514665a0b5484c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 2 Nov 2026 08:40:06 +0100 Subject: [PATCH 17/17] docs(ios): name the replay occupancy stamp owner --- .../AgentDeviceRunnerUITests/RunnerTests+Transport.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift index 403a92e616..18dd1c10ca 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift @@ -226,7 +226,7 @@ extension RunnerTests { command.commandId ?? "" ) case .terminal(let outcome): - // Replayed successes need current occupancy, so stamp them after reading the stored outcome. + // jsonResponse stamps replayed successes with current occupancy. NSLog( "AGENT_DEVICE_RUNNER_COMMAND_ALREADY_FINISHED command=%@ commandId=%@", command.command.rawValue,