From 887ebfdcb099e9123139a1686e94c5e770e8869e Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 16 Jul 2026 21:15:34 -0400 Subject: [PATCH 1/6] 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/6] 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 == [] From ccfc32f8f3c2bb18c3288586e1d9ace1e66215f4 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Thu, 13 Aug 2026 20:15:58 -0400 Subject: [PATCH 3/6] fix(images): delete intermediate records before files to close the promoted-image orphan race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses JPPhoto's round-2 merge blocker on PR #9361. The prior revision staged every intermediate file, conditionally deleted the records, then restored the files of any image promoted out of intermediate status mid-operation. That restore is unfixably racy: while a promoted image's files sit in our staging directory, a concurrent single-image or board delete can stage-empty (find no files to move) and then remove the record; our restore then puts the files back with no record referencing them and no staging dir for startup recovery — a permanent orphan. Holding the record-store write transaction across the restore (the suggested fix) narrows but does not close the window, because the competing delete's file-staging happens under no lock and can precede the restore. delete_intermediates() now deletes records first and files second. The conditional DELETE is atomic and returns exactly the names it removed; we then purge only those files, best-effort (a filesystem failure orphans one file but never aborts the remaining purges or undoes the committed deletions). A promoted image is never deleted and its files are never staged, so there is no restore step for a concurrent delete to race, and a concurrent delete of that image operates on real files in the output folder and stays consistent. delete_intermediates_by_names() now returns just the deleted names instead of (deleted, retained); the retained set is no longer needed. Tests rewritten to the records-first contract, including a regression test that concurrently deletes a promoted image right after the conditional DELETE keeps it and asserts its files are not resurrected. Co-Authored-By: Claude Opus 4.8 --- .../image_records/image_records_base.py | 8 +- .../image_records/image_records_sqlite.py | 12 +- .../app/services/images/images_default.py | 98 ++--- .../test_image_records_sqlite.py | 22 +- .../services/images/test_images_default.py | 339 ++++++------------ 5 files changed, 165 insertions(+), 314 deletions(-) diff --git a/invokeai/app/services/image_records/image_records_base.py b/invokeai/app/services/image_records/image_records_base.py index 41c1450fa62..161f1757ac5 100644 --- a/invokeai/app/services/image_records/image_records_base.py +++ b/invokeai/app/services/image_records/image_records_base.py @@ -75,12 +75,12 @@ def get_intermediates(self) -> list[tuple[str, str]]: pass @abstractmethod - def delete_intermediates_by_names(self, image_names: list[str]) -> tuple[list[str], list[str]]: + def delete_intermediates_by_names(self, image_names: 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. + Returns the names whose records this call actually removed. Names that were already gone, and + names whose records survive because they are no longer intermediates, are both excluded, so a + caller purges the files of exactly the returned names and touches nothing else. """ pass diff --git a/invokeai/app/services/image_records/image_records_sqlite.py b/invokeai/app/services/image_records/image_records_sqlite.py index a8d08362755..9da3c0a607e 100644 --- a/invokeai/app/services/image_records/image_records_sqlite.py +++ b/invokeai/app/services/image_records/image_records_sqlite.py @@ -317,7 +317,7 @@ 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]]: + def delete_intermediates_by_names(self, image_names: 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 @@ -325,12 +325,11 @@ def delete_intermediates_by_names(self, image_names: list[str]) -> tuple[list[st 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. + Returns the names whose records this call actually removed. Names that were already gone, and + names whose records survive because they are no longer intermediates, are both excluded — the + caller purges the files of exactly the returned names and touches nothing else. """ 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 @@ -350,11 +349,10 @@ def delete_intermediates_by_names(self, image_names: list[str]) -> tuple[list[st 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 + return deleted def save( self, diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index af7f18c9465..8b112adec99 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -385,85 +385,49 @@ 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 a live record's files are never destroyed — though a failed rollback leaves them - # in the staging dir until startup recovery restores them. + # Records first, files second. An earlier revision staged every file, then conditionally + # deleted the records, then restored the files of any image that had been promoted out of + # intermediate status mid-operation. That restore is unfixably racy: while a promoted + # image's files sit in our staging directory, a concurrent single-image or board delete can + # stage-empty (it finds no files to move) and then remove the record; our restore then puts + # the files back with no record referencing them and no staging dir to recover from — + # permanent orphans (JPPhoto, PR #9361). + # + # Deleting the records first removes that hazard entirely: the conditional DELETE is atomic + # and tells us exactly which rows it removed, and we only ever touch the files of rows that + # are already gone. A promoted image is never deleted and its files are never staged, so a + # concurrent delete of it operates on real files in the output folder and stays consistent. try: 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)) - # 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: - 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 - 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 + subfolders = dict(image_name_subfolder_pairs) + # Conditional on the row still being an intermediate: an image promoted between the + # snapshot above and this call keeps both its record and its files. Returns exactly the + # names this call removed (already-absent and promoted rows are excluded). + deleted_image_names = self.__invoker.services.image_records.delete_intermediates_by_names( + list(subfolders.keys()) + ) + # The records are committed as gone; purge each file best-effort. A filesystem failure + # here orphans that file (nothing references it) but must neither abort the remaining + # purges nor undo the committed deletions, so failures are logged and skipped rather + # than raised. + for image_name in deleted_image_names: try: - self.__invoker.services.image_files.commit_delete(token) + self.__invoker.services.image_files.delete( + image_name, image_subfolder=subfolders.get(image_name, "") + ) except Exception as cleanup_error: - self.__invoker.services.logger.error(f"Failed to purge staged image files: {cleanup_error}") - if image_name in deleted_names: - deleted_image_names.append(image_name) + self.__invoker.services.logger.error( + f"Failed to purge intermediate image files for {image_name}: {cleanup_error}" + ) for image_name in deleted_image_names: self._on_deleted(image_name) return len(deleted_image_names) except ImageRecordDeleteException: self.__invoker.services.logger.error("Failed to delete image records") raise - except ImageFileDeleteException: - self.__invoker.services.logger.error("Failed to delete image files") - raise except Exception as e: - self.__invoker.services.logger.error("Problem deleting image records and files") + self.__invoker.services.logger.error("Problem deleting intermediate image records and files") raise e def get_intermediates_count(self, user_id: Optional[str] = None) -> int: 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 f498a14aea9..62d7c58f5cf 100644 --- a/tests/app/services/image_records/test_image_records_sqlite.py +++ b/tests/app/services/image_records/test_image_records_sqlite.py @@ -144,10 +144,9 @@ def test_get_intermediates_does_not_delete(self, store: SqliteImageRecordStorage 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() - deleted, retained = store.delete_intermediates_by_names([name for name, _ in pairs]) + deleted = store.delete_intermediates_by_names([name for name, _ in pairs]) assert deleted == ["tmp.png"] - assert retained == [] with pytest.raises(ImageRecordNotFoundException): store.get("tmp.png") @@ -197,10 +196,10 @@ def test_promoted_image_keeps_its_record(self, store: SqliteImageRecordStorage) # 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) + deleted = store.delete_intermediates_by_names(snapshot) assert deleted == ["tmp.png"] - assert retained == ["promoted.png"] + # promoted.png is excluded from the returned names, so the caller never purges its files. assert store.get("promoted.png").is_intermediate is False with pytest.raises(ImageRecordNotFoundException): store.get("tmp.png") @@ -233,22 +232,21 @@ def trace(statement: str) -> None: store._db._conn.set_trace_callback(trace) try: - deleted, retained = store.delete_intermediates_by_names(snapshot) + deleted = 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.delete_intermediates_by_names([]) == [] + # "gone.png" has no record at all and "keep.png" is not an intermediate, so neither is + # deleted or returned; keep.png must still be present afterwards. + assert store.delete_intermediates_by_names(["gone.png", "keep.png"]) == [] assert store.get("keep.png").image_name == "keep.png" def test_more_names_than_sql_variable_limit(self, store: SqliteImageRecordStorage) -> None: @@ -261,10 +259,10 @@ def test_more_names_than_sql_variable_limit(self, store: SqliteImageRecordStorag survivor = names[chunk + 3] store.update(survivor, ImageRecordChanges(is_intermediate=False)) - deleted, retained = store.delete_intermediates_by_names(names) + deleted = store.delete_intermediates_by_names(names) assert set(deleted) == set(names) - {survivor} - assert retained == [survivor] + assert survivor not in deleted assert store.get(survivor).is_intermediate is False assert store.get_intermediates() == [] diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index 6a54e2c2740..fb0e0f97fa6 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -5,7 +5,6 @@ staged-deletion contracts of delete() and delete_intermediates(). """ -import sqlite3 from pathlib import Path from unittest.mock import MagicMock, patch @@ -45,8 +44,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), []) + # By default every named 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 @@ -289,7 +288,7 @@ def test_delete_intermediates_forwards_subfolder(self, image_service: ImageServi count = image_service.delete_intermediates() assert count == 2 - calls = invoker.services.image_files.stage_delete.call_args_list + calls = invoker.services.image_files.delete.call_args_list assert calls[0].args == ("img1.png",) assert calls[0].kwargs == {"image_subfolder": "intermediate"} assert calls[1].args == ("img2.png",) @@ -382,8 +381,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), []) + # By default every named 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) @@ -480,7 +479,9 @@ def test_delete_commit_failure_is_logged_not_raised(self, image_service: ImageSe class TestDeleteIntermediatesTransactional: - """delete_intermediates() must be all-or-nothing: stage everything, delete records once, commit.""" + """delete_intermediates() deletes records first, then purges the files of exactly the rows it + removed. It never stages or restores a promoted image's files, so there is no restore step for a + concurrent delete to race (PR #9361, JPPhoto round 2).""" def test_success_deletes_multiple_intermediates(self, disk_image_service: ImageService): invoker = disk_image_service._ImageService__invoker # type: ignore @@ -502,8 +503,8 @@ def test_success_deletes_multiple_intermediates(self, disk_image_service: ImageS 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.""" + def test_promoted_image_keeps_its_files(self, disk_image_service: ImageService): + """An image the DB refused to delete (no longer an intermediate) keeps its files untouched.""" invoker = disk_image_service._ImageService__invoker # type: ignore storage = invoker.services.image_files for name in ("tmp1.png", "promoted.png", "tmp2.png"): @@ -513,11 +514,10 @@ def test_image_promoted_out_of_intermediate_keeps_record_and_files(self, disk_im ("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"], - ) + # The store reports it removed everything except promoted.png, so that file is never purged. + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: [ + name for name in names if name != "promoted.png" + ] deleted_callbacks: list[str] = [] disk_image_service.on_deleted(deleted_callbacks.append) @@ -532,191 +532,92 @@ def test_image_promoted_out_of_intermediate_keeps_record_and_files(self, disk_im assert deleted_callbacks == ["tmp1.png", "tmp2.png"] assert _staging_dirs(storage) == [] - def test_promoted_image_is_rolled_back_not_committed(self, image_service: ImageService): + def test_only_deleted_rows_are_purged_and_announced(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") + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: ["tmp1.png"] 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]) + # The promoted row's file is never touched: only the deleted row is purged. + invoker.services.image_files.delete.assert_called_once_with("tmp1.png", image_subfolder="") 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): + def test_subfolder_is_forwarded_to_the_file_purge(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_records.get_intermediates.return_value = [("tmp1.png", "a/b")] + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: list(names) + image_service.delete_intermediates() - invoker.services.image_files.rollback_delete.assert_called_once_with(token) - invoker.services.image_files.commit_delete.assert_not_called() - assert deleted_callbacks == [] + invoker.services.image_files.delete.assert_called_once_with("tmp1.png", image_subfolder="a/b") - def test_first_staging_failure_aborts_without_db_delete(self, image_service: ImageService): + def test_file_purge_failure_is_logged_and_does_not_abort_or_raise(self, image_service: ImageService): + """A filesystem failure orphans one file but must not stop the other purges, undo the + committed record deletions, or raise: the records are already gone.""" 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") + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: list(names) + invoker.services.image_files.delete.side_effect = [ImageFileDeleteException("purge failed"), None] deleted_callbacks: list[str] = [] image_service.on_deleted(deleted_callbacks.append) - with pytest.raises(ImageFileDeleteException): - image_service.delete_intermediates() + count = image_service.delete_intermediates() - 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() - assert deleted_callbacks == [] + assert count == 2 + purged = [call.args[0] for call in invoker.services.image_files.delete.call_args_list] + assert purged == ["tmp1.png", "tmp2.png"] + # Both records were deleted, so both deletions are announced despite the file failure. + assert deleted_callbacks == ["tmp1.png", "tmp2.png"] + invoker.services.logger.error.assert_called() - def test_later_staging_failure_rolls_back_earlier_stages(self, image_service: ImageService): + def test_db_failure_raises_and_purges_nothing(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_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 == [] - - 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_intermediates_by_names.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_intermediates_by_names.side_effect = ImageRecordDeleteException() deleted_callbacks: list[str] = [] - disk_image_service.on_deleted(deleted_callbacks.append) + image_service.on_deleted(deleted_callbacks.append) with pytest.raises(ImageRecordDeleteException): - disk_image_service.delete_intermediates() + image_service.delete_intermediates() - for name in names: - assert storage.get_path(name).exists() - assert storage.get_path(name, thumbnail=True).exists() + # No record was removed, so no file may be purged. + invoker.services.image_files.delete.assert_not_called() assert deleted_callbacks == [] - assert _staging_dirs(storage) == [] - def test_one_rollback_failure_does_not_abandon_other_rollbacks(self, image_service: ImageService): + 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 = [ - ("tmp1.png", ""), - ("tmp2.png", ""), - ("tmp3.png", ""), - ] - tokens = [object(), object(), object()] - invoker.services.image_files.stage_delete.side_effect = tokens - invoker.services.image_records.delete_intermediates_by_names.side_effect = ImageRecordDeleteException() - invoker.services.image_files.rollback_delete.side_effect = [ - ImageFileDeleteException("rollback broken"), - None, - None, - ] + invoker.services.image_records.get_intermediates.return_value = [("promoted.png", "")] + invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: [] deleted_callbacks: list[str] = [] image_service.on_deleted(deleted_callbacks.append) - with pytest.raises(ImageRecordDeleteException): - image_service.delete_intermediates() + assert image_service.delete_intermediates() == 0 - # 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() + invoker.services.image_files.delete.assert_not_called() assert deleted_callbacks == [] - def test_commit_failure_is_logged_and_remaining_commits_attempted(self, image_service: ImageService): + def test_empty_intermediates_is_a_noop(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] + invoker.services.image_records.get_intermediates.return_value = [] deleted_callbacks: list[str] = [] image_service.on_deleted(deleted_callbacks.append) - count = image_service.delete_intermediates() + assert image_service.delete_intermediates() == 0 - 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() + invoker.services.image_records.delete_intermediates_by_names.assert_called_once_with([]) + invoker.services.image_files.delete.assert_not_called() + assert deleted_callbacks == [] 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. + exercise the real store, which is where the promoted-vs-already-gone distinction is actually made, + and where the concurrency hazards JPPhoto reported would surface. """ @pytest.fixture @@ -747,33 +648,48 @@ def _seed(self, records: SqliteImageRecordStorage, storage: DiskImageFileStorage ) _save_image_file(storage, name) - def _promote_after_staging( + def _promote_after_snapshot( self, records: SqliteImageRecordStorage, - storage: DiskImageFileStorage, monkeypatch, image_name: str, ) -> None: - """Promote an image out of intermediate status once its files have been staged. + """Promote an image out of intermediate status after the snapshot but before the DB delete. - 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. + Promoting it earlier would drop it from the snapshot entirely; the interesting case is an + image that is in the snapshot yet is no longer an intermediate by the time the conditional + DELETE runs, so its record (and files) must survive. """ - real_stage_delete = storage.stage_delete + real_get_intermediates = records.get_intermediates + + def snapshot_then_promote(): + pairs = real_get_intermediates() + records.update(image_name, ImageRecordChanges(is_intermediate=False)) + return pairs + + monkeypatch.setattr(records, "get_intermediates", snapshot_then_promote) + + def test_all_intermediates_are_deleted(self, wired) -> None: + svc, records, storage = wired + self._seed(records, storage, "tmp1.png") + self._seed(records, storage, "tmp2.png") + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) - 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 + assert svc.delete_intermediates() == 2 - monkeypatch.setattr(storage, "stage_delete", stage_then_promote) + for name in ("tmp1.png", "tmp2.png"): + assert not storage.get_path(name).exists() + with pytest.raises(ImageRecordNotFoundException): + records.get(name) + assert sorted(deleted_callbacks) == ["tmp1.png", "tmp2.png"] + assert _staging_dirs(storage) == [] 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") + self._promote_after_snapshot(records, monkeypatch, "promoted.png") deleted_callbacks: list[str] = [] svc.on_deleted(deleted_callbacks.append) @@ -785,92 +701,67 @@ def test_promoted_image_keeps_record_and_files(self, wired, monkeypatch) -> None 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. - """ + def test_record_removed_by_another_path_between_snapshot_and_delete(self, wired, monkeypatch) -> None: + """An image fully deleted elsewhere after the snapshot is not counted and its (now absent) + files are left to the path that owns that deletion — we never touch them.""" svc, records, storage = wired self._seed(records, storage, "tmp1.png") self._seed(records, storage, "gone.png") - real_stage_delete = storage.stage_delete + real_get_intermediates = records.get_intermediates - 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 + def snapshot_then_delete_gone(): + pairs = real_get_intermediates() + # A single-image delete elsewhere removes gone.png (record and files) after our snapshot. + records.delete("gone.png") + storage.delete("gone.png") + return pairs - monkeypatch.setattr(storage, "stage_delete", stage_then_lose_the_record) + monkeypatch.setattr(records, "get_intermediates", snapshot_then_delete_gone) 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"] + assert not storage.get_path("tmp1.png").exists() + # gone.png was purged by the other path; we neither resurrect nor re-report it. + assert not storage.get_path("gone.png").exists() + assert _staging_dirs(storage) == [] - 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. + def test_promoted_record_deleted_after_conditional_delete_is_not_resurrected(self, wired, monkeypatch) -> None: + """The B3 regression: a promoted image is concurrently deleted (record and files) right after + the conditional DELETE keeps it. Because we never staged its files, there is nothing to + restore — its files stay deleted and are not stranded on disk with no record. """ 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") + self._promote_after_snapshot(records, 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 + def delete_then_lose_the_promoted_record(names: list[str]): + deleted = real_delete_by_names(names) + # A concurrent single-image delete removes the promoted image entirely, right after the + # conditional DELETE chose to keep it. + records.delete("promoted.png") + storage.delete("promoted.png") + return deleted - monkeypatch.setattr(records, "delete_intermediates_by_names", delete_then_lose_the_retained_record) + monkeypatch.setattr(records, "delete_intermediates_by_names", delete_then_lose_the_promoted_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 not storage.get_path("tmp1.png").exists() + # promoted.png's files stay deleted — never resurrected into an orphan. + assert not storage.get_path("promoted.png").exists() + assert not storage.get_path("promoted.png", thumbnail=True).exists() + with pytest.raises(ImageRecordNotFoundException): + records.get("promoted.png") assert _staging_dirs(storage) == [] - assert deleted_callbacks == [] From c2d7dfcb3f50fe685c765dcfd89fbe8dfd325227 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 29 Aug 2026 11:28:32 -0400 Subject: [PATCH 4/6] fix(images): journal deletions and never restore files whose record is gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review (JPPhoto) found two ways the delete paths could still strand files on disk with nothing referencing them. B1 — delete_intermediates() commits the record deletions before purging the files, so a crash or a filesystem failure in that window left orphans with no trace, and the call still reported success. Deletion now writes a durable journal first: DiskImageFileStorage.begin_delete() fsyncs a manifest naming every image about to be purged, commit_delete() purges and drops it, and abandon_delete() discards it when the record deletion failed. Startup recovery reconciles any journal that outlives its operation by asking the record store: an image whose record survives keeps its files, an image whose record is gone has its files purged. A purge that fails keeps its journal and is retried at the next startup instead of being logged and forgotten. B2 — single-image delete staged the files before deleting the record, so two concurrent deletes of the same image could interleave such that the one that failed restored files the other had already unreferenced. delete() is now records-first over the same journal and moves nothing, so there is no restore to race. The one remaining staging user (delete_images_on_board, which keeps its documented per-item failure contract) is covered by rollback_delete(): it re-checks the record after restoring and purges instead of orphaning. That check is race-free because every deleter purges an image's files strictly after its record is committed as gone. Recovery now probes with image_records.exists() rather than a deserializing get(), and the manifest carries a list so one journal covers a whole intermediates sweep. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XwVyhSN5AUBXGACM8yaLKD --- docs/src/content/docs/features/gallery.mdx | 4 + .../services/image_files/image_files_base.py | 24 +- .../services/image_files/image_files_disk.py | 197 +++++++++++-- .../app/services/images/images_default.py | 108 ++++---- .../image_files/test_image_files_disk.py | 165 ++++++++++- .../services/images/test_images_default.py | 262 ++++++++++++++---- 6 files changed, 617 insertions(+), 143 deletions(-) diff --git a/docs/src/content/docs/features/gallery.mdx b/docs/src/content/docs/features/gallery.mdx index 2fc37776a19..3ea698efc91 100644 --- a/docs/src/content/docs/features/gallery.mdx +++ b/docs/src/content/docs/features/gallery.mdx @@ -138,6 +138,10 @@ Additionally, each image has a context menu (right-click or Ctrl+click) with pow Selecting **Delete Image** will remove the image entirely from your InvokeAI installation. This action cannot be undone. ::: +:::note +A deletion removes the image's gallery record first and its files immediately afterwards. If the record cannot be removed, the image is left completely untouched — it stays in the gallery with its files intact. If Invoke is interrupted, or the storage refuses the delete, after the record is gone, the leftover files are noted in a journal and cleaned up the next time Invoke starts. The same applies to **Clear Intermediates**. +::: + --- ## Videos in the Gallery diff --git a/invokeai/app/services/image_files/image_files_base.py b/invokeai/app/services/image_files/image_files_base.py index 782e661c4b0..a5eb8fdc89e 100644 --- a/invokeai/app/services/image_files/image_files_base.py +++ b/invokeai/app/services/image_files/image_files_base.py @@ -1,4 +1,5 @@ from abc import ABC, abstractmethod +from collections.abc import Collection, Sequence from pathlib import Path from typing import Optional @@ -67,8 +68,27 @@ def stage_delete(self, image_name: str, image_subfolder: str = "") -> object: pass @abstractmethod - def commit_delete(self, token: object) -> None: - """Permanently removes files represented by a staged-delete token.""" + def begin_delete(self, images: Sequence[tuple[str, str]]) -> object: + """Durably records the intent to purge the (image_name, image_subfolder) pairs' files. + + Call this before deleting the records, then ``commit_delete()`` after. If the process dies + in between, startup recovery uses the journal to purge the files of every listed image + whose record is gone, and leaves the files of every image whose record survives. + """ + pass + + @abstractmethod + def commit_delete(self, token: object, image_names: Optional[Collection[str]] = None) -> None: + """Permanently removes the files represented by a delete token. + + ``image_names`` narrows a pending-delete token to the records that were actually deleted; + it is ignored for a staged-delete token. + """ + pass + + @abstractmethod + def abandon_delete(self, token: object) -> None: + """Drops a pending-delete journal without purging anything, leaving the files in place.""" pass @abstractmethod diff --git a/invokeai/app/services/image_files/image_files_disk.py b/invokeai/app/services/image_files/image_files_disk.py index 133c71ab05d..939ed464980 100644 --- a/invokeai/app/services/image_files/image_files_disk.py +++ b/invokeai/app/services/image_files/image_files_disk.py @@ -6,6 +6,7 @@ import tempfile import threading import zlib +from collections.abc import Collection, Sequence from dataclasses import dataclass from pathlib import Path from queue import Queue @@ -32,8 +33,25 @@ @dataclass class _StagedDelete: + """Files moved aside by ``stage_delete()``, restorable until the token is committed.""" + directory: Path files: list[tuple[Path, Path]] + image_name: str + image_subfolder: str + + +@dataclass +class _PendingDelete: + """A durable record of an intent to purge files, written before the records are deleted. + + Nothing is moved: the journal directory names the images whose files are about to become + unreferenced. Startup recovery reconciles any journal that outlives its operation by asking the + record store which of its images are really gone. + """ + + directory: Path + images: list[tuple[str, str]] def _get_png_size(image: PILImageType, compress_type: Optional[int] = None) -> int: @@ -89,7 +107,7 @@ def __init__(self, output_folder: Union[str, Path]): def start(self, invoker: Invoker) -> None: self.__invoker = invoker - self.__recover_staged_deletes() + self.__recover_pending_deletes() @property def image_root(self) -> Path: @@ -202,10 +220,7 @@ def delete(self, image_name: str, image_subfolder: str = "") -> None: self.commit_delete(token) def stage_delete(self, image_name: str, image_subfolder: str = "") -> _StagedDelete: - candidates = [ - self.get_path(image_name, image_subfolder=image_subfolder), - self.get_path(image_name, thumbnail=True, image_subfolder=image_subfolder), - ] + candidates = self.__delete_candidates(image_name, image_subfolder) staging_dir = Path(tempfile.mkdtemp(prefix=".delete_", dir=self.__output_folder)) staged: list[tuple[Path, Path]] = [] try: @@ -220,7 +235,9 @@ def stage_delete(self, image_name: str, image_subfolder: str = "") -> _StagedDel destination = staging_dir / str(index) source.replace(destination) staged.append((source, destination)) - return _StagedDelete(directory=staging_dir, files=staged) + return _StagedDelete( + directory=staging_dir, files=staged, image_name=image_name, image_subfolder=image_subfolder + ) except Exception as e: for source, destination in reversed(staged): if destination.exists(): @@ -229,7 +246,49 @@ def stage_delete(self, image_name: str, image_subfolder: str = "") -> _StagedDel shutil.rmtree(staging_dir, ignore_errors=True) raise ImageFileDeleteException from e - def commit_delete(self, token: object) -> None: + def begin_delete(self, images: Sequence[tuple[str, str]]) -> _PendingDelete: + """Durably records the intent to purge these images' files, before their records are deleted. + + Callers delete the records first and purge afterwards, so the only inconsistency that can + outlive a crash or a storage failure is a file nobody references. The journal written here + is what makes that recoverable: ``__recover_pending_deletes()`` asks the record store about + every image it names, purges the ones whose record is gone, and leaves the rest untouched. + """ + # Resolve every path up front. A name that cannot be turned into a path must fail here, + # while the caller can still abort — not after it has deleted the records. + for image_name, image_subfolder in images: + self.__delete_candidates(image_name, image_subfolder) + journal_dir = Path(tempfile.mkdtemp(prefix=".delete_", dir=self.__output_folder)) + try: + entries = [ + {"image_name": image_name, "image_subfolder": image_subfolder} for image_name, image_subfolder in images + ] + manifest_path = journal_dir / "manifest.json" + with open(manifest_path, "w", encoding="utf-8") as manifest: + manifest.write(json.dumps({"version": 2, "images": entries})) + manifest.flush() + os.fsync(manifest.fileno()) + # Fsync the directory too: without it a crash can leave a journal directory whose + # manifest entry never reached the disk, which recovery cannot act on. + self.__fsync_directory(journal_dir) + return _PendingDelete(directory=journal_dir, images=[(name, subfolder) for name, subfolder in images]) + except Exception as e: + shutil.rmtree(journal_dir, ignore_errors=True) + raise ImageFileDeleteException from e + + def abandon_delete(self, token: object) -> None: + """Drops a pending-delete journal without purging anything. + + Used when the record deletion failed: the images are still live, so their files must stay. + """ + if not isinstance(token, _PendingDelete): + raise ImageFileDeleteException("Invalid pending-delete token") + shutil.rmtree(token.directory, ignore_errors=True) + + def commit_delete(self, token: object, image_names: Optional[Collection[str]] = None) -> None: + if isinstance(token, _PendingDelete): + self.__commit_pending_delete(token, image_names) + return if not isinstance(token, _StagedDelete): raise ImageFileDeleteException("Invalid staged-delete token") try: @@ -237,6 +296,25 @@ def commit_delete(self, token: object) -> None: except Exception as e: raise ImageFileDeleteException from e + def __commit_pending_delete(self, token: _PendingDelete, image_names: Optional[Collection[str]]) -> None: + # ``image_names`` narrows the purge to the records that were actually deleted; the journal + # still lists every candidate, which is harmless because recovery re-checks each one against + # the record store and skips any that survived. + selected = image_names if image_names is None else set(image_names) + failures: list[str] = [] + for image_name, image_subfolder in token.images: + if selected is not None and image_name not in selected: + continue + try: + self.__purge_files(image_name, image_subfolder) + except OSError as e: + failures.append(f"{image_name}: {e}") + if failures: + # Leave the journal in place so the next startup retries every entry whose record is + # gone. Removing it here would turn a transient storage error into a permanent orphan. + raise ImageFileDeleteException(f"Failed to purge deleted image files: {'; '.join(failures)}") + shutil.rmtree(token.directory, ignore_errors=True) + def rollback_delete(self, token: object) -> None: if not isinstance(token, _StagedDelete): raise ImageFileDeleteException("Invalid staged-delete token") @@ -245,10 +323,56 @@ def rollback_delete(self, token: object) -> None: if destination.exists(): source.parent.mkdir(parents=True, exist_ok=True) destination.replace(source) + # While these files sat in the staging directory another request may have deleted the + # record; restoring them would leave files nothing references and no journal to find + # them by. Re-check now that the files are back: every deleter purges an image's files + # only *after* its record is committed as gone, so a record still present here cannot + # have been purged before this restore, and a record already absent means the purge + # either found nothing or is still to come — either way the files must go. + self.__purge_if_record_absent(token.image_name, token.image_subfolder) shutil.rmtree(token.directory, ignore_errors=True) except Exception as e: raise ImageFileDeleteException from e + def __delete_candidates(self, image_name: str, image_subfolder: str) -> list[Path]: + return [ + self.get_path(image_name, image_subfolder=image_subfolder), + self.get_path(image_name, thumbnail=True, image_subfolder=image_subfolder), + ] + + def __purge_files(self, image_name: str, image_subfolder: str) -> None: + """Removes an image's file and thumbnail. Missing files are not an error.""" + for path in self.__delete_candidates(image_name, image_subfolder): + with self.__cache_lock: + self.__cache.pop(path, None) + path.unlink(missing_ok=True) + + def __purge_if_record_absent(self, image_name: str, image_subfolder: str) -> None: + try: + record_exists = self.__invoker.services.image_records.exists(image_name) + except Exception as e: + # A storage fault must never destroy a live image's files. Keep them: a stale file is + # recoverable at the next startup, a deleted one is not. + InvokeAILogger.get_logger().error(f"Could not confirm whether {image_name} still exists: {e}") + return + if record_exists: + return + self.__purge_files(image_name, image_subfolder) + + @staticmethod + def __fsync_directory(directory: Path) -> None: + try: + dir_fd = os.open(directory, os.O_RDONLY) + except OSError: + # Windows cannot open a directory for fsync; the manifest write above is all we get. + return + try: + os.fsync(dir_fd) + except OSError: + pass + finally: + os.close(dir_fd) + def get_path(self, image_name: str, thumbnail: bool = False, image_subfolder: str = "") -> Path: base_folder = self.__thumbnails_folder if thumbnail else self.__output_folder filename = get_thumbnail_name(image_name) if thumbnail else image_name @@ -315,36 +439,51 @@ def __validate_storage_folders(self) -> None: for folder in folders: folder.mkdir(parents=True, exist_ok=True) - def __recover_staged_deletes(self) -> None: + def __recover_pending_deletes(self) -> None: + """Reconciles every delete journal left behind by an interrupted or failed deletion. + + One rule covers both journal shapes, and the record store decides it: an image whose record + survives was never really deleted, so anything staged for it is put back and the journal + dropped; an image whose record is gone is an orphan, so its files are purged wherever the + interrupted operation left them. + """ logger = InvokeAILogger.get_logger() - for staging_dir in self.__output_folder.glob(".delete_*"): - manifest_path = staging_dir / "manifest.json" + for journal_dir in sorted(self.__output_folder.glob(".delete_*")): + manifest_path = journal_dir / "manifest.json" if not manifest_path.is_file(): - if not any(staging_dir.iterdir()): - staging_dir.rmdir() + # mkdtemp() ran but the manifest never landed, so this directory names nothing and + # there is nothing to reconcile. Only remove it when it is empty. + if not any(journal_dir.iterdir()): + journal_dir.rmdir() continue try: with open(manifest_path, encoding="utf-8") as manifest: data = json.load(manifest) - image_name = data["image_name"] - image_subfolder = data.get("image_subfolder", "") - candidates = [ - self.get_path(image_name, image_subfolder=image_subfolder), - self.get_path(image_name, thumbnail=True, image_subfolder=image_subfolder), - ] - token = _StagedDelete( - directory=staging_dir, - files=[(source, staging_dir / str(index)) for index, source in enumerate(candidates)], - ) - self.__invoker.services.image_records.get(image_name) - self.rollback_delete(token) + for image_name, image_subfolder in self.__manifest_images(data): + if self.__invoker.services.image_records.exists(image_name): + # Put back whatever stage_delete() moved aside. Only a single-image journal + # ever holds staged files, at indices 0 and 1; a pending-delete journal + # moves nothing, so these lookups simply find nothing to restore. + for index, source in enumerate(self.__delete_candidates(image_name, image_subfolder)): + staged = journal_dir / str(index) + if staged.exists(): + source.parent.mkdir(parents=True, exist_ok=True) + staged.replace(source) + continue + self.__purge_files(image_name, image_subfolder) + shutil.rmtree(journal_dir, ignore_errors=True) except Exception as error: - from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException + # Includes a record-store fault: leave the journal for the next startup rather than + # guess. Retrying is always safe; both branches above are idempotent. + logger.error(f"Failed to recover image deletion journal {journal_dir}: {error}") - if isinstance(error, ImageRecordNotFoundException): - shutil.rmtree(staging_dir, ignore_errors=True) - else: - logger.error(f"Failed to recover staged image deletion {staging_dir}: {error}") + @staticmethod + def __manifest_images(data: dict) -> list[tuple[str, str]]: + """Reads both journal shapes: a pending delete lists many images, a staged delete names one.""" + entries = data.get("images") + if entries is None: + return [(data["image_name"], data.get("image_subfolder", ""))] + return [(entry["image_name"], entry.get("image_subfolder", "")) for entry in entries] def __get_cache(self, image_name: Path) -> Optional[PILImageType]: with self.__cache_lock: diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index 8b112adec99..f14cdd97a4b 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -290,43 +290,39 @@ 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 + # Record first, files second, with a durable journal spanning the two. Deleting the record + # first means a database failure leaves the image completely intact, and the only state + # that can outlive this call is a file nothing references — which the journal lets startup + # recovery find and purge. Nothing is ever moved aside and put back, so a concurrent + # deleter of the same image cannot resurrect files whose record has already been removed. try: record = self.__invoker.services.image_records.get(image_name) - 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 + token = self.__invoker.services.image_files.begin_delete([(image_name, record.image_subfolder)]) + try: + self.__invoker.services.image_records.delete(image_name) + except Exception: + # The image is still live: drop the journal and leave its files alone. + try: + self.__invoker.services.image_files.abandon_delete(token) + except Exception as cleanup_error: + self.__invoker.services.logger.error( + f"Failed to discard the delete journal for {image_name}: {cleanup_error}" + ) + raise 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}") + # The record is committed as gone, so the delete succeeded. The journal stays + # behind and startup recovery purges the leftover files. + self.__invoker.services.logger.error(f"Failed to purge deleted 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 @@ -386,40 +382,52 @@ def delete_images_on_board(self, board_id: str, user_id: Optional[str] = None) - raise e def delete_intermediates(self) -> int: - # Records first, files second. An earlier revision staged every file, then conditionally - # deleted the records, then restored the files of any image that had been promoted out of - # intermediate status mid-operation. That restore is unfixably racy: while a promoted - # image's files sit in our staging directory, a concurrent single-image or board delete can - # stage-empty (it finds no files to move) and then remove the record; our restore then puts - # the files back with no record referencing them and no staging dir to recover from — - # permanent orphans (JPPhoto, PR #9361). + # Records first, files second, with a durable journal spanning the two. An earlier revision + # staged every file, then conditionally deleted the records, then restored the files of any + # image that had been promoted out of intermediate status mid-operation. That restore is + # unfixably racy: while a promoted image's files sit in a staging directory, a concurrent + # single-image or board delete can stage-empty (it finds no files to move) and then remove + # the record; the restore then puts the files back with no record referencing them and no + # journal to recover from — permanent orphans (JPPhoto, PR #9361). # - # Deleting the records first removes that hazard entirely: the conditional DELETE is atomic - # and tells us exactly which rows it removed, and we only ever touch the files of rows that - # are already gone. A promoted image is never deleted and its files are never staged, so a - # concurrent delete of it operates on real files in the output folder and stays consistent. + # Deleting the records first removes that hazard: the conditional DELETE is atomic and + # reports exactly which rows it removed, and only the files of already-deleted rows are + # touched. A promoted image is never deleted and its files are never moved, so a concurrent + # delete of it operates on real files in the output folder and stays consistent. The + # journal covers the window the reordering opens: if this process dies, or the filesystem + # fails, between the commit and the purge, startup recovery finishes the purge for every + # journalled image whose record is gone. try: image_name_subfolder_pairs = self.__invoker.services.image_records.get_intermediates() + if not image_name_subfolder_pairs: + return 0 subfolders = dict(image_name_subfolder_pairs) - # Conditional on the row still being an intermediate: an image promoted between the - # snapshot above and this call keeps both its record and its files. Returns exactly the - # names this call removed (already-absent and promoted rows are excluded). - deleted_image_names = self.__invoker.services.image_records.delete_intermediates_by_names( - list(subfolders.keys()) - ) - # The records are committed as gone; purge each file best-effort. A filesystem failure - # here orphans that file (nothing references it) but must neither abort the remaining - # purges nor undo the committed deletions, so failures are logged and skipped rather - # than raised. - for image_name in deleted_image_names: + token = self.__invoker.services.image_files.begin_delete(list(subfolders.items())) + try: + # Conditional on the row still being an intermediate: an image promoted between the + # snapshot above and this call keeps both its record and its files. Returns exactly + # the names this call removed (already-absent and promoted rows are excluded). + deleted_image_names = self.__invoker.services.image_records.delete_intermediates_by_names( + list(subfolders.keys()) + ) + except Exception: try: - self.__invoker.services.image_files.delete( - image_name, image_subfolder=subfolders.get(image_name, "") - ) + self.__invoker.services.image_files.abandon_delete(token) except Exception as cleanup_error: self.__invoker.services.logger.error( - f"Failed to purge intermediate image files for {image_name}: {cleanup_error}" + f"Failed to discard the intermediates delete journal: {cleanup_error}" ) + raise + try: + # Only the names whose records this call removed are purged; a promoted image keeps + # its files. The journal still lists it, which is harmless — recovery re-checks + # every entry against the record store and skips the ones that survived. + self.__invoker.services.image_files.commit_delete(token, image_names=deleted_image_names) + except Exception as cleanup_error: + # The records are committed as gone, so the deletion succeeded. A file that could + # not be purged keeps its journal entry and is retried at the next startup; it must + # neither fail the operation nor undo the committed deletions. + self.__invoker.services.logger.error(f"Failed to purge intermediate image files: {cleanup_error}") for image_name in deleted_image_names: self._on_deleted(image_name) return len(deleted_image_names) diff --git a/tests/app/services/image_files/test_image_files_disk.py b/tests/app/services/image_files/test_image_files_disk.py index caccb347397..84313d27e77 100644 --- a/tests/app/services/image_files/test_image_files_disk.py +++ b/tests/app/services/image_files/test_image_files_disk.py @@ -7,12 +7,20 @@ import pytest from PIL import Image -from invokeai.app.services.image_files.image_files_common import ImageFileSaveException +from invokeai.app.services.image_files.image_files_common import ImageFileDeleteException, ImageFileSaveException from invokeai.app.services.image_files.image_files_disk import DiskImageFileStorage, _should_use_png_rle -from invokeai.app.services.image_records.image_records_common import ImageRecordNotFoundException from invokeai.app.util.thumbnails import get_thumbnail_name +def _restart(storage: DiskImageFileStorage, record_exists: bool) -> DiskImageFileStorage: + """Simulates a restart over the same output folder, running journal recovery.""" + invoker = MagicMock() + invoker.services.image_records.exists.return_value = record_exists + restarted = DiskImageFileStorage(storage.image_root) + restarted.start(invoker) + return restarted + + @pytest.fixture def image_names() -> list[str]: # Determine the platform and return a path that matches its format @@ -359,23 +367,166 @@ def test_startup_restores_staged_files_when_record_exists(self, disk_storage: Di image_name = "recover.png" disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=image_name) image_path = disk_storage.get_path(image_name) + thumbnail_path = disk_storage.get_path(image_name, thumbnail=True) disk_storage.stage_delete(image_name) - invoker = MagicMock() - invoker.services.image_records.get.return_value = object() - restarted = DiskImageFileStorage(disk_storage.image_root) - restarted.start(invoker) + _restart(disk_storage, record_exists=True) assert image_path.exists() + assert thumbnail_path.exists() + assert not list(disk_storage.image_root.glob(".delete_*")) def test_startup_purges_staged_files_when_record_was_deleted(self, disk_storage: DiskImageFileStorage): image_name = "purge.png" disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=image_name) + image_path = disk_storage.get_path(image_name) + disk_storage.stage_delete(image_name) + + _restart(disk_storage, record_exists=False) + + assert not image_path.exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_startup_leaves_the_journal_when_the_record_store_is_unreadable(self, disk_storage: DiskImageFileStorage): + """A database fault must not decide an image's fate; the journal is retried next startup.""" + image_name = "unreadable.png" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=image_name) disk_storage.stage_delete(image_name) invoker = MagicMock() - invoker.services.image_records.get.side_effect = ImageRecordNotFoundException + invoker.services.image_records.exists.side_effect = RuntimeError("database is locked") restarted = DiskImageFileStorage(disk_storage.image_root) restarted.start(invoker) + assert list(disk_storage.image_root.glob(".delete_*")) + + +class TestPendingDeleteJournal: + """begin_delete() writes the journal that makes records-first deletion recoverable. + + Nothing is moved, so a failure can only ever leave files nothing references — and the journal + is what lets the next startup find and purge exactly those. + """ + + def test_begin_delete_leaves_the_files_in_place(self, disk_storage: DiskImageFileStorage): + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="live.png") + + disk_storage.begin_delete([("live.png", "")]) + + assert disk_storage.get_path("live.png").exists() + assert disk_storage.get_path("live.png", thumbnail=True).exists() + assert len(list(disk_storage.image_root.glob(".delete_*"))) == 1 + + def test_begin_delete_rejects_an_unusable_name_before_writing_a_journal( + self, disk_storage: DiskImageFileStorage, tmp_path: Path + ): + """The caller deletes records straight after this returns, so a bad name must fail here.""" + with pytest.raises(ValueError, match="Invalid image name"): + disk_storage.begin_delete([("ok.png", ""), ("../evil.png", "")]) + + assert not list(tmp_path.glob(".delete_*")) + + def test_commit_purges_the_files_and_drops_the_journal(self, disk_storage: DiskImageFileStorage): + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="gone.png") + token = disk_storage.begin_delete([("gone.png", "")]) + + disk_storage.commit_delete(token) + + assert not disk_storage.get_path("gone.png").exists() + assert not disk_storage.get_path("gone.png", thumbnail=True).exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_commit_purges_only_the_named_images(self, disk_storage: DiskImageFileStorage): + """The journal lists every candidate; only the records that were really deleted are purged.""" + for name in ("deleted.png", "promoted.png"): + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=name) + token = disk_storage.begin_delete([("deleted.png", ""), ("promoted.png", "")]) + + disk_storage.commit_delete(token, image_names=["deleted.png"]) + + assert not disk_storage.get_path("deleted.png").exists() + assert disk_storage.get_path("promoted.png").exists() + assert disk_storage.get_path("promoted.png", thumbnail=True).exists() + + def test_commit_keeps_the_journal_when_a_file_cannot_be_purged(self, disk_storage: DiskImageFileStorage): + """One unremovable file must not abort the other purges, and must not discard the journal: + the entry has to survive so the next startup can retry it.""" + for name in ("bad.png", "good.png"): + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=name) + token = disk_storage.begin_delete([("bad.png", ""), ("good.png", "")]) + bad_path = disk_storage.get_path("bad.png") + real_unlink = Path.unlink + + def unlink(self: Path, missing_ok: bool = False): + if self == bad_path: + raise OSError("device busy") + return real_unlink(self, missing_ok=missing_ok) + + with patch.object(Path, "unlink", unlink), pytest.raises(ImageFileDeleteException): + disk_storage.commit_delete(token) + + assert bad_path.exists() + # The failure did not stop the rest of the purge... + assert not disk_storage.get_path("good.png").exists() + # ...and the journal is still there for startup recovery to finish. + assert list(disk_storage.image_root.glob(".delete_*")) + + _restart(disk_storage, record_exists=False) + + assert not bad_path.exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_abandon_keeps_the_files_and_drops_the_journal(self, disk_storage: DiskImageFileStorage): + """The record delete failed, so the image is still live and must be left completely alone.""" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="kept.png") + token = disk_storage.begin_delete([("kept.png", "")]) + + disk_storage.abandon_delete(token) + + assert disk_storage.get_path("kept.png").exists() + assert disk_storage.get_path("kept.png", thumbnail=True).exists() assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_startup_purges_journalled_files_whose_record_is_gone(self, disk_storage: DiskImageFileStorage): + """The crash window records-first opens: records committed as deleted, purge never ran.""" + for name in ("orphan.png", "survivor.png"): + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=name) + disk_storage.begin_delete([("orphan.png", ""), ("survivor.png", "")]) + + invoker = MagicMock() + invoker.services.image_records.exists.side_effect = lambda name: name == "survivor.png" + restarted = DiskImageFileStorage(disk_storage.image_root) + restarted.start(invoker) + + assert not disk_storage.get_path("orphan.png").exists() + assert not disk_storage.get_path("orphan.png", thumbnail=True).exists() + # The record survived, so this one was never deleted and keeps its files. + assert disk_storage.get_path("survivor.png").exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_rollback_purges_instead_of_restoring_when_the_record_is_gone(self, disk_storage: DiskImageFileStorage): + """A staged delete that fails must not resurrect files another request has already + unreferenced. Restoring them would strand them with no record and no journal to find + them by (JPPhoto, PR #9361).""" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="raced.png") + token = disk_storage.stage_delete("raced.png") + # Meanwhile another request deleted the record. + disk_storage._DiskImageFileStorage__invoker.services.image_records.exists.return_value = False + + disk_storage.rollback_delete(token) + + assert not disk_storage.get_path("raced.png").exists() + assert not disk_storage.get_path("raced.png", thumbnail=True).exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_rollback_restores_when_the_record_store_cannot_be_read(self, disk_storage: DiskImageFileStorage): + """An unreadable database must never cost a live image its files.""" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="kept.png") + token = disk_storage.stage_delete("kept.png") + records = disk_storage._DiskImageFileStorage__invoker.services.image_records + records.exists.side_effect = RuntimeError("database is locked") + + disk_storage.rollback_delete(token) + + assert disk_storage.get_path("kept.png").exists() + assert disk_storage.get_path("kept.png", thumbnail=True).exists() diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index fb0e0f97fa6..608e0f970db 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -272,10 +272,10 @@ def test_delete_forwards_subfolder(self, image_service: ImageService): image_service.delete("test.png") - invoker.services.image_files.stage_delete.assert_called_once_with("test.png", image_subfolder="2026/04/05") + invoker.services.image_files.begin_delete.assert_called_once_with([("test.png", "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 + invoker.services.image_files.begin_delete.return_value ) def test_delete_intermediates_forwards_subfolder(self, image_service: ImageService): @@ -288,12 +288,13 @@ 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 - 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_files.begin_delete.assert_called_once_with( + [("img1.png", "intermediate"), ("img2.png", "intermediate")] + ) invoker.services.image_records.delete_intermediates_by_names.assert_called_once_with(["img1.png", "img2.png"]) + invoker.services.image_files.commit_delete.assert_called_once_with( + invoker.services.image_files.begin_delete.return_value, image_names=["img1.png", "img2.png"] + ) # ── Point 3: delete_images_on_board silent-failure contract ── @@ -398,8 +399,51 @@ def _staging_dirs(storage: DiskImageFileStorage) -> list[Path]: return list(storage.image_root.glob(".delete_*")) +@pytest.fixture +def wired(tmp_path: Path) -> tuple[ImageService, SqliteImageRecordStorage, DiskImageFileStorage]: + """ImageService wired to a real record store and a real disk store — no stub decides anything.""" + 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_record(records: SqliteImageRecordStorage, name: str, is_intermediate: bool = True) -> None: + records.save( + image_name=name, + image_origin=ResourceOrigin.INTERNAL, + image_category=ImageCategory.GENERAL, + width=64, + height=64, + has_workflow=False, + is_intermediate=is_intermediate, + ) + + +def _restart_file_storage(storage: DiskImageFileStorage, records: SqliteImageRecordStorage) -> DiskImageFileStorage: + """Simulates a restart over the same output folder, running delete-journal recovery.""" + invoker = MagicMock() + invoker.services.image_records = records + restarted = DiskImageFileStorage(storage.image_root) + restarted.start(invoker) + return restarted + + +def _unlink_always_fails(path: Path, missing_ok: bool = False) -> None: + raise OSError("device busy") + + class TestDeleteTransactional: - """delete() must stage files, delete the record, then commit — never losing files on failure.""" + """delete() journals its intent, deletes the record, then purges — 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 @@ -417,9 +461,10 @@ def test_delete_success_removes_files_record_and_fires_callback_once(self, disk_ assert deleted_callbacks == ["img.png"] assert _staging_dirs(storage) == [] - def test_delete_staging_failure_keeps_record_and_raises(self, image_service: ImageService): + def test_delete_journal_failure_keeps_record_and_raises(self, image_service: ImageService): + """The journal is written before the record is deleted, so failing to write it aborts.""" invoker = image_service._ImageService__invoker # type: ignore - invoker.services.image_files.stage_delete.side_effect = ImageFileDeleteException("disk error") + invoker.services.image_files.begin_delete.side_effect = ImageFileDeleteException("disk error") deleted_callbacks: list[str] = [] image_service.on_deleted(deleted_callbacks.append) @@ -430,7 +475,7 @@ def test_delete_staging_failure_keeps_record_and_raises(self, image_service: Ima 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): + def test_delete_db_failure_leaves_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") @@ -442,24 +487,24 @@ def test_delete_db_failure_restores_files_and_raises(self, disk_image_service: I with pytest.raises(ImageRecordDeleteException): disk_image_service.delete("img.png") - # The image and its thumbnail must be restored to their original paths. + # Nothing was moved, so the image and its thumbnail are still exactly where they were. 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): + def test_delete_journal_cleanup_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") + invoker.services.image_files.abandon_delete.side_effect = ImageFileDeleteException("journal locked") 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.abandon_delete.assert_called_once_with( + invoker.services.image_files.begin_delete.return_value ) invoker.services.image_files.commit_delete.assert_not_called() assert deleted_callbacks == [] @@ -473,7 +518,7 @@ def test_delete_commit_failure_is_logged_not_raised(self, image_service: ImageSe 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() + invoker.services.image_files.abandon_delete.assert_not_called() assert deleted_callbacks == ["test.png"] invoker.services.logger.error.assert_called() @@ -543,7 +588,9 @@ def test_only_deleted_rows_are_purged_and_announced(self, image_service: ImageSe assert count == 1 # The promoted row's file is never touched: only the deleted row is purged. - invoker.services.image_files.delete.assert_called_once_with("tmp1.png", image_subfolder="") + invoker.services.image_files.commit_delete.assert_called_once_with( + invoker.services.image_files.begin_delete.return_value, image_names=["tmp1.png"] + ) assert deleted_callbacks == ["tmp1.png"] def test_subfolder_is_forwarded_to_the_file_purge(self, image_service: ImageService): @@ -552,25 +599,24 @@ def test_subfolder_is_forwarded_to_the_file_purge(self, image_service: ImageServ invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: list(names) image_service.delete_intermediates() - invoker.services.image_files.delete.assert_called_once_with("tmp1.png", image_subfolder="a/b") + invoker.services.image_files.begin_delete.assert_called_once_with([("tmp1.png", "a/b")]) - def test_file_purge_failure_is_logged_and_does_not_abort_or_raise(self, image_service: ImageService): - """A filesystem failure orphans one file but must not stop the other purges, undo the - committed record deletions, or raise: the records are already gone.""" + def test_file_purge_failure_is_logged_and_does_not_raise(self, image_service: ImageService): + """A filesystem failure must not undo the committed record deletions or raise: the records + are already gone, and the journal the purge leaves behind is retried at the next startup.""" invoker = image_service._ImageService__invoker # type: ignore invoker.services.image_records.get_intermediates.return_value = [("tmp1.png", ""), ("tmp2.png", "")] invoker.services.image_records.delete_intermediates_by_names.side_effect = lambda names: list(names) - invoker.services.image_files.delete.side_effect = [ImageFileDeleteException("purge failed"), None] + invoker.services.image_files.commit_delete.side_effect = ImageFileDeleteException("purge failed") deleted_callbacks: list[str] = [] image_service.on_deleted(deleted_callbacks.append) count = image_service.delete_intermediates() assert count == 2 - purged = [call.args[0] for call in invoker.services.image_files.delete.call_args_list] - assert purged == ["tmp1.png", "tmp2.png"] # Both records were deleted, so both deletions are announced despite the file failure. assert deleted_callbacks == ["tmp1.png", "tmp2.png"] + invoker.services.image_files.abandon_delete.assert_not_called() invoker.services.logger.error.assert_called() def test_db_failure_raises_and_purges_nothing(self, image_service: ImageService): @@ -583,8 +629,11 @@ def test_db_failure_raises_and_purges_nothing(self, image_service: ImageService) with pytest.raises(ImageRecordDeleteException): image_service.delete_intermediates() - # No record was removed, so no file may be purged. - invoker.services.image_files.delete.assert_not_called() + # No record was removed, so no file may be purged and the journal must be discarded. + invoker.services.image_files.commit_delete.assert_not_called() + invoker.services.image_files.abandon_delete.assert_called_once_with( + invoker.services.image_files.begin_delete.return_value + ) assert deleted_callbacks == [] def test_nothing_deleted_returns_zero_and_fires_no_callbacks(self, image_service: ImageService): @@ -596,7 +645,10 @@ def test_nothing_deleted_returns_zero_and_fires_no_callbacks(self, image_service assert image_service.delete_intermediates() == 0 - invoker.services.image_files.delete.assert_not_called() + # The journal still lists the promoted image; the purge selects nothing. + invoker.services.image_files.commit_delete.assert_called_once_with( + invoker.services.image_files.begin_delete.return_value, image_names=[] + ) assert deleted_callbacks == [] def test_empty_intermediates_is_a_noop(self, image_service: ImageService): @@ -607,8 +659,9 @@ def test_empty_intermediates_is_a_noop(self, image_service: ImageService): assert image_service.delete_intermediates() == 0 - invoker.services.image_records.delete_intermediates_by_names.assert_called_once_with([]) - invoker.services.image_files.delete.assert_not_called() + # Nothing to delete: no journal is written and the record store is never asked to delete. + invoker.services.image_records.delete_intermediates_by_names.assert_not_called() + invoker.services.image_files.begin_delete.assert_not_called() assert deleted_callbacks == [] @@ -620,32 +673,8 @@ class TestDeleteIntermediatesAgainstRealRecords: and where the concurrency hazards JPPhoto reported would surface. """ - @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, - ) + _seed_record(records, name) _save_image_file(storage, name) def _promote_after_snapshot( @@ -765,3 +794,126 @@ def delete_then_lose_the_promoted_record(names: list[str]): with pytest.raises(ImageRecordNotFoundException): records.get("promoted.png") assert _staging_dirs(storage) == [] + + +class TestDeleteAgainstRealRecords: + """Single-image delete wired to a real record store, covering the concurrent-delete interleaving + JPPhoto reported (PR #9361 round 3).""" + + def test_a_failed_delete_never_resurrects_files_another_request_removed(self, wired, monkeypatch) -> None: + """Two requests delete the same image; one commits the record deletion and the other fails. + + The failing request must not put the files back: nothing references them any more, and the + journal that would let startup recovery find them is gone with the request that won. + """ + svc, records, storage = wired + _seed_record(records, "img.png") + _save_image_file(storage, "img.png") + + real_delete = records.delete + + def competing_delete_then_fail(image_name: str) -> None: + # The competing request wins the race: it removes the record and purges the files while + # this delete is still in flight, and only then does this one's own delete fail. + real_delete(image_name) + storage.delete(image_name) + raise ImageRecordDeleteException() + + monkeypatch.setattr(records, "delete", competing_delete_then_fail) + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) + + with pytest.raises(ImageRecordDeleteException): + svc.delete("img.png") + + assert not storage.get_path("img.png").exists() + assert not storage.get_path("img.png", thumbnail=True).exists() + assert deleted_callbacks == [] + assert _staging_dirs(storage) == [] + + def test_a_database_failure_leaves_the_image_completely_intact(self, wired, monkeypatch) -> None: + svc, records, storage = wired + _seed_record(records, "img.png") + _save_image_file(storage, "img.png") + + def failing_delete(image_name: str) -> None: + raise ImageRecordDeleteException() + + monkeypatch.setattr(records, "delete", failing_delete) + + with pytest.raises(ImageRecordDeleteException): + svc.delete("img.png") + + assert storage.get_path("img.png").exists() + assert storage.get_path("img.png", thumbnail=True).exists() + assert records.get("img.png").image_name == "img.png" + # The journal is discarded: the image is live, so nothing must be left pointing at it. + assert _staging_dirs(storage) == [] + + +class TestDeleteJournalSurvivesFailedPurges: + """Records-first deletion commits the record removal before the files are purged. A crash or a + filesystem failure in that window must leave a journal, not a silent orphan (JPPhoto, PR #9361 + round 3).""" + + def test_intermediates_purge_failure_leaves_a_journal_the_next_startup_finishes(self, wired, monkeypatch) -> None: + svc, records, storage = wired + for name in ("tmp1.png", "tmp2.png"): + _seed_record(records, name) + _save_image_file(storage, name) + deleted_callbacks: list[str] = [] + svc.on_deleted(deleted_callbacks.append) + + with patch.object(Path, "unlink", _unlink_always_fails): + count = svc.delete_intermediates() + + # The records are committed as gone, so the deletion succeeded and is announced... + assert count == 2 + assert sorted(deleted_callbacks) == ["tmp1.png", "tmp2.png"] + for name in ("tmp1.png", "tmp2.png"): + with pytest.raises(ImageRecordNotFoundException): + records.get(name) + # ...but the files could not be removed, so they must still be journalled. + assert storage.get_path(name).exists() + assert _staging_dirs(storage) != [] + + _restart_file_storage(storage, records) + + for name in ("tmp1.png", "tmp2.png"): + assert not storage.get_path(name).exists() + assert not storage.get_path(name, thumbnail=True).exists() + assert _staging_dirs(storage) == [] + + def test_a_crash_before_the_purge_leaves_a_journal_the_next_startup_finishes(self, wired) -> None: + """The process dies between the committed record deletion and the file purge.""" + svc, records, storage = wired + _seed_record(records, "img.png", is_intermediate=False) + _save_image_file(storage, "img.png") + + # Everything delete() does up to the point of no return, and then nothing. + record = records.get("img.png") + storage.begin_delete([("img.png", record.image_subfolder)]) + records.delete("img.png") + + assert storage.get_path("img.png").exists() + + _restart_file_storage(storage, records) + + assert not storage.get_path("img.png").exists() + assert not storage.get_path("img.png", thumbnail=True).exists() + assert _staging_dirs(storage) == [] + + def test_a_crash_before_the_record_delete_keeps_the_image(self, wired) -> None: + """The mirror case: the journal is written but the record deletion never happened.""" + svc, records, storage = wired + _seed_record(records, "img.png", is_intermediate=False) + _save_image_file(storage, "img.png") + + storage.begin_delete([("img.png", "")]) + + _restart_file_storage(storage, records) + + assert storage.get_path("img.png").exists() + assert storage.get_path("img.png", thumbnail=True).exists() + assert records.get("img.png").image_name == "img.png" + assert _staging_dirs(storage) == [] From c4c09b564b0196f9ed0985c95a0e787195b5dc67 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 29 Aug 2026 11:58:44 -0400 Subject: [PATCH 5/6] fix(images): close the orphan paths an adversarial review found in the journal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of them break the invariant the rollback re-check rests on — that every deleter purges an image's files strictly after its record is committed as gone. 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 — 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. 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. It is now records-first over a journal like every other path. 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 — so a power loss could drop the journal and keep the deletion. Both directories are now fsynced, and stage_delete() does the same before it moves any file (previously it fsynced neither, so a lost manifest stranded staged files in a directory naming nothing). Recovery also no longer aborts startup on a stray .delete_* entry that is not a directory, and says so when a journal has no manifest instead of silently walking past files it cannot attribute. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XwVyhSN5AUBXGACM8yaLKD --- .../services/image_files/image_files_disk.py | 43 +++++++-- .../app/services/images/images_default.py | 58 +++++++++--- .../image_files/test_image_files_disk.py | 86 +++++++++++++++++ .../services/images/test_images_default.py | 92 +++++++++++++++++++ 4 files changed, 255 insertions(+), 24 deletions(-) diff --git a/invokeai/app/services/image_files/image_files_disk.py b/invokeai/app/services/image_files/image_files_disk.py index 939ed464980..0d555f3ef72 100644 --- a/invokeai/app/services/image_files/image_files_disk.py +++ b/invokeai/app/services/image_files/image_files_disk.py @@ -228,6 +228,9 @@ def stage_delete(self, image_name: str, image_subfolder: str = "") -> _StagedDel manifest.write(json.dumps({"image_name": image_name, "image_subfolder": image_subfolder})) manifest.flush() os.fsync(manifest.fileno()) + # The manifest has to be durable before the files move, or a crash can leave staged + # files in a directory that names nothing and recovery cannot put them back. + self.__persist_journal_directory(staging_dir) for index, source in enumerate(candidates): with self.__cache_lock: self.__cache.pop(source, None) @@ -268,9 +271,7 @@ def begin_delete(self, images: Sequence[tuple[str, str]]) -> _PendingDelete: manifest.write(json.dumps({"version": 2, "images": entries})) manifest.flush() os.fsync(manifest.fileno()) - # Fsync the directory too: without it a crash can leave a journal directory whose - # manifest entry never reached the disk, which recovery cannot act on. - self.__fsync_directory(journal_dir) + self.__persist_journal_directory(journal_dir) return _PendingDelete(directory=journal_dir, images=[(name, subfolder) for name, subfolder in images]) except Exception as e: shutil.rmtree(journal_dir, ignore_errors=True) @@ -292,6 +293,12 @@ def commit_delete(self, token: object, image_names: Optional[Collection[str]] = if not isinstance(token, _StagedDelete): raise ImageFileDeleteException("Invalid staged-delete token") try: + # Purge the live paths as well as the staged copies. stage_delete() captures whatever + # was there at that instant, so a second deleter racing the first gets an empty token — + # and if that second one is the one whose record deletion succeeds, dropping its empty + # staging directory alone would strand the files the first deleter restores. Committing + # has to mean "no file for this image survives", whichever request moved them. + self.__purge_files(token.image_name, token.image_subfolder) shutil.rmtree(token.directory) except Exception as e: raise ImageFileDeleteException from e @@ -359,6 +366,17 @@ def __purge_if_record_absent(self, image_name: str, image_subfolder: str) -> Non return self.__purge_files(image_name, image_subfolder) + def __persist_journal_directory(self, journal_dir: Path) -> None: + """Makes a journal directory and its manifest survive a power loss. + + Both fsyncs are needed: the first commits ``manifest.json``'s entry inside the journal + directory, the second commits the journal directory's own entry in the output folder. + Without the second, the record deletion — which SQLite does fsync — can outlive the journal + that is supposed to make it recoverable. + """ + self.__fsync_directory(journal_dir) + self.__fsync_directory(self.__output_folder) + @staticmethod def __fsync_directory(directory: Path) -> None: try: @@ -449,14 +467,19 @@ def __recover_pending_deletes(self) -> None: """ logger = InvokeAILogger.get_logger() for journal_dir in sorted(self.__output_folder.glob(".delete_*")): - manifest_path = journal_dir / "manifest.json" - if not manifest_path.is_file(): - # mkdtemp() ran but the manifest never landed, so this directory names nothing and - # there is nothing to reconcile. Only remove it when it is empty. - if not any(journal_dir.iterdir()): - journal_dir.rmdir() - continue try: + manifest_path = journal_dir / "manifest.json" + if not manifest_path.is_file(): + # mkdtemp() ran but the manifest never landed, so this directory names nothing + # and cannot be reconciled. Drop it when it is empty; otherwise say so, because + # anything inside it is a staged file that can no longer be put back. + if not journal_dir.is_dir(): + logger.warning(f"Ignoring unexpected entry in the outputs folder: {journal_dir}") + elif not any(journal_dir.iterdir()): + journal_dir.rmdir() + else: + logger.warning(f"Image deletion journal {journal_dir} has no manifest and cannot be recovered") + continue with open(manifest_path, encoding="utf-8") as manifest: data = json.load(manifest) for image_name, image_subfolder in self.__manifest_images(data): diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index f14cdd97a4b..d5069c62b4a 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -106,25 +106,55 @@ def create( raise except ImageFileSaveException: self.__invoker.services.logger.error("Failed to save image file") - try: - self.__invoker.services.image_files.delete(image_name, image_subfolder=image_subfolder) - except Exception as cleanup_error: - self.__invoker.services.logger.error( - f"Failed to clean up image files after save failure: {str(cleanup_error)}" - ) - try: - # Deleting the record also removes any board association through the database - # foreign key cascade. Both cleanup operations are attempted independently. - self.__invoker.services.image_records.delete(image_name) - except Exception as cleanup_error: - self.__invoker.services.logger.error( - f"Failed to clean up image record after save failure: {str(cleanup_error)}" - ) + self.__clean_up_failed_save(image_name, image_subfolder) raise except Exception as e: self.__invoker.services.logger.error(f"Problem saving image record and file: {str(e)}") raise e + def __clean_up_failed_save(self, image_name: str, image_subfolder: str) -> None: + """Removes the half-created image left by a failed save, record first. + + Record-then-files is the order every delete path uses, and it is load-bearing rather than + cosmetic: a concurrent deleter that has to roll back decides whether to restore an image's + files by asking whether its record is still there. Purging files while the record survives + would tell that deleter to put them back, stranding them once this cleanup finally removes + the record. The journal covers the window in between. + """ + token: object | None = None + try: + token = self.__invoker.services.image_files.begin_delete([(image_name, image_subfolder)]) + except Exception as cleanup_error: + self.__invoker.services.logger.error( + f"Failed to journal the cleanup of {image_name} after a save failure: {str(cleanup_error)}" + ) + try: + # Deleting the record also removes any board association through the database foreign + # key cascade. + self.__invoker.services.image_records.delete(image_name) + except Exception as cleanup_error: + self.__invoker.services.logger.error( + f"Failed to clean up image record after save failure: {str(cleanup_error)}" + ) + # The record survived, so the image is still referenced; its files must stay with it. + if token is not None: + try: + self.__invoker.services.image_files.abandon_delete(token) + except Exception as journal_error: + self.__invoker.services.logger.error( + f"Failed to discard the delete journal for {image_name}: {str(journal_error)}" + ) + return + try: + if token is None: + self.__invoker.services.image_files.delete(image_name, image_subfolder=image_subfolder) + else: + self.__invoker.services.image_files.commit_delete(token) + except Exception as cleanup_error: + self.__invoker.services.logger.error( + f"Failed to clean up image files after save failure: {str(cleanup_error)}" + ) + def update( self, image_name: str, diff --git a/tests/app/services/image_files/test_image_files_disk.py b/tests/app/services/image_files/test_image_files_disk.py index 84313d27e77..d50442e641a 100644 --- a/tests/app/services/image_files/test_image_files_disk.py +++ b/tests/app/services/image_files/test_image_files_disk.py @@ -50,6 +50,9 @@ def disk_storage(tmp_path: Path) -> DiskImageFileStorage: # Mock the invoker for save() which needs compress_level mock_invoker = MagicMock() mock_invoker.services.configuration.pil_compress_level = 6 + # Deletion asks the record store whether an image is still referenced; say yes unless a test + # says otherwise, so nothing here depends on a bare MagicMock happening to be truthy. + mock_invoker.services.image_records.exists.return_value = True storage._DiskImageFileStorage__invoker = mock_invoker # type: ignore return storage @@ -530,3 +533,86 @@ def test_rollback_restores_when_the_record_store_cannot_be_read(self, disk_stora assert disk_storage.get_path("kept.png").exists() assert disk_storage.get_path("kept.png", thumbnail=True).exists() + + +class TestJournalDurability: + """The journal only makes a deletion recoverable if it outlives a power loss. + + SQLite fsyncs the record deletion, so a journal that is merely written — and not fsynced, both + its manifest and its own directory entry in the output folder — can be lost while the record + stays deleted, which is exactly the orphan the journal exists to prevent. + """ + + def _record_fsyncs(self, monkeypatch) -> list[Path]: + fsynced: list[Path] = [] + monkeypatch.setattr( + DiskImageFileStorage, + "_DiskImageFileStorage__fsync_directory", + staticmethod(lambda directory: fsynced.append(Path(directory))), + ) + return fsynced + + def test_begin_delete_fsyncs_the_journal_and_the_output_folder( + self, disk_storage: DiskImageFileStorage, monkeypatch + ): + fsynced = self._record_fsyncs(monkeypatch) + + token = disk_storage.begin_delete([("img.png", "")]) + + assert Path(token.directory) in fsynced + assert disk_storage.image_root in [path.resolve() for path in fsynced] + + def test_stage_delete_fsyncs_before_it_moves_the_files(self, disk_storage: DiskImageFileStorage, monkeypatch): + """The manifest has to be durable first, or a crash leaves staged files naming nothing.""" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="staged.png") + moved: list[str] = [] + fsynced: list[Path] = [] + + def record_fsync(directory): + fsynced.append(Path(directory)) + + real_replace = Path.replace + + def record_replace(self: Path, target): + moved.append(str(target)) + assert fsynced, "the manifest was not made durable before the files were moved" + return real_replace(self, target) + + monkeypatch.setattr(DiskImageFileStorage, "_DiskImageFileStorage__fsync_directory", staticmethod(record_fsync)) + with patch.object(Path, "replace", record_replace): + token = disk_storage.stage_delete("staged.png") + + assert moved + assert Path(token.directory) in fsynced + assert disk_storage.image_root in [path.resolve() for path in fsynced] + + +class TestRecoveryToleratesStrayEntries: + """Recovery runs during start(); anything it cannot make sense of must not stop the app.""" + + def test_a_stray_file_does_not_stop_startup(self, disk_storage: DiskImageFileStorage): + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="live.png") + (disk_storage.image_root / ".delete_stray").write_text("not a directory") + + _restart(disk_storage, record_exists=True) + + assert disk_storage.get_path("live.png").exists() + + def test_a_journal_with_no_manifest_is_left_alone(self, disk_storage: DiskImageFileStorage): + """Its contents cannot be attributed to an image, so removing them would destroy data.""" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="live.png") + orphan_journal = disk_storage.image_root / ".delete_nomanifest" + orphan_journal.mkdir() + (orphan_journal / "0").write_bytes(b"staged image bytes") + + _restart(disk_storage, record_exists=True) + + assert (orphan_journal / "0").read_bytes() == b"staged image bytes" + assert disk_storage.get_path("live.png").exists() + + def test_an_empty_journal_directory_is_removed(self, disk_storage: DiskImageFileStorage): + (disk_storage.image_root / ".delete_empty").mkdir() + + _restart(disk_storage, record_exists=True) + + assert not list(disk_storage.image_root.glob(".delete_*")) diff --git a/tests/app/services/images/test_images_default.py b/tests/app/services/images/test_images_default.py index 608e0f970db..67f99be04b4 100644 --- a/tests/app/services/images/test_images_default.py +++ b/tests/app/services/images/test_images_default.py @@ -917,3 +917,95 @@ def test_a_crash_before_the_record_delete_keeps_the_image(self, wired) -> None: assert storage.get_path("img.png", thumbnail=True).exists() assert records.get("img.png").image_name == "img.png" assert _staging_dirs(storage) == [] + + +class TestFailedSaveCleanup: + """A save that fails halfway must clean up record-first, like every other delete path. + + The order is load-bearing: a concurrent deleter rolling back decides whether to restore an + image's files by asking whether its record is still there, so purging files while the record + survives would tell it to put them back and strand them (adversarial review, PR #9361). + """ + + def test_the_record_is_deleted_before_the_files_are_purged(self, image_service: ImageService) -> None: + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_files.save.side_effect = ImageFileSaveException() + order: list[str] = [] + token = object() + + def journal(images): + order.append("journal") + return token + + invoker.services.image_files.begin_delete.side_effect = journal + invoker.services.image_records.delete.side_effect = lambda name: order.append("record") + invoker.services.image_files.commit_delete.side_effect = lambda t, image_names=None: order.append("purge") + + with pytest.raises(ImageFileSaveException): + image_service.create( + image=Image.new("RGB", (8, 8)), + image_origin=ResourceOrigin.INTERNAL, + image_category=ImageCategory.GENERAL, + ) + + assert order == ["journal", "record", "purge"] + invoker.services.image_files.commit_delete.assert_called_once_with(token) + + def test_a_surviving_record_keeps_its_files(self, image_service: ImageService) -> None: + """If the record cannot be deleted the image is still referenced, so nothing may be purged.""" + invoker = image_service._ImageService__invoker # type: ignore + invoker.services.image_files.save.side_effect = ImageFileSaveException() + invoker.services.image_records.delete.side_effect = ImageRecordDeleteException() + + with pytest.raises(ImageFileSaveException): + image_service.create( + image=Image.new("RGB", (8, 8)), + image_origin=ResourceOrigin.INTERNAL, + image_category=ImageCategory.GENERAL, + ) + + invoker.services.image_files.commit_delete.assert_not_called() + invoker.services.image_files.delete.assert_not_called() + invoker.services.image_files.abandon_delete.assert_called_once_with( + invoker.services.image_files.begin_delete.return_value + ) + + +class TestConcurrentBoardDeleteAgainstRealRecords: + """delete_images_on_board() still stages, because its per-item contract needs a pre-flight move. + Two of them racing for one image must not strand it (adversarial review, PR #9361).""" + + def test_committing_an_empty_token_still_purges_the_files(self, wired, monkeypatch) -> None: + """The loser moved the files aside; the winner staged nothing and removed the record. + + The winner's commit is the only thing standing between the loser's restore and a permanent + orphan: by the time the loser rolls back, the record is still there, so its own re-check + tells it to keep the files it just put back. + """ + svc, records, storage = wired + invoker = svc._ImageService__invoker # type: ignore + _seed_record(records, "img.png", is_intermediate=False) + _save_image_file(storage, "img.png") + invoker.services.board_image_records.get_all_board_image_names_for_board.return_value = ["img.png"] + + # The competing request wins the race to the files, so this delete stages an empty token. + competing = storage.stage_delete("img.png", "") + real_delete_many = records.delete_many + + def competitor_rolls_back_then_delete(image_names: list[str]) -> None: + # The competing request's own record deletion failed, so it restores the files — while + # this record is still present, which is what makes its re-check keep them. + storage.rollback_delete(competing) + real_delete_many(image_names) + + monkeypatch.setattr(records, "delete_many", competitor_rolls_back_then_delete) + + deleted, failed = svc.delete_images_on_board("board-1") + + assert deleted == ["img.png"] + assert failed == [] + with pytest.raises(ImageRecordNotFoundException): + records.get("img.png") + assert not storage.get_path("img.png").exists() + assert not storage.get_path("img.png", thumbnail=True).exists() + assert _staging_dirs(storage) == [] From cdf8458ab68355f374060f05c503c52584d29dd1 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 29 Aug 2026 18:32:46 -0400 Subject: [PATCH 6/6] fix(images): re-check the record after a recovery restore; 404 a delete that lost the race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup recovery restored a staged image's files after a single `exists()` check and then dropped the journal. Another Invoke sharing the output folder could delete the record while the files sat staged — its purge finds nothing — and the restore then stranded the files with no record and no journal to find them by. Recovery now re-checks the record after a restore, exactly as rollback_delete() already does, and purges when it is gone. A record-store fault on the re-check keeps the journal, like every other lookup in the recovery loop. The delete route answered 500 when another request deleted the image between its DTO lookup and the service call. The image is gone, which is what the client asked for; it now answers 404 the way the lookup would have. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SfBJJqxqmM3b6fKE1FiYGt --- invokeai/app/api/routers/images.py | 4 ++ .../services/image_files/image_files_disk.py | 16 +++++- .../app/services/images/images_default.py | 3 ++ tests/app/routers/test_images.py | 27 ++++++++++ .../image_files/test_image_files_disk.py | 53 +++++++++++++++++++ 5 files changed, 101 insertions(+), 2 deletions(-) diff --git a/invokeai/app/api/routers/images.py b/invokeai/app/api/routers/images.py index 52c29e7a1ae..e192dfd905a 100644 --- a/invokeai/app/api/routers/images.py +++ b/invokeai/app/api/routers/images.py @@ -248,6 +248,10 @@ def delete_image( board_id = image_dto.board_id or "none" try: ApiDependencies.invoker.services.images.delete(image_name) + except ImageRecordNotFoundException: + # Another request deleted the image between the lookup above and the service call. The + # image is gone, which is what the client asked for — answer as the lookup would have. + raise HTTPException(status_code=404, detail="Image not found") except Exception: raise HTTPException(status_code=500, detail="Failed to delete image") diff --git a/invokeai/app/services/image_files/image_files_disk.py b/invokeai/app/services/image_files/image_files_disk.py index 0d555f3ef72..2c6ab8286cc 100644 --- a/invokeai/app/services/image_files/image_files_disk.py +++ b/invokeai/app/services/image_files/image_files_disk.py @@ -482,17 +482,29 @@ def __recover_pending_deletes(self) -> None: continue with open(manifest_path, encoding="utf-8") as manifest: data = json.load(manifest) + records = self.__invoker.services.image_records for image_name, image_subfolder in self.__manifest_images(data): - if self.__invoker.services.image_records.exists(image_name): + if records.exists(image_name): # Put back whatever stage_delete() moved aside. Only a single-image journal # ever holds staged files, at indices 0 and 1; a pending-delete journal # moves nothing, so these lookups simply find nothing to restore. + restored = False for index, source in enumerate(self.__delete_candidates(image_name, image_subfolder)): staged = journal_dir / str(index) if staged.exists(): source.parent.mkdir(parents=True, exist_ok=True) staged.replace(source) - continue + restored = True + # Re-check after a restore, for the same reason rollback_delete() does: + # another Invoke sharing this output folder may have deleted the record + # while the files sat staged, and its own purge found nothing to remove. + # Every deleter purges only after its record is committed as gone, so a + # record still present cannot have been purged before this restore, and a + # record now absent means the files must go — this journal is the last + # thing that can find them. A record-store fault here propagates and keeps + # the journal, like every other lookup in this loop. + if not restored or records.exists(image_name): + continue self.__purge_files(image_name, image_subfolder) shutil.rmtree(journal_dir, ignore_errors=True) except Exception as error: diff --git a/invokeai/app/services/images/images_default.py b/invokeai/app/services/images/images_default.py index d5069c62b4a..8913d8470f7 100644 --- a/invokeai/app/services/images/images_default.py +++ b/invokeai/app/services/images/images_default.py @@ -346,6 +346,9 @@ def delete(self, image_name: str): # behind and startup recovery purges the leftover files. self.__invoker.services.logger.error(f"Failed to purge deleted image files: {cleanup_error}") self._on_deleted(image_name) + except ImageRecordNotFoundException: + # Already deleted by another request; nothing here failed, so nothing to log. + raise except ImageRecordDeleteException: self.__invoker.services.logger.error("Failed to delete image record") raise diff --git a/tests/app/routers/test_images.py b/tests/app/routers/test_images.py index 17802645e2e..ba4bc84bfa9 100644 --- a/tests/app/routers/test_images.py +++ b/tests/app/routers/test_images.py @@ -299,6 +299,33 @@ def test_delete_image_not_found_returns_404( assert response.json()["detail"] == "Image not found" +def test_delete_image_deleted_mid_request_returns_404( + monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient +) -> None: + """An image deleted between the DTO lookup and the service delete is gone, not a server fault. + + Answering 500 here sent the client a failure toast for a postcondition that already held + (JPPhoto, PR #9361 round 4). + """ + storage = prepare_delete_image_test(monkeypatch, mock_invoker, tmp_path) + _save_deletable_image(mock_invoker, storage, "del.png") + real_get_dto = mock_invoker.services.images.get_dto + + def get_dto_then_lose_the_race(image_name: str): + dto = real_get_dto(image_name) + # Another request completes its delete before this one reaches the service. + mock_invoker.services.image_records.delete(image_name) + return dto + + monkeypatch.setattr(mock_invoker.services.images, "get_dto", get_dto_then_lose_the_race) + + response = client.delete("/api/v1/images/i/del.png") + + assert response.status_code == 404 + assert response.json()["detail"] == "Image not found" + assert list(storage.image_root.glob(".delete_*")) == [] + + def test_delete_image_lookup_failure_returns_500_not_404( monkeypatch: Any, mock_invoker: Invoker, tmp_path: Path, client: TestClient ) -> None: diff --git a/tests/app/services/image_files/test_image_files_disk.py b/tests/app/services/image_files/test_image_files_disk.py index d50442e641a..9aaa8a57a3b 100644 --- a/tests/app/services/image_files/test_image_files_disk.py +++ b/tests/app/services/image_files/test_image_files_disk.py @@ -403,6 +403,59 @@ def test_startup_leaves_the_journal_when_the_record_store_is_unreadable(self, di assert list(disk_storage.image_root.glob(".delete_*")) + def test_startup_purges_restored_files_whose_record_vanished_during_recovery( + self, disk_storage: DiskImageFileStorage + ): + """Recovery re-checks the record after restoring, exactly as rollback_delete() does. + + Another Invoke sharing the output folder can delete the record while the files sit staged; + its purge finds nothing, and restoring them afterwards would strand them with no record and + no journal left to find them by (JPPhoto, PR #9361 round 4). + """ + image_name = "raced.png" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=image_name) + disk_storage.stage_delete(image_name) + + invoker = MagicMock() + # Present when recovery first looks, gone by the time the files are back. + invoker.services.image_records.exists.side_effect = [True, False] + restarted = DiskImageFileStorage(disk_storage.image_root) + restarted.start(invoker) + + assert not disk_storage.get_path(image_name).exists() + assert not disk_storage.get_path(image_name, thumbnail=True).exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + + def test_startup_keeps_the_journal_when_the_recheck_cannot_read_the_record_store( + self, disk_storage: DiskImageFileStorage + ): + """A fault on the post-restore re-check keeps the files and the journal for the next start.""" + image_name = "kept.png" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name=image_name) + disk_storage.stage_delete(image_name) + + invoker = MagicMock() + invoker.services.image_records.exists.side_effect = [True, RuntimeError("database is locked")] + restarted = DiskImageFileStorage(disk_storage.image_root) + restarted.start(invoker) + + assert disk_storage.get_path(image_name).exists() + assert disk_storage.get_path(image_name, thumbnail=True).exists() + assert list(disk_storage.image_root.glob(".delete_*")) + + def test_startup_does_not_recheck_a_pending_journal_that_restored_nothing(self, disk_storage: DiskImageFileStorage): + """Only a restore can strand files; a journal that moved nothing costs one lookup.""" + disk_storage.save(image=Image.new("RGB", (32, 32)), image_name="pending.png") + disk_storage.begin_delete([("pending.png", "")]) + + invoker = MagicMock() + invoker.services.image_records.exists.side_effect = [True, AssertionError("unexpected re-check")] + restarted = DiskImageFileStorage(disk_storage.image_root) + restarted.start(invoker) + + assert disk_storage.get_path("pending.png").exists() + assert not list(disk_storage.image_root.glob(".delete_*")) + class TestPendingDeleteJournal: """begin_delete() writes the journal that makes records-first deletion recoverable.