diff --git a/.github/aicodingflow-tests/test_post_pr_review.py b/.github/aicodingflow-tests/test_post_pr_review.py index e2ca9ac..582653f 100644 --- a/.github/aicodingflow-tests/test_post_pr_review.py +++ b/.github/aicodingflow-tests/test_post_pr_review.py @@ -6,6 +6,7 @@ import os import tempfile import unittest +import urllib.error from pathlib import Path from unittest import mock @@ -16,6 +17,72 @@ class PostPrReviewTest(unittest.TestCase): + def test_github_api_response_retries_network_failures_with_timeout(self) -> None: + calls = [] + + class Response: + headers = {} + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def read(self): + return b'{"id": 7}' + + def fake_urlopen(request, *, timeout): + calls.append((request.full_url, timeout)) + if len(calls) < 3: + raise urllib.error.URLError("temporary DNS failure") + return Response() + + with ( + mock.patch.object(post_pr_review.urllib.request, "urlopen", side_effect=fake_urlopen), + mock.patch.object(post_pr_review.time, "sleep") as sleep, + ): + response = post_pr_review.github_api_response("https://api.github.com/test", "token") + + self.assertEqual(response.data, {"id": 7}) + self.assertEqual(calls, [("https://api.github.com/test", 30)] * 3) + self.assertEqual([call.args[0] for call in sleep.call_args_list], [1, 2]) + + def test_github_api_response_stops_after_network_retry_limit(self) -> None: + with ( + mock.patch.object( + post_pr_review.urllib.request, + "urlopen", + side_effect=urllib.error.URLError("connection refused"), + ) as urlopen, + mock.patch.object(post_pr_review.time, "sleep") as sleep, + ): + with self.assertRaisesRegex(SystemExit, "after 3 attempts"): + post_pr_review.github_api_response("https://api.github.com/test", "token") + + self.assertEqual(urlopen.call_count, 3) + self.assertEqual([call.args[0] for call in sleep.call_args_list], [1, 2]) + + def test_github_api_response_does_not_retry_post_requests(self) -> None: + with ( + mock.patch.object( + post_pr_review.urllib.request, + "urlopen", + side_effect=urllib.error.URLError("response read timeout"), + ) as urlopen, + mock.patch.object(post_pr_review.time, "sleep") as sleep, + ): + with self.assertRaisesRegex(SystemExit, "after 1 attempt"): + post_pr_review.github_api_response( + "https://api.github.com/repos/owner/repo/pulls/1/reviews", + "token", + method="POST", + payload={"body": "review"}, + ) + + urlopen.assert_called_once() + sleep.assert_not_called() + def test_review_event_matrix_keeps_member_and_spec_reviews_as_comments(self) -> None: member_pr = {"author_association": "MEMBER", "user": {"login": "member", "type": "User"}} non_member_pr = {"author_association": "FIRST_TIMER", "user": {"login": "external", "type": "User"}} diff --git a/.github/scripts/post_pr_review.py b/.github/scripts/post_pr_review.py index 3360e71..4f78c60 100644 --- a/.github/scripts/post_pr_review.py +++ b/.github/scripts/post_pr_review.py @@ -7,6 +7,7 @@ import json import os import re +import time import urllib.error import urllib.request from pathlib import Path @@ -19,6 +20,10 @@ ORG_MEMBER_ASSOCIATIONS = {"COLLABORATOR", "MEMBER", "OWNER"} NON_MEMBER_ASSOCIATIONS = {"CONTRIBUTOR", "FIRST_TIMER", "FIRST_TIME_CONTRIBUTOR", "NONE"} DEFAULT_REVIEW_BOT_LOGIN = "github-actions[bot]" +GITHUB_API_TIMEOUT_SECONDS = 30 +GITHUB_API_MAX_ATTEMPTS = 3 +GITHUB_API_BACKOFF_SECONDS = 1 +GITHUB_API_RETRYABLE_METHODS = {"DELETE", "GET", "HEAD", "OPTIONS", "PUT"} class CodeownersRule(NamedTuple): @@ -67,13 +72,23 @@ def github_api_response( }, method=method, ) - try: - with urllib.request.urlopen(request) as response: - body = response.read().decode("utf-8") - return GitHubResponse(json.loads(body) if body else {}, response.headers) - except urllib.error.HTTPError as exc: - detail = exc.read().decode("utf-8", errors="replace") - raise SystemExit(f"GitHub API request failed: {exc.code} {detail}") from exc + max_attempts = GITHUB_API_MAX_ATTEMPTS if method.upper() in GITHUB_API_RETRYABLE_METHODS else 1 + for attempt in range(1, max_attempts + 1): + try: + with urllib.request.urlopen(request, timeout=GITHUB_API_TIMEOUT_SECONDS) as response: + body = response.read().decode("utf-8") + return GitHubResponse(json.loads(body) if body else {}, response.headers) + except urllib.error.HTTPError as exc: + detail = exc.read().decode("utf-8", errors="replace") + raise SystemExit(f"GitHub API request failed: {exc.code} {detail}") from exc + except (urllib.error.URLError, TimeoutError) as exc: + if attempt == max_attempts: + attempt_label = "attempt" if max_attempts == 1 else "attempts" + raise SystemExit( + "GitHub API request failed after " + f"{max_attempts} {attempt_label}: {exc}" + ) from exc + time.sleep(GITHUB_API_BACKOFF_SECONDS * (2 ** (attempt - 1))) def request_json(url: str, token: str, payload: dict[str, Any]) -> dict[str, Any]: