Skip to content

fix(llm-client): fall back when an upstream connect times out - #854

Open
ting-hong-shieh wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
ting-hong-shieh:fix/853-connect-timeout-fallback
Open

ting-hong-shieh wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
ting-hong-shieh:fix/853-connect-timeout-fallback

Conversation

@ting-hong-shieh

@ting-hong-shieh ting-hong-shieh commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

What

Classify an upstream connect timeout as Transport, so completion routing can try the next candidate after retries are exhausted.

Why

Closes #853. Reqwest reports connect timeouts as both is_connect() and is_timeout(). Checking timeout first produces Timeout, which stops candidate fallback. Checking connect first treats the failed connection like other connection failures.

Notes for reviewers

Start with convert_reqwest_error in crates/libsy-llm-client/src/client.rs.

  • Response timeouts and expiry of timeout_ms, including expiry during a connect, remain Timeout and stop completion candidate fallback.
  • Retry counts and delays are unchanged. With default retries, the first fallback can still take about 90 seconds on Linux. This PR does not add a per-client connect-timeout setting.
  • Connect-timeout error labels change from timeout to transport. If no completion candidate remains, the response is 502 upstream_error instead of 504 upstream_timeout.
  • Judge failures follow the existing fail_open policy; this PR adds no judge recovery policy. As with other transport failures, a redirected request may already have reached an earlier endpoint before a later connection fails.

The Linux connect-timeout regression fills a listener's accept queue and uses a 100 ms connect timeout. It checks that the source is a reqwest error with both connect and timeout flags, classified as Transport. Reverting the classification to timeout-first makes it fail with Timeout. The refused-connection routing test checks the existing fallback path and passes on main as well.

The branch includes current main, and the fallback fixture explicitly disables failure cooldown.

Validation

At ad10acc0:

  • cargo fmt --all --check: passed.
  • cargo clippy --workspace --all-targets --locked -- -D warnings: passed.
  • cargo test --workspace --locked: 925 passed, 1 ignored (external handoff fixture).
  • cargo clippy -p switchyard-server --all-targets --features prefill-router --locked -- -D warnings: passed.
  • cargo test -p switchyard-runner --features prefill-router --locked: 68 passed.
  • maturin develop --locked --uv: passed with Python 3.14.
  • uv run --no-sync ruff check . and uv run --no-sync mypy switchyard: passed.
  • uv run --no-sync pytest tests/ -v -m "not integration": 187 passed, 2 deselected, 2 subtests passed.
  • uv run --no-sync mkdocs build --strict: passed with the locked docs dependency group.

No live provider call was made.

Summary by CodeRabbit

  • Bug Fixes
    • Connection failures, including timeouts before a connection is established, now allow the next configured model to be tried. Other timeouts stop the request without trying another model.
  • Documentation
    • Clarified how connection failures and other timeouts affect model fallback in the configuration and client documentation.

Signed-off-by: Ting-Hong Shieh <shiehharry@gmail.com>
Signed-off-by: Ting-Hong Shieh <shiehharry@gmail.com>
@ting-hong-shieh
ting-hong-shieh marked this pull request as ready for review October 7, 2026 13:07
@ting-hong-shieh
ting-hong-shieh requested a review from a team as a code owner October 7, 2026 13:07
@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA-NeMo/Switchyard/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 8334c99c-69d5-40d8-8daa-72d68eb36d97
📥 Commits

Reviewing files that changed from the base of the PR and between a3cdc51 and ad10acc.

📒 Files selected for processing (4)
  • crates/libsy-llm-client/README.md
  • crates/libsy-llm-client/src/client.rs
  • crates/libsy-llm-client/src/run.rs
  • docs/reference/toml_schema.md

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.


Walkthrough

Connect errors, including connect timeouts, are classified as Transport failures. Other timeouts remain Timeout failures. Tests cover connect-timeout classification and fallback after a refused connection.

Changes

LLM candidate fallback

Layer / File(s) Summary
Connect error classification
crates/libsy-llm-client/src/client.rs, crates/libsy-llm-client/README.md
convert_reqwest_error checks for connect errors before timeouts. A Linux-only test covers a connect timeout, and the README describes the classification.
Fallback to the next candidate
crates/libsy-llm-client/src/run.rs, docs/reference/toml_schema.md
A test verifies that a refused connection to the first target is followed by a successful response from the next target. The documentation distinguishes connect timeouts from other timeouts.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to ad10a

The change permits fallback for connection failures while preserving request-wide timeout behavior. No concrete merge-blocking issue is established.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning [#853] requires fallback to the capable target when the efficient upstream is unavailable. This PR changes classification only when reqwest reports is_connect(), which also fixes connect-timeout cla… Update the retry/deadline handling so repeated connection-refused failures in the #853 scenario reach candidate fallback as an unavailable transport failure. Add a regression test that exercises the retry-to-deadline path in the issue, whil…
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: classifying upstream connect timeouts so completion routing can fall back to another candidate.
Out of Scope Changes check ✅ Passed The client and routing tests, README update, and TOML schema documentation all cover connect-timeout classification or candidate fallback. The reviewed changes show no unrelated implementation or docu…
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (2 skipped: 2 u…
Full details: Linked Issues check

Explanation

[#853] requires fallback to the capable target when the efficient upstream is unavailable. This PR changes classification only when reqwest reports is_connect(), which also fixes connect-timeout classification. A refused connection was already classified as Transport because it does not set is_timeout(). The new refused-connection test sets max_retries: 0, so it does not cover the issue’s retry-to-deadline failure. In send_with_retries, expiry of the backend deadline returns deadline_error(timeout); the PR description confirms that this remains Timeout and stops fallback. Therefore the reported connection-refused case that retries until the deadline remains unresolved.

Resolution

Update the retry/deadline handling so repeated connection-refused failures in the #853 scenario reach candidate fallback as an unavailable transport failure. Add a regression test that exercises the retry-to-deadline path in the issue, while preserving the intended behavior for genuine response timeouts.

  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit heard the first call fail,
Then watched the next request set sail.
“Connect-timeout,” the client declared,
While other timeouts stayed impaired.
The strong next target sent a reply,
And pleased the rabbit hopping by.

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] stage_router no longer falls back to the capable target when the connection to the efficient upstream times out (0.3.0; 0.2.0 fell back)

1 participant