diff --git a/src/web-server/routes/cliproxy-auth-routes.ts b/src/web-server/routes/cliproxy-auth-routes.ts index 17d761e2..af838c5b 100644 --- a/src/web-server/routes/cliproxy-auth-routes.ts +++ b/src/web-server/routes/cliproxy-auth-routes.ts @@ -50,29 +50,14 @@ import { validateAntigravityRiskAcknowledgement, isAntigravityResponsibilityBypassEnabled, } from '../../cliproxy/antigravity-responsibility'; +import { createRouteErrorHelpers } from './route-helpers'; const router = Router(); // Valid providers list - derived from canonical CLIPROXY_PROFILES const validProviders: CLIProxyProvider[] = [...CLIPROXY_PROFILES]; -function logRouteError(context: string, error: unknown): void { - if (error instanceof Error) { - console.error(`[cliproxy-auth-routes] ${context}: ${error.message}`); - return; - } - console.error(`[cliproxy-auth-routes] ${context}: unknown error`); -} - -function respondInternalError( - res: Response, - error: unknown, - fallbackMessage: string, - statusCode = 500 -): void { - logRouteError(fallbackMessage, error); - res.status(statusCode).json({ error: fallbackMessage }); -} +const { respondInternalError } = createRouteErrorHelpers('cliproxy-auth-routes'); function parseKiroMethod(raw: unknown): { method: KiroAuthMethod; invalid: boolean } { if (raw === undefined || raw === null) { diff --git a/src/web-server/routes/cliproxy-stats-routes.ts b/src/web-server/routes/cliproxy-stats-routes.ts index 99bfb23b..4b082730 100644 --- a/src/web-server/routes/cliproxy-stats-routes.ts +++ b/src/web-server/routes/cliproxy-stats-routes.ts @@ -100,62 +100,21 @@ function isQuotaRouteRateLimited(req: Request, provider: string): boolean { } /** - * Cache only stable failures; avoid pinning transient network failures (timeouts, 429s). + * Cache only stable failures; skip transient network errors (timeouts, 429s, 5xx). + * Generic across all quota result types. */ -function shouldCacheCodexQuotaResult(result: CodexQuotaResult): boolean { +function shouldCacheQuotaResult(result: { + success: boolean; + needsReauth?: boolean; + isForbidden?: boolean; + error?: string; +}): boolean { if (result.success) return true; if (result.needsReauth || result.isForbidden) return true; - const msg = (result.error || '').toLowerCase(); if (!msg) return false; - if (msg.includes('timeout')) return false; - if (msg.includes('rate limited')) return false; - if (msg.includes('api error: 5')) return false; - if (msg.includes('fetch failed')) return false; - - return false; -} - -function shouldCacheGeminiQuotaResult(result: GeminiCliQuotaResult): boolean { - if (result.success) return true; - if (result.needsReauth) return true; - - const msg = (result.error || '').toLowerCase(); - if (!msg) return false; - if (msg.includes('timeout')) return false; - if (msg.includes('rate limited')) return false; - if (msg.includes('api error: 5')) return false; - if (msg.includes('fetch failed')) return false; - - return false; -} - -function shouldCacheClaudeQuotaResult(result: ClaudeQuotaResult): boolean { - if (result.success) return true; - if (result.needsReauth) return true; - - const msg = (result.error || '').toLowerCase(); - if (!msg) return false; - if (msg.includes('timeout')) return false; - if (msg.includes('rate limited')) return false; - if (msg.includes('api error: 5')) return false; - if (msg.includes('fetch failed')) return false; - - return false; -} - -function shouldCacheGhcpQuotaResult(result: GhcpQuotaResult): boolean { - if (result.success) return true; - if (result.needsReauth) return true; - - const msg = (result.error || '').toLowerCase(); - if (!msg) return false; - if (msg.includes('timeout')) return false; - if (msg.includes('rate limited')) return false; - if (msg.includes('api error: 5')) return false; - if (msg.includes('fetch failed')) return false; - - return false; + const transientPatterns = ['timeout', 'rate limited', 'api error: 5', 'fetch failed']; + return !transientPatterns.some((p) => msg.includes(p)); } /** Get configured backend from config */ @@ -169,20 +128,21 @@ function getConfiguredBackend() { } /** - * Extract status code and model from error log file (lightweight parsing) - * Reads first 4KB for model, last 2KB for status code + * Extract status code and model from error log file (lightweight parsing). + * Reads first 4KB for model, last 2KB for status code. Async to avoid blocking event loop. */ async function extractErrorLogMetadata( filePath: string ): Promise<{ statusCode?: number; model?: string }> { + let fh: fs.promises.FileHandle | null = null; try { - const fd = fs.openSync(filePath, 'r'); - const stat = fs.fstatSync(fd); + fh = await fs.promises.open(filePath, 'r'); + const stat = await fh.stat(); const fileSize = stat.size; // Read first 4KB for model (in request body) const startBuffer = Buffer.alloc(Math.min(4096, fileSize)); - fs.readSync(fd, startBuffer, 0, startBuffer.length, 0); + await fh.read(startBuffer, 0, startBuffer.length, 0); const startContent = startBuffer.toString('utf-8'); // Extract model from request body JSON: "model":"gemini-3-flash-preview" @@ -193,7 +153,7 @@ async function extractErrorLogMetadata( let statusCode: number | undefined; if (fileSize > 2048) { const endBuffer = Buffer.alloc(2048); - fs.readSync(fd, endBuffer, 0, 2048, fileSize - 2048); + await fh.read(endBuffer, 0, 2048, fileSize - 2048); const endContent = endBuffer.toString('utf-8'); const statusMatch = endContent.match(/Status:\s*(\d{3})/); statusCode = statusMatch ? parseInt(statusMatch[1], 10) : undefined; @@ -203,10 +163,11 @@ async function extractErrorLogMetadata( statusCode = statusMatch ? parseInt(statusMatch[1], 10) : undefined; } - fs.closeSync(fd); return { statusCode, model }; } catch { return {}; + } finally { + await fh?.close(); } } @@ -237,7 +198,8 @@ const handleStatsRequest = async (_req: Request, res: Response): Promise = res.json(stats); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }; @@ -261,7 +223,8 @@ router.get('/status', async (_req: Request, res: Response): Promise => { const running = await isCliproxyRunning(); res.json({ running }); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -297,7 +260,8 @@ router.get('/proxy-status', async (_req: Request, res: Response): Promise res.json(sessionStatus); } } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -311,7 +275,8 @@ router.post('/proxy-start', async (_req: Request, res: Response): Promise const result = await ensureCliproxyService(); res.json(result); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -324,7 +289,8 @@ router.post('/proxy-stop', async (_req: Request, res: Response): Promise = const result = await stopProxy(); res.json(result); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -338,7 +304,8 @@ router.get('/update-check', async (_req: Request, res: Response): Promise const result = await checkCliproxyUpdate(backend); res.json(result); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -370,7 +337,8 @@ router.get('/models', async (_req: Request, res: Response): Promise => { res.json(modelsResponse); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -417,7 +385,8 @@ router.get('/error-logs', async (_req: Request, res: Response): Promise => res.json({ files: filesWithMetadata }); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -456,7 +425,8 @@ router.get('/error-logs/:name', async (req: Request, res: Response): Promise = const content = fs.readFileSync(configPath, 'utf8'); res.type('text/yaml').send(content); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -510,7 +481,8 @@ router.put('/config.yaml', async (req: Request, res: Response): Promise => res.json({ success: true, path: configPath }); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -544,7 +516,8 @@ router.get('/auth-files', async (_req: Request, res: Response): Promise => res.json({ files, directory: authDir }); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -580,7 +553,8 @@ router.get('/auth-files/download', async (req: Request, res: Response): Promise< res.setHeader('Content-Disposition', `attachment; filename="${name}"`); res.type('application/octet-stream').send(content); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -667,7 +641,8 @@ router.put('/models/:provider', async (req: Request, res: Response): Promise => { faultyRange: CLIPROXY_FAULTY_RANGE, }); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -1004,7 +985,8 @@ router.post('/install', async (req: Request, res: Response): Promise => { message: `Successfully installed CLIProxy Plus v${version}`, }); } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); @@ -1029,7 +1011,8 @@ router.post('/restart', async (_req: Request, res: Response): Promise => { res.json({ success: false, error: startResult.error || 'Failed to start proxy' }); } } catch (error) { - res.status(500).json({ error: (error as Error).message }); + console.error(`[cliproxy-stats] ${(error as Error).message}`); + res.status(500).json({ error: 'Internal server error' }); } }); diff --git a/src/web-server/routes/misc-routes.ts b/src/web-server/routes/misc-routes.ts index fb3037c9..a3c7fbdc 100644 --- a/src/web-server/routes/misc-routes.ts +++ b/src/web-server/routes/misc-routes.ts @@ -10,6 +10,7 @@ import { expandPath } from '../../utils/helpers'; import { loadOrCreateUnifiedConfig, saveUnifiedConfig, + mutateUnifiedConfig, getGlobalEnvConfig, getThinkingConfig, getConfigYamlPath, @@ -218,28 +219,27 @@ router.get('/global-env', (_req: Request, res: Response): void => { * Updates the global_env section in config.yaml */ router.put('/global-env', (req: Request, res: Response): void => { - try { - const { enabled, env } = req.body; - const config = loadOrCreateUnifiedConfig(); + const { enabled, env } = req.body; - // Validate env is an object with string values - if (env !== undefined && typeof env === 'object' && env !== null) { - for (const [key, value] of Object.entries(env)) { - if (typeof value !== 'string') { - res.status(400).json({ error: `Invalid value for ${key}: must be a string` }); - return; - } + // Validate env values before acquiring write lock + if (env !== undefined && typeof env === 'object' && env !== null) { + for (const [key, value] of Object.entries(env)) { + if (typeof value !== 'string') { + res.status(400).json({ error: `Invalid value for ${key}: must be a string` }); + return; } } + } - // Update global_env section - config.global_env = { - enabled: enabled ?? config.global_env?.enabled ?? true, - env: env ?? config.global_env?.env ?? {}, - }; - - saveUnifiedConfig(config); - res.json({ success: true, config: config.global_env }); + try { + // Atomic read-modify-write — avoids race between load and save + const updated = mutateUnifiedConfig((config) => { + config.global_env = { + enabled: enabled ?? config.global_env?.enabled ?? true, + env: env ?? config.global_env?.env ?? {}, + }; + }); + res.json({ success: true, config: updated.global_env }); } catch (error) { res.status(500).json({ error: (error as Error).message }); } diff --git a/src/web-server/routes/profile-routes.ts b/src/web-server/routes/profile-routes.ts index b450c77e..f8c87cab 100644 --- a/src/web-server/routes/profile-routes.ts +++ b/src/web-server/routes/profile-routes.ts @@ -13,9 +13,8 @@ import { updateApiProfileTarget, } from '../../api/services/profile-writer'; import { apiProfileExists, listApiProfiles } from '../../api/services/profile-reader'; -import type { TargetType } from '../../targets/target-adapter'; import { normalizeDroidProvider } from '../../targets/droid-provider'; -import { updateSettingsFile } from './route-helpers'; +import { updateSettingsFile, parseTarget } from './route-helpers'; const router = Router(); @@ -23,23 +22,6 @@ function isDenylistError(message: string | undefined): boolean { return typeof message === 'string' && message.toLowerCase().includes('denylist'); } -export function parseTarget(rawTarget: unknown): TargetType | null { - if (rawTarget === undefined || rawTarget === null || rawTarget === '') { - return null; - } - - if (typeof rawTarget !== 'string') { - return null; - } - - const normalized = rawTarget.trim().toLowerCase(); - if (normalized === 'claude' || normalized === 'droid') { - return normalized; - } - - return null; -} - // ==================== Profile CRUD ==================== /** diff --git a/src/web-server/routes/proxy-routes.ts b/src/web-server/routes/proxy-routes.ts index a8c92f84..2d9ba46d 100644 --- a/src/web-server/routes/proxy-routes.ts +++ b/src/web-server/routes/proxy-routes.ts @@ -8,7 +8,7 @@ */ import { Router, Request, Response } from 'express'; -import { loadOrCreateUnifiedConfig, saveUnifiedConfig } from '../../config/unified-config-loader'; +import { loadOrCreateUnifiedConfig, mutateUnifiedConfig } from '../../config/unified-config-loader'; import { testConnection } from '../../cliproxy/remote-proxy-client'; import { isProxyRunning } from '../../cliproxy/services/proxy-lifecycle-service'; import { DEFAULT_BACKEND } from '../../cliproxy/platform-detector'; @@ -36,32 +36,32 @@ router.get('/', async (_req: Request, res: Response) => { /** * PUT /api/cliproxy-server - Update proxy configuration */ -router.put('/', async (req: Request, res: Response) => { +router.put('/', (req: Request, res: Response) => { try { - const config = await loadOrCreateUnifiedConfig(); const updates = req.body as Partial; - // Deep merge with defaults and current config - config.cliproxy_server = { - remote: { - ...DEFAULT_CLIPROXY_SERVER_CONFIG.remote, - ...config.cliproxy_server?.remote, - ...updates.remote, - }, - fallback: { - ...DEFAULT_CLIPROXY_SERVER_CONFIG.fallback, - ...config.cliproxy_server?.fallback, - ...updates.fallback, - }, - local: { - ...DEFAULT_CLIPROXY_SERVER_CONFIG.local, - ...config.cliproxy_server?.local, - ...updates.local, - }, - }; + // Atomic read-modify-write — avoids race between load and save + const updated = mutateUnifiedConfig((config) => { + config.cliproxy_server = { + remote: { + ...DEFAULT_CLIPROXY_SERVER_CONFIG.remote, + ...config.cliproxy_server?.remote, + ...updates.remote, + }, + fallback: { + ...DEFAULT_CLIPROXY_SERVER_CONFIG.fallback, + ...config.cliproxy_server?.fallback, + ...updates.fallback, + }, + local: { + ...DEFAULT_CLIPROXY_SERVER_CONFIG.local, + ...config.cliproxy_server?.local, + ...updates.local, + }, + }; + }); - await saveUnifiedConfig(config); - res.json(config.cliproxy_server); + res.json(updated.cliproxy_server); } catch (error) { console.error('[cliproxy-server-routes] Failed to save proxy config:', error); res.status(500).json({ error: 'Failed to save proxy config' }); @@ -91,7 +91,7 @@ router.get('/backend', async (_req: Request, res: Response) => { * @throws {400} Invalid backend value * @throws {409} Proxy is running (unless force=true) */ -router.put('/backend', async (req: Request, res: Response) => { +router.put('/backend', (req: Request, res: Response) => { try { const { backend, force } = req.body; if (backend !== 'original' && backend !== 'plus') { @@ -99,9 +99,9 @@ router.put('/backend', async (req: Request, res: Response) => { return; } - // Check if proxy is running - warn about restart requirement - const config = await loadOrCreateUnifiedConfig(); - const currentBackend = config.cliproxy?.backend ?? DEFAULT_BACKEND; + // Pre-flight read: check running state before acquiring write lock + const currentConfig = loadOrCreateUnifiedConfig(); + const currentBackend = currentConfig.cliproxy?.backend ?? DEFAULT_BACKEND; if (currentBackend !== backend && isProxyRunning() && !force) { res.status(409).json({ error: 'Proxy is running. Stop proxy first or use force=true to change backend.', @@ -110,18 +110,21 @@ router.put('/backend', async (req: Request, res: Response) => { }); return; } - if (!config.cliproxy) { - config.cliproxy = { - backend, - oauth_accounts: {}, - providers: [...CLIPROXY_PROVIDER_IDS], - variants: {}, - }; - } else { - config.cliproxy.backend = backend; - } - await saveUnifiedConfig(config); + // Atomic write — avoids race between load and save + mutateUnifiedConfig((config) => { + if (!config.cliproxy) { + config.cliproxy = { + backend, + oauth_accounts: {}, + providers: [...CLIPROXY_PROVIDER_IDS], + variants: {}, + }; + } else { + config.cliproxy.backend = backend; + } + }); + res.json({ backend }); } catch (error) { console.error('[cliproxy-server-routes] Failed to save backend config:', error); diff --git a/src/web-server/routes/route-helpers.ts b/src/web-server/routes/route-helpers.ts index 4f5c048e..2b38e486 100644 --- a/src/web-server/routes/route-helpers.ts +++ b/src/web-server/routes/route-helpers.ts @@ -4,6 +4,7 @@ import * as fs from 'fs'; import * as path from 'path'; +import type { Response } from 'express'; import { getCcsDir, getConfigPath, loadConfigSafe, loadSettings } from '../../utils/config-manager'; import { expandPath } from '../../utils/helpers'; import { getClaudeSettingsPath } from '../../utils/claude-config-path'; @@ -16,6 +17,7 @@ import { } from '../../cliproxy/model-id-normalizer'; import type { CLIProxyProvider } from '../../cliproxy/types'; import type { Config, Settings } from '../../types/config'; +import type { TargetType } from '../../targets/target-adapter'; import { ValidationError } from '../../errors/error-types'; /** Model mapping for API profiles */ @@ -389,3 +391,58 @@ export function validateFilePath(filePath: string): { return { valid: false, readonly: false, error: 'Access to this path is not allowed' }; } + +/** + * Parse and validate a target param (claude/droid). Returns null if invalid/absent. + * Shared by profile-routes and variant-routes. + */ +export function parseTarget(rawTarget: unknown): TargetType | null { + if (rawTarget === undefined || rawTarget === null || rawTarget === '') { + return null; + } + + if (typeof rawTarget !== 'string') { + return null; + } + + const normalized = rawTarget.trim().toLowerCase(); + if (normalized === 'claude' || normalized === 'droid') { + return normalized; + } + + return null; +} + +/** + * Create route-specific error helpers with a log prefix. + * Eliminates duplicate logRouteError/respondInternalError in each route file. + */ +export function createRouteErrorHelpers(prefix: string): { + logRouteError: (context: string, error: unknown) => void; + respondInternalError: ( + res: Response, + error: unknown, + fallbackMessage: string, + statusCode?: number + ) => void; +} { + function logRouteError(context: string, error: unknown): void { + if (error instanceof Error) { + console.error(`[${prefix}] ${context}: ${error.message}`); + return; + } + console.error(`[${prefix}] ${context}: unknown error`); + } + + function respondInternalError( + res: Response, + error: unknown, + fallbackMessage: string, + statusCode = 500 + ): void { + logRouteError(fallbackMessage, error); + res.status(statusCode).json({ error: fallbackMessage }); + } + + return { logRouteError, respondInternalError }; +} diff --git a/src/web-server/routes/settings-routes.ts b/src/web-server/routes/settings-routes.ts index 4d6a7af7..69dc878e 100644 --- a/src/web-server/routes/settings-routes.ts +++ b/src/web-server/routes/settings-routes.ts @@ -32,6 +32,7 @@ import { extractProviderFromPathname, getDeniedModelIdReasonForProvider, } from '../../cliproxy/model-id-normalizer'; +import { createRouteErrorHelpers } from './route-helpers'; const router = Router(); const MODEL_ENV_KEYS = [ @@ -42,23 +43,7 @@ const MODEL_ENV_KEYS = [ ] as const; const PRESET_MODEL_KEYS = ['default', 'opus', 'sonnet', 'haiku'] as const; -function logRouteError(context: string, error: unknown): void { - if (error instanceof Error) { - console.error(`[settings-routes] ${context}: ${error.message}`); - return; - } - console.error(`[settings-routes] ${context}: unknown error`); -} - -function respondInternalError( - res: Response, - error: unknown, - fallbackMessage: string, - statusCode = 500 -): void { - logRouteError(fallbackMessage, error); - res.status(statusCode).json({ error: fallbackMessage }); -} +const { logRouteError, respondInternalError } = createRouteErrorHelpers('settings-routes'); function isLoopbackAddress(value: string | undefined): boolean { if (!value) return false; diff --git a/src/web-server/routes/variant-routes.ts b/src/web-server/routes/variant-routes.ts index 65a4c07b..7e9013ef 100644 --- a/src/web-server/routes/variant-routes.ts +++ b/src/web-server/routes/variant-routes.ts @@ -7,7 +7,7 @@ import { Router, Request, Response } from 'express'; import { isReservedName, RESERVED_PROFILE_NAMES } from '../../config/reserved-names'; import type { CLIProxyProvider } from '../../cliproxy/types'; -import type { TargetType } from '../../targets/target-adapter'; +import { parseTarget } from './route-helpers'; import { createVariant, removeVariant, @@ -24,23 +24,6 @@ import { const router = Router(); -export function parseTarget(rawTarget: unknown): TargetType | null { - if (rawTarget === undefined || rawTarget === null || rawTarget === '') { - return null; - } - - if (typeof rawTarget !== 'string') { - return null; - } - - const normalized = rawTarget.trim().toLowerCase(); - if (normalized === 'claude' || normalized === 'droid') { - return normalized; - } - - return null; -} - /** * GET /api/cliproxy - List cliproxy variants * Uses variant-service for consistent behavior with CLI