From 38a396d1509aa2ff991b93e26e39ed88ce1c5b55 Mon Sep 17 00:00:00 2001 From: dealerweb Date: Fri, 10 Jul 2026 14:08:17 +0200 Subject: [PATCH] Fix: end refresh loops on sign-out and back off failed retries Fixes #588. Sign-out already cleared the token-refresh timers and stopped the keep-alive interval - the reported endless loops came from async callbacks that were in flight at that moment. The token refresh's failure handler re-armed its retry after logout, and a failing keep-alive ping called reconnect() -> connect(), which restarts the keep-alive and thereby revived the interval disconnect() had just stopped. Only closing the tab ended it. Two mechanisms fix that class: transiently failed token refreshes only re-arm while the account is still signed in (checked when the failure lands, not when the request started), and the client carries an intentionallyDisconnected flag set by disconnect() - the ping callback, reconnect(), the SSE reconnect scheduling and the polling fallback all stop at it, so nothing revives after an intentional sign-out. Failed retries also back off instead of hammering a down server every 30 seconds: the token refresh climbs 30s/1m/2m/5m (capped, reset on success), and the keep-alive skips upcoming ticks on consecutive failures for the same effective ladder. Recovery after an outage is unchanged in substance - the session survives and reconnects within at most ~5 minutes, immediately on user activity. --- lib/jmap/client.ts | 27 +++++++++++ stores/__tests__/auth-store-logout.test.ts | 56 ++++++++++++++++++++-- stores/auth-store.ts | 51 ++++++++++++++++---- 3 files changed, 122 insertions(+), 12 deletions(-) diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 05bf6505..387f2393 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -507,6 +507,14 @@ export class JMAPClient implements IJMAPClient { private session: JMAPSession | null = null; private lastPingTime: number = 0; private pingInterval: NodeJS.Timeout | null = null; + // Set by disconnect() so async callbacks that were already in flight + // (keep-alive ping, SSE error handlers) cannot revive timers or + // reconnect after an intentional sign-out (#588). + private intentionallyDisconnected = false; + // Consecutive keep-alive failures; failed pings skip upcoming ticks + // (30s -> 1m -> 2m -> ~5m) instead of hammering a down server (#588). + private pingFailureCount = 0; + private pingSkipRemaining = 0; private accounts: Record = {}; private eventSource: EventSource | null = null; private stateChangeCallback: ((change: StateChange) => void) | null = null; @@ -689,6 +697,7 @@ export class JMAPClient implements IJMAPClient { } async connect(): Promise { + this.intentionallyDisconnected = false; const sessionUrl = `${this.serverUrl}/.well-known/jmap`; try { @@ -758,19 +767,33 @@ export class JMAPClient implements IJMAPClient { this.stopKeepAlive(); this.pingInterval = setInterval(async () => { + if (this.intentionallyDisconnected) return; // Skip ping while rate-limited to avoid compounding auth failures if (this.isRateLimited()) return; + // Back off while the server is down: each consecutive failure skips + // more ticks (30s -> 1m -> 2m -> ~5m) instead of retrying flat-out. + if (this.pingSkipRemaining > 0) { + this.pingSkipRemaining--; + return; + } try { await this.ping(); + this.pingFailureCount = 0; this.connectionChangeCallback?.(true); } catch (error) { if (error instanceof RateLimitError) { return; } + // A sign-out while the ping was in flight - stay down. + if (this.intentionallyDisconnected) return; + this.pingFailureCount++; + this.pingSkipRemaining = Math.min(2 ** this.pingFailureCount, 10) - 1; console.error('Keep-alive ping failed:', error); this.connectionChangeCallback?.(false); try { await this.reconnect(); + this.pingFailureCount = 0; + this.pingSkipRemaining = 0; this.connectionChangeCallback?.(true); } catch (reconnectError) { console.error('Reconnection failed:', reconnectError); @@ -803,10 +826,12 @@ export class JMAPClient implements IJMAPClient { } async reconnect(): Promise { + if (this.intentionallyDisconnected) return; await this.connect(); } disconnect(): void { + this.intentionallyDisconnected = true; this.stopKeepAlive(); this.closePushNotifications(); if (this.rateLimitTimeout) { @@ -5758,6 +5783,7 @@ export class JMAPClient implements IJMAPClient { } private scheduleSSEReconnect(): void { + if (this.intentionallyDisconnected) return; const eventSourceUrl = this.getEventSourceUrl(); if (!eventSourceUrl) { this.fallbackToPolling(); @@ -5783,6 +5809,7 @@ export class JMAPClient implements IJMAPClient { } private startPollingFallback(): void { + if (this.intentionallyDisconnected) return; if (this.isRateLimited()) { return; } diff --git a/stores/__tests__/auth-store-logout.test.ts b/stores/__tests__/auth-store-logout.test.ts index e8629c8a..8d0606eb 100644 --- a/stores/__tests__/auth-store-logout.test.ts +++ b/stores/__tests__/auth-store-logout.test.ts @@ -125,13 +125,61 @@ describe('auth-store logout redirects', () => { expect(sessionStorage.getItem('session_expired')).toBeNull(); expect(replaceSpy).not.toHaveBeenCalled(); + const countPuts = () => fetchMock.mock.calls.filter( + ([input, init]) => String(input) === '/api/auth/token?slot=0' && init?.method === 'PUT', + ).length; + // A retry is armed: advancing past the ~30 s window fires a second PUT. await vi.advanceTimersByTimeAsync(31_000); - const refreshPuts = fetchMock.mock.calls.filter( - ([input, init]) => String(input) === '/api/auth/token?slot=0' && init?.method === 'PUT', - ); - expect(refreshPuts.length).toBe(2); + expect(countPuts()).toBe(2); expect(useAuthStore.getState().isAuthenticated).toBe(true); + + // Backoff: after the second failure the next retry waits ~60 s, not 30. + await vi.advanceTimersByTimeAsync(31_000); + expect(countPuts()).toBe(2); + await vi.advanceTimersByTimeAsync(30_000); + expect(countPuts()).toBe(3); + }); + + it('stops retrying when the user signs out during the outage (#588)', async () => { + vi.useFakeTimers(); + + let resolveInFlight: ((value: { ok: boolean; status: number; json: () => Promise }) => void) | undefined; + const fetchMock = vi.fn(async (input: FetchInput, init?: FetchInit) => { + const url = String(input); + const method = init?.method ?? 'GET'; + + if (url === '/api/auth/token?slot=0' && method === 'PUT') { + return new Promise((resolve) => { resolveInFlight = resolve; }); + } + if (method === 'DELETE') { + return { ok: true, json: async () => ({}) }; + } + throw new Error(`Unexpected fetch call: ${method} ${url}`); + }); + + vi.stubGlobal('fetch', fetchMock); + vi.spyOn(browserNavigation, 'replaceWindowLocation').mockImplementation(() => {}); + + useAuthStore.setState({ + isAuthenticated: true, + authMode: 'oauth', + activeAccountId: null, + }); + + // Refresh goes in flight, then the user signs out before it settles. + const pending = useAuthStore.getState().refreshAccessToken(); + useAuthStore.getState().logout(); + resolveInFlight!({ ok: false, status: 503, json: async () => ({}) }); + await pending; + + // The failure lands after the sign-out - no retry may be re-armed. + const countPuts = () => fetchMock.mock.calls.filter( + ([input, init]) => String(input) === '/api/auth/token?slot=0' && init?.method === 'PUT', + ).length; + expect(countPuts()).toBe(1); + await vi.advanceTimersByTimeAsync(600_000); + expect(countPuts()).toBe(1); }); it('keeps the session when the refresh request fails with a network error', async () => { diff --git a/stores/auth-store.ts b/stores/auth-store.ts index 331933ba..85b39ec6 100644 --- a/stores/auth-store.ts +++ b/stores/auth-store.ts @@ -327,9 +327,34 @@ const clients = new Map(); const refreshTimers = new Map>(); const refreshPromises = new Map>(); -// Pseudo-expiry passed to scheduleRefresh when a refresh failed transiently: -// the "expiry - 60s" math below turns 90 into a retry in ~30 seconds. -const TOKEN_REFRESH_RETRY_SECONDS = 90; +// Retry backoff for transiently failed token refreshes (#588). The values +// are pseudo-expiries for scheduleRefresh - its "expiry - 60s" math turns +// them into delays of 30s, 1m, 2m and 5m (capped). Consecutive failures +// climb the ladder; any success resets it. +const TOKEN_REFRESH_RETRY_LADDER_SECONDS = [90, 120, 180, 360] as const; +const refreshFailureCounts = new Map(); + +function nextRefreshRetrySeconds(accountId?: string): number { + const key = accountId ?? '__global__'; + const failures = refreshFailureCounts.get(key) ?? 0; + refreshFailureCounts.set(key, failures + 1); + return TOKEN_REFRESH_RETRY_LADDER_SECONDS[ + Math.min(failures, TOKEN_REFRESH_RETRY_LADDER_SECONDS.length - 1) + ]; +} + +function resetRefreshBackoff(accountId?: string): void { + refreshFailureCounts.delete(accountId ?? '__global__'); +} + +// Only re-arm a failed refresh while someone is still signed in to that +// account. A sign-out during the outage - or while the request was in +// flight - must end the retry loop instead of keeping it alive with +// doomed requests (#588). +function shouldRetryRefresh(accountId?: string): boolean { + if (accountId) return !!useAccountStore.getState().getAccountById(accountId); + return useAuthStore.getState().isAuthenticated; +} function scheduleRefresh(expiresIn: number, refreshFn: () => Promise, accountId?: string): void { if (accountId) { @@ -360,11 +385,13 @@ function clearRefreshTimer(accountId?: string): void { refreshTimers.delete(accountId); } refreshPromises.delete(accountId); + refreshFailureCounts.delete(accountId); } else { if (refreshTimer) { clearTimeout(refreshTimer); refreshTimer = null; } + refreshFailureCounts.delete('__global__'); refreshPromise = null; } } @@ -375,6 +402,7 @@ function clearAllRefreshTimers(): void { for (const timer of refreshTimers.values()) clearTimeout(timer); refreshTimers.clear(); refreshPromises.clear(); + refreshFailureCounts.clear(); } /** @@ -990,13 +1018,17 @@ export const useAuthStore = create()( // the session and retry shortly so "stay signed in" survives // maintenance windows and offline spells. if (res.status === 401) { + resetRefreshBackoff(accountId ?? undefined); notifyParent('sso:session-expired'); markSessionExpired(); get().logout(); return null; } - debug.error(`Token refresh unavailable (${res.status}), retrying shortly`); - scheduleRefresh(TOKEN_REFRESH_RETRY_SECONDS, get().refreshAccessToken, accountId ?? undefined); + if (shouldRetryRefresh(accountId ?? undefined)) { + const retryIn = nextRefreshRetrySeconds(accountId ?? undefined); + debug.error(`Token refresh unavailable (${res.status}), retrying with backoff`); + scheduleRefresh(retryIn, get().refreshAccessToken, accountId ?? undefined); + } return null; } @@ -1018,13 +1050,16 @@ export const useAuthStore = create()( tokenExpiresAt: Date.now() + expires_in * 1000, }); + resetRefreshBackoff(accountId ?? undefined); scheduleRefresh(expires_in, get().refreshAccessToken, accountId ?? undefined); return access_token; } catch (error) { // Network failure (offline, Wi-Fi switch, server unreachable) - - // not a rejection. Keep the session and retry shortly. - debug.error('Token refresh failed, retrying shortly:', error); - scheduleRefresh(TOKEN_REFRESH_RETRY_SECONDS, get().refreshAccessToken, accountId ?? undefined); + // not a rejection. Keep the session and retry with backoff. + debug.error('Token refresh failed, retrying with backoff:', error); + if (shouldRetryRefresh(accountId ?? undefined)) { + scheduleRefresh(nextRefreshRetrySeconds(accountId ?? undefined), get().refreshAccessToken, accountId ?? undefined); + } return null; } finally { refreshPromise = null;