fix(app): let an explicit refresh bypass an in-flight api() request (#243)

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;
 }
 

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.

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

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;
+})();

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',