fix(store): traffic_share_score inflates with uptime — late observations re-append the same tx to byPathHop

[dborup]

Summary

traffic_share_score (the "Traffic share" axis, and through it 20 % of usefulness_score) grows steadily with server uptime and ends up far above reality. A repeater went from ~10 % to 50 %+ overnight with no change on the air. Cause: the same transmission is appended to a repeater's byPathHop bucket once per late-arriving observation, while the denominator counts it once.

Evidence (public /api/nodes, 2026-10-01)

Instance	Version	Uptime	Sum of all repeaters' traffic_share_score	Repeaters clamped at 1.0
prod (meshview.dk)	1d19cd5	98 h	235.8	many (e.g. DK_0999_DRBYEN, DK_5700_Svendborg_N)
staging	d264716c	47 h	147.8	many

The sum should be roughly the average number of relay hops per non-advert packet (a small number). Right after a restart the index is rebuilt cleanly and values look plausible; they then drift upwards as live observations arrive.

Cause (master be35eefb)

• computeRepeaterUsefulnessScoreMap() (cmd/server/repeater_enrich_bulk.go, around line 222): share = |non-advert txs in byPathHop[pubkey]| / |non-advert txs in byPayloadType|, clamped to 1. It counts list entries, not distinct transmissions.
• IngestNewObservations() (cmd/server/store.go, around line 3393) calls indexResolvedPathHops(tx, pks, …) for each new observation of an existing transmission.
• addResolvedPubkeysToPathHopIndex() (around line 4570) only de-duplicates within one call (hopsSeen is cleared on entry); it never checks whether tx is already in byPathHop[pk]. Every further observation through the same relays appends tx again.
• Load and background chunk load index the union of resolved pubkeys once per transmission, which is why a restart temporarily "fixes" the numbers.
• The eviction leak in fix(store): remove evicted transmissions from resolved byPathHop entries #115 (resolved keys not removed on eviction; fixed on master by fix(store): remove evicted transmissions from resolved byPathHop entries #144, not yet deployed) inflated the numerator further while the denominator shrank.

addToByNode() does not have this problem (it de-duplicates via nodeHashes[pubkey][tx.Hash]), and retainResolvedPathHops() already de-duplicates by *StoreTx when it walks the index.

Proposed fix

1. Make the byPathHop insert idempotent per (key, transmission): e.g. keep a small per-transmission set of resolved keys already indexed (or reuse an existing per-tx structure), and skip the append when present. Bounded: at most one entry per relay key per transmission. Keep eviction (fix(store): remove evicted transmissions from resolved byPathHop entries #144) consistent with it.
2. Defensive: count distinct transmissions in computeRepeaterUsefulnessScoreMap() (and GetRepeaterUsefulnessScore).
3. Audit the other byPathHop consumers for the same inflation: GetRepeaterNodeStatsBatch (repeater_usefulness.go), GetNodeHopAnalytics, computeMultiByteCapability (store.go), and fix any that count entries.

Acceptance criteria

• A transmission ingested once and then observed by N more observers through the same relays appears once in each relay's byPathHop bucket (test with N = 10), via both the live ingest and the late-observation paths.
• traffic_share_score for a fixed set of transmissions is identical right after load and after any number of extra observations (test).
• Property test: the sum of all repeaters' traffic_share_score is bounded by the maximum path length; no value is clamped at 1.0 in a realistic fixture.
• Audited consumers either count distinct transmissions or are shown not to be affected (tests).
• Memory/perf: no unbounded growth; benchmark the observation-ingest path before/after (AGENTS.md rule 0). Full cmd/server suite under -race.

Not in scope

No change to the score definition or weights; no deploy in this issue (a staging round should confirm the sum stays bounded over a day of uptime).

[dborup]

Fixed by #162 (merged as 1dbe141e).

byPathHop now holds each transmission at most once per relay key, however many late observations arrive. The traffic-share score counts distinct transmissions.

On staging, the sum of traffic_share_score dropped from 182.8 (53 nodes clamped at 1.0, 71 h uptime) to about 14–22, with 0 nodes clamped. RSS dropped from 2.8 GB to 1.5–2.1 GB.

The remaining slow rise after #162 (14 → about 22 over a day) has a different cause: live ingest and Load index relay hops from different resolvers. That is tracked and fixed in #182, which is in review.

Related follow-ups:

• fix(store): memory estimate ignores resolved byPathHop entries and the #162 pathHopResolved side map #164: the memory estimate does not count resolved byPathHop entries or the fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162 side map.
• ingestor resolves only ~half of relay paths (resolved_path NULL in ~53 % of observations); possible hasResolvedPath startup race #184, feat(ingestor): resolve the last hop from the observer, relay-only prefix index, backfill NULL resolved_path #188: the ingestor resolves only about half of relay paths; feat(ingestor): resolve the last hop from the observer (#188) #190 adds the observer as anchor.

[dborup]

Rapport — CS-cloud PR#ny upstream-port #162 — head a21edc2

Status: Port of the #162 fix is on upstream/pathhop-dedupe-158 (dborup/CoreScope), ready for an upstream PR after review. Red on upstream master, green on the branch; full suites green. Nothing was written to Kpa-clawbot/CoreScope.

Evidence tags: [T] test or CI run, [A] analysis, [K] taken from #162 and not rerun (none used).

Branch

• Branch: upstream/pathhop-dedupe-158 in dborup/CoreScope, pushed to origin only.
• Head: a21edc2e03f2acd333ea4365c79dd8cbac3a0776, on top of upstream master 415362c5 (checked again right before the push: unchanged).
• Commits, all authored and committed as dborup <kontakt@meshview.dk> (checked with git log after each commit; no rebase, amend or force-push):
  1. 785ad31b test(store): traffic share must not grow with late observations (red tests, benchmarks, setupTestDB(testing.TB))
  2. 362b4fd5 fix(store): index each transmission once per relay key in byPathHop
  3. a21edc2e test(store): record is cleared when a rebuild starts from an empty index (see M11 below)
• Diff: 7 files, +933/−54. New: pathhop_dedupe_test.go, pathhop_resolved_record_test.go. Changed: store.go, repeater_usefulness.go, repeater_enrich_bulk.go, pathhop_eviction_1908_test.go, db_test.go.
• Test and benchmark names use the suffix _PathHopDedupe; no downstream issue or PR numbers appear in code, comments or commit messages (grep on added lines and commit messages: none).
• The commit messages carry a Co-Authored-By: Claude <noreply@anthropic.com> trailer and a Claude-Session: link. Decide before the upstream PR whether to keep them; squash-merging upstream would drop them.

Differences from #162, and why

• [A] Eviction. Upstream's evictStaleInternal already sweeps every byPathHop bucket by evicted transmission ID (slices.DeleteFunc), raw and resolved keys, every occurrence. fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162's eviction part therefore reduces to deleting the evicted transmission's record (delete(s.pathHopResolved, tx)), placed in the first pass of the loop because upstream has no evictedTxSet of pointers.
• [A] fix(store): remove evicted transmissions from resolved byPathHop entries #144 is not a prerequisite. Nothing in the ported design depends on the prefix-scoped sweep of fix(store): remove evicted transmissions from resolved byPathHop entries #144. The port stays inside fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162's scope.
• [T] Upstream's own eviction test pinned the bug shape. TestPathHopEviction1908 created duplicate resolved entries by calling indexResolvedPathHops again and expected the duplicate. With the fix it fails on exactly that ([3 4 5 6 7 8], want [3 4 5 6 7 8 8]). It now appends the duplicates directly, so it still pins that eviction removes every occurrence. This replaces fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162's TestEvictRemovesDuplicateResolvedEntries; the other fix(store): remove evicted transmissions from resolved byPathHop entries #115 constant changes have no counterpart upstream.
• [A] Record tests rebuilt on upstream helpers. fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162's record tests used fork-only helpers; they now use a local phdRecordStore on top of makeTestStore.
• [A] Audit differs. Upstream GetNodeHopAnalytics no longer reads byPathHop (it walks s.packets), so it is dropped from the duplicate-entries test; computeRepeaterRelayInfoMap is added.
• [A] New audit finding. computeRepeaterRelayInfoMap dedupes by transmission ID only when it folds in the 1-byte prefix bucket; for a full key with no prefix bucket it would count duplicates in its own bucket. Not reachable with the index fixed (one entry per key and transmission), so left unchanged and noted in the PR description.
• [T] Extra test and commit (M11). The mutant "empty previous index keeps the record" survived the ported tests, so commit 3 adds TestPathHopResolvedRecordClearedWithEmptyIndex_PathHopDedupe.
• [A] Same as fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162 and unchanged by design: the bulk pass applies the distinct count to 64-character keys only; the record's memory is not counted in trackedBytes.

Red before / green after

[T] Upstream master code (415362c5 plus the commit-1 tests, 785ad31b); the three record tests and the helper test reference the new field/function and do not compile on master:

Test	master	branch
TestPathHopIndexOncePerTx_LateObservations_PathHopDedupe	FAIL (11 entries per relay)	PASS
TestPathHopIndexOncePerTx_LiveIngestBatch_PathHopDedupe	FAIL	PASS
TestPathHopIndexOncePerTx_AfterLoad_PathHopDedupe	FAIL	PASS
TestTrafficShareStableAcrossLateObservations_PathHopDedupe	FAIL (all four relays at the 1.0 clamp, sum 4.0 > 3)	PASS
TestPathHopIndexSizeBoundedByTransmissions_PathHopDedupe	FAIL	PASS
TestTrafficShareCountsDistinctTransmissions_PathHopDedupe	FAIL	PASS
TestPathHopConsumersIgnoreDuplicateEntries_PathHopDedupe	PASS (audit)	PASS
TestMultiByteCapabilityIgnoresDuplicateEntries_PathHopDedupe	PASS (audit)	PASS
TestPathHopResolvedRecordBounded_PathHopDedupe	does not compile	PASS
TestPathHopResolvedRecordAcrossRebuild_PathHopDedupe	does not compile	PASS
TestPathHopResolvedRecordClearedWithEmptyIndex_PathHopDedupe	does not compile	PASS
TestCountDistinctNonAdvert_PathHopDedupe	does not compile	PASS

Totals: master 8 compiled tests, 2 passed / 6 failed; branch 12 passed / 0 failed.

Suites and checks

• [T] cd cmd/server && go test -race -count=1 ./... on branch head: ok (56.9 s); also ok at the fix commit (60.8 s).
• [T] Non-race go test -count=1 . three times each, interleaved, on pristine upstream master and on the branch: ok in all six runs (about 20.5–21.1 s).
• [T] cd cmd/ingestor && go test ./...: ok (109 s).
• [T] go vet ./... clean; gofmt -l on all changed files: clean; added map[string]interface{}: 0.
• [T] Known flake (index loading 503 in the _1144 test): not reproduced, so no flake is called baseline. The _1144 test ran 20 times on both master and branch: ok. Across 1 race run plus 3 non-race runs on the branch and 3 on master, no failure occurred.
• [T] TestPathHopDedupe tests under -race -count=5: ok.

Mutants

[T] Each mutant was applied to a copy of the branch tree and the whole cmd/server package was run.

Mutant	Failing tests
M1 no dedupe check	7: OncePerTx ×3, SizeBounded, StableAcrossLateObservations, RecordBounded, RecordAcrossRebuild
M2 / M12 no record cleanup on eviction	RecordBounded
M3 distinct count counts entries	CountDistinctNonAdvert, CountsDistinctTransmissions
M4 rebuild clears the whole record	OncePerTx_AfterLoad, RecordAcrossRebuild
M5 rebuild keeps the record of removed transmissions	RecordAcrossRebuild
M6 batch score counts entries	CountsDistinctTransmissions
M7 bulk score counts entries	CountsDistinctTransmissions
M8 single score counts entries	CountsDistinctTransmissions
M9 ascending check <= → <	CountDistinctNonAdvert
M10 record never stored	same 7 as M1
M11 empty previous index keeps the record	RecordClearedWithEmptyIndex (new, commit 3)

(#162's M10 "eviction removes only the first occurrence" has no counterpart: upstream's sweep filters by ID and removes every occurrence.)

Benchmarks

[T] Upstream master code plus the branch's benchmark file against the branch; test binaries built once, 6 interleaved rounds, -test.benchtime 1s -test.benchmem. Platform linux/amd64, Go 1.24.7, Intel Xeon 2.1 GHz, 4 vCPU (container); 1-minute load average 1.1–1.7 around the runs. No benchstat: median of n=6 with [min–max].

Benchmark	master	branch	change
LateObservationIndex txs=20000	1.25 µs [1.06–1.40], 38 B/op	1.18 µs [1.06–1.38], 0 B/op	−6 % (overlap)
LateObservationIndex txs=100000	2.21 µs [1.91–2.35], 61.5 B/op	2.12 µs [1.90–2.67], 0 B/op	−4 % (overlap)
IngestNewObservations	1.49 ms [1.43–1.75]	1.53 ms [1.31–2.18]	+3 % (overlap)
TrafficShareScoreMap 20000 ingest	1.40 ms [1.30–1.67]	1.70 ms [1.59–1.87]	+21 %
TrafficShareScoreMap 100000 ingest	8.11 ms [7.71–8.83]	9.85 ms [9.56–10.72]	+21 %
TrafficShareScoreMap 20000 reversed	1.36 ms [1.29–1.48]	2.18 ms [2.09–2.26]	+60 %
TrafficShareScoreMap 100000 reversed	8.09 ms [7.77–8.50]	11.41 ms [10.44–12.61]	+41 %

• No speed-up is claimed. The late-observation step and IngestNewObservations are unchanged within noise; allocation of the index step drops to 0 B/op.
• The bulk score pass on a clean index is slower on the branch (+21 % in ingest order, +41 to +60 % when every bucket is out of order), as in fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162. It is the price of the defensive distinct count.
• [T] Memory: pathhop-entries/tx is 5.0 on the branch and grows on master (100 at 20k, 16 at 100k, depends on iterations); the record costs 81.7 B/tx (20k) and 68.5 B/tx (100k) on the heap; bulk pass allocations are unchanged (+0.1–0.5 % B/op). Not counted in estimateStoreTxBytes (suggest a follow-up).
• Not run: the memory and benchmark picture on a production-size store, and a staging round to confirm the score sum stays bounded over a day of uptime.

Not checked

• Browser validation (no UI change) and staging/prod behaviour.
• Nothing in this report is carried over from fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162: every number was produced again on this branch, so no [K] tags are used.
• Routine "Re-check CI PR 182" (trig_01TdvJUzB1qeXBGXMJSM9ejf) was already disabled (a fired one-shot), so nothing was changed.

Draft upstream PR description (also kept as a file in scratch)

fix(store): index each transmission once per relay key in byPathHop

Relates to upstream issue: <NUMBER — filled in by lead>

traffic_share_score (and through it the usefulness score) drifts upwards with server uptime: many repeaters end up at the 1.0 clamp, and the values look right again right after a restart. The same transmission is appended to a relay's byPathHop bucket once per late-arriving observation, while the score's denominator counts it once. This was also observed downstream in a fork with the same code shape (fixed there).

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: each transmission once.
• Numerator: bucket entries.

indexResolvedPathHops runs once per observation, on the live-ingest path (IngestNewFromDB, several observations of a transmission in one batch), on the late-observation path (IngestNewObservations), and in Load / the background chunk load. 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.

A rebuild (buildPathHopIndex → retainResolvedPathHops) deduplicates by *StoreTx. That is why the numbers looked right after a restart and then drifted.

Reproduced on upstream master 415362c5: after 1 + 10 observations through the same three relays, each relay bucket holds the transmission 11 times, and all four relays of the test fixture sit at the 1.0 clamp (their correct values are 0.4–0.5).

Design

1. Idempotent insert. addResolvedPubkeysToPathHopIndex keeps, per transmission, the hashes of the resolved keys it already appended the transmission under, in a new side map 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 (a side map rather than a struct field, so its size does not move).
• Keyed by *StoreTx, like the dedupe in retainResolvedPathHops.

2. Consistent with eviction and rebuild.

• Eviction: upstream's evictStaleInternal already sweeps every byPathHop bucket by evicted transmission ID, so resolved entries (and any duplicates) are already removed. The only addition is deleting the evicted transmission's record in the same first pass.
• Rebuild: retainResolvedPathHops carries the resolved entries of live transmissions over, so their record stays valid. It drops the record of transmissions that are no longer in s.packets, and clears it when there is no previous index.

3. Defence in depth. GetRepeaterUsefulnessScore, GetRepeaterNodeStatsBatch and computeRepeaterUsefulnessScoreMap 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 it to full-pubkey (64-character) 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 10x slower and was replaced.

Behaviour change to note. 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 existing contract (invalidate when byPathHop changes). Before, practically every late observation cleared that cache.

No change to the score definition or weights. No new map[string]interface{}. No writes from the server (read-only invariant untouched).

Audit of the other byPathHop consumers

Consumer	Affected?	Evidence
GetRepeaterUsefulnessScore, GetRepeaterNodeStatsBatch (score), computeRepeaterUsefulnessScoreMap	yes, counted entries, fixed	TestTrafficShareCountsDistinctTransmissions_PathHopDedupe, TestTrafficShareStableAcrossLateObservations_PathHopDedupe
collectRelayEntriesLocked (relay info, batch)	no, dedupes by tx.ID	TestPathHopConsumersIgnoreDuplicateEntries_PathHopDedupe
computeRepeaterRelayInfoMap (bulk relay info)	only partly: it dedupes by tx.ID when it folds in the 1-byte prefix bucket, but not within a bucket that has no prefix bucket. Not reachable with the index fixed (one entry per key and transmission); left unchanged	TestPathHopConsumersIgnoreDuplicateEntries_PathHopDedupe (with a prefix bucket present)
computeMultiByteCapability	no, reads raw prefix keys only and keeps a maximum	TestMultiByteCapabilityIgnoresDuplicateEntries_PathHopDedupe
handleNodePaths	no, dedupes by tx.ID (read, no test added)	n/a
GetNodeHopAnalytics	does not read byPathHop (it walks s.packets)	n/a

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 compared with a Load of the same data with persisted resolved_path.

Test	Covers
TestPathHopIndexOncePerTx_LateObservations_PathHopDedupe	1 + 10 observations via IngestNewObservations → once per relay bucket
TestPathHopIndexOncePerTx_LiveIngestBatch_PathHopDedupe	11 observations in one IngestNewFromDB batch → once
TestPathHopIndexOncePerTx_AfterLoad_PathHopDedupe	Load (with rebuild), then a live observation → once
TestTrafficShareStableAcrossLateObservations_PathHopDedupe	shares identical after load, after ingest and after each round of late observations; equal to the definition; sum ≤ max path length; nothing at the clamp; the three score functions agree
TestPathHopIndexSizeBoundedByTransmissions_PathHopDedupe	total byPathHop entries do not grow with observations
TestTrafficShareCountsDistinctTransmissions_PathHopDedupe	defence: a duplicated index still yields distinct counts (3 functions)
TestCountDistinctNonAdvert_PathHopDedupe	helper: ascending, descending, adjacent/interleaved duplicates, adverts, nils
TestPathHopResolvedRecordBounded_PathHopDedupe	record: 2 keys per transmission after 11 observations; evicted transmissions leave it
TestPathHopResolvedRecordAcrossRebuild_PathHopDedupe	rebuild keeps live records, drops removed transmissions
TestPathHopResolvedRecordClearedWithEmptyIndex_PathHopDedupe	rebuild from an index with no resolved entries clears the record
TestPathHopConsumersIgnoreDuplicateEntries_PathHopDedupe, TestMultiByteCapabilityIgnoresDuplicateEntries_PathHopDedupe	audit: unaffected consumers

TestPathHopEviction1908 seeded duplicate resolved entries by calling indexResolvedPathHops again. That no longer appends, so it now appends the duplicates directly: it still pins that eviction drops every occurrence.

Red before / green after (upstream master 415362c5 code with the new tests; the three record tests and the helper test reference the new field/function and do not compile there):

	Passed	Failed
upstream master code (the 8 tests that compile)	2	6
this branch (all 12)	12	0

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

Runs: go test -race -count=1 ./... in cmd/server: ok (about 57–61 s). Non-race go test -count=1 . three times each on master and on the branch: ok every time (about 20.5–21 s). cmd/ingestor: go test ./... ok. go vet and gofmt -l are clean on the changed files.

Mutants

Mutant	Failing tests
M1 drop the dedupe check	7 (…OncePerTx_* ×3, …SizeBounded…, …StableAcrossLateObservations…, …RecordBounded…, …RecordAcrossRebuild…)
M2 / M12 drop the record cleanup on eviction	TestPathHopResolvedRecordBounded_PathHopDedupe
M3 distinct count counts entries	TestCountDistinctNonAdvert_PathHopDedupe, TestTrafficShareCountsDistinctTransmissions_PathHopDedupe
M4 rebuild clears the whole record	…OncePerTx_AfterLoad…, …RecordAcrossRebuild…
M5 rebuild keeps the record of removed transmissions	…RecordAcrossRebuild…
M6 batch score counts entries	TestTrafficShareCountsDistinctTransmissions_PathHopDedupe
M7 bulk score counts entries	same
M8 single score counts entries	same
M9 ascending check <= → <	TestCountDistinctNonAdvert_PathHopDedupe
M10 record never stored	same 7 tests as M1
M11 empty previous index keeps the record	TestPathHopResolvedRecordClearedWithEmptyIndex_PathHopDedupe

Perf

Method. Baseline = upstream master code plus this branch's benchmark file (so the same benchmarks compile); branch = this branch. Test binaries built once, then 6 interleaved rounds (baseline, branch, baseline, branch, …), -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.1–1.7, sampled around each run (includes the benchmark itself).
• No benchstat: median of n=6 with [min–max], no significance test.

Benchmark	master	branch	change (median)
LateObservationIndex/txs=20000	1.25 µs [1.06–1.40], 38 B/op	1.18 µs [1.06–1.38], 0 B/op	−6 % (ranges overlap)
LateObservationIndex/txs=100000	2.21 µs [1.91–2.35], 61.5 B/op	2.12 µs [1.90–2.67], 0 B/op	−4 % (ranges overlap)
IngestNewObservations	1.49 ms [1.43–1.75]	1.53 ms [1.31–2.18]	+3 % (ranges overlap)
TrafficShareScoreMap/txs=20000/order=ingest	1.40 ms [1.30–1.67]	1.70 ms [1.59–1.87]	+21 %
TrafficShareScoreMap/txs=100000/order=ingest	8.11 ms [7.71–8.83]	9.85 ms [9.56–10.72]	+21 %
TrafficShareScoreMap/txs=20000/order=reversed	1.36 ms [1.29–1.48]	2.18 ms [2.09–2.26]	+60 %
TrafficShareScoreMap/txs=100000/order=reversed	8.09 ms [7.77–8.50]	11.41 ms [10.44–12.61]	+41 %

Reading.

• The bulk traffic-share pass (cache-miss path of GetRepeaterUsefulnessScoreMap) is slower on the branch, by about 21 % in ingest order and 41–60 % when every bucket is out of order. This is the price of the defensive distinct count, paid under the read lock on a cached path. It is real, not noise.
• Late-observation index step and IngestNewObservations: no speed-up is claimed. Both are unchanged within noise. What changes is allocation (about 38–61 B/op on master, 0 on the branch for the index step) and the index size.
• On a live master the numerator also walks every accumulated duplicate, so a real master pass is slower than the clean-index baseline above. That is the bug, not a win claimed for this branch.

Memory

• pathhop-entries/tx after the benchmark's late observations: master 100 (20k) and 16 (100k) and growing with every observation; branch 5.0 (3 raw + 2 resolved), constant.
• The record (pathHopResolved) measured on the heap with 2 relays per transmission: 81.7 B/tx (20k) and 68.5 B/tx (100k).
• Bulk score pass allocations: about 213 KiB, unchanged (+0.1–0.5 % B/op).
• Not counted in estimateStoreTxBytes: the record's memory is not part of trackedBytes, and neither are the resolved byPathHop entries today. Suggested as a separate follow-up.

Not verified

• A staging round: the sum of all repeaters' traffic_share_score should stay bounded (around the mean relay-hop count) and stop drifting over at least a day of uptime, and values should no longer jump across a restart.
• Lock-hold time of the bulk score pass and the record's memory on a production-size store and relay mix.
• No browser validation: no UI change, only the numbers the API reports.

---

Generated by Claude Code

[adminopenclaw8-sketch]

Review — CS-Macmini review upstream-port #162 — head a21edc2

Verdict: APPROVE with nits. The code fix is correct on all three indexing paths. Before the upstream PR: add one test (F1), open it from a branch with clean commit messages (F2), and fix the PR text (section 9).

Reviewed read-only: git archive of head a21edc2e03f2acd333ea4365c79dd8cbac3a0776 and of upstream master 415362c5, in scratch directories. git ls-remote showed the same head before and after the review. Nothing was pushed, and nothing was written to the upstream repository.

Evidence tags: [T] test or benchmark run here, [A] analysis, [K] taken from the cloud report and not rerun.

Findings

#	Prio	Where	Finding
F1	medium	tests	No test covers a later observation of the same transmission that resolves through a new relay. The server resolves hops with the observer as context, so this happens. The code handles it correctly (probe below), but mutant MH ("once a tx has a record, later observations add no new keys") survives the whole cmd/server package [T]. Add a test.
F2	medium	commit messages, branch name	All 3 commits carry a Claude-Session: https://claude.ai/code/session_… line. Upstream's last 200 commits have 0 such lines and 97 Co-authored-by: Claude trailers. Upstream's squash message includes the commit bodies (see 415362c5), so the link would land in upstream history. The branch name upstream/pathhop-dedupe-158 also carries a fork issue number. Recommend a new branch with identical trees and clean messages (details in 8).
F3	low–medium	repeater_enrich_bulk.go, repeater_usefulness.go	On Apple M4 the bulk score pass costs more than the cloud numbers show: +30–51 % in ingest order and +56–97 % reversed across two interleaved sets (the cleaner set: +35–37 % and +56–82 %), against +21 % / +41–60 % on the cloud Xeon. In absolute terms that is about +0.1–1.6 ms per pass, on a background-recompute path. Acceptable, but the PR should show it as platform-dependent. A cheaper fast path helps only marginally; dropping the defensive count from the bulk pass is the real option (see 6).
F4	low	PR text	Several claims are not proven on this branch, one wording is inaccurate, and the issue link and red-CI convention are missing (see 9).
F5	low	pathHopResolved	The record (68–82 B per transmission [K]) is not in trackedBytes. That is about 1.5 % of upstream's own estimate for a typical transmission with 5 observations (about 5 KB from estimateStoreTxBytesTypical) [A]. A follow-up is enough, but the PR should state the size.
F6	low	retainResolvedPathHops	The record is reset only when the previous index is empty (the M11 case). With a previous index that holds raw keys but lacks the resolved entries the record claims, the record survives, and the next observation is skipped (probe below: 3 entries, want 5) [T]. Not reachable through production code: only buildPathHopIndex replaces byPathHop, and it carries prev over [A]. Optional hardening: rebuild the record from the entries actually retained. That would also make the M11 special case and the separate liveness loop unnecessary.
F7	info	computeRepeaterRelayInfoMap	Confirmed: it dedupes by tx.ID only when a 1-byte prefix bucket exists. Not reachable with the fix [A]. Leaving it unchanged is right (see 4).

1. Correctness on upstream's paths

• [A] All three paths now go through indexResolvedPathHops → addResolvedPubkeysToPathHopIndex:

  • IngestNewFromDB (store.go around line 2967);
  • IngestNewObservations (around line 3300);
  • Load (around line 990) and the chunk loader (chunked_load.go around line 635), both followed by buildPathHopIndex (store.go:1663, chunked_load.go:493, index_ready_1008.go:177).

  The record check sits in that single function, so every path is covered.

• [T] Red on master, green on the branch. 785ad31b (upstream code plus the commit-1 tests) fails 6 of the 8 tests that compile there; the two audit tests pass as intended. Head passes all 12 new tests plus TestPathHopEviction1908. This matches the cloud table.

• [A] Correction to the premise. Upstream PR 2099 (415362c5) changes only frontend files (public/*.js, a JS unit test). The server's resolvePathForObs(pathJSON, observerID, …) has used the observer as resolution context since upstream PR 556 (ae38cdef). Either way, the answer is yes: observations of one transmission can resolve the same hop to different pubkeys.

• [T]/[A] This is handled correctly. The record holds every key the transmission was indexed under. A new key is appended once, and known keys are skipped. The result is the union of all observations' relays, each key once. The probe indexResolvedPathHops(tx, [A,B]), then [A,C], then both again, gives tx exactly once in A, B and C, with 3 hashes in the record. It is green on head and red under MH (key eeee…: tx indexed 0 times). The branch tests only use identical relay sets per observation, hence F1.

• [A] Note for the PR (not a defect): union attribution means an ambiguous prefix resolved to X by one observer and to Y by another credits both. Master did the same, with multiplicity. Attribution quality is out of scope.

• [A] Collision argument: a hash collision can only make the code skip an append, never add one, so it cannot inflate a count. Correct.

2. Eviction and the changed TestPathHopEviction1908

• [A] Confirmed. evictStaleInternal sweeps every byPathHop key with slices.DeleteFunc on the evicted ID set (store.go around line 5066), so every occurrence goes, raw and resolved. The only addition needed is delete(s.pathHopResolved, tx) in the first loop.
• [A] The changed test seeds exactly the duplicates the old version produced through the bug: tx 1 twice in shared and in expiredOnly, and tx 8 twice in shared. The expectations are unchanged.
• [T] It is still as strong. My mutant MA ("the sweep drops only the first occurrence per evicted ID per bucket") turns TestPathHopEviction1908 red.
• [A] Upstream acceptance risk is low. The old seeding called indexResolvedPathHops twice and relied on the very append the fix removes, and the new comment says why. Optional nit: say in the comment that the leftover 8, 8 in shared is deliberate (eviction is not a dedupe).

3. Rebuild

• [A] retainResolvedPathHops drops records of transmissions that are not in s.packets, keeps the others, and clears all of them when prev is empty. Carried-over entries are deduped by *StoreTx, so a pre-fix index with duplicates is cleaned on the first rebuild.
• [A] The record of a live transmission stays valid because its resolved entries are always in prev: the record is only written together with an append, and only eviction removes resolved entries, and it deletes the record too.
• [T] The M11 test pins the empty-prev case. The general form, prev without the resolved entries, is F6 (probe red, unreachable).
• [A] Interplay with the chunk loader: each chunk indexes under s.mu and writes records, and the final buildPathHopIndex carries both over. Live ingest between chunks uses the same idempotent insert, so neither the startup rebuild goroutine (index_ready_1008.go) nor the chunked rebuild can re-introduce duplicates.

4. computeRepeaterRelayInfoMap

• [A] It allocates the seen map only when snap[key[:2]] is non-empty, so a full-key bucket without a prefix bucket would count duplicates. With the fix, a full-key bucket holds each transmission once: the record covers live inserts, and retain dedupes on rebuild. Raw buckets are deduped per call by addTxToPathHopIndex. So it is not reachable.
• [A] Leave it. Fixing it means a map per key on every pass, which is the same cost argument the PR makes for the bulk score. Optional: a one-line comment in the code.
• The audit table is otherwise complete: the only other byPathHop readers are collectRelayEntriesLocked, handleNodePaths and computeMultiByteCapability. GetNodeHopAnalytics does not read it (0 references).

5. Tests and mutants

• [T] cd cmd/server && go test -race -count=1 ./... on head: ok (49 s).
• [T] cd cmd/ingestor && go test ./...: ok (87 s).
• [T] go vet . clean, and gofmt -l clean on the 7 changed files.
• [T] Added map[string]interface{}: 0.
• Platform: macOS, Apple M4 (10 cores), Go 1.27.0.
• [K] make test (all 14 modules) was not run, by cloud or by me.

Own mutants (not in the cloud table), each run against the whole cmd/server package on a copy of head:

Mutant	Result	Caught by
MA eviction sweep drops only the first occurrence per evicted ID	red	TestPathHopEviction1908
MB pathHopKeyHash constant (every key collides)	red	10 tests, incl. …OncePerTx_* ×3, record tests, TestRelaySnapshotSurvivesEviction1908
MC distinct count also counts adverts	red	TestCountDistinctNonAdvert_PathHopDedupe, TestTrafficShareCountsDistinctTransmissions_PathHopDedupe
MF invalidate the relay-stats cache even on a no-op repeat	red	TestAddResolvedPubkeysToPathHopIndex_NoMutation_PreservesCache (existing upstream test, which pins the claimed behaviour change)
MH once a tx has a record, later observations add no new relay keys	survives	none; F1. A probe test (below) catches it

Probe tests (scratch only, not pushed):

• TestReviewProbeLaterObservationNewRelay (F1): green on head, red under MH.
• TestReviewProbeRawOnlyPrevIndex (F6): red on head, unreachable in production.

Suggested F1 test, unit level:

store.indexResolvedPathHops(tx, []string{a, b}, hs) // observer 1
store.indexResolvedPathHops(tx, []string{a, c}, hs) // observer 2: same hop, other relay
store.indexResolvedPathHops(tx, []string{a, b}, hs)
store.indexResolvedPathHops(tx, []string{a, c}, hs)
// want: tx once in a, b and c; 3 hashes in pathHopResolved[tx]

A DB-backed variant through IngestNewObservations, with two observers whose paths resolve differently, would be even better.

6. Perf

[T] Apple M4, Go 1.27.0, test binaries built once: master = 785ad31b (upstream code plus the benchmark file), branch = head. There were 5 interleaved rounds, -test.benchtime 1s -test.benchmem, and the 1-minute load average was 2.3–3.0 during the rounds. Median [min–max] of n=5, no benchstat:

Benchmark	master	branch	change
LateObservationIndex txs=20000	0.29 µs [0.28–0.30], 46 B/op	0.30 µs [0.30–0.32], 0 B/op	+3 %
LateObservationIndex txs=100000	0.68 µs [0.65–0.70], 37 B/op	0.72 µs [0.70–0.73], 0 B/op	+6 %
IngestNewObservations	0.14 ms [0.13–0.16]	0.13 ms [0.13–0.16]	−11 % (overlap)
TrafficShareScoreMap 20000 ingest	0.22 ms [0.21–0.23]	0.30 ms [0.28–0.30]	+35 %
TrafficShareScoreMap 100000 ingest	2.34 ms [2.28–2.50]	3.21 ms [3.08–3.27]	+37 %
TrafficShareScoreMap 20000 reversed	0.23 ms [0.21–0.24]	0.42 ms [0.42–0.45]	+82 %
TrafficShareScoreMap 100000 reversed	2.33 ms [2.31–2.42]	3.64 ms [3.58–4.13]	+56 %

• Honest and reproducible: yes, in direction. The bulk pass is slower on a clean index, and the index step is flat with 0 B/op.

• Not reproduced in magnitude: the relative cost is higher on M4 than on the cloud Xeon. The PR should give both platforms, or a range, plus the absolute cost.

• LateObservationIndex is not a like-for-like comparison. On master the index grows during the run (414.8 and 40.3 pathhop-entries/tx at the end, against 4.99 on the branch), so its "+3–6 %" should not be read as a regression or as a win. The draft already avoids claiming one.

• Acceptable for upstream? I think so. The pass runs from the background enrichment recomputer (AnalyticsDefaultRecomputeInterval) and on cache misses, under the read lock, and adds about 1–1.6 ms at 100k transmissions.

• A cheaper distinct count: only marginally. The current fast path writes every ID into the scratch slice even when the bucket turns out to be ascending. A variant that only counts and compares with the previous ID, and falls back to collect-and-sort on the first out-of-order ID, passes the new tests. Measured in a second interleaved set (5 rounds of master/branch/prototype; load average 3.8–5.7, so noisier than the table above):

  Benchmark	master	branch	prototype
  20000 ingest	0.24 ms [0.22–0.25]	0.31 [0.30–0.34] (+30 %)	0.29 [0.26–0.34] (+21 %)
  100000 ingest	3.09 [2.54–3.58]	4.67 [4.10–5.93] (+51 %)	4.22 [3.66–5.06] (+36 %)
  20000 reversed	0.24 [0.23–0.25]	0.47 [0.47–0.57] (+97 %)	0.50 [0.46–0.54] (+106 %)
  100000 reversed	3.08 [2.80–3.26]	4.92 [4.26–5.42] (+60 %)	4.65 [3.76–5.22] (+51 %)

  The prototype gains a little in ingest order (medians −6 to −10 % against the branch, ranges overlapping) and nothing on reversed buckets. It does not close the gap. [T]

• The bigger lever is to drop the defensive count from the bulk pass only (the record already guarantees one entry per key) and keep it in the single and batch paths, where it is per key and cheap.

7. Memory

• [K] The record costs 68–82 B per transmission with 2 relays (cloud heap measurement). I did not re-measure.
• [A] Relative to upstream's own per-transmission estimate (about 2.2 KB without observations, about 5.1 KB with 5 observations) that is roughly 1.5–3.5 % that eviction does not see. The resolved byPathHop entries are not tracked today either (pre-existing).
• Recommendation: a follow-up is enough, because accounting it properly touches accountedBytes and rechargeTx. The PR should state the number and that eviction does not see it, instead of only "not counted".

8. Upstream hygiene

• [T] Added lines and commit messages contain no #N references, no fork issue or PR numbers, no hostnames, IPs, domains or private repos, and no "fork"/"downstream" wording.
• [T] Author and committer on all 3 commits: dborup <kontakt@meshview.dk>.
• [T] Test names use the neutral _PathHopDedupe suffix.
• Trailers. Keep Co-Authored-By: Claude (upstream norm: 97 of the last 200 commits). Drop Claude-Session: (0 of 200).
• Recommendation. Since commits must not be rewritten, create a new branch: cherry-pick the same three trees with edited messages (check that git diff a21edc2e <new-head> is empty). Use a name without the fork number, e.g. fix/pathhop-dedupe-once-per-tx, and open the upstream PR from it.

9. PR description: suggested changes

1. Issue line. Replace Relates to upstream issue: <NUMBER …> with upstream's usual first line, a closing keyword plus the upstream issue number 2108 (as in their recent PR bodies). Inside the upstream repo that reference resolves correctly; no other #N occurs in the draft.
2. Red CI evidence. Upstream PRs usually open with a "Red commit: sha (CI assertion failures link)" line. The fork's CI has no runs for 785ad31b, 362b4fd5 or a21edc2e, so either get a CI run of the red commit or say plainly that red/green was measured locally (with the table).
3. Not proven on this branch:
   • "A first version used a map per bucket; it made the bulk pass about 10x slower": measured on the fork only. Drop it, or label it as measured on the fork.
   • "Bounded: one 8-byte hash per relay key": the measured heap cost is 68–82 B per transmission. State that instead.
4. Inaccurate wording. "Only out-of-order buckets (background chunk loads) are sorted". A late observation that adds an older transmission to a relay bucket, and a rebuild's carry-over order, also make buckets out of order. Say "e.g." or list them.
5. Perf section. Add the second platform, or present a range, and give the absolute cost (about +0.1–1.6 ms per pass at 20k–100k transmissions, background recompute path).
6. Behaviour change paragraph. Cite TestAddResolvedPubkeysToPathHopIndex_NoMutation_PreservesCache as the test that pins it (mutant MF).
7. Tests. Add the F1 test to the table once it exists. Optionally add a line on observer-dependent resolution: union, each key once.
8. Memory. Quantify: about 70–80 B per transmission, about 1.5–3.5 % of upstream's per-transmission estimate, not seen by memory-based eviction; follow-up proposed.
9. Runs. Upstream's AGENTS.md mentions make test for all 14 modules; either run it or say that cmd/server and cmd/ingestor were run.
10. Optional scope note. The issue mentions a residual rise caused by live versus cold-load resolution. One sentence saying it is out of scope here would pre-empt the question.

The draft has no other problems against upstream's AGENTS.md: it gives a perf justification with benchmarks, tests, no new map[string]interface{}, says there is no UI change (no browser validation needed), and is clean markdown.

Not verified

• make test across all modules; behaviour on a production-size store or on staging; the score sum over a day of uptime.
• The record's heap size [K], and fix(store): index a transmission once per relay key in byPathHop (traffic share inflation) #162-era measurements quoted in the draft.
• Linux/amd64 benchmarks (only the cloud ran them [K]); benchstat significance.
• Whether upstream's squash setting keeps commit bodies for this PR: inferred from 415362c5's message format [A].

[dborup]

Follow-up: the residual rise of the traffic share sum with uptime, after #162, is fixed by #182 (merged as 9d29daec).

The cause was that live ingest indexed relays with the server's own resolver, while Load used the ingestor's persisted resolved_path. Live observations are now indexed from the same persisted column, so the score should no longer drift between restarts. A staging round (v0.2.1) will confirm this over a day of uptime.