Classify failures, report block height, bound whole steps - #19
Merged
Conversation
Four gaps, one of which contradicted a rule this project had already adopted. 1. Every failure was `ok: false` plus a prose string, so a monitor that wants to page differently for "unreachable" and "too few peers" had to regex it — exactly what #16 avoided for the genesis hash and then didn't do for the failure itself. Errors are now a ProbeError carrying a Failure kind, surfaced as a stable `failure` field: config, connect, timeout, transport, protocol, rpc_error, genesis_mismatch, requirement_unmet. `error` stays for humans and is free to be reworded; `failure` is the part safe to alert on. 2. The report never carried the node's height, so it could not answer "is this node stuck?" — the most common question asked of a node. chain_getHeader joins the pipelined query step and lands as `best_block`. No staleness check: a Substrate header carries no timestamp, so the honest thing is to report the number and let the caller compare across runs or against another node. 3. Timeouts bounded each frame, not each operation. The read loops skip frames they don't recognise, so a node emitting one unrelated frame every 9s would keep a 10s probe alive forever — every wait short, the total unbounded. A Deadline is now set once per step and every read inside draws down the same clock; --follow gets a fresh budget per header, which is the one place a long wait is legitimate. 4. The timeouts were compiled in. --connect-timeout, --rpc-timeout and --head-timeout expose them, rejecting 0 since an instant-fail probe is never what someone means. Verified against wss://rpc.polkadot.io: healthy run reports best_block 32264078 and no failure key; a genesis mismatch, an unmeetable --require-peers, a refused connection, a bad --genesis-hash and a 1s connect timeout each report their own kind. Tests 24 -> 29, including a chattering-node test that would hang on the old per-frame bound. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four gaps found reviewing the merged code. One of them contradicted a rule this project had already adopted.
1. The JSON couldn't say why it failed
Every failure was
ok: falseplus a proseerrorstring. A monitor wanting to page differently for "unreachable" vs "too few peers" had to regex it — which is exactly what #16 avoided for the genesis hash and then didn't do for the failure itself.Errors are now a
ProbeErrorcarrying aFailurekind, surfaced as a stablefailurefield.errorstays for humans and is free to be reworded;failureis the part safe to alert on.configconnecttimeouttransportprotocolrpc_errorgenesis_mismatchrequirement_unmet--require-*barconnectandrequirement_unmetare both "exited 1", but one means your monitoring can't see the node and the other means the node is up and telling you it's unhealthy. Those want different alerts.2. It never reported block height
The report couldn't answer "is this node stuck?" — the most common question asked of a node.
chain_getHeaderjoins the pipelined query step and lands asbest_block.No staleness check: a Substrate header carries no timestamp, so the honest thing is to report the number and let the caller compare across runs or against a second node. Claiming to judge freshness would have meant inventing a block time.
3. Timeouts bounded each frame, not each operation
The read loops skip frames they don't recognise, so a node emitting one unrelated frame every 9s would keep a 10s probe alive forever — every individual wait inside the limit, the total unbounded. A
Deadlineis now set once per step and every read inside draws down the same clock.--followgets a fresh budget per header, which is the one place a long wait is legitimate.There's a regression test that spams unrelated frames every 20ms; it hangs on the old per-frame bound, so it's wrapped in an outer timeout to fail rather than stall the suite.
4. Timeouts were compiled in
--connect-timeout,--rpc-timeout,--head-timeout. Zero is rejected — an instant-fail probe is never what someone means by asking for one.Verification
Each kind produced against real conditions rather than asserted:
(The
nulls are jq rendering an absent key — the field is genuinely omitted on success, pinned by a test.)Tests 24 → 29.
cargo fmt --check,cargo clippy --all-targets,cargo docand the live Polkadot test all pass.🤖 Generated with Claude Code