fix(ui): stop range-based fetching hooks from spinning in a render loop - #9439
fix(ui): stop range-based fetching hooks from spinning in a render loop#9439lstein wants to merge 15 commits into
Conversation
`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>
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>
PR #9439 — Stop range-based fetching hooks from spinning in a render loop
|
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>
|
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 What the findings actually warranted, and what changed — the retry is now bounded via a shared
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. On the mock stability point (bullet 5): the trigger's referential stability is RTK Query's documented contract (the trigger is Open question (bullet 7): |
Findings1. Medium - the bounded retry gives up permanently after ~31s and nothing re-arms it
A backend outage longer than the retry budget leaves grey tiles with no automatic recovery and no user affordance. Chain:
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 Blast radius is narrowed by the fact that an actively-generating user heals incidentally (a new image changes
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 2. Low - a backoff timer is armed on an unmounted instance and nothing clears it
The cleanup effect clears and nulls Measured directly: 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
|
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
|
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) — fixedConfirmed, including your point 5: I checked that RTK Query's structural sharing hands 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. 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
2 — timer armed on an unmounted instance (low) — fixedConfirmed. The retry state now tracks mount status and drops failures reported after unmount; the cleanup keeps nulling Note on the test: my first attempt at it passed against the broken code. Unmounting right after 3 —
|
|
Re-reviewed at Mutation results, three suites (
Full suite green here too: 171 files, 2271 tests, no type errors. Findings1. Low (coverage) - two of the three healing signals are unpinned
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
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:
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 - 2. Low - a vitest cache artifact is committed at the repo root
In the PR's changed-file list, present in the head tree, not on This is the only place where something in the PR is actually wrong. Confirmed fixed
Residual
|
Summary
Fix (performance).
useRangeBasedImageFetchinganduseRangeBasedQueueItemFetchingspin in a self-sustaining render loop for as long as the gallery grid / queue list is mounted.The shape of the bug, in
useRangeBasedImageFetching:setPendingRanges([])installs a fresh array, which is neverObject.is-equal to the previous one, so the effect re-runs.useThrottledCallbackresolves to{maxWait: 500, leading: true, trailing: true}, so the re-entry schedules a trailing invocation, which callsfetchItems, 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.onQueryStartedupserts only the DTOs the server actually returned: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
getImageDTOcache entry,selectCachedArgsForQuerynever reports it, and it is re-requested on every pass — twice a second, forever.The change: clear with the shared stable
EMPTY_ARRAYreference fromapp/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 throughonRangeChanged, which setslastRangeto 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.
ImageAtPositionandQueueItemAtPositionboth consume the cache with the documented "subscribe once it has data" hack:so a row whose DTO never arrived does not fetch for itself,
onQueryStartedswallows the failure incatch {}, 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'sstructuralSharingpreserves theimageNamesreference 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:
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_namesrepeats every ~500ms indefinitely in the Network tab. After: it fires once per range change.Regression checks:
imageNames).Notes for reviewers
Two things worth knowing, both found by adversarially reviewing this diff:
EMPTY_ARRAYinapp/store/constants.tsisnever[]— mutable, unfrozen, and now referenced from ~30 files and held as component state by these hooks. Nothing mutates it today (audited), but a futurependingRanges.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 Reactact+ fake timers in ahappy-domenvironment (scoped per-file via a@vitest-environmentdocblock —happy-domis 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: falsefetches 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 forcoalesceRanges, including the duplicate-per-cycle growth case. Mutation-verified: reverting theEMPTY_ARRAYclears, restoring the queue hook's early return, droppingonRangeChanged'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:knipall clean.Merge Plan
Ordinary merge. No redux slice changes, so no migration.
Checklist
What's Newcopy (if doing a release after this PR) — n/a