diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index f12d79f775..a731a90c41 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -1041,6 +1041,7 @@ "prime-client.test.ts": "clients", "privacy-mask-account.test.ts": "lib", "privacy-scan-meta-key.test.ts": "ci-workflows", + "probe-lease.test.ts": "routing", "process-control-graceful.test.ts": "lib", "process-control.test.ts": "lib", "process-state.test.ts": "service", diff --git a/src/lib/upstream-retry.ts b/src/lib/upstream-retry.ts index 9cd26731cb..aba90a96b2 100644 --- a/src/lib/upstream-retry.ts +++ b/src/lib/upstream-retry.ts @@ -139,7 +139,11 @@ export interface RetryBackoffOptions { * first instead of silently lengthening every adapter's backoff. */ retryAfterIsLowerBound?: boolean; - /** Hard ceiling for an honoured `Retry-After`, so an hour-long wait cannot park a request. */ + /** + * The wait deadline a caller applies to an honoured `Retry-After`. The delay itself is + * never shortened: an instruction longer than the deadline is a reason to END with the + * upstream answer, not to send early. Kept for callers that still pass it. + */ retryAfterCeilingMs?: number; } @@ -305,10 +309,10 @@ export function retryBackoffDelayMs(attempt: number, opts: RetryBackoffOptions): // A provider that names a wait is stating when it will serve again; sending earlier is a // request we already know will be refused, and refusing it twice is the retry storm the // header exists to prevent. The local maximum bounds our OWN exponential backoff and has no - // business shortening someone else's instruction. The ceiling is separate: it stops an - // hour-long Retry-After from parking a request forever. - const ceiling = opts.retryAfterCeilingMs ?? RETRY_AFTER_CEILING_MS; - return Math.min(Math.max(retryAfter, jittered), ceiling); + // business shortening someone else's instruction, so the instruction is returned in full. + // Whether the request can afford to wait that long is the caller's deadline decision -- + // fetchWithTransientRetry ends with the upstream answer rather than retrying early. + return Math.max(retryAfter, jittered); } export function cancelResponseBodyBestEffort(res: Response): void { @@ -364,6 +368,15 @@ export interface TransientRetryOptions extends ResetRetryOptions { * keep them on ONE budget instead of handing each leg a fresh one. */ onSendsConsumed?: (sends: number) => void; + /** + * How long this caller can wait on an honoured `Retry-After`, defaulting to + * {@link RETRY_AFTER_CEILING_MS}. It is a deadline, never a clamp: an instruction inside it + * is slept in full, and an instruction past it ends the call with the upstream answer and + * its `Retry-After` intact rather than sending early at a provider that already said it + * would refuse. A caller with a shorter budget than a minute says so and is not parked past + * it; a caller that can genuinely wait longer says so and is not cut short. + */ + retryAfterCeilingMs?: number; } export type UpstreamSendRecovery = "connection-reset" | "transient-5xx"; @@ -519,6 +532,19 @@ export async function fetchWithTransientRetry( // a response whose body we just cancelled. if (opts.abortSignal?.aborted) return res; if (Date.now() - attemptStart > slowAttemptMs) return res; + const instructedDelay = retryAfterDelayMs(res.headers); + // The deadline is the CALLER'S, not this module's default. Reading the constant directly + // broke it in both directions: a caller with a 30s budget slept the full 45s an upstream + // asked for, and a caller that could genuinely wait 120s was handed the error back for a + // 90s instruction it was willing to honour. + const waitDeadlineMs = opts.retryAfterCeilingMs ?? RETRY_AFTER_CEILING_MS; + if (instructedDelay !== undefined && instructedDelay > waitDeadlineMs) { + // Honouring the stated wait would park this request past the deadline it can commit + // to, and sleeping only up to the deadline is a send the provider already said it will + // refuse. End here instead: the caller receives the upstream answer with its + // Retry-After intact and applies its own policy, exactly as on the direct path. + return res; + } console.warn( `[upstream-retry] transient ${res.status}${opts.label ? ` (${opts.label})` : ""} — retrying (${sent + 1}/${budget})`, ); diff --git a/src/routing/probe-lease.ts b/src/routing/probe-lease.ts new file mode 100644 index 0000000000..fb63061e01 --- /dev/null +++ b/src/routing/probe-lease.ts @@ -0,0 +1,511 @@ +/** + * Half-open recovery for a held account, and the pool-wide retry/probe budget + * that sits above it (#4546, wp3 follow-up). + * + * The transient hold (#4616) keeps a thread's binding while its account serves a + * 5xx streak, and detours requests to a healthy sibling. What it cannot answer is + * whether the held account is actually back: a soft-avoided account receives no + * traffic, so the two-success clearing rule can only fire through the "held" + * fallback, which hands the failing account back to every pinned thread at once. + * This module is the bounded trial that closes that gap -- a single-holder probe + * lease keyed on the health domain (the quota-cooldown domain already has its own + * lease in src/codex/routing.ts and is a different thing). + * + * Three rules the lease enforces: + * + * - While an account is held, exactly one in-flight probe may test it. Every + * other request keeps the remembered detour, so a failed probe costs the + * caller nothing -- the detour's identity is never dropped to run the trial. + * - The lease has a deadline and is released on success, failure, or expiry. A + * response that arrives after its lease was lost is STALE: it must not + * overwrite a newer binding or a newer failure state, so every lease carries + * the generation it was issued under and a settle that fails the fence + * mutates nothing. + * - When every candidate is held the caller gets a typed "binding remembered, + * dispatch withheld" outcome -- not a send to an account already known to be + * failing. + * + * The pool-wide limiter exists because per-request send budgets do not prevent + * a retry storm: thousands of requests each staying inside their own allowance + * still compose into an unbounded rate against an already-failing upstream. + * Recovery dispatches (retries and probes, never the initial send of a new + * request) are admitted only while they stay under a ratio of observed initial + * sends in a sliding window -- the standard overload-guidance shape. + */ + +/** How long a granted probe may be in flight before its lease is forfeit. */ +export const TRANSIENT_PROBE_LEASE_MS = 30_000; +/** + * Minimum spacing between probes of the same held account. Without it every + * request that follows a settled probe becomes the next probe, which is the + * same storm the single-holder rule exists to bound -- just serialized. + */ +export const TRANSIENT_PROBE_INTERVAL_MS = 15_000; + +/** + * Grace kept on top of an entry's pacing and lease deadlines before it may be forgotten. + * Inside it a late settle can still answer "expired" rather than "stale", which is the + * distinction the settle contract exists to report. + */ +const PROBE_STATE_RETENTION_MS = 60_000; +/** + * Hard ceiling on remembered accounts. Pacing state is per account id, and account ids churn + * with configuration: without a ceiling a long-lived proxy accumulates one entry per id it + * ever probed. Above the ceiling the entries whose pacing lapses soonest are dropped, which + * at worst lets one dormant account be probed earlier than its interval; an entry holding a + * LIVE lease is never dropped, because that would hand out a second concurrent probe and + * break the single-holder rule the lease exists to enforce. + */ +export const MAX_TRANSIENT_PROBE_STATES = 1_024; +/** Below this the map is too small to be worth scanning on a grant. */ +const PROBE_STATE_SWEEP_THRESHOLD = 64; +/** + * Eviction target once the ceiling is reached. Clearing a block at a time keeps the ordering + * pass off the common grant path: it runs once per block of new accounts instead of once per + * grant forever after the first time the ceiling is touched. + */ +const PROBE_STATE_EVICTION_LOW_WATER = Math.floor(MAX_TRANSIENT_PROBE_STATES * 0.9); + +export interface TransientProbeLease { + readonly accountId: string; + readonly leaseId: string; + /** Epoch the lease was issued under; a settle must match the CURRENT epoch. */ + readonly generation: number; + readonly expiresAt: number; +} + +export type TransientProbeOutcome = "recovered" | "failed"; + +/** + * What a settle did to the lease. + * + * - `applied`: the probe still held the lease inside its deadline; the caller + * may act on the outcome (clear the hold, or record the fresh failure). + * - `stale`: the lease was already lost -- expired and re-issued, or invalidated + * by newer authoritative state. The result is dropped; nothing is overwritten. + * - `expired`: the probe finished after its own deadline. The lease is dead + * either way; this answer exists so the caller can tell "lost a race" from + * "ran long". + */ +export type TransientProbeSettle = "applied" | "stale" | "expired"; + +interface AccountProbeState { + /** Bumped on every lease grant and every external invalidation. */ + generation: number; + leaseId?: string; + leaseExpiresAt?: number; + lastProbeAt?: number; + /** + * Moment this account's pacing interval lapses, recorded at grant time from the interval + * that grant actually used. Kept alongside `lastProbeAt` so cleanup honours a caller's + * longer interval instead of assuming the default. + */ + pacedUntil?: number; + lastOutcome?: TransientProbeOutcome; +} + +const probeStates = new Map(); +let probeLeaseSeq = 0; + +function probeStateFor(accountId: string): AccountProbeState { + let state = probeStates.get(accountId); + if (!state) { + state = { generation: 0 }; + probeStates.set(accountId, state); + } + return state; +} + +function liveLease(state: AccountProbeState, now: number): boolean { + return state.leaseId !== undefined && state.leaseExpiresAt !== undefined && state.leaseExpiresAt > now; +} + +/** + * Moment an entry stops carrying anything a future decision can read: its pacing interval and + * any unsettled lease deadline, plus the grace above. + */ +function probeStateRetiresAt(state: AccountProbeState): number { + return Math.max(state.pacedUntil ?? 0, state.leaseExpiresAt ?? 0) + PROBE_STATE_RETENTION_MS; +} + +/** + * Bound the remembered accounts. Called on the one path that can grow the map -- a grant is + * the only insertion -- so the ceiling holds without a timer. + * + * The first pass drops only entries that can no longer change an answer: no live lease, the + * pacing interval lapsed, and the grace elapsed. Re-creating such an entry later yields the + * same decisions it would have produced, and a late settle against it still cannot be applied + * because lease ids are issued from a monotonic counter and never repeat. + */ +function sweepProbeStates(now: number): void { + if (probeStates.size <= PROBE_STATE_SWEEP_THRESHOLD) return; + for (const [accountId, state] of probeStates) { + if (liveLease(state, now)) continue; + if (now >= probeStateRetiresAt(state)) probeStates.delete(accountId); + } + if (probeStates.size <= MAX_TRANSIENT_PROBE_STATES) return; + // Still over the ceiling with nothing retired: churn is faster than the retention window. + // Evict in retirement order so the entries closest to meaningless go first, and never one + // holding a live lease. + const evictable = Array.from(probeStates) + .filter(([, state]) => !liveLease(state, now)) + .sort((a, b) => probeStateRetiresAt(a[1]) - probeStateRetiresAt(b[1])); + let excess = probeStates.size - PROBE_STATE_EVICTION_LOW_WATER; + for (const [accountId] of evictable) { + if (excess <= 0) break; + probeStates.delete(accountId); + excess -= 1; + } +} + +/** + * Grant the single in-flight probe for a held account, or null when another + * probe is already out or the pacing interval has not elapsed. The grant bumps + * the epoch, so a result from any earlier lease is stale the moment it lands. + */ +export function tryAcquireTransientProbe( + accountId: string, + now = Date.now(), + options?: { leaseMs?: number; minIntervalMs?: number }, +): TransientProbeLease | null { + const state = probeStateFor(accountId); + if (liveLease(state, now)) return null; + const interval = options?.minIntervalMs ?? TRANSIENT_PROBE_INTERVAL_MS; + if (state.lastProbeAt !== undefined && now - state.lastProbeAt < interval) return null; + const leaseMs = options?.leaseMs ?? TRANSIENT_PROBE_LEASE_MS; + const leaseId = `tprobe-${(probeLeaseSeq += 1).toString(36)}`; + const expiresAt = now + Math.max(1, leaseMs); + state.generation += 1; + state.leaseId = leaseId; + state.leaseExpiresAt = expiresAt; + state.lastProbeAt = now; + state.pacedUntil = now + Math.max(0, interval); + // After the grant, not before it: the entry this call just wrote holds a live lease and is + // therefore the one entry the sweep may never touch, so the ceiling is a real ceiling + // rather than "the ceiling plus whatever was inserted after the scan". + sweepProbeStates(now); + return { + accountId, + leaseId, + generation: state.generation, + expiresAt, + }; +} + +/** Side-effect-free mirror of {@link tryAcquireTransientProbe} eligibility. */ +export function canAcquireTransientProbe( + accountId: string, + now = Date.now(), + options?: { minIntervalMs?: number }, +): boolean { + const state = probeStates.get(accountId); + if (!state) return true; + if (liveLease(state, now)) return false; + const interval = options?.minIntervalMs ?? TRANSIENT_PROBE_INTERVAL_MS; + return state.lastProbeAt === undefined || now - state.lastProbeAt >= interval; +} + +/** + * Report a probe's outcome. Only the current lease holder inside its deadline + * applies: anything else is a late answer from a probe that already lost, and + * dropping it is what keeps it from overwriting a newer binding or a newer + * failure state. An applied settle clears the lease so the next probe is paced + * by the interval, not by the expiry. + */ +export function settleTransientProbe( + lease: TransientProbeLease, + outcome: TransientProbeOutcome, + now = Date.now(), +): TransientProbeSettle { + const state = probeStates.get(lease.accountId); + if (!state || state.leaseId !== lease.leaseId || state.generation !== lease.generation) { + return "stale"; + } + // `>=`, matching liveLease: at exactly the deadline the lease is already gone, so applying + // the outcome there would let a probe act on a lease the grant path would refuse to + // recognise -- two answers to the same instant. + if (now >= lease.expiresAt) return "expired"; + state.leaseId = undefined; + state.leaseExpiresAt = undefined; + state.lastOutcome = outcome; + return "applied"; +} + +/** + * Hand a lease back with no outcome -- the probe never reached upstream, so + * there is nothing to record. Only the holder may release; a stale lease is + * already dead and needs no cleanup. + */ +export function releaseTransientProbe(lease: TransientProbeLease): void { + const state = probeStates.get(lease.accountId); + if (!state || state.leaseId !== lease.leaseId || state.generation !== lease.generation) return; + state.leaseId = undefined; + state.leaseExpiresAt = undefined; +} + +/** + * Fence the epoch against newer authoritative state. A fresh failure recorded + * through the ordinary outcome path, or a binding that moved on, must not be + * overwritten by a probe result that was issued before it -- bumping the epoch + * makes every outstanding lease stale without waiting for its deadline. + */ +export function invalidateTransientProbe(accountId: string): void { + const state = probeStates.get(accountId); + if (!state) return; + state.generation += 1; + state.leaseId = undefined; + state.leaseExpiresAt = undefined; +} + +export interface TransientProbeDiagnostics { + readonly held: boolean; + readonly generation: number; + readonly leaseId?: string; + readonly leaseExpiresAt?: number; + readonly lastProbeAt?: number; + readonly lastOutcome?: TransientProbeOutcome; +} + +/** Current lease state for one account, for diagnostics. Never mutates. */ +export function transientProbeDiagnostics(accountId: string, now = Date.now()): TransientProbeDiagnostics { + const state = probeStates.get(accountId); + if (!state) return { held: false, generation: 0 }; + return { + held: liveLease(state, now), + generation: state.generation, + ...(state.leaseId !== undefined ? { leaseId: state.leaseId, leaseExpiresAt: state.leaseExpiresAt } : {}), + ...(state.lastProbeAt !== undefined ? { lastProbeAt: state.lastProbeAt } : {}), + ...(state.lastOutcome !== undefined ? { lastOutcome: state.lastOutcome } : {}), + }; +} + +/** Test seam: lease state is module-global and must not leak between cases. */ +export function clearTransientProbeLeasesForTests(): void { + probeStates.clear(); +} + +/** + * How many accounts currently carry probe state. Diagnostic, and the assertion surface for + * the {@link MAX_TRANSIENT_PROBE_STATES} bound. + */ +export function transientProbeStateCount(): number { + return probeStates.size; +} + +/** + * What a request may do while its bound account is held. + * + * - `probe`: this caller holds the lease and may send ONE trial to the held + * account. + * - `detour`: a probe is already out (or was refused); keep the remembered + * detour. The detour's identity survives the whole probing window -- a failed + * trial must not cost the caller its working route. + * - `withheld`: every candidate is held. The binding is remembered and dispatch + * is refused; `retryAt` is the earliest moment a probe could next go out. + * Sending anyway here is exactly the "must not send, sends anyway" defect the + * hold was added to close. + */ +export type HeldAccountDispatch = + | { kind: "probe"; lease: TransientProbeLease } + | { kind: "detour"; accountId: string } + | { kind: "withheld"; boundAccountId: string; detourAccountId?: string; retryAt: number }; + +/** + * Decide what a request bound to a held account may do this turn. The probe is + * tried first -- somebody has to find out whether the account is back, and the + * lease guarantees it is exactly one somebody. Everyone else keeps the detour, + * and a caller with no detour left is told to wait rather than sent at an + * account already known to be failing. + */ +export function resolveHeldAccountDispatch(input: { + boundAccountId: string; + detourAccountId?: string; + now?: number; + leaseMs?: number; + minProbeIntervalMs?: number; + backpressure?: PoolBackpressureLimiter; +}): HeldAccountDispatch { + const now = input.now ?? Date.now(); + const limiter = input.backpressure ?? sharedPoolBackpressure(); + // The lease check runs before the budget charge: a probe another holder already has out is + // not a dispatch, and charging the pool for it would shrink the recovery budget by phantom + // sends. Between the check and the grant there is no await, so eligibility cannot change. + if ( + canAcquireTransientProbe(input.boundAccountId, now, { + ...(input.minProbeIntervalMs !== undefined ? { minIntervalMs: input.minProbeIntervalMs } : {}), + }) + && limiter.tryPermitProbeDispatch(now) + ) { + const lease = tryAcquireTransientProbe(input.boundAccountId, now, { + ...(input.leaseMs !== undefined ? { leaseMs: input.leaseMs } : {}), + ...(input.minProbeIntervalMs !== undefined ? { minIntervalMs: input.minProbeIntervalMs } : {}), + }); + if (lease) return { kind: "probe", lease }; + } + if (input.detourAccountId !== undefined && input.detourAccountId !== input.boundAccountId) { + return { kind: "detour", accountId: input.detourAccountId }; + } + return { + kind: "withheld", + boundAccountId: input.boundAccountId, + ...(input.detourAccountId !== undefined ? { detourAccountId: input.detourAccountId } : {}), + retryAt: nextProbeAt(input.boundAccountId, now, input.minProbeIntervalMs), + }; +} + +/** Earliest moment a probe of this account could next be granted. */ +function nextProbeAt(accountId: string, now: number, minIntervalMs?: number): number { + const state = probeStates.get(accountId); + if (!state) return now; + const interval = minIntervalMs ?? TRANSIENT_PROBE_INTERVAL_MS; + const paced = state.lastProbeAt !== undefined ? state.lastProbeAt + interval : now; + const leased = liveLease(state, now) ? state.leaseExpiresAt! : now; + return Math.max(paced, leased); +} + +/* ------------------------------------------------------------------ */ +/* Pool-wide recovery backpressure */ +/* ------------------------------------------------------------------ */ + +export interface PoolBackpressurePolicy { + /** Sliding window the ratio is measured over. */ + readonly windowMs: number; + /** + * Recovery dispatches (retries + probes) admitted per observed initial send. + * 0.2 is the standard overload-guidance budget: at most one recovery send for + * every five new requests. + */ + readonly maxRetryRatio: number; + /** + * Floor under the ratio so a quiet pool can still recover: with almost no + * traffic a strict ratio admits nothing, which would wedge every held + * account behind a probe that can never run. + */ + readonly minRecoveryAllowance: number; +} + +export const DEFAULT_POOL_BACKPRESSURE_POLICY: PoolBackpressurePolicy = { + windowMs: 10_000, + maxRetryRatio: 0.2, + minRecoveryAllowance: 3, +}; + +export interface PoolBackpressureState { + readonly windowMs: number; + readonly initialSends: number; + readonly recoveryDispatches: number; + /** Dispatches admitted under the current window's allowance. */ + readonly allowance: number; + /** Lifetime refusals, including windows already rotated out. */ + readonly refusedTotal: number; + readonly ratioLimit: number; +} + +export interface PoolBackpressureLimiter { + /** A new request's FIRST send. Always recorded, never refused. */ + recordInitialSend(now?: number): void; + /** Admit one retry dispatch, or refuse when the window's ratio is spent. */ + tryPermitRetryDispatch(now?: number): boolean; + /** Admit one probe dispatch under the same shared recovery budget. */ + tryPermitProbeDispatch(now?: number): boolean; + state(now?: number): PoolBackpressureState; +} + +const BACKPRESSURE_BUCKETS = 10; + +/** + * Ratio limiter over a bucketed sliding window. Buckets give a sliding answer + * without keeping per-event state: the window is the sum of the buckets whose + * span falls inside it, and grant/refuse decisions read that sum. + */ +export function createPoolBackpressureLimiter( + policy: PoolBackpressurePolicy = DEFAULT_POOL_BACKPRESSURE_POLICY, +): PoolBackpressureLimiter { + const bucketMs = Math.max(1, Math.floor(policy.windowMs / BACKPRESSURE_BUCKETS)); + const buckets: Array<{ start: number; initials: number; recoveries: number }> = []; + let refusedTotal = 0; + + function bucketFor(now: number): { start: number; initials: number; recoveries: number } { + const start = Math.floor(now / bucketMs) * bucketMs; + const last = buckets[buckets.length - 1]; + if (last && last.start === start) return last; + while (buckets.length > 0 && buckets[0]!.start <= start - policy.windowMs) buckets.shift(); + const bucket = { start, initials: 0, recoveries: 0 }; + buckets.push(bucket); + return bucket; + } + + function totals(now: number): { initials: number; recoveries: number } { + let initials = 0; + let recoveries = 0; + for (const bucket of buckets) { + if (bucket.start <= now - policy.windowMs) continue; + initials += bucket.initials; + recoveries += bucket.recoveries; + } + return { initials, recoveries }; + } + + function allowanceFor(initials: number): number { + return Math.max(policy.minRecoveryAllowance, Math.floor(initials * policy.maxRetryRatio)); + } + + function tryPermit(now: number): boolean { + const bucket = bucketFor(now); + const { initials, recoveries } = totals(now); + if (recoveries + 1 > allowanceFor(initials)) { + refusedTotal += 1; + return false; + } + bucket.recoveries += 1; + return true; + } + + return { + recordInitialSend(now = Date.now()): void { + bucketFor(now).initials += 1; + }, + tryPermitRetryDispatch(now = Date.now()): boolean { + return tryPermit(now); + }, + tryPermitProbeDispatch(now = Date.now()): boolean { + return tryPermit(now); + }, + state(now = Date.now()): PoolBackpressureState { + const { initials, recoveries } = totals(now); + return { + windowMs: policy.windowMs, + initialSends: initials, + recoveryDispatches: recoveries, + allowance: allowanceFor(initials), + refusedTotal, + ratioLimit: policy.maxRetryRatio, + }; + }, + }; +} + +let sharedLimiter: PoolBackpressureLimiter | undefined; + +/** + * The process-wide limiter every recovery dispatch shares. A per-request + * limiter cannot see the storm, which is the entire reason this layer exists. + */ +export function sharedPoolBackpressure(): PoolBackpressureLimiter { + sharedLimiter ??= createPoolBackpressureLimiter(); + return sharedLimiter; +} + +/** + * Point the shared limiter at a different policy. The ceiling is deliberately + * configurable here and not yet plumbed into OcxConfig -- the wiring lane owns + * that seam; this is the knob it turns. + */ +export function configureSharedPoolBackpressure(policy: PoolBackpressurePolicy): void { + sharedLimiter = createPoolBackpressureLimiter(policy); +} + +/** Test seam: the shared limiter is module-global. */ +export function resetSharedPoolBackpressureForTests(): void { + sharedLimiter = undefined; +} diff --git a/structure/catalog.md b/structure/catalog.md index 9e97a330b6..fc7e65a9d5 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -230,6 +230,13 @@ Pool mode routes across main plus added Codex credentials. Key rules: an omitted flag preserves the established behavior of a nonempty hand-written selector map. - **Rotation is sticky.** A conversation stays on its selected account while that account is usable; failure moves it, success does not (`src/codex/pool-rotation.ts`). +- **A transient hold is probed half-open, never opened all at once.** While a bound account is + held for a 5xx streak, one in-flight probe may test it and every other request keeps the + remembered detour; the lease carries a deadline and a generation so a late answer from a + probe that already lost cannot overwrite a newer binding or failure state. When every + candidate is held the caller gets a typed withheld outcome, not a send. Recovery dispatches + (retries and probes, never a new request's initial send) sit under a pool-wide ratio ceiling + measured over a sliding window (`src/routing/probe-lease.ts`). - **The credential store is generation-guarded.** A refresh takes a lock and persists only if the generation it started from still holds; a lost race raises a generation-conflict error rather than overwriting the newer credential (`src/codex/account-store.ts`). Callers handle that error; diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 118089f2ae..8500c4fc78 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -869,6 +869,7 @@ "prime-client.test.ts": "clients", "privacy-mask-account.test.ts": "lib", "privacy-scan-meta-key.test.ts": "ci-workflows", + "probe-lease.test.ts": "routing", "process-control-graceful.test.ts": "lib", "process-control.test.ts": "lib", "process-state.test.ts": "service", diff --git a/tests/lib/upstream-retry.test.ts b/tests/lib/upstream-retry.test.ts index af98c18552..4e014c7645 100644 --- a/tests/lib/upstream-retry.test.ts +++ b/tests/lib/upstream-retry.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, spyOn, test } from "bun:test"; import { fetchWithResetRetry, + fetchWithTransientRetry, isConnectionResetError, prepareSameTarget429Wait, releaseResponseBodyBestEffort, @@ -263,15 +264,103 @@ describe("retryBackoffDelayMs", () => { })).toBe(30_000); }); - test("an honoured Retry-After is still ceilinged so it cannot park a request (#4546)", () => { + test("an honoured Retry-After is preserved in full, never shortened (#4546)", () => { const headers = new Headers({ "Retry-After": "3600" }); + // The instruction is the provider's statement of when it will serve again. Clamping it + // to a local ceiling produced a send the upstream already said it would refuse; whether + // the request can wait that long is the caller's deadline decision, not a shorter delay. expect(retryBackoffDelayMs(0, { baseDelayMs: 250, maxDelayMs: 5_000, headers, retryAfterIsLowerBound: true, retryAfterCeilingMs: 60_000, - })).toBe(60_000); + })).toBe(3_600_000); + }); + + test("an instruction past the wait deadline ends with the upstream answer intact (#4546)", async () => { + silenceWarn(); + const upstream = new Response("overloaded", { + status: 503, + headers: { "Retry-After": "3600" }, + }); + const { calls, doFetch } = mockDoFetch([upstream]); + const res = await fetchWithTransientRetry(doFetch); + // No early retry: one send, and the caller gets the real 503 with its Retry-After + // rather than a second refusal the provider already announced. + expect(calls.length).toBe(1); + expect(res.status).toBe(503); + expect(res.headers.get("retry-after")).toBe("3600"); + }); + + test("an instruction inside the wait deadline is still honoured before retrying (#4546)", async () => { + silenceWarn(); + const limited = new Response("overloaded", { + status: 503, + headers: { "Retry-After": "1" }, + }); + const ok = new Response("fine", { status: 200 }); + const { calls, doFetch } = mockDoFetch([limited, ok]); + const started = Date.now(); + const res = await fetchWithTransientRetry(doFetch); + expect(res.status).toBe(200); + expect(calls.length).toBe(2); + expect(Date.now() - started).toBeGreaterThanOrEqual(900); + }); + + test("a caller deadline shorter than the default is not slept past (#4546)", async () => { + silenceWarn(); + const limited = new Response("overloaded", { + status: 503, + headers: { "Retry-After": "1" }, + }); + const ok = new Response("fine", { status: 200 }); + const { calls, doFetch } = mockDoFetch([limited, ok]); + const started = Date.now(); + // The caller can wait 500ms; the upstream asked for 1s. Reading the module default + // instead of this deadline parked the request for the full second -- the 30s-budget / + // 45s-instruction shape, scaled down so the test does not have to sleep it. + const res = await fetchWithTransientRetry(doFetch, { retryAfterCeilingMs: 500 }); + expect(calls.length).toBe(1); + expect(res.status).toBe(503); + expect(res.headers.get("retry-after")).toBe("1"); + expect(Date.now() - started).toBeLessThan(500); + }); + + test("an instruction exactly at the caller deadline is honoured, not refused (#4546)", async () => { + silenceWarn(); + const limited = new Response("overloaded", { + status: 503, + headers: { "Retry-After": "1" }, + }); + const ok = new Response("fine", { status: 200 }); + const { calls, doFetch } = mockDoFetch([limited, ok]); + const started = Date.now(); + // Equality is inside the budget: the deadline is what the caller CAN wait, so a wait of + // exactly that length is affordable and the retry happens after it. + const res = await fetchWithTransientRetry(doFetch, { retryAfterCeilingMs: 1_000 }); + expect(res.status).toBe(200); + expect(calls.length).toBe(2); + expect(Date.now() - started).toBeGreaterThanOrEqual(900); + }); + + test("a caller deadline longer than the default waits instead of ending early (#4546)", async () => { + silenceWarn(); + const limited = new Response("overloaded", { + status: 503, + headers: { "Retry-After": "90" }, + }); + const { calls, doFetch } = mockDoFetch([limited, new Response("fine", { status: 200 })]); + const ac = new AbortController(); + // 90s is past the module default but inside this caller's 120s deadline, so the call must + // be waiting -- not returning the 503 the default ceiling used to hand back immediately. + // Aborting mid-wait is how the test observes the wait without sitting through it. + setTimeout(() => ac.abort(new DOMException("deadline probe", "AbortError")), 20); + await expect(fetchWithTransientRetry(doFetch, { + retryAfterCeilingMs: 120_000, + abortSignal: ac.signal, + })).rejects.toThrow("deadline probe"); + expect(calls.length).toBe(1); }); test("opting in never shortens a wait below the local backoff (#4546)", () => { diff --git a/tests/routing/probe-lease.test.ts b/tests/routing/probe-lease.test.ts new file mode 100644 index 0000000000..e3cf4ece83 --- /dev/null +++ b/tests/routing/probe-lease.test.ts @@ -0,0 +1,281 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { + canAcquireTransientProbe, + clearTransientProbeLeasesForTests, + configureSharedPoolBackpressure, + createPoolBackpressureLimiter, + invalidateTransientProbe, + releaseTransientProbe, + resetSharedPoolBackpressureForTests, + resolveHeldAccountDispatch, + settleTransientProbe, + sharedPoolBackpressure, + transientProbeDiagnostics, + transientProbeStateCount, + tryAcquireTransientProbe, + MAX_TRANSIENT_PROBE_STATES, + TRANSIENT_PROBE_INTERVAL_MS, +} from "../../src/routing/probe-lease"; + +afterEach(() => { + clearTransientProbeLeasesForTests(); + resetSharedPoolBackpressureForTests(); +}); + +describe("transient probe lease", () => { + test("a held account admits exactly one in-flight probe", () => { + const now = 1_000_000; + const first = tryAcquireTransientProbe("acct-a", now); + expect(first).not.toBeNull(); + // Everyone else is refused while the holder is out. + expect(tryAcquireTransientProbe("acct-a", now)).toBeNull(); + expect(canAcquireTransientProbe("acct-a", now)).toBe(false); + // A different account is a different lease domain. + expect(tryAcquireTransientProbe("acct-b", now)).not.toBeNull(); + }); + + test("a settled lease frees the account after the pacing interval", () => { + const now = 1_000_000; + const lease = tryAcquireTransientProbe("acct-a", now)!; + expect(settleTransientProbe(lease, "failed", now + 5)).toBe("applied"); + // Settling is not a license to probe again immediately -- the interval paces retries. + expect(tryAcquireTransientProbe("acct-a", now + 10)).toBeNull(); + expect(tryAcquireTransientProbe("acct-a", now + TRANSIENT_PROBE_INTERVAL_MS)).not.toBeNull(); + }); + + test("a late result from a replaced lease is stale and mutates nothing", () => { + const now = 1_000_000; + const first = tryAcquireTransientProbe("acct-a", now, { leaseMs: 100, minIntervalMs: 0 })!; + // The first lease lapses and a second probe is issued under a new epoch. + const second = tryAcquireTransientProbe("acct-a", now + 200, { leaseMs: 100, minIntervalMs: 0 })!; + expect(second.generation).toBe(first.generation + 1); + // The late answer must not overwrite the newer lease or record an outcome. + expect(settleTransientProbe(first, "recovered", now + 250)).toBe("stale"); + const diag = transientProbeDiagnostics("acct-a", now + 250); + expect(diag.leaseId).toBe(second.leaseId); + expect(diag.lastOutcome).toBeUndefined(); + // The live holder still settles normally. + expect(settleTransientProbe(second, "recovered", now + 260)).toBe("applied"); + expect(transientProbeDiagnostics("acct-a", now + 260).lastOutcome).toBe("recovered"); + }); + + test("a result after the lease deadline is expired, not applied", () => { + const now = 1_000_000; + const lease = tryAcquireTransientProbe("acct-a", now, { leaseMs: 100 })!; + expect(settleTransientProbe(lease, "recovered", now + 101)).toBe("expired"); + }); + + test("the deadline instant itself is expired for the settle and the grant alike (#4546)", () => { + const now = 1_000_000; + const lease = tryAcquireTransientProbe("acct-a", now, { leaseMs: 100 })!; + // The lease is already gone at its deadline as far as the grant path is concerned... + expect(transientProbeDiagnostics("acct-a", now + 100).held).toBe(false); + // ...so a settle at the same instant must not apply the outcome. Disagreeing about one + // millisecond is how a probe result gets written after the lease was handed to someone + // else. + expect(settleTransientProbe(lease, "recovered", now + 100)).toBe("expired"); + expect(transientProbeDiagnostics("acct-a", now + 100).lastOutcome).toBeUndefined(); + // One millisecond earlier the holder is still live and the outcome applies. + const inside = tryAcquireTransientProbe("acct-b", now, { leaseMs: 100 })!; + expect(settleTransientProbe(inside, "recovered", now + 99)).toBe("applied"); + }); + + test("invalidation fences the epoch so an outstanding probe cannot overwrite newer state", () => { + const now = 1_000_000; + const lease = tryAcquireTransientProbe("acct-a", now)!; + // A newer failure (or a moved binding) lands through the ordinary path. + invalidateTransientProbe("acct-a"); + expect(settleTransientProbe(lease, "recovered", now + 1)).toBe("stale"); + expect(transientProbeDiagnostics("acct-a", now + 1).held).toBe(false); + }); + + test("release hands back a probe that never reached upstream", () => { + const now = 1_000_000; + const lease = tryAcquireTransientProbe("acct-a", now, { minIntervalMs: 0 })!; + releaseTransientProbe(lease); + expect(canAcquireTransientProbe("acct-a", now, { minIntervalMs: 0 })).toBe(true); + // Releasing someone else's lease is a no-op. + releaseTransientProbe({ ...lease, leaseId: "forged" }); + }); +}); + +describe("probe state retention", () => { + test("a retired account is forgotten only once it can no longer pace a probe (#4546)", () => { + const now = 1_000_000; + for (let i = 0; i < 200; i++) { + const lease = tryAcquireTransientProbe(`gone-${i}`, now, { leaseMs: 100 })!; + settleTransientProbe(lease, "failed", now + 1); + } + expect(transientProbeStateCount()).toBe(200); + + // Still inside the pacing interval: dropping these now would let the very next request + // for any of them probe early, which is the storm the interval exists to bound. + tryAcquireTransientProbe("still-paced", now + TRANSIENT_PROBE_INTERVAL_MS - 1); + expect(transientProbeStateCount()).toBe(201); + // The sweep that ran on that grant kept every entry that can still refuse a probe. + expect(canAcquireTransientProbe("gone-0", now + TRANSIENT_PROBE_INTERVAL_MS - 1)).toBe(false); + + // Past the pacing interval and the retention grace the entries cannot change an answer, + // so they are dropped instead of being remembered for the life of the process. + const retired = now + TRANSIENT_PROBE_INTERVAL_MS + 60_000 + 1; + tryAcquireTransientProbe("fresh", retired); + // Two left: the account just probed, and `still-paced`, whose lease was never settled -- + // the grace keeps that one long enough for a late settle to still be answered "expired" + // rather than silently reclassified. + expect(transientProbeStateCount()).toBe(2); + // A dropped entry is indistinguishable from one that was never probed -- which is exactly + // why it was safe to drop: by now it would admit a probe either way. + expect(canAcquireTransientProbe("gone-0", retired)).toBe(true); + }); + + test("remembered accounts stay under the ceiling when churn outruns retention (#4546)", () => { + const now = 1_000_000; + // Every probe settles at once, so nothing holds a live lease: the shape a churning + // configuration produces, and the one that used to grow one entry per account id forever. + const churn = MAX_TRANSIENT_PROBE_STATES * 2; + for (let i = 0; i < churn; i++) { + const lease = tryAcquireTransientProbe(`churn-${i}`, now + i, { leaseMs: 10 })!; + settleTransientProbe(lease, "failed", now + i + 1); + } + expect(transientProbeStateCount()).toBeLessThanOrEqual(MAX_TRANSIENT_PROBE_STATES); + // The ceiling is enforced from the oldest end: the newest accounts keep their pacing. + expect(canAcquireTransientProbe(`churn-${churn - 1}`, now + churn)).toBe(false); + }); + + test("an account holding a live lease survives the ceiling (#4546)", () => { + const now = 1_000_000; + const held = tryAcquireTransientProbe("held-through-churn", now, { leaseMs: 10_000_000 })!; + for (let i = 0; i < MAX_TRANSIENT_PROBE_STATES * 2; i++) { + const lease = tryAcquireTransientProbe(`churn-${i}`, now + i, { leaseMs: 10 })!; + settleTransientProbe(lease, "failed", now + i + 1); + } + // Evicting a live lease would hand a second concurrent probe to the same held account. + expect(transientProbeDiagnostics("held-through-churn", now + 1).leaseId).toBe(held.leaseId); + expect(canAcquireTransientProbe("held-through-churn", now + 1)).toBe(false); + }); +}); + +describe("held account dispatch", () => { + test("one caller probes while the rest keep the remembered detour", () => { + const now = 1_000_000; + const limiter = createPoolBackpressureLimiter(); + const first = resolveHeldAccountDispatch({ + boundAccountId: "acct-a", + detourAccountId: "acct-b", + now, + backpressure: limiter, + }); + expect(first.kind).toBe("probe"); + const second = resolveHeldAccountDispatch({ + boundAccountId: "acct-a", + detourAccountId: "acct-b", + now, + backpressure: limiter, + }); + // The detour is not forgotten during probing: a failed trial must not cost + // the caller its working route. + expect(second).toEqual({ kind: "detour", accountId: "acct-b" }); + }); + + test("every candidate held yields a withheld outcome, never a send", () => { + const now = 1_000_000; + const limiter = createPoolBackpressureLimiter(); + // Spend the single probe so this caller has no trial available. + resolveHeldAccountDispatch({ boundAccountId: "acct-a", now, backpressure: limiter }); + const outcome = resolveHeldAccountDispatch({ boundAccountId: "acct-a", now, backpressure: limiter }); + expect(outcome.kind).toBe("withheld"); + if (outcome.kind === "withheld") { + expect(outcome.boundAccountId).toBe("acct-a"); + expect(outcome.retryAt).toBeGreaterThan(now); + } + }); + + test("a refused probe falls back to the detour, then to withheld", () => { + const now = 1_000_000; + // Zero-allowance limiter: recovery budget is spent, so no probe may go out. + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, + maxRetryRatio: 0, + minRecoveryAllowance: 0, + }); + const withDetour = resolveHeldAccountDispatch({ + boundAccountId: "acct-a", + detourAccountId: "acct-b", + now, + backpressure: limiter, + }); + expect(withDetour).toEqual({ kind: "detour", accountId: "acct-b" }); + const noDetour = resolveHeldAccountDispatch({ + boundAccountId: "acct-a", + now, + backpressure: limiter, + }); + expect(noDetour.kind).toBe("withheld"); + }); +}); + +describe("pool-wide backpressure", () => { + test("the initial send of a new request is never refused", () => { + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, + maxRetryRatio: 0, + minRecoveryAllowance: 0, + }); + // Even with a zero recovery budget, initials are recorded, not gated. + for (let i = 0; i < 100; i++) limiter.recordInitialSend(i); + expect(limiter.state(100).initialSends).toBe(100); + }); + + test("recovery dispatches are capped by the ratio of observed initials", () => { + const now = 1_000_000; + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, + maxRetryRatio: 0.2, + minRecoveryAllowance: 0, + }); + for (let i = 0; i < 10; i++) limiter.recordInitialSend(now); + // 20% of 10 initials admits exactly 2 recovery dispatches, shared by retries and probes. + expect(limiter.tryPermitRetryDispatch(now)).toBe(true); + expect(limiter.tryPermitProbeDispatch(now)).toBe(true); + expect(limiter.tryPermitRetryDispatch(now)).toBe(false); + const state = limiter.state(now); + expect(state.recoveryDispatches).toBe(2); + expect(state.allowance).toBe(2); + expect(state.refusedTotal).toBe(1); + }); + + test("the floor keeps a quiet pool recoverable", () => { + const now = 1_000_000; + const limiter = createPoolBackpressureLimiter(); + // No initials at all: the minimum allowance still admits bounded recovery. + expect(limiter.tryPermitProbeDispatch(now)).toBe(true); + expect(limiter.tryPermitRetryDispatch(now)).toBe(true); + expect(limiter.tryPermitRetryDispatch(now)).toBe(true); + expect(limiter.tryPermitRetryDispatch(now)).toBe(false); + }); + + test("the window slides: old sends stop funding new retries", () => { + const limiter = createPoolBackpressureLimiter({ + windowMs: 10_000, + maxRetryRatio: 0.5, + minRecoveryAllowance: 0, + }); + const t0 = 1_000_000; + for (let i = 0; i < 10; i++) limiter.recordInitialSend(t0); + expect(limiter.tryPermitRetryDispatch(t0)).toBe(true); + // A window later the initials have rotated out; the burst no longer funds retries. + const t1 = t0 + 11_000; + expect(limiter.state(t1).initialSends).toBe(0); + expect(limiter.tryPermitRetryDispatch(t1)).toBe(false); + }); + + test("the shared limiter is configurable and reports its state", () => { + configureSharedPoolBackpressure({ windowMs: 5_000, maxRetryRatio: 1, minRecoveryAllowance: 0 }); + const limiter = sharedPoolBackpressure(); + limiter.recordInitialSend(1_000_000); + expect(limiter.tryPermitRetryDispatch(1_000_000)).toBe(true); + const state = limiter.state(1_000_000); + expect(state.windowMs).toBe(5_000); + expect(state.ratioLimit).toBe(1); + }); +});