refactor(worker): split worker.rs into focused modules#1096
refactor(worker): split worker.rs into focused modules#1096
Conversation
github-actions
bot
added
model-gateway
📝 WalkthroughWalkthroughA refactor splits the monolithic worker module into focused submodules (traits, body, metadata, health_checker, basic), moves many worker-related types and helpers into those modules, and updates import paths across the codebase. No runtime logic or public APIs were functionally changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| pub struct WorkerRoutingKeyLoad { | ||
| url: String, | ||
| pub(crate) active_routing_keys: dashmap::DashMap<String, usize>, |
There was a problem hiding this comment.
🟡 Nit: active_routing_keys was private before the split and is now widened to pub(crate) solely to satisfy test assertions in basic.rs. The tests (test_worker_routing_key_load_cleanup_on_zero, test_worker_routing_key_load_multiple_requests_same_key) use load.active_routing_keys.len(), but that's identical to the existing public load.value() method. Switching the tests to use value() would let this field stay private.
| pub(crate) active_routing_keys: dashmap::DashMap<String, usize>, | |
| active_routing_keys: dashmap::DashMap<String, usize>, |
![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: 4bcd6f3613
ℹ️ 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".
| @@ -1,36 +1,36 @@ | |||
| //! Worker domain — identity, registry, health, resilience, monitoring, service. | |||
|
|
|||
| pub mod basic; | |||
There was a problem hiding this comment.
This refactor removes the public worker submodule from smg::worker (the old pub mod worker; path), but downstream code in this repo still imports that API shape (for example bindings/golang/src/policy.rs uses smg::worker::worker::{RuntimeType, WorkerMetadata, WorkerRoutingKeyLoad}). As a result, building smg-golang (and workspace builds that include it) will fail with unresolved imports unless this commit also provides a compatibility shim or updates all downstream imports in the same change.
Useful? React with 👍 / 👎.
![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 refactors the worker module by decomposing the large worker.rs file into several focused modules: basic.rs, body.rs, health_checker.rs, metadata.rs, and traits.rs. This reorganization improves maintainability and code structure while updating all internal references to use the new module paths. I have no feedback to provide.
![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/worker/metadata.rs`:
- Around line 11-14: The struct WorkerRoutingKeyLoad exposes mutable state via
the pub(crate) field active_routing_keys which lets internal code mutate
counters without going through update_metrics(); make active_routing_keys
private (remove pub(crate)) and add narrow, controlled accessors on
WorkerRoutingKeyLoad such as get_count(&self, key: &str) -> Option<usize> and
increment/decrement/update methods (or a test-only accessor behind cfg(test))
that perform mutations and call update_metrics() so all changes are tracked
centrally; update any callers to use these new methods rather than accessing the
field directly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 002acb19-c59b-48f0-93f0-412bf9f97af1
📒 Files selected for processing (16)
model_gateway/src/routers/openai/realtime/rest.rsmodel_gateway/src/server.rsmodel_gateway/src/worker/basic.rsmodel_gateway/src/worker/body.rsmodel_gateway/src/worker/builder.rsmodel_gateway/src/worker/health_checker.rsmodel_gateway/src/worker/metadata.rsmodel_gateway/src/worker/mod.rsmodel_gateway/src/worker/registry.rsmodel_gateway/src/worker/service.rsmodel_gateway/src/worker/traits.rsmodel_gateway/src/workflow/steps/classify.rsmodel_gateway/src/workflow/steps/external/create_workers.rsmodel_gateway/src/workflow/steps/local/create_worker.rsmodel_gateway/src/workflow/steps/local/remove_from_worker_registry.rsmodel_gateway/src/workflow/steps/shared/register.rs
| pub struct WorkerRoutingKeyLoad { | ||
| url: String, | ||
| pub(crate) active_routing_keys: dashmap::DashMap<String, usize>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep active_routing_keys encapsulated to protect metric consistency.
Line 13 exposes mutable state as pub(crate), so internal callers can mutate counters without update_metrics(). Prefer private storage plus narrow accessor(s) (or test-only accessor) to avoid metric drift.
♻️ Proposed refactor
pub struct WorkerRoutingKeyLoad {
url: String,
- pub(crate) active_routing_keys: dashmap::DashMap<String, usize>,
+ active_routing_keys: dashmap::DashMap<String, usize>,
}
+
+#[cfg(test)]
+impl WorkerRoutingKeyLoad {
+ pub(crate) fn routing_key_count(&self, routing_key: &str) -> usize {
+ self.active_routing_keys
+ .get(routing_key)
+ .map(|v| *v)
+ .unwrap_or(0)
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/worker/metadata.rs` around lines 11 - 14, The struct
WorkerRoutingKeyLoad exposes mutable state via the pub(crate) field
active_routing_keys which lets internal code mutate counters without going
through update_metrics(); make active_routing_keys private (remove pub(crate))
and add narrow, controlled accessors on WorkerRoutingKeyLoad such as
get_count(&self, key: &str) -> Option<usize> and increment/decrement/update
methods (or a test-only accessor behind cfg(test)) that perform mutations and
call update_metrics() so all changes are tracked centrally; update any callers
to use these new methods rather than accessing the field directly.
Split the 1,834-line worker.rs monolith into five focused modules: What changed: - traits.rs (357 lines): Worker trait (40+ methods), ConnectionModeExt and WorkerTypeExt extension traits, worker_to_info() helper, and re-exports of ConnectionMode/RuntimeType/WorkerType - basic.rs (1220 lines): BasicWorker struct, Debug impl, Worker impl, constants (DEFAULT_BOOTSTRAP_PORT, MOONCAKE_CONNECTOR), WORKER_CLIENT static, and all 850 lines of tests (moved from worker.rs inline) - metadata.rs (138 lines): WorkerMetadata struct with model matching methods, WorkerRoutingKeyLoad per-key load tracking - body.rs (94 lines): WorkerLoadGuard RAII guard, AttachedBody response body wrapper - health_checker.rs (51 lines): HealthChecker handle with graceful shutdown (pub(crate)) - Deleted worker.rs, removed #[expect(clippy::module_inception)] - Updated mod.rs re-exports to route through new module paths - Updated 10 files with import path changes (worker::worker::X → X or worker::traits::X / worker::basic::X / worker::metadata::X) Why: worker.rs was a 1,834-line monolith containing the Worker trait, BasicWorker impl, HealthChecker, WorkerMetadata, WorkerRoutingKeyLoad, WorkerLoadGuard, AttachedBody, constants, and 850 lines of tests. This split is prerequisite for the worker module deep refactor — later PRs need to modify Worker trait, BasicWorker, and HealthChecker independently. How: Pure code extraction — no behavioral changes. Each section moved to its natural module. Tests moved to basic.rs with updated imports. active_routing_keys field widened from private to pub(crate) so tests in basic.rs can access WorkerRoutingKeyLoad internals. All 133 worker unit tests and 100 integration tests pass identically. Refs: worker-module-deep-refactor plan (PR 3) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
force-pushed
the
refactor/split-worker-rs
branch
from
4bcd6f3 to
16b80e6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
model_gateway/src/worker/metadata.rs (1)
11-14: 🛠️ Refactor suggestion | 🟠 MajorKeep
active_routing_keysprivate.Line 13 widens the mutation surface for routing-key counters. Any crate-internal caller can now bypass
increment()/decrement(), which also means bypassingupdate_metrics()and driftingworker_routing_keys_active. A narrow#[cfg(test)]accessor is safer than exposing theDashMapitself.♻️ Proposed refactor
pub struct WorkerRoutingKeyLoad { url: String, - pub(crate) active_routing_keys: dashmap::DashMap<String, usize>, + active_routing_keys: dashmap::DashMap<String, usize>, } + +#[cfg(test)] +impl WorkerRoutingKeyLoad { + pub(crate) fn routing_key_count(&self, routing_key: &str) -> usize { + self.active_routing_keys + .get(routing_key) + .map(|count| *count) + .unwrap_or(0) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/worker/metadata.rs` around lines 11 - 14, The field active_routing_keys on WorkerRoutingKeyLoad should be made private (remove pub(crate)) to prevent crate-internal mutation; keep and use the existing increment()/decrement() methods (and their call to update_metrics() / worker_routing_keys_active) as the sole mutation API, and add a narrow #[cfg(test)] accessor (e.g., a fn active_routing_keys_snapshot(&self) -> HashMap<String, usize> or similar) so tests can read the map without mutating it directly; update any tests to use that accessor instead of accessing the DashMap field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model_gateway/src/worker/metadata.rs`:
- Around line 11-14: The field active_routing_keys on WorkerRoutingKeyLoad
should be made private (remove pub(crate)) to prevent crate-internal mutation;
keep and use the existing increment()/decrement() methods (and their call to
update_metrics() / worker_routing_keys_active) as the sole mutation API, and add
a narrow #[cfg(test)] accessor (e.g., a fn active_routing_keys_snapshot(&self)
-> HashMap<String, usize> or similar) so tests can read the map without mutating
it directly; update any tests to use that accessor instead of accessing the
DashMap field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea020578-b138-48aa-9877-26ffd3496458
📒 Files selected for processing (16)
model_gateway/src/routers/openai/realtime/rest.rsmodel_gateway/src/server.rsmodel_gateway/src/worker/basic.rsmodel_gateway/src/worker/body.rsmodel_gateway/src/worker/builder.rsmodel_gateway/src/worker/health_checker.rsmodel_gateway/src/worker/metadata.rsmodel_gateway/src/worker/mod.rsmodel_gateway/src/worker/registry.rsmodel_gateway/src/worker/service.rsmodel_gateway/src/worker/traits.rsmodel_gateway/src/workflow/steps/classify.rsmodel_gateway/src/workflow/steps/external/create_workers.rsmodel_gateway/src/workflow/steps/local/create_worker.rsmodel_gateway/src/workflow/steps/local/remove_from_worker_registry.rsmodel_gateway/src/workflow/steps/shared/register.rs
|
Closing — the split was premature and the file names didn't communicate their contents. worker.rs stays as-is; any splits will happen organically when later PRs (WorkerManager extraction, status state machine) actually need the separation. |
1 participant
Description
Problem
worker.rsis a 1,834-line monolith containing the Worker trait (40+ methods), BasicWorker impl, HealthChecker, WorkerMetadata, WorkerRoutingKeyLoad, WorkerLoadGuard, AttachedBody, constants, and 850 lines of tests — all in a single file with a#[expect(clippy::module_inception)]suppression.Solution
Split into five focused modules with clear responsibilities. Pure code extraction — no behavioral changes.
Changes
traits.rsbasic.rsmetadata.rsbody.rshealth_checker.rsDeleted:
worker.rs(1,834 lines), removed#[expect(clippy::module_inception)]Updated imports in 10 files:
worker::worker::X→ direct re-exports orworker::traits::X/worker::basic::X/worker::metadata::XMinimal visibility change:
WorkerRoutingKeyLoad.active_routing_keyswidened from private topub(crate)so tests inbasic.rscan access it (no public API change).Test Plan
cargo check --package smg— clean, no warningscargo test --package smg --lib worker::— all 133 tests passcargo test --package smg --test api_tests— all 100 integration tests passRefs: worker-module-deep-refactor plan (PR 3)
Checklist
Summary by CodeRabbit