Repository navigation
Conversation
… for rate-limit retries Lets retry-after extractors honor structured rate-limit hints (retry-after, x-ratelimit-reset-*) from HTTP response headers instead of parsing them out of unstructured error bodies. - KoogHttpClientException gains a Map<String, List<String>> of response headers. Keys are normalized to lowercase; added as a trailing optional constructor parameter (via JvmOverloads) to preserve source/binary compatibility. - Ktor, OkHttp, and Java HTTP clients populate headers at every throw site. - RetryAfterExtractor gains an extract(KoogHttpClientException) default overload that falls through to the existing extract(message) path, keeping fun interface SAM compatibility. - StandardHeaderRetryAfterExtractor parses retry-after (seconds / IMF-fixdate) and x-ratelimit-reset-* (OpenAI-style durations); CompositeRetryAfterExtractor stacks extractors. RetryConfig now defaults to Composite(Standard, Default) so existing users get header-aware retries for free when they become available. - RetryingLLMClient.calculateDelay dispatches to the header-aware overload when the caught throwable is a KoogHttpClientException (directly or one level deep via .cause, to cover wrappers like LLMClientException). Tests cover header capture end-to-end for all three HTTP clients, the new extractor's parsing rules, and virtual-time delay assertions through the retry layer. closes JetBrains#1354 (cherry picked from commit a6bc613)
shouldRetry only inspected error.message, so a wrapper whose own message lacked retry tokens (e.g. "LLM call failed") short-circuited to false before calculateDelay - which does walk .cause - ever ran. Factor the unwrap into Throwable.unwrapHttpException() and use it from both, then extract the retry-after dispatch into retryAfterHint() so calculateDelay reads as a single ?: expression. Regression test: wrapper with no retryable tokens in its own message but a 429 KoogHttpClientException as cause must still retry. (cherry picked from commit 0d1af97)
…er hint testCaptureHeadersOnNonSuccess only exercised processResponse, leaving the SSE error-before-stream paths in all three clients untested. Extend SSEEndpointConfig so the mock can return 429 + headers before streaming; add testCaptureHeadersOnSseError, overridden per client. Also: retry-after: 10 + x-ratelimit-reset-requests: 0s must resolve to 10s - zero must not clobber a usable hint. (cherry picked from commit 324f775)
- Rewrite the custom-extractor section: drop the noisy SAM-lambda + .let chaining example in favor of a clean object implementing RetryAfterExtractor. - Add a sentence clarifying that retry-after: 0 (and expired HTTP-dates / zero-length OpenAI durations) are treated as "no hint" rather than "retry immediately" - the caller falls back to exponential backoff. - Restructure the custom-extractor example into Kotlin/Java tabs matching the rest of the page; the Java tab plugs an anonymous RetryAfterExtractor into RetryConfig.builder(). (cherry picked from commit c741197)
- CompositeRetryAfterExtractor: override toString() to render the consultation chain so retry logs make extractor order obvious. - parseOpenAIDuration: replace the unit-millis Double arithmetic with amount.toDuration(unit) using kotlin.time.DurationUnit. (cherry picked from commit b65f376)
RetryConfig.builder() is a Kotlin-only extension on the Companion and isn't callable from Java; the builder class itself is the Java entry point. Switch the example to `new RetryConfigBuilder()` so it actually compiles when copy-pasted. (cherry picked from commit c2ed567)
…MClient Random.nextDouble requires from < until, so `Random.nextDouble(0.0, 0.0)` throws IllegalArgumentException. The retry path hit this whenever `jitterFactor == 0.0` (documented as a valid "no jitter" setting and accepted by RetryConfig.require) or `initialDelay == Duration.ZERO`, silently converting a retryable error into an unrelated IAE. Skip the draw when the upper bound collapses to zero and add a regression test exercising jitterFactor = 0.0 through the public execute() retry path. (cherry picked from commit bbf7dd4)
Address review findings on the header-exposure work: - Normalize header keys once in the KoogHttpClientException constructor instead of at every throw site; clients now pass native header maps. - Capture headers at the lines() streaming throw sites in the Ktor, OkHttp, and Java clients and at the Spring WebClient error chokepoint, so streaming 429s (e.g. Ollama) keep their rate-limit hints; cover the path with testCaptureHeadersOnLinesError in all four client suites. - Treat retry-after as authoritative over x-ratelimit-reset-* instead of taking the global minimum; scan every value of repeated headers, parse comma-folded values, and reject non-positive or non-finite hints. - Make header names configurable on StandardHeaderRetryAfterExtractor, switch its clock to KoogClock, and parse reset durations with Duration.parseOrNull instead of a hand-rolled token grammar. - Cap extracted hints at RetryConfig.maxDelay, walk the full cause chain when unwrapping KoogHttpClientException, and fall back to the outer wrapper's message when header extraction yields nothing. - Guard the OkHttp SSE onFailure body read so a stripped response body cannot skip close(exception) and hang the collector. - Refresh ABI dumps, docs, and tests accordingly. (cherry picked from commit 8669aad)
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1354. Includes and extends #1880.
This includes Kimon N. (@qflen)'s eight feature commits from #1880, preserving authorship and replayed onto current
develop. They expose HTTP response headers for rate-limit retries, add header-aware retry-after extraction, and handle zero jitter. The additional fix caps the final exponential backoff delay after adding jitter: an initial delay and maximum of one second must not produce a delay approaching two seconds.The final-cap fix is already included and working in production applications through Kroog. This PR adapts that part of the original implementation without adding Kroog's retry observers or classification changes.
Maintainers can merge this PR directly, merge #1880 first and have this branch rebased to retain the additional fix, or incorporate the additional commit into #1880. Credit for the header-aware retry implementation belongs to Kimon N. (@qflen).
The inherited changes add
http-client-coreas an API dependency ofprompt-executor-clients, expose response headers onKoogHttpClientException, and add header-aware extractor APIs. They also broaden retry matching to wrapped HTTP errors and cap server-provided retry hints. Our final-cap correction adds no dependency or public API. Existing Android and KLIB ABI changes come from #1880; local validation is JVM-only.Compatibility caveat inherited from #1880: changing the defaulted
KoogHttpClientExceptionconstructor changes its synthetic Kotlin JVM constructor descriptor. Already compiled Kotlin callers using default arguments may require recompilation;@JvmOverloadsdoes not preserve that synthetic entry point. Test-fixture constructor and copy signatures also change. Matching the checked-in ABI dumps below establishes dump consistency, not binary compatibility with earlier releases.Validation: the new cap regression failed before the fix (1,717ms against a 1,000ms limit) and passed afterwards. All 248 JVM tests passed: HTTP core 10, Ktor 23, Java 40, OkHttp 40, Spring WebClient 52, and prompt clients 83, with no skips. Affected JAR, lint and publication metadata checks passed; seven generated JVM ABI dumps match their checked-in dumps. Independent review accepted the change. Non-JVM targets were not tested.