From 5a75fce1bee74ab489d28da4f7463dfc2f60727f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 16 Jul 2026 11:59:20 -0700 Subject: [PATCH] Scope remaining GitHub rate-limit accounting --- src/main/github/client-pr-checks.test.ts | 27 +++--- src/main/github/client.ts | 8 +- .../github/pr-refresh-coordinator.test.ts | 87 ++++++++++++++++-- src/main/github/pr-refresh-coordinator.ts | 36 ++++++-- src/main/github/work-item-details.test.ts | 88 +++++++++++++------ 5 files changed, 192 insertions(+), 54 deletions(-) diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 782fa277d39..0c5a0ac82b4 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -8,8 +8,6 @@ const { gitExecFileAsyncMock, extractExecErrorMock, getRateLimitMock, - rateLimitGuardMock, - noteRateLimitSpendMock, repositoryRateLimitGuardMock, noteRepositoryRateLimitSpendMock, spendsSharedGitHubComQuotaMock, @@ -32,11 +30,11 @@ const { return { stderr: String(err), stdout: '' } }), getRateLimitMock: vi.fn(), - rateLimitGuardMock: vi.fn(() => ({ blocked: false })), - noteRateLimitSpendMock: vi.fn(), repositoryRateLimitGuardMock: vi.fn(() => ({ blocked: false })), noteRepositoryRateLimitSpendMock: vi.fn(), - spendsSharedGitHubComQuotaMock: vi.fn(() => true), + spendsSharedGitHubComQuotaMock: vi.fn< + (repository?: { host?: string } | null, options?: { wslDistro?: string }) => boolean + >(() => true), acquireMock: vi.fn(), releaseMock: vi.fn() })) @@ -71,8 +69,6 @@ vi.mock('../git/runner', () => ({ vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, - rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock, repositoryRateLimitGuard: repositoryRateLimitGuardMock, noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock, spendsSharedGitHubComQuota: spendsSharedGitHubComQuotaMock @@ -211,9 +207,14 @@ describe('getPRChecks', () => { extractExecErrorMock.mockClear() getRateLimitMock.mockReset() getRateLimitMock.mockResolvedValue({ resources: {} }) - rateLimitGuardMock.mockReset() - rateLimitGuardMock.mockReturnValue({ blocked: false }) - noteRateLimitSpendMock.mockReset() + repositoryRateLimitGuardMock.mockReset() + repositoryRateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRepositoryRateLimitSpendMock.mockReset() + spendsSharedGitHubComQuotaMock.mockReset() + spendsSharedGitHubComQuotaMock.mockImplementation( + (repository?: { host?: string } | null, options?: { wslDistro?: string }) => + (!repository?.host || repository.host.toLowerCase() === 'github.com') && !options?.wslDistro + ) acquireMock.mockReset() releaseMock.mockReset() acquireMock.mockResolvedValue(undefined) @@ -602,6 +603,12 @@ describe('getPRChecks', () => { env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) }) ) + expect(getRateLimitMock).not.toHaveBeenCalled() + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith( + { owner: 'acme', repo: 'widgets', host: 'github.com' }, + 'graphql', + expect.objectContaining({ cwd: '/repo-root', wslDistro: 'Ubuntu', host: 'github.com' }) + ) }) it('uses explicit PR repo for rollup and gh pr checks fallback', async () => { diff --git a/src/main/github/client.ts b/src/main/github/client.ts index c73230f323f..7529a66946b 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -117,7 +117,6 @@ import { mapGraphQLReactionGroups, type GitHubGraphQLReactionGroup } from './com import { getRateLimit, noteRepositoryRateLimitSpend, - rateLimitGuard, repositoryRateLimitGuard, spendsSharedGitHubComQuota, type RateLimitBucketKind @@ -189,11 +188,10 @@ async function assertRateLimitBudget( repository?: GitHubApiRepository | null, executionOptions?: Pick ): Promise { - if (!spendsSharedGitHubComQuota(repository, executionOptions)) { - return + if (spendsSharedGitHubComQuota(repository, executionOptions)) { + await getRateLimit() } - await getRateLimit() - const guard = rateLimitGuard(bucket) + const guard = repositoryRateLimitGuard(repository, bucket, executionOptions) if (guard.blocked) { throw new Error( `GitHub ${bucket} rate limit is low; retry after ${new Date(guard.resetAt * 1000).toLocaleTimeString()}` diff --git a/src/main/github/pr-refresh-coordinator.test.ts b/src/main/github/pr-refresh-coordinator.test.ts index 5ee1a882a62..91692e90697 100644 --- a/src/main/github/pr-refresh-coordinator.test.ts +++ b/src/main/github/pr-refresh-coordinator.test.ts @@ -8,15 +8,21 @@ const { sendToTrustedUIRendererMock, getAllWebContentsMock, getPRForBranchOutcomeMock, + getOriginGitHubApiRepositoryMock, getRateLimitMock, - rateLimitGuardMock + noteRepositoryRateLimitSpendMock, + repositoryRateLimitGuardMock, + spendsSharedGitHubComQuotaMock } = vi.hoisted(() => ({ sendMock: vi.fn(), sendToTrustedUIRendererMock: vi.fn(), getAllWebContentsMock: vi.fn(), getPRForBranchOutcomeMock: vi.fn(), + getOriginGitHubApiRepositoryMock: vi.fn(), getRateLimitMock: vi.fn(), - rateLimitGuardMock: vi.fn() + noteRepositoryRateLimitSpendMock: vi.fn(), + repositoryRateLimitGuardMock: vi.fn(), + spendsSharedGitHubComQuotaMock: vi.fn() })) vi.mock('electron', () => ({ @@ -29,10 +35,15 @@ vi.mock('./client', () => ({ getPRForBranchOutcome: getPRForBranchOutcomeMock })) +vi.mock('./github-api-repository', () => ({ + getOriginGitHubApiRepository: getOriginGitHubApiRepositoryMock +})) + vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, - noteRateLimitSpend: vi.fn(), - rateLimitGuard: rateLimitGuardMock + noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock, + repositoryRateLimitGuard: repositoryRateLimitGuardMock, + spendsSharedGitHubComQuota: spendsSharedGitHubComQuotaMock })) vi.mock('../ipc/ui', () => ({ @@ -94,9 +105,21 @@ describe('pr-refresh-coordinator', () => { }) getAllWebContentsMock.mockReset() getPRForBranchOutcomeMock.mockReset() + getOriginGitHubApiRepositoryMock.mockReset() + getOriginGitHubApiRepositoryMock.mockResolvedValue({ + owner: 'acme', + repo: 'widgets', + host: 'github.com' + }) getRateLimitMock.mockReset() - rateLimitGuardMock.mockReset() - rateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRepositoryRateLimitSpendMock.mockReset() + repositoryRateLimitGuardMock.mockReset() + repositoryRateLimitGuardMock.mockReturnValue({ blocked: false }) + spendsSharedGitHubComQuotaMock.mockReset() + spendsSharedGitHubComQuotaMock.mockImplementation( + (repository: { host?: string } | null, options?: { wslDistro?: string }) => + (!repository?.host || repository.host.toLowerCase() === 'github.com') && !options?.wslDistro + ) getAllWebContentsMock.mockReturnValue([ { id: 1, @@ -569,6 +592,56 @@ describe('pr-refresh-coordinator', () => { expect(pausedEvents).toHaveLength(0) }) + it.each([ + { + scope: 'GitHub Enterprise', + repository: { owner: 'acme', repo: 'widgets', host: 'github.acme-corp.com' }, + localGitOptions: undefined + }, + { + scope: 'WSL', + repository: { owner: 'acme', repo: 'widgets', host: 'github.com' }, + localGitOptions: { wslDistro: 'Ubuntu' } + } + ])('bypasses the shared budget for $scope background refreshes', async (testCase) => { + const { enqueuePRRefresh } = await import('./pr-refresh-coordinator') + getOriginGitHubApiRepositoryMock.mockResolvedValue(testCase.repository) + getPRForBranchOutcomeMock.mockResolvedValue({ + kind: 'no-pr', + fetchedAt: Date.now() + }) + const executionOptions = testCase.localGitOptions ?? {} + + enqueuePRRefresh(makeCandidate({ localGitOptions: testCase.localGitOptions }), 'active', 80, 1) + + await vi.advanceTimersByTimeAsync(0) + + expect(getRateLimitMock).not.toHaveBeenCalled() + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith( + testCase.repository, + 'core', + executionOptions + ) + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith( + testCase.repository, + 'graphql', + executionOptions + ) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( + testCase.repository, + 'core', + 1, + executionOptions + ) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( + testCase.repository, + 'graphql', + 1, + executionOptions + ) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1) + }) + it('does not consume active burst slots for rate-limit pauses', async () => { const { enqueuePRRefresh } = await import('./pr-refresh-coordinator') // Why: keyed on drained items (one getRateLimit call each) rather than @@ -579,7 +652,7 @@ describe('pr-refresh-coordinator', () => { drainedItems += 1 return { ok: true } }) - rateLimitGuardMock.mockImplementation(() => + repositoryRateLimitGuardMock.mockImplementation(() => drainedItems <= 3 ? { blocked: true, remaining: 0, limit: 5000, resetAt: 61 } : { blocked: false } diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts index 592bea4870b..07b4ceb8f6b 100644 --- a/src/main/github/pr-refresh-coordinator.ts +++ b/src/main/github/pr-refresh-coordinator.ts @@ -9,7 +9,14 @@ import type { PRRefreshOutcome } from '../../shared/types' import { getPRForBranchOutcome, type GitHubPRBranchLookupOptions } from './client' -import { getRateLimit, noteRateLimitSpend, rateLimitGuard } from './rate-limit' +import { getOriginGitHubApiRepository } from './github-api-repository' +import { ghRepoExecOptions, githubRepoContext } from './gh-utils' +import { + getRateLimit, + noteRepositoryRateLimitSpend, + repositoryRateLimitGuard, + spendsSharedGitHubComQuota +} from './rate-limit' import { recordCoalescedCrashBreadcrumb } from '../crash-reporting/crash-breadcrumb-store' import { sendToTrustedUIRenderer } from '../ipc/ui' @@ -728,11 +735,20 @@ async function drainQueue(): Promise { ) if (isBackground(next.reason)) { - // Why: the probe only warms rateLimitGuard's cache, so it must fail open — GHES with rate limiting disabled 404s every probe (#7553). - await getRateLimit() + const executionOptions = next.candidate.localGitOptions ?? {} + const repository = await getOriginGitHubApiRepository( + next.candidate.repoPath, + next.candidate.connectionId, + executionOptions + ) + // Why: only native github.com uses the singleton snapshot; scoped breakers protect GHES and WSL. + if (spendsSharedGitHubComQuota(repository, executionOptions)) { + // Why: the probe only warms the cache, so failures must fail open (#7553). + await getRateLimit() + } const buckets = backgroundRefreshBuckets() const blockedGuard = buckets - .map((bucket) => rateLimitGuard(bucket)) + .map((bucket) => repositoryRateLimitGuard(repository, bucket, executionOptions)) .find((guard) => guard.blocked) if (blockedGuard?.blocked) { const retryAt = blockedGuard.resetAt * 1000 @@ -755,7 +771,7 @@ async function drainQueue(): Promise { noteActiveStart(next) } for (const bucket of buckets) { - noteRateLimitSpend(bucket) + noteRepositoryRateLimitSpend(repository, bucket, 1, executionOptions) } } @@ -922,8 +938,16 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise } // Why: enforce the rate-limit gate so a stale renderer can't bypass it; refuse without spending quota until the later of the two cooldowns. + const manualExecutionOptions = ghRepoExecOptions( + githubRepoContext(candidate.repoPath, candidate.connectionId, candidate.localGitOptions) + ) + const manualRepository = await getOriginGitHubApiRepository( + candidate.repoPath, + candidate.connectionId, + manualExecutionOptions + ) const manualBlockedGuard = backgroundRefreshBuckets() - .map((bucket) => rateLimitGuard(bucket)) + .map((bucket) => repositoryRateLimitGuard(manualRepository, bucket, manualExecutionOptions)) .find((guard) => guard.blocked) const secondaryGateUntil = manualRetryGates.get(key) const gateUntil = Math.max( diff --git a/src/main/github/work-item-details.test.ts b/src/main/github/work-item-details.test.ts index 87ca15355f7..b34eb0acbda 100644 --- a/src/main/github/work-item-details.test.ts +++ b/src/main/github/work-item-details.test.ts @@ -14,8 +14,6 @@ const { getWorkItemByOwnerRepoMock, getPRChecksMock, getPRCommentsMock, - rateLimitGuardMock, - noteRateLimitSpendMock, repositoryRateLimitGuardMock, noteRepositoryRateLimitSpendMock, ghRepoExecOptionsMock, @@ -32,8 +30,6 @@ const { getWorkItemByOwnerRepoMock: vi.fn(), getPRChecksMock: vi.fn(), getPRCommentsMock: vi.fn(), - rateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), - noteRateLimitSpendMock: vi.fn(), repositoryRateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), noteRepositoryRateLimitSpendMock: vi.fn(), ghRepoExecOptionsMock: vi.fn((context) => @@ -73,8 +69,6 @@ vi.mock('./github-enterprise-repository', () => ({ })) vi.mock('./rate-limit', () => ({ - rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock, repositoryRateLimitGuard: repositoryRateLimitGuardMock, noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock })) @@ -102,9 +96,6 @@ describe('getWorkItemDetails', () => { getWorkItemByOwnerRepoMock.mockReset() getPRChecksMock.mockReset() getPRCommentsMock.mockReset() - rateLimitGuardMock.mockReset() - rateLimitGuardMock.mockReturnValue({ blocked: false }) - noteRateLimitSpendMock.mockReset() repositoryRateLimitGuardMock.mockReset() repositoryRateLimitGuardMock.mockReturnValue({ blocked: false }) noteRepositoryRateLimitSpendMock.mockReset() @@ -248,6 +239,68 @@ describe('getWorkItemDetails', () => { } ]) expect(details?.participants?.[0]?.login).toBe('issue-author') + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith( + { owner: 'acme', repo: 'widgets' }, + 'graphql', + {} + ) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( + { owner: 'acme', repo: 'widgets' }, + 'graphql', + 1, + {} + ) + }) + + it('scopes collapsed issue GraphQL accounting to the selected WSL runtime', async () => { + const repository = { owner: 'acme', repo: 'widgets' } + const localGitOptions = { wslDistro: 'Ubuntu' } + getWorkItemMock.mockResolvedValueOnce({ + id: 'issue:923', + type: 'issue', + number: 923, + title: 'Use upstream issues', + state: 'open', + url: 'https://github.com/acme/widgets/issues/923', + labels: [], + updatedAt: '2026-04-01T00:00:00Z', + author: 'issue-author' + }) + getIssueOwnerRepoMock.mockResolvedValue(repository) + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + data: { + repository: { + issue: { + body: 'WSL issue body', + assignees: { nodes: [] }, + participants: { nodes: [] }, + comments: { nodes: [] } + } + } + } + }) + }) + .mockResolvedValueOnce({ stdout: '' }) + + const details = await getWorkItemDetails('/repo-root', 923, 'issue', null, localGitOptions) + + expect(details?.body).toBe('WSL issue body') + expect(repositoryRateLimitGuardMock).toHaveBeenCalledWith( + repository, + 'graphql', + localGitOptions + ) + expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( + repository, + 'graphql', + 1, + localGitOptions + ) + expect(ghExecFileAsyncMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( + true + ) }) it('enriches a non-participating assignee avatar from the GraphQL assignees connection', async () => { @@ -432,14 +485,6 @@ describe('getWorkItemDetails', () => { }) it('skips optional GraphQL issue detail calls when the cached GraphQL budget is low', async () => { - // github.com repos share the singleton quota, so both the raw and - // repository-scoped guards report the same low-budget block. - rateLimitGuardMock.mockReturnValue({ - blocked: true, - remaining: 3, - limit: 5000, - resetAt: 1_800_000_000 - }) repositoryRateLimitGuardMock.mockReturnValue({ blocked: true, remaining: 3, @@ -467,7 +512,6 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(3) expect(ghExecFileAsyncMock.mock.calls.some((call) => call[0][1] === 'graphql')).toBe(false) - expect(noteRateLimitSpendMock).not.toHaveBeenCalled() expect(noteRepositoryRateLimitSpendMock).not.toHaveBeenCalled() expect(details?.body).toBe('Issue body') expect(details?.participants).toEqual([]) @@ -588,12 +632,6 @@ describe('getWorkItemDetails', () => { it('routes local WSL PR detail fan-out through the selected distro', async () => { const localGitOptions = { wslDistro: 'Ubuntu' } - rateLimitGuardMock.mockReturnValue({ - blocked: true, - remaining: 0, - limit: 5000, - resetAt: 1_800_000_000 - }) getWorkItemMock.mockResolvedValueOnce({ id: 'pr:42', type: 'pr', @@ -671,8 +709,6 @@ describe('getWorkItemDetails', () => { expect(ghExecFileAsyncMock.mock.calls.every((call) => call[1]?.wslDistro === 'Ubuntu')).toBe( true ) - expect(rateLimitGuardMock).not.toHaveBeenCalled() - expect(noteRateLimitSpendMock).not.toHaveBeenCalled() }) it('uses the GitHub Enterprise host for SSH-backed PR work item details', async () => {