mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
perf(github): coalesce stronger refreshes after pending requests (#22970)
* perf(github): coalesce stronger refreshes after pending requests * fix(github): bound the refresh-upgrade wait so a strict caller cannot starve The upgrade loop retried forever: a caller wanting a stronger refresh waited for each weaker in-flight request, rechecked, and waited again. A repeating weaker refresh (the quiet-refresh interval) could therefore pin a forced noCache caller for the life of the process with no timeout or escape hatch. Wait out at most one weaker request — enough for peers to share the upgrade — then issue our own. Call counts are unchanged; progress is now guaranteed. Retargets the coordination test at that invariant instead of asserting that strict callers block until the weaker replacement finishes. * fix(github): let only a dedupe key's current request write its cache Bounding the upgrade wait fixed the starvation but opened a window the unbounded loop never had: a stronger request can now run beside a weaker one for the same key. Nothing fenced the cache writes, so whichever settled last won. A force-only work-item request does not pass noCache, so gh's own cache can answer it; settling after the noCache request buried the fresher rows under a new fetchedAt and isFresh then served them for the rest of the TTL. Checks rewound run state the same way, and a superseded project request could stamp its failure over a newer table at the known view key. Stamp each request with a monotonic id on its inflight entry and recheck ownership after the provider call, immediately before every cache write — the work-items entry, both project-view branches, and checksCache plus the PR status syncPRChecksStatus derives from it. That is the same ownership question the cleanup guard already asked, so the cleanup now reads the stamp too; the promise itself cannot be compared from inside its own initializer. The wait stays bounded and the twenty-one-caller upgrade still collapses to two provider calls.
This commit is contained in:
@@ -12,7 +12,11 @@ import {
|
||||
} from './cache-identity'
|
||||
import { isFresh, withBoundedCacheEntry } from './cache-policy'
|
||||
import { debouncedSaveCache } from './cache-persistence'
|
||||
import { inflightChecksRequests } from './request-coordination'
|
||||
import {
|
||||
inflightChecksRequests,
|
||||
nextProviderRequestId,
|
||||
ownsInflightRequest
|
||||
} from './request-coordination'
|
||||
import { getGitHubRepoSourceSettings, getGitHubWorkItemRequestContext } from './work-item-routing'
|
||||
|
||||
export const createCheckActions = (
|
||||
@@ -87,18 +91,26 @@ export const createCheckActions = (
|
||||
return cachedChecks
|
||||
}
|
||||
|
||||
const inflightRequest = inflightChecksRequests.get(inflightKey)
|
||||
if (inflightRequest) {
|
||||
if (
|
||||
(options?.force && !inflightRequest.force) ||
|
||||
(options?.noCache && !inflightRequest.noCache)
|
||||
) {
|
||||
await inflightRequest.promise.catch(() => {})
|
||||
} else {
|
||||
let waitedForUpgrade = false
|
||||
for (;;) {
|
||||
const inflightRequest = inflightChecksRequests.get(inflightKey)
|
||||
if (!inflightRequest) {
|
||||
break
|
||||
}
|
||||
const weakerThanRequested =
|
||||
(options?.force && !inflightRequest.force) || (options?.noCache && !inflightRequest.noCache)
|
||||
if (!weakerThanRequested) {
|
||||
return inflightRequest.promise
|
||||
}
|
||||
// Why: wait out one weaker request so peers can share the upgrade, but never twice — a steady stream of weaker callers would otherwise starve this one forever.
|
||||
if (waitedForUpgrade) {
|
||||
break
|
||||
}
|
||||
waitedForUpgrade = true
|
||||
await inflightRequest.promise.catch(() => {})
|
||||
}
|
||||
|
||||
const requestId = nextProviderRequestId()
|
||||
const request = (async () => {
|
||||
try {
|
||||
const requestContext = getGitHubWorkItemRequestContext(
|
||||
@@ -131,6 +143,12 @@ export const createCheckActions = (
|
||||
noCache: Boolean(options?.force || options?.noCache),
|
||||
sourceContext: options?.sourceContext
|
||||
})) as PRCheckDetail[])
|
||||
// Why: the bounded upgrade wait can leave us running beside a stronger request for this
|
||||
// key. Both bypass gh's cache here, but the later-started one holds the newer run state —
|
||||
// let only the key's current owner write checksCache and the PR status it derives.
|
||||
if (!ownsInflightRequest(inflightChecksRequests, inflightKey, requestId)) {
|
||||
return checks
|
||||
}
|
||||
set((s) => {
|
||||
const nextState: Partial<AppState> = {
|
||||
checksCache: withBoundedCacheEntry(s.checksCache, cacheKey, {
|
||||
@@ -168,13 +186,16 @@ export const createCheckActions = (
|
||||
return latestCached.data
|
||||
}
|
||||
return []
|
||||
} finally {
|
||||
}
|
||||
})().finally(() => {
|
||||
if (ownsInflightRequest(inflightChecksRequests, inflightKey, requestId)) {
|
||||
inflightChecksRequests.delete(inflightKey)
|
||||
}
|
||||
})()
|
||||
})
|
||||
|
||||
inflightChecksRequests.set(inflightKey, {
|
||||
promise: request,
|
||||
requestId,
|
||||
force: Boolean(options?.force),
|
||||
noCache: Boolean(options?.force || options?.noCache)
|
||||
})
|
||||
|
||||
@@ -18,6 +18,8 @@ import { withBoundedCacheEntry, WORK_ITEMS_CACHE_TTL } from './cache-policy'
|
||||
import {
|
||||
acquireProviderRequestSlot as acquireWorkItemSlot,
|
||||
inflightProjectViewRequests,
|
||||
nextProviderRequestId,
|
||||
ownsInflightRequest,
|
||||
releaseProviderRequestSlot as releaseWorkItemSlot
|
||||
} from './request-coordination'
|
||||
import {
|
||||
@@ -57,16 +59,25 @@ export const createProjectActions = (
|
||||
}
|
||||
}
|
||||
|
||||
const existing = inflightProjectViewRequests.get(requestKey)
|
||||
if (existing) {
|
||||
let waitedForUpgrade = false
|
||||
for (;;) {
|
||||
const existing = inflightProjectViewRequests.get(requestKey)
|
||||
if (!existing) {
|
||||
break
|
||||
}
|
||||
// Why: a forcing caller must not dedupe to a non-forcing in-flight request; wait for it to settle, then issue a fresh forced call (mirrors fetchWorkItems).
|
||||
if (options?.force && !existing.force) {
|
||||
await existing.promise.catch(() => {})
|
||||
} else {
|
||||
if (!options?.force || existing.force) {
|
||||
return existing.promise
|
||||
}
|
||||
// Why: wait out one weaker request so peers can share the upgrade, but never twice — a steady stream of weaker callers would otherwise starve this one forever.
|
||||
if (waitedForUpgrade) {
|
||||
break
|
||||
}
|
||||
waitedForUpgrade = true
|
||||
await existing.promise.catch(() => {})
|
||||
}
|
||||
|
||||
const requestId = nextProviderRequestId()
|
||||
const request = (async (): Promise<GetProjectViewTableResult> => {
|
||||
await acquireWorkItemSlot()
|
||||
try {
|
||||
@@ -79,6 +90,12 @@ export const createProjectActions = (
|
||||
{ timeoutMs: 60_000 }
|
||||
)
|
||||
: await window.api.gh.getProjectViewTable(args)
|
||||
// Why: the bounded upgrade wait can leave us running beside a stronger request for this
|
||||
// key, so neither write below may land once it owns the key — a late non-OK reply would
|
||||
// otherwise stamp its error over the fresher table (or over a newer error) at the known key.
|
||||
if (!ownsInflightRequest(inflightProjectViewRequests, requestKey, requestId)) {
|
||||
return envelope
|
||||
}
|
||||
if (envelope.ok) {
|
||||
const table = envelope.data
|
||||
const key = projectViewCacheKey(
|
||||
@@ -119,12 +136,16 @@ export const createProjectActions = (
|
||||
}
|
||||
} finally {
|
||||
releaseWorkItemSlot()
|
||||
}
|
||||
})().finally(() => {
|
||||
if (ownsInflightRequest(inflightProjectViewRequests, requestKey, requestId)) {
|
||||
inflightProjectViewRequests.delete(requestKey)
|
||||
}
|
||||
})()
|
||||
})
|
||||
|
||||
inflightProjectViewRequests.set(requestKey, {
|
||||
promise: request,
|
||||
requestId,
|
||||
force: Boolean(options?.force)
|
||||
})
|
||||
return request
|
||||
|
||||
@@ -0,0 +1,343 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { GitHubWorkItem, ListWorkItemsResult } from '../../../../shared/github/work-item-types'
|
||||
import type { PRCheckDetail } from '../../../../shared/github/check-types'
|
||||
import type { GitHubProjectTable } from '../../../../shared/github/project-types'
|
||||
import type { GetProjectViewTableResult } from '../../../../shared/github/project-result-types'
|
||||
import type { FetchOptions } from './cache-model'
|
||||
import { projectViewCacheKey, projectViewRequestKey } from './cache-identity'
|
||||
import { inflightProjectViewRequests } from './request-coordination'
|
||||
import {
|
||||
createTestStore,
|
||||
mockApi,
|
||||
resetRemoteRuntimeMocks
|
||||
} from '../slices/github-slice-test-harness'
|
||||
|
||||
function workItems(title: string): ListWorkItemsResult<GitHubWorkItem> {
|
||||
return {
|
||||
items: [
|
||||
{
|
||||
id: 'issue-1',
|
||||
type: 'issue',
|
||||
number: 1,
|
||||
title,
|
||||
state: 'open',
|
||||
url: 'https://example.test/1',
|
||||
labels: [],
|
||||
updatedAt: '2026-09-25T00:00:00Z',
|
||||
author: null,
|
||||
repoId: 'repo-1'
|
||||
}
|
||||
],
|
||||
sources: { issues: null, prs: null, originCandidate: null, upstreamCandidate: null }
|
||||
}
|
||||
}
|
||||
|
||||
const projectViewRequest = {
|
||||
owner: 'acme',
|
||||
ownerType: 'organization' as const,
|
||||
projectNumber: 1,
|
||||
viewId: 'view-1'
|
||||
}
|
||||
const projectViewCacheKey1 = projectViewCacheKey('organization', 'acme', 1, 'view-1')
|
||||
|
||||
function projectTable(title: string): GitHubProjectTable {
|
||||
return {
|
||||
project: {
|
||||
id: 'project-1',
|
||||
owner: 'acme',
|
||||
ownerType: 'organization',
|
||||
number: 1,
|
||||
title,
|
||||
url: 'https://github.com/orgs/acme/projects/1'
|
||||
},
|
||||
selectedView: {
|
||||
id: 'view-1',
|
||||
number: 1,
|
||||
name: 'Table',
|
||||
layout: 'TABLE_LAYOUT',
|
||||
filter: '',
|
||||
fields: [],
|
||||
groupByFields: [],
|
||||
sortByFields: []
|
||||
},
|
||||
rows: [],
|
||||
totalCount: 0,
|
||||
parentFieldDropped: false
|
||||
}
|
||||
}
|
||||
|
||||
const strongerWorkItemOptions: FetchOptions[] = [
|
||||
{ force: true },
|
||||
{ force: true, noCache: true },
|
||||
{ force: true, requireComplete: true }
|
||||
]
|
||||
|
||||
describe('GitHub provider request upgrade coalescing', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
resetRemoteRuntimeMocks()
|
||||
})
|
||||
|
||||
it.each(strongerWorkItemOptions)('coalesces twenty work-item upgrades: %j', async (options) => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const fresh = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
mockApi.gh.listWorkItems.mockReturnValueOnce(weak.promise).mockReturnValue(fresh.promise)
|
||||
const first = store.getState().fetchWorkItems('repo-1', '/repo', 24, '')
|
||||
const settled = vi.fn()
|
||||
const followers = Array.from({ length: 20 }, () =>
|
||||
store.getState().fetchWorkItems('repo-1', '/repo', 24, '', options).then(settled)
|
||||
)
|
||||
weak.resolve(workItems('weak'))
|
||||
await first
|
||||
await vi.waitFor(() =>
|
||||
expect(mockApi.gh.listWorkItems.mock.calls.length).toBeGreaterThanOrEqual(2)
|
||||
)
|
||||
expect(settled).not.toHaveBeenCalled()
|
||||
fresh.resolve(workItems('fresh'))
|
||||
await Promise.all(followers)
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(2)
|
||||
expect(settled).toHaveBeenCalledTimes(20)
|
||||
expect(settled.mock.calls.every(([rows]) => rows[0].title === 'fresh')).toBe(true)
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenLastCalledWith({
|
||||
repoPath: '/repo',
|
||||
repoId: 'repo-1',
|
||||
limit: 24,
|
||||
query: undefined,
|
||||
...(options.noCache ? { noCache: true } : {})
|
||||
})
|
||||
})
|
||||
|
||||
it('never joins a weaker replacement and never waits out more than one', async () => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const forced = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const strict = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
mockApi.gh.listWorkItems
|
||||
.mockReturnValueOnce(weak.promise)
|
||||
.mockReturnValueOnce(forced.promise)
|
||||
.mockReturnValue(strict.promise)
|
||||
const first = store.getState().fetchWorkItems('repo-1', '/repo', 24, '')
|
||||
const forcedFetch = store.getState().fetchWorkItems('repo-1', '/repo', 24, '', { force: true })
|
||||
const strictSettled = vi.fn()
|
||||
const strictFollowers = Array.from({ length: 20 }, () =>
|
||||
store
|
||||
.getState()
|
||||
.fetchWorkItems('repo-1', '/repo', 24, '', {
|
||||
force: true,
|
||||
noCache: true,
|
||||
requireComplete: true
|
||||
})
|
||||
.then(strictSettled)
|
||||
)
|
||||
weak.resolve(workItems('weak'))
|
||||
await first
|
||||
// Why: the strict callers must reach the bridge without waiting out the weaker
|
||||
// replacement too — the upgrade wait is bounded, so a repeating weaker refresh
|
||||
// can never starve them. They still share exactly one strict request.
|
||||
await vi.waitFor(() => expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(3))
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ noCache: true })
|
||||
)
|
||||
expect(strictSettled).not.toHaveBeenCalled()
|
||||
strict.resolve(workItems('strict'))
|
||||
await Promise.all(strictFollowers)
|
||||
forced.resolve(workItems('forced'))
|
||||
await expect(forcedFetch).resolves.toEqual(workItems('forced').items)
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(3)
|
||||
expect(strictSettled).toHaveBeenCalledTimes(20)
|
||||
expect(strictSettled.mock.calls.every(([rows]) => rows[0].title === 'strict')).toBe(true)
|
||||
})
|
||||
|
||||
it('keeps an invalidated request from removing its replacement dedupe entry', async () => {
|
||||
const store = createTestStore()
|
||||
const stale = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const fresh = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
mockApi.gh.listWorkItems.mockReturnValueOnce(stale.promise).mockReturnValue(fresh.promise)
|
||||
const first = store.getState().fetchWorkItems('repo-1', '/repo', 24, '')
|
||||
store.getState().evictGitHubRepoCaches('repo-1', '/repo')
|
||||
const replacement = store.getState().fetchWorkItems('repo-1', '/repo', 24, '', { force: true })
|
||||
stale.resolve(workItems('stale'))
|
||||
await first
|
||||
const joined = store.getState().fetchWorkItems('repo-1', '/repo', 24, '', { force: true })
|
||||
fresh.resolve(workItems('fresh'))
|
||||
await expect(Promise.all([replacement, joined])).resolves.toEqual([
|
||||
workItems('fresh').items,
|
||||
workItems('fresh').items
|
||||
])
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('rejects partial results for every complete-result waiter and allows a later retry', async () => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const partial = {
|
||||
...workItems('partial'),
|
||||
errors: { issues: { type: 'network_error' as const, message: 'offline' } }
|
||||
}
|
||||
mockApi.gh.listWorkItems.mockReturnValueOnce(weak.promise).mockResolvedValue(partial)
|
||||
const first = store.getState().fetchWorkItems('repo-1', '/repo', 24, '')
|
||||
const followers = Array.from({ length: 20 }, () =>
|
||||
store.getState().fetchWorkItems('repo-1', '/repo', 24, '', {
|
||||
force: true,
|
||||
requireComplete: true
|
||||
})
|
||||
)
|
||||
const settled = Promise.allSettled(followers)
|
||||
weak.resolve(workItems('weak'))
|
||||
await first
|
||||
expect((await settled).every((result) => result.status === 'rejected')).toBe(true)
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(2)
|
||||
mockApi.gh.listWorkItems.mockResolvedValue(workItems('recovered'))
|
||||
await expect(
|
||||
store.getState().fetchWorkItems('repo-1', '/repo', 24, '', {
|
||||
force: true,
|
||||
requireComplete: true
|
||||
})
|
||||
).resolves.toEqual(workItems('recovered').items)
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(3)
|
||||
})
|
||||
|
||||
it.each([{ force: true }, { noCache: true }])(
|
||||
'coalesces twenty check upgrades: %j',
|
||||
async (options) => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<PRCheckDetail[]>()
|
||||
const fresh = Promise.withResolvers<PRCheckDetail[]>()
|
||||
mockApi.gh.prChecks.mockReturnValueOnce(weak.promise).mockReturnValue(fresh.promise)
|
||||
const first = store.getState().fetchPRChecks('/repo', 1, 'main', 'sha')
|
||||
const settled = vi.fn()
|
||||
const followers = Array.from({ length: 20 }, () =>
|
||||
store.getState().fetchPRChecks('/repo', 1, 'main', 'sha', undefined, options).then(settled)
|
||||
)
|
||||
weak.resolve([])
|
||||
await first
|
||||
await vi.waitFor(() =>
|
||||
expect(mockApi.gh.prChecks.mock.calls.length).toBeGreaterThanOrEqual(2)
|
||||
)
|
||||
expect(settled).not.toHaveBeenCalled()
|
||||
const checks: PRCheckDetail[] = [
|
||||
{ name: 'fresh', status: 'completed', conclusion: 'success', url: null }
|
||||
]
|
||||
fresh.resolve(checks)
|
||||
await Promise.all(followers)
|
||||
expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(2)
|
||||
expect(settled).toHaveBeenCalledTimes(20)
|
||||
expect(settled.mock.calls.every(([rows]) => rows === checks)).toBe(true)
|
||||
}
|
||||
)
|
||||
|
||||
// Why: the bounded wait lets a force-only request (which still allows gh's own cache to answer)
|
||||
// run beside a noCache one. Whichever settles last used to win the cache unconditionally.
|
||||
it('keeps a late weaker work-item reply from burying the stronger result', async () => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const forced = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
const strict = Promise.withResolvers<ListWorkItemsResult<GitHubWorkItem>>()
|
||||
mockApi.gh.listWorkItems
|
||||
.mockReturnValueOnce(weak.promise)
|
||||
.mockReturnValueOnce(forced.promise)
|
||||
.mockReturnValue(strict.promise)
|
||||
const first = store.getState().fetchWorkItems('repo-1', '/repo', 24, '')
|
||||
const forcedFetch = store.getState().fetchWorkItems('repo-1', '/repo', 24, '', { force: true })
|
||||
const strictFetch = store
|
||||
.getState()
|
||||
.fetchWorkItems('repo-1', '/repo', 24, '', { force: true, noCache: true })
|
||||
weak.resolve(workItems('weak'))
|
||||
await first
|
||||
await vi.waitFor(() => expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(3))
|
||||
|
||||
strict.resolve(workItems('fresh'))
|
||||
await expect(strictFetch).resolves.toEqual(workItems('fresh').items)
|
||||
forced.resolve(workItems('gh-cached'))
|
||||
await expect(forcedFetch).resolves.toEqual(workItems('gh-cached').items)
|
||||
|
||||
expect(store.getState().getCachedWorkItems('repo-1', 24, '', '/repo')).toEqual(
|
||||
workItems('fresh').items
|
||||
)
|
||||
// Why: the stale payload must not be what isFresh hands back for the rest of the TTL either.
|
||||
await expect(store.getState().fetchWorkItems('repo-1', '/repo', 24, '')).resolves.toEqual(
|
||||
workItems('fresh').items
|
||||
)
|
||||
expect(mockApi.gh.listWorkItems).toHaveBeenCalledTimes(3)
|
||||
})
|
||||
|
||||
it('keeps a superseded check reply from rewinding the cached run state', async () => {
|
||||
const store = createTestStore()
|
||||
const plain = Promise.withResolvers<PRCheckDetail[]>()
|
||||
const noCacheOnly = Promise.withResolvers<PRCheckDetail[]>()
|
||||
const forcedOnly = Promise.withResolvers<PRCheckDetail[]>()
|
||||
mockApi.gh.prChecks
|
||||
.mockReturnValueOnce(plain.promise)
|
||||
.mockReturnValueOnce(noCacheOnly.promise)
|
||||
.mockReturnValue(forcedOnly.promise)
|
||||
const first = store.getState().fetchPRChecks('/repo', 1, 'main', 'sha')
|
||||
// Why: force and noCache are incomparable, so each waits out the plain request and the second
|
||||
// one then breaks out of the bounded wait while the first is still in flight.
|
||||
const noCacheFetch = store
|
||||
.getState()
|
||||
.fetchPRChecks('/repo', 1, 'main', 'sha', undefined, { noCache: true })
|
||||
const forcedFetch = store
|
||||
.getState()
|
||||
.fetchPRChecks('/repo', 1, 'main', 'sha', undefined, { force: true })
|
||||
plain.resolve([])
|
||||
await first
|
||||
await vi.waitFor(() => expect(mockApi.gh.prChecks).toHaveBeenCalledTimes(3))
|
||||
|
||||
const newer: PRCheckDetail[] = [
|
||||
{ name: 'build', status: 'completed', conclusion: 'success', url: null }
|
||||
]
|
||||
const older: PRCheckDetail[] = [
|
||||
{ name: 'build', status: 'in_progress', conclusion: null, url: null }
|
||||
]
|
||||
forcedOnly.resolve(newer)
|
||||
await expect(forcedFetch).resolves.toEqual(newer)
|
||||
noCacheOnly.resolve(older)
|
||||
await expect(noCacheFetch).resolves.toEqual(older)
|
||||
|
||||
expect(Object.values(store.getState().checksCache).map((entry) => entry.data)).toEqual([newer])
|
||||
})
|
||||
|
||||
it('keeps a superseded project failure from stamping the known view key', async () => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<GetProjectViewTableResult>()
|
||||
const replacementWeak = Promise.withResolvers<GetProjectViewTableResult>()
|
||||
const forcedResult = Promise.withResolvers<GetProjectViewTableResult>()
|
||||
mockApi.gh.getProjectViewTable
|
||||
.mockReturnValueOnce(weak.promise)
|
||||
.mockReturnValueOnce(replacementWeak.promise)
|
||||
.mockReturnValue(forcedResult.promise)
|
||||
|
||||
const first = store.getState().fetchProjectViewTable(projectViewRequest)
|
||||
const forcedFetch = store.getState().fetchProjectViewTable(projectViewRequest, { force: true })
|
||||
// Why: only a non-forced entry can be superseded mid-flight, and it has to appear in the
|
||||
// microtask after the first request clears the key — before the forced waiter re-checks it.
|
||||
const pending = inflightProjectViewRequests.get(
|
||||
projectViewRequestKey(projectViewRequest, 'local')
|
||||
)
|
||||
expect(pending).toBeDefined()
|
||||
let replacementFetch: Promise<GetProjectViewTableResult> | undefined
|
||||
void pending?.promise.then(() => {
|
||||
replacementFetch = store.getState().fetchProjectViewTable(projectViewRequest)
|
||||
})
|
||||
|
||||
const staleError = { type: 'network_error' as const, message: 'first attempt offline' }
|
||||
weak.resolve({ ok: false, error: staleError })
|
||||
await first
|
||||
await vi.waitFor(() => expect(mockApi.gh.getProjectViewTable).toHaveBeenCalledTimes(3))
|
||||
expect(store.getState().projectViewCache[projectViewCacheKey1]).toMatchObject({
|
||||
error: staleError
|
||||
})
|
||||
|
||||
forcedResult.resolve({ ok: true, data: projectTable('forced') })
|
||||
await expect(forcedFetch).resolves.toEqual({ ok: true, data: projectTable('forced') })
|
||||
const error = { type: 'network_error' as const, message: 'offline' }
|
||||
replacementWeak.resolve({ ok: false, error })
|
||||
await expect(replacementFetch).resolves.toEqual({ ok: false, error })
|
||||
|
||||
expect(store.getState().projectViewCache[projectViewCacheKey1]).toMatchObject({
|
||||
data: projectTable('forced')
|
||||
})
|
||||
expect(store.getState().projectViewCache[projectViewCacheKey1].error).toBeUndefined()
|
||||
})
|
||||
})
|
||||
@@ -13,31 +13,56 @@ export type InflightPR = {
|
||||
}
|
||||
export type InflightChecks = {
|
||||
promise: Promise<PRCheckDetail[]>
|
||||
requestId: number
|
||||
force: boolean
|
||||
noCache: boolean
|
||||
}
|
||||
export type InflightWorkItems = {
|
||||
promise: Promise<readonly GitHubWorkItem[]>
|
||||
requestId: number
|
||||
force: boolean
|
||||
noCache: boolean
|
||||
requireComplete: boolean
|
||||
}
|
||||
export type InflightProjectView = {
|
||||
promise: Promise<GetProjectViewTableResult>
|
||||
requestId: number
|
||||
force: boolean
|
||||
}
|
||||
|
||||
export const inflightPRRequests = new Map<string, InflightPR>()
|
||||
export const inflightIssueRequests = new Map<string, Promise<IssueInfo | null>>()
|
||||
export const inflightChecksRequests = new Map<string, InflightChecks>()
|
||||
export const inflightCommentsRequests = new Map<string, Promise<PRComment[]>>()
|
||||
export const inflightWorkItemsRequests = new Map<string, InflightWorkItems>()
|
||||
export const inflightProjectViewRequests = new Map<
|
||||
string,
|
||||
{ promise: Promise<GetProjectViewTableResult>; force: boolean }
|
||||
>()
|
||||
export const inflightProjectViewRequests = new Map<string, InflightProjectView>()
|
||||
export const prRequestGenerations = new Map<string, number>()
|
||||
export const prRefreshStartedHostedReviewEntries = new Map<
|
||||
string,
|
||||
AppState['hostedReviewCache'][string] | undefined
|
||||
>()
|
||||
|
||||
let providerRequestSequence = 0
|
||||
|
||||
/** Stamp identifying one provider request, captured before it awaits so it can recheck ownership after. */
|
||||
export function nextProviderRequestId(): number {
|
||||
providerRequestSequence += 1
|
||||
return providerRequestSequence
|
||||
}
|
||||
|
||||
/**
|
||||
* Why: the upgrade wait is bounded, so a stronger request can run beside a weaker one for the same
|
||||
* key. Only the request the key currently resolves to may write that key's cache — otherwise a late
|
||||
* weaker reply overwrites the stronger request's fresher result under a brand-new `fetchedAt`.
|
||||
*/
|
||||
export function ownsInflightRequest<T extends { requestId: number }>(
|
||||
registry: ReadonlyMap<string, T>,
|
||||
key: string,
|
||||
requestId: number
|
||||
): boolean {
|
||||
return registry.get(key)?.requestId === requestId
|
||||
}
|
||||
|
||||
export function _getGitHubPRRequestGenerationCountForTest(): number {
|
||||
return prRequestGenerations.size
|
||||
}
|
||||
|
||||
@@ -13,6 +13,8 @@ import { isFresh, withBoundedCacheEntry, WORK_ITEMS_CACHE_TTL } from './cache-po
|
||||
import {
|
||||
acquireProviderRequestSlot as acquireWorkItemSlot,
|
||||
inflightWorkItemsRequests,
|
||||
nextProviderRequestId,
|
||||
ownsInflightRequest,
|
||||
releaseProviderRequestSlot as releaseWorkItemSlot
|
||||
} from './request-coordination'
|
||||
import { findRepoForGitHubOwner } from './repository-routing'
|
||||
@@ -109,20 +111,29 @@ export const createWorkItemFetchActions = (
|
||||
options?.sourceContext
|
||||
)
|
||||
const inflightKey = workItemsInflightRequestKey(key, requestContext.target)
|
||||
const existing = inflightWorkItemsRequests.get(inflightKey)
|
||||
if (existing) {
|
||||
let waitedForUpgrade = false
|
||||
for (;;) {
|
||||
const existing = inflightWorkItemsRequests.get(inflightKey)
|
||||
if (!existing) {
|
||||
break
|
||||
}
|
||||
// Why: a forcing/noCache caller must not dedupe to a weaker in-flight fetch (noCache is stricter — it must bypass gh api's cache too).
|
||||
if (
|
||||
const weakerThanRequested =
|
||||
(options?.force && !existing.force) ||
|
||||
(options?.noCache && !existing.noCache) ||
|
||||
(options?.requireComplete && !existing.requireComplete)
|
||||
) {
|
||||
await existing.promise.catch(() => {})
|
||||
} else {
|
||||
if (!weakerThanRequested) {
|
||||
return existing.promise
|
||||
}
|
||||
// Why: wait out one weaker request so peers can share the upgrade, but never twice — a steady stream of weaker callers would otherwise starve this one forever.
|
||||
if (waitedForUpgrade) {
|
||||
break
|
||||
}
|
||||
waitedForUpgrade = true
|
||||
await existing.promise.catch(() => {})
|
||||
}
|
||||
|
||||
const requestId = nextProviderRequestId()
|
||||
const request = (async () => {
|
||||
await acquireWorkItemSlot()
|
||||
try {
|
||||
@@ -163,6 +174,12 @@ export const createWorkItemFetchActions = (
|
||||
if (get().workItemsInvalidationNonce !== requestInvalidationNonce) {
|
||||
return items
|
||||
}
|
||||
// Why: a stronger request may have replaced us on this key while we were awaiting (a
|
||||
// force-only caller still lets gh's own cache answer), so writing here would bury its
|
||||
// fresher rows under a new fetchedAt and keep isFresh serving them for the whole TTL.
|
||||
if (!ownsInflightRequest(inflightWorkItemsRequests, inflightKey, requestId)) {
|
||||
return items
|
||||
}
|
||||
// Why: TaskPage useShallow-selects cache entry refs. A new { ...entry, fetchedAt }
|
||||
// still remaps every visible row. IPC structuredClone rebuilds nested records, so
|
||||
// data === previous.data never holds — reconcile structurally, then either mutate
|
||||
@@ -220,12 +237,16 @@ export const createWorkItemFetchActions = (
|
||||
throw err
|
||||
} finally {
|
||||
releaseWorkItemSlot()
|
||||
}
|
||||
})().finally(() => {
|
||||
if (ownsInflightRequest(inflightWorkItemsRequests, inflightKey, requestId)) {
|
||||
inflightWorkItemsRequests.delete(inflightKey)
|
||||
}
|
||||
})()
|
||||
})
|
||||
|
||||
inflightWorkItemsRequests.set(inflightKey, {
|
||||
promise: request,
|
||||
requestId,
|
||||
force: Boolean(options?.force),
|
||||
noCache: Boolean(options?.noCache),
|
||||
requireComplete: Boolean(options?.requireComplete)
|
||||
|
||||
@@ -73,7 +73,7 @@ describe('createGitHubSlice.fetchProjectViewTable coordination', () => {
|
||||
expect(mockApi.gh.getProjectViewTable).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('lets each forced waiter start after a weaker request settles', async () => {
|
||||
it('shares one fresh request across twenty forced waiters after a weaker request', async () => {
|
||||
const store = createTestStore()
|
||||
const weak = Promise.withResolvers<GetProjectViewTableResult>()
|
||||
mockApi.gh.getProjectViewTable
|
||||
@@ -81,12 +81,16 @@ describe('createGitHubSlice.fetchProjectViewTable coordination', () => {
|
||||
.mockResolvedValue({ ok: true, data: makeTable('forced') })
|
||||
|
||||
const first = store.getState().fetchProjectViewTable(request)
|
||||
const forcedOne = store.getState().fetchProjectViewTable(request, { force: true })
|
||||
const forcedTwo = store.getState().fetchProjectViewTable(request, { force: true })
|
||||
const forced = Array.from({ length: 20 }, () =>
|
||||
store.getState().fetchProjectViewTable(request, { force: true })
|
||||
)
|
||||
weak.resolve({ ok: true, data: makeTable('weak') })
|
||||
|
||||
await expect(Promise.all([first, forcedOne, forcedTwo])).resolves.toHaveLength(3)
|
||||
expect(mockApi.gh.getProjectViewTable).toHaveBeenCalledTimes(3)
|
||||
await expect(first).resolves.toEqual({ ok: true, data: makeTable('weak') })
|
||||
await expect(Promise.all(forced)).resolves.toEqual(
|
||||
Array.from({ length: 20 }, () => ({ ok: true, data: makeTable('forced') }))
|
||||
)
|
||||
expect(mockApi.gh.getProjectViewTable).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('stamps a classified failure onto stale data only when the view key is known', async () => {
|
||||
|
||||
Reference in New Issue
Block a user