Skip to content

fix(api): report partial failures and bound batch bodies on image routes - #9394

Open
lstein wants to merge 4 commits into
invoke-ai:mainfrom
lstein:fix/images-batch-partial-failures-and-bounds
Open

fix(api): report partial failures and bound batch bodies on image routes#9394
lstein wants to merge 4 commits into
invoke-ai:mainfrom
lstein:fix/images-batch-partial-failures-and-bounds

Conversation

@lstein

@lstein lstein commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-on to #9163 (deferred non-merge-blocker). The video endpoints got these three fixes during the review; the image endpoints they were modelled on never did, so images and videos in the same selection behave differently today.

1. Batch star/unstar aborted on the first foreign name

star_images_in_list / unstar_images_in_list did except HTTPException: raise inside the per-name loop. One name the caller doesn't own — or one deleted by a concurrent session — discarded the response payload for every image that had just been starred. The client then never invalidated those caches, so the UI showed them unstarred until a full refresh.

They now skip such names, matching delete_images_from_list and the video routes, and dedup repeated names so a name repeated in one request can't land in both result buckets.

test_non_owner_cannot_star_image is updated for the new response shape (200 + empty starred_images instead of 403) and now also asserts the underlying record is still unstarred — the authorization guarantee is unchanged, only the reporting is.

2. Storage failures were silently reported as success

The same loops swallowed real failures with except Exception: pass, so a star that never reached the DB came back looking applied and vanished on reload. StarredImagesResult / UnstarredImagesResult gain a failed_images list (mirroring DeleteImagesResult and StarredVideosResult), populated for genuine failures only — an auth skip is not a failure and must not be toasted as one. The frontend toasts the partial-failure warning the video mutations already show.

3. Unbounded request bodies and pagination

  • image_names on delete/star/unstar/images_by_names was unbounded; each name costs at least one DB lookup, so an authenticated client could pin a worker with one request. Adds MAX_IMAGE_BATCH_SIZE = 1000 (mirroring MAX_VIDEO_BATCH_SIZE) plus a 255-char per-name cap.
  • list_image_dtos had no pagination bounds. A negative LIMIT means unlimited in SQLite, so limit=-1 materialized every image row into a DTO. Adds ge=0 / le=MAX_PAGE_SIZE, matching the video list route. Lower bound is 0, not 1 — the frontend issues limit=0 count-only queries (useHasImages).

Testing

  • pytest tests/app/routers/test_images.py — 32 passed (7 new).
  • pytest tests/app/routers — 615 passed.
  • Frontend: lint:tsc, lint:eslint, lint:prettier clean; test:no-watch 1554 passed. schema.ts regenerated with the locked toolchain.

🤖 Generated with Claude Code

Three related gaps on the image endpoints, all of which the video endpoints
already handle (they were fixed there during the invoke-ai#9163 review):

1. `star_images_in_list` / `unstar_images_in_list` re-raised the first
   HTTPException mid-batch, so one foreign name discarded the response payload
   for images that HAD been starred — the client never invalidated their caches
   and the UI showed them unstarred until the next full refresh. They now skip
   foreign/missing names like `delete_images_from_list` does, and dedup repeated
   names so one name can't land in two result buckets.

2. Those same handlers swallowed genuine storage failures with `except
   Exception: pass`, reporting a success-shaped response for images that were
   never updated. `StarredImagesResult` / `UnstarredImagesResult` gain
   `failed_images` (mirroring `DeleteImagesResult` and the video models), and the
   frontend toasts a partial-failure warning like the video star/unstar
   mutations do.

3. The `image_names` batch bodies (delete/star/unstar/images_by_names) were
   unbounded, and `list_image_dtos` had no pagination bounds — a negative LIMIT
   means *unlimited* in SQLite. Adds MAX_IMAGE_BATCH_SIZE (mirroring
   MAX_VIDEO_BATCH_SIZE), a 255-char per-name cap, and ge=0/le=MAX_PAGE_SIZE on
   the list route. The lower bound on `limit` is 0, not 1: the frontend issues
   count-only queries with limit=0.

Deferred non-blocker from PR invoke-ai#9163.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added api python PRs that change python files services PRs that change app services frontend PRs that change frontend files python-tests PRs that change python tests labels Jul 28, 2026
…atch bounds

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lstein lstein added the 6.14.1 label Jul 28, 2026
@lstein lstein moved this to 6.14.1: Bug fixes to 6.14.0 in Invoke - Community Roadmap Jul 28, 2026

@JPPhoto JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came across the following issue:

  • invokeai/app/api/routers/images.py:695-727: /download still accepts unbounded image_names, performs per-name authorization, then schedules bulk work. An authenticated client can submit an oversized body and consume request/DB resources despite the new batch limits elsewhere. Test: POST 1001 names and assert 422 before authorization or background-task execution.

Suggestions:

  • Consider a shared ImageNamesBatch model for every explicit-name batch endpoint; this would apply limits consistently and prevent /download drift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.14.1 api frontend PRs that change frontend files python PRs that change python files python-tests PRs that change python tests services PRs that change app services

Projects

Status: 6.14.1: Bug fixes to 6.14.0

Development

Successfully merging this pull request may close these issues.

2 participants