From 480b79551b5f4e849c9f3341d6a8302141ce4bab Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 22 Jul 2026 15:08:06 -0400 Subject: [PATCH] fix: apply cliproxy retry settings safely --- .../routing/__tests__/retry-settings.test.ts | 182 +++++++++++++ .../__tests__/routing-strategy-http.test.ts | 40 +++ src/cliproxy/routing/retry-settings.ts | 244 ++++++++++++++++++ src/cliproxy/routing/routing-strategy-http.ts | 38 ++- .../routes/cliproxy-routing-routes.ts | 35 +++ .../cliproxy-routing-routes.test.ts | 58 +++++ .../cliproxy/cliproxy-retry-control.tsx | 112 ++++++++ .../cliproxy/routing-guidance-card.tsx | 89 +------ ui/src/hooks/use-cliproxy.ts | 46 +--- ui/src/lib/api-client.ts | 23 ++ .../cliproxy/cliproxy-retry-control.test.tsx | 90 +++++++ 11 files changed, 830 insertions(+), 127 deletions(-) create mode 100644 src/cliproxy/routing/__tests__/retry-settings.test.ts create mode 100644 src/cliproxy/routing/retry-settings.ts create mode 100644 ui/src/components/cliproxy/cliproxy-retry-control.tsx create mode 100644 ui/tests/unit/components/cliproxy/cliproxy-retry-control.test.tsx diff --git a/src/cliproxy/routing/__tests__/retry-settings.test.ts b/src/cliproxy/routing/__tests__/retry-settings.test.ts new file mode 100644 index 00000000..97405fdb --- /dev/null +++ b/src/cliproxy/routing/__tests__/retry-settings.test.ts @@ -0,0 +1,182 @@ +import { afterEach, beforeEach, describe, expect, it, mock } from 'bun:test'; +import type { ProxyTarget } from '../../proxy/proxy-target-resolver'; + +const localTarget: ProxyTarget = { + host: '127.0.0.1', + port: 8317, + protocol: 'http', + isRemote: false, +}; +const remoteTarget: ProxyTarget = { + host: 'proxy.example.com', + port: 443, + protocol: 'https', + isRemote: true, +}; + +describe('CLIProxy retry settings service', () => { + let target: ProxyTarget; + let config: { cliproxy?: { retry?: { request_retry?: number; max_retry_interval?: number } } }; + let regenerateMock: ReturnType; + let fetchRetryMock: ReturnType; + + beforeEach(() => { + target = localTarget; + config = { cliproxy: { retry: { request_retry: 1, max_retry_interval: 10 } } }; + regenerateMock = mock(() => '/tmp/config.yaml'); + fetchRetryMock = mock(); + + mock.module('../../config/generator', () => ({ regenerateConfig: regenerateMock })); + mock.module('../../config/path-resolver', () => ({ + getAuthDir: () => '/tmp/auth', + getConfigPathForPort: () => '/tmp/config.yaml', + })); + mock.module('../../../config/config-loader-facade', () => ({ + loadOrCreateUnifiedConfig: () => structuredClone(config), + mutateConfig: (mutator: (value: typeof config) => void) => { + mutator(config); + return structuredClone(config); + }, + })); + mock.module('../routing-strategy-http', () => ({ + getCliproxyRoutingTarget: () => target, + fetchCliproxyRetryResponse: fetchRetryMock, + getRoutingErrorMessage: async (response: Response, fallback: string) => { + const body = (await response.json().catch(() => null)) as { error?: string } | null; + return body?.error ?? fallback; + }, + })); + }); + + afterEach(() => mock.restore()); + + async function loadService() { + return import(`../retry-settings?test=${Date.now()}-${Math.random()}`) as Promise< + typeof import('../retry-settings') + >; + } + + it('reads both official management endpoints as one live state', async () => { + fetchRetryMock.mockImplementation( + async (_target: ProxyTarget, setting: string) => + new Response(JSON.stringify({ [setting]: setting === 'request-retry' ? 3 : 30 })) + ); + const { readCliproxyRetryState } = await loadService(); + + await expect(readCliproxyRetryState()).resolves.toEqual({ + request_retry: 3, + max_retry_interval: 30, + source: 'live', + target: 'local', + reachable: true, + manageable: true, + }); + expect(fetchRetryMock.mock.calls.map((call) => call[1])).toEqual([ + 'request-retry', + 'max-retry-interval', + ]); + }); + + it('keeps remote updates live-only', async () => { + target = remoteTarget; + fetchRetryMock.mockImplementation( + async (_target: ProxyTarget, setting: string, method: string) => + method === 'GET' + ? new Response(JSON.stringify({ [setting]: setting === 'request-retry' ? 1 : 10 })) + : new Response('{}') + ); + const { applyCliproxyRetrySettings } = await loadService(); + + const result = await applyCliproxyRetrySettings({ + request_retry: 4, + max_retry_interval: 40, + }); + + expect(result.applied).toBe('live'); + expect(config.cliproxy?.retry).toEqual({ request_retry: 1, max_retry_interval: 10 }); + expect(regenerateMock).not.toHaveBeenCalled(); + }); + + it('rolls back the first upstream value when the second PUT fails', async () => { + target = remoteTarget; + const puts: Array<[string, number]> = []; + fetchRetryMock.mockImplementation( + async (_target: ProxyTarget, setting: string, method: string, value?: number) => { + if (method === 'GET') { + return new Response(JSON.stringify({ [setting]: setting === 'request-retry' ? 2 : 20 })); + } + puts.push([setting, value as number]); + if (setting === 'max-retry-interval') { + return new Response(JSON.stringify({ error: 'second write failed' }), { status: 500 }); + } + return new Response('{}'); + } + ); + const { applyCliproxyRetrySettings } = await loadService(); + + await expect( + applyCliproxyRetrySettings({ request_retry: 5, max_retry_interval: 50 }) + ).rejects.toThrow('second write failed'); + expect(puts).toEqual([ + ['request-retry', 5], + ['max-retry-interval', 50], + ['request-retry', 2], + ]); + }); + + it('restores local persisted settings when regeneration fails', async () => { + fetchRetryMock.mockRejectedValue(new Error('offline')); + regenerateMock + .mockImplementationOnce(() => { + throw new Error('write failed'); + }) + .mockImplementationOnce(() => '/tmp/config.yaml'); + const { applyCliproxyRetrySettings } = await loadService(); + + await expect( + applyCliproxyRetrySettings({ request_retry: 9, max_retry_interval: 90 }) + ).rejects.toThrow('Saved retry settings were rolled back'); + expect(config.cliproxy?.retry).toEqual({ request_retry: 1, max_retry_interval: 10 }); + expect(regenerateMock).toHaveBeenCalledTimes(2); + }); + + it('serializes complete pair operations', async () => { + target = remoteTarget; + const events: string[] = []; + let releaseFirstRead: (() => void) | undefined; + const firstReadGate = new Promise((resolve) => { + releaseFirstRead = resolve; + }); + let requestReadCount = 0; + fetchRetryMock.mockImplementation( + async (_target: ProxyTarget, setting: string, method: string, value?: number) => { + events.push(`${method}:${setting}:${value ?? ''}`); + if (method === 'GET' && setting === 'request-retry' && requestReadCount++ === 0) { + await firstReadGate; + } + return method === 'GET' + ? new Response(JSON.stringify({ [setting]: setting === 'request-retry' ? 1 : 10 })) + : new Response('{}'); + } + ); + const { applyCliproxyRetrySettings } = await loadService(); + + const first = applyCliproxyRetrySettings({ request_retry: 2, max_retry_interval: 20 }); + const second = applyCliproxyRetrySettings({ request_retry: 3, max_retry_interval: 30 }); + await Promise.resolve(); + expect(events).toEqual(['GET:request-retry:']); + releaseFirstRead?.(); + await Promise.all([first, second]); + + expect(events).toEqual([ + 'GET:request-retry:', + 'GET:max-retry-interval:', + 'PUT:request-retry:2', + 'PUT:max-retry-interval:20', + 'GET:request-retry:', + 'GET:max-retry-interval:', + 'PUT:request-retry:3', + 'PUT:max-retry-interval:30', + ]); + }); +}); diff --git a/src/cliproxy/routing/__tests__/routing-strategy-http.test.ts b/src/cliproxy/routing/__tests__/routing-strategy-http.test.ts index 5f6682b9..2ae16bc6 100644 --- a/src/cliproxy/routing/__tests__/routing-strategy-http.test.ts +++ b/src/cliproxy/routing/__tests__/routing-strategy-http.test.ts @@ -104,4 +104,44 @@ describe('routing-strategy-http', () => { 'https://proxy.example.com:443/v0/management/routing/strategy' ); }); + + it('builds target-aware retry management URLs', async () => { + const { getCliproxyRetryManagementUrl } = await loadRoutingHttpModule(); + const target: ProxyTarget = { + host: 'proxy.example.com', + port: 443, + protocol: 'https', + isRemote: true, + }; + + expect(getCliproxyRetryManagementUrl(target, 'request-retry')).toBe( + 'https://proxy.example.com:443/v0/management/request-retry' + ); + expect(getCliproxyRetryManagementUrl(target, 'max-retry-interval')).toBe( + 'https://proxy.example.com:443/v0/management/max-retry-interval' + ); + }); + + it('sends retry updates as an integer value payload', async () => { + const originalFetch = globalThis.fetch; + const fetchMock = mock(async () => new Response('{}')); + globalThis.fetch = fetchMock as typeof fetch; + const target: ProxyTarget = { + host: '127.0.0.1', + port: 8317, + protocol: 'http', + isRemote: false, + }; + + try { + const { fetchCliproxyRetryResponse } = await loadRoutingHttpModule(); + await fetchCliproxyRetryResponse(target, 'request-retry', 'PUT', 4); + expect(fetchMock).toHaveBeenCalledWith( + 'http://127.0.0.1:8317/v0/management/request-retry', + expect.objectContaining({ method: 'PUT', body: JSON.stringify({ value: 4 }) }) + ); + } finally { + globalThis.fetch = originalFetch; + } + }); }); diff --git a/src/cliproxy/routing/retry-settings.ts b/src/cliproxy/routing/retry-settings.ts new file mode 100644 index 00000000..9bd9da63 --- /dev/null +++ b/src/cliproxy/routing/retry-settings.ts @@ -0,0 +1,244 @@ +import { regenerateConfig } from '../config/generator'; +import { getAuthDir, getConfigPathForPort } from '../config/path-resolver'; +import { loadOrCreateUnifiedConfig, mutateConfig } from '../../config/config-loader-facade'; +import type { ProxyTarget } from '../proxy/proxy-target-resolver'; +import { ConfigError, NetworkError } from '../../errors/error-types'; +import { + fetchCliproxyRetryResponse, + getCliproxyRoutingTarget, + getRoutingErrorMessage, + type CliproxyRetryManagementSetting, +} from './routing-strategy-http'; + +export interface CliproxyRetryValues { + request_retry: number; + max_retry_interval: number; +} + +export interface CliproxyRetryState extends CliproxyRetryValues { + source: 'live' | 'config'; + target: 'local' | 'remote'; + reachable: boolean; + manageable: boolean; + message?: string; +} + +export interface CliproxyRetryApplyResult extends CliproxyRetryState { + applied: 'live' | 'live-and-config' | 'config-only'; +} + +const DEFAULT_RETRY_VALUES: CliproxyRetryValues = { + request_retry: 0, + max_retry_interval: 0, +}; + +let retryOperationQueue: Promise = Promise.resolve(); + +export function normalizeCliproxyRetryValue(value: unknown): number | null { + return typeof value === 'number' && Number.isSafeInteger(value) && value >= 0 ? value : null; +} + +function serializeRetryOperation(operation: () => Promise): Promise { + const result = retryOperationQueue.then(operation, operation); + retryOperationQueue = result.then( + () => undefined, + () => undefined + ); + return result; +} + +function getConfiguredRetryValues(): CliproxyRetryValues { + const retry = loadOrCreateUnifiedConfig().cliproxy?.retry; + return { + request_retry: + normalizeCliproxyRetryValue(retry?.request_retry) ?? DEFAULT_RETRY_VALUES.request_retry, + max_retry_interval: + normalizeCliproxyRetryValue(retry?.max_retry_interval) ?? + DEFAULT_RETRY_VALUES.max_retry_interval, + }; +} + +async function readLiveRetryValue( + target: ProxyTarget, + setting: CliproxyRetryManagementSetting +): Promise { + const response = await fetchCliproxyRetryResponse(target, setting, 'GET'); + if (!response.ok) { + throw new NetworkError( + await getRoutingErrorMessage( + response, + `Failed to read CLIProxy ${setting} (${response.status})` + ) + ); + } + + const data = (await response.json()) as Record; + const value = normalizeCliproxyRetryValue(data[setting] ?? data.value); + if (value === null) { + throw new NetworkError(`CLIProxy returned an invalid ${setting} value`); + } + return value; +} + +async function readLiveRetryValues(target: ProxyTarget): Promise { + const requestRetry = await readLiveRetryValue(target, 'request-retry'); + const maxRetryInterval = await readLiveRetryValue(target, 'max-retry-interval'); + return { + request_retry: requestRetry, + max_retry_interval: maxRetryInterval, + }; +} + +async function putLiveRetryValue( + target: ProxyTarget, + setting: CliproxyRetryManagementSetting, + value: number +): Promise { + const response = await fetchCliproxyRetryResponse(target, setting, 'PUT', value); + if (!response.ok) { + throw new NetworkError( + await getRoutingErrorMessage( + response, + `Failed to update CLIProxy ${setting} (${response.status})` + ) + ); + } +} + +async function updateLiveRetryValues( + target: ProxyTarget, + values: CliproxyRetryValues, + previousRequestRetry: number +): Promise { + await putLiveRetryValue(target, 'request-retry', values.request_retry); + try { + await putLiveRetryValue(target, 'max-retry-interval', values.max_retry_interval); + } catch (error) { + try { + await putLiveRetryValue(target, 'request-retry', previousRequestRetry); + } catch (rollbackError) { + throw new NetworkError( + `${(error as Error).message}. Failed to roll back request-retry: ${(rollbackError as Error).message}` + ); + } + throw error; + } +} + +function persistLocalRetryValues(target: ProxyTarget, values: CliproxyRetryValues): void { + const previousRetry = loadOrCreateUnifiedConfig().cliproxy?.retry; + const previous = previousRetry ? { ...previousRetry } : undefined; + const configPath = getConfigPathForPort(target.port); + const authDir = getAuthDir(); + + mutateConfig((config) => { + config.cliproxy = config.cliproxy ?? {}; + config.cliproxy.retry = { ...values }; + }); + + try { + regenerateConfig(target.port, { configPath, authDir }); + } catch (error) { + mutateConfig((config) => { + config.cliproxy = config.cliproxy ?? {}; + if (previous) { + config.cliproxy.retry = previous; + } else { + delete config.cliproxy.retry; + } + }); + + try { + regenerateConfig(target.port, { configPath, authDir }); + } catch (rollbackError) { + throw new ConfigError( + `Failed to regenerate CLIProxy config: ${(error as Error).message}. Rollback regeneration also failed: ${(rollbackError as Error).message}` + ); + } + throw new ConfigError( + `Failed to regenerate CLIProxy config: ${(error as Error).message}. Saved retry settings were rolled back.` + ); + } +} + +export function readCliproxyRetryState(): Promise { + return serializeRetryOperation(async () => { + const target = getCliproxyRoutingTarget(); + try { + const values = await readLiveRetryValues(target); + return { + ...values, + source: 'live', + target: target.isRemote ? 'remote' : 'local', + reachable: true, + manageable: true, + }; + } catch (error) { + if (target.isRemote) throw error; + return { + ...getConfiguredRetryValues(), + source: 'config', + target: 'local', + reachable: false, + manageable: true, + message: 'Local CLIProxy is not reachable. Showing the saved startup defaults.', + }; + } + }); +} + +export function applyCliproxyRetrySettings( + values: CliproxyRetryValues +): Promise { + return serializeRetryOperation(async () => { + const target = getCliproxyRoutingTarget(); + let previousLive: CliproxyRetryValues; + + try { + previousLive = await readLiveRetryValues(target); + } catch (error) { + if (target.isRemote) throw error; + persistLocalRetryValues(target, values); + return { + ...values, + source: 'config', + target: 'local', + reachable: false, + manageable: true, + applied: 'config-only', + message: 'Saved the local startup defaults. They will apply the next time CLIProxy starts.', + }; + } + + if (!target.isRemote) { + persistLocalRetryValues(target, values); + } + + try { + await updateLiveRetryValues(target, values, previousLive.request_retry); + } catch (error) { + if (target.isRemote) throw error; + return { + ...values, + source: 'config', + target: 'local', + reachable: true, + manageable: true, + applied: 'config-only', + message: `Saved the local startup defaults, but the running proxy rejected the live update: ${(error as Error).message}`, + }; + } + + return { + ...values, + source: 'live', + target: target.isRemote ? 'remote' : 'local', + reachable: true, + manageable: true, + applied: target.isRemote ? 'live' : 'live-and-config', + message: target.isRemote + ? 'Updated the running remote CLIProxy. Local CCS config was not changed.' + : 'Updated the running local CLIProxy and saved the startup defaults.', + }; + }); +} diff --git a/src/cliproxy/routing/routing-strategy-http.ts b/src/cliproxy/routing/routing-strategy-http.ts index f361eb7e..1de1b57e 100644 --- a/src/cliproxy/routing/routing-strategy-http.ts +++ b/src/cliproxy/routing/routing-strategy-http.ts @@ -9,16 +9,38 @@ import { const ROUTING_TIMEOUT_MS = 5000; const CLIPROXY_ROUTING_MANAGEMENT_PATH = '/v0/management/routing/strategy'; +export type CliproxyRetryManagementSetting = 'request-retry' | 'max-retry-interval'; + export function getCliproxyRoutingManagementUrl(target: ProxyTarget): string { return buildProxyUrl(target, CLIPROXY_ROUTING_MANAGEMENT_PATH); } +export function getCliproxyRetryManagementUrl( + target: ProxyTarget, + setting: CliproxyRetryManagementSetting +): string { + return buildProxyUrl(target, `/v0/management/${setting}`); +} + export async function fetchCliproxyRoutingResponse( target: ProxyTarget, method: 'GET' | 'PUT', body?: Record ): Promise { - const url = getCliproxyRoutingManagementUrl(target); + return fetchCliproxyManagementResponse( + target, + getCliproxyRoutingManagementUrl(target), + method, + body + ); +} + +async function fetchCliproxyManagementResponse( + target: ProxyTarget, + url: string, + method: 'GET' | 'PUT', + body?: Record +): Promise { const headers = buildManagementHeaders( target, body ? { 'Content-Type': 'application/json' } : {} @@ -115,6 +137,20 @@ export async function fetchCliproxyRoutingResponse( }); } +export async function fetchCliproxyRetryResponse( + target: ProxyTarget, + setting: CliproxyRetryManagementSetting, + method: 'GET' | 'PUT', + value?: number +): Promise { + return fetchCliproxyManagementResponse( + target, + getCliproxyRetryManagementUrl(target, setting), + method, + value === undefined ? undefined : { value } + ); +} + export function getCliproxyRoutingTarget(): ProxyTarget { return getProxyTarget(); } diff --git a/src/web-server/routes/cliproxy-routing-routes.ts b/src/web-server/routes/cliproxy-routing-routes.ts index e9bf9e22..d104bd43 100644 --- a/src/web-server/routes/cliproxy-routing-routes.ts +++ b/src/web-server/routes/cliproxy-routing-routes.ts @@ -8,6 +8,11 @@ import { readCliproxyRoutingState, readCliproxySessionAffinityState, } from '../../cliproxy/routing/routing-strategy'; +import { + applyCliproxyRetrySettings, + normalizeCliproxyRetryValue, + readCliproxyRetryState, +} from '../../cliproxy/routing/retry-settings'; import { requireLocalAccessWhenAuthDisabled } from '../middleware/auth-middleware'; const router = Router(); @@ -84,4 +89,34 @@ router.put('/routing/session-affinity', async (req: Request, res: Response): Pro } }); +router.get('/retry', async (_req: Request, res: Response): Promise => { + try { + res.json(await readCliproxyRetryState()); + } catch (error) { + res.status(502).json({ error: (error as Error).message }); + } +}); + +router.put('/retry', async (req: Request, res: Response): Promise => { + const requestRetry = normalizeCliproxyRetryValue(req.body?.request_retry); + const maxRetryInterval = normalizeCliproxyRetryValue(req.body?.max_retry_interval); + if (requestRetry === null || maxRetryInterval === null) { + res.status(400).json({ + error: 'Invalid retry payload. Use non-negative safe integers for both retry fields.', + }); + return; + } + + try { + res.json( + await applyCliproxyRetrySettings({ + request_retry: requestRetry, + max_retry_interval: maxRetryInterval, + }) + ); + } catch (error) { + res.status(502).json({ error: (error as Error).message }); + } +}); + export default router; diff --git a/tests/unit/web-server/cliproxy-routing-routes.test.ts b/tests/unit/web-server/cliproxy-routing-routes.test.ts index 1c828b84..86038868 100644 --- a/tests/unit/web-server/cliproxy-routing-routes.test.ts +++ b/tests/unit/web-server/cliproxy-routing-routes.test.ts @@ -9,6 +9,8 @@ describe('cliproxy routing routes', () => { let applyStrategyMock: ReturnType; let readAffinityStateMock: ReturnType; let applyAffinityMock: ReturnType; + let readRetryMock: ReturnType; + let applyRetryMock: ReturnType; beforeEach(async () => { readStateMock = mock(async () => ({ @@ -41,6 +43,23 @@ describe('cliproxy routing routes', () => { manageable: true, applied: 'config-only', })); + readRetryMock = mock(async () => ({ + request_retry: 2, + max_retry_interval: 20, + source: 'live', + target: 'local', + reachable: true, + manageable: true, + })); + applyRetryMock = mock(async () => ({ + request_retry: 3, + max_retry_interval: 30, + source: 'live', + target: 'local', + reachable: true, + manageable: true, + applied: 'live-and-config', + })); mock.module('../../../src/cliproxy/routing/routing-strategy', () => ({ readCliproxyRoutingState: readStateMock, @@ -62,6 +81,12 @@ describe('cliproxy routing routes', () => { return null; }, })); + mock.module('../../../src/cliproxy/routing/retry-settings', () => ({ + readCliproxyRetryState: readRetryMock, + applyCliproxyRetrySettings: applyRetryMock, + normalizeCliproxyRetryValue: (value: unknown) => + typeof value === 'number' && Number.isSafeInteger(value) && value >= 0 ? value : null, + })); const { default: routingRoutes } = await import( `../../../src/web-server/routes/cliproxy-routing-routes?test=${Date.now()}-${Math.random()}` @@ -180,4 +205,37 @@ describe('cliproxy routing routes', () => { applied: 'config-only', }); }); + + it('returns and updates retry settings through the dedicated route', async () => { + const readResponse = await fetch(`${baseUrl}/api/cliproxy/retry`); + expect(readResponse.status).toBe(200); + expect((await readResponse.json()).request_retry).toBe(2); + + const updateResponse = await fetch(`${baseUrl}/api/cliproxy/retry`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ request_retry: 3, max_retry_interval: 30 }), + }); + expect(updateResponse.status).toBe(200); + expect(applyRetryMock).toHaveBeenCalledWith({ + request_retry: 3, + max_retry_interval: 30, + }); + }); + + it.each([ + { request_retry: -1, max_retry_interval: 30 }, + { request_retry: 1.5, max_retry_interval: 30 }, + { request_retry: Number.MAX_SAFE_INTEGER + 1, max_retry_interval: 30 }, + { request_retry: 1, max_retry_interval: '30' }, + ])('rejects invalid retry payload %#', async (payload) => { + const response = await fetch(`${baseUrl}/api/cliproxy/retry`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(payload), + }); + + expect(response.status).toBe(400); + expect(applyRetryMock).not.toHaveBeenCalled(); + }); }); diff --git a/ui/src/components/cliproxy/cliproxy-retry-control.tsx b/ui/src/components/cliproxy/cliproxy-retry-control.tsx new file mode 100644 index 00000000..830acb2c --- /dev/null +++ b/ui/src/components/cliproxy/cliproxy-retry-control.tsx @@ -0,0 +1,112 @@ +import { useState } from 'react'; +import { useTranslation } from 'react-i18next'; +import { useCliproxyRetryConfig, useUpdateCliproxyRetryConfig } from '@/hooks/use-cliproxy'; + +function parseRetryValue(value: string): number | null { + if (!/^\d+$/.test(value)) return null; + const parsed = Number(value); + return Number.isSafeInteger(parsed) ? parsed : null; +} + +export function CliproxyRetryControl() { + const { t } = useTranslation(); + const retryQuery = useCliproxyRetryConfig(); + const updateRetry = useUpdateCliproxyRetryConfig(); + const requestRetry = retryQuery.data?.request_retry ?? 0; + const maxRetryInterval = retryQuery.data?.max_retry_interval ?? 0; + const manageable = retryQuery.data?.manageable !== false; + const disabled = + retryQuery.isLoading || retryQuery.isError || updateRetry.isPending || !manageable; + const statusMessage = retryQuery.error?.message ?? retryQuery.data?.message; + + return ( +
+
+
+
+ {t('routingGuidance.retryTitle')} +
+
+ {t('routingGuidance.retryHint')} + {retryQuery.data ? ` · ${retryQuery.data.source} · ${retryQuery.data.target}` : ''} +
+
+ + updateRetry.mutate({ + request_retry: nextRequestRetry, + max_retry_interval: nextMaxRetryInterval, + }) + } + /> +
+ {statusMessage ? ( +
+ {statusMessage} +
+ ) : null} +
+ ); +} + +interface RetryInputsProps { + requestRetry: number; + maxRetryInterval: number; + disabled: boolean; + onUpdate: (requestRetry: number, maxRetryInterval: number) => void; +} + +function RetryInputs({ requestRetry, maxRetryInterval, disabled, onUpdate }: RetryInputsProps) { + const { t } = useTranslation(); + const [requestRetryInput, setRequestRetryInput] = useState(String(requestRetry)); + const [maxRetryIntervalInput, setMaxRetryIntervalInput] = useState(String(maxRetryInterval)); + const [fieldError, setFieldError] = useState(null); + + const handleBlur = () => { + if (disabled) return; + const nextRequestRetry = parseRetryValue(requestRetryInput.trim()); + const nextMaxRetryInterval = parseRetryValue(maxRetryIntervalInput.trim()); + if (nextRequestRetry === null || nextMaxRetryInterval === null) { + setFieldError(t('routingGuidance.retryRangeError')); + setRequestRetryInput(String(requestRetry)); + setMaxRetryIntervalInput(String(maxRetryInterval)); + return; + } + + setFieldError(null); + if (nextRequestRetry !== requestRetry || nextMaxRetryInterval !== maxRetryInterval) { + onUpdate(nextRequestRetry, nextMaxRetryInterval); + } + }; + + return ( +
+
+ setRequestRetryInput(event.target.value)} + onBlur={handleBlur} + disabled={disabled} + /> + / + setMaxRetryIntervalInput(event.target.value)} + onBlur={handleBlur} + disabled={disabled} + /> +
+ {fieldError ?
{fieldError}
: null} +
+ ); +} diff --git a/ui/src/components/cliproxy/routing-guidance-card.tsx b/ui/src/components/cliproxy/routing-guidance-card.tsx index ea175699..69e52caf 100644 --- a/ui/src/components/cliproxy/routing-guidance-card.tsx +++ b/ui/src/components/cliproxy/routing-guidance-card.tsx @@ -7,15 +7,10 @@ import type { RoutingStrategy, CliproxySessionAffinityState, } from '@/lib/api-client'; -import { useCliproxyRetryConfig, useUpdateCliproxyRetryConfig } from '@/hooks/use-cliproxy'; +import { CliproxyRetryControl } from './cliproxy-retry-control'; import { cn } from '@/lib/utils'; import { useTranslation } from 'react-i18next'; -/** Retry fields accept only non-negative integers (matches the CLIProxy schema bounds). */ -function isValidRetryFieldValue(value: number): boolean { - return Number.isInteger(value) && value >= 0; -} - interface RoutingGuidanceCardProps { className?: string; compact?: boolean; @@ -79,17 +74,6 @@ export function RoutingGuidanceCard({ const pendingAffinityRef = useRef<{ enabled: boolean; ttl: string } | null>(null); const suppressNextAffinityBlurRef = useRef(false); - const retryConfigQuery = useCliproxyRetryConfig(); - const updateRetryConfig = useUpdateCliproxyRetryConfig(); - const currentRequestRetry = retryConfigQuery.data?.request_retry ?? 0; - const currentMaxRetryInterval = retryConfigQuery.data?.max_retry_interval ?? 0; - const [requestRetryInput, setRequestRetryInput] = useState(String(currentRequestRetry)); - const [maxRetryIntervalInput, setMaxRetryIntervalInput] = useState( - String(currentMaxRetryInterval) - ); - const [retryFieldError, setRetryFieldError] = useState(null); - const retryControlDisabled = retryConfigQuery.isLoading || updateRetryConfig.isPending; - useEffect(() => { setSelected(currentStrategy); }, [currentStrategy]); @@ -99,11 +83,6 @@ export function RoutingGuidanceCard({ setSelectedAffinityTtl(currentAffinityTtl); }, [currentAffinityEnabled, currentAffinityTtl]); - useEffect(() => { - setRequestRetryInput(String(currentRequestRetry)); - setMaxRetryIntervalInput(String(currentMaxRetryInterval)); - }, [currentRequestRetry, currentMaxRetryInterval]); - useEffect(() => { if (isSaving || !pendingAffinityRef.current) { return; @@ -144,34 +123,6 @@ export function RoutingGuidanceCard({ onApplyAffinity({ enabled: selectedAffinityEnabled, ttl: nextTtl }); }; - const handleRetryBlur = () => { - const nextRequestRetry = Number(requestRetryInput.trim()); - const nextMaxRetryInterval = Number(maxRetryIntervalInput.trim()); - - if ( - !isValidRetryFieldValue(nextRequestRetry) || - !isValidRetryFieldValue(nextMaxRetryInterval) - ) { - setRetryFieldError(t('routingGuidance.retryRangeError')); - setRequestRetryInput(String(currentRequestRetry)); - setMaxRetryIntervalInput(String(currentMaxRetryInterval)); - return; - } - - setRetryFieldError(null); - if ( - nextRequestRetry === currentRequestRetry && - nextMaxRetryInterval === currentMaxRetryInterval - ) { - return; - } - - updateRetryConfig.mutate({ - request_retry: nextRequestRetry, - max_retry_interval: nextMaxRetryInterval, - }); - }; - if (compact) { const handleApply = (s: RoutingStrategy) => { setSelected(s); @@ -325,43 +276,7 @@ export function RoutingGuidanceCard({ ) : null} -
-
-
- {t('routingGuidance.retryTitle')} -
-
- {t('routingGuidance.retryHint')} -
-
-
- setRequestRetryInput(event.target.value.replace(/\D/g, ''))} - onBlur={handleRetryBlur} - disabled={retryControlDisabled} - /> - / - setMaxRetryIntervalInput(event.target.value.replace(/\D/g, ''))} - onBlur={handleRetryBlur} - disabled={retryControlDisabled} - /> -
-
- - {retryFieldError ? ( -
- {retryFieldError} -
- ) : null} + ); } diff --git a/ui/src/hooks/use-cliproxy.ts b/ui/src/hooks/use-cliproxy.ts index 15ced15d..b7a03751 100644 --- a/ui/src/hooks/use-cliproxy.ts +++ b/ui/src/hooks/use-cliproxy.ts @@ -12,6 +12,8 @@ import { type CreatePreset, type RoutingStrategy, type CliproxySessionAffinityApplyResult, + type CliproxyRetryApplyResult, + type CliproxyRetryValues, } from '@/lib/api-client'; import { toast } from 'sonner'; import { useTranslation } from 'react-i18next'; @@ -106,57 +108,23 @@ export function useUpdateCliproxySessionAffinity() { }); } -/** CLIProxy request-retry config (config.cliproxy.retry). Defaults to disabled (0/0). */ -export interface CliproxyRetryConfig { - request_retry: number; - max_retry_interval: number; -} - -const DEFAULT_CLIPROXY_RETRY_CONFIG: CliproxyRetryConfig = { - request_retry: 0, - max_retry_interval: 0, -}; - export function useCliproxyRetryConfig() { return useQuery({ queryKey: ['cliproxy-retry-config'], - queryFn: async (): Promise => { - const config = await api.config.get(); - const cliproxy = config.cliproxy as { retry?: Partial } | undefined; - return { - request_retry: - cliproxy?.retry?.request_retry ?? DEFAULT_CLIPROXY_RETRY_CONFIG.request_retry, - max_retry_interval: - cliproxy?.retry?.max_retry_interval ?? DEFAULT_CLIPROXY_RETRY_CONFIG.max_retry_interval, - }; - }, + queryFn: () => api.cliproxy.getRetrySettings(), }); } -/** - * Save CLIProxy retry config. Reuses the generic unified-config save path - * (PUT /config) — retry is a static opt-in default, not a live-proxy setting - * like routing.strategy/session_affinity, so it does not need a dedicated - * management-API round trip. - */ export function useUpdateCliproxyRetryConfig() { const queryClient = useQueryClient(); const { t } = useTranslation(); return useMutation({ - mutationFn: async (retry: CliproxyRetryConfig): Promise => { - const config = await api.config.get(); - const existingCliproxy = (config.cliproxy ?? {}) as Record; - await api.config.update({ - ...config, - cliproxy: { ...existingCliproxy, retry }, - }); - return retry; - }, - onSuccess: (retry) => { - queryClient.setQueryData(['cliproxy-retry-config'], retry); + mutationFn: (retry: CliproxyRetryValues) => api.cliproxy.updateRetrySettings(retry), + onSuccess: (result: CliproxyRetryApplyResult) => { + queryClient.setQueryData(['cliproxy-retry-config'], result); queryClient.invalidateQueries({ queryKey: ['cliproxy-retry-config'] }); - toast.success(t('toasts.cliproxyRetryUpdated')); + toast.success(result.message || t('toasts.cliproxyRetryUpdated')); }, onError: (error: Error) => { toast.error(error.message); diff --git a/ui/src/lib/api-client.ts b/ui/src/lib/api-client.ts index f8b12736..15d42477 100644 --- a/ui/src/lib/api-client.ts +++ b/ui/src/lib/api-client.ts @@ -550,6 +550,23 @@ export interface CliproxySessionAffinityApplyResult extends CliproxySessionAffin applied: 'config-and-live' | 'config-only' | 'unsupported'; } +export interface CliproxyRetryValues { + request_retry: number; + max_retry_interval: number; +} + +export interface CliproxyRetryState extends CliproxyRetryValues { + source: 'live' | 'config'; + target: 'local' | 'remote'; + reachable: boolean; + manageable: boolean; + message?: string; +} + +export interface CliproxyRetryApplyResult extends CliproxyRetryState { + applied: 'live' | 'live-and-config' | 'config-only'; +} + /** Auth file info for Config tab */ export interface AuthFile { name: string; @@ -1327,6 +1344,12 @@ export const api = { method: 'PUT', body: JSON.stringify(data), }), + getRetrySettings: () => request('/cliproxy/retry'), + updateRetrySettings: (data: CliproxyRetryValues) => + request('/cliproxy/retry', { + method: 'PUT', + body: JSON.stringify(data), + }), aiProviders: { list: () => request('/cliproxy/ai-providers'), create: (family: AiProviderFamilyId, data: UpsertAiProviderEntryInput) => diff --git a/ui/tests/unit/components/cliproxy/cliproxy-retry-control.test.tsx b/ui/tests/unit/components/cliproxy/cliproxy-retry-control.test.tsx new file mode 100644 index 00000000..fc467377 --- /dev/null +++ b/ui/tests/unit/components/cliproxy/cliproxy-retry-control.test.tsx @@ -0,0 +1,90 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { fireEvent, render, screen } from '../../../setup/test-utils'; +import { CliproxyRetryControl } from '@/components/cliproxy/cliproxy-retry-control'; + +const hookState = vi.hoisted(() => ({ + query: { + data: { + request_retry: 2, + max_retry_interval: 20, + source: 'live' as const, + target: 'local' as const, + reachable: true, + manageable: true, + message: undefined as string | undefined, + }, + isLoading: false, + isError: false, + error: null as Error | null, + }, + mutation: { + isPending: false, + mutate: vi.fn(), + }, +})); + +vi.mock('@/hooks/use-cliproxy', () => ({ + useCliproxyRetryConfig: () => hookState.query, + useUpdateCliproxyRetryConfig: () => hookState.mutation, +})); + +describe('CliproxyRetryControl', () => { + beforeEach(() => { + hookState.query.data = { + request_retry: 2, + max_retry_interval: 20, + source: 'live', + target: 'local', + reachable: true, + manageable: true, + message: undefined, + }; + hookState.query.isLoading = false; + hookState.query.isError = false; + hookState.query.error = null; + hookState.mutation.isPending = false; + hookState.mutation.mutate.mockReset(); + }); + + it('updates the pair through the dedicated retry mutation', () => { + render(); + const requestRetry = screen.getByRole('textbox', { name: 'Request retry count' }); + fireEvent.change(requestRetry, { target: { value: '4' } }); + fireEvent.blur(requestRetry); + + expect(hookState.mutation.mutate).toHaveBeenCalledWith({ + request_retry: 4, + max_retry_interval: 20, + }); + expect(screen.getByText(/live · local/i)).toBeInTheDocument(); + }); + + it('rejects values outside the safe non-negative integer range', () => { + render(); + const requestRetry = screen.getByRole('textbox', { name: 'Request retry count' }); + fireEvent.change(requestRetry, { target: { value: String(Number.MAX_SAFE_INTEGER + 1) } }); + fireEvent.blur(requestRetry); + + expect(hookState.mutation.mutate).not.toHaveBeenCalled(); + expect(screen.getByText('Must be a whole number, 0 or greater.')).toBeInTheDocument(); + }); + + it('disables editing on query errors, unmanageable state, or a pending update', () => { + hookState.query.isError = true; + hookState.query.error = new Error('Retry management unavailable'); + const { rerender } = render(); + expect(screen.getByRole('textbox', { name: 'Request retry count' })).toBeDisabled(); + expect(screen.getByText('Retry management unavailable')).toBeInTheDocument(); + + hookState.query.isError = false; + hookState.query.error = null; + hookState.query.data = { ...hookState.query.data, manageable: false }; + rerender(); + expect(screen.getByRole('textbox', { name: 'Request retry count' })).toBeDisabled(); + + hookState.query.data = { ...hookState.query.data, manageable: true }; + hookState.mutation.isPending = true; + rerender(); + expect(screen.getByRole('textbox', { name: 'Request retry count' })).toBeDisabled(); + }); +});