feat(llm-client): add bounded upstream retries - #173
Conversation
WalkthroughChangesLLM client retries and Anthropic normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/switchyard-server/src/config.rs (1)
202-242: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo upper bound on
max_retrieslets a config value cause unbounded request stalls.
build_backendcopiesconfig.max_retriesstraight intoHttpBackendConfigwith no sanity cap, unlike the other fields validated in this function (base_url,api_key_env). Combined with the client's exponential backoff (up to 2s/attempt) andRetry-Aftercap (up to 60s/attempt), a large or accidentalmax_retriesvalue can hold a request/task open for a very long time, degrading availability under load.🛡️ Proposed bound check
+const MAX_ALLOWED_RETRIES: u32 = 10; + fn build_backend(client_name: &str, config: &LlmClientConfig) -> ServerResult<Backend> { let base_url = config.base_url.trim(); if base_url.is_empty() { return Err(ServerError::new(format!( "llm client {client_name} base_url must not be empty" ))); } + if config.max_retries > MAX_ALLOWED_RETRIES { + return Err(ServerError::new(format!( + "llm client {client_name} max_retries must be at most {MAX_ALLOWED_RETRIES}" + ))); + } let api_key = config🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/switchyard-server/src/config.rs` around lines 202 - 242, Update build_backend to validate config.max_retries before constructing HttpBackendConfig, rejecting values above the established safe maximum with a contextual ServerError. Preserve valid retry values and ensure the validated value, rather than the unchecked configuration value, is assigned to HttpBackendConfig.max_retries.
🧹 Nitpick comments (5)
crates/switchyard-translation/tests/request_translation.rs (1)
48-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a message whose blocks are entirely stripped.
This fixture always leaves at least one signed/visible block, so the assistant-message-of-only-unsigned-thinking path (which currently produces empty content — see
crates/switchyard-translation/src/codecs/anthropic/buffered.rsLines 483-493) is untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/switchyard-translation/tests/request_translation.rs` around lines 48 - 56, Add a test fixture in the request translation coverage where an assistant message contains only unsigned thinking blocks, with no signed thinking or visible text, and assert that translation produces empty content. Use the existing request translation test structure and target the assistant-message handling exercised by the buffered Anthropic codec.crates/switchyard-translation/src/codecs/anthropic/buffered.rs (2)
402-402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd brief intent comments to these helpers.
system_text_from_content_blockandis_unsigned_thinking_blockencode non-obvious contracts (which block types count as system text; what "unsigned" means). As per coding guidelines: "Use///documentation comments for public Rust items and concise comments for module intent, public items, non-obvious helpers, important behavioral tests, and complex validation, routing, configuration, async, lifecycle, or concurrency logic."Also applies to: 496-496
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/switchyard-translation/src/codecs/anthropic/buffered.rs` at line 402, ||||Add concise intent comments immediately above the non-obvious helpers system_text_from_content_block and is_unsigned_thinking_block, documenting which content block types qualify as system text and what “unsigned” means for thinking blocks. Use regular concise comments appropriate to these private Rust helpers, without changing their behavior.Source: Coding guidelines
387-400: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-text content is stringified into the system prompt.
The
other => Some(other.to_string())arm serializes numbers/objects as raw JSON intosystem, and the array arm silently drops non-text blocks (e.g. images). ReturningNonefor non-textual content — with a lossy diagnostic where the codec already pushes them — would be more predictable than leaking JSON literals into the prompt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/switchyard-translation/src/codecs/anthropic/buffered.rs` around lines 387 - 400, The system_text_from_content function should return None for non-textual scalar/object values instead of serializing them with other.to_string(). Update the array handling so non-text blocks are not silently converted or leaked into the system prompt, while preserving text extraction and joining; emit the codec’s existing lossy diagnostic at the point where discarded content is reported.crates/libsy-llm-client/src/client.rs (2)
474-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd brief comments for the retry-classification/backoff internals.
AttemptFailure(what it captures and why) andretry_delay(exponential backoff shape) have no explanatory comment, unlikeretry_after_delayright above them. This is non-obvious async/retry logic that future maintainers will need to reason about.As per coding guidelines, "Use
///documentation comments for public Rust items and concise comments for module intent, public items, non-obvious helpers, ... complex ... async, lifecycle, or concurrency logic."📝 Proposed comments
+// Failure metadata for one upstream attempt: whether it's worth retrying and, +// if so, how long to wait first. struct AttemptFailure { error: LlmClientError, status: Option<u16>, retry_after: Option<Duration>, }+// Prefers the upstream's `Retry-After` hint; otherwise doubles the delay each +// attempt starting from `INITIAL_RETRY_DELAY`, capped at `MAX_RETRY_BACKOFF`. fn retry_delay(retry_number: u32, retry_after: Option<Duration>) -> Duration {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/libsy-llm-client/src/client.rs` around lines 474 - 513, Add concise explanatory comments for the non-obvious retry internals: document AttemptFailure’s captured error/status/retry metadata and its role in retry classification, and document retry_delay’s exponential backoff behavior, including its bounded growth and Retry-After override. Keep the existing retry logic unchanged and use comments appropriate for these private helpers.Source: Coding guidelines
173-219: 🩺 Stability & Availability | 🔵 TrivialRetries can duplicate non-idempotent, billable upstream calls; no overall deadline bounds the loop.
Two related considerations worth weighing, not blockers:
- A
Timeout/Transportfailure is always retried with the identical request body. If the failure is a response-read timeout that occurs after the upstream already processed the (billable) completion, the retry issues a second identical call — most chat/completions APIs don't offer an idempotency key to dedupe this.- The loop has no overall wall-clock deadline of its own; with a large
Retry-After(capped at 60s) andmax_retriesattempts, one logical call can legitimately take several minutes, bounded only by whatever timeout the caller configures onreqwest::Clientper attempt.Worth documenting as an accepted tradeoff, or wrapping the whole retry loop in an overall timeout (e.g.,
tokio::time::timeout) if callers need a hard latency ceiling.Also applies to: 480-489
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/libsy-llm-client/src/client.rs` around lines 173 - 219, Document the retry behavior around the upstream retry loop in the relevant client method: retries of Timeout/Transport failures may duplicate already-processed, billable non-idempotent requests, and Retry-After delays plus max_retries can extend total latency without an overall deadline. If the client contract requires a hard wall-clock bound, wrap the entire retry loop in an overall timeout; otherwise explicitly document this accepted tradeoff, preserving the existing per-attempt retry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/switchyard-translation/src/codecs/anthropic/buffered.rs`:
- Around line 483-493: The strip_unsigned_thinking_from_message flow must not
return a message with empty-string content when all blocks are removed; drop
that message or emit a non-empty placeholder while preserving normal content
handling. In crates/switchyard-translation/src/codecs/anthropic/buffered.rs
lines 483-493, update the kept.is_empty() branch accordingly. In
crates/switchyard-translation/tests/request_translation.rs lines 48-56, add an
assistant message containing only unsigned thinking blocks and assert the
resulting message shape.
---
Outside diff comments:
In `@crates/switchyard-server/src/config.rs`:
- Around line 202-242: Update build_backend to validate config.max_retries
before constructing HttpBackendConfig, rejecting values above the established
safe maximum with a contextual ServerError. Preserve valid retry values and
ensure the validated value, rather than the unchecked configuration value, is
assigned to HttpBackendConfig.max_retries.
---
Nitpick comments:
In `@crates/libsy-llm-client/src/client.rs`:
- Around line 474-513: Add concise explanatory comments for the non-obvious
retry internals: document AttemptFailure’s captured error/status/retry metadata
and its role in retry classification, and document retry_delay’s exponential
backoff behavior, including its bounded growth and Retry-After override. Keep
the existing retry logic unchanged and use comments appropriate for these
private helpers.
- Around line 173-219: Document the retry behavior around the upstream retry
loop in the relevant client method: retries of Timeout/Transport failures may
duplicate already-processed, billable non-idempotent requests, and Retry-After
delays plus max_retries can extend total latency without an overall deadline. If
the client contract requires a hard wall-clock bound, wrap the entire retry loop
in an overall timeout; otherwise explicitly document this accepted tradeoff,
preserving the existing per-attempt retry behavior.
In `@crates/switchyard-translation/src/codecs/anthropic/buffered.rs`:
- Line 402: ||||Add concise intent comments immediately above the non-obvious
helpers system_text_from_content_block and is_unsigned_thinking_block,
documenting which content block types qualify as system text and what “unsigned”
means for thinking blocks. Use regular concise comments appropriate to these
private Rust helpers, without changing their behavior.
- Around line 387-400: The system_text_from_content function should return None
for non-textual scalar/object values instead of serializing them with
other.to_string(). Update the array handling so non-text blocks are not silently
converted or leaked into the system prompt, while preserving text extraction and
joining; emit the codec’s existing lossy diagnostic at the point where discarded
content is reported.
In `@crates/switchyard-translation/tests/request_translation.rs`:
- Around line 48-56: Add a test fixture in the request translation coverage
where an assistant message contains only unsigned thinking blocks, with no
signed thinking or visible text, and assert that translation produces empty
content. Use the existing request translation test structure and target the
assistant-message handling exercised by the buffered Anthropic codec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c1c5a69-d06d-4701-8e45-79743e36b3de
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (14)
Cargo.tomlcrates/libsy-llm-client/Cargo.tomlcrates/libsy-llm-client/README.mdcrates/libsy-llm-client/src/backend.rscrates/libsy-llm-client/src/client.rscrates/libsy-llm-client/src/lib.rscrates/libsy/examples/ensemble.rscrates/switchyard-components/src/backends/anthropic.rscrates/switchyard-server/CONFIGURATION.mdcrates/switchyard-server/README.mdcrates/switchyard-server/src/config.rscrates/switchyard-server/tests/server.rscrates/switchyard-translation/src/codecs/anthropic/buffered.rscrates/switchyard-translation/tests/request_translation.rs
nachiketb-nvidia
changed the title
158747c to
16802c0
Compare
b5ab48f to
6a83860
Compare
nachiketb-nvidia
changed the title
|
I got this from Codex, looks legit. Could you check it?
This is likely because of the metrics stuff I just landed. Might need a rebase to see the issue. |
Signed-off-by: nachiketb <nachiketb@nvidia.com>
Signed-off-by: nachiketb <nachiketb@nvidia.com>
Signed-off-by: nachiketb <nachiketb@nvidia.com>
Signed-off-by: nachiketb <nachiketb@nvidia.com>
6a83860 to
503ed36
Compare
changed which codes are retryable! |
3 participants
What
switchyard-llm-client.max_retriesinswitchyard-serverTOML with a default of2and a maximum of10.Why
Transient transport failures, timeouts, HTTP 408/429 responses, and upstream 5xx responses should not immediately fail a request. The retry budget and delay policy must remain explicit and bounded because retries can replay billable requests and extend total latency.
How
Retry-Afterseconds or HTTP dates with a 60-second cap; otherwise use bounded exponential backoff from 250 ms to 2 seconds.libsy.upstream_attemptspan per physical attempt with status, retry, and delay fields.What to review
Retry-Afterparsing and bounded backoff.Validation
cargo fmt --all --checkcargo clippy -p switchyard-llm-client -p switchyard-server --all-targets -- -D warningscargo test -p switchyard-llm-client -p switchyard-server