Skip to content

fix(frontend): relay-aware staleness for infra nodes + dim-not-delete (#1598, PR A) - #1815

Open
Saarlandpower wants to merge 3 commits into
Kpa-clawbot:masterfrom
Saarlandpower:fix/1598-relay-aware-staleness
Open

fix(frontend): relay-aware staleness for infra nodes + dim-not-delete (#1598, PR A)#1815
Saarlandpower wants to merge 3 commits into
Kpa-clawbot:masterfrom
Saarlandpower:fix/1598-relay-aware-staleness

Conversation

@Saarlandpower

Copy link
Copy Markdown

Implements PR A from the #1598 triage fix path (r6 — all three operator decisions locked: confidence ≥0.75 [PR B], dim-not-delete, in-place getNodeStatus signature extension).

Changes

public/roles.jsgetNodeStatus()

  • Now accepts a full node object (preferred); the legacy (role, lastSeenMs) signature keeps working unchanged.
  • For infra roles (repeater/room), freshness = max(advert-based timestamp, last_relayed). Freshness precedence mirrors existing call sites: _liveSeen > _lastHeard > last_heard > last_seen.
  • last_relayed is only consulted for infra — companions keep pure advert/heard-based staleness (per @liquidraver's collision caveat; the ≥0.75-confidence _liveSeen refresh is PR B).

public/live.jspruneStaleNodes()

  • Repeater/room markers are dimmed, never deleted, regardless of _fromAPI origin. WS-only non-infra nodes are still removed to prevent unbounded memory growth.

Call sites — all seven (nodes.js ×3, map.js ×3, live.js ×1) now pass the node object. The Nodes-page status explanation shows "Last relayed …" when relay participation is the fresher signal, so an Active badge next to an old "last heard" isn't confusing.

Tests — 16 new getNodeStatus unit tests incl. the triage's backbone-repeater fixture (last_seen=25h, last_relayed=5min → active); pruneStaleNodes tests updated for dim-not-delete plus a new relay-aware prune test.

Test results

node test-frontend-helpers.js: 641 passed, 2 failed — the 2 failures (favStar ★ assertions) are pre-existing on current master (verified on a clean upstream clone).

Validation offer

live.saarmesh.de currently has 160 infra nodes past infraSilentMs=72h while actively relaying (incl. KatS disaster-relief repeaters) — a ready-made test population. Happy to run this branch there and report before/after node visibility.

Refs #1598 (PR A of two; PR B = _liveSeen refresh on resolved_path ≥0.75 confidence).

Mathias Kasper and others added 2 commits July 1, 2026 23:17
…Kpa-clawbot#1598)

Implements PR A from the Kpa-clawbot#1598 triage fix path (r6, decisions locked):

- getNodeStatus() now accepts a full node object (legacy (role, lastSeenMs)
  signature still works). For infra roles (repeater/room), freshness is
  max(advert-based timestamp, last_relayed), so actively-relaying backbone
  repeaters with long/disabled advert intervals are no longer marked stale.
- pruneStaleNodes() in the Live view dims repeater/room markers instead of
  deleting them, regardless of _fromAPI origin.
- All seven getNodeStatus call sites (nodes.js, map.js, live.js) pass the
  node object; nodes.js status explanation surfaces 'Last relayed' when
  relay participation is the fresher signal.
- Unit tests incl. the backbone-repeater fixture from triage
  (last_seen=25h, last_relayed=5min -> active).

Empirical motivation: four operator deployments confirm 88% of infra nodes
with last_relayed > last_seen; on live.saarmesh.de 160 infra nodes exceed
even infraSilentMs=72h while actively relaying.

Co-Authored-By: Claude <noreply@anthropic.com>
…t-delete + relay-aware fixture

Co-Authored-By: Claude <noreply@anthropic.com>
@Kpa-clawbot

Copy link
Copy Markdown
Owner

Bot polish review

Verdict: MERGE-READY

Findings

  • [MINOR] public/nodes.js:207 — the "Last relayed" branch fires only when relayedMs > lastHeardMs, but lastHeardMs here is _lastHeard || last_heard || last_seen (no _liveSeen). Callers on the live view will therefore keep showing "Last heard" even when _liveSeen is fresher. In practice this only matters for infra nodes whose live-view marker survives after your dim-not-delete change; the ordering is still safe (never reports a value older than truth), just occasionally suboptimal wording. Consider mirroring the _liveSeen precedence you added inside getNodeStatus.
  • [MINOR] public/nodes.js:192getStatusTooltip(role, status) receives only the role; the tooltip copy therefore doesn't mention "actively relaying" for the case where last_relayed is what pulled the node back to active. Not blocking (the details panel at nodes.js:634 already covers this), but a follow-up could enrich the tooltip.
  • [MINOR] Commit shape — 415ee37e bundles the production change AND the new getNodeStatus test cases in the same commit; the follow-up 8e1c537b only updates the pre-existing pruneStaleNodes test. Strict TDD would want the failing tests as a separate red commit. Since this is a signature change to an existing helper (not a pure net-new surface), a red-then-green history would be preferable next time. Not a blocker — the tests DO assert real behavior (they'd fail if the relay-aware branch were reverted), which is what actually matters.
  • [NIT] public/roles.js:462String(role).toLowerCase() is called twice for the infra check. Extract to var normRole = String(role || '').toLowerCase(); once. Cosmetic.
  • [NIT] public/live.js:3095 — comment says "WS-only non-infra nodes are removed" but the condition is n._fromAPI || isInfra → else. The current comment is correct; just verifying the reviewer trace. No change needed.

Persona votes

  • carmack: correctness is tight — signature is backward-compatible (accepts both (role, ms) and (node)), !(effectiveMs >= relayedMs) correctly handles the NaN/undefined case, infra gate uses lowercased role, thresholds unchanged. Ship it.
  • tufte: dim-not-delete is the right call — a 0.35-opacity marker preserves "infrastructure exists here, advert stale" as data-ink instead of silently rewriting the map. Consistent with the existing API-loaded dim path. No visual clutter concern at typical mesh sizes.
  • kent-beck: TDD verdict — tests are assertive (13 new getNodeStatus cases + 1 new relay-aware pruneStaleNodes case, all assert.strictEqual on real outcomes), cover legacy signature, node-object signature, _liveSeen precedence, _lastHeard precedence, companion exclusion, missing-role default, and no-timestamps. Would fail if the relay-aware branch were reverted. Not a textbook red→green history (see MINOR above), but the tests DO gate the behavior, which is the point.
  • meshcore: last_relayed semantics honored correctly — only repeater/room roles get the max(advert, last_relayed) treatment; companions are explicitly excluded, matching liquidraver's collision caveat on last_relayed being a repeater-hop marker. Threshold (infraSilentMs) unchanged. Correct.

Merge gate

  • mergeable: MERGEABLE
  • mergeState: BLOCKED
  • CI: action_required (community first-timer — needs maintainer to approve workflow runs)
  • Tests: present, assert behaviortest-frontend-helpers.js covers the new signature and the relay-aware branch end-to-end

Automated polish review. Human maintainer makes the final call. This is a clean, well-tested change; the MINORs are follow-up material, not merge blockers.

@SaarMesh-Bot

Copy link
Copy Markdown

Cross-referencing today's triage rounds on #1611, since they land on this PR.

The #1611 re-triage this morning concluded that the cadence-adaptive threshold should be split out as a separate follow-up, and that the #1598 r6 fix is the critical path to ship first — assessed at effort S, described as a Math.max on a field /api/nodes already returns.

That implementation is this PR. Open since 2026-07-01, bot review MERGE-READY since 2026-07-02, implementing all three locked r6 decisions: max(advert, last_relayed) for infra roles, dim-not-delete in pruneStaleNodes, in-place getNodeStatus signature extension across all seven call sites, plus 16 unit tests including the backbone-repeater fixture from the triage.

mergeable_state is blocked — the CI run sits at action_required pending workflow approval for a fork PR. Nothing else is outstanding on it as far as I can tell.

No rush intended; flagging it only because the design discussion has now converged on this change three times today across #1611, #1845 and #1598, and it may not be obvious that the code is already sitting here. Happy to rebase if it has drifted against master.

@Kpa-clawbot

Copy link
Copy Markdown
Owner

Delta re-review — 37b6a1ee

Only new commit is a clean merge of origin/master into the branch. No author code change since prior review (2026-07-02, verdict: MERGE-READY, 0 BLOCKER / 0 MAJOR, 3 MINOR follow-ups only).

Verdict: still merge-ready. Prior findings unchanged.

Merge gate: mergeable=MERGEABLE · mergeStateStatus=BLOCKED · CI = action_required (first-time contributor — workflow runs need maintainer approval). Not auto-mergeable; operator action required to release CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants