From 883badd957ccfc8b53d460d7eb3109532e1943b1 Mon Sep 17 00:00:00 2001 From: Matias Varnum Date: Thu, 26 Mar 2026 16:35:56 +0100 Subject: [PATCH 1/3] github: Make addComment idempotent to prevent duplicate PR comments --- revup/github_utils.py | 241 +++++++++++++++++++++++++++++------------- 1 file changed, 169 insertions(+), 72 deletions(-) diff --git a/revup/github_utils.py b/revup/github_utils.py index 33b976c..0b885e3 100644 --- a/revup/github_utils.py +++ b/revup/github_utils.py @@ -1,4 +1,5 @@ import argparse +import asyncio import logging import os import re @@ -18,7 +19,12 @@ from revup import config, git, github, logs from revup.git import GitHubRepoInfo -from revup.types import GitCommitHash, RevupGithubException, RevupUsageException +from revup.types import ( + GitCommitHash, + RevupGithubException, + RevupRequestException, + RevupUsageException, +) MAX_COMMENTS_TO_QUERY = 3 @@ -552,18 +558,59 @@ async def create_pull_requests( pr.url = result["url"] +TRANSIENT_STATUSES = frozenset({500, 502, 503, 504}) +RETRYABLE_GRAPHQL_ERRORS = frozenset({"RESOURCE_LIMITS_EXCEEDED"}) + + +async def _refresh_new_comment_ids(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: + """Re-query comments for PRs with new (id=None) comments. + + If a comment with matching body text already exists on the PR, set its + id in-place so the next mutation attempt uses updateIssueComment instead + of addComment, avoiding duplicates. + """ + prs_with_new = [pr for pr in prs if any(c.id is None for c in pr.comments)] + if not prs_with_new: + return + + node_args = get_args_dict([pr.id for pr in prs_with_new], "node") + node_outs = get_result_args(len(prs_with_new), "node_out") + arg_str = ", ".join(get_args_declaration(node_args, "ID!")) + + query_str = "".join( + len(prs_with_new) + * [ + "{}: node(id: ${}) {{ ... on PullRequest {{ comments(first: " + + str(MAX_COMMENTS_TO_QUERY) + + ") {{ nodes {{ body id }} }} }} }}," + ] + ) + query_str = query_str.format(*zip_and_flatten(node_outs, node_args.keys())) + query = f"query ({arg_str}) {{ {query_str} }}" + + result = await github_ep.graphql(query, max_retries=1, **node_args) + + for pr, out in zip(prs_with_new, node_outs): + pr_data = result["data"][out] + existing = pr_data.get("comments", {}).get("nodes", []) if pr_data else [] + existing_by_body = {c["body"]: c["id"] for c in existing} + for comment in pr.comments: + if comment.id is None and comment.text in existing_by_body: + comment.id = existing_by_body[comment.text] + logging.info("Comment already posted on PR, converting to edit") + + async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: """ Update the given pull request contents, and also add reviewers and labels. """ + # Build non-comment parts once (all idempotent, safe to retry as-is). inputs = [] labels = [] reviewers = [] assignees = [] convert_to_draft = [] convert_from_draft = [] - comments = [] - edit_comments = [] for pr in prs: update_dict = { "clientMutationId": "revup", @@ -611,20 +658,6 @@ async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpd "pullRequestId": pr.id, }) - for c in pr.comments: - if c.id: - edit_comments.append({ - "body": c.text, - "clientMutationId": "revup", - "id": c.id, - }) - else: - comments.append({ - "body": c.text, - "clientMutationId": "revup", - "subjectId": pr.id, - }) - inputs_args = get_args_dict(inputs, "pr") prs_out = get_result_args(len(inputs), "pr_out") @@ -643,23 +676,6 @@ async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpd from_draft_args = get_args_dict(convert_from_draft, "from_d") from_draft_out = get_result_args(len(convert_from_draft), "from_d_out") - comments_args = get_args_dict(comments, "com") - comments_out = get_result_args(len(comments), "com_out") - - edit_comments_args = get_args_dict(edit_comments, "edit_com") - edit_comments_out = get_result_args(len(edit_comments), "edit_com_out") - - arg_str = ", ".join( - get_args_declaration(inputs_args, "UpdatePullRequestInput!") - + get_args_declaration(labels_args, "AddLabelsToLabelableInput!") - + get_args_declaration(reviewers_args, "RequestReviewsInput!") - + get_args_declaration(assignees_args, "AddAssigneesToAssignableInput!") - + get_args_declaration(to_draft_args, "ConvertPullRequestToDraftInput!") - + get_args_declaration(from_draft_args, "MarkPullRequestReadyForReviewInput!") - + get_args_declaration(comments_args, "AddCommentInput!") - + get_args_declaration(edit_comments_args, "UpdateIssueCommentInput!") - ) - update_str = "".join( len(inputs) * [ @@ -727,57 +743,138 @@ async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpd ) from_draft_str = from_draft_str.format(*zip_and_flatten(from_draft_out, from_draft_args.keys())) - add_comments_str = "".join( - len(comments_args) - * [ - """ + idempotent_str = ( + f"{update_str}{request_reviewers_str}{assignees_str}{add_labels_str}" + f"{to_draft_str}{from_draft_str}" + ) + idempotent_decl = ( + get_args_declaration(inputs_args, "UpdatePullRequestInput!") + + get_args_declaration(labels_args, "AddLabelsToLabelableInput!") + + get_args_declaration(reviewers_args, "RequestReviewsInput!") + + get_args_declaration(assignees_args, "AddAssigneesToAssignableInput!") + + get_args_declaration(to_draft_args, "ConvertPullRequestToDraftInput!") + + get_args_declaration(from_draft_args, "MarkPullRequestReadyForReviewInput!") + ) + idempotent_kwargs = { + **inputs_args, + **reviewers_args, + **assignees_args, + **labels_args, + **to_draft_args, + **from_draft_args, + } + + # Retry loop with idempotent addComment handling: between attempts, + # re-query PR comments so already-posted ones become edits, not adds. + max_retries = 3 + base_delay = 1.0 + for attempt in range(max_retries): + # Build comment parts from current pr.comments state. + # After _refresh_new_comment_ids, previously-new comments that were + # already posted will have their id set, routing them to + # updateIssueComment instead of addComment. + comments = [] + edit_comments = [] + for pr in prs: + for c in pr.comments: + if c.id: + edit_comments.append({ + "body": c.text, + "clientMutationId": "revup", + "id": c.id, + }) + else: + comments.append({ + "body": c.text, + "clientMutationId": "revup", + "subjectId": pr.id, + }) + + comments_args = get_args_dict(comments, "com") + comments_out = get_result_args(len(comments), "com_out") + + edit_comments_args = get_args_dict(edit_comments, "edit_com") + edit_comments_out = get_result_args(len(edit_comments), "edit_com_out") + + arg_str = ", ".join( + idempotent_decl + + get_args_declaration(comments_args, "AddCommentInput!") + + get_args_declaration(edit_comments_args, "UpdateIssueCommentInput!") + ) + + add_comments_str = "".join( + len(comments_args) + * [ + """ {}: addComment(input: ${}) {{ clientMutationId }},""" - ] - ) - add_comments_str = add_comments_str.format(*zip_and_flatten(comments_out, comments_args.keys())) + ] + ) + add_comments_str = add_comments_str.format( + *zip_and_flatten(comments_out, comments_args.keys()) + ) - edit_comments_str = "".join( - len(edit_comments_args) - * [ - """ + edit_comments_str = "".join( + len(edit_comments_args) + * [ + """ {}: updateIssueComment(input: ${}) {{ clientMutationId }},""" - ] - ) - edit_comments_str = edit_comments_str.format( - *zip_and_flatten(edit_comments_out, edit_comments_args.keys()) - ) + ] + ) + edit_comments_str = edit_comments_str.format( + *zip_and_flatten(edit_comments_out, edit_comments_args.keys()) + ) - # Have any add comment mutations first in order to ensure that comments are at the top of the PR - mutation_str = f""" + # addComment first so new comments appear at the top of the PR + mutation_str = f""" mutation ({arg_str}) {{ - {add_comments_str}{update_str}{request_reviewers_str}{assignees_str}{add_labels_str}\ -{to_draft_str}{from_draft_str}{edit_comments_str} + {add_comments_str}{idempotent_str}{edit_comments_str} }}""" - try: - await github_ep.graphql( - mutation_str, - **comments_args, - **inputs_args, - **reviewers_args, - **assignees_args, - **labels_args, - **to_draft_args, - **from_draft_args, - **edit_comments_args, - ) - except RevupGithubException as e: - if "timeout" in e.message: + try: + await github_ep.graphql( + mutation_str, + max_retries=1, + **comments_args, + **idempotent_kwargs, + **edit_comments_args, + ) + return + except RevupRequestException as e: + if e.status not in TRANSIENT_STATUSES or attempt >= max_retries - 1: + raise + delay = base_delay * (2**attempt) logging.warning( - "Github update request timed out! Most likely this is a false alarm and changes" - " actually succeeded. You may want to rerun this command to verify." + "GitHub returned %d, retrying in %ss (attempt %d/%d)", + e.status, + delay, + attempt + 1, + max_retries, ) - else: - raise e + except RevupGithubException as e: + retryable = set(e.types) & RETRYABLE_GRAPHQL_ERRORS + is_timeout = "timeout" in e.message + if not (retryable or is_timeout) or attempt >= max_retries - 1: + raise + delay = base_delay * (2**attempt) + reason = ", ".join(retryable) if retryable else "timeout" + logging.warning( + "GitHub GraphQL error (%s), retrying in %ss (attempt %d/%d)", + reason, + delay, + attempt + 1, + max_retries, + ) + + # Before retrying, check which new comments were already posted + # and update their IDs so the next attempt edits instead of adds. + await asyncio.gather( + _refresh_new_comment_ids(github_ep, prs), + asyncio.sleep(delay), + ) RE_PR_URL = re.compile( From c248dc32d35fa32da728b6b86b9ac13ed7bf8a19 Mon Sep 17 00:00:00 2001 From: Matias Varnum Date: Tue, 21 Apr 2026 13:19:26 +0200 Subject: [PATCH 2/3] github: Batch GraphQL requests to avoid RESOURCE_LIMITS_EXCEEDED Large stacks exceed GitHub's GraphQL complexity budget when all operations are packed into a single request. Split query_everything, create_pull_requests, and update_pull_requests into batches of --github-batch-size PRs (default 5) per request. Also remove RETRYABLE_GRAPHQL_ERRORS since RESOURCE_LIMITS_EXCEEDED is a deterministic complexity rejection, not a transient error. --- revup/github.py | 9 ++ revup/github_real.py | 13 +- revup/github_utils.py | 296 +++++++++++++++++++++++++++--------------- revup/revup.py | 2 + 4 files changed, 206 insertions(+), 114 deletions(-) diff --git a/revup/github.py b/revup/github.py index 15d2dcc..7044ff3 100644 --- a/revup/github.py +++ b/revup/github.py @@ -1,8 +1,17 @@ from abc import ABCMeta, abstractmethod from typing import Any +# Number of PRs to pack into a single batched GraphQL request. GitHub's documented +# 500k-node cap isn't what we hit in practice; their undocumented "other resource +# limits" threshold is tighter and has no published number. 5 was chosen empirically +# by finding a value that works for a real-world 19-PR stack where update mutations +# (up to 8 sub-mutations per PR) are the tightest bottleneck. +DEFAULT_BATCH_SIZE = 5 + class GitHubEndpoint(metaclass=ABCMeta): + batch_size: int + @abstractmethod async def graphql(self, query: str, **kwargs: Any) -> Any: """ diff --git a/revup/github_real.py b/revup/github_real.py index f6989c9..1b91f84 100644 --- a/revup/github_real.py +++ b/revup/github_real.py @@ -11,7 +11,6 @@ from revup.types import RevupGithubException, RevupRequestException TRANSIENT_STATUSES = frozenset({500, 502, 503, 504}) -RETRYABLE_GRAPHQL_ERRORS = frozenset({"RESOURCE_LIMITS_EXCEEDED"}) class RealGitHubEndpoint(github.GitHubEndpoint): @@ -47,10 +46,12 @@ def __init__( oauth_token: str, github_url: str, proxy: Optional[str] = None, + batch_size: int = github.DEFAULT_BATCH_SIZE, ): self.github_url = github_url self.oauth_token = oauth_token self.proxy = proxy + self.batch_size = batch_size self.graphql_endpoint = f"https://api.{github_url}/graphql" async def close(self) -> None: @@ -139,13 +140,3 @@ async def graphql( 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 diff --git a/revup/github_utils.py b/revup/github_utils.py index 0b885e3..92836cf 100644 --- a/revup/github_utils.py +++ b/revup/github_utils.py @@ -122,16 +122,14 @@ def zip_and_flatten(l1: Iterable[str], l2: Iterable[str]) -> List[str]: return ret -async def query_everything( +async def _query_repo_users_labels( github_ep: github.GitHubEndpoint, repo_info: GitHubRepoInfo, - head_refs: List[str], user_ids: List[str], labels: List[str], teams: List[Tuple[str, str]], ) -> Tuple[ str, - List[Optional[PrInfo]], Dict[str, str], Dict[str, str], Dict[str, str], @@ -139,12 +137,12 @@ async def query_everything( Dict[str, Optional[Set[str]]], ]: """ - This function does all necessary graphql querying in one request. This dramatically reduces the - amount of time spent on querying. + Query the repository node id along with user, label, and team lookups in a single request. + + None of these scale with the number of PRs, so they're fetched once rather than per PR batch. Returns a tuple of: - Repository node id - - List of pull requests, one for each ref in head_refs. None if a pr wasn't found for that ref - Dict of user_ids as given to graphql node ids - Dict of user_ids as given to their full login name - Dict of labels to their graphql node ids @@ -152,39 +150,23 @@ async def query_everything( - Dict of "org/slug" team refs to their member logins. None if the team has more members than we fetched (meaning membership is unknown / incomplete). """ - head_refs_args = get_args_dict(head_refs, "pr") user_id_args = get_args_dict(user_ids, "user") label_args = get_args_dict(labels, "label") team_org_args = get_args_dict([t[0] for t in teams], "team_org") team_slug_args = get_args_dict([t[1] for t in teams], "team_slug") - prs_out = get_result_args(len(head_refs), "pr_out") user_id_out = get_result_args(len(user_ids), "user_out") label_out = get_result_args(len(labels), "label_out") team_out = get_result_args(len(teams), "team_out") arg_str = ", ".join( - get_args_declaration(head_refs_args, "String!") - + get_args_declaration(user_id_args, "String!") + get_args_declaration(user_id_args, "String!") + get_args_declaration(label_args, "String!") + get_args_declaration(team_org_args, "String!") + get_args_declaration(team_slug_args, "String!") ) - - # NOTE: There are possible limitations here because we depend on PRs being returned in order of - # OPEN prs, followed by MERGED prs in the order that they merged. github doesn't offer these - # options and it is excessively expensive to always fetch multiple prs and order them on this - # side. For now we hope that the most relevant PR will have the most recent update time. - request_str = "".join( - len(head_refs) - * [ - "{}: pullRequests (headRefName: ${}, states: [OPEN, MERGED], first: 1, " - "orderBy: {{direction: DESC, field:UPDATED_AT}}) {{" - "...PrResult" - "}}," - ] - ) - request_str = request_str.format(*zip_and_flatten(prs_out, head_refs_args.keys())) + if arg_str: + arg_str = ", " + arg_str user_str = "".join( len(user_ids) * ["{}: assignableUsers (query: ${}, first: 25) {{...UserResult}},"] @@ -202,16 +184,16 @@ async def query_everything( f"{{id, members(first: 100) {{nodes {{login}}, totalCount}}}}}}," ) - multi_query_str = f""" - query GetPrResults($owner: String!, $name: String!, {arg_str}) {{ + query_str = f""" + query ($owner: String!, $name: String!{arg_str}) {{ repository(name: $name, owner: $owner) {{ id - {request_str}{user_str}{label_str} + {user_str}{label_str} }} {team_str} }}""" if user_str: - multi_query_str += """ + query_str += """ fragment UserResult on UserConnection { nodes { login @@ -220,13 +202,110 @@ async def query_everything( totalCount }""" if label_str: - multi_query_str += """ + query_str += """ fragment LabelResult on Label { id name }""" - if request_str: - multi_query_str += f""" + + result = await github_ep.graphql( + query_str, + owner=repo_info.owner, + name=repo_info.name, + **user_id_args, + **label_args, + **team_org_args, + **team_slug_args, + ) + + repo_id = result["data"]["repository"]["id"] + + names_to_ids: Dict[str, str] = {} + names_to_logins: Dict[str, str] = {} + for i, user_id in enumerate(user_ids): + this_node = result["data"]["repository"][user_id_out[i]] + if len(this_node["nodes"]) == 0: + logging.warning("No matching user found for {}".format(user_id)) + else: + if this_node["totalCount"] > len(this_node["nodes"]): + logging.warning( + "Too many matching users found for {}, try being more specific".format(user_id) + ) + shortest_name = this_node["nodes"][0]["login"] + names_to_ids[user_id] = this_node["nodes"][0]["id"] + found_match = False + for user in this_node["nodes"]: + if len(user["login"]) <= len(shortest_name) and user["login"].startswith(user_id): + shortest_name = user["login"] + names_to_ids[user_id] = user["id"] + names_to_logins[user_id] = user["login"] + found_match = True + if not found_match: + logging.warning( + "Couldn't find a prefixed match for {}, going with {} instead".format( + user_id, shortest_name + ) + ) + + labels_to_ids: Dict[str, str] = {} + for i, label in enumerate(labels): + this_node = result["data"]["repository"][label_out[i]] + if this_node is not None: + labels_to_ids[label] = this_node["id"] + else: + logging.warning("Couldn't find an existing label named {}".format(label)) + + teams_to_ids: Dict[str, str] = {} + teams_to_members: Dict[str, Optional[Set[str]]] = {} + for i, (org, slug) in enumerate(teams): + team_node = result["data"][team_out[i]] + if team_node is not None and team_node["team"] is not None: + team_ref = f"{org}/{slug}" + teams_to_ids[team_ref] = team_node["team"]["id"] + members_node = team_node["team"]["members"] + member_logins = {m["login"] for m in members_node["nodes"]} + if members_node["totalCount"] > len(members_node["nodes"]): + # Team has more members than we fetched; we can't check membership reliably. + teams_to_members[team_ref] = None + else: + teams_to_members[team_ref] = member_logins + else: + logging.warning("Couldn't find a team matching {}/{}".format(org, slug)) + + return repo_id, names_to_ids, names_to_logins, labels_to_ids, teams_to_ids, teams_to_members + + +async def _query_prs_batch( + github_ep: github.GitHubEndpoint, + repo_info: GitHubRepoInfo, + head_refs: List[str], +) -> List[Optional[PrInfo]]: + head_refs_args = get_args_dict(head_refs, "pr") + prs_out = get_result_args(len(head_refs), "pr_out") + + arg_str = ", ".join(get_args_declaration(head_refs_args, "String!")) + + # NOTE: There are possible limitations here because we depend on PRs being returned in order of + # OPEN prs, followed by MERGED prs in the order that they merged. github doesn't offer these + # options and it is excessively expensive to always fetch multiple prs and order them on this + # side. For now we hope that the most relevant PR will have the most recent update time. + request_str = "".join( + len(head_refs) + * [ + "{}: pullRequests (headRefName: ${}, states: [OPEN, MERGED], first: 1, " + "orderBy: {{direction: DESC, field:UPDATED_AT}}) {{" + "...PrResult" + "}}," + ] + ) + request_str = request_str.format(*zip_and_flatten(prs_out, head_refs_args.keys())) + + query_str = f""" + query ($owner: String!, $name: String!, {arg_str}) {{ + repository(name: $name, owner: $owner) {{ + {request_str} + }} + }} fragment PrResult on PullRequestConnection {{ nodes {{ id @@ -330,14 +409,10 @@ async def query_everything( }}""" pr_result = await github_ep.graphql( - multi_query_str, + query_str, owner=repo_info.owner, name=repo_info.name, **head_refs_args, - **user_id_args, - **label_args, - **team_org_args, - **team_slug_args, ) prs: List[Optional[PrInfo]] = [] @@ -438,60 +513,55 @@ async def query_everything( else: prs.append(None) - names_to_ids = {} - names_to_logins = {} - for i, user_id in enumerate(user_ids): - this_node = pr_result["data"]["repository"][user_id_out[i]] - if len(this_node["nodes"]) == 0: - logging.warning("No matching user found for {}".format(user_id)) - else: - if this_node["totalCount"] > len(this_node["nodes"]): - logging.warning( - "Too many matching users found for {}, try being more specific".format(user_id) - ) - shortest_name = this_node["nodes"][0]["login"] - names_to_ids[user_id] = this_node["nodes"][0]["id"] - found_match = False - for user in this_node["nodes"]: - if len(user["login"]) <= len(shortest_name) and user["login"].startswith(user_id): - shortest_name = user["login"] - names_to_ids[user_id] = user["id"] - names_to_logins[user_id] = user["login"] - found_match = True - if not found_match: - logging.warning( - "Couldn't find a prefixed match for {}, going with {} instead".format( - user_id, shortest_name - ) - ) + return prs - labels_to_ids = {} - for i, label in enumerate(labels): - this_node = pr_result["data"]["repository"][label_out[i]] - if this_node is not None: - labels_to_ids[label] = this_node["id"] - else: - logging.warning("Couldn't find an existing label named {}".format(label)) - teams_to_ids = {} - teams_to_members: Dict[str, Optional[Set[str]]] = {} - for i, (org, slug) in enumerate(teams): - team_node = pr_result["data"][team_out[i]] - if team_node is not None and team_node["team"] is not None: - team_ref = f"{org}/{slug}" - teams_to_ids[team_ref] = team_node["team"]["id"] - members_node = team_node["team"]["members"] - member_logins = {m["login"] for m in members_node["nodes"]} - if members_node["totalCount"] > len(members_node["nodes"]): - # Team has more members than we fetched; we can't check membership reliably. - teams_to_members[team_ref] = None - else: - teams_to_members[team_ref] = member_logins - else: - logging.warning("Couldn't find a team matching {}/{}".format(org, slug)) +async def query_everything( + github_ep: github.GitHubEndpoint, + repo_info: GitHubRepoInfo, + head_refs: List[str], + user_ids: List[str], + labels: List[str], + teams: List[Tuple[str, str]], +) -> Tuple[ + str, + List[Optional[PrInfo]], + Dict[str, str], + Dict[str, str], + Dict[str, str], + Dict[str, str], + Dict[str, Optional[Set[str]]], +]: + """ + Query all necessary data from GitHub, batching PR lookups to stay within + GitHub's GraphQL resource limits. + + Returns a tuple of: + - Repository node id + - List of pull requests, one for each ref in head_refs. None if a pr wasn't found for that ref + - Dict of user_ids as given to graphql node ids + - Dict of user_ids as given to their full login name + - Dict of labels to their graphql node ids + - Dict of "org/slug" team refs to graphql node ids + - Dict of "org/slug" team refs to their member logins. None if the team has more members + than we fetched (meaning membership is unknown / incomplete). + """ + ( + repo_id, + names_to_ids, + names_to_logins, + labels_to_ids, + teams_to_ids, + teams_to_members, + ) = await _query_repo_users_labels(github_ep, repo_info, user_ids, labels, teams) + + batch_size = github_ep.batch_size + prs: List[Optional[PrInfo]] = [] + for i in range(0, len(head_refs), batch_size): + prs.extend(await _query_prs_batch(github_ep, repo_info, head_refs[i : i + batch_size])) return ( - pr_result["data"]["repository"]["id"], + repo_id, prs, names_to_ids, names_to_logins, @@ -501,16 +571,13 @@ async def query_everything( ) -async def create_pull_requests( +async def _create_pull_requests_batch( github_ep: github.GitHubEndpoint, repo_id: str, repo_info: GitHubRepoInfo, fork_info: GitHubRepoInfo, prs: List[PrInfo], ) -> None: - """ - Create all pull requests given in prs and modify them to add the new pr node id and URL. - """ inputs = [] for pr in prs: headRef = ( @@ -558,8 +625,24 @@ async def create_pull_requests( pr.url = result["url"] +async def create_pull_requests( + github_ep: github.GitHubEndpoint, + repo_id: str, + repo_info: GitHubRepoInfo, + fork_info: GitHubRepoInfo, + prs: List[PrInfo], +) -> None: + """ + Create all pull requests given in prs and modify them to add the new pr node id and URL. + """ + batch_size = github_ep.batch_size + for i in range(0, len(prs), batch_size): + await _create_pull_requests_batch( + github_ep, repo_id, repo_info, fork_info, prs[i : i + batch_size] + ) + + TRANSIENT_STATUSES = frozenset({500, 502, 503, 504}) -RETRYABLE_GRAPHQL_ERRORS = frozenset({"RESOURCE_LIMITS_EXCEEDED"}) async def _refresh_new_comment_ids(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: @@ -600,10 +683,9 @@ async def _refresh_new_comment_ids(github_ep: github.GitHubEndpoint, prs: List[P logging.info("Comment already posted on PR, converting to edit") -async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: - """ - Update the given pull request contents, and also add reviewers and labels. - """ +async def _update_pull_requests_batch( + github_ep: github.GitHubEndpoint, prs: List[PrUpdate] +) -> None: # Build non-comment parts once (all idempotent, safe to retry as-is). inputs = [] labels = [] @@ -855,15 +937,11 @@ async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpd max_retries, ) except RevupGithubException as e: - retryable = set(e.types) & RETRYABLE_GRAPHQL_ERRORS - is_timeout = "timeout" in e.message - if not (retryable or is_timeout) or attempt >= max_retries - 1: + if "timeout" not in e.message or attempt >= max_retries - 1: raise delay = base_delay * (2**attempt) - reason = ", ".join(retryable) if retryable else "timeout" logging.warning( - "GitHub GraphQL error (%s), retrying in %ss (attempt %d/%d)", - reason, + "GitHub GraphQL error (timeout), retrying in %ss (attempt %d/%d)", delay, attempt + 1, max_retries, @@ -877,6 +955,15 @@ async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpd ) +async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: + """ + Update the given pull request contents, and also add reviewers and labels. + """ + batch_size = github_ep.batch_size + for i in range(0, len(prs), batch_size): + await _update_pull_requests_batch(github_ep, prs[i : i + batch_size]) + + RE_PR_URL = re.compile( r"^https://(?P[^/]+)/(?P[^/]+)/(?P[^/]+)/pull/(?P[0-9]+)/?$" ) @@ -973,7 +1060,10 @@ async def github_connection( ) github_ep = github_real.RealGitHubEndpoint( - oauth_token=args.github_oauth, proxy=args.proxy, github_url=args.github_url + oauth_token=args.github_oauth, + proxy=args.proxy, + github_url=args.github_url, + batch_size=args.github_batch_size, ) try: yield github_ep, repo_info, fork_info diff --git a/revup/revup.py b/revup/revup.py index 21c5bf3..8d0a00a 100755 --- a/revup/revup.py +++ b/revup/revup.py @@ -18,6 +18,7 @@ topic_completer, ) from revup.config import RevupArgParser +from revup.github import DEFAULT_BATCH_SIZE from revup.topic_stack import PrBodySource from revup.types import RevupUsageException @@ -53,6 +54,7 @@ def make_toplevel_parser() -> RevupArgParser: revup_parser.add_argument("--github-oauth") revup_parser.add_argument("--github-username") revup_parser.add_argument("--github-url", default="github.com") + revup_parser.add_argument("--github-batch-size", type=int, default=DEFAULT_BATCH_SIZE) revup_parser.add_argument("--remote-name", default="origin") revup_parser.add_argument("--fork-name", default="") revup_parser.add_argument("--editor") From 97c4e05cf0ce5fe128f7a3420204d6dbc377d041 Mon Sep 17 00:00:00 2001 From: Matias Varnum Date: Thu, 4 Jun 2026 15:07:16 +0200 Subject: [PATCH 3/3] github: Split GraphQL batches on RESOURCE_LIMITS_EXCEEDED instead of failing Batching by PR count (--github-batch-size) does not bound the quantity that actually matters: the server-side work a single request triggers. A batch of 5 PRs expands to a variable number of sub-mutations depending on how many are new (each new PR adds two large addComment operations for the review-graph and patchsets comments) plus per-PR reviewer and label mutations. A real 10-PR aircam stack produced a 19-sub-mutation request that GitHub rejected with RESOURCE_LIMITS_EXCEEDED, while the batch right before it (10 sub-mutations) succeeded. We cannot precompute whether a request will fit. There are two distinct limits: - The documented 500k node limit is static and computable from the first/last literals in the query, and we are nowhere near it (a 5-PR query is ~500 nodes). This is not what we hit. - RESOURCE_LIMITS_EXCEEDED is a runtime resource budget. GitHub publishes no formula and no threshold for it; the docs only describe the behavior ("if a query consumes too many resources" it is terminated with partial results). The true cost depends on server-side factors we can't see: notifications and webhooks fired per addComment, repo size, index state, and current load. So there is no value of --github-batch-size, and no client-side estimate, that reliably stays under it. Since the limit can't be predicted, react to it instead. On RESOURCE_LIMITS_EXCEEDED, halve the batch and retry until it fits (or a single PR/ref is reached, which we then surface). This discovers the limit at runtime and backs off, rather than guessing a constant that drifts with stack shape and server load. The error is a *partial* success: GitHub applies some sub-mutations (including addComments) before running out of budget. Resending the same request as-is would re-post those comments, so _update_with_splitting first runs _refresh_new_comment_ids to convert already-posted comments into edits, reusing the idempotency machinery from e53869f, then splits and retries. The read-only query path (_query_prs_with_splitting) needs no such guard. --- revup/github_utils.py | 68 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/revup/github_utils.py b/revup/github_utils.py index 92836cf..be31749 100644 --- a/revup/github_utils.py +++ b/revup/github_utils.py @@ -28,6 +28,15 @@ MAX_COMMENTS_TO_QUERY = 3 +# GitHub returns this GraphQL error type when a single request consumes too many +# server-side resources. Unlike the documented 500k node limit (a static up-front +# rejection), this is a runtime budget hit partway through execution: the response +# contains partial data for the sub-operations that did run plus this error for the +# ones that didn't. There is no published formula or threshold, so we can't predict +# it; instead we send a batch, and if we get this error we split the batch in half +# and retry until it fits. +RESOURCE_LIMITS_EXCEEDED = "RESOURCE_LIMITS_EXCEEDED" + @dataclass class PrComment: @@ -516,6 +525,32 @@ async def _query_prs_batch( return prs +async def _query_prs_with_splitting( + github_ep: github.GitHubEndpoint, + repo_info: GitHubRepoInfo, + head_refs: List[str], +) -> List[Optional[PrInfo]]: + """ + Query PRs for head_refs, halving the batch and retrying on RESOURCE_LIMITS_EXCEEDED. + + The query path is read-only, so splitting and concatenating results is always safe. + Raises if a single ref still exceeds the limit (can't split further). + """ + try: + return await _query_prs_batch(github_ep, repo_info, head_refs) + except RevupGithubException as e: + if RESOURCE_LIMITS_EXCEEDED not in e.types or len(head_refs) <= 1: + raise + logging.warning( + "GitHub GraphQL RESOURCE_LIMITS_EXCEEDED querying %d PRs, splitting batch in half", + len(head_refs), + ) + mid = len(head_refs) // 2 + prs = await _query_prs_with_splitting(github_ep, repo_info, head_refs[:mid]) + prs.extend(await _query_prs_with_splitting(github_ep, repo_info, head_refs[mid:])) + return prs + + async def query_everything( github_ep: github.GitHubEndpoint, repo_info: GitHubRepoInfo, @@ -558,7 +593,9 @@ async def query_everything( batch_size = github_ep.batch_size prs: List[Optional[PrInfo]] = [] for i in range(0, len(head_refs), batch_size): - prs.extend(await _query_prs_batch(github_ep, repo_info, head_refs[i : i + batch_size])) + prs.extend( + await _query_prs_with_splitting(github_ep, repo_info, head_refs[i : i + batch_size]) + ) return ( repo_id, @@ -955,13 +992,40 @@ async def _update_pull_requests_batch( ) +async def _update_with_splitting(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: + """ + Update prs, halving the batch and retrying on RESOURCE_LIMITS_EXCEEDED. + + RESOURCE_LIMITS_EXCEEDED is a *partial* success: GitHub applies some sub-mutations + (including addComments) before running out of budget. Resending the same request as-is + would re-post those comments, so we first run _refresh_new_comment_ids to convert + already-posted comments into edits, then split the batch and retry the halves. + Raises if a single PR's update still exceeds the limit (can't split PRs further). + """ + try: + await _update_pull_requests_batch(github_ep, prs) + except RevupGithubException as e: + if RESOURCE_LIMITS_EXCEEDED not in e.types or len(prs) <= 1: + raise + logging.warning( + "GitHub GraphQL RESOURCE_LIMITS_EXCEEDED updating %d PRs, splitting batch in half", + len(prs), + ) + # Some sub-mutations already applied; convert posted comments to edits so the + # retry doesn't duplicate them. + await _refresh_new_comment_ids(github_ep, prs) + mid = len(prs) // 2 + await _update_with_splitting(github_ep, prs[:mid]) + await _update_with_splitting(github_ep, prs[mid:]) + + async def update_pull_requests(github_ep: github.GitHubEndpoint, prs: List[PrUpdate]) -> None: """ Update the given pull request contents, and also add reviewers and labels. """ batch_size = github_ep.batch_size for i in range(0, len(prs), batch_size): - await _update_pull_requests_batch(github_ep, prs[i : i + batch_size]) + await _update_with_splitting(github_ep, prs[i : i + batch_size]) RE_PR_URL = re.compile(