diff --git a/mobile/src/session/github-pr-rpc.ts b/mobile/src/session/github-pr-rpc.ts index 0ae009d92..24baef0a2 100644 --- a/mobile/src/session/github-pr-rpc.ts +++ b/mobile/src/session/github-pr-rpc.ts @@ -141,7 +141,10 @@ export async function fetchHostedReviewForBranch( { repo: mobileRepoSelectorFromWorktreeId(worktreeId), branch: args.branch, - linkedGitHubPR: args.linkedGitHubPR ?? null + linkedGitHubPR: args.linkedGitHubPR ?? null, + // Why: the mobile PR sidebar is only ever open on the selected worktree, + // so it belongs in the host's fast re-check tier (#11532). + active: true }, readForBranch ) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 4b4ede36e..4b5f1375a 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -170,6 +170,7 @@ import { getPRComments, getPRForBranch, getPRForBranchOutcome, + getGitHubPRLookupRateLimitBlock, getRepoSlug, getRepoUpstream, getWorkItem, @@ -4609,3 +4610,70 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(noteRateLimitSpendMock).not.toHaveBeenCalled() }) }) + +describe('getGitHubPRLookupRateLimitBlock', () => { + beforeEach(() => { + execFileAsyncMock.mockReset() + ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getRemoteUrlForRepoMock.mockReset() + gitExecFileAsyncMock.mockReset() + getRateLimitMock.mockReset() + getRateLimitMock.mockResolvedValue(undefined) + rateLimitGuardMock.mockReset() + rateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRateLimitSpendMock.mockReset() + _resetOwnerRepoCache() + }) + + it('reports no block while every lookup bucket has budget', async () => { + await expect(getGitHubPRLookupRateLimitBlock('/repo-root')).resolves.toBeNull() + expect(getRateLimitMock).toHaveBeenCalled() + }) + + it('reports a block when either lookup bucket is exhausted', async () => { + rateLimitGuardMock.mockImplementation(((bucket: string) => + bucket === 'graphql' + ? { blocked: true, remaining: 4, limit: 5000, resetAt: 1_800_000_000 } + : { blocked: false }) as () => RateLimitGuardResult) + + await expect(getGitHubPRLookupRateLimitBlock('/repo-root')).resolves.toEqual({ + resetAt: 1_800_000_000 + }) + }) + + it('reports the latest reset when both lookup buckets are exhausted', async () => { + rateLimitGuardMock.mockImplementation(((bucket: string) => ({ + blocked: true, + remaining: 4, + limit: 5000, + // Why: core resets first, so returning it would retry into graphql's block. + resetAt: bucket === 'core' ? 1_800_000_000 : 1_800_003_600 + })) as () => RateLimitGuardResult) + + await expect(getGitHubPRLookupRateLimitBlock('/repo-root')).resolves.toEqual({ + resetAt: 1_800_003_600 + }) + }) + + it('reports the later reset when graphql outlasts core', async () => { + // Retrying at the earlier reset would fail again on the bucket still blocked. + rateLimitGuardMock.mockImplementation(((bucket: string) => ({ + blocked: true, + remaining: 0, + limit: 5000, + resetAt: bucket === 'graphql' ? 1_800_000_600 : 1_800_000_000 + })) as () => RateLimitGuardResult) + + await expect(getGitHubPRLookupRateLimitBlock('/repo-root')).resolves.toEqual({ + resetAt: 1_800_000_600 + }) + }) + + it('fails open when the exempt rate-limit probe itself fails', async () => { + getRateLimitMock.mockRejectedValue(new Error('probe offline')) + + await expect(getGitHubPRLookupRateLimitBlock('/repo-root')).resolves.toBeNull() + }) +}) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index ff602bc4d..3684d7729 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -195,6 +195,52 @@ async function assertRateLimitBudget( } } +// Why: a branch lookup prefers REST but can fall back to `gh pr list` and +// `gh pr view`, so both buckets are guarded and charged. Mirrors the PR refresh +// coordinator's own estimate. +const PR_BRANCH_LOOKUP_BUCKETS = ['core', 'graphql'] as const + +/** + * Rate-limit floor for GitHub PR lookups that do not run through the PR refresh + * coordinator's queue (#11532). + * + * The coordinator guards and paces its own background refreshes, but + * `hostedReview:forBranch` polls the same lookup straight from the renderer. + * Ungated, the two paths together could spend the user's entire hourly quota — + * which is per user and shared with their own `gh` and CLI agents. + * Returns the reset time when the caller must not spend, else `null`. + */ +export async function getGitHubPRLookupRateLimitBlock( + repoPath: string, + connectionId?: string | null, + localGitOptions: LocalGitExecOptions = {} +): Promise<{ resetAt: number } | null> { + const executionOptions = ghRepoExecOptions( + githubRepoContext(repoPath, connectionId, localGitOptions) + ) + // Why: identity resolution runs local git, which can fail for reasons that + // have nothing to do with the budget; let the lookup itself classify those. + const repository = await getOriginGitHubApiRepository( + repoPath, + connectionId, + executionOptions + ).catch(() => null) + if (repository === null) { + return null + } + if (spendsSharedGitHubComQuota(repository, executionOptions)) { + // Why: the probe only warms the snapshot and is exempt from limits, so a + // failure must fail open rather than block the lookup (#7553). + await getRateLimit().catch(() => undefined) + } + // Why: retrying at the earlier reset would fail again on the bucket that has + // not reset yet, so the latest blocked reset is the only honest retry time. + const resets = PR_BRANCH_LOOKUP_BUCKETS.map((bucket) => + repositoryRateLimitGuard(repository, bucket, executionOptions) + ).flatMap((guard) => (guard.blocked ? [guard.resetAt] : [])) + return resets.length > 0 ? { resetAt: Math.max(...resets) } : null +} + function prRefreshUpstreamError( err: unknown ): Extract { @@ -2956,6 +3002,13 @@ export async function getPRForBranchOutcome( if (connectionId && candidates.length === 0) { return { kind: 'no-pr', fetchedAt: Date.now() } } + // Why (#11532): account every lookup, not just the coordinator's queue — + // `hostedReview:forBranch` reaches this directly from renderer polling and + // was spending the shared quota invisibly. headRepo is `origin`, the same + // identity the coordinator guards on. + for (const bucket of PR_BRANCH_LOOKUP_BUCKETS) { + noteRepositoryRateLimitSpend(headRepo ?? candidates[0], bucket, 1, ghOptions) + } let data: PullRequestLookupData | null = null let dataRepo: OwnerRepo | null = null let dataHeadRepo: OwnerRepo | null = headRepo diff --git a/src/main/github/default-branch-stale-pr.test.ts b/src/main/github/default-branch-stale-pr.test.ts index 23f3eb8b9..fde4b6b8b 100644 --- a/src/main/github/default-branch-stale-pr.test.ts +++ b/src/main/github/default-branch-stale-pr.test.ts @@ -32,6 +32,7 @@ const { getRateLimitMock, rateLimitGuardMock, noteRateLimitSpendMock, + noteRepositoryRateLimitSpendMock, ghRepoExecOptionsMock, githubRepoContextMock, getSshGitProviderMock, @@ -52,6 +53,7 @@ const { blocked: false })), noteRateLimitSpendMock: vi.fn(), + noteRepositoryRateLimitSpendMock: vi.fn(), ghRepoExecOptionsMock: vi.fn((context) => context.connectionId ? {} @@ -116,7 +118,8 @@ vi.mock('./local-git-config-signature', () => ({ vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, - noteRateLimitSpend: noteRateLimitSpendMock + noteRateLimitSpend: noteRateLimitSpendMock, + noteRepositoryRateLimitSpend: noteRepositoryRateLimitSpendMock })) import { diff --git a/src/main/github/pr-refresh-coordinator.test.ts b/src/main/github/pr-refresh-coordinator.test.ts index 7298c36af..30fc44b3c 100644 --- a/src/main/github/pr-refresh-coordinator.test.ts +++ b/src/main/github/pr-refresh-coordinator.test.ts @@ -649,18 +649,9 @@ describe('pr-refresh-coordinator', () => { 'graphql', executionOptions ) - expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( - testCase.repository, - 'core', - 1, - executionOptions - ) - expect(noteRepositoryRateLimitSpendMock).toHaveBeenCalledWith( - testCase.repository, - 'graphql', - 1, - executionOptions - ) + // Why (#11532): the lookup itself debits the snapshot now, so every caller + // is accounted for; the coordinator must not double-charge on top. + expect(noteRepositoryRateLimitSpendMock).not.toHaveBeenCalled() expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1) }) diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts index 70b13ec2e..190cc82c6 100644 --- a/src/main/github/pr-refresh-coordinator.ts +++ b/src/main/github/pr-refresh-coordinator.ts @@ -11,12 +11,11 @@ import type { import { getPRForBranchOutcome, type GitHubPRBranchLookupOptions } from './client' import { getOriginGitHubApiRepository } from './github-api-repository' import { ghRepoExecOptions, githubRepoContext } from './gh-utils' +import { getRateLimit, repositoryRateLimitGuard, spendsSharedGitHubComQuota } from './rate-limit' import { - getRateLimit, - noteRepositoryRateLimitSpend, - repositoryRateLimitGuard, - spendsSharedGitHubComQuota -} from './rate-limit' + lookupBackoffDelayMs, + NO_REVIEW_REFRESH_INTERVAL_MS +} from '../source-control/hosted-review-refresh-pacing' import { recordCoalescedCrashBreadcrumb } from '../crash-reporting/crash-breadcrumb-store' import { sendToTrustedUIRenderer } from '../ipc/ui' @@ -74,8 +73,6 @@ const BACKGROUND_BUDGET_WINDOW_MS = 5 * 60_000 const MIN_BACKGROUND_SPACING_MS = 10_000 const BACKGROUND_BUDGET_MAX = 20 const POST_PUSH_DELAY_MS = 2_500 -const BACKOFF_BASE_MS = 60_000 -const BACKOFF_MAX_MS = 15 * 60_000 const DIAGNOSTIC_BREADCRUMB_MIN_INTERVAL_MS = 30_000 const ACTIVE_BURST_WINDOW_MS = 30_000 const ACTIVE_BURST_MAX = 3 @@ -424,8 +421,7 @@ function removeQueuedAliasForInvalidCandidate(key: string, alias: GitHubPRRefres */ function nextVisibleErrorRetryAt(key: string): number { const failures = (errorBackoff.get(key)?.failures ?? 0) + 1 - const retryAt = - Date.now() + Math.min(BACKOFF_MAX_MS, BACKOFF_BASE_MS * 2 ** Math.min(failures - 1, 4)) + const retryAt = Date.now() + lookupBackoffDelayMs(failures) errorBackoff.set(key, { failures, retryAt }) return retryAt } @@ -509,7 +505,7 @@ function refreshIntervalForCandidate(candidate: GitHubPRRefreshCandidate): numbe return 30 * 60_000 } if (candidate.cachedHasPR === false) { - return 15 * 60_000 + return NO_REVIEW_REFRESH_INTERVAL_MS } if ( candidate.cachedHasPR === true && @@ -776,9 +772,8 @@ async function drainQueue(): Promise { // Why: tab/worktree churn can enqueue many distinct active refreshes that each probe local Git. noteActiveStart(next) } - for (const bucket of buckets) { - noteRepositoryRateLimitSpend(repository, bucket, 1, executionOptions) - } + // Why (#11532): the lookup itself now debits the snapshot, so every + // caller is accounted for; debiting here too would double-count. } const outcome = await getPRForBranchOutcome( diff --git a/src/main/ipc/hosted-review.test.ts b/src/main/ipc/hosted-review.test.ts index 2f0e3e155..cef5d7f1d 100644 --- a/src/main/ipc/hosted-review.test.ts +++ b/src/main/ipc/hosted-review.test.ts @@ -285,6 +285,26 @@ describe('registerHostedReviewHandlers', () => { localGitExecOptions: { wslDistro: 'Ubuntu' } }) ) + // Card-list polling is the O(N) tier and must not claim the fast one. + expect(getHostedReviewForBranchMock.mock.calls[0][0]).not.toHaveProperty('active') + }) + + it('carries a selected-worktree claim through to the branch lookup', async () => { + getHostedReviewForBranchMock.mockResolvedValueOnce(null) + registerHostedReviewHandlers(store as never, stats as never) + + await handlers['hostedReview:forBranch'](null, { + repoPath, + repoId: repo.id, + branch: 'feature/selected', + active: true + }) + + // Why: the right sidebar renders only the selected worktree, so its lookup + // earns the per-minute tier instead of the card-list interval (#11532). + expect(getHostedReviewForBranchMock).toHaveBeenCalledWith( + expect.objectContaining({ branch: 'feature/selected', active: true }) + ) }) it('passes SSH connectionId through create eligibility instead of blocking the worktree', async () => { diff --git a/src/main/ipc/hosted-review.ts b/src/main/ipc/hosted-review.ts index 07e36469e..670064d3e 100644 --- a/src/main/ipc/hosted-review.ts +++ b/src/main/ipc/hosted-review.ts @@ -91,6 +91,7 @@ export function registerHostedReviewHandlers(store: Store, stats: StatsCollector linkedAzureDevOpsPR: args.linkedAzureDevOpsPR ?? null, linkedGiteaPR: args.linkedGiteaPR ?? null, currentHeadOid: args.currentHeadOid ?? null, + ...(args.active === true ? { active: true } : {}), ...(Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : {}) }) if (review?.provider === 'github' && !stats.hasCountedPR(review.url)) { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 963c91811..18273e3a6 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -18623,6 +18623,7 @@ export class OrcaRuntimeService { repoSelector: string branch: string currentHeadOid?: string | null + active?: boolean linkedGitHubPR?: number | null fallbackGitHubPR?: number | null linkedGitLabMR?: number | null @@ -18637,6 +18638,7 @@ export class OrcaRuntimeService { connectionId: repo.connectionId ?? null, branch: args.branch, currentHeadOid: args.currentHeadOid ?? null, + ...(args.active === true ? { active: true } : {}), linkedGitHubPR: args.linkedGitHubPR ?? null, fallbackGitHubPR: args.linkedGitHubPR == null ? (args.fallbackGitHubPR ?? null) : null, linkedGitLabMR: args.linkedGitLabMR ?? null, diff --git a/src/main/runtime/rpc/methods/hosted-review.test.ts b/src/main/runtime/rpc/methods/hosted-review.test.ts index 7c2e3cb7b..3d132e82e 100644 --- a/src/main/runtime/rpc/methods/hosted-review.test.ts +++ b/src/main/runtime/rpc/methods/hosted-review.test.ts @@ -49,6 +49,28 @@ describe('hosted review RPC methods', () => { }) }) + it('carries a selected-worktree claim through to the runtime', async () => { + const runtime = { + getRuntimeId: () => 'test-runtime', + getHostedReviewForBranch: vi.fn().mockResolvedValue(null) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: HOSTED_REVIEW_METHODS }) + + await dispatcher.dispatch( + makeRequest('hostedReview.forBranch', { + repo: '/repo', + branch: 'feature/selected', + active: true + }) + ) + + // Why: without this the mobile PR sidebar would sit on the card-list pacing + // and take a no-review interval to notice a PR opened elsewhere (#11532). + expect(runtime.getHostedReviewForBranch).toHaveBeenCalledWith( + expect.objectContaining({ active: true }) + ) + }) + it('dispatches creation eligibility requests to the runtime', async () => { const runtime = { getRuntimeId: () => 'test-runtime', diff --git a/src/main/runtime/rpc/methods/hosted-review.ts b/src/main/runtime/rpc/methods/hosted-review.ts index 538c6267c..b658c4c74 100644 --- a/src/main/runtime/rpc/methods/hosted-review.ts +++ b/src/main/runtime/rpc/methods/hosted-review.ts @@ -6,6 +6,8 @@ const HostedReviewForBranch = z.object({ repo: requiredString('Missing repo selector'), branch: requiredString('Missing branch'), currentHeadOid: z.string().nullable().optional(), + // Only the caller's selected worktree; the host caps how many earn the fast tier. + active: z.boolean().optional(), linkedGitHubPR: z.number().int().positive().nullable().optional(), fallbackGitHubPR: z.number().int().positive().nullable().optional(), linkedGitLabMR: z.number().int().positive().nullable().optional(), @@ -54,6 +56,7 @@ export const HOSTED_REVIEW_METHODS: RpcMethod[] = [ repoSelector: params.repo, branch: params.branch, currentHeadOid: params.currentHeadOid ?? null, + ...(params.active === true ? { active: true } : {}), linkedGitHubPR: params.linkedGitHubPR ?? null, ...(fallbackGitHubPR !== null ? { fallbackGitHubPR } : {}), linkedGitLabMR: params.linkedGitLabMR ?? null, diff --git a/src/main/source-control/forge-provider.test.ts b/src/main/source-control/forge-provider.test.ts index 884b72f90..ae5e5623a 100644 --- a/src/main/source-control/forge-provider.test.ts +++ b/src/main/source-control/forge-provider.test.ts @@ -12,6 +12,7 @@ const { getProjectSlugMock, getPRForBranchOutcomeMock, getRepoSlugMock, + getGitHubPRLookupRateLimitBlockMock, getEnterpriseGitHubRepoSlugMock } = vi.hoisted(() => ({ createGitHubPullRequestMock: vi.fn(), @@ -25,12 +26,16 @@ const { getProjectSlugMock: vi.fn(), getPRForBranchOutcomeMock: vi.fn(), getRepoSlugMock: vi.fn(), + getGitHubPRLookupRateLimitBlockMock: vi.fn(async () => null), getEnterpriseGitHubRepoSlugMock: vi.fn() })) vi.mock('../gitlab/client', () => ({ getProjectSlug: getProjectSlugMock, getMergeRequestForBranch: getMergeRequestForBranchMock, + // Why: forge-provider resolves branch reviews via the OrThrow variant so + // lookup failures surface as unavailable instead of "no MR found". + getMergeRequestForBranchOrThrow: getMergeRequestForBranchMock, getMergeRequest: vi.fn() })) @@ -41,7 +46,8 @@ vi.mock('../gitlab/merge-request-creation', () => ({ vi.mock('../github/client', () => ({ createGitHubPullRequest: createGitHubPullRequestMock, getRepoSlug: getRepoSlugMock, - getPRForBranchOutcome: getPRForBranchOutcomeMock + getPRForBranchOutcome: getPRForBranchOutcomeMock, + getGitHubPRLookupRateLimitBlock: getGitHubPRLookupRateLimitBlockMock })) vi.mock('../github/github-enterprise-repository', () => ({ @@ -103,6 +109,8 @@ describe('forge provider interface', () => { getPRForBranchOutcomeMock.mockReset() getRepoSlugMock.mockReset() getEnterpriseGitHubRepoSlugMock.mockReset() + getGitHubPRLookupRateLimitBlockMock.mockReset() + getGitHubPRLookupRateLimitBlockMock.mockResolvedValue(null) }) it('preserves the existing hosted provider detection order', async () => { @@ -377,4 +385,48 @@ describe('forge provider interface', () => { }) ).rejects.toThrow(/network/) }) + + it('refuses a GitHub branch lookup while the rate-limit budget is exhausted (#11532)', async () => { + getGitHubPRLookupRateLimitBlockMock.mockResolvedValueOnce({ + resetAt: 1_800_000_000 + } as never) + + await expect( + getForgeProviderById('github').getReviewForBranch({ + repoPath: '/repo', + connectionId: null, + branch: 'feature/x' + }) + // Throwing (not null) keeps a low budget from reading as "no pull request". + ).rejects.toThrow(/rate_limited/) + expect(getPRForBranchOutcomeMock).not.toHaveBeenCalled() + }) + + it('refuses a GitHub lookup by number while the rate-limit budget is exhausted (#11532)', async () => { + getGitHubPRLookupRateLimitBlockMock.mockResolvedValueOnce({ + resetAt: 1_800_000_000 + } as never) + + await expect( + getForgeProviderById('github').getReviewByNumber({ + repoPath: '/repo', + connectionId: null, + number: 42 + }) + ).rejects.toThrow(/rate_limited/) + expect(getPRForBranchOutcomeMock).not.toHaveBeenCalled() + }) + + it('does not gate non-GitHub providers on the GitHub rate limit', async () => { + getGitHubPRLookupRateLimitBlockMock.mockResolvedValue({ resetAt: 1_800_000_000 } as never) + getMergeRequestForBranchMock.mockResolvedValue(null) + + await expect( + getForgeProviderById('gitlab').getReviewForBranch({ + repoPath: '/repo', + connectionId: null, + branch: 'feature/x' + }) + ).resolves.toBeNull() + }) }) diff --git a/src/main/source-control/forge-provider.ts b/src/main/source-control/forge-provider.ts index d4967486c..19526ad71 100644 --- a/src/main/source-control/forge-provider.ts +++ b/src/main/source-control/forge-provider.ts @@ -21,7 +21,12 @@ import { getGiteaRepoSlug } from '../gitea/client' import { createGiteaPullRequest } from '../gitea/pull-request-creation' -import { createGitHubPullRequest, getPRForBranchOutcome, getRepoSlug } from '../github/client' +import { + createGitHubPullRequest, + getGitHubPRLookupRateLimitBlock, + getPRForBranchOutcome, + getRepoSlug +} from '../github/client' import { getMergeRequest, getMergeRequestForBranchOrThrow, getProjectSlug } from '../gitlab/client' import { createGitLabMergeRequest } from '../gitlab/merge-request-creation' import { @@ -122,6 +127,29 @@ function unwrapGitHubPRForBranchOutcome( return outcome.kind === 'found' ? mapGitHubReview(outcome.pr) : null } +/** + * Why (#11532): hosted-review lookups reach GitHub outside the PR refresh + * coordinator's paced queue, so they need the same rate-limit floor. Throwing + * (rather than returning null) keeps a low budget from reading as "no pull + * request" — callers preserve the last known review and back off. + */ +async function assertGitHubReviewRateLimitBudget( + input: ForgeProviderRepositoryContext +): Promise { + const block = await getGitHubPRLookupRateLimitBlock( + input.repoPath, + input.connectionId, + getHostedReviewLocalGitOptions(input) + ) + if (block) { + throw new Error( + `GitHub PR lookup failed (rate_limited): GitHub rate limit is low. Try again after ${new Date( + block.resetAt * 1000 + ).toLocaleTimeString()}.` + ) + } +} + const gitHubForgeProvider = { id: 'github', supportsReviewCreation: true, @@ -131,6 +159,7 @@ const gitHubForgeProvider = { resolveRepository: async (context) => getRepoSlug(context.repoPath, context.connectionId, ...hostedReviewExecutionArgs(context)), async getReviewForBranch(input) { + await assertGitHubReviewRateLimitBudget(input) const fallbackReviewNumber = input.linkedReviewNumber == null ? (input.fallbackReviewNumber ?? null) : null const executionArgs = hostedReviewExecutionArgs(input) @@ -149,6 +178,7 @@ const gitHubForgeProvider = { return unwrapGitHubPRForBranchOutcome(outcome) }, async getReviewByNumber(input) { + await assertGitHubReviewRateLimitBudget(input) const executionArgs = hostedReviewExecutionArgs(input) const outcome = executionArgs.length > 0 diff --git a/src/main/source-control/hosted-review-branch-cache.test.ts b/src/main/source-control/hosted-review-branch-cache.test.ts new file mode 100644 index 000000000..80d9220aa --- /dev/null +++ b/src/main/source-control/hosted-review-branch-cache.test.ts @@ -0,0 +1,374 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { HostedReviewInfo } from '../../shared/hosted-review' +import { + __resetHostedReviewBranchCacheForTests, + invalidateHostedReviewBranchCache, + withHostedReviewBranchCache +} from './hosted-review-branch-cache' + +const identity = { repoPath: '/repo', connectionId: null, branch: 'feature/x' } + +const openReview: HostedReviewInfo = { + provider: 'github', + number: 7, + title: 'Open PR', + state: 'open', + url: 'https://github.com/acme/orca/pull/7', + status: 'success', + updatedAt: '2026-07-31T00:00:00.000Z', + mergeable: 'MERGEABLE' +} + +const mergedReview: HostedReviewInfo = { ...openReview, state: 'merged' } + +describe('hosted review branch cache (#11532)', () => { + beforeEach(() => { + __resetHostedReviewBranchCacheForTests() + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('holds a no-review answer far longer than a poll interval', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + vi.setSystemTime(1_000_000 + 5 * 60_000) + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + + expect(lookup).toHaveBeenCalledTimes(1) + + vi.setSystemTime(1_000_000 + 15 * 60_000 + 1) + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('keeps the no-review answer while the branch head moves', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: 'aaa' }, lookup) + vi.setSystemTime(1_000_000 + 60_000) + await withHostedReviewBranchCache(identity, { headOid: 'bbb' }, lookup) + + // A commit is not evidence that a review was opened, so it must not defeat + // the long interval — that is what kept busy worktrees polling every minute. + expect(lookup).toHaveBeenCalledTimes(1) + }) + + it('refreshes a found review at the caller cadence', async () => { + const lookup = vi.fn(async () => openReview) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + vi.setSystemTime(1_000_000 + 30_000) + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(1) + + vi.setSystemTime(1_000_000 + 60_001) + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('drops a merged review once the inspected head moves off it', async () => { + const lookup = vi.fn(async () => mergedReview) + + await withHostedReviewBranchCache(identity, { headOid: 'aaa' }, lookup) + await withHostedReviewBranchCache(identity, { headOid: 'aaa' }, lookup) + expect(lookup).toHaveBeenCalledTimes(1) + + await withHostedReviewBranchCache(identity, { headOid: 'bbb' }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('collapses concurrent callers onto one lookup', async () => { + let resolveLookup: (value: HostedReviewInfo | null) => void = () => {} + const lookup = vi.fn( + () => + new Promise((resolve) => { + resolveLookup = resolve + }) + ) + + const first = withHostedReviewBranchCache(identity, { headOid: null }, lookup) + const second = withHostedReviewBranchCache(identity, { headOid: null }, lookup) + resolveLookup(openReview) + + await expect(first).resolves.toEqual(openReview) + await expect(second).resolves.toEqual(openReview) + expect(lookup).toHaveBeenCalledTimes(1) + }) + + it('separates lookups that differ only by linked review number', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache({ ...identity, linkedGitHubPR: 1 }, { headOid: null }, lookup) + await withHostedReviewBranchCache({ ...identity, linkedGitHubPR: 2 }, { headOid: null }, lookup) + + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('backs a failing branch off instead of re-asking every poll', async () => { + const lookup = vi.fn(async () => { + throw new Error('rate limited') + }) + + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).rejects.toThrow( + 'rate limited' + ) + vi.setSystemTime(1_000_000 + 30_000) + // Nothing cached, so the caller must still hear a failure — but no API call. + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).rejects.toThrow( + /backing off/ + ) + expect(lookup).toHaveBeenCalledTimes(1) + + vi.setSystemTime(1_000_000 + 60_001) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).rejects.toThrow( + 'rate limited' + ) + expect(lookup).toHaveBeenCalledTimes(2) + + // The second failure doubles the window. + vi.setSystemTime(1_000_000 + 60_001 + 60_000) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).rejects.toThrow( + /backing off/ + ) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('serves the last known review from the failure itself, not only the backoff', async () => { + const lookup = vi + .fn<() => Promise>() + .mockResolvedValueOnce(openReview) + .mockRejectedValueOnce(new Error('transient')) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + vi.setSystemTime(1_000_000 + 60_001) + // The review must not blink out on the first failure and reappear on the next + // poll once the backoff window is what serves it. + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + openReview + ) + + vi.setSystemTime(1_000_000 + 60_001 + 1_000) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + openReview + ) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('resets the escalation once a lookup succeeds', async () => { + const lookup = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error('first')) + .mockResolvedValueOnce(openReview) + .mockRejectedValueOnce(new Error('second')) + .mockResolvedValue(mergedReview) + + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).rejects.toThrow( + 'first' + ) + vi.setSystemTime(1_000_000 + 60_001) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + openReview + ) + + // The success clears the counter, so the next failure starts at the base + // window again rather than resuming a doubled one. That failure is served + // from the stale entry, but it still counts. + vi.setSystemTime(1_000_000 + 2 * 60_001) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + openReview + ) + vi.setSystemTime(1_000_000 + 3 * 60_001) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + mergedReview + ) + expect(lookup).toHaveBeenCalledTimes(4) + }) + + it('retires a cached no-review answer when Orca opens a review', async () => { + const lookup = vi + .fn<() => Promise>() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(openReview) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + invalidateHostedReviewBranchCache('/repo', null) + + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + openReview + ) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('discards a lookup that was already in flight when Orca opened a review', async () => { + let resolveLookup: (value: HostedReviewInfo | null) => void = () => {} + const lookup = vi + .fn<() => Promise>() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveLookup = resolve + }) + ) + .mockResolvedValue(openReview) + + const inflight = withHostedReviewBranchCache(identity, { headOid: null }, lookup) + invalidateHostedReviewBranchCache('/repo', null) + // The poll started before the review existed, so its "no review" answer is + // older than the invalidation and must not be cached back over it. + resolveLookup(null) + await expect(inflight).resolves.toBeNull() + + await expect(withHostedReviewBranchCache(identity, { headOid: null }, lookup)).resolves.toEqual( + openReview + ) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('leaves another repo in-flight lookup cacheable across an invalidation', async () => { + let resolveLookup: (value: HostedReviewInfo | null) => void = () => {} + const other = { ...identity, repoPath: '/other' } + const lookup = vi.fn<() => Promise>().mockImplementationOnce( + () => + new Promise((resolve) => { + resolveLookup = resolve + }) + ) + + const inflight = withHostedReviewBranchCache(other, { headOid: null }, lookup) + invalidateHostedReviewBranchCache('/repo', null) + resolveLookup(null) + await inflight + + await withHostedReviewBranchCache(other, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(1) + }) + + it('scopes invalidation to one repo', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + await withHostedReviewBranchCache( + { ...identity, repoPath: '/other' }, + { headOid: null }, + lookup + ) + expect(lookup).toHaveBeenCalledTimes(2) + + invalidateHostedReviewBranchCache('/other', null) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + await withHostedReviewBranchCache( + { ...identity, repoPath: '/other' }, + { headOid: null }, + lookup + ) + expect(lookup).toHaveBeenCalledTimes(3) + }) + + it('keeps SSH and local repos with the same path apart', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + await withHostedReviewBranchCache( + { ...identity, connectionId: 'ssh-1' }, + { headOid: null }, + lookup + ) + + expect(lookup).toHaveBeenCalledTimes(2) + }) + + describe('selected-worktree tier', () => { + it('re-checks the selected branch every minute while the card list waits', async () => { + const selected = vi.fn(async () => null) + const listed = vi.fn(async () => null) + const other = { ...identity, branch: 'feature/y' } + + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, selected) + await withHostedReviewBranchCache(other, { headOid: null }, listed) + + vi.setSystemTime(1_000_000 + 60_001) + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, selected) + await withHostedReviewBranchCache(other, { headOid: null }, listed) + + expect(selected).toHaveBeenCalledTimes(2) + expect(listed).toHaveBeenCalledTimes(1) + }) + + it('retires a cached no-review answer when a branch becomes the selection', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + vi.setSystemTime(1_000_000 + 1_000) + + // Selecting the worktree is the user asking whether a review exists yet. + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + + // Staying on it does not re-ask; the minute interval takes over. + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('keeps a found review when a branch becomes the selection', async () => { + const lookup = vi.fn(async () => openReview) + + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + vi.setSystemTime(1_000_000 + 1_000) + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, lookup) + + // Only the long no-review answer is worth spending a call to retire. + expect(lookup).toHaveBeenCalledTimes(1) + }) + + it('caps the fast tier so a caller cannot promote a whole list', async () => { + const lookup = vi.fn(async () => null) + const branchAt = (index: number) => ({ ...identity, branch: `feature/${index}` }) + + // One more claim than the cap allows, so the first claim is evicted. + for (let index = 0; index <= 8; index += 1) { + await withHostedReviewBranchCache(branchAt(index), { headOid: null, active: true }, lookup) + } + expect(lookup).toHaveBeenCalledTimes(9) + + vi.setSystemTime(1_000_000 + 60_001) + // The evicted branch is back on card pacing; the newest claim is not. + await withHostedReviewBranchCache(branchAt(0), { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(9) + await withHostedReviewBranchCache(branchAt(8), { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(10) + }) + + it('paces a card poll of the selected branch at the selection interval', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, lookup) + vi.setSystemTime(1_000_000 + 60_001) + + // Freshness is a property of the branch, not of which surface asked. + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + }) + + it('returns a lapsed selection to card pacing', async () => { + const lookup = vi.fn(async () => null) + + await withHostedReviewBranchCache(identity, { headOid: null, active: true }, lookup) + // Nothing re-asserted the selection for a full no-review interval. + vi.setSystemTime(1_000_000 + 15 * 60_000 + 1) + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + + vi.setSystemTime(1_000_000 + 15 * 60_000 + 1 + 60_001) + await withHostedReviewBranchCache(identity, { headOid: null }, lookup) + expect(lookup).toHaveBeenCalledTimes(2) + }) + }) +}) diff --git a/src/main/source-control/hosted-review-branch-cache.ts b/src/main/source-control/hosted-review-branch-cache.ts new file mode 100644 index 000000000..b1d54774c --- /dev/null +++ b/src/main/source-control/hosted-review-branch-cache.ts @@ -0,0 +1,306 @@ +import type { HostedReviewInfo } from '../../shared/hosted-review' +import { + ACTIVE_CLAIM_TTL_MS, + ACTIVE_REFRESH_INTERVAL_MS, + lookupBackoffDelayMs, + LOOKUP_BACKOFF_MAX_MS, + MAX_ACTIVE_BRANCHES, + NO_REVIEW_REFRESH_INTERVAL_MS +} from './hosted-review-refresh-pacing' + +/** + * Process-wide cache for branch review lookups (#11532). + * + * `hostedReview:forBranch` is polled by every desktop window, the mobile client + * and `orca serve` alike, and each one used to reach the provider directly. The + * host's API quota is per user, so the only place that can pace them together is + * here — the single funnel they all pass through. + * + * Pacing is tiered by what the user is looking at rather than applied flat: the + * selected worktree is O(1) and can afford a per-minute re-check, while the + * worktree list is O(N) and is what exhausts the budget. + */ + +// Why: a found review still refreshes at the callers' poll cadence; the cache +// exists to collapse concurrent clients, not to make review state go stale. +const FOUND_REVIEW_TTL_MS = 60_000 +const MAX_ENTRIES = 500 + +type CacheEntry = { + review: HostedReviewInfo | null + fetchedAt: number + headOid: string | null +} + +const entries = new Map() +const inflight = new Map>() +const failureBackoff = new Map() +/** Branches a caller reported as its current selection, least recent first. */ +const activeClaims = new Map() +/** Bumped per repo on invalidation so a lookup that predates it cannot store. */ +const scopeGenerations = new Map() + +// Why: NUL is the one byte a repo path or branch name cannot contain, so a +// scope prefix cannot straddle a component boundary — invalidating `/a/b` must +// not also flush the unrelated repo at `/a/b c`. +const KEY_SEPARATOR = '\0' + +export type HostedReviewBranchCacheIdentity = { + repoPath: string + connectionId?: string | null + branch: string + linkedGitHubPR?: number | null + fallbackGitHubPR?: number | null + linkedGitLabMR?: number | null + linkedBitbucketPR?: number | null + linkedAzureDevOpsPR?: number | null + linkedGiteaPR?: number | null + localGitExecOptions?: unknown +} + +export type HostedReviewBranchCacheOptions = { + /** The worktree's checked-out HEAD oid, for merged-at-head visibility. */ + headOid: string | null + /** Set by surfaces that only ever render the selected worktree. */ + active?: boolean +} + +/** Repo-scoped prefix so a single repo's entries can be dropped without a full flush. */ +function repoScope(repoPath: string, connectionId?: string | null): string { + return `${connectionId ?? ''}${KEY_SEPARATOR}${repoPath}` +} + +export function hostedReviewBranchCacheKey(identity: HostedReviewBranchCacheIdentity): string { + return [ + repoScope(identity.repoPath, identity.connectionId), + identity.branch, + // Each linked id selects a different lookup, so it belongs in the identity. + identity.linkedGitHubPR ?? '', + identity.fallbackGitHubPR ?? '', + identity.linkedGitLabMR ?? '', + identity.linkedBitbucketPR ?? '', + identity.linkedAzureDevOpsPR ?? '', + identity.linkedGiteaPR ?? '', + identity.localGitExecOptions ? JSON.stringify(identity.localGitExecOptions) : '' + ].join(KEY_SEPARATOR) +} + +/** + * Records the caller's current selection, reporting whether the branch was not + * already active. Claims are least-recently-used so the fast tier stays bounded + * no matter how many a client asserts. + */ +function noteActiveClaim(key: string): boolean { + const now = Date.now() + for (const [candidate, claimedAt] of activeClaims) { + if (now - claimedAt > ACTIVE_CLAIM_TTL_MS) { + activeClaims.delete(candidate) + } + } + const wasActive = activeClaims.has(key) + activeClaims.delete(key) + activeClaims.set(key, now) + while (activeClaims.size > MAX_ACTIVE_BRANCHES) { + const oldest = activeClaims.keys().next().value + if (oldest === undefined) { + break + } + activeClaims.delete(oldest) + } + return !wasActive +} + +function isActiveBranch(key: string): boolean { + const claimedAt = activeClaims.get(key) + return claimedAt !== undefined && Date.now() - claimedAt <= ACTIVE_CLAIM_TTL_MS +} + +// Why: a merged review is the one answer that depends on the inspected head — +// the merged-at-head carve-out keeps it visible only while the head matches. +// Negative answers are deliberately head-insensitive, so a branch under active +// commits cannot defeat the long no-review interval. +function isHeadSensitive(entry: CacheEntry): boolean { + return entry.review?.state === 'merged' +} + +function refreshIntervalMs(entry: CacheEntry, active: boolean): number { + if (entry.review !== null) { + return FOUND_REVIEW_TTL_MS + } + return active ? ACTIVE_REFRESH_INTERVAL_MS : NO_REVIEW_REFRESH_INTERVAL_MS +} + +function isFresh(entry: CacheEntry, headOid: string | null, active: boolean): boolean { + if (isHeadSensitive(entry) && headOid !== null && entry.headOid !== null) { + if (headOid !== entry.headOid) { + return false + } + } + return Date.now() - entry.fetchedAt < refreshIntervalMs(entry, active) +} + +function storeEntry(key: string, entry: CacheEntry): void { + entries.delete(key) + entries.set(key, entry) + while (entries.size > MAX_ENTRIES) { + const oldest = entries.keys().next().value + if (oldest === undefined) { + break + } + entries.delete(oldest) + } +} + +function backoffUntil(key: string): number | null { + // Why: a lapsed window keeps its failure count, otherwise the very act of + // retrying resets the escalation and the backoff never grows past the base. + const entry = failureBackoff.get(key) + return entry !== undefined && entry.until > Date.now() ? entry.until : null +} + +function noteFailure(key: string): void { + const now = Date.now() + for (const [candidate, entry] of failureBackoff) { + // Why: only counts that lapsed a full max window ago are stale enough to + // forget; anything more eager would undo the escalation above. + if (now - entry.until > LOOKUP_BACKOFF_MAX_MS) { + failureBackoff.delete(candidate) + } + } + const failures = (failureBackoff.get(key)?.failures ?? 0) + 1 + failureBackoff.delete(key) + failureBackoff.set(key, { until: now + lookupBackoffDelayMs(failures), failures }) + while (failureBackoff.size > MAX_ENTRIES) { + const oldest = failureBackoff.keys().next().value + if (oldest === undefined) { + break + } + failureBackoff.delete(oldest) + } +} + +function scopeGeneration(scope: string): number { + return scopeGenerations.get(scope) ?? 0 +} + +function bumpScopeGeneration(scope: string): void { + const next = scopeGeneration(scope) + 1 + scopeGenerations.delete(scope) + scopeGenerations.set(scope, next) + // Why: an evicted scope reads as generation 0, which only makes a lookup in + // flight at eviction discard its result — a wasted call, never a stale one. + while (scopeGenerations.size > MAX_ENTRIES) { + const oldest = scopeGenerations.keys().next().value + if (oldest === undefined) { + break + } + scopeGenerations.delete(oldest) + } +} + +/** + * Drops every cached answer for a repo. Called when Orca itself opens a review, + * so the new one is visible immediately instead of after the no-review interval. + */ +export function invalidateHostedReviewBranchCache( + repoPath: string, + connectionId?: string | null +): void { + const scope = repoScope(repoPath, connectionId) + bumpScopeGeneration(scope) + const prefix = `${scope}${KEY_SEPARATOR}` + for (const key of entries.keys()) { + if (key.startsWith(prefix)) { + entries.delete(key) + } + } + for (const key of failureBackoff.keys()) { + if (key.startsWith(prefix)) { + failureBackoff.delete(key) + } + } +} + +/** @internal - exposed for tests only */ +export function __resetHostedReviewBranchCacheForTests(): void { + entries.clear() + inflight.clear() + failureBackoff.clear() + activeClaims.clear() + scopeGenerations.clear() +} + +/** + * Serves `lookup` through the shared cache: a fresh answer is reused, concurrent + * callers share one in-flight lookup, and a failing branch backs off instead of + * being re-asked at every caller's poll cadence. + */ +export async function withHostedReviewBranchCache( + identity: HostedReviewBranchCacheIdentity, + options: HostedReviewBranchCacheOptions, + lookup: () => Promise +): Promise { + const key = hostedReviewBranchCacheKey(identity) + const headOid = options.headOid + if (options.active === true && noteActiveClaim(key) && entries.get(key)?.review === null) { + // Why: switching to a worktree is the user asking whether a review exists + // yet, so the long no-review interval must not answer on their behalf. This + // is the cheap half of the fast tier — it costs one lookup per selection + // rather than one per minute. + entries.delete(key) + } + const active = isActiveBranch(key) + + const cached = entries.get(key) + if (cached && isFresh(cached, headOid, active)) { + return cached.review + } + + const pending = inflight.get(key) + if (pending) { + return pending + } + + const until = backoffUntil(key) + if (until !== null) { + // Why: a stale answer beats an error card, but with nothing cached the + // caller must hear the failure rather than read it as "no review". + if (cached) { + return cached.review + } + throw new Error( + `Hosted review lookup is backing off after repeated failures. Retrying after ${new Date( + until + ).toLocaleTimeString()}.` + ) + } + + const scope = repoScope(identity.repoPath, identity.connectionId) + const request = (async () => { + const generation = scopeGeneration(scope) + try { + const review = await lookup() + // Why: a review created while this lookup was out makes its answer older + // than the invalidation; storing it would re-pin the stale "no review". + if (generation === scopeGeneration(scope)) { + storeEntry(key, { review, fetchedAt: Date.now(), headOid }) + failureBackoff.delete(key) + } + return review + } catch (error) { + noteFailure(key) + // Why: the last good review beats an error card here just as it does on + // the backed-off path — otherwise it blinks out on the first failure. + // An invalidation drops the entry, so this cannot revive a retired answer. + const stale = entries.get(key) + if (stale) { + return stale.review + } + throw error + } finally { + inflight.delete(key) + } + })() + inflight.set(key, request) + return request +} diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index 43de5f425..83df9010d 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -403,7 +403,10 @@ describe('createHostedReview', () => { expect.objectContaining({ repoPath: '/repo', branch: 'feature', - localGitExecOptions: { wslDistro: 'Ubuntu' } + localGitExecOptions: { wslDistro: 'Ubuntu' }, + // Why: a stale no-review answer here would leave Create enabled after a + // review was opened outside Orca, so eligibility takes the fast tier. + active: true }) ) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 75e3189f4..f28c319f3 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -36,6 +36,7 @@ import { } from '../gitlab/gl-utils' import { getSshGitProvider } from '../providers/ssh-git-dispatch' import { detectHostedReviewProvider, getForgeProviderForRepository } from './forge-provider' +import { invalidateHostedReviewBranchCache } from './hosted-review-branch-cache' import { getHostedReviewForBranch } from './hosted-review' import { getHostedReviewLocalGitOptions, @@ -503,6 +504,10 @@ export async function getHostedReviewCreationEligibility( linkedAzureDevOpsPR: args.linkedAzureDevOpsPR ?? null, linkedGiteaPR: args.linkedGiteaPR ?? null, connectionId: args.connectionId ?? null, + // Why: eligibility is only ever asked for the worktree the user is acting + // on, so it earns the fast tier. Without it a review opened outside Orca + // in the last no-review interval would leave Create enabled (#11532). + active: true, ...hostedReviewExecutionContext(args) }) } catch (error) { @@ -624,7 +629,14 @@ export async function createHostedReview( return blocked } const localGitOptions = getHostedReviewLocalGitOptions(options) - return Object.keys(localGitOptions).length > 0 - ? provider.createReview(repoPath, input, connectionId, options) - : provider.createReview(repoPath, input, connectionId) + const result = + Object.keys(localGitOptions).length > 0 + ? await provider.createReview(repoPath, input, connectionId, options) + : await provider.createReview(repoPath, input, connectionId) + if (result.ok) { + // Why (#11532): the branch cache holds a "no review" answer for far longer + // than a poll interval, so Orca's own creation must retire it at once. + invalidateHostedReviewBranchCache(repoPath, connectionId) + } + return result } diff --git a/src/main/source-control/hosted-review-refresh-pacing.ts b/src/main/source-control/hosted-review-refresh-pacing.ts new file mode 100644 index 000000000..7c2eaf3c6 --- /dev/null +++ b/src/main/source-control/hosted-review-refresh-pacing.ts @@ -0,0 +1,42 @@ +/** + * Shared pacing policy for hosted-review lookups (#11532). + * + * The PR refresh coordinator's queue and the `hostedReview:forBranch` entry + * point spend the same per-user API quota, so they must agree on how long an + * answer stays good and how hard to back off after a failure. Keeping the + * numbers here stops the two paths from drifting apart. + */ + +/** A branch with no review only gains one when a review is opened, which is not a per-minute event. */ +export const NO_REVIEW_REFRESH_INTERVAL_MS = 15 * 60_000 + +/** + * The worktree the user has selected is re-checked at the old cadence: a review + * opened in a browser should show up while they are still looking at the tab. + */ +export const ACTIVE_REFRESH_INTERVAL_MS = 60_000 + +/** + * Why: the fast tier is only affordable because it is O(1) — one selected + * worktree per client. Capping it here means a caller that wrongly marks a whole + * list active costs stale cards, not the 5,000/hr budget. + */ +export const MAX_ACTIVE_BRANCHES = 8 + +/** A selection nobody has re-asserted for a whole no-review interval is not current. */ +export const ACTIVE_CLAIM_TTL_MS = NO_REVIEW_REFRESH_INTERVAL_MS + +export const LOOKUP_BACKOFF_BASE_MS = 60_000 +export const LOOKUP_BACKOFF_MAX_MS = 15 * 60_000 + +// Why: capped so a long-lived failure settles at LOOKUP_BACKOFF_MAX_MS rather +// than overflowing the exponent. +const MAX_BACKOFF_DOUBLINGS = 4 + +/** Exponential backoff delay for the nth consecutive lookup failure (1-based). */ +export function lookupBackoffDelayMs(failures: number): number { + return Math.min( + LOOKUP_BACKOFF_MAX_MS, + LOOKUP_BACKOFF_BASE_MS * 2 ** Math.min(Math.max(failures, 1) - 1, MAX_BACKOFF_DOUBLINGS) + ) +} diff --git a/src/main/source-control/hosted-review.test.ts b/src/main/source-control/hosted-review.test.ts index 102ebfebd..024ecff71 100644 --- a/src/main/source-control/hosted-review.test.ts +++ b/src/main/source-control/hosted-review.test.ts @@ -1,4 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { getHostedReviewForBranch } from './hosted-review' +import { __resetHostedReviewBranchCacheForTests } from './hosted-review-branch-cache' const { getProjectSlugMock, @@ -36,6 +38,7 @@ vi.mock('../gitlab/client', () => ({ vi.mock('../github/client', () => ({ getRepoSlug: getRepoSlugMock, getPRForBranchOutcome: getPRForBranchOutcomeMock, + getGitHubPRLookupRateLimitBlock: vi.fn(async () => null), createGitHubPullRequest: vi.fn() })) @@ -66,8 +69,6 @@ vi.mock('../gitea/client', () => ({ getGiteaPullRequest: vi.fn() })) -import { getHostedReviewForBranch } from './hosted-review' - describe('getHostedReviewForBranch', () => { beforeEach(() => { getProjectSlugMock.mockReset() @@ -80,6 +81,9 @@ describe('getHostedReviewForBranch', () => { getAzureDevOpsPullRequestForBranchMock.mockReset() getGiteaRepoSlugMock.mockReset() getGiteaPullRequestForBranchMock.mockReset() + // The branch cache is process-wide, so one test's answer would otherwise + // satisfy the next one's lookup. + __resetHostedReviewBranchCacheForTests() }) it('maps GitLab merge requests into the hosted review surface', async () => { diff --git a/src/main/source-control/hosted-review.ts b/src/main/source-control/hosted-review.ts index fbc5a71a5..50c68870b 100644 --- a/src/main/source-control/hosted-review.ts +++ b/src/main/source-control/hosted-review.ts @@ -1,5 +1,6 @@ import type { HostedReviewInfo } from '../../shared/hosted-review' import { getForgeProviderForRepository, type ForgeProviderId } from './forge-provider' +import { withHostedReviewBranchCache } from './hosted-review-branch-cache' import type { HostedReviewExecutionOptions } from './hosted-review-git-options' function reviewLinkForProvider( @@ -35,6 +36,11 @@ export async function getHostedReviewForBranch( linkedAzureDevOpsPR?: number | null linkedGiteaPR?: number | null currentHeadOid?: string | null + /** + * Set by surfaces that only ever render the selected worktree, which is the + * one branch cheap enough to re-check per minute (#11532). + */ + active?: boolean } & HostedReviewExecutionOptions ): Promise { const branchName = input.branch.replace(/^refs\/heads\//, '') @@ -52,20 +58,29 @@ export async function getHostedReviewForBranch( return null } - const provider = await getForgeProviderForRepository({ - repoPath: input.repoPath, - connectionId: input.connectionId, - ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}) - }) - if (!provider) { - return null - } - return provider.getReviewForBranch({ - repoPath: input.repoPath, - connectionId: input.connectionId, - branch: branchName, - ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}), - githubCurrentHeadOid: input.currentHeadOid ?? null, - ...reviewLinkForProvider(input, provider.id) - }) + const headOid = input.currentHeadOid?.trim() || null + // Why (#11532): every client polls this one entry point, and they share the + // host's per-user API quota, so the cache has to sit above the provider call. + return withHostedReviewBranchCache( + { ...input, branch: branchName }, + { headOid, ...(input.active === true ? { active: true } : {}) }, + async () => { + const provider = await getForgeProviderForRepository({ + repoPath: input.repoPath, + connectionId: input.connectionId, + ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}) + }) + if (!provider) { + return null + } + return provider.getReviewForBranch({ + repoPath: input.repoPath, + connectionId: input.connectionId, + branch: branchName, + ...(input.localGitExecOptions ? { localGitExecOptions: input.localGitExecOptions } : {}), + githubCurrentHeadOid: headOid, + ...reviewLinkForProvider(input, provider.id) + }) + } + ) } diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index ac5ad3242..30fade224 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -1400,7 +1400,10 @@ export default function ChecksPanel(): React.JSX.Element { linkedBitbucketPR, linkedAzureDevOpsPR, linkedGiteaPR, - staleWhileRevalidate: true + staleWhileRevalidate: true, + // Why: this panel only ever renders the selected worktree, so it earns + // the host's fast re-check tier (#11532). + active: true }) // Why: the gh-based refresh coordinator is GitHub-only; running it elsewhere gave a spurious gh_unavailable error hiding a valid composer. if (activeWorktreeId && isGitHubReviewContext) { diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 6f5a4cc5f..a579b4ff8 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -1721,7 +1721,10 @@ function SourceControlInner(): React.JSX.Element { linkedBitbucketPR, linkedAzureDevOpsPR, linkedGiteaPR, - staleWhileRevalidate: true + staleWhileRevalidate: true, + // Why: scoped to the active worktree, so it earns the host's fast + // re-check tier instead of the O(N) card pacing (#11532). + active: true }) // Why: keep the GitHub cache refresh behind the coordinator so Source Control doesn't bypass pacing. enqueueGitHubPRRefresh(activeWorktreeId, 'swr', 30) diff --git a/src/renderer/src/store/slices/hosted-review.ts b/src/renderer/src/store/slices/hosted-review.ts index 596c52b95..885425c4b 100644 --- a/src/renderer/src/store/slices/hosted-review.ts +++ b/src/renderer/src/store/slices/hosted-review.ts @@ -32,6 +32,11 @@ type FetchOptions = { repoId?: string staleWhileRevalidate?: boolean currentHeadOid?: string | null + /** + * Pass from surfaces that only render the selected worktree. The host re-checks + * that branch per minute and paces the O(N) card list far slower (#11532). + */ + active?: boolean } type CreateHostedReviewStoreInput = CreateHostedReviewInput & { repoId?: string | null } @@ -387,6 +392,7 @@ export const createHostedReviewSlice: StateCreator