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.

[lstein]

Thanks — both blockers were real, and they share a root cause: the delete paths had no durable record of intent, so any failure between the record write and the file write was invisible. Fixed in c2d7dfc (plus c4c09b5, below), and main is merged in — the branch was conflicting, though all three conflicts were additive-only.

B1 — delete_intermediates() commits records before the purge, with no journal

Right. Records-first closed the promoted-image race but opened a crash window with nothing recording it, and the call still returned success.

DiskImageFileStorage now has a durable delete journal:

• begin_delete(images) — fsyncs a manifest naming every (image_name, image_subfolder) about to be purged, before any record is touched. Nothing is moved.
• commit_delete(token, image_names=None) — purges the named images' files, then drops the journal. image_names narrows the purge to the records that were actually deleted, so a promoted image keeps its files.
• abandon_delete(token) — drops the journal without purging, for when the record deletion failed.

Startup recovery reconciles any journal that outlives its operation, and the record store decides: an image whose record still exists keeps its files (and anything an older stage_delete() moved aside is restored); an image whose record is gone has its files purged. A purge that fails keeps its journal, so a transient storage error is retried at the next startup rather than logged and forgotten. A record-store fault leaves the journal alone rather than guessing — a stale file is recoverable, a deleted one is not.

Your test, as TestDeleteJournalSurvivesFailedPurges:

• test_intermediates_purge_failure_leaves_a_journal_the_next_startup_finishes — every unlink fails after the records are committed; the records are gone, the files are still there, a journal exists, and a restart removes them.
• test_a_crash_before_the_purge_leaves_a_journal_the_next_startup_finishes / test_a_crash_before_the_record_delete_keeps_the_image — the two halves of the window, at the file-storage level in TestPendingDeleteJournal.

B2 — concurrent single deletes: the failing one restores files the other unreferenced

Also right, and I don't think staged-delete ownership can be made explicit enough to fix as a check — so delete() no longer stages at all. It is records-first over the same journal: read the record, write the journal, delete the record, purge. No file is ever moved, so there is no restore for a concurrent deleter to race. A database failure leaves the image completely intact; the only state that can outlive the call is a file nothing references, which the journal covers.

That leaves delete_images_on_board(), which still stages — deliberately, because its documented contract is per-item ("if an item's files cannot be removed, the item is kept rather than left orphaned"), and that needs a pre-flight move. Your second suggestion covers it: rollback_delete() re-checks the record after restoring and purges instead of orphaning. The check is race-free rather than check-then-act because of an invariant the rest of the change establishes — every deleter purges an image's files strictly after its record is committed as gone. If the re-check sees the record, the competing purge has not run yet and will unlink what we just restored; if it doesn't, we purge ourselves.

The invariant did not actually hold — c4c09b5

I ran a fresh-context adversarial pass over c2d7dfc, and it found three paths that break exactly that invariant. Worth spelling out, since one of them is your finding in a different costume:

1. commit_delete() on a staged token only removed the staging directory. stage_delete() captures whatever is on disk at that instant, so a second board delete racing the first gets an empty token. If that second one is the one whose delete_many() succeeds, it removes the record and purges nothing — while the first one's rollback, re-checking a record that is still present at that moment, faithfully puts the files back. Permanent orphan, no journal. commit_delete() now purges the live paths too: committing means no file for that image survives, whichever request moved them. (test_committing_an_empty_token_still_purges_the_files, against a real SQLite store.)
2. create()'s cleanup after a failed save purged the files before deleting the record, so a board delete rolling back in that window was told to restore an image that was about to lose its record. Now records-first over a journal like everything else.
3. begin_delete() fsynced the manifest and the journal directory, but not the journal directory's own entry in the output folder — while SQLite does fsync the record deletion. A power loss could therefore drop the journal and keep the deletion: B1 again, one level down. Both directories are now fsynced, and stage_delete() does the same before it moves anything (it previously fsynced neither, so a lost manifest stranded staged files in a directory naming nothing).

Recovery also no longer aborts startup on a stray non-directory .delete_* entry, and logs a journal it cannot attribute instead of silently walking past files inside it.

Notes

• Recovery probes with image_records.exists() rather than a deserializing get(), and the manifest carries a list, so one journal covers a whole intermediates sweep instead of one directory per image.
• Docs updated (docs/src/content/docs/features/gallery.mdx): record first, files second, and an interrupted deletion is finished at the next startup.
• Every new test was checked against the un-fixed code — 19 of them fail there.
• Known gap, unchanged by this PR: the journal pins an image's subfolder, so a move_all maintenance run that relocates an image mid-delete can still leave a file recovery cannot find. assert_image_move_maintenance_inactive() is a check-then-act at request entry, which is the actual hole.
• VideoService has the same staging shape as the old delete() and both hazards. Untouched here to keep the diff to the images subsystem — happy to file it as a follow-up.

[JPPhoto]

Merge blockers:

• invokeai/app/services/image_files/image_files_disk.py:485-497: Recovery checks exists() before restoring staged files, then drops the journal without rechecking. A concurrent delete can remove the record and purge while files remain staged; recovery then restores orphan files and deletes the only journal. Effect: permanent unreferenced image/thumbnail. Likelihood: Low, but reachable with shared output storage. Recovery: restart cannot find it. Test: race exists() before restore, delete the record, then assert no files remain. Docs: docs/src/content/docs/features/gallery.mdx promises startup cleanup.

Other findings/issues:

• invokeai/app/api/routers/images.py:239-252: ImageRecordNotFoundException from images.delete() becomes HTTP 500. If another request deletes the image after DTO lookup, the requested postcondition is already satisfied but the client receives a failure. Effect: false error/toast and retry churn. Likelihood: Medium with double-clicks or multiple clients. Test: delete between get_dto() and service delete(); expect 404 or idempotent success.

Suggestions:

• Instead of manual recovery restore, reuse rollback_delete() or recheck record existence after restoring and before removing the journal.
• Consider catching ImageRecordNotFoundException around service deletion and treating it as 404/idempotent success. Focused storage/service/record tests passed; router run did not complete under xdist, and the context collector could not reach GitHub API.

[lstein]

Thanks — both findings confirmed and fixed in cdf8458.

Blocker — recovery restores without re-checking the record

Real, with one qualification worth recording: within a single Invoke it's unreachable, because __recover_pending_deletes() runs from start() before any request is served. It is reachable when two Invoke processes share an output folder and database, which is exactly the "shared output storage" case you named: process A's recovery sees the record, process B deletes the record and purges (finding nothing, because the files are in A's staging directory), B drops its own journal, then A restores the files and drops the last journal that could find them.

The fix is the one rollback_delete() already uses — the invariant that makes it race-free rather than check-then-act is that every deleter purges only after its record is committed as gone. Recovery now re-checks exists() after a restore (only when something was actually restored; a pending-delete journal moves nothing and can't strand anything) and purges if the record is gone. A record-store fault on the re-check propagates and keeps the journal, the same as every other lookup in that loop — I deliberately did not route it through __purge_if_record_absent(), whose swallow-the-fault behaviour is right for rollback_delete() but would have dropped the journal here.

I didn't reuse rollback_delete() itself: it takes a _StagedDelete token with pre-resolved paths, and rebuilding one from a manifest written by a possibly-older process is more machinery than a second exists() call.

Tests: test_startup_purges_restored_files_whose_record_vanished_during_recovery (record present at the first look, gone after the restore → no files, no journal), test_startup_keeps_the_journal_when_the_recheck_cannot_read_the_record_store, and a guard that a pending journal is not re-checked. Both fix-verifying tests fail against the previous source.

ImageRecordNotFoundException from images.delete() → 500

Fixed: the route now answers 404 "Image not found" for that exception, matching what the DTO lookup a few lines earlier would have answered. The service re-raises it without the "Problem deleting image record and file" error log, since nothing failed. test_delete_image_deleted_mid_request_returns_404 runs the real service and deletes the record between get_dto() and the service call; it fails against the previous route.

I chose 404 over idempotent 200 because it is what the same route answers when the record was already gone at lookup time, so a client sees one status for "gone" regardless of when it went; a 200 with deleted_images=[name] would credit this request with a deletion it didn't perform. (The web UI deletes through the batch route, which already skips names deleted mid-batch, so this mostly affects API clients.)

Housekeeping

This same review was also posted on #9396, which touches none of these files — I've left a pointer there.

One thing the adversarial pass turned up that I've left alone: this race is now answered three ways across the routes — DELETE /i/{name} → 404, POST /delete → 200 with the name in deleted_images, DELETE /uncategorized → 200 with the name in failed_images. The web UI only uses the batch route. Happy to align /uncategorized with the list route's mid-batch skip in a follow-up if you'd like.

[JPPhoto]

Merge blockers:

• invokeai/app/services/images/images_default.py:434-445: PR releases the shared DB lock after get_intermediates(). In one Invoke, Clear Intermediates can pass invokeai/app/api/routers/images.py:267-278, then background move starts via invokeai/app/api/routers/image_moves.py:62-72, moves files to a new subfolder, and updates the row. The delete then removes that row using stale subfolder data and invokeai/app/services/image_files/image_files_disk.py:306-323 purges only the old path. Effect: orphaned full image and thumbnail; journal is removed. Likelihood: Plausible same-process race between admin operations. Recovery: manual cleanup only. Test: Pause after get_intermediates(), complete same-process folder move, resume, assert row and both new-path files are gone. Focused current-head tests passed but lack this race; context collector could not reach GitHub API. Docs: docs/src/content/docs/features/gallery.mdx.

Other findings/issues:

• None.

Suggestions:

• Consider a shared image-mutation lock or reservation covering snapshot, journal, record deletion, and purge, coordinated with image-folder maintenance.