From 95672ac2be4d3eb6c56de27cf6a48ce2a23b57f3 Mon Sep 17 00:00:00 2001 From: Alex B <142788550+A13xB0@users.noreply.github.com> Date: Sat, 10 Oct 2026 20:21:50 +0100 Subject: [PATCH] perf(server): share /api/nodes responses for identical queries for 15 s (#2165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- cmd/server/nodes_response_cache_test.go | 212 ++++++++++++++++++++++++ cmd/server/region_nodes_test.go | 1 + cmd/server/routes.go | 122 +++++++++++++- 3 files changed, 331 insertions(+), 4 deletions(-) create mode 100644 cmd/server/nodes_response_cache_test.go diff --git a/cmd/server/nodes_response_cache_test.go b/cmd/server/nodes_response_cache_test.go new file mode 100644 index 00000000..4fffde0d --- /dev/null +++ b/cmd/server/nodes_response_cache_test.go @@ -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") + } +} diff --git a/cmd/server/region_nodes_test.go b/cmd/server/region_nodes_test.go index f8f4a580..9ee63bb7 100644 --- a/cmd/server/region_nodes_test.go +++ b/cmd/server/region_nodes_test.go @@ -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) } diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 5f367d1b..4d40bf6a 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -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)