Skip to content

Commit 1111629

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 a129418 commit 1111629

3 files changed

Lines changed: 322 additions & 221 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
@@ -591,8 +591,11 @@ describe('OAuth access-token refresh headroom', () => {
591591
expect(guard).toContain('original-refresh-token')
592592
})
593593

594+
/** A Google token with a minute left: the refresh is awaited, and enough validity remains to serve it. */
595+
const WAITED_GOOGLE_REMAINING_MS = 60_000
596+
594597
it('returns no token when the rotation write finds the account gone', async () => {
595-
queueCredentialAccount(createOAuthAccount())
598+
queueCredentialAccount(createOAuthAccount(WAITED_GOOGLE_REMAINING_MS))
596599
dbChainMockFns.returning.mockResolvedValueOnce([])
597600
queueTableRows(account, [createOAuthAccount()])
598601
queueTableRows(account, [])
@@ -619,8 +622,17 @@ describe('OAuth access-token refresh headroom', () => {
619622
expect(markCredentialDead).not.toHaveBeenCalled()
620623
})
621624

625+
it('returns no token for a credential flagged dead, however long its stored token lasts', async () => {
626+
mocks.getRecentTerminalError.mockResolvedValue('invalid_client')
627+
queueCredentialAccount(createOAuthAccount(WAITED_GOOGLE_REMAINING_MS))
628+
await expect(
629+
resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
630+
).resolves.toBeNull()
631+
expect(mocks.withLeaderLock).not.toHaveBeenCalled()
632+
})
633+
622634
it('flags a credential dead on a terminal failure when its chain did not move', async () => {
623-
queueCredentialAccount(createOAuthAccount())
635+
queueCredentialAccount(createOAuthAccount(WAITED_GOOGLE_REMAINING_MS))
624636
vi.mocked(isTerminalRefreshError).mockReturnValue(true)
625637
mocks.refreshOAuthToken.mockResolvedValue({ ok: false, errorCode: 'invalid_client' })
626638
queueTableRows(account, [createOAuthAccount()])
@@ -738,6 +750,12 @@ describe('OAuth access-token refresh headroom', () => {
738750
redisConfigMockFns.mockAcquireLock.mockResolvedValueOnce(false)
739751
})
740752

753+
/** Every follower poll, once the lock is contested, reads the unchanged row the caller read. */
754+
async function keepChainStoredDuringWait(stored: OAuthAccount) {
755+
await vi.waitFor(() => expect(redisConfigMockFns.mockAcquireLock).toHaveBeenCalled())
756+
dbChainMockFns.limit.mockResolvedValue([stored])
757+
}
758+
741759
it.each([
742760
{
743761
label: 'serves a Google token still valid after the wait',
@@ -755,12 +773,59 @@ describe('OAuth access-token refresh headroom', () => {
755773
token: null,
756774
},
757775
])('$label', async ({ row, token }) => {
758-
queueCredentialAccount({ ...createOAuthAccount(row.remainingMs), providerId: row.providerId })
776+
const stored = { ...createOAuthAccount(row.remainingMs), providerId: row.providerId }
777+
queueCredentialAccount(stored)
759778
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
779+
await keepChainStoredDuringWait(stored)
760780
await vi.advanceTimersByTimeAsync(31_000)
761781
await expect(pending).resolves.toEqual(token === null ? null : { accessToken: token })
762782
expect(mocks.refreshOAuthToken).not.toHaveBeenCalled()
763783
})
784+
785+
it.each([
786+
{ label: 'the account is deleted', deleted: true, deadCode: null },
787+
{ label: 'another leader flags it dead', deleted: false, deadCode: 'invalid_client' },
788+
])('fails a valid Google token when $label during the wait', async ({ deleted, deadCode }) => {
789+
const stored = createOAuthAccount(100_000)
790+
queueCredentialAccount(stored)
791+
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
792+
if (!deleted) await keepChainStoredDuringWait(stored)
793+
if (deadCode) mocks.getRecentTerminalError.mockResolvedValue(deadCode)
794+
await vi.advanceTimersByTimeAsync(31_000)
795+
await expect(pending).resolves.toBeNull()
796+
})
797+
798+
it('lets the follower finish its wait before the in-process refresh times out', async () => {
799+
const { coalesceLocally } = await vi.importActual<
800+
typeof import('@/lib/concurrency/singleflight')
801+
>('@/lib/concurrency/singleflight')
802+
mocks.coalesceLocally.mockImplementation(coalesceLocally)
803+
const stored = createOAuthAccount(100_000)
804+
queueCredentialAccount(stored)
805+
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test')
806+
await keepChainStoredDuringWait(stored)
807+
await vi.advanceTimersByTimeAsync(31_000)
808+
await expect(pending).resolves.toEqual({ accessToken: 'original-access-token' })
809+
})
810+
})
811+
812+
it('fails callers of a refresh hung in this process once it times out', async () => {
813+
const { coalesceLocally } = await vi.importActual<
814+
typeof import('@/lib/concurrency/singleflight')
815+
>('@/lib/concurrency/singleflight')
816+
mocks.coalesceLocally.mockImplementation(coalesceLocally)
817+
mocks.refreshOAuthToken.mockReturnValue(new Promise(() => {}))
818+
queueCredentialAccount(createOAuthAccount(100_000))
819+
let settled = false
820+
const pending = resolveCredentialTokenBundle(RAW_CREDENTIAL_ID, RAW_USER_ID, 'test').finally(
821+
() => {
822+
settled = true
823+
}
824+
)
825+
await vi.advanceTimersByTimeAsync(44_000)
826+
expect(settled).toBe(false)
827+
await vi.advanceTimersByTimeAsync(2_000)
828+
await expect(pending).resolves.toBeNull()
764829
})
765830

766831
it('fails a Google chain the provider revokes even while its access token is valid', async () => {

0 commit comments

Comments
 (0)