feat(core): wire per-worker resilience and HTTP client into BasicWorker#803
feat(core): wire per-worker resilience and HTTP client into BasicWorker#803CatherineSue merged 2 commits intomainfrom
Conversation
Add http_client and resilience fields to BasicWorker and the Worker trait. Workers now own their resolved resilience config and an isolated HTTP connection pool, constructed at registration time. - Add resilience(), http_client(), is_retryable() to Worker trait - Add http_client/resilience builder methods to BasicWorkerBuilder - Wire resolve_resilience() and build_worker_http_client() into local and external worker creation steps - Preserve resilience/http_client during worker property updates - Update GrpcWorker in golang bindings to implement new trait methods Signed-off-by: Chang Su <chang.s.su@oracle.com>
github-actions
bot
added
dependencies
📝 WalkthroughWalkthroughAdds per-worker resilience and an HTTP client across worker types: new Changes
Sequence Diagram(s)sequenceDiagram
participant Step as CreateWorkerStep
participant Resolver as resolve_resilience
participant HTTPBuilder as build_worker_http_client
participant Builder as BasicWorkerBuilder
participant Worker as BasicWorker
Step->>Resolver: compute resolved_resilience(base_retry, base_cb, flags)
Resolver-->>Step: ResolvedResilience + circuit_breaker_config
Step->>HTTPBuilder: build_worker_http_client(worker_id, settings)
HTTPBuilder-->>Step: reqwest::Client (or error)
Step->>Builder: builder.resilience(resolved_resilience).http_client(client)
Builder->>Worker: build()
Worker-->>Step: BasicWorker { http_client, resilience, ... }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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 integrates per-worker resilience configurations and isolated HTTP clients into the 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
|
![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 wires up per-worker resilience configurations and dedicated HTTP clients, which is a great step towards more granular control and isolation. The implementation is solid, covering creation and update paths for both local and external workers. I've found a couple of areas for improvement related to error handling to make the implementation more robust. Specifically, I've suggested using a more appropriate error type when HTTP client creation fails in one of the workflow steps, and making client creation failures in the builder explicit instead of silently falling back to defaults. Overall, these are great changes.
![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: e9f981bb86
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/steps/worker/external/create_workers.rs`:
- Around line 68-69: The call that constructs the HTTP client using
build_worker_http_client currently maps errors to
WorkflowError::ContextValueNotFound; change this to map to
WorkflowError::StepFailed with a clear message indicating HTTP client
construction failed and include the underlying error (e.g., via format! or
chain) so the original error is preserved — update the map_err on
build_worker_http_client(...) to return WorkflowError::StepFailed("failed to
build worker HTTP client: ...", Box::new(err)) or equivalent to match the enum
variant and existing error-wrapping pattern used elsewhere (see usages around
create_worker.rs functions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 389b3ca1-60de-42d4-8d5b-ca3d3873a496
📒 Files selected for processing (7)
bindings/golang/Cargo.tomlbindings/golang/src/policy.rsmodel_gateway/src/core/steps/worker/external/create_workers.rsmodel_gateway/src/core/steps/worker/local/create_worker.rsmodel_gateway/src/core/steps/worker/local/update_worker_properties.rsmodel_gateway/src/core/worker.rsmodel_gateway/src/core/worker_builder.rs
Use WorkflowError::StepFailed instead of ContextValueNotFound when build_worker_http_client fails in external worker creation. Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/core/steps/worker/external/create_workers.rs (1)
101-107: 🧹 Nitpick | 🔵 TrivialExtract the shared builder wiring before it drifts.
The wildcard and discovered-model branches now duplicate the same new
http_client/resilienceplumbing. Pulling the commonBasicWorkerBuildersetup into a helper or local closure would make future per-worker additions much harder to miss in one path.Also applies to: 141-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/core/steps/worker/external/create_workers.rs` around lines 101 - 107, The two branches duplicate the same BasicWorkerBuilder wiring (new(normalized_url.clone()) plus .worker_type(...), .connection_mode(...), .runtime_type(...), .circuit_breaker_config(...), .http_client(http_client.clone()), .resilience(resolved_resilience.clone())); extract that common setup into a small helper function or local closure (e.g., make_worker_builder or build_base_worker) that accepts normalized_url (or &str) and returns a configured BasicWorkerBuilder, then call that helper from both the wildcard and discovered-model branches before applying branch-specific modifications; update uses of BasicWorkerBuilder::new and the chained setters to use the helper to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/core/steps/worker/external/create_workers.rs`:
- Around line 101-107: The two branches duplicate the same BasicWorkerBuilder
wiring (new(normalized_url.clone()) plus .worker_type(...),
.connection_mode(...), .runtime_type(...), .circuit_breaker_config(...),
.http_client(http_client.clone()), .resilience(resolved_resilience.clone()));
extract that common setup into a small helper function or local closure (e.g.,
make_worker_builder or build_base_worker) that accepts normalized_url (or &str)
and returns a configured BasicWorkerBuilder, then call that helper from both the
wildcard and discovered-model branches before applying branch-specific
modifications; update uses of BasicWorkerBuilder::new and the chained setters to
use the helper to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1bacbaa-c5bf-4b4e-a90d-0b3817f42cc3
📒 Files selected for processing (1)
model_gateway/src/core/steps/worker/external/create_workers.rs
…er (lightseekorg#803) Signed-off-by: Chang Su <chang.s.su@oracle.com>
…er (lightseekorg#803) Signed-off-by: Chang Su <chang.s.su@oracle.com>
2 participants
Description
Problem
PR #799 added the foundational types (
ResolvedResilience,HttpPoolConfig,ResilienceUpdate,build_worker_http_client) but workers don't use them yet.BasicWorkerhas no per-worker HTTP client or resilience config — all workers still share the global client and router-level retry/CB settings.Solution
Wire the new types into
BasicWorker, theWorkertrait, and all worker creation paths so that every worker constructed via the registration API gets its own resolved resilience config and isolated HTTP connection pool at construction time.Changes
resilience(),http_client(),is_retryable()methods to theWorkertraithttp_clientandresiliencefields toBasicWorkerstructhttp_client()andresilience()builder methods toBasicWorkerBuilderresolve_resilience()andbuild_worker_http_client()into local worker creation (CreateLocalWorkerStep)CreateExternalWorkersStep)resilience/http_clientduring worker property updates (UpdateWorkerPropertiesStep)GrpcWorkerin golang bindings to implement new trait methods (addreqwestdep)Test Plan
cargo test -p smg --lib— all 435 tests pass, 0 failurescargo check -p smg-golang— golang bindings compile cleanlyresilience/http_poolinWorkerSpecget per-worker configChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Summary by CodeRabbit