feat(toolkit): add REST healthcheck infrastructure#4167
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a shared readiness healthcheck registry in ChangesREST Readiness Healthcheck
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
libs/toolkit/src/contracts.rs (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: TODO is doubly-commented inside a doc comment.
Lines 74-75 use
///+//, so the rendered rustdoc shows literal// TODO: .... Drop the inner//for clean rendering. The gRPC hook is also tracked here as a follow-up — I can open an issue to track it if helpful.Proposed doc tweak
/// # gRPC healthcheck /// - /// // TODO: Add gRPC healthcheck hook in a follow-up PR. - /// // This PR intentionally implements REST readiness healthchecks only. + /// TODO: Add gRPC healthcheck hook in a follow-up PR. + /// This PR intentionally implements REST readiness healthchecks only.🤖 Prompt for 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. In `@libs/toolkit/src/contracts.rs` around lines 72 - 75, The rustdoc on the gRPC healthcheck section is doubly commented, so the rendered docs show literal comment markers instead of clean text. Update the doc comments in the contracts.rs healthcheck block to remove the inner “//” while keeping the note about the gRPC healthcheck hook and the REST-only scope; use the existing healthcheck-related doc section and surrounding contract comments to locate it.libs/toolkit/src/healthcheck.rs (2)
77-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional:
HealthcheckStatusandHealthcheckAggregateStatusare structurally identical.Two enums with the same three variants and serde config add maintenance overhead (any new variant must be mirrored, plus the manual mapping in
compute_aggregate). If the aggregate truly can never diverge from the per-check status set, consider a single type with an alias; if divergence is anticipated (e.g., a future aggregate-onlyInitializing), keeping them separate is justified — a short comment noting the rationale would help.🤖 Prompt for 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. In `@libs/toolkit/src/healthcheck.rs` around lines 77 - 93, The two status enums, HealthcheckStatus and HealthcheckAggregateStatus, are identical and currently require duplicated serde settings plus manual mirroring in compute_aggregate. Either consolidate them behind a single shared type/alias if the aggregate will always use the same variants, or keep both enums only if future divergence is intended; in that case, add a short rationale comment near the enum definitions and ensure compute_aggregate clearly uses the intended type mapping.
320-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: broad substring blocklist can mask useful diagnostics.
Entries like
"tenant","select ","update ", and"delete "will redact otherwise-safe operator messages (e.g.,"tenant pool exhausted"or"update queue backlog") to a generic"health check failed", reducing debuggability on/health. This is reasonable defense-in-depth for a public endpoint, but worth a brief comment documenting the intentional trade-off so future maintainers don't treat redacted messages as a bug.🤖 Prompt for 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. In `@libs/toolkit/src/healthcheck.rs` around lines 320 - 353, The broad blocklist in sanitize_message is intentionally redacting some otherwise-safe operator diagnostics, so add a brief comment near SUSPICIOUS_SUBSTRINGS or sanitize_message explaining that this is a deliberate defense-in-depth tradeoff for the public health endpoint. Keep the note focused on the intent behind entries like tenant, select , update , and delete , so future maintainers understand why messages may collapse to "health check failed" and do not treat it as an accidental bug.gears/system/api-gateway/src/web.rs (1)
40-57: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider caching/throttling readiness results.
/healthand/readyzare public routes that run every registered check (each spawning tasks, potentially pinging DB/upstreams) on every request with no caching. Under frequent probing or hostile traffic this can amplify load on downstream dependencies. Consider a short-lived cached report (e.g., a few hundred ms TTL) so bursts of probes reuse one result. Worth verifying whether these root-mounted health routes sit behind any rate limiting.🤖 Prompt for 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. In `@gears/system/api-gateway/src/web.rs` around lines 40 - 57, The health endpoints in health_detail and readyz_check currently call registry.report(HEALTHCHECK_TIMEOUT) on every request, which can repeatedly fan out expensive checks. Add a short-lived cached/throttled report layer inside the RestHealthcheckRegistry or a helper used by both handlers so bursty /health and /readyz probes reuse a recent result instead of spawning new checks each time. Keep the existing status_for_report and response shaping logic, but have both health_detail and readyz_check read from the shared cached report path.
🤖 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 `@gears/system/api-gateway/src/gear.rs`:
- Around line 105-138: The root-mounted health routes created in
apply_prefix_nesting/health_routes are being merged in a way that bypasses the
middleware stack applied to the prefixed router. Update apply_prefix_nesting so
the merged Router that serves /health, /healthz, and /readyz also goes through
the same apply_middleware_stack path as the rest of the API, using the existing
health_routes and apply_middleware_stack symbols to keep request IDs, tracing,
metrics, and CatchPanicLayer consistent.
In `@gears/system/api-gateway/src/web.rs`:
- Around line 51-54: Update the documentation comment for the `readyz` endpoint
in `web.rs` so it no longer calls `/readyz` “Shallow” or “all registered REST
healthchecks”; the implementation uses `registry.report(...)`, which is the deep
readiness probe. Align the wording with the module-level terminology in
`healthcheck.rs` by describing `/readyz` as readiness/deep and reserving
“shallow” or liveness language for `/healthz`, so operators are not misled when
wiring probes.
---
Nitpick comments:
In `@gears/system/api-gateway/src/web.rs`:
- Around line 40-57: The health endpoints in health_detail and readyz_check
currently call registry.report(HEALTHCHECK_TIMEOUT) on every request, which can
repeatedly fan out expensive checks. Add a short-lived cached/throttled report
layer inside the RestHealthcheckRegistry or a helper used by both handlers so
bursty /health and /readyz probes reuse a recent result instead of spawning new
checks each time. Keep the existing status_for_report and response shaping
logic, but have both health_detail and readyz_check read from the shared cached
report path.
In `@libs/toolkit/src/contracts.rs`:
- Around line 72-75: The rustdoc on the gRPC healthcheck section is doubly
commented, so the rendered docs show literal comment markers instead of clean
text. Update the doc comments in the contracts.rs healthcheck block to remove
the inner “//” while keeping the note about the gRPC healthcheck hook and the
REST-only scope; use the existing healthcheck-related doc section and
surrounding contract comments to locate it.
In `@libs/toolkit/src/healthcheck.rs`:
- Around line 77-93: The two status enums, HealthcheckStatus and
HealthcheckAggregateStatus, are identical and currently require duplicated serde
settings plus manual mirroring in compute_aggregate. Either consolidate them
behind a single shared type/alias if the aggregate will always use the same
variants, or keep both enums only if future divergence is intended; in that
case, add a short rationale comment near the enum definitions and ensure
compute_aggregate clearly uses the intended type mapping.
- Around line 320-353: The broad blocklist in sanitize_message is intentionally
redacting some otherwise-safe operator diagnostics, so add a brief comment near
SUSPICIOUS_SUBSTRINGS or sanitize_message explaining that this is a deliberate
defense-in-depth tradeoff for the public health endpoint. Keep the note focused
on the intent behind entries like tenant, select , update , and delete , so
future maintainers understand why messages may collapse to "health check failed"
and do not treat it as an accidental bug.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6107aa71-8c94-4412-a51c-37e23dd44ec2
📒 Files selected for processing (7)
gears/system/api-gateway/src/gear.rsgears/system/api-gateway/src/web.rsgears/system/api-gateway/tests/health_endpoints.rslibs/toolkit/src/contracts.rslibs/toolkit/src/healthcheck.rslibs/toolkit/src/lib.rslibs/toolkit/src/runtime/host_runtime.rs
refur-nfn
changed the title
7d7720c to
8c221d7
Compare
| /// Results are cached for [`REPORT_CACHE_TTL`] so bursty probes reuse the | ||
| /// most recent report instead of re-running every check on each request. | ||
| pub async fn report(&self, timeout_per_check: Duration) -> HealthcheckReport { | ||
| if let Some((ts, cached)) = self.cached_report.read().as_ref() |
There was a problem hiding this comment.
[RUST-CONC-001] HIGH — TOCTOU double fan-out in report() cache
The read lock is acquired and released on line 242, then a write lock is acquired at the end of the method. Any N concurrent callers that all observe a stale cache within that window each independently spawn a full fan-out of all checks, running every healthcheck N times instead of once.
// Current: read-lock dropped before awaits; concurrent callers all miss cache
if let Some((ts, cached)) = self.cached_report.read().as_ref()
&& ts.elapsed() < REPORT_CACHE_TTL
{
return cached.clone();
}
// ... all callers that passed the read-check now run all checks ...
*self.cached_report.write() = Some((Instant::now(), report.clone()));Fix: Use a tokio::sync::Mutex held across the full async body so that concurrent callers queue behind the in-flight computation:
// one write-lock gates the whole body
let mut guard = self.cached_report.lock().await;
if let Some((ts, cached)) = guard.as_ref() {
if ts.elapsed() < REPORT_CACHE_TTL {
return cached.clone();
}
}
let report = run_all_checks(&self.entries, timeout_per_check).await;
*guard = Some((Instant::now(), report.clone()));
report| /// message; the registry sanitises messages but defence-in-depth is preferred. | ||
| #[async_trait] | ||
| pub trait Healthcheck: Send + Sync + 'static { | ||
| fn name(&self) -> &'static str { |
There was a problem hiding this comment.
[RUST-SEC-001] HIGH — type_name::<Self>() exposed in unauthenticated /health JSON
The default name() returns the full Rust module path (e.g., api_gateway::module::MyCheck). This value is serialized verbatim into HealthcheckComponentReport.check and served on the unauthenticated /health endpoint, disclosing internal module structure to any caller.
// Current — emits "my_crate::module::DatabaseCheck" to external callers:
fn name(&self) -> &'static str {
std::any::type_name::<Self>()
}Fix: Remove the default and force implementors to supply an explicit, intentionally public name; or strip the module prefix before serializing:
// No default — implementors must supply a human-readable name:
fn name(&self) -> &'static str;| // Retrieve the healthcheck registry created in run_rest_phase before this call. | ||
| // Falls back to an empty registry so standalone tests still compile. | ||
| let hc_registry = ctx | ||
| .client_hub() |
There was a problem hiding this comment.
[RUST-ERR-001 + RUST-NO-002] HIGH — Discarded error + silent fallback masks misconfiguration in rest_prepare
unwrap_or_else(|_| ...) discards the original ClientHub::get error and silently substitutes an empty registry. If run_rest_phase fails to register the registry (e.g., due to ordering change or a future refactor), health checks will silently be unwired with no error or log.
// Current — error thrown away, empty registry substituted silently:
let hc_registry = ctx
.client_hub()
.get::<toolkit::RestHealthcheckRegistry>()
.unwrap_or_else(|_| Arc::new(toolkit::RestHealthcheckRegistry::new()));Fix: Propagate or at minimum log the original error:
let hc_registry = ctx
.client_hub()
.get::<toolkit::RestHealthcheckRegistry>()
.inspect_err(|e| tracing::warn!("healthcheck registry not found in ClientHub: {e:#}"))
.unwrap_or_else(|_| Arc::new(toolkit::RestHealthcheckRegistry::new()));Or return an error and let the runtime fail fast:
let hc_registry = ctx
.client_hub()
.get::<toolkit::RestHealthcheckRegistry>()
.context("RestHealthcheckRegistry not registered in ClientHub before rest_prepare")?;There was a problem hiding this comment.
Now uses self.healthcheck_registry OnceLock, and fallback logs tracing::warn!
| .healthcheck_registry | ||
| .lock() | ||
| .clone() | ||
| .unwrap_or_else(|| Arc::new(toolkit::RestHealthcheckRegistry::new())); |
There was a problem hiding this comment.
[RUST-NO-002] MEDIUM — Silent empty registry fallback in rest_finalize hides broken lifecycle
rest_finalize creates a fresh empty RestHealthcheckRegistry when self.healthcheck_registry is None, which means if rest_prepare was never called the finalized router will silently serve /health and /readyz with zero checks — no error, no warning.
This field is None only during broken lifecycle ordering. The fallback conceals the bug instead of surfacing it.
Fix: Fail fast or warn:
let hc_registry = self
.healthcheck_registry
.lock()
.clone()
.unwrap_or_else(|| {
tracing::warn!("healthcheck_registry is None in rest_finalize — rest_prepare may not have been called");
Arc::new(toolkit::RestHealthcheckRegistry::new())
});Or return Err if None is considered a programmer error here.
There was a problem hiding this comment.
Registry is a required param
| components: vec![], | ||
| } | ||
| } else { | ||
| let handles: Vec<_> = entries |
There was a problem hiding this comment.
[RUST-PERF-001] MEDIUM — Double-spawn per check: unnecessary task overhead
report() wraps each run_one_check call in an outer tokio::spawn, and run_one_check itself spawns an inner tokio::spawn for the check. Every registered check therefore allocates two tasks — the outer one adds no value since run_one_check doesn't panic and the inner spawn already catches check panics via JoinError.
// Outer spawn in report() — unnecessary:
tokio::spawn(run_one_check(gear, check, timeout_per_check))
// Inner spawn in run_one_check — this is the one that catches panics:
let mut handle = tokio::spawn(async move { check.check().await });Fix: Remove the outer tokio::spawn in report() and drive run_one_check directly via futures::future::join_all or FuturesUnordered; keep only the inner spawn for panic isolation:
let futures: Vec<_> = entries
.into_iter()
.map(|(gear, check)| run_one_check(gear, check, timeout_per_check))
.collect();
let components = futures::future::join_all(futures).await;There was a problem hiding this comment.
Outer spawn removed
| } | ||
|
|
||
| /// Register a healthcheck for the given gear. | ||
| pub fn register(&self, gear: &'static str, check: Arc<dyn Healthcheck>) { |
There was a problem hiding this comment.
[RUST-API-001] MEDIUM — gear: &'static str in register() is overly restrictive
Requiring a 'static string prevents callers from registering checks for dynamically-named gears (e.g., loaded from config, plugin instances, or OOP spawned gears with runtime names). Note that HealthcheckComponentReport::gear is already a String, so this restriction is artificial.
// Current — 'static only:
pub fn register(&self, gear: &'static str, check: Arc<dyn Healthcheck>) {Fix: Accept any string:
pub fn register(&self, gear: impl Into<String>, check: Arc<dyn Healthcheck>) {
self.entries.write().push(RegistryEntry {
gear: gear.into(),
check,
});
}And update RegistryEntry accordingly:
struct RegistryEntry {
gear: String, // was &'static str
check: Arc<dyn Healthcheck>,
}There was a problem hiding this comment.
Signature now `register(gear: impl Into, …)
| // diagnostics (e.g. "tenant lookup slow") and collapse the message to the generic | ||
| // "health check failed". That false-positive rate is accepted to avoid ever leaking | ||
| // credentials, DSNs, or query fragments to an unauthenticated caller. | ||
| static SUSPICIOUS_SUBSTRINGS: &[&str] = &[ |
There was a problem hiding this comment.
[RUST-SEC-001] MEDIUM — Blocklist missing common secret-bearing patterns
Several common patterns that appear in error messages from database drivers, cloud SDKs, and auth libraries are not in the blocklist and will pass through to unauthenticated callers:
"api_key","apikey","private_key","private""credential","access_key""mongodb://","redis://","amqp://","jdbc:""aws_","client_secret"
Consider extending the blocklist with these patterns. Alternatively, replace the blocklist approach with an allowlist that only passes messages matching a known-safe character set (alphanumeric, spaces, common punctuation, max length) — allowlists cannot have false negatives for novel secret formats:
fn sanitize_message(msg: &str) -> Option<String> {
if msg.len() > MAX_MESSAGE_LEN { return None; }
// Allow only printable ASCII, no control chars or URL-scheme chars
if msg.bytes().all(|b| b.is_ascii_alphanumeric() || b" .,!?-_:".contains(&b)) {
Some(msg.to_owned())
} else {
None // caller substitutes generic message
}
}There was a problem hiding this comment.
Blocklist extended
| /// | ||
| /// Runs all registered REST healthchecks and returns a JSON report. | ||
| /// Returns 200 (Healthy or Degraded), 503 (Unhealthy). | ||
| pub async fn health_detail(registry: Arc<RestHealthcheckRegistry>) -> Response { |
There was a problem hiding this comment.
[TOOLKIT-REST-001] MEDIUM — Closure capture instead of Extension<T> (non-golden-path)
health_detail and readyz_check receive the registry via closure capture in health_routes() rather than via Extension<Arc<RestHealthcheckRegistry>>. The ToolKit golden path states: "prefer stateless handlers that receive Extension<T>" — closures that capture environment are the explicitly-discouraged alternative.
// Current — closure capture (non-golden-path):
.route("/health", get(move || {
let r = reg_h.clone();
async move { web::health_detail(r).await }
}))Fix: Make health_detail and readyz_check accept Extension<Arc<RestHealthcheckRegistry>> and attach the layer on the router:
pub(crate) async fn health_detail(
Extension(registry): Extension<Arc<RestHealthcheckRegistry>>,
) -> Response { ... }
// In health_routes():
fn health_routes(hc_registry: Arc<RestHealthcheckRegistry>) -> Router {
Router::new()
.route("/health", get(health_detail))
.route("/readyz", get(readyz_check))
.route("/healthz", get(|| async { "ok" }))
.layer(Extension(hc_registry))
}Also change visibility to pub(crate) since these are only called from gear.rs.
There was a problem hiding this comment.
Clippy's redundant_pub_crate lint flags "(crate)" as redundant
|
|
||
| #[tokio::test] | ||
| async fn empty_registry_report_is_healthy_and_ready() { | ||
| let registry = RestHealthcheckRegistry::new(); |
There was a problem hiding this comment.
[RUST-TEST-001] MEDIUM — Missing cache-TTL test and mixed-aggregate tests
Two important behavioral properties have no test coverage:
1. Cache TTL: A second report() call within REPORT_CACHE_TTL must return the cached result without re-running checks. There is no test for this — a regression in the TOCTOU fix could silently break caching.
2. Mixed aggregate: One healthy + one unhealthy = Unhealthy; one healthy + one degraded = Degraded. The short-circuit in compute_aggregate (only Unhealthy returns early) needs a mixed-status test to catch regressions.
Please add both cases to healthcheck.rs unit tests.
| /// every [`Healthcheck`] implementor. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "lowercase")] | ||
| pub enum HealthcheckAggregateStatus { |
There was a problem hiding this comment.
[RUST-TYPE-001] LOW — Duplicate enum variants between HealthcheckStatus and HealthcheckAggregateStatus
Both enums have identical variants (Healthy, Degraded, Unhealthy). The doc comment acknowledges this and calls out a hypothetical divergence (e.g., a Starting variant), but that variant is not present.
Until there is an actual divergence the two-enum design adds conversion boilerplate in compute_aggregate with no payoff. Either:
- Add the diverging variant now (e.g.,
Startingfor the zero-checks case, currently returned asHealthy) to make the split immediately justified, or - Collapse to one enum until divergence is needed
The empty-registry path currently returns HealthcheckAggregateStatus::Healthy — returning Starting (or a separate Ok state meaning "no checks, assumed healthy") would be semantically cleaner and would justify the two-enum split.
There was a problem hiding this comment.
Collapsed to a single HealthcheckStatus
| }; | ||
|
|
||
| // REST healthcheck infrastructure | ||
| pub mod healthcheck; |
There was a problem hiding this comment.
[RUST-MOD-001] LOW — Redundant dual public path for healthcheck types
Both pub mod healthcheck and a flat pub use healthcheck::{...} are re-exported, making types reachable via two paths simultaneously:
toolkit::healthcheck::RestHealthcheckRegistrytoolkit::RestHealthcheckRegistry
Fix: Keep pub(crate) mod healthcheck and expose only the flat pub use at crate root, or expose the module without the flat re-exports. Either is fine; having both is the issue.
There was a problem hiding this comment.
Module made private mod healthcheck; + flat pub use only
| // ---------- outside prefix ---------- | ||
|
|
||
| #[tokio::test] | ||
| async fn health_endpoints_accessible_outside_prefix() { |
There was a problem hiding this comment.
[RUST-TEST-001] LOW — Missing negative-case assertion in prefix isolation test
health_endpoints_accessible_outside_prefix asserts that /healthz, /readyz, and /health return 200 at root paths when the prefix is /cf. But it does not assert that the same endpoints are NOT served under the prefix (i.e., /cf/healthz, /cf/readyz, /cf/health should return 404).
Without the negative case, the test does not fully verify the prefix-isolation invariant — a bug that duplicates health routes under the prefix would pass the test.
|
. |
| let body = Json(json!({ | ||
| "status": report.status, | ||
| "timestamp": Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true), | ||
| "components": report.components, |
There was a problem hiding this comment.
[HIGH] TOOLKIT-SEC-002 — health_detail exposes component internals to unauthenticated callers
The health_detail handler serializes report.components — including internal check names, gear names, and diagnostic messages — into the response body of the unauthenticated public /health endpoint without any authorization check.
Fix: Either require authentication for the detail /health endpoint (keeping /readyz as the public shallow probe), or strip the components array from the response for unauthenticated callers before serializing.
There was a problem hiding this comment.
/health no longer in public_routes
| let mut components = Vec::with_capacity(handles.len()); | ||
| for h in handles { | ||
| match h.await { | ||
| Ok(component) => components.push(component), |
There was a problem hiding this comment.
[MEDIUM] RUST-PERF-001 — Sequential collection of spawned check tasks
The for h in handles { h.await } loop awaits each task one at a time, serializing the collection phase. Tasks that finish early must wait for the previous .await to resolve, adding latency proportional to check count.
Fix: Use futures::future::join_all(handles).await or tokio::task::JoinSet to collect all results concurrently.
There was a problem hiding this comment.
join_all` Is used
| Some("health check failed".to_owned()), | ||
| ), | ||
| }, | ||
| () = tokio::time::sleep(timeout_per_check) => { |
There was a problem hiding this comment.
[MEDIUM] RUST-OBS-001 — No tracing on healthcheck timeout or panic
run_one_check converts check timeouts and panics to HealthcheckStatus::Unhealthy without emitting any tracing event. Operators see a 503 response but have no server-side log entry identifying the gear or check that failed.
Fix: Add a tracing::warn! in the timeout arm and a tracing::error! in the join-error arm, both including the gear name and check name as structured fields, before constructing the HealthcheckComponentReport.
There was a problem hiding this comment.
added error! on panic, warn! on timeout
|
|
||
| /// Result returned by a single [`Healthcheck::check`] call. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct HealthcheckResult { |
There was a problem hiding this comment.
[MEDIUM] RUST-DATA-001 — HealthcheckResult has no serialization versioning or contract documentation
HealthcheckResult derives Serialize/Deserialize with public fields and is re-exported from the crate root. Any future field rename or addition is a silently-breaking published type change.
Fix: Add a doc comment stating these field names are part of the stable serialized contract, and consider #[serde(deny_unknown_fields)] for forward-compatibility discipline.
There was a problem hiding this comment.
add stable-contract note
| /// Represents the result of a single gear's healthcheck execution, including | ||
| /// elapsed time and outcome status. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct HealthcheckComponentReport { |
There was a problem hiding this comment.
[MEDIUM] RUST-DATA-001 — HealthcheckComponentReport/HealthcheckReport serialized directly to HTTP with no versioning
Both structs have all fields pub, derive Serialize/Deserialize, are re-exported from the crate root, and are serialized directly by the /health endpoint. There is no version discriminator or format contract documentation.
Fix: Add doc comments declaring the JSON field names as a stable public contract. Consider an opaque version field or #[serde(deny_unknown_fields)] if forward-compatibility is required.
There was a problem hiding this comment.
added stable-contract one-liners on both structs
| // Always mark built-in health check routes as public | ||
| public_routes.insert((Method::GET, "/health".to_owned())); | ||
| public_routes.insert((Method::GET, "/healthz".to_owned())); | ||
| public_routes.insert((Method::GET, "/readyz".to_owned())); |
There was a problem hiding this comment.
[MEDIUM] TOOLKIT-REST-002 — /readyz auth declared via hardcoded set, not OperationBuilder.public()
The public auth status of /readyz is declared by inserting it into the public_routes set in build_route_policy_from_specs rather than at the point of route definition via OperationBuilder.public(). Auth declarations should be co-located with route definitions.
Fix: Register /readyz through OperationBuilder with .public() so its auth configuration is declared in one place and enforced uniformly.
There was a problem hiding this comment.
health routes (/health, /healthz, /readyz) now declare auth via OperationBuilder instead of a hardcoded set in gear.rs
| }, | ||
| () = tokio::time::sleep(timeout_per_check) => { | ||
| handle.abort(); | ||
| let _result = handle.await; |
There was a problem hiding this comment.
[LOW] RUST-NO-002 — let _result = handle.await silently discards join error after abort
After handle.abort(), the result of handle.await is unconditionally discarded. If the join fails for a reason other than cancellation, the error is invisible.
Fix: Ignore Err(e) if e.is_cancelled() explicitly but log any other JoinError as a debug/warn event to surface unexpected task failures.
There was a problem hiding this comment.
Logs any non-is_cancelled() JoinError after abort
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn default_healthcheck_check_returns_healthy() { |
There was a problem hiding this comment.
[LOW] TEST-QUALITY-1 — Constructor echo: default_healthcheck_check_returns_healthy
AlwaysHealthy does not override check(), so the test only confirms that the unmodified trait default returns what it is defined to return. This tests nothing that can actually fail.
Fix: Replace with a test that exercises a real check() implementation whose logic can fail, or remove it and rely on coverage from the report-level tests.
There was a problem hiding this comment.
Replaced by behavioral registry tests
| // AuthN Resolver client (resolved during init, None when auth_disabled) | ||
| pub(crate) authn_client: Mutex<Option<Arc<dyn AuthNResolverClient>>>, | ||
| // Readiness healthcheck registry — populated during REST wiring phase | ||
| pub(crate) healthcheck_registry: Mutex<Option<Arc<toolkit::RestHealthcheckRegistry>>>, |
There was a problem hiding this comment.
[LOW] RUST-NO-005 — Mutex<Option<Arc<RestHealthcheckRegistry>>> for a write-once value
healthcheck_registry is written exactly once during rest_prepare and then only read. Wrapping it in a parking_lot::Mutex<Option<...>> imposes unnecessary lock overhead on every router build thereafter.
Fix: Replace with std::sync::OnceLock<Arc<RestHealthcheckRegistry>> so initialization is atomic and subsequent reads are zero-cost.
| /// | ||
| /// TODO: Add gRPC healthcheck hook in a follow-up PR. | ||
| /// This PR intentionally implements REST readiness healthchecks only. | ||
| fn healthcheck( |
There was a problem hiding this comment.
[LOW] RUST-NO-007 — New healthcheck method on RestApiCapability has no version annotation
The new default healthcheck method is added to the published RestApiCapability trait without a /// Added in toolkit v0.X doc comment or corresponding CHANGELOG entry, making it impossible to track API version drift across crate releases.
Fix: Add a /// Since: toolkit v0.X doc line and a CHANGELOG entry so downstream implementors know when the method was introduced.
There was a problem hiding this comment.
added one-liner in the method doc
| } | ||
|
|
||
| #[test] | ||
| fn existing_rest_gear_compiles_without_healthcheck_impl() { |
There was a problem hiding this comment.
[LOW] TEST-QUALITY-1 — Constructor echo: existing_rest_gear_compiles_without_healthcheck_impl
The test calls the unoverridden trait default healthcheck() and asserts is_none(), which only confirms the default return value defined in the trait. Nothing runtime-dependent is verified.
Fix: Remove the test or convert it to a compile-only check, since the default-returns-None contract is implicitly verified by all other tests that rely on the default.
There was a problem hiding this comment.
Rewritten as gear_without_healthcheck_override_contributes_nothing_to_report — exercises registry report
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
[LOW] TEST-QUALITY-1 — Constructor echo: gear_can_return_some_healthcheck
GearWithHealthcheck::healthcheck is hardcoded to return Some(...). The assertion is_some() merely confirms the method returns what it was hardcoded to return, testing nothing beyond what the source code directly states.
Fix: Replace with a test that exercises runtime behaviour dependent on the healthcheck being present, such as verifying the check is invoked and its result propagates to a registry report.
There was a problem hiding this comment.
Rewritten as gear_with_healthcheck_override_is_invoked_via_registry — asserts the check runs and shows in the report
MikeFalcon77
left a comment
MikeFalcon77
left a comment
There was a problem hiding this comment.
Automated review: security, concurrency, observability, and test-coverage findings across the healthcheck feature.
| .route("/healthz", get(|| async { "ok" })); | ||
| let hc_registry = self | ||
| .healthcheck_registry | ||
| .lock() |
There was a problem hiding this comment.
RUST-NO-004 (HIGH)
Issue: parking_lot Mutex lock() is called inside async build_router. A contended parking_lot mutex blocks the OS thread and starves the Tokio worker.
Fix: Replace parking_lot Mutex on healthcheck_registry with tokio Mutex and await, or switch write-once fields to OnceLock.
There was a problem hiding this comment.
Replaced by OnceLock
| router = self.apply_middleware_stack(router, authn_client.clone())?; | ||
|
|
||
| let hc_registry = self | ||
| .healthcheck_registry |
There was a problem hiding this comment.
RUST-NO-004 (HIGH)
Issue: parking_lot Mutex lock() is called inside rest_finalize, which runs on the Tokio executor thread. Contention blocks the thread.
Fix: Same as build_router - use tokio Mutex with await or restructure healthcheck_registry as OnceLock.
| /// Minimum interval between fanning out a fresh round of healthchecks. | ||
| /// Bursty `/health` and `/readyz` probes within this window reuse the last | ||
| /// computed report instead of re-running every registered check. | ||
| const REPORT_CACHE_TTL: Duration = Duration::from_millis(500); |
There was a problem hiding this comment.
RUST-PERF-001 (MEDIUM)
Issue: REPORT_CACHE_TTL (500 ms) equals HEALTHCHECK_TIMEOUT (500 ms). Every cache miss triggers a full fan-out lasting up to 500 ms, during which concurrent callers also find the cache stale and each spawn their own full check set.
Fix: Set REPORT_CACHE_TTL larger than HEALTHCHECK_TIMEOUT (e.g. 2 s) and add coalescing so concurrent refreshes share one in-flight result.
| /// | ||
| /// Results are cached for [`REPORT_CACHE_TTL`] so bursty probes reuse the | ||
| /// most recent report instead of re-running every check on each request. | ||
| pub async fn report(&self, timeout_per_check: Duration) -> HealthcheckReport { |
There was a problem hiding this comment.
TOOLKIT-LIFE-001 (MEDIUM)
Issue: RestHealthcheckRegistry report() spawns Tokio tasks without accepting or checking a CancellationToken. In-flight healthcheck tasks are not cancelled on shutdown.
Fix: Add a CancellationToken parameter and use tokio::select! on it alongside the per-check timeout.
| for h in handles { | ||
| match h.await { | ||
| Ok(component) => components.push(component), | ||
| Err(_join_err) => { |
There was a problem hiding this comment.
RUST-ERR-001 (MEDIUM)
Issue: The JoinError from the outer tokio::spawn in report() is discarded via _join_err, losing which gear task panicked and why.
Fix: Log the error with tracing::warn! including gear name before pushing the fallback unhealthy entry.
There was a problem hiding this comment.
Outer spawn removed; inner join error logged with gear+check
| &self, | ||
| router: Router, | ||
| prefix: &str, | ||
| hc_registry: Arc<toolkit::RestHealthcheckRegistry>, |
There was a problem hiding this comment.
RUST-API-001 (MEDIUM)
Issue: apply_prefix_nesting now takes four parameters including Arc RestHealthcheckRegistry and Option Arc dyn AuthNResolverClient, tightly coupling prefix nesting with health-route construction and auth wiring.
Fix: Extract health-route creation and auth middleware into separate call-site steps; keep apply_prefix_nesting focused on router nesting only.
| /// | ||
| /// Runs all registered REST healthchecks and returns a JSON report. | ||
| /// Returns 200 (Healthy or Degraded), 503 (Unhealthy). | ||
| pub async fn health_detail(registry: Arc<RestHealthcheckRegistry>) -> Response { |
There was a problem hiding this comment.
RUST-MOD-001 (MEDIUM)
Issue: health_detail and readyz_check are declared pub but are only ever called from closures inside gear.rs - not part of any external API surface.
Fix: Change to pub(crate).
There was a problem hiding this comment.
Clippy's redundant_pub_crate lint flags "(crate)" as redundant
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn slow_check_times_out_and_becomes_unhealthy() { |
There was a problem hiding this comment.
RUST-TEST-001 (MEDIUM)
Issue: slow_check_times_out_and_becomes_unhealthy verifies aggregate status but not the component-level message field, leaving the health check timed out branch in run_one_check untested.
Fix: Add an assertion on report.components[0].message checking for the health check timed out string.
| assert_eq!(report.status, HealthcheckAggregateStatus::Unhealthy); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
RUST-TEST-001 (LOW)
Issue: No test covers compute_aggregate with a mix of Degraded and Unhealthy components to confirm Unhealthy takes priority.
Fix: Add a test registering both a DegradedCheck and an UnhealthyCheck and assert the aggregate is Unhealthy.
| /// - `check` must never leak secrets, passwords, DSNs, or stack traces in its | ||
| /// message; the registry sanitises messages but defence-in-depth is preferred. | ||
| #[async_trait] | ||
| pub trait Healthcheck: Send + Sync + 'static { |
There was a problem hiding this comment.
RUST-API-001 (LOW)
Issue: Both name() and check() have default implementations, so any Send+Sync+static type silently satisfies Healthcheck with a healthy default - accidental opt-in is invisible.
Fix: Remove the default for name() to force implementors to provide a meaningful name. Document check() default as an explicit opt-out escape hatch.
There was a problem hiding this comment.
name() default removed
Ideological Review: Handler Design & Rust Web ConventionsBeyond the automated checks, here is a design-level assessment of whether this handler pattern is idiomatic for Rust web services and convenient for gear authors. Ideological concerns
|
| Aspect | Convention | This PR |
|---|---|---|
State injection via State<T> |
✅ Axum standard | ❌ Manual closures |
| Configurable timeout | ✅ Production norm | ❌ Hardcoded |
| Cache with proper locking | ✅ tokio::sync::Mutex |
parking_lot::RwLock |
Bottom line: The design is ideologically sound — the right abstractions, the right K8s semantics, the right trait shape. The implementation has a few non-idiomatic patterns (closure wrapping instead of State<T>, double spawn, hardcoded timeout, TOCTOU cache) that should be addressed before merge but don't block the overall approach.
The RestApiCapability::healthcheck() hook is convenient for gear authors — implementing one method that returns Option<Arc<dyn Healthcheck>> is minimal friction, and the None default means zero breakage for existing gears.
| let reg_h = hc_registry.clone(); | ||
| let reg_r = hc_registry; | ||
| Router::new() | ||
| .route( |
There was a problem hiding this comment.
[HIGH] TOOLKIT-REST-001 / TOOLKIT-REST-002 — health routes bypass OperationBuilder and declare auth out-of-band.
health_routes() registers /health, /healthz, and /readyz via raw Router::new().route() calls with bare closures. Authentication intent is then declared separately by manually inserting into public_routes (lines 191-193). Route definition and auth declaration are decoupled, making auth audits error-prone.
Note: /healthz was already registered this way before this PR. /readyz is new and should follow whatever pattern the team decides to standardize on. Fix: Register these routes via OperationBuilder::new().public() so auth intent is co-located with the route definition, consistent with all other ToolKit REST endpoints.
There was a problem hiding this comment.
co-locating auth via .public() is a real readability win, but the mechanical fix conflicts with the root-mount + no-OpenAPI-exposure requirements. Current code compromises by de-duping paths through shared consts (HEALTH_DETAIL_PATH/HEALTHZ_PATH/READYZ_PATH) used at both the route and the public_routes site, so they can't silently drift
| use std::time::Duration; | ||
| use toolkit::RestHealthcheckRegistry; | ||
|
|
||
| const HEALTHCHECK_TIMEOUT: Duration = Duration::from_millis(500); |
There was a problem hiding this comment.
[MEDIUM] RUST-API-001 — HEALTHCHECK_TIMEOUT hardcoded at 500ms with no operator knob.
Services with slow downstream dependencies may never produce a meaningful readiness signal within 500ms, and there is no way to tune this without recompilation. Also note that REPORT_CACHE_TTL in healthcheck.rs is also 500ms — equal to the timeout — meaning a worst-case timed-out check fills the cache with an Unhealthy result that expires exactly when the next probe arrives, providing no buffering margin.
Fix: Add a healthcheck_timeout_ms: u64 field to ApiGatewayConfig (default 500) and thread it through to health_detail and readyz_check. Set REPORT_CACHE_TTL strictly greater than the configured timeout (e.g. 2x) so bursty probes reuse the last result.
There was a problem hiding this comment.
(default 500) threaded via HealthcheckTimeout Extension
| // Create the healthcheck registry and publish it to ClientHub so both | ||
| // the REST provider loop (below) and the API Gateway can access it. | ||
| let hc_registry = Arc::new(crate::healthcheck::RestHealthcheckRegistry::new()); | ||
| self.client_hub |
There was a problem hiding this comment.
[HIGH] TOOLKIT-CLIENT-001 / RUST-MOD-001 — RestHealthcheckRegistry stored in ClientHub, which is designed for inter-gear service clients (gRPC stubs, HTTP clients), not infrastructure registries.
Using ClientHub as a side-channel to pass state from run_rest_phase to ApiGateway::rest_prepare means any gear can retrieve the registry via client_hub.get::<RestHealthcheckRegistry>() and register checks outside the intended wiring phase, bypassing the write-once access control this PR intends to enforce.
Fix: Either (a) add Arc<RestHealthcheckRegistry> as an explicit parameter to ApiGatewayCapability::rest_prepare and rest_finalize, or (b) hold it as a field on HostRuntime and pass it explicitly. Do not route infrastructure state through the general-purpose service-locator.
There was a problem hiding this comment.
Registry created in run_rest_phase and passed explicitly to rest_prepare/rest_finalize
82dc45a to
2e1dac5
Compare
5c0d00a to
f7890f2
Compare
- Concurrent healthcheck execution with timeout protection - HealthcheckRegistry for registering and running per-gear probes - Message sanitization to prevent secret leaks in responses - API Gateway integration for /health and /readyz endpoints - Comprehensive test coverage with panic/timeout/degradation scenarios Signed-off-by: Raudur Refur <261796072+refur-nfn@users.noreply.github.com>
f7890f2 to
c1f4a5f
Compare
2 participants
Summary by CodeRabbit
/health,/healthz, and/readyzendpoints with JSON status, RFC3339 timestamp, and per-component details.200vs503.