Update

[ghstack-poisoned]

Author: jeanschmidt

greenlight/src/greenlight/review.py

@@ -73,10 +73,10 @@ def _is_trusted(login: str | None) -> bool:
     return login is not None and login.lower() in _TRUSTED_LOWER
 
 
-# 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.
+# 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
 
@@ -257,6 +257,7 @@ def run(
         else:
             fingerprint_numbers = pr_numbers
         failed: list[int] = []
+        abandoned: list[int] = []
         skips: list[tuple[int, ReviewSkip]] = []
         worker_count = min(_FINGERPRINT_WORKERS, len(fingerprint_numbers))
         # PyGithub is not thread-safe, so each concurrent task borrows a client for its
@@ -279,6 +280,7 @@ def run(
                 now=evaluated_at,
                 timeout=timeout,
                 failed=failed,
+                abandoned=abandoned,
                 skips=skips,
                 force=force,
             )
@@ -295,9 +297,17 @@ def run(
                 now=evaluated_at,
                 timeout=timeout,
                 failed=failed,
+                abandoned=abandoned,
                 skips=skips,
                 force=force,
             )
+        if abandoned:
+            logger.warning(
+                "rate limit hit: abandoned %d of %d fingerprint(s) (not evaluated); %d candidate(s) dispatchable",
+                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
         )
@@ -308,10 +318,12 @@ def run(
             scan_runner.post_refusals(
                 client, TARGET_REPO, skips, bot_login=bot_login, get_pr=get_pr, upsert_comment=upsert_comment
             )
-        if failed or dispatch_failed:
+        if failed or dispatch_failed or abandoned:
             errors: list[str] = []
             if failed:
                 errors.append(f"{len(failed)} PR(s) failed during scan: {sorted(failed)}")
             if dispatch_failed:
                 errors.append(f"failed to dispatch {len(dispatch_failed)} PR(s): {sorted(dispatch_failed)}")
+            if abandoned:
+                errors.append(f"{len(abandoned)} PR(s) abandoned due to rate limit: {sorted(abandoned)}")
             raise RuntimeError("; ".join(errors))

greenlight/src/greenlight/scan_runner.py

@@ -102,14 +102,17 @@ def _evaluate_pr(
     now: datetime,
     timeout: timedelta,
     failed: list[int],
+    abandoned: list[int],
     skips: list[tuple[int, ReviewSkip]],
     force: bool,
 ) -> _Candidate | None:
     try:
         result = future.result()
         # A cancelled task never ran its fingerprint (an earlier task hit a rate limit and tripped
-        # the shared cancel event), so it is neither a candidate nor a failure: drop it silently.
+        # the shared cancel event), so it is not a failure: record it as abandoned (never evaluated),
+        # kept distinct from `failed` so the end-of-scan signal can report both without conflating them.
         if result is _CANCELLED:
+            abandoned.append(number)
             return None
         # A ReviewSkip is a human decision, never a fingerprint failure: check before unpacking
         # so it is collected/dropped, not mistaken for an error and appended to failed.
@@ -154,6 +157,7 @@ def _fingerprint_all(
     now: datetime,
     timeout: timedelta,
     failed: list[int],
+    abandoned: list[int],
     skips: list[tuple[int, ReviewSkip]],
     force: bool,
 ) -> list[_Candidate]:
@@ -176,7 +180,15 @@ def _fingerprint_all(
     pending: list[_Candidate] = []
     for number in pr_numbers:
         candidate = _evaluate_pr(
-            number, futures[number], states, now=now, timeout=timeout, failed=failed, skips=skips, force=force
+            number,
+            futures[number],
+            states,
+            now=now,
+            timeout=timeout,
+            failed=failed,
+            abandoned=abandoned,
+            skips=skips,
+            force=force,
         )
         if candidate is not None:
             pending.append(candidate)
@@ -196,6 +208,7 @@ def _fingerprint_until_dispatchable(
     now: datetime,
     timeout: timedelta,
     failed: list[int],
+    abandoned: list[int],
     skips: list[tuple[int, ReviewSkip]],
     force: bool,
 ) -> list[_Candidate]:
@@ -230,6 +243,7 @@ def _fingerprint_until_dispatchable(
                         now=now,
                         timeout=timeout,
                         failed=failed,
+                        abandoned=abandoned,
                         skips=skips,
                         force=force,
                     )

greenlight/tests/test_review.py

@@ -1292,7 +1292,7 @@ def boom(_client, _number, _authorized, _skip):
     assert not cancel_event.is_set()
 
 
-def test_rate_limit_abandons_remaining_fingerprints(make_config, monkeypatch):
+def test_rate_limit_abandons_remaining_fingerprints(make_config, monkeypatch, caplog):
     from github import RateLimitExceededException
 
     # Pin one worker for a deterministic FIFO order: PR1 runs first and trips the cancel event before
@@ -1310,7 +1310,7 @@ def fingerprint(_client, number, _authorized, _skip):
     def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
         dispatched.append(number)
 
-    with pytest.raises(RuntimeError, match=r"1 PR\(s\) failed during scan: \[1\]"):
+    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,
@@ -1323,13 +1323,20 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
         )
 
     # PR1's rate limit sets the shared cancel event, so the queued PR2/PR3 short-circuit to _CANCELLED
-    # without ever calling fingerprint (only PR1 appears). A cancelled task is not a failure, so only
-    # PR1 lands in `failed`, and nothing dispatches.
+    # without ever calling fingerprint (only PR1 appears). A cancelled task is not a failure: PR1 lands
+    # in `failed`, PR2/PR3 are counted as abandoned (not failed), and nothing dispatches.
     assert fingerprinted == [1]
     assert dispatched == []
+    # The abandonment is surfaced, never silently dropped: a warning names the count, and the
+    # end-of-scan RuntimeError carries both the failed PR and the abandoned ones distinctly so the
+    # scan still fails closed on an incomplete pass.
+    message = str(excinfo.value)
+    assert "1 PR(s) failed during scan: [1]" in message
+    assert "2 PR(s) abandoned due to rate limit: [2, 3]" in message
+    assert "abandoned 2 of 3" in caplog.text
 
 
-def test_rate_limit_still_dispatches_earlier_completed_candidate(make_config, monkeypatch):
+def test_rate_limit_still_dispatches_earlier_completed_candidate(make_config, monkeypatch, caplog):
     from github import RateLimitExceededException
 
     monkeypatch.setattr(review, "_FINGERPRINT_WORKERS", 1)
@@ -1345,7 +1352,7 @@ def fingerprint(_client, number, _authorized, _skip):
     def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
         dispatched.append(number)
 
-    with pytest.raises(RuntimeError, match=r"1 PR\(s\) failed during scan: \[2\]"):
+    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,
@@ -1359,9 +1366,15 @@ def fake_dispatch(_client, number, _head_sha, _eval_hash, _ref):
 
     # 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, and only PR2 lands in `failed`.
+    # fingerprinted: PR2 lands in `failed`, PR3 is counted as abandoned (not failed).
     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.
+    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_abandonment_breaks_max_dispatch_batches(make_config, monkeypatch):

greenlight/tests/test_scan_runner.py

@@ -167,6 +167,7 @@ def upsert(pr, **_kwargs):
 
 def test_evaluate_pr_iteration_timeout_propagates():
     failed: list[int] = []
+    abandoned: list[int] = []
     skips: list[tuple[int, ReviewSkip]] = []
     future = cast(
         "Future[tuple[str, str] | ReviewSkip | scan_runner._Cancelled]", _RaisingFuture(IterationTimeout("boom"))
@@ -182,11 +183,13 @@ def test_evaluate_pr_iteration_timeout_propagates():
             now=datetime(2026, 1, 1, tzinfo=UTC),
             timeout=timedelta(hours=1),
             failed=failed,
+            abandoned=abandoned,
             skips=skips,
             force=False,
         )
 
     assert failed == []
+    assert abandoned == []
     assert skips == []
 
 
@@ -264,8 +267,9 @@ def fingerprint(*_args):
     assert pool.get_nowait() is client
 
 
-def test_evaluate_pr_cancelled_result_returns_none_without_failing():
+def test_evaluate_pr_cancelled_result_records_abandoned_not_failed():
     failed: list[int] = []
+    abandoned: list[int] = []
     skips: list[tuple[int, ReviewSkip]] = []
     future = cast(
         "Future[tuple[str, str] | ReviewSkip | scan_runner._Cancelled]", _ResultFuture(scan_runner._CANCELLED)
@@ -278,13 +282,16 @@ def test_evaluate_pr_cancelled_result_returns_none_without_failing():
         now=datetime(2026, 1, 1, tzinfo=UTC),
         timeout=timedelta(hours=1),
         failed=failed,
+        abandoned=abandoned,
         skips=skips,
         force=False,
     )
 
-    # A cancelled task was never attempted, so it is neither a candidate nor a failure: it is dropped
-    # without landing in `failed` (only tasks that actually hit the limit do).
+    # A cancelled task was never attempted, so it is not a candidate and not a failure: it is recorded
+    # as abandoned (distinct from `failed`, which holds only tasks that actually hit the limit) so the
+    # scan can surface it instead of dropping it silently.
     assert result is None
+    assert abandoned == [5]
     assert failed == []
     assert skips == []