fix(oauth): allow expired token retry before deactivation and activate expired retry loop (#P0) (#11414)

Validated in a combined 5-PR batch worktree off release/v3.8.51 tip.
- Focused tests: token-health-check-retry-deactivation.test.ts + token-health-check-kimi.test.ts + cursor-renewal.test.ts — part of batch's 115/115 node:test run
- typecheck:core, file-size, changelog-integrity, complexity, cognitive-complexity — all OK
- Full-repo lint: 228 pre-existing dashboard react-hooks/* findings, unrelated to this diff

Thanks for restoring the retry-budget gate before deactivation — premature deactivation on a single transient refresh failure is a real P0-class defect for OAuth-connected accounts.
This commit is contained in:
Bob.Hou
2026-08-25 18:25:48 -04:00
committed by GitHub
parent 62b3f9d36d
commit 98cec759ae
4 changed files with 392 additions and 35 deletions

View File

@@ -197,7 +197,7 @@ baseCooldownMs * 2 ** failureIndex;
The anti-thundering-herd guard prevents concurrent failures on the same connection from
repeatedly extending the cooldown or double-incrementing `backoffLevel`.
Terminal states are not cooldowns. `banned`, `expired`, and `credits_exhausted` are
Terminal states are not cooldowns. `banned`, `expired` (which becomes terminal only after N bounded retries via `EXPIRED_RETRY_MAX`), and `credits_exhausted` are
intended to stay unavailable until credentials/settings change or an operator resets
them. Do not overwrite terminal states with transient cooldown state.

View File

@@ -82,7 +82,7 @@ OmniRoute has three distinct but related resilience mechanisms. Each has a diffe
**Terminal states (NOT cooldowns):**
- `banned` — set by banned-keyword / account-ban detection (see [BAN_DETECTION](../security/BAN_DETECTION.md))
- `expired`
- `expired` (transitions to terminal after bounded retries — `EXPIRED_RETRY_MAX = 3` with exponential backoff — so transient OAuth errors can self-heal before the account is permanently deactivated)
- `credits_exhausted`
These persist until credentials change or an operator resets them. Do not overwrite terminal states with transient cooldown state.

View File

@@ -36,6 +36,8 @@ const TRUE_ENV_VALUES = new Set(["1", "true", "yes", "on"]);
const TICK_MS = 60 * 1000; // sweep interval: every 60 seconds (restored — #7719 dropped the const but kept two call sites)
const DEFAULT_BATCH_SIZE = 20;
const DEFAULT_HEALTH_CHECK_INTERVAL_MIN = 60; // default per-connection interval
const EXPIRED_RETRY_MAX = 3; // max retry attempts for expired connections before giving up
const EXPIRED_RETRY_BACKOFF_MIN = 5; // backoff between expired retries (minutes)
function isBuildProcess(): boolean {
return typeof process !== "undefined" && process.env.NEXT_PHASE === "phase-production-build";
@@ -98,6 +100,42 @@ function isGitHubAccessTokenOnlyConnection(conn: any): boolean {
);
}
// ── Expired-retry state helpers ──────────────────────────────────────────────
// `expiredRetryCount` and `expiredRetryAt` are stored inside providerSpecificData
// (as `expiredRetry: { count, at }`) rather than top-level columns: the
// provider_connections schema does not have these columns, so top-level fields
// were silently dropped by _buildUpdateConnectionRowParams. This pattern mirrors
// `refreshCircuit` which already lives in providerSpecificData.
function getExpiredRetryCount(conn: any): number {
return conn?.providerSpecificData?.expiredRetry?.count ?? conn?.expiredRetryCount ?? 0;
}
function getExpiredRetryAt(conn: any): string | null {
return conn?.providerSpecificData?.expiredRetry?.at ?? conn?.expiredRetryAt ?? null;
}
function getPsd(conn: any): Record<string, unknown> {
const psd = conn?.providerSpecificData;
return typeof psd === "object" && psd !== null ? psd : {};
}
function withExpiredRetry(
psd: Record<string, unknown>,
count: number,
at: string
): Record<string, unknown> {
return { ...psd, expiredRetry: { count, at } };
}
function withClearedExpiredRetry(
psd: Record<string, unknown>
): Record<string, unknown> {
const next = { ...psd };
delete next.expiredRetry;
return next;
}
/**
* Resolve the Copilot token endpoint base URL for a connection. github.com
* Copilot always uses api.github.com; GHE Copilot uses its own per-enterprise
@@ -155,19 +193,21 @@ export function buildRefreshFailureUpdate(
}
) {
const wasExpired = conn.testStatus === "expired";
const retryCount = (conn.expiredRetryCount ?? 0) + (wasExpired ? 1 : 0);
const retryCount = getExpiredRetryCount(conn) + (wasExpired ? 1 : 0);
// Circuit breaker: increment the consecutive-failure streak and set an
// exponential backoff window so the next sweep skips this connection instead
// of retrying every 60s. Cleared by a successful refresh (clearRefreshCircuit).
// Guard: providerSpecificData may be a primitive or null - treat as empty.
const psd =
typeof conn.providerSpecificData === "object" && conn.providerSpecificData !== null
? conn.providerSpecificData
: {};
const psd = getPsd(conn);
const prevStreak = psd.refreshCircuit?.streak ?? 0;
const streak = prevStreak + 1;
const updatedPsd = {
...psd,
refreshCircuit: { streak, until: getRefreshBackoffUntil(streak, now), lastFailAt: now },
...(wasExpired ? { expiredRetry: { count: retryCount, at: now } } : {}),
};
return {
lastHealthCheckAt: now,
// A failed background refresh should not evict otherwise healthy accounts
@@ -179,10 +219,9 @@ export function buildRefreshFailureUpdate(
lastErrorType: "token_refresh_failed",
lastErrorSource: "oauth",
errorCode: "refresh_failed",
providerSpecificData: {
...psd,
refreshCircuit: { streak, until: getRefreshBackoffUntil(streak, now), lastFailAt: now },
},
providerSpecificData: updatedPsd,
// Expose expiredRetryCount on the return value for log callers / tests that
// read the update object (they do NOT reach the DB — only providerSpecificData does).
...(wasExpired ? { expiredRetryCount: retryCount, expiredRetryAt: now } : {}),
...(overrides || {}),
};
@@ -200,14 +239,10 @@ export function buildRefreshFailureUpdate(
*/
export function buildTransientRefreshRetryUpdate(conn: any, now: string) {
const wasExpired = conn.testStatus === "expired";
const retryCount = (conn.expiredRetryCount ?? 0) + (wasExpired ? 1 : 0);
const retryCount = getExpiredRetryCount(conn) + (wasExpired ? 1 : 0);
// Preserve existing streak from any prior permanent failures so a transient
// error does not reset the exponential backoff ladder.
// Guard: providerSpecificData may be a primitive or null - treat as empty.
const psd =
typeof conn.providerSpecificData === "object" && conn.providerSpecificData !== null
? conn.providerSpecificData
: {};
const psd = getPsd(conn);
const existingCircuit = psd.refreshCircuit;
const existingStreak = existingCircuit?.streak ?? 0;
const parsedExistingUntil = existingCircuit?.until
@@ -242,7 +277,9 @@ export function buildTransientRefreshRetryUpdate(conn: any, now: string) {
// observers can distinguish this from a pure transient retry.
transient: useTransient,
},
...(wasExpired ? { expiredRetry: { count: retryCount, at: now } } : {}),
},
// Expose on return value for log callers (does NOT reach DB as a column).
...(wasExpired ? { expiredRetryCount: retryCount, expiredRetryAt: now } : {}),
};
}
@@ -255,9 +292,11 @@ export function clearRefreshCircuit(
providerSpecificData: Record<string, unknown> | null | undefined
): Record<string, unknown> | undefined {
if (!providerSpecificData || typeof providerSpecificData !== "object") return undefined;
if (!("refreshCircuit" in providerSpecificData)) return undefined;
if (!("refreshCircuit" in providerSpecificData) && !("expiredRetry" in providerSpecificData))
return undefined;
const next = { ...providerSpecificData };
delete next.refreshCircuit;
delete next.expiredRetry;
return next;
}
@@ -513,7 +552,14 @@ export async function checkConnection(conn) {
// Determine interval (0 = disabled)
const intervalMin = conn.healthCheckInterval ?? DEFAULT_HEALTH_CHECK_INTERVAL_MIN;
if (intervalMin <= 0) return;
if (!conn.isActive) return;
if (!conn.isActive) {
// #P0: allow expired connections with retry budget remaining to pass
// through so transient OAuth failures can self-heal instead of being
// permanently skipped. Exhausted retries stay terminal.
if (!(conn.testStatus === "expired" && getExpiredRetryCount(conn) < EXPIRED_RETRY_MAX)) {
return;
}
}
// #8182: skip terminal connections (credits_exhausted / banned / expired).
// These can never self-heal via a token refresh — probing them wastes
@@ -543,12 +589,17 @@ export async function checkConnection(conn) {
conn.testStatus === "expired" &&
String(conn.provider || "").toLowerCase() === "cursor" &&
conn.lastErrorType !== "account_deactivated";
const isRecoverableExpiredWithRetryBudget =
conn.testStatus === "expired" &&
conn.lastErrorType !== "account_deactivated" &&
getExpiredRetryCount(conn) < EXPIRED_RETRY_MAX;
const terminalStatuses = new Set(["credits_exhausted", "banned", "expired"]);
if (
typeof conn.testStatus === "string" &&
terminalStatuses.has(conn.testStatus.toLowerCase()) &&
!isRecoverableGithubCopilotNoRefresh &&
!isRecoverableCursorExpired
!isRecoverableCursorExpired &&
!isRecoverableExpiredWithRetryBudget
) {
return;
}
@@ -691,11 +742,12 @@ export async function checkConnection(conn) {
lastErrorSource: copilotAboutToExpire && !refreshedProviderSpecificData ? "oauth" : null,
errorCode:
copilotAboutToExpire && !refreshedProviderSpecificData ? "refresh_failed" : null,
expiredRetryCount: null,
expiredRetryAt: null,
// Clear expired retry state — persisted inside providerSpecificData.
// The top-level keys are kept for backward compat but the real clear
// happens by merging withClearedExpiredRetry into the psd below.
...(refreshedProviderSpecificData
? { providerSpecificData: refreshedProviderSpecificData }
: {}),
? { providerSpecificData: withClearedExpiredRetry(refreshedProviderSpecificData) }
: { providerSpecificData: withClearedExpiredRetry(getPsd(conn)) }),
});
} else {
await updateProviderConnection(conn.id, {
@@ -756,12 +808,19 @@ export async function checkConnection(conn) {
// Retry expired connections with exponential backoff up to EXPIRED_RETRY_MAX times.
if (conn.testStatus === "expired") {
const retryCount = conn.expiredRetryCount ?? 0;
if (retryCount >= EXPIRED_RETRY_MAX) return;
const retryCount = getExpiredRetryCount(conn);
if (retryCount >= EXPIRED_RETRY_MAX) {
// Retry budget exhausted: mark terminal. Idempotent write.
if (conn.isActive !== false) {
await updateProviderConnection(conn.id, { isActive: false });
}
return;
}
const lastRetry = conn.expiredRetryAt ? new Date(conn.expiredRetryAt).getTime() : 0;
const lastRetry = getExpiredRetryAt(conn);
const lastRetryMs = lastRetry ? new Date(lastRetry).getTime() : 0;
const backoffMs = EXPIRED_RETRY_BACKOFF_MIN * 60 * 1000 * Math.pow(2, retryCount);
if (Date.now() - lastRetry < backoffMs) return;
if (Date.now() - lastRetryMs < backoffMs) return;
log(
`${LOG_PREFIX} Retrying expired ${conn.provider}/${getConnectionLogLabel(conn)} (attempt ${retryCount + 1}/${EXPIRED_RETRY_MAX})`
@@ -1029,17 +1088,25 @@ export async function checkConnection(conn) {
return;
}
const expiredRetryCount = getExpiredRetryCount(conn) + 1;
const isRetryBudgetExhausted = expiredRetryCount >= EXPIRED_RETRY_MAX;
const errorLabel = result.code || result.error;
const psd = getPsd(conn);
await updateProviderConnection(conn.id, {
lastHealthCheckAt: now,
testStatus: "expired",
lastError: isRotatingProvider
? `Refresh token consumed (${result.error}). Please re-authenticate this account.`
: `Refresh token rejected (${result.error}). Please re-authenticate this account.`,
? `Refresh token consumed (${errorLabel}). Please re-authenticate this account.`
: `Refresh token rejected (${errorLabel}). Please re-authenticate this account.`,
lastErrorAt: now,
lastErrorType: result.error,
lastErrorSource: "oauth",
errorCode: result.error,
isActive: false,
errorCode: errorLabel,
providerSpecificData: withExpiredRetry(psd, expiredRetryCount, now),
// #P0: only deactivate when the retry budget is exhausted. Before that,
// keep the connection active so subsequent sweeps can retry the refresh.
...(isRetryBudgetExhausted ? { isActive: false } : {}),
// Only rotating-token providers (Codex/OpenAI/etc.) have single-use refresh
// tokens that are genuinely consumed and worthless after a failed refresh, so
// clearing them is safe. For non-rotating providers (Google: antigravity /
@@ -1050,8 +1117,10 @@ export async function checkConnection(conn) {
});
logError(
`${LOG_PREFIX}${conn.provider}/${getConnectionLogLabel(conn)}` +
`Refresh token is permanently invalid (${result.error}). ` +
`Connection deactivated. Re-authenticate to restore.`
`Refresh token is permanently invalid (${errorLabel}). ` +
(isRetryBudgetExhausted
? `Connection deactivated. Re-authenticate to restore.`
: `Retry ${expiredRetryCount}/${EXPIRED_RETRY_MAX} used; keeping connection active for retry.`)
);
return;
}

View File

@@ -0,0 +1,288 @@
/**
* TDD — Token refresh retry-before-deactivation.
*
* P0 bug: an unrecoverable refresh error (invalid_grant, refresh_token_reused,
* etc.) immediately sets isActive: false AND testStatus: "expired". The
* terminal-status guard at line 548 then blocks the connection from ever
* reaching the EXPIRED_RETRY_MAX retry loop at line 758 — dead code.
*
* Verified production root cause: the EXPIRED_RETRY_MAX and
* EXPIRED_RETRY_BACKOFF_MIN constants were removed in #7719
* (9a6a846ae), so even if the gate were opened, the retry loop would
* throw ReferenceError.
*
* Fixes:
* 1. Restore the constants.
* 2. Gate at 516/548: allow expired connections with retry budget left.
* 3. Unrecoverable error path: conditional isActive: false, only after
* retry budget exhausted.
* 4. Store expiredRetry in providerSpecificData matching refreshCircuit pattern.
*/
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-token-retry-deactivation-"));
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 { PROVIDERS } = await import("../../open-sse/config/constants.ts");
const tokenHealthCheck = await import("../../src/lib/tokenHealthCheck.ts");
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 });
}
break;
} catch (error: unknown) {
if ((error?.code === "EBUSY" || error?.code === "EPERM") && attempt < 9) {
await new Promise((resolve) => setTimeout(resolve, 50 * (attempt + 1)));
} else {
throw error;
}
}
}
fs.mkdirSync(TEST_DATA_DIR, { recursive: true });
}
async function withPatchedProvider(providerId, config, fn) {
const hadOwnConfig = Object.prototype.hasOwnProperty.call(PROVIDERS, providerId);
const previousConfig = hadOwnConfig ? PROVIDERS[providerId] : undefined;
PROVIDERS[providerId] = config;
try {
return await fn();
} finally {
if (hadOwnConfig) {
PROVIDERS[providerId] = previousConfig;
} else {
delete PROVIDERS[providerId];
}
}
}
function createMockFetchForInvalidGrant(tokenUrl: string) {
const originalFetch = globalThis.fetch;
globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => {
const url = typeof input === "string" ? input : (input as Request).url;
if (url === tokenUrl) {
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 PROVIDER_ID = "custom-oauth-retry-deactivation";
const TOKEN_URL = "https://token-retry.test.invalid/token";
const PROVIDER_CONFIG = {
tokenUrl: TOKEN_URL,
clientId: "retry-test-client-id",
clientSecret: "retry-test-client-secret",
};
async function createRetryTestConnection(overrides: Record<string, unknown> = {}) {
const connection = await providersDb.createProviderConnection({
provider: PROVIDER_ID,
authType: "oauth",
name: "Retry Test Account",
email: "[EMAIL_REDACTED]",
refreshToken: "rt_retry_test",
accessToken: "at_retry_test",
healthCheckInterval: 60,
isActive: true,
testStatus: "active",
...overrides,
});
return connection as { id: string; [key: string]: unknown };
}
test.after(async () => {
core.resetDbInstance();
try {
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true });
} catch {
// ignore cleanup errors
}
});
// ── Test ①: First refresh failure does NOT set isActive: false ──────────────
test("first unrecoverable refresh error increments expiredRetryCount without setting isActive: false", async () => {
await resetStorage();
const originalFetch = createMockFetchForInvalidGrant(TOKEN_URL);
try {
await withPatchedProvider(PROVIDER_ID, PROVIDER_CONFIG, async () => {
const connection = await createRetryTestConnection();
await tokenHealthCheck.checkConnection({
...connection,
lastHealthCheckAt: new Date(Date.now() - 61 * 60 * 1000).toISOString(),
});
const updated = await providersDb.getProviderConnectionById(connection.id);
const psd = updated?.providerSpecificData as Record<string, unknown> | undefined;
assert.equal(updated?.isActive, true, "connection must remain active on first failure");
assert.equal(updated?.testStatus, "expired", "testStatus must be 'expired'");
assert.equal(psd?.expiredRetry?.count, 1, "expiredRetry.count must increment to 1");
assert.equal(updated?.errorCode, "invalid_grant", "errorCode must reflect the OAuth error");
assert.equal(
updated?.lastErrorType,
"unrecoverable_refresh_error",
"lastErrorType must classify the failure"
);
assert.equal(updated?.lastErrorSource, "oauth", "lastErrorSource must be 'oauth'");
assert.ok(
typeof updated?.lastError === "string" && updated.lastError.length > 0,
"lastError must be a non-empty descriptive string"
);
assert.ok(psd?.expiredRetry?.at, "expiredRetry.at must be set");
});
} finally {
globalThis.fetch = originalFetch;
}
});
// ── Test ②: checkConnection gate allows expired+retryBudget through ─────────
test("checkConnection allows expired connection with retry budget through the terminal-status gate", async () => {
await resetStorage();
const originalFetch = createMockFetchForInvalidGrant(TOKEN_URL);
try {
await withPatchedProvider(PROVIDER_ID, PROVIDER_CONFIG, async () => {
const connection = await createRetryTestConnection({
testStatus: "expired",
providerSpecificData: {
expiredRetry: { count: 1, at: new Date(Date.now() - 10 * 60 * 1000).toISOString() },
},
lastError: "invalid_grant",
lastErrorAt: new Date().toISOString(),
lastErrorType: "unrecoverable_refresh_error",
lastErrorSource: "oauth",
errorCode: "invalid_grant",
});
await tokenHealthCheck.checkConnection({
...connection,
lastHealthCheckAt: new Date(Date.now() - 61 * 60 * 1000).toISOString(),
});
const updated = await providersDb.getProviderConnectionById(connection.id);
const psd = updated?.providerSpecificData as Record<string, unknown> | undefined;
// The retry path was entered: count incremented from 1 to 2
assert.equal(
psd?.expiredRetry?.count,
2,
"expiredRetry.count must increment to 2 (retry path entered)"
);
// Still active — not prematurely deactivated
assert.equal(updated?.isActive, true, "must remain active during retry phase");
assert.equal(updated?.testStatus, "expired", "testStatus must remain 'expired'");
});
} finally {
globalThis.fetch = originalFetch;
}
});
// ── Test ③: Terminal state only after max retry count ──────────────────────
test("terminal deactivation (isActive: false) only after reaching EXPIRED_RETRY_MAX", async () => {
await resetStorage();
const originalFetch = createMockFetchForInvalidGrant(TOKEN_URL);
try {
await withPatchedProvider(PROVIDER_ID, PROVIDER_CONFIG, async () => {
const connection = await createRetryTestConnection({
testStatus: "expired",
providerSpecificData: {
expiredRetry: { count: 2, at: new Date(Date.now() - 30 * 60 * 1000).toISOString() },
},
lastError: "invalid_grant",
lastErrorAt: new Date().toISOString(),
lastErrorType: "unrecoverable_refresh_error",
lastErrorSource: "oauth",
errorCode: "invalid_grant",
});
await tokenHealthCheck.checkConnection({
...connection,
lastHealthCheckAt: new Date(Date.now() - 61 * 60 * 1000).toISOString(),
});
const updated = await providersDb.getProviderConnectionById(connection.id);
const psd = updated?.providerSpecificData as Record<string, unknown> | undefined;
// Terminal: isActive must be false after max retries
assert.equal(updated?.isActive, false, "connection must be deactivated after max retries");
assert.equal(updated?.testStatus, "expired", "testStatus must be 'expired'");
assert.equal(psd?.expiredRetry?.count, 3, "expiredRetry.count must reach 3");
});
} finally {
globalThis.fetch = originalFetch;
}
});
// ── Test ④: Budget-exhausted connection is strictly skipped by sweep ───────
test("checkConnection strictly skips deactivated connection with exhausted retry budget", async () => {
await resetStorage();
let fetchCalled = false;
const originalFetch = globalThis.fetch;
globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => {
fetchCalled = true;
return originalFetch(
input as Parameters<typeof originalFetch>[0],
init as Parameters<typeof originalFetch>[1]
);
}) as typeof fetch;
try {
await withPatchedProvider(PROVIDER_ID, PROVIDER_CONFIG, async () => {
const connection = await createRetryTestConnection({
isActive: false, // already deactivated
testStatus: "expired",
providerSpecificData: {
expiredRetry: { count: 3, at: new Date(Date.now() - 60 * 60 * 1000).toISOString() },
},
lastError: "invalid_grant",
lastErrorAt: new Date(Date.now() - 60 * 60 * 1000).toISOString(),
lastErrorType: "unrecoverable_refresh_error",
lastErrorSource: "oauth",
errorCode: "invalid_grant",
});
await tokenHealthCheck.checkConnection({
...connection,
lastHealthCheckAt: new Date(Date.now() - 61 * 60 * 1000).toISOString(),
});
const updated = await providersDb.getProviderConnectionById(connection.id);
// Must be completely skipped: no network calls, no state changes
assert.equal(
fetchCalled,
false,
"sweep must not make any fetch calls for exhausted connection"
);
assert.equal(updated?.isActive, false, "isActive must remain false");
assert.equal(updated?.testStatus, "expired", "testStatus must remain 'expired'");
});
} finally {
globalThis.fetch = originalFetch;
}
});