From cf5e0872466d94ab104fdbef2dc736e94a885bbd Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 22:13:01 -0700 Subject: [PATCH] fix(github): bound PR-refresh alias fan-out and hosted-review cache growth (#16943) * wip: memory-growth * fix(github): preserve newer refresh candidates --- .../pr-refresh-queue-growth-bound.test.ts | 281 ++++++++++++++++++ src/main/github/pr-refresh-queue.ts | 92 +++++- .../src/store/github/pr-result-routing.ts | 39 +-- ...r-refresh-hosted-review-cache-leak.test.ts | 101 +++++++ 4 files changed, 484 insertions(+), 29 deletions(-) create mode 100644 src/main/github/pr-refresh-queue-growth-bound.test.ts create mode 100644 src/renderer/src/store/slices/github-pr-refresh-hosted-review-cache-leak.test.ts diff --git a/src/main/github/pr-refresh-queue-growth-bound.test.ts b/src/main/github/pr-refresh-queue-growth-bound.test.ts new file mode 100644 index 00000000000..9bd4c6e19bf --- /dev/null +++ b/src/main/github/pr-refresh-queue-growth-bound.test.ts @@ -0,0 +1,281 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitHubPRRefreshCandidate } from '../../shared/github/pull-request-refresh-types' + +const { coordinatorMocks, moduleMocks } = await vi.hoisted(async () => { + const moduleMocks = await import('./pr-refresh-coordinator-test-mocks') + return { coordinatorMocks: moduleMocks.createPRRefreshCoordinatorMocks(), moduleMocks } +}) + +vi.mock('electron', () => moduleMocks.electronModuleMock(coordinatorMocks)) +vi.mock('./client', () => moduleMocks.clientModuleMock(coordinatorMocks)) +vi.mock('./github-api-repository', () => + moduleMocks.githubApiRepositoryModuleMock(coordinatorMocks) +) +vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(coordinatorMocks)) +vi.mock('../ipc/ui', () => moduleMocks.ipcUiModuleMock(coordinatorMocks)) + +import { makeCandidate, makePR } from './pr-refresh-coordinator-test-harness' + +const { getPRForBranchOutcomeMock } = coordinatorMocks +const WORKTREES = 20 +const LINKED_PR_KEY = 'local::runtime:host::/repo::pr::42' + +function visibleCandidates(): GitHubPRRefreshCandidate[] { + return Array.from({ length: WORKTREES }, (_, i) => + makeCandidate({ + cacheKey: `/repo::feature/${i}`, + branch: `feature/${i}`, + worktreeId: `wt-${i}`, + cachedFetchedAt: null + }) + ) +} + +describe('pr-refresh queue growth bounds', () => { + beforeEach(() => { + moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks) + getPRForBranchOutcomeMock.mockImplementation(async () => ({ + kind: 'found' as const, + pr: makePR({ checksStatus: 'pending' as const }), + fetchedAt: Date.now() + })) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('keeps the queue bounded across repeated visible reports and background pauses', async () => { + const { reportVisiblePRRefreshCandidates, _getPRRefreshQueueSizeForTests } = + await import('./pr-refresh-coordinator') + + for (let cycle = 0; cycle < 200; cycle += 1) { + reportVisiblePRRefreshCandidates(visibleCandidates(), cycle + 1, 1) + await vi.advanceTimersByTimeAsync(30_000) + } + // 200 report cycles * 20 candidates = 4000 enqueues; the queue coalesces to one entry per key. + expect(_getPRRefreshQueueSizeForTests()).toBeLessThanOrEqual(WORKTREES) + }) + + it('keeps visibility windows and error backoff bounded when renderer windows churn', async () => { + const { + reportVisiblePRRefreshCandidates, + _getVisiblePRRefreshWindowCountForTests, + _getPRRefreshErrorBackoffCountForTests + } = await import('./pr-refresh-coordinator') + + getPRForBranchOutcomeMock.mockImplementation(async () => ({ + kind: 'upstream-error' as const, + errorType: 'unknown' as const, + message: 'boom', + fetchedAt: Date.now() + })) + + for (let windowId = 1; windowId <= 300; windowId += 1) { + coordinatorMocks.getAllWebContentsMock.mockReturnValue([ + { id: windowId, isDestroyed: () => false, send: coordinatorMocks.sendMock } + ]) + reportVisiblePRRefreshCandidates(visibleCandidates(), 1, windowId) + await vi.advanceTimersByTimeAsync(60_000) + } + expect(_getVisiblePRRefreshWindowCountForTests()).toBeLessThanOrEqual(1) + expect(_getPRRefreshErrorBackoffCountForTests()).toBeLessThanOrEqual(WORKTREES) + }) + + it("drops a worktree's stale branch aliases instead of fanning out to its whole history", async () => { + const { + reportVisiblePRRefreshCandidates, + _getPRRefreshAliasCountForTests, + _getPRRefreshQueueSizeForTests + } = await import('./pr-refresh-coordinator') + + // A linked-PR refreshKey ignores the branch, so one entry survives every + // branch switch, and each switch used to add an alias that only the next + // drain could shed — so the fan-out tracked branch churn instead of the one + // live branch. (Draining between switches caps the unfixed count in the low + // single digits here; the parked-entry case below is where it really piles up.) + for (let i = 0; i < 300; i += 1) { + reportVisiblePRRefreshCandidates( + [ + makeCandidate({ + cacheKey: `/repo::churn/${i}`, + branch: `churn/${i}`, + worktreeId: 'wt-churn', + linkedPRNumber: 42, + cachedFetchedAt: null + }) + ], + i + 1, + 1 + ) + await vi.advanceTimersByTimeAsync(20_000) + } + + expect(_getPRRefreshQueueSizeForTests()).toBe(1) + expect(_getPRRefreshAliasCountForTests(LINKED_PR_KEY)).toBe(1) + const broadcastAliasCounts = coordinatorMocks.sendMock.mock.calls + .filter((call) => call[0] === 'gh:prRefreshEvent') + .map((call) => (call[1] as { aliases: unknown[] }).aliases.length) + expect(Math.max(...broadcastAliasCounts)).toBe(1) + }) + + it('keeps the newer alias when a branch switch lands mid-request', async () => { + const { reportVisiblePRRefreshCandidates, _getPRRefreshAliasCountForTests } = + await import('./pr-refresh-coordinator') + + const churnCandidate = (index: number): GitHubPRRefreshCandidate => + makeCandidate({ + cacheKey: `/repo::churn/${index}`, + branch: `churn/${index}`, + worktreeId: 'wt-churn', + linkedPRNumber: 42, + cachedFetchedAt: null + }) + + let switched = false + getPRForBranchOutcomeMock.mockImplementation(async () => { + if (!switched) { + switched = true + // The entry is already out of the queue here, so this re-enqueue creates the + // entry the in-flight request's stale follow-up aliases merge back into. + reportVisiblePRRefreshCandidates([churnCandidate(2)], 2, 1) + } + return { + kind: 'found' as const, + pr: makePR({ checksStatus: 'pending' as const }), + fetchedAt: Date.now() + } + }) + + reportVisiblePRRefreshCandidates([churnCandidate(1)], 1, 1) + await vi.advanceTimersByTimeAsync(120_000) + + expect(_getPRRefreshAliasCountForTests(LINKED_PR_KEY)).toBe(1) + const broadcasts = coordinatorMocks.sendMock.mock.calls.filter( + (call) => call[0] === 'gh:prRefreshEvent' + ) as [string, { aliases: { cacheKey: string }[] }][] + const lastBroadcast = broadcasts.at(-1) + expect(lastBroadcast?.[1].aliases.map((alias) => alias.cacheKey)).toEqual(['/repo::churn/2']) + }) + + it('does not restore an older candidate over a fresher branch switch', async () => { + const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator') + const first = makeCandidate({ + cacheKey: '/repo::churn/1', + branch: 'churn/1', + worktreeId: 'wt-churn', + linkedPRNumber: 42, + cachedFetchedAt: null + }) + const switched = makeCandidate({ + cacheKey: '/repo::churn/2', + branch: 'churn/2', + worktreeId: 'wt-churn', + linkedPRNumber: 42, + cachedFetchedAt: Date.now(), + cachedHasPR: true, + cachedPRState: 'open', + cachedChecksStatus: 'success' + }) + + let didSwitch = false + getPRForBranchOutcomeMock.mockImplementation(async () => { + if (!didSwitch) { + didSwitch = true + reportVisiblePRRefreshCandidates([switched], 2, 1) + } + return { + kind: 'found' as const, + pr: makePR({ checksStatus: 'pending' as const }), + fetchedAt: Date.now() + } + }) + + reportVisiblePRRefreshCandidates([first], 1, 1) + await vi.advanceTimersByTimeAsync(100_000) + + expect(getPRForBranchOutcomeMock.mock.calls.map((call) => call[1])).toEqual(['churn/1']) + await vi.advanceTimersByTimeAsync(500_001) + expect(getPRForBranchOutcomeMock.mock.calls.map((call) => call[1])).toEqual([ + 'churn/1', + 'churn/2' + ]) + }) + + it('keeps distinct worktrees sharing one linked-PR key in the fan-out', async () => { + const { enqueuePRRefresh, _getPRRefreshAliasCountForTests } = + await import('./pr-refresh-coordinator') + + for (let i = 0; i < 4; i += 1) { + enqueuePRRefresh( + makeCandidate({ + cacheKey: `/repo::shared/${i}`, + branch: `shared/${i}`, + worktreeId: `wt-${i}`, + linkedPRNumber: 42, + cachedFetchedAt: Date.now() + 60_000 + }), + 'visible', + 40, + 1 + ) + } + expect(_getPRRefreshAliasCountForTests(LINKED_PR_KEY)).toBe(4) + }) + + it('coalesces branch churn into one alias while the entry waits to drain', async () => { + const { enqueuePRRefresh, _getPRRefreshAliasCountForTests, _getPRRefreshQueueSizeForTests } = + await import('./pr-refresh-coordinator') + + // No timer advance: the entry stays queued, so every enqueue lands on the same + // alias map and the coalescing bound is what keeps it from growing. + for (let i = 0; i < 200; i += 1) { + enqueuePRRefresh( + makeCandidate({ + cacheKey: `/repo::churn/${i}`, + branch: `churn/${i}`, + worktreeId: 'wt-churn', + linkedPRNumber: 42, + cachedFetchedAt: null + }), + 'swr', + 10, + 1 + ) + } + expect(_getPRRefreshQueueSizeForTests()).toBe(1) + expect(_getPRRefreshAliasCountForTests(LINKED_PR_KEY)).toBe(1) + + await vi.advanceTimersByTimeAsync(60_000) + const outcomeAliases = coordinatorMocks.sendMock.mock.calls + .filter((call) => call[0] === 'gh:prRefreshEvent') + .map((call) => (call[1] as { aliases: { cacheKey: string }[] }).aliases) + expect(outcomeAliases.every((aliases) => aliases.length === 1)).toBe(true) + expect(outcomeAliases.at(-1)?.[0].cacheKey).toBe('/repo::churn/199') + }) + + it('keeps a manually refreshed worktree to one alias across branch churn', async () => { + const { reportVisiblePRRefreshCandidates, refreshPRNow, _getPRRefreshAliasCountForTests } = + await import('./pr-refresh-coordinator') + + const churnCandidate = (index: number): GitHubPRRefreshCandidate => + makeCandidate({ + cacheKey: `/repo::churn/${index}`, + branch: `churn/${index}`, + worktreeId: 'wt-churn', + linkedPRNumber: 42, + cachedFetchedAt: null + }) + + // A manual refresh merges its alias into its own copy of the entry's map and + // writes that map back, so the coalescing bound has to hold on re-entry too. + reportVisiblePRRefreshCandidates([churnCandidate(0)], 1, 1) + await vi.advanceTimersByTimeAsync(1_000) + for (let i = 1; i <= 30; i += 1) { + await refreshPRNow(churnCandidate(i)) + await vi.advanceTimersByTimeAsync(1) + } + + expect(_getPRRefreshAliasCountForTests(LINKED_PR_KEY)).toBe(1) + }) +}) diff --git a/src/main/github/pr-refresh-queue.ts b/src/main/github/pr-refresh-queue.ts index 2cc185535fa..ef2a706b589 100644 --- a/src/main/github/pr-refresh-queue.ts +++ b/src/main/github/pr-refresh-queue.ts @@ -32,6 +32,80 @@ export type PRRefreshEnqueue = { coalesced: boolean } +/** A worktree has one live branch at a time, so a second cacheKey for it is a + * branch it moved off. Drop those: a linked-PR key survives every branch + * switch, so while its entry is parked (rate-limit pause, error backoff, or + * just waiting to drain) every switch used to add a cacheKey that the drain + * snapshot then carries forward, and each dead one costs IPC payload and a + * renderer cache write on every later broadcast. */ +function setLiveAlias( + aliases: Map, + alias: GitHubPRRefreshAlias +): void { + if (alias.worktreeId) { + for (const [cacheKey, existing] of aliases) { + if (cacheKey !== alias.cacheKey && existing.worktreeId === alias.worktreeId) { + aliases.delete(cacheKey) + } + } + } + aliases.set(alias.cacheKey, alias) +} + +/** Follow-up aliases were captured before the request ran, so anything enqueued + * while it was in flight is newer. Return that preserved alias so callers do + * not pair it with the stale request candidate. */ +function mergeFollowUpAlias( + aliases: Map, + alias: GitHubPRRefreshAlias +): GitHubPRRefreshAlias | undefined { + if (alias.worktreeId) { + for (const existing of aliases.values()) { + if (existing.worktreeId === alias.worktreeId) { + return existing + } + } + } + setLiveAlias(aliases, alias) + return undefined +} + +function sameAliasRequestIdentity( + left: GitHubPRRefreshAlias, + right: GitHubPRRefreshAlias +): boolean { + return ( + left.cacheKey === right.cacheKey && + left.repoId === right.repoId && + left.repoPath === right.repoPath && + left.branch === right.branch && + left.worktreeId === right.worktreeId && + left.connectionId === right.connectionId && + left.executionHostId === right.executionHostId && + left.linkedPRNumber === right.linkedPRNumber && + left.fallbackPRNumber === right.fallbackPRNumber && + left.fallbackPRSource === right.fallbackPRSource && + left.currentHeadOid === right.currentHeadOid + ) +} + +/** A manual refresh merges its alias into its own copy of the map and writes it + * back, so re-entry through `set` has to re-apply the same bound; later + * insertions are the newer branch and win. */ +function dropSupersededWorktreeAliases(aliases: Map): void { + const liveCacheKeyByWorktree = new Map() + for (const [cacheKey, alias] of aliases) { + if (!alias.worktreeId) { + continue + } + const superseded = liveCacheKeyByWorktree.get(alias.worktreeId) + if (superseded !== undefined) { + aliases.delete(superseded) + } + liveCacheKeyByWorktree.set(alias.worktreeId, cacheKey) + } +} + export class PRRefreshQueue { private readonly entries = new Map() private order = 0 @@ -47,6 +121,7 @@ export class PRRefreshQueue { } set(key: string, entry: PRRefreshQueueEntry): void { + dropSupersededWorktreeAliases(entry.aliases) this.entries.set(key, entry) } @@ -92,7 +167,7 @@ export class PRRefreshQueue { return { alias, key, dueAt, coalesced: false } } - existing.aliases.set(alias.cacheKey, alias) + setLiveAlias(existing.aliases, alias) const shouldPromote = priority > existing.priority || reason === 'manual' || @@ -190,20 +265,29 @@ export class PRRefreshQueue { setVisibleFollowUp(entry: PRRefreshQueueEntry): void { const existing = this.entries.get(entry.key) if (!existing) { - this.entries.set(entry.key, entry) + this.set(entry.key, entry) return } + let candidateSuperseded = false for (const alias of entry.aliases.values()) { - existing.aliases.set(alias.cacheKey, alias) + const preserved = mergeFollowUpAlias(existing.aliases, alias) + if ( + preserved && + alias.worktreeId === entry.candidate.worktreeId && + !sameAliasRequestIdentity(preserved, alias) + ) { + candidateSuperseded = true + } } if ( + candidateSuperseded || bypassesFreshnessDelay(existing.reason) || existing.priority > entry.priority || existing.dueAt <= entry.dueAt ) { return } - this.entries.set(entry.key, { ...entry, aliases: existing.aliases }) + this.set(entry.key, { ...entry, aliases: existing.aliases }) } ordered( diff --git a/src/renderer/src/store/github/pr-result-routing.ts b/src/renderer/src/store/github/pr-result-routing.ts index 8efd6e43b54..80f4a76bd20 100644 --- a/src/renderer/src/store/github/pr-result-routing.ts +++ b/src/renderer/src/store/github/pr-result-routing.ts @@ -5,6 +5,10 @@ import { getHostedReviewCacheKey, linkedReviewHintKey } from '../slices/hosted-review-cache-identity' +import { + hasNewerHostedReviewCacheEntry, + withHostedReviewCacheEntry +} from '../slices/hosted-review-cache-state' import type { GitHubPRFallbackSource } from './cache-model' export function githubHostedReviewFallbackPRNumber( @@ -67,20 +71,6 @@ export function linkedReviewHintKeyForNoGitHubPR( return entry?.linkedReviewHintKey } -export function hasNewerHostedReviewCacheEntry( - cache: AppState['hostedReviewCache'], - cacheKey: string, - requestStartedAt: number, - requestStartedEntry: AppState['hostedReviewCache'][string] | undefined -): boolean { - const entry = cache[cacheKey] - return ( - entry !== undefined && - (entry.fetchedAt > requestStartedAt || - (entry.fetchedAt === requestStartedAt && entry !== requestStartedEntry)) - ) -} - export function syncHostedReviewCacheFromGitHubPRResult(args: { cache: AppState['hostedReviewCache'] repoPath: string @@ -155,18 +145,17 @@ export function syncHostedReviewCacheFromGitHubPRResult(args: { hostedReviewEntry?.branchLookupGitHubPRNumber === args.pr.number) ? args.pr.number : undefined + // Why: the key embeds the branch, so this write path grows with every distinct + // (host, repo, branch) a session refreshes. Share the hosted-review slice's bound. return { - cache: { - ...args.cache, - [hostedReviewCacheKey]: { - data: args.pr ? hostedReviewInfoFromGitHubPRInfo(args.pr) : null, - fetchedAt: args.fetchedAt, - linkedReviewHintKey: args.pr - ? linkedReviewHintKey({ linkedGitHubPR: args.pr.number }) - : linkedReviewHintKeyForNoGitHubPR(hostedReviewEntry), - ...(branchLookupGitHubPRNumber !== undefined ? { branchLookupGitHubPRNumber } : {}) - } - }, + cache: withHostedReviewCacheEntry(args.cache, hostedReviewCacheKey, { + data: args.pr ? hostedReviewInfoFromGitHubPRInfo(args.pr) : null, + fetchedAt: args.fetchedAt, + linkedReviewHintKey: args.pr + ? linkedReviewHintKey({ linkedGitHubPR: args.pr.number }) + : linkedReviewHintKeyForNoGitHubPR(hostedReviewEntry), + ...(branchLookupGitHubPRNumber !== undefined ? { branchLookupGitHubPRNumber } : {}) + }), accepted: true } } diff --git a/src/renderer/src/store/slices/github-pr-refresh-hosted-review-cache-leak.test.ts b/src/renderer/src/store/slices/github-pr-refresh-hosted-review-cache-leak.test.ts new file mode 100644 index 00000000000..e7a3062fd42 --- /dev/null +++ b/src/renderer/src/store/slices/github-pr-refresh-hosted-review-cache-leak.test.ts @@ -0,0 +1,101 @@ +/** + * Memory-leak regression: hostedReviewCache must stay bounded on the GitHub + * PR-refresh write path. + * + * `withHostedReviewCacheEntry` caps the map at HOSTED_REVIEW_CACHE_MAX, but the + * GitHub refresh path writes through `syncHostedReviewCacheFromGitHubPRResult`, + * which spreads a new key in without ever applying that cap. The key embeds the + * branch, so the map grows with every distinct (host, repo, branch) tuple a + * session ever refreshes. + */ +import { describe, it, expect, vi } from 'vitest' +import { create } from 'zustand' +import { createGitHubSlice } from './github' +import { createHostedReviewSlice } from './hosted-review' +import type { AppState } from '../types' +import type { GitHubPRRefreshEvent } from '../../../../shared/github/pull-request-refresh-types' + +const MAX_ENTRIES = 500 + +const mockApi = { + gh: { + prForBranch: vi.fn().mockResolvedValue(null), + refreshPRNow: vi.fn(), + enqueuePRRefresh: vi.fn().mockResolvedValue(undefined), + issue: vi.fn().mockResolvedValue(null), + prChecks: vi.fn().mockResolvedValue([]) + }, + hostedReview: { forBranch: vi.fn().mockResolvedValue(null) }, + runtimeEnvironments: { call: vi.fn() }, + cache: { + getGitHub: vi.fn().mockResolvedValue(null), + setGitHub: vi.fn().mockResolvedValue(undefined) + } +} + +// @ts-expect-error -- minimal window.api stub for the slice under test +globalThis.window = { api: mockApi } + +function createTestStore() { + return create()( + (...a) => + ({ + ...createGitHubSlice(...a), + ...createHostedReviewSlice(...a) + }) as AppState + ) +} + +function foundEvent(branch: string, sequence: number): GitHubPRRefreshEvent { + return { + sequence, + reason: 'visible', + aliases: [ + { + cacheKey: `local::/repo::${branch}`, + repoId: '/repo', + repoPath: '/repo', + branch, + executionHostId: 'local' + } + ], + outcome: { + kind: 'found', + fetchedAt: sequence, + pr: { + number: 1, + title: 'pr', + state: 'open', + url: 'https://example.test/pr/1', + checksStatus: 'success', + updatedAt: new Date().toISOString(), + mergeable: 'MERGEABLE' + } + } + } +} + +describe('hostedReviewCache stays bounded on the PR-refresh write path', () => { + it('caps hostedReviewCache when driven past the cap by the real writer', () => { + const store = createTestStore() + const total = MAX_ENTRIES + 150 + for (let i = 0; i < total; i++) { + store.getState().applyGitHubPRRefreshEvent(foundEvent(`branch-${i}`, i + 1)) + } + const cache = store.getState().hostedReviewCache + expect(Object.keys(cache).length).toBeLessThanOrEqual(MAX_ENTRIES) + // Newest survives, oldest is evicted. + expect(cache[`local::/repo::branch-${total - 1}`]).toBeDefined() + expect(cache['local::/repo::branch-0']).toBeUndefined() + }) + + it('keeps every entry while under the cap', () => { + const store = createTestStore() + for (let i = 0; i < 10; i++) { + store.getState().applyGitHubPRRefreshEvent(foundEvent(`kept-${i}`, i + 1)) + } + const cache = store.getState().hostedReviewCache + expect(Object.keys(cache)).toHaveLength(10) + expect(cache['local::/repo::kept-0']).toBeDefined() + }) +})