From 9f67c90cd93d77c2956882edd862083a527e758a Mon Sep 17 00:00:00 2001 From: efiten Date: Wed, 2 Sep 2026 14:34:20 +0200 Subject: [PATCH 1/3] fix(#1904): keep resolved full-pubkey hops across a path-hop index rebuild (#1907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1904. ## The bug `buildPathHopIndex` reassigned `s.byPathHop` to a fresh map and refilled it from every packet's raw `path_json` hops: ```go func (s *PacketStore) buildPathHopIndex() { s.byPathHop = make(map[string][]*StoreTx, 4096) for _, tx := range s.packets { addTxToPathHopIndex(s.byPathHop, tx) // raw hops only } ... } ``` `byPathHop` carries two kinds of key, though: those raw wire hops, and the resolved full pubkeys fed per observation by `indexResolvedPathHops`. The pubkey strings behind the second kind are retained nowhere — #800 replaced the per-`StoreTx` `ResolvedPath` field with a hash-only membership index (`resolvedPubkeyIndex` stores FNV hashes, not strings) — so the rebuild could not reproduce them and dropped them. All three call sites run post-load: `LoadChunked` (`chunked_load.go:459`), the background fill loader (`store.go:1573`), and the deferred startup build (`index_ready_1008.go:177`). The `resolved_path` branch of the chunk scan populates the index and is then silently undone a few hundred lines later, while the `resolved_path IS NULL` fallback right beside it is explicitly documented as "byNode ONLY — the resolved_path/path-hop indexes must NOT be populated here". The two branches disagreed about who owns the index. Consequence: after a cold start every lookup keyed by a node's full pubkey missed, so `relay_count_1h/24h`, `last_relayed`, `unscoped_relay_count_24h`, `transported_scopes` (#1751) and the usefulness Traffic axis all read zero until live ingestion slowly refilled the index. ## Evidence Fixture built from live data: 2512 nodes, 17,056 transmissions, 528,891 observations, 123,057 of them carrying a non-NULL `resolved_path`. ``` before [store] Built path-hop index: 2924 unique keys /api/nodes → 0 of 2000 nodes with transported_scopes 0 with relay_count_24h > 0 after [store] Built path-hop index: 3881 unique keys (172181 resolved-hop entries retained) /api/nodes → 726 with transported_scopes 741 with relay_count_24h > 0 ``` The 957 extra keys are the full pubkeys. ## The change `retainResolvedPathHops` re-merges the pre-rebuild map's entries that the raw-hop pass cannot reproduce. Entries are carried over **only for transmissions still in `s.packets`**. That filter is load-bearing rather than defensive. `removeTxFromPathHopIndex` strips raw hops only — it derives them from `txGetParsedPath` — and its companion `removeFromResolvedPubkeyIndex` cleans the hash index, not `byPathHop`. So evicted transmissions linger under their resolved keys, and the wipe this PR removes was the only thing that ever cleared them. Filtering on liveness keeps the index bounded by the eviction policy instead of converting that gap into a permanent leak. `TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904` pins it. (The eviction gap itself is pre-existing and outside this change: between rebuilds, an evicted transmission still stays referenced under its resolved keys. Filed separately.) ## Perf `O(entries in prev)` with one scratch map reused across keys (`clear()` per key, the same idiom as `hopsSeen`), plus one `map[*StoreTx]struct{}` over `s.packets` for the liveness check. It runs only where `buildPathHopIndex` already ran — cold load and background-fill completion — never on an ingest or request path. Measured on the fixture above: index build stayed within the same `LoadChunked` step, 15.2s total for 17k transmissions / 527k observations. Memory: the retained entries point at transmissions already held by `s.packets`, so no `StoreTx` is kept alive beyond eviction; the cost is map/slice overhead for keys that the feature is supposed to have. ## Tests `cmd/server/pathhop_rebuild_1904_test.go`, red before / green after: 1. `TestBuildPathHopIndex_RetainsResolvedHops_1904` — a resolved full-pubkey key survives the rebuild alongside the raw hop. 2. `TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904` — a resolved key whose transmission is no longer in `s.packets` is dropped, and the now-empty key is not left behind. 3. `TestBuildPathHopIndex_NoDuplicateOnRepeatedBuild_1904` — building twice does not double-append (`indexResolvedPathHops` dedups within a call, not across the several observations of one transmission, so `prev` can legitimately contain duplicates). ``` cd cmd/server && go test ./... ok github.com/corescope/server 85.5s go vet ./... clean ``` Frontend and ingestor suites are untouched by this change (Go server only, no `public/` files). ## Interaction with #1903 Both touch `byPathHop` semantics, so I verified them composed on the same fixture. With #1904 alone the resolved keys come back and #1902's prefix collision is plainly visible again (51% of 1-byte prefix groups reporting an identical scope set). With both: ``` f79616 BE repeater ['#be','#de','#eu','#nl'] relay24h=542 f752c2 DE/NRW repeater ['#de','#de-nw'] relay24h=343 f788ad BE repeater none relay24h=383 ``` Identical-set prefix groups fall to 8%, relay counts stay intact, and each node's scopes match what its own `resolved_path` rows say. The two changes are independent and compose cleanly. Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit f081f91b88ff698286f201e67f2442ffac78a4da) Co-Authored-By: Claude Opus 5 --- cmd/server/pathhop_rebuild_1904_test.go | 105 ++++++++++++++++++++++++ cmd/server/store.go | 73 +++++++++++++++- 2 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 cmd/server/pathhop_rebuild_1904_test.go diff --git a/cmd/server/pathhop_rebuild_1904_test.go b/cmd/server/pathhop_rebuild_1904_test.go new file mode 100644 index 000000000..7848cb6af --- /dev/null +++ b/cmd/server/pathhop_rebuild_1904_test.go @@ -0,0 +1,105 @@ +package main + +import ( + "sync" + "testing" +) + +// Issue #1904: buildPathHopIndex() rebuilds byPathHop from raw path_json +// hops and opens by discarding the map. But byPathHop also holds resolved +// full-pubkey keys fed by indexResolvedPathHops, and those pubkey strings +// are retained nowhere (#800 dropped the per-StoreTx ResolvedPath field in +// favour of a hash-only index), so a plain rebuild silently drops every +// resolved attribution. On a cold start that leaves relay_count_*, +// last_relayed and transported_scopes empty for every node until live +// ingestion refills the index. +// +// The rebuild must therefore RETAIN resolved entries — but only for +// transmissions still in s.packets. Eviction's removeTxFromPathHopIndex +// only strips raw hops (it derives them from txGetParsedPath), so evicted +// transmissions linger under their resolved keys; the rebuild is where +// they get dropped. + +const ( + hop1904Raw = "a3" + hop1904Resolved = "a3f19c2b7d4e5081aa22bb33cc44dd55ee66ff778899000112233445566778899" + hop1904Evicted = "b7002211ffeeddccbbaa99887766554433221100aabbccddeeff001122334455" +) + +// pathHopTx builds a transmission whose wire path is a single raw hop. +func pathHopTx(id int, path string) *StoreTx { + return &StoreTx{ID: id, Hash: "pathhop-tx-" + path, PathJSON: `["` + hop1904Raw + `"]`} +} + +func newPathHopStore(packets []*StoreTx, idx map[string][]*StoreTx) *PacketStore { + return &PacketStore{packets: packets, byPathHop: idx, mu: sync.RWMutex{}} +} + +func hopKeyHas(idx map[string][]*StoreTx, key string, tx *StoreTx) bool { + for _, t := range idx[key] { + if t == tx { + return true + } + } + return false +} + +func TestBuildPathHopIndex_RetainsResolvedHops_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + // Shape produced by a chunk scan: raw hop from path_json PLUS the + // resolved full pubkey that indexResolvedPathHops added. + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + hop1904Resolved: {tx}, + }) + + store.buildPathHopIndex() + + if !hopKeyHas(store.byPathHop, hop1904Raw, tx) { + t.Fatalf("raw hop %q lost by rebuild", hop1904Raw) + } + if !hopKeyHas(store.byPathHop, hop1904Resolved, tx) { + t.Fatalf("resolved pubkey hop %q dropped by rebuild — #1904", hop1904Resolved) + } +} + +func TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904(t *testing.T) { + live := pathHopTx(1, hop1904Raw) + evicted := pathHopTx(2, hop1904Raw) // NOT in s.packets any more + + store := newPathHopStore([]*StoreTx{live}, map[string][]*StoreTx{ + hop1904Raw: {live}, + hop1904Resolved: {live}, + hop1904Evicted: {evicted}, + }) + + store.buildPathHopIndex() + + if hopKeyHas(store.byPathHop, hop1904Evicted, evicted) { + t.Fatal("evicted transmission retained under its resolved key — the rebuild must not turn eviction's known gap into a permanent leak") + } + if _, ok := store.byPathHop[hop1904Evicted]; ok { + t.Fatalf("empty key %q left behind after dropping its only entry", hop1904Evicted) + } + if !hopKeyHas(store.byPathHop, hop1904Resolved, live) { + t.Fatal("live resolved hop dropped while pruning evicted ones") + } +} + +func TestBuildPathHopIndex_NoDuplicateOnRepeatedBuild_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + hop1904Resolved: {tx}, + }) + + store.buildPathHopIndex() + store.buildPathHopIndex() + + if got := len(store.byPathHop[hop1904Raw]); got != 1 { + t.Fatalf("raw hop bucket has %d entries after two builds, want 1", got) + } + if got := len(store.byPathHop[hop1904Resolved]); got != 1 { + t.Fatalf("resolved hop bucket has %d entries after two builds, want 1", got) + } +} diff --git a/cmd/server/store.go b/cmd/server/store.go index 7a4a6ba86..e3334b122 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -4155,14 +4155,81 @@ func (s *PacketStore) buildSubpathIndex() { len(s.spIndex), s.spTotalPaths) } -// buildPathHopIndex scans all packets and populates byPathHop. +// buildPathHopIndex rebuilds byPathHop: raw wire hops from every packet's +// path_json, plus the resolved full-pubkey hops carried over from the +// previous index (see retainResolvedPathHops). // Must be called with s.mu held. func (s *PacketStore) buildPathHopIndex() { - s.byPathHop = make(map[string][]*StoreTx, 4096) + prev := s.byPathHop + s.byPathHop = make(map[string][]*StoreTx, max(4096, len(prev))) for _, tx := range s.packets { addTxToPathHopIndex(s.byPathHop, tx) } - log.Printf("[store] Built path-hop index: %d unique keys", len(s.byPathHop)) + retained := s.retainResolvedPathHops(prev) + log.Printf("[store] Built path-hop index: %d unique keys (%d resolved-hop entries retained)", + len(s.byPathHop), retained) +} + +// retainResolvedPathHops re-merges the entries of a pre-rebuild byPathHop +// that the raw-hop pass above cannot reproduce: the resolved full-pubkey +// keys fed by indexResolvedPathHops. Their pubkey strings are retained +// nowhere — #800 dropped the per-StoreTx ResolvedPath field in favour of a +// hash-only membership index — so a plain rebuild silently discarded every +// resolved relay attribution, leaving relay counts and transported scopes +// empty after each cold load until live ingestion refilled them (#1904). +// +// Only transmissions still in s.packets are carried over. This matters: +// eviction's removeTxFromPathHopIndex strips raw hops only (it derives them +// from txGetParsedPath), so evicted transmissions linger in prev under their +// resolved keys. Filtering them here is what keeps the index bounded by the +// eviction policy instead of turning that gap into a permanent leak. +// +// Cost is O(entries in prev) with one reused scratch map, and it runs only +// where buildPathHopIndex already runs — cold load and background-fill +// completion — never on an ingest or request path. +// +// Returns the number of entries carried over. Must be called with s.mu held, +// after s.byPathHop has been rebuilt from raw hops. +func (s *PacketStore) retainResolvedPathHops(prev map[string][]*StoreTx) int { + if len(prev) == 0 { + return 0 + } + live := make(map[*StoreTx]struct{}, len(s.packets)) + for _, tx := range s.packets { + live[tx] = struct{}{} + } + + // Reused across keys (cleared per key) so a large index does not churn + // one map allocation per key. Guards against both a key that the raw + // pass already produced and repeated appends of the same tx in prev — + // indexResolvedPathHops dedups within a call, not across the several + // observations of one transmission. + seen := make(map[*StoreTx]struct{}, 16) + retained := 0 + for key, list := range prev { + if len(list) == 0 { + continue + } + clear(seen) + for _, tx := range s.byPathHop[key] { + seen[tx] = struct{}{} + } + for _, tx := range list { + if tx == nil { + continue + } + if _, ok := live[tx]; !ok { + continue + } + if _, dup := seen[tx]; dup { + continue + } + seen[tx] = struct{}{} + s.byPathHop[key] = append(s.byPathHop[key], tx) + retained++ + } + } + return retained } // addTxToPathHopIndex indexes a transmission under each unique raw hop key. From d722ca27d29b6803e6f4ebe777d591e1f86fc365 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 18:50:45 +0200 Subject: [PATCH 2/3] fix(store): update the restart-shape precondition the retention fix invalidates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR as submitted broke an existing test on master. My full -race run caught it (CI would have too): TestNodeActivity_AlternateObservationRelay- SurvivesRestart passes on master and fails here, subtest persisted_resolved_path=true, at "fixture must reproduce the restart index shape". That test's precondition asserted the #1904 gap EXISTS — that after a restart the tx is absent from byPathHop under the resolved relay key — so the rest of it could check activity survives despite the gap. Retaining resolved hops makes that precondition false, which is the fix working, not a regression. It also answers the open question about whether this helps a cold start. It does, for a persisted resolved_path: buildPathHopIndex runs more than once during Load, and the later build retains what the earlier population established ("3 resolved-hop entries retained" in the test's own log). Without a persisted resolved_path there is still nothing to retain, which is why only the persisted=true subtest changed. The precondition now asserts the CORRECTED shape rather than dropping the check: the relay bucket must contain the tx exactly when resolved_path was persisted (got != persisted), so it fails both on a revert to the old behaviour and on over-retention. The separate assertion that a non-display raw hop ("a1") is never indexed is kept. Verified: disabling retention fails it, and dropping the live-set filter fails the eviction test. Full go test -race ./... is clean (262s, 0 races). Co-Authored-By: Claude Opus 5 --- cmd/server/node_liveness_regression_test.go | 24 +++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/cmd/server/node_liveness_regression_test.go b/cmd/server/node_liveness_regression_test.go index aef934f67..d5291f3eb 100644 --- a/cmd/server/node_liveness_regression_test.go +++ b/cmd/server/node_liveness_regression_test.go @@ -451,12 +451,32 @@ func TestNodeActivity_AlternateObservationRelaySurvivesRestart(t *testing.T) { if tx == nil || len(tx.Observations) != 2 || tx.PathJSON != `["D4","E7"]` { t.Fatalf("fixture must load two observations with the longer display path: %+v", tx) } - for _, bucket := range []string{relay, "a1"} { + // Index shape after a restart. The raw-hop pass never produces + // these buckets: the display path is ["D4","E7"], so "a1" and the + // full relay pubkey can only come from a RESOLVED path. + // + // Before #1904 they were absent in both cases, because + // buildPathHopIndex rebuilt from raw hops and discarded whatever + // indexResolvedPathHops had added. Since that fix, a restart that + // has resolved_path persisted keeps the relay bucket — the index + // is built more than once during Load, and the later build + // retains what the earlier population established. Without a + // persisted resolved_path there is still nothing to retain. + indexedIn := func(bucket string) bool { for _, indexed := range loaded.byPathHop[bucket] { if indexed == tx { - t.Fatalf("fixture must reproduce the restart index shape; tx found in byPathHop[%s]", bucket) + return true } } + return false + } + if got := indexedIn(relay); got != persisted { + t.Fatalf("byPathHop[relay] contains tx = %v, want %v (persisted resolved_path = %v)", got, persisted, persisted) + } + // "a1" is a raw wire hop of the SHORTER observation, which is not + // the display path, so it is never indexed either way. + if indexedIn("a1") { + t.Fatal("tx must not be indexed under a non-display raw hop") } // Live: same rows ingested after startup. From af3523e293a8368119351c1fa35d66a34517c4b7 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Sun, 20 Sep 2026 19:00:53 +0200 Subject: [PATCH 3/3] reviewfix(store): honour the relay-stats cache contract, close two mutants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review disproved my cold-start worry by execution: prev is NOT empty on a cold start, because both load paths populate byPathHop with resolved keys during the row scan (chunked_load.go scanAndMergeChunk, and the legacy Load) BEFORE buildPathHopIndex replaces the map. Measured on a DB with one persisted resolved_path through LoadChunked: 1 entry with the fix, 0 with retention stubbed out. The reported "relay counts empty after restart" symptom is genuinely fixed. Three findings addressed: 1. buildPathHopIndex violated a contract the codebase states about itself. addResolvedPubkeysToPathHopIndex says mutating byPathHop "MUST be paired with invalidateRelayStatsCache". A rebuild replaces the entire map and invalidated nothing. That was survivable while a rebuild only ever DISCARDED relay attribution; now that it restores it, the 300s batch cache could pin the pre-rebuild empty relay stats for minutes after the index was fixed — hiding exactly the data this retention restores. GetRepeaterNodeStatsBatchCached is not gated on PathHopIndexReady and HTTP binds before the load completes, so one request landing just before the rebuild is enough. Now invalidated. 2. Two mutants survived the original tests. Deleting the in-loop seen[tx] marking passed everything, as did skipping a whole key when ANY entry is non-live — because every test bucket held a single transmission. Added a mixed live+evicted bucket test and one for repeated appends of the same tx (which indexResolvedPathHops genuinely produces: it dedups within a call, not across the several observations of one transmission). Both mutants now fail. 3. Documented, not changed: that dedup alters repeater usefulness scores. Three readers count ENTRIES rather than distinct transmissions, so a multi-observer relay measured 3 entries before a rebuild and 1 after. The collapsed value is the correct one and the dedup is load-bearing for boundedness, but it is a user-visible score change worth naming. Also renamed the log line to "entries carried over", matching what the merge actually does rather than implying only resolved keys qualify. go test -race ./... clean (255s, 0 failures, 0 races). Co-Authored-By: Claude Opus 5 --- cmd/server/pathhop_rebuild_1904_test.go | 55 +++++++++++++++++++++++++ cmd/server/store.go | 11 ++++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/cmd/server/pathhop_rebuild_1904_test.go b/cmd/server/pathhop_rebuild_1904_test.go index 7848cb6af..b9704912b 100644 --- a/cmd/server/pathhop_rebuild_1904_test.go +++ b/cmd/server/pathhop_rebuild_1904_test.go @@ -103,3 +103,58 @@ func TestBuildPathHopIndex_NoDuplicateOnRepeatedBuild_1904(t *testing.T) { t.Fatalf("resolved hop bucket has %d entries after two builds, want 1", got) } } + +// A bucket holding BOTH a live and an evicted transmission is the realistic +// shape — several relays share a resolved pubkey key, and eviction removes +// them one at a time. Testing only single-tenant keys let a mutation that +// drops the whole key when ANY entry is non-live pass unnoticed. +func TestBuildPathHopIndex_MixedLiveAndEvictedBucket_1904(t *testing.T) { + live := pathHopTx(1, hop1904Raw) + evicted := pathHopTx(2, hop1904Raw) + + store := newPathHopStore([]*StoreTx{live}, map[string][]*StoreTx{ + hop1904Raw: {live}, + // One key, both transmissions: the evicted one must be pruned and + // the live one must survive. + hop1904Resolved: {evicted, live}, + }) + + store.buildPathHopIndex() + + if !hopKeyHas(store.byPathHop, hop1904Resolved, live) { + t.Fatal("live transmission dropped from a bucket it shared with an evicted one") + } + if hopKeyHas(store.byPathHop, hop1904Resolved, evicted) { + t.Fatal("evicted transmission retained from a mixed bucket") + } + if got := len(store.byPathHop[hop1904Resolved]); got != 1 { + t.Fatalf("mixed bucket has %d entries after rebuild, want exactly the live one", got) + } +} + +// indexResolvedPathHops dedups within a call but not across the several +// observations of one transmission, so a pre-rebuild bucket legitimately +// holds the same *StoreTx more than once. The retain pass must collapse +// those: without it the bucket grows on every rebuild, and the repeater +// usefulness readers count ENTRIES, not distinct transmissions. +func TestBuildPathHopIndex_CollapsesRepeatedTxInPrevBucket_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + // Three observations of one transmission → three appends. + hop1904Resolved: {tx, tx, tx}, + }) + + store.buildPathHopIndex() + + if got := len(store.byPathHop[hop1904Resolved]); got != 1 { + t.Fatalf("resolved bucket has %d entries after rebuild, want 1 — repeated appends of one tx must collapse", got) + } + + // And it must stay collapsed rather than regrowing across rebuilds. + store.buildPathHopIndex() + if got := len(store.byPathHop[hop1904Resolved]); got != 1 { + t.Fatalf("resolved bucket regrew to %d entries on a second rebuild", got) + } +} diff --git a/cmd/server/store.go b/cmd/server/store.go index 237708918..dbfb4d14e 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -4164,7 +4164,16 @@ func (s *PacketStore) buildPathHopIndex() { addTxToPathHopIndex(s.byPathHop, tx) } retained := s.retainResolvedPathHops(prev) - log.Printf("[store] Built path-hop index: %d unique keys (%d resolved-hop entries retained)", + // addResolvedPubkeysToPathHopIndex states the contract: mutating + // byPathHop must be paired with invalidateRelayStatsCache. A rebuild + // replaces the whole map, and now also restores relay attribution, so + // without this the 300s batch cache can pin the pre-rebuild empty relay + // stats for minutes AFTER the index was fixed — hiding exactly the data + // this retention restores. GetRepeaterNodeStatsBatchCached is not gated + // on PathHopIndexReady, and HTTP binds before the load completes, so a + // request landing just before the rebuild is enough to trigger it. + s.invalidateRelayStatsCache() + log.Printf("[store] Built path-hop index: %d unique keys (%d entries carried over from the previous index)", len(s.byPathHop), retained) }