Skip to content

fix(health): skip canary health check for trtllm/vllm disagg decode workers - #8215

Closed
nnshah1 wants to merge 13 commits into
mainfrom
neelays/dis-1737-bug-canary-health-check-incompatible-with-disagg-decode
Closed

fix(health): skip canary health check for trtllm/vllm disagg decode workers#8215
nnshah1 wants to merge 13 commits into
mainfrom
neelays/dis-1737-bug-canary-health-check-incompatible-with-disagg-decode

Conversation

@nnshah1

@nnshah1 nnshah1 commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • TRT-LLM: Skip canary health check payload registration for disagg decode workers
  • vLLM: Skip canary health check payload for multimodal disagg decode workers (text-only decode still gets canary)
  • SGLang: Already correct (uses FAKE_BOOTSTRAP_HOST for disagg decode health checks)

Motivation

Enabling DYN_HEALTH_CHECK_ENABLED=true on disagg decode workers causes a crash-loop (DIS-1737). The canary sends a standard inference request, but decode workers reject it with "Disaggregated params are required for decode mode." The canary has no disagg topology awareness.

SGLang solved this with FAKE_BOOTSTRAP_HOST (fake-transfer mode). TRT-LLM and vLLM have no equivalent — TRT-LLM decode requires real opaque_state from prefill, vLLM multimodal decode needs embedding_params.

This PR fixes what we can now. TRT-LLM decode fake-transfer mode is being discussed with the TRT-LLM team separately.

Changes

  • trtllm/workers/llm_worker.py: Gate health_check_payload on disaggregation_mode != DECODE
  • vllm/worker_factory.py: Gate on not (DECODE and MULTIMODAL)
  • trtllm/tests/test_health_check_disagg.py: Test verifying standard payload has no disagg params

Test plan

  • TRT-LLM decode: no health check payload registered (no crash)
  • TRT-LLM prefill/agg: standard health check payload (unchanged)
  • vLLM multimodal decode: no health check payload (no crash)
  • vLLM text-only decode: standard health check payload (unchanged)
  • SGLang: validated existing disagg path is correct
  • e2e validation with real backends in k8s

Related

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests

    • Added validation test for health check payload structure in specific scenarios
  • Improvements

    • Optimized health check behavior for decode workers in disaggregation mode, conditionally skipping canary checks based on model type and runtime configuration

Open with Devin

@nnshah1
nnshah1 requested review from a team as code owners April 15, 2026 04:13
@github-actions github-actions Bot added fix backend::vllm Relates to the vllm backend backend::trtllm Relates to the trtllm backend labels Apr 15, 2026
@coderabbitai

coderabbitai Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The changes implement conditional health check payload handling for decode workers across TrtLLM and vLLM frameworks. When disaggregation mode is DECODE, health check payloads are conditionally skipped (and logged). An additional condition for vLLM checks for MULTIMODAL model type. A test validates the health check payload structure excludes certain fields.

Changes

Cohort / File(s) Summary
Health Check Payload Tests
components/src/dynamo/trtllm/tests/test_health_check_disagg.py
Added test for TrtllmHealthCheckPayload verifying that converted payload excludes "disaggregated_params" and "prefill_result" while including "token_ids".
TrtLLM Worker
components/src/dynamo/trtllm/workers/llm_worker.py
Conditionally sets health_check_payload to None during init_llm_worker when config.disaggregation_mode == DisaggregationMode.DECODE, with logging; otherwise constructs TrtllmHealthCheckPayload(tokenizer=tokenizer).to_dict(). Updated payload is passed to endpoint.serve_endpoint(...).
vLLM Worker Factory
components/src/dynamo/vllm/worker_factory.py
Conditionally sets health_check_payload to None for decode workers when both config.disaggregation_mode == DECODE and model_type == MULTIMODAL, with logging; otherwise computes payload via VllmHealthCheckPayload(...).to_dict(). Updated payload is passed to generate_endpoint.serve_endpoint(...).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title accurately and concisely summarizes the primary change: skipping canary health checks for disaggregated decode workers in both trtllm and vllm frameworks.
Description check ✅ Passed The pull request description provides comprehensive details covering overview, motivation, changes, and test plan, with clear section structure matching the template requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]

This comment was marked as resolved.

@nnshah1 nnshah1 changed the title fix(health): skip canary health check for disagg decode workers fix(health): skip canary health check for trtllm/vllm disagg decode workers Apr 16, 2026
@nnshah1
nnshah1 force-pushed the neelays/dis-1737-bug-canary-health-check-incompatible-with-disagg-decode branch from b4addf9 to 2295069 Compare April 16, 2026 22:20
@nnshah1
nnshah1 requested a review from a team April 16, 2026 22:20
@nnshah1
nnshah1 changed the base branch from main to neelays/dis-1185-race-condition-for-canary-health-check April 16, 2026 22:20
Base automatically changed from neelays/dis-1185-race-condition-for-canary-health-check to main April 17, 2026 13:44
@nnshah1
nnshah1 force-pushed the neelays/dis-1737-bug-canary-health-check-incompatible-with-disagg-decode branch from 2295069 to 4e566ac Compare April 17, 2026 13:59
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Apr 17, 2026
@nnshah1
nnshah1 force-pushed the neelays/dis-1737-bug-canary-health-check-incompatible-with-disagg-decode branch from 4e566ac to abe3eed Compare April 21, 2026 17:09
…points

Revises the initial DIS-1737 fix. The previous approach set
`health_check_payload=None` for disagg decode workers in both vllm and
trtllm. After DIS-1185 made canary the sole readiness authority, that
branch silently trapped decode workers in NotReady: `set_endpoint_registered`
no-ops when canary is enabled, and with no registered target, nothing ever
flips the endpoint to Ready. CI test
`test_deployment[disaggregated_same_gpu-2]` timed out at 432s as a result.

This change:

- vllm decode: always register `VllmHealthCheckPayload`. vllm's
  `DecodeWorkerHandler._generate_token_mode` already handles a request with
  no `prefill_result` by running it agg-style (kv_params / embedding_params
  default to None), so a standard canary probe produces one token regardless
  of disagg mode. Decode now gets real engine-level canary.

- trtllm decode: extract the "no canary" decision into
  `build_worker_health_check_payload` in `trtllm/health_check.py`. Decode
  still returns None because the handler strictly rejects requests without
  `disaggregated_params` ("Disaggregated params are required for decode
  mode"). Proper engine-level probe is a follow-up (tracked with the
  forthcoming rank-pause fault test).

- Rust `SystemHealth::set_endpoint_registered`: auto-Ready an endpoint at
  registration when it has no registered canary target, regardless of
  global canary. Endpoints that opt in to canary (by passing a payload)
  still wait for canary to verify them — DIS-1185 contract preserved. Also
  extend `get_health_status` to consult `endpoint_health` (not just
  `system_health`) when there are no canary targets, so a worker whose
  endpoints have all registered Ready is reported healthy.

- Tests: parameterized trtllm helper tests + vllm regression guard for the
  decode-path payload, plus five new Rust unit tests covering the
  endpoint-registered branches and the get_health_status path updates.
Track B of the DIS-1737 fix. Launches a trtllm worker, waits for /health,
SIGSTOPs the engine-rank subprocess to simulate a hang, and asserts the
canary response.

Scenarios (parameterized):
  * trtllm-agg-canary-on   : expected "detect" — /health flips to 503
    after SIGSTOP. This is the positive case: proves the canary catches
    real engine hangs.
  * trtllm-agg-canary-off  : expected "miss" — /health stays 200 because
    there is no active probe. Negative control: proves the harness does
    not false-positive when canary is disabled.
  * trtllm-disagg-decode-canary-on : expected "miss" — trtllm disagg
    decode workers opt out of canary (handler rejects generic probes).
    Documents the current trade-off; flips to "detect" when the probe-
    canary follow-up lands.

Uses POSIX signals (os.kill SIGSTOP / SIGCONT) — no dependency on
tests/fault_tolerance/hardware/fault_injection_service. Style mirrors
tests/serve/test_trtllm.py::test_deployment (EngineProcess.from_script,
/health polling).

vllm and sglang rank-pause variants are follow-ups once we verify engine-
rank process discovery for each backend.
@nnshah1
nnshah1 marked this pull request as draft April 21, 2026 22:38
…stray shebang

Pre-commit's pytest-marker-report + check-shebang-scripts-are-executable
hooks flagged:

- tests/fault_tolerance/test_canary_rank_pause.py was missing a Lifecycle
  marker. Added pytest.mark.nightly (consistent with the sibling
  test_vllm_health_check.py in the same dir).

- components/src/dynamo/vllm/tests/test_vllm_health_check.py was missing
  a Hardware marker. Added pytest.mark.gpu_0 (pure-Python test).

- Same vllm test file had a stray #!/usr/bin/env python3 shebang despite
  not being marked executable. Removed the shebang — test modules aren't
  meant to be invoked directly.
Brings trtllm disagg decode to parity with vllm (natural agg-fallback)
and sglang (FAKE_BOOTSTRAP_HOST) for canary health checks. Decode
workers now register a probe payload and get real engine-level canary
coverage, including rank-pause detection.

Mechanism:

1. `TrtllmDisaggDecodeHealthCheckPayload` (new class) extends the
   standard payload with a reserved `_canary_probe=True` marker.

2. `_setup_disaggregated_params_for_mode` in handler_base.py checks for
   the marker before its strict DECODE rejection. When set, it returns
   synthetic `LlmDisaggregatedParams(request_type="context_and_generation")`.
   The cache transceiver only activates for request_type="generation_only",
   so this path is transceiver-free — the engine runs the probe as a full
   local prefill+decode, yields one token, and the canary succeeds.

3. `build_worker_health_check_payload` now returns the probe payload for
   DECODE mode instead of None, so the canary target is registered and
   the DIS-1185 "canary authority" contract applies.

Trade-off:
  * Before: decode `/health` was Ready at registration (via the Rust fix)
    with no active probing — engine hangs were invisible.
  * After: canary runs periodic probes; engine hang (rank paused, crash,
    stuck scheduler) → probe times out → `/health` flips to 503. k8s / LB
    can route away or restart the worker.

Tests:
  * test_health_check_disagg.py: flipped `test_decode_mode_opts_out_of_canary`
    to `test_decode_mode_registers_probe_payload` + added a payload-class
    direct-construction test. 5/5 pass locally.
  * test_canary_rank_pause.py: `trtllm-disagg-decode-canary-on` scenario
    flipped from expected="miss" to expected="detect", matching the new
    behavior. Needs GPU-bearing CI to verify end-to-end.
Coverage matrix: {trtllm, vllm, sglang} × {agg, disagg-prefill,
disagg-decode} — 9 detect scenarios plus one canary-off negative
control on trtllm-agg.

Per-backend engine-rank discovery via _RANK_PATTERNS, matching the
stragglers declared on each backend's test config:
  * trtllm: TRTLLM:EngineCore / tensorrt_llm
  * vllm:   VLLM::EngineCore  / EngineCoreProc
  * sglang: SGLANG:EngineCore / sgl_scheduler

Verifies each backend's disagg canary story end-to-end:
  * vllm decode:  handler agg-fallback when prefill_result absent
  * sglang decode: FAKE_BOOTSTRAP_HOST in SglangDisaggHealthCheckPayload
  * trtllm decode: _canary_probe short-circuit -> context_and_generation

Prefill scenarios confirm the canary-gated prefill path produces a
real response and is not blocked waiting for a decode peer.

Tests are @nightly + @gpu_1 — they run on a real GPU with real engine
startup per scenario (~2-3 min each) so they don't gate pre-merge CI.
Verified locally on Qwen3-0.6B disagg_same_gpu with canary enabled:

    decode /health = 200 (baseline, canary verified)
    SIGSTOP decode mpi rank  -> /health = 503 in 12s  (canary DETECTED hang)
    SIGCONT decode mpi rank  -> /health = 200 in  8s  (recovered)

TRT-LLM's PyTorch backend spawns each engine rank as
`python3 -R -m mpi4py.futures.server`, child of the
`python3 -m dynamo.trtllm` worker. The prior discovery patterns
(EngineCore / tensorrt_llm) matched nothing under this backend, so
the harness would fail before even trying the SIGSTOP.

Prepend mpi4py.futures.server as the primary pattern; keep
EngineCore / tensorrt_llm as defensive fallbacks for other backends
or future TRT-LLM changes.
@nnshah1
nnshah1 marked this pull request as ready for review April 22, 2026 04:05
@nnshah1

nnshah1 commented Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Update — significant rework since initial submission

The fix evolved substantially over the last round of iteration. Summary of the current state (HEAD 98452dfd3f) for reviewers:

Per-backend canary policy for disagg decode:

Backend Decode canary mechanism
vllm Standard VllmHealthCheckPayloadDecodeWorkerHandler._generate_token_mode already handles prefill_result=None by running agg-style locally.
sglang SglangDisaggHealthCheckPayload with FAKE_BOOTSTRAP_HOST (pre-existing, prod-proven).
trtllm (new) TrtllmDisaggDecodeHealthCheckPayload sets a _canary_probe marker; _setup_disaggregated_params_for_mode short-circuits to LlmDisaggregatedParams(request_type="context_and_generation"). Engine runs the probe as full local prefill+decode; cache transceiver activates only for "generation_only" and stays out of this path.

Rust-side fix in lib/runtime/src/system_health.rs:

  • set_endpoint_registered now auto-Readies endpoints with no registered canary target (DIS-1185 contract preserved for endpoints that opt in).
  • get_health_status path ci: update container registry location #3 consults endpoint_health when no targets are registered.
  • 5 new Rust unit tests.

Local end-to-end verification (GPU on an A6000):

  • Launched disagg_same_gpu.sh with DYN_HEALTH_CHECK_ENABLED=true; both prefill and decode workers became Ready via their canary probes.
  • SIGSTOP on decode's engine rank (mpi4py.futures.server) → /health flipped 503 in 12 s (canary detected the hang).
  • SIGCONT/health recovered to 200 in 8 s.

This is the correctness gate — proves the new probe actually reaches the engine (not a Python-layer bypass) and that canary detects real engine-level hangs for disagg decode, matching sglang.

New fault-tolerance test harness at tests/fault_tolerance/test_canary_rank_pause.py:

  • Parameterized across {trtllm, vllm, sglang} × {agg, disagg-prefill, disagg-decode} — 9 positive scenarios (expected detect).
  • One negative control (trtllm-agg-canary-off, expected miss) to prove the harness distinguishes both outcomes rather than always reporting success.
  • @nightly + @gpu_1; doesn't gate pre-merge.

CI status: trtllm-runtime / Test cuda13.1, amd64 is now green (was the original blocker). Remaining failure is sglang-runtime / Test cuda13.0, amd64 :: test_sql_tool_arguments_schema_valid, unrelated to this PR.

Happy to walk through any piece in detail.

@nnshah1

nnshah1 commented Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #8521, which ships a cleaner shape of the same DIS-1737 fix:

  • The Rust system_health.rs changes are split off (future separate PR)
  • The canary payload carries explicit top-level disaggregated_params instead of a _canary_probe marker — self-describing, no magic sentinel
  • Reworked test harness with port-aware rank discovery and @pytest.mark.model (saves ~30 GB of unnecessary model downloads)
  • Local E2E verified 6/6 cells: trtllm × {agg, disagg-prefill, disagg-decode} + vllm × same matrix

@nnshah1 nnshah1 closed this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::trtllm Relates to the trtllm backend backend::vllm Relates to the vllm backend fix size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant