diff --git a/routes/gallery_routes.py b/routes/gallery_routes.py index 826e16742..c641912dc 100644 --- a/routes/gallery_routes.py +++ b/routes/gallery_routes.py @@ -931,15 +931,23 @@ def setup_gallery_routes() -> APIRouter: raise HTTPException(404, "Image not found") img_filename = img.filename - # Remove the file from disk - img_path = _gallery_image_path(img_filename) - if img_path.exists(): - img_path.unlink() - - # Soft-delete the record + # Soft-delete the record first; the DB is the source of truth. img.is_active = False db.commit() + # Only after the soft-delete commit succeeds do we remove the file. + # If the file were deleted first and the commit then failed/rolled + # back, the still-active record would point at a missing file. + # Best-effort so a missing or locked file can't 500 a delete that + # already succeeded logically. Uses the path-confined resolver so a + # malformed stored filename can't escape generated_images. + try: + img_path = _gallery_image_path(img_filename) + if img_path.exists(): + img_path.unlink() + except Exception as e: + logger.warning(f"Could not remove gallery image file for {img_filename}: {e}") + # Strip stale chat-history references so the image bubble # (and its prompt caption) doesn't come back after a server # reboot replays the session. We remove the matching tool diff --git a/tests/test_gallery_delete_file_ordering.py b/tests/test_gallery_delete_file_ordering.py new file mode 100644 index 000000000..03e0ef73e --- /dev/null +++ b/tests/test_gallery_delete_file_ordering.py @@ -0,0 +1,83 @@ +"""Regression: deleting a gallery image must not remove the file before the DB +commit succeeds. + +delete_gallery_image() removed the on-disk file first and only then set +is_active=False and committed. If that commit failed and rolled back, the record +stayed active but its file was already gone — a broken, unviewable image (data +loss). The file is now removed only after the soft-delete commit succeeds, and +best-effort so a missing/locked file can't fail an otherwise-successful delete. +""" +import asyncio + +import pytest +from fastapi import HTTPException, Request +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from core.database import Base, GalleryImage +import routes.gallery_routes as gallery_routes + + +def _delete_endpoint(): + router = gallery_routes.setup_gallery_routes() + for route in router.routes: + if getattr(route, "path", "") == "/api/gallery/{image_id}" and "DELETE" in getattr(route, "methods", set()): + return route.endpoint + raise AssertionError("DELETE /api/gallery/{image_id} endpoint not found") + + +def _seed(tmp_path): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(bind=engine) + SessionLocal = sessionmaker(bind=engine) + db = SessionLocal() + db.add(GalleryImage(id="img-1", filename="x.png", owner="alice", is_active=True)) + db.commit() + db.close() + img_dir = tmp_path / "data" / "generated_images" + img_dir.mkdir(parents=True) + (img_dir / "x.png").write_bytes(b"image-bytes") + return SessionLocal + + +def test_file_kept_when_commit_fails(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + SessionLocal = _seed(tmp_path) + monkeypatch.setattr(gallery_routes, "get_current_user", lambda r: "alice") + + # A session whose commit always fails, to simulate a DB error mid-delete. + sess = SessionLocal() + + def _boom(): + raise RuntimeError("commit failed") + + monkeypatch.setattr(sess, "commit", _boom) + monkeypatch.setattr(gallery_routes, "SessionLocal", lambda: sess) + + delete = _delete_endpoint() + with pytest.raises(HTTPException): + asyncio.run(delete(Request(scope={"type": "http"}), "img-1")) + + # File must survive a failed commit — the record is still active after rollback. + assert (tmp_path / "data" / "generated_images" / "x.png").exists() + check = SessionLocal() + row = check.query(GalleryImage).filter(GalleryImage.id == "img-1").first() + assert row.is_active is True + check.close() + + +def test_file_removed_on_successful_delete(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + SessionLocal = _seed(tmp_path) + monkeypatch.setattr(gallery_routes, "get_current_user", lambda r: "alice") + monkeypatch.setattr(gallery_routes, "SessionLocal", SessionLocal) + + delete = _delete_endpoint() + result = asyncio.run(delete(Request(scope={"type": "http"}), "img-1")) + + assert result["status"] == "deleted" + assert not (tmp_path / "data" / "generated_images" / "x.png").exists() + check = SessionLocal() + row = check.query(GalleryImage).filter(GalleryImage.id == "img-1").first() + assert row.is_active is False + check.close()