fix(codex): promote a healthy detour instead of releasing it, and honour Retry-After (#4546)

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.

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

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

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 {