refactor(downloads): address /code-review on the download hotfix

- edit.py: extract _run_ffmpeg_best_effort (one run+timeout+swallow policy shared by
  thumbnail_to_jpg / ensure_poster / ensure_storyboard) + a build_thumbnail_jpg_cmd
  builder, matching the module's other ffmpeg commands.
- thumbnail_to_jpg: copy an already-JPEG thumbnail (Facebook, and Reddit's jpeg-under-
  a-.png-name) instead of re-encoding it; only webp/png-proper are decoded via ffmpeg.
- errors.py: drop the redundant "account authentication" classify marker
  ("authentication is required" already covers the known Reddit message).
- formats.py: document that the opt-in EmbedThumbnail is unaffected by dropping the
  convertor (it converts unsupported thumbs to PNG, not the crashing mjpeg2jpeg path).
- test_titles.py: guard the ReDoS regression with a thread + timeout (deterministic
  "must not hang") instead of a wall-clock assertion.

backend/app/downloads/edit.py

@@ -25,6 +25,7 @@ import json
 import logging
 import math
 import queue
+import shutil
 import subprocess
 import threading
 import time
@@ -489,24 +490,57 @@ def build_poster_cmd(src: Path, dest: Path, at_s: float) -> list[str]:
     ]
 
 
+def build_thumbnail_jpg_cmd(src: Path, dest: Path) -> list[str]:
+    """Decode a downloaded thumbnail (webp/png/…) to a single baseline JPEG frame."""
+    return [
+        "ffmpeg", "-y", "-hide_banner", "-loglevel", "error", "-nostdin",
+        "-i", str(src), "-frames:v", "1", "-update", "1", str(dest),
+    ]
+
+
+def _run_ffmpeg_best_effort(cmd: list[str], timeout_s: int) -> Exception | None:
+    """Run a one-shot ffmpeg command, bounded by `timeout_s`. Returns None on success, or the
+    exception on a timeout/non-zero-exit/OS error — never raises. Shared by the best-effort image
+    steps (poster / thumbnail / storyboard) so the run+timeout+swallow policy lives in ONE place;
+    each caller logs its own context from the returned exception. (The download/transcode path uses
+    the richer `run_ffmpeg` instead, which streams progress and honours cooperative cancel.)"""
+    try:
+        subprocess.run(cmd, capture_output=True, timeout=timeout_s, check=True)
+        return None
+    except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc:
+        return exc
+
+
+def _is_jpeg(path: Path) -> bool:
+    """True if the file starts with the JPEG SOI marker (FF D8 FF)."""
+    try:
+        with path.open("rb") as f:
+            return f.read(3) == b"\xff\xd8\xff"
+    except OSError:
+        return False
+
+
 def thumbnail_to_jpg(src: Path | None, dest: Path, timeout_s: int = 30) -> Path | None:
-    """Convert a downloaded thumbnail (webp / png / jpeg — whatever the source shipped) into a real
+    """Turn a downloaded thumbnail (webp / png / jpeg — whatever the source shipped) into a real
     baseline JPEG for the `<name>.jpg` poster + og:image share sidecar.
 
-    Uses a plain ffmpeg decode→mjpeg, deliberately NOT yt-dlp's `FFmpegThumbnailsConvertor` (whose
-    `-bsf:v mjpeg2jpeg` path crashes on some already-JPEG thumbnails, e.g. Reddit's, and used to
-    fail the entire download). Best-effort: returns None on any failure, so the caller falls back to
-    a cut video frame via `ensure_poster` and the download still succeeds."""
+    Deliberately NOT yt-dlp's `FFmpegThumbnailsConvertor` (whose `-bsf:v mjpeg2jpeg` path crashes on
+    some already-JPEG thumbnails, e.g. Reddit's, and used to fail the entire download). A thumbnail
+    that is already JPEG is copied as-is; anything else is decoded via ffmpeg. Best-effort: returns
+    None on any failure, so the caller falls back to a cut video frame (`ensure_poster`) and the
+    download still succeeds."""
     if src is None or not src.exists():
         return None
-    try:
-        subprocess.run(
-            ["ffmpeg", "-y", "-hide_banner", "-loglevel", "error", "-nostdin",
-             "-i", str(src), "-frames:v", "1", "-update", "1", str(dest)],
-            capture_output=True, timeout=timeout_s, check=True,
-        )
-    except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc:
-        log.info("thumbnail->jpg conversion failed (%s); poster falls back to a cut frame", str(exc)[:120])
+    if _is_jpeg(src):  # already JPEG (e.g. Facebook) — no decode/re-encode needed
+        try:
+            shutil.copyfile(src, dest)
+            return dest
+        except OSError as exc:
+            log.info("thumbnail copy failed (%s); poster falls back to a cut frame", str(exc)[:120])
+            return None
+    err = _run_ffmpeg_best_effort(build_thumbnail_jpg_cmd(src, dest), timeout_s)
+    if err is not None:
+        log.info("thumbnail->jpg conversion failed (%s); poster falls back to a cut frame", str(err)[:120])
         return None
     return dest if dest.exists() else None
 
@@ -528,10 +562,9 @@ def ensure_poster(asset, download_root: str, timeout_s: int = 30) -> str | None:
     if dest.exists():
         return rel
     at = min(3.0, (asset.duration_s or 10) * 0.1)
-    try:
-        subprocess.run(build_poster_cmd(src, dest, at), capture_output=True, timeout=timeout_s, check=True)
-    except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc:
-        log.info("poster gen skipped for asset %s: %s", asset.id, str(exc)[:120])
+    err = _run_ffmpeg_best_effort(build_poster_cmd(src, dest, at), timeout_s)
+    if err is not None:
+        log.info("poster gen skipped for asset %s: %s", asset.id, str(err)[:120])
         return None
     return rel if dest.exists() else None
 
@@ -560,10 +593,9 @@ def ensure_storyboard(asset, download_root: str, timeout_s: int = 90) -> dict |
         return None
     dest.parent.mkdir(parents=True, exist_ok=True)
     cmd = build_storyboard_cmd(src, dest, asset.duration_s, geom)
-    try:
-        subprocess.run(cmd, capture_output=True, timeout=timeout_s, check=True)
-    except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc:
-        log.info("storyboard gen skipped for asset %s: %s", asset.id, str(exc)[:120])
+    err = _run_ffmpeg_best_effort(cmd, timeout_s)
+    if err is not None:
+        log.info("storyboard gen skipped for asset %s: %s", asset.id, str(err)[:120])
         return None
     if not dest.exists():
         return None

backend/app/downloads/errors.py

@@ -24,7 +24,6 @@ _RULES: tuple[tuple[str, str], ...] = (
     # Some extractors (e.g. Reddit) gate content behind a logged-in account: "Account
     # authentication is required." — an auth wall we deliberately don't cross, not a transient error.
     ("authentication is required", "login_required"),
-    ("account authentication", "login_required"),
     # Geo-restricted.
     ("in your country", "geo_blocked"),
     ("available in your location", "geo_blocked"),

backend/app/downloads/formats.py

@@ -218,6 +218,9 @@ def build_ydl_opts(
     # The `<name>.jpg` poster sidecar is instead produced robustly by our own pipeline (worker
     # `_download` → `editmod.thumbnail_to_jpg`, with `ensure_poster` cutting a video frame as a
     # fallback), so the og:image share preview (Facebook Messenger / MS Teams) keeps working.
+    # The opt-in EmbedThumbnail below is unaffected: it does its OWN format handling — it embeds a
+    # jpg/png thumbnail directly and converts anything else to PNG (not the crashing mjpeg2jpeg-to-jpg
+    # path), so it never needed the convertor and does not reintroduce the crash.
     if s["embed_thumbnail"]:
         opts["postprocessors"].append(
             {"key": "EmbedThumbnail", "already_have_thumbnail": True}

backend/tests/test_titles.py

@@ -1,4 +1,9 @@
 """normalize_title: de-shout / de-clutter video titles without mangling non-English text."""
+from concurrent.futures import ThreadPoolExecutor
+from concurrent.futures import TimeoutError as FuturesTimeout
+
+import pytest
+
 from app.titles import normalize_title
 
 
@@ -29,20 +34,26 @@ def test_numeric_episode_marker_is_not_treated_as_a_hashtag():
     assert normalize_title("My show #3") == "My show #3"
 
 
-def test_midstring_hashtags_are_fast_and_kept_redos_regression():
+def test_midstring_hashtags_dont_hang_and_are_kept_redos_regression():
     # Facebook prepends an engagement prefix and appends a page name, leaving a long hashtag cluster
     # in the MIDDLE. That is not a trailing SEO cluster, so the tags stay — and, critically, this
     # exact shape used to trigger catastrophic backtracking (ReDoS) in `_TRAILING_HASHTAGS` that
     # pinned a download-worker thread at 100% CPU forever inside the yt-dlp progress hook.
-    import time
-
     raw = (
         "398K views · 6.1K reactions | The money should follow 🙄 #reel #reels #meme #memes "
         "#fun #funny #work #worklife #office #viral #trending #fyp #foryou #foryoupage | Memebonanza4U"
     )
-    t = time.perf_counter()
-    out = normalize_title(raw) or ""
-    assert time.perf_counter() - t < 1.0  # linear now; the old regex ran for minutes / never returned
+    # Run in a worker thread with a generous ceiling: the fixed regex returns in microseconds, so
+    # only a genuine catastrophic-backtracking regression fails to complete — a hang is caught
+    # deterministically rather than by asserting on wall-clock slack in the happy path.
+    ex = ThreadPoolExecutor(max_workers=1)
+    fut = ex.submit(normalize_title, raw)
+    try:
+        out = fut.result(timeout=5) or ""
+    except FuturesTimeout:
+        pytest.fail("normalize_title did not return within 5s — catastrophic backtracking regressed")
+    finally:
+        ex.shutdown(wait=False)  # never join a (hypothetically) wedged thread
     assert "#foryoupage" in out and "#reel" in out  # mid-string hashtags preserved (not trailing)
     assert "views" not in out  # the engagement-count prefix was still stripped