From bc6a1a1d3b73582b651190011f82e8afd5d57e02 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 1 Aug 2026 21:49:01 -0400 Subject: [PATCH 1/5] 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/5] 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/5] 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; From 4768246f1553ea9167a1d5fe4be67a83788858a3 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 29 Aug 2026 11:21:57 -0400 Subject: [PATCH 4/5] fix(ui): heal an abandoned range fetch on reconnect, and bound the retry's lifetime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review findings on the bounded range-fetch retry. - Giving up was permanent. The budget ends ~31s after the first failure, but an InvokeAI restart routinely takes longer, and for an idle user nothing re-arms it: `imageNames` keeps its identity through a reconnect refetch, `enabled` (`!isLoading`) does not toggle on a refetch, and in production `socketConnected` only invalidates `FetchOnReconnect` when the queue status changed. Ranges abandoned by an exhausted budget are now parked as a coalesced union instead of dropped, and restored on the next signal that the backend is answering: a socket reconnect, a successful fetch, or a fresh range report. - A fetch that rejected after unmount armed a backoff timer no cleanup could reach. The retry state now tracks mount status and drops late failures. - The `!enabled` guard returned before the clear — the same accumulate-forever pattern this PR fixes on the cached path. Both hooks now clear on that path. - `restoreRanges` is read through a ref, so an unstable callback can no longer churn `onFetchFailure` and the fetch effect behind it. Tests: reconnect healing, a post-unmount rejection arming no timer, and the disabled-window accumulation case in both hook suites, plus the queue suite's missing mount-time no-loop test. Each is mutation-verified against the fix it covers. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QWxRMfb5wDBi6isQ6XKrgE --- .../src/common/hooks/useBoundedRangeRetry.ts | 111 +++++++++----- .../hooks/useRangeBasedImageFetching.test.ts | 124 +++++++++++++++- .../hooks/useRangeBasedImageFetching.ts | 4 + .../useRangeBasedQueueItemFetching.test.ts | 135 +++++++++++++++++- .../hooks/useRangeBasedQueueItemFetching.ts | 4 + 5 files changed, 334 insertions(+), 44 deletions(-) diff --git a/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts index 370439b8af6..79b35a3df8f 100644 --- a/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts +++ b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, 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; @@ -36,7 +37,7 @@ 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. + * hook stops scheduling and parks the ranges until the budget is reset. */ onFetchFailure: (ranges: ListRange[]) => void; /** @@ -57,8 +58,15 @@ interface UseBoundedRangeRetryReturn { * 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`). + * 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. + * + * `restoreRanges` is invoked with the coalesced union of every range that failed since the last + * retry. It is read through a ref, so it does not need to be referentially stable. */ export const useBoundedRangeRetry = ( restoreRanges: (failedRanges: ListRange[]) => void @@ -66,12 +74,26 @@ export const useBoundedRangeRetry = ( const stateRef = useRef<{ attempts: number; failedRanges: ListRange[]; + abandonedRanges: ListRange[]; timeoutId: ReturnType | null; - }>({ attempts: 0, failedRanges: [], timeoutId: null }); + isMounted: boolean; + }>({ attempts: 0, failedRanges: [], abandonedRanges: [], timeoutId: null, isMounted: true }); + + // 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); + useEffect(() => { + 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) @@ -83,37 +105,62 @@ export const useBoundedRangeRetry = ( }; }, []); - 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 = []; + const restoreAbandonedRanges = useCallback(() => { + const state = stateRef.current; + if (!state.isMounted) { + return; + } + state.attempts = 0; + if (state.abandonedRanges.length === 0) { + return; + } + const abandonedRanges = state.abandonedRanges; + state.abandonedRanges = []; + restoreRangesRef.current(abandonedRanges); + }, []); + + 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; } - 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] - ); + restoreAbandonedRanges(); + }); + }, [restoreAbandonedRanges]); - const resetRetryBudget = useCallback(() => { - stateRef.current.attempts = 0; + 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 }; + return { onFetchFailure, resetRetryBudget: restoreAbandonedRanges }; }; 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 eeba69648aa..5d8a8b05f2e 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts @@ -2,6 +2,7 @@ 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, useRangeBasedImageFetching } from './useRangeBasedImageFetching'; @@ -19,6 +20,10 @@ const mocks = vi.hoisted(() => ({ 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', () => { @@ -33,6 +38,15 @@ vi.mock('features/gallery/store/types', () => ({ 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 @@ -70,15 +84,24 @@ describe('useRangeBasedImageFetching', () => { 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) => { - const Harness: FC = () => { - renderCount++; - hookReturn = useRangeBasedImageFetching({ imageNames, enabled }); - return null; - }; root = createRoot(document.createElement('div')); act(() => { - root!.render(createElement(Harness)); + root!.render(createElement(Harness, { imageNames, enabled })); + }); + }; + + const rerenderHook = (imageNames: string[], enabled: boolean) => { + act(() => { + root!.render(createElement(Harness, { imageNames, enabled })); }); }; @@ -108,7 +131,10 @@ describe('useRangeBasedImageFetching', () => { mocks.cachedImageNames = []; mocks.cacheLands = true; mocks.failFetches = false; + mocks.manualFailure = false; + mocks.rejectPending = []; renderCount = 0; + $isConnected.set(false); }); afterEach(() => { @@ -118,6 +144,7 @@ describe('useRangeBasedImageFetching', () => { }); root = null; } + $isConnected.set(false); vi.useRealTimers(); }); @@ -233,6 +260,91 @@ describe('useRangeBasedImageFetching', () => { expect(mocks.imageFetches.length).toBeGreaterThanOrEqual(fetchesAfterGiveUp + 3); }); + 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); + }); + + 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']); + }); + 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 diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts index 91ca4884906..c5a90b434c5 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts @@ -65,6 +65,10 @@ export const useRangeBasedImageFetching = ({ const fetchItems = useCallback( (ranges: ListRange[], allNames: string[]) => { 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(EMPTY_ARRAY); return; } const state = store.getState(); 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 74e6eb79ba6..a317c487eb0 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts @@ -2,6 +2,7 @@ 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, useRangeBasedQueueItemFetching } from './useRangeBasedQueueItemFetching'; @@ -18,6 +19,10 @@ const mocks = vi.hoisted(() => ({ 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', () => { @@ -28,6 +33,15 @@ vi.mock('app/store/storeHooks', () => { 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 @@ -86,15 +100,24 @@ describe('useRangeBasedQueueItemFetching', () => { 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) => { - const Harness: FC = () => { - renderCount++; - hookReturn = useRangeBasedQueueItemFetching({ itemIds, enabled }); - return null; - }; root = createRoot(document.createElement('div')); act(() => { - root!.render(createElement(Harness)); + root!.render(createElement(Harness, { itemIds, enabled })); + }); + }; + + const rerenderHook = (itemIds: number[], enabled: boolean) => { + act(() => { + root!.render(createElement(Harness, { itemIds, enabled })); }); }; @@ -124,7 +147,10 @@ describe('useRangeBasedQueueItemFetching', () => { mocks.cachedItemIds = []; mocks.cacheLands = true; mocks.failFetches = false; + mocks.manualFailure = false; + mocks.rejectPending = []; renderCount = 0; + $isConnected.set(false); }); afterEach(() => { @@ -134,9 +160,22 @@ describe('useRangeBasedQueueItemFetching', () => { }); 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 }); @@ -236,6 +275,90 @@ describe('useRangeBasedQueueItemFetching', () => { expect(mocks.queueFetches.length).toBeGreaterThanOrEqual(fetchesAfterGiveUp + 3); }); + 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); + }); + + 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]); + }); + 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 diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts index 7a66af302b8..b2643c378d7 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts @@ -77,6 +77,10 @@ export const useRangeBasedQueueItemFetching = ({ const fetchQueueItems = useCallback( (ranges: ListRange[], itemIds: number[]) => { 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(EMPTY_ARRAY); return; } const cachedItemIds = queueApi.util.selectCachedArgsForQuery(store.getState(), 'getQueueItemSummary'); From 3d278c59c816a7ed9a45ded631507a60969ea6ba Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 29 Aug 2026 11:47:50 -0400 Subject: [PATCH 5/5] fix(ui): stop losing restored ranges to the throttle, and floor the reconnect re-arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the previous commit found two real defects. Restored ranges could be lost permanently. `restoreRanges` dispatches a functional `setPendingRanges`, but the fetch pass ended with an absolute `setPendingRanges(EMPTY_ARRAY)`. A backoff timer and the throttle's trailing edge can expire in the same event-loop turn, so both land in one React batch: the absolute update runs last, the final state equals the base, React bails out of the re-render, and the ranges are gone with nothing left to re-report them. Reproduced deterministically in both hooks (fail a range, scroll elsewhere 600-1000ms later, recover: the first range is never fetched again — grey rows until the user scrolls back). The clear now only fires when `pendingRanges` is still the array that pass consumed, so it is a no-op once the state has moved on. The existing scroll-recovery test was pinned to a delay that happened to miss this window; it now sweeps 500-1250ms and fails at three of five without the fix. The reconnect signal made the bounded retry unbounded. `attempts` was zeroed on every `$isConnected` transition, even with nothing parked, so a socket that keeps completing a handshake while REST stays broken (crash-looping container, uvicorn accepting connections before startup finishes, a proxy splitting websocket and REST across replicas) pinned the backoff at its shortest delay: 300 requests over five minutes of 5s flapping, against a design intent of 12. The re-arm is now floored at one per 60s and only fires when there is something parked to heal — 70 requests in the same scenario. Also: the latest-callback ref moved to a layout effect so a restore firing before the passive flush sees the intended closure. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QWxRMfb5wDBi6isQ6XKrgE --- .../src/common/hooks/useBoundedRangeRetry.ts | 72 ++++++++--- .../hooks/useRangeBasedImageFetching.test.ts | 121 ++++++++++++++---- .../hooks/useRangeBasedImageFetching.ts | 15 ++- .../useRangeBasedQueueItemFetching.test.ts | 121 ++++++++++++++---- .../hooks/useRangeBasedQueueItemFetching.ts | 15 ++- 5 files changed, 267 insertions(+), 77 deletions(-) diff --git a/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts index 79b35a3df8f..89415b9dd2a 100644 --- a/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts +++ b/invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts @@ -1,10 +1,18 @@ -import { useCallback, useEffect, useRef } from 'react'; +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. @@ -63,10 +71,14 @@ interface UseBoundedRangeRetryReturn { * 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. + * 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 it does not need to be referentially stable. + * 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 @@ -77,13 +89,21 @@ export const useBoundedRangeRetry = ( abandonedRanges: ListRange[]; timeoutId: ReturnType | null; isMounted: boolean; - }>({ attempts: 0, failedRanges: [], abandonedRanges: [], timeoutId: null, isMounted: true }); + 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); - useEffect(() => { + useLayoutEffect(() => { restoreRangesRef.current = restoreRanges; }, [restoreRanges]); @@ -105,19 +125,27 @@ export const useBoundedRangeRetry = ( }; }, []); - const restoreAbandonedRanges = useCallback(() => { + 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; - if (state.abandonedRanges.length === 0) { - return; + const abandonedRanges = takeAbandonedRanges(); + if (abandonedRanges) { + restoreRangesRef.current(abandonedRanges); } - const abandonedRanges = state.abandonedRanges; - state.abandonedRanges = []; - restoreRangesRef.current(abandonedRanges); - }, []); + }, [takeAbandonedRanges]); useEffect(() => { // A reconnect means the backend is answering again. Nothing else re-arms an exhausted budget @@ -129,9 +157,23 @@ export const useBoundedRangeRetry = ( if (!isConnected) { return; } - restoreAbandonedRanges(); + 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); }); - }, [restoreAbandonedRanges]); + }, [takeAbandonedRanges]); const onFetchFailure = useCallback((ranges: ListRange[]) => { const state = stateRef.current; @@ -162,5 +204,5 @@ export const useBoundedRangeRetry = ( }, delay); }, []); - return { onFetchFailure, resetRetryBudget: restoreAbandonedRanges }; + 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 5d8a8b05f2e..53c45aac91e 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.test.ts @@ -289,6 +289,69 @@ describe('useRangeBasedImageFetching', () => { 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 () => { @@ -345,33 +408,37 @@ describe('useRangeBasedImageFetching', () => { expect(mocks.imageFetches.flat().sort()).toEqual(['d.png', 'e.png', 'f.png', 'g.png', 'h.png', 'i.png']); }); - 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']); - }); + // 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']; diff --git a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts index c5a90b434c5..84fa04501f3 100644 --- a/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts +++ b/invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts @@ -63,12 +63,12 @@ export const useRangeBasedImageFetching = ({ 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(EMPTY_ARRAY); + setPendingRanges((prev) => (prev === handledPendingRanges ? EMPTY_ARRAY : prev)); return; } const state = store.getState(); @@ -106,7 +106,14 @@ export const useRangeBasedImageFetching = ({ // 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); + // + // 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, onFetchFailure, resetRetryBudget, store] ); @@ -126,7 +133,7 @@ export const useRangeBasedImageFetching = ({ 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 a317c487eb0..abb385e0c9b 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts @@ -303,6 +303,69 @@ describe('useRangeBasedQueueItemFetching', () => { 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 () => { @@ -359,33 +422,37 @@ describe('useRangeBasedQueueItemFetching', () => { expect(mocks.queueFetches.flat().sort((a, b) => a - b)).toEqual([4, 5, 6, 7, 8, 9]); }); - 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]); - }); + // 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]; diff --git a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts index b2643c378d7..6465438314c 100644 --- a/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts +++ b/invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts @@ -75,12 +75,12 @@ export const useRangeBasedQueueItemFetching = ({ 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(EMPTY_ARRAY); + setPendingRanges((prev) => (prev === handledPendingRanges ? EMPTY_ARRAY : prev)); return; } const cachedItemIds = queueApi.util.selectCachedArgsForQuery(store.getState(), 'getQueueItemSummary'); @@ -114,7 +114,14 @@ export const useRangeBasedQueueItemFetching = ({ // 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); + // + // 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, onFetchFailure, resetRetryBudget, store] ); @@ -134,7 +141,7 @@ export const useRangeBasedQueueItemFetching = ({ useEffect(() => { const combinedRanges = lastRange ? [...pendingRanges, lastRange] : pendingRanges; - throttledFetchQueueItems(combinedRanges, itemIds); + throttledFetchQueueItems(combinedRanges, itemIds, pendingRanges); }, [itemIds, lastRange, pendingRanges, throttledFetchQueueItems]); return {