From 2765c464bde900075529a1573a6a2e9970ffc147 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 25 Apr 2026 22:55:52 -0400 Subject: [PATCH] feat(downloader): validate and quarantine truncated files post-download MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After gallery-dl returns, parse stdout for written file paths and run the magic-byte validator on each. Files that fail are moved to {download_path}/_quarantine/{subscription}/{platform}/ with a sidecar JSON capturing the source URL, validation reason, and original path — enough for an operator to redownload-from-source or delete. Adds ErrorType.VALIDATION_FAILED. A successful gallery-dl run that produced quarantined files is now a soft failure (success=False, error_message names the dominant failure reason and count) so the source's error_count ticks up and the dashboard surfaces it. Gated by download.validate_files setting (default True). files_quarantined and quarantined_paths are persisted into download metadata for the UI/API to consume. Co-Authored-By: Claude Opus 4.7 --- backend/app/models/setting.py | 4 + backend/app/services/gallery_dl.py | 208 +++++++++++++++++- backend/app/tasks/downloads.py | 10 +- .../services/test_gallery_dl_validation.py | 183 +++++++++++++++ 4 files changed, 401 insertions(+), 4 deletions(-) create mode 100644 backend/tests/services/test_gallery_dl_validation.py diff --git a/backend/app/models/setting.py b/backend/app/models/setting.py index a2427e1..2707191 100644 --- a/backend/app/models/setting.py +++ b/backend/app/models/setting.py @@ -41,6 +41,7 @@ DEFAULT_SETTINGS = { "download.rate_limit": 3.0, "download.retry_count": 3, "download.schedule_interval": 28800, # 8 hours + "download.validate_files": True, "notification.enabled": False, "notification.webhook_url": None, "dashboard.failure_threshold": 5, @@ -51,3 +52,6 @@ EXTENSION_API_KEY_SETTING = "security.extension_api_key" # Key for cached storage stats STORAGE_STATS_SETTING = "cache.storage_stats" + +# Key for cached library-validation report (last sweep) +LIBRARY_VALIDATION_REPORT_SETTING = "cache.library_validation_report" diff --git a/backend/app/services/gallery_dl.py b/backend/app/services/gallery_dl.py index b79d416..f9c44d8 100644 --- a/backend/app/services/gallery_dl.py +++ b/backend/app/services/gallery_dl.py @@ -33,6 +33,7 @@ class ErrorType(str, Enum): TIMEOUT = "timeout" HTTP_ERROR = "http_error" UNSUPPORTED_URL = "unsupported_url" + VALIDATION_FAILED = "validation_failed" UNKNOWN_ERROR = "unknown_error" @@ -102,6 +103,26 @@ class SourceConfig: } +def _summarize_validation_failures(failures: list[dict]) -> str: + """Human-readable error_message for a run that quarantined files. + + Picks the dominant failure reason (most common) and counts so the + dashboard's error column says something useful — "3 files truncated + (missing JPEG EOI marker)" rather than a generic VALIDATION_FAILED. + """ + if not failures: + return "Validation failed" + reasons: dict[str, int] = {} + for f in failures: + key = f.get("reason") or "unknown" + reasons[key] = reasons.get(key, 0) + 1 + top_reason, top_count = max(reasons.items(), key=lambda kv: kv[1]) + n = len(failures) + if top_count == n: + return f"{n} file{'s' if n != 1 else ''} quarantined: {top_reason}" + return f"{n} files quarantined ({top_count}× {top_reason}, mixed)" + + @dataclass class DownloadResult: """Result of a gallery-dl download execution.""" @@ -112,6 +133,8 @@ class DownloadResult: # Output details files_downloaded: int = 0 + files_quarantined: int = 0 + quarantined_paths: list[str] = field(default_factory=list) stdout: str = "" stderr: str = "" return_code: int = 0 @@ -258,15 +281,24 @@ class GalleryDLService: }, } - def __init__(self, rate_limit: Optional[float] = None): + def __init__( + self, + rate_limit: Optional[float] = None, + validate_files: bool = True, + ): """Initialize the GalleryDL service. Args: rate_limit: Override for download rate limit (seconds between requests). If None, uses environment default. + validate_files: When True, run magic-byte validation on each file + gallery-dl writes and quarantine truncated files. Caller + (tasks.downloads) typically reads this from the + `download.validate_files` DB setting. """ self.settings = get_settings() self._rate_limit = rate_limit if rate_limit is not None else self.settings.download_rate_limit + self._validate_files = validate_files self._base_config = self._load_base_config() def _load_base_config(self) -> dict: @@ -603,6 +635,121 @@ class GalleryDLService: return sum(1 for line in stdout.splitlines() if line.strip().startswith("/")) + def _written_paths(self, stdout: str) -> list[Path]: + """Extract absolute file paths gallery-dl wrote during this run. + + Mirrors `_count_downloaded_files` but returns the paths themselves so + post-download validation only inspects files we actually just wrote + (skips on '#' lines and log noise are excluded). Caller is responsible + for filtering further by extension if needed. + """ + if not stdout: + return [] + return [ + Path(stripped) + for line in stdout.splitlines() + if (stripped := line.strip()).startswith("/") + ] + + def _validate_and_quarantine( + self, + written_paths: list[Path], + subscription_name: str, + platform: str, + url: str, + ) -> tuple[list[str], list[dict]]: + """Validate freshly-written files; quarantine any that fail. + + Returns (quarantined_relpaths, failure_details). Each failure detail + is a dict suitable for logging/persisting: {path, format, reason, size}. + Files that pass validation or aren't a validatable format are left in + place. Failures are moved to {download_path}/_quarantine// + / along with a sidecar JSON capturing the source URL, the + validation reason, and the original on-disk path so an operator can + decide to redownload-from-source or delete. + """ + # Local import keeps the module import-cycle clean and lets tests + # patch validation behavior. + from app.services.file_validator import is_validatable, validate_file + + quarantined_relpaths: list[str] = [] + failures: list[dict] = [] + + if not written_paths: + return quarantined_relpaths, failures + + quarantine_root = ( + Path(self.settings.download_path) + / "_quarantine" + / subscription_name + / platform + ) + + for path in written_paths: + if not is_validatable(path): + continue + try: + result = validate_file(path) + except Exception as e: + # Validator must never crash the download flow. + logger.warning(f"Validator raised on {path}: {e}") + continue + + if result.ok: + continue + + # Move the bad file out of the library so downstream consumers + # (thumbnailer, ML, sync) don't trip on it. + try: + quarantine_root.mkdir(parents=True, exist_ok=True) + dest = quarantine_root / path.name + # Avoid overwriting prior quarantine entries with the same + # filename — append a numeric suffix until we find a free slot. + 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, + "subscription": subscription_name, + "platform": platform, + "format": result.format, + "reason": result.reason, + "size": result.size, + "quarantined_at": utcnow().isoformat(), + }, + indent=2, + ) + ) + except OSError as e: + # If we can't move it, log loudly but keep going. A failed + # quarantine isn't worse than the pre-fix status quo. + logger.error( + f"Failed to quarantine {path}: {e}. File left in place." + ) + continue + + logger.warning( + f"Quarantined corrupt file: {path} → {dest} " + f"({result.format}: {result.reason})" + ) + quarantined_relpaths.append(str(dest)) + failures.append( + { + "path": str(path), + "format": result.format, + "reason": result.reason, + "size": result.size, + } + ) + + return quarantined_relpaths, failures + def _compute_run_stats(self, return_code: int, stdout: str, stderr: str) -> dict: """Summarize a gallery-dl run as structured counts for UI/analytics. @@ -647,6 +794,9 @@ class GalleryDLService: "per_item_failures": per_item_failures, "warning_count": warning_count, "tier_gated_count": tier_gated_count, + # quarantined_count is overlaid by the caller (tasks.downloads) + # when persisting metadata; surfaced via DownloadResult.files_quarantined + # because run_stats has no access to the live filesystem. } @staticmethod @@ -796,14 +946,55 @@ class GalleryDLService: if proc.stderr: logger.debug(f"STDERR for {subscription_name}/{platform}:\n{proc.stderr}") + # Run validation on every fresh write before classifying. Files + # gallery-dl wrote and we then quarantined no longer count as + # "downloaded" — counting them would understate how many files + # need re-downloading and overstate the run's success. + quarantined_paths: list[str] = [] + quarantine_failures: list[dict] = [] + if self._validate_files: + quarantined_paths, quarantine_failures = self._validate_and_quarantine( + written_paths=self._written_paths(proc.stdout), + subscription_name=subscription_name, + platform=platform, + url=url, + ) + + files_written = self._count_downloaded_files(proc.stdout) + files_quarantined = len(quarantined_paths) + files_kept = max(0, files_written - files_quarantined) + # Determine success if proc.returncode == 0: + # Exit 0 with quarantines: the run technically succeeded but + # delivered partially-corrupt content. Surface as a soft + # failure so the source's error_count ticks up and the user + # sees it on the dashboard, but keep the per-item details + # (quarantined_paths) for the maintenance UI. + if files_quarantined > 0: + return DownloadResult( + success=False, + url=url, + subscription_name=subscription_name, + platform=platform, + files_downloaded=files_kept, + files_quarantined=files_quarantined, + quarantined_paths=quarantined_paths, + stdout=proc.stdout, + stderr=proc.stderr, + return_code=proc.returncode, + error_type=ErrorType.VALIDATION_FAILED, + error_message=_summarize_validation_failures(quarantine_failures), + duration_seconds=duration, + started_at=started_at, + completed_at=completed_at, + ) return DownloadResult( success=True, url=url, subscription_name=subscription_name, platform=platform, - files_downloaded=self._count_downloaded_files(proc.stdout), + files_downloaded=files_kept, stdout=proc.stdout, stderr=proc.stderr, return_code=proc.returncode, @@ -822,12 +1013,23 @@ class GalleryDLService: # current subscription tier can't see the requested posts). success = error_type in (ErrorType.NO_NEW_CONTENT, ErrorType.TIER_LIMITED) + # If a non-zero exit also produced quarantined files, prefer the + # validation-failed signal — it's actionable (the file needs to + # be redownloaded) and tells the operator the partial-write + # problem is happening, not just the categorized exit-code error. + if files_quarantined > 0: + error_type = ErrorType.VALIDATION_FAILED + error_message = _summarize_validation_failures(quarantine_failures) + success = False + return DownloadResult( success=success, url=url, subscription_name=subscription_name, platform=platform, - files_downloaded=self._count_downloaded_files(proc.stdout) if success else 0, + files_downloaded=files_kept if success else 0, + files_quarantined=files_quarantined, + quarantined_paths=quarantined_paths, stdout=proc.stdout, stderr=proc.stderr, return_code=proc.returncode, diff --git a/backend/app/tasks/downloads.py b/backend/app/tasks/downloads.py index 927f8f0..9b9ab6a 100644 --- a/backend/app/tasks/downloads.py +++ b/backend/app/tasks/downloads.py @@ -319,6 +319,9 @@ async def _download_source_async(source_id: int, download_id: int = None) -> dic # Get rate limit from database settings db_rate_limit = await _get_db_setting(session, "download.rate_limit", settings.download_rate_limit) + db_validate_files = bool( + await _get_db_setting(session, "download.validate_files", True) + ) # Session is now closed - DB connection released finally: @@ -331,7 +334,10 @@ async def _download_source_async(source_id: int, download_id: int = None) -> dic resolved_campaign_id: Optional[str] = None try: - gdl_service = GalleryDLService(rate_limit=db_rate_limit) + gdl_service = GalleryDLService( + rate_limit=db_rate_limit, + validate_files=db_validate_files, + ) attempt = await _execute_download_with_retry( gdl_service=gdl_service, platform=source_platform, @@ -409,6 +415,7 @@ async def _download_source_async(source_id: int, download_id: int = None) -> dic run_stats = gdl_service._compute_run_stats( dl_result.return_code, dl_result.stdout, dl_result.stderr ) + run_stats["quarantined_count"] = dl_result.files_quarantined stderr_summary = gdl_service._extract_errors_warnings(dl_result.stderr) download.metadata_ = { "stdout": gdl_service._truncate_log(dl_result.stdout) or None, @@ -416,6 +423,7 @@ async def _download_source_async(source_id: int, download_id: int = None) -> dic "stderr_errors_warnings": stderr_summary or None, "run_stats": run_stats, "duration_seconds": dl_result.duration_seconds, + "quarantined_paths": dl_result.quarantined_paths or None, } if dl_result.success: diff --git a/backend/tests/services/test_gallery_dl_validation.py b/backend/tests/services/test_gallery_dl_validation.py new file mode 100644 index 0000000..6393ae4 --- /dev/null +++ b/backend/tests/services/test_gallery_dl_validation.py @@ -0,0 +1,183 @@ +"""Tests for GalleryDLService's post-download validation hook. + +Focus: a successful gallery-dl run that wrote a truncated JPEG must +result in DownloadResult.success=False with VALIDATION_FAILED, the bad +file moved to the quarantine tree, and a sidecar JSON written next to +it. A run with no validation issues must behave exactly as before. +""" +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from app.services.file_validator import JPEG_HEAD, JPEG_TAIL +from app.services.gallery_dl import ( + DownloadResult, + ErrorType, + GalleryDLService, + _summarize_validation_failures, +) + + +def _make_service(tmp_path: Path, validate: bool = True) -> GalleryDLService: + """Service wired to a tmp download root, no real filesystem config.""" + fake_settings = SimpleNamespace( + download_path=str(tmp_path / "downloads"), + config_path=str(tmp_path / "config"), + download_rate_limit=1.0, + ) + Path(fake_settings.download_path).mkdir(parents=True, exist_ok=True) + Path(fake_settings.config_path).mkdir(parents=True, exist_ok=True) + with patch("app.services.gallery_dl.get_settings", return_value=fake_settings), \ + patch.object(GalleryDLService, "_load_base_config", return_value={}): + return GalleryDLService(rate_limit=1.0, validate_files=validate) + + +def _write_jpeg(path: Path, *, truncated: bool) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + body = JPEG_HEAD + b"\x00" * 64 + if not truncated: + body += JPEG_TAIL + path.write_bytes(body) + + +# --- _validate_and_quarantine --- + + +def test_quarantines_truncated_jpeg(tmp_path): + service = _make_service(tmp_path) + bad = Path(service.settings.download_path) / "Artist" / "patreon" / "01.jpg" + _write_jpeg(bad, truncated=True) + + paths, failures = service._validate_and_quarantine( + written_paths=[bad], + subscription_name="Artist", + platform="patreon", + url="https://patreon.com/post/1", + ) + + assert len(paths) == 1 + assert len(failures) == 1 + assert failures[0]["format"] == "jpeg" + # Original is gone, moved to _quarantine root + assert not bad.exists() + quarantine_root = ( + Path(service.settings.download_path) / "_quarantine" / "Artist" / "patreon" + ) + moved = quarantine_root / "01.jpg" + assert moved.exists() + sidecar = quarantine_root / "01.jpg.quarantine.json" + assert sidecar.exists() + assert "patreon.com/post/1" in sidecar.read_text() + + +def test_passes_through_well_formed_files(tmp_path): + service = _make_service(tmp_path) + good = Path(service.settings.download_path) / "Artist" / "patreon" / "01.jpg" + _write_jpeg(good, truncated=False) + + paths, failures = service._validate_and_quarantine( + written_paths=[good], + subscription_name="Artist", + platform="patreon", + url="https://patreon.com/post/1", + ) + + assert paths == [] + assert failures == [] + assert good.exists() # unchanged + + +def test_skips_unvalidatable_extensions(tmp_path): + service = _make_service(tmp_path) + sidecar = Path(service.settings.download_path) / "Artist" / "patreon" / "01.json" + sidecar.parent.mkdir(parents=True, exist_ok=True) + sidecar.write_bytes(b'{"junk": true}') + + paths, _ = service._validate_and_quarantine( + written_paths=[sidecar], + subscription_name="Artist", + platform="patreon", + url="https://example/1", + ) + assert paths == [] + assert sidecar.exists() + + +def test_quarantine_filename_collision_does_not_overwrite(tmp_path): + service = _make_service(tmp_path) + quarantine_root = ( + Path(service.settings.download_path) / "_quarantine" / "Artist" / "patreon" + ) + quarantine_root.mkdir(parents=True, exist_ok=True) + (quarantine_root / "01.jpg").write_bytes(b"PRIOR_QUARANTINE") + + bad = Path(service.settings.download_path) / "Artist" / "patreon" / "01.jpg" + _write_jpeg(bad, truncated=True) + + paths, _ = service._validate_and_quarantine( + written_paths=[bad], + subscription_name="Artist", + platform="patreon", + url="https://patreon.com/2", + ) + + # Original quarantine file untouched, new one suffixed. + assert (quarantine_root / "01.jpg").read_bytes() == b"PRIOR_QUARANTINE" + assert (quarantine_root / "01.1.jpg").exists() + assert paths and paths[0].endswith("01.1.jpg") + + +# --- _written_paths --- + + +def test_written_paths_extracts_only_absolute_paths(tmp_path): + service = _make_service(tmp_path) + stdout = ( + "/data/downloads/Artist/patreon/01.jpg\n" + "# /data/downloads/Artist/patreon/02.jpg\n" # archive skip — must be excluded + "/data/downloads/Artist/patreon/03.jpg\n" + "[patreon][info] some log line\n" + " /data/downloads/Artist/patreon/04.jpg \n" # surrounding whitespace + ) + paths = service._written_paths(stdout) + names = [p.name for p in paths] + assert names == ["01.jpg", "03.jpg", "04.jpg"] + + +# --- _summarize_validation_failures --- + + +def test_summary_with_uniform_reason(): + failures = [ + {"reason": "missing JPEG EOI marker (FF D9)"} for _ in range(3) + ] + msg = _summarize_validation_failures(failures) + assert "3 files" in msg + assert "EOI" in msg + + +def test_summary_with_mixed_reasons(): + failures = [ + {"reason": "missing JPEG EOI marker (FF D9)"}, + {"reason": "missing JPEG EOI marker (FF D9)"}, + {"reason": "zero-byte file"}, + ] + msg = _summarize_validation_failures(failures) + assert "3 files" in msg + assert "mixed" in msg + + +# --- validate_files=False respects opt-out --- + + +def test_validate_files_false_skips_quarantine(tmp_path): + service = _make_service(tmp_path, validate=False) + bad = Path(service.settings.download_path) / "Artist" / "patreon" / "01.jpg" + _write_jpeg(bad, truncated=True) + + # download() is async + spawns subprocess. Cheaper test: confirm the + # _validate_files flag is wired and the helper isn't called when false. + # We don't run download() here; the constructor flag is the contract. + assert service._validate_files is False