diff --git a/src/main/source-control/hosted-review-branch-cache.test.ts b/src/main/source-control/hosted-review-branch-cache.test.ts index 80d9220aa12..519cbe4435b 100644 --- a/src/main/source-control/hosted-review-branch-cache.test.ts +++ b/src/main/source-control/hosted-review-branch-cache.test.ts @@ -5,8 +5,27 @@ import { invalidateHostedReviewBranchCache, withHostedReviewBranchCache } from './hosted-review-branch-cache' +import { + HOSTED_REVIEW_LOOKUP_DEADLINE_MS, + MAX_INFLIGHT_LOOKUPS +} from './hosted-review-refresh-pacing' const identity = { repoPath: '/repo', connectionId: null, branch: 'feature/x' } +const START = 1_000_000 + +/** A lookup that never settles — the wedged provider this file's deadline exists for. */ +function stuckLookup() { + let resolve: (review: HostedReviewInfo | null) => void = () => {} + let reject: (error: unknown) => void = () => {} + const lookup = vi.fn( + () => + new Promise((settle, fail) => { + resolve = settle + reject = fail + }) + ) + return { lookup, resolve: (review) => resolve(review), reject: (error) => reject(error) } +} const openReview: HostedReviewInfo = { provider: 'github', @@ -371,4 +390,188 @@ describe('hosted review branch cache (#11532)', () => { expect(lookup).toHaveBeenCalledTimes(2) }) }) + + describe('in-flight deadline (P1-D)', () => { + it('releases a stuck lookup at the deadline instead of pinning the branch', async () => { + const { lookup } = stuckLookup() + + const request = withHostedReviewBranchCache(identity, { headOid: null }, lookup) + const rejects = expect(request).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await rejects + + // The dead lookup is no longer joinable, so the branch can recover in + // session rather than staying pinned for the life of the process. + await expect( + withHostedReviewBranchCache(identity, { headOid: null }, lookup) + ).rejects.toThrow(/backing off/) + expect(lookup).toHaveBeenCalledTimes(1) + }) + + it('lets a later poll succeed after a lookup times out', async () => { + const { lookup } = stuckLookup() + + const rejects = expect( + withHostedReviewBranchCache(identity, { headOid: null }, lookup) + ).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await rejects + + await vi.advanceTimersByTimeAsync(60_001) + const recovered = vi.fn(async () => openReview) + await expect( + withHostedReviewBranchCache(identity, { headOid: null }, recovered) + ).resolves.toEqual(openReview) + expect(recovered).toHaveBeenCalledTimes(1) + }) + + it('keeps the last known review when a refresh times out', async () => { + await withHostedReviewBranchCache(identity, { headOid: null }, async () => openReview) + vi.setSystemTime(START + 60_001) + + const { lookup } = stuckLookup() + const request = withHostedReviewBranchCache(identity, { headOid: null }, lookup) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + + // A wedged refresh must not blank the card any more than a failing one does. + await expect(request).resolves.toEqual(openReview) + }) + + it('adopts a lookup that lands after its deadline', async () => { + const { lookup, resolve } = stuckLookup() + + const rejects = expect( + withHostedReviewBranchCache(identity, { headOid: null }, lookup) + ).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await rejects + + resolve(openReview) + await vi.advanceTimersByTimeAsync(0) + + // The detached call still cost the quota, so its answer is worth keeping: + // a slow-but-alive host converges instead of failing on every poll. + const next = vi.fn(async () => null) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, next)).resolves.toEqual( + openReview + ) + expect(next).not.toHaveBeenCalled() + }) + + it('clears the backoff when a timed-out lookup finally answers', async () => { + const first = stuckLookup() + const firstRejects = expect( + withHostedReviewBranchCache(identity, { headOid: null }, first.lookup) + ).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await firstRejects + + // A second timeout doubles the window to 120s, so the backoff outlives the + // found-review TTL — that is what makes the clearing observable. + await vi.advanceTimersByTimeAsync(60_001) + const second = stuckLookup() + const secondRejects = expect( + withHostedReviewBranchCache(identity, { headOid: null }, second.lookup) + ).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await secondRejects + + second.resolve(openReview) + await vi.advanceTimersByTimeAsync(0) + + // Past the found-review TTL but still inside the doubled backoff window: a + // lookup runs, which it could not do if the late answer had left it armed. + await vi.advanceTimersByTimeAsync(60_001) + const recovered = vi.fn(async () => mergedReview) + await expect( + withHostedReviewBranchCache(identity, { headOid: null }, recovered) + ).resolves.toEqual(mergedReview) + expect(recovered).toHaveBeenCalledTimes(1) + }) + + it('does not let a straggler overwrite an answer newer than itself', async () => { + const stale = stuckLookup() + const rejects = expect( + withHostedReviewBranchCache(identity, { headOid: null }, stale.lookup) + ).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await rejects + + await vi.advanceTimersByTimeAsync(60_001) + await withHostedReviewBranchCache(identity, { headOid: null }, async () => mergedReview) + + stale.resolve(openReview) + await vi.advanceTimersByTimeAsync(0) + + const next = vi.fn(async () => null) + await expect(withHostedReviewBranchCache(identity, { headOid: null }, next)).resolves.toEqual( + mergedReview + ) + expect(next).not.toHaveBeenCalled() + }) + + it('does not let a straggler evict the lookup that replaced it', async () => { + const stale = stuckLookup() + const rejects = expect( + withHostedReviewBranchCache(identity, { headOid: null }, stale.lookup) + ).rejects.toThrow(/timed out/) + await vi.advanceTimersByTimeAsync(HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + await rejects + + await vi.advanceTimersByTimeAsync(60_001) + const replacement = stuckLookup() + const first = withHostedReviewBranchCache(identity, { headOid: null }, replacement.lookup) + + // The straggler settling must clear its own record, not the replacement's — + // otherwise the next caller starts a duplicate provider call. + stale.reject(new Error('late failure')) + await vi.advanceTimersByTimeAsync(0) + + const second = withHostedReviewBranchCache(identity, { headOid: null }, replacement.lookup) + replacement.resolve(openReview) + + await expect(first).resolves.toEqual(openReview) + await expect(second).resolves.toEqual(openReview) + expect(replacement.lookup).toHaveBeenCalledTimes(1) + }) + + it('releases a caller by wall clock when the deadline timer never fires', async () => { + const { lookup } = stuckLookup() + + const request = withHostedReviewBranchCache(identity, { headOid: null }, lookup) + const rejects = expect(request).rejects.toThrow(/timed out/) + // Why: main-process timers are suspended across a system sleep, so age — + // not setTimeout — is what has to bound the pin. + vi.setSystemTime(START + HOSTED_REVIEW_LOOKUP_DEADLINE_MS + 1) + + await expect( + withHostedReviewBranchCache(identity, { headOid: null }, lookup) + ).rejects.toThrow(/backing off/) + await rejects + expect(lookup).toHaveBeenCalledTimes(1) + }) + + it('bounds the in-flight map independently of the completed cache', async () => { + const { lookup } = stuckLookup() + const branchAt = (index: number) => ({ ...identity, branch: `feature/${index}` }) + const swallow = (promise: Promise): void => { + void promise.catch(() => {}) + } + + for (let index = 0; index <= MAX_INFLIGHT_LOOKUPS; index += 1) { + swallow(withHostedReviewBranchCache(branchAt(index), { headOid: null }, lookup)) + } + expect(lookup).toHaveBeenCalledTimes(MAX_INFLIGHT_LOOKUPS + 1) + + // The oldest stuck record was evicted, so its branch is no longer joinable. + swallow(withHostedReviewBranchCache(branchAt(0), { headOid: null }, lookup)) + expect(lookup).toHaveBeenCalledTimes(MAX_INFLIGHT_LOOKUPS + 2) + + // The newest is still tracked, so concurrent callers still collapse onto it. + swallow( + withHostedReviewBranchCache(branchAt(MAX_INFLIGHT_LOOKUPS), { headOid: null }, lookup) + ) + expect(lookup).toHaveBeenCalledTimes(MAX_INFLIGHT_LOOKUPS + 2) + }) + }) }) diff --git a/src/main/source-control/hosted-review-branch-cache.ts b/src/main/source-control/hosted-review-branch-cache.ts index b1d54774ca8..30faab9ad95 100644 --- a/src/main/source-control/hosted-review-branch-cache.ts +++ b/src/main/source-control/hosted-review-branch-cache.ts @@ -1,10 +1,17 @@ import type { HostedReviewInfo } from '../../shared/hosted-review' +import { + __resetHostedReviewLookupBackoffForTests, + backoffUntil, + clearFailures, + dropFailuresWithPrefix, + noteFailure +} from './hosted-review-lookup-backoff' import { ACTIVE_CLAIM_TTL_MS, ACTIVE_REFRESH_INTERVAL_MS, - lookupBackoffDelayMs, - LOOKUP_BACKOFF_MAX_MS, + HOSTED_REVIEW_LOOKUP_DEADLINE_MS, MAX_ACTIVE_BRANCHES, + MAX_INFLIGHT_LOOKUPS, NO_REVIEW_REFRESH_INTERVAL_MS } from './hosted-review-refresh-pacing' @@ -32,9 +39,17 @@ type CacheEntry = { headOid: string | null } +type InflightRecord = { + /** Identity, so a detached lookup can only ever clear its own entry. */ + token: object + startedAt: number + promise: Promise + /** Releases the callers and unpins the branch; idempotent. */ + expire: () => void +} + const entries = new Map() -const inflight = new Map>() -const failureBackoff = new Map() +const inflight = new Map() /** Branches a caller reported as its current selection, least recent first. */ const activeClaims = new Map() /** Bumped per repo on invalidation so a lookup that predates it cannot store. */ @@ -151,34 +166,6 @@ function storeEntry(key: string, entry: CacheEntry): void { } } -function backoffUntil(key: string): number | null { - // Why: a lapsed window keeps its failure count, otherwise the very act of - // retrying resets the escalation and the backoff never grows past the base. - const entry = failureBackoff.get(key) - return entry !== undefined && entry.until > Date.now() ? entry.until : null -} - -function noteFailure(key: string): void { - const now = Date.now() - for (const [candidate, entry] of failureBackoff) { - // Why: only counts that lapsed a full max window ago are stale enough to - // forget; anything more eager would undo the escalation above. - if (now - entry.until > LOOKUP_BACKOFF_MAX_MS) { - failureBackoff.delete(candidate) - } - } - const failures = (failureBackoff.get(key)?.failures ?? 0) + 1 - failureBackoff.delete(key) - failureBackoff.set(key, { until: now + lookupBackoffDelayMs(failures), failures }) - while (failureBackoff.size > MAX_ENTRIES) { - const oldest = failureBackoff.keys().next().value - if (oldest === undefined) { - break - } - failureBackoff.delete(oldest) - } -} - function scopeGeneration(scope: string): number { return scopeGenerations.get(scope) ?? 0 } @@ -198,6 +185,47 @@ function bumpScopeGeneration(scope: string): void { } } +/** Clears the key's in-flight record only if it is still this lookup's. */ +function releaseInflight(key: string, token: object): boolean { + if (inflight.get(key)?.token !== token) { + return false + } + inflight.delete(key) + return true +} + +/** + * Expires records that outlived the deadline without their timer firing. Main's + * timers are suspended across a system sleep, so wall-clock age — not + * `setTimeout` alone — is what actually bounds how long a branch stays pinned. + */ +function expireOverdueInflight(now: number): void { + let overdue: InflightRecord[] | undefined + for (const record of inflight.values()) { + if (now - record.startedAt >= HOSTED_REVIEW_LOOKUP_DEADLINE_MS) { + overdue ??= [] + overdue.push(record) + } + } + // Expire after the walk: each one deletes its own entry from the map. + for (const record of overdue ?? []) { + record.expire() + } +} + +function trackInflight(key: string, record: InflightRecord): void { + inflight.set(key, record) + while (inflight.size > MAX_INFLIGHT_LOOKUPS) { + const oldest = inflight.keys().next().value + if (oldest === undefined) { + break + } + // Why: drop the record without expiring it — its own deadline still + // releases its callers, and evicting is about memory, not about failing. + inflight.delete(oldest) + } +} + /** * Drops every cached answer for a repo. Called when Orca itself opens a review, * so the new one is visible immediately instead of after the no-review interval. @@ -214,22 +242,127 @@ export function invalidateHostedReviewBranchCache( entries.delete(key) } } - for (const key of failureBackoff.keys()) { - if (key.startsWith(prefix)) { - failureBackoff.delete(key) - } - } + dropFailuresWithPrefix(prefix) } /** @internal - exposed for tests only */ export function __resetHostedReviewBranchCacheForTests(): void { entries.clear() inflight.clear() - failureBackoff.clear() + __resetHostedReviewLookupBackoffForTests() activeClaims.clear() scopeGenerations.clear() } +/** A newer lookup has already answered for this key since `startedAt`. */ +function answeredSince(key: string, startedAt: number): boolean { + const current = entries.get(key) + return current !== undefined && current.fetchedAt >= startedAt +} + +/** + * Runs one lookup under a deadline. Nothing below this can be cancelled, so the + * deadline *detaches* instead: the branch is unpinned and the callers hear a + * failure, while the lookup keeps running and its answer is still adopted if it + * ever lands. That is what lets a wedged host recover in-session (P1-D). + */ +function startLookup( + key: string, + scope: string, + headOid: string | null, + lookup: () => Promise +): Promise { + const startedAt = Date.now() + const generation = scopeGeneration(scope) + const token = {} + /** The deadline released the callers; the lookup itself runs on, detached. */ + let timedOut = false + let completed = false + let release: (review: HostedReviewInfo | null) => void = () => {} + let fail: (error: unknown) => void = () => {} + const promise = new Promise((resolve, reject) => { + release = resolve + fail = reject + }) + let timer: ReturnType | undefined + + const expire = (): void => { + if (timedOut || completed) { + return + } + timedOut = true + if (timer !== undefined) { + clearTimeout(timer) + } + // Why: only the lookup still serving this key shapes the backoff. A record + // already replaced — or dropped by the size cap — has a live successor, and + // penalising the branch would slow down the retry that is already running. + if (releaseInflight(key, token)) { + noteFailure(key) + } + const stale = entries.get(key) + if (stale) { + release(stale.review) + return + } + fail( + new Error( + `Hosted review lookup timed out after ${Math.round( + HOSTED_REVIEW_LOOKUP_DEADLINE_MS / 1000 + )}s. It will be retried on a later poll.` + ) + ) + } + + timer = setTimeout(expire, HOSTED_REVIEW_LOOKUP_DEADLINE_MS) + if (typeof timer === 'object' && 'unref' in timer) { + timer.unref() + } + // Why: track before running, so a lookup that throws synchronously clears the + // record it would otherwise leave behind for a full deadline. + trackInflight(key, { token, startedAt, promise, expire }) + + void (async () => { + try { + const review = await lookup() + // Why: a review created while this lookup was out makes its answer older + // than the invalidation; storing it would re-pin the stale "no review". + // A timed-out lookup additionally yields to whatever answered after it + // started, so a slow straggler cannot overwrite the current answer. + if (generation === scopeGeneration(scope) && !(timedOut && answeredSince(key, startedAt))) { + storeEntry(key, { review, fetchedAt: Date.now(), headOid }) + // A late answer proves the provider recovered, so stop backing off. + clearFailures(key) + } + release(review) + } catch (error) { + // Why: the deadline already counted this lookup as a failure and released + // its callers; counting the late rejection again would double-escalate. + if (timedOut) { + return + } + noteFailure(key) + // Why: the last good review beats an error card here just as it does on + // the backed-off path — otherwise it blinks out on the first failure. + // An invalidation drops the entry, so this cannot revive a retired answer. + const stale = entries.get(key) + if (stale) { + release(stale.review) + return + } + fail(error) + } finally { + completed = true + if (timer !== undefined) { + clearTimeout(timer) + } + releaseInflight(key, token) + } + })() + + return promise +} + /** * Serves `lookup` through the shared cache: a fresh answer is reused, concurrent * callers share one in-flight lookup, and a failing branch backs off instead of @@ -242,6 +375,9 @@ export async function withHostedReviewBranchCache( ): Promise { const key = hostedReviewBranchCacheKey(identity) const headOid = options.headOid + // Why: sweep first, so a lookup whose deadline never fired cannot be joined — + // that is the pin this cache used to hold until the process restarted. + expireOverdueInflight(Date.now()) if (options.active === true && noteActiveClaim(key) && entries.get(key)?.review === null) { // Why: switching to a worktree is the user asking whether a review exists // yet, so the long no-review interval must not answer on their behalf. This @@ -258,7 +394,7 @@ export async function withHostedReviewBranchCache( const pending = inflight.get(key) if (pending) { - return pending + return pending.promise } const until = backoffUntil(key) @@ -275,32 +411,5 @@ export async function withHostedReviewBranchCache( ) } - const scope = repoScope(identity.repoPath, identity.connectionId) - const request = (async () => { - const generation = scopeGeneration(scope) - try { - const review = await lookup() - // Why: a review created while this lookup was out makes its answer older - // than the invalidation; storing it would re-pin the stale "no review". - if (generation === scopeGeneration(scope)) { - storeEntry(key, { review, fetchedAt: Date.now(), headOid }) - failureBackoff.delete(key) - } - return review - } catch (error) { - noteFailure(key) - // Why: the last good review beats an error card here just as it does on - // the backed-off path — otherwise it blinks out on the first failure. - // An invalidation drops the entry, so this cannot revive a retired answer. - const stale = entries.get(key) - if (stale) { - return stale.review - } - throw error - } finally { - inflight.delete(key) - } - })() - inflight.set(key, request) - return request + return startLookup(key, repoScope(identity.repoPath, identity.connectionId), headOid, lookup) } diff --git a/src/main/source-control/hosted-review-lookup-backoff.ts b/src/main/source-control/hosted-review-lookup-backoff.ts new file mode 100644 index 00000000000..9e95362e2b7 --- /dev/null +++ b/src/main/source-control/hosted-review-lookup-backoff.ts @@ -0,0 +1,60 @@ +import { lookupBackoffDelayMs, LOOKUP_BACKOFF_MAX_MS } from './hosted-review-refresh-pacing' + +/** + * Per-branch failure escalation for hosted-review lookups (#11532). + * + * Split out of the branch cache so the cache file stays about caching: this map + * has its own lifetime — a branch stays backed off after its cached answer is + * invalidated, and it is what a lookup deadline records against (P1-D). + */ + +/** Bounded like the answer cache, so a churn of dead branches cannot grow it. */ +const MAX_BACKOFF_ENTRIES = 500 + +const failureBackoff = new Map() + +export function backoffUntil(key: string): number | null { + // Why: a lapsed window keeps its failure count, otherwise the very act of + // retrying resets the escalation and the backoff never grows past the base. + const entry = failureBackoff.get(key) + return entry !== undefined && entry.until > Date.now() ? entry.until : null +} + +export function noteFailure(key: string): void { + const now = Date.now() + for (const [candidate, entry] of failureBackoff) { + // Why: only counts that lapsed a full max window ago are stale enough to + // forget; anything more eager would undo the escalation above. + if (now - entry.until > LOOKUP_BACKOFF_MAX_MS) { + failureBackoff.delete(candidate) + } + } + const failures = (failureBackoff.get(key)?.failures ?? 0) + 1 + failureBackoff.delete(key) + failureBackoff.set(key, { until: now + lookupBackoffDelayMs(failures), failures }) + while (failureBackoff.size > MAX_BACKOFF_ENTRIES) { + const oldest = failureBackoff.keys().next().value + if (oldest === undefined) { + break + } + failureBackoff.delete(oldest) + } +} + +/** A lookup answered, so the branch starts its next escalation from the base. */ +export function clearFailures(key: string): void { + failureBackoff.delete(key) +} + +export function dropFailuresWithPrefix(prefix: string): void { + for (const key of failureBackoff.keys()) { + if (key.startsWith(prefix)) { + failureBackoff.delete(key) + } + } +} + +/** @internal - exposed for tests only */ +export function __resetHostedReviewLookupBackoffForTests(): void { + failureBackoff.clear() +} diff --git a/src/main/source-control/hosted-review-refresh-pacing.ts b/src/main/source-control/hosted-review-refresh-pacing.ts index 7c2eaf3c666..d62eba32465 100644 --- a/src/main/source-control/hosted-review-refresh-pacing.ts +++ b/src/main/source-control/hosted-review-refresh-pacing.ts @@ -29,6 +29,21 @@ export const ACTIVE_CLAIM_TTL_MS = NO_REVIEW_REFRESH_INTERVAL_MS export const LOOKUP_BACKOFF_BASE_MS = 60_000 export const LOOKUP_BACKOFF_MAX_MS = 15 * 60_000 +/** + * Hard deadline for a single lookup. The steps underneath are individually + * bounded (30s `gh` exec, HTTP timeouts) but they chain, so this is a hang + * detector rather than a latency budget: a lookup that outlives it is wedged on + * something that has no timeout of its own, not merely slow. + */ +export const HOSTED_REVIEW_LOOKUP_DEADLINE_MS = 2 * 60_000 + +/** + * Why: a memory backstop, not a concurrency limit. Evicting a live lookup costs + * a duplicate provider call — the quota problem this cache exists to prevent — + * so the cap sits far above the branch count any client polls at once. + */ +export const MAX_INFLIGHT_LOOKUPS = 500 + // Why: capped so a long-lived failure settles at LOOKUP_BACKOFF_MAX_MS rather // than overflowing the exponent. const MAX_BACKOFF_DOUBLINGS = 4