fix(auth): revoke privileges immediately on role change, deactivation, or deletion - #9360
Conversation
960d607 to
efae7b4
Compare
7ff3da0 to
90e1f03
Compare
90e1f03 to
0c95caa
Compare
…, or deletion Three related gaps let stale credentials outlive database changes in multiuser mode: 1. JWT privileges survived demotion. Authentication verified the user exists and is active but kept trusting the token's is_admin claim, and the sliding-window middleware re-minted new tokens (and the media cookie) from those stale claims — so a demoted administrator kept admin rights indefinitely as long as they kept making requests. All auth dependencies now derive authorization fields from the database record on every request (the token proves identity only), and the middleware refuses to refresh for missing/inactive users and mints refreshed tokens from the database record. A promoted user symmetrically gains admin rights on their next request without re-login. 2. Open sockets retained revoked privileges. Socket room membership was established once at connect from the token's claims. Connect now derives is_admin from the database, and a new server-internal user_access_changed event (emitted by the user-management routes) re-authorizes live sockets: demotion leaves the admin room, promotion joins it, deactivation/deletion disconnects the user's sockets. 3. Deactivated users' queued work kept executing. The session processor now rejects (cancels) dequeued items whose owner is inactive, stops running sessions at the next node boundary, and cancels the current item immediately when its owner is deactivated (which also stops step-callback nodes mid-node via the existing cancel-event machinery). Invocation-context media reads and saves also require an active account. Single-user mode and the "system" user are exempt. Tests cover: demoted-admin 403 with no token/cookie refresh, DB-derived refresh claims, promoted-user semantics, socket reconnect-with-old-token, live socket demotion/promotion/deactivation, dequeue rejection, multi-node mid-session deactivation, and invocation-context read/save denial for inactive accounts, plus positive cases for unchanged admins, active users, and single-user mode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dmin test stubs Two CI failures introduced by the privilege-revocation commit: - `UserAccessChangedEvent` is dispatched only between server components, but `EventBase.get_events()` sweeps in every subclass carrying `__event_name__`, and its sole consumer is the OpenAPI generator. The event therefore leaked into `openapi.json`/`schema.ts`, failing openapi-checks and typegen-checks and contradicting the event's own documented contract. Events can now opt out with `__server_internal__ = True`. - The pre-existing "rejects non-admin users" tests in `test_app_info.py` stubbed the user lookup with a bare `Mock(is_active=True)`. Authorization is now derived from the database record on every request, so `TokenData` validation rejected the Mock-valued fields. The stub now carries concrete values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A JWT is self-contained: nothing in the database can make an already-issued token stop verifying. Authorization fields are re-derived from the user record on every request, so demotion and deactivation take effect immediately — but a password change had no way to invalidate anything. A stolen token therefore outlived the password rotation meant to evict the thief, and because the sliding window renews on every mutating request, it survived indefinitely rather than expiring after a day. Adds `users.token_epoch`, stamped into every minted token and compared against the record on each authenticated request. Rotating the password bumps it in SQL (`token_epoch = token_epoch + 1`, so concurrent bumps can't lose each other), invalidating every token issued before the change. This is the general "revoke everything issued so far" primitive the auth layer was missing. Details worth noting: - The sliding-window middleware also checks the epoch. It runs after the route and refreshes on any 2xx mutation, so an unauthenticated route carrying a stale bearer header would otherwise launder a revoked token into a valid one. - Changing your own password signs out your *other* sessions, not the tab you did it from: the route mints a replacement and returns it in X-Refreshed-Token. The middleware can't do this — it correctly refuses to refresh a stale-epoch token, and leaves an already-set header alone. - Rejection is reported as an ordinary invalid/expired token, so a stolen token's holder isn't told the password was just rotated. - Existing rows and existing tokens both start at 0, so upgrading logs nobody out; only a real bump revokes. Tests cover cross-session revocation, the calling session surviving, admin password reset revoking the target, the refresh-laundering path, non-password updates not revoking, and pre-existing tokens staying valid.
…ecks `queue_owner_is_active` exempted `user_id == "system"` on the stated grounds that the system user "has no database record". That premise is wrong: migration_27 creates a real, active `system` row that owns every board, image, and workflow carried over from before multiuser support. Because the row exists and is active, the exemption changed nothing in normal operation — it only took effect when the row was missing or inactive, which is exactly the case where this gate then disagreed with the save gates in `invocation_context`, which have no such exemption. A system-owned item would pass the gate that decides whether to spend GPU time, load models and denoise, then fail at the first `context.images.save()`. That is the worst possible ordering for two checks that disagree. Drops the exemption so all three checks agree: the system user now passes on its own merits, and if its row is ever gone the item is rejected at dequeue instead of after generating. Also protects the row, since orphaning it is what made the disagreement reachable in the first place. Neither `delete_user` nor `update_user` guarded it — `list_users` merely hides it from the UI, and the last-admin guard does not apply because the system row is deliberately not an admin. Deleting or deactivating it is now rejected; it is not a login account, so there is no legitimate reason to do either.
The system row owns everything carried over from before multiuser support. It is seeded unable to authenticate, but an administrator could once give it a password through `PATCH /auth/users/system`. Refusing that from now on repairs nothing that already happened, and the hole has three separate ends: - the row itself, which may still carry a usable hash under a fixed, public email — the migration now clears `password_hash` alongside `is_admin`; - a row damaged *after* the migration, by direct SQL or on a database that applied an earlier revision of the same migration id, since migrations run once — `UserService.authenticate` now refuses the account outright, whatever the row holds; - a token *already issued*, which the migration cannot reach at all. The row is deliberately left active and its epoch untouched, so nothing else rejected it and sliding-window refresh would renew it forever — `resolve_authorized_user` now refuses the id, which covers REST, media, sockets and the video-upload gate in one place. Single-user mode, where everything legitimately runs as `system`, never reaches `resolve_authorized_user`: its dependencies synthesize the TokenData and return first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t be read `queue_owner_is_active` treated any lookup exception as "active", which makes unknown database state executable: the account may have been deactivated a moment earlier, and this gate is what stands between that and GPU time spent on its behalf. It now retries the read before refusing, so a transient error — a busy-timeout on the shared SQLite connection under multi-GPU write contention, say — does not cost a valid user their queued work. Only a database that is unreadable across every attempt refuses the item, and that costs a cancellation, which is retryable. Both call sites run on a worker thread, so the wait between attempts blocks nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`invoke-usermod` and `invoke-userdel` write to the database from their own process, so no in-process event can be raised for them. Everything the server derives per request — REST authorization, the dequeue and between-node gates, the media read and save gates — already changes the instant those commands commit. Socket room membership does not: it is established at connect time and refreshed only by `user_access_changed`, so a demoted administrator's socket sat in the admin room, receiving other users' private events, until it happened to reconnect. A periodic sweep now re-derives each connected user from the database and publishes any difference as the same event the routes emit, so sockets re-authorize and the session processor cancels the user's running items through one code path rather than a second copy that drifts from the first. Staleness is judged against every socket of the user, not a representative one — a session that reconnected after a password change holds the current epoch while the superseded session is still connected under the old one. `_handle_user_access_changed` now re-reads the record and applies that, treating the event as a trigger, the same way `_on_user_access_changed` already does. Handlers are dispatched as independent tasks and the sweep's payload is a snapshot taken before an await, so an event can arrive already superseded: applying it would re-grant the admin room to someone just demoted, or disconnect the replacement session a password change had just issued. A read that fails leaves the event standing. The sweep is started and stopped from the app lifespan, in a `finally` so an abnormal exit cannot leave it running against a half-torn-down process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Session Management section described tokens as expiry-only, which invites the operator assumption that a demotion or deactivation does not take effect until the target's token runs out. Replace it with what actually happens: role changes derived from the database per request, epoch invalidation on password change, socket disconnection, queued-work cancellation, and the bounded staleness of a change made with the CLIs. Both limits are stated plainly too — the token stays cryptographically valid until it expires, and a session in flight is stopped, not rewound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — all three blockers were real. Fixed at 8a3d38d, along with the doc gap. 1. The
|
…d_user The middleware carried its own copy of the exists/active/epoch checks, which is exactly how a rule added later reaches every entry point but this one: the refusal of the internal `system` id landed in `resolve_authorized_user`, so REST, media, the socket handshake and the video-upload gate all stopped honoring those tokens while this kept minting fresh ones for them. The `system` row is deliberately active with an untouched epoch, so none of the local checks fired. Nothing accepted the renewed token, so no access followed from it — but an indefinitely renewed session is a hole waiting for one consumer that trusts a token without re-checking the id. Deciding in one place is the point of that function. The lookup still runs off the event loop, for the reason the old comment gave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up at 49d00b3, on my own point (1) above. I said No access followed from it — nothing accepts the renewed token — but that is precisely the shape the function's own docstring warns about ("a check added to some copies but not others is indistinguishable from no check at all on the paths that were missed"), and an indefinitely renewed session is a hole waiting for one consumer that trusts a token without re-checking the id. The middleware now decides through |
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
-
invokeai/app/api/sockets.py:312-317,339-370preserves cached privileges when DB reads fail. After CLI demotion, a stale admin socket can keep receiving private events.Test:commit demotion, forceusers.getto fail across sweeps, emit another user's event, assert the socket receives nothing. -
invokeai/app/api/sockets.py:393-418,441-447applies stale event authorization when revalidation fails. A superseded promotion event can re-add a demoted user toadmin.Test:send promotion event after demotion, make the reread raise, assertenter_room(..., "admin")is never called.
Other findings/issues:
-
invokeai/app/api/sockets.py:319-324andinvokeai/app/services/session_processor/session_processor_default.py:163-178leave a no-socket user's already-running single-node graph executing after CLI deletion; only save gates stop persistence. Docs atdocs/src/content/docs/features/Multi-User Mode/admin-guide.mdx:298-303overstate cancellation.Test:run a side-effecting one-node item, delete the owner with no socket, assert the node is not invoked. -
invokeai/app/services/users/users_default.py:100-133omitstoken_epochfromget_many(), so users whose passwords were changed receive DTOs reporting epoch0.Test:bump an account epoch, callget_many([user_id]), assert the returned epoch matchesget().
Suggestions:
-
Instead of preserving socket authorization on repeated DB errors, disconnect or remove privileged room membership after bounded failures; this fails closed and limits stale access.
-
Instead of sweeping only users with sockets, track authorization changes in the database or periodically revalidate every running queue owner; this closes the no-socket single-node gap.
-
Try using one shared row mapper/projection instead of manually maintaining each user
SELECT; this prevents future fields such astoken_epochfrom silently becoming stale.
Converting the routes from `async def` to `def` removed an implicit guarantee: a handler whose body contains no `await` could not be interleaved with another request, because the event loop had no point at which to switch. Two handlers relied on it. `POST /auth/setup` did has_admin() then create_admin() in separate transactions. Two concurrent requests both saw no admin and both created one, so the loser ended up with a persistent admin account instead of the intended 400. The condition now lives inside the INSERT's own transaction, behind BEGIN IMMEDIATE so a second process (invoke-useradd --admin) cannot slip a write in either. This mirrors what invoke-ai#9360 does for the update/delete last-admin invariant; create_admin was the one path it does not cover. Custom node install, uninstall and reload all mutate the same custom-nodes directory, sys.modules and invocation registry. Interleaved, a failed install's cleanup rmtree'd the directory a concurrent install had just cloned into. A module-level lock restores the exclusion; the install and uninstall bodies moved into helpers so the lock scope is visible rather than an 80-line reindent. Both regression tests fail without their fix: the admin one creates two administrators, the pack one loses the successful install's directory. perf(queue): render the queue list from summaries, with one sanitizer The list fetched full queue items for every visible row, each carrying its session graph and workflow — megabytes per screenful for fields no row draws. The rows now render from SessionQueueItemSummary and the full item is fetched only when a row is expanded, which is what the summary route added in this branch was for; until now nothing consumed it. The per-item summary query provides the same cache tags as getQueueItem, so every existing invalidation path covers the list rows with nothing to wire up, and the optimistic status write is mirrored so a row's status still flips without a round trip. The range hook batches at the backend's 1000-id limit, which a fast fling could otherwise exceed. Both sanitizers are now one generic function over a single redaction table: the summary and the full item are two projections of one row, and a field stripped from the list but left on the detail view is leaked anyway. A test walks the intersection of both models and asserts they redact it identically. `device` is deliberately not redacted in either. It names the instance's GPU rather than anything about the other user's work, and the list has always shown it — redacting it here would have quietly changed what non-admins see. parent_item_id joins the summary because the rows decide from it whether to offer a retry.
Two review follow-ups landed together here. Converting the routes from `async def` to `def` removed an implicit guarantee: a handler whose body contains no `await` could not be interleaved with another request, because the event loop had no point at which to switch. `POST /auth/setup` did has_admin() then create_admin() in separate transactions. Two concurrent requests both saw no admin and both created one, so the loser ended up with a persistent admin account instead of the intended 400. The condition now lives inside the INSERT's own transaction, behind BEGIN IMMEDIATE so a second process (invoke-useradd --admin) cannot slip a write in either. This mirrors what invoke-ai#9360 does for the update/delete last-admin invariant; create_admin is the one path it does not cover. Custom node install, uninstall and reload all mutate the same custom-nodes directory, sys.modules and invocation registry. Interleaved, a failed install's cleanup rmtree'd the directory a concurrent install had just cloned into. A module-level lock restores the exclusion; the install and uninstall bodies moved into helpers so the lock scope is visible rather than an 80-line reindent. Both regression tests fail without their fix: the admin one creates two administrators, the pack one loses the successful install's directory. The route added earlier in this branch had none — the list still fetched full queue items for every visible row, each carrying its session graph and workflow, so the claimed saving was not being realised. The rows now render from SessionQueueItemSummary and the full item is fetched only when a row is expanded. Measured against the previous commit, same backend and same 396-item queue, identical scroll (page load, queue tab, scroll to 60%): requests 62 -> 2 payload (gzip) 262 KB -> 1.3 KB (30 items) server time 60ms -> 4ms (30 items) The request count collapses because the old path was self-amplifying: the range hook re-asks which ids are uncached on every range event, and at ~60ms per response the cache had not filled yet, so overlapping fetches piled up. A side effect worth knowing: `items_by_ids` silently skips items it cannot deserialize, so a queue item whose graph references an unregistered node type left its row permanently blank. Summaries never touch the graph, so the row now renders and only the expanded detail is affected. The per-item summary query provides the same cache tags as getQueueItem, so every existing invalidation path covers the list rows with nothing to wire up; the optimistic status write is mirrored so a row still flips without a round trip. The range hook batches at the backend's 1000-id limit, which a fast fling could otherwise exceed. The summary and the full item are two projections of one row, and a field stripped from the list but left on the detail view is leaked anyway. Both now go through one generic function over a single redaction table; a test walks the intersection of the two models and asserts they redact it identically. `device` is deliberately not redacted in either. It names the instance's GPU rather than anything about the other user's work, and the list has always shown it — redacting it would have quietly changed what non-admins see. parent_item_id joins the summary because the rows decide from it whether to offer a retry.
Six follow-ups from @lstein's sweep that were left open. `require_admin` and `require_admin_or_default` go back to `async def`. They only read `is_admin` off already-resolved token data, so declaring them `def` bought a threadpool round-trip per admin request and nothing else. The `users.get` that can block lives in `get_current_user`, which stays synchronous — the docstrings now say why the two layers differ. The AST guard now inspects what it claims to. It walked only `tree.body`, so a handler registered from inside a factory function or an `if` block was never seen, and `_awaits_something` used `ast.walk`, which counts `await`s inside nested closures — a handler could have passed by defining an inner async helper it never awaits. Both are fixed and both now have their own tests, so the guard's behaviour is pinned rather than asserted in a comment. `convert_model` takes a lock non-blocking and answers 409 otherwise. Blocking would be wrong: a conversion runs for minutes, and for the same key the second caller reads a record the first is midway through replacing. Two conversions in flight also means two models resident at once, which nothing bounds. The body moved into `_convert_model` so the lock scope is visible. Tested for the 409 and for the lock surviving a failed conversion rather than wedging the endpoint for the process's lifetime. `do_hf_login` and `reset_hf_token` hold a lock across the write and the status read-back, which otherwise could report a status belonging to a different token than the one just written. The blocking-work doc gains the bound it was missing: anyio's thread limiter holds 40 tokens, so past forty concurrent blocking requests the stall moves rather than vanishes — and anything else needing a thread queues behind them, including the synchronous auth dependency that runs before a handler is reached. Noted there too that `test_event_loop_blocking.py` cannot show this, because its probe route has neither auth nor database access. `QueueItemDetail` tells a failed fetch apart from a pending one. A queue item the backend cannot serve — one whose graph references a node type this build no longer registers — previously read as "Loading" forever. Left alone deliberately: the stale `old_is_public` in the workflow-updated event, which is cosmetic and would need `workflow_records.update()` to return the previous row to fix properly; and the `delete_user` / `update_user` last-admin invariants, which belong to invoke-ai#9360.
Both of the second review round's blockers are about the same seam: what the socket layer does when the database read it re-derives authorization from fails. Round 1 made both paths re-read the record; neither said what to do when the re-read itself does not answer. `_handle_user_access_changed` fell back to the event payload. That payload can be a superseded promotion — the sweep snapshots before an await, so a demotion can commit while the event is in flight — and applying it re-granted the admin room to a user who had just lost it, with no record consulted to catch it. The event still stands in the direction that takes privileges away, since it is evidence of a committed change the re-read could not contradict, but `is_admin` is now forced False on that path: this handler can demote on an unreadable database and never promote. A genuine promotion is deferred, not lost — the sweep sees the record disagree with the cache and publishes it again. `_revalidate_socket_users` retried forever, which makes an unreadable database indistinguishable from a quiet one: a CLI demotion during an outage left the socket in the admin room, reading every other user's private events, for as long as the reads kept failing. It now retries a bounded number of times and then drops the privilege rather than the connection. Disconnecting would be the outage the sweep exists to avoid — `_handle_connect` fails closed, so the clients could not get back in — whereas a demoted socket keeps working and recovers through the ordinary staleness check once reads succeed. The counter that bounds this is the new piece of state, and the two orderings that get it wrong are both handled. The sweep's lookup awaits, so a user's last socket can close inside that window; the disconnect clears their count and a naive write would resurrect it for a user with no sockets, where nothing would collect it again because the loop skips sweeps entirely while none are open. And the count answers "how long have these sockets gone unchecked", so every successful read of the record clears it, not just the sweep's own — otherwise a socket admitted by a read that proved the user is an admin would be demoted by the next single failure instead of by three. The bound covers is_admin only. Deactivation, deletion and epoch revocations still wait for a successful read, because acting on those means disconnecting; that limit is now stated in the docstring and the admin guide rather than implied away. One existing test was pinning the bug: TestUserAccessChangedHandler never bound ApiDependencies, so every test in it ran the handler's exception path, and the promotion test passed only because of the fallback removed here. Same hazard in the epoch test. Both now bind the record they claim to be testing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both blockers are fixed at 592fee4. You were pointing at the same seam twice: round 1 made both paths re-read the record, and neither said what to do when the re-read itself doesn't answer.
Your test passes —
So it now retries Your test passes — after committed demotion with Scope, stated rather than implied. The bound covers On the other two findings
The no-socket single-node gap I'm leaving documented for now; DB-tracked auth changes or revalidating every running queue owner is a bigger change than this branch should carry. One thing worth flagging, because it cuts against my own round-1 work. I ran a fresh-context adversarial pass over the fix before pushing, and it found that The same pass found two defects in the new failure counter, both fixed here: the sweep's lookup awaits, so a user's last socket can disconnect inside it and the failure write would resurrect a counter for a user with zero sockets that nothing ever collects; and the counter has to be cleared by every successful read of the record, not just the sweep's own, or a socket admitted as an admin by a read that succeeded gets demoted by the next single failure instead of by three. New/changed tests are in |
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
invokeai/app/api/sockets.py:368-373leaves socketless queue items unchecked.invokeai/app/services/session_processor/session_processor_default.py:163-178checks owner only between nodes. CLI deletion emits no server event (invokeai/app/util/user_management.py:24-29). A deleted user’s running single-node graph can finish. Docs overpromise atdocs/src/content/docs/features/Multi-User Mode/admin-guide.mdx:327. Test: run socketless one-node job; delete owner via CLI during node; verify cancellation before node completion.
Other findings/issues:
-
invokeai/app/api/sockets.py:503-507sets cachedis_admin=Falsebeforeleave_room(). If room removal fails, later successful reads see no cache difference, so admin-room membership can persist while direct sends trust cache. Test: makeleave_room()fail at retry limit; keep DB role demoted; run recovery sweep; verify room removal retries. -
invokeai/app/api/sockets.py:547-586only forcesis_admin=Falseafter failed reread. Staleis_activeandtoken_epochremain trusted, so stale epoch-0 event disconnects replacement epoch-1 socket. Contradicts replacement-session behavior documented atdocs/src/content/docs/features/Multi-User Mode/admin-guide.mdx:328. Test: cache socket epoch 1; send active event epoch 0; makeusers.get()fail; verify socket remains connected. -
invokeai/app/services/users/users_default.py:100-133get_many()omitstoken_epoch; returnedUserDTOsilently defaults to 0 after password rotation. Test: bump epoch, compareget_many([id])[id].token_epochwithget(id).token_epoch.
Alternative implementation ideas:
-
Instead of tying revocation propagation to open sockets, persist a durable access-change version or recheck owners before and after every node; this closes socketless single-node gaps.
-
Instead of marking cached privilege false before room removal, reconcile desired room state and retry failed
leave_room()calls; this prevents cache/room divergence. -
Consider deferring active/epoch disconnects until successful reread, while retaining admin downgrade; this prevents stale event snapshots from killing replacement sessions.
-
Instead of separate SQL projections, share one user-row mapper and select list; this prevents future fields like
token_epochbecoming stale in bulk reads.
…ing an unreadable record Addresses the round-3 review. Blocker: an out-of-process deletion of a user with no socket open raised no event at all, so a graph of theirs already inside a node ran on. The revalidation sweep now covers the owner of every executing queue item as well as every connected user. Publishing for them routes the case through `DefaultSessionProcessor._on_user_access_changed`, which cancels the item and so sets the worker's cancel event, stopping a node with step callbacks part-way. A graph whose last node finishes before the sweep notices is caught by one more owner check after that node, so it is canceled rather than recorded as completed. That post-node check is deliberately narrow. Before a node, failing closed on an unreadable record costs a retryable cancellation and may save a GPU; after one there is no execution left to refuse, only a finished result to destroy — and for a workflow-call child, its whole parent chain. So it runs only for a session that completed its own work (an errored session is `is_complete()` too, and a suspended workflow call is not), and it treats an unreadable record as active. A genuinely revoked owner keeps nothing by that: the `invocation_context` save gates re-read the record and already refused every write. `worker.queue_item` was only ever overwritten by the next dequeue, and pausing blocks before that — a worker that finished an item and parked would have gone on reporting its owner as running work, making the sweep republish forever. It is now cleared at the top of the worker loop. `_handle_user_access_changed` no longer disconnects on a record it could not read. The payload cannot be trusted to close a connection for the same reason it cannot be trusted to grant the admin room: it may already be superseded, and a stale `is_active` or `token_epoch` would close the replacement session a password change had just issued, with no way back in while `_handle_connect` is also failing closed. The admin downgrade still applies; everything else waits for a read that succeeds, which the sweep republishes. Room membership and the cached `is_admin` can no longer diverge. The cache is written before the room call and rolled back if it raises, so a change that did not happen is never recorded as done — which matters because the sweep decides there is nothing left to do by comparing the record against that cache, and `_handle_sub_queue` re-derives room membership from it. A failed `disconnect` no longer escapes the handler either: it used to abandon the user's remaining sockets and, since every handler for an event shares one task, skip the queue cancellation entirely. `UserService.get_many` omitted `token_epoch`, so its DTOs reported the model default of 0 — a revoked epoch indistinguishable from a fresh one. All five queries that build a `UserDTO` now share one column list and one row mapper, so the next field cannot go stale the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — all four confirmed and fixed at Blocker: socket-less queue itemsConfirmed, and you were right that documenting it was not good enough. A node that finishes inside the sweep interval needed the other half, so there is now one more owner check after the graph's last node. But my first version of that check was worse than the gap it closed, and I want to be explicit about it, because it bears on where fail-closed belongs:
So the post-node check is now scoped to a session that finished its own work cleanly — errored sessions and suspended workflow calls are excluded — and it passes Two related defects came out of the same pass:
|
JPPhoto
left a comment
There was a problem hiding this comment.
This is good to merge.
There are a few corner cases, unlikely to appear during normal usage and perhaps not worth dealing with:
-
invokeai/app/api/sockets.py:489-496: socketless running owners have revalidation failures discarded, so repeated DB-read failures never emit cancellation. A deleted user’s long-running single-node job can run to completion.Test:use onlyrunning_owners={"u"}, makeusers.getraise for three sweeps, assert no access-change event or cancellation occurs. -
invokeai/app/services/session_processor/session_processor_default.py:114-119, 231-232: the final owner check treats an unreadable record as active. If revocation occurs during a one-node, no-save node, the queue item can still be recorded completed.Test:return active before execution, raise on all final lookups, run a no-save node, and assertcomplete_queue_itemis called. -
invokeai/app/api/sockets.py:651-691: when the access-change reread fails, stale sockets are demoted but not disconnected. A password-revoked or deleted socket remains in its user room and can receive private events until a DB read succeeds.Test:use an old-epoch socket, makeusers.getraise, dispatch the event, then emit touser:<id>; verify no disconnect occurs and the socket receives it. -
invokeai/app/services/session_processor/session_processor_base.py:73-82: the new abstract method breaks downstream/customSessionProcessorBaseimplementations withTypeErrorat instantiation.Test:instantiate an old subclass implementing the previous abstract methods only.
Suggestions:
-
Instead of clearing failures for socketless owners, track running-owner failures separately and cancel after the retry limit; this bounds revoked work during DB outages without fabricating socket state.
-
Instead of treating an unreadable final owner record as active, persist an owner authorization snapshot/epoch with the queue item; this distinguishes transient read failure from genuine revocation without false cancellation or false completion.
-
Consider serializing per-user revocations with a monotonic generation; this permits safe socket disconnects while ignoring stale access-change events.
-
Instead of adding a mandatory abstract method, provide a default empty implementation or optional capability; this preserves existing custom processors.
…9436) * fix(api): run gallery and search routes off the event loop The gallery list/name routes and the auth dependencies were declared `async def` while calling synchronous SQLite services, so their database work ran on the event loop. For its whole duration the process served no other request and delivered no socket.io event, which users experienced as the backend freezing mid-generation rather than as a slow gallery. Declaring them `def` hands them to Starlette's threadpool instead. Measured against a 200k-image, 1.7 GB database: the latency of an unrelated request issued while a gallery name query is in flight drops from 1662 ms to 4 ms (search) and from 2298 ms to 881 ms (no search). The queries themselves are unchanged; only the loop is freed. The residual 881 ms in the no-search case is response serialization of 202k items, which is tracked separately. Adds a regression test that stubs a blocking service call and asserts an unrelated route still answers during it, plus a contributor doc describing the rule. * perf(gallery): add a flat item-names endpoint and deprecate the legacy ones The name list that drives the virtualized gallery wrapped every entry in an object carrying a `kind` discriminator. Building those models cost 820ms of the 2225ms service call on a 200k-item library, and every consumer threw the field away — `itemRefsToNames` mapped it off immediately and each caller re-derived the kind from the file extension via `isVideoName`. Adds `GET /v1/gallery/item_names`, returning a flat name list in the same shape as the image-only `ImageNamesResult`. An optional `created_date` filter subsumes the separate by-date virtual-board route, so regular boards and virtual dates now share one endpoint, one cache and one query-args selector instead of a skipToken branch duplicated across the grid hook, range selection and both auto-select listeners. Measured on a 200k-image, 1.7 GB database: 2.51s -> 1.57s per request, 8.48 MB -> 3.85 MB of response, and the residual event-loop stall from serializing the response drops from 466ms to 102ms at p95. Existing integrations still call the old routes, so all five legacy name endpoints keep working and are marked `deprecated=True` with a pointer to the replacement. * perf(api): stop gzipping responses that are already compressed Starlette's GZipMiddleware compresses every response type except text/event-stream, so every image and video the gallery serves was being deflate-compressed a second time. Measured: a 1024x1024 PNG (3.00 MB) costs 52ms of event-loop time to gzip and comes back at 3.01 MB — larger than it went in; a 2048x2048 PNG costs 210ms for the same non-result. Compression runs on the event loop, so that time is a full stall of the process. With auto-switch enabled the UI fetches the full image after every generated image, so the cost lands repeatedly during a batch. Replaces it with a content-type-aware subclass that compresses an allowlist of text, JSON, XML and SVG responses and passes everything else through. The UI bundle and the API's JSON keep their compression unchanged. Lowering compresslevel is not an alternative for this case: on already-compressed input level 1 costs 51ms against level 9's 52ms, because deflate still scans the whole body. Making the level configurable is worthwhile for the *compressible* path and is tracked separately. Note for deployments: media responses no longer carry Content-Encoding: gzip. * feat(queue): add lightweight item summaries endpoint * build: unpin FastAPI and move to 0.141.1 The pin sat at 0.118.3 with a comment guessing the OpenAPI crash on 0.119 was "probably Invoke's [bug], because we are doing something unusual with AnyInvocation". It was not: fastapi/_compat/v2.py assumed every field mapping carries a `$ref` and raised KeyError otherwise. Upstream fixed it in 0.124.0 with no change needed here. Two later changes needed adapting to, both of which fail silently: - 0.130 emits `contentMediaType: application/octet-stream` instead of `format: binary` for file uploads. typegen.js mapped only the latter to `Blob`, so upload call sites would have started typing their `File` argument as `string`. It now maps both. - 0.141 keeps an included router as a single node in `app.routes` instead of copying its routes into it. The default-deny auth guard walked `app.routes` looking for APIRoute instances and found 2 of 197 — passing while inspecting almost nothing. It now walks `iter_route_contexts`, the traversal FastAPI's own OpenAPI generation uses, and asserts a floor on the route count so going blind fails loudly instead. Schema changes are limited to ValidationError gaining the optional `input`/`ctx` fields; upload fields still resolve to Blob. Starlette stays at 0.48.0. * perf(api): run every synchronous route handler off the event loop Package A converted the eight gallery and search routes that caused the reported multi-minute stalls. The same defect was present across the rest of the API: 167 route handlers were declared `async def` while awaiting nothing, so their synchronous service calls ran on the event loop. Each one stalls the entire process for its duration - no other request served, no socket.io event delivered - which is why the symptom looked like the application freezing rather than one slow endpoint. Candidates were identified by AST rather than by hand: `async def` route handlers with no `await`, `async with` or `async for` anywhere in the body, cross-checked for references to asyncio, anyio or the loop. Two flagged candidates were false positives (both the word "loop" in a comment). The diff is 167 signature lines plus one signature that ruff collapsed onto a single line once `async ` was removed. Adds tests/app/routers/test_no_blocking_async_routes.py, which enforces the rule for every handler including ones written later - a per-route test cannot cover a route that does not exist yet, and this failure mode is invisible until a user has a large enough library to notice. Two tests that invoked route handlers directly were updated to call them as the plain functions they now are. * Docs Changes * Chore openapi * fix(queue): bound and chunk the id list on the queue summary route `item_summaries_by_ids` expanded every client-supplied id into one SQLite bind parameter, with no limit on the route. Posting more ids than the per-statement variable limit (32766 on SQLite >= 3.32) raised `OperationalError: too many SQL variables`, which the route reported as a generic HTTP 500. Cap the request body at 1000 ids so oversized lists are rejected by validation before any database work starts, matching the existing MAX_VIDEO_BATCH_SIZE precedent. Independently, chunk the `IN (...)` expansion at 900 binds so no caller — including internal ones not covered by the route bound — can hit the ceiling; 900 stays under the 999 limit of pre-3.32 builds too. Both regression tests fail without the fix: the router test posts 32767 ids and gets 200 instead of 422, and the service test reproduces the OperationalError verbatim, sized off the limit the running SQLite build actually enforces. * fix(api): close the two check-then-act races the sync sweep opened Converting the routes from `async def` to `def` removed an implicit guarantee: a handler whose body contains no `await` could not be interleaved with another request, because the event loop had no point at which to switch. Two handlers relied on it. `POST /auth/setup` did has_admin() then create_admin() in separate transactions. Two concurrent requests both saw no admin and both created one, so the loser ended up with a persistent admin account instead of the intended 400. The condition now lives inside the INSERT's own transaction, behind BEGIN IMMEDIATE so a second process (invoke-useradd --admin) cannot slip a write in either. This mirrors what #9360 does for the update/delete last-admin invariant; create_admin was the one path it does not cover. Custom node install, uninstall and reload all mutate the same custom-nodes directory, sys.modules and invocation registry. Interleaved, a failed install's cleanup rmtree'd the directory a concurrent install had just cloned into. A module-level lock restores the exclusion; the install and uninstall bodies moved into helpers so the lock scope is visible rather than an 80-line reindent. Both regression tests fail without their fix: the admin one creates two administrators, the pack one loses the successful install's directory. perf(queue): render the queue list from summaries, with one sanitizer The list fetched full queue items for every visible row, each carrying its session graph and workflow — megabytes per screenful for fields no row draws. The rows now render from SessionQueueItemSummary and the full item is fetched only when a row is expanded, which is what the summary route added in this branch was for; until now nothing consumed it. The per-item summary query provides the same cache tags as getQueueItem, so every existing invalidation path covers the list rows with nothing to wire up, and the optimistic status write is mirrored so a row's status still flips without a round trip. The range hook batches at the backend's 1000-id limit, which a fast fling could otherwise exceed. Both sanitizers are now one generic function over a single redaction table: the summary and the full item are two projections of one row, and a field stripped from the list but left on the detail view is leaked anyway. A test walks the intersection of both models and asserts they redact it identically. `device` is deliberately not redacted in either. It names the instance's GPU rather than anything about the other user's work, and the list has always shown it — redacting it here would have quietly changed what non-admins see. parent_item_id joins the summary because the rows decide from it whether to offer a retry. * fix(api): close the sync-sweep races and wire up the queue summary route Two review follow-ups landed together here. Converting the routes from `async def` to `def` removed an implicit guarantee: a handler whose body contains no `await` could not be interleaved with another request, because the event loop had no point at which to switch. `POST /auth/setup` did has_admin() then create_admin() in separate transactions. Two concurrent requests both saw no admin and both created one, so the loser ended up with a persistent admin account instead of the intended 400. The condition now lives inside the INSERT's own transaction, behind BEGIN IMMEDIATE so a second process (invoke-useradd --admin) cannot slip a write in either. This mirrors what #9360 does for the update/delete last-admin invariant; create_admin is the one path it does not cover. Custom node install, uninstall and reload all mutate the same custom-nodes directory, sys.modules and invocation registry. Interleaved, a failed install's cleanup rmtree'd the directory a concurrent install had just cloned into. A module-level lock restores the exclusion; the install and uninstall bodies moved into helpers so the lock scope is visible rather than an 80-line reindent. Both regression tests fail without their fix: the admin one creates two administrators, the pack one loses the successful install's directory. The route added earlier in this branch had none — the list still fetched full queue items for every visible row, each carrying its session graph and workflow, so the claimed saving was not being realised. The rows now render from SessionQueueItemSummary and the full item is fetched only when a row is expanded. Measured against the previous commit, same backend and same 396-item queue, identical scroll (page load, queue tab, scroll to 60%): requests 62 -> 2 payload (gzip) 262 KB -> 1.3 KB (30 items) server time 60ms -> 4ms (30 items) The request count collapses because the old path was self-amplifying: the range hook re-asks which ids are uncached on every range event, and at ~60ms per response the cache had not filled yet, so overlapping fetches piled up. A side effect worth knowing: `items_by_ids` silently skips items it cannot deserialize, so a queue item whose graph references an unregistered node type left its row permanently blank. Summaries never touch the graph, so the row now renders and only the expanded detail is affected. The per-item summary query provides the same cache tags as getQueueItem, so every existing invalidation path covers the list rows with nothing to wire up; the optimistic status write is mirrored so a row still flips without a round trip. The range hook batches at the backend's 1000-id limit, which a fast fling could otherwise exceed. The summary and the full item are two projections of one row, and a field stripped from the list but left on the detail view is leaked anyway. Both now go through one generic function over a single redaction table; a test walks the intersection of the two models and asserts they redact it identically. `device` is deliberately not redacted in either. It names the instance's GPU rather than anything about the other user's work, and the list has always shown it — redacting it would have quietly changed what non-admins see. parent_item_id joins the summary because the rows decide from it whether to offer a retry. * fix(api): finish the review's non-blocking list Six follow-ups from @lstein's sweep that were left open. `require_admin` and `require_admin_or_default` go back to `async def`. They only read `is_admin` off already-resolved token data, so declaring them `def` bought a threadpool round-trip per admin request and nothing else. The `users.get` that can block lives in `get_current_user`, which stays synchronous — the docstrings now say why the two layers differ. The AST guard now inspects what it claims to. It walked only `tree.body`, so a handler registered from inside a factory function or an `if` block was never seen, and `_awaits_something` used `ast.walk`, which counts `await`s inside nested closures — a handler could have passed by defining an inner async helper it never awaits. Both are fixed and both now have their own tests, so the guard's behaviour is pinned rather than asserted in a comment. `convert_model` takes a lock non-blocking and answers 409 otherwise. Blocking would be wrong: a conversion runs for minutes, and for the same key the second caller reads a record the first is midway through replacing. Two conversions in flight also means two models resident at once, which nothing bounds. The body moved into `_convert_model` so the lock scope is visible. Tested for the 409 and for the lock surviving a failed conversion rather than wedging the endpoint for the process's lifetime. `do_hf_login` and `reset_hf_token` hold a lock across the write and the status read-back, which otherwise could report a status belonging to a different token than the one just written. The blocking-work doc gains the bound it was missing: anyio's thread limiter holds 40 tokens, so past forty concurrent blocking requests the stall moves rather than vanishes — and anything else needing a thread queues behind them, including the synchronous auth dependency that runs before a handler is reached. Noted there too that `test_event_loop_blocking.py` cannot show this, because its probe route has neither auth nor database access. `QueueItemDetail` tells a failed fetch apart from a pending one. A queue item the backend cannot serve — one whose graph references a node type this build no longer registers — previously read as "Loading" forever. Left alone deliberately: the stale `old_is_public` in the workflow-updated event, which is cosmetic and would need `workflow_records.update()` to return the previous row to fix properly; and the `delete_user` / `update_user` last-admin invariants, which belong to #9360. * chore: drop planning notes and scratch files from the branch These arrived via a merge of the fork's own branch, where they had been tracked since an earlier `git add -A`: ten *_PLAN.md files at the repo root, the `plans/` tree (fp8-compute, pid-porting, gzip-compresslevel) and `testscript.py`. None of them belong to this change — they are working notes for unrelated features — and they made up 21 of the 90 files a reviewer had to page past. The files stay on disk; only the index drops them. They are listed in .git/info/exclude locally rather than in .gitignore, so the repository carries no opinion about one contributor's notes. * fix(models): serialize the operations that share the models directory Follow-up to the conversion lock, which bounded conversions against each other but not against everything else that mutates a model now that those routes run in the threadpool too. `delete_model` and `bulk_delete_models` ran free alongside a conversion. Conversion is a read-modify-replace spanning many service calls — load, write a diffusers copy, rename the record, install the copy, delete the original — so a delete landing in the middle removes the record it is still working from. The conversion's own final delete then fails, and the copy it already installed survives: the admin is answered 204 and the model reappears under a new key. A per-key claim serializes operations on one model while leaving different models free to run in parallel; a global lock would have made every delete wait out an unrelated conversion. Bulk deletion claims each key separately and reports a busy one through its existing per-key `failed` list rather than aborting the request or racing the holder. Deletion never takes the conversion lock, so the two are always acquired in the same order. `DELETE /sync/orphaned` was the same collision from the other side. An orphan is defined as model files under the models root with no database record, which is also an exact description of a conversion in progress: it built its diffusers copy in a `TemporaryDirectory` directly under `models/`, so a scan taken during a conversion reported that working directory and the delete route would rmtree it mid-write. Fixed at the cause rather than with another lock — the copy is now built in `models/.convert_tmp`, still on the models volume so `install_path` moves rather than copies across a filesystem boundary, but named in `SKIP_DIRS`. The name lives next to that list as `CONVERSION_SCRATCH_DIRNAME` so writer and scanner cannot drift apart. All three regression tests fail without their fix: the delete reaches the installer mid-conversion, bulk deletion removes the busy key, and the scan reports `.convert_tmp` as an orphan. The scan test carries a control asserting a real orphan is still found, so a scan that has stopped finding anything cannot pass it. The same scan is equally blind to an in-flight install, but the installer has always run in its own worker thread — that race predates this branch and is left alone. * fix(models): extend the per-key claim to every operation on a model record Follow-up to 6c6e38e, which serialized conversion against deletion but left the operations that rewrite a record or its image running free beside it. The hazard is not the individual write — it is that conversion carries a snapshot. It reads the config before it starts and, minutes later, writes that snapshot's name, description, hash and source into the replacement record, then moves the model image over to the new key. Anything accepted on the old key in between is answered 200 and then silently discarded: a rename vanishes, a re-probe's findings vanish, an uploaded cover image is replaced by the one the conversion carried, a deleted one comes back. So `reidentify_model`, `update_model_record`, `update_model_image`, `delete_model_image` and `bulk_reidentify_models` now take the same per-key claim as conversion and deletion. Bulk reidentification claims each key separately and reports a busy one through its existing per-key `failed` list rather than aborting the request. `update_model_record` is the one `async def` among them; the claim only holds a threading lock across a set membership test, never across the `await`, so it cannot deadlock the loop. `reidentify_model`'s body moved into `_reidentify_model` so the bulk route can call it instead of carrying its own copy of the retain-these-fields logic — the two copies had already drifted to opposite `hasattr` orderings, and only one of them would have been updated the next time that list changes. Four new regression tests, each holding a conversion at a barrier and racing one operation against it; with the claim removed, all six of this file's race tests fail. * chore: typegen/openapi * Addressed PR comments * Addressed PR comments --------- Co-authored-by: JPPhoto <jpollack@jpollackphoto.com> Co-authored-by: Jonathan <34005131+JPPhoto@users.noreply.github.com>
Summary
Follow-on PR 1 from @JPPhoto's review of #9163 (the "Database role changes do not invalidate JWT privileges" / "Open sockets retain revoked account privileges" / "Deactivated users can continue queued execution" findings).
Note
Stacked on #9163 — this branch is based on the WAN video branch because the fixes build on machinery that only exists there (media cookie, video invocation-context authorization). The diff will show #9163's changes until it merges; only the top commit (
960d607c76) is new. I'll rebase/retarget once #9163 lands.1. JWT privileges now derive from the database on every request
get_current_user,get_current_user_or_default, the media-cookie validator) build the returnedTokenDatafrom the database record — the token proves identity only. A demoted administrator's old token gets 403 on admin endpoints immediately; a promoted user gains admin on their next request without re-login (this defines the promotion semantics JPPhoto asked to pin down).X-Refreshed-Token, no media cookie) and mints refreshed tokens from the database record, so a stale admin claim can never be renewed — the media cookie renewal path is closed with it.2. Open sockets are re-authorized live
_handle_connectderivesis_adminfrom the database, so a demoted admin reconnecting with an old token does not rejoin the admin room.user_access_changedevent (not registered withpayload_schema, never emitted to clients — no typegen churn) is emitted by the user-management routes on role/status changes. The socket layer responds: demotion leaves the admin room (and the cachedis_adminis corrected sosubscribe_queuecan't re-add it), promotion joins it, deactivation/deletion disconnects all the user's sockets.3. Deactivated users' queued execution is revoked
Policy: pending items are rejected (canceled) at dequeue; running items are canceled immediately where possible and always stop before the next node.
user_access_changedand cancels the currently running item immediately — this drives the existing cancel-event machinery, so step-callback nodes (e.g. denoising) stop mid-node rather than running to completion.systemuser are exempt, per the review spec.Tests (all per JPPhoto's specs)
tests/app/routers/test_privilege_revocation.py— demoted admin: 403 + noX-Refreshed-Token+ no media cookie; same token denied reading another user's private image (positive pre-demotion, negative post-demotion); promoted user gains admin with old token; unchanged admin refresh carriesis_admin=true; demoted user's allowed mutation refreshes withis_admin=false; event emission on demotion/deactivation/deletion and not on display-name changes.tests/app/api/test_sliding_window_token.py— new multiuser class: demoted/promoted refresh carries DB role; deactivated/deleted users get no refresh;remember_mepreserved. (Existing tests now run under an explicit single-user harness.)tests/app/test_socket_privilege_revocation.py— reconnect-with-old-token after demotion; deactivated reconnect rejected; live demotion leaves admin room and can't re-subscribe into it; deactivation/deletion disconnects; promotion joins; other users' sockets untouched (the positive unchanged-admin case).tests/app/services/session_processor/test_privilege_revocation.py—queue_owner_is_activematrix; dequeue rejection (incl. concurrent-deletion race); immediate cancel of the running item on deactivation; multi-node session stops after node 1 when the owner is deactivated mid-run; positive active-user and single-user/system cases.tests/app/services/shared/test_invocation_context_{images,videos}.py— inactive/deleted queue user denied reads and saves (even uncategorized); active user still saves.370 tests pass across the affected areas (auth routes, multiuser authorization, sockets, session queue/processor, invocation context, videos multiuser); ruff clean.
🤖 Generated with Claude Code