Skip to content

fix(miles): keep health checks in admission mode - #7

Open
TianyeGGBond wants to merge 1 commit into
rlops:zhenyu/m11-mvp-testfrom
TianyeGGBond:zhenyu/fix-router-health-admission
Open

fix(miles): keep health checks in admission mode#7
TianyeGGBond wants to merge 1 commit into
rlops:zhenyu/m11-mvp-testfrom
TianyeGGBond:zhenyu/fix-router-health-admission

Conversation

@TianyeGGBond

@TianyeGGBond TianyeGGBond commented May 30, 2026

Copy link
Copy Markdown

Context

This is a narrow follow-up to the M11 router admission path.

In admission mode, an empty enabled_workers set is a valid runtime state: the scheduler may have disabled/offloaded every registered worker, and generation requests should suspend until a worker is re-admitted.

Before this change, _health_check_loop used if self.enabled_workers to decide whether admission mode was active. That makes an empty enabled set ambiguous:

  • legacy/pre-admission mode: no enabled set has been declared yet, so probing the full registered worker set is correct;
  • admission/zero-active mode: all workers are intentionally disabled, so falling back to the full registered set incorrectly probes disabled workers.

That fallback can fight the sleep/offload lifecycle because disabled workers are parked and should not be health-probed until they are re-admitted.

Change

  • Use _admission_declared as the admission-mode switch in _health_check_loop.
  • In admission mode, probe only enabled, non-dead workers, even when the enabled set is empty.
  • Preserve legacy behavior before admission has ever been declared: probe registered workers minus dead workers.
  • Add a focused router admission test for the zero-active case, verifying a disabled worker is not health-probed.
  • Keep the production code intentionally small: no new helper and no extra feature-specific comments.

Validation

Passed:

python -m pytest tests/test_partial_sleep_wake.py::TestRouterAdmissionLifecycle -q
# 4 passed, 1 warning

Passed:

git diff --check origin/zhenyu/m11-mvp-test..HEAD

Also tried the whole file:

python -m pytest tests/test_partial_sleep_wake.py -q

Local result is limited by unrelated missing heavy dependencies in this Windows environment:

  • ModuleNotFoundError: No module named 'ray' for TestEngineInfoStateMachine
  • ModuleNotFoundError: No module named 'torch' for TestSchedulerPreemptClassification

@TianyeGGBond
TianyeGGBond force-pushed the zhenyu/fix-router-health-admission branch 2 times, most recently from 7fc79f1 to 56bf34a Compare May 30, 2026 03:44
@TianyeGGBond
TianyeGGBond force-pushed the zhenyu/fix-router-health-admission branch from 56bf34a to 4059f2a Compare May 30, 2026 03:51

@zhenyulincs zhenyulincs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified before approving:

  • The ambiguity is real and the new predicate is the established one. The health loop was the last remaining site keying "admission active?" off if self.enabled_workers: (set truthiness), which cannot distinguish "admission never declared" (legacy — probe the full registry) from "admission declared, scheduler disabled every worker" (the legitimate F3 zero-active suspension — probe nothing). _admission_declared is exactly the flag the other two sites already use (miles_admission_disabled classification and _candidate_set()), set by all four lifecycle internals, never set on legacy paths — so legacy behavior is preserved verbatim.
  • Provenance check: the replaced conditional was introduced by the port itself (iter-6 legacy-compat commit f1f1e2f); iter 7 (1978e31) introduced _admission_declared and converted the other call sites but missed this one. This PR completes that conversion — no upstream-original miles code is modified, the upstream health-loop body is untouched.
  • Blast radius confirmed: on the buggy path, parked/offloaded workers get probed and can be marked dead, causing _notify_workers_changed churn and false "Marking as DEAD" logs; _enable_worker_internal clears dead + failure counts, so it self-heals on re-admission — state pollution and noise rather than permanent quarantine. Which is also why runtime smokes never surfaced it.
  • Ran the test file on this head: the TestRouterAdmissionLifecycle class passes 4/4 including the new zero-active probe test (the other classes in the file fail only on missing torch/numpy in my slim env — unrelated to this change).

One non-blocking simplification for a follow-up (not this PR): the new inline branch duplicates _candidate_set() exactly (declared → enabled - dead, else → registry - dead), and _candidate_set's docstring already claims the health loop uses it. urls = list(self._candidate_set()) would remove the third copy of this set arithmetic and make that docstring true — and would have prevented exactly this kind of missed-conversion bug.

@JunzheJoe JunzheJoe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants