diff --git a/invokeai/app/api/routers/_access.py b/invokeai/app/api/routers/_access.py index fae3971a144..b039e3bc2d2 100644 --- a/invokeai/app/api/routers/_access.py +++ b/invokeai/app/api/routers/_access.py @@ -8,7 +8,10 @@ from invokeai.app.api.auth_dependencies import CurrentUserOrDefault from invokeai.app.api.dependencies import ApiDependencies -from invokeai.app.services.board_records.board_records_common import BoardVisibility +from invokeai.app.services.board_records.board_records_common import ( + BoardRecordNotFoundException, + BoardVisibility, +) def assert_image_owner(image_name: str, current_user: CurrentUserOrDefault) -> None: @@ -28,18 +31,56 @@ def assert_image_owner(image_name: str, current_user: CurrentUserOrDefault) -> N board_id = ApiDependencies.invoker.services.board_image_records.get_board_for_image(image_name) if board_id is not None: + # The board *record*, not its DTO: the decision needs only the owner and the + # visibility, and the DTO would drag in cover-image resolution plus three COUNT + # aggregates — five extra queries and five extra ways to fail per name. + # + # Only a board positively known to be gone falls through to the 403. A storage error + # propagates instead of being caught here: `board_records.get` deliberately does not + # translate sqlite errors into not-found, and a caller that cannot decide ownership + # must not report the name as an ordinary permission denial — the batch loops treat a + # 403 as a silent auth skip, which turned a locked database into images dropped from + # the response with no failure reported at all. try: - board = ApiDependencies.invoker.services.boards.get_dto(board_id=board_id) + board = ApiDependencies.invoker.services.board_records.get(board_id) + except BoardRecordNotFoundException: + pass + else: if board.user_id == current_user.user_id: return if board.board_visibility == BoardVisibility.Public: return - except Exception: - pass raise HTTPException(status_code=403, detail="Not authorized to modify this image") +def _assert_image_record_exists(image_name: str) -> None: + """Turn a refusal into a 404 when the image is positively gone. + + The two refusals mean opposite things to a client holding a reference to the image — a + workflow's image field, a reference image on a canvas layer. Gone is permanent, and the + reference should be dropped. Denied is a permission decision that can be reversed (a board + flipped back to Shared, an owner re-granting access), and dropping the reference over one + destroys work the user cannot get back by restoring the permission. + + Nothing above can tell them apart: the ownership test rests on `images.user_id`, which is + gone with the row, so a deleted image reaches that same 403 as a foreign one. So the + distinction is made here, on the refusal path only — the happy path pays nothing for it. + + A storage error propagates rather than answering either, so an unreadable database cannot + present as a deleted image and take the user's references down with it. `exists` is a bare + row probe rather than `get` for the same reason from the other side: `get` deserializes, so + a row written by a newer version — an enum value this one does not know — would fail exactly + as absence does, and a live image would be reported gone. + + The cost is that an authenticated caller can now tell an absent image from one they may not + read. Image names are generated UUIDs, so this buys an attacker nothing they could enumerate, + and it is the answer admins have always received. + """ + if not ApiDependencies.invoker.services.image_records.exists(image_name): + raise HTTPException(status_code=404, detail="Image not found") + + def assert_image_read_access(image_name: str, current_user: CurrentUserOrDefault) -> None: """Raise 403 if the current user may not view the image. @@ -57,13 +98,17 @@ def assert_image_read_access(image_name: str, current_user: CurrentUserOrDefault board_id = ApiDependencies.invoker.services.board_image_records.get_board_for_image(image_name) if board_id is not None: + # See `assert_image_owner` for why this reads the board record and catches only + # not-found: a lookup that cannot be decided must not present as a permission decision. try: - board = ApiDependencies.invoker.services.boards.get_dto(board_id=board_id) + board = ApiDependencies.invoker.services.board_records.get(board_id) + except BoardRecordNotFoundException: + pass + else: if board.board_visibility in (BoardVisibility.Shared, BoardVisibility.Public): return - except Exception: - pass + _assert_image_record_exists(image_name) raise HTTPException(status_code=403, detail="Not authorized to access this image") diff --git a/invokeai/app/api/routers/board_images.py b/invokeai/app/api/routers/board_images.py index 00c5c3a9bec..ab0b32cadcd 100644 --- a/invokeai/app/api/routers/board_images.py +++ b/invokeai/app/api/routers/board_images.py @@ -1,9 +1,14 @@ +from enum import Enum, auto + from fastapi import Body, HTTPException from fastapi.routing import APIRouter from invokeai.app.api.auth_dependencies import CurrentUserOrDefault from invokeai.app.api.dependencies import ApiDependencies from invokeai.app.api.routers.image_move_maintenance import assert_image_move_maintenance_inactive +from invokeai.app.api.routers.images import MAX_IMAGE_BATCH_SIZE, ImageName +from invokeai.app.services.board_records.board_records_common import BoardRecordNotFoundException +from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException from invokeai.app.services.images.images_common import AddImagesToBoardResult, RemoveImagesFromBoardResult board_images_router = APIRouter(prefix="/v1/board_images", tags=["boards"]) @@ -16,13 +21,26 @@ def _assert_board_write_access(board_id: str, current_user: CurrentUserOrDefault - The user is an admin. - The user owns the board. - The board visibility is Public (public boards accept contributions from any user). + + Reads the board *record*, not its DTO. The decision needs only the owner and the + visibility, while BoardService.get_dto also resolves the cover image and runs three COUNT + aggregates over the board's contents — six queries to answer a question two columns settle. + That cost is the only reason a batch route would be tempted to decide once and reuse the + answer for every name, and reusing it is what lets a permission revoked mid-batch keep + working until the request ends. One indexed SELECT per name is cheap enough to re-decide. + (These routes are sync `def`, so the queries occupy a threadpool worker rather than the + event loop — but a 1000-name batch still holds one for six thousand round trips.) """ from invokeai.app.services.board_records.board_records_common import BoardVisibility try: - board = ApiDependencies.invoker.services.boards.get_dto(board_id=board_id) - except Exception: + board = ApiDependencies.invoker.services.board_records.get(board_id) + except BoardRecordNotFoundException: raise HTTPException(status_code=404, detail="Board not found") + # Anything else — a locked or unreadable database — propagates. Catching it here would + # answer "no such board", which the batch loops below treat as a name to skip: a disk error + # would then drop names out of the response entirely, reported neither as moved nor as + # failed, and the client would show the move as done until the next refresh. if current_user.is_admin: return if board.user_id == current_user.user_id: @@ -32,6 +50,74 @@ def _assert_board_write_access(board_id: str, current_user: CurrentUserOrDefault raise HTTPException(status_code=403, detail="Not authorized to modify this board") +def _image_record_exists(image_name: str) -> bool: + """True if the image record is still present, False if it has been deleted. + + A storage error answers True: only a record positively known to be gone may be downgraded + from a reported failure to a silent skip. `ImageRecordStorage.get` no longer translates + sqlite errors into not-found, so the two cases are distinguishable here. + """ + try: + ApiDependencies.invoker.services.image_records.get(image_name) + return True + except ImageRecordNotFoundException: + return False + except Exception: + return True + + +class _ScopedRemoveOutcome(Enum): + """What a scoped board-image DELETE turned out to have done, judged by its row count.""" + + REMOVED = auto() + """The row was deleted -- or the image was concurrently uncategorized by someone else, in + which case the postcondition the caller asked for (off every board) holds, and reporting it + removed is what lets the client's stale view of the old board catch up. Safe to report, + unlike a deleted name: the DTO exists, so the tag-driven refetches succeed.""" + + MOVED = auto() + """Now on another board: the ask is not satisfied, and a retry will re-read and + re-authorize against the board the image actually sits on now. Report as failed.""" + + GONE = auto() + """Image deleted concurrently: a skip, never a success -- reporting it removed would drive + the client's tag-driven getImageDTO refetch straight into a 404.""" + + +def _remove_from_board_and_classify(image_name: str, old_board_id: str) -> _ScopedRemoveOutcome: + """Runs the scoped DELETE for a name read as sitting on `old_board_id`, then classifies. + + The scoped DELETE misses when the image leaves `old_board_id` between the caller's read + and this write. The row count is the only signal the scope held: ignore it and the route + reports a removal that did not happen, invalidating the wrong boards while the client + counts the name as done. A zero-row miss is classified by where the image is now. + + The existence probe is direct rather than through `_image_record_exists`: that helper + answers True on a storage error, which is the conservative bias where True means "report + as failed" (the add loop) -- here True means "report as removed", and a transient storage + error must not manufacture a success. Storage errors -- the DELETE's own and the + classification reads' -- propagate instead: a name whose state cannot be decided must be + reported by the caller as failed, never as done. + + `old_board_id` must be a real board id: uncategorized is the absence of a row, so a scoped + DELETE for "none" cannot match and the classification would spend two reads confirming + what the caller's DTO read already said. + """ + deleted_rows = ApiDependencies.invoker.services.board_images.remove_image_from_board( + image_name=image_name, board_id=old_board_id + ) + if deleted_rows > 0: + return _ScopedRemoveOutcome.REMOVED + current_board_id = ApiDependencies.invoker.services.board_image_records.get_board_for_image(image_name) + if current_board_id is not None: + return _ScopedRemoveOutcome.MOVED + try: + ApiDependencies.invoker.services.image_records.get(image_name) + except ImageRecordNotFoundException: + return _ScopedRemoveOutcome.GONE + return _ScopedRemoveOutcome.REMOVED + + def _assert_image_direct_owner(image_name: str, current_user: CurrentUserOrDefault) -> None: """Raise 403 if the current user is not the direct owner of the image. @@ -78,6 +164,8 @@ def add_image_to_board( return AddImagesToBoardResult( added_images=list(added_images), + # Single-image route: a failure here is a 500, never a partial success. + failed_images=[], affected_boards=list(affected_boards), ) except Exception: @@ -104,13 +192,31 @@ def remove_image_from_board( _assert_board_write_access(old_board_id, current_user) assert_image_move_maintenance_inactive() removed_images: set[str] = set() + failed_images: set[str] = set() affected_boards: set[str] = set() - ApiDependencies.invoker.services.board_images.remove_image_from_board(image_name=image_name) - removed_images.add(image_name) - affected_boards.add("none") - affected_boards.add(old_board_id) + if old_board_id == "none": + # Already off every board — the postcondition holds without a write. No + # board_images row ever carries board_id="none", so a scoped DELETE could not + # match anyway; see the identical shortcut in the batch loop below. + removed_images.add(image_name) + affected_boards.add("none") + else: + # The same row-count classification the batch loop uses. This route used to + # ignore the count, so an image that left old_board_id between the read above and + # the write was reported removed anyway — a false success that invalidated the + # wrong boards and told the client the name was done. + outcome = _remove_from_board_and_classify(image_name, old_board_id) + if outcome is _ScopedRemoveOutcome.REMOVED: + removed_images.add(image_name) + affected_boards.add("none") + affected_boards.add(old_board_id) + elif outcome is _ScopedRemoveOutcome.MOVED: + failed_images.add(image_name) + # GONE lands in neither list, matching the batch route's treatment of a name that + # vanished mid-flight; the client's refetches surface the deletion. return RemoveImagesFromBoardResult( removed_images=list(removed_images), + failed_images=list(failed_images), affected_boards=list(affected_boards), ) @@ -132,7 +238,9 @@ def remove_image_from_board( def add_images_to_board( current_user: CurrentUserOrDefault, board_id: str = Body(description="The id of the board to add to"), - image_names: list[str] = Body(description="The names of the images to add", embed=True), + image_names: list[ImageName] = Body( + description="The names of the images to add", embed=True, max_length=MAX_IMAGE_BATCH_SIZE + ), ) -> AddImagesToBoardResult: """Adds a list of images to a board""" _assert_board_write_access(board_id, current_user) @@ -144,9 +252,39 @@ def add_images_to_board( raise try: + # Skip — but do not re-raise — auth failures so a foreign name mid-batch doesn't + # discard the response payload for images that were already moved. Re-raising turned + # partial successes into an error-shaped response, so the client never invalidated + # caches for the images that did move and the UI kept showing them on their old board + # until the next full refresh. Matches star_images_in_list and delete_images_from_list. added_images: set[str] = set() + failed_images: set[str] = set() affected_boards: set[str] = set() - for image_name in image_names: + # Dedup while preserving order — a repeated name would otherwise be processed twice + # and could land in both added_images and failed_images. + for image_name in dict.fromkeys(image_names): + # The destination decision sits in its own arm because its refusal means the + # opposite of the per-image one. A foreign or vanished *image* is that name's own + # problem — a skip, matching the other batch routes. A revoked or deleted + # *destination* is the whole request's problem: every remaining name meets it too, + # and treating those as skips answers 201 with empty lists, which the client reads + # as success and clears the user's selection over. Still re-decided per name, and + # the loop keeps going rather than aborting: access restored mid-batch lets later + # names land, and every name refused while it was gone is reported as failed. + try: + # Re-decided per name rather than resting on the check above. Write access to + # the target board can be revoked while the batch is running — a board flipped + # from Public to Private — and a decision taken once at the top of a 1000-name + # request would let a contributor keep writing to it for the rest of the batch. + _assert_board_write_access(board_id, current_user) + except HTTPException: + failed_images.add(image_name) + continue + except Exception: + # The helper propagates storage errors precisely so they are not mistaken for + # "no such board"; a name whose destination could not be decided is reported. + failed_images.add(image_name) + continue try: _assert_image_direct_owner(image_name, current_user) old_board_id = ( @@ -161,11 +299,23 @@ def add_images_to_board( affected_boards.add(old_board_id) except HTTPException: - raise + continue except Exception: - pass + # A genuine storage failure, not an auth/404 skip: it used to be swallowed by + # `pass`, so the client counted the image as moved and the move silently + # reverted on reload. + # + # Except that a name deleted between the ownership check and the insert lands + # here too, and not as something recognizable: board_images.image_name is a + # foreign key onto images.image_name, so the INSERT fails with a bare + # sqlite3.IntegrityError. Nothing in the exception says "gone", so the record + # is probed instead — only on this path, so the happy path pays nothing. + if not _image_record_exists(image_name): + continue + failed_images.add(image_name) return AddImagesToBoardResult( added_images=list(added_images), + failed_images=list(failed_images), affected_boards=list(affected_boards), ) except HTTPException: @@ -185,36 +335,98 @@ def add_images_to_board( ) def remove_images_from_board( current_user: CurrentUserOrDefault, - image_names: list[str] = Body(description="The names of the images to remove", embed=True), + image_names: list[ImageName] = Body( + description="The names of the images to remove", embed=True, max_length=MAX_IMAGE_BATCH_SIZE + ), ) -> RemoveImagesFromBoardResult: """Removes a list of images from their board, if they had one""" try: assert_image_move_maintenance_inactive() except HTTPException: for image_name in image_names: - old_board_id = ApiDependencies.invoker.services.images.get_dto(image_name).board_id or "none" + try: + old_board_id = ApiDependencies.invoker.services.images.get_dto(image_name).board_id or "none" + except ImageRecordNotFoundException: + # A name deleted by a concurrent session. The main loop treats that as a skip; + # letting it escape from inside this handler would replace the 409 with a 500. + continue if old_board_id != "none": _assert_board_write_access(old_board_id, current_user) raise try: + # Skip — but do not re-raise — auth failures, for the same reason as add_images_to_board + # above: one name on a board the caller cannot write must not discard the payload for + # the images already removed. removed_images: set[str] = set() + failed_images: set[str] = set() affected_boards: set[str] = set() - for image_name in image_names: + + # Dedup while preserving order — a repeated name would otherwise be processed twice + # and could land in both removed_images and failed_images. + for image_name in dict.fromkeys(image_names): try: old_board_id = ApiDependencies.invoker.services.images.get_dto(image_name).board_id or "none" - if old_board_id != "none": + except ImageRecordNotFoundException: + # The image is gone — deleted by a concurrent session between the client + # building its selection and this request. That is a skip, not a failure, and + # must not be toasted as one. Resolved in its own block because unlike the + # other routes this one reads the DTO *before* any authorization check, so a + # 404 here would otherwise be indistinguishable from a storage failure below. + # Narrow on purpose: a real storage error must still reach failed_images. + continue + except Exception: + failed_images.add(image_name) + continue + + # The one authorization decision, and it is taken fresh for every name. Memoizing + # it per board is tempting — the skip removed the early abort that used to cap an + # unauthorized batch at one check — but a cached True outlives the permission it + # recorded: flip a board from Public to Private mid-batch and the rest of the names + # are removed on an answer that is no longer true. _assert_board_write_access reads + # the board record (one indexed SELECT), so re-deciding costs about what the + # get_dto() it replaced cost once. + # + # Kept out of the try below so that the only way to skip a name for auth is this + # branch. Note the two failure modes are not the same: "not allowed" is a skip, + # while a storage error means the decision could not be taken at all, and a name we + # could not decide about must be reported rather than silently dropped. + if old_board_id != "none": + try: _assert_board_write_access(old_board_id, current_user) - ApiDependencies.invoker.services.board_images.remove_image_from_board(image_name=image_name) + except HTTPException: + continue + except Exception: + failed_images.add(image_name) + continue + + # No board_images row ever carries board_id="none" — uncategorized is the absence + # of a row — so for a name already off every board the scoped DELETE cannot match + # and the zero-row classification below would spend two reads confirming what the + # DTO already said. Same report the classification would produce, minus the reads. + if old_board_id == "none": removed_images.add(image_name) affected_boards.add("none") - affected_boards.add(old_board_id) - except HTTPException: - raise + continue + + try: + outcome = _remove_from_board_and_classify(image_name, old_board_id) + if outcome is _ScopedRemoveOutcome.REMOVED: + removed_images.add(image_name) + affected_boards.add("none") + affected_boards.add(old_board_id) + elif outcome is _ScopedRemoveOutcome.MOVED: + failed_images.add(image_name) + # GONE: a skip, exactly as the gone-block above treats a name that vanished + # before the loop reached it. except Exception: - pass + # A genuine storage failure, not an auth/404 skip — see add_images_to_board. + # The zero-row classification's own reads land here too: a name whose state + # cannot be decided is reported, never dropped. + failed_images.add(image_name) return RemoveImagesFromBoardResult( removed_images=list(removed_images), + failed_images=list(failed_images), affected_boards=list(affected_boards), ) except HTTPException: diff --git a/invokeai/app/api/routers/images.py b/invokeai/app/api/routers/images.py index d352b7bd770..cd7e42f1a26 100644 --- a/invokeai/app/api/routers/images.py +++ b/invokeai/app/api/routers/images.py @@ -2,13 +2,13 @@ import io import json import traceback -from typing import ClassVar, Optional +from typing import Annotated, ClassVar, Optional from fastapi import BackgroundTasks, Body, HTTPException, Path, Query, Request, Response, UploadFile from fastapi.responses import FileResponse from fastapi.routing import APIRouter from PIL import Image -from pydantic import BaseModel, Field, model_validator +from pydantic import BaseModel, Field, StringConstraints, model_validator from invokeai.app.api.auth_dependencies import CurrentMediaUserOrDefault, CurrentUserOrDefault from invokeai.app.api.dependencies import ApiDependencies @@ -28,6 +28,7 @@ ImageCategory, ImageNamesResult, ImageRecordChanges, + ImageRecordNotFoundException, ResourceOrigin, ) from invokeai.app.services.images.images_common import ( @@ -37,7 +38,7 @@ StarredImagesResult, UnstarredImagesResult, ) -from invokeai.app.services.shared.pagination import OffsetPaginatedResults +from invokeai.app.services.shared.pagination import MAX_PAGE_SIZE, OffsetPaginatedResults from invokeai.app.services.shared.sqlite.sqlite_common import SQLiteDirection from invokeai.app.util.controlnet_utils import heuristic_resize_fast from invokeai.backend.image_util.util import np_to_pil, pil_to_np @@ -48,6 +49,21 @@ # images are immutable; set a high max-age IMAGE_MAX_AGE = 31536000 +# Every name in a batch body costs at least one DB lookup, so an unbounded list lets an +# authenticated client pin a worker with a single request. Mirrors MAX_VIDEO_BATCH_SIZE +# in the videos router. +# +# "At least one" is doing real work in that sentence. The authorization helpers in _access.py +# short-circuit on the first hit, so an admin or a direct owner costs 0-1 queries per name -- +# but a user reading someone else's Shared/Public board falls all the way through to +# boards.get_dto(), which is six queries including three COUNT aggregates over the board's +# contents. That is the case the bound has to hold, and it is why no route gets a laxer one: +# the oversized selections this cap rejects are split client-side instead. +MAX_IMAGE_BATCH_SIZE = 1000 +# Names are UUID-derived filenames; the bound only exists to keep a hostile body from +# turning into megabytes of SQL parameters. +ImageName = Annotated[str, StringConstraints(max_length=255)] + def _get_image_cache_control() -> str: if ApiDependencies.invoker.services.configuration.multiuser: @@ -231,6 +247,9 @@ def delete_image( return DeleteImagesResult( deleted_images=list(deleted_images), + # Single-image route: the swallowed failure above already leaves deleted_images empty, + # which is how this route has always reported it. + failed_images=[], affected_boards=list(affected_boards), ) @@ -298,7 +317,13 @@ def get_image_dto( try: return ApiDependencies.invoker.services.images.get_dto(image_name) - except Exception: + except ImageRecordNotFoundException: + # Only a genuinely missing record answers 404. This route is what a workflow's image + # field and a canvas reference image read, and they drop the user's reference when it + # 404s, so nothing else may wear that answer: a board lookup against an unreadable + # database, or a URL service failure, would otherwise clear a live image out of the + # workflows using it. Narrowed here rather than everywhere, because this is the 404 + # that is acted on destructively — the media, metadata and workflow routes keep theirs. raise HTTPException(status_code=404) @@ -464,8 +489,11 @@ def list_image_dtos( default=None, description="The board id to filter by. Use 'none' to find images without a board.", ), - offset: int = Query(default=0, description="The page offset"), - limit: int = Query(default=10, description="The number of images per page"), + # Bounds matter: these flow verbatim into SQL, and a negative LIMIT means *unlimited* + # in SQLite — one request would materialize every image row into a DTO. The lower + # bound on `limit` is 0, not 1: the frontend issues limit=0 count-only queries. + offset: int = Query(default=0, ge=0, description="The page offset"), + limit: int = Query(default=10, ge=0, le=MAX_PAGE_SIZE, description="The number of images per page"), order_dir: SQLiteDirection = Query(default=SQLiteDirection.Descending, description="The order of sort"), starred_first: bool = Query(default=True, description="Whether to sort by starred images first"), search_term: Optional[str] = Query(default=None, description="The term to search for"), @@ -497,7 +525,9 @@ def list_image_dtos( @images_router.post("/delete", operation_id="delete_images_from_list", response_model=DeleteImagesResult) def delete_images_from_list( current_user: CurrentUserOrDefault, - image_names: list[str] = Body(description="The list of names of images to delete", embed=True), + image_names: list[ImageName] = Body( + description="The list of names of images to delete", embed=True, max_length=MAX_IMAGE_BATCH_SIZE + ), ) -> DeleteImagesResult: try: assert_image_move_maintenance_inactive() @@ -518,6 +548,13 @@ def delete_images_from_list( # be processed twice, and the second pass's not-found error would land the same # name in both deleted_images and failed_images. for image_name in dict.fromkeys(image_names): + # Bound only once the record has been read, which is what separates the two ways + # this loop can raise ImageRecordNotFoundException. Still None means the read itself + # failed, so nothing here ever established that the record existed — for an admin + # the ownership check is a no-op returning before it touches storage, so a name that + # never existed reaches that raise. Set means the record was read a line earlier and + # only the delete lost the race. + board_id: str | None = None try: _assert_image_owner(image_name, current_user) image_dto = ApiDependencies.invoker.services.images.get_dto(image_name) @@ -527,6 +564,33 @@ def delete_images_from_list( affected_boards.add(board_id) except HTTPException: continue + except ImageRecordNotFoundException: + if board_id is None: + # Never read, so there is no postcondition the caller asked for and nothing + # for the client to clean up. A skip, as before. + continue + # The record is already gone — a concurrent session deleted it after this + # iteration read it. The caller asked for it to be gone and it is, so report the + # idempotently satisfied postcondition. The client uses deleted_images to remove + # stale selections and references. The board is reported with it: every + # board-scoped tag getDeleteImagesTags publishes comes from affected_boards, and + # it ignores deleted_images by design, so omitting it leaves the board's counts + # stale while the name is reported gone. + deleted_images.add(image_name) + affected_boards.add(board_id) + # + # Deliberately unlike remove_images_from_board, which skips the same race. Two + # reasons it cannot copy this. Its result list feeds + # getTagsToInvalidateForImageMutation, so a vanished name there would invalidate + # getImageDTO for a record that no longer exists and drive a 404 refetch — + # getDeleteImagesTags ignores deleted_images precisely to avoid that. And it + # reads the DTO *before* any authorization check, so reporting a 404 as success + # would answer for names the caller was never entitled to touch. + # + # This is narrow only because image_records.get() no longer translates a + # sqlite3.Error into this exception — see the comment there. If that + # translation ever comes back, a locked or corrupt database would land here + # and a whole failed batch would answer 200 with empty result lists. except Exception: # A genuine deletion failure (not an auth/404 skip) — report it so the # client can surface a partial-failure warning, matching the video path. @@ -584,7 +648,9 @@ class ImagesUpdatedFromListResult(BaseModel): @images_router.post("/star", operation_id="star_images_in_list", response_model=StarredImagesResult) def star_images_in_list( current_user: CurrentUserOrDefault, - image_names: list[str] = Body(description="The list of names of images to star", embed=True), + image_names: list[ImageName] = Body( + description="The list of names of images to star", embed=True, max_length=MAX_IMAGE_BATCH_SIZE + ), ) -> StarredImagesResult: try: assert_image_move_maintenance_inactive() @@ -594,9 +660,18 @@ def star_images_in_list( raise try: + # Skip — but do not re-raise — auth failures so a foreign name mid-batch doesn't + # discard the response payload for images that were already starred. Re-raising + # turned partial successes into an error-shaped response, so the client never + # invalidated caches for the images that did change and the UI showed them + # unstarred until the next full refresh. Matches delete_images_from_list and the + # video star/unstar routes. starred_images: set[str] = set() + failed_images: set[str] = set() affected_boards: set[str] = set() - for image_name in image_names: + # Dedup while preserving order — a repeated name would otherwise be processed + # twice and could land in both starred_images and failed_images. + for image_name in dict.fromkeys(image_names): try: _assert_image_owner(image_name, current_user) updated_image_dto = ApiDependencies.invoker.services.images.update( @@ -605,11 +680,21 @@ def star_images_in_list( starred_images.add(image_name) affected_boards.add(updated_image_dto.board_id or "none") except HTTPException: - raise + continue + except ImageRecordNotFoundException: + # Deleted by a concurrent session — a skip, not a storage failure. See + # delete_images_from_list. Reachable here through the get_dto read-back inside + # ImageService.update: the UPDATE itself matches no row and raises nothing, so + # a name that vanished mid-batch surfaces only on the read that follows. + continue except Exception: - pass + # A genuine storage failure, not an auth/404 skip: it used to be swallowed + # by `pass`, so the client counted the image as starred and the star + # silently vanished on reload. + failed_images.add(image_name) return StarredImagesResult( starred_images=list(starred_images), + failed_images=list(failed_images), affected_boards=list(affected_boards), ) except HTTPException: @@ -621,7 +706,9 @@ def star_images_in_list( @images_router.post("/unstar", operation_id="unstar_images_in_list", response_model=UnstarredImagesResult) def unstar_images_in_list( current_user: CurrentUserOrDefault, - image_names: list[str] = Body(description="The list of names of images to unstar", embed=True), + image_names: list[ImageName] = Body( + description="The list of names of images to unstar", embed=True, max_length=MAX_IMAGE_BATCH_SIZE + ), ) -> UnstarredImagesResult: try: assert_image_move_maintenance_inactive() @@ -631,9 +718,12 @@ def unstar_images_in_list( raise try: + # See star_images_in_list: skip foreign names instead of re-raising mid-batch, and + # report genuine storage failures instead of swallowing them. unstarred_images: set[str] = set() + failed_images: set[str] = set() affected_boards: set[str] = set() - for image_name in image_names: + for image_name in dict.fromkeys(image_names): try: _assert_image_owner(image_name, current_user) updated_image_dto = ApiDependencies.invoker.services.images.update( @@ -642,11 +732,15 @@ def unstar_images_in_list( unstarred_images.add(image_name) affected_boards.add(updated_image_dto.board_id or "none") except HTTPException: - raise + continue + except ImageRecordNotFoundException: + # See star_images_in_list. + continue except Exception: - pass + failed_images.add(image_name) return UnstarredImagesResult( unstarred_images=list(unstarred_images), + failed_images=list(failed_images), affected_boards=list(affected_boards), ) except HTTPException: @@ -670,8 +764,11 @@ class ImagesDownloaded(BaseModel): def download_images_from_list( current_user: CurrentUserOrDefault, background_tasks: BackgroundTasks, - image_names: Optional[list[str]] = Body( - default=None, description="The list of names of images to download", embed=True + image_names: Optional[list[ImageName]] = Body( + default=None, + description="The list of names of images to download", + embed=True, + max_length=MAX_IMAGE_BATCH_SIZE, ), board_id: Optional[str] = Body( default=None, description="The board from which image should be downloaded", embed=True @@ -797,7 +894,11 @@ def get_image_names( ) def get_images_by_names( current_user: CurrentUserOrDefault, - image_names: list[str] = Body(embed=True, description="Object containing list of image names to fetch DTOs for"), + image_names: list[ImageName] = Body( + embed=True, + description="Object containing list of image names to fetch DTOs for", + max_length=MAX_IMAGE_BATCH_SIZE, + ), ) -> list[ImageDTO]: """Gets image DTOs for the specified image names. Maintains order of input names.""" diff --git a/invokeai/app/api/routers/videos.py b/invokeai/app/api/routers/videos.py index 0604befb25c..cc8526e6f54 100644 --- a/invokeai/app/api/routers/videos.py +++ b/invokeai/app/api/routers/videos.py @@ -21,7 +21,11 @@ from invokeai.app.services.image_records.image_records_common import ImageCategory, ResourceOrigin from invokeai.app.services.shared.pagination import MAX_PAGE_SIZE, OffsetPaginatedResults from invokeai.app.services.shared.sqlite.sqlite_common import SQLiteDirection -from invokeai.app.services.video_records.video_records_common import VideoNamesResult, VideoRecordChanges +from invokeai.app.services.video_records.video_records_common import ( + VideoNamesResult, + VideoRecordChanges, + VideoRecordNotFoundException, +) from invokeai.app.services.videos.videos_common import ( AddVideosToBoardResult, DeleteVideosResult, @@ -143,7 +147,10 @@ def _assert_board_write_access(board_id: str, current_user: CurrentUserOrDefault def _assert_video_read_access(video_name: str, current_user: CurrentUserOrDefault) -> None: """Raise 403 if the current user may not view the video.""" - from invokeai.app.services.board_records.board_records_common import BoardVisibility + from invokeai.app.services.board_records.board_records_common import ( + BoardRecordNotFoundException, + BoardVisibility, + ) if current_user.is_admin: return @@ -153,13 +160,21 @@ def _assert_video_read_access(video_name: str, current_user: CurrentUserOrDefaul board_id = ApiDependencies.invoker.services.board_video_records.get_board_for_video(video_name) if board_id is not None: + # See `assert_image_read_access`: only a board positively known to be gone may fall + # through to a refusal; a lookup that cannot be decided propagates instead of + # impersonating a permission decision. try: - board = ApiDependencies.invoker.services.boards.get_dto(board_id=board_id) + board = ApiDependencies.invoker.services.board_records.get(board_id) + except BoardRecordNotFoundException: + pass + else: if board.board_visibility in (BoardVisibility.Shared, BoardVisibility.Public): return - except Exception: - pass + # Gone and denied mean opposite things to a client holding a reference to this video, and + # nothing above can tell them apart. See `_assert_image_record_exists`. + if not ApiDependencies.invoker.services.video_records.exists(video_name): + raise HTTPException(status_code=404, detail="Video not found") raise HTTPException(status_code=403, detail="Not authorized to access this video") @@ -477,7 +492,9 @@ def get_video_dto( _assert_video_read_access(video_name, current_user) try: return ApiDependencies.invoker.services.videos.get_dto(video_name) - except Exception: + except VideoRecordNotFoundException: + # See get_image_dto: this is the 404 a workflow's video field drops its reference on, + # so only a genuinely missing record may produce it. raise HTTPException(status_code=404) diff --git a/invokeai/app/services/board_image_records/board_image_records_base.py b/invokeai/app/services/board_image_records/board_image_records_base.py index 561eb79ce5f..34f74868cc1 100644 --- a/invokeai/app/services/board_image_records/board_image_records_base.py +++ b/invokeai/app/services/board_image_records/board_image_records_base.py @@ -20,8 +20,15 @@ def add_image_to_board( def remove_image_from_board( self, image_name: str, - ) -> None: - """Removes an image from a board.""" + board_id: str, + ) -> int: + """Removes an image from the given board. Returns the number of rows removed. + + Scoped to a board on purpose: callers authorize against the board the image is on, and + that decision must not be applied to a different board if the image moves in between. + Zero rows means the scope did not hold — the image left the board between the caller's + read and this write — and the caller must not report a removal that did not happen. + """ pass @abstractmethod diff --git a/invokeai/app/services/board_image_records/board_image_records_sqlite.py b/invokeai/app/services/board_image_records/board_image_records_sqlite.py index 108123c2ca1..0bb95cfad90 100644 --- a/invokeai/app/services/board_image_records/board_image_records_sqlite.py +++ b/invokeai/app/services/board_image_records/board_image_records_sqlite.py @@ -36,15 +36,24 @@ def add_image_to_board( def remove_image_from_board( self, image_name: str, - ) -> None: + board_id: str, + ) -> int: with self._db.transaction() as cursor: + # Scoped to the board the caller was authorized against, not just the image. The + # routes read the image's board, authorize against *that* board, and only then + # remove; an unscoped DELETE would follow the image if it were moved in between, + # applying a decision taken about one board to a different one. cursor.execute( """--sql DELETE FROM board_images - WHERE image_name = ?; + WHERE image_name = ? AND board_id = ?; """, - (image_name,), + (image_name, board_id), ) + # The row count is the only signal that the scope held. Zero rows means the image + # left this board between the caller's read and this write — swallowing that lets + # the route report a removal that did not happen. + return cursor.rowcount def get_images_for_board( self, diff --git a/invokeai/app/services/board_images/board_images_base.py b/invokeai/app/services/board_images/board_images_base.py index 269cebfeaea..e065f96c015 100644 --- a/invokeai/app/services/board_images/board_images_base.py +++ b/invokeai/app/services/board_images/board_images_base.py @@ -20,8 +20,9 @@ def add_image_to_board( def remove_image_from_board( self, image_name: str, - ) -> None: - """Removes an image from a board.""" + board_id: str, + ) -> int: + """Removes an image from the given board. Returns the number of rows removed.""" pass @abstractmethod diff --git a/invokeai/app/services/board_images/board_images_default.py b/invokeai/app/services/board_images/board_images_default.py index b42ae3db031..00b7038f7c8 100644 --- a/invokeai/app/services/board_images/board_images_default.py +++ b/invokeai/app/services/board_images/board_images_default.py @@ -21,8 +21,9 @@ def add_image_to_board( def remove_image_from_board( self, image_name: str, - ) -> None: - self.__invoker.services.board_image_records.remove_image_from_board(image_name) + board_id: str, + ) -> int: + return self.__invoker.services.board_image_records.remove_image_from_board(image_name, board_id) def get_all_board_image_names_for_board( self, diff --git a/invokeai/app/services/board_records/board_records_sqlite.py b/invokeai/app/services/board_records/board_records_sqlite.py index 1e3e11c8a36..b42852d6a29 100644 --- a/invokeai/app/services/board_records/board_records_sqlite.py +++ b/invokeai/app/services/board_records/board_records_sqlite.py @@ -58,20 +58,23 @@ def get( self, board_id: str, ) -> BoardRecord: + # A sqlite3.Error is deliberately NOT translated into BoardRecordNotFoundException. + # Translating it made the exception mean "no such board, OR the database is + # locked/corrupt/unreadable", and callers cannot tell those apart: the batch routes + # decide board write access off this read once per name, and a not-found answer there is + # a benign skip. A disk error would therefore drop names out of the response silently, + # reported neither as moved nor as failed. Mirrors ImageRecordStorage.get. with self._db.transaction() as cursor: - try: - cursor.execute( - """--sql - SELECT * - FROM boards - WHERE board_id = ?; - """, - (board_id,), - ) - - result = cast(Union[sqlite3.Row, None], cursor.fetchone()) - except sqlite3.Error as e: - raise BoardRecordNotFoundException from e + cursor.execute( + """--sql + SELECT * + FROM boards + WHERE board_id = ?; + """, + (board_id,), + ) + + result = cast(Union[sqlite3.Row, None], cursor.fetchone()) if result is None: raise BoardRecordNotFoundException return BoardRecord(**dict(result)) diff --git a/invokeai/app/services/image_records/image_records_base.py b/invokeai/app/services/image_records/image_records_base.py index 8c71dfba9e7..64a530b357a 100644 --- a/invokeai/app/services/image_records/image_records_base.py +++ b/invokeai/app/services/image_records/image_records_base.py @@ -25,6 +25,17 @@ def get(self, image_name: str) -> ImageRecord: """Gets an image record.""" pass + @abstractmethod + def exists(self, image_name: str) -> bool: + """Reports whether the image row is present, without reading it. + + Separate from `get` because the callers that need this are deciding whether something is + gone, and `get` cannot answer that alone: it also deserializes, so a row written by a + newer version -- an enum value this one does not know -- fails the same way a missing row + does. A storage error still propagates, because "could not look" is not "not there". + """ + pass + @abstractmethod def get_metadata(self, image_name: str) -> Optional[MetadataField]: """Gets an image's metadata'.""" diff --git a/invokeai/app/services/image_records/image_records_sqlite.py b/invokeai/app/services/image_records/image_records_sqlite.py index a69bb7005de..2d68967c282 100644 --- a/invokeai/app/services/image_records/image_records_sqlite.py +++ b/invokeai/app/services/image_records/image_records_sqlite.py @@ -28,19 +28,23 @@ def __init__(self, db: SqliteDatabase) -> None: self._db = db def get(self, image_name: str) -> ImageRecord: + # A sqlite3.Error is deliberately NOT translated into ImageRecordNotFoundException. + # This used to be caught and re-raised as not-found, which made the exception mean + # "the row is absent, OR the database is locked/corrupt/unreadable". Callers that + # treat not-found as a benign outcome — the concurrent-deletion skips in the images + # and board_images batch routes — would then swallow a disk I/O error as a routine + # race and answer 200 with the name in no result list at all. Let the storage error + # propagate: ImageService logs it and the route reports it as a real failure. with self._db.transaction() as cursor: - try: - cursor.execute( - f"""--sql - SELECT {IMAGE_DTO_COLS} FROM images - WHERE image_name = ?; - """, - (image_name,), - ) + cursor.execute( + f"""--sql + SELECT {IMAGE_DTO_COLS} FROM images + WHERE image_name = ?; + """, + (image_name,), + ) - result = cast(Optional[sqlite3.Row], cursor.fetchone()) - except sqlite3.Error as e: - raise ImageRecordNotFoundException from e + result = cast(Optional[sqlite3.Row], cursor.fetchone()) if not result: raise ImageRecordNotFoundException @@ -61,21 +65,29 @@ def get_user_id(self, image_name: str) -> Optional[str]: return None return cast(Optional[str], dict(result).get("user_id")) - def get_metadata(self, image_name: str) -> Optional[MetadataField]: + def exists(self, image_name: str) -> bool: with self._db.transaction() as cursor: - try: - cursor.execute( - """--sql - SELECT metadata FROM images - WHERE image_name = ?; - """, - (image_name,), - ) + cursor.execute( + """--sql + SELECT 1 FROM images + WHERE image_name = ?; + """, + (image_name,), + ) + return cursor.fetchone() is not None - result = cast(Optional[sqlite3.Row], cursor.fetchone()) + def get_metadata(self, image_name: str) -> Optional[MetadataField]: + # See get(): a storage error must not masquerade as a missing row. + with self._db.transaction() as cursor: + cursor.execute( + """--sql + SELECT metadata FROM images + WHERE image_name = ?; + """, + (image_name,), + ) - except sqlite3.Error as e: - raise ImageRecordNotFoundException from e + result = cast(Optional[sqlite3.Row], cursor.fetchone()) if not result: raise ImageRecordNotFoundException diff --git a/invokeai/app/services/images/images_common.py b/invokeai/app/services/images/images_common.py index 51679b43f4c..4c96fd825f1 100644 --- a/invokeai/app/services/images/images_common.py +++ b/invokeai/app/services/images/images_common.py @@ -47,23 +47,28 @@ class ResultWithAffectedBoards(BaseModel): class DeleteImagesResult(ResultWithAffectedBoards): deleted_images: list[str] = Field(description="The names of the images that were deleted") - failed_images: list[str] = Field( - default_factory=list, - description="The names of authorized images that could not be deleted", - ) + # Required, not defaulted: the client toasts off this field, and an optional one would reach + # it as `undefined`. Matches StarredImagesResult. + failed_images: list[str] = Field(description="The names of authorized images that could not be deleted") class StarredImagesResult(ResultWithAffectedBoards): starred_images: list[str] = Field(description="The names of the images that were starred") + failed_images: list[str] = Field(description="The names of images that were not starred") class UnstarredImagesResult(ResultWithAffectedBoards): unstarred_images: list[str] = Field(description="The names of the images that were unstarred") + failed_images: list[str] = Field(description="The names of images that were not unstarred") class AddImagesToBoardResult(ResultWithAffectedBoards): added_images: list[str] = Field(description="The image names that were added to the board") + # Required, not defaulted: the client toasts off this field, and an optional one would reach + # it as `undefined`. Matches StarredImagesResult. + failed_images: list[str] = Field(description="The names of authorized images that could not be added") class RemoveImagesFromBoardResult(ResultWithAffectedBoards): removed_images: list[str] = Field(description="The image names that were removed from their board") + failed_images: list[str] = Field(description="The names of authorized images that could not be removed") diff --git a/invokeai/app/services/video_records/video_records_base.py b/invokeai/app/services/video_records/video_records_base.py index 8492e4c4bf0..a8283718388 100644 --- a/invokeai/app/services/video_records/video_records_base.py +++ b/invokeai/app/services/video_records/video_records_base.py @@ -21,6 +21,17 @@ def get(self, video_name: str) -> VideoRecord: """Gets a video record.""" pass + @abstractmethod + def exists(self, video_name: str) -> bool: + """Reports whether the video row is present, without reading it. + + Separate from `get` because the callers that need this are deciding whether something is + gone, and `get` cannot answer that alone: it also deserializes, so a row written by a + newer version -- an enum value this one does not know -- fails the same way a missing row + does. A storage error still propagates, because "could not look" is not "not there". + """ + pass + @abstractmethod def get_metadata(self, video_name: str) -> Optional[MetadataField]: """Gets a video's metadata.""" diff --git a/invokeai/app/services/video_records/video_records_sqlite.py b/invokeai/app/services/video_records/video_records_sqlite.py index 94c943e5321..d7ca569be54 100644 --- a/invokeai/app/services/video_records/video_records_sqlite.py +++ b/invokeai/app/services/video_records/video_records_sqlite.py @@ -26,18 +26,20 @@ def __init__(self, db: SqliteDatabase) -> None: self._db = db def get(self, video_name: str) -> VideoRecord: + # A sqlite3.Error is deliberately NOT translated into VideoRecordNotFoundException, for + # the same reason as SqliteImageRecordStorage.get: it would make the exception mean "the + # row is absent, OR the database is unreadable". `_assert_video_read_access` now answers + # 404 on a positive not-found, and the clients drop their references to a video on that + # 404 — so a locked database would silently clear the user's workflow fields. with self._db.transaction() as cursor: - try: - cursor.execute( - f"""--sql - SELECT {VIDEO_DTO_COLS} FROM videos - WHERE video_name = ?; - """, - (video_name,), - ) - result = cast(Optional[sqlite3.Row], cursor.fetchone()) - except sqlite3.Error as e: - raise VideoRecordNotFoundException from e + cursor.execute( + f"""--sql + SELECT {VIDEO_DTO_COLS} FROM videos + WHERE video_name = ?; + """, + (video_name,), + ) + result = cast(Optional[sqlite3.Row], cursor.fetchone()) if not result: raise VideoRecordNotFoundException @@ -76,6 +78,17 @@ def get_most_recent_video_for_board(self, board_id: str) -> Optional[VideoRecord return None return deserialize_video_record(dict(result)) + def exists(self, video_name: str) -> bool: + with self._db.transaction() as cursor: + cursor.execute( + """--sql + SELECT 1 FROM videos + WHERE video_name = ?; + """, + (video_name,), + ) + return cursor.fetchone() is not None + def get_metadata(self, video_name: str) -> Optional[MetadataField]: with self._db.transaction() as cursor: try: diff --git a/invokeai/frontend/web/openapi.json b/invokeai/frontend/web/openapi.json index c9b8e7349b1..ccd0f318694 100644 --- a/invokeai/frontend/web/openapi.json +++ b/invokeai/frontend/web/openapi.json @@ -4941,6 +4941,7 @@ "required": false, "schema": { "type": "integer", + "minimum": 0, "description": "The page offset", "default": 0, "title": "Offset" @@ -4953,6 +4954,8 @@ "required": false, "schema": { "type": "integer", + "maximum": 1000, + "minimum": 0, "description": "The number of images per page", "default": 10, "title": "Limit" @@ -12642,10 +12645,18 @@ "type": "array", "title": "Added Images", "description": "The image names that were added to the board" + }, + "failed_images": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Failed Images", + "description": "The names of authorized images that could not be added" } }, "type": "object", - "required": ["affected_boards", "added_images"], + "required": ["affected_boards", "added_images", "failed_images"], "title": "AddImagesToBoardResult" }, "AddInvocation": { @@ -16149,9 +16160,11 @@ }, "image_names": { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, "type": "array", + "maxItems": 1000, "title": "Image Names", "description": "The names of the images to add" } @@ -16244,9 +16257,11 @@ "properties": { "image_names": { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, "type": "array", + "maxItems": 1000, "title": "Image Names", "description": "The list of names of images to delete" } @@ -16273,9 +16288,11 @@ "anyOf": [ { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, - "type": "array" + "type": "array", + "maxItems": 1000 }, { "type": "null" @@ -16321,9 +16338,11 @@ "properties": { "image_names": { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, "type": "array", + "maxItems": 1000, "title": "Image Names", "description": "Object containing list of image names to fetch DTOs for" } @@ -16431,9 +16450,11 @@ "properties": { "image_names": { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, "type": "array", + "maxItems": 1000, "title": "Image Names", "description": "The names of the images to remove" } @@ -16471,9 +16492,11 @@ "properties": { "image_names": { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, "type": "array", + "maxItems": 1000, "title": "Image Names", "description": "The list of names of images to star" } @@ -16486,9 +16509,11 @@ "properties": { "image_names": { "items": { - "type": "string" + "type": "string", + "maxLength": 255 }, "type": "array", + "maxItems": 1000, "title": "Image Names", "description": "The list of names of images to unstar" } @@ -24213,7 +24238,7 @@ } }, "type": "object", - "required": ["affected_boards", "deleted_images"], + "required": ["affected_boards", "deleted_images", "failed_images"], "title": "DeleteImagesResult" }, "DeleteOrphanedModelsRequest": { @@ -77444,10 +77469,18 @@ "type": "array", "title": "Removed Images", "description": "The image names that were removed from their board" + }, + "failed_images": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Failed Images", + "description": "The names of authorized images that could not be removed" } }, "type": "object", - "required": ["affected_boards", "removed_images"], + "required": ["affected_boards", "removed_images", "failed_images"], "title": "RemoveImagesFromBoardResult" }, "RemoveVideosFromBoardResult": { @@ -82180,10 +82213,18 @@ "type": "array", "title": "Starred Images", "description": "The names of the images that were starred" + }, + "failed_images": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Failed Images", + "description": "The names of images that were not starred" } }, "type": "object", - "required": ["affected_boards", "starred_images"], + "required": ["affected_boards", "starred_images", "failed_images"], "title": "StarredImagesResult" }, "StarredVideosResult": { @@ -86935,10 +86976,18 @@ "type": "array", "title": "Unstarred Images", "description": "The names of the images that were unstarred" + }, + "failed_images": { + "items": { + "type": "string" + }, + "type": "array", + "title": "Failed Images", + "description": "The names of images that were not unstarred" } }, "type": "object", - "required": ["affected_boards", "unstarred_images"], + "required": ["affected_boards", "unstarred_images", "failed_images"], "title": "UnstarredImagesResult" }, "UnstarredVideosResult": { diff --git a/invokeai/frontend/web/public/locales/en.json b/invokeai/frontend/web/public/locales/en.json index 6c87e0477aa..93127f422b1 100644 --- a/invokeai/frontend/web/public/locales/en.json +++ b/invokeai/frontend/web/public/locales/en.json @@ -2054,6 +2054,11 @@ "imageSavingFailed": "Image Saving Failed", "imageUploaded": "Image Uploaded", "imageUploadFailed": "Image Upload Failed", + "downloadsInterrupted": "Your session ended while downloads were being prepared. Run the download again after signing in.", + "imagesFailedToDownload": "{{count}} image could not be prepared for download.", + "imagesFailedToDownload_other": "{{count}} images could not be prepared for download.", + "imagesFailedToUpdate": "{{count}} image could not be updated.", + "imagesFailedToUpdate_other": "{{count}} images could not be updated.", "imageStorageMaintenanceActive": "Image storage maintenance is active. Recover it before retrying the upload.", "videoUploaded": "Video Uploaded", "videoUploadFailed": "Video Upload Failed", diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.test.ts b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.test.ts new file mode 100644 index 00000000000..823b7a4c402 --- /dev/null +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.test.ts @@ -0,0 +1,45 @@ +import { toast } from 'features/toast/toast'; +import { describe, expect, it, vi } from 'vitest'; + +import { addBulkDownloadListeners } from './bulkDownload'; + +vi.mock('features/toast/toast', () => ({ toast: vi.fn() })); +vi.mock('i18next', () => ({ t: vi.fn((key: string) => key) })); + +/** Collects the effects the listeners register, in registration order. */ +const collectEffects = () => { + const effects: ((action: { payload: unknown }) => void)[] = []; + const startAppListening = vi.fn(({ effect }: { effect: (action: { payload: unknown }) => void }) => { + effects.push(effect); + }); + /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ + addBulkDownloadListeners(startAppListening as any); + return effects; +}; + +describe('bulk download listeners', () => { + it('raises no toast for a fulfilled action with no payload', () => { + // `fetchBaseQuery` resolves an empty response entity as `data: null`, so a proxy that + // strips the body off the 202 fulfils this action with nothing in it. Dereferencing the + // payload would throw — but raising the toast anyway is no better: it is persistent + // (`duration: null`) and is dismissed by name when the zip lands, so without a name it + // gets a random id that the socket handler's close call can never match, and the banner + // stays on screen forever. The download is unaffected; its completion toast still arrives. + const [onFulfilled] = collectEffects(); + + expect(() => onFulfilled?.({ payload: null })).not.toThrow(); + expect(toast).not.toHaveBeenCalled(); + }); + + it('keys the preparing toast on the item name when there is one', () => { + // Distinct ids matter: the background task can finish in under 20ms, so the "ready" + // toast may already be on screen when this one is raised. + const [onFulfilled] = collectEffects(); + + onFulfilled?.({ payload: { bulk_download_item_name: 'item-1.zip', response: 'on its way' } }); + + expect(toast).toHaveBeenCalledWith( + expect.objectContaining({ id: 'preparing:item-1.zip', description: 'on its way' }) + ); + }); +}); diff --git a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.tsx b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.tsx index fa4c29b8f42..1cfdc0dd7d4 100644 --- a/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.tsx +++ b/invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.tsx @@ -17,13 +17,26 @@ export const addBulkDownloadListeners = (startAppListening: AppStartListening) = // socket event. The background task can complete in under 20ms, so the // socket event may arrive *before* this Redux middleware runs — without // distinct IDs the "preparing" toast would overwrite the "ready" toast. - const itemName = action.payload.bulk_download_item_name; + // Read through optionals: the payload is typed as a model, but `fetchBaseQuery` resolves + // an empty response entity as `data: null`, so a proxy that strips the body off the 202 + // fulfils this action with nothing in it. + const itemName = action.payload?.bulk_download_item_name; + if (!itemName) { + // Nothing to key the toast on, so there must be no toast. This one is raised with + // `duration: null` and is dismissed by name when the zip arrives + // (`toastApi.close(\`preparing:${name}\`)` in setEventListeners); raising it without an + // id gets it a random one instead, which that close call can never match — a permanent + // "preparing your download" banner for a download that has already landed. The + // download itself is unaffected: it was scheduled server-side, and its completion + // toast arrives over the socket. + return; + } toast({ - id: itemName ? `preparing:${itemName}` : undefined, + id: `preparing:${itemName}`, title: t('gallery.bulkDownloadRequested'), status: 'success', // Show the response message if it exists, otherwise show the default message - description: action.payload.response || t('gallery.bulkDownloadRequestedDesc'), + description: action.payload?.response || t('gallery.bulkDownloadRequestedDesc'), duration: null, }); }, diff --git a/invokeai/frontend/web/src/app/store/store.test.ts b/invokeai/frontend/web/src/app/store/store.test.ts index e98155b8aee..d849e710103 100644 --- a/invokeai/frontend/web/src/app/store/store.test.ts +++ b/invokeai/frontend/web/src/app/store/store.test.ts @@ -1,8 +1,18 @@ import { Buffer } from 'node:buffer'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; -import { externalTokenAdopted, logout, sessionExpiredLogout, setCredentials } from 'features/auth/store/authSlice'; +import { + externalTokenAdopted, + logout, + sessionExpiredLogout, + setCredentials, + staleCredentialsDiscarded, +} from 'features/auth/store/authSlice'; import { isModalOpenChanged, videosToChangeSelected } from 'features/changeBoardModal/store/slice'; +import { bboxHeightChanged, bboxWidthChanged, canvasUndo } from 'features/controlLayers/store/canvasSlice'; import { positivePromptChanged } from 'features/controlLayers/store/paramsSlice'; +import { refImageAdded } from 'features/controlLayers/store/refImagesSlice'; import { deleteVideosWithDialog } from 'features/deleteVideoModal/store/state'; import { $gallerySelection, @@ -15,9 +25,11 @@ import { imageSelected, selectionChanged, } from 'features/gallery/store/gallerySlice'; +import { undo as nodesUndo, workflowNameChanged } from 'features/nodes/store/nodesSlice'; +import { upscaleInitialImageChanged } from 'features/parameters/store/upscaleSlice'; import { appInfoApi } from 'services/api/endpoints/appInfo'; import type { S } from 'services/api/types'; -import { describe, expect, it } from 'vitest'; +import { beforeAll, describe, expect, it, vi } from 'vitest'; import { createStore } from './store'; @@ -34,6 +46,12 @@ const user = { is_active: true, }; +// The canvas undo filter throttles rapid same-type actions with a `window.setTimeout` reset +// timer, and this file runs in the node environment. Only that one API is needed. +beforeAll(() => { + vi.stubGlobal('window', { setTimeout }); +}); + const tokenFor = (userId: string) => `header.${Buffer.from(JSON.stringify({ user_id: userId })).toString('base64url')}.signature`; @@ -97,6 +115,116 @@ describe('auth cache isolation', () => { expect(store.getState().params.positivePrompt).toBe(''); }); + it.each([ + ['a deliberate logout', () => logout()], + ['a cross-tab account switch', () => externalTokenAdopted(tokenFor('other-user'))], + ])('clears the workspace slices and their undo stacks on %s', (_label, makeAction) => { + // Both account-change paths — a same-tab logout and another tab's foreign token — must + // wipe the workspace: it is personal state, and it is where deleted-image references live + // (raster/control layers, node image fields, reference images), so a cross-user batch + // delete aborted mid-run must not leave the next account holding references to images + // that no longer exist. The undo assertions are the half that is easy to lose: the + // undoable filters keep cross-slice actions out of history without emptying it, so a + // reset alone leaves the previous account's states one ctrl+Z away. The adoption case + // must work at the reducer level — the synthetic logout it triggers never reaches + // listeners, so a listener-based clear would pass the logout case and fail this one. + const store = createStore(); + store.dispatch(setCredentials({ token: tokenFor(user.user_id), user })); + const initialCanvas = store.getState().canvas.present; + const initialNodes = store.getState().nodes.present; + store.dispatch(bboxWidthChanged({ width: initialCanvas.bbox.rect.width + 64 })); + store.dispatch(bboxHeightChanged({ height: initialCanvas.bbox.rect.height + 64 })); + store.dispatch(workflowNameChanged('previous user workflow')); + store.dispatch(refImageAdded()); + store.dispatch(upscaleInitialImageChanged({ image_name: 'previous-user.png', width: 64, height: 64 })); + expect(store.getState().canvas.present).not.toEqual(initialCanvas); + expect(store.getState().nodes.present).not.toEqual(initialNodes); + expect(store.getState().refImages.entities).toHaveLength(1); + + store.dispatch(makeAction()); + + expect(store.getState().canvas.present).toEqual(initialCanvas); + expect(store.getState().nodes.present).toEqual(initialNodes); + expect(store.getState().refImages.entities).toHaveLength(0); + expect(store.getState().upscale.upscaleInitialImage).toBeNull(); + // The stacks are asserted directly as well as behaviorally: with few seeded actions an + // undo's target can coincide with the initial state, and the behavioral check alone would + // stay green with the clear missing. + expect(store.getState().canvas.past).toHaveLength(0); + expect(store.getState().nodes.past).toHaveLength(0); + store.dispatch(canvasUndo()); + store.dispatch(nodesUndo()); + expect(store.getState().canvas.present).toEqual(initialCanvas); + expect(store.getState().nodes.present).toEqual(initialNodes); + // The clears must land *after* the reset pass, and this is the assertion that pins the + // order: clears that run first leave the filtered reset as redux-undo's _latestUnfiltered, + // so the next account's first action pushes the previous account's state into past — one + // action and one ctrl+Z resurrect it. Dispatch as the new account, undo, and the state + // must come back to the *reset*, not to what was seeded above. + store.dispatch(bboxWidthChanged({ width: initialCanvas.bbox.rect.width + 128 })); + store.dispatch(workflowNameChanged('next user workflow')); + store.dispatch(canvasUndo()); + store.dispatch(nodesUndo()); + expect(store.getState().canvas.present).toEqual(initialCanvas); + expect(store.getState().nodes.present).toEqual(initialNodes); + }); + + it.each([ + ['the session merely expires', () => sessionExpiredLogout()], + ['another tab refreshes the same user token', () => externalTokenAdopted(tokenFor(user.user_id))], + ['stale multiuser credentials are discarded on a single-user switch', () => staleCredentialsDiscarded()], + ])('keeps the whole workspace when %s', (_label, makeAction) => { + // None of these is an account change. A timeout's user is coming back, and wiping hours of + // canvas or workflow work over it would be destructive — deleted-image references under + // expiry are handled by the batch loops resolving with partial data so `handleDeletions` + // can prune. A same-user token refresh changes nothing at all. And the single-user mode + // switch keeps the same human at the machine — worse, in single-user mode the + // unauthenticated persist is accepted, so a wipe there would overwrite the stored + // workspace for good. Asserted across every purged slice, not just one: each slice decides + // independently which actions it resets on, so a single-slice probe cannot see one of the + // others going over-eager. + const store = createStore(); + store.dispatch(setCredentials({ token: tokenFor(user.user_id), user })); + store.dispatch(workflowNameChanged('my unsaved workflow')); + store.dispatch(bboxWidthChanged({ width: 1024 })); + store.dispatch(refImageAdded()); + store.dispatch(upscaleInitialImageChanged({ image_name: 'mine.png', width: 64, height: 64 })); + + store.dispatch(makeAction()); + + expect(store.getState().nodes.present.name).toBe('my unsaved workflow'); + expect(store.getState().canvas.present.bbox.rect.width).toBe(1024); + expect(store.getState().refImages.entities).toHaveLength(1); + expect(store.getState().upscale.upscaleInitialImage?.image_name).toBe('mine.png'); + }); + + it('discards stale credentials on a mode switch without the account-change action', () => { + // A source guard in the manner of ChangeBoardModal.test.ts: the store tests above exercise + // the actions, but nothing else pins WHICH action the mode-switch branch dispatches. + // Reverting it to `logout()` re-arms the account-change wipe on a path where the same + // human keeps the machine — and where single-user mode persists the wipe over their stored + // workspace. `logout()` belongs to UserMenu alone. + const source = readFileSync( + fileURLToPath(new URL('../../features/auth/components/ProtectedRoute.tsx', import.meta.url)), + 'utf8' + ); + expect(source).toContain('dispatch(staleCredentialsDiscarded())'); + expect(source).not.toContain('dispatch(logout())'); + }); + + it('still clears credentials and the api cache when stale multiuser credentials are discarded', async () => { + const store = createStore(); + store.dispatch(setCredentials({ token: tokenFor(user.user_id), user })); + await store.dispatch(appInfoApi.util.upsertQueryData('getRuntimeConfig', undefined, runtimeConfig)); + + store.dispatch(staleCredentialsDiscarded()); + + expect(store.getState().auth.token).toBeNull(); + expect(store.getState().auth.isAuthenticated).toBe(false); + // The cache was fetched under multiuser visibility scoping, so it does not carry over. + expect(appInfoApi.endpoints.getRuntimeConfig.select()(store.getState()).data).toBeUndefined(); + }); + it.each([ ['logout', logout], ['session expiry', sessionExpiredLogout], @@ -108,6 +236,7 @@ describe('auth cache isolation', () => { store.dispatch(logOut()); expect(store.getState().changeBoardModal).toMatchObject({ + operation_id: 2, isModalOpen: false, image_names: [], video_names: [], diff --git a/invokeai/frontend/web/src/app/store/store.ts b/invokeai/frontend/web/src/app/store/store.ts index ea57993bf4c..a96dc156981 100644 --- a/invokeai/frontend/web/src/app/store/store.ts +++ b/invokeai/frontend/web/src/app/store/store.ts @@ -32,11 +32,12 @@ import { externalTokenAdopted, logout, sessionExpiredLogout, + staleCredentialsDiscarded, tokensBelongToSameUser, } from 'features/auth/store/authSlice'; -import { changeBoardModalSliceConfig, changeBoardReset } from 'features/changeBoardModal/store/slice'; +import { changeBoardModalSliceConfig, changeBoardOperationInvalidated } from 'features/changeBoardModal/store/slice'; import { canvasSettingsSliceConfig } from 'features/controlLayers/store/canvasSettingsSlice'; -import { canvasSliceConfig } from 'features/controlLayers/store/canvasSlice'; +import { canvasClearHistory, canvasSliceConfig } from 'features/controlLayers/store/canvasSlice'; import { canvasSessionSliceConfig } from 'features/controlLayers/store/canvasStagingAreaSlice'; import { canvasTextSliceConfig } from 'features/controlLayers/store/canvasTextSlice'; import { canvasWorkflowIntegrationSliceConfig } from 'features/controlLayers/store/canvasWorkflowIntegrationSlice'; @@ -60,7 +61,7 @@ import { uiSliceConfig } from 'features/ui/store/uiSlice'; import { diff } from 'jsondiffpatch'; import type { SerializeFunction, UnserializeFunction } from 'redux-remember'; import { REMEMBER_REHYDRATED, rememberEnhancer, rememberReducer } from 'redux-remember'; -import undoable, { newHistory } from 'redux-undo'; +import undoable, { ActionCreators, newHistory } from 'redux-undo'; import { serializeError } from 'serialize-error'; import { api } from 'services/api'; import type { JsonObject } from 'type-fest'; @@ -147,10 +148,24 @@ const rootReducer = combineReducers(ALL_REDUCERS); type RootReducerState = ReturnType; const accountAwareRootReducer = (state: RootReducerState | undefined, action: UnknownAction): RootReducerState => { - if (state && externalTokenAdopted.match(action) && !tokensBelongToSameUser(state.auth.token, action.payload)) { + const isForeignAdoption = + !!state && externalTokenAdopted.match(action) && !tokensBelongToSameUser(state.auth.token, action.payload); + if (isForeignAdoption) { state = rootReducer(state, logout()); } - return rootReducer(state, action); + state = rootReducer(state, action); + // A logout pass resets each workspace slice's *present* through its own `logout` case, but + // both undoable slices filter cross-slice actions out of history without emptying it — the + // stacks keep the previous account's states, and ctrl+Z would walk the next account straight + // back into them. The clears ride the same reducer call rather than the logout listener + // because the synthetic pass above (cross-tab adoption) never reaches listeners; this is the + // only spot both paths share. Two actions because canvas overrides `clearHistoryType` while + // nodes uses redux-undo's default — which also covers any undoable slice added later. + if (isForeignAdoption || logout.match(action)) { + state = rootReducer(state, canvasClearHistory()); + state = rootReducer(state, ActionCreators.clearHistory()); + } + return state; }; const rememberedRootReducer = rememberReducer(accountAwareRootReducer); @@ -286,10 +301,10 @@ export const addAppListener = addListener.withTypes(); const startAppListening = listenerMiddleware.startListening as AppStartListening; startAppListening({ - matcher: isAnyOf(logout, sessionExpiredLogout), + matcher: isAnyOf(logout, sessionExpiredLogout, staleCredentialsDiscarded), effect: (_action, { dispatch }) => { dispatch(api.util.resetApiState()); - dispatch(changeBoardReset()); + dispatch(changeBoardOperationInvalidated()); cancelDeletion(); }, }); @@ -301,7 +316,7 @@ startAppListening({ return; } dispatch(api.util.resetApiState()); - dispatch(changeBoardReset()); + dispatch(changeBoardOperationInvalidated()); cancelDeletion(); }, }); diff --git a/invokeai/frontend/web/src/features/auth/components/ProtectedRoute.tsx b/invokeai/frontend/web/src/features/auth/components/ProtectedRoute.tsx index d8ffaffe3bd..c82d355429e 100644 --- a/invokeai/frontend/web/src/features/auth/components/ProtectedRoute.tsx +++ b/invokeai/frontend/web/src/features/auth/components/ProtectedRoute.tsx @@ -1,7 +1,14 @@ import { Center, Spinner } from '@invoke-ai/ui-library'; +import { skipToken } from '@reduxjs/toolkit/query'; import type { RootState } from 'app/store/store'; import { useAppDispatch, useAppSelector } from 'app/store/storeHooks'; -import { externalTokenAdopted, logout, sessionExpiredLogout, setCredentials } from 'features/auth/store/authSlice'; +import { + externalTokenAdopted, + sessionExpiredLogout, + setCredentials, + staleCredentialsDiscarded, +} from 'features/auth/store/authSlice'; +import { shouldEndSessionForUnauthorized } from 'features/auth/store/authTokenRefresh'; import type { PropsWithChildren } from 'react'; import { memo, useEffect } from 'react'; import { useNavigate } from 'react-router-dom'; @@ -28,7 +35,7 @@ export const ProtectedRoute = memo(({ children, requireAdmin = false }: PropsWit data: currentUser, isLoading: isLoadingUser, error: userError, - } = useGetCurrentUserQuery(undefined, { + } = useGetCurrentUserQuery(token ?? skipToken, { skip: !shouldFetchUser, }); @@ -36,11 +43,27 @@ export const ProtectedRoute = memo(({ children, requireAdmin = false }: PropsWit // Only treat 401 as session expiry. Other errors (500, network, etc.) are // transient and should not force logout — the 401 handler in dynamicBaseQuery // already covers the actual expiry case. + // + // And only while this tab's token is still the live one. `sessionExpiredLogout` removes + // `auth_token` from localStorage, which is SHARED across tabs, so acting on a 401 that + // belongs to a superseded session deletes the credential of the session that replaced it: + // this query goes out during page load carrying an expired token, another tab logs in + // meanwhile, and this 401 lands before the adoption poll has run. `dynamicBaseQuery` + // already declines to end the session in that case; this is the same decision, and both + // ask `shouldEndSessionForUnauthorized`. + // + // That check alone would only postpone it, because the store's token catches up when the + // poll adopts the new one and this effect re-runs. What stops it then is the query being + // keyed by token: the adopted session reads its own cache entry, so the superseded 401 is + // no longer in hand to act on. See `getCurrentUser`. if (userError && isAuthenticated && 'status' in userError && userError.status === 401) { + if (!shouldEndSessionForUnauthorized(token ?? null)) { + return; + } dispatch(sessionExpiredLogout()); navigate('/login', { replace: true }); } - }, [userError, isAuthenticated, dispatch, navigate]); + }, [userError, isAuthenticated, token, dispatch, navigate]); // Detect when auth_token is removed from localStorage (e.g. by another tab, // browser devtools, or token expiry cleanup). The 'storage' event fires when @@ -91,9 +114,12 @@ export const ProtectedRoute = memo(({ children, requireAdmin = false }: PropsWit useEffect(() => { // If multiuser is disabled, allow access without authentication if (!multiuserEnabled) { - // Clear any persisted auth state when switching to single-user mode + // Discard the leftover auth state when switching to single-user mode. Deliberately not + // `logout()`: that is the account-change action, and the workspace slices reset on it — + // a mode switch keeps the same human at the machine, and in single-user mode the wipe + // would persist over their stored canvas and workflows. See staleCredentialsDiscarded. if (isAuthenticated) { - dispatch(logout()); + dispatch(staleCredentialsDiscarded()); } return; } diff --git a/invokeai/frontend/web/src/features/auth/store/authSlice.ts b/invokeai/frontend/web/src/features/auth/store/authSlice.ts index 1daab4a07f2..353b7ab9730 100644 --- a/invokeai/frontend/web/src/features/auth/store/authSlice.ts +++ b/invokeai/frontend/web/src/features/auth/store/authSlice.ts @@ -104,6 +104,26 @@ const authSlice = createSlice({ localStorage.removeItem('auth_token'); } }, + /** + * Discards leftover credentials without the account-change semantics of `logout`. The one + * caller is ProtectedRoute's multiuser-disabled branch: the server has switched to + * single-user mode and a token from the multiuser era is still lying around. That is a mode + * switch, not a hand-off to another person — the same human keeps the machine — so the + * workspace slices, which reset on `logout` to keep one account's canvas and workflow away + * from the next, must not fire. They would not merely flash empty: in single-user mode the + * unauthenticated persist is accepted, so the wipe would overwrite the stored workspace for + * good. The store listener still clears the api cache on this action, since what is cached + * was fetched under multiuser visibility scoping. + */ + staleCredentialsDiscarded: (state) => { + state.token = null; + state.user = null; + state.isAuthenticated = false; + state.sessionExpired = false; + if (typeof window !== 'undefined' && window.localStorage) { + localStorage.removeItem('auth_token'); + } + }, sessionExpiredLogout: (state) => { state.token = null; state.user = null; @@ -126,6 +146,7 @@ export const { currentUserUpdated, logout, sessionExpiredLogout, + staleCredentialsDiscarded, setLoading, } = authSlice.actions; diff --git a/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.test.ts b/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.test.ts index ed8530ba217..e1e99c97792 100644 --- a/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.test.ts +++ b/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.test.ts @@ -6,6 +6,7 @@ import { createMediaAuthLock, runWithMediaAuthLock, shouldAcceptRefreshedToken, + shouldEndSessionForUnauthorized, } from './authTokenRefresh'; describe('refreshed token acceptance', () => { @@ -117,3 +118,39 @@ describe('refreshed token acceptance', () => { await expect(lock(() => 'next write')).resolves.toBe('next write'); }); }); + +describe('ending a session over a 401', () => { + beforeEach(() => { + localStorage.clear(); + }); + + it('ends it when the token that got the 401 is still the live one', () => { + localStorage.setItem('auth_token', 'token-a'); + + expect(shouldEndSessionForUnauthorized('token-a')).toBe(true); + }); + + it('spares the session that replaced the one the 401 belongs to', () => { + // Someone else took the tab over while the request was in flight -- here, or in another + // tab, since localStorage is shared. Their token must survive a stranger's 401. + localStorage.setItem('auth_token', 'token-b'); + + expect(shouldEndSessionForUnauthorized('token-a')).toBe(false); + }); + + it('spares a session whose token a sliding-window refresh replaced', () => { + // Byte equality, not `isSameAuthContext`: the refreshed token is the same login, but a 401 + // for the token it replaced says nothing about it. The next request settles the question. + localStorage.setItem('auth_token', 'token-a-refreshed'); + + expect(shouldEndSessionForUnauthorized('token-a')).toBe(false); + }); + + it('ignores a 401 for a request that carried no credential', () => { + // Both forms `dynamicBaseQuery` can hold: no token at all, and the empty string, which + // sets no Authorization header yet compares equal to itself once stored. + expect(shouldEndSessionForUnauthorized(null)).toBe(false); + localStorage.setItem('auth_token', ''); + expect(shouldEndSessionForUnauthorized('')).toBe(false); + }); +}); diff --git a/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.ts b/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.ts index ba23a73f9c8..15f5be32850 100644 --- a/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.ts +++ b/invokeai/frontend/web/src/features/auth/store/authTokenRefresh.ts @@ -1,3 +1,5 @@ +import { tokensBelongToSameUser } from 'features/auth/store/authSlice'; + const AUTH_GENERATION_KEY = 'auth_generation'; const MEDIA_AUTH_LOCK = 'invokeai-media-auth'; const FALLBACK_LOCK_PREFIX = `${MEDIA_AUTH_LOCK}:`; @@ -43,6 +45,70 @@ export const beginAuthTransition = () => { export const shouldAcceptRefreshedToken = (requestToken: string, requestGeneration: number) => getAuthGeneration() === requestGeneration && localStorage.getItem('auth_token') === requestToken; +/** + * True when a 401 for a request that carried `requestToken` should end the live session. + * + * A 401 is evidence about the credential that was *sent*, and about nothing else. By the time + * one lands, the tab may already belong to someone else: `setCredentials` (a login here) and + * `externalTokenAdopted` (a login in another tab — localStorage is shared) both swap the token + * synchronously while earlier requests are still in flight. Ending the session on such a 401 + * destroys the session that just replaced it, and the user who never issued the request is the + * one logged out. So the token that was sent must still be the live one. + * + * Byte equality, deliberately — and note this is the opposite of what `isSameAuthContext` wants. + * A sliding-window refresh must NOT pass here: it mints a new token for the same login, and a + * 401 for the token it replaced says nothing about whether the replacement is still good. That + * costs nothing, because the session ending is not a one-shot event to be caught — if it really + * has ended, the next request carries the live token and its 401 ends it here. + * + * A falsy `requestToken` never qualifies: unauthenticated requests (client_state probes during + * page load, the setup-status query) 401 routinely and must not log anyone out. Truthiness + * rather than a null check, to match the condition `dynamicBaseQuery` actually sends the header + * under — an empty-string token sets no `Authorization` header, so its 401 proves nothing about + * any session, yet it is equal to the stored empty string. + */ +export const shouldEndSessionForUnauthorized = (requestToken: string | null): boolean => + !!requestToken && localStorage.getItem('auth_token') === requestToken; + +/** The session an operation started under. See `isSameAuthContext`. */ +export type AuthContext = { + token: string | null; +}; + +export const captureAuthContext = (): AuthContext => ({ + token: localStorage.getItem('auth_token'), +}); + +/** + * True while the session an operation began under is still the live one. + * + * Requests read the bearer token out of localStorage at send time (`dynamicBaseQuery`), so an + * operation that issues several of them does not carry one identity: log out and back in as + * someone else midway and the remaining requests go out as the new user. Nothing stops that on + * its own — RTK Query's `resetApiState` clears the store, not a `queryFn` that is already + * running. Callers that fan a single user action out into a sequence of requests must therefore + * capture the context up front and check it before each one. + * + * The comparison is on the identity the token carries, not on the token itself and not on the + * auth generation counter: + * - Not the bytes, because the sliding-window refresh mints a fresh token for the same login + * mid-operation, and aborting on that would abandon a batch for a routine event. + * - Not the generation counter, because `beginAuthTransition` bumps it when a login or logout + * *request is sent*, before anything has changed and regardless of whether it succeeds. A + * login in a second tab — even one that fails, or one that signs the same user back in — + * would abort an unrelated batch in this one. Identity answers the question the counter was + * standing in for, and answers it correctly: whoever the next chunk would be sent as is read + * fresh from localStorage every time. + * + * `sessionExpiredLogout` removes the token with no request at all, so the token comparison is + * what catches it. Two nulls compare equal, which keeps deployments that never issue a token — + * the single-user default — out of this entirely. + */ +export const isSameAuthContext = (context: AuthContext): boolean => { + const token = localStorage.getItem('auth_token'); + return token === context.token || tokensBelongToSameUser(context.token, token); +}; + const getFallbackLockTickets = (): FallbackLockTicket[] => { const tickets: FallbackLockTicket[] = []; const now = Date.now(); diff --git a/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.test.ts b/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.test.ts new file mode 100644 index 00000000000..1a88d7f71cc --- /dev/null +++ b/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.test.ts @@ -0,0 +1,153 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +import { + canRetainFailedSelection, + changeBoardModalSliceConfig, + changeBoardOperationInvalidated, + changeBoardReset, + imagesToChangeSelected, +} from 'features/changeBoardModal/store/slice'; +import { describe, expect, it } from 'vitest'; + +describe('canRetainFailedSelection', () => { + const unclaimed = { operation_id: 3, isModalOpen: false, image_names: [], video_names: [] }; + + it('allows the write-back when nothing has claimed the modal since', () => { + expect(canRetainFailedSelection(unclaimed, 3, true)).toBe(true); + }); + + it('refuses once another selection has claimed the modal', () => { + // Right-click a different image while a large move is in flight: the dialog is open again + // with that one name in it. Overwriting it with the earlier request's failures would move + // a set the user never chose, to the board they picked for something else. + expect(canRetainFailedSelection({ ...unclaimed, isModalOpen: true }, 3, true)).toBe(false); + expect(canRetainFailedSelection({ ...unclaimed, image_names: ['other.png'] }, 3, true)).toBe(false); + expect(canRetainFailedSelection({ ...unclaimed, video_names: ['other.mp4'] }, 3, true)).toBe(false); + }); + + it('refuses after a newer selection was opened and canceled', () => { + expect(canRetainFailedSelection(unclaimed, 4, true)).toBe(false); + }); + + it('refuses once the session has ended', () => { + // The logout listener clears this slice along with the api state; re-seeding it afterwards + // leaves one user's image names in the next user's store. + expect(canRetainFailedSelection(unclaimed, 3, false)).toBe(false); + }); +}); + +describe('change board operation ownership', () => { + it('advances on selection but preserves ownership across the accept reset', () => { + const reducer = changeBoardModalSliceConfig.slice.reducer; + const selected = reducer(undefined, imagesToChangeSelected(['first.png'])); + const reset = reducer(selected, changeBoardReset()); + const newer = reducer(reset, imagesToChangeSelected(['second.png'])); + const invalidated = reducer(newer, changeBoardOperationInvalidated()); + + expect(selected.operation_id).toBe(1); + expect(reset.operation_id).toBe(1); + expect(newer.operation_id).toBe(2); + expect(invalidated.operation_id).toBe(3); + expect(invalidated.image_names).toEqual([]); + }); +}); + +/** + * A source-level guard, in the manner of videoReviewRegressions: this repo does not do UI + * tests, and what is being guarded here is an ordering property rather than a value. Accepting + * the dialog resets the selection on the way out (ConfirmationAlertDialog calls acceptCallback + * and then onClose), so an image mutation that is fired and forgotten has its `failed_images` + * cleared away with everything else — the images that did not move leave no trace to retry from. + */ +describe('ChangeBoardModal', () => { + const source = readFileSync(fileURLToPath(new URL('./ChangeBoardModal.tsx', import.meta.url)), 'utf8'); + + it('awaits the image board mutation instead of firing and forgetting it', () => { + expect(source).toMatch(/addImagesToBoard\([\s\S]*?\.unwrap\(\)/); + expect(source).toContain('result.failed_images'); + expect(source).toMatch(/failedImageNames\.length === 0[\s\S]*changeBoardReset/); + // Every late write goes through the guard, the reset included — it can clear a selection + // that now belongs to someone else just as easily as the retain can overwrite one. + expect(source).toMatch(/canRetainFailedSelection\([\s\S]*?return;[\s\S]*changeBoardReset/); + }); + + it('keeps the images that did not move selected', () => { + expect(source).toContain('imagesToChangeSelected(failedImageNames)'); + // A rejected request moved nothing at all, so the whole request stays selected. + expect(source).toContain('.catch(() => imagesToChange)'); + }); + + // Both reopens are matched adjacently rather than through an unbounded `[\s\S]*`. This file + // holds two of them, so a gap-matching pattern is satisfied by the *other* one further down: + // deleting the image reopen leaves such an assertion green, which is the half that matters + // most here, image batches being the subject of these routes. + it('reopens with the failed images selected for retry', () => { + expect(source).toMatch(/imagesToChangeSelected\(failedImageNames\)\);\s*dispatch\(isModalOpenChanged\(true\)\);/); + }); + + it('reopens with the failed videos selected for retry', () => { + expect(source).toMatch(/videosToChangeSelected\(failedVideoNames\)\);\s*dispatch\(isModalOpenChanged\(true\)\);/); + }); + + it('reopens only on the far side of the guard', () => { + // Adjacency alone does not place them: a reopen moved above `canRetainFailedSelection` keeps + // both patterns above intact while popping the dialog open on a selection that may belong to + // someone else, seeded with the previous session's names. Neither reopen may precede it. + const guard = source.indexOf('canRetainFailedSelection('); + const firstReopen = source.indexOf('isModalOpenChanged(true)'); + + expect(guard).toBeGreaterThan(-1); + expect(firstReopen).toBeGreaterThan(-1); + expect(firstReopen).toBeGreaterThan(guard); + }); + + it('does not carry a hidden board target into the reopened dialog', () => { + // `selectedBoardId` is component state the accept does not reset, and `options` drops the + // board being viewed — so a reopen can show the placeholder while still armed for the old + // target. Cleared before either reopen so the combobox and Move agree. + const clear = source.indexOf('setSelectedBoardId(null)'); + const firstReopen = source.indexOf('isModalOpenChanged(true)'); + + expect(clear).toBeGreaterThan(-1); + expect(clear).toBeLessThan(firstReopen); + }); + + it('captures the operation id before awaiting the move, not after it', () => { + // The whole ownership check rests on this ordering. Read after the await, the id is + // whatever the slice holds once every newer selection has already come and gone, so the + // guard compares the current value against itself and admits every stale operation -- + // silently, since it still typechecks and every other assertion here still passes. + const capture = source.indexOf('const operationId ='); + const settle = source.indexOf('await Promise.all'); + + expect(capture).toBeGreaterThan(-1); + expect(settle).toBeGreaterThan(-1); + expect(capture).toBeLessThan(settle); + // And that it is the captured constant the guard is handed, not the slice re-read a second + // time: passing today's value as both operands leaves the ordering above intact and still + // admits every stale operation. + expect(source).toMatch( + /canRetainFailedSelection\(\s*selectChangeBoardModalSlice\(store\.getState\(\)\),\s*operationId,/ + ); + }); + + it('reports failed video moves ahead of the ownership guard', () => { + // This toast is the only failure report the video board routes have: no onQueryStarted, no + // matchRejected listener, unlike the image batch routes. Behind the guard, opening and + // cancelling any second dialog while the move was in flight leaves the user with no notice + // at all that it failed. + const videoToast = source.indexOf('VIDEOS_FAILED_TO_MOVE'); + const guard = source.indexOf('canRetainFailedSelection('); + + expect(videoToast).toBeGreaterThan(-1); + expect(guard).toBeGreaterThan(-1); + expect(videoToast).toBeLessThan(guard); + // Sitting ahead of the guard is not enough on its own -- re-checking ownership in the + // toast's own condition puts it back behind the guard by another route. The failure is + // reported to whoever started the move whatever owns the modal by the time it lands. + const toastGate = source.slice(source.lastIndexOf('if (', videoToast), videoToast); + expect(toastGate).toContain('failed.length > 0'); + expect(toastGate).not.toContain('operation'); + }); +}); diff --git a/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.tsx b/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.tsx index d24e812fc4a..3180380afd5 100644 --- a/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.tsx +++ b/invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.tsx @@ -1,11 +1,14 @@ import type { ComboboxOnChange, ComboboxOption } from '@invoke-ai/ui-library'; import { Combobox, ConfirmationAlertDialog, Flex, FormControl, Text } from '@invoke-ai/ui-library'; import { createSelector } from '@reduxjs/toolkit'; -import { useAppDispatch, useAppSelector } from 'app/store/storeHooks'; +import { useAppDispatch, useAppSelector, useAppStore } from 'app/store/storeHooks'; import { useAssertSingleton } from 'common/hooks/useAssertSingleton'; import { selectCurrentUser } from 'features/auth/store/authSlice'; +import { captureAuthContext, isSameAuthContext } from 'features/auth/store/authTokenRefresh'; import { + canRetainFailedSelection, changeBoardReset, + imagesToChangeSelected, isModalOpenChanged, selectChangeBoardModalSlice, videosToChangeSelected, @@ -37,6 +40,7 @@ const selectIsModalOpen = createSelector( const ChangeBoardModal = () => { useAssertSingleton('ChangeBoardModal'); const dispatch = useAppDispatch(); + const store = useAppStore(); const currentBoardId = useAppSelector(selectSelectedBoardId); const currentUser = useAppSelector(selectCurrentUser); const [selectedBoardId, setSelectedBoardId] = useState(); @@ -85,16 +89,25 @@ const ChangeBoardModal = () => { return; } - if (imagesToChange.length) { - if (selectedBoardId === 'none') { - removeImagesFromBoard({ image_names: imagesToChange }); - } else { - addImagesToBoard({ - image_names: imagesToChange, - board_id: selectedBoardId, - }); - } - } + const authContext = captureAuthContext(); + + // Awaited, not fired and forgotten. The batch routes report per-name failures in + // `failed_images`, and accepting this dialog resets the selection on the way out + // (ConfirmationAlertDialog calls acceptCallback then onClose) — so the names that did not + // move used to be dropped along with the ones that did, leaving nothing to retry from. + // + // Per-name failures and whole-request failures are toasted by the endpoint itself. What is + // left to do here is keep names that need a retry selected. + const operationId = selectChangeBoardModalSlice(store.getState()).operation_id; + const failedImageNamesPromise: Promise = !imagesToChange.length + ? Promise.resolve([]) + : (selectedBoardId === 'none' + ? removeImagesFromBoard({ image_names: imagesToChange }) + : addImagesToBoard({ image_names: imagesToChange, board_id: selectedBoardId }) + ) + .unwrap() + .then((result) => result.failed_images) + .catch(() => imagesToChange); const videoMutations: { videoName: string; promise: Promise }[] = []; if (videosToChange.length) { @@ -112,21 +125,65 @@ const ChangeBoardModal = () => { } } - const results = await Promise.allSettled(videoMutations.map(({ promise }) => promise)); + // Both kinds go out together: the video routes take one name at a time, and serializing + // them behind the image batch would leave a large move waiting on the other's round trips. + const [failedImageNames, results] = await Promise.all([ + failedImageNamesPromise, + Promise.allSettled(videoMutations.map(({ promise }) => promise)), + ]); const failed = results.filter((result) => result.status === 'rejected'); - if (failed.length === 0) { + const isSameSession = isSameAuthContext(authContext); + + // Reported ahead of the ownership guard below, not behind it. Nothing else reports a move + // made from this dialog: the video board routes carry no `onQueryStarted` and no + // `matchRejected` listener, unlike the image batch routes, and the one other emitter of this + // toast id — `settleVideoBoardMutations`, on the drag-and-drop path — only ever settles the + // mutations it fired itself. So behind the guard, opening and cancelling any second dialog + // while this move was in flight would leave the user with no notice at all that it failed. + // The guard exists to keep a stale write out of a shared slice, not to decide who gets told + // about a request they themselves started. Only the session check applies here: the failure + // belongs to whoever started the move, so it is not raised at whoever holds the tab after a + // logout. + if (failed.length > 0 && isSameSession) { + toast({ + id: 'VIDEOS_FAILED_TO_MOVE', + title: t('toast.videosFailedToMove', { count: failed.length }), + status: 'warning', + }); + } + + // Checked before *any* of the writes below, the reset included: all of them land after an + // unbounded await, and the reset is as capable of clearing a selection that now belongs to + // someone else as the retain is of overwriting it. + if (!canRetainFailedSelection(selectChangeBoardModalSlice(store.getState()), operationId, isSameSession)) { + return; + } + if (failed.length === 0 && failedImageNames.length === 0) { dispatch(changeBoardReset()); return; } + // Cleared before either reopen below. `selectedBoardId` is component state that the accept + // does not reset, and `options` drops whichever board is currently being viewed — so move a + // large selection to B, click into B to watch it arrive, and the dialog reopens showing the + // "select a board" placeholder while still armed for B. Move would then send the retry to a + // target the dialog is not showing. What the combobox displays has to be what Move uses. + setSelectedBoardId(null); + // At most one of these fires: the two selections are mutually exclusive by construction — + // imagesToChangeSelected clears video_names and videosToChangeSelected clears image_names, + // and every caller opens this dialog through one of them. Reopen the dialog so retained + // failures are actionable instead of inert state after the accept close. + if (failedImageNames.length > 0) { + dispatch(imagesToChangeSelected(failedImageNames)); + dispatch(isModalOpenChanged(true)); + } + if (failed.length === 0) { + return; + } const failedVideoNames = results.flatMap((result, index) => result.status === 'rejected' && videoMutations[index] ? [videoMutations[index].videoName] : [] ); dispatch(videosToChangeSelected(failedVideoNames)); - toast({ - id: 'VIDEOS_FAILED_TO_MOVE', - title: t('toast.videosFailedToMove', { count: failed.length }), - status: 'warning', - }); + dispatch(isModalOpenChanged(true)); }, [ addImagesToBoard, addVideoToBoard, @@ -135,6 +192,7 @@ const ChangeBoardModal = () => { removeImagesFromBoard, removeVideoFromBoard, selectedBoardId, + store, t, videosToChange, ]); diff --git a/invokeai/frontend/web/src/features/changeBoardModal/store/slice.ts b/invokeai/frontend/web/src/features/changeBoardModal/store/slice.ts index dadf7a43b36..3782ba36e0c 100644 --- a/invokeai/frontend/web/src/features/changeBoardModal/store/slice.ts +++ b/invokeai/frontend/web/src/features/changeBoardModal/store/slice.ts @@ -5,6 +5,7 @@ import type { SliceConfig } from 'app/store/types'; import z from 'zod'; const zChangeBoardModalState = z.object({ + operation_id: z.number().int().nonnegative().default(0), isModalOpen: z.boolean().default(false), image_names: z.array(z.string()).default(() => []), video_names: z.array(z.string()).default(() => []), @@ -21,10 +22,12 @@ const slice = createSlice({ state.isModalOpen = action.payload; }, imagesToChangeSelected: (state, action: PayloadAction) => { + state.operation_id += 1; state.image_names = action.payload; state.video_names = []; }, videosToChangeSelected: (state, action: PayloadAction) => { + state.operation_id += 1; state.video_names = action.payload; state.image_names = []; }, @@ -33,10 +36,51 @@ const slice = createSlice({ state.video_names = []; state.isModalOpen = false; }, + changeBoardOperationInvalidated: (state) => { + state.operation_id += 1; + state.image_names = []; + state.video_names = []; + state.isModalOpen = false; + }, }, }); -export const { isModalOpenChanged, imagesToChangeSelected, videosToChangeSelected, changeBoardReset } = slice.actions; +export const { + isModalOpenChanged, + imagesToChangeSelected, + videosToChangeSelected, + changeBoardReset, + changeBoardOperationInvalidated, +} = slice.actions; + +/** + * Whether a completed move may write the names it could not move back into the modal's pending + * selection. Operation ID must still match the move that completed. + * + * The write lands long after the dialog has closed — ConfirmationAlertDialog calls + * acceptCallback and then onClose without awaiting, so `changeBoardReset` has already run — and + * the slice it writes into is shared by every opener. Two things can have happened in between: + * + * - Another selection can have claimed and released the modal. Right-click a different image + * while a large move is in flight, then cancel that second dialog; the state is empty and + * closed again, but its operation ID is newer. Without that ID the slice looks unclaimed and + * the earlier request's failures are written in under the newer operation's identity. The + * owning component reopens the modal only after this check accepts a retained failure, so the + * operation ID still protects the reachable retry path. + * - The session can have ended. The logout listener invalidates this operation along with the + * api state, and re-seeding it afterwards would leave one user's image names in the next + * user's store. + */ +export const canRetainFailedSelection = ( + modalState: { operation_id: number; isModalOpen: boolean; image_names: string[]; video_names: string[] }, + operationId: number, + isSameSession: boolean +): boolean => + modalState.operation_id === operationId && + isSameSession && + !modalState.isModalOpen && + modalState.image_names.length === 0 && + modalState.video_names.length === 0; export const selectChangeBoardModalSlice = (state: RootState) => state.changeBoardModal; diff --git a/invokeai/frontend/web/src/features/controlLayers/components/RefImage/RefImageImage.tsx b/invokeai/frontend/web/src/features/controlLayers/components/RefImage/RefImageImage.tsx index a754f0e4da4..a6e0633f091 100644 --- a/invokeai/frontend/web/src/features/controlLayers/components/RefImage/RefImageImage.tsx +++ b/invokeai/frontend/web/src/features/controlLayers/components/RefImage/RefImageImage.tsx @@ -21,6 +21,7 @@ import { useTranslation } from 'react-i18next'; import { PiArrowCounterClockwiseBold, PiCropBold, PiRulerBold } from 'react-icons/pi'; import { useGetImageDTOQuery, useUploadImageMutation } from 'services/api/endpoints/images'; import type { ImageDTO } from 'services/api/types'; +import { isImageMissingError } from 'services/api/util/imageErrors'; import { $isConnected } from 'services/events/stores'; type Props = { @@ -56,10 +57,20 @@ export const RefImageImage = memo( }, [onChangeImage]); useEffect(() => { - if ((isConnected && croppedImageDTOReq.isError) || originalImageDTOReq.isError) { + // Cleared only on a confirmed 404. A 403 is a permission decision that can be + // reversed — a board flipped back to Shared — and the image behind it still + // exists; a 5xx or a dropped connection says nothing at all. Dropping the + // reference is silent and has no undo. See `isImageMissingError`. + // Both arms wait for the connection, where the original's used to clear regardless. The + // two images belong to one reference and there is no reading under which losing the crop + // is more suspect than losing the original. + if ( + isConnected && + (isImageMissingError(croppedImageDTOReq.error) || isImageMissingError(originalImageDTOReq.error)) + ) { handleResetControlImage(); } - }, [handleResetControlImage, isConnected, croppedImageDTOReq.isError, originalImageDTOReq.isError]); + }, [handleResetControlImage, isConnected, croppedImageDTOReq.error, originalImageDTOReq.error]); const onUpload = useCallback( (imageDTO: ImageDTO) => { diff --git a/invokeai/frontend/web/src/features/controlLayers/components/RegionalGuidance/RegionalGuidanceRefImageImage.tsx b/invokeai/frontend/web/src/features/controlLayers/components/RegionalGuidance/RegionalGuidanceRefImageImage.tsx index 85285dd4ef3..3a304e4093e 100644 --- a/invokeai/frontend/web/src/features/controlLayers/components/RegionalGuidance/RegionalGuidanceRefImageImage.tsx +++ b/invokeai/frontend/web/src/features/controlLayers/components/RegionalGuidance/RegionalGuidanceRefImageImage.tsx @@ -17,6 +17,7 @@ import { useTranslation } from 'react-i18next'; import { PiArrowCounterClockwiseBold, PiRulerBold } from 'react-icons/pi'; import { useGetImageDTOQuery } from 'services/api/endpoints/images'; import type { ImageDTO } from 'services/api/types'; +import { isImageMissingError } from 'services/api/util/imageErrors'; import { $isConnected } from 'services/events/stores'; type Props = { @@ -32,16 +33,20 @@ export const RegionalGuidanceRefImageImage = memo(({ image, onChangeImage, dndTa const isConnected = useStore($isConnected); const tab = useAppSelector(selectActiveTab); const isStaging = useCanvasIsStaging(); - const { currentData: imageDTO, isError } = useGetImageDTOQuery(image?.image_name ?? skipToken); + const { currentData: imageDTO, error } = useGetImageDTOQuery(image?.image_name ?? skipToken); const handleResetControlImage = useCallback(() => { onChangeImage(null); }, [onChangeImage]); useEffect(() => { - if (isConnected && isError) { + // Cleared only on a confirmed 404. A 403 is a permission decision that can be + // reversed — a board flipped back to Shared — and the image behind it still + // exists; a 5xx or a dropped connection says nothing at all. Dropping the + // reference is silent and has no undo. See `isImageMissingError`. + if (isConnected && isImageMissingError(error)) { handleResetControlImage(); } - }, [handleResetControlImage, isError, isConnected]); + }, [handleResetControlImage, error, isConnected]); const onUpload = useCallback( (imageDTO: ImageDTO) => { diff --git a/invokeai/frontend/web/src/features/controlLayers/store/canvasSlice.ts b/invokeai/frontend/web/src/features/controlLayers/store/canvasSlice.ts index c2c9b3a858b..d2cf81e97d6 100644 --- a/invokeai/frontend/web/src/features/controlLayers/store/canvasSlice.ts +++ b/invokeai/frontend/web/src/features/controlLayers/store/canvasSlice.ts @@ -5,6 +5,7 @@ import { moveOneToEnd, moveOneToStart, moveToEnd, moveToStart } from 'common/uti import { deepClone } from 'common/util/deepClone'; import { roundDownToMultiple, roundToMultiple } from 'common/util/roundDownToMultiple'; import { merge } from 'es-toolkit/compat'; +import { logout } from 'features/auth/store/authSlice'; import { getPrefixedId } from 'features/controlLayers/konva/util'; import { canvasReset } from 'features/controlLayers/store/actions'; import { aspectRatioIdChanged, modelChanged, resolutionPresetSelected } from 'features/controlLayers/store/paramsSlice'; @@ -1833,6 +1834,18 @@ const slice = createSlice({ builder.addCase(canvasReset, (state) => { return resetState(state); }); + // A deliberate sign-out hands this browser to whoever comes next: the canvas is personal + // workspace state, and it is also where deleted-image references live (raster layers, + // control layers), so leaving it standing hands the next account both the previous user's + // work and, after an aborted cross-user batch delete, references to images that no longer + // exist. `sessionExpiredLogout` is deliberately NOT handled — a session timeout must not + // destroy work, and the same user's committed deletions are pruned by `handleDeletions` + // off the batch's partial result instead. The undo stack is cleared separately: this case + // is a cross-slice action the undoable filter keeps out of history without emptying it, + // and the store's account-change reducer chains `canvasClearHistory` for it. + builder.addCase(logout, (state) => { + return resetState(state); + }); builder.addCase(modelChanged, (state, action) => { const { model } = action.payload; /** diff --git a/invokeai/frontend/web/src/features/controlLayers/store/refImagesSlice.ts b/invokeai/frontend/web/src/features/controlLayers/store/refImagesSlice.ts index 6c364e51e88..114ea629956 100644 --- a/invokeai/frontend/web/src/features/controlLayers/store/refImagesSlice.ts +++ b/invokeai/frontend/web/src/features/controlLayers/store/refImagesSlice.ts @@ -5,6 +5,7 @@ import { createMemoizedSelector } from 'app/store/createMemoizedSelector'; import type { RootState } from 'app/store/store'; import type { SliceConfig } from 'app/store/types'; import { clamp } from 'es-toolkit/compat'; +import { logout } from 'features/auth/store/authSlice'; import { getPrefixedId } from 'features/controlLayers/konva/util'; import type { CroppableImageWithDims, @@ -309,6 +310,13 @@ const slice = createSlice({ state.entities = next; }, }, + extraReducers(builder) { + // See canvasSlice: reference images are both personal workspace state and a place + // deleted-image references live; a deliberate sign-out clears them for the next account. + // `sessionExpiredLogout` is deliberately not handled. Not undoable, so the reset alone is + // the whole job here. + builder.addCase(logout, () => getInitialRefImagesState()); + }, }); export const { diff --git a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/ImageFieldInputComponent.tsx b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/ImageFieldInputComponent.tsx index 27c5d02625c..89b39aea848 100644 --- a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/ImageFieldInputComponent.tsx +++ b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/ImageFieldInputComponent.tsx @@ -14,6 +14,7 @@ import { memo, useCallback, useEffect, useMemo } from 'react'; import { useTranslation } from 'react-i18next'; import { useGetImageDTOQuery } from 'services/api/endpoints/images'; import type { ImageDTO } from 'services/api/types'; +import { isImageMissingError } from 'services/api/util/imageErrors'; import { $isConnected } from 'services/events/stores'; import type { FieldComponentProps } from './types'; @@ -24,7 +25,7 @@ const ImageFieldInputComponent = (props: FieldComponentProps { dispatch( fieldImageValueChanged({ @@ -45,10 +46,14 @@ const ImageFieldInputComponent = (props: FieldComponentProps { - if (isConnected && isError) { + // Cleared only on a confirmed 404. A 403 is a permission decision that can be + // reversed — a board flipped back to Shared — and the image behind it still + // exists; a 5xx or a dropped connection says nothing at all. Dropping the + // field is silent and has no undo. See `isImageMissingError`. + if (isConnected && isImageMissingError(error)) { handleReset(); } - }, [handleReset, isConnected, isError]); + }, [handleReset, isConnected, error]); const onUpload = useCallback( (imageDTO: ImageDTO) => { diff --git a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/VideoFieldInputComponent.tsx b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/VideoFieldInputComponent.tsx index cb3cdf6b0fa..6d83aa649da 100644 --- a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/VideoFieldInputComponent.tsx +++ b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/VideoFieldInputComponent.tsx @@ -48,9 +48,10 @@ const VideoFieldInputComponent = (props: FieldComponentProps { if (isConnected && isVideoMissingError(error)) { handleReset(); diff --git a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.test.ts b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.test.ts index d574e17ffd8..e51bfa41c11 100644 --- a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.test.ts +++ b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.test.ts @@ -8,10 +8,14 @@ describe('isVideoMissingError', () => { }); it.each([ + // Revoking access to a shared board answers 403 for videos that are all still there. + // Clearing on it would destroy the workflows pointing at them, and restoring the permission + // would not bring them back. + ['denied (403)', { status: 403, data: { detail: 'Not authorized' } }], ['auth (401)', { status: 401, data: {} }], - ['forbidden (403)', { status: 403, data: {} }], ['server error (500)', { status: 500, data: {} }], ['network failure', { status: 'FETCH_ERROR', error: 'TypeError: Failed to fetch' }], + ['timeout', { status: 'TIMEOUT_ERROR', error: 'AbortError' }], ['parsing failure', { status: 'PARSING_ERROR', originalStatus: 200, data: '', error: 'oops' }], ['no error', undefined], ])('is false for %s — the field value must be preserved', (_label, error) => { diff --git a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.ts b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.ts index c55f160b96c..f40d19a410d 100644 --- a/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.ts +++ b/invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.ts @@ -1,9 +1,16 @@ /** - * True only for a confirmed "video does not exist" (HTTP 404) query error. + * True only for a confirmed "this video no longer exists" (HTTP 404). * - * VideoFieldInputComponent clears its field value when the referenced video is gone, but - * a transient network error (`FETCH_ERROR`), auth failure (401/403), or server error (5xx) - * must not silently discard the user's input — only a 404 proves the video was deleted. + * `VideoFieldInputComponent` drops the user's reference when the video behind it is gone. That + * reset is silent and has no undo, so it may only follow an answer that is both definite and + * permanent — which a 403 is not. Access is revoked and restored: flip a board to Private and + * every field referencing its videos would clear; flip it back and the videos are all still + * there, but the workflows that pointed at them are not. + * + * `_assert_video_read_access` draws that distinction server-side, answering 404 for a video + * positively absent and 403 only for one it is refusing, and `video_records.get` no longer + * translates storage errors into not-found — without which an unreadable database would present + * as a deleted video. `isImageMissingError` is the same predicate for images. */ export const isVideoMissingError = (error: unknown): boolean => error instanceof Object && 'status' in error && error.status === 404; diff --git a/invokeai/frontend/web/src/features/nodes/store/nodesSlice.ts b/invokeai/frontend/web/src/features/nodes/store/nodesSlice.ts index 25fdb606a90..3faabbce8f0 100644 --- a/invokeai/frontend/web/src/features/nodes/store/nodesSlice.ts +++ b/invokeai/frontend/web/src/features/nodes/store/nodesSlice.ts @@ -14,6 +14,7 @@ import { applyEdgeChanges, applyNodeChanges, getConnectedEdges, getIncomers, get import type { SliceConfig } from 'app/store/types'; import { deepClone } from 'common/util/deepClone'; import { isPlainObject } from 'es-toolkit'; +import { logout } from 'features/auth/store/authSlice'; import { addElement, removeElement, @@ -801,6 +802,15 @@ const slice = createSlice({ undo: (state) => state, redo: (state) => state, }, + extraReducers(builder) { + // See canvasSlice: a deliberate sign-out clears personal workspace state for whoever uses + // this browser next, and node image fields are one of the places deleted-image references + // live. `sessionExpiredLogout` is deliberately not handled — a timeout must not destroy an + // unsaved workflow. History: this slice sets no `clearHistoryType`, so redux-undo's default + // clear-history action empties the stack; the store's account-change reducer dispatches it + // alongside this reset. + builder.addCase(logout, () => getInitialState()); + }, }); export const { diff --git a/invokeai/frontend/web/src/features/parameters/store/upscaleSlice.ts b/invokeai/frontend/web/src/features/parameters/store/upscaleSlice.ts index 9604e961b0e..56fa207c0b9 100644 --- a/invokeai/frontend/web/src/features/parameters/store/upscaleSlice.ts +++ b/invokeai/frontend/web/src/features/parameters/store/upscaleSlice.ts @@ -3,6 +3,7 @@ import { createSelector, createSlice } from '@reduxjs/toolkit'; import type { RootState } from 'app/store/store'; import type { SliceConfig } from 'app/store/types'; import { isPlainObject } from 'es-toolkit'; +import { logout } from 'features/auth/store/authSlice'; import type { ImageWithDims } from 'features/controlLayers/store/types'; import { zImageWithDims } from 'features/controlLayers/store/types'; import { zModelIdentifierField } from 'features/nodes/types/common'; @@ -80,6 +81,12 @@ const slice = createSlice({ state.tileOverlap = action.payload; }, }, + extraReducers(builder) { + // See canvasSlice: the upscale initial image is the fourth place `getImageUsage` tracks + // image references, and it is personal workspace state besides — a deliberate sign-out + // clears it for the next account. `sessionExpiredLogout` is deliberately not handled. + builder.addCase(logout, () => getInitialState()); + }, }); export const { diff --git a/invokeai/frontend/web/src/services/api/endpoints/auth.test.ts b/invokeai/frontend/web/src/services/api/endpoints/auth.test.ts new file mode 100644 index 00000000000..b3f35c79bc2 --- /dev/null +++ b/invokeai/frontend/web/src/services/api/endpoints/auth.test.ts @@ -0,0 +1,80 @@ +import { configureStore } from '@reduxjs/toolkit'; +import { authApi } from 'services/api/endpoints/auth'; +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { api } from '..'; + +/** + * `dynamicBaseQuery` reads the bearer token out of localStorage, and `getDeploymentBaseUrl` + * reads `window.location.origin`. Neither exists in the default (node) test environment. + */ +beforeAll(() => { + const values = new Map(); + vi.stubGlobal('localStorage', { + clear: () => values.clear(), + getItem: (key: string) => values.get(key) ?? null, + key: (index: number) => [...values.keys()][index] ?? null, + get length() { + return values.size; + }, + setItem: (key: string, value: string) => values.set(key, value), + removeItem: (key: string) => values.delete(key), + }); + vi.stubGlobal('window', { location: { origin: 'http://localhost' } }); +}); + +beforeEach(() => { + localStorage.clear(); +}); + +const buildStore = () => + configureStore({ + reducer: { [api.reducerPath]: api.reducer }, + middleware: (getDefaultMiddleware) => getDefaultMiddleware().concat(api.middleware), + }); + +describe('getCurrentUser', () => { + it('does not let a replacement session read the 401 of the token it replaced', async () => { + // The sequence this exists for: a tab page-loads with an expired token and asks who it is; + // another tab logs the same user back in, so localStorage now holds a new token and this tab + // adopts it — an adoption that deliberately keeps the API cache, since the user did not + // change. The first request's 401 arrives in between. Shared across logins, one cache entry + // would hand that 401 to the adopted session, and `ProtectedRoute` ends the session on a 401 + // from this query: the token the user just obtained would be deleted out of localStorage, + // taking the tab that minted it down too. + const requests: (string | null)[] = []; + vi.stubGlobal( + 'fetch', + vi.fn((request: Request) => { + const sent = request.headers.get('Authorization'); + requests.push(sent); + if (sent === 'Bearer token-expired') { + return Promise.resolve(new Response(null, { status: 401 })); + } + return Promise.resolve( + new Response(JSON.stringify({ user_id: 'user-1', email: 'user@example.com', is_admin: false }), { + headers: { 'content-type': 'application/json' }, + }) + ); + }) + ); + const store = buildStore(); + + localStorage.setItem('auth_token', 'token-expired'); + await store.dispatch(authApi.endpoints.getCurrentUser.initiate('token-expired')); + expect(authApi.endpoints.getCurrentUser.select('token-expired')(store.getState()).error).toBeDefined(); + + localStorage.setItem('auth_token', 'token-fresh'); + await store.dispatch(authApi.endpoints.getCurrentUser.initiate('token-fresh')); + + // A second request went out, under the new credential, and its result is what the adopted + // session reads. The superseded entry keeps its own 401 and is no longer anybody's answer. + expect(requests).toEqual(['Bearer token-expired', 'Bearer token-fresh']); + const fresh = authApi.endpoints.getCurrentUser.select('token-fresh')(store.getState()); + expect(fresh.error).toBeUndefined(); + expect(fresh.data).toMatchObject({ user_id: 'user-1' }); + // And the superseded entry is untouched, so this is a separate answer rather than one + // overwritten in place: an adopted session reads `token-fresh`'s and never sees the 401. + expect(authApi.endpoints.getCurrentUser.select('token-expired')(store.getState()).error).toBeDefined(); + }); +}); diff --git a/invokeai/frontend/web/src/services/api/endpoints/auth.ts b/invokeai/frontend/web/src/services/api/endpoints/auth.ts index dbbdf4b68f8..48e704eb557 100644 --- a/invokeai/frontend/web/src/services/api/endpoints/auth.ts +++ b/invokeai/frontend/web/src/services/api/endpoints/auth.ts @@ -103,7 +103,20 @@ export const authApi = api.injectEndpoints({ method: 'POST', }), }), - getCurrentUser: build.query({ + /** + * Keyed by the bearer token the request will carry, which is why it takes an argument it + * never sends: the token travels in the `Authorization` header, read out of localStorage at + * send time, but it is what the answer is *about*, so it belongs in the cache key. + * + * Shared across logins, one entry outlives the token that produced it. A 401 for a token + * that has since been replaced stays readable under its replacement — and `ProtectedRoute` + * ends the session on a 401 from this query, so it would end a session on a stranger's + * failure. Nothing refetches it on its own to correct that: the argument never changed, the + * endpoint has no tags, and the API-state reset that a login normally triggers is skipped + * when the new token belongs to the same user. Keyed by token, the replacement session + * simply reads a different entry, and asking for it is what fetches it. + */ + getCurrentUser: build.query({ query: () => 'api/v1/auth/me', }), setup: build.mutation({ diff --git a/invokeai/frontend/web/src/services/api/endpoints/images.test.ts b/invokeai/frontend/web/src/services/api/endpoints/images.test.ts new file mode 100644 index 00000000000..3bb3efd3f21 --- /dev/null +++ b/invokeai/frontend/web/src/services/api/endpoints/images.test.ts @@ -0,0 +1,1146 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +import { configureStore } from '@reduxjs/toolkit'; +import type { BaseQueryApi } from '@reduxjs/toolkit/query'; +import { sessionExpiredLogout } from 'features/auth/store/authSlice'; +import { toast } from 'features/toast/toast'; +import i18n from 'i18next'; +import { + buildChunkedImageBatchQueryFn, + bulkDownloadQueryFn, + chunkImageNames, + imageDTOsByNamesQueryFn, + imagesApi, + mergeImageBatchResults, + reportImageBatchOutcome, + sessionMismatchError, + toastFailedImageBatch, +} from 'services/api/endpoints/images'; +import type { ImageDTO } from 'services/api/types'; +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { api, buildV1Url, dynamicBaseQuery } from '..'; + +vi.mock('features/toast/toast', () => ({ toast: vi.fn() })); +vi.mock('i18next', () => ({ default: { t: vi.fn((key: string) => key) } })); + +/** Mirrors MAX_IMAGE_BATCH_SIZE in invokeai/app/api/routers/images.py. */ +const CHUNK_SIZE = 1000; + +describe('IMAGE_BATCH_CHUNK_SIZE', () => { + it('matches the bound the server actually publishes', () => { + // The client-side chunk size exists only to satisfy the server-side cap, and the two are + // declared in different languages. openapi.json is the committed contract between them + // (typegen-checks keeps it in sync with the routers), so drift on either side fails here + // rather than at runtime as a batch of 422s. + type ArraySchema = { maxItems?: number; anyOf?: ArraySchema[] }; + const openapi = JSON.parse( + readFileSync(fileURLToPath(new URL('../../../../openapi.json', import.meta.url)), 'utf8') + ) as { + components: { schemas: Record }; + }; + // A nullable list serializes as anyOf [bounded array, null], so the bound must be dug out + // of the variants as well — read flat, the download body silently drops off this check and + // its cap can drift alone. + const boundOf = (schema: ArraySchema | undefined): number | undefined => + schema?.maxItems ?? schema?.anyOf?.map((variant) => boundOf(variant)).find((bound) => bound !== undefined); + const bounds = Object.entries(openapi.components.schemas) + .map(([name, schema]) => [name, boundOf(schema.properties?.image_names)] as const) + .filter((entry): entry is [string, number] => entry[1] !== undefined); + + // Every image_names body the server bounds, bounded by the same number: the five batch + // mutations, the DTO read, and the nullable download body. The floor keeps this from + // passing vacuously if the server stops publishing the cap — and from quietly shrinking + // back to the flat six if the anyOf handling regresses. + expect(bounds.length).toBeGreaterThanOrEqual(7); + for (const [name, maxItems] of bounds) { + expect(maxItems, name).toBe(CHUNK_SIZE); + } + }); +}); + +const names = (count: number) => Array.from({ length: count }, (_, i) => `image-${i}.png`); + +/** + * The chunk loops read the live session out of localStorage before every request, so the tests + * need a real one to write to. Not available in the default (node) test environment. + */ +beforeAll(() => { + const values = new Map(); + vi.stubGlobal('localStorage', { + clear: () => values.clear(), + getItem: (key: string) => values.get(key) ?? null, + key: (index: number) => [...values.keys()][index] ?? null, + get length() { + return values.size; + }, + setItem: (key: string, value: string) => values.set(key, value), + removeItem: (key: string) => values.delete(key), + }); +}); + +beforeEach(() => { + localStorage.clear(); +}); + +/** A token shaped like the real JWT: only the `user_id` claim is read back out of it. */ +const tokenFor = (userId: string, issuedAt = 1) => + `header.${btoa(JSON.stringify({ user_id: userId, iat: issuedAt }))}.signature`; + +const login = (userId: string, issuedAt = 1) => { + localStorage.setItem('auth_token', tokenFor(userId, issuedAt)); +}; + +/** What a logout + login as someone else leaves behind: a new token AND a bumped generation. */ +const switchUser = (userId: string) => { + localStorage.setItem('auth_generation', String(Number(localStorage.getItem('auth_generation') ?? 0) + 1)); + login(userId); +}; + +describe('chunkImageNames', () => { + it('leaves a conforming list as a single request', () => { + expect(chunkImageNames(names(3))).toEqual([names(3)]); + expect(chunkImageNames(names(CHUNK_SIZE))).toHaveLength(1); + }); + + it('still issues one request for an empty list', () => { + // The routes answer an empty body with a well-formed empty result. Merging zero chunks + // would produce `{}`, and callers read fields like `failed_images` off the result. + expect(chunkImageNames([])).toEqual([[]]); + }); + + it('splits an oversized list into conforming chunks that cover it exactly', () => { + const all = names(2500); + const chunks = chunkImageNames(all); + expect(chunks.map((c) => c.length)).toEqual([1000, 1000, 500]); + expect(chunks.flat()).toEqual(all); + }); +}); + +describe('mergeImageBatchResults', () => { + it('unions each key and dedupes, so the caller sees one single-request-shaped result', () => { + expect( + mergeImageBatchResults([ + { deleted_images: ['a.png'], failed_images: [], affected_boards: ['board-1', 'none'] }, + { deleted_images: ['b.png'], failed_images: ['c.png'], affected_boards: ['board-1'] }, + ]) + ).toEqual({ + deleted_images: ['a.png', 'b.png'], + failed_images: ['c.png'], + affected_boards: ['board-1', 'none'], + }); + }); +}); + +describe('toastFailedImageBatch', () => { + beforeEach(() => { + vi.mocked(toast).mockClear(); + vi.mocked(i18n.t).mockClear(); + }); + + it('reports every unique name when the first request fails', () => { + toastFailedImageBatch(['a.png', 'a.png', 'b.png']); + + expect(i18n.t).toHaveBeenCalledWith('toast.imagesFailedToUpdate', { count: 2 }); + expect(toast).toHaveBeenCalledWith({ + id: 'IMAGES_FAILED_TO_UPDATE', + title: 'toast.imagesFailedToUpdate', + status: 'warning', + }); + }); +}); + +describe('sessionMismatchError', () => { + // Tested directly because no loop-level test can reach the pre-request takeover arm: a + // takeover staged inside a mocked baseQuery is always caught by the post-response check + // first, and same-tab nothing can interleave between one chunk's post-check and the next's + // pre-check. Cross-tab it is the guard that stops the next chunk going out as the new user. + it('classifies a takeover as the hard abort', () => { + login('user-b'); + expect(sessionMismatchError()).toMatchObject({ error: expect.stringContaining('session changed') }); + }); + + it('classifies a bare expiry as the soft stop', () => { + localStorage.removeItem('auth_token'); + expect(sessionMismatchError()).toMatchObject({ error: expect.stringContaining('session ended') }); + }); +}); + +describe('reportImageBatchOutcome', () => { + beforeEach(() => { + vi.mocked(toast).mockClear(); + vi.mocked(i18n.t).mockClear(); + }); + + it('reports the names the server could not apply when the request resolves', async () => { + await reportImageBatchOutcome( + { image_names: names(3) }, + { queryFulfilled: Promise.resolve({ data: { failed_images: ['image-1.png'] } }) } + ); + + expect(i18n.t).toHaveBeenCalledWith('toast.imagesFailedToUpdate', { count: 1 }); + }); + + it('says nothing when every name was applied', async () => { + await reportImageBatchOutcome( + { image_names: names(3) }, + { queryFulfilled: Promise.resolve({ data: { failed_images: [] } }) } + ); + + expect(toast).not.toHaveBeenCalled(); + }); + + it('reports the whole argument list when the request rejects', async () => { + // A rejection out of the chunked queryFn is raised only when nothing was committed, so + // every name really is unapplied. Swallowing it leaves a delete or a move that landed + // nothing saying nothing at all -- these endpoints have no matchRejected listener. + await reportImageBatchOutcome({ image_names: names(3) }, { queryFulfilled: Promise.reject(new Error('boom')) }); + + expect(i18n.t).toHaveBeenCalledWith('toast.imagesFailedToUpdate', { count: 3 }); + expect(toast).toHaveBeenCalledWith({ + id: 'IMAGES_FAILED_TO_UPDATE', + title: 'toast.imagesFailedToUpdate', + status: 'warning', + }); + }); + + it('does not report an outcome from a session that ended while it was pending', async () => { + login('user-a'); + let resolveQuery: (value: { data: { failed_images: string[] } }) => void = () => {}; + const queryFulfilled = new Promise<{ data: { failed_images: string[] } }>((resolve) => { + resolveQuery = resolve; + }); + + const outcome = reportImageBatchOutcome({ image_names: names(3) }, { queryFulfilled }); + switchUser('user-b'); + resolveQuery({ data: { failed_images: ['image-1.png'] } }); + await outcome; + + expect(toast).not.toHaveBeenCalled(); + }); + + it('does not report a rejection from a session that ended while it was pending', async () => { + // The branch the session change actually takes: an auth-changed abort comes back as an + // error, so `queryFulfilled` rejects. Guarding only the fulfilled branch would leave the + // previous user's whole selection toasted at whoever holds the tab next. + login('user-a'); + let rejectQuery: (reason: unknown) => void = () => {}; + const queryFulfilled = new Promise<{ data: { failed_images: string[] } }>((_resolve, reject) => { + rejectQuery = reject; + }); + + const outcome = reportImageBatchOutcome({ image_names: names(3) }, { queryFulfilled }); + switchUser('user-b'); + rejectQuery(new Error('aborted')); + await outcome; + + expect(toast).not.toHaveBeenCalled(); + }); + + it('is wired into every chunked batch mutation', () => { + // RTK exposes no way to reach an endpoint's `onQueryStarted` at runtime -- the built + // endpoint object carries only initiate/select/match*/hooks -- so the wiring is guarded at + // the source level, as elsewhere in this repo. Counted against the chunked endpoints rather + // than a fixed number, so a sixth one that forgets to report its failures fails this. The + // call sites are matched wherever they are, not only inline after `queryFn:`, since hoisting + // one to a const is otherwise enough to slip an unreporting endpoint past this. + const source = readFileSync(fileURLToPath(new URL('./images.ts', import.meta.url)), 'utf8'); + const chunked = source.match(/buildChunkedImageBatchQueryFn\(/g) ?? []; + const wired = source.match(/onQueryStarted: reportImageBatchOutcome,/g) ?? []; + + expect(chunked.length).toBeGreaterThan(0); + expect(wired).toHaveLength(chunked.length); + }); +}); + +describe('buildChunkedImageBatchQueryFn', () => { + type Arg = { image_names: string[]; board_id?: string }; + type Result = { added_images: string[]; failed_images: string[]; affected_boards: string[] }; + type Request = { url: string; method: string; body: Arg }; + type Response = { data: Result } | { error: { status: number | string; data: string } }; + + const getTags = () => ['ImageCollectionCounts' as const]; + + beforeEach(() => { + vi.mocked(toast).mockClear(); + }); + + const run = (baseQuery: (args: Request) => Promise, arg: Arg) => { + const dispatch = vi.fn(); + const queryFn = buildChunkedImageBatchQueryFn( + () => ({ url: '/api/v1/board_images/batch', method: 'POST' }), + getTags, + (image_names) => ({ added_images: image_names, failed_images: [], affected_boards: [] }) + ); + /* eslint-disable @typescript-eslint/no-explicit-any */ + return { dispatch, result: queryFn(arg, { dispatch } as any, undefined, baseQuery as any) }; + /* eslint-enable @typescript-eslint/no-explicit-any */ + }; + + it('sends one request per chunk, carrying the non-name body fields on each', async () => { + const baseQuery = vi.fn( + (_args: Request): Promise => + Promise.resolve({ data: { added_images: [], failed_images: [], affected_boards: [] } }) + ); + + const { result } = run(baseQuery, { image_names: names(2500), board_id: 'board-1' }); + await result; + + expect(baseQuery).toHaveBeenCalledTimes(3); + // board_id must ride along with every chunk, not just the first. + expect(baseQuery.mock.calls.map(([args]) => args.body.board_id)).toEqual(['board-1', 'board-1', 'board-1']); + expect(baseQuery.mock.calls.map(([args]) => args.body.image_names.length)).toEqual([1000, 1000, 500]); + }); + + it('merges the per-chunk results into one aggregate', async () => { + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + const { result } = run(baseQuery, { image_names: names(1500) }); + + expect(await result).toEqual({ + data: { added_images: ['chunk-1.png', 'chunk-2.png'], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + it('reports a mid-run failure as a partial success, keeping what the server already applied', async () => { + // A bare error would discard the first chunk's payload. It is not only the RTK cache at + // stake: handleDeletions drives the gallery selection and strips deleted images out of + // nodes, canvas layers and reference images off `deleted_images`, and none of that runs on + // a rejection — so 1000 images would be gone from the DB and still referenced by the canvas. + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 3) { + return Promise.resolve({ error: { status: 500, data: 'boom' } }); + } + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + const { dispatch, result } = run(baseQuery, { image_names: names(2500) }); + + // The failing chunk's 500 names are unreached, not merely un-reported, so they are folded + // into failed_images -- one place, so the endpoint's single toast reports one true total. + expect(await result).toEqual({ + data: { + added_images: ['chunk-1.png', 'chunk-2.png'], + failed_images: names(2500).slice(2000), + affected_boards: ['board-1'], + }, + }); + expect(baseQuery).toHaveBeenCalledTimes(3); // stopped, did not keep firing chunks + expect(dispatch).toHaveBeenCalledWith(api.util.invalidateTags(getTags())); + // Toasting from here as well would fire twice on one toast id, and the toast system + // updates in place -- the second count would replace the first rather than adding to it. + expect(toast).not.toHaveBeenCalled(); + }); + + it('reports an error when the first chunk fails, since nothing was applied', async () => { + const baseQuery = vi.fn( + (_args: Request): Promise => Promise.resolve({ error: { status: 403, data: 'nope' } }) + ); + + const { dispatch, result } = run(baseQuery, { image_names: names(1500) }); + + expect(await result).toEqual({ error: { status: 403, data: 'nope' } }); + expect(dispatch).not.toHaveBeenCalled(); + expect(toast).not.toHaveBeenCalled(); + }); + + it('aborts an errored chunk that returns into a taken-over session, consuming nothing', async () => { + // The error path consumes state too — since the indeterminate-error reconciliation it + // dispatches as-if-committed invalidations, and the partial path returns an aggregate the + // UI applies. A takeover discovered when the chunk comes back must therefore hard-abort + // BEFORE that handling runs, exactly as it does for a successful payload. Staged with an + // indeterminate 500, the worst case: without the triage it would both invalidate and + // return user A's partial result into user B's session. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + switchUser('user-b'); + return Promise.resolve({ error: { status: 500, data: 'boom' } }); + } + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + const { dispatch, result } = run(baseQuery, { image_names: names(1500) }); + + expect(await result).toEqual({ + error: { + status: 'CUSTOM_ERROR', + error: 'Aborted: the authenticated session changed while the operation was running', + }, + }); + expect(dispatch).not.toHaveBeenCalled(); + expect(toast).not.toHaveBeenCalled(); + }); + + it('invalidates the failing chunk as if it had landed when the error cannot prove otherwise', async () => { + // A transport error can strike after the server committed the chunk -- the response is + // what was lost, not necessarily the request. The names are still reported failed, which + // is the honest reading and safe to retry, but the caches the chunk may have touched are + // invalidated as if it had landed, so the refetch shows the truth either way. + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + return Promise.resolve({ error: { status: 'FETCH_ERROR', data: 'network' } }); + } + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + const dispatch = vi.fn(); + // Result-sensitive tags, so the two invalidations are distinguishable in the assertions. + const resultTags = (result: Result) => [{ type: 'Image' as const, id: result.added_images[0] ?? 'none' }]; + const queryFn = buildChunkedImageBatchQueryFn( + () => ({ url: '/api/v1/board_images/batch', method: 'POST' }), + resultTags, + (image_names) => ({ added_images: image_names, failed_images: [], affected_boards: [] }) + ); + /* eslint-disable @typescript-eslint/no-explicit-any */ + const result = await queryFn({ image_names: names(1500) }, { dispatch } as any, undefined, baseQuery as any); + /* eslint-enable @typescript-eslint/no-explicit-any */ + + // The assumed-committed invalidation is keyed by the failing chunk's own names, with the + // board-keyed tag types appended type-wide, since the lost chunk's boards are unknowable. + expect(dispatch).toHaveBeenCalledWith( + api.util.invalidateTags([ + { type: 'Image', id: 'image-1000.png' }, + 'ImageList', + 'Board', + 'BoardImagesTotal', + 'BoardVideosTotal', + ]) + ); + // ...and the merged invalidation for the chunks that did land still happens. + expect(dispatch).toHaveBeenCalledWith(api.util.invalidateTags([{ type: 'Image', id: 'chunk-1.png' }])); + expect(result).toEqual({ + data: { added_images: ['chunk-1.png'], failed_images: names(1500).slice(1000), affected_boards: ['board-1'] }, + }); + }); + + it('does not second-guess a chunk the server itself refused', async () => { + // A 4xx is the server saying it did nothing, so the failing chunk's caches hold no lie to + // reconcile -- only the merged invalidation for the landed chunks fires. + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + return Promise.resolve({ error: { status: 403, data: 'nope' } }); + } + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + const assumeCommitted = vi.fn((image_names: string[]): Result => { + return { added_images: image_names, failed_images: [], affected_boards: [] }; + }); + const dispatch = vi.fn(); + const queryFn = buildChunkedImageBatchQueryFn( + () => ({ url: '/api/v1/board_images/batch', method: 'POST' }), + getTags, + assumeCommitted + ); + /* eslint-disable @typescript-eslint/no-explicit-any */ + await queryFn({ image_names: names(1500) }, { dispatch } as any, undefined, baseQuery as any); + /* eslint-enable @typescript-eslint/no-explicit-any */ + + expect(assumeCommitted).not.toHaveBeenCalled(); + expect(dispatch).toHaveBeenCalledTimes(1); + }); + + it('reports a first-chunk transport failure as an error, but still reconciles its caches', async () => { + // Nothing is known to have landed, so the run is reported as the failure it probably was + // -- but "probably" is the point: the chunk may have committed, so its tags are + // invalidated before the error goes back. + const baseQuery = vi.fn( + (_args: Request): Promise => Promise.resolve({ error: { status: 'TIMEOUT_ERROR', data: 'slow' } }) + ); + const { dispatch, result } = run(baseQuery, { image_names: names(1500) }); + + expect(await result).toEqual({ error: { status: 'TIMEOUT_ERROR', data: 'slow' } }); + expect(dispatch).toHaveBeenCalledTimes(1); + expect(dispatch).toHaveBeenCalledWith( + api.util.invalidateTags([...getTags(), 'ImageList', 'Board', 'BoardImagesTotal', 'BoardVideosTotal']) + ); + expect(toast).not.toHaveBeenCalled(); + }); + + it('stops when the session changes mid-run, instead of applying the rest as the new user', async () => { + // Every request picks up whatever token localStorage holds when it is sent, so a loop that + // outlives its own session finishes as whoever logged in next -- on a public board those + // writes land, committing half of one user's action under another's name. resetApiState on + // the logout action does not help: it clears the cache, not a queryFn that is running. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + switchUser('user-b'); + } + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + const { dispatch, result } = run(baseQuery, { image_names: names(2500) }); + + // The second response is stale as soon as another user takes over, so neither it nor a + // partial aggregate may reach the new session. The server-side write cannot be rolled back + // here, but the next session must refetch its own state rather than consume the old result. + // Pinned to the *changed* wording: both abort errors contain "Aborted", and only the + // takeover one may discard committed work like this. + expect(await result).toEqual({ + error: { status: 'CUSTOM_ERROR', error: expect.stringContaining('session changed') }, + }); + expect(baseQuery).toHaveBeenCalledTimes(2); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('discards a response that arrived after the session changed', async () => { + // A single-chunk run, so there is no following chunk whose pre-request check could catch + // this: the response itself came back into a session that is no longer the one that asked + // for it. Without the check after the response, its payload is returned and applied. + login('user-a'); + const baseQuery = vi.fn((_args: Request): Promise => { + switchUser('user-b'); + return Promise.resolve({ data: { added_images: ['a.png'], failed_images: [], affected_boards: ['board-1'] } }); + }); + + const { dispatch, result } = run(baseQuery, { image_names: names(5) }); + + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(await result).toEqual({ + error: { status: 'CUSTOM_ERROR', error: expect.stringContaining('session changed') }, + }); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('reports an expired session as the failure it is, not as an abort', async () => { + // `dynamicBaseQuery` dispatches `sessionExpiredLogout` on a 401 before it returns, and that + // clears the token synchronously — so by the time the post-response check runs the session + // has "changed" for every 401 there is. Rewriting those as aborts would make the ordinary + // expired-session case fatal to the whole run, skipping the partial-success path that + // reports what the server already committed. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + localStorage.removeItem('auth_token'); + return Promise.resolve({ error: { status: 401, data: 'expired' } }); + } + return Promise.resolve({ + data: { added_images: [`chunk-${call}.png`], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + const { dispatch, result } = run(baseQuery, { image_names: names(2500) }); + + // Chunk 1 committed, so this is a partial success and its unreached names are reported -- + // the same shape a mid-run 500 produces, which is what lets `handleDeletions` prune. + expect(await result).toEqual({ + data: { + added_images: ['chunk-1.png'], + failed_images: names(2500).slice(1000), + affected_boards: ['board-1'], + }, + }); + expect(dispatch).toHaveBeenCalledWith(api.util.invalidateTags(getTags())); + }); + + it('keeps running when a login request elsewhere bumps the auth generation', async () => { + // `beginAuthTransition` bumps the shared counter when a login or logout request is *sent*, + // before anything has changed and whether or not it succeeds — a second tab opening the + // login page must not abandon this tab's batch. The session is judged by who the next + // request would go out as, which is unchanged here. + login('user-a'); + const baseQuery = vi.fn((_args: Request): Promise => { + localStorage.setItem('auth_generation', '7'); + return Promise.resolve({ data: { added_images: [], failed_images: [], affected_boards: [] } }); + }); + + const { result } = run(baseQuery, { image_names: names(2500) }); + await result; + + expect(baseQuery).toHaveBeenCalledTimes(3); + }); + + it('keeps running across a sliding-window token refresh, which is the same session', async () => { + // The middleware mints a fresh token on mutating requests. Comparing tokens byte-for-byte + // would abandon every batch long enough to be refreshed -- the exact operations chunking + // exists for. + login('user-a', 1); + const baseQuery = vi.fn((_args: Request): Promise => { + login('user-a', 2); + return Promise.resolve({ data: { added_images: [], failed_images: [], affected_boards: [] } }); + }); + + const { result } = run(baseQuery, { image_names: names(2500) }); + await result; + + expect(baseQuery).toHaveBeenCalledTimes(3); + }); + + it('stops on expiry but keeps reporting what the run committed', async () => { + // sessionExpiredLogout drops the token with no request of its own -- a 401 on ANY concurrent + // request (a gallery poll, a board refetch) clears it synchronously, and the generation + // counter never moves -- so the loop must stop: without the token half of the check it + // would run on with no credentials at all. But expiry is not a takeover. The committed + // chunk belongs to the very user heading for the login screen, and hard-aborting here + // discards the partial payload that handleDeletions needs to strip committed deletions out + // of the persisted canvas/nodes/reference-image slices -- none of which handle + // sessionExpiredLogout -- so the stale references would survive into their next login. + // Note the token vanishes while a *successful* chunk is in flight: the post-response check + // must consume that payload (same user, work committed), and only then stop. + login('user-a'); + const baseQuery = vi.fn((_args: Request): Promise => { + localStorage.removeItem('auth_token'); + return Promise.resolve({ + data: { added_images: ['chunk-1.png'], failed_images: [], affected_boards: ['board-1'] }, + }); + }); + + const { dispatch, result } = run(baseQuery, { image_names: names(2500) }); + + expect(await result).toEqual({ + data: { + added_images: ['chunk-1.png'], + failed_images: names(2500).slice(1000), + affected_boards: ['board-1'], + }, + }); + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(dispatch).toHaveBeenCalledWith(api.util.invalidateTags(getTags())); + }); +}); + +describe('bulkDownloadQueryFn', () => { + type Body = { image_names: string[]; board_id?: string }; + type Request = { url: string; method: string; body: Body }; + type Response = { data: { bulk_download_item_name: string } } | { error: { status: number; data: string } }; + + const run = (baseQuery: (args: Request) => Promise, body: Body) => + /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ + bulkDownloadQueryFn(body, undefined, undefined, baseQuery as any); + + beforeEach(() => { + vi.mocked(toast).mockClear(); + vi.mocked(i18n.t).mockClear(); + }); + + it('issues one request per chunk and returns the first item name', async () => { + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + return Promise.resolve({ data: { bulk_download_item_name: `item-${call}.zip` } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(baseQuery).toHaveBeenCalledTimes(3); + // Only the first name is returned; each background task announces its own zip over the + // socket, so the payload is just a handle for the single "preparing" toast. + expect(result).toEqual({ data: { bulk_download_item_name: 'item-1.zip' } }); + expect(toast).not.toHaveBeenCalled(); + }); + + it('sends one request for a board download, which the server expands itself', async () => { + const baseQuery = vi.fn( + (_args: Request): Promise => Promise.resolve({ data: { bulk_download_item_name: 'board.zip' } }) + ); + + await run(baseQuery, { image_names: [], board_id: 'board-1' }); + + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(baseQuery.mock.calls[0]?.[0].body.board_id).toBe('board-1'); + }); + + it('resolves a mid-run failure instead of rejecting, since the earlier zips still arrive', async () => { + // The route answers 202 as soon as it has scheduled the background task, so chunk 1's zip + // is already being built. Rejecting would drive `matchRejected` -> "problem preparing + // download" while that zip lands in the user's downloads anyway. + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + return Promise.resolve({ error: { status: 403, data: 'nope' } }); + } + return Promise.resolve({ data: { bulk_download_item_name: `item-${call}.zip` } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(result).toEqual({ data: { bulk_download_item_name: 'item-1.zip' } }); + expect(baseQuery).toHaveBeenCalledTimes(2); // stopped, did not keep firing chunks + // The 1500 names from the failing chunk on are in no zip at all -- reported here, because + // ImagesDownloaded carries no per-name failure list to fold them into. + expect(toast).toHaveBeenCalledTimes(1); + expect(vi.mocked(toast).mock.calls[0]?.[0]).toMatchObject({ + id: 'IMAGES_FAILED_TO_DOWNLOAD', + status: 'warning', + }); + // Asserted on the interpolation rather than the title, since i18n is mocked to echo the + // key. The count is the whole point of the toast: the failing chunk's 1000 names plus the + // 500 never sent. Off by one chunk in either direction and the user is told the wrong + // number of images are missing from their download. + expect(i18n.t).toHaveBeenCalledWith('toast.imagesFailedToDownload', { count: 1500 }); + }); + + it('still schedules-and-reports when the 202 body does not survive the trip back', async () => { + // `fetchBaseQuery` resolves an empty entity as `data: null`, so a proxy that strips the + // body off the 202 leaves nothing to return -- but the background task was scheduled all + // the same and its zip will arrive. Any nullish test over the payload (`!first`) treats + // that as nothing-happened and toasts an error over an arriving download. + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + return Promise.resolve({ error: { status: 500, data: 'boom' } }); + } + /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ + return Promise.resolve({ data: null as any }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(result).toEqual({ data: null }); + expect(i18n.t).toHaveBeenCalledWith('toast.imagesFailedToDownload', { count: 1500 }); + }); + + it('sends one request when a board id rides along with an oversized name list', async () => { + // The server picks board_id over image_names (BulkDownloadService), so each chunk would + // schedule the same full-board zip again: 1001 names plus a board id used to produce two + // identical whole-board downloads. + const baseQuery = vi.fn( + (_args: Request): Promise => Promise.resolve({ data: { bulk_download_item_name: 'board.zip' } }) + ); + + await run(baseQuery, { image_names: names(2500), board_id: 'board-1' }); + + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(baseQuery.mock.calls[0]?.[0].body).toEqual({ image_names: undefined, board_id: 'board-1' }); + }); + + it('stops scheduling zips when the session changes mid-run', async () => { + // Switched on the *second* call, so a zip is already scheduled and `first` is set. Switching + // on the first leaves `first` undefined, which makes returning it indistinguishable from + // returning nothing -- and returning it is exactly what must not happen. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + switchUser('user-b'); + } + return Promise.resolve({ data: { bulk_download_item_name: `item-${call}.zip` } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(baseQuery).toHaveBeenCalledTimes(2); + // Not `item-1.zip`: that zip belongs to the previous session. Do not expose its item name or + // toast the new session about work it did not request. + expect(result).toEqual({ data: undefined }); + expect(toast).not.toHaveBeenCalled(); + expect(i18n.t).not.toHaveBeenCalled(); + }); + + it('withholds the payload but says so when the session expires with zips scheduled', async () => { + // The payload is withheld like the takeover case -- the count is a miscount at the login + // screen, and the item name would raise the permanent `duration: null` "preparing" toast + // there. But expiry with zips already scheduled loses real work: they finish server-side + // and their completion events fire into a dying socket, so nothing will ever offer them. + // Until scheduled downloads are replayed after re-auth, the user is told to re-run rather + // than left reading silence as a download that never came. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + localStorage.removeItem('auth_token'); + } + return Promise.resolve({ data: { bulk_download_item_name: `item-${call}.zip` } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(baseQuery).toHaveBeenCalledTimes(2); + expect(result).toEqual({ data: undefined }); + expect(toast).toHaveBeenCalledTimes(1); + // The title pins the full key path. The i18n mock echoes the key, so a reference into the + // wrong section -- the first cut said `gallery.` while the key lives under `toast.` -- + // renders the raw key string in production while an id-only assertion stays green. + expect(vi.mocked(toast).mock.calls[0]?.[0]).toMatchObject({ + id: 'DOWNLOADS_INTERRUPTED', + title: 'toast.downloadsInterrupted', + status: 'warning', + }); + // No per-name failure count: that toast id belongs to genuine chunk failures, and its + // number would be wrong here anyway. + expect(i18n.t).not.toHaveBeenCalledWith('toast.imagesFailedToDownload', expect.anything()); + }); + + it('warns and withholds the payload when the session expires during the final chunk', async () => { + // Mid-loop expiry is caught by the next chunk's pre-request check, but the final chunk has + // no next iteration -- and fetchChunk's post-response check deliberately lets a mere + // expiry through. Without a post-loop check the run returns `first`, and matchFulfilled + // raises the `duration: null` "preparing" toast into a session whose socket will never + // deliver the dismissal -- while the zips, all scheduled, are lost silently. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 3) { + localStorage.removeItem('auth_token'); + } + return Promise.resolve({ data: { bulk_download_item_name: `item-${call}.zip` } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(baseQuery).toHaveBeenCalledTimes(3); + expect(result).toEqual({ data: undefined }); + expect(toast).toHaveBeenCalledTimes(1); + expect(vi.mocked(toast).mock.calls[0]?.[0]).toMatchObject({ + id: 'DOWNLOADS_INTERRUPTED', + title: 'toast.downloadsInterrupted', + status: 'warning', + }); + }); + + it("treats the chunk's own expired-session 401 as an interruption, not a partial failure", async () => { + // The everyday expiry vector: the 401 arrives on the download's own chunk, and + // dynamicBaseQuery has already cleared the token by the time it returns. The mutating + // loops keep that 401 on the partial path on purpose -- the payload feeds handleDeletions + // -- but here the partial path produces exactly the two wrong toasts: a failure count at + // the login screen and, via the returned item name, the permanent "preparing" toast. + login('user-a'); + let call = 0; + const baseQuery = vi.fn((_args: Request): Promise => { + call += 1; + if (call === 2) { + localStorage.removeItem('auth_token'); + return Promise.resolve({ error: { status: 401, data: 'expired' } }); + } + return Promise.resolve({ data: { bulk_download_item_name: `item-${call}.zip` } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(baseQuery).toHaveBeenCalledTimes(2); + expect(result).toEqual({ data: undefined }); + expect(toast).toHaveBeenCalledTimes(1); + expect(vi.mocked(toast).mock.calls[0]?.[0]).toMatchObject({ id: 'DOWNLOADS_INTERRUPTED' }); + expect(i18n.t).not.toHaveBeenCalledWith('toast.imagesFailedToDownload', expect.anything()); + }); + + it('stays silent on a first-chunk expired-session 401, which loses nothing', async () => { + // Nothing was scheduled: no zip exists or ever will, and the user re-runs after signing + // in. Withheld like every other session-ending outcome rather than rejected -- a rejection + // would toast "Problem Preparing Download" at the login screen for a download that simply + // needs re-running. + login('user-a'); + const baseQuery = vi.fn((_args: Request): Promise => { + localStorage.removeItem('auth_token'); + return Promise.resolve({ error: { status: 401, data: 'expired' } }); + }); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(result).toEqual({ data: undefined }); + expect(toast).not.toHaveBeenCalled(); + }); + + it('reports an error when the first chunk fails, since nothing was scheduled', async () => { + const baseQuery = vi.fn( + (_args: Request): Promise => Promise.resolve({ error: { status: 403, data: 'nope' } }) + ); + + const result = await run(baseQuery, { image_names: names(2500) }); + + expect(result).toEqual({ error: { status: 403, data: 'nope' } }); + expect(baseQuery).toHaveBeenCalledTimes(1); + // Nothing landed, so the failure toast belongs to the `matchRejected` listener alone. + expect(toast).not.toHaveBeenCalled(); + }); +}); + +describe('imageDTOsByNamesQueryFn', () => { + type Request = { url: string; method: string; body: { image_names: string[] } }; + type Response = { data: ImageDTO[] } | { error: { status: number; data: string } }; + + const dto = (image_name: string) => ({ image_name }) as ImageDTO; + + const run = (baseQuery: (args: Request) => Promise, image_names: string[]) => { + const dispatch = vi.fn(); + /* eslint-disable @typescript-eslint/no-explicit-any */ + const queryApi = { dispatch } as any; + const query = baseQuery as any; + /* eslint-enable @typescript-eslint/no-explicit-any */ + return { dispatch, result: imageDTOsByNamesQueryFn({ image_names }, queryApi, undefined, query) }; + }; + + it('publishes each chunk as it arrives, so a later failure cannot discard the earlier ones', async () => { + // This mutation rejects on any chunk failure and its only caller + // (useRangeBasedImageFetching) never looks at the rejection -- so DTOs held back until the + // end would be dropped for good. The hook re-requests only names missing from the cache, + // and only when the user scrolls, so nothing would come back for them. + let call = 0; + const baseQuery = vi.fn((args: Request): Promise => { + call += 1; + if (call === 2) { + return Promise.resolve({ error: { status: 500, data: 'boom' } }); + } + return Promise.resolve({ data: args.body.image_names.map(dto) }); + }); + + const { dispatch, result } = run(baseQuery, names(2500)); + + expect(await result).toEqual({ error: { status: 500, data: 'boom' } }); + // One dispatch, carrying chunk one -- not zero, which is what holding the DTOs back until + // the end would produce. Matched on the payload: the action also carries a request id and a + // timestamp, both fresh per call. + expect(dispatch).toHaveBeenCalledTimes(1); + const action = dispatch.mock.calls[0]?.[0] as { + type: string; + payload: { value: ImageDTO }[]; + }; + expect(action.type).toBe(imagesApi.util.upsertQueryEntries([]).type); + expect(action.payload.map((entry) => entry.value.image_name)).toEqual(names(1000)); + }); + + it('checks the session with nothing between the check and the publish', () => { + // Asserted structurally rather than by counting microtasks, which would break on any change + // to the async shape rather than on the property. `fetchChunk` already checks the context + // after the response, but resuming from it is a hop, and a logout landing in that hop passes + // its check and still clears the cache before the upsert runs. Only a check with no await + // between it and the write closes the window, so the write has to sit inside one. + const source = readFileSync(fileURLToPath(new URL('./images.ts', import.meta.url)), 'utf8'); + expect(source).toMatch(/if \(isSameAuthContext\(authContext\)\) \{\s*upsertImageDTOs\(dispatch, chunkDTOs\);\s*\}/); + }); + + it('does not publish a chunk that came back after the session changed', async () => { + // The DTOs were fetched as whoever was logged in when the chunk went out. By the time they + // land the logout listener may have reset the api state for someone else, and writing them + // in then seeds one user's cache with another's images. + login('user-a'); + const baseQuery = vi.fn((args: Request): Promise => { + switchUser('user-b'); + return Promise.resolve({ data: args.body.image_names.map(dto) }); + }); + + const { dispatch, result } = run(baseQuery, names(2500)); + await result; + + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('stops reading when the session changes mid-run', async () => { + login('user-a'); + const baseQuery = vi.fn((args: Request): Promise => { + switchUser('user-b'); + return Promise.resolve({ data: args.body.image_names.map(dto) }); + }); + + const { result } = run(baseQuery, names(2500)); + + expect(baseQuery).toHaveBeenCalledTimes(1); + expect(await result).toEqual({ error: { status: 'CUSTOM_ERROR', error: expect.stringContaining('Aborted') } }); + }); +}); + +describe('unauthorized responses', () => { + // `getDeploymentBaseUrl` reads `window.location.origin`, and these are the only tests in this + // file that issue a real request rather than driving a queryFn with a mocked baseQuery. + beforeAll(() => { + vi.stubGlobal('window', { location: { origin: 'http://localhost' } }); + }); + + /** One real request through `dynamicBaseQuery`, with the server's answer staged by `respond`. */ + const request = async (respond: () => Response) => { + const dispatch = vi.fn(); + vi.stubGlobal( + 'fetch', + vi.fn(() => Promise.resolve(respond())) + ); + const result = await dynamicBaseQuery( + buildV1Url('images/i/a.png'), + { + dispatch, + getState: () => ({}), + signal: new AbortController().signal, + abort: () => {}, + endpoint: 'getImageDTO', + type: 'query', + forced: false, + extra: undefined, + } as unknown as BaseQueryApi, + {} + ); + return { dispatch, result: result as { error?: { status?: unknown } } }; + }; + + it('ends the session when the token that got the 401 is still the live one', async () => { + login('user-a'); + + const { dispatch, result } = await request(() => new Response(null, { status: 401 })); + + expect(result.error?.status).toBe(401); + expect(dispatch).toHaveBeenCalledWith(sessionExpiredLogout()); + }); + + it('does not end the session that replaced the one the 401 belongs to', async () => { + // A's request is slow. While it is in flight B takes over the tab -- a login here, or one in + // another tab, which lands the same way because localStorage is shared. Then A's 401 arrives. + // Ending the session on it logs out B, who never issued the request and whose own credential + // the server never rejected. + login('user-a'); + + const { dispatch, result } = await request(() => { + switchUser('user-b'); + return new Response(null, { status: 401 }); + }); + + // The 401 is still reported to the caller -- that request did fail. What must not happen is + // the session-wide consequence. + expect(result.error?.status).toBe(401); + // The dispatch is the whole assertion: `sessionExpiredLogout`'s reducer is what removes + // B's token from localStorage, so not dispatching it is exactly "B stays authenticated". + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('does not end a session over a 401 for the token a refresh replaced', async () => { + // Same user throughout: the sliding window minted a new token mid-request. The old token's + // 401 says nothing about the new one, and if the session really is over the next request + // carries the live token and its 401 ends it. + login('user-a', 1); + + const { dispatch } = await request(() => { + login('user-a', 2); + return new Response(null, { status: 401 }); + }); + + expect(dispatch).not.toHaveBeenCalled(); + }); + + it('leaves a 401 on an unauthenticated request alone', async () => { + // No token was sent, so nothing about a session was disproved. These fire during page load. + const { dispatch } = await request(() => new Response(null, { status: 401 })); + + expect(dispatch).not.toHaveBeenCalled(); + }); +}); + +describe('star invalidation', () => { + beforeAll(() => { + vi.stubGlobal('window', { location: { origin: 'http://localhost' } }); + }); + + const dtoUrl = `http://localhost/${buildV1Url('images/i/a.png')}`; + + /** A store holding just the API slice: enough for tag invalidation to drive a refetch. */ + const buildStore = () => + configureStore({ + reducer: { [api.reducerPath]: api.reducer }, + middleware: (getDefaultMiddleware) => getDefaultMiddleware().concat(api.middleware), + }); + + type ApiStore = ReturnType; + + it('reconciles a single-chunk delete whose outcome was lost in transit', async () => { + // Anything up to the batch cap is one request, so this is what an ordinary delete looks like + // when the response never comes back: the mutation rejects, `invalidatesTags` never runs on + // a result, and `handleDeletions` prunes nothing because nothing was confirmed. The only + // thing that can settle it is the invalidation the queryFn dispatches on its way out, and + // the refetch it triggers — which answers 404 if the delete did commit, and the DTO if it + // did not. + login('user-a'); + const fetched: string[] = []; + vi.stubGlobal( + 'fetch', + vi.fn((request: Request) => { + fetched.push(request.url); + if (request.method === 'POST') { + return Promise.reject(new TypeError('Failed to fetch')); + } + return Promise.resolve( + new Response(JSON.stringify({ image_name: 'a.png', starred: false }), { + headers: { 'content-type': 'application/json' }, + }) + ); + }) + ); + const store = buildStore(); + + await store.dispatch(imagesApi.endpoints.getImageDTO.initiate('a.png')); + expect(fetched.filter((url) => url === dtoUrl)).toHaveLength(1); + + await store.dispatch(imagesApi.endpoints.deleteImages.initiate({ image_names: ['a.png'] })); + + await vi.waitFor(() => expect(fetched.filter((url) => url === dtoUrl)).toHaveLength(2)); + }); + + it.each([ + { + label: 'star', + body: { starred_images: [], failed_images: ['a.png'], affected_boards: [] }, + mutate: (store: ApiStore) => store.dispatch(imagesApi.endpoints.starImages.initiate({ image_names: ['a.png'] })), + }, + { + label: 'unstar', + body: { unstarred_images: [], failed_images: ['a.png'], affected_boards: [] }, + mutate: (store: ApiStore) => + store.dispatch(imagesApi.endpoints.unstarImages.initiate({ image_names: ['a.png'] })), + }, + { + // A delete whose outcome is unknown. `handleDeletions` prunes references only for names + // the server confirmed, so nothing else asks about this one; the refetch is what settles + // whether the components holding it should let it go. + label: 'deletion', + body: { deleted_images: [], failed_images: ['a.png'], affected_boards: [] }, + mutate: (store: ApiStore) => + store.dispatch(imagesApi.endpoints.deleteImages.initiate({ image_names: ['a.png'] })), + }, + ])('refetches an image whose $label the server could not confirm', async ({ body, mutate }) => { + // `ImageService.update` writes the record and then reads the DTO back to return it. A + // failure in that read reports the name in `failed_images` with the row already starred, so + // the client's cached DTO is now wrong and nothing else will ever contradict it. Invalidating + // only the successes leaves the gallery showing the old star until a full reload. + login('user-a'); + const fetched: string[] = []; + vi.stubGlobal( + 'fetch', + vi.fn((request: Request) => { + fetched.push(request.url); + const payload = request.method === 'POST' ? body : { image_name: 'a.png', starred: false }; + return Promise.resolve( + new Response(JSON.stringify(payload), { headers: { 'content-type': 'application/json' } }) + ); + }) + ); + const store = buildStore(); + + await store.dispatch(imagesApi.endpoints.getImageDTO.initiate('a.png')); + expect(fetched.filter((url) => url === dtoUrl)).toHaveLength(1); + + await mutate(store); + + await vi.waitFor(() => expect(fetched.filter((url) => url === dtoUrl)).toHaveLength(2)); + }); +}); diff --git a/invokeai/frontend/web/src/services/api/endpoints/images.ts b/invokeai/frontend/web/src/services/api/endpoints/images.ts index cd4ec8f39b3..c21e276dc31 100644 --- a/invokeai/frontend/web/src/services/api/endpoints/images.ts +++ b/invokeai/frontend/web/src/services/api/endpoints/images.ts @@ -1,7 +1,14 @@ +import type { Dispatch, UnknownAction } from '@reduxjs/toolkit'; +import type { FetchArgs, FetchBaseQueryError, QueryReturnValue } from '@reduxjs/toolkit/query'; import { skipToken } from '@reduxjs/toolkit/query'; import { getStore } from 'app/store/nanostores/store'; +import { uniq } from 'es-toolkit'; +import type { AuthContext } from 'features/auth/store/authTokenRefresh'; +import { captureAuthContext, isSameAuthContext } from 'features/auth/store/authTokenRefresh'; import type { CroppableImageWithDims } from 'features/controlLayers/store/types'; import { ASSETS_CATEGORIES, IMAGE_CATEGORIES } from 'features/gallery/store/types'; +import { toast } from 'features/toast/toast'; +import i18n from 'i18next'; import type { components, paths } from 'services/api/schema'; import type { GetImageNamesArgs, @@ -44,6 +51,604 @@ const buildImagesUrl = (path: string = '', query?: Parameters */ const buildBoardImagesUrl = (path: string = '') => buildV1Url(`board_images/${path}`); +/** + * The batch routes cap `image_names` server-side (`MAX_IMAGE_BATCH_SIZE` in + * `invokeai/app/api/routers/images.py`) so one authenticated client cannot pin a worker with a + * single request. Nothing caps a gallery *selection*: select-all reads the whole board's name + * list, so one keystroke on a large board produces a selection an order of magnitude past the + * cap. Oversized bodies are therefore split client-side into conforming requests. + * + * Must stay <= the backend constant. + */ +const IMAGE_BATCH_CHUNK_SIZE = 1000; + +/** + * Every batch route answers with an object whose values are all name lists, and every one of + * them reports its per-name failures in `failed_images` — which is where a chunked run folds + * the names it never reached. + */ +type ImageBatchResult = Record & { failed_images: string[] }; + +type InvalidateTagsArg = Parameters[0]; + +/** The `baseQuery` handed to a `queryFn`, matching what `fetchBaseQuery` produces. */ +type ImagesBaseQuery = ( + args: string | FetchArgs +) => QueryReturnValue | PromiseLike>; + +/** + * Another user's session now owns the tab. This is the hard abort: nothing from the old run may + * be consumed — not a payload, not a partial aggregate, not a toast — because everything it + * carries belongs to whoever started it, and the loops treat it as fatal (`isAuthChangedError`). + */ +const AUTH_CHANGED_ERROR: FetchBaseQueryError = { + status: 'CUSTOM_ERROR', + error: 'Aborted: the authenticated session changed while the operation was running', +}; + +/** + * The session ended with no successor: the token is gone — a 401 on *any* concurrent request + * dispatches `sessionExpiredLogout`, whose reducer clears `auth_token` synchronously, and a + * deliberate logout does the same — and nobody else has logged in. Deliberately NOT matched by + * `isAuthChangedError`, so it takes the same partial-success path as an ordinary chunk failure: + * there is no new session to protect, the committed chunks belong to the very user who is about + * to land on the login screen, and the partial reporting is what lets `handleDeletions` strip + * committed deletions out of the persisted slices (canvas, nodes, reference images — none of + * which handle `sessionExpiredLogout`) before their next login. + */ +const SESSION_ENDED_ERROR: FetchBaseQueryError = { + status: 'CUSTOM_ERROR', + error: 'Aborted: the authenticated session ended while the operation was running', +}; + +const isAuthChangedError = (error: FetchBaseQueryError | undefined): boolean => + error?.status === AUTH_CHANGED_ERROR.status && error.error === AUTH_CHANGED_ERROR.error; + +/** Either flavor of session mismatch — the "consume nothing from this run" superset. */ +const isSessionMismatchError = (error: FetchBaseQueryError | undefined): boolean => + isAuthChangedError(error) || + (error?.status === SESSION_ENDED_ERROR.status && error.error === SESSION_ENDED_ERROR.error); + +/** + * Errors that leave a chunk's outcome unknown. A transport failure or timeout can strike after + * the request reached the server -- fetch loses the response, not necessarily the request -- a + * parsing error means a response arrived for work that was already done, and a 5xx says the + * route died somewhere in a loop whose per-name writes had each already committed. In all of + * these the server may have applied the chunk with only the report lost. A 4xx is the server + * itself saying it refused, which is the one shape that proves the chunk did nothing. + */ +const isIndeterminateError = (error: FetchBaseQueryError): boolean => + error.status === 'FETCH_ERROR' || + error.status === 'TIMEOUT_ERROR' || + error.status === 'PARSING_ERROR' || + (typeof error.status === 'number' && error.status >= 500); + +/** + * Which of the two a failed session check means. The distinction is the whole ballgame: expiry + * must degrade into an ordinary failure so committed work is still reported, while a takeover + * must abort hard so the new user consumes nothing. Collapsing them in either direction is a + * bug this file has now had both ways. + * + * Exported for its unit test, and the test exists because no loop-level test can reach the + * takeover arm at the pre-request check: same-tab, everything from one chunk's post-response + * check to the next chunk's pre-request check is a single synchronous drain no dispatch can + * interleave with, so a takeover staged inside a mocked baseQuery is always caught by the + * post-response check first. The pre-request arm is live only cross-tab — another tab's + * login-as-B writes localStorage between chunks — which is real concurrency a same-thread test + * cannot stage, and it is the guard that stops the next chunk going out under B's token. + */ +export const sessionMismatchError = (): FetchBaseQueryError => + localStorage.getItem('auth_token') === null ? SESSION_ENDED_ERROR : AUTH_CHANGED_ERROR; + +/** + * Issues one chunk of a multi-request operation, unless the session it started under is gone. + * + * A selection past the batch cap is split into several requests, and each one picks up whatever + * bearer token localStorage holds when it is sent — the loop does not carry the identity it + * started with. Log out and back in as another user with the loop still running and the + * remaining chunks are applied as that user; on a public board they can land, so half of one + * user's delete is committed under another's name, with nothing to roll it back. Nothing else + * stops it: `resetApiState` on the logout action clears the cache, not a running `queryFn`. + * + * So the context is captured before the first chunk, rechecked before each request, and checked + * again before its response is consumed. See `isSameAuthContext` for what counts as the same + * session — notably a sliding-window token refresh does not, or every long batch would abandon + * itself. A failed check is then triaged by `sessionMismatchError`, because the two ways it can + * fail call for opposite treatments — and the token can vanish at *any* await in the run, not + * just this chunk's own request: a 401 on any concurrent request (a gallery poll, a board + * refetch) clears it synchronously mid-flight. Collapse expiry into the hard abort and every + * one of those windows silently discards whatever the earlier chunks already committed. + */ +const fetchChunk = async ( + baseQuery: ImagesBaseQuery, + authContext: AuthContext, + args: FetchArgs +): Promise> => { + if (!isSameAuthContext(authContext)) { + return { error: sessionMismatchError() }; + } + const response = await baseQuery(args); + // An error is triaged like a success, but only for takeover. Mere expiry passes through + // untriaged: this chunk's own expired-session 401 is the everyday case — `dynamicBaseQuery` + // dispatches `sessionExpiredLogout` before returning it (whenever the token it carried is + // still the live one, which for this chunk's own request is the ordinary case), so the token + // is already gone by this line, and a rewrite would turn the ordinary failure into an abort + // that discards the committed chunks' report. A takeover, though, must not reach the loops' + // error handling at all: that handling *consumes* — it dispatches as-if-committed invalidations and returns + // partial aggregates the UI applies — and everything it would consume belongs to the + // session that started the run, not to whoever owns the tab now. + if (response.error) { + if (localStorage.getItem('auth_token') !== null && !isSameAuthContext(authContext)) { + return { error: AUTH_CHANGED_ERROR }; + } + return response; + } + // A *successful* payload is consumed unless another user has taken over. Mere expiry keeps + // the payload: it belongs to the same user, the server committed it, and dropping it would + // un-report work that already happened — the loop then stops at the next pre-request check. + const token = localStorage.getItem('auth_token'); + if (token !== null && !isSameAuthContext(authContext)) { + return { error: AUTH_CHANGED_ERROR }; + } + return response; +}; + +export const chunkImageNames = (image_names: string[]): string[][] => { + if (image_names.length <= IMAGE_BATCH_CHUNK_SIZE) { + // Single-request path, byte-for-byte what it was before chunking existed. Note this also + // covers the empty list: the routes answer it with a well-formed empty result, which + // merging zero chunks could not reproduce. + return [image_names]; + } + const chunks: string[][] = []; + for (let i = 0; i < image_names.length; i += IMAGE_BATCH_CHUNK_SIZE) { + chunks.push(image_names.slice(i, i + IMAGE_BATCH_CHUNK_SIZE)); + } + return chunks; +}; + +/** + * Unions the per-chunk results key-by-key, so callers and `invalidatesTags` see one aggregate + * result with the same shape a single request would have returned. Keys are unioned rather + * than enumerated because the five batch routes name their outcome list differently + * (`deleted_images`, `starred_images`, `added_images`, ...) while sharing `affected_boards`. + */ +export const mergeImageBatchResults = (results: TResult[]): TResult => { + // Accumulator is the looser Record: it is only complete once every chunk has contributed. + const merged: Record = {}; + for (const result of results) { + for (const [key, names] of Object.entries(result)) { + merged[key] = merged[key] ? uniq(merged[key].concat(names)) : names; + } + } + return merged as TResult; +}; + +/** + * Builds a `queryFn` that runs a batch mutation one conforming chunk at a time. + * + * Chunks go sequentially on purpose. Each is already up to `IMAGE_BATCH_CHUNK_SIZE` rows of DB + * work, and the server-side bound exists to stop a single client from pinning a worker — firing + * the chunks concurrently would hand straight back what the bound took away. + * + * The mid-run failure is the case that matters, and it resolves to a partial success rather than + * an error. Earlier chunks have already been committed by the server; returning a bare error + * would discard their payload, which is exactly the bug the partial-failure reporting on these + * routes exists to fix. It is not only the RTK cache at stake — `handleDeletions` drives the + * gallery selection and strips deleted images out of nodes, canvas layers and reference images + * off `result.deleted_images`, and none of that runs on a rejection. So the merged result is + * returned, the unreached names are folded into its `failed_images`, and only a run where + * *nothing* landed surfaces as an error. + * + * They are folded into the payload rather than toasted here so that a single `onQueryStarted` + * reports one total. Toasting from both places would fire twice on the same toast id, and the + * toast system updates in place and appends "(2)" — so the second count would simply replace + * the first rather than adding to it. + */ +export const buildChunkedImageBatchQueryFn = + ( + request: (body: TArg) => { url: string; method: string }, + getTags: (result: TResult) => InvalidateTagsArg, + assumeCommitted: (image_names: string[], arg: TArg) => TResult + ) => + async ( + arg: TArg, + { dispatch }: { dispatch: (action: ReturnType) => unknown }, + _extraOptions: unknown, + baseQuery: ImagesBaseQuery + ) => { + const results: TResult[] = []; + const authContext = captureAuthContext(); + const chunks = chunkImageNames(arg.image_names); + for (const [index, image_names] of chunks.entries()) { + const response = await fetchChunk(baseQuery, authContext, { ...request(arg), body: { ...arg, image_names } }); + if (response.error) { + if (isAuthChangedError(response.error)) { + // A prior chunk may have committed, but its result belongs to the old session. Do not + // invalidate the new session or return partial data that its UI can apply. + return { error: response.error }; + } + if (isIndeterminateError(response.error)) { + // A transport-shaped failure proves only that the report was lost: the server may + // have committed this chunk with nothing coming back to say so. The names still go + // to failed_images below -- the honest reading, and retrying a name the server + // already satisfied is safe on every batch route -- but the caches this chunk may + // have touched must not keep serving the pre-request state until the user notices, + // so they are invalidated as if the chunk had landed: the refetch shows the truth + // either way. `assumeCommitted` builds the result this chunk would have returned on + // success, so the tags come from the same `getTags` the endpoint publishes and the + // two cannot drift. A lost chunk's affected boards are unknowable, so callers + // assume none — which reaches the global gallery-list tags but none of the + // board-keyed ones (`Board`, `BoardImagesTotal`, ..., all keyed by id). Those are + // appended type-wide instead: every board's sidebar count refetching once beats a + // count that stays wrong until something unrelated bumps it. + dispatch( + api.util.invalidateTags([ + ...getTags(assumeCommitted(image_names, arg)), + 'ImageList', + 'Board', + 'BoardImagesTotal', + 'BoardVideosTotal', + ]) + ); + } + if (results.length === 0) { + // Nothing was applied, so this is an ordinary failed request — report it as one. + return { error: response.error }; + } + // Everything from this chunk on is unreached, not merely un-reported. + const unreached = chunks.slice(index).flat(); + const merged = mergeImageBatchResults(results); + dispatch(api.util.invalidateTags(getTags(merged))); + return { data: { ...merged, failed_images: uniq(merged.failed_images.concat(unreached)) } as TResult }; + } + results.push(response.data as TResult); + } + return { data: mergeImageBatchResults(results) }; + }; + +/** + * Tag sets for the chunked batch mutations. Extracted from the endpoints so the chunked + * `queryFn` can invalidate for the chunks that landed before a mid-run failure using the exact + * tag set the endpoint publishes on success, rather than a hand-rolled approximation. + */ +const getDeleteImagesTags = (result: components['schemas']['DeleteImagesResult']): InvalidateTagsArg => [ + // We ignore the deleted images when getting tags to invalidate. If we did not, we will invalidate the queries + // that fetch image DTOs, metadata, and workflows. But we have just deleted those images! Invalidating the tags + // will force those queries to re-fetch, and the requests will of course 404. + // + // The *failed* names are the opposite case and are refetched deliberately. Their outcome is + // unknown — a chunk that timed out or 5xx'd may well have committed — so `handleDeletions` + // leaves every reference to them in place, since pruning on a guess would discard work over a + // request that merely failed. Asking is what settles it: a name that is really gone answers + // 404 and every component holding it drops it, and one that survived answers with its DTO and + // keeps it. Names folded in client-side for chunks that never went out ride along and simply + // confirm the cache. + // + // Partial by construction: it repairs what is mounted and subscribed. Canvas raster and + // control layers hold image names without a DTO query behind them, and only `handleDeletions` + // prunes those — which needs a definitive per-name outcome from the server (#9533). + ...getTagsToInvalidateForImageMutation(result.failed_images), + ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), + 'ImageCollectionCounts', + { type: 'ImageCollection', id: LIST_TAG }, +]; + +/** + * A failed name is refetched, not ignored — the star routes can fail *after* committing. + * `ImageService.update` writes the record and then reads the DTO back to return it; a failure in + * that read (or in the event emit that follows) leaves the row starred while the route reports + * the name in `failed_images`. Invalidating only the successes would leave the client showing + * the pre-star value indefinitely, since nothing else will contradict it. Names folded in + * client-side for chunks that never went out ride along; their refetch merely confirms the cache. + */ +const getStarImagesTags = (result: components['schemas']['StarredImagesResult']): InvalidateTagsArg => [ + ...getTagsToInvalidateForImageMutation(result.starred_images), + ...getTagsToInvalidateForImageMutation(result.failed_images), + ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), + 'ImageCollectionCounts', + { type: 'ImageCollection', id: 'starred' }, + { type: 'ImageCollection', id: 'unstarred' }, +]; + +/** See `getStarImagesTags` for why the failed names are invalidated too. */ +const getUnstarImagesTags = (result: components['schemas']['UnstarredImagesResult']): InvalidateTagsArg => [ + ...getTagsToInvalidateForImageMutation(result.unstarred_images), + ...getTagsToInvalidateForImageMutation(result.failed_images), + ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), + 'ImageCollectionCounts', + { type: 'ImageCollection', id: 'starred' }, + { type: 'ImageCollection', id: 'unstarred' }, +]; + +const getAddImagesToBoardTags = (result: components['schemas']['AddImagesToBoardResult']): InvalidateTagsArg => [ + ...getTagsToInvalidateForImageMutation(result.added_images), + ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), +]; + +const getRemoveImagesFromBoardTags = ( + result: components['schemas']['RemoveImagesFromBoardResult'] +): InvalidateTagsArg => [ + ...getTagsToInvalidateForImageMutation(result.removed_images), + // A name the zero-row classification reports as failed sits on a board this client did not + // expect — refetching its DTO is what shows where it actually went. Names folded in + // client-side for unreached chunks ride along; their refetch merely confirms the cache. + ...getTagsToInvalidateForImageMutation(result.failed_images), + ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), +]; + +/** + * Publishes fetched DTOs into the individual `getImageDTO` caches, which is where every + * subscribed component reads from — this batch endpoint holds no cache of its own. + */ +const upsertImageDTOs = (dispatch: Dispatch, imageDTOs: ImageDTO[]) => { + if (imageDTOs.length === 0) { + return; + } + const updates: Param0 = []; + for (const imageDTO of imageDTOs) { + updates.push({ + endpointName: 'getImageDTO', + arg: imageDTO.image_name, + value: imageDTO, + }); + } + dispatch(imagesApi.util.upsertQueryEntries(updates)); +}; + +/** + * Surfaces the partial-failure warning. Used for both kinds of partial failure: the names the + * server reported it could not apply, and the names a chunked run never reached. + */ +const toastFailedImages = (count: number) => { + if (count > 0) { + toast({ + id: 'IMAGES_FAILED_TO_UPDATE', + title: i18n.t('toast.imagesFailedToUpdate', { count }), + status: 'warning', + }); + } +}; + +/** Reports a whole batch that failed before its first request could commit. */ +export const toastFailedImageBatch = (image_names: string[]) => { + toastFailedImages(new Set(image_names).size); +}; + +/** + * The outcome handler shared by all five batch mutations, which report failure identically. + * + * Both branches matter and neither is reachable from anywhere else. `handleDeletions` and its + * siblings swallow every outcome, so without the fulfilled branch a run that only partly landed + * — server-side per-name failures, or chunks a mid-run failure never reached — says nothing at + * all. And on the ordinary failure path `buildChunkedImageBatchQueryFn` returns an error only + * when `results.length === 0`, i.e. the first chunk failed and the server committed nothing, so + * the whole argument list is genuinely unapplied and reporting all of it is exact rather than an + * over-count. Nothing else covers that case: these five endpoints have no `matchRejected` + * listener, unlike the single-image board routes. + * + * Three other paths reach this branch with chunks already committed, and the session check is + * what makes each safe. A *takeover* aborts the run as an error however much landed — + * deliberately, so the new session cannot consume the old one's aggregate — and the check below + * is why that does not surface as the previous user's count. An *expiry* mid-run resolves with + * partial data instead (see `SESSION_ENDED_ERROR`), so it lands on the fulfilled branch, where + * the same check keeps the count off the login screen while `handleDeletions` still does the + * state work off the partial payload. (The queryFn's own `invalidateTags` also runs, but under + * a same-tab expiry `resetApiState` has already emptied the store by then, so it is a no-op — + * the pruning is the part that matters.) And the queryFn can *throw* on the mid-run path + * (`getTags(merged)` and `merged.failed_images.concat(...)` both read keys straight off a + * server payload), which would over-count; that needs a response missing a documented key, so + * it is left as an over-report rather than a silence. + * + * Extracted rather than repeated inline five times so that the wiring is one unit a test can + * hold — an endpoint that swallows its rejection looks identical to one that reports it, and + * that difference is invisible to a test of the toast helpers alone. + */ +export const reportImageBatchOutcome = async ( + { image_names }: { image_names: string[] }, + { queryFulfilled }: { queryFulfilled: Promise<{ data: { failed_images: string[] } }> } +) => { + const authContext = captureAuthContext(); + try { + const { data: result } = await queryFulfilled; + if (!isSameAuthContext(authContext)) { + return; + } + toastFailedImages(result.failed_images.length); + } catch { + if (!isSameAuthContext(authContext)) { + return; + } + toastFailedImageBatch(image_names); + } +}; + +/** + * The download counterpart. Distinct id and wording: "could not be updated" is wrong for a + * download, and sharing the id would let one warning overwrite the other, since the toast + * system updates in place. + */ +const toastFailedDownloads = (count: number) => { + if (count > 0) { + toast({ + id: 'IMAGES_FAILED_TO_DOWNLOAD', + title: i18n.t('toast.imagesFailedToDownload', { count }), + status: 'warning', + }); + } +}; + +/** + * Runs `/images/download` one conforming chunk at a time. + * + * Chunked like the mutating batch routes, but it cannot merge: each request produces its own + * bulk-download item, so an oversized selection becomes several zips rather than one. That is + * the cost of keeping the selection downloadable at all — the alternative is a 422 the moment + * the user hits select-all on a board past the cap. + * + * Only the first item name is returned, and only to give `matchFulfilled` a name for the single + * "preparing" toast. The zips themselves arrive independently: each background task emits its + * own `bulk_download_complete`, and the socket handler fetches and saves per event, keyed on the + * item name in the event rather than on this payload. + * + * A mid-run failure therefore cannot be reported as a plain rejection. The route answers 202 the + * moment it has scheduled the background task, so every chunk before the failing one is already + * producing a zip that will land in the user's downloads. Rejecting drives `matchRejected` and + * toasts "problem preparing download" while those zips arrive anyway — the opposite of what + * happened. So, exactly as in `buildChunkedImageBatchQueryFn`, only a run where *nothing* was + * scheduled surfaces as an error; a partial run resolves and reports the names that never made + * it into any zip. + */ +export const bulkDownloadQueryFn = async ( + { image_names, board_id }: components['schemas']['Body_download_images_from_list'], + _api: unknown, + _extraOptions: unknown, + baseQuery: ImagesBaseQuery +) => { + // A board download expands server-side from board_id alone, so there is nothing to split — + // and the server picks board_id over image_names when both are set (`BulkDownloadService`), + // which makes a body carrying both a board download too, not a selection to chunk. Splitting + // it would ask for the same full-board zip once per chunk: 1001 names alongside a board id + // would schedule two identical full-board jobs. Normalized here to the one request the server + // is actually going to honour, so the client cannot amplify a download by sending a field the + // server ignores. No UI caller sends both today; the endpoint accepts it. + const chunks: (string[] | undefined)[] = board_id ? [undefined] : chunkImageNames(image_names ?? []); + const authContext = captureAuthContext(); + // Tracked separately from the payload rather than inferred from it. `fetchBaseQuery` resolves + // an empty response entity as `data: null`, so a 202 whose body did not survive the trip back + // leaves nothing to return even though the background task WAS scheduled and its zip is + // already being built. Any nullish test over the payload then sends us down the + // nothing-happened path and toasts a failure over an arriving download. + let scheduled = false; + let first: components['schemas']['ImagesDownloaded'] | undefined; + for (const [index, chunk] of chunks.entries()) { + const response = await fetchChunk(baseQuery, authContext, { + url: buildImagesUrl('download'), + method: 'POST', + body: { image_names: chunk, board_id }, + }); + if (response.error) { + // The mismatch errors from the pre/post-request checks, AND any error response that came + // back into a session that is no longer the one that asked — which is how this chunk's + // OWN expired-session 401 arrives: `dynamicBaseQuery` dispatches `sessionExpiredLogout` + // before returning it, so the token is already gone. (It declines to when the token has + // since been replaced — a takeover, caught by the same check here, or a sliding-window + // refresh, where the session is alive and this chunk is an ordinary failed request.) + // The mutating loops deliberately do + // NOT reclassify that 401 — they need it on the partial path, where the payload feeds + // `handleDeletions` — but a download has no state work to salvage, and its usual outputs + // are both wrong for a session that is ending: the failure count is a miscount at the + // login screen, and returning `first` drives `matchFulfilled` into raising the + // "preparing" toast with `duration: null` — dismissed only by a socket event this + // session will never receive. + if (isSessionMismatchError(response.error) || !isSameAuthContext(authContext)) { + // One thing IS lost on expiry and deserves saying so: zips already scheduled keep + // building server-side, but their completion events fire into a socket this session is + // tearing down, so nothing will ever offer them. Until scheduled downloads are queued + // per account and replayed after re-authentication (follow-up), the honest move is a + // plain, finite toast telling the user to re-run — not silence they will read as a + // download that never came. A takeover (someone else's token in localStorage) stays + // fully silent: the interruption belongs to whoever started the download, never to the + // user who takes the tab over. And an expiry with nothing scheduled has lost nothing. + if (localStorage.getItem('auth_token') === null && scheduled) { + toast({ + id: 'DOWNLOADS_INTERRUPTED', + title: i18n.t('toast.downloadsInterrupted'), + status: 'warning', + }); + } + return { data: undefined as unknown as components['schemas']['ImagesDownloaded'] }; + } + if (!scheduled) { + // Nothing was scheduled, so this is an ordinary failed request — report it as one and + // let the `matchRejected` listener raise the failure toast. + return { error: response.error }; + } + // Everything from this chunk on is unreached: those names are in no zip. The failing + // chunk counts too — it was reached, but nothing was scheduled for it. Toasted here + // rather than folded into the payload because `ImagesDownloaded` has no per-name failure + // list, and unlike the mutating routes this endpoint has no `onQueryStarted` that would + // make this a second reporting site for the same toast id. + toastFailedDownloads(chunks.slice(index).flatMap((names) => names ?? []).length); + // Cast, like the return below: `first` is nullish only when a 202 came back with no body + // (see `scheduled`), and ImagesDownloaded has no way to say "scheduled, but the body did + // not survive the trip". The fulfilled-action listener reads the payload through + // optionals for exactly that reason. + return { data: first as components['schemas']['ImagesDownloaded'] }; + } + scheduled = true; + first ??= response.data as components['schemas']['ImagesDownloaded']; + } + // The final chunk has no next iteration to catch an expiry for it. `fetchChunk`'s own + // post-response check deliberately passes a mere expiry through -- the mutating loops need + // that -- so a token cleared during the last chunk's await (a 401 on any concurrent request + // does it) would otherwise sail into the return below, and `matchFulfilled` would raise the + // `duration: null` "preparing" toast into a session whose socket will never deliver the + // dismissal -- or the zips. Same triage as the in-loop expiry arm: say what was lost, and + // hand `matchFulfilled` nothing to toast on. Synchronous from here to the return, so there + // is no later window this check misses. + if (!isSameAuthContext(authContext)) { + if (localStorage.getItem('auth_token') === null && scheduled) { + toast({ + id: 'DOWNLOADS_INTERRUPTED', + title: i18n.t('toast.downloadsInterrupted'), + status: 'warning', + }); + } + return { data: undefined as unknown as components['schemas']['ImagesDownloaded'] }; + } + return { data: first as components['schemas']['ImagesDownloaded'] }; +}; + +/** + * Fetches DTOs for a list of image names, one conforming chunk at a time. + * + * Chunked despite being a read: `useRangeBasedImageFetching` unions every virtuoso range seen + * inside its throttle window, and a dense grid with a 4096px overscan can carry that past the + * cap. The failure was silent — the hook swallows the rejection and nothing listens for it, so + * the affected thumbnails simply never loaded. + * + * Each chunk is published into the individual `getImageDTO` caches as it arrives, rather than + * all of them at the end. This mutation rejects on any chunk failure and its one caller never + * looks at the rejection, so a transient failure late in a wide range would otherwise throw + * away every thumbnail the earlier chunks had already fetched — and they would not be + * re-requested either, since the hook asks only for names it cannot find in the cache and the + * next request is driven by scrolling, not by the failure. + */ +export const imageDTOsByNamesQueryFn = async ( + { image_names }: components['schemas']['Body_get_images_by_names'], + // Typed as the plain store dispatch rather than against `imagesApi.util.upsertQueryEntries`: + // naming imagesApi in a signature the endpoint definitions themselves depend on makes its + // inferred type circular, and every consumer of the api silently degrades to `any`. + { dispatch }: { dispatch: Dispatch }, + _extraOptions: unknown, + baseQuery: ImagesBaseQuery +) => { + const authContext = captureAuthContext(); + const imageDTOs: ImageDTO[] = []; + for (const chunk of chunkImageNames(image_names)) { + const response = await fetchChunk(baseQuery, authContext, { + url: buildImagesUrl('images_by_names'), + method: 'POST', + body: { image_names: chunk }, + }); + if (response.error) { + return { error: response.error }; + } + const chunkDTOs = response.data as ImageDTO[]; + // Re-checked here as well as inside `fetchChunk`, and not folded into it: `fetchChunk` is + // async, so resuming from it is a microtask hop, and a logout that lands in that hop passes + // its check and still clears the cache before this line. Only a check with no await between + // it and the write closes that. Publishing then would seed one user's cache with another's + // images, into a store the logout listener has just reset. + if (isSameAuthContext(authContext)) { + upsertImageDTOs(dispatch, chunkDTOs); + } + imageDTOs.push(...chunkDTOs); + } + return { data: imageDTOs }; +}; + export const imagesApi = api.injectEndpoints({ endpoints: (build) => ({ /** @@ -131,24 +736,22 @@ export const imagesApi = api.injectEndpoints({ paths['/api/v1/images/delete']['post']['responses']['200']['content']['application/json'], paths['/api/v1/images/delete']['post']['requestBody']['content']['application/json'] >({ - query: (body) => ({ - url: buildImagesUrl('delete'), - method: 'POST', - body, - }), - invalidatesTags: (result) => { - if (!result) { - return []; - } - // We ignore the deleted images when getting tags to invalidate. If we did not, we will invalidate the queries - // that fetch image DTOs, metadata, and workflows. But we have just deleted those images! Invalidating the tags - // will force those queries to re-fetch, and the requests will of course 404. - return [ - ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), - 'ImageCollectionCounts', - { type: 'ImageCollection', id: LIST_TAG }, - ]; - }, + queryFn: buildChunkedImageBatchQueryFn( + () => ({ url: buildImagesUrl('delete'), method: 'POST' }), + getDeleteImagesTags, + // The only one of these that lists its names twice, because a lost delete is the only + // one whose two readings call for different tags. As committed, the names drive the + // board and collection counts. As unconfirmed — which is the truth, and what + // `failed_images` means here — their DTOs are re-asked, and that is the whole + // reconciliation: a name that is really gone answers 404 and the components holding it + // let go. Reporting them only as deleted would invalidate neither, since + // `getDeleteImagesTags` deliberately skips the DTOs of names it believes are gone. This + // is also the only path a single-chunk delete has — anything up to the batch cap is one + // request, so it never reaches the mid-run merge below. + (image_names) => ({ deleted_images: image_names, failed_images: image_names, affected_boards: [] }) + ), + onQueryStarted: reportImageBatchOutcome, + invalidatesTags: (result) => (result ? getDeleteImagesTags(result) : []), }), deleteUncategorizedImages: build.mutation< paths['/api/v1/images/uncategorized']['delete']['responses']['200']['content']['application/json'], @@ -198,23 +801,13 @@ export const imagesApi = api.injectEndpoints({ paths['/api/v1/images/star']['post']['responses']['200']['content']['application/json'], paths['/api/v1/images/star']['post']['requestBody']['content']['application/json'] >({ - query: (body) => ({ - url: buildImagesUrl('star'), - method: 'POST', - body, - }), - invalidatesTags: (result) => { - if (!result) { - return []; - } - return [ - ...getTagsToInvalidateForImageMutation(result.starred_images), - ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), - 'ImageCollectionCounts', - { type: 'ImageCollection', id: 'starred' }, - { type: 'ImageCollection', id: 'unstarred' }, - ]; - }, + queryFn: buildChunkedImageBatchQueryFn( + () => ({ url: buildImagesUrl('star'), method: 'POST' }), + getStarImagesTags, + (image_names) => ({ starred_images: image_names, failed_images: [], affected_boards: [] }) + ), + onQueryStarted: reportImageBatchOutcome, + invalidatesTags: (result) => (result ? getStarImagesTags(result) : []), }), /** * Unstar a list of images. @@ -223,23 +816,13 @@ export const imagesApi = api.injectEndpoints({ paths['/api/v1/images/unstar']['post']['responses']['200']['content']['application/json'], paths['/api/v1/images/unstar']['post']['requestBody']['content']['application/json'] >({ - query: (body) => ({ - url: buildImagesUrl('unstar'), - method: 'POST', - body, - }), - invalidatesTags: (result) => { - if (!result) { - return []; - } - return [ - ...getTagsToInvalidateForImageMutation(result.unstarred_images), - ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), - 'ImageCollectionCounts', - { type: 'ImageCollection', id: 'starred' }, - { type: 'ImageCollection', id: 'unstarred' }, - ]; - }, + queryFn: buildChunkedImageBatchQueryFn( + () => ({ url: buildImagesUrl('unstar'), method: 'POST' }), + getUnstarImagesTags, + (image_names) => ({ unstarred_images: image_names, failed_images: [], affected_boards: [] }) + ), + onQueryStarted: reportImageBatchOutcome, + invalidatesTags: (result) => (result ? getUnstarImagesTags(result) : []), }), uploadImage: build.mutation< paths['/api/v1/images/upload']['post']['responses']['201']['content']['application/json'], @@ -391,6 +974,9 @@ export const imagesApi = api.injectEndpoints({ } return [ ...getTagsToInvalidateForImageMutation(result.removed_images), + // A name the zero-row classification reported as failed sits on a board this client + // did not expect; refetching its DTO is what shows where it actually went. + ...getTagsToInvalidateForImageMutation(result.failed_images), ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), ]; }, @@ -399,52 +985,31 @@ export const imagesApi = api.injectEndpoints({ paths['/api/v1/board_images/batch']['post']['responses']['201']['content']['application/json'], paths['/api/v1/board_images/batch']['post']['requestBody']['content']['application/json'] >({ - query: (body) => ({ - url: buildBoardImagesUrl('batch'), - method: 'POST', - body, - }), - invalidatesTags: (result) => { - if (!result) { - return []; - } - return [ - ...getTagsToInvalidateForImageMutation(result.added_images), - ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), - ]; - }, + queryFn: buildChunkedImageBatchQueryFn( + () => ({ url: buildBoardImagesUrl('batch'), method: 'POST' }), + getAddImagesToBoardTags, + (image_names, arg) => ({ added_images: image_names, failed_images: [], affected_boards: [arg.board_id] }) + ), + onQueryStarted: reportImageBatchOutcome, + invalidatesTags: (result) => (result ? getAddImagesToBoardTags(result) : []), }), removeImagesFromBoard: build.mutation< paths['/api/v1/board_images/batch/delete']['post']['responses']['201']['content']['application/json'], paths['/api/v1/board_images/batch/delete']['post']['requestBody']['content']['application/json'] >({ - query: (body) => ({ - url: buildBoardImagesUrl('batch/delete'), - method: 'POST', - body, - }), - invalidatesTags: (result) => { - if (!result) { - return []; - } - return [ - ...getTagsToInvalidateForImageMutation(result.removed_images), - ...getTagsToInvalidateForBoardAffectingMutation(result.affected_boards), - ]; - }, + queryFn: buildChunkedImageBatchQueryFn( + () => ({ url: buildBoardImagesUrl('batch/delete'), method: 'POST' }), + getRemoveImagesFromBoardTags, + (image_names) => ({ removed_images: image_names, failed_images: [], affected_boards: [] }) + ), + onQueryStarted: reportImageBatchOutcome, + invalidatesTags: (result) => (result ? getRemoveImagesFromBoardTags(result) : []), }), bulkDownloadImages: build.mutation< components['schemas']['ImagesDownloaded'], components['schemas']['Body_download_images_from_list'] >({ - query: ({ image_names, board_id }) => ({ - url: buildImagesUrl('download'), - method: 'POST', - body: { - image_names, - board_id, - }, - }), + queryFn: bulkDownloadQueryFn, }), /** * Get ordered list of image names for selection operations @@ -467,30 +1032,9 @@ export const imagesApi = api.injectEndpoints({ paths['/api/v1/images/images_by_names']['post']['responses']['200']['content']['application/json'], paths['/api/v1/images/images_by_names']['post']['requestBody']['content']['application/json'] >({ - query: (body) => ({ - url: buildImagesUrl('images_by_names'), - method: 'POST', - body, - }), - // Don't provide cache tags - we'll manually upsert into individual getImageDTO caches - async onQueryStarted(_, { dispatch, queryFulfilled }) { - try { - const { data: imageDTOs } = await queryFulfilled; - - // Upsert each DTO into the individual image cache - const updates: Param0 = []; - for (const imageDTO of imageDTOs) { - updates.push({ - endpointName: 'getImageDTO', - arg: imageDTO.image_name, - value: imageDTO, - }); - } - dispatch(imagesApi.util.upsertQueryEntries(updates)); - } catch { - // Handle error if needed - } - }, + queryFn: imageDTOsByNamesQueryFn, + // No cache tags: the DTOs are upserted into the individual getImageDTO caches by the + // queryFn, chunk by chunk, which is also all an onQueryStarted handler would have done. }), }), }); diff --git a/invokeai/frontend/web/src/services/api/index.ts b/invokeai/frontend/web/src/services/api/index.ts index 92832823c16..36574d66723 100644 --- a/invokeai/frontend/web/src/services/api/index.ts +++ b/invokeai/frontend/web/src/services/api/index.ts @@ -17,6 +17,7 @@ import { MEDIA_COOKIE_SYNC_TIMEOUT_MS, runWithMediaAuthLock, shouldAcceptRefreshedToken, + shouldEndSessionForUnauthorized, } from 'features/auth/store/authTokenRefresh'; import queryString from 'query-string'; import stableHash from 'stable-hash'; @@ -93,7 +94,7 @@ export const getBaseUrl = (): string => { return getDeploymentBaseUrl(); }; -const dynamicBaseQuery: BaseQueryFn = async ( +export const dynamicBaseQuery: BaseQueryFn = async ( args, api, extraOptions @@ -138,10 +139,12 @@ const dynamicBaseQuery: BaseQueryFn rawBaseQuery(args, api, extraOptions); const result = changesMediaCookie ? await runWithMediaAuthLock(execute) : await execute(); - // If we sent an auth token but got 401, the token is invalid/expired. - // Only trigger session expiry when we actually sent a token — unauthenticated - // requests (e.g. client_state queries during page load) should not cause logout. - if (result.error && result.error.status === 401 && !isAuthEndpoint && token) { + // If we sent an auth token but got 401, the token is invalid/expired. Only trigger session + // expiry when we actually sent a token — unauthenticated requests (e.g. client_state queries + // during page load) should not cause logout — and only while that token is still the live one, + // so a slow request cannot log out the session that replaced its own. See + // `shouldEndSessionForUnauthorized`. + if (result.error && result.error.status === 401 && !isAuthEndpoint && shouldEndSessionForUnauthorized(token)) { api.dispatch(sessionExpiredLogout()); } diff --git a/invokeai/frontend/web/src/services/api/schema.ts b/invokeai/frontend/web/src/services/api/schema.ts index 14d26264807..c19832d09a1 100644 --- a/invokeai/frontend/web/src/services/api/schema.ts +++ b/invokeai/frontend/web/src/services/api/schema.ts @@ -3307,6 +3307,11 @@ export type components = { * @description The image names that were added to the board */ added_images: string[]; + /** + * Failed Images + * @description The names of authorized images that could not be added + */ + failed_images: string[]; }; /** * Add Integers @@ -9164,7 +9169,7 @@ export type components = { * Failed Images * @description The names of authorized images that could not be deleted */ - failed_images?: string[]; + failed_images: string[]; }; /** * DeleteOrphanedModelsRequest @@ -33151,6 +33156,11 @@ export type components = { * @description The image names that were removed from their board */ removed_images: string[]; + /** + * Failed Images + * @description The names of authorized images that could not be removed + */ + failed_images: string[]; }; /** RemoveVideosFromBoardResult */ RemoveVideosFromBoardResult: { @@ -35529,6 +35539,11 @@ export type components = { * @description The names of the images that were starred */ starred_images: string[]; + /** + * Failed Images + * @description The names of images that were not starred + */ + failed_images: string[]; }; /** StarredVideosResult */ StarredVideosResult: { @@ -38134,6 +38149,11 @@ export type components = { * @description The names of the images that were unstarred */ unstarred_images: string[]; + /** + * Failed Images + * @description The names of images that were not unstarred + */ + failed_images: string[]; }; /** UnstarredVideosResult */ UnstarredVideosResult: { diff --git a/invokeai/frontend/web/src/services/api/util/imageErrors.test.ts b/invokeai/frontend/web/src/services/api/util/imageErrors.test.ts new file mode 100644 index 00000000000..5765308f7c4 --- /dev/null +++ b/invokeai/frontend/web/src/services/api/util/imageErrors.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest'; + +import { isImageMissingError } from './imageErrors'; + +describe('isImageMissingError', () => { + it('is true for a 404 — the image is confirmed gone', () => { + expect(isImageMissingError({ status: 404, data: { detail: 'not found' } })).toBe(true); + }); + + it.each([ + // The one that matters most: revoking access to a shared board answers 403 for images that + // are all still there. Clearing on it would destroy the workflows pointing at them, and + // restoring the permission would not bring them back. The server answers 404 when an image + // is actually gone, so this arm costs nothing. + ['denied (403)', { status: 403, data: { detail: 'Not authorized' } }], + ['unauthorized (401)', { status: 401, data: {} }], + // A name lands in `failed_images` because a storage failure interrupted its write, and the + // refetch that the star/unstar invalidation triggers hits the same unwell store. + ['server error (500)', { status: 500, data: {} }], + ['network failure', { status: 'FETCH_ERROR', error: 'TypeError: Failed to fetch' }], + ['timeout', { status: 'TIMEOUT_ERROR', error: 'AbortError' }], + ['parsing failure', { status: 'PARSING_ERROR', originalStatus: 200, data: '', error: 'oops' }], + ['no error', undefined], + ])('is false for %s — the reference must be preserved', (_label, error) => { + expect(isImageMissingError(error)).toBe(false); + }); +}); diff --git a/invokeai/frontend/web/src/services/api/util/imageErrors.ts b/invokeai/frontend/web/src/services/api/util/imageErrors.ts new file mode 100644 index 00000000000..761cf5e7ab2 --- /dev/null +++ b/invokeai/frontend/web/src/services/api/util/imageErrors.ts @@ -0,0 +1,27 @@ +/** + * True only for a confirmed "this image no longer exists" (HTTP 404). + * + * Several components drop the user's reference when the image behind it is gone — a node's + * image field, a global reference image, a regional guidance reference image. That reset is + * silent and has no undo, so it may only follow an answer that is both definite and permanent. + * + * A 403 is neither, which is why it is excluded even though it is the answer a deleted image + * *used* to produce for a non-admin. Access is revoked and restored: flip a board to Private + * and every reference to its images would clear; flip it back and the images are all still + * there, but the workflows that pointed at them are not. The server draws the distinction + * instead — `assert_image_read_access` answers 404 for an image positively absent and 403 only + * for one it is refusing — and that is what makes 404 alone the right test here. + * + * Everything else is indeterminate and must NOT discard the user's input: a transient network + * failure (`FETCH_ERROR`), a timeout, a parse failure, a 401 or a 5xx says nothing about + * whether the image exists. That became load-bearing when the star/unstar mutations began + * invalidating the DTOs of names reported in `failed_images`: a name is reported there precisely + * because a storage failure interrupted its write, so the refetch the invalidation triggers is + * running against a store that is already unwell and is likelier than usual to answer 500. + * + * The server side carries the matching obligation, met in `image_records.get`: a storage error + * must never be translated into not-found, or an unreadable database would present as a + * deleted image and take the user's references down with it. + */ +export const isImageMissingError = (error: unknown): boolean => + error instanceof Object && 'status' in error && error.status === 404; diff --git a/tests/app/routers/test_board_images_batch_races.py b/tests/app/routers/test_board_images_batch_races.py new file mode 100644 index 00000000000..90c05dc0063 --- /dev/null +++ b/tests/app/routers/test_board_images_batch_races.py @@ -0,0 +1,314 @@ +"""Race handling on the board_images batch routes. + +Two races found in review, both between a batch loop's read and its write: + +- The scoped DELETE in the batch remove can match zero rows when a concurrent session moves the + image between the DTO read and the write. The row count is the only signal the scope held, and + the classification depends on where the image went. +- The per-name destination re-check in the batch add can start refusing mid-batch when the + destination board is revoked or deleted. That refusal is the request's problem, not the + name's: treated as a skip it answers 201 with empty lists, which the client reads as success + and clears the user's selection over. +""" + +from unittest.mock import MagicMock + +import pytest +from fastapi import HTTPException +from fastapi.testclient import TestClient + +from invokeai.app.api.dependencies import ApiDependencies +from invokeai.app.api_app import app +from invokeai.app.services.invoker import Invoker + + +class MockApiDependencies(ApiDependencies): + invoker: Invoker + + def __init__(self, invoker) -> None: + self.invoker = invoker + + +@pytest.fixture +def client() -> TestClient: + return TestClient(app) + + +def _install(monkeypatch: pytest.MonkeyPatch, mock_invoker: Invoker) -> None: + mock_deps = MockApiDependencies(mock_invoker) + # Several of these are None on the conftest's mock services; the routes need whole service + # doubles, installed via monkeypatch so they are restored between tests. + for name in ("image_moves", "images", "image_records", "board_images", "board_image_records", "board_records"): + monkeypatch.setattr(mock_invoker.services, name, MagicMock()) + mock_invoker.services.image_moves.is_maintenance_active.return_value = False + monkeypatch.setattr("invokeai.app.api.routers.board_images.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers.image_move_maintenance.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) + + +@pytest.mark.parametrize( + ("now_on_board", "record_exists", "expect_removed", "expect_failed", "expect_boards"), + [ + # Moved to another board mid-batch: the ask -- off every board -- is not satisfied, and + # a retry will re-read and re-authorize against the board it actually sits on now. + ("board-q", True, [], ["raced.png"], []), + # Concurrently uncategorized by someone else: the postcondition holds, and reporting it + # removed is what lets this client's stale view of the old board catch up. Safe in + # removed_images, unlike a deleted name: the DTO exists, so tag refetches succeed. + (None, True, ["raced.png"], [], ["none", "board-p"]), + # Deleted concurrently: a skip, matching the route's existing treatment of a name that + # vanished before the loop reached it. removed_images would drive a 404 refetch. + (None, False, [], [], []), + ], +) +def test_remove_classifies_a_zero_row_scoped_delete_by_where_the_image_went( + monkeypatch: pytest.MonkeyPatch, + mock_invoker: Invoker, + client: TestClient, + now_on_board: str | None, + record_exists: bool, + expect_removed: list[str], + expect_failed: list[str], + expect_boards: list[str], +) -> None: + _install(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = "board-p" + monkeypatch.setattr(mock_invoker.services.images, "get_dto", MagicMock(return_value=dto)) + # The default single-user identity is an admin, so the write-access check passes without + # touching board storage -- which is fine: the subject here is the write, not the check. + monkeypatch.setattr(mock_invoker.services.board_images, "remove_image_from_board", MagicMock(return_value=0)) + monkeypatch.setattr( + mock_invoker.services.board_image_records, "get_board_for_image", MagicMock(return_value=now_on_board) + ) + if record_exists: + monkeypatch.setattr(mock_invoker.services.image_records, "get", MagicMock(return_value=MagicMock())) + else: + from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException + + monkeypatch.setattr( + mock_invoker.services.image_records, "get", MagicMock(side_effect=ImageRecordNotFoundException) + ) + + response = client.post("/api/v1/board_images/batch/delete", json={"image_names": ["raced.png"]}) + + assert response.status_code == 201 + body = response.json() + assert body["removed_images"] == expect_removed + assert body["failed_images"] == expect_failed + assert set(body["affected_boards"]) == set(expect_boards) + + +def test_remove_reports_a_zero_row_name_whose_existence_cannot_be_decided_as_failed( + monkeypatch: pytest.MonkeyPatch, mock_invoker: Invoker, client: TestClient +) -> None: + """A transient storage error during the existence probe must not manufacture a success: + reported removed, the client's tag-driven getImageDTO refetch 404s if the image was in + fact deleted. A name whose state cannot be decided is reported as failed, never as done.""" + import sqlite3 + + _install(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = "board-p" + monkeypatch.setattr(mock_invoker.services.images, "get_dto", MagicMock(return_value=dto)) + monkeypatch.setattr(mock_invoker.services.board_images, "remove_image_from_board", MagicMock(return_value=0)) + monkeypatch.setattr(mock_invoker.services.board_image_records, "get_board_for_image", MagicMock(return_value=None)) + monkeypatch.setattr( + mock_invoker.services.image_records, + "get", + MagicMock(side_effect=sqlite3.OperationalError("database is locked")), + ) + + response = client.post("/api/v1/board_images/batch/delete", json={"image_names": ["undecidable.png"]}) + + assert response.status_code == 201 + body = response.json() + assert body["removed_images"] == [] + assert body["failed_images"] == ["undecidable.png"] + + +def test_facade_passes_the_row_count_through() -> None: + """The middle layer: CI has no type checker, so a dropped `return` here silently restores + report-a-removal-that-did-not-happen at the route (`None == 0` is False), while the route + tests mock this service and the sqlite test pins the layer below it.""" + from invokeai.app.services.board_images.board_images_default import BoardImagesService + + service = BoardImagesService.__new__(BoardImagesService) + invoker = MagicMock() + invoker.services.board_image_records.remove_image_from_board = MagicMock(return_value=0) + service._BoardImagesService__invoker = invoker # pyright: ignore[reportAttributeAccessIssue] + + assert service.remove_image_from_board("raced.png", "board-p") == 0 + + +def test_remove_still_reports_a_nonzero_scoped_delete_as_removed( + monkeypatch: pytest.MonkeyPatch, mock_invoker: Invoker, client: TestClient +) -> None: + _install(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = "board-p" + monkeypatch.setattr(mock_invoker.services.images, "get_dto", MagicMock(return_value=dto)) + monkeypatch.setattr(mock_invoker.services.board_images, "remove_image_from_board", MagicMock(return_value=1)) + + response = client.post("/api/v1/board_images/batch/delete", json={"image_names": ["ok.png"]}) + + assert response.status_code == 201 + body = response.json() + assert body["removed_images"] == ["ok.png"] + assert body["failed_images"] == [] + assert set(body["affected_boards"]) == {"none", "board-p"} + + +def test_add_reports_names_refused_by_a_destination_revoked_mid_batch( + monkeypatch: pytest.MonkeyPatch, mock_invoker: Invoker, client: TestClient +) -> None: + """A revoked destination fails the remaining names; it must not empty into a silent 201.""" + _install(monkeypatch, mock_invoker) + monkeypatch.setattr(mock_invoker.services.image_records, "get_user_id", MagicMock(return_value="system")) + monkeypatch.setattr(mock_invoker.services.board_image_records, "get_board_for_image", MagicMock(return_value=None)) + monkeypatch.setattr(mock_invoker.services.board_images, "add_image_to_board", MagicMock(return_value=None)) + # Pre-loop check passes, first per-name check passes, then the board flips Private (or is + # deleted): every later re-check refuses. Patched at the route seam because the default + # single-user identity is an admin, for whom the real helper never refuses. + calls = {"n": 0} + + def write_access(board_id: str, current_user: object) -> None: + calls["n"] += 1 + if calls["n"] > 2: + raise HTTPException(status_code=403, detail="Not authorized to modify this board") + + monkeypatch.setattr("invokeai.app.api.routers.board_images._assert_board_write_access", write_access) + + response = client.post( + "/api/v1/board_images/batch", + json={"board_id": "board-x", "image_names": ["first.png", "second.png", "third.png"]}, + ) + + assert response.status_code == 201 + body = response.json() + assert body["added_images"] == ["first.png"] + # The refused names are failures the client can toast and retry -- not skips that leave a + # 201 with empty lists for the UI to read as success. + assert set(body["failed_images"]) == {"second.png", "third.png"} + + +def test_sqlite_remove_returns_the_row_count() -> None: + """The storage layer itself: the row count is the only signal the scoped DELETE's scope + held, and a `None` return silently restores report-a-removal-that-did-not-happen at the + route (`None == 0` is False). Stubbed at the cursor in the manner of the image-records + storage tests.""" + from invokeai.app.services.board_image_records.board_image_records_sqlite import SqliteBoardImageRecordStorage + + storage = SqliteBoardImageRecordStorage.__new__(SqliteBoardImageRecordStorage) + + class _Cursor: + rowcount = 0 + + def execute(self, *args: object, **kwargs: object) -> None: + pass + + class _Db: + def transaction(self): + from contextlib import contextmanager + + @contextmanager + def _cm(): + yield _Cursor() + + return _cm() + + storage._db = _Db() # pyright: ignore[reportAttributeAccessIssue] + + assert storage.remove_image_from_board("raced.png", "board-p") == 0 + _Cursor.rowcount = 1 + assert storage.remove_image_from_board("ok.png", "board-p") == 1 + + +@pytest.mark.parametrize( + ("now_on_board", "record_exists", "expect_removed", "expect_failed", "expect_boards"), + [ + # Same three classifications as the batch loop above -- the single-image route runs + # the identical read-then-scoped-DELETE sequence, so it loses the identical race. It + # used to ignore the row count entirely and answer removed_images=[name] for all three + # of these, a false success the client had no way to see through. + ("board-q", True, [], ["raced.png"], []), + (None, True, ["raced.png"], [], ["none", "board-p"]), + (None, False, [], [], []), + ], +) +def test_single_remove_classifies_a_zero_row_scoped_delete_like_the_batch_route( + monkeypatch: pytest.MonkeyPatch, + mock_invoker: Invoker, + client: TestClient, + now_on_board: str | None, + record_exists: bool, + expect_removed: list[str], + expect_failed: list[str], + expect_boards: list[str], +) -> None: + _install(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = "board-p" + monkeypatch.setattr(mock_invoker.services.images, "get_dto", MagicMock(return_value=dto)) + monkeypatch.setattr(mock_invoker.services.board_images, "remove_image_from_board", MagicMock(return_value=0)) + monkeypatch.setattr( + mock_invoker.services.board_image_records, "get_board_for_image", MagicMock(return_value=now_on_board) + ) + if record_exists: + monkeypatch.setattr(mock_invoker.services.image_records, "get", MagicMock(return_value=MagicMock())) + else: + from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException + + monkeypatch.setattr( + mock_invoker.services.image_records, "get", MagicMock(side_effect=ImageRecordNotFoundException) + ) + + response = client.request("DELETE", "/api/v1/board_images/", json={"image_name": "raced.png"}) + + assert response.status_code == 201 + body = response.json() + assert body["removed_images"] == expect_removed + assert body["failed_images"] == expect_failed + assert set(body["affected_boards"]) == set(expect_boards) + + +def test_single_remove_of_an_uncategorized_image_reports_removed_without_a_write( + monkeypatch: pytest.MonkeyPatch, mock_invoker: Invoker, client: TestClient +) -> None: + """No board_images row ever carries board_id="none", so the scoped DELETE could not match: + issuing it and then classifying the guaranteed zero-row miss would spend reads confirming + what the DTO already said. The postcondition holds, so it is removed, with no write at all.""" + _install(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = None + monkeypatch.setattr(mock_invoker.services.images, "get_dto", MagicMock(return_value=dto)) + remove = MagicMock(return_value=1) + monkeypatch.setattr(mock_invoker.services.board_images, "remove_image_from_board", remove) + + response = client.request("DELETE", "/api/v1/board_images/", json={"image_name": "loose.png"}) + + assert response.status_code == 201 + body = response.json() + assert body["removed_images"] == ["loose.png"] + assert body["failed_images"] == [] + assert body["affected_boards"] == ["none"] + remove.assert_not_called() + + +def test_single_remove_still_reports_a_nonzero_scoped_delete_as_removed( + monkeypatch: pytest.MonkeyPatch, mock_invoker: Invoker, client: TestClient +) -> None: + _install(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = "board-p" + monkeypatch.setattr(mock_invoker.services.images, "get_dto", MagicMock(return_value=dto)) + monkeypatch.setattr(mock_invoker.services.board_images, "remove_image_from_board", MagicMock(return_value=1)) + + response = client.request("DELETE", "/api/v1/board_images/", json={"image_name": "ok.png"}) + + assert response.status_code == 201 + body = response.json() + assert body["removed_images"] == ["ok.png"] + assert body["failed_images"] == [] + assert set(body["affected_boards"]) == {"none", "board-p"} diff --git a/tests/app/routers/test_board_images_maintenance.py b/tests/app/routers/test_board_images_maintenance.py index 90c7df5aa47..c7e20fccdbd 100644 --- a/tests/app/routers/test_board_images_maintenance.py +++ b/tests/app/routers/test_board_images_maintenance.py @@ -7,7 +7,7 @@ from invokeai.app.api.dependencies import ApiDependencies from invokeai.app.api_app import app from invokeai.app.services.auth.token_service import TokenData -from invokeai.app.services.board_records.board_records_common import BoardVisibility +from invokeai.app.services.board_records.board_records_common import BoardRecord, BoardVisibility from invokeai.app.services.boards.boards_common import BoardDTO from invokeai.app.services.invoker import Invoker @@ -19,6 +19,25 @@ def __init__(self, invoker) -> None: self.invoker = invoker +def _board_record() -> BoardRecord: + """The record behind the DTO below. + + Board write access is decided off the record, not the DTO: ownership and visibility are + two columns, while boards.get_dto() also resolves a cover image and runs three COUNT + aggregates, and that cost per name is what the batch routes cannot afford. + """ + return BoardRecord( + board_id="board-id", + board_name="Board", + user_id="system", + created_at="2024-01-01 00:00:00.000", + updated_at="2024-01-01 00:00:00.000", + archived=False, + board_visibility=BoardVisibility.Private, + cover_image_name=None, + ) + + @pytest.fixture def client() -> TestClient: return TestClient(app) @@ -64,6 +83,7 @@ def test_board_image_mutations_are_blocked_during_image_move_maintenance( ) ), ) + monkeypatch.setattr(mock_invoker.services.board_records, "get", MagicMock(return_value=_board_record())) monkeypatch.setattr("invokeai.app.api.routers.board_images.ApiDependencies", mock_deps) monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) monkeypatch.setattr("invokeai.app.api.routers.image_move_maintenance.ApiDependencies", mock_deps) @@ -102,6 +122,7 @@ def test_board_image_mutation_checks_access_before_image_move_maintenance( ) ), ) + monkeypatch.setattr(mock_invoker.services.board_records, "get", MagicMock(return_value=_board_record())) monkeypatch.setattr("invokeai.app.api.routers.board_images.ApiDependencies", mock_deps) monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) monkeypatch.setattr("invokeai.app.api.routers.image_move_maintenance.ApiDependencies", mock_deps) diff --git a/tests/app/routers/test_images.py b/tests/app/routers/test_images.py index 1e4270abff7..85761472182 100644 --- a/tests/app/routers/test_images.py +++ b/tests/app/routers/test_images.py @@ -1,4 +1,6 @@ import os +import sqlite3 +from contextlib import contextmanager from pathlib import Path from typing import Any from unittest.mock import MagicMock @@ -9,10 +11,14 @@ from invokeai.app.api.auth_dependencies import get_current_user_or_default from invokeai.app.api.dependencies import ApiDependencies +from invokeai.app.api.routers.images import MAX_IMAGE_BATCH_SIZE from invokeai.app.api_app import app from invokeai.app.services.auth.token_service import TokenData from invokeai.app.services.board_records.board_records_common import BoardRecord +from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException +from invokeai.app.services.images.images_common import ImageDTO from invokeai.app.services.invoker import Invoker +from invokeai.app.services.shared.pagination import MAX_PAGE_SIZE, OffsetPaginatedResults @pytest.fixture(autouse=True, scope="module") @@ -220,3 +226,375 @@ def test_get_bulk_download_image_image_deleted_after_response( client.get("/api/v1/images/download/test.zip") assert not (tmp_path / "test.zip").exists() + + +def prepare_image_batch_test(monkeypatch: Any, mock_invoker: Invoker) -> MagicMock: + """Wires the image router to a MagicMock image service with maintenance inactive. + + Returns the mock service so tests can script per-name update outcomes. + """ + images_service = MagicMock() + monkeypatch.setattr(mock_invoker.services, "images", images_service) + mock_invoker.services.image_moves = MagicMock() + mock_invoker.services.image_moves.is_maintenance_active.return_value = False + monkeypatch.setattr(mock_invoker.services.board_image_records, "get_board_for_image", MagicMock(return_value=None)) + + mock_deps = MockApiDependencies(mock_invoker) + monkeypatch.setattr("invokeai.app.api.routers.images.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers.image_move_maintenance.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) + return images_service + + +@pytest.fixture +def non_admin_user(): + """Makes ownership decisions depend on image_records.get_user_id rather than admin bypass.""" + + async def current_user_override() -> TokenData: + return TokenData(user_id="request-user", email="request-user@example.com", is_admin=False) + + app.dependency_overrides[get_current_user_or_default] = current_user_override + yield + app.dependency_overrides.pop(get_current_user_or_default, None) + + +@pytest.mark.parametrize( + ("route", "updated_key"), + [("star", "starred_images"), ("unstar", "unstarred_images")], +) +def test_star_unstar_reports_failures_and_keeps_partial_successes( + monkeypatch: Any, + mock_invoker: Invoker, + client: TestClient, + non_admin_user: None, + route: str, + updated_key: str, +) -> None: + """A foreign name is skipped, a storage failure is reported, and the rest still apply. + + Both used to abort the whole batch on the first foreign name (discarding the images + that HAD been updated) and to silently swallow storage failures, so the client cached + a star that never reached the DB. + """ + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + owners = {"ok.png": "request-user", "broken.png": "request-user", "foreign.png": "someone-else"} + monkeypatch.setattr( + mock_invoker.services.image_records, "get_user_id", MagicMock(side_effect=lambda name: owners.get(name)) + ) + + def update(image_name: str, changes: Any) -> MagicMock: + del changes + if image_name == "broken.png": + raise RuntimeError("storage is on fire") + dto = MagicMock() + dto.board_id = "board-1" + return dto + + images_service.update.side_effect = update + + response = client.post(f"/api/v1/images/{route}", json={"image_names": ["ok.png", "broken.png", "foreign.png"]}) + + assert response.status_code == 200 + body = response.json() + assert body[updated_key] == ["ok.png"] + # The genuine failure is reported; the foreign name is an intentional skip and must + # not be toasted as a failure. + assert body["failed_images"] == ["broken.png"] + assert body["affected_boards"] == ["board-1"] + + +@pytest.mark.parametrize( + ("route", "updated_key"), + [("star", "starred_images"), ("unstar", "unstarred_images")], +) +def test_star_unstar_skips_names_deleted_mid_batch( + monkeypatch: Any, + mock_invoker: Invoker, + client: TestClient, + route: str, + updated_key: str, +) -> None: + """A name deleted by a concurrent session is a skip, not a storage failure. + + Reported as an admin, because that is the only caller for which this is reachable: for + anyone else `get_user_id` returns None for a missing record and the ownership check + already answers 403. It is also the default single-user path, so this WAS the common + case -- the name landed in failed_images and toasted "1 image could not be updated" + for an image the user no longer had. + """ + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + + def update(image_name: str, changes: Any) -> MagicMock: + del changes + if image_name == "vanished.png": + raise ImageRecordNotFoundException + dto = MagicMock() + dto.board_id = "board-1" + return dto + + images_service.update.side_effect = update + + response = client.post(f"/api/v1/images/{route}", json={"image_names": ["ok.png", "vanished.png"]}) + + assert response.status_code == 200 + body = response.json() + assert body[updated_key] == ["ok.png"] + assert body["failed_images"] == [] + + +@pytest.mark.parametrize("raise_from", ["get_dto", "delete"]) +def test_delete_skips_names_deleted_mid_batch( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient, raise_from: str +) -> None: + """Same for /delete: the caller asked for the image to be gone, and it is. + + Parametrized over both raise sites because the race window spans them: the loop reads the + DTO for its board id and only then deletes, and ImageService.delete re-reads the record, so + a name can vanish after the first read succeeds. The two sites answer differently on + purpose, and the difference is what this asserts — see the branch below. + """ + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + + def get_dto(image_name: str) -> MagicMock: + if image_name == "vanished.png" and raise_from == "get_dto": + raise ImageRecordNotFoundException + dto = MagicMock() + # Distinct boards on purpose: sharing one would let the surviving image supply the + # vanished one's board, and the assertion below could not tell whether it was reported. + dto.board_id = "board-2" if image_name == "vanished.png" else "board-1" + return dto + + def delete(image_name: str) -> None: + if image_name == "vanished.png" and raise_from == "delete": + raise ImageRecordNotFoundException + + images_service.get_dto.side_effect = get_dto + images_service.delete.side_effect = delete + + response = client.post("/api/v1/images/delete", json={"image_names": ["ok.png", "vanished.png"]}) + + assert response.status_code == 200 + body = response.json() + assert body["failed_images"] == [] + if raise_from == "delete": + # Read a line earlier, so the record verifiably existed and a concurrent session removed + # it: the requested postcondition holds and must reach the client cleanup path as a + # confirmed deletion. Order is intentionally unspecified — the route accumulates a set. + assert set(body["deleted_images"]) == {"ok.png", "vanished.png"} + # Reported with its board. Every board-scoped tag getDeleteImagesTags publishes comes + # from affected_boards, and it ignores deleted_images by design, so dropping the board + # here leaves its counts stale while the name is reported gone. + assert set(body["affected_boards"]) == {"board-1", "board-2"} + else: + # The read itself failed, so nothing established the record ever existed. That matters + # because assert_image_owner returns immediately for an admin — the default single-user + # identity — without touching storage, so a name that never existed reaches this path. + # Reporting it deleted would answer for something the caller never had. + assert body["deleted_images"] == ["ok.png"] + assert set(body["affected_boards"]) == {"board-1"} + + +def test_delete_does_not_report_a_name_that_never_existed( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + """The admin path specifically: the ownership check is a no-op that proves nothing.""" + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + + def get_dto(image_name: str) -> MagicMock: + raise ImageRecordNotFoundException + + images_service.get_dto.side_effect = get_dto + + response = client.post("/api/v1/images/delete", json={"image_names": ["never-existed.png"]}) + + assert response.status_code == 200 + body = response.json() + assert body["deleted_images"] == [] + assert body["failed_images"] == [] + + +def test_image_records_get_does_not_disguise_a_storage_error_as_not_found(monkeypatch: Any) -> None: + """The narrowing itself: a sqlite3.Error out of the SELECT must stay a sqlite3.Error.""" + from invokeai.app.services.image_records.image_records_sqlite import SqliteImageRecordStorage + + storage = SqliteImageRecordStorage.__new__(SqliteImageRecordStorage) + + class _Cursor: + def execute(self, *args: Any, **kwargs: Any) -> None: + raise sqlite3.OperationalError("database disk image is malformed") + + class _Db: + @contextmanager + def transaction(self): + yield _Cursor() + + storage._db = _Db() # pyright: ignore[reportAttributeAccessIssue] + + with pytest.raises(sqlite3.OperationalError): + storage.get("a.png") + + +def test_delete_still_reports_a_genuine_storage_failure( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient +) -> None: + """The not-found skip must stay narrow -- a real deletion failure is still reported. + + This is the half of the storage-error story the route owns. The other half is at the + storage layer: image_records.get() used to re-raise every sqlite3.Error as + ImageRecordNotFoundException, so a locked or corrupt database was indistinguishable from a + concurrent delete and the skip would have answered 200 with two empty lists and no toast at + all. See test_image_records_get_does_not_disguise_a_storage_error_as_not_found. + """ + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + dto = MagicMock() + dto.board_id = "board-1" + images_service.get_dto.return_value = dto + + def delete(image_name: str) -> None: + if image_name == "broken.png": + raise RuntimeError("storage is on fire") + + images_service.delete.side_effect = delete + + response = client.post("/api/v1/images/delete", json={"image_names": ["ok.png", "broken.png"]}) + + assert response.status_code == 200 + body = response.json() + assert body["deleted_images"] == ["ok.png"] + assert body["failed_images"] == ["broken.png"] + + +@pytest.mark.parametrize("route", ["star", "unstar"]) +def test_star_unstar_dedupes_repeated_names( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient, non_admin_user: None, route: str +) -> None: + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + monkeypatch.setattr(mock_invoker.services.image_records, "get_user_id", MagicMock(return_value="request-user")) + dto = MagicMock() + dto.board_id = "board-1" + images_service.update.return_value = dto + + response = client.post(f"/api/v1/images/{route}", json={"image_names": ["a.png", "a.png", "a.png"]}) + + assert response.status_code == 200 + assert images_service.update.call_count == 1 + + +@pytest.mark.parametrize( + "path", + [ + "/api/v1/images/delete", + "/api/v1/images/star", + "/api/v1/images/unstar", + "/api/v1/images/images_by_names", + "/api/v1/images/download", + "/api/v1/board_images/batch", + "/api/v1/board_images/batch/delete", + ], +) +def test_image_name_batches_are_bounded(monkeypatch: Any, mock_invoker: Invoker, client: TestClient, path: str) -> None: + """An unbounded name list is a free amplification: each name costs at least one DB lookup, + and up to six when the caller is reading someone else's shared board.""" + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + # /download would otherwise authorize every name and schedule a background task; if the + # bound ever regresses we want the assertion below to fail, not the service call to blow up. + bulk_download = MagicMock() + bulk_download.generate_item_id.return_value = "test" + monkeypatch.setattr(mock_invoker.services, "bulk_download", bulk_download) + + body: dict[str, Any] = {"image_names": [f"image-{index}.png" for index in range(MAX_IMAGE_BATCH_SIZE + 1)]} + if path == "/api/v1/board_images/batch": + body["board_id"] = "board-1" + response = client.post(path, json=body) + assert response.status_code == 422 + + response = client.post(path, json={**body, "image_names": ["x" * 256]}) + assert response.status_code == 422 + + # Rejection is FastAPI request validation, so it lands before the route body runs at all. + # These two only bite on the /v1/images routes -- the board_images router reads its own + # module-level ApiDependencies, which prepare_image_batch_test does not patch -- but they + # are what catches a bound that gets "enforced" inside the handler instead of on the body. + assert images_service.mock_calls == [] + assert bulk_download.mock_calls == [] + + +def test_every_image_names_body_is_bounded(client: TestClient) -> None: + """Drift guard: a new explicit-name batch route must not ship without a bound. + + /download shipped unbounded because the limits were applied route-by-route rather + than to the shape. Rather than restate the limit on every route, assert the published + contract: every request body that takes an `image_names` array declares both a list + bound and a per-name length bound. + """ + schema = client.get("/openapi.json").json() + components = schema["components"]["schemas"] + + unbounded: list[str] = [] + checked: list[str] = [] + for path, operations in schema["paths"].items(): + for method, operation in operations.items(): + ref = ( + operation.get("requestBody", {}) + .get("content", {}) + .get("application/json", {}) + .get("schema", {}) + .get("$ref") + ) + if ref is None: + continue + body = components[ref.rsplit("/", 1)[-1]] + image_names = body.get("properties", {}).get("image_names") + if image_names is None: + continue + # Optional fields are wrapped in anyOf: [{array}, {null}]. + variants = image_names.get("anyOf", [image_names]) + array = next((variant for variant in variants if variant.get("type") == "array"), None) + if array is None: + continue + checked.append(path) + if array.get("maxItems") is None or array.get("items", {}).get("maxLength") is None: + unbounded.append(f"{method.upper()} {path}") + + # Pin the exact route set rather than a floor. A floor cannot see the failure this test + # exists to catch: a route the walk *skips* (a body that nests image_names inside a model + # rather than declaring it flat with Body(embed=True)) leaves the count unchanged and the + # guard green. Adding a route here is deliberate — bound it, then add it to this list. + assert sorted(checked) == [ + "/api/v1/board_images/batch", + "/api/v1/board_images/batch/delete", + "/api/v1/images/delete", + "/api/v1/images/download", + "/api/v1/images/images_by_names", + "/api/v1/images/star", + "/api/v1/images/unstar", + ] + assert unbounded == [], f"unbounded image_names batch bodies: {unbounded}" + + +@pytest.mark.parametrize( + "params", + [ + # A negative LIMIT means *unlimited* in SQLite — every image row, materialized. + {"limit": -1}, + {"limit": MAX_PAGE_SIZE + 1}, + {"offset": -1}, + ], +) +def test_list_image_dtos_rejects_out_of_range_pagination( + monkeypatch: Any, mock_invoker: Invoker, client: TestClient, params: dict[str, int] +) -> None: + prepare_image_batch_test(monkeypatch, mock_invoker) + assert client.get("/api/v1/images/", params=params).status_code == 422 + + +def test_list_image_dtos_allows_count_only_query(monkeypatch: Any, mock_invoker: Invoker, client: TestClient) -> None: + """The frontend issues limit=0 to read `total` without fetching rows.""" + images_service = prepare_image_batch_test(monkeypatch, mock_invoker) + images_service.get_many.return_value = OffsetPaginatedResults[ImageDTO](items=[], offset=0, limit=0, total=7) + + response = client.get("/api/v1/images/", params={"limit": 0}) + + assert response.status_code == 200 + assert response.json()["total"] == 7 diff --git a/tests/app/routers/test_multiuser_authorization.py b/tests/app/routers/test_multiuser_authorization.py index 267c46a36f9..adf422f9291 100644 --- a/tests/app/routers/test_multiuser_authorization.py +++ b/tests/app/routers/test_multiuser_authorization.py @@ -65,6 +65,13 @@ def client(): return TestClient(app) +def _mock_urls() -> MagicMock: + """A urls service whose getters return real strings, so ImageDTO validates.""" + urls = MagicMock() + urls.get_image_url.return_value = "http://test.invalid/image.png" + return urls + + @pytest.fixture def mock_services() -> InvocationServices: from invokeai.app.services.board_image_records.board_image_records_sqlite import SqliteBoardImageRecordStorage @@ -110,7 +117,12 @@ def mock_services() -> InvocationServices: performance_statistics=InvocationStatsService(), session_processor=None, # type: ignore session_queue=None, # type: ignore - urls=None, # type: ignore + # Real enough for ImageService.get_dto to build a DTO. With None it raised + # AttributeError for *every* image, so any route resolving a DTO silently took its + # not-found path — which made board_images' batch-delete authorization untestable: + # every name was skipped before the ownership check ran, and the test passed no + # matter what the route did. The returns must be strings; ImageDTO validates them. + urls=_mock_urls(), workflow_records=SqliteWorkflowRecordsStorage(db=db), tensors=None, # type: ignore conditioning=None, # type: ignore @@ -174,6 +186,11 @@ def enable_multiuser(monkeypatch: Any, mock_invoker: Invoker): mock_board_images = MagicMock() mock_board_images.get_all_board_image_names_for_board.return_value = [] + # The real facade returns the scoped DELETE's row count, and the routes classify a zero-row + # miss as not-removed. A bare MagicMock return only passed the old `== 0` check by accident; + # under `> 0` it is a TypeError. One row deleted is the honest default for a mock whose + # remove is expected to succeed; tests that stage the miss override this per-call. + mock_board_images.remove_image_from_board.return_value = 1 mock_invoker.services.board_images = mock_board_images mock_workflow_thumbnails = MagicMock() @@ -377,7 +394,14 @@ def test_non_owner_cannot_add_other_users_image_to_own_board( def test_non_owner_cannot_batch_add_other_users_images_to_own_board( self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str ): - """Same attack via the batch endpoint.""" + """Same attack via the batch endpoint. + + Batch add skips foreign names instead of re-raising the first 403 — same rationale as + test_non_owner_cannot_star_image: re-raising mid-batch discarded the partial successes, + so the client never learned which images HAD moved. Only the response shape changes. + The attack itself must still fail: the victim's image must not move, and must not be + advertised as added. + """ user1 = mock_invoker.services.users.get_by_email("user1@test.com") assert user1 is not None _save_image(mock_invoker, "victim-batch-img", user1.user_id) @@ -389,7 +413,443 @@ def test_non_owner_cannot_batch_add_other_users_images_to_own_board( json={"board_id": attacker_board, "image_names": ["victim-batch-img"]}, headers=_auth(user2_token), ) - assert r.status_code == status.HTTP_403_FORBIDDEN + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["added_images"] == [] + # An auth skip is not a failure — it must not be reported (and toasted) as one. + assert body["failed_images"] == [] + # `board_images` is a MagicMock in this fixture, so asserting on board_image_records + # would pass no matter what the route did. Assert the move was never attempted. + mock_invoker.services.board_images.add_image_to_board.assert_not_called() + + def test_batch_add_keeps_partial_successes_when_one_name_is_foreign( + self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str + ): + """The point of the skip: the attacker's own image still moves, and is reported. + + Before, the first foreign name re-raised and discarded the payload for every image + that had already been moved in the same request, so the client never invalidated + their caches and the UI kept showing them on their old board. + """ + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user1 is not None and user2 is not None + _save_image(mock_invoker, "victim-mixed-img", user1.user_id) + _save_image(mock_invoker, "own-mixed-img", user2.user_id) + + board_id = _create_board(client, user2_token, "Mixed Batch Board") + mock_invoker.services.board_images.add_image_to_board.reset_mock() + + # Foreign name first, so the old `raise` would abort before reaching the owned one. + r = client.post( + "/api/v1/board_images/batch", + json={"board_id": board_id, "image_names": ["victim-mixed-img", "own-mixed-img"]}, + headers=_auth(user2_token), + ) + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["added_images"] == ["own-mixed-img"] + assert body["failed_images"] == [] + assert board_id in body["affected_boards"] + # Exactly one move attempted, and only for the caller's own image. + assert [ + call.kwargs["image_name"] for call in mock_invoker.services.board_images.add_image_to_board.call_args_list + ] == ["own-mixed-img"] + + def test_non_owner_cannot_batch_remove_images_from_foreign_board( + self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str + ): + """Batch remove skips names on boards the caller cannot write, instead of re-raising. + + The guarantee is unchanged: the image stays on the board and is not advertised as + removed. Only the response shape changes. + """ + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + _save_image(mock_invoker, "victim-remove-img", user1.user_id) + board_id = _create_board(client, user1_token, "User1 Private Remove Board") + mock_invoker.services.board_image_records.add_image_to_board(board_id, "victim-remove-img") + mock_invoker.services.board_images.remove_image_from_board.reset_mock() + + r = client.post( + "/api/v1/board_images/batch/delete", + json={"image_names": ["victim-remove-img"]}, + headers=_auth(user2_token), + ) + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["removed_images"] == [] + # An auth skip is not a failure — it must not be reported (and toasted) as one. + assert body["failed_images"] == [] + mock_invoker.services.board_images.remove_image_from_board.assert_not_called() + + def test_batch_remove_keeps_partial_successes_when_one_name_is_foreign( + self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str + ): + """The point of the skip, on the remove side: the caller's own image still comes off.""" + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user1 is not None and user2 is not None + _save_image(mock_invoker, "victim-rm-mixed", user1.user_id) + _save_image(mock_invoker, "own-rm-mixed", user2.user_id) + victim_board = _create_board(client, user1_token, "User1 Board For Remove Mix") + own_board = _create_board(client, user2_token, "User2 Board For Remove Mix") + mock_invoker.services.board_image_records.add_image_to_board(victim_board, "victim-rm-mixed") + mock_invoker.services.board_image_records.add_image_to_board(own_board, "own-rm-mixed") + mock_invoker.services.board_images.remove_image_from_board.reset_mock() + + # Foreign name first, so the old `raise` would abort before reaching the owned one. + r = client.post( + "/api/v1/board_images/batch/delete", + json={"image_names": ["victim-rm-mixed", "own-rm-mixed"]}, + headers=_auth(user2_token), + ) + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["removed_images"] == ["own-rm-mixed"] + assert body["failed_images"] == [] + assert own_board in body["affected_boards"] + assert victim_board not in body["affected_boards"] + assert [ + call.kwargs["image_name"] + for call in mock_invoker.services.board_images.remove_image_from_board.call_args_list + ] == ["own-rm-mixed"] + + def test_batch_remove_decides_board_write_access_once_per_name_and_cheaply( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user1_token: str, user2_token: str + ): + """Two invariants that pull against each other, asserted together. + + Skipping removed the early abort that used to cap an unauthorized batch at one check, so + the decision is now taken for every name -- caching it per board would let a permission + revoked mid-batch keep working until the request ends (see the revocation test below). + + Which is only affordable because the decision reads the board *record*: one indexed + SELECT. Through boards.get_dto() it would be six queries -- three of them COUNT + aggregates over the board's contents -- per name, synchronously, on the event loop, for + a 1000-name batch. So get_dto must not appear in this path at all. + """ + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, "User1 Board Perms") + names = [f"victim-perm-{index}" for index in range(5)] + for name in names: + _save_image(mock_invoker, name, user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, name) + + get_dto_spy = MagicMock(side_effect=mock_invoker.services.boards.get_dto) + monkeypatch.setattr(mock_invoker.services.boards, "get_dto", get_dto_spy) + record_spy = MagicMock(side_effect=mock_invoker.services.board_records.get) + monkeypatch.setattr(mock_invoker.services.board_records, "get", record_spy) + + r = client.post( + "/api/v1/board_images/batch/delete", + json={"image_names": names}, + headers=_auth(user2_token), + ) + assert r.status_code == status.HTTP_201_CREATED + assert r.json()["removed_images"] == [] + assert record_spy.call_count == len(names) + assert get_dto_spy.call_count == 0 + + def test_batch_remove_stops_when_board_write_access_is_revoked_mid_batch( + self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str + ): + """A contributor's write access ends the moment the board stops being public. + + Public boards accept contributions from anyone, so user2 may empty user1's public board. + Nothing about that decision holds for the rest of a 1000-name batch: user1 can make the + board private while it is still running. Caching the first `True` would remove every + remaining name on an answer that is no longer true, and none of it is undone. + """ + from invokeai.app.services.board_records.board_records_common import BoardChanges, BoardVisibility + + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, "User1 Public Board Revoked") + _set_board_visibility(client, user1_token, board_id, "public") + names = [f"revoke-rm-{index}" for index in range(3)] + for name in names: + _save_image(mock_invoker, name, user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, name) + + def _revoke_after_first_removal(image_name: str, board_id: str) -> int: + mock_invoker.services.board_records.update(board_id, BoardChanges(board_visibility=BoardVisibility.Private)) + # One row removed: a side_effect's return value overrides the mock's return_value, + # and the route classifies anything else as a miss. + return 1 + + mock_invoker.services.board_images.remove_image_from_board.reset_mock() + mock_invoker.services.board_images.remove_image_from_board.side_effect = _revoke_after_first_removal + + r = client.post( + "/api/v1/board_images/batch/delete", + json={"image_names": names}, + headers=_auth(user2_token), + ) + + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["removed_images"] == [names[0]] + # The rest are an authorization skip, not a failure: absent from both lists. + assert body["failed_images"] == [] + assert mock_invoker.services.board_images.remove_image_from_board.call_count == 1 + + def test_batch_add_stops_when_board_write_access_is_revoked_mid_batch( + self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str + ): + """The same window on the add side, where the target board is checked before the loop. + + The refused names are reported as failed, not skipped: the destination is the whole + request's problem, and skipping empties the rest of the batch into a 201 with empty + lists, which the client reads as success and clears the user's selection over. The + loop must still stop *issuing adds* the moment access is gone. + """ + from invokeai.app.services.board_records.board_records_common import BoardChanges, BoardVisibility + + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user2 is not None + board_id = _create_board(client, user1_token, "User1 Public Board Add Revoked") + _set_board_visibility(client, user1_token, board_id, "public") + names = [f"revoke-add-{index}" for index in range(3)] + for name in names: + _save_image(mock_invoker, name, user2.user_id) + + def _revoke_after_first_add(board_id: str, image_name: str) -> None: + mock_invoker.services.board_records.update(board_id, BoardChanges(board_visibility=BoardVisibility.Private)) + + mock_invoker.services.board_images.add_image_to_board.reset_mock() + mock_invoker.services.board_images.add_image_to_board.side_effect = _revoke_after_first_add + + r = client.post( + "/api/v1/board_images/batch", + json={"board_id": board_id, "image_names": names}, + headers=_auth(user2_token), + ) + + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["added_images"] == [names[0]] + assert set(body["failed_images"]) == set(names[1:]) + assert mock_invoker.services.board_images.add_image_to_board.call_count == 1 + + def test_batch_add_skips_an_image_deleted_after_its_ownership_check( + self, client: TestClient, mock_invoker: Invoker, user2_token: str + ): + """A name deleted mid-batch is a skip, not a failure -- even here, where it arrives + as a bare foreign-key error. + + board_images.image_name references images.image_name, so an image deleted between the + ownership check and the insert fails the INSERT with sqlite3.IntegrityError. Nothing in + that exception says "gone", so without the record probe the name is reported (and + toasted) as a storage failure for an image the user themselves just deleted. + """ + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user2 is not None + names = ["fk-ok", "fk-vanished"] + for name in names: + _save_image(mock_invoker, name, user2.user_id) + board_id = _create_board(client, user2_token, "User2 FK Race Board") + + def _delete_then_insert(board_id: str, image_name: str) -> None: + if image_name == "fk-vanished": + mock_invoker.services.image_records.delete(image_name) + # The real storage, so the foreign key fires for real rather than being simulated. + mock_invoker.services.board_image_records.add_image_to_board(board_id, image_name) + + mock_invoker.services.board_images.add_image_to_board.reset_mock() + mock_invoker.services.board_images.add_image_to_board.side_effect = _delete_then_insert + + r = client.post( + "/api/v1/board_images/batch", + json={"board_id": board_id, "image_names": names}, + headers=_auth(user2_token), + ) + + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["added_images"] == ["fk-ok"] + # Absent from both lists: not moved by us, and not a failure either. + assert body["failed_images"] == [] + + def test_batch_add_reports_a_failure_it_could_not_probe( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user2_token: str + ): + """The probe's own failure path: an unreadable record must not become a skip. + + The insert failure is classified by asking whether the image record is still there, so + the probe decides whether a name is reported. If the probe cannot answer — the same + locked database that broke the insert — the only safe answer is "still there": a skip + claims the user's own concurrent delete caused this, and says nothing at all. + """ + import sqlite3 + + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user2 is not None + _save_image(mock_invoker, "unprobeable", user2.user_id) + board_id = _create_board(client, user2_token, "User2 Unprobeable Board") + + def _fail(board_id: str, image_name: str) -> None: + raise RuntimeError("storage is on fire") + + mock_invoker.services.board_images.add_image_to_board.reset_mock() + mock_invoker.services.board_images.add_image_to_board.side_effect = _fail + monkeypatch.setattr( + mock_invoker.services.image_records, + "get", + MagicMock(side_effect=sqlite3.OperationalError("database is locked")), + ) + + r = client.post( + "/api/v1/board_images/batch", + json={"board_id": board_id, "image_names": ["unprobeable"]}, + headers=_auth(user2_token), + ) + + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["added_images"] == [] + assert body["failed_images"] == ["unprobeable"] + + @pytest.mark.parametrize("route", ["add", "remove"]) + def test_batch_routes_report_a_name_whose_board_check_hit_a_storage_error( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user2_token: str, route: str + ): + """A name we could not decide about must be reported, never silently dropped. + + Board write access is now decided once per name, off board_records.get(). That read used + to answer a locked or unreadable database with BoardRecordNotFoundException — the same + exception a board that simply does not exist raises — which the routes turn into a 404 + and then skip. A disk error mid-batch would therefore drop names out of the response + entirely: absent from added/removed, absent from failed_images, no toast, and the client + re-rendering them as moved until the next refresh. That is exactly the outcome + failed_images exists to prevent, so the storage error has to stay distinguishable. + """ + import sqlite3 + + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user2 is not None + names = ["decide-ok", "decide-broken"] + for name in names: + _save_image(mock_invoker, name, user2.user_id) + board_id = _create_board(client, user2_token, "User2 Board Check Failure") + if route == "remove": + for name in names: + mock_invoker.services.board_image_records.add_image_to_board(board_id, name) + + real_get = mock_invoker.services.board_records.get + calls = {"n": 0} + # One decision per name, plus — on the add route only — the up-front check that answers + # a wholly unauthorized request with a 403 before the loop starts. Either way the last + # decision is the one taken for "decide-broken". + failing_call = 3 if route == "add" else 2 + + def _fail_on_the_last_decision(requested_board_id: str): + calls["n"] += 1 + if calls["n"] == failing_call: + raise sqlite3.OperationalError("database is locked") + return real_get(requested_board_id) + + monkeypatch.setattr(mock_invoker.services.board_records, "get", _fail_on_the_last_decision) + mock_invoker.services.board_images.add_image_to_board.reset_mock() + mock_invoker.services.board_images.remove_image_from_board.reset_mock() + + if route == "add": + r = client.post( + "/api/v1/board_images/batch", + json={"board_id": board_id, "image_names": names}, + headers=_auth(user2_token), + ) + moved_key = "added_images" + else: + r = client.post( + "/api/v1/board_images/batch/delete", + json={"image_names": names}, + headers=_auth(user2_token), + ) + moved_key = "removed_images" + + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body[moved_key] == ["decide-ok"] + assert body["failed_images"] == ["decide-broken"] + + def test_batch_remove_only_touches_the_board_it_authorized_against( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user1_token: str, user2_token: str + ): + """Deciding freshly is not enough — the write has to be scoped to what was decided. + + The route reads the image's board, authorizes against *that* board, then removes. An + unscoped `DELETE ... WHERE image_name = ?` follows the image if it is moved in between, + so a decision taken about a public board could be applied to a private one: user2 is + authorized against public P, user1 moves the image to private Q, and the delete lands on + Q. The predicate belongs on the write. + """ + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + public_board = _create_board(client, user1_token, "User1 Public Source") + _set_board_visibility(client, user1_token, public_board, "public") + private_board = _create_board(client, user1_token, "User1 Private Destination") + _save_image(mock_invoker, "moving-target", user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(public_board, "moving-target") + + # The image moves to the private board between the route's board read and its delete. + real_get_dto = mock_invoker.services.images.get_dto + + def _move_after_reading(image_name: str): + dto = real_get_dto(image_name) + mock_invoker.services.board_image_records.add_image_to_board(private_board, image_name) + return dto + + monkeypatch.setattr(mock_invoker.services.images, "get_dto", _move_after_reading) + # The real storage, so the scoping is exercised rather than asserted on a mock. + mock_invoker.services.board_images.remove_image_from_board.side_effect = ( + mock_invoker.services.board_image_records.remove_image_from_board + ) + + r = client.post( + "/api/v1/board_images/batch/delete", + json={"image_names": ["moving-target"]}, + headers=_auth(user2_token), + ) + + assert r.status_code == status.HTTP_201_CREATED + # The image is still on the private board: user2 was never authorized against it. + assert mock_invoker.services.board_image_records.get_board_for_image("moving-target") == private_board + + def test_batch_add_still_reports_a_genuine_storage_failure( + self, client: TestClient, mock_invoker: Invoker, user2_token: str + ): + """The other half of the probe: a name whose record is still there stays a failure. + + Without this the skip above could be written as a blanket `continue` and nothing would + notice -- a move that silently reverted on reload is exactly what failed_images exists + to surface. + """ + user2 = mock_invoker.services.users.get_by_email("user2@test.com") + assert user2 is not None + names = ["storage-ok", "storage-broken"] + for name in names: + _save_image(mock_invoker, name, user2.user_id) + board_id = _create_board(client, user2_token, "User2 Storage Failure Board") + + def _fail_one(board_id: str, image_name: str) -> None: + if image_name == "storage-broken": + raise RuntimeError("storage is on fire") + + mock_invoker.services.board_images.add_image_to_board.reset_mock() + mock_invoker.services.board_images.add_image_to_board.side_effect = _fail_one + + r = client.post( + "/api/v1/board_images/batch", + json={"board_id": board_id, "image_names": names}, + headers=_auth(user2_token), + ) + + assert r.status_code == status.HTTP_201_CREATED + body = r.json() + assert body["added_images"] == ["storage-ok"] + assert body["failed_images"] == ["storage-broken"] # =========================================================================== @@ -404,6 +864,139 @@ def test_get_image_dto_requires_auth(self, enable_multiuser: Any, client: TestCl r = client.get("/api/v1/images/i/some-image") assert r.status_code == status.HTTP_401_UNAUTHORIZED + def test_deleted_image_reads_as_gone_rather_than_denied( + self, client: TestClient, mock_invoker: Invoker, user1_token: str + ): + """A deleted image answers 404 even to a non-admin, and the clients depend on it. + + The ownership decision rests on `images.user_id`, which is gone with the row, so + nothing above the refusal can tell a deleted image from a foreign one -- both used to + come back 403. The two mean opposite things to a client holding a reference: gone is + permanent and the reference should go with it, denied is reversible and it must not. + Only the refusal path pays for the distinction. + """ + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + _save_image(mock_invoker, "user1-doomed", user1.user_id) + mock_invoker.services.image_records.delete("user1-doomed") + + r = client.get("/api/v1/images/i/user1-doomed", headers=_auth(user1_token)) + + assert r.status_code == status.HTTP_404_NOT_FOUND + + def test_unreadable_storage_does_not_read_as_a_deleted_image( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user1_token: str + ): + """The DTO route's 404 is the one clients destroy references on, so only absence earns it. + + The route ended `except Exception: raise HTTPException(404)`, so any failure inside + `get_dto` -- the board lookup, the URL service, not just a missing row -- answered the + same 404 that tells a workflow field its image is gone. The caller here owns the image + and it is still there; the board lookup is what breaks. + """ + import sqlite3 + + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + _save_image(mock_invoker, "user1-unreadable", user1.user_id) + monkeypatch.setattr( + mock_invoker.services.board_image_records, + "get_board_for_image", + MagicMock(side_effect=sqlite3.OperationalError("database is locked")), + ) + + # Uncaught in the route, so a 500 in production; the test client re-raises instead of + # rendering it. Either way it must not be the 404 that clears the user's reference. + with pytest.raises(sqlite3.OperationalError): + client.get("/api/v1/images/i/user1-unreadable", headers=_auth(user1_token)) + + def test_revoking_access_to_a_live_image_stays_a_denial( + self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str + ): + """The other half, and the one with teeth: a reversible refusal must not read as gone. + + A shared board flipped back to Private refuses every image on it, and every one of + them still exists. The clients drop a workflow field or a reference image on a 404, so + answering one here would destroy work that restoring the permission could not bring + back. + """ + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, "User1 Formerly Shared Board") + _set_board_visibility(client, user1_token, board_id, "shared") + _save_image(mock_invoker, "user1-still-here", user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, "user1-still-here") + assert client.get("/api/v1/images/i/user1-still-here", headers=_auth(user2_token)).status_code == ( + status.HTTP_200_OK + ) + + _set_board_visibility(client, user1_token, board_id, "private") + + r = client.get("/api/v1/images/i/user1-still-here", headers=_auth(user2_token)) + + assert r.status_code == status.HTTP_403_FORBIDDEN + + def test_unreadable_board_does_not_read_as_unavailable( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user1_token: str, user2_token: str + ): + """A storage error must not reach the client wearing the deleted image's answer. + + `assert_image_read_access` used to catch every exception from the board lookup and + fall through to the same 403 a deleted image gets. Since the clients read that 403 as + "gone, drop your reference", a locked database would have taken every workflow field + and reference image pointing at a shared board's images down with it. Only a board + positively known to be gone may still answer 403. + """ + import sqlite3 + + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, "User1 Shared Read Board") + _set_board_visibility(client, user1_token, board_id, "shared") + _save_image(mock_invoker, "user1-shared-read", user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, "user1-shared-read") + + # Patched only after the setup above used the real store. user2 is neither admin nor + # direct owner, so the decision reaches the board lookup and cannot complete. + monkeypatch.setattr( + mock_invoker.services.board_records, + "get", + MagicMock(side_effect=sqlite3.OperationalError("database is locked")), + ) + + # The storage error leaves the route uncaught, which is a 500 in production; the test + # client re-raises unhandled server exceptions instead of rendering them. Either way the + # one thing that must not happen is a 403 -- the answer the clients act on destructively. + with pytest.raises(sqlite3.OperationalError): + client.get("/api/v1/images/i/user1-shared-read", headers=_auth(user2_token)) + + def test_vanished_board_still_reads_as_an_ordinary_refusal( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user1_token: str, user2_token: str + ): + """The narrowed catch stays exactly that narrow, in both directions. + + A dangling board_image row refuses the read, but the image itself is still there, so + the refusal is a 403 and not the 404 that would take the caller's reference with it. + """ + from invokeai.app.services.board_records.board_records_common import BoardRecordNotFoundException + + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, "User1 Vanishing Read Board") + _set_board_visibility(client, user1_token, board_id, "shared") + _save_image(mock_invoker, "user1-read-board-gone", user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, "user1-read-board-gone") + + monkeypatch.setattr( + mock_invoker.services.board_records, + "get", + MagicMock(side_effect=BoardRecordNotFoundException), + ) + + r = client.get("/api/v1/images/i/user1-read-board-gone", headers=_auth(user2_token)) + + assert r.status_code == status.HTTP_403_FORBIDDEN + def test_get_image_metadata_requires_auth(self, enable_multiuser: Any, client: TestClient): r = client.get("/api/v1/images/i/some-image/metadata") assert r.status_code == status.HTTP_401_UNAUTHORIZED @@ -765,6 +1358,13 @@ def test_non_owner_cannot_update_image( def test_non_owner_cannot_star_image( self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str ): + """Batch star skips foreign items instead of re-raising the first 403. + + Same rationale as test_non_owner_cannot_batch_delete_image: re-raising mid-batch + discarded the partial successes, so the client never learned which images HAD + changed. Only the response shape changes — the foreign image must still not be + starred, and must not be advertised as starred. + """ user1 = mock_invoker.services.users.get_by_email("user1@test.com") assert user1 is not None _save_image(mock_invoker, "user1-star-blocked", user1.user_id) @@ -774,7 +1374,95 @@ def test_non_owner_cannot_star_image( json={"image_names": ["user1-star-blocked"]}, headers=_auth(user2_token), ) - assert r.status_code == status.HTTP_403_FORBIDDEN + assert r.status_code == status.HTTP_200_OK + body = r.json() + assert body["starred_images"] == [] + # An auth skip is not a failure — it must not be reported (and toasted) as one. + assert body["failed_images"] == [] + assert mock_invoker.services.image_records.get("user1-star-blocked").starred is False + + @pytest.mark.parametrize("route", ["star", "unstar"]) + def test_star_reports_a_name_whose_board_lookup_hit_a_storage_error( + self, + client: TestClient, + mock_invoker: Invoker, + monkeypatch: Any, + user1_token: str, + user2_token: str, + route: str, + ): + """A database error during the board-ownership fallback must land in failed_images. + + `assert_image_owner` used to catch every exception from the board lookup and fall + through to the 403, and the batch loops treat a 403 as a silent auth skip -- so a + locked database made the star quietly vanish from the response: not applied, not + reported, and nothing for the client to toast. Only a board positively known to be + gone may still answer 403; a lookup that cannot be decided must propagate into the + loop's storage-failure arm. + """ + import sqlite3 + + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, f"User1 Public {route} Board") + _set_board_visibility(client, user1_token, board_id, "public") + name = f"user1-{route}-undecidable" + _save_image(mock_invoker, name, user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, name) + + # Patched only after the setup above used the real store. user2 is neither admin nor + # direct owner, so the decision reaches the board lookup and cannot complete. + monkeypatch.setattr( + mock_invoker.services.board_records, + "get", + MagicMock(side_effect=sqlite3.OperationalError("database is locked")), + ) + + r = client.post( + f"/api/v1/images/{route}", + json={"image_names": [name]}, + headers=_auth(user2_token), + ) + assert r.status_code == status.HTTP_200_OK + body = r.json() + assert body[f"{route}red_images"] == [] + assert body["failed_images"] == [name] + + def test_star_still_skips_a_name_whose_board_is_positively_gone( + self, client: TestClient, mock_invoker: Invoker, monkeypatch: Any, user1_token: str, user2_token: str + ): + """The narrowed catch must stay exactly that narrow, in both directions. + + A board positively known to be gone (a dangling board_image row) is the one lookup + outcome that may still answer the ordinary 403 -- an auth skip, absent from both + lists. An implementation that let BoardRecordNotFoundException propagate alongside + the storage errors would report it in failed_images and toast a failure for a name + whose only problem is that its board vanished mid-request. + """ + from invokeai.app.services.board_records.board_records_common import BoardRecordNotFoundException + + user1 = mock_invoker.services.users.get_by_email("user1@test.com") + assert user1 is not None + board_id = _create_board(client, user1_token, "User1 Vanishing Board") + _set_board_visibility(client, user1_token, board_id, "public") + _save_image(mock_invoker, "user1-board-gone", user1.user_id) + mock_invoker.services.board_image_records.add_image_to_board(board_id, "user1-board-gone") + + monkeypatch.setattr( + mock_invoker.services.board_records, + "get", + MagicMock(side_effect=BoardRecordNotFoundException), + ) + + r = client.post( + "/api/v1/images/star", + json={"image_names": ["user1-board-gone"]}, + headers=_auth(user2_token), + ) + assert r.status_code == status.HTTP_200_OK + body = r.json() + assert body["starred_images"] == [] + assert body["failed_images"] == [] def test_non_owner_cannot_batch_delete_image( self, client: TestClient, mock_invoker: Invoker, user1_token: str, user2_token: str diff --git a/tests/app/routers/test_videos_multiuser.py b/tests/app/routers/test_videos_multiuser.py index c0308c6e19c..ced769900d5 100644 --- a/tests/app/routers/test_videos_multiuser.py +++ b/tests/app/routers/test_videos_multiuser.py @@ -257,6 +257,112 @@ def test_delete_videos_from_list_dedupes_repeated_names(client: TestClient, mock assert sorted(delete_calls) == ["dup.mp4", "other.mp4"] +def test_deleted_video_reads_as_gone_rather_than_denied(client: TestClient, mock_invoker: Invoker, user1_token: str): + """A deleted video answers 404 even to a non-admin, and the clients depend on it. + + The ownership decision rests on ``videos.user_id``, which is gone with the row, so nothing + above the refusal can tell a deleted video from a foreign one -- both used to come back 403. + A workflow's video field drops its reference on a 404, so the two answers have to differ. + """ + mock_invoker.services.video_records.get_user_id.return_value = None + mock_invoker.services.board_video_records.get_board_for_video.return_value = None + mock_invoker.services.video_records.exists = MagicMock(return_value=False) + + response = client.get( + "/api/v1/videos/i/gone.mp4", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_unreadable_storage_does_not_read_as_a_deleted_video( + client: TestClient, mock_invoker: Invoker, admin_token: str +): + """The DTO route's 404 is the one clients destroy references on, so only absence earns it. + + The route ended ``except Exception: raise HTTPException(404)``, so any failure inside + ``get_dto`` answered the same 404 that tells a workflow field its video is gone. + """ + import sqlite3 + + mock_invoker.services.videos.get_dto = MagicMock(side_effect=sqlite3.OperationalError("database is locked")) + + # Uncaught in the route, so a 500 in production; the test client re-raises instead of + # rendering it. Either way it must not be the 404 that clears the user's reference. + with pytest.raises(sqlite3.OperationalError): + client.get("/api/v1/videos/i/unreadable.mp4", headers={"Authorization": f"Bearer {admin_token}"}) + + +def test_revoking_access_to_a_live_video_stays_a_denial(client: TestClient, mock_invoker: Invoker, user1_token: str): + """A reversible refusal must not read as gone. + + A shared board flipped back to Private refuses every video on it, and every one of them + still exists. Answering 404 would clear the workflow fields pointing at them, and restoring + the permission would not bring those back. + """ + from invokeai.app.services.board_records.board_records_common import BoardVisibility + + mock_invoker.services.video_records.get_user_id.return_value = "someone-else" + mock_invoker.services.board_video_records.get_board_for_video.return_value = "board-1" + private_board = MagicMock() + private_board.board_visibility = BoardVisibility.Private + mock_invoker.services.board_records.get = MagicMock(return_value=private_board) + # The video itself is untouched, which is what makes this a denial rather than a 404. + mock_invoker.services.video_records.exists = MagicMock(return_value=True) + + response = client.get( + "/api/v1/videos/i/still-here.mp4", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_unreadable_board_does_not_read_as_unavailable_for_videos( + client: TestClient, mock_invoker: Invoker, user1_token: str +): + """A storage error must not reach the client wearing the deleted video's answer. + + ``_assert_video_read_access`` used to catch every exception from the board lookup and fall + through to the same 403 a deleted video gets. Since the clients read that 403 as "gone, drop + your reference", a locked database would have taken every workflow field pointing at a + shared board's videos down with it. + """ + import sqlite3 + + mock_invoker.services.video_records.get_user_id.return_value = "someone-else" + mock_invoker.services.board_video_records.get_board_for_video.return_value = "board-1" + mock_invoker.services.board_records.get = MagicMock(side_effect=sqlite3.OperationalError("database is locked")) + + # The storage error leaves the route uncaught, which is a 500 in production; the test client + # re-raises unhandled server exceptions instead of rendering them. Either way the one thing + # that must not happen is a 403 -- the answer the clients act on destructively. + with pytest.raises(sqlite3.OperationalError): + client.get("/api/v1/videos/i/shared.mp4", headers={"Authorization": f"Bearer {user1_token}"}) + + +def test_vanished_board_still_reads_as_an_ordinary_refusal_for_videos( + client: TestClient, mock_invoker: Invoker, user1_token: str +): + """The narrowed catch stays exactly that narrow, in both directions.""" + from invokeai.app.services.board_records.board_records_common import BoardRecordNotFoundException + + mock_invoker.services.video_records.get_user_id.return_value = "someone-else" + mock_invoker.services.board_video_records.get_board_for_video.return_value = "board-1" + mock_invoker.services.board_records.get = MagicMock(side_effect=BoardRecordNotFoundException) + # The video itself is still there, so the refusal is a denial and not the 404 that would + # take the caller's reference with it. + mock_invoker.services.video_records.exists = MagicMock(return_value=True) + + response = client.get( + "/api/v1/videos/i/board-gone.mp4", + headers={"Authorization": f"Bearer {user1_token}"}, + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_video_batch_rejects_too_many_or_overlong_names() -> None: with pytest.raises(ValidationError): VideoNamesBatch(video_names=[f"{index}.mp4" for index in range(1001)]) diff --git a/tests/app/services/boards/test_boards_default.py b/tests/app/services/boards/test_boards_default.py index 6c13099a1fe..87d6fb9b959 100644 --- a/tests/app/services/boards/test_boards_default.py +++ b/tests/app/services/boards/test_boards_default.py @@ -122,3 +122,38 @@ def test_non_admin_board_listing_skips_owner_lookup(mock_invoker: Invoker) -> No assert [dto.owner_username for dto in result] == [None] mock_invoker.services.users.get.assert_not_called() # type: ignore[attr-defined] mock_invoker.services.users.get_many.assert_not_called() # type: ignore[attr-defined] + + +def test_board_records_get_does_not_disguise_a_storage_error_as_not_found() -> None: + """A sqlite3.Error out of the SELECT must stay a sqlite3.Error. + + Translating it made BoardRecordNotFoundException mean "no such board, OR the database is + unreadable", and the board-image batch routes cannot tell those apart: they decide write + access off this read once per name and treat not-found as a name to skip. A disk error would + then drop names out of the response silently — reported neither as moved nor as failed — + and the client would keep showing them as moved until the next refresh. Mirrors + test_image_records_get_does_not_disguise_a_storage_error_as_not_found. + """ + import sqlite3 + from contextlib import contextmanager + from typing import Any + + import pytest + + from invokeai.app.services.board_records.board_records_sqlite import SqliteBoardRecordStorage + + storage = SqliteBoardRecordStorage.__new__(SqliteBoardRecordStorage) + + class _Cursor: + def execute(self, *args: Any, **kwargs: Any) -> None: + raise sqlite3.OperationalError("database disk image is malformed") + + class _Db: + @contextmanager + def transaction(self): + yield _Cursor() + + storage._db = _Db() # pyright: ignore[reportAttributeAccessIssue] + + with pytest.raises(sqlite3.OperationalError): + storage.get("board-1") diff --git a/tests/app/services/video_files/test_video_files_disk.py b/tests/app/services/video_files/test_video_files_disk.py index f793746b45e..59c6edcf299 100644 --- a/tests/app/services/video_files/test_video_files_disk.py +++ b/tests/app/services/video_files/test_video_files_disk.py @@ -164,6 +164,28 @@ def test_start_restores_staged_delete_when_record_still_exists(storage: DiskVide assert not list((tmp_path / "videos").glob(".delete_*")) +def test_start_keeps_staged_delete_when_the_record_cannot_be_read(storage: DiskVideoFileStorage, tmp_path: Path): + """A database it merely could not read must not count as proof the delete committed. + + The recovery decides between purging the staged files and restoring them by asking whether + the record is still there, so it needs that read to distinguish "gone" from "could not + look". `SqliteVideoRecordStorage.get` used to translate every sqlite3.Error into + VideoRecordNotFoundException, which made an unreadable database delete the user's video + files outright. Now the staged copy survives for a later attempt. + """ + import sqlite3 + + source = _make_source(tmp_path) + storage.save(source_path=source, video_name=VIDEO_NAME, metadata='{"seed": 1}') + storage.stage_delete(VIDEO_NAME) + invoker = MagicMock() + invoker.services.video_records.get.side_effect = sqlite3.OperationalError("database is locked") + + DiskVideoFileStorage(tmp_path / "videos").start(invoker) + + assert list((tmp_path / "videos").glob(".delete_*")), "the staged files were destroyed" + + def test_start_purges_staged_delete_when_record_is_gone(storage: DiskVideoFileStorage, tmp_path: Path): from invokeai.app.services.video_records.video_records_common import VideoRecordNotFoundException diff --git a/tests/app/services/video_records/test_video_records_sqlite.py b/tests/app/services/video_records/test_video_records_sqlite.py index 70d21be87ec..d44858bfbbe 100644 --- a/tests/app/services/video_records/test_video_records_sqlite.py +++ b/tests/app/services/video_records/test_video_records_sqlite.py @@ -7,6 +7,8 @@ behaviour so the regression cannot reappear. """ +import sqlite3 + import pytest from invokeai.app.services.board_records.board_records_sqlite import SqliteBoardRecordStorage @@ -16,6 +18,7 @@ from invokeai.app.services.shared.sqlite.sqlite_database import SqliteDatabase from invokeai.app.services.users.users_common import UserCreateRequest from invokeai.app.services.users.users_default import UserService +from invokeai.app.services.video_records.video_records_common import VideoRecordNotFoundException from invokeai.app.services.video_records.video_records_sqlite import SqliteVideoRecordStorage from invokeai.backend.util.logging import InvokeAILogger from tests.fixtures.sqlite_database import create_mock_sqlite_database @@ -207,3 +210,51 @@ def test_videos_survive_owner_deletion_and_remain_admin_only(self, migrated_db: # ...and no regular user inherits it. other_view = store.get_many(user_id="bystander", is_admin=False) assert "doomed.mp4" not in {v.video_name for v in other_view.items} + + +def test_get_propagates_a_storage_error_instead_of_reporting_the_row_missing(store: SqliteVideoRecordStorage): + """An unreadable database must not be indistinguishable from a deleted video. + + `get` used to translate every sqlite3.Error into VideoRecordNotFoundException, which made + that exception mean "the row is absent, OR the read failed". Two callers act destructively + on it: `_assert_video_read_access` answers 404 for a positive not-found and the clients drop + their reference to the video on one, and the staged-delete recovery reads it as proof the + delete committed and purges the staged files. + """ + _save(store, "video-1.mp4", "user-1") + # A real storage failure rather than a patched one: the SELECT below cannot run at all, the + # same shape a locked or corrupt database presents. The row's absence is not what is being + # reported, and the caller must be able to tell. + with store._db.transaction() as cursor: + cursor.execute("DROP TABLE videos;") + + with pytest.raises(sqlite3.OperationalError): + store.get("video-1.mp4") + + +def test_get_still_reports_a_positively_absent_row_as_missing(store: SqliteVideoRecordStorage): + """The narrowing stays exactly that narrow: absence is still absence.""" + with pytest.raises(VideoRecordNotFoundException): + store.get("never-existed.mp4") + + +def test_exists_reports_a_row_get_cannot_deserialize(store: SqliteVideoRecordStorage): + """Presence, not readability. `get` would raise on an enum value this version does not know + — a row written by a newer one — and the refusal path reads that as absence, which would + report a live video gone.""" + _save(store, "video-1.mp4", "user-1") + with store._db.transaction() as cursor: + cursor.execute("UPDATE videos SET video_category = 'from_the_future' WHERE video_name = ?;", ("video-1.mp4",)) + + with pytest.raises(ValueError): + store.get("video-1.mp4") + assert store.exists("video-1.mp4") is True + + +def test_exists_propagates_a_storage_error(store: SqliteVideoRecordStorage): + """ "Could not look" is not "not there" — the caller answers 404 on a False.""" + with store._db.transaction() as cursor: + cursor.execute("DROP TABLE videos;") + + with pytest.raises(sqlite3.OperationalError): + store.exists("video-1.mp4")