feat(libsy-llm-client): harden upstream request handling - #352
Draft
gburachas wants to merge 4 commits into
Draft
Conversation
…meout Signed-off-by: Giedrius Burachas <gburachas@nvidia.com>
…anicking Signed-off-by: Giedrius Burachas <gburachas@nvidia.com>
…arameter reject Signed-off-by: Giedrius Burachas <gburachas@nvidia.com>
Signed-off-by: Giedrius Burachas <gburachas@nvidia.com>
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
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.
Draft — the third commit carries an open design question (#345) that I would like a view on before this is landable. The first two are ready.
Closes #277, closes #260, closes #345. Refs #271.
1. Bound upstream requests with a configurable timeout (#277)
timeout_secson[llm_clients.<name>], plus a guard treating an unusable value as no timeout rather than panicking.Complementary to #271, not overlapping: that PR adds connect and idle-read timeouts at the reqwest-builder level; this is the total per-request bound, exposed in the deployment TOML. An upstream that emits a token every 100 s never trips an idle-read timeout. If #271 lands first I will wire this to its
HttpTransportConfigrather than keeping a parallel mechanism.Since #316 moved token counting to an inherent method that still routes through
send_encoded, this also boundscount_tokens— desirable, but new, so worth stating.2. Retry once without injected
extra_bodyon a parameter reject (#260)merge_extra_bodyonly inserts keys the request omits, so it now returns the keys it actually injected. On a parameter reject those exact keys are stripped from the encoded body and the request is retried once.The strip is not a budgeted retry — it does not consume
max_retries— because it is a correction of our own injection rather than a hopeful repeat of the same request. A persistent reject surfaces rather than looping; there is a test for that.3. Evict targets that reject a request as unservable (#345) — the part needing a view
Adds
LlmClientError::CapabilityRejectedandRoutingFallbackReason::Capability, so a target that rejects a request as one it cannot serve at all falls forward instead of failing the turn.The question is which existing behaviour it should share. A capability reject looks like an availability problem, but it is a permanent property of that target for this conversation, so this maps it alongside
ContextWindow:Mapping it to
Unavailableinstead compiles cleanly and is invisible in a single-turn test — it just silently re-probes the rejecting target every turn for the rest of the session.a_capability_reject_falls_forward_and_evicts_like_an_overflowpins it by asserting the target is probed exactly once across three turns; I verified that test fails under theUnavailablemapping.This is a public-API addition to
protocol, which is why it is in draft.How tested
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace, and again with--test-threads=1uv run pytest tests/ -m "not integration"— 871 passed, 9 skippeduv run ruff check .,mkdocs build --strictEach commit builds and tests independently, so the first two can be split out if the third needs longer.
Rebased onto current
main. Thefall_through.rstest harness needed a full rewrite for #337/#338/#340 (LlmTargetlostllm_client,Arc<dyn Decision>→Arc<Decision>,Serveclosures replaced client structs); it is modelled on the existingoverflowinghelper.