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

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 ──────────────────────────────────────────────────────────────────

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