[dborup-agent]

Relates to #558

Problem

LoadChunked (the production startup loader) called s.buildDistanceIndex(nodeSnapshot) inside its post-load s.mu.Lock() section. That build never set the lazy builder's state (distLazyBuilt / distLazyBuiltGen), so DistanceIndexBuilt() stayed false. The first /api/analytics/distance request answered 202 and started the lazy build anyway, which built the same index a second time. Legacy Load() already left this build out (Kpa-clawbot#1011). TestDistanceIndexNotBuiltOnLoad only covered Load().

Plan / change

1. Tests first (commits 47abf286, b871c486). They reproduce the double build through the real LoadChunked path and guard the startup indexes plus fix(store): lazy distance build can crash (sync.Once reset during Do) and deadlock (lock order) #149's generation gate on that path.
2. Fix (b7a0eba0): drop the eager buildDistanceIndex call from LoadChunked. The node snapshot taken before the chunk loop now only supplies the relay prefix map. Nothing else changes:
   • buildPathHopIndex() stays in the post-load section, and markIndexesReadySync() still runs after the unlock. Startup dependants such as StartRepeaterEnrichmentRecomputer → WaitIndexesReady see the index ready immediately.
   • The lazy gate (TriggerDistanceIndexBuild / runDistanceIndexBuild) is untouched. Its fix(store): lazy distance build can crash (sync.Once reset during Do) and deadlock (lock order) #149 lock order (s.mu never taken under distLazyMu), the generation check and the cache reset plus RecomputeNow before the index is marked built all stay as they are.
   • No new config, no schema change, no server DB writes, no new map[string]interface{} outside tests.

Tests

Test	Covers	Before fix	After fix
TestDistanceIndexNotBuiltOnLoadChunked	LoadChunked variant of TestDistanceIndexNotBuiltOnLoad: index empty, not built, no lazy build started by startup; a later trigger does produce records	red (3 hops, 3 paths)	green
TestDistanceLoadChunkedFirstRequestBuildsOnce558	Recomputers running; 16 concurrent first requests get 202, a request during the held build gets 202, then 200 deep-equal to the old eager-build analytics; exactly 1 build, which starts on an empty index; later requests do not build	red (started on an index that already held 6 records)	green
TestLoadChunkedPathHopIndexReadyAtStartup558	PathHopIndexReady, WaitIndexesReady(0) and every raw path-hop key indexed after LoadChunked	green (guard)	green
TestDistanceLoadChunkedLoadCompletionDuringBuild558	Background-load completion after the build read the LoadChunked dataset: the old generation is not reported built, 202 during the rebuild, 200 after, 2 passes (channels and parked-goroutine checks, no sleeps)	green (guard)	green
TestDistanceLazyChunked558_Measure	opt-in measurement (CORESCOPE_558_MEASURE=1)	—	—

Mutants, all killed: eager build restored; lazy build kicked at the end of LoadChunked; buildPathHopIndex dropped; markIndexesReadySync dropped; in-flight guard dropped from TriggerDistanceIndexBuild; RecomputeNow dropped after the build; stale pass not rebuilt; built generation read at bookkeeping.

Perf justification

Hot path: the startup load, under s.mu. The change removes an O(transmissions × hops) build from the exclusive section. That work now runs once, through the existing lazy builder, on the first distance request. It is deferred, not removed.

Same synthetic DB for both sides (#534's large-instance generator: 150 000 tx, 2 435 578 obs, 2 000 nodes spread over about 1°, giving 303 111 hop / 148 672 path records). i7-1260P, GOMAXPROCS=4, go1.27.1, 3 interleaved runs each:

	before (median, runs)	after (median, runs)
distance builds until the first request is ready	2 (1 at startup + 1 lazy)	1 (lazy)
s.mu post-load hold seen by a reader	1.66 s (1.48 / 1.66 / 2.66)	0.38 s (0.36 / 0.38 / 0.49)
LoadChunked wall time	37.6 s (37.1 / 38.4 / 37.6)	35.2 s (33.2 / 35.2 / 38.5)
longest s.mu hold during load (chunk merge)	2.50 s	2.32 s
first request: trigger → index built	1.04 s	1.10 s

Trade-off: the first distance request pays the cold build (about 1 s at this size). It already did before, because the startup-built index was never marked built. Wall-time differences are within run-to-run noise. The clear wins are one fewer build and roughly 1.3 s less exclusive s.mu hold at the end of startup. The longest hold overall (the chunk merge) is unchanged by this PR.

Reproduce (the measurement file also compiles on the test-only commit, which gives the "before" numbers):

cd cmd/server
CORESCOPE_558_MEASURE=1 CORESCOPE_558_DB=/tmp/m558.db go test -run DistanceLazyChunked558_Measure -v -timeout 60m

Validation (local)

• cd cmd/server && go vet ./... && go test ./...: ok. The full suite with -race -timeout 60m: ok (no races). The four new tests with -race -count=30, together with TestDistanceIndexNotBuiltOnLoad, TestNoSQLUnderStoreLock and TestNoSQLUnderStoreLockDetects: ok.
• cd cmd/ingestor && go vet ./... && go test ./...: ok.
• sh test-all.sh: 270/270 files. node test-frontend-helpers.js: 725 passed.
• Local Go server on a copy of e2e-fixture.db prepared as in CI (freshen, seed rows, corescope-migrate, seed files): startup logs the path-hop build and no distance build. /api/analytics/distance returns 202 once, then 200 with data, and one Built distance index line in total. test-e2e-playwright.js against it: 154/157 passed. The 3 skips are built into the suite (two marked flaky, one needs GO_BASE_URL), and "Analytics Distance tab renders content" passes.
• scripts/check-xss-sinks.sh --diff origin/master: no frontend changes to scan. No .github changes.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent1 PR#589 #558 — head b7a0eba

Status: Draft. The fix and tests are complete, every local suite is green, and all 15 CI jobs that ran passed on the first run (no rerun); not merged and not marked ready.

Evidence: [T] automated test result · [A] analysis / code reading · [K] command run or measurement on this machine or in CI.

Requirement 1: no eager buildDistanceIndex in LoadChunked; the lazy builder keeps ready/generation consistent

Check	Test	Before fix	After fix	Mutant (killed)
Index empty, not built and not building after the real LoadChunked; no lazy build started by startup; a later trigger yields records (not vacuous)	TestDistanceIndexNotBuiltOnLoadChunked	red: 3 hops, 3 paths [T]	green [T]	M1 eager call restored → red; M2 TriggerDistanceIndexBuild() at the end of LoadChunked → red (DistanceIndexBuilding() = true) [T]

The fix (b7a0eba0) removes the call. The node snapshot now only supplies relayPM [A]. The lazy gate code is untouched [A].

Requirement 2: keep path-hop/subpath indexes and the readiness that startup and repeater enrichment need

Check	Test	Before fix	After fix	Mutant (killed)
PathHopIndexReady, WaitIndexesReady(0) and every raw path-hop key indexed after LoadChunked	TestLoadChunkedPathHopIndexReadyAtStartup558	green (regression guard; the behaviour was already correct) [T]	green [T]	M3 buildPathHopIndex dropped → red; M4 markIndexesReadySync dropped → red [T]

M3 first survived a weaker "byPathHop non-empty" check, because the chunk merge already adds resolved-pubkey keys. The test was strengthened in b871c486 [T][A].

Requirement 3: the first distance request builds exactly once and returns the same result as before

Check	Test	Before fix	After fix	Mutant (killed)
Recomputers running; 16 concurrent first requests → 202; request during the held build → 202 + Retry-After: 5; then 200 deep-equal to the old eager-build analytics; exactly 1 build, which starts on an empty index; later requests → 200 with no further build	TestDistanceLoadChunkedFirstRequestBuildsOnce558	red: the lazy build started on an index that already held 6 records [T]	green [T]	M1 eager call → red; M5 in-flight guard dropped → red (lazy builds = 17); M6 RecomputeNow dropped → red (200 differs from the eager result, a stale snapshot) [T]

Requirement 4: do not regress #149's race, generation and lock-order fixes

Check	Test	Before fix	After fix	Mutant (killed)
Background-load completion after the build read the LoadChunked dataset: the old generation is not reported built, the handler answers 202 during the rebuild and 200 after, 2 passes. Channels and a parked-goroutine check, no sleeps, run in a child process	TestDistanceLoadChunkedLoadCompletionDuringBuild558	green (guard) [T]	green [T]	M7 stale := false → red (timeout waiting for the rebuild); M8 built generation read at bookkeeping → red (DistanceIndexBuilt() = true for the stale index) [T]
Existing #149 / Kpa-clawbot#1011 suites (distance_lazy_build_149_test.go, distance_lazy_index_test.go, lock-order guard and contention tests)	full server suite	—	green, also with -race [T][K]	—
Lock order: no new s.mu / distLazyMu nesting; the diff only deletes a call inside an existing s.mu section	—	—	[A]	—

Requirement 5: -race, TestNoSQLUnderStoreLock

• The 4 new tests plus TestDistanceIndexNotBuiltOnLoad, TestNoSQLUnderStoreLock and TestNoSQLUnderStoreLockDetects with -race -count=30: ok [K].
• The full cmd/server suite with -race -timeout 60m: ok in 1229 s, no data races [K]. A first run under Go's default 10 m timeout panicked on the timeout (a wall-clock limit, not a test failure). CI uses 3 shards at 20 m each [K][A].
• In CI, the new tests ran and passed in Go Server Tests (3/3), which runs with -race. The measurement test was skipped there as intended [K].

Requirement 6: measure startup time and s.mu hold time in LoadChunked, before and after

The TestDistanceLazyChunked558_Measure harness (opt-in) used the same DB for both sides: #534's large-instance generator with 150 000 tx, 2 435 578 obs and 2 000 nodes (spread over about 1°), giving 303 111 hop and 148 672 path records. The machine was an i7-1260P with GOMAXPROCS=4 and go1.27.1. There were 3 interleaved runs per side, "before" being the test-only commit b871c486 [K].

Metric	Before (median; runs)	After (median; runs)
Distance builds until the first request is ready	2 (1 startup + 1 lazy)	1 (lazy)
s.mu post-load hold seen by a reader	1.66 s (1.48 / 1.66 / 2.66)	0.38 s (0.36 / 0.38 / 0.49)
LoadChunked wall time	37.6 s (37.1 / 38.4 / 37.6)	35.2 s (33.2 / 35.2 / 38.5)
Longest s.mu hold during the load (chunk merge)	2.50 s	2.32 s
First request: trigger → index built	1.04 s	1.10 s

Trade-off: the work is deferred, not removed. The first distance request pays about 1 s for the cold build, as it already did before, because the startup-built index was never marked built. Wall-time differences are within noise [K][A].

Standing constraints

Constraint	Result
cmd/server stays read-only	No DB access added; the diff only removes an in-memory build call [A]
No new map[string]interface{} outside tests	0 added lines outside _test.go [K]
No hardcoded colours	No frontend or CSS changes [K]
scripts/check-xss-sinks.sh --diff origin/master	Clean: no public/ changes to scan [K]
Fork guards	deploy.yml: 9 matches; release-fast-path.yml: 1; no .github diff [K]
Commits	3 commits, explicitly added files, no rebase, amend or force-push [K]

Local runs

Suite	Result
cmd/server: go vet ./... + go test ./...	ok [K]
cmd/ingestor: go vet ./... + go test ./...	ok [K]
sh test-all.sh	270/270 files [K]
node test-frontend-helpers.js	725 passed [K]
Local Go server on a copy of e2e-fixture.db prepared as in CI (freshen, seed rows, corescope-migrate, seed files); stopped by pid, port then free	Startup logs no distance build; /api/analytics/distance returns 202 once, then 200 with data; 1 Built distance index line in total [K]
test-e2e-playwright.js against it	154/157 passed, including "Analytics Distance tab renders content". The 3 skips are built into the suite (2 marked flaky, 1 needs GO_BASE_URL) [K]

CI per job (run 40173799303, head b7a0eba)

Job	Result
Go Build & Test	pass
Go Ingestor Tests	pass
Go Libraries & CLI	pass
Go Server Tests (1/3)	pass
Go Server Tests (2/3)	pass
Go Server Tests (3/3)	pass (ran the new tests)
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
Validate Docker Image	pass
Verify E2E Shards & Coverage	pass
Verify Go Test Assignments	pass
Build & Publish Docker Image	pass (fork-guarded steps)
Publish Badges & Summary, Release Artifacts, Deploy Staging	skipped (fork guards)

The known flaky test #271 did not fail, so there was no rerun [K].

Remaining items

• The local E2E used the plain public/ directory, not the instrumented copy CI uses (coverage collection only). CI covers the instrumented run [K].
• The measurement was synthetic data on one laptop-class machine; no staging or production measurement was made, by design [A].
• The longest s.mu hold at startup is still the chunk merge (about 2.3–2.5 s at this size). That is out of scope for perf(startup): defer chunked-loader distance indexing to the lazy builder #558 [K][A].

[dborup-agent]

Review — CS-pve-agent3 PR#589 — head b7a0eba

Dom: APPROVE with nits

Independent, read-only review. Evidence: [T] automated test result · [A] analysis / code reading · [K] command run or measurement by me (my own scratch trees, not the author's numbers).

Trees used: git archive of the head, the test-only commit b871c486 ("before"), and the merged tree git merge-tree --write-tree origin/master b7a0eba0… against origin/master = bb91292a (merge-base 2e6fd9b0, merges cleanly). git ls-remote showed the head at b7a0eba0 both before and after the review [K].

Findings

#	Severity	Finding	Evidence
1	Nit (pre-existing, out of scope)	A mutant that reads distDataGen right after s.mu.Unlock() in runDistanceIndexBuild (the "lost invalidation" ordering the code comment warns about) survives the new tests and the existing Distance|149 suites. The new #149-on-LoadChunked test parks pass 1 at its distLazyMu bookkeeping, which is after both the correct read and the mutated one, so it cannot tell the two apart. The lazy-builder code is untouched by this PR, so this is a gap in the #149 guards, not a regression. It may be worth a follow-up test that completes the load between the build's Unlock and the generation read	[K][A]
2	Nit (measurement harness)	TestDistanceLazyChunked558_Measure labels the last reader wait over 20 ms as the "post-load" hold. That is only the post-load section when the section itself exceeds 20 ms. At smaller scales, after the fix, it could report the last chunk-merge wait instead. It holds in both the author's runs and mine (about 100 ms after the fix), but the label is a heuristic	[A][K]
3	Info	The measurement file runs UPDATE nodes SET lat/lon on its own generated scratch DB, in test code that is opt-in and skipped in CI. It is not a server-side write path, so the Kpa-clawbot#1283 invariant is unaffected	[A]

No blocking findings.

The points to verify

Eager distance index removed from LoadChunked. The only production change deletes s.buildDistanceIndex(nodeSnapshot) from the post-load s.mu section and reduces the snapshot to relayPM := s.snapshotPathNodes().pm. The only remaining production caller of buildDistanceIndex is runDistanceIndexBuild (store.go:5580). The legacy Load() path already had it commented out [A]. On the merged tree, a local server on the CI-prepared fixture logs the path-hop build and LoadChunked at startup, with no Built distance index line [K].

Path-hop/subpath readiness intact. s.buildPathHopIndex() stays in the post-load section, and markIndexesReadySync() still runs after the unlock. TestLoadChunkedPathHopIndexReadyAtStartup558 checks PathHopIndexReady, WaitIndexesReady(0) and every raw path-hop key [T]. No startup caller (main.go) depends on the distance index. StartAnalyticsRecomputers only computes on the empty index, and the handler gates on DistanceIndexBuilt() [A].

The first distance request builds exactly once.

• TestDistanceLoadChunkedFirstRequestBuildsOnce558: 16 concurrent first requests return 202. A request during the held build returns 202 with Retry-After: 5. Then 200, deep-equal to the old eager-build result. The test asserts 1 build, starting on an empty index [T].
• Live, cold server: 202 200 200 200 200 200, and one Built distance index line [K].
• Over a cold full test-e2e-playwright.js run, there is again exactly one build. Over the cold icons E2E (run twice) plus the a11y suite (4 distance cells per pass), also one build, so no redundant rebuild [K].

#149 race/generation/lock-order work holds. TriggerDistanceIndexBuild, startDistanceBuildLocked, runDistanceIndexBuild and distIndexCurrentLocked are unchanged in the diff. The diff adds no new lock nesting; it only deletes a call inside an existing s.mu section [A]. TestDistanceLoadChunkedLoadCompletionDuringBuild558 covers the generation gate on the LoadChunked path (my mutant R3 below), and the existing #149 suite catches a lock-order inversion (mutant R5) [T].

s.mu held for a shorter time in LoadChunked (my measurement). I used the PR's harness with CORESCOPE_558_MEASURE=50000,1000,800000 on one shared DB (50 000 tx, 777 534 obs, 1 000 nodes; 100 876 hop / 49 540 path records). The setup was GOMAXPROCS=4, linux/amd64, go1.27.1, with 3 interleaved runs per side, "before" being b871c486 and "after" being b7a0eba0 [K]:

Metric	Before (median; runs)	After (median; runs)
Distance builds until the first request is ready	2 (1 startup + 1 lazy)	1 (lazy)
s.mu post-load hold seen by a reader	422 ms (403 / 444 / 422)	103 ms (103 / 110 / 101)
LoadChunked wall time	9.34 s (9.15 / 9.46 / 9.34)	9.00 s (9.31 / 8.95 / 9.00)
Longest s.mu wait (chunk merge)	1.46 s	1.49 s
First request: trigger → built	298 ms	370 ms

These numbers agree with the PR's direction and magnitude: the post-load exclusive hold drops by about 4×, and there is one build fewer. Wall time and the chunk-merge maximum are within noise. As stated, the cost is deferred to the first request, not removed [K][A].

-race and TestNoSQLUnderStoreLock. On the merged tree, the 4 new tests plus TestDistanceIndexNotBuiltOnLoad, TestNoSQLUnderStoreLock*, TestDistanceBuild149* and TestDistanceLockOrder* pass with -race -count=20 (252 s, no races) [K]. In CI, Go Server Tests (3/3) runs these tests under -race via scripts/run-go-server-shard.js and shows them passing; the measurement test is skipped there [K].

Acceptance criteria → tests (red before / green after)

Issue #558 criterion	Test	Before (b871c486)	After
Real LoadChunked: index not built, no lazy build started by startup	TestDistanceIndexNotBuiltOnLoadChunked	red: 3 hops, 3 paths [T]	green [T]
Path-hop/subpath readiness, no new timeout or deadlock	TestLoadChunkedPathHopIndexReadyAtStartup558	green (guard; behaviour already correct) [T]	green [T]
Recomputer lifecycle: 202 while the build is held, 200 with non-empty expected data	TestDistanceLoadChunkedFirstRequestBuildsOnce558	red: started on an index that already held 6 records [T]	green [T]
Concurrent first requests coalesce; no redundant startup build	same test	red (same) [T]	green [T]
Generation change during a build: old generation not reported ready	TestDistanceLoadChunkedLoadCompletionDuringBuild558	green (guard) [T]	green [T]
-race, go vet, server suite; channels, not sleeps	see above	—	ok [K]
Red with the eager call restored	mutant R1	—	red [T]
Measured before/after on the same fixture	see the measurement table	—	[K]

The two "guard" rows are regression guards for behaviour that already worked, so red-before is not applicable. I proved they are not vacuous with mutants (R3; the author's M3/M4) [T].

My mutants (on the PR head)

ID	Mutation	Result
R1	Eager buildDistanceIndex(nodeSnapshot) restored in LoadChunked	killed: NotBuiltOnLoadChunked and FirstRequestBuildsOnce558 red [T]
R2	Alternative "fix": keep the eager build but publish distLazyBuilt/BuiltGen/LastBuilt/LastObs after unlock	killed: all three distance tests red (index non-empty; first request 200 instead of 202) [T]
R3	distIndexCurrentLocked ignores the generation	killed: LoadCompletionDuringBuild558 red (DistanceIndexBuilt() = true for the stale index) [T]
R4	Handler skips the !DistanceIndexBuilt() 202 gate	killed: FirstRequestBuildsOnce558 and LoadCompletionDuringBuild558 red [T]
R5	s.mu.RLock taken while distLazyMu is held in TriggerDistanceIndexBuild (#149 lock order)	killed by the existing TestDistanceBuild149_DebouncedTriggerSkipsWhenABuildFinishedMeanwhile (deadlock, timeout) [T]
R6	Generation read moved after s.mu.Unlock() in runDistanceIndexBuild	survived, see finding 1 (pre-existing gap, code untouched by this PR) [T]

Standing constraints

Constraint	Result
No behaviour change beyond the purpose	The only production diff is the removed call plus the snapshot simplification (and a comment). The distance API contract (202 → 200) and other startup indexes are unchanged [A][K]
cmd/server read-only	No DB access added to production code [A]
No new map[string]interface{} outside tests	0 in non-test lines; 4 in _test.go assertions [K]
Hardcoded colours	No frontend or CSS changes [K]
scripts/check-xss-sinks.sh --diff origin/master	Clean on a detached head checkout: "no public/**/*.{js,html} changes to scan" [K]
Fork guards	deploy.yml: 9, release-fast-path.yml: 1; no .github diff [K]
Closing keywords	None in the PR body or commit messages ("Relates to #558"); closingIssuesReferences is empty [K]
Commit author	All 3 commits: author and committer dborup <kontakt@meshview.dk> [K]

Tests I ran (merged tree against origin/master bb91292a)

Suite	Result
cmd/server: go vet ./... + go test -count=1 ./...	ok [K]
cmd/ingestor: go vet ./... + go test -count=1 ./...	ok [K]
Focused -race -count=20 (see above)	ok [K]
sh test-all.sh	271/271 files passed [K]
node test-frontend-helpers.js	725 passed, 0 failed [K]
Local Go server on a copy of e2e-fixture.db prepared as in CI (freshen, seed SQL from deploy.yml, corescope-migrate, seeds 2073, 199, 245 and 122); stopped via the pid found on its port, port free afterwards	Startup: no distance build; first request 202, then 200 with data [K]
test-e2e-playwright.js, once with a warm index and once on a cold server	154/157 both times, 3 built-in skips (2 marked flaky, 1 needs GO_BASE_URL); "Analytics Distance tab renders content" passes [K]
test-issue-1648-m4-icons-e2e.js (cold, then warm)	9/9 both runs [K]
test-a11y-axe-1668.js	0 violations across 120 cells, including 4 /analytics?tab=distance cells [K]
PR-specific Go tests on b871c486 (red-before check)	2 red, 2 guards green, as tabled above [T]

CI (run 40173799303, head b7a0eba0, attempt 1). All 15 jobs that ran passed, including Go Server Tests 1–3/3, Go Ingestor, Playwright E2E 1–3/3, JS unit/lint, Verify E2E Shards and Verify Go Test Assignments. Publish Badges, Release Artifacts and Deploy Staging were skipped by the fork guards. The server shard logs contain 0 "Action":"fail" events. The known flaky tests (#256, #267) did not fail, so there was no rerun [K].

Not verified

• I did not repeat the author's 150K-tx / 2.4M-obs measurement. Mine is a smaller shape (50K tx) on the same harness; no staging or production numbers [K].
• I did not run the full cmd/server suite with -race, only the focused set above. CI runs the full suite with -race [K].
• Local E2E used plain public/, not the instrumented copy. I ran the main suite plus the two distance-touching E2E files, not all shards [K].
• test-issue-1648-m4-icons-e2e.js warns "no map-jump button rendered (empty dataset)" on the distance tab, both cold and warm. I did not check whether origin/master prints the same warning. The test passes either way, and the warning does not depend on the index being cold [K][A].
• No browser screenshot. The change has no UI component [A].