github: Make addComment idempotent to prevent duplicate PR comments by matias-christensen-skydio · Pull Request #225 · Skydio/revup

matias-christensen-skydio · 2026-03-27T14:27:51Z

Summary

Fixes a bug where retrying a GraphQL mutation after a transient error (5xx or RESOURCE_LIMITS_EXCEEDED) would re-post addComment mutations that the server had already applied, producing duplicate stack outline and diff table comments on PRs.
Moves the retry loop from the generic graphql() method into update_pull_requests() where it has context to handle idempotency. Between retry attempts, re-queries each PR's comments and converts any already-posted new comments to updateIssueComment (edit) operations.
Retry coverage for all transient GitHub API failures is preserved.
Batches GraphQL requests to avoid RESOURCE_LIMITS_EXCEEDED on large stacks. Splits query_everything, create_pull_requests, and update_pull_requests into chunks of --github-batch-size PRs (default 5) per request.
Removes RETRYABLE_GRAPHQL_ERRORS since RESOURCE_LIMITS_EXCEEDED is a deterministic complexity rejection, not a transient error worth retrying.

jerry-skydio · 2026-04-17T20:54:35Z

-            raise e
+        except RevupGithubException as e:
+            if "timeout" in e.message:
+                logging.warning(


is this warning still relevant given the updated handling?

jerry-skydio · 2026-04-17T20:58:07Z

+
+        # Before retrying, check which new comments were already posted
+        # and update their IDs so the next attempt edits instead of adds.
+        await _refresh_new_comment_ids(github_ep, prs)


can you asyncio.gather this? i'm assuming delay is the delay between mutations and not queries, so we can subtract the amount of time this query takes

jerry-skydio · 2026-04-17T21:00:05Z

are RevupRequestException and RevupGithubException actually different? it seems like they should be combined (with any additional context being put into object members)

jerry-skydio · 2026-04-17T21:00:17Z

oh also you need to rebase

matias-christensen-skydio · 2026-04-20T11:50:41Z

These two exceptions carry genuinely different data and are handled differently, so I'd prefer to keep them separate:

RevupRequestException represents an HTTP-layer failure (non-200 response). Fields: status, response. Raised before any GraphQL parsing happens.
RevupGithubException represents a GraphQL-level error on a 200 response. Fields: types, message, error_json. Raised after parsing the GraphQL error envelope.

They also map to different exit codes in __main__.py (5 vs 6). A merge would replace clean except dispatch with attribute-None checks. Happy to revisit if you see a cleaner unification.

matias-christensen-skydio · 2026-04-21T11:21:18Z

@jerry-skydio Note that I pushed another commit to this PR that fixes the issue with RESOURCE_LIMITS_EXCEEDED. Turns out that is not a retryable error, and actually means that your GraphQL query is too complex. So this breaks that up into multiple smaller queries to stay under GitHub's limit.

jerry-skydio · 2026-04-22T00:09:27Z

        oauth_token: str,
        github_url: str,
        proxy: Optional[str] = None,
+        batch_size: int = 5,


how did you determine the batch size? is this fixed or variable?

This was empirically chosen. As far as I can tell, it isn't very well documented how the "limit" of a GH GraphQL query is chosen and how we can predict if the current query will exceed it and cause a RESOURCE_LIMITS_EXCEEDED. So I tested a few different values and 5 PRs per batch seems to avoid issues in my current stack of about 20 PRs.

Pulled out the magic number to a constant and added a comment explaining where this number comes from.

can we do it more dynamically? ie attempt with all at once, if that fails split in half into 2 batches, etc exponentially

i'm somewhat wary of hardcoding something considering github seems to be all over the place and it could get stricter or looser

Yeah. I have actually hit the RESOURCE_LIMITS_EXCEEDED again with this code after posting this. Will investigate this a bit more and see if it is possible to do this in a smarter way.

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.

…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.

jerry-skydio · 2026-06-04T19:00:46Z

btw i've refactored a large amount of code here and reapplied this change here #254

matias-christensen-skydio · 2026-06-05T06:05:15Z

btw i've refactored a large amount of code here and reapplied this change here #254

So is this PR no longer needed? Should I just close it?

matias-christensen-skydio requested review from aaron-skydio and jerry-skydio March 27, 2026 14:28

matias-christensen-skydio force-pushed the fix-duplicate-comments branch 2 times, most recently from c9bd434 to da62129 Compare March 27, 2026 14:32

aaron-skydio assigned jerry-skydio Mar 27, 2026

jerry-skydio reviewed Apr 17, 2026

View reviewed changes

matias-christensen-skydio force-pushed the fix-duplicate-comments branch from da62129 to 5803d5a Compare April 20, 2026 11:50

matias-christensen-skydio requested a review from jerry-skydio April 20, 2026 11:50

matias-christensen-skydio force-pushed the fix-duplicate-comments branch from 5803d5a to e53869f Compare April 20, 2026 11:52

matias-christensen-skydio force-pushed the fix-duplicate-comments branch 2 times, most recently from 5d1e718 to d8e93b3 Compare April 21, 2026 11:25

jerry-skydio reviewed Apr 22, 2026

View reviewed changes

matias-christensen-skydio force-pushed the fix-duplicate-comments branch 2 times, most recently from 0737ccb to 8389e82 Compare April 22, 2026 09:04

matias-christensen-skydio requested a review from jerry-skydio April 22, 2026 09:05

github: Make addComment idempotent to prevent duplicate PR comments

883badd

matias-christensen-skydio force-pushed the fix-duplicate-comments branch from 8389e82 to 85a3a5c Compare June 4, 2026 14:47

matias-christensen-skydio added 2 commits June 4, 2026 16:52

matias-christensen-skydio force-pushed the fix-duplicate-comments branch from 85a3a5c to 97c4e05 Compare June 4, 2026 14:53

Conversation

matias-christensen-skydio commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerry-skydio commented Apr 17, 2026

Uh oh!

jerry-skydio commented Apr 17, 2026

Uh oh!

matias-christensen-skydio commented Apr 20, 2026

Uh oh!

matias-christensen-skydio commented Apr 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerry-skydio commented Jun 4, 2026

Uh oh!

matias-christensen-skydio commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matias-christensen-skydio commented Mar 27, 2026 •

edited

Loading