fix(gallery): bound concurrent image fetches and abort them on unmount (#1287)

Hardening for the large-gallery stall. The reporter could not isolate the
cause and neither could I from static reading; these are two defects that
are wrong independently of whether they are the whole story.

Gallery grids are NOT virtualized: a 546-photo event puts 546 PhotoCards
in the DOM, each mounting its own bare fetch. Two problems there: no cap,
and a cleanup that only set a flag while the request kept running.

withImageFetchSlot now holds requests to six in flight, with the BODY read
inside the slot — fetch resolves on headers, so releasing there would have
bounded header round-trips and nothing else. Teardown aborts via
AbortController.

A request queued indefinitely is PENDING, not failed, which is why the
failure left no console error, no failed request and nothing in the
backend log.

Review follow-ups: three tiers (current lightbox slide > neighbour
prefetch > grid thumbnails), because a single FIFO put the image the user
just clicked behind hundreds of thumbnails; and a synchronous throw now
releases its slot instead of permanently draining the pool.

If it recurs, capture performance.getEntriesByType('resource') for the
stalled thumbnails — a queued request shows responseStart === 0.

Relates to #1287
This commit is contained in:
Paul Nothaft
2026-09-04 14:26:48 +02:00
committed by GitHub
parent 8f98f6bdec
commit 4afe7a6f08
4 changed files with 502 additions and 9 deletions
@@ -1,5 +1,6 @@
import React, { useState, useEffect, useRef, useCallback } from 'react';
import { buildResourceUrl } from '../../utils/url';
import { withImageFetchSlot } from '../../utils/imageFetchQueue';
import {
getActiveGallerySlug,
getGalleryToken,
@@ -30,6 +31,16 @@ interface AuthenticatedImageProps extends Omit<React.ImgHTMLAttributes<HTMLImage
protectionLevel?: 'basic' | 'standard' | 'enhanced' | 'maximum';
useEnhancedProtection?: boolean;
onLoad?: () => void;
/**
* Priority in the shared fetch queue (#1287). NOT the native `fetchPriority`
* DOM attribute, which stays available on this component and takes
* "low"|"high"|"auto" — hence the distinct name.
*
* 'high' the image the user is looking at now (current lightbox slide)
* 'prefetch' one interaction away (lightbox neighbours)
* 'normal' grid thumbnails
*/
queuePriority?: 'high' | 'prefetch' | 'normal';
}
export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
@@ -56,6 +67,7 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
protectionLevel,
useEnhancedProtection,
onLoad,
queuePriority = 'normal',
...props
}) => {
const unusedProps = {
@@ -108,6 +120,10 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
useEffect(() => {
let aborted = false;
const objectUrls: string[] = [];
// #1287 — the previous cleanup only set a flag. The request itself kept
// running, holding a connection slot for a tile that is no longer on
// screen, which on a several-hundred-photo gallery is most of them.
const controller = new AbortController();
// Determine which token to use based on context
if (!src) {
@@ -159,17 +175,35 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
}
}
const response = await fetch(fullImageUrl, {
credentials: 'include',
headers: Object.keys(headers).length ? headers : undefined,
});
// Queued (#1287). Without a cap, a 546-photo grid hands the browser
// several hundred simultaneous fetches and some never come back —
// pending forever, so nothing is logged and nothing is "failed".
//
// The BODY read has to happen inside the slot. `fetch` resolves as soon
// as the headers arrive, so releasing there would free the slot while
// the image bytes are still streaming on that connection — the cap
// would bound header round-trips and nothing else, which is not the
// workload that stalls a large gallery.
const blob = await withImageFetchSlot(async () => {
const response = await fetch(fullImageUrl, {
credentials: 'include',
headers: Object.keys(headers).length ? headers : undefined,
signal: controller.signal,
});
if (!response.ok) {
throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`);
}
if (!response.ok) {
throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`);
}
const blob = await response.blob();
return await response.blob();
}, { priority: queuePriority });
const objectUrl = URL.createObjectURL(blob);
// The effect may have been torn down while this was in flight. Revoke
// immediately rather than pushing onto an array nobody will read again.
if (aborted) {
URL.revokeObjectURL(objectUrl);
throw new Error('aborted');
}
objectUrls.push(objectUrl);
return objectUrl;
};
@@ -182,6 +216,10 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
setError(false);
}
} catch (err) {
// Torn down mid-flight (#1287): the abort is expected, not a failure.
// Returning here also stops the fallback below from firing a second
// request against an already-aborted signal.
if (aborted) return;
setIsLoading(false);
if (fallbackSrc && fallbackSrc !== src) {
try {
@@ -211,10 +249,13 @@ export const AuthenticatedImage: React.FC<AuthenticatedImageProps> = ({
// Cleanup function
return () => {
aborted = true;
// Free the connection slot rather than leaving the request to run for
// a tile that is gone (#1287).
controller.abort();
objectUrls.forEach((url) => URL.revokeObjectURL(url));
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [src, fallbackSrc, slug]);
}, [src, fallbackSrc, slug, queuePriority]);
// Effect to draw to canvas when image is loaded and canvas rendering is enabled
useEffect(() => {
@@ -1143,6 +1143,10 @@ export const PhotoLightbox: React.FC<PhotoLightboxProps> = ({
/>
) : (
<AuthenticatedImage
// The user is looking at this one right now: top tier,
// ahead of both the grid backlog and the neighbour
// prefetches enqueued around it (#1287).
queuePriority="high"
// Prefer the lightbox preview tier when the admin
// opted in (#492). Falls back to `url` (the
// original) when preview_url is null — happens
@@ -1171,6 +1175,9 @@ export const PhotoLightbox: React.FC<PhotoLightboxProps> = ({
style={{ flex: '0 0 33.3333%' }}
>
<AuthenticatedImage
// One arrow-key press from being on screen: ahead of the grid
// backlog, but never ahead of the slide being viewed (#1287).
queuePriority="prefetch"
// Same preview-prefer-with-fallback logic as the
// off-screen tile above (#492).
src={lightboxImageUrl(photo)}
@@ -0,0 +1,320 @@
/**
* The concurrent-image-fetch gate (#1287).
*
* Gallery grids are not virtualized, so a several-hundred-photo event mounts
* every tile and each one fetches. The reported failure was a gallery that
* loaded in bursts and then stopped with tiles blank forever, leaving no
* error and no failed request — the signature of requests queued indefinitely
* rather than rejected.
*
* What has to hold for this gate to be safe on that path: the cap is never
* exceeded, every slot is released however the task settles (a leaked slot
* would reproduce the exact stall it is meant to prevent), and the queue
* drains in order.
*/
import { describe, it, expect } from 'vitest';
import { withImageFetchSlot, __imageFetchQueueState } from '../imageFetchQueue';
const deferred = <T,>() => {
let resolve!: (v: T) => void;
let reject!: (e: unknown) => void;
const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; });
return { promise, resolve, reject };
};
const flush = () => new Promise((r) => setTimeout(r, 0));
describe('withImageFetchSlot', () => {
it('runs a task and returns its value', async () => {
await expect(withImageFetchSlot(async () => 'ok')).resolves.toBe('ok');
});
it('never exceeds the concurrency cap', async () => {
const { max } = __imageFetchQueueState();
let running = 0;
let peak = 0;
const gates = Array.from({ length: max * 4 }, () => deferred<void>());
const tasks = gates.map((g) => withImageFetchSlot(async () => {
running += 1;
peak = Math.max(peak, running);
await g.promise;
running -= 1;
}));
await flush();
expect(peak).toBe(max);
expect(__imageFetchQueueState().active).toBe(max);
gates.forEach((g) => g.resolve());
await Promise.all(tasks);
expect(peak).toBe(max);
});
it('queues the overflow and drains it', async () => {
const { max } = __imageFetchQueueState();
const gates = Array.from({ length: max + 3 }, () => deferred<void>());
const started: number[] = [];
const tasks = gates.map((g, i) => withImageFetchSlot(async () => {
started.push(i);
await g.promise;
}));
await flush();
expect(started).toHaveLength(max);
expect(__imageFetchQueueState().queued).toBe(3);
gates.forEach((g) => g.resolve());
await Promise.all(tasks);
expect(started).toHaveLength(max + 3);
});
it('starts queued tasks in FIFO order', async () => {
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const order: string[] = [];
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
const queued = ['a', 'b', 'c'].map((label) =>
withImageFetchSlot(async () => { order.push(label); }));
blockers.forEach((g) => g.resolve());
await Promise.all([...held, ...queued]);
expect(order).toEqual(['a', 'b', 'c']);
});
it('releases the slot when a task rejects', async () => {
// The regression this guards: a leaked slot on the failure path drains
// the pool over a long scroll and stalls the grid permanently — which is
// the bug, reintroduced by the fix for it.
const { max } = __imageFetchQueueState();
await Promise.all(
Array.from({ length: max * 2 }, () =>
withImageFetchSlot(async () => { throw new Error('boom'); }).catch(() => undefined))
);
expect(__imageFetchQueueState().active).toBe(0);
expect(__imageFetchQueueState().queued).toBe(0);
});
it('releases the slot when a task rejects with an abort', async () => {
const abort = Object.assign(new Error('aborted'), { name: 'AbortError' });
await withImageFetchSlot(async () => { throw abort; }).catch(() => undefined);
expect(__imageFetchQueueState().active).toBe(0);
});
it('propagates the rejection to the caller', async () => {
await expect(withImageFetchSlot(async () => { throw new Error('nope'); }))
.rejects.toThrow('nope');
});
it('returns to fully idle after a burst', async () => {
await Promise.all(
Array.from({ length: 50 }, (_, i) =>
withImageFetchSlot(async () => i).catch(() => undefined))
);
const state = __imageFetchQueueState();
expect(state.active).toBe(0);
expect(state.queued).toBe(0);
});
it('releases the slot when a task throws SYNCHRONOUSLY', async () => {
// A task that throws before returning a promise used to bypass the
// `.finally` release entirely, leaving `active` incremented. Repeat that
// and the pool is permanently exhausted — the stall this queue exists to
// prevent, reintroduced by its own release path.
const { max } = __imageFetchQueueState();
await Promise.all(
Array.from({ length: max * 2 }, () =>
withImageFetchSlot((() => { throw new Error('sync boom'); }) as unknown as () => Promise<void>)
.catch(() => undefined))
);
expect(__imageFetchQueueState().active).toBe(0);
expect(__imageFetchQueueState().queued).toBe(0);
await expect(withImageFetchSlot(async () => 'alive')).resolves.toBe('alive');
});
it('a synchronous throw from a QUEUED task does not wedge the pump', async () => {
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
// Queued behind the blockers, so it runs from pump() rather than inline.
const queued = withImageFetchSlot(
(() => { throw new Error('sync boom'); }) as unknown as () => Promise<void>
).catch(() => 'rejected');
blockers.forEach((g) => g.resolve());
await Promise.all(held);
await expect(queued).resolves.toBe('rejected');
expect(__imageFetchQueueState().active).toBe(0);
});
it('holds the slot until the task fully settles, not just its first await', async () => {
// #1287 review: `fetch` resolves on HEADERS. If the slot were released
// there, the cap would bound header round-trips while bodies streamed
// unbounded. The gate must stay held for the whole task.
const { max } = __imageFetchQueueState();
const headers = Array.from({ length: max }, () => deferred<void>());
const bodies = Array.from({ length: max }, () => deferred<void>());
let started = 0;
const tasks = headers.map((h, i) => withImageFetchSlot(async () => {
started += 1;
await h.promise; // "headers arrived"
await bodies[i].promise; // "body consumed"
}));
await flush();
expect(started).toBe(max);
const extra = withImageFetchSlot(async () => { started += 1; });
// Headers in, bodies still streaming: no slot may free up.
headers.forEach((h) => h.resolve());
await flush();
expect(started).toBe(max);
bodies.forEach((b) => b.resolve());
await Promise.all([...tasks, extra]);
expect(started).toBe(max + 1);
});
describe('priority', () => {
// Round-2 review: a single FIFO put the image the user just clicked
// behind every thumbnail already enqueued. On a 546-photo gallery — and
// Masonry/Timeline/Mosaic enqueue every card at once, since they pass no
// `lazy` — that is minutes of waiting for the one image being looked at.
it('serves a high-priority task ahead of an existing backlog', async () => {
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
const order: string[] = [];
// A realistic backlog of grid thumbnails...
const normal = Array.from({ length: 20 }, (_, i) =>
withImageFetchSlot(async () => { order.push(`thumb${i}`); }));
// ...then the lightbox opens.
const high = withImageFetchSlot(async () => { order.push('lightbox'); }, { priority: 'high' });
blockers.forEach((g) => g.resolve());
await Promise.all([...held, ...normal, high]);
expect(order[0]).toBe('lightbox');
});
it('serves the current slide before its own neighbour prefetches', async () => {
// Round-3 review: the lightbox's effects enqueue in slide order
// (previous, current, next). With neighbours on the same tier as the
// current slide, the PREVIOUS one took the first freed slot and the
// image actually on screen waited behind an off-screen prefetch.
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
const order: string[] = [];
// Enqueued in the order the lightbox mounts them.
const prev = withImageFetchSlot(async () => { order.push('prev'); }, { priority: 'prefetch' });
const current = withImageFetchSlot(async () => { order.push('current'); }, { priority: 'high' });
const next = withImageFetchSlot(async () => { order.push('next'); }, { priority: 'prefetch' });
blockers.forEach((g) => g.resolve());
await Promise.all([...held, prev, current, next]);
expect(order[0]).toBe('current');
// Neighbours keep their own FIFO below it.
expect(order.slice(1)).toEqual(['prev', 'next']);
});
it('serves prefetch ahead of grid thumbnails', async () => {
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
const order: string[] = [];
const thumbs = Array.from({ length: 5 }, (_, i) =>
withImageFetchSlot(async () => { order.push(`thumb${i}`); }));
const prefetch = withImageFetchSlot(async () => { order.push('prefetch'); }, { priority: 'prefetch' });
blockers.forEach((g) => g.resolve());
await Promise.all([...held, ...thumbs, prefetch]);
expect(order[0]).toBe('prefetch');
});
it('still respects the concurrency cap for high-priority work', async () => {
const { max } = __imageFetchQueueState();
let running = 0;
let peak = 0;
const gates = Array.from({ length: max * 3 }, () => deferred<void>());
const tasks = gates.map((g) => withImageFetchSlot(async () => {
running += 1; peak = Math.max(peak, running);
await g.promise;
running -= 1;
}, { priority: 'high' }));
await flush();
expect(peak).toBe(max);
gates.forEach((g) => g.resolve());
await Promise.all(tasks);
});
it('keeps high-priority tasks in FIFO order among themselves', async () => {
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
const order: string[] = [];
const queued = ['a', 'b', 'c'].map((label) =>
withImageFetchSlot(async () => { order.push(label); }, { priority: 'high' }));
blockers.forEach((g) => g.resolve());
await Promise.all([...held, ...queued]);
expect(order).toEqual(['a', 'b', 'c']);
});
it('does not starve the normal tier once the high tier drains', async () => {
const { max } = __imageFetchQueueState();
const blockers = Array.from({ length: max }, () => deferred<void>());
const held = blockers.map((g) => withImageFetchSlot(() => g.promise));
await flush();
const done: string[] = [];
const normal = withImageFetchSlot(async () => { done.push('normal'); });
const high = withImageFetchSlot(async () => { done.push('high'); }, { priority: 'high' });
blockers.forEach((g) => g.resolve());
await Promise.all([...held, normal, high]);
expect(done).toEqual(['high', 'normal']);
expect(__imageFetchQueueState().active).toBe(0);
});
});
it('keeps working after a mixed burst of successes and failures', async () => {
await Promise.all(
Array.from({ length: 30 }, (_, i) =>
withImageFetchSlot(async () => {
if (i % 3 === 0) throw new Error('boom');
return i;
}).catch(() => undefined))
);
expect(__imageFetchQueueState().active).toBe(0);
await expect(withImageFetchSlot(async () => 'still works')).resolves.toBe('still works');
});
});
+125
View File
@@ -0,0 +1,125 @@
/**
* A process-wide gate on concurrent authenticated image fetches (#1287).
*
* Gallery grids are NOT virtualized: a 546-photo event puts 546 `PhotoCard`s
* in the DOM, each mounting its own `AuthenticatedImage` the moment its
* IntersectionObserver fires. Scrolling through such a gallery therefore hands
* the browser several hundred simultaneous `fetch` calls with nothing between
* them and the connection pool.
*
* That is not a load the browser degrades gracefully under. The reported
* symptom is a gallery that loads in bursts, then stops with tiles blank
* indefinitely — no console error, no failed request, nothing in the backend
* log, and no recovery from scrolling. A request that is queued forever is
* *pending*, not failed, which is exactly why it leaves no trace.
*
* Bounding it here fixes the shape of the problem rather than one instance of
* it: at most MAX_CONCURRENT requests are ever outstanding, the rest wait in
* an ordinary FIFO, and every slot is released in a `finally` so a rejection
* or an abort cannot leak one. Six matches what browsers allow per origin on
* HTTP/1.1 anyway, so throughput on a healthy connection is unchanged — the
* reporter measured the browser already effectively doing ~8 at a time.
*
* Deliberately module-level, not per-component: the point is a cap across the
* whole page, which is the thing that was missing.
*/
const MAX_CONCURRENT = 6;
let active = 0;
// Two tiers, drained high-first. A strict single FIFO meant the image the
// user just clicked queued behind every thumbnail already enqueued — on a
// 546-photo gallery that is minutes of waiting for the one image they are
// actually looking at. The layouts that render every card at once (Masonry,
// Timeline, Mosaic pass no `lazy`) make that the normal case, not the edge.
const waitingHigh: Array<() => void> = [];
const waitingPrefetch: Array<() => void> = [];
const waiting: Array<() => void> = [];
/** Highest non-empty tier first; FIFO within a tier. */
function nextWaiter() {
if (waitingHigh.length > 0) return waitingHigh.shift();
if (waitingPrefetch.length > 0) return waitingPrefetch.shift();
return waiting.shift();
}
/** Hand the next waiter a slot, if anyone is queued and one is free. */
function pump() {
while (active < MAX_CONCURRENT
&& (waitingHigh.length > 0 || waitingPrefetch.length > 0 || waiting.length > 0)) {
const next = nextWaiter();
if (!next) break;
active += 1;
next();
}
}
/**
* Run `task` once a slot is free. The slot is released when the returned
* promise settles, whatever way it settles.
*
* Callers that no longer need their result should still let it run — the work
* is already cheap, and an AbortController on the underlying fetch is the
* right way to cancel, not dropping the slot on the floor.
*/
export interface ImageFetchOptions {
/**
* Three tiers, drained highest-first:
*
* 'high' the image on screen right now — the current lightbox slide
* 'prefetch' one interaction away — the lightbox neighbours
* 'normal' grid thumbnails
*
* The middle tier exists because the lightbox's effects enqueue in slide
* order (previous, current, next). Sharing one tier with its neighbours
* meant the PREVIOUS slide could take the first freed slot while the image
* the user is actually looking at stayed queued behind it.
*
* Never mark thumbnails high; that is one queue again.
*/
priority?: 'high' | 'prefetch' | 'normal';
}
export function withImageFetchSlot<T>(
task: () => Promise<T>,
{ priority = 'normal' }: ImageFetchOptions = {},
): Promise<T> {
return new Promise<T>((resolve, reject) => {
const run = () => {
// Promise.resolve().then(task) rather than task() directly: a task that
// throws SYNCHRONOUSLY would otherwise never reach the `.finally`, and
// `active` would stay incremented. Repeat that and the pool is
// permanently exhausted — the precise failure this queue exists to
// prevent, reintroduced by its own release path.
Promise.resolve()
.then(task)
.then(resolve, reject)
.finally(() => {
active -= 1;
pump();
});
};
if (active < MAX_CONCURRENT) {
active += 1;
run();
} else if (priority === 'high') {
waitingHigh.push(run);
} else if (priority === 'prefetch') {
waitingPrefetch.push(run);
} else {
waiting.push(run);
}
});
}
/** Test-only visibility into the gate. */
export function __imageFetchQueueState() {
return {
active,
queued: waiting.length + waitingPrefetch.length + waitingHigh.length,
queuedHigh: waitingHigh.length,
queuedPrefetch: waitingPrefetch.length,
max: MAX_CONCURRENT,
};
}