mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-05 23:02:10 +03:00
fix(auth): stop retrying unrecoverable token refresh failures
Propagate invalid refresh-token errors instead of collapsing them to null so callers can distinguish expired credentials from transient failures. Mark affected connections as expired and inactive when refresh fails with an unrecoverable error, and persist that state during credential updates. Add tests covering retry bail-out and Claude/Codex refresh error handling.
This commit is contained in:
@@ -1190,12 +1190,16 @@ export class CodexExecutor extends BaseExecutor {
|
||||
return null;
|
||||
}
|
||||
const result = await getAccessToken("codex", credentials, log);
|
||||
if (!result || result.error) {
|
||||
if (!result) {
|
||||
log?.warn?.("TOKEN_REFRESH", "Codex: token refresh failed — re-authentication required");
|
||||
return null;
|
||||
}
|
||||
if (result.error) {
|
||||
log?.warn?.(
|
||||
"TOKEN_REFRESH",
|
||||
`Codex: token refresh failed${result?.error ? ` (${result.error})` : ""} — re-authentication required`
|
||||
`Codex: token refresh failed (${result.error}) — re-authentication required`
|
||||
);
|
||||
return null;
|
||||
return result;
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
@@ -13,7 +13,7 @@ import { ensureStreamReadiness } from "../utils/streamReadiness.ts";
|
||||
import { createStreamController, pipeWithDisconnect } from "../utils/streamHandler.ts";
|
||||
import { createSseHeartbeatTransform, shapeForClientFormat } from "../utils/sseHeartbeat.ts";
|
||||
import { addBufferToUsage, filterUsageForFormat, estimateUsage } from "../utils/usageTracking.ts";
|
||||
import { refreshWithRetry } from "../services/tokenRefresh.ts";
|
||||
import { refreshWithRetry, isUnrecoverableRefreshError } from "../services/tokenRefresh.ts";
|
||||
import { createRequestLogger } from "../utils/requestLogger.ts";
|
||||
import { getModelTargetFormat, PROVIDER_ID_TO_ALIAS } from "../config/providerModels.ts";
|
||||
import {
|
||||
@@ -3471,6 +3471,9 @@ export async function handleChatCore({
|
||||
}
|
||||
} else {
|
||||
log?.warn?.("TOKEN", `${provider.toUpperCase()} | refresh failed`);
|
||||
if (isUnrecoverableRefreshError(newCredentials) && onCredentialsRefreshed) {
|
||||
await onCredentialsRefreshed({ testStatus: "expired", isActive: false });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4134,7 +4137,10 @@ export async function handleChatCore({
|
||||
try {
|
||||
const firstChoice = translatedResponse?.choices?.[0];
|
||||
const msg = firstChoice?.message;
|
||||
cacheReasoningFromAssistantMessage(msg, provider, model, { requestId: skillRequestId, messageIndex: 0 });
|
||||
cacheReasoningFromAssistantMessage(msg, provider, model, {
|
||||
requestId: skillRequestId,
|
||||
messageIndex: 0,
|
||||
});
|
||||
} catch {
|
||||
// Cache capture is non-critical — never block the response
|
||||
}
|
||||
@@ -4421,7 +4427,10 @@ export async function handleChatCore({
|
||||
const body = streamResponseBody as Record<string, unknown>;
|
||||
const choices = body.choices as { message?: Record<string, unknown> }[] | undefined;
|
||||
const msg = choices?.[0]?.message;
|
||||
cacheReasoningFromAssistantMessage(msg, provider, model, { requestId: skillRequestId, messageIndex: 0 });
|
||||
cacheReasoningFromAssistantMessage(msg, provider, model, {
|
||||
requestId: skillRequestId,
|
||||
messageIndex: 0,
|
||||
});
|
||||
} catch {
|
||||
// Cache capture is non-critical — never block the stream
|
||||
}
|
||||
|
||||
@@ -346,11 +346,20 @@ export async function refreshClaudeOAuthToken(refreshToken, log, proxyConfig: un
|
||||
);
|
||||
|
||||
if (!response.ok) {
|
||||
const errorText = await response.text();
|
||||
let errorBody: { error?: string; error_description?: string } = {};
|
||||
try {
|
||||
errorBody = await response.json();
|
||||
} catch {
|
||||
const text = await response.text().catch(() => "unknown");
|
||||
errorBody = { error: text };
|
||||
}
|
||||
log?.error?.("TOKEN_REFRESH", "Failed to refresh Claude OAuth token", {
|
||||
status: response.status,
|
||||
error: errorText,
|
||||
error: errorBody,
|
||||
});
|
||||
if (errorBody.error === "invalid_grant" || errorBody.error === "invalid_request") {
|
||||
return { error: errorBody.error, code: `http_${response.status}` };
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -1277,6 +1286,13 @@ export async function refreshWithRetry(
|
||||
|
||||
try {
|
||||
const result = await withTimeout(refreshFn, REFRESH_TIMEOUT_MS);
|
||||
if (isUnrecoverableRefreshError(result)) {
|
||||
log?.warn?.(
|
||||
"TOKEN_REFRESH",
|
||||
`Unrecoverable refresh error for ${provider}: ${result.error} — skipping retries`
|
||||
);
|
||||
return result;
|
||||
}
|
||||
if (result) {
|
||||
recordSuccess(provider);
|
||||
return result;
|
||||
|
||||
@@ -334,7 +334,8 @@ export async function executeChatWithBreaker({
|
||||
// apiKey blob mid-request — forward it so the DB credential
|
||||
// doesn't go stale after Set-Cookie rotation.
|
||||
apiKey: newCreds.apiKey,
|
||||
testStatus: "active",
|
||||
testStatus: newCreds.testStatus ?? "active",
|
||||
isActive: newCreds.isActive,
|
||||
});
|
||||
},
|
||||
onRequestSuccess: async () => {
|
||||
|
||||
@@ -141,6 +141,9 @@ export async function updateProviderCredentials(connectionId: string, newCredent
|
||||
if (newCredentials.testStatus) {
|
||||
updates.testStatus = newCredentials.testStatus;
|
||||
}
|
||||
if (newCredentials.isActive !== undefined) {
|
||||
updates.isActive = newCredentials.isActive;
|
||||
}
|
||||
|
||||
const result = await updateProviderConnection(connectionId, updates);
|
||||
log.info("TOKEN_REFRESH", "Credentials updated in localDb", {
|
||||
|
||||
@@ -1219,6 +1219,24 @@ test("CodexExecutor.refreshCredentials refreshes OAuth tokens and returns null w
|
||||
}
|
||||
});
|
||||
|
||||
test("CodexExecutor.refreshCredentials propagates unrecoverable error object instead of returning null", async () => {
|
||||
const executor = new CodexExecutor();
|
||||
const originalFetch = globalThis.fetch;
|
||||
globalThis.fetch = async () =>
|
||||
new Response(
|
||||
JSON.stringify({ error: "invalid_grant", error_description: "Refresh token expired" }),
|
||||
{ status: 400, headers: { "Content-Type": "application/json" } }
|
||||
);
|
||||
|
||||
try {
|
||||
const result = await executor.refreshCredentials({ refreshToken: "dead-token" }, null);
|
||||
assert.ok(result !== null, "should return error object, not null");
|
||||
assert.equal((result as any).error, "unrecoverable_refresh_error");
|
||||
} finally {
|
||||
globalThis.fetch = originalFetch;
|
||||
}
|
||||
});
|
||||
|
||||
test("CodexExecutor maps usage_limit_reached websocket failures without explicit status to 429", () => {
|
||||
const raw = JSON.stringify({
|
||||
type: "response.failed",
|
||||
|
||||
@@ -1121,3 +1121,82 @@ test("getAccessToken per-connection mutex: mutex cleared after success, next cal
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
// ─── Unrecoverable error bail-out tests ──────────────────────────────────────
|
||||
|
||||
test("refreshWithRetry bails immediately on unrecoverable error without retrying", async () => {
|
||||
const provider = `bail-unrecoverable-${Date.now()}`;
|
||||
const log = createLog();
|
||||
let callCount = 0;
|
||||
|
||||
const result = await refreshWithRetry(
|
||||
async () => {
|
||||
callCount++;
|
||||
return { error: "unrecoverable_refresh_error", code: "http_400" };
|
||||
},
|
||||
3,
|
||||
log,
|
||||
provider
|
||||
);
|
||||
|
||||
assert.equal(callCount, 1, "should only call refreshFn once (no retries)");
|
||||
assert.deepEqual(result, { error: "unrecoverable_refresh_error", code: "http_400" });
|
||||
const warnMessages = log.entries.filter((e) => e.level === "warn").map((e) => e.message);
|
||||
assert.ok(
|
||||
warnMessages.some((m) => String(m).includes("Unrecoverable")),
|
||||
"should log an unrecoverable warning"
|
||||
);
|
||||
});
|
||||
|
||||
test("refreshWithRetry bails immediately on invalid_grant error without retrying", async () => {
|
||||
const provider = `bail-invalid-grant-${Date.now()}`;
|
||||
const log = createLog();
|
||||
let callCount = 0;
|
||||
|
||||
const result = await refreshWithRetry(
|
||||
async () => {
|
||||
callCount++;
|
||||
return { error: "invalid_grant", code: "http_400" };
|
||||
},
|
||||
3,
|
||||
log,
|
||||
provider
|
||||
);
|
||||
|
||||
assert.equal(callCount, 1, "should only call refreshFn once (no retries)");
|
||||
assert.deepEqual(result, { error: "invalid_grant", code: "http_400" });
|
||||
});
|
||||
|
||||
test("refreshClaudeOAuthToken returns error object for invalid_grant (expired refresh token)", async () => {
|
||||
const log = createLog();
|
||||
|
||||
await withMockedFetch(
|
||||
async () =>
|
||||
new Response(JSON.stringify({ error: "invalid_grant", error_description: "Token expired" }), {
|
||||
status: 400,
|
||||
headers: { "Content-Type": "application/json" },
|
||||
}),
|
||||
async () => {
|
||||
const result = await refreshClaudeOAuthToken("expired-token", log);
|
||||
assert.ok(result && typeof result === "object", "should return error object, not null");
|
||||
assert.equal((result as any).error, "invalid_grant");
|
||||
assert.ok(isUnrecoverableRefreshError(result), "should be detected as unrecoverable");
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
test("refreshClaudeOAuthToken returns null for transient server errors (not unrecoverable)", async () => {
|
||||
const log = createLog();
|
||||
|
||||
await withMockedFetch(
|
||||
async () =>
|
||||
new Response(JSON.stringify({ error: "server_error" }), {
|
||||
status: 503,
|
||||
headers: { "Content-Type": "application/json" },
|
||||
}),
|
||||
async () => {
|
||||
const result = await refreshClaudeOAuthToken("some-token", log);
|
||||
assert.equal(result, null, "transient server errors should return null (retryable)");
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user