Repository navigation
refactor(worker): delete set_healthy and migrate every caller (PR 9) - #1121
Conversation
Removes the legacy `Worker::set_healthy(bool)` compatibility shim and
migrates every call site in the workspace — runtime, tests, benches,
and the Go binding FFI — to `set_status(WorkerStatus)`. We control
every caller, so deprecation buys nothing over deletion: the shim's
behavioral guards are inlined where they actually matter, and dead
code goes away instead of lingering with a `#[deprecated]` tag.
Runtime call sites (semantics preserved):
- `model_gateway/src/worker/registry.rs`:
* `remove()` (~line 1044) — only demotes Ready → NotReady before
tearing down metrics, mirroring the legacy
`set_healthy(false)` no-op-on-Pending behavior.
* `WorkerStateSubscriber::on_remote_worker_state` (existing-worker
update) — `state.health=true` always promotes to Ready; `false`
only demotes when currently Ready. Pending / NotReady / Failed
stay where they are because the local state machine owns them.
The "no event on update" invariant is unchanged and still
covered by `test_mesh_worker_state_update_is_silent_on_existing_worker`.
* `WorkerStateSubscriber::on_remote_worker_state` (newly built
worker pre-registration) — `state.health=true` promotes to
Ready; `false` leaves the builder default (Pending) alone, same
as the legacy shim.
- `model_gateway/src/workflow/steps/shared/activate.rs`:
`ActivateWorkersStep` now flips non-Ready workers to Ready via
`set_status(WorkerStatus::Ready)` directly.
- `bindings/golang/src/policy.rs`:
`sgl_multi_client_set_worker_health` maps the FFI bool straight to
`WorkerStatus::Ready` / `WorkerStatus::NotReady`. FFI workers are
managed by the Go SDK and never run the local state machine, so
the legacy "only demote from Ready" guard does not apply here —
the boolean is authoritative and a Pending FFI worker can be
flipped straight to NotReady. The accompanying doc comment is
refreshed to point at `set_status` instead of the deleted shim.
Test sites (mechanical 1:1 mapping):
- All policy tests (`cache_aware`, `consistent_hashing`, `manual`,
`mod`, `prefix_hash`, `random`, `round_robin`) — `set_healthy(false)`
→ `set_status(WorkerStatus::NotReady)`, `set_healthy(true)` →
`set_status(WorkerStatus::Ready)`. Each affected test module gets
`WorkerStatus` added to its existing `openai_protocol::worker`
import.
- `model_gateway/src/worker/worker.rs`:
* `test_health_status` — migrated to `set_status` calls.
* `test_pending_worker_not_routable` — the compat-shim assertions
(Pending no-op + Pending → Ready promotion) are removed; the
state-machine semantics they covered are exercised by the
WorkerManager test suite, and the test's stated purpose is
"Pending is not routable", which the surviving assertions
still verify.
* `test_concurrent_health_updates` — alternating bool replaced
with an explicit `Ready` / `NotReady` ternary.
* `test_dp_aware_worker_delegated_methods` — direct `set_status`
call.
- `model_gateway/src/routers/http/router.rs` /
`model_gateway/src/routers/http/pd_router.rs` — test helpers
rewritten to call `set_status`.
- `model_gateway/benches/manual_policy_benchmark.rs` — bench setup
uses `set_status`.
Observability:
- `model_gateway/src/observability/metrics_ws/collectors.rs` —
doc comments that mentioned the bypass-the-broadcast `set_healthy()`
callers now point at the `set_status()` callers instead (FFI
bindings, registry teardown, mesh subscriber).
Trait change:
- `model_gateway/src/worker/worker.rs` — `Worker::set_healthy` (default
trait method) is deleted entirely. `is_healthy()` stays as the
routing predicate (`status() == Ready`). No `Worker` impl
overrode `set_healthy`, so deletion has no implementor fan-out.
Test plan:
- cargo check --workspace: clean (smg + smg-golang + smg-python all
build with no `set_healthy` references)
- cargo clippy -p smg --lib --tests -- -D warnings: clean
- cargo clippy -p smg --benches -- -D warnings: clean
- cargo clippy -p smg-golang --lib -- -D warnings: clean
- cargo fmt -p smg -- --check: clean
- cargo test -p smg --lib: 538 passed, 0 failed, 4 ignored
- cargo test -p smg --tests: 16 integration binaries, 468 tests, 0
failed
- ripgrep for `set_healthy(` across the workspace: zero hits, only
doc comments referring to the legacy name in migration notes
remain
Refs: .claude/plans/2026-04-09-worker-module-deep-refactor.md (PR 9)
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
benchmarks
📝 WalkthroughWalkthroughThis PR removes the deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
![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 removes the set_healthy compatibility shim from the Worker trait, replacing it with explicit set_status calls using WorkerStatus throughout the codebase, including FFI bindings, load-balancing policies, and the worker registry. Feedback was provided regarding the on_remote_worker_state implementation in the worker registry, specifically concerning the risk of stale updates if worker IDs are not verified and redundant status updates when a worker is already in the desired state.
| if let Some(existing) = self.get_by_url(&state.url) { | ||
| existing.set_healthy(state.health); | ||
| if state.health { | ||
| existing.set_status(WorkerStatus::Ready); | ||
| } else if existing.status() == WorkerStatus::Ready { | ||
| existing.set_status(WorkerStatus::NotReady); | ||
| } |
There was a problem hiding this comment.
This block contains issues related to concurrency and state consistency:
- Stale Updates (ID Mismatch): The code updates the status of whatever worker is currently at
state.url. If a worker was removed and a new one registered at the same URL (getting a newWorkerId), a stale mesh update for the oldworker_idwill incorrectly be applied to the new worker. You should verify thatstate.worker_idmatches the current ID in the registry. - Redundant Metric Updates: If
state.healthistrueand the worker is alreadyReady,set_statusis called unconditionally, triggering redundant atomic stores and Prometheus metric updates.
Note: The TOCTOU race condition previously flagged here is considered acceptable for asynchronous background operations in this repository, as long as the resulting state remains valid.
References
- For asynchronous operations, a best-effort pre-check with a known TOCTOU race condition is acceptable if it improves user experience and maintains a valid state.
![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: 86322114a9
ℹ️ 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".
| if state.health { | ||
| worker.set_status(WorkerStatus::Ready); | ||
| } |
There was a problem hiding this comment.
This new branch drops the health=false transition entirely for newly built mesh workers. When state.spec is present, BasicWorkerBuilder::from_spec(spec).build() can start a worker in Ready (e.g., disable_health_check=true), and with this code that worker remains routable even though the incoming mesh state explicitly says unhealthy; the previous set_healthy(state.health) path would demote Ready to NotReady in this case. That can cause a freshly imported unhealthy worker to receive traffic until another local transition happens.
Useful? React with 👍 / 👎.
![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 `@bindings/golang/src/policy.rs`:
- Around line 148-149: The comment about FFI workers starting in Pending is
inconsistent with GrpcWorker::new() which currently initializes status to
WorkerStatus::Ready; update the code to make the contract explicit by changing
GrpcWorker::new() to initialize status to WorkerStatus::Pending (so FFI callers
must explicitly set_status via sgl_multi_client_set_worker_health), and
simultaneously update the surrounding comment text that references the FFI
lifecycle (the comment mentioning sgl_multi_client_set_worker_health) to reflect
the Pending-by-default behavior; ensure you adjust any tests or usages that
assume Ready-on-construction to account for the new initial state.
🪄 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: 334b84c2-aec0-4e7e-972c-cdec7ac0a078
📒 Files selected for processing (15)
bindings/golang/src/policy.rsmodel_gateway/benches/manual_policy_benchmark.rsmodel_gateway/src/observability/metrics_ws/collectors.rsmodel_gateway/src/policies/cache_aware.rsmodel_gateway/src/policies/consistent_hashing.rsmodel_gateway/src/policies/manual.rsmodel_gateway/src/policies/mod.rsmodel_gateway/src/policies/prefix_hash.rsmodel_gateway/src/policies/random.rsmodel_gateway/src/policies/round_robin.rsmodel_gateway/src/routers/http/pd_router.rsmodel_gateway/src/routers/http/router.rsmodel_gateway/src/worker/registry.rsmodel_gateway/src/worker/worker.rsmodel_gateway/src/workflow/steps/shared/activate.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b324aabb7
ℹ️ 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".
| if state.health { | ||
| worker.set_status(WorkerStatus::Ready); | ||
| } |
There was a problem hiding this comment.
The new-worker path in on_remote_worker_state only applies a transition when state.health is true, so an incoming unhealthy state can still register as routable if the builder created the worker as Ready (for example, BasicWorkerBuilder::from_spec(...).build() starts Ready when disable_health_check=true). Previously set_healthy(false) would demote Ready -> NotReady; now health=false is a no-op here, which can send traffic to workers the mesh explicitly marked unhealthy.
Useful? React with 👍 / 👎.
1 participant
Summary
Removes the legacy `Worker::set_healthy(bool)` compatibility shim and migrates every call site in the workspace — runtime, tests, benches, and the Go binding FFI — to `set_status(WorkerStatus)`. We control every caller, so deprecation buys nothing over deletion: the shim's behavioral guards are inlined where they actually matter, and the dead code goes away instead of lingering with a `#[deprecated]` tag.
This is PR 9 of the worker module deep refactor (tracked in `.claude/plans/2026-04-09-worker-module-deep-refactor.md`). Lands on top of #1118 (PR 8 — event-driven WorkerMonitor).
What changed
Runtime call sites (semantics preserved)
Test sites (mechanical 1:1 mapping)
Observability
Trait change
Why
The plan originally called for `#[deprecated]` on `set_healthy` plus migration of remaining callers. After staring at the migrated diff: every caller is in this repo, every caller has been migrated, and the Go SDK FFI is also under our control. A deprecation tag would just be dead code with a soft warning that nobody benefits from. Deletion is cleaner: smaller surface area, no shim semantics to keep in sync with the underlying state machine, no future regressions where someone reaches for `set_healthy` and reintroduces the "only demote from Ready" guard in a context where that guard is wrong.
The behavioral guards the legacy shim provided are still honored at the sites where they actually matter (registry teardown, mesh subscriber existing-worker update, mesh subscriber newly-built worker). The sites where the guard never made sense (Go SDK FFI, tests with `disable_health_check` workers) drop it.
Test plan
Checklist
Summary by CodeRabbit
Release Notes
Refactor
Tests