fix: address MINOR notes on PR #1852's merge-ready review

[dijkstra] doc-noted that `total` in handleNodes' filtered path reflects
only the returned page-slice, not an all-pages count — matches
pre-existing semantics, not a regression, but worth being explicit that
fetchAllNodes (public/app.js) never reads it for pagination (relies on
page length instead).

[munger] the pagination compensation loop's maxIterations=50 safety cap
now logs a WARN breadcrumb when it's actually hit, instead of silently
returning a possibly-short page. New test seeds 51 consecutive
hidden-prefix nodes at limit=1 to force the cap and asserts both the
empty result and the log line.
This commit is contained in:
dborup
2026-07-20 11:02:39 +02:00
parent 81bc9930af
commit 60716facb4
2 changed files with 79 additions and 0 deletions
@@ -1,9 +1,12 @@
package main
import (
"bytes"
"encoding/json"
"fmt"
"log"
"net/http/httptest"
"strings"
"testing"
)
@@ -114,3 +117,61 @@ func TestHandleNodes_PostFilterDoesNotTruncatePagination(t *testing.T) {
t.Error("hidden-prefix node's pubkey should never appear across any page")
}
}
// TestHandleNodes_PostFilterCompensationLoopLogsOnExhaustion covers the
// bounded worst case of the compensation loop added above: if a post-filter
// drops an extreme, sustained run of consecutive rows (more than
// maxIterations*limit), the loop gives up rather than scanning forever —
// but must leave a log breadcrumb so a short response is diagnosable
// instead of silently under-filling the page (bot review MINOR, PR #1852).
func TestHandleNodes_PostFilterCompensationLoopLogsOnExhaustion(t *testing.T) {
srv, router := setupTestServer(t)
if _, err := srv.db.conn.Exec(`DELETE FROM nodes`); err != nil {
t.Fatalf("clear nodes: %v", err)
}
// With limit=1, maxIterations=50 caps the loop at 50 DB pages. Seed 51
// consecutive hidden nodes (so every page in that window filters to
// zero) followed by one real node the loop should never reach.
for i := 0; i < 51; i++ {
pk := fmt.Sprintf("pkhidden%056d", i)
lastSeen := fmt.Sprintf("2026-06-01T%02d:00:00Z", 23-(i%24))
if _, err := srv.db.conn.Exec(`INSERT INTO nodes
(public_key, name, role, lat, lon, last_seen, first_seen, advert_count)
VALUES (?, '🚫 Hidden', 'repeater', 55.0, 10.0, ?, '2026-05-01T00:00:00Z', 1)`,
pk, lastSeen); err != nil {
t.Fatalf("insert hidden %d: %v", i, err)
}
}
if _, err := srv.db.conn.Exec(`INSERT INTO nodes
(public_key, name, role, lat, lon, last_seen, first_seen, advert_count)
VALUES ('pkreal0000000000000000000000000000000000000000000000000001', 'RealNode', 'repeater', 55.0, 10.0, '2026-01-01T00:00:00Z', '2026-01-01T00:00:00Z', 1)`,
); err != nil {
t.Fatalf("insert real node: %v", err)
}
srv.cfg.SetHiddenNamePrefixes([]string{"🚫"})
var buf bytes.Buffer
prev := log.Writer()
log.SetOutput(&buf)
defer log.SetOutput(prev)
req := httptest.NewRequest("GET", "/api/nodes?limit=1&offset=0", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
if w.Code != 200 {
t.Fatalf("status=%d body=%s", w.Code, w.Body.String())
}
var resp struct {
Nodes []map[string]interface{} `json:"nodes"`
}
if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
t.Fatalf("decode: %v body=%s", err, w.Body.String())
}
if len(resp.Nodes) != 0 {
t.Errorf("expected 0 nodes (RealNode is beyond the 50-iteration cap), got %d: %+v", len(resp.Nodes), resp.Nodes)
}
if !strings.Contains(buf.String(), "maxIterations") {
t.Errorf("expected a log breadcrumb mentioning maxIterations when the compensation loop is exhausted, got log output: %q", buf.String())
}
}
+18
View File
@@ -1471,6 +1471,7 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) {
// (a raw page shorter than requested = genuine end of data).
const maxIterations = 50 // bounds worst case to 50*requestedLimit DB rows scanned
curOffset := requestedOffset
exhaustedIterations := true
for iter := 0; iter < maxIterations; iter++ {
page, _, pageCounts, err := s.db.GetNodes(requestedLimit, curOffset, role, search, before, lastHeard, sortBy, region)
if err != nil {
@@ -1484,12 +1485,29 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) {
nodes = append(nodes, filters.apply(page)...)
curOffset += requestedLimit
if dbPageLen < requestedLimit || len(nodes) >= requestedLimit {
exhaustedIterations = false
break
}
}
if exhaustedIterations {
// Only reachable if a post-filter is dropping an extreme,
// sustained run of consecutive rows (>50*requestedLimit) —
// astronomically unlikely for blacklist/hidden-prefix in
// practice, but log it so a truncated response is diagnosable
// instead of silently under-filling the page.
log.Printf("WARN handleNodes: pagination compensation loop hit maxIterations=%d (offset=%d, limit=%d) — returning %d rows, possibly short",
maxIterations, requestedOffset, requestedLimit, len(nodes))
}
if len(nodes) > requestedLimit {
nodes = nodes[:requestedLimit]
}
// total reflects only this returned page-slice under any active
// post-filter, not a true all-pages count — matches the
// pre-existing filtered-path semantics (each filter block used to
// set total = len(filtered) the same way). fetchAllNodes
// (public/app.js) never reads `total` for its own pagination
// termination — it relies on page-length — so this is display-only
// and safe to leave approximate.
total = len(nodes)
}