From 19b63c4cb408435d945e182f5094bdb586ea55af Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 11 Jul 2026 13:59:28 -0700 Subject: [PATCH] Prune detected worktree scan generation markers (#7679) * Prune detected worktree scan generations * Invalidate pending scans in test reset hook Co-authored-by: Orca --------- Co-authored-by: Orca --- src/main/ipc/worktrees.test.ts | 199 ++++++++++++++++++++++++++++++++- src/main/ipc/worktrees.ts | 51 ++++++--- 2 files changed, 235 insertions(+), 15 deletions(-) diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index db5bf3b1a..031f9cf70 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -246,7 +246,11 @@ import { notifyWorktreesChanged } from './worktree-remote' import { invalidateAuthorizedRootsCache, resolveRegisteredWorktreePath } from './filesystem-auth' -import { __resetDetectedWorktreeScanCacheForTests, registerWorktreeHandlers } from './worktrees' +import { + __getDetectedWorktreeScanCacheStatsForTests, + __resetDetectedWorktreeScanCacheForTests, + registerWorktreeHandlers +} from './worktrees' type HandlerMap = Record unknown> @@ -2633,6 +2637,199 @@ describe('registerWorktreeHandlers', () => { expect(listWorktreesMock).toHaveBeenCalledTimes(1) }) + it('does not retain invalidated detected scans after they settle', async () => { + let resolveScan: (worktrees: GitWorktreeInfo[]) => void = () => {} + listWorktreesMock.mockImplementation( + () => + new Promise((resolve) => { + resolveScan = resolve as (worktrees: GitWorktreeInfo[]) => void + }) + ) + + const pendingList = handlers['worktrees:listDetected'](null, { repoId: 'repo-1' }) + await Promise.resolve() + + expect(__getDetectedWorktreeScanCacheStatsForTests()).toMatchObject({ + cacheSize: 0, + inFlightSize: 1 + }) + + notifyWorktreesChanged(mainWindow as never, 'repo-1') + + expect(__getDetectedWorktreeScanCacheStatsForTests()).toMatchObject({ + cacheSize: 0, + inFlightSize: 0 + }) + + resolveScan([ + { + path: '/workspace/repo', + head: 'main-head', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ]) + await pendingList + + expect(__getDetectedWorktreeScanCacheStatsForTests()).toMatchObject({ + cacheSize: 0, + inFlightSize: 0 + }) + }) + + it('does not accumulate scan bookkeeping across prolonged repository churn', async () => { + store.getRepo.mockImplementation((repoId: string) => ({ + id: repoId, + path: `/workspace/${repoId}`, + displayName: repoId, + badgeColor: '#000', + addedAt: 0, + worktreeBaseRef: null + })) + listWorktreesMock.mockImplementation(async (repoPath: string) => [ + { + path: repoPath, + head: 'main-head', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ]) + + for (let index = 0; index < 128; index += 1) { + const repoId = `repo-${index}` + await handlers['worktrees:listDetected'](null, { repoId }) + notifyWorktreesChanged(mainWindow as never, repoId) + } + + expect(__getDetectedWorktreeScanCacheStatsForTests()).toEqual({ + cacheSize: 0, + inFlightSize: 0 + }) + expect(listWorktreesMock).toHaveBeenCalledTimes(128) + }) + + it('keeps a replacement scan current after an older scan settles first', async () => { + const resolvers: ((worktrees: GitWorktreeInfo[]) => void)[] = [] + listWorktreesMock.mockImplementation( + () => + new Promise((resolve) => { + resolvers.push(resolve as (worktrees: GitWorktreeInfo[]) => void) + }) + ) + const result = [ + { + path: '/workspace/repo', + head: 'main-head', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ] + + const staleList = handlers['worktrees:listDetected'](null, { repoId: 'repo-1' }) + await Promise.resolve() + notifyWorktreesChanged(mainWindow as never, 'repo-1') + const replacementList = handlers['worktrees:listDetected'](null, { repoId: 'repo-1' }) + await Promise.resolve() + + resolvers[0](result) + await staleList + expect(__getDetectedWorktreeScanCacheStatsForTests()).toEqual({ + cacheSize: 0, + inFlightSize: 1 + }) + + resolvers[1](result) + await replacementList + expect(__getDetectedWorktreeScanCacheStatsForTests()).toEqual({ + cacheSize: 1, + inFlightSize: 0 + }) + }) + + it('does not let an older scan overwrite a replacement that settles first', async () => { + const resolvers: ((worktrees: GitWorktreeInfo[]) => void)[] = [] + listWorktreesMock.mockImplementation( + () => + new Promise((resolve) => { + resolvers.push(resolve as (worktrees: GitWorktreeInfo[]) => void) + }) + ) + store.getAllWorktreeLineage.mockReturnValue({ + 'repo-1::/workspace/fresh-worktree': { + worktreeId: 'repo-1::/workspace/fresh-worktree', + worktreeInstanceId: 'child-instance', + parentWorktreeId: 'repo-1::/workspace/repo', + parentWorktreeInstanceId: 'parent-instance', + origin: 'manual', + capture: { + source: 'manual-action', + confidence: 'explicit' + }, + createdAt: 0 + } + }) + const mainWorktree: GitWorktreeInfo = { + path: '/workspace/repo', + head: 'main-head', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + + const staleList = handlers['worktrees:listDetected'](null, { repoId: 'repo-1' }) + await Promise.resolve() + notifyWorktreesChanged(mainWindow as never, 'repo-1') + const replacementList = handlers['worktrees:listDetected'](null, { repoId: 'repo-1' }) + await Promise.resolve() + + resolvers[1]([ + mainWorktree, + { + path: '/workspace/fresh-worktree', + head: 'fresh-head', + branch: 'refs/heads/fresh-worktree', + isBare: false, + isMainWorktree: false + } + ]) + await replacementList + + resolvers[0]([ + mainWorktree, + { + path: '/workspace/stale-worktree', + head: 'stale-head', + branch: 'refs/heads/stale-worktree', + isBare: false, + isMainWorktree: false + } + ]) + await staleList + + const cached = (await handlers['worktrees:listDetected'](null, { + repoId: 'repo-1' + })) as { worktrees: Worktree[] } + expect(cached.worktrees.map((worktree) => worktree.path)).toEqual([ + '/workspace/repo', + '/workspace/fresh-worktree' + ]) + expect(store.removeWorktreeLineage).not.toHaveBeenCalled() + await expect( + resolveRegisteredWorktreePath('/workspace/fresh-worktree', store as never) + ).resolves.toBe(resolve('/workspace/fresh-worktree')) + await expect( + resolveRegisteredWorktreePath('/workspace/stale-worktree', store as never) + ).rejects.toThrow('Access denied: unknown repository or worktree path') + expect(listWorktreesMock).toHaveBeenCalledTimes(2) + expect(__getDetectedWorktreeScanCacheStatsForTests()).toEqual({ + cacheSize: 1, + inFlightSize: 0 + }) + }) + it('fetches the same-repo PR head via the SSH tracking-ref RPC, not git.exec', async () => { const fetchRemoteTrackingRef = vi.fn(async () => {}) const exec = vi.fn(async (args: string[]) => { diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index df0dfd4c5..6aeb11443 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -469,37 +469,59 @@ type DetectedWorktreeScanCacheEntry = { worktrees: GitWorktreeInfo[] } +type DetectedWorktreeScan = { + invalidated: boolean + promise: Promise +} + type DetectedWorktreeScanResult = { gitWorktrees: GitWorktreeInfo[] fresh: boolean } const detectedWorktreeScanCache = new Map() -const detectedWorktreeScanInFlight = new Map>() -const detectedWorktreeScanGenerations = new Map() +const detectedWorktreeScanInFlight = new Map() function invalidateDetectedWorktreeScanCache(repoId: string): void { const keyPrefix = `${repoId}\0` for (const key of new Set([ ...detectedWorktreeScanCache.keys(), - ...detectedWorktreeScanInFlight.keys(), - ...detectedWorktreeScanGenerations.keys() + ...detectedWorktreeScanInFlight.keys() ])) { if (!key.startsWith(keyPrefix)) { continue } detectedWorktreeScanCache.delete(key) - detectedWorktreeScanInFlight.delete(key) - detectedWorktreeScanGenerations.set(key, (detectedWorktreeScanGenerations.get(key) ?? 0) + 1) + const inFlight = detectedWorktreeScanInFlight.get(key) + if (inFlight) { + // Why: the detached scan keeps this token, so later scans can settle + // without making an older result fresh again. + inFlight.invalidated = true + detectedWorktreeScanInFlight.delete(key) + } } } registerWorktreeChangeInvalidator(invalidateDetectedWorktreeScanCache) export function __resetDetectedWorktreeScanCacheForTests(): void { + // Why: scans still pending across a test reset must not repopulate the + // cache afterward and leak state into the next test. + for (const scan of detectedWorktreeScanInFlight.values()) { + scan.invalidated = true + } detectedWorktreeScanCache.clear() detectedWorktreeScanInFlight.clear() - detectedWorktreeScanGenerations.clear() +} + +export function __getDetectedWorktreeScanCacheStatsForTests(): { + cacheSize: number + inFlightSize: number +} { + return { + cacheSize: detectedWorktreeScanCache.size, + inFlightSize: detectedWorktreeScanInFlight.size + } } async function listDetectedGitWorktrees( @@ -522,24 +544,25 @@ async function listDetectedGitWorktrees( const inFlight = detectedWorktreeScanInFlight.get(cacheKey) if (inFlight) { - return { gitWorktrees: await inFlight, fresh: false } + return { gitWorktrees: await inFlight.promise, fresh: false } } - const scan = listRepoWorktrees(repo, localWorktreeGitOptions) - const generation = detectedWorktreeScanGenerations.get(cacheKey) ?? 0 + const scan: DetectedWorktreeScan = { + invalidated: false, + promise: listRepoWorktrees(repo, localWorktreeGitOptions) + } detectedWorktreeScanInFlight.set(cacheKey, scan) try { - const gitWorktrees = await scan + const gitWorktrees = await scan.promise // Why: a create/remove notification can invalidate while the git scan is // still running. Do not let that stale scan repopulate the cache afterward. - const isCurrentGeneration = (detectedWorktreeScanGenerations.get(cacheKey) ?? 0) === generation - if (isCurrentGeneration) { + if (!scan.invalidated) { detectedWorktreeScanCache.set(cacheKey, { worktrees: gitWorktrees, expiresAt: Date.now() + DETECTED_WORKTREE_SCAN_CACHE_TTL_MS }) } - return { gitWorktrees, fresh: isCurrentGeneration } + return { gitWorktrees, fresh: !scan.invalidated } } finally { if (detectedWorktreeScanInFlight.get(cacheKey) === scan) { detectedWorktreeScanInFlight.delete(cacheKey)