diff --git a/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx b/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx index 0efbffc18c..3a603a0c3c 100644 --- a/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx +++ b/apps/desktop/src/components/assistant-ui/tool/approval.test.tsx @@ -6,7 +6,7 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vite import type { HermesGateway } from '@/hermes' import { handleApprovalKey, releaseApprovalKey } from '@/lib/keybinds/approval-keys' import { $gateway } from '@/store/gateway' -import { $approvalRequest, clearAllPrompts, sessionApprovalRequests, setApprovalRequest } from '@/store/prompts' +import { $approvalRequest, APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, clearAllPrompts, sessionApprovalRequests, setApprovalRequest } from '@/store/prompts' import { hasOpenServerRequest, rememberServerRequest, resetServerRequestsForTests } from '@/store/server-requests' import { $activeSessionId } from '@/store/session' import { stubMenuDomApis, stubResizeObserver } from '@/test/jsdom' @@ -212,12 +212,18 @@ describe('PendingApprovalStack', () => { fireEvent.click(screen.getByRole('button', { name: /Run/ })) await waitFor(() => { - expect(request).toHaveBeenCalledWith('approval.respond', { - all: false, - choice: 'once', - request_id: 'apr-1', - session_id: 'sess-1' - }) + expect(request).toHaveBeenCalledWith( + 'approval.respond', + { + all: false, + choice: 'once', + request_id: 'apr-1', + session_id: 'sess-1' + }, + // #55433: the respond RPC carries an explicit deadline covering the backend's approvals window. + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, + undefined + ) }) expect($approvalRequest.get()).toBeNull() }) @@ -327,12 +333,18 @@ describe('PendingApprovalStack', () => { handleApprovalKey(new KeyboardEvent('keydown', { key: 'Enter', repeat: index > 0, cancelable: true })) }) await waitFor(() => - expect(rpc).toHaveBeenCalledWith('approval.respond', { - all: false, - choice: 'once', - request_id: id, - session_id: 'sess-1' - }) + expect(rpc).toHaveBeenCalledWith( + 'approval.respond', + { + all: false, + choice: 'once', + request_id: id, + session_id: 'sess-1' + }, + // #55433: the respond RPC carries an explicit deadline covering the backend's approvals window. + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, + undefined + ) ) await waitFor(() => expect(screen.queryAllByRole('button', { name: /Run/ })).toHaveLength(index === 2 ? 0 : 1)) } diff --git a/apps/desktop/src/store/native-notifications.test.ts b/apps/desktop/src/store/native-notifications.test.ts index 6be0342d3e..550ab44fb7 100644 --- a/apps/desktop/src/store/native-notifications.test.ts +++ b/apps/desktop/src/store/native-notifications.test.ts @@ -16,7 +16,7 @@ import { setNativeNotifyKind } from './native-notifications' import { __resetNativeNotifyBaselineForTests, markNativeNotifyBaseline } from './notify-baseline' -import { $approvalRequest, clearAllPrompts, setApprovalRequest } from './prompts' +import { $approvalRequest, APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, clearAllPrompts, setApprovalRequest } from './prompts' import { markSessionGone, resetBackgroundPollingGuard } from './runtime-gone' import { setActiveSessionId } from './session' import { dropSessionState, publishSessionState } from './session-states' @@ -319,7 +319,13 @@ describe('respondToApprovalAction', () => { await respondToApprovalAction('bg', 'approve') - expect(request).toHaveBeenCalledWith('approval.respond', { all: false, choice: 'once', session_id: 'bg' }) + expect(request).toHaveBeenCalledWith( + 'approval.respond', + { all: false, choice: 'once', session_id: 'bg' }, + // #55433: the respond RPC carries an explicit deadline covering the backend's approvals window. + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, + undefined + ) expect($approvalRequest.get()).toBeNull() }) @@ -328,12 +334,18 @@ describe('respondToApprovalAction', () => { setApprovalRequest({ command: 'first', description: 'first', requestId: 'r1', sessionId: 'bg' }) setApprovalRequest({ command: 'second', description: 'second', requestId: 'r2', sessionId: 'bg' }) await respondToApprovalAction('bg', 'approve:r1') - expect(request).toHaveBeenCalledWith('approval.respond', { - all: false, - choice: 'once', - request_id: 'r1', - session_id: 'bg' - }) + expect(request).toHaveBeenCalledWith( + 'approval.respond', + { + all: false, + choice: 'once', + request_id: 'r1', + session_id: 'bg' + }, + // #55433: the respond RPC carries an explicit deadline covering the backend's approvals window. + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, + undefined + ) expect($approvalRequest.get()?.requestId).toBe('r2') await respondToApprovalAction('bg', 'approve:r1') expect($approvalRequest.get()?.requestId).toBe('r2') @@ -341,7 +353,13 @@ describe('respondToApprovalAction', () => { it('rejects via approval.respond {choice: "deny"}', async () => { await respondToApprovalAction('bg', 'reject') - expect(request).toHaveBeenCalledWith('approval.respond', { all: false, choice: 'deny', session_id: 'bg' }) + expect(request).toHaveBeenCalledWith( + 'approval.respond', + { all: false, choice: 'deny', session_id: 'bg' }, + // #55433: the respond RPC carries an explicit deadline covering the backend's approvals window. + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, + undefined + ) }) it('ignores unknown action ids', async () => { diff --git a/apps/desktop/src/store/prompts.test.ts b/apps/desktop/src/store/prompts.test.ts index 2c54ced455..34c45f5752 100644 --- a/apps/desktop/src/store/prompts.test.ts +++ b/apps/desktop/src/store/prompts.test.ts @@ -7,6 +7,8 @@ import { $approvalRequest, $secretRequest, $sudoRequest, + answerApproval, + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS, clearAllPrompts, clearApprovalRequest, clearSecretRequest, @@ -19,6 +21,7 @@ import { setSudoRequest } from './prompts' import { isSessionGone, resetBackgroundPollingGuard } from './runtime-gone' +import { resetServerRequestsForTests } from './server-requests' import { $activeSessionId, setActiveSessionId } from './session' // Prompts are parked per-session; the exported $*Request views are scoped to the @@ -276,6 +279,85 @@ describe('approval prompt store', () => { }) }) +describe('answerApproval', () => { + const target = { requestId: 'r1', serverRequestId: undefined, sessionId: 's1' } + + beforeEach(() => { + resetServerRequestsForTests() + }) + + it('sends approval.respond with a deadline that covers the backend approvals window', async () => { + const calls: Array<[string, Record, number | undefined]> = [] + + const gateway = { + request: async (method: string, params: Record, timeoutMs?: number) => { + calls.push([method, params, timeoutMs]) + + return { resolved: 1 } + } + } + + await answerApproval(gateway as never, target, 'once') + + // #55433: the generic 30s default fires long before the backend's 300s + // approvals.timeout; the RPC must carry an explicit longer deadline. + expect(calls).toHaveLength(1) + expect(calls[0][0]).toBe('approval.respond') + expect(calls[0][2]).toBe(APPROVAL_RESPOND_REQUEST_TIMEOUT_MS) + expect(APPROVAL_RESPOND_REQUEST_TIMEOUT_MS).toBeGreaterThanOrEqual(300_000) + }) + + it('retries once when the respond deadline fires behind a stalled WS', async () => { + let calls = 0 + + const gateway = { + request: async (_method: string, _params: Record, _timeoutMs?: number) => { + calls += 1 + + if (calls === 1) { + throw new Error(`request timed out after 330s: approval.respond`) + } + + return { resolved: 1 } + } + } + + // Resolves on the retry; a duplicate resolve is idempotent server-side. + await expect(answerApproval(gateway as never, target, 'deny')).resolves.toBeUndefined() + expect(calls).toBe(2) + }) + + it('propagates non-timeout failures without a retry', async () => { + let calls = 0 + + const gateway = { + request: async () => { + calls += 1 + throw new JsonRpcGatewayError('session not found', { code: 4001 }) + } + } + + await expect(answerApproval(gateway as never, target, 'once')).rejects.toThrow('session not found') + expect(calls).toBe(1) + }) + + it('answers the live server request without any RPC when one is open', async () => { + const { rememberServerRequest } = await import('./server-requests') + const respond = vi.fn() + rememberServerRequest({ fail: vi.fn(), id: 'srv-1', method: 'approval', params: {}, respond }) + const request = vi.fn() + + await answerApproval( + { request } as never, + { requestId: 'r1', serverRequestId: 'srv-1', sessionId: 's1' }, + 'once' + ) + + expect(respond).toHaveBeenCalledWith({ choice: 'once' }) + expect(request).not.toHaveBeenCalled() + }) +}) + describe('sudo prompt store', () => { it('clears only when the request id matches the in-flight prompt', () => { setSudoRequest({ requestId: 'abc', sessionId: 's1' }) diff --git a/apps/desktop/src/store/prompts.ts b/apps/desktop/src/store/prompts.ts index aaccaec25d..fd0c253aae 100644 --- a/apps/desktop/src/store/prompts.ts +++ b/apps/desktop/src/store/prompts.ts @@ -344,6 +344,22 @@ export async function replayPendingApproval(gateway: ApprovalGateway | null, ses * was restored from `approval.pending` or is being answered from another * surface. Returns after the backend has the decision. */ + +// #55433: the backend honors an answer for the whole `approvals.timeout` window +// (default 300s), but `approval.respond` otherwise rides the generic 30s RPC +// deadline. During a long LLM stream the gateway's WS writes can stall well past +// 30s while the turn is still live and the approval is still pending — the +// client gives up with "request timed out: approval.respond" long before the +// backend would. Give the RPC a deadline that covers the backend window (300s +// plus margin for the write to drain), and retry once on a deadline failure: +// `resolve_gateway_approval` pops the queue entry before committing, so a +// duplicate resolve is a harmless `resolved: 0`. +export const APPROVAL_RESPOND_REQUEST_TIMEOUT_MS = 330_000 + +function isRequestTimeoutError(error: unknown): boolean { + return error instanceof Error && /request timed out/i.test(error.message) +} + export async function answerApproval( gateway: ApprovalGateway | null, request: Pick, @@ -358,12 +374,36 @@ export async function answerApproval( throw new Error('Hermes gateway is not connected') } - await requestForOwnedSession(request.sessionId, ambientRequestFor(gateway), 'approval.respond', { + const params = { all, choice, ...(request.requestId ? { request_id: request.requestId } : {}), session_id: request.sessionId ?? undefined - }) + } + + try { + await requestForOwnedSession( + request.sessionId, + ambientRequestFor(gateway), + 'approval.respond', + params, + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS + ) + } catch (error) { + if (!isRequestTimeoutError(error)) { + throw error + } + + // The deadline fired while the approval may still be pending server-side + // (WS stall behind a long LLM stream). Resolve is idempotent: re-send once. + await requestForOwnedSession( + request.sessionId, + ambientRequestFor(gateway), + 'approval.respond', + params, + APPROVAL_RESPOND_REQUEST_TIMEOUT_MS + ) + } } /** The prompt request for one specific session — the tile counterpart of the diff --git a/apps/desktop/src/store/session-gone-latch.ts b/apps/desktop/src/store/session-gone-latch.ts index 5e22ebf550..17d23ab30c 100644 --- a/apps/desktop/src/store/session-gone-latch.ts +++ b/apps/desktop/src/store/session-gone-latch.ts @@ -120,11 +120,20 @@ export function resetBackgroundPollingGuardAfterRebind( } /** Adapt a store-level gateway handle (`$gateway.get()` or the narrower - * `ApprovalGateway` shape) to the ambient-request callback - * `requestForOwnedSession` expects. The pollers never pass a deadline, so the - * 2-arg call shape is kept exactly (gateway.request callers assert on it). */ + * `ApprovalGateway` shape) to the ambient-request callback + * `requestForOwnedSession` expects. Callers that never pass a deadline keep the + * 2-arg call shape exactly (gateway.request callers assert on it); one that + * does (approval.respond, #55433) has the deadline forwarded. */ export function ambientRequestFor(gateway: { - request: (method: string, params: Record) => Promise -}): (method: string, params?: Record) => Promise { - return (method: string, params?: Record) => gateway.request(method, params ?? {}) as Promise + request: (method: string, params: Record, timeoutMs?: number, signal?: AbortSignal) => Promise +}): (method: string, params?: Record, timeoutMs?: number, signal?: AbortSignal) => Promise { + return ( + method: string, + params?: Record, + timeoutMs?: number, + signal?: AbortSignal + ) => + (timeoutMs === undefined && signal === undefined + ? gateway.request(method, params ?? {}) + : gateway.request(method, params ?? {}, timeoutMs, signal)) as Promise }