From 478c6633ca876a39dbe6e783e5325dcf2aa8b34c Mon Sep 17 00:00:00 2001 From: John McLear Date: Sat, 16 May 2026 06:10:51 +0100 Subject: [PATCH] fix(historical-author-cache): address Qodo review on #7769 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/node/db/HistoricalAuthorDataCache.ts | 76 ++++++++++++++----- src/node/db/Pad.ts | 13 ++++ .../specs/pad-historical-author-data.test.ts | 59 ++++++++++++-- 3 files changed, 122 insertions(+), 26 deletions(-) diff --git a/src/node/db/HistoricalAuthorDataCache.ts b/src/node/db/HistoricalAuthorDataCache.ts index 84df5bf4f..63be8c880 100644 --- a/src/node/db/HistoricalAuthorDataCache.ts +++ b/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; +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; +}; diff --git a/src/node/db/Pad.ts b/src/node/db/Pad.ts index e8c24b1ae..2ad936ed0 100644 --- a/src/node/db/Pad.ts +++ b/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(); diff --git a/src/tests/backend-new/specs/pad-historical-author-data.test.ts b/src/tests/backend-new/specs/pad-historical-author-data.test.ts index 9f46d16af..119c7c3d5 100644 --- a/src/tests/backend-new/specs/pad-historical-author-data.test.ts +++ b/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, ttlMs = 5_000, now = () => Date.now()) => - new HistoricalAuthorDataCache(() => ids, fetcher, ttlMs, now); +type Fetcher = (id: string) => Promise; +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 Promise>>; + let getAuthorMock: ReturnType>; 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((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 () => {