Update

[ghstack-poisoned]

Author: jeanschmidt

greenlight/src/greenlight/github_client.py

@@ -85,13 +85,24 @@ def build_client(token: str) -> Github:
 
 
 def is_rate_limit_error(exc: BaseException) -> bool:
-    # GitHub delivers a rate limit as 403 (-> RateLimitExceededException) or 429 (-> base
+    # GitHub delivers a rate limit as 403 (usually -> RateLimitExceededException) or 429 (-> base
     # GithubException); 429 must stay off _build_retry's forcelist or it surfaces as a RetryError.
     from github import GithubException, RateLimitExceededException
 
     if isinstance(exc, RateLimitExceededException):
         return True
-    return isinstance(exc, GithubException) and getattr(exc, "status", None) == 429
+    if not isinstance(exc, GithubException):
+        return False
+    status = getattr(exc, "status", None)
+    if status == 429:
+        return True
+    # PyGithub only maps a 403 to RateLimitExceededException when the body matches one of a few
+    # literal message strings, so a rate-limit 403 whose wording drifts arrives as a bare
+    # GithubException; key off the rate-limit headers GitHub sends with it instead of the message.
+    if status == 403:
+        headers = getattr(exc, "headers", None) or {}
+        return "retry-after" in headers or headers.get("x-ratelimit-remaining") == "0"
+    return False
 
 
 def list_open_prs_by_authors(client: _RepoClient, repo: str, authors: Iterable[str]) -> list[OpenPR]:

greenlight/src/greenlight/review.py

@@ -19,6 +19,7 @@
 import contextlib
 import logging
 import queue
+import threading
 from datetime import UTC, datetime, timedelta
 from typing import TYPE_CHECKING
 
@@ -248,6 +249,11 @@ def run(
         failed: list[int] = []
         abandoned: list[int] = []
         skips: list[tuple[int, ReviewSkip]] = []
+        # Owned here (not inside the helpers) so run can read it back after the fan-out: a rate limit
+        # trips it, which gates the dispatch phase below. It reflects "the cancel event fired," which
+        # is broader than a non-empty `abandoned` -- a rate limit on the last task leaves nothing to
+        # cancel (abandoned stays empty) yet must still skip dispatch.
+        cancel_event = threading.Event()
         worker_count = min(_FINGERPRINT_WORKERS, len(fingerprint_numbers))
         # PyGithub is not thread-safe, so each concurrent task borrows a client for its
         # exclusive use; sizing the pool to the worker count keeps queue.get non-blocking
@@ -272,6 +278,7 @@ def run(
                 abandoned=abandoned,
                 skips=skips,
                 force=force,
+                cancel_event=cancel_event,
             )
         else:
             pending = scan_runner._fingerprint_until_dispatchable(
@@ -289,17 +296,30 @@ def run(
                 abandoned=abandoned,
                 skips=skips,
                 force=force,
+                cancel_event=cancel_event,
             )
-        if abandoned:
+        dispatch_failed: list[int] = []
+        if cancel_event.is_set():
+            # A rate limit tripped the fan-out. The completed candidates are deferred, not lost: no
+            # state row is written for them, so the next scan re-fingerprints and dispatches them once
+            # the limit clears. Firing workflow_dispatch POSTs now on the same throttled token is what
+            # GitHub's secondary-rate-limit detection punishes most, so skip the dispatch phase.
             logger.warning(
-                "rate limit hit: abandoned %d of %d fingerprint(s) (not evaluated); %d candidate(s) dispatchable",
+                "rate limit hit: abandoned %d of %d fingerprint(s) (not evaluated); "
+                "skipping dispatch of %d completed candidate(s) this pass; will retry next scan",
                 len(abandoned),
                 len(fingerprint_numbers),
                 len(pending),
             )
-        dispatch_failed = scan_runner._dispatch_pending(
-            client, pending, ref=ref, max_dispatches=max_dispatches, dispatch=dispatch, emit_dispatched=emit_dispatched
-        )
+        else:
+            dispatch_failed = scan_runner._dispatch_pending(
+                client,
+                pending,
+                ref=ref,
+                max_dispatches=max_dispatches,
+                dispatch=dispatch,
+                emit_dispatched=emit_dispatched,
+            )
         # Only the --pr recheck path posts refusals; a listing-scan skip is dropped silently
         # (already logged). skips can hold a refusal only when skip_on_approval is False (--pr),
         # so this can never comment on a listing-scan approval.

greenlight/src/greenlight/scan_runner.py

@@ -9,7 +9,6 @@
 from __future__ import annotations
 
 import logging
-import threading
 from concurrent.futures import ThreadPoolExecutor
 from dataclasses import dataclass
 from datetime import datetime
@@ -24,6 +23,7 @@
 
 if TYPE_CHECKING:
     import queue
+    import threading
     from collections.abc import Callable, Sequence
     from concurrent.futures import Future
     from datetime import timedelta
@@ -160,8 +160,8 @@ def _fingerprint_all(
     abandoned: list[int],
     skips: list[tuple[int, ReviewSkip]],
     force: bool,
+    cancel_event: threading.Event,
 ) -> list[_Candidate]:
-    cancel_event = threading.Event()
     futures: dict[int, Future[tuple[str, str] | ReviewSkip | _Cancelled]] = {}
     if worker_count:
         with ThreadPoolExecutor(max_workers=worker_count) as pool:
@@ -211,10 +211,10 @@ def _fingerprint_until_dispatchable(
     abandoned: list[int],
     skips: list[tuple[int, ReviewSkip]],
     force: bool,
+    cancel_event: threading.Event,
 ) -> list[_Candidate]:
     ranked = sorted(pr_numbers, key=lambda number: _staleness_key_for_state(states.get(number)))
     pending: list[_Candidate] = []
-    cancel_event = threading.Event()
     if worker_count:
         with ThreadPoolExecutor(max_workers=worker_count) as pool:
             for start in range(0, len(ranked), worker_count):

greenlight/tests/test_github_client.py

@@ -420,11 +420,35 @@ def test_is_rate_limit_error_true_for_github_exception_status_429():
 
 
 def test_is_rate_limit_error_false_for_github_exception_status_403():
-    # A non-rate-limit 403 (bare GithubException, not RateLimitExceededException) is not a rate limit:
-    # only RateLimitExceededException or a 429 count.
+    # A bare 403 with no rate-limit headers is a plain error (e.g. a permission denial), not a limit.
     assert github_client.is_rate_limit_error(GithubException(403)) is False
 
 
+def test_is_rate_limit_error_true_for_403_with_retry_after_header():
+    # A secondary-limit 403 whose body does not match PyGithub's literal rate-limit strings arrives
+    # as a bare GithubException; the retry-after header is the robust signal that it is a rate limit.
+    exc = GithubException(403, headers={"retry-after": "60"})
+    assert github_client.is_rate_limit_error(exc) is True
+
+
+def test_is_rate_limit_error_true_for_403_with_exhausted_ratelimit_remaining():
+    # A primary-limit 403 that misclassifies is still recognizable by an exhausted budget.
+    exc = GithubException(403, headers={"x-ratelimit-remaining": "0"})
+    assert github_client.is_rate_limit_error(exc) is True
+
+
+def test_is_rate_limit_error_false_for_403_with_budget_remaining():
+    # A genuine permission 403 carries x-ratelimit-remaining with budget left and no retry-after, so
+    # it must not be misread as a rate limit.
+    exc = GithubException(403, headers={"x-ratelimit-remaining": "4999"})
+    assert github_client.is_rate_limit_error(exc) is False
+
+
+def test_is_rate_limit_error_false_for_github_exception_other_status():
+    # A GithubException carrying any other status (e.g. a 500) is not a rate limit.
+    assert github_client.is_rate_limit_error(GithubException(500)) is False
+
+
 def test_is_rate_limit_error_false_for_retry_error():
     # A urllib3-exhausted retry surfaces as requests RetryError, not a GithubException, so it is not
     # classified as a rate limit.

greenlight/tests/test_review.py

@@ -1325,7 +1325,7 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
     assert "abandoned 2 of 3" in caplog.text
 
 
-def test_rate_limit_still_dispatches_earlier_completed_candidate(make_config, monkeypatch, caplog):
+def test_rate_limit_defers_completed_candidate_without_dispatching(make_config, monkeypatch, caplog):
     from github import RateLimitExceededException
 
     monkeypatch.setattr(review, "_FINGERPRINT_WORKERS", 1)
@@ -1353,17 +1353,64 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
             now=lambda: _NOW,
         )
 
-    # PR1 completes as a candidate before PR2 hits the rate limit, so the partial success is preserved
-    # -- PR1 still dispatches. PR2 trips the cancel event, so PR3 short-circuits without being
-    # fingerprinted: PR2 lands in `failed`, PR3 is counted as abandoned (not failed).
+    # PR1 completes as a candidate before PR2 hits the rate limit, but a dispatch is a workflow_dispatch
+    # POST on the same throttled token -- exactly what secondary-rate-limit detection punishes -- so the
+    # whole dispatch phase is skipped once the cancel event trips. PR1 is deferred, not lost: no state
+    # row is written, so the next scan re-fingerprints and dispatches it once the limit clears. PR2 trips
+    # the event, so PR3 short-circuits without being fingerprinted.
     assert fingerprinted == [1, 2]
-    assert dispatched == [1]
-    # The failure and the abandonment are surfaced distinctly in the fail-closed signal, and the
-    # abandonment is logged as its own warning rather than folded into the failure count.
+    assert dispatched == []
+    # The deferral is surfaced by the merged rate-limit warning, and the scan still fails closed: PR2
+    # (failed) and PR3 (abandoned) are carried distinctly in the end-of-scan RuntimeError.
+    assert "skipping dispatch of 1 completed candidate(s) this pass" in caplog.text
+    assert "abandoned 1 of 3" in caplog.text
     message = str(excinfo.value)
     assert "1 PR(s) failed during scan: [2]" in message
     assert "1 PR(s) abandoned due to rate limit: [3]" in message
-    assert "abandoned 1 of 3" in caplog.text
+
+
+def test_rate_limit_on_last_task_skips_dispatch_with_no_abandoned(make_config, monkeypatch, caplog):
+    from github import RateLimitExceededException
+
+    # The gate keys off cancel_event, not the `abandoned` list, precisely for this case: the rate limit
+    # hits the LAST task to run, so no queued task is left to short-circuit to _CANCELLED and `abandoned`
+    # stays empty -- yet the event IS set, so dispatch must still be skipped. A regression to `if abandoned:`
+    # would pass every other test but wrongly dispatch the completed candidates onto the throttled token here.
+    monkeypatch.setattr(review, "_FINGERPRINT_WORKERS", 1)
+    fingerprinted: list[int] = []
+    dispatched: list[int] = []
+
+    def fingerprint(_client, number, _authorized, _skip):
+        fingerprinted.append(number)
+        if number == 3:
+            raise RateLimitExceededException(403)
+        return (f"headsha{number}", _HASH_A)
+
+    def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
+        dispatched.append(number)
+
+    with caplog.at_level(logging.WARNING, logger="greenlight"), pytest.raises(RuntimeError) as excinfo:
+        review.run(
+            make_config(github_token="t"),
+            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: {},
+            dispatch=fake_dispatch,
+            resolve_authorized=lambda: _AUTHORIZED,
+            now=lambda: _NOW,
+        )
+
+    # PR1 and PR2 complete as candidates; PR3 (the last task) trips the cancel event with nothing left to
+    # cancel, so abandoned is empty. Dispatch is still skipped -- the two completed candidates are deferred.
+    assert fingerprinted == [1, 2, 3]
+    assert dispatched == []
+    assert "skipping dispatch of 2 completed candidate(s) this pass" in caplog.text
+    # abandoned is empty, so the fail-closed RuntimeError carries only the failed clause (the last PR) and
+    # no "abandoned" clause -- the scan still signals incomplete via the rate-limited task landing in failed.
+    message = str(excinfo.value)
+    assert "1 PR(s) failed during scan: [3]" in message
+    assert "abandoned" not in message
 
 
 def test_rate_limit_abandonment_breaks_max_dispatch_batches(make_config, monkeypatch):
@@ -1396,10 +1443,43 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
         )
 
     # The capped path (_fingerprint_until_dispatchable) fingerprints in worker-sized batches (size 1
-    # here). PR1 dispatches, PR2 hits the rate limit and trips the cancel event, so the batch loop
-    # breaks before submitting PR3 -- the cap of 5 is never the limiter, the cancellation is.
+    # here). PR1 completes, PR2 hits the rate limit and trips the cancel event, so the batch loop
+    # breaks before submitting PR3 -- the cap of 5 is never the limiter, the cancellation is. The
+    # tripped event then gates the dispatch phase, so PR1 is deferred rather than dispatched.
     assert fingerprinted == [1, 2]
-    assert dispatched == [1]
+    assert dispatched == []
+
+
+def test_normal_scan_dispatches_when_not_rate_limited(make_config, monkeypatch, caplog):
+    # Guard against over-gating: with no rate limit the cancel event never trips, so the dispatch phase
+    # must run exactly as before -- every completed candidate is dispatched and no deferral warning fires.
+    monkeypatch.setattr(review, "_FINGERPRINT_WORKERS", 1)
+    fingerprinted: list[int] = []
+    dispatched: list[int] = []
+
+    def fingerprint(_client, number, _authorized, _skip):
+        fingerprinted.append(number)
+        return (f"headsha{number}", _HASH_A)
+
+    def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
+        dispatched.append(number)
+
+    with caplog.at_level(logging.WARNING, logger="greenlight"):
+        review.run(
+            make_config(github_token="t"),
+            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: {},
+            dispatch=fake_dispatch,
+            resolve_authorized=lambda: _AUTHORIZED,
+            now=lambda: _NOW,
+        )
+
+    assert fingerprinted == [1, 2, 3]
+    assert dispatched == [1, 2, 3]
+    assert "skipping dispatch" not in caplog.text
+    assert "rate limit" not in caplog.text
 
 
 def test_fetch_failure_still_closes_main_client(make_config):