From 6ccfc1246cc5d5071111f573844fb2a549fa58aa Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:37:28 -0700 Subject: [PATCH] fix: preserve newer pull request lookup ownership (#23119) * fix: preserve newer pull request lookup ownership * refactor: share one lookup generation sequence between review coordinators The sibling hosted-review fix needs the same never-reused generation id, so move the allocator into src/renderer/src/store/lookup-generation-sequence.ts instead of keeping a second private counter per coordinator. Also assert in the lifetime test that a stale lookup cannot publish into prCache and that the live lookup's answer is what lands there. * docs(store): describe the lookup generation sequence by its contract, not its callers The JSDoc claimed the sequence was "shared by the pull-request and hosted-review request coordinators", but the hosted-review call site arrives in a sibling change, so on this branch alone the comment named a caller that does not exist. Describe what the module provides instead: one process-lifetime monotonic allocator for lookup-ownership stamps, safe to share across every cache because callers compare stamps for equality only, never order or magnitude. That stays accurate whether one coordinator draws from it or several, so it needs no edit when the second call site lands. Co-Authored-By: Claude --------- Co-authored-by: Claude --- .../src/store/github/pull-request-actions.ts | 3 +- .../src/store/lookup-generation-sequence.ts | 22 +++ .../slices/github-pr-request-lifetime.test.ts | 154 ++++++++++++++++++ 3 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 src/renderer/src/store/lookup-generation-sequence.ts create mode 100644 src/renderer/src/store/slices/github-pr-request-lifetime.test.ts 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) + }) +})