perf(server): share /api/nodes responses for identical queries for 15 s (#2165)

## Summary `/api/nodes` scans every node row into a map, enriches it (hash size, multi-byte capability, relay info, usefulness scores, declared scope) and encodes the page on **every** request. On one production instance (2 vCPU / 2 GB) that was **95 GB of allocation in 45 hours, about 6.7 MB per request and a quarter of all bytes allocated**, on a process where garbage collection was 87% of CPU. The queries repeat heavily. In that instance's nginx log for one day, three pages of one query (`limit=500&offset=0|500|1000&lastHeard=30d`) were 10,000 of the 14,053 `/api/nodes` requests. This PR keeps the encoded response for 15 s per distinct query: - `handleNodes` serves the cached bytes when it can; otherwise it builds through `buildNodesResponse` (the old handler body, unchanged apart from returning instead of writing) inside a singleflight, so concurrent identical misses share one build. - **Cache key:** blacklist generation | hidden-prefix generation | sorted query string. Parameter order does not split the cache, and any change to either filter list misses it at once. This is the pattern `/api/nodes/{pubkey}/reach` already uses (#1629). - `setGeoFilter` drops the cache, so a geo-filter change through the admin endpoint shows immediately. - **Bounded twice:** by `maxCacheEntries` (256) like the other keyed caches, and by bytes, since a full-list response is several MB. Expired entries are dropped on every insert, the total is capped at 32 MB, and a single response over 8 MB is not cached. - **15 s** matches the relay/usefulness caches the response is built from (#1257); the client already keeps its own node list for 90 s. Related to #2146: `/api/nodes` is in its endpoint table (144 s max on one instance). This PR reduces how often the handler runs and allocates, not the SQL wait the issue is about, so it is not a fix for it. ## Performance ### Go benchmarks 1,100 nodes, half repeaters, 10 runs each: ``` cpu: AMD Ryzen 5 3600XT 6-Core Processor │ master │ this PR │ │ sec/op │ sec/op vs base │ HandleNodesRepeated-12 5991.14µ ± 3% 41.21µ ± 7% -99.31% (p=0.000 n=10) HandleNodesUnique-12 6.010m ± 2% 5.967m ± 0% ~ (p=0.393 n=10) │ B/op │ B/op vs base │ HandleNodesRepeated-12 1611.0Ki ± 1% 240.9Ki ± 0% -85.05% (p=0.000 n=10) HandleNodesUnique-12 1.566Mi ± 1% 1.753Mi ± 1% +11.96% (p=0.000 n=10) ``` `Repeated` is one query asked over and over (the production pattern). `Unique` makes every request a different query, so nothing is served from cache: the time is unchanged, and it allocates 12% more because the body is now built in memory so it can be kept. ### Expected hit rate Replaying that day's 14,053 `/api/nodes` requests through a cache of each lifetime: | TTL | served from cache | |---|---| | 5 s | 39% | | 10 s | 47% | | **15 s** | **52%** | | 30 s | 58% | | 60 s | 69% | ### Staging A/B The same 30 minutes of real production API traffic (5,640 requests, 279 of them `/api/nodes`), replayed at 2x on a staging host against a copy of a production database, on `master` and on this branch: | `/api/nodes` | master | this PR | |---|---|---| | p50 | 47.1 ms | **31.0 ms** | | p95 | 165.4 ms | **114.4 ms** | | max | 759.9 ms | 321.7 ms | | allocated by `handleNodes` during the replay | 1,000 MB | **594 MB (-41%)** | | total allocated by the process | 3,704 MB | 3,349 MB (-10%) | The -41% matches the expectation from the hit rate and the miss-path cost: about 0.48 × 1.12 + 0.52 × 0.15 ≈ 0.61 of the old allocation. ## Tests - Red commit `ab33bc29`: a test-only build hook, the tests and both benchmarks. Three fail on `master`: identical queries are built once, reordered parameters share an entry, ten concurrent misses share one build. Three more guard against over-caching and pass on both commits: different queries are built separately, an expired entry is rebuilt, a geo-filter change drops entries. - Green commit `ea0d1cf2`: the cache, plus `TestHandleNodes_FilterListChangesMissCache` (hidden-prefix and blacklist changes miss the cache) and `TestNodesCache_BoundedByBytes` (stays under 32 MB; an oversized response is not cached). - One existing test changed: `TestHandleNodesRegionUsesStore` already clears the store's own 30 s region cache by hand before its second request; it now clears this cache on the same line. `TestHiddenNamePrefix_1181_NodesList`, which changes the hidden-prefix list between requests, passes unchanged because the generation is in the key. - `go test -race` on the new tests passes. `make test`: 16 of 16 modules pass. ## Review notes - The response depends only on the query string and server state, not on who is asking (no auth or header-dependent output in the handler), so a shared cache is safe. - `writeJSONBytes` writes the cached bytes with the same `Content-Type` and status as `writeJSON`; the body has the trailing newline `json.Encoder` adds, so output is byte-identical. - No new `map[string]interface{}` added (the existing node maps are unchanged; converting them to a struct is a separate, larger job). - The TTL and byte budget are constants. They could be operator settings later (AGENTS.md rule 8); not added here. ## SonarLint SonarQube Community rules (the SonarLint engine) were run on the files this PR touches, and only issues on lines it adds are listed. - **S3776 on `buildNodesResponse`:** complexity 91. That is upstream's existing handler body moved unchanged (it is also 91 on `master`); this PR makes the handler itself simpler. - **Fixed** in `25d40950`: the test hook moved to the cache-miss path, so the builder stays identical to the old body rather than one point above it. Splitting that function is a refactor of its own. ## Checked against the repo rules - Rebased onto `master` (`673442cc`). Each red commit was re-run and still fails on its assertions; each green commit passes (TDD.md). - `make test`: all 16 modules pass. Frontend suites: all pass except `test-issue-1956-release-routing.js`, which needs CI's `GITHUB_REF_NAME` and fails the same way on `master` locally (#2160 addresses it). - No private data: test fixtures and examples use the codes the existing tests use (SJC/SFO/LAX, `#be`-style scopes). - No new `map[string]interface{}`, no hard-coded colours. `git add` was explicit, and there is one logical change per commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01KaR4xu8gVykQBkH8yG8m6H --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-22 20:21:17 +00:00 · 2026-10-10 21:21:50 +02:00
1 parent 323b8040d4
commit 95672ac2be
@@ -0,0 +1,212 @@
+package main
+
+import (
+	"fmt"
+	"io"
+	"log"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/gorilla/mux"
+)
+
+// /api/nodes builds every node row into a map, enriches it and encodes the
+// page on every request: on one production instance that was 95 GB of
+// allocation over 45 hours (about 6.7 MB per request), a quarter of all bytes
+// allocated. The same few queries repeat (three paginated pages of one query
+// were 10,000 of 14,000 requests in a day), so identical queries inside
+// nodesResponseTTL share one built and encoded response.
+
+func getNodes(t *testing.T, router http.Handler, url string) string {
+	t.Helper()
+	req := httptest.NewRequest("GET", url, nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+	if w.Code != 200 {
+		t.Fatalf("GET %s: %d %s", url, w.Code, w.Body.String())
+	}
+	return w.Body.String()
+}
+
+func countNodeBuilds(srv *Server) *int32 {
+	var n int32
+	srv.nodesComputeHook = func() { atomic.AddInt32(&n, 1) }
+	return &n
+}
+
+func TestHandleNodes_IdenticalQueryBuiltOnce(t *testing.T) {
+	srv, router := setupTestServer(t)
+	builds := countNodeBuilds(srv)
+	a := getNodes(t, router, "/api/nodes?limit=50")
+	b := getNodes(t, router, "/api/nodes?limit=50")
+	if got := atomic.LoadInt32(builds); got != 1 {
+		t.Fatalf("expected one build for two identical queries, got %d", got)
+	}
+	if a != b {
+		t.Fatal("cached response differs from the built one")
+	}
+}
+
+func TestHandleNodes_ParameterOrderSharesEntry(t *testing.T) {
+	srv, router := setupTestServer(t)
+	builds := countNodeBuilds(srv)
+	getNodes(t, router, "/api/nodes?limit=50&offset=0")
+	getNodes(t, router, "/api/nodes?offset=0&limit=50")
+	if got := atomic.LoadInt32(builds); got != 1 {
+		t.Fatalf("expected reordered parameters to share one build, got %d", got)
+	}
+}
+
+func TestHandleNodes_DistinctQueriesBuiltSeparately(t *testing.T) {
+	srv, router := setupTestServer(t)
+	builds := countNodeBuilds(srv)
+	a := getNodes(t, router, "/api/nodes?limit=50")
+	b := getNodes(t, router, "/api/nodes?limit=1")
+	if got := atomic.LoadInt32(builds); got != 2 {
+		t.Fatalf("expected two builds for two different queries, got %d", got)
+	}
+	if a == b {
+		t.Fatal("different limits returned the same body")
+	}
+}
+
+func TestHandleNodes_ConcurrentMissesShareOneBuild(t *testing.T) {
+	srv, router := setupTestServer(t)
+	var n int32
+	srv.nodesComputeHook = func() {
+		atomic.AddInt32(&n, 1)
+		time.Sleep(50 * time.Millisecond) // make sure the callers overlap
+	}
+	var wg sync.WaitGroup
+	for i := 0; i < 10; i++ {
+		wg.Add(1)
+		go func() { defer wg.Done(); getNodes(t, router, "/api/nodes?limit=50") }()
+	}
+	wg.Wait()
+	if got := atomic.LoadInt32(&n); got != 1 {
+		t.Fatalf("expected 10 concurrent identical requests to share one build, got %d", got)
+	}
+}
+
+func TestHandleNodes_RebuiltAfterTTL(t *testing.T) {
+	srv, router := setupTestServer(t)
+	builds := countNodeBuilds(srv)
+	getNodes(t, router, "/api/nodes?limit=50")
+	srv.expireNodesCacheForTest()
+	getNodes(t, router, "/api/nodes?limit=50")
+	if got := atomic.LoadInt32(builds); got != 2 {
+		t.Fatalf("expected a rebuild once the cached response expired, got %d builds", got)
+	}
+}
+
+func TestHandleNodes_GeoFilterChangeDropsCache(t *testing.T) {
+	srv, router := setupTestServer(t)
+	builds := countNodeBuilds(srv)
+	getNodes(t, router, "/api/nodes?limit=50")
+	srv.setGeoFilter(&GeoFilterConfig{Polygon: [][2]float64{{0, 0}, {0, 1}, {1, 1}}})
+	getNodes(t, router, "/api/nodes?limit=50")
+	if got := atomic.LoadInt32(builds); got != 2 {
+		t.Fatalf("expected a geo-filter change to drop cached node pages, got %d builds", got)
+	}
+}
+
+// expireNodesCacheForTest drops every cached /api/nodes response.
+func (s *Server) expireNodesCacheForTest() { s.invalidateNodesCache() }
+
+// nodesBenchServer serves 1,100 nodes (a production instance lists about
+// 1,085), half of them repeaters.
+func nodesBenchServer(b *testing.B) http.Handler {
+	b.Helper()
+	log.SetOutput(io.Discard)
+	b.Cleanup(func() { log.SetOutput(os.Stderr) })
+	db := setupTestDB(b)
+	now := time.Now().UTC()
+	for i := 0; i < 1100; i++ {
+		role := "companion"
+		if i%2 == 0 {
+			role = "repeater"
+		}
+		seen := now.Add(-time.Duration(i%720) * time.Hour).Format(time.RFC3339)
+		if _, err := db.conn.Exec(`INSERT INTO nodes (public_key, name, role, lat, lon, last_seen, first_seen, advert_count)
+			VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
+			fmt.Sprintf("%064x", i+1000), fmt.Sprintf("Bench node %d", i), role,
+			55+float64(i%100)/50, -4+float64(i%80)/40, seen, seen, i%50); err != nil {
+			b.Fatal(err)
+		}
+	}
+	srv := NewServer(db, &Config{Port: 3000}, NewHub())
+	store := NewPacketStore(db, nil)
+	if err := store.Load(); err != nil {
+		b.Fatal(err)
+	}
+	store.WaitIndexesReady(5 * time.Second)
+	srv.store = store
+	router := mux.NewRouter()
+	srv.RegisterRoutes(router)
+	return router
+}
+
+func benchGet(b *testing.B, router http.Handler, url string) {
+	req := httptest.NewRequest("GET", url, nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+	if w.Code != 200 {
+		b.Fatalf("GET %s: %d", url, w.Code)
+	}
+}
+
+// BenchmarkHandleNodesRepeated: the production pattern, one query asked over
+// and over (the top query was 3,378 of 14,053 requests in a day).
+func BenchmarkHandleNodesRepeated(b *testing.B) {
+	router := nodesBenchServer(b)
+	const url = "/api/nodes?limit=500&offset=0&lastHeard=30d"
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		benchGet(b, router, url)
+	}
+}
+
+// BenchmarkHandleNodesUnique: every request a different query, so nothing is
+// served from cache; shows the miss path does not get slower.
+func BenchmarkHandleNodesUnique(b *testing.B) {
+	router := nodesBenchServer(b)
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		benchGet(b, router, fmt.Sprintf("/api/nodes?limit=500&offset=0&lastHeard=30d&search=&n=%d", i))
+	}
+}
+
+func TestHandleNodes_FilterListChangesMissCache(t *testing.T) {
+	srv, router := setupTestServer(t)
+	builds := countNodeBuilds(srv)
+	getNodes(t, router, "/api/nodes?limit=50")
+	srv.cfg.SetHiddenNamePrefixes([]string{"zz-hidden"})
+	getNodes(t, router, "/api/nodes?limit=50")
+	srv.cfg.SetNodeBlacklist([]string{"0000000000000000"})
+	getNodes(t, router, "/api/nodes?limit=50")
+	if got := atomic.LoadInt32(builds); got != 3 {
+		t.Fatalf("expected hidden-prefix and blacklist changes to miss the cache, got %d builds", got)
+	}
+}
+
+func TestNodesCache_BoundedByBytes(t *testing.T) {
+	srv := &Server{}
+	body := make([]byte, nodesCacheMaxBytes/5) // under the per-entry limit
+	for i := 0; i < 20; i++ {
+		srv.storeNodesResponse(fmt.Sprintf("q%d", i), body)
+		if srv.nodesCacheBytes > nodesCacheMaxBytes {
+			t.Fatalf("cache holds %d bytes after %d inserts, limit %d", srv.nodesCacheBytes, i+1, nodesCacheMaxBytes)
+		}
+	}
+	srv.storeNodesResponse("huge", make([]byte, nodesCacheMaxBytes/4+1))
+	if _, ok := srv.cachedNodesResponse("huge"); ok {
+		t.Fatal("expected an oversized response not to be cached")
+	}
+}
@@ -186,6 +186,7 @@ func TestHandleNodesRegionUsesStore(t *testing.T) {
 	srv.store.regionNodesMu.Lock()
 	srv.store.regionNodesCache = map[string]regionNodesEntry{}
 	srv.store.regionNodesMu.Unlock()
+	srv.invalidateNodesCache() // the 15 s /api/nodes response cache, likewise
 	if got := get(); fmt.Sprint(got) != "[aabbccdd11223344 eeff00112233aabb]" {
 		t.Errorf("region SJC must come from the store: got %v", got)
 	}
@@ -68,6 +68,17 @@ type Server struct {
 	// queries against a 4-connection pool.
 	statsSF singleflight.Group

+	// /api/nodes responses, encoded, keyed by the canonical query string and
+	// kept for nodesResponseTTL. Identical queries in that window share one
+	// build (nodesSF collapses concurrent misses). setGeoFilter drops them.
+	nodesCacheMu    sync.Mutex
+	nodesCache      map[string]nodesCacheEntry
+	nodesCacheBytes int // sum of len(body) over nodesCache; bounded by nodesCacheMaxBytes
+	nodesSF         singleflight.Group
+	// Test-only hook fired each time handleNodes builds a response (not when
+	// one is served from cache). Nil in production. See nodes_response_cache_test.go.
+	nodesComputeHook func()
+
 	// Guards s.cfg.GeoFilter — read by ingest/handler goroutines, written by PUT handler
 	cfgMu sync.RWMutex

@@ -218,8 +229,10 @@ func (s *Server) getGeoFilter() *GeoFilterConfig {
 // setGeoFilter atomically swaps the geo_filter config; used by PUT /api/config/geo-filter.
 func (s *Server) setGeoFilter(gf *GeoFilterConfig) {
 	s.cfgMu.Lock()
-	defer s.cfgMu.Unlock()
 	s.cfg.GeoFilter = gf
+	s.cfgMu.Unlock()
+	// Cached /api/nodes pages were filtered with the old polygon.
+	s.invalidateNodesCache()
 }

 // RegisterRoutes sets up all HTTP routes on the given router.
@@ -1420,7 +1433,100 @@ func (s *Server) handleDecode(w http.ResponseWriter, r *http.Request) {

 // --- Node Handlers ---

+// nodesResponseTTL is how long an /api/nodes response is reused for an
+// identical query. Relay and usefulness data under it is already cached for
+// 15 s (#1257), and the client keeps its own node list for 90 s.
+const nodesResponseTTL = 15 * time.Second
+
+// nodesCacheMaxBytes bounds the encoded /api/nodes responses held at once. A
+// full-list response is a few MB, so an entry count alone would not bound the
+// heap if many distinct queries arrived inside one TTL.
+const nodesCacheMaxBytes = 32 << 20
+
+type nodesCacheEntry struct {
+	body []byte
+	exp  time.Time
+}
+
 func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) {
+	// Encode() sorts by key, so parameter order does not split the cache. The
+	// blacklist and hidden-prefix generations are part of the key so a change
+	// to either misses the cache at once (same pattern as #1629).
+	key := fmt.Sprintf("%d|%d|%s", s.cfg.BlacklistGeneration(), s.cfg.HiddenNamePrefixesGeneration(), r.URL.Query().Encode())
+	if body, ok := s.cachedNodesResponse(key); ok {
+		writeJSONBytes(w, body)
+		return
+	}
+	v, err, _ := s.nodesSF.Do(key, func() (interface{}, error) {
+		if body, ok := s.cachedNodesResponse(key); ok {
+			return body, nil
+		}
+		if s.nodesComputeHook != nil {
+			s.nodesComputeHook()
+		}
+		resp, err := s.buildNodesResponse(r)
+		if err != nil {
+			return nil, err
+		}
+		body, err := json.Marshal(resp)
+		if err != nil {
+			return nil, err
+		}
+		body = append(body, '\n') // byte-identical to json.Encoder output
+		s.storeNodesResponse(key, body)
+		return body, nil
+	})
+	if err != nil {
+		writeError(w, 500, err.Error())
+		return
+	}
+	writeJSONBytes(w, v.([]byte))
+}
+
+func (s *Server) cachedNodesResponse(key string) ([]byte, bool) {
+	s.nodesCacheMu.Lock()
+	defer s.nodesCacheMu.Unlock()
+	e, ok := s.nodesCache[key]
+	if !ok || time.Now().After(e.exp) {
+		return nil, false
+	}
+	return e.body, true
+}
+
+func (s *Server) storeNodesResponse(key string, body []byte) {
+	if len(body) > nodesCacheMaxBytes/4 {
+		return // one oversized response must not evict everything else
+	}
+	s.nodesCacheMu.Lock()
+	defer s.nodesCacheMu.Unlock()
+	now := time.Now()
+	for k, e := range s.nodesCache { // drop expired entries first
+		if now.After(e.exp) {
+			s.nodesCacheBytes -= len(e.body)
+			delete(s.nodesCache, k)
+		}
+	}
+	if old, ok := s.nodesCache[key]; ok {
+		s.nodesCacheBytes -= len(old.body)
+	}
+	if s.nodesCache == nil || len(s.nodesCache) >= maxCacheEntries || s.nodesCacheBytes+len(body) > nodesCacheMaxBytes {
+		s.nodesCache = make(map[string]nodesCacheEntry)
+		s.nodesCacheBytes = 0
+	}
+	s.nodesCache[key] = nodesCacheEntry{body: body, exp: now.Add(nodesResponseTTL)}
+	s.nodesCacheBytes += len(body)
+}
+
+// invalidateNodesCache drops every cached /api/nodes response.
+func (s *Server) invalidateNodesCache() {
+	s.nodesCacheMu.Lock()
+	s.nodesCache = nil
+	s.nodesCacheBytes = 0
+	s.nodesCacheMu.Unlock()
+}
+
+// buildNodesResponse builds one /api/nodes page from the request's query.
+func (s *Server) buildNodesResponse(r *http.Request) (NodeListResponse, error) {
 	q := r.URL.Query()
 	limit := queryLimit(r, 50, s.cfg.ListLimits.NodesMax)
 	offset := queryInt(r, "offset", 0)
@@ -1438,8 +1544,7 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) {
 	}
 	nodes, total, counts, err := s.db.GetNodes(nq)
 	if err != nil {
-		writeError(w, 500, err.Error())
-		return
+		return NodeListResponse{}, err
 	}
 	// Whether more rows exist is decided HERE, against the raw SQL page and the
 	// real COUNT(*), because both other candidate signals are destroyed further
@@ -1618,7 +1723,7 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) {
 			total = len(filtered)
 		}
 	}
-	writeJSON(w, NodeListResponse{Nodes: nodes, Total: total, Counts: counts, HasMore: hasMore})
+	return NodeListResponse{Nodes: nodes, Total: total, Counts: counts, HasMore: hasMore}, nil
 }

 func (s *Server) handleNodeSearch(w http.ResponseWriter, r *http.Request) {
@@ -3186,6 +3291,15 @@ func writeJSONStatus(w http.ResponseWriter, code int, v interface{}) {
 	}
 }

+// writeJSONBytes writes an already-encoded JSON body with status 200.
+func writeJSONBytes(w http.ResponseWriter, body []byte) {
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(http.StatusOK)
+	if _, err := w.Write(body); err != nil {
+		log.Printf("[routes] JSON write error: %v", err)
+	}
+}
+
 func writeError(w http.ResponseWriter, code int, msg string) {
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(code)