Stop writing node records that describe the wrong thing (GH-3698) - #3724
Merged
Conversation
…H-3698) Two independent defects found while diagnosing #3698, both of which made the wolverine_node_records table actively misleading during the reported incident. NodeRecord.For reads Options.Durability.AssignedNodeNumber, but the Balanced start path called the observer one line BEFORE adopting the number PersistAsync handed back, so every NodeStarted row carried the per-process default -- Guid.NewGuid().ToString().GetDeterministicHashCode() -- a random value unrelated to the node it describes. The reporter attributed those to the skeleton-node insert. The Solo path in StartLocally.cs already had the two in the right order. While here, "Node {NodeNumber} successfully assumed leadership" was being passed UniqueNodeId, a Guid; it now logs both, each under its own name. ejectStaleNodes continues past DeleteAsync for every node it deliberately spares -- this node itself, a leader a follower may not evict, and above all a node inside the GH-3604 hysteresis window -- and then reported the whole stale list to the observer, which writes one DormantNodeEjected record per node. A node that blipped stale for a single tick and recovered was recorded as ejected while still heartbeating, which is exactly the contradiction reported: an "ejected" record next to a fresh health_check. Only the nodes actually deleted are reported now. All three behavioural tests were red-checked against the unfixed source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ojaHNzLcE8m5krwfTNPxw
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.
Two independent defects found while diagnosing #3698.
Neither affects assignment behaviour, but both made
wolverine_node_recordsactively misleading during thereported incident — the reporter drew conclusions from these rows.
Independent of #3723 and #3719; this branch is off
main.NodeStartedrecords carried a randomnode_numberNodeRecord.ForreadsOptions.Durability.AssignedNodeNumber, but the Balanced start path called theobserver one line before adopting the number
PersistAsynchanded back:DurabilitySettings.AssignedNodeNumberdefaults toGuid.NewGuid().ToString().GetDeterministicHashCode(), so everyNodeStartedrow in a Balanced clusterdescribed itself with a random value unrelated to the node — the reporter's
1827159984/-1841074440,which they attributed to the skeleton-node insert. The Solo path in
StartLocally.csalready had the two inthe right order; this makes Balanced match.
While here:
"Node {NodeNumber} successfully assumed leadership"was being passedOptions.UniqueNodeId, aGuid. It now logs both, each under its own name.
DormantNodeEjectedfired for nodes that were never ejectedejectStaleNodescontinues pastDeleteAsyncfor every node it deliberately spares — this node itself,a leader a follower may not evict, and above all a node inside the GH-3604 hysteresis window — and then
handed the whole stale list to
_observer.StaleNodes, which writes oneDormantNodeEjectedrecord pernode it is given.
So a node that blipped stale for a single tick and recovered was recorded as ejected while still
heartbeating. That is exactly the contradiction reported against the production cluster: an "ejected" record
sitting next to a fresh
health_check. Only the nodesDeleteAsyncactually ran for are reported now.Tests
Three behavioural tests, all red-checked against the unfixed source:
node_reregisters_after_ejection.the_node_started_record_sees_the_assigned_node_numberejection_hysteresis_tests.records_no_ejection_for_a_peer_still_inside_the_hysteresis_windowejection_hysteresis_tests.records_no_ejection_for_a_stale_leader_a_follower_may_not_deleteplus
records_an_ejection_for_the_peer_it_actually_deletedguarding the positive case. Fullwolverine.slnxRelease build clean; 203/203 inCoreTests.Runtime.Agents.🤖 Generated with Claude Code
https://claude.ai/code/session_013ojaHNzLcE8m5krwfTNPxw