From 106f86976b9596959c636ac00cc3465a3fe1bbeb Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 08:54:03 +0200 Subject: [PATCH 1/3] test(nodes): pin the advert WS refresh against an in-flight /nodes request (#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 --- test-all.sh | 1 + test-channels-client-state-152.js | 66 ++++++ test-nodes-advert-ws-bust-279.js | 324 ++++++++++++++++++++++++++++++ 3 files changed, 391 insertions(+) create mode 100644 test-nodes-advert-ws-bust-279.js diff --git a/test-all.sh b/test-all.sh index 6f57b31bd..69be5b3d9 100755 --- a/test-all.sh +++ b/test-all.sh @@ -40,6 +40,7 @@ run test-issue-1065-gesture-hints-gates.js run test-frontend-helpers.js run test-app-api-inflight-cleanup-rejection.js run test-app-api-bust-inflight-243.js +run test-nodes-advert-ws-bust-279.js run test-issue-120-distance-building.js run test-privacy-page.js run test-nav-dynamic-link-lifecycle.js diff --git a/test-channels-client-state-152.js b/test-channels-client-state-152.js index 5963ad61b..2fdf6a030 100644 --- a/test-channels-client-state-152.js +++ b/test-channels-client-state-152.js @@ -1969,6 +1969,72 @@ async function test(name, fn) { assert.ok(h.row('#shared') && h.row('#shared').shared === true, 'the approved channel is listed'); }); + // ── #279 (N1 of the #263 round-2 review): pin the revoke path's bust ── + // + // `onApproved` and `onRevoked` both wire to refreshChannelList(), so the + // revoke path's bust was only pinned *transitively*: a change that + // re-splits the two hooks and gives `onRevoked` its own non-busting + // `invalidateApiCache('/channels'); loadChannels(true)` survived every + // unit suite. These two tests drive the revoke path directly. + const BEFORE_REVOKE = { channels: [serverChannel('public'), serverChannel('#shared')], approvedChannels: [{ hash: '#shared', name: '#shared' }] }; + const AFTER_REVOKE = { channels: [serverChannel('public')] }; + + // A region change is in flight (its answer was made before the revoke) + // when the revoke asks for a refresh. newestFirst picks which answer + // lands first. + async function revokeDuringRegionLoad(newestFirst) { + const h = makeHarness(); + const fetches = useRealApi(h); + const initDone = h.init(); + await flush(); + fetches[0].answer(BEFORE_REVOKE); + await initDone; + assert.ok(h.row('#shared'), 'precondition: the shared channel is listed'); + h.regionParam = 'SJC'; + h.regionChange(); + await flush(); + assert.strictEqual(fetches.length, 2, 'the region change fetches'); + h.revokedCallback('#shared'); + await flush(); + // Every request made after the revoke sees the channel gone. + const answers = fetches.slice(1).map((f, i) => () => f.answer(i === 0 ? BEFORE_REVOKE : AFTER_REVOKE)); + (newestFirst ? answers.reverse() : answers).forEach((answer) => answer()); + await flush(); + return { h, fetches }; + } + + for (const newestFirst of [true, false]) { + await test('#279: revoke while a /channels request is in flight drops the channel (' + (newestFirst ? 'older answer lands last' : 'older answer lands first') + ')', async () => { + const { h, fetches } = await revokeDuringRegionLoad(newestFirst); + assert.strictEqual(h.row('#shared'), undefined, + 'the revoked channel must leave the list (got ' + JSON.stringify(listedHashes(h)) + ')'); + assert.ok(!/data-hash="#shared"/.test(h.elements.chList.innerHTML), 'the revoked channel must not be rendered'); + assert.strictEqual(fetches.length, 3, 'one request for the region change, one for the revoke'); + assert.ok(/region=SJC/.test(fetches[1].url) && /region=SJC/.test(fetches[2].url), 'both requests are for the current region'); + // The answer the list now shows is also what the TTL cache holds, so + // the next refresh within the TTL keeps it gone without a request. + await h.w._channelsLoadChannelsForTest(true); + assert.strictEqual(fetches.length, 3, 'served from the cache'); + assert.strictEqual(h.row('#shared'), undefined, + 'the cached list still has the channel gone (got ' + JSON.stringify(listedHashes(h)) + ')'); + }); + } + + await test('#279: revoke with nothing in flight makes exactly one /channels request', async () => { + const h = makeHarness(); + const fetches = useRealApi(h); + const initDone = h.init(); + await flush(); + fetches[0].answer(BEFORE_REVOKE); + await initDone; + h.revokedCallback('#shared'); + await flush(); + assert.strictEqual(fetches.length, 2, 'exactly one request for the revoke (got ' + (fetches.length - 1) + ')'); + fetches[1].answer(AFTER_REVOKE); + await flush(); + assert.strictEqual(h.row('#shared'), undefined, 'the revoked channel is gone'); + }); + // ── #155: mobile channel rows show the unread badge ── console.log('\n=== #155 mobile rows show the unread badge ==='); diff --git a/test-nodes-advert-ws-bust-279.js b/test-nodes-advert-ws-bust-279.js new file mode 100644 index 000000000..1cd6fc95b --- /dev/null +++ b/test-nodes-advert-ws-bust-279.js @@ -0,0 +1,324 @@ +/** + * #279: the nodes advert WebSocket handler must not join an older in-flight + * /nodes request. + * + * public/nodes.js auto-refreshes the node list when ADVERT packets arrive + * (#131). Both of the handler's refresh paths did + * `invalidateApiCache('/nodes')` followed by a plain `loadNodes(true)`. + * `invalidateApiCache()` clears only the TTL cache, not api()'s `_inflight` + * map, so if a /nodes request was already in flight the refresh coalesced + * onto it. That request may have been answered by the server before the + * advert arrived, so the list rendered without the node that just + * advertised. This is the same shape as the #243 /channels bug, and the + * #243 fix lives at the call site (`api(..., { bust: true })`), not inside + * `invalidateApiCache()` — so it did not cover this path (N3 of the #263 + * round-2 review). + * + * The tests below load 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. + * Every /api/nodes fetch is parked on its own deferred, so each test decides + * when — and in which order — the server's answers land. + * + * Usage: node test-nodes-advert-ws-bust-279.js + */ +'use strict'; + +const vm = require('vm'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +const REPO = __dirname; + +function deferred() { + let resolve, reject; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + +// nodes.js's load path has several awaits per page (fetch, .json(), the +// pagination loop, getFleetSkew, MeshConfigReady), so drain generously. +function flush(n) { + let p = Promise.resolve(); + for (let i = 0; i < (n || 40); i++) p = p.then(() => new Promise((r) => setImmediate(r))); + return p; +} + +function advertFor(pubKey) { + return { + type: 'packet', + data: { + packet: { payload_type: 4, timestamp: new Date().toISOString() }, + decoded: { header: { payloadTypeName: 'ADVERT' }, payload: { pubKey } }, + }, + }; +} + +function node(pubKey, name) { + return { + public_key: pubKey, name, role: 'repeater', advert_count: 1, + last_seen: new Date().toISOString(), + }; +} + +const KEY_A = 'a'.repeat(64); +const KEY_B = 'b'.repeat(64); + +function el(id) { + return { + id, innerHTML: '', textContent: '', value: '', scrollTop: 0, + style: {}, dataset: {}, + classList: { add() {}, remove() {}, toggle() {}, contains() { return false; } }, + addEventListener() {}, removeEventListener() {}, + querySelectorAll() { return []; }, querySelector() { return null; }, + getAttribute() { return null; }, setAttribute() {}, appendChild() {}, + closest() { return null; }, contains() { return false; }, + }; +} + +// A nodes page wired to the real app.js api(). Only fetch is stubbed: +// /api/nodes?... requests are parked, everything else answers {} at once so +// app.js's own config fetches do not hang. +function makeHarness() { + const parked = []; + const els = {}; + const ctx = { + window: { addEventListener() {}, removeEventListener() {}, dispatchEvent() {} }, + console, Date, Infinity, Math, Array, Object, String, Number, JSON, RegExp, Error, TypeError, + parseInt, parseFloat, isNaN, isFinite, encodeURIComponent, decodeURIComponent, + Promise, Map, Set, URLSearchParams, + setTimeout: (fn) => setTimeout(fn, 0), clearTimeout: () => {}, + setInterval: () => 0, clearInterval: () => {}, + requestAnimationFrame: (fn) => { fn(); return 1; }, cancelAnimationFrame: () => {}, + performance: { now: () => Date.now() }, + location: { hash: '' }, + CustomEvent: class CustomEvent {}, + localStorage: (() => { + const store = {}; + return { + getItem: (k) => (k in store ? store[k] : null), + setItem: (k, v) => { store[k] = String(v); }, + removeItem: (k) => { delete store[k]; }, + }; + })(), + }; + ctx.document = { + readyState: 'complete', + head: { appendChild() {} }, + body: el('body'), + addEventListener() {}, removeEventListener() {}, + querySelectorAll: () => [], querySelector: () => null, + createElement: () => el('created'), + getElementById: (id) => { if (!els[id]) els[id] = el(id); return els[id]; }, + }; + ctx.addEventListener = () => {}; + ctx.dispatchEvent = () => {}; + ctx.getHashParams = () => new URLSearchParams((ctx.location.hash.split('?')[1] || '')); + ctx.fetch = function (url) { + if (url.indexOf('/api/nodes') !== 0) { + return Promise.resolve({ ok: true, status: 200, json: async () => ({}), headers: { get: () => null } }); + } + const d = deferred(); + parked.push({ + url, + answer: (body) => d.resolve({ ok: true, status: 200, json: async () => body, headers: { get: () => null } }), + }); + return d.promise; + }; + vm.createContext(ctx); + + function load(file) { + vm.runInContext(fs.readFileSync(path.join(REPO, file), 'utf8'), ctx, { filename: file }); + for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; + } + + load('public/roles.js'); + // The real api(): in-flight dedup, TTL cache, and the #243 `bust`. + load('public/app.js'); + + // Everything nodes.js reaches for that is not under test. + const h = { ctx, parked, els, regionParam: '', wsHandler: null, pageMod: null }; + ctx.RegionFilter = { init() {}, onChange() { return () => {}; }, offChange() {}, getRegionParam: () => h.regionParam }; + ctx.AreaFilter = { init() {}, onChange() { return () => {}; }, offChange() {}, getAreaParam: () => '' }; + ctx.onWS = () => {}; + ctx.offWS = () => {}; + // Capture the advert handler instead of debouncing it for 5s. + ctx.debouncedOnWS = (fn) => { h.wsHandler = fn; return fn; }; + ctx.getFleetSkew = () => Promise.resolve({}); + ctx.HopResolver = { init() {}, resolve: () => ({}), ready: () => false }; + ctx.connectWS = () => {}; + ctx.initTabBar = () => {}; + ctx.makeColumnsResizable = () => {}; + ctx.debounce = (fn) => fn; + ctx.getFavorites = () => []; + ctx.isFavorite = () => false; + ctx.favStar = () => ''; + ctx.bindFavStars = () => {}; + ctx.registerPage = (name, mod) => { if (name === 'nodes') h.pageMod = mod; }; + + load('public/nodes.js'); + + h.fetchesFor = (p) => parked.filter((f) => f.url.indexOf('/api' + p) === 0); + h.allNodes = () => ctx.window._nodesGetAllNodes(); + h.names = () => (h.allNodes() || []).map((n) => n.name).sort(); + h.setAllNodes = (n) => ctx.window._nodesSetAllNodes(n); + h.init = () => h.pageMod.init(ctx.document.getElementById('page')); + return h; +} + +let passed = 0, failed = 0; +async function test(name, fn) { + try { await fn(); passed++; console.log(' ✅ ' + name); } + catch (e) { failed++; console.log(' ❌ ' + name + ': ' + (e && e.message)); } +} + +(async () => { + console.log('\n=== #279 the nodes advert WS refresh does not join an in-flight /nodes request ==='); + + // Sanity: the harness really is driving the production advert handler. + await test('the page mounts an advert WS handler and the first load parks one /nodes request', async () => { + const h = makeHarness(); + h.init(); + await flush(); + assert.strictEqual(typeof h.wsHandler, 'function', 'init() must register a debouncedOnWS handler'); + assert.strictEqual(h.fetchesFor('/nodes?').length, 1, + 'the initial load makes exactly one request (got ' + h.fetchesFor('/nodes?').length + ')'); + assert.ok(h.ctx.window._nodesIsAdvertMessage(advertFor(KEY_B)), 'the fixture message is an ADVERT'); + }); + + // Path 1 — nodes.js:~684. An advert arrives while the very first load is + // still in flight, so `_allNodes` is still null. Its answer was produced + // before the advert, so the refresh must fetch again rather than join it. + // The older answer lands first, so only the bust's own request can decide + // what the list ends up holding. + await test('an advert during the first load refetches instead of joining it (_allNodes null path)', async () => { + const h = makeHarness(); + h.init(); + await flush(); + const before = h.fetchesFor('/nodes?'); + assert.strictEqual(before.length, 1, 'the initial load is in flight'); + + h.wsHandler([advertFor(KEY_B)]); + await flush(); + const after = h.fetchesFor('/nodes?'); + assert.strictEqual(after.length, 2, + 'the advert refresh must start its own /nodes request instead of joining the in-flight one (got ' + + after.length + ')'); + assert.strictEqual(after[0].url, after[1].url, 'both requests are for the same page of the same list'); + + // Older answer first (pre-advert), then the refresh's own answer. + after[0].answer({ nodes: [node(KEY_A, 'Alpha')], counts: {}, total: 1 }); + await flush(); + after[1].answer({ nodes: [node(KEY_A, 'Alpha'), node(KEY_B, 'Bravo')], counts: {}, total: 2 }); + await flush(); + + assert.deepStrictEqual(h.names(), ['Alpha', 'Bravo'], + 'the node that just advertised must be in the list (got ' + JSON.stringify(h.names()) + ')'); + }); + + // Path 2 — nodes.js:~711. `_allNodes` is populated and the advert names a + // node that is not in it, so the handler drops the cache and reloads. A + // /nodes request for the same page can still be in flight: two region + // switches in quick succession leave two loads running, and the one that + // answers first is the one that populates `_allNodes`. + await test('an advert for an unknown node refetches instead of joining an in-flight load (needReload path)', async () => { + const h = makeHarness(); + h.init(); + await flush(); + const inflight = h.fetchesFor('/nodes?'); + assert.strictEqual(inflight.length, 1, 'a load is in flight'); + // A load that already finished left this list behind. + h.setAllNodes([node(KEY_A, 'Alpha')]); + + h.wsHandler([advertFor(KEY_B)]); + await flush(); + const after = h.fetchesFor('/nodes?'); + assert.strictEqual(after.length, 2, + 'the reload must start its own /nodes request instead of joining the in-flight one (got ' + + after.length + ')'); + + after[0].answer({ nodes: [node(KEY_A, 'Alpha')], counts: {}, total: 1 }); + await flush(); + after[1].answer({ nodes: [node(KEY_A, 'Alpha'), node(KEY_B, 'Bravo')], counts: {}, total: 2 }); + await flush(); + + assert.deepStrictEqual(h.names(), ['Alpha', 'Bravo'], + 'the unknown node must have arrived with the reload (got ' + JSON.stringify(h.names()) + ')'); + }); + + // The bust takes the in-flight slot (#243), so a load started after it + // joins the newer request rather than fetching a third time. + await test('a load started after the advert refresh joins the refresh, not the superseded request', async () => { + const h = makeHarness(); + h.init(); + await flush(); + h.wsHandler([advertFor(KEY_B)]); + await flush(); + const after = h.fetchesFor('/nodes?'); + assert.strictEqual(after.length, 2, 'the refresh fetched (got ' + after.length + ')'); + // A region change with the same (empty) region param re-runs the load. + h.ctx.window._nodesSetAllNodes(null); + const joined = h.ctx.api(after[1].url.slice('/api'.length), { ttl: h.ctx.CLIENT_TTL.nodeList }); + await flush(); + assert.strictEqual(h.fetchesFor('/nodes?').length, 2, + 'a later plain load must join the refresh request (got ' + h.fetchesFor('/nodes?').length + ')'); + after[1].answer({ nodes: [node(KEY_B, 'Bravo')], counts: {}, total: 1 }); + const data = await joined; + assert.deepStrictEqual(data.nodes.map((n) => n.name), ['Bravo'], 'it got the refresh answer'); + }); + + // Guard: an advert that only updates a node already in the list is + // handled in place and must not make a request at all. + await test('an advert for a known node updates it in place and makes no request', async () => { + const h = makeHarness(); + h.init(); + await flush(); + h.fetchesFor('/nodes?')[0].answer({ nodes: [node(KEY_A, 'Alpha')], counts: {}, total: 1 }); + await flush(); + assert.deepStrictEqual(h.names(), ['Alpha'], 'the first load settled'); + const beforeCount = h.fetchesFor('/nodes?').length; + + const m = advertFor(KEY_A); + m.data.decoded.payload.name = 'Alpha renamed'; + h.wsHandler([m]); + await flush(); + assert.strictEqual(h.fetchesFor('/nodes?').length, beforeCount, + 'a known-node advert must not make a /nodes request (got ' + + (h.fetchesFor('/nodes?').length - beforeCount) + ' extra)'); + assert.deepStrictEqual(h.names(), ['Alpha renamed'], 'the advert was applied in place'); + }); + + // Guard: with nothing in flight, the refresh still makes exactly one + // request — `bust` must not turn one refresh into two. + await test('an advert with nothing in flight makes exactly one /nodes request', async () => { + const h = makeHarness(); + h.init(); + await flush(); + h.fetchesFor('/nodes?')[0].answer({ nodes: [node(KEY_A, 'Alpha')], counts: {}, total: 1 }); + await flush(); + const before = h.fetchesFor('/nodes?').length; + + h.wsHandler([advertFor(KEY_B)]); + await flush(); + const after = h.fetchesFor('/nodes?'); + assert.strictEqual(after.length - before, 1, + 'exactly one request for the refresh (got ' + (after.length - before) + ')'); + after[after.length - 1].answer({ nodes: [node(KEY_A, 'Alpha'), node(KEY_B, 'Bravo')], counts: {}, total: 2 }); + await flush(); + assert.deepStrictEqual(h.names(), ['Alpha', 'Bravo'], 'the refresh rendered'); + }); + + // Guard: a non-advert batch is ignored entirely. + await test('a batch with no advert makes no request', async () => { + const h = makeHarness(); + h.init(); + await flush(); + const before = h.fetchesFor('/nodes?').length; + h.wsHandler([{ type: 'packet', data: { packet: { payload_type: 1 }, decoded: {} } }]); + await flush(); + assert.strictEqual(h.fetchesFor('/nodes?').length, before, 'no request for a non-advert batch'); + }); + + console.log('\n' + passed + ' passed, ' + failed + ' failed'); + process.exit(failed ? 1 : 0); +})(); From 4dbd20a906cbf05f15f74090760c6377522cf7ff Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 08:55:30 +0200 Subject: [PATCH 2/3] fix(nodes): bust an in-flight /nodes request on an advert refresh (#279) 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 --- public/nodes.js | 17 +++++++++++++---- test-nodes-advert-ws-bust-279.js | 8 ++++++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/public/nodes.js b/public/nodes.js index 745abb854..654b80de8 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -705,9 +705,17 @@ const advertMsgs = msgs.filter(isAdvertMessage); if (!advertMsgs.length) return; + // #279: invalidateApiCache() clears only the TTL cache, so a plain + // loadNodes(true) could still join a /nodes request that was already + // in flight -- one the server may have answered before this advert + // arrived, leaving the node that just advertised out of the list. + // Same shape as the #243 /channels bug, and the same call-site fix: + // bust never joins an in-flight request and takes its slot, so a + // later plain load joins this newer one. A refresh that fetches + // nothing (the in-place branch below) is unaffected. if (!_allNodes) { invalidateApiCache('/nodes'); - loadNodes(true); + loadNodes(true, { bust: true }); return; } @@ -735,7 +743,7 @@ _fleetSkew = null; invalidateApiCache('/nodes'); } - loadNodes(true); + loadNodes(true, { bust: true }); // #279, as above }, 5000); } @@ -1433,7 +1441,8 @@ return ' (' + keys.length + ')'; } - async function loadNodes(refreshOnly) { + // opts.bust: fetch fresh pages even if the same request is in flight (#279). + async function loadNodes(refreshOnly, opts) { try { // Fetch all nodes via pagination loop — server clamps /api/nodes ?limit // to 500 (PR #1540 / v3.8.3 DoS guard), so a single fetch silently @@ -1458,7 +1467,7 @@ const nodesBody = document.getElementById('nodesBody'); while (offset < SAFETY_CAP) { baseParams.set('offset', String(offset)); - const data = await api('/nodes?' + baseParams, { ttl: CLIENT_TTL.nodeList }); + const data = await api('/nodes?' + baseParams, { ttl: CLIENT_TTL.nodeList, bust: !!(opts && opts.bust) }); if (!data || !Array.isArray(data.nodes)) break; accumulated.push.apply(accumulated, data.nodes); counts = data.counts || counts || {}; diff --git a/test-nodes-advert-ws-bust-279.js b/test-nodes-advert-ws-bust-279.js index 1cd6fc95b..68c73b92d 100644 --- a/test-nodes-advert-ws-bust-279.js +++ b/test-nodes-advert-ws-bust-279.js @@ -159,6 +159,11 @@ function makeHarness() { load('public/nodes.js'); h.fetchesFor = (p) => parked.filter((f) => f.url.indexOf('/api' + p) === 0); + // app.js's `api` / `CLIENT_TTL` are top-level lexical bindings of the + // sandbox script, not properties of its global object, so reach them the + // way nodes.js does -- from inside the context. + h.apiCall = (p) => vm.runInContext( + 'api(' + JSON.stringify(p) + ', { ttl: CLIENT_TTL.nodeList })', ctx); h.allNodes = () => ctx.window._nodesGetAllNodes(); h.names = () => (h.allNodes() || []).map((n) => n.name).sort(); h.setAllNodes = (n) => ctx.window._nodesSetAllNodes(n); @@ -257,8 +262,7 @@ async function test(name, fn) { const after = h.fetchesFor('/nodes?'); assert.strictEqual(after.length, 2, 'the refresh fetched (got ' + after.length + ')'); // A region change with the same (empty) region param re-runs the load. - h.ctx.window._nodesSetAllNodes(null); - const joined = h.ctx.api(after[1].url.slice('/api'.length), { ttl: h.ctx.CLIENT_TTL.nodeList }); + const joined = h.apiCall(after[1].url.slice('/api'.length)); await flush(); assert.strictEqual(h.fetchesFor('/nodes?').length, 2, 'a later plain load must join the refresh request (got ' + h.fetchesFor('/nodes?').length + ')'); From cec98a5ebcc0ec959a10f9a486f1876012b4a936 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 11:52:48 +0200 Subject: [PATCH 3/3] test(nodes): pin the advert bust per page and plain-load coalescing (#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. --- test-nodes-advert-ws-bust-279.js | 108 ++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 3 deletions(-) diff --git a/test-nodes-advert-ws-bust-279.js b/test-nodes-advert-ws-bust-279.js index 68c73b92d..07697d64e 100644 --- a/test-nodes-advert-ws-bust-279.js +++ b/test-nodes-advert-ws-bust-279.js @@ -64,6 +64,18 @@ function node(pubKey, name) { const KEY_A = 'a'.repeat(64); const KEY_B = 'b'.repeat(64); +// nodes.js paginates /api/nodes at 500 per page (the #1540 server clamp) and +// stops on the first short page, so a full page is what makes it ask for a +// second one. +const PAGE_SIZE = 500; +function fullPage() { + const out = []; + for (let i = 0; i < PAGE_SIZE; i++) { + out.push(node(('c' + i.toString(16)).padStart(64, '0'), 'Page1-' + i)); + } + return out; +} + function el(id) { return { id, innerHTML: '', textContent: '', value: '', scrollTop: 0, @@ -137,9 +149,20 @@ function makeHarness() { load('public/app.js'); // Everything nodes.js reaches for that is not under test. - const h = { ctx, parked, els, regionParam: '', wsHandler: null, pageMod: null }; - ctx.RegionFilter = { init() {}, onChange() { return () => {}; }, offChange() {}, getRegionParam: () => h.regionParam }; - ctx.AreaFilter = { init() {}, onChange() { return () => {}; }, offChange() {}, getAreaParam: () => '' }; + const h = { + ctx, parked, els, regionParam: '', + wsHandler: null, regionChange: null, areaChange: null, pageMod: null, + }; + // Capture the filter callbacks too: they are the ordinary, non-WS entry + // into loadNodes(), so they are how a test drives a plain load. + ctx.RegionFilter = { + init() {}, offChange() {}, getRegionParam: () => h.regionParam, + onChange(fn) { h.regionChange = fn; return () => {}; }, + }; + ctx.AreaFilter = { + init() {}, offChange() {}, getAreaParam: () => '', + onChange(fn) { h.areaChange = fn; return () => {}; }, + }; ctx.onWS = () => {}; ctx.offWS = () => {}; // Capture the advert handler instead of debouncing it for 5s. @@ -159,6 +182,9 @@ function makeHarness() { load('public/nodes.js'); h.fetchesFor = (p) => parked.filter((f) => f.url.indexOf('/api' + p) === 0); + // `offset` is the last query parameter nodes.js sets, so an exact tail + // match cannot be confused with `limit=500`. + h.fetchesAtOffset = (off) => parked.filter((f) => f.url.endsWith('offset=' + off)); // app.js's `api` / `CLIENT_TTL` are top-level lexical bindings of the // sandbox script, not properties of its global object, so reach them the // way nodes.js does -- from inside the context. @@ -271,6 +297,82 @@ async function test(name, fn) { assert.deepStrictEqual(data.nodes.map((n) => n.name), ['Bravo'], 'it got the refresh answer'); }); + // The bust must apply to *every* page, not just the first. With a >500-node + // deployment the refresh walks the pagination loop a second time, and a + // bust that only covered `offset=0` would let the refresh's page 2 join the + // superseded load's page 2 and inherit its pre-advert content. + await test('the bust applies to every page of a multi-page list, not just offset=0', async () => { + const h = makeHarness(); + h.init(); + await flush(); + assert.strictEqual(h.fetchesAtOffset(0).length, 1, 'the initial load asked for page 1'); + + h.wsHandler([advertFor(KEY_B)]); + await flush(); + const page1 = h.fetchesAtOffset(0); + assert.strictEqual(page1.length, 2, 'the refresh busted page 1 (got ' + page1.length + ')'); + + // The superseded load gets a full page first, so it walks on to page 2 + // and parks that request before the refresh reaches the same offset. + page1[0].answer({ nodes: fullPage(), counts: {}, total: 1000 }); + await flush(); + assert.strictEqual(h.fetchesAtOffset(PAGE_SIZE).length, 1, + 'the superseded load asked for page 2 (got ' + h.fetchesAtOffset(PAGE_SIZE).length + ')'); + + page1[1].answer({ nodes: fullPage(), counts: {}, total: 1000 }); + await flush(); + const page2 = h.fetchesAtOffset(PAGE_SIZE); + assert.strictEqual(page2.length, 2, + 'the refresh must bust page 2 as well instead of joining the superseded load\'s page 2 (got ' + + page2.length + ')'); + + // Pre-advert page 2 lands first; the refresh's own page 2 lands last and + // is the only one that carries the node that just advertised. + page2[0].answer({ nodes: [node(KEY_A, 'Alpha')], counts: {}, total: 1000 }); + await flush(); + page2[1].answer({ nodes: [node(KEY_A, 'Alpha'), node(KEY_B, 'Bravo')], counts: {}, total: 1000 }); + await flush(); + + const keys = (h.allNodes() || []).map((n) => n.public_key); + assert.ok(keys.indexOf(KEY_B) !== -1, + 'the advertising node must arrive on the refresh\'s own page 2 (list holds ' + keys.length + ' nodes)'); + assert.strictEqual(keys.length, PAGE_SIZE + 2, 'both pages were accumulated'); + }); + + // Guard: the bust is for the WS refresh only. Ordinary loads -- a region or + // area switch, a tab click, a search -- must still coalesce onto an + // in-flight /nodes request, or every one of them becomes its own fetch. + await test('plain loadNodes() calls outside the WS handler still coalesce', async () => { + const h = makeHarness(); + h.init(); + await flush(); + assert.strictEqual(typeof h.regionChange, 'function', 'init() must register a RegionFilter.onChange handler'); + assert.strictEqual(h.fetchesFor('/nodes?').length, 1, 'the initial load is in flight'); + + // Two region changes back onto the same (empty) region param: each one + // drops _allNodes and re-runs loadNodes() with no opts. + h.regionChange(); + await flush(); + h.regionChange(); + await flush(); + + assert.strictEqual(h.fetchesFor('/nodes?').length, 1, + 'plain loads must join the in-flight request, not fetch again (got ' + + h.fetchesFor('/nodes?').length + ')'); + + h.fetchesFor('/nodes?')[0].answer({ nodes: [node(KEY_A, 'Alpha')], counts: {}, total: 1 }); + await flush(); + assert.deepStrictEqual(h.names(), ['Alpha'], 'the shared answer rendered'); + + // And the WS refresh is still the exception: it busts even though the + // plain loads above did not. + const before = h.fetchesFor('/nodes?').length; + h.wsHandler([advertFor(KEY_B)]); + await flush(); + assert.strictEqual(h.fetchesFor('/nodes?').length - before, 1, + 'the advert refresh still fetches (got ' + (h.fetchesFor('/nodes?').length - before) + ')'); + }); + // Guard: an advert that only updates a node already in the list is // handled in place and must not make a request at all. await test('an advert for a known node updates it in place and makes no request', async () => {