fix(server): deterministic tie order in four more count-only sorts (#321)

[adminopenclaw8-sketch]

Relates to #321

Follow-up to #319 (#273), same tie-order bug class as #256/#293: five lists are built by ranging over a map and were sorted on the count alone, so entries with equal counts came out in a different order on each call. Where the list is capped it was random which tied entries made the cut.

Plan

1. Write the tests first, one per site, each recomputing its list 50 times and requiring one fully specified order — so the pre-fix failure is deterministic, not flaky.
2. Give each comparator a final, unique tie-break key: the map key the entry was built from. Comparator-only, no extra lookups.
3. Kill one mutant per site by flipping that tie-break's direction.

Sites

Site	List	Cap	Tie-break added
node_analytics_summary.go finalizeDisplayArrays	observerCoverage	none	observer_id asc
node_analytics_summary.go finalizeDisplayArrays	peerInteractions	20	peer_key asc
store.go GetBulkHealth	observerRows	none	observer_id asc
store.go GetNodeHealth	observerRows	none	observer_id asc
store.go computeHashCollisions	collisions	none	prefix asc, after class order and appearances

peerInteractions is the visible one: with ties at the cut-off, the set of 20 peers the node-analytics page rendered changed between calls. Its test builds 30 peers tied on 3 messages plus one with 9 and pins the exact 20 that survive the cap.

Every tie-break key is the key of the map the entry was built from, so it is unique within the list and makes each order total. No new fields, no new lookups, no allocation — the comparators only read values already on each entry, so there is no perf change beyond one extra integer compare on a tie.

Frontend is untouched: public/node-analytics.js, public/live.js and the collisions view render whatever order the server sends.

Tests

New cmd/server/count_sort_tie_order_321_test.go, reusing assertSameEveryRun / tieOrderRuns (50) and mapField from the #256 suite. Each test seeds a tie with 12–30 entries whose insertion order is deliberately not their sorted order, so a count-only comparator cannot land on the expected sequence by accident.

All five tests fail on master on run 0 or 1 and pass on this branch.

[adminopenclaw8-sketch]

Rapport — CS-Macmini PR#325 #321 — head 72b5eb4

Status: All 5 sites fixed, comparator-only; 5 new tests red on master and green here, one mutant killed per site; CI all jobs green, no re-runs needed.

Evidence markers: [T] verified by a test run, [K] verified by reading the code, [A] analysis/argument only.

Requirements

#	Requirement	Fix	Test (red on master → green here)	Mutant
1	peerInteractions (cap 20) — which 20 peers are shown must be stable	peer_key asc after messageCount desc, node_analytics_summary.go finalizeDisplayArrays	TestNodeAnalyticsPeerInteractionsTieOrder_321 — 30 peers tied on 3 msgs + one on 9; pins the exact 20 that survive the cap and asserts len(want)==20 so the cap really binds. Master: run 0 got [peer-high peer-10e4 peer-1028 …] vs want [peer-high peer-1000 peer-1003 …] [T]	M2: flip tie-break to > → KILLED [T]
2	observerCoverage (no cap)	observer_id asc after packetCount desc, same function	TestNodeAnalyticsObserverCoverageTieOrder_321 — 12 observers tied on 4 + one on 9 + one on 2. Master fails on run 0 [T]	M1: flip tie-break to > → KILLED [T]
3	observerRows in GetNodeHealth	observer_id asc after packetCount desc, store.go	TestNodeHealthObserverRowsTieOrder_321 — same 12-way tie through the real GetNodeHealth over an in-memory DB + byNode packets. Master fails on run 0 [T]	M4: flip tie-break to > → KILLED [T]
4	observerRows in GetBulkHealth	observer_id asc after packetCount desc, store.go	TestBulkHealthObserverRowsTieOrder_321 — same shape through GetBulkHealth(10,"",""). Master fails on run 0 [T]	M3: flip tie-break to > → KILLED [T]
5	collisions (computeHashCollisions) — comparator change only	prefix asc as a third key after class order and appearances desc	TestHashCollisionsTieOrder_321 — six 2-byte prefixes, each shared by two coordinate-less repeaters, so every entry ties on both existing keys (incomplete, appearances=2). Master: run 1 got [1094 10B9 1000 …] vs want [1000 1025 104A …] [T]	M5: flip tie-break to > → KILLED [T]
6	Same tie-break pattern as rankSubpaths (#293) and #319	Every key is the map key the entry was built from, so it is unique within the list and the order is total. Shape matches #319 verbatim: if a != b { return a > b }; return key_i < key_j [K]	—	—
7	Deterministically red, not flaky	Each test recomputes 50× (tieOrderRuns, reused from the #256 suite) and requires one fully specified order; seed keys are inserted in a scrambled order ((i*37)%256) so a count-only comparator cannot land on the expected sequence by accident. All five fail on run 0 or 1 on master [T]	—	—
8	No perf regression	Comparator-only. The added clauses read PacketCount/MessageCount/packetCount/Appearances and ObserverID/PeerKey/observer_id/Prefix — all already present on each entry. No new lookups, no allocation, no extra map reads; the extra work is one integer compare plus one string compare, and only on a tie [K]	—	—

Always-checks

Check	Result
cmd/server read-only	No DB writes added; the diff touches only three sort.Slice comparators [K]
New map[string]interface{} outside tests	None — git diff | grep '^+' | grep 'map\[string\]interface{}' is empty [T]
Hardcoded colors	No frontend change at all; the diff is Go-only [K]
scripts/check-xss-sinks.sh --diff origin/master	no public/**/*.{js,html} changes to scan [T]
Fork guards	9 in deploy.yml, 1 in release-fast-path.yml — workflows untouched [T]
gofmt	clean on all three changed files [T]

Local verification

Suite	Result
go test ./... -count=1 in cmd/server	ok, 41.9s [T]
go test ./... -count=1 in the other 17 modules	all ok (ingestor, migrate, decrypt, anomaly, regions, prunequeue, channelregistry, dbschema, channel, packetpath, brokerurl, lora, sigvalidate, dbconfig; 3 with no test files) [T]
go test -run TieOrder -race	ok [T]
sh test-all.sh	225 passed, 0 failed (225 files) [T]
node test-frontend-helpers.js	709 passed, 0 failed [T]
E2E vs a local server on the CI-prepared e2e-fixture.db	test-e2e-playwright.js 132/135 passed, 3 skipped; test-issue-1306-collisions-terminology-e2e.js 23/0; test-issue-2027-my-mesh-node-page-e2e.js 25/0; test-home-coverage-e2e.js 12/0; test-issue-1151-orphan-separators-e2e.js 5/0 (the "Heard By" rows, i.e. GetNodeHealth observers); test-issue-1281-location-row-e2e.js 6/0; test-issue-1639-observers-sort-e2e.js all passed [T]

E2E selection: the five lists have no dedicated E2E of their own — a grep for peerInteractions / observerCoverage across test-*.js only hits public/node-analytics.js [T] — so I ran the suites that render the affected server output (collisions view, node detail "Heard By", node page, observers sort) plus the headline Playwright suite.

CI per job

Job	Result
✅ Go Build & Test	pass (19m06s)
🎭 Playwright E2E Tests	pass (25m46s)
🏗️ Build & Publish Docker Image	pass
📦 Release Artifacts	skipping (not a release)
📝 Publish Badges & Summary	skipping
🚀 Deploy Staging	skipping

No job needed a re-run. The known-unstable #271 test did not fire.

Residuals (not filed)

1. finalizeDisplayArrays has two more lists built from a map with no sort at all — packetTypeBreakdown (from typeBuckets) and uptimeHeatmap (from heatBuckets) [K]. Same bug class, not listed in Deterministic tie order in four more count-only sorts (node analytics, node/bulk health) #321, so left out of scope. uptimeHeatmap is the one that matters: the client has to bucket by dayOfWeek/hour itself, so a random order is survivable but the payload differs byte-for-byte on every call, which defeats any response-level caching or diffing.
2. The tie-breaks use a type assertion (.(string), .(int)) because NodeObserverStatsResp.ObserverID is interface{} and the health rows are map[string]interface{}. Safe here — both are populated from the string map key a few lines above — and it matches the pattern fix(server): deterministic tie order in GetSubpathDetail (#273) #319 landed, but it is a panic waiting for a future refactor that stores a non-string there. A typed struct for the health observer row would remove the whole class [A].
3. computeHashCollisions still leaves one_byte_cells / two_byte_cells node lists and inconsistent_nodes in getAllNodes() order [K]. That order is DB-query order, not map order, so it is stable within a process; it is not a tie-order bug and I did not touch it.

[dborup]

Review — CS-Minimax PR#325 — head 72b5eb4

Dom: APPROVE with nits

Independent, read-only review. Verified on the merge of head into origin/master b0b9843c (git merge-tree --write-tree → 2048057a, clean, no conflicts). git ls-remote on the branch was 72b5eb4c before and after the review. Evidence markers: [T] verified by running it, [K] verified by reading the code, [A] analysis/argument only.

Findings

#	Sev	Where	Finding
1	nit	count_sort_tie_order_321_test.go	Three of the five fixtures derive the display name from the key ("Name-" + id, "Peer-" + key), so name order equals key order. A tie-break on the name instead of the map key therefore passes every test: my mutant M6 (observer_id → observer_name, both health sites at once) SURVIVED, and M1c (ObserverID → ObserverName in observerCoverage) SURVIVED [T]. The shipped code uses the right key, but the suite proves only "order is total and stable", not "the key is the unique map key" — and a name is not unique, so that mistake would silently re-introduce the bug. Fix: give two fixture entries the same name, or names that sort opposite to the keys.
2	nit	store.go GetBulkHealth, GetNodeHealth	The PR body says "no extra lookups". For the two observerRows comparators that is not quite true: the non-tie path now does observerRows[i]["packetCount"].(int) twice per side (guard, then compare) where master did it once — 4 map lookups + 4 type assertions per comparison instead of 2 [K]. Complexity is unchanged (still one sort.Slice, O(n log n)), the lists are per-node observer counts (≤ ~15 in the e2e fixture), and this is verbatim the shape #319 already landed at topParents/topObs, so it is consistency rather than a regression. Hoisting ci, cj := would remove it; wording in the body could be "no extra map reads on the tie path".
3	info	store.go GetHashCollisions	/api/analytics/hash-collisions is memoised behind collisionCache with collisionCacheTTL = 3600s [K], so 20 consecutive calls were already byte-identical on master [T]. The collisions tie-order only surfaced across a cache refresh or a restart — consistent with #321 calling that site lower priority. Not a defect; worth knowing when judging impact.
4	info	node_analytics_summary.go finalizeDisplayArrays	Agreeing with the author's residual 1: packetTypeBreakdown (from typeBuckets) and uptimeHeatmap (from heatBuckets) are built from a map with no sort at all [K]. Same bug class; not in #321's table, correctly left out of scope. packetTypeBreakdown is rendered straight into a chart, so its category order changes on every call.
5	info	both files	The .(string) / .(int) assertions are safe by construction: each is read from a value the same function literal-assigned from a map[string]... key a few lines above (obsDetail/observerStats/prefixMap are all map[string]), and each sort.Slice runs on a slice built locally in that function — no caller can inject a non-string [K]. Agreeing with the author's residual 2 that this is a latent trap for a future refactor, not a live one.

No blocking finding. Nothing else in the diff.

The review points

1. Each site has a test, red on master deterministically, green on head; one mutant per site.
All five tests are red on master and green on head, and the redness is not probabilistic: with the test file dropped onto origin/master unchanged, go test -run TieOrder_321 -count=5 fails all five tests in all five repetitions (25/25 test runs red), each failing inside the 50-iteration loop (run 0 or run 1, once run 4) [T]. On the merged tree all five pass, including -race -count=2 [T].

My own mutants, one per site, deliberately different shapes from the author's sign flips:

Mutant	Site	Change	Result
M1b	observerCoverage	tie-break → PacketCount < PacketCount (a no-op inside the tie branch)	KILLED [T]
M2	peerInteractions	move the [:20] cap before the sort	KILLED [T]
M3	GetNodeHealth observerRows	tie-break < → > (this site only)	KILLED — and only TestNodeHealthObserverRowsTieOrder_321 failed, TestBulkHealth… stayed green [T]
M4	GetBulkHealth observerRows	tie-break clause deleted, back to count-only (this site only)	KILLED — and only TestBulkHealthObserverRowsTieOrder_321 failed [T]
M5	collisions	tie-break → ByteSize (constant inside one size bucket, so a no-op)	KILLED [T]

M3/M4 landing on exactly one test each confirms the two health sites are independently guarded, not covered by one shared assertion. Two further probes (M1c, M6) survived — see finding 1.

2. peerInteractions: is which 20 survive the cap stable?
Yes. TestNodeAnalyticsPeerInteractionsTieOrder_321 puts the cut-off inside the tie — 30 peers on 3 messages plus one on 9, so 19 of the 30 tied peers make the cut — pins the exact 20 peer_keys, and asserts len(want) == 20 so the cap really binds [T]. M2 (cap before sort) is killed by it [T]. End to end against a local server on the CI-prepared e2e-fixture.db, 20 consecutive calls to /api/nodes/{pubkey}/analytics?days=30 returned one single observerCoverage+peerInteractions projection on head versus 20 distinct orders on a b0b9843c build [T].

3. Is the secondary key meaningful and consistent with #293/#319?
Yes, and I checked each key really is the map key rather than something that merely looks like one [K]:

• observerCoverage.ObserverID ← for id, o := range a.obsDetail (map[string]*nodeAnalyticsObsAccum) — the observers row id;
• peerInteractions.PeerKey ← pm.key, and peerDetail[c.key] is only ever created with key: c.key, so it equals the map key — a peer pubkey;
• both observerRows → "observer_id": id from for id, o := range observerStats;
• collisions.Prefix ← for prefix, pnodes := range prefixMap, and collisions is re-declared per byte-size bucket, so the prefix is unique within each sorted slice.

Each is unique within its list, so every comparator is now a total order. All are identifiers persisted in the DB (observer id, node pubkey, hex prefix), not per-process values, so the order survives a restart as well as repeated calls. The comparator shape is the if a != b { return a > b }; return key_i < key_j form already used by #319 at GetSubpathDetail, topParents and topObs.

4. Perf.
Comparator-only: no field added to any struct, no map or DB lookup outside the comparator, no allocation, the same single sort.Slice per site, so still O(n log n) [K]. See finding 2 for the one imprecision in the "no extra lookups" claim — it does not change the complexity and is the established pattern here.

5. Scope.
The diff is three files: two comparators in node_analytics_summary.go, three in store.go, plus the new test file (+259/−5). No frontend change, so no colour literal and no new XSS sink; scripts/check-xss-sinks.sh --diff origin/master reports no public/**/*.{js,html} changes to scan [T]. No INSERT/UPDATE/DELETE/Exec/Begin in any added line — cmd/server stays read-only [T]. No new map[string]interface{} outside tests [T]. The three consumers render the server order verbatim (public/node-analytics.js peer table and observer bar chart, public/live.js "Heard By" list), and no server-side caller indexes these lists positionally [K], so the only behaviour change is the order of ties.

Always-checks: fork guards 9 in deploy.yml and 1 in release-fast-path.yml, workflows untouched [T]; no closing keyword in the commit message or the PR body (both say "Relates to #321") [T]; commit author and committer are dborup <kontakt@meshview.dk> [T]; gofmt clean on all three files [T].

Tests I ran

On the merged tree (head into origin/master b0b9843c):

Suite	Result
go test ./... -count=1, cmd/server	ok 45.8s [T]
go test ./... -count=1, the other 17 modules	all ok (ingestor 108.8s, migrate, decrypt, anomaly, brokerurl, channel, channelregistry, dbconfig, dbschema, lora, packetpath, prunequeue, regions, sigvalidate; geofilter/mbcapqueue/perfio have no test files) [T]
go test -run TieOrder -race -count=2	ok [T]
sh test-all.sh	225 passed, 0 failed (225 files) [T]
node test-frontend-helpers.js	709 passed, 0 failed [T]

E2E against a local Go server on e2e-fixture.db prepared exactly as CI does (tools/freshen-fixture.sh, the Kpa-clawbot#1486 and Kpa-clawbot#1791 seed SQL, corescope-migrate, then seeds 2073, 199 and 245, instrumented frontend). The PR adds no E2E of its own — the diff is Go-only — so I ran the suites that render the five affected lists plus the headline suite:

Suite	Result
test-e2e-playwright.js	132/135 passed, 3 skipped, 0 failed [T] (see the note below)
test-issue-1306-collisions-terminology-e2e.js	23 passed, 0 failed [T]
test-issue-1151-orphan-separators-e2e.js ("Heard By" rows = GetNodeHealth observers)	5 passed, 0 failed [T]
test-issue-1147-section-order-e2e.js ("Heard By" section)	2/2 passed [T]
test-issue-1281-location-row-e2e.js	6 passed, 0 failed [T]
test-issue-1639-observers-sort-e2e.js	all passed [T]
test-issue-2027-my-mesh-node-page-e2e.js	25 passed, 0 failed [T]
test-home-coverage-e2e.js	12 passed, 0 failed [T]
test-node-liveness-e2e.js	35 checks passed [T]

Note on the headline suite: on the first run it stopped fail-fast at "Version info lives on Perf dashboard, not in navbar" (page.waitForFunction on #navStats timing out after 10s). That is a local-environment artifact, not this PR: a server built from b0b9843c with the same fixture and the same browser fails at the identical assertion [T], and #navStats does fill correctly ("500 pkts · 204 nodes · 31 obs", no console errors) when driven on its own [T]. With that one assertion skipped in my local copy, the remaining 132 pass with 0 failures — the same numbers CI reports.

Extra end-to-end determinism probe, 20 consecutive calls each, head vs a b0b9843c build on the same fixture (115 of 200 bulk-health rows have tied observer counts, so the ties are real fixture data, not synthetic) [T]:

Endpoint	distinct results on master	on head
/api/nodes/{pubkey}/health observers	20	1
/api/nodes/bulk-health?limit=200 observers	20	1
/api/nodes/{pubkey}/analytics observerCoverage+peerInteractions	20	1
/api/analytics/hash-collisions	1 (1h response cache — finding 3)	1

CI, per job

Run on head 72b5eb4c, run_attempt=1, no re-runs, and it is the only run for this sha [T]:

Job	Result
Go Build & Test	pass, 19m06s — --- FAIL count 0 in the job log [T]
Playwright E2E Tests	pass, 25m46s [T]
Build & Publish Docker Image	pass, 54s [T]
Release Artifacts / Publish Badges & Summary / Deploy Staging	skipped (not a release, not master) [T]

Neither known-flaky test fired: there is no --- FAIL and no FAIL<tab>github… line anywhere in the Go job log, so the Hash-Stats sort flake (#256) and the backfill write-hold flake (#267) both stayed green [T].

Not verified

• No staging or production check, and no API key used — local only, as scoped.
• I did not measure the comparator cost (finding 2 is read from the code, not benchmarked), and I ran no benchmark against master.
• The three skipped assertions in the headline suite are fixture-shape skips ("no multi-hop packet with byte breakdown in first 15 rows" etc.), unrelated to this change; I did not extend the fixture to cover them.
• One assertion in the headline suite (Version info lives on Perf dashboard) was skipped in my run after reproducing the identical failure on b0b9843c; I did not root-cause why it times out locally.
• The hash-collisions site was only exercised through its 1h-cached endpoint end to end; its per-call determinism is covered by the unit test, not by a cache-busting HTTP probe.
• uptimeHeatmap and packetTypeBreakdown (finding 4) are reported from reading the code; I wrote no test for them, as they are outside this PR's scope.
• The PR is still in draft; I reviewed the branch state only and made no change to the PR.