From 483baacf465646920ec3b4219114488c4330120e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 13:20:38 +0200 Subject: [PATCH 01/12] fix(webdriver): give cloud session creation its own budget and stop leaking billed sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cloud lease allocation ran under the generic 30s/1-retry request policy, so BrowserStack iOS real-device session creation (45-90s) aborted client-side at ~60s on most runs. Each timed-out POST /session still completed server-side and, being non-idempotent, was retried — leaving two billed provider sessions per failed open with no id to release them. - POST /session is its own phase: a 180s create budget (default), zero retries, and no request-bound abort, so the daemon always learns the session id. - lease_allocate carries a 300s allocation budget surfaced to providers as LeaseLifecycleContext.deadline, and a matching 330s client envelope that preserves the daemon on timeout (a reset would SIGKILL mid-create and orphan every billed session the daemon held). - The request's cancellation signal is ownership evidence: a session that completes after the requester left is released, not registered; a create that the transport gives up on surfaces typed evidence (provider + lease) so an operator can find and stop the maybe-orphaned session. Closes #1774 --- packages/contracts/src/device-provider.ts | 15 ++ .../src/runtime-session.test.ts | 168 ++++++++++++++++++ .../provider-webdriver/src/runtime-session.ts | 116 ++++++++++-- .../src/webdriver-client.test.ts | 95 +++++++++- .../src/webdriver-client.ts | 57 +++++- .../src/webdriver-transport.test.ts | 60 ++++++- .../src/webdriver-transport.ts | 100 +++++++++-- .../__tests__/timeout-policy.test.ts | 6 + src/core/command-descriptor/registry.ts | 3 +- src/core/command-descriptor/timeout-policy.ts | 35 ++++ .../__tests__/request-handler-catalog.test.ts | 54 ++++++ src/daemon/client/daemon-client-timeout.ts | 6 +- src/daemon/handlers/lease.ts | 10 +- src/daemon/request-handler-chain.ts | 1 + ...oud-webdriver-provider-regressions.test.ts | 17 +- 15 files changed, 694 insertions(+), 49 deletions(-) diff --git a/packages/contracts/src/device-provider.ts b/packages/contracts/src/device-provider.ts index 6558536fa4..ffa0f3f2d2 100644 --- a/packages/contracts/src/device-provider.ts +++ b/packages/contracts/src/device-provider.ts @@ -25,6 +25,21 @@ export type DeviceLease = { export type LeaseLifecycleContext = { flags?: Readonly>; cwd?: string; + /** + * Request-bound cancellation: aborted once the requester is gone (explicit + * cancel or client disconnect). For allocation it is an OWNERSHIP signal, not + * an interrupt — a provider whose remote allocation has already committed + * finishes it and releases the result rather than abandoning a billed + * resource nobody holds the id of. + */ + signal?: AbortSignal; + /** + * Epoch-ms deadline by which `allocate` must have settled. The daemon derives + * it from the same budget as the client's `lease_allocate` request envelope, + * so a provider that fits its remote phases within it is never abandoned by + * a client that stopped waiting first. + */ + deadline?: number; }; export type LeaseLifecycleProvider = { diff --git a/packages/provider-webdriver/src/runtime-session.test.ts b/packages/provider-webdriver/src/runtime-session.test.ts index 88e3a4422c..c61607e727 100644 --- a/packages/provider-webdriver/src/runtime-session.test.ts +++ b/packages/provider-webdriver/src/runtime-session.test.ts @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; import type { DeviceLease } from '@agent-device/contracts/device'; +import { deviceFieldsFromPublicPlatform, type DeviceInfo } from '@agent-device/kernel/device'; import { AppError } from '@agent-device/kernel/errors'; import { createCloudWebDriverRuntime } from './runtime.ts'; @@ -47,6 +48,173 @@ test('session allocation preserves its primary failure when provider cleanup als } }); +// #1774: a request already gone before the create is issued must create +// NOTHING. Prepared provider resources are cleaned up and no `POST /session` +// goes out, so there is no billed device session to leak. +test('allocation canceled before create issues no session and cleans up prepared work', async () => { + const previousFetch = globalThis.fetch; + let sessionRequests = 0; + let cleanupCalled = false; + globalThis.fetch = async (input) => { + if (String(input instanceof Request ? input.url : input).endsWith('/session')) { + sessionRequests += 1; + } + return new Response(JSON.stringify({ value: { sessionId: 'wd-1', capabilities: {} } }), { + status: 200, + 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 }, + prepareSession: async ({ base }) => ({ + ...base, + cleanup: async () => { + cleanupCalled = true; + return undefined; + }, + }), + }); + const controller = new AbortController(); + controller.abort(); + + try { + const allocate = runtime.leaseLifecycle.allocate; + assert.ok(allocate); + await assert.rejects( + () => allocate(makeLease(), { signal: controller.signal }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, 'request_canceled'); + return true; + }, + ); + assert.equal(sessionRequests, 0, 'no POST /session may be issued once canceled'); + assert.equal(cleanupCalled, true); + } finally { + await runtime.shutdown(); + globalThis.fetch = previousFetch; + } +}); + +// #1774: the classic leak. The requester vanishes WHILE the provider is +// allocating; the create still completes server-side. The manager must not +// register that session (nobody is waiting on it) — it deletes it, holding the +// id it just learned, so the billed session is released instead of orphaned. +test('a session that completes after cancellation is released, not registered', async () => { + const previousFetch = globalThis.fetch; + const controller = new AbortController(); + let deletedSessionId: string | undefined; + globalThis.fetch = async (input, init) => { + const url = String(input instanceof Request ? input.url : input); + const method = init?.method ?? 'GET'; + if (url.endsWith('/session') && method === 'POST') { + // The client disconnects mid-create; the provider finishes anyway. + controller.abort(); + return new Response(JSON.stringify({ value: { sessionId: 'wd-live', capabilities: {} } }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + } + if (url.endsWith('/session/wd-live') && method === 'DELETE') { + deletedSessionId = 'wd-live'; + return new Response(JSON.stringify({ value: null }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + } + throw new Error(`unexpected ${method} ${url}`); + }; + const runtime = createCloudWebDriverRuntime({ + clientVersion: 'test', + provider: 'webdriver-test', + endpoint: 'https://webdriver.test/wd/hub/', + platform: 'android', + deviceName: 'Test device', + requestPolicy: { retryAttempts: 0 }, + }); + const lease = makeLease(); + + try { + const allocate = runtime.leaseLifecycle.allocate; + assert.ok(allocate); + await assert.rejects( + () => allocate(lease, { signal: controller.signal }), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.details?.reason, 'request_canceled'); + assert.equal(error.details?.releasedWebDriverSessionId, 'wd-live'); + return true; + }, + ); + assert.equal(deletedSessionId, 'wd-live', 'the completed session must be deleted'); + // Nothing registered: the device is not owned and no session answers for it. + assert.equal(runtime.getInteractor(makeDevice(lease)), undefined); + } 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 () => { + const previousFetch = globalThis.fetch; + globalThis.fetch = async (input, init) => { + if (String(input instanceof Request ? input.url : input).endsWith('/session')) { + return await new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); + }); + } + throw new Error('unexpected request'); + }; + const runtime = createCloudWebDriverRuntime({ + clientVersion: 'test', + provider: 'browserstack', + endpoint: 'https://webdriver.test/wd/hub/', + platform: 'ios', + deviceName: 'iPhone 17', + requestPolicy: { retryAttempts: 0, sessionCreateTimeoutMs: 40 }, + }); + const lease = { ...makeLease(), leaseProvider: 'browserstack' }; + + try { + const allocate = runtime.leaseLifecycle.allocate; + assert.ok(allocate); + await assert.rejects( + () => 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(); + globalThis.fetch = previousFetch; + } +}); + +function makeDevice(lease: DeviceLease): DeviceInfo { + return { + ...deviceFieldsFromPublicPlatform('android'), + id: `webdriver-test:android:${lease.leaseId}`, + name: 'Test device', + kind: 'device', + target: 'mobile', + booted: true, + }; +} + 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 216d19ec4f..4cb468f941 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -10,7 +10,8 @@ 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 type { @@ -41,6 +42,8 @@ type CloudWebDriverCloseResult = Readonly<{ warnings: CloudWebDriverReleaseWarning[]; }>; type LeaseResult = Record | undefined; +/** The half of a provider session that exists once the WebDriver session does, registered or not. */ +type ProviderSessionHandle = Pick; /** * Owns WebDriver session lifecycle and stale-owner retention. Deployment decisions stay in the @@ -89,7 +92,7 @@ export class WebDriverSessionManager { headers: prepared.headers, requestPolicy: this.options.requestPolicy, }); - const session = await this.createSessionWithPreparedCleanup(client, prepared); + const session = await this.createOwnedSession({ client, prepared }, lease, req); const device = this.deviceForLease(lease, prepared); const providerSessionId = prepared.providerSessionId ?? session.sessionId; const capabilities = createCloudWebDriverCapabilities({ @@ -164,18 +167,65 @@ export class WebDriverSessionManager { this.ownedDeviceIds.clear(); } - private async createSessionWithPreparedCleanup( - client: WebDriverClient, - prepared: CloudWebDriverPreparedSession, - ): Promise>> { + /** + * Creates the WebDriver session and settles who owns it. `POST /session` is + * non-idempotent with an indeterminate outcome once abandoned (see + * `WebDriverClient.createSession`), so the request's cancellation is treated + * as ownership evidence rather than an interrupt: a requester that left + * before creation started gets nothing created; one that left while the + * provider was allocating gets the finished session released, because by + * then its id is in hand and nothing else will ever release it (#1774). + */ + private async createOwnedSession( + handle: ProviderSessionHandle, + lease: DeviceLease, + req: LeaseLifecycleContext | undefined, + ): Promise { + if (req?.signal?.aborted) { + const canceled = requestCanceledError(this.options.provider, lease, {}); + await cleanupAfterCreateSessionFailure(handle.prepared, canceled); + throw canceled; + } + const session = await this.createSessionOrCleanup(handle, lease, req); + if (req?.signal?.aborted) { + throw await this.releaseCanceledSession(handle, lease, session); + } + return session; + } + + private async createSessionOrCleanup( + handle: ProviderSessionHandle, + lease: DeviceLease, + req: LeaseLifecycleContext | undefined, + ): Promise { try { - return await client.createSession(prepared.webdriverCapabilities); + return await handle.client.createSession(handle.prepared.webdriverCapabilities, { + deadline: req?.deadline, + }); } catch (error) { - await cleanupAfterCreateSessionFailure(prepared, error); - throw error; + const failure = isWebDriverRequestTimeout(error) + ? sessionCreateTimeoutError(error, this.options.provider, lease, handle.prepared) + : error; + await cleanupAfterCreateSessionFailure(handle.prepared, failure); + throw failure; } } + private async releaseCanceledSession( + handle: ProviderSessionHandle, + lease: DeviceLease, + session: WebDriverSession, + ): Promise { + const close = await this.closeSession(handle); + return requestCanceledError(this.options.provider, lease, { + releasedWebDriverSessionId: session.sessionId, + ...(handle.prepared.providerSessionId + ? { releasedProviderSessionId: handle.prepared.providerSessionId } + : {}), + ...(close.warnings.length > 0 ? { warnings: close.warnings } : {}), + }); + } + private async prepareSession( lease: DeviceLease, req: LeaseLifecycleContext | undefined, @@ -221,9 +271,7 @@ export class WebDriverSessionManager { }; } - private async closeSession( - session: WebDriverProviderSession, - ): Promise { + private async closeSession(session: ProviderSessionHandle): Promise { const warnings: CloudWebDriverReleaseWarning[] = []; let cleanup: Record | undefined; try { @@ -296,3 +344,47 @@ async function cleanupAfterCreateSessionFailure( } } } + +/** + * The transport gave up on `POST /session`; the provider may still finish it. + * Nothing here can learn that session's id, so the error carries what the + * provider dashboard can be searched by instead — the lease the capabilities + * were labelled with — rather than guessing at REST cleanup of a session this + * process never owned. + */ +function sessionCreateTimeoutError( + timeout: AppError, + provider: string, + lease: DeviceLease, + prepared: CloudWebDriverPreparedSession, +): 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, + ); +} + +function requestCanceledError( + provider: string, + lease: DeviceLease, + released: Record, +): AppError { + return new AppError('COMMAND_FAILED', 'request canceled', { + reason: 'request_canceled', + provider, + leaseId: lease.leaseId, + ...released, + hint: 'The lease request was canceled (explicit cancel or client disconnect) while the provider session was being created; the session it produced, if any, was released instead of registered.', + }); +} diff --git a/packages/provider-webdriver/src/webdriver-client.test.ts b/packages/provider-webdriver/src/webdriver-client.test.ts index bc5c27fbe9..3f7d714f4d 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. diff --git a/packages/provider-webdriver/src/webdriver-client.ts b/packages/provider-webdriver/src/webdriver-client.ts index cc3f41f54d..e4a2dc19c6 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; @@ -58,18 +66,50 @@ export type W3CActionSequence = { actions: W3CPointerAction[]; }; +export type WebDriverCreateSessionOptions = { + /** + * Epoch-ms deadline of the operation this creation belongs to. It can only + * shorten the client's own session-creation budget, never extend it, so a + * daemon request that has spent most of its allocation budget on provider + * preparation does not start a device allocation it cannot wait out. + */ + deadline?: number; +}; + 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 the one non-idempotent request in the protocol, and its + * outcome after a client-side abort is indeterminate: a hub that has already + * started allocating a device finishes the session whether or not anyone is + * still listening. So it runs under its own budget with NO retries and NO + * request-bound cancellation — a retry after a timed-out attempt is a second + * billed session, and aborting the request would lose the id of the first + * (#1774). Callers that stop wanting the session while it is being created + * release it once they hold the id (see WebDriverSessionManager). + */ + async createSession( + capabilities: Record, + options?: WebDriverCreateSessionOptions, + ): 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; @@ -366,6 +406,15 @@ function requestBudget(deadline: number | undefined): WebDriverRequestOverrides return { retryAttempts: 0, timeoutMs: Math.max(0, deadline - Date.now()) }; } +/** + * A phase budget capped by the operation deadline it runs under, if any. Zero + * once the deadline has passed, for the same reason as `requestBudget`. + */ +function budgetWithin(budgetMs: number, deadline: number | undefined): number { + if (deadline === undefined) return budgetMs; + return Math.max(0, Math.min(budgetMs, deadline - Date.now())); +} + /** Nothing is focused right now — an expected state, not a driver defect. */ function isNoSuchElementError(error: unknown): boolean { if (!(error instanceof AppError)) return false; diff --git a/packages/provider-webdriver/src/webdriver-transport.test.ts b/packages/provider-webdriver/src/webdriver-transport.test.ts index e2cce84b4d..f45de1fff1 100644 --- a/packages/provider-webdriver/src/webdriver-transport.test.ts +++ b/packages/provider-webdriver/src/webdriver-transport.test.ts @@ -1,6 +1,11 @@ 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 { + WEBDRIVER_REQUEST_TIMEOUT_REASON, + WebDriverTransport, + isWebDriverRequestTimeout, +} from './webdriver-transport.ts'; const realFetch = globalThis.fetch; @@ -8,6 +13,59 @@ 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?.reason, WEBDRIVER_REQUEST_TIMEOUT_REASON); + 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 dd2d1e83af..c12e4bbf13 100644 --- a/packages/provider-webdriver/src/webdriver-transport.ts +++ b/packages/provider-webdriver/src/webdriver-transport.ts @@ -12,8 +12,27 @@ 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. */ +export const WEBDRIVER_REQUEST_TIMEOUT_REASON = 'webdriver_request_timeout'; + +/** + * A request the transport stopped waiting on. Its outcome is INDETERMINATE: + * the server may still complete it — which is why a non-idempotent caller must + * neither retry it nor assume nothing was created. + */ +export function isWebDriverRequestTimeout(error: unknown): error is AppError { + return error instanceof AppError && error.details?.reason === WEBDRIVER_REQUEST_TIMEOUT_REASON; +} + export type WebDriverRequestOverrides = { retryAttempts?: number; /** @@ -45,11 +64,15 @@ type ResolvedWebDriverRequestOverrides = { signal?: AbortSignal; }; +type ResolvedWebDriverRequestPolicy = Required< + Pick +>; + /** 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 +137,55 @@ 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 { 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 (status < 200 || status >= 300) { + throw webdriverError(status, payload); } return readWebDriverValue(payload); } + + private async fetchWebDriver( + method: string, + path: string, + body: unknown, + timeoutMs: number, + requestSignal?: AbortSignal, + ): Promise<{ status: number; 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: this.requestHeaders(body), + body: body === undefined ? undefined : JSON.stringify(body), + signal, + }); + return { 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; + } + } + + private requestHeaders(body: unknown): Record { + return { + Accept: 'application/json', + ...(body === undefined ? {} : { 'Content-Type': 'application/json' }), + ...this.headers, + }; + } } function shouldRetryWebDriverRequest( @@ -173,10 +226,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/src/core/command-descriptor/__tests__/timeout-policy.test.ts b/src/core/command-descriptor/__tests__/timeout-policy.test.ts index ff164aa37d..1625a487b9 100644 --- a/src/core/command-descriptor/__tests__/timeout-policy.test.ts +++ b/src/core/command-descriptor/__tests__/timeout-policy.test.ts @@ -70,6 +70,9 @@ 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. + // lease_allocate joined in #1774: allocation creates a BILLED provider + // session the daemon owns, so a client-side timeout must not SIGKILL the + // daemon mid-create and orphan it (and every other provider session held). const preserving = commandDescriptors .filter((descriptor) => descriptor.timeoutPolicy.onTimeout === 'preserve-daemon') .map((descriptor) => descriptor.name); @@ -81,6 +84,7 @@ test('daemon-preserving timeout commands are a bounded, reviewed set', () => { 'get', 'hover', 'is', + 'lease_allocate', 'longpress', 'press', 'scroll', @@ -133,6 +137,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 a1c0f2858a..629f49cc59 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_TIMEOUT_POLICY, PREPARE_REQUEST_TIMEOUT_MS, } from './timeout-policy.ts'; import { resolvePostActionObservationSupport } from './post-action-observation.ts'; @@ -397,7 +398,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, }, diff --git a/src/core/command-descriptor/timeout-policy.ts b/src/core/command-descriptor/timeout-policy.ts index ed9b969848..69719248d0 100644 --- a/src/core/command-descriptor/timeout-policy.ts +++ b/src/core/command-descriptor/timeout-policy.ts @@ -11,6 +11,25 @@ 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 the daemon lets a lease lifecycle provider allocate one lease. Cloud + * providers spend most of it waiting on remote device allocation (BrowserStack + * iOS real devices take 45–90s to create a session; AWS Device Farm remote + * access takes ~2 minutes to reach RUNNING before the session is even created). + * The provider receives it as `LeaseLifecycleContext.deadline` and bounds its + * remote phases within it; the client's `lease_allocate` envelope is derived + * from it below, so the two cannot drift apart (#1774). + */ +export const LEASE_ALLOCATION_BUDGET_MS = 300_000; + +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 @@ -25,3 +44,19 @@ export const DEFAULT_TIMEOUT_POLICY: CommandTimeoutPolicy = { envelopeMs: DAEMON_REQUEST_TIMEOUT_MS, onTimeout: 'reset-daemon', }; + +/** + * Lease allocation against a cloud provider is remote work the daemon owns on + * the caller's behalf: the provider session it creates is billed until the + * daemon releases it. A client that stops waiting must therefore leave the + * daemon alive — the allocation either finishes and is released for the gone + * requester (see `LeaseLifecycleContext.signal`) or fails under the daemon's + * own budget with typed evidence. Resetting the daemon here would SIGKILL a + * process mid-`POST /session` and orphan the billed session it was about to + * own, along with every other provider session that daemon held. + */ +export const LEASE_ALLOCATE_TIMEOUT_POLICY: CommandTimeoutPolicy = { + budget: { source: 'none' }, + envelopeMs: LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, + onTimeout: 'preserve-daemon', +}; diff --git a/src/daemon/__tests__/request-handler-catalog.test.ts b/src/daemon/__tests__/request-handler-catalog.test.ts index 74ea11d5b2..688e944a4b 100644 --- a/src/daemon/__tests__/request-handler-catalog.test.ts +++ b/src/daemon/__tests__/request-handler-catalog.test.ts @@ -93,6 +93,7 @@ test('lease handler executes commands owned by the lease route', async () => { sessionName: 'catalog-test', sessionStore, leaseRegistry, + requestSignal: new AbortController().signal, }); assert.notEqual(response, null, `${command} should be handled by lease handler`); @@ -120,6 +121,7 @@ test('lease handler preserves device-aware lease fields', async () => { sessionName: 'catalog-test', sessionStore, leaseRegistry, + requestSignal: new AbortController().signal, }); assert.equal(allocateResponse?.ok, true); @@ -147,6 +149,7 @@ test('lease handler preserves device-aware lease fields', async () => { sessionName: 'catalog-test', sessionStore, leaseRegistry, + requestSignal: new AbortController().signal, }); assert.equal(heartbeatResponse?.ok, true); @@ -167,6 +170,7 @@ test('lease artifacts lists daemon inventory for proxy lease scopes', async () = sessionName: 'catalog-test', sessionStore, leaseRegistry, + requestSignal: new AbortController().signal, }); assertProxyLeaseArtifactInventory(response, tracked.artifactId); @@ -197,6 +201,7 @@ test('lease release calls provider hook using the released lease without heartbe sessionName: 'catalog-test', sessionStore, leaseRegistry, + requestSignal: new AbortController().signal, }); assert.equal(allocateResponse?.ok, true); const lease = readLeaseResponse(allocateResponse); @@ -225,6 +230,7 @@ test('lease release calls provider hook using the released lease without heartbe return { provider: releasedLease.leaseProvider }; }, }, + requestSignal: new AbortController().signal, }); assert.equal(releaseResponse?.ok, true); @@ -235,6 +241,54 @@ 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 passes the request signal and a deadline to the provider', async () => { + const leaseRegistry = new LeaseRegistry(); + const sessionStore = makeSessionStore('agent-device-lease-ownership-'); + const requestSignal = new AbortController().signal; + const before = Date.now(); + let observed: { signal?: AbortSignal; deadline?: number } | undefined; + + const response = await handleLeaseCommands({ + req: { + command: INTERNAL_COMMANDS.leaseAllocate, + token: 'test-token', + session: 'catalog-test', + meta: { + 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' }; + }, + }, + requestSignal, + }); + const after = Date.now(); + + assert.equal(response?.ok, true); + assert.equal(observed?.signal, requestSignal); + assert.ok( + typeof observed?.deadline === 'number' && + observed.deadline > before && + observed.deadline > after, + `allocation deadline must be a future instant, got ${String(observed?.deadline)}`, + ); +}); + function catalogCommandsForRoute(route: Exclude): string[] { return [...Object.values(PUBLIC_COMMANDS), ...Object.values(INTERNAL_COMMANDS)].filter( (command) => getDaemonCommandRoute(command) === route, diff --git a/src/daemon/client/daemon-client-timeout.ts b/src/daemon/client/daemon-client-timeout.ts index 7fd723c005..0f5a7b6ac6 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 9a7920fcb8..bfcbce653c 100644 --- a/src/daemon/handlers/lease.ts +++ b/src/daemon/handlers/lease.ts @@ -18,6 +18,7 @@ import { leaseScopeToReleaseRequest, } from '../../core/lease-scope.ts'; import { AppError } from '@agent-device/kernel/errors'; +import { LEASE_ALLOCATION_BUDGET_MS } from '../../core/command-descriptor/timeout-policy.ts'; import { listDownloadableArtifacts } from '../artifact-tracking.ts'; type LeaseHandlerArgs = { @@ -29,6 +30,8 @@ type LeaseHandlerArgs = { providerRuntimeRequiredIds?: readonly string[]; leaseLifecycleProvider?: LeaseLifecycleProvider; cloudArtifactProvider?: CloudArtifactProvider; + /** Request-bound cancellation, handed to the provider as ownership evidence for allocation. */ + requestSignal: AbortSignal; }; export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise { @@ -41,6 +44,7 @@ export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise | undefined; try { - providerData = await leaseLifecycleProvider?.allocate?.(lease, leaseLifecycleContext(req)); + providerData = await leaseLifecycleProvider?.allocate?.(lease, { + ...leaseLifecycleContext(req), + signal: requestSignal, + deadline: Date.now() + LEASE_ALLOCATION_BUDGET_MS, + }); } catch (error) { leaseRegistry.releaseLease( leaseScopeToReleaseRequest({ diff --git a/src/daemon/request-handler-chain.ts b/src/daemon/request-handler-chain.ts index 0dfbc0492e..20c1f5c51f 100644 --- a/src/daemon/request-handler-chain.ts +++ b/src/daemon/request-handler-chain.ts @@ -120,6 +120,7 @@ async function runLeaseHandler( providerRuntimeRequiredIds: params.providerRuntimeRequiredIds, leaseLifecycleProvider: params.leaseLifecycleProvider, cloudArtifactProvider: params.cloudArtifactProvider, + requestSignal: params.requestScope.signal, }), ); } 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 0b95e1229e..81c22d5bf8 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(); } }); From 2e14a7d854e00bb6f5e51ff606abbcde96918f24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 13:52:47 +0200 Subject: [PATCH 02/12] refactor: one canceled-request error, and tighten the #1774 shapes Review pass over the session-create fix: - The canceled-request error had nine hand-rolled copies (src/request/cancel, maestro shared, exec, retry, install-source x2, and the new provider one). It now has one definition in @agent-device/kernel/errors: createRequestCanceledError(details?, cause?) + isRequestCanceledError + REQUEST_CANCELED_REASON. Callers add evidence or a sharper hint; the reason itself is not overridable, so nothing can build one the predicate misses. - lease_allocate's timeout bundle moves beside INSTALL_TIMEOUT_POLICY in the registry (same {...DEFAULT, envelopeMs, onTimeout} shape); the request timeout constant stays exported from timeout-policy like its siblings. - Transport: fetch helper returns Response's own ok/status; the timeout reason const is private behind isWebDriverRequestTimeout. - Client: one-use options type inlined; the two deadline helpers share one floor. - Session-manager tests: shared makeRuntime/jsonResponse/afterEach restore. Net -29 lines with the feature in. --- packages/kernel/src/errors.ts | 34 +++++ packages/maestro/src/internal/engine-flow.ts | 3 +- .../maestro/src/internal/program-loader.ts | 2 +- packages/maestro/src/internal/shared.ts | 9 +- .../src/runtime-session.test.ts | 126 +++++++----------- .../provider-webdriver/src/runtime-session.ts | 25 ++-- .../src/webdriver-client.ts | 31 ++--- .../src/webdriver-transport.test.ts | 7 +- .../src/webdriver-transport.ts | 12 +- src/core/command-descriptor/registry.ts | 16 ++- src/core/command-descriptor/timeout-policy.ts | 18 +-- src/core/device-inventory-context.ts | 3 +- .../daemon-runtime-port-observation.ts | 3 +- .../maestro/maestro-screenshot-comparison.ts | 2 +- src/daemon/handlers/session-open-execution.ts | 3 +- src/daemon/server/transport.ts | 3 +- .../core/__tests__/runner-client.test.ts | 7 +- .../runner-request-cancellation.test.ts | 3 +- .../apple/core/runner/runner-artifact.ts | 3 +- .../apple/core/runner/runner-contract.ts | 8 +- .../apple/core/runner/runner-lifecycle.ts | 3 +- .../apple/core/runner/runner-session.ts | 3 +- .../core/runner/runner-startup-transport.ts | 7 +- .../apple/core/runner/runner-transport.ts | 3 +- .../core/runner/runner-usbmux-protocol.ts | 3 +- .../apple/core/runner/runner-usbmux.ts | 3 +- src/platforms/install-source-network.ts | 10 +- src/platforms/install-source.ts | 11 +- src/request/cancel.test.ts | 23 +++- src/request/cancel.ts | 20 +-- src/utils/exec.ts | 9 +- src/utils/retry.ts | 4 +- 32 files changed, 194 insertions(+), 223 deletions(-) diff --git a/packages/kernel/src/errors.ts b/packages/kernel/src/errors.ts index e2ad36c8dd..d6c95e5ad0 100644 --- a/packages/kernel/src/errors.ts +++ b/packages/kernel/src/errors.ts @@ -128,6 +128,40 @@ 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. + */ +export 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; +} + 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 5d31a0bebf..6de04600a8 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 6f8f7505ad..5163b00b88 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 fb507f52dd..75e5047cf8 100644 --- a/packages/maestro/src/internal/shared.ts +++ b/packages/maestro/src/internal/shared.ts @@ -1,4 +1,4 @@ -import { AppError } from '@agent-device/kernel/errors'; +import {} from '@agent-device/kernel/errors'; import type { Point, Rect, SnapshotNode } from '@agent-device/kernel/snapshot'; export function stripUndefined>(value: T): T { @@ -45,10 +45,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/runtime-session.test.ts b/packages/provider-webdriver/src/runtime-session.test.ts index c61607e727..aee56c6a00 100644 --- a/packages/provider-webdriver/src/runtime-session.test.ts +++ b/packages/provider-webdriver/src/runtime-session.test.ts @@ -1,25 +1,20 @@ import assert from 'node:assert/strict'; -import { test } from 'vitest'; +import { afterEach, test } from 'vitest'; import type { DeviceLease } from '@agent-device/contracts/device'; import { deviceFieldsFromPublicPlatform, type DeviceInfo } from '@agent-device/kernel/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 () => { @@ -30,10 +25,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/); @@ -44,7 +37,6 @@ test('session allocation preserves its primary failure when provider cleanup als assert.equal(cleanupCalled, true); } finally { await runtime.shutdown(); - globalThis.fetch = previousFetch; } }); @@ -52,25 +44,13 @@ test('session allocation preserves its primary failure when provider cleanup als // NOTHING. Prepared provider resources are cleaned up and no `POST /session` // goes out, so there is no billed device session to leak. test('allocation canceled before create issues no session and cleans up prepared work', async () => { - const previousFetch = globalThis.fetch; let sessionRequests = 0; let cleanupCalled = false; globalThis.fetch = async (input) => { - if (String(input instanceof Request ? input.url : input).endsWith('/session')) { - sessionRequests += 1; - } - return new Response(JSON.stringify({ value: { sessionId: 'wd-1', capabilities: {} } }), { - status: 200, - headers: { 'Content-Type': 'application/json' }, - }); + if (requestUrl(input).endsWith('/session')) sessionRequests += 1; + return jsonResponse({ value: { sessionId: 'wd-1', capabilities: {} } }); }; - const runtime = createCloudWebDriverRuntime({ - clientVersion: 'test', - provider: 'webdriver-test', - endpoint: 'https://webdriver.test/wd/hub/', - platform: 'android', - deviceName: 'Test device', - requestPolicy: { retryAttempts: 0 }, + const runtime = makeRuntime({ prepareSession: async ({ base }) => ({ ...base, cleanup: async () => { @@ -83,10 +63,8 @@ test('allocation canceled before create issues no session and cleans up prepared controller.abort(); try { - const allocate = runtime.leaseLifecycle.allocate; - assert.ok(allocate); await assert.rejects( - () => allocate(makeLease(), { signal: controller.signal }), + () => runtime.leaseLifecycle.allocate!(makeLease(), { signal: controller.signal }), (error: unknown) => { assert.ok(error instanceof AppError); assert.equal(error.details?.reason, 'request_canceled'); @@ -97,7 +75,6 @@ test('allocation canceled before create issues no session and cleans up prepared assert.equal(cleanupCalled, true); } finally { await runtime.shutdown(); - globalThis.fetch = previousFetch; } }); @@ -106,44 +83,28 @@ test('allocation canceled before create issues no session and cleans up prepared // register that session (nobody is waiting on it) — it deletes it, holding the // id it just learned, so the billed session is released instead of orphaned. test('a session that completes after cancellation is released, not registered', async () => { - const previousFetch = globalThis.fetch; const controller = new AbortController(); let deletedSessionId: string | undefined; globalThis.fetch = async (input, init) => { - const url = String(input instanceof Request ? input.url : input); + const url = requestUrl(input); const method = init?.method ?? 'GET'; if (url.endsWith('/session') && method === 'POST') { // The client disconnects mid-create; the provider finishes anyway. controller.abort(); - return new Response(JSON.stringify({ value: { sessionId: 'wd-live', capabilities: {} } }), { - status: 200, - headers: { 'Content-Type': 'application/json' }, - }); + return jsonResponse({ value: { sessionId: 'wd-live', capabilities: {} } }); } if (url.endsWith('/session/wd-live') && method === 'DELETE') { deletedSessionId = 'wd-live'; - return new Response(JSON.stringify({ value: null }), { - status: 200, - headers: { 'Content-Type': 'application/json' }, - }); + return jsonResponse({ value: null }); } throw new Error(`unexpected ${method} ${url}`); }; - const runtime = createCloudWebDriverRuntime({ - clientVersion: 'test', - provider: 'webdriver-test', - endpoint: 'https://webdriver.test/wd/hub/', - platform: 'android', - deviceName: 'Test device', - requestPolicy: { retryAttempts: 0 }, - }); + const runtime = makeRuntime(); const lease = makeLease(); try { - const allocate = runtime.leaseLifecycle.allocate; - assert.ok(allocate); await assert.rejects( - () => allocate(lease, { signal: controller.signal }), + () => runtime.leaseLifecycle.allocate!(lease, { signal: controller.signal }), (error: unknown) => { assert.ok(error instanceof AppError); assert.equal(error.details?.reason, 'request_canceled'); @@ -156,7 +117,6 @@ test('a session that completes after cancellation is released, not registered', assert.equal(runtime.getInteractor(makeDevice(lease)), undefined); } finally { await runtime.shutdown(); - globalThis.fetch = previousFetch; } }); @@ -165,30 +125,20 @@ test('a session that completes after cancellation is released, not registered', // 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 () => { - const previousFetch = globalThis.fetch; - globalThis.fetch = async (input, init) => { - if (String(input instanceof Request ? input.url : input).endsWith('/session')) { - return await new Promise((_resolve, reject) => { - init?.signal?.addEventListener('abort', () => reject(init.signal?.reason as Error)); - }); - } - throw new Error('unexpected request'); - }; - const runtime = createCloudWebDriverRuntime({ - clientVersion: 'test', + 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', - endpoint: 'https://webdriver.test/wd/hub/', platform: 'ios', - deviceName: 'iPhone 17', requestPolicy: { retryAttempts: 0, sessionCreateTimeoutMs: 40 }, }); const lease = { ...makeLease(), leaseProvider: 'browserstack' }; try { - const allocate = runtime.leaseLifecycle.allocate; - assert.ok(allocate); await assert.rejects( - () => allocate(lease), + () => runtime.leaseLifecycle.allocate!(lease), (error: unknown) => { assert.ok(error instanceof AppError); assert.equal(error.details?.reason, 'provider_session_create_timeout'); @@ -200,10 +150,32 @@ test('a create-timeout surfaces provider evidence for the maybe-leaked session', ); } finally { await runtime.shutdown(); - globalThis.fetch = previousFetch; } }); +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 requestUrl(input: RequestInfo | URL): string { + return String(input instanceof Request ? input.url : input); +} + function makeDevice(lease: DeviceLease): DeviceInfo { return { ...deviceFieldsFromPublicPlatform('android'), diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index 4cb468f941..e4651fef61 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -4,7 +4,11 @@ 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, + createRequestCanceledError, + type AppErrorDetails, +} from '@agent-device/kernel/errors'; import { unavailableCloudArtifactsResult } from './artifact-results.ts'; import { createCloudWebDriverCapabilities, @@ -182,7 +186,9 @@ export class WebDriverSessionManager { req: LeaseLifecycleContext | undefined, ): Promise { if (req?.signal?.aborted) { - const canceled = requestCanceledError(this.options.provider, lease, {}); + const canceled = createRequestCanceledError( + canceledAllocationEvidence(this.options.provider, lease), + ); await cleanupAfterCreateSessionFailure(handle.prepared, canceled); throw canceled; } @@ -217,7 +223,8 @@ export class WebDriverSessionManager { session: WebDriverSession, ): Promise { const close = await this.closeSession(handle); - return requestCanceledError(this.options.provider, lease, { + return createRequestCanceledError({ + ...canceledAllocationEvidence(this.options.provider, lease), releasedWebDriverSessionId: session.sessionId, ...(handle.prepared.providerSessionId ? { releasedProviderSessionId: handle.prepared.providerSessionId } @@ -375,16 +382,10 @@ function sessionCreateTimeoutError( ); } -function requestCanceledError( - provider: string, - lease: DeviceLease, - released: Record, -): AppError { - return new AppError('COMMAND_FAILED', 'request canceled', { - reason: 'request_canceled', +function canceledAllocationEvidence(provider: string, lease: DeviceLease): AppErrorDetails { + return { provider, leaseId: lease.leaseId, - ...released, hint: 'The lease request was canceled (explicit cancel or client disconnect) while the provider session was being created; the session it produced, if any, was released instead of registered.', - }); + }; } diff --git a/packages/provider-webdriver/src/webdriver-client.ts b/packages/provider-webdriver/src/webdriver-client.ts index e4a2dc19c6..992ad32acd 100644 --- a/packages/provider-webdriver/src/webdriver-client.ts +++ b/packages/provider-webdriver/src/webdriver-client.ts @@ -66,16 +66,6 @@ export type W3CActionSequence = { actions: W3CPointerAction[]; }; -export type WebDriverCreateSessionOptions = { - /** - * Epoch-ms deadline of the operation this creation belongs to. It can only - * shorten the client's own session-creation budget, never extend it, so a - * daemon request that has spent most of its allocation budget on provider - * preparation does not start a device allocation it cannot wait out. - */ - deadline?: number; -}; - export class WebDriverClient { private readonly transport: WebDriverTransport; private readonly sessionCreateTimeoutMs: number; @@ -96,10 +86,15 @@ export class WebDriverClient { * billed session, and aborting the request would lose the id of the first * (#1774). Callers that stop wanting the session while it is being created * release it once they hold the id (see WebDriverSessionManager). + * + * `deadline` (epoch ms) is the operation this creation belongs to; it can + * only SHORTEN the client's own budget, never extend it, so a daemon request + * that spent most of its allocation window on provider preparation does not + * start a device allocation it cannot wait out. */ async createSession( capabilities: Record, - options?: WebDriverCreateSessionOptions, + options?: { deadline?: number }, ): Promise { const value = await this.requestValue( 'POST', @@ -403,16 +398,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 budget capped by the operation deadline it runs under, if any. Zero - * once the deadline has passed, for the same reason as `requestBudget`. - */ +/** A phase's own budget, capped by the operation deadline it runs under, if any. */ function budgetWithin(budgetMs: number, deadline: number | undefined): number { - if (deadline === undefined) return budgetMs; - return Math.max(0, Math.min(budgetMs, deadline - Date.now())); + 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 f45de1fff1..7d85b57c37 100644 --- a/packages/provider-webdriver/src/webdriver-transport.test.ts +++ b/packages/provider-webdriver/src/webdriver-transport.test.ts @@ -1,11 +1,7 @@ import assert from 'node:assert/strict'; import { afterEach, test } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; -import { - WEBDRIVER_REQUEST_TIMEOUT_REASON, - WebDriverTransport, - isWebDriverRequestTimeout, -} from './webdriver-transport.ts'; +import { WebDriverTransport, isWebDriverRequestTimeout } from './webdriver-transport.ts'; const realFetch = globalThis.fetch; @@ -32,7 +28,6 @@ test('a transport-deadline abort surfaces as a typed timeout, not a DOMException await assert.rejects(transport.requestValue('POST', '/session', {}), (error: unknown) => { assert.ok(error instanceof AppError); assert.ok(isWebDriverRequestTimeout(error)); - assert.equal(error.details?.reason, WEBDRIVER_REQUEST_TIMEOUT_REASON); assert.equal(error.details?.timeoutMs, 30); assert.equal(error.details?.method, 'POST'); assert.equal(error.details?.path, '/session'); diff --git a/packages/provider-webdriver/src/webdriver-transport.ts b/packages/provider-webdriver/src/webdriver-transport.ts index c12e4bbf13..a87597da38 100644 --- a/packages/provider-webdriver/src/webdriver-transport.ts +++ b/packages/provider-webdriver/src/webdriver-transport.ts @@ -22,7 +22,7 @@ export type WebDriverRequestPolicy = { }; /** Machine-readable `details.reason` of a request the transport gave up waiting on. */ -export const WEBDRIVER_REQUEST_TIMEOUT_REASON = 'webdriver_request_timeout'; +const WEBDRIVER_REQUEST_TIMEOUT_REASON = 'webdriver_request_timeout'; /** * A request the transport stopped waiting on. Its outcome is INDETERMINATE: @@ -137,7 +137,7 @@ export class WebDriverTransport { timeoutMs: number, requestSignal?: AbortSignal, ): Promise { - const { status, text } = await this.fetchWebDriver( + const { ok, status, text } = await this.fetchWebDriver( method, path, body, @@ -145,9 +145,7 @@ export class WebDriverTransport { requestSignal, ); const payload = text ? parseJsonResponse(text) : {}; - if (status < 200 || status >= 300) { - throw webdriverError(status, payload); - } + if (!ok) throw webdriverError(status, payload); return readWebDriverValue(payload); } @@ -157,7 +155,7 @@ export class WebDriverTransport { body: unknown, timeoutMs: number, requestSignal?: AbortSignal, - ): Promise<{ status: number; text: string }> { + ): Promise & { text: string }> { const timeoutSignal = AbortSignal.timeout(timeoutMs); const signal = requestSignal ? AbortSignal.any([requestSignal, timeoutSignal]) : timeoutSignal; try { @@ -167,7 +165,7 @@ export class WebDriverTransport { body: body === undefined ? undefined : JSON.stringify(body), signal, }); - return { status: response.status, text: await response.text() }; + 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` diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index 629f49cc59..e9d17311ce 100644 --- a/src/core/command-descriptor/registry.ts +++ b/src/core/command-descriptor/registry.ts @@ -8,7 +8,7 @@ import { resolveWaitBudgetMs } from '../wait-positionals.ts'; import { DEFAULT_TIMEOUT_POLICY, INSTALL_REQUEST_TIMEOUT_MS, - LEASE_ALLOCATE_TIMEOUT_POLICY, + LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, PREPARE_REQUEST_TIMEOUT_MS, } from './timeout-policy.ts'; import { resolvePostActionObservationSupport } from './post-action-observation.ts'; @@ -319,6 +319,20 @@ const INSTALL_TIMEOUT_POLICY: CommandTimeoutPolicy = { envelopeMs: INSTALL_REQUEST_TIMEOUT_MS, }; +// Lease allocation against a cloud provider is remote work the daemon owns on +// the caller's behalf: the provider session it creates is billed until the +// daemon releases it. A client that stops waiting must leave the daemon alive — +// the allocation either finishes and is released for the gone requester (see +// `LeaseLifecycleContext.signal`) or fails under the daemon's own budget with +// typed evidence. Resetting the daemon here would SIGKILL it mid-`POST /session` +// and orphan the billed session it was about to own, along with every other +// provider session that daemon held (#1774). +const LEASE_ALLOCATE_TIMEOUT_POLICY: CommandTimeoutPolicy = { + ...DEFAULT_TIMEOUT_POLICY, + envelopeMs: LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, + onTimeout: 'preserve-daemon', +}; + const DEFAULT_SETTLE_TIMEOUT_MS = 10_000; // Settle-capable interaction commands also resolve their target through the diff --git a/src/core/command-descriptor/timeout-policy.ts b/src/core/command-descriptor/timeout-policy.ts index 69719248d0..a246401f11 100644 --- a/src/core/command-descriptor/timeout-policy.ts +++ b/src/core/command-descriptor/timeout-policy.ts @@ -27,7 +27,7 @@ export const REQUEST_TIMEOUT_BUDGET_MARGIN_MS = 30_000; */ export const LEASE_ALLOCATION_BUDGET_MS = 300_000; -const LEASE_ALLOCATE_REQUEST_TIMEOUT_MS = +export const LEASE_ALLOCATE_REQUEST_TIMEOUT_MS = LEASE_ALLOCATION_BUDGET_MS + REQUEST_TIMEOUT_BUDGET_MARGIN_MS; /** @@ -44,19 +44,3 @@ export const DEFAULT_TIMEOUT_POLICY: CommandTimeoutPolicy = { envelopeMs: DAEMON_REQUEST_TIMEOUT_MS, onTimeout: 'reset-daemon', }; - -/** - * Lease allocation against a cloud provider is remote work the daemon owns on - * the caller's behalf: the provider session it creates is billed until the - * daemon releases it. A client that stops waiting must therefore leave the - * daemon alive — the allocation either finishes and is released for the gone - * requester (see `LeaseLifecycleContext.signal`) or fails under the daemon's - * own budget with typed evidence. Resetting the daemon here would SIGKILL a - * process mid-`POST /session` and orphan the billed session it was about to - * own, along with every other provider session that daemon held. - */ -export const LEASE_ALLOCATE_TIMEOUT_POLICY: CommandTimeoutPolicy = { - budget: { source: 'none' }, - envelopeMs: LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, - onTimeout: 'preserve-daemon', -}; diff --git a/src/core/device-inventory-context.ts b/src/core/device-inventory-context.ts index eedff4f603..41d62115bc 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/adapters/maestro/daemon-runtime-port-observation.ts b/src/daemon/adapters/maestro/daemon-runtime-port-observation.ts index 8746b2c63f..96a886f91c 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 fe10079780..adb1535e14 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/handlers/session-open-execution.ts b/src/daemon/handlers/session-open-execution.ts index 574112c9bc..f5fc227218 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 9b93451f1c..d5cbe7595a 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 c22152d1cf..cab25794c7 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 16ee6dfe5e..e0c10caea5 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 17ef0bf3ad..7e8131e3ed 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 d0d2c4b1c9..2b5c61849d 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 897caa716b..6f705aa84e 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 632dcdced0..583b600315 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 93a05e9ec7..7df0522d67 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 3db2d8e39d..ed089831f3 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 c8d501fe76..b7ca5fa1e5 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 06839073e7..dbaf575abf 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 e02be00d0a..d135e95c19 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 9555ef78fc..a01c9de567 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 f9950f9652..89e13ecc32 100644 --- a/src/request/cancel.test.ts +++ b/src/request/cancel.test.ts @@ -1,11 +1,11 @@ import { test } from 'vitest'; import assert from 'node:assert/strict'; -import { AppError } from '@agent-device/kernel/errors'; import { + AppError, createRequestCanceledError, isRequestCanceledError, - resolveRequestTrackingId, -} from './cancel.ts'; +} from '@agent-device/kernel/errors'; +import { resolveRequestTrackingId } from './cancel.ts'; test('resolveRequestTrackingId generates unique ids for fallback seeds', () => { const first = resolveRequestTrackingId(undefined, 42); @@ -23,6 +23,23 @@ 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); diff --git a/src/request/cancel.ts b/src/request/cancel.ts index a24f8c69fa..246870a6d6 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; @@ -114,22 +110,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 cdb05148e8..3bf80eb7d7 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 95ee8c7540..cbc35a543e 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 { From c04fd4888f6cc703f8bce0325ff2d2e8d2244834 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 13:54:04 +0200 Subject: [PATCH 03/12] chore: keep the canceled-request reason private to the kernel --- packages/kernel/src/errors.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kernel/src/errors.ts b/packages/kernel/src/errors.ts index d6c95e5ad0..3cae51fce6 100644 --- a/packages/kernel/src/errors.ts +++ b/packages/kernel/src/errors.ts @@ -134,7 +134,7 @@ export function throwDaemonError(error: DaemonError): never { * cancellation through untouched (retry loops, provider adapters, runner * transports) dispatches on the same typed reason. */ -export const REQUEST_CANCELED_REASON = 'request_canceled'; +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.'; From 81a32c914867674edfadb994440f7e69e4119b1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 14:24:34 +0200 Subject: [PATCH 04/12] fix: typed cancellation everywhere + own the AWS remote-access ARN through startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-order follow-ups the #1774 refactor made cheap: - markRequestCanceled aborts the request signal WITH the kernel's typed canceled error as its reason. Every signal.throwIfAborted(), aborted fetch, and 'throw signal.reason' in the daemon (20+ sites) now surfaces a canceled request as such instead of a bare DOMException that normalized to UNKNOWN — and no site has to know the factory exists. - AWS Device Farm prepareSession owns the remote-access ARN from the moment create-remote-access-session answers: a startup timeout, the allocation deadline, or a canceled request now stops it before the failure surfaces (previously a timed-out startup left a RUNNING billed session behind — the same leak class as the WebDriver session, one phase earlier). The startup wait is capped by LeaseLifecycleContext.deadline and wakes on cancellation. - BrowserStack's pre-session local app upload honors the request signal (an upload is not billed, so plain abort is right there). - lease_heartbeat/lease_release share lease_allocate's preserve-daemon policy: the rationale — the daemon owns billed sessions; a reset orphans them all — applies verbatim. Each AWS ownership test proven red without the guard (3/3). --- .../src/aws-device-farm.test.ts | 142 ++++++++++++++++++ .../provider-webdriver/src/aws-device-farm.ts | 85 ++++++++--- .../src/provider-definitions.ts | 21 ++- .../__tests__/timeout-policy.test.ts | 9 +- src/core/command-descriptor/registry.ts | 28 ++-- src/request/cancel.test.ts | 27 +++- src/request/cancel.ts | 6 +- 7 files changed, 276 insertions(+), 42 deletions(-) create mode 100644 packages/provider-webdriver/src/aws-device-farm.test.ts 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 0000000000..52b1135c22 --- /dev/null +++ b/packages/provider-webdriver/src/aws-device-farm.test.ts @@ -0,0 +1,142 @@ +import assert from 'node:assert/strict'; +import { test } 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'; + +// 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]); +}); + +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[] } { + const stopped: string[] = []; + return { + stopped, + 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 15fc1e5e12..238be7983f 100644 --- a/packages/provider-webdriver/src/aws-device-farm.ts +++ b/packages/provider-webdriver/src/aws-device-farm.ts @@ -15,7 +15,11 @@ 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'; @@ -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,27 @@ 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, - }); + // From here the ARN is OURS: a billed remote-access session that nothing + // else will ever stop. Whatever ends the startup wait short of RUNNING — + // startup timeout, allocation deadline, or the requester leaving — must + // stop it before the failure surfaces, or it keeps billing until AWS reaps + // it (the same ownership rule as the WebDriver session in + // WebDriverSessionManager, one phase earlier). + 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 stopRemoteAccessSessionAfterFailure(options.client, remoteAccess.arn, error); + throw error; } const deviceName = running.device?.name ?? options.deviceName; const configured = @@ -276,31 +294,60 @@ 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 startup wait fits inside the lease-allocation deadline, so a client + // that stops waiting at that deadline never abandons a still-polling daemon. + const deadline = Math.min(startedAt + timeoutMs, req?.deadline ?? Infinity); + 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, }); } +async function stopRemoteAccessSessionAfterFailure( + client: AwsDeviceFarmClient, + arn: string, + primaryError: unknown, +): Promise { + try { + await client.stopRemoteAccessSession(arn); + } catch (cleanupError) { + if (primaryError instanceof AppError) { + primaryError.details = { + ...primaryError.details, + cleanupError: cleanupError instanceof Error ? cleanupError.message : String(cleanupError), + }; + } + } +} + async function runAwsJson( runHostCommand: RunHostCommand, command: string, diff --git a/packages/provider-webdriver/src/provider-definitions.ts b/packages/provider-webdriver/src/provider-definitions.ts index 6ff6110fd5..81289f42bb 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/src/core/command-descriptor/__tests__/timeout-policy.test.ts b/src/core/command-descriptor/__tests__/timeout-policy.test.ts index 1625a487b9..77e0b5d365 100644 --- a/src/core/command-descriptor/__tests__/timeout-policy.test.ts +++ b/src/core/command-descriptor/__tests__/timeout-policy.test.ts @@ -70,9 +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. - // lease_allocate joined in #1774: allocation creates a BILLED provider - // session the daemon owns, so a client-side timeout must not SIGKILL the - // daemon mid-create and orphan it (and every other provider session held). + // 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); @@ -85,6 +86,8 @@ test('daemon-preserving timeout commands are a bounded, reviewed set', () => { 'hover', 'is', 'lease_allocate', + 'lease_heartbeat', + 'lease_release', 'longpress', 'press', 'scroll', diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index e9d17311ce..481050cd23 100644 --- a/src/core/command-descriptor/registry.ts +++ b/src/core/command-descriptor/registry.ts @@ -319,19 +319,23 @@ const INSTALL_TIMEOUT_POLICY: CommandTimeoutPolicy = { envelopeMs: INSTALL_REQUEST_TIMEOUT_MS, }; -// Lease allocation against a cloud provider is remote work the daemon owns on -// the caller's behalf: the provider session it creates is billed until the -// daemon releases it. A client that stops waiting must leave the daemon alive — -// the allocation either finishes and is released for the gone requester (see -// `LeaseLifecycleContext.signal`) or fails under the daemon's own budget with -// typed evidence. Resetting the daemon here would SIGKILL it mid-`POST /session` -// and orphan the billed session it was about to own, along with every other -// provider session that daemon held (#1774). -const LEASE_ALLOCATE_TIMEOUT_POLICY: CommandTimeoutPolicy = { +// Lease-route commands act on cloud provider sessions the daemon owns on the +// caller's behalf — billed until the daemon releases them. A client that stops +// waiting on one must leave the daemon alive: resetting it would SIGKILL a +// process mid-`POST /session` (allocate) or mid-`DELETE` (release) and orphan +// the session in flight, along with every other provider session that daemon +// held (#1774). Allocation additionally carries its own envelope: the request +// either finishes and is released for the gone requester (see +// `LeaseLifecycleContext.signal`) or fails under the daemon's budget with +// typed evidence. +const LEASE_TIMEOUT_POLICY: CommandTimeoutPolicy = { ...DEFAULT_TIMEOUT_POLICY, - envelopeMs: LEASE_ALLOCATE_REQUEST_TIMEOUT_MS, 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; @@ -423,7 +427,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, }, @@ -434,7 +438,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/request/cancel.test.ts b/src/request/cancel.test.ts index 89e13ecc32..0104b58a89 100644 --- a/src/request/cancel.test.ts +++ b/src/request/cancel.test.ts @@ -5,7 +5,12 @@ import { createRequestCanceledError, isRequestCanceledError, } from '@agent-device/kernel/errors'; -import { resolveRequestTrackingId } from './cancel.ts'; +import { + clearRequestAbortRegistration, + markRequestCanceled, + registerRequestAbort, + resolveRequestTrackingId, +} from './cancel.ts'; test('resolveRequestTrackingId generates unique ids for fallback seeds', () => { const first = resolveRequestTrackingId(undefined, 42); @@ -45,3 +50,23 @@ test('isRequestCanceledError accepts structured and legacy cancellation errors', 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 246870a6d6..c5f07a3be4 100644 --- a/src/request/cancel.ts +++ b/src/request/cancel.ts @@ -74,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( From ade7b5c6ac3cb0a0b8f7ddc7d007b88e6877015d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 14:39:15 +0200 Subject: [PATCH 05/12] refactor: dedupe billed-resource cleanup and lease-signal wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shrink pass — same behavior, less duplication: - releaseOnFailure(primaryError, release) in webdriver-utils replaces the two identical 'best-effort stop the billed resource, attach cleanupError to the primary AppError' helpers (WebDriver session + AWS remote-access ARN); shared errorMessage too. - The lease handler pulls the request signal from getRequestSignal(requestId) like every sibling handler, instead of threading a requestSignal arg through LeaseHandlerArgs and the request-handler chain. Drops the field, the wiring, and five mechanical test edits; the handler test now proves the request-bound signal (abort it, watch the provider's signal flip) rather than arg identity. - Inlined the one-use requestHeaders back into fetchWebDriver. Handler-signal test proven red without the wiring. --- .../provider-webdriver/src/runtime-session.ts | 25 +----- .../src/webdriver-transport.ts | 14 ++-- .../provider-webdriver/src/webdriver-utils.ts | 25 ++++++ .../__tests__/request-handler-catalog.test.ts | 84 ++++++++++--------- src/daemon/handlers/lease.ts | 9 +- src/daemon/request-handler-chain.ts | 1 - 6 files changed, 83 insertions(+), 75 deletions(-) diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index e4651fef61..50eb03a4aa 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -18,6 +18,7 @@ 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 { errorMessage, releaseOnFailure } from './webdriver-utils.ts'; import type { CloudWebDriverBaseSession, CloudWebDriverPlatform, @@ -189,7 +190,7 @@ export class WebDriverSessionManager { const canceled = createRequestCanceledError( canceledAllocationEvidence(this.options.provider, lease), ); - await cleanupAfterCreateSessionFailure(handle.prepared, canceled); + await releaseOnFailure(canceled, () => handle.prepared.cleanup?.()); throw canceled; } const session = await this.createSessionOrCleanup(handle, lease, req); @@ -212,7 +213,7 @@ export class WebDriverSessionManager { const failure = isWebDriverRequestTimeout(error) ? sessionCreateTimeoutError(error, this.options.provider, lease, handle.prepared) : error; - await cleanupAfterCreateSessionFailure(handle.prepared, failure); + await releaseOnFailure(failure, () => handle.prepared.cleanup?.()); throw failure; } } @@ -332,26 +333,6 @@ export function buildCloudWebDriverBaseCapabilities( }; } -function errorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error); -} - -async function cleanupAfterCreateSessionFailure( - prepared: CloudWebDriverPreparedSession, - primaryError: unknown, -): Promise { - try { - await prepared.cleanup?.(); - } catch (cleanupError) { - if (primaryError instanceof AppError) { - primaryError.details = { - ...primaryError.details, - cleanupError: errorMessage(cleanupError), - }; - } - } -} - /** * The transport gave up on `POST /session`; the provider may still finish it. * Nothing here can learn that session's id, so the error carries what the diff --git a/packages/provider-webdriver/src/webdriver-transport.ts b/packages/provider-webdriver/src/webdriver-transport.ts index a87597da38..0e836e06a0 100644 --- a/packages/provider-webdriver/src/webdriver-transport.ts +++ b/packages/provider-webdriver/src/webdriver-transport.ts @@ -161,7 +161,11 @@ export class WebDriverTransport { try { const response = await fetch(new URL(trimLeadingSlash(path), this.endpoint), { method, - headers: this.requestHeaders(body), + headers: { + Accept: 'application/json', + ...(body === undefined ? {} : { 'Content-Type': 'application/json' }), + ...this.headers, + }, body: body === undefined ? undefined : JSON.stringify(body), signal, }); @@ -176,14 +180,6 @@ export class WebDriverTransport { throw error; } } - - private requestHeaders(body: unknown): Record { - return { - Accept: 'application/json', - ...(body === undefined ? {} : { 'Content-Type': 'application/json' }), - ...this.headers, - }; - } } function shouldRetryWebDriverRequest( diff --git a/packages/provider-webdriver/src/webdriver-utils.ts b/packages/provider-webdriver/src/webdriver-utils.ts index a4c887f526..1788fab5b9 100644 --- a/packages/provider-webdriver/src/webdriver-utils.ts +++ b/packages/provider-webdriver/src/webdriver-utils.ts @@ -1,7 +1,32 @@ import type { DeviceLease } from '@agent-device/contracts/device'; +import { AppError } from '@agent-device/kernel/errors'; export type LeaseValue = T | ((lease: DeviceLease) => T); +export function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +/** + * Best-effort release of a billed provider resource once its allocation has + * already failed. The primary failure is what the caller wants to see, so a + * failure to release only rides along as `details.cleanupError` rather than + * masking it — and cleanup runs to completion regardless, since the resource + * bills until it is stopped (#1774). + */ +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/daemon/__tests__/request-handler-catalog.test.ts b/src/daemon/__tests__/request-handler-catalog.test.ts index 688e944a4b..1143feb56a 100644 --- a/src/daemon/__tests__/request-handler-catalog.test.ts +++ b/src/daemon/__tests__/request-handler-catalog.test.ts @@ -11,6 +11,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, @@ -93,7 +98,6 @@ test('lease handler executes commands owned by the lease route', async () => { sessionName: 'catalog-test', sessionStore, leaseRegistry, - requestSignal: new AbortController().signal, }); assert.notEqual(response, null, `${command} should be handled by lease handler`); @@ -121,7 +125,6 @@ test('lease handler preserves device-aware lease fields', async () => { sessionName: 'catalog-test', sessionStore, leaseRegistry, - requestSignal: new AbortController().signal, }); assert.equal(allocateResponse?.ok, true); @@ -149,7 +152,6 @@ test('lease handler preserves device-aware lease fields', async () => { sessionName: 'catalog-test', sessionStore, leaseRegistry, - requestSignal: new AbortController().signal, }); assert.equal(heartbeatResponse?.ok, true); @@ -170,7 +172,6 @@ test('lease artifacts lists daemon inventory for proxy lease scopes', async () = sessionName: 'catalog-test', sessionStore, leaseRegistry, - requestSignal: new AbortController().signal, }); assertProxyLeaseArtifactInventory(response, tracked.artifactId); @@ -201,7 +202,6 @@ test('lease release calls provider hook using the released lease without heartbe sessionName: 'catalog-test', sessionStore, leaseRegistry, - requestSignal: new AbortController().signal, }); assert.equal(allocateResponse?.ok, true); const lease = readLeaseResponse(allocateResponse); @@ -230,7 +230,6 @@ test('lease release calls provider hook using the released lease without heartbe return { provider: releasedLease.leaseProvider }; }, }, - requestSignal: new AbortController().signal, }); assert.equal(releaseResponse?.ok, true); @@ -246,47 +245,54 @@ test('lease release calls provider hook using the released lease without heartbe // 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 passes the request signal and a deadline to the provider', async () => { +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-'); - const requestSignal = new AbortController().signal; + // 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; - const response = await handleLeaseCommands({ - req: { - command: INTERNAL_COMMANDS.leaseAllocate, - token: 'test-token', - session: 'catalog-test', - meta: { - tenantId: 'tenant-a', - runId: 'run-a', - leaseBackend: 'android-instance', - leaseProvider: 'fake-provider', + 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: [], }, - positionals: [], - }, - sessionName: 'catalog-test', - sessionStore, - leaseRegistry, - leaseLifecycleProvider: { - allocate: async (_lease, context) => { - observed = { signal: context?.signal, deadline: context?.deadline }; - return { provider: 'fake-provider' }; + sessionName: 'catalog-test', + sessionStore, + leaseRegistry, + leaseLifecycleProvider: { + allocate: async (_lease, context) => { + observed = { signal: context?.signal, deadline: context?.deadline }; + return { provider: 'fake-provider' }; + }, }, - }, - requestSignal, - }); - const after = Date.now(); + }); - assert.equal(response?.ok, true); - assert.equal(observed?.signal, requestSignal); - assert.ok( - typeof observed?.deadline === 'number' && - observed.deadline > before && - observed.deadline > after, - `allocation deadline must be a future instant, got ${String(observed?.deadline)}`, - ); + 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); + } }); function catalogCommandsForRoute(route: Exclude): string[] { diff --git a/src/daemon/handlers/lease.ts b/src/daemon/handlers/lease.ts index bfcbce653c..533789336b 100644 --- a/src/daemon/handlers/lease.ts +++ b/src/daemon/handlers/lease.ts @@ -19,6 +19,7 @@ import { } from '../../core/lease-scope.ts'; import { AppError } from '@agent-device/kernel/errors'; import { LEASE_ALLOCATION_BUDGET_MS } from '../../core/command-descriptor/timeout-policy.ts'; +import { getRequestSignal } from '../../request/cancel.ts'; import { listDownloadableArtifacts } from '../artifact-tracking.ts'; type LeaseHandlerArgs = { @@ -30,8 +31,6 @@ type LeaseHandlerArgs = { providerRuntimeRequiredIds?: readonly string[]; leaseLifecycleProvider?: LeaseLifecycleProvider; cloudArtifactProvider?: CloudArtifactProvider; - /** Request-bound cancellation, handed to the provider as ownership evidence for allocation. */ - requestSignal: AbortSignal; }; export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise { @@ -44,7 +43,6 @@ export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise Date: Mon, 17 Aug 2026 15:36:18 +0200 Subject: [PATCH 06/12] fix(lease): the daemon releases a lease allocated for a gone requester; honest release evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The provider was doing the daemon's job: it treated the request signal as 'ownership evidence, not an interrupt' and needed three paragraphs to say so. The daemon owns the request, so it now decides — generically, for every provider — what happens to a lease that finished allocating after its requester left: release it (provider + registry) and answer with the canceled error. - lease.ts: after allocate returns, isRequestCanceled(requestId) → releaseAllocationForGoneRequester(). Release evidence is claimed ONLY on a clean release (no warnings, no throw); a WEBDRIVER_SESSION_DELETE_FAILED release is reported released:false with providerSessionId + a stop-by-hand hint (thymikee's finding: the previous evidence was success-shaped even when DELETE failed). - WebDriverSessionManager: the createOwnedSession/releaseCanceledSession trio is gone; allocate is plain 'create with a budget; on failure clean up' again. - LeaseLifecycleContext.signal is just cancellation, like everywhere else; the ownership-semantics comments on the contract, client, registry, AWS prepare and utils shrink to what the code no longer says itself. - Tests: the two provider-level cancellation tests move to the daemon handler (where the logic now lives), plus the failing-DELETE regression; both proven red without the post-allocate check. --- packages/contracts/src/device-provider.ts | 15 +-- .../provider-webdriver/src/aws-device-farm.ts | 10 +- .../src/runtime-session.test.ts | 96 --------------- .../provider-webdriver/src/runtime-session.ts | 86 +++----------- .../src/webdriver-client.ts | 18 +-- .../src/webdriver-transport.ts | 6 +- .../provider-webdriver/src/webdriver-utils.ts | 8 +- src/core/command-descriptor/registry.ts | 13 +- src/core/command-descriptor/timeout-policy.ts | 11 +- .../__tests__/request-handler-catalog.test.ts | 112 ++++++++++++++++++ src/daemon/handlers/lease.ts | 102 +++++++++++++--- 11 files changed, 233 insertions(+), 244 deletions(-) diff --git a/packages/contracts/src/device-provider.ts b/packages/contracts/src/device-provider.ts index ffa0f3f2d2..4cf6c88e2a 100644 --- a/packages/contracts/src/device-provider.ts +++ b/packages/contracts/src/device-provider.ts @@ -25,19 +25,12 @@ export type DeviceLease = { export type LeaseLifecycleContext = { flags?: Readonly>; cwd?: string; - /** - * Request-bound cancellation: aborted once the requester is gone (explicit - * cancel or client disconnect). For allocation it is an OWNERSHIP signal, not - * an interrupt — a provider whose remote allocation has already committed - * finishes it and releases the result rather than abandoning a billed - * resource nobody holds the id of. - */ + /** Request-bound cancellation (explicit cancel or client disconnect). */ signal?: AbortSignal; /** - * Epoch-ms deadline by which `allocate` must have settled. The daemon derives - * it from the same budget as the client's `lease_allocate` request envelope, - * so a provider that fits its remote phases within it is never abandoned by - * a client that stopped waiting first. + * 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; }; diff --git a/packages/provider-webdriver/src/aws-device-farm.ts b/packages/provider-webdriver/src/aws-device-farm.ts index 238be7983f..99208c4e71 100644 --- a/packages/provider-webdriver/src/aws-device-farm.ts +++ b/packages/provider-webdriver/src/aws-device-farm.ts @@ -216,12 +216,8 @@ export function createAwsDeviceFarmPrepareSession( interactionMode: options.interactionMode, configuration: options.configuration, }); - // From here the ARN is OURS: a billed remote-access session that nothing - // else will ever stop. Whatever ends the startup wait short of RUNNING — - // startup timeout, allocation deadline, or the requester leaving — must - // stop it before the failure surfaces, or it keeps billing until AWS reaps - // it (the same ownership rule as the WebDriver session in - // WebDriverSessionManager, one phase earlier). + // 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 { @@ -299,8 +295,6 @@ async function waitForRunningRemoteAccessSession( const timeoutMs = options.startupTimeoutMs ?? 120_000; const pollIntervalMs = options.pollIntervalMs ?? 5_000; const startedAt = Date.now(); - // The startup wait fits inside the lease-allocation deadline, so a client - // that stops waiting at that deadline never abandons a still-polling daemon. const deadline = Math.min(startedAt + timeoutMs, req?.deadline ?? Infinity); const signal = req?.signal; let last = await options.client.getRemoteAccessSession(arn); diff --git a/packages/provider-webdriver/src/runtime-session.test.ts b/packages/provider-webdriver/src/runtime-session.test.ts index aee56c6a00..f07ad5c183 100644 --- a/packages/provider-webdriver/src/runtime-session.test.ts +++ b/packages/provider-webdriver/src/runtime-session.test.ts @@ -1,7 +1,6 @@ import assert from 'node:assert/strict'; import { afterEach, test } from 'vitest'; import type { DeviceLease } from '@agent-device/contracts/device'; -import { deviceFieldsFromPublicPlatform, type DeviceInfo } from '@agent-device/kernel/device'; import { AppError } from '@agent-device/kernel/errors'; import { createCloudWebDriverRuntime, type CloudWebDriverRuntimeOptions } from './runtime.ts'; @@ -40,86 +39,6 @@ test('session allocation preserves its primary failure when provider cleanup als } }); -// #1774: a request already gone before the create is issued must create -// NOTHING. Prepared provider resources are cleaned up and no `POST /session` -// goes out, so there is no billed device session to leak. -test('allocation canceled before create issues no session and cleans up prepared work', async () => { - let sessionRequests = 0; - let cleanupCalled = false; - globalThis.fetch = async (input) => { - if (requestUrl(input).endsWith('/session')) sessionRequests += 1; - return jsonResponse({ value: { sessionId: 'wd-1', capabilities: {} } }); - }; - const runtime = makeRuntime({ - prepareSession: async ({ base }) => ({ - ...base, - cleanup: async () => { - cleanupCalled = true; - return undefined; - }, - }), - }); - const controller = new AbortController(); - controller.abort(); - - try { - await assert.rejects( - () => runtime.leaseLifecycle.allocate!(makeLease(), { signal: controller.signal }), - (error: unknown) => { - assert.ok(error instanceof AppError); - assert.equal(error.details?.reason, 'request_canceled'); - return true; - }, - ); - assert.equal(sessionRequests, 0, 'no POST /session may be issued once canceled'); - assert.equal(cleanupCalled, true); - } finally { - await runtime.shutdown(); - } -}); - -// #1774: the classic leak. The requester vanishes WHILE the provider is -// allocating; the create still completes server-side. The manager must not -// register that session (nobody is waiting on it) — it deletes it, holding the -// id it just learned, so the billed session is released instead of orphaned. -test('a session that completes after cancellation is released, not registered', async () => { - const controller = new AbortController(); - let deletedSessionId: string | undefined; - globalThis.fetch = async (input, init) => { - const url = requestUrl(input); - const method = init?.method ?? 'GET'; - if (url.endsWith('/session') && method === 'POST') { - // The client disconnects mid-create; the provider finishes anyway. - controller.abort(); - return jsonResponse({ value: { sessionId: 'wd-live', capabilities: {} } }); - } - if (url.endsWith('/session/wd-live') && method === 'DELETE') { - deletedSessionId = 'wd-live'; - return jsonResponse({ value: null }); - } - throw new Error(`unexpected ${method} ${url}`); - }; - const runtime = makeRuntime(); - const lease = makeLease(); - - try { - await assert.rejects( - () => runtime.leaseLifecycle.allocate!(lease, { signal: controller.signal }), - (error: unknown) => { - assert.ok(error instanceof AppError); - assert.equal(error.details?.reason, 'request_canceled'); - assert.equal(error.details?.releasedWebDriverSessionId, 'wd-live'); - return true; - }, - ); - assert.equal(deletedSessionId, 'wd-live', 'the completed session must be deleted'); - // Nothing registered: the device is not owned and no session answers for it. - assert.equal(runtime.getInteractor(makeDevice(lease)), undefined); - } finally { - await runtime.shutdown(); - } -}); - // #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 @@ -172,21 +91,6 @@ function jsonResponse(body: unknown, status = 200): Response { }); } -function requestUrl(input: RequestInfo | URL): string { - return String(input instanceof Request ? input.url : input); -} - -function makeDevice(lease: DeviceLease): DeviceInfo { - return { - ...deviceFieldsFromPublicPlatform('android'), - id: `webdriver-test:android:${lease.leaseId}`, - name: 'Test device', - kind: 'device', - target: 'mobile', - booted: true, - }; -} - 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 50eb03a4aa..a749b84009 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -4,11 +4,7 @@ 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, - createRequestCanceledError, - type AppErrorDetails, -} from '@agent-device/kernel/errors'; +import { AppError } from '@agent-device/kernel/errors'; import { unavailableCloudArtifactsResult } from './artifact-results.ts'; import { createCloudWebDriverCapabilities, @@ -47,8 +43,6 @@ type CloudWebDriverCloseResult = Readonly<{ warnings: CloudWebDriverReleaseWarning[]; }>; type LeaseResult = Record | undefined; -/** The half of a provider session that exists once the WebDriver session does, registered or not. */ -type ProviderSessionHandle = Pick; /** * Owns WebDriver session lifecycle and stale-owner retention. Deployment decisions stay in the @@ -97,7 +91,7 @@ export class WebDriverSessionManager { headers: prepared.headers, requestPolicy: this.options.requestPolicy, }); - const session = await this.createOwnedSession({ client, prepared }, lease, req); + const session = await this.createSessionWithPreparedCleanup(client, prepared, lease, req); const device = this.deviceForLease(lease, prepared); const providerSessionId = prepared.providerSessionId ?? session.sessionId; const capabilities = createCloudWebDriverCapabilities({ @@ -172,68 +166,25 @@ export class WebDriverSessionManager { this.ownedDeviceIds.clear(); } - /** - * Creates the WebDriver session and settles who owns it. `POST /session` is - * non-idempotent with an indeterminate outcome once abandoned (see - * `WebDriverClient.createSession`), so the request's cancellation is treated - * as ownership evidence rather than an interrupt: a requester that left - * before creation started gets nothing created; one that left while the - * provider was allocating gets the finished session released, because by - * then its id is in hand and nothing else will ever release it (#1774). - */ - private async createOwnedSession( - handle: ProviderSessionHandle, - lease: DeviceLease, - req: LeaseLifecycleContext | undefined, - ): Promise { - if (req?.signal?.aborted) { - const canceled = createRequestCanceledError( - canceledAllocationEvidence(this.options.provider, lease), - ); - await releaseOnFailure(canceled, () => handle.prepared.cleanup?.()); - throw canceled; - } - const session = await this.createSessionOrCleanup(handle, lease, req); - if (req?.signal?.aborted) { - throw await this.releaseCanceledSession(handle, lease, session); - } - return session; - } - - private async createSessionOrCleanup( - handle: ProviderSessionHandle, + private async createSessionWithPreparedCleanup( + client: WebDriverClient, + prepared: CloudWebDriverPreparedSession, lease: DeviceLease, req: LeaseLifecycleContext | undefined, ): Promise { try { - return await handle.client.createSession(handle.prepared.webdriverCapabilities, { + return await client.createSession(prepared.webdriverCapabilities, { deadline: req?.deadline, }); } catch (error) { const failure = isWebDriverRequestTimeout(error) - ? sessionCreateTimeoutError(error, this.options.provider, lease, handle.prepared) + ? sessionCreateTimeoutError(error, this.options.provider, lease, prepared) : error; - await releaseOnFailure(failure, () => handle.prepared.cleanup?.()); + await releaseOnFailure(failure, () => prepared.cleanup?.()); throw failure; } } - private async releaseCanceledSession( - handle: ProviderSessionHandle, - lease: DeviceLease, - session: WebDriverSession, - ): Promise { - const close = await this.closeSession(handle); - return createRequestCanceledError({ - ...canceledAllocationEvidence(this.options.provider, lease), - releasedWebDriverSessionId: session.sessionId, - ...(handle.prepared.providerSessionId - ? { releasedProviderSessionId: handle.prepared.providerSessionId } - : {}), - ...(close.warnings.length > 0 ? { warnings: close.warnings } : {}), - }); - } - private async prepareSession( lease: DeviceLease, req: LeaseLifecycleContext | undefined, @@ -279,7 +230,9 @@ export class WebDriverSessionManager { }; } - private async closeSession(session: ProviderSessionHandle): Promise { + private async closeSession( + session: WebDriverProviderSession, + ): Promise { const warnings: CloudWebDriverReleaseWarning[] = []; let cleanup: Record | undefined; try { @@ -334,11 +287,10 @@ export function buildCloudWebDriverBaseCapabilities( } /** - * The transport gave up on `POST /session`; the provider may still finish it. - * Nothing here can learn that session's id, so the error carries what the - * provider dashboard can be searched by instead — the lease the capabilities - * were labelled with — rather than guessing at REST cleanup of a session this - * process never owned. + * 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, @@ -362,11 +314,3 @@ function sessionCreateTimeoutError( timeout, ); } - -function canceledAllocationEvidence(provider: string, lease: DeviceLease): AppErrorDetails { - return { - provider, - leaseId: lease.leaseId, - hint: 'The lease request was canceled (explicit cancel or client disconnect) while the provider session was being created; the session it produced, if any, was released instead of registered.', - }; -} diff --git a/packages/provider-webdriver/src/webdriver-client.ts b/packages/provider-webdriver/src/webdriver-client.ts index 992ad32acd..96973c9e0e 100644 --- a/packages/provider-webdriver/src/webdriver-client.ts +++ b/packages/provider-webdriver/src/webdriver-client.ts @@ -78,19 +78,11 @@ export class WebDriverClient { } /** - * `POST /session` is the one non-idempotent request in the protocol, and its - * outcome after a client-side abort is indeterminate: a hub that has already - * started allocating a device finishes the session whether or not anyone is - * still listening. So it runs under its own budget with NO retries and NO - * request-bound cancellation — a retry after a timed-out attempt is a second - * billed session, and aborting the request would lose the id of the first - * (#1774). Callers that stop wanting the session while it is being created - * release it once they hold the id (see WebDriverSessionManager). - * - * `deadline` (epoch ms) is the operation this creation belongs to; it can - * only SHORTEN the client's own budget, never extend it, so a daemon request - * that spent most of its allocation window on provider preparation does not - * start a device allocation it cannot wait out. + * `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, diff --git a/packages/provider-webdriver/src/webdriver-transport.ts b/packages/provider-webdriver/src/webdriver-transport.ts index 0e836e06a0..e9d18fde4c 100644 --- a/packages/provider-webdriver/src/webdriver-transport.ts +++ b/packages/provider-webdriver/src/webdriver-transport.ts @@ -24,11 +24,7 @@ export type WebDriverRequestPolicy = { /** 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. Its outcome is INDETERMINATE: - * the server may still complete it — which is why a non-idempotent caller must - * neither retry it nor assume nothing was created. - */ +/** 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; } diff --git a/packages/provider-webdriver/src/webdriver-utils.ts b/packages/provider-webdriver/src/webdriver-utils.ts index 1788fab5b9..fac8ce86e6 100644 --- a/packages/provider-webdriver/src/webdriver-utils.ts +++ b/packages/provider-webdriver/src/webdriver-utils.ts @@ -7,13 +7,7 @@ export function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error); } -/** - * Best-effort release of a billed provider resource once its allocation has - * already failed. The primary failure is what the caller wants to see, so a - * failure to release only rides along as `details.cleanupError` rather than - * masking it — and cleanup runs to completion regardless, since the resource - * bills until it is stopped (#1774). - */ +/** 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, diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index 481050cd23..bcc0093da7 100644 --- a/src/core/command-descriptor/registry.ts +++ b/src/core/command-descriptor/registry.ts @@ -319,15 +319,10 @@ const INSTALL_TIMEOUT_POLICY: CommandTimeoutPolicy = { envelopeMs: INSTALL_REQUEST_TIMEOUT_MS, }; -// Lease-route commands act on cloud provider sessions the daemon owns on the -// caller's behalf — billed until the daemon releases them. A client that stops -// waiting on one must leave the daemon alive: resetting it would SIGKILL a -// process mid-`POST /session` (allocate) or mid-`DELETE` (release) and orphan -// the session in flight, along with every other provider session that daemon -// held (#1774). Allocation additionally carries its own envelope: the request -// either finishes and is released for the gone requester (see -// `LeaseLifecycleContext.signal`) or fails under the daemon's budget with -// typed evidence. +// 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', diff --git a/src/core/command-descriptor/timeout-policy.ts b/src/core/command-descriptor/timeout-policy.ts index a246401f11..9bf218cb99 100644 --- a/src/core/command-descriptor/timeout-policy.ts +++ b/src/core/command-descriptor/timeout-policy.ts @@ -17,13 +17,10 @@ export const INSTALL_REQUEST_TIMEOUT_MS = 180_000; export const REQUEST_TIMEOUT_BUDGET_MARGIN_MS = 30_000; /** - * How long the daemon lets a lease lifecycle provider allocate one lease. Cloud - * providers spend most of it waiting on remote device allocation (BrowserStack - * iOS real devices take 45–90s to create a session; AWS Device Farm remote - * access takes ~2 minutes to reach RUNNING before the session is even created). - * The provider receives it as `LeaseLifecycleContext.deadline` and bounds its - * remote phases within it; the client's `lease_allocate` envelope is derived - * from it below, so the two cannot drift apart (#1774). + * 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; diff --git a/src/daemon/__tests__/request-handler-catalog.test.ts b/src/daemon/__tests__/request-handler-catalog.test.ts index 1143feb56a..2a5fcf6e80 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'; @@ -295,6 +296,117 @@ test('lease allocation hands the provider the request-bound signal and a deadlin } }); +// #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.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 ?? {}; + assert.equal(details.released, false); + 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); + } +}); + +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/handlers/lease.ts b/src/daemon/handlers/lease.ts index 533789336b..155c2d1a61 100644 --- a/src/daemon/handlers/lease.ts +++ b/src/daemon/handlers/lease.ts @@ -1,4 +1,8 @@ -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, @@ -17,9 +21,9 @@ import { leaseScopeToHeartbeatRequest, leaseScopeToReleaseRequest, } from '../../core/lease-scope.ts'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, createRequestCanceledError } from '@agent-device/kernel/errors'; import { LEASE_ALLOCATION_BUDGET_MS } from '../../core/command-descriptor/timeout-policy.ts'; -import { getRequestSignal } from '../../request/cancel.ts'; +import { getRequestSignal, isRequestCanceled } from '../../request/cancel.ts'; import { listDownloadableArtifacts } from '../artifact-tracking.ts'; type LeaseHandlerArgs = { @@ -67,26 +71,18 @@ export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise { + const outcome = await releaseProviderLease(lease, leaseLifecycleProvider); + releaseRegistryLease(leaseRegistry, lease); + return canceledAllocationError(lease, outcome); +} + +type ProviderReleaseOutcome = { + providerSessionId?: unknown; + warnings: unknown[]; + releaseError?: string; +}; + +async function releaseProviderLease( + lease: DeviceLease, + leaseLifecycleProvider: LeaseLifecycleProvider | undefined, +): Promise { + try { + const released = await leaseLifecycleProvider?.release?.(lease); + return { + providerSessionId: released?.providerSessionId, + warnings: Array.isArray(released?.warnings) ? released.warnings : [], + }; + } catch (error) { + return { warnings: [], releaseError: errorMessage(error) }; + } +} + +function canceledAllocationError(lease: DeviceLease, outcome: ProviderReleaseOutcome): AppError { + const { providerSessionId, warnings, releaseError } = outcome; + const released = releaseError === undefined && warnings.length === 0; + return createRequestCanceledError({ + leaseId: lease.leaseId, + leaseProvider: lease.leaseProvider, + providerSessionId, + ...(warnings.length > 0 ? { warnings } : {}), + ...(releaseError !== undefined ? { releaseError } : {}), + released, + hint: released + ? 'The lease request was canceled while the provider was still allocating; the session it produced was released.' + : `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 ${lease.leaseId} by hand.`, + }); +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + function leaseLifecycleContext(req: DaemonRequest): LeaseLifecycleContext { return { flags: req.flags, From 8b3de4a72b7d5f9f1234f5a061edf1da075423fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 17:56:43 +0200 Subject: [PATCH 07/12] fix(aws): the allocation deadline bounds remote-access startup, not the 120s default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live iOS real-device run: startup needed ~128s and hit the standalone 120s default while the daemon's 300s allocation budget still had room — the new ownership guard correctly stopped the ARN, but the open failed for no reason. When the daemon supplies a deadline it is the bound; the default only applies standalone. Rerun: open in 112s, snapshot, clean close, session STOPPING. --- packages/provider-webdriver/src/aws-device-farm.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/provider-webdriver/src/aws-device-farm.ts b/packages/provider-webdriver/src/aws-device-farm.ts index 99208c4e71..58ded3c784 100644 --- a/packages/provider-webdriver/src/aws-device-farm.ts +++ b/packages/provider-webdriver/src/aws-device-farm.ts @@ -292,10 +292,12 @@ async function waitForRunningRemoteAccessSession( }, req: LeaseLifecycleContext | undefined, ): Promise { - const timeoutMs = options.startupTimeoutMs ?? 120_000; const pollIntervalMs = options.pollIntervalMs ?? 5_000; const startedAt = Date.now(); - const deadline = Math.min(startedAt + timeoutMs, req?.deadline ?? Infinity); + // 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() < deadline) { From c6d51f691f7fdb49f410926d8107727f630daa60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 18:32:17 +0200 Subject: [PATCH 08/12] test(aws): pin that the allocation deadline outlives the 120s startup default; drop empty import Review follow-ups on 7f9d1481a: a virtual-clock test (Date.now advanced 10s per poll, RUNNING at 150s, deadline 300s) that fails on the old min(default, deadline) logic and passes now; and the empty 'import {} from kernel/errors' left in maestro/shared.ts is removed. --- packages/maestro/src/internal/shared.ts | 1 - .../src/aws-device-farm.test.ts | 43 ++++++++++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/maestro/src/internal/shared.ts b/packages/maestro/src/internal/shared.ts index 75e5047cf8..1b9b0d841c 100644 --- a/packages/maestro/src/internal/shared.ts +++ b/packages/maestro/src/internal/shared.ts @@ -1,4 +1,3 @@ -import {} from '@agent-device/kernel/errors'; import type { Point, Rect, SnapshotNode } from '@agent-device/kernel/snapshot'; export function stripUndefined>(value: T): T { diff --git a/packages/provider-webdriver/src/aws-device-farm.test.ts b/packages/provider-webdriver/src/aws-device-farm.test.ts index 52b1135c22..89a83e6f9c 100644 --- a/packages/provider-webdriver/src/aws-device-farm.test.ts +++ b/packages/provider-webdriver/src/aws-device-farm.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { test } from 'vitest'; +import { afterEach, test, vi } from 'vitest'; import type { DeviceLease } from '@agent-device/contracts/device'; import { AppError, @@ -15,6 +15,10 @@ 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 @@ -75,6 +79,40 @@ test('the allocation deadline caps the startup wait below its own default', asyn 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', @@ -91,10 +129,11 @@ test('a session that reaches RUNNING is handed on and not stopped', async () => function fakeClient( session: Partial, onPoll?: () => void, -): AwsDeviceFarmClient & { stopped: string[] } { +): AwsDeviceFarmClient & { stopped: string[]; session: Partial } { const stopped: string[] = []; return { stopped, + session, createRemoteAccessSession: async () => ({ arn: ARN, status: 'PENDING' }), getRemoteAccessSession: async (arn) => { onPoll?.(); From 8c83aec6202d5fbdedcb923b382d8c5ce42a7847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 18:41:34 +0200 Subject: [PATCH 09/12] =?UTF-8?q?refactor:=20finish=20the=20dedupe=20?= =?UTF-8?q?=E2=80=94=20one=20release=20path,=20kernel=20errorMessage,=20AW?= =?UTF-8?q?S=20on=20releaseOnFailure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-quality review at 7f9d1481a: 1. aws-device-farm.ts still carried its own copy of releaseOnFailure (the dedupe commit's script aborted before reaching it and I mis-verified). Now uses the shared helper; private copy deleted. 2. Empty 'import {} from kernel/errors' in maestro/shared.ts removed (273870099). 3. errorMessage() lives in @agent-device/kernel/errors; the two copies this PR had added (lease.ts, webdriver-utils.ts) import it. Sweeping the pre-existing copies is a follow-up. 4. lease.ts has ONE release path: releaseLease(registry, provider, lease, request, ctx) → { released (registry), provider } used by both the lease_release case (wire shape unchanged) and the gone-requester branch, which folds a throwing provider release into releaseError. 'released' now means the same thing in both; the provider verdict is a separate 'providerReleased' (warnings-free, no throw) that drives the stop-by-hand hint. -~35 lines. 5. sessionCreateTimeoutMs is Omit-ed at the WebDriverTransportOptions boundary instead of Pick-ed back out internally. --- packages/kernel/src/errors.ts | 5 + .../provider-webdriver/src/aws-device-farm.ts | 21 +-- .../provider-webdriver/src/runtime-session.ts | 4 +- .../src/webdriver-transport.ts | 5 +- .../provider-webdriver/src/webdriver-utils.ts | 6 +- .../__tests__/request-handler-catalog.test.ts | 4 +- src/daemon/handlers/lease.ts | 122 ++++++++++-------- 7 files changed, 82 insertions(+), 85 deletions(-) diff --git a/packages/kernel/src/errors.ts b/packages/kernel/src/errors.ts index 3cae51fce6..4c3b976403 100644 --- a/packages/kernel/src/errors.ts +++ b/packages/kernel/src/errors.ts @@ -162,6 +162,11 @@ export function isRequestCanceledError(error: unknown): boolean { 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/provider-webdriver/src/aws-device-farm.ts b/packages/provider-webdriver/src/aws-device-farm.ts index 58ded3c784..02f35f7ff2 100644 --- a/packages/provider-webdriver/src/aws-device-farm.ts +++ b/packages/provider-webdriver/src/aws-device-farm.ts @@ -24,7 +24,7 @@ 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 = { @@ -231,7 +231,7 @@ export function createAwsDeviceFarmPrepareSession( ); } } catch (error) { - await stopRemoteAccessSessionAfterFailure(options.client, remoteAccess.arn, error); + await releaseOnFailure(error, () => options.client.stopRemoteAccessSession(remoteAccess.arn)); throw error; } const deviceName = running.device?.name ?? options.deviceName; @@ -327,23 +327,6 @@ function throwIfRemoteAccessSessionEnded(session: AwsDeviceFarmRemoteAccessSessi }); } -async function stopRemoteAccessSessionAfterFailure( - client: AwsDeviceFarmClient, - arn: string, - primaryError: unknown, -): Promise { - try { - await client.stopRemoteAccessSession(arn); - } catch (cleanupError) { - if (primaryError instanceof AppError) { - primaryError.details = { - ...primaryError.details, - cleanupError: cleanupError instanceof Error ? cleanupError.message : String(cleanupError), - }; - } - } -} - async function runAwsJson( runHostCommand: RunHostCommand, command: string, diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index a749b84009..b309290dd6 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -4,7 +4,7 @@ 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, @@ -14,7 +14,7 @@ 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 { errorMessage, releaseOnFailure } from './webdriver-utils.ts'; +import { releaseOnFailure } from './webdriver-utils.ts'; import type { CloudWebDriverBaseSession, CloudWebDriverPlatform, diff --git a/packages/provider-webdriver/src/webdriver-transport.ts b/packages/provider-webdriver/src/webdriver-transport.ts index e9d18fde4c..735592b34e 100644 --- a/packages/provider-webdriver/src/webdriver-transport.ts +++ b/packages/provider-webdriver/src/webdriver-transport.ts @@ -46,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 = { @@ -61,7 +62,7 @@ type ResolvedWebDriverRequestOverrides = { }; type ResolvedWebDriverRequestPolicy = Required< - Pick + NonNullable >; /** Focused HTTP/retry policy for one WebDriver endpoint; session semantics stay in WebDriverClient. */ diff --git a/packages/provider-webdriver/src/webdriver-utils.ts b/packages/provider-webdriver/src/webdriver-utils.ts index fac8ce86e6..5af69a9f4c 100644 --- a/packages/provider-webdriver/src/webdriver-utils.ts +++ b/packages/provider-webdriver/src/webdriver-utils.ts @@ -1,12 +1,8 @@ import type { DeviceLease } from '@agent-device/contracts/device'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, errorMessage } from '@agent-device/kernel/errors'; export type LeaseValue = T | ((lease: DeviceLease) => T); -export function errorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error); -} - /** Best-effort release after a failure; a failed release rides along as `cleanupError`, never masks the primary. */ export async function releaseOnFailure( primaryError: unknown, diff --git a/src/daemon/__tests__/request-handler-catalog.test.ts b/src/daemon/__tests__/request-handler-catalog.test.ts index 2a5fcf6e80..064f5254d9 100644 --- a/src/daemon/__tests__/request-handler-catalog.test.ts +++ b/src/daemon/__tests__/request-handler-catalog.test.ts @@ -331,6 +331,7 @@ test('a lease allocated for a requester that left is released, not registered', assert.ok(isRequestCanceledError(error)); const details = (error as AppError).details ?? {}; assert.equal(details.released, true); + assert.equal(details.providerReleased, true); assert.match(String(details.providerSessionId), /^bs-/); return true; }, @@ -376,7 +377,8 @@ test('a failed provider release after cancellation is reported as unreleased wit (error: unknown) => { assert.ok(isRequestCanceledError(error)); const details = (error as AppError).details ?? {}; - assert.equal(details.released, false); + assert.equal(details.released, true, 'the daemon lease record is gone either way'); + assert.equal(details.providerReleased, false); assert.equal(details.providerSessionId, 'bs-live'); assert.match(String(details.hint), /could NOT be confirmed released/); assert.match(String(details.hint), /bs-live/); diff --git a/src/daemon/handlers/lease.ts b/src/daemon/handlers/lease.ts index 155c2d1a61..1ec3d3b430 100644 --- a/src/daemon/handlers/lease.ts +++ b/src/daemon/handlers/lease.ts @@ -9,7 +9,7 @@ import type { 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, @@ -21,7 +21,7 @@ import { leaseScopeToHeartbeatRequest, leaseScopeToReleaseRequest, } from '../../core/lease-scope.ts'; -import { AppError, createRequestCanceledError } 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'; @@ -75,7 +75,7 @@ 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 { released: 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 the (absent) requester - * would have received. Release evidence is only claimed on a clean release: a - * provider that could not delete its session (`warnings`) is reported as such, - * with the identifiers an operator needs to stop it by hand. + * 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 outcome = await releaseProviderLease(lease, leaseLifecycleProvider); - releaseRegistryLease(leaseRegistry, lease); - return canceledAllocationError(lease, outcome); -} - -type ProviderReleaseOutcome = { - providerSessionId?: unknown; - warnings: unknown[]; - releaseError?: string; -}; - -async function releaseProviderLease( - lease: DeviceLease, - leaseLifecycleProvider: LeaseLifecycleProvider | undefined, -): Promise { + const request = leaseReleaseRequestFor(lease); + let outcome: LeaseReleaseOutcome; + let releaseError: string | undefined; try { - const released = await leaseLifecycleProvider?.release?.(lease); - return { - providerSessionId: released?.providerSessionId, - warnings: Array.isArray(released?.warnings) ? released.warnings : [], - }; + outcome = await releaseLease(leaseRegistry, leaseLifecycleProvider, lease, request); } catch (error) { - return { warnings: [], releaseError: errorMessage(error) }; + releaseError = errorMessage(error); + outcome = { released: leaseRegistry.releaseLease(request).released }; } + return canceledAllocationError(lease, outcome, releaseError); } -function canceledAllocationError(lease: DeviceLease, outcome: ProviderReleaseOutcome): AppError { - const { providerSessionId, warnings, releaseError } = outcome; - const released = releaseError === undefined && warnings.length === 0; +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 : []; + const providerReleased = releaseError === undefined && warnings.length === 0; return createRequestCanceledError({ leaseId: lease.leaseId, leaseProvider: lease.leaseProvider, + released: outcome.released, + providerReleased, providerSessionId, ...(warnings.length > 0 ? { warnings } : {}), ...(releaseError !== undefined ? { releaseError } : {}), - released, - hint: released + hint: providerReleased ? 'The lease request was canceled while the provider was still allocating; the session it produced was released.' : `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 ${lease.leaseId} by hand.`, }); } -function errorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error); -} - function leaseLifecycleContext(req: DaemonRequest): LeaseLifecycleContext { return { flags: req.flags, From d5a6c0cab03a9799dd3b714d14bc2c2540c8b0d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Mon, 17 Aug 2026 19:21:13 +0200 Subject: [PATCH 10/12] fix(lease): 'released' on a canceled allocation means the billed session is confirmed gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review at 3665ea06: unifying the release path had made the cancellation error report released:true from the daemon's registry record while the provider DELETE had failed — success-shaped again, with the operator verdict demoted to a second key. Fixed at the source of the ambiguity: - LeaseReleaseOutcome names its bookkeeping field registryReleased. - On the canceled error, 'released' is true only when registryReleased AND the provider released without warnings AND without throwing; the registry record is exposed as 'registryReleased'. The stop-by-hand hint keys on 'released'. - lease_release keeps its existing wire field ('released' = registry; provider cleanup rides in 'provider'), unchanged. - Regressions: failed DELETE and throwing release both pin released:false / registryReleased:true (+ providerSessionId, warnings|releaseError, hint); both proven red on registry-only semantics. --- .../__tests__/request-handler-catalog.test.ts | 47 +++++++++++++++++-- src/daemon/handlers/lease.ts | 34 +++++++++----- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/src/daemon/__tests__/request-handler-catalog.test.ts b/src/daemon/__tests__/request-handler-catalog.test.ts index 064f5254d9..09e31d5f6d 100644 --- a/src/daemon/__tests__/request-handler-catalog.test.ts +++ b/src/daemon/__tests__/request-handler-catalog.test.ts @@ -331,7 +331,7 @@ test('a lease allocated for a requester that left is released, not registered', assert.ok(isRequestCanceledError(error)); const details = (error as AppError).details ?? {}; assert.equal(details.released, true); - assert.equal(details.providerReleased, true); + assert.equal(details.registryReleased, true); assert.match(String(details.providerSessionId), /^bs-/); return true; }, @@ -377,8 +377,10 @@ test('a failed provider release after cancellation is reported as unreleased wit (error: unknown) => { assert.ok(isRequestCanceledError(error)); const details = (error as AppError).details ?? {}; - assert.equal(details.released, true, 'the daemon lease record is gone either way'); - assert.equal(details.providerReleased, false); + // `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/); @@ -393,6 +395,45 @@ test('a failed provider release after cancellation is reported as unreleased wit } }); +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, diff --git a/src/daemon/handlers/lease.ts b/src/daemon/handlers/lease.ts index 1ec3d3b430..9b235d878b 100644 --- a/src/daemon/handlers/lease.ts +++ b/src/daemon/handlers/lease.ts @@ -111,7 +111,8 @@ export async function handleLeaseCommands(args: LeaseHandlerArgs): Promise; }; @@ -149,7 +150,7 @@ async function releaseLease( context?: LeaseLifecycleContext, ): Promise { const provider = lease ? await leaseLifecycleProvider?.release?.(lease, context) : undefined; - return { released: leaseRegistry.releaseLease(request).released, provider }; + return { registryReleased: leaseRegistry.releaseLease(request).released, provider }; } /** @@ -171,7 +172,7 @@ async function releaseAllocationForGoneRequester( outcome = await releaseLease(leaseRegistry, leaseLifecycleProvider, lease, request); } catch (error) { releaseError = errorMessage(error); - outcome = { released: leaseRegistry.releaseLease(request).released }; + outcome = { registryReleased: leaseRegistry.releaseLease(request).released }; } return canceledAllocationError(lease, outcome, releaseError); } @@ -183,21 +184,32 @@ function canceledAllocationError( ): AppError { const providerSessionId = outcome.provider?.providerSessionId; const warnings = Array.isArray(outcome.provider?.warnings) ? outcome.provider.warnings : []; - const providerReleased = releaseError === undefined && warnings.length === 0; + // `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: outcome.released, - providerReleased, + released, + registryReleased: outcome.registryReleased, providerSessionId, ...(warnings.length > 0 ? { warnings } : {}), ...(releaseError !== undefined ? { releaseError } : {}), - hint: providerReleased - ? 'The lease request was canceled while the provider was still allocating; the session it produced was released.' - : `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 ${lease.leaseId} by hand.`, + 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, From c5595fa4e733c13f0f65f6fbdcf647ca8c340195 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 11:36:10 +0200 Subject: [PATCH 11/12] ci: retrigger default-setup CodeQL Run 32051017472 is wedged on GitHub's side: status=completed with Analyze (python) still queued and Analyze (java-kotlin) failed only at SARIF upload (503, 'No server is currently available'). It can be neither cancelled nor rerun, and default-setup CodeQL has no dispatchable workflow, so a new push is the only way to get a fresh run. No source change. From ba79c23fa21f195edf06908dfd055d1babc66ff4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 14:22:07 +0200 Subject: [PATCH 12/12] test(webdriver): assert the typed timeout contract on the shared-budget probe main's #1790 tightened this test to expect the raw TimeoutError DOMException, which this PR intentionally normalizes into AppError{reason: webdriver_request_timeout}. On the merge ref the two met and Coverage went red. The regression now asserts the structured contract and that the second request's budget is the shared remainder (~118ms of 200 after an 80ms first call). --- .../src/webdriver-client.test.ts | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/provider-webdriver/src/webdriver-client.test.ts b/packages/provider-webdriver/src/webdriver-client.test.ts index 3f7d714f4d..a8bd8e627f 100644 --- a/packages/provider-webdriver/src/webdriver-client.test.ts +++ b/packages/provider-webdriver/src/webdriver-client.test.ts @@ -317,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.