feat: surface the swallowed errors across the fleet#3469
Conversation
|
@coderabbitai full_review, thanks! |
|
(ᵘ‿ᵘ)♡ ✅ Action performedFull review finished. |
|
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:
Summary by CodeRabbit
WalkthroughThe changes make previously ignored rollback, cleanup, filesystem, configuration, and subprocess failures observable. They add health-report persistence telemetry, warn on empty DPU serial numbers, and ensure BMC reset metrics count only successful resets. ChangesAPI transaction rollback observability
Runtime failure diagnostics
Health report persistence telemetry
BMC reset outcome accounting
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
crates/site-explorer/src/lib.rs (1)
3938-3969: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the outcome test table-driven and cover both transports.
This test only exercises
Ipmitool, manually repeats the two cases, and does not verify the new failure log fields. Add table-driven cases for bothIpmitoolandRedfish, asserting the returned boolean, metric count, and logged method/address/error.As per coding guidelines, Rust tests for functions mapping inputs to observable results should use table-driven cases.
🤖 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 `@crates/site-explorer/src/lib.rs` around lines 3938 - 3969, Update bmc_reset_counter_counts_successes_not_attempts to use table-driven cases covering successful and failed outcomes for both BmcResetMethod::Ipmitool and BmcResetMethod::Redfish. For each case, assert the returned boolean, bmc_reset_count, and failure log fields including method, address, and error; retain the success behavior and isolate or reset metrics/log capture per case.Source: Coding guidelines
crates/preingestion-manager/src/lib.rs (1)
2160-2191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated kill/wait/warn cleanup sequence.
The stdout-missing and stderr-missing branches duplicate the same 4-statement kill → wait → warn cleanup verbatim. Consider a small helper to avoid drift if this cleanup logic changes later.
♻️ Proposed helper extraction
+async fn kill_and_reap(cmd: &mut tokio::process::Child, address: &str, context: &str) { + if let Err(e) = cmd.kill().await { + tracing::warn!("Upgrade script {address} cleanup: failed to kill subprocess after {context}: {e}"); + } + if let Err(e) = cmd.wait().await { + tracing::warn!("Upgrade script {address} cleanup: failed to reap subprocess after {context}: {e}"); + } +}Then call
kill_and_reap(&mut cmd, &address, "STDOUT capture failure").await;andkill_and_reap(&mut cmd, &address, "STDERR capture failure").await;at the two call sites.🤖 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 `@crates/preingestion-manager/src/lib.rs` around lines 2160 - 2191, Extract the duplicated subprocess cleanup from the stdout and stderr failure branches into a small async helper, such as kill_and_reap, accepting the command, address, and failure context. Preserve the existing kill-then-wait order and warning messages, then call the helper from both branches with their respective capture-failure descriptions before marking completion and returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dsx-exchange-consumer/tests/metric_exposition.rs`:
- Around line 79-84: Update the test documentation comment above the
persist-failure metric test to use the complete metric name
carbide_dsx_exchange_consumer_health_report_persist_failures_total, matching the
event definition and assertion; leave the test behavior unchanged.
In `@crates/site-explorer/src/lib.rs`:
- Around line 2565-2567: Update the documentation for
carbide_site_explorer_bmc_reset_count and all dependent dashboards or alerts to
define and interpret the metric as the number of successfully completed BMC
resets, not initiated attempts. Locate references using the metric name and
revise labels, descriptions, queries, and thresholds consistently without
changing the implementation.
- Around line 2568-2581: Separate the remote BMC reset result from timestamp
persistence errors in redfish_reset_bmc and ipmitool_reset_bmc, and update
record_bmc_reset_outcome and their callers accordingly. Treat a successful
physical reset as success for metrics and control flow even when timestamp
persistence fails, while still surfacing the bookkeeping error; ensure the
Redfish path does not fall through to a second IPMI reset after the hardware
reset succeeds.
---
Nitpick comments:
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 2160-2191: Extract the duplicated subprocess cleanup from the
stdout and stderr failure branches into a small async helper, such as
kill_and_reap, accepting the command, address, and failure context. Preserve the
existing kill-then-wait order and warning messages, then call the helper from
both branches with their respective capture-failure descriptions before marking
completion and returning.
In `@crates/site-explorer/src/lib.rs`:
- Around line 3938-3969: Update bmc_reset_counter_counts_successes_not_attempts
to use table-driven cases covering successful and failed outcomes for both
BmcResetMethod::Ipmitool and BmcResetMethod::Redfish. For each case, assert the
returned boolean, bmc_reset_count, and failure log fields including method,
address, and error; retain the success behavior and isolate or reset metrics/log
capture per case.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dba974ad-5c4a-49e9-bfb5-3acecfab5634
📒 Files selected for processing (15)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/dhcp/src/lib.rs (1)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s structured logging contract for the new warnings.
These changes introduce free-form interpolation of errors, paths, and endpoint identifiers. Use structured
tracing::warn!fields consistently.
crates/dhcp/src/lib.rs#L78-L98: replace the threelog::warn!calls with structured fields and describeVarErroraccurately.crates/preingestion-manager/src/lib.rs#L2162-L2187: emitaddressanderrorfields for cleanup warnings.crates/preingestion-manager/src/lib.rs#L2206-L2210: emitaddressanderrorfields for the stderr-task warning.As per coding guidelines and path instructions, ordinary Rust logging should use structured
tracing::macros with common values passed as fields.🤖 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 `@crates/dhcp/src/lib.rs` around lines 78 - 98, Replace the three log::warn! calls in crates/dhcp/src/lib.rs lines 78-98 with structured tracing::warn! fields, recording the VarError accurately alongside the relevant fallback path. In crates/preingestion-manager/src/lib.rs lines 2162-2187, update cleanup warnings to emit structured address and error fields; likewise update the stderr-task warning at lines 2206-2210 to emit address and error fields. No direct changes are needed outside these three sites.Sources: Coding guidelines, Path instructions
crates/site-explorer/src/lib.rs (2)
3938-3967: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the physical-success/bookkeeping-failure path.
This test only feeds
record_bmc_reset_outcomepre-composed results; it cannot catch a successful remote reset followed by timestamp persistence failure. Add coverage ensuring that success is counted and the IPMI fallback is not triggered.🤖 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 `@crates/site-explorer/src/lib.rs` around lines 3938 - 3967, Add a test covering the real BMC reset flow where the remote reset succeeds but timestamp persistence fails: verify the successful reset increments bmc_reset_count and does not trigger the IPMI fallback. Exercise the higher-level reset method rather than only record_bmc_reset_outcome, and use the existing persistence-failure setup and fallback-observable symbols.
3943-3967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s table-driven test pattern.
The test maps outcomes to a boolean and counter state but encodes cases sequentially. Convert these cases to the existing
check_cases/scenario style. As per coding guidelines, tests mapping inputs to outputs or observable results must be table-driven.🤖 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 `@crates/site-explorer/src/lib.rs` around lines 3943 - 3967, Convert bmc_reset_counter_counts_successes_not_attempts into the repository’s table-driven check_cases/scenario pattern, defining success and failure inputs with their expected boolean result and bmc_reset_count. Iterate through the cases while preserving the existing record_bmc_reset_outcome assertions and expected counter behavior.Source: Coding guidelines
crates/dsx-exchange-consumer/src/health_updater.rs (1)
215-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a handler-level assertion for the new emission.
test_api_failure_does_not_cache_stateverifies only cache behavior, whilemetric_exposition.rsemitsHealthReportPersistFaileddirectly. Capture the failure-path log aroundhandle_value_messageand assert the rack ID and error; otherwise this test suite can pass even if this call site stops emitting the event.🤖 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 `@crates/dsx-exchange-consumer/src/health_updater.rs` around lines 215 - 222, Update test_api_failure_does_not_cache_state around handle_value_message to capture the handler-level HealthReportPersistFailed emission, then assert that the captured event contains the expected rack ID and persistence error. Keep the existing cache-behavior assertions and ensure the test specifically covers this call site rather than only metric_exposition.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/health/src/sink/log_file.rs`:
- Around line 236-254: Update shift_backups to avoid using Path::exists() before
rotating or pruning files. Call fs::rename and fs::remove_file directly,
treating only ErrorKind::NotFound as an ignorable absence and logging other
filesystem errors through the existing tracing warnings.
- Around line 211-215: The rotate_if_needed() flush path must stop rotation when
file.flush() fails instead of dropping the handle and continuing. Preserve
self.current_file on failure, propagate the I/O error immediately, and only
allow the existing ownership transfer/drop behavior after a successful flush.
---
Nitpick comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Replace the three log::warn! calls in
crates/dhcp/src/lib.rs lines 78-98 with structured tracing::warn! fields,
recording the VarError accurately alongside the relevant fallback path. In
crates/preingestion-manager/src/lib.rs lines 2162-2187, update cleanup warnings
to emit structured address and error fields; likewise update the stderr-task
warning at lines 2206-2210 to emit address and error fields. No direct changes
are needed outside these three sites.
In `@crates/dsx-exchange-consumer/src/health_updater.rs`:
- Around line 215-222: Update test_api_failure_does_not_cache_state around
handle_value_message to capture the handler-level HealthReportPersistFailed
emission, then assert that the captured event contains the expected rack ID and
persistence error. Keep the existing cache-behavior assertions and ensure the
test specifically covers this call site rather than only metric_exposition.rs.
In `@crates/site-explorer/src/lib.rs`:
- Around line 3938-3967: Add a test covering the real BMC reset flow where the
remote reset succeeds but timestamp persistence fails: verify the successful
reset increments bmc_reset_count and does not trigger the IPMI fallback.
Exercise the higher-level reset method rather than only
record_bmc_reset_outcome, and use the existing persistence-failure setup and
fallback-observable symbols.
- Around line 3943-3967: Convert bmc_reset_counter_counts_successes_not_attempts
into the repository’s table-driven check_cases/scenario pattern, defining
success and failure inputs with their expected boolean result and
bmc_reset_count. Iterate through the cases while preserving the existing
record_bmc_reset_outcome assertions and expected counter behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e1d2c76-325f-4155-ab95-ce22450149af
📒 Files selected for processing (15)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rs
| if let Some(mut file) = self.current_file.take() | ||
| && let Err(e) = file.flush() | ||
| { | ||
| tracing::warn!(error = %e, "failed to flush log file before rotation"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Map the file shape first, then inspect the relevant section.
ast-grep outline crates/health/src/sink/log_file.rs --view expanded || true
echo
wc -l crates/health/src/sink/log_file.rs
echo
sed -n '170,290p' crates/health/src/sink/log_file.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 7202
🏁 Script executed:
set -euo pipefail
# Inspect the call path and any rotation tests.
sed -n '1,170p' crates/health/src/sink/log_file.rs | cat -n
echo
sed -n '300,560p' crates/health/src/sink/log_file.rs | cat -n
echo
rg -n "rotate_if_needed|write_line\\(" crates/health/src/sink/log_file.rs crates/health/src -g '!target'Repository: NVIDIA/infra-controller
Length of output: 18896
Do not continue rotation after a flush error.
rotate_if_needed() takes ownership of self.current_file before checking flush(). If that flush fails, the handle is dropped and rotation still proceeds, which can strand any pending bytes and mask the underlying I/O failure. Keep the writer alive, return the error, and only drop it after a successful flush.
🤖 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 `@crates/health/src/sink/log_file.rs` around lines 211 - 215, The
rotate_if_needed() flush path must stop rotation when file.flush() fails instead
of dropping the handle and continuing. Preserve self.current_file on failure,
propagate the I/O error immediately, and only allow the existing ownership
transfer/drop behavior after a successful flush.
| if from.exists() | ||
| && let Err(e) = fs::rename(&from, &to) | ||
| { | ||
| tracing::warn!(error = %e, from = %from.display(), to = %to.display(), "failed to shift rotated log file"); | ||
| } | ||
| } | ||
|
|
||
| // current -> .1 | ||
| let backup = self.rotated_path(1); | ||
| let _ = fs::rename(current_path, &backup); | ||
| if let Err(e) = fs::rename(current_path, &backup) { | ||
| tracing::warn!(error = %e, from = %current_path.display(), to = %backup.display(), "failed to rotate current log file to backup"); | ||
| } | ||
|
|
||
| // prune the oldest backup beyond the limit | ||
| let oldest = self.rotated_path(self.max_backups + 1); | ||
| if oldest.exists() { | ||
| let _ = fs::remove_file(&oldest); | ||
| if oldest.exists() | ||
| && let Err(e) = fs::remove_file(&oldest) | ||
| { | ||
| tracing::warn!(error = %e, path = %oldest.display(), "failed to prune oldest rotated log file"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='crates/health/src/sink/log_file.rs'
echo "== file size =="
wc -l "$file"
echo "== relevant slice =="
sed -n '200,290p' "$file"
echo "== search exists/try_exists in file =="
rg -n 'try_exists|\.exists\(\)|rename\(|remove_file\(' "$file"Repository: NVIDIA/infra-controller
Length of output: 4000
Avoid gating backup rotation on Path::exists()
exists() turns permission and other metadata failures into false, so shift_backups() can skip a real error without logging it. Call rename/remove_file directly and ignore only NotFound, or switch to try_exists() with explicit Err handling.
🤖 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 `@crates/health/src/sink/log_file.rs` around lines 236 - 254, Update
shift_backups to avoid using Path::exists() before rotating or pruning files.
Call fs::rename and fs::remove_file directly, treating only ErrorKind::NotFound
as an ignorable absence and logging other filesystem errors through the existing
tracing warnings.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3469.docs.buildwithfern.com/infra-controller |
|
Thanks @coderabbitai! Addressed all four actionable items in the latest push:
On the reset-outcome finding — a physical reset that succeeds but whose timestamp write fails is counted as a failure and, on the Redfish path, falls through to a second IPMI reset — that's a real issue, but it's pre-existing: this PR only swapped the counting mechanism, not the reset methods' |
|
@coderabbitai full_review, thanks! |
|
Confirmed my understanding of the addressed items:
Also, good call filing ᕕ( ᐛ )ᕗ ✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
Your call on the reset-outcome issue (timestamp-write failure being counted as a reset failure, and the Redfish→IPMI fall-through) makes sense — since this PR only changed how successes/failures are counted rather than the underlying (๑˃̵ᴗ˂̵)و |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/dhcp/src/lib.rs (1)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured
tracingfields for the new operational diagnostics.These messages interpolate errors, paths, addresses, and cleanup context into free-form text; DHCP also uses
log::warn!. Keep messages static and pass values as fields such aserror = %e,fallback = %path,address = %address, andcontext.
crates/dhcp/src/lib.rs#L78-L98: replace eachlog::warn!with structuredtracing::warn!.crates/preingestion-manager/src/lib.rs#L2168-L2176: structureaddress,context, and cleanup errors.crates/preingestion-manager/src/lib.rs#L2208-L2211: structure the endpoint and join error.As per coding guidelines and path instructions, ordinary Rust logging should use structured
tracing::macros with common values passed as fields.🤖 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 `@crates/dhcp/src/lib.rs` around lines 78 - 98, Replace the three log::warn! calls in crates/dhcp/src/lib.rs lines 78-98 with tracing::warn! calls using static messages and structured fields for the environment-variable errors and fallback paths. In crates/preingestion-manager/src/lib.rs lines 2168-2176, structure address, context, and cleanup errors; in lines 2208-2211, structure the endpoint and join error. Preserve the existing warning content and fallback behavior while using fields such as error = %e, fallback = %path, address = %address, and context.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dsx-exchange-consumer/src/health_updater.rs`:
- Around line 215-222: Extend test_api_failure_does_not_cache_state to capture
the HealthReportPersistFailed telemetry emitted by the Err branch, then assert
the WARN message, matching rack_id, persist error text, and expected counter
increment. Keep the existing assertion that failed state is not cached.
---
Nitpick comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Replace the three log::warn! calls in
crates/dhcp/src/lib.rs lines 78-98 with tracing::warn! calls using static
messages and structured fields for the environment-variable errors and fallback
paths. In crates/preingestion-manager/src/lib.rs lines 2168-2176, structure
address, context, and cleanup errors; in lines 2208-2211, structure the endpoint
and join error. Preserve the existing warning content and fallback behavior
while using fields such as error = %e, fallback = %path, address = %address, and
context.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2475c6ec-8ada-491c-9f8f-42b7e4b781aa
📒 Files selected for processing (17)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
|
Thanks @coderabbitai — on the structured-logging finding:
|
|
@coderabbitai full_review, thanks! |
|
ヽ(´▽`)/ ✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
Nothing further from me on this thread. ᕦ(ò_óˇ)ᕤ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Update the TLS path configuration fields
forge_root_ca_path, forge_client_cert_path, and forge_client_key_path to reject
empty values returned by std::env::var, accepting only non-empty paths and
falling back to the corresponding tls_default constants with the existing
warnings. Apply identical validation to all three paths before they reach
build_forge_client_config.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 64b1080c-9343-4c87-95a2-995f6e47aab9
📒 Files selected for processing (17)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/api-core/src/handlers/credential.rs
- crates/site-explorer/src/metrics.rs
- crates/api-core/src/handlers/expected_machine.rs
- crates/api-core/src/handlers/rack.rs
- crates/machine-controller/src/handler/dpf.rs
- crates/dsx-exchange-consumer/src/health_updater.rs
- crates/health/src/sink/log_file.rs
- crates/preingestion-manager/src/lib.rs
- crates/api-core/src/handlers/switch.rs
- crates/dsx-exchange-consumer/src/metrics.rs
- crates/api-core/src/handlers/instance.rs
- crates/dsx-exchange-consumer/Cargo.toml
- crates/site-explorer/src/lib.rs
| forge_root_ca_path: std::env::var("FORGE_ROOT_CAFILE_PATH").unwrap_or_else(|e| { | ||
| log::warn!( | ||
| "FORGE_ROOT_CAFILE_PATH unset or unreadable ({e}); falling back to the built-in default root CA at {}", | ||
| tls_default::ROOT_CA | ||
| ); | ||
| tls_default::ROOT_CA.to_string() | ||
| }), | ||
| forge_client_cert_path: std::env::var("FORGE_CLIENT_CERT_PATH").unwrap_or_else(|e| { | ||
| log::warn!( | ||
| "FORGE_CLIENT_CERT_PATH unset or unreadable ({e}); falling back to the built-in default client certificate at {}", | ||
| tls_default::CLIENT_CERT | ||
| ); | ||
| tls_default::CLIENT_CERT.to_string() | ||
| }), | ||
| forge_client_key_path: std::env::var("FORGE_CLIENT_KEY_PATH").unwrap_or_else(|e| { | ||
| log::warn!( | ||
| "FORGE_CLIENT_KEY_PATH unset or unreadable ({e}); falling back to the built-in default client key at {}", | ||
| tls_default::CLIENT_KEY | ||
| ); | ||
| tls_default::CLIENT_KEY.to_string() | ||
| }), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat empty TLS-path environment variables as invalid.
std::env::var returns Ok("") for an explicitly empty variable, so these branches skip the fallback and pass empty certificate/key paths to build_forge_client_config. Validate the returned paths before accepting them, and use the built-in defaults when they are empty.
Proposed approach
- forge_root_ca_path: std::env::var("FORGE_ROOT_CAFILE_PATH").unwrap_or_else(|e| {
+ forge_root_ca_path: match std::env::var("FORGE_ROOT_CAFILE_PATH") {
+ Ok(path) if !path.is_empty() => path,
+ Ok(_) | Err(_) => {
+ // log the appropriate reason, then:
+ tls_default::ROOT_CA.to_string()
+ }
+ },Apply the same validation to the client certificate and key paths.
As per path instructions, DHCP TLS-path fallbacks must be reviewed for configuration correctness and runtime behavior.
🤖 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 `@crates/dhcp/src/lib.rs` around lines 78 - 98, Update the TLS path
configuration fields forge_root_ca_path, forge_client_cert_path, and
forge_client_key_path to reject empty values returned by std::env::var,
accepting only non-empty paths and falling back to the corresponding tls_default
constants with the existing warnings. Apply identical validation to all three
paths before they reach build_forge_client_config.
Source: Path instructions
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/preingestion-manager/src/lib.rs (1)
2160-2196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
start_kill()for separate kill/reap diagnostics.
tokio::process::Child::kill().awaitalready performs the kill and waits, so the followingwait().awaitis not an independent reap step. Usestart_kill()followed bywait().await, or keep onlykill().awaitand log one cleanup failure. Tokio documentskillas SIGKILL followed bywait. (docs.rs)🤖 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 `@crates/preingestion-manager/src/lib.rs` around lines 2160 - 2196, Update the local kill_and_reap helper to call Child::start_kill() before Child::wait().await, preserving the separate warning diagnostics for kill initiation and process reaping. Keep the existing address, context, and error fields and cleanup call sites unchanged.
🤖 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.
Nitpick comments:
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 2160-2196: Update the local kill_and_reap helper to call
Child::start_kill() before Child::wait().await, preserving the separate warning
diagnostics for kill initiation and process reaping. Keep the existing address,
context, and error fields and cleanup call sites unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30d872a5-7572-4f7e-83f9-01cda9c528b0
📒 Files selected for processing (17)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
|
@coderabbitai full_review, thanks! |
|
(๑˃̵ᴗ˂̵)و ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 20 minutes. |
| .await?; | ||
|
|
||
| let _ = txn.rollback().await; | ||
| if let Err(e) = txn.rollback().await { |
There was a problem hiding this comment.
Proably better to add some function to txn that does this (infallible rollback with some additional &str to add to logs.
There was a problem hiding this comment.
Great call -- done. Added a Transaction::rollback_or_log(context) method (matching the existing rollback/commit style) so the roll-back-and-warn-but-don't-fail logic lives in one place, and collapsed all 7 sites to txn.rollback_or_log("…").await. The expected_machine error-unwind paths still surface the original operation error afterward. Thanks for the suggestion!
|
@coderabbitai full_review, thanks! |
|
٩(◕‿◕)۶ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/dhcp/src/lib.rs (1)
78-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject explicitly empty TLS paths before accepting environment overrides.
std::env::varreturnsOk("")for an explicitly empty variable, so these branches skip the fallback and pass empty certificate/key paths downstream. Accept only non-empty values; otherwise log the reason and use the correspondingtls_defaultpath. This is the same unresolved issue reported in the previous review.🤖 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 `@crates/dhcp/src/lib.rs` around lines 78 - 98, Update the environment-variable handling for forge_root_ca_path, forge_client_cert_path, and forge_client_key_path to accept overrides only when their values are non-empty. Treat Ok("") like an unset or unreadable variable, log the fallback reason, and use the corresponding tls_default path.Source: Path instructions
🤖 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.
Duplicate comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Update the environment-variable handling for
forge_root_ca_path, forge_client_cert_path, and forge_client_key_path to accept
overrides only when their values are non-empty. Treat Ok("") like an unset or
unreadable variable, log the fallback reason, and use the corresponding
tls_default path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54a7f7c6-a0b3-42f5-a871-faf36889695e
📒 Files selected for processing (18)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/api-db/src/lib.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/dsx-exchange-consumer/Cargo.toml
- crates/api-core/src/handlers/power_shelf.rs
- crates/api-core/src/handlers/expected_machine.rs
- crates/dsx-exchange-consumer/tests/metric_exposition.rs
- crates/site-explorer/src/metrics.rs
- crates/api-core/src/handlers/instance.rs
- crates/api-core/src/handlers/rack.rs
- crates/dsx-exchange-consumer/src/metrics.rs
- crates/health/src/sink/log_file.rs
- crates/dsx-exchange-consumer/src/health_updater.rs
- docs/observability/core_metrics.md
- crates/preingestion-manager/src/lib.rs
- crates/site-explorer/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/preingestion-manager/src/lib.rs (1)
2163-2167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel the capture source as an enum.
contexthas only two valid values, stdout and stderr capture failure. Accept an enum withDisplayinstead of&strso future call sites cannot introduce arbitrary context values.🤖 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 `@crates/preingestion-manager/src/lib.rs` around lines 2163 - 2167, Update kill_and_reap to accept a dedicated enum representing the two capture-failure sources, stdout and stderr, instead of an arbitrary &str context. Define or reuse an enum implementing Display, update all call sites to pass the appropriate variant, and preserve the existing context text in error reporting.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/handlers/instance.rs`:
- Around line 281-282: Move each handler’s rollback_or_log call before its
fallible query and conversion work so rollback failures are reported on all
early-return paths. Apply this pattern in
crates/api-core/src/handlers/instance.rs:281-282, credential.rs:529-530,
power_shelf.rs:141-142, rack.rs:128, and switch.rs:179-180, preserving each
existing rollback context message.
---
Nitpick comments:
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 2163-2167: Update kill_and_reap to accept a dedicated enum
representing the two capture-failure sources, stdout and stderr, instead of an
arbitrary &str context. Define or reuse an enum implementing Display, update all
call sites to pass the appropriate variant, and preserve the existing context
text in error reporting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47f215b2-7846-4147-8559-a5966798b7d9
📒 Files selected for processing (18)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/api-db/src/lib.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
| txn.rollback_or_log("read-only load of instances by id") | ||
| .await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant handler sections with line numbers.
for f in \
crates/api-core/src/handlers/instance.rs \
crates/api-core/src/handlers/credential.rs \
crates/api-core/src/handlers/power_shelf.rs \
crates/api-core/src/handlers/rack.rs \
crates/api-core/src/handlers/switch.rs
do
echo "===== $f ====="
sed -n '1,260p' "$f" | nl -ba | sed -n '1,260p' | tail -n +1
echo
doneRepository: NVIDIA/infra-controller
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact ranges around the cited lines.
sed -n '240,310p' crates/api-core/src/handlers/instance.rs | nl -ba
echo "-----"
sed -n '500,560p' crates/api-core/src/handlers/credential.rs | nl -ba
echo "-----"
sed -n '110,170p' crates/api-core/src/handlers/power_shelf.rs | nl -ba
echo "-----"
sed -n '100,150p' crates/api-core/src/handlers/rack.rs | nl -ba
echo "-----"
sed -n '150,220p' crates/api-core/src/handlers/switch.rs | nl -baRepository: NVIDIA/infra-controller
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "rollback_or_log|fn rollback_or_log|impl .*rollback_or_log" crates/api-core/src crates -g '*.rs'Repository: NVIDIA/infra-controller
Length of output: 1878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
show() {
f="$1"
start="$2"
end="$3"
echo "===== $f:$start-$end ====="
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e { printf "%d:%s\n", NR, $0 }' "$f"
echo
}
show crates/api-core/src/handlers/instance.rs 250 295
show crates/api-core/src/handlers/credential.rs 500 545
show crates/api-core/src/handlers/power_shelf.rs 120 155
show crates/api-core/src/handlers/rack.rs 110 140
show crates/api-core/src/handlers/switch.rs 160 205
show crates/api-db/src/lib.rs 680 735Repository: NVIDIA/infra-controller
Length of output: 9537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Transaction drop/commit behavior and any auto-rollback path.
rg -n "impl Drop for Transaction|rollback\(\)|commit\(\)|auto-rollback|Drop for .*Transaction" crates/api-db/src/lib.rs crates/api-db/src -g '*.rs'Repository: NVIDIA/infra-controller
Length of output: 9308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the transaction type around the drop/commit implementation.
awk 'NR>=735 && NR<=860 { printf "%d:%s\n", NR, $0 }' crates/api-db/src/lib.rsRepository: NVIDIA/infra-controller
Length of output: 4879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Drop implementation and any references to implicit rollback.
rg -n "Drop for Transaction|drop\(self\)|rollback on drop|auto.*rollback|transaction.*drop" crates/api-db/src/lib.rs crates/api-db/src -g '*.rs'Repository: NVIDIA/infra-controller
Length of output: 161
🌐 Web query:
sqlx Transaction drop automatically rolls back rollback errors ignored documentation
💡 Result:
In sqlx, when a Transaction object goes out of scope without being explicitly committed or rolled back, its Drop implementation triggers an automatic rollback [1][2]. Key points regarding this behavior include: 1. Automatic Rollback: If neither commit nor rollback is called before the transaction goes out of scope, the Transaction destructor (Drop) queues a rollback operation [1][2]. This is designed to ensure that pending transactions do not persist if an error occurs or the code path exits prematurely [1][2]. 2. Error Handling Limitations: Because the Drop trait in Rust is synchronous and cannot be async, the rollback operation triggered by dropping a transaction cannot be awaited [2][3]. Consequently, any errors that might occur during this automatic rollback cannot be captured or handled by your application code [3]. The rollback is effectively "fire-and-forget," and its success or failure remains opaque to the caller [2][3]. 3. Recommended Practice: Relying on the Drop implementation for rollback is considered a fallback mechanism rather than a best practice [3][4]. Since the automatic rollback relies on the underlying connection's state and future I/O, developers are encouraged to explicitly call.rollback.await or use helper functions like Connection::transaction (which handles commit/rollback automatically based on the result of the provided closure) to ensure predictable and reliable transaction lifecycle management [5][3]. 4. Performance Implications: Relying on Drop can lead to delayed rollbacks [3][4]. The rollback command is queued to be executed during the next asynchronous operation on that connection (or when the connection is returned to the pool and validated), which may result in database locks being held longer than necessary [3][4].
Citations:
- 1: https://docs.rs/sqlx/latest/sqlx/struct.Transaction.html
- 2: https://github.com/launchbadge/sqlx/blob/main/sqlx-core/src/transaction.rs
- 3: Not sure how to compose multi-statement transactions with reliable rollback on Error transact-rs/sqlx#1560
- 4: improve docs: Transactions can use up connection pool transact-rs/sqlx#2980
- 5: https://docs.rs/sqlx/latest/sqlx/trait.Connection.html
🌐 Web query:
sqlx Transaction on Drop rollback silently ignore errors docs
💡 Result:
In sqlx, if a Transaction goes out of scope without an explicit call to commit or rollback, the transaction is automatically rolled back via its Drop implementation [1][2][3]. This automatic rollback occurs silently in the sense that the Drop trait cannot return a Result, and therefore it cannot propagate errors to the caller [2][3]. Because the rollback is initiated during the synchronous Drop method, sqlx queues a rollback operation using start_rollback, which is intended to be executed upon the next asynchronous invocation of the underlying connection (such as when the connection is returned to a connection pool) [2][3]. Key points regarding this behavior: - Automatic Rollback: If you do not explicitly commit or rollback, sqlx will ensure the transaction is aborted when the Transaction object is dropped [1][3]. - Error Handling: Because the automatic process happens in the drop implementation, any errors that occur during this background rollback cannot be caught or handled by your application code [2][3]. - Implementation: The library performs this by setting a flag in the connection state to trigger a rollback at the next available opportunity [2][3]. It is considered best practice to explicitly call commit on your transactions to ensure they are finalized successfully and to handle potential errors resulting from the commit operation [2][3].
Citations:
- 1: https://docs.rs/sqlx/latest/sqlx/struct.Transaction.html
- 2: https://github.com/launchbadge/sqlx/blob/main/sqlx-core/src/transaction.rs
- 3: https://docs.rs/sqlx-core/latest/src/sqlx_core/transaction.rs.html
Move rollback_or_log ahead of the fallible work.
These read-only handlers return early on query/conversion errors, so the explicit rollback/log path is skipped and the transaction falls back to Drop’s silent rollback. That leaves rollback failures unreported on the error path. Apply the same pattern in the other listed handlers.
📍 Affects 5 files
crates/api-core/src/handlers/instance.rs#L281-L282(this comment)crates/api-core/src/handlers/credential.rs#L529-L530crates/api-core/src/handlers/power_shelf.rs#L141-L142crates/api-core/src/handlers/rack.rs#L128-L128crates/api-core/src/handlers/switch.rs#L179-L180
🤖 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 `@crates/api-core/src/handlers/instance.rs` around lines 281 - 282, Move each
handler’s rollback_or_log call before its fallible query and conversion work so
rollback failures are reported on all early-return paths. Apply this pattern in
crates/api-core/src/handlers/instance.rs:281-282, credential.rs:529-530,
power_shelf.rs:141-142, rack.rs:128, and switch.rs:179-180, preserving each
existing rollback context message.
|
@coderabbitai full_review, thanks! |
|
(づ｡◕‿‿◕｡)づ ✅ Action performedFull review finished. |
A handful of fallible operations were silent in both pipelines -- no log, no metric -- so observability couldn't see them at all, including a rack health-report persist failure discarded with `Err(_) => { /* will retry */ }`. Every one now logs, and the one whose failure rate is worth watching also counts.
Primary callouts are:
- `carbide_dsx_exchange_consumer_health_report_persist_failures_total` -- the dsx health-report persist failure now emits a single framework event that both warns and counts (the rack id and error ride as log context). It's the one failure here worth a rate, so it's the one that gets a counter.
- The silently-dropped failures now warn: the read-only and error-unwinding `txn.rollback()` drops across the handlers, now funneled through a shared `Transaction::rollback_or_log` helper; the DHCP TLS-material reads that fall back to a built-in default cert; the empty product serial fed into DPF; the preingestion subprocess kill/wait drops; and the health log-file flush, rotate, and prune drops. Each keeps its crate's logging convention, and the error-unwinding rollbacks log without clobbering the original error being returned.
- The site-explorer `bmc_reset_count` counted attempts, not successes: the ipmitool reset dropped its result with `.ok()` while the counter ticked regardless. A shared `record_bmc_reset_outcome` now increments only on success and logs the failure, and both the ipmitool and Redfish paths route through it -- so the counter finally means what its name says, with no rename.
The MQTT state-change send drop the issue pointed at turned out to be a test mock; the production send path already logs and meters every outcome, so nothing was left silent there.
Tests added!
This supports NVIDIA#3179
|
Thanks @coderabbitai -- done. |
|
@coderabbitai full_review, thanks! |
|
(⌐■_■) ✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes.
Nothing further from me on this thread. (づ｡◕‿‿◕｡)づ |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
crates/health/src/sink/log_file.rs (2)
211-222: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRotation still proceeds after a flush failure.
self.current_file.take()drops the writer before checkingflush(); on failure the handle is gone and rotation continues, which can strand pending bytes while only logging a warning. This was flagged previously and remains unresolved.🐛 Proposed fix
- if let Some(mut file) = self.current_file.take() - && let Err(e) = file.flush() - { - tracing::warn!(error = %e, "failed to flush log file before rotation"); - } + if let Some(file) = self.current_file.as_mut() { + if let Err(e) = file.flush() { + tracing::warn!(error = %e, "failed to flush log file before rotation"); + return Err(format!("failed to flush log file before rotation: {e}")); + } + } + self.current_file = None;🤖 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 `@crates/health/src/sink/log_file.rs` around lines 211 - 222, Update the rotation logic around current_file.take() so a failed file.flush() returns or otherwise aborts rotation immediately instead of only logging a warning. Preserve the writer handle until the flush succeeds, ensuring pending bytes are not stranded and the existing rotation steps run only after a successful flush.
236-254: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStill gating rename/remove on
Path::exists()for two of three operations.
from.exists()(line 236) andoldest.exists()(line 251) mask real I/O errors (permissions, etc.) as silent absence, and are TOCTOU-prone. This was flagged previously; the current→.1rename was fixed to callfs::renamedirectly, but the other two sites weren't.🛡️ Proposed fix
- if from.exists() - && let Err(e) = fs::rename(&from, &to) - { - tracing::warn!(error = %e, from = %from.display(), to = %to.display(), "failed to shift rotated log file"); - } + if let Err(e) = fs::rename(&from, &to) + && e.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!(error = %e, from = %from.display(), to = %to.display(), "failed to shift rotated log file"); + } ... - if oldest.exists() - && let Err(e) = fs::remove_file(&oldest) - { - tracing::warn!(error = %e, path = %oldest.display(), "failed to prune oldest rotated log file"); + if let Err(e) = fs::remove_file(&oldest) + && e.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!(error = %e, path = %oldest.display(), "failed to prune oldest rotated log file"); }🤖 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 `@crates/health/src/sink/log_file.rs` around lines 236 - 254, Remove the exists() guards around the rotation shift and oldest-backup pruning in the log rotation logic, and call fs::rename(&from, &to) and fs::remove_file(&oldest) unconditionally. Preserve the existing error warnings and path context, allowing missing files and other filesystem errors to be handled through those results consistently with the current_path-to-backup rename.crates/dhcp/src/lib.rs (1)
78-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEmpty
FORGE_*_PATHvalues still bypass the fallback.
std::env::varreturnsOk("")for an explicitly-empty variable, so all three branches (FORGE_ROOT_CAFILE_PATH,FORGE_CLIENT_CERT_PATH,FORGE_CLIENT_KEY_PATH) still skip the warning/fallback path and pass an empty path straight tobuild_forge_client_config. This was flagged on the prior commit and remains unresolved.🛡️ Proposed fix
- forge_root_ca_path: std::env::var("FORGE_ROOT_CAFILE_PATH").unwrap_or_else(|e| { - log::warn!( - "FORGE_ROOT_CAFILE_PATH unset or unreadable ({e}); falling back to the built-in default root CA at {}", - tls_default::ROOT_CA - ); - tls_default::ROOT_CA.to_string() - }), + forge_root_ca_path: match std::env::var("FORGE_ROOT_CAFILE_PATH") { + Ok(path) if !path.is_empty() => path, + Ok(_) => { + log::warn!( + "FORGE_ROOT_CAFILE_PATH is empty; falling back to the built-in default root CA at {}", + tls_default::ROOT_CA + ); + tls_default::ROOT_CA.to_string() + } + Err(e) => { + log::warn!( + "FORGE_ROOT_CAFILE_PATH unset or unreadable ({e}); falling back to the built-in default root CA at {}", + tls_default::ROOT_CA + ); + tls_default::ROOT_CA.to_string() + } + },Apply the same pattern to
forge_client_cert_pathandforge_client_key_path.As per path instructions, "DHCP and networking code" configuration correctness should be reviewed for runtime behavior.
🤖 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 `@crates/dhcp/src/lib.rs` around lines 78 - 98, Update the three environment-variable initializers for forge_root_ca_path, forge_client_cert_path, and forge_client_key_path to treat an explicitly empty FORGE_*_PATH value like an unset or unreadable value. Preserve the warning and tls_default fallback behavior for empty values, while retaining non-empty configured paths.Source: Path instructions
🤖 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.
Duplicate comments:
In `@crates/dhcp/src/lib.rs`:
- Around line 78-98: Update the three environment-variable initializers for
forge_root_ca_path, forge_client_cert_path, and forge_client_key_path to treat
an explicitly empty FORGE_*_PATH value like an unset or unreadable value.
Preserve the warning and tls_default fallback behavior for empty values, while
retaining non-empty configured paths.
In `@crates/health/src/sink/log_file.rs`:
- Around line 211-222: Update the rotation logic around current_file.take() so a
failed file.flush() returns or otherwise aborts rotation immediately instead of
only logging a warning. Preserve the writer handle until the flush succeeds,
ensuring pending bytes are not stranded and the existing rotation steps run only
after a successful flush.
- Around line 236-254: Remove the exists() guards around the rotation shift and
oldest-backup pruning in the log rotation logic, and call fs::rename(&from, &to)
and fs::remove_file(&oldest) unconditionally. Preserve the existing error
warnings and path context, allowing missing files and other filesystem errors to
be handled through those results consistently with the current_path-to-backup
rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ec51933-e5bc-4688-b6f8-f595669873c3
📒 Files selected for processing (18)
crates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/power_shelf.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/switch.rscrates/api-db/src/lib.rscrates/dhcp/src/lib.rscrates/dsx-exchange-consumer/Cargo.tomlcrates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rscrates/health/src/sink/log_file.rscrates/machine-controller/src/handler/dpf.rscrates/preingestion-manager/src/lib.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/metrics.rsdocs/observability/core_metrics.md
3 participants
A handful of fallible operations were silent in both pipelines -- no log, no metric -- so observability couldn't see them at all, including a rack health-report persist failure discarded with
Err(_) => { /* will retry */ }. Every one now logs, and the one whose failure rate is worth watching also counts.carbide_dsx_exchange_consumer_health_report_persist_failures_total-- the dsx health-report persist failure now emits a single framework event that both warns and counts (rack id and error ride as log context). It's the one failure here worth a rate.txn.rollback()drops in the instance / power-shelf / expected-machine handlers; the DHCP TLS-material reads that fall back to a built-in default cert; the empty product serial fed into DPF; the preingestion subprocess kill/wait drops; and the health log-file flush, rotate, and prune drops. Each keeps its crate's logging convention, and the error-unwinding rollbacks log without clobbering the original error being returned.bmc_reset_countcounted attempts, not successes: the ipmitool reset dropped its result with.ok()while the counter ticked regardless. A sharedrecord_bmc_reset_outcomenow increments only on success and logs the failure, and both the ipmitool and Redfish paths route through it -- so the counter finally means what its name says, with no rename.The MQTT state-change send drop the issue pointed at turned out to be a test mock; the production send path already logs and meters every outcome, so nothing was left silent there.
This supports #3179