mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-17 01:04:20 +00:00
Closes #1816. Closes #1818 (confirmed duplicate of #1816 by the triage bot). ## Root cause `byNode` is an involvement index (`indexResolvedPathHops`, `store.go:1696-1705`, #1558/#1352): a transmission is indexed under every relay-hop pubkey found in an observation's `resolved_path`, not just its originator. `getNodeClockSkewLocked` (`clock_skew.go:489`) iterated every ADVERT transaction under a pubkey without checking who actually signed it, so a relay inherited the clock skew of every broken-clock node it forwarded as if it were its own. This produced: - Fleet-wide false `no_clock`/`bimodal_clock` classifications on healthy relays whose only "bad" samples were adverts they merely relayed. - Bit-identical `RecentMedianSkewSec` "clusters" across unrelated relays that all forwarded the same broken-clock originator. - Single relays showing a multi-day skew even though their own self-adverts are healthy, because 1-2 relayed adverts from a broken originator landed in the tail of their small recent-window sample (the #1818 "island" repro from @cwichura). ## Fix Add `txOriginatedBy(tx, pubkey)`: ADVERTs are self-signed, so `decoded["pubKey"]` is the originator per protocol (case-insensitive compare as a defensive measure). Apply it as a guard in both the main skew-aggregation loop and the per-hash evidence loop in `getNodeClockSkewLocked`. `byNode` itself is untouched — #1558/#1352 still rely on the broader involvement index for other consumers. ## Tests - Existing `clock_skew_test.go` / `clock_skew_issue1094_test.go` / `clock_skew_issue1285_test.go` fixtures built synthetic ADVERT transactions without a `pubKey` field and seeded `s.byNode` directly, bypassing the normal `indexByNode` path where every real ADVERT carries `pubKey`. Added `pubKey` to each fixture so it reflects a self-originated advert, which is what these tests already intended to represent. All pre-existing tests pass unchanged in behavior. - New `clock_skew_issue1816_test.go`: - `TestTxOriginatedBy` — unit coverage of the new guard (self, foreign, missing pubKey, case-insensitivity). - `TestIssue1816_RelayDoesNotInheritOriginatorSkew` — a relay with healthy self-adverts plus relayed adverts from a broken-clock originator (matching the report's +100.5k s band) must report `ok` severity based only on its own adverts. - `TestIssue1816_PureRelaysReportNoSkew_NoBitIdenticalCluster` — five relay pubkeys that only ever forward a broken originator's advert (never self-advert) must report `nil`, not a bit-identical copy of the originator's skew. - `TestIssue1818_TwoForeignAdvertsDoNotPoisonIslandNode` — reproduces the cwichura island scenario: 8 healthy self-adverts + 2 foreign adverts at ~10 days skew must not flip severity or pollute `RecentMedianSkewSec`. Full suite: `go test ./...` passes (one pre-existing, unrelated flaky test — `TestHandleNodePaths_PrefixCollision_1352`, an index-loading race — reproduces intermittently on unmodified `master` too). Operator context: running CoreScope for SaarMesh (SaarLorLux, DE/FR/LU, 800+ nodes); this bug was surfacing as fleet-wide clock-skew false positives on our infra nodes. Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
110 lines
3.4 KiB
Go
110 lines
3.4 KiB
Go
package main
|
|
|
|
// Regression test for #1094: the bimodal-clock warning currently exposes only
|
|
// RecentBadSampleCount, leaving the UI to render "⚠️ N of M adverts had
|
|
// nonsense timestamps" without telling the operator WHICH packets were bad.
|
|
//
|
|
// This test pins the additive API contract: alongside the count, the response
|
|
// must expose RecentBadSamples — a slice of (hash, advertTS, skewSec) — so the
|
|
// frontend can render each offending hash as a clickable link with its bad
|
|
// timestamp.
|
|
|
|
import (
|
|
"testing"
|
|
"time"
|
|
)
|
|
|
|
// Seeds 5 recent adverts: 3 healthy (~-20s skew) and 2 with a "nonsense"
|
|
// bimodal-bad timestamp (|skew| in (1h, 24h]). The recent window is exactly
|
|
// 5 samples, so all five are inside it.
|
|
func seedIssue1094Repro(t *testing.T) (*PacketStore, []string, []int64) {
|
|
t.Helper()
|
|
ps := NewPacketStore(nil, nil)
|
|
pt := 4 // ADVERT
|
|
|
|
const pubkey = "BADTS1094"
|
|
baseObs := int64(1779000000)
|
|
|
|
var txs []*StoreTx
|
|
var badHashes []string
|
|
var badAdvertTSs []int64
|
|
|
|
// 3 healthy adverts (skew = -20s).
|
|
for i := 0; i < 3; i++ {
|
|
obsTS := baseObs + int64(i)*60
|
|
advTS := obsTS - 20
|
|
txs = append(txs, &StoreTx{
|
|
Hash: "healthy-1094-" + formatInt64(int64(i)),
|
|
PayloadType: &pt,
|
|
DecodedJSON: `{"payload":{"timestamp":` + formatInt64(advTS) + `},"pubKey":"BADTS1094"}`,
|
|
Observations: []*StoreObs{
|
|
{ObserverID: "obs1", Timestamp: time.Unix(obsTS, 0).UTC().Format(time.RFC3339)},
|
|
},
|
|
})
|
|
}
|
|
|
|
// 2 nonsense-timestamp adverts (skew = -7200s = -2h — bimodal-bad,
|
|
// below the 24h RTC-reset exclusion so they DO count in recentBadCount).
|
|
for i := 0; i < 2; i++ {
|
|
obsTS := baseObs + int64(3+i)*60
|
|
advTS := obsTS - 7200
|
|
hash := "bad-1094-" + formatInt64(int64(i))
|
|
txs = append(txs, &StoreTx{
|
|
Hash: hash,
|
|
PayloadType: &pt,
|
|
DecodedJSON: `{"payload":{"timestamp":` + formatInt64(advTS) + `},"pubKey":"BADTS1094"}`,
|
|
Observations: []*StoreObs{
|
|
{ObserverID: "obs1", Timestamp: time.Unix(obsTS, 0).UTC().Format(time.RFC3339)},
|
|
},
|
|
})
|
|
badHashes = append(badHashes, hash)
|
|
badAdvertTSs = append(badAdvertTSs, advTS)
|
|
}
|
|
|
|
ps.mu.Lock()
|
|
ps.byNode[pubkey] = txs
|
|
for _, tx := range txs {
|
|
ps.byPayloadType[4] = append(ps.byPayloadType[4], tx)
|
|
}
|
|
ps.clockSkew.computeInterval = 0
|
|
ps.mu.Unlock()
|
|
return ps, badHashes, badAdvertTSs
|
|
}
|
|
|
|
func TestIssue1094_RecentBadSamples_ExposesHashAndTimestamp(t *testing.T) {
|
|
ps, wantHashes, wantAdvertTSs := seedIssue1094Repro(t)
|
|
r := ps.GetNodeClockSkew("BADTS1094")
|
|
if r == nil {
|
|
t.Fatal("expected clock skew result")
|
|
}
|
|
|
|
// Pre-condition: count must already be 2 (gates the test against the
|
|
// existing field — if this drops we'd be measuring the wrong thing).
|
|
if r.RecentBadSampleCount != 2 {
|
|
t.Fatalf("RecentBadSampleCount = %d, want 2 (seed bug, not the field-under-test)",
|
|
r.RecentBadSampleCount)
|
|
}
|
|
|
|
if len(r.RecentBadSamples) != 2 {
|
|
t.Fatalf("RecentBadSamples len = %d, want 2 — operators need to see which "+
|
|
"adverts had nonsense timestamps, not just the count",
|
|
len(r.RecentBadSamples))
|
|
}
|
|
|
|
gotByHash := map[string]int64{}
|
|
for _, bs := range r.RecentBadSamples {
|
|
gotByHash[bs.Hash] = bs.AdvertTS
|
|
}
|
|
for i, h := range wantHashes {
|
|
ts, ok := gotByHash[h]
|
|
if !ok {
|
|
t.Errorf("RecentBadSamples missing hash %q", h)
|
|
continue
|
|
}
|
|
if ts != wantAdvertTSs[i] {
|
|
t.Errorf("RecentBadSamples[%q].AdvertTS = %d, want %d (the bad advertTS)",
|
|
h, ts, wantAdvertTSs[i])
|
|
}
|
|
}
|
|
}
|