Files
meshcore-analyzer/cmd/server/pathhop_eviction_1908_test.go
T
dborupandClaude Opus 5.5 7309bdb5a9 fix(store): index a transmission once per relay key in byPathHop (#2117)
Relates to #2108

## Problem

`traffic_share_score` grows with server uptime until it is far above
reality, and many relays end up clamped at 1.0.

The score is the number of non-advert entries in `byPathHop[pubkey]`
divided by the number of non-advert transmissions.
`indexResolvedPathHops` runs once per **observation**, on the
live-ingest path (`IngestNewFromDB`) and on the late-observation path
(`IngestNewObservations`). `addResolvedPubkeysToPathHopIndex` only
de-duplicates within one call, so every further observation through the
same relay appends the transmission to that relay's bucket again. The
denominator counts each transmission once.

After a restart the values look right, because `buildPathHopIndex` →
`retainResolvedPathHops` de-duplicates by `*StoreTx`. They then drift
upwards as live observations arrive.

None of the affected functions has changed on `master` since the
diagnosis against `415362c`. This branch is based on `000d9ab`.

## Fix

1. **Idempotent insert, per (transmission, relay key).**
`addResolvedPubkeysToPathHopIndex` keeps a side map `pathHopResolved
map[*StoreTx][]string`. It holds the resolved keys each transmission is
already indexed under and skips those.
- Keys are compared **exactly**, so no collision can ever drop an entry.
- A later observation through a **new** relay still adds that relay,
once.
   - `StoreTx` is unchanged.
2. **Interned keys.** `pathHopKeys map[string]string` keeps one shared
copy of each resolved key. The record then holds a 16-byte string header
per entry, and not the string each observation's resolve allocated: a
fresh `json.Unmarshal` string per persisted observation on `Load`, and a
`strings.ToLower` result on the live paths. The `byPathHop` key is the
same shared copy.
3. **Eviction and rebuild.**
- `evictStaleInternal` drops the record of evicted transmissions, plus
any interned key whose bucket it deletes.
- `retainResolvedPathHops` keeps the records of live transmissions only
and drops interned keys whose bucket was not carried over. When a
rebuild starts from an empty index, it clears both.
4. **Defence in depth.** The single (`GetRepeaterUsefulnessScore`),
batch (`GetRepeaterNodeStatsBatch`) and bulk
(`computeRepeaterUsefulnessScoreMap`) scores count **distinct**
non-advert transmissions per bucket, via `countDistinctNonAdvert`.
   - IDs are collected in a reused slice.
- A bucket in ascending ID order is already distinct; only out-of-order
buckets are sorted.
- The bulk pass does this for full-pubkey keys only. Raw-hop buckets get
one entry per transmission from `addTxToPathHopIndex`.
5. **Cache side effect.** A repeated observation through known relays no
longer mutates `byPathHop`, so it no longer drops the batch relay-stats
cache, as the cache contract from `1164` intends. An observation through
a new relay still drops it.

The other `byPathHop` consumers already de-duplicate by `tx.ID` or read
only raw prefix keys: `GetNodeHopAnalytics`,
`computeMultiByteCapability`, `handleNodePaths`,
`computeRepeaterRelayInfoMap` and the relay info in
`GetRepeaterNodeStatsBatch`. Two tests pin that they ignore duplicate
entries.

## Exact keys vs. a 64-bit fingerprint

The first version of this fix stored a 64-bit FNV-1a hash per key. As
requested, I measured both, plus exact keys without interning, on the
real `addResolvedPubkeysToPathHopIndex`.

**Keys per transmission.** In the e2e fixture, the union of resolved
relay keys over all observations of one transmission has a mean of 5.4
and a maximum of 17. The protocol limit is 64 path bytes
(`MAX_PATH_SIZE`), i.e. 64 / hash size hops.

**Memory** retained by the record, per transmission.
`BenchmarkPathHopRecordMemory_2108`: 100K transmissions, keys from 4,000
relays, each transmission heard twice with freshly allocated keys. The
record map and the interned keys are included.

| keys per tx | exact, interned (this PR) | 64-bit fingerprint | exact,
not interned |
|---|---|---|---|
| 2 | 88 B | 68 B | 207 B |
| 5 | 136 B | 101 B | 447 B |
| 17 | 344 B | 197 B | 1,423 B |

- At realistic key counts, exact keys cost 20–35 B per transmission more
than the fingerprint. That is about 3.5 MB per 100K transmissions, and
small next to what the store already charges per transmission
(`storeTxBaseBytes` alone is 384 B).
- Without interning, exact keys would cost 3–4× more, and most of that
would be held during every `Load`. Interning is what makes exact keys
affordable.

**Time.** I ran the variants interleaved, 3 rounds each, median ns/op on
an Apple M4. The record step is not measurable inside the full call. The
isolated record step was faster with exact keys in a separate
micro-benchmark, because comparing a handful of 64-char strings is
cheaper than hashing each one.

| benchmark | exact, interned | fingerprint | exact, not interned |
|---|---|---|---|
| late observation, 20K txs | 312 | 358 | 331 |
| late observation, 100K txs | 760 | 836 | 758 |
| `IngestNewObservations` (SQL + resolve + index) | 531 µs | 497 µs |
527 µs |

**Collision probability of the fingerprint.** For k distinct keys in one
transmission it is about k(k−1)/2 / 2^64:

| k | per transmission | per 10^9 transmissions |
|---|---|---|
| 2 | 5.4e-20 | 5.4e-11 |
| 5 | 5.4e-19 | 5.4e-10 |
| 17 | 7.4e-18 | 7.4e-9 |
| 64 | 1.1e-16 | 1.1e-7 |

For random keys this is negligible. FNV-1a is unkeyed, though, so two
relay keys that collide could be found deliberately (a birthday search
over about 2^32 key pairs). A collision would leave a transmission out
of one relay's bucket.

**Decision.** Exact keys are cheap enough once interned: the same speed
within noise, and a few tens of bytes per transmission. They remove the
question of collision-driven omissions entirely, so this PR uses exact
keys and drops the fingerprint.

The two new maps (`pathHopResolved`, `pathHopKeys`) are not added to
`trackedBytes`, so the `maxMemoryMB` trigger undercounts by roughly the
per-transmission figures above. Both are bounded by live state (eviction
and rebuild prune them), so this is a steady proportional undercount
rather than a leak.

## Tests

**New: `cmd/server/pathhop_dedupe_2108_test.go`.** Behaviour tests that
use only existing API. Each one runs against the real SQLite schema
through `Load`, `IngestNewFromDB` and `IngestNewObservations` where
noted.

| test | covers | on `master` |
|---|---|---|
| `TestPathHopIndexOncePerTx_LateObservations_2108` | 1 + 10 late
observations through the same relays: one entry per relay | **fails**
(11 entries) |
| `TestPathHopIndexOncePerTx_LiveIngestBatch_2108` | 11 observations in
one `IngestNewFromDB` batch | **fails** (11) |
| `TestPathHopIndexOncePerTx_AfterLoad_2108` | `Load` + rebuild, then
one live observation | **fails** (2) |
| `TestPathHopIndexAddsNewRelayFromLaterObservation_2108` | a later
observation through a **new** relay adds it exactly once, and its share
becomes correct | **fails** (2) |
| `TestTrafficShareStableAcrossLateObservations_2108` | single, batch
and bulk scores agree, equal the definition, stay put over rounds of
late observations, and equal a fresh `Load` of the same data | **fails**
(all four relays at 1.0, want 0.4–0.5) |
| `TestPathHopIndexSizeBoundedByTransmissions_2108` | the index size
does not grow with observations | **fails** |
| `TestRelayStatsCacheAcrossRepeatedObservations_2108` | a repeated
observation keeps the relay-stats cache, and the kept cache equals a
fresh compute; a new relay drops it, and the next read sees the relay |
**fails** |
| `TestAddResolvedPubkeysToPathHopIndex_PerRelayIdempotent_2108` | the
helper's return value and cache invalidation per (tx, relay), in any key
order | **fails** |
| `TestTrafficShareCountsDistinctTransmissions_2108` | all three scores
count distinct transmissions in a duplicated bucket | **fails** |
| `TestPathHopConsumersIgnoreDuplicateEntries_2108` | pin of the
consumer audit | passes (by design) |
| `TestMultiByteCapabilityIgnoresDuplicateEntries_2108` | pin of the
consumer audit | passes (by design) |

**New: `cmd/server/pathhop_record_2108_test.go`.** These tests use the
new symbols, so they do not compile against `master`.
- `TestCountDistinctNonAdvert_2108`: ascending, descending, interleaved
duplicates, adverts, nils, untyped.
- `TestPathHopResolvedRecordBoundedAndEvicted_2108`: the record stays at
2 keys after 11 observations. Eviction removes the record and every
entry. A survivor's next observation adds nothing. Evicting everything
leaves no record, no interned key and no bucket.
- `TestPathHopResolvedRecordAcrossRebuild_2108`: a rebuild keeps the
records of live transmissions, so the next observation adds nothing. It
drops the record of a removed transmission and the interned key only
that transmission used.
- `TestPathHopResolvedRecordClearedWithEmptyIndex_2108`: a rebuild from
an empty index clears the record and the interned keys, and the next
observation puts the transmission back.
- `TestPathHopResolvedRecordInternsKeys_2108`: record entries and the
`byPathHop` key share one copy, even though every observation passes
freshly allocated keys.

**Changed: `pathhop_eviction_1908_test.go`.** It built its duplicate
entries by repeating `indexResolvedPathHops`, which no longer
duplicates. It now seeds the duplicates directly, so the `1908` sweep is
still tested against buckets that hold one transmission several times.
The expected buckets are unchanged.

**Changed: `db_test.go`.** `setupTestDB` takes `testing.TB`, so the SQL
benchmark can use it.

**Mutants.** I applied each mutant on its own to this branch. All 14 are
killed:

| mutant | killed by |
|---|---|
| record never consulted | the OncePerTx, stable-share, size, cache and
record tests |
| dedupe per transmission instead of per relay |
`AddsNewRelayFromLaterObservation`,
`RelayStatsCacheAcrossRepeatedObservations`, `PerRelayIdempotent` |
| eviction keeps the record | `RecordBoundedAndEvicted` |
| rebuild keeps records of removed transmissions | `RecordAcrossRebuild`
|
| rebuild from an empty index keeps the record |
`RecordClearedWithEmptyIndex` |
| cache dropped on every call |
`RelayStatsCacheAcrossRepeatedObservations`, `PerRelayIdempotent`,
existing `NoMutation_PreservesCache` |
| cache kept although `byPathHop` changed |
`RelayStatsCacheAcrossRepeatedObservations`, `PerRelayIdempotent`,
existing `InvalidatesRelayStatsCache` |
| single score counts entries |
`TrafficShareCountsDistinctTransmissions` |
| batch score counts entries | `TrafficShareCountsDistinctTransmissions`
|
| bulk score counts entries | `TrafficShareCountsDistinctTransmissions`
|
| distinct count trusts any bucket order |
`TrafficShareCountsDistinctTransmissions`, `CountDistinctNonAdvert` |
| keys not interned | `RecordInternsKeys` |
| eviction keeps interned keys | `RecordBoundedAndEvicted` |
| rebuild keeps interned keys of dropped buckets | `RecordAcrossRebuild`
|

**Commands run:**
- `gofmt -l` on all tracked Go files: clean.
- `go vet ./...` in all 14 modules: clean.
- `cd cmd/server && go test -race ./...`: pass.
- `cd cmd/ingestor && go test ./...`: pass.
- `sh test-all.sh`: 184 of 186 suites pass locally. The other two,
`test-issue-1956-release-routing.js` and `test-preflight-xss-gate.js`,
shell out to scripts that need bash ≥ 4 (`mapfile`). They fail under
macOS's bash 3.2 regardless of this change. This PR touches no frontend
or script files.

## Benchmark: `master` vs. this branch

I ran `master`'s sources (`000d9ab`) and this branch interleaved, 5
rounds, with the same benchmark files. Medians on an Apple M4.

| benchmark | `master` | this PR | change |
|---|---|---|---|
| late observation through known relays, 20K txs | 446 ns, 38 B/op | 441
ns, 0 B/op | within noise |
| late observation through known relays, 100K txs | 710 ns, 69 B/op |
755 ns, 0 B/op | within noise (runs overlap) |
| … index size afterwards, entries/tx (20K / 100K) | 24.99 / 8.99, still
growing | 4.99 / 4.99 | bounded |
| `IngestNewObservations`, one observation for each of 20 txs | 532 µs |
513 µs | within noise |
| bulk score pass, clean index, 20K, ingest order | 246 µs | 316 µs |
+28 % |
| bulk score pass, clean index, 100K, ingest order | 3.03 ms | 4.30 ms |
+42 % |
| bulk score pass, clean index, 20K, reversed buckets | 257 µs | 467 µs
| +82 % |
| bulk score pass, clean index, 100K, reversed buckets | 3.38 ms | 4.79
ms | +42 % |
| bulk score pass after 11 observations per tx, 20K | 663 µs | 284 µs |
−57 % |
| bulk score pass after 11 observations per tx, 100K | 4.72 ms | 3.19 ms
| −32 % |

- On an index that is clean on both builds, the distinct count makes the
bulk pass slower. That pass runs on the cache-miss path, which the
background recomputer refreshes every 5 minutes by default.
- On the index a live server actually holds without this fix, `master`
walks every accumulated duplicate, so the real-world pass is faster
after the fix. The gap grows with uptime.
- The late-observation step no longer allocates. On `master` its index
grows with every observation.

Benchmarks: `BenchmarkLateObservationIndex_2108`,
`BenchmarkIngestNewObservations_2108`,
`BenchmarkTrafficShareScoreMap_2108` and
`BenchmarkPathHopRecordMemory_2108`.

## Production motivation

We have run this fix on two production instances. Before the fix, the
summed `traffic_share_score` grew past 180 and many relays sat at the
1.0 clamp; with the fix no node reaches `≥ 0.999`.

A controlled 12 h A/B run makes the drift explicit. Two servers read one
identical database — one on this `master` base, one with the fix —
alongside a reference server that freshly loads the same database (a
fresh load is correct, because the rebuild dedupes). The unfixed server
drifted to 9 relays at the 1.0 clamp and up to 0.95 absolute error per
node against the reference; the fixed server stayed within 0.05 of the
reference for every node, with no relay at the clamp.

A smaller residual rise remains and is a separate cause (startup-vs-live
hop resolution); it is deliberately left for a follow-up so its effect
stays measurable.

## Out of scope

That remaining slow rise comes from a separate cause: live ingest
resolves some hops that the startup load does not. This PR deliberately
leaves that drift alone, so its effect stays measurable. The fix will
follow in a separate PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-08 14:34:31 +02:00

188 lines
6.6 KiB
Go

package main
import (
"fmt"
"reflect"
"strings"
"testing"
"time"
)
func TestPathHopEviction1908(t *testing.T) {
for _, mode := range []string{"time-direct", "time-run", "memory-empty-batch", "memory-index-disabled"} {
t.Run(mode, func(t *testing.T) {
now := time.Now().UTC()
store := makeTestStore(8, now.Add(-30*time.Minute), 0)
store.byPayloadType[5] = store.byPayloadType[4]
delete(store.byPayloadType, 4)
store.byPathHop = make(map[string][]*StoreTx)
store.useResolvedPathIndex = mode != "memory-index-disabled"
store.initResolvedPathIndex()
store.retentionHours = 1
if strings.HasPrefix(mode, "memory-") {
store.maxMemoryMB = 1
store.trackedBytes = 8 * 1048576 // The 25% cap evicts the first two.
} else {
for _, tx := range store.packets[:2] {
tx.FirstSeen = now.Add(-2 * time.Hour).Format(time.RFC3339)
}
}
shared := strings.Repeat("a", 64)
expiredOnly := strings.Repeat("b", 64)
retainedOnly := strings.Repeat("c", 64)
expiredScope, retainedScope := "expired-scope", "retained-scope"
hopsSeen := make(map[string]bool)
for i, tx := range store.packets {
*tx.PayloadType = 5 // Non-advert traffic contributes to relay stats.
pk := retainedOnly
tx.ScopeName = &retainedScope
if i < 2 {
pk = expiredOnly
tx.ScopeName = &expiredScope
}
addTxToPathHopIndex(store.byPathHop, tx)
store.indexResolvedPathHops(tx, []string{shared, pk}, hopsSeen)
}
// Repeated observations no longer append the same resolved
// association again (#2108), but eviction must still drop every
// occurrence from a bucket that holds one, so seed duplicates directly.
store.byPathHop[shared] = append(store.byPathHop[shared], store.packets[0], store.packets[7])
store.byPathHop[expiredOnly] = append(store.byPathHop[expiredOnly], store.packets[0])
addTxToPathHopIndex(store.byPathHop, store.packets[0])
// Keep the original backing arrays observable: a shortened slice must
// not hide pointers that keep evicted transmissions alive for the GC.
backingArrays := make(map[string][]*StoreTx, len(store.byPathHop))
for key, list := range store.byPathHop {
backingArrays[key] = list[:cap(list)]
}
before := store.GetRepeaterNodeStatsBatchCached([]string{shared}, 24)[shared].Info
if before.RelayCount24h != 8 || len(before.TransportedScopes) != 2 {
t.Fatalf("fixture must index all transmissions and scopes: %+v", before)
}
var evicted int
if mode == "time-run" {
evicted = store.RunEviction() // No DB: cleanup cannot depend on SQL.
} else {
store.mu.Lock()
if strings.HasPrefix(mode, "memory-") {
evicted = store.EvictStaleWithRP(map[int][]string{})
} else {
evicted = store.EvictStale()
}
store.mu.Unlock()
}
if evicted != 2 {
t.Fatalf("evicted %d transmissions, want 2", evicted)
}
expected := map[string][]int{
"aa": {3, 4, 5, 6, 7, 8},
"bb": {3, 4, 5, 6, 7, 8},
"cc": {3, 4, 5, 6, 7, 8},
shared: {3, 4, 5, 6, 7, 8, 8},
retainedOnly: {3, 4, 5, 6, 7, 8},
}
if len(store.byPathHop) != len(expected) {
t.Errorf("path-hop index has %d keys, want %d (expired-only keys must disappear)", len(store.byPathHop), len(expected))
}
for key, want := range expected {
var ids []int
for _, tx := range store.byPathHop[key] {
ids = append(ids, tx.ID)
}
if !reflect.DeepEqual(ids, want) {
t.Errorf("path-hop bucket %q = %v, want %v", key, ids, want)
}
}
for key, list := range backingArrays {
for _, tx := range list {
if tx != nil && tx.ID <= 2 {
t.Errorf("backing array for %q still retains evicted tx %d", key, tx.ID)
break
}
}
}
if store.relayStatsCache != nil {
t.Error("eviction must invalidate the relay-stats cache")
}
after := store.GetRepeaterNodeStatsBatchCached([]string{shared}, 24)[shared].Info
if after.RelayCount24h != 6 {
t.Errorf("relay count after eviction = %d, want 6", after.RelayCount24h)
}
if !reflect.DeepEqual(after.TransportedScopes, []string{retainedScope}) {
t.Errorf("transported scopes after eviction = %v, want only retained scope", after.TransportedScopes)
}
// A later sweep that removes every survivor must also remove every key.
for _, tx := range store.packets {
tx.FirstSeen = now.Add(-2 * time.Hour).Format(time.RFC3339)
}
if got := store.RunEviction(); got != 6 {
t.Fatalf("final eviction removed %d transmissions, want 6", got)
}
if len(store.byPathHop) != 0 {
t.Errorf("empty store retains %d path-hop keys", len(store.byPathHop))
}
})
}
}
// Benchmark the eviction path with 8 raw and 8 resolved hops per transmission,
// 2 observations each, and 2,048 resolved relays sharing 256 raw-prefix buckets.
// Other secondary indexes are omitted to isolate path-hop cleanup cost. Setup
// is outside the timer; the timed operation includes acquiring the store lock.
func BenchmarkPathHopEviction1908(b *testing.B) {
const hops = 8
pubkeys := make([]string, 2048)
for i := range pubkeys {
pubkeys[i] = fmt.Sprintf("%02x%062x", i%256, i)
}
for _, count := range []int{30000, 100000} {
for _, evictCount := range []int{1, count / 10, count / 4} {
b.Run(fmt.Sprintf("tx=%d/evict=%d", count, evictCount), func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
b.StopTimer()
now := time.Now().UTC()
old := now.Add(-2 * time.Hour).Format(time.RFC3339)
recent := now.Add(-30 * time.Minute).Format(time.RFC3339)
store := &PacketStore{
packets: make([]*StoreTx, count),
byPathHop: make(map[string][]*StoreTx, len(pubkeys)+256),
spIndex: make(map[string]int),
retentionHours: 1,
}
hopsSeen := make(map[string]bool, hops*2)
for j := range store.packets {
tx := &StoreTx{ID: j + 1, FirstSeen: recent, parsedPath: make([]string, hops), pathParsed: true}
if j < evictCount {
tx.FirstSeen = old
}
tx.Observations = []*StoreObs{{ID: j * 2}, {ID: j*2 + 1}}
resolved := make([]string, hops)
for k := range resolved {
resolved[k] = pubkeys[(j*17+k*13)%len(pubkeys)]
tx.parsedPath[k] = resolved[k][:2]
}
store.packets[j] = tx
addTxToPathHopIndex(store.byPathHop, tx)
store.addResolvedPubkeysToPathHopIndex(tx, resolved, hopsSeen)
}
b.StartTimer()
store.mu.Lock()
got := store.EvictStale()
store.mu.Unlock()
b.StopTimer()
if got != evictCount {
b.Fatalf("evicted %d transmissions, want %d", got, evictCount)
}
}
b.ReportMetric(float64(count*hops*2), "entries/batch")
})
}
}
}