[ting-hong-shieh]

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 main 9ddd4879, and the fallback fixture explicitly disables failure cooldown.

Validation

At 8aaaa0fb, integrated with main 9ddd4879:

• Rust workspace: 1,109 passed, 1 ignored (external handoff fixture).
• Prefill-router runner: 82 passed.
• Workspace and prefill Clippy with warnings denied, formatting, and strict docs build: passed.
• Python bindings rebuilt with maturin on Python 3.12; Python: 198 passed, 2 integration tests deselected, 2 subtests passed.
• Ruff and mypy: passed.

Local validation ran on macOS. The accept-queue connect-timeout regression is Linux-only and must be confirmed by the new Linux CI run. Mock HTTP tests made no live provider calls.

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.

[coderabbitai]

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		[#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
Out of Scope Changes check		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		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…
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the primary change: upstream connect timeouts now trigger candidate fallback instead of a timeout error. The raw summary and objectives confirm that convert_reqwest_error now classifie…

Full details: Out of Scope Changes check

Explanation

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 documentation work.

Full details: Docstring Coverage

Explanation

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 unsupported.)

Full details: Title check

Explanation

The title names the primary change: upstream connect timeouts now trigger candidate fallback instead of a timeout error. The raw summary and objectives confirm that convert_reqwest_error now classifies connect-phase errors as Transport, so the fallback follows the transport path. The title is specific and concise, and it matches the linked issue #853 behavior. It does not mention that response timeouts and timeout_ms expiry still stop fallback, but that is a detail the title does not need to carry.

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.

• 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.}

[ting-hong-shieh]

Updated with main 9ddd4879 at 8aaaa0fb. Could a maintainer approve the pending fork workflow runs and review this PR?

Local validation passed: Rust workspace (1,109 passed, 1 ignored), prefill runner (82 passed), Python (198 passed, 2 integration tests deselected), Clippy, formatting, Ruff, mypy, and strict docs build. The connect-timeout regression is Linux-only, so the new Linux CI result is still needed.

The latest CI, documentation, getting-started, and package-portability runs show action_required. Commitlint separately reports startup_failure before creating a job; could you also help check that workflow? The PR description contains the updated validation details.

[ayushag-nv]

Thanks @ting-hong-shieh for this one. Neat fix.