mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-17 20:32:25 +03:00
fix(oauth): soft-fail Claude refresh so CredentialHealth is not sticky-dead (#13185)
Merged after boarding with #13426 into one worktree cut from `release/v3.8.51` (both verified as ancestors of the combined HEAD before validating).
**Evidence**
- Your own test plus **every sibling** in the module — 14 files across `tokenHealthCheck*`, `token-health-check*`, `credential-health*` and `issue-13470-token-refresh-proxy-bypass`: **72/72 pass** on the combined tree. Running the siblings and not just the PR's own file is deliberate: this PR changes sweep-path state that several of those files exercise independently.
- Gates: `check-changelog-integrity` PASS, `check-complexity` PASS (2842 vs baseline 3218), `check-cognitive-complexity` PASS (1284 vs 1437), `typecheck:core` PASS.
**Reconciled — one real gate violation, fixed in your branch (d9164886)**
`check-file-size` genuinely tripped on this PR: `src/lib/tokenHealthCheck.ts` goes 1214 → 1221, past a frozen ceiling of 1218 that had only 4 lines of headroom. Attributed by measuring both sides, not assumed — the tip is at 1214 with no violation. Rebaselined the ceiling to 1221 with a dated justification key, since the growth *is* the fix: preserving the `refresh_token` and telling a transient failure apart from a dead credential needs extra state on the sweep path that cannot leave the module without breaking its internal API.
One thing that looked like your problem and is not, recorded so nobody re-raises it: `open-sse/executors/codex.ts: 1529 > 1528` shows up when the gate runs on your branch. Your branch carries an older merge of the release where that file was longer; the tip has it at 1524, your diff never touches it, and it does not survive the squash.
Thanks, @RaviTharuma — a sticky-dead `CredentialHealth` is the worst failure mode here, because a transient refresh blip permanently parks a working account and nothing ever retries it. Driving the real provider through two consecutive sweeps and re-reading the DB row in between is the right way to prove the state actually clears.
This commit is contained in:
3
changelog.d/fixes/claude-oauth-sticky-refresh.md
Normal file
3
changelog.d/fixes/claude-oauth-sticky-refresh.md
Normal file
@@ -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)
|
||||
@@ -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,
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)} — ` +
|
||||
|
||||
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