Repository navigation
feat(core): add execute_with_resilience as primary resilience API - #811
CatherineSue wants to merge 2 commits into
Conversation
Add execute_with_resilience() function that encapsulates retry loop, circuit breaker checks, and outcome recording using the worker's own resolved config. This is the single entry point routers will use instead of calling RetryExecutor directly. - Checks circuit breaker before first attempt (when CB enabled) - Skips retry loop when retries are disabled (single attempt) - Uses worker's is_retryable() for retry decisions - Records backoff metrics via Metrics::record_worker_retry_backoff - 5 new async tests covering success, CB open, retries disabled, retry on retryable status, and CB disabled bypass Signed-off-by: Chang Su <chang.s.su@oracle.com>
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 introduces a unified resilience API, Highlights
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. Footnotes
|
📝 WalkthroughWalkthroughAdds and publicly re-exports a new async function Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant EWR as execute_with_resilience
participant CB as CircuitBreaker
participant Worker as Worker
participant Retry as RetryExecutor
participant Metrics as Metrics/Logger
Caller->>EWR: execute_with_resilience(worker, operation)
EWR->>CB: check state
alt circuit open
CB-->>EWR: open
EWR->>Metrics: record rejection
EWR-->>Caller: 503 response
else circuit closed/half-open
alt retries disabled
EWR->>Worker: execute_single(operation)
Worker-->>EWR: response
EWR->>CB: record outcome
EWR-->>Caller: response
else retries enabled
EWR->>Retry: start retry loop
loop retry attempts
Retry->>Worker: execute(operation)
Worker-->>Retry: response
Retry->>Metrics: log attempt/backoff
alt retryable
Retry->>Retry: backoff & retry
else final
Retry-->>EWR: final response
end
end
EWR->>CB: record final outcome
EWR-->>Caller: response
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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: 18b23114f7
ℹ️ 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 introduces a new execute_with_resilience function to centralize retry and circuit breaker logic. While this is a valuable addition, the current implementation has a critical flaw: it fails to record circuit breaker outcomes when retries are enabled, which undermines the circuit breaker's effectiveness. I've provided a detailed comment with a suggested fix that addresses this by adjusting the function signature to correctly handle lifetimes and ensure outcomes are recorded on every attempt. I've also suggested a minor cleanup to remove a now-redundant helper function.
![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
🤖 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/core/resilience.rs`:
- Around line 141-145: The response currently leaks the internal worker_url by
returning format!("Circuit breaker open for worker {worker_url}") as the 503
body; instead, return a generic message (e.g., "Service unavailable") for the
StatusCode::SERVICE_UNAVAILABLE response and move the worker_url detail into a
debug! or error! log call so the internal address is only logged (keep the
existing debug! usage). Update the code that constructs the response (the block
invoking StatusCode::SERVICE_UNAVAILABLE and format! for the circuit breaker
case in resilience.rs) to use a generic body while preserving the worker_url in
logging.
- Around line 159-176: The retry-enabled branch currently awaits
RetryExecutor::execute_response_with_retry(...) and returns its result without
invoking the circuit-breaker outcome recorder; capture the returned result from
RetryExecutor::execute_response_with_retry into a variable (e.g., let result =
... .await), call the same record_outcome(...) function used in the non-retry
branch with that result (matching the breaker and outcome signature used
elsewhere), and then return the captured result; ensure you still perform
Metrics::record_worker_retry_backoff and the debug logs as before and that you
use the same operation/resilience/retry and worker.is_retryable arguments to
preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba851e8b-4dbe-4a29-95d9-84b70182ae38
📒 Files selected for processing (2)
model_gateway/src/core/mod.rsmodel_gateway/src/core/resilience.rs
- Record circuit breaker outcome after retry loop completes, not just in the no-retry path — fixes bug where CB never learned from retried requests - Replace internal worker URL in 503 body with generic message to avoid leaking backend addresses to clients - Inline execute_single helper (was just operation(0).await) 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: c320e017e0
ℹ️ 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".
| pub async fn execute_with_resilience<F, Fut>( | ||
| worker: &(dyn Worker + '_), | ||
| mut operation: F, |
There was a problem hiding this comment.
I checked model_gateway/src/routers/http/router.rs:216-221 and :281-289: each retry currently re-enters select_worker_for_model, and that selector filters w.is_available() at :152-155. This helper bakes a single Worker into the retry loop, so any router migration will retry the same backend instead of failing over to another healthy worker. In multi-worker pools, a transient 429/503 from one instance will become a user-visible failure even when capacity is still available elsewhere.
Useful? React with 👍 / 👎.
| let response = RetryExecutor::execute_response_with_retry( | ||
| &resilience.retry, | ||
| operation, | ||
| |res, _attempt| worker.is_retryable(res), |
There was a problem hiding this comment.
This path calls can_execute() only once before handing control to RetryExecutor. If another request opens the same worker's breaker while this call is sleeping in backoff, the next attempt still invokes operation on that worker because there is no per-attempt gate here. The current HTTP flow avoids that by selecting an available worker on every attempt (model_gateway/src/routers/http/router.rs:152-155, :216-221, :281-289), so migrating to this helper would let concurrent failure bursts keep hammering a worker that the breaker has already quarantined.
Useful? React with 👍 / 👎.
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/core/resilience.rs`:
- Around line 242-274: Add an assertion after the existing response checks in
the test that verifies the circuit-breaker state/outcome was updated by the
retry-enabled path: after calling execute_with_resilience(&worker, ...) and
asserting StatusCode::OK and call_count, query the worker's resilience/breaker
(the breaker instance configured via ResolvedResilience/RetryConfig on
BasicWorkerBuilder::resilience) using the actual breaker API in the codebase
(e.g., the breaker state or last outcome accessor) and assert it reflects a
successful outcome (closed/success or equivalent enum variant). Ensure you
reference the same worker variable and use the breaker state accessor
implemented on the worker/resilience types so the test fails if breaker state is
not updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 686882ef-6f61-427c-8b6f-6123897f57f6
📒 Files selected for processing (1)
model_gateway/src/core/resilience.rs
| #[tokio::test] | ||
| async fn test_execute_with_resilience_retries_on_retryable_status() { | ||
| let resolved = ResolvedResilience { | ||
| retry: RetryConfig { | ||
| max_retries: 3, | ||
| initial_backoff_ms: 1, | ||
| max_backoff_ms: 2, | ||
| backoff_multiplier: 1.0, | ||
| jitter_factor: 0.0, | ||
| }, | ||
| ..Default::default() | ||
| }; | ||
| let worker = BasicWorkerBuilder::new("http://test:8080") | ||
| .resilience(resolved) | ||
| .build(); | ||
|
|
||
| let call_count = Arc::new(AtomicU32::new(0)); | ||
| let cc = call_count.clone(); | ||
| let response = execute_with_resilience(&worker, move |_attempt| { | ||
| let count = cc.fetch_add(1, Ordering::Relaxed); | ||
| async move { | ||
| if count < 2 { | ||
| (StatusCode::SERVICE_UNAVAILABLE, "fail").into_response() | ||
| } else { | ||
| (StatusCode::OK, "ok").into_response() | ||
| } | ||
| } | ||
| }) | ||
| .await; | ||
|
|
||
| assert_eq!(response.status(), StatusCode::OK); | ||
| assert_eq!(call_count.load(Ordering::Relaxed), 3); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression test that asserts breaker state updates in the retry-enabled path.
Current tests validate status and retry counts, but there is no explicit assertion that breaker outcome/state changes after the retry loop. A focused assertion here would lock in this fix against regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/core/resilience.rs` around lines 242 - 274, Add an
assertion after the existing response checks in the test that verifies the
circuit-breaker state/outcome was updated by the retry-enabled path: after
calling execute_with_resilience(&worker, ...) and asserting StatusCode::OK and
call_count, query the worker's resilience/breaker (the breaker instance
configured via ResolvedResilience/RetryConfig on BasicWorkerBuilder::resilience)
using the actual breaker API in the codebase (e.g., the breaker state or last
outcome accessor) and assert it reflects a successful outcome (closed/success or
equivalent enum variant). Ensure you reference the same worker variable and use
the breaker state accessor implemented on the worker/resilience types so the
test fails if breaker state is not updated.
|
Closing — the per-worker retry approach is wrong. Retries should re-select workers (like Envoy), not retry the same backend. The existing router retry loops already do this correctly. Per-worker improvements (http_client, is_retryable, resilience config) will be adopted directly within the existing router retry loops in subsequent PRs. |
1 participant
Description
Part of the per-worker resilience refactor series: #799 → #803 → this PR → router migration → cleanup.
Problem
Routers currently call
RetryExecutor::execute_response_with_retry()directly, each duplicating the same retry/CB boilerplate — selecting retry config, wiringshould_retry, callingrecord_outcome, etc. There's no single entry point that uses the worker's own resolved resilience config.Solution
Add
execute_with_resilience()as the primary resilience API. It encapsulates the full retry + circuit breaker flow using the worker's ownResolvedResilienceconfig:RetryExecutorwith the worker's retry config andis_retryable()predicateRouters will migrate to this in subsequent PRs.
Changes
execute_with_resilience()function inmodel_gateway/src/core/resilience.rsmodel_gateway/src/core/mod.rsTest Plan
cargo test -p smg --lib core::resilience— all 12 tests pass (7 existing + 5 new)cargo test -p smg --lib— all 440 tests pass, 0 failuresChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Summary by CodeRabbit