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+CommandDispatch.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift index e49a86bfdd..7f77331073 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandDispatch.swift @@ -189,6 +189,11 @@ 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 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) 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..53b50f0295 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,54 +25,134 @@ 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 + /// Distinguishes joiners when the same id is claimed again before an earlier delivery finishes. + let executionToken: Int + let isReadOnly: Bool +} + +/// The journal's verdict on one command that carries a `commandId`. +enum RunnerCommandAdmission { + /// 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) + /// 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, +/// and what `status` answers for it. The transport asks this type before enqueuing anything, so one +/// 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 + /// 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 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. 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] = [] + /// 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 - func accept(command: Command) { - guard let commandId = command.commandId?.trimmedNonEmpty else { return } + /// 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, + registeringJoin register: (Int) -> Void = { _ in } + ) -> RunnerCommandAdmission { lock.lock() defer { lock.unlock() } - 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() + 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 + // repeat asked for. + if closedCommandIds.contains(commandId) { + // 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) + ) + } + return claim(command: command, commandId: commandId) + } + if !entry.state.isTerminal { + let executionToken = entry.executionToken + register(executionToken) + return .join(executionToken: executionToken) + } + // 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) + } + return .terminal(outcome: recordedOutcome(for: entry)) + } + + /// Callers hold the journal's lock, including for commands without an id. + private func mintExecutionToken() -> Int { + nextExecutionToken += 1 + return nextExecutionToken } func start(command: Command) { - update(command: command, state: .started, responseOk: nil, responseJson: nil, error: nil) + transition(command: command, reorders: true) { entry in + entry.state = .started + } } + /// 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) { - update( - command: command, - state: response.ok ? .completed : .failed, - responseOk: response.ok, - responseJson: encodeResponseJson(command: command, response: response), - error: response.error - ) + 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 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) { - update( - command: command, - state: .failed, - responseOk: nil, - responseJson: nil, - error: ErrorPayload(message: error.localizedDescription) - ) + let payload = RunnerTests.thrownErrorPayload(for: error) + transition(command: command, reorders: true) { entry in + entry.state = .failed + entry.error = payload + } } func status(normalizedCommandId commandId: String) -> DataPayload { @@ -89,39 +177,133 @@ final class RunnerCommandJournal { ) } - private func update( - command: Command, - state: RunnerCommandLifecycleState, - responseOk: Bool?, - responseJson: String?, - error: ErrorPayload? - ) { - guard let commandId = command.commandId?.trimmedNonEmpty else { return } - lock.lock() - defer { lock.unlock() } - var entry = entries[commandId] ?? RunnerCommandJournalEntry( + /// 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, + isReadOnly: command.traits.isReadOnly ) - entry.state = state - entry.responseOk = responseOk - entry.responseJson = responseJson - entry.error = error + order.removeAll { $0 == commandId } + order.append(commandId) + pruneIfNeeded() + return .execute(executionToken: executionToken) + } + + /// 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, + 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 } + mutate(&entry) entries[commandId] = entry + guard reorders else { return } order.removeAll { $0 == commandId } order.append(commandId) pruneIfNeeded() } + /// 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 { - 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 } + // 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 { + 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 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()) + } + } + 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 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 { + 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: reason, + hint: + "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." + ) + ) } private func encodeResponseJson(command: Command, response: Response) -> String? { diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Models.swift index 9d73aabea5..0ec529dc58 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 ) { self.isInteraction = isInteraction self.retryOnSessionLoss = retryOnSessionLoss self.launchPolicy = launchPolicy self.convertsRecordedFailure = convertsRecordedFailure self.retainsJournalResponseJson = retainsJournalResponseJson + self.isReadOnly = isReadOnly } } @@ -131,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` @@ -139,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 @@ -147,7 +153,8 @@ fileprivate extension CommandTraits { static func appRead(retainsJournalResponseJson: Bool) -> CommandTraits { CommandTraits( retryOnSessionLoss: true, launchPolicy: .existingApp, - retainsJournalResponseJson: retainsJournalResponseJson + retainsJournalResponseJson: retainsJournalResponseJson, + isReadOnly: true ) } @@ -158,7 +165,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 @@ -166,17 +174,18 @@ fileprivate extension CommandTraits { static func runnerCaptureRead(retainsJournalResponseJson: Bool) -> CommandTraits { CommandTraits( retryOnSessionLoss: true, launchPolicy: .noApp, - retainsJournalResponseJson: retainsJournalResponseJson + retainsJournalResponseJson: retainsJournalResponseJson, + isReadOnly: true ) } /// 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 @@ -184,14 +193,16 @@ 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. 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 7deeea8b19..18dd1c10ca 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..) -> Void ) { - commandJournal.accept(command: command) commandExecutionQueue.async { completion(Result { try self.executeAccepted(command: command) }) } @@ -288,42 +314,34 @@ 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( - 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. 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 executionToken: Int, completion: @escaping @Sendable ((response: RunnerHttpResponse, shouldFinish: Bool)) -> Void - ) -> Bool { - guard let commandId = command.commandId?.trimmedNonEmpty else { return false } + ) { inFlightCommandLock.lock() - if inFlightCommandIds.contains(commandId) { - inFlightCommandWaiters[commandId, default: []].append(completion) - inFlightCommandLock.unlock() - NSLog( - "AGENT_DEVICE_RUNNER_COMMAND_COALESCED command=%@ commandId=%@", - command.command.rawValue, - commandId - ) - return true - } - inFlightCommandIds.insert(commandId) + inFlightCommandWaiters[executionToken, default: []].append(completion) inFlightCommandLock.unlock() - return false } + /// 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() - inFlightCommandIds.remove(commandId) - 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) @@ -357,30 +375,11 @@ 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 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 nsError = error as NSError - if nsError.domain == RunnerErrorDomain.general, - nsError.code == RunnerErrorCode.mainThreadExecutionTimedOut - { - return Response( - ok: false, - error: ErrorPayload( - code: RunnerWireErrorCode.mainThreadTimeout, - message: nsError.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", - 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 d152fa5cda..1a2709a017 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests.swift @@ -44,6 +44,25 @@ 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, 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" + } + + /// 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: 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." + : "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" @@ -104,11 +123,13 @@ 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 = [] - 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+CommandJournalTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift index 44555b0b76..ac9892a664 100644 --- a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift +++ b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+CommandJournalTests.swift @@ -200,6 +200,272 @@ 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. + /// `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.. 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+ModelsTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+ModelsTests.swift index 59cf06ba41..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 @@ -154,6 +177,7 @@ extension RunnerTests { let launchPolicy: CommandLaunchPolicy let convertsRecordedFailure: Bool let retainsJournalResponseJson: Bool + let isReadOnly: Bool } private func expectation( @@ -161,14 +185,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 +220,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 +281,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 +359,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 { diff --git a/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift b/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TransportTests.swift index 59ebfcca2f..eb5a90a89f 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 { @@ -361,7 +364,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) @@ -471,59 +474,452 @@ 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 response that both sends must be answered + /// with. + @discardableResult + private func runExecutionWithJoiner( + _ command: Command, + into delivered: TransportJoiner, + outcome: Response? + ) -> RunnerHttpResponse { + let executed = commandJournal.accept(command: command) + 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 + } + } + guard case .execute(let claimedToken) = executed else { + XCTFail("the first send of a commandId must be admitted for execution") + 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 RunnerHttpResponse(status: 500, body: "") + } + 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) + 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(executionToken: claimedToken, result: (deliveredBytes, false)) { result in + delivered.primaryData = result.response + } + return deliveredBytes + } + func testDuplicateCommandIdCoalescesOntoInFlightExecution() throws { - let command = try JSONDecoder().decode( - Command.self, - from: Data(#"{"command":"snapshot","commandId":"snapshot-coalesce"}"#.utf8) + // `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")) ) - final class Delivered { - var primaryData: RunnerHttpResponse? - var waiterData: RunnerHttpResponse? + + XCTAssertEqual(delivered.primaryData, delivery) + XCTAssertEqual(delivered.joinerData, delivery) + 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 + // already went out. + var registeredLateSend = false + let lateVerdict = commandJournal.accept(command: command) { _ in + registeredLateSend = true } - let delivered = Delivered() - defer { - inFlightCommandIds.removeAll() - inFlightCommandWaiters.removeAll() + 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") + assertNoJoinersLeft("a journal answer must not register a waiter") + } - XCTAssertFalse( - attachToInFlightCommandIfNeeded(command: command) { result in - delivered.primaryData = result.response - } + /// 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")) ) - XCTAssertTrue( - attachToInFlightCommandIfNeeded(command: command) { result in - delivered.waiterData = result.response + assertJournalWindowDropped("scroll-join-pruned") + + XCTAssertEqual(delivery, delivered.joinerData, "the close that pruned the entry must answer its joiner") + 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 + /// 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") + 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: (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: (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. + private func assertNoJoinersLeft(_ message: String) { + XCTAssertFalse( + inFlightCommandWaiters.values.contains { !$0.isEmpty }, + message ) + } - let result = RunnerHttpResponse(status: 200, body: "single-result") - deliverCommandResult( - command: command, - result: (result, false) - ) { result in - delivered.primaryData = result.response + /// 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 recordStop("record-stop-twice") + XCTAssertTrue(first.ok, String(describing: first.error)) + XCTAssertEqual(first.data?.message, "recording already stopped") + + 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( + 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 + ) + + // 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) + } + + #if os(iOS) + @MainActor + func testTransportReplaysPasteboardWriteWithoutWritingAgain() throws { + let originalItems = UIPasteboard.general.items + defer { UIPasteboard.general.items = originalItems } + 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 + + @MainActor + func testTransportReexecutesAFinishedSnapshotWithTheSameId() throws { + app.launch() + defer { app.terminate() } + 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) + } + } + } + + @MainActor + func testTransportReexecutesASelectorReadWithTheSameId() throws { + 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: payload + ) + XCTAssertTrue(first.ok, String(describing: first.error)) + 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) + XCTAssertEqual( + commandJournal.status(normalizedCommandId: "selector-twice").lifecycleState, + RunnerCommandLifecycleState.completed.rawValue + ) + } + } + + @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() } + try body(client, port) + } + + private func transportCommand(_ command: String, id: String) throws -> Command { + try JSONDecoder().decode( + Command.self, + from: Data(#"{"command":"\#(command)","commandId":"\#(id)"}"#.utf8) + ) + } + + /// 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":"accept","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 + + /// 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/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/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 add1b40a31..486b8fb7e5 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. +- 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. @@ -114,6 +116,34 @@ 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 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 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. +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 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. +`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 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. + +`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. + 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`. 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-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 e4e715600e..7c881cc936 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,8 @@ import assert from 'node:assert/strict'; 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, @@ -13,12 +15,14 @@ import { isRetryableRunnerError, isRunnerBusyError, resolveRunnerFatalErrorReason, + runnerMainThreadOccupancyReport, shouldRebuildCachedRunnerArtifact, shouldRestartRunnerAfterReadinessPreflight, shouldRestartRunnerBeforeCommandSend, 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; @@ -457,6 +461,65 @@ 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' } }, + }); +} + +// 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'); + // 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'); +}); + +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'); +}); + +test('the runner declares the wire codes the daemon reads for occupancy', () => { + 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.finishedCode, + COMMAND_ALREADY_FINISHED_RUNNER_CODE, + 'the runner must encode its finished-id refusal under the code the daemon reads as silent', + ); +}); + // --- 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/__tests__/runner-swift-settlement-fixtures.ts b/packages/apple-runner/src/__tests__/runner-swift-settlement-fixtures.ts index 05f2e91730..2fae53f42d 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,49 @@ 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 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; +} { + 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 finishedCode = codes['commandAlreadyFinished']; + assert.ok( + finishedCode !== undefined, + 'RunnerWireErrorCode must declare commandAlreadyFinished: the daemon reads this code as silent about main-thread occupancy.', + ); + return { + timeoutCode, + finishedCode, + }; +} 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..a063b63b05 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,35 @@ 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. 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'; } /** @@ -691,7 +714,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; } diff --git a/packages/apple-runner/src/runner-session-types.ts b/packages/apple-runner/src/runner-session-types.ts index 450e777769..7b42300270 100644 --- a/packages/apple-runner/src/runner-session-types.ts +++ b/packages/apple-runner/src/runner-session-types.ts @@ -333,8 +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 never held, - * 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([ 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