fix(api): admission control for perf memory diagnostics (#371)

[dborup-agent]

Relates to #371

Problem

GET /api/perf?mem=1 is public and runs GetStoreMemoryBreakdown(), which walks every stored transmission and observation under the packet-store read lock. Overlapping or rapidly repeated mem=1 requests each run that full walk. Together they can hold the read lock long enough to stall ingest and eviction, which need the write lock.

Plan and change

1. Tests first (test(perf): …, red on master): they pin the behaviour below over a fake clock and a parked scan, so nothing depends on wall-clock timing.
2. Limiter (cmd/server/perf_mem_admission.go): perfMemLimiter, a per-Server (that is, per-process) gate. It admits one scan at a time and refuses new ones for perfMemScanCooldown (5 s) after a scan completes. The zero value is ready to use, as with the Reach cold limiter (perf(reach): bound concurrent cold-cache scans across distinct node requests #341/perf(reach): bound concurrent cold-cache scans across distinct nodes (#341) #358). There are configure and hook seams for tests, plus acquired and rejected counters.
3. Handler (handlePerf): when there is a store and mem=1, admission is the first thing the handler does, before any store call. A refused request gets 429, Retry-After (whole seconds, rounded up) and {"error":…,"retryAfter":N}, the same shape as writeReachColdBusy429, and returns before any store call. The existing setRetryAfter helper writes the header. An admitted request releases its slot as soon as the walk ends, not after the response write, so a client that reads slowly cannot hold the slot. The release function is single-use, and a defer still covers a panic.
4. Docs: docs/api-spec.md and the OpenAPI entry document mem and the 429.

Unchanged: /api/perf without mem=1 (including mem=0 and mem=true), and mem=1 on a storeless server, which still returns 200 with no breakdown and never consults the limiter. The response schema and frontend are unchanged; nothing in public/ uses mem=1.

Why not queue like Reach?

Reach waits up to 3 s for a slot because each request is a page a user is waiting on. The breakdown is an operator diagnostic, and the issue asks to reject the excess, so this limiter refuses at once and never parks a goroutine.

Retry-After while a scan is running

The remaining scan time is unknown, so the limiter advertises the full cooldown. That is the earliest moment a new scan could be admitted.

Tests (cmd/server/perf_mem_admission_test.go, all under -race)

Acceptance point	Test
Concurrent requests are rejected before the store lock, with 429 and headers	TestPerfMemAdmission_ConcurrentRejectedBeforeStoreLock: one scan is parked after admission. The test holds store.mu write-locked while 16 more mem=1 requests run. Any of them that reached RLock or the traversal would block, and the test would time out. All 16 get 429 + Retry-After: 5, and the parked scan then returns 200 with a breakdown. acquired == 1, rejected == 16.
Immediate repeat, cooldown boundary, release	TestPerfMemAdmission_RepeatCooldownBoundary: repeat → 429/5; 1.5 s left → 2; 1 ns left → 1; at the boundary → 200 again.
Cooldown starts at completion	TestPerfMemAdmission_CooldownStartsAtCompletion: a scan that takes 30 s on the fake clock still leaves the full 5 s afterwards.
Release does not wait for the response write	TestPerfMemAdmission_ReleasedBeforeResponseWrite: the response writer blocks, and the next scan is still admitted after the cooldown.
Ordinary /api/perf unchanged	TestPerfMemAdmission_OrdinaryPerfUnaffected: no mem, mem=0 and mem=true all return 200 with no breakdown and no Retry-After, both during a scan and during the cooldown.
Storeless unchanged	TestPerfMemAdmission_StorelessUnaffected: the limiter is held busy and mem=1 still returns 200.
Release is single-use	TestPerfMemLimiter_StaleReleaseIsNoop
Zero value works in production	TestPerfMemLimiter_ZeroValueDefaults

15 mutants were all killed; the report comment lists them. The PerfMem tests also passed 30× under -race.

Perf justification

Admission is O(1) under a tiny mutex, on the mem=1 path only. Ordinary /api/perf gains one query-string check. Refused requests do strictly less work than before: no store lock and no walk.

Left for the owner

• Cooldown length (5 s). It is hard-coded as perfMemScanCooldown. A tight polling loop now costs at most one walk per scan time + 5 s. If operators want a different value, it could become a config knob; I have not added one (no need demonstrated yet).
• Public vs API-key. mem=1 stays public, as the issue scopes it. Putting it behind the API key instead would be a product decision.
• Process-local state. Each server process has its own gate, as the issue asks. A multi-instance deployment would get one scan per instance.

Invariants

• cmd/server stays read-only: no DB writes and no write locks were added.
• No new map[string]interface{}; the 429 body is a fixed literal like Reach's.
• No frontend change; bash scripts/check-xss-sinks.sh --diff origin/master is clean.
• Fork guards are unchanged (9 in deploy.yml, 1 in release-fast-path.yml).

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent3 PR#425 #371 — head 9f9e054

Status: Implemented and self-verified. The draft is open and every CI job is green. No merge, ready or approve actions were taken. The #271 flake did not appear, so nothing was rerun.

Evidence tags: [T] = I ran it. [A] = from reading the code. [K] = from CI.

Scope recheck

• [A] I rechecked current master 4a5ec5e2 (the issue was written against 1f423e06). handlePerf still calls GetStoreMemoryBreakdown() for every mem=1 request with no gate. That function holds s.mu.RLock for a full O(tx+obs) walk.
• [A] Nothing in public/ requests mem=1. The perf page uses plain /api/perf, so no frontend consumer changes.
• [A] Reused patterns rather than adding parallel ones:
  • the 429 + Retry-After + {"error","retryAfter"} shape and the zero-value / configure / hook / counter structure of the Reach cold limiter (node_reach_cold_limit.go, perf(reach): bound concurrent cold-cache scans across distinct node requests #341/perf(reach): bound concurrent cold-cache scans across distinct nodes (#341) #358);
  • the existing setRetryAfter helper (channel_proposals.go) for the rounded-up header;
  • noStoreAPIMiddleware, which already adds Cache-Control: no-store to the 429.
• [A] fix(api): cap and validate multi-node packet filters before query work (#335) #385 (multi-node packet filter caps) touches a different handler, and fix(ui): escape untrusted node, observer and route values in HTML rendering #373, fix(ui): escape perf dashboard values (#392) #394 and fix: Follow-ups to #373: analytics row a11y, delegated navigate test, compare.js onclick, loadL (#393) #402 are frontend XSS work. None of them overlaps.

Per-requirement evidence (red before → green after, plus mutants)

"Before" means the new tests run against master's handlePerf (no gate). On the real master commit 97e28f8e (tests only), the package does not compile because perfMem does not exist yet.

#	Requirement	Test	Before	After	Mutants (all red) [T]
1	One active scan; concurrent requests get 429	TestPerfMemAdmission_ConcurrentRejectedBeforeStoreLock (16-request burst while one scan is parked)	red [T]	green [T]	M7 (busy wait 0), M8 (no single-active check), M13 (status 200)
2	Reject before the store read lock; traversal not entered	same test: it holds store.mu write-locked during the burst, so any rejected request that reached RLock or the walk would block and time out. acquired == 1, rejected == 16	red [T]	green [T]	M1 (gate moved after the store stats calls) → red because the burst blocks on the lock
3	Immediate repeat, cooldown boundary and release	TestPerfMemAdmission_RepeatCooldownBoundary: repeat → 429/5; 1.5 s left → 2; 1 ns left → 1; at the boundary → 200	red [T]	green [T]	M4 (>= boundary), M12 (release never frees)
4	Cooldown starts at completion	TestPerfMemAdmission_CooldownStartsAtCompletion (a 30 s scan on the fake clock)	red [T]	green [T]	M9 (cooldown from admission)
5	Response headers (Retry-After, JSON, body retryAfter equal to the header)	asserted in tests 1, 3 and 4 via assertPerfMemBusy	red [T]	green [T]	M5 (Retry-After floored), M6 (no Retry-After header)
6	Slot released at scan end, not after the response write	TestPerfMemAdmission_ReleasedBeforeResponseWrite (blocking ResponseWriter)	green before (master has no gate) [T]	green [T]	M10 (release only via defer)
7	Release is single-use (it is both explicit and deferred)	TestPerfMemLimiter_StaleReleaseIsNoop	n/a (new type)	green [T]	M11 (release not single-use). This mutant first survived; I tightened the test to step past the cooldown, and it is now red.
8	Ordinary /api/perf unchanged (no mem, mem=0, mem=true)	TestPerfMemAdmission_OrdinaryPerfUnaffected (during a scan and during the cooldown)	red [T] (the scan is never admitted on master)	green [T]	M3 (gate every /api/perf)
9	Storeless behaviour unchanged	TestPerfMemAdmission_StorelessUnaffected (limiter held busy; mem=1 → 200, no breakdown)	green before (an invariant) [T]	green [T]	M2 (gate ignores the storeless case)
10	The zero value works in production	TestPerfMemLimiter_ZeroValueDefaults	n/a (new type)	green [T]	M14 (no default cooldown fallback)

Mutant run: [T] 15 mutants (M1–M14 plus the "before" handler), each applied to the working tree, run with go test -race -run PerfMem, then restored. All are red. [T] go test -race -count=30 -run PerfMem → ok (240 runs, no flake).

CI per job (run 38384878638, head 9f9e054) [K]

Job	Result
Go Build & Test	✅ pass 24m56s
Playwright E2E Tests	✅ pass 25m38s (main suite 132/135, 3 skipped as on master)
Build & Publish Docker Image	✅ pass 51s
Release Artifacts / Publish Badges / Deploy Staging	skipped (fork guard)

The #271 flake did not appear, so no rerun was needed.

Local verification

• [T] cmd/server: go test -timeout 20m -race -count=1 ./... → ok (746 s). A first run with Go's default 10 m timeout timed out on total runtime only. No test was hung or failing; CI also uses -timeout 20m.
• [T] cmd/ingestor: go test ./... → ok. go vet on cmd/server is clean.
• [T] sh test-all.sh → 238 passed, 0 failed. node test-frontend-helpers.js → 713 passed, 0 failed.
• [T] Ran E2E against a local Go server built from this branch, using e2e-fixture.db prepared as in CI (freshen, seed SQL, migrate, the three seed files) on a free high port, stopped by pid afterwards:
  • test-xss-392-perf-e2e.js → 1/0;
  • test-e2e-playwright.js → 132/135, with 3 pre-existing skips (flaky-marked and fixture-dependent).
• [T] curl probe against that server: mem=1 → 200 with memoryBreakdown; an immediate mem=1 → 429 with Retry-After: 5, Cache-Control: no-store and {"error":…,"retryAfter":5}; plain /api/perf → 200 with no breakdown.

Left for the owner

• Cooldown length (5 s, perfMemScanCooldown). It is hard-coded. A config knob, or a customizer entry for operators, can come later if there is demand.
• Public vs API key. mem=1 stays public, as scoped by the issue. Gating it behind the API key would be a product decision.
• Process-local state. Each server process gets one scan at a time, as the issue specifies. Multi-instance deployments get one scan per instance.
• Perf stats counting. Refused 429s are counted under /api/perf in the perf stats, like every other response. I left that unchanged.

Checklist

Item	Result
Every point has a red-before/green-after test	Yes [T]. Points 6 and 9 are invariants that are already green on master; their mutants are red.
At least one mutant per point	15 mutants, all red [T]
cmd/server read-only	No DB writes or write locks added. The limiter uses only an in-memory mutex. [A]
No new map[string]interface{}	0 in the diff [T]
No hardcoded colours	No CSS or frontend change [T]
bash scripts/check-xss-sinks.sh --diff origin/master	exit 0, no public/ changes [T]
Fork guards	9 in deploy.yml, 1 in release-fast-path.yml, unchanged [T]
Closing keywords	none; closingIssuesReferences is empty and the body opens with "Relates to #371" [T]
Commit author/committer	dborup <kontakt@meshview.dk>, two commits, no rebase, amend or force-push [T]
Scope	5 files: the limiter, its tests, routes.go, openapi.go, docs/api-spec.md [T]

[adminopenclaw8-sketch]

Review — CS-Macmini PR#425 — head 9f9e054

Dom: APPROVE med nits

Independent, read-only review. No push, merge, ready, label or gh pr review action was taken. git ls-remote on the branch was 9f9e0540 before and after this review; origin/master was 8f40525f throughout. Everything below was verified on the merged tree (git merge-tree --write-tree origin/master 9f9e0540 → dac3d248), not on the branch alone.

Evidence tags: [T] = I ran it here. [A] = read from the code. [K] = from CI.

Findings

#	Sev	Where	Finding
N1	nit	cmd/server/openapi.go:59	The new 429 is documented only in the route's prose Description. routeMeta already has an Errors []routeError field added for exactly this (#334) and used for 400/413 on /api/packets, /api/packets/observations and /api/decode. [T] The live spec from GET /api/spec lists responses: ["200"] for /api/perf and contains zero 429 declarations repo-wide, so a generated client still believes this route only ever answers 200. Consistent with the Reach cold 429, which is also prose-only — so this is a missed opportunity, not a regression.
N2	nit (test gap)	cmd/server/routes.go:1143	defer releaseMem() survives deletion: with it removed, all 8 PerfMem tests stay green under -race [T]. [A] There is no early return between acquire() and the walk, so the defer is panic-only insurance and there is nothing a test can reach today — but the PR body states "a defer still covers a panic", and that claim is unpinned.
N3	nit (test gap)	cmd/server/perf_mem_admission.go:113	No test pins Cache-Control: no-store on the 429, although the author's report lists it as part of the shape. [T] I confirmed it is present on the real server (set by noStoreAPIMiddleware, routes.go:293, which RegisterRoutes installs on the test router too — so a one-line assert in assertPerfMemBusy would cover it).
N4	note	—	Deployment effect not in "Left for the owner" (see below).

Nothing here blocks the PR. No correctness, concurrency or security defect found.

Points, one by one, against issue #371

The issue's three "Requested change" bullets and three "Acceptance" bullets:

Issue point	Status	Evidence
One active scan per server process + a short cooldown after completion	Done, tested	perfMemLimiter is a per-Server field (routes.go:60), so per process. [T] Killed by mutants X3 (cooldown from admission instead of completion) and X4 (release not single-use).
Reject excess before taking the packet-store read lock; traversal not entered	Done, tested	Admission is the first statement in handlePerf (routes.go:1134-1144), before any store call. The test holds store.mu write-locked for the whole 16-request burst, so any request that reached RLock would block. [T] Mutant X1 (gate moved into the breakdown block) → FAIL: rejected mem=1 requests blocked on the packet-store lock.
Clear retry response, e.g. 429 with Retry-After	Done, tested	429 + Retry-After (whole seconds, rounded up) + {"error",…,"retryAfter":N}, same shape as writeReachColdBusy429, built on the existing setRetryAfter helper. [T] Mutants X5 (floored instead of rounded up) and P3 (429→503) both killed.
Ordinary /api/perf unchanged	Done, tested	No mem, mem=0 and mem=true all stay 200 with no breakdown and no Retry-After, during a scan and during the cooldown. [T] Mutant X6 (gate every /api/perf) killed.
Storeless behaviour unchanged	Done, tested	[T] Mutant X8 (drop the s.store != nil guard) killed by StorelessUnaffected.
Admission state process-local	Done	[A] Plain struct field on Server, in-memory mutex only; no file, DB or cross-process state.
Deterministic tests for concurrency, immediate repeats, completion/release, cooldown boundary, response headers	Done	All five are present. Determinism comes from a manually advanced clock plus a scan parked in the limiter's admission hook — no wall-clock sleeps in the assertions. [T] go test -race -count=20 -run PerfMem → ok, 0 flakes.
Race tests pass	Done	[T] go test -timeout 25m -race -count=1 ./... in cmd/server → ok, 393s.
Full server tests pass; existing /api/perf consumers compatible	Done	Full suites below. [A][T] Nothing in public/ requests mem=1, so the 429 is unreachable from the UI; the perf page keeps using plain /api/perf.

I found no issue point that was silently dropped, and no point parked in "Left for the owner" that should have been implemented.

Red before → green after

The author's report is honest that the test-only commit 97e28f8e does not compile on master (the type does not exist yet). I built a compiling before-state instead: master's cmd/server/routes.go (unmodified handlePerf) plus a bare perfMem perfMemLimiter field so the package links.

• [T] Before: 4 of 8 tests fail — ConcurrentRejectedBeforeStoreLock, RepeatCooldownBoundary, CooldownStartsAtCompletion, OrdinaryPerfUnaffected ("first mem=1 request was never admitted" / "status = 200, want 429").
• [T] After: ok, 1.8s.
• The remaining 4 (ReleasedBeforeResponseWrite, StorelessUnaffected, StaleReleaseIsNoop, ZeroValueDefaults) are invariants or unit tests of the new type, which is exactly how the author's table labels them. No overclaiming.

My own mutants (9, independent of the author's)

ID	Mutation	Result
X1	Gate moved out of the handler top into the breakdown block (after the store stats calls)	killed — burst blocked on the store write lock, 5s timeout
X2	Cooldown boundary left > 0 → left >= 0	killed — 4 tests fail, boundary request got 429/retryAfter:0
X3	until set at admission instead of at release	killed — CooldownStartsAtCompletion
X4	sync.Once removed → release not single-use	killed — "stale release freed the active scan's slot"
X5	setRetryAfter replaced by a floored int(wait.Seconds())	killed — Retry-After = "0", want "1" and the body/header mismatch
X6	mem == "1" check dropped → every /api/perf gated	killed — OrdinaryPerfUnaffected
X7	Explicit releaseMem() after the walk removed (defer only)	killed — ReleasedBeforeResponseWrite
X8	s.store != nil guard dropped	killed — StorelessUnaffected
P1	settings() default cd <= 0 → cd < 0	killed — ZeroValueDefaults
P3	429 → 503	killed — 3 tests
P4	rejected.Add(1) dropped in the active branch only	killed — rejected = 0, want 16
P2	defer releaseMem() removed	SURVIVED → N2 above

Boundaries measured against a real server

Local Go server built from the merged tree, e2e-fixture.db prepared as in CI (freshen → inline Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL → corescope-migrate → seed-2073, seed-199, seed-245), on a free high port, stopped afterwards by the port's listening pid.

Probe	Result [T]
GET /api/perf	200, no memoryBreakdown, no Retry-After
GET /api/perf?mem=1 (first)	200 with memoryBreakdown
immediate repeat	429, Retry-After: 5, Content-Type: application/json, Cache-Control: no-store, body {"error":"store memory breakdown is busy or cooling down","retryAfter":5}
mem=0, mem=true	200, no breakdown, no Retry-After
plain /api/perf during the cooldown	200, unaffected
?mem=0&mem=1 / ?mem=1&mem=0	200 / 429 — first-value Query().Get semantics, identical to master
cooldown countdown on the wall clock	t+1s → 429/4, t+2s → 429/3, t+3s → 429/2, t+4s → 429/1, t+6s → 200
16 concurrent mem=1	exactly 1× 200, 15× 429

Tests I ran on the merged tree

Suite	Result [T]
cmd/server go test -timeout 25m -race -count=1 ./...	ok, 393s
cmd/server go test -race -count=20 -run PerfMem	ok, 0 flakes
cmd/server go vet ./..., gofmt -l on the 4 touched Go files	clean
cmd/ingestor go test ./...	ok, 107s
sh test-all.sh	241 passed, 0 failed (241 files)
node test-frontend-helpers.js	713 passed, 0 failed
node test-xss-392-perf-e2e.js (the affected perf E2E)	1 passed, 0 failed
node test-e2e-playwright.js	132/135, 3 skipped — the same 3 pre-existing skips as on master (2 flaky-marked packets-detail tests, 1 Go-staging-only perf test)
test-perf-disk-io-1120.js / test-perf-go-runtime.js / test-perf-render-1258.js / test-perf-anomaly.js / test-xss-escape-sinks.js	15 / 30 / 3 / 3 / 121 passed, 0 failed
bash scripts/check-xss-sinks.sh --diff origin/master	exit 0 — "no public/**/*.{js,html} changes to scan"

CI on head 9f9e0540 (run 38384878638) [K]: Go Build & Test ✅ 24m56s, Playwright E2E ✅ 25m38s, Docker Image ✅ 51s; Release Artifacts / Publish Badges / Deploy Staging skipped by the fork guard. No red job, so no flake assessment was needed — neither of the known flaky tests appeared.

Invariants

Item	Result
cmd/server read-only	[A] No DB write and no new write lock; the limiter is an in-memory sync.Mutex plus two atomic.Uint64
New map[string]interface{} outside tests	[T] none in the diff
Hardcoded colours	[T] none — no frontend or CSS file is touched
check-xss-sinks.sh --diff origin/master	[T] clean
Fork guards	[T] 9 in deploy.yml, 1 in release-fast-path.yml; [T] the diff touches no workflow file
Closing keywords	[T] none; closingIssuesReferences is empty and the body opens "Relates to #371"
Commit author and committer	[T] dborup <kontakt@meshview.dk> on both commits
Scope creep	[T] 5 files, 555 insertions. The openapi.go churn is gofmt realignment forced by the longer map entry, nothing else. No new config knob, no auth change, no product decision taken silently.

Left for the owner

The author's four bullets (cooldown length, public vs API-key, process-local state, 429s counted under /api/perf in the perf stats) are each a real product decision and are correctly parked rather than implemented. Two deployment effects to add (N4):

• Any existing external mem=1 poller starts getting 429s. /api/perf is public, so a consumer outside this repo could be scraping mem=1 on an interval; after this change it gets one sample per scan + 5s and 429 otherwise. Nothing in public/ is affected [T], but an operator's own dashboard or exporter might be.
• The gate is shared by everyone, so the diagnostic itself becomes contended. A single unauthenticated client polling mem=1 once a cooldown wins the post-cooldown race most of the time, and an operator's manual request will often see 429 and have to retry. That is inherent in what the issue asks for (reject the excess) and is the same trade-off the "public vs API-key" bullet describes — worth stating explicitly rather than leaving implicit.

Not verified

• Storeless mem=1 on a real running binary — covered by a unit test only; the normal server wiring always has a store.
• Lock-hold time and walk cost on a production-sized store. The E2E fixture holds 3 transmissions, so the walk is instantaneous here; I confirmed the admission boundary, not the magnitude of the problem it protects against.
• Multi-instance behaviour (out of scope by the issue's own wording).
• The full Playwright matrix. I ran the main suite plus every test that touches /api/perf; the nav, gesture, a11y and map suites I left to CI [K].
• Staging and production: no access used, no API key used, no write to any upstream.