diff --git a/scripts/profile_sync.py b/scripts/profile_sync.py index a3b0010d..24681a3c 100644 --- a/scripts/profile_sync.py +++ b/scripts/profile_sync.py @@ -1723,8 +1723,8 @@ def build_report( anything that is not a browser. Letting the first refusal abort the report leaves the mirror unread and the profile unverifiable, so the repository is muted for the rest of the pass instead. A quota signal - still stops everything, because continuing would only burn the rest of - the budget on the same wall. + still ends the profile, and upstream stops asking that host, because + continuing would only burn the rest of the budget on the same wall. """ # Keyed by host as well as slug: a mirror carries the same slug on # another forge, and muting one must not silence the other. @@ -3847,7 +3847,7 @@ def _attach_detection( Kept on the report rather than printed, so --json carries them as the text report does. A forge that refuses is named on the report: the ref report already muted it, and one refusal must not end a pass over every - profile. A quota signal still stops everything. + profile. A quota signal still ends the profile. """ if not report.head or not (args.detect_new_files or args.watch_hashes): return @@ -3948,7 +3948,7 @@ def _print_extras( The ref report mutes a forge that refuses, but the extras read the same forge again, so a refusal here is reported on the profile's own line instead of ending a pass over every profile. A quota signal still - stops everything. + ends the profile. """ _print_detection(report) if not (args.full_diff or args.tree_diff) or not report.head: @@ -4165,8 +4165,6 @@ def main() -> None: profile_path, args.cache_dir, args.offline, args.dry_run ): print(f"{name}: {line}") - except upstream.RateLimitError: - raise except upstream.UpstreamError as exc: print(f"{name}: {exc}", file=sys.stderr) failed.add(name) @@ -4179,47 +4177,21 @@ def main() -> None: reports = [] for name, profile in selected.items(): + profile_path = Path(args.emulators_dir) / f"{name}.yml" try: - profile_path = Path(args.emulators_dir) / f"{name}.yml" - report = build_report( - name, profile, args.cache_dir, args.offline, - profile_path.read_text(encoding="utf-8") - if profile_path.is_file() else None, - ) - except upstream.RateLimitError: - raise - except upstream.NotCachedError as exc: - # Offline with a cache that never stored this file: the profile - # is unverified, not wrong. Reporting GONE sent readers after - # files that exist. - report = ProfileReport( - name=name, entries=[], counts={}, skipped=f"offline: {exc}" - ) - except upstream.GoneError as exc: - # The forge is not coming back: a takedown, or a host that no - # longer resolves. Saying so once in the summary beats repeating - # it on stderr every pass, and it keeps the profile out of the - # backlog, where nobody can act on it anyway. - report = ProfileReport( - name=name, entries=[], counts={}, skipped=f"upstream gone: {exc}" - ) - except upstream.UpstreamError as exc: - # One unreachable forge must not abandon the other profiles. + report = _build_full_report(args, name, profile, profile_path, failed) + except upstream.RateLimitError as exc: + # A quota is one forge's: git.libretro.com answering 429 said + # nothing about the 600 profiles on GitHub. upstream stops asking + # that host, so the rest of the pass costs it nothing. print(f"{name}: {exc}", file=sys.stderr) report = ProfileReport( - name=name, entries=[], counts={}, skipped=f"upstream error: {exc}" + name=name, entries=[], counts={}, + skipped=f"rate limited: {exc.host or exc}", ) failed.add(name) - if not report.skipped and profile_path.is_file(): - report.stale_prose = _read_stale_prose(name, profile_path, args, failed) - if args.check_version and profile.get("version_source"): - report.declared_version = str(profile.get("core_version") or "") or None - report.page_version = page_version(profile, args.cache_dir, args.offline) - report.no_genuine_copy = str( - profile["version_source"].get("no_genuine_copy") or "") or None - if args.check_version: - _attach_version(args, profile, report) - _attach_detection(args, profile, report) + reports.append(report) + continue reports.append(report) if writes: try: @@ -4242,6 +4214,61 @@ def main() -> None: raise SystemExit(1) +def _build_full_report( + args: argparse.Namespace, + name: str, + profile: dict, + profile_path: Path, + failed: set[str], +) -> ProfileReport: + """The report of one profile, with every section the flags ask for. + + A rate limit propagates: the caller decides what one forge's quota + means for the pass. Any other forge failure becomes a skip. + """ + try: + report = build_report( + name, profile, args.cache_dir, args.offline, + profile_path.read_text(encoding="utf-8") + if profile_path.is_file() else None, + ) + except upstream.RateLimitError: + raise + except upstream.NotCachedError as exc: + # Offline with a cache that never stored this file: the profile + # is unverified, not wrong. Reporting GONE sent readers after + # files that exist. + report = ProfileReport( + name=name, entries=[], counts={}, skipped=f"offline: {exc}" + ) + except upstream.GoneError as exc: + # The forge is not coming back: a takedown, or a host that no + # longer resolves. Saying so once in the summary beats repeating + # it on stderr every pass, and it keeps the profile out of the + # backlog, where nobody can act on it anyway. + report = ProfileReport( + name=name, entries=[], counts={}, skipped=f"upstream gone: {exc}" + ) + except upstream.UpstreamError as exc: + # One unreachable forge must not abandon the other profiles. + print(f"{name}: {exc}", file=sys.stderr) + report = ProfileReport( + name=name, entries=[], counts={}, skipped=f"upstream error: {exc}" + ) + failed.add(name) + if not report.skipped and profile_path.is_file(): + report.stale_prose = _read_stale_prose(name, profile_path, args, failed) + if args.check_version and profile.get("version_source"): + report.declared_version = str(profile.get("core_version") or "") or None + report.page_version = page_version(profile, args.cache_dir, args.offline) + report.no_genuine_copy = str( + profile["version_source"].get("no_genuine_copy") or "") or None + if args.check_version: + _attach_version(args, profile, report) + _attach_detection(args, profile, report) + return report + + def _emit( args: argparse.Namespace, selected: dict, diff --git a/scripts/upstream.py b/scripts/upstream.py index 56391ce8..487c9e3c 100644 --- a/scripts/upstream.py +++ b/scripts/upstream.py @@ -30,6 +30,10 @@ ABSENT = "\0absent\0" GITHUB_COMPARE_CAP = 300 RETRIES = 3 RETRY_BACKOFF = (0.5, 2.0) +# A 429 is waited out, as base_scraper does: git.libretro.com answers it +# a few hundred requests into a pass and serves again a minute later. +RATE_LIMIT_WAITS = (5.0, 20.0, 60.0) +RATE_LIMIT_WAIT_CAP = 120.0 GITHUB_HOSTS = frozenset( {"github.com", "api.github.com", "raw.githubusercontent.com"} ) @@ -85,6 +89,9 @@ _HOSTS: dict[str, tuple[str, str, str]] = { _sleep = time.sleep +# Hosts still refusing after every wait. Asking them again in the same pass +# only spends the waits again, while the other forges keep answering. +_exhausted_hosts: set[str] = set() class UpstreamError(Exception): @@ -94,6 +101,10 @@ class UpstreamError(Exception): class RateLimitError(UpstreamError): """The forge refused the request for quota reasons.""" + def __init__(self, message: str, host: str = "") -> None: + super().__init__(message) + self.host = host + class NotCachedError(UpstreamError): """An offline read of something the cache never stored. @@ -232,15 +243,16 @@ def _http_failure(url: str, exc: urllib.error.HTTPError) -> UpstreamError: GitHub reports exhaustion with X-RateLimit-Remaining: 0, and 429 is the standard quota status everywhere, so those two are the only fatal cases. """ + host = urllib.parse.urlsplit(url).hostname or "" if exc.code == 429: - return RateLimitError(f"{url}: HTTP 429") + return RateLimitError(f"{url}: HTTP 429", host) if exc.code in (410, 451): reason = "withdrawn for legal reasons" if exc.code == 451 else "gone" return GoneError(f"{url}: HTTP {exc.code}, {reason}") if exc.code == 403 and exc.headers is not None: remaining = exc.headers.get("X-RateLimit-Remaining") if remaining is not None and remaining.strip() == "0": - return RateLimitError(f"{url}: HTTP 403, quota exhausted") + return RateLimitError(f"{url}: HTTP 403, quota exhausted", host) return UpstreamError(f"{url}: HTTP {exc.code}") @@ -278,15 +290,37 @@ def _resolver_answers() -> bool: return True +def _rate_limit_wait(exc: urllib.error.HTTPError, waited: int) -> float | None: + """Seconds to wait before asking again, or None when waiting is futile. + + GitHub's exhausted quota (403) resets on the hour. A 429 names its wait + in Retry-After, which is followed up to a cap, else the fixed steps. + """ + if exc.code != 429 or waited >= len(RATE_LIMIT_WAITS): + return None + retry_after = (exc.headers.get("Retry-After") or "").strip() if exc.headers else "" + if retry_after.isdigit(): + delay = float(retry_after) + return delay if delay <= RATE_LIMIT_WAIT_CAP else None + return RATE_LIMIT_WAITS[waited] + + def _fetch(url: str, accept_json: bool = False) -> bytes | None: """Body of a GET, or None on 404. A pass over every profile issues thousands of requests, so a dropped connection or a transient 5xx is a certainty rather than an accident. - Those are retried, a definitive answer from the forge is not. + Those are retried, a definitive answer from the forge is not. A rate + limit is waited out, and a host that outlasts the waits is not asked + again in this process. """ + host = urllib.parse.urlsplit(url).hostname or "" + if host in _exhausted_hosts: + raise RateLimitError(f"{url}: {host} rate limited earlier in this run", host) failure: UpstreamError | None = None - for attempt in range(RETRIES): + attempt = 0 + waited = 0 + while True: req = urllib.request.Request(url, headers=_headers(url, accept_json)) try: with urllib.request.urlopen(req, timeout=30) as resp: @@ -295,15 +329,24 @@ def _fetch(url: str, accept_json: bool = False) -> bytes | None: if exc.code == 404: return None failure = _http_failure(url, exc) - if isinstance(failure, RateLimitError) or exc.code < 500: + if isinstance(failure, RateLimitError): + delay = _rate_limit_wait(exc, waited) + if delay is None: + _exhausted_hosts.add(host) + raise failure from exc + waited += 1 + _sleep(delay) + continue + if exc.code < 500: raise failure from exc except (urllib.error.URLError, http.client.HTTPException, OSError) as exc: if _host_is_unresolvable(exc): raise GoneError(f"{url}: host does not resolve") from exc failure = UpstreamError(f"{url}: {exc}") - if attempt + 1 < RETRIES: - _sleep(RETRY_BACKOFF[attempt]) - raise failure + attempt += 1 + if attempt >= RETRIES: + raise failure + _sleep(RETRY_BACKOFF[attempt - 1]) def _http_text(url: str) -> str | None: diff --git a/tests/test_profile_sync.py b/tests/test_profile_sync.py index 901d74ee..d5644cd0 100644 --- a/tests/test_profile_sync.py +++ b/tests/test_profile_sync.py @@ -4163,6 +4163,40 @@ class TestRealignProse(unittest.TestCase): self.assertIn("(deep.c:14)", self.path.read_text()) +class TestRateLimitEndsOneProfile(unittest.TestCase): + """git.libretro.com answered 429 on cemu's tree and the pass over 647 + profiles stopped with a traceback and no report at all.""" + + def test_the_pass_goes_on_and_fails(self): + from unittest import mock + + def build(name, *_args, **_kwargs): + if name == "cemu": + raise profile_sync.upstream.RateLimitError( + "https://git.libretro.com/x: HTTP 429", "git.libretro.com" + ) + return ProfileReport(name=name, counts={"CHANGED": 1}) + + out, err = io.StringIO(), io.StringIO() + saved = sys.argv + try: + sys.argv = ["profile_sync.py", "--all", "--json"] + with mock.patch.object(profile_sync, "load_emulator_profiles", return_value={}), \ + mock.patch.object(profile_sync, "select_profiles", + return_value={"cemu": {}, "after": {}}), \ + mock.patch.object(profile_sync, "_check_quota"), \ + mock.patch.object(profile_sync, "build_report", side_effect=build), \ + contextlib.redirect_stdout(out), contextlib.redirect_stderr(err): + with self.assertRaises(SystemExit) as caught: + profile_sync.main() + finally: + sys.argv = saved + self.assertEqual(caught.exception.code, 1) + by_name = {r["name"]: r for r in json.loads(out.getvalue())} + self.assertEqual(by_name["cemu"]["skipped"], "rate limited: git.libretro.com") + self.assertEqual(by_name["after"]["needs_review"], 1) + + class TestOutputModesApplyTheirFlags(unittest.TestCase): """--changed-only narrows --json too. Extras are refused off the text report.""" diff --git a/tests/test_upstream.py b/tests/test_upstream.py index f4c5f536..faebdf4c 100644 --- a/tests/test_upstream.py +++ b/tests/test_upstream.py @@ -237,9 +237,11 @@ class TestFetchRetry(unittest.TestCase): self._orig = (urllib.request.urlopen, upstream._sleep) self.slept: list[float] = [] upstream._sleep = self.slept.append + upstream._exhausted_hosts.clear() def tearDown(self): urllib.request.urlopen, upstream._sleep = self._orig + upstream._exhausted_hosts.clear() def _serve(self, outcomes): self.calls = 0 @@ -283,11 +285,50 @@ class TestFetchRetry(unittest.TestCase): self.assertIsNone(upstream._fetch("https://host/x")) self.assertEqual(self.calls, 1) - def test_rate_limit_is_not_retried(self): - self._serve([_http_error(429)]) + @staticmethod + def _limited(retry_after: str | None = None) -> urllib.error.HTTPError: + import email.message # noqa: PLC0415 + + headers = email.message.Message() + if retry_after is not None: + headers["Retry-After"] = retry_after + return _http_error(429, headers) + + def test_rate_limit_is_waited_out(self): + # git.libretro.com answered 429 on cemu's tree a few hundred + # requests into a pass, and the whole pass stopped. + self._serve([self._limited("7"), self._limited(), b"payload"]) + self.assertEqual(upstream._fetch("https://host/x"), b"payload") + self.assertEqual(self.slept, [7.0, upstream.RATE_LIMIT_WAITS[1]]) + + def test_lasting_rate_limit_mutes_the_host_only(self): + self._serve([self._limited()]) + with self.assertRaises(upstream.RateLimitError) as caught: + upstream._fetch("https://host/x") + self.assertEqual(caught.exception.host, "host") + self.assertEqual(self.calls, len(upstream.RATE_LIMIT_WAITS) + 1) + # The same host is not asked again, another one still is. + with self.assertRaises(upstream.RateLimitError): + upstream._fetch("https://host/y") + self.assertEqual(self.calls, len(upstream.RATE_LIMIT_WAITS) + 1) + self._serve([b"other"]) + self.assertEqual(upstream._fetch("https://other/x"), b"other") + + def test_long_retry_after_is_not_waited(self): + self._serve([self._limited(str(int(upstream.RATE_LIMIT_WAIT_CAP) + 1))]) with self.assertRaises(upstream.RateLimitError): upstream._fetch("https://host/x") - self.assertEqual(self.calls, 1) + self.assertEqual((self.calls, self.slept), (1, [])) + + def test_exhausted_github_quota_is_not_waited(self): + import email.message # noqa: PLC0415 + + headers = email.message.Message() + headers["X-RateLimit-Remaining"] = "0" + self._serve([_http_error(403, headers)]) + with self.assertRaises(upstream.RateLimitError): + upstream._fetch("https://host/x") + self.assertEqual((self.calls, self.slept), (1, [])) class TestGoneUpstream(unittest.TestCase):