From 7cab11a75c3d948ba0cb7efb7a1f1b4886fa3f79 Mon Sep 17 00:00:00 2001 From: Gabriel Grasel Moura Date: Tue, 11 Aug 2026 19:56:31 -0300 Subject: [PATCH] fix oauth refresh lock races --- src/utils/auth.refresh.test.ts | 121 ++++++++++++++++++++++++++++++++- src/utils/auth.ts | 80 ++++++++++++++++++---- 2 files changed, 185 insertions(+), 16 deletions(-) diff --git a/src/utils/auth.refresh.test.ts b/src/utils/auth.refresh.test.ts index a4ee20dbe3..4cf5042a06 100644 --- a/src/utils/auth.refresh.test.ts +++ b/src/utils/auth.refresh.test.ts @@ -8,6 +8,9 @@ let stored: SecureStorageData = {} let refreshMode: 'success' | 'invalid_grant' | 'transient_error' = 'success' let refreshCalls = 0 let storageWritesFail = false +let lockReleaseErrorCode: string | null = null +let refreshGate: Promise | null = null +let markRefreshStarted: (() => void) | null = null const originalAxiosPost = axios.post beforeAll(() => { @@ -27,7 +30,13 @@ beforeAll(() => { }), })) mock.module('./lockfile.js', () => ({ - lock: async () => async () => {}, + lock: async () => async () => { + if (lockReleaseErrorCode) { + throw Object.assign(new Error('Lock is not acquired/owned by you'), { + code: lockReleaseErrorCode, + }) + } + }, lockSync: () => () => {}, })) mock.module('../services/oauth/getOauthProfile.js', () => ({ @@ -35,6 +44,9 @@ beforeAll(() => { })) axios.post = mock(async () => { refreshCalls++ + markRefreshStarted?.() + markRefreshStarted = null + if (refreshGate) await refreshGate if (refreshMode === 'invalid_grant') { throw Object.assign(new Error('invalid_grant'), { response: { data: { error: 'invalid_grant' } }, @@ -151,3 +163,110 @@ test('preserves the stored session after a transient refresh failure', async () expect(stored.verbooOauth?.accessToken).toBe('rejected-access-4') expect(stored.verbooOauth?.refreshToken).toBe('old-refresh-4') }) + +test('does not abort a successful refresh when the lock was already released', async () => { + stored = { + verbooOauth: { + accessToken: 'rejected-access-5', + refreshToken: 'old-refresh-5', + expiresAt: Date.now() + 600_000, + scopes: ['user:profile', 'user:inference'], + }, + } + refreshMode = 'success' + refreshCalls = 0 + storageWritesFail = false + lockReleaseErrorCode = 'ENOTACQUIRED' + + // @ts-expect-error cache-busting query keeps module state isolated. + const releaseRaceModule = await import('./auth.js?refresh-release-race') + const { handleOAuth401ErrorWithOutcome } = releaseRaceModule + + try { + const outcome = await handleOAuth401ErrorWithOutcome('rejected-access-5') + + expect(outcome).toBe('refreshed') + expect(refreshCalls).toBe(1) + expect(stored.verbooOauth?.accessToken).toBe('fresh-access') + } finally { + lockReleaseErrorCode = null + } +}) + +test('does not hide an unexpected lock release failure', async () => { + stored = { + verbooOauth: { + accessToken: 'rejected-access-unexpected-release', + refreshToken: 'old-refresh-unexpected-release', + expiresAt: Date.now() + 600_000, + scopes: ['user:profile', 'user:inference'], + }, + } + refreshMode = 'success' + refreshCalls = 0 + storageWritesFail = false + lockReleaseErrorCode = 'EIO' + + // @ts-expect-error cache-busting query keeps module state isolated. + const unexpectedReleaseModule = await import('./auth.js?unexpected-release') + + try { + await expect( + unexpectedReleaseModule.handleOAuth401ErrorWithOutcome( + 'rejected-access-unexpected-release', + ), + ).rejects.toMatchObject({ code: 'EIO' }) + } finally { + lockReleaseErrorCode = null + } +}) + +test('shares one refresh between an expiry check and a simultaneous 401', async () => { + stored = { + verbooOauth: { + accessToken: 'rejected-access-6', + refreshToken: 'old-refresh-6', + expiresAt: Date.now() - 1, + scopes: ['user:profile', 'user:inference'], + }, + } + refreshMode = 'success' + refreshCalls = 0 + storageWritesFail = false + lockReleaseErrorCode = null + + let releaseRefresh = () => {} + refreshGate = new Promise((resolve) => { + releaseRefresh = resolve + }) + const refreshStarted = new Promise((resolve) => { + markRefreshStarted = resolve + }) + + // @ts-expect-error cache-busting query keeps module state isolated. + const singleFlightModule = await import('./auth.js?refresh-single-flight') + const { + checkAndRefreshOAuthTokenIfNeeded, + handleOAuth401ErrorWithOutcome, + } = singleFlightModule + + const expiryCheck = checkAndRefreshOAuthTokenIfNeeded() + await refreshStarted + const rejectedRequest = handleOAuth401ErrorWithOutcome('rejected-access-6') + + try { + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(refreshCalls).toBe(1) + } finally { + releaseRefresh() + refreshGate = null + } + + const [expiryRecovered, rejectionOutcome] = await Promise.all([ + expiryCheck, + rejectedRequest, + ]) + expect(expiryRecovered).toBe(true) + expect(['refreshed', 'token_changed']).toContain(rejectionOutcome) + expect(stored.verbooOauth?.accessToken).toBe('fresh-access') +}) diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 314e4f7a13..af4724571e 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -1465,7 +1465,7 @@ async function handleOAuth401ErrorImpl( } // Same token that failed - force refresh, bypassing local expiration check - return checkAndRefreshOAuthTokenIfNeededImpl(0, true, failedAccessToken) + return runOAuthRefreshCheck(0, true, failedAccessToken) } /** @@ -1543,29 +1543,63 @@ function clearStoredVerbooOAuthIfRefreshTokenMatches( } } -// In-flight promise for deduplicating concurrent calls +// One in-flight refresh across proactive expiry checks and reactive 401s. let pendingRefreshCheck: Promise | null = null export function checkAndRefreshOAuthTokenIfNeeded( retryCount = 0, force = false, ): Promise { - // Deduplicate concurrent non-retry, non-force calls - if (retryCount === 0 && !force) { - if (pendingRefreshCheck) { - return pendingRefreshCheck.then(didOAuthRefreshRecover) + return runOAuthRefreshCheck(retryCount, force).then(didOAuthRefreshRecover) +} + +async function runOAuthRefreshCheck( + retryCount: number, + force: boolean, + failedAccessToken?: string, +): Promise { + if (retryCount !== 0) { + return checkAndRefreshOAuthTokenIfNeededImpl( + retryCount, + force, + failedAccessToken, + ) + } + + const pending = pendingRefreshCheck + if (pending) { + const outcome = await pending + if (!force) return outcome + + clearOAuthTokenCache() + const currentTokens = await getClaudeAIOAuthTokensAsync() + if ( + failedAccessToken && + currentTokens?.accessToken && + currentTokens.accessToken !== failedAccessToken + ) { + return 'token_changed' } - const promise = checkAndRefreshOAuthTokenIfNeededImpl(retryCount, force) - pendingRefreshCheck = promise.finally(() => { - pendingRefreshCheck = null - }) - return pendingRefreshCheck.then(didOAuthRefreshRecover) + if (outcome !== 'unchanged') return outcome + + // A proactive check can legitimately decide that the token is still + // valid. A simultaneous server 401 must still force one refresh after it. + return runOAuthRefreshCheck(retryCount, force, failedAccessToken) } - return checkAndRefreshOAuthTokenIfNeededImpl(retryCount, force).then( - didOAuthRefreshRecover, + const refresh = checkAndRefreshOAuthTokenIfNeededImpl( + retryCount, + force, + failedAccessToken, ) + const trackedRefresh = refresh.finally(() => { + if (pendingRefreshCheck === trackedRefresh) { + pendingRefreshCheck = null + } + }) + pendingRefreshCheck = trackedRefresh + return trackedRefresh } async function checkAndRefreshOAuthTokenIfNeededImpl( @@ -1731,8 +1765,24 @@ async function checkAndRefreshOAuthTokenIfNeededImpl( return 'transient_error' } finally { logEvent('tengu_oauth_token_refresh_lock_releasing', {}) - await release() - logEvent('tengu_oauth_token_refresh_lock_released', {}) + try { + await release() + logEvent('tengu_oauth_token_refresh_lock_released', {}) + } catch (error) { + const code = (error as { code?: string }).code + if (code !== 'ENOTACQUIRED' && code !== 'ERELEASED') { + throw error + } + + // proper-lockfile can report ownership loss when a stale lock was + // replaced and release callbacks complete out of order. The refresh + // result above is still authoritative; do not replace it with a raw + // release error. + logError(error) + logEvent('tengu_oauth_token_refresh_lock_release_ownership_lost', { + code: code as AnalyticsMetadata_I_VERIFIED_THIS_IS_NOT_CODE_OR_FILEPATHS, + }) + } } }