diff --git a/backend/server.js b/backend/server.js index 33c3aa7a..02a7c715 100644 --- a/backend/server.js +++ b/backend/server.js @@ -239,7 +239,12 @@ const corsOptions = { // deployments can read the server's chosen download filename. Used // by the gallery/admin download flows to honour the #493 "original // camera filename" toggle on individual photo downloads (#507). - exposedHeaders: ['Content-Disposition'], + // + // Retry-After is not CORS-safelisted either. AuthenticatedImage reads it + // off a 429 to wait out the rate-limit window before retrying a thumbnail + // fetch; without it a split-origin deployment would spend its retry budget + // inside the window and leave the tile blank after the limit had lifted. + exposedHeaders: ['Content-Disposition', 'Retry-After'], }; // Only attach CORS to API endpoints, not static assets diff --git a/frontend/src/components/common/AuthenticatedImage.tsx b/frontend/src/components/common/AuthenticatedImage.tsx index 0437413a..0db0b7ad 100644 --- a/frontend/src/components/common/AuthenticatedImage.tsx +++ b/frontend/src/components/common/AuthenticatedImage.tsx @@ -30,6 +30,44 @@ interface AuthenticatedImageProps extends Omit + status >= 400 && status < 500 && status !== 408 && status !== 429; + /** * Fetches an image with the gallery's bearer token and renders it. * @@ -76,6 +114,17 @@ export const AuthenticatedImage: React.FC = ({ const [canvasFailed, setCanvasFailed] = useState(false); const canvasRef = useRef(null); const imageRef = useRef(null); + // Retry state (#1287). The nonce is a dependency of the fetch effect, so + // bumping it is the retry; the counter is per src, so a new image gets a + // fresh budget without an extra effect run to reset it. + const [retryNonce, setRetryNonce] = useState(0); + const attemptsRef = useRef(0); + // Cooldown the server asked for on the last failure (#1287). The backoff + // alone would spend all three retries inside a 15-minute rate-limit window + // and leave the tile blank after the limit had actually lifted. + const retryAfterRef = useRef(0); + const lastSrcRef = useRef(undefined); + const retryRef = useRef(null); // Draw image to canvas when canvas rendering is enabled // Returns whether the pixels actually made it onto the canvas, so the @@ -108,9 +157,15 @@ export const AuthenticatedImage: React.FC = ({ // screen, which on a several-hundred-photo gallery is most of them. const controller = new AbortController(); + if (lastSrcRef.current !== src) { + lastSrcRef.current = src; + attemptsRef.current = 0; + } + // Determine which token to use based on context if (!src) { setImageSrc(fallbackSrc || ''); + setError(false); setIsLoading(false); return; } @@ -175,7 +230,11 @@ export const AuthenticatedImage: React.FC = ({ }); if (!response.ok) { - throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`); + throw new HttpError( + response.status, + response.statusText, + response.headers?.get?.('Retry-After') ?? null, + ); } return await response.blob(); @@ -203,6 +262,13 @@ export const AuthenticatedImage: React.FC = ({ // Returning here also stops the fallback below from firing a second // request against an already-aborted signal. if (aborted) return; + // A final 4xx exhausts the retry budget: on a 68-tile viewport, three + // retries per tile against an expired token would be ~200 requests + // that cannot succeed. + if (err instanceof HttpError && isFinalStatus(err.status)) { + attemptsRef.current = MAX_RETRIES; + } + retryAfterRef.current = err instanceof HttpError ? err.retryAfterMs : 0; setIsLoading(false); if (fallbackSrc && fallbackSrc !== src) { try { @@ -238,7 +304,65 @@ export const AuthenticatedImage: React.FC = ({ objectUrls.forEach((url) => URL.revokeObjectURL(url)); }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [src, fallbackSrc, slug, queuePriority]); + }, [src, fallbackSrc, slug, queuePriority, retryNonce]); + + // Retry a failed fetch once the tile is back on screen (#1287). + // + // Before this, a rejected fetch set `error` and nothing ever asked again: + // the effect above only re-runs when its inputs change, and for a grid + // tile they never do. On the original reporter's install that was the + // difference between a transient failure and a permanently blank tile — + // a hiccup on cellular, or Safari cancelling loads when the tab goes to + // the background, left a tile with no image, no request in flight and + // nothing in any log, for as long as the gallery stayed open. That is the + // retry-on-scrolling-back-into-view the reporter asked for in the issue. + // + // Bounded, and gated on visibility. The delay doubles per attempt so a + // server that is actually down is not hammered, and an attempt does not + // fire until the placeholder intersects the viewport and the document is + // visible — a tile that failed while backgrounded retries when the user + // comes back, not while they are still away. Without IntersectionObserver + // the placeholder counts as visible. + const retryable = error && !fallbackSrc && attemptsRef.current < MAX_RETRIES; + useEffect(() => { + if (!retryable) return; + const el = retryRef.current; + if (!el) return; + + let cancelled = false; + let delayElapsed = false; + let onScreen = typeof IntersectionObserver === 'undefined'; + + const retry = () => { + if (cancelled || !delayElapsed || !onScreen) return; + if (typeof document !== 'undefined' && document.visibilityState === 'hidden') return; + cancelled = true; + attemptsRef.current += 1; + setRetryNonce((n) => n + 1); + }; + + const timer = setTimeout(() => { + delayElapsed = true; + retry(); + }, Math.max(RETRY_BASE_DELAY_MS * 2 ** attemptsRef.current, retryAfterRef.current)); + + let observer: IntersectionObserver | null = null; + if (typeof IntersectionObserver !== 'undefined') { + observer = new IntersectionObserver((entries) => { + onScreen = entries.some((entry) => entry.isIntersecting); + retry(); + }); + observer.observe(el); + } + document.addEventListener('visibilitychange', retry); + + return () => { + cancelled = true; + clearTimeout(timer); + observer?.disconnect(); + document.removeEventListener('visibilitychange', retry); + }; + }, [retryable, retryNonce]); // Effect to draw to canvas when image is loaded and canvas rendering is enabled useEffect(() => { @@ -314,6 +438,15 @@ export const AuthenticatedImage: React.FC = ({ return {alt}; } + if (error) { + // Same box as the loading placeholder, and the element the retry effect + // observes. Returning null here (as this used to) left nothing to watch + // and nothing for the user to see either. + return ( +
+ ); + } + if (!imageSrc) { return null; } diff --git a/frontend/src/components/common/__tests__/AuthenticatedImage.retry.test.tsx b/frontend/src/components/common/__tests__/AuthenticatedImage.retry.test.tsx new file mode 100644 index 00000000..fb3c82ef --- /dev/null +++ b/frontend/src/components/common/__tests__/AuthenticatedImage.retry.test.tsx @@ -0,0 +1,216 @@ +/** + * Retry after a failed fetch (#1287). + * + * A rejected fetch used to set `error`, render nothing, and never ask again: + * the fetch effect only re-runs when its inputs change, and for a grid tile + * they never do. A transient failure — a hiccup on cellular, Safari + * cancelling loads when the tab is backgrounded — was therefore a + * permanently blank tile with nothing in any log. + * + * The retry is bounded (three attempts, doubling delay) and gated on the + * placeholder being on screen and the document being visible. + */ +import { act, render } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('../../../utils/galleryAuthStorage', () => ({ + getActiveGallerySlug: () => 'demo', + getGalleryToken: () => 'token', + inferGallerySlugFromLocation: () => 'demo', + resolveSlugFromRequestUrl: () => 'demo', +})); +vi.mock('../../../utils/url', () => ({ buildResourceUrl: (u: string) => `http://localhost${u}` })); + +import { AuthenticatedImage } from '../AuthenticatedImage'; + +const ok = () => Promise.resolve({ + ok: true, + blob: async () => new Blob(['x'], { type: 'image/png' }), +}); +const loadFailed = () => Promise.reject(new TypeError('Load failed')); +const status = (code: number, headers: Record = {}) => () => Promise.resolve({ + ok: false, + status: code, + statusText: 'x', + headers: { get: (name: string) => headers[name] ?? null }, +}); + +/** The observer callbacks registered by the component, so a test can drive them. */ +type IOCallback = (entries: Array<{ isIntersecting: boolean }>) => void; +let observers: IOCallback[]; + +beforeEach(() => { + vi.useFakeTimers(); + observers = []; + URL.createObjectURL = vi.fn(() => 'blob:mock') as unknown as typeof URL.createObjectURL; + URL.revokeObjectURL = vi.fn() as unknown as typeof URL.revokeObjectURL; +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.useRealTimers(); +}); + +/** Let every pending promise in the fetch/queue chain settle. */ +const flush = () => act(async () => { await vi.advanceTimersByTimeAsync(0); }); +const advance = (ms: number) => act(async () => { await vi.advanceTimersByTimeAsync(ms); }); + +/** IntersectionObserver stub that never fires on its own; tests drive it. */ +function stubIntersectionObserver() { + vi.stubGlobal('IntersectionObserver', class { + constructor(cb: IOCallback) { observers.push(cb); } + observe() {} + disconnect() {} + unobserve() {} + }); +} + +describe('AuthenticatedImage retry', () => { + it('retries a failed fetch after the delay and renders the image', async () => { + const fetchMock = vi.fn().mockImplementationOnce(loadFailed).mockImplementation(ok); + vi.stubGlobal('fetch', fetchMock); + + const { container } = render(); + await flush(); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(container.querySelector('img')).toBeNull(); + + await advance(1999); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await advance(1); + await flush(); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(container.querySelector('img')?.getAttribute('src')).toBe('blob:mock'); + }); + + it('gives up after three retries with a doubling delay', async () => { + const fetchMock = vi.fn().mockImplementation(loadFailed); + vi.stubGlobal('fetch', fetchMock); + + render(); + await flush(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await advance(2000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(2); + await advance(4000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(3); + await advance(8000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(4); + + await advance(60_000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(4); + }); + + it('waits until the placeholder is on screen before retrying', async () => { + stubIntersectionObserver(); + const fetchMock = vi.fn().mockImplementationOnce(loadFailed).mockImplementation(ok); + vi.stubGlobal('fetch', fetchMock); + + const { container } = render(); + await flush(); + expect(observers).toHaveLength(1); + + // Delay elapsed, but the tile is not on screen: no retry. + await advance(2000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // Scrolled back into view: retry fires now, without another delay. + act(() => observers[0]([{ isIntersecting: true }])); + await flush(); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(container.querySelector('img')).not.toBeNull(); + }); + + it('does not retry while the document is hidden, and does when it comes back', async () => { + const fetchMock = vi.fn().mockImplementationOnce(loadFailed).mockImplementation(ok); + vi.stubGlobal('fetch', fetchMock); + const visibility = vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('hidden'); + + render(); + await flush(); + await advance(2000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + visibility.mockReturnValue('visible'); + act(() => { document.dispatchEvent(new Event('visibilitychange')); }); + await flush(); + expect(fetchMock).toHaveBeenCalledTimes(2); + visibility.mockRestore(); + }); + + it('does not retry a final 4xx such as an expired token', async () => { + const fetchMock = vi.fn().mockImplementation(status(401)); + vi.stubGlobal('fetch', fetchMock); + + render(); + await flush(); + await advance(60_000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('retries a 5xx and a 429', async () => { + const fetchMock = vi.fn() + .mockImplementationOnce(status(503)) + .mockImplementationOnce(status(429)) + .mockImplementation(ok); + vi.stubGlobal('fetch', fetchMock); + + const { container } = render(); + await flush(); + await advance(2000); await flush(); + await advance(4000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(3); + expect(container.querySelector('img')?.getAttribute('src')).toBe('blob:mock'); + }); + + it('waits out a Retry-After longer than the backoff before spending a retry', async () => { + const fetchMock = vi.fn() + .mockImplementationOnce(status(429, { 'Retry-After': '30' })) + .mockImplementation(ok); + vi.stubGlobal('fetch', fetchMock); + + const { container } = render(); + await flush(); + // The 2 s backoff alone would have fired here. + await advance(29_999); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(1); + await advance(1); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(container.querySelector('img')).not.toBeNull(); + }); + + it('leaves the fallbackSrc path alone', async () => { + const fetchMock = vi.fn().mockImplementation(loadFailed); + vi.stubGlobal('fetch', fetchMock); + + const { container } = render( + , + ); + await flush(); + // primary + fallback, then the plain takes over — no retry loop. + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(container.querySelector('img')?.getAttribute('src')).toBe('/static/fallback.png'); + await advance(60_000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('a new src gets a fresh retry budget', async () => { + const fetchMock = vi.fn().mockImplementation(loadFailed); + vi.stubGlobal('fetch', fetchMock); + + const { rerender } = render(); + await flush(); + await advance(2000); await flush(); + await advance(4000); await flush(); + await advance(8000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(4); + + rerender(); + await flush(); + expect(fetchMock).toHaveBeenCalledTimes(5); + await advance(2000); await flush(); + expect(fetchMock).toHaveBeenCalledTimes(6); + }); +});