fix(historical-author-cache): address Qodo review on #7769

Four issues raised on the first push:

1. **Cache recompute race.** If compute() ran past ttlMs, a second
   get() would start a duplicate compute and the older resolution
   could clobber the newer cached value. Fixed by piggybacking on
   the in-flight promise regardless of TTL — never start a second
   compute while one is running.

2. **Unexplained conditional type.** `AuthorRecord extends never ?
   never : ...` was a leftover that confused future readers
   without earning anything. Replaced with a plain `CacheState |
   null` typed field with documented members.

3. **Shared cache object.** Cache hits returned the same object
   reference; handleClientReady embeds it in clientVars exposed to
   the clientVars hook, where a mutation by one join could bleed
   into the next. Now defensively shallow-clones on every get().

4. **Missing author logs.** The previous inline Promise.all loop
   in handleClientReady emitted `messageLogger.error('There is no
   author for authorId...')` per missing id; the refactor dropped
   that. Cache now takes an optional onMissingAuthor callback;
   Pad.ts wires it to the same log4js logger so the error still
   surfaces.

Plus: commit-state-promise-identity guard in refresh() — only
commit the resolved data if the in-flight promise is still the
one the state references (covers invalidate() racing a compute).

8/8 tests green: original 6 + new fresh-object guarantee + slow-
compute-past-TTL guarantee.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

src/node/db/HistoricalAuthorDataCache.ts

@@ -1,6 +1,6 @@
 // Per-pad cache for the `{authorId -> {name, colorId}}` map used by
 // PadMessageHandler.handleClientReady to populate clientVars
-// (#7756 connect-handshake cliff investigation).
+// (#7756 connect-handshake investigation).
 //
 // At 200+ authors a burst of 50 simultaneous CLIENT_READY handshakes
 // would otherwise each do Promise.all(authors.map(getAuthor)) =
@@ -13,42 +13,63 @@
 
 export type AuthorRecord = {name: string; colorId: string};
 export type GetAuthorFn = (id: string) => Promise<AuthorRecord | null | undefined>;
+export type OnMissingAuthorFn = (id: string) => void;
+
+type CacheState = {
+  /** Resolved data. Empty `{}` until the first compute() resolves. */
+  data: {[id: string]: AuthorRecord};
+  /** Set iff a compute() is currently in flight. New callers await this same
+   *  promise rather than starting a duplicate compute. Cleared on resolve. */
+  promise?: Promise<{[id: string]: AuthorRecord}>;
+  /** Wall-clock time the current data was committed. Used for TTL only. */
+  builtAt: number;
+};
 
 export class HistoricalAuthorDataCache {
-  private cached: AuthorRecord extends never ? never : {
-    data: {[id: string]: AuthorRecord};
-    promise?: Promise<{[id: string]: AuthorRecord}>;
-    builtAt: number;
-  } | null = null;
+  private state: CacheState | null = null;
 
   constructor(
     private readonly listAuthorIds: () => string[],
     private readonly getAuthor: GetAuthorFn,
     private readonly ttlMs: number = 5_000,
     private readonly now: () => number = Date.now,
+    /** Called once per author id that the fetcher returns falsy for.
+     *  Lets the consumer preserve the error log that lived in the
+     *  previous inline Promise.all loop. Optional. */
+    private readonly onMissingAuthor: OnMissingAuthorFn = () => {},
   ) {}
 
   async get(): Promise<{[id: string]: AuthorRecord}> {
     const now = this.now();
-    const cached = this.cached;
-    if (cached && now - cached.builtAt < this.ttlMs) {
-      return cached.promise ?? cached.data;
-    }
-    const promise = this.compute();
-    this.cached = {data: {}, promise, builtAt: now};
-    try {
-      const data = await promise;
-      this.cached = {data, builtAt: now};
-      return data;
-    } catch (err) {
-      this.cached = null;
-      throw err;
-    }
+    const s = this.state;
+    // In-flight compute: piggyback on it regardless of TTL — never start a
+    // second compute on top of a running one. The previous version could
+    // race two computes if the first ran past ttlMs, and the older
+    // resolution would clobber the newer cached value.
+    if (s?.promise) return cloneData(await s.promise);
+    if (s && now - s.builtAt < this.ttlMs) return cloneData(s.data);
+    return cloneData(await this.refresh(now));
   }
 
   /** Force the next get() to refetch. PadMessageHandler can call this when
    *  a new author commits, if we add hookable author-add events later. */
-  invalidate(): void { this.cached = null; }
+  invalidate(): void { this.state = null; }
+
+  private refresh(now: number): Promise<{[id: string]: AuthorRecord}> {
+    const promise = this.compute();
+    this.state = {data: {}, promise, builtAt: now};
+    promise.then(
+      (data) => {
+        // Only commit if our promise is still the one the state references —
+        // covers the (unlikely) case where invalidate() ran during compute.
+        if (this.state?.promise === promise) {
+          this.state = {data, builtAt: this.now()};
+        }
+      },
+      () => { if (this.state?.promise === promise) this.state = null; },
+    );
+    return promise;
+  }
 
   private async compute(): Promise<{[id: string]: AuthorRecord}> {
     const ids = this.listAuthorIds();
@@ -56,7 +77,20 @@ export class HistoricalAuthorDataCache {
     await Promise.all(ids.map(async (id) => {
       const a = await this.getAuthor(id);
       if (a) out[id] = {name: a.name, colorId: a.colorId};
+      else this.onMissingAuthor(id);
     }));
     return out;
   }
 }
+
+// Defensive shallow copy on every get(). Callers (notably handleClientReady,
+// which embeds the result in clientVars and exposes it to the clientVars
+// hook) historically received a fresh object per call; preserving that
+// here so a mutation by one join can't bleed into the next.
+const cloneData = (
+  src: {[id: string]: AuthorRecord},
+): {[id: string]: AuthorRecord} => {
+  const out: {[id: string]: AuthorRecord} = {};
+  for (const k in src) out[k] = {name: src[k]!.name, colorId: src[k]!.colorId};
+  return out;
+};

src/node/db/Pad.ts

@@ -23,6 +23,8 @@ const groupManager = require('./GroupManager');
 const CustomError = require('../utils/customError');
 import readOnlyManager from './ReadOnlyManager';
 import {HistoricalAuthorDataCache} from './HistoricalAuthorDataCache';
+import log4js from 'log4js';
+const padMessageLogger = log4js.getLogger('message');
 import randomString from '../utils/randomstring';
 const hooks = require('../../static/js/pluginfw/hooks');
 import pad_utils from "../../static/js/pad_utils";
@@ -343,6 +345,17 @@ class Pad {
       this.historicalAuthorDataCache = new HistoricalAuthorDataCache(
         () => this.getAllAuthors(),
         (id: string) => authorManager.getAuthor(id),
+        5_000,
+        Date.now,
+        (id: string) => {
+          // Preserves the explicit error log emitted by the previous inline
+          // Promise.all loop in handleClientReady before this cache landed.
+          // Don't drop missing-author logs silently — they point at
+          // https://github.com/ether/etherpad-lite/issues/2802.
+          padMessageLogger.error(
+            `There is no author for authorId: ${id}. ` +
+            'This is possibly related to https://github.com/ether/etherpad-lite/issues/2802');
+        },
       );
     }
     return this.historicalAuthorDataCache.get();

src/tests/backend-new/specs/pad-historical-author-data.test.ts

@@ -10,11 +10,19 @@
 import {describe, it, expect, vi, beforeEach} from 'vitest';
 import {HistoricalAuthorDataCache, type AuthorRecord} from '../../../node/db/HistoricalAuthorDataCache';
 
-const makeCache = (ids: string[], fetcher: (id: string) => Promise<AuthorRecord | null | undefined>, ttlMs = 5_000, now = () => Date.now()) =>
-  new HistoricalAuthorDataCache(() => ids, fetcher, ttlMs, now);
+type Fetcher = (id: string) => Promise<AuthorRecord | null | undefined>;
+type OnMissing = (id: string) => void;
+
+const makeCache = (
+  ids: string[],
+  fetcher: Fetcher,
+  ttlMs = 5_000,
+  now = () => Date.now(),
+  onMissing: OnMissing = () => {},
+) => new HistoricalAuthorDataCache(() => ids, fetcher, ttlMs, now, onMissing);
 
 describe('HistoricalAuthorDataCache', () => {
-  let getAuthorMock: ReturnType<typeof vi.fn<(id: string) => Promise<AuthorRecord | null>>>;
+  let getAuthorMock: ReturnType<typeof vi.fn<Fetcher>>;
 
   beforeEach(() => {
     getAuthorMock = vi.fn(async (id: string) => ({name: `n-${id}`, colorId: `c-${id}`}));
@@ -53,12 +61,53 @@ describe('HistoricalAuthorDataCache', () => {
     expect(getAuthorMock).toHaveBeenCalledTimes(2);
   });
 
-  it('omits authors the fetcher returns falsy for', async () => {
+  it('returns a fresh object on every get() — callers may safely mutate without bleeding into other joiners', async () => {
+    const cache = makeCache(['a.1'], getAuthorMock);
+    const first = await cache.get();
+    const second = await cache.get();
+    // Two distinct top-level objects and per-author records.
+    expect(first).not.toBe(second);
+    expect(first['a.1']).not.toBe(second['a.1']);
+    expect(first).toEqual(second);
+    // Mutating the returned object must not affect the next caller.
+    first['a.1']!.name = 'mutated';
+    const third = await cache.get();
+    expect(third).toEqual({'a.1': {name: 'n-a.1', colorId: 'c-a.1'}});
+  });
+
+  it('a slow compute that runs past TTL still resolves callers; no duplicate fetch starts in flight', async () => {
+    // Compute that hangs on a gate; TTL is 10ms. Without the in-flight
+    // guard, the second get() after 10ms would start a duplicate compute,
+    // and the older resolution could clobber the newer cached value.
+    let release: () => void;
+    const gate = new Promise<void>((r) => { release = r; });
+    let calls = 0;
+    let clock = 0;
+    const fetcher = vi.fn(async (id: string) => {
+      calls++;
+      await gate;
+      return {name: `n-${id}`, colorId: `c-${id}`};
+    });
+    const cache = makeCache(['a.1'], fetcher, 10, () => clock);
+    const first = cache.get();
+    clock = 50; // well past ttlMs
+    const second = cache.get();
+    expect(calls).toBe(1);
+    release!();
+    const [a, b] = await Promise.all([first, second]);
+    expect(a).toEqual(b);
+    expect(calls).toBe(1);
+  });
+
+  it('calls onMissingAuthor exactly once per id the fetcher returns falsy for', async () => {
     const fetcher = vi.fn(async (id: string) =>
       id === 'a.gone' ? null : {name: `n-${id}`, colorId: 'c'});
-    const cache = makeCache(['a.1', 'a.gone', 'a.2'], fetcher);
+    const onMissing = vi.fn();
+    const cache = makeCache(['a.1', 'a.gone', 'a.2'], fetcher, 5_000, Date.now, onMissing);
     const data = await cache.get();
     expect(Object.keys(data).sort()).toEqual(['a.1', 'a.2']);
+    expect(onMissing).toHaveBeenCalledTimes(1);
+    expect(onMissing).toHaveBeenCalledWith('a.gone');
   });
 
   it('invalidate() forces the next call to refetch', async () => {