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 95f5ffa47..086d10975 100644 --- a/public/channels.js +++ b/public/channels.js @@ -2010,20 +2010,22 @@ 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; } // Reload the list after a shared channel was approved (#232) or revoked // (#251). loadChannels() merges the user's PSK rows itself (#152), before - // it reconciles the selection. + // it reconciles the selection. bust: a /channels request already in + // flight may predate the approval or revocation (#243). function refreshChannelList() { invalidateApiCache('/channels'); - loadChannels(true); + loadChannels(true, { bust: true }); } - 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 { @@ -2033,7 +2035,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. 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 cf6e55649..5389f080e 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 c4717444f..328b2ca3b 100644 --- a/test-channels-client-state-152.js +++ b/test-channels-client-state-152.js @@ -1795,6 +1795,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 ===');