mirror of
https://github.com/renovatebot/renovate.git
synced 2026-10-24 22:48:06 +00:00
fix(github): treat graphql_rate_limit as rate limiting (#45396)
* fix(github): treat graphql_rate_limit as rate limiting GitHub answers a spent app-installation budget with a GraphQL error carrying code graphql_rate_limit, which neither call site recognised: the repository query reported platform-unknown-error, and app authentication reported Init: Authentication failure. Both readings send the user looking for a problem that is not there, and neither is retried the way rate limiting is. The two shapes are now recognised in one place and both call sites use it. * fix(github): propagate GraphQL rate limits * fix(github): preserve GraphQL response contract --------- Co-authored-by: Sergei Zharinov <zharinov@users.noreply.github.com>
This commit is contained in:
@@ -32,6 +32,15 @@ import * as github from './index.ts';
|
||||
import type { ApiPageCache, GhRestPr } from './types.ts';
|
||||
|
||||
const githubApiHost = 'https://api.github.com';
|
||||
const graphqlRateLimitResponse = {
|
||||
errors: [
|
||||
{
|
||||
type: 'RATE_LIMIT',
|
||||
code: 'graphql_rate_limit',
|
||||
message: 'API rate limit already exceeded for installation ID XXXXXXX.',
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
vi.mock('timers/promises');
|
||||
|
||||
@@ -520,6 +529,16 @@ describe('modules/platform/github/index', () => {
|
||||
).rejects.toThrowWithMessage(Error, 'Init: Authentication failure');
|
||||
});
|
||||
|
||||
it('should report a spent App budget as rate limiting, not as authentication failure', async () => {
|
||||
httpMock
|
||||
.scope(githubApiHost)
|
||||
.post('/graphql')
|
||||
.reply(200, graphqlRateLimitResponse);
|
||||
await expect(
|
||||
github.initPlatform({ token: 'x-access-token:ghs_123test' }),
|
||||
).rejects.toThrowWithMessage(Error, PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('should autodetect email/user on custom endpoint with GitHub App', async () => {
|
||||
httpMock
|
||||
.scope('https://ghe.renovatebot.com', {
|
||||
@@ -1170,6 +1189,16 @@ describe('modules/platform/github/index', () => {
|
||||
).rejects.toThrow(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('should abort when graphql_rate_limit is returned', async () => {
|
||||
httpMock
|
||||
.scope(githubApiHost)
|
||||
.post(`/graphql`)
|
||||
.reply(200, graphqlRateLimitResponse);
|
||||
await expect(
|
||||
github.initRepo({ repository: 'some/repo' }),
|
||||
).rejects.toThrow(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('should throw error if renamed', async () => {
|
||||
httpMock
|
||||
.scope(githubApiHost)
|
||||
@@ -2709,6 +2738,41 @@ describe('modules/platform/github/index', () => {
|
||||
});
|
||||
|
||||
describe('ensureIssue()', () => {
|
||||
it('propagates GraphQL rate limit errors', async () => {
|
||||
const scope = httpMock.scope(githubApiHost);
|
||||
initRepoMock(scope, 'some/repo');
|
||||
await github.initRepo({ repository: 'some/repo' });
|
||||
scope.post('/graphql').reply(200, graphqlRateLimitResponse);
|
||||
|
||||
await expect(
|
||||
github.ensureIssue({ title: 'new-title', body: 'new-content' }),
|
||||
).rejects.toThrow(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('handles repositories with issues disabled', async () => {
|
||||
const scope = httpMock.scope(githubApiHost);
|
||||
initRepoMock(scope, 'some/repo');
|
||||
await github.initRepo({ repository: 'some/repo' });
|
||||
scope
|
||||
.post('/graphql')
|
||||
.reply(200, {
|
||||
data: {
|
||||
repository: {
|
||||
issues: {
|
||||
pageInfo: { hasNextPage: false, endCursor: null },
|
||||
nodes: [],
|
||||
},
|
||||
},
|
||||
},
|
||||
})
|
||||
.post('/repos/some/repo/issues')
|
||||
.reply(410, { message: 'Issues are disabled for this repo' });
|
||||
|
||||
await expect(
|
||||
github.ensureIssue({ title: 'new-title', body: 'new-content' }),
|
||||
).resolves.toBeNull();
|
||||
});
|
||||
|
||||
it('creates issue', async () => {
|
||||
const scope = httpMock.scope(githubApiHost);
|
||||
initRepoMock(scope, 'some/repo');
|
||||
@@ -4230,6 +4294,15 @@ describe('modules/platform/github/index', () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it('should propagate GraphQL rate limit errors', async () => {
|
||||
const scope = await mockScope();
|
||||
scope.post('/graphql').reply(200, graphqlRateLimitResponse);
|
||||
|
||||
await expect(github.createPr(prConfig)).rejects.toThrow(
|
||||
PLATFORM_RATE_LIMIT_EXCEEDED,
|
||||
);
|
||||
});
|
||||
|
||||
it('should pass commit message as commitHeadline and commitBody for squash merge', async () => {
|
||||
const scope = await mockScope();
|
||||
scope.post('/graphql').reply(200, graphqlAutomergeResp);
|
||||
@@ -4832,6 +4905,17 @@ describe('modules/platform/github/index', () => {
|
||||
).rejects.toThrow(PR_ALREADY_IN_MERGE_QUEUE);
|
||||
});
|
||||
|
||||
it('propagates rate limits from the base branch check', async () => {
|
||||
const scope = httpMock.scope(githubApiHost);
|
||||
initRepoMock(scope, 'some/repo');
|
||||
await github.initRepo({ repository: 'some/repo' });
|
||||
scope.post('/graphql').reply(200, graphqlRateLimitResponse);
|
||||
|
||||
await expect(
|
||||
github.assertPrNotInMergeQueue('somebranch', 'main'),
|
||||
).rejects.toThrow(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('logs if the merge queue check returns errors', async () => {
|
||||
const scope = httpMock.scope(githubApiHost);
|
||||
initRepoMock(scope, 'some/repo');
|
||||
@@ -4865,6 +4949,19 @@ describe('modules/platform/github/index', () => {
|
||||
).toResolve();
|
||||
});
|
||||
|
||||
it('propagates rate limits from the PR merge queue check', async () => {
|
||||
const scope = httpMock.scope(githubApiHost);
|
||||
initRepoMock(scope, 'some/repo');
|
||||
await github.initRepo({ repository: 'some/repo' });
|
||||
mergeQueueEnabledMock(scope);
|
||||
prListMock(scope);
|
||||
scope.post('/graphql').reply(200, graphqlRateLimitResponse);
|
||||
|
||||
await expect(
|
||||
github.assertPrNotInMergeQueue('somebranch', 'main'),
|
||||
).rejects.toThrow(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('skips merge queue check on GHE <3.12.0', async () => {
|
||||
const scope = httpMock
|
||||
.scope('https://github.company.com')
|
||||
@@ -5114,6 +5211,15 @@ describe('modules/platform/github/index', () => {
|
||||
'Error re-attempting PR platform automerge',
|
||||
);
|
||||
});
|
||||
|
||||
it('propagates GraphQL rate limit errors', async () => {
|
||||
const scope = await mockScope();
|
||||
scope.post('/graphql').reply(200, graphqlRateLimitResponse);
|
||||
|
||||
await expect(github.reattemptPlatformAutomerge(pr)).rejects.toThrow(
|
||||
PLATFORM_RATE_LIMIT_EXCEEDED,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('mergePr(prNo)', () => {
|
||||
|
||||
@@ -589,10 +589,6 @@ export async function initRepo({
|
||||
});
|
||||
|
||||
if (res?.errors) {
|
||||
if (res.errors.find((err) => err.type === 'RATE_LIMITED')) {
|
||||
logger.debug({ res }, 'GraphQL rate limit exceeded.');
|
||||
throw new Error(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
}
|
||||
logger.debug({ res }, 'Unexpected GraphQL errors');
|
||||
throw new Error(PLATFORM_UNKNOWN_ERROR);
|
||||
}
|
||||
@@ -1541,7 +1537,10 @@ export async function ensureIssue({
|
||||
// reset issueList so that it will be fetched again as-needed
|
||||
GithubIssueCache.updateIssue(createdIssue);
|
||||
return 'created';
|
||||
} catch (err) /* v8 ignore next -- issue creation failure handling is not mocked in specs */ {
|
||||
} catch (err) {
|
||||
if (err instanceof Error && err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
|
||||
throw err;
|
||||
}
|
||||
if (err.body?.message?.startsWith('Issues are disabled for this repo')) {
|
||||
logger.debug(`Issues are disabled, so could not create issue: ${title}`);
|
||||
} else {
|
||||
@@ -1936,7 +1935,10 @@ async function tryPrAutomerge(
|
||||
}
|
||||
|
||||
logger.debug(`GitHub-native automerge: success...PrNo: ${prNumber}`);
|
||||
} catch (err) /* v8 ignore next: missing test #22198 */ {
|
||||
} catch (err) {
|
||||
if (err instanceof Error && err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
|
||||
throw err;
|
||||
}
|
||||
logger.warn({ prNumber, err }, 'GitHub-native automerge: REST API error');
|
||||
}
|
||||
}
|
||||
@@ -2038,6 +2040,9 @@ async function isMergeQueueEnabled(baseBranch: string): Promise<boolean> {
|
||||
result = isNonEmptyObject(res?.data?.repository?.mergeQueue);
|
||||
}
|
||||
} catch (err) {
|
||||
if (err instanceof Error && err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
|
||||
throw err;
|
||||
}
|
||||
logger.debug(
|
||||
{ baseBranch, err },
|
||||
'Error fetching merge queue status - assuming merge queue is enabled',
|
||||
@@ -2142,7 +2147,10 @@ export async function reattemptPlatformAutomerge({
|
||||
await tryPrAutomerge(number, node_id, platformPrOptions);
|
||||
|
||||
logger.debug(`PR platform automerge re-attempted...prNo: ${number}`);
|
||||
} catch (err) /* v8 ignore next -- defensive: automerge re-attempt failures are logged and swallowed, not simulated in specs */ {
|
||||
} catch (err) {
|
||||
if (err instanceof Error && err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
|
||||
throw err;
|
||||
}
|
||||
logger.warn({ err }, 'Error re-attempting PR platform automerge');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { isEmptyArray, isNonEmptyArray } from '@sindresorhus/is';
|
||||
import { DateTime } from 'luxon';
|
||||
import { GlobalConfig } from '../../../config/global.ts';
|
||||
import { PLATFORM_RATE_LIMIT_EXCEEDED } from '../../../constants/error-messages.ts';
|
||||
import { instrument } from '../../../instrumentation/index.ts';
|
||||
import { logger } from '../../../logger/index.ts';
|
||||
import { ExternalHostError } from '../../../types/errors/external-host-error.ts';
|
||||
@@ -227,6 +228,9 @@ export async function isPrInMergeQueue(
|
||||
}
|
||||
return res?.data?.repository?.pullRequest?.isInMergeQueue === true;
|
||||
} catch (err) {
|
||||
if (err instanceof Error && err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
|
||||
throw err;
|
||||
}
|
||||
logger.debug({ prNo, err }, 'Error fetching PR merge queue status');
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { PLATFORM_RATE_LIMIT_EXCEEDED } from '../../../constants/error-messages.ts';
|
||||
import { logger } from '../../../logger/index.ts';
|
||||
import * as githubHttp from '../../../util/http/github.ts';
|
||||
import type { EmailAddress } from '../../../util/schema-utils/index.ts';
|
||||
@@ -26,6 +27,9 @@ export async function getAppDetails(token: string): Promise<UserDetails> {
|
||||
email: null,
|
||||
};
|
||||
} catch (err) {
|
||||
if (err instanceof Error && err.message === PLATFORM_RATE_LIMIT_EXCEEDED) {
|
||||
throw err;
|
||||
}
|
||||
logger.debug({ err }, 'Error authenticating with GitHub');
|
||||
throw new Error('Init: Authentication failure');
|
||||
}
|
||||
|
||||
@@ -889,6 +889,26 @@ describe('util/http/github', () => {
|
||||
).toMatchInlineSnapshot(`[]`);
|
||||
});
|
||||
|
||||
it('throws when an app installation exhausts its GraphQL budget', async () => {
|
||||
httpMock
|
||||
.scope(githubApiHost)
|
||||
.post('/graphql')
|
||||
.reply(200, {
|
||||
errors: [
|
||||
{
|
||||
type: 'RATE_LIMIT',
|
||||
code: 'graphql_rate_limit',
|
||||
message:
|
||||
'API rate limit already exceeded for installation ID XXXXXXX.',
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
githubApi.queryRepoField(graphqlQuery, 'testItem'),
|
||||
).rejects.toThrow(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
});
|
||||
|
||||
it('queryRepo', async () => {
|
||||
const repository = {
|
||||
foo: 'foo',
|
||||
|
||||
+22
-1
@@ -70,10 +70,24 @@ export type GithubGraphqlResponse<T = unknown> =
|
||||
data?: never;
|
||||
errors: {
|
||||
type?: string;
|
||||
code?: string;
|
||||
message: string;
|
||||
}[];
|
||||
};
|
||||
|
||||
/**
|
||||
* GitHub reports a spent GraphQL budget in two shapes: `RATE_LIMITED` from the
|
||||
* GraphQL API itself, and `graphql_rate_limit` when an app installation has
|
||||
* exhausted its allowance. Both mean "come back later", not "unknown error".
|
||||
*/
|
||||
function isGraphqlRateLimited(
|
||||
errors: { type?: string; code?: string }[] | undefined,
|
||||
): boolean {
|
||||
return !!errors?.some(
|
||||
(err) => err.type === 'RATE_LIMITED' || err.code === 'graphql_rate_limit',
|
||||
);
|
||||
}
|
||||
|
||||
const GithubError = z.object({
|
||||
field: z.string().optional(),
|
||||
code: z.string().optional(),
|
||||
@@ -580,9 +594,10 @@ export class GithubHttp extends HttpBase<GithubHttpOptions> {
|
||||
}
|
||||
logger.trace(`Performing Github GraphQL request`);
|
||||
|
||||
let response: GithubGraphqlResponse<T> | null;
|
||||
try {
|
||||
const res = await this.postJson<GithubGraphqlResponse<T>>(path, opts);
|
||||
return res?.body;
|
||||
response = res.body;
|
||||
} catch (err) {
|
||||
logger.debug({ err, query, options }, 'Unexpected GraphQL Error');
|
||||
if (err instanceof ExternalHostError && count && count > 10) {
|
||||
@@ -591,6 +606,12 @@ export class GithubHttp extends HttpBase<GithubHttpOptions> {
|
||||
}
|
||||
throw handleGotError(err, path, opts);
|
||||
}
|
||||
|
||||
if (isGraphqlRateLimited(response?.errors)) {
|
||||
throw new Error(PLATFORM_RATE_LIMIT_EXCEEDED);
|
||||
}
|
||||
|
||||
return response;
|
||||
}
|
||||
|
||||
async queryRepoField<T = Record<string, unknown>>(
|
||||
|
||||
Reference in New Issue
Block a user