diff --git a/cmd/server/nodes_pagination_has_more_test.go b/cmd/server/nodes_pagination_has_more_test.go new file mode 100644 index 00000000..de1eada4 --- /dev/null +++ b/cmd/server/nodes_pagination_has_more_test.go @@ -0,0 +1,92 @@ +package main + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" +) + +// TestNodesHasMoreSurvivesPostLimitFiltering pins the contract that lets a +// client paginate /api/nodes safely. +// +// handleNodes applies the geo-filter, blacklist, hidden-prefix and area passes +// AFTER the SQL LIMIT/OFFSET, and rewrites Total to the filtered length. So a +// page that loses a row is short without being the last page, and neither the +// page length nor Total can tell a client whether to ask for another page. +// has_more is computed from the raw SQL page against the real COUNT(*), before +// those passes run, and must therefore stay true on a page that filtering +// shortened. +// +// Anti-tautology: move the hasMore assignment below the filter block (or +// compute it from the filtered slice) and the page-1 assertion fails — that is +// exactly the arrangement that stranded every node behind a filtered row. +func TestNodesHasMoreSurvivesPostLimitFiltering(t *testing.T) { + srv, router := setupTestServer(t) + + // setupTestServer seeds its own fixture nodes; clear them so the page + // boundaries below are exactly the ones this test sets up. + if _, err := srv.db.conn.Exec(`DELETE FROM nodes`); err != nil { + t.Fatalf("clear fixture nodes: %v", err) + } + + // 7 nodes, newest first by last_seen so page order is deterministic. + for i := 0; i < 7; i++ { + name := fmt.Sprintf("visible-%d", i) + if i == 1 { + name = "🚫 hidden-1" // lands inside page 1 (offset 0, limit 3) + } + lastSeen := fmt.Sprintf("2026-06-0%dT00:00:00Z", 7-i) + if _, err := srv.db.conn.Exec(`INSERT INTO nodes + (public_key, name, role, lat, lon, last_seen, first_seen, advert_count) + VALUES (?, ?, 'repeater', 0, 0, ?, '2026-06-01T00:00:00Z', 1)`, + fmt.Sprintf("deadbeef0000200%d", i), name, lastSeen); err != nil { + t.Fatalf("insert %d: %v", i, err) + } + } + srv.cfg.SetHiddenNamePrefixes([]string{"🚫"}) + + page := func(offset int) NodeListResponse { + req := httptest.NewRequest("GET", fmt.Sprintf("/api/nodes?limit=3&offset=%d", offset), nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("offset %d: status %d body=%s", offset, w.Code, w.Body.String()) + } + var got NodeListResponse + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatalf("offset %d: decode: %v", offset, err) + } + return got + } + + // Page 1 is a row short because the hidden node was dropped after the LIMIT. + first := page(0) + if len(first.Nodes) != 2 { + t.Fatalf("page 1: expected 2 rows after filtering, got %d", len(first.Nodes)) + } + if !first.HasMore { + t.Fatalf("page 1: has_more must stay true on a page shortened by filtering " + + "(4 more nodes are waiting) — a client stopping here strands them") + } + + // Walking has_more reaches every visible node, including the last page. + seen := map[string]bool{} + for offset, more := 0, true; more && offset < 100; offset += 3 { + p := page(offset) + for _, n := range p.Nodes { + pk, _ := n["public_key"].(string) + seen[pk] = true + } + more = p.HasMore + } + if len(seen) != 6 { + t.Fatalf("expected all 6 visible nodes across the walk, got %d", len(seen)) + } + + // The final page reports has_more=false rather than relying on a short page. + if last := page(6); last.HasMore { + t.Fatalf("final page: has_more must be false, got true") + } +} diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index 80053fd9..924f23e5 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -208,9 +208,10 @@ func componentSchemas() map[string]interface{} { "NodeListResponse": map[string]interface{}{ "type": "object", "properties": map[string]interface{}{ - "nodes": map[string]interface{}{"type": "array", "items": schemaRef("Node")}, - "total": map[string]interface{}{"type": "integer", "description": "Total nodes matching the query after filtering."}, - "counts": map[string]interface{}{"type": "object", "additionalProperties": map[string]interface{}{"type": "integer"}, "description": "Per-role node counts."}, + "nodes": map[string]interface{}{"type": "array", "items": schemaRef("Node")}, + "total": map[string]interface{}{"type": "integer", "description": "Total nodes matching the query after filtering."}, + "counts": map[string]interface{}{"type": "object", "additionalProperties": map[string]interface{}{"type": "integer"}, "description": "Per-role node counts."}, + "has_more": map[string]interface{}{"type": "boolean", "description": "True when rows exist past this page. Decided before the blacklist / hidden-prefix / geo-filter / area passes, which drop rows from the page and rewrite total — so neither the page length nor total can be used to stop paginating. Paginate until this is false."}, }, }, "NodeDetailResponse": map[string]interface{}{ diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 8bf16b78..b9dbcdc0 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -1334,9 +1334,10 @@ func (s *Server) handleDecode(w http.ResponseWriter, r *http.Request) { func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() + limit := queryLimit(r, 50, s.cfg.ListLimits.NodesMax) + offset := queryInt(r, "offset", 0) nodes, total, counts, err := s.db.GetNodes( - queryLimit(r, 50, s.cfg.ListLimits.NodesMax), - queryInt(r, "offset", 0), + limit, offset, q.Get("role"), q.Get("search"), q.Get("before"), q.Get("lastHeard"), q.Get("sortBy"), q.Get("region"), ) @@ -1344,6 +1345,14 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { writeError(w, 500, err.Error()) return } + // Whether more rows exist is decided HERE, against the raw SQL page and the + // real COUNT(*), because both other candidate signals are destroyed further + // down: the geo-filter / blacklist / hidden-prefix / area passes drop rows + // from this page AND rewrite `total` to the filtered length. A page that + // loses a row is then short without being the last page, so a client that + // stops on a short page strands every node behind it (#1606 fixed the + // no-filter case only). has_more survives those passes untouched. + hasMore := offset+len(nodes) < total if s.store != nil { hashInfo := s.store.GetNodeHashSizeInfo() relayWindow := s.cfg.GetHealthThresholds().RelayActiveHours @@ -1513,7 +1522,7 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { total = len(filtered) } } - writeJSON(w, NodeListResponse{Nodes: nodes, Total: total, Counts: counts}) + writeJSON(w, NodeListResponse{Nodes: nodes, Total: total, Counts: counts, HasMore: hasMore}) } func (s *Server) handleNodeSearch(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/server/types.go b/cmd/server/types.go index 40b3fa0b..bc23faef 100644 --- a/cmd/server/types.go +++ b/cmd/server/types.go @@ -435,6 +435,12 @@ type NodeListResponse struct { Nodes []map[string]interface{} `json:"nodes"` Total int `json:"total"` Counts map[string]int `json:"counts"` + // HasMore reports whether rows exist past this page. Computed from the raw + // SQL page before the post-LIMIT filters in handleNodes, which shorten the + // page and rewrite Total — so it is the only field a paginating client can + // trust to decide whether to ask for another page. Always emitted (no + // omitempty): a client must be able to tell "false" from "old server". + HasMore bool `json:"has_more"` } type NodeSearchResponse struct { diff --git a/public/app.js b/public/app.js index 9d9485bb..28b97b39 100644 --- a/public/app.js +++ b/public/app.js @@ -237,11 +237,25 @@ async function fetchAllNodes(extraQuery = '', { ttl = 0, pageSize = 500, safetyC : (Array.isArray(data) ? data : []); accumulated.push.apply(accumulated, page); if (offset === 0) counts = (data && data.counts) || {}; - // Canonical stop: a short page is the end. The server's `total` is a real - // COUNT(*) for the query, but the handler overwrites it with the filtered - // length under area/geo/blacklist filtering — so we never loop on it, nor - // surface it; a short page is the reliable end-of-data signal. See #1606. - if (page.length < pageSize) break; + // Canonical stop: the server's `has_more`. Neither of the other two signals + // can be trusted — the handler rewrites `total` to the filtered length, AND + // the same filters (blacklist / hidden prefix / geo-filter / area) drop rows + // from the page itself, so a page can be short while later pages still hold + // rows. #1606 stopped on a short page, which is correct only on deployments + // where nothing is ever filtered; elsewhere one filtered node in page 1 + // stranded every node behind it. `has_more` is computed server-side before + // those filters run. + // An empty page always ends it: there is nothing here, and OFFSET means + // nothing behind it either. Checked first so a server reporting has_more + // against a concurrently-shrinking table cannot spin us to safetyCap. + if (page.length === 0) break; + if (data && typeof data.has_more === 'boolean') { + if (!data.has_more) break; + continue; + } + // Server predates `has_more`: fall back to a zero-length page, the only + // remaining end-of-data signal that filtering cannot fake. Costs one extra + // request per load against an old server; a short page no longer stops us. } // Dedup by public_key: the sort window (last_seen DESC by default) can shift // under concurrent ingest, repeating a row across a page boundary. Rows diff --git a/public/area-map.html b/public/area-map.html index 3c30b48f..5f6d7290 100644 --- a/public/area-map.html +++ b/public/area-map.html @@ -239,7 +239,11 @@ async function fetchAllNodesPaged(extra) { const d = await r.json(); const page = Array.isArray(d) ? d : (d.nodes || []); out.push.apply(out, page); - if (page.length < PAGE) break; + // has_more, not page length: /api/nodes filters rows out AFTER the SQL + // LIMIT, so a short page can still have pages behind it. Zero-length is + // the fallback for a server that predates the flag. + if (page.length === 0) break; + if (typeof d.has_more === 'boolean' && !d.has_more) break; } // Dedup by public_key; rows missing one get a unique key so they aren't collapsed. const seen = new Map(); diff --git a/public/nodes.js b/public/nodes.js index 05f98a72..0b4a355f 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -1232,9 +1232,10 @@ try { // Fetch all nodes via pagination loop — server clamps /api/nodes ?limit // to 500 (PR #1540 / v3.8.3 DoS guard), so a single fetch silently - // truncates large deployments. Loop exit uses data.nodes.length < PAGE_SIZE - // as canonical stop — server total is unreliable under area filters - // (routes.go:1357 overwrites total = len(filtered)). See #1606. + // truncates large deployments. Loop exit uses data.has_more — server + // total is unreliable under area filters (overwritten with + // len(filtered)), and so is the page length, since the same filters drop + // rows from the page. See #1606 and the has_more note in app.js. if (!_allNodes) { const PAGE_SIZE = 500; const SAFETY_CAP = 10000; // hard ceiling to bound runaway loops @@ -1263,8 +1264,16 @@ const estTotal = firstTotal || '?'; nodesBody.innerHTML = 'Loading nodes\u2026 ' + accumulated.length + '/' + estTotal + ''; } - // M1 fix: exit when page is short (canonical stop), not based on total - if (data.nodes.length < PAGE_SIZE) break; + // Exit on has_more; fall back to a zero-length page against a server + // that predates it. A short page is NOT the end: handleNodes drops + // blacklisted / hidden / out-of-geofilter rows after the SQL LIMIT, + // so one filtered node in page 1 used to strand the whole rest of + // the list — including nodes that are actively relaying right now. + // Empty page ends it unconditionally (nothing here, nothing behind + // it); otherwise has_more decides, falling back to empty-page on a + // server that predates the flag. + if (data.nodes.length === 0) break; + if (typeof data.has_more === 'boolean' && !data.has_more) break; offset += PAGE_SIZE; } // TODO(m2): per-page cache invalidation — currently each page uses diff --git a/tests/e2e/test-map-nodes-pagination-e2e.js b/tests/e2e/test-map-nodes-pagination-e2e.js index 7786790d..0a9de850 100644 --- a/tests/e2e/test-map-nodes-pagination-e2e.js +++ b/tests/e2e/test-map-nodes-pagination-e2e.js @@ -26,6 +26,7 @@ const { chromium } = require('playwright'); const BASE = process.env.BASE_URL || 'http://localhost:13581'; const PAGE_CAP = 500; // client page size; a node sits past it on page 2 +const FILTERED_INDEX = 450; // a page-1 row the server drops AFTER the SQL LIMIT const PAGE2_KEY = 'page2deadbeef00000000000000000000000000000000000000000000000beef02'; const PAGE2_NAME = 'PAGE2 RP'; @@ -152,13 +153,20 @@ async function checkMapTeardown(browser, stage, revisit) { nodesRequests++; const limit = Math.min(parseInt(url.searchParams.get('limit') || '50', 10), PAGE_CAP); const offset = parseInt(url.searchParams.get('offset') || '0', 10); - const slice = fixture.slice(offset, offset + limit); + const raw = fixture.slice(offset, offset + limit); + // Model handleNodes exactly: the blacklist / hidden-prefix / geo-filter + // passes run AFTER the SQL LIMIT, so FILTERED_INDEX is counted by the + // limit and by COUNT(*) but removed from the page. Page 1 comes back a + // row short WITHOUT being the last page — the shape that stranded every + // node behind it once a client stopped on a short page. + const slice = raw.filter((_n, i) => offset + i !== FILTERED_INDEX); return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ nodes: slice, total: slice.length, // deliberately wrong per-page total; the helper must ignore it + has_more: offset + raw.length < fixture.length, // decided pre-filter, as the server does counts: { repeaters: fixture.length, rooms: 0, companions: 0, sensors: 0 }, }), }); @@ -173,15 +181,21 @@ async function checkMapTeardown(browser, stage, revisit) { await page.goto(BASE + '/#/map', { waitUntil: 'load', timeout: 60000 }); await page.waitForSelector('#leaflet-map', { timeout: 15000 }); - await step('map loads all 501 nodes by paginating past the 500-row cap', async () => { - // Wait until loadNodes() has populated the app node set. + await step('a page shortened by post-LIMIT filtering does not end pagination', async () => { + // 500 = 501 fixture nodes minus the one the mock filters out of page 1. + // Pre-fix the run stops on that 499-row page and __mc_nodes holds 499. await page.waitForFunction( - () => Array.isArray(window.__mc_nodes) && window.__mc_nodes.length >= 501, + () => Array.isArray(window.__mc_nodes) && window.__mc_nodes.length >= 500, { timeout: 15000 } ); const len = await page.evaluate(() => window.__mc_nodes.length); - assert(len === 501, 'expected 501 nodes in __mc_nodes, got ' + len); + assert(len === 500, 'expected 500 nodes in __mc_nodes (501 minus the filtered row), got ' + len); assert(nodesRequests >= 2, 'expected ≥2 /api/nodes page requests, got ' + nodesRequests); + const filteredGone = await page.evaluate( + (k) => !window.__mc_nodes.some((n) => n.public_key === k), + 'p1' + String(FILTERED_INDEX).padStart(62, '0') + ); + assert(filteredGone, 'the filtered row must stay filtered, not reappear'); }); await step('the page-2 node (cut by the cap pre-fix) is present in the node set', async () => { diff --git a/tests/unit/test-fetch-all-nodes-pagination.js b/tests/unit/test-fetch-all-nodes-pagination.js index b5368949..35eb2d84 100644 --- a/tests/unit/test-fetch-all-nodes-pagination.js +++ b/tests/unit/test-fetch-all-nodes-pagination.js @@ -81,6 +81,13 @@ function makeNodesFetch(total, cap, opts = {}) { for (let i = 0; i < total; i++) fixture.push({ public_key: 'pk' + i, name: 'N' + i }); // Optionally repeat the last row of page 1 as the first row of page 2. if (opts.dupAtBoundary) fixture[cap] = fixture[cap - 1]; + // `dropIndexes` models handleNodes' POST-LIMIT filters (geo-filter, + // nodeBlacklist, hiddenNamePrefixes, area): the row is counted by the SQL + // LIMIT and by COUNT(*), then removed from the page in Go. The page is a row + // short WITHOUT being the last page. `hasMore` mirrors the server field, + // which is computed before those filters run; omit it to model a server that + // predates the field. + const dropped = new Set(opts.dropIndexes || []); return { calls, fetch: (url) => { @@ -89,10 +96,13 @@ function makeNodesFetch(total, cap, opts = {}) { const p = new URLSearchParams(qs); const limit = Math.min(parseInt(p.get('limit') || '50', 10), cap); const offset = parseInt(p.get('offset') || '0', 10); - const page = fixture.slice(offset, offset + limit); + const raw = fixture.slice(offset, offset + limit); + const page = raw.filter((_n, i) => !dropped.has(offset + i)); + const body = { nodes: page, counts: { repeaters: total }, total: page.length }; + if (opts.hasMore) body.has_more = offset + raw.length < total; return Promise.resolve({ ok: true, - json: () => Promise.resolve({ nodes: page, counts: { repeaters: total }, total: page.length }), + json: () => Promise.resolve(body), }); }, }; @@ -108,13 +118,57 @@ test('surfaces ALL nodes past the 500 server cap (1200 > 500)', async () => { const out = await ctx.fetchAllNodes(''); assert.strictEqual(out.nodes.length, 1200, 'expected all 1200 nodes, got ' + out.nodes.length); assert.strictEqual(out.total, 1200, 'total must be the real deduped count, not the clamped per-page total'); - assert.strictEqual(m.calls.length, 3, 'expected 3 pages (500+500+200), got ' + m.calls.length); + // 4, not 3: this mock emits no has_more, so the 200-row page cannot end the + // loop (a short page is exactly what a filtered page looks like) and a + // zero-length probe follows. Against a current server has_more ends it at 3. + assert.strictEqual(m.calls.length, 4, 'expected 3 data pages + 1 probe, got ' + m.calls.length); }); -test('stops on a short page rather than the unreliable server total', async () => { +test('a page shortened by a POST-LIMIT filter does NOT end pagination', async () => { const ctx = makeSandbox(); loadInCtx(ctx, 'public/app.js'); - // Exactly 1000 → pages 500, 500, then a 0-length page stops the loop. + // One node inside page 1 is dropped by handleNodes' blacklist / hidden-prefix + // / geo-filter pass, which runs AFTER the SQL LIMIT. Page 1 returns 499 of + // 500 while 700 more rows are waiting. Treating that short page as the end + // stranded every node past it (map, Nodes page, live, analytics). + const m = makeNodesFetch(1200, 500, { dropIndexes: [450] }); + ctx.fetch = m.fetch; + const out = await ctx.fetchAllNodes(''); + assert.strictEqual(out.nodes.length, 1199, 'expected 1199 surviving nodes, got ' + out.nodes.length); + assert.ok(out.nodes.some(n => n.public_key === 'pk700'), 'a page-2 node must be reachable'); + assert.ok(!out.nodes.some(n => n.public_key === 'pk450'), 'the filtered node must stay filtered'); +}); + +test('uses the server has_more flag and stops without an extra empty page', async () => { + const ctx = makeSandbox(); + loadInCtx(ctx, 'public/app.js'); + // Same filtered page, but against a server that reports has_more. Exactly + // 1000 rows → two full pages; has_more=false on page 2 ends the loop with no + // third request (the short-page fallback needs one to see a zero-length page). + const m = makeNodesFetch(1000, 500, { dropIndexes: [10], hasMore: true }); + ctx.fetch = m.fetch; + const out = await ctx.fetchAllNodes(''); + assert.strictEqual(out.nodes.length, 999, 'expected 999 surviving nodes, got ' + out.nodes.length); + assert.strictEqual(m.calls.length, 2, 'has_more must end the loop without a probe page, got ' + m.calls.length); +}); + +test('has_more=false ends the loop even on a full page', async () => { + const ctx = makeSandbox(); + loadInCtx(ctx, 'public/app.js'); + // Guards the inverse of the bug: the flag, not the page length, decides. + const m = makeNodesFetch(500, 500, { hasMore: true }); + ctx.fetch = m.fetch; + const out = await ctx.fetchAllNodes(''); + assert.strictEqual(out.nodes.length, 500); + assert.strictEqual(m.calls.length, 1, 'a full final page with has_more=false must not be followed, got ' + m.calls.length); +}); + +test('stops on an EMPTY page when the server predates has_more', async () => { + const ctx = makeSandbox(); + loadInCtx(ctx, 'public/app.js'); + // Exactly 1000 → pages 500, 500, then a 0-length page stops the loop. Without + // has_more a zero-length page is the only trustworthy end-of-data signal, so + // the probe request is the documented cost of talking to an older server. const m = makeNodesFetch(1000, 500); ctx.fetch = m.fetch; const out = await ctx.fetchAllNodes(''); @@ -171,10 +225,14 @@ test('rows missing public_key are NOT collapsed into one', async () => { const ctx = makeSandbox(); loadInCtx(ctx, 'public/app.js'); // Two distinct rows both lacking public_key must survive as two entries. - ctx.fetch = () => Promise.resolve({ - ok: true, - json: () => Promise.resolve({ nodes: [{ name: 'A' }, { name: 'B' }, { public_key: 'pk1', name: 'C' }] }), - }); + // The stub must stop itself: with the short-page rule gone, a constant body + // would be paged until safetyCap. One data page, then empty. + let served = false; + ctx.fetch = () => { + const nodes = served ? [] : [{ name: 'A' }, { name: 'B' }, { public_key: 'pk1', name: 'C' }]; + served = true; + return Promise.resolve({ ok: true, json: () => Promise.resolve({ nodes }) }); + }; const out = await ctx.fetchAllNodes(''); assert.strictEqual(out.nodes.length, 3, 'falsy-key rows must not collapse, got ' + out.nodes.length); });