From 78cb299beee976d06ff4016ac1e62b4a5b7809fd Mon Sep 17 00:00:00 2001 From: Dominik Boeckle Date: Wed, 14 Oct 2026 08:39:25 +0200 Subject: [PATCH] fix(api): retry safe requests rejected by the per-user concurrency limit The per-user concurrency limiter answers request bursts with 429 Too Many Requests and Retry-After: 1. The API client surfaced that as a hard failure, so views that fan out many reads at once broke intermittently: on collection boards the item detail fallback (which now hydrates six reference lists in parallel) failed with "Too many concurrent requests" instead of rendering, and pickers loaded empty. Retry GET/HEAD/OPTIONS up to twice on 429, honouring Retry-After (capped at 5s, jittered), and stop waiting when the caller aborts. Writes are never retried. In-flight GET de-duplication keeps sharing a single retried request. Co-Authored-By: Claude Opus 5.5 --- frontend/src/lib/api/core.js | 80 +++++++++++++++++++++++++++-- frontend/src/lib/api/core.spec.js | 83 +++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 3 deletions(-) create mode 100644 frontend/src/lib/api/core.spec.js diff --git a/frontend/src/lib/api/core.js b/frontend/src/lib/api/core.js index c17f6b64f..1ea79c24d 100644 --- a/frontend/src/lib/api/core.js +++ b/frontend/src/lib/api/core.js @@ -14,6 +14,15 @@ export const ADMIN_UI_MUTATION_EVENT = 'windshift:admin-ui-mutation'; // Ensure the clock-drift warning toast fires at most once per session let driftWarningShown = false; +// The per-user concurrency limiter (internal/middleware/user_concurrency.go) +// answers bursts with 429 + Retry-After. Views that fan out many reads at once +// (collection boards, the item detail fallback) would otherwise surface +// "Too many concurrent requests" as a hard failure. Safe methods are retried a +// bounded number of times; writes are never retried here. +const CONCURRENCY_RETRY_ATTEMPTS = 2; +const CONCURRENCY_RETRY_MAX_DELAY_MS = 5000; +const SAFE_METHODS = ['GET', 'HEAD', 'OPTIONS']; + // Chromium does not consistently report fetches cancelled by a document // navigation as AbortError. Track the page lifecycle explicitly so callers do // not mistake unload cancellation for a connectivity failure. `pageshow` @@ -134,10 +143,75 @@ function createApiError(response, responseText) { // Add HTTP status info /** @type {any} */ (error).status = response.status; /** @type {any} */ (error).statusText = response.statusText; + /** @type {any} */ (error).retryAfter = response.headers?.get?.('Retry-After') ?? null; return error; } +/** + * Delay before retrying a 429: Retry-After (seconds) when present, otherwise + * exponential backoff, capped and jittered so parallel requests spread out. + * @param {any} error + * @param {number} attempt + */ +export function concurrencyRetryDelayMs(error, attempt, random = Math.random) { + const seconds = Number(error?.retryAfter); + const base = + error?.retryAfter != null && Number.isFinite(seconds) && seconds >= 0 + ? seconds * 1000 + : 500 * 2 ** attempt; + return Math.min(base, CONCURRENCY_RETRY_MAX_DELAY_MS) + Math.floor(random() * 250); +} + +/** + * @param {number} ms + * @param {AbortSignal | null | undefined} signal + */ +function waitForRetry(ms, signal) { + return new Promise((resolve, reject) => { + const aborted = () => new DOMException('The operation was aborted', 'AbortError'); + if (signal?.aborted) { + reject(aborted()); + return; + } + const timer = setTimeout(() => { + signal?.removeEventListener('abort', onAbort); + resolve(undefined); + }, ms); + function onAbort() { + clearTimeout(timer); + reject(aborted()); + } + signal?.addEventListener('abort', onAbort, { once: true }); + }); +} + +/** + * performFetchAPI plus bounded retries of safe requests rejected by the + * per-user concurrency limiter. + * @param {string} base + * @param {string} endpoint + * @param {RequestInit & { timeout?: number; text?: boolean }} [options] + */ +async function performFetchWithRetry(base, endpoint, options = {}) { + const method = String(options.method || 'GET').toUpperCase(); + const retryable = SAFE_METHODS.includes(method); + for (let attempt = 0; ; attempt += 1) { + try { + return await performFetchAPI(base, endpoint, options); + } catch (error) { + if ( + !retryable || + /** @type {any} */ (error)?.status !== 429 || + attempt >= CONCURRENCY_RETRY_ATTEMPTS + ) { + throw error; + } + await waitForRetry(concurrencyRetryDelayMs(error, attempt), options.signal); + } + } +} + /** * @param {string} base * @param {string} endpoint @@ -297,7 +371,7 @@ async function performFetchAPI(base, endpoint, options = {}) { } const method = String(fetchOptions.method || 'GET').toUpperCase(); - if (!['GET', 'HEAD', 'OPTIONS'].includes(method)) { + if (!SAFE_METHODS.includes(method)) { notifyAdminUIMutation(endpoint, method); } @@ -360,13 +434,13 @@ export async function fetchAllV2Pages(endpoint, options = {}) { function fetchFromAPI(base, endpoint, options) { const key = inFlightGETKey(base, endpoint, options); - if (!key) return performFetchAPI(base, endpoint, options); + if (!key) return performFetchWithRetry(base, endpoint, options); const existing = inFlightGetRequests.get(key); if (existing) return existing; let trackedRequest; - trackedRequest = performFetchAPI(base, endpoint, options).finally(() => { + trackedRequest = performFetchWithRetry(base, endpoint, options).finally(() => { if (inFlightGetRequests.get(key) === trackedRequest) { inFlightGetRequests.delete(key); } diff --git a/frontend/src/lib/api/core.spec.js b/frontend/src/lib/api/core.spec.js new file mode 100644 index 000000000..c78c3d397 --- /dev/null +++ b/frontend/src/lib/api/core.spec.js @@ -0,0 +1,83 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { concurrencyRetryDelayMs, fetchAPI, fetchAPIV2 } from './core.js'; + +const json = (body, status = 200, headers = {}) => + new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json', ...headers }, + }); +const tooMany = () => + json({ error: { code: 'TOO_MANY_REQUESTS', message: 'Too many concurrent requests' } }, 429, { + 'Retry-After': '1', + }); + +describe('concurrency limit retries', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it('retries a GET rejected with 429 and resolves with the later response', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(tooMany()) + .mockResolvedValueOnce(json({ data: [{ id: 1 }] })); + vi.stubGlobal('fetch', fetchMock); + + const result = fetchAPIV2('/custom-fields'); + await vi.runAllTimersAsync(); + + await expect(result).resolves.toEqual({ data: [{ id: 1 }] }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('gives up after the bounded number of retries and surfaces the 429', async () => { + const fetchMock = vi.fn().mockImplementation(() => Promise.resolve(tooMany())); + vi.stubGlobal('fetch', fetchMock); + + const result = fetchAPIV2('/milestones'); + const assertion = expect(result).rejects.toMatchObject({ status: 429 }); + await vi.runAllTimersAsync(); + + await assertion; + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it('never retries writes', async () => { + const fetchMock = vi.fn().mockImplementation(() => Promise.resolve(tooMany())); + vi.stubGlobal('fetch', fetchMock); + + const result = fetchAPI('/items/1', { method: 'PATCH', body: '{}' }); + const assertion = expect(result).rejects.toMatchObject({ status: 429 }); + await vi.runAllTimersAsync(); + + await assertion; + expect(fetchMock).toHaveBeenCalledOnce(); + }); + + it('stops waiting when the caller aborts', async () => { + const fetchMock = vi.fn().mockImplementation(() => Promise.resolve(tooMany())); + vi.stubGlobal('fetch', fetchMock); + const controller = new AbortController(); + + const result = fetchAPIV2('/iterations', { signal: controller.signal }); + const assertion = expect(result).rejects.toMatchObject({ name: 'AbortError' }); + await vi.advanceTimersByTimeAsync(0); + controller.abort(); + + await assertion; + expect(fetchMock).toHaveBeenCalledOnce(); + }); + + it('derives the delay from Retry-After, falling back to capped backoff', () => { + const noJitter = () => 0; + expect(concurrencyRetryDelayMs({ retryAfter: '1' }, 0, noJitter)).toBe(1000); + expect(concurrencyRetryDelayMs({ retryAfter: '60' }, 0, noJitter)).toBe(5000); + expect(concurrencyRetryDelayMs({ retryAfter: null }, 0, noJitter)).toBe(500); + expect(concurrencyRetryDelayMs({}, 2, noJitter)).toBe(2000); + }); +});