From 41cbdb1d1d65f9c9538a2347684e1f4ddb3f06ec Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 31 May 2026 01:35:21 -0700 Subject: [PATCH] fix: clear completed PR request generations --- docs/reference/memory-leak-audit-pass-3.md | 44 ++++++++++++++++++++ src/renderer/src/store/slices/github.test.ts | 20 +++++++++ src/renderer/src/store/slices/github.ts | 18 ++++---- 3 files changed, 75 insertions(+), 7 deletions(-) create mode 100644 docs/reference/memory-leak-audit-pass-3.md diff --git a/docs/reference/memory-leak-audit-pass-3.md b/docs/reference/memory-leak-audit-pass-3.md new file mode 100644 index 000000000..aeab81878 --- /dev/null +++ b/docs/reference/memory-leak-audit-pass-3.md @@ -0,0 +1,44 @@ +# Memory Leak Audit Pass 3 + +Started: 2026-05-31 PDT + +Objective: continue the repository-wide leak audit from pass 2 on current +`origin/main`, with special attention to code changed after +`a85e4e8d88` (`docs: record memory leak audit pass 2`). + +## Delta Inventory + +- 2026-05-31: Fast-forwarded the audit worktree to current `origin/main`. +- 2026-05-31: Counted 1431 changed code files since pass 2 (`*.ts`, + `*.tsx`, `*.js`, `*.jsx`, `*.mjs`, `*.cjs`, `*.swift`). +- 2026-05-31: Re-ran delta heuristics for DOM/RN listeners, timers, + animation frames, observers, EventEmitter subscriptions, runtime + subscriptions, workers, WebSockets, abort controllers, streams, and + module-scope `Map`/`Set` caches. +- 2026-05-31: Manually followed up high-risk delta hits in GitHub renderer + state, PR diff caches, combined diff caches, mobile browser frame caches, + main subprocess listeners, runtime file watchers, browser/webContents + listeners, notification lifetimes, and renderer UI timers. + +## Finding + +- `src/renderer/src/store/slices/github.ts`: `prRequestGenerations` was a + module-scoped map keyed by PR cache key. Each unique PR lookup inserted a + generation entry, but completed requests only removed `inflightPRRequests`; + the generation entry remained for the lifetime of the renderer. Fixed by + deleting the generation key when the request that owns the current + generation is also the active in-flight request. Overlapping forced refreshes + still keep the stale-response guard because older requests cannot delete a + newer generation. Risk: low. + +## Validation + +- `pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/store/slices/github.test.ts` +- `pnpm exec oxlint src/renderer/src/store/slices/github.ts src/renderer/src/store/slices/github.test.ts` +- `pnpm exec tsgo --noEmit -p config/tsconfig.tc.web.json` +- `git diff --check` + +## Remaining Work + +- Continue the pass-3 delta audit beyond the confirmed GitHub PR generation + leak. Current evidence does not prove the full repository is leak-free. diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 09921264d..af959ecc6 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -4,6 +4,7 @@ GitHub slice's cross-cutting invariants verifiable in one place. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { create } from 'zustand' import { + _getGitHubPRRequestGenerationCountForTest, createGitHubSlice, mergePRCommentIntoList, prChecksCacheSuffix, @@ -1015,6 +1016,25 @@ describe('createGitHubSlice.fetchPRForBranch', () => { } }) + it('does not retain PR request generation keys after the active request settles', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/no-generation-leak' + const beforeCount = _getGitHubPRRequestGenerationCountForTest() + const refreshPRNow = mockApi.gh.refreshPRNow + ;(mockApi.gh as unknown as { refreshPRNow?: typeof refreshPRNow }).refreshPRNow = undefined + mockApi.gh.prForBranch.mockResolvedValueOnce(makePR({ number: 31 })) + + try { + await expect( + store.getState().fetchPRForBranch(repoPath, branch, { force: true }) + ).resolves.toMatchObject({ number: 31 }) + expect(_getGitHubPRRequestGenerationCountForTest()).toBe(beforeCount) + } finally { + mockApi.gh.refreshPRNow = refreshPRNow + } + }) + it('passes SSH connection identity to GitHub refresh IPC for SSH-backed repos', async () => { const store = createTestStore() const repoPath = '/repo' diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index e27efd961..495913d62 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -365,6 +365,11 @@ const prRefreshStartedHostedReviewEntries = new Map< AppState['hostedReviewCache'][string] | undefined >() +/** @internal - exposed for leak-regression tests only */ +export function _getGitHubPRRequestGenerationCountForTest(): number { + return prRequestGenerations.size +} + // Why: cap in-flight cross-repo fan-out and hover-prefetches at the renderer // boundary — the main-side gate is behind the IPC queue, so it can't see a // stampede until the calls are already mid-flight. 8 balances responsiveness @@ -2067,6 +2072,9 @@ export const createGitHubSlice: StateCreator = (s const activeRequest = inflightPRRequests.get(cacheKey) if (activeRequest?.generation === generation) { inflightPRRequests.delete(cacheKey) + if (prRequestGenerations.get(cacheKey) === generation) { + prRequestGenerations.delete(cacheKey) + } } } })() @@ -2741,13 +2749,9 @@ export const createGitHubSlice: StateCreator = (s issueCache: evictStaleEntries(s.issueCache) })) - // Why: prRequestGenerations tracks generation counters for inflight - // fetch deduplication. Pruning keys that were just evicted from prCache - // would race with inflight requests — their generation check would fail - // and silently discard valid responses. Since each entry is just a number, - // the memory overhead is negligible; let it shrink naturally as keys stop - // being fetched. The eviction on prCache/issueCache above is sufficient - // to bound the dominant source of growth. + // Why: prRequestGenerations tracks only live inflight fetches and is + // cleared when the active request settles. Do not prune it here; deleting + // a live generation would make the corresponding response look stale. // Only re-fetch PR/issue entries that are already stale — skip fresh ones const state = get()