[adminopenclaw8-sketch]

Relates to #622

Audit of upstream pull request 2284 ("fix(analytics): preserve distance invalidation during an active build", upstream issue 2281; read-only, nothing was written there).

Verdict: the upstream defect cannot occur in our fork. Our #149 already replaced the reset sync.Once with a dataset generation, which is strictly stronger than upstream's fix. This PR therefore adds tests only and changes no production code. It does close a real coverage gap that the audit found.

Plan

1. Map each of upstream 2284's four production changes onto our store and decide whether the defect it fixes exists here.
2. For every mapped site, mutate our fix and check that an existing test fails. Where nothing covers the site at runtime, add a test.
3. Report the gaps the mutants expose and close them with red-before/green-after tests.

Audit: upstream 2284 against our store

Upstream change	Our fork	Existing test	Mutant
Drop the distLazyOnce reset in the background-fill completion; a reset of an in-use sync.Once fatally unlocks an unlocked mutex	No sync.Once anywhere in the gate since #149. The fill bumps distDataGen (an atomic.Uint64) under s.mu after the last chunk merged	TestDistanceBuild149_LoadCompletionDuringBuildDoesNotCrash (runs in a child process)	— (nothing left to revert; the Once is gone)
Generation check so a stale completion cannot consume the invalidation	runDistanceIndexBuild reads distDataGen inside its own s.mu section, records it as distLazyBuiltGen, and distIndexCurrentLocked() reports the index current only while distLazyBuiltGen == distDataGen. A pass that went stale repeats instead of returning	TestDistanceBuild149_LoadAfterBuildSnapshotRebuildsOnce, TestDistanceBuild149_TriggersDuringStaleRepassStartNoBuild, TestDistanceLoadChunkedLoadCompletionDuringBuild558	M3
Build admission under the mutex (distLazyBuilding = true before the goroutine starts)	startDistanceBuildLocked sets it under distLazyMu and then starts the goroutine	TestDistanceBuild149_BuildingIsSetBeforeTheBuildGoroutineRuns, TestDistanceBuild149_ConcurrentTriggersStartOneBuild	M4
The loader releases the store lock before it takes the distance gate	Our loader never takes distLazyMu at all — the atomic generation makes it unnecessary — and the trigger reads totalObs between two distLazyMu sections. #149's documented rule: s.mu is never acquired while distLazyMu is held	TestDistLazyMuNeverHeldWhileTakingStoreMu (static AST guard), TestDistanceBuild149_TriggerAndLoadCompletionDoNotDeadlock	M5

Upstream's open question in issue 2281 ("worth fixing given the 5 minute bound?") does not apply either: our gate does not rely on the debounce at all. TriggerDistanceIndexBuild checks distIndexCurrentLocked() first and always builds a stale index; the Δobs/5-minute policy only governs a current index.

The gap the audit found

Every #149 scenario, and #558's, runs the background loader with an empty chunk loop — newDistBuildStore / the 558 helper set oldestLoaded past the retention cutoff so the loop exits on its first check. So:

• no test ever merged a transmission while an invalidation was in flight, and
• no test asserted what the rebuilt index contains — they all assert gate state and HTTP status only.

That blind spot is not theoretical. M1, which moves the generation bump from after the chunk merge to before the loop, leaves the entire cmd/server package green (one 76 s run, 0 failures). On that mutant a build whose snapshot lands part-way through the fill records the already-bumped generation, so the gate reports an index that is missing the chunks merged after the snapshot as current — exactly what upstream issue 2281 describes.

Tests added

cmd/server/distance_fill_invalidation_622_test.go, test-only, driving the production LoadChunked + loadBackgroundChunks + the real /api/analytics/distance handler.

Fixture: 60 transmissions inside the 2 h hot startup window and 2 older ones inside the 48 h retention window that only the chunk loader reaches, one observation each, over GPS-bearing nodes in the nodes table so each transmission yields a distance path record with its own content hash. dist622RequireBelowDebounce pins the premise that 2 observations on top of 60 (3.3 %) do not reach the 5 % debounce threshold, so a rebuild can only come from the generation invalidation — upstream's "two cold observations stay below the normal 5% rebuild threshold".

#	Test	What it pins	Mutant that kills it
1	TestDistanceFill622RebuildBelowTheDebounceThreshold	A fill that merges new transmissions invalidates a just-built index below the debounce threshold; 10 concurrent callers share one rebuild and all get 202 + Retry-After; the hot-window 200 lacks the cold paths and the rebuilt 200 carries them	M2
2	TestDistanceFill622StaleCompletionKeepsTheInvalidation	Upstream 2281's window: a build reads the hot dataset, the fill merges the cold chunk and bumps the generation, and only then does that build publish. Its index must not be reported current; one rebuild follows and its 200 carries the merged paths. Runs in a child process (#149 convention)	M3
3	TestDistanceFill622MidFillSnapshotIsNotCurrent	A build whose snapshot lands between a chunk being scanned and merged (loadChunkScannedHook) records the pre-fill generation, so its index is stale the moment the fill ends	M1

Mutants

Each mutant is a single edit to cmd/server/store.go; the production tree is unchanged.

Mutant	Edit	New tests	Existing tests
M1	distDataGen.Add(1) moved from after the chunk merge to before the loop	test 3 FAIL ("the index built from part-way through the background fill is reported current")	all green, including the whole package (0 other failures in 76 s) — this is the gap
M2	distIndexCurrentLocked() → s.distLazyBuilt in TriggerDistanceIndexBuild (gate on the flag, ignore the generation)	tests 1 and 3 FAIL (the rebuild never fires; the debounce suppresses it)	TestDistanceBuild149_HandlerContractAndLoadInvalidation FAIL
M3	distLazyBuiltGen = gen → distLazyBuiltGen = s.distDataGen.Load() (record the live generation, not the one this pass read)	test 2 FAIL ("the build that read the pre-fill dataset consumed the invalidation")	TestDistanceBuild149_TriggersDuringStaleRepassStartNoBuild, TestDistanceLoadChunkedLoadCompletionDuringBuild558 FAIL
M4	distLazyBuilding = true moved from startDistanceBuildLocked into the build goroutine	—	TestDistanceBuild149_BuildingIsSetBeforeTheBuildGoroutineRuns, TestDistanceBuild149_ConcurrentTriggersStartOneBuild, TestDistanceLoadChunkedFirstRequestBuildsOnce558 FAIL
M5	pre-#149 lock nesting restored: the trigger reads totalObs while distLazyMu is held and the fill touches the gate under s.mu	—	TestDistLazyMuNeverHeldWhileTakingStoreMu FAIL, TestDistanceBuild149_TriggerAndLoadCompletionDoNotDeadlock FAIL (deadlock, 10 s timeout)

M4 and M5 cover the two upstream changes that need no new test: existing tests already kill them.

Validation

• cmd/server: full package go test . green (76.9 s). New tests -count=10 -race green, and -count=3 -cpu 1 green (deterministic — no sleeps, no polling for a race).
• Every other Go module: go build ./... and go test ./... green.
• sh test-all.sh: 274 passed, 0 failed. node test-frontend-helpers.js: 731 passed, 0 failed.
• Local Go server against a freshened, migrated copy of the E2E fixture: /api/analytics/distance answered 202 + Retry-After: 5, then 200 with 1808 hops / 357 paths / 20 top paths. test-issue-1648-m4-icons-e2e.js (the one E2E that visits the distance tab) 9 passed, 0 failed. No E2E and no frontend file is affected: the diff is a single Go test file.
• scripts/check-xss-sinks.sh --diff origin/master: clean (no public/** changes). Fork guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml. No new map[string]interface{}. No SQL under PacketStore.mu, no write helper added — cmd/server stays read-only.

Performance

Test-only. No production code, no hot path, no schema, settings or API change. The three new tests add about 0.1 s to the package (1.6 s under -race).

Limits

• The mid-fill interleaving in test 3 uses the existing loadChunkScannedHook seam, which fires per chunk that has rows to merge. An empty chunk is not a park point; the test asserts that nothing was merged yet when it parks, so the ordering it needs holds either way.
• The audit covers the distance gate. distDataGen documents itself as counting only dataset changes the incremental maintenance does not cover, and today the background fill's completion is the only such writer; ingest, eviction and the hash migration all maintain distHops/distPaths directly. Nothing enforces that statically, so a future bulk mutator that forgets both would go unnoticed. Not filed.
• Node positions moving (an advert with new GPS) do not invalidate the index either; only the 5-minute debounce bounds that. Pre-existing and out of scope here.

[adminopenclaw8-sketch]

Rapport — CS-Macmini PR#636 #622 — head 140a445

Status: Audit done — upstream 2284's defect cannot occur here (our #149 fix is strictly stronger); one real coverage gap found and closed with 3 tests, 5 mutants run, no production code changed.

Requirements

Requirement	Test (red before / green after)	Mutant	Evidence
R1 — Audit upstream 2284 against our fork before porting	All four upstream production changes mapped to our store; each mapped site has an existing killing test (table in the PR body)	M4 (distLazyBuilding set inside the goroutine) kills TestDistanceBuild149_BuildingIsSetBeforeTheBuildGoroutineRuns + 2 more; M5 (pre-#149 lock nesting) kills TestDistLazyMuNeverHeldWhileTakingStoreMu + the deadlock test	[A] no sync.Once in our gate since #149; the fill bumps distDataGen under s.mu after the merge and the gate compares it in distIndexCurrentLocked(). [K] M4 → 3 FAIL, M5 → 2 FAIL (one a 10 s deadlock)
R2 — Gap 1: a fill that merges new transmissions invalidates a just-built index below the Δobs debounce threshold; callers coalesce; the ready 200 carries the merged paths	TestDistanceFill622RebuildBelowTheDebounceThreshold	M2: distIndexCurrentLocked() → s.distLazyBuilt in TriggerDistanceIndexBuild	[T] red on M2 (the rebuild never fires — the debounce suppresses it), green on head. [K] M2 also kills TestDistanceBuild149_HandlerContractAndLoadInvalidation
R3 — Gap 2: upstream 2281's window — a build that read the pre-fill dataset publishes after the fill and must not consume the invalidation	TestDistanceFill622StaleCompletionKeepsTheInvalidation (child process, #149 convention)	M3: distLazyBuiltGen = gen → = s.distDataGen.Load()	[T] red on M3 with the semantic message "the build that read the pre-fill dataset consumed the invalidation", green on head. [K] M3 also kills TestDistanceBuild149_TriggersDuringStaleRepassStartNoBuild and TestDistanceLoadChunkedLoadCompletionDuringBuild558
R4 — Gap 3 (the real blind spot): a build whose snapshot lands part-way through the fill records the pre-fill generation, so its index is stale when the fill ends	TestDistanceFill622MidFillSnapshotIsNotCurrent	M1: distDataGen.Add(1) moved from after the chunk merge to before the loop	[T] red on M1 ("the index built from part-way through the background fill is reported current"), green on head. [K] the whole cmd/server package stays green on M1 — one 76 s run, 0 other failures. That is the gap: every #149/#558 scenario runs the loader with an empty chunk loop
R5 — No production change; constraints held	— (test-only)	—	[A] diff is one new Go test file, 438 lines; git diff on tracked files empty. No SQL under PacketStore.mu, no write helper → cmd/server stays read-only; 0 new map[string]interface{}; no UI change so no colours

Fixture premise is pinned in code: dist622RequireBelowDebounce fails the test if 2 observations on top of 60 (3.3 %) ever reach the 5 % threshold, so a rebuild can only come from the generation invalidation — upstream's "two cold observations stay below the normal 5% rebuild threshold".

CI per job

Job	Result
✅ Go Build & Test (rollup gate)	pass
✅ Go Server Tests (1/3)	pass
✅ Go Server Tests (2/3)	pass
✅ Go Server Tests (3/3) — the catch-all shard that runs the three new tests	pass
✅ Go Ingestor Tests	pass
✅ Go Libraries & CLI	pass
✅ JavaScript Unit Tests & Lint	pass
✅ Script & Container Unit Checks	pass
🎭 Playwright E2E Tests (1/3)	pass
🎭 Playwright E2E Tests (2/3)	pass
🎭 Playwright E2E Tests (3/3)	pass
📊 Verify E2E Shards & Coverage	pass
📋 Verify Go Test Assignments	pass
🧱 Validate Docker Image	pass

All 14 jobs green on the first attempt, no re-run needed. The known flake #271 did not appear. (The earlier run on d416c4fc was auto-cancelled by the concurrency group when 140a4450 was pushed; the four jobs it had finished were green.)

Local validation

• cmd/server: full go test . green (76.9 s). New tests -count=10 -race green; -count=3 -cpu 1 green. Deterministic: channel handoffs only, no sleeps.
• All other Go modules: go build ./... + go test ./... green.
• sh test-all.sh: 274 passed, 0 failed. node test-frontend-helpers.js: 731 passed, 0 failed.
• node scripts/run-go-server-shard.js --validate and --runtime-list: 3009 runnable entries, each in exactly one shard; the new tests land in the catch-all shard.
• Local Go server on a freshened + migrated copy of the E2E fixture: /api/analytics/distance → 202 with Retry-After: 5, then 200 with 1808 hops / 357 paths / 20 top paths. test-issue-1648-m4-icons-e2e.js (the only E2E that visits the distance tab) 9 passed, 0 failed.
• scripts/check-xss-sinks.sh --diff origin/master: clean. Fork guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml.

Remainders (not filed)

1. distDataGen documents itself as counting dataset changes the incremental distance maintenance does not cover, and the background fill's completion is the only such writer today (ingest, eviction and the hash migration all maintain distHops/distPaths directly). Nothing enforces that statically — a future bulk mutator that forgets both would silently serve a stale index until the 5-minute debounce.
2. Node positions moving (an advert with new GPS) do not invalidate the index; only the 5-minute debounce bounds the staleness. Pre-existing, out of scope.
3. The mid-fill seam (loadChunkScannedHook) fires only for a chunk that has rows to merge, so an empty chunk is not a park point. Test 3 asserts nothing was merged yet when it parks, so the ordering holds either way.
4. test-issue-1648-m4-icons-e2e.js reports two soft warnings against the shared fixture ("route sidebar not present", "/analytics distance: no map-jump button rendered (empty dataset)"). Fixture-shape, not assertions, and unreachable from a test-only diff.

[dborup]

Review — CS-Minimax PR#636 — head 140a445

Verdict: APPROVE with nits

Independent, read-only review. Merged tree 90f3d26e = origin/master 960c910c + this PR (the PR is based on current master, so head and the merged tree are byte-identical). Evidence tags: [T] test/command I ran myself, [A] read in the code, [K] inference/judgement.

Findings

#	Severity	Finding	Evidence
1	Nit (recommend adding here)	The read site of the generation check is unpinned. Moving gen := s.distDataGen.Load() out of the build's s.mu section (e.g. to just before distLazyMu.Lock()) is exactly the lost-invalidation shape the code comment in runDistanceIndexBuild warns about, and the upstream 2281 class. No existing or new test kills it. The PR's M3 covers the record site (distLazyBuiltGen = gen), not the read site. A 25-line test closes it: hold store.analyticsRecomputerMu.Lock(), trigger a build so it parks in RLock after its s.mu section, run loadBackgroundChunks() to completion, release, and assert the ready 200 carries the cold paths. My probe of that shape is green on head (5/5) and red on the mutant (5/5: "cold path … present=false"). (cacheMu does not work as the park point: the fill takes it too.)	[T] mutant MA2 + probe; [A] store.go runDistanceIndexBuild
2	Nit (PR text only)	The PR body says M1 "leaves the entire cmd/server package green … 0 other failures". I could not reproduce that: a full-package run on M1 fails exactly 2 tests. With the bump moved before the chunk loop, TestDistanceBuild149_TriggerAndLoadCompletionDoNotDeadlock fails deterministically (5/5, 10 s): the trigger takes the stale-index start path instead of the debounce RLock path the test waits for. The kill is incidental, not semantic, so the conclusion stands that only the new test 3 pins the mid-fill snapshot. But the "0 other failures" claim is inaccurate. If the author's M1 placed the bump somewhere other than directly before the for loop, the body should say where.	[T] -count=5 on M1; [A] test lines 404–441
3	Info	A variant of finding 1 that reads gen immediately after s.mu.Unlock() (mutant MA) has no park point between the unlock and the read, so it cannot be killed deterministically without a new seam. I'm noting it only so nobody counts it as covered.	[T] MA survived; [K]

No blocking findings. The audit conclusion is correct.

Answers to the review points

• Does upstream 2284's defect exist here? No. [A] There is no sync.Once anywhere in the distance gate. The gate is distLazyMu + distLazyBuilt/distLazyBuilding/distLazyBuiltGen + the atomic distDataGen. The only sync.Once near the load path is chunkInitOnce, used via .Do only and never reassigned. The stray var _ = sync.Once{} in chunked_load.go is a compile-time reference, not a reset. So no Once can be reset while active, and upstream's fatal-unlock crash mode is impossible here.
• Can a stale completion consume an invalidation? No. [A] runDistanceIndexBuild reads gen inside the same s.mu.Lock section as buildDistanceIndex, and the fill bumps distDataGen under s.mu.Lock after the last chunk merged. A pass therefore records the generation that matches its data. distIndexCurrentLocked() requires distLazyBuiltGen == distDataGen, and a pass that sees gen != distDataGen keeps distLazyBuilding = true and loops. [T] Mutants M3, MB and ME are all killed, by the new tests and by existing tests. The read placement itself is pinned by no test (finding 1).
• Mid-fill snapshot (perf(startup): defer chunked-loader distance indexing to the lazy builder #558/perf(startup): defer chunked-loader distance indexing to the lazy builder #589 lazy + chunked). [A] The chunk merge publishes in batches under s.mu, but the generation moves only once, at fill completion, after loadReheardTransmissions and buildPathHopIndex. A build between chunks therefore records the pre-fill generation and goes stale at completion. [T] Test 3 kills M1, and the fixture really does park between scan and merge: it asserts that 0 packets were merged at the hook.
• Debounce independence. [A] TriggerDistanceIndexBuild checks distIndexCurrentLocked() before the Δobs/5-min policy, so a stale index always builds. [T] Test 1 kills M2, and dist622RequireBelowDebounce pins the below-threshold premise (2/60 = 3.3 % < 5 %).
• Lock order / SQL under PacketStore.mu. No production change, so there is nothing new to check. [A] Existing order holds: the trigger reads totalObs between two distLazyMu sections, and the loader never takes distLazyMu. The test file's SQL (db.conn.Exec INSERTs) runs only in fixture setup, before any store exists. [T] M5-class protection is the existing TestDistLazyMuNeverHeldWhileTakingStoreMu, green 50× under -race.
• Acceptance criteria → red/green. The issue's "does not apply" branch asks for tests that pin correct behaviour. Each new test is green on head, and each is red on at least one single-edit mutant of store.go (test 1 on M2/ME, test 2 on M3/MB/ME, test 3 on M1/ME). Since there is no production change, "red before" is demonstrated via mutants, not via a master run. [T]
• No behaviour change beyond the goal. [T] git diff --stat origin/master..head shows one new file, cmd/server/distance_fill_invalidation_622_test.go (+438). Nothing outside cmd/server tests changes.
• Constraints. [T] 0 new map[string]interface{} in the diff. cmd/server stays read-only: no production code, no write helper. No UI, so no colours. scripts/check-xss-sinks.sh --diff origin/master run from a checkout at head is clean (no public/** changes). Fork guards are unchanged: github.repository == 'Kpa-clawbot/CoreScope' appears 9 times in deploy.yml and 1 time in release-fast-path.yml, and .github/ is untouched. No closing keywords in title, body or commit messages; the body uses "Relates to". Both commits (d416c4fc, 140a4450) are authored and committed by dborup <kontakt@meshview.dk>.
• CI per job. [T] All required jobs on run 40646677858 are green: Go Build & Test, Go Server 1/3, 2/3, 3/3, Go Ingestor, Go Libraries & CLI, JS Unit & Lint, Script & Container, Playwright 1/3, 2/3, 3/3, Verify E2E Shards & Coverage, Verify Go Test Assignments, Validate Docker Image, Build & Publish Docker Image. Release, Badges and Deploy Staging were skipped as expected on a fork PR. Neither known flake (flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256, flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267) appeared.

Tests and mutants (all run locally on the merged tree)

Run	Result
go test -race -count=50 -run TestDistanceFill622	[T] 150/150 PASS, 0 data races (98.8 s)
go test -race -count=50 over TestDistanceBuild149*, TestDistanceLoadChunked*558, TestDistLazyMuNeverHeldWhileTakingStoreMu	[T] 800/800 PASS, 0 data races (601 s on a heavily loaded host). A first attempt with the default 10 min go test timeout timed out from host load (~60 load average), not from a hang; the rerun with -timeout 0 was green
cmd/server go test ./...	[T] ok (84.5 s), 0 failures
go vet (cmd/server)	[T] clean
sh test-all.sh	[T] 274 passed, 0 failed
node test-frontend-helpers.js	[T] 731 passed, 0 failed
Local Go server on port 13700 with e2e-fixture.db prepared as CI does (freshen → CI seed SQL → corescope-migrate → seeds 2073, 199, 245, 122)	[T] /api/analytics/distance 202 + Retry-After: 5, then 200 with 1808 hops / 357 paths / 20 top paths
test-issue-1648-m4-icons-e2e.js (the distance-tab E2E)	[T] 9 passed, 0 failed (same three fixture-shape soft warnings the author reports)
test-e2e-playwright.js (includes "Analytics Distance tab renders content")	[T] 156/159 passed, 3 skipped. The first attempt fail-fast-stopped on #1110 Live node filter applies on suggestion click (5 s waitForSelector timeout under host load). It passed on the immediate rerun. No frontend or prod code changes in this PR, so I'm treating it as an environment flake [K]

Mutants (single edits to cmd/server/store.go, run against all distance tests):

Mutant	Edit	Killed by
M1 (author's, re-run)	distDataGen.Add(1) moved to just before the chunk for loop	new test 3; also TestDistanceBuild149_TriggerAndLoadCompletionDoNotDeadlock (deterministic, 5/5, see finding 2). Full-package run on a complete tree with M1: [T] exactly these 2 failures, everything else green (93 s)
M3 (author's, re-run)	distLazyBuiltGen = s.distDataGen.Load()	new test 2, TestDistanceBuild149_TriggersDuringStaleRepassStartNoBuild, TestDistanceLoadChunkedLoadCompletionDuringBuild558
MB (own)	stale pass returns instead of re-building (if !stale → if true)	new test 2, TestDistanceBuild149_LoadAfterBuildSnapshotRebuildsOnce, …TriggersDuringStaleRepassStartNoBuild, …LoadChunkedLoadCompletionDuringBuild558
ME (own)	fill completion's distDataGen.Add(1) deleted	all three new tests + 4 existing #149/#558 tests
MA (own)	gen read right after s.mu.Unlock()	survives (no park point; finding 3)
MA2 (own)	gen read just before distLazyMu.Lock()	survives the PR's suite; killed 5/5 by the probe in finding 1

The mutant copies lacked the repo-root compose files, so TestStagingComposes_PprofHostPortsBindExplicitlyAndStayDistinct failed in them on file-not-found. That is an artifact of my scratch layout and is excluded from the tables above. The M1 full-package result is from a complete tree copy, where nothing else failed.

Not verified

• I did not read upstream PR 2284 or issue 2281 myself. I'm relying on the description of upstream's change in issue port/audit upstream PR 2284: fix(analytics): preserve distance invalidation during an active build #622 and in the PR body. The fork-side mapping I verified against our code.
• I did not run cmd/ingestor or the other Go modules: the PR touches only a cmd/server test file. I did not run the full Playwright shard set via scripts/run-e2e-shard.js, only the two E2Es that visit the distance tab. I did not run -cpu 1.
• I ran Go 1.26.0 locally (CI uses 1.27.1).
• Lock-order safety under real production traffic is argued from the code and the existing static/deadlock tests, not measured.