diff --git a/apps/backend/src/oauth/providers/base.test.ts b/apps/backend/src/oauth/providers/base.test.ts new file mode 100644 index 000000000..73d90a3b0 --- /dev/null +++ b/apps/backend/src/oauth/providers/base.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { isRetryableOAuthUserInfoError } from "./base"; + +describe("isRetryableOAuthUserInfoError", () => { + it("returns true for openid-client timeout errors", () => { + expect(isRetryableOAuthUserInfoError({ + name: "RPError", + message: "outgoing request timed out after 3500ms", + })).toBe(true); + }); + + it("returns true for retryable network error codes", () => { + expect(isRetryableOAuthUserInfoError({ + code: "ETIMEDOUT", + message: "socket hangup", + })).toBe(true); + }); + + it("returns true when retryable errors are wrapped in cause", () => { + expect(isRetryableOAuthUserInfoError({ + message: "request failed", + cause: { + name: "AbortError", + }, + })).toBe(true); + }); + + it("returns false for non-retryable OAuth errors", () => { + expect(isRetryableOAuthUserInfoError({ + error: "invalid_client", + message: "client credentials are invalid", + })).toBe(false); + }); +}); diff --git a/apps/backend/src/oauth/providers/base.tsx b/apps/backend/src/oauth/providers/base.tsx index c0fb8ee4d..ff1d5da15 100644 --- a/apps/backend/src/oauth/providers/base.tsx +++ b/apps/backend/src/oauth/providers/base.tsx @@ -5,6 +5,19 @@ import { mergeScopeStrings } from "@stackframe/stack-shared/dist/utils/strings"; import { CallbackParamsType, Client, Issuer, TokenSet as OIDCTokenSet, generators } from "openid-client"; import { OAuthUserInfo } from "../utils"; +const OAUTH_USERINFO_TOTAL_ATTEMPTS = 3; +const OAUTH_USERINFO_RETRY_DELAY_BASE_MS = 250; +const RETRYABLE_OAUTH_NETWORK_ERROR_CODES = new Set([ + "ETIMEDOUT", + "ECONNRESET", + "ECONNREFUSED", + "EAI_AGAIN", + "ENETUNREACH", + "UND_ERR_CONNECT_TIMEOUT", + "UND_ERR_HEADERS_TIMEOUT", + "UND_ERR_BODY_TIMEOUT", +]); + export type TokenSet = { accessToken: string, refreshToken?: string, @@ -12,6 +25,48 @@ export type TokenSet = { idToken?: string, }; +function getStringProperty(obj: unknown, key: string): string | undefined { + if (typeof obj !== "object" || obj === null || !(key in obj)) { + return undefined; + } + const value = Reflect.get(obj, key); + return typeof value === "string" ? value : undefined; +} + +function getUnknownProperty(obj: unknown, key: string): unknown { + if (typeof obj !== "object" || obj === null || !(key in obj)) { + return undefined; + } + return Reflect.get(obj, key); +} + +export function isRetryableOAuthUserInfoError(error: unknown): boolean { + const code = getStringProperty(error, "code"); + if (code && RETRYABLE_OAUTH_NETWORK_ERROR_CODES.has(code)) { + return true; + } + + const name = getStringProperty(error, "name"); + if (name === "AbortError" || name === "TimeoutError") { + return true; + } + + const message = getStringProperty(error, "message")?.toLowerCase(); + if (message?.includes("outgoing request timed out")) { + return true; + } + if (message?.includes("timed out")) { + return true; + } + + const cause = getUnknownProperty(error, "cause"); + if (cause !== undefined && cause !== error) { + return isRetryableOAuthUserInfoError(cause); + } + + return false; +} + function processTokenSet(providerName: string, tokenSet: OIDCTokenSet, defaultAccessTokenExpiresInMillis?: number): TokenSet { if (!tokenSet.access_token) { throw new StackAssertionError(`No access token received from ${providerName}.`, { tokenSet, providerName }); @@ -193,8 +248,29 @@ export abstract class OAuthBaseProvider { } tokenSet = processTokenSet(this.constructor.name, tokenSet, this.defaultAccessTokenExpiresInMillis); + const userInfoResult = await Result.retry(async () => { + try { + return Result.ok(await this.postProcessUserInfo(tokenSet)); + } catch (error) { + if (isRetryableOAuthUserInfoError(error)) { + return Result.error(error); + } + throw error; + } + }, OAUTH_USERINFO_TOTAL_ATTEMPTS, { + exponentialDelayBase: OAUTH_USERINFO_RETRY_DELAY_BASE_MS, + }); + + if (userInfoResult.status === "error") { + throw new StackAssertionError("Failed to fetch OAuth user info after retries.", { + attempts: userInfoResult.attempts, + provider: this.constructor.name, + cause: userInfoResult.error, + }); + } + return { - userInfo: await this.postProcessUserInfo(tokenSet), + userInfo: userInfoResult.data, tokenSet, }; } diff --git a/claude/CLAUDE-KNOWLEDGE.md b/claude/CLAUDE-KNOWLEDGE.md index 255e179af..d77751f26 100644 --- a/claude/CLAUDE-KNOWLEDGE.md +++ b/claude/CLAUDE-KNOWLEDGE.md @@ -175,3 +175,6 @@ A: `useUser()` filters out restricted users by default. In `packages/template/sr Q: Why can external-db-sync sequencer throw `operator does not exist: text = uuid` on team updates? A: In `apps/backend/src/app/api/latest/internal/external-db-sync/sequencer/route.ts`, the TEAM_INVITATION cascade compares JSON text (`"VerificationCode"."data"->>'team_id'`) against `"Team"."teamId"` (`uuid`). Cast the UUID side to text (`changed_teams."teamId"::text`) in the WHERE clause so Postgres type resolution succeeds and team-invitation re-sync marking works. + +Q: Why shouldn't OAuth callback retries wrap the whole `getCallback` flow? +A: The authorization code exchange (`oauthClient.callback` / `oauthCallback`) is effectively one-shot, so retrying the full callback can convert a transient downstream failure into `invalid_grant` on the next attempt. Retries should wrap only post-exchange user-info fetches (`postProcessUserInfo`) and only for transient network/timeout errors.