[dborup]

Summary

Keep expensive neighbor graph construction out of requests once a graph exists.

• Serve the published Server.neighborGraph, even past the old TTL, while the background recomputer builds a replacement.
• Keep the pointer mutex limited to snapshot/swap. A separate build mutex serializes initial and periodic builds without blocking readers of an existing graph.
• Every configured recompute tick builds a new graph, including when the previous snapshot is still fresh. A cold first request may still wait for the initial graph.
• Preserve injected graphs when no store is present and retain the previous published snapshot after a failed background build.

The distinct persisted PacketStore.graph / 60-second recomputer is untouched. Base: 8798f74eeec124f21d0e82a8d64b93b31d7facc0. No schema, API, dependency or workflow change. Only comments for the existing recompute-interval setting are updated; no new knob or default is introduced.

Deterministic verification / independent review

• Four intended regressions fail on unchanged production code, then pass with this change; the initial test-fixture mistakes were corrected before recording the valid red run.
• Six new tests exercise stale reads, fresh-tick replacement, a real SQL-pool-blocked build with a non-blocking reader, sixteen simultaneous initial readers, no-store injection and panic recovery retaining the previous graph.
• Focused -race -count=3 passes; parent independently ran the six tests twice under -race (1.531 s), with non-empty execution and no races.
• Full server suite passes (82.863 s); vet and diff checks pass. Ingestor code is unchanged and the full verified baseline passed.
• Parent and separate reviewer inspected the immutable tree: APPROVE without blockers.
• Parent's actual-browser smoke on the candidate server passes: 154 tests, 3 fixture-dependent skips, 0 failures.
• Parent's full frontend/script inventory on this immutable tree passes in one invocation: 260 files, 0 failures (host-tool capability skips remain explicit). CI gate guards and test assignment checks are retained.

Measured request-side cost

The reproducible benchmark deliberately presents a stale graph with 30,000 packets. Baseline getter median (three rounds of three iterations): 16.832 ms, approximately 13.07 MB and 280,032 allocations per operation. The new getter is constant-time with 0 B / 0 allocations; a longer 100,000-iteration control measured 7.493–12.11 ns/op (median 8.789).

This measures the snapshot getter, not endpoint latency, production throughput or eliminated work. The graph scan, CPU and allocations are moved to the background and still paid on each tick. Lower configured intervals cost more CPU; higher intervals allow older results. No live-instance performance claim is made.

Limitations

Serialization is not perfect startup coalescing: if an HTTP request builds first and the initial recompute follows, two builds can occur sequentially. They cannot overlap, and existing readers do not wait for the second build. No new panic-recovery policy or destructive historical rebuild is added.

Local validation is macOS arm64 / Go 1.26.3. Linux PR CI is separate. Draft only, auto-merge off; no ready, merge, publish, deployment or staging/production contact.

Post-push Chromium confirmation on the published commit: Nodes list/detail and real neighbor/Paths API contracts pass on a disposable local fixture. Initial PR CI run: 40067027978; no rerun was requested.

Source credit / discovery

Adapted from the original contribution by efiten:

Source repository: Kpa-clawbot/CoreScope
Pull request number: 2222
Head commit: de7ae981875e0676e4680cb4dbd92808c6ec1618
Merge commit: efd2aeabb4ecb62de98bbfd7356a01b21a7926be

[dborup]

Coordinator note for the review: the first CI run failed in Playwright shard 1 at test-region-scope-approval-e2e.js. The failure was sqlite3 … INSERT INTO observer_neighbors … Error: stepping, database is locked (5) (job 120792334355).

This PR moves neighbour-graph rebuilds into the background, so please check whether those rebuilds hold a read transaction on the DB long enough to block an external writer. That would happen if the fixture DB is not in WAL mode, or if a rebuild keeps a statement or transaction open across the whole build. Also check whether the test is simply flaky on master.

I have started a rerun of the failed jobs.

[adminopenclaw8-sketch]

Review — CS-Macmini PR#541 — head 5196586

Dom: APPROVE med nits

Independent, read-only review. origin/master = c48510b3; merged tree = abae89e1 (git merge-tree --write-tree), identical to head for every file this PR touches. git ls-remote showed the same head before and after the review. No PR state was changed.

The change does what it says: the expensive store scan moves off the request goroutine and out of the reader's mutex, the swap is a genuine atomic publish of a finished graph, and initial readers coalesce. The perf claim holds and is in fact understated — see the request-latency measurement below. The nits are two confirmed test gaps, one widened config knob, and a red CI run the body does not disclose.

Findings

#	Sev	Where	Finding
F1	nit (behaviour)	cmd/server/config.go:495, neighbor_api.go:85	cacheRecomputeIntervalSeconds is now the only bound on graph age, and every tick does a full build. The knob's blast radius widened with no floor.
F2	nit (test gap)	cmd/server/neighbor_graph_cache.go:76	Nothing pins that the cached response is built from the graph the same tick just produced. Mutant survives the whole suite.
F3	nit (test gap)	cmd/server/neighbor_api.go:118-120	The pointer-mutex invariant — the core claim — is unpinned. Dropping neighborMu around the publish survives -race.
F4	nit	cmd/server/config.go:495, config.example.json:386	Rewritten comments dropped the #1481 P0-1 / #1483 issue references the old text carried.
F5	nit (reporting)	PR body	CI run 40067027978 is red (3 failed jobs); the body cites the id without saying so. Cause is an unrelated infra flake.
F6	nit (pre-existing)	cmd/server/neighbor_api.go:127-130	The NeighborMaxEdgeKm() / DebugAffinity wiring this PR moved is untested; a mutant that drops it survives the whole suite.
F7	nit (optional)	cmd/server/routes.go:78	neighborMu + pointer is the house atomic.Pointer pattern (store.graph, neighborGraphCache.ptr); using it would delete the mutex and make F3's mutant unrepresentable.

F1 detail. Before: getNeighborGraph rebuilt on request once neighborGraphTTL (5 min, neighbor_graph.go:22) expired, so the graph was never older than 5 minutes regardless of the configured interval, and the knob only changed the response-cache cadence. After: the knob alone governs graph age, and the graph is rebuilt on every tick. Consequences for existing configs:

• cacheRecomputeIntervalSeconds: 3600 previously meant "refresh the cached response hourly, graph still ≤ 5 min old". It now serves a one-hour-old affinity graph to /api/nodes/{pubkey}/neighbors, /api/debug/affinity and the hop-resolution affinity path (routes.go:3283). [K]
• A low interval previously cost only a re-filter; it now costs a full graph build per tick. Measured: a build at 30 000 tx / 2 000 nodes takes ≈ 1.30 s [T], so any interval below ~2 s leaves the background recomputer permanently building.

The default (300 s) equals the old TTL, so default behaviour is unchanged, and the new comment does say "higher = an older graph between ticks". That is why this is a nit and not a blocker. Suggestion: clamp the graph rebuild to at most once per neighborGraphTTL (or document a minimum interval) and keep the response-cache cadence free, so an operator who tuned the knob for response freshness does not silently change graph freshness or CPU.

F2 detail. Mutant M2 — swap the two statements so the response is built before the rebuild:

resp := s.buildDefaultNeighborGraphResponse()
s.rebuildNeighborGraph()

go test ./... in cmd/server: ok, 65.5 s, exit 0 [T]. With that mutant every cached /api/analytics/neighbor-graph response is one full interval older than the graph — exactly the staleness this PR exists to bound — and no test notices. A test that asserts the cached response reflects the post-rebuild graph (e.g. a node that only exists after the rebuild appearing in loadNeighborGraphCache()) would close it.

F3 detail. Mutant M3 — publish without the pointer mutex in rebuildNeighborGraph:

g := s.buildNeighborGraph()
s.neighborGraph = g

go test -race -count=2 -run TestNeighbor ./...: ok [T]. A 12-line reviewer probe (4 goroutines calling rebuildNeighborGraph, 4 calling getNeighborGraph) reports WARNING: DATA RACE under that mutant and passes on the unmodified tree [T] — so the race is real and simply unexercised: no shipped test ever rebuilds and reads concurrently. TestNeighborGraphReadersContinueDuringBlockedRebuild comes closest, but its reader completes before the publish, so -race establishes a happens-before and sees nothing. The comment at routes.go:78 ("protects the published pointer, never a graph build") is currently documentation only.

F5 detail. Failing jobs: 🎭 Playwright E2E Tests (3/3), and 📊 Verify E2E Shards & Coverage + 🏗️ Build & Publish Docker Image cascading from it. Single root cause [T]:

test-region-scope-approval-e2e.js: FAIL — Error: stepping, database is locked (5)
  sqlite3 … INSERT INTO observer_neighbors(observer_id, neighbor_pubkey, scopes, status, reported_at) …

An sqlite3 CLI write racing the ingestor/server that test starts against the same DB. Nothing to do with the neighbour graph, and not #256 (Hash Stats sort) or #267 (backfill write-hold) — a third flake. That test is 11 passed, 0 failed locally on the merged tree [T]. A rerun is owed, and the body should state the run is red and why rather than only citing the id.

Answers to the review points

#495 / #412 lock rule. TestNoSQLUnderStoreLock and TestStoreSQLDoesNotHoldPacketLock412 pass on the merged tree [T]. BuildFromStoreWithOptions takes store.mu.RLock() only to copy the tx/observation pointers and releases it before the scan, phase 1.5 and disambiguate() (neighbor_graph.go:283-291) — unchanged by this PR [K]. Neither the cold request build nor the tick build ever holds PacketStore.mu; they hold neighborBuildMu only. The one SQL-capable call under that mutex is getCachedNodesAndPM(), which manages its own locking and is unchanged.

No half-built graph. Verified [K]: BuildFromStoreWithOptions constructs a fresh NewNeighborGraph() that is unreachable from Server until the finished pointer is assigned under neighborMu; readers take the same mutex, which gives the happens-before edge; nothing mutates a published graph afterwards (builtAt and nodeGeo are written before return, MarkAmbiguous is a test-only helper). On a failed build the publish never happens, so the previous snapshot stands (TestNeighborGraphFailedRebuildKeepsPublishedSnapshot, red on master [T]).

No stampede. neighborBuildMu serializes builds and getNeighborGraph re-checks the pointer after acquiring it. My mutant M1 (delete that inner double-check) is killed by TestNeighborGraphConcurrentFirstReadersShareBuild [T]. Remaining, as the body states [A]: a cold HTTP build and the startup recompute can run sequentially (never overlapping), and that is not coalesced [K].

Perf claim — reproduced and extended.

• Author's benchmark [A], same harness, median of 3 [T]: master getter 13.61 ms/op, 6.67 MB, 160 038 allocs; head getter 8.50 ns/op, 0 B, 0 allocs. Direction and magnitude confirmed; my absolute numbers differ from the body's 16.832 ms / 13.07 MB / 280 032 allocs [A] (different machine and Go version), the ratio does not.
• My own measurement at the requested shape, 30 000 tx / 2 000 nodes, one recompute tick running while a competing request goroutine samples getNeighborGraph [T], 3 rounds each:

	tick duration	request p50	request max
master	1.29–1.30 s	42 ns	1.27–1.28 s
head	1.35–1.37 s	0–42 ns	70 µs – 2.6 ms

On master exactly one request absorbs the entire build. On head the worst request-side wait drops by roughly three orders of magnitude while the tick cost is unchanged (the ~50 ms delta is contention from my own 26 M-iteration sampling loop). This is stronger evidence than the getter microbenchmark, and it is the number the PR should quote.

• Live wiring, local server with cacheRecomputeIntervalSeconds: 2 on e2e-fixture.db [T]: 60/60 HTTP 200 on /api/nodes/{pubkey}/neighbors at max 2.1 ms while the recomputer rebuilt every 2 s; 13 ticks, 0 panics, no deadlock.

Overlap with #540 (bridge-TX filter), head 9583855a. No conflict and no interaction [T]:

• git merge-tree origin/master 9583855a 51965868 reports no conflict; sequential merges in both orders produce the identical tree 99e5b238.
• Zero file overlap: fix(neighbors): exclude bridge TX records from reception-derived edges #540 touches cmd/ingestor/neighbor_builder.go + cmd/server/neighbor_graph.go (a 4-line obs.Direction == "tx" skip inside BuildFromStoreWithOptions); perf(neighbors): rebuild graphs outside request and pointer locks #541 touches neighbor_api.go, neighbor_graph_cache.go, routes.go, config.go, config.example.json.
• Semantically orthogonal: fix(neighbors): exclude bridge TX records from reception-derived edges #540 changes what the build emits, perf(neighbors): rebuild graphs outside request and pointer locks #541 changes when the build runs. perf(neighbors): rebuild graphs outside request and pointer locks #541's fixtures leave StoreObs.Direction empty, so fix(neighbors): exclude bridge TX records from reception-derived edges #540's filter does not touch them.
• Recommendation: merge order is free. Either order yields the same tree.

Acceptance criteria / red before green. There is no issue, so I used the four behaviours the body claims. With the new test file kept and only the three production files reverted to master, 5 of 6 new tests fail [T] — ExistingStaleSnapshotDoesNotRebuild, TickRebuildsEvenFreshSnapshot, ReadersContinueDuringBlockedRebuild, RecomputeWithoutStoreKeepsInjectedGraph, FailedRebuildKeepsPublishedSnapshot. The body says four [A]; the count is five [T]. The sixth, ConcurrentFirstReadersShareBuild, passes on master (master's single mutex serialized everything) but is still a valid guard — it is what kills M1.

No behaviour change beyond the purpose. The persisted PacketStore.graph / 60-second recomputer and path_inspect.go's stale-while-revalidate path are untouched [K]. One residual semantic change beyond F1: with s.store == nil master returned a fresh empty graph per request (zero builtAt ⇒ always stale), head publishes one empty graph permanently. Only reachable in tests; no production path constructs a store-less server.

Tests and mutants

All on the merged tree (abae89e1), macOS arm64, Go 1.27.0 (CI pins Linux / 1.27.1).

Legend: [T] measured by me, [A] author's claim, [K] code reading.

Suite	Result
go vet ./... (cmd/server)	clean
go test ./... (cmd/server)	ok 64.9 s
go test ./... (cmd/ingestor)	ok 107.6 s
go test -race -count=3 -run 'TestNeighbor|TestNoSQLUnderStoreLock|TestStoreSQLDoesNotHoldPacketLock412'	ok 14.5 s, no races
sh test-all.sh	260 passed, 0 failed
node test-frontend-helpers.js	719 passed, 0 failed

E2E against a local Go server on port 13802 with e2e-fixture.db prepared exactly as CI does (tools/freshen-fixture.sh, the Kpa-clawbot#1486 + Kpa-clawbot#1791 seed SQL, corescope-migrate, then seeds 2073, 199, 245, 122; plus a .git-commit file). The PR adds no E2E of its own, so I ran every neighbour/affinity/path consumer [T]:

test-issue-1758-ng-filter-rerenders-e2e PASS · test-issue-254-affinity-toggle-mobile-aria-e2e 14/14 · test-path-inspector-e2e 7/7 · test-path-inspector-coverage-e2e 10/10 · test-observer-direct-neighbors-panel 21/21 · test-show-neighbors 4/4 · test-hop-resolver-affinity 18/18 · test-issue-254-affinity-debug-toggle 12/12 · test-observer-neighbors-tool 13/13 · test-observer-neighbors-report-badge 3/3 · test-region-scope-approval-e2e 11/11 (the CI-red one). Servers were stopped by port lookup.

Mutants (all mine, 4 run):

#	Mutation	Outcome
M1	Delete the inner pointer re-check after acquiring neighborBuildMu in getNeighborGraph	killed — ConcurrentFirstReadersShareBuild: "concurrent initial requests did not share one populated graph"
M2	Build the cached response before rebuildNeighborGraph() instead of after	survived the full suite → F2
M3	Publish s.neighborGraph = g without neighborMu in rebuildNeighborGraph	survived -race -count=2; a reviewer probe proves it is a genuine DATA RACE → F3
M4	Drop opts.MaxEdgeKm = s.cfg.NeighborMaxEdgeKm() from buildNeighborGraph	survived the full suite → F6 (pre-existing)

Policy checks [T]: commit author and committer are dborup <kontakt@meshview.dk>; no closing keywords in the commit message or the PR body; fork guards unchanged at 9 (deploy.yml) and 1 (release-fast-path.yml), no workflow file touched; no new map[string]interface{}; no hardcoded colours (the only hex-shaped match is the issue reference #1228); cmd/server gains no SQL and no DB write; scripts/check-xss-sinks.sh --diff origin/master is vacuously clean — its file set is public/**/*.{js,html} and this PR changes zero frontend files.

Not verified

• No Linux CI rerun was triggered, and I cannot attest the flake does not recur there. Everything above is macOS arm64 / Go 1.27.0.
• No staging or production contact, no real-instance throughput or live-latency figure. My 2 000-node / 30 000-tx numbers come from synthetic in-memory fixtures, not captured data.
• The coverage-instrumented frontend variant (public-instrumented) was not built; E2E ran against plain public.
• Untouched modules (internal/*, cmd/anomaly, cmd/decrypt, cmd/migrate) were not exercised beyond what cmd/server and cmd/ingestor pull in.
• BenchmarkNeighborGraphStaleRead30K is not executed by CI (benchmarks are excluded), so it is unguarded against bit-rot; I ran it manually.
• Behaviour at the 5-minute default interval boundary over a real multi-hour run was not observed; the live check used a 2-second interval over ~30 seconds.