fix(webdriver): give cloud session creation its own budget and stop leaking billed sessions

[thymikee]

Summary

Cloud WebDriver session creation ran under the generic per-request policy — timeoutMs ?? 30_000, retryAttempts ?? 1 — and lease_allocate carried the default 90s/reset-daemon envelope. On BrowserStack iOS real devices (session creation 45–90s) open aborted client-side at ~60s on most runs, and because POST /session is non-idempotent and completes server-side even after the client aborts, each failed open leaked two billed provider sessions (the abandoned create + the automatic retry). Closes #1774.

What changes (as of the reviewed head)

Session creation is its own phase — WebDriverClient.createSession runs under a dedicated sessionCreateTimeoutMs (default 180s), with retryAttempts: 0 and no cancellation signal: a retry is a second billed session, an abort loses the id of the first. An optional operation deadline can only shorten that budget.

lease_allocate gets a cloud-sized policy — a 300s LEASE_ALLOCATION_BUDGET_MS handed to providers as LeaseLifecycleContext.deadline, a matching 330s client envelope derived from it (so the two can't drift), and preserve-daemon for the whole lease route: resetting the daemon on a client timeout would SIGKILL it mid-create/mid-release and orphan every billed session it held.

The daemon owns what happens to a lease allocated for a requester that left. After allocate returns, if the request was canceled meanwhile (explicit cancel or client disconnect), the lease handler releases it — provider and registry — and answers with the canceled error. This is generic across providers and shares the one release path with lease_release (releaseLease() → { registryReleased, provider }). On the canceled error, released answers the operator's question — is the billed session gone? — and is true only when the provider released without warnings and without throwing; daemon bookkeeping is the separate registryReleased. A provider that could not delete its session (WEBDRIVER_SESSION_DELETE_FAILED), or whose release threw, is reported released: false with providerSessionId, the warnings/releaseError, and a stop-it-by-hand hint. (lease_release's existing wire field released keeps its registry meaning; provider cleanup rides in provider there, as before.)

Provider-side ownership of billed resources on failure — the AWS Device Farm remote-access ARN is stopped on any failure short of RUNNING (startup timeout, allocation deadline, cancellation); previously a startup timeout left it RUNNING and billing. A create the transport gives up on raises a typed provider_session_create_timeout error naming the lease so an operator can find the maybe-orphaned session, rather than heuristic REST cleanup of a session this process never owned. Shared releaseOnFailure covers both cleanups.

Typed cancellation, everywhere — the canceled-request error had nine hand-rolled copies; it is now one kernel definition (createRequestCanceledError / isRequestCanceledError), and markRequestCanceled aborts the request signal with that error as its reason, so every signal.throwIfAborted(), aborted fetch, and throw signal.reason in the daemon surfaces request_canceled instead of a bare DOMException → UNKNOWN. The transport's own deadline is likewise a typed webdriver_request_timeout, distinct from a caller cancel.

Also: BrowserStack's pre-session local app upload honors the request signal (an upload isn't billed, so plain abort is right there).

Validation

Every behavioral pin was proven red-then-green (production change reverted locally, test watched fail, restored):

• webdriver-transport.test.ts — deadline abort is a typed timeout; caller abort is not reclassified.
• webdriver-client.test.ts — createSession does not retry a transient create; waits past the 30s per-request default; is bounded by a shorter deadline.
• runtime-session.test.ts — create-timeout surfaces provider+lease evidence; primary failure survives a failing cleanup.
• aws-device-farm.test.ts — startup timeout / cancellation / deadline each stop the ARN; RUNNING is handed on unstopped.
• request-handler-catalog.test.ts — allocation hands the provider the request-bound signal (abort it, watch it flip) and a deadline; a lease allocated for a gone requester is released and not registered; a failed DELETE and a throwing release are each reported released: false / registryReleased: true with recovery evidence (both proven red on registry-only semantics).
• cancel.test.ts — factory keeps evidence and never loses its reason; a canceled request's signal carries the typed error.
• timeout-policy.test.ts reviewed-set pins updated; the integration test that pinned session-create retry was flipped to pin zero-retry.

Gates: pnpm check:affected --run green (815 files), typecheck, lint, fallow, layering, daemon-wire-compat (protocol unchanged), full provider-integration. The mutation lane reports "nothing to mutate" because its baseline is gating: false (pre-graduation), not a skip.

Live cloud validation (real devices, this branch's daemon):

• BrowserStack — Google Pixel 8 / Android 14, app-release.apk: open in 18s, 65-node snapshot, close, disconnect; REST API shows the session done / CLIENT_STOPPED_SESSION and zero running sessions afterward. (A first attempt with a wrong package name failed at activateApp after a successful create — and was still released cleanly, no leak.) The account has no iOS app uploaded, so the iOS path was validated on AWS instead.
• AWS Device Farm — iPhone 16 / iOS 18.0, Safari: open in 112s (would have died at the old 90s lease_allocate envelope), 84-node snapshot, close, disconnect → session STOPPING. The first attempt exposed that the AWS startup wait still capped at its standalone 120s default while the daemon's 300s budget had room; the new ARN-ownership guard correctly stopped that session (COMPLETED / STOPPED — before this PR it would have sat RUNNING and billing), and 7f9d1481a makes the allocation deadline the bound when present.

Smoke (wait_capture_stalled): the runner files in this diff are import-only switches to the kernel canceled-error and no runner code inspects the abort reason; re-running on the current head.

Scope

43 files across 10 commits. ~15 files are the fix proper (WebDriver provider package, descriptor timeout policy, lease handler, contract); the rest is the mechanical import switch for the canceled-error consolidation. LeaseLifecycleContext gained optional signal/deadline; other lifecycle implementers (limrun) ignore them. Two fallow clone-group findings in maestro/shared.ts are pre-existing (they entered scope because a function was deleted from that file) and are excluded by the audit gate as inherited.

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	2.28 MB	2.28 MB	-624 B
JS gzip	749.3 kB	749.0 kB	-275 B
npm tarball	871.2 kB	871.1 kB	-68 B
npm unpacked	3.04 MB	3.04 MB	-586 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	29.3 ms	29.8 ms	+0.5 ms
CLI --help	69.9 ms	69.9 ms	-0.0 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/src2.js	+1.5 kB	+542 B
dist/src/snapshot.js	-1.1 kB	-428 B
dist/src/runner-disposal.js	+1.1 kB	+402 B
dist/src/internal/daemon.js	-1.1 kB	-387 B
dist/src/sdk-batch-runner.js	+94 B	+53 B

[thymikee]

Cancellation cleanup reports a session as released even when WebDriver DELETE fails. releaseCanceledSession() always adds releasedWebDriverSessionId / releasedProviderSessionId, while closeSession() can return WEBDRIVER_SESSION_DELETE_FAILED; requestCanceledError() then still tells the operator the session was released. In that branch the billed session may remain running, and the success-shaped fields/hint hide the exact recovery action this fix is meant to preserve. Please distinguish successful deletion from attempted deletion, surface the provider/lease/session identifiers with a stop-session hint on failure, and add a DELETE-failure regression.\n\nSeparately, the PR body appears stale: the exact head is one commit and 15 changed files, rather than the claimed 32-file second-commit cancellation-consolidation change. Please update the description so it matches the reviewed head.

[thymikee]

Re-review at 8bfbc01c: the prior billed-session cleanup finding remains unresolved. releaseCanceledSession() still always reports releasedWebDriverSessionId / releasedProviderSessionId and uses a hint saying the session “was released,” while closeSession() catches a failed DELETE /session as WEBDRIVER_SESSION_DELETE_FAILED and returns normally. In that path the billed session may still be running, so the response is materially false and conceals required operator cleanup. The current regression covers successful DELETE only. Please make release evidence conditional on confirmed DELETE, provide explicit operator-recovery evidence/hint on DELETE failure, and add the failing-DELETE regression.\n\nReadiness is also blocked by the exact-head iOS Smoke failure (wait_capture_stalled, zero readable captures). It looks plausibly environmental, but needs a rerun or a same-load main comparison before classification. Finally, the PR body says 32 files touched; exact origin/main..8bfbc01c is 43 files (1,056 insertions / 198 deletions) across five commits. Update the scope summary. No ready-for-human label yet. Residual risk: no live BrowserStack iOS validation.

[thymikee]

Addressed in dd20855cd (pushing behind the local gate; the box is under heavy external load right now so the chained gate is re-rolling).

Release evidence was success-shaped even when DELETE failed — fixed at the root, not patched. The reason releaseCanceledSession() could lie is that the provider was doing the daemon's job: it treated the request signal as "ownership evidence, not an interrupt" and needed three paragraphs of comments to say so. That logic now lives where the request is owned. After allocate returns, the lease handler checks isRequestCanceled(requestId) and, if the requester left, releases the lease — provider and registry — via releaseAllocationForGoneRequester(). It's generic across providers (limrun gets it for free) and it consumes release()'s existing return shape, which already reports WEBDRIVER_SESSION_DELETE_FAILED as warnings.

Release evidence is now claimed only on a clean release (no warnings, no throw). A failed DELETE yields released: false, providerSessionId, the warnings, and a hint that says the session could NOT be confirmed released and must be stopped by hand — never "was released". Both cases are pinned in request-handler-catalog.test.ts (a lease allocated for a requester that left is released, not registered / a failed provider release after cancellation is reported as unreleased with recovery evidence), each proven red without the post-allocate check.

Side effect: WebDriverSessionManager is back to plain "create with a budget; on failure clean up" — the createOwnedSession/releaseCanceledSession trio and the ownership-semantics comments on the contract, client, registry and AWS prepare are gone. LeaseLifecycleContext.signal is just cancellation, like everywhere else.

PR body rewritten to describe the reviewed head as a whole (43 files, +1052/−205, 6 commits) instead of a per-commit history.

Smoke wait_capture_stalled: the runner files in the diff are import-only switches to the kernel canceled-error and no runner code inspects the abort reason; I'll rerun the lane on the new head. Residual risk unchanged: no live BrowserStack iOS run was possible from here.

[thymikee]

Re-review at dd20855c: the prior billed-session false-success defect is fixed. The daemon now owns cancellation cleanup, reports released: false when provider release returns WEBDRIVER_SESSION_DELETE_FAILED, and includes provider-session/warning/manual-recovery evidence. The regression at the daemon/provider seam is valid: it exercises that warning path and would fail before the post-allocation cancellation cleanup.

One cleanup remains: packages/maestro/src/internal/shared.ts now contains only import {} from "@agent-device/kernel/errors";. Please remove this introduced dead file content.

Readiness still requires rerunning/classifying red CI. iOS Smoke and Swift Runner Unit Compile failed while downloading pnpm/action-setup from GitHub codeload (429), before project code ran. Coverage hit an unrelated ENOTEMPTY cleanup/worker crash and produced no lcov. Residual risk: no live BrowserStack iOS validation.

[thymikee]

Re-review at 7f9d1481: the prior cancellation-release correctness fix remains sound, and the AWS iPhone live run is useful evidence. However, this commit changes waitForRunningRemoteAccessSession() from min(startupTimeoutMs, req.deadline) to using the allocation deadline exclusively—the behavior that permits a startup past the old 120s bound—without changing a test. The existing 40ms-shorter-deadline case passes before and after, so it cannot prove this regression. Please add a deterministic fake-clock/injectable-time case that crosses the old 120s bound and fails on the previous Math.min logic.

The previously noted cleanup is also still present: packages/maestro/src/internal/shared.ts contains only import {} from "@agent-device/kernel/errors";; remove it.

Exact-head readiness remains blocked pending rerun/classification: Swift Runner, FreeRange, and CodeQL JS failed while downloading actions (codeload 429/503); Coverage ended in a worker/unexplained crash. iOS Smoke is still pending. No label.

[thymikee]

Code-quality review (strict pass, head 7f9d1481a)

Overall: this is structurally sound. The canceled-error consolidation is a genuine deletion (nine copies → one kernel definition, and the signal carries the typed reason so call sites can't forget it), the client envelope is derived from LEASE_ALLOCATION_BUDGET_MS instead of being a second magic number, and no file crosses 1k lines. The findings below are about the PR not finishing its own dedupe and one place where the lease handler grew two definitions of the same concept.

1. stopRemoteAccessSessionAfterFailure is a verbatim copy of releaseOnFailure — the dedupe commit never touched the AWS file

8bfbc01c3 says releaseOnFailure "replaces the two identical … helpers (WebDriver session + AWS remote-access ARN)" and the PR body repeats "Shared releaseOnFailure covers both cleanups", but packages/provider-webdriver/src/aws-device-farm.ts:330-345 still carries its own try/catch/cleanupError-on-details helper, byte-for-byte the same shape as webdriver-utils.ts:releaseOnFailure. Line 234 should just be:

await releaseOnFailure(error, () => options.client.stopRemoteAccessSession(remoteAccess.arn));

and the private helper deleted. As-is the PR ships a near-duplicate of a helper it introduced in the same diff.

2. Leftover empty import

packages/maestro/src/internal/shared.ts:1 is now import {} from '@agent-device/kernel/errors'; — dead line left by the mechanical switch. Delete it.

3. errorMessage — the PR adds two more copies of a helper that already exists ~6 times

src/daemon/handlers/lease.ts:187 and packages/provider-webdriver/src/webdriver-utils.ts:6 both add error instanceof Error ? error.message : String(error); the repo already has the same one-liner in application-lifecycle-recovery.ts, durable-capture-resource-*.ts (×3), request-runtime-binding.ts. This PR is already editing packages/kernel/src/errors.ts — that's the canonical home. Export it once from the kernel and have the two files this PR touches import it (sweeping the pre-existing copies can be a follow-up, but let's not add to the count).

4. lease.ts now has two definitions of "released" — fold the gone-requester path onto the release path

The gone-requester branch grew four helpers plus an outcome type (releaseAllocationForGoneRequester, releaseProviderLease, ProviderReleaseOutcome, canceledAllocationError, ~75 lines) to do what the lease_release case a few lines up already does: provider release → registry release → report. But the two paths now disagree on what release evidence means:

• lease_release reports released from the registry result and passes provider warnings through untouched;
• the gone-requester path derives released from warnings.length === 0 && no throw and swallows a throwing release() into releaseError.

Both are defensible individually, but they live 60 lines apart in one handler with different semantics for the same word. Suggested judo: one releaseLease(lease, ctx): Promise<{ released: boolean; providerSessionId?; warnings }> used by both the lease_release case and the gone-requester branch (the branch wraps it in a try that turns a throw into releaseError, since nobody is there to receive it). Then the branch is a single throw createRequestCanceledError({...outcome, hint}), releaseRegistryLease + releaseProviderLease collapse into it, and there's exactly one answer to "did the daemon release this lease". Also drops the standalone errorMessage copy here (see #3).

5. (minor) sessionCreateTimeoutMs sits in the transport's policy type but is consumed by the client

WebDriverRequestPolicy.sessionCreateTimeoutMs forced the new ResolvedWebDriverRequestPolicy = Required<Pick<…, 'timeoutMs'|'retryAttempts'|'retryDelayMs'>> — the Pick exists only to carve the client-owned field back out. Keeping it in requestPolicy is fine as the single user-facing knob, but the doc comment saying "consumed by WebDriverClient rather than the transport" is the type telling you it's in the wrong bag. Not blocking; if it stays, at least the transport type shouldn't need to know the field exists (Omit it at the WebDriverTransportOptions boundary rather than Pick internally).

Verdict

Request changes for #1 and #2 (both trivial; #1 is the PR contradicting its own commit message). #3/#4 are the ones I'd actually push on — the lease handler is the piece a maintainer will re-read next time a provider misbehaves, and right now it has to be read twice. Everything else (typed transport timeout vs caller cancel, budgetWithin, ARN ownership through startup, preserve-daemon on the lease route) reads as inevitable in hindsight — good.

[thymikee]

All five addressed at 3665ea06b (plus 273870099 for the deadline test).

1. AWS copy of releaseOnFailure — real, and my fault twice over: the dedupe script asserted on the runtime-session edit and aborted before reaching the AWS file, and I mis-verified. aws-device-farm.ts:234 is now await releaseOnFailure(error, () => options.client.stopRemoteAccessSession(remoteAccess.arn)); the private helper is gone.
2. Empty import in maestro/shared.ts removed (273870099).
3. errorMessage now lives once in @agent-device/kernel/errors; lease.ts and webdriver-utils.ts import it (the two copies this PR had added are gone). Sweeping the ~6 pre-existing copies is a follow-up, as you suggested.
4. One release path in lease.ts. releaseLease(registry, provider, lease, request, ctx) → { released, provider } is used by both the lease_release case (wire shape unchanged: released = registry, provider data passed through) and the gone-requester branch, which wraps it in a try that folds a throwing release() into releaseError. So released means exactly one thing in both places — the daemon's lease record is gone — and the provider verdict is a separate providerReleased (no warnings, no throw) that drives the stop-by-hand hint. releaseRegistryLease + releaseProviderLease + the outcome type collapsed into it; net −35 lines in the handler. Both regressions updated (released: true in both; providerReleased carries the DELETE-failure verdict).
5. sessionCreateTimeoutMs is now Omit-ed at the WebDriverTransportOptions boundary; the internal Pick is gone (ResolvedWebDriverRequestPolicy = Required<NonNullable<WebDriverTransportOptions['requestPolicy']>>).

Also from the 16:23 review: the deterministic virtual-clock test (Date.now advanced 10s/poll, RUNNING at 150s, deadline 300s) that fails on the old Math.min logic and passes now — aws-device-farm.test.ts, proven red/green.

CI on 273870099 came back green for everything that had failed on action downloads (Swift Runner Unit Compile etc.); Coverage/Smoke were still pending at push time and are re-running on 3665ea06b.

[thymikee]

Re-review at 3665ea06: the lease refactor reintroduces the release-reporting ambiguity. On a provider DELETE warning, canceledAllocationError() now returns released: true because the daemon registry record was removed, alongside providerReleased: false. The cancellation error is still success-shaped even though the billed provider session may be running; the regression was changed to bless that combination. Keep released false or absent unless provider cleanup is confirmed, and expose daemon bookkeeping under an unambiguous separate key such as registryReleased. The failed-DELETE regression should enforce that distinction. This also conflicts with the current body, which says failed DELETE yields released:false. Resolved: AWS virtual-time regression and empty Maestro import. CI is green except iOS Smoke pending. Scope is 43 files / 9 commits, not 7.

[thymikee]

You're right — in unifying the release path I traded one ambiguity for another and then blessed it in the test. Fixed at the source in b96f4f93b:

• LeaseReleaseOutcome names its bookkeeping field registryReleased; there is no field called released in the shared outcome anymore, so neither caller can pick up the wrong meaning by accident.
• On the canceled error, released is the operator's answer — is the billed session gone? It is true only when registryReleased and the provider released without warnings and without throwing. Daemon bookkeeping is exposed as the separate registryReleased. The stop-by-hand hint keys on released.
• lease_release's existing wire field keeps its registry meaning (provider cleanup rides in provider, as before) — that contract is unchanged.
• Regressions now enforce the distinction rather than bless the old combination: a failed DELETE and a throwing release each pin released: false / registryReleased: true with providerSessionId, warnings/releaseError, and the recovery hint. Both were proven red against registry-only semantics before restoring.

PR body corrected to match (the "failed DELETE yields released: false" statement is true again, and it now says which key carries what), scope updated to 43 files / 10 commits.

[thymikee]

Clean re-review at b96f4f93: registryReleased now separates daemon bookkeeping from the operator-facing released result. Failed DELETE and throwing provider-release regressions correctly require released: false plus registryReleased: true and recovery evidence; the prior ambiguity is resolved. The body matches the exact 43-file / 10-commit scope. Exact-head CI and iOS Smoke are green except CodeQL Java/Kotlin, whose analysis completed but SARIF/status upload hit a GitHub 503; CodeQL Python remains queued. Please rerun/classify those external states before applying a ready label. Residual risk: no live BrowserStack iOS validation.

[thymikee]

Classified and retriggered.

CodeQL run 32051017472 on b96f4f93b — external, twice over:

• Analyze (java-kotlin): analysis completed; the SARIF upload step got GitHub's 503 ("No server is currently available to service your request"). Nothing repo-side.
• Analyze (python): sat queued for 40+ min with no runner, during GitHub's "intermittent failures in runner group" incident. That left the run in an inconsistent state — status: completed overall with a job still queued — so it could be neither cancelled nor rerun (gh run rerun --failed → "cannot be retried"; gh run cancel → "already completed"). Default-setup CodeQL has no dispatchable workflow, so the only way to a fresh result was a new push.

002994038 is an empty commit (no source change) pushed behind the usual gate to retrigger. Fresh CodeQL run 32123368232 is now in progress on the new head, and the runner backlog appears to have drained (all lanes picking up). Head is otherwise unchanged from the b96f4f93b you re-reviewed.

[thymikee]

Fresh exact-head CI on 0029940 exposed one actionable missed assertion: packages/provider-webdriver/src/webdriver-client.test.ts:324 still requires TimeoutError, while this PR intentionally normalizes the shared-budget timeout to AppError with details.reason=webdriver_request_timeout and timeoutMs=118. Update the regression to assert the structured timeout contract and bounded budget, then rerun Coverage. The retrigger commit is byte-for-byte empty from reviewed b96f4f9; iOS Smoke remains pending. Not ready-for-human.

[thymikee]

Fixed — and it's a merge-ref interaction, not a miss in the branch's own tree: main's #1790 ("assert the specific error code instead of any failure") tightened webdriver-client.test.ts to require the raw TimeoutError, which is exactly what this PR normalizes into AppError{reason: webdriver_request_timeout}. The two met on the Coverage merge ref.

Rebased onto current main (clean, no conflicts) and updated that regression to the structured contract: details.reason === 'webdriver_request_timeout' and details.timeoutMs < budgetMs − firstCallMs/2 (i.e. the second request was handed the shared remainder, ~118 of 200). Also picked up main's .claude/** format ignore, so the corpus-YAML sweep noise from my worktree is gone. Local gate green (825 files); force-with-lease pushed. Coverage is re-running on the new head.

[thymikee]

Re-reviewed exact head ba79c23: the prior Coverage blocker is correctly fixed. The activeElement regression now requires the typed webdriver_request_timeout AppError and proves the second request receives only the shared remaining budget; reverting either timeout normalization or shared budgeting makes it fail. No new code or architecture finding, and the existing cloud-device evidence remains applicable. Code review is clean, but final G2G waits for Coverage and the remaining exact-head CI/smoke lanes currently in progress.

[thymikee]

Android Smoke on ba79c23f classified as environmental and re-run green (same head, no change).

The failure was assertPersistentAndroidHelper … helperSessionReused: false !== true on the catalog snapshot in smoke-android-emulator — the persistent helper session was recreated between two snapshots (reused = session.capturedCount > 0). Evidence it's the lane, not the diff:

• No Android/helper/smoke file is in this branch's diff; the only Android-adjacent change (request signals now abort with the typed canceled error) is only ever propagated by the helper protocol (reject(signal?.reason ?? …)), never branched on.
• That run's emulator boot was unhealthy before the test began: Unable to connect to adb daemon on port 5037 and 10× adb … failed with exit code 1.
• The same lane failed on main at 12:12 (2b6d04a13, adb-daemon signature) and passed at 12:33 — flaky today independent of this PR.
• Rerun of the failed job on the identical head: success.

iOS ×2 and Web smoke were already green on this head.

[thymikee]

iOS Smoke on ba79c23f classified as environmental and re-run green (same head, no change).

The failure was assertFormInput … typed email suffix should be observable — the field held ale.test instead of ada@example.test, i.e. dropped keystrokes on the CI simulator during type (the #1676 synthesized-typing commit-lag class). Evidence it's the lane, not the diff:

• No iOS typing / runner-transport code is in this branch (the Apple runner files here are import-only switches to the kernel canceled-error).
• main's iOS Smoke failed the same lane at 12:33 today (3908559fe, wait for Agent Device Tester step) — flaky today independent of this PR.
• Rerun of the failed job on the identical head: success.

With that, every check on ba79c23f is green.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-18 13:49 UTC