feat(health): suppress unchanged success-only reports within configurable interval

[rsd-darshan]

Summary

Fixes #1811

Health clients sent reports to core on every collection tick regardless of whether content had changed. For stable machines, the vast majority of reports are success-only and carry identical data — causing unnecessary API and network load even though the database already deduplicates at storage time via hash_without_timestamps.

• Add a suppress_unchanged_interval config field (default 5m) to HealthReportSinkConfig
• Add a per-key last-sent cache in HealthReportSink that hashes report content (excluding timestamps) and skips re-enqueuing a success-only report whose content is unchanged within the interval
• Reports that contain any alert bypass suppression entirely and are always forwarded immediately
• Set suppress_unchanged_interval = null to disable suppression

What changes

crates/health/src/config.rs

• New field suppress_unchanged_interval: Option<Duration> on HealthReportSinkConfig, serialized with humantime_serde (e.g. "5m"), defaults to Some(300s)

crates/health/src/sink/health_report.rs

• content_hash() — deterministic hash over sorted successes and alerts, excluding timestamps; mirrors the DB-side hash_without_timestamps approach
• LastSent — lightweight per-key struct tracking content hash and send time
• Suppression gate in handle_event before queue.save_latest
• 4 new unit tests: suppress within interval, alerts bypass suppression, changed content forwarded, suppression disabled

Test plan

• cargo test -p carbide-health --lib — all 147 tests pass
• Deploy to a staging environment and confirm API call rate drops for stable machines
• Confirm machines with active alerts still receive timely report updates

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3bc3fbb4-915a-414f-aad5-1455f8e4625c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[yoks]

Thanks @rsd-darshan can you also please add quick criterion test for hashing function? As it sits on hot path we may need to be sure it is not overly heavy on CPU.

[yoks]

/ok to test 055260e

[github-actions]

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

[rsd-darshan]

Thanks for the feedback! Added a criterion benchmark for the hashing function in the latest commit — it covers five report shapes: empty, small/large success-only, and small/large with alerts, to capture the realistic common case as well as worst-case scenarios.

[yoks]

Thanks, can you post results here? Latest CI run also had lint issues.

[yoks]

There is also one minor problem. HashMap with last seen report would grow without bounds over lifetime if report keys will drift (e.g. hardware added/removed). So maybe we can think of struct which would evict stale keys (hours old) or evict them on schedule.

[rsd-darshan]

Benchmark results (Apple M-series, release mode):

Report shape	Time
empty	~32 ns
small success-only (8 probes)	~1.27 µs
small with alerts (4 alerts)	~1.90 µs
large with alerts (64 alerts)	~52 µs
large success-only (256 probes)	~110 µs

The common case (small success-only reports) is ~1.3 µs per hash. The hashing only runs on success-only reports, so the alert path is unaffected. The large_success_only case (256 probes) at ~110 µs is an extreme outlier unlikely in practice.

Unbounded map growth — good catch. Fixed in the latest commit: entries older than 2 × suppress_unchanged_interval are evicted on each insert, keeping the map bounded to the set of recently active machines.

[rsd-darshan]

@yoks both points addressed in the latest commit — stale entry eviction is in, and benchmark results are in the comment above. Let me know if anything else needs changing!

[ajf]

/ok to test 4108f43

[yoks]

Can you please address remaining comments to the review.

Thanks!

[rsd-darshan]

All review points addressed in the latest commit:

• Eviction throttling — introduced LastSentCache that tracks last_evicted and only runs retain() when 2× suppress_interval has elapsed, so it's O(n) at most once per interval rather than on every insert
• Dead code — removed the alert-path hashing from content_hash; it only runs on success-only reports so the alert branch was unreachable
• Alert clears suppression — added cache.entries.remove(&key) on the alert path so the first all-clear success after an alert is never throttled
• Alert test — rewrote report_with_alerts_never_suppressed to verify the full alert → recovery sequence rather than just checking the dedup queue

The nit about updating last_sent before confirming the send succeeded is noted — happy to address that too if you'd like, though it would require threading the result back from the worker.

[yoks]

/ok to test db3e903

[yoks]

/ok to test ddd7ee3

[rsd-darshan]

Fixed the collapsible_if clippy lint in the latest commit — could someone run /ok to test on 9bfa3781 to trigger CI?

[yoks]

/ok to test 9bfa378

[rsd-darshan]

The only change in the latest commit is a mechanical rustfmt reformat — no logic was touched. Could either of you re-approve when you get a chance? Thanks!

[yoks]

/ok to test 3475806