diff --git a/changelog.d/fixes/claude-oauth-sticky-refresh.md b/changelog.d/fixes/claude-oauth-sticky-refresh.md new file mode 100644 index 0000000000..548160d700 --- /dev/null +++ b/changelog.d/fixes/claude-oauth-sticky-refresh.md @@ -0,0 +1,3 @@ +- **fix(oauth):** stop nulling the Claude refresh token on the first unrecoverable refresh + failure so `CredentialHealth` no longer gets stuck sticky-dead — the retry budget from #11414 + can now actually spend its second attempt instead of finding an already-cleared token (#13183) diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index 9133ad7cad..872d6116ca 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -1,4 +1,5 @@ { + "_rebaseline_2026_09_17_13185_claude_oauth_sticky_refresh": "PR #13185 (@RaviTharuma): soft-fail do refresh do Claude para CredentialHealth nao ficar sticky-dead. src/lib/tokenHealthCheck.ts 1214 (tip) -> 1220 na branch e 1221 na arvore combinada com #13426; teto fixado em 1221. O teto anterior (1218) tinha apenas 4 linhas de folga. O crescimento e o proprio fix: preservar o refresh_token e distinguir falha transitoria de credencial morta exige estado extra no caminho de sweep, que nao pode sair do modulo sem quebrar a API interna. Coberto por tests/unit/tokenHealthCheck-claude-refresh-token-preserved.test.ts; os 14 arquivos irmaos de tokenHealthCheck/credentialHealth foram rodados juntos (72/72).", "_rebaseline_2026_09_16_jxnlexn_wave_growth": "Combined growth of the 2026-09-15 maxmad64bis uplift batch (each PR rebaselined its own growth; the merged sum is larger): src/sse/handlers/chat.ts->2498; open-sse/handlers/chatCore.ts->6181. Every hunk is flag-gated or a verified fix covered by that PR's tests; see the batch report.", "_rebaseline_2026_09_16_wave22_growth": "Combined growth of the 2026-09-15 maxmad64bis uplift batch (each PR rebaselined its own growth; the merged sum is larger): src/sse/handlers/chatHelpers.ts->1231; open-sse/executors/cursor.ts->1808. Every hunk is flag-gated or a verified fix covered by that PR's tests; see the batch report.", "_rebaseline_2026_09_15_13572_combined_growth": "Combined growth of the 2026-09-15 maxmad64bis uplift batch (each PR rebaselined its own growth; the merged sum is larger): open-sse/executors/base.ts->1754. Every hunk is flag-gated or a verified fix covered by that PR's tests; see the batch report.", @@ -497,7 +498,7 @@ "src/lib/db/core.ts": 1788, "src/lib/db/migrationRunner.ts": 1206, "src/lib/tailscaleTunnel.ts": 1208, - "src/lib/tokenHealthCheck.ts": 1218, + "src/lib/tokenHealthCheck.ts": 1221, "src/shared/components/RequestLoggerV2.tsx": 1718, "src/shared/constants/providers/apikey/gateways.ts": 1502, "src/shared/services/cliRuntime.ts": 1296, diff --git a/src/lib/credentialHealth/scheduler.ts b/src/lib/credentialHealth/scheduler.ts index dbbeb1d842..51507ed9e9 100644 --- a/src/lib/credentialHealth/scheduler.ts +++ b/src/lib/credentialHealth/scheduler.ts @@ -27,6 +27,7 @@ import { isCredentialProbeInconclusive, resolveInconclusiveProbeRecheckDelayMs, } from "@/lib/credentialHealth/probePolicy"; +import { isInRefreshBackoff } from "@/lib/tokenRefreshCircuit"; import { emit } from "@/lib/events/eventBus"; import { isAutomatedTestProcess } from "@/shared/utils/testProcess"; import { SEARCH_VALIDATOR_CONFIGS } from "@/lib/providers/validation/searchProviders"; @@ -331,6 +332,7 @@ export async function sweep(): Promise { provider: string; authType?: string; healthCheckInterval?: number | null; + providerSpecificData?: { refreshCircuit?: { until?: string } } | null; }>; try { @@ -348,6 +350,7 @@ export async function sweep(): Promise { provider: string; authType?: string; healthCheckInterval?: number | null; + providerSpecificData?: { refreshCircuit?: { until?: string } } | null; }>; } catch (err) { console.error(LOG_PREFIX, "Failed to load provider connections:", err); @@ -364,6 +367,20 @@ export async function sweep(): Promise { // Per-connection opt-out: never tested. if (intervalMs === null) return false; const state_ = getSchedulerState(); + // Honor the OAuth refresh circuit (#13183): probing a connection whose token + // refresh is already in backoff just re-reports the same failure every sweep + // and keeps the dashboard red until the window expires or the user re-auths. + // Park the next attempt on the circuit's own deadline instead. + if (isInRefreshBackoff(conn, now)) { + const untilMs = new Date( + String(conn.providerSpecificData?.refreshCircuit?.until) + ).getTime(); + state_.perConnTiming.set(conn.id, { + lastAttemptAt: state_.perConnTiming.get(conn.id)?.lastAttemptAt ?? now, + nextAttemptAt: untilMs, + }); + return false; + } const timing = state_.perConnTiming.get(conn.id); // No timing entry = never tested since boot → due now if (!timing) return true; diff --git a/src/lib/tokenHealthCheck.ts b/src/lib/tokenHealthCheck.ts index ddeaa1cf5f..774ab27e8d 100644 --- a/src/lib/tokenHealthCheck.ts +++ b/src/lib/tokenHealthCheck.ts @@ -31,6 +31,10 @@ import { checkWebCookieConnectionIfNeeded, isWebCookieHealthProbeCandidate, } from "@/lib/tokenHealthCheckWebCookie"; +import { + isInRefreshBackoff, + preservesRefreshTokenOnUnrecoverable, +} from "@/lib/tokenRefreshCircuit"; const LOG_PREFIX = "[HealthCheck]"; const TRUE_ENV_VALUES = new Set(["1", "true", "yes", "on"]); @@ -174,12 +178,10 @@ export function getRefreshBackoffUntil(streak: number, now: string): string { return new Date(new Date(now).getTime() + backoffMin * 60 * 1000).toISOString(); } -export function isInRefreshBackoff(conn: any, nowMs: number): boolean { - const until = conn?.providerSpecificData?.refreshCircuit?.until; - if (typeof until !== "string") return false; - const untilMs = new Date(until).getTime(); - return Number.isFinite(untilMs) && untilMs > nowMs; -} +// Both live in `@/lib/tokenRefreshCircuit` so CredentialHealth can import them +// without pulling this module's auto-starting scheduler. Re-exported for +// existing callers and tests. +export { isInRefreshBackoff, preservesRefreshTokenOnUnrecoverable }; export function buildRefreshFailureUpdate( conn: any, @@ -1129,7 +1131,11 @@ export async function checkConnection(conn) { // gemini) the stored refresh_token is the user's only recovery // artifact — nulling it caused #3679 (the connection reports "No valid refresh // token available" and can never recover even after re-activation). Preserve it. - ...(isRotatingProvider ? { refreshToken: null } : {}), + // PRESERVE_REFRESH_TOKEN_PROVIDERS (Claude) opt out too: nulling on the first + // failure makes the #11414 retry budget above unreachable (#13183). + ...(isRotatingProvider && !preservesRefreshTokenOnUnrecoverable(conn.provider) + ? { refreshToken: null } + : {}), }); logError( `${LOG_PREFIX} ✗ ${conn.provider}/${getConnectionLogLabel(conn)} — ` + diff --git a/src/lib/tokenRefreshCircuit.ts b/src/lib/tokenRefreshCircuit.ts new file mode 100644 index 0000000000..89a7a3a5ca --- /dev/null +++ b/src/lib/tokenRefreshCircuit.ts @@ -0,0 +1,40 @@ +/** + * Shared refresh-policy helpers for Token Health Check + CredentialHealth. + * + * Kept tiny and dependency-free on purpose: CredentialHealth's sweep needs to + * honor the OAuth refresh backoff window, but importing `tokenHealthCheck` + * would pull in its module-level scheduler (which auto-starts timers). + * + * Moved verbatim out of `src/lib/tokenHealthCheck.ts`, which re-exports it for + * existing callers and tests. + */ + +export function isInRefreshBackoff(conn: any, nowMs: number): boolean { + const until = conn?.providerSpecificData?.refreshCircuit?.until; + if (typeof until !== "string") return false; + const untilMs = new Date(until).getTime(); + return Number.isFinite(untilMs) && untilMs > nowMs; +} + +/** + * Rotating-refresh providers whose refresh token must survive an "unrecoverable" + * refresh error instead of being nulled on the first failure. + * + * #11414 keeps the connection active for EXPIRED_RETRY_MAX retries, but the same + * update also nulled the refresh token for every rotating provider. The next sweep + * then hits the `!conn.refreshToken` guard, whose self-heal branch only fires while + * testStatus is empty or "active" — the row is already "expired", so the sweep + * returns silently and the retry budget is never spent. The connection stays active, + * expired and unrecoverable until a manual re-auth (#13183). + * + * Claude access tokens are short-lived (~8h) and an invalid_grant / + * refresh_token_reused is frequently a dual-consumer race (the same Claude Max + * account refreshed by another OAuth client), not a confirmed revoke — so the token + * is worth keeping for the retries. Same reasoning as #3679 for non-rotating + * providers: the stored refresh token is the user's only recovery artifact. + */ +const PRESERVE_REFRESH_TOKEN_PROVIDERS = new Set(["claude"]); + +export function preservesRefreshTokenOnUnrecoverable(provider: unknown): boolean { + return PRESERVE_REFRESH_TOKEN_PROVIDERS.has(String(provider || "").toLowerCase()); +} diff --git a/tests/unit/tokenHealthCheck-claude-refresh-token-preserved.test.ts b/tests/unit/tokenHealthCheck-claude-refresh-token-preserved.test.ts new file mode 100644 index 0000000000..4d2a5ca350 --- /dev/null +++ b/tests/unit/tokenHealthCheck-claude-refresh-token-preserved.test.ts @@ -0,0 +1,210 @@ +/** + * TDD — #13183: a Claude OAuth refresh failure must not make the #11414 retry + * budget unreachable. + * + * #11414 keeps an unrecoverable refresh failure retryable: the connection stays + * active with testStatus "expired" until EXPIRED_RETRY_MAX attempts are spent. + * The same update, however, ran `refreshToken: null` for every rotating provider + * — Claude included. On the next sweep `checkConnection` hits the + * `!conn.refreshToken` guard, whose self-heal branch only fires while testStatus + * is empty or "active"; the row is already "expired", so the sweep returns + * silently. The connection sits active, expired and unrecoverable until a manual + * re-auth — the sticky-dead report in #13183. + * + * The existing #11414 regression test never caught it: it drives a synthetic + * provider that is NOT in ROTATING_REFRESH_PROVIDERS, so the refresh token was + * never nulled there. + * + * Guards, with the real `claude` provider: + * ① first unrecoverable failure preserves the refresh token + * ② the second sweep therefore reaches the retry path (budget is spendable) + * ③ a rotating provider that does NOT opt in still gets its token cleared + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-claude-refresh-preserve-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.NODE_ENV = "test"; + +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const tokenHealthCheck = await import("../../src/lib/tokenHealthCheck.ts"); + +const ANTHROPIC_TOKEN_URL = "https://api.anthropic.com/v1/oauth/token"; +const CODEX_TOKEN_URL = "https://auth.openai.com/oauth/token"; + +async function resetStorage() { + core.resetDbInstance(); + for (let attempt = 0; attempt < 10; attempt++) { + try { + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } + break; + } catch (error: unknown) { + const code = (error as { code?: string })?.code; + if ((code === "EBUSY" || code === "EPERM") && attempt < 9) { + await new Promise((resolve) => setTimeout(resolve, 50 * (attempt + 1))); + } else { + throw error; + } + } + } + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +/** Answer every OAuth token endpoint with invalid_grant; pass everything else through. */ +function mockInvalidGrant() { + const originalFetch = globalThis.fetch; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : (input as Request).url; + if (url === ANTHROPIC_TOKEN_URL || url === CODEX_TOKEN_URL) { + return new Response(JSON.stringify({ error: "invalid_grant" }), { + status: 400, + headers: { "content-type": "application/json" }, + }); + } + return originalFetch( + input as Parameters[0], + init as Parameters[1] + ); + }) as typeof fetch; + return originalFetch; +} + +const EXPIRED_ISO = new Date(Date.now() - 60 * 60 * 1000).toISOString(); + +async function createOAuthConnection(provider: string, overrides: Record = {}) { + return (await providersDb.createProviderConnection({ + provider, + authType: "oauth", + name: `${provider} sticky-refresh account`, + email: "[EMAIL_REDACTED]", + refreshToken: `rt_${provider}_test`, + accessToken: `at_${provider}_test`, + // Expired access token: without it the rotating-provider sweep returns before + // ever attempting a refresh (refresh is expiry-driven, not interval-driven). + expiresAt: EXPIRED_ISO, + tokenExpiresAt: EXPIRED_ISO, + healthCheckInterval: 60, + isActive: true, + testStatus: "active", + ...overrides, + })) as { id: string; [key: string]: unknown }; +} + +test.after(async () => { + core.resetDbInstance(); + try { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } catch { + // ignore cleanup errors + } +}); + +// ── ① Claude keeps its refresh token on the first unrecoverable failure ────── +test("claude preserves refreshToken on the first unrecoverable refresh failure", async () => { + await resetStorage(); + const originalFetch = mockInvalidGrant(); + try { + const connection = await createOAuthConnection("claude"); + + await tokenHealthCheck.checkConnection({ + ...connection, + lastHealthCheckAt: new Date(Date.now() - 61 * 60 * 1000).toISOString(), + }); + + const updated = await providersDb.getProviderConnectionById(connection.id); + + assert.equal( + updated?.refreshToken, + "rt_claude_test", + "refresh token must survive — the retry budget cannot be spent without it" + ); + assert.equal(updated?.isActive, true, "connection stays active while retries remain"); + assert.equal(updated?.testStatus, "expired", "status reflects the expired token"); + } finally { + globalThis.fetch = originalFetch; + } +}); + +// ── ② The retry budget is actually reachable on the next sweep ─────────────── +// Two REAL consecutive sweeps, re-reading the row in between — the second sweep +// must see whatever the first one persisted. With the refresh token nulled the +// second sweep bails out at the `!conn.refreshToken` guard and the counter is +// stuck at 1 forever. +test("claude spends a second retry on the next sweep instead of returning silently", async () => { + await resetStorage(); + const originalFetch = mockInvalidGrant(); + try { + const created = await createOAuthConnection("claude"); + const staleCheck = new Date(Date.now() - 61 * 60 * 1000).toISOString(); + + await tokenHealthCheck.checkConnection({ ...created, lastHealthCheckAt: staleCheck }); + + const afterFirst = await providersDb.getProviderConnectionById(created.id); + const firstPsd = afterFirst?.providerSpecificData as + { expiredRetry?: { count?: number } } | undefined; + assert.equal(firstPsd?.expiredRetry?.count, 1, "first sweep spends retry 1"); + + // Backdate the retry timestamp so the exponential backoff window has elapsed. + await providersDb.updateProviderConnection(created.id, { + providerSpecificData: { + ...(afterFirst?.providerSpecificData as Record), + expiredRetry: { count: 1, at: new Date(Date.now() - 60 * 60 * 1000).toISOString() }, + }, + }); + + const beforeSecond = await providersDb.getProviderConnectionById(created.id); + await tokenHealthCheck.checkConnection({ + ...(beforeSecond as Record), + lastHealthCheckAt: staleCheck, + }); + + const afterSecond = await providersDb.getProviderConnectionById(created.id); + const secondPsd = afterSecond?.providerSpecificData as + { expiredRetry?: { count?: number } } | undefined; + + assert.equal( + secondPsd?.expiredRetry?.count, + 2, + "second sweep must spend retry 2 — a nulled refresh token makes it return early" + ); + } finally { + globalThis.fetch = originalFetch; + } +}); + +// ── ③ Rotating providers that do not opt in still get the token cleared ────── +test("codex still clears its single-use refresh token", async () => { + await resetStorage(); + const originalFetch = mockInvalidGrant(); + try { + const connection = await createOAuthConnection("codex"); + + await tokenHealthCheck.checkConnection({ + ...connection, + lastHealthCheckAt: new Date(Date.now() - 61 * 60 * 1000).toISOString(), + }); + + const updated = await providersDb.getProviderConnectionById(connection.id); + assert.ok( + !updated?.refreshToken, + "a consumed one-time-use Codex token is worthless and must still be cleared" + ); + } finally { + globalThis.fetch = originalFetch; + } +}); + +// ── Opt-in list is explicit ────────────────────────────────────────────────── +test("only claude opts out of clearing the rotating refresh token", () => { + assert.equal(tokenHealthCheck.preservesRefreshTokenOnUnrecoverable("claude"), true); + assert.equal(tokenHealthCheck.preservesRefreshTokenOnUnrecoverable("Claude"), true); + assert.equal(tokenHealthCheck.preservesRefreshTokenOnUnrecoverable("codex"), false); + assert.equal(tokenHealthCheck.preservesRefreshTokenOnUnrecoverable(undefined), false); +});