From aa1583a111b9b3f0435db74ed2d2bdb5745e6c19 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 20 Aug 2026 16:47:24 -0400 Subject: [PATCH] Retry review comments with smaller pages GitHub can return an HTML 502 response for a review-thread query that succeeds when fewer threads are requested. The fixed page size makes the entire refresh fail, including pages already fetched. Retain the normal 20-thread page from e0e76278bc6. On HTTP 502, retry the same cursor with five threads and then one, retaining the smaller size for later pages. Stop reducing at one and leave other errors alone. Both current and legacy queries accept the page size, preserving the legacy pagination added by 552316684e6. Cover cursor retention, retry exhaustion, non-502 failures, and reduced page sizes through the legacy query. --- src/github/pullRequestModel.ts | 38 ++++++++++++------ src/github/queriesShared.gql | 8 ++-- src/test/github/pullRequestModel.test.ts | 49 +++++++++++++++++++++--- 3 files changed, 75 insertions(+), 20 deletions(-) diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 1c7ad1eaa8..f0021bde13 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -67,7 +67,7 @@ import { ReviewEventEnum, } from './interface'; import { IssueChangeEvent, IssueModel } from './issueModel'; -import { compareCommits, GraphQLError, GraphQLErrorType } from './loggingOctokit'; +import { compareCommits, getErrorCode, GraphQLError, GraphQLErrorType } from './loggingOctokit'; import { convertRESTPullRequestToRawPullRequest, convertRESTReviewEvent, @@ -1493,29 +1493,45 @@ export class PullRequestModel extends IssueModel implements IPullRe const { remote, query, schema } = await this.githubRepository.ensure(); let after: string | null = null; - let hasNextPage = false; + let pageSize = 20; const reviewThreads: ReviewThread[] = []; try { - do { + while (reviewThreads.length < 1000) { const variables = { owner: remote.owner, name: remote.repositoryName, number: this.number, + first: pageSize, after, }; - const { data } = await query({ - query: schema.PullRequestComments, - variables, - }, false, { query: schema.LegacyPullRequestComments, variables }); + let data: PullRequestCommentsResponse | null; + try { + ({ data } = await query({ + query: schema.PullRequestComments, + variables, + }, false, { query: schema.LegacyPullRequestComments, variables })); + } catch (e) { + if (getErrorCode(e) !== '502' || pageSize === 1) { + throw e; + } + // Large review-thread queries can fail with HTTP 502. + // Retry the same cursor with 5, then 1 thread, and keep that size. + pageSize = Math.max(1, Math.floor(pageSize / 4)); + Logger.warn(`Retrying review comments for PR #${this.number} with ${pageSize} threads per page after HTTP 502.`, PullRequestModel.ID); + continue; + } if (!data?.repository) { throw new Error('Review comments response did not include a repository.'); } - reviewThreads.push(...data.repository.pullRequest.reviewThreads.nodes); + const page = data.repository.pullRequest.reviewThreads; + reviewThreads.push(...page.nodes); - hasNextPage = data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage; - after = data.repository.pullRequest.reviewThreads.pageInfo.endCursor; - } while (hasNextPage && reviewThreads.length < 1000); + if (!page.pageInfo.hasNextPage) { + break; + } + after = page.pageInfo.endCursor; + } Logger.debug(`Fetching review comments for PR #${this.number} - exit`, PullRequestModel.ID); return reviewThreads; diff --git a/src/github/queriesShared.gql b/src/github/queriesShared.gql index 833bd796f6..661c66da0c 100644 --- a/src/github/queriesShared.gql +++ b/src/github/queriesShared.gql @@ -566,10 +566,10 @@ query GetPendingReviewId($pullRequestId: ID!, $author: String!) { } } -query PullRequestComments($owner: String!, $name: String!, $number: Int!, $after: String) { +query PullRequestComments($owner: String!, $name: String!, $number: Int!, $first: Int!, $after: String) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { - reviewThreads(first: 20, after: $after) { + reviewThreads(first: $first, after: $after) { nodes { id isResolved @@ -608,10 +608,10 @@ query PullRequestComments($owner: String!, $name: String!, $number: Int!, $after } } -query LegacyPullRequestComments($owner: String!, $name: String!, $number: Int!, $after: String) { +query LegacyPullRequestComments($owner: String!, $name: String!, $number: Int!, $first: Int!, $after: String) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { - reviewThreads(first: 20, after: $after) { + reviewThreads(first: $first, after: $after) { nodes { id isResolved diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 14d1c86ae1..5084d1b0ee 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -125,8 +125,11 @@ describe('PullRequestModel', function () { sinon.stub(repository, 'ensure').resolves(repository); graphql.query.onCall(0).rejects(new Error('Unsupported query')); graphql.query.onCall(1).resolves(page('1', 'first')); - graphql.query.onCall(2).rejects(new Error('Unsupported query')); - graphql.query.onCall(3).resolves(page('2', null)); + const gatewayError = Object.assign(new Error('Bad Gateway'), { networkError: { statusCode: 502 } }); + graphql.query.onCall(2).rejects(gatewayError); + graphql.query.onCall(3).rejects(gatewayError); + graphql.query.onCall(4).rejects(new Error('Unsupported query')); + graphql.query.onCall(5).resolves(page('2', null)); try { const pr = new PullRequestBuilder().build(); @@ -134,12 +137,12 @@ describe('PullRequestModel', function () { const threads = await model.getReviewThreads(); assert.deepStrictEqual(threads.map(thread => thread.id), ['1', '2']); - assert.strictEqual(graphql.query.callCount, 4); - for (const [call, after] of [[graphql.query.secondCall, null], [graphql.query.lastCall, 'first']] as const) { + assert.strictEqual(graphql.query.callCount, 6); + for (const [call, after, first] of [[graphql.query.secondCall, null, 20], [graphql.query.lastCall, 'first', 5]] as const) { const [fallback] = call.args; assert.strictEqual(fallback.query, repository.schema.LegacyPullRequestComments); assert.deepStrictEqual(fallback.variables, { - owner: remote.owner, name: remote.repositoryName, number: pr.number, after, + owner: remote.owner, name: remote.repositoryName, number: pr.number, first, after, }); } } finally { @@ -147,6 +150,42 @@ describe('PullRequestModel', function () { } }); + it('retries gateway failures with smaller pages without losing the cursor', async function () { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const gatewayError = Object.assign(new Error('Bad Gateway'), { networkError: { statusCode: 502 } }); + const query = sinon.stub(repo, 'query'); + query.onCall(0).resolves(page('1', 'first')); + query.onCall(1).rejects(gatewayError); + query.onCall(2).rejects(gatewayError); + query.onCall(3).resolves(page('2', 'second')); + query.onCall(4).resolves(page('3', null)); + + const threads = await model.getReviewThreads(); + + assert.deepStrictEqual(threads.map(thread => thread.id), ['1', '2', '3']); + assert.deepStrictEqual(query.getCalls().map(call => call.args[0].variables), [ + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 20, after: null }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 20, after: 'first' }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 5, after: 'first' }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 1, after: 'first' }, + { owner: remote.owner, name: remote.repositoryName, number: pr.number, first: 1, after: 'second' }, + ]); + }); + + for (const [statusCode, pageSizes] of [[502, [20, 5, 1]], [403, [20]]] as const) { + it(`stops retrying review comments after HTTP ${statusCode}`, async function () { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const query = sinon.stub(repo, 'query').rejects(Object.assign(new Error('Request failed'), { + networkError: { statusCode }, + })); + + assert.deepStrictEqual(await model.getReviewThreads(), []); + assert.deepStrictEqual(query.getCalls().map(call => call.args[0].variables?.first), [...pageSizes]); + }); + } + it('reports missing review data without retrying', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo));