diff --git a/cmd/server/count_sort_tie_order_321_test.go b/cmd/server/count_sort_tie_order_321_test.go new file mode 100644 index 000000000..d91cc2549 --- /dev/null +++ b/cmd/server/count_sort_tie_order_321_test.go @@ -0,0 +1,220 @@ +package main + +import ( + "fmt" + "sort" + "testing" + "time" +) + +// Issue #321: four more count-only sorts (plus the hash-collision list) are +// built by ranging over a map and were sorted on the count alone. Go map +// iteration order is random, so entries with equal counts came out in a +// different order on each call, and where the list is capped (peerInteractions +// at 20) it was random *which* tied entries made the cut. Same bug class and +// same fix as rankSubpaths (#256/#293) and GetSubpathDetail (#319): give every +// comparator a final, unique tie-break key. +// +// Each test recomputes its list tieOrderRuns times (see +// analytics_tie_order_256_test.go) and requires the same, fully specified +// order every time, so the failure before the fix is deterministic rather +// than flaky. + +// scrambledKeys returns n keys with the given prefix whose insertion order is +// deliberately not their sorted order, so a comparator that leaves ties to map +// iteration cannot accidentally produce the expected sequence. +func scrambledKeys(prefix string, n int) []string { + out := make([]string, 0, n) + for i := 0; i < n; i++ { + out = append(out, fmt.Sprintf("%s-%04x", prefix, (i*37)%256+0x1000)) + } + return out +} + +// newDisplayArrayAccumulator builds an accumulator in display-array mode with +// the observer and peer detail maps the two node-analytics lists are ranked +// from. finalizeDisplayArrays only reads those maps for these two lists. +func newDisplayArrayAccumulator(obs map[string]int, peers map[string]int) *nodeAnalyticsAccumulator { + a := newNodeAnalyticsAccumulator("node-321", true) + for id, n := range obs { + a.obsDetail[id] = &nodeAnalyticsObsAccum{ + name: "Name-" + id, + count: n, + first: "2026-05-01T12:00:00Z", + last: "2026-05-01T13:00:00Z", + } + } + for key, n := range peers { + a.peerDetail[key] = &nodeAnalyticsPeerAccum{ + key: key, name: "Peer-" + key, count: n, + lastContact: "2026-05-01T12:00:00Z", + } + } + return a +} + +func TestNodeAnalyticsObserverCoverageTieOrder_321(t *testing.T) { + // observerCoverage has no cap, so only the order was random. 12 observers + // tie on 4 packets, one is seen 9 times and one twice. + tied := scrambledKeys("obs", 12) + obs := map[string]int{"obs-high": 9, "obs-low": 2} + for _, id := range tied { + obs[id] = 4 + } + a := newDisplayArrayAccumulator(obs, nil) + + sorted := append([]string(nil), tied...) + sort.Strings(sorted) + want := append([]string{"obs-high"}, sorted...) + want = append(want, "obs-low") + assertSameEveryRun(t, "observerCoverage observer_id", want, func() interface{} { + _, _, _, coverage, _, _, _ := a.finalizeDisplayArrays() + out := []string{} + for _, e := range coverage { + out = append(out, e.ObserverID.(string)) + } + return out + }) +} + +func TestNodeAnalyticsPeerInteractionsTieOrder_321(t *testing.T) { + // peerInteractions is capped at 20, so the tie decided *which* peers the + // node-analytics page showed, not just their order. 30 peers tie on 3 + // messages and one has 9, so 19 of the 30 tied peers make the cut. + tied := scrambledKeys("peer", 30) + peers := map[string]int{"peer-high": 9} + for _, key := range tied { + peers[key] = 3 + } + a := newDisplayArrayAccumulator(nil, peers) + + sorted := append([]string(nil), tied...) + sort.Strings(sorted) + want := append([]string{"peer-high"}, sorted[:19]...) + assertSameEveryRun(t, "peerInteractions peer_key (top 20)", want, func() interface{} { + _, _, _, _, _, peerInteractions, _ := a.finalizeDisplayArrays() + out := []string{} + for _, e := range peerInteractions { + out = append(out, e.PeerKey) + } + return out + }) + if len(want) != 20 { + t.Fatalf("expected the cap to bind at 20, want has %d entries", len(want)) + } +} + +// newHealthTieStore seeds one node row plus in-memory packets so that each +// observer in counts heard the node exactly that many times. +func newHealthTieStore(t *testing.T, pubkey string, counts map[string]int) *PacketStore { + t.Helper() + db := setupTestDBv2(t) + mustExecDB(t, db, `INSERT INTO nodes (public_key, name, role, lat, lon, last_seen, first_seen) + VALUES ('`+pubkey+`', 'Node 321', 'repeater', 0, 0, '2026-05-01T13:00:00Z', '2026-05-01T12:00:00Z')`) + s := newTestStoreWithDB(t, db, &Config{}) + snr := 5.0 + n := 0 + for obsID, c := range counts { + for i := 0; i < c; i++ { + n++ + s.byNode[pubkey] = append(s.byNode[pubkey], &StoreTx{ + ID: n, Hash: fmt.Sprintf("hl%d", n), FirstSeen: "2026-05-01T12:00:00Z", + SNR: &snr, ObservationCount: 1, + ObserverID: obsID, ObserverName: "Name-" + obsID, + }) + } + } + return s +} + +// healthTieCounts is the shared shape for both health endpoints: 12 observers +// tie on 4 packets, one is heard 9 times and one twice. +func healthTieCounts() (counts map[string]int, want []string) { + tied := scrambledKeys("obs", 12) + counts = map[string]int{"obs-high": 9, "obs-low": 2} + for _, id := range tied { + counts[id] = 4 + } + sorted := append([]string(nil), tied...) + sort.Strings(sorted) + want = append([]string{"obs-high"}, sorted...) + want = append(want, "obs-low") + return counts, want +} + +func TestNodeHealthObserverRowsTieOrder_321(t *testing.T) { + const pubkey = "aa11bb22cc33dd44" + counts, want := healthTieCounts() + s := newHealthTieStore(t, pubkey, counts) + + assertSameEveryRun(t, "GetNodeHealth observers observer_id", want, func() interface{} { + res, err := s.GetNodeHealth(pubkey) + if err != nil { + t.Fatalf("GetNodeHealth: %v", err) + } + return mapField(res["observers"], "observer_id") + }) +} + +func TestBulkHealthObserverRowsTieOrder_321(t *testing.T) { + const pubkey = "bb11cc22dd33ee44" + counts, want := healthTieCounts() + s := newHealthTieStore(t, pubkey, counts) + + assertSameEveryRun(t, "GetBulkHealth observers observer_id", want, func() interface{} { + rows := s.GetBulkHealth(10, "", "") + if len(rows) != 1 { + t.Fatalf("expected 1 bulk-health row, got %d", len(rows)) + } + return mapField(rows[0]["observers"], "observer_id") + }) +} + +func TestHashCollisionsTieOrder_321(t *testing.T) { + // Six 2-byte prefixes, each shared by exactly two coordinate-less + // repeaters. Every entry therefore ties on both existing sort keys — + // classification ("incomplete") and Appearances (2) — so the order was + // pure map iteration order. + db := setupTestDBv2(t) + prefixes := []string{"1000", "1025", "104a", "106f", "1094", "10b9"} + var adverts []*StoreTx + now := time.Now().UTC() + for _, p := range prefixes { + for j := 0; j < 2; j++ { + pk := fmt.Sprintf("%s%02dcafebabe00", p, j) + mustExecDB(t, db, `INSERT INTO nodes (public_key, name, role, lat, lon, last_seen, first_seen) + VALUES ('`+pk+`', 'R-`+pk+`', 'repeater', 0, 0, '2026-05-01T13:00:00Z', '2026-05-01T12:00:00Z')`) + // Path byte 0x41 declares 2-byte hashes, so HashSize == 2. + adv := makeHashSizeAdvert(pk, "R-"+pk, now.Add(-time.Duration(j)*time.Minute).Format(time.RFC3339)) + adverts = append(adverts, adv) + } + } + s := newTestStoreWithDB(t, db, &Config{}) + s.byPayloadType[4] = adverts + + want := make([]string, 0, len(prefixes)) + for _, p := range prefixes { + want = append(want, fmt.Sprintf("%04X", mustParseHex(t, p))) + } + sort.Strings(want) + assertSameEveryRun(t, "hash collisions prefix (2-byte)", want, func() interface{} { + s.hashSizeInfoCache = nil + res := s.computeHashCollisions("", "") + bySize := res["by_size"].(map[string]interface{}) + two := bySize["2"].(map[string]interface{}) + out := []string{} + for _, c := range two["collisions"].([]collisionEntry) { + out = append(out, c.Prefix) + } + return out + }) +} + +func mustParseHex(t *testing.T, s string) int { + t.Helper() + var v int + if _, err := fmt.Sscanf(s, "%x", &v); err != nil { + t.Fatalf("parse %q: %v", s, err) + } + return v +} diff --git a/cmd/server/node_analytics_summary.go b/cmd/server/node_analytics_summary.go index dc7cc3d5d..a897e5625 100644 --- a/cmd/server/node_analytics_summary.go +++ b/cmd/server/node_analytics_summary.go @@ -388,8 +388,15 @@ func (a *nodeAnalyticsAccumulator) finalizeDisplayArrays() ( LastSeen: o.last, }) } + // #321: obsDetail iteration order is random, so observers tied on + // PacketCount came out in a different order on each call. ObserverID is + // the map key the entry was built from, so it is unique and makes the + // order total (sibling of rankSubpaths/#256 and GetSubpathDetail/#273). sort.Slice(observerCoverage, func(i, j int) bool { - return observerCoverage[i].PacketCount > observerCoverage[j].PacketCount + if observerCoverage[i].PacketCount != observerCoverage[j].PacketCount { + return observerCoverage[i].PacketCount > observerCoverage[j].PacketCount + } + return observerCoverage[i].ObserverID.(string) < observerCoverage[j].ObserverID.(string) }) hopDistribution = make([]HopDistEntry, 0) @@ -406,8 +413,14 @@ func (a *nodeAnalyticsAccumulator) finalizeDisplayArrays() ( MessageCount: pm.count, LastContact: pm.lastContact, }) } + // #321: peerDetail iteration order is random and this list is capped at + // 20, so with ties at the cut-off it was random *which* peers the client + // showed, not just their order. PeerKey is the map key, so it is unique. sort.Slice(peerInteractions, func(i, j int) bool { - return peerInteractions[i].MessageCount > peerInteractions[j].MessageCount + if peerInteractions[i].MessageCount != peerInteractions[j].MessageCount { + return peerInteractions[i].MessageCount > peerInteractions[j].MessageCount + } + return peerInteractions[i].PeerKey < peerInteractions[j].PeerKey }) if len(peerInteractions) > 20 { peerInteractions = peerInteractions[:20] diff --git a/cmd/server/store.go b/cmd/server/store.go index 661d2f8b5..0611d4029 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -9619,12 +9619,19 @@ func (s *PacketStore) computeHashCollisions(region, area string) map[string]inte // Sort: local first, then regional, distant, incomplete classOrder := map[string]int{"local": 0, "regional": 1, "distant": 2, "incomplete": 3, "unknown": 4} + // #321: prefixMap iteration order is random, so entries tied on both + // class and Appearances came out in a different order on each call. + // The prefix is the map key the entry was built from, so it is unique + // and makes the order total. sort.Slice(collisions, func(i, j int) bool { oi, oj := classOrder[collisions[i].Classification], classOrder[collisions[j].Classification] if oi != oj { return oi < oj } - return collisions[i].Appearances > collisions[j].Appearances + if collisions[i].Appearances != collisions[j].Appearances { + return collisions[i].Appearances > collisions[j].Appearances + } + return collisions[i].Prefix < collisions[j].Prefix }) // Stats @@ -10403,8 +10410,15 @@ func (s *PacketStore) GetBulkHealth(limit int, region, area string) []map[string "avgSnr": avgSnr, "avgRssi": avgRssi, "packetCount": o.count, }) } + // #321: observerStats iteration order is random, so observers tied + // on packetCount came out in a different order on each call. The + // observer_id is the map key the row was built from, so it is unique + // and makes the order total (sibling of GetSubpathDetail/#273). sort.Slice(observerRows, func(i, j int) bool { - return observerRows[i]["packetCount"].(int) > observerRows[j]["packetCount"].(int) + if observerRows[i]["packetCount"].(int) != observerRows[j]["packetCount"].(int) { + return observerRows[i]["packetCount"].(int) > observerRows[j]["packetCount"].(int) + } + return observerRows[i]["observer_id"].(string) < observerRows[j]["observer_id"].(string) }) var avgSnr *float64 @@ -10571,8 +10585,15 @@ func (s *PacketStore) GetNodeHealth(pubkey string) (map[string]interface{}, erro "can_relay": canRelay, }) } + // #321: observerStats iteration order is random, so observers tied on + // packetCount came out in a different order on each call. The observer_id + // is the map key the row was built from, so it is unique and makes the + // order total (sibling of GetSubpathDetail/#273). sort.Slice(observerRows, func(i, j int) bool { - return observerRows[i]["packetCount"].(int) > observerRows[j]["packetCount"].(int) + if observerRows[i]["packetCount"].(int) != observerRows[j]["packetCount"].(int) { + return observerRows[i]["packetCount"].(int) > observerRows[j]["packetCount"].(int) + } + return observerRows[i]["observer_id"].(string) < observerRows[j]["observer_id"].(string) }) var avgSnr *float64