Conversation
…deadline `parseRateLimitRetryAt` understands one phrasing: the gateway's "rate limited until <timestamp>". A plain 429 does not use it -- it carries a `Retry-After`, which reaches an extension only as text, rendered as a relative wait. The existing suite pins that gap in place: `"429 Too Many Requests"` is asserted to yield undefined. When it does, `rateLimitWaitMs` has nothing to report, so the notice cannot say when the limit lifts and `prepareRetry` falls back to exponential backoff against a limit that had just said how long it lasts. A relative form is now read as a fallback: "retry after 30 seconds", "try again in 45s", "resets in 1 hour", "retry after 1500ms". The absolute deadline is still preferred when a message states both -- it needs no arithmetic and no assumption about when the message was produced. A zero wait, an unrecognised unit, and a duration with no retry wording all stay undefined, so an unrelated "the request took 30 seconds" is not mistaken for a deadline. Verified: `vitest run --dir src` -- 10949 passed, 17 skipped. The new cases fail on the unmodified parser (6 failures) and pass with it.
|
📚 No documentation changes were needed for this PR. |
Kimchi Code Review
Summary📊 Review Score: 86/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Tests are thorough: six positive variants covering all unit families (including fractional amounts and the 📝 Found 4 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 86/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Tests are thorough: six positive variants covering all unit families (including fractional amounts and the ms abbreviation with no space), a precedence test for absolute-over-relative, and three negative cases (0 seconds, unknown unit, duration without retry wording). Minor gaps: no false-positive guard test (e.g. Service unavailable in 5 minutes), no test for an expired/invalid absolute timestamp combined with a valid relative duration, and no test for absurdly large durations.
📝 Found 4 issue(s). See inline comments for details.
| // 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. |
There was a problem hiding this comment.
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 retryAt = Date.parse(EXPLICIT_ZONE_RE.test(stamp) ? stamp : `${stamp}Z`) | ||
| if (Number.isNaN(retryAt) || retryAt <= now) return undefined | ||
| return retryAt | ||
| if (match?.[1]) { |
There was a problem hiding this comment.
ℹ️
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.
| // 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]) |
There was a problem hiding this comment.
ℹ️
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).
| ])("returns undefined for $name", ({ message }) => { | ||
| expect(parseRateLimitRetryAt(message, NOW)).toBeUndefined() | ||
| }) | ||
|
|
There was a problem hiding this comment.
ℹ️🔧 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.
1 participant
parseRateLimitRetryAtunderstands one phrasing — the gateway'srate limited until <timestamp>.A plain 429 does not use it. It carries a
Retry-After, which reaches an extension only as text, rendered as a relative wait. The existing suite pins that gap in place:When no deadline is found,
rateLimitWaitMshas nothing to report — sorateLimitNoticecannot say when the limit lifts, andprepareRetryfalls back to exponential backoff against a limit that had just said how long it lasts.What changed
A relative form is read as a fallback:
429 Too Many Requests, retry after 30 secondsToo many requests. Try again in 45s.Rate limit exceeded; please try again in 2 minutesquota exhausted, resets in 1 hourrate limited, retry after 1500msThe absolute deadline is still preferred when a message states both — it needs no arithmetic and no assumption about when the message was produced.
A zero wait, an unrecognised unit, and a duration with no retry wording all stay
undefined, so an unrelated "the request took 30 seconds" is not mistaken for a deadline.Testing
vitest run --dir src— 10949 passed, 17 skipped.The new cases fail on the unmodified parser (6 failures) and pass with it, checked by restoring
src/llm-gateway-error.tsfrom HEAD and re-running.pnpm run lintclean.