Bound the fingerprint fan-out's request rate (#8508)

Stack from [ghstack](https://github.com/ezyang/ghstack/tree/0.14.0)
(oldest at bottom):
* #8568
* __->__ #8508
* #8507
* #8506
* #8505

**Impact:** greenlight scan (fingerprint fan-out)
**Risk:** low

## What
Halve the fingerprint pool to 4 workers and pace each worker client at
0.5s
between requests (PyGithub's built-in pacer), via a new build_client
keyword.
Only the fan-out worker clients are slowed; the
listing/dispatch/verdict/authz
clients keep the 0.25s default.

## Why
The 8-worker pool issued ~32 concurrent requests/second, which trips
GitHub's
secondary (burst-rate) limit even though it is well under the
100-concurrent
ceiling -- GitHub's guidance is to issue requests serially, not in a
wide burst.
Four workers at 0.5s cap the fan-out at ~8 req/s (~4x lower), under the
secondary
limit with wide margin, while the listing client (which paginates a
large
open-PR set) is left at full speed since it is sequential, not the
burst.

# Notes
Aggregate fan-out rate is workers / seconds-between-requests: more
workers or a
shorter interval raise it; cutting workers 8 -> 4 and lengthening the
interval
0.25s -> 0.5s both lower it, to ~8 req/s (a test pins the invariant at
<= 8
req/s). build_client pins PyGithub's 0.25s default explicitly so a
library
default change cannot silently alter the request rate. Pure
concurrency/pacing
change: fingerprint results, decisions, and dispatch are unaffected, and
a
rate-limited or slow fan-out remains fail-closed.

Signed-off-by: Jean Schmidt <contato@jschmidt.me>

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

@@ -35,6 +35,7 @@
 
 if TYPE_CHECKING:
     from collections.abc import Callable, Sequence
+    from typing import Protocol
 
     from github import Github
 
@@ -45,6 +46,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] = {
@@ -68,7 +74,12 @@ def _is_trusted(login: str | None) -> bool:
     return login is not None and login.lower() in _TRUSTED_LOWER
 
 
-_FINGERPRINT_WORKERS = 8
+# Aggregate fan-out request rate is workers / seconds-between-requests: more workers or a
+# shorter interval raise it. Cutting workers (8->4) and lengthening the interval (0.25s->0.5s)
+# both lower it, to ~8 req/s, keeping the fan-out under GitHub's secondary (burst-rate) limit
+# that the prior 8-worker / 0.25s pool tripped.
+_FINGERPRINT_WORKERS = 4
+_FINGERPRINT_SECONDS_BETWEEN_REQUESTS = 0.5
 
 
 def _utcnow() -> datetime:
@@ -186,7 +197,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,
@@ -260,7 +271,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,19 @@ 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():
+    # The asserted quantity is the fan-out's aggregate request rate (workers / seconds-between-requests).
+    # 8 req/s is our own conservative budget, NOT a GitHub-published limit -- it is the ceiling we chose
+    # to stay comfortably under GitHub's (undocumented, variable) secondary/burst rate limit. It bounds
+    # only this fingerprint fan-out; the listing, dispatch, verdict, and authz clients each pace
+    # themselves independently and sit outside this budget.
+    assert review._FINGERPRINT_WORKERS / review._FINGERPRINT_SECONDS_BETWEEN_REQUESTS <= 8
 
 
 def test_close_client_swallows_close_errors(caplog):
@@ -1302,7 +1316,7 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     with caplog.at_level(logging.WARNING, 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=fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1344,7 +1358,7 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     with caplog.at_level(logging.WARNING, 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=fingerprint,
             read_state=lambda _repo, _numbers: {},
@@ -1433,7 +1447,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: {},
@@ -1491,7 +1505,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: {},
@@ -1564,7 +1578,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: {},