[Bugfix]: Fix health checker to preserve worker load during checks by wuhang2014 · Pull Request #216 · vllm-project/router

wuhang2014 · 2026-08-16T08:23:08Z

Purpose

Fixes #197: the runtime health checker unconditionally reset every worker's load counter to zero every 10 health-check cycles, even while requests were in flight. The cache-aware prefix tree was never reset, so the policy saw all workers as balanced and kept routing by prefix affinity to already-saturated workers, creating a positive feedback loop of hot spots.

Changes:

Remove the periodic load reset from WorkerRegistry::start_health_checker(); health checks now run in parallel and never mutate load accounting. Load counters are neither reset periodically nor reset on health transitions: a failed /health probe does not cancel in-flight requests, so the load a worker reports at recovery time can still be real work.
Delete the unused standalone start_health_checker() with its conditional low-load reset (zero callers).
Redesign WorkerLoadGuard as an owning RAII guard with share()/release() and adopt it in the OpenAI router proxy path. This also fixes a pre-existing double-decrement on retryable responses and guarantees paired increment/decrement on every path (success, failure, retry, stream abort, client disconnect).
Adopt WorkerLoadGuard for both phases of the P/D two-stage flow (process_vllm_two_stage_request), fixing a permanent load leak when a P/D request task is cancelled (e.g. client disconnect) between the manual increment and a decrement branch. Cancellation now releases the prefill/decode load via guard drop; covered by cancellation tests for both phases.
Clamp decrement at zero with a warning and wire the vllm_router_worker_load gauge on increment/decrement/reset.

Changes since first review

Removed the unhealthy-to-healthy reset_load() call that the first review round flagged: it could erase real in-flight load (a failed /health probe does not cancel requests) and raced with new routing. The health checker now never mutates load accounting; reset_load is documented as an administrative operation that is only safe when the caller has established that no requests are in flight.
Strengthened the [Bug]: Periodic health checks reset active worker load counters and cause cache-aware hot spots #197 regression test: it now runs under paused Tokio time with a no-I/O health check and advances through 12 health-check cycles, so it fails against the pre-fix 10-cycle reset (verified by mutation probe: left: 0, right: 5).
Added P/D cancellation tests for both phases (see Test Plan); verified they fail against the previous manual accounting (left: 1, right: 0) and pass with the guard-based fix.

Test Plan

cargo fmt --check
cargo clippy --all-targets --all-features -- -D warnings
cargo test --lib --bins
cargo test --test '*'
pre-commit run --files src/core/mod.rs src/core/worker.rs src/core/worker_registry.rs \
  src/routers/http/router.rs src/routers/http/vllm_pd_router.rs Cargo.toml

New tests:

src/core/worker.rs: WorkerLoadGuard unit tests (drop decrements, release idempotence, shared handle decrements once, multi-worker, decrement-at-zero clamps).
src/core/worker_registry.rs: test_health_checker_preserves_inflight_load (regression test for [Bug]: Periodic health checks reset active worker load counters and cause cache-aware hot spots #197; runs under paused Tokio time with a no-I/O health check and advances through 12 cycles, so it fails against the pre-fix 10-cycle reset) and test_health_checker_preserves_load_across_recovery.
src/routers/http/vllm_pd_router.rs: test_pd_request_cancellation_releases_prefill_load and test_pd_request_cancellation_releases_decode_load — run the real two-stage flow against mock prefill/decode servers, abort the request while the upstream response is pending, and assert load returns to zero (verified to fail against the previous manual accounting).

Test Result

All Rust unit/integration tests pass: cargo test --lib --bins (494 passed), cargo test --test '*' (all suites passed), cargo fmt --check and CI-grade clippy clean, all pre-commit hooks pass.

End-to-end verification with 3 mock workers (requests held in flight, 1s health-check interval) comparing router-reported load vs actual in-flight requests:

Event	Before	After
10 requests in flight, 10 health-check cycles elapsed	`Resetting worker loads (cycle 10)`; router view `max_load=0, min_load=0`	No reset lines; `max_load=4, min_load=3` (≈ actual in-flight)
Probes while 13-15 requests in flight	Router view `max_load=0..1`, `is_imbalanced=false` -> prefix-affinity routing continues	`max_load=4..5` tracking actual in-flight; balancing decisions see true load

Mutation probes: restoring the old 10-cycle reset makes the regression test fail (left: 0, right: 5); restoring the manual P/D accounting makes both cancellation tests fail (left: 1, right: 0).

Essential Elements of an Effective PR Description Checklist

The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
The test plan, such as providing test command.
The test results, such as pasting the results comparison before and after, or e2e results

…llm-project#197) The registry health checker unconditionally reset every worker's load counter every 10 health-check cycles, even with requests in flight. Since the cache-aware prefix tree was not reset, the policy kept routing by prefix affinity to saturated workers, creating hot spots. - Remove the periodic load reset from WorkerRegistry::start_health_checker (health checks now run in parallel) - Reset load only when a worker transitions unhealthy -> healthy, where remaining load is drift from the down period (no new requests are routed to unhealthy workers) - Delete the unused standalone start_health_checker with its conditional low-load reset (zero callers) - Redesign WorkerLoadGuard as an owning RAII guard with share()/release() and adopt it in the OpenAI router proxy path, fixing a double-decrement on retryable responses and guaranteeing paired accounting on every path - Clamp decrement at zero with a warning and wire the vllm_router_worker_load gauge on increment/decrement/reset Adds unit tests for guard semantics and registry tests proving the health checker preserves in-flight load and resets only on recovery. Signed-off-by: WU Hang <whlbx@hotmail.com>

Copilot

Pull request overview

Fixes a load-accounting bug where periodic health checks reset worker load counters while requests were in flight, which could mislead cache-aware routing and create hot-spot feedback loops. The PR decouples health checking from load accounting and hardens load tracking via an owning RAII guard.

Changes:

Removes periodic load resets from the registry health checker; runs health checks in parallel and only resets load on unhealthy → healthy recovery.
Redesigns WorkerLoadGuard into an owning, shareable RAII guard to ensure exactly-once decrement across success/failure/retry/stream paths, and clamps load decrements at zero with metrics updates.
Updates the HTTP router streaming/non-streaming request paths to use WorkerLoadGuard instead of manual increment/decrement logic.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/routers/http/router.rs	Switches cache-aware load accounting to `WorkerLoadGuard`, including streaming lifecycle handling.
src/core/worker.rs	Implements new owning `WorkerLoadGuard`, clamps decrements at zero, and updates load metrics wiring.
src/core/worker_registry.rs	Removes periodic load resets; parallelizes health checks; resets load only on recovery; adds regression tests.
src/core/mod.rs	Removes re-export of deleted standalone `start_health_checker` and updates public exports accordingly.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7314e6214b

ℹ️ 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".

…ssion test Address review findings on the vllm-project#197 fix: P1: resetting the load counter on an unhealthy->healthy transition can erase real in-flight load. A failed /health probe does not cancel requests routed before the failure, and between set_healthy(true) and the reset a newly routed request's increment can be wiped, or an older guard's decrement can consume a newer request's count. The health checker now never mutates load accounting; WorkerLoadGuard keeps increments and decrements paired on every request path. P2: the regression test only observed ~3 health-check cycles and could not catch the previous 10-cycle periodic reset. It now runs under paused Tokio time with a no-I/O health check and advances through 12 cycles; verified to fail against the pre-fix implementation (load reset to 0) and pass with the fix. Adds a recovery test asserting load is preserved across an unhealthy->healthy transition. Adds tokio's test-util feature as a dev-dependency for paused-time testing. Signed-off-by: WU Hang <whlbx@hotmail.com>

The only usage is the fully-qualified futures::future::join_all, which resolves via the edition extern prelude, so the use statement is unnecessary. Signed-off-by: WU Hang <whlbx@hotmail.com>

Address follow-up review findings on the vllm-project#197 fix: P1: the P/D router tracked prefill and decode load with manual increments and decrements. Cancelling the request task (e.g. client disconnect dropping the handler future) between the increment and an explicit decrement branch leaked the load permanently, since health checks no longer mask it. Both phases now use WorkerLoadGuard: the prefill guard is released at the transition to decode, the decode guard at phase completion, and cancellation releases either guard via Drop. Adds cancellation tests that run the real two-stage request flow against mock prefill/decode servers, abort the request while the upstream response is pending, and assert load returns to zero for both phases. Verified these tests fail against the previous manual accounting (load left at 1) and pass with the fix. P3: reset_load is no longer called by the runtime; update the trait and BasicWorker docs to describe it as an administrative operation that is only safe when the caller has established that no requests are in flight. Signed-off-by: WU Hang <whlbx@hotmail.com>

hsliuustc0106 · 2026-08-16T22:30:52Z

@ywang96 @zhuohan123 PTAL

bvolpato

Thanks for covering both cancellation and periodic-reset paths. One blocking streaming retry case remains in send_typed_request: retryable response headers call guard.release() before the forwarding task finishes consuming the backend body. The retry can start while the previous attempt is still draining or stalled, but that active attempt is already counted as zero. Cache-aware routing can then select the same busy worker and recreate the undercounting feedback loop.

Could we keep the unreleased guard in the forwarding task for retryable responses too, releasing it only when that stream ends or is cancelled? A focused test with retryable headers plus a held response body would protect this branch. Everything else in the accounting and P/D cancellation changes looks good.

bvolpato · 2026-09-03T03:30:03Z

Could we combine receiver-closure handling from #215 and decode-body lifetime from #176 so cancellation also covers stalled bodies?

At 9e6fce2, the regular streaming task still waits only on stream.next() (router.rs:902). Dropping the client response does not wake it until another upstream chunk arrives, so a stalled stream retains the guard and worker connection. In the PD path, decode_guard.release() still runs at response headers (vllm_pd_router.rs:1471), before either buffering or forwarding the body.

A headers-then-stall mock would cover both cases: close the downstream response and expect prompt release; while it stays connected, keep decode load counted until EOF/drop. #200 overlaps this work and has the same remaining boundaries, so consolidating these pieces would avoid another competing fix.

…ill affinity The wheel build pinned the head of vllm-project/router#216, which fixes the decode-node hot spot (issue #197). Shipping #216 alone turns out to be worse than not shipping it for prefill-heavy benchmarks such as SWE-bench Pro. #216 makes the worker load counters honest, and that switches on a second latent bug: cache_aware decides whether to use prefix affinity from the fleet-wide load spread, so one hot worker discards affinity for every request -- including requests whose own worker is idle. Under P/D disaggregation that gate is open almost permanently, because prefill worker load counts queued requests as well as running ones. Routing degenerates to shortest-queue, already-cached prompts get recomputed, prefill saturates and decode starves behind it. Repin to the head of vllm-project/router#238, which carries #216 unchanged plus a fix applying the same load check per request, against the worker the request actually wants, rather than against the fleet maximum. Still fetched from vllm-project/router by bare SHA, as the #216 pin already was. Also in the wheel build: - VLLM_ROUTER_SRC builds from a local router checkout instead of the pinned SHA, for iterating on a fork. The directory is mounted read-only and copied into the build root so the build never writes into the source tree. - cargo check --all-targets. Neither the wheel build nor cargo test --lib compiles the vllm-router binary target, so a break in src/main.rs would ship silently. - Run the cache_aware policy tests alongside the load-accounting ones. Signed-off-by: Sugam Devare <sdevare@nvidia.com>

…ill affinity The wheel build pins the head of vllm-project/router#216, which fixes the decode-node hot spot (issue #197). Shipping #216 alone turns out to be worse than not shipping it for prefill-heavy benchmarks such as SWE-bench Pro. #216 makes the worker load counters honest, and that switches on a second latent bug: cache_aware decides whether to use prefix affinity from the fleet-wide load spread, so one hot worker discards affinity for every request -- including requests whose own worker is idle. Under P/D disaggregation that gate is open almost permanently, because prefill worker load counts queued requests as well as running ones. Routing degenerates to shortest-queue, already-cached prompts get recomputed, prefill saturates and decode starves behind it. Repin to the head of vllm-project/router#238, which carries #216 unchanged plus a fix applying the same load check per request, against the worker the request actually wants, rather than against the fleet maximum. Still fetched from vllm-project/router by bare SHA, exactly as the #216 pin already was. Signed-off-by: Sugam Devare <sdevare@nvidia.com>

… fixes (#3080) ## What does this PR do? Repins the `vllm-router` build used by the Super 3.5 eval container, from the head of [vllm-project/router#216](vllm-project/router#216) to the head of [#238](vllm-project/router#238). #216 fixes the decode-node hot spot (issue #197), but shipping it alone is worse than not shipping it for prefill-heavy benchmarks like SWE-bench Pro. #216 makes the worker load counters honest, and that switches on a second latent bug: `cache_aware` decides whether to use prefix affinity from the **fleet-wide** load spread, so one hot worker discards affinity for *every* request — including requests whose own worker is idle. Under P/D disaggregation that gate is open almost permanently, because prefill worker load counts queued requests as well as running ones. Routing degenerates to shortest-queue, already-cached prompts get recomputed, prefill saturates, and decode starves behind it. #238 carries #216 unchanged plus a fix that applies the same load check per request, against the worker the request actually wants, rather than against the fleet maximum. Still fetched from `vllm-project/router` by bare SHA, exactly as the #216 pin already was — a PR head is a ref in the upstream repo even when the branch lives on a contributor's fork. ## Testing Validated on SWE-bench Pro (4 prefill / 6 decode) against two baselines on the same checkpoint and vLLM config: the pre-#216 router, and #216 alone. With this pin the prefill prefix-cache hit rate and end-to-end throughput return to pre-#216 behaviour while keeping #216's decode hot-spot fix. Signed-off-by: Sugam Devare <sdevare@nvidia.com>

… fixes (NVIDIA-NeMo#3080) ## What does this PR do? Repins the `vllm-router` build used by the Super 3.5 eval container, from the head of [vllm-project/router#216](vllm-project/router#216) to the head of [NVIDIA-NeMo#238](vllm-project/router#238). NVIDIA-NeMo#216 fixes the decode-node hot spot (issue NVIDIA-NeMo#197), but shipping it alone is worse than not shipping it for prefill-heavy benchmarks like SWE-bench Pro. NVIDIA-NeMo#216 makes the worker load counters honest, and that switches on a second latent bug: `cache_aware` decides whether to use prefix affinity from the **fleet-wide** load spread, so one hot worker discards affinity for *every* request — including requests whose own worker is idle. Under P/D disaggregation that gate is open almost permanently, because prefill worker load counts queued requests as well as running ones. Routing degenerates to shortest-queue, already-cached prompts get recomputed, prefill saturates, and decode starves behind it. NVIDIA-NeMo#238 carries NVIDIA-NeMo#216 unchanged plus a fix that applies the same load check per request, against the worker the request actually wants, rather than against the fleet maximum. Still fetched from `vllm-project/router` by bare SHA, exactly as the NVIDIA-NeMo#216 pin already was — a PR head is a ref in the upstream repo even when the branch lives on a contributor's fork. ## Testing Validated on SWE-bench Pro (4 prefill / 6 decode) against two baselines on the same checkpoint and vLLM config: the pre-NVIDIA-NeMo#216 router, and NVIDIA-NeMo#216 alone. With this pin the prefill prefix-cache hit rate and end-to-end throughput return to pre-NVIDIA-NeMo#216 behaviour while keeping NVIDIA-NeMo#216's decode hot-spot fix. Signed-off-by: Sugam Devare <sdevare@nvidia.com> Signed-off-by: Oleg Sudakov <oleg.sudakov@outlook.com>

… fixes (#3080) ## What does this PR do? Repins the `vllm-router` build used by the Super 3.5 eval container, from the head of [vllm-project/router#216](vllm-project/router#216) to the head of [#238](vllm-project/router#238). #216 fixes the decode-node hot spot (issue #197), but shipping it alone is worse than not shipping it for prefill-heavy benchmarks like SWE-bench Pro. #216 makes the worker load counters honest, and that switches on a second latent bug: `cache_aware` decides whether to use prefix affinity from the **fleet-wide** load spread, so one hot worker discards affinity for *every* request — including requests whose own worker is idle. Under P/D disaggregation that gate is open almost permanently, because prefill worker load counts queued requests as well as running ones. Routing degenerates to shortest-queue, already-cached prompts get recomputed, prefill saturates, and decode starves behind it. #238 carries #216 unchanged plus a fix that applies the same load check per request, against the worker the request actually wants, rather than against the fleet maximum. Still fetched from `vllm-project/router` by bare SHA, exactly as the #216 pin already was — a PR head is a ref in the upstream repo even when the branch lives on a contributor's fork. ## Testing Validated on SWE-bench Pro (4 prefill / 6 decode) against two baselines on the same checkpoint and vLLM config: the pre-#216 router, and #216 alone. With this pin the prefill prefix-cache hit rate and end-to-end throughput return to pre-#216 behaviour while keeping #216's decode hot-spot fix. Signed-off-by: Sugam Devare <sdevare@nvidia.com> Signed-off-by: Junkeun Yi <jkyi@nvidia.com>

Copilot AI lite review requested due to automatic review settings August 16, 2026 08:23

Copilot started reviewing on behalf of wuhang2014 August 16, 2026 08:23 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Comment thread src/core/worker_registry.rs

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

Comment thread src/core/worker_registry.rs Outdated

wuhang2014 added 3 commits August 16, 2026 18:23

chore: remove redundant single-component futures import

ad83cec

The only usage is the fully-qualified futures::future::join_all, which resolves via the edition extern prelude, so the use statement is unnecessary. Signed-off-by: WU Hang <whlbx@hotmail.com>

wuhang2014 mentioned this pull request Aug 31, 2026

Fix vulnerable Python packages in router image #226

Closed

bvolpato suggested changes Sep 1, 2026

View reviewed changes

sdevare-nv mentioned this pull request Sep 4, 2026

feat: cache aware per request affinity #238

Open

sdevare-nv mentioned this pull request Sep 4, 2026

fix(nemotron_3.5_super): pin a vllm-router build with prefix affinity fixes NVIDIA-NeMo/Gym#3080

Merged

hsliuustc0106 added the bug Something isn't working label Sep 14, 2026

Navigation Menu

[Bugfix]: Fix health checker to preserve worker load during checks - #216

[Bugfix]: Fix health checker to preserve worker load during checks#216
wuhang2014 wants to merge 4 commits into
vllm-project:mainfrom
wuhang2014:fix/197-health-check-load-reset

wuhang2014 commented Aug 16, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

hsliuustc0106 commented Aug 16, 2026

Uh oh!

bvolpato left a comment

Uh oh!

bvolpato commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

wuhang2014 commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes since first review

Test Plan

Test Result

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

hsliuustc0106 commented Aug 16, 2026

Uh oh!

bvolpato left a comment

Choose a reason for hiding this comment

Uh oh!

bvolpato commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wuhang2014 commented Aug 16, 2026 •

edited

Loading