Skip to content

feat(libsy-llm-client): harden upstream request handling - #352

Draft
gburachas wants to merge 4 commits into
NVIDIA-NeMo:mainfrom
gburachas:pr/e1-capability-reject
Draft

feat(libsy-llm-client): harden upstream request handling#352
gburachas wants to merge 4 commits into
NVIDIA-NeMo:mainfrom
gburachas:pr/e1-capability-reject

Conversation

@gburachas

Copy link
Copy Markdown

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_secs on [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 HttpTransportConfig rather than keeping a parallel mechanism.

Since #316 moved token counting to an inherent method that still routes through send_encoded, this also bounds count_tokens — desirable, but new, so worth stating.

2. Retry once without injected extra_body on a parameter reject (#260)

merge_extra_body only 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::CapabilityRejected and RoutingFallbackReason::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:

RoutingFallbackReason::ContextWindow | RoutingFallbackReason::Capability => {
    evictions.record(identity, failed)
}
RoutingFallbackReason::Unavailable => target_unavailable(&request, failed),

Mapping it to Unavailable instead 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_overflow pins it by asserting the target is probed exactly once across three turns; I verified that test fails under the Unavailable mapping.

This is a public-API addition to protocol, which is why it is in draft.

How tested

  • cargo fmt --all --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace, and again with --test-threads=1
  • uv run pytest tests/ -m "not integration" — 871 passed, 9 skipped
  • uv run ruff check ., mkdocs build --strict
  • Commits signed off per the DCO

Each commit builds and tests independently, so the first two can be split out if the third needs longer.

Rebased onto current main. The fall_through.rs test harness needed a full rewrite for #337/#338/#340 (LlmTarget lost llm_client, Arc<dyn Decision>Arc<Decision>, Serve closures replaced client structs); it is modelled on the existing overflowing helper.

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant