fix(store): index a transmission once per relay key in byPathHop (traffic share inflation)

[dborup]

Relates to #158

Cause

traffic_share_score is |non-advert txs in byPathHop[pubkey]| / |non-advert txs in byPayloadType|, clamped to 1. The two sides counted different things:

• Denominator: counts each transmission once.
• Numerator: counted bucket entries.

indexResolvedPathHops runs once per observation, on both the live-ingest path (IngestNewFromDB, several observations in one batch) and the late-observation path (IngestNewObservations). addResolvedPubkeysToPathHopIndex only deduplicated within one call (hopsSeen is cleared on entry). So every further observation through the same relays appended the transmission to their buckets again.

Load and the background chunk load end in buildPathHopIndex, and its retainResolvedPathHops deduplicates by *StoreTx. That is why values looked right after a restart and then drifted upwards with uptime (prod: the sum over all repeaters was 235.8 after 98 h, many repeaters clamped at 1.0).

Reproduced on master be35eefb: after 11 observations, every relay bucket holds the transmission 11 times, and all four fixture relays sit at the 1.0 clamp (their correct values are 0.4–0.5).

Design

Plan as given in the issue and the task. The work was requested autonomously, so the plan is written here instead of waiting for sign-off (AGENTS.md rule 5).

1. Idempotent insert (cmd/server/store.go). addResolvedPubkeysToPathHopIndex keeps, per transmission, the hashes of the resolved keys it has already appended the transmission under, in the new field PacketStore.pathHopResolved map[*StoreTx][]uint64, and skips keys already recorded.

• Bounded: one 8-byte hash per relay key per live transmission.
• Hash: 64-bit FNV-1a, computed inline without allocating. A collision would need two different relay pubkeys of the same transmission to hash alike. At worst it would leave that transmission out of one bucket; it can never inflate a count.
• StoreTx is unchanged. Its 320-byte layout is pinned by TestStoreTxLayoutFits*, so the record is a side map rather than a struct field.
• Keyed by *StoreTx, like the evictedTxSet that eviction already uses and the dedupe in retainResolvedPathHops.

2. Consistent with eviction (#115/#144) and rebuild.

• evictStaleInternal deletes the record of evicted transmissions, next to the existing prefix-scoped bucket sweep. That sweep is unchanged and still removes any legacy duplicates.
• retainResolvedPathHops carries the resolved entries of live transmissions over, so their record stays valid. It drops the record of transmissions no longer in s.packets, and clears it when there is no previous index.

3. Defence in depth. GetRepeaterUsefulnessScore, computeRepeaterUsefulnessScoreMap and the score in GetRepeaterNodeStatsBatch count distinct non-advert transmissions via countDistinctNonAdvert.

• It collects IDs in a reused slice. Buckets are appended in ingest order, so strictly increasing IDs already prove they are distinct. Only out-of-order buckets (background chunk loads) are sorted.
• The bulk pass applies this to full-pubkey keys only: those are the resolved hops that could hold duplicates, and the only keys a repeater's score is read under. Raw hop buckets get one entry per transmission from addTxToPathHopIndex and are counted as before.
• A first version used a map per bucket. It made the bulk pass about 10× slower, so it was replaced in the third commit.

4. Audit of the other byPathHop consumers.

Consumer	Affected?	Evidence
GetRepeaterNodeStatsBatch, Score	yes, counted entries → fixed	TestTrafficShareCountsDistinctTransmissions_158, TestTrafficShareStableAcrossLateObservations_158
GetRepeaterNodeStatsBatch, Info (collectRelayEntriesLocked)	no, dedupes by tx.ID	TestPathHopConsumersIgnoreDuplicateEntries_158
GetNodeHopAnalytics	no, dedupes by tx.ID	TestPathHopConsumersIgnoreDuplicateEntries_158
computeMultiByteCapability	no, reads raw prefix keys only and keeps a maximum	TestPathHopConsumersIgnoreDuplicateEntries_158, TestMultiByteCapabilityIgnoresDuplicateEntries_158
handleNodePaths, computeRepeaterRelayInfoMap, updateIndexedRelayActivityLocked	no, all dedupe by tx.ID (read, no test added)	—

Behaviour change to note.

• Relay-stats cache: a repeat observation through known relays no longer mutates byPathHop, so it no longer invalidates the 300 s batch relay-stats cache either. This follows the perf(nodes): batch relay stats to fix O(N×M) /api/nodes regression Kpa-clawbot/CoreScope#1164 contract (invalidate when byPathHop changes) and TestAddResolvedPubkeysToPathHopIndex_NoMutation_PreservesCache. Before, practically every late observation cleared that cache.
• fix(store): remove evicted transmissions from resolved byPathHop entries #115 eviction tests: their constants encoded the duplicates (3+3*2 entries per transmission). They now expect one entry per key. TestEvictRemovesDuplicateResolvedEntries_115 (commit 4) inserts duplicates itself, so eviction of a legacy index is still pinned.

No change to the score definition or weights. No new map[string]interface{}. No writes from the server (mode=ro is untouched).

Commits

1. 610e70e4: tests, red on master. Also benchmarks; setupTestDB now takes testing.TB so the DB-backed benchmark can reuse it.
2. b671ee14: the fix, the record tests, and the updated fix(store): remove evicted transmissions from resolved byPathHop entries #115 expectations.
3. 682cd623: perf. The distinct count no longer uses a map per bucket; adds a helper unit test and a reversed-order benchmark case.
4. baa68cdd: test only. TestEvictRemovesDuplicateResolvedEntries_115 (review nit N1).

Tests

The fixture is a real SQLite DB with 4 relays (distinct first bytes, so a raw 1-byte hop resolves uniquely), 11 observers and 20 non-advert transmissions with paths of 1–3 hops. Both live-ingest paths are covered, and both are compared to a Load of the same data with persisted resolved_path.

Test	Covers
TestPathHopIndexOncePerTx_LateObservations_158	1 + 10 observations via IngestNewObservations → once per relay bucket
TestPathHopIndexOncePerTx_LiveIngestBatch_158	11 observations in one IngestNewFromDB batch → once
TestPathHopIndexOncePerTx_AfterLoad_158	Load (with rebuild), then a live observation → once
TestTrafficShareStableAcrossLateObservations_158	shares identical after load, after ingest, and after each round of late observations; equal to the definition; sum ≤ max path length (3); nothing at the 1.0 clamp; all three score functions agree
TestPathHopIndexSizeBoundedByTransmissions_158	total byPathHop entries do not grow with observations
TestTrafficShareCountsDistinctTransmissions_158	defence: a duplicated index still yields distinct counts (3 functions)
TestCountDistinctNonAdvert_158	helper: ascending, descending, adjacent/interleaved duplicates, adverts, nils
TestPathHopResolvedRecordBounded_158	record: 2 keys per transmission after 12 observations; evicted transmissions leave it
TestPathHopResolvedRecordAcrossRebuild_158	rebuild keeps live records (no duplicate after it) and drops removed transmissions
TestPathHopConsumersIgnoreDuplicateEntries_158, TestMultiByteCapabilityIgnoresDuplicateEntries_158	audit: unaffected consumers
TestEvictRemovesDuplicateResolvedEntries_115	eviction removes every occurrence of a transmission duplicated in a resolved bucket (legacy index) and deletes a bucket left empty. Green on master 727efca0 as well (behaviour unchanged)

Red before / green after

	Passed	Failed
master be35eefb (the 8 tests that compile there)	2	6
this branch (all 11)	11	0

The 2 that pass on master are the audit tests for unaffected consumers, which is the intent.

Runs

• New tests plus the fix(store): remove evicted transmissions from resolved byPathHop entries #115 suite (-run '_158|_115'): -count=10 ok, and -race -count=10 ok (also on baa68cdd).
• Full cmd/server, go test -race -count=1 ./...:
  • after the fix commit: ok github.com/corescope/server 847.813s;
  • on the branch merged with origin/master 727efca0: ok github.com/corescope/server 881.643s (merge commit 16a65007).
• On baa68cdd, non-race go test -count=1 ./... in cmd/server: ok (68.8 s). One earlier run failed TestHandleNodePaths_HopName_CanonicalPathShowsTarget_1144 with a 503 index loading; it failed 2 of 20 runs on master 727efca0 too, and passed 20 of 20 in isolation on this branch, so it is an existing timing flake, not caused by this PR. cmd/ingestor go test ./... ok.

Merge with origin/master 727efca0 (one new commit, which only changes test-analytics-fluid-charts.js): go vet ok, cmd/ingestor go test ./... ok, node test-packet-filter.js ok, node test-aging.js ok.

• node test-frontend-helpers.js: 705 passed, 2 failed (favStar …). It fails identically on plain master be35eefb. Unrelated; this PR touches no frontend file.

Mutants

Mutant	Failing tests
M1 drop the dedupe check in addResolvedPubkeysToPathHopIndex	12 (7 × _158, 5 × _115)
M2 drop the record cleanup on eviction	TestPathHopResolvedRecordBounded_158
M3 countDistinctNonAdvert counts entries	TestTrafficShareCountsDistinctTransmissions_158
M4 rebuild clears the whole record	…RecordAcrossRebuild_158, …OncePerTx_AfterLoad_158
M5 rebuild keeps the record of removed transmissions	…RecordAcrossRebuild_158
M6 batch score counts entries	TestTrafficShareCountsDistinctTransmissions_158
M7 bulk score counts entries	TestTrafficShareCountsDistinctTransmissions_158
M8 single score counts entries	TestTrafficShareCountsDistinctTransmissions_158
M9 ascending check <= → <	TestCountDistinctNonAdvert_158
M10 eviction sweep removes only the first occurrence of each transmission per bucket	TestEvictRemovesDuplicateResolvedEntries_115 (only that test)

Perf (AGENTS.md rule 0)

Correction. An earlier version of this section claimed the branch is faster on the late-observation paths (−10 to −20 %). That is not reproduced and is withdrawn. A reviewer measured on darwin arm64 (10 cores, load < 4, n=6, median) and found the opposite direction on the bulk score pass; remeasured below.

Method. Master 727efca0 (with this branch's _158 benchmark file and the setupTestDB(testing.TB) change copied in so the same benchmarks compile) against branch 682cd623 (the production code of baa68cdd; that commit is test-only). Test binaries built once, then 6 interleaved rounds (master, head, master, head, …), -test.benchtime 1s -test.benchmem.

• Platform: linux/amd64, Go 1.24.7, Intel Xeon 2.1 GHz, 4 vCPU (cloud container).
• Load: 1-minute load average 1.2–1.8 sampled after each run (includes the benchmark itself).
• benchstat is not installed here, so: median of n=6 with [min–max]. No significance test.

Benchmark	master	branch	change (median)
LateObservationIndex_158/txs=20000	1.0 µs [0.9–1.1], 52 B/op	1.0 µs [0.8–1.2], 0 B/op	−4 % (ranges overlap)
LateObservationIndex_158/txs=100000	1.8 µs [1.6–2.0], 50 B/op	2.1 µs [1.8–2.2], 0 B/op	+19 % (ranges barely overlap)
IngestNewObservations_158	2.27 ms [1.99–2.53]	2.47 ms [2.32–2.72]	+9 % (ranges overlap)
TrafficShareScoreMap_158/txs=20000/order=ingest	0.98 ms [0.88–1.17]	1.20 ms [1.16–1.25]	+23 %
TrafficShareScoreMap_158/txs=100000/order=ingest	7.12 ms [6.73–7.54]	8.22 ms [7.60–8.68]	+15 %
TrafficShareScoreMap_158/txs=20000/order=reversed	1.04 ms [0.98–1.22]	1.66 ms [1.57–1.83]	+60 %
TrafficShareScoreMap_158/txs=100000/order=reversed	7.09 ms [6.61–7.94]	8.56 ms [8.35–9.93]	+21 %

Reading.

• Bulk score pass (cache-miss path of GetRepeaterUsefulnessScoreMap) is slower on the branch, by +15 % to +60 %. This agrees in direction with the darwin arm64 measurement (ingest order 220 µs → 285 µs at 20K and 2.43 ms → 3.22 ms at 100K, +30–32 %; reversed +58–86 %). Absolute times differ with CPU and cache, and the percentages differ by platform (here the 100K cases are smaller, +15 % and +21 %); I have not isolated why (sort cost on the reversed buckets and cache behaviour are the likely factors, not verified). The direction is the same on both platforms, so the cost is real: it is the price of the defensive distinct count, paid under the read lock on a cached path.
• Late-observation index step: no improvement shown. The darwin run saw +12–18 %; here −4 % and +19 %, with overlapping ranges at 20K. Treat it as unchanged within noise, not as a speed-up. What does change is allocation: 0 B/op on the branch against 50–52 B/op on master.
• IngestNewObservations_158: +9 % here with overlapping ranges, ±0 % on darwin: unchanged within noise. Bytes per op, first run: 776,727 on master, 832,404 on the branch (+7 %); not investigated.
• On a live master the numerator also walks every accumulated duplicate (here 137.5 entries per transmission at 20K and 18.1 at 100K after the benchmark's late observations, growing with every observation), so a real master pass is slower than the clean-index master numbers above. That is the bug being fixed, not a perf win claimed for this branch.

Memory

• pathhop-entries/tx after the benchmark's late observations: master 137.5 (20k) and 18.1 (100k), growing with every observation; branch 4.99 (3 raw + 2 resolved), constant.
• The record (pathHopResolved) measured on the heap earlier: 81.6 B/tx (20k), 68.5 B/tx (100k), with 2 relays per transmission. On master each extra observation added 16 B/tx of duplicate pointers, without bound.
• Bulk score pass allocations: about 213 KiB, unchanged (+0.1–1 % in B/op).
• Not counted in estimateStoreTxBytes: the record's memory is not included in trackedBytes. A reviewer measured the record on darwin arm64 at 6–12 MB at 100K transmissions and 46–74 MB at 500K. This will be handled in a separate follow-up issue (it also covers the resolved byPathHop entries, which estimateStoreTxBytes does not count today).

Not verified (Ikke verificeret)

• A staging round. It should confirm that the sum of all repeaters' traffic_share_score stays bounded (around the mean relay-hop count) and stops drifting over at least a day of uptime. It should also confirm that values no longer jump across a restart.
• Production-size behaviour. I have not measured the real lock-hold time of the bulk score pass, or the record's memory, on a production-size store and relay mix.
• Why the percentages differ between platforms. Only measured, not explained.
• Batch relay-stats cache. It is no longer cleared by repeat observations (see Design); it expires on its 300 s TTL as designed. I have not observed its effect on the relay counts shown in the UI.
• Browser. No browser validation (AGENTS.md rule 2): no UI change, only the numbers the API reports. The values should be checked on staging.

Overlap with other open PRs

• cmd/server/store.go is also changed by PR API: expose on-wire channelHashHex on channel messages #11 (codex/expose-channel-hash-hex). store.go auto-merges. That branch's existing conflict with master is in openapi.go and is not caused by this PR.
• No other open PR touches the files changed here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VRiEwpZQZpYPxKTJRJAvc5

[dborup]

Review feedback addressed (commit baa68cdd)

1. N1: added TestEvictRemovesDuplicateResolvedEntries_115. It puts the same transmission into a resolved bucket several times (a legacy index from before fix(store): traffic_share_score inflates with uptime — late observations re-append the same tx to byPathHop #158) and requires every occurrence to go on eviction, and the emptied bucket to be deleted. Green on this head and on master 727efca0. Mutant M10 (the sweep removes only the first occurrence of each transmission per bucket) turns it red, and only it.
2. N2: perf section rewritten with remeasured numbers (interleaved master/head, n=6, median and min–max, platform and load stated). The earlier improvement claims are withdrawn: the bulk score pass is +15 % to +60 % slower, the late-observation step is unchanged within noise (0 B/op against ~50 B/op). The record's memory (6–12 MB at 100K, 46–74 MB at 500K transmissions) not being counted in estimateStoreTxBytes is noted and goes to a separate follow-up issue.

The commit changes one test file only; no production code.

---

Generated by Claude Code