diff --git a/packages/command-registry/src/flag-definitions-workflow.ts b/packages/command-registry/src/flag-definitions-workflow.ts index 5399fe3e0f..1ab0eb7a47 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 (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, + }, { 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..c909ed3065 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 (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 // 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/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', 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/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}`; } 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..5b3749c60d --- /dev/null +++ b/src/daemon/__tests__/open-device-contention-wait.test.ts @@ -0,0 +1,315 @@ +import { test, expect, vi, afterEach } from 'vitest'; +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, +} from '../open-device-contention-wait.ts'; + +// `--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 { + token: 'token', + session: 'default', + command: 'open', + positionals: [], + flags, + meta: { requestId: 'req-open-wait' }, + }; +} + +function beginWait( + params: Omit[0], 'budgetMs'>, +): ReturnType { + return beginOpenDeviceWait({ + ...params, + budgetMs: readOpenWaitBudgetMs(params.req), + }); +} + +function session(address: string): SessionState { + return { + name: 'default', + sessionScope: { kind: 'cwd', id: address.split(':')[1] ?? '' }, + device: IOS_SIMULATOR, + createdAt: 0, + actions: [], + }; +} + +function storeWithHolder(): SessionStore { + const store = makeSessionStore('agent-device-open-wait-'); + store.set(HOLDER_ADDRESS, session(HOLDER_ADDRESS)); + return store; +} + +/** 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 { + 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'); +}); + +// 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(); + + for (const waitMs of [(min ?? 0) - 1, (max ?? 0) + 1, 0, 1.5]) { + expect(thrownBy(() => readOpenWaitBudgetMs(openRequest({ waitMs })))).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 }); + expect(describeOpenWaitForRefusal(carried)).toEqual({ offersDeviceWait: false }); + + const spent: DaemonRequest = { ...carried, internal: { openDeviceWait: { waitedMs: 1_000 } } }; + expect(describeOpenWaitForRefusal(spent)).toEqual({ waitedMs: 1_000, offersDeviceWait: false }); +}); + +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( + beginWait({ + req: { ...budget, command: 'tap' }, + sessionName: OPENER_ADDRESS, + sessionStore, + deviceId: IOS_SIMULATOR.id, + }), + ).toBeUndefined(); + expect( + beginWait({ + req: budget, + sessionName: OPENER_ADDRESS, + sessionStore, + deviceId: undefined, + }), + ).toBeUndefined(); + expect( + beginWait({ + 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( + beginWait({ + 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 = makeSessionStore('agent-device-open-wait-'); + const looks = vi.spyOn(store, 'findByDevice'); + + const wait = beginWait({ + req, + sessionName: OPENER_ADDRESS, + sessionStore: store, + deviceId: IOS_SIMULATOR.id, + })!; + await wait.waitForDeviceOutsideLocks(); + + expect(looks).toHaveBeenCalledTimes(1); + expect(readOpenWaitAttempt(req)).toEqual({}); +}); + +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(); + const release = setTimeout(() => store.delete(HOLDER_ADDRESS), 600); + + await waitUntil( + () => + beginWait({ + 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({}); +}); + +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 = beginWait({ + req, + sessionName: OPENER_ADDRESS, + sessionStore: store, + 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)); + }, + }); + + 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; + + // 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 costs the whole budget, then lets the open refuse', async () => { + vi.useFakeTimers(); + const req = openRequest({ waitMs: 1000 }); + const store = storeWithHolder(); + const looks = vi.spyOn(store, 'findByDevice'); + const wait = beginWait({ + req, + 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); + // 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 wait = beginWait({ + req, + sessionName: OPENER_ADDRESS, + sessionStore: storeWithHolder(), + 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..c623e1f707 --- /dev/null +++ b/src/daemon/__tests__/request-execution-scope-open-device-wait.test.ts @@ -0,0 +1,208 @@ +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'; +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, + 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 }, + }; +} + +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. +// +// 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(); + 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 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('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); + 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']); +}); + +// 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/__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/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..58dfc8679f --- /dev/null +++ b/src/daemon/open-device-contention-wait.ts @@ -0,0 +1,196 @@ +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 { 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'; + +/** + * 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. + * + * 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 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, + }); +} + +/** + * 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. 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; + return waitedMs === undefined ? {} : { waitedMs }; +} + +/** + * 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: req.command === 'open' && 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. + * + * `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, 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 }); +} + +/** 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, +): ReturnType { + const inUse = sessionStore.findByDevice(deviceId); + if (!inUse || inUse.address === sessionName) return undefined; + 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 { + 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..d8c0ef622b 100644 --- a/src/daemon/request-binding.ts +++ b/src/daemon/request-binding.ts @@ -4,44 +4,49 @@ 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'; 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: { @@ -78,6 +83,22 @@ function resolveFreshSessionDeviceLock(req: DaemonRequest): return resolveExplicitDeviceLock(req); } +/** + * 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. + */ +async function resolveFreshSessionDevice(req: DaemonRequest): Promise { + const resolution = resolveFreshSessionDeviceLock(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..c4a65011a0 100644 --- a/src/daemon/request-execution-scope.ts +++ b/src/daemon/request-execution-scope.ts @@ -19,7 +19,12 @@ import { assertLockedLeaseAdmissionPreflight, cleanupExpiredLeasedSession, } from './lease-lifecycle.ts'; -import { prepareLockedRequestBinding, resolveRequestExecutionLockKeys } from './request-binding.ts'; +import { + prepareLockedRequestBinding, + resolveRequestExecutionLockPlan, + type RequestExecutionLockPlan, +} from './request-binding.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'; @@ -171,13 +176,28 @@ export async function createRequestExecutionScope(params: { } try { assertLockedLeaseAdmissionPreflight(scopedReq); - const executionLockKeys = shouldLockSessionExecution(command) - ? await resolveRequestExecutionLockKeys({ req: scopedReq, sessionName, sessionStore }) - : []; + // 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 }; + // 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, + budgetMs: openWaitBudgetMs, + 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, @@ -252,7 +272,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/__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..83f9d25bf0 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 { describeOpenWaitForRefusal } from '../../open-device-contention-wait.ts'; import { isImplicitSessionScopeConflict, resolveSessionScope, @@ -63,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; @@ -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; + // 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 = describeOpenWaitForRefusal(req); 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/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 18a4aefd42..be2d5d9d78 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. 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 f4beba56fd..6b5eafe804 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 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: - `open ` within an existing session switches the active app and updates the session bundle id.