fix(P1-D): bound hosted-review lookups with a detachable deadline

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.
This commit is contained in:
Jinjing
2026-08-01 18:27:57 -07:00
parent 8fc892dd02
commit 0a1ba0f987
4 changed files with 454 additions and 67 deletions
@@ -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<HostedReviewInfo | null>((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<unknown>): 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)
})
})
})
@@ -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<HostedReviewInfo | null>
/** Releases the callers and unpins the branch; idempotent. */
expire: () => void
}
const entries = new Map<string, CacheEntry>()
const inflight = new Map<string, Promise<HostedReviewInfo | null>>()
const failureBackoff = new Map<string, { until: number; failures: number }>()
const inflight = new Map<string, InflightRecord>()
/** Branches a caller reported as its current selection, least recent first. */
const activeClaims = new Map<string, number>()
/** 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<HostedReviewInfo | null>
): Promise<HostedReviewInfo | null> {
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<HostedReviewInfo | null>((resolve, reject) => {
release = resolve
fail = reject
})
let timer: ReturnType<typeof setTimeout> | 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<HostedReviewInfo | null> {
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)
}
@@ -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<string, { until: number; failures: number }>()
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()
}
@@ -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