Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/llm-gateway-error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,35 @@ describe("parseRateLimitRetryAt", () => {
])("returns undefined for $name", ({ message }) => {
expect(parseRateLimitRetryAt(message, NOW)).toBeUndefined()
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️🔧 Maintainability

The comment block above the new tests repeats the rationale paragraph from src/llm-gateway-error.ts (lines ~152-156) almost verbatim, so the two copies can drift apart as the wording evolves in one place only.

💡 Suggestion: Keep the full rationale in the source next to RATE_LIMIT_IN_RE and shorten the test comment to a one-line pointer, e.g. // Covers the relative Retry-After text form; see RATE_LIMIT_IN_RE for why this exists.

// A plain 429 does not use the gateway's "rate limited until" wording. It
// carries a Retry-After, which reaches this extension only as text, rendered
// as a relative wait -- so without this the deadline is lost and the retry
// backs off blindly against a limit that just said how long it lasts.
it.each([
{ name: "seconds", message: "429 Too Many Requests, retry after 30 seconds", ms: 30_000 },
{ name: "abbreviated seconds", message: "Too many requests. Try again in 45s.", ms: 45_000 },
{ name: "minutes", message: "Rate limit exceeded; please try again in 2 minutes", ms: 120_000 },
{ name: "hours", message: "quota exhausted, resets in 1 hour", ms: 3_600_000 },
{ name: "milliseconds", message: "rate limited, retry after 1500ms", ms: 1_500 },
{ name: "a fractional amount", message: "retry after 1.5 seconds", ms: 1_500 },
])("reads a relative wait stated in $name", ({ message, ms }) => {
expect(parseRateLimitRetryAt(message, NOW)).toBe(NOW + ms)
})

it("prefers the absolute deadline when the message states both", () => {
// The absolute form needs no arithmetic and no assumption about when the
// message was produced.
expect(parseRateLimitRetryAt("rate limited until 2026-08-05T16:27:33Z, retry after 5 seconds", NOW)).toBe(EXPECTED)
})

it.each([
{ name: "a zero wait", message: "retry after 0 seconds" },
{ name: "an unknown unit", message: "retry after 5 fortnights" },
{ name: "a duration with no retry wording", message: "the request took 30 seconds" },
])("returns undefined for $name", ({ message }) => {
expect(parseRateLimitRetryAt(message, NOW)).toBeUndefined()
})
})

describe("formatWait", () => {
Expand Down
50 changes: 44 additions & 6 deletions src/llm-gateway-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,53 @@ export function parseModelRetiredInfo(rawMessage: string): ModelRetiredInfo | un
const RATE_LIMIT_UNTIL_RE = /rate.?limited\s+until\s+([0-9T][0-9TZ:+.-]*[0-9Z])/i
const EXPLICIT_ZONE_RE = /[Zz]$|[+-]\d{2}:?\d{2}$/

// A plain 429 does not use the gateway's "rate limited until" wording. What it
// carries instead is a Retry-After, which reaches this extension only as text --
// pi hands the message along and not the response -- rendered as a relative
// wait. Without this, "429 Too Many Requests, retry after 30 seconds" yields no
// deadline at all and the retry backs off blindly against a limit that had just
// said how long it lasts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️🐛 Bug

RATE_LIMIT_IN_RE has no leading word boundary on the trigger alternation, so available matches as a substring of unavailable and reset[s]? matches inside words like presets. A message such as Service unavailable in 5 minutes (a plausible non-rate-limit gateway error) would be misread as a rate-limit hint and produce a bogus retry deadline 5 minutes out, delaying or scheduling retries on the wrong signal.

💡 Suggestion: Anchor the trigger group with a word boundary: /\b(?:retry|try again|available|reset[s]?)\s*(?:after|in)\s+(\d+(?:\.\d+)?)\s*(ms|milliseconds?|s|secs?|seconds?|m|mins?|minutes?|h|hours?)\b/i, and add a negative test for a message like Service unavailable in 5 minutes returning undefined.

const RATE_LIMIT_IN_RE =
/(?:retry|try again|available|reset[s]?)\s*(?:after|in)\s+(\d+(?:\.\d+)?)\s*(ms|milliseconds?|s|secs?|seconds?|m|mins?|minutes?|h|hours?)\b/i

const UNIT_MS: Record<string, number> = {
ms: 1,
millisecond: 1,
milliseconds: 1,
s: 1_000,
sec: 1_000,
secs: 1_000,
second: 1_000,
seconds: 1_000,
m: 60_000,
min: 60_000,
mins: 60_000,
minute: 60_000,
minutes: 60_000,
h: 3_600_000,
hour: 3_600_000,
hours: 3_600_000,
}

/** Epoch ms the gateway says the limit lifts, or undefined when it named none or it has passed. */
export function parseRateLimitRetryAt(rawMessage: string, now: number = Date.now()): number | undefined {
const match = RATE_LIMIT_UNTIL_RE.exec(rawMessage)
if (!match?.[1]) return undefined
const stamp = match[1]
// The gateway reports UTC; Date.parse would read an unzoned stamp as local time.
const retryAt = Date.parse(EXPLICIT_ZONE_RE.test(stamp) ? stamp : `${stamp}Z`)
if (Number.isNaN(retryAt) || retryAt <= now) return undefined
return retryAt
if (match?.[1]) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️⚠️ Error Handling

When RATE_LIMIT_UNTIL_RE matches but the captured timestamp is unparsable or already in the past, the function returns undefined immediately and never attempts the new relative fallback. A message that states both forms with a stale absolute stamp (e.g. a delayed/replayed rate limited until <past>, retry after 5 seconds) loses a perfectly usable relative wait.

💡 Suggestion: Restructure so the relative path is tried when the absolute parse fails: extract the absolute-parsing into a block that falls through instead of returning (e.g. compute retryAt inside the if (match?.[1]) branch and only return retryAt when valid), or document that an expired absolute stamp intentionally suppresses the relative hint.

const stamp = match[1]
// The gateway reports UTC; Date.parse would read an unzoned stamp as local time.
const retryAt = Date.parse(EXPLICIT_ZONE_RE.test(stamp) ? stamp : `${stamp}Z`)
if (Number.isNaN(retryAt) || retryAt <= now) return undefined
return retryAt
}

// The absolute form is preferred above because it needs no arithmetic; this
// is the fallback for a message that states a duration instead.
const relative = RATE_LIMIT_IN_RE.exec(rawMessage)
if (!relative?.[1]) return undefined
const amount = Number(relative[1])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️⚠️ Error Handling

Relative durations are unbounded: \d+ accepts arbitrarily large amounts, so retry after 100000000 hours yields a far-future timestamp and a ~300-digit amount produces Infinity from now + amount * unit (the Number.isFinite(amount) check only guards the parsed amount, not the product). Downstream callers feeding retryAt - now into a timer can hit runtime overflow clamping (e.g. Node's setTimeout overflow warning clamps to ~1 ms), turning an hours-long wait into an immediate retry storm.

💡 Suggestion: Clamp the computed delay to a sane ceiling before returning, e.g. const delay = Math.min(amount * unit, MAX_RETRY_WAIT_MS) with a constant such as 24 hours, and guard with Number.isFinite(delay).

const unit = UNIT_MS[relative[2].toLowerCase()]
if (!Number.isFinite(amount) || amount <= 0 || unit === undefined) return undefined
return now + amount * unit
}

/** The gateway reports UTC; only local wall-clock time tells the user when to come back. */
Expand Down