Skip to content

fix(ui): stop range-based fetching hooks from spinning in a render loop - #9439

Open
lstein wants to merge 15 commits into
invoke-ai:mainfrom
lstein:fix/range-based-fetching-render-loop
Open

fix(ui): stop range-based fetching hooks from spinning in a render loop#9439
lstein wants to merge 15 commits into
invoke-ai:mainfrom
lstein:fix/range-based-fetching-render-loop

Conversation

@lstein

@lstein lstein commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix (performance). useRangeBasedImageFetching and useRangeBasedQueueItemFetching spin in a self-sustaining render loop for as long as the gallery grid / queue list is mounted.

The shape of the bug, in useRangeBasedImageFetching:

const fetchItems = useCallback((ranges, allNames) => {
  ...
  setPendingRanges([]);           // ← new array identity, every call
}, [enabled, getImageDTOsByNames, store]);

const throttledFetchItems = useThrottledCallback(fetchItems, 500);

useEffect(() => {
  throttledFetchItems([...pendingRanges, lastRange], imageNames);
}, [imageNames, lastRange, pendingRanges, throttledFetchItems]);
//                         ^^^^^^^^^^^^^ dependency

setPendingRanges([]) installs a fresh array, which is never Object.is-equal to the previous one, so the effect re-runs. useThrottledCallback resolves to {maxWait: 500, leading: true, trailing: true}, so the re-entry schedules a trailing invocation, which calls fetchItems, which clears again. Round and round, several times a second, indefinitely, with no user input. In the gallery hook the clear is unconditional, so the loop runs from mount even when there is nothing to fetch; the queue hook returned early before clearing when everything was cached, so there it only ran while items were genuinely uncached.

Most of the time this only burns CPU and re-renders. It turns into a permanent 2Hz request stream whenever a name in the visible range never lands in the cache — because getImageDTOsByNames.onQueryStarted upserts only the DTOs the server actually returned:

for (const imageDTO of imageDTOs) {
  updates.push({ endpointName: 'getImageDTO', arg: imageDTO.image_name, value: imageDTO });
}

A requested name that comes back missing (a deleted image still present in the name list, an item filtered out by ownership in multiuser mode) therefore never gets a getImageDTO cache entry, selectCachedArgsForQuery never reports it, and it is re-requested on every pass — twice a second, forever.

The change: clear with the shared stable EMPTY_ARRAY reference from app/store/constants.ts, which is already used across the app for exactly this purpose. Setting state to the value it already holds makes React bail out instead of re-running the effect. Real range changes still flow through onRangeChanged, which sets lastRange to a new object, so fetching on scroll is unaffected.

The queue variant also returned early without clearing when nothing was uncached, letting ranges accumulate for the lifetime of the list and growing the scan on every subsequent pass. It now clears on both paths — the ranges have been handled either way.

The loop was also an accidental retry, so the retry is now explicit. These bulk fetches are the only fetcher for their rows. ImageAtPosition and QueueItemAtPosition both consume the cache with the documented "subscribe once it has data" hack:

imagesApi.endpoints.getImageDTO.useQuerySubscription(isVideo ? '' : imageName, {
  skip: isVideo || imageState.isUninitialized,
});

so a row whose DTO never arrived does not fetch for itself, onQueryStarted swallows the failure in catch {}, and nobody reads the mutation's error state. Videos have a retry button; images and queue items do not. Pre-fix, a failed bulk fetch was simply re-attempted by the loop until it succeeded. Removing the loop without replacing that would mean a transient failure — a backend restart, a 502 from a reverse proxy — leaves grey placeholders until the user happens to scroll, since nothing else changes any dependency of the effect (RTK Query's structuralSharing preserves the imageNames reference even across a refetch). So the failure path now hands the ranges to a shared bounded retry (common/hooks/useBoundedRangeRetry.ts): exponential backoff between attempts (1s, 2s, 4s, 8s, capped at 16s), giving up after 5 consecutive scheduled retries, with the failed ranges accumulated as a coalesced (sorted, disjoint) union that is merged into whatever is pending rather than replacing it. Any new range report resets the retry budget, so scrolling revives a list that gave up. (The first iteration restored the ranges immediately, bounded only by the throttle; Pfannkuchensack's review correctly flagged that as a fixed-rate request storm under sustained failure, with unbounded state growth and a window where a failed range could be dropped mid-scroll — the bounded design resolves all three.)

Related Issues / Discussions

Found while investigating an unrelated report of repeated socket connections; see #9438 for that one. No existing issue.

QA Instructions

The loop is easiest to see with React DevTools:

  1. Open the gallery with a board that has enough images to virtualize.
  2. In React DevTools → Components → Settings, enable "Highlight updates when components render".
  3. Before this change: the gallery re-renders continuously, twice a second, with the mouse untouched. After: it goes quiet once scrolling stops.

For the network half, you need a name that the server will not return — e.g. delete an image directly from the DB (or via another client) so it stays in the cached name list, then scroll it into view. Before: POST /api/v1/images/images_by_names repeats every ~500ms indefinitely in the Network tab. After: it fires once per range change.

Regression checks:

  • Scroll the gallery fast through un-fetched regions; thumbnails should still resolve, with no gaps.
  • Same for the queue list with many pending items.
  • Both lists should still fetch correctly after switching boards / filters (which changes imageNames).
  • Self-healing after a failed fetch: stop the backend while the gallery is open, scroll to an un-fetched region so the bulk fetch fails, then restart the backend. The placeholders should fill in on their own, without scrolling. This is the behaviour the loop was providing accidentally.

Notes for reviewers

Two things worth knowing, both found by adversarially reviewing this diff:

  • The retry-on-failure is not gold-plating; it is preserving existing behaviour. Without it this change introduces a real regression (permanent placeholders after any transient bulk-fetch failure), because the loop it removes was the only retry these rows had.
  • Out of scope, but adjacent: EMPTY_ARRAY in app/store/constants.ts is never[] — mutable, unfrozen, and now referenced from ~30 files and held as component state by these hooks. Nothing mutates it today (audited), but a future pendingRanges.push(...) would compile fine and silently corrupt unrelated selectors app-wide. Object.freeze([]) there would close that off without any type churn. Happy to do it separately if wanted.

Automated: regression tests added for both hooks (useRangeBasedImageFetching.test.ts, useRangeBasedQueueItemFetching.test.ts). They render the real 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; the rest of the suite stays in the node environment), mocking only the thin API-endpoint modules so the actual state/effect/throttle cycle is exercised. Covered per hook: a reported range fetches its uncached items once and then renders and fetches go quiet; never-cached items (the deleted-image / multiuser-filter case) are re-requested boundedly rather than forever; a failed bulk fetch retries until it succeeds, then goes quiet; every range reported within a throttle window is fetched, not just the last; handled ranges are dropped rather than accumulated (an item evicted from a long-handled range is not re-requested — the queue hook's pre-fix early return regressed exactly this); new ranges after settling still fetch; enabled: false fetches nothing. Also covered: sustained failure terminates (fetch count bounded, then frozen over a further 30 simulated seconds); scrolling revives a given-up list; a range that fails while the user is scrolling elsewhere is recovered; and unit tests for coalesceRanges, including the duplicate-per-cycle growth case. Mutation-verified: reverting the EMPTY_ARRAY clears, restoring the queue hook's early return, dropping onRangeChanged's accumulation, removing the retry, removing the backoff/cap, removing the budget reset, or replacing the merge-on-restore with either side each makes at least one test fail; all 30 pass with the fix in place.

pnpm test:no-watch — full suite passes (1817 tests). pnpm lint:eslint, pnpm lint:prettier, pnpm lint:tsc, pnpm lint:knip all clean.

Merge Plan

Ordinary merge. No redux slice changes, so no migration.

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • ❗Changes to a redux slice have a corresponding migration — n/a
  • Documentation added / updated (if applicable) — n/a
  • Updated What's New copy (if doing a release after this PR) — n/a

`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) <noreply@anthropic.com>
@github-actions github-actions Bot added the frontend PRs that change frontend files label Aug 2, 2026
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 <noreply@anthropic.com>
@github-actions github-actions Bot added the frontend-deps PRs that change frontend dependencies label Aug 2, 2026
@Pfannkuchensack

Copy link
Copy Markdown
Member

PR #9439 — Stop range-based fetching hooks from spinning in a render loop

  • The new retry is itself an unbounded request storm. invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts:76 (and queue :62) closes the loop: catch reinstalls rangespendingRanges is a dependency of the effect (:111) → effect re-runs → throttle → fetch fails → catch. Measured: 120 requests in 30 simulated seconds, exactly 4/s, linear, still climbing — the same order as the 3.79/s measured live for the pre-fix loop. There is no attempt counter, no backoff, and no retry()/maxRetries anywhere in the API layer (verified). Trigger is the scenario the PR's own comment names: backend restart, OOM kill, 502 from a reverse proxy. Not a regression against main, but the pathology is preserved on the failure path and now locked in by a test. (401 is not a vector — it self-terminates via sessionExpiredLogout.)
  • The retry state grows without bound while the failure persists. Each cycle appends lastRange again and nothing truncates: measured +8 range entries per 500 ms cycle, monotonic — linear scan cost, quadratic total work. This is precisely the pathology the PR's own queue-hook comment cites as the reason to clear unconditionally.
  • prev.length > 0 ? prev : ranges drops failed ranges during a scroll. Reproduced: range A dispatches; 100 ms later onRangeChanged pushes B into pendingRanges; A then rejects; the catch sees prev.length > 0, keeps B, and A is lost. Those rows stay grey placeholders — unlike videos, images have no error tile and no retry button (invokeai/frontend/web/src/features/gallery/components/GalleryImageGrid.tsx:81-95). Virtuoso fires onRangeChanged many times per second, so the window is wide open exactly while the user scrolls. The retry test scrolls once and then leaves the hook idle.
  • Consider removing the retry entirely by giving images the error tile + manual retry button videos already have. That would resolve all three items above. The PR went the other way without stating why.
  • The tests mock away the assumption the fix rests on. Both suites hardcode a referentially stable mutation trigger. If a future @reduxjs/toolkit returned a fresh trigger identity per render, the render loop would come straight back and all 17 tests would still pass. Assert trigger stability across a forced re-render instead.
  • Missing negative-path coverage: no test asserts that sustained failure terminates, that the retry state stops growing, or that a range failing during a concurrent scroll is ever recovered.
  • Open question: does the gallery panel stay mounted when its dockview panel is hidden or the user switches tabs? If so, the storm continues off-screen and invisible.

Pfannkuchensack
Pfannkuchensack previously approved these changes Aug 7, 2026

@Pfannkuchensack Pfannkuchensack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. we need to check more for if a version bump is needed. Wrong pr

@Pfannkuchensack
Pfannkuchensack self-requested a review August 7, 2026 19:26
lstein and others added 2 commits August 8, 2026 08:33
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 <noreply@anthropic.com>
@lstein

lstein commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for this review — the three findings on the retry path are all real, and they're now addressed in e464cf3. On the headline suggestion (remove the retry, add a retry button): I went the other way deliberately, and here's the reasoning.

Why not a retry button (yet): the video error tile works because each video row owns a real getVideoDTO query whose state transitions to isError. Image and queue rows deliberately have no per-row query on failure — the whole skip: isUninitialized design means a row whose DTO never arrived via the bulk mutation is uninitialized, indistinguishable from "not fetched yet". There is no error signal to render a tile from, and the batch mutation fails as a unit, so it can't say which names failed. Building that plumbing (or letting rows fall back to self-fetching on error, which reintroduces the request-per-row pattern the bulk hook exists to avoid) is a real redesign, not a drop-in. It also only covers half the surface: the queue hook has the identical retry, and queue rows are text rows with no tile to hang a button on. And since these failures are overwhelmingly transient (backend restart, brief 502), button-only would turn every blip into dozens of dead tiles needing individual clicks. An error tile after retries exhaust would be a fine complement — happy to see that as a follow-up — but it shouldn't replace automatic recovery for the transient case.

What the findings actually warranted, and what changed — the retry is now bounded via a shared useBoundedRangeRetry hook:

  • Storm → exponential backoff (1s, 2s, 4s, 8s, capped at 16s), giving up after 5 consecutive scheduled retries. A sustained outage now produces ~6–12 requests total and then goes quiet, instead of 2/s per hook forever. (One clarification on the measurement: the pre-fix(ui): stop range-based fetching hooks from spinning in a render loop #9439 loop ran unconditionally, even on success; the retry only ran while requests were actively failing. But "fixed-rate, no backoff, forever, from every open tab" was still wrong, agreed.)
  • Unbounded state growth → failed ranges accumulate as a coalesced union (sorted, disjoint, deduplicated), so repeated failures over the same viewport collapse to one entry instead of growing per cycle.
  • Dropped ranges during scroll → the restore now merges the failed ranges into whatever is pending (coalesceRanges([...prev, ...failed])) instead of prev.length > 0 ? prev : ranges. Worth noting the practical severity was lower than it looks: lastRange is appended to every fetch pass and virtuoso re-reports ranges on scroll-back, so permanently-grey rows required the narrow overscan case — but the either/or was still wrong and is gone.
  • Given-up ≠ dead: any new onRangeChanged resets the retry budget, so scrolling revives a list that gave up.

Negative-path coverage (your bullet 6) is now in both hook suites: sustained failure terminates (fetch count bounded, then frozen over a further 30 simulated seconds); scrolling revives a given-up list; and a range that fails while the user is scrolling elsewhere is recovered — that last one is timed so the backoff retry fires while another scroll report is mid-throttle-window, which is exactly the interleaving that kills the either/or restore. All of these are mutation-verified: removing the backoff/cap, the budget reset, the merge-on-restore, or the retry itself each fails at least one test. coalesceRanges has its own unit tests including the duplicate-per-cycle growth case.

On the mock stability point (bullet 5): the trigger's referential stability is RTK Query's documented contract (the trigger is useCallback'd on [dispatch, initiate, fixedCacheKey]), and the mock mirrors that contract rather than assuming it away. A test asserting the real trigger's identity across renders would be testing the library — and if a future RTK Query broke it, every useCallback depending on a trigger across the app would churn, not just these hooks. I'd rather catch that in a toolkit upgrade PR than pin it here.

Open question (bullet 7): enabled is !isLoading at both call sites — it tracks query state, not panel visibility — so yes, a mounted-but-hidden panel kept retrying pre-change. With the bound it self-terminates within ~31s regardless of visibility, and nothing re-arms it until the user actually scrolls again.

@Pfannkuchensack

Copy link
Copy Markdown
Member

Findings

1. Medium - the bounded retry gives up permanently after ~31s and nothing re-arms it

invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts:94-99

A backend outage longer than the retry budget leaves grey tiles with no automatic recovery and no user affordance.

Chain:

  1. Delays are 1/2/4/8/16s (useBoundedRangeRetry.ts:4-6,101), so give-up lands ~31s after the first failed fetch.
  2. On give-up the hook drops state.failedRanges and returns (useBoundedRangeRetry.ts:94-99). The only re-arms are resetRetryBudget from onRangeChanged (useRangeBasedImageFetching.ts:112-121) or a successful fetch (:78).
  3. A fetch only fires when an effect dependency changes: imageNames, lastRange, pendingRanges, throttledFetchItems (useRangeBasedImageFetching.ts:123-126).
  4. On backend recovery in a production build, app/store/middleware/listenerMiddleware/listeners/socketConnected.ts:41-43 resets the API state only in development; production reaches :68-70 and returns early unless the queue status changed.
  5. Even when FetchOnReconnect is invalidated (:74), RTK Query structural sharing preserves currentData.items, so the useMemo in features/gallery/components/use-gallery-image-names.ts:72-75 returns the same imageNames reference. No dependency changes.
  6. enabled is !isLoading (GalleryImageGrid.tsx:402-405); a refetch sets isFetching, not isLoading, so enabled does not toggle either.
  7. The row has no fallback: GalleryImageGrid.tsx:103-106 renders a bare placeholder for images, with no error state and no retry button - unlike the video branch at :81-95.

Verified with a probe test against the PR head: after 35s of failures the hook has given up; the backend then recovers and the component re-renders with identical props; 120 further simulated seconds produce zero fetches and zero cached DTOs.

This is a behaviour regression against main, where the render loop retried at ~2Hz forever and therefore always recovered. It also means the PR's own QA step - "stop the backend ... then restart the backend. The placeholders should fill in on their own, without scrolling" - only holds for restarts under ~31s, which an InvokeAI backend restart (config load, DB migrations, model scan) frequently exceeds.

Blast radius is narrowed by the fact that an actively-generating user heals incidentally (a new image changes imageNames, and lastRange is appended to every pass); an idle user watching the gallery during a restart does not. Dev builds heal via resetApiState().

socketConnected / $isConnected is an existing signal that would close this in one line.

To expose this issue, add a test that exhausts the retry budget under sustained failure, then makes the fetch succeed again without reporting a new range, and asserts the rows are eventually fetched. The existing resumes retrying after giving up when the user scrolls (useRangeBasedImageFetching.test.ts:220-234) bakes the user input in as a precondition, so it passes either way.

2. Low - a backoff timer is armed on an unmounted instance and nothing clears it

invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts:72-84

The cleanup effect clears and nulls timeoutId at unmount. An in-flight mutation that rejects after that reaches .catch() at useRangeBasedImageFetching.ts:79-86, calls onFetchFailure, finds timeoutId === null and attempts < 5, and schedules a fresh setTimeout of up to 16s (useBoundedRangeRetry.ts:102-109) that no cleanup will ever reach.

Measured directly: vi.getTimerCount() goes 0 -> 1 across the post-unmount rejection. Consequence is a leaked timer holding the closure plus a no-op setPendingRanges on an unmounted root (silent in React 18/19). Triggered by closing the gallery or switching tabs while the backend is down. The comment at :77-81 shows cleanup-without-unmount was considered; failure-after-unmount was not.

To expose this issue, add a test that unmounts the hook while a bulk fetch is in flight, then rejects it, and asserts no timer remains scheduled.

3. Low - the !enabled path still returns early without clearing

invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.ts:57-59 and invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts:67-69

This is the exact defect the PR names and fixes on the cached path. The comment the PR adds at useRangeBasedQueueItemFetching.ts:75-78 states that returning early without clearing "let ranges accumulate for the lifetime of the list, growing the scan on every subsequent pass." The !enabled guard sits above the clear at :84 and does precisely that.

Practically bounded by how long isLoading stays true and flushed when enabled flips, so severity is low - but it is the same pattern, left in both hooks. The does not fetch when disabled tests (useRangeBasedImageFetching.test.ts:308-313, useRangeBasedQueueItemFetching.test.ts:284-289) only assert zero fetches, never that state stays bounded.

To expose this issue, add a test that reports many ranges while enabled: false, then enables, and asserts the resulting fetch covers only the current viewport rather than every range reported while disabled.

4. Low - coverage gap: the queue suite is missing the mount-time no-loop test

invokeai/frontend/web/src/features/queue/hooks/useRangeBasedQueueItemFetching.test.ts

useRangeBasedImageFetching.test.ts:141-149 asserts the grid does not loop when mounted with nothing to fetch. The queue hook previously could not loop on that path because it returned early before clearing; this PR makes its clear unconditional (useRangeBasedQueueItemFetching.ts:84), so the queue hook now depends on the EMPTY_ARRAY identity for exactly that case - newly introduced, and untested there.

To expose this issue, add a test that mounts the queue hook with nothing to fetch and asserts render count and fetch count stay frozen over several throttle windows.

Open Questions

  • useBoundedRangeRetry documents that restoreRanges must be referentially stable (useBoundedRangeRetry.ts:61) but nothing enforces it, and an unstable callback would churn onFetchFailure -> fetchItems -> throttledFetchItems -> the effect on every render. I could not construct a case where that recreates the 2Hz loop today - the EMPTY_ARRAY bail-out still breaks the self-sustaining cycle - so this is a contract risk on a newly shared hook, not a proven defect.
  • Is ~31s the intended ceiling? Raising RETRY_MAX_ATTEMPTS is a one-line partial mitigation for Finding 1, but it does not remove the unbounded-outage case; a reconnect-driven re-arm does.

lstein and others added 3 commits August 29, 2026 11:21
The queue hook was rewritten on main (getQueueItemSummary batching plus an
in-flight item-id guard). Kept both sides: main's batching and pending-id
dedup, and this branch's stable-reference clear and bounded retry. Main's
`if (uncachedItemIds.length === 0) return;` is gone — the clear at the end is
what stops the render loop, so it has to run on every path.
…try's lifetime

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QWxRMfb5wDBi6isQ6XKrgE
…econnect re-arm

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QWxRMfb5wDBi6isQ6XKrgE
@lstein

lstein commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Round-2 findings all confirmed and fixed, plus main is merged (the queue hook was rewritten on main in the meantime). Pushed as three commits: the merge, the fixes for your findings, and a third for two defects my own adversarial pass turned up in that fix — details below, since one of them affects code you reviewed.

1 — permanent give-up (medium) — fixed

Confirmed, including your point 5: I checked that RTK Query's structural sharing hands use-gallery-image-names.ts back the same imageNames reference across the reconnect refetch, so nothing in [imageNames, lastRange, pendingRanges, throttledFetchItems] changes. Worth adding that throttledFetchItems is referentially stable too (use-debounce keeps the callback in a ref), so even an enabled flip would not re-run the effect. There was no route back.

Giving up no longer means dropping. Ranges abandoned by an exhausted budget are parked as a coalesced union (so parking stays bounded, same as the retry state) and restored on the next signal that the backend is answering: a socket reconnect, a later successful fetch, or a fresh range report. $isConnected was the right hook, as you suggested.

One wrinkle that turned it into more than a one-liner: re-arming on every reconnect makes the budget per-reconnect rather than per-outage. The socket and REST can disagree — a crash-looping container completes a handshake on every restart, uvicorn accepts connections before startup finishes, a proxy can route the websocket to a healthy replica and REST to a sick one. Zeroing attempts on each transition pins the backoff at its shortest delay for as long as the flapping lasts: I measured 300 requests over five minutes of 5s flapping, against a design intent of 12. So the re-arm is floored at one per 60s and only fires when something is actually parked — same scenario now measures 70. There is a test for exactly this.

resumes retrying after giving up when the user scrolls was, as you say, baking the input in as a precondition. The new test does what you asked: exhaust the budget under sustained failure, then let the backend answer again without any range report, and assert the rows get fetched. It fails if the listener, the parking, or the re-arm is removed.

2 — timer armed on an unmounted instance (low) — fixed

Confirmed. The retry state now tracks mount status and drops failures reported after unmount; the cleanup keeps nulling timeoutId for the cleanup-without-unmount case.

Note on the test: my first attempt at it passed against the broken code. Unmounting right after onRangeChanged unmounts before the throttle's trailing edge has fired, so there was never a request in flight to reject. The real test needs a mutation whose rejection the test delivers by hand, after the unmount. It now fails without the guard.

3 — !enabled returns before the clear (low) — fixed in both hooks

Confirmed, and your test suggestion is in both suites: report ranges while disabled, enable, assert the resulting fetch covers only the current viewport. One detail worth recording — enabling does not by itself re-run the fetch effect (throttledFetchItems is stable and enabled is not a dependency), so the test has to flip enabled together with a fresh imageNames/itemIds identity, which is what production does: enabled is !isLoading, and the names arrive as loading ends.

4 — queue suite missing the mount-time no-loop test (low) — added

Added, matching the grid's. Correct that this hook only started depending on the EMPTY_ARRAY identity for that path in this PR.

Open questions

restoreRanges stability. Contract removed rather than documented — it is read through a ref now, so an unstable callback cannot churn onFetchFailure and the effect behind it. (Updated in a layout effect, so a restore firing before the passive flush still sees the intended closure.)

Is ~31s the intended ceiling? For the retry schedule, yes — that is the point at which hammering a backend that is trying to come up stops being useful. It is no longer the recovery ceiling, which is what your finding was really about: an outage of any length now heals on reconnect. I did not raise RETRY_MAX_ATTEMPTS, for the reason you gave — it does not remove the unbounded-outage case.

Two further defects found while verifying the above

I ran the fix through a fresh-context adversarial pass before pushing. It found two things, both now fixed in the third commit:

Restored ranges could be lost permanently — and part of this predates the fix you reviewed. 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, the effect never re-runs, and the ranges exist nowhere. Deterministic repro in both hooks — fail a range, scroll elsewhere 600–1000ms later, let the backend recover: the first range is never requested again. Grey rows until the user scrolls back, i.e. the same end state as your finding 3 from round 1, by a different route. 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 test that was supposed to guard this was pinned to a lucky delay. recovers a range that failed while the user was scrolling elsewhere used a single 1250ms scroll delay, which sits outside the losing window. It now sweeps 500/600/750/1000/1250ms and fails at three of the five without the fix.

Every fix here is mutation-verified: reverting the parking, the reconnect listener, the re-arm floor, the mount guard, the !enabled clear, the identity-guarded clear, or the delay sweep each turns at least one test red. Full suite green (2268 tests) and all five lint targets pass.

@Pfannkuchensack

Copy link
Copy Markdown
Member

Re-reviewed at 3e5149f50f. All four round-2 findings are genuinely fixed, and both defects your own pass found are fixed and pinned. I verified by mutation rather than by reading: nine mutations against the shipped code, eight die.

Mutation results, three suites (useBoundedRangeRetry, both hooks), baseline 53 passed:

Mutation Result
parking reverted (exhausted budget drops the ranges) killed - 2 failed
reconnect listener neutered killed - 2 failed
re-arm cooldown floor removed killed - 2 failed
unmount guard in onFetchFailure removed killed - 2 failed
identity-guarded clear -> absolute clear, gallery killed - 3 failed
identity-guarded clear -> absolute clear, queue killed - 3 failed
!enabled clear reverted, gallery killed - 1 failed
!enabled clear reverted, queue killed - 1 failed
resetRetryBudget no longer restores parked ranges SURVIVES - 53 passed

Full suite green here too: 171 files, 2271 tests, no type errors.

Findings

1. Low (coverage) - two of the three healing signals are unpinned

invokeai/frontend/web/src/common/hooks/useBoundedRangeRetry.ts, resetRetryBudget

The behaviour is correct - there is nothing to fix in the implementation. I wrote a probe for this path and it passes on your code. What is missing is the test.

You describe parked ranges as restored "on the next signal that the backend is answering: a socket reconnect, a later successful fetch, or a fresh range report." Only the reconnect signal is pinned. Deleting the restore block from resetRetryBudget - which is what serves the other two - leaves all 53 tests green.

resumes retrying after giving up when the user scrolls does not catch it because it re-reports the same range {0, 2}. The current viewport is re-fetched from lastRange whether or not parking was restored, so the assertion holds either way. The distinction only shows for a range that is parked but no longer on screen.

That case has a concrete failure mode if the line is ever removed. Probe, which passes on your code and fails with the restore deleted:

park {0,2} under sustained failure (35s, budget exhausted)
backend recovers WITHOUT a socket transition (transient proxy 502, socket never dropped)
user scrolls to {6,8}
-> expected a,b,c,g,h,i    actual g,h,i

AssertionError: expected [ 'g.png', 'h.png', 'i.png' ] to deeply equal [ 'a.png', 'b.png', 'c.png', ... ]. The rows the user scrolled past during the outage would stay grey placeholders permanently, because the socket never dropped and so the reconnect path - the only pinned one - never fires. Same end state as round-1 finding 3, reached by a third route.

Raising it at all only because this PR sets the bar itself ("every fix here is mutation-verified") and lists three healing signals as delivered. Today's damage is zero.

To expose this issue, add a test that parks a range under sustained failure, then after recovery reports a different range, and asserts both the parked and the new range are fetched. (I tried a success-signal variant for symmetry and could not make it discriminating on its own - lastRange is appended to every pass, so the viewport heals anyway. One test is enough.)

2. Low - a vitest cache artifact is committed at the repo root

node_modules/.vite/vitest/da39a3ee5e6b4b0d3255bfef95601890afd80709/results.json

In the PR's changed-file list, present in the head tree, not on main. It slipped through because the root .gitignore has no node_modules entry - only invokeai/frontend/web/.gitignore:10 does - so a repo-root node_modules/ is untracked but not ignored. Worth dropping from the PR, and adding node_modules/ to the root .gitignore so the next one does not slip through.

This is the only place where something in the PR is actually wrong.

Confirmed fixed

  • Permanent give-up. Parking plus the reconnect listener closes it. Your reasoning about the flapping socket is borne out rather than merely asserted: removing the 60s floor turns two tests red, so the per-outage-rather-than-per-reconnect budget is pinned.
  • Timer armed on an unmounted instance. The isMounted guard is pinned; removing it fails two tests. Your note about the first attempt passing against broken code matches what I found in round 2 - the rejection has to be delivered by hand after unmount, which the manual-failure mock now does.
  • !enabled returns before the clear. Fixed and pinned in both hooks independently.
  • Queue mount-time no-loop test. Present.
  • Both self-found defects. The identity-guarded clear is pinned in both hooks (3 failures each when reverted), so the batching loss is genuinely covered, not just fixed. The delay sweep is the right shape for it.
  • restoreRanges stability. Reading through a ref updated in a layout effect is a better answer than documenting the contract - it removes the round-2 open question instead of deferring it.

Residual

  • The retry hook now imports $isConnected from services/events/stores, so a generic common/hooks utility is coupled to the socket layer. It works and is documented; it does mean the hook is no longer reusable outside a socket-connected app. Not worth changing for this PR.
  • The 60s floor means a genuine recovery within 60s of a previous re-arm does not restore parked ranges on that reconnect. They stay parked rather than dropped, and takeAbandonedRanges() is correctly called after the cooldown check, so nothing is consumed when the cooldown blocks - a later success, scroll or reconnect still heals.
  • Unchanged from round 2: verification is code-path analysis plus the hook-level happy-dom harness, not a real browser. Nothing here needed a rendered component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.14.1 frontend PRs that change frontend files frontend-deps PRs that change frontend dependencies

Projects

Status: 6.14.1: Bug fixes to 6.14.0

Development

Successfully merging this pull request may close these issues.

2 participants