diff --git a/CHANGELOG.md b/CHANGELOG.md index 44145d4ec..16a083dbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,31 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Root-cause the hourly PR-review-fix scheduler's silent `autofix_dispatches: 0` + on nearly every run (surfaced while investigating why 40 of `.github`'s 81 + open PRs were stuck reporting "This branch has conflicts that must be + resolved"): `github-hourly-review-repair.yml`'s most recent run inspected + 50 PRs and dispatched zero autofixes, with every candidate PR's decision + reading `"error": "API rate limit exceeded for installation ID ..."`. Two + compounding causes in `scripts/ci/pr_review_fix_scheduler.py`: (1) + `issue_comments()` fetched a PR's *entire* issue-comment history with the + default 30-per-page pagination even though `recent_fix_marker_exists()` + only ever needs the most recent marker; (2) `process_queue()`'s concurrent + comment-prefetch (up to 10 simultaneous `gh api --paginate` calls against + the same shared, org-wide-contended OpenCode app installation) silently + swallowed a failed fetch and then had `inspect_pr()` immediately retry the + *same* doomed call sequentially with zero backoff, doubling the wasted + request volume for every already-failing PR. `issue_comments()` now + requests `per_page=100` (cutting page count for long comment threads by + up to 3x) and retries a detected rate-limit error with a short linear + backoff (up to 2 attempts) before propagating; `process_queue()` now + caps prefetch concurrency at 4 workers instead of 10, and a PR whose + comment fetch still fails after retries is deferred to the next scheduled + pass (`"wait"`) instead of silently prefetch-swallowed and then + redundantly re-fetched and reported as a scary `"error"`. This is a + single shared script, so the fix applies identically to every one of the + ~19 product-specific hourly review-repair callers, not just `.github`'s + own. - Fix a Devin Review finding on PR #1456: the REST fallback path (`rest_pr_node`, used when GraphQL is unavailable) only ever fetched a head commit's CheckRuns (`commits/{sha}/check-runs`), never its classic @@ -443,8 +468,7 @@ Semantic Versioning where the repository publishes a release. Informational, no change: the gap-baseline's repeated review-round narrative is this repo's own documented, intentional convention (ADR-0002: the baseline is "an operational snapshot," not a duplicate of - the ADR's design record), not accidental redundancy. -- Raise `contextual_orchestrator_review_sidecar.sh`'s + the ADR's design record), not accidental redundancy.- Raise `contextual_orchestrator_review_sidecar.sh`'s `ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the live "no provider route passed the Strix plain-chat preflight" outage blocking `noema-review`/`opencode-review`/`strix` org-wide to diff --git a/scripts/ci/pr_review_fix_scheduler.py b/scripts/ci/pr_review_fix_scheduler.py index cde53977a..79a838864 100755 --- a/scripts/ci/pr_review_fix_scheduler.py +++ b/scripts/ci/pr_review_fix_scheduler.py @@ -76,12 +76,47 @@ def run_json(args: list[str]) -> Any: return json.loads(run(["gh", *args]) or "null") +RATE_LIMIT_ERROR_MARKERS = ("api rate limit exceeded", "secondary rate limit") +ISSUE_COMMENTS_RETRY_ATTEMPTS = 2 +ISSUE_COMMENTS_RETRY_BACKOFF_SECONDS = 15 + + +def is_rate_limit_error(exc: BaseException) -> bool: + """Return whether an exception's message names a GitHub API rate limit.""" + message = str(exc).lower() + return any(marker in message for marker in RATE_LIMIT_ERROR_MARKERS) + + def issue_comments(repo: str, number: int) -> list[dict[str, Any]]: - """Return issue comments for a PR.""" - pages = run_json( - ["api", f"repos/{repo}/issues/{number}/comments", "--paginate", "--slurp"] - ) - return [comment for page in pages for comment in page] + """Return issue comments for a PR, retrying transient rate-limit errors. + + The shared OpenCode app installation's API budget is contended by many + concurrent org-wide scheduled workflows, so a rate-limit error here is + often transient. Retry it with a short linear backoff up to + ISSUE_COMMENTS_RETRY_ATTEMPTS times before propagating; any other error, + or a rate-limit error past the retry budget, propagates immediately. + ``per_page=100`` bounds the paginated request count for PRs with a long + review-comment history. + """ + attempt = 0 + while True: + try: + pages = run_json( + [ + "api", + f"repos/{repo}/issues/{number}/comments", + "--paginate", + "--slurp", + "-f", + "per_page=100", + ] + ) + return [comment for page in pages for comment in page] + except RuntimeError as exc: + if attempt >= ISSUE_COMMENTS_RETRY_ATTEMPTS or not is_rate_limit_error(exc): + raise + attempt += 1 + time.sleep(ISSUE_COMMENTS_RETRY_BACKOFF_SECONDS * attempt) def recent_fix_marker_exists( @@ -384,12 +419,20 @@ def process_queue(args: argparse.Namespace) -> int: prs_needing_comments.append(pr) comments_by_pr: dict[int, list[dict[str, Any]]] = {} + comment_fetch_errors: dict[int, str] = {} if len(prs_needing_comments) <= 1: for pr in prs_needing_comments: pr_number = int(pr["number"]) - comments_by_pr[pr_number] = issue_comments(args.repo, pr_number) + try: + comments_by_pr[pr_number] = issue_comments(args.repo, pr_number) + except Exception as exc: + comment_fetch_errors[pr_number] = str(exc) else: - max_workers = min(10, len(prs_needing_comments)) + # Bounded well below GitHub's per-installation rate-limit budget: this + # scheduler is one of many concurrent org-wide callers sharing the + # same OpenCode app installation, so a wide burst of simultaneous + # comment fetches here can exhaust that shared budget on its own. + max_workers = min(4, len(prs_needing_comments)) with concurrent.futures.ThreadPoolExecutor( max_workers=max_workers ) as executor: @@ -400,16 +443,17 @@ def fetch_comments( """Fetch one PR's issue comments for parallel queue inspection.""" return pr_number, issue_comments(args.repo, pr_number) - futures = [ - executor.submit(fetch_comments, int(pr["number"])) + futures = { + executor.submit(fetch_comments, int(pr["number"])): int(pr["number"]) for pr in prs_needing_comments - ] + } for future in concurrent.futures.as_completed(futures): + pr_number = futures[future] try: - pr_number, comments = future.result() + _, comments = future.result() comments_by_pr[pr_number] = comments - except Exception: - pass + except Exception as exc: + comment_fetch_errors[pr_number] = str(exc) for pr in prs: inspected += 1 @@ -422,8 +466,24 @@ def fetch_comments( } ) continue + pr_number = int(pr["number"]) + if pr_number in comment_fetch_errors: + decisions.append( + { + "pr": pr["number"], + "action": "wait", + "reasons": [ + "issue comment fetch failed; deferring to next scheduled " + f"pass: {comment_fetch_errors[pr_number]}" + ], + } + ) + print( + f"PR #{pr['number']}: wait: issue comment fetch failed; " + "deferring to next scheduled pass" + ) + continue try: - pr_number = int(pr["number"]) action, reasons = inspect_pr( args.repo, pr, diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 39ae12ce1..8bd4cfcd3 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -628,6 +628,122 @@ def fake_run(argv, *, stdin=None): assert payload["client_payload"]["target_repository"] == "owner/repo" +def test_is_rate_limit_error_matches_known_github_signatures(): + """Rate-limit detection matches GitHub's primary and secondary wording.""" + assert fix.is_rate_limit_error(RuntimeError("gh: API rate limit exceeded for installation ID 1")) + assert fix.is_rate_limit_error(RuntimeError("You have exceeded a Secondary rate limit")) + assert not fix.is_rate_limit_error(RuntimeError("gh: Resource not accessible by integration")) + + +def test_issue_comments_retries_rate_limit_then_succeeds(monkeypatch): + """A transient rate-limit error is retried with backoff before succeeding.""" + calls = [] + sleeps = [] + attempts = {"count": 0} + + def fake_run(argv, *, stdin=None): + calls.append(argv) + attempts["count"] += 1 + if attempts["count"] < 2: + raise RuntimeError("gh: API rate limit exceeded for installation ID 1") + return "[[{\"id\": 1}]]" + + monkeypatch.setattr(fix, "run", fake_run) + monkeypatch.setattr(fix.time, "sleep", lambda seconds: sleeps.append(seconds)) + + assert fix.issue_comments("owner/repo", 7) == [{"id": 1}] + assert len(calls) == 2 + assert sleeps == [fix.ISSUE_COMMENTS_RETRY_BACKOFF_SECONDS] + assert all("per_page=100" in argv for argv in calls) + + +def test_issue_comments_exhausts_retries_and_raises(monkeypatch): + """A persistent rate-limit error still propagates once retries are spent.""" + sleeps = [] + + def always_rate_limited(argv, *, stdin=None): + raise RuntimeError("gh: API rate limit exceeded for installation ID 1") + + monkeypatch.setattr(fix, "run", always_rate_limited) + monkeypatch.setattr(fix.time, "sleep", lambda seconds: sleeps.append(seconds)) + + with pytest.raises(RuntimeError, match="rate limit exceeded"): + fix.issue_comments("owner/repo", 7) + assert len(sleeps) == fix.ISSUE_COMMENTS_RETRY_ATTEMPTS + + +def test_issue_comments_does_not_retry_non_rate_limit_errors(monkeypatch): + """A non-rate-limit failure propagates immediately without backoff.""" + sleeps = [] + + def fail_once(argv, *, stdin=None): + raise RuntimeError("gh: Resource not accessible by integration") + + monkeypatch.setattr(fix, "run", fail_once) + monkeypatch.setattr(fix.time, "sleep", lambda seconds: sleeps.append(seconds)) + + with pytest.raises(RuntimeError, match="not accessible"): + fix.issue_comments("owner/repo", 7) + assert sleeps == [] + + +def test_process_queue_defers_prs_whose_comment_fetch_failed(monkeypatch, capsys): + """A single failing comment fetch defers that PR instead of erroring.""" + pr = make_pr() + monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr]) + monkeypatch.setattr(fix, "needs_autofix", lambda pr: (True, ("reason",))) + + def failing_issue_comments(repo, number): + raise RuntimeError("gh: API rate limit exceeded for installation ID 1") + + monkeypatch.setattr(fix, "issue_comments", failing_issue_comments) + inspect_calls = [] + monkeypatch.setattr( + fix, + "inspect_pr", + lambda repo, pr, args, **kwargs: inspect_calls.append(kwargs) or ("dispatch", ("reason",)), + ) + + assert fix.main(["--repo", "owner/repo", "--base-branch", "main", "--dry-run"]) == 0 + + assert inspect_calls == [] + payload = json.loads(capsys.readouterr().out.strip().splitlines()[-1]) + assert payload["autofix_dispatches"] == 0 + assert payload["decisions"][0]["action"] == "wait" + assert "deferring to next scheduled pass" in payload["decisions"][0]["reasons"][0] + + +def test_process_queue_concurrent_fetch_defers_only_the_failing_pr(monkeypatch, capsys): + """The concurrent comment-fetch path defers only the PR whose fetch failed.""" + pr1 = make_pr(number=1) + pr2 = make_pr(number=2) + monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr1, pr2]) + monkeypatch.setattr(fix, "needs_autofix", lambda pr: (True, ("reason",))) + + def flaky_issue_comments(repo, number): + if number == 1: + raise RuntimeError("gh: API rate limit exceeded for installation ID 1") + return [] + + monkeypatch.setattr(fix, "issue_comments", flaky_issue_comments) + inspect_calls = [] + + def fake_inspect_pr(repo, pr, args, **kwargs): + inspect_calls.append((pr["number"], kwargs.get("comments"))) + return "dispatch", ("reason",) + + monkeypatch.setattr(fix, "inspect_pr", fake_inspect_pr) + + assert fix.main(["--repo", "owner/repo", "--base-branch", "main", "--dry-run", "--max-dispatches", "2"]) == 0 + + assert inspect_calls == [(2, [])] + payload = json.loads(capsys.readouterr().out.strip().splitlines()[-1]) + decisions_by_pr = {d["pr"]: d for d in payload["decisions"]} + assert decisions_by_pr[1]["action"] == "wait" + assert "deferring to next scheduled pass" in decisions_by_pr[1]["reasons"][0] + assert decisions_by_pr[2]["action"] == "dispatch" + + def _approved_dirty_pr(**overrides): """Return an approved PR that GitHub reports as conflicting.""" fields = {