From d472b63cbff0b20dbd09f8a543b591f73d197725 Mon Sep 17 00:00:00 2001 From: jay Date: Sun, 7 Jun 2026 13:20:12 -0400 Subject: [PATCH] Validate images actually load (fix overcounted coverage) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stored og:image isn't proof it renders: signed/hotlink-protected URLs (e.g. the Guardian's i.guim.co.uk) 401 on a direct browser load, so they counted toward coverage yet always fell back. Now fetch_og_image confirms the image truly returns 200 + image/* (requested no-referrer, same SSRF-safe redirect handling) before storing it. Add prune_broken_images() to clear already-stored URLs that no longer load, so coverage is honest and those cards show the placeholder cleanly. The browser onerror→placeholder remains the final safety net. Co-Authored-By: Claude Opus 4.8 (1M context) --- goodnews/enrich.py | 62 ++++++++++++++++++++++++++++++++++++- tests/test_enrich_images.py | 12 +++++++ 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/goodnews/enrich.py b/goodnews/enrich.py index 3b329d7..667dbdc 100644 --- a/goodnews/enrich.py +++ b/goodnews/enrich.py @@ -163,10 +163,70 @@ def fetch_og_image(url: str | None) -> str | None: body = response.read(MAX_BYTES) finally: response.close() - return og_image_from_html(body) + image = og_image_from_html(body) + # A stored URL is not proof it renders — confirm it actually loads. + return image if (image and _image_loads(image)) else None return None # too many redirects +def _image_loads(url: str) -> bool: + """Confirm an image URL truly returns an image (HTTP 200 + image/* type). + + Many publishers serve a signed or hotlink-protected og:image that 401/403s + on a direct request (e.g. the Guardian's i.guim.co.uk), so storing the URL + would overstate coverage and the card would never render it. We request as + the browser does — no referrer — with the same per-hop host safety as the + page fetch. Returns False on any error. + """ + opener = urllib.request.build_opener(_NoRedirect) + for _ in range(MAX_REDIRECTS + 1): + if not url: + return False + parts = urlsplit(url) + if parts.scheme not in ("http", "https") or not _host_is_public(parts.hostname): + return False + request = urllib.request.Request(url, headers={"User-Agent": USER_AGENT, "Accept": "image/*,*/*"}) + try: + response = opener.open(request, timeout=TIMEOUT) + except (urllib.error.URLError, OSError, ValueError): + return False + try: + status = getattr(response, "status", 200) or 200 + if status in (301, 302, 303, 307, 308): + location = response.headers.get("Location") + if not location: + return False + url = urljoin(url, location) + continue + ctype = (response.headers.get("Content-Type") or "").lower() + return status == 200 and ctype.startswith("image/") + finally: + response.close() + return False + + +def prune_broken_images(conn: sqlite3.Connection, check=_image_loads, limit: int = 3000) -> int: + """Clear stored image URLs that no longer load (signed/expired/hotlink- + protected), so coverage is honest and those cards fall back to the calm + placeholder cleanly instead of attempting a doomed fetch. Returns count cleared. + """ + rows = conn.execute( + "SELECT id, image_url FROM articles WHERE image_url IS NOT NULL AND image_url != '' " + "ORDER BY id DESC LIMIT ?", + (limit,), + ).fetchall() + cleared = 0 + for row in rows: + if not check(row["image_url"]): + conn.execute( + "UPDATE articles SET image_url = NULL, image_checked_at = CURRENT_TIMESTAMP WHERE id = ?", + (row["id"],), + ) + cleared += 1 + conn.commit() + return cleared + + def enrich_brief_images( conn: sqlite3.Connection, brief_date: str, fetch=fetch_og_image, limit: int = 7, retry_days: int = 2 ) -> int: diff --git a/tests/test_enrich_images.py b/tests/test_enrich_images.py index 19de071..a13947d 100644 --- a/tests/test_enrich_images.py +++ b/tests/test_enrich_images.py @@ -47,6 +47,18 @@ def test_enrich_records_checked_even_when_none_found(tmp_path): assert again is False # still within retry window +def test_prune_clears_only_broken_images(tmp_path): + c = _setup(tmp_path) + _add(c, 1, image="http://good/a.jpg") + _add(c, 2, image="http://broken/b.jpg") + _add(c, 3, image=None) # nothing to check + # Pretend only the 'good' host loads. + cleared = enrich.prune_broken_images(c, check=lambda u: "good" in u) + assert cleared == 1 + assert c.execute("SELECT image_url FROM articles WHERE id=1").fetchone()[0] == "http://good/a.jpg" + assert c.execute("SELECT image_url FROM articles WHERE id=2").fetchone()[0] is None + + def test_backfill_only_targets_summarized_accepted_imageless(tmp_path): c = _setup(tmp_path) _add(c, 1, image=None, summarized=True, accepted=1) # eligible