From b21190039072d2141ab66ae9a0f5dd9ee7de6acf Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 6 Jun 2026 11:11:25 -0400 Subject: [PATCH] =?UTF-8?q?refactor(downloads):=20DRY=20the=20ingester/gal?= =?UTF-8?q?lery-dl=20seam=20=E2=80=94=20A1=E2=80=93A4=20(plan=20#707)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates duplication that owning the native ingester left against the still- live gallery-dl path, and fixes a parity gap the duplication hid. A1 — shared quarantine: extract file_validator.quarantine_file (move to _quarantine// + write the .quarantine.json provenance sidecar). gallery_dl._validate_and_quarantine and patreon_downloader._validate_path both call it. PARITY FIX: the native path now writes the provenance sidecar it previously skipped — threads the media url through for source_url. A2 — make_run_stats(**counts) factory in gallery_dl for the canonical run_stats key set; gallery_dl._compute_run_stats and ingest_core both build through it so the shape can't drift (gallery-dl path gains a benign dead_lettered_count=0). A3 — one safe_ext in utils/paths.py; importer._safe_ext (thin wrapper, kept for the Path call sites + memory pointer) and patreon_client both use it. Closes the double-impl of the URL-encoded-basename ext gotcha. A4 — promote gallery_dl._truncate_log/_extract_errors_warnings to module-level truncate_log/extract_errors_warnings; download_service calls them directly instead of reaching through self.gdl for native-result log shaping. The staticmethods stay as thin delegators for existing callers/tests. Behavior-preserving except the A1 sidecar parity fix. Test: native quarantine writes a .quarantine.json (test_patreon_downloader). Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/app/services/download_service.py | 8 +- backend/app/services/file_validator.py | 55 ++++++++ backend/app/services/gallery_dl.py | 145 ++++++++++++--------- backend/app/services/importer.py | 31 ++--- backend/app/services/ingest_core.py | 20 ++- backend/app/services/patreon_client.py | 18 +-- backend/app/services/patreon_downloader.py | 40 +++--- backend/app/utils/paths.py | 20 +++ tests/test_patreon_downloader.py | 22 ++++ 9 files changed, 222 insertions(+), 137 deletions(-) diff --git a/backend/app/services/download_service.py b/backend/app/services/download_service.py index 390d854..86d8324 100644 --- a/backend/app/services/download_service.py +++ b/backend/app/services/download_service.py @@ -33,6 +33,8 @@ from .gallery_dl import ( ErrorType, GalleryDLService, SourceConfig, + extract_errors_warnings, + truncate_log, ) from .importer import Importer from .patreon_ingester import PatreonIngester @@ -450,7 +452,7 @@ class DownloadService: dl_result.return_code, dl_result.stdout, dl_result.stderr ) run_stats["quarantined_count"] = dl_result.files_quarantined - stderr_summary = self.gdl._extract_errors_warnings(dl_result.stderr) + stderr_summary = extract_errors_warnings(dl_result.stderr) # Plan #544: PARTIAL means the run downloaded ≥1 file but the # subprocess didn't finish in budget (typically wall-clock timeout @@ -470,8 +472,8 @@ class DownloadService: ev.metadata_ = { "run_stats": run_stats, "error_type": dl_result.error_type.value if dl_result.error_type else None, - "stdout": self.gdl._truncate_log(dl_result.stdout) or None, - "stderr": self.gdl._truncate_log(dl_result.stderr) or None, + "stdout": truncate_log(dl_result.stdout) or None, + "stderr": truncate_log(dl_result.stderr) or None, "stderr_errors_warnings": stderr_summary or None, "duration_seconds": dl_result.duration_seconds, "quarantined_paths": dl_result.quarantined_paths or None, diff --git a/backend/app/services/file_validator.py b/backend/app/services/file_validator.py index ca6e40f..e007422 100644 --- a/backend/app/services/file_validator.py +++ b/backend/app/services/file_validator.py @@ -11,9 +11,15 @@ expected to write. from __future__ import annotations +import json +import logging +import shutil from dataclasses import dataclass +from datetime import UTC, datetime from pathlib import Path +log = logging.getLogger(__name__) + JPEG_HEAD = b"\xff\xd8\xff" JPEG_TAIL = b"\xff\xd9" @@ -108,3 +114,52 @@ def validate_file(path: Path) -> ValidationResult: return ValidationResult(ok=True, format="webp", size=size) return ValidationResult(ok=True, format=None, size=size) + + +def quarantine_file( + images_root: Path, + path: Path, + artist_slug: str, + platform: str, + *, + url: str | None, + result: ValidationResult, +) -> Path | None: + """Move a validation-failed file to `_quarantine//` and write + a `.quarantine.json` provenance sidecar next to it. + + Returns the destination path, or None if the move itself failed (file left + in place — the caller decides what to report). The caller has already run + `validate_file` and seen `result.ok is False`. ONE implementation for both + download backends — gallery-dl's batch post-process and the native ingester's + per-media path — so the quarantine layout + provenance sidecar can't drift. + """ + quarantine_root = images_root / "_quarantine" / artist_slug / platform + try: + quarantine_root.mkdir(parents=True, exist_ok=True) + dest = quarantine_root / path.name + counter = 1 + while dest.exists(): + dest = quarantine_root / f"{path.stem}.{counter}{path.suffix}" + counter += 1 + shutil.move(str(path), str(dest)) + sidecar = dest.with_suffix(dest.suffix + ".quarantine.json") + sidecar.write_text( + json.dumps( + { + "original_path": str(path), + "source_url": url, + "artist_slug": artist_slug, + "platform": platform, + "format": result.format, + "reason": result.reason, + "size": result.size, + "quarantined_at": datetime.now(UTC).isoformat(), + }, + indent=2, + ) + ) + except OSError as exc: + log.error("Failed to quarantine %s: %s. File left in place.", path, exc) + return None + return dest diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index f4c9967..d9e9541 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -22,7 +22,7 @@ from datetime import UTC, datetime from enum import StrEnum from pathlib import Path -from .file_validator import is_validatable, validate_file +from .file_validator import is_validatable, quarantine_file, validate_file log = logging.getLogger(__name__) @@ -165,6 +165,39 @@ class DownloadResult: posts_processed: int = 0 +def extract_errors_warnings(stderr: str) -> str: + """Keep only the `[error]`/`[warning]` lines from a captured stderr blob. + + A generic download-log helper (module-level so the native-ingester result + path can shape its logs without reaching through a GalleryDLService instance). + """ + if not stderr: + return "" + kept = [ + line for line in stderr.splitlines() + if "][error]" in line.lower() or "][warning]" in line.lower() + ] + return "\n".join(kept) + + +def truncate_log(text: str, max_bytes: int = 500_000) -> str: + """Cap a captured log to `max_bytes`, eliding the middle (head + tail kept).""" + if not text: + return text + encoded = text.encode("utf-8") + if len(encoded) <= max_bytes: + return text + half = max_bytes // 2 + head = encoded[:half].decode("utf-8", errors="ignore") + tail = encoded[-half:].decode("utf-8", errors="ignore") + head_lines = head.count("\n") + tail_lines = tail.count("\n") + total_lines = text.count("\n") + elided = max(0, total_lines - head_lines - tail_lines) + marker = f"\n\n... [{elided} lines elided, {len(encoded) - max_bytes} bytes] ...\n\n" + return head + marker + tail + + def _summarize_validation_failures(failures: list[dict]) -> str: if not failures: return "Validation failed" @@ -184,6 +217,35 @@ def _summarize_validation_failures(failures: list[dict]) -> str: # no log text to scrape — and gallery-dl platforms never had a cursor.) +def make_run_stats( + *, + exit_code: int = 0, + downloaded_count: int = 0, + skipped_count: int = 0, + per_item_failures: int = 0, + warning_count: int = 0, + tier_gated_count: int = 0, + quarantined_count: int = 0, + dead_lettered_count: int = 0, +) -> dict: + """The canonical `run_stats` dict shape, in ONE place. + + Both result producers — gallery-dl's `_compute_run_stats` (log scrape) and the + native ingester's per-outcome tally (ingest_core) — build through this so the + key set can't drift between backends. Phase 3 + the Logs UI read these keys. + """ + return { + "exit_code": exit_code, + "downloaded_count": downloaded_count, + "skipped_count": skipped_count, + "per_item_failures": per_item_failures, + "warning_count": warning_count, + "tier_gated_count": tier_gated_count, + "quarantined_count": quarantined_count, + "dead_lettered_count": dead_lettered_count, + } + + class GalleryDLService: """Service for executing gallery-dl downloads.""" @@ -521,10 +583,6 @@ class GalleryDLService: if not written_paths: return quarantined_relpaths, failures - quarantine_root = ( - self.images_root / "_quarantine" / artist_slug / platform - ) - for path in written_paths: if not is_validatable(path): continue @@ -535,34 +593,14 @@ class GalleryDLService: continue if result.ok: continue - try: - quarantine_root.mkdir(parents=True, exist_ok=True) - dest = quarantine_root / path.name - counter = 1 - while dest.exists(): - dest = quarantine_root / f"{path.stem}.{counter}{path.suffix}" - counter += 1 - path.rename(dest) - sidecar = dest.with_suffix(dest.suffix + ".quarantine.json") - sidecar.write_text( - json.dumps( - { - "original_path": str(path), - "source_url": url, - "artist_slug": artist_slug, - "platform": platform, - "format": result.format, - "reason": result.reason, - "size": result.size, - "quarantined_at": datetime.now(UTC).isoformat(), - }, - indent=2, - ) - ) - except OSError as exc: - log.error("Failed to quarantine %s: %s. File left in place.", path, exc) - continue - + # Shared move+sidecar (file_validator.quarantine_file) — same impl the + # native ingester uses, so the layout + provenance sidecar can't drift. + dest = quarantine_file( + self.images_root, path, artist_slug, platform, + url=url, result=result, + ) + if dest is None: + continue # move failed → left in place, not counted log.warning( "Quarantined corrupt file: %s → %s (%s: %s)", path, dest, result.format, result.reason, @@ -600,41 +638,24 @@ class GalleryDLService: if "][warning]" in line.lower() and "not allowed to view post" in line.lower() ) - return { - "exit_code": return_code, - "downloaded_count": self._count_downloaded_files(stdout), - "skipped_count": skipped_stdout + skipped_stderr, - "per_item_failures": per_item_failures, - "warning_count": warning_count, - "tier_gated_count": tier_gated_count, - } + return make_run_stats( + exit_code=return_code, + downloaded_count=self._count_downloaded_files(stdout), + skipped_count=skipped_stdout + skipped_stderr, + per_item_failures=per_item_failures, + warning_count=warning_count, + tier_gated_count=tier_gated_count, + ) @staticmethod def _extract_errors_warnings(stderr: str) -> str: - if not stderr: - return "" - kept = [ - line for line in stderr.splitlines() - if "][error]" in line.lower() or "][warning]" in line.lower() - ] - return "\n".join(kept) + # Thin delegator to the module-level helper (kept for existing callers). + return extract_errors_warnings(stderr) @staticmethod def _truncate_log(text: str, max_bytes: int = 500_000) -> str: - if not text: - return text - encoded = text.encode("utf-8") - if len(encoded) <= max_bytes: - return text - half = max_bytes // 2 - head = encoded[:half].decode("utf-8", errors="ignore") - tail = encoded[-half:].decode("utf-8", errors="ignore") - head_lines = head.count("\n") - tail_lines = tail.count("\n") - total_lines = text.count("\n") - elided = max(0, total_lines - head_lines - tail_lines) - marker = f"\n\n... [{elided} lines elided, {len(encoded) - max_bytes} bytes] ...\n\n" - return head + marker + tail + # Thin delegator to the module-level helper (kept for existing callers). + return truncate_log(text, max_bytes) async def download( self, diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index 213b86e..e1acc6e 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -31,7 +31,12 @@ from ..models import ( Source, ) from ..utils import safe_probe -from ..utils.paths import derive_subdir, derive_top_level_artist, hash_suffixed_name +from ..utils.paths import ( + derive_subdir, + derive_top_level_artist, + hash_suffixed_name, + safe_ext, +) from ..utils.phash import compute_phash, find_similar from ..utils.sidecar import find_sidecar, parse_sidecar from ..utils.slug import slugify @@ -82,27 +87,9 @@ def is_video(path: Path) -> bool: def _safe_ext(path: Path) -> str: """Conservatively extract a file extension for PostAttachment.ext - (varchar(32)). - - gallery-dl produces some filenames with URL-encoded query-string - artifacts embedded into the basename (e.g. - `79507046_media_..._https___www.patreon.com_media-u_Z0FBQUFBQm5q...`). - `Path.suffix` finds the LAST dot and returns everything after, which - in those cases yields a 50+ char "extension" of mostly base64-ish - junk. That blows the column. Operator-flagged 2026-05-25. - - Real extensions are short and alphanumeric. We accept anything ≤ 16 - chars where every post-dot character is alphanumeric; anything else - means the input wasn't a real extension and we return the empty - string. ext is nullable-ish (empty string still satisfies NOT NULL) - and consumers should treat "" as "no known extension". - """ - suffix = path.suffix.lower() - if not suffix or len(suffix) > 16: - return "" - if not all(c.isalnum() for c in suffix[1:]): - return "" - return suffix + (varchar(32)). Thin wrapper over the shared `utils.paths.safe_ext` (kept for + the Path-typed call sites + the [[path_suffix_sanitize]] memory pointer).""" + return safe_ext(path) def _mime_for(path: Path) -> str: diff --git a/backend/app/services/ingest_core.py b/backend/app/services/ingest_core.py index 0e6fb5d..be418b0 100644 --- a/backend/app/services/ingest_core.py +++ b/backend/app/services/ingest_core.py @@ -32,7 +32,7 @@ from collections.abc import Callable from sqlalchemy import delete, func, select, text from sqlalchemy.dialects.postgresql import insert as pg_insert -from .gallery_dl import DownloadResult, ErrorType +from .gallery_dl import DownloadResult, ErrorType, make_run_stats log = logging.getLogger(__name__) @@ -153,16 +153,14 @@ class Ingester: duration_seconds=time.monotonic() - start, cursor=emitted_cursor, posts_processed=posts_processed, - run_stats={ - "exit_code": return_code, - "downloaded_count": downloaded, - "skipped_count": skipped_count, - "per_item_failures": errors, - "warning_count": 0, - "tier_gated_count": 0, - "quarantined_count": quarantined, - "dead_lettered_count": dead_lettered, - }, + run_stats=make_run_stats( + exit_code=return_code, + downloaded_count=downloaded, + skipped_count=skipped_count, + per_item_failures=errors, + quarantined_count=quarantined, + dead_lettered_count=dead_lettered, + ), ) try: diff --git a/backend/app/services/patreon_client.py b/backend/app/services/patreon_client.py index 5d1ef2b..f2af42f 100644 --- a/backend/app/services/patreon_client.py +++ b/backend/app/services/patreon_client.py @@ -39,6 +39,8 @@ from urllib.parse import parse_qs, urlsplit import requests +from ..utils.paths import safe_ext + log = logging.getLogger(__name__) _POSTS_URL = "https://www.patreon.com/api/posts" @@ -99,11 +101,6 @@ _FILEHASH_RE = re.compile(r"([0-9a-fA-F]{32})") # by filehash. Tolerant of attribute ordering and single/double quotes. _CONTENT_IMG_RE = re.compile(r"]*?\bsrc=[\"']([^\"']+)[\"']", re.IGNORECASE) -# A bounded, sane extension parse (see importer._safe_ext for the FC gotcha: -# URL-encoded basenames make Path.suffix return base64-ish junk). We only ever -# use this for a fallback filename, never a network call. -_MAX_EXT_LEN = 16 - class PatreonAPIError(Exception): """Base for native Patreon client failures. @@ -187,15 +184,6 @@ def _filehash(url: str) -> str | None: return match.group(1).lower() if match else None -def _safe_ext(name: str) -> str: - suffix = Path(name).suffix.lower() - if not suffix or len(suffix) > _MAX_EXT_LEN: - return "" - if not all(c.isalnum() for c in suffix[1:]): - return "" - return suffix - - def _basename_from_url(url: str) -> str: """Derive a sane filename from a URL when the media has no file_name. @@ -206,7 +194,7 @@ def _basename_from_url(url: str) -> str: path = urlsplit(url).path base = os.path.basename(path) if base: - ext = _safe_ext(base) + ext = safe_ext(base) stem = base[: -len(Path(base).suffix)] if Path(base).suffix else base # Keep the stem bounded; URL-encoded stems can be enormous. stem = stem[:120] or "file" diff --git a/backend/app/services/patreon_downloader.py b/backend/app/services/patreon_downloader.py index ad001c8..06be462 100644 --- a/backend/app/services/patreon_downloader.py +++ b/backend/app/services/patreon_downloader.py @@ -31,7 +31,6 @@ import contextlib import json import logging import os -import shutil import subprocess import time from collections.abc import Callable @@ -42,7 +41,7 @@ from urllib.parse import urlsplit import requests -from .file_validator import is_validatable, validate_file +from .file_validator import is_validatable, quarantine_file, validate_file from .patreon_client import ( _BACKOFF_CAP_SECONDS, _load_session, @@ -268,7 +267,9 @@ class PatreonDownloader: out_path = Path(out_path) else: out_path = self._fetch_get(media.url, media_path) - reason, quarantine_dest = self._validate_path(out_path, artist_slug) + reason, quarantine_dest = self._validate_path( + out_path, artist_slug, media.url + ) if reason is not None: # Quarantined (corrupt/invalid) — distinct from a download error # so the run can report a real files_quarantined count + paths. @@ -426,16 +427,16 @@ class PatreonDownloader: # -- validation -------------------------------------------------------- def _validate_path( - self, path: Path, artist_slug: str + self, path: Path, artist_slug: str, source_url: str | None = None ) -> tuple[str | None, Path | None]: """Validate a freshly-written file; quarantine if bad. - Mirrors gallery_dl._validate_and_quarantine: fail-open for unknown - formats, move the corrupt file to _quarantine//patreon. Returns - `(reason, quarantine_dest)` when quarantined (dest is the original path if - the move itself failed), else `(None, None)` (ok / not validatable / - disabled). plan #704: the dest is surfaced so the run reports a real - quarantined-paths list instead of a blank count. + Uses the shared `file_validator.quarantine_file` — same move + provenance + sidecar gallery-dl writes (the native path used to skip the sidecar; that + parity gap is closed here). Returns `(reason, quarantine_dest)` when + quarantined (dest is the original path if the move itself failed), else + `(None, None)` (ok / not validatable / disabled). plan #704: the dest is + surfaced so the run reports a real quarantined-paths list. """ if not self._validate or not is_validatable(path): return None, None @@ -446,20 +447,11 @@ class PatreonDownloader: return None, None if result.ok: return None, None - quarantine_root = self.images_root / "_quarantine" / artist_slug / "patreon" - dest = path - try: - quarantine_root.mkdir(parents=True, exist_ok=True) - dest = quarantine_root / path.name - counter = 1 - while dest.exists(): - dest = quarantine_root / f"{path.stem}.{counter}{path.suffix}" - counter += 1 - shutil.move(str(path), str(dest)) - except OSError as exc: - log.error("Failed to quarantine %s: %s. File left in place.", path, exc) - dest = path - return (result.reason or "validation failed"), dest + dest = quarantine_file( + self.images_root, path, artist_slug, "patreon", + url=source_url, result=result, + ) + return (result.reason or "validation failed"), (dest or path) # -- sidecar ----------------------------------------------------------- diff --git a/backend/app/utils/paths.py b/backend/app/utils/paths.py index c19b7f8..d9bc48e 100644 --- a/backend/app/utils/paths.py +++ b/backend/app/utils/paths.py @@ -2,6 +2,26 @@ from pathlib import Path +_MAX_EXT_LEN = 16 + + +def safe_ext(name: str | Path) -> str: + """Conservatively extract a short, alphanumeric file extension. + + gallery-dl and Patreon CDN URLs produce basenames with URL-encoded + query-string artifacts, so `Path.suffix` can return 50+ chars of base64-ish + junk that blows bounded VARCHAR columns (e.g. PostAttachment.ext varchar(32)). + Accept only a suffix ≤16 chars whose post-dot characters are all alphanumeric; + otherwise return "" (no known extension). Operator-flagged 2026-05-25 — ONE + impl for the importer and the native Patreon client. + """ + suffix = Path(name).suffix.lower() + if not suffix or len(suffix) > _MAX_EXT_LEN: + return "" + if not all(c.isalnum() for c in suffix[1:]): + return "" + return suffix + def derive_subdir(source_path: Path, import_root: Path) -> str: """Returns the relative subdirectory of source_path under import_root. diff --git a/tests/test_patreon_downloader.py b/tests/test_patreon_downloader.py index e606489..774f6fe 100644 --- a/tests/test_patreon_downloader.py +++ b/tests/test_patreon_downloader.py @@ -305,6 +305,28 @@ def test_validation_quarantines_corrupt(tmp_path): assert any(quarantine.iterdir()) +def test_quarantine_writes_provenance_sidecar(tmp_path): + """A1 parity fix: the native path now writes the same `.quarantine.json` + provenance sidecar gallery-dl writes (it skipped it before the shared + file_validator.quarantine_file helper).""" + import json + + bad = b"not a real png" + url = "https://cdn.patreon.com/corrupt.png" + session = _FakeSession({url: bad}) + dl = _downloader(tmp_path, session=session, validate=True) + outcomes = dl.download_post(_post(), [_img("corrupt.png", url=url)], "artist-x") + + dest = outcomes[0].path + sidecar = dest.with_suffix(dest.suffix + ".quarantine.json") + assert sidecar.is_file() + meta = json.loads(sidecar.read_text()) + assert meta["source_url"] == url + assert meta["platform"] == "patreon" + assert meta["artist_slug"] == "artist-x" + assert meta["reason"] + + def test_validation_off_keeps_bytes(tmp_path): bad = b"not a real png" session = _FakeSession({"https://cdn.patreon.com/x.png": bad})