fix(#1726): clear stale "varies" hash size once a node settles (#1788)

Fixes #1726. ## Problem A MeshCore v1.16.0 repeater configured for 2-byte path hashes (`path.hash.mode=1`) — e.g. `36f6c7c7…` (`DK_3400_RAK_TEST`) — kept showing as **"varies"** / mixed 1-byte + 2-byte for the full 7-day advert window. Per the live data in the issue triage: of the node's ~20 recent adverts, exactly **one** (2026-06-09, across 15 distinct observer paths) was a genuine 1-byte flood advert; every other advert was 2-byte. The flip-flop heuristic in `computeNodeHashSizeInfo` weighs that stale advert equally with recent ones, so an operator who flips `path.hash.mode` mid-flight (or a single old 1-byte advert) stays flagged for the full window with no way to signal "the config is settled now." ## Fix Two coupled changes in `cmd/server/store.go` `computeNodeHashSizeInfo`: 1. **Chronological ordering.** `byPayloadType[4]` iterates in insertion order, not timestamp order, so `HashSize = Seq[last]` could pick the wrong advert under out-of-order MQTT ingest or chunked cold-load (the "carmack" concern from triage). We now collect `(FirstSeen, size)` pairs and **stable-sort by `FirstSeen`**; ties keep insertion order, preserving prior behavior when timestamps are equal. 2. **Recency decay.** After `transitions >= 2` raises the flip-flop flag, clear it when the most recent `hashSizeRecentAgreeCount` (= **3**) non-zero-hop adverts all agree on a single size. A node still flapping (recent adverts disagree) stays flagged. `3` mirrors the existing ≥3-observation threshold used to raise the flag. ## Policy note Triage marked this **needs-operator-input** because the decay is a behavior/policy change. This PR implements the rule the triage proposed ("if the last 3 adverts agree, clear inconsistent"), which matches the reporter's stated expectation. Happy to adjust the threshold or gate it differently per your call. ## Tests `cmd/server/issue1726_hash_decay_test.go`: - `TestIssue1726_SettledNodeNotInconsistent` — reporter's case (`[2,1,2,2,2]` within window) → `Inconsistent=false`, `HashSize=2`. - `TestIssue1726_HashSizeUsesChronologicallyLatest` — out-of-order insertion still reports the chronologically-latest size. - `TestIssue1726_ActiveFlapperStaysInconsistent` — a node whose recent adverts disagree stays flagged. Existing flip-flop / hash-collision tests unchanged and green; full `cmd/server` package suite passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Erwin Fiten <e.fiten@opteco.be> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-08-24 18:49:55 +00:00 · 2026-06-25 05:05:46 -07:00
parent d437958474
commit f0763aecce
2 changed files with 233 additions and 16 deletions
@@ -0,0 +1,156 @@
+package main
+
+import (
+	"testing"
+	"time"
+)
+
+// Issue #1726: a MeshCore v1.16.0 repeater configured for 2-byte path hashes
+// (path.hash.mode=1) was still shown as "varies" / mixed 1-byte+2-byte. The
+// node had one genuine 1-byte flood advert mid-window, but every advert since
+// has been 2-byte. The flip-flop heuristic weighed that stale 1-byte advert
+// equally with recent ones, so the node stayed flagged for the full 7-day
+// window even though its current config is settled.
+//
+// Expected: when the most recent non-zero-hop adverts all agree on a size, the
+// node is "settled" and must NOT be flagged inconsistent. HashSize must reflect
+// the chronologically-latest advert.
+
+// hashAdvertTx builds an ADVERT StoreTx (route FLOOD, non-zero hop) for a given
+// pubkey, hash size and FirstSeen timestamp.
+//
+//	hs=1 → path byte 0x01 (top 2 bits 00, hop bits non-zero)
+//	hs=2 → path byte 0x41 (top 2 bits 01)
+func hashAdvertTx(pk string, hs int, firstSeen string) *StoreTx {
+	pt := 4 // ADVERT
+	pathByte := "01"
+	if hs == 2 {
+		pathByte = "41"
+	}
+	return &StoreTx{
+		RawHex:      "11" + pathByte + "aabb", // header 0x11 → routeType FLOOD
+		FirstSeen:   firstSeen,
+		PayloadType: &pt,
+		DecodedJSON: `{"pubKey":"` + pk + `","name":"DK_3400_RAK_TEST","type":"ADVERT"}`,
+	}
+}
+
+// ts formats a relative timestamp the way the ingestor writes first_seen
+// (time.RFC3339, no fractional seconds).
+func ts(d time.Duration) string {
+	return time.Now().UTC().Add(d).Format(time.RFC3339)
+}
+
+func TestIssue1726_SettledNodeNotInconsistent(t *testing.T) {
+	ps := NewPacketStore(nil, nil)
+	pk := "36f6c7c73dfd265db996aeee25e1dc0cfe1ad4e5c1d6dd575325e3b241f4af78"
+
+	// Chronological history within the 7-day window: a single 1-byte blip
+	// 5 days ago, then settled to 2-byte. Old heuristic: AllSizes={1,2},
+	// 2 transitions → Inconsistent=true. The node is actually settled at 2-byte.
+	ps.byPayloadType[4] = []*StoreTx{
+		hashAdvertTx(pk, 2, ts(-6*24*time.Hour)),
+		hashAdvertTx(pk, 1, ts(-5*24*time.Hour)),
+		hashAdvertTx(pk, 2, ts(-2*24*time.Hour)),
+		hashAdvertTx(pk, 2, ts(-1*24*time.Hour)),
+		hashAdvertTx(pk, 2, ts(-1*time.Hour)),
+	}
+
+	info := ps.GetNodeHashSizeInfo()
+	ni := info[pk]
+	if ni == nil {
+		t.Fatalf("expected hash size info for %s", pk)
+	}
+	if ni.HashSize != 2 {
+		t.Errorf("HashSize = %d, want 2 (latest advert is 2-byte)", ni.HashSize)
+	}
+	if ni.Inconsistent {
+		t.Error("settled node (last 3 adverts all 2-byte) must NOT be flagged inconsistent")
+	}
+}
+
+func TestIssue1726_HashSizeUsesChronologicallyLatest(t *testing.T) {
+	ps := NewPacketStore(nil, nil)
+	pk := "aaaa0000bbbb1111cccc2222dddd3333eeee4444ffff5555aaaa6666bbbb7777"
+
+	// Inserted out of chronological order: the 2-byte advert is newest by
+	// FirstSeen but appended first. HashSize must follow time, not insertion.
+	ps.byPayloadType[4] = []*StoreTx{
+		hashAdvertTx(pk, 2, ts(-1*time.Hour)),    // newest
+		hashAdvertTx(pk, 1, ts(-5*24*time.Hour)), // oldest, appended last
+	}
+
+	info := ps.GetNodeHashSizeInfo()
+	ni := info[pk]
+	if ni == nil {
+		t.Fatalf("expected hash size info for %s", pk)
+	}
+	if ni.HashSize != 2 {
+		t.Errorf("HashSize = %d, want 2 (chronologically-latest advert)", ni.HashSize)
+	}
+}
+
+// Chronological ordering must be robust to FirstSeen format differences:
+// the ingestor writes RFC3339 with no fractional seconds, but a fractional
+// (".000Z") form must still order correctly relative to it. A naive string
+// compare would sort the no-fraction "...05Z" after a same-second "...05.000Z"
+// (because 'Z' > '.'), picking the wrong "latest" advert.
+func TestIssue1726_OrderingRobustToTimestampFormat(t *testing.T) {
+	ps := NewPacketStore(nil, nil)
+	pk := "1234000056780000abcd0000ef120000345600007890000012340000567800ab"
+
+	// Same wall-clock second, sub-second apart: the older advert in the
+	// no-fraction form, the newer 1ms later in the fractional form. A string
+	// compare sorts "...05Z" AFTER "...05.001Z" ('Z' > '.'), so it would treat
+	// the older 1-byte advert as latest; chronological parsing must not.
+	base := time.Now().UTC().Truncate(time.Second).Add(-3 * 24 * time.Hour)
+	older := base.Format(time.RFC3339)                                         // "...05Z"
+	newer := base.Add(1 * time.Millisecond).Format("2006-01-02T15:04:05.000Z") // "...05.001Z"
+
+	pt := 4
+	mk := func(pathByte, firstSeen string) *StoreTx {
+		return &StoreTx{
+			RawHex:      "11" + pathByte + "aabb",
+			FirstSeen:   firstSeen,
+			PayloadType: &pt,
+			DecodedJSON: `{"pubKey":"` + pk + `","type":"ADVERT"}`,
+		}
+	}
+	ps.byPayloadType[4] = []*StoreTx{
+		mk("41", newer), // 2-byte, newest, appended first
+		mk("01", older), // 1-byte, oldest
+	}
+
+	info := ps.GetNodeHashSizeInfo()
+	ni := info[pk]
+	if ni == nil {
+		t.Fatalf("expected hash size info for %s", pk)
+	}
+	if ni.HashSize != 2 {
+		t.Errorf("HashSize = %d, want 2 (newest advert, despite mixed timestamp formats)", ni.HashSize)
+	}
+}
+
+// A node still genuinely flip-flopping (recent adverts disagree) must remain
+// flagged — the decay only clears settled nodes, not active flappers.
+func TestIssue1726_ActiveFlapperStaysInconsistent(t *testing.T) {
+	ps := NewPacketStore(nil, nil)
+	pk := "ffff111122223333444455556666777788889999aaaabbbbccccddddeeeeffff"
+
+	ps.byPayloadType[4] = []*StoreTx{
+		hashAdvertTx(pk, 2, ts(-4*24*time.Hour)),
+		hashAdvertTx(pk, 1, ts(-3*24*time.Hour)),
+		hashAdvertTx(pk, 2, ts(-2*24*time.Hour)),
+		hashAdvertTx(pk, 1, ts(-1*24*time.Hour)),
+		hashAdvertTx(pk, 2, ts(-1*time.Hour)),
+	}
+
+	info := ps.GetNodeHashSizeInfo()
+	ni := info[pk]
+	if ni == nil {
+		t.Fatalf("expected hash size info for %s", pk)
+	}
+	if !ni.Inconsistent {
+		t.Error("node whose recent adverts disagree must stay flagged inconsistent")
+	}
+}
@@ -8448,7 +8448,19 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo {
 	s.mu.RLock()
 	defer s.mu.RUnlock()

-	info := make(map[string]*hashSizeNodeInfo)
+	// Collect (timestamp, hashSize) per pubkey so we can order adverts
+	// chronologically below. byPayloadType iteration is insertion order, which
+	// is not guaranteed to be chronological (out-of-order MQTT ingest, chunked
+	// cold-load), so we must sort by timestamp before reasoning about "latest"
+	// or "most recent" adverts. We parse FirstSeen rather than string-compare it
+	// so ordering is robust to timestamp-format differences (RFC3339 with or
+	// without fractional seconds); an unparseable/empty FirstSeen sorts oldest
+	// so it can never masquerade as the latest advert.
+	type hsEntry struct {
+		ts   time.Time
+		size int
+	}
+	entries := make(map[string][]hsEntry)

 	cutoff := time.Now().UTC().Add(-7 * 24 * time.Hour).Format("2006-01-02T15:04:05.000Z")

@@ -8504,26 +8516,34 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo {
 			continue
 		}

-		ni := info[pk]
-		if ni == nil {
-			ni = &hashSizeNodeInfo{AllSizes: make(map[int]bool)}
-			info[pk] = ni
-		}
-		ni.AllSizes[hs] = true
-		ni.Seq = append(ni.Seq, hs)
+		// time.Parse(time.RFC3339, ...) accepts both the ingestor's no-fraction
+		// form ("...05Z") and a fractional form ("...05.000Z"). Zero time on
+		// parse failure → sorts oldest.
+		ts, _ := time.Parse(time.RFC3339, tx.FirstSeen)
+		entries[pk] = append(entries[pk], hsEntry{ts: ts, size: hs})
 	}

-	// Post-process: use latest advert hash size and compute flip-flop flag.
-	// The most recent advert reflects the node's current hash size
-	// configuration. The upstream firmware bug causing stale path bytes in
-	// flood adverts was fixed (meshcore-dev/MeshCore#2154).
-	for _, ni := range info {
+	info := make(map[string]*hashSizeNodeInfo)
+	for pk, es := range entries {
+		// Order adverts chronologically. Stable sort so that adverts with equal
+		// (or unparseable) timestamps keep insertion order.
+		sort.SliceStable(es, func(i, j int) bool { return es[i].ts.Before(es[j].ts) })
+
+		ni := &hashSizeNodeInfo{AllSizes: make(map[int]bool), Seq: make([]int, len(es))}
+		for i, e := range es {
+			ni.Seq[i] = e.size
+			ni.AllSizes[e.size] = true
+		}
+		info[pk] = ni
+
 		// Use the most recent advert's hash size (last in chronological order).
+		// The upstream firmware bug causing stale path bytes in flood adverts
+		// was fixed (meshcore-dev/MeshCore#2154).
 		ni.HashSize = ni.Seq[len(ni.Seq)-1]

-		// Flip-flop (inconsistent) flag: need >= 3 observations,
+		// Flip-flop (inconsistent) flag: need a minimum number of observations,
 		// >= 2 unique sizes, and >= 2 transitions in the sequence.
-		if len(ni.Seq) < 3 || len(ni.AllSizes) < 2 {
+		if len(ni.Seq) < hashSizeMinObservations || len(ni.AllSizes) < 2 {
 			continue
 		}
 		transitions := 0
@@ -8532,12 +8552,53 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo {
 				transitions++
 			}
 		}
-		ni.Inconsistent = transitions >= 2
+		if transitions < 2 {
+			continue
+		}
+		// Recency decay (issue #1726): if the most recent adverts all agree on
+		// a single size, the node has settled on its current hash mode (e.g. an
+		// operator flipped path.hash.mode mid-flight, or a lone stale 1-byte
+		// advert sits earlier in the 7-day window). Don't keep reporting "varies"
+		// over older history once the node is consistent again. A node whose
+		// recent adverts still disagree remains flagged.
+		//
+		// Known limitation: a node that flaps slowly (long stable stretches
+		// between toggles) is not flagged during a stable stretch. This is
+		// intentional — "varies" describes the node's *current* state — and the
+		// full history stays visible via hash_sizes_seen / AllSizes.
+		if recentAdvertsAgree(ni.Seq, hashSizeRecentAgreeCount) {
+			continue
+		}
+		ni.Inconsistent = true
 	}

 	return info
 }

+// hashSizeMinObservations is the minimum number of non-zero-hop adverts in the
+// window before a node is eligible to be flagged as flip-flopping at all.
+const hashSizeMinObservations = 3
+
+// hashSizeRecentAgreeCount is how many of the most recent non-zero-hop adverts
+// must share a single hash size for a node to be considered "settled", clearing
+// its flip-flop ("varies") flag.
+const hashSizeRecentAgreeCount = 3
+
+// recentAdvertsAgree reports whether the last n entries of a chronologically
+// ordered hash-size sequence are all equal.
+func recentAdvertsAgree(seq []int, n int) bool {
+	if len(seq) < n {
+		return false
+	}
+	last := seq[len(seq)-1]
+	for i := len(seq) - n; i < len(seq)-1; i++ {
+		if seq[i] != last {
+			return false
+		}
+	}
+	return true
+}
+
 // EnrichNodeWithHashSize populates hash_size, hash_size_inconsistent, and
 // hash_sizes_seen on a node map using precomputed hash size info.
 func EnrichNodeWithHashSize(node map[string]interface{}, info *hashSizeNodeInfo) {