Repository navigation
Fix restart-recovery crash in passive height-catchup + add diagnostics - #1
Merged
Merged
Conversation
Temporary debugging aid: restart_recovery's CI failure gives no signal about which stage timed out or what the restarted node last logged. This prints each node's last 10 log lines on timeout so the next CI failure is diagnosable instead of another blind guess. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior diagnostic only showed stdout: one "SYNCED to height=16" line, then total silence for the remaining ~90s. That silence is itself suspicious for a process that should keep casting votes/timing out every ~150ms - and this test harness was throwing away exactly the stream (stderr) a Rust panic message would appear on. Merge stderr lines (tagged) into the same log so the next timeout's diagnostic dump can actually show a panic if that's what's happening. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root cause of both original CI failures (and the debug branch's own reproduction, with stderr now captured): maybe_sync_forward's height jump replaces ConsensusEngine wholesale, discarding any cached proposal. If a QuorumCertificate for the just-jumped-to (height, round) arrives right after, commit_finalized_block correctly rejects it with UnknownProposedBlock (this engine instance never itself saw that block's Proposal) -- but the old code propagated that as a fatal error all the way out of main(), exiting the whole node process (ADR-0038's own "no-panic, exit cleanly on error" discipline, applied to a condition that isn't actually unrecoverable). Real block/state sync (ADR-0016) is still out of scope, so this commit doesn't try to apply that block. Instead: log a SKIPPED line and leave the engine at Finalize for that one height/round. The very same ongoing vote/QC gossip that revealed this gap keeps arriving and triggers the next maybe_sync_forward jump, which replaces the engine wholesale anyway -- self-healing with no new mechanism, matching ADR-0039's own already-acknowledged "round at a time" fallback in spirit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nikethebike
changed the title
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.
Started as a debug PR to diagnose two flaky CI failures in
multi_node_view_change.rs/restart_recovery.rs(both "timed out waiting for all nodes to log a matching line"). Turned up a real bug, now fixed.Root cause
maybe_sync_forward(ADR-0039's passive height-observation catch-up) replacesConsensusEnginewholesale on every forward jump, discarding any cached proposal. If aQuorumCertificatefor the just-jumped-to (height, round) arrives right after,commit_finalized_blockcorrectly rejects it withUnknownProposedBlock— but the old code propagated that as a fatal error all the way out ofmain(), exiting the whole node process. Confirmed via real CI logs (after adding stderr capture to the test harness, which had been silently discarding panic/error output):Changes
hn-node/src/node.rs:act_on'sFinalizedbranch now matches oncommit_finalized_block's result;UnknownProposedBlocklogs aSKIPPEDline and leaves the engine atFinalizefor that one height/round instead of exiting. The nextmaybe_sync_forwardjump (driven by the same ongoing gossip) replaces the engine wholesale anyway, self-healing with no new mechanism. Real block/state sync (ADR-0016) is still out of scope.hn-node/tests/support/mod.rs:wait_until_all_log's timeout error now dumps each node's last 10 log lines (previously just "timed out..." with no context), and stderr is captured into the same log instead of discarded — this is what surfaced the actual panic-equivalent error above.hn-node/tests/multi_node_view_change.rs,restart_recovery.rs: widenedWAITfrom 25s to 90s for CI-load headroom (kept as extra margin; not the actual fix).hn-node/src/lib.rs: crate doc comment updated to describe the new skip-and-self-heal behavior.Verified with 6 consecutive green CI reruns after the fix (previously failing intermittently, ~1-in-3 to ~1-in-2).
🤖 Generated with Claude Code