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 <help@stably.ai> * 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 <help@stably.ai> * 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 <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
GitHub
parent
d90366f720
commit
b0a4d04fbf
|
|
@ -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<AppState>()(
|
||||
(...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<string, SeedState> = { '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<string, SeedState> = {}
|
||||
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()
|
||||
})
|
||||
})
|
||||
|
|
@ -1365,23 +1365,77 @@ function withBoundedCacheEntry<T extends { fetchedAt: number }>(
|
|||
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<T>(
|
||||
record: Record<string, T>,
|
||||
maxEntries = MAX_CACHE_ENTRIES
|
||||
): Record<string, T> {
|
||||
const keys = Object.keys(record)
|
||||
if (keys.length <= maxEntries) {
|
||||
return record
|
||||
}
|
||||
const capped: Record<string, T> = {}
|
||||
for (const key of keys.slice(keys.length - maxEntries)) {
|
||||
capped[key] = record[key]
|
||||
}
|
||||
return capped
|
||||
}
|
||||
|
||||
function capPrRefreshSequences(
|
||||
sequences: Record<string, number>,
|
||||
maxEntries = MAX_CACHE_ENTRIES
|
||||
): Record<string, number> {
|
||||
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<PRRefreshState['status']>(['error', 'skipped'])
|
||||
|
||||
function capPrRefreshStates(
|
||||
states: Record<string, PRRefreshState>,
|
||||
maxEntries = MAX_PR_REFRESH_STATE_ENTRIES
|
||||
): Record<string, PRRefreshState> {
|
||||
const keys = Object.keys(states)
|
||||
let toEvict = keys.length - maxEntries
|
||||
if (toEvict <= 0) {
|
||||
return states
|
||||
}
|
||||
const capped: Record<string, number> = {}
|
||||
for (const key of keys.slice(keys.length - maxEntries)) {
|
||||
capped[key] = sequences[key]
|
||||
const evicted = new Set<string>()
|
||||
// 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<string, PRRefreshState> = {}
|
||||
for (const key of keys) {
|
||||
if (!evicted.has(key)) {
|
||||
capped[key] = states[key]
|
||||
}
|
||||
}
|
||||
return capped
|
||||
}
|
||||
|
|
@ -3523,6 +3577,9 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (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<AppState, [], [], GitHubSlice> = (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
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue