diff --git a/packages/provider-webdriver/src/webdriver-client.test.ts b/packages/provider-webdriver/src/webdriver-client.test.ts index a8bd8e627f..ed574fb9c4 100644 --- a/packages/provider-webdriver/src/webdriver-client.test.ts +++ b/packages/provider-webdriver/src/webdriver-client.test.ts @@ -106,6 +106,29 @@ test('installApp aborts an in-flight provider request when its binding is cancel } }); +// #2509: a page-source read that outlived its request was dropped by the client +// while Appium kept walking the tree server-side. Providers serialize commands +// per session, so every later request queued behind an orphan nobody was +// waiting for. Cancellation must reach `/source` the way it reaches `/install_app`. +test('source aborts an in-flight provider request when its binding is cancelled', async () => { + const controller = new AbortController(); + let requestSignal: AbortSignal | undefined; + const client = await connectedClient( + async (_input, init) => + await new Promise((_resolve, reject) => { + requestSignal = init?.signal ?? undefined; + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); + }), + ); + + const pending = client.source({ signal: controller.signal }); + await Promise.resolve(); + controller.abort(new Error('request cancelled')); + + await assert.rejects(pending, /request cancelled/); + assert.equal(requestSignal?.aborted, true); +}); + // #1774: `POST /session` is the one non-idempotent request, and a retry after a // failed create is a second billed device session. A 5xx that the transport // would retry on any other route must reach the caller unretried here. diff --git a/packages/provider-webdriver/src/webdriver-client.ts b/packages/provider-webdriver/src/webdriver-client.ts index 96973c9e0e..b9cc59d87a 100644 --- a/packages/provider-webdriver/src/webdriver-client.ts +++ b/packages/provider-webdriver/src/webdriver-client.ts @@ -244,8 +244,14 @@ export class WebDriverClient { await this.sessionRequest('POST', '/orientation', { orientation }); } - async source(): Promise { - const value = await this.sessionRequest('GET', '/source'); + /** + * The driver's whole page source. This is the provider's most expensive call and + * the only one whose duration the device's own UI decides — a screen that never + * goes idle has no reason to settle — so a caller that can be cancelled should + * hand its `signal` down rather than abandon a read still running server-side. + */ + async source(overrides?: WebDriverRequestOverrides): Promise { + const value = await this.sessionRequest('GET', '/source', undefined, overrides); if (typeof value !== 'string') { throw new AppError('COMMAND_FAILED', 'WebDriver source response was not a string', { valueType: typeof value, diff --git a/packages/provider-webdriver/src/webdriver-interactor.test.ts b/packages/provider-webdriver/src/webdriver-interactor.test.ts index d0abfc6355..5424860c04 100644 --- a/packages/provider-webdriver/src/webdriver-interactor.test.ts +++ b/packages/provider-webdriver/src/webdriver-interactor.test.ts @@ -5,6 +5,7 @@ import { AppError } from '@agent-device/kernel/errors'; import { createCloudWebDriverCapabilities } from './capabilities.ts'; import type { WebDriverClient, W3CActionSequence } from './webdriver-client.ts'; import { createWebDriverInteractor } from './webdriver-interactor.ts'; +import { isWebDriverRequestTimeout } from './webdriver-transport.ts'; // #1658: `fill` used to send its keys in the request right after the tap. A // WebView input takes first responder asynchronously, so on a web login form @@ -291,6 +292,86 @@ test('Android WebDriver interactor keeps legacy-derived source facts at its call assert.equal(source.mock.calls.length, 1); }); +// #2509: the interactor took a request-bound signal and named it away. A capture +// that ran past its budget could therefore never be cancelled: the client gave up +// while the provider kept walking the tree, and being per-session-serial it made +// every later command queue behind an orphan nobody was waiting for. +test('Android snapshot binds the provider source read to its request signal', async () => { + const controller = new AbortController(); + const forwarded: Array<{ signal?: AbortSignal } | undefined> = []; + const interactor = createWebDriverInteractor({ + client: { + source: async (overrides?: { signal?: AbortSignal }) => { + forwarded.push(overrides); + return ANDROID_ONBOARDING_SOURCE; + }, + } as unknown as WebDriverClient, + backend: 'android', + capabilities: createCloudWebDriverCapabilities({ provider: 'test', platform: 'android' }), + }); + + await interactor.snapshot({ signal: controller.signal }); + + assert.deepEqual(forwarded, [{ signal: controller.signal }]); +}); + +// The iOS acquisition adapter reads the same route, so it needs the same binding. +test('iOS snapshot binds the provider source read to its request signal', async () => { + const controller = new AbortController(); + const forwarded: Array<{ signal?: AbortSignal } | undefined> = []; + const interactor = createWebDriverInteractor({ + client: { + source: async (overrides?: { signal?: AbortSignal }) => { + forwarded.push(overrides); + return ''; + }, + } as unknown as WebDriverClient, + backend: 'xctest', + capabilities: createCloudWebDriverCapabilities({ provider: 'test', platform: 'ios' }), + targetId: 'ios-1', + }); + + await interactor.snapshot({ signal: controller.signal }); + + assert.deepEqual(forwarded, [{ signal: controller.signal }]); +}); + +// #2509 asked for an error that names the problem: a screen that never goes idle +// (looping video, live marquee) keeps the provider's tree walk from settling, and +// on rented hardware every second of it is billed. The reason code stays the +// transport's; what the capture adds is what it means. +test('a source capture that runs out of budget keeps the timeout reason and names the cause', async () => { + const interactor = createWebDriverInteractor({ + client: { source: async () => throwWebDriverSourceTimeout() } as unknown as WebDriverClient, + backend: 'android', + capabilities: createCloudWebDriverCapabilities({ provider: 'test', platform: 'android' }), + }); + + await assert.rejects(interactor.snapshot(), (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, 'webdriver_request_timeout'); + assert.equal(isWebDriverRequestTimeout(error), true); + assert.match(String(error.details?.hint), /never goes idle/); + // Reviewing #2509 found the advice that failed there was a longer `--timeout`, + // which cannot reach this read. The hint says so and offers what does work. + assert.match(String(error.details?.hint), /does not grow with --timeout/); + assert.match(String(error.details?.hint), /screenshot/); + return true; + }); +}); + +function throwWebDriverSourceTimeout(): never { + throw new AppError('COMMAND_FAILED', 'WebDriver GET /source timed out after 30000ms.', { + reason: 'webdriver_request_timeout', + method: 'GET', + path: '/source', + timeoutMs: 30_000, + }); +} + +const ANDROID_ONBOARDING_SOURCE = + ''; + async function runFill(world: ReturnType) { vi.useFakeTimers(); try { diff --git a/packages/provider-webdriver/src/webdriver-interactor.ts b/packages/provider-webdriver/src/webdriver-interactor.ts index 894a9510fb..239839c3be 100644 --- a/packages/provider-webdriver/src/webdriver-interactor.ts +++ b/packages/provider-webdriver/src/webdriver-interactor.ts @@ -22,6 +22,7 @@ import { type CloudWebDriverProviderCapabilities, } from './capabilities.ts'; import type { W3CPointerAction, WebDriverClient, WebDriverWindowRect } from './webdriver-client.ts'; +import { isWebDriverRequestTimeout } from './webdriver-transport.ts'; import { touchPointer } from './webdriver-gestures.ts'; import { scrollFrameFromAndroidWebDriverSource, @@ -303,20 +304,39 @@ class WebDriverInteractor implements Interactor { await this.client.screenshot(outPath); } - async snapshot(_options?: SnapshotOptions) { + async snapshot(options?: SnapshotOptions) { this.requireSupport('snapshot'); + return await this.captureSource(options?.signal); + } + + /** + * One page-source read, bound to the request that asked for it. Providers answer + * commands one at a time, so a read the client merely gave up on is not gone: the + * driver keeps walking the tree and every later command queues behind it (#2509). + */ + private async captureSource(signal?: AbortSignal) { + const source = await this.readSource(signal); if (this.backend === 'xctest') { - const { captureWebDriverIosSnapshot } = await import('./webdriver-ios-snapshot.ts'); - return await captureWebDriverIosSnapshot(this.client, this.targetId); + const { acquireWebDriverIosSnapshot } = await import('./webdriver-ios-snapshot.ts'); + return acquireWebDriverIosSnapshot(source, this.targetId); } const { parseWebDriverSourceFacts } = await import('./webdriver-source.ts'); return { backend: 'android' as const, producer: 'appium-source' as const, - nodes: parseWebDriverSourceFacts(await this.client.source(), 'android').nodes, + nodes: parseWebDriverSourceFacts(source, 'android').nodes, }; } + private async readSource(signal?: AbortSignal): Promise { + try { + return await this.client.source(signal === undefined ? {} : { signal }); + } catch (error) { + if (!isWebDriverRequestTimeout(error)) throw error; + throw webDriverSourceTimeoutError(error); + } + } + async back(_mode?: BackMode): Promise { this.requireSupport('back'); await this.client.back(); @@ -573,3 +593,29 @@ function webDriverOperationForGesture(plan: GesturePlan): CloudWebDriverOperatio return 'rotateGesture'; } } + +/** + * A source read that ran out of budget is the one WebDriver timeout a caller can + * do something about, and #2509 showed it reading as an unexplained hang: the + * driver answers this call by walking the live UI tree, so a screen that never + * goes idle — looping video, live ticker, continuous animation — gives the walk no + * reason to settle. The transport's reason code is kept as-is; what the capture + * adds is what the wait was waiting for, and the one thing the caller cannot do, + * since this read's budget is the transport's own and no wider than the command's + * `--timeout` envelope around it. + */ +function webDriverSourceTimeoutError(error: AppError): AppError { + return new AppError( + 'COMMAND_FAILED', + 'The cloud driver did not finish reading the screen in its budget.', + { + ...error.details, + hint: + 'A screen that never goes idle (looping video, live ticker, continuous animation) ' + + "gives the driver no moment to read the UI tree. This read has the transport's own " + + 'budget and does not grow with --timeout; take a screenshot, or drive the screen from ' + + 'refs an earlier snapshot already captured.', + }, + error, + ); +} diff --git a/packages/provider-webdriver/src/webdriver-ios-snapshot.test.ts b/packages/provider-webdriver/src/webdriver-ios-snapshot.test.ts index 144c7838ed..6169bceb28 100644 --- a/packages/provider-webdriver/src/webdriver-ios-snapshot.test.ts +++ b/packages/provider-webdriver/src/webdriver-ios-snapshot.test.ts @@ -1,9 +1,6 @@ import assert from 'node:assert/strict'; -import { test, vi } from 'vitest'; -import { - captureWebDriverIosSnapshot, - acquireWebDriverIosSnapshot, -} from './webdriver-ios-snapshot.ts'; +import { test } from 'vitest'; +import { acquireWebDriverIosSnapshot } from './webdriver-ios-snapshot.ts'; const SOURCE = ` @@ -13,9 +10,8 @@ const SOURCE = ` `; -test('Appium iOS adapter returns provider facts with explicit unavailable residue', async () => { - const source = vi.fn(async () => SOURCE); - const result = await captureWebDriverIosSnapshot({ source }, 'cloud-ios-1'); +test('Appium iOS adapter returns provider facts with explicit unavailable residue', () => { + const result = acquireWebDriverIosSnapshot(SOURCE, 'cloud-ios-1'); assert.equal(result.stage, 'acquired'); assert.equal(result.acquisition.producer, 'appium-source'); @@ -34,7 +30,6 @@ test('Appium iOS adapter returns provider facts with explicit unavailable residu { kind: 'unavailable-fact', fact: 'acquisition-depth' }, { kind: 'unavailable-fact', fact: 'truncation' }, ]); - assert.equal(source.mock.calls.length, 1); }); test('Appium iOS adapter preserves provider-reported node facts for the host presenter', () => { diff --git a/packages/provider-webdriver/src/webdriver-ios-snapshot.ts b/packages/provider-webdriver/src/webdriver-ios-snapshot.ts index 94acec4a0a..432c5a6597 100644 --- a/packages/provider-webdriver/src/webdriver-ios-snapshot.ts +++ b/packages/provider-webdriver/src/webdriver-ios-snapshot.ts @@ -3,18 +3,11 @@ import { resolveIosViewportEvidenceFromRoots, } from '@agent-device/capture-kit/ios-snapshot-acquisition'; import type { SnapshotRuntimeAcquiredResult } from '@agent-device/contracts/interactor-types'; -import type { WebDriverClient } from './webdriver-client.ts'; import { parseWebDriverSourceFacts } from './webdriver-source.ts'; const APPIUM_PRODUCER = 'appium-source' as const; -export async function captureWebDriverIosSnapshot( - client: Pick, - targetId?: string, -): Promise { - return acquireWebDriverIosSnapshot(await client.source(), targetId); -} - +/** Turns one already-read Appium page source into acquired iOS facts. */ export function acquireWebDriverIosSnapshot( source: string, targetId?: string, diff --git a/test/integration/provider-scenarios/cloud-webdriver-runtime.test.ts b/test/integration/provider-scenarios/cloud-webdriver-runtime.test.ts index 9390466034..3dd4633731 100644 --- a/test/integration/provider-scenarios/cloud-webdriver-runtime.test.ts +++ b/test/integration/provider-scenarios/cloud-webdriver-runtime.test.ts @@ -2,6 +2,11 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { test } from 'vitest'; +import { + clearRequestAbortRegistration, + markRequestCanceled, + registerRequestAbort, +} from '@agent-device/host-kit/request'; import { CLOUD_WEBDRIVER_PROVIDERS, createProviderWebDriver, @@ -29,6 +34,9 @@ import { const WEBDRIVER_PROVIDER = CLOUD_WEBDRIVER_PROVIDERS.browserStack; const CLIENT_VERSION = '0.20.3-test'; +/** One transparent pixel: enough for a driver answer to be real base64 PNG. */ +const TINY_PNG_BASE64 = + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=='; test('packaged Cloud WebDriver facade drives provider devices through daemon commands', async () => { await withProviderScenarioResource(createCloudWebDriverWorld, async (world) => { @@ -208,6 +216,82 @@ test('packaged Cloud WebDriver expiry releases the live provider session', async }); }, 15_000); +// #2509: on a screen that never goes idle — a looping onboarding video — the driver's +// page-source read outlived the request that asked for it. The client had given up, +// but nothing at the wire said so: the read stayed in flight and every command after +// it, including one that never touches the UI tree, queued behind a capture nobody was +// waiting for. A capture nobody is waiting for has to end, and the session has to stay +// usable after it. +test('packaged Cloud WebDriver cancels an abandoned source capture and keeps its session', async () => { + await withProviderScenarioResource(createCloudWebDriverWorld, async (world) => { + const { daemon, server } = world; + const requestId = 'cloud-webdriver-cancelled-source'; + const lease = await openWebDriverSession(daemon); + const registration = registerRequestAbort(requestId); + server.sourceBehavior = 'never'; + + try { + const capture = daemon.callCommand('snapshot', [], leaseFlags(lease.leaseId), { + meta: { ...leaseMeta(lease.leaseId), requestId }, + }); + await waitForSourceRequest(server); + markRequestCanceled(requestId); + + // Bounded on purpose: the bug was that this never returned at all, and a + // regression should fail here with its reason rather than as a test timeout. + const canceled = await settleWithin( + capture, + 3_000, + 'the abandoned capture was still running after its client hung up', + ); + assert.notEqual(canceled.json?.error, undefined, 'an abandoned capture must not succeed'); + assert.equal(canceled.json?.error?.data?.details?.reason, 'request_canceled'); + assert.equal( + sourceCalls(server)[0]?.signal?.aborted, + true, + 'the abandoned read must be hung up at the wire, not left held by our transport', + ); + + await withProviderScenarioTempDir('agent-device-cloud-webdriver-cancel-', async (tempDir) => { + // The command that never walks the UI tree still works on the same session — + // the whole user-visible symptom, in one assertion. + const shot = await daemon.callCommand( + 'screenshot', + [path.join(tempDir, 'after-cancel.png')], + leaseFlags(lease.leaseId), + { meta: leaseMeta(lease.leaseId) }, + ); + assertRpcOk(shot); + }); + } finally { + clearRequestAbortRegistration(registration); + } + }); +}, 20_000); + +function sourceCalls(server: FakeWebDriverServer): readonly CloudWebDriverHttpCall[] { + return server.calls.filter((call) => call.method === 'GET' && call.path.endsWith('/source')); +} + +async function settleWithin(work: Promise, budgetMs: number, failure: string): Promise { + const stillRunning = Symbol('still-running'); + const settled = await Promise.race([ + work, + new Promise((resolve) => { + setTimeout(() => resolve(stillRunning), budgetMs).unref?.(); + }), + ]); + assert.notEqual(settled, stillRunning, failure); + return settled as T; +} + +async function waitForSourceRequest(server: FakeWebDriverServer): Promise { + for (let attempt = 0; attempt < 200 && sourceCalls(server).length === 0; attempt += 1) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + assert.ok(sourceCalls(server).length > 0, 'the driver never received a page-source request'); +} + async function createCloudWebDriverWorld() { const server = await FakeWebDriverServer.start(); const providerWebDriver = createProviderWebDriver({ @@ -422,6 +506,13 @@ const FOCUSED_FIELD_RECT = { x: 0, y: 0, width: 1080, height: 1920 }; class FakeWebDriverServer extends CloudWebDriverTestServer { artifactFailuresRemaining = 0; sessionDeleteFailuresRemaining = 0; + /** + * How the driver answers `GET /source`. `never` is a driver still walking a UI + * that never goes idle: the read stays in flight until the caller's own + * cancellation reaches it, which is the only way to test what a client giving up + * actually does to the session (#2509). + */ + sourceBehavior: 'ok' | 'never' = 'ok'; /** * How the driver answers `POST /rotation`. `unsupported` is a driver that does not implement the * route; `server-error` is one that implements it and failed. Orientation must treat those @@ -451,7 +542,11 @@ class FakeWebDriverServer extends CloudWebDriverTestServer { }), 'POST /app-automate/upload': () => cloudWebDriverTestJson({ app_url: 'bs://uploaded-app' }), 'GET /wd/hub/session/wd-1/source': () => - cloudWebDriverTestJson({ value: fakeWebDriverSource() }), + this.sourceBehavior === 'never' + ? { body: null, neverAnswers: true } + : cloudWebDriverTestJson({ value: fakeWebDriverSource() }), + 'GET /wd/hub/session/wd-1/screenshot': () => + cloudWebDriverTestJson({ value: TINY_PNG_BASE64 }), 'GET /wd/hub/session/wd-1/window/rect': () => cloudWebDriverTestJson({ value: { x: 0, y: 0, width: 1080, height: 1920 } }), 'DELETE /wd/hub/session/wd-1/actions': () => diff --git a/test/integration/provider-scenarios/cloud-webdriver-test-server.ts b/test/integration/provider-scenarios/cloud-webdriver-test-server.ts index 7f72b38c54..e2b166100f 100644 --- a/test/integration/provider-scenarios/cloud-webdriver-test-server.ts +++ b/test/integration/provider-scenarios/cloud-webdriver-test-server.ts @@ -5,11 +5,20 @@ export type CloudWebDriverHttpCall = { path: string; headers: IncomingHttpHeaders; body?: unknown; + /** The caller's cancellation, so a fixture can prove it reached the wire. */ + signal?: AbortSignal; }; export type CloudWebDriverTestResponse = { body: unknown; status?: number; + /** + * A driver that never answers. The request stays in flight until the caller's own + * cancellation reaches the transport, then rejects with that caller's reason. + * Models the #2509 shape — a server-side command the client gave up on — which a + * fixture that can only answer fast or answer wrong cannot express. + */ + neverAnswers?: boolean; }; /** @@ -34,10 +43,16 @@ export abstract class CloudWebDriverTestServer { method: request.method, path: new URL(request.url).pathname, headers: Object.fromEntries(request.headers.entries()), + ...(init?.signal === null || init?.signal === undefined + ? {} + : { signal: init.signal as AbortSignal }), ...(await requestBody(request)), }; this.calls.push(call); const response = this.respond(call); + if (response.neverAnswers === true) { + return await neverAnsweredResponse(call.signal); + } return new Response(JSON.stringify(response.body), { status: response.status ?? 200, headers: { 'Content-Type': 'application/json' }, @@ -65,6 +80,21 @@ export function cloudWebDriverTestJson(body: unknown, status = 200): CloudWebDri return { body, status }; } +/** A driver that never answers: only the caller's cancellation ends it. */ +async function neverAnsweredResponse(signal: AbortSignal | undefined): Promise { + return await new Promise((_resolve, reject) => { + if (!signal) { + reject(new Error('A never-answered request needs the caller cancellation to end it.')); + return; + } + if (signal.aborted) { + reject(signal.reason as Error); + return; + } + signal.addEventListener('abort', () => reject(signal.reason as Error), { once: true }); + }); +} + async function requestBody(request: Request): Promise<{ body?: unknown }> { if (!request.body) return {}; const buffer = Buffer.from(await request.arrayBuffer()); diff --git a/website/docs/docs/aws-device-farm.md b/website/docs/docs/aws-device-farm.md index 5f6fa0ebd7..bb92be698c 100644 --- a/website/docs/docs/aws-device-farm.md +++ b/website/docs/docs/aws-device-farm.md @@ -109,3 +109,5 @@ agent-device artifacts --provider aws-device-farm -- If `connect` fails, use the reported `aws devicefarm get-*` error to check the credential chain, ARN, region, resource platform, or upload readiness. The provider has not allocated a device yet. If artifacts are pending immediately after `close`, retry the lookup. On hosted WebDriver sessions, `fill` checks that the field received focus before it sends keys. If it cannot confirm focus, it fails without typing. Use `snapshot -i` to confirm the target. If the driver cannot expose focus at all, use `press ` followed by `type `. That sends text without confirming the destination. + +A screen that never goes still — a looping video, a live ticker, continuous animation — gives the provider's driver no quiet moment to read the UI tree, so `snapshot -i` can run out of its read budget on a rented device while `screenshot` of the same screen still returns. Snapshots covers what that failure means and what to do instead; on a metered device the difference matters, because every second of the walk is billed. Take the screenshot, drive from `@refs` an earlier snapshot captured, and remember that `--depth` trims a tree after it arrives, so it cannot shorten a read that never returned. diff --git a/website/docs/docs/snapshots.md b/website/docs/docs/snapshots.md index 474becad7a..9a4a1a2492 100644 --- a/website/docs/docs/snapshots.md +++ b/website/docs/docs/snapshots.md @@ -60,6 +60,7 @@ agent-device snapshot --diff # Alias for the same diff operation - Re-snapshot after any UI mutation before reusing refs. - On Android after navigation or submit, snapshot capture retries suspicious trees for a short post-action deadline and `@ref` interactions refresh while that freshness window is active. If `snapshot -i` still disagrees with the visible screen, trust `screenshot`, wait briefly, then take one fresh snapshot instead of looping stale snapshots. - For automation runs affected by Android animation churn, use `settings animations off` as an opt-in stabilizer and restore with `settings animations on` after the run. +- On a device cloud the tree is read by the provider's driver, so a screen that never goes still — a looping video, a live ticker, continuous animation — gives that read no quiet moment and it can run out of its budget while `screenshot` still returns. The read is cancelled with the request that asked for it, so agent-device stops waiting on it and stops holding the session open for it; the driver's own walk can continue on the provider, where it may still occupy that session's queue. Two dead ends: the read carries its own budget, so a larger `--timeout` cannot lengthen it, and `settings animations` is not implemented on hosted WebDriver sessions. - Use `diff snapshot` between mutations to validate structural changes with lower output volume. - Use `snapshot --diff` when you discover the feature from snapshot help, but keep `diff snapshot` as the default exploration command. - Keep `--raw` for troubleshooting only when you need the full tree instead of visible-first output.