From 887ebfdcb099e9123139a1686e94c5e770e8869e Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 21:15:34 -0400 Subject: [PATCH 1/2] fix(images): make single-image and intermediate deletion transactional Addresses two review findings from JPPhoto: 1. Single-image deletion was nontransactional and reported failure as success. ImageService.delete() now stages the image and thumbnail via stage_delete(), deletes the database record, then commits the stage and fires on-deleted callbacks. A database failure rolls the staged files back to their original paths and re-raises; a failed rollback is logged without masking the database error; a failed final purge is logged but does not fail the deletion (startup recovery cleans the staging directory). The delete_image route no longer swallows exceptions into an empty 200 payload: a missing image returns 404 and a service failure returns 500, mirroring the reviewed video route. 2. Intermediate cleanup deleted records before files, so a filesystem failure orphaned files and aborted cleanup. delete_intermediates() is now all-or-nothing: every intermediate file is staged first (any staging failure rolls back all prior stages and aborts before any record is touched), records are then deleted in a single delete_many call, and stages are committed afterwards with per-item error isolation. Callbacks fire only for committed deletions and no .delete_* staging directories remain after success. The destructive ImageRecordStorage.delete_intermediates() DB method is replaced by a read-only get_intermediates() so listing and record deletion are separate steps. Test coverage: - Service: positive single-delete (files, thumbnail, record, callback exactly once, no staging dirs); staging failure; database failure with on-disk restore of image and thumbnail; rollback failure preserving the database error; purge failure logged without failing. - Service: positive multi-intermediate cleanup; first and later staging failures (mock orchestration plus on-disk restore proof); database failure restoring all staged files; one rollback failure not abandoning remaining rollbacks; commit failure logged with remaining commits attempted and callbacks fired for committed deletions. - Route: successful delete through a real ImageService with real disk storage and SQLite records; missing image returns 404; database failure returns 500 with image and thumbnail restored and the record intact. - DB: get_intermediates() returns pairs without deleting; deletion via delete_many() verified separately. The public-board delete authorization test now wires urls/image_files services and asserts the deleted payload, since the route no longer masks service failures behind an empty success response. Co-Authored-By: Claude Opus 4.8 (1M context) --- invokeai/app/api/routers/images.py | 22 +- .../image_records/image_records_base.py | 4 +- .../image_records/image_records_sqlite.py | 32 +-- .../app/services/images/images_default.py | 60 +++- tests/app/routers/test_images.py | 96 +++++++ .../routers/test_multiuser_authorization.py | 7 + .../test_image_records_sqlite.py | 21 +- .../services/images/test_images_default.py | 267 +++++++++++++++++- 8 files changed, 459 insertions(+), 50 deletions(-) diff --git a/invokeai/app/api/routers/images.py b/invokeai/app/api/routers/images.py index b9e06befb9c..bc51544080a 100644 --- a/invokeai/app/api/routers/images.py +++ b/invokeai/app/api/routers/images.py @@ -207,22 +207,24 @@ async def delete_image( _assert_image_owner(image_name, current_user) assert_image_move_maintenance_inactive() - deleted_images: set[str] = set() - affected_boards: set[str] = set() - + # Let service-level failures surface as errors rather than swallowing them and returning + # a success-shaped response. A previous version of this handler caught everything and + # returned an empty ``deleted_images`` list with HTTP 200; the frontend treated that as + # success and dropped the item from its cache even though the record was still live. try: image_dto = ApiDependencies.invoker.services.images.get_dto(image_name) - board_id = image_dto.board_id or "none" + except Exception: + raise HTTPException(status_code=404, detail="Image not found") + + board_id = image_dto.board_id or "none" + try: ApiDependencies.invoker.services.images.delete(image_name) - deleted_images.add(image_name) - affected_boards.add(board_id) except Exception: - # TODO: Does this need any exception handling at all? - pass + raise HTTPException(status_code=500, detail="Failed to delete image") return DeleteImagesResult( - deleted_images=list(deleted_images), - affected_boards=list(affected_boards), + deleted_images=[image_name], + affected_boards=[board_id], ) diff --git a/invokeai/app/services/image_records/image_records_base.py b/invokeai/app/services/image_records/image_records_base.py index 8c71dfba9e7..e72475fe1f5 100644 --- a/invokeai/app/services/image_records/image_records_base.py +++ b/invokeai/app/services/image_records/image_records_base.py @@ -70,8 +70,8 @@ def delete_many(self, image_names: list[str]) -> None: pass @abstractmethod - def delete_intermediates(self) -> list[tuple[str, str]]: - """Deletes all intermediate image records, returning a list of (image_name, image_subfolder) tuples.""" + def get_intermediates(self) -> list[tuple[str, str]]: + """Gets all intermediate image records as (image_name, image_subfolder) tuples, without deleting them.""" pass @abstractmethod diff --git a/invokeai/app/services/image_records/image_records_sqlite.py b/invokeai/app/services/image_records/image_records_sqlite.py index b9d03a81866..430e3152bcb 100644 --- a/invokeai/app/services/image_records/image_records_sqlite.py +++ b/invokeai/app/services/image_records/image_records_sqlite.py @@ -302,30 +302,20 @@ def get_intermediates_count(self, user_id: Optional[str] = None) -> int: count = cast(int, cursor.fetchone()[0]) return count - def delete_intermediates(self) -> list[tuple[str, str]]: - """Deletes all intermediate image records. + def get_intermediates(self) -> list[tuple[str, str]]: + """Gets all intermediate image records without deleting them. - Returns a list of (image_name, image_subfolder) tuples for file cleanup. + Returns a list of (image_name, image_subfolder) tuples for staged file deletion. """ with self._db.transaction() as cursor: - try: - cursor.execute( - """--sql - SELECT image_name, image_subfolder FROM images - WHERE is_intermediate = TRUE; - """ - ) - result = cast(list[sqlite3.Row], cursor.fetchall()) - image_name_subfolder_pairs = [(r[0], r[1]) for r in result] - cursor.execute( - """--sql - DELETE FROM images - WHERE is_intermediate = TRUE; - """ - ) - except sqlite3.Error as e: - raise ImageRecordDeleteException from e - return image_name_subfolder_pairs + cursor.execute( + """--sql + SELECT image_name, image_subfolder FROM images + WHERE is_intermediate = TRUE; + """ + ) + result = cast(list[sqlite3.Row], cursor.fetchall()) + return [(r[0], r[1]) for r in result] def save( self, diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index 9fded083cbc..7cbd939aaa3 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -276,18 +276,43 @@ def get_many( raise e def delete(self, image_name: str): + # Stage the file deletion first so a database failure can be rolled back by + # restoring the files, keeping the record and files consistent either way. + token: object | None = None + record_deleted = False try: record = self.__invoker.services.image_records.get(image_name) - self.__invoker.services.image_files.delete(image_name, image_subfolder=record.image_subfolder) + token = self.__invoker.services.image_files.stage_delete(image_name, image_subfolder=record.image_subfolder) self.__invoker.services.image_records.delete(image_name) + record_deleted = True + try: + self.__invoker.services.image_files.commit_delete(token) + except Exception as cleanup_error: + # The record is gone; a failed purge only leaves a staging directory + # behind, which startup recovery will clean up. Not a delete failure. + self.__invoker.services.logger.error(f"Failed to purge staged image files: {cleanup_error}") self._on_deleted(image_name) except ImageRecordDeleteException: + if token is not None: + try: + self.__invoker.services.image_files.rollback_delete(token) + except Exception as rollback_error: + self.__invoker.services.logger.error( + f"Failed to restore staged image files for {image_name}: {rollback_error}" + ) self.__invoker.services.logger.error("Failed to delete image record") raise except ImageFileDeleteException: self.__invoker.services.logger.error("Failed to delete image file") raise except Exception as e: + if token is not None and not record_deleted: + try: + self.__invoker.services.image_files.rollback_delete(token) + except Exception as rollback_error: + self.__invoker.services.logger.error( + f"Failed to restore staged image files for {image_name}: {rollback_error}" + ) self.__invoker.services.logger.error("Problem deleting image record and file") raise e @@ -347,13 +372,36 @@ def delete_images_on_board(self, board_id: str, user_id: Optional[str] = None) - raise e def delete_intermediates(self) -> int: + # All-or-nothing transaction: stage every file first, then delete the records in + # one operation, then purge the stages. Any staging or database failure rolls + # back every staged file so records always point at accessible files. try: - image_name_subfolder_pairs = self.__invoker.services.image_records.delete_intermediates() - count = len(image_name_subfolder_pairs) - for image_name, image_subfolder in image_name_subfolder_pairs: - self.__invoker.services.image_files.delete(image_name, image_subfolder=image_subfolder) + image_name_subfolder_pairs = self.__invoker.services.image_records.get_intermediates() + staged_deletes: list[tuple[str, object]] = [] + try: + for image_name, image_subfolder in image_name_subfolder_pairs: + token = self.__invoker.services.image_files.stage_delete( + image_name, image_subfolder=image_subfolder + ) + staged_deletes.append((image_name, token)) + self.__invoker.services.image_records.delete_many([name for name, _ in staged_deletes]) + except Exception: + for image_name, token in staged_deletes: + try: + self.__invoker.services.image_files.rollback_delete(token) + except Exception as rollback_error: + self.__invoker.services.logger.error( + f"Failed to restore staged image files for {image_name}: {rollback_error}" + ) + raise + for _, token in staged_deletes: + try: + self.__invoker.services.image_files.commit_delete(token) + except Exception as cleanup_error: + self.__invoker.services.logger.error(f"Failed to purge staged image files: {cleanup_error}") + for image_name, _ in staged_deletes: self._on_deleted(image_name) - return count + return len(staged_deletes) except ImageRecordDeleteException: self.__invoker.services.logger.error("Failed to delete image records") raise diff --git a/tests/app/routers/test_images.py b/tests/app/routers/test_images.py index 1e4270abff7..b92e33f79f2 100644 --- a/tests/app/routers/test_images.py +++ b/tests/app/routers/test_images.py @@ -220,3 +220,99 @@ def test_get_bulk_download_image_image_deleted_after_response( client.get("/api/v1/images/download/test.zip") assert not (tmp_path / "test.zip").exists() + + +# ── Transactional single-image deletion (DELETE /api/v1/images/i/{image_name}) ── + + +def prepare_delete_image_test(monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path): + """Wire the delete route to a real ImageService + real DiskImageFileStorage + real SQLite records.""" + from invokeai.app.services.image_files.image_files_disk import DiskImageFileStorage + + mock_deps = MockApiDependencies(mock_invoker) + monkeypatch.setattr("invokeai.app.api.routers.images.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers._access.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.routers.image_move_maintenance.ApiDependencies", mock_deps) + monkeypatch.setattr("invokeai.app.api.auth_dependencies.ApiDependencies", mock_deps) + + mock_invoker.services.urls = MagicMock() + mock_invoker.services.urls.get_image_url.return_value = "http://localhost/img.png" + + storage = DiskImageFileStorage(tmp_path / "outputs") + mock_invoker.services.image_files = storage + storage.start(mock_invoker) + mock_invoker.services.images.start(mock_invoker) + return storage + + +def _save_deletable_image(mock_invoker: Invoker, storage, image_name: str) -> None: + from PIL import Image + + from invokeai.app.services.image_records.image_records_common import ImageCategory, ResourceOrigin + + mock_invoker.services.image_records.save( + image_name=image_name, + image_origin=ResourceOrigin.INTERNAL, + image_category=ImageCategory.GENERAL, + width=64, + height=64, + has_workflow=False, + ) + storage.save(image=Image.new("RGB", (64, 64)), image_name=image_name) + + +def test_delete_image_success_deletes_files_and_record( + monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient +) -> None: + from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException + + storage = prepare_delete_image_test(monkeypatch, mock_invoker, tmp_path) + _save_deletable_image(mock_invoker, storage, "del.png") + + response = client.delete("/api/v1/images/i/del.png") + + assert response.status_code == 200 + json_response = response.json() + assert json_response["deleted_images"] == ["del.png"] + assert json_response["affected_boards"] == ["none"] + assert not storage.get_path("del.png").exists() + assert not storage.get_path("del.png", thumbnail=True).exists() + with pytest.raises(ImageRecordNotFoundException): + mock_invoker.services.image_records.get("del.png") + assert list(storage.image_root.glob(".delete_*")) == [] + + +def test_delete_image_not_found_returns_404( + monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient +) -> None: + prepare_delete_image_test(monkeypatch, mock_invoker, tmp_path) + + response = client.delete("/api/v1/images/i/does-not-exist.png") + + assert response.status_code == 404 + assert response.json()["detail"] == "Image not found" + + +def test_delete_image_db_failure_returns_500_and_restores_files( + monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient +) -> None: + from invokeai.app.services.image_records.image_records_common import ImageRecordDeleteException + + storage = prepare_delete_image_test(monkeypatch, mock_invoker, tmp_path) + _save_deletable_image(mock_invoker, storage, "del.png") + + def failing_delete(image_name: str) -> None: + raise ImageRecordDeleteException() + + monkeypatch.setattr(mock_invoker.services.image_records, "delete", failing_delete) + + response = client.delete("/api/v1/images/i/del.png") + + # The route must report the failure, not a success-shaped empty payload. + assert response.status_code == 500 + assert response.json()["detail"] == "Failed to delete image" + # The staged files must be rolled back: image and thumbnail restored, record intact. + assert storage.get_path("del.png").exists() + assert storage.get_path("del.png", thumbnail=True).exists() + assert mock_invoker.services.image_records.get("del.png").image_name == "del.png" + assert list(storage.image_root.glob(".delete_*")) == [] diff --git a/tests/app/routers/test_multiuser_authorization.py b/tests/app/routers/test_multiuser_authorization.py index be5d2a61beb..fd566af1e22 100644 --- a/tests/app/routers/test_multiuser_authorization.py +++ b/tests/app/routers/test_multiuser_authorization.py @@ -812,11 +812,18 @@ def test_non_owner_can_delete_image_from_public_board( _save_image(mock_invoker, "user1-public-delete", user1.user_id) mock_invoker.services.board_image_records.add_image_to_board(public_board_id, "user1-public-delete") + # The delete route no longer swallows service failures, so the test env needs + # working urls/image_files services for the deletion to actually succeed. + mock_invoker.services.urls = MagicMock() + mock_invoker.services.urls.get_image_url.return_value = "http://localhost/img.png" + mock_invoker.services.image_files = MagicMock() + r = client.delete( "/api/v1/images/i/user1-public-delete", headers=_auth(user2_token), ) assert r.status_code == status.HTTP_200_OK + assert r.json()["deleted_images"] == ["user1-public-delete"] def test_clear_intermediates_non_admin_forbidden(self, client: TestClient, user1_token: str): r = client.delete("/api/v1/images/intermediates", headers=_auth(user1_token)) diff --git a/tests/app/services/image_records/test_image_records_sqlite.py b/tests/app/services/image_records/test_image_records_sqlite.py index bd73c04fdb1..dfd9a41d22f 100644 --- a/tests/app/services/image_records/test_image_records_sqlite.py +++ b/tests/app/services/image_records/test_image_records_sqlite.py @@ -1,7 +1,7 @@ """DB-backed tests for SqliteImageRecordStorage. Verifies that image_subfolder round-trips correctly through save(), get(), -get_many(), and delete_intermediates() against a real (in-memory) SQLite database, +get_many(), and get_intermediates() against a real (in-memory) SQLite database, and that get_many()/get_image_names() enforce per-user ownership isolation. """ @@ -93,15 +93,15 @@ def test_get_many_returns_subfolders(self, store: SqliteImageRecordStorage) -> N assert by_name["hashed.png"] == "ab" -class TestDeleteIntermediatesSubfolder: - """delete_intermediates() returns (name, subfolder) pairs and removes rows.""" +class TestGetIntermediatesSubfolder: + """get_intermediates() returns (name, subfolder) pairs without deleting rows.""" def test_returns_subfolder_pairs(self, store: SqliteImageRecordStorage) -> None: _save(store, "keep.png", subfolder="general", is_intermediate=False) _save(store, "tmp1.png", subfolder="intermediate", is_intermediate=True) _save(store, "tmp2.png", subfolder="intermediate", is_intermediate=True) - pairs = store.delete_intermediates() + pairs = store.get_intermediates() # Should return only intermediate images with their subfolders assert len(pairs) == 2 @@ -113,9 +113,18 @@ def test_returns_subfolder_pairs(self, store: SqliteImageRecordStorage) -> None: record = store.get("keep.png") assert record.image_subfolder == "general" - def test_intermediates_are_deleted(self, store: SqliteImageRecordStorage) -> None: + def test_get_intermediates_does_not_delete(self, store: SqliteImageRecordStorage) -> None: _save(store, "tmp.png", subfolder="x", is_intermediate=True) - store.delete_intermediates() + store.get_intermediates() + + # Listing intermediates must not remove them. + record = store.get("tmp.png") + assert record.image_subfolder == "x" + + def test_intermediates_are_deleted_via_delete_many(self, store: SqliteImageRecordStorage) -> None: + _save(store, "tmp.png", subfolder="x", is_intermediate=True) + pairs = store.get_intermediates() + store.delete_many([name for name, _ in pairs]) from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index c97916dd139..52cdc3d8c7c 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -1,17 +1,22 @@ """Tests for ImageService (images_default.py). -Covers subfolder forwarding for all strategies and the delete_images_on_board -silent-failure contract (Points 2 & 3 from PR review). +Covers subfolder forwarding for all strategies, the delete_images_on_board +silent-failure contract (Points 2 & 3 from PR review), and the transactional +staged-deletion contracts of delete() and delete_intermediates(). """ +from pathlib import Path from unittest.mock import MagicMock import pytest from PIL import Image +from invokeai.app.services.image_files.image_files_common import ImageFileDeleteException +from invokeai.app.services.image_files.image_files_disk import DiskImageFileStorage from invokeai.app.services.image_records.image_records_common import ( ImageCategory, ImageRecord, + ImageRecordDeleteException, ResourceOrigin, ) from invokeai.app.services.images.images_default import ImageService @@ -182,12 +187,15 @@ def test_delete_forwards_subfolder(self, image_service: ImageService): image_service.delete("test.png") - invoker.services.image_files.delete.assert_called_once_with("test.png", image_subfolder="2026/04/05") + invoker.services.image_files.stage_delete.assert_called_once_with("test.png", image_subfolder="2026/04/05") invoker.services.image_records.delete.assert_called_once_with("test.png") + invoker.services.image_files.commit_delete.assert_called_once_with( + invoker.services.image_files.stage_delete.return_value + ) def test_delete_intermediates_forwards_subfolder(self, image_service: ImageService): invoker = image_service._ImageService__invoker # type: ignore - invoker.services.image_records.delete_intermediates.return_value = [ + invoker.services.image_records.get_intermediates.return_value = [ ("img1.png", "intermediate"), ("img2.png", "intermediate"), ] @@ -195,11 +203,12 @@ def test_delete_intermediates_forwards_subfolder(self, image_service: ImageServi count = image_service.delete_intermediates() assert count == 2 - calls = invoker.services.image_files.delete.call_args_list + calls = invoker.services.image_files.stage_delete.call_args_list assert calls[0].args == ("img1.png",) assert calls[0].kwargs == {"image_subfolder": "intermediate"} assert calls[1].args == ("img2.png",) assert calls[1].kwargs == {"image_subfolder": "intermediate"} + invoker.services.image_records.delete_many.assert_called_once_with(["img1.png", "img2.png"]) # ── Point 3: delete_images_on_board silent-failure contract ── @@ -276,3 +285,251 @@ def test_database_failure_restores_staged_files(self, image_service: ImageServic invoker.services.image_files.rollback_delete.assert_called_once_with(token) invoker.services.image_files.commit_delete.assert_not_called() + + +# ── Transactional staged deletion (single image and intermediates) ── + + +@pytest.fixture +def disk_image_service(tmp_path: Path) -> ImageService: + """ImageService wired to a real DiskImageFileStorage; all other services are mocks.""" + svc = ImageService() + invoker = MagicMock() + invoker.services.configuration.pil_compress_level = 1 + storage = DiskImageFileStorage(tmp_path / "outputs") + invoker.services.image_files = storage + storage.start(invoker) + svc.start(invoker) + return svc + + +def _save_image_file(storage: DiskImageFileStorage, image_name: str, image_subfolder: str = "") -> None: + storage.save(image=Image.new("RGB", (64, 64)), image_name=image_name, image_subfolder=image_subfolder) + + +def _staging_dirs(storage: DiskImageFileStorage) -> list[Path]: + return list(storage.image_root.glob(".delete_*")) + + +class TestDeleteTransactional: + """delete() must stage files, delete the record, then commit — never losing files on failure.""" + + def test_delete_success_removes_files_record_and_fires_callback_once(self, disk_image_service: ImageService): + invoker = disk_image_service._ImageService__invoker # type: ignore + storage = invoker.services.image_files + _save_image_file(storage, "img.png") + invoker.services.image_records.get.return_value = _make_record(image_name="img.png") + deleted_callbacks: list[str] = [] + disk_image_service.on_deleted(deleted_callbacks.append) + + disk_image_service.delete("img.png") + + assert not storage.get_path("img.png").exists() + assert not storage.get_path("img.png", thumbnail=True).exists() + invoker.services.image_records.delete.assert_called_once_with("img.png") + assert deleted_callbacks == ["img.png"] + assert _staging_dirs(storage) == [] + + def test_delete_staging_failure_keeps_record_and_raises(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_files.stage_delete.side_effect = ImageFileDeleteException("disk error") + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageFileDeleteException): + image_service.delete("test.png") + + invoker.services.image_records.delete.assert_not_called() + invoker.services.image_files.commit_delete.assert_not_called() + assert deleted_callbacks == [] + + def test_delete_db_failure_restores_files_and_raises(self, disk_image_service: ImageService): + invoker = disk_image_service._ImageService__invoker # type: ignore + storage = invoker.services.image_files + _save_image_file(storage, "img.png") + invoker.services.image_records.get.return_value = _make_record(image_name="img.png") + invoker.services.image_records.delete.side_effect = ImageRecordDeleteException() + deleted_callbacks: list[str] = [] + disk_image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageRecordDeleteException): + disk_image_service.delete("img.png") + + # The image and its thumbnail must be restored to their original paths. + assert storage.get_path("img.png").exists() + assert storage.get_path("img.png", thumbnail=True).exists() + assert deleted_callbacks == [] + assert _staging_dirs(storage) == [] + + def test_delete_rollback_failure_still_raises_db_error(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.delete.side_effect = ImageRecordDeleteException() + invoker.services.image_files.rollback_delete.side_effect = ImageFileDeleteException("rollback broken") + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageRecordDeleteException): + image_service.delete("test.png") + + invoker.services.image_files.rollback_delete.assert_called_once_with( + invoker.services.image_files.stage_delete.return_value + ) + invoker.services.image_files.commit_delete.assert_not_called() + assert deleted_callbacks == [] + + def test_delete_commit_failure_is_logged_not_raised(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_files.commit_delete.side_effect = ImageFileDeleteException("purge failed") + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + image_service.delete("test.png") + + invoker.services.image_records.delete.assert_called_once_with("test.png") + invoker.services.image_files.rollback_delete.assert_not_called() + assert deleted_callbacks == ["test.png"] + invoker.services.logger.error.assert_called() + + +class TestDeleteIntermediatesTransactional: + """delete_intermediates() must be all-or-nothing: stage everything, delete records once, commit.""" + + def test_success_deletes_multiple_intermediates(self, disk_image_service: ImageService): + invoker = disk_image_service._ImageService__invoker # type: ignore + storage = invoker.services.image_files + names = ["tmp1.png", "tmp2.png", "tmp3.png"] + for name in names: + _save_image_file(storage, name) + invoker.services.image_records.get_intermediates.return_value = [(name, "") for name in names] + deleted_callbacks: list[str] = [] + disk_image_service.on_deleted(deleted_callbacks.append) + + count = disk_image_service.delete_intermediates() + + assert count == 3 + for name in names: + assert not storage.get_path(name).exists() + assert not storage.get_path(name, thumbnail=True).exists() + invoker.services.image_records.delete_many.assert_called_once_with(names) + assert deleted_callbacks == names + assert _staging_dirs(storage) == [] + + def test_first_staging_failure_aborts_without_db_delete(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [("tmp1.png", ""), ("tmp2.png", "")] + invoker.services.image_files.stage_delete.side_effect = ImageFileDeleteException("disk error") + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageFileDeleteException): + image_service.delete_intermediates() + + invoker.services.image_records.delete_many.assert_not_called() + # Nothing was staged, so nothing needs rolling back. + invoker.services.image_files.rollback_delete.assert_not_called() + invoker.services.image_files.commit_delete.assert_not_called() + assert deleted_callbacks == [] + + def test_later_staging_failure_rolls_back_earlier_stages(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [("tmp1.png", ""), ("tmp2.png", "")] + token1 = object() + invoker.services.image_files.stage_delete.side_effect = [token1, ImageFileDeleteException("disk error")] + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageFileDeleteException): + image_service.delete_intermediates() + + invoker.services.image_records.delete_many.assert_not_called() + invoker.services.image_files.rollback_delete.assert_called_once_with(token1) + invoker.services.image_files.commit_delete.assert_not_called() + assert deleted_callbacks == [] + + def test_later_staging_failure_restores_earlier_files_on_disk(self, disk_image_service: ImageService): + invoker = disk_image_service._ImageService__invoker # type: ignore + storage = invoker.services.image_files + _save_image_file(storage, "tmp1.png") + # The second entry's subfolder fails path validation, so staging it raises after + # tmp1.png has already been staged. + invoker.services.image_records.get_intermediates.return_value = [ + ("tmp1.png", ""), + ("tmp2.png", "bad\\path"), + ] + deleted_callbacks: list[str] = [] + disk_image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ValueError): + disk_image_service.delete_intermediates() + + assert storage.get_path("tmp1.png").exists() + assert storage.get_path("tmp1.png", thumbnail=True).exists() + invoker.services.image_records.delete_many.assert_not_called() + assert deleted_callbacks == [] + assert _staging_dirs(storage) == [] + + def test_db_failure_restores_all_staged_files(self, disk_image_service: ImageService): + invoker = disk_image_service._ImageService__invoker # type: ignore + storage = invoker.services.image_files + names = ["tmp1.png", "tmp2.png"] + for name in names: + _save_image_file(storage, name) + invoker.services.image_records.get_intermediates.return_value = [(name, "") for name in names] + invoker.services.image_records.delete_many.side_effect = ImageRecordDeleteException() + deleted_callbacks: list[str] = [] + disk_image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageRecordDeleteException): + disk_image_service.delete_intermediates() + + for name in names: + assert storage.get_path(name).exists() + assert storage.get_path(name, thumbnail=True).exists() + assert deleted_callbacks == [] + assert _staging_dirs(storage) == [] + + def test_one_rollback_failure_does_not_abandon_other_rollbacks(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [ + ("tmp1.png", ""), + ("tmp2.png", ""), + ("tmp3.png", ""), + ] + tokens = [object(), object(), object()] + invoker.services.image_files.stage_delete.side_effect = tokens + invoker.services.image_records.delete_many.side_effect = ImageRecordDeleteException() + invoker.services.image_files.rollback_delete.side_effect = [ + ImageFileDeleteException("rollback broken"), + None, + None, + ] + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageRecordDeleteException): + image_service.delete_intermediates() + + # Every staged item must have a rollback attempt, even after one fails. + rollback_tokens = [call.args[0] for call in invoker.services.image_files.rollback_delete.call_args_list] + assert rollback_tokens == tokens + invoker.services.image_files.commit_delete.assert_not_called() + assert deleted_callbacks == [] + + def test_commit_failure_is_logged_and_remaining_commits_attempted(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [("tmp1.png", ""), ("tmp2.png", "")] + tokens = [object(), object()] + invoker.services.image_files.stage_delete.side_effect = tokens + invoker.services.image_files.commit_delete.side_effect = [ImageFileDeleteException("purge failed"), None] + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + count = image_service.delete_intermediates() + + assert count == 2 + commit_tokens = [call.args[0] for call in invoker.services.image_files.commit_delete.call_args_list] + assert commit_tokens == tokens + # Records were deleted, so the deletions are committed and callbacks must fire. + assert deleted_callbacks == ["tmp1.png", "tmp2.png"] + invoker.services.logger.error.assert_called() + invoker.services.image_files.rollback_delete.assert_not_called() From 82cb7b37a76988411d2dba0859375852ac145f98 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sun, 9 Aug 2026 18:55:43 -0400 Subject: [PATCH 2/2] fix(images): address review of intermediate cleanup and delete route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JPPhoto's review raised two merge blockers. Intermediate cleanup snapshotted the intermediates, then deleted those names unconditionally after the database window. An image promoted out of intermediate status in between lost both its record and its staged files. Deletion now runs through `delete_intermediates_by_names()`, which carries the `is_intermediate` predicate on the DELETE itself rather than on a preceding SELECT — Python's legacy sqlite3 transaction control opens a transaction only before a write, so a SELECT there holds no read lock to rely on. The method reports `(deleted, retained)` so the service can tell a promoted record from one that is simply gone: only a record still present earns a file restore. Restoring files for a record deleted elsewhere would strand them with no row and no staging dir for startup recovery, so the rollback path re-checks existence and errs towards keeping the files when the database can't answer. The name lists are chunked to stay under SQLITE_MAX_VARIABLE_NUMBER, which the previous `delete_many(all_intermediates)` call could exceed on a large library. The delete route turned every `get_dto()` failure into a 404, so a database fault on a live image told the frontend to drop it. It now returns 404 only for `ImageRecordNotFoundException` and 500 otherwise. That split could not work on its own: the record store converted every `sqlite3.Error` from `get()` and `get_metadata()` into `ImageRecordNotFoundException`, so a fault on the primary lookup still read as "missing". Those two methods now raise not-found only when the row is genuinely absent. This also stops `__recover_staged_deletes` from purging a live image's staged files on a transient database fault. Tests cover the promotion race at both the store and the service level (including a promotion interleaved inside the call, and a record deleted between the database window and the rollback), chunk boundaries, and that a database fault reaches the route as 500 rather than 404. Co-Authored-By: Claude Opus 5 (1M context) --- invokeai/app/api/routers/images.py | 7 +- .../image_records/image_records_base.py | 10 + .../image_records/image_records_sqlite.py | 85 ++++-- .../app/services/images/images_default.py | 54 +++- tests/app/routers/test_images.py | 52 ++++ .../test_image_records_sqlite.py | 158 +++++++++- .../services/images/test_images_default.py | 277 +++++++++++++++++- 7 files changed, 601 insertions(+), 42 deletions(-) diff --git a/invokeai/app/api/routers/images.py b/invokeai/app/api/routers/images.py index bc51544080a..5b3db684c47 100644 --- a/invokeai/app/api/routers/images.py +++ b/invokeai/app/api/routers/images.py @@ -27,6 +27,7 @@ ImageCategory, ImageNamesResult, ImageRecordChanges, + ImageRecordNotFoundException, ResourceOrigin, ) from invokeai.app.services.images.images_common import ( @@ -213,8 +214,12 @@ async def delete_image( # success and dropped the item from its cache even though the record was still live. try: image_dto = ApiDependencies.invoker.services.images.get_dto(image_name) - except Exception: + except ImageRecordNotFoundException: raise HTTPException(status_code=404, detail="Image not found") + except Exception: + # A record/URL/board lookup failure for an image that does exist is a server fault, not a + # missing image — reporting it as 404 would tell the frontend to drop a live item. + raise HTTPException(status_code=500, detail="Failed to delete image") board_id = image_dto.board_id or "none" try: diff --git a/invokeai/app/services/image_records/image_records_base.py b/invokeai/app/services/image_records/image_records_base.py index e72475fe1f5..41c1450fa62 100644 --- a/invokeai/app/services/image_records/image_records_base.py +++ b/invokeai/app/services/image_records/image_records_base.py @@ -74,6 +74,16 @@ def get_intermediates(self) -> list[tuple[str, str]]: """Gets all intermediate image records as (image_name, image_subfolder) tuples, without deleting them.""" pass + @abstractmethod + def delete_intermediates_by_names(self, image_names: list[str]) -> tuple[list[str], list[str]]: + """Deletes the named image records, skipping any that are no longer intermediates. + + Returns ``(deleted, retained)``: the names whose records were removed, and the names whose + records are still present because they are no longer intermediates. Names whose records were + already gone appear in neither list, so a caller holding their files must not restore them. + """ + pass + @abstractmethod def get_intermediates_count(self, user_id: Optional[str] = None) -> int: """Gets a count of intermediate images. If user_id is provided, only counts that user's intermediates.""" diff --git a/invokeai/app/services/image_records/image_records_sqlite.py b/invokeai/app/services/image_records/image_records_sqlite.py index b92fede9a47..a8d08362755 100644 --- a/invokeai/app/services/image_records/image_records_sqlite.py +++ b/invokeai/app/services/image_records/image_records_sqlite.py @@ -23,24 +23,27 @@ class SqliteImageRecordStorage(ImageRecordStorageBase): + # Conservative bound on bound parameters per statement. SQLITE_MAX_VARIABLE_NUMBER defaults to + # 999 on SQLite builds older than 3.32, and an image library can hold far more intermediates. + _MAX_SQL_VARIABLES = 500 + def __init__(self, db: SqliteDatabase) -> None: super().__init__() self._db = db def get(self, image_name: str) -> ImageRecord: + # A query failure means the database is unavailable, not that the image is missing. Reporting + # it as "not found" makes callers (and the routes above them) delete live images from view. with self._db.transaction() as cursor: - try: - cursor.execute( - f"""--sql - SELECT {IMAGE_DTO_COLS} FROM images - WHERE image_name = ?; - """, - (image_name,), - ) + cursor.execute( + f"""--sql + SELECT {IMAGE_DTO_COLS} FROM images + WHERE image_name = ?; + """, + (image_name,), + ) - result = cast(Optional[sqlite3.Row], cursor.fetchone()) - except sqlite3.Error as e: - raise ImageRecordNotFoundException from e + result = cast(Optional[sqlite3.Row], cursor.fetchone()) if not result: raise ImageRecordNotFoundException @@ -62,20 +65,17 @@ def get_user_id(self, image_name: str) -> Optional[str]: return cast(Optional[str], dict(result).get("user_id")) def get_metadata(self, image_name: str) -> Optional[MetadataField]: + # As in get(): a query failure is a database fault, not a missing record. with self._db.transaction() as cursor: - try: - cursor.execute( - """--sql - SELECT metadata FROM images - WHERE image_name = ?; - """, - (image_name,), - ) - - result = cast(Optional[sqlite3.Row], cursor.fetchone()) + cursor.execute( + """--sql + SELECT metadata FROM images + WHERE image_name = ?; + """, + (image_name,), + ) - except sqlite3.Error as e: - raise ImageRecordNotFoundException from e + result = cast(Optional[sqlite3.Row], cursor.fetchone()) if not result: raise ImageRecordNotFoundException @@ -317,6 +317,45 @@ def get_intermediates(self) -> list[tuple[str, str]]: result = cast(list[sqlite3.Row], cursor.fetchall()) return [(r[0], r[1]) for r in result] + def delete_intermediates_by_names(self, image_names: list[str]) -> tuple[list[str], list[str]]: + """Deletes the named image records, skipping any that are no longer intermediates. + + The ``is_intermediate`` predicate rides on the DELETE itself rather than on a preceding + SELECT, so an image promoted out of intermediate status keeps its record however the + promotion interleaves with this call. (Python's legacy sqlite3 transaction control opens a + transaction only before a write, so a SELECT here holds no read lock to rely on.) + + Returns ``(deleted, retained)``: the names whose records this call removed, and the names + whose records are still present because they are no longer intermediates. Names whose + records were already gone appear in neither list — the caller must not restore their files. + """ + deleted: list[str] = [] + retained: list[str] = [] + try: + with self._db.transaction() as cursor: + # Chunked to stay under SQLITE_MAX_VARIABLE_NUMBER; every chunk runs inside the one + # transaction above. + for start in range(0, len(image_names), self._MAX_SQL_VARIABLES): + chunk = image_names[start : start + self._MAX_SQL_VARIABLES] + placeholders = ",".join("?" for _ in chunk) + select_query = f"SELECT image_name FROM images WHERE image_name IN ({placeholders})" + + cursor.execute(select_query, chunk) + present_before = {cast(str, r[0]) for r in cursor.fetchall()} + cursor.execute( + f"DELETE FROM images WHERE image_name IN ({placeholders}) AND is_intermediate = TRUE", + chunk, + ) + cursor.execute(select_query, chunk) + present_after = {cast(str, r[0]) for r in cursor.fetchall()} + + deleted.extend(name for name in chunk if name in present_before and name not in present_after) + retained.extend(name for name in chunk if name in present_after) + except sqlite3.Error as e: + # The try wraps the context manager so a failure in its commit is reported too. + raise ImageRecordDeleteException from e + return deleted, retained + def save( self, image_name: str, diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index 7cbd939aaa3..1856d225cf6 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -371,10 +371,27 @@ def delete_images_on_board(self, board_id: str, user_id: Optional[str] = None) - self.__invoker.services.logger.error(f"Problem deleting image records and files: {str(e)}") raise e + def _record_still_exists(self, image_name: str) -> bool: + """Whether an image record is still present, erring towards "yes". + + Used to decide whether staged files must be restored. A restore is only correct while the + record is live; if we cannot tell, restoring is the safer error, because leftover files can + be cleaned up later but files purged against a live record are gone. + """ + try: + self.__invoker.services.image_records.get(image_name) + return True + except ImageRecordNotFoundException: + return False + except Exception as e: + self.__invoker.services.logger.error(f"Could not confirm whether {image_name} still exists: {e}") + return True + def delete_intermediates(self) -> int: - # All-or-nothing transaction: stage every file first, then delete the records in - # one operation, then purge the stages. Any staging or database failure rolls - # back every staged file so records always point at accessible files. + # All-or-nothing transaction: stage every file first, then delete the records in one + # operation, then purge the stages. Any staging or database failure rolls back every staged + # file, so a live record's files are never destroyed — though a failed rollback leaves them + # in the staging dir until startup recovery restores them. try: image_name_subfolder_pairs = self.__invoker.services.image_records.get_intermediates() staged_deletes: list[tuple[str, object]] = [] @@ -384,7 +401,14 @@ def delete_intermediates(self) -> int: image_name, image_subfolder=image_subfolder ) staged_deletes.append((image_name, token)) - self.__invoker.services.image_records.delete_many([name for name, _ in staged_deletes]) + # Deletion is conditional on the row still being an intermediate. An image can be + # promoted out of intermediate status between the snapshot above and this call, and + # such an image must keep both its record and its files. + deleted, retained = self.__invoker.services.image_records.delete_intermediates_by_names( + [name for name, _ in staged_deletes] + ) + deleted_names = set(deleted) + retained_names = set(retained) except Exception: for image_name, token in staged_deletes: try: @@ -394,14 +418,30 @@ def delete_intermediates(self) -> int: f"Failed to restore staged image files for {image_name}: {rollback_error}" ) raise - for _, token in staged_deletes: + deleted_image_names: list[str] = [] + for image_name, token in staged_deletes: + # Only a record that is still there earns a restore. A name in neither list had its + # record removed by someone else while we held its files, and a retained record can + # still be deleted while this loop works through the other names — restoring either + # would strand the files on disk with no record and no staging dir to recover from. + # Re-checking here shrinks that window from the whole loop to a single lookup. + if image_name in retained_names and self._record_still_exists(image_name): + try: + self.__invoker.services.image_files.rollback_delete(token) + except Exception as rollback_error: + self.__invoker.services.logger.error( + f"Failed to restore staged image files for {image_name}: {rollback_error}" + ) + continue try: self.__invoker.services.image_files.commit_delete(token) except Exception as cleanup_error: self.__invoker.services.logger.error(f"Failed to purge staged image files: {cleanup_error}") - for image_name, _ in staged_deletes: + if image_name in deleted_names: + deleted_image_names.append(image_name) + for image_name in deleted_image_names: self._on_deleted(image_name) - return len(staged_deletes) + return len(deleted_image_names) except ImageRecordDeleteException: self.__invoker.services.logger.error("Failed to delete image records") raise diff --git a/tests/app/routers/test_images.py b/tests/app/routers/test_images.py index b92e33f79f2..5d90d654b99 100644 --- a/tests/app/routers/test_images.py +++ b/tests/app/routers/test_images.py @@ -293,6 +293,58 @@ def test_delete_image_not_found_returns_404( assert response.json()["detail"] == "Image not found" +def test_delete_image_lookup_failure_returns_500_not_404( + monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient +) -> None: + """A DTO lookup that fails for a reason other than a missing record is a 500, not a 404. + + Reporting it as 404 would tell the frontend the image is gone and drop a live item from its cache. + """ + storage = prepare_delete_image_test(monkeypatch, mock_invoker, tmp_path) + _save_deletable_image(mock_invoker, storage, "del.png") + + def failing_get_dto(image_name: str): + raise RuntimeError("database unavailable") + + monkeypatch.setattr(mock_invoker.services.images, "get_dto", failing_get_dto) + + response = client.delete("/api/v1/images/i/del.png") + + assert response.status_code == 500 + assert response.json()["detail"] == "Failed to delete image" + # Nothing was touched: the record and its files are intact. + assert storage.get_path("del.png").exists() + assert mock_invoker.services.image_records.get("del.png").image_name == "del.png" + + +def test_delete_image_db_fault_during_lookup_returns_500_not_404( + monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient +) -> None: + """A database fault while reading the record is a 500, driven through the real record store. + + The store used to convert every ``sqlite3.Error`` into ``ImageRecordNotFoundException``, which + made a database fault indistinguishable from a missing image and produced a 404 for a live one. + This drives the real store rather than stubbing it, so the store's translation is what is under + test — stubbing ``get`` would bypass the very code that used to be wrong. + """ + storage = prepare_delete_image_test(monkeypatch, mock_invoker, tmp_path) + _save_deletable_image(mock_invoker, storage, "del.png") + + # Break the table out from under the query. Any sqlite3.Error would do; this one is deterministic. + records = mock_invoker.services.image_records + records._db._conn.execute("ALTER TABLE images RENAME TO images_moved;") + try: + response = client.delete("/api/v1/images/i/del.png") + finally: + records._db._conn.execute("ALTER TABLE images_moved RENAME TO images;") + + assert response.status_code == 500 + assert response.json()["detail"] == "Failed to delete image" + # The image is still there once the database recovers. + assert records.get("del.png").image_name == "del.png" + assert storage.get_path("del.png").exists() + + def test_delete_image_db_failure_returns_500_and_restores_files( monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient ) -> None: diff --git a/tests/app/services/image_records/test_image_records_sqlite.py b/tests/app/services/image_records/test_image_records_sqlite.py index 9bf0feb7571..f498a14aea9 100644 --- a/tests/app/services/image_records/test_image_records_sqlite.py +++ b/tests/app/services/image_records/test_image_records_sqlite.py @@ -5,12 +5,19 @@ and that get_many()/get_image_names() enforce per-user ownership isolation. """ +import sqlite3 + import pytest from invokeai.app.services.board_image_records.board_image_records_sqlite import SqliteBoardImageRecordStorage from invokeai.app.services.board_records.board_records_sqlite import SqliteBoardRecordStorage from invokeai.app.services.config.config_default import InvokeAIAppConfig -from invokeai.app.services.image_records.image_records_common import ImageCategory, ResourceOrigin +from invokeai.app.services.image_records.image_records_common import ( + ImageCategory, + ImageRecordChanges, + ImageRecordNotFoundException, + ResourceOrigin, +) from invokeai.app.services.image_records.image_records_sqlite import SqliteImageRecordStorage from invokeai.app.services.shared.sqlite.sqlite_common import SQLiteDirection from invokeai.backend.util.logging import InvokeAILogger @@ -134,16 +141,159 @@ def test_get_intermediates_does_not_delete(self, store: SqliteImageRecordStorage record = store.get("tmp.png") assert record.image_subfolder == "x" - def test_intermediates_are_deleted_via_delete_many(self, store: SqliteImageRecordStorage) -> None: + def test_intermediates_are_deleted_via_delete_intermediates_by_names(self, store: SqliteImageRecordStorage) -> None: _save(store, "tmp.png", subfolder="x", is_intermediate=True) pairs = store.get_intermediates() - store.delete_many([name for name, _ in pairs]) + deleted, retained = store.delete_intermediates_by_names([name for name, _ in pairs]) + + assert deleted == ["tmp.png"] + assert retained == [] + with pytest.raises(ImageRecordNotFoundException): + store.get("tmp.png") + + +class TestQueryFaultsAreNotNotFound: + """A failing query means the database is unavailable, not that the image is missing. + + Reporting a query fault as "not found" propagates all the way to the API, where it becomes a 404 + and tells the frontend to drop a live image from its cache. + """ + + def _break_the_images_table(self, store: SqliteImageRecordStorage) -> None: + store._db._conn.execute("ALTER TABLE images RENAME TO images_moved;") + + def test_get_raises_the_db_error_not_not_found(self, store: SqliteImageRecordStorage) -> None: + _save(store, "live.png") + self._break_the_images_table(store) + + with pytest.raises(sqlite3.Error): + store.get("live.png") - from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException + def test_get_metadata_raises_the_db_error_not_not_found(self, store: SqliteImageRecordStorage) -> None: + _save(store, "live.png") + self._break_the_images_table(store) + with pytest.raises(sqlite3.Error): + store.get_metadata("live.png") + + def test_missing_row_still_raises_not_found(self, store: SqliteImageRecordStorage) -> None: + """The genuine not-found path is untouched.""" + with pytest.raises(ImageRecordNotFoundException): + store.get("never-existed.png") + with pytest.raises(ImageRecordNotFoundException): + store.get_metadata("never-existed.png") + + +class TestDeleteIntermediatesByNames: + """delete_intermediates_by_names() deletes only rows that are still intermediates.""" + + def test_promoted_image_keeps_its_record(self, store: SqliteImageRecordStorage) -> None: + """An image promoted out of intermediate status after the snapshot must survive.""" + _save(store, "tmp.png", subfolder="x", is_intermediate=True) + _save(store, "promoted.png", subfolder="x", is_intermediate=True) + snapshot = [name for name, _ in store.get_intermediates()] + assert set(snapshot) == {"tmp.png", "promoted.png"} + + # Simulate the race: the image stops being an intermediate between the snapshot and delete. + store.update("promoted.png", ImageRecordChanges(is_intermediate=False)) + + deleted, retained = store.delete_intermediates_by_names(snapshot) + + assert deleted == ["tmp.png"] + assert retained == ["promoted.png"] + assert store.get("promoted.png").is_intermediate is False with pytest.raises(ImageRecordNotFoundException): store.get("tmp.png") + def test_promotion_interleaved_inside_the_call_keeps_the_record(self, store: SqliteImageRecordStorage) -> None: + """The is_intermediate predicate must ride on the DELETE, not on a preceding SELECT. + + Python's legacy sqlite3 transaction control opens a transaction only before a write, so a + SELECT inside this method holds no read lock. A writer that promotes an image after that + SELECT but before the DELETE must still not lose its record. + """ + _save(store, "tmp.png", is_intermediate=True) + _save(store, "promoted.png", is_intermediate=True) + snapshot = [name for name, _ in store.get_intermediates()] + + # Promote from inside the call, between the first SELECT and the DELETE. + real_execute = store._db._conn.execute + promoted = False + + def trace(statement: str) -> None: + nonlocal promoted + # The trace fires when a statement *begins*, so hooking the first SELECT would promote + # before that SELECT reads anything — indistinguishable from promoting up front. Hooking + # the DELETE puts the promotion after the SELECT has already seen the row as an + # intermediate, which is the interleaving that a SELECT-then-unconditional-DELETE + # implementation gets wrong. + if not promoted and statement.strip().upper().startswith("DELETE FROM IMAGES"): + promoted = True + real_execute("UPDATE images SET is_intermediate = 0 WHERE image_name = 'promoted.png'") + + store._db._conn.set_trace_callback(trace) + try: + deleted, retained = store.delete_intermediates_by_names(snapshot) + finally: + store._db._conn.set_trace_callback(None) + + assert promoted, "the interleaved promotion never ran; the test proves nothing" + assert deleted == ["tmp.png"] + assert retained == ["promoted.png"] + assert store.get("promoted.png").is_intermediate is False + + def test_unknown_and_empty_names_are_no_ops(self, store: SqliteImageRecordStorage) -> None: + _save(store, "keep.png", is_intermediate=False) + + assert store.delete_intermediates_by_names([]) == ([], []) + # "gone.png" has no record at all, so it is neither deleted nor retained; "keep.png" exists + # but is not an intermediate, so it is retained. + assert store.delete_intermediates_by_names(["gone.png", "keep.png"]) == ([], ["keep.png"]) + assert store.get("keep.png").image_name == "keep.png" + + def test_more_names_than_sql_variable_limit(self, store: SqliteImageRecordStorage) -> None: + """Chunking must not lose rows: exercise a name list spanning several chunks.""" + chunk = SqliteImageRecordStorage._MAX_SQL_VARIABLES + names = [f"tmp{i:05d}.png" for i in range(chunk * 2 + 7)] + for name in names: + _save(store, name, is_intermediate=True) + # One image in the middle chunk is promoted and must survive. + survivor = names[chunk + 3] + store.update(survivor, ImageRecordChanges(is_intermediate=False)) + + deleted, retained = store.delete_intermediates_by_names(names) + + assert set(deleted) == set(names) - {survivor} + assert retained == [survivor] + assert store.get(survivor).is_intermediate is False + assert store.get_intermediates() == [] + + def test_chunking_stays_within_the_declared_variable_limit(self, store: SqliteImageRecordStorage) -> None: + """No statement may bind more parameters than the declared limit.""" + chunk = SqliteImageRecordStorage._MAX_SQL_VARIABLES + names = [f"tmp{i:05d}.png" for i in range(chunk * 2 + 7)] + for name in names: + _save(store, name, is_intermediate=True) + + # The trace callback reports statements with their parameters already expanded, so count the + # bound image names in each one rather than the placeholders. + widest = 0 + + def trace(statement: str) -> None: + nonlocal widest + if "images WHERE image_name IN (" in statement: + widest = max(widest, statement.count(".png")) + + store._db._conn.set_trace_callback(trace) + try: + store.delete_intermediates_by_names(names) + finally: + store._db._conn.set_trace_callback(None) + + # 999 is the SQLITE_MAX_VARIABLE_NUMBER default on builds older than 3.32. Asserting the + # literal rather than _MAX_SQL_VARIABLES keeps the test meaningful if that constant is raised. + assert 0 < widest <= 999 + class TestOwnershipFilteringOmittedBoard: """get_many()/get_image_names() enforce per-user isolation when board_id is omitted. diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index 52cdc3d8c7c..61bfb0c94aa 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -5,22 +5,28 @@ staged-deletion contracts of delete() and delete_intermediates(). """ +import sqlite3 from pathlib import Path from unittest.mock import MagicMock import pytest from PIL import Image +from invokeai.app.services.config.config_default import InvokeAIAppConfig from invokeai.app.services.image_files.image_files_common import ImageFileDeleteException from invokeai.app.services.image_files.image_files_disk import DiskImageFileStorage from invokeai.app.services.image_records.image_records_common import ( ImageCategory, ImageRecord, + ImageRecordChanges, ImageRecordDeleteException, ResourceOrigin, ) +from invokeai.app.services.image_records.image_records_sqlite import SqliteImageRecordStorage from invokeai.app.services.images.images_default import ImageService from invokeai.app.util.misc import get_iso_timestamp +from invokeai.backend.util.logging import InvokeAILogger +from tests.fixtures.sqlite_database import create_mock_sqlite_database @pytest.fixture @@ -34,6 +40,8 @@ def image_service() -> ImageService: invoker.services.board_image_records.get_board_for_image.return_value = None invoker.services.urls.get_image_url.return_value = "http://localhost/img.png" invoker.services.configuration.image_subfolder_strategy = "flat" + # By default every staged intermediate is still an intermediate when the delete runs. + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: (list(names), []) svc.start(invoker) return svc @@ -208,7 +216,7 @@ def test_delete_intermediates_forwards_subfolder(self, image_service: ImageServi assert calls[0].kwargs == {"image_subfolder": "intermediate"} assert calls[1].args == ("img2.png",) assert calls[1].kwargs == {"image_subfolder": "intermediate"} - invoker.services.image_records.delete_many.assert_called_once_with(["img1.png", "img2.png"]) + invoker.services.image_records.delete_intermediates_by_names.assert_called_once_with(["img1.png", "img2.png"]) # ── Point 3: delete_images_on_board silent-failure contract ── @@ -296,6 +304,8 @@ def disk_image_service(tmp_path: Path) -> ImageService: svc = ImageService() invoker = MagicMock() invoker.services.configuration.pil_compress_level = 1 + # By default every staged intermediate is still an intermediate when the delete runs. + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: (list(names), []) storage = DiskImageFileStorage(tmp_path / "outputs") invoker.services.image_files = storage storage.start(invoker) @@ -410,10 +420,99 @@ def test_success_deletes_multiple_intermediates(self, disk_image_service: ImageS for name in names: assert not storage.get_path(name).exists() assert not storage.get_path(name, thumbnail=True).exists() - invoker.services.image_records.delete_many.assert_called_once_with(names) + invoker.services.image_records.delete_intermediates_by_names.assert_called_once_with(names) assert deleted_callbacks == names assert _staging_dirs(storage) == [] + def test_image_promoted_out_of_intermediate_keeps_record_and_files(self, disk_image_service: ImageService): + """An image that stops being an intermediate mid-operation keeps its record and its files.""" + invoker = disk_image_service._ImageService__invoker # type: ignore + storage = invoker.services.image_files + for name in ("tmp1.png", "promoted.png", "tmp2.png"): + _save_image_file(storage, name) + invoker.services.image_records.get_intermediates.return_value = [ + ("tmp1.png", ""), + ("promoted.png", ""), + ("tmp2.png", ""), + ] + # The database reports that promoted.png was no longer an intermediate, so its record stands. + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: ( + [name for name in names if name != "promoted.png"], + ["promoted.png"], + ) + deleted_callbacks: list[str] = [] + disk_image_service.on_deleted(deleted_callbacks.append) + + count = disk_image_service.delete_intermediates() + + assert count == 2 + assert storage.get_path("promoted.png").exists() + assert storage.get_path("promoted.png", thumbnail=True).exists() + for name in ("tmp1.png", "tmp2.png"): + assert not storage.get_path(name).exists() + assert not storage.get_path(name, thumbnail=True).exists() + assert deleted_callbacks == ["tmp1.png", "tmp2.png"] + assert _staging_dirs(storage) == [] + + def test_promoted_image_is_rolled_back_not_committed(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [("tmp1.png", ""), ("promoted.png", "")] + tokens = [object(), object()] + invoker.services.image_files.stage_delete.side_effect = tokens + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: ( + ["tmp1.png"], + ["promoted.png"], + ) + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + count = image_service.delete_intermediates() + + assert count == 1 + invoker.services.image_files.commit_delete.assert_called_once_with(tokens[0]) + invoker.services.image_files.rollback_delete.assert_called_once_with(tokens[1]) + assert deleted_callbacks == ["tmp1.png"] + + def test_rollback_failure_for_promoted_image_does_not_abort_the_rest(self, image_service: ImageService): + """A failed restore is logged; the surviving deletions still commit and fire callbacks.""" + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [ + ("promoted.png", ""), + ("tmp1.png", ""), + ] + tokens = [object(), object()] + invoker.services.image_files.stage_delete.side_effect = tokens + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: ( + ["tmp1.png"], + ["promoted.png"], + ) + invoker.services.image_files.rollback_delete.side_effect = ImageFileDeleteException("rollback broken") + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + count = image_service.delete_intermediates() + + assert count == 1 + invoker.services.image_files.rollback_delete.assert_called_once_with(tokens[0]) + invoker.services.image_files.commit_delete.assert_called_once_with(tokens[1]) + assert deleted_callbacks == ["tmp1.png"] + invoker.services.logger.error.assert_called() + + def test_nothing_deleted_returns_zero_and_fires_no_callbacks(self, image_service: ImageService): + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_records.get_intermediates.return_value = [("promoted.png", "")] + token = object() + invoker.services.image_files.stage_delete.return_value = token + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: ([], ["promoted.png"]) + deleted_callbacks: list[str] = [] + image_service.on_deleted(deleted_callbacks.append) + + assert image_service.delete_intermediates() == 0 + + invoker.services.image_files.rollback_delete.assert_called_once_with(token) + invoker.services.image_files.commit_delete.assert_not_called() + assert deleted_callbacks == [] + def test_first_staging_failure_aborts_without_db_delete(self, image_service: ImageService): invoker = image_service._ImageService__invoker # type: ignore invoker.services.image_records.get_intermediates.return_value = [("tmp1.png", ""), ("tmp2.png", "")] @@ -424,7 +523,7 @@ def test_first_staging_failure_aborts_without_db_delete(self, image_service: Ima with pytest.raises(ImageFileDeleteException): image_service.delete_intermediates() - invoker.services.image_records.delete_many.assert_not_called() + invoker.services.image_records.delete_intermediates_by_names.assert_not_called() # Nothing was staged, so nothing needs rolling back. invoker.services.image_files.rollback_delete.assert_not_called() invoker.services.image_files.commit_delete.assert_not_called() @@ -441,7 +540,7 @@ def test_later_staging_failure_rolls_back_earlier_stages(self, image_service: Im with pytest.raises(ImageFileDeleteException): image_service.delete_intermediates() - invoker.services.image_records.delete_many.assert_not_called() + invoker.services.image_records.delete_intermediates_by_names.assert_not_called() invoker.services.image_files.rollback_delete.assert_called_once_with(token1) invoker.services.image_files.commit_delete.assert_not_called() assert deleted_callbacks == [] @@ -464,7 +563,7 @@ def test_later_staging_failure_restores_earlier_files_on_disk(self, disk_image_s assert storage.get_path("tmp1.png").exists() assert storage.get_path("tmp1.png", thumbnail=True).exists() - invoker.services.image_records.delete_many.assert_not_called() + invoker.services.image_records.delete_intermediates_by_names.assert_not_called() assert deleted_callbacks == [] assert _staging_dirs(storage) == [] @@ -475,7 +574,7 @@ def test_db_failure_restores_all_staged_files(self, disk_image_service: ImageSer for name in names: _save_image_file(storage, name) invoker.services.image_records.get_intermediates.return_value = [(name, "") for name in names] - invoker.services.image_records.delete_many.side_effect = ImageRecordDeleteException() + invoker.services.image_records.delete_intermediates_by_names.side_effect = ImageRecordDeleteException() deleted_callbacks: list[str] = [] disk_image_service.on_deleted(deleted_callbacks.append) @@ -497,7 +596,7 @@ def test_one_rollback_failure_does_not_abandon_other_rollbacks(self, image_servi ] tokens = [object(), object(), object()] invoker.services.image_files.stage_delete.side_effect = tokens - invoker.services.image_records.delete_many.side_effect = ImageRecordDeleteException() + invoker.services.image_records.delete_intermediates_by_names.side_effect = ImageRecordDeleteException() invoker.services.image_files.rollback_delete.side_effect = [ ImageFileDeleteException("rollback broken"), None, @@ -533,3 +632,167 @@ def test_commit_failure_is_logged_and_remaining_commits_attempted(self, image_se assert deleted_callbacks == ["tmp1.png", "tmp2.png"] invoker.services.logger.error.assert_called() invoker.services.image_files.rollback_delete.assert_not_called() + + +class TestDeleteIntermediatesAgainstRealRecords: + """delete_intermediates() wired to a real record store, so no stub stands in for the DB decision. + + The mocked tests above can only assert that the service honours whatever the store reports. These + exercise the real store, which is where the promoted-vs-already-gone distinction is actually made. + """ + + @pytest.fixture + def wired(self, tmp_path: Path) -> tuple[ImageService, SqliteImageRecordStorage, DiskImageFileStorage]: + config = InvokeAIAppConfig(use_memory_db=True) + logger = InvokeAILogger.get_logger(config=config) + records = SqliteImageRecordStorage(db=create_mock_sqlite_database(config, logger)) + storage = DiskImageFileStorage(tmp_path / "outputs") + + svc = ImageService() + invoker = MagicMock() + invoker.services.configuration.pil_compress_level = 1 + invoker.services.image_records = records + invoker.services.image_files = storage + storage.start(invoker) + svc.start(invoker) + return svc, records, storage + + def _seed(self, records: SqliteImageRecordStorage, storage: DiskImageFileStorage, name: str) -> None: + records.save( + image_name=name, + image_origin=ResourceOrigin.INTERNAL, + image_category=ImageCategory.GENERAL, + width=64, + height=64, + has_workflow=False, + is_intermediate=True, + ) + _save_image_file(storage, name) + + def _promote_after_staging( + self, + records: SqliteImageRecordStorage, + storage: DiskImageFileStorage, + monkeypatch, + image_name: str, + ) -> None: + """Promote an image out of intermediate status once its files have been staged. + + Promoting it *before* delete_intermediates() runs would keep it out of the snapshot entirely, + so it would never reach the retained path these tests are about. + """ + real_stage_delete = storage.stage_delete + + def stage_then_promote(name: str, *args, **kwargs): + token = real_stage_delete(name, *args, **kwargs) + if name == image_name: + records.update(image_name, ImageRecordChanges(is_intermediate=False)) + return token + + monkeypatch.setattr(storage, "stage_delete", stage_then_promote) + + def test_promoted_image_keeps_record_and_files(self, wired, monkeypatch) -> None: + svc, records, storage = wired + self._seed(records, storage, "tmp1.png") + self._seed(records, storage, "promoted.png") + self._promote_after_staging(records, storage, monkeypatch, "promoted.png") + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) + + assert svc.delete_intermediates() == 1 + + assert storage.get_path("promoted.png").exists() + assert records.get("promoted.png").is_intermediate is False + assert not storage.get_path("tmp1.png").exists() + assert deleted_callbacks == ["tmp1.png"] + assert _staging_dirs(storage) == [] + + def test_record_removed_by_another_path_does_not_resurrect_its_files(self, wired, monkeypatch) -> None: + """A record deleted elsewhere while we hold its files must not have those files restored. + + "Not deleted by us" is not the same as "still there". Restoring files for a record that is + gone orphans them on disk forever: no row references them and no staging dir remains for + startup recovery to find. + """ + svc, records, storage = wired + self._seed(records, storage, "tmp1.png") + self._seed(records, storage, "gone.png") + + real_stage_delete = storage.stage_delete + + def stage_then_lose_the_record(image_name: str, *args, **kwargs): + token = real_stage_delete(image_name, *args, **kwargs) + if image_name == "gone.png": + # Another path (single-image delete, board delete, maintenance script) removes the + # record after we have already staged its files. + records.delete("gone.png") + return token + + monkeypatch.setattr(storage, "stage_delete", stage_then_lose_the_record) + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) + + count = svc.delete_intermediates() + + # gone.png's files must be purged, not restored. + assert not storage.get_path("gone.png").exists() + assert not storage.get_path("gone.png", thumbnail=True).exists() + assert not storage.get_path("tmp1.png").exists() + assert _staging_dirs(storage) == [] + # Only the record this call actually removed is counted and announced. + assert count == 1 + assert deleted_callbacks == ["tmp1.png"] + + def test_retained_record_deleted_before_rollback_does_not_resurrect_its_files(self, wired, monkeypatch) -> None: + """A retained record can still be deleted while the commit/rollback loop is running. + + The loop can work through thousands of names before reaching a given token, so "retained at + DB-call time" is not enough to justify restoring files. Restoring them against a record that + has since been deleted strands them: no row refers to them and the staging dir is gone, so + startup recovery can never find them. + """ + svc, records, storage = wired + self._seed(records, storage, "tmp1.png") + self._seed(records, storage, "promoted.png") + self._promote_after_staging(records, storage, monkeypatch, "promoted.png") + + real_delete_by_names = records.delete_intermediates_by_names + + def delete_then_lose_the_retained_record(names: list[str]): + deleted, retained = real_delete_by_names(names) + # Another path deletes the promoted image after we decided to keep its files. + for name in retained: + records.delete(name) + return deleted, retained + + monkeypatch.setattr(records, "delete_intermediates_by_names", delete_then_lose_the_retained_record) + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) + + count = svc.delete_intermediates() + + assert not storage.get_path("promoted.png").exists() + assert not storage.get_path("promoted.png", thumbnail=True).exists() + assert _staging_dirs(storage) == [] + # Only this call's own deletion is counted and announced. + assert count == 1 + assert deleted_callbacks == ["tmp1.png"] + + def test_lookup_failure_during_rollback_check_keeps_the_files(self, wired, monkeypatch) -> None: + """If we cannot tell whether the record survived, keep the files — a lost file is final.""" + svc, records, storage = wired + self._seed(records, storage, "promoted.png") + self._promote_after_staging(records, storage, monkeypatch, "promoted.png") + + def unavailable(image_name: str): + raise sqlite3.OperationalError("database is locked") + + monkeypatch.setattr(records, "get", unavailable) + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) + + assert svc.delete_intermediates() == 0 + + assert storage.get_path("promoted.png").exists() + assert _staging_dirs(storage) == [] + assert deleted_callbacks == []