Update

[ghstack-poisoned]

Author: jeanschmidt

greenlight/src/greenlight/github_client.py

@@ -72,15 +72,19 @@ def _build_retry() -> Retry:
     )
 
 
-def build_client(token: str) -> Github:
+def build_client(token: str, *, seconds_between_requests: float = 0.25) -> Github:
     from github import Auth, Github  # lazy: keeps this module importable without the dep
 
+    # Pin PyGithub's 0.25s default pacing explicitly (cf. _GITHUB_TIMEOUT_SECONDS) so a library
+    # default change can't silently alter our request rate; the fingerprint fan-out passes a
+    # slower value to bound its aggregate rate.
     return Github(
         auth=Auth.Token(token),
         per_page=100,
         timeout=_GITHUB_TIMEOUT_SECONDS,
         retry=_build_retry(),
         lazy=True,
+        seconds_between_requests=seconds_between_requests,
     )
 
 

greenlight/src/greenlight/review.py

@@ -34,6 +34,7 @@
 
 if TYPE_CHECKING:
     from collections.abc import Callable, Sequence
+    from typing import Protocol
 
     from github import Github
 
@@ -44,6 +45,11 @@
     from greenlight.scan_runner import FingerprintFn
     from greenlight.state import PRState
 
+    class _BuildClient(Protocol):
+        # token positional-only so injected test doubles needn't match the parameter name.
+        def __call__(self, token: str, /, *, seconds_between_requests: float = 0.25) -> Github: ...
+
+
 logger = logging.getLogger(__name__)
 
 TRUSTED_AUTHORS: set[str] = {
@@ -65,7 +71,12 @@ def _is_trusted(login: str | None) -> bool:
     return login is not None and login.lower() in _TRUSTED_LOWER
 
 
-_FINGERPRINT_WORKERS = 8
+# The fan-out's aggregate GitHub request rate is workers / seconds-between-requests; both are
+# tuned down together to stay under GitHub's secondary (burst-rate) limit, which the prior
+# 8-worker / default-paced pool tripped. Raising either without lowering the other raises the
+# burst rate.
+_FINGERPRINT_WORKERS = 4
+_FINGERPRINT_SECONDS_BETWEEN_REQUESTS = 0.5
 
 
 def _utcnow() -> datetime:
@@ -183,7 +194,7 @@ def run(
     requester: str | None = None,
     allow_untrusted_author: bool = False,
     bot_login: str = "",
-    build_github: Callable[[str], Github] = github_client.build_client,
+    build_github: _BuildClient = github_client.build_client,
     fetch: Callable[[Github], list[OpenPR]] = _default_fetch,
     fetch_author: Callable[[Github, int], str | None] = _default_fetch_author,
     fingerprint: FingerprintFn = _default_fingerprint,
@@ -251,7 +262,7 @@ def run(
         # and guarantees no two running tasks ever share one.
         client_pool: queue.Queue[Github] = queue.Queue()
         for _ in range(worker_count):
-            worker_client = build_github(token)
+            worker_client = build_github(token, seconds_between_requests=_FINGERPRINT_SECONDS_BETWEEN_REQUESTS)
             clients.callback(_close_client, worker_client)
             client_pool.put(worker_client)
         if max_dispatches is None:

greenlight/tests/test_github_client.py

@@ -407,6 +407,36 @@ def _fake_github(**kwargs: object) -> object:
     assert retry.respect_retry_after_header is False
 
 
+def test_build_client_passes_seconds_between_requests_when_given(monkeypatch):
+    captured: dict[str, object] = {}
+
+    def _fake_github(**kwargs: object) -> object:
+        captured.update(kwargs)
+        return object()
+
+    monkeypatch.setattr("github.Github", _fake_github)
+
+    github_client.build_client("tok", seconds_between_requests=0.5)
+
+    assert captured["seconds_between_requests"] == 0.5
+
+
+def test_build_client_defaults_pacing_to_pygithub_default(monkeypatch):
+    captured: dict[str, object] = {}
+
+    def _fake_github(**kwargs: object) -> object:
+        captured.update(kwargs)
+        return object()
+
+    monkeypatch.setattr("github.Github", _fake_github)
+
+    github_client.build_client("tok")
+
+    # Pins PyGithub's 0.25s default explicitly, so a library default change cannot silently alter
+    # the request rate.
+    assert captured["seconds_between_requests"] == 0.25
+
+
 def test_is_rate_limit_error_true_for_rate_limit_exceeded_exception():
     # A 403 rate limit arrives as RateLimitExceededException (status 403), matched by the isinstance
     # arm, not by the 429 status check.

greenlight/tests/test_review.py

@@ -171,7 +171,7 @@ def fake_emit(*, repo, pr_number, head_sha, eval_hash, run_id):
         requester=requester,
         allow_untrusted_author=allow_untrusted_author,
         bot_login=bot_login,
-        build_github=lambda _token: _CLIENT,
+        build_github=lambda _token, **_kwargs: _CLIENT,
         fetch=fake_fetch,
         fetch_author=fake_fetch_author,
         fingerprint=fake_fingerprint,
@@ -618,7 +618,7 @@ def fake_dispatch(_client, number, head_sha, eval_hash, dispatch_ref):
         review.run(
             make_config(github_token="t"),
             max_dispatches=1,
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(n) for n in numbers],
             fingerprint=boom_fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -856,7 +856,7 @@ def fake_dispatch(_client, number, head_sha, eval_hash, dispatch_ref):
             make_config(github_token="t"),
             pr=7,
             force=True,
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [],
             fetch_author=lambda _client, _number: "albanD",
             fingerprint=boom_fingerprint,
@@ -955,7 +955,7 @@ def fake_dispatch(_client, number, head_sha, eval_hash, dispatch_ref):
     ):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=boom_fingerprint,
             read_state=lambda _repo, _numbers: {2: _state(2, STATUS_LAND, _HASH_A, _NEW)},
@@ -989,7 +989,7 @@ def fake_dispatch(_client, number, head_sha, eval_hash, dispatch_ref):
     ):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(3), _open_pr(1), _open_pr(2)],
             fingerprint=boom_fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1020,7 +1020,7 @@ def boom_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     ):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=lambda _client, number, _authorized, _skip: (f"headsha{number}", _HASH_A),
             read_state=lambda _repo, _numbers: {},
@@ -1049,7 +1049,7 @@ def boom_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     ):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=lambda _client, number, _authorized, _skip: (f"headsha{number}", _HASH_A),
             read_state=lambda _repo, _numbers: {},
@@ -1074,7 +1074,7 @@ def timeout_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     with pytest.raises(IterationTimeout):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=lambda _client, number, _authorized, _skip: (f"headsha{number}", _HASH_A),
             read_state=lambda _repo, _numbers: {},
@@ -1105,7 +1105,7 @@ def boom_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     with caplog.at_level(logging.ERROR, logger="greenlight"), pytest.raises(RuntimeError) as excinfo:
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=boom_fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1153,7 +1153,7 @@ def fake_dispatch(_client, number, head_sha, eval_hash, dispatch_ref):
 
     review.run(
         make_config(github_token="t"),
-        build_github=lambda _token: _CLIENT,
+        build_github=lambda _token, **_kwargs: _CLIENT,
         fetch=lambda _client: [_open_pr(n) for n in numbers],
         fingerprint=barrier_fingerprint,
         read_state=lambda _repo, _numbers: {},
@@ -1176,7 +1176,7 @@ def test_worker_clients_are_isolated_and_exclude_main_client(make_config):
     barrier = threading.Barrier(k, timeout=5)
     built: list[object] = []
 
-    def factory(_token):
+    def factory(_token, **_kwargs):
         client = object()
         built.append(client)
         return cast("Github", client)
@@ -1218,16 +1218,17 @@ def fake_dispatch(_client, number, head_sha, eval_hash, dispatch_ref):
 
 def test_run_closes_main_and_worker_clients(make_config):
     class _Closeable:
-        def __init__(self) -> None:
+        def __init__(self, seconds_between_requests: float | None) -> None:
             self.closed = 0
+            self.seconds_between_requests = seconds_between_requests
 
         def close(self) -> None:
             self.closed += 1
 
     built: list[_Closeable] = []
 
-    def factory(_token):
-        client = _Closeable()
+    def factory(_token, *, seconds_between_requests=None):
+        client = _Closeable(seconds_between_requests)
         built.append(client)
         return cast("Github", client)
 
@@ -1246,6 +1247,16 @@ def factory(_token):
     # each is closed exactly once as the scan unwinds -- the connection pools are not leaked.
     assert len(built) == 3
     assert all(client.closed == 1 for client in built)
+    # The main client (built first) keeps PyGithub's default pacing -- no kwarg passed -- while the
+    # two fingerprint worker clients are throttled to bound the fan-out's aggregate request rate.
+    assert built[0].seconds_between_requests is None
+    assert [c.seconds_between_requests for c in built[1:]] == [review._FINGERPRINT_SECONDS_BETWEEN_REQUESTS] * 2
+
+
+def test_fingerprint_throttle_stays_under_burst_limit():
+    # Aggregate fan-out rate is workers / seconds-between-requests; keep it <= 8 req/s to stay
+    # under GitHub's secondary (burst-rate) limit.
+    assert review._FINGERPRINT_WORKERS / review._FINGERPRINT_SECONDS_BETWEEN_REQUESTS <= 8
 
 
 def test_close_client_swallows_close_errors(caplog):
@@ -1302,7 +1313,7 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     with pytest.raises(RuntimeError, match=r"1 PR\(s\) failed during scan: \[1\]"):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1337,7 +1348,7 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     with pytest.raises(RuntimeError, match=r"1 PR\(s\) failed during scan: \[2\]"):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1373,7 +1384,7 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
         review.run(
             make_config(github_token="t"),
             max_dispatches=5,
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1), _open_pr(2), _open_pr(3)],
             fingerprint=fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1398,7 +1409,7 @@ def boom_fetch(_client):
     with pytest.raises(RuntimeError, match="fetch boom"):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: cast("Github", client),
+            build_github=lambda _token, **_kwargs: cast("Github", client),
             fetch=boom_fetch,
             fingerprint=lambda _client, number, _authorized, _skip: (f"headsha{number}", _HASH_A),
             read_state=lambda _repo, _numbers: {},
@@ -1471,7 +1482,7 @@ def boom_resolve() -> frozenset[str]:
     with pytest.raises(RuntimeError, match="merge_rules unreachable"):
         review.run(
             make_config(github_token="t"),
-            build_github=lambda _token: _CLIENT,
+            build_github=lambda _token, **_kwargs: _CLIENT,
             fetch=lambda _client: [_open_pr(1)],
             fingerprint=lambda _client, number, _authorized, _skip: (f"headsha{number}", _HASH_A),
             read_state=lambda _repo, _numbers: {},