Skip to content

Clean up retry backoff per PR #222 review #223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
matias-christensen-skydio wants to merge 1 commit into
base: main
Choose a base branch
from
+66 −59
Open

Clean up retry backoff per PR #222 review #223

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 66 additions & 59 deletions revup/github_real.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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."""
Expand All @@ -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)
Expand All @@ -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
Loading