fix(cli): bound orchestration ask timeouts (#10689)
* fix(cli): bound orchestration ask timeouts * fix(cli): harden remote timeout boundaries
This commit is contained in:
parent
8b25cfc0f8
commit
76b2a3b44d
|
|
@ -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<string, string | boolean>) =>
|
||||
ORCHESTRATION_HANDLERS['orchestration check']({
|
||||
flags,
|
||||
client: { call: callMock },
|
||||
cwd: '/tmp/repo',
|
||||
json: true
|
||||
} as never)
|
||||
|
||||
const invokeAsk = (flags: Map<string, string | boolean>) =>
|
||||
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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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 }
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
@ -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<string, string | boolean>) =>
|
||||
ORCHESTRATION_HANDLERS['orchestration check']({
|
||||
flags,
|
||||
client: { call: callMock },
|
||||
cwd: '/tmp/repo',
|
||||
json: true
|
||||
} as never)
|
||||
|
||||
const invokeAsk = (flags: Map<string, string | boolean>) =>
|
||||
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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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<string, string | boolean>([
|
||||
['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({
|
||||
|
|
|
|||
|
|
@ -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<string, CommandHandler> = {
|
|||
|
||||
'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<string, CommandHandler> = {
|
|||
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) {
|
||||
|
|
|
|||
|
|
@ -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<ReturnType<typeof createServer>>()
|
||||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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<TResult>(
|
||||
metadata: RuntimeMetadata,
|
||||
|
|
@ -10,6 +11,12 @@ export async function sendRequest<TResult>(
|
|||
params: unknown,
|
||||
timeoutMs: number
|
||||
): Promise<RuntimeRpcResponse<TResult>> {
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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(',')
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
)
|
||||
})
|
||||
|
|
@ -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<string, unknown>,
|
||||
options?: { timeoutMs?: number }
|
||||
): Promise<unknown> {
|
||||
) {
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>): 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, unknown>): string[] | null {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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')
|
||||
|
||||
|
|
|
|||
|
|
@ -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<TResult>(
|
|||
params: unknown,
|
||||
timeoutMs: number
|
||||
): Promise<RuntimeRpcResponse<TResult>> {
|
||||
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<TResult>(
|
|||
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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
}
|
||||
)
|
||||
})
|
||||
|
|
@ -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')
|
||||
}
|
||||
Loading…
Reference in New Issue