fix: address PR review — timeouts, pagination safety, standardized cooldowns

- 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
This commit is contained in:
Chris Staley
2026-04-02 15:48:36 -06:00
parent b1183c2c9d
commit a069df41b8
5 changed files with 23 additions and 6 deletions

View File

@@ -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, {

View File

@@ -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, {

View File

@@ -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();

View File

@@ -701,6 +701,7 @@ export async function GET(
let pageUrl = url;
let pageCount = 0;
const MAX_PAGES = 20; // Safety limit
const seenTokens = new Set<string>();
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}`;

View File

@@ -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)`