fix(gateway): surface actionable message for local model server connection errors - #86729
Conversation
trevorgordon981
left a comment
There was a problem hiding this comment.
Good, well-tested fix for #86570. Giving a specific "local model server not running / unreachable" message when the configured endpoint is down is genuinely more actionable than the generic fallback, and the tests are solid (connection-error samples produce the specific reply, non-connection errors keep the generic one, auth/rate-limit branches preserved). Two things worth a look.
1. The connection patterns now double as a provider-error gate, not just a reply selector
_GATEWAY_CONNECTION_ERROR_RE is used in two places: to pick the reply inside _gateway_provider_error_reply, and the same patterns are appended to the _looks_like_gateway_provider_error gate (via the added alternation). That means a message containing e.g. cannot connect / could not connect / connection reset / connection missing can now classify as a "provider error" even when it isn't one. Most of these are specific enough that false positives are unlikely, but connection reset, network is unreachable, and the bare cannot connect / could not connect phrases are generic enough to appear in unrelated user content that flows through the same error-classification path (e.g. a user pasting a network diagnostic, or an agent turn that happens to mention a connection problem). Worth confirming the gate isn't overly broad — the safest split is to keep the reply selector on the full set, but gate _looks_like_gateway_provider_error only on the unambiguous forms (connection refused/reset/aborted, WinError 10061, errno 111, all connection attempts failed).
2. Regex ordering: a connection error that also mentions auth/rate-limit is hijacked
In _gateway_provider_error_reply, the auth and rate-limit branches are checked before the new connection branch. If a connection failure message happens to include authentication or rate limit text (some networking errors do — e.g. a proxy or TLS error that surfaces a 401/429-style message), it returns the auth/rate-limit reply instead of the (more accurate) connection reply. Minor, but the ordering means the new branch is strictly lowest-priority; that's probably fine given the existing conventions, just worth being deliberate about.
3. Minor
- The
winerror 10061anderrno 111patterns are good, concrete Windows/Linux anchors — nice.
Tests
Good coverage: specific-reply for 7 connection samples, generic preserved for 3 non-connection errors, regex negative cases, and auth/rate-limit branches still win. The gap is finding #1 — no test asserts that a non-error message containing cannot connect (in prose, not as an error) stays out of the provider-error gate.
fix(gateway): surface actionable message for local model server connection errors A targeted fix with good regression coverage. A few observations:
|
f6927e8 to
d600150
Compare
|
Thanks — split the gate from the reply selector. Unambiguous transport failures (refused/reset/aborted, WinError 10061, errno 111, all connection attempts failed) still open the rewrite. Broader phrases like |
trevorgordon981
left a comment
There was a problem hiding this comment.
Re-reviewing the follow-up commit ("keep broad connection phrases out of the provider-error gate"): my finding #1 is addressed cleanly. You split the gate from the reply selector — _GATEWAY_CONNECTION_ERROR_RE now anchors the _looks_like_gateway_provider_error gate on the unambiguous transport forms (refused/reset/aborted, WinError 10061, errno 111, all connection attempts failed), while the reply selector keeps the broader phrase set for messages already classified as provider errors. That is exactly the right split.
The regression coverage closes the gap I flagged: test_prose_cannot_connect_is_not_a_provider_error pins a prose mention of "cannot connect" out of the provider-error gate, and test_broad_connection_phrases_still_map_once_classified confirms the wider phrase set still routes to the specific reply once classified. Both verified passing here.
Finding #2 (reply-branch ordering, auth/rate-limit before connection) is preserved and now pinned by test_auth_and_rate_limit_preserved — that matches my "probably fine given existing conventions" note, so non-blocking. Approving.
What does this PR do?
Local model server connection errors now get a specific "not running or unreachable" reply instead of the generic provider-failure catch-all.
Review follow-up: the provider-error gate only matches unambiguous transport failures (refused/reset/aborted, WinError 10061, errno 111, all connection attempts failed). Broader phrases like
cannot connectstill map to the specific reply once classified, but they no longer open the rewrite gate on user prose.Related Issue
Fixes #86570
Type of Change
Changes Made
gateway/run.py: connection-error reply branch; narrower connection markers in_GATEWAY_PROVIDER_ERROR_SHAPE_RE.cannot connect, and reply-only broad phrases.How to Test
uv run python -m pytest tests/gateway/test_local_model_connection_reply.py -q→ 6 passed.test_prose_cannot_connect_is_not_a_provider_errorfails if the gate still treatscannot connectas an envelope.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/gateway/test_local_model_connection_reply.py -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A