Repository navigation
fix(router): distinguish overload from unavailable [DYN-3279] - #10941
Conversation
Signed-off-by: PeaBrane <yanrpei@gmail.com>
GitHub Actions
Inactive
github-actions
Bot
added
fix
documentation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
🐛 1 issue in files not directly in the diff
🐛 Anthropic path returns 500 instead of 503/529 for Unavailable and ResourceExhausted errors (lib/llm/src/http/service/anthropic.rs:352-371)
This PR introduces ErrorType::Unavailable in push_router.rs:655-665 and properly handles it on the OpenAI path via from_anyhow (openai.rs:391-396), returning HTTP 503. However, the Anthropic endpoint at anthropic.rs:352-371 uses its own inline error handler that only checks request_was_rejected (for metrics only — no early return) and request_was_cancelled, then falls through to SanitizedError::Internal (HTTP 500) for everything else. When no workers are available, the push router now emits ErrorType::Unavailable, which the OpenAI path correctly maps to 503 but the Anthropic path misclassifies as 500 Internal Server Error. Additionally, ResourceExhausted errors on the Anthropic path only increment the rejection metric at line 353 but don't return SanitizedError::Overloaded — they also fall through to 500. The ResourceExhausted → 500 issue is pre-existing, but the Unavailable → 500 inconsistency is newly introduced because Unavailable is a new error type from this PR.
WalkthroughUpdated overload handling to use HTTP 529 across runtime errors, HTTP service mappings, OpenAI responses, router tests, and docs. Added a distinct ChangesHTTP 529 overload handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/router/test_router_e2e_with_mockers.py (1)
1181-1193: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd an explicit timeout to this E2E test.
This case starts services, waits for frontend readiness, and exercises multiple HTTP calls, but it still has no
@pytest.mark.timeout(...). If/busy_thresholdor discovery wedges, this can hang the pytest worker indefinitely.Suggested fix
+@pytest.mark.timeout(120) def test_busy_threshold_endpoint(As per coding guidelines, "Flag any test running over 30 seconds or containing
time.sleep(), polling loops, network calls, or subprocess waits without a@pytest.mark.timeout(...)marker as a required change"; as per path instructions,.ai/pytest-guidelines.mdsays "any polling/sleep/network/subprocess waits must have@pytest.mark.timeout(...)"; based on learnings, this repo does not configure a global pytest-timeout default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/router/test_router_e2e_with_mockers.py` around lines 1181 - 1193, Add an explicit pytest timeout marker to the busy_threshold E2E test so it cannot hang indefinitely if discovery or /busy_threshold wedges. Update the test function test_busy_threshold_endpoint in the router E2E suite by adding an appropriate `@pytest.mark.timeout`(...) decorator, matching the repo’s timeout expectations for network/waiting tests.Sources: Coding guidelines, Path instructions, Learnings
🧹 Nitpick comments (1)
tests/router/common.py (1)
1157-1159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lazy logging in the new overload helpers.
These new
logger.*calls eagerly format strings with f-strings. Please switch them to placeholder-style logging so the helpers stay aligned with the repo’s Python logging rule.Suggested fix
- logger.info(f"Request {req_id} got expected 529: {body}") + logger.info( + "Request %s got expected 529: %s", req_id, body + ) ... - logger.info( - f"Results: {num_succeeded} succeeded, {num_rejected} rejected (529), " - f"{num_other} other" - ) + logger.info( + "Results: %s succeeded, %s rejected (529), %s other", + num_succeeded, + num_rejected, + num_other, + ) ... - logger.info( - f"Successfully verified overload 529: {num_rejected} rejected, " - f"{num_succeeded} succeeded, metrics match" - ) + logger.info( + "Successfully verified overload 529: %s rejected, %s succeeded, metrics match", + num_rejected, + num_succeeded, + ) ... - logger.info( - f"Starting disagg KV router frontend on port {frontend_port} for overload 529 test" - ) + logger.info( + "Starting disagg KV router frontend on port %s for overload 529 test", + frontend_port, + )As per coding guidelines, "Use lazy formatting for log messages; pass format strings and arguments separately to logging calls instead of using f-strings or eager formatting"; as per path instructions,
.ai/python-guidelines.mdsays to "use lazy logging (no f-strings / eager formatting insidelogger.*calls)".Also applies to: 1225-1245, 1367-1369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/router/common.py` around lines 1157 - 1159, Replace the eager f-string logging in the overload helper flow with lazy placeholder-style logging so it matches the repo’s Python logging rule. Update the relevant `logger.*` calls in `tests/router/common.py`, including the `logger.info` in the `response.status == 529` branch and the other helper logs around the mentioned overload sections, to pass a format string plus arguments instead of interpolating values upfront. Keep the same messages and context, but ensure the logging calls defer formatting until needed.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/llm/src/http/service/metrics.rs`:
- Around line 39-45: Add a dedicated unavailable metrics classification and stop
routing all 503 responses through ErrorType::Overload. Update the HTTP error
classifier in openai.rs so request_was_unavailable() maps no-routable-worker
503s to a new ErrorType::Unavailable variant, while keeping true overload cases
on Overload. Then update test_classify_error_for_metrics_status_codes and any
related metrics assertions to expect the new unavailable value, and ensure the
metrics enum and all match sites are plumbed through consistently.
In `@lib/llm/src/http/service/openai.rs`:
- Around line 140-145: The status-to-ErrorType mapping in openai.rs is still
collapsing every 503 into ErrorType::Overload, which makes the new unavailable
path indistinguishable from overload. Update the classification logic in the
response/error mapping used by the OpenAI service so the unavailable case from
from_anyhow(...Unavailable...) and model_unavailable() is preserved as its own
category or keyed off the sanitized unavailable message, while keeping any
legacy/backend overload handling separate. Use the existing OpenAI error mapping
branch that matches StatusCode::SERVICE_UNAVAILABLE and related helpers to make
this distinction without changing other status handling.
---
Outside diff comments:
In `@tests/router/test_router_e2e_with_mockers.py`:
- Around line 1181-1193: Add an explicit pytest timeout marker to the
busy_threshold E2E test so it cannot hang indefinitely if discovery or
/busy_threshold wedges. Update the test function test_busy_threshold_endpoint in
the router E2E suite by adding an appropriate `@pytest.mark.timeout`(...)
decorator, matching the repo’s timeout expectations for network/waiting tests.
---
Nitpick comments:
In `@tests/router/common.py`:
- Around line 1157-1159: Replace the eager f-string logging in the overload
helper flow with lazy placeholder-style logging so it matches the repo’s Python
logging rule. Update the relevant `logger.*` calls in `tests/router/common.py`,
including the `logger.info` in the `response.status == 529` branch and the other
helper logs around the mentioned overload sections, to pass a format string plus
arguments instead of interpolating values upfront. Keep the same messages and
context, but ensure the logging calls defer formatting until needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eab56319-d341-451d-9a2f-a1a7629937d7
📒 Files selected for processing (16)
docs/components/router/router-configuration.mddocs/fault-tolerance/request-rejection.mddocs/getting-started/introduction.mddocs/getting-started/introduction.zh-CN.mdlib/llm/src/discovery/worker_monitor.rslib/llm/src/http/service/busy_threshold.rslib/llm/src/http/service/error.rslib/llm/src/http/service/metrics.rslib/llm/src/http/service/openai.rslib/llm/src/http/service/openapi_docs.rslib/runtime/src/error.rslib/runtime/src/metrics/prometheus_names.rslib/runtime/src/pipeline/network/egress/addressed_router.rslib/runtime/src/pipeline/network/egress/push_router.rstests/router/common.pytests/router/test_router_e2e_with_mockers.py
Signed-off-by: PeaBrane <yanrpei@gmail.com>
GitHub Actions
Inactive
|
Addressed the review feedback in 97e235f:
The timeout finding required no change: test_busy_threshold_endpoint already has @pytest.mark.timeout(120) immediately above it. Validation: cargo clippy for dynamo-runtime and dynamo-llm with --no-default-features and warnings denied, cargo fmt, and all pre-commit hooks passed. Test suites were not run. |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
There was a problem hiding this comment.
lib/llm/src/grpc/service/openai.rs — The new router ErrorType::Unavailable falls through to Status::internal in the gRPC completions generate path, so no-worker requests return gRPC code 13 instead of retryable code 14. Fix: handle request_was_unavailable before the internal fallback and return Status::unavailable.
🤖 AI Fix
In lib/llm/src/grpc/service/openai.rs, inside completion_response_stream's engine.generate(request).await.map_err closure, add if crate::http::service::metrics::request_was_unavailable(e.as_ref()) { return Status::unavailable(e.to_string()); } immediately after the request_was_rejected branch and before Status::internal(...).
lib/llm/src/grpc/service/tensor.rs — The new router ErrorType::Unavailable falls through to Status::internal in the gRPC tensor generate path, so no-worker requests return gRPC code 13 instead of retryable code 14. Fix: handle request_was_unavailable before the internal fallback and return Status::unavailable.
🤖 AI Fix
In lib/llm/src/grpc/service/tensor.rs, inside tensor_response_stream's engine.generate(request).await.map_err closure, add if crate::http::service::metrics::request_was_unavailable(e.as_ref()) { return Status::unavailable(e.to_string()); } immediately after the request_was_rejected branch and before Status::internal(...).
| let error_type = if res_bytes.starts_with(OVERLOAD_PREFIX) { | ||
| ErrorType::ResourceExhausted | ||
| } else if res_bytes.starts_with(UNAVAILABLE_PREFIX) { | ||
| ErrorType::Unavailable |
There was a problem hiding this comment.
Mapping the closed worker-pool ACK to ErrorType::Unavailable means PushRouter::wrap_with_fault_detection no longer marks that selected worker overloaded or down, so the router can keep selecting a worker that has already reported its ingress channel closed. Fix: treat ErrorType::Unavailable from a selected worker as an inhibited/faulted instance and remove it from routing candidates.
🤖 AI Fix
In lib/runtime/src/pipeline/network/egress/push_router.rs, update is_inhibited to include ErrorType::Unavailable in the INHIBITED list, or add an equivalent branch in PushRouter::wrap_with_fault_detection before the ResourceExhausted branch that calls self.client.report_instance_down(instance_id) when match_error_chain(err.as_ref(), &[ErrorType::Unavailable], &[]) is true; add a unit test covering a "Server unavailable: worker pool channel closed" ACK causing the selected instance to be reported down.
PeaBrane
changed the title
PeaBrane
changed the title
This branch was previously deployed
2 participants
Overview:
Distinguish temporary capacity overload from genuine backend unavailability at the HTTP boundary.
Summary
Details:
Previously overload was mapped to 503 while the runtime no-worker path was untyped and could fall through to 500. Worker ACKs and the Anthropic pre-stream error path also collapsed overload and unavailable failures. This change introduces typed unavailable propagation, maps capacity rejection to 529, and records separate
overloadandunavailablemetric labels.Where should the reviewer start?
lib/llm/src/http/service/error.rsfor the HTTP status contract.lib/llm/src/http/service/openai.rsandlib/llm/src/http/service/anthropic.rsfor protocol handling.lib/runtime/src/pipeline/network/egress/push_router.rsandaddressed_router.rsfor typed routing errors.Validation
cargo clippy -p dynamo-runtime -p dynamo-llm --no-default-features -- -D warningscargo fmt --allRelated Issues
🚫 This PR is NOT linked to an issue: