fix: health check race condition by waiting for watch stream discovery

[tzulingk]

The Fix:
Added synchronization in send_health_check_request() to wait for the watch stream to complete its initial discovery before attempting to send the health check:

match tokio::time::timeout(
    Duration::from_secs(10),
    router.client.wait_for_instances()
).await {
    Ok(Ok(instances)) => {
        debug!("watch stream ready, found {} instance(s)", instances.len());
    }
    Ok(Err(e)) => return Err(...),
    Err(_) => return Err(...), // 10s timeout
}

Why this approach:

• Uses existing wait_for_instances() method (no new API)
• Only affects health check code path (minimal blast radius)
• Has timeout protection (10s) to prevent hanging
• Doesn't impact normal request routing or other Client users
• First health check now succeeds deterministically

Expected behavior after fix:

• First health check succeeds (waits for watch stream)
• Subsequent health checks succeed (use cached router)
• Clear error messages if discovery fails or times out
• Decode workers no longer crash on startup

Where should the reviewer start?

lib/runtime/src/health_check.rs

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

DIS-1167

Summary by CodeRabbit

• Bug Fixes
  • Improved health check reliability by ensuring instance discovery completes before health checks proceed, reducing potential race conditions during startup.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Health check flow modified to wait up to 10 seconds for PushRouter's watch stream to discover instances via wait_for_instances(). Returns error on discovery failure or timeout; logs instance count on success. Prevents race conditions during initial health check.

Changes

Cohort / File(s)	Summary
Health check instance discovery waiting lib/runtime/src/health_check.rs	Added synchronous wait mechanism (up to 10 seconds) after creating/obtaining a PushRouter to ensure instance discovery completes before health check proceeds. Handles success (logs count), error (returns error), and timeout (returns timeout error) cases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Specific areas requiring attention:
  • Verify the 10-second timeout duration is appropriate for the expected instance discovery latency
  • Confirm the error handling and logging messages are clear and actionable
  • Validate that the timing change effectively prevents the intended race condition without introducing new issues

Poem

🐰 A patient rabbit waits with care,
For instances to gather there,
Ten seconds' grace, no race today,
Healthy checks shall have their way!
No more stumbling in the dark,
Discovery leaves its proper mark. ✨

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: fixing a health check race condition by waiting for watch stream discovery.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The pull request description follows the required template structure with all key sections present: overview with the fix details, explanation of the approach, expected behavior, reviewer guidance, and related issue reference.

---

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: 0

🧹 Nitpick comments (1)

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

270-299: Discovery wait logic looks sound; consider edge‑case handling and configurability

The new timeout(Duration::from_secs(10), router.client.wait_for_instances()) block correctly addresses the startup race by ensuring instance discovery (or a clear failure) before sending the health check, and the error messages are descriptive.

A couple of non‑blocking refinements you might consider:

• If wait_for_instances() can ever return Ok(instances) with an empty list, treating that as at least a warn! (or even an early Err) would make the “target registered but no backing instances discovered” case more obvious rather than relying solely on a later routing error.
• The hard‑coded 10 second discovery timeout could be promoted into HealthCheckConfig (e.g., discovery_timeout) or at least a const so it’s easier to tune per environment and stays consistent with the other health‑check timeouts.

These are optional and don’t block the fix; the current change is functionally correct for the stated PR goals.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 664b458 and 949e38e.

📒 Files selected for processing (1)

• lib/runtime/src/health_check.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-09-11T03:24:47.820Z

Learnt from: kthui
Repo: ai-dynamo/dynamo PR: 3004
File: lib/runtime/src/pipeline/network/ingress/push_handler.rs:271-277
Timestamp: 2025-09-11T03:24:47.820Z
Learning: In lib/runtime/src/pipeline/network/ingress/push_handler.rs, the maintainer prefers to keep the existing error comparison logic using format!("{:?}", err) == STREAM_ERR_MSG unchanged until proper error types are implemented, even though it has technical debt. Avoid suggesting changes to working legacy code that will be refactored later.

Applied to files:

• lib/runtime/src/health_check.rs

🧬 Code graph analysis (1)

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

lib/runtime/src/component/client.rs (1)

• instances (63-65)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: sglang (amd64)
• GitHub Check: trtllm (amd64)
• GitHub Check: vllm (arm64)
• GitHub Check: clippy (lib/bindings/python)
• GitHub Check: clippy (.)
• GitHub Check: clippy (launch/dynamo-run)
• GitHub Check: Build and Test - dynamo