[ Major Refactor ] Modular to Crates#9
Conversation
Extract AppError, DatabaseError, SshError and EnvironmentError into crates/err and remove local error modules. Rename the Result alias to AppResult and update call sites. Add axum_backend crate and update workspace Cargo.toml/Cargo.lock with new dependencies (ssh2, libssh2- sys, dotenvy, tracing-subscriber, etc.).
Add a new crates/env crate (lib + loader) and register it in the workspace/Cargo.toml. Remove the old src/sys/env module files and update call sites and imports. Rename the crate's Result alias to EnvResult to avoid conflicts and adjust loader function signatures accordingly.
Introduce a new workspace crate `hlt` implementing a health check framework (HealthRegistry, traits, models) with unit and integration tests. Replace the old internal `sys::health` modules with `hlt` usage: update AppState to hold a HealthRegistry, migrate DB health checker and Axum handler to use the new API, and wire the registry in initialization. Update Cargo workspace to include the new crate.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRestructures the repo into a Cargo workspace and extracts environment, error, health, and logging functionality into four new crates. Rewires application to use HealthRegistry, centralized errors (err), env utilities, and logging crate; removes many legacy sys/* error/health/env modules and updates imports and return types accordingly. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 review |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the application by extracting three internal modules (err, env, and health) into separate workspace crates. The goal is to improve modularity, reusability, and maintainability by creating independent, well-tested crates with clear APIs.
- Removes internal
err,env, andsys/healthmodules from the main crate - Creates three new workspace crates:
crates/err,crates/env, andcrates/hlt(health) - Updates all imports throughout the codebase to use the new external crates
- Adds comprehensive documentation and tests to the new crates
Reviewed Changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Converts to workspace structure with shared dependencies |
| crates/err/* | New error handling crate with domain-specific errors |
| crates/env/* | New environment variable loading crate |
| crates/hlt/* | New health check framework crate |
| src/sys/mod.rs | Removes extracted modules from sys exports |
| src/sys/init.rs | Updates to use new hlt::HealthRegistry API |
| src/dbs/health.rs | Missing import for env crate |
| src/ssh/connector.rs | Updates imports to use new crates |
| src/rts/health.rs | Refactored to use HealthRegistry with added tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/dbs/connector.rs (1)
37-51: Add missing import forenvcrate.The code on lines 37, 40, 43, 46, and 49 uses
env::get_required()but theenvcrate is not imported insrc/dbs/connector.rs. Althoughenvis declared as a workspace dependency inCargo.toml, it must be explicitly imported in the file where it's used.Add the following import at the top of the file:
use env;src/ssh/connector.rs (1)
44-69: Timeout drops the JoinHandle but doesn’t stop the blocking task
spawn_blockingkeeps running after the outertimeoutfires; dropping the handle doesn’t cancel the work. This can accumulate stragglers under load.
- Ensure inner timeouts (
SSH_CONNECTION_TIMEOUT,SSH_OPERATION_TIMEOUT) are strictly lower than the outer request timeout.- Consider a cooperative cancel flag checked within
execute_blocking_ssh_connection(e.g.,AtomicBool), or move to a fully async SSH client if possible.- Alternatively, gate the call with a bounded semaphore to limit concurrency.
Example guard:- let task_result = timeout( + // Acquire a permit to limit concurrent blocking SSH attempts + let _permit = crate::limits::SSH_BLOCKING_SEMAPHORE.acquire().await.unwrap(); + let task_result = timeout( Duration::from_secs(request_timeout), task::spawn_blocking(move || execute_blocking_ssh_connection(&creds)), )
♻️ Duplicate comments (1)
src/dbs/health.rs (1)
12-12: Missing import for theenvcrate.Line 12 uses
env::get_parsed_or_defaultbut there is no correspondinguse env;or similar import statement. This will cause a compilation error.Add the import at the top of the file:
use crate::dbs::Database; use hlt::{ComponentHealth, HealthCheck}; +use env; use tokio::time::{Duration, Instant, timeout}; use tracing::{debug, warn};
🧹 Nitpick comments (19)
crates/err/README.md (1)
3-3: Consider hyphenating the compound adjective."error handling crate" should be "error-handling crate" when used as a compound adjective before a noun.
Apply this diff:
-A comprehensive error handling crate for Axum-based applications, providing type-safe error types with automatic HTTP response conversion. +A comprehensive error-handling crate for Axum-based applications, providing type-safe error types with automatic HTTP response conversion.Cargo.toml.backup (1)
1-27: Consider removing the backup file from version control.Backup files are unnecessary in version control systems like Git, which already preserve the complete history of changes. The previous version of
Cargo.tomlcan be retrieved usinggit logandgit show.Consider removing this file:
git rm Cargo.toml.backupcrates/err/src/domain/mod.rs (1)
1-7: Clean domain surface; re-exports look good.Keeps modules private and exposes only types — good API hygiene. Consider adding a short module-level doc on intended stability of these re-exports.
crates/err/src/domain/ssh.rs (2)
4-21: Improve error ergonomics: add non_exhaustive, derives, and optional source chaining.Current String payloads lose the original cause. Recommend:
- Add #[non_exhaustive] to allow adding variants without semver break.
- Derive Clone, PartialEq, Eq for easier testing.
- Add optional #[source] to carry an underlying error (where available).
Example diff (minimal, no new deps):
use thiserror::Error; +use std::error::Error as StdError; +use std::time::Duration; /// SSH-specific errors. -#[derive(Debug, Error)] +#[derive(Debug, Error, Clone, PartialEq, Eq)] +#[non_exhaustive] pub enum SshError { /// SSH connection failed - #[error("SSH connection failed: {0}")] - ConnectionFailed(String), + #[error("SSH connection failed: {msg}")] + ConnectionFailed { msg: String, #[source] source: Option<Box<dyn StdError + Send + Sync>> }, /// SSH authentication failed - #[error("SSH authentication failed: {0}")] - AuthenticationFailed(String), + #[error("SSH authentication failed: {msg}")] + AuthenticationFailed { msg: String, #[source] source: Option<Box<dyn StdError + Send + Sync>> }, /// Internal SSH task error - #[error("Internal SSH task error: {0}")] - InternalTaskError(String), + #[error("Internal SSH task error")] + InternalTaskError { #[source] source: Box<dyn StdError + Send + Sync> }, /// SSH operation timed out - #[error("SSH operation timed out: {0}")] - TimeoutError(String), + #[error("SSH operation timed out after {elapsed:?}")] + TimeoutError { elapsed: Duration }, }
27-31: Add tests for all variants (not only ConnectionFailed).Cover AuthenticationFailed, InternalTaskError, and TimeoutError to lock Display strings and future-proof refactors.
crates/err/src/lib.rs (1)
4-8: LGTM; consider a small prelude.The surface is clear. Optional: add a prelude to simplify imports for dependents.
pub use app_error::AppError; pub use domain::{DatabaseError, EnvironmentError, SshError}; pub type AppResult<T> = std::result::Result<T, AppError>; + +/// Common re-exports for convenient `use err::prelude::*;` +pub mod prelude { + pub use super::{AppError, AppResult, DatabaseError, EnvironmentError, SshError}; +}crates/hlt/src/lib.rs (1)
1-12: Solid public API; add prelude and a short doc example.Exports are clear. Optional: provide a prelude and a minimal example to improve discoverability.
//! # hlt - Health Check Framework //! -//! A lightweight, extensible health check framework with Axum integration. +//! A lightweight, extensible health check framework with Axum integration. +//! +//! Example: +//! ``` +//! use hlt::{HealthRegistry, HealthStatus, ComponentHealth, HealthCheck}; +//! # struct Pinger; +//! # #[async_trait::async_trait] impl HealthCheck for Pinger { +//! # async fn check(&self) -> HealthStatus { HealthStatus::Healthy } +//! # } +//! let mut reg = HealthRegistry::default(); +//! reg.register("pinger", Box::new(Pinger)); +//! # let _ = reg; +//! ``` mod models; mod registry; mod traits; // Public API exports (NO handler export!) pub use models::{ComponentHealth, HealthStatus, SystemHealthResponse}; pub use registry::HealthRegistry; pub use traits::HealthCheck; + +/// Common re-exports for convenient `use hlt::prelude::*;` +pub mod prelude { + pub use crate::{ComponentHealth, HealthCheck, HealthRegistry, HealthStatus, SystemHealthResponse}; +}crates/hlt/tests/integration.rs (1)
120-123: Relax timing assertion to avoid CI flakiness50ms for 10 concurrent 10ms sleeps is tight on shared runners. Use a safer bound or relative assertion.
- assert!(duration.as_millis() < 50); + // Expect << 100ms (sequential). Allow headroom for scheduler jitter. + assert!(duration < std::time::Duration::from_millis(80));src/rts/health.rs (1)
15-35: Add tracing span to the handlerInstrument the handler for better correlation in logs.
-use tracing::{debug, warn}; +use tracing::{debug, warn, instrument}; @@ -pub async fn health_handler(State(state): State<Arc<AppState>>) -> impl IntoResponse { +#[instrument(skip(state))] +pub async fn health_handler(State(state): State<Arc<AppState>>) -> impl IntoResponse {src/sys/init.rs (1)
106-111: Downgrade log level when .env is missingAbsence of a
.envis common in prod; usewarn!instead oferror!.- } else { - error!("No .env file found"); - } + } else { + warn!("No .env file found"); + }crates/err/src/domain/dbs.rs (1)
27-33: Map SurrealDB errors more preciselyCurrent impl funnels all
surrealdb::ErrorintoQueryError. Consider matching variants to retain semantics (auth vs not found vs query).-impl From<surrealdb::Error> for DatabaseError { - fn from(err: surrealdb::Error) -> Self { - Self::QueryError(err.to_string()) - } -} +impl From<surrealdb::Error> for DatabaseError { + fn from(err: surrealdb::Error) -> Self { + use surrealdb::error::Db as SDb; + match err { + surrealdb::Error::Db(SDb::AuthenticationFailed | SDb::InvalidAuth) => + Self::AuthenticationError(err.to_string()), + surrealdb::Error::Db(SDb::TableNotFound | SDb::RecordNotFound) => + Self::NotFound(err.to_string()), + _ => Self::QueryError(err.to_string()), + } + } +}Optionally mark the enum as non-exhaustive for evolution:
-#[derive(Debug, Error)] -pub enum DatabaseError { +#[derive(Debug, Error)] +#[non_exhaustive] +pub enum DatabaseError {src/ssh/connector.rs (1)
71-123: Minor hardening and log hygiene
- Don’t log internal error text as
error.messagefield name; prefererror = %e(structured) to align with tracing conventions.- Clamp
set_timeoutto a sane upper bound and document units.- sess.set_timeout(u32::try_from(op_timeout.saturating_mul(1000)).unwrap_or(u32::MAX)); + let op_ms = op_timeout.saturating_mul(1000); + let op_ms = u32::try_from(op_ms).unwrap_or(u32::MAX / 2); + sess.set_timeout(op_ms);Also consider masking sensitive fields in spans; current fields exclude password — good. Keep it that way.
crates/err/src/app_error.rs (2)
9-9: Unify tracing macro usage for clarity.Use either fully qualified macros or bring both into scope. Recommend importing both and using
warn!consistently.-use tracing::error; +use tracing::{error, warn}; @@ - _ => { - tracing::warn!(error = ?self, status = %status, "Handled error occurred"); - } + _ => { + warn!(error = ?self, status = %status, "Handled error occurred"); + }Also applies to: 41-46
130-135: Consider 503 for bind failures.Startup bind errors usually indicate a temporary unavailability (port in use). Mapping to 503 may better reflect client-facing state.
- Self::BindError(_) => ( - StatusCode::INTERNAL_SERVER_ERROR, + Self::BindError(_) => ( + StatusCode::SERVICE_UNAVAILABLE, "bind_error", "Server startup failed", ),crates/env/src/loader.rs (2)
92-103: Avoid allocation in boolean parsing.Use
eq_ignore_ascii_caseinstead ofto_lowercase().- .and_then(|v| match v.to_lowercase().as_str() { - "true" | "1" | "yes" | "on" => Some(true), - "false" | "0" | "no" | "off" => Some(false), + .and_then(|v| { + let s = v.as_str(); + if s.eq_ignore_ascii_case("true") || s == "1" || s.eq_ignore_ascii_case("yes") || s.eq_ignore_ascii_case("on") { + Some(true) + } else if s.eq_ignore_ascii_case("false") || s == "0" || s.eq_ignore_ascii_case("no") || s.eq_ignore_ascii_case("off") { + Some(false) + } else { + debug!(key = %key, value = %v, "Invalid boolean value, using default"); + None + } - _ => { - debug!( - key = %key, - value = %v, - "Invalid boolean value, using default" - ); - None - } - }) + })
131-134: Remove unnecessaryunsafewrappers in tests.
std::env::set_var/remove_varare safe. Keepingunsafeis misleading.- unsafe { - std::env::remove_var(&self.key); - } + std::env::remove_var(&self.key); @@ - unsafe { - std::env::set_var(key, value); - } + std::env::set_var(key, value); @@ - unsafe { - std::env::remove_var("NONEXISTENT_VAR"); - } + std::env::remove_var("NONEXISTENT_VAR"); @@ - unsafe { - std::env::remove_var("MISSING_VAR"); - } + std::env::remove_var("MISSING_VAR"); @@ - unsafe { - std::env::remove_var("MISSING_NUM"); - } + std::env::remove_var("MISSING_NUM"); @@ - unsafe { - std::env::remove_var("MISSING_BOOL"); - } + std::env::remove_var("MISSING_BOOL");Also applies to: 139-145, 161-163, 185-187, 223-225, 264-266
crates/hlt/src/models.rs (3)
4-12: Consider derivingCopyforHealthStatus.It’s a small enum;
Copyremoves clone calls in hot paths.-#[derive(Serialize, Clone, Debug, PartialEq, Eq)] +#[derive(Serialize, Copy, Clone, Debug, PartialEq, Eq)] pub enum HealthStatus {
16-25: Optionally addDeserializefor round‑trips.If these types appear in request/response or tests,
Deserializehelps with round‑trip and fixtures.-#[derive(Serialize, Debug, Clone)] +#[derive(Serialize, Deserialize, Debug, Clone)] pub struct ComponentHealth { @@ -#[derive(Serialize, Debug)] +#[derive(Serialize, Deserialize, Debug)] pub struct SystemHealthResponse {Also applies to: 60-68
165-171: Make timestamp test a bit more tolerant in slow CI.Allow ±2–3s to avoid flakiness on congested runners.
- assert!((response.timestamp - now).abs() <= 1); + assert!((response.timestamp - now).abs() <= 3);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
Cargo.toml(1 hunks)Cargo.toml.backup(1 hunks)crates/env/Cargo.toml(1 hunks)crates/env/README.md(1 hunks)crates/env/src/lib.rs(1 hunks)crates/env/src/loader.rs(1 hunks)crates/env/tests/integration.rs(1 hunks)crates/err/Cargo.toml(1 hunks)crates/err/README.md(1 hunks)crates/err/src/app_error.rs(1 hunks)crates/err/src/domain/dbs.rs(1 hunks)crates/err/src/domain/env.rs(1 hunks)crates/err/src/domain/mod.rs(1 hunks)crates/err/src/domain/ssh.rs(1 hunks)crates/err/src/lib.rs(1 hunks)crates/err/tests/integration.rs(1 hunks)crates/hlt/Cargo.toml(1 hunks)crates/hlt/src/lib.rs(1 hunks)crates/hlt/src/models.rs(1 hunks)crates/hlt/src/registry.rs(1 hunks)crates/hlt/src/traits.rs(1 hunks)crates/hlt/tests/integration.rs(1 hunks)src/dbs/connector.rs(1 hunks)src/dbs/error.rs(0 hunks)src/dbs/health.rs(2 hunks)src/dbs/mod.rs(1 hunks)src/dbs/models.rs(1 hunks)src/err/README.md(0 hunks)src/err/error.rs(0 hunks)src/err/mod.rs(0 hunks)src/lib.rs(0 hunks)src/main.rs(2 hunks)src/rts/health.rs(1 hunks)src/rts/sshconnect.rs(1 hunks)src/ssh/connector.rs(2 hunks)src/ssh/error.rs(0 hunks)src/ssh/mod.rs(0 hunks)src/sys/config/server.rs(0 hunks)src/sys/config/state.rs(1 hunks)src/sys/env/README.md(0 hunks)src/sys/env/error.rs(0 hunks)src/sys/env/loader.rs(0 hunks)src/sys/env/mod.rs(0 hunks)src/sys/health/components.rs(0 hunks)src/sys/health/mod.rs(0 hunks)src/sys/health/models.rs(0 hunks)src/sys/health/traits.rs(0 hunks)src/sys/init.rs(2 hunks)src/sys/log/config.rs(0 hunks)src/sys/mod.rs(0 hunks)
💤 Files with no reviewable changes (18)
- src/sys/env/README.md
- src/sys/mod.rs
- src/sys/env/mod.rs
- src/ssh/mod.rs
- src/err/README.md
- src/sys/env/error.rs
- src/sys/health/traits.rs
- src/err/mod.rs
- src/sys/health/mod.rs
- src/sys/health/components.rs
- src/err/error.rs
- src/sys/env/loader.rs
- src/sys/config/server.rs
- src/ssh/error.rs
- src/sys/health/models.rs
- src/lib.rs
- src/dbs/error.rs
- src/sys/log/config.rs
🧰 Additional context used
🪛 LanguageTool
crates/err/README.md
[grammar] ~3-~3: Use a hyphen to join words.
Context: # err A comprehensive error handling crate for Axum-based applicatio...
(QB_NEW_EN_HYPHEN)
⏰ 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). (1)
- GitHub Check: Build & Test
🔇 Additional comments (26)
src/dbs/health.rs (1)
14-42: LGTM! Implementation correctly uses the timeout pattern.The health check implementation properly:
- Wraps the query in a timeout
- Returns structured
ComponentHealthvalues- Handles both query errors and timeouts with appropriate messages
crates/env/Cargo.toml (1)
6-14: LGTM! Workspace dependencies correctly configured.The manifest properly uses workspace-managed dependencies and includes appropriate dev-dependencies for testing.
src/dbs/connector.rs (1)
3-3: LGTM! Error import correctly updated to use theerrcrate.The import path has been properly migrated from the local error module to the centralized
errcrate.crates/err/Cargo.toml (1)
6-12: LGTM! Dependencies are appropriate for the error handling crate.The manifest correctly combines workspace-managed dependencies with crate-specific dependencies (
surrealdbfor database error conversion andthiserrorfor error derivation).src/dbs/models.rs (1)
35-35: LGTM! Test import correctly migrated to theerrcrate.The test module's import has been properly updated to reference
err::DatabaseError, consistent with the workspace refactoring.crates/hlt/Cargo.toml (1)
6-20: LGTM! Health check crate dependencies are well-structured.The manifest appropriately combines workspace-managed dependencies with health-check-specific dependencies (
async-traitfor trait implementations andfuturesfor async utilities).Cargo.toml.backup (1)
4-4: Invalid Rust edition specified.
edition = "2024"is not a valid Rust edition. The latest stable edition as of early 2025 is2021. If this was intended to be forward-looking, note that Rust 2024 edition has not been released yet.Apply this diff to use the correct edition:
-edition = "2024" +edition = "2021"Likely an incorrect or invalid review comment.
src/rts/sshconnect.rs (1)
3-3: LGTM! Clean migration to centralized error handling.The switch from
Result<_, AppError>toAppResult<_>improves consistency across the codebase while maintaining identical semantics.Also applies to: 11-11
src/dbs/mod.rs (1)
1-7: LGTM! Public API correctly streamlined.The removal of the local
errormodule andDatabaseErrorre-export aligns with the centralized error handling in theerrcrate. Consumers can now importDatabaseErrordirectly fromerr.src/main.rs (1)
2-3: LGTM! Consistent migration to centralized error handling.The main function signature and imports have been properly updated to use
AppResultfrom the centralizederrcrate, maintaining identical error handling behavior.Also applies to: 12-12
crates/env/README.md (1)
1-105: Well-documented crate with comprehensive examples.The README effectively covers the API surface, usage patterns, and error handling. The examples are clear and demonstrate all major features.
crates/hlt/src/traits.rs (2)
1-12: LGTM! Well-designed async trait with proper bounds.The
HealthChecktrait correctly usesasync_trait, includes appropriateSend + Syncbounds for concurrent usage, and provides clear documentation about non-blocking expectations.
14-60: Excellent test coverage with compile-time safety verification.The test suite validates both healthy and unhealthy outcomes, and includes a clever compile-time assertion (lines 56-59) ensuring
Box<dyn HealthCheck>isSend + Sync, which is critical for the concurrent health check registry.src/sys/config/state.rs (1)
1-9: LGTM! Clean migration to registry pattern.Replacing
Vec<Box<dyn HealthCheck>>withArc<HealthRegistry>provides a cleaner, more encapsulated API for health check management. The registry pattern better supports the centralized health monitoring architecture.crates/err/tests/integration.rs (5)
4-20: LGTM! Solid validation of AppResult type alias.The test correctly validates both success and error cases, including the ergonomic use of
.into()for error conversion.
22-39: LGTM! Excellent test of error propagation.This test validates the critical behavior that domain errors automatically convert to
AppErrorwhen using the?operator, which is essential for ergonomic error handling throughout the application.
41-54: LGTM! Comprehensive conversion testing.All domain error types (Database, Ssh, Environment) are validated to correctly convert into their respective
AppErrorvariants.
56-78: LGTM! HTTP status mapping correctly validated.The test verifies critical HTTP status code mappings: 404 for NotFound, 401 for AuthenticationError, 408 for Timeout, and 500 for environment errors.
80-86: LGTM! Error source chain verified.Testing that
AppErrorcorrectly implementssource()ensures proper error chain propagation for debugging and logging.crates/env/tests/integration.rs (1)
1-42: LGTM! Comprehensive integration test with proper concurrency control.The test correctly uses a static mutex to serialize environment modifications (preventing data races) and validates all five loader functions. The
unsafeblocks are properly used as required by thestd::envAPI, with the mutex ensuring thread safety.crates/env/src/lib.rs (1)
1-5: Env crate public API looks goodModule boundary and re-exports are clean; the
EnvResultalias is ergonomic. No changes requested.src/sys/init.rs (1)
31-42: Good use of nested?with timeoutMapping timeout to
AppErrorand then?-propagating inner errors is clean. No change needed.Ensure
connect(&config)returns an error type that converts intoAppError(e.g.,DatabaseError: Into<AppError>), otherwise the second?won’t compile.Cargo.toml (2)
4-4: Ensure CI/toolchain supports Rust 1.85.0 or later for Rust 2024 editionRust Edition 2024 is stable and can be used with Rust 1.85.0 and Cargo 1.85 or newer. Verify your CI/toolchain meets this minimum version requirement; otherwise fall back to 2021 edition.
35-36: Verify Rust 1.84+ is available in your CI environmentCargo's resolver = "3" was stabilized in Cargo 1.84 (Rust 1.84). Ensure your CI toolchain meets this minimum version requirement.
crates/env/src/loader.rs (1)
11-16: API surface looks solid.Functions are cohesive, signatures are ergonomic, and tests serialize env mutation correctly with a mutex and guards.
If there are integration tests in other crates that also mutate env, ensure they use a similar global lock to avoid cross-crate races during workspace
cargo test.Also applies to: 33-61, 68-81, 89-112
crates/hlt/src/models.rs (1)
70-105: Aggregation logic is clear and correct.Short‑circuit priority (Unhealthy > Degraded > Healthy) is implemented cleanly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dbs/models.rs (1)
39-45: Remove unnecessaryunsafeand stabilize env-mutation tests.
std::env::set_var/remove_varare safe; theunsafe { ... }blocks are unnecessary and will trigger clippy. Also, env is process‑global; parallel tests can race.Apply to drop
unsafe:- unsafe { - std_env::set_var("DB_ENDPOINT", "ws://test.com"); - std_env::set_var("DB_NAMESPACE", "ns_test"); - std_env::set_var("DB_NAME", "db_test"); - std_env::set_var("DB_USERNAME", "user_test"); - std_env::set_var("DB_PASSWORD", "pass_test"); - } + std_env::set_var("DB_ENDPOINT", "ws://test.com"); + std_env::set_var("DB_NAMESPACE", "ns_test"); + std_env::set_var("DB_NAME", "db_test"); + std_env::set_var("DB_USERNAME", "user_test"); + std_env::set_var("DB_PASSWORD", "pass_test");- unsafe { - std_env::remove_var("DB_ENDPOINT"); - std_env::remove_var("DB_NAMESPACE"); - std_env::remove_var("DB_NAME"); - std_env::remove_var("DB_USERNAME"); - std_env::remove_var("DB_PASSWORD"); - } + std_env::remove_var("DB_ENDPOINT"); + std_env::remove_var("DB_NAMESPACE"); + std_env::remove_var("DB_NAME"); + std_env::remove_var("DB_USERNAME"); + std_env::remove_var("DB_PASSWORD");Optionally serialize these tests (recommended):
- Add dev-dep serial_test and annotate: #[serial].
- Or use an RAII guard to save/restore env per test.
Also applies to: 51-56
♻️ Duplicate comments (2)
crates/hlt/src/registry.rs (1)
9-12: Send + Sync bounds correctly applied.The trait object bounds have been properly added as suggested in the previous review. This ensures thread-safety when
check_allis called from multiple tasks.crates/env/src/loader.rs (1)
47-60: Partial resolution: value still stored in error variant.While the debug log at line 51 has been commented out (addressing the immediate logging concern from the previous review), the raw value is still stored in the
EnvironmentError::Parsevariant at line 57. This value can still be leaked if the error is logged, displayed, or serialized elsewhere in the application.To fully resolve the security concern, consider one of these approaches:
- Update
EnvironmentError::Parseincrates/errto omit thevaluefield entirely- Store
"<redacted>"instead of the actual value- Add a separate boolean
redactfield to control whether the value should be includedNote: This follows up on the previous review comment about not logging raw environment values.
🧹 Nitpick comments (3)
crates/hlt/src/registry.rs (2)
9-21: Consider removing redundantDefaultderive.The struct derives
Default(line 9) but also provides an explicitnew()method (lines 17-21) that does the same thing. Sincenew()is more idiomatic and has the#[must_use]attribute, you could remove theDefaultderive.Apply this diff:
-#[derive(Default)] pub struct HealthRegistry { checkers: Vec<Box<dyn HealthCheck + Send + Sync>>, }
111-118: Consider documenting empty registry behavior.The test at line 116 shows that an empty registry returns
HealthStatus::Healthy. This is a reasonable design decision (no checks = no failures), but it would be helpful to document this behavior in the struct or method documentation.Add a note to the struct documentation:
/// Registry for managing and executing health checks. /// /// The registry maintains a collection of health checkers and provides /// methods to execute them concurrently and aggregate their results. +/// +/// An empty registry (with no registered checkers) is considered healthy. #[derive(Default)] pub struct HealthRegistry {src/dbs/connector.rs (1)
37-41: Optional: alias the env crate for clarity.To avoid confusion with
std::env, consider aliasing the externalenvcrate locally.Apply:
use err::{DatabaseError, EnvironmentError}; +use env as app_env; @@ - endpoint: env::get_required("DB_ENDPOINT")?, // EnvironmentError - namespace: env::get_required("DB_NAMESPACE")?, - database: env::get_required("DB_NAME")?, - username: env::get_required("DB_USERNAME")?, - password: env::get_required("DB_PASSWORD")?, + endpoint: app_env::get_required("DB_ENDPOINT")?, // EnvironmentError + namespace: app_env::get_required("DB_NAMESPACE")?, + database: app_env::get_required("DB_NAME")?, + username: app_env::get_required("DB_USERNAME")?, + password: app_env::get_required("DB_PASSWORD")?,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/env/src/loader.rs(1 hunks)crates/err/src/app_error.rs(1 hunks)crates/err/src/domain/env.rs(1 hunks)crates/hlt/src/registry.rs(1 hunks)src/dbs/connector.rs(2 hunks)src/dbs/models.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/err/src/domain/env.rs
- crates/err/src/app_error.rs
🔇 Additional comments (11)
crates/hlt/src/registry.rs (4)
23-26: LGTM!The registration method is straightforward and correctly accepts trait objects with the required bounds.
50-60: LGTM!The utility methods are correctly implemented with appropriate
#[must_use]attributes.
63-82: LGTM!The test mock is well-designed and correctly implements the
HealthChecktrait.
84-178: Excellent test coverage.The tests comprehensively cover all scenarios: empty registry, single status types, and mixed statuses. The assertions correctly validate the expected behavior.
crates/env/src/loader.rs (5)
1-4: LGTM: Clean, necessary imports.The imports are well-organized and appropriate for environment variable handling with error management and debug logging.
11-16: LGTM: Secure and correct implementation.The function correctly retrieves required environment variables and only logs the key (not the value), avoiding potential secret leakage.
22-31: LGTM: Proper default handling with good attribute usage.The
#[must_use]attribute is appropriate here, and the logging of key and default value is acceptable since defaults are typically non-sensitive constants.
68-81: LGTM: Safe default handling with proper type constraints.The function correctly handles parsing with defaults, and the debug logging only includes the key and default value, avoiding any potential secret leakage.
147-294: LGTM: Comprehensive and well-structured tests.The test suite provides excellent coverage:
- Success and failure paths for all functions
- Edge cases (missing vars, parse failures, invalid booleans)
- Multiple data types (u8, i32, f64, bool)
- 14 boolean format variations tested (case-insensitive, multiple representations)
- Proper synchronization and cleanup in all tests
src/dbs/connector.rs (1)
3-3: Import changes look good.Using err::{DatabaseError, EnvironmentError} aligns with the new workspace crates.
src/dbs/models.rs (1)
79-79: Verification complete—all variant names and import paths confirmed.The
EnvironmentError::NotFoundErrorvariant is correctly defined incrates/err/src/domain/env.rs:9and properly re-exported through theerrcrate's public API. The import at line 79 resolves correctly, and the assertion at lines 94-97 matches the enum definition.
Reference ENV_MUTEX in EnvVarGuard drop comment to explain why cleanup is safe. Update connector docs to state that EnvironmentError::NotFoundError is returned when a required environment variable is missing.
Introduce timeout() in the HealthCheck trait; HealthRegistry applies tokio::time::timeout per checker and warns on timeouts. Add tests for timeout behavior and different per-component timeouts. Update database and route health check implementations to provide timeouts. Remove unused .cargo config and tidy README files.
CLoaKY233
force-pushed
the
Crate-refactor
branch
from
8af69f9 to
f91ab63
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/dbs/connector.rs (1)
32-34: Doc comment still mentionsDatabaseError.The signature now returns
Result<Self, EnvironmentError>, yet the docs continue to referenceDatabaseError::ConfigError. Please rewrite the# Errorssection to call out the relevantEnvironmentErrorvariant(s) so the docs match the API.crates/err/src/app_error.rs (1)
42-49: Stop logging raw error structures.
error = ?selfstill dumps the fullAppErrorDebug tree, so anEnvironmentError::Parsewill leak the failing env value into logs—exactly the issue raised earlier. Please log only redacted fields (e.g., the tuple you already computed) instead of the full error object.- StatusCode::INTERNAL_SERVER_ERROR | StatusCode::SERVICE_UNAVAILABLE => { - error!(error = ?self, status = %status, "Critical error occurred"); - } - _ => { - tracing::warn!(error = ?self, status = %status, "Handled error occurred"); - } + StatusCode::INTERNAL_SERVER_ERROR | StatusCode::SERVICE_UNAVAILABLE => { + error!( + error_type = %error_type, + message = %message, + status = %status, + "Critical error occurred" + ); + } + _ => { + tracing::warn!( + error_type = %error_type, + message = %message, + status = %status, + "Handled error occurred" + ); + }
🧹 Nitpick comments (2)
crates/env/src/lib.rs (1)
1-4: Consider expanding the module documentation.The current documentation is functional but could be more comprehensive for discoverability. Consider adding:
- Overview of available functions (
get_required,get_parsed, etc.)- Error handling approach (
EnvResultandEnvironmentError)- Basic usage example
crates/hlt/tests/integration.rs (1)
128-131: Relax the timing assertion to avoid flakes.On a contended CI runner, ten sleepers can easily drift past 50 ms even when run concurrently, so this assertion will intermittently fail despite correct behaviour. Please give the check more headroom (or otherwise make it deterministic) so that we only fail when the registry actually runs sequentially.
- assert!(duration.as_millis() < 50); + assert!(duration < Duration::from_millis(95));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.cargo/config.toml(0 hunks)crates/env/README.md(1 hunks)crates/env/src/lib.rs(1 hunks)crates/env/src/loader.rs(1 hunks)crates/err/README.md(1 hunks)crates/err/src/app_error.rs(1 hunks)crates/err/src/domain/dbs.rs(1 hunks)crates/err/src/domain/env.rs(1 hunks)crates/err/src/domain/mod.rs(1 hunks)crates/err/src/domain/ssh.rs(1 hunks)crates/err/src/lib.rs(1 hunks)crates/hlt/README.md(1 hunks)crates/hlt/src/lib.rs(1 hunks)crates/hlt/src/models.rs(1 hunks)crates/hlt/src/registry.rs(1 hunks)crates/hlt/src/traits.rs(1 hunks)crates/hlt/tests/integration.rs(1 hunks)src/dbs/README.md(0 hunks)src/dbs/connector.rs(2 hunks)src/dbs/health.rs(1 hunks)src/rts/README.md(0 hunks)src/rts/health.rs(1 hunks)
💤 Files with no reviewable changes (3)
- src/dbs/README.md
- .cargo/config.toml
- src/rts/README.md
✅ Files skipped from review due to trivial changes (2)
- crates/hlt/README.md
- crates/err/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/err/src/lib.rs
- crates/env/README.md
- crates/hlt/src/registry.rs
- crates/hlt/src/lib.rs
- crates/env/src/loader.rs
- crates/err/src/domain/env.rs
- crates/hlt/src/models.rs
⏰ 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). (1)
- GitHub Check: Build & Test
🔇 Additional comments (5)
crates/env/src/lib.rs (2)
6-8: Clean API design with proper encapsulation.The private
loadermodule with selective public re-exports follows Rust best practices. Re-exportingEnvironmentErrorfrom the centralizederrcrate promotes consistency across the workspace.
10-11: Good ergonomic choice.The
EnvResult<T>type alias reduces boilerplate and improves readability throughout the crate, following standard Rust patterns.crates/err/src/domain/ssh.rs (1)
10-26: Nice coverage of SSH cases.Variants map cleanly onto the expected failure modes, and the Display messaging plus unit test look good.
crates/err/src/domain/mod.rs (1)
6-12: Re-export structure looks clean.The domain module now presents a tidy, discoverable surface for callers.
crates/err/src/domain/dbs.rs (1)
32-70: Database error mapping is thorough.Good to see the surrealdb conversion and HTTP status expectations verified in tests.
CLoaKY233
force-pushed
the
Crate-refactor
branch
from
628d48a to
fd7f9ba
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/dbs/connector.rs (1)
33-36: Doc comment still refers toDatabaseError.
DbConfig::from_envnow returnsResult<Self, EnvironmentError>, but the comment continues to promiseDatabaseError::ConfigError. Please update the# Errorssection so it lists the correctEnvironmentErrorvariant(s) emitted byenv::get_required, keeping the docs in sync with the signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/hlt/src/lib.rs(1 hunks)crates/hlt/src/models.rs(1 hunks)crates/hlt/src/registry.rs(1 hunks)crates/hlt/src/traits.rs(1 hunks)crates/hlt/tests/integration.rs(1 hunks)src/dbs/connector.rs(2 hunks)src/dbs/health.rs(1 hunks)src/rts/health.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rts/health.rs
- crates/hlt/src/traits.rs
⏰ 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). (1)
- GitHub Check: Build & Test
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
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 (4)
src/dbs/models.rs (2)
38-64: Test isolation is correct, but cleanup won't run if assertions fail.The
#[serial]attribute correctly prevents concurrent execution of tests that manipulate environment variables. However, if any assertion (lines 51-55) fails or panics, the cleanup code (lines 57-63) will never execute, leaving environment variables set and potentially causing test pollution.Consider using a guard pattern to ensure cleanup always runs:
#[test] #[serial] fn test_dbconfig_from_env_success() { + struct EnvGuard; + impl Drop for EnvGuard { + fn drop(&mut self) { + unsafe { + std_env::remove_var("DB_ENDPOINT"); + std_env::remove_var("DB_NAMESPACE"); + std_env::remove_var("DB_NAME"); + std_env::remove_var("DB_USERNAME"); + std_env::remove_var("DB_PASSWORD"); + } + } + } + let _guard = EnvGuard; + unsafe { std_env::set_var("DB_ENDPOINT", "ws://test.com"); std_env::set_var("DB_NAMESPACE", "ns_test"); std_env::set_var("DB_NAME", "db_test"); std_env::set_var("DB_USERNAME", "user_test"); std_env::set_var("DB_PASSWORD", "pass_test"); } let config = DbConfig::from_env().expect("Should create config successfully"); assert_eq!(config.endpoint, "ws://test.com"); assert_eq!(config.namespace, "ns_test"); assert_eq!(config.database, "db_test"); assert_eq!(config.username, "user_test"); assert_eq!(config.password, "pass_test"); - - unsafe { - std_env::remove_var("DB_ENDPOINT"); - std_env::remove_var("DB_NAMESPACE"); - std_env::remove_var("DB_NAME"); - std_env::remove_var("DB_USERNAME"); - std_env::remove_var("DB_PASSWORD"); - } }
66-101: Error migration looks correct, but cleanup reliability issue remains.The migration to
EnvironmentError::NotFoundError(lines 90-93) correctly aligns with the PR's centralized error handling in theerrcrate. The pre-cleanup (lines 71-77) is good practice to ensure a clean initial state. However, the same cleanup issue exists: if assertions fail, the final cleanup (lines 95-100) won't execute.Apply the same guard pattern as suggested for the first test:
#[test] #[serial] fn test_dbconfig_from_env_missing_variable() { use err::EnvironmentError; + + struct EnvGuard; + impl Drop for EnvGuard { + fn drop(&mut self) { + unsafe { + std_env::remove_var("DB_ENDPOINT"); + std_env::remove_var("DB_NAMESPACE"); + std_env::remove_var("DB_NAME"); + std_env::remove_var("DB_USERNAME"); + std_env::remove_var("DB_PASSWORD"); + } + } + } + let _guard = EnvGuard; unsafe { std_env::remove_var("DB_ENDPOINT"); std_env::remove_var("DB_NAMESPACE"); std_env::remove_var("DB_NAME"); std_env::remove_var("DB_USERNAME"); std_env::remove_var("DB_PASSWORD"); } unsafe { std_env::set_var("DB_ENDPOINT", "ws://test.com"); std_env::set_var("DB_NAMESPACE", "ns_test"); // DB_NAME is missing std_env::set_var("DB_USERNAME", "user_test"); std_env::set_var("DB_PASSWORD", "pass_test"); } let result = DbConfig::from_env(); assert!(result.is_err()); assert!(matches!( result.unwrap_err(), EnvironmentError::NotFoundError(_) )); - - unsafe { - std_env::remove_var("DB_ENDPOINT"); - std_env::remove_var("DB_NAMESPACE"); - std_env::remove_var("DB_USERNAME"); - std_env::remove_var("DB_PASSWORD"); - } }src/sys/init.rs (1)
31-31: Add missinguse env;import to src/sys/init.rs and other affected files.The code uses
env::get_parsed_or_default()at line 31, but theenvcrate is not imported. Verification shows this issue also affects:
src/sys/config/server.rssrc/dbs/connector.rssrc/dbs/health.rssrc/ssh/connector.rsAdd
use env;to the import section of each affected file.src/sys/config/server.rs (1)
11-12: Add missing import for theenvcrate.The code at lines 11-12 uses
env::get_or_default()andenv::get_parsed_or_default(), butenvis not in scope. The removal ofuse crate::sys::env;indicates thatenvwas refactored from a local module to a workspace crate (crates/env). Add the required import at the top of the file:use env;Alternatively, use the full path:
env::get_or_default()requires thatenvis in scope viause env;.
♻️ Duplicate comments (2)
crates/env/Cargo.toml (1)
4-4: Invalid Rust edition specified.
edition = "2024"is not a valid Rust edition. The latest stable edition is2021. This issue was previously flagged and remains unresolved.Apply this diff:
-edition = "2024" +edition = "2021"crates/env/src/loader.rs (1)
43-61: Secrets still leak via error value field.While the debug log at line 51 is now commented out, line 57 still passes the raw
valuetoEnvironmentError::Parse. If this error is logged, returned in an API response, or serialized anywhere in the application, secrets will leak.To fully address the security concern from previous reviews, the error type should not store the raw value. Consider one of these approaches:
- Store a redacted placeholder instead:
EnvironmentError::Parse { key: key.to_string(), - value: value.clone(), + value: "<redacted>".to_string(), type_name: std::any::type_name::<T>(), }
- Remove the value field entirely from
EnvironmentError::Parse(requires changes incrates/err).The first approach is safer and maintains backward compatibility with error handling code.
🧹 Nitpick comments (3)
crates/env/tests/integration.rs (1)
4-38: LGTM! Well-structured integration test.The test comprehensively exercises the public API with proper serialization and cleanup. The use of
unsafefor environment variable manipulation is acceptable in test code when properly synchronized with#[serial].Consider adding negative test cases (e.g., testing
get_requiredwith a missing variable, orget_parsedwith an invalid value) to improve coverage of error paths.Cargo.toml (2)
59-64: Aggressive release optimizations may impact build times.The release profile is configured for maximum size optimization:
opt-level = "z"prioritizes binary size over speedlto = trueandcodegen-units = 1significantly increase compile timespanic = "abort"eliminates unwinding supportThese settings are appropriate for production binaries where size matters, but be aware of the trade-offs: slower release builds and potential performance regression compared to
opt-level = 3.
45-56: Dependency versions are generally current; tokio has an optional minor update available.Verification shows most workspace dependencies are up-to-date as of November 2025. Serde (1.0.228) and tracing (0.1.41) match the latest stable releases. Tokio (1.47.1) is one patch version behind the latest (1.48.0)—consider updating if the patch includes relevant fixes, though this is optional and not critical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.toml(1 hunks)crates/env/Cargo.toml(1 hunks)crates/env/src/loader.rs(1 hunks)crates/env/tests/integration.rs(1 hunks)crates/log/Cargo.toml(1 hunks)crates/log/src/config.rs(6 hunks)crates/log/src/init.rs(1 hunks)crates/log/src/lib.rs(1 hunks)crates/log/src/models.rs(1 hunks)src/dbs/models.rs(3 hunks)src/lib.rs(1 hunks)src/sys/config/server.rs(2 hunks)src/sys/init.rs(2 hunks)src/sys/log/mod.rs(0 hunks)src/sys/mod.rs(0 hunks)
💤 Files with no reviewable changes (2)
- src/sys/mod.rs
- src/sys/log/mod.rs
⏰ 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). (1)
- GitHub Check: Build & Test
🔇 Additional comments (16)
src/dbs/models.rs (1)
35-35: LGTM! Appropriate dependency for test isolation.The
serial_testimport is necessary for serializing tests that manipulate global environment variables, preventing race conditions between concurrent test executions.src/sys/config/server.rs (1)
27-27: LGTM: Proper serialization of environment-dependent tests.The use of
#[serial]ensures that tests manipulating environment variables do not interfere with each other during parallel execution.Also applies to: 30-30, 46-46
src/sys/init.rs (1)
126-136: LGTM: Clean health check registry refactor.The transition from direct health checkers to a
HealthRegistrypattern improves modularity and provides a clearer registration API. The use ofArc<HealthRegistry>is appropriate for shared state.crates/log/src/models.rs (1)
5-5: LGTM: Clean enum extension.Adding the
Prettyvariant toLogFormatis straightforward and provides a new formatting option.crates/log/src/lib.rs (1)
1-5: LGTM: Clear module organization.The module structure and re-export of
init_tracingprovide a clean public API for the logging crate.crates/log/src/init.rs (2)
1-2: LGTM: Clearer import paths.The change from
super::modelstocrate::modelsand the explicituse tracing;import improve clarity.
9-18: LGTM: Consistent Pretty format implementation.The
Prettyformat branch follows the same pattern as the existingJsonandCompactbranches, with appropriate formatting options enabled (target, line numbers, file, level).src/lib.rs (1)
8-8: Review comment is based on incorrect assumptions about the codebase.The review claims
init_tracingis no longer re-exported fromsysand thatAppErrorwas removed from the public surface. However:
init_tracingwas never part of the public API. It's used internally withinsys/init.rsbut imported from thelogcrate.AppErroris never exported from the main crate's public API. It comes from theerrworkspace crate.- No workspace members depend on the main crate; they all depend on sibling crates like
errandlog.These are internal implementation details, not breaking API changes. There are no external or internal consumers to break.
Likely an incorrect or invalid review comment.
crates/log/src/config.rs (3)
5-5: Good addition of#[must_use]attributes.Adding
#[must_use]tofrom_env()methods is excellent practice, ensuring that configuration objects are not inadvertently discarded.Also applies to: 26-26
12-12: LGTM! Pretty format support is well-integrated.The new
Prettyvariant is consistently implemented with proper test coverage following the established pattern.Also applies to: 76-85
46-46: Excellent addition of test serialization.Properly applying
#[serial]to all tests that manipulate environment variables prevents race conditions and ensures reliable test execution.Also applies to: 49-49, 62-62, 75-75, 88-88, 102-102
crates/env/src/loader.rs (4)
13-18: LGTM! Clean implementation.The function correctly handles missing variables without exposing any sensitive information in logs.
68-81: LGTM! Secure implementation with proper logging.The function correctly redacts values and only logs the key and default, preventing any potential secret leakage.
89-112: LGTM! Previous logging concern has been addressed.The raw value logging at line 98 has been properly commented out, and the function securely handles boolean parsing without exposing sensitive values.
114-317: Excellent test coverage with proper isolation.The test suite is comprehensive, covering:
- Success and failure paths for all functions
- Edge cases (invalid values, type variations, boolean formats)
- Proper serialization with
#[serial]to prevent race conditions- Clean setup and teardown with
unsafeblocks (appropriate in tests)The test quality significantly strengthens confidence in the implementation.
Cargo.toml (1)
39-41: Consider downgrading toresolver = "2"unless resolver 3 features are explicitly required.Rust 2024 was released alongside Rust 1.85 in February 2025. The workspace specifies
resolver = "3", which typically benefits scenarios with multiple workspace members having different MSRVs or complex path dependencies. However:
- No workspace members define
rust-version(MSRV)- No path dependencies exist with differing version specifications
- All workspace members share uniform dependency versions
If MSRV-aware resolution or complex dependency scenarios aren't part of your project plan,
resolver = "2"would provide broader compatibility with older Rust toolchains while maintaining full functionality for this workspace structure. Keepresolver = "3"only if you specifically need its MSRV resolution capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
crates/hlt/src/registry.rs (2)
10-14: Send + Sync bounds correctly applied.The
HealthRegistrynow storesBox<dyn HealthCheck + Send + Sync>, addressing the prior review concern about thread-safety guarantees.
38-57: Timeout handling with component identity preservation implemented correctly.The implementation captures
checker.name()before the async block and uses per-component timeouts viachecker.timeout(). On timeout, the component name is correctly preserved in the unhealthy response. This addresses both prior concerns about timeout protection and component identity.src/dbs/health.rs (1)
1-4: Missing import forenvcrate.Line 46 uses
env::get_parsed_or_defaultbut theenvcrate is not imported. Add the missing import:use crate::dbs::Database; +use env; use hlt::{ComponentHealth, HealthCheck, HealthStatus}; use tokio::time::{Duration, Instant}; use tracing::{debug, warn};
🧹 Nitpick comments (2)
crates/hlt/src/traits.rs (1)
21-21: Remove leftover development comment from doc example.The
"// ✅ Add this"comment appears to be a leftover from development and should be removed from the public documentation.-/// fn name(&self) -> &'static str { // ✅ Add this +/// fn name(&self) -> &'static str {src/dbs/health.rs (1)
45-48: Consider making the timeout configurable via a struct field instead.Reading from environment on every
timeout()call adds overhead. Consider caching the timeout value in theDatabasestruct during construction, or document that this is intentional for dynamic reconfiguration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/hlt/src/registry.rs(1 hunks)crates/hlt/src/traits.rs(1 hunks)crates/hlt/tests/integration.rs(1 hunks)src/dbs/health.rs(1 hunks)src/rts/health.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/hlt/src/traits.rs (2)
src/rts/health.rs (2)
name(49-51)check(53-57)crates/hlt/src/models.rs (5)
healthy(42-53)None(196-196)unhealthy(72-83)status(116-119)message(123-126)
crates/hlt/tests/integration.rs (4)
crates/hlt/src/registry.rs (8)
name(96-98)name(224-226)check(100-107)check(228-231)None(230-230)timeout(109-111)timeout(233-235)new(19-23)crates/hlt/src/traits.rs (10)
name(43-43)name(60-62)name(77-79)check(46-46)check(64-66)check(81-83)None(65-65)timeout(49-49)timeout(68-70)timeout(85-87)src/dbs/health.rs (3)
name(9-11)check(16-43)timeout(45-48)crates/hlt/src/models.rs (10)
unhealthy(72-83)None(196-196)healthy(42-53)degraded(57-68)latency_ms(130-133)new(105-112)new(161-170)status(116-119)message(123-126)builder(87-89)
🔇 Additional comments (10)
crates/hlt/src/traits.rs (2)
37-50: Well-designed trait with appropriate thread-safety bounds.The
HealthChecktrait correctly requiresSend + Syncsupertraits, enabling safe concurrent execution in the registry. The three-method contract (name,check,timeout) provides a clean abstraction for health checks with per-component timeout configuration.
52-116: Comprehensive test coverage for the trait.The tests validate healthy/unhealthy paths and include a compile-time assertion for
Send + SynconBox<dyn HealthCheck>, which is essential for the concurrent registry design.crates/hlt/tests/integration.rs (4)
125-133: Timing assertion relaxed appropriately.The threshold has been increased to 150ms (from the previous stricter value), addressing the CI flakiness concern from the prior review.
169-191: Thread safety test validates concurrent access.Using
Arcto share the registry across multiple spawned tasks correctly exercises theSend + Syncguarantees of theHealthRegistryand its stored checkers.
193-231: Timeout enforcement test validates component identity preservation.The test correctly verifies that when a check times out, the component name is preserved (
"TimeoutTest") and the message indicates the timeout. This addresses the prior concern about losing component identity on timeout.
281-332: Comprehensive builder pattern test with multiple latency thresholds.Good coverage of the builder API across healthy, degraded, and unhealthy states based on latency thresholds.
src/rts/health.rs (2)
15-35: Clean delegation to the health registry with appropriate HTTP status mapping.The handler correctly delegates to
health_registry.check_all()and maps statuses to HTTP codes. The logging at appropriate levels (debug for healthy, warn for degraded/unhealthy) provides good observability.
37-121: Good test coverage for the handler's status mapping logic.Tests validate the HTTP status derivation logic and registry behavior with multiple component statuses.
crates/hlt/src/registry.rs (1)
218-250: Excellent timeout test with SlowChecker.The test validates that a slow check (10s sleep with 100ms timeout) correctly produces an unhealthy response with the component name preserved and a "timed out" message.
src/dbs/health.rs (1)
8-43: Clean implementation of the HealthCheck trait.The
name()method, builder pattern usage, and latency tracking are well-implemented. The#[allow(clippy::cast_possible_truncation)]is appropriately justified in the comment.
This pull request introduces a Rust workspace structure and adds two new crates:
envfor environment variable management anderrfor error handling. It refactors dependency management to use workspace dependencies, and provides comprehensive documentation and tests for both new crates. The changes improve modularity, type safety, error handling, and maintainability across the codebase.Workspace setup and dependency management:
Cargo.tomlto define a workspace with shared dependencies and addedcrates/env,crates/err, andcrates/hltas workspace members. Moved common dependencies to[workspace.dependencies]for consistency and easier management.Cargo.tomlasCargo.toml.backupfor reference and migration safety.envcrate (environment variable utilities):envcrate with type-safe, error-aware environment variable loading functions (get_required,get_or_default,get_parsed,get_parsed_or_default,get_bool) and thorough unit/integration tests. [1] [2] [3] [4]README.mdfor theenvcrate, documenting features, usage patterns, API reference, error handling, and testing instructions.errcrate (error handling):errcrate with domain-specific error types (AppError,DatabaseError,SshError,EnvironmentError), automatic Axum HTTP response conversion, logging integration, and a unifiedResulttype alias.README.mdfor theerrcrate, covering error types, usage examples, HTTP status mapping, logging, and testing.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.