diff --git a/packages/contracts/src/device-provider.ts b/packages/contracts/src/device-provider.ts index 6558536fa..4cf6c88e2 100644 --- a/packages/contracts/src/device-provider.ts +++ b/packages/contracts/src/device-provider.ts @@ -25,6 +25,14 @@ export type DeviceLease = { export type LeaseLifecycleContext = { flags?: Readonly>; cwd?: string; + /** Request-bound cancellation (explicit cancel or client disconnect). */ + signal?: AbortSignal; + /** + * Epoch-ms deadline by which `allocate` must have settled; derived from the + * same budget as the client's `lease_allocate` envelope, so a provider that + * fits its remote phases within it is never abandoned by a client first. + */ + deadline?: number; }; export type LeaseLifecycleProvider = { diff --git a/packages/kernel/src/errors.ts b/packages/kernel/src/errors.ts index e2ad36c8d..4c3b97640 100644 --- a/packages/kernel/src/errors.ts +++ b/packages/kernel/src/errors.ts @@ -128,6 +128,45 @@ export function throwDaemonError(error: DaemonError): never { }); } +/** + * `details.reason` of a request its requester abandoned — an explicit cancel or + * a client disconnect. One definition, so every layer that must let a + * cancellation through untouched (retry loops, provider adapters, runner + * transports) dispatches on the same typed reason. + */ +const REQUEST_CANCELED_REASON = 'request_canceled'; +const REQUEST_CANCELED_MESSAGE = 'request canceled'; +const REQUEST_CANCELED_HINT = + 'The request was canceled intentionally (explicit cancel or client disconnect) — no retry is needed unless the cancellation was unintended.'; + +/** + * The canceled-request error. `details` may add evidence (what was released, + * which command was interrupted) or override the hint; the reason itself is + * not overridable, so a caller cannot build one this predicate misses. + */ +export function createRequestCanceledError(details?: AppErrorDetails, cause?: unknown): AppError { + return new AppError( + 'COMMAND_FAILED', + REQUEST_CANCELED_MESSAGE, + { hint: REQUEST_CANCELED_HINT, ...details, reason: REQUEST_CANCELED_REASON }, + cause, + ); +} + +export function isRequestCanceledError(error: unknown): boolean { + if (!(error instanceof AppError)) return false; + if (error.code !== 'COMMAND_FAILED') return false; + if (error.details?.reason === REQUEST_CANCELED_REASON) return true; + // Owned debt: canceled errors that crossed a wire without their details keep + // the message; do not add new message sniffs beside it. + return error.message === REQUEST_CANCELED_MESSAGE; +} + +/** The message of whatever was thrown, for diagnostics that must not themselves throw. */ +export function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + export function asAppError(err: unknown, fallbackCode: AppErrorCode = 'UNKNOWN'): AppError { if (err instanceof AppError) return err; if (err instanceof Error) { diff --git a/packages/maestro/src/internal/engine-flow.ts b/packages/maestro/src/internal/engine-flow.ts index 5d31a0beb..6de04600a 100644 --- a/packages/maestro/src/internal/engine-flow.ts +++ b/packages/maestro/src/internal/engine-flow.ts @@ -1,6 +1,5 @@ import path from 'node:path'; -import { AppError } from '@agent-device/kernel/errors'; -import { createRequestCanceledError } from './shared.ts'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { MAESTRO_NUMERIC_FIELD_CONSTRAINTS, numericDescription, diff --git a/packages/maestro/src/internal/program-loader.ts b/packages/maestro/src/internal/program-loader.ts index 6f8f7505a..5163b00b8 100644 --- a/packages/maestro/src/internal/program-loader.ts +++ b/packages/maestro/src/internal/program-loader.ts @@ -1,6 +1,6 @@ import fs from 'node:fs'; import path from 'node:path'; -import { createRequestCanceledError } from './shared.ts'; +import { createRequestCanceledError } from '@agent-device/kernel/errors'; import type { MaestroProgram } from './program-ir.ts'; import { parseMaestroProgram } from './program-ir-parser.ts'; diff --git a/packages/maestro/src/internal/shared.ts b/packages/maestro/src/internal/shared.ts index fb507f52d..1b9b0d841 100644 --- a/packages/maestro/src/internal/shared.ts +++ b/packages/maestro/src/internal/shared.ts @@ -1,4 +1,3 @@ -import { AppError } from '@agent-device/kernel/errors'; import type { Point, Rect, SnapshotNode } from '@agent-device/kernel/snapshot'; export function stripUndefined>(value: T): T { @@ -45,10 +44,3 @@ export function extractNodeText(node: SnapshotNode): string { ?.trim() ?? '' ); } - -export function createRequestCanceledError(): AppError { - return new AppError('COMMAND_FAILED', 'request canceled', { - reason: 'request_canceled', - hint: 'The request was canceled intentionally (explicit cancel or client disconnect) — no retry is needed unless the cancellation was unintended.', - }); -} diff --git a/packages/provider-webdriver/src/aws-device-farm.test.ts b/packages/provider-webdriver/src/aws-device-farm.test.ts new file mode 100644 index 000000000..89a83e6f9 --- /dev/null +++ b/packages/provider-webdriver/src/aws-device-farm.test.ts @@ -0,0 +1,181 @@ +import assert from 'node:assert/strict'; +import { afterEach, test, vi } from 'vitest'; +import type { DeviceLease } from '@agent-device/contracts/device'; +import { + AppError, + createRequestCanceledError, + isRequestCanceledError, +} from '@agent-device/kernel/errors'; +import { + createAwsDeviceFarmPrepareSession, + type AwsDeviceFarmClient, + type AwsDeviceFarmRemoteAccessSession, +} from './aws-device-farm.ts'; +import { buildCloudWebDriverBaseCapabilities } from './runtime.ts'; + +const ARN = 'arn:aws:devicefarm:us-west-2:1:session/pending'; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +// Once `create-remote-access-session` answers, the ARN is a billed session that +// nothing else will stop. Every way the startup wait can end short of RUNNING +// must stop it before the failure surfaces (#1774 ownership rule, one phase +// earlier than the WebDriver session). +test('a startup timeout stops the remote-access session it created', async () => { + const client = fakeClient({ status: 'PENDING' }); + const prepare = createAwsDeviceFarmPrepareSession({ + ...baseOptions(client), + startupTimeoutMs: 30, + pollIntervalMs: 5, + }); + + await assert.rejects( + () => prepare({ lease: makeLease(), base: baseSession() }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.match(error.message, /Timed out waiting for AWS Device Farm/); + return true; + }, + ); + assert.deepEqual(client.stopped, [ARN]); +}); + +test('a canceled request stops the remote-access session mid-startup', async () => { + const controller = new AbortController(); + const client = fakeClient({ status: 'PENDING' }, () => + controller.abort(createRequestCanceledError()), + ); + const prepare = createAwsDeviceFarmPrepareSession({ + ...baseOptions(client), + startupTimeoutMs: 5_000, + pollIntervalMs: 5, + }); + + await assert.rejects( + () => prepare({ lease: makeLease(), req: { signal: controller.signal }, base: baseSession() }), + (error: unknown) => isRequestCanceledError(error), + ); + assert.deepEqual(client.stopped, [ARN]); +}); + +// The daemon's allocation deadline caps the startup wait so a client that +// stops waiting at that deadline never abandons a still-polling daemon. +test('the allocation deadline caps the startup wait below its own default', async () => { + const client = fakeClient({ status: 'PENDING' }); + const prepare = createAwsDeviceFarmPrepareSession({ + ...baseOptions(client), + startupTimeoutMs: 60_000, + pollIntervalMs: 5, + }); + const startedAt = Date.now(); + + await assert.rejects( + () => prepare({ lease: makeLease(), req: { deadline: Date.now() + 40 }, base: baseSession() }), + /Timed out waiting for AWS Device Farm/, + ); + assert.ok(Date.now() - startedAt < 5_000, 'the wait must end at the deadline, not the default'); + assert.deepEqual(client.stopped, [ARN]); +}); + +// Live iOS real devices needed ~128s to reach RUNNING while the daemon's 300s +// allocation budget still had room, and the standalone 120s default cut them +// off. When the daemon supplies a deadline it is THE bound; the default only +// applies without one. Time is a virtual clock advanced 10s per poll, so this is +// deterministic and fails on the old `min(default, deadline)` logic (which +// throws at 120s, before the 150s RUNNING). +test('the allocation deadline lets startup run past the standalone 120s default', async () => { + const startedAt = 1_700_000_000_000; + let virtualNow = startedAt; + vi.spyOn(Date, 'now').mockImplementation(() => virtualNow); + const client = fakeClient({ status: 'PENDING' }, () => { + virtualNow += 10_000; + if (virtualNow - startedAt >= 150_000) { + client.session.status = 'RUNNING'; + client.session.endpoints = { appium: 'https://appium.example/wd/hub' }; + } + }); + const prepare = createAwsDeviceFarmPrepareSession({ + ...baseOptions(client), + // The standalone default; a daemon-supplied deadline must override it. + startupTimeoutMs: 120_000, + pollIntervalMs: 1, + }); + + const prepared = await prepare({ + lease: makeLease(), + req: { deadline: startedAt + 300_000 }, + base: baseSession(), + }); + assert.equal(prepared.providerSessionId, ARN); + assert.ok(virtualNow - startedAt >= 150_000, 'RUNNING must have been observed after 120s'); + assert.deepEqual(client.stopped, []); +}); + +test('a session that reaches RUNNING is handed on and not stopped', async () => { + const client = fakeClient({ + status: 'RUNNING', + endpoints: { appium: 'https://appium.example/wd/hub' }, + }); + const prepare = createAwsDeviceFarmPrepareSession(baseOptions(client)); + + const prepared = await prepare({ lease: makeLease(), base: baseSession() }); + assert.equal(prepared.providerSessionId, ARN); + assert.equal(prepared.endpoint, 'https://appium.example/wd/hub'); + assert.deepEqual(client.stopped, []); +}); + +function fakeClient( + session: Partial, + onPoll?: () => void, +): AwsDeviceFarmClient & { stopped: string[]; session: Partial } { + const stopped: string[] = []; + return { + stopped, + session, + createRemoteAccessSession: async () => ({ arn: ARN, status: 'PENDING' }), + getRemoteAccessSession: async (arn) => { + onPoll?.(); + return { arn, ...session }; + }, + stopRemoteAccessSession: async (arn) => { + stopped.push(arn); + return { arn, status: 'STOPPING' }; + }, + listArtifacts: async () => [], + }; +} + +function baseOptions(client: AwsDeviceFarmClient) { + return { + client, + platform: 'android' as const, + deviceName: 'Pixel', + projectArn: 'arn:aws:devicefarm:us-west-2:1:project/p', + deviceArn: 'arn:aws:devicefarm:us-west-2::device/d', + }; +} + +function baseSession() { + return { + provider: 'aws-device-farm', + endpoint: 'http://127.0.0.1/', + platform: 'android' as const, + deviceName: 'Pixel', + webdriverCapabilities: buildCloudWebDriverBaseCapabilities('android', 'Pixel'), + }; +} + +function makeLease(): DeviceLease { + return { + leaseId: 'lease-aws', + tenantId: 'team-a', + runId: 'run-a', + leaseProvider: 'aws-device-farm', + backend: 'android-instance', + createdAt: 1, + expiresAt: 2, + heartbeatAt: 1, + }; +} diff --git a/packages/provider-webdriver/src/aws-device-farm.ts b/packages/provider-webdriver/src/aws-device-farm.ts index 15fc1e5e1..02f35f7ff 100644 --- a/packages/provider-webdriver/src/aws-device-farm.ts +++ b/packages/provider-webdriver/src/aws-device-farm.ts @@ -15,12 +15,16 @@ import type { CloudWebDriverRuntimeOptions, CloudWebDriverPrepareSession, } from './runtime.ts'; -import type { DeviceLease, ProviderDeviceRuntime } from '@agent-device/contracts/device'; +import type { + DeviceLease, + LeaseLifecycleContext, + ProviderDeviceRuntime, +} from '@agent-device/contracts/device'; import { setTimeout as sleep } from 'node:timers/promises'; import { AppError } from '@agent-device/kernel/errors'; import type { RunHostCommand } from './dependencies.ts'; import { CLOUD_WEBDRIVER_PROVIDERS } from './providers.ts'; -import { resolveLeaseValue, type LeaseValue } from './webdriver-utils.ts'; +import { releaseOnFailure, resolveLeaseValue, type LeaseValue } from './webdriver-utils.ts'; const AWS_DEVICE_FARM_PROVIDER = CLOUD_WEBDRIVER_PROVIDERS.awsDeviceFarm; export const AWS_DEVICE_FARM_CAPABILITY_OVERRIDES = { @@ -203,7 +207,7 @@ export function createAwsDeviceFarmPrepareSession( 'client' | 'platform' | 'deviceName' | 'clientVersion' >, ): CloudWebDriverPrepareSession { - return async ({ lease, base }) => { + return async ({ lease, req, base }) => { const remoteAccess = await options.client.createRemoteAccessSession({ projectArn: options.projectArn, deviceArn: options.deviceArn, @@ -212,13 +216,23 @@ export function createAwsDeviceFarmPrepareSession( interactionMode: options.interactionMode, configuration: options.configuration, }); - const running = await waitForRunningRemoteAccessSession(remoteAccess.arn, options); - const endpoint = selectAwsDeviceFarmWebDriverEndpoint(running); - if (!endpoint) { - throw new AppError('COMMAND_FAILED', 'AWS Device Farm did not expose a WebDriver endpoint.', { - sessionArn: running.arn, - status: running.status, - }); + // The ARN is a billed session from here on; any failure short of RUNNING + // must stop it before surfacing, or it bills until AWS reaps it. + let running: AwsDeviceFarmRemoteAccessSession; + let endpoint: string | undefined; + try { + running = await waitForRunningRemoteAccessSession(remoteAccess.arn, options, req); + endpoint = selectAwsDeviceFarmWebDriverEndpoint(running); + if (!endpoint) { + throw new AppError( + 'COMMAND_FAILED', + 'AWS Device Farm did not expose a WebDriver endpoint.', + { sessionArn: running.arn, status: running.status }, + ); + } + } catch (error) { + await releaseOnFailure(error, () => options.client.stopRemoteAccessSession(remoteAccess.arn)); + throw error; } const deviceName = running.device?.name ?? options.deviceName; const configured = @@ -276,28 +290,40 @@ async function waitForRunningRemoteAccessSession( pollIntervalMs?: number; startupTimeoutMs?: number; }, + req: LeaseLifecycleContext | undefined, ): Promise { - const timeoutMs = options.startupTimeoutMs ?? 120_000; const pollIntervalMs = options.pollIntervalMs ?? 5_000; const startedAt = Date.now(); + // The daemon's allocation deadline is the bound when present (real-device + // startup routinely needs the whole ~2 min); the standalone default only + // applies when no allocation budget was supplied. + const deadline = req?.deadline ?? startedAt + (options.startupTimeoutMs ?? 120_000); + const signal = req?.signal; let last = await options.client.getRemoteAccessSession(arn); - while (Date.now() - startedAt < timeoutMs) { + while (Date.now() < deadline) { + signal?.throwIfAborted(); if (last.status === 'RUNNING') return last; - if (last.status === 'ERRORED' || last.status === 'STOPPED' || last.status === 'COMPLETED') { - throw new AppError('COMMAND_FAILED', 'AWS Device Farm remote access session did not start.', { - sessionArn: arn, - status: last.status, - result: last.result, - }); - } - await sleep(pollIntervalMs); + throwIfRemoteAccessSessionEnded(last); + // Wake early on cancellation; the typed reason is rethrown at the loop top. + await sleep(pollIntervalMs, undefined, { signal }).catch(() => signal?.throwIfAborted()); last = await options.client.getRemoteAccessSession(arn); } throw new AppError('COMMAND_FAILED', 'Timed out waiting for AWS Device Farm remote access.', { sessionArn: arn, status: last.status, result: last.result, - timeoutMs, + timeoutMs: deadline - startedAt, + }); +} + +const ENDED_REMOTE_ACCESS_STATUSES = new Set(['ERRORED', 'STOPPED', 'COMPLETED']); + +function throwIfRemoteAccessSessionEnded(session: AwsDeviceFarmRemoteAccessSession): void { + if (!session.status || !ENDED_REMOTE_ACCESS_STATUSES.has(session.status)) return; + throw new AppError('COMMAND_FAILED', 'AWS Device Farm remote access session did not start.', { + sessionArn: session.arn, + status: session.status, + result: session.result, }); } diff --git a/packages/provider-webdriver/src/provider-definitions.ts b/packages/provider-webdriver/src/provider-definitions.ts index 6ff6110fd..81289f42b 100644 --- a/packages/provider-webdriver/src/provider-definitions.ts +++ b/packages/provider-webdriver/src/provider-definitions.ts @@ -119,6 +119,10 @@ export function createCloudWebDriverProviderDefinitions( username, accessKey, uploadEndpoint: env.BROWSERSTACK_APP_UPLOAD_ENDPOINT, + // A local IPA/APK upload can run long (130 MB is routine); an + // upload is not a billed resource, so the request's cancellation + // may simply abort it — unlike the session creation that follows. + signal: request.signal, }); return { ...base, @@ -252,6 +256,7 @@ async function resolveBrowserStackAppReference(options: { username: string; accessKey: string; uploadEndpoint?: string; + signal?: AbortSignal; }): Promise { if (isProviderAppReference(options.app)) return options.app; const appPath = path.resolve(options.cwd ?? process.cwd(), options.app); @@ -262,12 +267,16 @@ async function resolveBrowserStackAppReference(options: { { providerApp: options.app }, ); } - return await uploadBrowserStackApp(appPath, { - clientVersion: options.clientVersion, - username: options.username, - accessKey: options.accessKey, - endpoint: options.uploadEndpoint, - }); + return await uploadBrowserStackApp( + appPath, + { + clientVersion: options.clientVersion, + username: options.username, + accessKey: options.accessKey, + endpoint: options.uploadEndpoint, + }, + options.signal, + ); } function isProviderAppReference(value: string): boolean { diff --git a/packages/provider-webdriver/src/runtime-session.test.ts b/packages/provider-webdriver/src/runtime-session.test.ts index 88e3a4422..f07ad5c18 100644 --- a/packages/provider-webdriver/src/runtime-session.test.ts +++ b/packages/provider-webdriver/src/runtime-session.test.ts @@ -1,24 +1,19 @@ import assert from 'node:assert/strict'; -import { test } from 'vitest'; +import { afterEach, test } from 'vitest'; import type { DeviceLease } from '@agent-device/contracts/device'; import { AppError } from '@agent-device/kernel/errors'; -import { createCloudWebDriverRuntime } from './runtime.ts'; +import { createCloudWebDriverRuntime, type CloudWebDriverRuntimeOptions } from './runtime.ts'; + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; +}); test('session allocation preserves its primary failure when provider cleanup also fails', async () => { - const previousFetch = globalThis.fetch; let cleanupCalled = false; - globalThis.fetch = async () => - new Response(JSON.stringify({ value: { message: 'create session failed' } }), { - status: 500, - headers: { 'Content-Type': 'application/json' }, - }); - const runtime = createCloudWebDriverRuntime({ - clientVersion: 'test', - provider: 'webdriver-test', - endpoint: 'https://webdriver.test/wd/hub/', - platform: 'android', - deviceName: 'Test device', - requestPolicy: { retryAttempts: 0 }, + globalThis.fetch = async () => jsonResponse({ value: { message: 'create session failed' } }, 500); + const runtime = makeRuntime({ prepareSession: async ({ base }) => ({ ...base, cleanup: async () => { @@ -29,10 +24,8 @@ test('session allocation preserves its primary failure when provider cleanup als }); try { - const allocate = runtime.leaseLifecycle.allocate; - assert.ok(allocate); await assert.rejects( - () => allocate(makeLease()), + () => runtime.leaseLifecycle.allocate!(makeLease()), (error: unknown) => { assert.ok(error instanceof AppError); assert.match(error.message, /create session failed/); @@ -43,10 +36,61 @@ test('session allocation preserves its primary failure when provider cleanup als assert.equal(cleanupCalled, true); } finally { await runtime.shutdown(); - globalThis.fetch = previousFetch; } }); +// #1774: when the transport gives up on `POST /session`, the provider may still +// finish it. The error names the lease the capabilities were labelled with so +// an operator can find and stop the maybe-orphaned billed session, rather than +// this process guessing at REST cleanup of a session it never owned. +test('a create-timeout surfaces provider evidence for the maybe-leaked session', async () => { + globalThis.fetch = async (_input, init) => + await new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); + }); + const runtime = makeRuntime({ + provider: 'browserstack', + platform: 'ios', + requestPolicy: { retryAttempts: 0, sessionCreateTimeoutMs: 40 }, + }); + const lease = { ...makeLease(), leaseProvider: 'browserstack' }; + + try { + await assert.rejects( + () => runtime.leaseLifecycle.allocate!(lease), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, 'provider_session_create_timeout'); + assert.equal(error.details?.provider, 'browserstack'); + assert.equal(error.details?.leaseId, lease.leaseId); + assert.match(String(error.details?.hint), new RegExp(lease.leaseId)); + return true; + }, + ); + } finally { + await runtime.shutdown(); + } +}); + +function makeRuntime(overrides: Partial = {}) { + return createCloudWebDriverRuntime({ + clientVersion: 'test', + provider: 'webdriver-test', + endpoint: 'https://webdriver.test/wd/hub/', + platform: 'android', + deviceName: 'Test device', + requestPolicy: { retryAttempts: 0 }, + ...overrides, + }); +} + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }); +} + function makeLease(): DeviceLease { return { leaseId: 'lease-1', diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index 216d19ec4..b309290dd 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -4,15 +4,17 @@ import type { } from '@agent-device/contracts/observability'; import type { DeviceLease, LeaseLifecycleContext } from '@agent-device/contracts/device'; import { deviceFieldsFromPublicPlatform, type DeviceInfo } from '@agent-device/kernel/device'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, errorMessage } from '@agent-device/kernel/errors'; import { unavailableCloudArtifactsResult } from './artifact-results.ts'; import { createCloudWebDriverCapabilities, type CloudWebDriverProviderCapabilities, } from './capabilities.ts'; -import { WebDriverClient } from './webdriver-client.ts'; +import { WebDriverClient, type WebDriverSession } from './webdriver-client.ts'; +import { isWebDriverRequestTimeout } from './webdriver-transport.ts'; import { createWebDriverInteractor } from './webdriver-interactor.ts'; import { snapshotBackendForPlatform } from './runtime-helpers.ts'; +import { releaseOnFailure } from './webdriver-utils.ts'; import type { CloudWebDriverBaseSession, CloudWebDriverPlatform, @@ -89,7 +91,7 @@ export class WebDriverSessionManager { headers: prepared.headers, requestPolicy: this.options.requestPolicy, }); - const session = await this.createSessionWithPreparedCleanup(client, prepared); + const session = await this.createSessionWithPreparedCleanup(client, prepared, lease, req); const device = this.deviceForLease(lease, prepared); const providerSessionId = prepared.providerSessionId ?? session.sessionId; const capabilities = createCloudWebDriverCapabilities({ @@ -167,12 +169,19 @@ export class WebDriverSessionManager { private async createSessionWithPreparedCleanup( client: WebDriverClient, prepared: CloudWebDriverPreparedSession, - ): Promise>> { + lease: DeviceLease, + req: LeaseLifecycleContext | undefined, + ): Promise { try { - return await client.createSession(prepared.webdriverCapabilities); + return await client.createSession(prepared.webdriverCapabilities, { + deadline: req?.deadline, + }); } catch (error) { - await cleanupAfterCreateSessionFailure(prepared, error); - throw error; + const failure = isWebDriverRequestTimeout(error) + ? sessionCreateTimeoutError(error, this.options.provider, lease, prepared) + : error; + await releaseOnFailure(failure, () => prepared.cleanup?.()); + throw failure; } } @@ -277,22 +286,31 @@ export function buildCloudWebDriverBaseCapabilities( }; } -function errorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error); -} - -async function cleanupAfterCreateSessionFailure( +/** + * The transport gave up on `POST /session`; the provider may still finish it, + * and nothing here can learn that session's id — so the error names the lease + * the capabilities were labelled with, which the provider dashboard can be + * searched by. + */ +function sessionCreateTimeoutError( + timeout: AppError, + provider: string, + lease: DeviceLease, prepared: CloudWebDriverPreparedSession, - primaryError: unknown, -): Promise { - try { - await prepared.cleanup?.(); - } catch (cleanupError) { - if (primaryError instanceof AppError) { - primaryError.details = { - ...primaryError.details, - cleanupError: errorMessage(cleanupError), - }; - } - } +): AppError { + const timeoutMs = timeout.details?.timeoutMs; + return new AppError( + 'COMMAND_FAILED', + `${provider} did not create the WebDriver session within ${String(timeoutMs)}ms.`, + { + reason: 'provider_session_create_timeout', + provider, + leaseId: lease.leaseId, + runId: lease.runId, + timeoutMs, + ...(prepared.providerSessionId ? { providerSessionId: prepared.providerSessionId } : {}), + hint: `${provider} may still finish creating the session after agent-device stopped waiting, and that session would keep billing until the provider reaps it. Before retrying, check ${provider} for a running session created for lease ${lease.leaseId} (run ${lease.runId}) and stop it.`, + }, + timeout, + ); } diff --git a/packages/provider-webdriver/src/webdriver-client.test.ts b/packages/provider-webdriver/src/webdriver-client.test.ts index bc5c27fbe..a8bd8e627 100644 --- a/packages/provider-webdriver/src/webdriver-client.test.ts +++ b/packages/provider-webdriver/src/webdriver-client.test.ts @@ -68,8 +68,9 @@ test('is_keyboard_shown honors a caller timeout shorter than the client default' }), ); - await assert.rejects(client.isKeyboardShown(50), (error: Error) => { - assert.match(`${error.name} ${error.message}`, /timeout|abort/i); + await assert.rejects(client.isKeyboardShown(50), (error: AppError) => { + assert.equal(error.details?.reason, 'webdriver_request_timeout'); + assert.equal(error.details?.timeoutMs, 50); return true; }); }); @@ -105,6 +106,96 @@ test('installApp aborts an in-flight provider request when its binding is cancel } }); +// #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. +test('createSession does not retry a transient create failure', async () => { + let calls = 0; + globalThis.fetch = async () => { + calls += 1; + return new Response(JSON.stringify({ value: { message: 'grid busy' } }), { + status: 503, + headers: { 'Content-Type': 'application/json' }, + }); + }; + const client = new WebDriverClient({ + clientVersion: '0.0.0-test', + endpoint: 'http://cloud-webdriver.test/wd/hub/', + // A retry-enabled policy would double a normal request; createSession must + // still refuse to retry regardless of the client's retry budget. + requestPolicy: { timeoutMs: 30_000, retryAttempts: 3 }, + }); + + await assert.rejects(client.createSession({ platformName: 'iOS' }), /grid busy/); + assert.equal(calls, 1); +}); + +// The device-allocation phase routinely runs 45–90s on cloud iOS real devices, +// far past the per-request default that suits a settled session's round trips. +// A create that answers after the default 30s window must still land (#1774). +test('createSession waits on its own budget, not the per-request default', async () => { + let sessionRequestTimeoutMs: number | undefined; + globalThis.fetch = async (input, init) => { + const url = String(input instanceof Request ? input.url : input); + if (url.endsWith('/session')) { + const startedAt = Date.now(); + init?.signal?.addEventListener('abort', () => { + sessionRequestTimeoutMs = Date.now() - startedAt; + }); + await new Promise((resolve) => setTimeout(resolve, 60)); + return new Response(JSON.stringify({ value: { sessionId: 'wd-slow', capabilities: {} } }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + } + throw new Error(`unexpected ${url}`); + }; + const client = new WebDriverClient({ + clientVersion: '0.0.0-test', + endpoint: 'http://cloud-webdriver.test/wd/hub/', + // A create bound by this 20ms default would abort before the 60ms answer; + // the dedicated session-create budget is what keeps it alive. + requestPolicy: { timeoutMs: 20, retryAttempts: 0, sessionCreateTimeoutMs: 5_000 }, + }); + + const session = await client.createSession({ platformName: 'iOS' }); + assert.equal(session.sessionId, 'wd-slow'); + assert.equal(sessionRequestTimeoutMs, undefined, 'the create request must not have been aborted'); +}); + +// The operation deadline (the daemon's remaining lease-allocation budget) can +// only SHORTEN the create budget: a request that already spent most of its +// allocation window on provider preparation must not start a 180s device +// allocation it cannot wait out (#1774). +test('createSession is bounded by the operation deadline when it is shorter', async () => { + globalThis.fetch = async (input, init) => { + const url = String(input instanceof Request ? input.url : input); + if (url.endsWith('/session')) { + return await new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); + }); + } + throw new Error(`unexpected ${url}`); + }; + const client = new WebDriverClient({ + clientVersion: '0.0.0-test', + endpoint: 'http://cloud-webdriver.test/wd/hub/', + requestPolicy: { timeoutMs: 30_000, retryAttempts: 0, sessionCreateTimeoutMs: 180_000 }, + }); + + await assert.rejects( + client.createSession({ platformName: 'iOS' }, { deadline: Date.now() + 40 }), + (error: AppError) => { + assert.equal(error.details?.reason, 'webdriver_request_timeout'); + assert.ok( + typeof error.details?.timeoutMs === 'number' && error.details.timeoutMs <= 40, + `create should be bounded by the ~40ms deadline, got ${String(error.details?.timeoutMs)}`, + ); + return true; + }, + ); +}); + // The focused element is the only signal that can say WHICH field took focus, // so `fill` fails closed when it is unavailable. Each wire shape it // distinguishes is pinned here. @@ -226,14 +317,19 @@ test('activeElement bounds its two sequential requests by one shared budget', as }); }); - // Not an AppError: this is the raw AbortSignal.timeout() rejection the transport - // re-throws unwrapped once the combined signal is already aborted (see - // shouldRetryWebDriverRequest in webdriver-transport.ts) — assert the timeout's - // own reason instead of any failure. - await assert.rejects( - client.activeElement(budgetMs), - (error: unknown) => error instanceof Error && error.name === 'TimeoutError', - ); + // The transport's own deadline surfaces as the typed timeout (#1774), not a + // raw AbortSignal.timeout() DOMException — assert that contract, and that the + // budget the second request was handed is what the first call left over. + await assert.rejects(client.activeElement(budgetMs), (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, 'webdriver_request_timeout'); + assert.ok( + typeof error.details?.timeoutMs === 'number' && + error.details.timeoutMs < budgetMs - firstCallMs / 2, + `the rect request's budget must be the shared remainder, got ${String(error.details?.timeoutMs)}`, + ); + return true; + }); assert.ok(rectRequestBudgetMs !== undefined, 'the rect request should have been made'); // It must get what the first call left (~120ms), never a fresh 200ms. diff --git a/packages/provider-webdriver/src/webdriver-client.ts b/packages/provider-webdriver/src/webdriver-client.ts index cc3f41f54..96973c9e0 100644 --- a/packages/provider-webdriver/src/webdriver-client.ts +++ b/packages/provider-webdriver/src/webdriver-client.ts @@ -9,6 +9,14 @@ import { export type { WebDriverAuth, WebDriverRequestPolicy } from './webdriver-transport.ts'; +/** + * Default budget for `POST /session`. Cloud providers allocate a physical + * device inside that one request — BrowserStack iOS real devices routinely + * take 45–90s (#1774) — so it is far above the per-request default that + * suits a settled session's round trips. + */ +const DEFAULT_SESSION_CREATE_TIMEOUT_MS = 180_000; + export type WebDriverClientOptions = { clientVersion: string; endpoint: string | URL; @@ -60,16 +68,35 @@ export type W3CActionSequence = { export class WebDriverClient { private readonly transport: WebDriverTransport; + private readonly sessionCreateTimeoutMs: number; private sessionId: string | undefined; constructor(options: WebDriverClientOptions) { this.transport = new WebDriverTransport(options); + this.sessionCreateTimeoutMs = + options.requestPolicy?.sessionCreateTimeoutMs ?? DEFAULT_SESSION_CREATE_TIMEOUT_MS; } - async createSession(capabilities: Record): Promise { - const value = await this.requestValue('POST', '/session', { - capabilities: normalizeCapabilities(capabilities), - }); + /** + * `POST /session` is non-idempotent and its outcome after a client-side abort + * is indeterminate (the hub finishes allocating whether or not anyone is + * listening), so it takes no cancellation signal and never retries: a retry + * is a second billed session, an abort loses the id of the first (#1774). + * `deadline` (epoch ms) can only shorten the create budget, never extend it. + */ + async createSession( + capabilities: Record, + options?: { deadline?: number }, + ): Promise { + const value = await this.requestValue( + 'POST', + '/session', + { capabilities: normalizeCapabilities(capabilities) }, + { + retryAttempts: 0, + timeoutMs: budgetWithin(this.sessionCreateTimeoutMs, options?.deadline), + }, + ); const session = readSession(value); this.sessionId = session.sessionId; return session; @@ -363,7 +390,16 @@ function readW3CElementId(value: unknown): string | undefined { */ function requestBudget(deadline: number | undefined): WebDriverRequestOverrides { if (deadline === undefined) return { retryAttempts: 0 }; - return { retryAttempts: 0, timeoutMs: Math.max(0, deadline - Date.now()) }; + return { retryAttempts: 0, timeoutMs: remainingMs(deadline) }; +} + +/** A phase's own budget, capped by the operation deadline it runs under, if any. */ +function budgetWithin(budgetMs: number, deadline: number | undefined): number { + return deadline === undefined ? budgetMs : Math.min(budgetMs, remainingMs(deadline)); +} + +function remainingMs(deadline: number): number { + return Math.max(0, deadline - Date.now()); } /** Nothing is focused right now — an expected state, not a driver defect. */ diff --git a/packages/provider-webdriver/src/webdriver-transport.test.ts b/packages/provider-webdriver/src/webdriver-transport.test.ts index e2cce84b4..7d85b57c3 100644 --- a/packages/provider-webdriver/src/webdriver-transport.test.ts +++ b/packages/provider-webdriver/src/webdriver-transport.test.ts @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; import { afterEach, test } from 'vitest'; -import { WebDriverTransport } from './webdriver-transport.ts'; +import { AppError } from '@agent-device/kernel/errors'; +import { WebDriverTransport, isWebDriverRequestTimeout } from './webdriver-transport.ts'; const realFetch = globalThis.fetch; @@ -8,6 +9,58 @@ afterEach(() => { globalThis.fetch = realFetch; }); +// The transport's own deadline must surface as a machine-readable reason, not +// as fetch's `AbortError`/`TimeoutError` DOMException name: the session manager +// keys its "the provider may still be creating a billed session" branch on +// `details.reason`, and a name-sniffing consumer would confuse it with a +// caller-driven cancellation (#1774). +test('a transport-deadline abort surfaces as a typed timeout, not a DOMException name', async () => { + const transport = new WebDriverTransport({ + clientVersion: '0.0.0-test', + endpoint: 'http://cloud-webdriver.test/wd/hub/', + requestPolicy: { timeoutMs: 30, retryAttempts: 0 }, + }); + globalThis.fetch = async (_input, init) => + await new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); + }); + + await assert.rejects(transport.requestValue('POST', '/session', {}), (error: unknown) => { + assert.ok(error instanceof AppError); + assert.ok(isWebDriverRequestTimeout(error)); + assert.equal(error.details?.timeoutMs, 30); + assert.equal(error.details?.method, 'POST'); + assert.equal(error.details?.path, '/session'); + return true; + }); +}); + +// A caller that cancels its own request keeps a caller-cancellation error — +// only the transport's deadline becomes a typed timeout. Otherwise a client +// disconnect during `POST /session` would read as a provider timeout and drive +// the wrong ownership branch. +test('a caller-driven abort is NOT reclassified as a transport timeout', async () => { + const controller = new AbortController(); + const transport = new WebDriverTransport({ + clientVersion: '0.0.0-test', + endpoint: 'http://cloud-webdriver.test/wd/hub/', + requestPolicy: { timeoutMs: 30_000, retryAttempts: 0 }, + }); + globalThis.fetch = async (_input, init) => + await new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); + }); + + const pending = transport.requestValue('POST', '/session', {}, { signal: controller.signal }); + await Promise.resolve(); + controller.abort(new Error('client disconnected')); + + await assert.rejects(pending, (error: unknown) => { + assert.equal(isWebDriverRequestTimeout(error), false); + return /client disconnected/.test(error instanceof Error ? error.message : String(error)); + }); +}); + test('cancels a retry delay when the request binding aborts', async () => { const controller = new AbortController(); const transport = new WebDriverTransport({ diff --git a/packages/provider-webdriver/src/webdriver-transport.ts b/packages/provider-webdriver/src/webdriver-transport.ts index dd2d1e83a..735592b34 100644 --- a/packages/provider-webdriver/src/webdriver-transport.ts +++ b/packages/provider-webdriver/src/webdriver-transport.ts @@ -12,8 +12,23 @@ export type WebDriverRequestPolicy = { timeoutMs?: number; retryAttempts?: number; retryDelayMs?: number; + /** + * Budget for creating the session, consumed by `WebDriverClient.createSession` + * rather than the transport: a cloud provider allocates a physical device + * inside that one request, so it cannot share `timeoutMs`, which is sized for + * a settled session's round trips. + */ + sessionCreateTimeoutMs?: number; }; +/** Machine-readable `details.reason` of a request the transport gave up waiting on. */ +const WEBDRIVER_REQUEST_TIMEOUT_REASON = 'webdriver_request_timeout'; + +/** A request the transport stopped waiting on; the server may still complete it. */ +export function isWebDriverRequestTimeout(error: unknown): error is AppError { + return error instanceof AppError && error.details?.reason === WEBDRIVER_REQUEST_TIMEOUT_REASON; +} + export type WebDriverRequestOverrides = { retryAttempts?: number; /** @@ -31,7 +46,8 @@ export type WebDriverTransportOptions = { endpoint: string | URL; auth?: WebDriverAuth; headers?: Record; - requestPolicy?: WebDriverRequestPolicy; + /** Session creation is the client's phase; the transport sees only per-request policy. */ + requestPolicy?: Omit; }; type WebDriverResponse = { @@ -45,11 +61,15 @@ type ResolvedWebDriverRequestOverrides = { signal?: AbortSignal; }; +type ResolvedWebDriverRequestPolicy = Required< + NonNullable +>; + /** Focused HTTP/retry policy for one WebDriver endpoint; session semantics stay in WebDriverClient. */ export class WebDriverTransport { private readonly endpoint: URL; private readonly headers: Record; - private readonly requestPolicy: Required; + private readonly requestPolicy: ResolvedWebDriverRequestPolicy; constructor(options: WebDriverTransportOptions) { this.endpoint = withTrailingSlash(new URL(options.endpoint)); @@ -114,25 +134,49 @@ export class WebDriverTransport { timeoutMs: number, requestSignal?: AbortSignal, ): Promise { - const timeoutSignal = AbortSignal.timeout(timeoutMs); - const signal = requestSignal ? AbortSignal.any([requestSignal, timeoutSignal]) : timeoutSignal; - const response = await fetch(new URL(trimLeadingSlash(path), this.endpoint), { + const { ok, status, text } = await this.fetchWebDriver( method, - headers: { - Accept: 'application/json', - ...(body === undefined ? {} : { 'Content-Type': 'application/json' }), - ...this.headers, - }, - body: body === undefined ? undefined : JSON.stringify(body), - signal, - }); - const text = await response.text(); + path, + body, + timeoutMs, + requestSignal, + ); const payload = text ? parseJsonResponse(text) : {}; - if (!response.ok) { - throw webdriverError(response.status, payload); - } + if (!ok) throw webdriverError(status, payload); return readWebDriverValue(payload); } + + private async fetchWebDriver( + method: string, + path: string, + body: unknown, + timeoutMs: number, + requestSignal?: AbortSignal, + ): Promise & { text: string }> { + const timeoutSignal = AbortSignal.timeout(timeoutMs); + const signal = requestSignal ? AbortSignal.any([requestSignal, timeoutSignal]) : timeoutSignal; + try { + const response = await fetch(new URL(trimLeadingSlash(path), this.endpoint), { + method, + headers: { + Accept: 'application/json', + ...(body === undefined ? {} : { 'Content-Type': 'application/json' }), + ...this.headers, + }, + body: body === undefined ? undefined : JSON.stringify(body), + signal, + }); + return { ok: response.ok, status: response.status, text: await response.text() }; + } catch (error) { + // The caller's own cancellation keeps its reason; only the transport's + // deadline becomes a typed timeout, so callers key on `details.reason` + // instead of sniffing fetch's DOMException name. + if (timeoutSignal.aborted && !requestSignal?.aborted) { + throw webdriverTimeoutError(method, path, timeoutMs, error); + } + throw error; + } + } } function shouldRetryWebDriverRequest( @@ -173,10 +217,25 @@ function webdriverError(status: number, payload: unknown): AppError { return new AppError('COMMAND_FAILED', message, { status, response: payload }); } +function webdriverTimeoutError( + method: string, + path: string, + timeoutMs: number, + cause: unknown, +): AppError { + return new AppError( + 'COMMAND_FAILED', + `WebDriver ${method} ${path} timed out after ${timeoutMs}ms.`, + { reason: WEBDRIVER_REQUEST_TIMEOUT_REASON, method, path, timeoutMs }, + cause instanceof Error ? cause : undefined, + ); +} + function isRetriableWebDriverError(error: unknown): boolean { + if (isWebDriverRequestTimeout(error)) return true; if (error instanceof AppError) { const status = error.details?.status; return typeof status === 'number' && status >= 500; } - return error instanceof TypeError || (error instanceof Error && error.name === 'TimeoutError'); + return error instanceof TypeError; } diff --git a/packages/provider-webdriver/src/webdriver-utils.ts b/packages/provider-webdriver/src/webdriver-utils.ts index a4c887f52..5af69a9f4 100644 --- a/packages/provider-webdriver/src/webdriver-utils.ts +++ b/packages/provider-webdriver/src/webdriver-utils.ts @@ -1,7 +1,22 @@ import type { DeviceLease } from '@agent-device/contracts/device'; +import { AppError, errorMessage } from '@agent-device/kernel/errors'; export type LeaseValue = T | ((lease: DeviceLease) => T); +/** Best-effort release after a failure; a failed release rides along as `cleanupError`, never masks the primary. */ +export async function releaseOnFailure( + primaryError: unknown, + release: () => Promise | undefined, +): Promise { + try { + await release(); + } catch (cleanupError) { + if (primaryError instanceof AppError) { + primaryError.details = { ...primaryError.details, cleanupError: errorMessage(cleanupError) }; + } + } +} + export function resolveLeaseValue( value: LeaseValue | undefined, lease: DeviceLease, diff --git a/src/core/command-descriptor/__tests__/timeout-policy.test.ts b/src/core/command-descriptor/__tests__/timeout-policy.test.ts index ff164aa37..77e0b5d36 100644 --- a/src/core/command-descriptor/__tests__/timeout-policy.test.ts +++ b/src/core/command-descriptor/__tests__/timeout-policy.test.ts @@ -70,6 +70,10 @@ test('daemon-preserving timeout commands are a bounded, reviewed set', () => { // destroyed healthy app sessions. // scroll/back joined in #1638: `--settle` gives them the same post-action // capture loop, so a wedged bridge is now their dominant hang mode too. + // The lease route joined in #1774: those commands act on BILLED provider + // sessions the daemon owns, so a client-side timeout must not SIGKILL the + // daemon mid-create/mid-release and orphan them (and every other provider + // session held). const preserving = commandDescriptors .filter((descriptor) => descriptor.timeoutPolicy.onTimeout === 'preserve-daemon') .map((descriptor) => descriptor.name); @@ -81,6 +85,9 @@ test('daemon-preserving timeout commands are a bounded, reviewed set', () => { 'get', 'hover', 'is', + 'lease_allocate', + 'lease_heartbeat', + 'lease_release', 'longpress', 'press', 'scroll', @@ -133,6 +140,8 @@ test('request envelopes deviating from the default are bounded, reviewed sets', reinstall: 180_000, install_source: 180_000, longpress: 210_000, + // #1774: base allocation budget (300s) + client/daemon race margin (30s). + lease_allocate: 330_000, test: 'unbounded', }; for (const descriptor of commandDescriptors) { diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index a1c0f2858..bcc0093da 100644 --- a/src/core/command-descriptor/registry.ts +++ b/src/core/command-descriptor/registry.ts @@ -8,6 +8,7 @@ import { resolveWaitBudgetMs } from '../wait-positionals.ts'; import { DEFAULT_TIMEOUT_POLICY, INSTALL_REQUEST_TIMEOUT_MS, + LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, PREPARE_REQUEST_TIMEOUT_MS, } from './timeout-policy.ts'; import { resolvePostActionObservationSupport } from './post-action-observation.ts'; @@ -318,6 +319,19 @@ const INSTALL_TIMEOUT_POLICY: CommandTimeoutPolicy = { envelopeMs: INSTALL_REQUEST_TIMEOUT_MS, }; +// Lease-route commands act on billed cloud sessions the daemon owns; resetting +// the daemon on a client timeout would SIGKILL it mid-create/mid-release and +// orphan them all (#1774). Allocation also gets an envelope sized for remote +// device allocation (see LEASE_ALLOCATION_BUDGET_MS). +const LEASE_TIMEOUT_POLICY: CommandTimeoutPolicy = { + ...DEFAULT_TIMEOUT_POLICY, + onTimeout: 'preserve-daemon', +}; +const LEASE_ALLOCATE_TIMEOUT_POLICY: CommandTimeoutPolicy = { + ...LEASE_TIMEOUT_POLICY, + envelopeMs: LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, +}; + const DEFAULT_SETTLE_TIMEOUT_MS = 10_000; // Settle-capable interaction commands also resolve their target through the @@ -397,7 +411,7 @@ export const RAW_COMMAND_DESCRIPTORS = [ catalog: { group: 'internal', key: 'leaseAllocate' }, recordsSessionAction: false, daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT }, - timeoutPolicy: DEFAULT_TIMEOUT_POLICY, + timeoutPolicy: LEASE_ALLOCATE_TIMEOUT_POLICY, batchable: false, platformExecution: NO_PLATFORM_EXECUTION, }, @@ -408,7 +422,7 @@ export const RAW_COMMAND_DESCRIPTORS = [ catalog: { group: 'internal', key: 'leaseHeartbeat' }, recordsSessionAction: false, daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT }, - timeoutPolicy: DEFAULT_TIMEOUT_POLICY, + timeoutPolicy: LEASE_TIMEOUT_POLICY, batchable: false, platformExecution: NO_PLATFORM_EXECUTION, }, @@ -419,7 +433,7 @@ export const RAW_COMMAND_DESCRIPTORS = [ catalog: { group: 'internal', key: 'leaseRelease' }, recordsSessionAction: false, daemon: { route: 'lease', refFrameEffect: 'preserve', ...ADMISSION_AND_LOCK_EXEMPT }, - timeoutPolicy: DEFAULT_TIMEOUT_POLICY, + timeoutPolicy: LEASE_TIMEOUT_POLICY, batchable: false, platformExecution: NO_PLATFORM_EXECUTION, }, diff --git a/src/core/command-descriptor/timeout-policy.ts b/src/core/command-descriptor/timeout-policy.ts index ed9b96984..9bf218cb9 100644 --- a/src/core/command-descriptor/timeout-policy.ts +++ b/src/core/command-descriptor/timeout-policy.ts @@ -11,6 +11,22 @@ export const PREPARE_REQUEST_TIMEOUT_MS = 240_000; // envelope does not abort a still-progressing device install first. export const INSTALL_REQUEST_TIMEOUT_MS = 180_000; +// Margin over a daemon-side budget so the daemon's own timeout result (with +// diagnostics) wins the race against the client envelope. Never shrinks the +// envelope below the command's declared base. +export const REQUEST_TIMEOUT_BUDGET_MARGIN_MS = 30_000; + +/** + * How long a lease lifecycle provider may spend allocating one lease (cloud + * device allocation: BrowserStack iOS ~45–90s, AWS remote access ~2 min to + * RUNNING). Handed to providers as `LeaseLifecycleContext.deadline`; the + * client's `lease_allocate` envelope derives from it so they cannot drift. + */ +export const LEASE_ALLOCATION_BUDGET_MS = 300_000; + +export const LEASE_ALLOCATE_REQUEST_TIMEOUT_MS = + LEASE_ALLOCATION_BUDGET_MS + REQUEST_TIMEOUT_BUDGET_MARGIN_MS; + /** * The timeout policy most commands share: standard envelope, no user-supplied * budget, and a daemon reset on timeout (a hung request usually means daemon diff --git a/src/core/device-inventory-context.ts b/src/core/device-inventory-context.ts index eedff4f60..41d62115b 100644 --- a/src/core/device-inventory-context.ts +++ b/src/core/device-inventory-context.ts @@ -1,3 +1,4 @@ +import { AppError, isRequestCanceledError } from '@agent-device/kernel/errors'; import type { DeviceInventoryRequest } from '@agent-device/contracts/device'; import { type DeviceInventoryDiscovery, @@ -6,9 +7,7 @@ import { type ProviderAwareDeviceInventoryGateway, } from '@agent-device/contracts/platform'; import type { DeviceInfo } from '@agent-device/kernel/device'; -import { AppError } from '@agent-device/kernel/errors'; import { AsyncLocalStorage } from 'node:async_hooks'; -import { isRequestCanceledError } from '../request/cancel.ts'; const DEVICE_INVENTORY_CONTEXT_UNAVAILABLE_REASON = 'device_inventory_context_unavailable'; diff --git a/src/daemon/__tests__/request-handler-catalog.test.ts b/src/daemon/__tests__/request-handler-catalog.test.ts index 74ea11d5b..09e31d5f6 100644 --- a/src/daemon/__tests__/request-handler-catalog.test.ts +++ b/src/daemon/__tests__/request-handler-catalog.test.ts @@ -4,6 +4,7 @@ import path from 'node:path'; import { test } from 'vitest'; import { withTestDeviceInventoryProvider as withTargetDeviceResolutionScope } from '../../__tests__/test-utils/device-inventory-gateways.ts'; import { INTERNAL_COMMANDS, PUBLIC_COMMANDS } from '../../command-catalog.ts'; +import { isRequestCanceledError, type AppError } from '@agent-device/kernel/errors'; import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; import { getDaemonCommandRoute, type DaemonCommandRoute } from '../daemon-command-registry.ts'; import { cleanupDownloadableArtifact, trackDownloadableArtifact } from '../artifact-tracking.ts'; @@ -11,6 +12,11 @@ import { contextFromFlags } from '../context.ts'; import { handleLeaseCommands } from '../handlers/lease.ts'; import { LeaseRegistry } from '../lease-registry.ts'; import { runRequestHandlerChain } from '../request-handler-chain.ts'; +import { + clearRequestAbortRegistration, + markRequestCanceled, + registerRequestAbort, +} from '../../request/cancel.ts'; import { unavailableBindDevice, unavailableBindExactDevice, @@ -235,6 +241,215 @@ test('lease release calls provider hook using the released lease without heartbe }); }); +// #1774: allocation hands the provider the request's cancellation signal and a +// deadline so a cloud provider can bound its remote device-creation phase and, +// on client disconnect, release the billed session it produced instead of +// orphaning it. If the daemon dropped either, the provider would have no way to +// know the requester is gone. +test('lease allocation hands the provider the request-bound signal and a deadline', async () => { + const leaseRegistry = new LeaseRegistry(); + const sessionStore = makeSessionStore('agent-device-lease-ownership-'); + // The signal must be the one bound to THIS request id (aborted on cancel or + // client disconnect), so the provider can release a session it produced for a + // requester that left — not an inert placeholder. + const requestId = 'lease-alloc-cancel-req'; + const registration = registerRequestAbort(requestId); + const before = Date.now(); + let observed: { signal?: AbortSignal; deadline?: number } | undefined; + + try { + const response = await handleLeaseCommands({ + req: { + command: INTERNAL_COMMANDS.leaseAllocate, + token: 'test-token', + session: 'catalog-test', + meta: { + requestId, + tenantId: 'tenant-a', + runId: 'run-a', + leaseBackend: 'android-instance', + leaseProvider: 'fake-provider', + }, + positionals: [], + }, + sessionName: 'catalog-test', + sessionStore, + leaseRegistry, + leaseLifecycleProvider: { + allocate: async (_lease, context) => { + observed = { signal: context?.signal, deadline: context?.deadline }; + return { provider: 'fake-provider' }; + }, + }, + }); + + assert.equal(response?.ok, true); + assert.equal(observed?.signal?.aborted, false); + markRequestCanceled(requestId); + assert.equal(observed?.signal?.aborted, true, 'the provider signal must track this request'); + assert.ok( + typeof observed?.deadline === 'number' && observed.deadline > before, + `allocation deadline must be a future instant, got ${String(observed?.deadline)}`, + ); + } finally { + clearRequestAbortRegistration(registration); + } +}); + +// #1774: the classic leak. The requester vanishes WHILE the provider is +// allocating; the provider still returns a real, billed session. The daemon +// owns the request, so it releases that lease immediately — provider AND +// registry — and answers with a canceled error carrying the release evidence. +test('a lease allocated for a requester that left is released, not registered', async () => { + const leaseRegistry = new LeaseRegistry(); + const sessionStore = makeSessionStore('agent-device-lease-gone-'); + const requestId = 'lease-alloc-gone'; + const registration = registerRequestAbort(requestId); + const released: string[] = []; + + try { + await assert.rejects( + () => + handleLeaseCommands({ + req: leaseAllocateRequest(requestId), + sessionName: 'catalog-test', + sessionStore, + leaseRegistry, + leaseLifecycleProvider: { + allocate: async (lease) => { + // The client disconnects mid-allocation; the provider finishes anyway. + markRequestCanceled(requestId); + return { providerSessionId: `bs-${lease.leaseId}` }; + }, + release: async (lease) => { + released.push(lease.leaseId); + return { providerSessionId: `bs-${lease.leaseId}` }; + }, + }, + }), + (error: unknown) => { + assert.ok(isRequestCanceledError(error)); + const details = (error as AppError).details ?? {}; + assert.equal(details.released, true); + assert.equal(details.registryReleased, true); + assert.match(String(details.providerSessionId), /^bs-/); + return true; + }, + ); + assert.equal(released.length, 1, 'the provider must be asked to release the lease'); + assert.equal( + leaseRegistry.getLease({ tenantId: 'tenant-a', runId: 'run-a', leaseId: released[0]! }), + undefined, + ); + } finally { + clearRequestAbortRegistration(registration); + } +}); + +// A release that could not delete the provider session must NOT be reported as +// released: the billed session may still be running, and the operator needs the +// identifiers to stop it by hand. +test('a failed provider release after cancellation is reported as unreleased with recovery evidence', async () => { + const leaseRegistry = new LeaseRegistry(); + const sessionStore = makeSessionStore('agent-device-lease-gone-unreleased-'); + const requestId = 'lease-alloc-gone-delete-failed'; + const registration = registerRequestAbort(requestId); + + try { + await assert.rejects( + () => + handleLeaseCommands({ + req: leaseAllocateRequest(requestId), + sessionName: 'catalog-test', + sessionStore, + leaseRegistry, + leaseLifecycleProvider: { + allocate: async () => { + markRequestCanceled(requestId); + return { providerSessionId: 'bs-live' }; + }, + release: async () => ({ + providerSessionId: 'bs-live', + warnings: [{ code: 'WEBDRIVER_SESSION_DELETE_FAILED', message: 'HTTP 502' }], + }), + }, + }), + (error: unknown) => { + assert.ok(isRequestCanceledError(error)); + const details = (error as AppError).details ?? {}; + // `released` is the operator's answer: the billed session is NOT confirmed + // gone. Daemon bookkeeping is a separate, unambiguous key. + assert.equal(details.released, false); + assert.equal(details.registryReleased, true); + assert.equal(details.providerSessionId, 'bs-live'); + assert.match(String(details.hint), /could NOT be confirmed released/); + assert.match(String(details.hint), /bs-live/); + assert.deepEqual(details.warnings, [ + { code: 'WEBDRIVER_SESSION_DELETE_FAILED', message: 'HTTP 502' }, + ]); + return true; + }, + ); + } finally { + clearRequestAbortRegistration(registration); + } +}); + +test('a throwing provider release after cancellation is reported as unreleased, not raised', async () => { + const leaseRegistry = new LeaseRegistry(); + const sessionStore = makeSessionStore('agent-device-lease-gone-throw-'); + const requestId = 'lease-alloc-gone-release-threw'; + const registration = registerRequestAbort(requestId); + + try { + await assert.rejects( + () => + handleLeaseCommands({ + req: leaseAllocateRequest(requestId), + sessionName: 'catalog-test', + sessionStore, + leaseRegistry, + leaseLifecycleProvider: { + allocate: async () => { + markRequestCanceled(requestId); + return { providerSessionId: 'bs-live' }; + }, + release: async () => { + throw new Error('hub unreachable'); + }, + }, + }), + (error: unknown) => { + assert.ok(isRequestCanceledError(error), 'nobody is left to receive the release failure'); + const details = (error as AppError).details ?? {}; + assert.equal(details.released, false); + assert.equal(details.registryReleased, true); + assert.equal(details.releaseError, 'hub unreachable'); + assert.match(String(details.hint), /could NOT be confirmed released/); + return true; + }, + ); + } finally { + clearRequestAbortRegistration(registration); + } +}); + +function leaseAllocateRequest(requestId: string): DaemonRequest { + return { + command: INTERNAL_COMMANDS.leaseAllocate, + token: 'test-token', + session: 'catalog-test', + meta: { + requestId, + tenantId: 'tenant-a', + runId: 'run-a', + leaseBackend: 'android-instance', + leaseProvider: 'fake-provider', + }, + positionals: [], + }; +} + function catalogCommandsForRoute(route: Exclude): string[] { return [...Object.values(PUBLIC_COMMANDS), ...Object.values(INTERNAL_COMMANDS)].filter( (command) => getDaemonCommandRoute(command) === route, diff --git a/src/daemon/adapters/maestro/daemon-runtime-port-observation.ts b/src/daemon/adapters/maestro/daemon-runtime-port-observation.ts index 8746b2c63..96a886f91 100644 --- a/src/daemon/adapters/maestro/daemon-runtime-port-observation.ts +++ b/src/daemon/adapters/maestro/daemon-runtime-port-observation.ts @@ -1,3 +1,4 @@ +import { createRequestCanceledError, AppError } from '@agent-device/kernel/errors'; import { createHash } from 'node:crypto'; import { literalFromMaestroRegex, @@ -16,12 +17,10 @@ import { type MaestroTargetQuery, } from '@agent-device/maestro'; import { attachRefs } from '@agent-device/kernel/snapshot'; -import { createRequestCanceledError } from '../../../request/cancel.ts'; import { getSnapshotReferenceFrame, type TouchReferenceFrame, } from '../../touch-reference-frame.ts'; -import { AppError } from '@agent-device/kernel/errors'; import { isPositiveFiniteRect, rectContains } from '@agent-device/kernel/rect'; import type { Rect, SnapshotState } from '@agent-device/kernel/snapshot'; import { buildIosInteractiveSnapshotPresentation } from '../../snapshot-presentation/ios/index.ts'; diff --git a/src/daemon/adapters/maestro/maestro-screenshot-comparison.ts b/src/daemon/adapters/maestro/maestro-screenshot-comparison.ts index fe1007978..adb1535e1 100644 --- a/src/daemon/adapters/maestro/maestro-screenshot-comparison.ts +++ b/src/daemon/adapters/maestro/maestro-screenshot-comparison.ts @@ -2,7 +2,7 @@ import { promises as fs } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { MAESTRO_RUNTIME_ADAPTER_POLICY } from '@agent-device/maestro'; -import { createRequestCanceledError, isRequestCanceledError } from '../../../request/cancel.ts'; +import { createRequestCanceledError, isRequestCanceledError } from '@agent-device/kernel/errors'; import { emitDiagnostic } from '../../../utils/diagnostics.ts'; import { computePngRgbDifferenceAsync } from '../../../utils/png-worker-client.ts'; import type { PngRgbDifferenceResult } from '../../../utils/png-rgb-difference.ts'; diff --git a/src/daemon/client/daemon-client-timeout.ts b/src/daemon/client/daemon-client-timeout.ts index 7fd723c00..0f5a7b6ac 100644 --- a/src/daemon/client/daemon-client-timeout.ts +++ b/src/daemon/client/daemon-client-timeout.ts @@ -4,6 +4,7 @@ import { emitDiagnostic } from '../../utils/diagnostics.ts'; import { isAgentDeviceDaemonProcess } from '../daemon-process.ts'; import { PUBLIC_COMMANDS } from '../../command-catalog.ts'; import { resolveCommandTimeoutPolicy } from '../../core/command-descriptor/registry.ts'; +import { REQUEST_TIMEOUT_BUDGET_MARGIN_MS } from '../../core/command-descriptor/timeout-policy.ts'; import type { CommandTimeoutBudget, CommandTimeoutPolicy, @@ -235,8 +236,3 @@ function resetDaemonAfterTimeout(info: DaemonInfo, paths: DaemonPaths): { forced } return { forcedKill }; } - -// Margin over a user-supplied budget so the daemon-side timeout result (with -// diagnostics) wins the race against the client envelope. Never shrinks the -// envelope below the command's declared base. -const REQUEST_TIMEOUT_BUDGET_MARGIN_MS = 30_000; diff --git a/src/daemon/handlers/lease.ts b/src/daemon/handlers/lease.ts index 9a7920fcb..9b235d878 100644 --- a/src/daemon/handlers/lease.ts +++ b/src/daemon/handlers/lease.ts @@ -1,11 +1,15 @@ -import type { LeaseLifecycleContext, LeaseLifecycleProvider } from '@agent-device/contracts/device'; +import type { + DeviceLease, + LeaseLifecycleContext, + LeaseLifecycleProvider, +} from '@agent-device/contracts/device'; import { PUBLIC_COMMANDS } from '../../command-catalog.ts'; import type { AgentArtifactsResult, CloudArtifactProvider, } from '@agent-device/contracts/observability'; import type { DaemonRequest, DaemonResponse } from '../types.ts'; -import type { LeaseRegistry } from '../lease-registry.ts'; +import type { LeaseRegistry, ReleaseLeaseRequest } from '../lease-registry.ts'; import type { SessionStore } from '../session-store.ts'; import { isProxyLeaseScope, @@ -17,7 +21,9 @@ import { leaseScopeToHeartbeatRequest, leaseScopeToReleaseRequest, } from '../../core/lease-scope.ts'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError, errorMessage } from '@agent-device/kernel/errors'; +import { LEASE_ALLOCATION_BUDGET_MS } from '../../core/command-descriptor/timeout-policy.ts'; +import { getRequestSignal, isRequestCanceled } from '../../request/cancel.ts'; import { listDownloadableArtifacts } from '../artifact-tracking.ts'; type LeaseHandlerArgs = { @@ -63,21 +69,20 @@ export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise | undefined; try { - providerData = await leaseLifecycleProvider?.allocate?.(lease, leaseLifecycleContext(req)); + providerData = await leaseLifecycleProvider?.allocate?.(lease, { + ...leaseLifecycleContext(req), + signal: getRequestSignal(req.meta?.requestId), + deadline: Date.now() + LEASE_ALLOCATION_BUDGET_MS, + }); } catch (error) { - leaseRegistry.releaseLease( - leaseScopeToReleaseRequest({ - leaseId: lease.leaseId, - tenantId: lease.tenantId, - runId: lease.runId, - leaseBackend: lease.backend, - leaseProvider: lease.leaseProvider, - deviceKey: lease.deviceKey, - clientId: lease.clientId, - }), - ); + leaseRegistry.releaseLease(leaseReleaseRequestFor(lease)); throw error; } + if (isRequestCanceled(req.meta?.requestId)) { + // The requester left while the provider was allocating; the lease it + // produced is real (and billed) and nobody will ever release it. + throw await releaseAllocationForGoneRequester(lease, leaseLifecycleProvider, leaseRegistry); + } return { ok: true, data: { lease, ...(providerData ? { provider: providerData } : {}) }, @@ -96,14 +101,20 @@ export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise; +}; + +/** THE release path: provider first (it still needs the lease record), then the registry. */ +async function releaseLease( + leaseRegistry: LeaseRegistry, + leaseLifecycleProvider: LeaseLifecycleProvider | undefined, + lease: DeviceLease | undefined, + request: ReleaseLeaseRequest, + context?: LeaseLifecycleContext, +): Promise { + const provider = lease ? await leaseLifecycleProvider?.release?.(lease, context) : undefined; + return { registryReleased: leaseRegistry.releaseLease(request).released, provider }; +} + +/** + * Releases a lease that finished allocating after its requester was gone and + * turns the outcome into the canceled-request error nobody is left to receive: + * a throwing provider release is folded into `releaseError` rather than raised, + * and the provider session counts as released only when it reported no + * warnings — otherwise the error names what an operator must stop by hand. + */ +async function releaseAllocationForGoneRequester( + lease: DeviceLease, + leaseLifecycleProvider: LeaseLifecycleProvider | undefined, + leaseRegistry: LeaseRegistry, +): Promise { + const request = leaseReleaseRequestFor(lease); + let outcome: LeaseReleaseOutcome; + let releaseError: string | undefined; + try { + outcome = await releaseLease(leaseRegistry, leaseLifecycleProvider, lease, request); + } catch (error) { + releaseError = errorMessage(error); + outcome = { registryReleased: leaseRegistry.releaseLease(request).released }; + } + return canceledAllocationError(lease, outcome, releaseError); +} + +function canceledAllocationError( + lease: DeviceLease, + outcome: LeaseReleaseOutcome, + releaseError: string | undefined, +): AppError { + const providerSessionId = outcome.provider?.providerSessionId; + const warnings = Array.isArray(outcome.provider?.warnings) ? outcome.provider.warnings : []; + // `released` answers the operator's question — is the billed session gone? — + // and is only true when the provider released without warnings or throwing. + const released = outcome.registryReleased && releaseError === undefined && warnings.length === 0; + return createRequestCanceledError({ + leaseId: lease.leaseId, + leaseProvider: lease.leaseProvider, + released, + registryReleased: outcome.registryReleased, + providerSessionId, + ...(warnings.length > 0 ? { warnings } : {}), + ...(releaseError !== undefined ? { releaseError } : {}), + hint: canceledAllocationHint(released, lease.leaseId, providerSessionId), + }); +} + +function canceledAllocationHint( + released: boolean, + leaseId: string, + providerSessionId: unknown, +): string { + if (released) { + return 'The lease request was canceled while the provider was still allocating; the session it produced was released.'; + } + return `The lease request was canceled while the provider was still allocating, and the session it produced could NOT be confirmed released — it may still be running and billing. Stop provider session ${String(providerSessionId ?? '(unknown)')} for lease ${leaseId} by hand.`; +} + function leaseLifecycleContext(req: DaemonRequest): LeaseLifecycleContext { return { flags: req.flags, diff --git a/src/daemon/handlers/session-open-execution.ts b/src/daemon/handlers/session-open-execution.ts index 574112c9b..f5fc22721 100644 --- a/src/daemon/handlers/session-open-execution.ts +++ b/src/daemon/handlers/session-open-execution.ts @@ -12,7 +12,8 @@ import { armAuthoringOnOpen, isAuthoringArmedSession, } from '../session-script-publication-capability.ts'; -import { createRequestCanceledError, isRequestCanceled } from '../../request/cancel.ts'; +import { isRequestCanceled } from '../../request/cancel.ts'; +import { createRequestCanceledError } from '@agent-device/kernel/errors'; import { resolveSessionRequestLogPath, resolveSessionRunnerLogPath, diff --git a/src/daemon/server/transport.ts b/src/daemon/server/transport.ts index 9b93451f1..d5cbe7595 100644 --- a/src/daemon/server/transport.ts +++ b/src/daemon/server/transport.ts @@ -1,10 +1,9 @@ +import { AppError, normalizeError, createRequestCanceledError } from '@agent-device/kernel/errors'; import net from 'node:net'; import type { Server as HttpServer } from 'node:http'; -import { AppError, normalizeError } from '@agent-device/kernel/errors'; import type { DaemonInvokeFn, DaemonRequest, DaemonResponse } from '../types.ts'; import { clearRequestAbortRegistration, - createRequestCanceledError, isRequestCanceled, markRequestCanceled, registerRequestAbort, diff --git a/src/platforms/apple/core/__tests__/runner-client.test.ts b/src/platforms/apple/core/__tests__/runner-client.test.ts index c22152d1c..cab25794c 100644 --- a/src/platforms/apple/core/__tests__/runner-client.test.ts +++ b/src/platforms/apple/core/__tests__/runner-client.test.ts @@ -1,3 +1,8 @@ +import { + createRequestCanceledError, + isRequestCanceledError, + AppError, +} from '@agent-device/kernel/errors'; import type { RequestProgressEvent } from '@agent-device/contracts/progress'; import { beforeEach, test, onTestFinished, vi } from 'vitest'; import assert from 'node:assert/strict'; @@ -42,12 +47,10 @@ vi.mock('../../../../utils/host-process.ts', async (importOriginal) => { import type { DeviceInfo } from '@agent-device/kernel/device'; import { withRequestProgressSink } from '../../../../request/progress.ts'; -import { createRequestCanceledError, isRequestCanceledError } from '../../../../request/cancel.ts'; import { flushDiagnosticsToSessionFile, withDiagnosticsScope, } from '../../../../utils/diagnostics.ts'; -import { AppError } from '@agent-device/kernel/errors'; import { isReadOnlyRunnerCommand } from '../runner/runner-command-traits.ts'; import { isRetryableRunnerError, diff --git a/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts b/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts index 16ee6dfe5..e0c10caea 100644 --- a/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts +++ b/src/platforms/apple/core/__tests__/runner-request-cancellation.test.ts @@ -101,12 +101,11 @@ vi.mock('../runner/runner-xctestrun.ts', async () => { import { clearRequestCanceled, - createRequestCanceledError, getRequestSignal, - isRequestCanceledError, markRequestCanceled, registerRequestAbort, } from '../../../../request/cancel.ts'; +import { createRequestCanceledError, isRequestCanceledError } from '@agent-device/kernel/errors'; import { abortAllIosRunnerSessions, getRunnerSessionSnapshot } from '../runner/runner-session.ts'; import { setRunnerLeaseOwnerStateDir, type RunnerLease } from '../runner/runner-lease.ts'; import { executeRunnerCommand, prepareLocalIosRunner } from '../runner/runner-lifecycle.ts'; diff --git a/src/platforms/apple/core/runner/runner-artifact.ts b/src/platforms/apple/core/runner/runner-artifact.ts index 17ef0bf3a..7e8131e3e 100644 --- a/src/platforms/apple/core/runner/runner-artifact.ts +++ b/src/platforms/apple/core/runner/runner-artifact.ts @@ -1,12 +1,11 @@ +import { AppError, isRequestCanceledError } from '@agent-device/kernel/errors'; import fs from 'node:fs'; import crypto from 'node:crypto'; import os from 'node:os'; import path from 'node:path'; -import { AppError } from '@agent-device/kernel/errors'; import { runCmdStreaming, type ExecBackgroundResult } from '../../../../utils/exec.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { withKeyedLock } from '../../../../utils/keyed-lock.ts'; -import { isRequestCanceledError } from '../../../../request/cancel.ts'; import { emitRequestProgress } from '../../../../request/progress.ts'; import { findProjectRoot } from '../../../../utils/version.ts'; import { resolveRunnerBuildFailureHint } from './runner-contract.ts'; diff --git a/src/platforms/apple/core/runner/runner-contract.ts b/src/platforms/apple/core/runner/runner-contract.ts index d0d2c4b1c..2b5c61849 100644 --- a/src/platforms/apple/core/runner/runner-contract.ts +++ b/src/platforms/apple/core/runner/runner-contract.ts @@ -1,3 +1,4 @@ +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import crypto from 'node:crypto'; import type { DeviceRotation } from '@agent-device/contracts/device'; import type { @@ -6,12 +7,7 @@ import type { GesturePlan, ScrollDirection, } from '@agent-device/contracts/interaction'; -import { AppError } from '@agent-device/kernel/errors'; -import { - createRequestCanceledError, - getRequestSignal, - isRequestCanceled, -} from '../../../../request/cancel.ts'; +import { getRequestSignal, isRequestCanceled } from '../../../../request/cancel.ts'; import { bootFailureHint, classifyBootFailure, diff --git a/src/platforms/apple/core/runner/runner-lifecycle.ts b/src/platforms/apple/core/runner/runner-lifecycle.ts index 897caa716..6f705aa84 100644 --- a/src/platforms/apple/core/runner/runner-lifecycle.ts +++ b/src/platforms/apple/core/runner/runner-lifecycle.ts @@ -1,7 +1,6 @@ -import { AppError, asAppError } from '@agent-device/kernel/errors'; +import { AppError, asAppError, isRequestCanceledError } from '@agent-device/kernel/errors'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { emitDiagnostic } from '../../../../utils/diagnostics.ts'; -import { isRequestCanceledError } from '../../../../request/cancel.ts'; import { RUNNER_STARTUP_TIMEOUT_MS } from './runner-startup-transport.ts'; import { RUNNER_COMMAND_TIMEOUT_MS } from './runner-transport.ts'; import { diff --git a/src/platforms/apple/core/runner/runner-session.ts b/src/platforms/apple/core/runner/runner-session.ts index 632dcdced..583b60031 100644 --- a/src/platforms/apple/core/runner/runner-session.ts +++ b/src/platforms/apple/core/runner/runner-session.ts @@ -1,4 +1,4 @@ -import { AppError, toAppErrorCode } from '@agent-device/kernel/errors'; +import { AppError, toAppErrorCode, createRequestCanceledError } from '@agent-device/kernel/errors'; import { runCmdBackground, type ExecResult, @@ -10,7 +10,6 @@ import { isIosFamily, isApplePlatform, type DeviceInfo } from '@agent-device/ker import type { RunnerLogicalLeaseContext } from '@agent-device/contracts/platform'; import type { AppleRunnerLifecycleOptions } from './runner-provider.ts'; import { emitRequestProgress } from '../../../../request/progress.ts'; -import { createRequestCanceledError } from '../../../../request/cancel.ts'; import { emitDiagnostic, withDiagnosticTimer } from '../../../../utils/diagnostics.ts'; import { buildSimctlArgsForDevice } from '../simctl.ts'; import { runAppleToolCommand, runXcrun } from '../tool-provider.ts'; diff --git a/src/platforms/apple/core/runner/runner-startup-transport.ts b/src/platforms/apple/core/runner/runner-startup-transport.ts index 93a05e9ec..7df0522d6 100644 --- a/src/platforms/apple/core/runner/runner-startup-transport.ts +++ b/src/platforms/apple/core/runner/runner-startup-transport.ts @@ -1,5 +1,8 @@ -import { createRequestCanceledError, isRequestCanceledError } from '../../../../request/cancel.ts'; -import { AppError } from '@agent-device/kernel/errors'; +import { + createRequestCanceledError, + isRequestCanceledError, + AppError, +} from '@agent-device/kernel/errors'; import { requireExecSuccess } from '../../../../utils/exec.ts'; import { Deadline, retryWithPolicy } from '../../../../utils/retry.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; diff --git a/src/platforms/apple/core/runner/runner-transport.ts b/src/platforms/apple/core/runner/runner-transport.ts index 3db2d8e39..ed089831f 100644 --- a/src/platforms/apple/core/runner/runner-transport.ts +++ b/src/platforms/apple/core/runner/runner-transport.ts @@ -1,5 +1,4 @@ -import { createRequestCanceledError } from '../../../../request/cancel.ts'; -import { AppError } from '@agent-device/kernel/errors'; +import { createRequestCanceledError, AppError } from '@agent-device/kernel/errors'; import { Deadline } from '../../../../utils/retry.ts'; import type { DeviceInfo } from '@agent-device/kernel/device'; import { resolveIosPhysicalDeviceControl } from '../physical-device-control.ts'; diff --git a/src/platforms/apple/core/runner/runner-usbmux-protocol.ts b/src/platforms/apple/core/runner/runner-usbmux-protocol.ts index c8d501fe7..b7ca5fa1e 100644 --- a/src/platforms/apple/core/runner/runner-usbmux-protocol.ts +++ b/src/platforms/apple/core/runner/runner-usbmux-protocol.ts @@ -1,7 +1,6 @@ +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import net, { type Socket } from 'node:net'; -import { AppError } from '@agent-device/kernel/errors'; import { escapeXmlTextAndAttribute, parseXmlDocumentSync, type XmlNode } from '@agent-device/xml'; -import { createRequestCanceledError } from '../../../../request/cancel.ts'; import { Deadline } from '../../../../utils/retry.ts'; const USBMUX_HEADER_BYTES = 16; diff --git a/src/platforms/apple/core/runner/runner-usbmux.ts b/src/platforms/apple/core/runner/runner-usbmux.ts index 06839073e..dbaf575ab 100644 --- a/src/platforms/apple/core/runner/runner-usbmux.ts +++ b/src/platforms/apple/core/runner/runner-usbmux.ts @@ -1,7 +1,6 @@ +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import http, { type IncomingMessage } from 'node:http'; import { type Socket } from 'node:net'; -import { AppError } from '@agent-device/kernel/errors'; -import { createRequestCanceledError } from '../../../../request/cancel.ts'; import { Deadline } from '../../../../utils/retry.ts'; import type { RunnerCommand } from './runner-contract.ts'; import { openUsbmuxRunnerSocket } from './runner-usbmux-protocol.ts'; diff --git a/src/platforms/install-source-network.ts b/src/platforms/install-source-network.ts index e02be00d0..d135e95c1 100644 --- a/src/platforms/install-source-network.ts +++ b/src/platforms/install-source-network.ts @@ -1,6 +1,10 @@ import dns from 'node:dns/promises'; import net from 'node:net'; -import { AppError } from '@agent-device/kernel/errors'; +import { + AppError, + createRequestCanceledError, + isRequestCanceledError, +} from '@agent-device/kernel/errors'; import ipaddr from 'ipaddr.js'; export async function approveDownloadSourceUrl( @@ -26,7 +30,7 @@ export async function approveDownloadSourceUrl( try { resolved = await lookupWithSignal(hostname, signal); } catch (error) { - if (error instanceof AppError && error.details?.reason === 'request_canceled') throw error; + if (isRequestCanceledError(error)) throw error; throw new AppError( 'INVALID_ARGS', `Source URL host could not be resolved: ${hostname}`, @@ -62,7 +66,7 @@ function throwIfAborted(signal: AbortSignal | undefined): void { } function canceledError(cause: unknown): AppError { - return new AppError('COMMAND_FAILED', 'request canceled', { reason: 'request_canceled' }, cause); + return createRequestCanceledError(undefined, cause); } export function isBlockedSourceHostname(hostname: string): boolean { diff --git a/src/platforms/install-source.ts b/src/platforms/install-source.ts index 9555ef78f..a01c9de56 100644 --- a/src/platforms/install-source.ts +++ b/src/platforms/install-source.ts @@ -1,7 +1,7 @@ import { promises as fs } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { expandUserHomePath } from '../utils/path-resolution.ts'; import { ArchiveBudget } from '../utils/archive-safety.ts'; import { resolveInstallableCandidate } from './install-source-archive.ts'; @@ -127,7 +127,7 @@ async function downloadToTempFile( ): Promise { const requestSignal = options?.signal; if (requestSignal?.aborted) { - throw new AppError('COMMAND_FAILED', 'request canceled', { reason: 'request_canceled' }); + throw createRequestCanceledError(); } const timeoutMs = options?.downloadTimeoutMs ?? DEFAULT_SOURCE_DOWNLOAD_TIMEOUT_MS; const timeoutSignal = AbortSignal.timeout(timeoutMs); @@ -146,12 +146,7 @@ function classifyDownloadError( timeoutMs: number, ): unknown { if (requestSignal?.aborted) { - return new AppError( - 'COMMAND_FAILED', - 'request canceled', - { reason: 'request_canceled' }, - error, - ); + return createRequestCanceledError(undefined, error); } if (timeoutSignal.aborted) { return new AppError( diff --git a/src/request/cancel.test.ts b/src/request/cancel.test.ts index f9950f965..0104b58a8 100644 --- a/src/request/cancel.test.ts +++ b/src/request/cancel.test.ts @@ -1,9 +1,14 @@ import { test } from 'vitest'; import assert from 'node:assert/strict'; -import { AppError } from '@agent-device/kernel/errors'; import { + AppError, createRequestCanceledError, isRequestCanceledError, +} from '@agent-device/kernel/errors'; +import { + clearRequestAbortRegistration, + markRequestCanceled, + registerRequestAbort, resolveRequestTrackingId, } from './cancel.ts'; @@ -23,8 +28,45 @@ test('createRequestCanceledError includes stable cancellation reason marker', () assert.match(String(err.details?.hint), /canceled intentionally/); }); +// The factory is the ONE way to build a canceled error (#1774 consolidated the +// hand-rolled copies): callers may add evidence or a sharper hint, but cannot +// build one the predicate misses, and the cause survives for diagnostics. +test('createRequestCanceledError carries caller evidence but never loses its reason', () => { + const cause = new Error('socket closed'); + const err = createRequestCanceledError( + { releasedSessionId: 'wd-1', hint: 'released it', reason: 'something_else' }, + cause, + ); + assert.equal(err.details?.reason, 'request_canceled'); + assert.equal(err.details?.releasedSessionId, 'wd-1'); + assert.equal(err.details?.hint, 'released it'); + assert.equal(err.cause, cause); + assert.equal(isRequestCanceledError(err), true); + assert.equal(isRequestCanceledError(new AppError('UNKNOWN', 'request canceled')), false); +}); + test('isRequestCanceledError accepts structured and legacy cancellation errors', () => { assert.equal(isRequestCanceledError(createRequestCanceledError()), true); assert.equal(isRequestCanceledError(new AppError('COMMAND_FAILED', 'request canceled')), true); assert.equal(isRequestCanceledError(new AppError('COMMAND_FAILED', 'different message')), false); }); + +// A canceled request's signal carries the typed error as its reason, so every +// `signal.throwIfAborted()`, aborted fetch, and `throw signal.reason` downstream +// reports a canceled request rather than a bare DOMException that normalizes to +// UNKNOWN — without each site having to know the factory exists. +test('a canceled request aborts its signal with the typed canceled error', () => { + const registration = registerRequestAbort('req-typed-abort'); + try { + markRequestCanceled('req-typed-abort'); + const signal = registration!.controller.signal; + assert.equal(signal.aborted, true); + assert.equal(isRequestCanceledError(signal.reason), true); + assert.throws( + () => signal.throwIfAborted(), + (error: unknown) => isRequestCanceledError(error), + ); + } finally { + clearRequestAbortRegistration(registration); + } +}); diff --git a/src/request/cancel.ts b/src/request/cancel.ts index a24f8c69f..c5f07a3be 100644 --- a/src/request/cancel.ts +++ b/src/request/cancel.ts @@ -1,11 +1,7 @@ -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; const canceledRequestIds = new Set(); const requestAbortControllers = new Map(); -const REQUEST_CANCELED_REASON = 'request_canceled'; -const REQUEST_CANCELED_MESSAGE = 'request canceled'; -const REQUEST_CANCELED_HINT = - 'The request was canceled intentionally (explicit cancel or client disconnect) — no retry is needed unless the cancellation was unintended.'; export type RequestAbortRegistration = { requestId: string; @@ -78,7 +74,11 @@ export function markRequestCanceled(requestId: string | undefined): void { if (!requestId) return; evictOldestSetEntries(canceledRequestIds); canceledRequestIds.add(requestId); - requestAbortControllers.get(requestId)?.abort(); + // Abort WITH the typed error as the reason: every `signal.throwIfAborted()`, + // fetch, and `throw signal.reason` downstream then surfaces the canceled + // request as such, instead of a bare DOMException that normalizes to UNKNOWN. + // No call site has to remember the factory — the signal carries it. + requestAbortControllers.get(requestId)?.abort(createRequestCanceledError()); } export function clearRequestCanceled( @@ -114,22 +114,8 @@ export function getRequestSignal(requestId: string | undefined): AbortSignal | u return requestAbortControllers.get(requestId)?.signal; } -export function createRequestCanceledError(): AppError { - return new AppError('COMMAND_FAILED', REQUEST_CANCELED_MESSAGE, { - reason: REQUEST_CANCELED_REASON, - hint: REQUEST_CANCELED_HINT, - }); -} - export function throwIfRequestCanceled(requestId: string | undefined): void { if (isRequestCanceled(requestId)) { throw createRequestCanceledError(); } } - -export function isRequestCanceledError(error: unknown): boolean { - if (!(error instanceof AppError)) return false; - if (error.code !== 'COMMAND_FAILED') return false; - if (error.details?.reason === REQUEST_CANCELED_REASON) return true; - return error.message === REQUEST_CANCELED_MESSAGE; -} diff --git a/src/utils/exec.ts b/src/utils/exec.ts index cdb05148e..3bf80eb7d 100644 --- a/src/utils/exec.ts +++ b/src/utils/exec.ts @@ -5,7 +5,7 @@ import path from 'node:path'; import { spawn, spawnSync, type ChildProcess, type StdioOptions } from 'node:child_process'; import { Readable } from 'node:stream'; import { pipeline } from 'node:stream/promises'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { emitDiagnostic, getDiagnosticsMeta, updateDiagnosticsScope } from './diagnostics.ts'; import { parseBooleanLiteral } from './source-value.ts'; @@ -567,12 +567,7 @@ function createStdinError( } function createCommandCanceledError(executable: string, cmd: string, args: string[]): AppError { - return new AppError('COMMAND_FAILED', 'request canceled', { - cmd, - args, - executable, - reason: 'request_canceled', - }); + return createRequestCanceledError({ cmd, args, executable }); } function createTimeoutError( diff --git a/src/utils/retry.ts b/src/utils/retry.ts index 95ee8c754..cbc35a543 100644 --- a/src/utils/retry.ts +++ b/src/utils/retry.ts @@ -1,4 +1,4 @@ -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { emitDiagnostic } from './diagnostics.ts'; type RetryPolicy = { @@ -84,7 +84,7 @@ export async function retryWithPolicy( let lastError: unknown; for (let attempt = 1; attempt <= merged.maxAttempts; attempt += 1) { if (options.signal?.aborted) { - throw new AppError('COMMAND_FAILED', 'request canceled', { reason: 'request_canceled' }); + throw createRequestCanceledError(); } if (options.deadline?.isExpired() && attempt > 1) break; try { diff --git a/test/integration/provider-scenarios/cloud-webdriver-provider-regressions.test.ts b/test/integration/provider-scenarios/cloud-webdriver-provider-regressions.test.ts index 0b95e1229..81c22d5bf 100644 --- a/test/integration/provider-scenarios/cloud-webdriver-provider-regressions.test.ts +++ b/test/integration/provider-scenarios/cloud-webdriver-provider-regressions.test.ts @@ -46,7 +46,11 @@ test('AWS Device Farm endpoint selection skips live-control WebSocket URLs', asy }); }); -test('WebDriver session creation retries transient provider failures', async () => { +// #1774: `POST /session` is never retried, even on a retriable-looking 5xx — +// the outcome of a failed create is indeterminate on a cloud grid, and a second +// attempt is a second billed device session. The transient failure surfaces to +// the caller, who decides whether to allocate again. +test('WebDriver session creation is not retried on transient provider failures', async () => { await withProviderScenarioResource(ProviderRegressionServer.start, async (server) => { server.sessionFailuresRemaining = 1; const runtime = providerRuntimeFor( @@ -62,14 +66,13 @@ test('WebDriver session creation retries transient provider failures', async () ); const lease = providerRegressionLease(CLOUD_WEBDRIVER_PROVIDERS.browserStack); try { - const allocation = await runtime.leaseLifecycle.allocate?.( - lease, - browserStackRegressionContext(), + await assert.rejects( + () => runtime.leaseLifecycle.allocate!(lease, browserStackRegressionContext()), + /transient provider failure/, ); - assert.equal(allocation?.providerSessionId, 'wd-regression'); - assert.equal(server.calls.filter((call) => call.path === '/wd/hub/session').length, 2); + assert.equal(server.calls.filter((call) => call.path === '/wd/hub/session').length, 1); + assert.equal(await runtime.leaseLifecycle.release?.(lease), undefined); } finally { - await runtime.leaseLifecycle.release?.(lease); await runtime.shutdown(); } });