Merge pull request #1315 from PicPeak/fix/1287-retry-failed-tiles

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

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

frontend/src/components/common/AuthenticatedImage.tsx

@@ -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;
   }

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<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);
+  });
+});