fix(gallery): retry a failed image fetch once the tile is back on screen

A rejected fetch in AuthenticatedImage set the error state, rendered
nothing, and never asked again. The fetch effect only re-runs when its
inputs change, and for a grid tile they never do — so a transient
failure (a hiccup on cellular, or Safari cancelling loads when the tab
goes to the background) was a permanently blank tile with no request in
flight and nothing in any log. Grid passes no fallbackSrc, so there was
not even a broken-image icon to point at.

The retry is bounded and gated. Three attempts with a doubling delay
(2 s, 4 s, 8 s), and an attempt fires only once the placeholder
intersects the viewport and the document is visible, so a tile that
failed while the user was away retries when they come back rather than
while they are still gone. A new src gets a fresh budget. The
fallbackSrc path is untouched: it already renders a plain <img> and
should not loop.

Two refinements from review. A final 4xx (anything but 408 and 429)
exhausts the budget at once: an expired gallery token or a missing
photo cannot be retried into existence, and on a 68-tile viewport three
retries each would be ~200 requests that cannot succeed. And a 429's
Retry-After is honoured as the minimum delay, because the backoff alone
would spend every retry inside a 15-minute rate-limit window and leave
the tile blank after the limit had lifted. Retry-After is not
CORS-safelisted, so server.js now exposes it for split-origin
deployments alongside Content-Disposition.

The error branch now renders the same grey box as the loading state
instead of null. That is what the retry effect observes, and it is
also something the user can see. The empty-src branch now clears the
error flag too, so a tile whose src is removed after a failure does not
keep showing the failure box.

Nine tests in AuthenticatedImage.retry.test.tsx; the retry cases fail
against the previous version.

Not presented as the fix for the iOS report. It closes the one gap that
turns a transient failure into a permanent one, which the reporter asked
for in the original issue, and it is worth having on any device.

Relates to issue 1287
This commit is contained in:
Paul Nothaft
2026-09-06 19:59:53 +02:00
parent b801f3a6b8
commit 77ae94e649
3 changed files with 357 additions and 3 deletions
+6 -1
View File
@@ -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
@@ -30,6 +30,44 @@ interface AuthenticatedImageProps extends Omit<React.ImgHTMLAttributes<HTMLImage
queuePriority?: 'high' | 'prefetch' | 'normal';
}
/**
* Retry budget for a fetch that rejects (#1287). Three attempts with a
* doubling delay — 2 s, 4 s, 8 s — and each one waits until the placeholder
* is actually on screen and the document is visible before it fires.
*/
const MAX_RETRIES = 3;
const RETRY_BASE_DELAY_MS = 2000;
/** A non-OK response, with its status so the retry can tell transient from final. */
class HttpError extends Error {
status: number;
/** Server-stated cooldown from `Retry-After`, in ms; 0 when absent. */
retryAfterMs: number;
constructor(status: number, statusText: string, retryAfter: string | null) {
super(`Failed to fetch image: ${status} ${statusText}`);
this.status = status;
this.retryAfterMs = parseRetryAfter(retryAfter);
}
}
/** `Retry-After` is either delay-seconds or an HTTP date. */
function parseRetryAfter(value: string | null): number {
if (!value) return 0;
const seconds = Number(value);
if (Number.isFinite(seconds)) return Math.max(0, seconds * 1000);
const at = Date.parse(value);
return Number.isFinite(at) ? Math.max(0, at - Date.now()) : 0;
}
/**
* A 4xx other than 408 (timeout) and 429 (rate limited) is the server's final
* answer for this URL — an expired gallery token, a missing photo — and asking
* again cannot change it. Everything else (network failure, 5xx, aborts that
* were not ours) may.
*/
const isFinalStatus = (status: number) =>
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<AuthenticatedImageProps> = ({
const [canvasFailed, setCanvasFailed] = useState(false);
const canvasRef = useRef<HTMLCanvasElement>(null);
const imageRef = useRef<HTMLImageElement | null>(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<string | undefined>(undefined);
const retryRef = useRef<HTMLDivElement | null>(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<AuthenticatedImageProps> = ({
// 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<AuthenticatedImageProps> = ({
});
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<AuthenticatedImageProps> = ({
// 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<AuthenticatedImageProps> = ({
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<AuthenticatedImageProps> = ({
return <img src={fallbackSrc} alt={alt} {...props} />;
}
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 (
<div ref={retryRef} className={props.className} style={{ backgroundColor: '#f3f4f6', ...props.style }} />
);
}
if (!imageSrc) {
return null;
}
@@ -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<string, string> = {}) => () => 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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
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(
<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" fallbackSrc="/static/fallback.png" alt="t" />,
);
await flush();
// primary + fallback, then the plain <img> 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(<AuthenticatedImage src="/api/gallery/demo/thumbnail/1" alt="t" />);
await flush();
await advance(2000); await flush();
await advance(4000); await flush();
await advance(8000); await flush();
expect(fetchMock).toHaveBeenCalledTimes(4);
rerender(<AuthenticatedImage src="/api/gallery/demo/thumbnail/2" alt="t" />);
await flush();
expect(fetchMock).toHaveBeenCalledTimes(5);
await advance(2000); await flush();
expect(fetchMock).toHaveBeenCalledTimes(6);
});
});