From 0f58d95ea0a7ed853978fa47c16348dcd7e75bd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 08:11:58 +0100 Subject: [PATCH 1/5] fix(provider-webdriver): allocate one WebDriver session per lease Two concurrent lease_allocate calls for the same lease id each rented a billed cloud session: the reuse check ran after an await, so neither call could see the other's session, and only the last one published was ever releasable. A release or shutdown that landed while a create was in flight also found nothing to dispose. Publish the lease's entry before the first await so a duplicate allocation joins the in-flight create instead of starting its own. Model the entry as a discriminated union: allocating until the hub answers, then ready. Release and shutdown take the entry out of the map in one synchronous step, so whoever holds it disposes the session exactly once, including a session that finishes renting after the lease is already gone. --- .../src/runtime-session-allocation.test.ts | 262 +++++++++++++++ .../provider-webdriver/src/runtime-session.ts | 299 ++++++++++++++---- 2 files changed, 503 insertions(+), 58 deletions(-) create mode 100644 packages/provider-webdriver/src/runtime-session-allocation.test.ts diff --git a/packages/provider-webdriver/src/runtime-session-allocation.test.ts b/packages/provider-webdriver/src/runtime-session-allocation.test.ts new file mode 100644 index 0000000000..b31d188f72 --- /dev/null +++ b/packages/provider-webdriver/src/runtime-session-allocation.test.ts @@ -0,0 +1,262 @@ +import assert from 'node:assert/strict'; +import { afterEach, test } from 'vitest'; +import type { DeviceLease } from '@agent-device/contracts/device'; +import { AppError } from '@agent-device/kernel/errors'; +import { createCloudWebDriverRuntime, type CloudWebDriverRuntimeOptions } from './runtime.ts'; +import { consumeAllProfileFields } from './profile-fields.fixtures.ts'; + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; +}); + +/** + * `POST /session` is billed and the lease is the only handle that can stop it again. Two allocations + * that overlapped both passed the live-session check, so the second hub session overwrote the first + * in the manager's map and only one of the two was ever releasable. + */ +test('two concurrent allocations of one lease create one billed session', async () => { + const hub = startWebDriverHub(); + globalThis.fetch = hub.fetch; + const provider = holdProviderPreparation(); + const runtime = makeRuntime({ prepareSession: provider.prepareSession }); + const lease = makeLease(); + + try { + const first = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await provider.waitForPreparation(); + const second = settle(() => runtime.leaseLifecycle.allocate!(lease)); + // One scheduler boundary while the first allocation is still held before the hub: the second has + // then reached the lease, and the first cannot have published its session yet. + await schedulerYield(); + provider.answerPreparation(); + + const [firstOutcome, secondOutcome] = await Promise.all([first, second]); + assert.equal(hub.createdSessionIds.length, 1, 'one lease bills one session'); + assertResolvedWith(firstOutcome, 'session-1'); + assertResolvedWith(secondOutcome, 'session-1'); + + await runtime.leaseLifecycle.release!(lease); + assert.deepEqual(hub.deletedSessionIds, hub.createdSessionIds); + assert.equal(provider.cleanups, 1); + } finally { + provider.answerPreparation(); + await runtime.shutdown(); + } +}); + +/** + * A release that landed mid-creation found no published session and answered that the lease held + * nothing, so the hub session that creation went on to make had no route left to stop it. The caller + * whose session was disposed is refused, rather than handed a session id nothing will release. + */ +test('a release during creation disposes the session that creation makes', async () => { + const hub = startWebDriverHub(); + globalThis.fetch = hub.fetch; + const provider = holdProviderPreparation(); + const runtime = makeRuntime({ prepareSession: provider.prepareSession }); + const lease = makeLease(); + + try { + const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await provider.waitForPreparation(); + // The release takes the lease's entry in its first synchronous step, while the create is held. + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + provider.answerPreparation(); + + assert.deepEqual(await releasing, { + status: 'resolved', + value: { provider: 'webdriver-test', providerSessionId: 'session-1' }, + }); + assert.deepEqual(hub.deletedSessionIds, ['session-1']); + assert.equal(provider.cleanups, 1); + assertRejectedWith( + await allocating, + 'provider_session_released_during_allocation', + lease.leaseId, + ); + } finally { + provider.answerPreparation(); + await runtime.shutdown(); + } +}); + +/** The same race on the route that ends every session this process holds. */ +test('shutdown during creation disposes the session that creation makes', async () => { + const hub = startWebDriverHub(); + globalThis.fetch = hub.fetch; + const provider = holdProviderPreparation(); + const runtime = makeRuntime({ prepareSession: provider.prepareSession }); + + try { + const allocating = settle(() => runtime.leaseLifecycle.allocate!(makeLease())); + await provider.waitForPreparation(); + // Shutdown takes every entry in its first synchronous step, while the create is held. + const shuttingDown = settle(() => runtime.shutdown()); + provider.answerPreparation(); + + assert.deepEqual(await shuttingDown, { status: 'resolved', value: undefined }); + assert.deepEqual(hub.deletedSessionIds, ['session-1']); + assert.equal(provider.cleanups, 1); + assertRejectedWith(await allocating, 'provider_session_shutdown_during_allocation'); + } finally { + provider.answerPreparation(); + await runtime.shutdown(); + } +}); + +/** + * The hub answers every WebDriver request and records the session ids it was asked to create and + * delete, so a test's lever over an in-flight create is {@link holdProviderPreparation}. + */ +function startWebDriverHub() { + const createdSessionIds: string[] = []; + const deletedSessionIds: string[] = []; + return { + createdSessionIds, + deletedSessionIds, + fetch: (async (input: unknown, init?: RequestInit) => { + const request = new Request(input as RequestInfo, init); + const path = new URL(request.url).pathname; + if (request.method === 'POST' && path.endsWith('/session')) { + const sessionId = `session-${String(createdSessionIds.length + 1)}`; + createdSessionIds.push(sessionId); + return jsonResponse({ sessionId, capabilities: {} }); + } + if (request.method === 'DELETE') { + deletedSessionIds.push(decodeURIComponent(path.split('/').pop()!)); + } + return jsonResponse({ value: null }); + }) as typeof fetch, + }; +} + +/** + * Holds `prepareSession` — the step that runs before this manager sends `POST /session` — until the + * test answers it, so a create stays in flight across a release or a shutdown instead of the test + * racing one with a sleep. It also counts the provider cleanup a prepared session owes, which is the + * half of disposal the hub's `DELETE` does not cover. + */ +function holdProviderPreparation() { + const state = { prepared: 0, cleanups: 0 }; + const arrivals: (() => void)[] = []; + let settleGate: () => void = () => {}; + const gate = new Promise((settle) => { + settleGate = settle; + }); + const prepareSession: NonNullable = async ({ + base, + }) => { + state.prepared += 1; + for (const arrived of arrivals.splice(0)) arrived(); + await gate; + return { + ...base, + cleanup: async () => { + state.cleanups += 1; + return undefined; + }, + }; + }; + return Object.assign(state, { + prepareSession, + /** Resolves once an allocation has reached `prepareSession` and is held there. */ + waitForPreparation: async () => { + while (state.prepared < 1) { + await new Promise((arrived) => { + arrivals.push(arrived); + }); + } + }, + answerPreparation: () => { + settleGate(); + }, + }); +} + +/** How one lease-lifecycle call ended, so a test can assert disposal before the caller's outcome. */ +type Settled = + | Readonly<{ status: 'resolved'; value: T }> + | Readonly<{ status: 'rejected'; error: unknown }>; + +/** Runs a call and turns its outcome into a value, so no rejection goes unobserved. */ +async function settle(run: () => Promise): Promise> { + try { + return { status: 'resolved', value: await run() }; + } catch (error) { + return { status: 'rejected', error }; + } +} + +/** Yields one macrotask, so every promise the test has started runs the microtasks it already queued. */ +async function schedulerYield(): Promise { + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); +} + +/** The allocation envelope names the hub session the lease now holds. */ +function assertResolvedWith( + outcome: Settled | undefined>, + sessionId: string, +): void { + assert.equal(outcome.status, 'resolved', `expected ${sessionId}, got ${describe(outcome)}`); + assert.equal( + (outcome as Readonly<{ value: Record | undefined }>).value?.sessionId, + sessionId, + ); +} + +/** + * The typed reason an allocation reports when the session it made had already been taken to be + * disposed, and the lease it was refused for. + */ +function assertRejectedWith(outcome: Settled, reason: string, leaseId?: string): void { + assert.equal(outcome.status, 'rejected', `expected ${reason}, got ${describe(outcome)}`); + const error = (outcome as Readonly<{ error: unknown }>).error; + assert.ok(error instanceof AppError, `expected an AppError, got ${String(error)}`); + assert.equal(error.details?.reason, reason); + if (leaseId !== undefined) assert.equal(error.details?.leaseId, leaseId); +} + +function describe(outcome: Settled): string { + return outcome.status === 'rejected' + ? `rejection: ${String(outcome.error)}` + : `resolution: ${JSON.stringify(outcome.value)}`; +} + +function makeRuntime(overrides: Partial = {}) { + return createCloudWebDriverRuntime({ + clientVersion: 'test', + provider: 'webdriver-test', + endpoint: 'https://webdriver.test/wd/hub/', + platform: 'android', + deviceName: 'Test device', + profileFields: consumeAllProfileFields('webdriver-test'), + requestPolicy: { retryAttempts: 0 }, + ...overrides, + }); +} + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }); +} + +function makeLease(): DeviceLease { + return { + leaseId: 'lease-1', + tenantId: 'team-a', + runId: 'run-a', + clientId: 'client-a', + leaseProvider: 'webdriver-test', + backend: 'android-instance', + deviceKey: 'webdriver-test:device-a', + createdAt: 1, + expiresAt: 2, + heartbeatAt: 1, + }; +} diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index 4425f761b6..dca9497a3f 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -44,15 +44,36 @@ type CloudWebDriverCloseResult = Readonly<{ }>; type LeaseResult = Record | undefined; +/** + * What one lease has in this manager: the create the hub has not answered, or the session that create + * produced. This is the only place a billed session is recorded, so every route that can end one — + * release, shutdown, and the allocation still waiting for the create — reads it here. + * + * Whoever holds an entry may dispose its session, and only one caller can hold one: an entry leaves + * the map in a single synchronous step. + */ +type WebDriverLeaseEntry = WebDriverAllocatingEntry | WebDriverReadyEntry; +type WebDriverAllocatingEntry = Readonly<{ + status: 'allocating'; + /** Settles with the session the hub rented, or rejects when the create failed and owed nothing. */ + creation: Promise; +}>; +type WebDriverReadyEntry = Readonly<{ + status: 'ready'; + session: WebDriverProviderSession; +}>; + /** * Owns WebDriver session lifecycle and stale-owner retention. Deployment decisions stay in the * sibling runtime-deployment module so neither concern widens the public runtime façade. */ export class WebDriverSessionManager { - private readonly sessionsByLeaseId = new Map(); + private readonly entriesByLeaseId = new Map(); private readonly ownedDeviceIds = new Set(); private readonly releasedProviderSessionIdsByLeaseId = new Map(); private readonly options: CloudWebDriverRuntimeOptions; + /** Whether this manager has shut down; it can then hold nothing, so it must create nothing. */ + private shutDown = false; constructor(options: CloudWebDriverRuntimeOptions) { this.options = options; @@ -63,11 +84,11 @@ export class WebDriverSessionManager { } findSessionForDevice(device: DeviceInfo): WebDriverProviderSession | undefined { - return [...this.sessionsByLeaseId.values()].find((session) => session.device.id === device.id); + return this.readySessions().find((session) => session.device.id === device.id); } findSessionForLease(leaseId: string): WebDriverProviderSession | undefined { - return this.sessionsByLeaseId.get(leaseId); + return this.readySession(this.entriesByLeaseId.get(leaseId)); } getInteractor(device: DeviceInfo): Interactor | undefined { @@ -76,68 +97,51 @@ export class WebDriverSessionManager { heartbeat(lease: DeviceLease): Record | undefined { if (lease.leaseProvider !== this.options.provider) return undefined; - if (!this.sessionsByLeaseId.has(lease.leaseId)) return undefined; + if (!this.readySession(this.entriesByLeaseId.get(lease.leaseId))) return undefined; return { provider: this.options.provider }; } async allocate(lease: DeviceLease, req?: LeaseLifecycleContext): Promise { if (lease.leaseProvider !== this.options.provider) return undefined; - // Before the reuse below: a repeat allocation of a live lease carries flags of its own. - // Loaded here rather than eagerly so the package entry's closure stays unchanged. - const { rejectRefusedProviderProfileFields } = - await import('@agent-device/contracts/provider-profile-fields'); - rejectRefusedProviderProfileFields(req?.flags, this.options.profileFields); - if (this.sessionsByLeaseId.has(lease.leaseId)) return this.heartbeat(lease); - const prepared = await this.prepareSession(lease, req); - const client = new WebDriverClient({ - clientVersion: this.options.clientVersion, - endpoint: prepared.endpoint, - auth: prepared.auth, - headers: prepared.headers, - requestPolicy: this.options.requestPolicy, - }); - const session = await this.createSessionWithPreparedCleanup(client, prepared, lease, req); - const device = this.deviceForLease(lease, prepared); - const providerSessionId = prepared.providerSessionId ?? session.sessionId; - const capabilities = createCloudWebDriverCapabilities({ - provider: this.options.provider, - platform: prepared.platform, - overrides: this.options.capabilityOverrides, - }); - this.sessionsByLeaseId.set( - lease.leaseId, - Object.freeze({ - lease, - device, - client, - prepared, - capabilities, - webDriverSessionId: session.sessionId, - providerSessionId, - interactor: createWebDriverInteractor({ - client, - backend: snapshotBackendForPlatform(prepared.platform), - capabilities, - targetId: device.id, - }), - }), - ); - this.ownedDeviceIds.add(device.id); - return { - provider: this.options.provider, - deviceId: device.id, - sessionId: session.sessionId, - providerSessionId, - capabilities, - ...prepared.providerData, - }; + // The lease's entry is published here, before the first `await`: a second allocation of this lease + // joins the create rather than renting a second billed session, and a release or shutdown that + // arrives while the create runs finds the session it is renting. + const existing = this.entriesByLeaseId.get(lease.leaseId); + if (existing === undefined && this.shutDown) { + throw allocationRefusedAfterShutdown(this.options.provider, lease.leaseId); + } + const entry = existing ?? this.beginAllocation(lease, req); + if (entry.status === 'ready') { + // A repeat allocation of a live lease carries flags of its own, and is refused for the ones no + // profile field of this provider reads. + await this.rejectRefusedProfileFields(req); + return this.heartbeat(lease); + } + const session = await entry.creation; + // The caller that started the create is refused for its own flags inside it, before anything is + // billed; a caller that joined one is refused here, for the flags it carried with it. + await this.rejectRefusedProfileFields(req); + // The lease is still this call's only while the map holds the session it rented. Otherwise release + // or shutdown took the entry while the create was running and disposed what it rented, so this + // caller is refused rather than handed a session id nothing will release. + if (this.readySession(this.entriesByLeaseId.get(lease.leaseId)) !== session) { + if (this.shutDown) { + throw allocationRefusedAfterShutdown(this.options.provider, lease.leaseId, true); + } + throw allocationRefusedByRelease(this.options.provider, lease); + } + return this.allocationResult(session); } async release(lease: DeviceLease): Promise { if (lease.leaseProvider !== this.options.provider) return undefined; - const session = this.sessionsByLeaseId.get(lease.leaseId); + // Taken synchronously, so a second release of the same lease cannot also dispose its session. + const entry = this.entriesByLeaseId.get(lease.leaseId); + if (!entry) return undefined; + this.entriesByLeaseId.delete(lease.leaseId); + const session = await this.disposableSession(entry); + // A create that failed billed nothing and has already given back what its preparation rented. if (!session) return undefined; - this.sessionsByLeaseId.delete(lease.leaseId); this.releasedProviderSessionIdsByLeaseId.set(lease.leaseId, session.providerSessionId); const close = await this.closeSession(session); const artifacts = await this.safeListArtifacts(session); @@ -152,7 +156,7 @@ export class WebDriverSessionManager { async listCloudArtifacts(query: CloudArtifactsQuery): Promise { if (query.provider !== this.options.provider) return undefined; - const session = this.sessionsByLeaseId.get(query.leaseId ?? ''); + const session = this.findSessionForLease(query.leaseId ?? ''); if (session) return await this.listActiveCloudArtifacts(session, query.providerSessionId); return await this.listReleasedCloudArtifacts(query); } @@ -177,14 +181,139 @@ export class WebDriverSessionManager { }); } + /** + * Ends every session this manager holds or is on its way to holding. Each entry is taken out of the + * map before anything is awaited, so a create that answers afterwards is disposed here and by no + * one else. A create that fails owes nothing: it has already given back what its preparation rented. + */ async shutdown(): Promise { + this.shutDown = true; + // Snapshot and clear in one synchronous step: `allocate` refuses once the flag is set and nothing + // between that flag and this clear awaits, so no entry can be added while these are disposed. + const entries = [...this.entriesByLeaseId.values()]; + this.entriesByLeaseId.clear(); await Promise.allSettled( - [...this.sessionsByLeaseId.values()].map(async (session) => await this.closeSession(session)), + entries.map(async (entry) => { + const session = await this.disposableSession(entry); + if (session) await this.closeSession(session); + }), ); - this.sessionsByLeaseId.clear(); this.ownedDeviceIds.clear(); } + /** + * Publishes the entry one lease's create is standing on before that create can answer, so a + * duplicate allocation joins it and a release or shutdown that arrives mid-create has somewhere to + * find the session the create will rent. The entry becomes the lease's ready session when the hub + * answers, and is withdrawn when the create failed and billed nothing. + */ + private beginAllocation( + lease: DeviceLease, + req: LeaseLifecycleContext | undefined, + ): WebDriverAllocatingEntry { + let rentSession!: (session: WebDriverProviderSession) => void; + let refuseSession!: (error: unknown) => void; + const creation = new Promise((rent, refuse) => { + rentSession = rent; + refuseSession = refuse; + }); + const entry: WebDriverAllocatingEntry = Object.freeze({ status: 'allocating', creation }); + this.entriesByLeaseId.set(lease.leaseId, entry); + // Started after the publication above, so the create's answer can never arrive without an entry + // beside it. Whichever route took the entry in the meantime disposes the session this answers. + void this.createSessionForLease(lease, req).then( + (session) => { + if (this.entriesByLeaseId.get(lease.leaseId) === entry) { + this.entriesByLeaseId.set(lease.leaseId, { status: 'ready', session }); + this.ownedDeviceIds.add(session.device.id); + } + rentSession(session); + }, + (error: unknown) => { + if (this.entriesByLeaseId.get(lease.leaseId) === entry) { + this.entriesByLeaseId.delete(lease.leaseId); + } + refuseSession(error); + }, + ); + return entry; + } + + /** + * Prepares the lease, rents the hub session, and hands back the session this manager can dispose. + * Flags no profile field of this provider reads are refused here, before the hub is asked for + * anything: an allocation that cannot be honoured must not bill a device to find out. + */ + private async createSessionForLease( + lease: DeviceLease, + req: LeaseLifecycleContext | undefined, + ): Promise { + await this.rejectRefusedProfileFields(req); + const prepared = await this.prepareSession(lease, req); + const client = new WebDriverClient({ + clientVersion: this.options.clientVersion, + endpoint: prepared.endpoint, + auth: prepared.auth, + headers: prepared.headers, + requestPolicy: this.options.requestPolicy, + }); + const session = await this.createSessionWithPreparedCleanup(client, prepared, lease, req); + const device = this.deviceForLease(lease, prepared); + const capabilities = createCloudWebDriverCapabilities({ + provider: this.options.provider, + platform: prepared.platform, + overrides: this.options.capabilityOverrides, + }); + return Object.freeze({ + lease, + device, + client, + prepared, + capabilities, + webDriverSessionId: session.sessionId, + providerSessionId: prepared.providerSessionId ?? session.sessionId, + interactor: createWebDriverInteractor({ + client, + backend: snapshotBackendForPlatform(prepared.platform), + capabilities, + targetId: device.id, + }), + }); + } + + /** + * The session an entry stands for, once its create has answered, or `undefined` when it stands for + * none because the create billed nothing. The create's failure is the answer of the allocation that + * started it, so a release or shutdown that gets here first takes no error from it. + */ + private async disposableSession( + entry: WebDriverLeaseEntry, + ): Promise { + if (entry.status === 'ready') return entry.session; + return await entry.creation.catch(() => undefined); + } + + private readySession(entry: WebDriverLeaseEntry | undefined) { + return entry?.status === 'ready' ? entry.session : undefined; + } + + private readySessions(): WebDriverProviderSession[] { + return [...this.entriesByLeaseId.values()].flatMap((entry) => + entry.status === 'ready' ? [entry.session] : [], + ); + } + + private allocationResult(session: WebDriverProviderSession): Record { + return { + provider: this.options.provider, + deviceId: session.device.id, + sessionId: session.webDriverSessionId, + providerSessionId: session.providerSessionId, + capabilities: session.capabilities, + ...session.prepared.providerData, + }; + } + private async createSessionWithPreparedCleanup( client: WebDriverClient, prepared: CloudWebDriverPreparedSession, @@ -204,6 +333,17 @@ export class WebDriverSessionManager { } } + /** + * Refuses the flags this provider reads no profile field for, so a field that would be ignored is + * reported instead of dropped. The module is loaded on first use to keep the package entry's own + * closure unchanged. + */ + private async rejectRefusedProfileFields(req: LeaseLifecycleContext | undefined): Promise { + const { rejectRefusedProviderProfileFields } = + await import('@agent-device/contracts/provider-profile-fields'); + rejectRefusedProviderProfileFields(req?.flags, this.options.profileFields); + } + private async prepareSession( lease: DeviceLease, req: LeaseLifecycleContext | undefined, @@ -321,3 +461,46 @@ function sessionCreateTimeoutError( timeout, ); } + +/** + * A release took the lease's entry while its create was still running and disposed the session that + * create rented. The caller is refused so it cannot drive a session agent-device has already ended. + */ +function allocationRefusedByRelease(provider: string, lease: DeviceLease): AppError { + return new AppError( + 'COMMAND_FAILED', + `${provider} released lease ${lease.leaseId} while it was creating the WebDriver session.`, + { + reason: 'provider_session_released_during_allocation', + provider, + leaseId: lease.leaseId, + runId: lease.runId, + hint: 'Allocate the lease again; agent-device already stopped the session this allocation created.', + }, + ); +} + +/** + * Either this manager was already shut down when the allocation arrived, or it shut down while the + * create was running and disposed what it rented. Either way no session survives, so nothing here has + * to be stopped by hand. + */ +function allocationRefusedAfterShutdown( + provider: string, + leaseId: string, + disposedSession = false, +): AppError { + return new AppError( + 'COMMAND_FAILED', + `${provider} is shut down and created no WebDriver session for lease ${leaseId}.`, + { + reason: 'provider_session_shutdown_during_allocation', + provider, + leaseId, + disposedSession, + hint: disposedSession + ? 'Nothing to clean up: agent-device stopped the session it was creating.' + : 'Start the daemon again to allocate a provider session.', + }, + ); +} From 8b2bdb05f5cd37d18d91932c27027784fbe2f65b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 09:29:07 +0100 Subject: [PATCH 2/5] fix(provider-webdriver): keep a releasing lease owned until disposal ends A release took a lease out of the manager's map and then waited on the hub, so a fresh allocation in that window saw a free lease and rented a second billed session the release never saw, and a shutdown that arrived there answered with the first session still live. A repeated allocation validated its flags after reading the live session, so a teardown landing inside that await left the caller told the lease was held by a session that no longer existed. A shutdown whose DELETE was refused reported a clean close, and the allocation refused for that session was told nothing needed cleaning up. A lease entry now keeps owning its session while it is being ended: the take and the one disposal it starts are recorded in the same synchronous step, the entry leaves the map only once that disposal settles, and a second release or a shutdown joins it. An allocation reports the disposal it waited on, naming the session and whether its end is confirmed, pending, or failed; a shutdown that could not stop a session now fails the way a plugin cleanup failure is reported at close. Every allocation is refused for its own refused profile flags before it joins or publishes anything. --- .../src/profile-fields.fixtures.ts | 12 + .../src/runtime-session-allocation.test.ts | 477 +++++++++++++++--- .../provider-webdriver/src/runtime-session.ts | 459 ++++++++++++----- 3 files changed, 733 insertions(+), 215 deletions(-) diff --git a/packages/provider-webdriver/src/profile-fields.fixtures.ts b/packages/provider-webdriver/src/profile-fields.fixtures.ts index 2acc0b6bed..bd53b63379 100644 --- a/packages/provider-webdriver/src/profile-fields.fixtures.ts +++ b/packages/provider-webdriver/src/profile-fields.fixtures.ts @@ -40,3 +40,15 @@ export function consumedProfileFields({ fields }: ProviderProfileFieldDeclaratio .filter((field) => fields[field] === 'consumed') .sort(); } + +/** A hub that reads none of the profile fields, so every one of them is refused. */ +export function refuseAllProfileFields(provider: string): ProviderProfileFieldDeclaration { + const consumed = consumeAllProfileFields(provider); + return { + provider, + label: consumed.label, + fields: Object.fromEntries( + (Object.keys(consumed.fields) as ProviderProfileField[]).map((field) => [field, 'refused']), + ) as Record, + }; +} diff --git a/packages/provider-webdriver/src/runtime-session-allocation.test.ts b/packages/provider-webdriver/src/runtime-session-allocation.test.ts index b31d188f72..419cef3500 100644 --- a/packages/provider-webdriver/src/runtime-session-allocation.test.ts +++ b/packages/provider-webdriver/src/runtime-session-allocation.test.ts @@ -3,7 +3,7 @@ import { afterEach, test } from 'vitest'; import type { DeviceLease } from '@agent-device/contracts/device'; import { AppError } from '@agent-device/kernel/errors'; import { createCloudWebDriverRuntime, type CloudWebDriverRuntimeOptions } from './runtime.ts'; -import { consumeAllProfileFields } from './profile-fields.fixtures.ts'; +import { consumeAllProfileFields, refuseAllProfileFields } from './profile-fields.fixtures.ts'; const realFetch = globalThis.fetch; @@ -13,35 +13,74 @@ afterEach(() => { /** * `POST /session` is billed and the lease is the only handle that can stop it again. Two allocations - * that overlapped both passed the live-session check, so the second hub session overwrote the first - * in the manager's map and only one of the two was ever releasable. + * that overlapped both passed the live-session check, so the second hub session overwrote the first in + * the manager's map and only one of the two was ever releasable. */ test('two concurrent allocations of one lease create one billed session', async () => { const hub = startWebDriverHub(); - globalThis.fetch = hub.fetch; - const provider = holdProviderPreparation(); - const runtime = makeRuntime({ prepareSession: provider.prepareSession }); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ prepareSession: preparation.prepareSession }); const lease = makeLease(); try { + globalThis.fetch = hub.fetch; const first = settle(() => runtime.leaseLifecycle.allocate!(lease)); - await provider.waitForPreparation(); + await preparation.waitFor(1); const second = settle(() => runtime.leaseLifecycle.allocate!(lease)); // One scheduler boundary while the first allocation is still held before the hub: the second has // then reached the lease, and the first cannot have published its session yet. await schedulerYield(); - provider.answerPreparation(); - const [firstOutcome, secondOutcome] = await Promise.all([first, second]); - assert.equal(hub.createdSessionIds.length, 1, 'one lease bills one session'); + const [firstOutcome, secondOutcome] = await Promise.all([ + first, + second, + preparation.answerHeld(), + ]); + assert.equal(hub.created.length, 1, 'one lease bills one session'); assertResolvedWith(firstOutcome, 'session-1'); assertResolvedWith(secondOutcome, 'session-1'); await runtime.leaseLifecycle.release!(lease); - assert.deepEqual(hub.deletedSessionIds, hub.createdSessionIds); - assert.equal(provider.cleanups, 1); + assert.deepEqual(hub.deleted, hub.created); + assert.equal(preparation.cleanups, 1); + } finally { + await preparation.answerHeld(); + await runtime.shutdown(); + } +}); + +/** + * An allocation that cannot be honoured is refused for its own flags, and one that joins a create in + * flight is refused for those flags without taking the create's caller down with it: the flags are the + * caller's, and the session belongs to whoever the lease was first allocated for. + */ +test('an allocation refused for its own flags leaves the create it joined alone', async () => { + const hub = startWebDriverHub(); + const preparation = startProviderPreparation(); + // This runtime reads none of the Cloud profile fields, so the flags below are refused. + const runtime = makeRuntime({ + prepareSession: preparation.prepareSession, + profileFields: refuseAllProfileFields('webdriver-test'), + }); + const lease = makeLease(); + // The flags bag is keyed by profile field, and `providerApp` is one this runtime refuses. + const refusedFlags = { providerApp: 'binary-123' }; + + try { + globalThis.fetch = hub.fetch; + const valid = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await preparation.waitFor(1); + const refused = settle(() => + runtime.leaseLifecycle.allocate!(lease, { flags: refusedFlags } as never), + ); + await schedulerYield(); + await preparation.answerHeld(); + + assertRejectedWith(await refused, 'INVALID_ARGS', { flags: ['--provider-app'] }); + assertResolvedWith(await valid, 'session-1'); + assert.equal(hub.created.length, 1, 'a refused allocation bills nothing'); } finally { - provider.answerPreparation(); + await preparation.answerHeld(); await runtime.shutdown(); } }); @@ -53,31 +92,31 @@ test('two concurrent allocations of one lease create one billed session', async */ test('a release during creation disposes the session that creation makes', async () => { const hub = startWebDriverHub(); - globalThis.fetch = hub.fetch; - const provider = holdProviderPreparation(); - const runtime = makeRuntime({ prepareSession: provider.prepareSession }); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ prepareSession: preparation.prepareSession }); const lease = makeLease(); try { + globalThis.fetch = hub.fetch; const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); - await provider.waitForPreparation(); - // The release takes the lease's entry in its first synchronous step, while the create is held. + await preparation.waitFor(1); + // The release takes the lease in its first synchronous step, while the create is held. const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); - provider.answerPreparation(); + await preparation.answerHeld(); assert.deepEqual(await releasing, { status: 'resolved', value: { provider: 'webdriver-test', providerSessionId: 'session-1' }, }); - assert.deepEqual(hub.deletedSessionIds, ['session-1']); - assert.equal(provider.cleanups, 1); - assertRejectedWith( - await allocating, - 'provider_session_released_during_allocation', - lease.leaseId, - ); + assert.deepEqual(hub.deleted, ['session-1']); + assert.equal(preparation.cleanups, 1); + assertRejectedWith(await allocating, 'provider_session_released_during_allocation', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + cleanup: 'confirmed', + }); } finally { - provider.answerPreparation(); + await preparation.answerHeld(); await runtime.shutdown(); } }); @@ -85,72 +124,307 @@ test('a release during creation disposes the session that creation makes', async /** The same race on the route that ends every session this process holds. */ test('shutdown during creation disposes the session that creation makes', async () => { const hub = startWebDriverHub(); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ prepareSession: preparation.prepareSession }); + const lease = makeLease(); + + try { + globalThis.fetch = hub.fetch; + const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await preparation.waitFor(1); + // Shutdown takes every lease in its first synchronous step, while the create is held. + const shuttingDown = settle(() => runtime.shutdown()); + await preparation.answerHeld(); + + assert.deepEqual(await shuttingDown, { status: 'resolved', value: undefined }); + assert.deepEqual(hub.deleted, ['session-1']); + assert.equal(preparation.cleanups, 1); + assertRejectedWith(await allocating, 'provider_session_shutdown_during_allocation', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + cleanup: 'confirmed', + }); + } finally { + await preparation.answerHeld(); + await runtime.shutdown(); + } +}); + +/** + * Taking a lease out of the map to dispose it opened the lease up again: an allocation that arrived + * while that disposal still waited on the hub saw nothing holding the lease and rented a second session + * the release could never see, and a shutdown that arrived in the same window answered with the first + * session live. Until the disposal settles, the lease belongs to the route that is ending it. + */ +test('an allocation that arrives during a release still disposing creates no second session', async () => { + // The hub keeps the release's DELETE pending, which is the window this test needs. + const hub = startWebDriverHub({ heldDeletes: 1 }); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ prepareSession: preparation.prepareSession }); + const lease = makeLease(); + + try { + globalThis.fetch = hub.fetch; + const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await preparation.waitFor(1); + await preparation.answerHeld(); + assertResolvedWith(await allocating, 'session-1'); + + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + const racing = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await schedulerYield(); + assert.equal( + hub.created.length, + 1, + 'an allocation that arrives mid-release must not bill a second session', + ); + assert.equal(preparation.started, 1, 'an allocation refused mid-release prepares nothing'); + + const shuttingDown = settle(() => runtime.shutdown()); + await schedulerYield(); + // The release is still waiting on the hub, so neither it nor the shutdown may have answered. + assert.deepEqual(hub.deleted, []); + assert.equal(await alreadySettled(shuttingDown), false, 'shutdown must await the release'); + + hub.answerHeldDeletes(); + assertRejectedWith(await racing, 'provider_session_release_in_progress', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + }); + // Shutdown joined the disposal that was already running, so no session of this provider outlives it. + assert.deepEqual(await shuttingDown, { status: 'resolved', value: undefined }); + assert.deepEqual(hub.deleted, ['session-1']); + assert.equal(preparation.cleanups, 1); + assert.deepEqual(await releasing, { + status: 'resolved', + value: { provider: 'webdriver-test', providerSessionId: 'session-1' }, + }); + } finally { + hub.answerHeldDeletes(); + await preparation.answerHeld(); + await runtime.shutdown(); + } +}); + +/** + * A repeated allocation used to answer for the live session and then await, which gave a release room to + * end that session before the answer reached the caller, who was told the lease was alive. Its flags are + * validated first and the answer that follows the lookup carries no await, so a release that gets there + * first is the outcome the caller gets instead. + */ +test('a repeated allocation cannot answer that a released lease is alive', async () => { + // The hub holds the release's DELETE, so the release is certainly still ending the session at the + // moment the repeat allocation answers. + const hub = startWebDriverHub({ heldDeletes: 1 }); globalThis.fetch = hub.fetch; - const provider = holdProviderPreparation(); - const runtime = makeRuntime({ prepareSession: provider.prepareSession }); + const runtime = makeRuntime(); + const lease = makeLease(); try { - const allocating = settle(() => runtime.leaseLifecycle.allocate!(makeLease())); - await provider.waitForPreparation(); - // Shutdown takes every entry in its first synchronous step, while the create is held. + await runtime.leaseLifecycle.allocate!(lease); + assert.deepEqual(hub.created, ['session-1']); + // The healthy case first: a repeat allocation of a live lease reports that lease as held. + assert.deepEqual(await runtime.leaseLifecycle.allocate!(lease), { + provider: 'webdriver-test', + }); + + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + const repeated = settle(() => runtime.leaseLifecycle.allocate!(lease)); + assertRejectedWith(await repeated, 'provider_session_release_in_progress', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + cleanup: 'pending', + }); + assert.equal(hub.created.length, 1, 'a refused repeat allocation bills nothing'); + + hub.answerHeldDeletes(); + assert.deepEqual(await releasing, { + status: 'resolved', + value: { provider: 'webdriver-test', providerSessionId: 'session-1' }, + }); + assert.deepEqual(hub.deleted, ['session-1']); + } finally { + hub.answerHeldDeletes(); + await runtime.shutdown(); + } +}); + +/** + * The same window on the route that ends every session: a shutdown that lands first is the outcome a + * repeat allocation reports, rather than a success with no session behind it. + */ +test('a repeated allocation cannot answer that a shut down lease is alive', async () => { + const hub = startWebDriverHub({ heldDeletes: 1 }); + globalThis.fetch = hub.fetch; + const runtime = makeRuntime(); + const lease = makeLease(); + + try { + await runtime.leaseLifecycle.allocate!(lease); + assert.deepEqual(hub.created, ['session-1']); + const shuttingDown = settle(() => runtime.shutdown()); - provider.answerPreparation(); + const repeated = settle(() => runtime.leaseLifecycle.allocate!(lease)); + assertRejectedWith(await repeated, 'provider_session_shutdown_during_allocation', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + cleanup: 'pending', + }); + assert.equal(hub.created.length, 1, 'a refused repeat allocation bills nothing'); + hub.answerHeldDeletes(); assert.deepEqual(await shuttingDown, { status: 'resolved', value: undefined }); - assert.deepEqual(hub.deletedSessionIds, ['session-1']); - assert.equal(provider.cleanups, 1); - assertRejectedWith(await allocating, 'provider_session_shutdown_during_allocation'); + assert.deepEqual(hub.deleted, ['session-1']); + } finally { + hub.answerHeldDeletes(); + await runtime.shutdown(); + } +}); + +/** + * A hub that will not end a session leaves it billing, so the release that was told to end it names the + * session and the provider's refusal, exactly as it does for a session it was handed ready-made. + */ +test('a release the provider refused reports the session left running', async () => { + const hub = startWebDriverHub({ deleteFailure: 'provider rejected the delete' }); + globalThis.fetch = hub.fetch; + const runtime = makeRuntime(); + const lease = makeLease(); + + try { + await runtime.leaseLifecycle.allocate!(lease); + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + + const result = await releasing; + assert.equal(result.status, 'resolved', describe(result)); + const value = (result as Readonly<{ value: Record | undefined }>).value; + assert.equal(value?.providerSessionId, 'session-1'); + assert.deepEqual(warningCodes(value?.warnings), ['WEBDRIVER_SESSION_DELETE_FAILED']); + assert.match(String(warningMessages(value?.warnings)[0]), /provider rejected the delete/); + assert.deepEqual(hub.deleted, []); } finally { - provider.answerPreparation(); + hub.allowDeletes(); + await runtime.shutdown(); + } +}); + +/** + * A shutdown that could not stop a session may not report a clean close, and the allocation whose + * session it was is told the outcome is uncertain rather than that nothing needs cleaning up. + */ +test('a session shutdown could not stop fails the shutdown and the refused allocation', async () => { + const hub = startWebDriverHub({ deleteFailure: 'provider rejected the delete' }); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ prepareSession: preparation.prepareSession }); + const lease = makeLease(); + + try { + globalThis.fetch = hub.fetch; + const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); + await preparation.waitFor(1); + const shuttingDown = settle(() => runtime.shutdown()); + await preparation.answerHeld(); + + assertRejectedWith(await allocating, 'provider_session_shutdown_during_allocation', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + cleanup: 'uncertain', + cleanupWarning: 'WEBDRIVER_SESSION_DELETE_FAILED', + }); + // A daemon close reports a plugin that left work behind, so the session and the provider's refusal + // have to travel on the error rather than be dropped. + assertRejectedWith(await shuttingDown, 'provider_session_cleanup_incomplete', { + providerSessionIds: ['session-1'], + cleanup: 'uncertain', + cleanupWarning: 'WEBDRIVER_SESSION_DELETE_FAILED', + }); + // The create-retry policy stays: nothing here deletes or creates a second session on its own. + assert.deepEqual(hub.created, ['session-1']); + assert.deepEqual(hub.deleted, []); + assert.equal(preparation.cleanups, 1); + } finally { + hub.allowDeletes(); + await preparation.answerHeld(); await runtime.shutdown(); } }); /** * The hub answers every WebDriver request and records the session ids it was asked to create and - * delete, so a test's lever over an in-flight create is {@link holdProviderPreparation}. + * delete, so a test's levers over an in-flight create and an in-flight disposal are + * {@link startProviderPreparation} and the `heldDeletes` option. */ -function startWebDriverHub() { - const createdSessionIds: string[] = []; - const deletedSessionIds: string[] = []; +function startWebDriverHub( + options: Readonly<{ deleteFailure?: string; heldDeletes?: number }> = {}, +) { + const created: string[] = []; + const deleted: string[] = []; + let deletesToHold = options.heldDeletes ?? 0; + let deleteFailure = options.deleteFailure; + const held: (() => void)[] = []; return { - createdSessionIds, - deletedSessionIds, + created, + deleted, + allowDeletes: () => { + deleteFailure = undefined; + }, + /** Lets every held `DELETE` finish, which is how a release that is still disposing resumes. */ + answerHeldDeletes: () => { + for (const ready of held.splice(0)) ready(); + }, fetch: (async (input: unknown, init?: RequestInit) => { const request = new Request(input as RequestInfo, init); const path = new URL(request.url).pathname; if (request.method === 'POST' && path.endsWith('/session')) { - const sessionId = `session-${String(createdSessionIds.length + 1)}`; - createdSessionIds.push(sessionId); + const sessionId = `session-${String(created.length + 1)}`; + created.push(sessionId); return jsonResponse({ sessionId, capabilities: {} }); } if (request.method === 'DELETE') { - deletedSessionIds.push(decodeURIComponent(path.split('/').pop()!)); + const sessionId = decodeURIComponent(path.split('/').pop()!); + if (deletesToHold > 0) { + deletesToHold -= 1; + await new Promise((ready) => { + held.push(ready); + }); + } + if (deleteFailure) { + return jsonResponse( + { value: { error: 'unknown error', message: `${deleteFailure}: ${sessionId}` } }, + 500, + ); + } + deleted.push(sessionId); } return jsonResponse({ value: null }); }) as typeof fetch, }; } +type PrepareSession = NonNullable; + /** - * Holds `prepareSession` — the step that runs before this manager sends `POST /session` — until the - * test answers it, so a create stays in flight across a release or a shutdown instead of the test - * racing one with a sleep. It also counts the provider cleanup a prepared session owes, which is the - * half of disposal the hub's `DELETE` does not cover. + * `prepareSession` is the step that runs before this manager sends `POST /session`, so holding it per + * call keeps a create in flight across a release or a shutdown without racing one with a sleep. It + * counts the creates that reached the provider — the difference between one billed session and two — + * and the provider cleanup a prepared session owes, which is the half of disposal the hub's `DELETE` + * does not cover. */ -function holdProviderPreparation() { - const state = { prepared: 0, cleanups: 0 }; - const arrivals: (() => void)[] = []; - let settleGate: () => void = () => {}; - const gate = new Promise((settle) => { - settleGate = settle; - }); - const prepareSession: NonNullable = async ({ - base, - }) => { - state.prepared += 1; - for (const arrived of arrivals.splice(0)) arrived(); - await gate; +function startProviderPreparation() { + const state = { started: 0, cleanups: 0 }; + const calls: { readonly released: boolean; readonly release: () => void }[] = []; + const prepareSession: PrepareSession = async ({ base }) => { + state.started += 1; + const call = { released: false, release: () => {} }; + const held = new Promise((ready) => { + call.release = () => { + if (call.released) return; + call.released = true; + ready(); + }; + }); + calls.push(call); + await held; return { ...base, cleanup: async () => { @@ -161,16 +435,20 @@ function holdProviderPreparation() { }; return Object.assign(state, { prepareSession, - /** Resolves once an allocation has reached `prepareSession` and is held there. */ - waitForPreparation: async () => { - while (state.prepared < 1) { - await new Promise((arrived) => { - arrivals.push(arrived); - }); + /** Resolves once `expected` allocations have reached the provider and are held there. */ + waitFor: async (expected: number) => { + for (let attempt = 0; calls.length < expected && attempt < 200; attempt += 1) { + await schedulerYield(); } + assert.ok( + calls.length >= expected, + `expected ${String(expected)} allocations to reach the provider, got ${String(calls.length)}`, + ); }, - answerPreparation: () => { - settleGate(); + /** Lets every create that is held reach the hub. */ + answerHeld: async () => { + for (const call of calls) call.release(); + await schedulerYield(); }, }); } @@ -189,6 +467,16 @@ async function settle(run: () => Promise): Promise> { } } +/** Whether a call has answered, so a test can name work that must still be pending. */ +async function alreadySettled(outcome: Promise>): Promise { + return await Promise.race([ + outcome.then(() => true), + new Promise((pending) => { + setTimeout(() => pending(false), 0); + }), + ]); +} + /** Yields one macrotask, so every promise the test has started runs the microtasks it already queued. */ async function schedulerYield(): Promise { await new Promise((resolve) => { @@ -209,15 +497,52 @@ function assertResolvedWith( } /** - * The typed reason an allocation reports when the session it made had already been taken to be - * disposed, and the lease it was refused for. + * What a refused call reports: its typed reason or error code, the lease and session it was refused + * for, and what the teardown that took the session came to. */ -function assertRejectedWith(outcome: Settled, reason: string, leaseId?: string): void { +function assertRejectedWith( + outcome: Settled, + reason: string, + expected: Readonly<{ + leaseId?: string; + providerSessionId?: string; + providerSessionIds?: string[]; + cleanup?: string; + cleanupWarning?: string; + flags?: string[]; + }> = {}, +): void { assert.equal(outcome.status, 'rejected', `expected ${reason}, got ${describe(outcome)}`); const error = (outcome as Readonly<{ error: unknown }>).error; assert.ok(error instanceof AppError, `expected an AppError, got ${String(error)}`); - assert.equal(error.details?.reason, reason); - if (leaseId !== undefined) assert.equal(error.details?.leaseId, leaseId); + const details = error.details as Record; + assert.equal(details.reason ?? error.code, reason); + if (expected.leaseId !== undefined) assert.equal(details.leaseId, expected.leaseId); + if (expected.providerSessionId !== undefined) { + assert.equal(details.providerSessionId, expected.providerSessionId); + } + if (expected.providerSessionIds !== undefined) { + assert.deepEqual(details.providerSessionIds, expected.providerSessionIds); + } + if (expected.cleanup !== undefined) assert.equal(details.cleanup, expected.cleanup); + if (expected.cleanupWarning !== undefined) { + const warnings = details.cleanupWarnings as Record; + assert.deepEqual(Object.keys(warnings ?? {}), [expected.cleanupWarning]); + assert.equal(typeof warnings[expected.cleanupWarning], 'string'); + } + if (expected.flags !== undefined) assert.deepEqual(details.flags, expected.flags); +} + +function warningCodes(warnings: unknown): string[] { + return ((warnings as Array> | undefined) ?? []).map((warning) => + String(warning.code), + ); +} + +function warningMessages(warnings: unknown): unknown[] { + return ((warnings as Array> | undefined) ?? []).map( + (warning) => warning.message, + ); } function describe(outcome: Settled): string { diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index dca9497a3f..8ef73152e7 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -38,31 +38,66 @@ type CloudWebDriverReleaseWarning = Readonly<{ code: 'WEBDRIVER_SESSION_DELETE_FAILED' | 'PROVIDER_CLEANUP_FAILED'; message: string; }>; -type CloudWebDriverCloseResult = Readonly<{ - cleanup?: Record; - warnings: CloudWebDriverReleaseWarning[]; -}>; type LeaseResult = Record | undefined; /** - * What one lease has in this manager: the create the hub has not answered, or the session that create - * produced. This is the only place a billed session is recorded, so every route that can end one — - * release, shutdown, and the allocation still waiting for the create — reads it here. + * What one lease has in this manager: the create the hub has not answered, the session that create + * rented, or the disposal that is ending it. This is the only place a billed session is recorded, so + * every route that can end one — release, shutdown, and the allocation still waiting for the create — + * reads it here. Entries never leave this module, so their fields stay mutable. * - * Whoever holds an entry may dispose its session, and only one caller can hold one: an entry leaves - * the map in a single synchronous step. + * The entry in the map owns the lease's session, and it keeps owning it while it is being ended: + * `taken` is set in the same synchronous step that starts the disposal, and the entry leaves the map + * only once that disposal settles. So a lease is never between owners while its session may be live — + * an allocation that arrives mid-release is refused, a second release joins the disposal in flight, and + * shutdown awaits it instead of starting another. + * + * An allocation reads its own refusal from the `taken` of the entry it joined, so the reason names the + * teardown that actually happened rather than whatever state the manager is in by the time the create + * answers, and it carries what that disposal came to. */ -type WebDriverLeaseEntry = WebDriverAllocatingEntry | WebDriverReadyEntry; -type WebDriverAllocatingEntry = Readonly<{ - status: 'allocating'; - /** Settles with the session the hub rented, or rejects when the create failed and owed nothing. */ +type WebDriverLeaseEntry = { + lease: DeviceLease; + /** Settles with the session the hub rented, or rejects when the create billed nothing. */ creation: Promise; -}>; -type WebDriverReadyEntry = Readonly<{ - status: 'ready'; - session: WebDriverProviderSession; + /** What the create rented. Missing while the create is still running, and never set when it failed. */ + session: WebDriverProviderSession | undefined; + /** Which route took this lease from its allocation, and the one disposal that owns its session. */ + taken: WebDriverLeaseTake | undefined; +}; + +/** + * Whichever route took a lease away from its allocation, and the single disposal of what that entry + * stood for. `settled` is what that disposal reported and stays missing while it is still running, so a + * caller can tell a session that is gone from one whose outcome it cannot see yet. + */ +type WebDriverLeaseTake = { + by: 'release' | 'shutdown'; + /** The session being ended, known as soon as the create named it. */ + providerSessionId: string | undefined; + disposal: Promise; + settled: WebDriverDisposalOutcome | undefined; +}; + +/** + * What ending one lease's session came to. `cleanup` answers whether a billed session is certainly + * gone: `confirmed` once the hub ended it, `uncertain` when ending it failed, and `none` when the + * create rented nothing. + */ +type WebDriverDisposalOutcome = Readonly<{ + cleanup: 'confirmed' | 'uncertain' | 'none'; + session: WebDriverProviderSession | undefined; + warnings: CloudWebDriverReleaseWarning[]; + /** What the provider's own cleanup reported, for the release envelope. */ + cleanupPayload?: Record; + artifacts?: CloudArtifactsResult; }>; +/** The fields of one warning, keyed as `details.cleanupWarnings` carries them on a refusal. */ +function warningMessagesByCode(warnings: CloudWebDriverReleaseWarning[]): Record { + return Object.fromEntries(warnings.map((warning) => [warning.code, warning.message])); +} + /** * Owns WebDriver session lifecycle and stale-owner retention. Deployment decisions stay in the * sibling runtime-deployment module so neither concern widens the public runtime façade. @@ -84,11 +119,11 @@ export class WebDriverSessionManager { } findSessionForDevice(device: DeviceInfo): WebDriverProviderSession | undefined { - return this.readySessions().find((session) => session.device.id === device.id); + return this.liveSessions().find((session) => session.device.id === device.id); } findSessionForLease(leaseId: string): WebDriverProviderSession | undefined { - return this.readySession(this.entriesByLeaseId.get(leaseId)); + return this.liveSession(this.entriesByLeaseId.get(leaseId)); } getInteractor(device: DeviceInfo): Interactor | undefined { @@ -97,60 +132,53 @@ export class WebDriverSessionManager { heartbeat(lease: DeviceLease): Record | undefined { if (lease.leaseProvider !== this.options.provider) return undefined; - if (!this.readySession(this.entriesByLeaseId.get(lease.leaseId))) return undefined; + if (!this.liveSession(this.entriesByLeaseId.get(lease.leaseId))) return undefined; return { provider: this.options.provider }; } async allocate(lease: DeviceLease, req?: LeaseLifecycleContext): Promise { if (lease.leaseProvider !== this.options.provider) return undefined; - // The lease's entry is published here, before the first `await`: a second allocation of this lease - // joins the create rather than renting a second billed session, and a release or shutdown that - // arrives while the create runs finds the session it is renting. - const existing = this.entriesByLeaseId.get(lease.leaseId); - if (existing === undefined && this.shutDown) { - throw allocationRefusedAfterShutdown(this.options.provider, lease.leaseId); - } - const entry = existing ?? this.beginAllocation(lease, req); - if (entry.status === 'ready') { - // A repeat allocation of a live lease carries flags of its own, and is refused for the ones no - // profile field of this provider reads. - await this.rejectRefusedProfileFields(req); - return this.heartbeat(lease); - } - const session = await entry.creation; - // The caller that started the create is refused for its own flags inside it, before anything is - // billed; a caller that joined one is refused here, for the flags it carried with it. + // Every allocation is refused for the flags this provider reads no profile field for, before it + // joins or publishes anything: a call answers for its own flags and never inherits another call's, + // and one that cannot be honoured bills nothing. await this.rejectRefusedProfileFields(req); - // The lease is still this call's only while the map holds the session it rented. Otherwise release - // or shutdown took the entry while the create was running and disposed what it rented, so this - // caller is refused rather than handed a session id nothing will release. - if (this.readySession(this.entriesByLeaseId.get(lease.leaseId)) !== session) { - if (this.shutDown) { - throw allocationRefusedAfterShutdown(this.options.provider, lease.leaseId, true); - } - throw allocationRefusedByRelease(this.options.provider, lease); + // The lease's entry is published here, in the same synchronous step as the lookup above: a second + // allocation of this lease joins the create rather than renting a second billed session, and a + // release or shutdown that arrives while the create runs finds the session it is renting. + let entry = this.entriesByLeaseId.get(lease.leaseId); + if (entry === undefined) { + if (this.shutDown) throw allocationRefusedAfterShutdown(this.options.provider, lease); + entry = this.beginAllocation(lease, req); } + // A lease whose disposal is running belongs to the route that is ending it, so this call rented + // nothing and is refused for that disposal rather than for a session of its own. + if (entry.taken) throw refusalForTeardownInProgress(this.options.provider, lease, entry.taken); + // Reported without awaiting anything after the lookup, so the session is live at the moment the + // caller is told about it: a release or a shutdown cannot end it inside this call. + if (entry.session) return { provider: this.options.provider }; + const session = await entry.creation; + // The session this call rented is its own only while nobody took the lease. Otherwise a release or + // shutdown ended what the create rented, and this caller is refused rather than handed a session id + // nothing will release. + if (entry.taken) throw await this.refusalForTakenSession(lease, entry.taken); return this.allocationResult(session); } async release(lease: DeviceLease): Promise { if (lease.leaseProvider !== this.options.provider) return undefined; - // Taken synchronously, so a second release of the same lease cannot also dispose its session. const entry = this.entriesByLeaseId.get(lease.leaseId); - if (!entry) return undefined; - this.entriesByLeaseId.delete(lease.leaseId); - const session = await this.disposableSession(entry); - // A create that failed billed nothing and has already given back what its preparation rented. - if (!session) return undefined; - this.releasedProviderSessionIdsByLeaseId.set(lease.leaseId, session.providerSessionId); - const close = await this.closeSession(session); - const artifacts = await this.safeListArtifacts(session); + // Nothing holds the lease: it was never allocated here, or its release finished and gave the lease + // back. A release that arrives while another one disposes joins that disposal. + if (entry === undefined) return undefined; + const outcome = await this.takeForRelease(entry); + // A create that failed billed nothing, and gave back what its preparation had already rented. + if (outcome.session === undefined) return undefined; return { provider: this.options.provider, - providerSessionId: session.providerSessionId, - ...close.cleanup, - ...(close.warnings.length > 0 ? { warnings: close.warnings } : {}), - ...(artifacts ? { cloudArtifacts: artifacts } : {}), + providerSessionId: outcome.session.providerSessionId, + ...outcome.cleanupPayload, + ...(outcome.warnings.length > 0 ? { warnings: outcome.warnings } : {}), + ...(outcome.artifacts ? { cloudArtifacts: outcome.artifacts } : {}), }; } @@ -182,55 +210,54 @@ export class WebDriverSessionManager { } /** - * Ends every session this manager holds or is on its way to holding. Each entry is taken out of the - * map before anything is awaited, so a create that answers afterwards is disposed here and by no - * one else. A create that fails owes nothing: it has already given back what its preparation rented. + * Ends every session this manager holds or is on its way to holding, and says so when it could not. + * Each lease is taken, or joined, in one synchronous step, so a create that answers afterwards is + * disposed by the one disposal of that entry and by no other route, and a release already running is + * awaited rather than raced. */ async shutdown(): Promise { this.shutDown = true; - // Snapshot and clear in one synchronous step: `allocate` refuses once the flag is set and nothing - // between that flag and this clear awaits, so no entry can be added while these are disposed. - const entries = [...this.entriesByLeaseId.values()]; - this.entriesByLeaseId.clear(); - await Promise.allSettled( - entries.map(async (entry) => { - const session = await this.disposableSession(entry); - if (session) await this.closeSession(session); - }), + // Snapshot before anything is awaited: `allocate` publishes an entry only when none exists and + // refuses once this manager is shut down, and nothing between that flag and this loop awaits. + const disposals = [...this.entriesByLeaseId.values()].map( + async (entry) => await this.takeForShutdown(entry), ); + const outcomes = await Promise.all(disposals); this.ownedDeviceIds.clear(); + // A session the provider would not give back is still billing, and this process will not stop it. + // The plugin's close is where an operator learns what a daemon close left running, so the session + // and the provider's reason travel on the error rather than being dropped. + const unended = outcomes.filter((outcome) => outcome.cleanup === 'uncertain'); + if (unended.length > 0) throw shutdownLeftSessionsRunning(this.options.provider, unended); } /** - * Publishes the entry one lease's create is standing on before that create can answer, so a - * duplicate allocation joins it and a release or shutdown that arrives mid-create has somewhere to - * find the session the create will rent. The entry becomes the lease's ready session when the hub - * answers, and is withdrawn when the create failed and billed nothing. + * Publishes the entry one lease's create is standing on before that create can answer, so a duplicate + * allocation joins it and a release or shutdown that arrives mid-create has somewhere to find the + * session the create will rent. The entry records that session when the hub answers, and leaves the + * map when the create failed and billed nothing. */ - private beginAllocation( - lease: DeviceLease, - req: LeaseLifecycleContext | undefined, - ): WebDriverAllocatingEntry { + private beginAllocation(lease: DeviceLease, req: LeaseLifecycleContext | undefined) { let rentSession!: (session: WebDriverProviderSession) => void; let refuseSession!: (error: unknown) => void; const creation = new Promise((rent, refuse) => { rentSession = rent; refuseSession = refuse; }); - const entry: WebDriverAllocatingEntry = Object.freeze({ status: 'allocating', creation }); + const entry: WebDriverLeaseEntry = { lease, creation, session: undefined, taken: undefined }; this.entriesByLeaseId.set(lease.leaseId, entry); // Started after the publication above, so the create's answer can never arrive without an entry // beside it. Whichever route took the entry in the meantime disposes the session this answers. void this.createSessionForLease(lease, req).then( (session) => { - if (this.entriesByLeaseId.get(lease.leaseId) === entry) { - this.entriesByLeaseId.set(lease.leaseId, { status: 'ready', session }); - this.ownedDeviceIds.add(session.device.id); - } + entry.session = session; + this.ownedDeviceIds.add(session.device.id); rentSession(session); }, (error: unknown) => { - if (this.entriesByLeaseId.get(lease.leaseId) === entry) { + // The create billed nothing and gave back what its preparation rented, so the lease is free + // again as soon as nobody is left to dispose anything for it. + if (entry.taken === undefined && this.entriesByLeaseId.get(lease.leaseId) === entry) { this.entriesByLeaseId.delete(lease.leaseId); } refuseSession(error); @@ -240,15 +267,14 @@ export class WebDriverSessionManager { } /** - * Prepares the lease, rents the hub session, and hands back the session this manager can dispose. - * Flags no profile field of this provider reads are refused here, before the hub is asked for - * anything: an allocation that cannot be honoured must not bill a device to find out. + * Prepares the lease and rents the hub session, and hands back the session this manager can dispose. + * The allocation's own flags were refused before it got here, so nothing that cannot be honoured ever + * asks the hub to bill a device. */ private async createSessionForLease( lease: DeviceLease, req: LeaseLifecycleContext | undefined, ): Promise { - await this.rejectRefusedProfileFields(req); const prepared = await this.prepareSession(lease, req); const client = new WebDriverClient({ clientVersion: this.options.clientVersion, @@ -281,26 +307,95 @@ export class WebDriverSessionManager { }); } + /** Takes the lease for `release`, or joins the disposal already ending it. */ + private async takeForRelease(entry: WebDriverLeaseEntry): Promise { + return await this.take(entry, 'release'); + } + + /** Takes the lease for `shutdown`, or joins the disposal already ending it. */ + private async takeForShutdown(entry: WebDriverLeaseEntry): Promise { + return await this.take(entry, 'shutdown'); + } + /** - * The session an entry stands for, once its create has answered, or `undefined` when it stands for - * none because the create billed nothing. The create's failure is the answer of the allocation that - * started it, so a release or shutdown that gets here first takes no error from it. + * Hands one lease's session to the one route that may end it. The take and the disposal it starts are + * recorded before this step returns, so no route can reach the lease in between and find it free, and + * the entry leaves the map only once that disposal has settled. */ - private async disposableSession( - entry: WebDriverLeaseEntry, - ): Promise { - if (entry.status === 'ready') return entry.session; - return await entry.creation.catch(() => undefined); + private take(entry: WebDriverLeaseEntry, by: 'release' | 'shutdown') { + const taken = entry.taken; + if (taken) return taken.disposal; + let reportDisposal!: (outcome: WebDriverDisposalOutcome) => void; + const disposal = new Promise((report) => { + reportDisposal = report; + }); + entry.taken = { + by, + providerSessionId: entry.session?.providerSessionId, + disposal, + settled: undefined, + }; + void this.disposeTakenEntry(entry).then((outcome) => { + const take = entry.taken!; + take.settled = outcome; + take.providerSessionId = outcome.session?.providerSessionId; + // Whatever the create rented is now ended, or was never rented, so this entry owns nothing and the + // lease is free again. + if (this.entriesByLeaseId.get(entry.lease.leaseId) === entry) { + this.entriesByLeaseId.delete(entry.lease.leaseId); + } + reportDisposal(outcome); + }); + return disposal; } - private readySession(entry: WebDriverLeaseEntry | undefined) { - return entry?.status === 'ready' ? entry.session : undefined; + /** + * Ends what one taken entry stands for: the session its create rented, once that create answers, or + * nothing when the create billed nothing. This is the only place a session is ended, and one entry is + * taken by exactly one route, so a session is ended once. + */ + private async disposeTakenEntry(entry: WebDriverLeaseEntry): Promise { + const session = entry.session ?? (await entry.creation.catch(() => undefined)); + entry.session = session; + if (session === undefined) return { cleanup: 'none', session: undefined, warnings: [] }; + this.releasedProviderSessionIdsByLeaseId.set(entry.lease.leaseId, session.providerSessionId); + const warnings: CloudWebDriverReleaseWarning[] = []; + let cleanupPayload: Record | undefined; + try { + await session.client.deleteSession(); + } catch (error) { + warnings.push({ + code: 'WEBDRIVER_SESSION_DELETE_FAILED', + message: errorMessage(error), + }); + } + try { + cleanupPayload = await session.prepared.cleanup?.(); + } catch (error) { + warnings.push({ code: 'PROVIDER_CLEANUP_FAILED', message: errorMessage(error) }); + } + const artifacts = await this.safeListArtifacts(session); + // Only a hub that answered can say the session is gone. A delete the provider refused leaves the + // session billing, and this manager has no way left to learn otherwise. + const cleanup = warnings.length > 0 ? 'uncertain' : 'confirmed'; + return { cleanup, session, warnings, cleanupPayload, artifacts }; } - private readySessions(): WebDriverProviderSession[] { - return [...this.entriesByLeaseId.values()].flatMap((entry) => - entry.status === 'ready' ? [entry.session] : [], - ); + /** + * The refusal for the allocation whose session a teardown took. The disposal's own answer is the only + * thing that can confirm a billed session is gone, so the refused caller waits for it rather than + * claiming an end it cannot see. + */ + private async refusalForTakenSession( + lease: DeviceLease, + take: WebDriverLeaseTake, + ): Promise { + const outcome = await take.disposal; + return refusalForTeardown(this.options.provider, lease, take.by, { + providerSessionId: outcome.session?.providerSessionId, + cleanup: outcome.cleanup, + warnings: outcome.warnings, + }); } private allocationResult(session: WebDriverProviderSession): Record { @@ -389,25 +484,16 @@ export class WebDriverSessionManager { }; } - private async closeSession( - session: WebDriverProviderSession, - ): Promise { - const warnings: CloudWebDriverReleaseWarning[] = []; - let cleanup: Record | undefined; - try { - await session.client.deleteSession(); - } catch (error) { - warnings.push({ - code: 'WEBDRIVER_SESSION_DELETE_FAILED', - message: errorMessage(error), - }); - } - try { - cleanup = await session.prepared.cleanup?.(); - } catch (error) { - warnings.push({ code: 'PROVIDER_CLEANUP_FAILED', message: errorMessage(error) }); - } - return { cleanup, warnings }; + /** The session a lease holds and can still drive: rented, and not on its way to being ended. */ + private liveSession(entry: WebDriverLeaseEntry | undefined) { + return entry?.taken === undefined ? entry?.session : undefined; + } + + private liveSessions(): WebDriverProviderSession[] { + return [...this.entriesByLeaseId.values()].flatMap((entry) => { + const session = this.liveSession(entry); + return session ? [session] : []; + }); } private async safeListArtifacts( @@ -462,45 +548,140 @@ function sessionCreateTimeoutError( ); } +/** What a route that ended one lease's session can say about it, `pending` while it still cannot. */ +type WebDriverTeardownReport = Readonly<{ + providerSessionId: string | undefined; + cleanup: WebDriverDisposalOutcome['cleanup'] | 'pending'; + warnings: CloudWebDriverReleaseWarning[]; +}>; + /** - * A release took the lease's entry while its create was still running and disposed the session that - * create rented. The caller is refused so it cannot drive a session agent-device has already ended. + * The refusal an allocation gets for the teardown that ended the session it stood for: which route ended + * it, and whether that end is confirmed or known to have failed. */ -function allocationRefusedByRelease(provider: string, lease: DeviceLease): AppError { +function refusalForTeardown( + provider: string, + lease: DeviceLease, + by: 'release' | 'shutdown', + report: WebDriverTeardownReport, +): AppError { return new AppError( 'COMMAND_FAILED', - `${provider} released lease ${lease.leaseId} while it was creating the WebDriver session.`, + `${provider} ended the WebDriver session allocated for lease ${lease.leaseId} before the allocation returned.`, { - reason: 'provider_session_released_during_allocation', + reason: + by === 'release' + ? 'provider_session_released_during_allocation' + : 'provider_session_shutdown_during_allocation', provider, leaseId: lease.leaseId, runId: lease.runId, - hint: 'Allocate the lease again; agent-device already stopped the session this allocation created.', + ...(report.providerSessionId ? { providerSessionId: report.providerSessionId } : {}), + cleanup: report.cleanup, + ...(report.warnings.length > 0 + ? { cleanupWarnings: warningMessagesByCode(report.warnings) } + : {}), + hint: teardownHint(provider, report.cleanup), }, ); } /** - * Either this manager was already shut down when the allocation arrived, or it shut down while the - * create was running and disposed what it rented. Either way no session survives, so nothing here has - * to be stopped by hand. + * The refusal for an allocation that arrived while a release was still ending the lease's previous + * session. It rented nothing, so it is told the lease is being ended rather than that a session of its + * own died, and it may not allocate again until that disposal finishes. */ -function allocationRefusedAfterShutdown( +function refusalForTeardownInProgress( provider: string, - leaseId: string, - disposedSession = false, + lease: DeviceLease, + take: WebDriverLeaseTake, ): AppError { + if (take.by === 'shutdown') { + return refusalForTeardown(provider, lease, 'shutdown', { + providerSessionId: take.providerSessionId ?? take.settled?.session?.providerSessionId, + cleanup: take.settled?.cleanup ?? 'pending', + warnings: take.settled?.warnings ?? [], + }); + } + const cleanup = take.settled?.cleanup ?? 'pending'; + return new AppError( + // ADR 0010: the lease slot is held by the release in flight, so this is contention, and the caller + // can expect the same allocate to succeed once that release finishes. + 'DEVICE_IN_USE', + `${provider} is releasing lease ${lease.leaseId} and allocated no WebDriver session for it.`, + { + reason: 'provider_session_release_in_progress', + provider, + leaseId: lease.leaseId, + runId: lease.runId, + ...(take.providerSessionId ? { providerSessionId: take.providerSessionId } : {}), + cleanup, + hint: + cleanup === 'uncertain' + ? `agent-device could not stop the previous session of this lease. Check ${provider} for it before allocating again.` + : 'Wait for this lease to finish releasing, then allocate it again.', + }, + ); +} + +/** + * This manager was already shut down when the allocation arrived, so it created nothing and owes nothing. + */ +function allocationRefusedAfterShutdown(provider: string, lease: DeviceLease): AppError { return new AppError( 'COMMAND_FAILED', - `${provider} is shut down and created no WebDriver session for lease ${leaseId}.`, + `${provider} is shut down and created no WebDriver session for lease ${lease.leaseId}.`, { reason: 'provider_session_shutdown_during_allocation', provider, - leaseId, - disposedSession, - hint: disposedSession - ? 'Nothing to clean up: agent-device stopped the session it was creating.' - : 'Start the daemon again to allocate a provider session.', + leaseId: lease.leaseId, + runId: lease.runId, + cleanup: 'none', + hint: 'Start the daemon again to allocate a provider session.', + }, + ); +} + +/** + * What the refused allocation should do next: nothing once the hub confirmed the session is gone, and a + * manual stop on the provider when the hub did not confirm it. + */ +function teardownHint( + provider: string, + cleanup: WebDriverDisposalOutcome['cleanup'] | 'pending', +): string { + if (cleanup === 'uncertain') { + return `${provider} did not confirm it stopped the session this allocation created, and that session may keep billing. Check ${provider} for it before allocating the lease again.`; + } + if (cleanup === 'pending') { + return `agent-device is still stopping the session this allocation created. Wait for that release to finish before allocating the lease again.`; + } + if (cleanup === 'none') { + return 'Nothing was billed for this allocation. Allocate the lease again.'; + } + return 'Allocate the lease again; agent-device stopped the session this allocation created.'; +} + +/** + * Sessions this manager could not end at close. A daemon close reports what it left running, so the + * error names every session still billing and why it is still there, and the create-retry policy stays + * untouched: nothing here deletes or creates a session again on its own. + */ +function shutdownLeftSessionsRunning( + provider: string, + outcomes: WebDriverDisposalOutcome[], +): AppError { + const providerSessionIds = outcomes.map((outcome) => outcome.session?.providerSessionId); + return new AppError( + 'COMMAND_FAILED', + `${provider} left ${String(outcomes.length)} WebDriver session(s) running after shutdown: ${providerSessionIds.join(', ')}.`, + { + reason: 'provider_session_cleanup_incomplete', + provider, + providerSessionIds, + cleanup: 'uncertain', + cleanupWarnings: warningMessagesByCode(outcomes.flatMap((outcome) => outcome.warnings)), + hint: `${provider} did not stop these sessions and they keep billing. Stop the session ids above on ${provider}.`, }, ); } From 7490b82b000422baf792fb729ca2feda2c39fc54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 10:50:56 +0100 Subject: [PATCH 3/5] fix(provider-webdriver): keep a lease owned through its whole disposal Two windows stayed open on the reviewed head. An allocation validated its profile fields before it published the lease's entry, so a release issued while that check was pending found no entry, answered that the lease held nothing, and the create that resumed billed a session no route was left to stop. Cloud leases without a device key are not serialized by the daemon's lease registry, so both calls can be in flight at once. The entry is now published before the allocation's first await, and the publishing caller's own flags are refused by the create it stands on, as its first step, so a call that cannot be honoured still bills nothing. A caller that joined an existing entry is still refused for its own flags, and its live session answer is still read with no await after the lookup. Ending a session also waited on the plugin's artifact lookup, which has no bound of its own. A custom listArtifacts that never answered kept the lease DEVICE_IN_USE for every later caller and was awaited by daemon shutdown. Ownership now ends once the remote DELETE and the provider's cleanup settle; the artifact report is started at that point and joined only by the release that carries it. Both provider callbacks the manager awaits are bounded by providerCallbackTimeoutMs, defaulting to this package's one-round-trip budget, and a cleanup that outlives it is reported as PROVIDER_CLEANUP_FAILED with the session's outcome uncertain rather than holding the lease. Also: a disposal that could not be observed at all now reports itself instead of hanging every waiter; the reuse answer and heartbeat share one envelope builder; and a shutdown's hint names the shutdown rather than a release. --- .../src/runtime-session-allocation.test.ts | 205 +++++++++++++++++- .../provider-webdriver/src/runtime-session.ts | 203 ++++++++++++----- packages/provider-webdriver/src/runtime.ts | 6 + 3 files changed, 358 insertions(+), 56 deletions(-) diff --git a/packages/provider-webdriver/src/runtime-session-allocation.test.ts b/packages/provider-webdriver/src/runtime-session-allocation.test.ts index 419cef3500..7cc2522aee 100644 --- a/packages/provider-webdriver/src/runtime-session-allocation.test.ts +++ b/packages/provider-webdriver/src/runtime-session-allocation.test.ts @@ -85,6 +85,35 @@ test('an allocation refused for its own flags leaves the create it joined alone' } }); +/** + * A new allocation publishes its entry before it can await, and its own flags are the create's first + * step, so a call this provider cannot honour still bills nothing and never reaches preparation. + */ +test('an allocation refused for its own flags bills nothing before it reaches the provider', async () => { + const hub = startWebDriverHub(); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ + prepareSession: preparation.prepareSession, + profileFields: refuseAllProfileFields('webdriver-test'), + }); + const lease = makeLease(); + + try { + globalThis.fetch = hub.fetch; + assertRejectedWith( + await settle(() => + runtime.leaseLifecycle.allocate!(lease, { flags: { providerApp: 'b' } } as never), + ), + 'INVALID_ARGS', + { flags: ['--provider-app'] }, + ); + assert.deepEqual(hub.created, [], 'a refused allocation never asks the hub to bill a device'); + assert.equal(preparation.started, 0, 'a refused allocation never reaches the provider'); + } finally { + await runtime.shutdown(); + } +}); + /** * A release that landed mid-creation found no published session and answered that the lease held * nothing, so the hub session that creation went on to make had no route left to stop it. The caller @@ -281,6 +310,162 @@ test('a repeated allocation cannot answer that a shut down lease is alive', asyn } }); +/** + * An allocation reaches its first await before provider preparation begins, and a release issued in + * that moment used to find nothing holding the lease and answer that it had released it. The create that + * went on to run then billed a session no route was left to stop. The lease has to belong to the + * allocation from the moment it is asked for, so the release ends up taking it and disposing what the + * create rents. + */ +test('a release issued before provider preparation begins disposes the session the allocation rents', async () => { + const hub = startWebDriverHub(); + const preparation = startProviderPreparation(); + const runtime = makeRuntime({ prepareSession: preparation.prepareSession }); + const lease = makeLease(); + + try { + globalThis.fetch = hub.fetch; + // Both calls are issued in one synchronous step, so the release reaches the manager while the + // allocation is at its first await and provider preparation has not answered. + const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + assert.deepEqual(hub.created, [], 'nothing may be billed before preparation answers'); + // The allocation has reached provider preparation, which is still held. + await preparation.waitFor(1); + await preparation.answerHeld(); + + assert.deepEqual(await releasing, { + status: 'resolved', + value: { provider: 'webdriver-test', providerSessionId: 'session-1' }, + }); + assertRejectedWith(await allocating, 'provider_session_released_during_allocation', { + leaseId: lease.leaseId, + providerSessionId: 'session-1', + cleanup: 'confirmed', + }); + // The session the create rented is gone: a release that reported success leaves no billed session. + assert.deepEqual(hub.deleted, ['session-1']); + } finally { + await preparation.answerHeld(); + await runtime.shutdown(); + } +}); + +/** + * Ending a session ends the billed resource and hands back what the provider prepared; the artifact + * lookup that follows is only what the release envelope reports. A plugin's own artifact callback has + * no bound of its own, so it may not be what keeps a lease from being allocated again, what keeps a + * second release from answering, or what a daemon close waits through. + */ +test('an artifact callback that never answers holds neither the lease nor shutdown', async () => { + const hub = startWebDriverHub(); + globalThis.fetch = hub.fetch; + let answerArtifacts: (() => void) | undefined; + let lookupStarted = false; + const runtime = makeRuntime({ + providerCallbackTimeoutMs: 40, + listArtifacts: async () => { + lookupStarted = true; + await new Promise((ready) => { + answerArtifacts = ready; + }); + return { provider: 'webdriver-test', status: 'ready', cloudArtifacts: [] }; + }, + }); + const lease = makeLease(); + + try { + await runtime.leaseLifecycle.allocate!(lease); + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + // The lookup starts only once the hub has ended the session and the provider's cleanup has answered, + // so its having started is the moment the billed work is finished. + await untilSettled(() => lookupStarted); + assert.deepEqual(hub.deleted, ['session-1']); + await schedulerYield(); + + // A re-allocation during the hanging lookup is the lease's own business, not the lookup's. + const reallocated = settle(() => runtime.leaseLifecycle.allocate!(lease)); + assertResolvedWith(await reallocated, 'session-2', describe(await reallocated)); + const shuttingDown = settle(() => runtime.shutdown()); + assert.deepEqual( + await shuttingDown, + { status: 'resolved', value: undefined }, + 'shutdown may not wait on an optional artifact lookup', + ); + + const result = await releasing; + assert.equal(result.status, 'resolved', describe(result)); + const value = (result as Readonly<{ value: Record | undefined }>).value; + assert.equal(value?.providerSessionId, 'session-1'); + // What the lookup could not answer for becomes an unavailable report, the way its failures do. + assert.equal( + (value?.cloudArtifacts as Record | undefined)?.status, + 'unavailable', + ); + assert.deepEqual(hub.created, ['session-1', 'session-2']); + assert.deepEqual(hub.deleted, ['session-1', 'session-2']); + } finally { + answerArtifacts?.(); + await runtime.shutdown(); + } +}); + +/** + * A provider's own cleanup is the half of disposal the hub's `DELETE` does not cover, and a custom one + * has no bound either. Once its budget runs out this manager cannot learn whether the resource was + * handed back, so it says so and lets the lease be used again rather than holding it forever. + */ +test('a provider cleanup that outlives its budget ends the disposal as uncertain', async () => { + const hub = startWebDriverHub(); + globalThis.fetch = hub.fetch; + let answerCleanup: (() => void) | undefined; + let cleanupCalls = 0; + const runtime = makeRuntime({ + providerCallbackTimeoutMs: 40, + prepareSession: async ({ base }) => ({ + ...base, + cleanup: async () => { + cleanupCalls += 1; + if (cleanupCalls > 1) return undefined; + await new Promise((ready) => { + answerCleanup = ready; + }); + return undefined; + }, + }), + }); + const lease = makeLease(); + + try { + await runtime.leaseLifecycle.allocate!(lease); + const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); + await untilSettled(() => hub.deleted.includes('session-1')); + + const result = await releasing; + assert.equal(result.status, 'resolved', describe(result)); + const value = (result as Readonly<{ value: Record | undefined }>).value; + assert.deepEqual(warningCodes(value?.warnings), ['PROVIDER_CLEANUP_FAILED']); + assert.match(String(warningMessages(value?.warnings)[0]), /40ms/); + + // The lease is the next caller's again, and a later session this manager can end still ends cleanly. + assertResolvedWith( + await settle(() => runtime.leaseLifecycle.allocate!(lease)), + 'session-2', + 'a cleanup whose outcome is unknown may not keep the lease', + ); + assert.deepEqual( + await settle(() => runtime.shutdown()), + { status: 'resolved', value: undefined }, + 'only a session the provider would not end may fail a close', + ); + assert.deepEqual(hub.created, ['session-1', 'session-2']); + assert.deepEqual(hub.deleted, ['session-1', 'session-2']); + } finally { + answerCleanup?.(); + await runtime.shutdown(); + } +}); + /** * A hub that will not end a session leaves it billing, so the release that was told to end it names the * session and the provider's refusal, exactly as it does for a session it was handed ready-made. @@ -484,12 +669,30 @@ async function schedulerYield(): Promise { }); } +/** + * Waits for the manager to reach `condition` — the hub having been asked to delete a session, say — so a + * test can name the moment after the billed work is done and before an optional callback has answered. + */ +async function untilSettled(condition: () => boolean, withinMs = 2_000): Promise { + for (let waited = 0; !condition() && waited < withinMs; waited += 5) { + await new Promise((ready) => { + setTimeout(ready, 5); + }); + } + assert.ok(condition(), 'the manager never reached the step the test was waiting for'); +} + /** The allocation envelope names the hub session the lease now holds. */ function assertResolvedWith( outcome: Settled | undefined>, sessionId: string, + expectation?: string, ): void { - assert.equal(outcome.status, 'resolved', `expected ${sessionId}, got ${describe(outcome)}`); + assert.equal( + outcome.status, + 'resolved', + `${expectation ?? `expected ${sessionId}`}, got ${describe(outcome)}`, + ); assert.equal( (outcome as Readonly<{ value: Record | undefined }>).value?.sessionId, sessionId, diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index 8ef73152e7..78117c087f 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -69,7 +69,9 @@ type WebDriverLeaseEntry = { /** * Whichever route took a lease away from its allocation, and the single disposal of what that entry * stood for. `settled` is what that disposal reported and stays missing while it is still running, so a - * caller can tell a session that is gone from one whose outcome it cannot see yet. + * caller can tell a session that is gone from one whose outcome it cannot see yet. `artifacts` is the + * release envelope's report and is deliberately separate: it is optional, so it is never what a lease or + * a daemon close waits through. */ type WebDriverLeaseTake = { by: 'release' | 'shutdown'; @@ -77,6 +79,8 @@ type WebDriverLeaseTake = { providerSessionId: string | undefined; disposal: Promise; settled: WebDriverDisposalOutcome | undefined; + /** The artifact report, started once the billed resources are settled, and joined by the release. */ + artifacts: Promise | undefined; }; /** @@ -90,7 +94,6 @@ type WebDriverDisposalOutcome = Readonly<{ warnings: CloudWebDriverReleaseWarning[]; /** What the provider's own cleanup reported, for the release envelope. */ cleanupPayload?: Record; - artifacts?: CloudArtifactsResult; }>; /** The fields of one warning, keyed as `details.cleanupWarnings` carries them on a refusal. */ @@ -98,6 +101,13 @@ function warningMessagesByCode(warnings: CloudWebDriverReleaseWarning[]): Record return Object.fromEntries(warnings.map((warning) => [warning.code, warning.message])); } +/** + * Budget for one provider callback this manager waits on. A plugin may supply its own `prepareSession` + * cleanup and artifact lookup, and those callbacks are the plugin's code, so this manager cannot assume + * they answer: they get the budget this package already gives one WebDriver round trip. + */ +const DEFAULT_PROVIDER_CALLBACK_TIMEOUT_MS = 30_000; + /** * Owns WebDriver session lifecycle and stale-owner retention. Deployment decisions stay in the * sibling runtime-deployment module so neither concern widens the public runtime façade. @@ -133,21 +143,25 @@ export class WebDriverSessionManager { heartbeat(lease: DeviceLease): Record | undefined { if (lease.leaseProvider !== this.options.provider) return undefined; if (!this.liveSession(this.entriesByLeaseId.get(lease.leaseId))) return undefined; - return { provider: this.options.provider }; + return this.liveLeaseEnvelope(); } async allocate(lease: DeviceLease, req?: LeaseLifecycleContext): Promise { if (lease.leaseProvider !== this.options.provider) return undefined; - // Every allocation is refused for the flags this provider reads no profile field for, before it - // joins or publishes anything: a call answers for its own flags and never inherits another call's, - // and one that cannot be honoured bills nothing. - await this.rejectRefusedProfileFields(req); - // The lease's entry is published here, in the same synchronous step as the lookup above: a second - // allocation of this lease joins the create rather than renting a second billed session, and a - // release or shutdown that arrives while the create runs finds the session it is renting. + const held = this.entriesByLeaseId.get(lease.leaseId); + // Joining someone else's entry, so this call answers for its own flags, and it settles them before it + // reads any state. Nothing else can await on this route: the lookup below is the last one, and the + // answer is read from it with no await after, so a release or a shutdown that lands while the flags + // are being checked is the outcome this call reports rather than a session already ended. + if (held !== undefined) await this.rejectRefusedProfileFields(req); + // Re-read after that await, and nothing awaits after this lookup except joining the create below, so + // a release or a shutdown that landed in the meantime is what this call reports. let entry = this.entriesByLeaseId.get(lease.leaseId); if (entry === undefined) { if (this.shutDown) throw allocationRefusedAfterShutdown(this.options.provider, lease); + // Published before this call awaits anything, so a lease is never unowned while its allocation is at + // its first await: a release or a shutdown that arrives there finds this entry and takes it, and the + // create the entry stands on checks this call's own profile flags before it bills anything. entry = this.beginAllocation(lease, req); } // A lease whose disposal is running belongs to the route that is ending it, so this call rented @@ -155,7 +169,7 @@ export class WebDriverSessionManager { if (entry.taken) throw refusalForTeardownInProgress(this.options.provider, lease, entry.taken); // Reported without awaiting anything after the lookup, so the session is live at the moment the // caller is told about it: a release or a shutdown cannot end it inside this call. - if (entry.session) return { provider: this.options.provider }; + if (entry.session) return this.liveLeaseEnvelope(); const session = await entry.creation; // The session this call rented is its own only while nobody took the lease. Otherwise a release or // shutdown ended what the create rented, and this caller is refused rather than handed a session id @@ -170,15 +184,20 @@ export class WebDriverSessionManager { // Nothing holds the lease: it was never allocated here, or its release finished and gave the lease // back. A release that arrives while another one disposes joins that disposal. if (entry === undefined) return undefined; - const outcome = await this.takeForRelease(entry); + const take = this.take(entry, 'release'); + const outcome = await take.disposal; // A create that failed billed nothing, and gave back what its preparation had already rented. if (outcome.session === undefined) return undefined; + // The artifact report is joined here, after the lease has been given back and the billed session is + // known to be ended: a slow plugin lookup may delay this answer, and nothing that runs against the + // lease. + const artifacts = await take.artifacts; return { provider: this.options.provider, providerSessionId: outcome.session.providerSessionId, ...outcome.cleanupPayload, ...(outcome.warnings.length > 0 ? { warnings: outcome.warnings } : {}), - ...(outcome.artifacts ? { cloudArtifacts: outcome.artifacts } : {}), + ...(artifacts ? { cloudArtifacts: artifacts } : {}), }; } @@ -219,10 +238,8 @@ export class WebDriverSessionManager { this.shutDown = true; // Snapshot before anything is awaited: `allocate` publishes an entry only when none exists and // refuses once this manager is shut down, and nothing between that flag and this loop awaits. - const disposals = [...this.entriesByLeaseId.values()].map( - async (entry) => await this.takeForShutdown(entry), - ); - const outcomes = await Promise.all(disposals); + const takes = [...this.entriesByLeaseId.values()].map((entry) => this.take(entry, 'shutdown')); + const outcomes = await Promise.all(takes.map((take) => take.disposal)); this.ownedDeviceIds.clear(); // A session the provider would not give back is still billing, and this process will not stop it. // The plugin's close is where an operator learns what a daemon close left running, so the session @@ -268,13 +285,15 @@ export class WebDriverSessionManager { /** * Prepares the lease and rents the hub session, and hands back the session this manager can dispose. - * The allocation's own flags were refused before it got here, so nothing that cannot be honoured ever - * asks the hub to bill a device. + * The caller's own profile flags are refused here, first, so an allocation that published its entry + * before it could await anything still bills nothing for flags this provider reads no field for, and a + * release that took the entry meanwhile has nothing to end. */ private async createSessionForLease( lease: DeviceLease, req: LeaseLifecycleContext | undefined, ): Promise { + await this.rejectRefusedProfileFields(req); const prepared = await this.prepareSession(lease, req); const client = new WebDriverClient({ clientVersion: this.options.clientVersion, @@ -307,52 +326,63 @@ export class WebDriverSessionManager { }); } - /** Takes the lease for `release`, or joins the disposal already ending it. */ - private async takeForRelease(entry: WebDriverLeaseEntry): Promise { - return await this.take(entry, 'release'); - } - - /** Takes the lease for `shutdown`, or joins the disposal already ending it. */ - private async takeForShutdown(entry: WebDriverLeaseEntry): Promise { - return await this.take(entry, 'shutdown'); - } - /** * Hands one lease's session to the one route that may end it. The take and the disposal it starts are * recorded before this step returns, so no route can reach the lease in between and find it free, and - * the entry leaves the map only once that disposal has settled. + * the entry leaves the map once that disposal has settled — which is once the billed resources have + * been ended, not once every optional report about them has been collected. */ - private take(entry: WebDriverLeaseEntry, by: 'release' | 'shutdown') { + private take(entry: WebDriverLeaseEntry, by: 'release' | 'shutdown'): WebDriverLeaseTake { const taken = entry.taken; - if (taken) return taken.disposal; + if (taken) return taken; let reportDisposal!: (outcome: WebDriverDisposalOutcome) => void; const disposal = new Promise((report) => { reportDisposal = report; }); - entry.taken = { + const take: WebDriverLeaseTake = { by, providerSessionId: entry.session?.providerSessionId, disposal, settled: undefined, + artifacts: undefined, }; - void this.disposeTakenEntry(entry).then((outcome) => { - const take = entry.taken!; + entry.taken = take; + const settle = (outcome: WebDriverDisposalOutcome): void => { take.settled = outcome; take.providerSessionId = outcome.session?.providerSessionId; + // The artifact report belongs to the release envelope and to nothing else: it starts here and the + // release joins it, while the lease and a daemon close are answered by `disposal` alone. A plugin's + // artifact lookup therefore cannot keep a lease from being allocated again, nor a close from + // finishing. + take.artifacts = + outcome.session === undefined + ? undefined + : this.safeListArtifacts(outcome.session, true).catch(() => undefined); // Whatever the create rented is now ended, or was never rented, so this entry owns nothing and the // lease is free again. if (this.entriesByLeaseId.get(entry.lease.leaseId) === entry) { this.entriesByLeaseId.delete(entry.lease.leaseId); } reportDisposal(outcome); + }; + // `disposeTakenEntry` reports its own failures, so the rejection branch is only for what it could not + // report at all. It still gives the lease back, because a disposal this manager lost sight of is + // reported, not waited on forever. + void this.disposeTakenEntry(entry).then(settle, (error: unknown) => { + settle({ + cleanup: 'uncertain', + session: entry.session, + warnings: [{ code: 'PROVIDER_CLEANUP_FAILED', message: errorMessage(error) }], + }); }); - return disposal; + return take; } /** * Ends what one taken entry stands for: the session its create rented, once that create answers, or * nothing when the create billed nothing. This is the only place a session is ended, and one entry is - * taken by exactly one route, so a session is ended once. + * taken by exactly one route, so a session is ended once. Everything here is bounded: the provider's own + * cleanup belongs to the plugin that supplied it, and this manager must still answer when it does not. */ private async disposeTakenEntry(entry: WebDriverLeaseEntry): Promise { const session = entry.session ?? (await entry.creation.catch(() => undefined)); @@ -370,15 +400,53 @@ export class WebDriverSessionManager { }); } try { - cleanupPayload = await session.prepared.cleanup?.(); + cleanupPayload = await this.providerCallback( + `${this.options.provider} cleanup for lease ${entry.lease.leaseId}`, + () => session.prepared.cleanup?.(), + ); } catch (error) { warnings.push({ code: 'PROVIDER_CLEANUP_FAILED', message: errorMessage(error) }); } - const artifacts = await this.safeListArtifacts(session); // Only a hub that answered can say the session is gone. A delete the provider refused leaves the // session billing, and this manager has no way left to learn otherwise. const cleanup = warnings.length > 0 ? 'uncertain' : 'confirmed'; - return { cleanup, session, warnings, cleanupPayload, artifacts }; + return { cleanup, session, warnings, cleanupPayload }; + } + + /** + * Awaits one provider callback the manager was handed, within the budget this package gives a provider + * round trip. Past it, the outcome is a refusal: the callback keeps running and this manager reports + * what it could not learn rather than holding the lease open for it. + */ + private async providerCallback( + label: string, + run: () => Promise | undefined, + ): Promise { + const timeoutMs = + this.options.providerCallbackTimeoutMs ?? DEFAULT_PROVIDER_CALLBACK_TIMEOUT_MS; + const pending = Promise.resolve(run()); + // A callback that was too slow is no longer awaited, so its late outcome must not surface as an + // unhandled rejection. + void pending.catch(() => undefined); + let timer: ReturnType | undefined; + try { + return await Promise.race([ + pending, + new Promise((_resolve, refuse) => { + timer = setTimeout(() => { + refuse( + new AppError( + 'COMMAND_FAILED', + `${label} did not answer within ${String(timeoutMs)}ms.`, + ), + ); + }, timeoutMs); + timer.unref?.(); + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } } /** @@ -398,6 +466,14 @@ export class WebDriverSessionManager { }); } + /** + * What this manager says about a lease it holds a live session for. `heartbeat` and the reuse answer of + * `allocate` share it, so the daemon's `recordProviderSession` sees one envelope from both routes. + */ + private liveLeaseEnvelope(): Record { + return { provider: this.options.provider }; + } + private allocationResult(session: WebDriverProviderSession): Record { return { provider: this.options.provider, @@ -430,8 +506,9 @@ export class WebDriverSessionManager { /** * Refuses the flags this provider reads no profile field for, so a field that would be ignored is - * reported instead of dropped. The module is loaded on first use to keep the package entry's own - * closure unchanged. + * reported instead of dropped. Each allocation answers for its own flags: the one that published the + * entry is refused by its own create, before it can bill anything, and one that joined an entry is + * refused here. The module is loaded on first use to keep the package entry's own closure unchanged. */ private async rejectRefusedProfileFields(req: LeaseLifecycleContext | undefined): Promise { const { rejectRefusedProviderProfileFields } = @@ -496,19 +573,31 @@ export class WebDriverSessionManager { }); } + /** + * Collects one session's artifacts, reporting anything the provider could not answer for. The + * disposal path asks for the callback budget, because it must answer for a lease; `listCloudArtifacts` + * is a caller's own request and is left to the provider's own bound. + */ private async safeListArtifacts( session: WebDriverProviderSession, + withinCallbackBudget = false, ): Promise { const listArtifacts = session.prepared.listArtifacts ?? this.options.listArtifacts; if (!listArtifacts) return undefined; + const params = { + provider: this.options.provider, + lease: session.lease, + device: session.device, + webDriverSessionId: session.webDriverSessionId, + providerSessionId: session.providerSessionId, + }; try { - return await listArtifacts({ - provider: this.options.provider, - lease: session.lease, - device: session.device, - webDriverSessionId: session.webDriverSessionId, - providerSessionId: session.providerSessionId, - }); + return withinCallbackBudget + ? await this.providerCallback( + `${this.options.provider} artifact lookup for session ${session.providerSessionId}`, + () => listArtifacts(params), + ) + : await listArtifacts(params); } catch (error) { return unavailableCloudArtifactsResult({ provider: this.options.provider, @@ -581,7 +670,7 @@ function refusalForTeardown( ...(report.warnings.length > 0 ? { cleanupWarnings: warningMessagesByCode(report.warnings) } : {}), - hint: teardownHint(provider, report.cleanup), + hint: teardownHint(provider, by, report.cleanup), }, ); } @@ -596,14 +685,17 @@ function refusalForTeardownInProgress( lease: DeviceLease, take: WebDriverLeaseTake, ): AppError { + // Both answers describe the same disposal, so both read the same two facts from it. The hint stays + // separate: this caller created nothing, so it is told about the lease, not about a session of its own. + const cleanup = take.settled?.cleanup ?? 'pending'; + const providerSessionId = take.providerSessionId ?? take.settled?.session?.providerSessionId; if (take.by === 'shutdown') { return refusalForTeardown(provider, lease, 'shutdown', { - providerSessionId: take.providerSessionId ?? take.settled?.session?.providerSessionId, - cleanup: take.settled?.cleanup ?? 'pending', + providerSessionId, + cleanup, warnings: take.settled?.warnings ?? [], }); } - const cleanup = take.settled?.cleanup ?? 'pending'; return new AppError( // ADR 0010: the lease slot is held by the release in flight, so this is contention, and the caller // can expect the same allocate to succeed once that release finishes. @@ -614,7 +706,7 @@ function refusalForTeardownInProgress( provider, leaseId: lease.leaseId, runId: lease.runId, - ...(take.providerSessionId ? { providerSessionId: take.providerSessionId } : {}), + ...(providerSessionId ? { providerSessionId } : {}), cleanup, hint: cleanup === 'uncertain' @@ -648,13 +740,14 @@ function allocationRefusedAfterShutdown(provider: string, lease: DeviceLease): A */ function teardownHint( provider: string, + by: 'release' | 'shutdown', cleanup: WebDriverDisposalOutcome['cleanup'] | 'pending', ): string { if (cleanup === 'uncertain') { return `${provider} did not confirm it stopped the session this allocation created, and that session may keep billing. Check ${provider} for it before allocating the lease again.`; } if (cleanup === 'pending') { - return `agent-device is still stopping the session this allocation created. Wait for that release to finish before allocating the lease again.`; + return `agent-device is still stopping the session this allocation created. Wait for that ${by === 'release' ? 'release' : 'shutdown'} to finish before allocating the lease again.`; } if (cleanup === 'none') { return 'Nothing was billed for this allocation. Allocate the lease again.'; diff --git a/packages/provider-webdriver/src/runtime.ts b/packages/provider-webdriver/src/runtime.ts index 4f400bf28c..66dbca8298 100644 --- a/packages/provider-webdriver/src/runtime.ts +++ b/packages/provider-webdriver/src/runtime.ts @@ -103,6 +103,12 @@ export type CloudWebDriverRuntimeOptions = { capabilityOverrides?: CloudWebDriverCapabilityOverrides; /** Profile fields this provider reads; any field it refuses fails lease allocation. */ profileFields: ProviderProfileFieldDeclaration; + /** + * Budget for one provider callback the session manager waits on — a prepared session's `cleanup` and + * the artifact lookup — so a plugin callback that never answers cannot hold a lease open. Defaults to + * the budget one WebDriver round trip gets. + */ + providerCallbackTimeoutMs?: number; }; export function createCloudWebDriverRuntime( From 86216483fb980ab3d9c6fe6ddd5a64b6cfa60041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 12:15:29 +0100 Subject: [PATCH 4/5] fix(provider-webdriver): bound the create's cleanup and keep shutdown warnings per session Two review threads on the reviewed head. The new provider-callback budget covered a session's disposal but not the hand-back a create owes when the hub refuses it. That cleanup is the plugin's own code too, and no session exists yet for a release or a shutdown to end, so a custom cleanup that never answered held the allocation, the lease behind it, and daemon shutdown forever. It now runs through the same bounded providerCallback. A close that could not stop several sessions collapsed their warnings into one map keyed by code, so two sessions refused by the provider were reported with one reason. Each warning now travels as its own entry with the session id it is about, so every session in the shutdown error stays diagnosable. The allocation tests stopped polling the clock for a step to happen: waiting for a create to reach the provider, for the hub to end a session, and for an artifact lookup to start are all arrival signals now, each resolved in the step that reaches it, with one watchdog that only fires when a step never comes. A busy CI worker can no longer fail a test whose allocation is still progressing. --- .../src/runtime-session-allocation.test.ts | 239 ++++++++++++++++-- .../provider-webdriver/src/runtime-session.ts | 19 +- packages/provider-webdriver/src/runtime.ts | 7 +- 3 files changed, 232 insertions(+), 33 deletions(-) diff --git a/packages/provider-webdriver/src/runtime-session-allocation.test.ts b/packages/provider-webdriver/src/runtime-session-allocation.test.ts index 7cc2522aee..4b5fcf9e9d 100644 --- a/packages/provider-webdriver/src/runtime-session-allocation.test.ts +++ b/packages/provider-webdriver/src/runtime-session-allocation.test.ts @@ -361,11 +361,11 @@ test('an artifact callback that never answers holds neither the lease nor shutdo const hub = startWebDriverHub(); globalThis.fetch = hub.fetch; let answerArtifacts: (() => void) | undefined; - let lookupStarted = false; + const lookupStarted = startSignal(); const runtime = makeRuntime({ providerCallbackTimeoutMs: 40, listArtifacts: async () => { - lookupStarted = true; + lookupStarted.report(); await new Promise((ready) => { answerArtifacts = ready; }); @@ -379,7 +379,7 @@ test('an artifact callback that never answers holds neither the lease nor shutdo const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); // The lookup starts only once the hub has ended the session and the provider's cleanup has answered, // so its having started is the moment the billed work is finished. - await untilSettled(() => lookupStarted); + await awaitSignal(lookupStarted.arrived, 'the artifact lookup to start'); assert.deepEqual(hub.deleted, ['session-1']); await schedulerYield(); @@ -439,7 +439,7 @@ test('a provider cleanup that outlives its budget ends the disposal as uncertain try { await runtime.leaseLifecycle.allocate!(lease); const releasing = settle(() => runtime.leaseLifecycle.release!(lease)); - await untilSettled(() => hub.deleted.includes('session-1')); + await hub.deletedArrived('session-1'); const result = await releasing; assert.equal(result.status, 'resolved', describe(result)); @@ -493,6 +493,118 @@ test('a release the provider refused reports the session left running', async () } }); +/** + * A create the hub refused still hands back what the provider prepared, and that hand-back is the + * plugin's own code too. A `cleanup` that never answers there would keep the allocation from answering, + * with the lease held behind it and nothing on the lease left to end, so it gets the same budget the + * disposal's cleanup gets. + */ +test('a create the hub refused still answers when its provider cleanup hangs', async () => { + const hub = startWebDriverHub({ createFailure: 'hub refused the create' }); + globalThis.fetch = hub.fetch; + let answerCleanup: (() => void) | undefined; + const runtime = makeRuntime({ + providerCallbackTimeoutMs: 40, + prepareSession: async ({ base }) => ({ + ...base, + cleanup: async () => { + await new Promise((ready) => { + answerCleanup = ready; + }); + return undefined; + }, + }), + }); + const lease = makeLease(); + + try { + const allocating = settle(() => runtime.leaseLifecycle.allocate!(lease)); + // The create failed and its cleanup is the step that will not answer, so the allocation cannot be + // left waiting on it. + const outcome = await allocating; + assert.equal( + outcome.status, + 'rejected', + `the refused create still reports the hub's own refusal, got ${describe(outcome)}`, + ); + const error = (outcome as Readonly<{ error: AppError }>).error; + const details = (error.details ?? {}) as Record; + assert.equal(details.status, 500, 'the hub refusal is what the caller is told'); + assert.match(String(details.cleanupError), /did not answer within 40ms/); + + // The lease is the next caller's again: an allocation issued after the hung cleanup reaches the hub + // and is refused for the hub's own refusal, not for a teardown that never finishes. And a close does + // not wait on a callback it gave up on. + const nextAllocation = await settle(() => runtime.leaseLifecycle.allocate!(lease)); + assert.equal(nextAllocation.status, 'rejected', describe(nextAllocation)); + const nextDetails = ((nextAllocation as Readonly<{ error: AppError }>).error.details ?? + {}) as Record; + assert.equal( + nextDetails.status, + 500, + 'the lease is free, so this caller hears the hub again rather than a contention refusal', + ); + assert.deepEqual( + await settle(() => runtime.shutdown()), + { status: 'resolved', value: undefined }, + 'a create cleanup this manager gave up on may not hold a close', + ); + assert.deepEqual(hub.created, []); + } finally { + answerCleanup?.(); + await runtime.shutdown(); + } +}); + +/** + * A close reports every session it left billing, and each session keeps its own reason in that report: + * two sessions the provider refused to end carry two refusals, and collapsing them by warning code would + * hand the operator one message for both. + */ +test('a shutdown that could not stop two sessions names both refusals', async () => { + const hub = startWebDriverHub({ deleteFailure: 'provider rejected the delete' }); + globalThis.fetch = hub.fetch; + const runtime = makeRuntime(); + const first = { ...makeLease(), leaseId: 'lease-a' }; + const second = { ...makeLease(), leaseId: 'lease-b' }; + + try { + await runtime.leaseLifecycle.allocate!(first); + await runtime.leaseLifecycle.allocate!(second); + assert.deepEqual(hub.created, ['session-1', 'session-2']); + + const shuttingDown = await settle(() => runtime.shutdown()); + assert.equal( + shuttingDown.status, + 'rejected', + 'a close that left two sessions billing is not a clean close', + ); + const details = ((shuttingDown as Readonly<{ error: AppError }>).error.details ?? {}) as Record< + string, + unknown + >; + assert.equal(details.reason, 'provider_session_cleanup_incomplete'); + assert.deepEqual(details.providerSessionIds, ['session-1', 'session-2']); + // Each named session carries the reason this provider gave for it, so an operator can tell the two + // refusals apart instead of being handed one message for both sessions. + const warnings = details.cleanupWarnings as Array>; + assert.deepEqual( + warnings.map((warning) => [warning.providerSessionId, warning.code]), + [ + ['session-1', 'WEBDRIVER_SESSION_DELETE_FAILED'], + ['session-2', 'WEBDRIVER_SESSION_DELETE_FAILED'], + ], + ); + assert.deepEqual( + warnings.map((warning) => /session-\d/.exec(String(warning.message))?.[0]), + ['session-1', 'session-2'], + ); + } finally { + hub.allowDeletes(); + await runtime.shutdown(); + } +}); + /** * A shutdown that could not stop a session may not report a clean close, and the allocation whose * session it was is told the outcome is uncertain rather than that nothing needs cleaning up. @@ -540,16 +652,31 @@ test('a session shutdown could not stop fails the shutdown and the refused alloc * {@link startProviderPreparation} and the `heldDeletes` option. */ function startWebDriverHub( - options: Readonly<{ deleteFailure?: string; heldDeletes?: number }> = {}, + options: Readonly<{ + deleteFailure?: string; + heldDeletes?: number; + createFailure?: string; + }> = {}, ) { const created: string[] = []; const deleted: string[] = []; let deletesToHold = options.heldDeletes ?? 0; let deleteFailure = options.deleteFailure; const held: (() => void)[] = []; + /** Per-session arrival so a test can await the hub ending one rather than watching the clock. */ + const deleteSignals = new Map>(); + const signalForDelete = (sessionId: string) => { + const existing = deleteSignals.get(sessionId) ?? startSignal(); + deleteSignals.set(sessionId, existing); + return existing; + }; return { created, deleted, + /** Resolves once the hub has answered a `DELETE` for `sessionId`. */ + deletedArrived: async (sessionId: string) => { + await awaitSignal(signalForDelete(sessionId).arrived, `the hub to end session ${sessionId}`); + }, allowDeletes: () => { deleteFailure = undefined; }, @@ -561,6 +688,12 @@ function startWebDriverHub( const request = new Request(input as RequestInfo, init); const path = new URL(request.url).pathname; if (request.method === 'POST' && path.endsWith('/session')) { + if (options.createFailure) { + return jsonResponse( + { value: { error: 'unknown error', message: `${options.createFailure}` } }, + 500, + ); + } const sessionId = `session-${String(created.length + 1)}`; created.push(sessionId); return jsonResponse({ sessionId, capabilities: {} }); @@ -580,6 +713,7 @@ function startWebDriverHub( ); } deleted.push(sessionId); + signalForDelete(sessionId).report(); } return jsonResponse({ value: null }); }) as typeof fetch, @@ -588,6 +722,53 @@ function startWebDriverHub( type PrepareSession = NonNullable; +/** + * How long an arrival wait gives a step that has not happened at all. It is a watchdog, not a poll + * budget: the step that arrives resolves its own signal in that same step, however slow the worker is, + * so this only ever elapses on a manager or a seam that never reached the step. + */ +const ARRIVAL_BUDGET_MS = 2_000; + +/** + * Waits for a step to be reported — a create that reached the provider, the hub asked to delete a + * session, a plugin callback that was entered — rather than polling the clock for it. The timer only + * names a step that never came, so a slow worker cannot fail a test whose allocation is progressing. + */ +async function awaitSignal(signal: Promise, step: string): Promise { + let timer: ReturnType | undefined; + try { + const arrived = await Promise.race([ + signal.then(() => true), + new Promise((giveUp) => { + timer = setTimeout(() => giveUp(false), ARRIVAL_BUDGET_MS); + timer.unref?.(); + }), + ]); + assert.ok(arrived, `the manager never reached ${step}`); + } finally { + if (timer !== undefined) clearTimeout(timer); + } +} + +/** + * A step a test waits to be reported, reported once by the seam that reaches it. + */ +function startSignal() { + let reportArrival!: () => void; + const arrived = new Promise((ready) => { + reportArrival = ready; + }); + let reported = false; + return { + arrived, + report: () => { + if (reported) return; + reported = true; + reportArrival(); + }, + }; +} + /** * `prepareSession` is the step that runs before this manager sends `POST /session`, so holding it per * call keeps a create in flight across a release or a shutdown without racing one with a sleep. It @@ -598,6 +779,8 @@ type PrepareSession = NonNullable void }[] = []; + /** Who is waiting to hear that a create has reached the provider. */ + const waiting: (() => void)[] = []; const prepareSession: PrepareSession = async ({ base }) => { state.started += 1; const call = { released: false, release: () => {} }; @@ -609,6 +792,9 @@ function startProviderPreparation() { }; }); calls.push(call); + // This create has arrived, so anyone waiting for one hears it in this same step rather than on the + // next round of a poll. + for (const announce of waiting.splice(0)) announce(); await held; return { ...base, @@ -620,15 +806,20 @@ function startProviderPreparation() { }; return Object.assign(state, { prepareSession, - /** Resolves once `expected` allocations have reached the provider and are held there. */ + /** + * Resolves once `expected` allocations have reached the provider and are held there. Each wait is + * ended by the arrival itself, so a test neither polls the scheduler nor races the create; the one + * timer only reports a create that never arrived, and never elapses while the seam works. + */ waitFor: async (expected: number) => { - for (let attempt = 0; calls.length < expected && attempt < 200; attempt += 1) { - await schedulerYield(); + while (calls.length < expected) { + await awaitSignal( + new Promise((announce) => { + waiting.push(announce); + }), + `${String(expected)} allocation(s) to reach the provider (got ${String(calls.length)})`, + ); } - assert.ok( - calls.length >= expected, - `expected ${String(expected)} allocations to reach the provider, got ${String(calls.length)}`, - ); }, /** Lets every create that is held reach the hub. */ answerHeld: async () => { @@ -669,19 +860,6 @@ async function schedulerYield(): Promise { }); } -/** - * Waits for the manager to reach `condition` — the hub having been asked to delete a session, say — so a - * test can name the moment after the billed work is done and before an optional callback has answered. - */ -async function untilSettled(condition: () => boolean, withinMs = 2_000): Promise { - for (let waited = 0; !condition() && waited < withinMs; waited += 5) { - await new Promise((ready) => { - setTimeout(ready, 5); - }); - } - assert.ok(condition(), 'the manager never reached the step the test was waiting for'); -} - /** The allocation envelope names the hub session the lease now holds. */ function assertResolvedWith( outcome: Settled | undefined>, @@ -729,9 +907,14 @@ function assertRejectedWith( } if (expected.cleanup !== undefined) assert.equal(details.cleanup, expected.cleanup); if (expected.cleanupWarning !== undefined) { - const warnings = details.cleanupWarnings as Record; - assert.deepEqual(Object.keys(warnings ?? {}), [expected.cleanupWarning]); - assert.equal(typeof warnings[expected.cleanupWarning], 'string'); + // A refusal for one session reports its warnings keyed by code, which loses nothing because one + // session's warnings carry distinct codes; a shutdown reports every session it left billing as one + // entry per warning. Either way the caller is told which refusal it got. + const warnings = details.cleanupWarnings; + const codes = Array.isArray(warnings) + ? (warnings as Array>).map((warning) => warning.code) + : Object.keys((warnings ?? {}) as Record); + assert.deepEqual(codes, [expected.cleanupWarning]); } if (expected.flags !== undefined) assert.deepEqual(details.flags, expected.flags); } diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index 78117c087f..91dd5b05ec 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -499,7 +499,13 @@ export class WebDriverSessionManager { const failure = isWebDriverRequestTimeout(error) ? sessionCreateTimeoutError(error, this.options.provider, lease, prepared) : error; - await releaseOnFailure(failure, () => prepared.cleanup?.()); + // The hand-back is the plugin's own code, and no session exists yet for a release or a shutdown to + // end, so this is the one cleanup the allocation itself must not wait on forever. + await releaseOnFailure(failure, () => + this.providerCallback(`${this.options.provider} cleanup for lease ${lease.leaseId}`, () => + prepared.cleanup?.(), + ), + ); throw failure; } } @@ -773,7 +779,16 @@ function shutdownLeftSessionsRunning( provider, providerSessionIds, cleanup: 'uncertain', - cleanupWarnings: warningMessagesByCode(outcomes.flatMap((outcome) => outcome.warnings)), + // One entry per warning, each naming the session it is about: several sessions the provider would + // not give back carry several refusals, and keying them by code alone would report one reason for + // all of them. + cleanupWarnings: outcomes.flatMap((outcome) => + outcome.warnings.map((warning) => ({ + providerSessionId: outcome.session?.providerSessionId, + code: warning.code, + message: warning.message, + })), + ), hint: `${provider} did not stop these sessions and they keep billing. Stop the session ids above on ${provider}.`, }, ); diff --git a/packages/provider-webdriver/src/runtime.ts b/packages/provider-webdriver/src/runtime.ts index 66dbca8298..96b7f6e0d7 100644 --- a/packages/provider-webdriver/src/runtime.ts +++ b/packages/provider-webdriver/src/runtime.ts @@ -104,9 +104,10 @@ export type CloudWebDriverRuntimeOptions = { /** Profile fields this provider reads; any field it refuses fails lease allocation. */ profileFields: ProviderProfileFieldDeclaration; /** - * Budget for one provider callback the session manager waits on — a prepared session's `cleanup` and - * the artifact lookup — so a plugin callback that never answers cannot hold a lease open. Defaults to - * the budget one WebDriver round trip gets. + * Budget for one provider callback the session manager waits on — a prepared session's `cleanup`, on + * disposal and on a create that failed, and the artifact lookup — so a plugin callback that never + * answers cannot hold a lease open or delay a close. Defaults to the budget one WebDriver round trip + * gets. */ providerCallbackTimeoutMs?: number; }; From e094faebb3a6899a89b95410647fa9331ad33fa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sun, 1 Nov 2026 22:12:37 +0100 Subject: [PATCH 5/5] docs(webdriver): clarify callback and artifact wait boundaries --- .../provider-webdriver/src/runtime-session.ts | 20 +++++++++---------- packages/provider-webdriver/src/runtime.ts | 7 +++---- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/packages/provider-webdriver/src/runtime-session.ts b/packages/provider-webdriver/src/runtime-session.ts index 91dd5b05ec..8e91b83f9e 100644 --- a/packages/provider-webdriver/src/runtime-session.ts +++ b/packages/provider-webdriver/src/runtime-session.ts @@ -70,8 +70,8 @@ type WebDriverLeaseEntry = { * Whichever route took a lease away from its allocation, and the single disposal of what that entry * stood for. `settled` is what that disposal reported and stays missing while it is still running, so a * caller can tell a session that is gone from one whose outcome it cannot see yet. `artifacts` is the - * release envelope's report and is deliberately separate: it is optional, so it is never what a lease or - * a daemon close waits through. + * release envelope's report and is deliberately separate: releasing the lease slot and `shutdown()` + * await only disposal. A release also awaits artifacts, including when it joins a shutdown's take. */ type WebDriverLeaseTake = { by: 'release' | 'shutdown'; @@ -102,9 +102,8 @@ function warningMessagesByCode(warnings: CloudWebDriverReleaseWarning[]): Record } /** - * Budget for one provider callback this manager waits on. A plugin may supply its own `prepareSession` - * cleanup and artifact lookup, and those callbacks are the plugin's code, so this manager cannot assume - * they answer: they get the budget this package already gives one WebDriver round trip. + * Budget for prepared-session cleanup on disposal and failed creation, and for teardown artifact lookup. + * The preparation callback itself is not bounded by this budget. */ const DEFAULT_PROVIDER_CALLBACK_TIMEOUT_MS = 30_000; @@ -350,10 +349,9 @@ export class WebDriverSessionManager { const settle = (outcome: WebDriverDisposalOutcome): void => { take.settled = outcome; take.providerSessionId = outcome.session?.providerSessionId; - // The artifact report belongs to the release envelope and to nothing else: it starts here and the - // release joins it, while the lease and a daemon close are answered by `disposal` alone. A plugin's - // artifact lookup therefore cannot keep a lease from being allocated again, nor a close from - // finishing. + // The artifact report starts once session deletion and provider cleanup settle. The lease slot is + // freed and `shutdown()` answers without awaiting it. A release still joins the report, even when + // shutdown started this take. take.artifacts = outcome.session === undefined ? undefined @@ -381,8 +379,8 @@ export class WebDriverSessionManager { /** * Ends what one taken entry stands for: the session its create rented, once that create answers, or * nothing when the create billed nothing. This is the only place a session is ended, and one entry is - * taken by exactly one route, so a session is ended once. Everything here is bounded: the provider's own - * cleanup belongs to the plugin that supplied it, and this manager must still answer when it does not. + * taken by exactly one route, so a session is ended once. After creation answers, session deletion and + * provider cleanup are bounded. Creation still awaits `prepareSession` without a callback budget. */ private async disposeTakenEntry(entry: WebDriverLeaseEntry): Promise { const session = entry.session ?? (await entry.creation.catch(() => undefined)); diff --git a/packages/provider-webdriver/src/runtime.ts b/packages/provider-webdriver/src/runtime.ts index 96b7f6e0d7..601d54f1af 100644 --- a/packages/provider-webdriver/src/runtime.ts +++ b/packages/provider-webdriver/src/runtime.ts @@ -104,10 +104,9 @@ export type CloudWebDriverRuntimeOptions = { /** Profile fields this provider reads; any field it refuses fails lease allocation. */ profileFields: ProviderProfileFieldDeclaration; /** - * Budget for one provider callback the session manager waits on — a prepared session's `cleanup`, on - * disposal and on a create that failed, and the artifact lookup — so a plugin callback that never - * answers cannot hold a lease open or delay a close. Defaults to the budget one WebDriver round trip - * gets. + * Budget for a prepared session's `cleanup` on disposal and on a failed create, and for `listArtifacts` + * during teardown. Defaults to 30,000 ms. This budget does not bound `prepareSession`; release and + * shutdown still wait for preparation to finish. */ providerCallbackTimeoutMs?: number; };