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

[lstein]

Summary

Follow-on to #9163 (deferred non-merge-blocker). The video endpoints got these 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 mutations aborted on the first foreign name

star_images_in_list / unstar_images_in_list — and, as of the latest round, add_images_to_board / remove_images_from_board — 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 already been changed. Those changes are committed; only the report is lost, so the client never invalidated their caches and the UI showed them stale until a full refresh.

All four 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.

The authorization guarantees are unchanged — only the reporting is. test_non_owner_cannot_star_image and test_non_owner_cannot_batch_add_other_users_images_to_own_board are updated for the new response shape and now also assert the underlying mutation was never attempted.

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 / AddImagesToBoardResult / RemoveImagesFromBoardResult gain a failed_images list (mirroring DeleteImagesResult), 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; deleteImages gains it too, having never surfaced its own failed_images at all.

3. Unbounded request bodies and pagination

• image_names was unbounded on every explicit-name batch route: delete, star, unstar, images_by_names, download, and both board_images batch routes. Adds MAX_IMAGE_BATCH_SIZE = 1000 (mirroring MAX_VIDEO_BATCH_SIZE) plus a 255-char per-name cap, uniformly.

  The per-name cost is worse than "one DB lookup". The helpers in _access.py short-circuit on the first hit, so an admin or a direct owner costs 0–1 queries — 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. The routes are async def and the loops are synchronous, so that work blocks the event loop. Hence one bound everywhere, with no route granted a laxer one — and remove_images_from_board memoizes its per-board check, since skipping removed the early abort that used to cap an unauthorized batch at one lookup.

• 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).

4. Nothing capped the selection those bodies come from

The bound above is only safe because the client stopped exceeding it. It doesn't: selectAllOnPage (mod+a) reads the whole board's name list from an unpaginated endpoint, so one keystroke on a large board produces a selection an order of magnitude past the cap, and no batch call chunked. Delete was the worst case — handleDeletions swallows the rejection, so an oversized delete silently did nothing at all.

All seven batch calls now split oversized bodies into conforming requests. The five mutating ones merge the per-chunk results, so callers and invalidatesTags still see one aggregate result with the shape a single request would have returned; images_by_names concatenates (a plain ordered list, and its caller only upserts by name); /download cannot merge — each request produces its own bulk-download item, so an oversized selection becomes several zips. That is fine in practice: the socket handler already fetches and saves per bulk_download_complete event, keyed on the event's item name rather than on the POST payload.

Chunks run sequentially. Each is already up to 1000 names of DB work, and firing them concurrently would hand straight back what the bound took away.

A mid-run failure resolves to a partial success, not an error — the same principle as §1. The earlier chunks are already committed, and returning a bare error would discard their payload. It isn't 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 so one toast reports one total, and only a run where nothing landed surfaces as an error.

Testing

• pytest tests/app — 2213 passed, 8 skipped, 6 xfailed.
• New test_every_image_names_body_is_bounded walks the published OpenAPI schema and fails if any image_names request body ships without both a list bound and a per-name length bound. It pins the exact route set rather than a count floor, so a route the walk skips — a body nesting image_names inside a model instead of declaring it flat — can't pass unnoticed.
• Frontend: lint:tsc, lint:eslint, lint:prettier clean; test:no-watch 1797 passed (8 new, covering chunk splitting, result merging, and both failure paths). schema.ts / openapi.json regenerated with the locked toolchain.
• Every fix was reverted individually to confirm its test fails against the un-fixed code. That check earned its keep: /board_images/batch/delete turned out to have no authorization test anywhere, and could not have had one — the fixture left urls as None, so ImageService.get_dto raised AttributeError for every image and every name was skipped before the ownership check ran. Any test written there would have passed regardless of what the route did. Fixed, and the remove side now has three.

Reviewer notes

Two things I looked at and deliberately left alone, both pre-existing and both PR-wide rather than specific to these routes:

• The assert_image_move_maintenance_inactive() pre-check loop (present on star/unstar and both board routes) still uses the raising helpers, so during an image-move maintenance window a batch containing one foreign name 403s where it would otherwise return 200/201. Consistent across every route that has the pre-check; changing it is a separate decision about whether maintenance state should stay behind an ownership check.
• On remove_images_from_board the authorization is board-level, not image-level, so a skip does not strictly mean "not yours": an image you own that sits on a Shared board you don't own is skipped silently. Reachable only via an admin move or a visibility change after the fact, and the previous behaviour (403, aborting the whole batch) didn't move it either — but worth knowing.

🤖 Generated with Claude Code

[JPPhoto]

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.

[lstein]

Confirmed, and fixed — thanks, this was a good catch and pulling on it turned up more than the one route.

/download now carries the same ImageName / MAX_IMAGE_BATCH_SIZE constraints as its four siblings. Rejection is FastAPI request validation, so it lands before any authorization lookup or background task, and your test is in: test_image_name_batches_are_bounded posts MAX_IMAGE_BATCH_SIZE + 1 names to all seven batch routes and asserts 422, plus that neither the image service nor the bulk-download service was touched.

On the shared ImageNamesBatch model — I went with a drift guard instead. test_every_image_names_body_is_bounded walks the published OpenAPI schema and fails if any image_names request body ships without both a list bound and a per-name length bound. It pins the exact route set rather than a count floor, so a route the walk skips (a body that nests image_names inside a model instead of declaring it flat with Body(embed=True)) can't slide through unnoticed either. That covers the same drift without renaming five body schemas, but happy to do the model too if you'd rather have it.

Three things fell out of chasing it, all now in the PR:

Two more unbounded routes. /board_images/batch and /batch/delete were unbounded too, and loop per name for permission checks. Same treatment.

The comment on MAX_IMAGE_BATCH_SIZE understated what it guards. I had written "each name costs at least one DB lookup". True for an admin or a direct owner — the helpers in _access.py short-circuit — 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. The routes are async def with synchronous loops, so that blocks the event loop. I'd briefly given /download a laxer bound of 10k on the strength of the wrong cost model; it's back to 1000 like everything else. Related: extending the skip-don't-abort fix to the board routes removed the early abort that used to cap an unauthorized batch at one check, so remove_images_from_board now memoizes its per-board lookup.

The client could exceed the bound in one keystroke. mod+a in the gallery selects the whole board — it reads an unpaginated name list — and nothing chunked. So the bounds this PR adds were about to break large selections, and delete was already the worst case: handleDeletions swallows the rejection, so an oversized delete silently did nothing. All seven batch calls now split oversized bodies client-side and merge the results. /download can't merge (one request, one zip), so an oversized selection becomes several zips — the socket handler already fetches per bulk_download_complete event, keyed on the event's own item name.

A mid-run chunk failure resolves as a partial success rather than an error, for the same reason §1 of this PR exists: the earlier chunks are committed, and a bare error discards their payload. That one isn't only about the RTK cache — handleDeletions strips deleted images out of nodes, canvas layers and reference images off deleted_images, and none of that runs on a rejection.

Two things I looked at and left alone deliberately, both pre-existing and PR-wide rather than specific to these routes — written up at the bottom of the PR description: the maintenance pre-check loop still 403s a batch containing a foreign name, and on remove_images_from_board the check is board-level so a skip doesn't strictly mean "not yours". Say the word if you'd rather either land here.

Full suites green: pytest tests/app 2213 passed, frontend 1797 passed, lint clean, generated artifacts regenerated with the locked toolchain. Every fix was reverted individually to confirm its test fails against the un-fixed code — which is how I found that /board_images/batch/delete had no authorization test at all and couldn't have had one: the fixture left urls as None, so get_dto raised AttributeError for every image and every name was skipped before the ownership check ran. Fixed, and it has three now.

[JPPhoto]

A few things to fix:

• invokeai/frontend/web/src/services/api/endpoints/images.ts:616-631 can schedule earlier download chunks, then reject after a later chunk fails. The UI shows “request failed” while earlier ZIPs still arrive. Test: mock chunk 1 as 202 and chunk 2 as 403; verify partial status handling.

• invokeai/app/api/routers/images.py:534-547, 624-638, 670-681 reports ImageRecordNotFoundException as failed_images after authorization passes. Concurrent deletion is an intentional skip, not a storage failure. Test: pass ownership, raise the exception during the subsequent service call, and assert the name is absent from both result lists.

Suggestions:

• Consider one server-side download job for the full authorized selection; this preserves atomic authorization and one coherent result.

• Instead of generic-catching not-found races, classify ImageRecordNotFoundException as a skip; this aligns image routes with video and board-removal behavior.

• Consider deriving the client chunk size from the generated schema; this prevents frontend/backend limit drift.

[lstein]

Both confirmed and fixed at 93420df. Also merged main (the branch was conflicting; the only real conflict was two additive en.json keys — schema.ts/openapi.json regenerate byte-identical to the merge result).

1. images.ts:616-631 — partial download reported as a total failure

Confirmed. The route answers 202 the moment it schedules the background task, so by the time chunk 2 returns 403, chunk 1's zip is already being built. The bare error drove bulkDownloadImages.matchRejected → "Problem preparing download", while chunk 1's zip landed in the user's downloads anyway via bulk_download_complete. The mutating batch routes already resolve this correctly in buildChunkedImageBatchQueryFn; the download path was the one that didn't.

It now follows the same rule: only a run where nothing was scheduled surfaces as an error; a partial run resolves with the first chunk's payload and warns with the count of names that made it into no zip. Own toast id — the toast system updates in place, so sharing IMAGES_FAILED_TO_UPDATE would let one count silently replace the other.

One thing that fell out of writing the test: "something was scheduled" can't be inferred from the payload. fetchBaseQuery resolves an empty response entity as data: null, so a 202 whose body didn't survive the trip back leaves nothing to return even though the task was scheduled — a truthiness check on the payload would take the nothing-happened path and toast an error over an arriving download. It's tracked in its own flag, and there's a test that fails against the truthiness variant.

The queryFn is extracted as bulkDownloadQueryFn so it's testable. Tests cover: one request per chunk; mid-run failure resolves and reports the exact unreached count (1500, not 500 — asserted on the interpolation, since an off-by-one-chunk error is invisible if you only assert the toast id); first-chunk failure still errors.

2. images.py — ImageRecordNotFoundException as failed_images

Confirmed, and the reachability is worth recording. Star/unstar reach it through the get_dto read-back inside ImageService.update — the UPDATE matches no row and raises nothing, so a name that vanished mid-batch surfaces only on the read that follows. All three routes now skip it, matching remove_images_from_board, and the name is absent from both result lists as you asked.

The skip was unsound as a straight except … : continue, though, and fixing that is the larger half of this commit. image_records.get() re-raised every sqlite3.Error as ImageRecordNotFoundException:

except sqlite3.Error as e:
    raise ImageRecordNotFoundException from e

so a locked, corrupt or unreadable database was indistinguishable from a concurrent delete. With the skip in place, a 500-name delete against a faulted DB would have answered 200 {"deleted_images": [], "failed_images": [], "affected_boards": []} — no toast, no cache invalidation, total silence — which is strictly worse than the spurious warning the skip removes. Worse, it was role-asymmetric: get_user_id has no such translation, so the fault would still be reported for a caller who goes through the ownership lookup and silenced for an admin or the default single user.

I dropped the translation in get() and get_metadata(); storage errors now propagate as themselves. That also un-swallows them for the two existing not-found skips in board_images.py. Nothing regressed — the single-image routes catch bare Exception → 404 either way, and the full backend suite (4184 tests) is green.

On "aligns with video": videos.py documents that intent but has the same gap — VideoRecordNotFoundException still lands in failed_videos for an admin-equivalent caller, and video_records_sqlite.get() has the identical sqlite3.Error → not-found translation. It's outside this PR's diff so I left it; happy to file it as a follow-up.

On the two suggestions

• One server-side download job for the full selection. Agreed that it's the better shape, but it needs /download to accept an unbounded name list again, which is the bound you asked for in the previous round. Doing it properly means a different mechanism (a server-side selection handle, or expanding from the same filter the gallery used) — I'd rather not fold that into this PR.
• Deriving the client chunk size from the generated schema. maxLength does reach openapi.json, but openapi-typescript drops validation keywords, so schema.ts has no value to read at runtime. Doable by parsing openapi.json, but that file isn't shipped to the client. Left as the documented constant with a test that mirrors it.

One open question, on the delete route

You asked for the concurrently-deleted name to be absent from both lists, and that's what I did. But deleted_images is what drives handleDeletions — stripping the name out of the gallery selection, node ImageFields, canvas layers and reference images. A name in neither list means a client holding a raster layer that references it keeps a broken reference and now gets no signal at all (previously the spurious "could not be updated" toast was at least a prompt to refresh). Since the postcondition the route is asserting is exactly "this name no longer exists", putting it in deleted_images would be accurate and repair that client's state — and it's safe now that the exception can no longer be a disguised storage error. Say the word and I'll move it; I didn't want to quietly deviate from what you asked for.

[JPPhoto]

Merge blockers:

• invokeai/frontend/web/src/services/api/endpoints/images.ts:128-156,251-292 rereads auth token per chunk via invokeai/frontend/web/src/services/api/index.ts:111-125. If user A starts a large mutation, then logout/login switches to B, later chunks run as B. RTK 2.8.2 resetApiState only resets reducer state, not the running request source. Public-board permissions can let B mutate A's images. Effect: cross-user partial mutations/downloads; earlier changes cannot roll back. Likelihood: Low-medium, but reachable. Recovery: manually restore board/star state; deletes may be irreversible. Test: pause after chunk one, change token, resume, and assert later chunks abort or retain A's credentials.

• invokeai/app/api/routers/board_images.py:239-248,267-278 caches board write authorization for the entire remove batch. If a public board becomes private after the first image, later removals reuse True without rechecking. Effect: a previously authorized contributor can remove additional images after permission revocation. Likelihood: Low, but valid under concurrent multiuser updates. Recovery: restore removed board assignments. Test: change board visibility after the first removal and assert subsequent removals are skipped.

Other findings/issues:

• invokeai/app/api/routers/board_images.py:165-185 does not catch ImageRecordNotFoundException or the SQLite FK failure from invokeai/app/services/shared/sqlite_migrator/migrations/migration_1.py:22-34. Deleting an image after ownership validation makes the insert fail and adds it to failed_images, contradicting the PR metadata claim that deleted names are skipped. Effect: false failure warning for an already-deleted image. Likelihood: Medium under concurrent deletion. Test: coordinate deletion between ownership validation and insert; assert the name is omitted from failed_images.

• invokeai/frontend/web/src/services/api/endpoints/images.ts:704-747 discards all successful DTO chunks when a later chunk fails. useRangeBasedImageFetching ignores the rejected promise at invokeai/frontend/web/src/features/gallery/hooks/useRangeBasedImageFetching.ts:63-68. Effect: earlier thumbnails never enter cache after a transient later failure. Likelihood: Medium for large gallery ranges. Recovery: scroll/reload and hope the range retries. Test: return DTOs for chunk one, an error for chunk two, and assert chunk-one DTOs are still upserted.

• invokeai/frontend/web/src/features/changeBoardModal/components/ChangeBoardModal.tsx:88-118 fires the new partial-result image mutation without awaiting it, then resets selection after only the video promises settle. Effect: failed_images are left on the old board while the modal clears them, making retry difficult. Likelihood: Medium during partial batch failures. Recovery: manually reselect failed images. Test: resolve the mutation with failed_images and assert those names remain selected.

• invokeai/frontend/web/src/services/api/endpoints/images.ts:257-274 chunks image_names even when board_id is also supplied. The backend prioritizes board_id at invokeai/app/services/bulk_download/bulk_download_default.py:47-55; thus 1001 names plus a board ID schedules duplicate full-board ZIP jobs. Effect: avoidable CPU, disk, and download amplification. Likelihood: Low; no current UI caller, but the API accepts both fields. Recovery: cancel/delete duplicate downloads. Test: submit both fields with over 1000 names and assert rejection or one normalized request.

Corner/impossible cases:

• invokeai/frontend/web/src/services/api/endpoints/images.ts:262-292 permits a null 202 payload, but invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/bulkDownload.tsx:11-28 dereferences it immediately. The current route always returns a model at invokeai/app/api/routers/images.py:733-772, so this needs a body-stripping proxy. Effect: fulfilled download action throws and the preparing toast is lost. Likelihood: Very low with the current server, reachable with the proxy behavior modeled by the PR's own test. Test: dispatch a fulfilled action with null payload and assert the listener does not throw.

Suggestions:

• Instead of reading ambient auth state for every chunk, capture the auth generation at operation start and abort when it changes; this prevents cross-account continuation.

• Instead of caching board permission across a mutable batch, authorize and mutate within one transaction or snapshot; this removes the permission TOCTOU window.

• Instead of waiting for every DTO chunk before upserting, upsert each successful chunk immediately; this preserves useful results after later failures.

• Instead of accepting both board_id and image_names, reject the combination or normalize it before chunking; this eliminates duplicate full-board jobs.

• Instead of clearing board-change selection immediately, await the image mutation and retain failed_images; this makes partial moves retryable.

[lstein]

Thanks — all three confirmed and addressed in 1defaf5, with two residual gaps an adversarial pass over that fix then caught, closed in a18445b.

board_images.py single-image remove ignoring the row count — Real, and exactly the race the batch loop already classifies. Took your suggestion directly: the batch loop's zero-row classification is extracted into _remove_from_board_and_classify (outcomes: removed / moved-elsewhere / gone), and both the batch loop and the single-image route now share it. The single route reports a moved-elsewhere name in failed_images instead of a false success, treats a concurrently-uncategorized name as satisfied, and skips a concurrently-deleted one — same semantics as the batch. Tests cover all three classifications on the single route plus the already-uncategorized shortcut (no write issued at all), and each was checked to fail against the unfixed route.

images.ts treating a failed chunk as having applied nothing — Real. Full idempotency keys / server-side reconciliation is a bigger change than this PR should carry, but the harmful half is fixable client-side: for a transport-shaped failure (FETCH_ERROR, TIMEOUT_ERROR, PARSING_ERROR, 5xx — anything that doesn't prove the server refused), the failing chunk's tags are now invalidated as if it had committed, so the refetch shows the truth either way. The names still go to failed_images — that stays the honest reading, and retrying a name the server already satisfied is safe on every batch route (delete skips a gone name, star/unstar/add are idempotent upserts, remove now classifies). A 4xx keeps the old behavior: the server itself said nothing landed. The assumed-committed result is synthesized per endpoint and fed through the same getTags the endpoint publishes, so the two can't drift; since a lost chunk's affected boards are unknowable, the board-keyed tag types (Board, BoardImagesTotal, ...) are appended type-wide on this path rather than left un-invalidated. The batch remove's tag set also now invalidates failed_images DTOs, so a name the zero-row classification refuses gets refetched and shows which board it actually sits on.

bulkDownloadQueryFn final-chunk expiry window — Real. Mid-loop expiry was caught by the next chunk's pre-request check, but the final chunk has no next iteration, and fetchChunk's post-response check deliberately passes mere expiry through — so an expiry during the last chunk's await sailed into return { data: first }, and matchFulfilled raised the undismissible duration: null "preparing" toast into a dying session. A post-loop session check now applies the same triage as the in-loop expiry arm: warn (DOWNLOADS_INTERRUPTED) when zips were scheduled and the session ended, and hand matchFulfilled nothing to toast on. Everything from the check to the return is synchronous, so there is no later window behind it.

On the zips that are scheduled and then orphaned by expiry: agreed that per-account persistence + replay after re-auth is the actual fix — filed as #9531 so it doesn't live only in a code comment.

[JPPhoto]

(Note: This review excludes the issue mentioned in #9531.)

Merge blockers:

• Cross-user takeover can consume old-session results. invokeai/frontend/web/src/services/api/endpoints/images.ts:170-176,257-295 handles errors without rechecking auth. Effect: old results/tags may affect the new user's state. Likelihood: Low but plausible with multi-tab use or flaky requests. Recovery: classify auth changes before processing any error. Test: switch users before a chunk returns an error and assert no old result or invalidation is applied.

Other findings/issues:

• Lost mutation responses leave stale local references. invokeai/frontend/web/src/services/api/endpoints/images.ts:263-295 only invalidates caches; it does not report which deletes actually committed. Effect: canvas, node, and reference-image references can remain stale. Likelihood: Low-medium during timeout, disconnect, parsing, or 5xx failures. Recovery: add mutation ids and server-side outcome reconciliation. Test: commit a delete, return FETCH_ERROR, and assert local deletion cleanup.

• Storage errors during image authorization are silently skipped. invokeai/frontend/web/src/services/api/endpoints/images.py:670-688,722-741 catches all HTTPException, while _access.py:29-40 converts board lookup failures into 403s. Effect: star/unstar can report success without updating or warning. Likelihood: Very low. Recovery: distinguish authorization denial from storage failure. Test: make board lookup raise a database error and assert failed_images.

Suggestions:

• Instead of treating indeterminate mutations as failed without an outcome, use per-operation ids and a reconciliation endpoint, because cache invalidation cannot repair persisted workspace references.

• Consider typed authorization and storage exceptions, because this prevents database faults from being mistaken for ordinary permission skips.

[lstein]

All three confirmed. The blocker and the star/unstar finding are fixed in 8a508f7 (+ a coverage pin in 4c56cd0, the new head); the lost-response finding is the one thing I'm deliberately not solving inside this PR — filed as #9533 with the design constraint that makes it a server-side change, reasoning below.

Takeover can consume old-session results on the error path — Real, and it's a regression my own round-9 change created: the "error carries nothing the next session could consume" rationale in fetchChunk stopped being true the moment the indeterminate-error arm started dispatching as-if-committed invalidations and the partial path started returning aggregates the UI applies. fetchChunk now triages an error response exactly as it triages a success: token present + context mismatch → rewritten to the auth-changed hard abort, so the loops consume nothing — no dispatch, no partial payload, no toast. Mere expiry (token gone) still passes the raw error through untriaged, because the everyday expired-session 401 must keep reaching the partial path where committed work is reported and handleDeletions prunes. The download loop's behavior is unchanged — its triage arm already matched !isSameAuthContext alongside the sentinel, so the rewrite lands in the same branch it did as a raw error. Test stages your exact sequence: switch users while a chunk is in flight, chunk returns an (indeterminate, worst-case) error, assert no invalidation and no result are applied.

Storage errors laundered into auth skips on star/unstar — Real, and took both halves of your typed-exceptions suggestion as far as this PR's routes go: assert_image_owner's board fallback now reads the board record (owner + visibility are all the decision needs — the DTO's cover/count queries only added failure modes, to the point that a failed cover resolution used to 403 a legitimate owner), catches only BoardRecordNotFoundException (a board positively known to be gone is still the ordinary 403/auth-skip), and lets storage errors propagate into each batch loop's failed_images arm. Tests pin both directions: a locked database lands the name in failed_images (star and unstar), and a vanished board stays a silent skip rather than becoming a spurious failure toast. Two knowingly-left residuals, both pre-existing and flagged in earlier rounds: _assert_video_owner and the read-side assert_image_read_access still catch broadly — the video twin is the same fix if you want it in this PR, but it widens the diff into the video routes again; the read side fails closed (a denied read is safe in a way a silently-skipped write is not).

Lost responses leave stale workspace references — Real, and I attempted the client-side version before concluding it must be yours truly's follow-up instead: the natural probe (images_by_names on the failing chunk, absent ⇒ deleted) is unsafe with that route's semantics, because it answers per-name auth failures and per-name storage errors with a silent skip — a locked database returns 200 [], which would "confirm" every probed name deleted and mass-prune canvas/nodes/reference-image entries for images that all still exist. Absence from that response is not evidence of deletion, so any honest fix needs a server-side source of truth. Filed as #9533 with two shapes: the tri-state existence route (existing / gone / undecided, where gone requires a positive not-found read, and storage errors and unreadable names answer undecided so a dead database prunes nothing), or your per-operation ids + outcome endpoint, which also covers the non-delete mutations and retry dedup. Round 9's invalidate-as-committed remains the client-fixable half.

[JPPhoto]

Merge blockers:

• In-flight session A 401s can log out session B before the new guard runs. invokeai/frontend/web/src/services/api/index.ts:141-146 dispatches logout without checking the request token is still current; invokeai/frontend/web/src/services/api/endpoints/images.ts:170-183 checks only afterward. Effect: cross-user session destruction. Likelihood: Low-medium. Recovery: reauthenticate. Test: return A's 401 after B takes over and assert B remains authenticated.

Other findings/issues:

• Lost delete responses leave stale workspace references. invokeai/frontend/web/src/services/api/endpoints/images.ts:270-302 reports uncertain names only as failed, while invokeai/frontend/web/src/features/deleteImageModal/store/state.ts:96-126 cleans only deleted_images. Effect: deleted images remain in canvas, nodes, or reference-image state. Likelihood: Low-medium. Recovery: manual cleanup or reconciliation. Test: commit a delete, return FETCH_ERROR, and assert local references are removed.

• Star/unstar can commit, then report failure without invalidating the image cache. invokeai/app/services/images/images_default.py:128-143 updates the record before its DTO read; invokeai/app/api/routers/images.py:670-688,723-734 reports later errors as failed_images, but invokeai/frontend/web/src/services/api/endpoints/images.ts:323-337 invalidates only successful names. Effect: stale starred state. Likelihood: Low. Recovery: invalidate failed-image tags and refetch. Test: commit the update, fail DTO retrieval, and assert the image cache is invalidated.

Suggestions:

• Instead of dispatching session expiry for every token-bearing 401, verify that the request token is still current, because an old request must not terminate a newly adopted session.

• Instead of treating uncertain deletes as permanently failed, use operation IDs and outcome reconciliation, because retrying cannot repair local workspace references.

• Instead of separating board authorization from mutation, use a transaction or conditional write, because per-name checks still leave a permission-revocation race.

[lstein]

All three confirmed and fixed, at e97dc0bee9. The blocker turned out to have a second half that my first fix only postponed, and chasing it down surfaced a regression I had introduced in the process — both are in the four commits below.

Merge blocker: an in-flight 401 logging out the session that replaced it

Confirmed, and worse than one site. dynamicBaseQuery compared nothing: it ended the session for any 401 that had carried a token, using the token captured when the request went out. A request still in flight when someone else takes the tab over — a login here, or one in another tab, since localStorage is shared — logged out the user who never issued it.

The session now ends only while the token that was sent is still the live one (shouldEndSessionForUnauthorized). Byte equality, deliberately the opposite of what isSameAuthContext wants: a sliding-window refresh must not qualify either, because a 401 for the token it replaced says nothing about the replacement. Nothing is lost by declining — if the session really has ended, the next request carries the live token and its 401 ends it there.

ProtectedRoute was doing the same thing with no check at all, and its own comment pointed at the handler that no longer covered it. It matters more than it looks: sessionExpiredLogout removes auth_token from shared localStorage, so the tab acting on the stale 401 deletes the other tab's credential, and that tab's poll then logs itself out too. Reachable exactly where a stale token is most likely to be in flight — page load, when the query goes out with an expired token and another tab logs in before the response lands.

Gating that effect on the same predicate was not enough, which my own adversarial pass caught: the store's token catches up when the poll adopts the new one, the effect re-runs, and the superseded 401 is still in the cache. getCurrentUser is shared across logins, its argument never changes, it carries no tags, and the API-state reset that a login normally brings is deliberately skipped when the new token belongs to the same user (pinned in store.test.ts as "retains API data when another tab refreshes the same user token"). On the ordering where the storage event beats the response — the common one, since it needs no network round trip — the guard never even delayed it.

So the query is now keyed by the token it will carry. It takes an argument it never sends, because the token travels in a header, but it is what the answer is about: the adopted session reads its own cache entry, and the superseded 401 is no longer in hand to act on. The call site cannot forget to pass it — the argument type is what enforces the wiring, and reverting it fails tsc rather than a test.

Test as you asked: A's 401 returns after B has taken over, and B stays authenticated. Also the refresh case and the unauthenticated case, plus a store-level test that a replacement token cannot read its predecessor's 401.

Star/unstar committing, then reporting failure without invalidating

Confirmed. ImageService.update writes the record and then reads the DTO back; a failure in that read reports the name in failed_images with the row already starred, and invalidating only the successes left the client showing the pre-star value with nothing to ever contradict it. Both tag helpers now invalidate the failed names too — getRemoveImagesFromBoardTags already did, for the same reason.

That fix needed a second one to be safe, which is the part I would flag for your attention. A node's image field and both reference-image components clear the user's input on any query error, so the refetch the new invalidation triggers could silently drop a workflow input — and it is likelier than usual to fail, since a name is in failed_images precisely when a storage failure interrupted its write. The video field was narrowed to a confirmed-gone answer for exactly this reason; the image side never was.

Narrowing it to 404 was wrong on its own, though: a deleted image only answers 404 to an admin. assert_image_read_access decides on images.user_id, which is gone with the row, so in a multiuser deployment a deleted image is indistinguishable from someone else's and both are refused 403. Requiring 404 would have stranded every deleted image in the workflows referencing it — worse than the over-broad clear it replaced.

So the clients act on 403 as well, and that puts an obligation on the answer: the read helper laundered every storage error from its board lookup into that same 403, so an unreadable database presented as a permission decision and would now take the user's references down with it. It reads the board record and catches only a positive not-found, matching what assert_image_owner already does on the mutation side. Tests pin both directions, and the 403-for-a-deleted-image behaviour the frontend now depends on.

Videos got the same pair. isVideoMissingError accepted only 404 — which no non-admin ever sees for a deleted video — so a deleted video stayed pinned in a workflow field indefinitely; the video read helper had the identical laundering. Both fixed alongside, since leaving them would have made the image behaviour look arbitrary.

Lost delete responses leaving stale workspace references

Still deferred, for the reason I gave last round: the only client-side repair is a probe that cannot distinguish "gone" from "couldn't look". images_by_names skips per-name auth failures and storage errors, so a locked database answers 200 [] — which would "confirm" every name deleted and mass-prune canvas, nodes and reference-image state for images that still exist. Your suggestion (operation ids and outcome reconciliation) is the right shape and is server-side work: #9533.

On the suggestions

Conditional writes: the remove path already got that treatment in this PR — the DELETE is scoped to the board the caller was authorized against, so a name that moved in between matches zero rows and is classified rather than reported as a success that did not happen. Extending it to star, unstar and add is worth doing, but not as a WHERE clause: the policy is a four-way disjunction across three tables, and duplicating it into SQL at each call site guarantees drift. It wants the decision pushed into the service transaction, which is #9534.

What is not covered

The three components and ProtectedRoute have no mount test — there is no @testing-library/react in the repo, so the predicates and the endpoint contract are tested directly and the component wiring is not. getCurrentUser's key is the exception: tsc enforces it. Every other behavioural change was reverted individually to confirm its test fails against the unfixed code.

The remaining laundering I know of is _assert_video_owner, on the mutation side, still catching broadly — same shape as the read helper, left alone because it changes batch outcomes rather than reference-clearing and wants its own tests.

[JPPhoto]

Merge blockers:

• invokeai/frontend/web/src/features/nodes/components/flow/nodes/Invocation/fields/inputs/videoFieldErrors.ts:2-23: Treats every 403 as deleted/unavailable. Revoking access to a shared video's board returns 403 and silently clears workflow fields. Effect: reversible permission changes cause local workflow data loss. Likelihood: Plausible. Recovery: Manual reattachment. Test: Reference a shared video, make its board private, refetch, and assert the field remains.

Other findings/issues:

• invokeai/frontend/web/src/services/api/endpoints/images.ts:271-303: An indeterminate delete chunk may commit server-side, then return an error; handleDeletions only cleans references for fulfilled deleted_images at invokeai/frontend/web/src/features/deleteImageModal/store/state.ts:96-129. Effect: Deleted images remain referenced by nodes, canvas, and control layers. Likelihood: Plausible during timeout, disconnect, or 5xx. Recovery: Manual cleanup; deleted files are unrecoverable. Test: Commit deletion, return FETCH_ERROR, and assert persisted references are removed.

Suggestions:

• Consider distinguishing confirmed deletion from denied read access in video field handling; this preserves references across reversible permission changes.

• Instead of treating an indeterminate delete chunk as an ordinary rejected mutation, reconcile it through an operation ID and outcome endpoint; this gives cleanup code definitive deletion results.

• Instead of hardcoding frontend batch limits, derive them from a shared contract or generated schema check; this prevents backend/frontend limit drift.

[lstein]

Both confirmed and fixed. You caught the half of the tradeoff I got wrong, and chasing it showed the choice was never the client's to make.

Blocker: clearing a reference on a 403

Confirmed, and the harm is exactly as you describe — the refusal is reversible and the loss is not. Flip a shared board to Private and every field pointing at its images clears; flip it back and the images are all still there, but the workflows are not.

The reason I widened it to 403 last round was the other half of the same problem: a deleted image also answered 403 to every non-admin, because the ownership decision rests on images.user_id and that is gone with the row. So 404-only stranded deleted images in workflows forever, and 404-or-403 destroyed live ones. Neither answer was the client's to guess.

So the server now draws the line it is the only party able to draw. On the refusal path — the happy path pays nothing — both read helpers ask whether the record is actually there: positively absent answers 404, present answers 403. The clients are back to treating 404 alone as gone, which is your suggestion, and the case you asked for is pinned in both directions: a shared board flipped to Private still answers 403 for an image that exists, and a deleted image now answers 404 to a non-admin instead of 403. Videos got the same pair.

The cost is that an authenticated caller can now tell an absent image from one they may not read. That is the answer admins have always received, and image names are generated UUIDs, so it is not something an attacker can enumerate — but it is a real change and worth naming.

Two consequences fell out of making 404 load-bearing, and both were pre-existing laundering of the same shape this PR has been unwinding:

• video_records.get translated sqlite3.Error into VideoRecordNotFoundException, so an unreadable database would have presented as a deleted video and cleared the user's fields. It no longer does — matching image_records.get, which stopped doing this earlier in the PR. That also un-breaks the staged-delete recovery in video_files_disk, which read the same exception as "the delete committed" and purged the staged files for a video whose record it merely could not read.
• Both DTO routes ended except Exception: raise HTTPException(404), so any failure inside get_dto — a board lookup on a locked database, not just a missing row — answered 404. With clients acting on 404 destructively that becomes data loss, so only a genuine not-found answers 404 now and everything else propagates.

Lost delete responses

The uncertain names are now refetched, which is as much of your reconciliation as can be done without the server-side outcome record.

Their references stay in place, deliberately: pruning on a guess would discard work over a request that merely failed, and handleDeletions is right to touch only deleted_images. But asking is what settles it, and it is now safe to ask, because a 404 finally means what it says. A name that is really gone answers 404 and every component holding it lets go; 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.

It is partial by construction, and I would rather say so than imply otherwise: it repairs what is mounted and subscribed. Canvas raster and control layers hold image names with no DTO query behind them, and only handleDeletions prunes those — which needs the definitive per-name outcome your suggestion describes. That is #9533, and this does not close it.

What the self-review then found in that

Four things, all fixed in the same push:

• The existence probe read the whole record, so a row this version cannot deserialize — an enum value written by a newer one — failed exactly as absence does, and would have reported a live image gone. It is a bare row probe now, which also drops it to one point SELECT per refused name.
• video_records.get not translating storage errors had no test at all, and after the DTO routes were narrowed it is the only thing standing between an unreadable database and a 404 that clears the user's fields. Pinned from both ends now: the store propagates rather than reporting the row missing, and the staged-delete recovery keeps the staged files instead of purging them when it cannot read the record.
• The delete reconciliation was inert for the deletes people actually perform. Anything up to the batch cap is a single chunk, and a single chunk that fails reports nothing back, so the endpoint's invalidation never runs on a result. The only invalidation is the one the queryFn dispatches for the lost chunk, and it described those names as committed — exactly the case the delete tag set skips DTOs for. It describes them as unconfirmed as well now, which is what they are, so the refetch actually happens. The test covers the single-chunk path specifically.
• Both arms of the reference-image reset wait for the connection; the original image's used to clear regardless of it.

On the third suggestion

Already there: IMAGE_BATCH_CHUNK_SIZE in images.test.ts reads the committed openapi.json, digs the bound out of every schema that carries an image_names array — including the nullable download body, whose anyOf hides it from a flat read — and asserts each equals the client's chunk size, with a floor on the count so it cannot pass vacuously if the server stops publishing the cap. typegen-checks keeps that file in step with the routers, so drift on either side fails in CI rather than at runtime as a batch of 422s.

[JPPhoto]

Since issue #9533 covers what's missing here, this is approved.