From 08f03a0fa6493c26abc4425b661d75a9e7ac0d92 Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Sat, 16 May 2026 19:12:58 -0300 Subject: [PATCH] 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. --- open-sse/executors/codex.ts | 10 ++- open-sse/handlers/chatCore.ts | 15 ++++- open-sse/services/tokenRefresh.ts | 20 +++++- src/sse/handlers/chatHelpers.ts | 3 +- src/sse/services/tokenRefresh.ts | 3 + tests/unit/executor-codex.test.ts | 18 ++++++ tests/unit/token-refresh-service.test.ts | 79 ++++++++++++++++++++++++ 7 files changed, 139 insertions(+), 9 deletions(-) diff --git a/open-sse/executors/codex.ts b/open-sse/executors/codex.ts index b68ed08c6b..4270d8a869 100644 --- a/open-sse/executors/codex.ts +++ b/open-sse/executors/codex.ts @@ -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; } diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 7146188bc2..249463352d 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -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; const choices = body.choices as { message?: Record }[] | 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 } diff --git a/open-sse/services/tokenRefresh.ts b/open-sse/services/tokenRefresh.ts index 33f0848756..7f5964fece 100755 --- a/open-sse/services/tokenRefresh.ts +++ b/open-sse/services/tokenRefresh.ts @@ -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; diff --git a/src/sse/handlers/chatHelpers.ts b/src/sse/handlers/chatHelpers.ts index ec4dc2774a..b9d34d1b8e 100644 --- a/src/sse/handlers/chatHelpers.ts +++ b/src/sse/handlers/chatHelpers.ts @@ -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 () => { diff --git a/src/sse/services/tokenRefresh.ts b/src/sse/services/tokenRefresh.ts index 244eb70558..ad9b77e484 100755 --- a/src/sse/services/tokenRefresh.ts +++ b/src/sse/services/tokenRefresh.ts @@ -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", { diff --git a/tests/unit/executor-codex.test.ts b/tests/unit/executor-codex.test.ts index 092320f075..41e093c8a7 100644 --- a/tests/unit/executor-codex.test.ts +++ b/tests/unit/executor-codex.test.ts @@ -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", diff --git a/tests/unit/token-refresh-service.test.ts b/tests/unit/token-refresh-service.test.ts index 8dfb5e93fe..339e912056 100644 --- a/tests/unit/token-refresh-service.test.ts +++ b/tests/unit/token-refresh-service.test.ts @@ -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)"); + } + ); +});