Skip to content

fix(nodes): bust in-flight /nodes on advert refresh (#279) - #306

Merged
dborup merged 4 commits into
masterfrom
codex/issue-279-nodes-ws-bust
Oct 6, 2026
Merged

dborup merged 4 commits into
masterfrom
codex/issue-279-nodes-ws-bust

Conversation

@dborup

@dborup dborup commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Relates to #279

Problem

public/nodes.js auto-refreshes the node list when ADVERT packets arrive over the WebSocket (#131). Both of that handler's refresh paths did invalidateApiCache('/nodes') followed by a plain loadNodes(true).

invalidateApiCache() clears only the TTL cache — it does not touch api()'s _inflight map. So if a /nodes request was already in flight when the advert arrived, the refresh coalesced onto it. The server may have answered that request before the advert, so the list could render without the node that just advertised, until the next refresh.

This is the same shape as #243 on /channels. The #243 fix (#263) lives at the call site — api(path, { bust: true }) — not inside invalidateApiCache(), so it never reached this path. Found as N3 in the round-2 re-review of #263.

Plan

  1. loadNodes() takes an opts.bust and passes it to api(); both WS refresh paths use it.
  2. A new unit test drives the real nodes.js + the real app.js api() with every /api/nodes fetch parked on its own deferred, so the test decides when and in which order answers land. Three mutants.
  3. A direct test of the revoke path's bust in channels.js (N1 of the same review), which was only pinned transitively.

Why the call site and not invalidateApiCache()

Both options were on the table in #279. This PR keeps the call-site bust, for three reasons:

  • invalidateApiCache(prefix) is prefix-based and global. invalidateApiCache('/nodes') also matches /nodes/<pubkey>, /nodes/<pubkey>/health, /nodes/search?… and every region/area variant. Dropping all of those from _inflight would make unrelated concurrent callers each issue their own fetch — a new request-amplification regression on every existing caller of invalidateApiCache, on a path whose only job today is the TTL cache.
  • It is a strictly weaker guarantee. Clearing _inflight only stops the refresh from joining a request that exists at that instant. bust guarantees the refresh itself always fetches, whatever started in between.
  • api()'s fix(app): a requested refresh can coalesce onto an older in-flight api() request (invalidateApiCache does not clear _inflight) #243 ownership checks are built around the bust taking the slot — the cache-write check and the .finally() check both compare _inflight.get(key) === promise. A refresh that only deletes the entry leaves no newer owner, so a later plain caller fetches a third time instead of joining. Keeping one mechanism keeps /channels and /nodes behaving identically.

bust is passed unconditionally from the handler, matching refreshChannelList() in channels.js. It is a no-op on the in-place branch (an advert for a node already in _allNodes updates it in memory and loadNodes() makes no request at all), which a test pins.

Tests

The tests were committed first (106f8697) and are red on master. The fix is in 4dbd20a9.

Test Checks Red on master
test-nodes-advert-ws-bust-279.js (new, 7) Real nodes.js + real app.js api(), parked fetches. An advert during the first load refetches instead of joining it and the advertising node ends up in the list (_allNodes null path). The same for an advert naming an unknown node while a load is in flight (needReload path). A load started after the refresh joins the refresh. Guards: the handler mounts and the first load makes one request; a known-node advert updates in place with no request; nothing in flight → exactly one request; a non-advert batch makes no request 3 of 7 (the 4 guards pass)
test-channels-client-state-152.js (+3) Direct revoke-path tests: a revoke while a /channels request is in flight drops the channel, in both answer orders, with one request per load and the post-revoke answer in the TTL cache; a revoke with nothing in flight makes exactly one request green: pins existing behaviour (the N1 test gap)

test-nodes-advert-ws-bust-279.js is registered in test-all.sh (test-test-all.js enforces that).

Mutants

Each mutant was applied to the fixed code and run against test-nodes-advert-ws-bust-279.js, test-channels-client-state-152.js and test-frontend-helpers.js.

Mutant Killed by
M1 nodes.js: _allNodes-null branch loses bust nodes-279 2 ✗
M2 nodes.js: needReload branch loses bust nodes-279 1 ✗
M3 nodes.js: loadNodes() drops the opts.bust passthrough to api() nodes-279 3 ✗
M4 channels.js: onRevoked re-split into its own non-busting invalidateApiCache('/channels'); loadChannels(true) (the mutant that survived in the #263 review, N1) channels-152 2 ✗
M5 channels.js: refreshChannelList() drops bust (the #263 M6, re-run) channels-152 4 ✗ (2 existing #243 + 2 new #279)

Scope

public/nodes.js only, plus the two test files and test-all.sh. cmd/server and every other Go module are untouched. No new map[string]interface{}, no colours. scripts/check-xss-sinks.sh --diff origin/master is clean. The workflow files are byte-identical to origin/master, so the fork guards are unchanged (9 in deploy.yml, 1 in release-fast-path.yml).

Residuals from the #263 review, still open

  • N2 (channels.js, pre-existing fix(channels): hide revoked shared channels from the channel list (#251) #257 gap): dropping invalidateApiCache('/channels') from refreshChannelList() still survives. It survives the equivalent mutation on origin/master too, so it is not introduced here, and it is out of this issue's scope. The same gap now exists on the /nodes side: with bust in place, invalidateApiCache('/nodes')'s only remaining job is the other region/area/detail variants, and no test covers that cross-variant invalidation.

🤖 Generated with Claude Code

dborup and others added 2 commits October 6, 2026 08:54
…quest (#279)

The three tests that need the fix are red on master: an advert arriving
while a /nodes request is in flight joins that older request, so the node
that just advertised is missing from the list.

Also adds a direct test of the revoke path's bust in channels.js (N1 of
the #263 round-2 review), which was only pinned transitively through the
shared refreshChannelList().

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
loadNodes() takes an opts.bust that it passes to api() for every page, and
both refresh paths of the advert WS handler use it. invalidateApiCache()
clears only the TTL cache, so without this the refresh joined a /nodes
request that was already in flight -- one the server may have answered
before the advert arrived.

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

dborup commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Rapport — CS-Minimax PR#306 #279 — head 4dbd20a

Status: Done — both requirements implemented, 5 mutants killed (one of them the mutant that survived in the #263 round-2 review), every CI job green on the first run, no residuals introduced.

Requirement 1 — the nodes advert WS handler must not coalesce onto an in-flight /nodes request

public/nodes.js: loadNodes() takes an opts.bust and passes it to api() for every page of the pagination loop; both refresh paths of the advert WS handler (nodes.js:708-718 and nodes.js:746) pass { bust: true }.

Choice made: the call site, not invalidateApiCache(). The reasoning is in the PR body. Short version: invalidateApiCache(prefix) is prefix-based and global — invalidateApiCache('/nodes') also matches /nodes/<pubkey>, /nodes/<pubkey>/health, /nodes/search?… and every region/area variant — so clearing _inflight there would make unrelated concurrent callers each issue their own fetch, a request-amplification regression on every existing caller. It is also a strictly weaker guarantee (it only stops a join against what exists at that instant, not against what starts next), and api()'s #243 ownership checks are built around the bust taking the slot, so a refresh that merely deletes the entry leaves no newer owner for a later plain caller to join. [A]

Item Test Red before Green after Mutant
bust on the _allNodes-null path (nodes.js:716) test-nodes-advert-ws-bust-279.js — "an advert during the first load refetches instead of joining it" ✗ (1 fetch, not 2) [T] ✅ [T] M1 loadNodes(true, { bust: true }) → loadNodes(true) on that branch → 2 ✗ [T]
bust on the needReload path (nodes.js:746) same file — "an advert for an unknown node refetches instead of joining an in-flight load" ✗ (1 fetch, not 2) [T] ✅ [T] M2 same mutation on that branch → 1 ✗ [T]
the opts → api() passthrough same file — "a load started after the advert refresh joins the refresh, not the superseded request" ✗ [T] ✅ [T] M3 api(…, { ttl, bust: !!(opts && opts.bust) }) → api(…, { ttl }) → 3 ✗ [T]
guards (no behaviour change) 4 more in the same file: the handler mounts and the first load makes one request; a known-node advert updates in place and makes no request; nothing in flight → exactly one request; a non-advert batch makes no request green on master (that is the point) [T] ✅ [T] covered by M1-M3 above

7 tests total, 3 of them red on origin/master, 4 green there as guards. [T]

The suite loads the real public/nodes.js and the real public/app.js, so api()'s in-flight dedup, TTL cache and bust are the production ones; only fetch is stubbed, and every /api/nodes fetch is parked on its own deferred so each test decides when and in which order answers land. [T]

Requirement 2 — direct test of the revoke-path bust (N1 of the #263 round-2 review)

Item Test State Mutant
onRevoked's bust is pinned directly, not transitively test-channels-client-state-152.js +3: a revoke while a /channels request is in flight drops the channel, in both answer orders, with one request per load and the post-revoke answer in the TTL cache; and a revoke with nothing in flight makes exactly one request green before and after — it closes a test gap, so the mutant is the evidence [T] M4 onRevoked: refreshChannelList → its own non-busting invalidateApiCache('/channels'); loadChannels(true) (this is the mutant that survived all three suites in the #263 review) → 2 ✗ [T]
the shared implementation stays pinned same file — M5 (= #263's M6, re-run) refreshChannelList() drops bust → 4 ✗: 2 existing #243 tests + 2 new #279 tests [T]

Each mutant was applied to the fixed code and run against test-nodes-advert-ws-bust-279.js, test-channels-client-state-152.js and test-frontend-helpers.js; the files not listed above stayed green for every mutant, which is what makes the attribution specific. [T]

CI per job (run on head 4dbd20a9, first attempt, no reruns)

Job Result
Go Build & Test ✅ success [K]
Playwright E2E Tests ✅ success [K]
Build & Publish Docker Image ✅ success [K]
Release Artifacts skipped [K]
Deploy Staging skipped [K]
Publish Badges & Summary skipped [K]

The known-flaky test from #271 did not fire; no job was rerun. [K]

Local runs (head 4dbd20a9)

Constraints

  • cmd/server untouched: the whole diff is public/nodes.js, test-nodes-advert-ws-bust-279.js, test-channels-client-state-152.js, test-all.sh. [T]
  • No new map[string]interface{} anywhere in the diff. [T]
  • No hardcoded colours: the added lines in public/ contain no hex, rgb() or hsl() literal. [T]
  • scripts/check-xss-sinks.sh --diff origin/master: clean, exit 0, no findings. [T]
  • Fork guards unchanged — the workflow files are byte-identical to origin/master (git diff origin/master HEAD -- .github/ is empty), and the counts still read 9 in deploy.yml and 1 in release-fast-path.yml. [T]

Residuals

@dborup-agent

Copy link
Copy Markdown
Collaborator

Review — CS-pve-agent1 PR#306 — head 4dbd20a

Dom: APPROVE med nits

Independent, read-only review. Merged tree = git merge-tree --write-tree origin/master 4dbd20a9 (origin/master 03f9a9d6), tree 2dd7c6cc, no conflicts. Evidence tags: [T] I ran it, [A] code reading/analysis, [K] taken from CI / GitHub.

Findings

# Severity Finding Evidence
1 Minor, pre-existing, non-blocking _allNodes is still last-writer-wins. If the superseded request's answer lands after the bust's answer, the older loadNodes() overwrites _allNodes and the node that just advertised disappears again. The bust guarantees a fresh request, not that the fresh answer wins. On master the same scenario never even fetched, so this is strictly better than before, and the author flags it as a residual. Follow-up candidate: a request-id token like channels.js got in #225. Unit probe P4 (older answer lands last → list ["Alpha"]) [T]; browser, PR frontend, oldLast scenario → probe node missing, 2 requests [T]
2 Nit (test gap) The new suite only uses single-page answers, so nothing pins that the bust applies to every page. Mutant R3 (bust only when offset === 0) survives all 7 new tests; on a >500-node deployment the refresh's page 2 would join the superseded load's page 2. R3 survives test-nodes-advert-ws-bust-279.js (7/7) and test-frontend-helpers.js (709/709); killed by my multipage probe P5 [T]
3 Nit (test gap) Nothing pins that plain (non-WS) loadNodes() calls still coalesce. Mutant R4 (bust: true unconditionally in loadNodes) survives every suite, and it would turn every region/area/tab/search load into an uncoalesced fetch. The implementation is right (!!(opts && opts.bust)); only the guard is missing. R4 survives the PR suite and frontend-helpers; killed by my probe P6 (init + 2 region changes → must stay 1 request) [T]
4 Info The PR's harness replaces debouncedOnWS with a passthrough and runs setTimeout at 0 ms, so the 5 s coalescing is not exercised by the new tests. I measured it separately (point 4 below); it holds. [T]

Nothing blocking. Nits 2 and 3 are each a ~15-line addition to the existing harness (two-page answers; a captured RegionFilter.onChange callback).

Point-by-point

1. The fix. [A] The bust stays at the call site. loadNodes(refreshOnly, opts) forwards bust: !!(opts && opts.bust) to api() for every page of the pagination loop, and both WS refresh paths pass { bust: true } (public/nodes.js:718 for _allNodes null, :746 for needReload / in-place). Those are the only two invalidate-then-reload sites in nodes.js. The other loadNodes() callers (:679, :680, :690, :701, :1653, :1657, :1665, :1675) are user-driven and correctly stay non-busting. invalidateApiCache() is unchanged, so its other callers are unaffected: app.js:948-949 (the 5 s WS cache-invalidation timer for /stats and /nodes) and channels.js:2059 (refreshChannelList, already busting since #263). No extra requests and no lost coalescing for them. On the in-place branch loadNodes() makes no request, so the unconditional bust there is a no-op, and a guard test pins this [T]. I also tested the alternative design the issue offered (mutant R6: invalidateApiCache also clears _inflight, call sites reverted to plain). It passes all 7 new tests but fails my multipage probe P5, because only the first page sees the cleared slot. That confirms the author's "strictly weaker guarantee" argument [T].

2. Tests. test-nodes-advert-ws-bust-279.js loads the real nodes.js and the real app.js api(), with every /api/nodes fetch parked on a deferred, so a request is in flight when the advert arrives. On master it fails 3 of 7 (the _allNodes-null path, the needReload path, and the "later load joins the refresh" test); on the merged tree it passes 7 of 7. The 4 guards are green on both [T]. My own mutants are in the table below.

3. N1 (onRevoked bust). Yes, there is now a direct test. test-channels-client-state-152.js adds 3 tests: a revoke during an in-flight region load, in both answer orders, plus a revoke with nothing in flight. Mutant C1 (onRevoked re-split into a non-busting invalidateApiCache('/channels'); loadChannels(true)) is killed by both in-flight tests (73/75). Mutant C2 (onRevoked busts but skips invalidateApiCache) is killed by the existing #276 test (74/75). The revoke path is therefore pinned on both halves of refreshChannelList() [T].

4. Rapid adverts. This uses the real app.js debouncedOnWS on a virtual clock [T]:

Scenario master merged
P1: 20 unknown-node adverts in 1 s, list loaded 1 request 1 request
P2: 20 adverts in 1 s while the first load is in flight 0 extra (joins, which is the bug) 1 extra
P3: 1 advert/s for 20 s, server never answers 1 (joins the hung request) 4 (one per 5 s window)

The browser gives the same result: 20 injected adverts in 1 s lead to exactly 1 /nodes request after injection, both with and without a held initial load [T]. In the worst case (a hung server plus a sustained advert stream), the extra load is bounded at one request per 5 s debounce window. That matches refreshChannelList() and is not a storm.

5. Browser. I used Playwright/Chromium against a local Go server on the CI-prepared fixture: the PR frontend on one port and master's public/ on another, with the same binary and the same DB. Adverts were injected through a captured page WebSocket (onmessage). page.route held /api/nodes and added a synthetic ZZ-279-Probe node to every answer after the first [T]:

Scenario master frontend PR frontend
Advert during a held first load, older answer lands first 1 request, probe missing (bug reproduced) 2 requests, probe listed at the top of #nodesBody (205 rows)
Same, older answer lands last 1 request, missing 2 requests, missing (finding 1)
20 adverts in 1 s during a held first load 0 extra requests, missing 1 extra request, listed
20 adverts in 1 s, nothing held 1 extra request, listed 1 extra request, listed

data-loaded="true" was set in every run, and there were 0 console errors and 0 page errors across all 8 runs [T].

Tests and mutants

Runs on the merged tree:

  • node test-nodes-advert-ws-bust-279.js: 7/7. On master: 4/7, with the expected 3 red [T].
  • node test-channels-client-state-152.js: 75/75, stable over 3 standalone runs [T].
  • node test-frontend-helpers.js: 709 passed, 0 failed [T].
  • sh test-all.sh: 223/223 files passed on an idle machine. An earlier run with the Go suites running in parallel had one pre-existing timing test fail (N1: a remap mid-decrypt restarts loading… in test-channels-client-state-152.js, present on master and not touched by this PR). That test is 75/75 in 3 standalone runs and passes in the rerun [T].
  • cmd/server go test ./...: ok (576 s, -timeout 20m as in CI; a first run in parallel with the ingestor suite hit the 10 m default timeout) [T].
  • cmd/ingestor go test ./...: ok (682 s, -timeout 20m; same note) [T].
  • E2E against a local Go server with e2e-fixture.db, prepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL, corescope-migrate, then seeds 2073, 199 and 245). The server was stopped by pid/port afterwards. The PR adds no E2E of its own; these are the nodes-affected ones [T]:
    • test-e2e-playwright.js: 132/135 passed, 3 skipped (matches CI). Note: a git archive tree has no .git, so the server reports commit: "unknown" and the "Version info lives on Perf dashboard" test times out, on master's frontend too. With a .git-commit file it passes, so this is environmental
    • test-nodes-export-e2e.js: OK (182 contacts)
    • test-map-nodes-pagination-e2e.js: 3/3
    • test-node-liveness-e2e.js: 35 checks passed
    • test-nodes-favorite-rerender-e2e.js: 4/4
    • test-issue-1758-ng-filter-rerenders-e2e.js: PASS
    • test-issue-2073-recent-adverts-e2e.js: 10/10
    • test-issue-245-advert-intervals-e2e.js: 7/7
    • test-issue-2027-my-mesh-node-page-e2e.js: 25/25
    • test-channels-client-state-152-e2e.js: 6/6

Own mutants, each applied to the merged tree:

Mutant PR suite (…-279.js) frontend-helpers Own probes Verdict
R1 both WS sites loadNodes(true, { bust: false }) 3 ✗ 709 ✓ 3 ✗ killed
R5 api() ignores bust for the in-flight join 3 ✗ 709 ✓ 3 ✗ killed
C1 onRevoked non-busting re-split channels-152: 2 ✗ — — killed
C2 onRevoked busts without invalidateApiCache channels-152: 1 ✗ (#276) — — killed
R3 bust only on offset === 0 7 ✓ 709 ✓ P5 ✗ survives the PR suite (finding 2)
R4 bust: true unconditionally in loadNodes 7 ✓ 709 ✓ P6 ✗ survives the PR suite (finding 3)
R6 alternative fix: invalidateApiCache clears _inflight, call sites plain 7 ✓ 709 ✓ P5 ✗ design check, see point 1
R2 both WS sites loadNodes(false, { bust: true }) 7 ✓ 709 ✓ ✓ equivalent for this scope: refreshOnly only picks renderRows() vs renderLeft() [A]

(The "own probes" column excludes P4, which fails on every variant, the fix included. It is finding 1.)

Constraints

  • Diff scope. The diff touches only public/nodes.js, test-nodes-advert-ws-bust-279.js, test-channels-client-state-152.js and test-all.sh. Nothing under cmd/ or .github/ differs between origin/master and the merged tree, so cmd/server stays read-only [T].
  • No new map[string]interface{}. There are 0 added occurrences [T].
  • No hardcoded colours. The only hex-like matches on added lines are issue references (#279, #243) [T].
  • XSS check clean. scripts/check-xss-sinks.sh --diff origin/master on head exits 0 with no findings [T].
  • Fork guards unchanged. github.repository == 'Kpa-clawbot/CoreScope' appears 9 times in deploy.yml and 1 time in release-fast-path.yml. Both files are byte-identical to origin/master [T].
  • No closing keywords. The PR body says "Relates to", and neither commit message contains a closing keyword [T].
  • Commit author. Both commits have author and committer dborup <kontakt@meshview.dk> [T].
  • No behaviour change beyond the purpose. Non-WS loadNodes() callers still coalesce (P6), the in-place advert branch makes no request (PR guard), and a non-advert batch makes no request (PR guard) [T].
  • CI on head 4dbd20a9, per job [K]:

Not verified

  • I did not inject a real ADVERT end-to-end (MQTT → ingestor → server WS → page). The browser check simulates the WS frame inside the page, and no ingestor ran.
  • I did not test a deployment with more than 500 nodes in the browser. Multipage behaviour was covered only by the unit probe P5.
  • I did not review map.js:790 (a plain loadNodes() on advert, with no invalidate). It is outside Nodes advert WS handler: invalidate-then-loadNodes(true) can coalesce onto an in-flight /nodes request (same shape as #243) #279's scope, but it may have a sibling of the same coalescing shape. That needs reading before anyone files it.
  • I did not run the remaining Go modules, -race, the full CI E2E list, or frontend coverage.
  • My probes and mutant files live in my scratch area only. Nothing was pushed to the PR.

dborup added 2 commits October 6, 2026 11:52
…279)

Round-2 review nits 2 and 3 on PR #306: the suite only used single-page
answers and never drove a plain, non-WS loadNodes(), so two mutants
survived all 7 tests.

- A two-page scenario: the superseded load and the advert refresh each
  walk the pagination loop, and the refresh must bust page 2 as well.
  Kills 'bust only when offset === 0' (R3).
- A plain load via the captured RegionFilter.onChange callback must still
  coalesce onto an in-flight /nodes request. Kills an unconditional
  'bust: true' in loadNodes() (R4).

public/nodes.js is unchanged.
@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Rapport — CS-Macmini PR#306 runde 2 — head 93dfae3

Review feedback addressed (commit cec98a5e)

Round 2 covers the two test gaps from "Review — CS-pve-agent1 PR#306" (APPROVE med nits). public/nodes.js is byte-identical to 4dbd20a9 — the only change is +105/−3 in test-nodes-advert-ws-bust-279.js. Neither new test found a production defect, so nothing in the implementation moved. origin/master (03f9a9d6) is merged in as 93dfae36, no conflicts.

Evidence tags: [T] I ran it, [A] code reading/analysis, [K] taken from CI.

1. Finding 2 (nit, test gap) — the bust must apply to every page, not just offset=0

Fixed. New test: "the bust applies to every page of a multi-page list, not just offset=0".

The suite only ever answered with single-page bodies, so loadNodes()'s pagination loop ran exactly one iteration and nothing could tell a per-page bust from a first-page-only one. The new test drives two full pages through the loop:

  1. init() parks the initial load's offset=0 request.
  2. An advert arrives → the bust parks a second offset=0 request.
  3. The superseded load's page 1 is answered with a full 500-node page, so it walks on and parks its offset=500 request — in flight before the refresh reaches the same offset.
  4. The refresh's page 1 is answered with a full page. It must now start its own offset=500 request rather than join the one already parked.
  5. Pre-advert page 2 lands first; the refresh's own page 2 lands last and is the only one carrying the advertising node.

Assertions: two offset=0 fetches, two offset=500 fetches, the advertising node present, and 502 nodes accumulated. The fetch lookup matches offset=<n> as an exact URL tail so limit=500 cannot be mistaken for it. [T]

Mutant Result
R3 bust: !!(opts && opts.bust) && offset === 0 killed — test-nodes-advert-ws-bust-279.js 8/9, the sole failure being the new multi-page test (the refresh must bust page 2 as well instead of joining the superseded load's page 2 (got 1)). Every other test in the file, including the 7 pre-existing ones, stays green [T]

2. Finding 3 (nit, test gap) — plain, non-WS loadNodes() must still coalesce

Fixed. New test: "plain loadNodes() calls outside the WS handler still coalesce", built on the captured RegionFilter.onChange callback the review suggested.

The harness now captures the callbacks init() hands to RegionFilter.onChange and AreaFilter.onChange instead of discarding them. The test holds the initial /nodes request in flight, fires the region-change callback twice (each one drops _allNodes and re-runs loadNodes() with no opts), and asserts the fetch count is still 1 — the plain loads joined. It then answers that single request, checks the shared answer rendered, and finally fires an advert to confirm the WS refresh is still the exception and does fetch. [T]

Mutant Result
R4 bust: true unconditional in loadNodes() killed — 8/9, the sole failure being the new plain-load test (plain loads must join the in-flight request, not fetch again (got 3)): init + 2 region changes become 3 separate fetches [T]

Attribution is 1:1 in both directions — each mutant kills exactly one test, and it is the test written for it. [T]

3. Finding 1 (minor, pre-existing) — _allNodes last-writer-wins

Not fixed; out of scope, and I agree with the review's own framing. [A] The nodes page has no request-id token of the kind channels.js got in #225, so two overlapping loads still resolve last-writer-wins on _allNodes. bust guarantees a fresh request, not that the fresh answer wins. The scenario the reviewer measured (probe P4 / browser oldLast) is a genuine gap, but it is a separate mechanism from #279's and it is strictly better than master, where the refresh never fetched at all. Fixing it means adding a generation counter to loadNodes() and discarding stale answers — a behaviour change to the production load path, which this round was explicitly not to make.

Consistent with that, the new multi-page test lands the refresh's answer last and asserts on fetch counts as well as content, so it measures the bust and not the ordering gap. Worth a follow-up issue covering the nodes page the way #225 covered channels. [A]

4. Finding 4 (info) — the 5 s debounce is not exercised by the unit suite

Acknowledged, no change. [A] The harness replaces debouncedOnWS with a passthrough and runs setTimeout at 0 ms, deliberately: these tests are about api()'s in-flight behaviour, and a virtual clock on top would add a second variable per test. The reviewer measured the debounce separately (P1–P3, plus the browser runs) and it holds — worst case one extra request per 5 s window, matching refreshChannelList(). Nothing here contradicts that, so I left the harness as it is rather than duplicating a check that was already made.

Tests

Suite Result
node test-nodes-advert-ws-bust-279.js 9 passed, 0 failed (7 existing + 2 new); no swallowed Failed to load nodes / TypeError in its output [T]
sh test-all.sh 223 / 223 files, 0 failed, on an idle machine [T]
node test-frontend-helpers.js 709 passed, 0 failed [T]
cmd/server go test -timeout 25m -race ./... ok, 445.6 s, exit 0, no WARNING: DATA RACE [T]
cmd/ingestor go test -timeout 25m ./... ok, 104.1 s, exit 0 [T]
The other 16 Go modules all ok; internal/geofilter, internal/mbcapqueue, internal/perfio have no test files [T]

E2E against a local Go server (port 13801) on e2e-fixture.db, prepared exactly as CI does — freshen → the Kpa-clawbot#1486 / Kpa-clawbot#1791 seed SQL → corescope-migrate → seeds 2073 / 199 / 245 — with public-instrumented. The server was stopped by pid and port 13801 confirmed free afterwards; the fixture DB was restored and the build artifacts removed, so the worktree is clean. [T]

E2E file Result
test-e2e-playwright.js 132/135 passed, 3 skipped (matches CI)
test-node-liveness-e2e.js 35 checks passed
test-map-nodes-pagination-e2e.js 3/3
test-nodes-export-e2e.js OK (182 contacts, prefix-narrow → exactly 1)
test-nodes-favorite-rerender-e2e.js 4/4
test-issue-1758-ng-filter-rerenders-e2e.js PASS
test-issue-2073-recent-adverts-e2e.js 10/10
test-issue-245-advert-intervals-e2e.js 7/7
test-issue-2027-my-mesh-node-page-e2e.js 25/25
test-channels-client-state-152-e2e.js 6/6

Unlike the reviewer's git archive run, this worktree has a .git, so the "Version info lives on Perf dashboard" test passed rather than timing out — confirming that failure was environmental. [T]

Mutants

Mutant PR suite frontend-helpers Verdict
R3 bust only when offset === 0 8/9 ✗ (multi-page test) — killed (was: survives 7/7) [T]
R4 bust: true unconditional in loadNodes() 8/9 ✗ (plain-load test) — killed (was: survives 7/7) [T]

One mutant per finding, each applied to the fixed tree and reverted afterwards; git diff --stat public/nodes.js is empty at head. The round-1 mutants M1–M5 are unaffected — the production code they target did not change. [T]

CI per job — run 37446052762 on head 93dfae36, first attempt, no reruns

Job Result
Go Build & Test ✅ success [K]
Playwright E2E Tests ✅ success [K]
Build & Publish Docker Image ✅ success [K]
Release Artifacts skipped [K]
Deploy Staging skipped [K]
Publish Badges & Summary skipped [K]

Neither known flake (#271, #301) fired, so no job was rerun. [K]

Constraints

  • Diff scope. vs origin/master: public/nodes.js, test-all.sh, test-channels-client-state-152.js, test-nodes-advert-ws-bust-279.js. The round-2 commit alone touches only test-nodes-advert-ws-bust-279.js (+105/−3). Nothing under cmd/ [T]
  • No new map[string]interface{}: 0 added occurrences [T]
  • No hardcoded colours: the only hex-like match on added lines is the issue reference #1540 [T]
  • XSS check: scripts/check-xss-sinks.sh --diff origin/master exits 0, no findings [T]
  • Fork guards unchanged: git diff origin/master HEAD -- .github/ is empty; counts still 9 in deploy.yml, 1 in release-fast-path.yml [T]
  • Commit author: both cec98a5e and the merge 93dfae36 are authored and committed by dborup <kontakt@meshview.dk> [T]
  • No closing keywords in either commit message; the PR stays a draft [T]

Residuals, unchanged from round 1

@dborup-agent

Copy link
Copy Markdown
Collaborator

Review — CS-pve-agent1 PR#306 — head 93dfae3

Dom: APPROVE

This is an independent, read-only re-review of round 2. In round 1 (head 4dbd20a9) I gave APPROVE with nits, and this round checks nits 2 and 3. The merged tree is git merge-tree --write-tree origin/master 93dfae36 against origin/master 30c7de46. It gives tree 85e20d88, with no conflicts. The head was 93dfae36 in git ls-remote both before and after the review. Evidence tags: [T] I ran it, [A] code reading or analysis, [K] taken from CI or GitHub.

Findings

# Severity Finding Evidence
1 Resolved (was nit 2) The new multi-page test kills R3 (bust only when offset === 0). It fails as the only red test in the file, with the refresh must bust page 2 as well … (got 1). 8/9 under R3 [T]
2 Resolved (was nit 3) The new plain-load test kills R4 (bust: true unconditionally). It fails as the only red test, with plain loads must join the in-flight request … (got 3). 8/9 under R4 [T]
3 Info, non-blocking The coalescing guard drives only the region callback. A bust added to the area callback (R10) or to the search input (R11) survives every suite. R4 still catches the general regression inside loadNodes(), so the remaining exposure is a deliberate per-call-site edit. The harness already captures h.areaChange, so a single extra h.areaChange() call in the same test would cover the area path. R10 and R11: 9/9 and 709/709 [T]
4 Minor, pre-existing (round-1 finding 1, unchanged) _allNodes is last-writer-wins. The bust guarantees a fresh request, not that the fresh answer wins. The author agrees this is out of scope, and the new multi-page test deliberately lands the refresh last. A request-id/generation token like the one #225 added to channels.js is still the follow-up candidate. [A]

Nothing blocks the PR. Findings 3 and 4 are follow-up material.

Point-by-point

1. Nit 2: the bust applies to every page. [T] The test drives two full 500-node pages through the loadNodes() pagination loop. The superseded load's page 2 is parked before the refresh reaches offset 500, so a first-page-only bust would join it. The test asserts two offset=0 fetches, two offset=500 fetches, that the advertising node is present, and 502 accumulated nodes. I re-ran R3 on the merged tree: test-nodes-advert-ws-bust-279.js 8 passed / 1 failed, with the multi-page test as the only failure, and test-frontend-helpers.js 709/709. Killed. I also checked the fetchesAtOffset() helper [A]. endsWith('offset=' + n) cannot confuse limit=500 or offset=500 with offset=0, and offset really is the last parameter that baseParams carries, because set() keeps the position of its first insertion.

2. Nit 3: plain loads still coalesce. [T] The harness now captures the RegionFilter.onChange and AreaFilter.onChange callbacks. The test fires the region callback twice while the initial load is in flight. That callback is nodes.js:679, which drops _allNodes and calls loadNodes() with no opts. The test then asserts there is still exactly 1 fetch, that the shared answer renders, and that a subsequent advert still adds exactly one fetch. I re-ran R4: 8 passed / 1 failed, with the plain-load test as the only failure (got 3). Killed. The test is a guard and is green on master as well, which is expected, since the property it protects predates the PR.

3. No production change since round 1. [T]

  • git diff 4dbd20a9 cec98a5e touches only test-nodes-advert-ws-bust-279.js (+105/−3).
  • git diff 4dbd20a9 93dfae36 -- public/ also shows public/packet-path-map.js and public/ping-scores.js. Both files are byte-identical to 03f9a9d6, the master commit merged in by 93dfae36, so they come from the master merge and not from this PR.
  • Against its own merge base (03f9a9d6..93dfae36 -- public/), the PR touches only public/nodes.js. That file is byte-identical to round 1, and to the merged tree.

4. CI per job. [K] There is one run on head 93dfae36, run 37446052762, attempt 1 with no reruns:

  • Go Build & Test: success
  • Playwright E2E Tests: success
  • Build & Publish Docker Image: success
  • Release Artifacts, Deploy Staging and Publish Badges & Summary: skipped, as expected for this PR

None of the known flakes (#256 Hash Stats sort, #267 backfill write-hold) fired.

Acceptance criteria (#279)

Criterion Test Red on master Green on merged
The WS advert refresh does not join an in-flight /nodes request (_allNodes null path) nodes-279 test 2 ✗ ✓ [T]
The same on the needReload path nodes-279 test 3 ✗ ✓ [T]
A later load joins the refresh, not the superseded request nodes-279 test 4 ✗ ✓ [T]
The bust covers every page (new) nodes-279 multi-page test ✗ ✓ [T]
A direct onRevoked bust test (N1) channels-152 +3 pins existing behaviour; killed by C1 and C2 in round 1 ✓ [T]

When I run the PR's test file against master's nodes.js, I get 5 passed / 4 failed. The 4 red tests are exactly the bug tests above, and the 5 guards are green [T].

Tests (merged tree, origin/master 30c7de46)

  • node test-nodes-advert-ws-bust-279.js: 9/9 on head and on the merged tree [T]
  • sh test-all.sh: 225/225 files, 0 failed [T]
  • node test-frontend-helpers.js: 709 passed, 0 failed [T]
  • cmd/server go test -timeout 20m ./...: ok (825 s) [T]
  • cmd/ingestor go test -timeout 20m ./...: ok (1012 s) [T]
  • E2E [T]. These ran against a local Go server built from the merged tree and serving public/, on e2e-fixture.db prepared as in CI: freshen, then the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486/"Group Data" message type missing from packet view window's "message type" filter Kpa-clawbot/CoreScope#1791 seed SQL extracted from deploy.yml, then corescope-migrate, then seeds 2073, 199 and 245. The server was stopped by its port-derived pid, and I confirmed the port was free afterwards. I added a .git-commit file because a git archive tree has no .git, as noted in round 1.
    • test-e2e-playwright.js: 132/135, 3 skipped (matches CI)
    • test-nodes-export-e2e.js: OK (182 contacts)
    • test-map-nodes-pagination-e2e.js: 3/3
    • test-node-liveness-e2e.js: 35 checks passed
    • test-nodes-favorite-rerender-e2e.js: 4/4
    • test-issue-1758-ng-filter-rerenders-e2e.js: PASS
    • test-issue-2073-recent-adverts-e2e.js: 10/10
    • test-issue-245-advert-intervals-e2e.js: 7/7
    • test-issue-2027-my-mesh-node-page-e2e.js: 25/25
    • test-channels-client-state-152-e2e.js: 6/6
    • test-issue-123-map-lifecycle-e2e.js: 15/15

Mutants

Each mutant was applied to public/nodes.js in the merged tree and reverted afterwards. After the last one, the file is byte-identical to head again [T].

Mutant …-279.js frontend-helpers Verdict
R3 bust: … && offset === 0 8/9 ✗ (multi-page) 709 ✓ killed (survived in round 1)
R4 bust: true unconditionally 8/9 ✗ (plain-load) 709 ✓ killed (survived in round 1)
R7 (own) bust: … && offset > 0 5/9 ✗ (all 4 bug tests) 709 ✓ killed
R9 (own) region callback calls loadNodes(false, { bust: true }) 8/9 ✗ (plain-load) 709 ✓ killed
R10 (own) area callback calls loadNodes(false, { bust: true }) 9/9 ✓ 709 ✓ survives (finding 3)
R11 (own) search input calls loadNodes(false, { bust: true }) 9/9 ✓ 709 ✓ survives (finding 3)
R8 (own) bust: !!refreshOnly 9/9 ✓ 709 ✓ equivalent: the only loadNodes(true…) callers are the two WS paths (:718, :746) [A]

The round-1 mutants (R1, R5, C1, C2, M1–M5) target production code that has not changed, so I did not re-run them.

Constraints

Not verified

  • I did not repeat the round-1 browser runs, because public/nodes.js is byte-identical to round 1. This round's browser coverage is only the E2E list above.
  • I did not run a real MQTT → ingestor → WS advert end-to-end, or test a deployment with more than 500 nodes in a browser. Multi-page behaviour is covered only by the unit test.
  • I did not run the other Go modules, -race, the full CI E2E list or frontend coverage.
  • I did not read map.js:790. It is still open from round 1 and outside Nodes advert WS handler: invalidate-then-loadNodes(true) can coalesce onto an in-flight /nodes request (same shape as #243) #279's scope.
  • The mutant files exist only in my scratch area. Nothing was pushed to the PR.

@dborup
dborup marked this pull request as ready for review October 6, 2026 12:47
@dborup
dborup merged commit 08393e7 into master Oct 6, 2026
6 checks passed
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