diff --git a/src/renderer/src/store/slices/github-pr-refresh-sequences-leak.test.ts b/src/renderer/src/store/slices/github-pr-refresh-sequences-leak.test.ts new file mode 100644 index 000000000..62c1b7a8f --- /dev/null +++ b/src/renderer/src/store/slices/github-pr-refresh-sequences-leak.test.ts @@ -0,0 +1,117 @@ +/** + * Memory-leak regression: prRefreshSequences must stay bounded. + * + * `prRefreshSequences` is a Record keyed by PR cache key (repo/branch/execution + * host). `applyGitHubPRRefreshEvent` only ever wrote entries and never removed + * them, so the map grew monotonically with the number of distinct (host, repo, + * branch) tuples observed — branches are ephemeral and unbounded over a long + * session. The fix caps it to MAX_CACHE_ENTRIES, evicting the oldest-touched + * keys (the writer moves each touched key to the most-recent position). + */ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { create } from 'zustand' +import { createGitHubSlice } from './github' +import { createHostedReviewSlice } from './hosted-review' +import type { AppState } from '../types' + +// MAX_CACHE_ENTRIES is module-private; mirror its value here. +const MAX_CACHE_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 + ) +} + +describe('prRefreshSequences stays bounded (leak regression)', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('caps prRefreshSequences and keeps the most recently touched key', () => { + const store = createTestStore() + + // Seed more sequence entries than the cap allows. + const seeded: Record = {} + const seedCount = MAX_CACHE_ENTRIES + 100 + for (let i = 0; i < seedCount; i++) { + seeded[`seed-${i}`] = 1 + } + store.setState({ prRefreshSequences: seeded }) + + // One more refresh event for a brand-new PR cache key pushes over the cap. + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'visible', + status: 'in-flight', + aliases: [{ cacheKey: 'key-new', repoPath: '/repo/new', branch: 'branch-new' }] + }) + + const sequences = store.getState().prRefreshSequences + // Bounded — not seedCount + 1. + expect(Object.keys(sequences)).toHaveLength(MAX_CACHE_ENTRIES) + // The just-touched key survives; the oldest seeded key is evicted. + expect(sequences['key-new']).toBe(1) + expect(sequences['seed-0']).toBeUndefined() + }) + + it('does not evict anything while under the cap', () => { + const store = createTestStore() + store.getState().applyGitHubPRRefreshEvent({ + sequence: 3, + reason: 'visible', + status: 'in-flight', + aliases: [{ cacheKey: 'only-key', repoPath: '/repo', branch: 'b' }] + }) + expect(store.getState().prRefreshSequences['only-key']).toBe(3) + }) + + it('keeps a refreshed older key by moving it to most-recent before capping', () => { + const store = createTestStore() + const seeded: Record = {} + const seedCount = MAX_CACHE_ENTRIES + 100 + for (let i = 0; i < seedCount; i++) { + seeded[`seed-${i}`] = 1 + } + store.setState({ prRefreshSequences: seeded }) + + // Refresh the OLDEST key. The writer moves it to most-recent (delete+set), + // so capping must evict the next-oldest keys, not this freshly-touched one. + store.getState().applyGitHubPRRefreshEvent({ + sequence: 9, + reason: 'visible', + status: 'in-flight', + aliases: [{ cacheKey: 'seed-0', repoPath: '/repo/0', branch: 'branch-0' }] + }) + + const sequences = store.getState().prRefreshSequences + expect(Object.keys(sequences)).toHaveLength(MAX_CACHE_ENTRIES) + // Survives with its updated sequence; without move-to-end it would be evicted. + expect(sequences['seed-0']).toBe(9) + // The next-oldest key is the one evicted instead. + expect(sequences['seed-1']).toBeUndefined() + }) +}) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index d67ab1b73..ee0c6e2ac 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -1365,6 +1365,27 @@ function withBoundedCacheEntry( return evictStaleEntries({ ...cache, [key]: entry }) } +// Why: prRefreshSequences only ever grows — one entry per PR cache key +// (repo/branch/execution-host) ever observed, and branches are ephemeral and +// unbounded over a long session. It has no `fetchedAt` to sort by, so bound it +// by insertion order (oldest-touched keys evicted first; the writer moves each +// touched key to the end). An evicted long-idle branch simply restarts sequence +// comparison from 0, which is acceptable. +function capPrRefreshSequences( + sequences: Record, + maxEntries = MAX_CACHE_ENTRIES +): Record { + const keys = Object.keys(sequences) + if (keys.length <= maxEntries) { + return sequences + } + const capped: Record = {} + for (const key of keys.slice(keys.length - maxEntries)) { + capped[key] = sequences[key] + } + return capped +} + function shouldRefreshIssueDecorations(state: AppState): boolean { return (state.worktreeCardProperties ?? []).includes('issue') } @@ -3361,6 +3382,9 @@ export const createGitHubSlice: StateCreator = (s } continue } + // Why: delete-then-set moves this key to the end of insertion order so + // capPrRefreshSequences evicts genuinely idle keys, not active ones. + delete nextSequences[alias.cacheKey] nextSequences[alias.cacheKey] = event.sequence changed = true @@ -3510,7 +3534,7 @@ export const createGitHubSlice: StateCreator = (s return changed ? { - prRefreshSequences: nextSequences, + prRefreshSequences: capPrRefreshSequences(nextSequences), prRefreshStates: nextStates, prCache: nextPRCache, hostedReviewCache: nextHostedReviewCache