From 6a5e2cdc37ef7cdec960ede612686b41c4ef3d2a Mon Sep 17 00:00:00 2001 From: efiten Date: Thu, 15 Oct 2026 15:40:07 +0200 Subject: [PATCH] perf(neighbors): rebuild the neighbor graph only in the background (#2221) getNeighborGraph rebuilt Server.neighborGraph on the request goroutine once it passed its 5 min TTL, holding neighborMu for the whole build (~11 s on a production store). The 5 min recomputer went through the same staleness check, so a tick that landed just before the TTL skipped the rebuild and left it to the next request: /api/nodes/{pubkey}/neighbors ran at p95 10.4 s. The request path now serves the current graph and builds only when there is none yet. Every recompute tick rebuilds unconditionally, outside neighborMu, and swaps the pointer. neighborBuildMu keeps a startup request and the first tick from building twice. Without a store the current graph is kept. Co-Authored-By: Claude Opus 5.5 --- cmd/server/neighbor_api.go | 72 ++++++++++++++++----- cmd/server/neighbor_graph_cache.go | 1 + cmd/server/neighbor_graph_rebuild_test.go | 79 +++++++++++++++++++++++ cmd/server/routes.go | 6 +- 4 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 cmd/server/neighbor_graph_rebuild_test.go 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_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