From c895eec54b50119496cc9c962682d1fafc18c405 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 23 Aug 2026 16:13:49 +0900 Subject: [PATCH 1/3] fix(scheduler): retry and gracefully defer shared installation rate limits pr_review_merge_scheduler.py had no rate-limit detection at all: a shared GitHub App installation-token 403 ("API rate limit exceeded") did not match TRANSIENT_GITHUB_API_ERRORS, so gh_graphql() raised immediately instead of retrying, gh_api_json() had zero retry logic, and the resulting RuntimeError propagated as an undifferentiated per-repository failure that failed the whole org-queue-sweep job. Empirical evidence: 5 sampled rate-limited sweep runs over 15+ hours, 4 of which failed within 5-18 seconds on the very first of 66 swept repositories -- before the sweep's own loop could burn meaningful budget -- pointing at shared cross-workflow contention on installation 141441800 (used by at least 8 other central workflows) rather than this scheduler's own call volume. - Add is_rate_limited_error()/RATE_LIMIT_DIAGNOSTIC_RE, matching the same "API rate limit exceeded" signature scripts/ci/agent_mention_router.py already retries on, kept distinct from is_transient_github_api_error() since it needs a reset-time-aware wait, not a short fixed backoff. - Add rate_limit_retry_delay_seconds(), which reads GET /rate_limit (exempt from the primary limit it reports per GitHub's docs) for the actual reset time, capped at 60s so one repository's invocation cannot stall the sweep; falls back to the existing capped exponential backoff otherwise. - Gate gh_graphql()'s existing retry loop on the new check and give gh_api_json() the same bounded retry it previously lacked entirely. - Teach pr-review-merge-scheduler.yml's org-queue-sweep loop to recognize this signature as a skipped, non-fatal "deferred" repository -- mirroring the existing "Resource not accessible by integration" handling -- instead of counting it toward `failures` and failing the whole sweep job. Deliberately no fail-closed ceiling on this count, unlike ORG_SWEEP_MAX_UNAVAILABLE: contention can legitimately affect most or all repositories in one tick, and that is the expected, self-healing case this branch exists to absorb. Citations: - https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api (installation tokens share one 5,000-12,500/hr bucket; GET /rate_limit does not count against the primary limit) - https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/rate-limits-for-github-apps - https://docs.github.com/rest/guides/best-practices-for-using-the-rest-api (read remaining budget from response headers/`/rate_limit` rather than guessing; honor server-reported reset/retry-after) Out of scope by design (see PR body): deduping the 2-3x redundant actions/runs re-fetches, REST/GraphQL enrichment overlap, and moving repo-scoped calls to the default GITHUB_TOKEN are real, separately-scoped follow-ups, not attempted here. Co-Authored-By: Claude Sonnet 5 --- .../workflows/pr-review-merge-scheduler.yml | 30 +++- scripts/ci/pr_review_merge_scheduler.py | 103 ++++++++++- tests/test_pr_review_merge_scheduler.py | 170 ++++++++++++++++++ .../test_required_workflow_queue_contract.py | 44 +++++ 4 files changed, 337 insertions(+), 10 deletions(-) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index a9bb54f8a..dea890e64 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -960,6 +960,8 @@ jobs: failures=0 unavailable=0 unavailable_repos=() + rate_limited=0 + rate_limited_repos=() # These are organization-wide budgets. They must be consumed across # the repository loop, not reset for every target repository; resetting # them here can enqueue hundreds of long-running review jobs per sweep. @@ -1048,12 +1050,28 @@ jobs: # repository at all — the OpenCode app is not installed there or # PR_REVIEW_MERGE_TOKEN does not cover it. The automation can never # merge those PRs regardless, so this is a skipped, non-fatal - # "unavailable" repository, not a failure the sweep can act on. Any - # other non-zero exit is a genuine per-repository failure. + # "unavailable" repository, not a failure the sweep can act on. + # + # "API rate limit exceeded" means the shared GitHub App + # installation-token bucket (5,000-12,500 requests/hour, pooled + # across at least eight other central workflows that mint tokens + # for the same installation) is exhausted for this hourly window. + # That is routine cross-workflow contention, not a defect in this + # repository, and it self-heals on GitHub's own reset schedule; + # treating it as a hard failure previously turned one exhausted + # bucket into a permanently red */15 * * * * cron for as long as + # the contention lasted. The deferred repository is simply picked + # up again on the next sweep rotation once the bucket refills. + # + # Any other non-zero exit is a genuine per-repository failure. if printf '%s' "$sweep_output" | grep -qF "Resource not accessible by integration"; then echo "::warning::Skipping ${repo_full_name}: the sweep credential lacks access (HTTP 403 Resource not accessible by integration). Install the OpenCode app on this repository or grant PR_REVIEW_MERGE_TOKEN access to include it in the sweep." unavailable=$((unavailable + 1)) unavailable_repos+=("$repo_full_name") + elif printf '%s' "$sweep_output" | grep -qiF "API rate limit exceeded"; then + echo "::warning::Deferring ${repo_full_name}: the shared GitHub App installation-token rate limit is exhausted (HTTP 403 API rate limit exceeded). This repository is retried automatically on the next sweep rotation once the bucket resets." + rate_limited=$((rate_limited + 1)) + rate_limited_repos+=("$repo_full_name") else echo "::error::Queue sweep failed for ${repo_full_name}; see the decision log above for the concrete per-PR reason." failures=$((failures + 1)) @@ -1213,6 +1231,14 @@ jobs: if [ "$unavailable" -gt 0 ]; then echo "::warning::${unavailable} repository(ies) were skipped as unreachable by the sweep credential (HTTP 403): ${unavailable_repos[*]}. These do not fail the sweep; install the OpenCode app or grant PR_REVIEW_MERGE_TOKEN access to include them." fi + if [ "$rate_limited" -gt 0 ]; then + # No fail-closed ceiling here, unlike ORG_SWEEP_MAX_UNAVAILABLE below: + # a shared installation-token bucket exhausted by sibling workflows + # can legitimately affect most or all repositories in a single sweep + # tick, and that is the expected, self-healing failure mode this + # branch exists to absorb, not a credential regression to fail on. + echo "::warning::${rate_limited} repository(ies) were deferred this rotation because the shared GitHub App installation-token rate limit was exhausted: ${rate_limited_repos[*]}. These do not fail the sweep; they are retried automatically once the bucket resets." + fi # Fail-closed guard: a handful of un-enrolled repositories is expected, # but if MORE than ORG_SWEEP_MAX_UNAVAILABLE repositories become # unreachable at once the sweep credential itself has regressed and the diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 44620fcab..1727e3bce 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -718,6 +718,16 @@ def repository_dispatch_wait_reason(repo: str, workflow: str) -> str | None: "unexpected EOF", "received from peer", ) +# The exact diagnostic GitHub emits when a GitHub App installation token's +# shared primary rate limit (5,000-12,500 requests/hour, pooled across every +# workflow that mints a token for the same installation -- at least eight +# other central workflows in this repository alone) is exhausted. Matches +# the pattern scripts/ci/agent_mention_router.py already retries on. Kept +# distinct from TRANSIENT_GITHUB_API_ERRORS because this is routine +# cross-workflow contention, not infrastructure flakiness, and needs a +# reset-time-aware wait rather than a short fixed backoff. +RATE_LIMIT_DIAGNOSTIC_RE = re.compile(r"API rate limit exceeded", re.IGNORECASE) +GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS = 60 def is_transient_github_api_error(exc: Exception) -> bool: @@ -729,6 +739,50 @@ def is_transient_github_api_error(exc: Exception) -> bool: return any(marker in message or marker.lower() in folded for marker in TRANSIENT_GITHUB_API_ERRORS) +def is_rate_limited_error(exc: Exception) -> bool: + """Return whether a GitHub API failure is the shared installation rate limit. + + Distinct from :func:`is_transient_github_api_error`: this is routine + contention from sibling workflows sharing one GitHub App installation's + request bucket, not an infrastructure error, so callers give it a + reset-time-aware wait via :func:`rate_limit_retry_delay_seconds` instead + of the short fixed backoff used for a passing transient failure. + """ + return RATE_LIMIT_DIAGNOSTIC_RE.search(str(exc)) is not None + + +def rate_limit_retry_delay_seconds(resource: str, attempt: int) -> int: + """Return how long to wait before retrying a rate-limited GitHub API call. + + Prefers GitHub's own reported reset time for ``resource`` (``"core"`` + for REST, ``"graphql"`` for GraphQL), read from ``GET /rate_limit`` -- + which GitHub documents as exempt from the primary rate limit it reports, + so checking it does not deepen the exhaustion it is diagnosing. Falls + back to the same capped exponential backoff already used for other + transient errors when that lookup is itself unavailable or does not + confirm the bucket is empty, and never waits longer than + ``GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS`` so one repository's scheduler + invocation cannot stall the whole organization queue sweep; a bucket + that needs longer than that to refill is left for the calling + workflow's skip-and-defer handling to pick back up on the next sweep + rotation instead of blocking this process. + """ + fallback = min(2 ** (attempt - 1), GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS) + try: + status = json.loads(run_github_read(["gh", "api", "rate_limit"])) + bucket = (status.get("resources") or {}).get(resource) or {} + remaining = bucket.get("remaining") + reset_epoch = bucket.get("reset") + except (RuntimeError, json.JSONDecodeError, AttributeError): + return fallback + if remaining != 0 or not isinstance(reset_epoch, int): + return fallback + delay = reset_epoch - int(time.time()) + 5 + if delay <= 0: + return fallback + return min(delay, GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS) + + def gh_graphql(query: str, **fields: str | int) -> dict[str, Any]: """Run a GitHub GraphQL query through gh and decode the JSON response.""" cmd = ["gh", "api", "graphql", "-F", "query=@-"] @@ -740,13 +794,21 @@ def gh_graphql(query: str, **fields: str | int) -> dict[str, Any]: try: return json.loads(run_github_read(cmd, stdin=query)) except (RuntimeError, json.JSONDecodeError) as exc: - if attempt >= max_attempts or not is_transient_github_api_error(exc): + rate_limited = is_rate_limited_error(exc) + if attempt >= max_attempts or not (rate_limited or is_transient_github_api_error(exc)): raise - delay = min(2 ** (attempt - 1), 8) - print( - f"Transient GitHub GraphQL error on attempt {attempt}/{max_attempts}; retrying in {delay}s", - file=sys.stderr, - ) + if rate_limited: + delay = rate_limit_retry_delay_seconds("graphql", attempt) + print( + f"Rate-limited GitHub GraphQL error on attempt {attempt}/{max_attempts}; retrying in {delay}s", + file=sys.stderr, + ) + else: + delay = min(2 ** (attempt - 1), 8) + print( + f"Transient GitHub GraphQL error on attempt {attempt}/{max_attempts}; retrying in {delay}s", + file=sys.stderr, + ) time.sleep(delay) @@ -757,9 +819,34 @@ def github_resource_inaccessible(exc: RuntimeError) -> bool: def gh_api_json(path: str) -> Any: - """Run a GitHub REST API request through gh and decode the JSON response.""" + """Run a GitHub REST API request through gh and decode the JSON response. - return json.loads(run_github_read(["gh", "api", path])) + Retries the shared installation rate limit or another transient GitHub + API error up to ``max_attempts`` times, mirroring :func:`gh_graphql`'s + existing retry convention; any other failure raises immediately exactly + as before. + """ + max_attempts = 4 + for attempt in range(1, max_attempts + 1): # pragma: no branch - last failed attempt always raises + try: + return json.loads(run_github_read(["gh", "api", path])) + except (RuntimeError, json.JSONDecodeError) as exc: + rate_limited = is_rate_limited_error(exc) + if attempt >= max_attempts or not (rate_limited or is_transient_github_api_error(exc)): + raise + if rate_limited: + delay = rate_limit_retry_delay_seconds("core", attempt) + print( + f"Rate-limited GitHub REST error on attempt {attempt}/{max_attempts} for {path}; retrying in {delay}s", + file=sys.stderr, + ) + else: + delay = min(2 ** (attempt - 1), 8) + print( + f"Transient GitHub REST error on attempt {attempt}/{max_attempts} for {path}; retrying in {delay}s", + file=sys.stderr, + ) + time.sleep(delay) def rest_review_node(review: dict[str, Any]) -> dict[str, Any]: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index 0e71bdbe2..8d66a5cec 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -439,6 +439,176 @@ def fake_run(args, stdin=None): assert len(calls) == 1 +def test_is_rate_limited_error_matches_only_the_shared_installation_signature(): + assert sched.is_rate_limited_error( + RuntimeError( + "Command failed (1): gh api graphql\n" + "gh: API rate limit exceeded for installation ID 141441800. (HTTP 403)" + ) + ) + # GitHub's own casing varies by surface; the check must not be case-sensitive. + assert sched.is_rate_limited_error(RuntimeError("gh: api rate limit EXCEEDED for installation ID 1")) + assert not sched.is_rate_limited_error(RuntimeError("Resource not accessible by integration")) + assert not sched.is_rate_limited_error(RuntimeError("Command failed (1): gh api graphql\ngh: HTTP 502")) + assert not sched.is_rate_limited_error( + RuntimeError("gh: You have exceeded a secondary rate limit. Please wait a few minutes.") + ) + + +def test_rate_limit_retry_delay_seconds_uses_the_reported_reset_time(monkeypatch): + calls = [] + + def fake_run(args, stdin=None): + calls.append(args) + return json.dumps({"resources": {"core": {"remaining": 0, "reset": 1_000_050}}}) + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "time", lambda: 1_000_000) + + assert sched.rate_limit_retry_delay_seconds("core", 1) == 55 + assert calls == [["gh", "api", "rate_limit"]] + + +def test_rate_limit_retry_delay_seconds_caps_a_long_reset_wait(monkeypatch): + def fake_run(args, stdin=None): + return json.dumps({"resources": {"graphql": {"remaining": 0, "reset": 1_010_000}}}) + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "time", lambda: 1_000_000) + + assert sched.rate_limit_retry_delay_seconds("graphql", 1) == sched.GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS + + +def test_rate_limit_retry_delay_seconds_falls_back_when_bucket_is_not_empty(monkeypatch): + def fake_run(args, stdin=None): + return json.dumps({"resources": {"core": {"remaining": 42, "reset": 1_000_050}}}) + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "time", lambda: 1_000_000) + + assert sched.rate_limit_retry_delay_seconds("core", 2) == 2 + + +def test_rate_limit_retry_delay_seconds_falls_back_when_reset_is_missing(monkeypatch): + def fake_run(args, stdin=None): + return json.dumps({"resources": {"core": {"remaining": 0}}}) + + monkeypatch.setattr(sched, "run", fake_run) + + assert sched.rate_limit_retry_delay_seconds("core", 3) == 4 + + +def test_rate_limit_retry_delay_seconds_falls_back_when_reset_is_in_the_past(monkeypatch): + def fake_run(args, stdin=None): + return json.dumps({"resources": {"core": {"remaining": 0, "reset": 999_990}}}) + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "time", lambda: 1_000_000) + + assert sched.rate_limit_retry_delay_seconds("core", 1) == 1 + + +def test_rate_limit_retry_delay_seconds_falls_back_on_malformed_payload(monkeypatch): + def fake_run(args, stdin=None): + return json.dumps([]) + + monkeypatch.setattr(sched, "run", fake_run) + + assert sched.rate_limit_retry_delay_seconds("core", 2) == 2 + + +def test_rate_limit_retry_delay_seconds_falls_back_when_lookup_fails(monkeypatch): + def fake_run(args, stdin=None): + raise RuntimeError("Command failed (1): gh api rate_limit\nHTTP 500") + + monkeypatch.setattr(sched, "run", fake_run) + + assert sched.rate_limit_retry_delay_seconds("core", 7) == sched.GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS + + +def test_gh_graphql_retries_rate_limited_errors_using_the_reset_time(monkeypatch): + calls = [] + sleeps = [] + reset_epoch = 1_700_000_100 + + def fake_run(args, stdin=None): + calls.append(args) + if len(args) >= 3 and args[2] == "graphql": + if len(calls) == 1: + raise RuntimeError( + "Command failed (1): gh api graphql\n" + "gh: API rate limit exceeded for installation ID 141441800. (HTTP 403)" + ) + return '{"data":{"repository":{"pullRequests":{"nodes":[],"pageInfo":{"hasNextPage":false}}}}}' + assert args == ["gh", "api", "rate_limit"] + return json.dumps({"resources": {"graphql": {"remaining": 0, "reset": reset_epoch}}}) + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "time", lambda: reset_epoch - 10) + monkeypatch.setattr(sched.time, "sleep", lambda seconds: sleeps.append(seconds)) + + payload = sched.gh_graphql("query", owner="owner", name="repo", pageSize=100) + + assert payload["data"]["repository"]["pullRequests"]["nodes"] == [] + assert sleeps == [15] + + +def test_gh_api_json_retries_rate_limited_errors_then_succeeds(monkeypatch): + calls = [] + sleeps = [] + + def fake_run(args, stdin=None): + calls.append(args) + if args == ["gh", "api", "repos/owner/repo/pulls/1"]: + if len(calls) == 1: + raise RuntimeError( + "Command failed (1): gh api repos/owner/repo/pulls/1\n" + "gh: API rate limit exceeded for installation ID 141441800. (HTTP 403)" + ) + return '{"number": 1}' + assert args == ["gh", "api", "rate_limit"] + # The reset lookup itself failing must not be fatal: the retry falls + # back to capped exponential backoff instead of raising. + raise RuntimeError("Command failed (1): gh api rate_limit\nHTTP 500") + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "sleep", lambda seconds: sleeps.append(seconds)) + + assert sched.gh_api_json("repos/owner/repo/pulls/1") == {"number": 1} + assert sleeps == [1] + + +def test_gh_api_json_retries_transient_errors(monkeypatch): + calls = [] + sleeps = [] + + def fake_run(args, stdin=None): + calls.append(args) + if len(calls) == 1: + raise RuntimeError("Command failed (1): gh api repos/owner/repo/pulls/1\ngh: HTTP 502") + return '{"number": 1}' + + monkeypatch.setattr(sched, "run", fake_run) + monkeypatch.setattr(sched.time, "sleep", lambda seconds: sleeps.append(seconds)) + + assert sched.gh_api_json("repos/owner/repo/pulls/1") == {"number": 1} + assert sleeps == [1] + + +def test_gh_api_json_does_not_retry_non_transient_errors(monkeypatch): + calls = [] + + def fake_run(args, stdin=None): + calls.append(args) + raise RuntimeError("Command failed (1): gh api repos/owner/repo/pulls/1\ngh: HTTP 404") + + monkeypatch.setattr(sched, "run", fake_run) + + with pytest.raises(RuntimeError, match="HTTP 404"): + sched.gh_api_json("repos/owner/repo/pulls/1") + assert calls == [["gh", "api", "repos/owner/repo/pulls/1"]] + + def test_rest_mergeable_state_helpers(monkeypatch): calls = [] diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index d7d0ec8ac..e1fa5a2a7 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1206,6 +1206,50 @@ def test_org_queue_sweep_treats_inaccessible_repositories_as_non_fatal() -> None assert "ORG_SWEEP_MAX_UNAVAILABLE must be a non-negative integer" in workflow +def test_org_queue_sweep_treats_rate_limited_repositories_as_non_fatal() -> None: + """A shared installation-token rate-limit exhaustion must not fail the sweep. + + Installation 141441800's primary rate limit (5,000-12,500 requests/hour) + is shared by at least eight other central workflows that mint tokens for + the same GitHub App installation. When that bucket is exhausted, ``gh`` + fails with "API rate limit exceeded" — routine cross-workflow contention, + not a defect in the target repository — and self-heals on GitHub's own + hourly reset. Treating it as a hard failure previously turned one + exhausted bucket into a permanently red ``*/15 * * * *`` cron for as long + as the contention lasted (observed: repeated same-signature failures + spanning 15+ hours). That repository is now reported as a skipped, + non-fatal "deferred" repository instead, exactly like the existing + inaccessible-repository handling, and is retried on the next rotation. + + Unlike ``ORG_SWEEP_MAX_UNAVAILABLE``, there is deliberately no fail-closed + ceiling on the rate-limited count: contention from sibling workflows can + legitimately affect most or all repositories in one sweep tick, and that + is the expected failure mode this branch exists to absorb, not a + credential-scope regression to fail loudly on. + """ + workflow = workflow_text("pr-review-merge-scheduler.yml") + + # The rate-limit signal is classified as a skipped, non-fatal "deferred" repo. + assert 'grep -qiF "API rate limit exceeded"' in workflow + assert "rate_limited=$((rate_limited + 1))" in workflow + assert 'rate_limited_repos+=("$repo_full_name")' in workflow + assert "the shared GitHub App installation-token rate limit is exhausted" in workflow + assert "retried automatically" in workflow + # It must be checked as its own branch, distinct from both the existing + # 403 "unavailable" classification and the generic hard-failure branch — + # a rate-limited sweep must not also increment unavailable or failures. + assert ( + 'elif printf \'%s\' "$sweep_output" | grep -qiF "API rate limit exceeded"; then' + in workflow + ) + # A genuine (non-403, non-rate-limit) failure must still be a hard failure. + assert "failures=$((failures + 1))" in workflow + # No fail-closed ceiling on rate-limited repositories (see docstring): + # unlike ORG_SWEEP_MAX_UNAVAILABLE, no configured limit ever turns + # widespread rate-limiting into a hard "exit 1" job failure. + assert "ORG_SWEEP_MAX_RATE_LIMITED" not in workflow + + def test_fix_scheduler_cancels_superseded_cron_runs() -> None: """Cancel stale scheduled repair runs before they duplicate mutation work.""" workflow = workflow_text("pr-review-fix-scheduler.yml") From d67ead608e19784587ece0c73efdd475ec8d5786 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Sun, 23 Aug 2026 22:41:20 +0900 Subject: [PATCH 2/3] docs(scheduler): state rate-limit retry scope accurately --- scripts/ci/pr_review_merge_scheduler.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index 1727e3bce..96b5599cc 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -761,11 +761,10 @@ def rate_limit_retry_delay_seconds(resource: str, attempt: int) -> int: back to the same capped exponential backoff already used for other transient errors when that lookup is itself unavailable or does not confirm the bucket is empty, and never waits longer than - ``GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS`` so one repository's scheduler - invocation cannot stall the whole organization queue sweep; a bucket - that needs longer than that to refill is left for the calling - workflow's skip-and-defer handling to pick back up on the next sweep - rotation instead of blocking this process. + ``GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS`` for any one retry interval. + After the bounded attempts are exhausted, the error reaches the calling + workflow's skip-and-defer handling so the repository can be picked back + up on the next sweep rotation. """ fallback = min(2 ** (attempt - 1), GITHUB_API_RATE_LIMIT_RETRY_CAP_SECONDS) try: From 92624300414b19dbed0f96a0295b1ac516181b4b Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 24 Aug 2026 00:37:15 +0900 Subject: [PATCH 3/3] fix(scheduler): stop exhausted installation sweep --- .../workflows/pr-review-merge-scheduler.yml | 20 ++++---- CHANGELOG.md | 6 +++ docs/doctoring/org-queue-sweep-rotation.md | 46 +++++++++++++++++++ .../test_required_workflow_queue_contract.py | 40 ++++++++++++++-- 4 files changed, 100 insertions(+), 12 deletions(-) diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index dea890e64..faf12a40c 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -1060,8 +1060,11 @@ jobs: # repository, and it self-heals on GitHub's own reset schedule; # treating it as a hard failure previously turned one exhausted # bucket into a permanently red */15 * * * * cron for as long as - # the contention lasted. The deferred repository is simply picked - # up again on the next sweep rotation once the bucket refills. + # the contention lasted. Because the installation bucket is shared + # by every remaining repository, the current rotation stops after + # recording the first exhausted request instead of repeating the + # same bounded retries and queue-hygiene calls for every target. + # Deferred work is picked up on a later rotation after reset. # # Any other non-zero exit is a genuine per-repository failure. if printf '%s' "$sweep_output" | grep -qF "Resource not accessible by integration"; then @@ -1069,9 +1072,11 @@ jobs: unavailable=$((unavailable + 1)) unavailable_repos+=("$repo_full_name") elif printf '%s' "$sweep_output" | grep -qiF "API rate limit exceeded"; then - echo "::warning::Deferring ${repo_full_name}: the shared GitHub App installation-token rate limit is exhausted (HTTP 403 API rate limit exceeded). This repository is retried automatically on the next sweep rotation once the bucket resets." + echo "::warning::Deferring ${repo_full_name} and stopping this rotation: the shared GitHub App installation-token rate limit is exhausted (HTTP 403 API rate limit exceeded). Deferred repositories are retried automatically on the next sweep rotation once the bucket resets." rate_limited=$((rate_limited + 1)) rate_limited_repos+=("$repo_full_name") + echo "::endgroup::" + break else echo "::error::Queue sweep failed for ${repo_full_name}; see the decision log above for the concrete per-PR reason." failures=$((failures + 1)) @@ -1233,11 +1238,10 @@ jobs: fi if [ "$rate_limited" -gt 0 ]; then # No fail-closed ceiling here, unlike ORG_SWEEP_MAX_UNAVAILABLE below: - # a shared installation-token bucket exhausted by sibling workflows - # can legitimately affect most or all repositories in a single sweep - # tick, and that is the expected, self-healing failure mode this - # branch exists to absorb, not a credential regression to fail on. - echo "::warning::${rate_limited} repository(ies) were deferred this rotation because the shared GitHub App installation-token rate limit was exhausted: ${rate_limited_repos[*]}. These do not fail the sweep; they are retried automatically once the bucket resets." + # one exhausted shared installation-token bucket affects every + # remaining repository, so the rotation stops after the first + # observed exhaustion instead of multiplying retries and API calls. + echo "::warning::The organization sweep stopped after ${rate_limited} observed rate-limit exhaustion(s): ${rate_limited_repos[*]}. Deferred work does not fail this sweep and is retried automatically once the shared bucket resets." fi # Fail-closed guard: a handful of un-enrolled repositories is expected, # but if MORE than ORG_SWEEP_MAX_UNAVAILABLE repositories become diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b0ef8d44..72c4867f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,12 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Stop the organization PR sweep after the first exhausted shared GitHub App + installation bucket, rather than repeating up to three reset-aware waits and + follow-on queue-hygiene reads for every remaining repository. The current + target is recorded as deferred, the run remains non-fatal for this external + capacity condition, and later rotations retry the unfinished repository set. + - Publish only the sanitized cumulative Strix report tree, avoiding a later copy of relative scanner output that could reintroduce known internal warning text into uploaded security evidence. diff --git a/docs/doctoring/org-queue-sweep-rotation.md b/docs/doctoring/org-queue-sweep-rotation.md index 8146de9fb..cfd0cfaba 100644 --- a/docs/doctoring/org-queue-sweep-rotation.md +++ b/docs/doctoring/org-queue-sweep-rotation.md @@ -90,6 +90,36 @@ ceiling turns out to be conservative. strict per-execution guarantee for that one run, logged as a `::warning::`. +## Shared-installation rate-limit boundary + +The scheduler and several sibling workflows use installation access tokens +from one GitHub App installation. GitHub applies one primary request bucket to +that installation: at least 5,000 requests per hour, scaling by organization +users and repositories to at most 12,500 requests per hour outside GitHub +Enterprise Cloud. In a 30-run scheduler sample, 5 runs failed with the same +primary-limit diagnostic across more than 15 hours; 4 failed on the first of +66 repositories within 5 to 18 seconds. That aggregate timing evidence is +consistent with shared-bucket contention rather than one target repository +consuming the budget. + +REST and GraphQL reads therefore make at most four attempts. Primary-limit +failures use the reset epoch reported by `GET /rate_limit`, capped at 60 +seconds for each retry interval; other transient failures retain the shorter +exponential backoff. GitHub documents that the rate-limit endpoint does not +consume the primary REST budget, although it can consume secondary capacity, +and recommends waiting until the reported reset rather than continuing to +send requests after a primary limit is exhausted. + +If bounded retries still end with `API rate limit exceeded`, the workflow +records the current repository as deferred and stops the organization loop. +The bucket is shared, so visiting the remaining repositories cannot produce +new authoritative state before reset; it would only repeat up to three +one-minute waits per repository and add queue-hygiene requests that GitHub +explicitly advises against. The capacity condition remains non-fatal and the +rotating next execution retries unfinished work. Secondary-limit diagnostics +remain outside this narrow classifier because GitHub gives them a different +retry contract and may provide `Retry-After` instead of a primary reset epoch. + ## Verification - `tests/test_required_workflow_queue_contract.py::test_org_queue_sweep_rotation_offset_is_deterministic_and_reorders_targets` @@ -113,6 +143,10 @@ ceiling turns out to be conservative. locks the `#1219` cross-reference, confirms `github.run_number` is not reintroduced as the source, and confirms the shared budget constant itself is untouched. +- `test_org_queue_sweep_treats_rate_limited_repositories_as_non_fatal` + confirms the primary-limit diagnostic is deferred without becoming a generic + hard failure and that the repository loop stops immediately after recording + the exhausted shared bucket. - `actionlint` (with `shellcheck` on `PATH`) reports no findings against the modified workflow. @@ -125,3 +159,15 @@ per-execution-guarantee review discussion. `ContextualWisdomLab/.github#1223` — wall-clock correction, then the persistent-counter correction this document and the current workflow source reflect. + +GitHub, Inc. (n.d.-a). *Best practices for creating a GitHub App*. GitHub +Docs. Retrieved August 24, 2026, from +https://docs.github.com/en/apps/creating-github-apps/about-creating-github-apps/best-practices-for-creating-a-github-app + +GitHub, Inc. (n.d.-b). *Rate limits for GitHub Apps*. GitHub Docs. Retrieved +August 24, 2026, from +https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/rate-limits-for-github-apps + +GitHub, Inc. (n.d.-c). *Rate limits for the REST API*. GitHub Docs. Retrieved +August 24, 2026, from +https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 6665b29bc..7142717c4 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -1251,10 +1251,10 @@ def test_org_queue_sweep_treats_rate_limited_repositories_as_non_fatal() -> None inaccessible-repository handling, and is retried on the next rotation. Unlike ``ORG_SWEEP_MAX_UNAVAILABLE``, there is deliberately no fail-closed - ceiling on the rate-limited count: contention from sibling workflows can - legitimately affect most or all repositories in one sweep tick, and that - is the expected failure mode this branch exists to absorb, not a - credential-scope regression to fail loudly on. + ceiling on the rate-limited count: one exhausted installation bucket is + shared by every remaining repository, so the sweep records the current + repository and stops the rotation instead of repeating the same bounded + retries and API calls for every later repository. """ workflow = workflow_text("pr-review-merge-scheduler.yml") @@ -1271,6 +1271,38 @@ def test_org_queue_sweep_treats_rate_limited_repositories_as_non_fatal() -> None 'elif printf \'%s\' "$sweep_output" | grep -qiF "API rate limit exceeded"; then' in workflow ) + rate_limited_branch = workflow.split( + 'elif printf \'%s\' "$sweep_output" | grep -qiF "API rate limit exceeded"; then', + maxsplit=1, + )[1].split("\n else\n", maxsplit=1)[0] + assert 'rate_limited_repos+=("$repo_full_name")' in rate_limited_branch + assert 'echo "::endgroup::"' in rate_limited_branch + assert "break" in rate_limited_branch + assert rate_limited_branch.index('rate_limited_repos+=("$repo_full_name")') < ( + rate_limited_branch.index('echo "::endgroup::"') + ) < ( + rate_limited_branch.index("break") + ) + script = ( + "rate_limited=0\n" + "rate_limited_repos=()\n" + "visited_repos=()\n" + "for repo_full_name in ContextualWisdomLab/first ContextualWisdomLab/second; do\n" + " visited_repos+=(\"$repo_full_name\")\n" + + textwrap.indent(textwrap.dedent(rate_limited_branch).strip() + "\n", " ") + + "done\n" + + "printf 'RESULT|%s|%s|%s\\n' \"$rate_limited\" " + '"${rate_limited_repos[*]}" "${visited_repos[*]}"\n' + ) + result = subprocess.run( + ["bash", "-euo", "pipefail", "-c", script], + capture_output=True, + text=True, + ) + assert result.returncode == 0, result.stderr + assert result.stdout.splitlines()[-1] == ( + "RESULT|1|ContextualWisdomLab/first|ContextualWisdomLab/first" + ) # A genuine (non-403, non-rate-limit) failure must still be a hard failure. assert "failures=$((failures + 1))" in workflow # No fail-closed ceiling on rate-limited repositories (see docstring):