Skip to content

Commit 82cf74d

Browse files
waleedlatif1claude
andauthored
fix(oauth): keep a five-minute floor on served tokens and refresh ahead earlier (#9528)
* fix(oauth): keep a five-minute floor on served tokens and refresh ahead earlier * improvement(oauth): share refresh-ahead through the refresh single-flight and log it at debug Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 20fb744 commit 82cf74d

2 files changed

Lines changed: 130 additions & 23 deletions

File tree

‎apps/sim/lib/oauth/credential-service.test.ts‎

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -442,11 +442,16 @@ describe('OAuth access-token refresh headroom', () => {
442442
describe.each(readers)('$name', ({ read, throwsOnFailure }) => {
443443
it.each([
444444
{ providerId: 'google-drive', remainingMs: 120_000, refresh: 'wait' },
445-
{ providerId: 'google-drive', remainingMs: 240_000, refresh: 'ahead' },
446-
{ providerId: 'google-drive', remainingMs: 300_001, refresh: 'none' },
445+
{ providerId: 'google-drive', remainingMs: 240_000, refresh: 'wait' },
446+
{ providerId: 'google-drive', remainingMs: 300_000, refresh: 'wait' },
447+
{ providerId: 'google-drive', remainingMs: 300_001, refresh: 'ahead' },
448+
/** Inside the refresh-ahead window for every account, whatever its jitter. */
449+
{ providerId: 'google-drive', remainingMs: 540_000, refresh: 'ahead' },
450+
{ providerId: 'google-drive', remainingMs: 600_001, refresh: 'none' },
447451
{ providerId: 'google-drive', remainingMs: null, refresh: 'none' },
448452
{ providerId: 'confluence', remainingMs: 300_000, refresh: 'wait' },
449453
{ providerId: 'confluence', remainingMs: 300_001, refresh: 'none' },
454+
{ providerId: 'confluence', remainingMs: 540_000, refresh: 'none' },
450455
])(
451456
'handles $providerId with $remainingMs milliseconds remaining',
452457
async ({ providerId, remainingMs, refresh }) => {
@@ -463,7 +468,7 @@ describe('OAuth access-token refresh headroom', () => {
463468
it('uses a still-valid Google token while its refresh is still with the provider', async () => {
464469
const provider = createDeferred<unknown>()
465470
mocks.refreshOAuthToken.mockReturnValue(provider.promise)
466-
await expect(read(createOAuthAccount(240_000))).resolves.toBe('original-access-token')
471+
await expect(read(createOAuthAccount(360_000))).resolves.toBe('original-access-token')
467472
await vi.waitFor(() => expect(mocks.refreshOAuthToken).toHaveBeenCalledOnce())
468473
expect(dbChainMockFns.set).not.toHaveBeenCalled()
469474
provider.resolve({
@@ -495,6 +500,16 @@ describe('OAuth access-token refresh headroom', () => {
495500
providerId: 'google-drive',
496501
remainingMs: 60_000,
497502
},
503+
{
504+
label: 'a Google token with less than five minutes left',
505+
providerId: 'google-drive',
506+
remainingMs: 240_000,
507+
},
508+
{
509+
label: 'a Google token with exactly five minutes left',
510+
providerId: 'google-drive',
511+
remainingMs: 300_000,
512+
},
498513
])('waits for the provider with $label', async ({ providerId, remainingMs }) => {
499514
const provider = createDeferred<unknown>()
500515
mocks.refreshOAuthToken.mockReturnValue(provider.promise)
@@ -632,6 +647,56 @@ describe('OAuth access-token refresh headroom', () => {
632647
expect(mocks.refreshOAuthToken).toHaveBeenCalledTimes(1)
633648
})
634649

650+
it('shares one background refresh across every caller served inside the refresh-ahead window', async () => {
651+
const { coalesceLocally } = await vi.importActual<
652+
typeof import('@/lib/concurrency/singleflight')
653+
>('@/lib/concurrency/singleflight')
654+
mocks.coalesceLocally.mockImplementation(coalesceLocally)
655+
const provider = createDeferred<unknown>()
656+
mocks.refreshOAuthToken.mockReturnValue(provider.promise)
657+
const row = createOAuthAccount(360_000)
658+
await expect(refreshTokenIfNeeded('test', row, RAW_ACCOUNT_ID)).resolves.toEqual({
659+
accessToken: 'original-access-token',
660+
refreshed: false,
661+
})
662+
await expect(refreshTokenIfNeeded('test', row, RAW_ACCOUNT_ID)).resolves.toEqual({
663+
accessToken: 'original-access-token',
664+
refreshed: false,
665+
})
666+
await vi.waitFor(() => expect(mocks.refreshOAuthToken).toHaveBeenCalled())
667+
provider.resolve({
668+
ok: true,
669+
accessToken: 'refreshed-access-token',
670+
refreshToken: 'rotated-refresh-token',
671+
expiresIn: 3600,
672+
})
673+
await vi.waitFor(() => expect(dbChainMockFns.set).toHaveBeenCalled())
674+
await flushMicrotasks()
675+
expect(mocks.refreshOAuthToken).toHaveBeenCalledOnce()
676+
})
677+
678+
it('uses a token another writer stored after the caller read the account instead of refreshing again', async () => {
679+
queueCredentialAccount(createOAuthAccount())
680+
queueTableRows(account, [
681+
{ ...createOAuthAccount(3_600_000), accessToken: 'stored-meanwhile-token', updatedAt: now },
682+
])
683+
await expect(
684+
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
685+
).resolves.toEqual({ accessToken: 'stored-meanwhile-token' })
686+
expect(mocks.refreshOAuthToken).not.toHaveBeenCalled()
687+
})
688+
689+
it('refreshes when the token another writer stored is itself due to refresh ahead', async () => {
690+
queueCredentialAccount(createOAuthAccount())
691+
queueTableRows(account, [
692+
{ ...createOAuthAccount(420_000), accessToken: 'stored-meanwhile-token', updatedAt: now },
693+
])
694+
await expect(
695+
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
696+
).resolves.toEqual({ accessToken: 'refreshed-access-token' })
697+
expect(mocks.refreshOAuthToken).toHaveBeenCalledOnce()
698+
})
699+
635700
it.each([
636701
{ remainingMs: 300_000, refresh: true },
637702
{ remainingMs: 300_001, refresh: false },

‎apps/sim/lib/oauth/credential-service.ts‎

Lines changed: 62 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -886,8 +886,8 @@ interface CoalescedRefreshOptions {
886886
/** External provider account id (`account.accountId`), used to scope Slack refreshes per installation. */
887887
providerAccountId?: string | null
888888
oauthConfig?: string | null
889-
/** The account row's refresh token and recorded revocation, read together. */
890-
chain?: RevocableChain
889+
/** The account row the caller read: its tokens, recorded revocation, and version, read together. */
890+
chain?: StoredOAuthTokens
891891
requestId?: string
892892
userId?: string
893893
privacyMode?: 'selector'
@@ -1003,9 +1003,18 @@ function isOAuthAccessTokenExpiring(
10031003
return expiresAt != null && expiresAt.getTime() <= now.getTime() + refreshWindowMs
10041004
}
10051005

1006-
/** Below this much remaining validity a caller waits for the refresh even where it could refresh ahead. */
1007-
const OAUTH_ACCESS_TOKEN_MUST_REFRESH_MS = 2 * 60 * 1000
1008-
/** Spreads when credentials that expire together start refreshing ahead. */
1006+
/**
1007+
* Where a provider can refresh ahead of expiry, a caller is served the stored token with this much
1008+
* validity left or less while a background refresh replaces it. It starts above
1009+
* {@link OAUTH_ACCESS_TOKEN_REFRESH_WINDOW_MS}, so a served token keeps the same headroom a caller
1010+
* that waits for its refresh gets: callers that resolve once and reuse the token through a long
1011+
* batch (the pollers) depend on it.
1012+
*/
1013+
const OAUTH_REFRESH_AHEAD_WINDOW_MS = 10 * 60 * 1000
1014+
/**
1015+
* Spreads when credentials that expire together start refreshing ahead. Kept below the gap between
1016+
* the two windows so every account still refreshes ahead before callers have to wait.
1017+
*/
10091018
const OAUTH_REFRESH_AHEAD_JITTER_MS = 60 * 1000
10101019

10111020
/**
@@ -1061,15 +1070,17 @@ function planAccessTokenRefresh(
10611070
: { kind: 'fresh' }
10621071
}
10631072
if (!accessToken) return { kind: 'must-refresh', refreshToken }
1064-
const refreshesAhead = refreshesAheadOfExpiry(providerId)
10651073
if (accessTokenExpiresAt && isOAuthAccessTokenExpiring(accessTokenExpiresAt, providerId, now)) {
1066-
const remainingMs = accessTokenExpiresAt.getTime() - now.getTime()
1067-
if (!refreshesAhead || remainingMs <= OAUTH_ACCESS_TOKEN_MUST_REFRESH_MS) {
1068-
return { kind: 'must-refresh', refreshToken }
1069-
}
1070-
if (remainingMs <= OAUTH_ACCESS_TOKEN_REFRESH_WINDOW_MS - refreshAheadJitterMs(accountId)) {
1071-
return { kind: 'refresh-ahead', refreshToken }
1072-
}
1074+
return { kind: 'must-refresh', refreshToken }
1075+
}
1076+
const refreshesAhead = refreshesAheadOfExpiry(providerId)
1077+
if (
1078+
refreshesAhead &&
1079+
accessTokenExpiresAt &&
1080+
accessTokenExpiresAt.getTime() - now.getTime() <=
1081+
OAUTH_REFRESH_AHEAD_WINDOW_MS - refreshAheadJitterMs(accountId)
1082+
) {
1083+
return { kind: 'refresh-ahead', refreshToken }
10731084
}
10741085
/** Microsoft refresh tokens expire after 90 days without use. */
10751086
const refreshTokenAging =
@@ -1307,6 +1318,31 @@ async function performCoalescedRefresh({
13071318
if (recordedMeanwhile && !isRevocationReprobeDue(current)) {
13081319
return revokedOutcome(recordedMeanwhile)
13091320
}
1321+
/**
1322+
* A writer that stored a new token after the caller's read (a refresh that finished just
1323+
* before this one took the lock, or a reconnect) already answers it, so this refresh
1324+
* would only spend a provider call. The token must still clear the headroom a waiting
1325+
* caller is promised, and sit outside the refresh-ahead window so the next caller does
1326+
* not start another refresh straight away.
1327+
*/
1328+
const storedSinceRead =
1329+
!slackTeamId &&
1330+
!recordedMeanwhile &&
1331+
current &&
1332+
chain &&
1333+
current.updatedAt > chain.updatedAt &&
1334+
current.accessToken !== chain.accessToken &&
1335+
!(
1336+
refreshesAheadOfExpiry(providerId) &&
1337+
current.accessTokenExpiresAt &&
1338+
current.accessTokenExpiresAt.getTime() <= Date.now() + OAUTH_REFRESH_AHEAD_WINDOW_MS
1339+
)
1340+
? usableStoredToken(current, providerId)
1341+
: null
1342+
if (storedSinceRead) {
1343+
logger.info('Using the token stored since this caller read the account', logContext)
1344+
return tokenOutcome(storedSinceRead)
1345+
}
13101346
let chainVersion = current?.updatedAt
13111347
let refreshTokenToUse = refreshToken
13121348
let slackChainVersion: Date | null = null
@@ -1532,6 +1568,18 @@ async function performCoalescedRefresh({
15321568
}
15331569
}
15341570

1571+
/** Starts a refresh ahead of expiry; callers in the window share it through the refresh's own single-flight. */
1572+
function refreshInBackground(options: CoalescedRefreshOptions, logPrefix: string): void {
1573+
logger.debug(`${logPrefix}Refreshing ahead of expiry; using the still-valid access token`)
1574+
// The refresh settles and logs its own failures; this catches only a setup error before it starts.
1575+
performCoalescedRefresh(options).catch((error: unknown) => {
1576+
logger.error('Background token refresh failed to start', {
1577+
providerId: options.providerId,
1578+
...(options.privacyMode === 'selector' ? {} : { error: toError(error).message }),
1579+
})
1580+
})
1581+
}
1582+
15351583
type StoredAccessTokenResult =
15361584
| { kind: 'unrefreshable' }
15371585
| { kind: 'token'; accessToken: string | null; refreshed: boolean }
@@ -1561,13 +1609,7 @@ async function resolveStoredAccessToken(
15611609
}
15621610
const prefix = refresh.requestId ? `[${refresh.requestId}] ` : ''
15631611
if (plan.kind === 'refresh-ahead') {
1564-
performCoalescedRefresh(options).catch((error: unknown) => {
1565-
logger.error('Background token refresh failed', {
1566-
providerId: refresh.providerId,
1567-
...(refresh.privacyMode === 'selector' ? {} : { error: toError(error).message }),
1568-
})
1569-
})
1570-
logger.info(`${prefix}Refreshing ahead of expiry; using the still-valid access token`)
1612+
refreshInBackground(options, prefix)
15711613
return { kind: 'token', accessToken: stored.accessToken, refreshed: false }
15721614
}
15731615
const fresh = await waitWithAbort(performCoalescedRefresh(options), signal)

0 commit comments

Comments
 (0)