fix(github): bound PR-refresh alias fan-out and hosted-review cache growth (#16943)

* wip: memory-growth

* fix(github): preserve newer refresh candidates

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)
+  })
+})

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<string, GitHubPRRefreshAlias>,
+  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<string, GitHubPRRefreshAlias>,
+  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<string, GitHubPRRefreshAlias>): void {
+  const liveCacheKeyByWorktree = new Map<string, string>()
+  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<string, PRRefreshQueueEntry>()
   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(

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
   }
 }

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<AppState>()(
+    (...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()
+  })
+})