Skip to content

Commit 3be3109

Browse files
committed
fix(oauth): serve a stored token only when a refresh wait ran out on a held lease
- A failed blocking refresh now falls back to the stored token only on a distinct lease-held outcome: this caller followed another lease holder for its whole wait, its last read still found the chain it started from, and no leader flagged it dead. A dead flag, a terminal refresh error, a vanished account or a leader exception stay failures. - The in-process refresh settles after the follower's full wait plus headroom instead of the default 30s, which fired before the follower's last read could count; a leader hung in this process still fails its callers, now at that bound.
1 parent edd0621 commit 3be3109

3 files changed

Lines changed: 346 additions & 245 deletions

File tree

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

Lines changed: 69 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -601,8 +601,11 @@ describe('OAuth access-token refresh headroom', () => {
601601
expect(guard).toContain('original-refresh-token')
602602
})
603603

604+
/** A Google token with a minute left: the refresh is awaited, and enough validity remains to serve it. */
605+
const WAITED_GOOGLE_REMAINING_MS = 60_000
606+
604607
it('returns no token when the rotation write finds the account gone', async () => {
605-
queueCredentialAccount(createOAuthAccount())
608+
queueCredentialAccount(createOAuthAccount(WAITED_GOOGLE_REMAINING_MS))
606609
dbChainMockFns.returning.mockResolvedValueOnce([])
607610
queueTableRows(account, [createOAuthAccount()])
608611
queueTableRows(account, [])
@@ -629,8 +632,17 @@ describe('OAuth access-token refresh headroom', () => {
629632
expect(markCredentialDead).not.toHaveBeenCalled()
630633
})
631634

635+
it('returns no token for a credential flagged dead, however long its stored token lasts', async () => {
636+
mocks.getRecentTerminalError.mockResolvedValue('invalid_client')
637+
queueCredentialAccount(createOAuthAccount(WAITED_GOOGLE_REMAINING_MS))
638+
await expect(
639+
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
640+
).resolves.toBeNull()
641+
expect(mocks.withLeaderLock).not.toHaveBeenCalled()
642+
})
643+
632644
it('flags a credential dead on a terminal failure when its chain did not move', async () => {
633-
queueCredentialAccount(createOAuthAccount())
645+
queueCredentialAccount(createOAuthAccount(WAITED_GOOGLE_REMAINING_MS))
634646
vi.mocked(isTerminalRefreshError).mockReturnValue(true)
635647
mocks.refreshOAuthToken.mockResolvedValue({ ok: false, errorCode: 'invalid_client' })
636648
queueTableRows(account, [createOAuthAccount()])
@@ -798,6 +810,12 @@ describe('OAuth access-token refresh headroom', () => {
798810
redisConfigMockFns.mockAcquireLock.mockResolvedValueOnce(false)
799811
})
800812

813+
/** Every follower poll, once the lock is contested, reads the unchanged row the caller read. */
814+
async function keepChainStoredDuringWait(stored: OAuthAccount) {
815+
await vi.waitFor(() => expect(redisConfigMockFns.mockAcquireLock).toHaveBeenCalled())
816+
dbChainMockFns.limit.mockResolvedValue([stored])
817+
}
818+
801819
it.each([
802820
{
803821
label: 'serves a Google token still valid after the wait',
@@ -815,12 +833,59 @@ describe('OAuth access-token refresh headroom', () => {
815833
token: null,
816834
},
817835
])('$label', async ({ row, token }) => {
818-
queueCredentialAccount({ ...createOAuthAccount(row.remainingMs), providerId: row.providerId })
836+
const stored = { ...createOAuthAccount(row.remainingMs), providerId: row.providerId }
837+
queueCredentialAccount(stored)
819838
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
839+
await keepChainStoredDuringWait(stored)
820840
await vi.advanceTimersByTimeAsync(31_000)
821841
await expect(pending).resolves.toEqual(token === null ? null : { accessToken: token })
822842
expect(mocks.refreshOAuthToken).not.toHaveBeenCalled()
823843
})
844+
845+
it.each([
846+
{ label: 'the account is deleted', deleted: true, deadCode: null },
847+
{ label: 'another leader flags it dead', deleted: false, deadCode: 'invalid_client' },
848+
])('fails a valid Google token when $label during the wait', async ({ deleted, deadCode }) => {
849+
const stored = createOAuthAccount(100_000)
850+
queueCredentialAccount(stored)
851+
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
852+
if (!deleted) await keepChainStoredDuringWait(stored)
853+
if (deadCode) mocks.getRecentTerminalError.mockResolvedValue(deadCode)
854+
await vi.advanceTimersByTimeAsync(31_000)
855+
await expect(pending).resolves.toBeNull()
856+
})
857+
858+
it('lets the follower finish its wait before the in-process refresh times out', async () => {
859+
const { coalesceLocally } = await vi.importActual<
860+
typeof import('@/lib/concurrency/singleflight')
861+
>('@/lib/concurrency/singleflight')
862+
mocks.coalesceLocally.mockImplementation(coalesceLocally)
863+
const stored = createOAuthAccount(100_000)
864+
queueCredentialAccount(stored)
865+
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
866+
await keepChainStoredDuringWait(stored)
867+
await vi.advanceTimersByTimeAsync(31_000)
868+
await expect(pending).resolves.toEqual({ accessToken: 'original-access-token' })
869+
})
870+
})
871+
872+
it('fails callers of a refresh hung in this process once it times out', async () => {
873+
const { coalesceLocally } = await vi.importActual<
874+
typeof import('@/lib/concurrency/singleflight')
875+
>('@/lib/concurrency/singleflight')
876+
mocks.coalesceLocally.mockImplementation(coalesceLocally)
877+
mocks.refreshOAuthToken.mockReturnValue(new Promise(() => {}))
878+
queueCredentialAccount(createOAuthAccount(100_000))
879+
let settled = false
880+
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test').finally(
881+
() => {
882+
settled = true
883+
}
884+
)
885+
await vi.advanceTimersByTimeAsync(44_000)
886+
expect(settled).toBe(false)
887+
await vi.advanceTimersByTimeAsync(2_000)
888+
await expect(pending).resolves.toBeNull()
824889
})
825890

826891
it('fails a Google chain the provider revokes even while its access token is valid', async () => {
@@ -867,7 +932,7 @@ describe('OAuth access-token refresh headroom', () => {
867932
})
868933

869934
it('spreads refresh-ahead start across accounts expiring at the same moment', async () => {
870-
const row = createOAuthAccount(270_000)
935+
const row = createOAuthAccount(570_000)
871936
/** Chosen because their hashes fall on opposite sides of the 30s jitter cut, so a hash-rule change fails loudly. */
872937
await expect(refreshTokenIfNeeded('test', row, 'account-2')).resolves.toEqual({
873938
accessToken: 'original-access-token',

0 commit comments

Comments
 (0)