fix(images): make single-image and intermediate deletion transactional

[lstein]

Summary

Follow-on PR 2 (items 2 and 3) from @JPPhoto's review of #9163 — the "Single-image deletion is nontransactional and reports failure as success" and "Intermediate-image cleanup deletes records before files" findings. (Item 1 of that list, the image list/names ownership filter, was folded into #9358 where it belongs thematically.)

Note

Stacked on #9163 — this branch is based on the WAN video branch because it reuses the stage_delete/commit_delete/rollback_delete machinery and startup recovery that only exist there. The diff will show #9163's changes until it merges; only the top commit (fcef797e26) is new. I'll rebase/retarget once #9163 lands.

Single-image deletion (ImageService.delete)

Previously files were permanently removed before the DB record was deleted — a DB failure left a live record pointing at missing files, and the route swallowed the exception and returned HTTP 200 with an empty result (the frontend treated that as success and dropped the item from its cache).

Now, mirroring the reviewer-approved video pattern: stage image+thumbnail → delete record → commit stage → fire callbacks. On DB failure the staged files are rolled back to their original paths and the error re-raises; a failed rollback is logged without masking the DB error; a failed final purge is logged but doesn't fail the deletion (startup recovery cleans the staging dir). The delete_image route returns 404 for a missing image and 500 on service failure instead of a success-shaped payload, mirroring the reviewed delete_video route.

Intermediate cleanup (ImageService.delete_intermediates)

Previously records were deleted first, then files sequentially — a filesystem failure orphaned files and aborted cleanup of later entries.

Now all-or-nothing, favoring the existing integer response as the review suggested: stage every intermediate file first (any staging failure rolls back all prior stages with per-item isolation and aborts before any record is touched) → delete all records in one delete_many (deleting exactly the staged names avoids racing an intermediate created mid-operation) → commit stages with per-item isolation → callbacks only for committed deletions. No .delete_* dirs remain after success. The destructive DB-layer delete_intermediates() is replaced by a read-only get_intermediates() so listing and record deletion are separate steps (query-level only, no migration).

Deliberately unchanged

delete_images_from_list / delete_uncategorized_images keep their per-image partial-success reporting — each per-image failure now goes through the transactional delete(), so no record/file divergence can occur; only the reporting style is preserved. delete_images_on_board and the video services already used the staged pattern.

Tests (per JPPhoto's specs)

• Service, single delete (tests/app/services/images/test_images_default.py, real DiskImageFileStorage + mocked records): success deletes image, thumbnail, record, and fires callback exactly once with no staging dirs left; staging failure keeps the record; DB failure restores image and thumbnail on disk; rollback failure still surfaces the DB error; purge failure logged, not raised.
• Service, intermediates: multi-intermediate success; first and later staging failures (all prior stages rolled back, records untouched); DB failure restores all staged files; one rollback failure doesn't abandon the remaining rollbacks (all attempted); commit failure logged with remaining commits attempted and callbacks fired only for committed deletions.
• Route (tests/app/routers/test_images.py, real service + disk + SQLite): success returns the deleted name; missing image → 404; DB failure → 500 with image and thumbnail restored and the record intact — no success-shaped payload.
• DB: get_intermediates() returns (name, subfolder) pairs without deleting.
• One pre-existing multiuser test (test_non_owner_can_delete_image_from_public_board) previously "passed" only because the route masked a service crash behind 200-empty; it now wires the needed services and asserts the actual deletion — strictly stronger.

Full sweep of image/board/video service and route tests: 457 passed; ruff clean.

🤖 Generated with Claude Code

[JPPhoto]

Merge blockers:

• invokeai/app/services/images/images_default.py:379-387: Cleanup snapshots intermediates, then unconditionally deletes names after the DB window. If an image becomes non-intermediate meanwhile, its record and staged files are deleted. Test: stage img, change is_intermediate to FALSE before delete_many, assert record remains.

• invokeai/app/api/routers/images.py:214-217: Every get_dto() failure becomes 404, including DB/URL failures for existing images. Test: make get_dto raise RuntimeError; current route returns 404, expected 500.

Suggestions:

• Consider conditional delete_many(... WHERE is_intermediate = TRUE) or one transaction covering selection and deletion.

[lstein]

Thanks — both blockers are fixed in 82cb7b3.

Cleanup deleting images that stopped being intermediates

Took the second half of your suggestion (one transaction covering selection and deletion), then went further, because the obvious version of it doesn't actually hold.

A new image_records.delete_intermediates_by_names(names) replaces the unconditional delete_many(...). Two things worth calling out:

The predicate rides on the DELETE, not on a preceding SELECT. My first attempt did SELECT ... WHERE is_intermediate = TRUE and then DELETE ... WHERE image_name IN (selected), both inside one self._db.transaction() block, on the theory that sharing the transaction made it atomic. It doesn't. SqliteDatabase connects with the default isolation_level="", and Python's legacy transaction control issues the implicit BEGIN only before a write — never before a SELECT. The SELECT therefore holds no read lock, and a promotion landing between the two statements still loses the record. In-process this is masked by the process-wide RLock on the shared connection, but it's reachable cross-process (gallery_maintenance.py and import_images.py each open their own connection). The DELETE now carries AND is_intermediate = TRUE itself, so correctness no longer depends on isolation at all. test_promotion_interleaved_inside_the_call_keeps_the_record promotes from inside the call to cover it.

The method reports (deleted, retained), and only retained earns a file restore. A name is omitted from deleted for two different reasons: it was promoted, or its record was removed by something else while we held its files staged. Restoring files in the second case strands them permanently — no row references them, and rollback removes the staging dir, so startup recovery can't find them either. The rollback path also re-checks that the record is still there immediately before restoring, since the loop can work through thousands of other names first, and errs toward keeping the files when the database can't answer.

Also chunked the name list at 500 bound parameters. The previous delete_many(all_intermediate_names) could exceed SQLITE_MAX_VARIABLE_NUMBER on a large library, where the pre-PR code used an unparameterized DELETE ... WHERE is_intermediate = TRUE and had no such limit.

Every get_dto() failure becoming 404

The route now maps ImageRecordNotFoundException to 404 and everything else to 500, per your test.

That split alone doesn't accomplish anything, though: SqliteImageRecordStorage.get() (and get_metadata()) converted every sqlite3.Error into ImageRecordNotFoundException, and get_dto() calls get() first. A locked or faulted database on a live image therefore still produced a 404 — precisely the "frontend drops a live item from its cache" failure the change exists to prevent. Your RuntimeError probe passes either way, since that isn't a sqlite error. Both methods now raise not-found only when the row is genuinely absent, and the route test drives a real sqlite fault through the real store rather than stubbing get.

Blast radius is small. Three sites discriminate on the exception type:

• the delete route (intended);
• bulk_download_default.py:59-69, which now signals job-failed and re-raises a DB fault instead of swallowing it as not-found — that file's existing policy for unexpected errors, so I left it alone;
• image_files_disk.py:322-330, where this is a strict improvement: a transient DB fault during __recover_staged_deletes used to read as "record gone" and shutil.rmtree a live image's staged files.

Every other caller of get()/get_metadata() uses a bare except Exception and is unaffected.

Tests

Store level: promotion both before and interleaved inside the call, chunk boundaries, and names whose records are already gone. Service level: wired to a real SqliteImageRecordStorage and a real DiskImageFileStorage, since the mocked tests can only assert that the service honours whatever the store reports, and the promoted-vs-already-gone distinction is made in the store.

One process note, since it affects how much the green check is worth: three of the tests in my first pass at this passed against both the fixed and the broken code — one had setup that kept the image out of the snapshot entirely so it never reached the path under test, one used a sqlite3 trace hook that fires when a statement begins and so landed before the SELECT read anything, and one stubbed the very method it claimed to exercise. I caught them by reverting each fix and confirming the matching test failed. Every test here has now been through that check.

tests/app: 2227 passed, ruff clean.

Left alone deliberately

• Staging every intermediate before any DB work means all of those images are unreadable for the duration of the operation, so a graph reading a canvas intermediate mid-cleanup will fail its node. That's inherent to the all-or-nothing shape rather than something this PR introduces — happy to bound it if you'd rather.
• clear_intermediates is async def with a fully synchronous body and blocks the event loop for the whole operation. fix(api): stop synchronous route work from stalling the whole server #9436 is moving routes off the loop separately.
• assert_image_owner raises 403, not 404, for a nonexistent image to a non-admin, so the 404 branch is effectively admin/single-user only. Pre-existing and untouched here.

[JPPhoto]

Merge blockers:

• invokeai/app/services/images/images_default.py:428-430: Retained-row check and staged-file restore are separate operations. Concurrent single-image or board deletion can remove row after _record_still_exists() returns but before rollback_delete(); cleanup then restores image and thumbnail, removes staging dir, and leaves permanent unreferenced files. Reproduced by deleting promoted row from image_records.get() after reading it: delete_intermediates() returned 0 with row absent, both files present, and zero staging dirs. Test: add this interleaving test; expect absent row to leave files purged.

Suggestions:

• Instead of releasing record-store transaction before retained-token rollback, restore retained files while same write transaction is held; competing deletes then run after restoration and can stage those files.

[lstein]

Round-2 blocker (promoted-file orphan) fixed in ccfc32f8f3.

Your suggestion — restore the retained files while holding the record-store write transaction — narrows the window but doesn't close it: the competing single/board delete's file-staging (stage_delete) runs under no lock, so it can stage-empty before our restore and then delete the record after we release, resurrecting the files as an orphan regardless.

So I dropped the stage-then-restore approach entirely and went records-first: delete_intermediates() now conditionally deletes the records first, gets back exactly the names it removed, and purges only those files (best-effort — a filesystem failure orphans one file, logged, but never aborts the batch or undoes a committed deletion). A promoted image is never deleted and its files are never staged, so there is no restore step for a concurrent delete to race. delete_intermediates_by_names() now returns just the deleted names instead of (deleted, retained). A regression test concurrently deletes a promoted image right after the conditional DELETE keeps it and asserts its files aren't resurrected.

[JPPhoto]

Merge blockers:

• invokeai/app/services/images/images_default.py:410-422: Records commit before file purge. A staging failure, or crash before line 414, leaves files without records or recovery journal; service still returns success. Test: fail stage_delete() after delete_intermediates_by_names() succeeds; assert record absent, files present, no .delete_*. Docs: docs/src/content/docs/features/gallery.mdx.

• invokeai/app/services/images/images_default.py:298-326: Concurrent deletes can both stage one image; one DB delete succeeds while the other fails and rolls back its nonempty token, restoring files after the record is gone. Test: barrier after both staging calls, make one record delete fail and the other succeed, then assert absent record plus present files.

Suggestions:

• Instead of records-first deletion, use a durable deletion journal or coordinated per-image lock; preserve retryability across crashes and filesystem failures.

• Consider making staged-delete ownership explicit; a failed delete must not restore files after another request removed the record.