fix(multiuser): fix cross-user queue badge, progress bar, and gallery hijacking#9314
fix(multiuser): fix cross-user queue badge, progress bar, and gallery hijacking#9314lstein wants to merge 12 commits into
Conversation
In multiuser mode, queue item status changes and batch-enqueued events are
emitted only to the owning user's socket room (for privacy). The frontend badge
and progress bar only refetch the global queue status in response to those
events, so a non-admin only learns about queue changes from their *own* jobs.
Once their jobs finish they get no further signal: the global total (the "/Y"
in "X/Y") and the in_progress count freeze, leaving the badge wrong and the
progress bar animating indefinitely.
Broadcast a content-free `queue_counts_changed` event to the whole queue room
whenever counts change (a status transition or an enqueue). It carries only the
queue_id — no user_id, batch_id, session_id, or counts — so every subscriber can
refetch GET /queue/{id}/status, which already redacts per-user data. Nothing
private is leaked, and it never fires on high-frequency invocation progress
events.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In multiuser mode, admins are subscribed to the "admin" socket room and so receive invocation_complete events for every user. addImagesToGallery ran for all of them, so any image generated by another user would insert into the admin's gallery and auto-switch the admin's selected board to that user's board, displaying their image. This did not happen between two unprivileged users because they never receive each other's events. Gate addImagesToGallery on ownership: when an authenticated user is present and the event's user_id differs from theirs, skip the gallery insertion and auto-switch entirely. In single-user mode there is no authenticated user, so the original behavior is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The badge-livening change emits a redacted queue_counts_changed nudge to the whole queue room on status changes and batch enqueues. Update the two routing tests to check per-event: the private event still must not reach the queue_id room, but the data-free counts nudge legitimately does. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JPPhoto
left a comment
There was a problem hiding this comment.
I found one thing so far.
-
invokeai/invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:262The new cross-user guard only wraps
addImagesToGallery()at lines 48-61, but admin clients still process another user'sinvocation_completebefore that guard. The backend sends every invocation event to theadminroom ininvokeai/invokeai/app/api/sockets.py:304-316, andInvocationCompleteEventcarries the owneruser_id. When an admin receives another user's completion event, the handler still reads and upserts$nodeExecutionStatesat lines 265-280, clears canvas workflow integration processing at lines 283-284, and clears$lastProgressEventat line 289. This can mark the admin's local workflow nodes complete, append another user's outputs to matching node IDs, or stop the admin's canvas workflow integration spinner even though the event was not theirs. Common node IDs make this realistic, andgetUpdatedNodeExecutionStateOnInvocationComplete()creates state even when one did not exist.To expose this issue, add a frontend
vitestforbuildOnInvocationCompletethat setsauth.user.user_idto an admin user, seeds$nodeExecutionStatesor canvas integration processing state, invokes the handler withdata.user_idfor a different user, and asserts that node execution state, canvas processing state, gallery state, and$lastProgressEventare not mutated.
# Conflicts: # invokeai/app/api/sockets.py # tests/app/routers/test_multiuser_authorization.py
…hoto review) Admins receive every user's invocation events via the "admin" socket room. The cross-user guard only wrapped addImagesToGallery(), so another user's invocation_complete still upserted this client's $nodeExecutionStates, cleared the canvas workflow integration spinner, and reset $lastProgressEvent. Hoist the guard to the top of the returned handler so a foreign event is skipped before any side effect runs. Single-user mode (no authenticated user) still processes every event. Add a vitest covering the admin-receives-foreign, own-event, and single-user paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @JPPhoto — good catch, that was a real hole. The cross-user guard only wrapped Fixed in 2f7547a:
Also merged latest |
|
@lstein I found one issue:
|
…Photo review) The ownership check in onInvocationComplete ran too late: the workflow execution coordinator had already recorded the foreign event. And invocation_started/progress/error were never filtered at all, nor were the full queue_item_status_changed events that admins receive via the "admin" room (the sanitized redacted companion skips owner/admin sids). Move the check to the listener, before the coordinator sees the event: - invocation_started/progress/error/complete: drop foreign events outright, so they cannot create or overwrite node execution state, clear the canvas workflow integration spinner, replace $lastProgressEvent, or record completed-invocation keys. - queue_item_status_changed: treat a foreign full event like the sanitized companion — invalidate queue tags only, so the admin's queue list and badge still refresh, but no reconciliation, node-state reset, progress clear, or failure toast for another user's item. Single-user mode (no authenticated user) is unaffected. Regression tests drive the real listeners and real coordinator via setEventListeners; all seven fail without the guard and the two positive controls pass, confirming the gate doesn't over-block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @JPPhoto — you're right on every count, and the root cause was that I put the check in the wrong layer. Fixed in 529c0c2 by moving it to the socket listener, before the coordinator ever sees the event.
Single-user mode (no authenticated user) is unaffected. The guard inside Tests — you were right that the old test bypassed the coordinator. The new ones in That check was worth doing — my first cut of the |
Remove the queue_counts_changed listener and its hand-written type. The backend emitter was deleted when main's superseding sanitized-companion approach won the merge conflict in 6de25a0, leaving a listener for an event nobody emits. Nothing caught it: the type was hand-written rather than generated from the schema, so tsc was happy, and knip cannot see a runtime-dead socket.on registration. Its removal is behavior-neutral — main's sanitized batch_enqueued and queue_item_status_changed companions already invalidate a superset of the tags it did. But queue_items_retried had no companion, and retrying re-enqueues items — raising the queue's global total — while emitting no per-item status change. Other users' badge totals therefore went stale. Broadcast a sanitized queue_items_retried companion to the queue room, skipping owners and admins who already received the full event. It carries no item ids or user ids; the frontend handler needs it only for tag invalidation and already skips per-item invalidation when the list is empty, so no frontend change is required. _owner_and_admin_sids now accepts a collection, since a retry batch can span several owners and every one of them must be skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JPPhoto
left a comment
There was a problem hiding this comment.
It looks like there's a problem with an admin's events:
invokeai/app/api/sockets.py:328: An admin's owninvocation_completeevent is delivered twice because admin sockets belong to bothuser:{user_id}andadminrooms, and lines 331-340 emit separately to each room. The same file explicitly documents at lines 398-406 that separate emits duplicate delivery for sockets belonging to both rooms. AlthoughgetUpdatedNodeExecutionStateOnInvocationComplete()suppresses the second node-state update,invokeai/frontend/web/src/services/events/onInvocationComplete.tsx:274only logs the duplicate and continues intoclearCanvasWorkflowIntegrationProcessing(),addImagesToGallery(), and$lastProgressEvent.set(null)at lines 285-291.addImagesToGallery()then increments cached board totals andboard.image_countat lines 74-98 for both deliveries. This affects authenticated admins generating their own images and every single-user client, which is registered as bothuser:systemandadmin. The image-name insertion helper rejects the duplicate image name, but it does not prevent the independent board-count increments, leaving cached board counts inflated until a refetch. To expose this issue, add a test that routes an admin-ownedInvocationCompleteEventthrough_handle_queue_event, delivers every resulting socket payload through the frontend completion listener, and asserts that gallery insertion and board-count updates occur exactly once.
|
Closing in favor of #9358, which supersedes this PR. #9358 carries this PR's work forward: the sanitized One deliberate change from this PR's final state, agreed with @JPPhoto: a foreign Note: this PR's description predates the review rework and still describes the removed |
This PR fixes three related multiuser-mode issues that all stem from how socket events are (correctly) isolated per user, while frontend state was assuming it sees every relevant event.
1. Queue badge shows the wrong global total
Reproduction: User A queues 2 jobs; User B immediately queues 2 jobs. User A's badge shows
2/2(wrong — should be2/4); User B's shows2/4(correct).Root cause:
queue_item_status_changedandbatch_enqueuedevents are emitted only to the owning user's socket room (privacy — they carry unsanitizeduser_id/batch_id/session_id). The frontend badge only refetches the global queue status in response to those events, so a non-admin only learns about queue changes from their own jobs. Whoever queued first captured the global total before the other user's jobs existed, then stopped refetching.2. Progress bar animates indefinitely
Same root cause: once a user's own jobs finish they get no further signal, so the global
in_progresscount freezes at a non-zero value.ProgressBar.tsxreads that frozenin_progress > 0(withlastProgressEventnull) and staysisIndeterminateforever.Fix for 1 & 2
Broadcast a content-free
queue_counts_changedevent to the whole queue room whenever counts change (a status transition or an enqueue). It carries onlyqueue_id— nouser_id,batch_id,session_id, or counts — so every subscriber simply refetchesGET /queue/{id}/status, which already redacts per-user data. Nothing private is leaked. It never fires on high-frequency invocation-progress events.invokeai/app/api/sockets.py—_broadcast_queue_counts_changed()helper, called from the status-changed and batch-enqueued branches of_handle_queue_event.invokeai/frontend/web/src/services/events/types.ts— type the new event.invokeai/frontend/web/src/services/events/setEventListeners.tsx— handler invalidating onlySessionQueueStatus.3. Another user's image hijacks an admin's gallery
Reproduction: Admin (User A) and unprivileged User B logged in on separate browsers. Any image B renders switches A's selected board to B's board and displays B's image. Does not happen between two unprivileged users.
Root cause: Admins are in the
adminsocket room and receive invocation_complete events for every user.addImagesToGalleryran for all of them, inserting other users' images into the admin's gallery and auto-switching the admin's selected board.Fix for 3
Gate
addImagesToGalleryon ownership: when an authenticated user is present and the event'suser_iddiffers from theirs, skip the gallery insertion and auto-switch. In single-user mode there is no authenticated user, so the original behavior is preserved.invokeai/frontend/web/src/services/events/onInvocationComplete.tsxTesting
2/4, count down together, and clear with no lingering spinner.tsc --noEmit,eslint --max-warnings=0,prettier --checkall pass.ruff checkandruff format --checkpass.🤖 Generated with Claude Code