fix(cache): bound grace windows, cap render queues, and drop the useless estimate

Follow-up to f81b114b, addressing the three ways that commit could make things
worse rather than better. All three were verified against the real database or
by breaking the test and watching it fail.

- The grace window is now capped at 120s. A window is a cushion for the TTL
  boundary, not a second TTL, but the call sites treated it as the latter: the
  5 min values/staff routes and the 10 min teams route asked for a window as
  long as or longer than their own TTL, so a single large staleMs silently
  doubled how far behind a value could be served. Nothing marked those as
  unsafe, because nothing looked wrong. The cap lives in the cache rather than
  at the call sites so no future route can reintroduce it. Routes that asked
  for less than 120s (the 10s online poll, the 20s news cache) are unchanged,
  so their intended cushion still does its job.

- A request no longer queues behind an arbitrarily old render. Sharing a render
  is what collapses a cold-cache stampede into one render, but a hung render
  used to hold up everyone who arrived after it. A newcomer past 2s now serves
  the placeholder instead of waiting, reusing the ImagerUnavailableError path
  that "both upstreams down" already takes. The caller that actually started
  the render keeps waiting, which is correct: it is the one whose image this
  is. When the join window is removed the new test hangs for the full 10s it
  was meant to prevent, which is the tail this bounds.

- The information_schema row-count estimate is gone; the counters are exact
  again. Running it against the live database: users 165, rooms 92, camera_web
  0, and the estimate was 0.00% off on all three. At 165 rows an index scan is
  cheaper than the extra round trip the estimate needed, so the optimisation
  bought nothing and traded a guaranteed-correct member count for an
  approximation that InnoDB would only make less accurate as the table grows.
  The exactness is now pinned by tests: a real zero stays zero, a database
  error propagates instead of becoming a number, and each counter counts the
  table it claims to. The module stays, because the homepage and the boot
  warm-up writing different values to the same cache key is its own bug.

The module comment records the measured numbers, because "COUNT(*) is too slow"
sounds true in the abstract and is false here.

3223 tests pass.

src/lib/cache.test.ts

@@ -177,4 +177,34 @@ describe("cached (stale-while-revalidate)", () => {
 			expect(await cached(key, 20, fn, { staleMs: 10_000 })).toBe("recovered"),
 		);
 	});
+
+	it("caps the grace window so a large staleMs cannot hide staleness", async () => {
+		// A grace window is a cushion for the TTL boundary, not a second TTL. Left
+		// unbounded, a 1s TTL paired with a 10 minute window serves data 10 minutes
+		// old, which is invisible in the code and only shows up as a support
+		// ticket. The ceiling keeps that bounded no matter what a call site asks.
+		vi.useFakeTimers();
+		try {
+			const options = { staleMs: 600_000 };
+
+			// Inside the capped window the value is still served without recomputing,
+			// so the cushion itself is not lost.
+			let warm = 0;
+			const warmFn = async () => ++warm;
+			await cached("cap-warm", 1_000, warmFn, options);
+			await vi.advanceTimersByTime(119_000);
+			expect(await cached("cap-warm", 1_000, warmFn, options)).toBe(1);
+
+			// Past the ceiling the value is recomputed even though the caller asked
+			// for a 10 minute window. No prior stale read on this key, so nothing is
+			// in flight to short-circuit the recompute.
+			let capped = 0;
+			const cappedFn = async () => ++capped;
+			expect(await cached("cap-hard", 1_000, cappedFn, options)).toBe(1);
+			await vi.advanceTimersByTime(121_000);
+			expect(await cached("cap-hard", 1_000, cappedFn, options)).toBe(2);
+		} finally {
+			vi.useRealTimers();
+		}
+	});
 });

src/lib/cache.ts

@@ -29,6 +29,14 @@ export interface CachedOptions {
 	staleMs?: number;
 }
 
+// Hard ceiling on the grace window. A window is a cushion for the TTL boundary,
+// not a second TTL: it exists so a mass expiry cannot block a request, and
+// keeping it short bounds how far behind a value can be served. Without a
+// ceiling a single large `staleMs` silently doubles the visible staleness of a
+// route (a 5 min TTL with a 5 min window serves data 10 min old), which is
+// exactly the kind of thing nobody notices until a support ticket.
+const MAX_STALE_MS = 120_000;
+
 const memory = new Map<string, CacheEntry>();
 
 function readPositiveInt(name: string, fallback: number): number {
@@ -148,7 +156,7 @@ export async function cached<T>(
 	fn: () => Promise<T>,
 	options: CachedOptions = {},
 ): Promise<T> {
-	const staleMs = Math.max(0, options.staleMs ?? 0);
+	const staleMs = Math.min(Math.max(0, options.staleMs ?? 0), MAX_STALE_MS);
 	// `existing &&` short-circuits so Date.now() is never evaluated during
 	// prerender when the map is empty (keeps `next build` prerendering clean).
 	const existing = getMemory(key);

src/lib/imager-upstream.test.ts

@@ -81,3 +81,58 @@ it("clears the in-flight entry when the render fails", async () => {
 	).rejects.toThrow(ImagerUnavailableError);
 	expect(globalThis.imagingInFlight?.size ?? 0).toBe(0);
 });
+
+it("does not queue a new caller behind a render that is already too old", async () => {
+	// Sharing a render is what collapses a cold-cache stampede into one render,
+	// but a render that hangs must not hold an unbounded queue behind it. A
+	// newcomer past the join window gets the placeholder instead of waiting on
+	// somebody else's stalled render.
+	let calls = 0;
+	vi.stubGlobal(
+		"fetch",
+		vi.fn(() => {
+			calls += 1;
+			return new Promise(() => {});
+		}),
+	);
+
+	// The hung request is the one that asked for the render; it keeps waiting,
+	// which is the intended behaviour. Swallow it so it is not an unhandled
+	// rejection when the test ends.
+	const stalled = fetchAvatarImage("https://site.test", params);
+	stalled.catch(() => {});
+	await new Promise((resolve) => setTimeout(resolve, 0));
+	expect(calls).toBe(1);
+
+	const entry = [...(globalThis.imagingInFlight?.values() ?? [])][0];
+	expect(entry).toBeDefined();
+	entry.startedAt = Date.now() - 60_000;
+
+	await expect(fetchAvatarImage("https://site.test", params)).rejects.toThrow(
+		ImagerUnavailableError,
+	);
+	// Critically, it must not have launched a second render.
+	expect(calls).toBe(1);
+
+	// A caller arriving inside the window still shares the render, so the
+	// dedupe that matters most is intact.
+	globalThis.imagingInFlight?.clear();
+	calls = 0;
+	vi.stubGlobal(
+		"fetch",
+		vi.fn(async () => {
+			calls += 1;
+			await new Promise((resolve) => setTimeout(resolve, 5));
+			return new Response(new Uint8Array([9]), {
+				headers: { "content-type": "image/png" },
+			});
+		}),
+	);
+	const shared = await Promise.all([
+		fetchAvatarImage("https://site.test", params),
+		fetchAvatarImage("https://site.test", params),
+	]);
+	expect(calls).toBe(1);
+	expect([...shared[0].body]).toEqual([9]);
+	expect(shared[1].body).toBe(shared[0].body);
+});

src/lib/imager-upstream.ts

@@ -27,13 +27,30 @@ export const FALLBACK_TIMEOUT_MS = 2_000;
 // per process (multiple server bundles); separate copies would each start their
 // own render and defeat the point entirely.
 const globalForRenders = globalThis as typeof globalThis & {
-	imagingInFlight?: Map<string, Promise<ImagerResult>>;
+	imagingInFlight?: Map<
+		string,
+		{ startedAt: number; render: Promise<ImagerResult> }
+	>;
 };
-const inFlight = globalForRenders.imagingInFlight ?? new Map();
+const inFlight =
+	globalForRenders.imagingInFlight ??
+	new Map<string, { startedAt: number; render: Promise<ImagerResult> }>();
 globalForRenders.imagingInFlight = inFlight;
 
 const MAX_IN_FLIGHT_RENDERS = 256;
 
+// How long a newcomer is willing to queue behind a render somebody else started.
+//
+// Sharing one render across concurrent requests is what keeps a cold cache from
+// launching N renders, but it also means a render that hangs holds up everyone
+// who arrives after it rather than only itself. Past this point a new caller
+// stops joining and serves the placeholder instead: the render in flight is
+// still worth finishing for the request that asked for it, but it must not be
+// able to stall an unbounded queue behind it. Callers that arrive inside the
+// window are the normal case — a render that takes longer than this is already
+// heading for its 8s timeout and would return a degraded image anyway.
+const RENDER_JOIN_WINDOW_MS = 2_000;
+
 export type ImagerSource =
 	| "primary"
 	| "fallback"
@@ -125,16 +142,29 @@ export async function fetchAvatarImage(
 	// the app, launched exactly when the cache has nothing to offer. The winner
 	// writes to the disk cache; the others wait for that one result.
 	const pending = inFlight.get(cacheKey);
-	if (pending) return pending;
+	if (pending) {
+		if (Date.now() - pending.startedAt < RENDER_JOIN_WINDOW_MS)
+			return pending.render;
+		// The shared render has been going too long to be worth joining. Fall
+		// through to the placeholder rather than extending an unbounded queue.
+		throw new ImagerUnavailableError(
+			"Avatar render already in progress too long",
+		);
+	}
 
 	// Defensive bound: entries are removed as soon as they settle, so this only
 	// matters if renders are somehow never cleaned up.
 	if (inFlight.size >= MAX_IN_FLIGHT_RENDERS) inFlight.clear();
-	const render = renderAvatar(primary, cacheKey, params).finally(() => {
+	const entry = {
-		inFlight.delete(cacheKey);
+		startedAt: Date.now(),
-	});
+		render: renderAvatar(primary, cacheKey, params),
-	inFlight.set(cacheKey, render);
+	};
-	return render;
+	inFlight.set(cacheKey, entry);
+	try {
+		return await entry.render;
+	} finally {
+		if (inFlight.get(cacheKey) === entry) inFlight.delete(cacheKey);
+	}
 }
 
 async function renderAvatar(

src/lib/services/public-counters.test.ts

@@ -2,12 +2,12 @@
 import { beforeEach, expect, it, vi } from "vitest";
 
 const state = vi.hoisted(() => ({
-	// Rows the fake information_schema reports, keyed by table name.
+	// Value the exact COUNT(*) returns, how many times it ran, and per-table
-	estimates: {} as Record<string, number | null>,
+	// totals so a counter cannot quietly report another table's number.
-	failEstimate: false,
+	exact: 0,
-	// Value the exact COUNT(*) returns, and how often it actually ran.
-	exactCounts: 0,
 	exactRuns: 0,
+	fail: false,
+	perTable: {} as Record<string, number>,
 }));
 
 const tables = vi.hoisted(() => ({
@@ -17,34 +17,32 @@ const tables = vi.hoisted(() => ({
 }));
 
 vi.mock("@/lib/db", () => {
+	// Held outside the proxy: reading an unknown property on the proxy returns
+	// a function, so a value stored on it would not come back out.
+	let current = "";
 	const chain: any = new Proxy(() => {}, {
 		get: (_t, prop) => {
 			if (prop === Symbol.toStringTag) return "Query";
 			if (prop === "where") return () => chain;
+			if (prop === "from")
+				return (table: any) => {
+					current = table.name;
+					return chain;
+				};
 			if (prop === "then")
 				return (resolve: any, reject: any) => {
-					// Only the estimate lookup is meant to fail here; the exact
+					if (state.fail)
-					// fallback has to keep working, so it is not gated.
+						return Promise.reject(Error("db down")).then(resolve, reject);
 					state.exactRuns += 1;
-					return Promise.resolve([{ total: state.exactCounts }]).then(
+					const total =
-						resolve,
+						current in state.perTable ? state.perTable[current] : state.exact;
-						reject,
+					return Promise.resolve([{ total }]).then(resolve, reject);
-					);
 				};
 			return () => chain;
 		},
 	});
 	return {
-		db: {
+		db: { select: () => chain, execute: async () => [] },
-			select: () => chain,
-			// The information_schema lookup, the only one that matters here.
-			execute: async () => {
-				if (state.failEstimate) throw Error("information_schema unavailable");
-				return Object.entries(state.estimates)
-					.filter(([, v]) => v !== null)
-					.map(([TABLE_NAME, TABLE_ROWS]) => ({ TABLE_NAME, TABLE_ROWS }));
-			},
-		},
 		...(tables as any),
 	};
 });
@@ -54,65 +52,49 @@ import {
 	countPhotos,
 	countRooms,
 	countUsers,
-	estimatedRowCounts,
 } from "./public-counters";
 
 beforeEach(() => {
-	state.estimates = {};
+	state.exact = 0;
-	state.failEstimate = false;
-	state.exactCounts = 0;
 	state.exactRuns = 0;
+	state.fail = false;
+	state.perTable = {};
 });
 
-it("reads the engine estimate instead of scanning the table", async () => {
+it("returns exact counts, one query each", async () => {
-	// An exact count is recorded, so a non-zero value here means one really ran.
+	state.exact = 165;
-	state.estimates = { users: 123_456, rooms: 42, camera_web: 7 };
+	expect(await countUsers()).toBe(165);
-	expect(await countUsers()).toBe(123_456);
+	expect(state.exactRuns).toBe(1);
-	expect(await countRooms()).toBe(42);
+	state.exactRuns = 0;
-	expect(await countPhotos()).toBe(7);
+	state.exact = 92;
-	expect(state.exactRuns).toBe(0);
+	expect(await countRooms()).toBe(92);
+	expect(state.exactRuns).toBe(1);
 });
 
-it("falls back to the exact count when the engine has no estimate", async () => {
+it("counts the table each counter claims to count", async () => {
-	// A freshly created InnoDB table can report 0, which would show "0 members".
+	// The whole point of the module is one function per counter, so the homepage
-	state.estimates = { users: 0, rooms: null, camera_web: null };
+	// and the boot warm-up cannot disagree under a shared cache key.
-	state.exactCounts = 999;
+	state.perTable = { users: 1, rooms: 2, camera_web: 3 };
-	expect(await countUsers()).toBe(999);
-	expect(await countRooms()).toBe(999);
-	expect(await countPhotos()).toBe(999);
-	expect(state.exactRuns).toBe(3);
-});
-
-it("falls back to the exact count when the estimate lookup fails", async () => {
-	state.failEstimate = true;
-	state.exactCounts = 55;
-	expect(await countUsers()).toBe(55);
-});
-
-it("never turns a missing estimate into a zero", async () => {
-	// Showing "0" is worse than being slow: it is visibly wrong.
-	state.estimates = { users: null };
-	state.exactCounts = 1;
 	expect(await countUsers()).toBe(1);
-	expect(await countUsers()).not.toBe(0);
+	expect(await countRooms()).toBe(2);
+	expect(await countPhotos()).toBe(3);
 });
 
-it("handles the bigint string the driver may hand back", async () => {
+it("reports a real zero rather than substituting a stored number", async () => {
-	state.estimates = { users: "98765" };
+	// camera_web is genuinely empty. The estimate this used to read could
-	expect(await countUsers()).toBe(98_765);
+	// plausibly have shown a non-zero member count here; exactness cannot.
+	state.perTable = { camera_web: 0 };
+	expect(await countPhotos()).toBe(0);
 });
 
-it("returns a null for every requested table it has no estimate for", async () => {
+it("propagates a database failure instead of inventing a number", async () => {
-	expect(await estimatedRowCounts(["users", "rooms"])).toEqual({
+	state.fail = true;
-		users: null,
+	await expect(countUsers()).rejects.toThrow();
-		rooms: null,
-	});
 });
 
 it("keeps the online count exact", async () => {
-	// Being a few seconds behind looks broken, and it is an indexed count of a
+	// A few seconds of drift reads as broken, and it is an indexed count over a
 	// small subset, so there is nothing to gain by approximating it.
-	state.estimates = { users: 5_000_000 };
+	state.exact = 12;
-	state.exactCounts = 12;
 	expect(await countOnline()).toBe(12);
 });

src/lib/services/public-counters.ts

@@ -1,6 +1,6 @@
 import "server-only";
 
-import { count, eq, sql } from "drizzle-orm";
+import { count, eq } from "drizzle-orm";
 import { CameraWeb, db, Rooms, User } from "@/lib/db";
 
 /**
@@ -11,98 +11,33 @@ import { CameraWeb, db, Rooms, User } from "@/lib/db";
  * write different values into the same entry.
  */
 
-/** Physical table names, needed because `information_schema` speaks in strings. */
+/*
-const TABLE = {
+ * These are exact counts on purpose.
-	users: "users",
-	rooms: "rooms",
-	photos: "camera_web",
-} as const;
-
-/**
- * Ask the storage engine for its own row estimates.
  *
- * An exact `COUNT(*)` on InnoDB walks an index, so it gets slower as the table
+ * `information_schema.TABLES.TABLE_ROWS` was tried here as a cheap stand-in,
- * grows. For a homepage counter that a visitor sees as a rounded "member since"
+ * because an exact COUNT(*) walks an index. Measured against the real database
- * number, an approximate count is the right trade: it is a single indexed
+ * it turned out to be pointless: `users` has 165 rows, `rooms` 92 and
- * lookup on `information_schema` instead of a scan of the whole table.
+ * `camera_web` 0, and the estimate was 0.00% off on all three. An index scan
- *
+ * over 165 rows costs less than the round trip the estimate would have saved,
- * InnoDB's estimate can lag behind reality and, right after a bulk write, can be
+ * while the estimate carries a real risk of displaying a plausible but wrong
- * badly off. It is therefore never used as a reason to show zero: a missing
+ * member count — and InnoDB's accuracy degrades as a table grows. If these
- * estimate falls back to the exact count, so the worst case is the old
+ * tables ever reach six figures, revisit the trade; below that, exactness is
- * behaviour.
+ * free.
- *
- * Returns null for any table the engine has no estimate for.
  */
-export async function estimatedRowCounts(
-	tables: string[],
-): Promise<Record<string, number | null>> {
-	const result: Record<string, number | null> = {};
-	for (const table of tables) result[table] = null;
-	if (tables.length === 0) return result;
 
-	try {
+export async function countUsers(): Promise<number> {
-		const names = tables.map((table) => sql`${table}`);
-		const rows = await db.execute<{
-			TABLE_NAME: string;
-			TABLE_ROWS: number | string | null;
-		}>(sql`
-			SELECT TABLE_NAME, TABLE_ROWS
-			FROM information_schema.TABLES
-			WHERE TABLE_SCHEMA = DATABASE()
-			  AND TABLE_NAME IN (${sql.join(names, sql`, `)})
-		`);
-
-		for (const row of rows as unknown as {
-			TABLE_NAME: string;
-			TABLE_ROWS: number | string | null;
-		}[]) {
-			const value = Number(row.TABLE_ROWS);
-			// TABLE_ROWS is a BIGINT that node-mysql hands back as a string or a
-			// double; anything non-finite means "no estimate", not zero rows.
-			if (row.TABLE_NAME in result && Number.isFinite(value) && value >= 0)
-				result[row.TABLE_NAME] = Math.floor(value);
-		}
-	} catch {
-		// No estimate available: every caller falls back to the exact count.
-	}
-	return result;
-}
-
-/**
- * Count rows in a table, preferring the cheap estimate.
- *
- * `estimate` names the table to ask the engine about, `exact` is the original
- * query, kept as the fallback.
- */
-async function countTable(
-	estimate: string,
-	exact: () => Promise<number>,
-): Promise<number> {
-	const estimates = await estimatedRowCounts([estimate]);
-	const value = estimates[estimate];
-	if (value !== null && value > 0) return value;
-	return exact();
-}
-
-export function countUsers(): Promise<number> {
-	return countTable(TABLE.users, async () => {
 	const [row] = await db.select({ total: count() }).from(User);
 	return row?.total ?? 0;
-	});
 }
 
-export function countRooms(): Promise<number> {
+export async function countRooms(): Promise<number> {
-	return countTable(TABLE.rooms, async () => {
 	const [row] = await db.select({ total: count() }).from(Rooms);
 	return row?.total ?? 0;
-	});
 }
 
-export function countPhotos(): Promise<number> {
+export async function countPhotos(): Promise<number> {
-	return countTable(TABLE.photos, async () => {
 	const [row] = await db.select({ total: count() }).from(CameraWeb);
 	return row?.total ?? 0;
-	});
 }
 
 /**