From 9cbdb70e13510d39b6ed37c9aa109a9a835bec14 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 1 Jun 2026 21:57:29 -0400 Subject: [PATCH] fix(thumbnails): surface backfill results + tighten validity check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two coupled problems, operator-flagged 2026-06-01: "missing thumbnails but triggering backfill found nothing." 1. **Backfill UI was a black box.** `POST /api/thumbnails/backfill` returned just `{celery_task_id}` and the admin card said "Enqueued." with no counts. There was no way to tell whether the scan found 0 candidates, 5000 candidates, or whether the worker even picked up the task. "Found nothing" was indistinguishable from a broken queue. Fix: refactor the scan into a sync helper (`_run_backfill_scan`) shared by the Celery task and the API endpoint. The API now runs the scan in an executor and returns `{scanned, enqueued, ok, regenerated}`. The actual thumbnail generation work still goes to the thumbnail Celery queue per row via `generate_thumbnail.delay()` — the scan itself is fast (SELECT id+thumbnail_path + a file.stat() per row). 2. **`_thumb_is_valid` accepted header-only corrupt files.** The magic-byte check passed for any 8-byte file starting with a JPEG or PNG header, including empty/truncated/zero-pad files that browsers render as broken. Backfill counted these as `ok` and never regenerated. Fix: also require file size ≥ MIN_THUMB_BYTES (256). Real thumbnails are minimum ~2KB even on solid-color sources; header- only corrupt files top out around 12 bytes. 256 is well above the corrupt floor and well below any legitimate thumbnail. Plus the admin card now shows the per-run counts instead of "Enqueued.": Scanned 5,432 · enqueued 3 (2 regenerated) · 5,429 ok --- backend/app/api/thumbnails.py | 21 ++- backend/app/tasks/thumbnail.py | 132 +++++++++++------- .../settings/ThumbnailBackfillCard.vue | 14 +- frontend/src/stores/thumbnails.js | 5 +- tests/test_api_thumbnails.py | 14 +- tests/test_backfill_thumbnails.py | 24 +++- 6 files changed, 148 insertions(+), 62 deletions(-) diff --git a/backend/app/api/thumbnails.py b/backend/app/api/thumbnails.py index eff1491..7fe1845 100644 --- a/backend/app/api/thumbnails.py +++ b/backend/app/api/thumbnails.py @@ -1,5 +1,7 @@ """Thumbnail admin API: backfill trigger.""" +import asyncio + from quart import Blueprint, jsonify thumbnails_bp = Blueprint("thumbnails", __name__, url_prefix="/api/thumbnails") @@ -7,7 +9,20 @@ thumbnails_bp = Blueprint("thumbnails", __name__, url_prefix="/api/thumbnails") @thumbnails_bp.route("/backfill", methods=["POST"]) async def trigger_backfill(): - from ..tasks.thumbnail import backfill_thumbnails + """Run the backfill scan synchronously, return the counts. The actual + thumbnail generation work is still off-loaded to the thumbnail Celery + queue via `generate_thumbnail.delay()` per missing row — so this + handler is fast even on a 100k-image library (a scan is just SELECT + id, thumbnail_path + a file.stat() per row, no heavy work). - r = backfill_thumbnails.delay() - return jsonify({"celery_task_id": r.id}), 202 + Operator-flagged 2026-06-01: the previous fire-and-forget shape + returned `{celery_task_id}` only, so the admin UI had no idea whether + backfill found 0 or 5000 candidates — \"found nothing\" was + indistinguishable from \"the worker isn't picking up the task.\"""" + from ..tasks.thumbnail import _run_backfill_scan + + # Sync scan inside an executor so we don't block the event loop. + counts = await asyncio.get_running_loop().run_in_executor( + None, _run_backfill_scan, + ) + return jsonify(counts), 200 diff --git a/backend/app/tasks/thumbnail.py b/backend/app/tasks/thumbnail.py index 0061bb1..7e67e38 100644 --- a/backend/app/tasks/thumbnail.py +++ b/backend/app/tasks/thumbnail.py @@ -20,14 +20,32 @@ IMAGES_ROOT = Path("/images") THUMB_MAGIC_JPEG = b"\xff\xd8\xff" THUMB_MAGIC_PNG = b"\x89PNG\r\n\x1a\n" +# Minimum file size for a thumbnail to count as valid. Anything smaller +# is almost certainly truncated/corrupt — a legitimate 400×400 JPEG@85 +# bottoms out around 2KB even on a solid-color image; 400×400 PNG starts +# around 1KB. 256 bytes is well below any real thumbnail and well above +# header-only corrupt files (~8-12 bytes). Operator-flagged 2026-06-01: +# header-only corrupt files were silently passing the magic-byte check +# and backfill counted them as "ok" — so broken-image tiles in the UI +# never got regenerated even after running backfill. +MIN_THUMB_BYTES = 256 + def _thumb_is_valid(path: Path) -> bool: - """Return True iff `path` exists and starts with a JPEG or PNG magic header. + """Return True iff `path` exists, starts with a JPEG or PNG magic + header, AND is at least MIN_THUMB_BYTES on disk. - The on-disk thumbnail format is set by services/thumbnailer.py — JPEG for - opaque sources, PNG for alpha sources. Anything else (missing file, OSError, - truncated, wrong magic) is invalid. + The on-disk thumbnail format is set by services/thumbnailer.py — JPEG + for opaque sources, PNG for alpha sources. Anything else (missing + file, OSError, truncated below the size floor, wrong magic) is + invalid and gets re-enqueued. """ + try: + size = path.stat().st_size + except OSError: + return False + if size < MIN_THUMB_BYTES: + return False try: with path.open("rb") as f: head = f.read(12) @@ -42,6 +60,59 @@ def _thumb_is_valid(path: Path) -> bool: return False +def _run_backfill_scan() -> dict: + """Synchronous scan logic shared by the Celery task and the API + endpoint. Returns {enqueued, ok, regenerated, scanned}. + + Operator-flagged 2026-06-01: the original task was fire-and-forget, + so the admin UI couldn't show what backfill actually found — + operator saw \"Enqueued.\" with no counts and assumed nothing was + happening. Now the API runs this synchronously and returns the + real numbers; the periodic Celery task wraps it too.""" + from sqlalchemy import select, update + + SessionLocal = _sync_session_factory() + enqueued = 0 + ok = 0 + regenerated = 0 + scanned = 0 + last_id = 0 + with SessionLocal() as session: + while True: + rows = session.execute( + select(ImageRecord.id, ImageRecord.thumbnail_path) + .where(ImageRecord.id > last_id) + .order_by(ImageRecord.id.asc()) + .limit(500) + ).all() + if not rows: + break + scanned += len(rows) + for image_id, thumb_path in rows: + if thumb_path is None: + generate_thumbnail.delay(image_id) + enqueued += 1 + elif _thumb_is_valid(Path(thumb_path)): + ok += 1 + else: + session.execute( + update(ImageRecord) + .where(ImageRecord.id == image_id) + .values(thumbnail_path=None) + ) + generate_thumbnail.delay(image_id) + enqueued += 1 + regenerated += 1 + session.commit() + last_id = rows[-1][0] + return { + "scanned": scanned, + "enqueued": enqueued, + "ok": ok, + "regenerated": regenerated, + } + + @celery.task( name="backend.app.tasks.thumbnail.generate_thumbnail", bind=True, @@ -84,48 +155,15 @@ def backfill_thumbnails(self) -> dict: """Scan ImageRecord and enqueue generate_thumbnail for rows whose thumbnail is missing, gone from disk, or has wrong magic bytes. - Keyset paginates by id ASC, page size 500. NULLs out thumbnail_path for - rows that point at a missing or corrupt file before enqueueing — keeps - the DB self-consistent on partial runs and makes re-runs safe. + Keyset paginates by id ASC, page size 500. NULLs out thumbnail_path + for rows that point at a missing or corrupt file before enqueueing — + keeps the DB self-consistent on partial runs and makes re-runs safe. - Returns {"enqueued": N, "ok": M, "regenerated": K} where: - - enqueued = total generate_thumbnail.delay() calls - - ok = rows whose existing thumbnail file is valid (skipped) - - regenerated = subset of enqueued that had a non-NULL thumbnail_path - cleared (i.e. missing + corrupt) + Returns {scanned, enqueued, ok, regenerated} where: + - scanned = total rows examined + - enqueued = total generate_thumbnail.delay() calls + - ok = rows whose existing thumbnail file is valid (skipped) + - regenerated = subset of enqueued that had a non-NULL + thumbnail_path cleared (i.e. missing + corrupt) """ - from sqlalchemy import select, update - - SessionLocal = _sync_session_factory() - enqueued = 0 - ok = 0 - regenerated = 0 - last_id = 0 - with SessionLocal() as session: - while True: - rows = session.execute( - select(ImageRecord.id, ImageRecord.thumbnail_path) - .where(ImageRecord.id > last_id) - .order_by(ImageRecord.id.asc()) - .limit(500) - ).all() - if not rows: - break - for image_id, thumb_path in rows: - if thumb_path is None: - generate_thumbnail.delay(image_id) - enqueued += 1 - elif _thumb_is_valid(Path(thumb_path)): - ok += 1 - else: - session.execute( - update(ImageRecord) - .where(ImageRecord.id == image_id) - .values(thumbnail_path=None) - ) - generate_thumbnail.delay(image_id) - enqueued += 1 - regenerated += 1 - session.commit() - last_id = rows[-1][0] - return {"enqueued": enqueued, "ok": ok, "regenerated": regenerated} + return _run_backfill_scan() diff --git a/frontend/src/components/settings/ThumbnailBackfillCard.vue b/frontend/src/components/settings/ThumbnailBackfillCard.vue index 7d73e0d..fa23034 100644 --- a/frontend/src/components/settings/ThumbnailBackfillCard.vue +++ b/frontend/src/components/settings/ThumbnailBackfillCard.vue @@ -10,7 +10,14 @@ mdi-image-refresh Run backfill now - Enqueued. + + Scanned {{ result.scanned }} · enqueued + {{ result.enqueued }} + + ({{ result.regenerated }} regenerated) + + · {{ result.ok }} ok + @@ -23,10 +30,11 @@ import { useThumbnailsStore } from '../../stores/thumbnails.js' import QueueStatusBar from './QueueStatusBar.vue' const store = useThumbnailsStore() const busy = ref(false) -const done = ref(false) +const result = ref(null) async function run () { busy.value = true - try { await store.triggerBackfill(); done.value = true } + result.value = null + try { result.value = await store.triggerBackfill() } catch (e) { toast({ text: e.message, type: 'error' }) } finally { busy.value = false } } diff --git a/frontend/src/stores/thumbnails.js b/frontend/src/stores/thumbnails.js index 5f86f2b..e7da7ba 100644 --- a/frontend/src/stores/thumbnails.js +++ b/frontend/src/stores/thumbnails.js @@ -4,8 +4,11 @@ import { useApi } from '../composables/useApi.js' export const useThumbnailsStore = defineStore('thumbnails', () => { const api = useApi() + // Returns { scanned, enqueued, ok, regenerated } — the API now runs + // the scan synchronously so the operator gets immediate feedback + // instead of just a Celery task id with no visible outcome. async function triggerBackfill () { - await api.post('/api/thumbnails/backfill') + return await api.post('/api/thumbnails/backfill') } return { triggerBackfill } diff --git a/tests/test_api_thumbnails.py b/tests/test_api_thumbnails.py index 9a9c729..9f4450b 100644 --- a/tests/test_api_thumbnails.py +++ b/tests/test_api_thumbnails.py @@ -13,8 +13,16 @@ def eager(): @pytest.mark.asyncio -async def test_trigger_thumbnail_backfill(client): +async def test_trigger_thumbnail_backfill_returns_counts(client): + """The API runs the scan synchronously now and returns + {scanned, enqueued, ok, regenerated} — operator-flagged 2026-06-01: + the previous fire-and-forget shape returned only a celery_task_id, + so the admin UI couldn't show whether backfill found 0 or 5000 + candidates. \"Found nothing\" was indistinguishable from \"the + worker isn't picking up the task.\"""" r = await client.post("/api/thumbnails/backfill") - assert r.status_code == 202 + assert r.status_code == 200 body = await r.get_json() - assert "celery_task_id" in body + assert set(body) == {"scanned", "enqueued", "ok", "regenerated"} + for key in ("scanned", "enqueued", "ok", "regenerated"): + assert isinstance(body[key], int) diff --git a/tests/test_backfill_thumbnails.py b/tests/test_backfill_thumbnails.py index faf2a77..878b1e7 100644 --- a/tests/test_backfill_thumbnails.py +++ b/tests/test_backfill_thumbnails.py @@ -12,13 +12,16 @@ pytestmark = pytest.mark.integration def test_thumb_is_valid_jpeg(tmp_path): p = tmp_path / "good.jpg" - p.write_bytes(b"\xff\xd8\xff\xe0" + b"\x00" * 100) + # Real thumbnails are at least ~2KB; size check (MIN_THUMB_BYTES=256) + # requires the file body be plausible. 300 bytes here clears the + # floor with margin. + p.write_bytes(b"\xff\xd8\xff\xe0" + b"\x00" * 300) assert _thumb_is_valid(p) is True def test_thumb_is_valid_png(tmp_path): p = tmp_path / "good.png" - p.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + p.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 300) assert _thumb_is_valid(p) is True @@ -38,6 +41,16 @@ def test_thumb_is_valid_missing_file(tmp_path): assert _thumb_is_valid(tmp_path / "nope") is False +def test_thumb_is_valid_header_only_below_min_size(tmp_path): + """Operator-flagged 2026-06-01: header-only corrupt files were + silently passing the magic-byte check and backfill counted them as + `ok`, so the UI's broken-image tiles never got regenerated. Files + smaller than MIN_THUMB_BYTES are now invalid even with valid magic.""" + p = tmp_path / "header_only.jpg" + p.write_bytes(b"\xff\xd8\xff\xe0" + b"\x00" * 50) # 54 bytes total + assert _thumb_is_valid(p) is False + + # --- backfill_thumbnails planner tests ------------------------------------ @@ -80,13 +93,14 @@ def _rec(db_sync, path, *, sha, thumb_path=None, mime="image/jpeg"): def _write_jpeg(p: Path) -> Path: p.parent.mkdir(parents=True, exist_ok=True) - p.write_bytes(b"\xff\xd8\xff\xe0" + b"\x00" * 100) + # ≥ MIN_THUMB_BYTES (256) so the size floor doesn't reject it. + p.write_bytes(b"\xff\xd8\xff\xe0" + b"\x00" * 300) return p def _write_png(p: Path) -> Path: p.parent.mkdir(parents=True, exist_ok=True) - p.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 100) + p.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 300) return p @@ -238,5 +252,5 @@ def test_backfill_mixed_aggregate(db_sync, tmp_path, monkeypatch): ) result = m.backfill_thumbnails() - assert result == {"enqueued": 3, "ok": 2, "regenerated": 2} + assert result == {"scanned": 5, "enqueued": 3, "ok": 2, "regenerated": 2} assert sorted(delayed) == sorted([r_null.id, r_missing.id, r_bad.id])