diff --git a/AGENTS.md b/AGENTS.md index bb96a1b20b..9125c81bde 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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. diff --git a/docs/architecture/RESILIENCE_GUIDE.md b/docs/architecture/RESILIENCE_GUIDE.md index 0048761e60..d2744a0e81 100644 --- a/docs/architecture/RESILIENCE_GUIDE.md +++ b/docs/architecture/RESILIENCE_GUIDE.md @@ -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. diff --git a/src/lib/tokenHealthCheck.ts b/src/lib/tokenHealthCheck.ts index dd039399c0..8b04b488d6 100644 --- a/src/lib/tokenHealthCheck.ts +++ b/src/lib/tokenHealthCheck.ts @@ -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 { + const psd = conn?.providerSpecificData; + return typeof psd === "object" && psd !== null ? psd : {}; +} + +function withExpiredRetry( + psd: Record, + count: number, + at: string +): Record { + return { ...psd, expiredRetry: { count, at } }; +} + +function withClearedExpiredRetry( + psd: Record +): Record { + 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 | null | undefined ): Record | 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; } diff --git a/tests/unit/token-health-check-retry-deactivation.test.ts b/tests/unit/token-health-check-retry-deactivation.test.ts new file mode 100644 index 0000000000..2a99b35e65 --- /dev/null +++ b/tests/unit/token-health-check-retry-deactivation.test.ts @@ -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[0], + init as Parameters[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 = {}) { + 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 | 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 | 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 | 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[0], + init as Parameters[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; + } +});