fix: gRPC error handling — proper status codes and circuit breaker accuracy - #645
Conversation
Change GrpcClient::generate/embed to return tonic::Status instead of Box<dyn Error>, so callers can inspect the gRPC status code directly without downcasting. Circuit breaker now only counts server errors (Internal, Unavailable, Unknown, DataLoss, DeadlineExceeded) as failures. Client errors like InvalidArgument no longer trip the breaker since they indicate a healthy backend rejecting bad input. gRPC status codes are mapped to proper HTTP status in both stream creation (request_execution.rs) and stream iteration (utils.rs) instead of always returning 500. In-band error path marked as legacy. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSwitches internal error types to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPC_Client as GRPC Client
participant ExecStage as RequestExecutionStage
participant Backend
participant CircuitBreaker
Client->>GRPC_Client: call generate/embed
GRPC_Client->>ExecStage: dispatch request
ExecStage->>Backend: start prefill/decode or generate
Backend-->>ExecStage: stream responses or tonic::Status error
ExecStage->>CircuitBreaker: evaluate outcome (is_healthy / to_http_error)
ExecStage-->>GRPC_Client: return ProtoStream or tonic::Status
GRPC_Client-->>Client: stream or gRPC status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
grpc
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and accuracy of gRPC error handling and circuit breaker functionality within the model gateway. By preserving and correctly mapping gRPC status codes to appropriate HTTP responses, it provides more granular feedback to callers. Furthermore, the circuit breaker now intelligently differentiates between client-induced errors and genuine backend failures, preventing healthy workers from being incorrectly marked as unhealthy. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 315bc286f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves gRPC error handling by propagating tonic::Status throughout the request pipeline, which enables more accurate HTTP status code mapping and refines the circuit breaker logic to differentiate between client and server errors. The introduction of TonicStatusExt and TonicResultExt provides a clean and centralized way to manage this logic. The changes make the error handling more robust and the codebase cleaner. The suggestion to refine gRPC to HTTP status code mapping further aligns with handling external system errors gracefully and propagating them appropriately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
- Map Code::Cancelled to 400 Bad Request (client-initiated, not 500) - Map Code::Unimplemented to 501 Not Implemented (dedicated HTTP status) - Add Code::Unimplemented to is_server_error() to trip circuit breaker for workers on incompatible/older backends Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00f6c9ecc5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The previous change from is_success() to !is_server_error() fixed client errors (400) not tripping the circuit breaker, but also made 408 (Request Timeout) and 429 (Too Many Requests) count as healthy. These indicate backend degradation and should trip the circuit breaker. Using !is_retryable_status() aligns the circuit breaker with the retry predicate: 400 = healthy, 408/429/5xx = failure. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Replace is_server_error() with is_cb_failure() that composes http_status() with is_retryable_status(). This ensures the circuit breaker uses the same predicate for both HTTP and gRPC paths. Key change: ResourceExhausted (429) now correctly trips the circuit breaker. Previously it was treated as healthy despite indicating backend overload. Unimplemented (501) no longer trips CB since it is a permanent condition, not a transient failure. Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/tonic_ext.rs`:
- Around line 27-44: In the http_status method on tonic_ext.rs (function
http_status), explicitly handle the Code::Ok case instead of letting it fall
through to the catch-all; update the match to include an arm for Code::Ok that
either returns StatusCode::OK or triggers a debug/assertion (e.g.,
debug_assert!(false, "unexpected Code::Ok in error path") and return
INTERNAL_SERVER_ERROR) so that accidental Ok values are clearly signaled; ensure
the change references the Code::Ok variant in the match alongside the existing
arms like Code::InvalidArgument, Code::Unauthenticated, etc., to make the
behavior defensive and obvious.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3ad1628-86f6-44c0-ad20-c1fdc314af1f
📒 Files selected for processing (3)
model_gateway/src/routers/grpc/common/stages/request_execution.rsmodel_gateway/src/routers/grpc/tonic_ext.rsmodel_gateway/src/routers/http/router.rs
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48fff6d890
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
Description
Problem
Related to #639.
The gRPC request execution pipeline had two error handling deficiencies:
All gRPC errors mapped to HTTP 500.
GrpcClient::generate()returnedBox<dyn Error>, erasing thetonic::Statustype. Every downstream error handler callederror::internal_error(...), so a400 Bad Requestfrom the backend (e.g., invalid prompt) surfaced as500 Internal Server Errorto the caller.Circuit breaker had incorrect failure classification. The circuit breaker recorded
result.is_ok()— any gRPC failure (includingInvalidArgument,NotFound, etc.) counted as a backend failure, potentially marking a healthy worker as unhealthy. Meanwhile, retryable errors likeResourceExhausted(429) were not tripping the circuit breaker.Solution
Preserve the gRPC status code throughout the pipeline, then map it correctly at the HTTP boundary.
GrpcClient::generate()andGrpcClient::embed()return types fromResult<T, Box<dyn Error>>toResult<T, tonic::Status>, adding aninto_status()helper for the downcast.TonicStatusExt/TonicResultExtextension traits (zero-cost at runtime, monomorphized) that provide:http_status()— maps gRPC codes to HTTP codes (e.g.,InvalidArgument→400,ResourceExhausted→429,Unavailable→503,DeadlineExceeded→504)is_cb_failure()— composeshttp_status()withis_retryable_status()so the circuit breaker uses the same predicate for both HTTP and gRPC paths (408, 429, 500, 502, 503, 504)to_http_error()— creates an HTTP error response with the correct status codeis_healthy()— onResult<T, tonic::Status>, returnstruewhenOkor when the error is not a CB failureerror::internal_error(...)calls in request execution withe.to_http_error(...).result.is_ok()circuit breaker checks withresult.is_healthy().!is_retryable_status(status)instead of!status.is_server_error().grpc_to_http_statusfromutils.rsinto the trait (was previously a free function).Circuit Breaker Alignment
Both HTTP and gRPC paths now use the same predicate (
is_retryable_status) for circuit breaker decisions:Changes
client.rs: Changegenerate()/embed()return type toResult<T, tonic::Status>. Addinto_status()to downcast fromBox<dyn Error>.tonic_ext.rs(new):TonicStatusExtandTonicResultExtextension traits with gRPC→HTTP mapping, CB failure classification viais_retryable_status(), and error response construction.request_execution.rs: Replace allerror::internal_error(...)withe.to_http_error(...). Replaceresult.is_ok()withresult.is_healthy(). Use!e.is_cb_failure()for sequential PD error paths.http/router.rs: Use!is_retryable_status(status)for circuit breaker outcome instead of!status.is_server_error().utils.rs: Consolidate inlinegrpc_to_http_statusinto the new trait; updatecollect_stream_responsesto usee.to_http_error().Test Plan
cargo check -p smg— zero warningscargo clippy --all-targets --all-features -- -D warnings— cleancargo test -p smg— all tests passInvalidArgumentnow returns HTTP 400 (was 500); circuit breaker correctly trips on 408/429 but not on 400Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Bug Fixes
Refactor