perf(neighbors): rebuild the neighbor graph only in the background (#2221) (#2222)

Fixes #2221.

## Problem

`getNeighborGraph` rebuilt `Server.neighborGraph` on the request
goroutine once it passed its 5 min TTL. It held `neighborMu` for the
whole build, which takes 11.1-11.4 s on a production store.

The 5 min recomputer went through the same staleness check. A tick that
landed just before the TTL skipped the rebuild and left it to the next
request. As a result `/api/nodes/{pubkey}/neighbors` ran at p95 10.4 s
and max 11.0 s (57 requests).

## Change

- **`getNeighborGraph`** serves the current graph. It builds only when
there is none yet, which happens at startup.
- **`rebuildNeighborGraph`** builds from the packet store whatever the
age of the current graph, outside `neighborMu`, and then swaps the
pointer.
  - `recomputeNeighborGraphCache` calls it first on every tick.
- Without a store it keeps the current graph, so tests that inject a
graph are unaffected.
- **`neighborBuildMu`** serializes builds, so a startup request and the
first tick do not both build.

Freshness is now bounded by the recompute interval
(`neighborGraph.cacheRecomputeIntervalSeconds`, default 5 min) plus one
build. Before this change it was the 5 min TTL plus one build, whatever
the interval. So the bound is the same only at the default 300:
- with a longer interval (for example 3600), `/api/nodes/{pk}/neighbors`
can serve a graph up to that old, where before it was about 5 min;
- with a shorter interval (for example 30), every tick does a full
build, where before there was at most one build per 5 min.

The config comment and `config.example.json` now say this (commit
`de7ae981`).

`IsStale`/`neighborGraphTTL` stay, because `path_inspect.go` still uses
them for `store.graph`.

## Perf

- **Request path:** after the first build it does no build at all. It
takes one mutex and reads one pointer.
- **Background:** the recompute tick now always builds, once every 5
min. Before, it built only on the ticks that found the graph stale, and
a request built in the other cases.
- **Total:** the number of builds stays about the same.

Before: `/api/perf` on production shows p95 10404 ms. After: the
follow-up comment gives client-side curl timings from staging (40
requests, p95 0.204 s), not `/api/perf`. Staging builds the graph in
1.03 s against 11 s on production, so the two are different instances.

## Tests

`neighbor_graph_rebuild_test.go`:
- a stale graph is served without a rebuild;
- a graph is built when there is none yet;
- a recompute tick replaces a graph that is not yet stale;
- a recompute without a store keeps the injected graph.

The first and third tests fail on master. `go vet` and `go test .` pass
in `cmd/server`, and `-race` passes on the neighbor-graph tests.

No frontend change.

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

cmd/server/config.go

@@ -350,8 +350,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"`
 }
 

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
 	}
-	return s.neighborGraph
+
+	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 BuildFromStoreWithOptions(s.store, opts)
 }
 
 // ─── Handlers ──────────────────────────────────────────────────────────────────

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

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

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")
+	}
+}

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

config.example.json

@@ -360,7 +360,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,