diff --git a/src/cli/handlers/orchestration-timeout.test.ts b/src/cli/handlers/orchestration-timeout.test.ts new file mode 100644 index 000000000..342475c81 --- /dev/null +++ b/src/cli/handlers/orchestration-timeout.test.ts @@ -0,0 +1,284 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const callMock = vi.fn() +const getTerminalHandleMock = vi.hoisted(() => vi.fn()) +const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE +const originalPaneKey = process.env.ORCA_PANE_KEY + +vi.mock('../format', () => ({ printResult: vi.fn() })) +vi.mock('../selectors', () => ({ getTerminalHandle: getTerminalHandleMock })) + +import { printResult } from '../format' +import { ORCHESTRATION_HANDLERS } from './orchestration' + +const invalidTimeoutValues: [string, string | boolean][] = [ + ['missing', true], + ['empty', ''], + ['non-numeric', 'not-a-number'], + ['zero', '0'], + ['negative', '-1'], + ['fractional', '1.5'], + ['rounded fractional', '9007199254740991.1'], + ['unsafe integer', String(Number.MAX_SAFE_INTEGER + 1)] +] + +const invokeCheck = (flags: Map) => + ORCHESTRATION_HANDLERS['orchestration check']({ + flags, + client: { call: callMock }, + cwd: '/tmp/repo', + json: true + } as never) + +const invokeAsk = (flags: Map) => + ORCHESTRATION_HANDLERS['orchestration ask']({ + flags, + client: { call: callMock }, + cwd: '/tmp/repo', + json: true + } as never) + +beforeEach(() => { + callMock.mockReset() + getTerminalHandleMock.mockReset() + vi.mocked(printResult).mockReset() + delete process.env.ORCA_TERMINAL_HANDLE + delete process.env.ORCA_PANE_KEY +}) + +afterEach(() => { + if (originalTerminalHandle === undefined) { + delete process.env.ORCA_TERMINAL_HANDLE + } else { + process.env.ORCA_TERMINAL_HANDLE = originalTerminalHandle + } + if (originalPaneKey === undefined) { + delete process.env.ORCA_PANE_KEY + } else { + process.env.ORCA_PANE_KEY = originalPaneKey + } + vi.restoreAllMocks() +}) + +describe('orchestration timeout flag validation', () => { + it.each(invalidTimeoutValues)('rejects invalid check --timeout-ms: %s', async (_label, value) => { + const flags = new Map([ + ['wait', true], + ['timeout-ms', value] + ]) + + await expect(invokeCheck(flags)).rejects.toThrow(/--timeout-ms/) + expect(callMock).not.toHaveBeenCalled() + expect(getTerminalHandleMock).not.toHaveBeenCalled() + }) + + it('passes a parsed check timeout and peek mode into the RPC payload', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ result: { messages: [], count: 0 } }) + + await invokeCheck( + new Map([ + ['wait', true], + ['peek', true], + ['timeout-ms', '250'] + ]) + ) + + expect(callMock).toHaveBeenCalledWith('orchestration.check', { + terminal: 'term_worker', + unread: false, + peek: true, + all: undefined, + types: undefined, + inject: undefined, + wait: true, + timeoutMs: 250 + }) + }) + + it('filters already-read rows from a peek response for pre-peek runtimes', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ + result: { + messages: [ + { id: 'msg_old', from_handle: 'a', subject: 'seen', read: 1 }, + { id: 'msg_new', from_handle: 'a', subject: 'fresh', read: 0 } + ], + count: 2, + formatted: 'banners built from all rows' + } + }) + + await invokeCheck(new Map([['peek', true]])) + + const response = vi.mocked(printResult).mock.calls[0]?.[0] as { + result: { messages: { id: string }[]; count: number; formatted?: string } + } + expect(response.result.messages.map((message) => message.id)).toEqual(['msg_new']) + expect(response.result.count).toBe(1) + expect(response.result.formatted).toBeUndefined() + }) + + it('rejects combined read modes before calling the runtime', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + + await expect( + invokeCheck( + new Map([ + ['unread', true], + ['peek', true] + ]) + ) + ).rejects.toMatchObject({ + code: 'invalid_argument', + message: expect.stringContaining('read mode') + }) + expect(callMock).not.toHaveBeenCalled() + }) + + it('warns when a pre-peek runtime returned a full 100-row page', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + const rows = Array.from({ length: 100 }, (_, index) => ({ + id: `msg_${index}`, + from_handle: 'a', + subject: `s${index}`, + read: index === 0 ? 0 : 1 + })) + callMock.mockResolvedValue({ result: { messages: rows, count: 100 } }) + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + await invokeCheck(new Map([['peek', true]])) + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining('newest 100 messages')) + }) + + it('fails --peek --wait against a runtime that returned only read rows', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ + result: { + messages: [{ id: 'msg_old', from_handle: 'a', subject: 'seen', read: 1 }], + count: 1 + } + }) + + await expect( + invokeCheck( + new Map([ + ['peek', true], + ['wait', true] + ]) + ) + ).rejects.toMatchObject({ code: 'peek_wait_unsupported' }) + }) + + it.each(invalidTimeoutValues)('rejects invalid ask --timeout-ms: %s', async (_label, value) => { + const flags = new Map([ + ['to', 'term_coord'], + ['question', 'Proceed?'], + ['timeout-ms', value] + ]) + + await expect(invokeAsk(flags)).rejects.toThrow(/--timeout-ms/) + expect(callMock).not.toHaveBeenCalled() + expect(getTerminalHandleMock).not.toHaveBeenCalled() + }) + + it.each([String(2_147_483_647), String(Number.MAX_SAFE_INTEGER)])( + 'clamps a safe ask timeout %s before adding transport headroom', + async (rawTimeout) => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ + result: { answer: 'yes', messageId: 'msg_1', threadId: 'thread_1', timedOut: false } + }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await invokeAsk( + new Map([ + ['to', 'term_coord'], + ['question', 'Proceed?'], + ['timeout-ms', rawTimeout] + ]) + ) + + expect(callMock).toHaveBeenCalledWith( + 'orchestration.ask', + expect.objectContaining({ timeoutMs: 1_800_000 }), + { timeoutMs: 1_805_000 } + ) + } + ) + + it.each(['+1000', '1000.0', '1e3', '0x3e8'])( + 'preserves CLI-compatible exact integer timeout syntax %s', + async (rawTimeout) => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ + result: { answer: 'yes', messageId: 'msg_1', threadId: 'thread_1', timedOut: false } + }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await invokeAsk( + new Map([ + ['to', 'term_coord'], + ['question', 'Proceed?'], + ['timeout-ms', rawTimeout] + ]) + ) + + expect(callMock).toHaveBeenCalledWith( + 'orchestration.ask', + expect.objectContaining({ timeoutMs: 1_000 }), + { timeoutMs: 6_000 } + ) + } + ) + + it('keeps an omitted ask timeout out of the payload while using default headroom', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ + result: { answer: 'yes', messageId: 'msg_1', threadId: 'thread_1', timedOut: false } + }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await invokeAsk( + new Map([ + ['to', 'term_coord'], + ['question', 'Proceed?'] + ]) + ) + + expect(callMock).toHaveBeenCalledWith( + 'orchestration.ask', + expect.objectContaining({ timeoutMs: undefined }), + { timeoutMs: 605_000 } + ) + }) + + it('uses the parsed ask timeout for both runtime wait and client timeout', async () => { + process.env.ORCA_TERMINAL_HANDLE = 'term_worker' + callMock.mockResolvedValue({ + result: { answer: 'yes', messageId: 'msg_1', threadId: 'thread_1', timedOut: false } + }) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await invokeAsk( + new Map([ + ['to', 'term_coord'], + ['question', 'Proceed?'], + ['timeout-ms', '123'] + ]) + ) + + expect(callMock).toHaveBeenCalledWith( + 'orchestration.ask', + { + to: 'term_coord', + question: 'Proceed?', + options: undefined, + timeoutMs: 123, + from: 'term_worker' + }, + { timeoutMs: 5_123 } + ) + }) +}) diff --git a/src/cli/handlers/orchestration.test.ts b/src/cli/handlers/orchestration.test.ts index 2a8f6d44d..55f2482f5 100644 --- a/src/cli/handlers/orchestration.test.ts +++ b/src/cli/handlers/orchestration.test.ts @@ -670,198 +670,6 @@ describe('orchestration task-create caller handle', () => { }) }) -describe('orchestration timeout flag validation', () => { - const invalidTimeoutValues: [string, string | boolean][] = [ - ['missing', true], - ['empty', ''], - ['non-numeric', 'not-a-number'], - ['zero', '0'], - ['negative', '-1'] - ] - - beforeEach(() => { - callMock.mockReset() - delete process.env.ORCA_TERMINAL_HANDLE - delete process.env.ORCA_PANE_KEY - }) - - const invokeCheck = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration check']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) - - const invokeAsk = (flags: Map) => - ORCHESTRATION_HANDLERS['orchestration ask']({ - flags, - client: { call: callMock }, - cwd: '/tmp/repo', - json: true - } as never) - - it.each(invalidTimeoutValues)('rejects invalid check --timeout-ms: %s', async (_label, value) => { - const flags = new Map([ - ['wait', true], - ['timeout-ms', value] - ]) - - await expect(invokeCheck(flags)).rejects.toThrow(/--timeout-ms/) - expect(callMock).not.toHaveBeenCalled() - }) - - it('passes a parsed check timeout and peek mode into the RPC payload', async () => { - process.env.ORCA_TERMINAL_HANDLE = 'term_worker' - callMock.mockResolvedValue({ result: { messages: [], count: 0 } }) - - await invokeCheck( - new Map([ - ['wait', true], - ['peek', true], - ['timeout-ms', '250'] - ]) - ) - - // Why: --peek rides with unread:false so pre-peek runtimes fall back to - // the non-consuming all mode instead of the destructive mark-read default. - expect(callMock).toHaveBeenCalledWith('orchestration.check', { - terminal: 'term_worker', - unread: false, - peek: true, - all: undefined, - types: undefined, - inject: undefined, - wait: true, - timeoutMs: 250 - }) - }) - - it('filters already-read rows from a peek response for pre-peek runtimes', async () => { - process.env.ORCA_TERMINAL_HANDLE = 'term_worker' - callMock.mockResolvedValue({ - result: { - messages: [ - { id: 'msg_old', from_handle: 'a', subject: 'seen', read: 1 }, - { id: 'msg_new', from_handle: 'a', subject: 'fresh', read: 0 } - ], - count: 2, - formatted: 'banners built from all rows' - } - }) - vi.mocked(printResult).mockClear() - - await invokeCheck(new Map([['peek', true]])) - - const response = vi.mocked(printResult).mock.calls[0]?.[0] as { - result: { messages: { id: string }[]; count: number; formatted?: string } - } - expect(response.result.messages.map((m) => m.id)).toEqual(['msg_new']) - expect(response.result.count).toBe(1) - // Why: the pre-peek runtime built `formatted` from all rows, including - // the read one the filter just removed. - expect(response.result.formatted).toBeUndefined() - }) - - it('rejects combined read modes before calling the runtime', async () => { - process.env.ORCA_TERMINAL_HANDLE = 'term_worker' - callMock.mockClear() - - await expect( - invokeCheck( - new Map([ - ['unread', true], - ['peek', true] - ]) - ) - ).rejects.toMatchObject({ - code: 'invalid_argument', - message: expect.stringContaining('read mode') - }) - expect(callMock).not.toHaveBeenCalled() - }) - - it('warns when a pre-peek runtime returned a full 100-row page', async () => { - process.env.ORCA_TERMINAL_HANDLE = 'term_worker' - const rows = Array.from({ length: 100 }, (_, i) => ({ - id: `msg_${i}`, - from_handle: 'a', - subject: `s${i}`, - read: i === 0 ? 0 : 1 - })) - callMock.mockResolvedValue({ result: { messages: rows, count: 100 } }) - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) - - await invokeCheck(new Map([['peek', true]])) - - expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining('newest 100 messages')) - errorSpy.mockRestore() - }) - - it('fails --peek --wait against a runtime that returned only read rows', async () => { - process.env.ORCA_TERMINAL_HANDLE = 'term_worker' - callMock.mockResolvedValue({ - result: { - messages: [{ id: 'msg_old', from_handle: 'a', subject: 'seen', read: 1 }], - count: 1 - } - }) - - await expect( - invokeCheck( - new Map([ - ['peek', true], - ['wait', true] - ]) - ) - ).rejects.toMatchObject({ code: 'peek_wait_unsupported' }) - }) - - it.each(invalidTimeoutValues)('rejects invalid ask --timeout-ms: %s', async (_label, value) => { - const flags = new Map([ - ['to', 'term_coord'], - ['question', 'Proceed?'], - ['timeout-ms', value] - ]) - - await expect(invokeAsk(flags)).rejects.toThrow(/--timeout-ms/) - expect(callMock).not.toHaveBeenCalled() - }) - - it('uses the parsed ask timeout for both runtime wait and client timeout', async () => { - process.env.ORCA_TERMINAL_HANDLE = 'term_worker' - callMock.mockResolvedValue({ - result: { - answer: 'yes', - messageId: 'msg_1', - threadId: 'thread_1', - timedOut: false - } - }) - vi.spyOn(console, 'log').mockImplementation(() => {}) - - await invokeAsk( - new Map([ - ['to', 'term_coord'], - ['question', 'Proceed?'], - ['timeout-ms', '123'] - ]) - ) - - expect(callMock).toHaveBeenCalledWith( - 'orchestration.ask', - { - to: 'term_coord', - question: 'Proceed?', - options: undefined, - timeoutMs: 123, - from: 'term_worker' - }, - { timeoutMs: 5_123 } - ) - }) -}) - describe('orchestration task-list brief output', () => { it('requests server-side brief and falls back client-side for older runtimes', async () => { callMock.mockReset().mockResolvedValue({ diff --git a/src/cli/handlers/orchestration.ts b/src/cli/handlers/orchestration.ts index 1f8e2b609..ec02ded26 100644 --- a/src/cli/handlers/orchestration.ts +++ b/src/cli/handlers/orchestration.ts @@ -8,7 +8,12 @@ import { } from '../flags' import { RuntimeClientError } from '../runtime-client' import { getTerminalHandle } from '../selectors' +import { + clampOrchestrationAskTimeoutMs, + resolveOrchestrationAskClientTimeoutMs +} from '../../shared/orchestration-ask-timeout' import { abbreviateOrchestrationTasks } from '../../shared/orchestration-task-summary' +import { parsePositiveSafeIntegerText } from '../../shared/timer-delay' // Why: 15 s is well under Claude Code's ~2 min Bash-tool silence budget while keeping log volume low. See design doc ยง3.4. const DEFAULT_KEEPALIVE_INTERVAL_MS = 15_000 @@ -319,11 +324,11 @@ function getOptionalPositiveIntegerValueFlag( if (typeof raw !== 'string' || raw.length === 0) { throw new RuntimeClientError('invalid_argument', `Missing value for --${name}.`) } - const value = Number(raw) - if (!Number.isFinite(value) || !Number.isInteger(value) || value <= 0) { + const value = parsePositiveSafeIntegerText(raw) + if (value === null) { throw new RuntimeClientError( 'invalid_argument', - `Invalid positive integer for --${name}: ${raw}` + `Invalid positive safe integer for --${name}: ${raw}` ) } return value @@ -611,8 +616,8 @@ export const ORCHESTRATION_HANDLERS: Record = { 'orchestration ask': async ({ flags, client, cwd, json }) => { const parsedTimeoutMs = getOptionalPositiveIntegerValueFlag(flags, 'timeout-ms') + const timeoutMs = clampOrchestrationAskTimeoutMs(parsedTimeoutMs) const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from') - const timeoutMs = parsedTimeoutMs ?? 600_000 const result = await client.call<{ answer: string | null messageId: string | null @@ -625,11 +630,11 @@ export const ORCHESTRATION_HANDLERS: Record = { to: getRequiredStringFlag(flags, 'to'), question: getRequiredStringFlag(flags, 'question'), options: getOptionalStringFlag(flags, 'options'), - timeoutMs: parsedTimeoutMs, + timeoutMs: parsedTimeoutMs === undefined ? undefined : timeoutMs, from }, // Why: extend past timeoutMs so the RPC transport's 60s default doesn't abort before the runtime's own timeout resolves. - { timeoutMs: timeoutMs + 5_000 } + { timeoutMs: resolveOrchestrationAskClientTimeoutMs(parsedTimeoutMs) } ) // Why: bypass printResult so --json emits a bare JSON object (no envelope) pipeable via `jq -r .answer`, unlike other verbs. if (json) { diff --git a/src/cli/runtime/transport.test.ts b/src/cli/runtime/transport.test.ts index d722de220..8de35f619 100644 --- a/src/cli/runtime/transport.test.ts +++ b/src/cli/runtime/transport.test.ts @@ -4,6 +4,7 @@ import { join } from 'node:path' import { createServer, type Socket } from 'node:net' import { afterEach, describe, expect, it } from 'vitest' import type { RuntimeMetadata } from '../../shared/runtime-bootstrap' +import { MAX_TIMER_DELAY_MS } from '../../shared/timer-delay' import { sendRequest } from './transport' const servers = new Set>() @@ -25,6 +26,27 @@ afterEach(async () => { servers.clear() }) +describe('runtime transport timeout validation', () => { + it.each([-1, 1.5, MAX_TIMER_DELAY_MS + 1, Number.MAX_SAFE_INTEGER + 1])( + 'rejects invalid timer delay %s before transport discovery', + async (timeoutMs) => { + const metadata: RuntimeMetadata = { + runtimeId: 'runtime-1', + pid: 123, + transports: [], + authToken: 'token', + startedAt: 1 + } + + await expect(sendRequest(metadata, 'status.get', undefined, timeoutMs)).rejects.toMatchObject( + { + code: 'invalid_argument' + } + ) + } + ) +}) + // Why: these tests create Unix domain socket servers in temp directories. // Windows does not support Unix domain sockets in the same way. describe.skipIf(process.platform === 'win32')('runtime transport', () => { diff --git a/src/cli/runtime/transport.ts b/src/cli/runtime/transport.ts index 60da1e2a9..18385cb1a 100644 --- a/src/cli/runtime/transport.ts +++ b/src/cli/runtime/transport.ts @@ -3,6 +3,7 @@ import { randomUUID } from 'node:crypto' import { findTransport, type RuntimeMetadata } from '../../shared/runtime-bootstrap' import { isKeepaliveFrame, RuntimeRpcEnvelopeSchema } from './envelope-schema' import { RuntimeClientError, type RuntimeRpcResponse } from './types' +import { MAX_TIMER_DELAY_MS, isSafeTimerDelayMs } from '../../shared/timer-delay' export async function sendRequest( metadata: RuntimeMetadata, @@ -10,6 +11,12 @@ export async function sendRequest( params: unknown, timeoutMs: number ): Promise> { + if (!isSafeTimerDelayMs(timeoutMs)) { + throw new RuntimeClientError( + 'invalid_argument', + `Runtime request timeout must be an integer between 0 and ${MAX_TIMER_DELAY_MS}ms.` + ) + } return await new Promise((resolve, reject) => { const transport = findTransport(metadata, 'unix', 'named-pipe') if (!transport) { diff --git a/src/main/runtime/rpc/methods/orchestration.test.ts b/src/main/runtime/rpc/methods/orchestration.test.ts index 3078fd952..780cb2efd 100644 --- a/src/main/runtime/rpc/methods/orchestration.test.ts +++ b/src/main/runtime/rpc/methods/orchestration.test.ts @@ -1,11 +1,12 @@ /* eslint-disable max-lines -- Why: orchestration tests share a mock runtime factory; splitting by method would duplicate 40 lines of setup per file without improving clarity. */ import { afterEach, describe, expect, it, vi } from 'vitest' -import { ORCHESTRATION_METHODS, clampAskTimeoutMs } from './orchestration' +import { ORCHESTRATION_METHODS } from './orchestration' import { RpcDispatcher } from '../dispatcher' import { buildRegistry, type RpcContext, type RpcRequest } from '../core' import { OrchestrationDb } from '../../orchestration/db' import { OrcaRuntimeService } from '../../orca-runtime' import type { RuntimeTerminalSummary } from '../../../../shared/runtime-types' +import { ORCHESTRATION_ASK_MAX_TIMEOUT_MS } from '../../../../shared/orchestration-ask-timeout' function lifecycleGroupRecipientError(type: 'worker_done' | 'heartbeat'): string { return `${type} messages must be sent to a concrete coordinator terminal handle, not a group address.` @@ -1792,16 +1793,18 @@ describe('orchestration RPC methods', () => { expect(result.answer).toBe('correct answer') }) - it('clamps an absurd caller-supplied timeoutMs so the long-poll slot is bounded', async () => { + it.each<[number | undefined, number]>([ + [undefined, 600_000], + [ORCHESTRATION_ASK_MAX_TIMEOUT_MS, ORCHESTRATION_ASK_MAX_TIMEOUT_MS], + [Number.MAX_SAFE_INTEGER, ORCHESTRATION_ASK_MAX_TIMEOUT_MS] + ])('applies effective timeout %s at the RPC handler boundary', async (requested, expected) => { setup() vi.spyOn(runtime, 'deliverPendingMessagesForHandle').mockImplementation(() => {}) vi.spyOn(runtime, 'notifyMessageArrived').mockImplementation(() => {}) let observedTimeoutMs: number | undefined vi.spyOn(runtime, 'waitForMessage').mockImplementation(async (_handle, options) => { observedTimeoutMs = options?.timeoutMs - // End the wait loop so the assertion runs against the first budget slice. const outbound = db.getInbox(10).find((m) => m.type === 'decision_gate') - // Why: without a reply the handler's while(true) spins on this mock until vitest times out, hanging instead of failing. expect(outbound).toBeDefined() db.insertMessage({ from: 'term_coord', @@ -1815,23 +1818,28 @@ describe('orchestration RPC methods', () => { const result = (await call('orchestration.ask', { from: 'term_worker', to: 'term_coord', - question: 'forever?', - timeoutMs: Number.MAX_SAFE_INTEGER + question: 'bounded?', + timeoutMs: requested })) as { timeoutMs: number } - expect(observedTimeoutMs).toBeLessThanOrEqual(1_800_000) - expect(observedTimeoutMs).toBeGreaterThan(1_700_000) - // The clamp must be observable: callers report the budget waited, not the one they asked for. - expect(result.timeoutMs).toBe(1_800_000) + expect(observedTimeoutMs).toBeLessThanOrEqual(expected) + expect(observedTimeoutMs).toBeGreaterThan(expected - 1_000) + expect(result.timeoutMs).toBe(expected) }) - it('clamps timeoutMs at the exported boundary', () => { - expect(clampAskTimeoutMs(undefined)).toBe(600_000) - expect(clampAskTimeoutMs(1_000)).toBe(1_000) - expect(clampAskTimeoutMs(1_800_000)).toBe(1_800_000) - expect(clampAskTimeoutMs(86_400_000)).toBe(1_800_000) - expect(clampAskTimeoutMs(Number.MAX_SAFE_INTEGER)).toBe(1_800_000) - expect(clampAskTimeoutMs(-5)).toBe(0) + it('returns a zero effective timeout without entering the waiter', async () => { + setup() + const waitForMessage = vi.spyOn(runtime, 'waitForMessage') + + const result = (await call('orchestration.ask', { + from: 'term_worker', + to: 'term_coord', + question: 'negative?', + timeoutMs: -5 + })) as { timedOut: boolean; timeoutMs: number } + + expect(waitForMessage).not.toHaveBeenCalled() + expect(result).toMatchObject({ timedOut: true, timeoutMs: 0 }) }) it('parses options CSV with whitespace and empty entries', async () => { diff --git a/src/main/runtime/rpc/methods/orchestration.ts b/src/main/runtime/rpc/methods/orchestration.ts index 11a0e67e7..ddf86825e 100644 --- a/src/main/runtime/rpc/methods/orchestration.ts +++ b/src/main/runtime/rpc/methods/orchestration.ts @@ -8,6 +8,7 @@ import { formatMessageBanner } from '../../orchestration/formatter' import { isGroupAddress, resolveGroupAddress } from '../../orchestration/groups' import { reconcileLifecycleMessage } from '../../orchestration/lifecycle-reconciliation' import { abbreviateOrchestrationTasks } from '../../../../shared/orchestration-task-summary' +import { clampOrchestrationAskTimeoutMs } from '../../../../shared/orchestration-ask-timeout' import { ORCHESTRATION_GATE_METHODS } from './orchestration-gates' const MESSAGE_TYPES: MessageType[] = [ @@ -30,19 +31,6 @@ const TASK_STATUSES: TaskStatus[] = [ 'blocked' ] -const ASK_DEFAULT_TIMEOUT_MS = 600_000 - -// Why: ask pins a shared long-poll slot for its entire timeout, so a caller-supplied -// value can't be unbounded; 30 min covers a slow human gate and still frees the slot. -const ASK_MAX_TIMEOUT_MS = 1_800_000 - -export function clampAskTimeoutMs(timeoutMs: number | undefined): number { - if (timeoutMs === undefined) { - return ASK_DEFAULT_TIMEOUT_MS - } - return Math.min(Math.max(0, timeoutMs), ASK_MAX_TIMEOUT_MS) -} - function getLifecycleGroupRecipientError(type: 'worker_done' | 'heartbeat'): string { return `${type} messages must be sent to a concrete coordinator terminal handle, not a group address.` } @@ -581,7 +569,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [ const db = runtime.getOrchestrationDb() const from = params.from ?? 'unknown' // Why: echoed on every return so a clamped caller reports the budget actually waited, not the one it asked for. - const timeoutMs = clampAskTimeoutMs(params.timeoutMs) + const timeoutMs = clampOrchestrationAskTimeoutMs(params.timeoutMs) const options = params.options ?.split(',') diff --git a/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts b/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts index 22920a666..a65ba6f91 100644 --- a/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts +++ b/src/main/ssh/ssh-remote-cli-host-passthrough.test.ts @@ -19,6 +19,9 @@ import { resolveHostCliKillTimeoutMs, runHostOrcaCliPassthrough } from './ssh-remote-cli-host-passthrough' +import { resolveOrchestrationAskClientTimeoutMs } from '../../shared/orchestration-ask-timeout' +import { remoteCliRequestTimeoutMs } from '../../relay/remote-cli-timeout' +import { MAX_TIMER_DELAY_MS } from '../../shared/timer-delay' type FakeChild = EventEmitter & { stdout: EventEmitter @@ -96,6 +99,74 @@ describe('resolveHostCliKillTimeoutMs', () => { ) expect(resolveHostCliKillTimeoutMs(['worktree', 'list'])).toBe(600_000) }) + + it.each([ + [[], 720_000], + [['--timeout-ms', String(Number.MAX_SAFE_INTEGER)], 1_920_000], + [['--timeout-ms', String(Number.MAX_SAFE_INTEGER + 1)], 720_000], + [['--timeout-ms', '9007199254740991.1'], 720_000], + [['--timeout-ms', '1', '--timeout-ms=1800000'], 1_920_000], + [['--timeout-ms=1800000', '--timeout-ms', '1'], 600_000], + [['--timeout-ms', '1800000', '--timeout-ms'], 720_000], + [['--timeout-ms=1800000', '--timeout-ms='], 720_000], + [['--timeout-ms=1800000', '--timeout-ms', 'bad'], 720_000], + [['--timeout-ms', 'bad', '--timeout-ms=1800000'], 1_920_000], + [['--timeout-ms=bad', '--timeout-ms', '1800000'], 1_920_000] + ])('bounds ask child timers with last-wins flags %#', (timeoutArgs, expected) => { + expect(resolveHostCliKillTimeoutMs(['orchestration', '--json', 'ask', ...timeoutArgs])).toBe( + expected + ) + }) + + it('does not apply the ask maximum to other commands', () => { + expect(resolveHostCliKillTimeoutMs(['terminal', 'wait', '--timeout-ms', '1800001'])).toBe( + 1_920_001 + ) + }) + + it.each(['+1000000', '1000000.0', '1e6'])( + 'extends non-ask child timers using CLI-compatible integer syntax %s', + (raw) => { + expect(resolveHostCliKillTimeoutMs(['terminal', 'wait', '--timeout-ms', raw])).toBe(1_120_000) + } + ) + + it.each([ + 'Infinity', + '1.5', + '-1', + 'bad', + String(Number.MAX_SAFE_INTEGER), + String(MAX_TIMER_DELAY_MS - 120_000 + 1) + ])('falls back to the default kill timer when a non-ask --timeout-ms %s is unusable', (raw) => { + expect(resolveHostCliKillTimeoutMs(['terminal', 'wait', '--timeout-ms', raw])).toBe(600_000) + }) + + it('keeps the largest non-ask kill timer that stays inside the timer range', () => { + expect( + resolveHostCliKillTimeoutMs([ + 'terminal', + 'wait', + '--timeout-ms', + String(MAX_TIMER_DELAY_MS - 120_000) + ]) + ).toBe(MAX_TIMER_DELAY_MS) + }) + + it.each<[string[], number | undefined]>([ + [[], undefined], + [['--timeout-ms', '1'], 1], + [['--timeout-ms', String(Number.MAX_SAFE_INTEGER)], Number.MAX_SAFE_INTEGER], + [['--timeout-ms', String(Number.MAX_SAFE_INTEGER + 1)], undefined] + ])('keeps inner, host, and relay ask deadlines ordered %#', (timeoutArgs, parsedTimeout) => { + const argv = ['orchestration', 'ask', '--to', 'term_x', ...timeoutArgs] + const innerTimeout = resolveOrchestrationAskClientTimeoutMs(parsedTimeout) + const hostTimeout = resolveHostCliKillTimeoutMs(argv) + const relayTimeout = remoteCliRequestTimeoutMs({ argv }) + + expect(innerTimeout).toBeLessThan(hostTimeout) + expect(hostTimeout).toBeLessThan(relayTimeout!) + }) }) describe('runHostOrcaCliPassthrough', () => { @@ -191,6 +262,17 @@ describe('runHostOrcaCliPassthrough', () => { expect(spawn).not.toHaveBeenCalled() }) + it('rejects an invalid injected kill timeout before spawning', async () => { + const spawn = vi.fn() + await expect( + runHostOrcaCliPassthrough( + { argv: ['status'], cwd: '/', env: {} }, + { ...BASE_OPTIONS, spawn: spawn as never, killTimeoutMs: 2_147_483_648 } + ) + ).rejects.toBeInstanceOf(RangeError) + expect(spawn).not.toHaveBeenCalled() + }) + it('throws HostCliUnavailableError when the subprocess fails to launch', async () => { const child = createFakeChild() const spawn = vi.fn(() => child) diff --git a/src/main/ssh/ssh-remote-cli-host-passthrough.ts b/src/main/ssh/ssh-remote-cli-host-passthrough.ts index 3ca55edfd..09db31d47 100644 --- a/src/main/ssh/ssh-remote-cli-host-passthrough.ts +++ b/src/main/ssh/ssh-remote-cli-host-passthrough.ts @@ -9,6 +9,14 @@ import { spawn as nodeSpawn } from 'node:child_process' import { existsSync } from 'node:fs' import { join } from 'node:path' import { getCanonicalUserDataPath } from '../persistence' +import { parseRemoteCliArgs } from './ssh-remote-cli-args' +import { clampOrchestrationAskTimeoutMs } from '../../shared/orchestration-ask-timeout' +import { + MAX_TIMER_DELAY_MS, + isSafeTimerDelayMs, + parsePositiveSafeIntegerNumericText, + parsePositiveSafeIntegerText +} from '../../shared/timer-delay' export type RemoteOrcaCliRequest = { argv: string[] @@ -72,9 +80,23 @@ export function resolveHostCliEntryPath(app: { * budget in `--timeout-ms`; extend past it so the CLI's own timeout fires * first and produces a proper error message. */ export function resolveHostCliKillTimeoutMs(argv: string[]): number { - const explicit = parseTimeoutMsFlag(argv) - if (explicit !== null && Number.isFinite(explicit) && explicit > 0) { - return Math.max(DEFAULT_KILL_TIMEOUT_MS, explicit + KILL_TIMEOUT_GRACE_MS) + const parsed = parseRemoteCliArgs(argv) + const rawTimeout = parsed.flags.get('timeout-ms') + if (parsed.commandPath[0] === 'orchestration' && parsed.commandPath[1] === 'ask') { + const explicit = + typeof rawTimeout === 'string' ? parsePositiveSafeIntegerText(rawTimeout) : null + return Math.max( + DEFAULT_KILL_TIMEOUT_MS, + clampOrchestrationAskTimeoutMs(explicit ?? undefined) + KILL_TIMEOUT_GRACE_MS + ) + } + const explicit = + typeof rawTimeout === 'string' ? parsePositiveSafeIntegerNumericText(rawTimeout) : null + // Why: this feeds the kill timer directly, so a post-grace budget outside the + // timer range degrades to the default instead of throwing at spawn time. + const extended = explicit === null ? null : explicit + KILL_TIMEOUT_GRACE_MS + if (extended !== null && isSafeTimerDelayMs(extended)) { + return Math.max(DEFAULT_KILL_TIMEOUT_MS, extended) } return DEFAULT_KILL_TIMEOUT_MS } @@ -141,6 +163,11 @@ export async function runHostOrcaCliPassthrough( const spawn = options.spawn ?? nodeSpawn const entryExists = options.entryExists ?? existsSync const killTimeoutMs = options.killTimeoutMs ?? resolveHostCliKillTimeoutMs(request.argv) + if (!isSafeTimerDelayMs(killTimeoutMs)) { + throw new RangeError( + `Host CLI kill timeout must be an integer between 0 and ${MAX_TIMER_DELAY_MS}ms.` + ) + } if (!entryExists(cliEntryPath)) { throw new HostCliUnavailableError(`Orca CLI entry not found at ${cliEntryPath}`) @@ -252,19 +279,3 @@ class CappedOutputCollector { return this.truncated ? `${text}\n[orca ssh cli] output truncated\n` : text } } - -function parseTimeoutMsFlag(argv: string[]): number | null { - for (let i = 0; i < argv.length; i += 1) { - const token = argv[i] - if (token === '--timeout-ms') { - const next = argv[i + 1] - const parsed = next === undefined ? Number.NaN : Number(next) - return Number.isFinite(parsed) ? parsed : null - } - if (token.startsWith('--timeout-ms=')) { - const parsed = Number(token.slice('--timeout-ms='.length)) - return Number.isFinite(parsed) ? parsed : null - } - } - return null -} diff --git a/src/relay/dispatcher-timeout.test.ts b/src/relay/dispatcher-timeout.test.ts new file mode 100644 index 000000000..25ade25a5 --- /dev/null +++ b/src/relay/dispatcher-timeout.test.ts @@ -0,0 +1,31 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { MAX_TIMER_DELAY_MS } from '../shared/timer-delay' +import { RelayDispatcher } from './dispatcher' + +describe('RelayDispatcher request timeout validation', () => { + let dispatcher: RelayDispatcher + let writes: Buffer[] + + beforeEach(() => { + vi.useFakeTimers() + writes = [] + dispatcher = new RelayDispatcher((data) => { + writes.push(Buffer.from(data)) + }) + }) + + afterEach(() => { + dispatcher.dispose() + vi.useRealTimers() + }) + + it.each([-1, 1.5, MAX_TIMER_DELAY_MS + 1, Number.MAX_SAFE_INTEGER + 1])( + 'rejects invalid timer delay %s without sending a frame', + async (timeoutMs) => { + await expect( + dispatcher.requestPrimary('status.get', undefined, { timeoutMs }) + ).rejects.toThrow(/Request timeout/) + expect(writes).toHaveLength(0) + } + ) +}) diff --git a/src/relay/dispatcher.ts b/src/relay/dispatcher.ts index 1f1445da2..912b66691 100644 --- a/src/relay/dispatcher.ts +++ b/src/relay/dispatcher.ts @@ -12,6 +12,7 @@ import { type JsonRpcResponse } from './protocol' import { ClientRequestAborts } from './client-request-aborts' +import { MAX_TIMER_DELAY_MS, isSafeTimerDelayMs } from '../shared/timer-delay' export type RequestContext = { clientId: number @@ -265,7 +266,7 @@ export class RelayDispatcher { method: string, params?: Record, options?: { timeoutMs?: number } - ): Promise { + ) { return this.requestClient(this.primaryClient.id, method, params, options) } @@ -295,6 +296,12 @@ export class RelayDispatcher { if (this.disposed || !client || client.closed) { return Promise.reject(new Error('Relay client is not connected')) } + const timeoutMs = options?.timeoutMs ?? RELAY_TO_CLIENT_REQUEST_TIMEOUT_MS + if (!isSafeTimerDelayMs(timeoutMs)) { + return Promise.reject( + new Error(`Request timeout must be an integer between 0 and ${MAX_TIMER_DELAY_MS}ms`) + ) + } const id = this.nextRequestId++ const msg: JsonRpcRequest = { jsonrpc: '2.0', @@ -302,7 +309,6 @@ export class RelayDispatcher { method, ...(params !== undefined ? { params } : {}) } - const timeoutMs = options?.timeoutMs ?? RELAY_TO_CLIENT_REQUEST_TIMEOUT_MS return new Promise((resolve, reject) => { const timer = setTimeout(() => { this.pendingRelayRequests.delete(id) diff --git a/src/relay/remote-cli-timeout.test.ts b/src/relay/remote-cli-timeout.test.ts index c09a0ccfe..d7489b4a3 100644 --- a/src/relay/remote-cli-timeout.test.ts +++ b/src/relay/remote-cli-timeout.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import { remoteCliRequestTimeoutMs } from './remote-cli-timeout' +import { MAX_TIMER_DELAY_MS } from '../shared/timer-delay' describe('remoteCliRequestTimeoutMs', () => { it('gives Linear issue context reads the general CLI budget', () => { @@ -28,7 +29,7 @@ describe('remoteCliRequestTimeoutMs', () => { remoteCliRequestTimeoutMs({ argv: ['orchestration', 'ask', '--to', 'term_x', '--question', 'ok?'] }) - ).toBe(600_000) + ).toBe(780_000) }) it('extends past an explicit --timeout-ms waiter budget', () => { @@ -52,6 +53,66 @@ describe('remoteCliRequestTimeoutMs', () => { ).toBe(600_000) }) + it.each([ + [['--timeout-ms', String(Number.MAX_SAFE_INTEGER)], 1_980_000], + [['--timeout-ms', String(Number.MAX_SAFE_INTEGER + 1)], 780_000], + [['--timeout-ms', '9007199254740991.1'], 780_000], + [['--timeout-ms', '1', '--timeout-ms=1800000'], 1_980_000], + [['--timeout-ms=1800000', '--timeout-ms', '1'], 660_000], + [['--timeout-ms', '1800000', '--timeout-ms'], 780_000], + [['--timeout-ms=1800000', '--timeout-ms='], 780_000], + [['--timeout-ms=1800000', '--timeout-ms', 'bad'], 780_000], + [['--timeout-ms', 'bad', '--timeout-ms=1800000'], 1_980_000], + [['--timeout-ms=bad', '--timeout-ms', '1800000'], 1_980_000] + ])('bounds ask outer timers with last-wins flags %#', (timeoutArgs, expected) => { + expect( + remoteCliRequestTimeoutMs({ + argv: ['orchestration', 'ask', '--to', 'term_x', ...timeoutArgs] + }) + ).toBe(expected) + }) + + it('does not apply the ask maximum to other wait commands', () => { + expect( + remoteCliRequestTimeoutMs({ + argv: ['terminal', 'wait', '--timeout-ms', '1800001'] + }) + ).toBe(1_860_001) + }) + + it.each(['+1000000', '1000000.0', '1e6'])( + 'extends non-ask waits using CLI-compatible integer syntax %s', + (raw) => { + expect( + remoteCliRequestTimeoutMs({ + argv: ['terminal', 'wait', '--timeout-ms', raw] + }) + ).toBe(1_060_000) + } + ) + + it.each([ + ['Infinity'], + ['1.5'], + ['-1'], + ['bad'], + [String(Number.MAX_SAFE_INTEGER)], + [String(MAX_TIMER_DELAY_MS - 60_000 + 1)] + ])('falls back to the base budget when a non-ask --timeout-ms %s is unusable', (raw) => { + expect(remoteCliRequestTimeoutMs({ argv: ['terminal', 'wait', '--timeout-ms', raw] })).toBe( + 600_000 + ) + expect(remoteCliRequestTimeoutMs({ argv: ['status', '--timeout-ms', raw] })).toBe(300_000) + }) + + it('keeps the largest non-ask budget that stays inside the timer range', () => { + expect( + remoteCliRequestTimeoutMs({ + argv: ['terminal', 'wait', '--timeout-ms', String(MAX_TIMER_DELAY_MS - 60_000)] + }) + ).toBe(MAX_TIMER_DELAY_MS) + }) + it('does not treat a flag value named wait as a command path element', () => { expect(remoteCliRequestTimeoutMs({ argv: ['terminal', 'read', '--terminal', 'wait'] })).toBe( 300_000 diff --git a/src/relay/remote-cli-timeout.ts b/src/relay/remote-cli-timeout.ts index b28ef0ee3..f03f84528 100644 --- a/src/relay/remote-cli-timeout.ts +++ b/src/relay/remote-cli-timeout.ts @@ -1,3 +1,10 @@ +import { clampOrchestrationAskTimeoutMs } from '../shared/orchestration-ask-timeout' +import { + isSafeTimerDelayMs, + parsePositiveSafeIntegerNumericText, + parsePositiveSafeIntegerText +} from '../shared/timer-delay' + // Why: the host bridges the full Orca CLI over the relay (#7716), so mutation // commands (worktree create, orchestration dispatch, Linear writes, ...) can // legitimately outlive the relay's 30 s default request timeout. Long-poll @@ -7,6 +14,8 @@ const REMOTE_CLI_DEFAULT_TIMEOUT_MS = 5 * 60_000 const REMOTE_CLI_WAIT_TIMEOUT_MS = 10 * 60_000 const REMOTE_CLI_TIMEOUT_GRACE_MS = 60_000 +const ORCHESTRATION_ASK_RELAY_GRACE_MS = 3 * 60_000 +const ORCHESTRATION_ASK_RELAY_BASE_MS = 11 * 60_000 const REMOTE_TIMEOUT_BOOLEAN_FLAGS = new Set([ 'all', @@ -28,42 +37,50 @@ export function remoteCliRequestTimeoutMs(params: Record): numb if (!argv) { return undefined } - const base = isWaitStyleCliRequest(argv) + const commandPath = parseRemoteCommandPath(argv) + const timeoutFlag = findLastTimeoutMsFlag(argv) + if (commandPath[0] === 'orchestration' && commandPath[1] === 'ask') { + const parsed = + timeoutFlag?.raw === undefined ? null : parsePositiveSafeIntegerText(timeoutFlag.raw) + const effective = clampOrchestrationAskTimeoutMs(parsed ?? undefined) + return Math.max(ORCHESTRATION_ASK_RELAY_BASE_MS, effective + ORCHESTRATION_ASK_RELAY_GRACE_MS) + } + const base = isWaitStyleCliRequest(argv, commandPath) ? REMOTE_CLI_WAIT_TIMEOUT_MS : REMOTE_CLI_DEFAULT_TIMEOUT_MS - const explicit = parseTimeoutMsFlag(argv) - if (explicit !== null && explicit > 0) { - return Math.max(base, explicit + REMOTE_CLI_TIMEOUT_GRACE_MS) + const explicit = + timeoutFlag?.raw === undefined ? null : parsePositiveSafeIntegerNumericText(timeoutFlag.raw) + // Why: the relay forwards this straight into a timer, so a budget that would + // overflow the timer range after grace has to degrade to the base budget. + const extended = explicit === null ? null : explicit + REMOTE_CLI_TIMEOUT_GRACE_MS + if (extended !== null && isSafeTimerDelayMs(extended)) { + return Math.max(base, extended) } return base } -function isWaitStyleCliRequest(argv: string[]): boolean { +function isWaitStyleCliRequest(argv: string[], commandPath: string[]): boolean { if (argv.includes('--wait')) { return true } - const commandPath = parseRemoteCommandPath(argv) return ( (commandPath[0] === 'terminal' && commandPath[1] === 'wait') || (commandPath[0] === 'orchestration' && commandPath[1] === 'ask') ) } -function parseTimeoutMsFlag(argv: string[]): number | null { +function findLastTimeoutMsFlag(argv: string[]): { raw: string | undefined } | null { + let result: { raw: string | undefined } | null = null for (let index = 0; index < argv.length; index += 1) { const token = argv[index] - let raw: string | undefined if (token === '--timeout-ms') { - raw = argv[index + 1] + const next = argv[index + 1] + result = { raw: next?.startsWith('--') ? undefined : next } } else if (token.startsWith('--timeout-ms=')) { - raw = token.slice('--timeout-ms='.length) - } else { - continue + result = { raw: token.slice('--timeout-ms='.length) } } - const parsed = raw === undefined ? Number.NaN : Number(raw) - return Number.isFinite(parsed) ? parsed : null } - return null + return result } function getStringArgv(params: Record): string[] | null { diff --git a/src/shared/orchestration-ask-timeout.test.ts b/src/shared/orchestration-ask-timeout.test.ts new file mode 100644 index 000000000..ee1b58ba6 --- /dev/null +++ b/src/shared/orchestration-ask-timeout.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest' +import { + clampOrchestrationAskTimeoutMs, + ORCHESTRATION_ASK_DEFAULT_TIMEOUT_MS, + ORCHESTRATION_ASK_MAX_TIMEOUT_MS, + resolveOrchestrationAskClientTimeoutMs +} from './orchestration-ask-timeout' +import { MAX_TIMER_DELAY_MS } from './timer-delay' + +describe('orchestration ask timeout policy', () => { + it('defaults and clamps to the shared server maximum', () => { + expect(clampOrchestrationAskTimeoutMs(undefined)).toBe(ORCHESTRATION_ASK_DEFAULT_TIMEOUT_MS) + expect(clampOrchestrationAskTimeoutMs(1_000)).toBe(1_000) + expect(clampOrchestrationAskTimeoutMs(ORCHESTRATION_ASK_MAX_TIMEOUT_MS)).toBe( + ORCHESTRATION_ASK_MAX_TIMEOUT_MS + ) + expect(clampOrchestrationAskTimeoutMs(Number.MAX_SAFE_INTEGER)).toBe( + ORCHESTRATION_ASK_MAX_TIMEOUT_MS + ) + expect(clampOrchestrationAskTimeoutMs(-5)).toBe(0) + }) + + it('leaves room for the longest outer ask transport grace', () => { + expect(resolveOrchestrationAskClientTimeoutMs(ORCHESTRATION_ASK_MAX_TIMEOUT_MS)).toBe(1_805_000) + expect(ORCHESTRATION_ASK_MAX_TIMEOUT_MS + 3 * 60_000).toBeLessThan(MAX_TIMER_DELAY_MS) + }) +}) diff --git a/src/shared/orchestration-ask-timeout.ts b/src/shared/orchestration-ask-timeout.ts new file mode 100644 index 000000000..7cbbf0813 --- /dev/null +++ b/src/shared/orchestration-ask-timeout.ts @@ -0,0 +1,14 @@ +export const ORCHESTRATION_ASK_DEFAULT_TIMEOUT_MS = 600_000 +export const ORCHESTRATION_ASK_MAX_TIMEOUT_MS = 1_800_000 +export const ORCHESTRATION_ASK_CLIENT_GRACE_MS = 5_000 + +export function clampOrchestrationAskTimeoutMs(timeoutMs: number | undefined): number { + if (timeoutMs === undefined) { + return ORCHESTRATION_ASK_DEFAULT_TIMEOUT_MS + } + return Math.min(Math.max(0, timeoutMs), ORCHESTRATION_ASK_MAX_TIMEOUT_MS) +} + +export function resolveOrchestrationAskClientTimeoutMs(timeoutMs: number | undefined): number { + return clampOrchestrationAskTimeoutMs(timeoutMs) + ORCHESTRATION_ASK_CLIENT_GRACE_MS +} diff --git a/src/shared/remote-runtime-client.test.ts b/src/shared/remote-runtime-client.test.ts index 5febb2f2d..4c09c1e90 100644 --- a/src/shared/remote-runtime-client.test.ts +++ b/src/shared/remote-runtime-client.test.ts @@ -12,6 +12,7 @@ import { publicKeyToBase64 } from './e2ee-crypto' import { sendRemoteRuntimeRequest, subscribeRemoteRuntimeRequest } from './remote-runtime-client' +import { MAX_TIMER_DELAY_MS } from './timer-delay' const servers: WebSocketServer[] = [] @@ -173,6 +174,15 @@ describe('subscribeRemoteRuntimeRequest', () => { }) describe('sendRemoteRuntimeRequest', () => { + it.each([-1, 1.5, MAX_TIMER_DELAY_MS + 1, Number.MAX_SAFE_INTEGER + 1])( + 'rejects invalid timer delay %s before reading pairing data', + async (timeoutMs) => { + await expect( + sendRemoteRuntimeRequest({} as PairingOffer, 'status.get', {}, timeoutMs) + ).rejects.toMatchObject({ code: 'invalid_argument' }) + } + ) + it('includes WebSocket close details when one-shot admission is rejected', async () => { const server = await createClosingServer(1013, 'Maximum connections reached') diff --git a/src/shared/remote-runtime-client.ts b/src/shared/remote-runtime-client.ts index b82f515a0..da1fff5bb 100644 --- a/src/shared/remote-runtime-client.ts +++ b/src/shared/remote-runtime-client.ts @@ -42,6 +42,7 @@ import { type RemoteRuntimeSocketLivenessOptions } from './remote-runtime-socket-liveness' import { createWsOutboundBackpressureQueue } from './ws-outbound-backpressure-queue' +import { MAX_TIMER_DELAY_MS, isSafeTimerDelayMs } from './timer-delay' export { RemoteRuntimeClientError } from './remote-runtime-client-error' @@ -82,6 +83,12 @@ export async function sendRemoteRuntimeRequest( params: unknown, timeoutMs: number ): Promise> { + if (!isSafeTimerDelayMs(timeoutMs)) { + throw new RemoteRuntimeClientError( + 'invalid_argument', + `Runtime request timeout must be an integer between 0 and ${MAX_TIMER_DELAY_MS}ms.` + ) + } const requestId = randomUUID() const serializedAuth = serializeRemoteRuntimePayload({ type: 'e2ee_auth', @@ -140,8 +147,7 @@ export async function sendRemoteRuntimeRequest( refreshableTimeout.refresh() return } - // Why: mobile typechecks shared code with DOM timer types, where - // setTimeout returns a number and Node's Timeout.refresh is absent. + // Mobile's DOM timer type has no refresh(). clearTimeout(timeout) timeout = setTimeout(onTimeout, timeoutMs) } diff --git a/src/shared/timer-delay.test.ts b/src/shared/timer-delay.test.ts new file mode 100644 index 000000000..23171a60e --- /dev/null +++ b/src/shared/timer-delay.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' +import { + isSafeTimerDelayMs, + MAX_TIMER_DELAY_MS, + parsePositiveSafeIntegerNumericText, + parsePositiveSafeIntegerText +} from './timer-delay' + +describe('timer delay policy', () => { + it.each([0, 1, MAX_TIMER_DELAY_MS])('accepts timer delay %s', (value) => { + expect(isSafeTimerDelayMs(value)).toBe(true) + }) + + it.each([-1, 1.5, MAX_TIMER_DELAY_MS + 1, Number.MAX_SAFE_INTEGER + 1])( + 'rejects timer delay %s', + (value) => { + expect(isSafeTimerDelayMs(value)).toBe(false) + } + ) + + it.each([ + ['1', 1], + ['00123', 123], + ['+1000', 1_000], + ['1000.0', 1_000], + ['1e3', 1_000], + ['.1e4', 1_000], + ['0x3e8', 1_000], + ['0b1000', 8], + ['0o10', 8], + [String(Number.MAX_SAFE_INTEGER), Number.MAX_SAFE_INTEGER] + ])('parses exact positive safe integer text %s', (raw, expected) => { + expect(parsePositiveSafeIntegerText(raw)).toBe(expected) + }) + + it.each(['', '0', '-1', '1.5', '.1', '1e-1', '9007199254740991.1', '9007199254740992'])( + 'rejects inexact or unsafe integer text %s', + (raw) => { + expect(parsePositiveSafeIntegerText(raw)).toBeNull() + } + ) + + it.each([ + ['+1000', 1_000], + ['1000.0', 1_000], + ['1e3', 1_000] + ])('parses CLI-compatible positive integer text %s', (raw, expected) => { + expect(parsePositiveSafeIntegerNumericText(raw)).toBe(expected) + }) + + it.each(['', '0', '-1', '1.5', 'Infinity', '9007199254740992'])( + 'rejects invalid CLI-compatible integer text %s', + (raw) => { + expect(parsePositiveSafeIntegerNumericText(raw)).toBeNull() + } + ) +}) diff --git a/src/shared/timer-delay.ts b/src/shared/timer-delay.ts new file mode 100644 index 000000000..99172b99f --- /dev/null +++ b/src/shared/timer-delay.ts @@ -0,0 +1,53 @@ +export const MAX_TIMER_DELAY_MS = 2_147_483_647 + +export function isSafeTimerDelayMs(value: unknown): value is number { + return ( + typeof value === 'number' && + Number.isSafeInteger(value) && + value >= 0 && + value <= MAX_TIMER_DELAY_MS + ) +} + +export function parsePositiveSafeIntegerText(raw: string): number | null { + const trimmed = raw.trim() + const value = Number(trimmed) + if (!Number.isSafeInteger(value) || value <= 0) { + return null + } + const exactValue = parseExactIntegerNumericText(trimmed) + return exactValue === BigInt(value) ? value : null +} + +export function parsePositiveSafeIntegerNumericText(raw: string): number | null { + const value = Number(raw) + return Number.isSafeInteger(value) && value > 0 ? value : null +} + +function parseExactIntegerNumericText(raw: string): bigint | null { + if ( + /^\+?0[xX][\da-fA-F]+$/.test(raw) || + /^\+?0[bB][01]+$/.test(raw) || + /^\+?0[oO][0-7]+$/.test(raw) + ) { + return BigInt(raw.startsWith('+') ? raw.slice(1) : raw) + } + const match = /^\+?(\d+(?:\.\d*)?|\.\d+)(?:[eE]([+-]?\d+))?$/.exec(raw) + if (!match) { + return null + } + const [whole = '', fraction = ''] = match[1].split('.') + const digits = `${whole}${fraction}`.replace(/^0+/, '') || '0' + const shift = Number(match[2] ?? 0) - fraction.length + if (!Number.isSafeInteger(shift)) { + return null + } + if (shift >= 0) { + return BigInt(digits) * 10n ** BigInt(shift) + } + const removedDigits = -shift + if (removedDigits > digits.length || !digits.endsWith('0'.repeat(removedDigits))) { + return null + } + return BigInt(digits.slice(0, -removedDigits) || '0') +}