From 899e92329896a99dc41fbbad20378649421111cd Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 14:26:00 +0000 Subject: [PATCH 1/2] test(app): reproduce an approval refresh joining an older in-flight api() request (#243) - test-app-api-bust-inflight-243.js: api(path, { bust: true }) during an in-flight request must fetch again, take the in-flight slot, and keep its newer data in the TTL cache when the older answer lands last. - test-channels-client-state-152.js: the real channels.js with the real app.js api(): an approval while a region load is in flight must list the approved channel, in both answer orders; nothing in flight makes exactly one request. - test-channel-proposals.js: ChannelProposals.unmount() cancels the suggest poller (no scheduled poll, no status request afterwards). Red on master: 4 api() tests and the 2 in-flight channel tests. The unmount test is green on master and red without suggestPoller.cancel(). Co-Authored-By: Claude Opus 5.5 --- test-all.sh | 1 + test-app-api-bust-inflight-243.js | 194 ++++++++++++++++++++++++++++++ test-channel-proposals.js | 22 +++- test-channels-client-state-152.js | 88 ++++++++++++++ 4 files changed, 302 insertions(+), 3 deletions(-) create mode 100644 test-app-api-bust-inflight-243.js diff --git a/test-all.sh b/test-all.sh index 9ce3bdbcc..237b2f886 100755 --- a/test-all.sh +++ b/test-all.sh @@ -39,6 +39,7 @@ run test-aging.js 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-issue-120-distance-building.js run test-privacy-page.js run test-nav-dynamic-link-lifecycle.js diff --git a/test-app-api-bust-inflight-243.js b/test-app-api-bust-inflight-243.js new file mode 100644 index 000000000..222753ff8 --- /dev/null +++ b/test-app-api-bust-inflight-243.js @@ -0,0 +1,194 @@ +/** + * #243: an explicit refresh, api(path, { bust: true }), must not join an + * older in-flight request for the same path. + * + * api() deduplicates in-flight requests per path. `bust` used to skip only + * the TTL cache, so a refresh asked for while a request was in flight got + * that older request's answer, which the server may have produced before + * the change the caller wants to see (a channel approved a moment ago). + * + * Loads the real public/app.js in a vm sandbox; only fetch is stubbed, and + * every fetch is parked on its own deferred so the test decides the order + * in which responses land. + * + * Usage: node test-app-api-bust-inflight-243.js + */ +'use strict'; + +const vm = require('vm'); +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +const APP_JS_PATH = path.join(__dirname, 'public', 'app.js'); + +function deferred() { + let resolve, reject; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + +function flush(n) { + let p = Promise.resolve(); + for (let i = 0; i < (n || 10); i++) p = p.then(() => new Promise((r) => setImmediate(r))); + return p; +} + +function makeSandbox() { + const parked = []; + const ctx = { + window: { addEventListener: () => {}, dispatchEvent: () => {} }, + document: { + readyState: 'complete', + getElementById: () => null, + addEventListener: () => {}, + querySelectorAll: () => [], + querySelector: () => null, + createElement: () => ({ style: {} }), + head: { appendChild: () => {} }, + }, + console, Date, Promise, Map, Set, JSON, Math, Error, TypeError, Array, Object, String, Number, RegExp, + parseInt, parseFloat, isNaN, isFinite, encodeURIComponent, decodeURIComponent, + setTimeout: (fn) => setTimeout(fn, 0), clearTimeout: () => {}, + setInterval: () => 1, clearInterval: () => {}, + performance: { now: () => Date.now() }, + location: { hash: '' }, + addEventListener: () => {}, + dispatchEvent: () => {}, + }; + ctx.fetch = function (url) { + // app.js's own top-level config fetch is answered at once. + if (url.indexOf('/api/config/') === 0) return Promise.resolve({ ok: true, status: 200, json: async () => ({}), headers: { get: () => null } }); + const d = deferred(); + parked.push({ url, d }); + return d.promise; + }; + vm.createContext(ctx); + vm.runInContext(fs.readFileSync(APP_JS_PATH, 'utf8'), ctx, { filename: APP_JS_PATH }); + for (const k of Object.keys(ctx.window)) ctx[k] = ctx.window[k]; + return { + api: ctx.api, + invalidateApiCache: ctx.invalidateApiCache, + parked, + fetchesFor: (p) => parked.filter((f) => f.url === '/api' + p), + answer: (entry, body) => entry.d.resolve({ ok: true, status: 200, json: async () => body, headers: { get: () => null } }), + }; +} + +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=== #243 api(path, { bust: true }) does not join an older in-flight request ==='); + + await test('a bust during an in-flight request fetches again and gets the newer data', async () => { + const h = makeSandbox(); + const older = h.api('/x', { ttl: 15000 }); + await flush(); + const newer = h.api('/x', { ttl: 15000, bust: true }); + await flush(); + const fetches = h.fetchesFor('/x'); + assert.strictEqual(fetches.length, 2, 'the bust must start its own request (got ' + fetches.length + ')'); + h.answer(fetches[1], { v: 'new' }); + h.answer(fetches[0], { v: 'old' }); + assert.deepStrictEqual(await newer, { v: 'new' }); + assert.deepStrictEqual(await older, { v: 'old' }, 'the older caller still gets its own answer'); + }); + + await test('a later call without bust joins the newer request, not the older one', async () => { + const h = makeSandbox(); + h.api('/x'); + await flush(); + const newer = h.api('/x', { bust: true }); + const joined = h.api('/x'); + await flush(); + const fetches = h.fetchesFor('/x'); + assert.strictEqual(fetches.length, 2, 'the plain call must join an in-flight request, not fetch (got ' + fetches.length + ')'); + h.answer(fetches[1], { v: 'new' }); + h.answer(fetches[0], { v: 'old' }); + assert.deepStrictEqual(await joined, { v: 'new' }, 'it joins the bust request'); + assert.deepStrictEqual(await newer, { v: 'new' }); + }); + + await test('the older request settling first does not remove the newer in-flight entry', async () => { + const h = makeSandbox(); + const older = h.api('/x'); + await flush(); + h.api('/x', { bust: true }); + await flush(); + const fetches = h.fetchesFor('/x'); + assert.strictEqual(fetches.length, 2, 'the bust must start its own request (got ' + fetches.length + ')'); + h.answer(fetches[0], { v: 'old' }); + await older; + await flush(); + const joined = h.api('/x'); + await flush(); + assert.strictEqual(h.fetchesFor('/x').length, 2, 'a plain call while the bust is in flight must join it (got ' + h.fetchesFor('/x').length + ' fetches)'); + h.answer(fetches[1], { v: 'new' }); + assert.deepStrictEqual(await joined, { v: 'new' }); + }); + + await test('an older response landing last does not overwrite the newer data in the TTL cache', async () => { + const h = makeSandbox(); + const older = h.api('/x', { ttl: 15000 }); + await flush(); + const newer = h.api('/x', { ttl: 15000, bust: true }); + await flush(); + const fetches = h.fetchesFor('/x'); + assert.strictEqual(fetches.length, 2, 'the bust must start its own request (got ' + fetches.length + ')'); + h.answer(fetches[1], { v: 'new' }); + await newer; + h.answer(fetches[0], { v: 'old' }); + await older; + await flush(); + const cached = await h.api('/x', { ttl: 15000 }); + assert.strictEqual(h.fetchesFor('/x').length, 2, 'served from the cache'); + assert.deepStrictEqual(cached, { v: 'new' }, 'the cache must keep the newer data (got ' + JSON.stringify(cached) + ')'); + }); + + await test('a bust with nothing in flight makes exactly one request and fills the cache', async () => { + const h = makeSandbox(); + const p = h.api('/x', { ttl: 15000, bust: true }); + await flush(); + assert.strictEqual(h.fetchesFor('/x').length, 1); + h.answer(h.fetchesFor('/x')[0], { v: 1 }); + assert.deepStrictEqual(await p, { v: 1 }); + assert.deepStrictEqual(await h.api('/x', { ttl: 15000 }), { v: 1 }); + assert.strictEqual(h.fetchesFor('/x').length, 1, 'the next plain call is a cache hit'); + }); + + await test('guard: calls without bust still share one in-flight request and the TTL cache', async () => { + const h = makeSandbox(); + const a = h.api('/x', { ttl: 15000 }); + const b = h.api('/x', { ttl: 15000 }); + await flush(); + assert.strictEqual(h.fetchesFor('/x').length, 1, 'concurrent plain calls are deduplicated'); + h.answer(h.fetchesFor('/x')[0], { v: 1 }); + assert.deepStrictEqual(await a, { v: 1 }); + assert.deepStrictEqual(await b, { v: 1 }); + await h.api('/x', { ttl: 15000 }); + assert.strictEqual(h.fetchesFor('/x').length, 1, 'TTL hit'); + h.invalidateApiCache('/x'); + h.api('/x', { ttl: 15000 }); + await flush(); + assert.strictEqual(h.fetchesFor('/x').length, 2, 'refetch after invalidation'); + }); + + await test('guard: a failed bust leaves no in-flight entry behind', async () => { + const h = makeSandbox(); + const p = h.api('/x', { bust: true }); + await flush(); + h.fetchesFor('/x')[0].d.resolve({ ok: false, status: 500, json: async () => ({}), headers: { get: () => null } }); + await assert.rejects(p, /API 500/); + await flush(); + h.api('/x'); + await flush(); + assert.strictEqual(h.fetchesFor('/x').length, 2, 'the next call fetches again'); + }); + + console.log(`\ntest-app-api-bust-inflight-243.js: ${passed} passed, ${failed} failed`); + process.exitCode = failed ? 1 : 0; +})(); diff --git a/test-channel-proposals.js b/test-channel-proposals.js index bc3e8a5aa..a979606a5 100644 --- a/test-channel-proposals.js +++ b/test-channel-proposals.js @@ -603,8 +603,8 @@ function fireDocKeydown(env, evt) { const fastTimers = { setTimeout: (fn) => setImmediate(fn), clearTimeout: (id) => clearImmediate(id) }; // Mount with a suggest section and an onApproved spy; statusFor(requestId, -// pollIndex) answers each status request. -async function mountSuggest(statusFor, extraRoute) { +// pollIndex) answers each status request. timers defaults to fastTimers. +async function mountSuggest(statusFor, extraRoute, timers) { const polls = {}; const approved = []; const env = loadWithDom((url, o) => { @@ -622,7 +622,7 @@ async function mountSuggest(statusFor, extraRoute) { } if (extraRoute) { const r = extraRoute(url, method); if (r) return r; } return { status: 404, body: { error: 'unexpected ' + method + ' ' + url } }; - }, fastTimers); + }, timers || fastTimers); const section = env.document.createElement('section'); section.setAttribute('hidden', ''); env.document.body.appendChild(section); @@ -710,6 +710,22 @@ test('a proposal approved again after a revoke refreshes again (#232)', async () assert.deepStrictEqual(env.approved, ['#Again', '#Again']); }); +// #243: leaving the Channels page must stop the suggest poller, or it keeps +// polling a queued suggestion's status after the page is gone. +test('unmount() cancels the suggest poller: no poll stays scheduled, no status request afterwards (#243)', async () => { + const timers = fakeTimers(); + const env = await mountSuggest(() => ({ status: 'queued' }), null, timers); + await env.suggest('StillQueued'); + assert.strictEqual(timers.size(), 1, 'a queued suggestion schedules a status poll'); + const polls = () => env.fetchCalls.filter((c) => /\/requests\//.test(c.url)).length; + const before = polls(); + env.CP.unmount(); + assert.strictEqual(timers.size(), 0, 'unmount() must clear the scheduled poll'); + while (await timers.fireNext()) { /* run whatever is still scheduled */ } + await flush(); + assert.strictEqual(polls(), before, 'no status request after unmount()'); +}); + // ── Invisible formatting characters (PR #99 review, finding 3) ─────────── test('normalizeName rejects invisible format characters (Cf) and line separators', () => { const chars = ['\u00AD', '\u0600', '\u180E', '\u200B', '\u200C', '\u2060', '\u2062', '\uFEFF', '\uFFF9', diff --git a/test-channels-client-state-152.js b/test-channels-client-state-152.js index e9ca3ba04..a5770b394 100644 --- a/test-channels-client-state-152.js +++ b/test-channels-client-state-152.js @@ -1719,6 +1719,94 @@ async function test(name, fn) { assert.strictEqual(row.unread, 2, 'unread carried'); }); + // ── #243: an approval refresh must not join an older in-flight request ── + console.log('\n=== #243 an approval refresh does not join an in-flight /channels request ==='); + + // Puts the real app.js api() (in-flight dedup, TTL cache, bust) in front + // of /channels. app.js runs in its own small sandbox with a stubbed fetch; + // every /channels fetch is parked on its own deferred, so a test decides + // the order in which the server's answers land. + function useRealApi(h) { + const fetches = []; + const app = { + window: { addEventListener() {}, dispatchEvent() {} }, + document: { readyState: 'complete', getElementById: () => null, addEventListener() {}, querySelectorAll: () => [], querySelector: () => null, createElement: () => ({ style: {} }), head: { appendChild() {} } }, + console, Date: RealDate, Promise, Map, Set, JSON, Math, Error, TypeError, Array, Object, String, Number, RegExp, + parseInt, parseFloat, isNaN, isFinite, encodeURIComponent, decodeURIComponent, + setTimeout: (fn) => setTimeout(fn, 0), clearTimeout() {}, setInterval: () => 1, clearInterval() {}, + performance: { now: () => RealDate.now() }, + location: { hash: '' }, addEventListener() {}, dispatchEvent() {}, + fetch(url) { + if (url.indexOf('/api/channels') !== 0) return Promise.resolve({ ok: true, status: 200, json: async () => ({}), headers: { get: () => null } }); + const d = deferred(); + fetches.push({ url, answer: (body) => d.resolve({ ok: true, status: 200, json: async () => body, headers: { get: () => null } }) }); + return d.promise; + }, + }; + vm.createContext(app); + vm.runInContext(fs.readFileSync(path.join(__dirname, 'public', 'app.js'), 'utf8'), app, { filename: 'public/app.js' }); + const stubApi = h.ctx.api; + h.ctx.api = (p, o) => (p.indexOf('/channels') === 0 && p.indexOf('/messages') === -1 ? app.api(p, o) : stubApi(p, o)); + h.ctx.invalidateApiCache = app.invalidateApiCache; + return fetches; + } + const BEFORE_APPROVAL = { channels: [serverChannel('public')] }; + const AFTER_APPROVAL = { channels: [serverChannel('public')], approvedChannels: [{ hash: '#shared', name: '#shared' }] }; + + // A region change is in flight (its answer was made before the approval) + // when the approval asks for a refresh. newestFirst picks which answer + // lands first. + async function approvalDuringRegionLoad(newestFirst) { + const h = makeHarness(); + const fetches = useRealApi(h); + const initDone = h.init(); + await flush(); + fetches[0].answer(BEFORE_APPROVAL); + await initDone; + h.regionParam = 'SJC'; + h.regionChange(); + await flush(); + assert.strictEqual(fetches.length, 2, 'the region change fetches'); + h.approvedCallback(); + await flush(); + // Every request made after the approval sees the approved channel. + const answers = fetches.slice(1).map((f, i) => () => f.answer(i === 0 ? BEFORE_APPROVAL : AFTER_APPROVAL)); + (newestFirst ? answers.reverse() : answers).forEach((answer) => answer()); + await flush(); + return { h, fetches }; + } + + for (const newestFirst of [true, false]) { + await test('#243: approval while a /channels request is in flight lists the approved channel (' + (newestFirst ? 'older answer lands last' : 'older answer lands first') + ')', async () => { + const { h, fetches } = await approvalDuringRegionLoad(newestFirst); + assert.ok(h.row('#shared') && h.row('#shared').shared === true, + 'the approved channel must be listed (got ' + JSON.stringify(listedHashes(h)) + ')'); + assert.ok(/data-hash="#shared"/.test(h.elements.chList.innerHTML), 'the approved channel must be rendered'); + assert.strictEqual(fetches.length, 3, 'one request for the region change, one for the approval'); + 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 the channel without a request. + await h.w._channelsLoadChannelsForTest(true); + assert.strictEqual(fetches.length, 3, 'served from the cache'); + assert.ok(h.row('#shared'), 'the cached list still has the approved channel (got ' + JSON.stringify(listedHashes(h)) + ')'); + }); + } + + await test('#243: approval 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_APPROVAL); + await initDone; + h.approvedCallback(); + await flush(); + assert.strictEqual(fetches.length, 2, 'exactly one request for the approval (got ' + (fetches.length - 1) + ')'); + fetches[1].answer(AFTER_APPROVAL); + await flush(); + assert.ok(h.row('#shared') && h.row('#shared').shared === true, 'the approved channel is listed'); + }); + // ── #155: mobile channel rows show the unread badge ── console.log('\n=== #155 mobile rows show the unread badge ==='); From 601bd000a5aa382abf48489f72a8705f271bf86f Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 14:32:09 +0000 Subject: [PATCH 2/2] fix(app): let an explicit refresh bypass an in-flight api() request (#243) api(path, { bust: true }) skipped only the TTL cache. A refresh asked for while the same request was in flight joined that older request, whose answer may predate the change (an approval), so the approved channel was missing until the next refresh. - api(): a bust never joins _inflight; it fetches and takes the in-flight slot, so later plain callers join the newer request. A superseded request neither writes the TTL cache nor removes the newer slot entry. Calls without bust are unchanged. - channels.js: onApproved refreshes with loadChannels(true, { bust: true }). The #225 request token still drops the older response. The only other bust caller, the observers manual refresh, now gets fresh data instead of joining an in-flight auto-refresh, as its #1563 request guard already expects. Co-Authored-By: Claude Opus 5.5 --- public/app.js | 15 ++++++++++++--- public/channels.js | 14 ++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/public/app.js b/public/app.js index 425e82e35..cd4c1896e 100644 --- a/public/app.js +++ b/public/app.js @@ -154,7 +154,10 @@ async function api(path, { ttl = 0, bust = false, retry503 = true } = {}) { // that retries 503s itself never waits on the retry loop below, and a // default caller never gets a 503 that loop would have ridden out. const inflightKey = retry503 ? path : path + '\n#no-retry503'; - if (_inflight.has(inflightKey)) return _inflight.get(inflightKey); + // #243: an explicit refresh (bust) never joins an in-flight request, which + // may have been answered before the change the caller wants to see. It + // takes the in-flight slot instead, so later callers join the newer one. + if (!bust && _inflight.has(inflightKey)) return _inflight.get(inflightKey); const promise = (async () => { // Issue #1659: 503 with Retry-After indicates server-side warm-up // (analytics recomputer first-pass, index build, etc.). Retry with @@ -217,7 +220,9 @@ async function api(path, { ttl = 0, bust = false, retry503 = true } = {}) { if (data && typeof data === 'object' && isFinite(ra) && ra > 0) { Object.defineProperty(data, 'retryAfterSeconds', { value: ra, enumerable: false }); } - } else if (ttl > 0) { + } else if (ttl > 0 && _inflight.get(inflightKey) === promise) { + // #243: a request a bust has superseded keeps its late answer out + // of the cache, where it would replace the newer data. _apiCache.set(path, { data, expires: Date.now() + ttl }); } return data; @@ -235,7 +240,11 @@ async function api(path, { ttl = 0, bust = false, retry503 = true } = {}) { // duplicated as a second, unobserved rejection on this derived one. // The `.catch()` here only silences that duplicate -- it does not // touch `promise` itself or its resolution to callers. - promise.finally(() => _inflight.delete(inflightKey)).catch(() => {}); + // #243: remove the entry only while it is still this request's; a bust + // may have taken the slot over. + promise.finally(() => { + if (_inflight.get(inflightKey) === promise) _inflight.delete(inflightKey); + }).catch(() => {}); return promise; } diff --git a/public/channels.js b/public/channels.js index 25639903b..9f870b05a 100644 --- a/public/channels.js +++ b/public/channels.js @@ -1219,8 +1219,9 @@ onApproved: function () { invalidateApiCache('/channels'); // loadChannels() merges the user's PSK rows itself (#152), before - // it reconciles the selection. - loadChannels(true); + // it reconciles the selection. bust: a /channels request already + // in flight may predate the approval (#243). + loadChannels(true, { bust: true }); } }); } @@ -2008,12 +2009,13 @@ let channelsRequestId = 0; let latestChannelsLoad = null; - function loadChannels(silent) { - latestChannelsLoad = loadChannelsFor(++channelsRequestId, silent); + // opts.bust: fetch fresh data even if the same request is in flight. + function loadChannels(silent, opts) { + latestChannelsLoad = loadChannelsFor(++channelsRequestId, silent, !!(opts && opts.bust)); return latestChannelsLoad; } - async function loadChannelsFor(requestId, silent) { + async function loadChannelsFor(requestId, silent, bust) { // #152: WS activity stamped after this point is newer than the snapshot. const seqAtRequestStart = wsActivitySeq; try { @@ -2023,7 +2025,7 @@ if (rp) params.push('region=' + encodeURIComponent(rp)); if (showEnc) params.push('includeEncrypted=true'); const qs = params.length ? '?' + params.join('&') : ''; - const data = await api('/channels' + qs, { ttl: CLIENT_TTL.channels }); + const data = await api('/channels' + qs, { ttl: CLIENT_TTL.channels, bust: bust }); // #154: a newer request owns the list. Resolve when it is done, so a // caller's follow-up (init()'s deep link, the region handler) sees the // list that actually renders.