mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-11-11 11:57:57 +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>
235 lines
8.0 KiB
Go
235 lines
8.0 KiB
Go
package main
|
|
|
|
import (
|
|
"encoding/json"
|
|
"net/http/httptest"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/gorilla/mux"
|
|
)
|
|
|
|
// The packet detail panel (public/packets.js) renders a "Scope" row gated on
|
|
// `pkt.scope_name != null`, distinguishing three states that transmissions.scope_name
|
|
// encodes: SQL NULL (not transport-scoped, row hidden), "" (transport-scoped but the
|
|
// region did not match a configured key → "unknown scope") and "#name" (matched
|
|
// region). txToMap is the shape /api/packets and /api/packets/{id} serve from the
|
|
// in-memory store, so it must carry all three states through.
|
|
|
|
func TestTxToMapScopeNameMatchedRegion(t *testing.T) {
|
|
scope := "#belgium"
|
|
m := txToMap(&StoreTx{ID: 1, Hash: "aa", ScopeName: &scope})
|
|
if m["scope_name"] != "#belgium" {
|
|
t.Errorf("scope_name = %#v, want %q", m["scope_name"], "#belgium")
|
|
}
|
|
}
|
|
|
|
func TestTxToMapScopeNameUnknownScope(t *testing.T) {
|
|
scope := ""
|
|
m := txToMap(&StoreTx{ID: 1, Hash: "aa", ScopeName: &scope})
|
|
v, ok := m["scope_name"]
|
|
if !ok {
|
|
t.Fatal("scope_name key missing for a transport-scoped packet with an unmatched region")
|
|
}
|
|
if v != "" {
|
|
t.Errorf("scope_name = %#v, want %q (frontend renders this as 'unknown scope')", v, "")
|
|
}
|
|
}
|
|
|
|
func TestTxToMapScopeNameNotTransportScoped(t *testing.T) {
|
|
m := txToMap(&StoreTx{ID: 1, Hash: "aa", ScopeName: nil})
|
|
if m["scope_name"] != nil {
|
|
t.Errorf("scope_name = %#v, want nil for a non-transport-scoped packet", m["scope_name"])
|
|
}
|
|
// A typed nil *string in the map would marshal as null but compare non-nil in
|
|
// Go; assert the JSON the browser actually receives.
|
|
b, err := json.Marshal(m)
|
|
if err != nil {
|
|
t.Fatalf("marshal: %v", err)
|
|
}
|
|
var decoded map[string]interface{}
|
|
if err := json.Unmarshal(b, &decoded); err != nil {
|
|
t.Fatalf("unmarshal: %v", err)
|
|
}
|
|
if decoded["scope_name"] != nil {
|
|
t.Errorf("JSON scope_name = %#v, want null", decoded["scope_name"])
|
|
}
|
|
}
|
|
|
|
// TestPacketDetailExposesScopeName is the end-to-end guard: the packet-detail
|
|
// endpoint is served from the in-memory store, so scope_name must survive the
|
|
// SQL scan (nullStrPtr) and the map conversion (txToMap) with all three states
|
|
// intact. Before this test, txToMap dropped the field entirely and the Scope row
|
|
// only ever rendered for packets old enough to fall through to the DB.
|
|
func TestPacketDetailExposesScopeName(t *testing.T) {
|
|
db := setupTestDB(t)
|
|
if _, err := db.conn.Exec(`ALTER TABLE transmissions ADD COLUMN scope_name TEXT DEFAULT NULL`); err != nil {
|
|
t.Fatalf("add scope_name column: %v", err)
|
|
}
|
|
db.hasScopeName = true
|
|
|
|
now := time.Now().UTC().Format(time.RFC3339)
|
|
// route_type 1 = FLOOD (never transport-scoped → NULL); 0 = TRANSPORT_FLOOD.
|
|
if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type)
|
|
VALUES ('AABB', 'aaaaaaaaaaaaaaa1', ?, 1, 4)`, now); err != nil {
|
|
t.Fatalf("insert unscoped: %v", err)
|
|
}
|
|
if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, scope_name)
|
|
VALUES ('AABB', 'aaaaaaaaaaaaaaa2', ?, 0, 4, '')`, now); err != nil {
|
|
t.Fatalf("insert unknown-scope: %v", err)
|
|
}
|
|
if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, scope_name)
|
|
VALUES ('AABB', 'aaaaaaaaaaaaaaa3', ?, 0, 4, '#belgium')`, now); err != nil {
|
|
t.Fatalf("insert matched-scope: %v", err)
|
|
}
|
|
|
|
srv := NewServer(db, &Config{Port: 3000}, NewHub())
|
|
store := NewPacketStore(db, nil)
|
|
if err := store.Load(); err != nil {
|
|
t.Fatalf("store.Load: %v", err)
|
|
}
|
|
if !store.WaitIndexesReady(5 * time.Second) {
|
|
t.Fatal("background indexes never became ready")
|
|
}
|
|
srv.store = store
|
|
router := mux.NewRouter()
|
|
srv.RegisterRoutes(router)
|
|
|
|
cases := []struct {
|
|
name string
|
|
hash string
|
|
want interface{}
|
|
}{
|
|
{"not transport-scoped", "aaaaaaaaaaaaaaa1", nil},
|
|
{"transport-scoped, region unmatched", "aaaaaaaaaaaaaaa2", ""},
|
|
{"transport-scoped, region matched", "aaaaaaaaaaaaaaa3", "#belgium"},
|
|
}
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
if store.GetPacketByHash(tc.hash) == nil {
|
|
t.Fatalf("precondition: %s not in store (would hit the DB fallback)", tc.hash)
|
|
}
|
|
req := httptest.NewRequest("GET", "/api/packets/"+tc.hash, nil)
|
|
w := httptest.NewRecorder()
|
|
router.ServeHTTP(w, req)
|
|
if w.Code != 200 {
|
|
t.Fatalf("expected 200, got %d (body: %s)", w.Code, w.Body.String())
|
|
}
|
|
var body map[string]interface{}
|
|
if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil {
|
|
t.Fatalf("unmarshal: %v", err)
|
|
}
|
|
pkt, ok := body["packet"].(map[string]interface{})
|
|
if !ok {
|
|
t.Fatal("expected packet object")
|
|
}
|
|
if got := pkt["scope_name"]; got != tc.want {
|
|
t.Errorf("scope_name = %#v, want %#v", got, tc.want)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// --- grouped view ---
|
|
//
|
|
// The Packets tab defaults to "Group by Hash", which is served by a separate
|
|
// mapper (groupedRowsToPage in the store, a dedicated query in the DB fallback).
|
|
// The Scope column reads scope_name off those rows, so both paths must carry it.
|
|
|
|
func TestGroupedTxsToPageCarriesScopeName(t *testing.T) {
|
|
matched := "#belgium"
|
|
unmatched := ""
|
|
txs := []*StoreTx{
|
|
{ID: 1, Hash: "aa", ScopeName: &matched},
|
|
{ID: 2, Hash: "bb", ScopeName: &unmatched},
|
|
{ID: 3, Hash: "cc", ScopeName: nil},
|
|
}
|
|
rows := make([]groupedRow, len(txs))
|
|
for i, tx := range txs {
|
|
rows[i] = snapshotGroupedRow(tx)
|
|
}
|
|
res := groupedRowsToPage(rows, len(txs))
|
|
want := []interface{}{"#belgium", "", nil}
|
|
for i, w := range want {
|
|
if got := res.Packets[i]["scope_name"]; got != w {
|
|
t.Errorf("packet %d: scope_name = %#v, want %#v", i, got, w)
|
|
}
|
|
}
|
|
}
|
|
|
|
func TestGroupedPacketsEndpointExposesScopeName(t *testing.T) {
|
|
db := setupTestDB(t)
|
|
if _, err := db.conn.Exec(`ALTER TABLE transmissions ADD COLUMN scope_name TEXT DEFAULT NULL`); err != nil {
|
|
t.Fatalf("add scope_name column: %v", err)
|
|
}
|
|
db.hasScopeName = true
|
|
if _, err := db.conn.Exec(`DELETE FROM transmissions`); err != nil {
|
|
t.Fatalf("clear transmissions: %v", err)
|
|
}
|
|
|
|
now := time.Now().UTC().Format(time.RFC3339)
|
|
if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type)
|
|
VALUES ('AABB', 'bbbbbbbbbbbbbbb1', ?, 1, 4)`, now); err != nil {
|
|
t.Fatalf("insert unscoped: %v", err)
|
|
}
|
|
if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, scope_name)
|
|
VALUES ('AABB', 'bbbbbbbbbbbbbbb2', ?, 0, 4, '')`, now); err != nil {
|
|
t.Fatalf("insert unknown-scope: %v", err)
|
|
}
|
|
if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, scope_name)
|
|
VALUES ('AABB', 'bbbbbbbbbbbbbbb3', ?, 0, 4, '#belgium')`, now); err != nil {
|
|
t.Fatalf("insert matched-scope: %v", err)
|
|
}
|
|
|
|
want := map[string]interface{}{
|
|
"bbbbbbbbbbbbbbb1": nil,
|
|
"bbbbbbbbbbbbbbb2": "",
|
|
"bbbbbbbbbbbbbbb3": "#belgium",
|
|
}
|
|
|
|
// Both the store-backed path and the DB-only fallback must agree.
|
|
for _, withStore := range []bool{true, false} {
|
|
name := "store"
|
|
if !withStore {
|
|
name = "db"
|
|
}
|
|
t.Run(name, func(t *testing.T) {
|
|
srv := NewServer(db, &Config{Port: 3000}, NewHub())
|
|
if withStore {
|
|
store := NewPacketStore(db, nil)
|
|
if err := store.Load(); err != nil {
|
|
t.Fatalf("store.Load: %v", err)
|
|
}
|
|
if !store.WaitIndexesReady(5 * time.Second) {
|
|
t.Fatal("background indexes never became ready")
|
|
}
|
|
srv.store = store
|
|
}
|
|
router := mux.NewRouter()
|
|
srv.RegisterRoutes(router)
|
|
|
|
req := httptest.NewRequest("GET", "/api/packets?groupByHash=true&limit=50", nil)
|
|
w := httptest.NewRecorder()
|
|
router.ServeHTTP(w, req)
|
|
if w.Code != 200 {
|
|
t.Fatalf("expected 200, got %d (body: %s)", w.Code, w.Body.String())
|
|
}
|
|
var body struct {
|
|
Packets []map[string]interface{} `json:"packets"`
|
|
}
|
|
if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil {
|
|
t.Fatalf("unmarshal: %v", err)
|
|
}
|
|
if len(body.Packets) != 3 {
|
|
t.Fatalf("expected 3 grouped packets, got %d", len(body.Packets))
|
|
}
|
|
for _, p := range body.Packets {
|
|
h, _ := p["hash"].(string)
|
|
if got := p["scope_name"]; got != want[h] {
|
|
t.Errorf("%s: scope_name = %#v, want %#v", h, got, want[h])
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|