diff --git a/cmd/server/config.go b/cmd/server/config.go index acc3315bc..fb64a55cf 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -342,8 +342,10 @@ type NeighborGraphConfig struct { // CacheRecomputeIntervalSeconds: cadence for the background // recomputer that rebuilds the default-shape neighbor-graph - // response (#1481 P0-1). 0/missing = default 300 (5 min). - // Lower = fresher data, more CPU per minute. #1483. + // response (#1481 P0-1) and the neighbor graph itself, which + // /api/nodes/{pk}/neighbors serves until the next tick (#2221). + // 0/missing = default 300 (5 min). Lower = fresher data, and a + // full graph build on every tick. #1483. CacheRecomputeIntervalSeconds int `json:"cacheRecomputeIntervalSeconds,omitempty"` } diff --git a/cmd/server/neighbor_api.go b/cmd/server/neighbor_api.go index ebdcc5724..27479e95e 100644 --- a/cmd/server/neighbor_api.go +++ b/cmd/server/neighbor_api.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "log" "net/http" "sort" "strconv" @@ -80,25 +81,64 @@ type GraphStats struct { // ─── Graph accessor on Server ────────────────────────────────────────────────── -// getNeighborGraph returns the current neighbor graph, rebuilding if stale. +// getNeighborGraph returns the current neighbor graph. It builds one only +// when there is none yet: a rebuild takes ~11 s on a production store, and +// doing it on the request path made whoever arrived after the TTL wait for +// it (#2221). Keeping the graph fresh is the recomputer's job, see +// rebuildNeighborGraph. func (s *Server) getNeighborGraph() *NeighborGraph { s.neighborMu.Lock() - defer s.neighborMu.Unlock() - - if s.neighborGraph == nil || s.neighborGraph.IsStale() { - if s.store != nil { - opts := BuildOptions{MaxEdgeKm: DefaultMaxEdgeKm} - if s.cfg != nil { - opts.EnableLog = s.cfg.DebugAffinity - opts.MaxEdgeKm = s.cfg.NeighborMaxEdgeKm() - opts.PathTrust = s.cfg.PathTrust - } - s.neighborGraph = BuildFromStoreWithOptions(s.store, opts) - } else { - s.neighborGraph = NewNeighborGraph() - } + g := s.neighborGraph + s.neighborMu.Unlock() + if g != nil { + return g + } + + s.neighborBuildMu.Lock() + defer s.neighborBuildMu.Unlock() + s.neighborMu.Lock() + g = s.neighborGraph + s.neighborMu.Unlock() + if g != nil { + return g // built by whoever held neighborBuildMu before us + } + g = s.buildNeighborGraph() + s.neighborMu.Lock() + s.neighborGraph = g + s.neighborMu.Unlock() + return g +} + +// rebuildNeighborGraph builds a new graph from the packet store and swaps it +// in, whatever the age of the current one. Called on every recompute tick. +// The build runs outside neighborMu, so readers keep the previous graph +// meanwhile. Without a store there is nothing to build from, and the current +// graph is kept. +func (s *Server) rebuildNeighborGraph() { + if s.store == nil { + return + } + s.neighborBuildMu.Lock() + defer s.neighborBuildMu.Unlock() + start := time.Now() + g := s.buildNeighborGraph() + s.neighborMu.Lock() + s.neighborGraph = g + s.neighborMu.Unlock() + log.Printf("[neighbor-graph] rebuilt from the packet store in %v", time.Since(start)) +} + +func (s *Server) buildNeighborGraph() *NeighborGraph { + if s.store == nil { + return NewNeighborGraph() + } + opts := BuildOptions{MaxEdgeKm: DefaultMaxEdgeKm} + if s.cfg != nil { + opts.EnableLog = s.cfg.DebugAffinity + opts.MaxEdgeKm = s.cfg.NeighborMaxEdgeKm() + opts.PathTrust = s.cfg.PathTrust } - return s.neighborGraph + return BuildFromStoreWithOptions(s.store, opts) } // ─── Handlers ────────────────────────────────────────────────────────────────── diff --git a/cmd/server/neighbor_graph.go b/cmd/server/neighbor_graph.go index 19ecc63bf..daef2c358 100644 --- a/cmd/server/neighbor_graph.go +++ b/cmd/server/neighbor_graph.go @@ -20,7 +20,8 @@ const ( affinitySaturationCount = 100 // Time-decay half-life: 7 days. affinityHalfLifeHours = 168.0 - // Cache TTL for the built graph. + // Cache TTL for store.graph (path_inspect.go). Server.neighborGraph is + // rebuilt by the recompute tick instead and does not use it (#2221). neighborGraphTTL = 5 * time.Minute // Auto-resolve confidence: best must be >= this factor × second-best. affinityConfidenceRatio = 3.0 diff --git a/cmd/server/neighbor_graph_cache.go b/cmd/server/neighbor_graph_cache.go index 319af56b1..cccfd8c76 100644 --- a/cmd/server/neighbor_graph_cache.go +++ b/cmd/server/neighbor_graph_cache.go @@ -72,6 +72,7 @@ func (s *Server) recomputeNeighborGraphCache() { atomic.AddUint64(&s.neighborGraphCacheRebuildFailures, 1) } }() + s.rebuildNeighborGraph() start := time.Now() resp := s.buildDefaultNeighborGraphResponse() var buf bytes.Buffer diff --git a/cmd/server/neighbor_graph_rebuild_test.go b/cmd/server/neighbor_graph_rebuild_test.go new file mode 100644 index 000000000..0189001ef --- /dev/null +++ b/cmd/server/neighbor_graph_rebuild_test.go @@ -0,0 +1,79 @@ +package main + +import ( + "testing" + "time" +) + +// #2221: the request path must never rebuild an existing neighbor graph. +// On a production store the build takes ~11 s, and getNeighborGraph held +// neighborMu while doing it, so whoever arrived after the TTL waited for it. + +func rebuildTestStore() *PacketStore { + nodes := []nodeInfo{ + {Role: "repeater", PublicKey: "aaaa1111", Name: "NodeX"}, + {Role: "repeater", PublicKey: "r1aabbcc", Name: "R1"}, + {Role: "repeater", PublicKey: "obs00001", Name: "Observer"}, + } + tx := ngMakeTx(1, 4, ngFromNodeJSON("aaaa1111"), []*StoreObs{ + ngMakeObs("obs00001", `["r1aa"]`, nowStr, nil), + }) + return ngTestStore(nodes, []*StoreTx{tx}) +} + +func TestGetNeighborGraph_ServesStaleGraphWithoutRebuild(t *testing.T) { + stale := NewNeighborGraph() + stale.builtAt = time.Now().Add(-2 * neighborGraphTTL) + srv := &Server{store: rebuildTestStore(), neighborGraph: stale} + + if got := srv.getNeighborGraph(); got != stale { + t.Fatal("getNeighborGraph rebuilt a stale graph on the request path") + } +} + +func TestGetNeighborGraph_BuildsWhenNone(t *testing.T) { + srv := &Server{store: rebuildTestStore()} + + g := srv.getNeighborGraph() + if g == nil || len(g.Neighbors("aaaa1111")) == 0 { + t.Fatal("getNeighborGraph with no graph yet did not build one from the store") + } +} + +func TestRecomputeNeighborGraphCache_RebuildsFreshGraph(t *testing.T) { + fresh := NewNeighborGraph() + fresh.builtAt = time.Now() + srv := &Server{ + store: rebuildTestStore(), + neighborGraph: fresh, + computeNeighborGraphResponseFn: func(int, float64, string, string) NeighborGraphResponse { + return NeighborGraphResponse{} + }, + } + + srv.recomputeNeighborGraphCache() + + g := srv.getNeighborGraph() + if g == fresh { + t.Fatal("recompute kept a graph that was not stale yet; it must rebuild on every tick") + } + if len(g.Neighbors("aaaa1111")) == 0 { + t.Fatal("rebuilt graph is missing the store's X↔R1 edge") + } +} + +func TestRecomputeNeighborGraphCache_KeepsGraphWithoutStore(t *testing.T) { + injected := makeTestGraph(newEdge("aaaa", "bbbb", "bb", 50, time.Now())) + srv := &Server{ + neighborGraph: injected, + computeNeighborGraphResponseFn: func(int, float64, string, string) NeighborGraphResponse { + return NeighborGraphResponse{} + }, + } + + srv.recomputeNeighborGraphCache() + + if srv.getNeighborGraph() != injected { + t.Fatal("recompute without a store replaced the graph with an empty one") + } +} diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 608e95d5b..829b53b2c 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -88,9 +88,13 @@ type Server struct { // can't race on the .tmp file or interleave disk/memory updates. saveMu sync.Mutex - // Neighbor affinity graph (lazy-built, cached with TTL) + // Neighbor affinity graph: built on first use, then rebuilt on every + // neighbor-graph recompute tick (#2221) neighborMu sync.Mutex neighborGraph *NeighborGraph + // neighborBuildMu serializes graph builds, so a startup request and the + // first recompute tick do not both build (#2221). + neighborBuildMu sync.Mutex // Cached /api/scope-stats response — per-window, recomputed at most once every 30s scopeStatsMu sync.Mutex diff --git a/config.example.json b/config.example.json index 74809dee6..d07b488fc 100644 --- a/config.example.json +++ b/config.example.json @@ -355,7 +355,7 @@ "maxAgeDays": 5, "maxEdgeKm": 500, "cacheRecomputeIntervalSeconds": 300, - "_comment": "maxAgeDays: neighbor edges older than this many days are pruned on startup and daily. Default 5. maxEdgeKm: geo-implausibility filter — when both endpoints have GPS, edges with haversine distance > maxEdgeKm are rejected at build time to prevent disambiguator self-reinforcement on wide-geo MQTT deployments. Default 500 km (well above any plausible terrestrial LoRa hop). 0 ⇒ use default; set negative to disable the filter. Rejected count surfaces in /api/analytics/neighbor-graph stats. Issue #1228. cacheRecomputeIntervalSeconds: how often the background recomputer rebuilds the default-shape /api/analytics/neighbor-graph response (#1481 P0-1). Default 300 (5 min). Lower = fresher data, more CPU per minute. Issue #1483." + "_comment": "maxAgeDays: neighbor edges older than this many days are pruned on startup and daily. Default 5. maxEdgeKm: geo-implausibility filter — when both endpoints have GPS, edges with haversine distance > maxEdgeKm are rejected at build time to prevent disambiguator self-reinforcement on wide-geo MQTT deployments. Default 500 km (well above any plausible terrestrial LoRa hop). 0 ⇒ use default; set negative to disable the filter. Rejected count surfaces in /api/analytics/neighbor-graph stats. Issue #1228. cacheRecomputeIntervalSeconds: how often the background recomputer rebuilds the neighbor graph and the default-shape /api/analytics/neighbor-graph response (#1481 P0-1). /api/nodes/{pk}/neighbors serves the graph from the last tick, so this is also how old it can get (#2221). Default 300 (5 min). Lower = fresher data, and a full graph build on every tick. Issue #1483." }, "observersCache": { "ttlSeconds": 30,