diff --git a/src/cli/browser.test.ts b/src/cli/browser.test.ts index cc3529d2c9b..9a7887fc69e 100644 --- a/src/cli/browser.test.ts +++ b/src/cli/browser.test.ts @@ -710,7 +710,7 @@ describe('orca cli browser waits and viewport flags', () => { vi.spyOn(console, 'log').mockImplementation(() => {}) await main( - ['wait', '--selector', '#ready', '--timeout', '12000', '--worktree', 'all', '--json'], + ['wait', '--selector', '#ready', '--timeout', '45000', '--worktree', 'all', '--json'], '/tmp/not-an-orca-worktree' ) @@ -718,7 +718,7 @@ describe('orca cli browser waits and viewport flags', () => { 'browser.wait', { selector: '#ready', - timeout: 12000, + timeout: 45000, text: undefined, url: undefined, load: undefined, @@ -726,7 +726,7 @@ describe('orca cli browser waits and viewport flags', () => { state: undefined, worktree: undefined }, - { timeoutMs: 17000 } + { timeoutMs: 75000 } ) }) diff --git a/src/cli/handlers/browser-nav.ts b/src/cli/handlers/browser-nav.ts index 2ce1c2bde8e..41ad8647755 100644 --- a/src/cli/handlers/browser-nav.ts +++ b/src/cli/handlers/browser-nav.ts @@ -18,9 +18,7 @@ import { } from '../flags' import { RuntimeClientError } from '../runtime-client' import { getBrowserCommandTarget } from '../selectors' - -// Browser waits can outlast ordinary RPCs without indicating a disconnected runtime. -const DEFAULT_BROWSER_WAIT_RPC_TIMEOUT_MS = 60_000 +import { browserWaitRequestTimeoutMs } from '../../shared/browser-wait-timeout' export const BROWSER_NAV_HANDLERS: Record = { snapshot: async ({ flags, client, cwd, json }) => { @@ -109,9 +107,7 @@ export const BROWSER_NAV_HANDLERS: Record = { state, ...target }, - { - timeoutMs: timeout ? timeout + 5000 : DEFAULT_BROWSER_WAIT_RPC_TIMEOUT_MS - } + { timeoutMs: browserWaitRequestTimeoutMs(timeout) } ) printResult(result, json, (v) => JSON.stringify(v, null, 2)) }, diff --git a/src/cli/specs/browser-basic.ts b/src/cli/specs/browser-basic.ts index d03d8e5fe6a..0d92a115d69 100644 --- a/src/cli/specs/browser-basic.ts +++ b/src/cli/specs/browser-basic.ts @@ -1,5 +1,6 @@ import type { CommandSpec } from '../args' import { GLOBAL_FLAGS } from '../args' +import { BROWSER_WAIT_DEFAULT_TIMEOUT_MS } from '../../shared/browser-wait-timeout' export const BROWSER_BASIC_COMMAND_SPECS: CommandSpec[] = [ { @@ -102,6 +103,9 @@ export const BROWSER_BASIC_COMMAND_SPECS: CommandSpec[] = [ 'fn', 'state', 'worktree' + ], + notes: [ + `With a condition, --timeout is how long to wait for it (default ${BROWSER_WAIT_DEFAULT_TIMEOUT_MS} ms). Without one, --timeout is a plain pause.` ] }, { diff --git a/src/main/browser/agent-browser-bridge-interaction-commands.ts b/src/main/browser/agent-browser-bridge-interaction-commands.ts index 9d476f6014b..8e07f983cc5 100644 --- a/src/main/browser/agent-browser-bridge-interaction-commands.ts +++ b/src/main/browser/agent-browser-bridge-interaction-commands.ts @@ -11,7 +11,10 @@ import type { BrowserPdfResult } from '../../shared/runtime-types' import { BrowserError } from './cdp-bridge' -import { WAIT_PROCESS_TIMEOUT_GRACE_MS } from './agent-browser-bridge-types' +import { + agentBrowserWaitArguments, + type AgentBrowserWaitOptions +} from './agent-browser-wait-arguments' import { acquireElectronDebugger } from './electron-debugger-lease' import { parseCdpKeyEvent, imeFallbackKeyEvent } from './cdp-keyboard-us-layout' import { dispatchPageKeyEvent, type GuestCdpSender } from './guest-direct-key-input' @@ -57,56 +60,24 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser } async wait( - options?: { - selector?: string - timeout?: number - text?: string - url?: string - load?: string - fn?: string - state?: string - }, + options?: AgentBrowserWaitOptions, worktreeId?: string, browserPageId?: string ): Promise { return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName) => { - const args = ['wait'] - const hasCondition = - !!options?.selector || !!options?.text || !!options?.url || !!options?.load || !!options?.fn - if (options?.selector) { - args.push(options.selector) - } else if (options?.timeout != null && !hasCondition) { - args.push(String(options.timeout)) - } - if (options?.text) { - args.push('--text', options.text) - } - if (options?.url) { - args.push('--url', options.url) - } - if (options?.load) { - args.push('--load', options.load) - } - if (options?.fn) { - args.push('--fn', options.fn) - } - const normalizedState = options?.state === 'visible' ? undefined : options?.state - if (normalizedState) { - args.push('--state', normalizedState) - } - // Bound condition waits here because agent-browser lacks a per-command timeout. + const { args, conditionTimeoutMs, processTimeoutMs } = agentBrowserWaitArguments( + options ?? {} + ) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: agent-browser's wait --json answers the BrowserWaitResult shape. return (await this.execAgentBrowser(sessionName, args, { - timeoutMs: - options?.timeout != null && hasCondition - ? options.timeout + WAIT_PROCESS_TIMEOUT_GRACE_MS - : undefined, + timeoutMs: processTimeoutMs, timeoutError: - options?.timeout != null && hasCondition - ? new BrowserError( + conditionTimeoutMs === undefined + ? undefined + : new BrowserError( 'browser_timeout', - `Timed out waiting for browser condition after ${options.timeout}ms.` + `Timed out waiting for browser condition after ${conditionTimeoutMs}ms.` ) - : undefined })) as BrowserWaitResult }) } diff --git a/src/main/browser/agent-browser-bridge-types.ts b/src/main/browser/agent-browser-bridge-types.ts index 1cbb884de44..84b0a88a638 100644 --- a/src/main/browser/agent-browser-bridge-types.ts +++ b/src/main/browser/agent-browser-bridge-types.ts @@ -2,10 +2,9 @@ import type { ChildProcess } from 'node:child_process' import type { CdpWsProxy } from './cdp-ws-proxy' import type { BrowserError } from './cdp-bridge' -// Exceed agent-browser's internal navigation (30s) and wait (60s) timeouts. +// Exceed agent-browser's internal navigation (30s) and default wait (25s) timeouts. export const EXEC_TIMEOUT_MS = 90_000 export const CONSECUTIVE_TIMEOUT_LIMIT = 3 -export const WAIT_PROCESS_TIMEOUT_GRACE_MS = 1_000 export const STALE_SESSION_CLOSE_TIMEOUT_MS = 3_000 export const AGENT_BROWSER_EXIT_DRAIN_MS = 500 // Cleanup must finish within the 20s quit barrier. diff --git a/src/main/browser/agent-browser-wait-arguments.test.ts b/src/main/browser/agent-browser-wait-arguments.test.ts new file mode 100644 index 00000000000..f09f9df1d8c --- /dev/null +++ b/src/main/browser/agent-browser-wait-arguments.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from 'vitest' +import { agentBrowserWaitArguments } from './agent-browser-wait-arguments' + +describe('agentBrowserWaitArguments', () => { + it.each([ + [ + { fn: 'window.ready', timeout: 60_000 }, + ['wait', '--fn', 'window.ready', '--timeout', '60000'], + 60_000, + 65_000 + ], + [{ selector: '#done' }, ['wait', '#done', '--timeout', '25000'], 25_000, 30_000], + [{ timeout: 40_000 }, ['wait', '40000'], undefined, undefined] + ])('builds %j', (options, args, conditionTimeoutMs, processTimeoutMs) => { + expect(agentBrowserWaitArguments(options)).toEqual({ + args, + conditionTimeoutMs, + processTimeoutMs + }) + }) +}) diff --git a/src/main/browser/agent-browser-wait-arguments.ts b/src/main/browser/agent-browser-wait-arguments.ts new file mode 100644 index 00000000000..183bf4c92a9 --- /dev/null +++ b/src/main/browser/agent-browser-wait-arguments.ts @@ -0,0 +1,49 @@ +import { BROWSER_WAIT_DEFAULT_TIMEOUT_MS } from '../../shared/browser-wait-timeout' + +// Lets agent-browser report its own wait timeout before Orca kills the process. +const WAIT_PROCESS_TIMEOUT_GRACE_MS = 5_000 + +export type AgentBrowserWaitOptions = { + selector?: string + timeout?: number + text?: string + url?: string + load?: string + fn?: string + state?: string +} + +export function agentBrowserWaitArguments(options: AgentBrowserWaitOptions): { + args: string[] + conditionTimeoutMs?: number + processTimeoutMs?: number +} { + if (!options.selector && !options.text && !options.url && !options.load && !options.fn) { + return { + args: options.timeout == null ? ['wait'] : ['wait', String(options.timeout)] + } + } + const args = ['wait'] + if (options.selector) { + args.push(options.selector) + } + for (const [value, flag] of [ + [options.text, '--text'], + [options.url, '--url'], + [options.load, '--load'], + [options.fn, '--fn'], + [options.state === 'visible' ? undefined : options.state, '--state'] + ] as const) { + if (value) { + args.push(flag, value) + } + } + // Why: without --timeout agent-browser stops at its own default, whatever Orca was asked for. + const conditionTimeoutMs = options.timeout ?? BROWSER_WAIT_DEFAULT_TIMEOUT_MS + args.push('--timeout', String(conditionTimeoutMs)) + return { + args, + conditionTimeoutMs, + processTimeoutMs: conditionTimeoutMs + WAIT_PROCESS_TIMEOUT_GRACE_MS + } +} diff --git a/src/main/orcad/electron-serve-browser-process.ts b/src/main/orcad/electron-serve-browser-process.ts index ff4a354ceeb..d428ff00c9f 100644 --- a/src/main/orcad/electron-serve-browser-process.ts +++ b/src/main/orcad/electron-serve-browser-process.ts @@ -17,6 +17,7 @@ import { BROWSER_SIDECAR_OWNER_ENV } from '../../shared/browser-sidecar-lifetime import { electronIsolatedProfileSwitches } from '../../shared/electron-isolated-profile-switches' import { headlessServeOzonePlatformSwitches } from '../../shared/headless-serve-ozone-platform' import { sendOrcadSidecarRequest } from './orcad-sidecar-runtime-client' +import { browserWaitRequestTimeoutMs } from '../../shared/browser-wait-timeout' import { electronServeEnvironment } from './electron-serve-browser-environment' import { ElectronSidecarTabRegistry, @@ -256,7 +257,12 @@ export class ElectronServeBrowserProcess { let result: unknown try { - result = await sendOrcadSidecarRequest(metadata, rpcMethod, params) + result = await sendOrcadSidecarRequest( + metadata, + rpcMethod, + params, + method === 'browserWait' ? browserWaitRequestTimeoutMs(params.timeout) : undefined + ) } catch (error) { if ( targetPage && diff --git a/src/main/orcad/external-chromium-browser-process.ts b/src/main/orcad/external-chromium-browser-process.ts index 1bf8eca3015..d9bc75e92cc 100644 --- a/src/main/orcad/external-chromium-browser-process.ts +++ b/src/main/orcad/external-chromium-browser-process.ts @@ -9,6 +9,7 @@ import { import { normalizeBrowserNavigationUrl } from '../../shared/browser-url' import { externalChromiumCommandArguments, + externalChromiumWaitCommand, normalizeExternalChromiumCommandResult } from './external-chromium-command-arguments' import { @@ -160,6 +161,11 @@ export class ExternalChromiumBrowserProcess { if (method === 'browserEval') { return this.session.run(['eval', String(params.expression ?? '')]) } + if (method === 'browserWait') { + const { args, processTimeoutMs } = externalChromiumWaitCommand(params) + await this.session.run(args, processTimeoutMs) + return { waited: true } + } if (method === 'browserSelectAll') { await this.session.run(['focus', String(params.element ?? '')]) await this.session.run(['press', 'Control+a']) diff --git a/src/main/orcad/external-chromium-command-arguments.ts b/src/main/orcad/external-chromium-command-arguments.ts index 8c93b01955d..103432e2abf 100644 --- a/src/main/orcad/external-chromium-command-arguments.ts +++ b/src/main/orcad/external-chromium-command-arguments.ts @@ -1,5 +1,6 @@ import { BrowserError } from '../browser/browser-error' import { agentBrowserSetMediaArguments } from '../browser/agent-browser-set-media-arguments' +import { agentBrowserWaitArguments } from '../browser/agent-browser-wait-arguments' const TARGET_OVERRIDE_FLAGS: Record = { '--args': true, '--cdp': true, @@ -64,25 +65,20 @@ function stripTargetOverrideArguments(args: string[]): string[] { return stripped } -function waitArguments(params: Record): string[] { - const args = ['wait'] - if (params.selector) { - args.push(String(params.selector)) - } else if (params.timeout != null && !params.text && !params.url && !params.load && !params.fn) { - args.push(String(params.timeout)) - } - for (const [key, flag] of [ - ['text', '--text'], - ['url', '--url'], - ['load', '--load'], - ['fn', '--fn'], - ['state', '--state'] - ] as const) { - if (params[key]) { - args.push(flag, String(params[key])) - } - } - return args +export function externalChromiumWaitCommand( + params: Record +): ReturnType { + const optional = (key: string): string | undefined => + params[key] ? String(params[key]) : undefined + return agentBrowserWaitArguments({ + selector: optional('selector'), + timeout: typeof params.timeout === 'number' ? params.timeout : undefined, + text: optional('text'), + url: optional('url'), + load: optional('load'), + fn: optional('fn'), + state: optional('state') + }) } function cookieSetArguments(params: Record): string[] { @@ -236,8 +232,6 @@ export function externalChromiumCommandArguments( return null } return ['set', 'viewport', text('width'), text('height')] - case 'browserWait': - return waitArguments(params) case 'browserCookieSet': return cookieSetArguments(params) case 'browserInterceptEnable': @@ -279,8 +273,6 @@ export function normalizeExternalChromiumCommandResult( return { selected: String(params.element) } case 'browserKeypress': return { pressed: String(params.key) } - case 'browserWait': - return { waited: true } case 'browserSetViewport': return { width: params.width, diff --git a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-route.ts b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-route.ts index 0f8b7968392..b9ae9896867 100644 --- a/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-route.ts +++ b/src/main/runtime/desktop-browser-bridge/desktop-browser-bridge-route.ts @@ -6,6 +6,7 @@ import { DESKTOP_BROWSER_BRIDGE_UNAVAILABLE_MESSAGE, isDesktopBrowserBridgeMethod } from '../../../shared/desktop-browser-bridge-protocol' +import { browserWaitRequestTimeoutMs } from '../../../shared/browser-wait-timeout' import type { RpcCallerIdentity } from '../rpc/rpc-caller-identity' import { assertClientOpenableUrl } from '../browser/browser-client-page-opening' import type { DesktopBrowserBridgeRegistry } from './desktop-browser-bridge-registry' @@ -13,7 +14,6 @@ import type { DesktopBrowserBridgeRegistry } from './desktop-browser-bridge-regi export type DesktopBrowserBridgeRoute = { handled: false } | { handled: true; result: unknown } const DEFAULT_TIMEOUT_MS = 60_000 -const WAIT_MARGIN_MS = 30_000 const MAX_TIMEOUT_MS = 10 * 60_000 export async function routeDesktopBrowserBridgeRpc(options: { @@ -75,9 +75,9 @@ export async function routeDesktopBrowserBridgeRpc(options: { } function forwardTimeoutMs(method: string, params: Record): number { - const requested = typeof params.timeout === 'number' ? params.timeout : 0 - const base = method === 'browser.wait' ? requested + WAIT_MARGIN_MS : DEFAULT_TIMEOUT_MS - return Math.min(Math.max(base, DEFAULT_TIMEOUT_MS), MAX_TIMEOUT_MS) + return method === 'browser.wait' + ? Math.min(browserWaitRequestTimeoutMs(params.timeout), MAX_TIMEOUT_MS) + : DEFAULT_TIMEOUT_MS } function isRecord(value: unknown): value is Record { diff --git a/src/shared/browser-wait-timeout.ts b/src/shared/browser-wait-timeout.ts new file mode 100644 index 00000000000..926d14c1676 --- /dev/null +++ b/src/shared/browser-wait-timeout.ts @@ -0,0 +1,7 @@ +/** How long `orca wait` waits for a condition when no --timeout is given. */ +export const BROWSER_WAIT_DEFAULT_TIMEOUT_MS = 25_000 + +/** Deadline for a hop that forwards `browser.wait`; it outlives the wait so the host reports the timeout. */ +export function browserWaitRequestTimeoutMs(timeout: unknown): number { + return Math.max(60_000, (typeof timeout === 'number' ? timeout : 0) + 30_000) +}