fix(store): remove evicted transmissions from resolved byPathHop entries

[dborup]

Relates to #115

Plan and design

The user asked for autonomous work, so the plan is written here instead of waiting for sign-off (AGENTS.md rule 5).

Commits:

1. 1e7ce403: tests (red on master).
2. 1a2f7733: complete removal.
3. 1477f579: cost proportional to the batch. The test fixture now uses realistic, prefix-consistent pubkeys, and there is a new case plus a one-minute-batch benchmark.

Claims in the issue, verified against master

• indexResolvedPathHops → addResolvedPubkeysToPathHopIndex puts a transmission under resolved full-pubkey keys in byPathHop, once per observation that resolved it.
• Eviction removed a transmission only from the keys of its raw PathJSON hops, and only one occurrence each.
• removeFromResolvedPubkeyIndex cleans a different (hash-only, perf: remove per-StoreTx ResolvedPath, replace with membership index + on-demand decode (final spec) Kpa-clawbot/CoreScope#800) index.
• Evicted transmissions therefore stayed in relay counts, transported scopes and memory until a later full rebuild. Reproduced.

Change (cmd/server/store.go)

evictFromPathHopIndex(idx, evicted) runs once per eviction batch, under the write lock eviction already holds.

Which buckets. Only buckets whose key starts (case-insensitively) with a hop of an evicted transmission. Hops are taken from tx.PathJSON and every observation's PathJSON. This is complete because:

• byPathHop's resolved keys come only from persisted resolved_path (the path_json reconstruction on cold load goes into byNode only).
• The ingestor resolves each hop through prefixIndex, which maps a hop prefix to the pubkeys that start with it.

What happens in those buckets.

• All duplicates are removed (slices.DeleteFunc).
• Empty buckets are deleted.
• The compacted tail is zeroed, so no backing array keeps an evicted transmission alive. This also covers a transmission that was not at the front of its bucket; background chunks load older transmissions after newer ones.

Other changes.

• removeTxFromSlice (raw path change) now removes every occurrence and zeroes the tail as well.
• The relay-stats cache is invalidated after the sweep.

Unchanged. The rebuild defence (retainResolvedPathHops), byNode/nodeHashes, the hash-only reverse index, and relay readers materializing owned values under RLock (the fork's model, as the issue asks).

How this differs from upstream Kpa-clawbot/CoreScope#1966

Upstream is read as a reference only; nothing was cherry-picked.

• The reader-side changes are not copied (the fork keeps materializing under RLock).
• The removal is prefix-scoped, not a global sweep.
• Hops from every observation's path are included.
• Tail zeroing and bucket deletion are pinned by tests.

Acceptance criteria

Criterion	Status	Evidence
Remove every evicted transmission from all raw and resolved buckets, including duplicates	Met	TestEvictRemovesRawAndResolvedPathHops_115 (both resolved-index modes), TestEvictRemovesResolvedKeysOfOtherObservationPaths_115
Delete empty buckets; no retention through oversized pointer arrays	Met	Same tests (bucket deletion), TestEvictDoesNotRetainPointerInRawBucketTail_115
Surviving entries and related invariants preserved	Met	Survivors keep all their entries (count checked); existing eviction, path-hop and relay suites pass
Relay/stat caches invalidated	Met	TestEvictionInvalidatesRelayStatsCache_115
Retention and memory eviction; rebuild cannot resurrect	Met	TestRunEvictionRemovesResolvedPathHops_115, TestMemoryEvictionRemovesResolvedPathHops_115, TestRebuildAfterEvictionDoesNotResurrect_115
Relay aggregation concurrent with eviction stays race-free	Met	TestRelayStatsConcurrentWithEviction_115 under -race

Tests

	Passed	Failed
master (same test file)	2	6
this branch	8	0

• The two that pass on master are the rebuild and cache-invalidation guards.
• -race -run _115 on the branch: ok.
• Mutation: dropping the observation-path hops makes TestEvictRemovesResolvedKeysOfOtherObservationPaths_115 fail.

Perf (AGENTS.md rule 0; eviction runs every minute under the write lock)

BenchmarkEvictPathHops_115: 3 raw one-byte hops per transmission, 2 resolved to one of 4000 prefix-consistent relays, 2 observations each. Min / median in ms, both run interleaved on the same loaded machine. Master's code is the incomplete raw-only removal.

Store / batch	master	this branch
20k / 2 (a one-minute batch)	0.10 / 0.13	0.86 / 1.06
100k / 10 (a one-minute batch)	0.43 / 0.56	2.1 / 2.6
20k / 200 (1%)	0.66 / 1.31	5.1 / 5.7
100k / 1000 (1%, e.g. after downtime)	5.1 / 9.1	15.4 / 15.5

• The extra cost is one short prefix lookup per byPathHop key (≈ relays + hop prefixes), plus sweeping the candidate buckets that master left dirty.
• A first version swept every bucket for every batch; the prefix scoping replaced that.
• About 2 ms per minute at 100k transmissions is the price of complete cleanup.

Full server -race

• A run of the first version on this branch: FAIL only in TestIssue1008_HandlerReturns503WhileSubpathIndexLoading (timing-dependent; passes 30/30 in isolation on master and branch). Reproduced on origin/master d264716c: go test -race -count=200 -cpu 1,2,8 -run '^TestIssue1008_HandlerReturns503WhileSubpathIndexLoading$' failed 2 of 600 runs, twice, with the identical message (status = 200, want 503). It is a scheduling race in the test: the background subpath build on a tiny DB can finish before the handler call. This PR does not touch that build or the handler. The master full-suite run that passed (ok, 1307 s) simply did not hit it.
• The final version (with 1477f579): ok github.com/corescope/server 1613.295s.

Not verified

• Eviction on a production-size store and relay mix (staging), and the real lock-hold time there.
• That no deployment has a resolved_path whose pubkey does not start with its hop. The ingestor's prefixIndex guarantees it. A violating key would stay until the next rebuild, as every resolved key does on master today.

Overlap with other open PRs

• cmd/server/store.go is also changed by PR fix(store): preserve first_seen ordering when merging background chunks #137 (fix(store): preserve first_seen ordering when merging background chunks #114, loadChunk publication) and the upcoming fix(analytics): recompute cached snapshots after the complete startup load #116 PR (TriggerDistanceIndexBuild, a struct field). Different functions.
• The branch was cut from 85bfee49 and merges cleanly with origin/master d264716c. A merge simulation of all 13 batch branches in issue order merges this one without conflicts.

🤖 Generated with Claude Code

https://claude.ai/code/session_019TcZHooUiiknVWbECVWzk8

---

Generated by Claude Code

[dborup]

Independent review of 1477f579

Verdict: APPROVE with nits. This is a recommendation only; merging is the owner's call.

Reviewed head: 1477f579529512a9cf673ad22d78152ec796b8f2 (unchanged before and after the review). Work done on git archive trees of the head, of 1e7ce403 (commit A, test-first), of 1a2f7733 (commit B), of origin/master 5f493f1d, and of a stacked merge tree master -> PR #137 (1ca172d8) -> this PR.

Labels: [F] freshly verified by me · [T] taken from the PR text · [A] assumption · [K] known limitation.

Findings

1. P3. The eviction tests use only lowercase wire hops; real path_json hops are uppercase cmd/server/store.go:4557. The ingestor writes hops with strings.ToUpper (cmd/ingestor/decoder.go:268), and test-fixtures/e2e-fixture.db has paths such as ["8A","33","D6",...]. The fixture used by every _115 test is ["aa","bb","cc"] (cmd/server/eviction_test.go:48). Mutant M6 removes h = strings.ToLower(h) in addHops, and all 109 related tests stay green. On real data that mutant would sweep nothing, and every evicted transmission would stay in byPathHop. The head is correct: my probe test re-indexes the fixture with ["AA","BB","CC"] and passes on the head, but fails under M6 ("evicted tx 1 still in byPathHop 9 times"). Suggest adding an uppercase-hop case. [F]

2. P3. The behaviour change in removeTxFromSlice is untested cmd/server/store.go:4522. It now removes every occurrence and zeroes the tail. Its only production caller is the path-change branch (store.go:3438). Mutant M9 restores the old single-occurrence append(list[:i], list[i+1:]...), and all 109 related tests stay green. The change is harmless, but it is not pinned by any test. [F]

3. P3 (perf observation, not blocking). With a skewed relay mix, the lock hold per minute grows with the size of the hot buckets cmd/server/store.go:4575-4588. slices.DeleteFunc always scans each candidate bucket in full. Master stopped at the first match, which is near the front for the oldest transmissions. In my benchmark, 20 hot relays carry 80% of the resolved hops, the wire hops are uppercase, and each transmission has 2 observations. Eviction under the write lock took (3 interleaved runs, noisy host):

   • 30k/5: 0.37–0.55 ms
   • 100k/10: 2.9–4.0 ms
   • 300k/30: 12.5–27 ms
   • master (incomplete cleanup), same cases: 0.03–0.05 ms, 0.10–0.24 ms and 0.51–0.59 ms

   This is bounded by the entries under the touched prefixes and runs once a minute. At AGENTS.md's 30K scale it is well under 1 ms. The production lock-hold time is already listed as not verified in the PR. [F]

4. nit. Stale comment cmd/server/pathhop_rebuild_1904_test.go:18-21. It still says eviction's removeTxFromPathHopIndex "only strips raw hops … evicted transmissions linger under their resolved keys". After this PR, eviction does not call that function. [F]

5. nit. The PR body's reasoning is incomplete. It says byPathHop resolved keys "come only from persisted resolved_path". Live ingest also resolves on the server with resolvePathForObs (store.go:2955-2959, store.go:3314-3318). The prefix invariant the fix relies on still holds on that path:

   • resolveWithContext takes its candidates from pm.m[hop] (store.go:7093).
   • buildPrefixMap only keys a node under pk[:l] and the full pk (store.go:6995-7001).

   So the code is correct. Only the argument in the PR body is incomplete. [F]

Metadata

• CI on 1477f579 is complete: Go Build & Test, Playwright E2E and Docker are SUCCESS; the rest are SKIPPED [F].
• Commits origin/master..head: 1e7ce403, 1a2f7733, 1477f579. Author and committer are dborup <kontakt@meshview.dk> on all three. [F]
• Files changed: cmd/server/store.go (+92/−21) and cmd/server/evict_resolved_pathhop_115_test.go (new, 305 lines). No workflow changes. [F]
• merge-base origin/master head = 85bfee49. The branch is behind 5f493f1d. [F]
• git merge-tree --write-tree origin/master head is clean (09df8fef). [F]
• Stacked merge-tree: master+fix(store): preserve first_seen ordering when merging background chunks #137 (d0afc227, throwaway commit 115c8ee3) then +fix(store): remove evicted transmissions from resolved byPathHop entries #144 is clean (a06dc0db). The stacked store.go contains both mergeByFirstSeen and evictFromPathHopIndex. [F]
• PR body: says "Relates to fix(store): remove evicted transmissions from resolved byPathHop entries #115", with no closing keyword and closingIssuesReferences: []. There are no @mentions. Upstream appears only in code format (Kpa-clawbot/CoreScope#1966), with no github.com/Kpa-clawbot URL. It is a draft. [F]
• gofmt -l is clean on both touched files, and go vet . is clean. staticcheck is not installed. [F]

Acceptance criteria (issue #115)

Criterion	Result
Remove every evicted tx from all raw and resolved byPathHop buckets, including duplicates	Met [F]. A single pass per batch (store.go:5111) covers the hops of tx.PathJSON plus every observation's PathJSON. Each resolved key comes from resolved_path (the ingestor's unique-prefix resolvePath) or from the server's resolvePathForObs, so it starts with a hop of the path of the observation that produced it. The ingestor upsert keys on (transmission_id, observer_idx, path_json), so resolved_path stays aligned with its path_json. Mutants M1, M2, M3 and M10 are caught. It also works for uppercase hops (probe), though that is untested (finding 1).
Delete empty buckets; no retention via backing arrays	Met [F]. Mutant M5 (keep empty bucket) is caught, and M7 (compaction without zeroing the tail) is caught by TestEvictDoesNotRetainPointerInRawBucketTail_115.
Preserve surviving entries and index invariants	Met [F]. Removal is by pointer identity against the evicted set. The tests check that each survivor keeps exactly 7 refs (9 in the memory-eviction case). byNode, nodeHashes and the Kpa-clawbot#800 index are untouched. Readers iterate under RLock (routes.go:2282, store.go:10005-10025, forEachRelayCandidate is nil-safe).
Invalidate relay/stat caches	Met [F]. invalidateRelayStatsCache() runs after the sweep. Mutant M8 is caught. The test was already green on master, which already invalidated the cache there, so it is a guard rather than a red test.
Retention and memory eviction; rebuild cannot resurrect	Met [F]. TestRunEvictionRemovesResolvedPathHops_115, TestMemoryEvictionRemovesResolvedPathHops_115 and TestRebuildAfterEvictionDoesNotResurrect_115 (the rebuild defence retainResolvedPathHops is unchanged).
Relay aggregation concurrent with eviction is race-free	Met [F]. TestRelayStatsConcurrentWithEviction_115 passes under -race on the head and on the stacked tree. Only one RunEviction overlaps the readers, so it is a light race probe.

Test-first and mutants

Red and green:

• Head test file on master: 6 FAIL, 2 PASS. The two passes are the rebuild and cache guards, as the PR states. [F]
• Commit A's tests on the A tree: 5 FAIL, 2 PASS. [F]
• B: 7/7 PASS. Head: 8/8 PASS. [F]

Changes to the tests between A and the head, both justified [F]:

• B: A asserted that the evict115Only bucket is deleted after a partial memory eviction, which was a test bug. B split this into assertEvictedGone115Partial.
• Head: the fixture pubkeys became prefix-consistent (aa…/bb…/cc…) to match the prefix-scoped design and real resolver output. A new case for other-observation paths was added, and the benchmark was extended.

Mutants were run against the head's store.go. Each was checked against 109 tests (-run 'Evict|PathHop|Relay|_115|Removal|RemoveTx'). The original was restored afterwards (shasum 496c1684… matches git show 1477f579:cmd/server/store.go). [F]

#	Mutant	Result
M1	Revert to per-tx removeTxFromPathHopIndex (raw only)	caught (5 _115 tests)
M2	Drop the observation-path hops	caught (…OtherObservationPaths_115)
M3	Exact key match only (no prefix match for resolved keys)	caught (5)
M4	hasEvictedHopPrefix always true (global sweep)	survived. Equivalent for correctness (perf only), and covered by the benchmark
M5	Keep empty buckets	caught (4)
M6	No ToLower on hops	survived. A real gap on uppercase data, proven by the probe (finding 1)
M7	Compact without zeroing the tail	caught (…RawBucketTail_115)
M8	Drop invalidateRelayStatsCache()	caught
M9	Old single-occurrence removeTxFromSlice	survived (finding 2)
M10	Drop the tx.PathJSON hops	caught (6)
M11	No ToLower on the key prefix	survived. Equivalent in practice: byPathHop keys are lowercased on insert (addTxToPathHopIndex, and resolvers lowercase the pubkeys)

Suites run locally

• Head, cd cmd/server && go test -race -count=1 -timeout 60m ./...: ok github.com/corescope/server 869.739s, exit 0. The known TestIssue1008 flake did not occur. This includes readonly_invariant_test.go. [F]
• Stacked tree (master + fix(store): preserve first_seen ordering when merging background chunks #137 + fix(store): remove evicted transmissions from resolved byPathHop entries #144), -race on the new tests of both PRs (_114|_115): 6 _114 tests and 8 _115 tests PASS, ok. A wider stacked -race subset (_114|_115|Evict|PathHop|Chunk|MergeByFirstSeen|Relay) also passes: ok 29.1s. [F]
• Stacked tree, full go test -race -count=1 -timeout 60m ./...: ok github.com/corescope/server 876.103s, exit 0. [F]
• The ingestor is not touched, so it was not run. [F]

Performance and security

• Complexity per batch [F]:

  • Collecting the hops: O(evicted × (hops + observations)).
  • Then one pass over the byPathHop keys with ≤ len(lens) (usually 1–3) map lookups each. strings.ToLower does not allocate for keys that are already lowercase.
  • Then a full DeleteFunc over the candidate buckets only.
  • Everything runs under the write lock eviction already holds, and nothing new is taken under it.
  • The worst case (a batch touching every 1-byte prefix) is O(total entries), the same shape as compactDistIndex.

• PR benchmark BenchmarkEvictPathHops_115, reproduced interleaved (3 rounds, host shared with other agents, so noisy). Master vs head, ms/op [F]:

  • 20k/2: 0.017–0.03 vs 0.25–0.61
  • 100k/10: 0.07–0.47 vs 1.2–3.0
  • 20k/200: 0.29–0.98 vs 2.9–4.6
  • 100k/1000: 1.8–5.0 vs 7.7–11.5

  The order of magnitude is consistent with the PR's table [T]. The skewed-mix numbers are in finding 3.

• Bounded structures [F]:

  • The fix removes an unbounded-until-rebuild leak.
  • The temporary maps (prefixes, evictedTxSet) are per batch.
  • evictedTxSet is now built earlier and reused by compactDistIndex, so no extra allocation.

• No new map[string]interface{}, no DB writes in cmd/server, and no goroutines or timers added. [F]

• Interaction with fix(store): preserve first_seen ordering when merging background chunks #137 (loadChunk merges by first_seen) [F]:

  • The two PRs change different functions.
  • evictFromPathHopIndex makes no ordering assumption about s.packets or the buckets. It works on the evicted set, so fix(store): preserve first_seen ordering when merging background chunks #137's correct head-of-slice eviction feeds it directly.

Not verified

• Lock-hold time on a production-size store and relay mix (staging). [K], also listed in the PR.
• That no persisted resolved_path holds a pubkey that does not start with its hop, for example rows written by older ingestor versions or a backfill. Checked only for the current ingestor resolvePath and the server resolvePathForObs. Such a key would stay until the next rebuild. [A]
• No browser check: there is no UI change. [F]