From e00b82d16402255444a82e15663146500c09c058 Mon Sep 17 00:00:00 2001 From: Matias Varnum Date: Mon, 16 Feb 2026 13:49:51 +0100 Subject: [PATCH] github: Clean up retry backoff per PR #222 review feedback Address jerry-skydio's review comments: - Rename _retry_with_backoff to _should_retry to reflect the return value - Extract transient_statuses and retryable_graphql_errors to module-level frozenset constants, make max_retries and base_delay kwargs on graphql() - Split graphql into _graphql_once (single attempt) and graphql (retry wrapper) to reduce indentation and separate concerns --- revup/github_real.py | 125 +++++++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 59 deletions(-) diff --git a/revup/github_real.py b/revup/github_real.py index 0c9a57d..f6989c9 100644 --- a/revup/github_real.py +++ b/revup/github_real.py @@ -10,6 +10,9 @@ from revup import github from revup.types import RevupGithubException, RevupRequestException +TRANSIENT_STATUSES = frozenset({500, 502, 503, 504}) +RETRYABLE_GRAPHQL_ERRORS = frozenset({"RESOURCE_LIMITS_EXCEEDED"}) + class RealGitHubEndpoint(github.GitHubEndpoint): """ @@ -54,7 +57,7 @@ async def close(self) -> None: if self.session: await self.session.close() - async def _retry_with_backoff( + async def _should_retry( self, attempt: int, max_retries: int, base_delay: float, message: str ) -> bool: """Sleep with exponential backoff if retries remain. Returns True to retry.""" @@ -67,16 +70,11 @@ async def _retry_with_backoff( await asyncio.sleep(delay) return True - async def graphql(self, query: str, **kwargs: Any) -> Any: + async def _graphql_once(self, query: str, **kwargs: Any) -> Any: + """Execute a single GraphQL request. Raises on any error.""" if self.session is None: self.session = ClientSession() - # Retry config: 3 attempts with exponential backoff (1s, 2s, 4s) - max_retries = 3 - base_delay = 1.0 - transient_statuses = {500, 502, 503, 504} - retryable_graphql_errors = {"RESOURCE_LIMITS_EXCEEDED"} - headers = {} if self.oauth_token: headers["Authorization"] = "bearer {}".format(self.oauth_token) @@ -85,60 +83,69 @@ async def graphql(self, query: str, **kwargs: Any) -> Any: logging.debug("Request GraphQL query:\n{}".format(query)) logging.debug("Request GraphQL variables:\n{}".format(json.dumps(kwargs, indent=1))) - for attempt in range(max_retries): - start_time = time.time() - async with self.session.post( - self.graphql_endpoint, - json={"query": query, "variables": kwargs}, - headers=headers, - proxy=self.proxy, - ) as resp: - logging.debug( - "Response status: {} took {}".format(resp.status, time.time() - start_time) + start_time = time.time() + async with self.session.post( + self.graphql_endpoint, + json={"query": query, "variables": kwargs}, + headers=headers, + proxy=self.proxy, + ) as resp: + logging.debug( + "Response status: {} took {}".format(resp.status, time.time() - start_time) + ) + ratelimit_reset = resp.headers.get("x-ratelimit-reset") + if ratelimit_reset is not None: + reset_timestamp = datetime.datetime.fromtimestamp(int(ratelimit_reset)).isoformat() + else: + reset_timestamp = "Unknown" + logging.debug( + "Ratelimit: {} remaining, resets at {}".format( + resp.headers.get("x-ratelimit-remaining"), + reset_timestamp, ) - ratelimit_reset = resp.headers.get("x-ratelimit-reset") - if ratelimit_reset is not None: - reset_timestamp = datetime.datetime.fromtimestamp( - int(ratelimit_reset) - ).isoformat() - else: - reset_timestamp = "Unknown" - logging.debug( - "Ratelimit: {} remaining, resets at {}".format( - resp.headers.get("x-ratelimit-remaining"), - reset_timestamp, - ) - ) - - if resp.status in transient_statuses: - msg = "GitHub returned {}".format(resp.status) - if await self._retry_with_backoff(attempt, max_retries, base_delay, msg): - continue - logging.warning("Response body:\n{}".format(await resp.text())) - raise RevupRequestException(resp.status, {}) + ) + if resp.status != 200: try: r = await resp.json() - except (ValueError, ContentTypeError): + except (ValueError, ContentTypeError) as exc: logging.warning("Response body:\n{}".format(await resp.text())) + raise RevupRequestException(resp.status, {}) from exc + raise RevupRequestException(resp.status, r) + + try: + r = await resp.json() + except (ValueError, ContentTypeError): + logging.warning("Response body:\n{}".format(await resp.text())) + raise + else: + pretty_json = json.dumps(r, indent=1) + logging.debug("Response JSON:\n{}".format(pretty_json)) + + if "errors" in r: + raise RevupGithubException(r["errors"]) + + return r + + async def graphql( + self, query: str, *, max_retries: int = 3, base_delay: float = 1.0, **kwargs: Any + ) -> Any: + for attempt in range(max_retries): + try: + return await self._graphql_once(query, **kwargs) + except RevupRequestException as e: + if e.status not in TRANSIENT_STATUSES: + raise + msg = "GitHub returned {}".format(e.status) + if not await self._should_retry(attempt, max_retries, base_delay, msg): + raise + except RevupGithubException as e: + retryable = set(e.types) & RETRYABLE_GRAPHQL_ERRORS + if not retryable: + raise + # TODO: For RESOURCE_LIMITS_EXCEEDED, use x-ratelimit-reset header + # instead of exponential backoff - either wait until reset time or + # fail immediately if the wait would be too long. + msg = "GitHub GraphQL error ({})".format(", ".join(retryable)) + if not await self._should_retry(attempt, max_retries, base_delay, msg): raise - else: - pretty_json = json.dumps(r, indent=1) - logging.debug("Response JSON:\n{}".format(pretty_json)) - - if "errors" in r: - error_types = {err.get("type", "Unknown") for err in r["errors"]} - # TODO: For RESOURCE_LIMITS_EXCEEDED, use x-ratelimit-reset header - # instead of exponential backoff - either wait until reset time or - # fail immediately if the wait would be too long. - if error_types & retryable_graphql_errors: - matched = ", ".join(error_types & retryable_graphql_errors) - msg = "GitHub GraphQL error ({})".format(matched) - if await self._retry_with_backoff(attempt, max_retries, base_delay, msg): - continue - raise RevupGithubException(r["errors"]) - - if resp.status != 200: - raise RevupRequestException(resp.status, r) - - return r