diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index 74ab4cae05..ae57ae9285 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -55,7 +55,7 @@ import { User, } from './interface'; import { IssueChangeEvent, IssueModel } from './issueModel'; -import { LoggingOctokit } from './loggingOctokit'; +import { getErrorCode, LoggingOctokit } from './loggingOctokit'; import { PullRequestModel } from './pullRequestModel'; import defaultSchema from './queries.gql'; import * as extraSchema from './queriesExtra.gql'; @@ -144,52 +144,6 @@ export function isRateLimitError(e: unknown): boolean { return false; } -function isObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null; -} - -export function getErrorCode(e: unknown): string | undefined { - if (!isObject(e)) { - return undefined; - } - - if (e.status !== undefined) { - return String(e.status); - } - - const networkError = e.networkError; - if (isObject(networkError) && networkError.statusCode !== undefined) { - return String(networkError.statusCode); - } - - const graphQLErrors = e.graphQLErrors; - if (Array.isArray(graphQLErrors)) { - const firstGraphQLError = graphQLErrors[0]; - if (isObject(firstGraphQLError)) { - const extensions = firstGraphQLError.extensions; - if (isObject(extensions) && extensions.code !== undefined) { - return String(extensions.code); - } - } - } - - if (e.code !== undefined) { - return String(e.code); - } - - if (typeof e.name === 'string' && e.name) { - const message = typeof e.message === 'string' ? e.message : ''; - if (e.name !== 'Error') { - return message ? `${e.name}: ${message}` : e.name; - } - if (message) { - return message; - } - } - - return undefined; -} - export enum TeamReviewerRefreshKind { None, Try, @@ -258,6 +212,11 @@ export class GitHubRepository extends Disposable { // eslint-disable-next-line rulesdir/no-any-except-union-method-signature private _queriesSchema: any; private _areQueriesLimited: boolean = false; + + private static readonly MAX_ITEM_NUMBER_TTL_MS = 60 * 60 * 1000; /* 1 hour */ + private static readonly MAX_ITEM_NUMBER_BUFFER = 50; + private _maxItemNumberCache: { value: number; fetchedAt: number } | undefined; + private _maxItemNumberPromise: Promise | undefined; get areQueriesLimited(): boolean { return this._areQueriesLimited; } private _branchesCache: Map = new Map(); @@ -1012,6 +971,62 @@ export class GitHubRepository extends Disposable { return this._getMaxItem(false); } + /** + * Returns the highest known issue or pull request number for this repository. + * Issues and pull requests share the same number sequence on GitHub, so the + * larger of the two latest items is an upper bound for any valid number. + * Result is cached for a short TTL to avoid extra round-trips. + */ + private async getCachedMaxItemNumber(forceRefresh: boolean = false): Promise { + const now = Date.now(); + if (!forceRefresh && this._maxItemNumberCache && (now - this._maxItemNumberCache.fetchedAt) < GitHubRepository.MAX_ITEM_NUMBER_TTL_MS) { + return this._maxItemNumberCache.value; + } + if (this._maxItemNumberPromise) { + return this._maxItemNumberPromise; + } + this._maxItemNumberPromise = (async () => { + const [maxIssue, maxPr] = await Promise.all([this._getMaxItem(true), this._getMaxItem(false)]); + const max = Math.max(maxIssue ?? 0, maxPr ?? 0); + if (max > 0) { + this._maxItemNumberCache = { value: max, fetchedAt: Date.now() }; + return max; + } + return undefined; + })(); + try { + return await this._maxItemNumberPromise; + } finally { + this._maxItemNumberPromise = undefined; + } + } + + /** + * Returns false when `number` is implausibly higher than the latest known + * issue/PR number for this repository. Used to short-circuit fetches for + * numbers that came from arbitrary text (e.g. `#1234567890` in source code) + * before they hit the network and produce noisy error telemetry. + */ + private async isPlausibleItemNumber(itemNumber: number): Promise { + if (!Number.isFinite(itemNumber) || itemNumber <= 0) { + return false; + } + let max = await this.getCachedMaxItemNumber(); + if (max === undefined) { + // Couldn't determine the max (e.g. network error); allow through. + return true; + } + if (itemNumber <= max + GitHubRepository.MAX_ITEM_NUMBER_BUFFER) { + return true; + } + // Number is above the cached max; refresh once before deciding to handle newly-created items. + max = await this.getCachedMaxItemNumber(true); + if (max === undefined) { + return true; + } + return itemNumber <= max + GitHubRepository.MAX_ITEM_NUMBER_BUFFER; + } + async getViewerPermission(): Promise { try { Logger.debug(`Fetch viewer permission - enter`, this.id); @@ -1277,6 +1292,11 @@ export class GitHubRepository extends Disposable { return this._pullRequestModelsByNumber.get(id)!.model; } + if (!(await this.isPlausibleItemNumber(id))) { + Logger.debug(`Skipping pull request fetch for implausible number ${id} (caller: ${callerName})`, this.id); + return; + } + try { const { query, remote, schema } = await this.ensure(); Logger.debug(`Fetch pull request ${remote.owner}/${remote.repositoryName} ${id} - enter`, this.id); @@ -1341,6 +1361,10 @@ export class GitHubRepository extends Disposable { return cached; } } + if (!(await this.isPlausibleItemNumber(id))) { + Logger.debug(`Skipping issue fetch for implausible number ${id}`, this.id); + return undefined; + } try { Logger.debug(`Fetch issue ${id} - enter`, this.id); const { query, remote, schema } = await this.ensure(); @@ -1581,11 +1605,14 @@ export class GitHubRepository extends Disposable { let after: string | null = null; let hasNextPage = false; const ret: IAccount[] = []; + // Once we fall back to the legacy assignableUsers query, the cursors are not compatible + // with suggestedActors, so stay on the legacy query for the rest of the pagination. + let useLegacyAssignableUsers = false; do { try { - let result: { data: AssignableUsersResponse | SuggestedActorsResponse } | undefined; - if (schema.GetSuggestedActors) { + let result: { data: AssignableUsersResponse | SuggestedActorsResponse | null } | undefined; + if (schema.GetSuggestedActors && !useLegacyAssignableUsers) { result = await query({ query: schema.GetSuggestedActors, variables: { @@ -1617,13 +1644,20 @@ export class GitHubRepository extends Disposable { }, true); // we ignore SAML errors here because this query can happen at startup } - if (result.data.repository === null) { + if (result.data?.repository === null) { Logger.error('Unexpected null repository when getting assignable users', this.id); return []; } const users = (result.data as AssignableUsersResponse).repository?.assignableUsers ?? (result.data as SuggestedActorsResponse).repository?.suggestedActors; + // If we got assignableUsers back (either because we already used the legacy query, or + // because the legacy fallback kicked in inside query()), the cursor is incompatible with + // suggestedActors. Stay on the legacy query for subsequent pages. + if ((result.data as AssignableUsersResponse).repository?.assignableUsers) { + useLegacyAssignableUsers = true; + } + ret.push( ...(users?.nodes.map(node => { return parseAccount(node, this); diff --git a/src/github/loggingOctokit.ts b/src/github/loggingOctokit.ts index 59d6afa8a8..5fc39ba634 100644 --- a/src/github/loggingOctokit.ts +++ b/src/github/loggingOctokit.ts @@ -28,6 +28,52 @@ interface RateLimitResult { } | undefined; } +function isObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +export function getErrorCode(e: unknown): string | undefined { + if (!isObject(e)) { + return undefined; + } + + if (e.status !== undefined) { + return String(e.status); + } + + const networkError = e.networkError; + if (isObject(networkError) && networkError.statusCode !== undefined) { + return String(networkError.statusCode); + } + + const graphQLErrors = e.graphQLErrors; + if (Array.isArray(graphQLErrors)) { + const firstGraphQLError = graphQLErrors[0]; + if (isObject(firstGraphQLError)) { + const extensions = firstGraphQLError.extensions; + if (isObject(extensions) && extensions.code !== undefined) { + return String(extensions.code); + } + } + } + + if (e.code !== undefined) { + return String(e.code); + } + + if (typeof e.name === 'string' && e.name) { + const message = typeof e.message === 'string' ? e.message : ''; + if (e.name !== 'Error') { + return message ? `${e.name}: ${message}` : e.name; + } + if (message) { + return message; + } + } + + return undefined; +} + export class RateLogger { private bulkhead: BulkheadPolicy = bulkhead(140); private static ID = 'RateLimit'; @@ -111,6 +157,25 @@ export class RateLogger { } } + public logApiError(info: string | undefined, apiResult: Promise): void { + apiResult.catch(e => { + const properties: { operation: string; errorCode?: string } = { + operation: RateLogger.sanitizeOperationName(info ?? 'unknown'), + }; + const errorCode = getErrorCode(e); + if (errorCode) { + properties.errorCode = errorCode; + } + /* __GDPR__ + "pr.apiCallFailed" : { + "operation": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth" }, + "errorCode": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth" } + } + */ + this.telemetry.sendTelemetryErrorEvent('pr.apiCallFailed', properties); + }); + } + public async logRestRateLimit(info: string | undefined, restResponse: Promise) { let result; try { @@ -139,6 +204,7 @@ export class LoggingApolloClient { throw new Error('API call count has exceeded a rate limit.'); } this._rateLogger.logRateLimit(logInfo, result as Promise); + this._rateLogger.logApiError(logInfo, result); return result; } @@ -149,6 +215,7 @@ export class LoggingApolloClient { throw new Error('API call count has exceeded a rate limit.'); } this._rateLogger.logRateLimit(logInfo, result as Promise); + this._rateLogger.logApiError(logInfo, result); return result; } } @@ -163,6 +230,7 @@ export class LoggingOctokit { throw new Error('API call count has exceeded a rate limit.'); } this._rateLogger.logRestRateLimit(logInfo, result as Promise as Promise); + this._rateLogger.logApiError(logInfo, result); return result; } }