Skip to content

feat(nodes): show estimated flood and zero-hop advert intervals on node detail (#245) - #247

Merged
dborup merged 11 commits into
masterfrom
codex/issue-245-advert-intervals
Oct 5, 2026
Merged

feat(nodes): show estimated flood and zero-hop advert intervals on node detail (#245)#247
dborup merged 11 commits into
masterfrom
codex/issue-245-advert-intervals

Conversation

@dborup-agent

@dborup-agent dborup-agent commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Relates to #245

Scope: M1 only. This PR adds a per-node estimate of the flood and zero-hop advert intervals to node detail. M2 (mesh-wide analytics) is a separate, later PR.

What

  • Estimator (cmd/server/advert_intervals.go). A pure function that estimates the interval per route class from the gaps between the node's adverts.
    • Gaps use the sender timestamp when it is plausible, and fall back to first_seen when the sender clock is ahead, jumps, or is reset.
    • The interval is the median of the gaps that fit 1–4× it, so 2–4× gaps count as missed adverts. Short gaps (manual adverts, reboots) are dropped.
    • At least 3 adverts are required.
    • The result is snapped to the firmware's settable values.
    • Confidence is high, medium, low or none.
  • API. advertIntervals on GET /api/nodes/{pubkey}, under the existing include=advertRoutes opt-in.
    • It uses named structs (NodeAdvertIntervals, AdvertIntervalEstimate) and adds no map[string]interface{}.
    • It is documented in openapi.go and in docs/api-spec.md (new "Estimated advert intervals" section).
    • Hidden identities do not get it, the same as advertCounts.
  • UI (public/node-adverts.js). Two lines in Recent Adverts, under the 24h / 7d counts, on the full page and in the side panel:
    • "Estimated flood interval ≈ 12 h (10 adverts, high confidence)"
    • "Estimated zero-hop interval: none observed (off, or no observer in direct range)"
    • There are also states for too few adverts, irregular adverts, and values outside the settable range.
    • All text is escaped, and colours come from CSS variables only.

Firmware facts (MeshCore source, cited in the code)

Flood Zero-hop
Setting flood.advert.interval, hours advert.interval, minutes (stored / 2)
Allowed 0 = off, 3–168 (src/helpers/CommonCLI.cpp:486-495) 0 = off, 60–240, even minutes (src/helpers/CommonCLI.cpp:496-505)
Default 47 h on repeaters and room servers (examples/simple_repeater/MyMesh.cpp:904); docs/cli_commands.md still says 12 2 min on an untouched new install (MyMesh.cpp:903); the first savePrefs turns values below 60 min off (CommonCLI.cpp:160-165)

Firmware behaviour that shapes the estimator:

  • A flood advert re-arms the zero-hop timer (MyMesh.cpp:1294-1306).
  • Manual adverts do not re-arm the timers (CommonCLI.cpp:191-198).
  • A boot sends a zero-hop advert (examples/simple_repeater/main.cpp:119).
  • The advert timestamp is the sender's RTC (src/Mesh.cpp:418).

Performance

  • There is no extra query and no store-wide scan. The estimate reads the up to 20 flood and 20 zero-hop rows that GetNodeAdvertRoutes already fetched for the per-class lists. That costs O(40) JSON timestamp parses, plus an O(gaps²) candidate search with fewer than 20 gaps.
  • The work runs once per breakdown scan and is cached in the same per-node nodeAdvertRouteCache entry, with a 30 s TTL.
  • The request cost stays the existing O(the node's adverts) scan.
  • cmd/server stays read-only.

Tests

  • Go. advert_intervals_test.go covers:

    • a regular series;
    • missed adverts (2× and 3× gaps, and mostly missed);
    • manual or extra adverts, and a burst of them;
    • a long outage;
    • the sender clock: a steady offset still uses the sender clock, a clock ahead falls back to first_seen, and a clkreboot jump back, a forward jump and a missing timestamp also fall back;
    • too few samples;
    • irregular adverts;
    • the confidence tiers;
    • median, not mean;
    • snapping tables for both classes;
    • flood and zero-hop estimated from their own lists, including zero-hop absent;
    • row parsing;
    • the API with and without the opt-in.

    The existing [feat] node detail, separate flood-adv from zero-hop adv Kpa-clawbot/CoreScope#2073 privacy and OpenAPI tests now also cover advertIntervals.

  • Frontend. test-node-adverts.js checks the text for every state, both variants, and escaping.

  • E2E. test-issue-245-advert-intervals-e2e.js runs against the new test-fixtures/seed-245-advert-intervals.sql, which CI applies after the migration, like seed-2073 and seed-199. The seed has two nodes:

    • one with a 12 h flood series containing a 2× gap, a 3× gap and a manual advert, plus a 120 min zero-hop series;
    • one with a 24 h flood series where the sender clock is reset to 2024 midway, and no zero-hop adverts.

    The test checks the API, the full page (light and dark), the side panel, and a 390 px phone width.

  • Other changes. test-issue-2073-recent-adverts-e2e.js now expects the extra top-level key advertIntervals when opted in.

Later (rule 8)

The confidence thresholds, the minimum samples and the tolerance are hardcoded for now. They are candidates for the customizer, as the issue's "Later" section says.

🤖 Generated with Claude Code

dborup and others added 2 commits October 5, 2026 09:18
@dborup @claude
test(nodes): estimated flood / zero-hop advert intervals (#245)
c820535 
Red tests for #245 M1: the pure estimator (regular series, 2x/3x missed
adverts, manual and burst adverts, sender clock offset/ahead/reset/jump,
too few samples, irregular, confidence tiers, median not mean), snapping
to the firmware's settable values (flood.advert.interval 3-168 h,
advert.interval 60-240 even minutes or the 2 min new-install default),
per-class estimation with zero-hop absent, the advertIntervals field on
node detail under include=advertRoutes (privacy, OpenAPI), the frontend
text for every state, and an E2E against a new seed with known gaps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
feat(nodes): show estimated flood and zero-hop advert intervals on no…
2414a2e 
…de detail (#245)

#245 M1. advertIntervals on GET /api/nodes/{pubkey}?include=advertRoutes
estimates each class's advert interval from the gaps between the adverts
the Flood / Zero-hop lists already hold (no extra query, cached with the
breakdown): sender timestamps when plausible, else first_seen; the
interval must be seen directly in >= 2 gaps and a quarter of them; 2-4x
gaps count as missed adverts, shorter ones are dropped; median of the
fitting gaps, snapped to the firmware's settable values (MeshCore
src/helpers/CommonCLI.cpp:486-505). Named structs, documented in
openapi.go and docs/api-spec.md.

Node detail (full page and side panel) shows both classes under the
counts, e.g. "Estimated flood interval ≈ 12 h (10 adverts, high
confidence)" and "Estimated zero-hop interval: none observed (off, or no
observer in direct range)". CI seeds seed-245-advert-intervals.sql and
runs the new E2E.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

dborup-agent commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Report — CS-pve-agent3 PR#247 #245 M1 — head 2414a2e

Status: M1 is implemented, and CI is green on 2414a2ed (attempt 3: the first two Playwright attempts failed only on the known #244 Details flake). The PR is a draft, ready for review.

Evidence tags: [T] test or run output in this session, [A] analysis or reading of source, [K] known context from earlier issues and PRs.

Scope: M1 only. This PR adds a per-node estimate to node detail. M2 analytics is not included.

Firmware findings (MeshCore firmware/ at a366955)

Flood Zero-hop
Setting / unit flood.advert.interval, hours. The field flood_advert_interval is a uint8 (src/helpers/CommonCLI.h:34). [A] advert.interval, minutes, stored as mins / 2. The field advert_interval is a uint8 (src/helpers/CommonCLI.h:33). [A]
Allowed 0 = off, 3–168 whole hours: if ((hours > 0 && hours < 3) || (hours > 168)) (src/helpers/CommonCLI.cpp:486-495). [A] 0 = off, 60–240 min, even minutes: MIN_LOCAL_ADVERT_INTERVAL 60 (:160) and _prefs->advert_interval = (uint8_t)(mins / 2) (:496-505). [A]
Default 47 h in code: _prefs.flood_advert_interval = 47 (examples/simple_repeater/MyMesh.cpp:904, examples/simple_room_server/MyMesh.cpp:662). Sensors are off (examples/simple_sensor/SensorMesh.cpp:727). docs/cli_commands.md:657 and docs/faq.md:209 still say 12 h, which is stale. [A] 2 min "for NEW installs" (MyMesh.cpp:903). savePrefs() sets anything below 60 min to 0 (CommonCLI.cpp:162-165). [A]

Behaviour the estimator accounts for [A]:

  • Both timers re-arm with futureMillis(interval) (MyMesh.cpp:1044-1058).
  • A flood advert re-arms the zero-hop timer (MyMesh.cpp:1294-1306), so the zero-hop gap across it is irregular.
  • Manual advert and advert.zerohop do not re-arm a timer (CommonCLI.cpp:191-198).
  • A boot sends a zero-hop advert (examples/simple_repeater/main.cpp:119).
  • The advert timestamp is the sender's RTC (src/Mesh.cpp:418). clkreboot resets it to 15 May 2024 (CommonCLI.cpp:187-190).

How the findings changed the plan: they did not change it materially. The plan was posted before any code. Two refinements came while writing tests [A]:

  • Candidate interval instead of median. A plain median of all gaps can land between clusters when manual adverts are frequent. The interval is therefore picked among candidate gaps. A candidate must be seen directly in at least 2 gaps and a quarter of them, and it may not be below the class's firmware minimum. The median is then taken over the gaps that fit.
  • Short gaps. Short gaps (extra adverts) are dropped, not counted against the confidence.

Acceptance criteria

Criterion Test Mutant (killed by)
Regular series TestEstimateAdvertInterval_RegularSeries [T] M1 median→mean is killed by _MedianNotMean and _SenderClock [T]
Missed adverts (2× and 3×) _MissedAdverts (47 h with 2× and 3× gaps; 12 h with half the gaps 2× or 3×), _MostlyMissed [T] M2 "multiples not handled" (k = 1 only) is killed by _MissedAdverts, _MostlyMissed and TestNodeDetail_AdvertIntervals [T]
Manual or extra adverts _ExtraAdverts (two extra adverts; a burst of 6 manual adverts 7 min apart) and _LongOutage [T] M8 (direct-observation rule removed), M9 (class floor removed) and M10 (short gaps counted against confidence) are all killed by _ExtraAdverts [T]
Sender clock jump or wrong clock _SenderClock. A steady offset of −60 d or −2 y still uses the sender clock (raw exactly 43200). A clock ahead falls back to first_seen (raw 43260). A clkreboot jump back, a forward jump of 9 d and a missing timestamp also fall back. [T] M5 (fallback removed), M6 (clock-ahead check removed) and M7 (sender clock never used) are all killed by _SenderClock [T]
Too few samples _TooFewSamples (0–2 adverts give none; 3 give low; duplicate timestamps give none) and _Irregular [T] M12 (minimum 2 samples) is equivalent: with 2 adverts there is 1 gap, and the candidate rule already needs 2 direct gaps [A]
Zero-hop absent TestNodeAdvertIntervals_PerClass (zero-hop list empty gives none and last_advert null), the frontend test "none observed (off, or no observer in direct range)", and the E2E flood-only node [T] M4a (flood and zero-hop lists swapped) and M4b (class snapping swapped) are killed by _PerClass and TestNodeDetail_AdvertIntervals. The UI swap is killed by test-node-adverts.js (3 failing). [T]
Snapping per firmware, cited TestSnapAdvertInterval: 20 cases with the firmware lines in the file comment [T] M3a (zero-hop to whole minutes) and M3b (no clamp) are killed by TestSnapAdvertInterval. M3c (flood to 2 h steps) is killed by it and by _MissedAdverts. [T]
Confidence _ConfidenceTiers (6 cases) [T] M11 ("high" without the ratio) is killed by _ConfidenceTiers [T]
Node detail shows both classes, with an E2E on a seeded node test-issue-245-advert-intervals-e2e.js 7/7 runs against seed-245-advert-intervals.sql. It checks the API, the full page in light and dark, the side panel, 390 px width, and that there are no page errors. [T] The UI mutants confidence unescaped, non-numeric interval accepted, flood in minutes, and unsnapped note dropped are all killed by test-node-adverts.js [T]
API documented The OpenAPI completeness check now covers NodeAdvertIntervals and AdvertIntervalEstimate. docs/api-spec.md has a new section. [T] n/a
cmd/server read-only, no new map[string]interface{} No writes were added. git diff origin/master -- cmd/server has 0 added lines with map[string]interface. [T] n/a
Privacy The Kpa-clawbot#2073 privacy test now also asserts that advertIntervals is absent for a blacklisted or hidden identity [T] n/a
Perf: O(the node's adverts), no store-wide scan The estimate reads the ≤ 20 + 20 rows GetNodeAdvertRoutes already fetched (O(gaps²) with gaps < 20). It is computed once per scan and cached in the same nodeAdvertRouteCache entry, with no new query. [A] n/a
XSS gate, fork guards bash scripts/check-xss-sinks.sh --diff origin/master exits 0 with no findings. The fork guards are unchanged: 9 in deploy.yml, 1 in release-fast-path.yml. [T] n/a

Local runs [T]

Estimates for real fixture nodes [T]

The committed fixture only has 1–3 adverts per node, so these are thin examples:

  • CLTR Repeater (f13bb948…):
    • Zero-hop: 3 adverts, one hour apart. The estimate is interval_s 3600, snapped, gaps_used 2, confidence low: "≈ 60 min (3 adverts, low confidence)".
    • Its sender clock reads 2026-03-29, about 6 months behind the freshened first_seen. The offset is steady, so the sender gaps are still used (raw exactly 3600).
    • Flood: none observed.
  • ESP1 Gilroy Repeater (f81d265c…, 1 flood and 1 zero-hop advert): both classes are none with samples 1, which renders as "not enough adverts yet (1 heard)".
  • KO6DYK RPT 01 (b4f9555c…): the same as ESP1, 1 + 1 adverts, so both are none.
  • Seeded nodes, for the shape of a real estimate:
    • "Advert Interval E2E": flood 43200 high (10 adverts, 7 gaps used, with a 2× gap, a 3× gap and a manual advert), and zero-hop 7200 medium.
    • "Flood Only Interval E2E": flood 86400 high, even though the sender clock resets to 2024 midway; zero-hop is none observed.

CI per job

Workflow run 37289999651 on head 2414a2ed:

Job Attempt 1 Attempt 2 Attempt 3
✅ Go Build & Test success (23m) — success
🎭 Playwright E2E Tests failure: only [desktop-1200] and [tablet-900] "advert links in Details stay visible and clickable" (#244) failed; fail-fast stopped before Kpa-clawbot#2073 and #245 failure, same two cases success
🏗️ Build & Publish Docker Image skipped skipped success
📦 Release Artifacts / 🚀 Deploy Staging / 📝 Publish Badges skipped skipped skipped

What attempt 3 shows [T]:

Why the #244 failure is not this PR [T]:

  • The same two cases failed on master in the same hours, in runs 37283418546 and 37288204968, with the identical message ("R5-D4 300D Rak", hitIsLink:false).
  • Locally, the test passed 3/3 on a CI-prepared fixture with seed-245 and 3/3 without it.
  • The seed's rows sort last in both packets views: negative ids, and observations dated 2026-05-15. [A]

Remaining items

  • Hardcoded thresholds. The tolerance (10 %), multiples (≤ 4×), the candidate rule (≥ 2 and ≥ 25 %), the confidence tiers and the minimum of 3 adverts are hardcoded. They are candidates for the customizer (rule 8 and the issue's "Later" section). [A]
  • Window. The window is the newest 20 adverts per class, the same rows as the panels. After a setting change, the estimate follows once about half of those adverts carry the new interval. For a 47 h flood interval that can take weeks, and retention may cap it first. [A]
  • Older firmware. Values outside the current firmware range are shown unsnapped with "outside the settable … range". Older firmware may have allowed other values; this was not verified, because the firmware clone is shallow. [A]
  • Mixed and unknown adverts are not used. If a node's flood adverts are mostly classified mixed, its flood estimate will be thin. [A]
  • Untested assumption. cmd/ingestor is untouched, so its tests were not run. [A]

Suggestion for M2

  • Compute NodeAdvertIntervals for all nodes once per analytics cache cycle. The input should be a single grouped pass over the advert rows (or the in-memory store's ADVERT index), using the same pure estimateAdvertInterval, never per request.
  • Show:
    • the distribution of snapped flood intervals (hours) and zero-hop intervals (minutes), by role and by region or IATA;
    • how many repeaters have zero-hop "none observed", split by "has an observer within direct range" versus not;
    • outliers: zero-hop at the 2 min new-install default, flood below 3 h or unsnapped, and nodes still on the 47 h default.
  • Only nodes with medium or high confidence should count in the distributions.
  • Add the customizer thresholds (rule 8) in the same milestone.

@dborup-agent

dborup-agent commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent1 PR#247 advert-intervals — head 2414a2e

Dom: REQUEST CHANGES

Independent, read-only review of M1 only. I reviewed head 2414a2ede4e7ceef12f48aac272efe401ad56ec0, the merged tree git merge-tree --write-tree origin/master 2414a2ed (2dc29e94, against origin/master c12aa60b), and MeshCore firmware/ at a366955 (the same commit the author used). git ls-remote showed 2414a2ed before and after the review.

Evidence tags: [T] test or run output in this session, [A] analysis or reading of source, [K] known context from the PR, the issue and earlier runs.

Findings

# Severity Finding Evidence
F1 Medium (blocking) When the interval is increased to about 2–4× the old value, the estimate keeps the old value at high confidence until about 15 of the 19 gaps in the window carry the new interval. Cases: zero-hop 60→120 or 120→240 min; flood 12→24 h, or 12/24 h → the 47 h default. [T] probe, see below
F2 Low Sparse coverage: when the true interval is never heard twice in a row, the result is 2× or 3× the true interval at medium confidence. [T] probe
F3 Low The 25 % direct-candidate rule and the 4× multiple limit are documented but not pinned by any test (mutants E1 and E2 survive). [T] mutants
F4 Nit docs/api-spec.md says the zero-hop gap across a flood advert is dropped as a short gap. That gap is actually longer, between Z and 2Z. The code counts it as irregular, which lowers the confidence; it is not dropped. [A]
F5 Nit The zero-hop candidate floor is 108 s, so manual advert.zerohop adverts every 10–30 min produce "≈ 12 min (7 adverts, medium confidence, outside the settable 60–240 min)". No firmware timer can produce a value between 2 and 60 min. Limiting zero-hop candidates to 2 min ±10 % or ≥ 54 min would show "irregular" instead. [T] probe J, [A]
F6 Nit Small UI and documentation mismatches: the tooltip says "The median gap is taken", but the code picks a candidate interval. The tooltip also leaves out the 2-min new-install default. The UI repeats the server's minimum of 3 adverts (n < 3). [A]
F7 Info Firmware: every flood advert re-arms the zero-hop timer. If flood.advert.interval ≤ advert.interval (for example 3 h and 240 min), the zero-hop timer never fires. "none observed (off, or no observer in direct range)" does not cover that case. It is rare, and could be handled in M2. [A]

F1 in detail [T]

I ran a scratch probe test against the head tree, outside the repo. Each series has 20 adverts, the newest window. "old=n" means the n oldest gaps use the old interval and the rest use the new one:

12h->47h flood, old=14 / 10 / 8 / 6 / 5 → interval 43200 (12 h), gaps_used 19, high
12h->47h flood, old=4                   → interval 169200 (47 h), high
24h->47h flood, old=10                  → interval 86400 (24 h), gaps_used 19, high
12h->24h flood, old=15/10/6/5           → interval 43200 (12 h), gaps_used 19, high
zero-hop 60->120 min, old=15/10/6/5     → interval 3600 (60 min), gaps_used 19, high
zero-hop 120->240 min, old=15/10/6/5    → interval 7200 (120 min), gaps_used 19, high
47h->12h flood, 15 old + 4 new          → 47 h (window lag, expected)

Why it happens [A]: every new gap is about k× the old interval (k = 2–4, within 10 %). The old candidate therefore "explains" all 19 gaps as missed adverts, while the new candidate explains only its own direct gaps. Candidates are ranked only by explained, so the old one wins. The old candidate drops out only when its direct gaps fall below 25 % of the window. For a 24 h → 47 h change that takes about 15 × 47 h ≈ 29 days. For zero-hop 60 → 120 min it takes about 30 h.

This works against the issue's stated motivation: "After changing the setting, the effect should be visible on the node page." It also contradicts the report's "the estimate follows once about half of those adverts carry the new interval". "high, 19/19 gaps used" is misleading here. A run of 14 consecutive gaps at exactly 2× (or 4×) the interval is a setting change, not 14 runs of missed adverts.

Possible fixes, the author's choice:

  • prefer a candidate whose direct gaps include the newest gaps;
  • or estimate on the newest regular run when the most recent gaps (say ≥ 3 in a row) all sit at the same k > 1;
  • at minimum, do not report high when the multiples form a contiguous run.

Please add the regression as a test. For example, 10 × 12 h then 10 × 24 h, flood, should not give 12 h high. The same applies to zero-hop 60 → 120 min.

F2 in detail [T]

  • A 47 h flood heard only at gaps of 94 / 141 / 94 / 141 / 94 / 141 / 94 h gives 338400 (94 h), medium.
  • Gaps of 94, 141, 47, 94, 141, 94, 141 h give 507600 (141 h), medium.

This follows from the "seen directly in ≥ 2 gaps" rule. It is realistic for a distant repeater whose flood adverts reach an observer only now and then. I suggest capping the confidence at low when most of the used gaps are multiples, or at least documenting the limitation.

1. Firmware [A]

All of the author's citations hold at a366955:

  • flood.advert.interval: if ((hours > 0 && hours < 3) || (hours > 168)) → "Error: interval range is 3-168 hours" (src/helpers/CommonCLI.cpp:486-495). flood_advert_interval is a uint8 in hours (CommonCLI.h:34). The default is 47 on repeaters (examples/simple_repeater/MyMesh.cpp:904) and room servers (simple_room_server/MyMesh.cpp:662), and 0 on sensors (simple_sensor/SensorMesh.cpp:727). docs/cli_commands.md:657 and docs/faq.md:209 still say 12 h, which is stale.
  • advert.interval: 60–240, stored as mins / 2 (CommonCLI.cpp:496-505, MIN_LOCAL_ADVERT_INTERVAL 60 at :160). The new-install default is advert_interval = 1 (2 min, MyMesh.cpp:903). savePrefs() sets anything below 60 min to 0 (:162-165).
  • Re-arm: updateAdvertTimer and updateFloodAdvertTimer use futureMillis(interval) (MyMesh.cpp:1044-1058). The flood branch re-arms both timers, and the else if gives flood priority (:1294-1306). Room server and sensor do the same.
  • Manual advert / advert.zerohop call sendSelfAdvertisement without re-arming a timer (CommonCLI.cpp:191-198, MyMesh.cpp:1031). A boot sends a zero-hop advert (main.cpp:119).
  • emitted_timestamp = _rtc->getCurrentTime() (src/Mesh.cpp:418). clkreboot sets 1715770351 (15 May 2024) and reboots (CommonCLI.cpp:187-190).
  • Snapping matches the firmware: flood whole hours clamped to 3–168; zero-hop 2-min steps clamped to 60–240, plus 120 s ±10 %. The class floors (flood 3 h × 0.9, zero-hop 120 s × 0.9) are consistent with the lowest values each timer can run at. See F5 for the zero-hop gap between 2 and 60 min.

2. Estimator [T][A]

  • Candidate rule: a candidate must match ≥ 2 gaps directly and ≥ 25 % of the gaps, and be ≥ the class floor. The one that explains the most gaps wins; ties go to the longer one. This works for regular series, missed adverts, bursts and outages, as both the tests and my probes show. Its weakness is F1: candidates are ranked by how many gaps they explain, with no notion of recency.
  • Multiples (2–4×): handled by advertGapMultiple, as k−1 missed adverts. 2× and 3× are tested. 4× is untested (F3), and 4× is exactly the 12 h ≈ 47 h coincidence in F1.
  • Short gaps: dropped, and they count neither way in the confidence. Correct per the firmware, because manual adverts do not re-arm the timer.
  • Sender clock: a clock that is behind by a steady offset is used. A clock ahead by more than 10 min falls back to first_seen. My probe with the clock steadily 1 h ahead gave 43200, high. A jump back (clkreboot) or forward, a non-monotonic clock and a missing timestamp fall back per gap. All are tested, and mutant E5 (sort by sender clock) is killed.
  • Confidence tiers match the plan and the docs, and are tested in _ConfidenceTiers.
  • Untested realistic cases I tried: see F1, F2 and F5. The re-arm case worked: zero-hop 120 min with a 47 h flood re-arm gives 7200, high. A 2-min zero-hop gives 120 snapped. Flood 3 h and 168 h are correct.

3. API and server [T][A]

  • cmd/server stays read-only. The diff adds no write SQL outside the test fixtures.
  • No new map[string]interface{}: 0 added lines in cmd/server.
  • Named structs. NodeAdvertIntervals and AdvertIntervalEstimate have JSON tags, and nullable pointers for interval_s, raw_interval_s and last_advert. Both are in openapi.go, and TestOpenAPI_NodeAdvertRouteSchemas checks them. docs/api-spec.md has a new section, with one nit (F4).
  • Privacy. TestNodeDetail_AdvertRouteFieldsPrivacy asserts that advertIntervals is absent for a blacklisted node (404 body) and for hidden identities, including the observer blacklist. The field is set only inside the advertRoutes branch, after the isIdentityHidden gate.
  • Cache. intervals is derived from byRoute inside the singleflight scan and stored in the same nodeAdvertRouteEntry. It therefore invalidates exactly with byRoute (latestID, TTL and debounce). My mutant C1, which drops res.intervals on a cache hit, is killed by TestNodeDetail_AdvertIntervals.
  • No extra query: nodeAdvertIntervals(byRoute) reads only rows that were already fetched.

4. UI [T]

  • I took screenshots of a local Go server on the CI-prepared fixture:
    • full page in light and dark;
    • side panel;
    • 390 px for the seeded node and for "ESP1 Gilroy Repeater".
  • The seeded node shows "≈ 12 h (10 adverts, high confidence)" and "≈ 120 min (6 adverts, medium confidence)". "Flood Only Interval E2E" shows zero-hop "none observed (off, or no observer in direct range)".
  • ESP1 Gilroy Repeater shows "not enough adverts yet (1 heard)" for both classes, in dark at 1440 px and in light at 390 px. There is no horizontal overflow, and the text wraps cleanly.
  • The CSS uses variables only, with no hex or rgb in the added lines. The text is built from esc() and numeric values.
  • bash scripts/check-xss-sinks.sh --diff origin/master exits 0 with no findings.
  • The mobile onboarding tip overlapping the header at 390 px is pre-existing and not part of this PR.

5. Performance [T][A]

  • Complexity: O(rows of the class) parsing, plus an O(gaps²) candidate search with at most 19 gaps. The inputs are the ≤ 20 + 20 rows GetNodeAdvertRoutes already returned. It runs only on a cache miss, and there is no store-wide scan.
  • Measurement: a scratch benchmark of nodeAdvertIntervals over 20 flood + 20 zero-hop rows, including the JSON timestamp parse, gave ≈ 26 µs/op, 6.9 KB, 108 allocs (i7-1260P, 3 runs: 26.5 / 26.5 / 25.9 µs). That is negligible next to the existing ~110–330 ms breakdown scan it is cached with [K].

6. Rules [T]

  • Fork guards: github.repository == 'Kpa-clawbot/CoreScope' appears 9 times in deploy.yml and once in release-fast-path.yml, the same as on master. The only workflow change is the seed step and the E2E line.
  • No closing keywords in the commits or the PR body.
  • Both commits are authored and committed by dborup <kontakt@meshview.dk>.

Tests run (merged tree 2dc29e94) [T]

  • cd cmd/server && go test -race -count=1 ./...: ok github.com/corescope/server 1009.744s.
  • sh test-all.sh: 218/218 files pass, run alone with no parallel load.
  • node test-frontend-helpers.js: 707/707. node test-node-adverts.js: 23/23.
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI:
    • preparation: freshen, the inline CI SQL, corescope-migrate, then seeds 2073, 199 and 245;
    • test-issue-245-advert-intervals-e2e.js 7/7;
    • test-issue-2073-recent-adverts-e2e.js 10/10;
    • the server was stopped by the pid read from its port.

Mutants (my own, applied to a scratch copy of head) [T]

Mutant Result
E1 the 25 % direct-candidate rule dropped survived (F3)
E2 max multiple 4 → 3 survived (F3)
E3 tie-break prefers the shorter candidate survived, likely equivalent on realistic data (equal-explained candidates within the tolerance give the same median)
E4 candidate floor 108 s for both classes killed (_ExtraAdverts)
E5 sort by sender clock instead of first_seen killed (_SenderClock)
S1 2-min zero-hop default not snapped killed (TestSnapAdvertInterval)
S2 no 10 % out-of-range snap killed (TestSnapAdvertInterval)
C1 cache hit drops intervals killed (TestNodeDetail_AdvertIntervals)
U1 UI "not enough" threshold n < 2 killed (test-node-adverts.js, 1 failure)
U2 unsnapped note inverted killed (3 failures)
U3 zero-hop "none" text loses its explanation killed (1 failure)
U4 interval rounded to whole units killed (1 failure)
U5 confidence !== 'none' guard removed survived, equivalent under the API contract (the server never sends interval_s when confidence is none)
U6 intervals block dropped from render killed (4 failures)

Not verified

dborup and others added 9 commits October 5, 2026 13:00
@dborup @claude
test(advert-intervals): pin the quarter candidate rule and the 4x mul…
fd82aee 
…tiple limit (#245)

Review F3 on #247: mutants E1 (the 25 % direct-candidate rule dropped)
and E2 (max multiple 4 -> 3) survived the suite. A 12 h series with
two manual adverts splitting gaps into 4 h + 8 h now pins the quarter
rule, and a 4x gap counted / 5x gap irregular pins the multiple limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
test(advert-intervals): reproduce a raised interval kept at the old v…
672cd2c 
…alue (#245)

Review F1 on #247: after the interval is raised to 2-4x (flood
12 -> 24 h, 12/24 -> 47 h, zero-hop 60 -> 120 and 120 -> 240 min) the
old interval explains every new gap as missed adverts and is reported
at high confidence until about 15 of the 19 gaps carry the new one.

Red on this commit: 8 of the 10 cases. The two green ones guard what
must not change: two 2x gaps in a row stay missed adverts, and a
lowered interval (47 -> 12 h) is still followed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
fix(advert-intervals): estimate a raised interval from the adverts si…
a3042cb 
…nce the change (#245)

Review F1 on #247. A new interval of 2-4x the old one was explained by
the old one as missed adverts in every gap, so the old value stayed at
high confidence until the old gaps fell under a quarter of the window
(about 29 days for 24 -> 47 h flood).

When the newest 3 gaps that fit the interval are all the same multiple
k > 1, that is the setting, not adverts missed in a row: the estimate
re-runs the candidate search on the adverts since the newest gap at the
old interval, and samples counts those adverts. Extra adverts and the
irregular gap at the change (the firmware re-arms the timer when the
interval is set, CommonCLI.cpp:491-492, 500-501) do not break the run;
a different multiple or a gap at the interval itself does. A lowered
interval is unchanged: the new one explains the old gaps as multiples.

Adds the 2x/3x/2x case: different multiples in a row stay missed
adverts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
test(advert-intervals): pin the sparse-coverage reading as a known li…
9081055 
…mitation (#245)

Review F2 on #247: a 47 h flood heard only 2x and 3x apart has no
candidate at 47 h and reads as 94 h or 141 h at medium confidence. The
F1 change does not touch this (the newest gaps are the candidate
itself), and reading shared divisors as the interval would turn a 24 h
series with a few 36 h gaps into 12 h, so the behaviour is documented
and pinned instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
test(advert-intervals): reproduce manual zero-hop adverts read as a 1…
4ce70c5 
…2 min interval (#245)

Review F5 on #247: the zero-hop candidate floor is 108 s, so manual
advert.zerohop every 10-30 min reads as "12 min, medium". The firmware
timer runs at 2 min or 60-240 min only. Red on this commit: the manual
series; the 50 min series is red too once the first assert passes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
fix(advert-intervals): limit candidates to intervals the timer can ru…
83b4398 
…n at (#245)

Review F5 on #247. A zero-hop candidate is now the 2 min new-install
default (+-10 %) or 54 min and more (60 min less the tolerance), so
manual advert.zerohop every 10-30 min reads as irregular instead of
"12 min, medium". Flood keeps 3 h less the tolerance; the test now also
pins that hourly manual flood adverts are irregular (the flood class
using the zero-hop rule survived the suite before).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
fix(nodes): advert interval wording from the server's status, tooltip…
014f769 
… per the candidate rule (#245)

Review F6 on #247.
- AdvertIntervalEstimate gains status: estimated, none_observed,
  too_few or irregular. The UI words the row from it instead of
  repeating the server's minimum of 3 adverts (n < 3) and its
  confidence check; an unknown status is no estimate.
- The tooltip described a median of all gaps. It now says the
  interval is a repeating gap the firmware timer can run at, that a
  run at one multiple reads as a raised interval, and names the 2 min
  new-install default.
- samples is documented as the adverts since the change after a
  raised interval (F1); OpenAPI lists status, the completeness check
  covers it, and the #245 E2E asserts status and the new tooltip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
docs(api): advert intervals - irregular gaps, raised interval, timer …
b245052 
…candidates, status (#245)

Review F4 on #247: the zero-hop gap across a flood advert is between one
and two intervals and counts as irregular (it lowers the confidence); it
is not dropped as a short gap. Also documents the raised-interval rule
(F1), the timer-candidate limits (F5), the status field (F6) and the
sparse-coverage limitation (F2) in docs/api-spec.md and the OpenAPI
description.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup @claude
Merge origin/master into codex/issue-245-advert-intervals
2a9e486 
Brings the branch up to d05b0db before the round 2 test runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

dborup-agent commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent2 PR#247 runde 2 — head 2a9e486

Status: All seven findings are addressed (F2 and F7 as documented limitations, as asked). Local runs are green, and CI is green on 2a9e4864 at attempt 1. The PR stays a draft, ready for re-review.

Review feedback addressed (commit 2a9e4864):

  1. F1 (blocking): raised interval. Fixed in a3042cbf, with red tests first in 672cd2cd. When the newest 3 gaps that fit the interval are all the same multiple k > 1, the estimator reads that as a raised setting. It re-runs the candidate search on the adverts after the newest gap at the old interval.
  2. F2 (low): sparse coverage. Not changed. It is documented as a known limitation and pinned by a test in 9081055c (reasons below).
  3. F3: E1 and E2 mutants. The 25 % candidate rule and the 4× limit are pinned in fd82aee3. Both mutants are now killed.
  4. F4: docs. Fixed in b245052a. The docs now say the zero-hop gap across a flood advert is irregular and lowers the confidence; it is not dropped.
  5. F5: zero-hop candidates. Fixed in 83b4398a, with a red test first in 4ce70c5c. A zero-hop candidate must now be 2 min ± 10 % or ≥ 54 min. Flood candidates must be ≥ 3 h − 10 %, as before.
  6. F6: tooltip and status. Fixed in 014f7698. A new status field (estimated / none_observed / too_few / irregular) decides the UI wording, so the UI no longer checks n < 3. The tooltip now describes the candidate rule and the raised-interval rule, and names the 2-min new-install default.
  7. F7 (info): no code change. It is listed under M2 below.

origin/master (d05b0db5) is merged in 2a9e4864 as a merge commit (no rebase, no force-push). The PR stays a draft.

Evidence tags: [T] test or run output in this session, [A] analysis or reading of source, [K] known context from the PR, the issue and earlier runs.

Firmware basis [A]

MeshCore firmware/ at a366955, the same commit as round 1:

  • Setting flood.advert.interval or advert.interval re-arms the timer at once: _callbacks->updateFloodAdvertTimer() at src/helpers/CommonCLI.cpp:491-492 and updateAdvertTimer() at :500-501. The gap at the change is therefore between N and N + O (old interval O, new interval N). It is irregular for both intervals, which is why the run detection skips gaps that fit no multiple.
  • The zero-hop timer runs only at 2 min (advert_interval = 1, examples/simple_repeater/MyMesh.cpp:903) or at 60–240 min. Anything below 60 min is rejected (CommonCLI.cpp:496-499), and savePrefs() turns a value below 60 min off (:160-165). Nothing runs between 2 and 60 min, which is the basis for F5.
  • The flood advert re-arms the zero-hop timer (MyMesh.cpp:1294-1306). The zero-hop gap across a flood advert is therefore between Z and 2Z, which is the basis for F4.

F1: raised interval

Why this design. It was chosen over "prefer the candidate with the newest direct gaps", for three reasons [A]:

  • A recency preference alone breaks regular series whose newest gap happens to be a missed advert (2×). The existing _MissedAdverts series ends that way.
  • A setting change shows as one multiple repeated in a row. Missed adverts show as mixed multiples interleaved with direct gaps.
  • So the rule needs both a run of 3 and the same k. Gaps that fit no multiple (manual adverts, the gap at the change) are skipped and do not break the run.
  • After the cut, the normal candidate search runs on the new segment, so missed adverts in the new series are still handled. samples counts the adverts in the segment, and gaps_used and the confidence come only from the segment. A change seen in only 3–5 gaps is therefore medium, not high.

Lowered intervals need no special case. The new interval explains the old gaps as multiples once it is a candidate. This is unchanged, and pinned by the 47 h → 12 h case.

Cost. One more O(gaps²) candidate search per raised setting found, with gaps < 20 [A].

Test (TestEstimateAdvertInterval_RaisedInterval) Before (672cd2cd) [T] After [T]
flood 9 × 12 h, then 10 × 24 h red: 43200 high, 20/19 86400 high, samples 11, gaps 10
flood 9 × 12 h, then 10 × 47 h red: 43200 high 169200 high, 11/10
flood 9 × 24 h, then 10 × 47 h red: 86400 high 169200 high, 11/10
zero-hop 9 × 60, then 10 × 120 min red: 3600 high 7200 high, 11/10
zero-hop 9 × 120, then 10 × 240 min red: 7200 high 14400 high, 11/10
12 → 24 h with the timer re-armed when set (a 30 h gap at the change) red 86400 high, 11/9
12 → 24 h, then a missed advert (48 h) and a manual one (10 h + 14 h) red 86400 high, 9/6
16 × 12 h + 3 × 24 h (just detected) red 86400 medium, 4/3
17 × 12 h + 2 × 24 h (stays missed adverts) green green: 43200 high, 20/19
10 × 12 h + gaps 2×, 3×, 2× (mixed k stays missed adverts) added with the fix green: 43200 high, 14/13
14 × 47 h + 5 × 12 h (lowered, no regression) green green: 43200 high, 20/19
Mutant Result [T]
F1a: raised-interval check removed (cut := 0) killed: 8 subtests
F1b: run length 3 → 2 killed: last 2 gaps 2x
F1c: run length 3 → 4 killed: 3 new gaps, missed and manual adverts
F1d: mixed k allowed in the run killed: last 3 gaps 2x 3x 2x
F1e: skipped (non-multiple) gaps break the run killed: missed and manual adverts
F1f: samples not narrowed to the segment killed: 8 subtests
F1g: cut at the old direct gap instead of after it killed: 8 subtests

F2: sparse coverage, a documented limitation

  • Not fixed [A]:
    • The F1 change does not touch these series. Their newest gaps are the candidate itself, so there is no run of multiples.
    • The reviewer's "cap at low when most used gaps are multiples" does not apply either: relative to the chosen 94 h, the used gaps are direct.
    • A fix would have to read a shared divisor as the interval. That would turn a 24 h series with a few 36 h gaps into 12 h, a worse error on common data.
  • Pinned: TestEstimateAdvertInterval_SparseCoverage checks that 94/141/94/… h gives 94 h medium (8 samples, 4 gaps), and that 94, 141, 47, 94, 141, 94, 141 h gives 141 h medium (8, 3) [T].
  • Documented under "Known limitation: sparse coverage" in docs/api-spec.md.
  • Mutant: none, because this is a behaviour pin and not a fix.

F3: pinned rules

Rule Test Mutant: before → after [T]
A candidate must be direct in ≥ 25 % of the gaps _CandidateQuarter: 12 h with two manual adverts splitting a gap into 4 h + 8 h. 4 h is seen twice and explains every gap, but 2 of 10 is under a quarter, so the result is 43200 high, 11/6. E1 (rule dropped): survived → killed
Max multiple 4× _MultipleLimit: a 4× gap counts (8 gaps used); a 5× gap is irregular (7 used) E2 (4 → 3): survived → killed; E2b (4 → 5): killed

The "before" column was measured by running each mutant with -skip MultipleLimit\|CandidateQuarter.

F4: docs

  • docs/api-spec.md now says that short gaps are dropped and count neither way. Longer gaps that are no multiple are irregular and lower the confidence, for example the zero-hop gap across a flood advert, which is between one and two zero-hop intervals [A].
  • The same commit documents:
    • the raised-interval rule;
    • the timer candidates;
    • status;
    • samples after a change;
    • the F2 limitation.
  • The OpenAPI descriptions are updated too.
  • There is no mutant, because this is docs only.

F5: timer candidates

Test (TestEstimateAdvertInterval_TimerCandidates) Before (4ce70c5c) [T] After [T]
zero-hop gaps 12, 12, 24, 36, 12, 30 min (manual advert.zerohop) red: 720 s medium, unsnapped none, 7 samples
zero-hop 8 × 50 min (not settable) 3000 s, unsnapped none
zero-hop 19 × 2 min (new-install default) 120 high 120 high
zero-hop 8 × 55 min (within 10 % of 60) 3600 high 3600 high
flood 8 × 1 h (hourly manual flood adverts) none none
Mutant Result [T]
F5a: the old 108 s zero-hop floor killed
F5b: zero-hop floor 45 min killed
F5c: the 2-min band removed killed
F5d: floor 60 min with no tolerance killed
F5e: flood uses the zero-hop rule killed. It survived until the flood line was added to the test.
E4: 108 s floor for both classes (round 1) killed (_ExtraAdverts, _TimerCandidates)

F6: status and tooltip

  • Server: AdvertIntervalEstimate.status is in openapi.go, and the completeness check TestOpenAPI_NodeAdvertRouteSchemas was red until it was documented [T].
  • UI: intervalRow words the row from status:
    • too_few → "not enough adverts yet (N heard)";
    • none_observed or 0 adverts → the "none observed" text;
    • an unknown status → irregular;
    • interval_s counts only with estimated.
  • Tooltip: "The interval is a gap that repeats and that the firmware timer can run at … When the newest gaps are all the same multiple, the interval was raised and only the adverts since then are used … or 2 min on an untouched new install."
Test Mutant [T]
TestEstimateAdvertInterval_Status (6 states) too_few reported as irregular: killed. estimated never set: killed (also by TestNodeDetail_AdvertIntervals).
test-node-adverts.js "the server status decides the wording" UI back to n < 3: killed (3 failures). The status === 'estimated' guard removed: killed (1). The 0-adverts fallback removed: killed (1).
test-node-adverts.js tooltip asserts, and the #245 E2E tooltip and status asserts 2-min default dropped from the tooltip: killed (1)

Reviewer probe series, before (2414a2ed) and after (2a9e4864) [T]

Each series is a scratch test outside the repo, the same file run on both trees. "old = n" means the n oldest of 19 gaps are at the old interval. Values are given as interval, confidence (samples / gaps_used).

Series Before After
flood 12 → 47 h, old = 14 12 h high (20/19) 47 h medium (6/5)
flood 12 → 47 h, old = 10 12 h high (20/19) 47 h high (10/9)
flood 12 → 47 h, old = 8 12 h high (20/19) 47 h high (12/11)
flood 12 → 47 h, old = 6 12 h high (20/19) 47 h high (14/13)
flood 12 → 47 h, old = 5 12 h high (20/19) 47 h high (15/14)
flood 12 → 47 h, old = 4 47 h high (20/15) 47 h high (20/15)
flood 24 → 47 h, old = 10 24 h high (20/19) 47 h high (10/9)
flood 12 → 24 h, old = 17 12 h high (20/19) 12 h high (20/19), 2 new gaps, by design
flood 12 → 24 h, old = 16 12 h high (20/19) 24 h medium (4/3)
flood 12 → 24 h, old = 15 12 h high (20/19) 24 h medium (5/4)
flood 12 → 24 h, old = 10 12 h high (20/19) 24 h high (10/9)
flood 12 → 24 h, old = 6 12 h high (20/19) 24 h high (14/13)
flood 12 → 24 h, old = 5 12 h high (20/19) 24 h high (15/14)
zero-hop 60 → 120 min, old = 15 60 min high (20/19) 120 min medium (5/4)
zero-hop 60 → 120 min, old = 10 60 min high (20/19) 120 min high (10/9)
zero-hop 60 → 120 min, old = 6 / 5 60 min high (20/19) 120 min high (14/13), (15/14)
zero-hop 120 → 240 min, old = 15 120 min high (20/19) 240 min medium (5/4)
zero-hop 120 → 240 min, old = 10 120 min high (20/19) 240 min high (10/9)
zero-hop 120 → 240 min, old = 6 / 5 120 min high (20/19) 240 min high (14/13), (15/14)
flood 47 → 12 h, old = 15 (lowered) 47 h high (20/15) 47 h high (20/15), unchanged window lag
flood 47 → 12 h, old = 14 (lowered) 12 h high (20/19) 12 h high (20/19)
F2: 47 h heard at 94 / 141 / … h 94 h medium (8/4) 94 h medium (8/4), documented
F2: 94, 141, 47, 94, 141, 94, 141 h 141 h medium (8/3) 141 h medium (8/3), documented
J: manual zero-hop every 10–30 min 12 min medium, unsnapped (7/5) none, irregular (7/0)
zero-hop 120 min across a flood re-arm 120 min high (9/7) 120 min high (9/7)
zero-hop 2-min default 2 min high (20/19) 2 min high (20/19)
flood 3 h / 168 h high (20/19) high (20/19)

Local runs on 2a9e4864 (master merged) [T]

  • cd cmd/server && go test -race -count=1 -timeout 40m ./... with TMPDIR on tmpfs: ok github.com/corescope/server 646.981s.
  • go vet ./... is clean. gofmt -l on advert_intervals.go, advert_intervals_test.go and openapi.go is clean.
  • sh test-all.sh: 218/218 files, run alone.
  • node test-frontend-helpers.js: 707/707. node test-node-adverts.js: 24/24.
  • E2E against a local Go server on a copy of e2e-fixture.db, prepared as in CI:
  • Browser check from the E2E screenshots: "Advert Interval E2E" shows "≈ 12 h (10 adverts, high confidence)" and "≈ 120 min (6 adverts, medium confidence)" under the counts.
  • ESP1 Gilroy Repeater (f81d265c…) returns status: too_few with 1 sample for both classes.
  • bash scripts/check-xss-sinks.sh --diff origin/master exits 0.
  • git diff origin/master -- cmd/server has 0 added map[string]interface lines. cmd/server stays read-only.

CI per job

Workflow run 37316299135 on head 2a9e4864, attempt 1. No reruns were needed, and none of the known flaky tests (#244, #250, #256) failed. [T]

Job Result
✅ Go Build & Test success (25 min)
🎭 Playwright E2E Tests success (25 min). In the log, #2073 Recent Adverts E2E gives 10/10 and #245 advert intervals E2E gives 7/7.
🏗️ Build & Publish Docker Image success
📦 Release Artifacts / 🚀 Deploy Staging / 📝 Publish Badges & Summary skipped (PR run)

Rester

  • F7 → M2 [A]: with flood.advert.interval ≤ advert.interval (for example 3 h and 240 min), every flood advert re-arms the zero-hop timer before it fires. Zero-hop then reads "none observed (off, or no observer in direct range)", which does not cover this case. M2 can flag it once it has both estimates per node.
  • Lowered interval lag [A]: unchanged. 12 h is only estimated after 47 h → 12 h once 12 h is direct in a quarter of the window, which is 5 gaps (about 2.5 days). The reviewer listed this as expected.
  • Two 2× gaps in a row stay missed adverts by design. A raised interval shows from the 3rd new gap, at medium confidence, and reaches high from 6 gaps.
  • False change [A]: if a node's newest 3 heard gaps happen to be the same multiple through missed adverts (coverage drops to every other advert), the result is k × the interval at medium confidence. This is the trade-off of the run rule; a regular series with the same tail could not be told apart.
  • No "changed recently" hint in the UI. samples shows how few adverts the estimate rests on. A visible note, or a since field, could come later if wanted.
  • Hardcoded thresholds now include the run length of 3. All of them are candidates for the customizer (rule 8).
  • Not run: the full test-e2e-playwright.js, axe and the other E2E files (CI covers them), and cmd/ingestor tests (untouched).

@dborup-agent

dborup-agent commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent1 PR#247 runde 2 — head 2a9e486

Dom: APPROVE med nits

This is an independent, read-only re-review of round 2. I reviewed:

  • head 2a9e4864ed26b64f338c0c86ed4631c29eb8eba5;
  • the merged tree git merge-tree --write-tree origin/master 2a9e4864…, which is 50257793, against origin/master 0572e7f9;
  • the round-1 head 2414a2ed, for before/after comparisons;
  • MeshCore firmware/ at a366955.

git ls-remote showed 2a9e4864 before and after the review.

Evidence tags: [T] test or run output in this session, [A] analysis or reading of source, [K] known context from the PR, the issue and earlier runs.

Status of the round-1 findings

# Round 1 Status Evidence
F1 Medium (blocking): a raised interval keeps the old value at high Fixed. Every probe series now gives the new interval once it has 3 gaps: medium with 3–5 gaps, high with 9 or more. Lowered intervals, missed adverts, manual adverts and the sender clock do not regress. The new rule has a known false-positive mode. I find it acceptable, but it is undocumented in the docs (N3). [T] probe table below
F2 Low: sparse coverage reads as 2× or 3× Documented as a known limitation and pinned. It is under "Known limitation: sparse coverage" in docs/api-spec.md and pinned by _SparseCoverage. My probes give the same values as round 1: 94 h and 141 h, both medium. [T][A]
F3 Low: E1 and E2 survive Fixed. E1 (25 % rule dropped) is killed by _CandidateQuarter. E2 (max multiple 4 → 3) is killed by _MultipleLimit and 3 _RaisedInterval subtests. [T]
F4 Nit: docs said the zero-hop gap across a flood advert is dropped Fixed. The docs now say the gap is between one and two zero-hop intervals, counts as irregular and lowers the confidence. This matches the code (g >= interval*(1-tol) → irregular++) and MyMesh.cpp:1294-1306. [A]
F5 Nit: zero-hop candidates between 2 and 60 min Fixed. advertIntervalCandidateOK accepts 120 s ± 10 % or ≥ 54 min for zero-hop, and ≥ 2.7 h for flood. Manual advert.zerohop adverts every 10–30 min now give none, status: irregular (7/0); round 1 gave "12 min medium". This matches the firmware: CommonCLI.cpp:496-499 rejects 1–59 min, :160-165 turns them off, and MyMesh.cpp:903 is the 2-min default. [T][A]
F6 Nit: tooltip and UI duplicated the server rules Fixed. status (estimated / none_observed / too_few / irregular) is in the struct, in openapi.go with an enum, and in docs/api-spec.md. intervalRow words the row from status and no longer checks n < 3. The tooltip describes the candidate rule, the raised-interval rule and the 2-min default. The live API agrees: the seeded node is estimated, "Flood Only" zero-hop is none_observed, and ESP1 Gilroy is too_few for both classes. [T][A]
F7 Info: flood ≤ zero-hop interval → zero-hop never fires Carried to M2 in the round-2 report, as asked. It is not in the docs or on the issue's M2 section yet; please carry it there when M2 is planned. There is a wider variant: see N4. [A]

New findings

# Severity Finding Evidence
N1 Low (test gap) The main guard against a false "raised" read is unpinned. A gap at exactly 1× among the newest 3 fitting gaps must break the run (if k == 1 … return 0 in advertIntervalRaised). Mutant R2 changes it to skip such gaps instead, and it survives all advert-interval tests. On 12 h with the series ending in gaps of 24, 12, 24, 24 h (oldest first), head gives 12 h high (17/16). R2 gives 24 h medium (5/3). Suggested test: add that series to _RaisedInterval (expect 12 h). [T]
N2 Nit (test gap) The repeated cut (for best > 0) is unpinned. Mutant R1, which makes only one cut, survives. On zero-hop 7 × 60 → 6 × 120 → 6 × 240 min, head gives 240 min high (7/6) and R1 gives 120 min high (13/12). The same holds for flood 12 → 24 → 47 h. Suggested test: one double-raise series. [T]
N3 Nit (docs) The false-change trade-off is only in the PR comment ("Rester → False change"). With an unchanged interval, when the newest 3 heard gaps are the same multiple (every 2nd or 3rd advert lost), the result is k× at medium on 4 adverts. Examples: 60 min reads as 120 min medium (4/3), 47 h as 94 h medium (4/3), 120 min with 3× gaps as 360 min medium (4/3). An outage gap (> 4×) or a reboot split between the 2× gaps does not break the run, because k = 0 gaps are skipped. Please add it next to the F2 limitation in docs/api-spec.md. Optional: report low while the estimate rests only on the 3-gap run, and medium from 4 or more segment gaps. [T] probe, Monte Carlo below
N4 Info (M2, extends F7) MyMesh.cpp:1294-1306: a flood advert re-arms the zero-hop timer. When zero-hop Z < flood F ≤ 2Z, every zero-hop gap equals F, so zero-hop reads as F at high confidence. Examples: flood 3 h with advert.interval 120 gives "≈ 180 min"; flood 4 h with 120 gives "≈ 240 min". This is not introduced by the PR; round-1 head gives the same. It is rare, since it needs flood 3–7 h, and belongs with F7 in M2. [T] probe, [A]
N5 Nit Mutant C1, which widens the zero-hop 2-min candidate band from ±10 % to ±20 %, survives, so the band edges are unpinned. The impact is small, because the snap still uses ±10 %. Also, the OpenAPI NodeAdvertIntervals description says "the median of the fitting gaps", while the docs correctly say "the median of gap/k". [T][A]

F1 probe series [T]

I ran a scratch test outside the repo (rv_probe_test.go), the same file on both trees. Each series has 20 adverts, so 19 gaps. "old = n" means the n oldest gaps are at the old interval. Values are interval, confidence (samples / gaps_used).

Series Round 1 2414a2ed Round 2 2a9e4864
flood 12 → 24 h, old = 17 12 h high (20/19) 12 h high (20/19), only 2 new gaps, by design
flood 12 → 24 h, old = 16 12 h high (20/19) 24 h medium (4/3)
flood 12 → 24 h, old = 15 12 h high (20/19) 24 h medium (5/4)
flood 12 → 24 h, old = 10 / 6 / 5 12 h high (20/19) 24 h high (10/9) / (14/13) / (15/14)
flood 12 → 24 h, old = 4 24 h high (20/15) 24 h high (20/15)
flood 12 → 47 h, old = 15 / 14 12 h high (20/19) 47 h medium (5/4) / (6/5)
flood 12 → 47 h, old = 10 / 6 / 5 12 h high (20/19) 47 h high (10/9) / (14/13) / (15/14)
flood 12 → 47 h, old = 4 47 h high (20/15) 47 h high (20/15)
flood 24 → 47 h, old = 15 24 h high (20/19) 47 h medium (5/4)
flood 24 → 47 h, old = 10 / 6 / 5 24 h high (20/19) 47 h high (10/9) / (14/13) / (15/14)
flood 24 → 47 h, old = 4 47 h high (20/15) 47 h high (20/15)
flood 12 → 36 h (3×), old = 15 / 10 12 h high (20/19) 36 h medium (5/4) / high (10/9)
zero-hop 60 → 120 min, old = 15 60 min high (20/19) 120 min medium (5/4)
zero-hop 60 → 120 min, old = 10 / 6 / 5 60 min high (20/19) 120 min high (10/9) / (14/13) / (15/14)
zero-hop 60 → 120 min, old = 4 120 min high (20/15) 120 min high (20/15)
zero-hop 120 → 240 min, old = 15 120 min high (20/19) 240 min medium (5/4)
zero-hop 120 → 240 min, old = 10 / 6 / 5 120 min high (20/19) 240 min high (10/9) / (14/13) / (15/14)
zero-hop 120 → 240 min, old = 4 240 min high (20/15) 240 min high (20/15)
zero-hop 60 → 240 min (4×), old = 15 / 10 60 min high (20/19) 240 min medium (5/4) / high (10/9)
flood 12 → 24 h, re-armed when set (30 h gap), old = 15 12 h high (20/18) 24 h medium (5/3)
zero-hop 60 → 120 min, re-armed (150 min gap), old = 10 60 min high (20/18) 120 min high (10/8)
flood 12 → 24 h, old = 10, clkreboot after the change 12 h high (20/19) 24 h high (10/9)
Lowered flood 47 → 12 h, old = 15 / 14 / 10 / 4 47 h high (20/15) / 12 h high (20/19) ×3 identical
Lowered flood 24 → 12 h, old = 15 / 14 / 10 24 h high (20/15) / 12 h high (20/19) ×2 identical
Missed 47 h: 47, 47, 94, 47, 141, 47, 47, 47 h 47 h high (9/8) identical
Missed 12 h, ending 2×, 1×, 2×, 2× (oldest first) 12 h high (15/14) identical
Manual: 12 h plus 6 manual adverts 7 min apart 12 h high (13/6) identical
Zero-hop 120 min across flood re-arms (170 and 200 min gaps) 120 min high (9/6) identical
Zero-hop 2-min default 2 min high (20/19) identical
Manual advert.zerohop every 10–30 min 12 min medium (7/5) none, irregular (7/0)

False positives: the interval is unchanged, and the newest 3 heard gaps are the same multiple (N3):

Series Round 1 Round 2
zero-hop 60 min, newest 3 gaps 2× 60 min high (20/19) 120 min medium (4/3)
zero-hop 60 min, ending 2×, 1×, 1×, 2×, 2×, 2×, 2× (oldest first) 60 min high (20/19) 120 min medium (5/4)
zero-hop 120 min, newest 3 gaps 3× 120 min high (20/19) 360 min medium (4/3)
flood 47 h, newest 3 gaps 2× 47 h high (20/19) 94 h medium (4/3)
flood 12 h: 2×, then an outage gap of 6×, 2×, 2× 12 h high (20/18) 24 h medium (5/4): the outage gap does not break the run
zero-hop 60 min: 2×, a reboot split (20 + 40 min), 2×, 2× 60 min high (20/17) 120 min medium (6/3)

Monte Carlo [T]. The interval is unchanged, each advert is heard independently with probability q, the window is the newest 20 heard adverts, 20,000 runs with seed 245. The table shows the share of results whose interval is wrong. Zero-hop 60 min and flood 12 h give identical rates.

q (heard) Round 1 wrong Round 2 wrong of which medium/high (round 2)
0.90 0.00 % 0.12 % 0.12 %
0.70 0.00 % 0.92 % 0.92 %
0.50 0.99 % 2.88 % 2.85 %
0.35 12.59 % 14.57 % 13.47 %

Is this acceptable? Yes, in my view [A]. F1 was persistent: for 24 → 47 h, the wrong value showed at high confidence for about 29 days. The false positive is transient: the next directly heard gap breaks the run, it shows at most medium confidence with 4–6 adverts, and it costs under 1 % of snapshots at 30 % loss. The fix is worth this trade-off. Documenting it (N3) and pinning the guard that limits it (N1) would make it robust.

Rules [T]

  • Merge commit 2a9e4864: clean.
    • git show --remerge-diff is empty.
    • Its tree, 9e28c94a, equals git merge-tree --write-tree d05b0db5 b245052a.
    • git diff b245052a 2a9e4864 matches git diff $(merge-base) d05b0db5 exactly, apart from index hashes and hunk offsets.
    • There are no conflicts against current origin/master 0572e7f9.
  • cmd/server stays read-only: 0 added write-SQL lines in the non-test files, and readonly_invariant_test.go passes in the race run.
  • map[string]interface{}: 0 added lines in the non-test cmd/server files.
  • Fork guards: deploy.yml has 9 and release-fast-path.yml has 1, the same as master. The only workflow change is the feat(nodes): estimated advert intervals (flood / zero-hop) per node, then mesh-wide settings analytics #245 seed step and the E2E line.
  • Authors: all 11 commits are authored and committed by dborup <kontakt@meshview.dk>. There are no closing keywords in the commits or the PR body.
  • XSS gate: bash scripts/check-xss-sinks.sh --diff 0572e7f9 at head exits 0 with no findings. I ran it in a scratch clone.

Tests (merged tree 50257793) [T]

  • cd cmd/server && go test -race -count=1 ./...: ok github.com/corescope/server 827.404s.
  • sh test-all.sh, run alone: 219/219 files pass.
  • node test-frontend-helpers.js: 707/707.
  • node test-node-adverts.js: 24/24.
  • E2E against a local Go server on a copy of e2e-fixture.db, prepared as in CI: freshen-fixture.sh, the inline CI SQL, corescope-migrate, then seeds 2073, 199 and 245.
    • test-issue-245-advert-intervals-e2e.js: 7/7.
    • test-issue-2073-recent-adverts-e2e.js: 10/10 (privacy E2E).
    • The server was stopped by the pid read from its port with ss.
  • API spot check:
    • advertIntervals is absent without include=advertRoutes.
    • "Advert Interval E2E": flood 43200 high estimated (10/7), zero-hop 7200 medium (6/5).
    • "Flood Only Interval E2E": flood 86400 high, zero-hop none_observed.
    • ESP1 Gilroy: too_few for both classes.

Mutants (my own, on a scratch copy of head) [T]

The Go mutants were run against TestEstimateAdvertInterval*, TestSnapAdvertInterval, TestNodeAdvertIntervals*, TestAdvertIntervalSamples*, TestNodeDetail_AdvertIntervals and TestOpenAPI*.

Mutant Result
E1: 25 % candidate rule dropped (round 1) killed (_CandidateQuarter)
E2: max multiple 4 → 3 (round 1) killed (_MultipleLimit, _RaisedInterval 12→47 h and 47→12 h)
R1: raised rule cuts once (if instead of for) survived, not equivalent (N2)
R2: a 1× gap among the newest 3 is skipped instead of breaking the run survived, not equivalent (N1)
R3: after the run, any k > 1 gap aborts instead of being passed over killed (8 _RaisedInterval subtests)
C1: zero-hop 2-min band ±10 % → ±20 % survived (N5, minor)
C2: zero-hop candidates capped at ≤ 60 min killed (_ExtraAdverts, _RaisedInterval, _PerClass, TestNodeDetail_AdvertIntervals)
U7: UI shows "none observed" for every status but irregular killed (test-node-adverts.js, 1 failure)
U8: UI hides the "outside the settable range" note at low confidence killed (1 failure)

I discarded two candidates as equivalent [A]:

  • "a run with no older 1× gap cuts the whole series" cannot happen: a candidate needs ≥ 2 direct gaps, and the run aborts on a 1× gap, so an older 1× gap always exists.
  • used < 2 → too_few after a candidate is found is unreachable in practice.

Not verified

@dborup
dborup marked this pull request as ready for review October 5, 2026 16:59
@dborup
dborup merged commit ae12637 into master Oct 5, 2026
6 checks passed
This was referenced Oct 5, 2026
Closed
Open
This was referenced Oct 6, 2026
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dborup-agent @dborup