mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
fix(browser): pass orca wait's timeout through to agent-browser (#34283)
Condition waits never forwarded --timeout, so agent-browser always stopped at its own 25s default and --timeout 60000 still failed at 25s. orcad's external Chromium run and Electron sidecar socket also capped waits at a fixed 90s. One builder now passes --timeout (explicit or the documented 25000 default) on the local and orcad paths, process kills sit 5s above it, and every forwarding hop waits max(60s, T+30s).
This commit is contained in:
@@ -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 }
|
||||
)
|
||||
})
|
||||
|
||||
|
||||
@@ -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<string, CommandHandler> = {
|
||||
snapshot: async ({ flags, client, cwd, json }) => {
|
||||
@@ -109,9 +107,7 @@ export const BROWSER_NAV_HANDLERS: Record<string, CommandHandler> = {
|
||||
state,
|
||||
...target
|
||||
},
|
||||
{
|
||||
timeoutMs: timeout ? timeout + 5000 : DEFAULT_BROWSER_WAIT_RPC_TIMEOUT_MS
|
||||
}
|
||||
{ timeoutMs: browserWaitRequestTimeoutMs(timeout) }
|
||||
)
|
||||
printResult(result, json, (v) => JSON.stringify(v, null, 2))
|
||||
},
|
||||
|
||||
@@ -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.`
|
||||
]
|
||||
},
|
||||
{
|
||||
|
||||
@@ -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<BrowserWaitResult> {
|
||||
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
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
@@ -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 &&
|
||||
|
||||
@@ -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'])
|
||||
|
||||
@@ -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<string, true> = {
|
||||
'--args': true,
|
||||
'--cdp': true,
|
||||
@@ -64,25 +65,20 @@ function stripTargetOverrideArguments(args: string[]): string[] {
|
||||
return stripped
|
||||
}
|
||||
|
||||
function waitArguments(params: Record<string, unknown>): 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<string, unknown>
|
||||
): ReturnType<typeof agentBrowserWaitArguments> {
|
||||
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, unknown>): 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,
|
||||
|
||||
@@ -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<string, unknown>): 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<string, unknown> {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
Reference in New Issue
Block a user