diff --git a/src/renderer/src/store/github/pull-request-actions.ts b/src/renderer/src/store/github/pull-request-actions.ts index c9a43e075ac..17852c1d57f 100644 --- a/src/renderer/src/store/github/pull-request-actions.ts +++ b/src/renderer/src/store/github/pull-request-actions.ts @@ -9,6 +9,7 @@ import { prCacheKey } from './cache-identity' import { isFresh } from './cache-policy' import { buildGitHubPRRefreshStateClearToken } from './pr-refresh-state' import { githubHostedReviewFallbackPRNumber, prLookupHintKey } from './pr-result-routing' +import { nextLookupGeneration } from '../lookup-generation-sequence' import { inflightPRRequests, prRequestGenerations } from './request-coordination' import { settingsForGitHubRepoOwner } from './work-item-routing' import { @@ -113,7 +114,7 @@ export const createPullRequestActions = ( return inflightRequest.promise } - const generation = (prRequestGenerations.get(cacheKey) ?? 0) + 1 + const generation = nextLookupGeneration() const requestStartedAt = Date.now() const requestStartedHostedReviewEntry = get().hostedReviewCache[hostedReviewCacheKey] const requestStartedPRRefreshState = get().prRefreshStates[cacheKey] diff --git a/src/renderer/src/store/lookup-generation-sequence.ts b/src/renderer/src/store/lookup-generation-sequence.ts new file mode 100644 index 00000000000..c253b9d0184 --- /dev/null +++ b/src/renderer/src/store/lookup-generation-sequence.ts @@ -0,0 +1,22 @@ +/** + * Id source for the generation stamps that decide which in-flight lookup owns a + * cache key. One counter, allocated from for the life of the process, serves + * every cache and every coordinator that needs such a stamp. + * + * A coordinator that drops a key's generation entry once its newest lookup + * settles lets a per-key counter restart at 1 while an older lookup of the same + * key is still out: that straggler then reads as the current owner, publishes + * its stale answer, and tears down the live newer lookup's ownership so the + * fresh answer is dropped. Ids that are never reused remove the collision. + * + * Callers only ever compare stamps for equality, never order or magnitude, so a + * single shared sequence is safe no matter how many caches draw from it. + * Deliberately has no reset: rewinding it while a lookup is out recreates the + * very collision this exists to prevent. + */ +let lookupGenerationSequence = 0 + +export function nextLookupGeneration(): number { + lookupGenerationSequence += 1 + return lookupGenerationSequence +} diff --git a/src/renderer/src/store/slices/github-pr-request-lifetime.test.ts b/src/renderer/src/store/slices/github-pr-request-lifetime.test.ts new file mode 100644 index 00000000000..ddd96471231 --- /dev/null +++ b/src/renderer/src/store/slices/github-pr-request-lifetime.test.ts @@ -0,0 +1,154 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { PRRefreshOutcome } from '../../../../shared/github/pull-request-refresh-types' +import { inflightPRRequests, prRequestGenerations } from '../github/request-coordination' +import { + createTestStore, + makePR, + mockApi, + resetRemoteRuntimeMocks, + runtimeEnvironmentCall +} from './github-slice-test-harness' + +beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(1000) + vi.clearAllMocks() + resetRemoteRuntimeMocks() + mockApi.gh.refreshPRNow.mockReset() + vi.spyOn(console, 'error').mockImplementation(() => undefined) +}) + +afterEach(() => { + vi.clearAllTimers() + vi.useRealTimers() + vi.restoreAllMocks() +}) + +function found(number: number): PRRefreshOutcome { + return { kind: 'found', pr: makePR({ number }), fetchedAt: Date.now() } +} + +const cases = [ + { route: 'local', replacement: 'force', outcome: 'found' }, + { route: 'local', replacement: 'hint', outcome: 'found' }, + { route: 'local', replacement: 'force', outcome: 'rejected' }, + { route: 'runtime', replacement: 'force', outcome: 'found' }, + { route: 'runtime', replacement: 'force', outcome: 'upstream-error' }, + { route: 'runtime', replacement: 'hint', outcome: 'no-pr' } +] as const + +describe('pull-request lookup ownership after newer settlement', () => { + it.each(cases)( + 'preserves $route lookup after late $outcome with $replacement replacement', + async ({ route, replacement, outcome }) => { + const store = createTestStore() + const pending: ReturnType>[] = [] + const startRequest = () => { + const request = Promise.withResolvers() + pending.push(request) + return request.promise + } + if (route === 'runtime') { + store.setState({ + repos: [ + { + id: 'runtime-repo', + path: '/runtime/repo', + displayName: 'Runtime repo', + badgeColor: 'blue', + addedAt: 1, + executionHostId: 'runtime:env-1' + } + ] + }) + runtimeEnvironmentCall.mockImplementation(async () => ({ + id: 'rpc', + ok: true, + result: await startRequest() + })) + } else { + mockApi.gh.refreshPRNow.mockImplementation(startRequest) + } + const path = route === 'runtime' ? '/runtime/repo' : '/local/repo' + const branch = `lifetime-${route}-${replacement}-${outcome}` + const options = replacement === 'hint' ? { force: true, fallbackPRNumber: 10 } : undefined + const first = store.getState().fetchPRForBranch(path, branch, options) + await vi.waitFor(() => expect(pending).toHaveLength(1)) + vi.setSystemTime(2000) + const secondOptions = + replacement === 'hint' ? { force: true, fallbackPRNumber: 20 } : { force: true } + const second = store.getState().fetchPRForBranch(path, branch, secondOptions) + await vi.waitFor(() => expect(pending).toHaveLength(2)) + pending[1].resolve(found(20)) + await second + expect(prRequestGenerations.size).toBe(0) + + vi.setSystemTime(3000) + const third = store.getState().fetchPRForBranch(path, branch, secondOptions) + await vi.waitFor(() => expect(pending).toHaveLength(3)) + const activeBefore = [...inflightPRRequests.values()][0] + const key = [...inflightPRRequests.keys()][0] + expect(activeBefore).toBeDefined() + if (outcome === 'rejected') { + pending[0].reject(new Error('old lookup failed')) + } else if (outcome === 'upstream-error') { + pending[0].resolve({ + kind: 'upstream-error', + message: 'old error', + errorType: 'unknown', + fetchedAt: 3000 + }) + } else if (outcome === 'no-pr') { + pending[0].resolve({ kind: 'no-pr', fetchedAt: 3000 }) + } else { + pending[0].resolve(found(10)) + } + await first + const activeAfter = inflightPRRequests.get(key) + const cachedAfterStale = store.getState().prCache[key]?.data?.number + const refreshStateAfter = store.getState().prRefreshStates[key] + vi.setSystemTime(4000) + const follower = store.getState().fetchPRForBranch(path, branch, secondOptions) + await vi.waitFor(() => expect(pending.length).toBeGreaterThanOrEqual(3)) + await Promise.resolve() + const requestCount = pending.length + pending[2].resolve(found(30)) + pending[3]?.resolve(found(40)) + const results = await Promise.all([third, follower]) + + expect(activeAfter).toBe(activeBefore) + // The stale lookup asked for PR 10; only the live lookup may publish. + expect(cachedAfterStale).toBe(20) + expect(refreshStateAfter).toBeUndefined() + expect(requestCount).toBe(3) + expect(results.map((result) => result?.number)).toEqual([30, 30]) + expect(store.getState().prCache[key]?.data?.number).toBe(30) + expect(inflightPRRequests.size).toBe(0) + expect(prRequestGenerations.size).toBe(0) + if (route === 'runtime') { + expect(mockApi.gh.refreshPRNow).not.toHaveBeenCalled() + } else { + expect(runtimeEnvironmentCall).not.toHaveBeenCalled() + } + } + ) + + it('dedupes each branch independently and removes settled generation entries', async () => { + const store = createTestStore() + const pending = Promise.withResolvers() + mockApi.gh.refreshPRNow.mockReturnValue(pending.promise) + const calls = ['one', 'two'].flatMap((branch) => + Array.from({ length: 10 }, () => + store.getState().fetchPRForBranch('/repo', branch, { force: true }) + ) + ) + expect(mockApi.gh.refreshPRNow).toHaveBeenCalledTimes(2) + expect(prRequestGenerations.size).toBe(2) + + pending.resolve(found(12)) + await Promise.all(calls) + + expect(inflightPRRequests.size).toBe(0) + expect(prRequestGenerations.size).toBe(0) + }) +})