diff --git a/.gitignore b/.gitignore index cc037f09abd..33967877853 100644 --- a/.gitignore +++ b/.gitignore @@ -190,3 +190,8 @@ installer/InvokeAI-Installer/ # Weblate configuration file weblate.ini + +# Node dependencies and tool caches. The web app has its own .gitignore, but a repo-root +# node_modules/ (created by running a JS tool from the repo root) was not covered by anything, +# so vitest cache artifacts could be committed by accident. +node_modules/ diff --git a/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.test.ts b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.test.ts new file mode 100644 index 00000000000..2674f340664 --- /dev/null +++ b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest'; + +import { coalesceRanges } from './useBoundedRangeRetry'; + +describe('coalesceRanges', () => { + it('returns empty and single-range inputs as-is', () => { + expect(coalesceRanges([])).toEqual([]); + expect(coalesceRanges([{ startIndex: 3, endIndex: 7 }])).toEqual([{ startIndex: 3, endIndex: 7 }]); + }); + + it('merges overlapping ranges', () => { + expect( + coalesceRanges([ + { startIndex: 0, endIndex: 5 }, + { startIndex: 3, endIndex: 8 }, + ]) + ).toEqual([{ startIndex: 0, endIndex: 8 }]); + }); + + it('merges adjacent ranges', () => { + expect( + coalesceRanges([ + { startIndex: 0, endIndex: 2 }, + { startIndex: 3, endIndex: 5 }, + ]) + ).toEqual([{ startIndex: 0, endIndex: 5 }]); + }); + + it('collapses duplicates — the per-retry-cycle growth case', () => { + // Pre-change, each retry cycle appended the viewport range again, so the pending state grew + // by a duplicate entry per cycle for as long as the failure persisted. + const range = { startIndex: 10, endIndex: 30 }; + expect(coalesceRanges([range, range, range, range])).toEqual([range]); + }); + + it('absorbs contained ranges', () => { + expect( + coalesceRanges([ + { startIndex: 0, endIndex: 10 }, + { startIndex: 2, endIndex: 4 }, + ]) + ).toEqual([{ startIndex: 0, endIndex: 10 }]); + }); + + it('keeps disjoint ranges separate and sorts them', () => { + expect( + coalesceRanges([ + { startIndex: 6, endIndex: 8 }, + { startIndex: 0, endIndex: 2 }, + ]) + ).toEqual([ + { startIndex: 0, endIndex: 2 }, + { startIndex: 6, endIndex: 8 }, + ]); + }); + + it('does not mutate its input', () => { + const input = [ + { startIndex: 0, endIndex: 5 }, + { startIndex: 3, endIndex: 8 }, + ]; + coalesceRanges(input); + expect(input).toEqual([ + { startIndex: 0, endIndex: 5 }, + { startIndex: 3, endIndex: 8 }, + ]); + }); +}); diff --git a/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts new file mode 100644 index 00000000000..89415b9dd2a --- /dev/null +++ b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts @@ -0,0 +1,208 @@ +import { useCallback, useEffect, useLayoutEffect, useRef } from 'react'; +import type { ListRange } from 'react-virtuoso'; +import { $isConnected } from 'services/events/stores'; + +const RETRY_INITIAL_DELAY_MS = 1_000; +const RETRY_MAX_DELAY_MS = 16_000; +const RETRY_MAX_ATTEMPTS = 5; +/** + * Floor on how often a socket reconnect may re-arm an exhausted budget. A reconnect is evidence + * the backend is answering, but the socket and the REST API can disagree: a proxy can route the + * websocket to a healthy replica while REST hits a sick one, and a crash-looping container + * completes a handshake on every restart. Without this floor the budget would be per-reconnect + * rather than per-outage, and a flapping socket would turn the bounded retry back into a stream. + */ +const RETRY_REARM_COOLDOWN_MS = 60_000; + +/** + * Merge overlapping or adjacent ranges into a minimal, sorted, disjoint set. + * + * This is what bounds the retry state: failed ranges are accumulated as a coalesced union, so + * repeated failures over the same viewport collapse into one entry instead of growing by a + * duplicate range per retry cycle. + */ +export const coalesceRanges = (ranges: ListRange[]): ListRange[] => { + if (ranges.length <= 1) { + return ranges; + } + const sorted = [...ranges].sort((a, b) => a.startIndex - b.startIndex); + const first = sorted[0]!; + const coalesced: ListRange[] = [{ startIndex: first.startIndex, endIndex: first.endIndex }]; + for (let i = 1; i < sorted.length; i++) { + const range = sorted[i]!; + const last = coalesced[coalesced.length - 1]!; + if (range.startIndex <= last.endIndex + 1) { + last.endIndex = Math.max(last.endIndex, range.endIndex); + } else { + coalesced.push({ startIndex: range.startIndex, endIndex: range.endIndex }); + } + } + return coalesced; +}; + +interface UseBoundedRangeRetryReturn { + /** + * Report a failed bulk fetch, with the ranges it was fetching. Schedules a single retry with + * exponential backoff (1s, 2s, ... capped at 16s); while one is already scheduled, additional + * failures only merge their ranges into it. After RETRY_MAX_ATTEMPTS consecutive failures the + * hook stops scheduling and parks the ranges until the budget is reset. + */ + onFetchFailure: (ranges: ListRange[]) => void; + /** + * End the current failure streak. Call when a fetch succeeds (the backend is answering again) + * and on new user input (a fresh range report), so a list that gave up resumes retrying as the + * user scrolls. + */ + resetRetryBudget: () => void; +} + +/** + * Bounded, backoff-driven retry of failed range fetches. + * + * The range-based fetching hooks are the ONLY fetcher for their rows (the row components consume + * the cache with `skip: isUninitialized`), so a failed bulk fetch must be retried or the rows stay + * placeholders until the user happens to scroll. But an unbounded retry is a fixed-rate request + * storm from every open tab against a backend that is trying to come back up. This hook bounds it: + * exponential backoff between attempts, a cap on consecutive failures, and coalesced accumulation + * of the failed ranges. + * + * Giving up is not the same as dying. Ranges abandoned when the budget runs out are parked (as a + * coalesced union, so parking them is bounded too) and restored on the next event that says the + * backend is answering again: a successful fetch, a fresh range report from the user, or a socket + * reconnect. The reconnect signal is what covers the case the retry budget cannot — a restart that + * takes longer than the ~31s schedule, where an idle user is watching a gallery whose `imageNames` + * never change and so has no other reason to re-run the fetch effect. Success and user input are + * self-limiting signals; a reconnect is not, so it re-arms at most once per + * RETRY_REARM_COOLDOWN_MS and only when there is something parked to heal. + * + * `restoreRanges` is invoked with the coalesced union of every range that failed since the last + * retry. It is read through a ref, so an unstable callback cannot churn `onFetchFailure`; the ref + * is updated in a layout effect, so a restore firing between render and commit still sees the + * previous render's closure. + */ +export const useBoundedRangeRetry = ( + restoreRanges: (failedRanges: ListRange[]) => void +): UseBoundedRangeRetryReturn => { + const stateRef = useRef<{ + attempts: number; + failedRanges: ListRange[]; + abandonedRanges: ListRange[]; + timeoutId: ReturnType | null; + isMounted: boolean; + lastRearmAt: number; + }>({ + attempts: 0, + failedRanges: [], + abandonedRanges: [], + timeoutId: null, + isMounted: true, + lastRearmAt: 0, + }); + + // Read `restoreRanges` through a ref so an unstable callback cannot churn `onFetchFailure` (and + // through it the caller's fetch callback, its throttle, and the effect that drives it) on every + // render. The hook's contract shouldn't depend on the caller remembering to useCallback. + const restoreRangesRef = useRef(restoreRanges); + useLayoutEffect(() => { + restoreRangesRef.current = restoreRanges; + }, [restoreRanges]); + + useEffect(() => { + const state = stateRef.current; + state.isMounted = true; + return () => { + // A bulk fetch may still be in flight and reject after unmount; without this flag its + // `onFetchFailure` would schedule a fresh backoff timer that no cleanup will ever reach. + state.isMounted = false; + if (state.timeoutId !== null) { + clearTimeout(state.timeoutId); + // Null the sentinel too: effect cleanup can run while the instance (and this ref) + // survives — Fast Refresh, or a re-suspending Suspense/Activity boundary. A stale + // non-null timeoutId would make every future onFetchFailure early-return, silently + // disabling retry for the lifetime of the instance. + state.timeoutId = null; + } + }; + }, []); + + const takeAbandonedRanges = useCallback((): ListRange[] | null => { + const state = stateRef.current; + if (!state.isMounted || state.abandonedRanges.length === 0) { + return null; + } + const abandonedRanges = state.abandonedRanges; + state.abandonedRanges = []; + return abandonedRanges; + }, []); + + const resetRetryBudget = useCallback(() => { + const state = stateRef.current; + if (!state.isMounted) { + return; + } + state.attempts = 0; + const abandonedRanges = takeAbandonedRanges(); + if (abandonedRanges) { + restoreRangesRef.current(abandonedRanges); + } + }, [takeAbandonedRanges]); + + useEffect(() => { + // A reconnect means the backend is answering again. Nothing else re-arms an exhausted budget + // for an idle user: in production `socketConnected` only invalidates `FetchOnReconnect` when + // the queue status changed, and even then RTK Query's structural sharing hands the gallery + // back the same `imageNames` reference, so no dependency of the fetch effect changes. + // `listen` fires on transitions only, so this runs on reconnect, not on the initial connect. + return $isConnected.listen((isConnected) => { + if (!isConnected) { + return; + } + const state = stateRef.current; + // Unlike a success or a scroll, reconnects are not self-limiting — see the cooldown's note. + // Both guards matter: re-arming with nothing parked would zero `attempts` mid-streak, so a + // socket flapping faster than the backoff would pin the delay at 1s indefinitely. + const now = Date.now(); + if (now - state.lastRearmAt < RETRY_REARM_COOLDOWN_MS) { + return; + } + const abandonedRanges = takeAbandonedRanges(); + if (!abandonedRanges) { + return; + } + state.lastRearmAt = now; + state.attempts = 0; + restoreRangesRef.current(abandonedRanges); + }); + }, [takeAbandonedRanges]); + + const onFetchFailure = useCallback((ranges: ListRange[]) => { + const state = stateRef.current; + if (!state.isMounted) { + return; + } + state.failedRanges = coalesceRanges([...state.failedRanges, ...ranges]); + if (state.timeoutId !== null) { + // A retry is already scheduled; it will pick up the merged ranges when it fires. + return; + } + if (state.attempts >= RETRY_MAX_ATTEMPTS) { + // Budget exhausted — stop scheduling, but park the ranges (coalesced, so parking is bounded) + // rather than dropping them, so a reconnect, a later success, or a scroll can heal the rows. + state.abandonedRanges = coalesceRanges([...state.abandonedRanges, ...state.failedRanges]); + state.failedRanges = []; + return; + } + state.attempts += 1; + const delay = Math.min(RETRY_INITIAL_DELAY_MS * 2 ** (state.attempts - 1), RETRY_MAX_DELAY_MS); + state.timeoutId = setTimeout(() => { + state.timeoutId = null; + const failedRanges = state.failedRanges; + state.failedRanges = []; + if (failedRanges.length > 0) { + restoreRangesRef.current(failedRanges); + } + }, delay); + }, []); + + return { onFetchFailure, resetRetryBudget }; +}; diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts index 6cec16aa043..bb447b847a7 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts @@ -1,6 +1,524 @@ -import { describe, expect, it } from 'vitest'; +// @vitest-environment happy-dom +import { act, createElement, type FC } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import type { ListRange } from 'react-virtuoso'; +import { $isConnected } from 'services/events/stores'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { getVideoPrefetchOptions, hasCachedVideoDTO } from './useRangeBasedImageFetching'; +import { getVideoPrefetchOptions, hasCachedVideoDTO, useRangeBasedImageFetching } from './useRangeBasedImageFetching'; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const mocks = vi.hoisted(() => ({ + // Args of every getImageDTOsByNames call, in order. + imageFetches: [] as string[][], + // Names with a getImageDTO cache entry, as reported by selectCachedArgsForQuery. + cachedImageNames: [] as string[], + // When true, a successful fetch upserts the requested names into the cache, like + // getImageDTOsByNames.onQueryStarted does. When false, requested names never land in the + // cache — the deleted-image / multiuser-filtered case that drove the pre-fix request stream. + cacheLands: true, + // When true, the mutation rejects, like a backend restart or a 502 from a reverse proxy. + failFetches: false, + // When true, the mutation returns a promise the test rejects by hand, so a rejection can be + // delivered at a chosen moment (e.g. after unmount) rather than on the next microtask. + manualFailure: false, + rejectPending: [] as (() => void)[], +})); + +vi.mock('app/store/storeHooks', () => { + const store = { getState: () => ({}), dispatch: () => undefined }; + return { useAppStore: () => store }; +}); + +vi.mock('features/gallery/store/types', () => ({ + isVideoName: (name: string) => name.endsWith('.mp4'), +})); + +vi.mock('services/api/endpoints/images', () => { + const trigger = (arg: { image_names: string[] }) => { + mocks.imageFetches.push(arg.image_names); + if (mocks.manualFailure) { + let reject!: () => void; + const pending = new Promise((_, rej) => { + reject = () => rej(new Error('fetch failed')); + }); + pending.catch(() => undefined); + mocks.rejectPending.push(reject); + return { unwrap: () => pending.then((r) => r) }; + } + // Like the real mutation: onQueryStarted upserts when the request fulfills, whether or not + // the caller unwraps, and only the promise returned by unwrap() surfaces the rejection. + const settled = mocks.failFetches + ? Promise.reject(new Error('fetch failed')) + : Promise.resolve().then(() => { + if (mocks.cacheLands) { + mocks.cachedImageNames.push(...arg.image_names); + } + return []; + }); + settled.catch(() => undefined); + return { unwrap: () => settled.then((r) => r) }; + }; + // RTK Query's mutation trigger is referentially stable across renders; the hook's fetchItems + // callback (and therefore its throttle and effect) depend on that. + const result = [trigger]; + return { + imagesApi: { util: { selectCachedArgsForQuery: () => mocks.cachedImageNames } }, + useGetImageDTOsByNamesMutation: () => result, + }; +}); + +vi.mock('services/api/endpoints/videos', () => ({ + videosApi: { + util: { selectCachedArgsForQuery: () => [] }, + endpoints: { getVideoDTO: { select: () => () => ({ data: undefined }), initiate: () => ({ type: 'noop' }) } }, + }, +})); + +const IMAGE_NAMES = ['a.png', 'b.png', 'c.png']; +const THROTTLE_MS = 500; + +describe('useRangeBasedImageFetching', () => { + let root: Root | null = null; + let renderCount = 0; + let hookReturn: ReturnType; + + // One stable component type, so re-rendering with new props updates the existing instance + // instead of remounting it — a remount would silently reset the state under test. + const Harness: FC<{ imageNames: string[]; enabled: boolean }> = ({ imageNames, enabled }) => { + renderCount++; + hookReturn = useRangeBasedImageFetching({ imageNames, enabled }); + return null; + }; + + const renderHook = (imageNames: string[], enabled: boolean) => { + root = createRoot(document.createElement('div')); + act(() => { + root!.render(createElement(Harness, { imageNames, enabled })); + }); + }; + + const rerenderHook = (imageNames: string[], enabled: boolean) => { + act(() => { + root!.render(createElement(Harness, { imageNames, enabled })); + }); + }; + + const scrollTo = (range: ListRange) => { + act(() => { + hookReturn.onRangeChanged(range); + }); + }; + + // Advance fake time in small steps, flushing React work (renders + effects) between steps. A + // single long advance would defer all effect re-runs to the end of the act scope, which breaks + // the feedback cycle this suite exists to detect: state update -> effect -> throttle -> fetch -> + // state update. Stepping mimics real event-loop turns, letting a loop sustain itself if the + // code allows one. + const advance = async (ms: number) => { + const step = 250; + for (let elapsed = 0; elapsed < ms; elapsed += step) { + await act(async () => { + await vi.advanceTimersByTimeAsync(step); + }); + } + }; + + beforeEach(() => { + vi.useFakeTimers(); + mocks.imageFetches = []; + mocks.cachedImageNames = []; + mocks.cacheLands = true; + mocks.failFetches = false; + mocks.manualFailure = false; + mocks.rejectPending = []; + renderCount = 0; + $isConnected.set(false); + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + root = null; + } + $isConnected.set(false); + vi.useRealTimers(); + }); + + it('fetches uncached names for a reported range, then goes quiet', async () => { + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + + expect(mocks.imageFetches).toEqual([IMAGE_NAMES]); + + // Regression: clearing pendingRanges with a fresh `[]` (a new identity every time) re-ran the + // effect, re-armed the throttle, and cleared again — a self-sustaining render loop that + // re-rendered every ~500ms for as long as the grid was mounted, with no user input. Once the + // range has been handled and the throttle has drained, both renders and fetches must stop. + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.imageFetches).toEqual([IMAGE_NAMES]); + }); + + it('does not loop even while the grid is mounted with nothing to fetch', async () => { + // Pre-fix, the loop ran from mount even with no ranges reported, because the clear was + // unconditional and every pass installed a new [] identity. + renderHook(IMAGE_NAMES, true); + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.imageFetches).toEqual([]); + }); + + it('stops re-requesting names that never land in the cache', async () => { + // onQueryStarted upserts only the DTOs the server actually returned, so a requested name that + // comes back missing (deleted image, multiuser ownership filter) never gets a cache entry. + // Pre-fix, the render loop re-requested such names every ~500ms, forever. + mocks.cacheLands = false; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + + // The range-change pass fetches once, and clearing pendingRanges ([range] -> EMPTY_ARRAY) is a + // real state change, so one follow-up pass may re-check the cache and re-request the + // still-missing names. After that the state is stable and the stream must stop — pre-fix it + // continued at one request per throttle window, forever. + expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(1); + expect(mocks.imageFetches.length).toBeLessThanOrEqual(2); + const settledFetches = mocks.imageFetches.length; + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches.length).toBe(settledFetches); + }); + + it('retries a failed fetch until it succeeds, then goes quiet', async () => { + // The pre-fix loop was also an accidental retry, and this bulk fetch is the only fetcher for + // these rows (ImageAtPosition subscribes with `skip: isUninitialized`). Without an explicit + // retry, a transient failure would leave grey placeholders until the user happens to scroll. + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + + // The initial failure produces a fetch at the leading and trailing edges of the throttle + // window, and the first backoff retry (1s) restores the ranges for at least one more pass. + // Without the retry, clearing pendingRanges after the failed fetch still re-runs the effect + // once, so the count caps at two — three or more requires the retry restoring the ranges. + expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(3); + expect(mocks.cachedImageNames).toEqual([]); + + mocks.failFetches = false; + await advance(THROTTLE_MS * 4); + expect(mocks.cachedImageNames).toEqual(IMAGE_NAMES); + + const fetchesAfterRecovery = mocks.imageFetches.length; + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches.length).toBe(fetchesAfterRecovery); + expect(renderCount).toBe(settledRenders); + }); + + it('stops retrying when failure is sustained, instead of storming', async () => { + // Review finding on the original retry: restoring the ranges immediately meant a sustained + // backend outage produced a request every throttle window, forever — a fixed-rate storm from + // every open tab against a backend trying to come back up. The bounded retry backs off + // (1s, 2s, 4s, 8s, 16s) and gives up after five consecutive scheduled retries, so the request + // stream must terminate. Each retry pass produces at most a leading and a trailing fetch, + // bounding the total at 12; six requires every backoff retry to have actually fired. + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(35_000); + + expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(6); + expect(mocks.imageFetches.length).toBeLessThanOrEqual(12); + + const settledFetches = mocks.imageFetches.length; + const settledRenders = renderCount; + await advance(30_000); + expect(mocks.imageFetches.length).toBe(settledFetches); + expect(renderCount).toBe(settledRenders); + }); + + it('resumes retrying after giving up when the user scrolls', async () => { + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(35_000); + const fetchesAfterGiveUp = mocks.imageFetches.length; + + // A new range report is fresh user input: it restarts the retry budget, so the grid does not + // stay dead until reload. With the budget still exhausted, only the scroll-triggered fetch and + // its trailing companion would fire — three or more new fetches requires the backoff schedule + // to have restarted. + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(2_000); + expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(fetchesAfterGiveUp + 3); + }); + + it('restores parked ranges on the next scroll when the socket never dropped', async () => { + // Review finding (coverage): parked ranges are restored by three signals — a socket reconnect, + // a later success, and a fresh range report — but only the reconnect was pinned. `resumes + // retrying after giving up when the user scrolls` re-reports the *same* range, which + // `lastRange` re-fetches whether or not the parked set was restored, so deleting the restore + // from `resetRetryBudget` left the suite green. The distinction only shows for a range that is + // parked but no longer on screen, after a recovery the socket never observed — a transient 502 + // from a reverse proxy, say, where the websocket stays up throughout and the reconnect path + // never fires. + const imageNames = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png', 'g.png', 'h.png', 'i.png']; + $isConnected.set(true); + mocks.failFetches = true; + renderHook(imageNames, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(35_000); + expect(mocks.cachedImageNames).toEqual([]); + + // REST answers again with no socket transition, so the scroll is the only signal that can heal + // the parked range. + mocks.failFetches = false; + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(THROTTLE_MS * 4); + + // The rows the user scrolled past during the outage land along with the new viewport. Without + // the restore only g-i would be fetched and a-c would stay grey placeholders permanently. + expect([...mocks.cachedImageNames].sort()).toEqual(['a.png', 'b.png', 'c.png', 'g.png', 'h.png', 'i.png']); + }); + + it('heals a grid that gave up when the socket reconnects, with no user input', async () => { + // Review finding: the retry budget ends ~31s after the first failure, but an InvokeAI restart + // (config load, DB migrations, model scan) routinely takes longer. For an idle user nothing + // else re-arms it — in production `socketConnected` only invalidates `FetchOnReconnect` when + // the queue status changed, and RTK Query's structural sharing hands back the same + // `imageNames` reference either way, so no dependency of the fetch effect changes and + // `enabled` (`!isLoading`) does not toggle on a refetch. Ranges abandoned by the exhausted + // budget are parked, not dropped, and the socket reconnect restores them. + $isConnected.set(true); + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + + // Backend goes down: the socket drops and the retry budget runs out while it is down. + $isConnected.set(false); + await advance(35_000); + const fetchesAfterGiveUp = mocks.imageFetches.length; + await advance(30_000); + expect(mocks.imageFetches.length).toBe(fetchesAfterGiveUp); + expect(mocks.cachedImageNames).toEqual([]); + + // Backend comes back, well past the retry budget. No scroll, no change to imageNames. + mocks.failFetches = false; + act(() => { + $isConnected.set(true); + }); + await advance(THROTTLE_MS * 4); + + expect(mocks.cachedImageNames).toEqual(IMAGE_NAMES); + + // And the heal must settle. Restoring the parked ranges without emptying the parked set would + // make every later success restore them again — success -> restore -> fetch -> success — a + // loop that the cache assertion alone cannot see. + const fetchesAfterHeal = mocks.imageFetches.length; + await advance(30_000); + expect(mocks.imageFetches.length).toBe(fetchesAfterHeal); + }); + + it('empties the parked set when it heals, even if the rows never reach the cache', async () => { + // The parked set is handed to the restore and cleared in one step. Restoring without clearing + // it looks harmless while the rows do land in the cache — the follow-up pass finds nothing to + // request — but a name the server never returns is uncached on every pass, so a parked set + // that outlived its restore would be re-fetched by every later success. + $isConnected.set(true); + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + $isConnected.set(false); + await advance(35_000); + + // The backend answers again, but these rows never land in the cache (deleted, or filtered out + // for this user). + mocks.failFetches = false; + mocks.cacheLands = false; + act(() => { + $isConnected.set(true); + }); + await advance(THROTTLE_MS * 4); + + const fetchesAfterHeal = mocks.imageFetches.length; + await advance(60_000); + expect(mocks.imageFetches.length).toBe(fetchesAfterHeal); + }); + + it('does not turn a flapping socket into a request stream', async () => { + // Review finding: re-arming on every reconnect made the budget per-reconnect rather than + // per-outage. A socket that keeps completing a handshake while REST stays broken — a + // crash-looping container, uvicorn accepting connections before startup finishes, a proxy + // routing the websocket to a healthy replica and REST to a sick one — would then pin the + // backoff at its shortest delay for as long as the flapping lasted. The re-arm is now floored + // at one per RETRY_REARM_COOLDOWN_MS (60s) and only fires when there is something parked. + $isConnected.set(true); + mocks.failFetches = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + + // Five minutes of flapping every 5s, REST failing throughout, no user input. + for (let i = 0; i < 60; i++) { + act(() => { + $isConnected.set(false); + }); + await advance(2_500); + act(() => { + $isConnected.set(true); + }); + await advance(2_500); + } + + // Design intent with no flapping at all is 12 requests (one bounded streak). Five minutes of + // flapping buys at most five re-arms, each worth another bounded streak. Pre-fix this ran at + // the flap rate and measured 240. + expect(mocks.imageFetches.length).toBeLessThanOrEqual(80); + }); + + it('does not schedule a retry for a fetch that rejects after unmount', async () => { + // Review finding: the unmount cleanup clears the pending timer, but a mutation still in flight + // rejects afterwards, reaching onFetchFailure on a dead instance and arming a fresh timer of + // up to 16s that no cleanup will ever reach. Triggered by closing the gallery panel or + // switching tabs while the backend is down. + mocks.manualFailure = true; + renderHook(IMAGE_NAMES, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + expect(mocks.rejectPending.length).toBeGreaterThan(0); + + // Unmount with the request still in flight, then let it reject. + act(() => { + root!.unmount(); + }); + root = null; + const timersAfterUnmount = vi.getTimerCount(); + + for (const reject of mocks.rejectPending) { + reject(); + } + // Deliver the rejection without advancing the clock, so a backoff timer armed by it (>=1s) + // is still pending and countable rather than already fired and cleared. + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(vi.getTimerCount()).toBe(timersAfterUnmount); + }); + + it('does not accumulate ranges reported while disabled', async () => { + // Review finding: the `!enabled` guard returned before the clear, so every range reported + // while disabled stayed in pendingRanges and the first enabled pass scanned all of them. + const imageNames = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png', 'g.png', 'h.png', 'i.png']; + renderHook(imageNames, false); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([]); + + // Enable. In production `enabled` is `!isLoading`, so it flips as the names arrive — a new + // array identity, which is what re-runs the fetch effect (`throttledFetchItems` is + // referentially stable across callback changes, so `enabled` alone does not re-run it). + // The pass that follows must cover the last reported viewport (d-f) and nothing else: the + // earlier range (a-c), long scrolled past, must not still be sitting in pendingRanges. + rerenderHook([...imageNames], true); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches.flat().sort()).toEqual(['d.png', 'e.png', 'f.png']); + + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches.flat().sort()).toEqual(['d.png', 'e.png', 'f.png', 'g.png', 'h.png', 'i.png']); + }); + + // Review finding: pinned to a single delay, this passed only on a lucky phase of the + // throttle/backoff alignment. Sweeping it covers the batch in which the backoff retry and the + // throttle's trailing edge land together — the interleaving in which an absolute clear discards + // the restore. + it.each([500, 600, 750, 1_000, 1_250])( + 'recovers a range that failed while the user was scrolling elsewhere (scroll at t=%dms)', + async (delayBeforeScroll) => { + // Review finding on the original retry: the catch (`prev.length > 0 ? prev : ranges`) + // dropped the failed range whenever another range had been reported in the meantime — rows + // the user had scrolled past stayed blank placeholders. The retry now merges the failed + // ranges with whatever is pending instead of choosing one side, and the clear only fires + // when the pending state is still the array the pass consumed, so both ranges end up + // fetched with no further user input. + const imageNames = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png', 'g.png', 'h.png', 'i.png']; + mocks.failFetches = true; + renderHook(imageNames, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(delayBeforeScroll); + + // The backend recovers and the user scrolls to a disjoint range while the backoff retry for + // the failed range is still pending. + mocks.failFetches = false; + scrollTo({ startIndex: 6, endIndex: 8 }); + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(10_000); + + // Both the failed range and the new one land, with no user input beyond the one scroll — + // and nothing outside the reported ranges is fetched. + expect([...mocks.cachedImageNames].sort()).toEqual(['a.png', 'b.png', 'c.png', 'g.png', 'h.png', 'i.png']); + } + ); + + it('still fetches for new ranges after settling', async () => { + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches).toEqual([['a.png', 'b.png', 'c.png']]); + + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([ + ['a.png', 'b.png', 'c.png'], + ['d.png', 'e.png', 'f.png'], + ]); + }); + + it('fetches every range reported within a throttle window, not just the last', async () => { + // onRangeChanged accumulates ranges into pendingRanges precisely so that ranges reported + // mid-window are not dropped when the trailing invocation only sees the latest call's args. + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']]); + }); + + it('drops handled ranges instead of accumulating them', async () => { + // A handled range must not be re-scanned by later passes. Pre-fix, the queue variant of this + // hook returned early without clearing when everything was cached, so ranges accumulated for + // the lifetime of the list and a later pass would re-request an item evicted from a range + // handled long ago. + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png']; + mocks.cachedImageNames = ['a.png', 'b.png', 'c.png']; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.imageFetches).toEqual([]); + + mocks.cachedImageNames = ['a.png', 'c.png']; + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.imageFetches).toEqual([['d.png', 'e.png', 'f.png']]); + }); + + it('does not fetch when disabled', async () => { + renderHook(IMAGE_NAMES, false); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + expect(mocks.imageFetches).toEqual([]); + }); +}); describe('video range prefetch', () => { it('does not retain an RTK Query subscription', () => { diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts index eab38776e5b..84fa04501f3 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts @@ -1,4 +1,6 @@ +import { EMPTY_ARRAY } from 'app/store/constants'; import { useAppStore } from 'app/store/storeHooks'; +import { coalesceRanges, useBoundedRangeRetry } from 'common/hooks/useBoundedRangeRetry'; import { isVideoName } from 'features/gallery/store/types'; import { useCallback, useEffect, useState } from 'react'; import type { ListRange } from 'react-virtuoso'; @@ -51,11 +53,22 @@ export const useRangeBasedImageFetching = ({ const store = useAppStore(); const [getImageDTOsByNames] = useGetImageDTOsByNamesMutation(); const [lastRange, setLastRange] = useState(null); - const [pendingRanges, setPendingRanges] = useState([]); + const [pendingRanges, setPendingRanges] = useState(EMPTY_ARRAY); + + const restoreFailedRanges = useCallback((failedRanges: ListRange[]) => { + // Merge with whatever is pending — replacing either side would drop ranges the user reported + // while the failed fetch was in flight, or ranges that failed while the user was scrolling. + setPendingRanges((prev) => (prev.length > 0 ? coalesceRanges([...prev, ...failedRanges]) : failedRanges)); + }, []); + const { onFetchFailure, resetRetryBudget } = useBoundedRangeRetry(restoreFailedRanges); const fetchItems = useCallback( - (ranges: ListRange[], allNames: string[]) => { + (ranges: ListRange[], allNames: string[], handledPendingRanges: ListRange[]) => { if (!enabled) { + // Clear here too, for the same reason as the clear at the end of this callback: returning + // early while disabled let ranges pile up until `enabled` flipped, so the first enabled + // pass scanned every range reported during the disabled window instead of the viewport. + setPendingRanges((prev) => (prev === handledPendingRanges ? EMPTY_ARRAY : prev)); return; } const state = store.getState(); @@ -64,7 +77,17 @@ export const useRangeBasedImageFetching = ({ const cachedImageNames = imagesApi.util.selectCachedArgsForQuery(state, 'getImageDTO'); const uncachedImageNames = getUncachedNames(allNames, cachedImageNames, ranges).filter((n) => !isVideoName(n)); if (uncachedImageNames.length > 0) { - getImageDTOsByNames({ image_names: uncachedImageNames }); + getImageDTOsByNames({ image_names: uncachedImageNames }) + .unwrap() + .then(resetRetryBudget) + .catch(() => { + // This bulk fetch is the ONLY fetcher for these rows: `ImageAtPosition` consumes the + // cache with `skip: isUninitialized`, so a row whose DTO never arrived does not fetch + // for itself, and images (unlike videos) have no retry affordance. Hand the ranges to + // the bounded retry so they are restored after a backoff — otherwise a transient + // failure leaves grey placeholders until the user happens to scroll. + onFetchFailure(ranges); + }); } // Videos — fetch one at a time (no batch endpoint yet). Each `initiate()` is a no-op for @@ -77,21 +100,40 @@ export const useRangeBasedImageFetching = ({ store.dispatch(videosApi.endpoints.getVideoDTO.initiate(videoName, getVideoPrefetchOptions())); } - setPendingRanges([]); + // Clear with a stable reference. `pendingRanges` is a dependency of the effect that + // calls this function, so a fresh `[]` — a new identity every time — re-runs the + // effect, which re-arms the throttle, which calls this again: a self-sustaining + // render loop, running as fast as the throttle allows, for as long as the grid is + // mounted and with no user input. Setting state to the value it already holds makes + // React bail out instead. + // + // Clear only if `pendingRanges` is still the array this pass consumed. An absolute + // `setPendingRanges(EMPTY_ARRAY)` silently discards a restore dispatched in the same React + // batch: a backoff timer and the throttle's trailing edge can expire in the same event-loop + // turn, the absolute update runs last and wins, the final state equals the base, React bails + // out of the re-render, and the restored ranges are gone with nothing left to re-report + // them. The identity check makes the clear a no-op whenever the state has moved on. + setPendingRanges((prev) => (prev === handledPendingRanges ? EMPTY_ARRAY : prev)); }, - [enabled, getImageDTOsByNames, store] + [enabled, getImageDTOsByNames, onFetchFailure, resetRetryBudget, store] ); const throttledFetchItems = useThrottledCallback(fetchItems, 500); - const onRangeChanged = useCallback((range: ListRange) => { - setLastRange(range); - setPendingRanges((prev) => [...prev, range]); - }, []); + const onRangeChanged = useCallback( + (range: ListRange) => { + // A new range report is fresh user input — restart the retry budget so a grid that gave up + // after sustained failure resumes retrying as the user scrolls. + resetRetryBudget(); + setLastRange(range); + setPendingRanges((prev) => [...prev, range]); + }, + [resetRetryBudget] + ); useEffect(() => { const combinedRanges = lastRange ? [...pendingRanges, lastRange] : pendingRanges; - throttledFetchItems(combinedRanges, imageNames); + throttledFetchItems(combinedRanges, imageNames, pendingRanges); }, [imageNames, lastRange, pendingRanges, throttledFetchItems]); return { diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts index a2dba466f76..1b9253ff7f9 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts @@ -1,6 +1,71 @@ -import { describe, expect, it } from 'vitest'; +// @vitest-environment happy-dom +import { act, createElement, type FC } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import type { ListRange } from 'react-virtuoso'; +import { $isConnected } from 'services/events/stores'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { getItemIdBatches, getUncachedItemIds } from './useRangeBasedQueueItemFetching'; +import { getItemIdBatches, getUncachedItemIds, useRangeBasedQueueItemFetching } from './useRangeBasedQueueItemFetching'; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const mocks = vi.hoisted(() => ({ + // Args of every getQueueItemSummariesByItemIds call, in order. + queueFetches: [] as number[][], + // Item ids with a getQueueItemSummary cache entry, as reported by selectCachedArgsForQuery. + cachedItemIds: [] as number[], + // When true, a successful fetch upserts the requested ids into the cache, like the mutation's + // onQueryStarted does. When false, requested ids never land in the cache. + cacheLands: true, + // When true, the mutation rejects, like a backend restart or a 502 from a reverse proxy. + failFetches: false, + // When true, the mutation returns a promise the test rejects by hand, so a rejection can be + // delivered at a chosen moment (e.g. after unmount) rather than on the next microtask. + manualFailure: false, + rejectPending: [] as (() => void)[], +})); + +vi.mock('app/store/storeHooks', () => { + const store = { getState: () => ({}), dispatch: () => undefined }; + return { useAppStore: () => store }; +}); + +vi.mock('services/api/endpoints/queue', () => { + const trigger = (arg: { item_ids: number[] }) => { + mocks.queueFetches.push(arg.item_ids); + if (mocks.manualFailure) { + let reject!: () => void; + const pending = new Promise((_, rej) => { + reject = () => rej(new Error('fetch failed')); + }); + pending.catch(() => undefined); + mocks.rejectPending.push(reject); + return { unwrap: () => pending.then((r) => r) }; + } + // Like the real mutation: onQueryStarted upserts when the request fulfills, whether or not + // the caller unwraps, and only the promise returned by unwrap() surfaces the rejection. + const settled = mocks.failFetches + ? Promise.reject(new Error('fetch failed')) + : Promise.resolve().then(() => { + if (mocks.cacheLands) { + mocks.cachedItemIds.push(...arg.item_ids); + } + return []; + }); + settled.catch(() => undefined); + return { unwrap: () => settled.then((r) => r) }; + }; + // RTK Query's mutation trigger is referentially stable across renders; the hook's + // fetchQueueItems callback (and therefore its throttle and effect) depend on that. + const result = [trigger]; + return { + queueApi: { util: { selectCachedArgsForQuery: () => mocks.cachedItemIds } }, + useGetQueueItemSummariesByItemIdsMutation: () => result, + }; +}); + +const ITEM_IDS = [1, 2, 3]; +const THROTTLE_MS = 500; describe('queue item summary batching', () => { it('sends nothing when there is nothing to fetch', () => { @@ -29,3 +94,441 @@ describe('queue item summary batching', () => { expect(getUncachedItemIds([11, 12, 13], [], ranges, new Set([12]))).toEqual([11, 13]); }); }); + +describe('useRangeBasedQueueItemFetching', () => { + let root: Root | null = null; + let renderCount = 0; + let hookReturn: ReturnType; + + // One stable component type, so re-rendering with new props updates the existing instance + // instead of remounting it — a remount would silently reset the state under test. + const Harness: FC<{ itemIds: number[]; enabled: boolean }> = ({ itemIds, enabled }) => { + renderCount++; + hookReturn = useRangeBasedQueueItemFetching({ itemIds, enabled }); + return null; + }; + + const renderHook = (itemIds: number[], enabled: boolean) => { + root = createRoot(document.createElement('div')); + act(() => { + root!.render(createElement(Harness, { itemIds, enabled })); + }); + }; + + const rerenderHook = (itemIds: number[], enabled: boolean) => { + act(() => { + root!.render(createElement(Harness, { itemIds, enabled })); + }); + }; + + const scrollTo = (range: ListRange) => { + act(() => { + hookReturn.onRangeChanged(range); + }); + }; + + // Advance fake time in small steps, flushing React work (renders + effects) between steps. A + // single long advance would defer all effect re-runs to the end of the act scope, which breaks + // the feedback cycle this suite exists to detect: state update -> effect -> throttle -> fetch -> + // state update. Stepping mimics real event-loop turns, letting a loop sustain itself if the + // code allows one. + const advance = async (ms: number) => { + const step = 250; + for (let elapsed = 0; elapsed < ms; elapsed += step) { + await act(async () => { + await vi.advanceTimersByTimeAsync(step); + }); + } + }; + + beforeEach(() => { + vi.useFakeTimers(); + mocks.queueFetches = []; + mocks.cachedItemIds = []; + mocks.cacheLands = true; + mocks.failFetches = false; + mocks.manualFailure = false; + mocks.rejectPending = []; + renderCount = 0; + $isConnected.set(false); + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + root = null; + } + $isConnected.set(false); + vi.useRealTimers(); + }); + + it('does not loop when mounted with nothing to fetch', async () => { + // The clear at the end of the fetch callback is unconditional, so this hook now relies on the + // EMPTY_ARRAY identity for the nothing-to-do path that the old early return used to cover. + renderHook(ITEM_IDS, true); + await advance(THROTTLE_MS * 2); + const settledRenders = renderCount; + + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.queueFetches).toEqual([]); + }); + + it('fetches uncached items for a reported range, then goes quiet', async () => { + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + + expect(mocks.queueFetches).toEqual([ITEM_IDS]); + + // Regression: clearing pendingRanges with a fresh `[]` (a new identity every time) re-ran the + // effect, re-armed the throttle, and cleared again — a self-sustaining render loop. Once the + // range has been handled and the throttle has drained, both renders and fetches must stop. + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(renderCount).toBe(settledRenders); + expect(mocks.queueFetches).toEqual([ITEM_IDS]); + }); + + it('stops re-requesting items that never land in the cache', async () => { + // A requested id the server does not return never gets a getQueueItemSummary cache entry, so + // it is uncached on every pass. Pre-fix, that sustained the loop: the list re-requested such + // ids every ~500ms for as long as it was mounted. + mocks.cacheLands = false; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + + // The range-change pass fetches once, and clearing pendingRanges ([range] -> EMPTY_ARRAY) is a + // real state change, so one follow-up pass may re-check the cache and re-request the + // still-missing ids. After that the state is stable and the stream must stop — pre-fix it + // continued at one request per throttle window, forever. + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(1); + expect(mocks.queueFetches.length).toBeLessThanOrEqual(2); + const settledFetches = mocks.queueFetches.length; + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches.length).toBe(settledFetches); + }); + + it('retries a failed fetch until it succeeds, then goes quiet', async () => { + // This bulk fetch is the only fetcher for these rows (QueueItemAtPosition subscribes with + // `skip: isUninitialized`), so a transient failure must be retried or the placeholders stay + // empty until the user happens to scroll. + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + + // Without the retry, clearing pendingRanges after the failed fetch still re-runs the effect + // once, so the count caps at two — three or more requires the retry restoring the ranges. + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(3); + expect(mocks.cachedItemIds).toEqual([]); + + mocks.failFetches = false; + await advance(THROTTLE_MS * 8); + expect(mocks.cachedItemIds).toEqual(ITEM_IDS); + + const fetchesAfterRecovery = mocks.queueFetches.length; + const settledRenders = renderCount; + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches.length).toBe(fetchesAfterRecovery); + expect(renderCount).toBe(settledRenders); + }); + + it('stops retrying when failure is sustained, instead of storming', async () => { + // Review finding on the original retry: restoring the ranges immediately meant a sustained + // backend outage produced a request every throttle window, forever — a fixed-rate storm from + // every open tab against a backend trying to come back up. The bounded retry backs off + // (1s, 2s, 4s, 8s, 16s) and gives up after five consecutive scheduled retries, so the request + // stream must terminate. Each retry pass produces at most a leading and a trailing fetch, + // bounding the total at 12; six requires every backoff retry to have actually fired. + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(35_000); + + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(6); + expect(mocks.queueFetches.length).toBeLessThanOrEqual(12); + + const settledFetches = mocks.queueFetches.length; + const settledRenders = renderCount; + await advance(30_000); + expect(mocks.queueFetches.length).toBe(settledFetches); + expect(renderCount).toBe(settledRenders); + }); + + it('resumes retrying after giving up when the user scrolls', async () => { + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(35_000); + const fetchesAfterGiveUp = mocks.queueFetches.length; + + // A new range report is fresh user input: it restarts the retry budget, so the list does not + // stay dead until reload. With the budget still exhausted, only the scroll-triggered fetch and + // its trailing companion would fire — three or more new fetches requires the backoff schedule + // to have restarted. + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(2_000); + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(fetchesAfterGiveUp + 3); + }); + + it('restores parked ranges on the next scroll when the socket never dropped', async () => { + // Review finding (coverage): parked ranges are restored by three signals — a socket reconnect, + // a later success, and a fresh range report — but only the reconnect was pinned. `resumes + // retrying after giving up when the user scrolls` re-reports the *same* range, which + // `lastRange` re-fetches whether or not the parked set was restored, so deleting the restore + // from `resetRetryBudget` left the suite green. The distinction only shows for a range that is + // parked but no longer on screen, after a recovery the socket never observed — a transient 502 + // from a reverse proxy, say, where the websocket stays up throughout and the reconnect path + // never fires. + const itemIds = [1, 2, 3, 4, 5, 6, 7, 8, 9]; + $isConnected.set(true); + mocks.failFetches = true; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(35_000); + expect(mocks.cachedItemIds).toEqual([]); + + // REST answers again with no socket transition, so the scroll is the only signal that can heal + // the parked range. + mocks.failFetches = false; + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(THROTTLE_MS * 4); + + // The rows the user scrolled past during the outage land along with the new viewport. Without + // the restore only 7-9 would be fetched and 1-3 would stay placeholders permanently. + expect([...mocks.cachedItemIds].sort((a, b) => a - b)).toEqual([1, 2, 3, 7, 8, 9]); + }); + + it('heals a list that gave up when the socket reconnects, with no user input', async () => { + // Review finding: the retry budget ends ~31s after the first failure, but an InvokeAI restart + // (config load, DB migrations, model scan) routinely takes longer. For an idle user nothing + // else re-arms it — `itemIds` keeps its identity through the reconnect refetch and `enabled` + // does not toggle — so without the reconnect signal the rows stayed placeholders until the + // user scrolled. Ranges abandoned by the exhausted budget are parked, not dropped, and the + // socket reconnect restores them. + $isConnected.set(true); + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + + // Backend goes down: the socket drops and the retry budget runs out while it is down. + $isConnected.set(false); + await advance(35_000); + const fetchesAfterGiveUp = mocks.queueFetches.length; + await advance(30_000); + expect(mocks.queueFetches.length).toBe(fetchesAfterGiveUp); + expect(mocks.cachedItemIds).toEqual([]); + + // Backend comes back, well past the retry budget. No scroll, no change to itemIds. + mocks.failFetches = false; + act(() => { + $isConnected.set(true); + }); + await advance(THROTTLE_MS * 4); + + expect(mocks.cachedItemIds).toEqual(ITEM_IDS); + + // And the heal must settle. Restoring the parked ranges without emptying the parked set would + // make every later success restore them again — success -> restore -> fetch -> success — a + // loop that the cache assertion alone cannot see. + const fetchesAfterHeal = mocks.queueFetches.length; + await advance(30_000); + expect(mocks.queueFetches.length).toBe(fetchesAfterHeal); + }); + + it('empties the parked set when it heals, even if the rows never reach the cache', async () => { + // The parked set is handed to the restore and cleared in one step. Restoring without clearing + // it looks harmless while the rows do land in the cache — the follow-up pass finds nothing to + // request — but a name the server never returns is uncached on every pass, so every success + // would restore the same parked ranges again: success -> restore -> request -> success. + $isConnected.set(true); + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + $isConnected.set(false); + await advance(35_000); + + // The backend answers again, but these rows never land in the cache (deleted, or filtered out + // for this user). + mocks.failFetches = false; + mocks.cacheLands = false; + act(() => { + $isConnected.set(true); + }); + await advance(THROTTLE_MS * 4); + + const fetchesAfterHeal = mocks.queueFetches.length; + await advance(60_000); + expect(mocks.queueFetches.length).toBe(fetchesAfterHeal); + }); + + it('does not turn a flapping socket into a request stream', async () => { + // Review finding: re-arming on every reconnect made the budget per-reconnect rather than + // per-outage. A socket that keeps completing a handshake while REST stays broken — a + // crash-looping container, uvicorn accepting connections before startup finishes, a proxy + // routing the websocket to a healthy replica and REST to a sick one — would then pin the + // backoff at its shortest delay for as long as the flapping lasted. The re-arm is now floored + // at one per RETRY_REARM_COOLDOWN_MS (60s) and only fires when there is something parked. + $isConnected.set(true); + mocks.failFetches = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + + // Five minutes of flapping every 5s, REST failing throughout, no user input. + for (let i = 0; i < 60; i++) { + act(() => { + $isConnected.set(false); + }); + await advance(2_500); + act(() => { + $isConnected.set(true); + }); + await advance(2_500); + } + + // Design intent with no flapping at all is 12 requests (one bounded streak). Five minutes of + // flapping buys at most five re-arms, each worth another bounded streak. Pre-fix this ran at + // the flap rate and measured 240. + expect(mocks.queueFetches.length).toBeLessThanOrEqual(80); + }); + + it('does not schedule a retry for a fetch that rejects after unmount', async () => { + // Review finding: the unmount cleanup clears the pending timer, but a mutation still in flight + // rejects afterwards, reaching onFetchFailure on a dead instance and arming a fresh timer of + // up to 16s that no cleanup will ever reach. Triggered by closing the queue tab while the + // backend is down. + mocks.manualFailure = true; + renderHook(ITEM_IDS, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + expect(mocks.rejectPending.length).toBeGreaterThan(0); + + // Unmount with the request still in flight, then let it reject. + act(() => { + root!.unmount(); + }); + root = null; + const timersAfterUnmount = vi.getTimerCount(); + + for (const reject of mocks.rejectPending) { + reject(); + } + // Deliver the rejection without advancing the clock, so a backoff timer armed by it (>=1s) + // is still pending and countable rather than already fired and cleared. + await act(async () => { + await vi.advanceTimersByTimeAsync(0); + }); + expect(vi.getTimerCount()).toBe(timersAfterUnmount); + }); + + it('does not accumulate ranges reported while disabled', async () => { + // Review finding: the `!enabled` guard returned before the clear, so every range reported + // while disabled stayed in pendingRanges and the first enabled pass scanned all of them. + const itemIds = [1, 2, 3, 4, 5, 6, 7, 8, 9]; + renderHook(itemIds, false); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 2); + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([]); + + // Enable. In production `enabled` is `!isLoading`, so it flips as the item ids arrive — a new + // array identity, which is what re-runs the fetch effect (`throttledFetchQueueItems` is + // referentially stable across callback changes, so `enabled` alone does not re-run it). + // The pass that follows must cover the last reported viewport (4-6) and nothing else: the + // earlier range (1-3), long scrolled past, must not still be sitting in pendingRanges. + rerenderHook([...itemIds], true); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches.flat().sort((a, b) => a - b)).toEqual([4, 5, 6]); + + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches.flat().sort((a, b) => a - b)).toEqual([4, 5, 6, 7, 8, 9]); + }); + + // Review finding: pinned to a single delay, this passed only on a lucky phase of the + // throttle/backoff alignment. Sweeping it covers the batch in which the backoff retry and the + // throttle's trailing edge land together — the interleaving in which an absolute clear discards + // the restore. + it.each([500, 600, 750, 1_000, 1_250])( + 'recovers a range that failed while the user was scrolling elsewhere (scroll at t=%dms)', + async (delayBeforeScroll) => { + // Review finding on the original retry: the catch (`prev.length > 0 ? prev : ranges`) + // dropped the failed range whenever another range had been reported in the meantime — rows + // the user had scrolled past stayed blank placeholders. The retry now merges the failed + // ranges with whatever is pending instead of choosing one side, and the clear only fires + // when the pending state is still the array the pass consumed, so both ranges end up + // fetched with no further user input. + const itemIds = [1, 2, 3, 4, 5, 6, 7, 8, 9]; + mocks.failFetches = true; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(delayBeforeScroll); + + // The backend recovers and the user scrolls to a disjoint range while the backoff retry for + // the failed range is still pending. + mocks.failFetches = false; + scrollTo({ startIndex: 6, endIndex: 8 }); + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(10_000); + + // Both the failed range and the new one land, with no user input beyond the one scroll — + // and nothing outside the reported ranges is fetched. + expect([...mocks.cachedItemIds].sort((a, b) => a - b)).toEqual([1, 2, 3, 7, 8, 9]); + } + ); + + it('still fetches for new ranges after settling', async () => { + const itemIds = [1, 2, 3, 4, 5, 6]; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches).toEqual([[1, 2, 3]]); + + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([ + [1, 2, 3], + [4, 5, 6], + ]); + }); + + it('fetches every range reported within a throttle window, not just the last', async () => { + // onRangeChanged accumulates ranges into pendingRanges precisely so that ranges reported + // mid-window are not dropped when the trailing invocation only sees the latest call's args. + const itemIds = [1, 2, 3, 4, 5, 6]; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([[1, 2, 3, 4, 5, 6]]); + }); + + it('drops handled ranges instead of accumulating them', async () => { + // A handled range must not be re-scanned by later passes. Pre-fix, this hook returned early + // without clearing when everything was cached, so ranges accumulated for the lifetime of the + // list and a later pass would re-request an item evicted from a range handled long ago. + const itemIds = [1, 2, 3, 4, 5, 6]; + mocks.cachedItemIds = [1, 2, 3]; + renderHook(itemIds, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 10); + expect(mocks.queueFetches).toEqual([]); + + mocks.cachedItemIds = [1, 3]; + scrollTo({ startIndex: 3, endIndex: 5 }); + await advance(THROTTLE_MS * 2); + expect(mocks.queueFetches).toEqual([[4, 5, 6]]); + }); + + it('does not fetch when disabled', async () => { + renderHook(ITEM_IDS, false); + scrollTo({ startIndex: 0, endIndex: 2 }); + await advance(THROTTLE_MS * 4); + expect(mocks.queueFetches).toEqual([]); + }); +}); diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts index 645dcc5b8e5..6465438314c 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts @@ -1,4 +1,6 @@ +import { EMPTY_ARRAY } from 'app/store/constants'; import { useAppStore } from 'app/store/storeHooks'; +import { coalesceRanges, useBoundedRangeRetry } from 'common/hooks/useBoundedRangeRetry'; import { useCallback, useEffect, useRef, useState } from 'react'; import type { ListRange } from 'react-virtuoso'; import { queueApi, useGetQueueItemSummariesByItemIdsMutation } from 'services/api/endpoints/queue'; @@ -62,43 +64,84 @@ export const useRangeBasedQueueItemFetching = ({ const store = useAppStore(); const [getQueueItemSummariesByItemIds] = useGetQueueItemSummariesByItemIdsMutation(); const [lastRange, setLastRange] = useState(null); - const [pendingRanges, setPendingRanges] = useState([]); + const [pendingRanges, setPendingRanges] = useState(EMPTY_ARRAY); const pendingItemIdsRef = useRef>(new Set()); + const restoreFailedRanges = useCallback((failedRanges: ListRange[]) => { + // Merge with whatever is pending — replacing either side would drop ranges the user reported + // while the failed fetch was in flight, or ranges that failed while the user was scrolling. + setPendingRanges((prev) => (prev.length > 0 ? coalesceRanges([...prev, ...failedRanges]) : failedRanges)); + }, []); + const { onFetchFailure, resetRetryBudget } = useBoundedRangeRetry(restoreFailedRanges); + const fetchQueueItems = useCallback( - (ranges: ListRange[], itemIds: number[]) => { + (ranges: ListRange[], itemIds: number[], handledPendingRanges: ListRange[]) => { if (!enabled) { + // Clear here too, for the same reason as the unconditional clear below: returning early + // while disabled let ranges pile up until `enabled` flipped, so the first enabled pass + // scanned every range reported during the disabled window instead of the viewport. + setPendingRanges((prev) => (prev === handledPendingRanges ? EMPTY_ARRAY : prev)); return; } const cachedItemIds = queueApi.util.selectCachedArgsForQuery(store.getState(), 'getQueueItemSummary'); const uncachedItemIds = getUncachedItemIds(itemIds, cachedItemIds, ranges, pendingItemIdsRef.current); - if (uncachedItemIds.length === 0) { - return; - } for (const item_ids of getItemIdBatches(uncachedItemIds)) { item_ids.forEach((item_id) => pendingItemIdsRef.current.add(item_id)); void getQueueItemSummariesByItemIds({ item_ids }) .unwrap() .then( - () => item_ids.forEach((item_id) => pendingItemIdsRef.current.delete(item_id)), - () => item_ids.forEach((item_id) => pendingItemIdsRef.current.delete(item_id)) + () => { + item_ids.forEach((item_id) => pendingItemIdsRef.current.delete(item_id)); + resetRetryBudget(); + }, + () => { + item_ids.forEach((item_id) => pendingItemIdsRef.current.delete(item_id)); + // This bulk fetch is the ONLY fetcher for these rows: `QueueItemAtPosition` consumes + // the cache with `skip: isUninitialized`, so a row whose summary never arrived does + // not fetch for itself. Hand the ranges to the bounded retry so they are restored + // after a backoff — otherwise a transient failure leaves placeholders until the user + // happens to scroll. + onFetchFailure(ranges); + } ); } - setPendingRanges([]); + // Clear unconditionally. Returning early without clearing (the previous behaviour when + // everything was already cached) let ranges accumulate for the lifetime of the list, + // growing the scan on every subsequent pass. + // + // Clear with a stable reference. `pendingRanges` is a dependency of the effect that calls + // this function, so a fresh `[]` — a new identity every time — re-runs the effect, which + // re-arms the throttle, which calls this again. The old early return happened to prevent + // that while everything was cached, so the loop only ran while items were genuinely + // uncached; clearing on both paths means the stable reference is now what stops it. + // + // Clear only if `pendingRanges` is still the array this pass consumed. An absolute + // `setPendingRanges(EMPTY_ARRAY)` silently discards a restore dispatched in the same React + // batch: a backoff timer and the throttle's trailing edge can expire in the same event-loop + // turn, the absolute update runs last and wins, the final state equals the base, React bails + // out of the re-render, and the restored ranges are gone with nothing left to re-report + // them. The identity check makes the clear a no-op whenever the state has moved on. + setPendingRanges((prev) => (prev === handledPendingRanges ? EMPTY_ARRAY : prev)); }, - [enabled, getQueueItemSummariesByItemIds, store] + [enabled, getQueueItemSummariesByItemIds, onFetchFailure, resetRetryBudget, store] ); const throttledFetchQueueItems = useThrottledCallback(fetchQueueItems, 500); - const onRangeChanged = useCallback((range: ListRange) => { - setLastRange(range); - setPendingRanges((prev) => [...prev, range]); - }, []); + const onRangeChanged = useCallback( + (range: ListRange) => { + // A new range report is fresh user input — restart the retry budget so a list that gave up + // after sustained failure resumes retrying as the user scrolls. + resetRetryBudget(); + setLastRange(range); + setPendingRanges((prev) => [...prev, range]); + }, + [resetRetryBudget] + ); useEffect(() => { const combinedRanges = lastRange ? [...pendingRanges, lastRange] : pendingRanges; - throttledFetchQueueItems(combinedRanges, itemIds); + throttledFetchQueueItems(combinedRanges, itemIds, pendingRanges); }, [itemIds, lastRange, pendingRanges, throttledFetchQueueItems]); return {