From 4c9ccd71a566835832f6394093edd3d17581d9df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 4 Jul 2026 14:22:17 +0200 Subject: [PATCH] refactor: declare command timeout policy on descriptors (ADR 0011) The wait timeout bug (#1075) happened because request-envelope budgets and on-timeout daemon policy lived in two hand-maintained lists in the daemon client: isExplicitTimeoutCommand (daemon-client.ts) and DAEMON_PRESERVING_TIMEOUT_COMMANDS / shouldResetDaemonAfterRequestTimeout (daemon-client-timeout.ts). A command could fall through both without anyone noticing. Both lists are deleted. Each command descriptor (ADR 0008 registry) now declares a required timeoutPolicy: timeoutPolicy: { budget: { source: 'none' | 'flag' | 'positional-parser'; parser? }; envelopeMs: number | 'unbounded'; onTimeout: 'preserve-daemon' | 'reset-daemon'; } The client derives the request envelope and the on-timeout daemon policy from the declaration; the +30s margin / never-shrink rule for positional budgets is preserved generically. Envelope constants move from src/daemon/request-timeouts.ts to src/core/command-descriptor/timeout-policy.ts next to the policies they parameterize. A completeness gate (timeout-policy.test.ts) asserts every public command declares a policy and pins the deviating sets (preserve-daemon = snapshot/ wait/find; flag budget = prepare/replay/snapshot; positional = wait; envelopes = prepare 240s, install-like 180s, test unbounded) as bounded diffable lists. The pre-existing oracle tests in src/utils/__tests__/daemon-client.test.ts pass byte-for-byte unchanged, proving the migration is behaviorally exact. --- .../__tests__/timeout-policy.test.ts | 101 ++++++++++++++ src/core/command-descriptor/registry.ts | 126 +++++++++++++++++- src/core/command-descriptor/timeout-policy.ts | 27 ++++ src/core/command-descriptor/types.ts | 53 +++++++- src/core/wait-positionals.ts | 13 ++ src/daemon/client/daemon-client-timeout.ts | 21 +-- src/daemon/client/daemon-client.ts | 54 +++----- src/daemon/handlers/session.ts | 2 +- src/daemon/request-timeouts.ts | 6 - 9 files changed, 342 insertions(+), 61 deletions(-) create mode 100644 src/core/command-descriptor/__tests__/timeout-policy.test.ts create mode 100644 src/core/command-descriptor/timeout-policy.ts delete mode 100644 src/daemon/request-timeouts.ts diff --git a/src/core/command-descriptor/__tests__/timeout-policy.test.ts b/src/core/command-descriptor/__tests__/timeout-policy.test.ts new file mode 100644 index 0000000000..80b051230c --- /dev/null +++ b/src/core/command-descriptor/__tests__/timeout-policy.test.ts @@ -0,0 +1,101 @@ +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { PUBLIC_COMMANDS } from '../../../command-catalog.ts'; +import { commandDescriptors, resolveCommandTimeoutPolicy } from '../registry.ts'; +import { DEFAULT_TIMEOUT_POLICY } from '../timeout-policy.ts'; + +// ADR 0011 completeness gate for the descriptor timeout policy (the layer that +// replaced the two hand-maintained client lists `isExplicitTimeoutCommand` and +// `DAEMON_PRESERVING_TIMEOUT_COMMANDS`): every public command must carry a +// declared policy, and the sets of commands that deviate from the shared +// default are bounded, diffable lists — they may only change in the same PR +// that updates them here. Behavioral derivation (envelope arithmetic, wait +// budget parsing, flag overrides) is proven by the pre-existing oracle tests in +// src/utils/__tests__/daemon-client.test.ts, which survived this migration +// unchanged. + +test('every public command declares a timeout policy on its descriptor', () => { + const byName = new Map(commandDescriptors.map((descriptor) => [descriptor.name, descriptor])); + for (const command of Object.values(PUBLIC_COMMANDS)) { + const descriptor = byName.get(command); + assert.ok(descriptor, `public command ${command} is missing from the descriptor registry`); + assert.ok(descriptor.timeoutPolicy, `public command ${command} declares no timeoutPolicy`); + } +}); + +test('declared timeout policies are structurally valid', () => { + for (const descriptor of commandDescriptors) { + const policy = descriptor.timeoutPolicy; + assert.ok( + policy.onTimeout === 'preserve-daemon' || policy.onTimeout === 'reset-daemon', + `${descriptor.name}: invalid onTimeout ${String(policy.onTimeout)}`, + ); + if (policy.envelopeMs !== 'unbounded') { + assert.ok( + Number.isFinite(policy.envelopeMs) && policy.envelopeMs > 0, + `${descriptor.name}: envelopeMs must be a positive duration`, + ); + } + if (policy.budget.source === 'positional-parser') { + assert.equal( + typeof policy.budget.parser, + 'function', + `${descriptor.name}: positional-parser budget requires a parser`, + ); + } + } +}); + +test('daemon-preserving timeout commands are a bounded, reviewed set', () => { + // CONSERVATIVE: this list may only change in the same PR that updates it + // here. Preserving the daemon on timeout is for read-only capture/polling + // commands that can block in platform accessibility bridges — a timed-out + // poll must not turn into a daemon reset that loses every session (#1075). + const preserving = commandDescriptors + .filter((descriptor) => descriptor.timeoutPolicy.onTimeout === 'preserve-daemon') + .map((descriptor) => descriptor.name); + assert.deepEqual(preserving.sort(), ['find', 'snapshot', 'wait']); +}); + +test('budget sources deviating from the default are bounded, reviewed sets', () => { + const flagBudget: string[] = []; + const positionalBudget: string[] = []; + for (const descriptor of commandDescriptors) { + if (descriptor.timeoutPolicy.budget.source === 'flag') flagBudget.push(descriptor.name); + if (descriptor.timeoutPolicy.budget.source === 'positional-parser') { + positionalBudget.push(descriptor.name); + } + } + // --timeout bounds the request envelope for these commands only. + assert.deepEqual(flagBudget.sort(), ['prepare', 'replay', 'snapshot']); + // wait's budget travels as a positional and must widen the envelope. + assert.deepEqual(positionalBudget, ['wait']); +}); + +test('request envelopes deviating from the default are bounded, reviewed sets', () => { + const EXPECTED_ENVELOPES: Record = { + prepare: 240_000, + install: 180_000, + reinstall: 180_000, + install_source: 180_000, + test: 'unbounded', + }; + for (const descriptor of commandDescriptors) { + const expected = EXPECTED_ENVELOPES[descriptor.name] ?? 90_000; + assert.equal( + descriptor.timeoutPolicy.envelopeMs, + expected, + `${descriptor.name}: unexpected request envelope`, + ); + } +}); + +test('commands outside the registry fall back to the explicit default policy', () => { + // Matches the deleted hand lists: not listed meant default envelope and a + // daemon reset on timeout. + assert.equal(resolveCommandTimeoutPolicy(undefined), DEFAULT_TIMEOUT_POLICY); + assert.equal(resolveCommandTimeoutPolicy('not-a-registered-command'), DEFAULT_TIMEOUT_POLICY); + assert.equal(DEFAULT_TIMEOUT_POLICY.onTimeout, 'reset-daemon'); + assert.equal(DEFAULT_TIMEOUT_POLICY.envelopeMs, 90_000); + assert.equal(DEFAULT_TIMEOUT_POLICY.budget.source, 'none'); +}); diff --git a/src/core/command-descriptor/registry.ts b/src/core/command-descriptor/registry.ts index 99bdaee499..162cccd7d4 100644 --- a/src/core/command-descriptor/registry.ts +++ b/src/core/command-descriptor/registry.ts @@ -5,7 +5,13 @@ import { } from '../../command-catalog.ts'; import type { CommandCapability } from '../capabilities.ts'; import type { DaemonRequest } from '../../daemon/types.ts'; -import type { CommandDescriptor } from './types.ts'; +import { resolveWaitBudgetMs } from '../wait-positionals.ts'; +import { + DEFAULT_TIMEOUT_POLICY, + INSTALL_REQUEST_TIMEOUT_MS, + PREPARE_REQUEST_TIMEOUT_MS, +} from './timeout-policy.ts'; +import type { CommandDescriptor, CommandTimeoutPolicy } from './types.ts'; // --------------------------------------------------------------------------- // Daemon request-policy trait bundles — copied VERBATIM from @@ -67,6 +73,32 @@ const APP_INSTALL_CAPABILITY = { linux: LINUX_NONE, } satisfies CommandCapability; +// --------------------------------------------------------------------------- +// Timeout policies (ADR-0011) — the request-envelope budget source and the +// on-timeout daemon policy, derived VERBATIM from the two deleted client hand +// lists (`isExplicitTimeoutCommand` in daemon-client.ts and +// `DAEMON_PRESERVING_TIMEOUT_COMMANDS` in daemon-client-timeout.ts) plus the +// per-command envelope branches of `resolveDaemonRequestTimeoutMs`. +// --------------------------------------------------------------------------- + +// Read-only capture commands that can block in platform accessibility bridges +// while the app is crashed or never idle share snapshot's failure mode. Keep the +// daemon/session alive on their timeouts so callers can still collect +// screenshot/perf/log evidence and close the session after the runner abort +// path has been triggered — resetting the daemon here turned one timed-out wait +// into a lost session for every session the daemon owned. +const PRESERVE_DAEMON_TIMEOUT_POLICY: CommandTimeoutPolicy = { + ...DEFAULT_TIMEOUT_POLICY, + onTimeout: 'preserve-daemon', +}; + +// Installs run long device subprocesses; their envelope stays above the longest +// platform install subprocess timeout (see INSTALL_REQUEST_TIMEOUT_MS). +const INSTALL_TIMEOUT_POLICY: CommandTimeoutPolicy = { + ...DEFAULT_TIMEOUT_POLICY, + envelopeMs: INSTALL_REQUEST_TIMEOUT_MS, +}; + // --------------------------------------------------------------------------- // The additive single source. Each entry carries the daemon route/traits + // capability + batchable flag copied VERBATIM from today's hand tables. @@ -81,21 +113,25 @@ const RAW_COMMAND_DESCRIPTORS = [ { name: INTERNAL_COMMANDS.leaseAllocate, daemon: { route: 'lease', ...ADMISSION_AND_LOCK_EXEMPT }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: INTERNAL_COMMANDS.leaseHeartbeat, daemon: { route: 'lease', ...ADMISSION_AND_LOCK_EXEMPT }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: INTERNAL_COMMANDS.leaseRelease, daemon: { route: 'lease', ...ADMISSION_AND_LOCK_EXEMPT }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: PUBLIC_COMMANDS.artifacts, daemon: { route: 'lease', ...ADMISSION_AND_LOCK_EXEMPT }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, @@ -103,6 +139,7 @@ const RAW_COMMAND_DESCRIPTORS = [ { name: INTERNAL_COMMANDS.sessionList, daemon: { route: 'session', sessionKind: 'inventory', ...REQUEST_EXECUTION_EXEMPT }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { @@ -113,6 +150,7 @@ const RAW_COMMAND_DESCRIPTORS = [ lockPolicySelectorOverride: true, ...REQUEST_EXECUTION_EXEMPT, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -124,6 +162,7 @@ const RAW_COMMAND_DESCRIPTORS = [ allowSessionlessDefaultDevice: allowAnyDeviceSessionless, ...REQUEST_EXECUTION_EXEMPT, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -135,6 +174,7 @@ const RAW_COMMAND_DESCRIPTORS = [ preferExplicitDeviceOverExistingSession: true, }, capability: APP_INVENTORY_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -145,6 +185,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -155,29 +196,34 @@ const RAW_COMMAND_DESCRIPTORS = [ android: { emulator: true }, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.appState, daemon: { route: 'session', sessionKind: 'state' }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.perf, daemon: { route: 'session', sessionKind: 'observability' }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.logs, daemon: { route: 'session', sessionKind: 'observability' }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.network, daemon: { route: 'session', sessionKind: 'observability' }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -188,6 +234,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: { emulator: true }, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -197,6 +244,8 @@ const RAW_COMMAND_DESCRIPTORS = [ sessionKind: 'replay', skipSessionlessProviderDevice: isShardedTestRequest, }, + // Replay durations are script-dependent; --timeout bounds the envelope. + timeoutPolicy: { ...DEFAULT_TIMEOUT_POLICY, budget: { source: 'flag' } }, batchable: false, }, { @@ -206,11 +255,15 @@ const RAW_COMMAND_DESCRIPTORS = [ sessionKind: 'replay', skipSessionlessProviderDevice: isShardedTestRequest, }, + // Test runs stream per-scenario progress and are budgeted downstream; no + // client envelope at all. + timeoutPolicy: { ...DEFAULT_TIMEOUT_POLICY, envelopeMs: 'unbounded' }, batchable: true, }, { name: INTERNAL_COMMANDS.runtime, daemon: { route: 'session' }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { @@ -221,6 +274,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_DEVICE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -231,28 +285,33 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.install, daemon: { route: 'session' }, capability: APP_INSTALL_CAPABILITY, + timeoutPolicy: INSTALL_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.reinstall, daemon: { route: 'session' }, capability: APP_INSTALL_CAPABILITY, + timeoutPolicy: INSTALL_TIMEOUT_POLICY, batchable: true, }, { name: INTERNAL_COMMANDS.installSource, daemon: { route: 'session' }, + timeoutPolicy: INSTALL_TIMEOUT_POLICY, batchable: false, }, { name: INTERNAL_COMMANDS.releaseMaterializedPaths, daemon: { route: 'session', ...REQUEST_EXECUTION_EXEMPT }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { @@ -263,34 +322,45 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.triggerAppEvent, daemon: { route: 'session' }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.open, daemon: { route: 'session', allowSessionlessDefaultDevice: allowAnyDeviceSessionless }, capability: APP_RUNTIME_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.prepare, daemon: { route: 'session' }, + // Runner warm-up builds are the longest fixed envelope; --timeout overrides. + timeoutPolicy: { + budget: { source: 'flag' }, + envelopeMs: PREPARE_REQUEST_TIMEOUT_MS, + onTimeout: 'reset-daemon', + }, batchable: false, }, { name: PUBLIC_COMMANDS.batch, daemon: { route: 'session' }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: PUBLIC_COMMANDS.close, daemon: { route: 'session', allowInvalidRecording: true }, capability: APP_RUNTIME_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, @@ -299,18 +369,28 @@ const RAW_COMMAND_DESCRIPTORS = [ name: PUBLIC_COMMANDS.snapshot, daemon: { route: 'snapshot', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + // First Apple snapshot on a device can sit behind runner startup; --timeout + // widens the envelope, and a timeout must not tear down the daemon. + timeoutPolicy: { ...PRESERVE_DAEMON_TIMEOUT_POLICY, budget: { source: 'flag' } }, batchable: true, }, { name: PUBLIC_COMMANDS.diff, daemon: { route: 'snapshot', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.wait, daemon: { route: 'snapshot', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + // The wait budget travels as a positional, not a flag; parse it the same + // way the daemon will so the request envelope extends past it (#1075). + timeoutPolicy: { + ...PRESERVE_DAEMON_TIMEOUT_POLICY, + budget: { source: 'positional-parser', parser: resolveWaitBudgetMs }, + }, batchable: true, }, { @@ -321,6 +401,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -331,6 +412,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, @@ -339,6 +421,7 @@ const RAW_COMMAND_DESCRIPTORS = [ name: PUBLIC_COMMANDS.reactNative, daemon: { route: 'reactNative', replayScopedAction: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -350,17 +433,20 @@ const RAW_COMMAND_DESCRIPTORS = [ allowSessionlessDefaultDevice: isRecordingStartRequest, }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.trace, daemon: { route: 'recordTrace' }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.find, daemon: { route: 'find', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + timeoutPolicy: PRESERVE_DAEMON_TIMEOUT_POLICY, batchable: true, }, @@ -369,42 +455,49 @@ const RAW_COMMAND_DESCRIPTORS = [ name: PUBLIC_COMMANDS.click, daemon: { route: 'interaction', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.fill, daemon: { route: 'interaction', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.longPress, daemon: { route: 'interaction', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.press, daemon: { route: 'interaction', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.type, daemon: { route: 'interaction', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.get, daemon: { route: 'interaction', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.is, daemon: { route: 'interaction', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, @@ -413,11 +506,13 @@ const RAW_COMMAND_DESCRIPTORS = [ name: PUBLIC_COMMANDS.back, daemon: { route: 'generic', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.gesture, daemon: { route: 'generic', replayScopedAction: true, androidBlockingDialogGuard: true }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -428,6 +523,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_DEVICE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -438,18 +534,21 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.scroll, daemon: { route: 'generic', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.swipe, daemon: { route: 'generic', replayScopedAction: true, androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { @@ -460,36 +559,42 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: PUBLIC_COMMANDS.focus, daemon: { route: 'generic', androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.screenshot, daemon: { route: 'generic', replayScopedAction: true }, capability: ALL_DEVICE_COMMAND_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.viewport, daemon: { route: 'generic', replayScopedAction: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: 'pan', daemon: { route: 'generic', androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_DEVICE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { name: 'fling', daemon: { route: 'generic', androidBlockingDialogGuard: true }, capability: { apple: APPLE_SIM_AND_DEVICE, android: ANDROID_ALL, linux: LINUX_NONE }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { @@ -500,6 +605,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, { @@ -510,6 +616,7 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: false, }, @@ -521,11 +628,13 @@ const RAW_COMMAND_DESCRIPTORS = [ android: ANDROID_ALL, linux: LINUX_NONE, }, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, { name: PUBLIC_COMMANDS.installFromSource, capability: APP_INSTALL_CAPABILITY, + timeoutPolicy: DEFAULT_TIMEOUT_POLICY, batchable: true, }, ] as const satisfies readonly Omit[]; @@ -547,3 +656,18 @@ export const commandDescriptors = RAW_COMMAND_DESCRIPTORS.map((descriptor) => ({ /** The literal union of every registered command name. */ export type Command = (typeof commandDescriptors)[number]['name']; + +const TIMEOUT_POLICY_BY_COMMAND: ReadonlyMap = new Map( + commandDescriptors.map((descriptor) => [descriptor.name, descriptor.timeoutPolicy]), +); + +/** + * The declared timeout policy for a command (ADR-0011). Command names outside + * the registry (internal probes, unknown commands) fall back to + * {@link DEFAULT_TIMEOUT_POLICY} — standard envelope, reset-daemon — exactly as + * the deleted hand lists treated unlisted commands. + */ +export function resolveCommandTimeoutPolicy(command: string | undefined): CommandTimeoutPolicy { + if (command === undefined) return DEFAULT_TIMEOUT_POLICY; + return TIMEOUT_POLICY_BY_COMMAND.get(command) ?? DEFAULT_TIMEOUT_POLICY; +} diff --git a/src/core/command-descriptor/timeout-policy.ts b/src/core/command-descriptor/timeout-policy.ts new file mode 100644 index 0000000000..3669308a69 --- /dev/null +++ b/src/core/command-descriptor/timeout-policy.ts @@ -0,0 +1,27 @@ +import type { CommandTimeoutPolicy } from './types.ts'; + +// Request-envelope constants, relocated from src/daemon/request-timeouts.ts when +// timeout policy joined the descriptor registry (ADR-0011): the envelopes are now +// declared per command on the descriptors, so their values live beside them. + +const DAEMON_REQUEST_TIMEOUT_MS = 90_000; +export const PREPARE_REQUEST_TIMEOUT_MS = 240_000; + +// Keep this above the longest platform install subprocess timeout so the client +// envelope does not abort a still-progressing device install first. +export const INSTALL_REQUEST_TIMEOUT_MS = 180_000; + +/** + * The timeout policy most commands share: standard envelope, no user-supplied + * budget, and a daemon reset on timeout (a hung request usually means daemon + * state is suspect). Referenced explicitly by each descriptor — required, not + * inherited — so adding a command forces a decision (ADR-0011). Also the + * fallback for command names outside the registry (internal probes, unknown + * commands), which matches the old hand lists: not listed meant default + * envelope + reset-daemon. + */ +export const DEFAULT_TIMEOUT_POLICY: CommandTimeoutPolicy = { + budget: { source: 'none' }, + envelopeMs: DAEMON_REQUEST_TIMEOUT_MS, + onTimeout: 'reset-daemon', +}; diff --git a/src/core/command-descriptor/types.ts b/src/core/command-descriptor/types.ts index 31843db739..f5548f4339 100644 --- a/src/core/command-descriptor/types.ts +++ b/src/core/command-descriptor/types.ts @@ -10,6 +10,47 @@ import type { DaemonCommandDescriptor } from '../../daemon/daemon-command-regist */ export type DaemonCommandTraits = Omit; +/** + * Where a command's user-facing time budget comes from (ADR-0011, "Timeout + * policy joins the descriptor registry"). + * + * - `'none'` — the command has no user-supplied budget; the request + * envelope is exactly `envelopeMs`. + * - `'flag'` — the `--timeout` flag (`flags.timeoutMs`) overrides the + * envelope when present. + * - `'positional-parser'`— the budget travels inside the positionals; `parser` + * extracts it (or returns null when none was given). + * The client widens the envelope to + * budget + margin, never shrinking below `envelopeMs`. + */ +export type CommandTimeoutBudget = + | { source: 'none' } + | { source: 'flag' } + | { source: 'positional-parser'; parser: (positionals: string[]) => number | null }; + +/** + * The request-envelope + on-timeout daemon policy for one command. This is what + * used to live in two hand-maintained client lists (`isExplicitTimeoutCommand` + * in daemon-client.ts and `DAEMON_PRESERVING_TIMEOUT_COMMANDS` in + * daemon-client-timeout.ts) — the split that let `wait` fall through both + * (#1075). Declared per descriptor so a new command must decide, and read by + * the daemon client via `resolveCommandTimeoutPolicy`. + * + * - `envelopeMs` — the base client request envelope; `'unbounded'` disables the + * client-side timeout entirely (only `test`, which streams + * per-scenario progress and has its own budgets downstream). + * - `onTimeout` — whether a timed-out request tears the local daemon down + * (`'reset-daemon'`) or keeps it alive so sessions survive and + * evidence commands still work (`'preserve-daemon'`; read-only + * capture/polling commands that can block in platform + * accessibility bridges). + */ +export type CommandTimeoutPolicy = { + budget: CommandTimeoutBudget; + envelopeMs: number | 'unbounded'; + onTimeout: 'preserve-daemon' | 'reset-daemon'; +}; + /** * The single additive command-descriptor shape (ADR-0008, Phase 1 step 1). * @@ -23,10 +64,15 @@ export type DaemonCommandTraits = Omit; * - `batchable` — whether the command is exposed through `batch` * (from STRUCTURED_BATCH_COMMAND_NAMES). * - `mcpExposed` — whether the command is surfaced over MCP. + * - `timeoutPolicy` — the request-envelope budget source + on-timeout daemon + * policy (ADR-0011). REQUIRED on every entry — most commands + * share the explicit `DEFAULT_TIMEOUT_POLICY` constant, but a + * new command must say so rather than inherit silently. * - * This registry is dormant: nothing reads it yet. It exists only to be proven - * byte-equal to the live hand tables by the parity tests, as the strangler-fig - * foundation for later slices that flip consumers and delete the hand tables. + * The registry started dormant (proven byte-equal to the hand tables by the + * parity tests) and is now the live source: the daemon registry, capability + * matrix, batch allowlist, and the daemon client's timeout policy are all + * built from it. */ export type CommandDescriptor = { name: string; @@ -34,6 +80,7 @@ export type CommandDescriptor = { capability?: CommandCapability; batchable: boolean; mcpExposed: boolean; + timeoutPolicy: CommandTimeoutPolicy; }; /** Identity helper that pins each entry to the {@link CommandDescriptor} shape. */ diff --git a/src/core/wait-positionals.ts b/src/core/wait-positionals.ts index 124fbf59c3..ca6452064d 100644 --- a/src/core/wait-positionals.ts +++ b/src/core/wait-positionals.ts @@ -33,3 +33,16 @@ export function parseWaitPositionals(args: string[]): WaitParsed | null { const text = timeoutMs !== null ? args.slice(0, -1).join(' ') : args.join(' '); return { kind: 'text', text: text.trim(), timeoutMs }; } + +/** + * The user-supplied budget of a `wait` invocation, or null when none was given. + * The budget travels as a positional, not a flag, so it is parsed the same way + * the daemon will. Referenced by the `wait` descriptor's timeout policy + * (ADR-0011) so the client's request envelope can extend past it. + */ +export function resolveWaitBudgetMs(positionals: string[]): number | null { + const parsed = parseWaitPositionals(positionals); + if (!parsed) return null; + if (parsed.kind === 'sleep') return parsed.durationMs; + return parsed.timeoutMs; +} diff --git a/src/daemon/client/daemon-client-timeout.ts b/src/daemon/client/daemon-client-timeout.ts index 37f42c1709..dd19096978 100644 --- a/src/daemon/client/daemon-client-timeout.ts +++ b/src/daemon/client/daemon-client-timeout.ts @@ -3,6 +3,7 @@ import { runCmdSync } from '../../utils/exec.ts'; import { emitDiagnostic } from '../../utils/diagnostics.ts'; import { isAgentDeviceDaemonProcess } from '../../utils/process-identity.ts'; import { PUBLIC_COMMANDS } from '../../command-catalog.ts'; +import { resolveCommandTimeoutPolicy } from '../../core/command-descriptor/registry.ts'; import type { DaemonPaths } from '../config.ts'; import { removeDaemonInfo, @@ -52,21 +53,13 @@ export function handleRequestTimeout( }); } -// Read-only capture/polling commands that can block in platform accessibility -// bridges while the app is crashed or never idle. `wait` and `find` are repeated -// snapshot captures, so they share snapshot's failure mode. Keep the -// daemon/session alive on their timeouts so callers can still collect -// screenshot/perf/log evidence and close the session after the runner abort -// path has been triggered — resetting the daemon here turned one timed-out wait -// into a lost session for every session the daemon owned. -const DAEMON_PRESERVING_TIMEOUT_COMMANDS: ReadonlySet = new Set([ - PUBLIC_COMMANDS.snapshot, - PUBLIC_COMMANDS.wait, - PUBLIC_COMMANDS.find, -]); - +// Whether a timed-out request tears down the local daemon is declared on the +// command's descriptor (ADR-0011, `timeoutPolicy.onTimeout`): read-only +// capture/polling commands preserve the daemon so sessions survive and evidence +// commands still work; everything else resets it. Unknown/undefined commands +// fall back to the default reset-daemon policy. export function shouldResetDaemonAfterRequestTimeout(command: string | undefined): boolean { - return command === undefined || !DAEMON_PRESERVING_TIMEOUT_COMMANDS.has(command); + return resolveCommandTimeoutPolicy(command).onTimeout === 'reset-daemon'; } function resolveRequestTimeoutHint(params: { diff --git a/src/daemon/client/daemon-client.ts b/src/daemon/client/daemon-client.ts index 7db474ec95..2124cc1c14 100644 --- a/src/daemon/client/daemon-client.ts +++ b/src/daemon/client/daemon-client.ts @@ -5,7 +5,7 @@ import type { import type { RequestProgressSink } from '../request-progress.ts'; import { createRequestId, emitDiagnostic, withDiagnosticTimer } from '../../utils/diagnostics.ts'; import { INTERNAL_COMMANDS, PUBLIC_COMMANDS } from '../../command-catalog.ts'; -import { parseWaitPositionals } from '../../core/wait-positionals.ts'; +import { resolveCommandTimeoutPolicy } from '../../core/command-descriptor/registry.ts'; import { prepareRemoteRequestArtifacts } from '../../remote/daemon-artifacts.ts'; import { cleanupDaemonAfterRequest, @@ -13,11 +13,6 @@ import { resolveClientSettings, } from './daemon-client-lifecycle.ts'; import { sendRequest } from './daemon-client-transport.ts'; -import { - DAEMON_REQUEST_TIMEOUT_MS, - INSTALL_REQUEST_TIMEOUT_MS, - PREPARE_REQUEST_TIMEOUT_MS, -} from '../request-timeouts.ts'; export { computeDaemonCodeSignature } from '../code-signature.ts'; export { downloadRemoteArtifact } from '../../remote/daemon-artifacts.ts'; @@ -122,42 +117,29 @@ function isInstallLikeCommand(command: string | undefined): boolean { ); } +// Derives the request envelope from the command's declared timeout policy +// (ADR-0011) instead of the former per-command-name special cases. export function resolveDaemonRequestTimeoutMs( req: Omit, ): number | undefined { - if (req.command === PUBLIC_COMMANDS.test) return undefined; - if (req.command === PUBLIC_COMMANDS.wait) { - // The wait budget travels as a positional, not a flag, so parse it the same - // way the daemon will. Without this, a `wait ... 180000` dies at the default - // request timeout with the runner/daemon torn down as collateral. - const waitBudgetMs = resolveWaitRequestBudgetMs(req.positionals); - if (waitBudgetMs !== null) { - return Math.max(DAEMON_REQUEST_TIMEOUT_MS, waitBudgetMs + WAIT_REQUEST_TIMEOUT_MARGIN_MS); + const policy = resolveCommandTimeoutPolicy(req.command); + if (policy.envelopeMs === 'unbounded') return undefined; + if (policy.budget.source === 'positional-parser') { + // The user budget travels inside the positionals (e.g. `wait ... 180000`). + // Without extending the envelope past it, the request dies at the default + // timeout with the runner/daemon torn down as collateral (#1075). + const budgetMs = policy.budget.parser(req.positionals ?? []); + if (budgetMs !== null) { + return Math.max(policy.envelopeMs, budgetMs + REQUEST_TIMEOUT_BUDGET_MARGIN_MS); } } - if (typeof req.flags?.timeoutMs === 'number' && isExplicitTimeoutCommand(req.command)) { + if (policy.budget.source === 'flag' && typeof req.flags?.timeoutMs === 'number') { return req.flags.timeoutMs; } - if (req.command === PUBLIC_COMMANDS.prepare) return PREPARE_REQUEST_TIMEOUT_MS; - if (isInstallLikeCommand(req.command)) return INSTALL_REQUEST_TIMEOUT_MS; - return DAEMON_REQUEST_TIMEOUT_MS; + return policy.envelopeMs; } -function isExplicitTimeoutCommand(command: string | undefined): boolean { - return ( - command === PUBLIC_COMMANDS.prepare || - command === PUBLIC_COMMANDS.replay || - command === PUBLIC_COMMANDS.snapshot - ); -} - -// Margin over the user-supplied wait budget so the daemon-side timeout result -// (with its stable/wait diagnostics) wins the race against the client envelope. -const WAIT_REQUEST_TIMEOUT_MARGIN_MS = 30_000; - -function resolveWaitRequestBudgetMs(positionals: string[] | undefined): number | null { - const parsed = parseWaitPositionals(positionals ?? []); - if (!parsed) return null; - if (parsed.kind === 'sleep') return parsed.durationMs; - return parsed.timeoutMs; -} +// Margin over a user-supplied positional budget so the daemon-side timeout +// result (with its stable/wait diagnostics) wins the race against the client +// envelope. Never shrinks the envelope below the command's declared base. +const REQUEST_TIMEOUT_BUDGET_MARGIN_MS = 30_000; diff --git a/src/daemon/handlers/session.ts b/src/daemon/handlers/session.ts index 917a867148..93e3d9f179 100644 --- a/src/daemon/handlers/session.ts +++ b/src/daemon/handlers/session.ts @@ -39,7 +39,7 @@ import { handleSessionReplayCommands } from './session-replay.ts'; import { handleDoctorCommand } from './session-doctor.ts'; import { getSessionCommandKind } from '../daemon-command-registry.ts'; import { LeaseRegistry } from '../lease-registry.ts'; -import { PREPARE_REQUEST_TIMEOUT_MS } from '../request-timeouts.ts'; +import { PREPARE_REQUEST_TIMEOUT_MS } from '../../core/command-descriptor/timeout-policy.ts'; import { Deadline } from '../../utils/retry.ts'; import type { LeaseLifecycleProvider } from './lease.ts'; diff --git a/src/daemon/request-timeouts.ts b/src/daemon/request-timeouts.ts deleted file mode 100644 index a726a2467d..0000000000 --- a/src/daemon/request-timeouts.ts +++ /dev/null @@ -1,6 +0,0 @@ -export const DAEMON_REQUEST_TIMEOUT_MS = 90_000; -export const PREPARE_REQUEST_TIMEOUT_MS = 240_000; - -// Keep this above the longest platform install subprocess timeout so the client -// envelope does not abort a still-progressing device install first. -export const INSTALL_REQUEST_TIMEOUT_MS = 180_000;