From 1f20650ef16ab365f1c653573f7971b991531789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=92=D0=B0=D0=BB=D0=B5=D1=80=D0=B8=D0=B9=20=D0=91=D1=80?= =?UTF-8?q?=D0=B8=D0=B6=D0=B0=D1=82=D1=8E=D0=BA?= Date: Fri, 28 Aug 2026 19:25:52 +0300 Subject: [PATCH] fix(openai): honor Gemini RetryInfo retryDelay on 429/503 retries Gemini's OpenAI-compatible endpoint advertises its cooldown only as google.rpc.RetryInfo inside the error JSON (error.details[].retryDelay, a protobuf Duration like "39s") and sends no retry-after header. The retry loop derived retryAfterMs solely from the header, so throttled requests burned all three attempts on the 150/300ms backoff before the provider's cooldown had elapsed. httpErrorFromResponse now falls back to parsing that structured delay from the already-read body on 429/503 when the header is absent. The value flows through the existing retryAfterMs field, so header precedence, the 5s interactive cap in resolveWaitMs, and the cancellation-aware sleep all apply unchanged; malformed or negative durations are ignored. Fake-timer tests cover each acceptance criterion of #106. Fixes #106 Co-Authored-By: Claude Fable 5 --- src/llm/provider/openai/openai-http.test.ts | 151 +++++++++++++++++++- src/llm/provider/openai/openai-http.ts | 50 ++++++- 2 files changed, 198 insertions(+), 3 deletions(-) diff --git a/src/llm/provider/openai/openai-http.test.ts b/src/llm/provider/openai/openai-http.test.ts index eecd66a9..ec727b2a 100644 --- a/src/llm/provider/openai/openai-http.test.ts +++ b/src/llm/provider/openai/openai-http.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { OpenAiHttpError, @@ -115,6 +115,155 @@ describe("openAiPostJson", () => { expect(fetchImpl).toHaveBeenCalledTimes(2); }); + describe("structured RetryInfo metadata", () => { + // Gemini's OpenAI-compatible endpoint sends its cooldown only in + // the error JSON — google.rpc.RetryInfo with a protobuf Duration + // string — and no `retry-after` header. Fake timers plus a pinned + // Math.random (0.5 zeroes the ±20% jitter) make every wait exact. + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + function retryInfoBody(retryDelay: unknown, status = 429): string { + return JSON.stringify({ + error: { + code: status, + details: [ + { "@type": "type.googleapis.com/google.rpc.RetryInfo", retryDelay }, + ], + }, + }); + } + + async function expectSecondFetchAfter( + pending: Promise, + fetchImpl: ReturnType, + waitMs: number, + ): Promise { + await vi.advanceTimersByTimeAsync(waitMs - 1); + expect(fetchImpl).toHaveBeenCalledTimes(1); + await vi.advanceTimersByTimeAsync(1); + await expect(pending).resolves.toEqual({ ok: true }); + expect(fetchImpl).toHaveBeenCalledTimes(2); + } + + it.each([ + { status: 429, retryDelay: "1.5s", expectedMs: 1_500 }, + { status: 503, retryDelay: "2s", expectedMs: 2_000 }, + ])( + "honors error.details[].retryDelay on a headerless $status", + async ({ status, retryDelay, expectedMs }) => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0.5); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(errorResponse(status, retryInfoBody(retryDelay, status))) + .mockResolvedValueOnce(jsonResponse({ ok: true })); + const pending = openAiPostJson( + depsWith(fetchImpl as unknown as typeof fetch), + "/x", + {}, + {}, + ); + await expectSecondFetchAfter(pending, fetchImpl, expectedMs); + }, + ); + + it.each(["not-a-duration", "-1s", 39])( + "ignores unusable retryDelay %j and keeps the plain backoff", + async (retryDelay) => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0.5); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(errorResponse(429, retryInfoBody(retryDelay))) + .mockResolvedValueOnce(jsonResponse({ ok: true })); + const pending = openAiPostJson( + depsWith(fetchImpl as unknown as typeof fetch), + "/x", + {}, + {}, + ); + await expectSecondFetchAfter(pending, fetchImpl, 150); + }, + ); + + it("prefers a valid retry-after header over the structured delay", async () => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0.5); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce( + errorResponse(429, retryInfoBody("4s"), { "retry-after": "0.5" }), + ) + .mockResolvedValueOnce(jsonResponse({ ok: true })); + const pending = openAiPostJson( + depsWith(fetchImpl as unknown as typeof fetch), + "/x", + {}, + {}, + ); + await expectSecondFetchAfter(pending, fetchImpl, 500); + }); + + it("caps a long structured delay like a header-declared one", async () => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0.5); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(errorResponse(429, retryInfoBody("39s"))) + .mockResolvedValueOnce(jsonResponse({ ok: true })); + const pending = openAiPostJson( + depsWith(fetchImpl as unknown as typeof fetch), + "/x", + {}, + {}, + ); + await expectSecondFetchAfter(pending, fetchImpl, 5_000); + }); + + it("reads the metadata only on throttling statuses", async () => { + // A 500 carrying RetryInfo-shaped JSON keeps the plain backoff. + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0.5); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(errorResponse(500, retryInfoBody("4s", 500))) + .mockResolvedValueOnce(jsonResponse({ ok: true })); + const pending = openAiPostJson( + depsWith(fetchImpl as unknown as typeof fetch), + "/x", + {}, + {}, + ); + await expectSecondFetchAfter(pending, fetchImpl, 150); + }); + + it("lets caller cancellation interrupt the structured wait", async () => { + vi.useFakeTimers(); + vi.spyOn(Math, "random").mockReturnValue(0.5); + const controller = new AbortController(); + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(errorResponse(429, retryInfoBody("39s"))); + const pending = openAiPostJson( + depsWith(fetchImpl as unknown as typeof fetch), + "/x", + {}, + { signal: controller.signal }, + ); + await vi.advanceTimersByTimeAsync(0); + expect(fetchImpl).toHaveBeenCalledTimes(1); + controller.abort(); + await expect(pending).rejects.toMatchObject({ + status: null, + message: "completion aborted by caller", + }); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + }); + it("wraps network failures as status null and retries them", async () => { const fetchImpl = vi .fn() diff --git a/src/llm/provider/openai/openai-http.ts b/src/llm/provider/openai/openai-http.ts index f64bff84..c25b9c55 100644 --- a/src/llm/provider/openai/openai-http.ts +++ b/src/llm/provider/openai/openai-http.ts @@ -30,7 +30,9 @@ export type OpenAiHttpDeps = { * surface as aborts, but a timeout is "the provider is slower than * the budget" — replaying it just burns another full timeout. * - `retryAfterMs` is populated from a `retry-after` header when the - * provider sent one (429/503), so the retry loop can honor it. + * provider sent one (429/503), falling back to structured + * `RetryInfo` metadata in the error body, so the retry loop can + * honor the provider's cooldown either way. */ export class OpenAiHttpError extends Error { constructor( @@ -280,12 +282,21 @@ async function httpErrorFromResponse( res: Response, ): Promise { const text = await res.text().catch(() => ""); + // The standard `retry-after` header wins; some providers advertise + // their cooldown only inside the error JSON (Gemini's OpenAI-compat + // endpoint sends `google.rpc.RetryInfo` and no header), so fall back + // to that for the throttling statuses the retry loop honors. + const retryAfterMs = + parseRetryAfterMs(res.headers.get("retry-after")) ?? + (res.status === 429 || res.status === 503 + ? parseRetryInfoDelayMs(text) + : null); return new OpenAiHttpError( `openai provider ${res.status}: ${text.slice(0, OPENAI_ERROR_DETAIL_MAX_LEN)}`, res.status, `${deps.baseUrl}${path}`, false, - parseRetryAfterMs(res.headers.get("retry-after")), + retryAfterMs, deps.label, ); } @@ -351,6 +362,41 @@ function resolveWaitMs(err: unknown, attemptNumber: number): number { return Math.max(backoff, retryAfter); } +/** + * Cooldown from structured error JSON, for providers that never send a + * `retry-after` header. Gemini answers 429/503 with an + * `error.details[]` entry of `@type google.rpc.RetryInfo` whose + * `retryDelay` is a protobuf Duration string ("39s", "1.5s"). The + * duration grammar admits only non-negative seconds, so anything else — + * malformed, negative, non-string — is ignored and the caller keeps the + * plain exponential backoff. The result flows through the same + * `retryAfterMs` field as the header, so `resolveWaitMs` caps it at + * `OPENAI_RETRY_AFTER_CAP_MS` exactly like a header-declared wait. + */ +function parseRetryInfoDelayMs(body: string): number | null { + let parsed: unknown; + try { + parsed = JSON.parse(body) as unknown; + } catch { + return null; + } + if (!isRecord(parsed) || !isRecord(parsed.error)) return null; + const details = parsed.error.details; + if (!Array.isArray(details)) return null; + for (const detail of details) { + if (!isRecord(detail) || typeof detail.retryDelay !== "string") continue; + const match = /^(\d+(?:\.\d+)?)s$/.exec(detail.retryDelay); + if (!match) continue; + const ms = Math.round(Number(match[1]) * 1000); + if (Number.isFinite(ms)) return ms; + } + return null; +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + function parseRetryAfterMs(header: string | null): number | null { if (!header) return null; const seconds = Number(header);