From f5e878a534803e6b1fb39fe7cc01384fd1c9aeb2 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 19:20:51 +0900 Subject: [PATCH 1/2] fix(codex): promote a healthy detour instead of releasing it, and honour Retry-After (#4546) Refs #4546. PRD R07. When a transient hold outlived its window, routing deleted the whole affinity entry -- including the detour account that had actually been serving the thread -- and re-picked cold. The timer expiring restores the right to re-decide; it is not itself a recovery, and treating it as one threw away the single piece of evidence the request had. A still-healthy detour is now promoted to the binding instead, with the move recorded as rebound/transient_hold_expired so the reason is visible. A detour that has itself gone unhealthy still falls through to the cold path. Retry-After is a lower bound on the transient path. The local maximum delay bounds our own exponential backoff and has no business shortening a wait the provider stated: sending early is a request we already know will be refused, which is the storm the header exists to prevent. It is opt-in per caller so the change lands on the transient path first rather than silently lengthening every adapter's backoff, and an honoured wait is ceilinged at one minute so an hour-long Retry-After cannot park a request. --- src/codex/routing.ts | 26 +++++++++++++++++++++++ src/lib/upstream-retry.ts | 27 ++++++++++++++++++++++-- tests/lib/upstream-retry.test.ts | 36 ++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) diff --git a/src/codex/routing.ts b/src/codex/routing.ts index 5288d9441d..51779de4c1 100644 --- a/src/codex/routing.ts +++ b/src/codex/routing.ts @@ -2946,6 +2946,32 @@ export function resolveCodexAccountForThreadDetailed( // A model-only exclusion does not invalidate the shared task binding. Health, // generation, pause, cooldown, and failure evidence still retire it normally. if (!modelScopedSelection || !healthyForSharedAffinity) { + // A hold that outlived its window is not the same as a conversation with nowhere to go. + // If the account that has actually been serving this thread is still healthy, promote it + // instead of deleting the entry and re-picking cold: releasing here threw away the one + // piece of evidence the request had -- that B works -- and handed the thread back to a + // fresh strategy choice, which is the cold-prefix cost #4546 is about. A timer expiring + // restores the right to re-decide; it is not itself a recovery. + const expiredDetour = entry.transientDetourAccountId; + if ( + isTransientHoldExpired(entry, now) + && generationLive + && !quotaRefused + && expiredDetour !== undefined + && expiredDetour !== entry.accountId + && isCodexAccountSelectable(config, expiredDetour, now, quotaScope, selectionOptions) + && !hasUnrecoveredCodexQuotaRefusal(expiredDetour, quotaScope) + && !shouldFailover(config, expiredDetour, now) + && !isCodexAccountSoftAvoided(expiredDetour, now) + ) { + if (!isIndependentCodexQuotaScope(quotaScope)) promoteActiveCodexAccount(config, expiredDetour); + bindThreadAffinity(threadId, expiredDetour, now, quotaScope); + return { + status: "selected", + accountId: expiredDetour, + affinity: { move: "rebound", reason: "transient_hold_expired" }, + }; + } releaseReason = !generationLive ? "generation" : quotaRefused diff --git a/src/lib/upstream-retry.ts b/src/lib/upstream-retry.ts index 805a1e347a..9cd26731cb 100644 --- a/src/lib/upstream-retry.ts +++ b/src/lib/upstream-retry.ts @@ -133,8 +133,19 @@ export interface RetryBackoffOptions { baseDelayMs: number; maxDelayMs: number; headers?: Headers; + /** + * Treat a provider's `Retry-After` as the earliest legal send rather than something the + * local maximum may shorten. Opt-in per caller so the change lands on the transient path + * 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. */ + retryAfterCeilingMs?: number; } +/** One minute, matching the same-target 429 ceiling the key-failover path already uses. */ +export const RETRY_AFTER_CEILING_MS = 60_000; + export function abortError(signal?: AbortSignal): unknown { return signal?.reason ?? new DOMException("The operation was aborted", "AbortError"); } @@ -284,9 +295,20 @@ function retryAfterDelayMs(headers: Headers): number | undefined { export function retryBackoffDelayMs(attempt: number, opts: RetryBackoffOptions): number { const retryAfter = opts.headers ? retryAfterDelayMs(opts.headers) : undefined; - if (retryAfter !== undefined) return Math.min(retryAfter, opts.maxDelayMs); const exp = Math.min(opts.baseDelayMs * (2 ** attempt), opts.maxDelayMs); - return Math.floor(exp * (0.8 + Math.random() * 0.4)); + const jittered = Math.floor(exp * (0.8 + Math.random() * 0.4)); + if (retryAfter === undefined) return jittered; + if (opts.retryAfterIsLowerBound !== true) { + // Historical behaviour, still the default for every caller that has not opted in. + return Math.min(retryAfter, opts.maxDelayMs); + } + // 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); } export function cancelResponseBodyBestEffort(res: Response): void { @@ -504,6 +526,7 @@ export async function fetchWithTransientRetry( baseDelayMs: TRANSIENT_RETRY_BASE_DELAY_MS, maxDelayMs: TRANSIENT_RETRY_MAX_DELAY_MS, headers: res.headers, + retryAfterIsLowerBound: true, }); cancelResponseBodyBestEffort(res); // Throws on abort (see sleepWithAbort): the rejection propagates, and the body we just diff --git a/tests/lib/upstream-retry.test.ts b/tests/lib/upstream-retry.test.ts index 59fca58637..af98c18552 100644 --- a/tests/lib/upstream-retry.test.ts +++ b/tests/lib/upstream-retry.test.ts @@ -250,6 +250,42 @@ describe("retryBackoffDelayMs", () => { } }); + test("treats Retry-After as a lower bound when the caller opts in (#4546)", () => { + const headers = new Headers({ "Retry-After": "30" }); + // The local maximum bounds our OWN exponential backoff. Shortening a provider's stated + // wait to 5s just sends a request we already know will be refused, which is the storm the + // header exists to prevent. + expect(retryBackoffDelayMs(0, { + baseDelayMs: 250, + maxDelayMs: 5_000, + headers, + retryAfterIsLowerBound: true, + })).toBe(30_000); + }); + + test("an honoured Retry-After is still ceilinged so it cannot park a request (#4546)", () => { + const headers = new Headers({ "Retry-After": "3600" }); + expect(retryBackoffDelayMs(0, { + baseDelayMs: 250, + maxDelayMs: 5_000, + headers, + retryAfterIsLowerBound: true, + retryAfterCeilingMs: 60_000, + })).toBe(60_000); + }); + + test("opting in never shortens a wait below the local backoff (#4546)", () => { + const headers = new Headers({ "Retry-After": "0" }); + // A past or zero Retry-After means "no enforced wait", not "send immediately with no + // backoff at all" -- the count and ratio budgets still apply and so does our own pacing. + expect(retryBackoffDelayMs(0, { + baseDelayMs: 1_000, + maxDelayMs: 5_000, + headers, + retryAfterIsLowerBound: true, + })).toBeGreaterThanOrEqual(800); + }); + test("falls back to capped exponential jitter when Retry-After is absent", () => { const randomSpy = spyOn(Math, "random").mockReturnValue(0); try { From 85c99012c3efbbafc202b98ce3585bf2b7b9eb16 Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 19:21:24 +0900 Subject: [PATCH 2/2] docs(devlog): record the R07 detour-promotion outcome (#4546) --- .../020_backoff_preserves_binding.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/devlog/_plan/260914_cost_guard_stabilization/020_backoff_preserves_binding.md b/devlog/_plan/260914_cost_guard_stabilization/020_backoff_preserves_binding.md index 24a1f25dc3..091a95b0ec 100644 --- a/devlog/_plan/260914_cost_guard_stabilization/020_backoff_preserves_binding.md +++ b/devlog/_plan/260914_cost_guard_stabilization/020_backoff_preserves_binding.md @@ -77,3 +77,22 @@ the generation check and the pinned-account guard in 4. A 429 on the bound account still releases the binding immediately, unchanged. 5. A late transient failure from an account the thread already left does not touch the current binding. + +## R07 outcome: expiry is permission to re-decide, not a recovery + +The rule above bounded the hold correctly and then threw away its own evidence. On expiry the +entry was deleted whole -- `transientDetourAccountId` with it -- and the thread re-picked cold, +so an account that had been serving the conversation happily for ten minutes got no more +consideration than any other. A timer running out restores the right to re-decide; it is not +itself a reason to prefer a stranger. + +A still-healthy detour is now promoted to the binding, recorded as `rebound` with reason +`transient_hold_expired`. Promotion is refused when the release reason is generation +invalidation or a quota refusal: those are hard invalidations, and a detour that merely looks +healthy must not rescue them. + +What this still does not do: a soft-avoided account receives no traffic at all, so the +two-consecutive-success clearing rule can only be met through the "held" fallback, which hands +the failing account back to every pinned thread at once. A half-open probe lease -- one thread +probes, the rest keep detouring -- is the missing piece and needs a lease keyed on the health +domain rather than the quota cooldown domain the existing one uses.