From 006de710d59d4c4d6f70817bc87051b661590b88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Kondratek?= <19799111+mkondratek@users.noreply.github.com> Date: Thu, 27 Aug 2026 22:10:00 +0200 Subject: [PATCH] fix(sdk): don't log out Codex/OCA users when token refresh fails transiently (#13565) * fix(sdk): don't log out Codex/OCA users when token refresh fails transiently Port the cline-provider refresh semantics to openai-codex and oca: a transient refresh failure (network error, timeout, server 5xx) with an already-expired access token now rethrows instead of returning null. A null return means the refresh token was REJECTED and re-auth is required; treating an outage blip as a rejection is what turned it into a forced 'openai-codex requires re-authentication.' task stop while the settings UI still showed the user as signed in. Both providers also emit user.auth_refresh_soft_failure telemetry on transient failures (the 'prevented logout' counter the cline provider already has) and attach status/errorCode details to the genuine invalid_grant logout event. * refactor: collapse duplicate soft-failure telemetry branches and test Review feedback: compute tokenExpired once and emit the soft-failure event once in both providers, then return current credentials or rethrow. Fold the codex soft-failure telemetry assertions into the existing still-usable-token test instead of a near-duplicate case. --- sdk/packages/core/src/auth/codex.test.ts | 86 ++++++++++++++++++++++++ sdk/packages/core/src/auth/codex.ts | 35 +++++++++- sdk/packages/core/src/auth/oca.test.ts | 71 +++++++++++++++++++ sdk/packages/core/src/auth/oca.ts | 28 +++++++- 4 files changed, 214 insertions(+), 6 deletions(-) diff --git a/sdk/packages/core/src/auth/codex.test.ts b/sdk/packages/core/src/auth/codex.test.ts index bae94deef3..f7be15bfd2 100644 --- a/sdk/packages/core/src/auth/codex.test.ts +++ b/sdk/packages/core/src/auth/codex.test.ts @@ -128,12 +128,27 @@ describe("auth/codex token lifecycle", () => { ), ); + const capture = vi.fn(); const current = createCredentials({ expires: 150_000 }); const result = await getValidOpenAICodexCredentials(current, { refreshBufferMs: 60_000, retryableTokenGraceMs: 30_000, + telemetry: { capture } as never, }); expect(result).toBe(current); + expect(capture).toHaveBeenCalledWith( + expect.objectContaining({ + event: "user.auth_refresh_soft_failure", + properties: expect.objectContaining({ + provider: "openai-codex", + status: 500, + tokenExpired: false, + }), + }), + ); + expect(capture).not.toHaveBeenCalledWith( + expect.objectContaining({ event: "user.auth_logged_out" }), + ); nowSpy.mockRestore(); }); @@ -153,4 +168,75 @@ describe("auth/codex token lifecycle", () => { "Failed to refresh OpenAI Codex token", ); }); + + it("throws on transient refresh error when the token is already expired", async () => { + // A server error landing after expiry is NOT an invalid grant; returning + // null here is what turned an outage blip into a forced + // "requires re-authentication" stop. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000); + vi.stubGlobal( + "fetch", + vi.fn( + async () => + new Response( + JSON.stringify({ + error: "server_error", + error_description: "temporary issue", + }), + { status: 500, headers: { "Content-Type": "application/json" } }, + ), + ), + ); + + const capture = vi.fn(); + await expect( + getValidOpenAICodexCredentials(createCredentials({ expires: 90_000 }), { + telemetry: { capture } as never, + }), + ).rejects.toThrow("Token refresh failed: 500"); + expect(capture).toHaveBeenCalledWith( + expect.objectContaining({ + event: "user.auth_refresh_soft_failure", + properties: expect.objectContaining({ + provider: "openai-codex", + status: 500, + tokenExpired: true, + }), + }), + ); + expect(capture).not.toHaveBeenCalledWith( + expect.objectContaining({ event: "user.auth_logged_out" }), + ); + nowSpy.mockRestore(); + }); + + it("throws on a network-level refresh failure when the token is already expired", async () => { + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new TypeError("fetch failed"); + }), + ); + + const capture = vi.fn(); + await expect( + getValidOpenAICodexCredentials(createCredentials({ expires: 90_000 }), { + telemetry: { capture } as never, + }), + ).rejects.toThrow("Failed to refresh OpenAI Codex token"); + expect(capture).toHaveBeenCalledWith( + expect.objectContaining({ + event: "user.auth_refresh_soft_failure", + properties: expect.objectContaining({ + provider: "openai-codex", + tokenExpired: true, + }), + }), + ); + expect(capture).not.toHaveBeenCalledWith( + expect.objectContaining({ event: "user.auth_logged_out" }), + ); + nowSpy.mockRestore(); + }); }); diff --git a/sdk/packages/core/src/auth/codex.ts b/sdk/packages/core/src/auth/codex.ts index 91f1a10a56..85ff1ecbb0 100644 --- a/sdk/packages/core/src/auth/codex.ts +++ b/sdk/packages/core/src/auth/codex.ts @@ -10,6 +10,7 @@ import { nanoid } from "nanoid"; import { captureAuthFailed, captureAuthLoggedOut, + captureAuthRefreshSoftFailure, captureAuthStarted, captureAuthSucceeded, identifyAccount, @@ -423,16 +424,44 @@ export async function getValidOpenAICodexCredentials( ); return refreshed; } catch (error) { + const failureDetails = { + status: + error instanceof OpenAICodexOAuthTokenError ? error.status : undefined, + errorCode: + error instanceof OpenAICodexOAuthTokenError + ? error.errorCode + : undefined, + errorName: error instanceof Error ? error.name : undefined, + }; if ( error instanceof OpenAICodexOAuthTokenError && error.isLikelyInvalidGrant() ) { - captureAuthLoggedOut(options?.telemetry, "openai-codex", "invalid_grant"); + captureAuthLoggedOut( + options?.telemetry, + "openai-codex", + "invalid_grant", + { + status: error.status, + errorCode: error.errorCode, + }, + ); return null; } - if (currentCredentials.expires - Date.now() > retryableTokenGraceMs) { + const tokenExpired = + currentCredentials.expires - Date.now() <= retryableTokenGraceMs; + captureAuthRefreshSoftFailure(options?.telemetry, "openai-codex", { + ...failureDetails, + tokenExpired, + }); + if (!tokenExpired) { return currentCredentials; } - return null; + // Rethrow instead of returning null. A null from this function means the + // refresh token was REJECTED (re-auth required); a network blip or server + // error that happens to land after expiry must not be mistaken for that — + // callers turn null into a forced "requires re-authentication" stop even + // though the very next refresh attempt would likely succeed. + throw error; } } diff --git a/sdk/packages/core/src/auth/oca.test.ts b/sdk/packages/core/src/auth/oca.test.ts index 6f886ac814..a8d8f3e6ec 100644 --- a/sdk/packages/core/src/auth/oca.test.ts +++ b/sdk/packages/core/src/auth/oca.test.ts @@ -213,6 +213,77 @@ describe("auth/oca getValidOcaCredentials", () => { nowSpy.mockRestore(); }); + it("throws on transient refresh error when the token is already expired", async () => { + // A server error landing after expiry is NOT an invalid grant; returning + // null here is what turned an outage blip into a forced + // "requires re-authentication" stop. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000); + const fetchMock = vi + .fn() + .mockImplementationOnce( + async () => + new Response( + JSON.stringify({ + token_endpoint: "https://idcs.expired/oauth2/v1/token", + }), + { + status: 200, + headers: { "Content-Type": "application/json" }, + }, + ), + ) + .mockImplementationOnce( + async () => + new Response( + JSON.stringify({ + error: "server_error", + error_description: "temporary issue", + }), + { + status: 500, + headers: { "Content-Type": "application/json" }, + }, + ), + ); + vi.stubGlobal("fetch", fetchMock); + + const capture = vi.fn(); + await expect( + getValidOcaCredentials( + createCredentials({ expires: 90_000 }), + { + refreshBufferMs: 60_000, + retryableTokenGraceMs: 30_000, + telemetry: { capture } as never, + }, + { + config: { + internal: { + clientId: "client-3", + idcsUrl: "https://idcs.expired", + scopes: "openid offline_access", + baseUrl: "https://oca.example.com", + }, + }, + }, + ), + ).rejects.toThrow("Token refresh failed: 500"); + expect(capture).toHaveBeenCalledWith( + expect.objectContaining({ + event: "user.auth_refresh_soft_failure", + properties: expect.objectContaining({ + provider: "oca", + status: 500, + tokenExpired: true, + }), + }), + ); + expect(capture).not.toHaveBeenCalledWith( + expect.objectContaining({ event: "user.auth_logged_out" }), + ); + nowSpy.mockRestore(); + }); + it("re-discovers token endpoint shortly after discovery fallback errors", async () => { const nowSpy = vi.spyOn(Date, "now").mockReturnValue(100_000); const fetchMock = vi diff --git a/sdk/packages/core/src/auth/oca.ts b/sdk/packages/core/src/auth/oca.ts index c0975d9066..bf2281cab9 100644 --- a/sdk/packages/core/src/auth/oca.ts +++ b/sdk/packages/core/src/auth/oca.ts @@ -3,6 +3,7 @@ import { nanoid } from "nanoid"; import { captureAuthFailed, captureAuthLoggedOut, + captureAuthRefreshSoftFailure, captureAuthStarted, captureAuthSucceeded, identifyAccount, @@ -512,13 +513,34 @@ export async function getValidOcaCredentials( try { return await refreshOcaToken(currentCredentials, providerOptions); } catch (error) { + const telemetry = providerOptions?.telemetry ?? options?.telemetry; + const failureDetails = { + status: error instanceof OcaOAuthTokenError ? error.status : undefined, + errorCode: + error instanceof OcaOAuthTokenError ? error.errorCode : undefined, + errorName: error instanceof Error ? error.name : undefined, + }; if (error instanceof OcaOAuthTokenError && error.isLikelyInvalidGrant()) { - captureAuthLoggedOut(providerOptions?.telemetry, "oca", "invalid_grant"); + captureAuthLoggedOut(telemetry, "oca", "invalid_grant", { + status: error.status, + errorCode: error.errorCode, + }); return null; } - if (currentCredentials.expires - Date.now() > retryableTokenGraceMs) { + const tokenExpired = + currentCredentials.expires - Date.now() <= retryableTokenGraceMs; + captureAuthRefreshSoftFailure(telemetry, "oca", { + ...failureDetails, + tokenExpired, + }); + if (!tokenExpired) { return currentCredentials; } - return null; + // Rethrow instead of returning null. A null from this function means the + // refresh token was REJECTED (re-auth required); a network blip or server + // error that happens to land after expiry must not be mistaken for that — + // callers turn null into a forced "requires re-authentication" stop even + // though the very next refresh attempt would likely succeed. + throw error; } }