fix(ui): make multi-GPU viewer previews survive owner termination and queue lifecycle events - #9389
Conversation
ee4b44b to
34a1d94
Compare
|
Rebased cleanly onto the current #9263 head ( |
34a1d94 to
6c6fb0d
Compare
… queue lifecycle events Two related lifecycle gaps in the image viewer's multi-session preview state (ImageViewer/context.tsx): 1. Terminal-owner fallback: when the session owning the shared $progressEvent/$progressImage globals reached a terminal state, its tile was removed but the globals were cleared (or parked on the finished session's stale frame via the resolve illusion). Since the tiled view only renders with >1 active session, the remaining session's still-running preview disappeared until its next image event. The globals are now handed to the most recently updated remaining session immediately, for every terminal status. 2. Stale lifecycle: $progressData was cleaned only by per-item terminal events. It is now also cleared on queue_cleared (scoped like workflowExecutionCoordinator.onQueueCleared, and marking the cleared items finished so a trailing progress event from a worker stopped only by the clear cannot repopulate the preview), on socket disconnect, and on $socket replacement (auth-token/user change). The store logic is extracted into viewerProgressLifecycle.ts so it can be unit tested without rendering; the provider keeps the socket subscriptions and ownership/scope checks. 16 new vitest cases cover promotion across terminal statuses and auto-switch modes, non-owner termination, clear scoping (own/unscoped/foreign/sanitized), and disconnect resets. Follow-up to PR invoke-ai#9263 (JPPhoto review, 2026-07-25). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6c6fb0d to
4cbbb9d
Compare
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.ts:77: Queue clear marks only image-tracked items plus current global event. A no-image session can later emit an image and resurrect deleted preview. Test: record item 1 without image, item 2 with image, clear queue, then record item 1 with image; expect rejection.
Other findings/issues to fix:
invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.ts:144: A final-image callback has no session identity. If A promotes B, then B completes before A's image loads, A's callback clears B's retained preview. Test: complete A, complete B, invoke A's delayed load; expect B preview retained.
Suggestions:
-
Consider tracking every session on progress, including
image: null, before queue-clear suppression. -
Consider passing completed item identity through
onLoadImage; ignore stale loads from previous sessions.
Addresses JPPhoto's review of invoke-ai#9389. A queue clear marked only sessions that had produced a preview image (plus the owner of the shared globals) as finished. A session that had reported progress without an image yet was left unmarked, so its first image event after the clear resurrected the preview the clear had just dropped. Every item seen on progress is now tracked, image or not, and marked finished by the clear. The final-image load callback carried no session identity, so a late load from an already-completed session cleared whatever preview was retained at the time — including a *different* session's pending resolve illusion (A completes and hands the preview to B, B completes and starts its own illusion, then A's image finally loads). The callback now takes the loaded DTO's session_id and ignores loads it can attribute to another tracked session; unattributable loads (uploads, pre-mount images) still clear, so a retained preview cannot cover the viewer indefinitely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the previous commit, from an adversarial review of it. Ignoring a mismatched final-image load removed the only thing that ever took a retained preview down. The retained session's own image may never load: the viewer's <Image> reports load failures through onError, not onLoad, and with concurrent completions auto-switch can settle on another session's image, so the retained one is never rendered at all. In those cases the preview — an opaque overlay — covered the viewer until the next generation. The illusion now carries a timeout armed alongside it, so an ignored load can only end it early, never keep it up. Also from the same review: - A queue clear cancels the items already running before deleting the rows, so a worker that claims an item in between gets no terminal event and its first progress event lands after the clear. Only the in_progress claim event names that item, so the viewer now tracks it. - Session attributions outlived the state they described: a disconnect or socket swap dropped the previews but kept the session ids, so images from before the reset were still read as another session's and refused to end a later illusion. They are dropped with everything else. - A completed item that never reported progress armed an illusion with nothing retained, leaving the resolving flag on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of the three disarm cases could not fail: after a load or a reset the stores are already null, so a leaked timer's clear was indistinguishable from no timer at all. Assert the timer count instead. Also correct the setPendingResolve docstring, which claimed more than the code does: a progress event carrying no image cancels the illusion without replacing the retained frame, so that frame is bounded by the next preview image rather than by this timeout — as it is today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks @JPPhoto — both findings confirmed against the code and fixed, plus a few things that fell out of adversarially reviewing the fixes themselves. Head is now 1. Queue clear only marked image-tracked items (merge blocker)Confirmed. The lifecycle now tracks Your test is in as written ( While tracing this I checked the ordering that makes the hole reachable, and it's narrower than the fix's first framing suggested. Its claim is the only event that names it, so the viewer now also tracks 2. Final-image callback has no session identityConfirmed, with the sequence you described: A completes and hands the preview to B, B completes and starts its own resolve illusion, then A's image finally loads and clears B's retained frame.
Both call sites wrap the callback rather than passing it through — One thing worth your attention here. Ignoring loads by identity removes the only thing that ever took a retained preview down, and the retained session's own image is not guaranteed to load at all:
In all three the preview is an opaque 3 s is a judgment call — long enough that a normal DTO-fetch-plus-full-res-load wins the race, short enough that a stale frame over a deliberately clicked image is brief. Easy to retune if you'd rather bias one way. Two smaller things from the same pass: the session→item attributions are now dropped in Known residuals (pre-existing, untouched)A progress event carrying no image replaces Tests27 lifecycle cases (1816 frontend tests total), tsc/eslint/prettier clean. Each new test was checked by reverting its fix in place and confirming it fails — including two timeout cases that initially passed against a leaked timer because the stores were already null at that point; they now assert the timer count instead. |
JPPhoto
left a comment
There was a problem hiding this comment.
No merge blockers!
I did find a few corner cases - your call as to whether they get addressed:
-
invokeai/frontend/web/src/features/gallery/components/ImageViewer/viewerProgressLifecycle.ts:150-160: anin_progressevent arriving afterqueue_clearedre-registers a deleted item; later progress resurrects its preview. Test: deliver clear, then claim, then progress; expect progress rejected. -
invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx:127: sessions with no progress never enteritemIdBySessionId; their final-image load is treated as unknown and clears another session's retained preview. Test: A retains preview, B completes without progress, B image loads; expect A preview retained.
Suggestions:
- Consider backend clear-generation/tombstone data to reject post-clear claims.
- Consider recording
session_idfrom claim/terminal events.
Summary
Follow-up to #9263, addressing the two ImageViewer preview issues deferred in @JPPhoto's 2026-07-25 review. They share the same store and handler, so they are fixed together:
1. Terminal-owner fallback (
context.tsx): when the session owning the shared$progressEvent/$progressImageglobals reached a terminal state, its tile was removed but the globals were cleared — or, for successful completion with auto-switch, parked on the finished session's stale frame via the resolve illusion. Since the tiled view only renders with more than one active session, the remaining session's still-running preview disappeared until its next image event. The globals are now handed to the most recently updated remaining session immediately, for every terminal status and auto-switch mode.2. Stale progress lifecycle:
$progressDatawas cleaned only by per-item terminal events. It is now also cleared on:queue_cleared— scoped exactly likeworkflowExecutionCoordinator.onQueueCleared(unscoped and own-user clears apply; foreign scoped clears and the sanitizeduser_id="redacted"broadcast do not), and the cleared items are marked finished so a trailinginvocation_progressevent from a worker stopped only by the clear cannot repopulate the preview;setEventListeners);$socketreplacement after an auth-token/user change.Implementation
The store logic is extracted into
viewerProgressLifecycle.tsso it can be unit tested without rendering (per the web CLAUDE.md convention of no UI tests); the provider keeps the socket subscriptions and ownership/scope checks. Sessions gain aseqcounter so "most recently updated" promotion is exact rather than timestamp-granular.Tests
16 vitest cases: preview promotion for completed/canceled/failed and both auto-switch modes, multi-survivor promotion order, non-owner termination leaving the preview alone, last-session clear/resolve behavior, repeat-event idempotence, clear scoping (own/unscoped/foreign/sanitized), trailing-progress suppression after a clear, and disconnect reset.
Merge order
Stacked on #9263 — this branch contains the multi-GPU branch's commits. Draft until #9263 merges; will then rebase onto main and mark ready for review.
Checklist
What's Newcopy (if doing a release after this PR)🤖 Generated with Claude Code