fix(health): make canary sole authority on endpoint readiness when enabled

[nnshah1]

Summary

• When DYN_HEALTH_CHECK_ENABLED=true, transport endpoints (push, HTTP, TCP) no longer eagerly set endpoints to Ready at startup
• The canary health check becomes the sole authority on endpoint readiness, only marking Ready after verifying end-to-end request handling
• When DYN_HEALTH_CHECK_ENABLED=false (current default), behavior is unchanged

Motivation

Enabling canary health checks in k8s causes crash loops (DIS-1185). Root cause: push_endpoint eagerly sets Ready when its NATS subscription starts, then the canary health check fires, fails (discovery not yet propagated), and overrides to NotReady. Since /live and /health share the same handler, the liveness probe (failureThreshold=1) restarts the pod immediately.

This fix eliminates the race by making the canary the single writer. The startup probe's 2-hour window (failureThreshold=720) provides ample time for the canary to verify the endpoint before k8s transitions to liveness probes.

Changes

• system_health.rs: Add health_check_enabled field and getter to SystemHealth
• distributed.rs: Pass config.health_check_enabled to SystemHealth::new()
• push_endpoint.rs, http_endpoint.rs, shared_tcp_endpoint.rs: Gate eager set_endpoint_health_status(Ready) on !health_check_enabled()
• test_canary_health_race.py: Python repro test proving the fix

Test plan

• cargo fmt --check — pass
• cargo clippy --workspace — 0 warnings
• cargo test -p dynamo-runtime — all pass
• Python repro test (test_canary_health_race.py) — verifies endpoint stays NotReady until canary succeeds
• k8s deployment with DYN_HEALTH_CHECK_ENABLED=true — no crash loop

Related

• DIS-1185
• PR fix: health check race condition by waiting for watch stream discovery #4876, fix: delay canary health checks until system ready #4985, fix: turn off DYN_HEALTH_CHECK_ENABLED #5038 (prior partial fixes)

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Refined endpoint health status determination when canary health checks are enabled; status is now determined by actual health checks rather than eagerly marked as ready.

• Tests

  • Added integration test to validate health check race condition handling.

[coderabbitai]

Walkthrough

The pull request adds a new Python integration test for canary health-check race conditions (DIS-1185) and modifies the health status initialization logic to conditionally defer the "Ready" status when health canary checks are enabled. The health_check_enabled flag is threaded through the system via SystemHealth and checked at multiple endpoint registration/start points.

Changes

Cohort / File(s)	Summary
System Health Infrastructure lib/runtime/src/system_health.rs, lib/runtime/src/distributed.rs	Added health_check_enabled: bool field to SystemHealth with a public accessor method. Updated DistributedRuntime::new to pass config.health_check_enabled when constructing SystemHealth.
Conditional Health Status Updates lib/runtime/src/pipeline/network/ingress/http_endpoint.rs, lib/runtime/src/pipeline/network/ingress/push_endpoint.rs, lib/runtime/src/pipeline/network/ingress/shared_tcp_endpoint.rs	Modified endpoint registration/start logic to conditionally set health status to Ready only when health_check_enabled() is false; when canary checks are enabled, eagerly setting Ready is deferred.
Canary Health Race Test lib/bindings/python/tests/test_canary_health_race.py	Added new integration test reproducing DIS-1185 with async handler, port allocation, environment variable setup, and timed polling of /health and /live endpoints to capture health status transitions during canary timeout scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: making the canary health check the sole authority on endpoint readiness when enabled, which is the primary purpose of the PR.
Description check	✅ Passed	The description comprehensively covers all required template sections: Overview/Summary, Details of changes, specific files to review, and Related Issues, providing clear context for the fix and its motivation.
Docstring Coverage	✅ Passed	Docstring coverage is 84.62% which is sufficient. The required threshold is 80.00%.

_{✏️ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

lib/runtime/src/system_health.rs (1)

69-90: ⚠️ Potential issue | 🟠 Major

Force initial endpoint state to NotReady in canary mode.

SystemHealth::new(...) still seeds endpoint_health from starting_health_status. If that status is ever Ready, /health can come up green before the first canary run, which breaks the new “canary is the sole authority” guarantee. When health_check_enabled is true, initialize endpoint entries as NotReady regardless of starting_health_status.

Suggested fix

-        let mut endpoint_health = HashMap::new();
-        for endpoint in &use_endpoint_health_status {
-            endpoint_health.insert(endpoint.clone(), starting_health_status.clone());
-        }
+        let initial_endpoint_status = if health_check_enabled {
+            HealthStatus::NotReady
+        } else {
+            starting_health_status.clone()
+        };
+
+        let mut endpoint_health = HashMap::new();
+        for endpoint in &use_endpoint_health_status {
+            endpoint_health.insert(endpoint.clone(), initial_endpoint_status.clone());
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/runtime/src/system_health.rs` around lines 69 - 90, In SystemHealth::new,
the endpoint_health map is currently seeded from starting_health_status; when
health_check_enabled is true this can expose endpoints as Ready before canary
runs. Change the seeding logic in the constructor (where endpoint_health is
populated from use_endpoint_health_status) to set each endpoint's initial status
to NotReady whenever health_check_enabled == true, otherwise keep using
starting_health_status; update the code that inserts into endpoint_health (the
loop that fills endpoint_health in SystemHealth::new) to choose NotReady vs
starting_health_status accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/bindings/python/tests/test_canary_health_race.py`:
- Around line 33-61: The module currently picks a port at import time via
_get_free_port and mutates os.environ (setting _SYSTEM_PORT, DYN_SYSTEM_PORT,
DYN_HEALTH_CHECK_ENABLED, DYN_CANARY_WAIT_TIME,
DYN_HEALTH_CHECK_REQUEST_TIMEOUT) which causes cross-test leakage and race
conditions; instead move this logic into a test-scoped fixture that uses the
repository dynamic-port fixtures (runtime_services_dynamic_ports and
dynamo_dynamic_ports) and monkeypatch to set the env vars per-test, and stop
using the module-level _SYSTEM_PORT — generate the dynamic port(s) inside the
fixture, monkeypatch os.environ keys there, and ensure tests depend on that
fixture so each test gets isolated env and ports.
- Around line 63-92: The test is missing the suite scheduling marker and an
explicit timeout; add the required scheduling marker to the module-level
pytestmark list (alongside pytest.mark.gpu_0 and pytest.mark.integration) and
annotate the test_canary_overrides_transport_ready coroutine with an explicit
timeout decorator (e.g., `@pytest.mark.timeout`(30)) so the polling/network/sleep
work in never_responds and _poll_health cannot hang the test run.

---

Outside diff comments:
In `@lib/runtime/src/system_health.rs`:
- Around line 69-90: In SystemHealth::new, the endpoint_health map is currently
seeded from starting_health_status; when health_check_enabled is true this can
expose endpoints as Ready before canary runs. Change the seeding logic in the
constructor (where endpoint_health is populated from use_endpoint_health_status)
to set each endpoint's initial status to NotReady whenever health_check_enabled
== true, otherwise keep using starting_health_status; update the code that
inserts into endpoint_health (the loop that fills endpoint_health in
SystemHealth::new) to choose NotReady vs starting_health_status accordingly.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fe1fc577-8184-49e4-bfb8-f09023acbfa4

📥 Commits

Reviewing files that changed from the base of the PR and between 326a702 and 1def978.

📒 Files selected for processing (6)

• lib/bindings/python/tests/test_canary_health_race.py
• lib/runtime/src/distributed.rs
• lib/runtime/src/pipeline/network/ingress/http_endpoint.rs
• lib/runtime/src/pipeline/network/ingress/push_endpoint.rs
• lib/runtime/src/pipeline/network/ingress/shared_tcp_endpoint.rs
• lib/runtime/src/system_health.rs

[tmonty12]

LGTM

[nnshah1]

Backward Compatibility Analysis

Verified all scenarios with DYN_HEALTH_CHECK_ENABLED:

Scenario	Behavior	Status
false (operator default)	Transport sets Ready immediately, no canary	Identical to current ✅
Not set (local dev, tests)	Config defaults to false, same as above	Identical to current ✅
true (canary enabled)	Transport skips Ready, canary is sole authority	Fixes the race ✅
Shadow engine + false	set_health_status(True) still works	Unchanged ✅
Shadow engine + true	set_health_status(True) is a no-op (pre-existing, tracked in separate issue)	No regression ⚠️

Minor note: The transport endpoints acquire the system_health lock twice (once to check health_check_enabled(), once to call set_endpoint_health_status). Not a correctness issue (the field is immutable after construction), but slightly inefficient. Will clean up in a follow-up PR.