From bc6a1a1d3b73582b651190011f82e8afd5d57e02 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 1 Aug 2026 21:49:01 -0400 Subject: [PATCH 1/3] fix(ui): stop range-based fetching hooks from spinning in a render loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fetchItems` cleared the accumulated ranges with `setPendingRanges([])`, and `pendingRanges` is a dependency of the effect that calls `fetchItems`. A fresh `[]` is a new identity every time, so the effect re-ran, re-armed the 500ms throttle, and cleared again — a self-sustaining render loop that ran as fast as the throttle allowed, with no user input, for as long as the gallery grid was mounted. Clear with the shared stable `EMPTY_ARRAY` reference instead, so React bails out rather than re-running the effect. The queue variant returned early — before clearing — when nothing was uncached, which happened to prevent the loop while everything was cached, at the cost of letting ranges accumulate for the lifetime of the list and growing the scan on every pass. It now clears on both paths, with the stable reference doing the work of stopping the loop. Retry on failure explicitly, because the loop was doing it accidentally. These bulk fetches are the only fetcher for their rows: `ImageAtPosition` and `QueueItemAtPosition` both consume the cache with `skip: isUninitialized`, so a row whose DTO never arrived does not fetch for itself, and images have no retry affordance. Without this, a transient failure would leave placeholders until the user happened to scroll, where before the loop re-tried until it succeeded. Co-Authored-By: Claude Opus 5 (1M context) --- .../hooks/useRangeBasedImageFetching.ts | 22 ++++++++++++--- .../hooks/useRangeBasedQueueItemFetching.ts | 27 +++++++++++++++---- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts index eab38776e5b..6264189747f 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts @@ -1,3 +1,4 @@ +import { EMPTY_ARRAY } from 'app/store/constants'; import { useAppStore } from 'app/store/storeHooks'; import { isVideoName } from 'features/gallery/store/types'; import { useCallback, useEffect, useState } from 'react'; @@ -51,7 +52,7 @@ 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 fetchItems = useCallback( (ranges: ListRange[], allNames: string[]) => { @@ -64,7 +65,16 @@ 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() + .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. Put the ranges back + // so the effect re-runs and tries again — otherwise a transient failure leaves grey + // placeholders until the user happens to scroll. The throttle bounds the retry rate. + setPendingRanges((prev) => (prev.length > 0 ? prev : ranges)); + }); } // Videos — fetch one at a time (no batch endpoint yet). Each `initiate()` is a no-op for @@ -77,7 +87,13 @@ 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. + setPendingRanges(EMPTY_ARRAY); }, [enabled, getImageDTOsByNames, store] ); diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts index b2d4c4ac813..33697875542 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts @@ -1,3 +1,4 @@ +import { EMPTY_ARRAY } from 'app/store/constants'; import { useAppStore } from 'app/store/storeHooks'; import { useCallback, useEffect, useState } from 'react'; import type { ListRange } from 'react-virtuoso'; @@ -41,7 +42,7 @@ export const useRangeBasedQueueItemFetching = ({ const store = useAppStore(); const [getQueueItemDTOsByItemIds] = useGetQueueItemDTOsByItemIdsMutation(); const [lastRange, setLastRange] = useState(null); - const [pendingRanges, setPendingRanges] = useState([]); + const [pendingRanges, setPendingRanges] = useState(EMPTY_ARRAY); const fetchQueueItems = useCallback( (ranges: ListRange[], itemIds: number[]) => { @@ -50,11 +51,27 @@ export const useRangeBasedQueueItemFetching = ({ } const cachedItemIds = queueApi.util.selectCachedArgsForQuery(store.getState(), 'getQueueItem'); const uncachedItemIds = getUncachedItemIds(itemIds, cachedItemIds, ranges); - if (uncachedItemIds.length === 0) { - return; + if (uncachedItemIds.length > 0) { + getQueueItemDTOsByItemIds({ item_ids: uncachedItemIds }) + .unwrap() + .catch(() => { + // This bulk fetch is the ONLY fetcher for these rows: `QueueItemAtPosition` consumes + // the cache with `skip: isUninitialized`, so a row whose DTO never arrived does not + // fetch for itself. Put the ranges back so the effect re-runs and tries again — + // otherwise a transient failure leaves placeholders until the user happens to scroll. + setPendingRanges((prev) => (prev.length > 0 ? prev : ranges)); + }); } - getQueueItemDTOsByItemIds({ item_ids: uncachedItemIds }); - 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. + setPendingRanges(EMPTY_ARRAY); }, [enabled, getQueueItemDTOsByItemIds, store] ); From 392e5ae5548af74beeb91ffbfffd81a04608ffc7 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 2 Aug 2026 15:53:30 -0400 Subject: [PATCH 2/3] test(ui): regression tests for the range-based fetching render loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Render both hooks with React act + fake timers in a happy-dom environment (scoped per-file via a @vitest-environment docblock; happy-dom is the only new dev dependency) and mock only the thin API-endpoint modules, so the tests exercise the real state/effect/throttle cycle the fix changed. Covered per hook: - a reported range fetches its uncached items once, then renders and fetches both go quiet (the pre-fix loop re-rendered every throttle window forever, and in the gallery hook ran from mount even with nothing to fetch) - items that never land in the cache (deleted image, multiuser ownership filter) are not re-requested indefinitely — bounded, then quiet, where the pre-fix loop was a permanent one-request-per-window stream - a failed bulk fetch is retried until it succeeds, then goes quiet — the explicit replacement for the retry the loop provided accidentally - every range reported within a throttle window is fetched, not just the last (the pendingRanges accumulation onRangeChanged exists for) - handled ranges are dropped, not accumulated: an item evicted from a long-handled range is not re-requested by later passes (the queue hook's pre-fix early return without clearing regressed exactly this) - new ranges after settling still fetch, and enabled=false fetches nothing The time-advance helper steps in small increments with an act flush per step; a single long advance would defer effect re-runs to the end of the act scope and break the very feedback cycle (state update -> effect -> throttle -> fetch) the suite exists to detect. Mutation-verified: reverting the EMPTY_ARRAY clears, restoring the queue hook's early return, dropping onRangeChanged's accumulation, or neutering the retry catch each makes at least one test fail; all pass with the fix in place. Co-Authored-By: Claude Fable 5 --- invokeai/frontend/web/package.json | 1 + invokeai/frontend/web/pnpm-lock.yaml | 77 +++++- .../hooks/useRangeBasedImageFetching.test.ts | 248 +++++++++++++++++- .../useRangeBasedQueueItemFetching.test.ts | 223 ++++++++++++++++ 4 files changed, 542 insertions(+), 7 deletions(-) create mode 100644 invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts diff --git a/invokeai/frontend/web/package.json b/invokeai/frontend/web/package.json index 6c7ea3f65ca..2ba037653e5 100644 --- a/invokeai/frontend/web/package.json +++ b/invokeai/frontend/web/package.json @@ -142,6 +142,7 @@ "eslint-plugin-storybook": "^10.3.6", "eslint-plugin-unused-imports": "^4.4.1", "globals": "^16.5.0", + "happy-dom": "^20.11.1", "knip": "^5.77.4", "magic-string": "^0.30.21", "openapi-types": "^12.1.3", diff --git a/invokeai/frontend/web/pnpm-lock.yaml b/invokeai/frontend/web/pnpm-lock.yaml index 4901ad00405..b45e368e497 100644 --- a/invokeai/frontend/web/pnpm-lock.yaml +++ b/invokeai/frontend/web/pnpm-lock.yaml @@ -297,6 +297,9 @@ importers: globals: specifier: ^16.5.0 version: 16.5.0 + happy-dom: + specifier: ^20.11.1 + version: 20.11.1 knip: specifier: ^5.77.4 version: 5.77.4(@types/node@22.19.3)(typescript@5.9.3) @@ -338,7 +341,7 @@ importers: version: 1.8.1(eslint@9.39.2(jiti@2.6.1))(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) vitest: specifier: ^4.1.5 - version: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) + version: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) packages: @@ -1969,6 +1972,12 @@ packages: '@types/uuid@10.0.0': resolution: {integrity: sha512-7gqG38EyHgyP1S+7+xomFtL+ZNHcKv6DwNaCZmJmo1vgMugyF3TCnXVg4t1uk89mLNwnLtnY3TpOpCOyp1/xHQ==} + '@types/whatwg-mimetype@3.0.2': + resolution: {integrity: sha512-c2AKvDT8ToxLIOUlN51gTiHXflsfIFisS4pO7pDPoKouJCESkhZnEy623gwP9laCy5lnLDAw1vAzu2vM2YLOrA==} + + '@types/ws@8.18.1': + resolution: {integrity: sha512-ThVF6DCVhA8kUGy+aazFQ4kXQ7E1Ty7A3ypFOe0IcJV8O/M511G99AW24irKrW56Wt44yG9+ij8FaqoBGkuBXg==} + '@typescript-eslint/eslint-plugin@8.59.2': resolution: {integrity: sha512-j/bwmkBvHUtPNxzuWe5z6BEk3q54YRyGlBXkSsmfoih7zNrBvl5A9A98anlp/7JbyZcWIJ8KXo/3Tq/DjFLtuQ==} engines: {node: ^18.18.0 || ^20.9.0 || >=21.1.0} @@ -2331,6 +2340,10 @@ packages: engines: {node: ^6 || ^7 || ^8 || ^9 || ^10 || ^11 || ^12 || >=13.7} hasBin: true + buffer-image-size@0.6.4: + resolution: {integrity: sha512-nEh+kZOPY1w+gcCMobZ6ETUp9WfibndnosbpwB1iJk/8Gt5ZF2bhS6+B6bPYz424KtwsR6Rflc3tCz1/ghX2dQ==} + engines: {node: '>=4.0'} + buffer@5.7.1: resolution: {integrity: sha512-EHcyIPBQ4BSGlvjB16k5KgAJ27CIsHY/2JBmCRReo48y9rQ3MaUzWX3KVlBa4U7MyX02HdVj0K7C3WaB3ju7FQ==} @@ -2660,6 +2673,10 @@ packages: resolution: {integrity: sha512-HqD3yTBfnBxIrbnM1DoD6Pcq8NECnh8d4As1Qgh0z5Gg3jRRIqijury0CL3ghu/edArpUYiYqQiDUQBIs4np3Q==} engines: {node: '>=10.0.0'} + entities@7.0.1: + resolution: {integrity: sha512-TWrgLOFUQTH994YUyl1yT4uyavY5nNB5muff+RtWaqNVCAK408b5ZnnbNAUEWLTCpum9w6arT70i1XdQ4UeOPA==} + engines: {node: '>=0.12'} + error-ex@1.3.4: resolution: {integrity: sha512-sqQamAnR14VgCr1A618A3sGrygcpK+HEbenA/HiEAkkUwcZIIB/tgWqHFxWgOyDh4nB4JCRimh79dR5Ywc9MDQ==} @@ -3070,6 +3087,10 @@ packages: graceful-fs@4.2.11: resolution: {integrity: sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==} + happy-dom@20.11.1: + resolution: {integrity: sha512-XSt8tMzbW9ymE7687xztkO1ckR7qJNQ3LywY9vlYGhGi3zXrGBHuUo2Cl1ztZaICW+1eAGdkLbj6iwVqDT33kg==} + engines: {node: '>=20.0.0'} + has-bigints@1.1.0: resolution: {integrity: sha512-R3pbpkcIqv2Pm3dUwgjclDRVmWpTJW2DcMzcIhEXEx1oh/CEMObMm3KLmRJOdvhM7o4uQBnwr8pzRK2sJWIqfg==} engines: {node: '>= 0.4'} @@ -4854,6 +4875,10 @@ packages: webpack-virtual-modules@0.6.2: resolution: {integrity: sha512-66/V2i5hQanC51vBQKPH4aI8NMAcBW59FVBs+rC7eGHupMyfn34q7rZIE+ETlJ+XTevqfUhVVBgSUNSW2flEUQ==} + whatwg-mimetype@3.0.0: + resolution: {integrity: sha512-nt+N2dzIutVRxARx1nghPKGv1xHikU7HKdfafKkLNLindmPU/ch3U31NOCGGA/dmPcmb1VlofO0vnKAcsm0o/Q==} + engines: {node: '>=12'} + whatwg-url@5.0.0: resolution: {integrity: sha512-saE57nupxk6v3HY35+jzBwYa0rKSy0XR8JSxZPwgLr7ys0IBzhGviA1/TUGJLmSVqs8pb9AnvICXEuOHLprYTw==} @@ -4919,6 +4944,18 @@ packages: utf-8-validate: optional: true + ws@8.21.1: + resolution: {integrity: sha512-+0NTnW77fFN/DjQi6k/Sq/Yvk4Sgajw7urW8V+asjXnRgDs9gyGkdb7EzgfhA4goXsRIZKE28fzIXBHEzhuiWw==} + engines: {node: '>=10.0.0'} + peerDependencies: + bufferutil: ^4.0.1 + utf-8-validate: '>=5.0.2' + peerDependenciesMeta: + bufferutil: + optional: true + utf-8-validate: + optional: true + wsl-utils@0.1.0: resolution: {integrity: sha512-h3Fbisa2nKGPxCpm89Hk33lBLsnaGBvctQopaBSOW/uIs6FTe1ATyAnKFJrzVs9vpGdsTe73WF3V4lIsk4Gacw==} engines: {node: '>=18'} @@ -6497,7 +6534,7 @@ snapshots: '@testing-library/dom@10.4.0': dependencies: - '@babel/code-frame': 7.29.0 + '@babel/code-frame': 7.29.7 '@babel/runtime': 7.29.2 '@types/aria-query': 5.0.4 aria-query: 5.3.0 @@ -6635,6 +6672,12 @@ snapshots: '@types/uuid@10.0.0': {} + '@types/whatwg-mimetype@3.0.2': {} + + '@types/ws@8.18.1': + dependencies: + '@types/node': 22.19.3 + '@typescript-eslint/eslint-plugin@8.59.2(@typescript-eslint/parser@8.59.2(eslint@9.39.2(jiti@2.6.1))(typescript@5.9.3))(eslint@9.39.2(jiti@2.6.1))(typescript@5.9.3)': dependencies: '@eslint-community/regexpp': 4.12.2 @@ -6797,7 +6840,7 @@ snapshots: obug: 2.1.1 std-env: 4.1.0 tinyrainbow: 3.1.0 - vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) + vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) '@vitest/expect@3.2.4': dependencies: @@ -6859,7 +6902,7 @@ snapshots: sirv: 3.0.2 tinyglobby: 0.2.16 tinyrainbow: 3.1.0 - vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) + vitest: 4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) '@vitest/utils@3.2.4': dependencies: @@ -7110,6 +7153,10 @@ snapshots: node-releases: 2.0.38 update-browserslist-db: 1.2.3(browserslist@4.28.2) + buffer-image-size@0.6.4: + dependencies: + '@types/node': 22.19.3 + buffer@5.7.1: dependencies: base64-js: 1.5.1 @@ -7445,6 +7492,8 @@ snapshots: engine.io-parser@5.2.3: {} + entities@7.0.1: {} + error-ex@1.3.4: dependencies: is-arrayish: 0.2.1 @@ -7992,6 +8041,19 @@ snapshots: graceful-fs@4.2.11: {} + happy-dom@20.11.1: + dependencies: + '@types/node': 22.19.3 + '@types/whatwg-mimetype': 3.0.2 + '@types/ws': 8.18.1 + buffer-image-size: 0.6.4 + entities: 7.0.1 + whatwg-mimetype: 3.0.0 + ws: 8.21.1 + transitivePeerDependencies: + - bufferutil + - utf-8-validate + has-bigints@1.1.0: {} has-flag@4.0.0: {} @@ -9744,7 +9806,7 @@ snapshots: fsevents: 2.3.3 jiti: 2.6.1 - vitest@4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)): + vitest@4.1.5(@types/node@22.19.3)(@vitest/coverage-v8@4.1.5)(@vitest/ui@4.1.5)(happy-dom@20.11.1)(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)): dependencies: '@vitest/expect': 4.1.5 '@vitest/mocker': 4.1.5(vite@8.0.11(@types/node@22.19.3)(esbuild@0.27.7)(jiti@2.6.1)) @@ -9770,6 +9832,7 @@ snapshots: '@types/node': 22.19.3 '@vitest/coverage-v8': 4.1.5(vitest@4.1.5) '@vitest/ui': 4.1.5(vitest@4.1.5) + happy-dom: 20.11.1 transitivePeerDependencies: - msw @@ -9785,6 +9848,8 @@ snapshots: webpack-virtual-modules@0.6.2: {} + whatwg-mimetype@3.0.0: {} + whatwg-url@5.0.0: dependencies: tr46: 0.0.3 @@ -9858,6 +9923,8 @@ snapshots: ws@8.20.0: {} + ws@8.21.1: {} + wsl-utils@0.1.0: dependencies: is-wsl: 3.1.1 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..8194c5c965c 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,250 @@ -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 { 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, +})); + +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); + // 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; + + const renderHook = (imageNames: string[], enabled: boolean) => { + const Harness: FC = () => { + renderCount++; + hookReturn = useRangeBasedImageFetching({ imageNames, enabled }); + return null; + }; + root = createRoot(document.createElement('div')); + act(() => { + root!.render(createElement(Harness)); + }); + }; + + 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; + renderCount = 0; + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + root = null; + } + 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 catch-driven retry produces a fetch per throttle window. Without it, clearing + // pendingRanges after the failed fetch still re-runs the effect once, so the count caps at + // two — three or more requires the catch handler 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('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/queue/hooks/useRangeBasedQueueItemFetching.test.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts new file mode 100644 index 00000000000..fb40bd55d35 --- /dev/null +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts @@ -0,0 +1,223 @@ +// @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 { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useRangeBasedQueueItemFetching } from './useRangeBasedQueueItemFetching'; + +(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const mocks = vi.hoisted(() => ({ + // Args of every getQueueItemDTOsByItemIds call, in order. + queueFetches: [] as number[][], + // Item ids with a getQueueItem 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, +})); + +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); + // 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 } }, + useGetQueueItemDTOsByItemIdsMutation: () => result, + }; +}); + +const ITEM_IDS = [1, 2, 3]; +const THROTTLE_MS = 500; + +describe('useRangeBasedQueueItemFetching', () => { + let root: Root | null = null; + let renderCount = 0; + let hookReturn: ReturnType; + + const renderHook = (itemIds: number[], enabled: boolean) => { + const Harness: FC = () => { + renderCount++; + hookReturn = useRangeBasedQueueItemFetching({ itemIds, enabled }); + return null; + }; + root = createRoot(document.createElement('div')); + act(() => { + root!.render(createElement(Harness)); + }); + }; + + 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; + renderCount = 0; + }); + + afterEach(() => { + if (root) { + act(() => { + root!.unmount(); + }); + root = null; + } + vi.useRealTimers(); + }); + + 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 getQueueItem 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); + + // The catch-driven retry produces a fetch per throttle window. Without it, clearing + // pendingRanges after the failed fetch still re-runs the effect once, so the count caps at + // two — three or more requires the catch handler restoring the ranges. + expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(3); + expect(mocks.cachedItemIds).toEqual([]); + + mocks.failFetches = false; + await advance(THROTTLE_MS * 4); + 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('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([]); + }); +}); From e464cf3d6cca21fdebd66728c0d92dadac2abbdb Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 8 Aug 2026 08:57:42 -0400 Subject: [PATCH 3/3] fix(ui): bound the range-fetch retry with backoff and coalesced ranges Review feedback on the retry added in this PR: restoring the failed ranges immediately meant a sustained backend outage produced a request every throttle window forever, the restored state grew by a duplicate range per cycle, and `prev.length > 0 ? prev : ranges` dropped a failed range whenever another had been reported in the meantime. Replace the immediate restore with a shared useBoundedRangeRetry hook: - Exponential backoff between retries (1s, 2s, 4s, 8s, capped at 16s), giving up after 5 consecutive scheduled retries, so a sustained failure terminates instead of storming a backend that is trying to come back up. - Failed ranges accumulate as a coalesced (sorted, disjoint) union, and the restore merges them into whatever is pending instead of choosing one side, so nothing is dropped and nothing grows without bound. - A new range report resets the retry budget: fresh user input revives a list that gave up, and rows still in view are re-reported by virtuoso when the user scrolls back anyway. Tests: negative-path coverage for both hooks (sustained failure terminates; scrolling revives a given-up list; a range that failed mid-scroll is recovered) plus unit tests for coalesceRanges. Mutation-verified: removing the backoff/cap, the budget reset, the merge-on-restore, or the retry itself each makes at least one test fail; all 30 pass with the change in place. Co-Authored-By: Claude Fable 5 --- .../common/hooks/useBoundedRangeRetry.test.ts | 68 ++++++++++ .../src/common/hooks/useBoundedRangeRetry.ts | 119 ++++++++++++++++++ .../hooks/useRangeBasedImageFetching.test.ts | 73 ++++++++++- .../hooks/useRangeBasedImageFetching.ts | 33 +++-- .../useRangeBasedQueueItemFetching.test.ts | 73 ++++++++++- .../hooks/useRangeBasedQueueItemFetching.ts | 32 +++-- 6 files changed, 375 insertions(+), 23 deletions(-) create mode 100644 invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.test.ts create mode 100644 invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts 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..370439b8af6 --- /dev/null +++ b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts @@ -0,0 +1,119 @@ +import { useCallback, useEffect, useRef } from 'react'; +import type { ListRange } from 'react-virtuoso'; + +const RETRY_INITIAL_DELAY_MS = 1_000; +const RETRY_MAX_DELAY_MS = 16_000; +const RETRY_MAX_ATTEMPTS = 5; + +/** + * 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 gives up 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. + * + * `restoreRanges` is invoked when a retry fires, with the coalesced union of every range that + * failed since the last retry. It must be referentially stable (wrap it in `useCallback`). + */ +export const useBoundedRangeRetry = ( + restoreRanges: (failedRanges: ListRange[]) => void +): UseBoundedRangeRetryReturn => { + const stateRef = useRef<{ + attempts: number; + failedRanges: ListRange[]; + timeoutId: ReturnType | null; + }>({ attempts: 0, failedRanges: [], timeoutId: null }); + + useEffect(() => { + const state = stateRef.current; + return () => { + 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 onFetchFailure = useCallback( + (ranges: ListRange[]) => { + const state = stateRef.current; + 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 — abandon these ranges rather than letting them accumulate. The rows + // still in view are re-reported by the next range change, which also resets the budget. + 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) { + restoreRanges(failedRanges); + } + }, delay); + }, + [restoreRanges] + ); + + const resetRetryBudget = useCallback(() => { + stateRef.current.attempts = 0; + }, []); + + 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 8194c5c965c..eeba69648aa 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts @@ -177,9 +177,10 @@ describe('useRangeBasedImageFetching', () => { scrollTo({ startIndex: 0, endIndex: 2 }); await advance(THROTTLE_MS * 4); - // The catch-driven retry produces a fetch per throttle window. Without it, clearing - // pendingRanges after the failed fetch still re-runs the effect once, so the count caps at - // two — three or more requires the catch handler restoring the ranges. + // 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([]); @@ -194,6 +195,72 @@ describe('useRangeBasedImageFetching', () => { 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('recovers a range that failed while the user was scrolling elsewhere', async () => { + // 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 grey placeholders. The retry now merges the failed ranges with + // whatever is pending instead of choosing one side, so both ranges end up fetched with no + // further user input. + const names = ['a.png', 'b.png', 'c.png', 'd.png', 'e.png', 'f.png', 'g.png', 'h.png', 'i.png']; + mocks.failFetches = true; + renderHook(names, true); + scrollTo({ startIndex: 0, endIndex: 2 }); + // The fetches for the failed range land at t=500 (throttle edges), scheduling the 1s backoff + // retry for t=1500. + await advance(1_250); + + // The backend recovers, and the user scrolls to a disjoint range. The first report fires on + // the throttle's leading edge (t=1250); the second lands in pendingRanges and stays there + // until the trailing edge (t=1750) — so the backoff retry at t=1500 finds a non-empty + // pendingRanges and must merge into it rather than pick a side. + mocks.failFetches = false; + scrollTo({ startIndex: 6, endIndex: 8 }); + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(3_000); + + // Both the failed range (a-c) and the new one (g-i) 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); diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts index 6264189747f..91ca4884906 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts @@ -1,5 +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'; @@ -54,6 +55,13 @@ export const useRangeBasedImageFetching = ({ const [lastRange, setLastRange] = useState(null); 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[]) => { if (!enabled) { @@ -67,13 +75,14 @@ export const useRangeBasedImageFetching = ({ if (uncachedImageNames.length > 0) { 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. Put the ranges back - // so the effect re-runs and tries again — otherwise a transient failure leaves grey - // placeholders until the user happens to scroll. The throttle bounds the retry rate. - setPendingRanges((prev) => (prev.length > 0 ? prev : ranges)); + // 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); }); } @@ -95,15 +104,21 @@ export const useRangeBasedImageFetching = ({ // React bail out instead. setPendingRanges(EMPTY_ARRAY); }, - [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; 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 fb40bd55d35..6efc5e77f73 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts @@ -154,9 +154,10 @@ describe('useRangeBasedQueueItemFetching', () => { scrollTo({ startIndex: 0, endIndex: 2 }); await advance(THROTTLE_MS * 4); - // The catch-driven retry produces a fetch per throttle window. Without it, clearing - // pendingRanges after the failed fetch still re-runs the effect once, so the count caps at - // two — three or more requires the catch handler restoring the ranges. + // 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.queueFetches.length).toBeGreaterThanOrEqual(3); expect(mocks.cachedItemIds).toEqual([]); @@ -171,6 +172,72 @@ describe('useRangeBasedQueueItemFetching', () => { 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('recovers a range that failed while the user was scrolling elsewhere', async () => { + // 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, 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 }); + // The fetches for the failed range land at t=500 (throttle edges), scheduling the 1s backoff + // retry for t=1500. + await advance(1_250); + + // The backend recovers, and the user scrolls to a disjoint range. The first report fires on + // the throttle's leading edge (t=1250); the second lands in pendingRanges and stays there + // until the trailing edge (t=1750) — so the backoff retry at t=1500 finds a non-empty + // pendingRanges and must merge into it rather than pick a side. + mocks.failFetches = false; + scrollTo({ startIndex: 6, endIndex: 8 }); + scrollTo({ startIndex: 6, endIndex: 8 }); + await advance(3_000); + + // Both the failed range (1-3) and the new one (7-9) 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); diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts index 33697875542..58cccd696e0 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts @@ -1,5 +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, useState } from 'react'; import type { ListRange } from 'react-virtuoso'; import { queueApi, useGetQueueItemDTOsByItemIdsMutation } from 'services/api/endpoints/queue'; @@ -44,6 +45,13 @@ export const useRangeBasedQueueItemFetching = ({ const [lastRange, setLastRange] = useState(null); 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 fetchQueueItems = useCallback( (ranges: ListRange[], itemIds: number[]) => { if (!enabled) { @@ -54,12 +62,14 @@ export const useRangeBasedQueueItemFetching = ({ if (uncachedItemIds.length > 0) { getQueueItemDTOsByItemIds({ item_ids: uncachedItemIds }) .unwrap() + .then(resetRetryBudget) .catch(() => { // This bulk fetch is the ONLY fetcher for these rows: `QueueItemAtPosition` consumes // the cache with `skip: isUninitialized`, so a row whose DTO never arrived does not - // fetch for itself. Put the ranges back so the effect re-runs and tries again — - // otherwise a transient failure leaves placeholders until the user happens to scroll. - setPendingRanges((prev) => (prev.length > 0 ? prev : ranges)); + // 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); }); } // Clear unconditionally. Returning early without clearing (the previous behaviour when @@ -73,15 +83,21 @@ export const useRangeBasedQueueItemFetching = ({ // uncached; clearing on both paths means the stable reference is now what stops it. setPendingRanges(EMPTY_ARRAY); }, - [enabled, getQueueItemDTOsByItemIds, store] + [enabled, getQueueItemDTOsByItemIds, 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;