From 878a45371e30f649c68e791abd12671a0bed3b93 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Sat, 15 Aug 2026 14:23:56 -0400 Subject: [PATCH 1/2] feat: guard the paths that assume an album entry is a still image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six places would misbehave once the directory walk starts collecting videos. Each is fixed and pinned here, ahead of the flip. find_duplicate_clusters now excludes videos. A video's embedding describes one extracted frame, and opening frames are so often a black slate or a title card that two unrelated clips would sit at ~1.0 cosine and be reported as duplicates — which users act on by deleting. The owner had already excluded videos from dedupe as a product decision; nothing enforced it. _load_image_path raises 400 for videos. It is the choke point for /invokeai/use_ref_image, which POSTs the file to InvokeAI's /images/upload; an .mp4 would come back as an opaque 502 carrying raw upstream text. The drawer already withholds the button for videos, so this closes the API path behind it. (Recall reaches videos via the metadata path and already refused them; a test pins that too.) search_with_text_and_image returns 400 rather than 500 when the query blob is not a decodable still — a video dropped on the search panel used to surface as an opaque error from deep inside PIL. download_images_zip stores video members instead of deflating them (their streams are already compressed) and refuses selections over 2 GB. The archive is assembled entirely in memory, so twenty bookmarked 200 MB clips would have been several gigabytes resident — an OOM only reachable once videos exist. index_metadata reports image_count and video_count alongside filename_count, so the album card can explain why the number jumped rather than leaving the user to guess. Legacy indexes report video_count == 0. progress_tracker gains add_completion_warning. The completion warning was a single slot already used for the board-album "N of M missing" notice, so a video-skip warning would have silently discarded it — a run can legitimately produce both. bookmarks.js downloads a video by pointing an straight at the URL instead of buffering it into a blob, and derives its fallback extension from the real path rather than hardcoding .jpg. Tests: 14 new in test_video_guards.py. Backend 590 passed, frontend 501 passed, ruff and eslint clean. Co-Authored-By: Claude Opus 5 (1M context) --- photomap/backend/embeddings.py | 14 +- photomap/backend/progress.py | 19 + photomap/backend/routers/index.py | 13 +- photomap/backend/routers/invoke.py | 14 +- photomap/backend/routers/search.py | 50 ++- .../frontend/static/javascript/bookmarks.js | 22 +- tests/backend/test_video_guards.py | 346 ++++++++++++++++++ 7 files changed, 470 insertions(+), 8 deletions(-) create mode 100644 tests/backend/test_video_guards.py diff --git a/photomap/backend/embeddings.py b/photomap/backend/embeddings.py index 37cf6cf3..a1984af3 100644 --- a/photomap/backend/embeddings.py +++ b/photomap/backend/embeddings.py @@ -41,7 +41,7 @@ capture_download_progress, get_cached_encoder, ) -from .media_types import IMAGE_EXTENSIONS +from .media_types import IMAGE_EXTENSIONS, is_video from .metadata_extraction import MetadataExtractor from .metadata_formatting import format_metadata from .metadata_modules import SlideSummary @@ -1857,6 +1857,18 @@ def find_duplicate_clusters(self, similarity_threshold=0.995): embeddings = data["embeddings"] filenames = data["filenames"] + # Videos are excluded from duplicate detection. Their embedding + # describes one extracted frame, and opening frames are frequently a + # black slate or a title card — two unrelated clips would then sit at + # ~1.0 cosine and be reported as duplicates, which users act on by + # deleting. Their frames can also legitimately duplicate a photo. + keep = np.array([not is_video(Path(str(f))) for f in filenames], dtype=bool) + if not keep.all(): + embeddings = embeddings[keep] + filenames = filenames[keep] + if len(embeddings) == 0: + return + # Normalize embeddings. ``_l2_normalize`` carries an epsilon guard so # an all-zero row can't produce NaN here. norm_embeddings = _l2_normalize(embeddings, axis=-1) diff --git a/photomap/backend/progress.py b/photomap/backend/progress.py index 38171f0f..ddd31dbd 100644 --- a/photomap/backend/progress.py +++ b/photomap/backend/progress.py @@ -199,6 +199,9 @@ def set_completion_warning(self, album_key: str, message: str | None) -> None: and is folded in atomically by ``complete_operation``. A falsy ``message`` clears any pending notice so a clean re-run doesn't inherit a stale one. + + Replaces whatever was pending. Use :meth:`add_completion_warning` to + contribute an additional notice without discarding an existing one. """ with self._lock: if message: @@ -206,6 +209,22 @@ def set_completion_warning(self, album_key: str, message: str | None) -> None: else: self._completion_warnings.pop(album_key, None) + def add_completion_warning(self, album_key: str, message: str | None) -> None: + """Append a notice, keeping any already pending for this album. + + A run can now produce more than one: a board album may have images + missing on disk *and* videos that could not be decoded. This used to + be a single slot, so the second writer silently discarded the first. + """ + if not message: + return + with self._lock: + existing = self._completion_warnings.get(album_key) + if existing and message not in existing: + self._completion_warnings[album_key] = f"{existing} {message}" + elif not existing: + self._completion_warnings[album_key] = message + def get_progress(self, album_key: str) -> ProgressInfo | None: """Get progress info for an album.""" with self._lock: diff --git a/photomap/backend/routers/index.py b/photomap/backend/routers/index.py index 71f5b417..fd5bcc72 100644 --- a/photomap/backend/routers/index.py +++ b/photomap/backend/routers/index.py @@ -18,6 +18,7 @@ from .. import invokeai_client from ..config import get_config_manager from ..embeddings import LAST_UPDATED_FILENAME, Embeddings, peek_encoder_spec +from ..media_types import is_video from ..progress import IndexingCancelled, progress_tracker from .album import ( AlbumDep, @@ -69,6 +70,12 @@ class EmbeddingsIndexMetadata(BaseModel): filename_count: int embeddings_path: str last_modified: float + # Broken out so the album card can say "120 images, 4 videos" rather than + # leaving the user to wonder why the single count jumped. Derived from the + # filename suffixes, so indexes written before video support report + # image_count == filename_count and video_count == 0. + image_count: int = 0 + video_count: int = 0 # Note: How check_album_lock is used in this file: @@ -275,12 +282,16 @@ async def index_metadata(album_config: AlbumDep) -> EmbeddingsIndexMetadata: marker = index_path.parent / LAST_UPDATED_FILENAME if marker.exists(): last_modified = max(last_modified, marker.stat().st_mtime) - filename_count = len(Embeddings.open_cached_embeddings(index_path)["filenames"]) + filenames = Embeddings.open_cached_embeddings(index_path)["filenames"] + filename_count = len(filenames) + video_count = sum(1 for f in filenames if is_video(Path(str(f)))) return EmbeddingsIndexMetadata( filename_count=filename_count, embeddings_path=str(index_path), last_modified=last_modified, + image_count=filename_count - video_count, + video_count=video_count, ) diff --git a/photomap/backend/routers/invoke.py b/photomap/backend/routers/invoke.py index 2e554b51..feeb1d30 100644 --- a/photomap/backend/routers/invoke.py +++ b/photomap/backend/routers/invoke.py @@ -42,6 +42,7 @@ _request_with_auth_fallback, _validate_invokeai_url, ) +from ..media_types import is_video from ..metadata_modules.invoke.invoke_metadata_view import InvokeMetadataView from ..metadata_modules.invokemetadata import GenerationMetadataAdapter from .album import get_embeddings_for_album, require_no_lock @@ -494,7 +495,18 @@ def _load_image_path(album_key: str, index: int) -> Path: filenames = indexes["sorted_filenames"] if index < 0 or index >= len(filenames): raise HTTPException(status_code=404, detail="Index out of range") - return Path(str(filenames[index])) + path = Path(str(filenames[index])) + # The single choke point for both /recall and /use_ref_image, which upload + # the file to InvokeAI as a reference image. InvokeAI's upload endpoint + # takes images, so handing it an .mp4 would surface as an opaque 502 + # carrying raw upstream text. The drawer already withholds the button for + # videos; this closes the API path behind it. + if is_video(path): + raise HTTPException( + status_code=400, + detail="InvokeAI actions are not available for video files.", + ) + return path def _build_recall_payload(raw_metadata: dict, include_seed: bool) -> dict: diff --git a/photomap/backend/routers/search.py b/photomap/backend/routers/search.py index e13d7b70..3855d4c6 100644 --- a/photomap/backend/routers/search.py +++ b/photomap/backend/routers/search.py @@ -47,6 +47,10 @@ _MAX_THUMB_SIZE = 2048 _MAX_THUMB_RADIUS = 512 +# ``download_images_zip`` builds its archive in memory. Videos make it easy to +# ask for far more than fits, so cap the total selection size. +_MAX_ZIP_BYTES = 2_000_000_000 + # Response Models class SearchResult(BaseModel): @@ -104,8 +108,21 @@ async def search_with_text_and_image( try: # If image_data is provided, decode and save to temp file if req.image_data: - image_bytes = base64.b64decode(req.image_data.split(",")[-1]) - query_image_data = Image.open(BytesIO(image_bytes)) + # A query blob that isn't a still image — a video file dropped on + # the search panel, say — used to surface as an opaque 500 from + # deep inside PIL. The encoder only takes stills. + try: + image_bytes = base64.b64decode(req.image_data.split(",")[-1]) + query_image_data = Image.open(BytesIO(image_bytes)) + query_image_data.load() + except HTTPException: + raise + except Exception as e: + logger.info(f"Rejected an unreadable search query image: {e}") + raise HTTPException( + status_code=400, + detail="The query image could not be read. Search by image needs a still image.", + ) from e logger.info( f"Search request: {req.min_search_score=}, {req.max_search_results=}" @@ -531,6 +548,28 @@ async def download_images_zip( """ Download multiple images as a ZIP file. """ + # The archive is assembled entirely in memory, which was fine for photos + # but is not for video: twenty bookmarked 200 MB clips would be several + # gigabytes resident. Refuse above a ceiling rather than exhausting the + # server. + total_bytes = 0 + for index in req.indices: + try: + candidate = embeddings.get_image_path(index) + if candidate.is_file(): + total_bytes += candidate.stat().st_size + except Exception: + continue + if total_bytes > _MAX_ZIP_BYTES: + raise HTTPException( + status_code=413, + detail=( + f"That selection is {total_bytes / 1_000_000_000:.1f} GB, over the " + f"{_MAX_ZIP_BYTES // 1_000_000_000} GB download limit. " + "Select fewer files, or copy them to a folder instead." + ), + ) + # Create ZIP file in memory zip_buffer = BytesIO() with zipfile.ZipFile(zip_buffer, "w", zipfile.ZIP_DEFLATED) as zip_file: @@ -543,8 +582,13 @@ async def download_images_zip( if not image_path.exists() or not image_path.is_file(): logger.warning(f"Image not found at index {index}") continue + # Video containers hold already-compressed streams, so + # deflating them burns CPU for no gain. Store them instead. + compression = ( + zipfile.ZIP_STORED if is_video(image_path) else zipfile.ZIP_DEFLATED + ) # Add file to ZIP with just the filename (not full path) - zip_file.write(image_path, image_path.name) + zip_file.write(image_path, image_path.name, compress_type=compression) except Exception as e: logger.warning(f"Error adding image at index {index} to ZIP: {e}") continue diff --git a/photomap/frontend/static/javascript/bookmarks.js b/photomap/frontend/static/javascript/bookmarks.js index 0baa8467..862607ef 100644 --- a/photomap/frontend/static/javascript/bookmarks.js +++ b/photomap/frontend/static/javascript/bookmarks.js @@ -509,8 +509,26 @@ class BookmarkManager { async downloadSingleImage(globalIndex) { const data = await fetchJson(`retrieve_image/${encodeURIComponent(state.album)}/${globalIndex}`); - const imageUrl = data.image_url; - const filename = data.filename || `image_${globalIndex}.jpg`; + const isVideo = data.media_type === "video"; + // For a video, download the playable file rather than its still frame. + const imageUrl = isVideo && data.video_url ? data.video_url : data.image_url; + // Derive the fallback extension from the real path — the old hardcoded + // .jpg would save a video under a name no player would open. + const fallbackExtension = data.filepath?.split(".").pop() || (isVideo ? "mp4" : "jpg"); + const filename = data.filename || `image_${globalIndex}.${fallbackExtension}`; + + if (isVideo) { + // Videos are far too large to buffer into a blob: a 200 MB clip would + // sit entirely in browser memory before the save dialog appeared. Point + // the download straight at the URL and let the browser stream it. + const a = document.createElement("a"); + a.href = imageUrl; + a.download = filename; + document.body.appendChild(a); + a.click(); + document.body.removeChild(a); + return; + } // Fetch the actual image (binary, not JSON — fetch directly) const imageResponse = await fetch(imageUrl); diff --git a/tests/backend/test_video_guards.py b/tests/backend/test_video_guards.py new file mode 100644 index 00000000..0308fff7 --- /dev/null +++ b/tests/backend/test_video_guards.py @@ -0,0 +1,346 @@ +"""Guards for the paths that assume an album entry is a still image. + +Each of these is somewhere a video would otherwise misbehave once indexing +picks them up: duplicate detection would report unrelated clips as identical, +the InvokeAI actions would upload an .mp4 as a reference image, a video blob +posted to search would 500, and the in-memory ZIP would happily try to hold +several gigabytes. +""" + +from __future__ import annotations + +import base64 +import shutil +from io import BytesIO +from pathlib import Path + +import numpy as np +import pytest +from fixtures import media_fixture_path +from PIL import Image + +from photomap.backend.embeddings import Embeddings, _open_npz_file +from photomap.backend.progress import ProgressTracker +from photomap.backend.util import atomic_savez + +ENCODER_SPEC = "openai-clip:ViT-B/32" +EMBEDDING_DIM = 8 + + +def _write_index(index_path: Path, files: list[Path], embeddings: np.ndarray) -> None: + index_path.parent.mkdir(parents=True, exist_ok=True) + atomic_savez( + index_path, + embeddings=embeddings.astype(np.float32), + filenames=np.array([f.resolve().as_posix() for f in files]), + modification_times=np.array( + [float(i + 1) for i in range(len(files))], dtype=float + ), + metadata=np.array([{} for _ in files], dtype=object), + model_id=np.array(ENCODER_SPEC), + embedding_dim=np.array(EMBEDDING_DIM), + ) + _open_npz_file.cache_clear() + + +# -------------------------------------------------------------------------- +# Duplicate detection +# -------------------------------------------------------------------------- + + +def test_duplicate_detection_ignores_videos(tmp_path, capsys): + """Two clips whose opening frames match must not be called duplicates. + + Opening frames are so often a black slate or a title card that unrelated + videos land at ~1.0 cosine — and users act on a duplicate report by + deleting. + """ + media = tmp_path / "media" + media.mkdir() + files = [media / "a.mp4", media / "b.mp4", media / "x.jpg", media / "y.jpg"] + for f in files: + f.write_bytes(b"stub") + + # Both pairs are internally identical. + identical = np.array([[1.0, 0.0] + [0.0] * 6]) + embeddings = np.vstack([identical, identical, identical, identical]) + + index_path = media / "index" / "embeddings.npz" + _write_index(index_path, files, embeddings) + + Embeddings(embeddings_path=index_path, encoder_spec=ENCODER_SPEC).find_duplicate_clusters() + + printed = capsys.readouterr().out + assert "x.jpg" in printed and "y.jpg" in printed + assert "a.mp4" not in printed and "b.mp4" not in printed + + +def test_duplicate_detection_on_an_all_video_index_is_a_noop(tmp_path, capsys): + media = tmp_path / "media" + media.mkdir() + files = [media / "a.mp4", media / "b.mp4"] + for f in files: + f.write_bytes(b"stub") + embeddings = np.vstack([np.array([[1.0] + [0.0] * 7])] * 2) + + index_path = media / "index" / "embeddings.npz" + _write_index(index_path, files, embeddings) + + Embeddings(embeddings_path=index_path, encoder_spec=ENCODER_SPEC).find_duplicate_clusters() + + assert capsys.readouterr().out.strip() == "" + + +# -------------------------------------------------------------------------- +# Search by image +# -------------------------------------------------------------------------- + + +def test_search_rejects_a_non_image_query_blob(client, new_album): + """A video dropped on the search panel gets a 400, not an opaque 500.""" + payload = base64.b64encode(media_fixture_path("clip.mp4").read_bytes()).decode() + + response = client.post( + f"/search_with_text_and_image/{new_album['key']}", + json={ + "image_data": f"data:video/mp4;base64,{payload}", + "positive_query": "", + "negative_query": "", + }, + ) + + assert response.status_code == 400 + assert "still image" in response.json()["detail"].lower() + + +def test_search_still_accepts_an_image_query(client, new_album): + buffer = BytesIO() + Image.new("RGB", (64, 64), (10, 20, 30)).save(buffer, format="JPEG") + payload = base64.b64encode(buffer.getvalue()).decode() + + response = client.post( + f"/search_with_text_and_image/{new_album['key']}", + json={ + "image_data": f"data:image/jpeg;base64,{payload}", + "positive_query": "", + "negative_query": "", + }, + ) + + assert response.status_code != 400 + + +# -------------------------------------------------------------------------- +# InvokeAI actions +# -------------------------------------------------------------------------- + + +@pytest.fixture +def video_album(client, tmp_path): + media = tmp_path / "invoke_media" + media.mkdir() + video = media / "clip.mp4" + shutil.copy(media_fixture_path("clip.mp4"), video) + + index_path = media / "index" / "embeddings.npz" + _write_index(index_path, [video], np.array([[1.0] + [0.0] * 7])) + + album = { + "key": "invoke_video_album", + "name": "Invoke Video Album", + "image_paths": [media.as_posix()], + "index": index_path.as_posix(), + "umap_eps": 0.1, + "description": "", + "encoder_spec": ENCODER_SPEC, + } + assert client.post("/add_album/", json=album).status_code == 201 + try: + yield album + finally: + client.delete(f"/delete_album/{album['key']}") + + +@pytest.fixture +def invokeai_configured(): + """Point the config at an InvokeAI URL that is never actually reached. + + The endpoints refuse early without a configured URL, which would mask the + guard under test. + """ + from photomap.backend.config import get_config_manager + + manager = get_config_manager() + manager.set_invokeai_settings(url="http://localhost:9090") + yield + manager.set_invokeai_settings(url=None, username=None, password=None) + + +def test_use_ref_image_refuses_videos(client, video_album, invokeai_configured): + """``_load_image_path`` is the choke point for the InvokeAI upload path. + + This endpoint POSTs the file to InvokeAI's /images/upload, which takes + images — an .mp4 would surface as an opaque 502 carrying raw upstream + text. The guard has to fire before any network call is attempted, which + is what makes this test safe to run with an unreachable URL configured. + """ + response = client.post( + "/invokeai/use_ref_image", + json={"album_key": video_album["key"], "index": 0}, + ) + assert response.status_code == 400 + assert "video" in response.json()["detail"].lower() + + +def test_recall_also_refuses_videos(client, video_album, invokeai_configured): + """Recall reaches videos through the metadata path rather than the file. + + It therefore already refused them — a video carries no InvokeAI + generation parameters — but the behaviour is worth pinning so a future + refactor that routes recall through the file path stays correct. + """ + response = client.post( + "/invokeai/recall", + json={"album_key": video_album["key"], "index": 0}, + ) + assert response.status_code == 400 + + +# -------------------------------------------------------------------------- +# ZIP download +# -------------------------------------------------------------------------- + + +def test_zip_download_refuses_an_oversized_selection( + client, new_album, monkeypatch +): + """The archive is built in memory, so the ceiling has to be enforced.""" + from photomap.backend.routers import search as search_router_module + + monkeypatch.setattr(search_router_module, "_MAX_ZIP_BYTES", 10) + + from fixtures import build_index + + build_index(client, new_album) + + response = client.post( + f"/download_images_zip/{new_album['key']}", json={"indices": [0, 1]} + ) + + assert response.status_code == 413 + assert "limit" in response.json()["detail"].lower() + + +def test_zip_download_still_works_under_the_limit(client, new_album): + from fixtures import build_index + + build_index(client, new_album) + + response = client.post( + f"/download_images_zip/{new_album['key']}", json={"indices": [0]} + ) + + assert response.status_code == 200 + assert response.headers["content-type"] == "application/zip" + + +# -------------------------------------------------------------------------- +# Index metadata +# -------------------------------------------------------------------------- + + +def test_index_metadata_splits_images_and_videos(client, tmp_path): + media = tmp_path / "counted" + media.mkdir() + files = [media / "a.mp4", media / "b.jpg", media / "c.jpg"] + for f in files: + f.write_bytes(b"stub") + _write_index( + media / "index" / "embeddings.npz", + files, + np.vstack([np.array([[1.0] + [0.0] * 7])] * 3), + ) + + album = { + "key": "counted_album", + "name": "Counted", + "image_paths": [media.as_posix()], + "index": (media / "index" / "embeddings.npz").as_posix(), + "umap_eps": 0.1, + "description": "", + "encoder_spec": ENCODER_SPEC, + } + assert client.post("/add_album/", json=album).status_code == 201 + try: + payload = client.get("/index_metadata/counted_album").json() + assert payload["filename_count"] == 3 + assert payload["image_count"] == 2 + assert payload["video_count"] == 1 + finally: + client.delete("/delete_album/counted_album") + + +def test_index_metadata_reports_legacy_indexes_as_all_images(client, new_album): + from fixtures import build_index + + build_index(client, new_album) + payload = client.get(f"/index_metadata/{new_album['key']}").json() + assert payload["video_count"] == 0 + assert payload["image_count"] == payload["filename_count"] + + +# -------------------------------------------------------------------------- +# Completion warnings +# -------------------------------------------------------------------------- + + +def test_completion_warnings_compose(): + """A run can now produce more than one non-fatal notice. + + A board album may have images missing on disk *and* videos that could not + be decoded; the single slot used to let the second writer discard the + first silently. + """ + tracker = ProgressTracker() + tracker.set_completion_warning("album", "3 images missing on disk.") + tracker.add_completion_warning("album", "2 videos could not be read.") + + tracker.start_operation("album", total_images=1, operation_type="indexing") + tracker.complete_operation("album") + + warning = tracker.get_progress("album").warning_message + assert "3 images missing on disk." in warning + assert "2 videos could not be read." in warning + + +def test_adding_the_same_warning_twice_does_not_duplicate_it(): + tracker = ProgressTracker() + tracker.add_completion_warning("album", "2 videos could not be read.") + tracker.add_completion_warning("album", "2 videos could not be read.") + + tracker.start_operation("album", total_images=1, operation_type="indexing") + tracker.complete_operation("album") + + assert tracker.get_progress("album").warning_message.count("2 videos") == 1 + + +def test_adding_an_empty_warning_is_a_noop(): + tracker = ProgressTracker() + tracker.add_completion_warning("album", "kept") + tracker.add_completion_warning("album", None) + + tracker.start_operation("album", total_images=1, operation_type="indexing") + tracker.complete_operation("album") + + assert tracker.get_progress("album").warning_message == "kept" + + +def test_set_completion_warning_still_replaces(): + tracker = ProgressTracker() + tracker.set_completion_warning("album", "first") + tracker.set_completion_warning("album", "second") + + tracker.start_operation("album", total_images=1, operation_type="indexing") + tracker.complete_operation("album") + + assert tracker.get_progress("album").warning_message == "second" From b9ff05d4a9fa6c328ce2c847eff0af0a29d51791 Mon Sep 17 00:00:00 2001 From: Lincoln Stein Date: Mon, 17 Aug 2026 06:52:21 -0400 Subject: [PATCH 2/2] fix: address adversarial review of the video backend guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The positive search test could not fail. test_search_still_accepts_an_ image_query never built an index, so the endpoint 500s on the missing .npz — and the assertion was `!= 400`, which a 500 satisfies. The test would have held even if the new guard rejected every valid image, which is the one thing it was there to rule out. It now builds the index and asserts 200 plus a non-empty result set. add_completion_warning silently dropped distinct notices. Dedupe tested `message not in existing` against the accumulated text, so "2 videos could not be read." is a substring of "12 videos could not be read." and was discarded — exactly the silent discard the method was added to prevent. Notices are now held as a list per album and compared whole, joined with a space only when handed to the poller, so the text the user sees is unchanged. ZIP_STORED for videos was claimed but untested. compress_type is a per-member argument that can be dropped with no other visible effect; now asserted against the returned archive (and confirmed to fail when the argument is removed). The bookmarks.js change shipped with no test at all. New bookmarks-download.test.js covers the video link path (no fetch of the media, no object URL), the still-image blob path it must not disturb, and the extension fallback — including a video whose index carries no path, where the old hardcoded .jpg would have named a clip nothing can open. The ZIP size pre-pass now applies the same validate_image_access check as the loop that writes the archive, so the ceiling is measured over files that would actually be included; counting a rejected path could refuse a selection that zips to nothing. Also: the 413 message renders sub-gigabyte ceilings as MB instead of "over the 0 GB download limit", and the dead `except HTTPException` in the query-image guard is gone — nothing inside that block raises one. --- photomap/backend/progress.py | 27 ++-- photomap/backend/routers/search.py | 27 +++- tests/backend/test_video_guards.py | 105 ++++++++++++++- tests/frontend/bookmarks-download.test.js | 155 ++++++++++++++++++++++ 4 files changed, 297 insertions(+), 17 deletions(-) create mode 100644 tests/frontend/bookmarks-download.test.js diff --git a/photomap/backend/progress.py b/photomap/backend/progress.py index ddd31dbd..fcf57874 100644 --- a/photomap/backend/progress.py +++ b/photomap/backend/progress.py @@ -85,8 +85,10 @@ def __init__(self): # folded into the ProgressInfo when the run completes (see # ``complete_operation``). Kept separate from ``_progress`` because the # per-phase ``start_operation`` calls recreate ProgressInfo and would - # otherwise wipe a warning recorded earlier in the same run. - self._completion_warnings: dict[str, str] = {} + # otherwise wipe a warning recorded earlier in the same run. Held as a + # list per album so several notices can accumulate and be compared + # exactly; joined with a space when handed to the poller. + self._completion_warnings: dict[str, list[str]] = {} self._lock = threading.Lock() def start_operation(self, album_key: str, total_images: int, operation_type: str): @@ -205,7 +207,7 @@ def set_completion_warning(self, album_key: str, message: str | None) -> None: """ with self._lock: if message: - self._completion_warnings[album_key] = message + self._completion_warnings[album_key] = [message] else: self._completion_warnings.pop(album_key, None) @@ -215,15 +217,19 @@ def add_completion_warning(self, album_key: str, message: str | None) -> None: A run can now produce more than one: a board album may have images missing on disk *and* videos that could not be decoded. This used to be a single slot, so the second writer silently discarded the first. + + Repeating an identical notice is ignored. Notices are held as a list so + that comparison is against whole notices: testing ``message not in + existing`` against the joined text made "2 videos could not be read." + a substring of "12 videos could not be read." and dropped it, which is + exactly the silent discard this method exists to prevent. """ if not message: return with self._lock: - existing = self._completion_warnings.get(album_key) - if existing and message not in existing: - self._completion_warnings[album_key] = f"{existing} {message}" - elif not existing: - self._completion_warnings[album_key] = message + pending = self._completion_warnings.setdefault(album_key, []) + if message not in pending: + pending.append(message) def get_progress(self, album_key: str) -> ProgressInfo | None: """Get progress info for an album.""" @@ -277,9 +283,8 @@ def complete_operation( progress.images_processed = progress.total_images # Fold in (and consume) any pending non-fatal notice so it # lands atomically with the COMPLETED status the poller reads. - progress.warning_message = self._completion_warnings.pop( - album_key, None - ) + pending = self._completion_warnings.pop(album_key, None) + progress.warning_message = " ".join(pending) if pending else None # Global instance diff --git a/photomap/backend/routers/search.py b/photomap/backend/routers/search.py index 3855d4c6..eab84526 100644 --- a/photomap/backend/routers/search.py +++ b/photomap/backend/routers/search.py @@ -52,6 +52,19 @@ _MAX_ZIP_BYTES = 2_000_000_000 +def _format_bytes(size: int) -> str: + """Human-readable size for the download-limit message. + + Falls back to MB below a gigabyte so a lowered ceiling doesn't render as + "over the 0 GB download limit". + """ + if size >= 1_000_000_000: + return f"{size / 1_000_000_000:.1f} GB" + if size >= 1_000_000: + return f"{size / 1_000_000:.0f} MB" + return f"{size} bytes" + + # Response Models class SearchResult(BaseModel): index: int @@ -114,9 +127,10 @@ async def search_with_text_and_image( try: image_bytes = base64.b64decode(req.image_data.split(",")[-1]) query_image_data = Image.open(BytesIO(image_bytes)) + # ``open`` only reads the header; without an explicit load the + # decode failure would surface later, from inside the encoder, + # as the 500 this guard is meant to replace. query_image_data.load() - except HTTPException: - raise except Exception as e: logger.info(f"Rejected an unreadable search query image: {e}") raise HTTPException( @@ -552,11 +566,14 @@ async def download_images_zip( # but is not for video: twenty bookmarked 200 MB clips would be several # gigabytes resident. Refuse above a ceiling rather than exhausting the # server. + # Applies the same access check as the loop below, so the total only counts + # files that would actually be written. Counting a rejected path could + # refuse a selection that zips to nothing. total_bytes = 0 for index in req.indices: try: candidate = embeddings.get_image_path(index) - if candidate.is_file(): + if validate_image_access(album_config, candidate) and candidate.is_file(): total_bytes += candidate.stat().st_size except Exception: continue @@ -564,8 +581,8 @@ async def download_images_zip( raise HTTPException( status_code=413, detail=( - f"That selection is {total_bytes / 1_000_000_000:.1f} GB, over the " - f"{_MAX_ZIP_BYTES // 1_000_000_000} GB download limit. " + f"That selection is {_format_bytes(total_bytes)}, over the " + f"{_format_bytes(_MAX_ZIP_BYTES)} download limit. " "Select fewer files, or copy them to a folder instead." ), ) diff --git a/tests/backend/test_video_guards.py b/tests/backend/test_video_guards.py index 0308fff7..f8406eb0 100644 --- a/tests/backend/test_video_guards.py +++ b/tests/backend/test_video_guards.py @@ -11,6 +11,7 @@ import base64 import shutil +import zipfile from io import BytesIO from pathlib import Path @@ -114,6 +115,16 @@ def test_search_rejects_a_non_image_query_blob(client, new_album): def test_search_still_accepts_an_image_query(client, new_album): + """The guard must not cost us search-by-image. + + This has to build the index and assert a 200: without one the endpoint + 500s on the missing .npz, and an ``!= 400`` assertion passes on that 500 — + so the test would hold even if the guard rejected every valid image. + """ + from fixtures import build_index + + build_index(client, new_album) + buffer = BytesIO() Image.new("RGB", (64, 64), (10, 20, 30)).save(buffer, format="JPEG") payload = base64.b64encode(buffer.getvalue()).decode() @@ -127,7 +138,8 @@ def test_search_still_accepts_an_image_query(client, new_album): }, ) - assert response.status_code != 400 + assert response.status_code == 200, response.text + assert len(response.json()) > 0 # -------------------------------------------------------------------------- @@ -231,6 +243,78 @@ def test_zip_download_refuses_an_oversized_selection( assert "limit" in response.json()["detail"].lower() +def test_zip_stores_videos_and_deflates_images(client, tmp_path): + """Video streams are already compressed; deflating them burns CPU for ~0. + + Asserted on the archive that comes back rather than on the call, because + the compress_type is a per-member argument that is easy to drop without + any other visible effect. + """ + media = tmp_path / "zipmix" + media.mkdir() + video = media / "clip.mp4" + shutil.copy(media_fixture_path("clip.mp4"), video) + photo = media / "shot.png" + Image.new("RGB", (64, 64), (200, 40, 40)).save(photo) + + files = sorted([video, photo]) + index_path = media / "index" / "embeddings.npz" + _write_index( + index_path, files, np.vstack([np.array([[1.0] + [0.0] * 7])] * len(files)) + ) + + album = { + "key": "zipmix_album", + "name": "Zip Mix", + "image_paths": [media.as_posix()], + "index": index_path.as_posix(), + "umap_eps": 0.1, + "description": "", + "encoder_spec": ENCODER_SPEC, + } + assert client.post("/add_album/", json=album).status_code == 201 + try: + response = client.post( + "/download_images_zip/zipmix_album", json={"indices": [0, 1]} + ) + assert response.status_code == 200, response.text + + with zipfile.ZipFile(BytesIO(response.content)) as archive: + by_name = {info.filename: info for info in archive.infolist()} + assert by_name["clip.mp4"].compress_type == zipfile.ZIP_STORED + assert by_name["shot.png"].compress_type == zipfile.ZIP_DEFLATED + finally: + client.delete("/delete_album/zipmix_album") + + +def test_zip_size_check_ignores_files_it_would_not_include(client, new_album, monkeypatch): + """The ceiling must be measured over what actually gets written. + + An index can name a path outside the album (a moved directory, an edited + config); those are skipped when building the archive, so counting their + bytes could refuse a selection that zips to almost nothing. + """ + from fixtures import build_index + + from photomap.backend.routers import search as search_router_module + + build_index(client, new_album) + + monkeypatch.setattr( + search_router_module, "validate_image_access", lambda *args, **kwargs: False + ) + monkeypatch.setattr(search_router_module, "_MAX_ZIP_BYTES", 1) + + response = client.post( + f"/download_images_zip/{new_album['key']}", json={"indices": [0, 1]} + ) + + # Nothing is includable, so nothing counts: an empty archive, not a 413. + assert response.status_code == 200, response.text + with zipfile.ZipFile(BytesIO(response.content)) as archive: + assert archive.namelist() == [] + + def test_zip_download_still_works_under_the_limit(client, new_album): from fixtures import build_index @@ -324,6 +408,25 @@ def test_adding_the_same_warning_twice_does_not_duplicate_it(): assert tracker.get_progress("album").warning_message.count("2 videos") == 1 +def test_adding_a_warning_that_is_a_substring_of_another_keeps_both(): + """Deduplication compares whole notices, not substrings of the joined text. + + "2 videos could not be read." is a substring of "12 videos could not be + read.", so a containment check against the accumulated string dropped it — + the exact silent discard this method exists to prevent. + """ + tracker = ProgressTracker() + tracker.add_completion_warning("album", "12 videos could not be read.") + tracker.add_completion_warning("album", "2 videos could not be read.") + + tracker.start_operation("album", total_images=1, operation_type="indexing") + tracker.complete_operation("album") + + warning = tracker.get_progress("album").warning_message + assert "12 videos could not be read." in warning + assert warning.count("videos could not be read.") == 2 + + def test_adding_an_empty_warning_is_a_noop(): tracker = ProgressTracker() tracker.add_completion_warning("album", "kept") diff --git a/tests/frontend/bookmarks-download.test.js b/tests/frontend/bookmarks-download.test.js new file mode 100644 index 00000000..57128c99 --- /dev/null +++ b/tests/frontend/bookmarks-download.test.js @@ -0,0 +1,155 @@ +// downloadSingleImage has to treat a video differently from a photo: a photo +// is fetched into a blob so the object URL can carry a chosen filename, but a +// 200 MB clip buffered the same way sits entirely in browser memory before the +// save dialog appears. +import { beforeEach, describe, expect, it, jest } from "@jest/globals"; + +const mockFetchJson = jest.fn(); + +jest.unstable_mockModule("../../photomap/frontend/static/javascript/utils.js", () => ({ + errorDetail: jest.fn(), + fetchJson: mockFetchJson, + hideSpinner: jest.fn(), + setCheckmarkOnIcon: jest.fn(), + showSpinner: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/control-panel.js", () => ({ + showDeleteConfirmModal: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/filetree.js", () => ({ + createSimpleDirectoryPicker: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/index.js", () => ({ + deleteImages: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/modal-utils.js", () => ({ + showConfirmModal: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/search.js", () => ({ + setSearchResults: jest.fn(), +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/slide-state.js", () => ({ + slideState: { getCurrentSlide: () => ({ globalIndex: 0 }) }, +})); +jest.unstable_mockModule("../../photomap/frontend/static/javascript/state.js", () => ({ + state: { album: "album", swiper: null, single_swiper: null }, + saveSettingsToLocalStorage: jest.fn(), +})); + +const { bookmarkManager } = await import("../../photomap/frontend/static/javascript/bookmarks.js"); + +/** Capture the the download path synthesises, and swallow its click. */ +function captureAnchor() { + const anchors = []; + const realCreate = document.createElement.bind(document); + jest.spyOn(document, "createElement").mockImplementation((tag) => { + const el = realCreate(tag); + if (tag === "a") { + el.click = jest.fn(); + anchors.push(el); + } + return el; + }); + return anchors; +} + +beforeEach(() => { + jest.restoreAllMocks(); + mockFetchJson.mockReset(); + global.fetch = jest.fn(); + global.URL.createObjectURL = jest.fn(() => "blob:mock"); + global.URL.revokeObjectURL = jest.fn(); +}); + +describe("downloading a video", () => { + const VIDEO = { + media_type: "video", + image_url: "video_frame/album/3", + video_url: "videos/album/clip.mp4", + filename: "clip.mp4", + filepath: "/photos/clip.mp4", + }; + + it("links straight at the video instead of buffering it into a blob", async () => { + mockFetchJson.mockResolvedValue(VIDEO); + const anchors = captureAnchor(); + + await bookmarkManager.downloadSingleImage(3); + + // No fetch of the media itself, and no object URL: that is the whole point. + expect(global.fetch).not.toHaveBeenCalled(); + expect(global.URL.createObjectURL).not.toHaveBeenCalled(); + + expect(anchors).toHaveLength(1); + expect(anchors[0].getAttribute("href")).toBe("videos/album/clip.mp4"); + expect(anchors[0].download).toBe("clip.mp4"); + expect(anchors[0].click).toHaveBeenCalled(); + }); + + it("downloads the playable file, not the still frame", async () => { + mockFetchJson.mockResolvedValue(VIDEO); + const anchors = captureAnchor(); + + await bookmarkManager.downloadSingleImage(3); + + expect(anchors[0].getAttribute("href")).not.toBe(VIDEO.image_url); + }); + + it("falls back to the still frame when there is no video URL", async () => { + mockFetchJson.mockResolvedValue({ ...VIDEO, video_url: "" }); + const anchors = captureAnchor(); + + await bookmarkManager.downloadSingleImage(3); + + expect(anchors[0].getAttribute("href")).toBe("video_frame/album/3"); + }); + + it("names an unnamed video with its real extension, not .jpg", async () => { + // The old hardcoded image_${i}.jpg would save a clip under a name no + // player would open. + mockFetchJson.mockResolvedValue({ ...VIDEO, filename: "", filepath: "/photos/holiday.webm" }); + const anchors = captureAnchor(); + + await bookmarkManager.downloadSingleImage(7); + + expect(anchors[0].download).toBe("image_7.webm"); + }); + + it("still names a video when the index carries no path at all", async () => { + mockFetchJson.mockResolvedValue({ ...VIDEO, filename: "", filepath: undefined }); + const anchors = captureAnchor(); + + await bookmarkManager.downloadSingleImage(7); + + expect(anchors[0].download).toBe("image_7.mp4"); + }); +}); + +describe("downloading a still image", () => { + const IMAGE = { + media_type: "image", + image_url: "images/album/shot.jpg", + filename: "shot.jpg", + filepath: "/photos/shot.jpg", + }; + + it("keeps the blob path, so the chosen filename is honoured", async () => { + mockFetchJson.mockResolvedValue(IMAGE); + global.fetch.mockResolvedValue({ ok: true, blob: async () => new Blob(["x"]) }); + const anchors = captureAnchor(); + + await bookmarkManager.downloadSingleImage(1); + + expect(global.fetch).toHaveBeenCalledWith("images/album/shot.jpg"); + expect(global.URL.createObjectURL).toHaveBeenCalled(); + expect(anchors[0].getAttribute("href")).toBe("blob:mock"); + expect(anchors[0].download).toBe("shot.jpg"); + }); + + it("still throws when the image cannot be fetched", async () => { + mockFetchJson.mockResolvedValue(IMAGE); + global.fetch.mockResolvedValue({ ok: false }); + + await expect(bookmarkManager.downloadSingleImage(1)).rejects.toThrow(/Failed to fetch/); + }); +});