Repository navigation
My Mesh: accurate 24h node activity and compact repeater observer cards - #351
Conversation
There was a problem hiding this comment.
Independent review - one actionable finding:
[P2] Unlimited-retention stores can never show the 24-hour chart (cmd/server/node_activity_24h.go, line 75). loadBackgroundChunks returns immediately when retentionHours <= 0 (cmd/server/store.go, line 1679) after setting backgroundLoadDone, but it never computes loadCoverageRatio; that field remains zero. The new coverage gate therefore always returns unknown, and home.js renders Activity unavailable indefinitely, even after LoadChunked loaded the complete database. This is a documented configuration: retentionHours = 0 means unlimited. Please derive proof of coverage for that load mode separately, or explicitly handle the unlimited-retention path, and add a regression test.
The observer preview/dialog, HTML escaping of observer names, rolling bucket boundaries, and fixture coverage otherwise look sound in this diff.
|
Review feedback addressed (commit
|
dborup
left a comment
There was a problem hiding this comment.
Follow-up review of c8c14f4: the unlimited-retention finding is resolved. For hotStartupHours >= 24, the revised gate accepts the proven hot window and still checks oldestLoaded; for shorter hot windows it correctly leaves coverage incomplete. The hotStartupHours = 0 path computes loadCoverageRatio in RunStartupLoad after the full synchronous load, so it remains covered by the ordinary gate. Focused node-activity Go tests passed. No remaining findings from my review.
|
Taking this PR over now (the author has finished). Next: an independent review. |
Review — CS-Minimax PR#351 — head b039258Dom: REQUEST CHANGES Independent read-only review. Evidence: [T] = I executed it, [A] = analysis, [K] = read in code/docs. Everything was run on the merged tree ( Findings
Finding 1 in detail[T] I ran the real
In the first row the in-memory index demonstrably reaches back seven days — far past [T] Reproduced end to end in a browser, too. A local Go server on the CI-prepared [K] [A] Blast radius: broken whenever Finding 2 in detail[T] Browser, local server, light and dark, 1280×900 and 375×812: a repeater card with one observer named The PR description's claims
#304 acceptance criteria
Always-checks
Tests I ran (merged tree, master
|
| Suite | Result |
|---|---|
cd cmd/server && go test -race -count=1 -timeout 45m ./... |
[T] ok, 817.9s |
cd cmd/ingestor && go test -count=1 -timeout 45m ./... |
[T] ok, 113.0s |
go vet ./... in both modules |
[T] clean |
sh test-all.sh |
[T] 232 passed, 0 failed (232 files) |
node test-frontend-helpers.js |
[T] 709 passed, 0 failed |
CHROMIUM_REQUIRE=1 node test-issue-304-my-mesh-e2e.js |
[T] pass |
CHROMIUM_REQUIRE=1 node test-issue-2027-my-mesh-node-page-e2e.js |
[T] 25 passed, 0 failed |
BenchmarkNodeActivity24h30K -benchtime=10x |
[T] 1.355 ms/op (M2 Pro) |
Browser: local Go server on the CI-prepared e2e-fixture.db, My Mesh home with two repeater cards, light + dark, 1280×900 + 375×812 |
[T] no console errors, no page errors, no failed requests, no horizontal overflow; dialog theme-aware and inside the viewport (341 px at 375); card heights 241–286 px |
Browser, second server with 30 h of history so complete is true |
[T] 24 bars render with per-bucket title/aria-label, still no console errors |
Servers were started on free ports from 13700 up and stopped by looking the listener up by port.
Red before: [T] the new E2E run against master's public/ fails at its first new assertion (.mnc-observer-name count 0 !== 3). The Go tests cannot compile against master (new symbols), so their red-before state is established by the mutants below instead.
One-off: [T] TestHandleNodePaths_PrefixCollision_1352_FallbackBranch failed once in a non--race full run while several suites, two servers and a browser were running concurrently. It then passed 12/12 on the merged tree and 12/12 on master's tree, and the clean -race full run never hit it. Treating it as load-induced, not PR-related. Neither known flake (#256, #267) appeared.
My mutants (8; 7 killed, 1 survived)
| # | Mutant | Result |
|---|---|---|
| M1 | add() counts observations instead of unique transmissions |
killed — TestNodeActivity24hRollingBoundariesAndUniqueTransmissions [T] |
| M2 | half-open window end becomes inclusive (when.After(a.end)) |
killed — same test, via index out of range [24]; the exclusivity check is load-bearing for bounds safety [T] |
| M3 | a.Complete = true whenever coverage is known |
SURVIVED the whole cmd/server suite — finding 3 [T] |
| M4 | frontend renders bars even when complete === false |
killed — E2E (24 !== 0) [T] |
| M5 | observer preview not limited to three names | killed — E2E (51 !== 3) [T] |
| M6 | drop the retentionHours < 24 guard |
killed — TestNodeActivity24hCoverageRequiresLoadedHistory [T] |
| M7 | dialog list uses innerHTML |
killed — E2E (dialog img count 1 !== 0) [T] |
| M8 | inline preview name not escaped | killed — E2E (svg count 1 !== 0) [T] |
CI
[T] Run 37610399888, pull_request, head b0392585, attempt 1: Go Build & Test pass (24m31s), Playwright E2E pass (24m18s), Build & Publish Docker Image pass (59s); Release Artifacts, Deploy Staging and Publish Badges skipped (tag-/push-/fork-gated). The two earlier runs on 4130188f and c8c14f46 were cancelled by the next push. [A] CI ran the branch as-is, 45 commits behind master at the time, not the merged tree.
Not verified
- No staging or production validation, and no server API key used — out of scope by instruction.
- Whether a real deployment sets
hotStartupHours > 0. The repo only shipsconfig.example.json(168/0), which is the configuration finding 1 breaks; I could not confirm what the target instance runs, and that determines whether finding 1 is already live. - The chart's visual appearance beyond the DOM/geometry assertions and the screenshots I took — I did not do a pixel diff.
- Screen-reader behaviour of 24
role="img"bars; I checked only that the labels are present and escaped. GetBulkHealthand the otherrecentPacketsconsumers (nodes.js,live.js,observer-detail.js) were read but not re-tested; the API change is additive and they are untouched.- The remaining My Mesh: make repeater cards compact and fix misleading activity and health metrics #304 criteria (status evidence, SNR attribution, traffic semantics, advert intervals) — the PR does not claim them.
…lth (#351 review F1,F3,F4,F5,F6) F1 (blocking): RunStartupLoad's hotStartupHours==0 branch measured loadCoverageRatio against a whole-table COUNT(*), so on the shipped default (retentionHours=168, hotStartupHours=0) with a DB holding more than 168h of history the ratio was structurally below 1 and the 24h activity gate never proved completeness — the chart showed "Activity unavailable" forever. Divide by the rows LoadChunked actually targeted (the retention/hot-start-filtered count it already computes), matching docs/api-spec.md and the background-fill path. New Test351F1StartupCoverageHonoursRetentionWindow covers BOTH startup paths (hotStartupHours==0 and >0). F3 (major): add TestNodeActivity24hKnownButShorterCoverageIsIncomplete — coverage that is known but shorter than 24h must read as incomplete, not as confirmed silence. Kills mutant M3 (unconditional Complete=true), which survived the whole suite before. F4 (nit): read the bgErrMu-guarded coverage ratio in GetNodeHealth BEFORE taking s.mu and pass it into nodeActivityCoverageStartLocked, so the store never nests s.mu -> bgErrMu (bgErrMu exists precisely so readers avoid s.mu). F5 (nit): drop the getNodeHealthAt extraction; inject now into GetNodeHealth directly so no second untyped-map-returning signature is added (AGENTS.md). F6 (nit): skip the per-packet RFC3339Nano parse for transmissions that sort below the window start (the common case for long in-memory history). Only the lower bound is string-filtered — the upper bound carries now's sub-second precision and cannot be compared safely as a string. Guarded by TestNodeActivity24hAddPreFilterKeepsBoundaryPackets. Relates to #304
…review F2) F2 (blocking): .mnc-observer-name clips at 14ch, but the full name was only recoverable on repeater cards with >3 observers (the View all dialog). On a repeater with 1-3 observers, and on every non-repeater card, a long name was clipped with no tooltip and no disclosure — the name was lost. #304 requires the full list reachable "without losing names". Each observer span now carries a title with the full name (pointer + assistive tech), and the accessible dialog is offered whenever the preview hides names OR any shown name is clipped — on all card roles, not just repeaters with >3. E2E: mixed card roles (repeater with 51/1/0 observers, a <=3-observer repeater with a clipped name, and a non-repeater with a clipped name) at 1280x900 and 375x812; assert title present, name clipped, dialog offered, and the full name recoverable via the dialog, with cards staying compact. Kills the mutant that restores the old "clipped with no recourse" behavior. Relates to #304
Rapport — CS-MacBook PR#351 runde-2 — head 43ba651Review feedback addressed (commit Findings
Not changed: F7 (info — unconditional Tests & mutants
F4/F5 are structural (no behavior change); covered by the existing Suites [T]
CI per job (run 37661809142, head 43ba651, pull_request)
Attempt 1 failed on the Notes
|
Review — CS-pve-agent2 PR#351 — head 43ba651Dom: REQUEST CHANGES This is an independent, read-only re-review of round 2 (the commits I ran everything on a F1, F3, F4 and F5 are resolved. F6 is resolved to nit level. F2 is only partly resolved. A name that is visually clipped but is 14 characters or shorter still gets no dialog. On a touch device, the only way to read it would be the Findings
Answers to the round-2 checklistF1 — coverage source in both startup paths: resolved. [K] [T] I ran real servers on a fixture copy with 150 rows aged 10 days past a 168 h retention window, 349 retained rows spread over about 6.5 days,
[T] Red-before: mutant O1 restores the whole-table F2 — observer names recoverable on all cards: partly. [T] Every observer span now has an escaped F3 — mutant M3: dies. [T] With F5 — no new F4 — lock order: avoided. [K] F6 — Always-checks
Tests I ran (merged tree
|
| Suite | Result |
|---|---|
cd cmd/server && go test -count=1 ./... |
[T] ok (529 s) on a clean run. The first run happened while browsers, mutant runs and benchmarks were loading this 4-core host. It failed once in Test1690_BackgroundLoadHonesty (638 s, 5000/5000 packets loaded). That test then passed 3/3 in isolation (568, 376 and 298 s), and the clean full run is green. See below. |
cd cmd/ingestor && go test -count=1 ./... |
[T] ok (1807 s, under the same concurrent load) |
sh test-all.sh |
[T] 233 passed, 0 failed (233 files) |
node test-frontend-helpers.js |
[T] 709 passed, 0 failed |
CHROMIUM_REQUIRE=1 node test-issue-304-my-mesh-e2e.js |
[T] pass |
CHROMIUM_REQUIRE=1 node test-issue-2027-my-mesh-node-page-e2e.js |
[T] 25 passed, 0 failed |
test-home-coverage-e2e.js against a local server |
[T] 12 passed, 0 failed |
test-a11y-axe-1668.js against a local server (includes /home with the new <dialog>) |
[T] net=0 across 120 cells |
Browser, local Go server on CI-prepared e2e-fixture.db (freshen, seed SQL, corescope-migrate, seeds 2073/199/245) |
[T] served; every complete: false (R6) |
| Browser, F1 fixture copy (table above), 3 repeater cards, 1280×900 + 375×812, light + dark | [T] see F2 answer; R1 repro with substituted names |
Servers ran on free ports 13711–13713 and were stopped by looking up the listener by port. Neither known flaky test (#256, #267) appeared. [A] I do not attribute the single Test1690_BackgroundLoadHonesty failure to this PR. This PR changes nothing in LoadChunked's memory cap or in loadBackgroundChunks; the only change in LoadChunked is the added s.loadWindowTotalInDB assignment. Earlier reviews on these hosts (#264, #266, #313, #320) recorded the same test as I/O-bound and very slow under load, and it was green in 4 of 5 runs here. It is not one of the known flaky tests, so I'm listing it rather than hiding it.
Mutants
| # | Mutant | Result |
|---|---|---|
| M3 | a.Complete = true whenever coverage is known |
killed — TestNodeActivity24hKnownButShorterCoverageIsIncomplete [T] |
| O1 | RunStartupLoad back to whole-table COUNT(*) |
killed — Test351F1StartupCoverageHonoursRetentionWindow [T] |
| O3 | ignore memory-cap eviction (packets[0] lower bound) |
killed — TestNodeActivity24hCoverageRequiresLoadedHistory [T] |
| O6 | pre-filter < → <= (drops the exact-start packet) |
killed — three tests incl. …AddPreFilterKeepsBoundaryPackets [T] |
| O7 | coverage gate < 1 → < 0.9 |
killed — TestNodeActivity24hCoverageRequiresLoadedHistory [T] |
| O8 | drop the backgroundLoadFailed guard |
killed — same test [T] |
| O4 | dialog only when names are hidden (round-1 behaviour) | killed — E2E card 3 must offer the observer dialog [T] |
| O5 | drop title from the observer span |
killed — E2E (title null) [T] |
| O9 | title="${obsFullName(o)}" without escapeAttr |
survived E2E and XSS gate — R2 [T] |
All mutants were applied in a separate scratch copy and restored afterwards; I checked each restore with cmp against the merged tree.
CI
[T] Run 37661809142 (pull_request, head 43ba651e). In attempt 2, Go Build & Test, Playwright E2E and Build & Publish Docker Image succeeded. Release Artifacts, Deploy Staging and Publish Badges & Summary were skipped (gated). In attempt 1, Go Build & Test failed only at Verify proto syntax (5-minute timeout during protoc install); its Go test steps had passed (R5). [A] CI tests the GitHub PR merge ref, not a re-merge onto today's master. My merged-tree runs above cover that.
Not verified
- No staging or production check, and the server API key was not used, as instructed.
- I did not determine which
packetStoreconfig the deployed instance uses. F1 is verified for bothhotStartupHourssettings withretentionHours: 168.retentionHours: 0withhotStartupHours: 0relies on the existing whole-table ratio, which was correct before this round. - Font-dependence of R1 beyond Chromium's default sans on Linux. Other platforms' fonts change exactly which names clip, which is why a measured check is preferable to a character count.
- Screen-reader behaviour of the 24
role="img"bars and of the<dialog>, beyond axe and the labels being present. - My Mesh: show estimated zero-hop and flood advert intervals on repeater cards #352, which is stacked on this branch, was not reviewed. The remaining My Mesh: make repeater cards compact and fix misleading activity and health metrics #304 criteria (status evidence, SNR/hops labels, previous-period comparison, advert intervals) are outside this slice and the PR doesn't claim them.
…e escaping (#351 review R1, R2) R1: the observer clamp is CSS max-width: 14ch, so 14-char uppercase names (WWWWWWWWWWWWWW, MOUNT WOLFHAWK) clip without the dialog button. Assert on every card that the button is visible exactly when the preview hides observers or a span renders clipped (scrollWidth > clientWidth), and that a touch tap on 375x812 opens the full name. R2: an observer name containing a double quote must stay inside the title attribute, which kills the review's mutant O9 (escapeAttr dropped). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The dialog button was gated on a character count (length > 14), but the clamp is glyph width. Render the button hidden when the preview hides no observers and reveal it after layout on any card whose name actually clips (scrollWidth > clientWidth, which is when the ellipsis is drawn). All reads happen before the writes, so the grid costs one layout pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…351 review R3) RFC3339Nano trims trailing zeros, so WindowStart ("...:00Z", ".5Z") can carry fewer fraction digits than a stamp (".100Z", ".501Z") and the string pre-filter sorts an in-window packet below the bound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
review R3, R4) Compare against the window start truncated to whole seconds in a fixed width layout. A UTC stamp at or after the start always sorts at or above it whatever its fraction, and the start's own second falls through to the exact time check. Also name the caller GetNodeHealth in the lock-order comment instead of the removed getNodeHealthAt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent3 PR#351 runde-3 — head d971bbdReview feedback addressed (commit
Not changed: R5 and R6 (info). R5 corrects the round-2 report: the attempt-1 Go tests did run and pass. R6 is pre-existing and not caused by this PR; it still shows on the local fixture server ( Tests & mutants
I made one E2E tweak alongside: the round-2 F2 loop's Suites [T]
Browser check [T]Local Go server on the fixture copy, with two observer names substituted in the scratch DB (
CI per job (run 37736044834, head d971bbd, pull_request, attempt 1)
No reruns were needed, and neither #271 nor #301 appeared. Notes
|
Review — CS-pve-agent2 PR#351 — head d971bbdDom: APPROVE med nits This is an independent, read-only re-review of round 3: commits I ran everything on All four round-2 findings are resolved. R1 was the blocking one. It is now decided by measuring the rendered spans. I verified this in a browser at 1280×900, 375×812 (touch) and 320×640, in light and dark, and across live resizes. What remains are three nits and two info items, none blocking. Findings
Answers to the round-3 checklistR1 — clipping decided by rendering: resolved. [K] The [T] Browser setup: a local Go server on the CI-prepared
[T] Resize checks, without re-rendering:
R2 — the XSS test gap: resolved in the test. [T] I ran mutant O9 myself ( R3 — fraction-width-safe pre-filter: resolved. [K] The pre-filter now compares
R4 — stale comment: resolved. [K] The comment now names History / #352: intact. [T] Head is a plain fast-forward from Scope: no creep. [K] Always-checks
Tests I ran (merged tree
|
| Suite | Result |
|---|---|
cd cmd/server && go vet ./... && go test -count=1 -timeout 60m ./... |
[T] ok |
cd cmd/server && go test -race -count=1 -timeout 60m ./... (as in CI) |
[T] ok (682.8 s) |
cd cmd/ingestor && go test -count=1 -timeout 60m ./... (no ingestor diff; run because the merged tree includes master changes) |
[T] ok (203.3 s) |
sh test-all.sh |
[T] 234 passed, 0 failed (234 files) |
node test-frontend-helpers.js |
[T] 712 passed, 0 failed |
CHROMIUM_REQUIRE=1 node test-issue-304-my-mesh-e2e.js |
[T] pass |
CHROMIUM_REQUIRE=1 node test-issue-2027-my-mesh-node-page-e2e.js |
[T] 25 passed, 0 failed |
test-home-coverage-e2e.js against a local server |
[T] 12 passed, 0 failed |
test-a11y-axe-1668.js against a local server |
[T] 0 violations across 120 cells |
| Browser probes against a local server (R1 table, resize, sweep) | [T] see R1 |
The local server was corescope-server built from the merged tree. It used e2e-fixture.db prepared as in CI: freshen, the Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL, corescope-migrate, then seeds 2073 and 199 (and 245, as CI does). It logged coverage=93.8%, loaded=500, totalInDB=533, which is R6 from round 2: pre-existing, and the reason every real-server card shows "Activity unavailable". It ran on a free port and was stopped by looking up the listener by port. Neither known flaky test (#256, #267) appeared.
Mutants
| # | Mutant | Result |
|---|---|---|
| O9 | title="${obsFullName(o)}" without escapeAttr |
killed by the E2E (observer span gained attributes: class,onmouseover,title); survives the XSS gate (N4) [T] |
| N1 (mine) | measure only the first preview name (querySelector instead of querySelectorAll(...).some) |
killed — E2E card 7 at 1280px: clipped=[…] hides=false but button visible=false (the Shorty + wide-names client card) [T] |
| N6 (mine) | scrollWidth >= clientWidth (reveals on every card) |
killed — E2E (1 !== 0, a control card gains a button) [T] |
| N2 (mine) | startSecond layout 2006-01-02T15:04:05.000Z (fixed ms fraction instead of whole seconds) |
killed — TestNodeActivity24hPreFilterIgnoresFractionWidth (1 ns stamp and .500001Z stamp dropped) [T] |
| N3 (mine) | startSecond from end − 23h (bound one hour late) |
killed — three tests, including …RollingBoundaries… and …AddPreFilterKeepsBoundaryPackets [T] |
All mutants were applied on a scratch clone at the merged tree and reverted with git checkout. Afterwards I checked that git status was clean.
CI
[T] Run 37736044834 (pull_request, head d971bbd3, attempt 1): Go Build & Test success (ok github.com/corescope/server 979.454s, 0 --- FAIL, ingestor ok). Playwright E2E Tests success: the log shows My Mesh #304: desktop/mobile card, sparkline, coverage and observer dialog passed. Build & Publish Docker Image success. Release Artifacts, Deploy Staging and Publish Badges & Summary were skipped (gated). [A] CI tests the PR merge ref, not a re-merge onto today's master; my merged-tree runs above cover that.
Not verified
- No staging or production check, and the server API key was not used, as instructed.
- Fonts other than Chromium's default sans on Linux. Because the check is measured, other fonts only change which names clip. N1's sub-pixel artifact may land on different names or widths.
- Browsers other than Chromium (WebKit/Firefox rounding of
scrollWidth), real touch hardware (I used Playwright'shasTouch/isMobileemulation), and browser zoom or minimum-font-size settings. - Screen-reader behaviour beyond axe.
- My Mesh: show estimated zero-hop and flood advert intervals on repeater cards #352 itself was not reviewed, only its relation to this branch. The remaining My Mesh: make repeater cards compact and fix misleading activity and health metrics #304 criteria (status evidence, SNR/hops labels, previous-period comparison, advert intervals) and My Mesh: show observer identity, status and traffic separately from mesh role #262 (observer identity badge) are outside this slice, and the PR doesn't claim them.
Bring #351 rounds 2-3 (merged as 1f423e0) under #352's advert cadence. Conflicts: - public/home.js: keep master's clipped-name disclosure (title + revealClippedObserverButtons) and add #352's cadence block after the observer row. - test-issue-304-my-mesh-e2e.js: keep master's eight mixed-role cards, wide/clipped names and the escapeAttr quote test; carry #352's advertIntervals/advertRouteBackfill fixtures on repeaters 0-2 and the opt-in health URL. Also pin that the no-estimate repeater shows no provisional badge and a non-repeater shows no cadence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…retention floor (#509) With hotStartupHours=0 the load floor is s.packets[0].FirstSeen. A loaded re-heard row (old first_seen, recent last_seen) pulls it below the retention floor, so the verification counts DB rows that were never meant to be loaded and coverage stays unknown although every retained row is in memory. No retention tick is needed; this is the default-config case (#351 F1). The second test adds concurrent ingest during the load and between the two counts, and rows on both sides of the retention floor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
First, deliberately limited slice of #304 for the My Mesh repeater cards:
The broader #304 status, SNR, and traffic-semantics work remains open. Observer/room role-card classification is separate (#262).
Validation
cmd/server:go test ./...— passed.cmd/ingestor:go test ./...— passed (unchanged production code).sh test-all.sh— 227/227 test files passed.CHROMIUM_REQUIRE=1 node test-issue-304-my-mesh-e2e.js— desktop and mobile passed; checked 0/1/51 observers, long/malicious names, full/partial/missing activity, dialog keyboard/focus and touch.CHROMIUM_REQUIRE=1 node test-issue-2027-my-mesh-node-page-e2e.js— 25/25 passed.bash scripts/check-xss-sinks.sh --file public/home.jsandgit diff --check— passed.Performance
The 24 counters are updated inside the existing
GetNodeHealthloop overbyNode[pubkey]; there is no extra per-card API call or second scan. Complexity stays O(node-associated transmissions) per health request, with constant-size 24-bucket output. A 30K-transmission accumulator benchmark measured ~1.05 ms/op on an Apple M5 (-benchtime=3x).