[dborup-agent]

Relates to #548

Follow-ups from the #542 review for loadCanonicalResolvedPathsAt (cmd/server/resolved_index.go). cmd/server stays read-only; no new map[string]interface{}.

Plan (small, bounded; tests written first)

#	Change	Test
N1	Document the intended order: ascending id, so when one request inserts past lruMaxSize the highest ids survive	EvictionOrderPastCapacity (10050 txs, primary and fallback)
N2	Pin that an absent row / NULL resolved_path is not cached	AbsentAndNullNotCached
N3	Pin chunk determinism for both sort.Ints calls	ChunkInsertOrderAscending, FallbackListSortedFromCachedEmpty
N4	longestTx becomes map[int][]int; missing queued once per id	SharedLongestObservation
N5	New lruPutBatch: one lruMu hold per chunk (primary and fallback pass) instead of one per id	LRULockedOncePerChunk

N4 and N5 tests were red before the code change; N1-N3 pin existing behaviour and are proven by mutants (see the report comment).

Performance

Hot path is /api/nodes/{pk}/paths. Complexity is unchanged: O(n log n) for the sorts, O(n) puts. Lock acquisitions for 5000 cold ids drop from 5000 to 10 (500 per chunk). Contended benchmark (BenchmarkCanonicalPaths548LRUContention, 4 readers on lruMu): about 36-41 ms/op per-chunk vs 40-44 ms/op per-id (noisy). Uncontended, BenchmarkCanonicalPaths2224 is flat to slightly slower (~+20 allocs per chunk for the batch slices); an uncontended mutex costs about 20 ns, so there is no gain without contention.

Config / customizer

No new configurable values.

[dborup-agent]

Rapport — CS-pve-agent2 PR#552 #548 — head 6c9d69c

Status: All five items implemented with tests; CI green on every job; draft, not merged.

Evidence tags: [T] = test or measurement run, [A] = artifact or diff inspection, [K] = known or reasoned, not run.

Requirements, tests and mutants

Req	Test (cmd/server/paths_lru_548_test.go)	Red before?	Mutant (killed unless noted)
N1 eviction order past lruMaxSize	EvictionOrderPastCapacity (10050 txs, primary + fallback): lowest 50 ids evicted, highest survive	Green before: pins existing order [T]	Remove sort.Ints(missing) killed; descending sort killed [T]
N2 absent / NULL not cached	AbsentAndNullNotCached	Green before: pins existing guard [T]	Drop the present guard killed (also by 2 other tests) [T]
N3 chunk determinism	ChunkInsertOrderAscending, FallbackListSortedFromCachedEmpty	Green before: pins existing sorts [T]	Remove sort.Ints(missing) killed; remove sort.Ints(txIDs) killed. The first fallback test alone did not kill the second sort, so the second test was added [T]
N4 longestTx safe	SharedLongestObservation	Red before (tx 1 lost its path) [T]	Revert to a single-tx mapping killed [T]
N5 one lruMu hold per chunk	LRULockedOncePerChunk (primary + fallback)	Red before (reader saw 2 of 6 mid-chunk) [T]	Per-id locking in lruPutBatch killed [T]

N5 measurement

• Lock acquisitions for 5000 cold ids: 5000 → 10 [K, follows from the test and the 500 chunk size].
• Contended benchmark, 4 readers on lruMu (BenchmarkCanonicalPaths548LRUContention): about 36-41 ms/op per-chunk vs 40-44 ms/op per-id. Noisy, roughly 10% [T].
• Uncontended BenchmarkCanonicalPaths2224 batch: flat to slightly slower (e.g. 30000 tx, no fallback: ~90-125 ms before, ~112-125 ms after; +~30 allocs per chunk for batch slices). There is no gain without contention [T].

CI (run 40087188500), per job

All pass: Go Build & Test, Go Server Tests 1/3, 2/3, 3/3, Go Ingestor Tests, Go Libraries & CLI, JavaScript Unit Tests & Lint, Script & Container Unit Checks, Playwright E2E 1/3, 2/3, 3/3, Validate Docker Image, Verify E2E Shards & Coverage, Verify Go Test Assignments, Build & Publish Docker Image [T]. Publish Badges, Release Artifacts and Deploy Staging were skipped by the workflow. The flaky test #271 did not fail; no re-run was needed.

Local checks

• go test ./... in cmd/server (30m timeout) and cmd/ingestor: pass [T].
• -race on TestNoSQLUnderStoreLock and on Canonical/Paths/LRU/Resolved/Eviction/Backfill subsets: pass [T].
• sh test-all.sh: 265 passed, 0 failed. node test-frontend-helpers.js: 719 passed [T].
• Local Go server on a prepared copy of the fixture DB: /api/nodes/{pk}/paths returned 50 paths; server stopped by pid [T].
• scripts/check-xss-sinks.sh --diff origin/master: no frontend changes to scan [T]. No new map[string]interface{} in the diff [A]. No colors touched [A].
• Only cmd/server/resolved_index.go and the new test file changed; workflows are untouched, so the fork guards are unchanged [A].

Remaining

• Full-suite -race on cmd/server hit Go's default 10 min timeout in an unrelated startup test (TestHotStartup_ConcurrentQueryDuringBackgroundLoad). I did not re-run it with a longer timeout; CI does not run the server suite with -race [T].
• Playwright was not run locally (not installed); the three CI E2E shards passed [T].
• The fallback pass (loadFallbackResolvedPathsAt) also uses lruPutBatch; the issue only named the main chunk loop.

[dborup]

Review — CS-Minimax PR#552 — head 6c9d69c

Dom: APPROVE med nits

Evidence tags: [T] = test or measurement I ran, [A] = diff/artifact inspection, [K] = reasoned, not run. Review is read-only; everything ran in scratch exports (git archive of the head and of the merge tree with origin/master ff1acc02). Remote head was 6c9d69cc before and after.

Findings

#	Severity	Finding
1	nit	The author's contended benchmark claim (~10%) understates what I measured: 4 spinning readers, -benchtime 20x -count 4, master code vs head: ~22.3-23.2 ms/op → ~14.3-15.3 ms/op (about 35% faster), allocs 79.8k → 84.8k (+6%), B/op 3.66 MB → 4.05 MB (+10%) [T]. The direction matches the PR text; the number should be updated or left as "noisy". Uncontended cost was not re-measured by me [K].
2	nit	TestCanonicalPaths548LRULockedOncePerChunk relies on a 50 ms sleep to let the reader queue. It cannot fail for correct code (a late reader still sees all n entries), but a very slow scheduler could let the per-id mutant pass. Acceptable; it killed the mutant on every run here [T].
3	nit	Behaviour change beyond pure refactor is exactly N4: when several txs share a longest observation id, all are now answered (before: only the last). This is the intended, issue-listed change and is pinned by a test that is red on master [T].
4	info	The fallback pass now also batches (lruPutBatch); the issue only named the main loop. Covered by the fallback sub-tests, no behaviour difference [A][T].

No blocking findings.

Answers to the points

• LRU order test pins the intended order. EvictionOrderPastCapacity seeds 10050 txs (primary and fallback variants), asserts the LRU holds exactly lruMaxSize, the 50 lowest ids are gone and the 50 highest remain. Removing sort.Ints(missing) and replacing it with a descending sort both fail it (and ChunkInsertOrderAscending) [T].
• NULL/absent stay out of the LRU. AbsentAndNullNotCached covers a NULL longest row (sibling supplies the fallback), a plain hit, and an absent row. Mutants: guard present forced true, present || true, and including NULL rows in stored (raw.Valid → true): all killed by this test [T]. The stored map only gets valid rows, so "present" already means non-NULL [A].
• Determinism test has teeth. Removing sort.Ints(missing) is killed by two tests; removing sort.Ints(txIDs) in the fallback pass is killed only by FallbackListSortedFromCachedEmpty (the first test alone would not do it, as the author reported, and I confirmed the second test is what kills it) [T].
• N4 longestTx structurally safe. map[int][]int plus a queued check, so missing has each id once and every tx gets answered. Mutant reverting to a single-tx mapping is killed; the new test is red on master's resolved_index.go [T].
• N5 lock ordering and SQL. lruPutBatch takes only lruMu, runs no SQL, and is called from loadCanonicalResolvedPathsAt / loadFallbackResolvedPathsAt, which run after s.mu is released (documented contract, unchanged) so s.mu → lruMu order is not touched [A]. TestNoSQLUnderStoreLock passes under -race [T]. The SQL query, scan and rows.Close() all complete before lruPutBatch is called [A]. Mutant putting Lock/Unlock back per id inside lruPutBatch is killed by LRULockedOncePerChunk, which is red on master [T].
• Measurement is real. See finding 1 [T]. The "5000 → 10 acquisitions" figure follows from chunk size 500 [K].

Acceptance criteria → red-before / green-after

Criterion	Test	Red on master code?
N1	EvictionOrderPastCapacity	No (pins existing order); mutants kill it
N2	AbsentAndNullNotCached	No (pins existing guard); mutants kill it
N3	ChunkInsertOrderAscending, FallbackListSortedFromCachedEmpty	No (pins existing sorts); mutants kill them
N4	SharedLongestObservation	Yes [T]
N5	LRULockedOncePerChunk	Yes [T]

Running the 548 tests against origin/master's resolved_index.go failed exactly SharedLongestObservation and LRULockedOncePerChunk.

Tests and mutants (my own runs, merged tree)

• go vet ./... in cmd/server: clean [T].
• go test -race -count=1 -run 'Canonical|Paths|LRU|ResolvedPath|NoSQLUnderStoreLock|548|2224' in cmd/server: pass [T].
• go test -count=1 ./... in cmd/server: pass (127 s). cmd/ingestor: pass (144 s) [T].
• sh test-all.sh: 266 passed, 0 failed. node test-frontend-helpers.js: 719 passed, 0 failed [T].
• E2E: local Go server on port 13700 with e2e-fixture.db prepared as in CI (freshen, grouped-packet seed SQL, corescope-migrate, seeds 2073 and 199, .git-commit file): test-e2e-playwright.js 154/157 passed, 3 skipped, 0 failed. /api/nodes/{pk}/paths returns paths on this tree. Server stopped by port [T]. I ran the non-instrumented public/, not public-instrumented.
• Mutants (all killed): M1 remove sort.Ints(missing); M2 drop the present guard (two variants); M2c cache NULL rows; M3 remove sort.Ints(txIDs); M4 per-id locking in lruPutBatch; M5 longestTx last-wins; M6 descending sort [T].
• CI per job on run 40087188500: all required jobs pass (Go Build & Test, Go Server 1/3-3/3, Go Ingestor, Go Libraries & CLI, JS Unit & Lint, Script & Container Checks, Playwright 1/3-3/3, Validate Docker Image, Verify E2E Shards, Verify Go Test Assignments, Docker publish); Badges, Release and Deploy Staging skipped by the workflow. No flaky-test (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) failures [T].

Hygiene

• Only cmd/server/resolved_index.go and the new test file change; cmd/server stays read-only (no write path added) [A].
• New map[string]interface{} in the diff: 0 [A]. No colors/frontend touched; scripts/check-xss-sinks.sh --diff origin/master reports nothing to scan [T].
• .github/ is untouched, so fork-guard counts are unchanged [A]. I did not independently recount the "9 and 1" figures beyond that.
• PR body uses "Relates to paths: follow-ups from #542 (LRU order/NULL pinning tests, per-chunk lruMu) #548"; no closing keywords in the body or the commit message [A].
• Commit author and committer: dborup <kontakt@meshview.dk> [A].

Not verified

• Full-suite -race on cmd/server and cmd/ingestor (only the relevant subset; the author reports the full-suite race run hits Go's default timeout in an unrelated startup test).
• Uncontended performance regression numbers (author reports flat to slightly slower).
• Instrumented-frontend E2E and the CI shard split; I relied on the green CI shards for those.
• Behaviour on staging/production data volumes.