From 0a1ba0f987a1f7d095e022a6a5a145dcc0d0e2b2 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 1 Aug 2026 18:27:57 -0700 Subject: [PATCH] fix(P1-D): bound hosted-review lookups with a detachable deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `inflight` map in the hosted-review branch cache was only ever cleared when the lookup settled, and nothing bounded how long that took. One wedged provider call pinned its branch for the life of the process: every later poll joined the same dead promise, so the card loaded forever with no in-session recovery. Each lookup now runs under a 120s deadline. Nothing below the funnel can be cancelled, so the deadline detaches instead: the record is released, the callers get the last known review (or a timeout error), and the branch enters the existing failure backoff. The lookup keeps running and its answer is still adopted if it lands, so a slow-but-alive host converges rather than failing forever. A token identity keeps a detached lookup from evicting the record that replaced it, and a wall-clock sweep expires records whose timer never fired — main's timers are suspended across system sleep. `inflight` is capped independently of the completed cache. The failure backoff moves to its own module: it has a different lifetime from the answer cache and is what a deadline records against. --- .../hosted-review-branch-cache.test.ts | 203 +++++++++++++++ .../hosted-review-branch-cache.ts | 243 +++++++++++++----- .../hosted-review-lookup-backoff.ts | 60 +++++ .../hosted-review-refresh-pacing.ts | 15 ++ 4 files changed, 454 insertions(+), 67 deletions(-) create mode 100644 src/main/source-control/hosted-review-lookup-backoff.ts 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