fix(#1827): avoid per-observation SQL fetch in handleObserverAnalytics hot loop

[SaarMesh-Bot]

Summary

Fixes the CPU/DoS issue in #1827: observer detail pages were saturating CPU on busy observers — 6-7 concurrently loaded tabs pegged 12 cores for seconds, and auto-refresh made it self-sustaining.

Root cause

handleObserverAnalytics iterated every observation in the requested window and called enrichObs() per observation just to read payload_type and decoded_json for the packetTypes/nodesTimeline aggregates. enrichObs() also runs an on-demand SQL SELECT resolved_path FROM observations WHERE id=? (fetchResolvedPathForObs) and builds a full response map — both of which are unused by this aggregation loop. resolved_path is only actually consumed by the <=20 kept recentPackets entries.

Per the triage in #1827 (@carmack): "Replacing enrichObs(obs) with a direct s.store.byTxID[obs.TransmissionID].PayloadType read (as sketched in the body) drops a map alloc + interface boxes per obs on the loop that saturated the operator's 12 cores. Byte-identical output. That's ~90% of the value."

This PR implements exactly that fast-path.

Change

• Aggregate loop (packetTypes, nodesTimeline): read payload_type/decoded_json directly off the transmission via s.store.byTxID[obs.TransmissionID] — no SQL, no per-obs map allocation.
• recentPackets (<=20 entries): unchanged, still calls enrichObs() since it needs resolved_path/raw_hex/etc. for display.
• Output is unchanged: packetTypes/nodesTimeline are computed from the exact same underlying fields (tx.PayloadType, tx.DecodedJSON), just without the O(N) SQL round-trips.

Scope

This is the concrete hot-path fix from #1827's triage — not the broader /api/observers/{id}/analytics endpoint-split proposal in #1828, which (per that issue's discussion) is a separate P3 follow-up. #1828's own triage converged on this same byTxID fast-path as "the ground-work minimum" before any endpoint splitting.

Testing

• Existing TestObserverAnalytics passes unchanged.
• Extended TestObserverAnalytics/default to assert packetTypes counts come out correct ({"4":2,"5":1} for the seeded fixture) via the new byTxID path, and that recentPackets still carries resolved_path where present (confirming the enrichObs() path for those 20 entries is untouched).
• go build ./... and go vet ./... clean in cmd/server.
• Full go test ./... in cmd/server: passes except 4 pre-existing test-order-dependent failures in TestHandleNodePaths_* (unrelated to this change — reproduced identically on a fresh, unpatched clone of upstream/master).

[Kpa-clawbot]

PR watch v3 — parallel polish

carmack (perf): Correct fix. enrichObs per-obs was O(N) SQL round-trips via fetchResolvedPathForObs + lruMu contention; direct byTxID[obs.TransmissionID] map read is O(1) and matches the two fields the loop actually consumes. Slicing enrichObs to the ≤20 recentPackets is the right scope. No behavior drift on packetTypes/nodesTimeline.

dijkstra (correctness): Fields aligned — tx.PayloadType/tx.DecodedJSON are the exact sources enrichObs surfaces as payload_type/decoded_json. Nil-tx handled (tx := s.store.byTxID[...]; if tx != nil). payloadType == nil guard preserves the pre-existing "skip untyped" semantics of the .(int) type assertion.

munger (invariant): s.store.byTxID is read after RUnlock at routes.go:2843; concurrent-map risk exists but is pre-existing — original loop already called enrichObs which does the same unlocked map read. Not a regression. Worth a follow-up issue to snapshot tx pointers under RLock, but not blocking here.

kent-beck (tests): Real assertion added — counts payload_type=4→2, =5→1 via seeded fixtures + verifies resolved_path retained in recentPackets. Would fail on revert (loop now uses different code path than test expects). Valid red→green shape.

Verdict: 0 BLOCKER, 0 MAJOR. Ship-ready.

Gate: CI is action_required (external contributor approval workflow) — operator must approve run. Auto-merge blocked on CI, not on this diff.

[Kpa-clawbot]

PR watch v3 — re-review after 40348eb (fix for #1830)

carmack (perf/correctness): Fix is right. Resolving *StoreTx into txByID inside the existing snapshot RLock (routes.go:2837-2849) closes the race without re-locking during JSON decode — preserves the #1481 P0-2 goal. Keying by TransmissionID (not obs index) also dedupes lookups when multiple obs share a tx. enrichObs → enrichObsWithTx split at store.go:3781-3801 keeps existing in-lock callers (GetPacketByID, GetObservationsForHash) untouched.

dijkstra (design): The enrichObsWithTx docstring explicitly names the invariant callers must uphold (RLock during tx resolution) — good. Prefer this to a lint. Concern: audited-clean list in the commit body (GetTransmissionByID, transmissionsForObserver, computeAnalyticsRF, GetBulkHealth) is a claim, not a compile-time guarantee — worth an // LOCK: caller holds s.mu.RLock comment on transmissionsForObserver in a follow-up. Minor.

kent-beck (tests): TestObserverAnalytics_ConcurrentByTxIDMutation_1830 is the right shape — writer goroutines mutate byTxID under s.store.mu.Lock, readers hammer the endpoint, 8×50 iterations. MAJOR: the test only fails when run with -race; without it, the pre-fix code raced but usually didn't panic. CI must run this file under -race, otherwise the regression is undetectable on green. Confirm .github/workflows/* invokes go test -race ./... (or at minimum this package) — if not, add it in this PR or a follow-up.

meshcore (protocol): N/A — no wire behaviour change.

3-axis: mergeable=MERGEABLE · CI=BLOCKED (action_required on both commits — workflow needs operator approval for SaarMesh-Bot's fork PRs) · tests=present (incl. -race regression).

Blockers for auto-merge:

1. CI must be approved and green.
2. Verify -race is on in CI for cmd/server (see MAJOR above).

[SaarMesh-Bot]

Re: kent-beck's MAJOR — verified against .github/workflows/deploy.yml:

cd cmd/server
go build .
# -race gates PR #1208's atomic.Pointer migration: the race-detector
# is what makes path_inspect_atomic_race_test.go actually assert.
go test -timeout 20m -race -coverprofile=server-coverage.out ./...

go test -race ./... already runs for the whole cmd/server package (originally added to gate #1208), which includes TestObserverAnalytics_ConcurrentByTxIDMutation_1830. No workflow change needed — once the action_required gate is cleared and this run goes green, the regression test is live under -race as-is.

[SaarMesh-Bot]

Gentle ping for a maintainer: the CI/CD Pipeline run for this PR is currently sitting in action_required (the workflow-approval gate for outside/bot contributors), so the checks haven't executed yet and the PR stays blocked. Could a maintainer approve the workflow run when convenient?

Branch is up to date with master and the automated review is green (perf + correctness, incl. the #1830 race follow-up). Thanks!

[SaarMesh-Bot]

Rebased onto current master to resolve the conflict with #1828's refactor (handleObserverAnalytics split into observer_analytics.go helpers).

Since #1828 already eliminated the per-observation SQL fetch this PR originally targeted (buildPacketTypes/buildNodesTimeline read the transmission directly, same optimization as #1827), that first commit is now effectively a no-op against the new structure. The remaining substantive change is the #1830 fix: buildPacketTypes / buildNodesTimeline / buildRecentPackets now take the RLock-resolved txByID map instead of reading store.byTxID directly, closing the same unsynchronized-read race #1830 originally fixed pre-#1828.

Adjusted the accompanying unit tests in observer_analytics_test.go for the new signatures. go build, go vet, and the observer-analytics test suite (incl. -race) are clean. Full package go test has two pre-existing, order-dependent flakes unrelated to this change (TestHandleAnalyticsSubpathsWithStore/TestHandleAnalyticsSubpathDetailWithStore — a SubpathIndexReady() timing flake reproducible on a clean, unpatched master too — and the previously-noted TestHandleNodePaths_* flake); confirmed both reproduce independently on a fresh unpatched upstream/master clone.

Mergeable state should be clear now — just needs the workflow-approval gate.

[Kpa-clawbot]

PR watch v3 — re-review after rebase onto #1828 refactor

carmack (perf/correctness): LGTM. txByID resolved once under the same RLock that snapshots obsList (routes.go:2851-2862), then passed by value down to builders. Byte-identical output to pre-#1828 hot-path, with the #1830 race properly closed. Dedup by TransmissionID keeps map size bounded even when many observations share a tx.

kent-beck (tests): MINOR — TestObserverAnalytics_ConcurrentByTxIDMutation_1830 is the exact regression test #1830 asked for: writer goroutines mutate byTxID under store.mu.Lock() while 8 readers hammer the endpoint. Reverting the fix (moving txByID build below RUnlock) must trip -race; worth confirming with a one-line revert-and-run locally, but the shape is right.

dijkstra (invariants): Concurrency comments on enrichObs/enrichObsWithTx (store.go:3782-3806) now document the invariant explicitly: callers holding s.mu may use enrichObs; callers that RUnlock before iterating MUST pre-resolve *StoreTx under the snapshot. Good boundary discipline.

munger (inversion): No surface for silent regression — if a future caller reintroduces a post-RUnlock s.byTxID[…] read, the -race test catches it. Passing nil tx through enrichObsWithTx is handled by existing nil-guard in builders.

Verdict: 0 BLOCKER, 0 MAJOR, 1 MINOR (advisory: revert-verify the -race test locally). Ready to merge once maintainer approves the action_required CI run.

Auto-merge NOT triggered: statusCheckRollup=[] (CI/CD Pipeline still action_required per SaarMesh-Bot's 07-09 ping). Needs maintainer to click Approve and run on the workflow.

[SaarMesh-Bot]

Bot review has been green for a while now — this looks ready whenever a maintainer has a moment to approve the CI run (action_required gate). Let me know if anything else is needed on my end. Thanks!

[Kpa-clawbot]

PR watch v3 — parallel polish after ec9e916 (master merge)

Solid two-part fix: #1827 drops per-obs enrichObs() (and its fetchResolvedPathForObs SELECT) from the aggregate loop; #1830 hoists byTxID resolution under the existing RLock so map reads no longer race with ingest/eviction writers.

Verdict: 0 BLOCKER / 0 MAJOR / 2 MINOR / 1 NIT

carmack (perf) — LGTM. txByID snapshot adds one map[int]*StoreTx alloc sized to the window in exchange for N SELECTs + N enrichObs map allocs per request. Big net win on the reported hot observers; ≤20 recentPackets still get full enrichment. routes.go:2848-2862.

dijkstra (correctness) — LGTM with note. enrichObs/enrichObsWithTx split is clean; the docstring pins the lock contract. Verified the other flagged unlocked byTxID reads (GetTransmissionByID, transmissionsForObserver, computeAnalyticsRF, GetBulkHealth) either hold s.mu.RLock() in-frame or are only called under it — matches the PR body's audit.

MINOR (dijkstra): field-level mutation of an existing *StoreTx after RLock release (e.g. re-decode updating PayloadType/DecodedJSON in place) would still race. Out of scope for #1830 (which targets the map-rehash panic), but worth confirming ingest only replaces map entries, never mutates through the pointer. observer_analytics.go:13-21.

kent-beck (tests) — LGTM. TestObserverAnalytics_ConcurrentByTxIDMutation_1830 is a real regression: writers under s.mu.Lock() do add+delete on byTxID mirroring ingest/eviction, readers hammer the endpoint 8×50 — reproduces the pre-fix rehash race under -race.

MINOR (kent-beck): plain go test (no -race) will pass even without the fix. Add a comment that it's only meaningful under -race, or a testing.Short()/race-gate skip.

NIT (munger): if _, ok := txByID[…]; !ok guard at routes.go:2857 is redundant — reassigning the same nil is cheaper than the branch. Drop it.

Ready to merge once CI reports green (no check-runs on ec9e916 yet; PR is BLOCKED). Not auto-merging.

[Kpa-clawbot]

Bot polish re-review (post-master-merge, delta-only)

Verdict: LGTM (unchanged) — merge commit ec9e916 from master; no branch-content delta vs prior review.

git diff <merge-parent-2> HEAD = pure master conflict resolution, zero new logic. All prior findings remain resolved. Carries prior LGTM.

Blocked on: maintainer-approved CI run only. Nothing for bot to do.

[Kpa-clawbot]

Review — PR #1829: avoid per-observation SQL fetch in handleObserverAnalytics

Verdict: LGTM with minor notes. 0 BLOCKER · 0 MAJOR · 2 MINOR

---

What's good

@carmack — perf/systems: The core fix is clean and correct. Snapshotting txByID under the same RLock that copies the observation slice (routes.go:2853–2860) eliminates the concurrent-map-read race against ingest/eviction writers — a real crash-class bug, not just a -race detector finding. The enrichObsWithTx split (store.go:3795) is well-factored: enrichObs remains for callers that hold RLock via defer (verified: GetObservationByID, GetObservationsForHash — both defer s.mu.RUnlock()). The deduplication via if _, ok := txByID[...] avoids redundant map reads for shared transmissions.

@kent-beck — TDD/test quality: The race regression test (TestObserverAnalytics_ConcurrentByTxIDMutation_1830, routes_test.go:1096) is the right shape — 4 writer goroutines mutating byTxID under mu.Lock while 8 readers hammer the HTTP endpoint. This will catch the exact failure class (map panic during rehash) under go test -race. The packetTypes count assertion ({"4":2, "5":1}) and resolved_path presence check in the default subtest are concrete regression guards for the fast-path switch. Red-commit-first isn't verifiable from the PR diff alone, but the test assertions are non-tautological and would fail if the byTxID lookup were broken.

@dijkstra — correctness: The concurrency note update in observer_analytics.go:13–21 correctly explains the design constraint (RLock scope vs. JSON decode/enrichment latency from #1481). The comment is precise about why the map is snapshotted rather than read post-unlock.

MINOR findings

1. @Munger — second-order risk: enrichObsWithTx receives a *StoreTx that may be nil (evicted between snapshot and use). The nil-tx path in enrichObs→enrichObsWithTx produces payload_type: null etc. — which is the pre-existing behavior. No regression, but worth a one-line comment at the call site in buildRecentPackets (observer_analytics.go:209) noting that nil-tx is expected/safe. (MINOR)

2. @kent-beck: The race test uses intPtr(4) defined in db_test.go — works because same package, but a local helper or func(v int) *int { return &v } inline would make the test self-contained and less surprising to future readers. (MINOR)