fix(health): ensure reconciliation of reachability and nmx-c collectors

[jayzhudev]

A collector spawn failure can end a discovery iteration before reachability and nmx-c collector reconciliation run, leaving the collectors out of sync until a successful iteration happens later.

This change reconciles reachability and nmx-c collectors before returning the ordinary collector spawn error.

Related issues

Fixes #4865

Type of Change

• Add - New feature or capability
• Change - Changes in existing functionality
• Fix - Bug fixes
• Remove - Removed features or deprecated functionality
• Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

• This PR contains breaking changes

Testing

• Unit tests added/updated
• Integration tests added/updated
• Manual testing performed
• No testing required (docs, internal refactor, etc.)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f4592cf2-caaf-475c-a3ed-452e15927a53

📥 Commits

Reviewing files that changed from the base of the PR and between 1da8adc and 15f2ff7.

📒 Files selected for processing (1)

• crates/health/src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/health/src/lib.rs

---

Summary by CodeRabbit

• Bug Fixes
  • Improved endpoint discovery so all endpoints are attempted, even when an earlier collector encounters an error.
  • Ensured reachability information is reconciled before discovery reports failures.
  • Preserved clear error reporting by prioritizing collector startup errors over reachability errors.
  • Added detailed reporting for collector startup failures across multiple endpoints.
  • Improved diagnostics by reporting failures from multiple endpoints together, helping identify all affected services in a single discovery attempt.

Walkthrough

The discovery iteration now attempts all ordinary collectors, reconciles reachability collectors after spawn attempts, and returns aggregate spawn errors before reachability errors. Tests verify later endpoint spawning, logging, and cleanup.

Changes

Reachability reconciliation

Layer / File(s)	Summary
Spawn and reconciliation control flow crates/health/src/discovery/iteration.rs, crates/health/src/lib.rs	Discovery aggregates endpoint-keyed collector spawn failures, always reconciles reachability collectors, and returns HealthError::CollectorSpawnErrors before reachability errors.
Failure-path test coverage crates/health/src/discovery/iteration.rs	The test configures multiple conflicts, confirms later endpoints still spawn, checks reachability failure logs, and performs cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to 15f2f

This is a localized fix that reconciles reachability and nmx-c collectors after a collector spawn failure; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant DiscoveryIteration
  participant CollectorRegistry
  participant ReachabilityCollectors
  DiscoveryIteration->>CollectorRegistry: attempt all endpoint collector spawns
  DiscoveryIteration->>ReachabilityCollectors: reconcile reachability collectors
  DiscoveryIteration-->>DiscoveryIteration: return CollectorSpawnErrors before reachability errors

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary fix: reconciling reachability and NMX-C collectors after collector spawn failures.
Description check	✅ Passed	The description directly explains the discovery failure, reconciliation fix, linked issue, and testing performed.
Linked Issues check	✅ Passed	The changes address issue #4865 by continuing collector spawning and reconciling reachability collectors before returning spawn errors.
Out of Scope Changes check	✅ Passed	The changes remain within scope because error aggregation, reconciliation ordering, documentation, and tests support the stated health discovery fix.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/health/src/discovery/iteration.rs`:
- Around line 121-123: Update the iteration around spawn_collectors_for_endpoint
to invoke it for every sharded_endpoints entry instead of short-circuiting on
the first error, while preserving and returning the first spawn error after all
attempts complete. Add a regression test covering a conflicting first endpoint
and confirming that a non-conflicting second endpoint is still spawned.

🪄 Autofix

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

Run ID: da3b9b63-559e-4f8d-a3b7-9bfa9cfec564

📥 Commits

Reviewing files that changed from the base of the PR and between 81c1627 and a707e4b.

📒 Files selected for processing (1)

• crates/health/src/discovery/iteration.rs

[yoks]

Worth moving nmx-c reconciler above as well, it seems it has similar issue?

[github-actions]

🌿 Preview your docs: https://nvidia-preview-pull-request-4866.docs.buildwithfern.com/infra-controller

[jayzhudev]

Worth moving nmx-c reconciler above as well, it seems it has similar issue?

Yep, good catch!