fix(server): bound resolved-path LRU entry age for /paths confirmation (#277)

cmd/server/pathlen_fast_test.go

@@ -9,8 +9,13 @@ import (
 // pathLen is called once per observation of every candidate transmission in
 // /api/nodes/{pk}/paths and /hop_analytics. The fast path must agree with the
 // original json.Unmarshal-based implementation (pathLenSlow) for every input.
+//
+// The two EquivalenceGuard tests compare pathLen with the reference. That
+// comparison alone also passes on a build without a fast path (pathLen ==
+// pathLenSlow), so each test also checks pathLenFast directly and requires it
+// to serve inputs: a fast path that declines everything fails them (#277).
 
-func TestPathLenFast_MatchesReference(t *testing.T) {
+func TestPathLen_EquivalenceGuard_Corpus(t *testing.T) {
 	cases := []string{
 		"", " ", "invalid", "null", "true", "0", `""`, `"aa"`, `{}`, `{"a":1}`,
 		`[]`, ` [] `, "\t[\n]\r\n", `[ ]`, `[,]`, `[""]`, `["",""]`,
@@ -27,28 +32,57 @@ func TestPathLenFast_MatchesReference(t *testing.T) {
 		if got, want := pathLen(in), pathLenSlow(in); got != want {
 			t.Errorf("pathLen(%q) = %d, want %d (reference)", in, got, want)
 		}
+		if got, ok := pathLenFast(in); ok && got != pathLenSlow(in) {
+			t.Errorf("pathLenFast(%q) = %d, want %d (reference)", in, got, pathLenSlow(in))
+		}
+	}
+	// The well-formed members of the corpus must be served by the fast path
+	// itself, not by the fallback.
+	for _, in := range []string{
+		`[]`, ` [] `, "\t[\n]\r\n", `[ ]`, `[""]`, `["",""]`, `["aa"]`, `["aa","bb"]`,
+		`["aa","bb","cc"]`, ` [ "aa" , "bb" ] `, `["a,b","c]d","[e"]`,
+		`["aa","bb","cc","dd","ee","ff","00","11"]`,
+	} {
+		if _, ok := pathLenFast(in); !ok {
+			t.Errorf("pathLenFast(%q) fell back; the corpus's well-formed inputs must stay on the fast path", in)
+		}
 	}
 }
 
-// TestPathLenFast_RandomisedAgainstReference throws structurally "almost
-// valid" JSON at both implementations. The token alphabet is chosen so most
-// strings are close to the fast path's grammar, which is where a divergence
-// would hide.
-func TestPathLenFast_RandomisedAgainstReference(t *testing.T) {
+// TestPathLen_EquivalenceGuard_Randomised throws structurally "almost valid"
+// JSON at both implementations. The token alphabet is chosen so most strings
+// are close to the fast path's grammar, which is where a divergence would
+// hide.
+func TestPathLen_EquivalenceGuard_Randomised(t *testing.T) {
 	tokens := []string{
 		"[", "]", ",", `"`, `"aa"`, `"bb"`, `""`, " ", "\n", "\t", `\`, `\"`,
 		"a", "1", "null", "é", "\x00", "{", "}", ":", "-", "e",
 	}
 	rng := rand.New(rand.NewSource(1352))
+	fastNonEmpty := 0
 	for i := 0; i < 300000; i++ {
 		var b strings.Builder
 		for j, n := 0, rng.Intn(14); j < n; j++ {
 			b.WriteString(tokens[rng.Intn(len(tokens))])
 		}
 		in := b.String()
-		if got, want := pathLen(in), pathLenSlow(in); got != want {
+		want := pathLenSlow(in)
+		if got := pathLen(in); got != want {
 			t.Fatalf("pathLen(%q) = %d, want %d (reference)", in, got, want)
 		}
+		if got, ok := pathLenFast(in); ok {
+			if got != want {
+				t.Fatalf("pathLenFast(%q) = %d, want %d (reference)", in, got, want)
+			}
+			if got > 0 {
+				fastNonEmpty++
+			}
+		}
+	}
+	// With this seed the fast path serves 11 non-empty arrays. Zero would mean
+	// the comparison above never exercised it.
+	if fastNonEmpty == 0 {
+		t.Error("pathLenFast served no non-empty input; the randomised comparison did not exercise the fast path")
 	}
 }
 

cmd/server/paths_confirm_deferred_test.go

@@ -59,6 +59,32 @@ func reloadConfirmStore(t *testing.T, srv *Server) *PacketStore {
 	return store
 }
 
+// lruHasTx reports whether any observation of txID has a resolved-path LRU
+// entry, i.e. whether a handler read the tx's canonical path.
+func lruHasTx(store *PacketStore, txID int) bool {
+	store.mu.RLock()
+	tx := store.byTxID[txID]
+	store.mu.RUnlock()
+	if tx == nil {
+		return false
+	}
+	store.lruMu.RLock()
+	defer store.lruMu.RUnlock()
+	for _, obs := range tx.Observations {
+		if _, ok := store.apiResolvedPathLRU[obs.ID]; ok {
+			return true
+		}
+	}
+	return false
+}
+
+func resetLRU(store *PacketStore) {
+	store.lruMu.Lock()
+	defer store.lruMu.Unlock()
+	store.apiResolvedPathLRU = make(map[int]resolvedPathLRUEntry, lruMaxSize)
+	store.lruOrder = store.lruOrder[:0]
+}
+
 type confirmPathsResp struct {
 	Paths              []json.RawMessage `json:"paths"`
 	TotalTransmissions int               `json:"totalTransmissions"`
@@ -94,7 +120,12 @@ func TestNodePaths_NoPerCandidateSQLConfirm(t *testing.T) {
 // contain the queried pubkey (hash collision, or an index entry made stale by
 // a later resolved_path overwrite) must still be excluded — now by the
 // canonical-path check instead of the SQL pre-filter.
-func TestNodePaths_StaleIndexEntryStillExcluded(t *testing.T) {
+//
+// The exclusion assertions are regression guards: they held before #246 too,
+// when the SQL pre-filter did the excluding. What tells before from after is
+// how the decision is made: no confirmation query, and the tx's canonical path
+// was read (it has an LRU entry; the SQL pre-filter dropped it unread).
+func TestNodePaths_StaleIndexEntryExcludedWithoutSQLConfirm(t *testing.T) {
 	srv, router := setupTestServer(t)
 	staleID := seedConfirmTx(t, srv, "confirm_stale_hash",
 		`["aa","bb"]`, `["aacafe0000000000","eeff00112233aabb"]`)
@@ -113,6 +144,10 @@ func TestNodePaths_StaleIndexEntryStillExcluded(t *testing.T) {
 	if strings.Contains(w.Body.String(), "confirm_stale_hash") {
 		t.Error("tx whose resolved_path does not contain the target leaked into /paths via a stale index entry")
 	}
+	if !lruHasTx(store, staleID) {
+		t.Error("/paths excluded the stale-index tx without reading its canonical resolved_path")
+	}
+	resetLRU(store)
 
 	w = httptest.NewRecorder()
 	router.ServeHTTP(w, httptest.NewRequest("GET", "/api/nodes/"+confirmTestTarget+"/hop_analytics", nil))
@@ -122,6 +157,9 @@ func TestNodePaths_StaleIndexEntryStillExcluded(t *testing.T) {
 	if strings.Contains(w.Body.String(), "confirm_stale_hash") {
 		t.Error("tx whose resolved_path does not contain the target leaked into /hop_analytics via a stale index entry")
 	}
+	if !lruHasTx(store, staleID) {
+		t.Error("/hop_analytics excluded the stale-index tx without reading its canonical resolved_path")
+	}
 	if q := store.confirmResolvedPathQueries.Load(); q != 0 {
 		t.Errorf("confirmResolvedPathContains ran %d times, want 0", q)
 	}
@@ -130,7 +168,11 @@ func TestNodePaths_StaleIndexEntryStillExcluded(t *testing.T) {
 // Candidates with no canonical resolved_path are decided by the legacy
 // fallback arm, which still relies on the SQL confirmation. That behaviour is
 // preserved: the query runs (once) and a NULL resolved_path does not confirm.
-func TestNodePaths_NoCanonicalPathStillConfirmedBySQL(t *testing.T) {
+//
+// Only the query count tells this apart from the code before #246, which ran
+// the same query for every candidate: the exclusion assertion held then too,
+// and the response is identical by design. The count is the behaviour pinned.
+func TestNodePaths_NoCanonicalPathConfirmedBySQLExactlyOnce(t *testing.T) {
 	srv, router := setupTestServer(t)
 	nullID := seedConfirmTx(t, srv, "confirm_nullrp_hash", `["aa","bb"]`, "")
 	store := reloadConfirmStore(t, srv)
@@ -154,3 +196,40 @@ func TestNodePaths_NoCanonicalPathStillConfirmedBySQL(t *testing.T) {
 		t.Errorf("confirmResolvedPathContains ran %d times, want exactly 1 (the tx without a canonical path)", q)
 	}
 }
+
+// #277 point 2. Since #246 a candidate admitted by the hash index costs a
+// canonical-path fetch, and an LRU entry, even when its stored path turns out
+// not to contain the target. That stays cheap only because prefix collisions,
+// the usual reason a /paths candidate does not belong to the node, are dropped
+// by the index check before any fetch; only hash collisions and stale index
+// entries get as far as the fetch. This pins that order for both endpoints.
+func TestNodePaths_PrefixCollisionsExcludedBeforeCanonicalFetch(t *testing.T) {
+	srv, router := setupTestServer(t)
+	ownID := seedConfirmTx(t, srv, "collide_own", `["aa","bb"]`, `["`+confirmTestTarget+`","eeff00112233aabb"]`)
+	var otherIDs []int
+	for i := 0; i < 10; i++ {
+		// Same first-hop prefix as the target, resolved to another node.
+		otherIDs = append(otherIDs, seedConfirmTx(t, srv, fmt.Sprintf("collide_other_%02d", i),
+			`["aa","bb"]`, `["aacafe0000000000","eeff00112233aabb"]`))
+	}
+	store := reloadConfirmStore(t, srv)
+
+	for _, ep := range []string{"paths", "hop_analytics"} {
+		resetLRU(store)
+		body := nodeEndpointBody(t, router, ep)
+		if !strings.Contains(body, "collide_own") {
+			t.Errorf("%s: the target's own tx is missing", ep)
+		}
+		if !lruHasTx(store, ownID) {
+			t.Errorf("%s: the target's own tx was decided without its canonical path", ep)
+		}
+		for i, id := range otherIDs {
+			if strings.Contains(body, fmt.Sprintf("collide_other_%02d", i)) {
+				t.Errorf("%s: prefix-colliding tx %d attributed to the target", ep, id)
+			}
+			if lruHasTx(store, id) {
+				t.Errorf("%s: prefix-colliding tx %d reached the canonical-path fetch (LRU entry for a tx that is not the node's)", ep, id)
+			}
+		}
+	}
+}