diff --git a/frontend/src/components/common/AuthenticatedImage.tsx b/frontend/src/components/common/AuthenticatedImage.tsx index ea862703..bd3bc8fe 100644 --- a/frontend/src/components/common/AuthenticatedImage.tsx +++ b/frontend/src/components/common/AuthenticatedImage.tsx @@ -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 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 = ({ @@ -56,6 +67,7 @@ export const AuthenticatedImage: React.FC = ({ protectionLevel, useEnhancedProtection, onLoad, + queuePriority = 'normal', ...props }) => { const unusedProps = { @@ -108,6 +120,10 @@ export const AuthenticatedImage: React.FC = ({ 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 = ({ } } - 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 = ({ 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 = ({ // 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(() => { diff --git a/frontend/src/components/gallery/PhotoLightbox.tsx b/frontend/src/components/gallery/PhotoLightbox.tsx index 962bf2d7..d5c475c0 100644 --- a/frontend/src/components/gallery/PhotoLightbox.tsx +++ b/frontend/src/components/gallery/PhotoLightbox.tsx @@ -1143,6 +1143,10 @@ export const PhotoLightbox: React.FC = ({ /> ) : ( = ({ style={{ flex: '0 0 33.3333%' }} > () => { + let resolve!: (v: T) => void; + let reject!: (e: unknown) => void; + const promise = new Promise((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()); + + 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()); + 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()); + 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) + .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()); + 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 + ).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()); + const bodies = Array.from({ length: max }, () => deferred()); + 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()); + 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()); + 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()); + 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()); + + 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()); + 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()); + 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'); + }); +}); diff --git a/frontend/src/utils/imageFetchQueue.ts b/frontend/src/utils/imageFetchQueue.ts new file mode 100644 index 00000000..da3d37a3 --- /dev/null +++ b/frontend/src/utils/imageFetchQueue.ts @@ -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( + task: () => Promise, + { priority = 'normal' }: ImageFetchOptions = {}, +): Promise { + return new Promise((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, + }; +}