Classify failures, report block height, bound whole steps

README.md

@@ -21,7 +21,7 @@ INFO  Node information queried!
 
 1. **Connects** to the node's JSON-RPC endpoint over WebSocket (`ws://` or `wss://`).
 2. **Checks chain identity** by asking for the hash of block 0 via `chain_getBlockHash` and comparing it to the `--genesis-hash` you supplied. A node on a different chain is rejected and the client exits non-zero without querying anything further.
-3. **Queries node information** — `system_name`, `system_chain`, `system_version` and `system_health` — matching each response to its request by JSON-RPC id, since nodes are free to answer out of order (and do). All four go out before any reply is read, so the step costs one round trip rather than four.
+3. **Queries node information** — `system_name`, `system_chain`, `system_version`, `system_health` and `chain_getHeader` — matching each response to its request by JSON-RPC id, since nodes are free to answer out of order (and do). All five go out before any reply is read, so the step costs one round trip rather than five.
 4. **Follows new blocks**, optionally — with `--follow N` it subscribes via `chain_subscribeNewHeads`, reports headers as the node pushes them, then unsubscribes.
 5. **Reports**, either as logs through `env_logger` (`RUST_LOG=debug` shows the full exchange) or as a single JSON object with `--json`.
 
@@ -36,23 +36,51 @@ $ substrate-node-probe --node-address wss://rpc.polkadot.io --follow 2 --json 2>
   "ok": true,
   "genesis_hash": "0x91b171bb158e2d3848fa23a9f1c25182fb8e20313b2c1eb49219da7a70ce90c3",
   "genesis_verified": true,
-  "rpc_latency_ms": 63,
+  "rpc_latency_ms": 58,
   "name": "Parity Polkadot",
   "chain": "Polkadot",
   "version": "1.24.0-660acefe665",
-  "peers": 65,
+  "peers": 69,
   "is_syncing": false,
   "should_have_peers": true,
+  "best_block": 32264078,
   "heads_followed": 2
 }
 ```
 
-Two properties worth relying on:
+Three properties worth relying on:
 
-- **A failed run still prints a report** — `"ok": false` with an `error`, plus whatever was gathered before the failure. A probe that goes silent exactly when the node breaks is no use to whatever is parsing it. A genesis mismatch, for instance, still reports the hash the node actually serves, so you needn't parse it back out of the error string.
+- **A failed run still prints a report** — `"ok": false` with a `failure` and an `error`, plus whatever was gathered before the failure. A probe that goes silent exactly when the node breaks is no use to whatever is parsing it. A genesis mismatch, for instance, still reports the hash the node actually serves.
 - **Fields the node did not answer are omitted, not `null`** — `"peers": 0` means an isolated node; a missing `peers` means the node never said. `system_health` is not exposed everywhere, and a refusal costs only its own fields rather than the whole report.
+- **Nothing requires parsing prose.** The `error` string is for humans and is free to be reworded; `failure` is the part that is safe to alert on.
 
-`genesis_verified` is `true` only when `--genesis-hash` was supplied *and* matched; without the flag the hash is reported but nothing about it is proven. `should_have_peers` is what makes `peers: 0` interpretable — it is `false` on a dev chain running alone.
+`genesis_verified` is `true` only when `--genesis-hash` was supplied *and* matched; without the flag the hash is reported but nothing about it is proven. `should_have_peers` is what makes `peers: 0` interpretable — it is `false` on a dev chain running alone. `best_block` is the node's current height, which is how you tell a stuck node from a healthy one; the probe reports it but cannot judge staleness on its own, because a Substrate header carries no timestamp — compare it across runs, or against a second node.
+
+### Failure kinds
+
+On failure, `failure` carries one of these. They are a stable contract; the prose in `error` is not.
+
+| `failure` | Meaning |
+| --- | --- |
+| `config` | The command line asked for something impossible. The node is blameless. |
+| `connect` | The connection was never established. |
+| `timeout` | A wait expired. The node may be up but is not answering in time. |
+| `transport` | The connection failed or closed part-way through. |
+| `protocol` | The node answered with something unusable — bad JSON, or a result of the wrong shape. |
+| `rpc_error` | The node returned a JSON-RPC error for a call the probe needs. |
+| `genesis_mismatch` | The node is serving a different chain than you required. |
+| `requirement_unmet` | The node is reachable and correct, but failed a `--require-*` bar. |
+
+The distinction that motivates this: `connect` and `requirement_unmet` are both "the probe exited 1", but one means your monitoring cannot see the node and the other means the node is up and telling you it is unhealthy. Those usually want different alerts.
+
+```bash
+case "$(substrate-node-probe --node-address "$RPC" --require-peers 1 --json 2>/dev/null | jq -r '.failure // "ok"')" in
+  ok)                page=none ;;
+  requirement_unmet) page=degraded ;;
+  connect|timeout)   page=unreachable ;;
+  *)                 page=investigate ;;
+esac
+```
 
 ### Why WebSocket
 
@@ -80,8 +108,9 @@ The `HandshakeMessage` struct in `src/scale.rs` is a worked example of SCALE enc
 | --- | --- |
 | `src/main.rs` | The CLI flags, and the order the steps run in. |
 | `src/rpc.rs` | The transport: opening the WebSocket, request ids, timeouts, reading frames. Knows nothing about chains. |
-| `src/probe.rs` | The JSON-RPC calls — genesis hash, identity and health, following heads. |
+| `src/probe.rs` | The JSON-RPC calls — genesis hash, identity, health and height, following heads. |
 | `src/report.rs` | The findings, their JSON shape, and the `--require-*` checks. |
+| `src/error.rs` | The failure taxonomy. |
 | `src/scale.rs` | The SCALE codec example. |
 
 ## Requirements
@@ -103,6 +132,9 @@ cargo run -- [--node-address <url>] [--genesis-hash <hex>]
 | `--require-peers <N>` | *(none)* | Fail unless the node reports at least `N` connected peers. |
 | `--require-synced` | *(off)* | Fail if the node is still syncing. |
 | `--json` | *(off)* | Print the findings to stdout as one JSON object. Logs stay on stderr. |
+| `--connect-timeout <SECS>` | `10` | Seconds to allow for the connection. |
+| `--rpc-timeout <SECS>` | `10` | Seconds to allow the node to answer a request. |
+| `--head-timeout <SECS>` | `120` | Seconds to allow for each pushed header under `--follow`. Raise it for a slow parachain. |
 
 Exit code is `0` on success and `1` on any failure, so it works as a health check directly:
 
@@ -179,4 +211,4 @@ It is excluded from `cargo test` and never runs on a pull request, because it de
 ## Notes
 
 - TLS uses `rustls` with the `ring` provider, so the build needs no OpenSSL system libraries.
-- Both the connect and per-response paths are bounded by a 10-second timeout; a node that accepts the socket and goes silent produces an error rather than a hang. Waiting for a pushed block header uses a separate, much longer bound (120s), since that waits on the chain's block time rather than on the node being responsive.
+- Every wait is bounded, so a node that accepts the socket and goes silent produces an error rather than a hang. The bounds apply to a **step as a whole**, not to each frame within it: the read loops skip frames they do not recognise, and a per-frame clock would let a node emitting one unrelated frame just inside the limit keep the probe waiting forever — every individual wait short, the total unbounded. Waiting for a pushed block header gets a fresh budget per header, since that waits on the chain's block time rather than on the node being responsive.

src/error.rs

@@ -0,0 +1,115 @@
+//! What went wrong, in a form a machine can branch on.
+//!
+//! The probe's whole output contract is that a consumer should never have to
+//! parse prose to act — see the `failure` field on
+//! [`ProbeReport`](crate::report::ProbeReport). A `Box<dyn Error>` carrying a
+//! sentence cannot support that, so every fallible step returns a
+//! [`ProbeError`] whose [`Failure`] says which *kind* of thing went wrong while
+//! the message stays free to be as specific as it likes.
+
+use serde::Serialize;
+use std::fmt;
+
+/// The class of failure, stable enough to alert on.
+///
+/// A monitor pages differently for "I cannot reach this node" than for "this
+/// node is up but has no peers", and those two must be distinguishable without
+/// matching on error text that is free to be reworded at any time.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)]
+#[serde(rename_all = "snake_case")]
+pub(crate) enum Failure {
+    /// The command line asked for something impossible; the node is blameless.
+    Config,
+    /// The connection was never established.
+    Connect,
+    /// A wait expired. The node may be up but is not answering in time.
+    Timeout,
+    /// The connection failed or closed part-way through.
+    Transport,
+    /// The node answered, but with something unusable — bad JSON, or a result
+    /// of the wrong shape.
+    Protocol,
+    /// The node returned a JSON-RPC error for a call the probe needs.
+    RpcError,
+    /// The node is serving a different chain than the caller required.
+    GenesisMismatch,
+    /// The node is reachable and correct, but failed a `--require-*` bar.
+    RequirementUnmet,
+}
+
+/// A failure, classified.
+#[derive(Debug)]
+pub(crate) struct ProbeError {
+    kind: Failure,
+    message: String,
+}
+
+impl ProbeError {
+    pub(crate) fn new(kind: Failure, message: impl Into<String>) -> Self {
+        ProbeError {
+            kind,
+            message: message.into(),
+        }
+    }
+
+    pub(crate) fn config(message: impl Into<String>) -> Self {
+        Self::new(Failure::Config, message)
+    }
+
+    pub(crate) fn connect(message: impl Into<String>) -> Self {
+        Self::new(Failure::Connect, message)
+    }
+
+    pub(crate) fn timeout(message: impl Into<String>) -> Self {
+        Self::new(Failure::Timeout, message)
+    }
+
+    pub(crate) fn transport(message: impl Into<String>) -> Self {
+        Self::new(Failure::Transport, message)
+    }
+
+    pub(crate) fn protocol(message: impl Into<String>) -> Self {
+        Self::new(Failure::Protocol, message)
+    }
+
+    pub(crate) fn rpc(message: impl Into<String>) -> Self {
+        Self::new(Failure::RpcError, message)
+    }
+
+    pub(crate) fn kind(&self) -> Failure {
+        self.kind
+    }
+
+    /// Adds detail without changing the classification.
+    ///
+    /// Used where an inner step already knows *what* went wrong and the outer
+    /// step knows *how far it had got* — a timeout is still a timeout when you
+    /// note it happened on the second of three headers.
+    pub(crate) fn context(mut self, extra: impl AsRef<str>) -> Self {
+        self.message = format!("{} {}", self.message, extra.as_ref());
+        self
+    }
+}
+
+impl fmt::Display for ProbeError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(&self.message)
+    }
+}
+
+impl std::error::Error for ProbeError {}
+
+/// A malformed frame is the node's fault, not the transport's.
+impl From<serde_json::Error> for ProbeError {
+    fn from(e: serde_json::Error) -> Self {
+        ProbeError::protocol(format!("node sent unparseable JSON: {e}"))
+    }
+}
+
+/// A WebSocket error mid-run means the connection broke under us; failures to
+/// *establish* one are classified at the call site in [`crate::rpc::connect`].
+impl From<tokio_tungstenite::tungstenite::Error> for ProbeError {
+    fn from(e: tokio_tungstenite::tungstenite::Error) -> Self {
+        ProbeError::transport(format!("websocket error: {e}"))
+    }
+}

src/main.rs

@@ -5,6 +5,7 @@
 //! themselves live in [`probe`], the connection in [`rpc`], and the findings in
 //! [`report`].
 
+mod error;
 mod probe;
 mod report;
 mod rpc;
@@ -15,12 +16,14 @@ mod test_support;
 use clap::Parser;
 use env_logger::Env;
 use log::{error, info};
+use std::time::Duration;
 
+use error::ProbeError;
 use probe::{
     fetch_genesis_hash, follow_new_heads, parse_genesis_hash, query_node_info, verify_genesis_hash,
 };
 use report::{check_requirements, ProbeReport};
-use rpc::Timeouts;
+use rpc::{Timeouts, CONNECT_TIMEOUT, RPC_TIMEOUT, SUBSCRIPTION_TIMEOUT};
 
 /// Connect to a Substrate node, verify which chain it serves, and query its
 /// identity over JSON-RPC.
@@ -58,6 +61,32 @@ struct Opt {
     /// straight into `jq`.
     #[arg(long)]
     json: bool,
+
+    /// Seconds to allow for the connection to be established.
+    #[arg(long, value_name = "SECS", default_value_t = CONNECT_TIMEOUT.as_secs(), value_parser = clap::value_parser!(u64).range(1..))]
+    connect_timeout: u64,
+
+    /// Seconds to allow the node to answer a request. Bounds each step as a
+    /// whole, so a node dribbling unrelated frames cannot extend it.
+    #[arg(long, value_name = "SECS", default_value_t = RPC_TIMEOUT.as_secs(), value_parser = clap::value_parser!(u64).range(1..))]
+    rpc_timeout: u64,
+
+    /// Seconds to allow for each pushed block header under `--follow`. Far
+    /// longer than the RPC wait by default: this waits on the chain's block
+    /// time, not on the node being responsive. Raise it for a slow parachain.
+    #[arg(long, value_name = "SECS", default_value_t = SUBSCRIPTION_TIMEOUT.as_secs(), value_parser = clap::value_parser!(u64).range(1..))]
+    head_timeout: u64,
+}
+
+impl Opt {
+    /// The network waits this invocation asked for.
+    fn timeouts(&self) -> Timeouts {
+        Timeouts {
+            connect: Duration::from_secs(self.connect_timeout),
+            rpc: Duration::from_secs(self.rpc_timeout),
+            head: Duration::from_secs(self.head_timeout),
+        }
+    }
 }
 
 /// Connects to the node, verifies its chain identity and queries node info.
@@ -75,18 +104,15 @@ struct Opt {
 /// # Returns
 ///
 /// A Result indicating the success or failure of the run.
-async fn run(
-    opt: &Opt,
-    timeouts: Timeouts,
-    report: &mut ProbeReport,
-) -> Result<(), Box<dyn std::error::Error>> {
+async fn run(opt: &Opt, timeouts: Timeouts, report: &mut ProbeReport) -> Result<(), ProbeError> {
     let expected_genesis = opt
         .genesis_hash
         .as_deref()
         .map(parse_genesis_hash)
-        .transpose()?;
+        .transpose()
+        .map_err(|e| ProbeError::config(format!("--genesis-hash is unusable: {e}")))?;
 
-    let mut ws_stream = rpc::connect(&opt.node_address).await?;
+    let mut ws_stream = rpc::connect(&opt.node_address, timeouts.connect).await?;
 
     // Recorded before the comparison, so a mismatch is reported alongside the
     // hash that caused it rather than only in the error text.
@@ -102,6 +128,7 @@ async fn run(
     report.peers = info.peers;
     report.is_syncing = info.is_syncing;
     report.should_have_peers = info.should_have_peers;
+    report.best_block = info.best_block;
     info!("Node information queried!");
 
     // Judged before `--follow`, which can block for as long as the chain takes
@@ -138,10 +165,13 @@ async fn main() {
         ..Default::default()
     };
 
-    let result = run(&opt, Timeouts::default(), &mut report).await;
+    let result = run(&opt, opt.timeouts(), &mut report).await;
     match &result {
         Ok(()) => report.ok = true,
-        Err(e) => report.error = Some(e.to_string()),
+        Err(e) => {
+            report.failure = Some(e.kind());
+            report.error = Some(e.to_string());
+        }
     }
 
     // Printed on failure too: a probe that emits nothing precisely when the node
@@ -192,13 +222,16 @@ mod tests {
             require_peers: Some(1),
             require_synced: true,
             json: false,
+            connect_timeout: CONNECT_TIMEOUT.as_secs(),
+            rpc_timeout: RPC_TIMEOUT.as_secs(),
+            head_timeout: SUBSCRIPTION_TIMEOUT.as_secs(),
         };
 
         let mut report = ProbeReport {
             endpoint: opt.node_address.clone(),
             ..Default::default()
         };
-        let result = run(&opt, Timeouts::default(), &mut report).await;
+        let result = run(&opt, opt.timeouts(), &mut report).await;
 
         assert!(
             result.is_ok(),
@@ -216,6 +249,28 @@ mod tests {
             "system_health reported no peers: {report:#?}"
         );
         assert_eq!(report.is_syncing, Some(false));
+        assert!(
+            report.best_block.unwrap_or(0) > 0,
+            "chain_getHeader gave no height: {report:#?}"
+        );
         assert_eq!(report.heads_followed, Some(1), "no header was pushed");
     }
+
+    /// The CLI is a contract too. A zero timeout would make every run fail
+    /// instantly, which is never what someone means by asking for one.
+    #[test]
+    fn timeout_flags_reject_zero() {
+        assert!(
+            Opt::try_parse_from(["substrate-node-probe", "--rpc-timeout", "0"]).is_err(),
+            "a zero RPC timeout must not be accepted"
+        );
+        let opt = Opt::try_parse_from(["substrate-node-probe", "--rpc-timeout", "3"])
+            .expect("a positive timeout is valid");
+        assert_eq!(opt.timeouts().rpc, Duration::from_secs(3));
+        assert_eq!(
+            opt.timeouts().head,
+            SUBSCRIPTION_TIMEOUT,
+            "unset flags keep their defaults"
+        );
+    }
 }