Fix health check #428
Fix health check #428
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the health check system to separately monitor both L2 client and builder client health, with different health status responses based on which component is failing and the current execution mode.
- Added L2 client health monitoring with separate status reporting (ServiceUnavailable vs PartialContent)
- Modified execution mode behavior: disabled/dry-run modes now only check L2 client health
- Added comprehensive test coverage for the new L2 client failure scenarios
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| crates/rollup-boost/src/server.rs | Passes l2_client to HealthHandle constructor |
| crates/rollup-boost/src/health.rs | Implements dual health checking for L2 and builder clients with distinct failure modes and adds corresponding tests |
| crates/rollup-boost/Cargo.toml | Adds serial_test dependency for test serialization |
| Cargo.lock | Updates lock file with serial_test and its transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/rollup-boost/Cargo.toml
Outdated
| tokio-util = { version = "0.7.13" } | ||
|
|
||
| [dev-dependencies] | ||
| serial_test = "*" |
There was a problem hiding this comment.
Using '*' for dependency versions is not recommended. Specify an explicit version to ensure reproducible builds and avoid unexpected breaking changes.
| serial_test = "*" | |
| serial_test = "2.0.0" |
| #[tokio::test] | ||
| async fn tick_advances_after_sleep() { | ||
| let mut ts = MonotonicTimestamp::new(); | ||
| let mut ts: MonotonicTimestamp = MonotonicTimestamp::new(); |
There was a problem hiding this comment.
The explicit type annotation : MonotonicTimestamp is redundant since the type can be inferred from MonotonicTimestamp::new(). Remove it for cleaner code.
| let mut ts: MonotonicTimestamp = MonotonicTimestamp::new(); | |
| let mut ts = MonotonicTimestamp::new(); |
| if t.saturating_sub(block.header.timestamp) | ||
| .gt(&self.max_unsafe_interval) | ||
| { | ||
| warn!(target: "rollup_boost::health", curr_unix = %t, unsafe_unix = %block.header.timestamp, "Builder client - unsafe block timestamp is too old updating health status"); |
There was a problem hiding this comment.
Missing comma after 'too old'. The message should read 'too old, updating health status' for proper grammar.
| warn!(target: "rollup_boost::health", curr_unix = %t, unsafe_unix = %block.header.timestamp, "Builder client - unsafe block timestamp is too old updating health status"); | |
| warn!(target: "rollup_boost::health", curr_unix = %t, unsafe_unix = %block.header.timestamp, "Builder client - unsafe block timestamp is too old, updating health status"); |
|
|
||
| use super::*; | ||
| use crate::{Probes, payload::PayloadSource}; | ||
| use serial_test::serial; |
There was a problem hiding this comment.
Seems like serial test is unnecessary and it's making tests very slow - consider removing it
There was a problem hiding this comment.
added it because they were flaky without it
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .expect("Time went backwards") | ||
| .as_secs(); | ||
|
|
||
| // L2 healthy unhealth |
There was a problem hiding this comment.
The comment contains garbled text 'healthy unhealth'. Based on the context (the L2 client is being set up with an old timestamp), this should be 'L2 unhealthy'.
| // L2 healthy unhealth | |
| // L2 unhealthy |
| self.probes.set_health(Health::Healthy); | ||
| if self.execution_mode.lock().is_enabled() { | ||
| // Only check builder client health if execution mode is enabled | ||
| // If its unhealthy, set the health status to PartialContent |
There was a problem hiding this comment.
Grammatical error: 'its' should be 'it's' (contraction of 'it is').
| // If its unhealthy, set the health status to PartialContent | |
| // If it's unhealthy, set the health status to PartialContent |
| #[tokio::test] | ||
| async fn tick_advances_after_sleep() { | ||
| let mut ts = MonotonicTimestamp::new(); | ||
| let mut ts: MonotonicTimestamp = MonotonicTimestamp::new(); |
There was a problem hiding this comment.
[nitpick] Unnecessary explicit type annotation. The type MonotonicTimestamp can be inferred from the new() call.
| let mut ts: MonotonicTimestamp = MonotonicTimestamp::new(); | |
| let mut ts = MonotonicTimestamp::new(); |
6 participants
The health check did not previously check the l2 client health. If the l2 client was unhealthy but the builder was healthy, it would set the service status to Healthy.
It checks the L2 client health first. If execution mode is enabled, it will then check the builder client health