fix(store): charge the path_json fallback and pin the hash-migration accounting (#202)

[dborup]

Relates to #202 (and #113, #164, #198).

Draft. Both leaks from #202 are covered, but the first one is not what the issue describes, so read that part before reviewing.

1. Hash migration: the double credit is not reachable

migrateContentHashesAsync has an in-memory branch that would copy a merged duplicate's observations to the survivor while the duplicate stays in s.packets, which would credit them twice at eviction. That branch never runs: it is selected by u.newHash == "", but the marker is set on u, the copy of the element made by for _, u := range updates, so updates keeps the real hash. On master a merged duplicate stays in memory with its own observations (probe on a two-duplicate store: both transmissions keep their observation, packets=2), and each observation is credited once. This is the legacy ghost-row behaviour the existing characterization test already pins.

• Test: TestHashMigrationMerge_EvictionCreditsEveryObservationOnce_202 merges two duplicates, evicts both and asserts that trackedBytes equals a live ballast transmission exactly, so the clamp at 0 cannot hide a double credit. It also checks trackedBytes against the live charges right after the merge. It passes on master, so there is no red-before for this leak.
• Change: remove the dead branch and the dead marker. Behaviour is identical, and a comment says why copying would double-credit and that a real fix must move the observations. Making the branch live (removing the duplicate from s.packets and every index) is the separate change the existing characterization test calls for, and it changes QueryPackets cardinality, so it is out of scope here.
• Mutants (the branch made live again, a merge that drops a duplicate's charge, a merge that copies observations to the survivor): all three fail the new test.

2. The path_json fallback: real, and a bit bigger than the issue says

Without a persisted resolved_path, indexObservationRelayHops (used by Load, scanAndMergeChunk, IngestNewFromDB and IngestNewObservations) and the merge step of loadChunk (the background fill) added relays to byNode and nodeHashes without charging trackedBytes. Eviction also never removed those entries, because it only knows decoded pubkeys and the resolved_path pubkeys read from SQL. So an evicted transmission stayed pinned in byNode/nodeHashes for ever (an emptied store still held 256 nodes in the tests).

• addFallbackRelay charges each new (relay, tx) pair and records it in s.fallbackByNode, a per-transmission map like pathHopResolved. txChargedBytes adds fallbackRelayBytes(len(record)), a function of the count alone, so eviction credits exactly what was charged, and removes the entries from byNode/nodeHashes using the record.
• A relay a transmission is already indexed under (a decoded pubkey, a resolved relay) adds nothing and costs nothing.
• StoreTx is unchanged (320 bytes, layout tests unchanged). No new map[string]interface{}; cmd/server stays read-only.
• Tests (5 paths each: Load, LoadChunked, background fill, IngestNewFromDB, IngestNewObservations): entries are charged (red on master), eviction removes and credits (red on master), the charge equals the records exactly, partial eviction stays exact, and one call charges once per relay. acct113Sum, the shared accounting invariant, gains the fallback term.
• Mutants, all failing at least one test: no charge, no credit, eviction keeps the entries, double charge, record not deleted, and either of the two call sites left on addToByNode.

Performance

Interleaved master/PR runs, 5 rounds of 7 runs each (median per round), same fixture, CORESCOPE_PERF_202 tests in this PR (they skip otherwise):

Measurement	master	this PR
Load, 5000 tx, all hops via fallback	66.0–70.5 ms	68.3–74.3 ms (+3 to +5%)
Evict all 5000 tx	26.2–28.4 ms	30.8–33.1 ms (+16 to +19%)
Hash-migrate merge, 1000 duplicate pairs	129.8–131.8 ms	129.6–131.8 ms (unchanged)
trackedBytes after Load	14,666,965	18,366,965

• Load: one map lookup and one append per added relay, under the lock the code already held. No new O(n) work.
• Evict: it now also removes the fallback entries (O(relays of the evicted transmissions)), which it did not do before; that is the cost of not pinning evicted transmissions.
• Charge against heap: the marginal heap of the fallback's entries is 3.83 MB against 3.70 MB charged (97%) on the 5000-tx fixture. The per-relay and per-record constants are derived from the resolved-relay ones, not calibrated separately, so this is one data point.
• Because relays are now charged, trackedBytes rises on stores whose observations lack resolved_path (here +25% on a store where every hop resolves through the fallback). That is memory the store really held; memory-based eviction will see it.

Tests run

• New and affected tests, -race -count=3: pass.
• cd cmd/server && go test ./...: pass. go vet: clean. gofmt -l on the changed files: clean.
• sh test-all.sh: 214/214 files.

CI has not been checked here (see the report comment).

Remaining

• Making the dead merge branch live (remove the duplicate from s.packets and its indexes, re-parent its observations) is still the separate change the characterization test names.
• nodeHashes is keyed by tx.Hash, which the hash migration rewrites after indexing, so entries made under the old hash are not reached at eviction for migrated transmissions. Not touched here.

🤖 Generated with Claude Code

[dborup]

Rapport — CS-MacBook PR#209 #202 — head 4073d40

Status: Draft PR open, local tests green, CI pending (one check queued); leak 2 fixed, leak 1 does not exist as described (the double credit is unreachable), so there is no red-before test for it.

Evidence: [T] = ran it, [A] = read in source or diff, [K] = known from earlier work or another report, not re-verified here.

Base c91a793f. Master has since moved to 6bdaa8f7 (one unrelated ingestor test file); git merge-tree against it is clean [T]. No rebase, amend or force-push. Three commits: tests (715d5d66), fix (7540a02d), env-gated perf tests (4073d408).

Leak 1: hash-migration double credit — not reachable on master

• The merge branch that would copy a duplicate's observations to the survivor is selected by u.newHash == "". The marker is written on u, the loop copy from for _, u := range updates (updates is []hashUpdate, not pointers), so updates keeps the real hash and the branch never runs [A]. A probe on a two-duplicate store confirmed it: after the migration both transmissions kept their own observation, packets=2, byTxID=2 [T]. That is the legacy ghost-row behaviour the existing characterization test already pins.
• So each observation is credited once on master. The fix(store): trackedBytes leaks left after #198 (hash-migration double credit, uncharged path_json fallback) #202 scenario cannot be made red: TestHashMigrationMerge_EvictionCreditsEveryObservationOnce_202 (merge two duplicates, evict both, assert trackedBytes equals a live ballast transmission exactly, so the clamp at 0 cannot hide it) passes on master [T]. It also asserts trackedBytes against the live charges right after the merge.
• Change: remove the dead branch and the dead marker, with a comment explaining that copying would double-credit and a real fix must move the observations [A]. Behaviour is identical (existing migration and characterization tests pass) [T].
• Not done, deliberately: making the branch live (remove the duplicate from s.packets and every index). That changes QueryPackets cardinality and is the separate change the existing characterization test names. Say if you want it.
• Mutants (each fails the new test) [T]: branch made live again (also fails 3 existing migration tests); merge drops a duplicate's charge; merge copies observations to the survivor.

Leak 2: path_json fallback — real, and larger than described

• Fallback relays went into byNode and nodeHashes uncharged, at two sites: indexObservationRelayHops (used by Load, scanAndMergeChunk, IngestNewFromDB, IngestNewObservations) and the merge step of loadChunk [A].
• Found while testing: eviction never removed those entries either (it only knows decoded pubkeys and resolved_path pubkeys read from SQL). An emptied store still held 256 nodes in byNode and nodeHashes, so evicted transmissions stayed pinned [T, red on master].
• Fix: addFallbackRelay charges each new (relay, tx) pair and records it in s.fallbackByNode (per-transmission, like pathHopResolved). txChargedBytes adds fallbackRelayBytes(len(record)), a function of the count alone, so eviction credits exactly the charge and removes the entries from byNode/nodeHashes using the record. A relay the transmission is already indexed under adds nothing [A].
• StoreTx unchanged: 320 bytes, both layout tests pass unchanged [T]. No new map[string]interface{}. cmd/server stays read-only; the read-only invariant test passes [T].
• Tests: for each of 5 entry paths (Load, LoadChunked, background fill, IngestNewFromDB, IngestNewObservations): entries are charged [T, red on master, 5/5], eviction removes and credits [T, red on master, 5/5], the charge equals the records exactly, partial eviction stays exact, and one call charges once per relay. acct113Sum, the shared accounting invariant, gains the fallback term.
• Mutants, 8, each fails at least one test [T]: no charge; no credit; eviction keeps the entries; double charge; record not deleted; loadChunk site left uncharged; ingest site left uncharged. (The first version of "no charge" did not compile; it was rewritten and re-run.)

Tests run [T]

• New and affected tests, go test -race -count=3: pass (after the final commit).
• cd cmd/server && go test ./...: pass, once, after the last change.
• go vet: clean. gofmt -l on the changed files: clean (many other files in the package are listed by gofmt -l on master; I did not touch them).
• sh test-all.sh from the worktree root: 214/214 files.

Benchmark [T]

Interleaved master and PR binaries, 5 rounds of 7 runs each (median per round), same fixture. The tests are in the PR (CORESCOPE_PERF_202=<runs>; skipped otherwise) and use only functions that exist on master.

Measurement	master	PR
Load, 5000 tx, all hops via fallback	66.0–70.5 ms	68.3–74.3 ms (+3 to +5%)
Evict all 5000 tx	26.2–28.4 ms	30.8–33.1 ms (+16 to +19%)
Hash-migrate merge, 1000 duplicate pairs	129.8–131.8 ms	129.6–131.8 ms (unchanged)
trackedBytes after Load	14,666,965	18,366,965

• The eviction increase is the new work of removing the fallback entries, which master skipped; no new O(n) work on a hot path.
• Charge against heap: marginal heap of the fallback entries 3.83 MB against 3.70 MB charged (97%), 3 runs [T]. The constants are derived from the resolved-relay ones, not calibrated on their own, so this is one data point.
• trackedBytes rises on stores whose observations lack resolved_path (+25% here, where every hop resolves through the fallback). That memory was really held, but memory-based eviction will now see it.

CI

Pending: the app's PR monitor shows 1 check queued, 0 passing, 0 failing, PR mergeable, review required [T]. I did not poll it. Nothing in CI has been seen yet.

Remaining

• The dead-branch removal does not fix the ghost duplicate rows in s.packets after a hash migration (existing, characterized).
• nodeHashes is keyed by tx.Hash, which the hash migration rewrites after indexing, so entries made under the old hash are not reached at eviction for migrated transmissions. Not touched here [A].
• Not verified: behaviour on a real production-size store, and the per-relay and per-record constants beyond the one heap comparison above.

[adminopenclaw8-sketch]

Review — CS-Macmini PR#209 trackedBytes-leaks — head 4073d40

Dom: APPROVE med nits

Both claims hold. Leak 1 cannot be reached, and removing the dead branch changes no behaviour. Leak 2 is fixed correctly on all five entry paths, with no double removal and no references left behind after eviction. The nits are an unpinned cost constant and a separate pre-existing hash-migration issue, which I propose to file as its own issue.

Evidence: [T] = ran it, [A] = read in source or diff, [K] = taken from the author's report or earlier work, not re-verified here.

This review was read-only. git ls-remote showed head 4073d408 before and after the review. I reviewed it against origin/master 8bafcdf2. The PR is based on c91a793f; master has moved since, but only in ingestor tests and frontend files. git merge-tree is clean, and the merged tree is 3099766a [T]. All work ran on git archive copies in scratch. Nothing was pushed or changed.

Findings

#	Severity	Finding
1	Nit	The per-relay constant perFallbackRelayBytes is not pinned by any test that runs by default. Mutant M4 (80 → 0) survives every _202/_113 test, because the "charged" test only checks with > without, and the 140-byte record still makes that true. The heap comparison that would catch it is env-gated (CORESCOPE_PERF_202) [T]. A cheap ungated bound would close this, e.g. charge per fallback relay within a band of the measured marginal heap, or the derivation asserted against the resolved constants.
2	Info, separate issue proposed	The hash migration writes from cmd/server on the server's mode=ro handle. Probe P4: OpenDB (mode=ro), two stale-hash duplicates, migrateContentHashesAsync [T]: every UPDATE fails, and each failure is logged as collides — merging duplicate. The DB is unchanged (rows=2, old hash kept), so the migration repeats at every start. In memory both transmissions take the new hash, so they become ghost duplicates sharing one hash, with byHash pointing to only one of them [T]. This exists on master and is not introduced here. Details under point 1.
3	Info	nodeHashes entries made before a hash migration are never removed. Probe P3 (merged tree): fallback-indexed duplicates, migration, then eviction of both [T]: byNode is empty (no transmission stays referenced), trackedBytes=0 and fallbackByNode is empty, but nodeHashes still holds the two relay keys under the old hashes. The author lists this under "Remaining". It applies to every byNode source, not just the fallback, and it is bounded by migrated transmissions × relays.
4	Info	A relay indexed first by the fallback and later by a persisted resolved_path is charged under both allowances. Probe P2, fallback first: record [a2] plus pathHopResolved [T]. There is one byNode slot. Eviction credits both charges exactly, so there is no drift (the check is exact against the ballast). The overcharge is small and depends on order. The reverse order charges once.
5	Info	The migration leaves chargedBytes stale by the hash length difference (master probe: 890 charged against an estimate of 894). It is credited consistently, so there is no drift. It exists on master [T].

No blocking defects found.

1. Leak 1: is the merge branch dead?

• Yes [A]. updates is []hashUpdate (values). u.newHash = "" is written to the loop copy in for _, u := range updates, so updates[i].newHash keeps the computed hash. ComputeContentHash returns "" only for an empty rawHex, and the batch scan skips tx.RawHex == "". So the in-memory if u.newHash == "" can never be true, neither through the marker nor through a real empty hash.
• Probe P1, identical on master and on the merged tree [T]:
  • The DB merge happens (1 row left).
  • In memory packets=2, byTxID[1] and byTxID[2] are both present, and the observations are not moved.
  • Both transmissions carry the new hash, and byHash points to tx 2.
  • Had the branch run, byTxID[2] would be gone.
• Removing the branch changes no behaviour: the else arm the PR keeps is exactly what always ran [A]. Every existing migration test passes in the -race run [T].
• TestHashMigrationMerge_EvictionCreditsEveryObservationOnce_202 passes on master [T], as the author says. It is a guard test, not a red-before test.
• Are ghost duplicates a separate problem? Yes. I propose an issue like "hash_migrate: server-side DB writes on the mode=ro handle; failures reported as collisions; ghost duplicate rows share a hash":
  • It breaks the read/write invariant (bug(db): vacuumOnStartup fails with SQLITE_BUSY when ingestor + server share DB (auto_vacuum migration #919 broken in single-container topology) Kpa-clawbot/CoreScope#1283): cmd/server/hash_migrate.go issues UPDATE/DELETE, and readonly_invariant_test.go only checks three method names [A].
  • On the production handle the writes fail and are logged as collisions, so the log is misleading and the work repeats at every start (P4) [T].
  • Distinct transmissions that end up with the same content hash stay as two rows in s.packets, so QueryPackets shows both [T, P1]. nodeHashes is keyed by hash, so evicting one ghost removes the shared nodeHashes[pk][hash] entry that the live one still relies on. That one is inferred from the code and not probed [A].
  • Stale nodeHashes keys (finding 3) and stale chargedBytes (finding 5) [T].
  • Fix direction: move the DB part to the ingestor, as all writes should be; let the server only rehash in memory and merge in memory, removing the duplicate and moving its observations. That is the scope the characterization test already names. This PR is right not to do it.

2. Leak 2: correctness on the five entry paths

• Call sites. indexObservationRelayHops is the only fallback site used by Load (store.go:1068), scanAndMergeChunk (chunked_load.go:659, used by LoadChunked), IngestNewFromDB (store.go:3211) and IngestNewObservations (store.go:3592). The background fill uses loadChunk's merge (store.go:1554). Both sites now call addFallbackRelay [A].
• Red before: with the PR's two test files compiled against master, both TestFallbackRelays_AreCharged_202 and TestFallbackRelays_EvictionRemovesAndCredits_202 fail on all 5 subtests [T]:
  • "trackedBytes = X with the fallback's byNode entries, X without them";
  • "an emptied store still indexes 256 nodes in byNode and 256 in nodeHashes", 119 and 127 respectively for the two ingest paths.
• Rebuild. byNode/nodeHashes are never rebuilt. They are only appended to and filtered at eviction. retainResolvedPathHops rebuilds byPathHop and does not touch byNode, so fallbackByNode cannot drift from a rebuild [A].
• Late observation. P2 covers both orders: a fallback row then a persisted resolved_path row for the same relay, and the reverse. A decoded srcPubKey that is also a hop is included [T]:
  • the decoded relay is never recorded, because indexByNodeKey reports added=false;
  • fallback-first records only the new relay;
  • resolved-first records nothing;
  • acct113Check passes after the late observation.
• Eviction: candidate selection.
  • Memory-based cutoff uses txChargedBytes, which now includes the fallback term, so the selection matches the credit [A].
  • The rpBatch SQL prefetch covers only phase-1 candidates. Fallback cleanup does not depend on it, because it reads the in-memory record, so a candidate set that grows between the phases still cleans fallback relays [A].
• Eviction: removal.
  • The fallback loop skips any pk already handled by the decoded or resolved_path loops (evictedFromNode), so nodeHashes is not deleted twice [A].
  • The byNode filter is per affected key and removes the evicted IDs idempotently.
  • After eviction in P2, byNode, nodeHashes and fallbackByNode hold only the ballast, and trackedBytes equals the ballast's charge exactly, with no clamp involved [T].
  • After feat(ingestor): resolve the last hop from the observer (#188) #190, backfilled rows put the same relay into rpBatch, and the fallback loop skips it. Removal still happens exactly once [A].
• Dangling references: none after eviction, on any path above [T]. The only leftover is the hash-migration case in finding 3, which holds strings only and no transmission pointers.
• Background fill: addFallbackRelay charges s.trackedBytes before the batch is published into s.packets, the same pattern as addResolvedPubkeysToPathHopIndex (store.go ~4826) [A]. The overstatement is transient and goes away at the final publish. Not an issue.

3. Interaction with #182 and #190

• After feat(ingestor): resolve the last hop from the observer (#188) #190, the fallback is reached less often [A]. The ingestor persists resolved_path at insert (resolved_path silently stopped being written since #1289 (~2026-05-21) — all v3.8.x affected Kpa-clawbot/CoreScope#1547), and backfills NULL rows once the prefix index and the neighbour graph are ready. Rows that still do not resolve stay NULL for good.
• What still reaches it:
  • rows from the start-up window that the server polls before the backfill runs, through live ingest (fix(store): index live observations from the persisted resolved_path, as Load does (#158) #182);
  • backfill disabled (resolvedPathBackfill.disabled);
  • backfill not yet started (graph not ready);
  • rows the backfill could not resolve that still have a unique prefix in the server's own prefix map.
• Synthetic tests vs reality. In test-fixtures/e2e-fixture.db, 101 of 500 observations have a NULL resolved_path, and all 101 have an empty path_json, so the fixture never reaches the fallback [T]. The PR's tests use synthetic all-NULL stores. That state is reachable, but it is the worst case, as is the perf fixture (every hop through the fallback). The tests are right on correctness. The perf and trackedBytes growth numbers are upper bounds.
• Bonus: after feat(ingestor): resolve the last hop from the observer (#188) #190, master's leak partly heals itself, because backfilled pubkeys arrive through rpBatch at eviction. Relays the backfill never resolved stayed pinned on master. With this PR they no longer do [A].

4. Performance (rule 0)

Interleaved run on this machine: master and merged-tree test binaries, the PR's TestPerf_FallbackLoadAndEvict_202, 4 rounds alternating master and PR, 5 runs per round (median per round) [T].

Round	Load master	Load PR	Evict master	Evict PR
1	71.2 ms	71.9 ms	29.9 ms	34.1 ms
2	72.1 ms	73.0 ms	29.2 ms	35.4 ms
3	72.6 ms	73.9 ms	29.8 ms	34.9 ms
4	71.9 ms	74.0 ms	28.9 ms	35.7 ms

• Load +1 to +3%, Evict +14 to +23%. trackedBytes is 14,666,965 on master and 18,366,965 with the PR. This matches the author's +3–5% and +16–19% [T].
• Heap. TestPerf_FallbackChargeVsHeap_202, 3 runs each [T]:
  • with the PR: marginal heap 3.83 MB, charged 3.70 MB (97%);
  • on master: marginal heap 2.68 MB, charged 0. Master's figure is lower because it has no record.
• Is +14–23% on Evict acceptable? Yes.
  • It is about +5–6 ms per 5000 evicted transmissions, in a store where every hop goes through the fallback.
  • The baseline is cheap only because it skips cleanup that it needs, so the comparison is against a leaking baseline.
  • On stores where resolved_path is persisted, the added cost is one map lookup and delete per evicted transmission [A].
• Complexity [A]:
  • Load: one indexByNodeKey lookup, plus one append and one charge per new (relay, tx) pair, under the lock already held.
  • Eviction: O(fallback relays of the evicted transmissions).
  • No nested scans, so no O(n²).
• Growth. fallbackByNode has at most one entry per live transmission and one slice element per distinct relay. Entries are deleted at eviction, so it is bounded by the live store [A, T via P2 and the partial-eviction test].

5. Tests, mutants

• cd cmd/server && go test -race -count=1 ./... on the merged tree: ok, 411.7 s, exit 0. One run [T].

• StoreTx [T]:

  • TestStoreTxLayoutFitsObservedPathHashSizeMaskInPadding passes, asserting unsafe.Sizeof(StoreTx{}) == 320;
  • TestStoreTxLayoutFitsRouteMaskInPadding passes;
  • the PR adds no field to StoreTx [A].

• Own mutants in store.go, each run against the _202, _113 and P2 tests [T]:

  Mutant	Result
  M1: fallback eviction loop does not add affectedNodes (byNode keeps the evicted tx)	killed (EvictionRemovesAndCredits, PartialEvictionStaysExact)
  M2: fallback eviction loop does not delete nodeHashes	killed (same two)
  M3: charges the full fallbackRelayBytes(before+1) instead of the delta	killed (4 tests)
  M4: perFallbackRelayBytes = 0	survived (finding 1)
  M5: record deleted before the fallback eviction loop	killed (2 tests)
  M6: charge even when indexByNodeKey reports already indexed	killed (FallbackChargesOncePerRelay)

  I restored the original and checked it byte for byte with cmp [T].

• Probes P1–P4 are reviewer-only test files in scratch, not committed. Results are quoted above.

6. Rules

• cmd/server read-only: the PR adds no DB write [A]. It only removes code from hash_migrate.go. The writes that file already does exist on master (finding 2).
• No new map[string]interface{}: 0 added lines [A]. fallbackByNode is map[*StoreTx][]string.
• Fork guards: deploy.yml 9 and release-fast-path.yml 1 on the merged tree [T].
• No closing keywords in the PR body or the three commit messages [T]. Commit author is dborup <kontakt@meshview.dk> [T].

CI

Run 37189322977, head 4073d408, conclusion success [T]:

• Go Build & Test: success.
• Playwright E2E Tests: success.
• Build & Publish Docker Image: success.
• Release Artifacts, Deploy Staging and Publish Badges & Summary were skipped, as expected for a PR from a fork.

CI ran the PR head, not the merged tree. The merged tree was covered locally by the -race run above.

Not verified

• Behaviour on a production-size store, and the real share of observations that go through the fallback on a live deployment after feat(ingestor): resolve the last hop from the observer (#188) #190.
• The ghost-duplicate nodeHashes interaction in finding 2 (evicting one ghost drops the shared entry) is inferred from the code and was not probed.
• -race on the PR head on its own (I ran it on the merged tree only, as instructed).
• The constants beyond this one heap comparison.