From a89205f275e908bdf660df50eb51f9d74a6b2426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 18 Sep 2026 10:34:29 +0200 Subject: [PATCH 1/7] fix(cli): name the flag an unknown guess was reaching for --output and --path are the spellings an agent reaches for before reading the synopsis, and the refusal only said "Unknown flag". Suggestions now come from the live flag registry filtered by what the command in scope accepts, so a guess is never answered with a flag that command would refuse: "trace start --path" keeps the bare refusal, while "screenshot --output" is pointed at --out. --- .../__tests__/command-suggestions.test.ts | 70 +++++++++- src/cli/parser/args.ts | 2 +- src/cli/parser/command-suggestions.ts | 122 +++++++++++++++--- 3 files changed, 174 insertions(+), 20 deletions(-) diff --git a/src/cli/parser/__tests__/command-suggestions.test.ts b/src/cli/parser/__tests__/command-suggestions.test.ts index ee1a4777c2..9c0191b914 100644 --- a/src/cli/parser/__tests__/command-suggestions.test.ts +++ b/src/cli/parser/__tests__/command-suggestions.test.ts @@ -1,11 +1,18 @@ import assert from 'node:assert/strict'; import { test } from 'vitest'; -import { isKnownCliCommandName } from '@agent-device/command-registry/catalog'; +import { isKnownCliCommandName, listCliCommandNames } from '@agent-device/command-registry/catalog'; import { keyboardCliReader } from '../../../commands/system/index.ts'; import { AppError } from '@agent-device/kernel/errors'; import { parseArgs } from '../args.ts'; import type { CliFlags } from '@agent-device/contracts/command'; -import { listCommandAliasSuggestionEntries, suggestCommandFor } from '../command-suggestions.ts'; +import { getFlagDefinitions } from '../../../cli-schema/command-schema.ts'; +import { isFlagSupportedForCommand } from '../../../cli-schema/option-schema.ts'; +import { + listCommandAliasSuggestionEntries, + listFlagNameSuggestionEntries, + suggestCommandFor, + suggestFlagFor, +} from '../command-suggestions.ts'; // Guards against the curated alias map drifting to a command that no longer // exists (renamed, removed, gated) in the live command registry. @@ -222,3 +229,62 @@ test('unrelated unknown flags are unaffected', () => { error.message === 'Unknown flag: --not-a-real-flag', ); }); + +// Guards against the curated flag map drifting to a flag that no longer exists, or to +// one no command accepts — a suggestion the parser would then refuse itself. +test('every curated flag suggestion names a registered flag at least one command accepts', () => { + const commands = listCliCommandNames(); + for (const [guess, canonical] of listFlagNameSuggestionEntries()) { + const definition = getFlagDefinitions().find((entry) => entry.names.includes(canonical)); + assert.ok(definition, `flag suggestion for "${guess}" names unregistered flag "${canonical}"`); + assert.ok( + definition && commands.some((command) => isFlagSupportedForCommand(definition.key, command)), + `flag suggestion "${canonical}" for "${guess}" is accepted by no command`, + ); + } +}); + +for (const guess of ['--output', '--path', '--Output']) { + test(`a guessed output-path flag on screenshot names ${guess} -> --out`, () => { + assert.throws( + () => parseArgs(['screenshot', guess, './shot.png']), + (error: unknown) => + error instanceof AppError && + error.code === 'INVALID_ARGS' && + error.message === `Unknown flag: ${guess}. Did you mean --out?`, + ); + }); +} + +// The closest negative: `--path` means nothing to `trace`, whose path is positional and +// which accepts no `--out` at all. A suggestion map applied without the per-command gate +// would push a flag this command refuses. +test('a guessed flag the command does not accept is not pushed at it', () => { + assert.throws( + () => parseArgs(['trace', 'start', '--path', './trace.log']), + (error: unknown) => + error instanceof AppError && + error.code === 'INVALID_ARGS' && + error.message === 'Unknown flag: --path', + ); +}); + +// The registry-derived fallback, not the curated map: `--pixel-densityy` is one edit from +// a flag screenshot accepts, and the candidate list comes from that command's own surface. +test('a mistyped flag the command accepts is suggested by nearest name', () => { + assert.throws( + () => parseArgs(['screenshot', '--pixel-densityy', '2']), + (error: unknown) => + error instanceof AppError && + error.code === 'INVALID_ARGS' && + error.message === 'Unknown flag: --pixel-densityy. Did you mean --pixel-density?', + ); +}); + +test('suggestFlagFor never throws for arbitrary input', () => { + for (const input of ['', '-', '--', '@#$%', '--a'.repeat(200), '--JSON']) { + for (const command of ['screenshot', 'open', null]) { + assert.doesNotThrow(() => suggestFlagFor(input, command)); + } + } +}); diff --git a/src/cli/parser/args.ts b/src/cli/parser/args.ts index c90e303f36..93fc29755a 100644 --- a/src/cli/parser/args.ts +++ b/src/cli/parser/args.ts @@ -101,7 +101,7 @@ export function parseRawArgs(argv: string[]): RawParsedArgs { else positionals.push(arg); continue; } - throw new AppError('INVALID_ARGS', formatUnknownFlagMessage(token)); + throw new AppError('INVALID_ARGS', formatUnknownFlagMessage(token, command)); } const parsed = parseFlagValue(definition, token, inlineValue, argv[i + 1]); diff --git a/src/cli/parser/command-suggestions.ts b/src/cli/parser/command-suggestions.ts index c34c99e327..5acb916df1 100644 --- a/src/cli/parser/command-suggestions.ts +++ b/src/cli/parser/command-suggestions.ts @@ -1,5 +1,7 @@ import { RETIRED_SCREENSHOT_MAX_SIZE } from '@agent-device/contracts/capture'; import { listCliCommandNames } from '@agent-device/command-registry/catalog'; +import { getFlagDefinitions } from '../../cli-schema/command-schema.ts'; +import { isFlagSupportedForCommand } from '../../cli-schema/option-schema.ts'; /** * Curated guess -> canonical command mapping for unknown CLI command names. @@ -51,7 +53,7 @@ export function listCommandAliasSuggestionEntries(): Array<[string, CommandAlias return Object.entries(COMMAND_ALIAS_SUGGESTIONS); } -const NEAREST_COMMAND_SUGGESTION_LIMIT = 3; +const NEAREST_SUGGESTION_LIMIT = 3; /** * Nearest registered command names for an unrecognized (lowercased) command @@ -68,30 +70,40 @@ function getNearestCommandNames(command: string): string[] { const names = listCliCommandNames(); const prefixMatches = names.filter((name) => name.startsWith(command)); if (prefixMatches.length > 0) { - return prefixMatches - .sort((a, b) => a.length - b.length || a.localeCompare(b)) - .slice(0, NEAREST_COMMAND_SUGGESTION_LIMIT); + return prefixMatches.sort(shortestThenAlpha).slice(0, NEAREST_SUGGESTION_LIMIT); } - const threshold = nearestMatchThreshold(command); - const scored = names - .map((name) => ({ name, distance: commandNameDistance(command, name) })) - .filter((entry) => entry.distance <= threshold); + return getClosestNames(command, names); +} + +function shortestThenAlpha(a: string, b: string): number { + return a.length - b.length || a.localeCompare(b); +} + +/** + * The shared nearest-name rule: keep only the closest candidates within a + * length-derived threshold, so a strong match is never bundled with a + * coincidental weak one. + */ +function getClosestNames(token: string, candidates: readonly string[]): string[] { + const scored = candidates + .map((name) => ({ name, distance: nameDistance(token, name) })) + .filter((entry) => entry.distance <= nearestMatchThreshold(token)); if (scored.length === 0) return []; const best = Math.min(...scored.map((entry) => entry.distance)); return scored .filter((entry) => entry.distance === best) .sort((a, b) => a.name.localeCompare(b.name)) - .slice(0, NEAREST_COMMAND_SUGGESTION_LIMIT) + .slice(0, NEAREST_SUGGESTION_LIMIT) .map((entry) => entry.name); } -function nearestMatchThreshold(command: string): number { - if (command.length < 4) return 1; - if (command.length <= 6) return 2; +function nearestMatchThreshold(token: string): number { + if (token.length < 4) return 1; + if (token.length <= 6) return 2; return 3; } -function commandNameDistance(a: string, b: string): number { +function nameDistance(a: string, b: string): number { if (a === b) return 0; if (a.startsWith(b) || b.startsWith(a)) { return Math.abs(a.length - b.length); @@ -156,15 +168,91 @@ const POSITIONAL_APP_FLAG_GUESSES = new Set([ // "Unknown flag" error. const REMOVED_SESSION_LOCK_ALIASES = new Set(['--session-locked', '--session-lock-conflicts']); -export function formatUnknownFlagMessage(token: string): string { - if (POSITIONAL_APP_FLAG_GUESSES.has(token.toLowerCase())) { +/** + * Curated guess -> canonical flag name for unknown CLI flag names. + * + * Agents reach for the conventional spellings of an output path before reading the + * synopsis, and the nearest-name fallback below cannot bridge `--path` to `--out` at + * edit distance 4. An entry is offered only when the command in scope accepts the + * named flag, so a guess that means something else there — `trace --path`, whose + * path is positional and `--out`-less — falls through instead of being pushed at it. + * Keys must be lowercase and are matched case-insensitively. + */ +const FLAG_NAME_SUGGESTIONS: Record = { + '--output': '--out', + '--path': '--out', +}; + +/** + * @internal Exposes the curated flag-name map for drift tests. + */ +export function listFlagNameSuggestionEntries(): Array<[string, string]> { + return Object.entries(FLAG_NAME_SUGGESTIONS); +} + +/** + * Builds the "Did you mean ...?" fragment for an unknown flag under the command + * in scope, or `undefined` when neither the curated guess map nor the nearest-name + * fallback has a confident suggestion. Candidates come from the live flag registry + * filtered by what that command actually accepts, so the suggestion cannot drift + * from the accepted surface and never names a flag the command would refuse. + */ +export function suggestFlagFor(token: string, command: string | null): string | undefined { + const normalized = token.toLowerCase(); + const curated = FLAG_NAME_SUGGESTIONS[normalized]; + if (curated && isFlagTokenSupportedForCommand(curated, command)) return curated; + const nearest = getNearestFlagNames(normalized, command); + if (nearest.length === 0) return undefined; + if (nearest.length === 1) return nearest[0]; + return `one of: ${nearest.join(', ')}`; +} + +function isFlagTokenSupportedForCommand(token: string, command: string | null): boolean { + const definition = getFlagDefinitions().find((candidate) => candidate.names.includes(token)); + return definition ? isFlagSupportedForCommand(definition.key, command) : false; +} + +function getNearestFlagNames(token: string, command: string | null): string[] { + const bareToken = stripFlagDashes(token); + if (bareToken.length <= 2) return []; + const names = listSupportedFlagNames(command); + const prefixMatches = names.filter((name) => stripFlagDashes(name).startsWith(bareToken)); + if (prefixMatches.length > 0) { + return prefixMatches.sort(shortestThenAlpha).slice(0, NEAREST_SUGGESTION_LIMIT); + } + const closest = getClosestNames( + bareToken, + names.map((name) => stripFlagDashes(name)), + ); + return closest.map((name) => `--${name}`); +} + +function listSupportedFlagNames(command: string | null): string[] { + const names = new Set(); + for (const definition of getFlagDefinitions()) { + if (!isFlagSupportedForCommand(definition.key, command)) continue; + const primary = definition.names[0]; + if (primary) names.add(primary); + } + return [...names]; +} + +function stripFlagDashes(token: string): string { + return token.replace(/^--?/, ''); +} + +export function formatUnknownFlagMessage(token: string, command: string | null): string { + const normalized = token.toLowerCase(); + if (POSITIONAL_APP_FLAG_GUESSES.has(normalized)) { return `Unknown flag: ${token}. The app or bundle id is a positional argument, e.g. ${OPEN_RELAUNCH_EXAMPLE}.`; } - if (REMOVED_SESSION_LOCK_ALIASES.has(token.toLowerCase())) { + if (REMOVED_SESSION_LOCK_ALIASES.has(normalized)) { return `Unknown flag: ${token}. Use --session-lock reject|strip instead.`; } - if (token.toLowerCase() === RETIRED_SCREENSHOT_MAX_SIZE.cliToken) { + if (normalized === RETIRED_SCREENSHOT_MAX_SIZE.cliToken) { return `Unknown flag: ${token}. ${RETIRED_SCREENSHOT_MAX_SIZE.migration.screenshot}. ${RETIRED_SCREENSHOT_MAX_SIZE.migration.record}.`; } + const suggestion = suggestFlagFor(token, command); + if (suggestion) return `Unknown flag: ${token}. Did you mean ${suggestion}?`; return `Unknown flag: ${token}`; } From c663a7c45189f2dc25c7813ac323c1393652ead3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 18 Sep 2026 10:34:36 +0200 Subject: [PATCH 2/7] feat(cli): open --wait queues for a device another session holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A device another workspace's session is holding refused at once, leaving the caller to poll by hand, and the foreign-workspace refusal named a session its `--session` value could not reach. `open --wait ` spends a budget on that contention before the request takes the device's execution lock, which is the whole design: `close`, `record stop` and every other operation that could free the device need the same lock, so an open that waited while holding it would have blocked its own recovery. Verified on a live emulator — a waiting open now lets a competing `close` return in 0.1s and opens the device itself once the holder is gone; the first attempt at a daemon-side wait inside the lock stalled that close for the full budget. The refusal that ends a spent budget names the owning session address, says the wait happened and does not offer it again, while a refusal from a command that cannot wait never mentions the flag. The budget extends the command's timeout envelope rather than eating into it, so a long wait cannot end in a client-side daemon reset. --- .../src/flag-definitions-workflow.ts | 14 ++ .../command-registry/src/timeout-policy.ts | 25 ++- packages/contracts/src/cli-flags.ts | 1 + packages/contracts/src/client-app.ts | 8 + scripts/integration-progress-model.ts | 8 + .../command-descriptor-timeout-policy.test.ts | 34 ++++ .../command-input-option-field.test.ts | 1 + src/commands/management/app.ts | 5 +- .../open-device-contention-wait.test.ts | 182 ++++++++++++++++++ src/daemon/daemon-request.ts | 7 + src/daemon/open-device-contention-wait.ts | 103 ++++++++++ src/daemon/request-binding.ts | 21 ++ src/daemon/request-execution-scope.ts | 35 +++- .../session-open-device-in-use.test.ts | 77 +++++++- .../internal/session-open-execution.ts | 28 +-- src/daemon/session-recovery-hints.ts | 88 ++++++++- website/docs/docs/commands.md | 1 + website/docs/docs/sessions.md | 8 + 18 files changed, 621 insertions(+), 25 deletions(-) create mode 100644 src/daemon/__tests__/open-device-contention-wait.test.ts create mode 100644 src/daemon/open-device-contention-wait.ts diff --git a/packages/command-registry/src/flag-definitions-workflow.ts b/packages/command-registry/src/flag-definitions-workflow.ts index 5399fe3e0f..8c2c025f52 100644 --- a/packages/command-registry/src/flag-definitions-workflow.ts +++ b/packages/command-registry/src/flag-definitions-workflow.ts @@ -89,6 +89,20 @@ export const WORKFLOW_FLAG_DEFINITIONS: readonly FlagDefinition[] = [ projectConfig: true, recorded: false, }, + { + key: 'waitMs', + names: ['--wait'], + type: 'int', + min: 100, + max: 120000, + usageLabel: '--wait ', + usageDescription: + 'Open: wait up to n ms for a device another session is holding before failing with DEVICE_IN_USE', + inputDescription: + "Block this open for up to n milliseconds while another session holds the device, then fail with DEVICE_IN_USE naming that session. Only session contention is waited for: a device claim held by another workspace is refused at once with its recovery command. A device that never frees costs the full budget, which extends this command's timeout envelope rather than eating into it.", + projectConfig: false, + recorded: false, + }, { key: 'retries', names: ['--retries'], diff --git a/packages/command-registry/src/timeout-policy.ts b/packages/command-registry/src/timeout-policy.ts index b199b59a8c..9cfb3383a6 100644 --- a/packages/command-registry/src/timeout-policy.ts +++ b/packages/command-registry/src/timeout-policy.ts @@ -46,7 +46,7 @@ type BoundedTimeoutPolicy = CommandTimeoutPolicy & { envelopeMs: number }; type FlagTimeoutBudget = Extract; type RequestTimeoutInput = Readonly<{ positionals?: string[]; - flags?: Readonly<{ timeoutMs?: number; settle?: boolean }>; + flags?: Readonly<{ timeoutMs?: number; settle?: boolean; waitMs?: number }>; }>; /** Resolves the request envelope from its declared policy and user-supplied budget. */ @@ -81,13 +81,30 @@ function resolveFlagBudgetTimeoutMs( return resolveWideningFlagBudget(policy, policy.budget, flags); } if (policy.budget.envelope === 'margin') { - return typeof flags?.timeoutMs === 'number' - ? widenToUserBudget(policy, flags.timeoutMs) - : policy.envelopeMs; + return resolveMarginEnvelopeTimeoutBudget(policy, flags); } return typeof flags?.timeoutMs === 'number' ? flags.timeoutMs : policy.envelopeMs; } +/** + * The `--timeout` budget, widened again for a `--wait` budget. A wait is work the command does + * before its own startup budget can even begin — the device is still somebody else's — so it + * extends the envelope rather than eating into it. With no wait this is exactly the margin + * envelope `--timeout` has always produced. + */ +function resolveMarginEnvelopeTimeoutBudget( + policy: BoundedTimeoutPolicy, + flags: RequestTimeoutInput['flags'], +): number { + const startupEnvelopeMs = + typeof flags?.timeoutMs === 'number' + ? widenToUserBudget(policy, flags.timeoutMs) + : policy.envelopeMs; + return typeof flags?.waitMs === 'number' + ? widenPastBaseEnvelope({ ...policy, envelopeMs: startupEnvelopeMs }, flags.waitMs) + : startupEnvelopeMs; +} + function resolveWideningFlagBudget( policy: BoundedTimeoutPolicy, budget: FlagTimeoutBudget, diff --git a/packages/contracts/src/cli-flags.ts b/packages/contracts/src/cli-flags.ts index c5ba3f1649..3a5ed79ae0 100644 --- a/packages/contracts/src/cli-flags.ts +++ b/packages/contracts/src/cli-flags.ts @@ -145,6 +145,7 @@ export type CliFlags = CloudProviderProfileFields & record?: boolean; retainPaths?: boolean; retentionMs?: number; + waitMs?: number; replayMaestro?: boolean; reporter?: string[]; reportJunit?: string; diff --git a/packages/contracts/src/client-app.ts b/packages/contracts/src/client-app.ts index 7f7a833828..ec95dccc78 100644 --- a/packages/contracts/src/client-app.ts +++ b/packages/contracts/src/client-app.ts @@ -68,6 +68,14 @@ export type AppOpenOptions = AgentDeviceRequestOverrides & relaunch?: boolean; /** Startup budget in milliseconds: bounds the Simulator boot wait on a cold device. */ timeoutMs?: number; + /** + * Block this open for up to n milliseconds while another session holds the device, then fail + * with DEVICE_IN_USE naming that session. Only session contention is waited for: a device + * claim held by another workspace is refused at once with its recovery command. A device that + * never frees costs the full budget, which extends this command's timeout envelope rather + * than eating into it. + */ + waitMs?: number; // Editor documentation for a public type: a `.d.ts` is read where no // FlagDefinition resolves, and nothing generates these docs. It is not a second // statement of what the option does — it is the option's ONE declaration (the diff --git a/scripts/integration-progress-model.ts b/scripts/integration-progress-model.ts index f8da3addcd..9b679adba9 100644 --- a/scripts/integration-progress-model.ts +++ b/scripts/integration-progress-model.ts @@ -347,6 +347,14 @@ function summarizeProviderScenarioFlagExclusions() { owner: 'daemon session-open-foreground lifecycle unit tests', keys: ['foreground'], }, + { + // Contention is this daemon's session store and this host's device-claim files. A + // provider scenario allocates a device of its own, so no provider-backed flow can be + // refused as busy and there is nothing for a scenario lane to observe. + name: 'local device contention waiting', + owner: 'daemon session-open contention unit tests', + keys: ['waitMs'], + }, { name: 'Apple simulator screenshot rendering options', owner: 'iOS platform and screenshot-diff runtime tests', diff --git a/src/__tests__/command-descriptor-timeout-policy.test.ts b/src/__tests__/command-descriptor-timeout-policy.test.ts index d12d682520..017390dbe3 100644 --- a/src/__tests__/command-descriptor-timeout-policy.test.ts +++ b/src/__tests__/command-descriptor-timeout-policy.test.ts @@ -364,4 +364,38 @@ test('open and prepare startup budgets keep a client-envelope margin over the da resolveCommandRequestTimeoutMs(resolveCommandTimeoutPolicy('open'), { ...base }), 90_000, ); + + // A --wait budget is spent before the startup budget can begin, so it extends the envelope + // instead of being consumed by it; otherwise a 60s wait would leave a cold boot 30s of envelope + // and end in a client-side daemon reset rather than the open's own refusal. + assert.equal( + resolveCommandRequestTimeoutMs(resolveCommandTimeoutPolicy('open'), { + ...base, + flags: { waitMs: 60_000 }, + }), + 180_000, + ); + assert.equal( + resolveCommandRequestTimeoutMs(resolveCommandTimeoutPolicy('open'), { + ...base, + flags: { timeoutMs: 600_000, waitMs: 60_000 }, + }), + 720_000, + ); + // Even a small wait keeps the margin over the base, so the open's own refusal wins the race. + assert.equal( + resolveCommandRequestTimeoutMs(resolveCommandTimeoutPolicy('open'), { + ...base, + flags: { waitMs: 1_000 }, + }), + 121_000, + ); + // Only `open` reads a wait budget; other commands' envelopes ignore it. + assert.equal( + resolveCommandRequestTimeoutMs(resolveCommandTimeoutPolicy('press'), { + ...base, + flags: { waitMs: 60_000 }, + }), + 90_000, + ); }); diff --git a/src/commands/command-input-option-field.test.ts b/src/commands/command-input-option-field.test.ts index a8236035c8..0508e0b3c2 100644 --- a/src/commands/command-input-option-field.test.ts +++ b/src/commands/command-input-option-field.test.ts @@ -186,6 +186,7 @@ test('the two audiences of one option are declared side by side and stay distinc */ const SDK_DOCUMENTED_OPTIONS = [ { key: 'foreground', file: 'packages/contracts/src/client-app.ts', field: 'foreground' }, + { key: 'waitMs', file: 'packages/contracts/src/client-app.ts', field: 'waitMs' }, { key: 'snapshotCustomActions', file: 'packages/contracts/src/client-capture.ts', diff --git a/src/commands/management/app.ts b/src/commands/management/app.ts index b209f4f263..a44948d9c6 100644 --- a/src/commands/management/app.ts +++ b/src/commands/management/app.ts @@ -53,6 +53,7 @@ const openCommandMetadata = defineFieldCommandMetadata( 'Startup budget in milliseconds. Bounds the Simulator boot wait, so a never-booted Simulator can finish its first-boot migration; omit for the default startup behavior.', { min: 1 }, ), + waitMs: optionField('waitMs'), foreground: optionField('foreground'), saveScript: jsonSchemaField({ oneOf: [booleanSchema(), stringSchema()], @@ -123,6 +124,7 @@ const openCliSchema = { 'relaunch', 'foreground', 'timeoutMs', + 'waitMs', 'surface', ...METRO_RELOAD_FLAGS, 'launchUrl', @@ -150,6 +152,7 @@ const openCliReader: CliReader = (positionals, flags) => ({ relaunch: flags.relaunch, foreground: flags.foreground, timeoutMs: flags.timeoutMs, + waitMs: flags.waitMs, saveScript: flags.saveScript, force: flags.force, deviceHub: flags.deviceHub, @@ -195,7 +198,7 @@ export const openCommandFacet = defineCommandFacet({ text: { summary: 'Open an app, deep link or URL, save replays', cliDetail: - 'Use --platform to bind URL/deep-link opens to the target platform. For iOS simulator initial stdout/stderr, put --launch-console on this open command, for example agent-device open "Agent Device Tester" --platform ios --launch-console artifacts/launch-console.log. Expo Go/dev-client shells accept host + URL, for example agent-device open "Expo Go" exp://127.0.0.1:8081 --platform ios. macOS also supports --surface app|frontmost-app|desktop|menubar. --metro-host/--metro-port/--bundle-url/--launch-url set this session\'s Metro/debug runtime hints as part of open itself (applied to the app\'s dev-server prefs and recorded as the session\'s dev-server binding), so a fresh session has them before its first reload instead of needing a throwaway reload-first call just to seed hints; a later plain metro reload in the same session reuses whichever of these were set. A fresh open without these flags clears any leftover binding from a previous same-name session; close also clears it.', + 'Use --platform to bind URL/deep-link opens to the target platform. For iOS simulator initial stdout/stderr, put --launch-console on this open command, for example agent-device open "Agent Device Tester" --platform ios --launch-console artifacts/launch-console.log. Expo Go/dev-client shells accept host + URL, for example agent-device open "Expo Go" exp://127.0.0.1:8081 --platform ios. macOS also supports --surface app|frontmost-app|desktop|menubar. --metro-host/--metro-port/--bundle-url/--launch-url set this session\'s Metro/debug runtime hints as part of open itself (applied to the app\'s dev-server prefs and recorded as the session\'s dev-server binding), so a fresh session has them before its first reload instead of needing a throwaway reload-first call just to seed hints; a later plain metro reload in the same session reuses whichever of these were set. A fresh open without these flags clears any leftover binding from a previous same-name session; close also clears it. A device another session is holding refuses at once; add --wait to block up to that budget for it and only then fail with DEVICE_IN_USE naming the owning session.', mcpDetail: "Metro and debug runtime hints given here are recorded as the session's dev-server binding, so a later reload reuses them; a fresh open without them clears any binding left by a previous same-name session.", }, diff --git a/src/daemon/__tests__/open-device-contention-wait.test.ts b/src/daemon/__tests__/open-device-contention-wait.test.ts new file mode 100644 index 0000000000..5c3401fc5a --- /dev/null +++ b/src/daemon/__tests__/open-device-contention-wait.test.ts @@ -0,0 +1,182 @@ +import { test, expect, vi, afterEach } from 'vitest'; +import path from 'node:path'; +import type { CommandFlags } from '@agent-device/contracts/command'; +import { clearRequestCanceled, markRequestCanceled } from '@agent-device/host-kit/request'; +import { IOS_SIMULATOR } from '../../__tests__/test-utils/device-fixtures.ts'; +import type { DaemonRequest } from '../daemon-request.ts'; +import type { SessionState } from '../session-state.ts'; +import { SessionStore } from '../session-store.ts'; +import { + readOpenWaitAttempt, + readOpenWaitBudgetMs, + waitForOpenDeviceContention, +} from '../open-device-contention-wait.ts'; + +// `--wait ` is the one answer to a device another session is holding. What makes this wait +// safe is where it runs: outside the device execution lock, because every operation that could +// free the device needs that lock too. These tests pin the loop's accounting and the one case it +// must never wait on — its own session. + +const HOLDER_ADDRESS = 'cwd:8bea844ab16aa9b3:default'; + +function openRequest(flags: CommandFlags): DaemonRequest { + return { + token: 'token', + session: 'default', + command: 'open', + positionals: [], + flags, + meta: { requestId: 'req-open-wait' }, + }; +} + +function storeWithHolder(deviceId = IOS_SIMULATOR.id): SessionStore { + const store = new SessionStore( + path.join('/tmp', `ad-wait-${Math.random().toString(36).slice(2)}`), + ); + const holder: SessionState = { + name: 'default', + sessionScope: { kind: 'cwd', id: '8bea844ab16aa9b3' }, + device: { ...IOS_SIMULATOR, id: deviceId }, + createdAt: 0, + actions: [], + }; + store.set(HOLDER_ADDRESS, holder); + return store; +} + +function resolved(device: typeof IOS_SIMULATOR | undefined) { + let calls = 0; + return { + calls: () => calls, + resolve: async () => { + calls += 1; + return device; + }, + }; +} + +afterEach(() => { + vi.useRealTimers(); + clearRequestCanceled('req-open-wait'); +}); + +test('the budget is read from the request, and a non-positive one is no budget', () => { + expect(readOpenWaitBudgetMs(openRequest({ waitMs: 5000 }))).toBe(5000); + expect(readOpenWaitBudgetMs(openRequest({}))).toBeUndefined(); + expect(readOpenWaitBudgetMs(openRequest({ waitMs: 0 }))).toBeUndefined(); +}); + +test('a free device costs nothing and records no wait', async () => { + const req = openRequest({ waitMs: 30_000 }); + const device = resolved(IOS_SIMULATOR); + + await waitForOpenDeviceContention({ + req, + sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionStore: new SessionStore('/tmp/ad-wait-free'), + budgetMs: 30_000, + resolveDevice: device.resolve, + }); + + expect(device.calls()).toBe(1); + expect(readOpenWaitAttempt(req)).toEqual({}); +}); + +// An open onto the device its own session already holds is not contention: waiting there would +// spend the whole budget on itself. +test('the session that already holds the device is not waited for', async () => { + const req = openRequest({ waitMs: 30_000 }); + const store = storeWithHolder(); + + await waitForOpenDeviceContention({ + req, + sessionName: HOLDER_ADDRESS, + sessionStore: store, + budgetMs: 30_000, + resolveDevice: resolved(IOS_SIMULATOR).resolve, + }); + + expect(readOpenWaitAttempt(req)).toEqual({}); +}); + +test('a device that frees up ends the wait and records what it cost', async () => { + vi.useFakeTimers(); + const req = openRequest({ waitMs: 30_000 }); + const store = storeWithHolder(); + let looks = 0; + + let settled = false; + const running = waitForOpenDeviceContention({ + req, + sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionStore: store, + budgetMs: 30_000, + resolveDevice: async () => { + looks += 1; + if (looks > 2) store.delete(HOLDER_ADDRESS); + return IOS_SIMULATOR; + }, + }).then(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(1_000); + await running; + + expect(settled).toBe(true); + expect(readOpenWaitAttempt(req).waitedMs).toBeGreaterThan(0); +}); + +test('a budget that runs out busy records the spend and lets the open refuse', async () => { + vi.useFakeTimers(); + const req = openRequest({ waitMs: 1000 }); + const device = resolved(IOS_SIMULATOR); + + const running = waitForOpenDeviceContention({ + req, + sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionStore: storeWithHolder(), + budgetMs: 1000, + resolveDevice: device.resolve, + }); + await vi.advanceTimersByTimeAsync(10_000); + await running; + + expect(readOpenWaitAttempt(req).waitedMs).toBe(1000); + // 250ms polls across a 1000ms budget: never an unbounded spin. + expect(device.calls()).toBe(5); +}); + +test('a device that cannot be resolved yet is not waited for', async () => { + const req = openRequest({ waitMs: 30_000 }); + const device = resolved(undefined); + + await waitForOpenDeviceContention({ + req, + sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionStore: storeWithHolder(), + budgetMs: 30_000, + resolveDevice: device.resolve, + }); + + expect(device.calls()).toBe(1); + expect(readOpenWaitAttempt(req)).toEqual({}); +}); + +test('a request the client gave up on stops waiting at its next poll', async () => { + vi.useFakeTimers(); + const req = openRequest({ waitMs: 60_000 }); + markRequestCanceled('req-open-wait'); + + const running = waitForOpenDeviceContention({ + req, + sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionStore: storeWithHolder(), + budgetMs: 60_000, + resolveDevice: resolved(IOS_SIMULATOR).resolve, + }); + const rejection = expect(running).rejects.toThrow(); + await vi.advanceTimersByTimeAsync(250); + + await rejection; +}); diff --git a/src/daemon/daemon-request.ts b/src/daemon/daemon-request.ts index 61b52354d1..cb00d50b2e 100644 --- a/src/daemon/daemon-request.ts +++ b/src/daemon/daemon-request.ts @@ -29,6 +29,13 @@ export type DaemonOpenLifecycle = { }; type DaemonRequestInternal = { + /** + * What an `open` spent from its `--wait` budget while it waited for this device outside the + * device execution lock. The refusal the open ends with has to say the budget was spent, and + * only the daemon that spent it can know that; `internal` never crosses the transport, so no + * client can claim a wait it did not perform. + */ + openDeviceWait?: { waitedMs: number }; publicNetworkOnly?: true; openLifecycle?: DaemonOpenLifecycle; /** diff --git a/src/daemon/open-device-contention-wait.ts b/src/daemon/open-device-contention-wait.ts new file mode 100644 index 0000000000..657d464a40 --- /dev/null +++ b/src/daemon/open-device-contention-wait.ts @@ -0,0 +1,103 @@ +import type { DeviceInfo } from '@agent-device/kernel/device'; +import { emitRequestProgress, throwIfRequestCanceled } from '@agent-device/host-kit/request'; +import { Deadline, sleep } from '@agent-device/host-kit/retry'; +import type { DaemonRequest } from './daemon-request.ts'; +import type { SessionStore } from './session-store.ts'; +import type { SessionRecoveryOptions } from './session-recovery-hints.ts'; + +/** + * How often a waiting open re-checks its device. Contention ends when another session closes or + * its owner dies, both of which are host-side changes on an agent timescale; a faster poll would + * only add reads. + */ +const DEVICE_CONTENTION_POLL_MS = 250; + +/** + * The `--wait ` budget an open carries, or `undefined` when it must not block. A non-positive + * budget is the caller's own spelling of "don't block", so it is not clamped up to the flag's + * declared minimum. + */ +export function readOpenWaitBudgetMs(req: DaemonRequest): number | undefined { + const budgetMs = req.flags?.waitMs; + return typeof budgetMs === 'number' && budgetMs > 0 ? budgetMs : undefined; +} + +/** + * What this request's `--wait` budget actually spent, for the refusal that ends it. + * + * The spend rides on `internal`, which the transport strips, so only the daemon that waited can + * report a wait and the recovery text can say the budget was spent without a client claiming one + * that never happened. + */ +export function readOpenWaitAttempt(req: DaemonRequest): SessionRecoveryOptions { + const waitedMs = req.internal?.openDeviceWait?.waitedMs; + return waitedMs === undefined ? {} : { waitedMs }; +} + +/** + * Spend an open's `--wait` budget waiting for the device it is about to bind, and record what + * that cost. + * + * This runs before the request takes the device's execution lock, which is the whole point: the + * operations that free a contended device — `close`, `record stop`, the holder's own commands — + * need that same lock, so an open that waited while holding it would block the only thing that + * could end its wait. Whatever is decided about the device afterwards stays with the locked open + * path, which re-checks and refuses on its own authority; this wait only declines to queue for + * the lock while the device is visibly spoken for. + * + * Only another session's claim on the device is waited for. A host-global device claim held by + * another workspace's daemon is not: that refusal names an operator command rather than a retry, + * and a budget spent on it buys nothing. + */ +export async function waitForOpenDeviceContention(params: { + req: DaemonRequest; + sessionName: string; + sessionStore: SessionStore; + budgetMs: number; + resolveDevice: () => Promise; +}): Promise { + const deadline = Deadline.fromTimeoutMs(params.budgetMs); + let waitedMs = 0; + for (;;) { + const device = await params.resolveDevice(); + const holder = device + ? findSessionHoldingDevice(params.sessionStore, device.id, params.sessionName) + : undefined; + if (!holder || deadline.isExpired()) { + recordOpenWaitSpend(params.req, waitedMs); + return; + } + emitRequestProgress({ + type: 'command', + status: 'progress', + message: `Waiting for ${device!.name}: held by session "${holder}" (${Math.round( + waitedMs, + )}ms of the ${Math.round(params.budgetMs)}ms wait budget used)`, + }); + await sleep(Math.min(DEVICE_CONTENTION_POLL_MS, deadline.remainingMs())); + waitedMs = deadline.elapsedMs(); + recordOpenWaitSpend(params.req, waitedMs); + // A client that gave up on the request must not leave the daemon queueing for a device + // nobody wants any more. Checked after the poll rather than before the first look because + // whether an already-canceled request runs at all is the request pipeline's own question. + throwIfRequestCanceled(params.req.meta?.requestId); + } +} + +/** The other session holding `deviceId`, or `undefined` when nothing stands between this open + * and the device — including when this open's own session is what holds it. */ +function findSessionHoldingDevice( + sessionStore: SessionStore, + deviceId: string, + sessionName: string, +): string | undefined { + const inUse = sessionStore.findByDevice(deviceId); + if (!inUse || inUse.address === sessionName) return undefined; + return inUse.address; +} + +function recordOpenWaitSpend(req: DaemonRequest, waitedMs: number): void { + if (waitedMs <= 0) return; + const internal = (req.internal ??= {}); + internal.openDeviceWait = { waitedMs }; +} diff --git a/src/daemon/request-binding.ts b/src/daemon/request-binding.ts index 43aa6becca..1ac0326f3b 100644 --- a/src/daemon/request-binding.ts +++ b/src/daemon/request-binding.ts @@ -4,6 +4,7 @@ import { applyRequestLockPolicy } from './request-lock-policy.ts'; import { buildOpenTargetDeviceResolutionOptions } from './open-device-selection.ts'; import { buildReplayTargetDeviceResolution } from './replay-device-selection.ts'; import type { SessionStore } from './session-store.ts'; +import type { DeviceInfo } from '@agent-device/kernel/device'; import type { DaemonRequest } from './daemon-request.ts'; import type { SessionRef } from './session-state.ts'; @@ -78,6 +79,26 @@ function resolveFreshSessionDeviceLock(req: DaemonRequest): return resolveExplicitDeviceLock(req); } +/** + * The device an `open` is expected to bind, resolved the same advisory way its device execution + * lock is chosen — before the lock is held, and never as an authority on what happens next. A + * caller that must not hold the device lock uses this to look at contention; the locked open path + * still resolves and binds the target device itself. + */ +export async function resolveAdvisoryOpenDevice( + req: DaemonRequest, +): Promise { + if (req.command !== 'open') return undefined; + const resolution = resolveOpenDeviceLock(resolveFreshSessionBindingRequest(req)); + if (!resolution) return undefined; + try { + return await resolveTargetDevice(resolution.flags, resolution.options); + } catch { + // Unresolvable here means unresolvable for lock selection too, which is already tolerated. + return undefined; + } +} + function resolveOpenDeviceLock(req: DaemonRequest) { const options = buildOpenTargetDeviceResolutionOptions(req.positionals?.[0]); return options ? { flags: req.flags ?? {}, options } : undefined; diff --git a/src/daemon/request-execution-scope.ts b/src/daemon/request-execution-scope.ts index 638fd90a81..30acf9c00a 100644 --- a/src/daemon/request-execution-scope.ts +++ b/src/daemon/request-execution-scope.ts @@ -19,7 +19,15 @@ import { assertLockedLeaseAdmissionPreflight, cleanupExpiredLeasedSession, } from './lease-lifecycle.ts'; -import { prepareLockedRequestBinding, resolveRequestExecutionLockKeys } from './request-binding.ts'; +import { + prepareLockedRequestBinding, + resolveAdvisoryOpenDevice, + resolveRequestExecutionLockKeys, +} from './request-binding.ts'; +import { + readOpenWaitBudgetMs, + waitForOpenDeviceContention, +} from './open-device-contention-wait.ts'; import { createRequestExecutionLocks } from './request-execution-locks.ts'; import { throwIfRequestCanceled } from '@agent-device/host-kit/request'; import { finalizeDaemonResponse } from './request-finalization.ts'; @@ -112,6 +120,30 @@ export type LockedRequestScopeResult = | { type: 'scope'; scope: LockedRequestScope } | { type: 'response'; response: DaemonResponse }; +/** + * Spend an `--wait ` budget on a contended device while the request still has no locks. Must + * run before {@link resolveRequestExecutionLockKeys}: the device's execution lock is what every + * operation that could free the device also needs, so waiting after taking it would have an open + * block its own recovery. A fresh open only — an open onto a session that already exists is bound + * to a device nobody else is being refused. + */ +async function waitForOpenDeviceBeforeLock( + req: DaemonRequest, + sessionName: string, + sessionStore: SessionStore, +): Promise { + if (req.command !== 'open' || sessionStore.get(sessionName)) return; + const budgetMs = readOpenWaitBudgetMs(req); + if (budgetMs === undefined) return; + await waitForOpenDeviceContention({ + req, + sessionName, + sessionStore, + budgetMs, + resolveDevice: () => resolveAdvisoryOpenDevice(req), + }); +} + export async function createRequestExecutionScope(params: { req: DaemonRequest; sessionStore: SessionStore; @@ -171,6 +203,7 @@ export async function createRequestExecutionScope(params: { } try { assertLockedLeaseAdmissionPreflight(scopedReq); + await waitForOpenDeviceBeforeLock(scopedReq, sessionName, sessionStore); const executionLockKeys = shouldLockSessionExecution(command) ? await resolveRequestExecutionLockKeys({ req: scopedReq, sessionName, sessionStore }) : []; diff --git a/src/daemon/session-lifecycle/internal/__tests__/session-open-device-in-use.test.ts b/src/daemon/session-lifecycle/internal/__tests__/session-open-device-in-use.test.ts index 63c0988786..db7ff7ad8d 100644 --- a/src/daemon/session-lifecycle/internal/__tests__/session-open-device-in-use.test.ts +++ b/src/daemon/session-lifecycle/internal/__tests__/session-open-device-in-use.test.ts @@ -1,5 +1,8 @@ import { test, expect } from 'vitest'; -import { buildDeviceInUseBySessionError } from '../../../session-recovery-hints.ts'; +import { + buildDeviceInUseBySessionError, + buildForeignWorkspaceSessionConflict, +} from '../../../session-recovery-hints.ts'; import type { SessionRef } from '../../../session-state.ts'; import { IOS_SIMULATOR } from '../../../../__tests__/test-utils/device-fixtures.ts'; @@ -32,3 +35,75 @@ test('the by-session conflict reports the address, in the message, details and h `agent-device close --session ${SCOPED_ADDRESS}`, ); }); + +// The other workspace's session used to be reported as "another workspace session" with no +// address at all: the caller was told to wait for a session it could neither name, reach with +// --session, nor close. +test('the foreign-workspace conflict names the owning session address', () => { + const foreignAddress = 'cwd:1d9b7c2f4a6e8b03:default'; + const foreignRef: SessionRef = { + address: foreignAddress, + session: { + name: 'default', + sessionScope: { kind: 'cwd', id: '1d9b7c2f4a6e8b03' }, + device: IOS_SIMULATOR, + createdAt: 0, + actions: [], + }, + }; + + const response = buildForeignWorkspaceSessionConflict(foreignRef, IOS_SIMULATOR); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error?.code).toBe('DEVICE_IN_USE'); + expect(response.error?.message).toContain(foreignAddress); + expect(response.error?.details).toMatchObject({ + reason: 'WORKSPACE_SESSION_SCOPE_CONFLICT', + session: foreignAddress, + deviceId: IOS_SIMULATOR.id, + }); + const hint = String(response.error?.details?.hint); + expect(hint).toContain(`agent-device close --session ${foreignAddress}`); +}); + +// A caller that already spent a --wait budget is not helped by being told to wait, and is misled +// by a hint that reads as though nothing was tried. The spend flips the phrasing. +test('a spent wait budget is reported as spent and never re-offered', () => { + const response = buildForeignWorkspaceSessionConflict(scopedRef, IOS_SIMULATOR, { + waitedMs: 30_000, + offersDeviceWait: true, + }); + + expect(response.ok).toBe(false); + if (response.ok) return; + expect(response.error?.details?.waitedMs).toBe(30_000); + const hint = String(response.error?.details?.hint); + expect(hint).toMatch(/^Waited 30000ms for this device and it stayed busy\./); + expect(hint).not.toContain('--wait'); +}); + +test('an open that never waited offers the wait instead of claiming one', () => { + const hint = String( + buildDeviceInUseBySessionError(scopedRef, IOS_SIMULATOR, { offersDeviceWait: true }).error + ?.details?.hint, + ); + + expect(hint).not.toMatch(/Waited/); + expect(hint).toContain('--wait '); +}); + +// Only `open` carries --wait. An interaction refused by a busy device has to reuse or close the +// owning session, and a hint ending in a flag its command rejects sends it away from both. +test('a refusal from a command that cannot wait never offers the flag', () => { + const hints = [ + buildDeviceInUseBySessionError(scopedRef, IOS_SIMULATOR), + buildForeignWorkspaceSessionConflict(scopedRef, IOS_SIMULATOR), + ]; + + for (const response of hints) { + expect(response.ok).toBe(false); + if (response.ok) return; + expect(String(response.error?.details?.hint)).not.toContain('--wait'); + } +}); diff --git a/src/daemon/session-lifecycle/internal/session-open-execution.ts b/src/daemon/session-lifecycle/internal/session-open-execution.ts index b06f7e9664..da582d48ef 100644 --- a/src/daemon/session-lifecycle/internal/session-open-execution.ts +++ b/src/daemon/session-lifecycle/internal/session-open-execution.ts @@ -37,7 +37,11 @@ import { prepareOpenCommandDetails, type ResolvedOpenRuntimeHintPlan, } from './session-open-prepare.ts'; -import { buildDeviceInUseBySessionError } from '../../session-recovery-hints.ts'; +import { + buildDeviceInUseBySessionError, + buildForeignWorkspaceSessionConflict, +} from '../../session-recovery-hints.ts'; +import { readOpenWaitAttempt } from '../../open-device-contention-wait.ts'; import { isImplicitSessionScopeConflict, resolveSessionScope, @@ -381,26 +385,26 @@ function createProvisionalOpenDispatchSession(params: { return provisionalSession; } +/** + * The refusal an open gets when another session already holds the device. The wait an expired + * `--wait` budget spent is carried into the recovery text, because a caller that waited is not + * helped by being told to wait. + */ function findNewSessionDeviceConflict(params: { req: DaemonRequest; device: DeviceInfo; sessionStore: SessionStore; -}): DaemonResponse | undefined { +}): DaemonFailureResponse | undefined { const { req, device, sessionStore } = params; const inUse = sessionStore.findByDevice(device.id); if (!inUse) return undefined; + // `offersDeviceWait` belongs to this producer alone: an interaction that hits the same busy + // device cannot wait for it, and would be sent off with a flag its own command rejects. + const attempt = { ...readOpenWaitAttempt(req), offersDeviceWait: true } as const; if (isImplicitSessionScopeConflict(req, inUse.session)) { - return errorResponse( - 'DEVICE_IN_USE', - 'Device is already in use by another workspace session.', - { - deviceId: device.id, - deviceName: device.name, - hint: 'Use a different device selector, wait for the other workspace to close its session, or run agent-device devices to choose another target.', - }, - ); + return buildForeignWorkspaceSessionConflict(inUse, device, attempt); } - return buildDeviceInUseBySessionError(inUse, device); + return buildDeviceInUseBySessionError(inUse, device, attempt); } async function acquireDeviceClaimForOwner(params: { diff --git a/src/daemon/session-recovery-hints.ts b/src/daemon/session-recovery-hints.ts index 8090282baa..a8f23cb417 100644 --- a/src/daemon/session-recovery-hints.ts +++ b/src/daemon/session-recovery-hints.ts @@ -1,8 +1,7 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import { shellQuoteIfNeeded } from '@agent-device/host-kit/command'; -import type { DaemonResponse } from './daemon-request.ts'; import type { SessionRef, SessionState } from './session-state.ts'; -import { errorResponse } from '@agent-device/kernel/contracts'; +import { errorResponse, type DaemonFailureResponse } from '@agent-device/kernel/contracts'; export type SessionRecoveryContext = 'device-in-use' | 'selector-conflict'; @@ -10,6 +9,18 @@ export type SessionRecoveryOptions = { /** Whether the request disagrees with the bound session on *platform*, the one conflict another * platform's implicit session actually answers. */ offersPlatformSession?: boolean; + /** + * Milliseconds an `open` already waited for the device under its `--wait` budget. Set only + * once a budget was spent, and it flips the recovery text: a caller that waited is told the + * wait happened, and one that did not is told waiting is an option. + */ + waitedMs?: number; + /** + * Whether the refused command is one that can wait for a device at all. Only `open` carries + * `--wait`, so only its refusal may offer it; an interaction refused mid-session is told to + * reuse or close the owning session, which is what it can actually run. + */ + offersDeviceWait?: boolean; }; export function describeSessionDevice(session: SessionState): string { @@ -40,15 +51,42 @@ export function buildSessionRecoveryHint( export function buildDeviceInUseBySessionError( inUse: SessionRef, device: DeviceInfo, -): DaemonResponse { + options: SessionRecoveryOptions = {}, +): DaemonFailureResponse { return errorResponse('DEVICE_IN_USE', `Device is already in use by session "${inUse.address}".`, { session: inUse.address, deviceId: device.id, deviceName: device.name, - hint: buildSessionRecoveryHint(inUse, 'device-in-use'), + ...(options.waitedMs === undefined ? {} : { waitedMs: options.waitedMs }), + hint: buildSessionRecoveryHint(inUse, 'device-in-use', options), }); } +/** + * The refusal an `open` gets when the device is held by another workspace's implicit session. + * The address is named because it is the only handle that reaches that session: an implicit + * workspace session is stored under `cwd::default` while `session list` shows `default`, + * so a hint that said only "another workspace" handed the caller nothing it could act on (#2580). + */ +export function buildForeignWorkspaceSessionConflict( + inUse: SessionRef, + device: DeviceInfo, + options: SessionRecoveryOptions = {}, +): DaemonFailureResponse { + return errorResponse( + 'DEVICE_IN_USE', + `Device is already in use by workspace session "${inUse.address}".`, + { + reason: 'WORKSPACE_SESSION_SCOPE_CONFLICT', + session: inUse.address, + deviceId: device.id, + deviceName: device.name, + ...(options.waitedMs === undefined ? {} : { waitedMs: options.waitedMs }), + hint: buildForeignWorkspaceSessionRecoveryHint(inUse.address, options), + }, + ); +} + function buildRecordingSessionRecoveryHint( sessionAddress: string, context: SessionRecoveryContext, @@ -69,6 +107,27 @@ function buildRecordingSessionRecoveryHint( ); } +/** + * The recovery text for the one session-in-use conflict the caller cannot simply join: the + * holder is another workspace's implicit session. The address is still the whole point — it is + * what `close --session` and `--session` take, and for an implicit session it is the store key + * rather than the `default` name `session list` shows (#2031) — but reusing it would run this + * workspace's commands inside the other workspace's session, so closing it, switching device, and + * waiting are the moves offered. + */ +function buildForeignWorkspaceSessionRecoveryHint( + sessionAddress: string, + options: SessionRecoveryOptions, +): string { + const sessionArg = shellQuoteIfNeeded(sessionAddress); + return ( + `${describeOpenWaitAttempt(options)}Session "${sessionAddress}" belongs to another workspace. ` + + `Run agent-device close --session ${sessionArg} to release this device from that workspace, ` + + `agent-device devices to choose another device. ` + + `${waitForDeviceHint(options)}Run agent-device session list to inspect active sessions.` + ); +} + function buildOpenSessionRecoveryHint( ref: SessionRef, context: SessionRecoveryContext, @@ -87,12 +146,29 @@ function buildOpenSessionRecoveryHint( } return ( - `Run agent-device session list to inspect active sessions. ` + + `${describeOpenWaitAttempt(options)}Run agent-device session list to inspect active sessions. ` + `To reuse this device, rerun the command with --session ${sessionArg}. ` + - `To open a new session on this device, first run ${closeCommand}.` + `To open a new session on this device, first run ${closeCommand}. ` + + `${waitForDeviceHint(options)}`.trimEnd() ); } +/** + * The record of a spent `--wait` budget, phrased so a caller that already waited is not told to + * wait again. Only an `open` can wait, so this is the only place the budget is spoken of. + */ +function describeOpenWaitAttempt(options: SessionRecoveryOptions): string { + return options.waitedMs === undefined + ? '' + : `Waited ${Math.round(options.waitedMs)}ms for this device and it stayed busy. `; +} + +function waitForDeviceHint(options: SessionRecoveryOptions): string { + return options.offersDeviceWait && options.waitedMs === undefined + ? 'To wait for the device to free up, run the same open with --wait . ' + : ''; +} + /** * An implicit workspace session is addressed by platform, so switching platforms needs no invented * session name (#2580). A session the caller named by hand has no platform address to fall back to, diff --git a/website/docs/docs/commands.md b/website/docs/docs/commands.md index 18a4aefd42..82908ae188 100644 --- a/website/docs/docs/commands.md +++ b/website/docs/docs/commands.md @@ -70,6 +70,7 @@ agent-device app-switcher - Android: `shutdown --platform android --device ` stops a running emulator. - `open [app|url] [url]` already boots/activates the selected target when needed. - `open --timeout ` is a startup budget for that boot. A never-booted iOS Simulator runs Apple's first-boot migration, which can take several minutes; without the flag the boot wait is capped at 120 seconds. When the budget runs out the command fails with `error.details.reason: boot_timeout` and the Simulator keeps booting, so a retry finds it further along. +- `open --wait ` waits up to that budget for a device another session is holding instead of failing at once. The open reports each poll, then either opens the device or fails with `DEVICE_IN_USE` naming the owning session and saying the budget was spent. Only session contention is waited for: a device claim held by another workspace's daemon is never retriable and returns its recovery command immediately. The wait extends the command's timeout envelope, so a long budget does not need a longer `--timeout`. - `open ` deep links are supported on Android and iOS. - `open ` opens a deep link on iOS. - `open --launch-console ` captures launch-time stdout/stderr for direct iOS simulator app launches. It is not valid for URL opens or diff --git a/website/docs/docs/sessions.md b/website/docs/docs/sessions.md index f4beba56fd..4834f53a22 100644 --- a/website/docs/docs/sessions.md +++ b/website/docs/docs/sessions.md @@ -73,6 +73,14 @@ A never-booted iOS Simulator can take several minutes to finish its first boot. agent-device open Settings --platform ios --udid --timeout 600000 ``` +When the device is held by a session that will let go soon — a parallel agent finishing its run — `open --wait ` blocks for the device instead of refusing at once, and reports who holds it while it waits: + +```bash +agent-device open Demo --platform android --wait 60000 +``` + +If the budget runs out the device is still busy, the open fails with `DEVICE_IN_USE` naming the owning session address, which `close --session
` releases. + Notes: - `open ` within an existing session switches the active app and updates the session bundle id. From d9b1708848988b0126e8d7d63ba69cc3e7c1f3ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 18 Sep 2026 10:35:57 +0200 Subject: [PATCH 3/7] fix(ios): say what a non-macOS host can drive instead of refusing "Apple tools are only available on macOS" is true and useless to a caller on Linux that just wants its app driven: it reads as a missing install, and `retriable` was unset so replay could retry it. The refusal now carries `supportedOn`, the observed `hostOs`, `retriable: false`, and a hint naming the Android commands for the same workflow plus the remote-Mac-daemon route. --- packages/platform-apple/src/inventory.test.ts | 29 ++++++++++++++++++ packages/platform-apple/src/inventory.ts | 30 ++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/packages/platform-apple/src/inventory.test.ts b/packages/platform-apple/src/inventory.test.ts index 4d17616a93..edbd247d93 100644 --- a/packages/platform-apple/src/inventory.test.ts +++ b/packages/platform-apple/src/inventory.test.ts @@ -51,6 +51,35 @@ test('Apple inventory rejects unsupported hosts and missing tools with typed err ); }); +test('the non-macOS refusal names what this host can drive instead', async () => { + const refusal = await createAppleInventorySource(createInventoryHost({ hostOs: 'linux' })) + .discover({ platform: 'ios' }, inventoryScope) + .catch((error: unknown) => error); + + assert.ok(refusal instanceof AppError); + assert.equal(refusal.code, 'UNSUPPORTED_PLATFORM'); + assert.equal(refusal.details?.supportedOn, 'macOS'); + assert.equal(refusal.details?.hostOs, 'linux'); + assert.equal(refusal.details?.retriable, false); + const hint = String(refusal.details?.hint); + assert.match(hint, /runs Linux/); + assert.match(hint, /agent-device devices --platform android/); + assert.match(hint, /agent-device open --platform android/); + assert.match(hint, /--daemon-base-url/); +}); + +// The label is what makes the refusal readable, and an unmapped host must still refuse +// rather than render `undefined`. +test('every host operating system renders a readable refusal', async () => { + for (const hostOs of ['win32', 'other'] as const) { + const refusal = await createAppleInventorySource(createInventoryHost({ hostOs })) + .discover({ platform: 'ios' }, inventoryScope) + .catch((error: unknown) => error); + assert.ok(refusal instanceof AppError); + assert.match(String(refusal.details?.hint), /runs (Windows|a non-macOS host),/); + } +}); + test('simulator-only inventory avoids physical discovery and observes fresh booted devices', async () => { const calls: string[][] = []; const observed: string[] = []; diff --git a/packages/platform-apple/src/inventory.ts b/packages/platform-apple/src/inventory.ts index b6c78dec41..9290db7526 100644 --- a/packages/platform-apple/src/inventory.ts +++ b/packages/platform-apple/src/inventory.ts @@ -1,6 +1,7 @@ import type { DeviceInventoryRequest } from '@agent-device/contracts/device'; import type { DeviceInventoryHostFor, + HostOperatingSystem, PlatformRequestScope, } from '@agent-device/contracts/platform-runtime-host'; import type { DeviceInventorySource } from '@agent-device/contracts/platform-module'; @@ -26,7 +27,7 @@ async function discoverAppleDevices( return [hostMacDevice(host)]; } if (host.hostOs !== 'darwin') { - throw new AppError('UNSUPPORTED_PLATFORM', 'Apple tools are only available on macOS'); + throw appleHostUnsupportedError(host.hostOs); } if (!(await host.appleTools.isXcrunAvailable(scope.signal))) { throw new AppError('TOOL_MISSING', 'xcrun not found in PATH'); @@ -59,6 +60,33 @@ function isMacOnlyRequest(request: Readonly): boolean { ); } +const HOST_OS_LABELS: Record = { + darwin: 'macOS', + linux: 'Linux', + win32: 'Windows', + other: 'a non-macOS host', +}; + +/** + * The refusal a non-macOS host gets for Apple sessions. Naming only the requirement left + * Linux and Windows callers at a dead end — the host OS never changes for a running + * daemon, so there is nothing to retry and nothing to install here. The hint carries the + * Android shape of the same request, which is what this host can drive, and the one way + * to reach a real iOS Simulator from here: a daemon running on a Mac. + */ +function appleHostUnsupportedError(hostOs: HostOperatingSystem): AppError { + return new AppError('UNSUPPORTED_PLATFORM', 'Apple tools are only available on macOS', { + supportedOn: 'macOS', + hostOs, + retriable: false, + hint: + `This host runs ${HOST_OS_LABELS[hostOs]}, so it cannot boot iOS Simulators or run macOS sessions. ` + + 'Drive Android here instead with the same command shape: agent-device devices --platform android to list ' + + 'targets, then agent-device open --platform android. To use an iOS Simulator from this machine, ' + + 'run agent-device on a macOS host and pass --daemon-base-url .', + }); +} + function hostMacDevice(host: DeviceInventoryHostFor<'apple'>): DeviceInfo { return { platform: 'apple', From 4f745602dea2005e1ab78b99835f29132b9b293f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 18 Sep 2026 16:44:37 +0200 Subject: [PATCH 4/7] fix(daemon): let a waiting open re-wait when the device is retaken An `--wait` budget ended as soon as no session held the device, but nothing reserved the device until the request took its execution lock, so two waiters could both see one free device and the loser refused with most of its budget left. The open now looks at the device again under its own locks, hands them back when another session took it in that window, and waits for what remains of its budget. The recorded spend moves to that same look, so only a budget that ran out with a holder visible reports a spent wait, and a refusal stops offering `--wait` to a caller that arrived carrying it. The target device is resolved once, as part of the request's execution-lock plan, and contention is polled in the session store alone. A budget that reaches the daemon anywhere but through the CLI is now checked against the bounds the option declares, which the Node client never enforced. --- .../src/flag-definitions-workflow.ts | 2 +- packages/contracts/src/client-app.ts | 10 +- .../open-device-contention-wait.test.ts | 302 ++++++++++++------ ...t-execution-scope-open-device-wait.test.ts | 165 ++++++++++ .../__tests__/request-router-open.test.ts | 8 +- src/daemon/open-device-contention-wait.ts | 189 ++++++++--- src/daemon/request-binding.ts | 56 ++-- src/daemon/request-execution-scope.ts | 60 ++-- .../internal/session-open-execution.ts | 6 +- .../nightly/concurrency-torture.test.ts | 2 +- .../nightly/concurrency-torture/bindings.ts | 13 +- .../nightly/concurrency-torture/harness.ts | 2 +- website/docs/docs/commands.md | 2 +- website/docs/docs/sessions.md | 2 +- 14 files changed, 587 insertions(+), 232 deletions(-) create mode 100644 src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts diff --git a/packages/command-registry/src/flag-definitions-workflow.ts b/packages/command-registry/src/flag-definitions-workflow.ts index 8c2c025f52..1ab0eb7a47 100644 --- a/packages/command-registry/src/flag-definitions-workflow.ts +++ b/packages/command-registry/src/flag-definitions-workflow.ts @@ -99,7 +99,7 @@ export const WORKFLOW_FLAG_DEFINITIONS: readonly FlagDefinition[] = [ usageDescription: 'Open: wait up to n ms for a device another session is holding before failing with DEVICE_IN_USE', inputDescription: - "Block this open for up to n milliseconds while another session holds the device, then fail with DEVICE_IN_USE naming that session. Only session contention is waited for: a device claim held by another workspace is refused at once with its recovery command. A device that never frees costs the full budget, which extends this command's timeout envelope rather than eating into it.", + "Block this open for up to n milliseconds (100-120000) while another session holds the device, then fail with DEVICE_IN_USE naming that session. Only session contention is waited for: a device claim held by another workspace is refused at once with its recovery command. A device that never frees, or is taken again while this open waits, costs the full budget, which extends this command's timeout envelope rather than eating into it.", projectConfig: false, recorded: false, }, diff --git a/packages/contracts/src/client-app.ts b/packages/contracts/src/client-app.ts index ec95dccc78..c909ed3065 100644 --- a/packages/contracts/src/client-app.ts +++ b/packages/contracts/src/client-app.ts @@ -69,11 +69,11 @@ export type AppOpenOptions = AgentDeviceRequestOverrides & /** Startup budget in milliseconds: bounds the Simulator boot wait on a cold device. */ timeoutMs?: number; /** - * Block this open for up to n milliseconds while another session holds the device, then fail - * with DEVICE_IN_USE naming that session. Only session contention is waited for: a device - * claim held by another workspace is refused at once with its recovery command. A device that - * never frees costs the full budget, which extends this command's timeout envelope rather - * than eating into it. + * Block this open for up to n milliseconds (100-120000) while another session holds the device, + * then fail with DEVICE_IN_USE naming that session. Only session contention is waited for: a + * device claim held by another workspace is refused at once with its recovery command. A device + * that never frees, or is taken again while this open waits, costs the full budget, which + * extends this command's timeout envelope rather than eating into it. */ waitMs?: number; // Editor documentation for a public type: a `.d.ts` is read where no diff --git a/src/daemon/__tests__/open-device-contention-wait.test.ts b/src/daemon/__tests__/open-device-contention-wait.test.ts index 5c3401fc5a..a2b60f1dbc 100644 --- a/src/daemon/__tests__/open-device-contention-wait.test.ts +++ b/src/daemon/__tests__/open-device-contention-wait.test.ts @@ -1,23 +1,28 @@ import { test, expect, vi, afterEach } from 'vitest'; -import path from 'node:path'; +import { isRequestCanceledError } from '@agent-device/kernel/errors'; import type { CommandFlags } from '@agent-device/contracts/command'; import { clearRequestCanceled, markRequestCanceled } from '@agent-device/host-kit/request'; import { IOS_SIMULATOR } from '../../__tests__/test-utils/device-fixtures.ts'; +import { getFlagDefinitionsForKey } from '@agent-device/command-registry/flag-registry'; +import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; import type { DaemonRequest } from '../daemon-request.ts'; import type { SessionState } from '../session-state.ts'; import { SessionStore } from '../session-store.ts'; import { + beginOpenDeviceWait, + describeOpenWaitForRefusal, readOpenWaitAttempt, readOpenWaitBudgetMs, - waitForOpenDeviceContention, } from '../open-device-contention-wait.ts'; -// `--wait ` is the one answer to a device another session is holding. What makes this wait -// safe is where it runs: outside the device execution lock, because every operation that could -// free the device needs that lock too. These tests pin the loop's accounting and the one case it -// must never wait on — its own session. +// `--wait ` is the one answer to a device another session is holding. Two things make that +// wait trustworthy, and both are pinned here: it runs outside the device execution lock, because +// every operation that could free the device needs that lock too; and the open re-looks at the +// device under the locks, so a device taken in the window between the last look and the lock is +// waited for again instead of refused with budget still unspent. const HOLDER_ADDRESS = 'cwd:8bea844ab16aa9b3:default'; +const OPENER_ADDRESS = 'cwd:1d9b7c2f4a6e8b03:default'; function openRequest(flags: CommandFlags): DaemonRequest { return { @@ -30,153 +35,266 @@ function openRequest(flags: CommandFlags): DaemonRequest { }; } -function storeWithHolder(deviceId = IOS_SIMULATOR.id): SessionStore { - const store = new SessionStore( - path.join('/tmp', `ad-wait-${Math.random().toString(36).slice(2)}`), - ); - const holder: SessionState = { +function session(address: string): SessionState { + return { name: 'default', - sessionScope: { kind: 'cwd', id: '8bea844ab16aa9b3' }, - device: { ...IOS_SIMULATOR, id: deviceId }, + sessionScope: { kind: 'cwd', id: address.split(':')[1] ?? '' }, + device: IOS_SIMULATOR, createdAt: 0, actions: [], }; - store.set(HOLDER_ADDRESS, holder); +} + +function storeWithHolder(): SessionStore { + const store = makeSessionStore('agent-device-open-wait-'); + store.set(HOLDER_ADDRESS, session(HOLDER_ADDRESS)); return store; } -function resolved(device: typeof IOS_SIMULATOR | undefined) { - let calls = 0; +/** Drives {@link OpenDeviceWait.runWhenDeviceIsUnheld} the way the request scope does, and keeps + * the order the locks, the open, and any re-wait actually happened in. */ +function lockTrace(params: { onAcquire?: (pass: number) => void }) { + const order: string[] = []; + let passes = 0; return { - calls: () => calls, - resolve: async () => { - calls += 1; - return device; + order, + acquireLocks: async (task: () => Promise): Promise => { + passes += 1; + order.push('acquire'); + params.onAcquire?.(passes); + const outcome = await task(); + order.push('release'); + return outcome; }, }; } +function thrownBy(build: () => unknown): unknown { + try { + build(); + } catch (error) { + return error; + } + throw new Error('Expected the request to be refused.'); +} + afterEach(() => { vi.useRealTimers(); clearRequestCanceled('req-open-wait'); }); -test('the budget is read from the request, and a non-positive one is no budget', () => { - expect(readOpenWaitBudgetMs(openRequest({ waitMs: 5000 }))).toBe(5000); +// The CLI parser refuses a `--wait` outside the option's declared bounds and the tool input derived +// from it refuses the same. A budget assembled by the Node client or posted straight to the wire +// reaches the daemon with neither check applied, so the reader that spends it applies the bounds of +// the same declaration rather than a copy of them. +test('the daemon holds a budget to the bounds its own option declares', () => { + const [waitFlag] = getFlagDefinitionsForKey('waitMs'); + const { min, max } = waitFlag ?? {}; + expect({ min, max }).toEqual({ min: expect.any(Number), max: expect.any(Number) }); + + expect(readOpenWaitBudgetMs(openRequest({ waitMs: 5_000 }))).toBe(5_000); + expect(readOpenWaitBudgetMs(openRequest({ waitMs: min }))).toBe(min); + expect(readOpenWaitBudgetMs(openRequest({ waitMs: max }))).toBe(max); expect(readOpenWaitBudgetMs(openRequest({}))).toBeUndefined(); - expect(readOpenWaitBudgetMs(openRequest({ waitMs: 0 }))).toBeUndefined(); + + for (const waitMs of [(min ?? 0) - 1, (max ?? 0) + 1, 0, 1.5]) { + expect(thrownBy(() => readOpenWaitBudgetMs(openRequest({ waitMs })))).toMatchObject({ + code: 'INVALID_ARGS', + }); + } }); -test('a free device costs nothing and records no wait', async () => { - const req = openRequest({ waitMs: 30_000 }); - const device = resolved(IOS_SIMULATOR); +test('a refusal offers the flag only to a caller that did not arrive carrying it', () => { + expect(describeOpenWaitForRefusal(openRequest({}))).toEqual({ offersDeviceWait: true }); - await waitForOpenDeviceContention({ - req, - sessionName: 'cwd:1d9b7c2f4a6e8b03:default', - sessionStore: new SessionStore('/tmp/ad-wait-free'), - budgetMs: 30_000, - resolveDevice: device.resolve, - }); + // The open that passed `--wait` and still hit a busy device is not being sent off to run the + // same open with the flag it used. + const carried = openRequest({ waitMs: 1_000 }); + expect(describeOpenWaitForRefusal(carried)).toEqual({ offersDeviceWait: false }); - expect(device.calls()).toBe(1); - expect(readOpenWaitAttempt(req)).toEqual({}); + const spent: DaemonRequest = { ...carried, internal: { openDeviceWait: { waitedMs: 1_000 } } }; + expect(describeOpenWaitForRefusal(spent)).toEqual({ waitedMs: 1_000, offersDeviceWait: false }); }); -// An open onto the device its own session already holds is not contention: waiting there would -// spend the whole budget on itself. -test('the session that already holds the device is not waited for', async () => { +test('only a fresh open with a budget and a resolved device gets a wait', () => { + const sessionStore = new SessionStore('/tmp/ad-wait-none'); + const budget = openRequest({ waitMs: 5_000 }); + + expect( + beginOpenDeviceWait({ + req: { ...budget, command: 'tap' }, + sessionName: OPENER_ADDRESS, + sessionStore, + deviceId: IOS_SIMULATOR.id, + }), + ).toBeUndefined(); + expect( + beginOpenDeviceWait({ + req: budget, + sessionName: OPENER_ADDRESS, + sessionStore, + deviceId: undefined, + }), + ).toBeUndefined(); + expect( + beginOpenDeviceWait({ + req: openRequest({}), + sessionName: OPENER_ADDRESS, + sessionStore, + deviceId: IOS_SIMULATOR.id, + }), + ).toBeUndefined(); + + // An open onto a session that already exists is bound to a device nobody else is refused for. + sessionStore.set(OPENER_ADDRESS, session(OPENER_ADDRESS)); + expect( + beginOpenDeviceWait({ + req: budget, + sessionName: OPENER_ADDRESS, + sessionStore, + deviceId: IOS_SIMULATOR.id, + }), + ).toBeUndefined(); +}); + +test('a free device is read once in the store and records no wait', async () => { const req = openRequest({ waitMs: 30_000 }); - const store = storeWithHolder(); + const store = makeSessionStore('agent-device-open-wait-'); + const looks = vi.spyOn(store, 'findByDevice'); - await waitForOpenDeviceContention({ + const wait = beginOpenDeviceWait({ req, - sessionName: HOLDER_ADDRESS, + sessionName: OPENER_ADDRESS, sessionStore: store, - budgetMs: 30_000, - resolveDevice: resolved(IOS_SIMULATOR).resolve, - }); + deviceId: IOS_SIMULATOR.id, + })!; + await wait.waitForDeviceOutsideLocks(); + expect(looks).toHaveBeenCalledTimes(1); expect(readOpenWaitAttempt(req)).toEqual({}); }); -test('a device that frees up ends the wait and records what it cost', async () => { +test('a device that frees up ends the wait without claiming a spent budget', async () => { vi.useFakeTimers(); const req = openRequest({ waitMs: 30_000 }); const store = storeWithHolder(); - let looks = 0; + const release = setTimeout(() => store.delete(HOLDER_ADDRESS), 600); + + await waitUntil( + () => + beginOpenDeviceWait({ + req, + sessionName: OPENER_ADDRESS, + sessionStore: store, + deviceId: IOS_SIMULATOR.id, + })!.waitForDeviceOutsideLocks(), + 30_000, + ); + clearTimeout(release); + + // The open this wait hands off to succeeds, and only a refusal may report a wait. + expect(readOpenWaitAttempt(req)).toEqual({}); +}); - let settled = false; - const running = waitForOpenDeviceContention({ +test('an open yields the device lock to a session that took the device after the last look', async () => { + vi.useFakeTimers(); + const req = openRequest({ waitMs: 30_000 }); + const store = makeSessionStore('agent-device-open-wait-'); + const wait = beginOpenDeviceWait({ req, - sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionName: OPENER_ADDRESS, sessionStore: store, - budgetMs: 30_000, - resolveDevice: async () => { - looks += 1; - if (looks > 2) store.delete(HOLDER_ADDRESS); - return IOS_SIMULATOR; + deviceId: IOS_SIMULATOR.id, + })!; + // A competing open puts its session on the device in the window between this open's look at the + // free store and its first pass under the locks, and hands it back 300ms later. + setTimeout(() => store.delete(HOLDER_ADDRESS), 300); + const trace = lockTrace({ + onAcquire: (pass) => { + if (pass === 1) store.set(HOLDER_ADDRESS, session(HOLDER_ADDRESS)); }, - }).then(() => { - settled = true; }); + + let opened = 0; + const running = wait + .runWhenDeviceIsUnheld({ + acquireLocks: trace.acquireLocks, + task: async () => { + opened += 1; + return 'opened'; + }, + }) + .then((outcome) => { + expect(outcome).toBe('opened'); + }); await vi.advanceTimersByTimeAsync(1_000); await running; - expect(settled).toBe(true); - expect(readOpenWaitAttempt(req).waitedMs).toBeGreaterThan(0); + // Two passes at the locks with one wait between them, and the open ran only while it held them. + expect(trace.order).toEqual(['acquire', 'release', 'acquire', 'release']); + expect(opened).toBe(1); + expect(readOpenWaitAttempt(req)).toEqual({}); }); -test('a budget that runs out busy records the spend and lets the open refuse', async () => { +test('a budget that runs out busy costs the whole budget, then lets the open refuse', async () => { vi.useFakeTimers(); const req = openRequest({ waitMs: 1000 }); - const device = resolved(IOS_SIMULATOR); - - const running = waitForOpenDeviceContention({ + const store = storeWithHolder(); + const looks = vi.spyOn(store, 'findByDevice'); + const wait = beginOpenDeviceWait({ req, - sessionName: 'cwd:1d9b7c2f4a6e8b03:default', - sessionStore: storeWithHolder(), - budgetMs: 1000, - resolveDevice: device.resolve, - }); + sessionName: OPENER_ADDRESS, + sessionStore: store, + deviceId: IOS_SIMULATOR.id, + })!; + const trace = lockTrace({}); + + let opened = 0; + const running = wait + .runWhenDeviceIsUnheld({ + acquireLocks: trace.acquireLocks, + task: async () => { + opened += 1; + return 'refused'; + }, + }) + .then((outcome) => { + expect(outcome).toBe('refused'); + }); await vi.advanceTimersByTimeAsync(10_000); await running; expect(readOpenWaitAttempt(req).waitedMs).toBe(1000); - // 250ms polls across a 1000ms budget: never an unbounded spin. - expect(device.calls()).toBe(5); -}); - -test('a device that cannot be resolved yet is not waited for', async () => { - const req = openRequest({ waitMs: 30_000 }); - const device = resolved(undefined); - - await waitForOpenDeviceContention({ - req, - sessionName: 'cwd:1d9b7c2f4a6e8b03:default', - sessionStore: storeWithHolder(), - budgetMs: 30_000, - resolveDevice: device.resolve, - }); - - expect(device.calls()).toBe(1); - expect(readOpenWaitAttempt(req)).toEqual({}); + // The refusal is the open's own, so the task ran — once, under the locks, after the budget that + // bought the second pass was gone. + expect(opened).toBe(1); + expect(trace.order).toEqual(['acquire', 'release', 'acquire', 'release']); + // 250ms polls across a 1000ms budget, plus one look per pass at the locks: never a spin. + expect(looks.mock.calls.length).toBeLessThanOrEqual(7); }); test('a request the client gave up on stops waiting at its next poll', async () => { vi.useFakeTimers(); const req = openRequest({ waitMs: 60_000 }); markRequestCanceled('req-open-wait'); - - const running = waitForOpenDeviceContention({ + const wait = beginOpenDeviceWait({ req, - sessionName: 'cwd:1d9b7c2f4a6e8b03:default', + sessionName: OPENER_ADDRESS, sessionStore: storeWithHolder(), - budgetMs: 60_000, - resolveDevice: resolved(IOS_SIMULATOR).resolve, - }); - const rejection = expect(running).rejects.toThrow(); + deviceId: IOS_SIMULATOR.id, + })!; + + const rejection = expect(wait.waitForDeviceOutsideLocks()).rejects.toSatisfy( + isRequestCanceledError, + ); await vi.advanceTimersByTimeAsync(250); await rejection; }); + +/** Runs `start()` under fake timers and advances the clock until it settles. */ +async function waitUntil(start: () => Promise, budgetMs: number): Promise { + const running = start(); + await vi.advanceTimersByTimeAsync(budgetMs); + await running; +} diff --git a/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts b/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts new file mode 100644 index 0000000000..db5273ce0c --- /dev/null +++ b/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts @@ -0,0 +1,165 @@ +import { afterAll, test, expect, vi } from 'vitest'; +import fs from 'node:fs'; +import type { CommandFlags } from '@agent-device/contracts/command'; +import type { DeviceInfo } from '@agent-device/kernel/device'; +import { makeSession } from '../../__tests__/test-utils/session-factories.ts'; +import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; +import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; +import type { DaemonRequest } from '../daemon-request.ts'; +import { LeaseRegistry } from '../lease-registry.ts'; +import { readOpenWaitAttempt } from '../open-device-contention-wait.ts'; +import { createRequestExecutionScope } from '../request-execution-scope.ts'; + +// Everything `open --wait ` is for sits between two facts. A waiting open must not hold the +// device execution lock, because `close` — the thing that could free the device — needs that same +// lock. And a waiting open must look at the device again once it owns the lock, because the device +// can be taken in the window between the last look and the lock. These drive two real request +// scopes over one lock map, so both halves are pinned together. + +const CONTESTED_DEVICE = vi.hoisted(() => ({ + platform: 'apple', + id: 'sim-contested', + name: 'Contested iPhone', + kind: 'simulator', + appleOs: 'ios', + booted: true, +})) as DeviceInfo; + +vi.mock('@agent-device/device-selection/dispatch-resolve', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + resolveTargetDevice: vi.fn(async () => CONTESTED_DEVICE), + }; +}); + +// The smallest budget the option allows, with room to tell "opened after re-waiting" apart from +// "spent the whole budget and refused" on a wall clock. +const WAIT_BUDGET_MS = 1_000; + +const TEST_ROOT = mkdtempForTestSync('agent-device-open-device-wait-'); + +function openRequest(session: string): DaemonRequest { + const flags: CommandFlags = { waitMs: WAIT_BUDGET_MS }; + return { + token: 'token', + session, + command: 'open', + positionals: [], + flags, + meta: { cwd: TEST_ROOT, requestId: `req-${session}` }, + }; +} + +function closeRequest(session: string): DaemonRequest { + return { + token: 'token', + session, + command: 'close', + positionals: [], + meta: { cwd: TEST_ROOT, requestId: `req-${session}-close` }, + }; +} + +function sessionOnDevice(name: string) { + return makeSession(name, { device: CONTESTED_DEVICE }); +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +// Two opens waiting on one device can both see it free on the same poll. The loser of that race +// still has most of its budget, and the device is reachably its own as soon as the winner hands it +// back, so refusing it where it stands is the bug this pins shut. +test('an open that lost the race to a free device re-waits and opens rather than refusing', async () => { + const sessionStore = makeSessionStore('agent-device-open-wait-race-'); + const leaseRegistry = new LeaseRegistry(); + sessionStore.set('holder', sessionOnDevice('holder')); + // The holder lets go shortly after; the first open to bind the device lets go again after that, + // the way any session closed by its owner would. + setTimeout(() => sessionStore.delete('holder'), 50); + setTimeout(() => sessionStore.delete('first-opener'), 400); + + const first = await createRequestExecutionScope({ + req: openRequest('first-opener'), + sessionStore, + leaseRegistry, + }); + const second = await createRequestExecutionScope({ + req: openRequest('second-opener'), + sessionStore, + leaseRegistry, + }); + + const order: string[] = []; + const startedAtMs = Date.now(); + // Each open binds the device to its own session under its own locks, which is what a real open + // does — the second one can only get in once the first releases it. + const firstOpened = first.runLocked(async () => { + sessionStore.set('first-opener', sessionOnDevice('first-opener')); + order.push('first'); + return 'first-opened'; + }); + const secondOpened = second.runLocked(async () => { + sessionStore.set('second-opener', sessionOnDevice('second-opener')); + order.push('second'); + return 'second-opened'; + }); + + await expect(firstOpened).resolves.toBe('first-opened'); + await expect(secondOpened).resolves.toBe('second-opened'); + expect(order).toEqual(['first', 'second']); + + // Both opened inside the budget, so neither owes a caller a story about one that ran out. + expect(Date.now() - startedAtMs).toBeLessThan(WAIT_BUDGET_MS); + expect(readOpenWaitAttempt(first.req)).toEqual({}); + expect(readOpenWaitAttempt(second.req)).toEqual({}); +}); + +// An open that waited while holding the device lock would stall the only command able to end its +// wait. So the open's locked work is already in flight when the `close` starts, and the close still +// has to get through — well inside a budget the waiting open has no hope of finishing meantime. +test('a close that frees the device mid-wait gets through while the open is waiting', async () => { + const sessionStore = makeSessionStore('agent-device-open-wait-close-'); + const leaseRegistry = new LeaseRegistry(); + sessionStore.set('holder', sessionOnDevice('holder')); + const order: string[] = []; + + const opened = createRequestExecutionScope({ + req: openRequest('waiter'), + sessionStore, + leaseRegistry, + }).then((scope) => + scope.runLocked(async () => { + order.push('open-bound'); + sessionStore.set('waiter', sessionOnDevice('waiter')); + return 'opened'; + }), + ); + await sleep(50); + const closer = await createRequestExecutionScope({ + req: closeRequest('holder'), + sessionStore, + leaseRegistry, + }); + + const closedAtMs = Date.now(); + await expect( + closer.runLocked(async () => { + order.push('close-freed-the-device'); + sessionStore.delete('holder'); + return 'closed'; + }), + ).resolves.toBe('closed'); + expect(Date.now() - closedAtMs).toBeLessThan(WAIT_BUDGET_MS); + + await expect(opened).resolves.toBe('opened'); + expect(order).toEqual(['close-freed-the-device', 'open-bound']); +}); + +afterAll(() => { + vi.useRealTimers(); + fs.rmSync(TEST_ROOT, { recursive: true, force: true }); +}); diff --git a/src/daemon/__tests__/request-router-open.test.ts b/src/daemon/__tests__/request-router-open.test.ts index a6ab97541c..3c946449fb 100644 --- a/src/daemon/__tests__/request-router-open.test.ts +++ b/src/daemon/__tests__/request-router-open.test.ts @@ -72,7 +72,7 @@ import { lifecycleDeviceRuntimeGateway, } from './test-device-runtime-gateway.ts'; import { createRequestHandler as createProductionRequestHandler } from '../request-router.ts'; -import { resolveRequestExecutionLockKeys } from '../request-binding.ts'; +import { resolveRequestExecutionLockPlan } from '../request-binding.ts'; import { LeaseRegistry } from '../lease-registry.ts'; import { ensureDeviceReady } from '../device-ready.ts'; import { @@ -216,7 +216,7 @@ test('fresh replay reserves its authored app simulator before any replay step', options?.appleSimulatorAppTarget === 'com.example.demo' ? appDevice : genericDevice, ); - const keys = await resolveRequestExecutionLockKeys({ + const { keys } = await resolveRequestExecutionLockPlan({ req: { token: 'test-token', session: 'fresh-replay', @@ -245,7 +245,7 @@ test('fresh replay leaves a first deep-link open unbound when a later app target ); const sessionStore = makeSessionStore('agent-device-router-replay-deep-link-lock-'); - const keys = await resolveRequestExecutionLockKeys({ + const { keys } = await resolveRequestExecutionLockPlan({ req: { token: 'test-token', session: 'fresh-replay-deep-link', @@ -273,7 +273,7 @@ test('fresh replay preserves an authored Android platform before advisory lockin const androidDevice = makeAndroidDevice('ANDROID-EMULATOR'); mockResolveTargetDevice.mockResolvedValue(androidDevice); - const keys = await resolveRequestExecutionLockKeys({ + const { keys } = await resolveRequestExecutionLockPlan({ req: { token: 'test-token', session: 'fresh-replay-android', diff --git a/src/daemon/open-device-contention-wait.ts b/src/daemon/open-device-contention-wait.ts index 657d464a40..abf0a7077e 100644 --- a/src/daemon/open-device-contention-wait.ts +++ b/src/daemon/open-device-contention-wait.ts @@ -1,4 +1,5 @@ -import type { DeviceInfo } from '@agent-device/kernel/device'; +import { getFlagDefinitionsForKey } from '@agent-device/command-registry/flag-registry'; +import { readOptionalInteger } from '@agent-device/contracts/command'; import { emitRequestProgress, throwIfRequestCanceled } from '@agent-device/host-kit/request'; import { Deadline, sleep } from '@agent-device/host-kit/retry'; import type { DaemonRequest } from './daemon-request.ts'; @@ -13,13 +14,19 @@ import type { SessionRecoveryOptions } from './session-recovery-hints.ts'; const DEVICE_CONTENTION_POLL_MS = 250; /** - * The `--wait ` budget an open carries, or `undefined` when it must not block. A non-positive - * budget is the caller's own spelling of "don't block", so it is not clamped up to the flag's - * declared minimum. + * The `--wait ` budget an open carries, or `undefined` when it must not block. + * + * The bounds are the option's own declaration. The CLI parser refuses a value outside them and so + * does the tool input derived from it, but a budget built by the Node client or posted straight to + * the wire has no other reader standing between it and a device, so this reader checks the + * declaration instead of trusting whichever surface assembled the request. */ export function readOpenWaitBudgetMs(req: DaemonRequest): number | undefined { - const budgetMs = req.flags?.waitMs; - return typeof budgetMs === 'number' && budgetMs > 0 ? budgetMs : undefined; + const declaration = getFlagDefinitionsForKey('waitMs')[0]; + return readOptionalInteger((req.flags ?? {}) as Record, 'waitMs', { + min: declaration?.min, + max: declaration?.max, + }); } /** @@ -27,7 +34,8 @@ export function readOpenWaitBudgetMs(req: DaemonRequest): number | undefined { * * The spend rides on `internal`, which the transport strips, so only the daemon that waited can * report a wait and the recovery text can say the budget was spent without a client claiming one - * that never happened. + * that never happened. It is recorded only when a budget ran out with a holder still on the + * device, which is the one story a refusal can tell honestly. */ export function readOpenWaitAttempt(req: DaemonRequest): SessionRecoveryOptions { const waitedMs = req.internal?.openDeviceWait?.waitedMs; @@ -35,65 +43,140 @@ export function readOpenWaitAttempt(req: DaemonRequest): SessionRecoveryOptions } /** - * Spend an open's `--wait` budget waiting for the device it is about to bind, and record what - * that cost. - * - * This runs before the request takes the device's execution lock, which is the whole point: the - * operations that free a contended device — `close`, `record stop`, the holder's own commands — - * need that same lock, so an open that waited while holding it would block the only thing that - * could end its wait. Whatever is decided about the device afterwards stays with the locked open - * path, which re-checks and refuses on its own authority; this wait only declines to queue for - * the lock while the device is visibly spoken for. + * What a `DEVICE_IN_USE` refusal may say about `--wait`, for the command that owns it. The spend + * rides along when there was one, and the flag itself is offered only to a caller that did not + * already arrive with it: an open that came with a budget is not helped by being told to run the + * same open with the flag it used, and an interaction has no budget to offer in the first place. + */ +export function describeOpenWaitForRefusal(req: DaemonRequest): SessionRecoveryOptions { + return { + ...readOpenWaitAttempt(req), + offersDeviceWait: typeof req.flags?.waitMs !== 'number', + }; +} + +export type OpenDeviceWait = { + /** + * Spend what is left of the budget waiting for the session that holds the device to let go. + * + * The caller must not hold this request's execution locks here, which is the whole point of the + * arrangement: the operations that could free a contended device — `close`, `record stop`, the + * holder's own commands — need that same device lock, so an open that waited while holding it + * would block the only things that could end its wait. + * + * Only another session's claim on the device is waited for. A host-global device claim held by + * another workspace's daemon is not: that refusal names an operator command rather than a + * retry, and a budget spent on it buys nothing. + */ + waitForDeviceOutsideLocks(): Promise; + /** + * Run the open inside `acquireLocks`, and hand those locks back when another session took the + * device in the window between the last look and the lock. + * + * The look inside `acquireLocks` is what closes that window: an open can only put its session on + * a device by writing the session store, and it writes that store while holding the same device + * lock this request holds. So either nobody holds the device for as long as `task` runs, or + * this open gives the device back and spends the rest of its budget waiting again — which is + * what lets several callers queue on one device instead of all but the fastest refusing early. + */ + runWhenDeviceIsUnheld(params: { + acquireLocks: (task: () => Promise) => Promise; + task: () => Promise; + }): Promise; +}; + +/** + * The `--wait ` discipline for this request, or `undefined` when nothing about it may block. + * Only a fresh open carries one: an open onto a session that already exists is bound to a device + * nobody else is being refused for, and a request that did not ask to wait is answered at once. * - * Only another session's claim on the device is waited for. A host-global device claim held by - * another workspace's daemon is not: that refusal names an operator command rather than a retry, - * and a budget spent on it buys nothing. + * `deviceId` is the device the request's own execution-lock plan reserves, so the wait and the + * lock agree on which device is being waited for without resolving a target a second time. */ -export async function waitForOpenDeviceContention(params: { +export function beginOpenDeviceWait(params: { req: DaemonRequest; sessionName: string; sessionStore: SessionStore; - budgetMs: number; - resolveDevice: () => Promise; -}): Promise { - const deadline = Deadline.fromTimeoutMs(params.budgetMs); - let waitedMs = 0; - for (;;) { - const device = await params.resolveDevice(); - const holder = device - ? findSessionHoldingDevice(params.sessionStore, device.id, params.sessionName) - : undefined; - if (!holder || deadline.isExpired()) { - recordOpenWaitSpend(params.req, waitedMs); - return; - } - emitRequestProgress({ - type: 'command', - status: 'progress', - message: `Waiting for ${device!.name}: held by session "${holder}" (${Math.round( - waitedMs, - )}ms of the ${Math.round(params.budgetMs)}ms wait budget used)`, - }); - await sleep(Math.min(DEVICE_CONTENTION_POLL_MS, deadline.remainingMs())); - waitedMs = deadline.elapsedMs(); - recordOpenWaitSpend(params.req, waitedMs); - // A client that gave up on the request must not leave the daemon queueing for a device - // nobody wants any more. Checked after the poll rather than before the first look because - // whether an already-canceled request runs at all is the request pipeline's own question. - throwIfRequestCanceled(params.req.meta?.requestId); - } + deviceId: string | undefined; +}): OpenDeviceWait | undefined { + const { req, sessionName, sessionStore, deviceId } = params; + if (req.command !== 'open' || deviceId === undefined) return undefined; + if (sessionStore.get(sessionName)) return undefined; + const budgetMs = readOpenWaitBudgetMs(req); + if (budgetMs === undefined) return undefined; + return createOpenDeviceWait({ req, sessionName, sessionStore, deviceId, budgetMs }); } -/** The other session holding `deviceId`, or `undefined` when nothing stands between this open - * and the device — including when this open's own session is what holds it. */ +/** The other session holding `deviceId`, or `undefined` when nothing stands between this open and + * the device — including when this open's own session is what holds it. */ function findSessionHoldingDevice( sessionStore: SessionStore, deviceId: string, sessionName: string, -): string | undefined { +): ReturnType { const inUse = sessionStore.findByDevice(deviceId); if (!inUse || inUse.address === sessionName) return undefined; - return inUse.address; + return inUse; +} + +type LockedAttempt = { ran: true; outcome: Outcome } | { ran: false }; + +function createOpenDeviceWait(params: { + req: DaemonRequest; + sessionName: string; + sessionStore: SessionStore; + deviceId: string; + budgetMs: number; +}): OpenDeviceWait { + const { req, sessionName, sessionStore, deviceId, budgetMs } = params; + const deadline = Deadline.fromTimeoutMs(budgetMs); + const holder = () => findSessionHoldingDevice(sessionStore, deviceId, sessionName); + + const waitForDeviceOutsideLocks = async (): Promise => { + for (;;) { + const holding = holder(); + // Either nobody is on the device and this open goes looking for it under the locks, or the + // budget is gone and the locks are where the refusal gets its say. Both end the wait. + if (!holding || deadline.isExpired()) return; + emitRequestProgress({ + type: 'command', + status: 'progress', + message: `Waiting for ${holding.session.device.name}: held by session "${holding.address}" (${Math.round( + deadline.elapsedMs(), + )}ms of the ${Math.round(budgetMs)}ms wait budget used)`, + }); + await sleep(Math.min(DEVICE_CONTENTION_POLL_MS, deadline.remainingMs())); + // A client that gave up on the request must not leave the daemon queueing for a device + // nobody wants any more. Checked after the poll rather than before the first look because + // whether an already-canceled request runs at all is the request pipeline's own question. + throwIfRequestCanceled(req.meta?.requestId); + } + }; + + return { + waitForDeviceOutsideLocks, + runWhenDeviceIsUnheld: async (params: { + acquireLocks: (task: () => Promise) => Promise; + task: () => Promise; + }): Promise => { + const { acquireLocks, task } = params; + for (;;) { + const attempt = await acquireLocks>(async () => { + if (holder()) { + if (!deadline.isExpired()) return { ran: false }; + // The refusal this open is about to get can say the budget was spent, because under + // these locks it is: nobody else can hand the device over any more. + recordOpenWaitSpend(req, deadline.elapsedMs()); + } + return { ran: true, outcome: await task() }; + }); + if (attempt.ran) return attempt.outcome; + // The device was taken after the look that let this open reach the locks. Leaving them + // go is what keeps the session that took it able to close and hand the device back. + await waitForDeviceOutsideLocks(); + } + }, + }; } function recordOpenWaitSpend(req: DaemonRequest, waitedMs: number): void { diff --git a/src/daemon/request-binding.ts b/src/daemon/request-binding.ts index 1ac0326f3b..d8c0ef622b 100644 --- a/src/daemon/request-binding.ts +++ b/src/daemon/request-binding.ts @@ -10,39 +10,43 @@ import type { SessionRef } from './session-state.ts'; export type RequestExecutionLockKey = `session:${string}` | `device:${string}`; +export type RequestExecutionLockPlan = { + keys: RequestExecutionLockKey[]; + /** + * The device {@link RequestExecutionLockPlan.keys} reserves, or `undefined` when the plan locks + * only the session. A caller that must look at this device before it holds the lock reads it + * here rather than resolving a target of its own, so one request has one answer to "which + * device am I waiting for"; the locked request still resolves and binds its device alone. + */ + deviceId: string | undefined; +}; + export type LockedRequestBinding = { req: DaemonRequest; existingRef: SessionRef | undefined; }; -export async function resolveRequestExecutionLockKeys(params: { +export async function resolveRequestExecutionLockPlan(params: { req: DaemonRequest; sessionName: string; sessionStore: SessionStore; -}): Promise { +}): Promise { const { req, sessionName, sessionStore } = params; const existingSession = sessionStore.get(sessionName); if (existingSession) { - return orderRequestExecutionLockKeys([ - sessionExecutionLockKey(sessionName), - deviceExecutionLockKey(existingSession.device.id), - ]); + return { + keys: orderRequestExecutionLockKeys([ + sessionExecutionLockKey(sessionName), + deviceExecutionLockKey(existingSession.device.id), + ]), + deviceId: existingSession.device.id, + }; } const keys = new Set([sessionExecutionLockKey(sessionName)]); - const bindingReq = resolveFreshSessionBindingRequest(req); - const resolution = resolveFreshSessionDeviceLock(bindingReq); - if (resolution) { - try { - // This is advisory lock selection before the request enters the lock; the - // locked request still resolves and binds the target device authoritatively. - const device = await resolveTargetDevice(resolution.flags, resolution.options); - keys.add(deviceExecutionLockKey(device.id)); - } catch { - // Fall back to session scoping when device resolution is not yet available. - } - } - return orderRequestExecutionLockKeys(keys); + const device = await resolveFreshSessionDevice(req); + if (device) keys.add(deviceExecutionLockKey(device.id)); + return { keys: orderRequestExecutionLockKeys(keys), deviceId: device?.id }; } export function prepareLockedRequestBinding(params: { @@ -80,16 +84,12 @@ function resolveFreshSessionDeviceLock(req: DaemonRequest): } /** - * The device an `open` is expected to bind, resolved the same advisory way its device execution - * lock is chosen — before the lock is held, and never as an authority on what happens next. A - * caller that must not hold the device lock uses this to look at contention; the locked open path - * still resolves and binds the target device itself. + * The device a fresh session is expected to bind, resolved the advisory way its device execution + * lock is chosen. This runs before the request holds that lock and is never an authority on what + * happens after: the locked request resolves and binds its device on its own. */ -export async function resolveAdvisoryOpenDevice( - req: DaemonRequest, -): Promise { - if (req.command !== 'open') return undefined; - const resolution = resolveOpenDeviceLock(resolveFreshSessionBindingRequest(req)); +async function resolveFreshSessionDevice(req: DaemonRequest): Promise { + const resolution = resolveFreshSessionDeviceLock(resolveFreshSessionBindingRequest(req)); if (!resolution) return undefined; try { return await resolveTargetDevice(resolution.flags, resolution.options); diff --git a/src/daemon/request-execution-scope.ts b/src/daemon/request-execution-scope.ts index 30acf9c00a..fd1df3676f 100644 --- a/src/daemon/request-execution-scope.ts +++ b/src/daemon/request-execution-scope.ts @@ -21,13 +21,10 @@ import { } from './lease-lifecycle.ts'; import { prepareLockedRequestBinding, - resolveAdvisoryOpenDevice, - resolveRequestExecutionLockKeys, + resolveRequestExecutionLockPlan, + type RequestExecutionLockPlan, } from './request-binding.ts'; -import { - readOpenWaitBudgetMs, - waitForOpenDeviceContention, -} from './open-device-contention-wait.ts'; +import { beginOpenDeviceWait } from './open-device-contention-wait.ts'; import { createRequestExecutionLocks } from './request-execution-locks.ts'; import { throwIfRequestCanceled } from '@agent-device/host-kit/request'; import { finalizeDaemonResponse } from './request-finalization.ts'; @@ -120,30 +117,6 @@ export type LockedRequestScopeResult = | { type: 'scope'; scope: LockedRequestScope } | { type: 'response'; response: DaemonResponse }; -/** - * Spend an `--wait ` budget on a contended device while the request still has no locks. Must - * run before {@link resolveRequestExecutionLockKeys}: the device's execution lock is what every - * operation that could free the device also needs, so waiting after taking it would have an open - * block its own recovery. A fresh open only — an open onto a session that already exists is bound - * to a device nobody else is being refused. - */ -async function waitForOpenDeviceBeforeLock( - req: DaemonRequest, - sessionName: string, - sessionStore: SessionStore, -): Promise { - if (req.command !== 'open' || sessionStore.get(sessionName)) return; - const budgetMs = readOpenWaitBudgetMs(req); - if (budgetMs === undefined) return; - await waitForOpenDeviceContention({ - req, - sessionName, - sessionStore, - budgetMs, - resolveDevice: () => resolveAdvisoryOpenDevice(req), - }); -} - export async function createRequestExecutionScope(params: { req: DaemonRequest; sessionStore: SessionStore; @@ -203,14 +176,23 @@ export async function createRequestExecutionScope(params: { } try { assertLockedLeaseAdmissionPreflight(scopedReq); - await waitForOpenDeviceBeforeLock(scopedReq, sessionName, sessionStore); - const executionLockKeys = shouldLockSessionExecution(command) - ? await resolveRequestExecutionLockKeys({ req: scopedReq, sessionName, sessionStore }) - : []; + const lockPlan: RequestExecutionLockPlan = shouldLockSessionExecution(command) + ? await resolveRequestExecutionLockPlan({ req: scopedReq, sessionName, sessionStore }) + : { keys: [], deviceId: undefined }; + // An `--wait ` open spends the first of its budget here, while the request holds no locks + // yet: the device execution lock is what every operation that could free the device also + // needs, so waiting after taking it would have an open block its own recovery. + const openWait = beginOpenDeviceWait({ + req: scopedReq, + sessionName, + sessionStore, + deviceId: lockPlan.deviceId, + }); + await openWait?.waitForDeviceOutsideLocks(); const executionLocks = getLeaseRegistryExecutionLocks(leaseRegistry); const requestExecutionLocks = createRequestExecutionLocks({ locks: executionLocks, - initialKeys: executionLockKeys, + initialKeys: lockPlan.keys, }); const { claimAdmission, runtimeBindings } = createRequestDeviceAccess({ command, @@ -285,7 +267,13 @@ export async function createRequestExecutionScope(params: { }, runLocked: async (task) => { throwIfRequestCanceled(scopedReq.meta?.requestId); - return await requestExecutionLocks.run(async () => await scope.runAdmitted(task)); + if (!openWait) { + return await requestExecutionLocks.run(async () => await scope.runAdmitted(task)); + } + return await openWait.runWhenDeviceIsUnheld({ + acquireLocks: requestExecutionLocks.run, + task: async () => await scope.runAdmitted(task), + }); }, // Claims outlive the bindings they guard: release only once no device // operation from this request can still run. diff --git a/src/daemon/session-lifecycle/internal/session-open-execution.ts b/src/daemon/session-lifecycle/internal/session-open-execution.ts index da582d48ef..9e23944b6a 100644 --- a/src/daemon/session-lifecycle/internal/session-open-execution.ts +++ b/src/daemon/session-lifecycle/internal/session-open-execution.ts @@ -41,7 +41,7 @@ import { buildDeviceInUseBySessionError, buildForeignWorkspaceSessionConflict, } from '../../session-recovery-hints.ts'; -import { readOpenWaitAttempt } from '../../open-device-contention-wait.ts'; +import { describeOpenWaitForRefusal } from '../../open-device-contention-wait.ts'; import { isImplicitSessionScopeConflict, resolveSessionScope, @@ -398,9 +398,9 @@ function findNewSessionDeviceConflict(params: { const { req, device, sessionStore } = params; const inUse = sessionStore.findByDevice(device.id); if (!inUse) return undefined; - // `offersDeviceWait` belongs to this producer alone: an interaction that hits the same busy + // The wait the caller paid for belongs to `open` alone: an interaction that hits the same busy // device cannot wait for it, and would be sent off with a flag its own command rejects. - const attempt = { ...readOpenWaitAttempt(req), offersDeviceWait: true } as const; + const attempt = describeOpenWaitForRefusal(req); if (isImplicitSessionScopeConflict(req, inUse.session)) { return buildForeignWorkspaceSessionConflict(inUse, device, attempt); } diff --git a/test/integration/nightly/concurrency-torture.test.ts b/test/integration/nightly/concurrency-torture.test.ts index f9265ef95f..a3b5bc5a18 100644 --- a/test/integration/nightly/concurrency-torture.test.ts +++ b/test/integration/nightly/concurrency-torture.test.ts @@ -6,7 +6,7 @@ // `SessionStore` + `LeaseRegistry`, an in-memory device-claim model), with ALL // concurrency routed through a deterministic scheduler so a seed fully // determines execution order. Each operation's lock plan is derived from the -// production router primitive `resolveRequestExecutionLockKeys`, so reverting +// production router primitive `resolveRequestExecutionLockPlan`, so reverting // the router's same-device serialization trips the overlap invariant. After // every run the harness asserts: // - no leaked leases or claims, diff --git a/test/integration/nightly/concurrency-torture/bindings.ts b/test/integration/nightly/concurrency-torture/bindings.ts index 090a5f3fcc..06fc88d0b1 100644 --- a/test/integration/nightly/concurrency-torture/bindings.ts +++ b/test/integration/nightly/concurrency-torture/bindings.ts @@ -1,7 +1,7 @@ // Production lock-plan + device bindings for the concurrency torture lane (#1416). // // The lane does NOT hand-write which locks an operation takes. It derives the -// plan from the REAL router primitive `resolveRequestExecutionLockKeys` +// plan from the REAL router primitive `resolveRequestExecutionLockPlan` // (src/daemon/request-binding.ts), driven with a fake in-memory device // inventory via the production `withDeviceInventoryProvider` seam. This is what // makes the same-device serialization invariant revert-sensitive: if production @@ -12,7 +12,7 @@ // The plan is built the SAME way the daemon builds it in // `createRequestExecutionScope`: gate on the production decision // `shouldLockSessionExecution(command)`, and only then resolve keys via -// `resolveRequestExecutionLockKeys`. So the lane is revert-sensitive to BOTH +// `resolveRequestExecutionLockPlan`. So the lane is revert-sensitive to BOTH // production decision points — exempting a command from execution locking, or // dropping the device key — even though the mutex GRANT itself stays modeled // (by the deterministic scheduler) because `withKeyedLock`'s native microtask @@ -22,7 +22,7 @@ import type { DeviceInfo } from '@agent-device/kernel/device'; import type { CommandFlags } from '@agent-device/contracts/command'; import type { DaemonRequest } from '../../../../src/daemon/daemon-request.ts'; import type { SessionStore } from '../../../../src/daemon/session-store.ts'; -import { resolveRequestExecutionLockKeys } from '../../../../src/daemon/request-binding.ts'; +import { resolveRequestExecutionLockPlan } from '../../../../src/daemon/request-binding.ts'; import { shouldLockSessionExecution } from '../../../../src/daemon/daemon-command-registry.ts'; import { PUBLIC_COMMANDS } from '@agent-device/command-registry/catalog'; import { withTestDeviceInventoryProvider as withDeviceInventoryProvider } from '../../../../src/__tests__/test-utils/device-inventory-gateways.ts'; @@ -131,9 +131,10 @@ async function resolveExecutionLockPlan( sessionStore: SessionStore, ): Promise { if (!shouldLockSessionExecution(req.command)) return []; - return (await withPool(() => - resolveRequestExecutionLockKeys({ req, sessionName, sessionStore }), - )) as LockKey[]; + const plan = await withPool(() => + resolveRequestExecutionLockPlan({ req, sessionName, sessionStore }), + ); + return plan.keys as LockKey[]; } /** diff --git a/test/integration/nightly/concurrency-torture/harness.ts b/test/integration/nightly/concurrency-torture/harness.ts index 0c31e9adff..f300b586c9 100644 --- a/test/integration/nightly/concurrency-torture/harness.ts +++ b/test/integration/nightly/concurrency-torture/harness.ts @@ -11,7 +11,7 @@ // - REAL: `SessionStore` (session map/consistency) and `LeaseRegistry` (lease // allocation, per-device exclusivity, release, scope checks). // - REAL lock PLAN: every operation's lock keys come from the production -// router primitive `resolveRequestExecutionLockKeys` (see bindings.ts), +// router primitive `resolveRequestExecutionLockPlan` (see bindings.ts), // driven with a fake device inventory. Only the mutex GRANT is modeled by // the scheduler, because `withKeyedLock`'s native microtask hand-off cannot // be reproduced from a seed. Reverting the router's same-device diff --git a/website/docs/docs/commands.md b/website/docs/docs/commands.md index 82908ae188..be2d5d9d78 100644 --- a/website/docs/docs/commands.md +++ b/website/docs/docs/commands.md @@ -70,7 +70,7 @@ agent-device app-switcher - Android: `shutdown --platform android --device ` stops a running emulator. - `open [app|url] [url]` already boots/activates the selected target when needed. - `open --timeout ` is a startup budget for that boot. A never-booted iOS Simulator runs Apple's first-boot migration, which can take several minutes; without the flag the boot wait is capped at 120 seconds. When the budget runs out the command fails with `error.details.reason: boot_timeout` and the Simulator keeps booting, so a retry finds it further along. -- `open --wait ` waits up to that budget for a device another session is holding instead of failing at once. The open reports each poll, then either opens the device or fails with `DEVICE_IN_USE` naming the owning session and saying the budget was spent. Only session contention is waited for: a device claim held by another workspace's daemon is never retriable and returns its recovery command immediately. The wait extends the command's timeout envelope, so a long budget does not need a longer `--timeout`. +- `open --wait ` waits up to that budget for a device another session is holding instead of failing at once. The open reports each poll, then either opens the device or fails with `DEVICE_IN_USE` naming the owning session and saying the budget was spent. A wait that finds the device taken again keeps waiting for the rest of its budget, so several opens can queue on one device and none of them is refused before its budget is spent. Only session contention is waited for: a device claim held by another workspace's daemon is never retriable and returns its recovery command immediately. The wait extends the command's timeout envelope, so a long budget does not need a longer `--timeout`. - `open ` deep links are supported on Android and iOS. - `open ` opens a deep link on iOS. - `open --launch-console ` captures launch-time stdout/stderr for direct iOS simulator app launches. It is not valid for URL opens or diff --git a/website/docs/docs/sessions.md b/website/docs/docs/sessions.md index 4834f53a22..6b5eafe804 100644 --- a/website/docs/docs/sessions.md +++ b/website/docs/docs/sessions.md @@ -79,7 +79,7 @@ When the device is held by a session that will let go soon — a parallel agent agent-device open Demo --platform android --wait 60000 ``` -If the budget runs out the device is still busy, the open fails with `DEVICE_IN_USE` naming the owning session address, which `close --session
` releases. +If the budget runs out with the device still busy, the open fails with `DEVICE_IN_USE` naming the owning session address, which `close --session
` releases. Every open waiting on a device gets its whole budget: when the holder lets go, the waiting open that did not get the device waits again for what is left of its own budget rather than refusing early, so several agents can queue on one device. Notes: From 8c8d62bfa1b245d1a6147b11fb51ce9ba24b05c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 18 Sep 2026 16:48:09 +0200 Subject: [PATCH 5/7] fix(daemon): refuse an unspendable open --wait budget before device work Reading the budget after the target device was resolved left a hole: an open whose device could not be resolved never reached the bounds check, so a Node client or a raw wire request carrying `waitMs: 5000000` passed the check the CLI parser would have applied and went looking for a device with a budget it could not spend. The budget is now the first thing the wait reads. --- .../__tests__/open-device-contention-wait.test.ts | 12 ++++++++++++ src/daemon/open-device-contention-wait.ts | 6 ++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/daemon/__tests__/open-device-contention-wait.test.ts b/src/daemon/__tests__/open-device-contention-wait.test.ts index a2b60f1dbc..680f28b84f 100644 --- a/src/daemon/__tests__/open-device-contention-wait.test.ts +++ b/src/daemon/__tests__/open-device-contention-wait.test.ts @@ -102,6 +102,18 @@ test('the daemon holds a budget to the bounds its own option declares', () => { code: 'INVALID_ARGS', }); } + + // A budget that cannot be spent is refused before the device it was meant for is even in play. + expect( + thrownBy(() => + beginOpenDeviceWait({ + req: openRequest({ waitMs: (max ?? 0) + 1 }), + sessionName: OPENER_ADDRESS, + sessionStore: new SessionStore('/tmp/ad-wait-unresolved'), + deviceId: undefined, + }), + ), + ).toMatchObject({ code: 'INVALID_ARGS' }); }); test('a refusal offers the flag only to a caller that did not arrive carrying it', () => { diff --git a/src/daemon/open-device-contention-wait.ts b/src/daemon/open-device-contention-wait.ts index abf0a7077e..728a5401f9 100644 --- a/src/daemon/open-device-contention-wait.ts +++ b/src/daemon/open-device-contention-wait.ts @@ -100,10 +100,12 @@ export function beginOpenDeviceWait(params: { deviceId: string | undefined; }): OpenDeviceWait | undefined { const { req, sessionName, sessionStore, deviceId } = params; - if (req.command !== 'open' || deviceId === undefined) return undefined; - if (sessionStore.get(sessionName)) return undefined; + if (req.command !== 'open') return undefined; + // The budget is read before anything else, so a request carrying a budget it may not spend is + // refused whether or not its device turned out to be resolvable. const budgetMs = readOpenWaitBudgetMs(req); if (budgetMs === undefined) return undefined; + if (deviceId === undefined || sessionStore.get(sessionName)) return undefined; return createOpenDeviceWait({ req, sessionName, sessionStore, deviceId, budgetMs }); } From 8cbf480b23fddae43e66a27ce749e76b77e0c7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Fri, 18 Sep 2026 18:03:48 +0200 Subject: [PATCH 6/7] test(daemon): prove the open contention guard decides the race The race test's open task bound the device unconditionally, so adding an `open --wait` budget to the current execution locks would allow all old checks to be demonstrated. The task refuses with the same fact the real open does. Out-of-range `waitMs` budgets fail before any device work. --- ...t-execution-scope-open-device-wait.test.ts | 79 ++++++++++++++----- src/daemon/request-execution-scope.ts | 8 +- .../internal/session-open-execution.ts | 2 +- 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts b/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts index db5273ce0c..c623e1f707 100644 --- a/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts +++ b/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts @@ -1,7 +1,9 @@ import { afterAll, test, expect, vi } from 'vitest'; import fs from 'node:fs'; +import { getFlagDefinitionsForKey } from '@agent-device/command-registry/flag-registry'; import type { CommandFlags } from '@agent-device/contracts/command'; import type { DeviceInfo } from '@agent-device/kernel/device'; +import { resolveTargetDevice } from '@agent-device/device-selection/dispatch-resolve'; import { makeSession } from '../../__tests__/test-utils/session-factories.ts'; import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; @@ -40,15 +42,19 @@ const WAIT_BUDGET_MS = 1_000; const TEST_ROOT = mkdtempForTestSync('agent-device-open-device-wait-'); -function openRequest(session: string): DaemonRequest { - const flags: CommandFlags = { waitMs: WAIT_BUDGET_MS }; +function openRequest( + session: string, + waitMs: number = WAIT_BUDGET_MS, + requestId = `req-${session}`, +): DaemonRequest { + const flags: CommandFlags = { waitMs }; return { token: 'token', session, command: 'open', positionals: [], flags, - meta: { cwd: TEST_ROOT, requestId: `req-${session}` }, + meta: { cwd: TEST_ROOT, requestId }, }; } @@ -73,6 +79,31 @@ function sleep(ms: number): Promise { // Two opens waiting on one device can both see it free on the same poll. The loser of that race // still has most of its budget, and the device is reachably its own as soon as the winner hands it // back, so refusing it where it stands is the bug this pins shut. +// +// The task below has to refuse just like the real open does when it finds another session on the +// device. Without that check it would open after simply waiting on the lock, and the test would +// pass even if runLocked stopped calling runWhenDeviceIsUnheld and did not re-check device +// contention. +type OpenOutcome = string; + +function openOnUncontendedDevice( + scope: Awaited>, + name: string, + sessionStore: ReturnType, + order: string[], +): Promise { + return scope.runLocked(async () => { + const currentOwner = sessionStore.findByDevice(CONTESTED_DEVICE.id); + if (currentOwner && currentOwner.address !== scope.sessionName) { + return `refused:${currentOwner.address}`; + } + + sessionStore.set(name, sessionOnDevice(name)); + order.push(name); + return `opened:${name}`; + }); +} + test('an open that lost the race to a free device re-waits and opens rather than refusing', async () => { const sessionStore = makeSessionStore('agent-device-open-wait-race-'); const leaseRegistry = new LeaseRegistry(); @@ -95,22 +126,14 @@ test('an open that lost the race to a free device re-waits and opens rather than const order: string[] = []; const startedAtMs = Date.now(); - // Each open binds the device to its own session under its own locks, which is what a real open - // does — the second one can only get in once the first releases it. - const firstOpened = first.runLocked(async () => { - sessionStore.set('first-opener', sessionOnDevice('first-opener')); - order.push('first'); - return 'first-opened'; - }); - const secondOpened = second.runLocked(async () => { - sessionStore.set('second-opener', sessionOnDevice('second-opener')); - order.push('second'); - return 'second-opened'; - }); + // Each open binds a device to its own session under its own device lock, which is what a real + // open does. The second can therefore only get in once the first releases it. + const firstOpened = openOnUncontendedDevice(first, 'first-opener', sessionStore, order); + const secondOpened = openOnUncontendedDevice(second, 'second-opener', sessionStore, order); - await expect(firstOpened).resolves.toBe('first-opened'); - await expect(secondOpened).resolves.toBe('second-opened'); - expect(order).toEqual(['first', 'second']); + await expect(firstOpened).resolves.toBe('opened:first-opener'); + await expect(secondOpened).resolves.toBe('opened:second-opener'); + expect(order).toEqual(['first-opener', 'second-opener']); // Both opened inside the budget, so neither owes a caller a story about one that ran out. expect(Date.now() - startedAtMs).toBeLessThan(WAIT_BUDGET_MS); @@ -159,6 +182,26 @@ test('a close that frees the device mid-wait gets through while the open is wait expect(order).toEqual(['close-freed-the-device', 'open-bound']); }); +// A Node client or raw wire request can carry a budget the CLI parser would have rejected. The +// daemon must reject it at its own request boundary before the device is resolved. This catches +// regression where example stored plans compute a device before budget validation. +test('an out-of-range wait budget is refused before resolving the target device', async () => { + const max = getFlagDefinitionsForKey('waitMs')[0]?.max; + if (typeof max !== 'number') { + throw new Error('waitMs declaration lost its maximum'); + } + + const resolveCallsBefore = vi.mocked(resolveTargetDevice).mock.calls.length; + await expect( + createRequestExecutionScope({ + req: openRequest('unbounded-opener', max + 1, 'request-unbounded'), + sessionStore: makeSessionStore('agent-device-open-wait-bounds-'), + leaseRegistry: new LeaseRegistry(), + }), + ).rejects.toMatchObject({ code: 'INVALID_ARGS' }); + expect(vi.mocked(resolveTargetDevice).mock.calls.length).toBe(resolveCallsBefore); +}); + afterAll(() => { vi.useRealTimers(); fs.rmSync(TEST_ROOT, { recursive: true, force: true }); diff --git a/src/daemon/request-execution-scope.ts b/src/daemon/request-execution-scope.ts index fd1df3676f..0325ea4736 100644 --- a/src/daemon/request-execution-scope.ts +++ b/src/daemon/request-execution-scope.ts @@ -24,7 +24,7 @@ import { resolveRequestExecutionLockPlan, type RequestExecutionLockPlan, } from './request-binding.ts'; -import { beginOpenDeviceWait } from './open-device-contention-wait.ts'; +import { beginOpenDeviceWait, readOpenWaitBudgetMs } from './open-device-contention-wait.ts'; import { createRequestExecutionLocks } from './request-execution-locks.ts'; import { throwIfRequestCanceled } from '@agent-device/host-kit/request'; import { finalizeDaemonResponse } from './request-finalization.ts'; @@ -176,6 +176,12 @@ export async function createRequestExecutionScope(params: { } try { assertLockedLeaseAdmissionPreflight(scopedReq); + // An out-of-range `--wait` is refused before resolving the target device or taking any lock. + // `beginOpenDeviceWait` still needs the device id from the lock plan, but validating the budget + // is independent of that lookup. + if (scopedReq.command === 'open') { + readOpenWaitBudgetMs(scopedReq); + } const lockPlan: RequestExecutionLockPlan = shouldLockSessionExecution(command) ? await resolveRequestExecutionLockPlan({ req: scopedReq, sessionName, sessionStore }) : { keys: [], deviceId: undefined }; diff --git a/src/daemon/session-lifecycle/internal/session-open-execution.ts b/src/daemon/session-lifecycle/internal/session-open-execution.ts index 9e23944b6a..83f9d25bf0 100644 --- a/src/daemon/session-lifecycle/internal/session-open-execution.ts +++ b/src/daemon/session-lifecycle/internal/session-open-execution.ts @@ -67,7 +67,7 @@ import { } from '../../device-claim-conflict.ts'; import { requireAllocatorHeldDeviceClaim } from '../../device-claim-allocator.ts'; import { deviceClaimRuleForOwner } from '../../device-claim-rule.ts'; -import { errorResponse } from '@agent-device/kernel/contracts'; +import { errorResponse, type DaemonFailureResponse } from '@agent-device/kernel/contracts'; type OpenTiming = { totalDurationMs?: number; From 46334729f915ae2446857747947c9e5d763d23e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 19 Sep 2026 09:10:00 +0200 Subject: [PATCH 7/7] refactor(daemon): tighten the open-wait request seams The request scope now parses the `waitMs` option once and supplies the resulting budget to the wait, instead of validating it before the device lookup and reading it again afterward. The reader fails closed when a caller supplies a budget but its option declaration has gone missing. The refusal helper checks the command directly, so reuse outside the open path cannot offer `--wait` to an interaction. --- .../open-device-contention-wait.test.ts | 45 ++++++++++--------- src/daemon/open-device-contention-wait.ts | 30 ++++++++----- src/daemon/request-execution-scope.ts | 11 +++-- 3 files changed, 48 insertions(+), 38 deletions(-) diff --git a/src/daemon/__tests__/open-device-contention-wait.test.ts b/src/daemon/__tests__/open-device-contention-wait.test.ts index 680f28b84f..5b3749c60d 100644 --- a/src/daemon/__tests__/open-device-contention-wait.test.ts +++ b/src/daemon/__tests__/open-device-contention-wait.test.ts @@ -35,6 +35,15 @@ function openRequest(flags: CommandFlags): DaemonRequest { }; } +function beginWait( + params: Omit[0], 'budgetMs'>, +): ReturnType { + return beginOpenDeviceWait({ + ...params, + budgetMs: readOpenWaitBudgetMs(params.req), + }); +} + function session(address: string): SessionState { return { name: 'default', @@ -102,23 +111,17 @@ test('the daemon holds a budget to the bounds its own option declares', () => { code: 'INVALID_ARGS', }); } - - // A budget that cannot be spent is refused before the device it was meant for is even in play. - expect( - thrownBy(() => - beginOpenDeviceWait({ - req: openRequest({ waitMs: (max ?? 0) + 1 }), - sessionName: OPENER_ADDRESS, - sessionStore: new SessionStore('/tmp/ad-wait-unresolved'), - deviceId: undefined, - }), - ), - ).toMatchObject({ code: 'INVALID_ARGS' }); }); test('a refusal offers the flag only to a caller that did not arrive carrying it', () => { expect(describeOpenWaitForRefusal(openRequest({}))).toEqual({ offersDeviceWait: true }); + // An interaction has no `--wait` of its own, even though the shared refusal builder may one day + // receive its request. + expect(describeOpenWaitForRefusal({ ...openRequest({}), command: 'tap' })).toEqual({ + offersDeviceWait: false, + }); + // The open that passed `--wait` and still hit a busy device is not being sent off to run the // same open with the flag it used. const carried = openRequest({ waitMs: 1_000 }); @@ -133,7 +136,7 @@ test('only a fresh open with a budget and a resolved device gets a wait', () => const budget = openRequest({ waitMs: 5_000 }); expect( - beginOpenDeviceWait({ + beginWait({ req: { ...budget, command: 'tap' }, sessionName: OPENER_ADDRESS, sessionStore, @@ -141,7 +144,7 @@ test('only a fresh open with a budget and a resolved device gets a wait', () => }), ).toBeUndefined(); expect( - beginOpenDeviceWait({ + beginWait({ req: budget, sessionName: OPENER_ADDRESS, sessionStore, @@ -149,7 +152,7 @@ test('only a fresh open with a budget and a resolved device gets a wait', () => }), ).toBeUndefined(); expect( - beginOpenDeviceWait({ + beginWait({ req: openRequest({}), sessionName: OPENER_ADDRESS, sessionStore, @@ -160,7 +163,7 @@ test('only a fresh open with a budget and a resolved device gets a wait', () => // An open onto a session that already exists is bound to a device nobody else is refused for. sessionStore.set(OPENER_ADDRESS, session(OPENER_ADDRESS)); expect( - beginOpenDeviceWait({ + beginWait({ req: budget, sessionName: OPENER_ADDRESS, sessionStore, @@ -174,7 +177,7 @@ test('a free device is read once in the store and records no wait', async () => const store = makeSessionStore('agent-device-open-wait-'); const looks = vi.spyOn(store, 'findByDevice'); - const wait = beginOpenDeviceWait({ + const wait = beginWait({ req, sessionName: OPENER_ADDRESS, sessionStore: store, @@ -194,7 +197,7 @@ test('a device that frees up ends the wait without claiming a spent budget', asy await waitUntil( () => - beginOpenDeviceWait({ + beginWait({ req, sessionName: OPENER_ADDRESS, sessionStore: store, @@ -212,7 +215,7 @@ test('an open yields the device lock to a session that took the device after the vi.useFakeTimers(); const req = openRequest({ waitMs: 30_000 }); const store = makeSessionStore('agent-device-open-wait-'); - const wait = beginOpenDeviceWait({ + const wait = beginWait({ req, sessionName: OPENER_ADDRESS, sessionStore: store, @@ -253,7 +256,7 @@ test('a budget that runs out busy costs the whole budget, then lets the open ref const req = openRequest({ waitMs: 1000 }); const store = storeWithHolder(); const looks = vi.spyOn(store, 'findByDevice'); - const wait = beginOpenDeviceWait({ + const wait = beginWait({ req, sessionName: OPENER_ADDRESS, sessionStore: store, @@ -289,7 +292,7 @@ test('a request the client gave up on stops waiting at its next poll', async () vi.useFakeTimers(); const req = openRequest({ waitMs: 60_000 }); markRequestCanceled('req-open-wait'); - const wait = beginOpenDeviceWait({ + const wait = beginWait({ req, sessionName: OPENER_ADDRESS, sessionStore: storeWithHolder(), diff --git a/src/daemon/open-device-contention-wait.ts b/src/daemon/open-device-contention-wait.ts index 728a5401f9..58dfc8679f 100644 --- a/src/daemon/open-device-contention-wait.ts +++ b/src/daemon/open-device-contention-wait.ts @@ -2,6 +2,7 @@ import { getFlagDefinitionsForKey } from '@agent-device/command-registry/flag-re import { readOptionalInteger } from '@agent-device/contracts/command'; import { emitRequestProgress, throwIfRequestCanceled } from '@agent-device/host-kit/request'; import { Deadline, sleep } from '@agent-device/host-kit/retry'; +import { AppError } from '@agent-device/kernel/errors'; import type { DaemonRequest } from './daemon-request.ts'; import type { SessionStore } from './session-store.ts'; import type { SessionRecoveryOptions } from './session-recovery-hints.ts'; @@ -22,10 +23,18 @@ const DEVICE_CONTENTION_POLL_MS = 250; * declaration instead of trusting whichever surface assembled the request. */ export function readOpenWaitBudgetMs(req: DaemonRequest): number | undefined { - const declaration = getFlagDefinitionsForKey('waitMs')[0]; - return readOptionalInteger((req.flags ?? {}) as Record, 'waitMs', { - min: declaration?.min, - max: declaration?.max, + const flags = (req.flags ?? {}) as Record; + if (flags.waitMs === undefined) return undefined; + const [declaration] = getFlagDefinitionsForKey('waitMs'); + if (!declaration) { + throw new AppError( + 'INTERNAL_ERROR', + 'The waitMs option has no flag declaration whose bounds the daemon can enforce.', + ); + } + return readOptionalInteger(flags, 'waitMs', { + min: declaration.min, + max: declaration.max, }); } @@ -51,7 +60,7 @@ export function readOpenWaitAttempt(req: DaemonRequest): SessionRecoveryOptions export function describeOpenWaitForRefusal(req: DaemonRequest): SessionRecoveryOptions { return { ...readOpenWaitAttempt(req), - offersDeviceWait: typeof req.flags?.waitMs !== 'number', + offersDeviceWait: req.command === 'open' && typeof req.flags?.waitMs !== 'number', }; } @@ -92,19 +101,18 @@ export type OpenDeviceWait = { * * `deviceId` is the device the request's own execution-lock plan reserves, so the wait and the * lock agree on which device is being waited for without resolving a target a second time. + * `budgetMs` must already have been read through {@link readOpenWaitBudgetMs}, allowing the request + * scope to reject an out-of-range budget before resolving that device. */ export function beginOpenDeviceWait(params: { req: DaemonRequest; + budgetMs: number | undefined; sessionName: string; sessionStore: SessionStore; deviceId: string | undefined; }): OpenDeviceWait | undefined { - const { req, sessionName, sessionStore, deviceId } = params; - if (req.command !== 'open') return undefined; - // The budget is read before anything else, so a request carrying a budget it may not spend is - // refused whether or not its device turned out to be resolvable. - const budgetMs = readOpenWaitBudgetMs(req); - if (budgetMs === undefined) return undefined; + const { req, budgetMs, sessionName, sessionStore, deviceId } = params; + if (req.command !== 'open' || budgetMs === undefined) return undefined; if (deviceId === undefined || sessionStore.get(sessionName)) return undefined; return createOpenDeviceWait({ req, sessionName, sessionStore, deviceId, budgetMs }); } diff --git a/src/daemon/request-execution-scope.ts b/src/daemon/request-execution-scope.ts index 0325ea4736..c4a65011a0 100644 --- a/src/daemon/request-execution-scope.ts +++ b/src/daemon/request-execution-scope.ts @@ -176,12 +176,10 @@ export async function createRequestExecutionScope(params: { } try { assertLockedLeaseAdmissionPreflight(scopedReq); - // An out-of-range `--wait` is refused before resolving the target device or taking any lock. - // `beginOpenDeviceWait` still needs the device id from the lock plan, but validating the budget - // is independent of that lookup. - if (scopedReq.command === 'open') { - readOpenWaitBudgetMs(scopedReq); - } + // Parse the budget once, before resolving the target device or taking any lock. The lock plan + // still supplies the device to wait for, but an out-of-range budget is refused before either. + const openWaitBudgetMs = + scopedReq.command === 'open' ? readOpenWaitBudgetMs(scopedReq) : undefined; const lockPlan: RequestExecutionLockPlan = shouldLockSessionExecution(command) ? await resolveRequestExecutionLockPlan({ req: scopedReq, sessionName, sessionStore }) : { keys: [], deviceId: undefined }; @@ -190,6 +188,7 @@ export async function createRequestExecutionScope(params: { // needs, so waiting after taking it would have an open block its own recovery. const openWait = beginOpenDeviceWait({ req: scopedReq, + budgetMs: openWaitBudgetMs, sessionName, sessionStore, deviceId: lockPlan.deviceId,