Repository navigation
port(upstream#1907): keep resolved full-pubkey hops across a path-hop index rebuild - #35
Merged
dborup merged 4 commits intoSep 20, 2026
Conversation
…op index rebuild (Kpa-clawbot#1907) Fixes Kpa-clawbot#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 — Kpa-clawbot#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` (Kpa-clawbot#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 Kpa-clawbot#1903 Both touch `byPathHop` semantics, so I verified them composed on the same fixture. With Kpa-clawbot#1904 alone the resolved keys come back and Kpa-clawbot#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) <noreply@anthropic.com> (cherry picked from commit f081f91) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nvalidates 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 Kpa-clawbot#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 <noreply@anthropic.com>
…tants 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #25 (commit
b251f299there). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
f081f91b88ff698286f201e67f2442ffac78a4dagit cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.cmd/server/store.go, newpathhop_rebuild_1904_test.go.Problem
buildPathHopIndexrebuiltbyPathHoponly from each packet's rawpath_jsonhops. The resolved full-pubkey keys thatindexResolvedPathHopsadds are stored nowhere else, so every cold load or background-fill completion silently dropped all resolved relay attribution. Relay counts and transported scopes were then empty until live ingest refilled them.Change
retainResolvedPathHopsre-merges entries from the pre-rebuild index for transmissions still present ins.packets, de-duplicated and with evicted transmissions filtered out so the index stays bounded. Cost is O(entries in the old index), only where the rebuild already runs (cold load / background fill), never on ingest or request paths.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Notes for review
Relevant to this fork: our relay/transported-scope views read
byPathHop.Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5go-server-build-vetgo-server-test-racechannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-lintBaseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Not applicable (no frontend change).
Not run:
eslint(not installed locally; CI installs it on the fly).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34750398749 on
9f67c90c. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code