From a069df41b86136d3be7d3efe05f4f71517ed0523 Mon Sep 17 00:00:00 2001 From: Chris Staley Date: Thu, 2 Apr 2026 15:48:36 -0600 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20time?= =?UTF-8?q?outs,=20pagination=20safety,=20standardized=20cooldowns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add 30s AbortSignal timeout to sync-models fetch in progress dialog - Add duplicate nextPageToken detection to prevent infinite pagination - Standardize retry-after defaults to COOLDOWN_MS.rateLimit (120s) - Add connection metadata update (lastErrorType/lastError) for per-model lockout early return in auth.ts - Clarify lockModel race safety in single-threaded Node.js --- open-sse/handlers/chatCore.ts | 9 +++++---- open-sse/services/accountFallback.ts | 4 +++- src/app/(dashboard)/dashboard/providers/[id]/page.tsx | 1 + src/app/api/providers/[id]/models/route.ts | 6 ++++++ src/sse/services/auth.ts | 9 ++++++++- 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index b4e6ebe9b1..e2e7c1b8d3 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -15,6 +15,7 @@ import { getModelTargetFormat, PROVIDER_ID_TO_ALIAS } from "../config/providerMo import { resolveModelAlias } from "../services/modelDeprecation.ts"; import { getUnsupportedParams } from "../config/providerRegistry.ts"; import { hasPerModelQuota, lockModelIfPerModelQuota } from "../services/accountFallback.ts"; +import { COOLDOWN_MS } from "../config/constants.ts"; import { buildErrorBody, createErrorResult, @@ -1294,9 +1295,9 @@ export async function handleChatCore({ // For providers with per-model quotas (passthrough providers, Gemini), // each model has independent quota. A 429 on one model must NOT lock out // the entire connection — other models may still have quota available. - if (lockModelIfPerModelQuota(provider, connectionId, model, "rate_limited", retryAfterMs || 120_000)) { + if (lockModelIfPerModelQuota(provider, connectionId, model, "rate_limited", retryAfterMs || COOLDOWN_MS.rateLimit)) { console.warn( - `[provider] Node ${connectionId} model-only rate limited (${statusCode}) for ${model} - ${Math.ceil((retryAfterMs || 120_000) / 1000)}s (connection stays active)` + `[provider] Node ${connectionId} model-only rate limited (${statusCode}) for ${model} - ${Math.ceil((retryAfterMs || COOLDOWN_MS.rateLimit) / 1000)}s (connection stays active)` ); } else { const rateLimitedUntil = new Date(Date.now() + retryAfterMs).toISOString(); @@ -1315,9 +1316,9 @@ export async function handleChatCore({ } } else if (errorType === PROVIDER_ERROR_TYPES.QUOTA_EXHAUSTED) { // Providers with per-model quotas — lock the model only, not the connection - if (lockModelIfPerModelQuota(provider, connectionId, model, "quota_exhausted", retryAfterMs || 120_000)) { + if (lockModelIfPerModelQuota(provider, connectionId, model, "quota_exhausted", retryAfterMs || COOLDOWN_MS.rateLimit)) { console.warn( - `[provider] Node ${connectionId} model-only quota exhausted (${statusCode}) for ${model} - ${Math.ceil((retryAfterMs || 120_000) / 1000)}s (connection stays active)` + `[provider] Node ${connectionId} model-only quota exhausted (${statusCode}) for ${model} - ${Math.ceil((retryAfterMs || COOLDOWN_MS.rateLimit) / 1000)}s (connection stays active)` ); } else { await updateProviderConnection(connectionId, { diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index f0e4775325..d0c09e845e 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -101,7 +101,9 @@ export function lockModel(provider, connectionId, model, reason, cooldownMs) { ensureCleanupTimer(); const key = `${provider}:${connectionId}:${model}`; const newUntil = Date.now() + cooldownMs; - // Preserve the longer cooldown if an existing lock has more time remaining + // Preserve the longer cooldown if an existing lock has more time remaining. + // Safe without a mutex: no await between get/set, so this runs atomically + // within Node.js's single-threaded event loop. const existing = modelLockouts.get(key); if (existing && existing.until > newUntil) return; modelLockouts.set(key, { diff --git a/src/app/(dashboard)/dashboard/providers/[id]/page.tsx b/src/app/(dashboard)/dashboard/providers/[id]/page.tsx index e6572d7c6a..c6b1c29350 100644 --- a/src/app/(dashboard)/dashboard/providers/[id]/page.tsx +++ b/src/app/(dashboard)/dashboard/providers/[id]/page.tsx @@ -1114,6 +1114,7 @@ export default function ProviderDetailPage() { try { const syncRes = await fetch(`/api/providers/${newConnection.id}/sync-models`, { method: "POST", + signal: AbortSignal.timeout(30_000), // 30s timeout — model sync shouldn't hang }); const syncData = await syncRes.json(); diff --git a/src/app/api/providers/[id]/models/route.ts b/src/app/api/providers/[id]/models/route.ts index c5ec50161d..8b91c356bc 100644 --- a/src/app/api/providers/[id]/models/route.ts +++ b/src/app/api/providers/[id]/models/route.ts @@ -701,6 +701,7 @@ export async function GET( let pageUrl = url; let pageCount = 0; const MAX_PAGES = 20; // Safety limit + const seenTokens = new Set(); while (pageUrl && pageCount < MAX_PAGES) { pageCount++; @@ -721,6 +722,11 @@ export async function GET( const nextPageToken = data.nextPageToken; if (!nextPageToken) break; + if (seenTokens.has(nextPageToken)) { + console.warn(`[models] ${provider}: duplicate nextPageToken detected, stopping pagination`); + break; + } + seenTokens.add(nextPageToken); pageUrl = `${config.url}${config.url.includes("?") ? "&" : "?"}pageToken=${encodeURIComponent(nextPageToken)}`; if (config.authQuery) { pageUrl += `&${config.authQuery}=${token}`; diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index 26e8b02593..76e5ee1aa9 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -734,8 +734,15 @@ export async function markAccountUnavailable( const reason = status === 404 ? "not_found" : "rate_limited"; const cooldown = status === 404 ? COOLDOWN_MS.notFoundLocal - : (COOLDOWN_MS.rateLimit || 60_000); + : COOLDOWN_MS.rateLimit; lockModel(provider, connectionId, model, reason, cooldown); + // Update last error for observability (without changing terminal status) + updateProviderConnection(connectionId, { + lastErrorType: reason, + lastError: `Model ${model} ${reason}`, + lastErrorAt: new Date().toISOString(), + errorCode: status, + }).catch(() => {}); log.info( "AUTH", `Model-only lockout for ${provider}:${model} — ${status} ${reason} ${Math.ceil(cooldown / 1000)}s (connection stays active)`