From b0a4d04fbfd768916ace072fe9b93def76d09439 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 19 Jun 2026 12:34:30 -0700 Subject: [PATCH] fix(store): cap prRefreshStates the same way prRefreshSequences is capped (#5802) * fix(store): cap prRefreshStates the same way prRefreshSequences is capped prRefreshStates is keyed by the same unbounded PR cache key (execution-host/repo/branch) as prRefreshSequences, but was committed uncapped one line below the capped sequences map. Status-only events (paused/skipped) and upstream-error outcomes add entries that no prune path ever removes, so it grew monotonically over a long session. Bound it by insertion order via a shared capRecordByInsertionOrder helper (capPrRefreshSequences now delegates to it), and delete-then-set the touched key in the status writer so only idle keys are evicted. Regression test fails before the fix (map grows past the cap) and passes after. Co-authored-by: Orca * test(store): fix prRefreshStates leak-test types for tsc/tsgo The leak test ran under vitest (esbuild strips types) but failed tsgo: the status-event helper widened literals and the seeded entries typed reason as string. Annotate the helper as GitHubPRRefreshEvent (in-flight variant) and type seeded values' reason as GitHubPRRefreshReason. Co-authored-by: Orca * fix(store): make prRefreshStates eviction status-aware to prevent UI regression prRefreshStates backs visible status pills, so a plain insertion-order cap could drop an in-progress (in-flight/queued/paused) indicator under a large working set. Raise the bound to 2000 (well above any realistic tracked-branch count) and evict settled statuses (error/skipped) first; active entries are dropped only as a last-resort hard bound. Evicted entries self-heal on the next refresh event. Test asserts settled-before-active eviction and that the bound holds; still fails on the un-capped (main) behavior. Co-authored-by: Orca --------- Co-authored-by: Orca --- .../github-pr-refresh-states-leak.test.ts | 153 ++++++++++++++++++ src/renderer/src/store/slices/github.ts | 85 ++++++++-- 2 files changed, 225 insertions(+), 13 deletions(-) create mode 100644 src/renderer/src/store/slices/github-pr-refresh-states-leak.test.ts diff --git a/src/renderer/src/store/slices/github-pr-refresh-states-leak.test.ts b/src/renderer/src/store/slices/github-pr-refresh-states-leak.test.ts new file mode 100644 index 000000000..d9d1a91ec --- /dev/null +++ b/src/renderer/src/store/slices/github-pr-refresh-states-leak.test.ts @@ -0,0 +1,153 @@ +/** + * Memory-leak regression: prRefreshStates must stay bounded. + * + * `prRefreshStates` is a Record keyed by PR cache key (repo/branch/execution host) + * — the SAME unbounded, ephemeral key space the sibling `prRefreshSequences` is + * already capped against. `applyGitHubPRRefreshEvent` writes a status entry on every + * status-only refresh event (paused/skipped/in-flight) and re-adds one for + * upstream-error outcomes, but no prune path ever removed them, so the map grew + * monotonically with the number of distinct (host, repo, branch) tuples observed. + * + * The fix bounds it to MAX_PR_REFRESH_STATE_ENTRIES, but because this map backs + * visible status pills it uses status-aware eviction: settled statuses (error/ + * skipped) are evicted first so an in-progress (in-flight/queued/paused) indicator + * is never dropped except as a last-resort hard bound. + */ +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' +import type { GitHubPRRefreshEvent, GitHubPRRefreshReason } from '../../../../shared/types' + +// MAX_PR_REFRESH_STATE_ENTRIES is module-private; mirror its value here. +const MAX_ENTRIES = 2000 + +// prRefreshStates entry shapes (the module's PRRefreshState type is not exported). +type SeedState = { + status: 'in-flight' | 'error' + reason: GitHubPRRefreshReason + updatedAt: number + message?: string +} + +function inFlightState(): SeedState { + return { status: 'in-flight', reason: 'visible', updatedAt: 0 } +} + +function errorState(): SeedState { + return { status: 'error', reason: 'visible', updatedAt: 0, message: 'boom' } +} + +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 + ) +} + +// A status-only refresh event (no outcome) lands in the prRefreshStates writer. +function statusEvent(cacheKey: string, sequence: number): GitHubPRRefreshEvent { + return { + sequence, + reason: 'visible', + status: 'in-flight', + aliases: [{ cacheKey, repoPath: `/repo/${cacheKey}`, branch: cacheKey }] + } +} + +describe('prRefreshStates stays bounded (leak regression)', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('caps prRefreshStates when driven past the cap by the real writer', () => { + const store = createTestStore() + + // Each distinct branch produces a distinct cache key — an unbounded key space. + const total = MAX_ENTRIES + 150 + for (let i = 0; i < total; i++) { + store.getState().applyGitHubPRRefreshEvent(statusEvent(`branch-${i}`, 1)) + } + + const states = store.getState().prRefreshStates + // Bounded — not `total`. + expect(Object.keys(states)).toHaveLength(MAX_ENTRIES) + // The most-recently-written key survives; the oldest is evicted. + expect(states[`branch-${total - 1}`]).toBeDefined() + expect(states['branch-0']).toBeUndefined() + }) + + it('evicts settled (error/skipped) statuses before active ones', () => { + const store = createTestStore() + + // Oldest entry is a settled error; the rest are active in-flight refreshes, + // filling the map exactly to the cap. + const seeded: Record = { 'stale-error': errorState() } + for (let i = 0; i < MAX_ENTRIES - 1; i++) { + seeded[`active-${i}`] = inFlightState() + } + store.setState({ prRefreshStates: seeded }) + + // One more active refresh pushes over the cap by one. + store.getState().applyGitHubPRRefreshEvent(statusEvent('fresh', 1)) + + const states = store.getState().prRefreshStates + expect(Object.keys(states)).toHaveLength(MAX_ENTRIES) + // The settled error is evicted first; every active entry survives. + expect(states['stale-error']).toBeUndefined() + expect(states['fresh']).toBeDefined() + expect(states['active-0']).toBeDefined() + expect(states['active-500']).toBeDefined() + }) + + it('does not evict anything while under the cap', () => { + const store = createTestStore() + store.getState().applyGitHubPRRefreshEvent(statusEvent('only-key', 3)) + expect(store.getState().prRefreshStates['only-key']).toBeDefined() + expect(store.getState().prRefreshStates['only-key']?.status).toBe('in-flight') + }) + + it('keeps a refreshed older key by moving it to most-recent before capping', () => { + const store = createTestStore() + // Fill exactly to the cap with active entries (no settled ones to evict first). + const seeded: Record = {} + for (let i = 0; i < MAX_ENTRIES; i++) { + seeded[`seed-${i}`] = inFlightState() + } + store.setState({ prRefreshStates: seeded }) + + // Refresh the OLDEST key (move-to-end), then add a brand-new key to force a + // last-resort eviction: the just-refreshed key must survive, the next-oldest not. + store.getState().applyGitHubPRRefreshEvent(statusEvent('seed-0', 9)) + store.getState().applyGitHubPRRefreshEvent(statusEvent('newcomer', 1)) + + const states = store.getState().prRefreshStates + expect(Object.keys(states)).toHaveLength(MAX_ENTRIES) + expect(states['seed-0']).toBeDefined() + expect(states['seed-1']).toBeUndefined() + expect(states['newcomer']).toBeDefined() + }) +}) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index ee0c6e2ac..72126169b 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -1365,23 +1365,77 @@ 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. +// Why: the prRefresh* maps are keyed by PR cache key (repo/branch/execution-host) +// — an ephemeral, unbounded key space over a long session. They have no +// `fetchedAt` to sort by, so bound them by insertion order (oldest-touched keys +// evicted first; the writers move each touched key to the end). An evicted +// long-idle branch simply restarts from a clean state, which is acceptable. +function capRecordByInsertionOrder( + record: Record, + maxEntries = MAX_CACHE_ENTRIES +): Record { + const keys = Object.keys(record) + if (keys.length <= maxEntries) { + return record + } + const capped: Record = {} + for (const key of keys.slice(keys.length - maxEntries)) { + capped[key] = record[key] + } + return capped +} + function capPrRefreshSequences( sequences: Record, maxEntries = MAX_CACHE_ENTRIES ): Record { - const keys = Object.keys(sequences) - if (keys.length <= maxEntries) { - return sequences + return capRecordByInsertionOrder(sequences, maxEntries) +} + +// Why: prRefreshStates backs visible status pills (refreshing/queued/paused/error) +// so — unlike the invisible sequence guard — eviction must never drop an in-progress +// indicator. Bound it well above any realistic tracked-branch count, and when over +// cap evict *settled* statuses (error/skipped) first; only fall back to evicting an +// active (in-flight/queued/paused) entry as a last-resort hard memory bound that +// realistic usage never reaches. Evicted entries self-heal on the next refresh event. +const MAX_PR_REFRESH_STATE_ENTRIES = 2000 +const SETTLED_PR_REFRESH_STATUSES = new Set(['error', 'skipped']) + +function capPrRefreshStates( + states: Record, + maxEntries = MAX_PR_REFRESH_STATE_ENTRIES +): Record { + const keys = Object.keys(states) + let toEvict = keys.length - maxEntries + if (toEvict <= 0) { + return states } - const capped: Record = {} - for (const key of keys.slice(keys.length - maxEntries)) { - capped[key] = sequences[key] + const evicted = new Set() + // First pass: evict oldest settled (error/skipped) entries. + for (const key of keys) { + if (toEvict === 0) { + break + } + if (SETTLED_PR_REFRESH_STATUSES.has(states[key].status)) { + evicted.add(key) + toEvict-- + } + } + // Last resort: evict oldest remaining keys to enforce the hard bound. + for (const key of keys) { + if (toEvict === 0) { + break + } + if (!evicted.has(key)) { + evicted.add(key) + toEvict-- + } + } + const capped: Record = {} + for (const key of keys) { + if (!evicted.has(key)) { + capped[key] = states[key] + } } return capped } @@ -3523,6 +3577,9 @@ export const createGitHubSlice: StateCreator = (s // longer live and would otherwise accumulate per refresh sequence. deletePRRefreshStartedEntry(event.sequence, alias.cacheKey) } + // Why: delete-then-set moves this key to the end of insertion order so + // capRecordByInsertionOrder evicts genuinely idle keys, not active ones. + delete nextStates[alias.cacheKey] nextStates[alias.cacheKey] = { status: event.status, reason: event.reason, @@ -3535,7 +3592,9 @@ export const createGitHubSlice: StateCreator = (s return changed ? { prRefreshSequences: capPrRefreshSequences(nextSequences), - prRefreshStates: nextStates, + // Why: bound prRefreshStates too (same unbounded PR-cache-key space), + // but with status-aware eviction so visible in-progress pills survive. + prRefreshStates: capPrRefreshStates(nextStates), prCache: nextPRCache, hostedReviewCache: nextHostedReviewCache }