mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-19 21:32:20 +03:00
fix(oauth): keep the Claude refresh token so the retry budget is reachable
An unrecoverable Claude OAuth refresh (invalid_grant / refresh_token_reused, often a dual-consumer race on the same Claude Max account) left the connection sticky-dead: active, expired and unable to recover without a manual re-auth. #11414 already keeps such a connection retryable for EXPIRED_RETRY_MAX sweeps, but the very same update ran `refreshToken: null` for every rotating provider, Claude included. The next sweep then stops at the `!conn.refreshToken` guard, whose self-heal branch only fires while testStatus is empty or "active" — the row is already "expired", so checkConnection returns silently and the retry budget is never spent. The #11414 regression test missed this because it drives a synthetic provider that is not in ROTATING_REFRESH_PROVIDERS. - Claude opts out of clearing the rotating refresh token on the unrecoverable path, for the same reason #3679 preserves it for non-rotating providers: it is the user's only recovery artifact. Codex and the other rotating providers keep clearing their genuinely consumed one-time-use tokens. - CredentialHealth's sweep now honors the refresh circuit and parks the next attempt on the circuit deadline instead of re-probing a connection whose token refresh is already backing off. - isInRefreshBackoff moves to src/lib/tokenRefreshCircuit.ts so CredentialHealth can use it without importing tokenHealthCheck's auto-starting scheduler; tokenHealthCheck re-exports it for existing callers. Regression test drives the real `claude` provider through two consecutive sweeps: without the fix the token is gone after the first and the second sweep never spends retry 2. A Codex case guards the unchanged behavior. Closes #13183
This commit is contained in:
committed by
diegosouzapw
parent
81bf3cc36e
commit
1683d57749
@@ -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<void> {
|
||||
provider: string;
|
||||
authType?: string;
|
||||
healthCheckInterval?: number | null;
|
||||
providerSpecificData?: { refreshCircuit?: { until?: string } } | null;
|
||||
}>;
|
||||
|
||||
try {
|
||||
@@ -348,6 +350,7 @@ export async function sweep(): Promise<void> {
|
||||
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<void> {
|
||||
// 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;
|
||||
|
||||
@@ -30,6 +30,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"]);
|
||||
@@ -173,12 +177,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,
|
||||
@@ -1126,7 +1128,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)} — ` +
|
||||
|
||||
40
src/lib/tokenRefreshCircuit.ts
Normal file
40
src/lib/tokenRefreshCircuit.ts
Normal file
@@ -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());
|
||||
}
|
||||
@@ -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<typeof originalFetch>[0],
|
||||
init as Parameters<typeof originalFetch>[1]
|
||||
);
|
||||
}) as typeof fetch;
|
||||
return originalFetch;
|
||||
}
|
||||
|
||||
const EXPIRED_ISO = new Date(Date.now() - 60 * 60 * 1000).toISOString();
|
||||
|
||||
async function createOAuthConnection(provider: string, overrides: Record<string, unknown> = {}) {
|
||||
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<string, unknown>),
|
||||
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<string, unknown>),
|
||||
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);
|
||||
});
|
||||
Reference in New Issue
Block a user