From 669bc5ecad1a3bdb3c259ec4b4caeb48522cd81f Mon Sep 17 00:00:00 2001 From: Mayur021 Date: Fri, 2 Oct 2026 11:29:02 +0530 Subject: [PATCH] fix(contributor-check): retry transient GitHub API failures within a bound `_api` already looped three times, but only a 403 continued the loop. A 5xx raised on the first attempt, and `URLError` was not caught at all, so the `timeout=15` socket timeout, a DNS failure, a reset or a TLS abort propagated straight out of the loop. `URLError` was not even imported. `_count_maintainer_merged` alone walks up to ten PR-detail calls for one candidate, paced half a second apart, and the run makes more calls besides. One blip anywhere in that walk ended the whole check. Since #46 that no longer mislabels anyone, which was the `needs-review:UNKNOWN` symptom in #27; it fails the action instead. But a transient blip still ends a check a retry would have passed. The existing loop now covers the transient cases, with full-jitter exponential backoff capped at 60s, and the bound stays at three attempts: - 404 returns None on the first attempt and is never retried. It is an answer, not a failure. Moved above the 403 branch for readability; the codes are distinct, so behaviour is unchanged. - 403 with Retry-After is honoured, clamped to 5-60s, as before. If it outlasts the bound it raises, and now says it was a rate limit. - 5xx and URLError back off, then raise as themselves. - Every other status raises on the first attempt, unchanged. Each exhausted path prints a line naming its own cause before re-raising. `_run_check` in contributor_check_action.py takes the last stderr line as the CheckExecutionError detail, and upstream's 403 path raised silently, so an exhausted rate limit used to reach a maintainer as a bare `exited 1` with nothing to say whether to wait or re-run. This is a second declared local modification to the vendored copy, so contributor_check.py moves out of the checksum table and the three remaining vendored files stay hash-pinned in CI. vendor/agt/README.md records both modifications and drops the upstream sync framing: AGT is not being invested in and this copy is owned here. 24 tests pass. The eight added cover both outcomes of each transient path, the 404 short-circuit, the attempt bound on every exhausted path, and the backoff cap. Each was checked against a mutated source to confirm it fails when the behaviour it names is removed. Refs: #27 Signed-off-by: Mayur021 --- .../contributor-check/tests/test_api_retry.py | 153 ++++++++++++++++++ .../contributor-check/vendor/agt/README.md | 50 ++++-- .../vendor/agt/contributor_check.py | 79 ++++++++- .github/workflows/contributor-check-tests.yml | 3 +- 4 files changed, 266 insertions(+), 19 deletions(-) create mode 100644 .github/actions/contributor-check/tests/test_api_retry.py diff --git a/.github/actions/contributor-check/tests/test_api_retry.py b/.github/actions/contributor-check/tests/test_api_retry.py new file mode 100644 index 0000000..c3d2aa1 --- /dev/null +++ b/.github/actions/contributor-check/tests/test_api_retry.py @@ -0,0 +1,153 @@ +"""Regression tests for the bounded retry in vendor/agt/contributor_check.py `_api`. + +Covers agentrust-io/.github#27 item 1. The rule under test: retry transient +failures within a bound, and report everything else as itself. A 404 is an +answer. A rate limit that outlasts the bound is still a rate limit. +""" +from __future__ import annotations + +import importlib.util +import io +import sys +from pathlib import Path +from urllib.error import HTTPError, URLError + +import pytest + +VENDOR = Path(__file__).resolve().parents[1] / "vendor" / "agt" + + +def _load(): + spec = importlib.util.spec_from_file_location( + "agt_contributor_check", VENDOR / "contributor_check.py" + ) + mod = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = mod + spec.loader.exec_module(mod) + return mod + + +@pytest.fixture() +def cc(monkeypatch): + mod = _load() + monkeypatch.setattr(mod, "_get_token", lambda: "t") + monkeypatch.setattr(mod.time, "sleep", lambda _s: None) + return mod + + +class _Resp: + def __init__(self, payload: bytes): + self._payload = payload + + def read(self): + return self._payload + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + +def _http(code: int, headers: dict | None = None) -> HTTPError: + return HTTPError("https://api.github.com/x", code, "err", headers or {}, None) + + +def _seq(monkeypatch, cc, outcomes): + """urlopen that yields each outcome in turn; raises if an exception.""" + calls = {"n": 0} + + def fake(_req, timeout=None): + i = calls["n"] + calls["n"] += 1 + out = outcomes[min(i, len(outcomes) - 1)] + if isinstance(out, Exception): + raise out + return _Resp(out) + + monkeypatch.setattr(cc, "urlopen", fake) + return calls + + +def test_5xx_retries_then_succeeds(cc, monkeypatch): + calls = _seq(monkeypatch, cc, [_http(502), _http(502), b'{"ok":1}']) + assert cc._api("/x") == {"ok": 1} + assert calls["n"] == 3 + + +def test_urlerror_retries_then_succeeds(cc, monkeypatch): + calls = _seq(monkeypatch, cc, [URLError("timed out"), b'{"ok":1}']) + assert cc._api("/x") == {"ok": 1} + assert calls["n"] == 2 + + +def test_404_returns_none_without_retrying(cc, monkeypatch): + calls = _seq(monkeypatch, cc, [_http(404)]) + assert cc._api("/x") is None + assert calls["n"] == 1, "a 404 is an answer and must not be retried" + + +def test_rate_limit_exhausted_reports_as_a_rate_limit(cc, monkeypatch, capsys): + calls = _seq(monkeypatch, cc, [_http(403, {"Retry-After": "7"})]) + with pytest.raises(HTTPError): + cc._api("/x") + assert calls["n"] == cc._RETRY_MAX_ATTEMPTS + last = capsys.readouterr().err.strip().splitlines()[-1] + # Must be the final cause line, not one of the "waiting Ns..." lines that + # also contain "Rate limited". contributor_check_action.py reports the last + # stderr line, so that line is what a maintainer sees. + assert "still limited after" in last, ( + f"last stderr line must name the exhausted rate limit, got {last!r}" + ) + + +def test_server_error_exhausted_reports_as_a_server_error(cc, monkeypatch, capsys): + calls = _seq(monkeypatch, cc, [_http(503)]) + with pytest.raises(HTTPError): + cc._api("/x") + assert calls["n"] == cc._RETRY_MAX_ATTEMPTS + last = capsys.readouterr().err.strip().splitlines()[-1] + assert "503" in last and "attempts" in last + + +def test_network_error_exhausted_reports_as_a_network_error(cc, monkeypatch, capsys): + calls = _seq(monkeypatch, cc, [URLError("dns failure")]) + with pytest.raises(URLError): + cc._api("/x") + assert calls["n"] == cc._RETRY_MAX_ATTEMPTS + last = capsys.readouterr().err.strip().splitlines()[-1] + # Not just "Network error": the per-attempt "retrying in Xs..." lines say + # that too, so a bare substring check passes even with the final line gone. + assert "on all" in last and str(cc._RETRY_MAX_ATTEMPTS) in last, ( + f"last stderr line must name the exhausted network error, got {last!r}" + ) + + +def test_other_http_status_raises_on_first_attempt(cc, monkeypatch): + calls = _seq(monkeypatch, cc, [_http(422)]) + with pytest.raises(HTTPError): + cc._api("/x") + assert calls["n"] == 1, "a 422 is the API answering and must not be retried" + + +def test_backoff_is_bounded_and_jittered(cc): + for attempt in range(cc._RETRY_MAX_ATTEMPTS): + for _ in range(50): + s = cc._retry_sleep_seconds(attempt) + assert 0 <= s <= cc._RETRY_MAX_SLEEP_SECONDS + + +def test_backoff_cap_binds_above_the_curve(cc): + # Across the attempts the loop actually uses, the raw curve stays under the + # cap, so those attempts cannot show whether the cap is applied at all. + # Exercise it at an attempt where it has to bite. + attempt = 30 + assert cc._RETRY_BASE_SECONDS * (2 ** attempt) > cc._RETRY_MAX_SLEEP_SECONDS + for _ in range(200): + assert cc._retry_sleep_seconds(attempt) <= cc._RETRY_MAX_SLEEP_SECONDS + + +def test_backoff_is_not_a_constant(cc): + # Full jitter, so a fleet of runners hitting the same limit does not retry + # in lockstep. A fixed attempt must not return one value. + assert len({cc._retry_sleep_seconds(2) for _ in range(50)}) > 1 diff --git a/.github/actions/contributor-check/vendor/agt/README.md b/.github/actions/contributor-check/vendor/agt/README.md index 06363e1..a044d14 100644 --- a/.github/actions/contributor-check/vendor/agt/README.md +++ b/.github/actions/contributor-check/vendor/agt/README.md @@ -16,16 +16,19 @@ level. See agentrust-io/.github#27. | File | Upstream path | SHA-256 | |---|---|---| -| `contributor_check.py` | `scripts/contributor_check.py` | `f1c1179c921f59f836e85355b2efdf8641f65b763516f05853f4d216e70ae164` | +| `contributor_check.py` | `scripts/contributor_check.py` | modified, see below | | `contributor_check_allowlist.json` | `scripts/contributor_check_allowlist.json` | `d8a5ddbb0695c21b3eae88e6bc85edcda6cbd9721cad3ca9d10fbeeed4fc0636` | | `credential_audit.py` | `scripts/credential_audit.py` | `9650675038e7720a6d5cdd8be058fa38b2e004cc1e6f7bc81c48ed100e4680e1` | | `cluster_detect.py` | `scripts/cluster_detect.py` | `da36c10074aa638bf61c5873fbe6cff975533563553689eba5e90329e3d8d7cf` | | `contributor_check_action.py` | `scripts/contributor_check_action.py` | modified, see below | -The first four are byte-identical to upstream at that commit. The checksums -above are the upstream bytes, so `sha256sum` here is a one-line verification. +The first three unmodified files are byte-identical to upstream at that commit. +The checksums above are the upstream bytes, so `sha256sum` here is a one-line +verification for those three. -## The one local modification +## The two local modifications + +### `contributor_check_action.py` `contributor_check_action.py` differs from upstream in `_run_check` and in the block of `main()` that calls it. `_run_check` now reads the subprocess return @@ -36,6 +39,31 @@ contributor-risk level `UNKNOWN`. `UNKNOWN` still means what its upstream comment says it means: a check that ran and could not determine an answer. That case is unchanged and still labels. +### `contributor_check.py` + +`_api` now carries a bounded retry. Upstream loops three times but only a 403 +retries; a 5xx raises on the first attempt and `URLError` is not imported at +all, so the `timeout=15` socket timeout propagates straight through the loop. +That is the failure behind agentrust-io/.github#27. Since the +`contributor_check_action.py` change above, it no longer mislabels a +contributor; it fails the action instead. A single transient blip still ends a +check that a retry would have passed, which is what this change addresses. + +The retry is limited to transient failures and reports anything else as itself: + +| Condition | Behaviour | +|---|---| +| 404 | Returns `None`. An answer, never retried. | +| 403 rate limit | Honours `Retry-After`, clamped to 5-60s, within the attempt bound. | +| 5xx | Full-jitter exponential backoff within the attempt bound. | +| `URLError` | Full-jitter exponential backoff within the attempt bound. | +| Any other HTTP status | Raised on the first attempt. | + +When a retryable condition outlasts the bound, the final line written to stderr +names the cause before the exception propagates. `contributor_check_action.py` +reports the last stderr line, so a rate limit surfaces as a rate limit and a +server error as a server error rather than as an unattributed execution fault. + ## Why more than two files `contributor_check_action.py` resolves the scripts it runs as siblings of @@ -51,8 +79,12 @@ than a silent `UNKNOWN`. All five move together or none of them do. ## Updating -Re-copy from the same upstream paths at a newer commit, update the commit SHA -and the checksums above, and re-apply the `_run_check` modification. Upstream -has a known divergence between `scripts/` and the packaged -`agent_compliance/cli/` copies (AGT#3571), so check which copy carries the -retry behaviour before advancing. +These files are no longer tracked against upstream. Per the decision recorded +on agentrust-io/.github#27, AGT is not being invested in and this copy is owned +here. The commit above is provenance, not a sync point, and no resync is +planned. + +Change these files directly, as you would any other file in this repository. +If a change lands in one of the three that remain byte-identical, move it out of +the checksum table above and out of the `vendor-integrity` job, and describe the +change here, the way the two modified files are described. diff --git a/.github/actions/contributor-check/vendor/agt/contributor_check.py b/.github/actions/contributor-check/vendor/agt/contributor_check.py index 1008b23..9c8ace2 100644 --- a/.github/actions/contributor-check/vendor/agt/contributor_check.py +++ b/.github/actions/contributor-check/vendor/agt/contributor_check.py @@ -21,6 +21,7 @@ import json import math import os +import random import subprocess import sys import time @@ -28,7 +29,7 @@ from datetime import datetime, timezone from pathlib import Path from typing import Any -from urllib.error import HTTPError +from urllib.error import HTTPError, URLError from urllib.parse import quote from urllib.request import Request, urlopen @@ -67,6 +68,22 @@ def _get_token() -> str: return token +# Bounded retry, transient failures only. A 404 is an answer and is never +# retried. A rate limit and a server error are retried within the bound and, +# if they outlast it, are reported as themselves: the final stderr line names +# the cause, which is what contributor_check_action.py surfaces when a check +# cannot be executed. See agentrust-io/.github#27. +_RETRY_MAX_ATTEMPTS = 3 +_RETRY_BASE_SECONDS = 1.0 +_RETRY_MAX_SLEEP_SECONDS = 60.0 + + +def _retry_sleep_seconds(attempt: int) -> float: + """Full-jitter exponential backoff: random in [0, base * 2**attempt).""" + upper = min(_RETRY_BASE_SECONDS * (2 ** attempt), _RETRY_MAX_SLEEP_SECONDS) + return random.uniform(0, upper) + + def _api(path: str, params: dict[str, str] | None = None) -> Any: """Call the GitHub REST API and return parsed JSON.""" url = f"https://api.github.com{path}" @@ -79,19 +96,65 @@ def _api(path: str, params: dict[str, str] | None = None) -> Any: req.add_header("Accept", "application/vnd.github+json") req.add_header("X-GitHub-Api-Version", "2022-11-28") - for attempt in range(3): + last = _RETRY_MAX_ATTEMPTS - 1 + for attempt in range(_RETRY_MAX_ATTEMPTS): try: with urlopen(req, timeout=15) as resp: return json.loads(resp.read()) except HTTPError as exc: - if exc.code == 403 and attempt < 2: - wait = int(exc.headers.get("Retry-After", "10")) - wait = min(max(wait, 5), 60) - print(f" Rate limited, waiting {wait}s...", file=sys.stderr) - import time; time.sleep(wait) - continue + # 404 is an answer, not a failure. Never retried. if exc.code == 404: return None + # GitHub's documented rate-limit envelope: 403 with Retry-After. + # Honour it, clamped to a sane band. + if exc.code == 403: + if attempt < last: + wait = int(exc.headers.get("Retry-After", "10")) + wait = min(max(wait, 5), 60) + print(f" Rate limited, waiting {wait}s...", file=sys.stderr) + time.sleep(wait) + continue + print( + f" Rate limited by the GitHub API and still limited after " + f"{_RETRY_MAX_ATTEMPTS} attempts", + file=sys.stderr, + ) + raise + # 5xx is transient on GitHub's side. + if 500 <= exc.code < 600: + if attempt < last: + wait = _retry_sleep_seconds(attempt) + print( + f" Server error {exc.code}, retrying in {wait:.1f}s...", + file=sys.stderr, + ) + time.sleep(wait) + continue + print( + f" GitHub API returned {exc.code} on all " + f"{_RETRY_MAX_ATTEMPTS} attempts", + file=sys.stderr, + ) + raise + # Anything else is the API answering. Report it as itself. + raise + except URLError as exc: + # Network-layer failures: DNS, TCP reset, TLS abort, socket + # timeout. The timeout=15 above raises here, which is the case + # that produced the UNKNOWN labels in #27. + if attempt < last: + wait = _retry_sleep_seconds(attempt) + print( + f" Network error ({exc.reason}), retrying in {wait:.1f}s...", + file=sys.stderr, + ) + time.sleep(wait) + continue + print( + f" Network error ({exc.reason}) on all " + f"{_RETRY_MAX_ATTEMPTS} attempts", + file=sys.stderr, + ) raise diff --git a/.github/workflows/contributor-check-tests.yml b/.github/workflows/contributor-check-tests.yml index 9daf714..34019f1 100644 --- a/.github/workflows/contributor-check-tests.yml +++ b/.github/workflows/contributor-check-tests.yml @@ -35,11 +35,10 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - name: Vendored files match the checksums recorded in vendor/agt/README.md + - name: Unmodified vendored files match the checksums in vendor/agt/README.md run: | cd .github/actions/contributor-check/vendor/agt sha256sum -c --strict - <<'SUMS' - f1c1179c921f59f836e85355b2efdf8641f65b763516f05853f4d216e70ae164 contributor_check.py 9650675038e7720a6d5cdd8be058fa38b2e004cc1e6f7bc81c48ed100e4680e1 credential_audit.py da36c10074aa638bf61c5873fbe6cff975533563553689eba5e90329e3d8d7cf cluster_detect.py d8a5ddbb0695c21b3eae88e6bc85edcda6cbd9721cad3ca9d10fbeeed4fc0636 contributor_check_allowlist.json