diff --git a/app/verify/cli.py b/app/verify/cli.py index 45c3e88..310f92f 100644 --- a/app/verify/cli.py +++ b/app/verify/cli.py @@ -409,9 +409,18 @@ def cmd_check_urls(args: argparse.Namespace) -> int: max_workers=args.workers, min_interval=args.min_interval, ) + # A rate-limited answer is not a verdict — leave it out so the next run asks + # again instead of parking the URL as dead for the whole TTL. + throttled = sum(1 for r in results if r.transient) for r in results: - cache[r.url] = http_check.result_to_entry(r, ts) + if not r.transient: + cache[r.url] = http_check.result_to_entry(r, ts) http_check.save_cache(cache) + if throttled: + print( + f"note: {throttled} URL(s) rate-limited after retries; " + "not cached, will retry next run" + ) print(f"cache: wrote {len(cache)} URL result(s) to data/_verify/state/url_cache.jsonl") _summarize_cache(cache, targets) return 0 diff --git a/app/verify/http_check.py b/app/verify/http_check.py index d56b754..eeb3e19 100644 --- a/app/verify/http_check.py +++ b/app/verify/http_check.py @@ -32,6 +32,17 @@ ) +# "Come back later" is not "this link is dead". A host that rate-limits us says +# nothing about whether the page exists, so these answers must never be cached as +# a verdict — otherwise one impatient run marks a whole host dead for a TTL. +TRANSIENT_STATUSES = frozenset({429, 503}) +RETRY_ATTEMPTS = 3 +RETRY_BACKOFF_S = (2.0, 6.0) +MAX_RETRY_AFTER_S = 15.0 +# How much to slow a host down for the rest of the run once it has pushed back. +RATE_LIMIT_PENALTY = 4.0 + + class CheckResult(NamedTuple): url: str status: int | None @@ -39,6 +50,10 @@ class CheckResult(NamedTuple): alive: bool reason: str + @property + def transient(self) -> bool: + return self.status in TRANSIENT_STATUSES + # --- opener abstraction (injectable for tests) ----------------------------------- @@ -93,10 +108,21 @@ def classify(original_url: str, status: int | None, final_url: str | None) -> tu return True, f"http-{status}" -def check_one(url: str, opener: Any) -> CheckResult: - """HEAD first; fall back to GET when HEAD is rejected (405/403) or errors.""" +def _retry_after_seconds(exc: Exception) -> float | None: + """Seconds requested by a ``Retry-After`` header, clamped to something sane.""" + headers = getattr(exc, "headers", None) + raw = headers.get("Retry-After") if headers is not None else None + try: + return min(float(raw), MAX_RETRY_AFTER_S) if raw is not None else None + except (TypeError, ValueError): + return None # HTTP-date form; fall back to our own backoff + + +def _attempt(url: str, opener: Any) -> tuple[int | None, str | None, float | None]: + """One HEAD-then-GET pass. Returns (status, final_url, retry_after).""" status: int | None = None final: str | None = None + retry_after: float | None = None for method in ("HEAD", "GET"): try: status, final = opener.open(url, method) @@ -107,10 +133,31 @@ def check_one(url: str, opener: Any) -> CheckResult: code = getattr(exc, "code", None) if isinstance(code, int): status, final = code, getattr(exc, "url", None) or url + retry_after = _retry_after_seconds(exc) if method == "HEAD" and code in (400, 403, 405, 501): continue break status, final = None, None + return status, final, retry_after + + +def check_one( + url: str, opener: Any, *, on_rate_limit: Callable[[str], None] | None = None +) -> CheckResult: + """HEAD first; fall back to GET when HEAD is rejected (405/403) or errors. + + A rate-limit answer (429/503) is retried with backoff — the host is telling us + to wait, not that the page is gone. + """ + status = final = retry_after = None + for attempt in range(RETRY_ATTEMPTS): + status, final, retry_after = _attempt(url, opener) + if status not in TRANSIENT_STATUSES: + break + if on_rate_limit is not None: + on_rate_limit(host_of(url)) + if attempt < RETRY_ATTEMPTS - 1: + time.sleep(retry_after if retry_after is not None else RETRY_BACKOFF_S[attempt]) alive, reason = classify(url, status, final) return CheckResult(url, status, final, alive, reason) @@ -124,13 +171,26 @@ class HostRateLimiter: def __init__(self, min_interval: float = 1.0) -> None: self.min_interval = min_interval self._last: dict[str, float] = {} + self._interval: dict[str, float] = {} self._lock = threading.Lock() + def interval_for(self, host: str) -> float: + return self._interval.get(host, self.min_interval) + + def back_off(self, host: str, factor: float = RATE_LIMIT_PENALTY) -> None: + """A host pushed back — slow it down for the rest of the run. + + Without this, one rate-limited host keeps being hammered at the global + pace and every subsequent URL on it comes back 429. + """ + with self._lock: + self._interval[host] = self.interval_for(host) * factor + def wait(self, host: str) -> None: with self._lock: now = time.time() prev = self._last.get(host, 0.0) - sleep_for = max(0.0, self.min_interval - (now - prev)) + sleep_for = max(0.0, self.interval_for(host) - (now - prev)) self._last[host] = now + sleep_for if sleep_for > 0: time.sleep(sleep_for) @@ -172,7 +232,7 @@ def _get_opener() -> Any: def _task(url: str) -> CheckResult: limiter.wait(host_of(url)) - return check_one(url, _get_opener()) + return check_one(url, _get_opener(), on_rate_limit=limiter.back_off) if not urls: return [] @@ -184,7 +244,18 @@ def _task(url: str) -> CheckResult: def load_cache(path: Path = URL_CACHE_PATH) -> dict[str, dict[str, Any]]: - return {e["url"]: e for e in ledger.iter_entries(path) if isinstance(e.get("url"), str)} + """Load the cache, dropping rate-limit answers written by older runs. + + A 429/503 is not a verdict, so an entry holding one is not a cache hit — + it is a URL we still have to check. Filtering on load heals a cache that a + previous run poisoned (3,998 GSMArena pages were parked as dead this way, + all of which answer 200 when asked at a civil pace). + """ + return { + e["url"]: e + for e in ledger.iter_entries(path) + if isinstance(e.get("url"), str) and e.get("status") not in TRANSIENT_STATUSES + } def _parse_ts(ts: str) -> datetime | None: diff --git a/tests/verify/test_http_check.py b/tests/verify/test_http_check.py index a2852b5..315ef47 100644 --- a/tests/verify/test_http_check.py +++ b/tests/verify/test_http_check.py @@ -103,3 +103,67 @@ def test_cache_roundtrip(): assert loaded["https://x.com/y"]["alive"] is True finally: path.unlink(missing_ok=True) + + +class _Http429(Exception): + """urllib-shaped 429, optionally carrying a Retry-After header.""" + + def __init__(self, url, retry_after=None): + super().__init__("Too Many Requests") + self.code = 429 + self.url = url + self.headers = {"Retry-After": retry_after} if retry_after else {} + + +class FlakyOpener(FakeOpener): + """Rate-limits the first ``fail_times`` calls, then answers normally.""" + + def __init__(self, table, fail_times): + super().__init__(table) + self.remaining = fail_times + + def open(self, url, method): + self.calls.append((url, method)) + if self.remaining > 0: + self.remaining -= 1 + raise _Http429(url, retry_after="0") + return self.table[url] + + +def test_rate_limit_is_retried_then_succeeds(monkeypatch): + monkeypatch.setattr(http_check.time, "sleep", lambda _s: None) + url = "https://www.gsmarena.com/x-1.php" + op = FlakyOpener({url: (200, url)}, fail_times=2) + [res] = http_check.check_urls([url], opener_factory=lambda: op, min_interval=0) + assert res.alive and res.status == 200 and not res.transient + + +def test_persistent_rate_limit_is_transient_not_dead(monkeypatch): + monkeypatch.setattr(http_check.time, "sleep", lambda _s: None) + url = "https://www.gsmarena.com/y-2.php" + op = FlakyOpener({url: (200, url)}, fail_times=99) + [res] = http_check.check_urls([url], opener_factory=lambda: op, min_interval=0) + assert res.status == 429 and res.transient # caller must not cache this as a verdict + + +def test_rate_limit_slows_the_host_down(monkeypatch): + monkeypatch.setattr(http_check.time, "sleep", lambda _s: None) + url = "https://www.gsmarena.com/z-3.php" + limiter = http_check.HostRateLimiter(min_interval=1.0) + op = FlakyOpener({url: (200, url)}, fail_times=1) + http_check.check_urls([url], opener_factory=lambda: op, limiter=limiter) + assert limiter.interval_for("gsmarena.com") > 1.0 + + +def test_cached_rate_limit_entries_are_not_cache_hits(tmp_path): + path = tmp_path / "url_cache.jsonl" + path.write_text( + '{"url": "https://www.gsmarena.com/a-1.php", "status": 429, "alive": false,' + ' "reason": "http-429", "checked_at": "2026-08-03T00:00:00Z"}\n' + '{"url": "https://en.wikipedia.org/wiki/X", "status": 200, "alive": true,' + ' "reason": "http-200", "checked_at": "2026-08-03T00:00:00Z"}\n', + encoding="utf-8", + ) + cache = http_check.load_cache(path) + assert "https://en.wikipedia.org/wiki/X" in cache + assert "https://www.gsmarena.com/a-1.php" not in cache # a 429 is not an answer