mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-11-11 22:18:02 +00:00
Fixes #2237. Ingest appends to `tx.Observations` and rewrites a transmission's header fields (`LatestSeen`, observer, path, counts) under `s.mu.Lock`. Several readers touched that state without the lock. This PR follows the approach of dborup/CoreScope#475, the reporter's fix: copy what each reader needs under the RLock it already takes, and sort or encode outside it. That fix does not apply to upstream as is (the fork has diverged), so this is a rewrite against current master. ## What changes, per site | Site | Before | After | |---|---|---| | `QueryGroupedPackets` (`store.go`) | Sort read `tx.LatestSeen` after `RUnlock`; the page (`groupedTxsToPage`, `storeTxDistinctIatas`) was built from the shared cache with no lock | The sort key is copied under the existing RLock (`groupedEntry`). The cache holds those entries. `groupedPage` copies only the requested page's rows under a short RLock; the maps are built after it | | `txGetParsedPath` (`store.go`) | Filled `tx.parsedPath` lazily under RLock, so concurrent readers wrote the same fields | `pickBestObservation` fills it through `setTxPath` where the path is written, under the write lock or before the tx is shared | | `BuildFromStoreWithOptions` (`neighbor_graph.go`) | Ranged `tx.Observations` after releasing the lock | Each tx's `Observations` slice header is copied with the packet list | | `/api/audio-lab/buckets` (`routes.go`) | Read path, observer and count after `RUnlock` | The sort stays outside the lock (`RawHex` never changes); the picked rows are read under a second short RLock | | `trackedBytes` (`/api/stats`, perf stats) | Read with no lock | Read under `s.mu` | | Observer analytics (`routes.go`, `observer_analytics.go`) | Read `tx.Hash` after `RUnlock`; the content-hash migration rewrites it | `txByID` holds copies made under the existing RLock (`txAnalyticsFields`) | `/api/nodes/{pk}/paths` and `GetNodeHopAnalytics`, items 2 in the issue, are not changed: on current master they already copy under the lock (`snapshotRPObs`) or run inside it. Appending to `tx.Observations` never changes the elements an already copied slice header covers, so copying the header is enough there. ## Tests `store_read_race_2237_test.go` adds one `-race` regression test per site. Each runs an ingest-style writer under `s.mu.Lock` against the reader: - `TestQueryGroupedPacketsIngestRace2237` (cold and cached paths) - `TestTxGetParsedPathReadersRace2237` - `TestBuildFromStoreIngestRace2237` - `TestAudioLabBucketsIngestRace2237` - `TestPerfStoreStatsTrackedBytesRace2237` - `TestStatsTrackedBytesRace2237` - `TestObserverAnalyticsHashMigrationRace2237` Plus `TestGroupedPageReflectsHeaderAfterIngest2237` (no race detector): header fields, `distinct_iatas`, count and order after ingest. How I checked them: - With the fix, `go test -race -count=3 -run 'Race2237'` passes. - Each site reverted on its own, one at a time, makes its test report `DATA RACE` (between 1 and 24 reports per site). - `go test ./...` in `cmd/server` passes; `go vet` and `gofmt -l` are clean. ## Performance `QueryGroupedPackets` is on the packets page's hot path. `BenchmarkQueryGroupedPackets2237`, 30K transmissions, median of 8 alternating runs per binary (Windows, noisy: single runs varied up to 2x): | Case | Before (ns/op) | After (ns/op) | B/op before → after | |---|---|---|---| | cold, limit 50 | 1,015,512 | 1,022,879 (+1%) | 1.62 MB → 2.10 MB | | cold, limit 1000 | 1,711,237 | 1,793,032 (+5%) | 2.93 MB → 3.64 MB | | cached, limit 50 | 29,439 | 32,608 (+11%, about 3 µs) | 69 KB → 82 KB | | cached, limit 1000 | 606,970 | 662,204 (+9%) | 1.38 MB → 1.61 MB | The extra cost is copying the page's rows. The lock is held for that copy only (at most `limit` rows), never for the sort or for building the maps. ## Not done - `BuildFromStoreWithOptions` still calls `tx.ParsedDecoded()` outside the lock, and `tx_rewrite_feed.go` resets `decodedOnce` / `parsedDecoded` under the write lock. That pair is still a race. Fixing it means parsing under the RLock on the first build, so I left it for a follow-up rather than widen this PR. - I ran the new tests under `-race`, not the whole package. - No load test against a live server; backend only, no browser check. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
232 lines
7.9 KiB
Go
232 lines
7.9 KiB
Go
// Package main — observer analytics helpers.
|
||
//
|
||
// #1828 Phase A: extracted from cmd/server/routes.go handleObserverAnalytics
|
||
// (routes.go:2819-2953). Split the 5 aggregate builders into standalone
|
||
// functions for readability + isolated tests. No behavior change relative to
|
||
// the pre-#1828 handler; JSON output is byte-identical.
|
||
//
|
||
// The `filtered` argument is the time-window-filtered, timestamp-desc sorted
|
||
// slice of *StoreObs produced by the handler after snapshotting under RLock
|
||
// (see #1481 P0-2). Helpers do NOT touch store.mu — the handler owns lock
|
||
// scoping.
|
||
//
|
||
// Concurrency note (#1830): buildPacketTypes / buildNodesTimeline /
|
||
// buildRecentPackets take a txByID map instead of *PacketStore for
|
||
// transmission lookups. store.byTxID is guarded by store.mu (writes from
|
||
// ingest/eviction); reading it here — after the handler's RLock snapshot has
|
||
// already been released, to keep JSON decode/enrichment off the hot lock per
|
||
// #1481 P0-2 — would race with those writers (Go maps can panic with
|
||
// "concurrent map read and map write" during a rehash, not just fail under
|
||
// -race). The handler resolves txByID once, under the same RLock that
|
||
// snapshots the observation slice, and passes it down instead.
|
||
//
|
||
// Perf note (#1839 MINOR): dropping enrichObs on the histogram / nodes-
|
||
// timeline paths also eliminates N fetchResolvedPathForObs SQL calls per
|
||
// /analytics request (store.go:fetchResolvedPathForObs), not just the alloc/
|
||
// boxing win — /analytics SQL-load drops materially.
|
||
package main
|
||
|
||
import (
|
||
"encoding/json"
|
||
"fmt"
|
||
"sort"
|
||
"strconv"
|
||
"time"
|
||
)
|
||
|
||
// observerAnalyticsBucketDur returns the timeline bucket size for a given day
|
||
// range. Mirrors the constant table in the legacy handler.
|
||
func observerAnalyticsBucketDur(days int) time.Duration {
|
||
bucketDur := 24 * time.Hour
|
||
if days <= 1 {
|
||
bucketDur = time.Hour
|
||
} else if days <= 7 {
|
||
bucketDur = 4 * time.Hour
|
||
}
|
||
return bucketDur
|
||
}
|
||
|
||
// observerAnalyticsFormatLabel returns a closure that formats a bucket time
|
||
// as a human-readable label appropriate for the day range.
|
||
func observerAnalyticsFormatLabel(days int) func(time.Time) string {
|
||
return func(t time.Time) string {
|
||
if days <= 1 {
|
||
return t.UTC().Format("15:04")
|
||
}
|
||
if days <= 7 {
|
||
return t.UTC().Format("Mon 15:04")
|
||
}
|
||
return t.UTC().Format("Jan 02")
|
||
}
|
||
}
|
||
|
||
// buildTimelineBuckets is the shared kernel used by both buildTimeline and
|
||
// buildNodesTimeline. Sorts the count map by bucket-key and returns
|
||
// TimeBucket entries with labels formatted per days.
|
||
func buildTimelineBuckets(counts map[int64]int, days int) []TimeBucket {
|
||
formatLabel := observerAnalyticsFormatLabel(days)
|
||
keys := make([]int64, 0, len(counts))
|
||
for k := range counts {
|
||
keys = append(keys, k)
|
||
}
|
||
sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] })
|
||
out := make([]TimeBucket, 0, len(keys))
|
||
for _, k := range keys {
|
||
lbl := formatLabel(time.Unix(k, 0))
|
||
out = append(out, TimeBucket{Label: &lbl, Count: counts[k]})
|
||
}
|
||
return out
|
||
}
|
||
|
||
// buildTimeline builds the packet-timeline aggregate for /analytics.
|
||
func buildTimeline(filtered []*StoreObs, days int) []TimeBucket {
|
||
bucketDur := observerAnalyticsBucketDur(days)
|
||
counts := map[int64]int{}
|
||
for _, obs := range filtered {
|
||
ts, ok := obs.ParsedTime()
|
||
if !ok {
|
||
continue
|
||
}
|
||
bucketStart := ts.UTC().Truncate(bucketDur).Unix()
|
||
counts[bucketStart]++
|
||
}
|
||
return buildTimelineBuckets(counts, days)
|
||
}
|
||
|
||
// buildPacketTypes builds the payload-type histogram. Uses a direct
|
||
// store.byTxID read (see #1828 triage) rather than enrichObs — the loop only
|
||
// needs payload_type, which is a single indirection off the transmission.
|
||
// This avoids one map allocation + several interface-boxing conversions per
|
||
// observation (routes.go:2886 hot path pre-#1828).
|
||
func buildPacketTypes(filtered []*StoreObs, txByID map[int]*StoreTx) map[string]int {
|
||
out := map[string]int{}
|
||
for _, obs := range filtered {
|
||
tx := txByID[obs.TransmissionID]
|
||
if tx == nil || tx.PayloadType == nil {
|
||
continue
|
||
}
|
||
out[strconv.Itoa(*tx.PayloadType)]++
|
||
}
|
||
return out
|
||
}
|
||
|
||
// buildNodesTimeline builds the distinct-node-per-bucket timeline aggregate.
|
||
// Nodes = path-json hops ∪ decoded_json pubKey/srcHash/destHash.
|
||
func buildNodesTimeline(filtered []*StoreObs, days int, txByID map[int]*StoreTx) []TimeBucket {
|
||
bucketDur := observerAnalyticsBucketDur(days)
|
||
nodeBucketSets := map[int64]map[string]struct{}{}
|
||
for _, obs := range filtered {
|
||
ts, ok := obs.ParsedTime()
|
||
if !ok {
|
||
continue
|
||
}
|
||
bucketStart := ts.UTC().Truncate(bucketDur).Unix()
|
||
if nodeBucketSets[bucketStart] == nil {
|
||
nodeBucketSets[bucketStart] = map[string]struct{}{}
|
||
}
|
||
// Legacy handler read decoded_json via enrichObs (which pulls it off
|
||
// tx.DecodedJSON). Read tx directly for parity + savings.
|
||
if tx := txByID[obs.TransmissionID]; tx != nil && tx.DecodedJSON != "" {
|
||
var decoded map[string]interface{}
|
||
if json.Unmarshal([]byte(tx.DecodedJSON), &decoded) == nil {
|
||
for _, k := range []string{"pubKey", "srcHash", "destHash"} {
|
||
if v, ok := decoded[k].(string); ok && v != "" {
|
||
nodeBucketSets[bucketStart][v] = struct{}{}
|
||
}
|
||
}
|
||
}
|
||
}
|
||
for _, hop := range parsePathJSON(obs.PathJSON) {
|
||
if hop != "" {
|
||
nodeBucketSets[bucketStart][hop] = struct{}{}
|
||
}
|
||
}
|
||
}
|
||
nodeCounts := make(map[int64]int, len(nodeBucketSets))
|
||
for k, nodes := range nodeBucketSets {
|
||
nodeCounts[k] = len(nodes)
|
||
}
|
||
return buildTimelineBuckets(nodeCounts, days)
|
||
}
|
||
|
||
// buildSnrDistribution builds the SNR histogram (2-unit buckets, floor).
|
||
func buildSnrDistribution(filtered []*StoreObs) []SnrDistributionEntry {
|
||
snrBuckets := map[int]*SnrDistributionEntry{}
|
||
for _, obs := range filtered {
|
||
if obs.SNR == nil {
|
||
continue
|
||
}
|
||
bucket := int(*obs.SNR) / 2 * 2
|
||
if *obs.SNR < 0 && int(*obs.SNR) != bucket {
|
||
bucket -= 2
|
||
}
|
||
if snrBuckets[bucket] == nil {
|
||
snrBuckets[bucket] = &SnrDistributionEntry{Range: fmt.Sprintf("%d to %d", bucket, bucket+2)}
|
||
}
|
||
snrBuckets[bucket].Count++
|
||
}
|
||
keys := make([]int, 0, len(snrBuckets))
|
||
for k := range snrBuckets {
|
||
keys = append(keys, k)
|
||
}
|
||
sort.Ints(keys)
|
||
out := make([]SnrDistributionEntry, 0, len(keys))
|
||
for _, k := range keys {
|
||
out = append(out, *snrBuckets[k])
|
||
}
|
||
return out
|
||
}
|
||
|
||
// buildRecentPackets builds the "first N enriched observations" list. This is
|
||
// the only aggregate that needs the full enrichObs map — recentPackets is a
|
||
// UI-facing payload and the extra fields matter here.
|
||
//
|
||
// Legacy parity (#1839): the pre-#1828 routes.go loop was
|
||
//
|
||
// for i, obs := range filtered {
|
||
// if _, ok := obs.ParsedTime(); !ok { continue }
|
||
// ...
|
||
// if i < limit { recentPackets = append(..., enriched) }
|
||
// }
|
||
//
|
||
// The `i` in the gate is the RAW slice index — a bad-ts obs at position k<limit
|
||
// consumed its slot (via `continue`) and could not be replaced by a later
|
||
// good-ts obs. Result can be <limit when bad-ts obs sit in the head of
|
||
// `filtered`. We reproduce that exact semantic here to keep output byte-
|
||
// identical.
|
||
func buildRecentPackets(store *PacketStore, filtered []*StoreObs, limit int, txByID map[int]*StoreTx) []map[string]interface{} {
|
||
if limit <= 0 {
|
||
return []map[string]interface{}{}
|
||
}
|
||
out := make([]map[string]interface{}, 0, limit)
|
||
for i, obs := range filtered {
|
||
if i >= limit {
|
||
break
|
||
}
|
||
if _, ok := obs.ParsedTime(); !ok {
|
||
continue
|
||
}
|
||
out = append(out, store.enrichObsWithTx(obs, txByID[obs.TransmissionID]))
|
||
}
|
||
return out
|
||
}
|
||
|
||
// txAnalyticsFields returns a detached copy of the tx fields the helpers in
|
||
// this file read (hash, raw hex, payload and route type, decoded JSON), or
|
||
// nil for a nil tx. Caller holds store.mu: the content-hash migration and the
|
||
// rewrite feed change tx.Hash and tx.DecodedJSON under it, and the helpers
|
||
// run after the handler released it (#2237).
|
||
func txAnalyticsFields(tx *StoreTx) *StoreTx {
|
||
if tx == nil {
|
||
return nil
|
||
}
|
||
return &StoreTx{
|
||
ID: tx.ID,
|
||
Hash: tx.Hash,
|
||
RawHex: tx.RawHex,
|
||
PayloadType: tx.PayloadType,
|
||
RouteType: tx.RouteType,
|
||
DecodedJSON: tx.DecodedJSON,
|
||
}
|
||
}
|