From 7f0ef81640eb597363439a156de9dc1df3f7587c Mon Sep 17 00:00:00 2001 From: Nick the Sick Date: Sat, 15 Aug 2026 06:47:34 +0200 Subject: [PATCH 1/4] perf: make the /explore aggregates index-only and cache them with SWR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /explore/authors took 9-14.5s on every production hit; /explore took 6-9s cold. All three author-directory queries grouped the whole of hive_book_author joined to hive_book and planned as an index probe plus a full-row fetch from the 1.8 GB table — 371k random reads. bun:sqlite is synchronous, so each one froze a whole worker's event loop, and at WEB_CONCURRENCY=3 that stalls a third of all traffic, not just those routes. Migration 024 adds hive_book(id, ratingsCount, rating, language) and hive_book_author(position, author, hiveId) so both sides of the join are covering. The queries name it with INDEXED BY deliberately: this database has no sqlite_stat1, and with no stats the planner prefers the UNIQUE autoindex and goes straight back to the table, so the index would be dead weight. ANALYZE was rejected as the alternative because it would re-plan every query in an app whose indexes were all tuned against the no-stats planner. Measured on a read-only snapshot of production (371,400 books, 1.86 GB): /explore/authors 14,414ms -> 565ms, /explore -> 358ms, both 2-5ms warm. Output is unchanged: same 500 rows in the same order, 8/8 featured covers. Also in this change: - Featured authors is now a strict prefix of the directory list rather than a second copy of the same aggregate, and covers come from a bounded per-author CTE instead of scanning the globally top-1200 books. - Caching moved inside the helpers (24h TTL, 1h stale-while-revalidate) so /explore, /explore/authors and XRPC getExplore share one policy. They had three, one of which was no cache at all. - /explore/authors honours ?lang=, and lang is validated against getAvailableLanguages everywhere before it can key a cached aggregate. - Fix WITH ... SELECT being classified as a non-reader in the Kysely wrapper, which made any CTE query return zero rows with no error at all. Reader status now comes from SQLite via stmt.columnNames. - Fix the anon page cache measuring its size limit on uncompressed bytes: /explore/genres is 641 KB raw but 30 KB gzipped, so it was rejected outright and has never been cached in production. - Fix the anon page cache key discarding the query string (unstorage's normalizeKey splits on "?"), which served /explore?lang=French the English render and /authors/X?page=2 page 1. - Stamp readThroughCache entries after the fetch resolves, so a slow fetch is not born stale. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 21 +- src/bun-sqlite-kysely.test.ts | 31 ++- src/bun-sqlite-kysely.ts | 18 +- src/db.ts | 60 +++++ src/middleware/anon-page-cache.test.ts | 141 ++++++++++++ src/middleware/anon-page-cache.ts | 29 ++- src/pages/authorDirectory.tsx | 151 +++---------- src/pages/explore.tsx | 48 +--- src/pages/genres.tsx | 5 +- src/routes/pages.tsx | 27 ++- src/utils/authorStats.test.ts | 297 +++++++++++++++++++++++++ src/utils/authorStats.ts | 188 ++++++++++++++++ src/utils/exploreGenres.ts | 80 +++++++ src/utils/getLanguages.ts | 20 ++ src/utils/readThroughCache.ts | 7 +- src/xrpc/router.ts | 34 ++- 16 files changed, 961 insertions(+), 196 deletions(-) create mode 100644 src/middleware/anon-page-cache.test.ts create mode 100644 src/utils/authorStats.test.ts create mode 100644 src/utils/authorStats.ts create mode 100644 src/utils/exploreGenres.ts diff --git a/AGENTS.md b/AGENTS.md index 99aab41..8c50f5f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,6 +57,8 @@ Worker threads (bundled to .output/server/workers/): - **Enrichment is queued, never inline**: routes call `enqueueEnrichment`/`enqueueEnrichmentBatch` (`src/utils/enrichQueue.ts`). The primary worker drains it every 5s at concurrency 3, with exponential backoff. `enrichBookWithDetailedData` holds its own semaphore (4) + 45s deadline. That drain interval × concurrency is also the **only** rate limit on requests to Goodreads — 36 fetches/min, healthy or not. Don't add a second one. - **`enrich_queue.attempts` counts answers from Goodreads, not failures.** A run reports `enrich_retry` in the wide-event bag: `retry` spends an attempt, `defer` costs nothing and re-queues on a decaying schedule, `dead` tombstones the book immediately. Anything the app decided on its own — a WAF challenge, a timeout, a transport error — is a `defer`. Getting this wrong is expensive: when refusals counted as attempts, one 6h window wrote off 2,854 books for 7 days apiece **without sending a single request on their behalf** (98% of everything the queue gave up on). The bound on defers is `MAX_QUEUE_AGE_MS` (7d from `enqueuedAt`, which survives re-enqueue), not the attempt counter. - **Author lookups use `hive_book_author` join** (mig 020), not `LIKE`. This is exact identity, not text search. +- **The `/explore` aggregates say `INDEXED BY idx_hive_book_stats`, and that is not decoration.** Each groups the whole of `hive_book_author`/`hive_book_genre` joined to `hive_book`, and migration 024 added `hive_book(id, ratingsCount, rating, language)` so that join can be index-only. But **this database has never been `ANALYZE`d** — with no `sqlite_stat1` the planner prefers the UNIQUE `sqlite_autoindex_hive_book_1` for an `id = ?` equality and fetches the whole row from the 1.62 GB table anyway, so the index does nothing unless the query names it. Measured at 350k books: `/explore/authors` 2742ms → 260ms, `/explore`'s genre list 209ms → 21ms. Don't "clean up" the hint, and don't reach for `ANALYZE` instead — it would re-plan every query in an app whose indexes were all tuned against the no-stats planner. `src/utils/authorStats.test.ts` asserts the plans. +- **`bun:sqlite` is synchronous, so a slow query is a whole-worker outage.** `stmt.all()` blocks the event loop, and production runs 3 processes — a 3s aggregate on a request path stalls a third of _all_ traffic, not just that route. Hence the explore aggregates are cached with **stale-while-revalidate** (`ttl: 24h, revalidateAfter: 1h`), not a plain TTL: a plain TTL makes every expiry a synchronous cliff for whichever request draws the short straw. The caching lives _inside_ `src/utils/authorStats.ts` / `src/utils/exploreGenres.ts`, not at the call sites — the three consumers (`/explore`, `/explore/authors`, XRPC `getExplore`) used to wrap the same query in three different policies, one of which was no cache at all. - **Library re-sync** fans out at most `REFETCH_SEARCH_CONCURRENCY` (3) searches. **The app shell scroller — never put `overflow-*-auto` on `
`.** The `jsxRenderer` in @@ -123,6 +125,10 @@ into a 304. Without it an e-reader re-downloads every book on every sync. **Anonymous page cache** (`src/middleware/anon-page-cache.ts`): serves GET requests without a `sid` cookie on `/books/*`, `/explore*`, `/authors/*` from KV (gzipped HTML, 1h TTL). Prod-only. +**The cache key percent-encodes the query; it must never be joined with a literal `?`.** unstorage's `normalizeKey` is `key.split("?")[0].replace(/[/\\]/g, ":")…` — it _discards the query string_. A `?`-joined key therefore collapsed every variant of a path onto one entry, which made `ALLOWED_QUERY_PARAMS` and the sorted-query construction dead code and served visibly wrong pages: `/explore?lang=French` got the English render, `/authors/X?page=2` got page 1, `/explore/genres/Y?sort=relevance` got the popularity sort. The key is now `page:{pathname}:q:{encodeURIComponent(query)}` — `encodeURIComponent` escapes `?`, `/` and `\`, the only characters `normalizeKey` touches. Changing the key format orphans the old rows; the 15-minute sweep in `src/context.ts` clears them at 2× TTL. + +**The size limit is measured on the bytes we store, i.e. after gzip** (`MAX_STORED_BYTES`, 256 KB), with a separate 4 MB ceiling on the uncompressed buffer purely to bound memory. It used to compare the _uncompressed_ body against 512 KB and then store the gzipped form, which rejected pages that would have cost ~25 KB of KV. That matters because production inlines the whole CSS bundle into `` (`getInlineCss`, `src/utils/manifest.ts`) and `/explore/authors` renders 500 author rows on top of it — near enough to the old ceiling to fall off it, and the failure is silent (a rejected page sets no `x-page-cache` header at all, which is the diagnostic: `curl -sD- ` and look for it). + **Caching policy** lives in one place: `src/utils/cacheHeaders.ts`. One rule — **signed in (`sid` cookie) → `private, no-store` on every path; signed out → cache aggressively** so Cloudflare absorbs the scraper load. Three layers apply @@ -186,9 +192,12 @@ Two traps this encodes, both of which caused real bugs: - `/app` → `src/pages/app.tsx` — iOS app landing - `/import` → `src/pages/import.tsx` — CSV import page, SSE progress - `/search` → `src/pages/searchResults.tsx` (zValidator query `q`/`page`/`lang`) -- `/explore` → `src/pages/explore.tsx` — explore hub +- `/explore` → `src/pages/explore.tsx` — explore hub (`?lang=`) - `/explore/genres` → `src/pages/genres.tsx`; `/explore/genres/:genre` → `src/pages/genreBooks.tsx` -- `/explore/authors` → `src/pages/authorDirectory.tsx` +- `/explore/authors` → `src/pages/authorDirectory.tsx` (`?lang=`) + +**`?lang=` is validated against `getAvailableLanguages`, never passed through** (`resolveLanguage`, `src/utils/getLanguages.ts`). It keys a cached 356k-row aggregate _and_ is in the anon page cache's `ALLOWED_QUERY_PARAMS`, so an arbitrary string is an unbounded KV-cardinality and CPU amplifier. Related: `/explore/authors` used to ignore `lang` entirely while `/explore` linked to it _with_ `lang` and the page cache keyed on it — every language a crawler found became its own cache entry holding byte-identical HTML, each paying its own cold render. + - `/authors/:author` → `src/pages/authorBooks.tsx` - `/genres`, `/genres/:genre` → 301 redirects to `/explore/genres` - `/.well-known/atproto-did` → returns DID constant @@ -455,7 +464,9 @@ Client hooks/utils: `useSearchBooks.ts`, `useDebounce.ts`, `icons.tsx`, `debounc ### Database (`src/db.ts`) -SQLite via Kysely. Schema + all migrations (001–023) in one file. `createDb` sets WAL/perf PRAGMAs. `mmap_size` defaults to 0 (see `DB_MMAP_SIZE` in `src/env.ts`). Kysely talks to `bun:sqlite` through `src/bun-sqlite-kysely.ts`, which rewrites `begin` to `BEGIN IMMEDIATE` (deferred transactions fail with `SQLITE_BUSY_SNAPSHOT` across cluster processes). +SQLite via Kysely. Schema + all migrations (001–024) in one file. `createDb` sets WAL/perf PRAGMAs. `mmap_size` defaults to 0 (see `DB_MMAP_SIZE` in `src/env.ts`). Kysely talks to `bun:sqlite` through `src/bun-sqlite-kysely.ts`, which rewrites `begin` to `BEGIN IMMEDIATE` (deferred transactions fail with `SQLITE_BUSY_SNAPSHOT` across cluster processes). + +That wrapper also decides `statement.reader`, which is how Kysely picks `all()` (rows) over `run()` (changes). **It asks SQLite — `stmt.columnNames` is empty for anything that doesn't produce rows — rather than pattern-matching the SQL text.** The old regex was anchored on a leading `SELECT`, so `WITH cte AS (…) SELECT …` was classified as a write and Kysely got **zero rows with no error of any kind**; the author-directory cover lookup is a window function over a CTE and silently returned nothing. `columnNames` also gets the converse right, which a regex struggles with: `WITH cte AS (…) INSERT INTO …` is not a reader. | Table | Purpose | Key columns | | --------------------- | ------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | @@ -520,7 +531,9 @@ SQLite-backed unstorage. Mounts: `search:` (in-memory LRU), `profile:`, `identit | `circuitBreaker.ts` | Three-state breaker — **`auth/restore-guard.ts` only.** Right when refusing is cheaper for a waiting user than failing; wrong for scraping, where the queue can defer instead. See the note under Scrapers | | `bookIdentifiers.ts` | ISBN/ID normalization + persistence | | `bookProgress.ts` | BookProgress serialization | -| `readThroughCache.ts` | KV read-through with TTL | +| `readThroughCache.ts` | KV read-through with TTL + optional `revalidateAfter` (stale-while-revalidate). Prefer SWR for anything expensive — a plain TTL makes every expiry a blocking recompute on a request path. The entry is stamped **after** the fetch resolves, so a slow fetch isn't born stale | +| `authorStats.ts` | `getAuthorStats` / `getFeaturedAuthors` — the `/explore` author aggregates, SWR-cached inside the helper, `INDEXED BY idx_hive_book_stats`. Featured is a strict prefix of the directory list, not its own query | +| `exploreGenres.ts` | `getTopGenres` — same, for genres. Only joins `hive_book` when a language is given | | `csv.ts` | Goodreads/StoryGraph CSV parsers | | `lists.ts` | Book list (shelf) CRUD against PDS | | `readingStats.ts` | Reading stats aggregation by year | diff --git a/src/bun-sqlite-kysely.test.ts b/src/bun-sqlite-kysely.test.ts index 41517b6..b96655a 100644 --- a/src/bun-sqlite-kysely.test.ts +++ b/src/bun-sqlite-kysely.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, beforeEach } from "bun:test"; import { Database as DatabaseSync } from "bun:sqlite"; -import { Kysely, SqliteDialect, type Generated } from "kysely"; +import { Kysely, SqliteDialect, sql, type Generated } from "kysely"; import { toImmediateTransaction, wrapBunSqliteForKysely } from "./bun-sqlite-kysely"; @@ -86,6 +86,35 @@ describe("wrapBunSqliteForKysely", () => { expect(rows[0]?.note).toBe("n"); }); + it("returns rows from a query fronted by a CTE", async () => { + // `WITH ... SELECT` used to be classified as a non-reader (the check was + // anchored on a leading SELECT), so Kysely called run() and the query + // yielded zero rows with no error whatsoever. The author-directory cover + // lookup is a window function over a CTE, and it silently returned nothing. + await db.insertInto("thing").values({ name: "a", note: "n" }).execute(); + + const rows = await sql<{ name: string }>` + WITH ranked AS ( + SELECT name, ROW_NUMBER() OVER (ORDER BY id) AS rn FROM thing + ) + SELECT name FROM ranked WHERE rn = 1 + `.execute(db); + + expect(rows.rows.map((r) => r.name)).toEqual(["a"]); + }); + + it("still reports affected rows for a write fronted by a CTE", async () => { + // The converse: a leading CTE does not make a statement a reader. + await db.insertInto("thing").values({ name: "a", note: null }).execute(); + + const res = await sql` + WITH targets AS (SELECT id FROM thing WHERE name = 'a') + UPDATE thing SET name = 'b' WHERE id IN (SELECT id FROM targets) + `.execute(db); + + expect(Number(res.numAffectedRows)).toBe(1); + }); + it("runs transactions", async () => { await db.transaction().execute(async (trx) => { await trx.insertInto("thing").values({ name: "in-txn", note: null }).execute(); diff --git a/src/bun-sqlite-kysely.ts b/src/bun-sqlite-kysely.ts index 74a3b9b..479bb54 100644 --- a/src/bun-sqlite-kysely.ts +++ b/src/bun-sqlite-kysely.ts @@ -25,9 +25,21 @@ export interface KyselySqliteStatement { * rows must report true — including `INSERT`/`UPDATE`/`DELETE ... RETURNING` * (SQLite >= 3.35), which otherwise executes fine but hands Kysely zero rows, so * `.returning(...).executeTakeFirstOrThrow()` fails with "no result". + * + * SQLite answers this exactly, so ask it rather than pattern-matching the text: + * a prepared statement's `columnNames` is empty for anything that doesn't + * produce rows. The regex below is only a fallback for a runtime that doesn't + * expose it. + * + * This used to be `/^\s*SELECT\b/` plus a RETURNING check, which got + * `WITH cte AS (...) SELECT ...` wrong — a leading CTE is extremely common for + * window-function queries, and misclassifying one produces **zero rows with no + * error at all**. `columnNames` also gets the converse right, where a regex + * struggles: `WITH cte AS (...) INSERT INTO ...` is not a reader. */ -function isReaderStatement(sql: string): boolean { - return /^\s*SELECT\b/i.test(sql) || /\bRETURNING\b/i.test(sql); +function isReaderStatement(stmt: { columnNames?: string[] }, sql: string): boolean { + if (Array.isArray(stmt.columnNames)) return stmt.columnNames.length > 0; + return /^\s*(?:SELECT|WITH)\b/i.test(sql) || /\bRETURNING\b/i.test(sql); } /** @@ -55,7 +67,7 @@ export function wrapBunSqliteForKysely(db: DatabaseSync): KyselySqliteDatabase { prepare(rawSql: string): KyselySqliteStatement { const sql = toImmediateTransaction(rawSql); const stmt = db.prepare(sql); - const reader = isReaderStatement(sql); + const reader = isReaderStatement(stmt, sql); return { get reader() { return reader; diff --git a/src/db.ts b/src/db.ts index 6d00483..017e224 100644 --- a/src/db.ts +++ b/src/db.ts @@ -1006,6 +1006,66 @@ migrations["023"] = { }, }; +/** + * Covering indexes for the /explore family's author and genre aggregates. + * + * Every one of them is a `GROUP BY` over the whole of `hive_book_author` (or + * `hive_book_genre`) joined to `hive_book`, and every one of them planned as + * `SEARCH b USING INDEX sqlite_autoindex_hive_book_1 (id=?)` — an index probe + * to get a rowid, then a fetch of the whole 356k-row, 1.62 GB `hive_book` row + * just to read `ratingsCount`/`rating`/`language`. Against the 16 MB + * `cache_size` with `mmap_size = 0` that is 356k random reads; `/explore` took + * 6-9s and `/explore/authors` 9-14.5s, and `bun:sqlite` is synchronous, so + * each one froze a whole worker's event loop. + * + * `idx_hive_book_stats` is ~18 MB and holds exactly the columns those + * aggregates read, so the join becomes index-only and the working set fits in + * the page cache. Deliberately WITHOUT `thumbnail` — URLs are 60-100 bytes a + * row and would triple the index, evicting the thing we are trying to keep + * resident. The handful of thumbnails the featured row needs are fetched by id + * afterwards (see `src/utils/authorStats.ts`). + * + * IMPORTANT: the index alone does nothing. This database has never been + * ANALYZEd, and with no `sqlite_stat1` the planner prefers the UNIQUE + * `sqlite_autoindex_hive_book_1` for an `id = ?` equality and goes right back + * to the table. The queries therefore say `INDEXED BY idx_hive_book_stats` + * explicitly. Shipping `ANALYZE` instead would re-plan every other query in an + * app whose indexes were all hand-tuned against the no-stats planner — far too + * much blast radius for an index migration. If you drop this index, the + * `INDEXED BY` clauses become hard errors rather than silent 9s regressions, + * which is the intent. + */ +migrations["024"] = { + async up(db: Kysely) { + // `IF NOT EXISTS` throughout (as migration 012 does): a half-applied state + // — a backup restored without its WAL, an index created by hand while + // debugging — otherwise makes this throw `index already exists` on every + // boot, and since migrations run inside the startup barrier that is a + // permanent crash loop rather than a degraded page. + await sql`CREATE INDEX IF NOT EXISTS idx_hive_book_stats ON hive_book(id, ratingsCount, rating, language)`.execute( + db, + ); + + // `idx_hive_book_author_first` (migration 020) is a strict prefix of this. + // Adding `hiveId` makes the `WHERE position = 0 GROUP BY author` side of + // the aggregate index-only too — the join key no longer costs a rowid + // fetch per row. Every other consumer of this table filters on `author` + // alone and is served by `idx_hive_book_author_author`, so the old index + // has no remaining reader. + await sql`CREATE INDEX IF NOT EXISTS idx_hive_book_author_first_cover ON hive_book_author(position, author, hiveId)`.execute( + db, + ); + await sql`DROP INDEX IF EXISTS idx_hive_book_author_first`.execute(db); + }, + async down(db: Kysely) { + await sql`CREATE INDEX IF NOT EXISTS idx_hive_book_author_first ON hive_book_author(position, author)`.execute( + db, + ); + await sql`DROP INDEX IF EXISTS idx_hive_book_author_first_cover`.execute(db); + await sql`DROP INDEX IF EXISTS idx_hive_book_stats`.execute(db); + }, +}; + // APIs export const createDb = (location: string): { db: Database; sqlite: DatabaseSync } => { diff --git a/src/middleware/anon-page-cache.test.ts b/src/middleware/anon-page-cache.test.ts new file mode 100644 index 0000000..cb9653f --- /dev/null +++ b/src/middleware/anon-page-cache.test.ts @@ -0,0 +1,141 @@ +/** + * The anon page cache is prod-only, so nothing else exercises it. The size + * guard in particular fails *silently* — an oversized response is served + * normally and simply never stored, so the only symptom is that the page stays + * slow forever. + */ +import { describe, it, expect, beforeEach } from "bun:test"; +import { Hono } from "hono"; +import { createStorage, type Storage } from "unstorage"; + +import sqliteKv, { createSharedKvDb } from "../sqlite-kv"; +import { anonPageCache } from "./anon-page-cache"; + +let kv: Storage; + +beforeEach(() => { + // The real driver, not unstorage's memory one: freshness is decided by + // `getMeta().mtime`, and the memory driver returns no meta at all, so every + // read would look stale and the cache would never appear to work. + const { db } = createSharedKvDb(":memory:"); + kv = createStorage({ driver: sqliteKv({ table: "page_cache", db }) }); +}); + +/** Hono app with the cache in front of a handler that counts its renders. */ +function appServing(body: string | (() => string)) { + let renders = 0; + const app = new Hono(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + app.use("/p/*", anonPageCache(kv) as any); + app.get("/p/:name", (c) => { + renders++; + return c.html(typeof body === "function" ? body() : body); + }); + return { app, renders: () => renders }; +} + +const get = (app: Hono, path: string, headers?: Record) => + app.request(new Request(`http://test.local${path}`, { headers })); + +describe("anonPageCache storage", () => { + it("stores and replays an anonymous page", async () => { + const { app, renders } = appServing("hello"); + + const first = await get(app, "/p/a"); + expect(first.headers.get("x-page-cache")).toBe("miss"); + + const second = await get(app, "/p/a"); + expect(second.headers.get("x-page-cache")).toBe("hit"); + expect(await second.text()).toBe("hello"); + expect(renders()).toBe(1); + }); + + it("caches a large but compressible page", async () => { + // The regression this guards: the limit used to be measured on the + // uncompressed body, so a page like /explore/authors — 500 near-identical + // author rows on top of the inlined CSS bundle — could be rejected at + // ~513 KB even though it stores in ~25 KB. Every hit then re-rendered, + // which meant re-running the aggregate behind it. + const row = `Someone`; + const big = `${row.repeat(3000)}`; + expect(Buffer.byteLength(big)).toBeGreaterThan(512 * 1024); + + const { app, renders } = appServing(big); + expect((await get(app, "/p/big")).headers.get("x-page-cache")).toBe("miss"); + expect((await get(app, "/p/big")).headers.get("x-page-cache")).toBe("hit"); + expect(renders()).toBe(1); + }); + + it("refuses a page that is still too large once compressed", async () => { + // Random data doesn't compress, so this exceeds the stored-bytes ceiling. + const incompressible = `${Buffer.from( + crypto.getRandomValues(new Uint8Array(400 * 1024)), + ).toString("base64")}`; + + const { app, renders } = appServing(incompressible); + // No x-page-cache header at all is the signal that a response was judged + // uncacheable — that is the diagnostic to reach for in production. + expect((await get(app, "/p/rand")).headers.get("x-page-cache")).toBeNull(); + await get(app, "/p/rand"); + expect(renders()).toBe(2); + }); + + it("bypasses signed-in requests and downgrades their Cache-Control", async () => { + const { app, renders } = appServing("personal"); + + const res = await get(app, "/p/a", { cookie: "sid=abc" }); + expect(res.headers.get("x-page-cache")).toBeNull(); + expect(res.headers.get("cache-control")).toBe("private, no-store"); + + await get(app, "/p/a", { cookie: "sid=abc" }); + expect(renders()).toBe(2); + }); + + it("passes through requests carrying a param outside the allowlist", async () => { + const { app, renders } = appServing("x"); + + expect((await get(app, "/p/a?utm_source=x")).headers.get("x-page-cache")).toBeNull(); + await get(app, "/p/a?utm_source=x"); + expect(renders()).toBe(2); + }); + + it("keys allowlisted params separately, order-independently", async () => { + // unstorage's normalizeKey is `key.split("?")[0]...`, so a key joined with + // a literal `?` loses its query entirely and every variant of a path + // collapses onto one entry — `?lang=fr` was served the `?lang=en` page. + let n = 0; + const { app } = appServing(() => `${n++}`); + + expect(await (await get(app, "/p/a?lang=en&page=2")).text()).toBe("0"); + // Same params in a different order is the same page. + expect(await (await get(app, "/p/a?page=2&lang=en")).text()).toBe("0"); + // A different value is a different page. + expect(await (await get(app, "/p/a?lang=fr&page=2")).text()).toBe("1"); + // As is no params at all. + expect(await (await get(app, "/p/a")).text()).toBe("2"); + // ...and each stays independently addressable afterwards. + expect(await (await get(app, "/p/a?lang=en&page=2")).text()).toBe("0"); + expect(await (await get(app, "/p/a?lang=fr&page=2")).text()).toBe("1"); + }); + + it("keeps paths distinct when a param value contains a separator", async () => { + let n = 0; + const { app } = appServing(() => `${n++}`); + + expect(await (await get(app, "/p/a?lang=x/y")).text()).toBe("0"); + expect(await (await get(app, "/p/a?lang=x%3Fy")).text()).toBe("1"); + expect(await (await get(app, "/p/a?lang=x/y")).text()).toBe("0"); + }); + + it("does not store a response carrying a Set-Cookie", async () => { + const app = new Hono(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + app.use("/p/*", anonPageCache(kv) as any); + app.get("/p/:name", (c) => { + c.header("set-cookie", "sid=new"); + return c.html("x"); + }); + + expect((await get(app, "/p/a")).headers.get("x-page-cache")).toBeNull(); + }); +}); diff --git a/src/middleware/anon-page-cache.ts b/src/middleware/anon-page-cache.ts index ca67fe9..8cfb4d9 100644 --- a/src/middleware/anon-page-cache.ts +++ b/src/middleware/anon-page-cache.ts @@ -21,7 +21,19 @@ import { NO_STORE, hasSessionCookie } from "../utils/cacheHeaders"; const ALLOWED_QUERY_PARAMS = new Set(["page", "sort", "lang", "review-id"]); export const PAGE_CACHE_TTL_MS = 60 * 60 * 1000; // matches Cache-Control max-age=3600 on these routes -const MAX_BODY_BYTES = 512 * 1024; + +/** + * What we actually store, so this is what the limit is measured against. The + * guard used to compare the *uncompressed* body against 512 KB and then store + * the gzipped form — which rejected pages that would have cost ~25 KB of KV. + * `/explore/authors` renders 500 near-identical author rows on top of the + * inlined CSS bundle (see `getInlineCss` in src/utils/manifest.ts) and sits + * close enough to that old ceiling to fall off it. + */ +const MAX_STORED_BYTES = 256 * 1024; +/** Separate ceiling on the pre-compression buffer, purely to bound the memory + * a single response can cost us. Nothing legitimate on these routes is close. */ +const MAX_BODY_BYTES = 4 * 1024 * 1024; /** Served on cache hits and misses when the route's Cache-Control didn't reach * the final response (headers set via the cacheControl helper after next() @@ -73,8 +85,10 @@ async function extractCacheable(res: Response): Promise { if (!contentType.includes("text/html")) return null; const body = await res.clone().text(); if (Buffer.byteLength(body) > MAX_BODY_BYTES) return null; + const gzipped = Bun.gzipSync(body); + if (gzipped.byteLength > MAX_STORED_BYTES) return null; return { - bodyGzipB64: Buffer.from(Bun.gzipSync(body)).toString("base64"), + bodyGzipB64: Buffer.from(gzipped).toString("base64"), contentType, cacheControl: res.headers.get("cache-control") || DEFAULT_CACHE_CONTROL, }; @@ -104,7 +118,16 @@ export function anonPageCache(kv: Storage) { .sort(([a], [b]) => a.localeCompare(b)) .map(([k, v]) => `${k}=${v}`) .join("&"); - const key = `page:${url.pathname}${query ? `?${query}` : ""}`; + // The query is percent-encoded into the key, NOT appended after a literal + // `?`. unstorage's `normalizeKey` is `key.split("?")[0].replace(...)` — it + // throws the query string away — so a `?`-joined key silently collapsed + // every variant of a path onto one entry: `/explore?lang=French` was served + // the English page, `/authors/X?page=2` was served page 1, and + // `/explore/genres/Y?sort=relevance` was served the popularity sort. That + // made ALLOWED_QUERY_PARAMS and this sort dead code. `encodeURIComponent` + // escapes `?`, `/` and `\`, which are the only characters normalizeKey + // touches, so the key survives it intact. + const key = `page:${url.pathname}${query ? `:q:${encodeURIComponent(query)}` : ""}`; // Fresh cached copy? const meta = await kv.getMeta(key); diff --git a/src/pages/authorDirectory.tsx b/src/pages/authorDirectory.tsx index ebe202c..5c86d54 100644 --- a/src/pages/authorDirectory.tsx +++ b/src/pages/authorDirectory.tsx @@ -1,22 +1,10 @@ import { type FC } from "hono/jsx"; import { useRequestContext } from "hono/jsx-renderer"; -import { sql } from "kysely"; import { endTime, startTime } from "hono/timing"; -import type { Kysely } from "kysely"; -import type { Storage } from "unstorage"; -import type { DatabaseSchema } from "../db"; -import { readThroughCache } from "../utils/readThroughCache"; +import { getAuthorStats, getFeaturedAuthors } from "../utils/authorStats"; import { sourceCoverImageUrl } from "../utils/imageProxy"; - -export interface AuthorWithStats { - author: string; - totalRatings: number; - avgRating: number | null; - bookCount: number; - thumbnail: string | null; -} - -type AuthorStats = Omit; +import { LanguageSelect } from "./components/LanguageSelect"; +import { buildUrl } from "./utils/buildUrl"; function formatCount(count: number): string { if (count < 10) return `${count}`; @@ -27,84 +15,6 @@ function formatCount(count: number): string { const FEATURED_COUNT = 8; -/** - * Returns top authors with stats and their most popular book thumbnail. - * Uses two queries to avoid a slow correlated subquery: - * 1. Aggregation (GROUP BY first author, no thumbnail) - * 2. Single scan of top books by ratingsCount to resolve thumbnails in JS - * - * @param language - optional language filter; when set, only books in that language are counted - */ -export async function getTopAuthors( - db: Kysely, - limit: number, - language?: string, -): Promise { - // Groups the normalized hive_book_author table (migration 020) rather than - // re-deriving the first author per row with instr/substr/trim over the whole - // of hive_book. `position = 0` is the credited first author. - const langCondition = language ? sql`AND b.language = ${language}` : sql``; - const statsResult = await sql` - SELECT - a.author as author, - SUM(COALESCE(b.ratingsCount, 0)) as totalRatings, - ROUND(AVG(CASE WHEN b.rating IS NOT NULL AND b.rating > 0 THEN b.rating END) / 1000.0, 1) as avgRating, - COUNT(*) as bookCount - FROM hive_book_author a - JOIN hive_book b ON b.id = a.hiveId - WHERE a.position = 0 ${langCondition} - GROUP BY a.author - HAVING bookCount >= 2 AND totalRatings > 0 - ORDER BY totalRatings DESC - LIMIT ${limit} - `.execute(db); - - const authors = statsResult.rows; - if (authors.length === 0) return []; - - // Resolve thumbnails with a single forward scan of the most-rated books. - // All top-N authors' best books appear well within the first limit*150 rows. - const thumbLangCondition = language ? sql`AND b.language = ${language}` : sql``; - const thumbResult = await sql<{ author: string; thumbnail: string }>` - SELECT a.author as author, b.thumbnail as thumbnail - FROM hive_book_author a - JOIN hive_book b ON b.id = a.hiveId - WHERE a.position = 0 AND b.thumbnail IS NOT NULL AND b.thumbnail != '' ${thumbLangCondition} - ORDER BY b.ratingsCount DESC - LIMIT ${limit * 150} - `.execute(db); - - const thumbnailByAuthor = new Map(); - for (const row of thumbResult.rows) { - if (!thumbnailByAuthor.has(row.author)) { - thumbnailByAuthor.set(row.author, row.thumbnail); - } - } - - return authors.map((a) => ({ - ...a, - thumbnail: thumbnailByAuthor.get(a.author) ?? null, - })); -} - -async function getAllAuthors(db: Kysely): Promise { - const result = await sql` - SELECT - a.author as author, - SUM(COALESCE(b.ratingsCount, 0)) as totalRatings, - ROUND(AVG(CASE WHEN b.rating IS NOT NULL AND b.rating > 0 THEN b.rating END) / 1000.0, 1) as avgRating, - COUNT(*) as bookCount - FROM hive_book_author a - JOIN hive_book b ON b.id = a.hiveId - WHERE a.position = 0 - GROUP BY a.author - HAVING bookCount >= 2 AND totalRatings > 0 - ORDER BY totalRatings DESC - LIMIT 500 - `.execute(db); - return result.rows; -} - const AuthorCover: FC<{ thumbnail: string | null; author: string }> = ({ thumbnail, author }) => { if (thumbnail) { return ( @@ -123,33 +33,28 @@ const AuthorCover: FC<{ thumbnail: string | null; author: string }> = ({ thumbna ); }; -export const AuthorDirectory: FC = async () => { +interface AuthorDirectoryProps { + lang?: string; + languages: string[]; +} + +export const AuthorDirectory: FC = async ({ lang, languages }) => { const c = useRequestContext(); const { db, kv } = c.get("ctx"); - const cacheOpts = { ttl: 300_000 }; // 5 minutes startTime(c, "authors-featured"); startTime(c, "authors-list"); + // `featured` is the top FEATURED_COUNT of `all` plus covers, so on a cold + // cache this is one aggregate, not two. Both are cached with SWR inside the + // helpers (see src/utils/authorStats.ts). const [featured, all] = await Promise.all([ - readThroughCache( - kv as Storage, - "authors:featured", - () => getTopAuthors(db, FEATURED_COUNT), - [], - cacheOpts, - ).then((r) => { + getFeaturedAuthors(db, kv, FEATURED_COUNT, lang).then((r) => { endTime(c, "authors-featured"); return r; }), - readThroughCache( - kv as Storage, - "authors:all", - () => getAllAuthors(db), - [], - cacheOpts, - ).then((r) => { + getAuthorStats(db, kv, lang).then((r) => { endTime(c, "authors-list"); return r; }), @@ -167,7 +72,7 @@ export const AuthorDirectory: FC = async () => { Explore @@ -176,13 +81,21 @@ export const AuthorDirectory: FC = async () => { Authors -
-

- Explore Authors -

-

- Discover books by your favourite authors. -

+
+
+

+ Explore Authors +

+

+ Discover books by your favourite authors. +

+
+
{/* Featured authors */} @@ -193,7 +106,7 @@ export const AuthorDirectory: FC = async () => {
{featured.map((author) => ( @@ -249,7 +162,7 @@ export const AuthorDirectory: FC = async () => {
{all.map((author) => ( diff --git a/src/pages/explore.tsx b/src/pages/explore.tsx index 4fca945..2d47856 100644 --- a/src/pages/explore.tsx +++ b/src/pages/explore.tsx @@ -1,12 +1,10 @@ import { type FC } from "hono/jsx"; import { useRequestContext } from "hono/jsx-renderer"; -import { sql } from "kysely"; import { endTime, startTime } from "hono/timing"; -import type { Storage } from "unstorage"; import { getEmoji } from "./genreEmoji"; -import { getTopAuthors, type AuthorWithStats } from "./authorDirectory"; +import { getFeaturedAuthors } from "../utils/authorStats"; +import { getTopGenres } from "../utils/exploreGenres"; import { StarDisplay } from "./components/cards/StarDisplay"; -import { readThroughCache } from "../utils/readThroughCache"; import { sourceCoverImageUrl } from "../utils/imageProxy"; import { LanguageSelect } from "./components/LanguageSelect"; import { buildUrl } from "./utils/buildUrl"; @@ -18,12 +16,8 @@ function formatCount(count: number): string { return `${Math.floor(count / 100) * 100}+`; } -interface GenreCount { - genre: string; - count: number; -} - -const CACHE_TTL = 3_600_000; // 1 hour +const TOP_GENRE_COUNT = 6; +const TOP_AUTHOR_COUNT = 8; interface ExploreProps { lang?: string; @@ -37,39 +31,15 @@ export const Explore: FC = async ({ lang, languages }) => { startTime(c, "explore-genres"); startTime(c, "explore-authors"); - const langCacheKey = lang || "all"; - + // Both aggregates are cached with stale-while-revalidate inside their + // helpers, and shared with /explore/authors and XRPC getExplore so the three + // callers can't drift to three different TTLs again. const [genres, topAuthors] = await Promise.all([ - readThroughCache( - kv as Storage, - `explore:genres:${langCacheKey}`, - () => { - let query = db - .selectFrom("hive_book_genre") - .innerJoin("hive_book", "hive_book.id", "hive_book_genre.hiveId") - .select(["hive_book_genre.genre", sql`COUNT(*)`.as("count")]); - if (lang) { - query = query.where("hive_book.language", "=", lang); - } - return query - .groupBy("hive_book_genre.genre") - .orderBy(sql`COUNT(*)`, "desc") - .limit(6) - .execute(); - }, - [], - { ttl: CACHE_TTL }, - ).then((r) => { + getTopGenres(db, kv, TOP_GENRE_COUNT, lang).then((r) => { endTime(c, "explore-genres"); return r; }), - readThroughCache( - kv as Storage, - `authors:featured:${langCacheKey}`, - () => getTopAuthors(db, 8, lang), - [], - { ttl: CACHE_TTL }, - ).then((r) => { + getFeaturedAuthors(db, kv, TOP_AUTHOR_COUNT, lang).then((r) => { endTime(c, "explore-authors"); return r; }), diff --git a/src/pages/genres.tsx b/src/pages/genres.tsx index 2bf7694..c1ec301 100644 --- a/src/pages/genres.tsx +++ b/src/pages/genres.tsx @@ -45,7 +45,10 @@ export const GenresDirectory: FC = async () => { .orderBy(sql`COUNT(*)`, "desc") .execute(), [], - { ttl: 3_600_000 }, + // SWR rather than a plain TTL: this is index-only but still a full scan of + // hive_book_genre, and a plain TTL makes every expiry a synchronous cliff + // for whichever request lands on it (bun:sqlite blocks the event loop). + { ttl: 86_400_000, revalidateAfter: 3_600_000 }, ); endTime(c, "genres-query"); diff --git a/src/routes/pages.tsx b/src/routes/pages.tsx index 0b516d4..db9fb0a 100644 --- a/src/routes/pages.tsx +++ b/src/routes/pages.tsx @@ -28,7 +28,7 @@ import { AuthorBooks, getBooksByAuthor } from "../pages/authorBooks"; import { SearchResults } from "../pages/searchResults"; import { searchBooks, cacheControl } from "./lib"; import { NO_STORE } from "../utils/cacheHeaders"; -import { getAvailableLanguages } from "../utils/getLanguages"; +import { getAvailableLanguages, resolveLanguage } from "../utils/getLanguages"; const app = new Hono() .get("/home", async (c) => { @@ -248,8 +248,12 @@ const app = new Hono() .use("/authors/*", cacheControl("public, max-age=3600, stale-while-revalidate=600")) .get("/explore", async (c) => { const { db, kv } = c.get("ctx"); - const lang = c.req.query("lang") || undefined; - const languages = await getAvailableLanguages(db, kv); + // Validated, not passed through: `lang` keys a cached 356k-row aggregate, + // so an arbitrary string is an unbounded KV-cardinality and CPU amplifier. + const [lang, languages] = await Promise.all([ + resolveLanguage(db, kv, c.req.query("lang")), + getAvailableLanguages(db, kv), + ]); return c.render(, { title: "BookHive | Explore", description: "Discover books by genre or author on BookHive", @@ -297,12 +301,21 @@ const app = new Hono() }, ); }) - .get("/explore/authors", (c) => - c.render(, { + .get("/explore/authors", async (c) => { + const { db, kv } = c.get("ctx"); + // This route used to ignore `lang` entirely while /explore linked here with + // it and the anon page cache keyed on it — every language a crawler found + // became a separate cache entry holding byte-identical HTML, each paying + // its own cold render. + const [lang, languages] = await Promise.all([ + resolveLanguage(db, kv, c.req.query("lang")), + getAvailableLanguages(db, kv), + ]); + return c.render(, { title: "BookHive | Explore Authors", description: "Explore books by author on BookHive", - }), - ) + }); + }) // Legacy redirects .get("/genres", (c) => c.redirect("/explore/genres", 301)) .get("/genres/:genre", (c) => diff --git a/src/utils/authorStats.test.ts b/src/utils/authorStats.test.ts new file mode 100644 index 0000000..f42ec7c --- /dev/null +++ b/src/utils/authorStats.test.ts @@ -0,0 +1,297 @@ +/** + * Author/genre aggregates for the /explore family. + * + * The important tests here are the query-plan ones. `/explore/authors` took + * 9-14.5s in production because these aggregates fetched the whole `hive_book` + * row for every one of 356k books, and the covering index that fixes it is + * *only* used because the queries name it with `INDEXED BY` — this database has + * no `sqlite_stat1`, and without stats the planner prefers the UNIQUE + * `sqlite_autoindex_hive_book_1` and goes straight back to the table. Nothing + * about that failure mode is visible in the results, so it needs a test. + */ +import { describe, it, expect, beforeEach, afterEach } from "bun:test"; +import { Database as DatabaseSync } from "bun:sqlite"; +import { Kysely, SqliteDialect } from "kysely"; +import { createStorage } from "unstorage"; +import memoryDriver from "unstorage/drivers/memory"; + +import { wrapBunSqliteForKysely } from "../bun-sqlite-kysely"; +import { migrateToLatest, type Database, type DatabaseSchema } from "../db"; +import { AUTHOR_DIRECTORY_LIMIT, getAuthorStats, getFeaturedAuthors } from "./authorStats"; +import { getTopGenres } from "./exploreGenres"; + +let db: Database; +let sqlite: DatabaseSync; + +const newKv = () => createStorage({ driver: memoryDriver() }); + +beforeEach(async () => { + sqlite = new DatabaseSync(":memory:"); + sqlite.exec("PRAGMA journal_mode = WAL"); + db = new Kysely({ + dialect: new SqliteDialect({ database: wrapBunSqliteForKysely(sqlite) }), + }); + await migrateToLatest(db, sqlite); +}); + +afterEach(async () => { + await db.destroy(); +}); + +function insertBook(opts: { + id: string; + authors: string; + ratingsCount?: number; + rating?: number; + thumbnail?: string; + language?: string; +}) { + sqlite.exec( + `INSERT INTO hive_book (id, title, rawTitle, authors, source, thumbnail, ratingsCount, rating, language, createdAt, updatedAt) + VALUES (?1, ?2, ?2, ?3, 'goodreads', ?4, ?5, ?6, ?7, ?8, ?8)`, + [ + opts.id, + `Title ${opts.id}`, + opts.authors, + opts.thumbnail ?? "", + opts.ratingsCount ?? 0, + opts.rating ?? null, + opts.language ?? null, + new Date().toISOString(), + ], + ); +} + +const indexNames = () => + ( + sqlite + .query("SELECT name FROM sqlite_master WHERE type = 'index' AND name LIKE 'idx_%'") + .all() as Array<{ name: string }> + ).map((r) => r.name); + +const plan = (query: string, params: unknown[] = []) => + ( + sqlite.query(`EXPLAIN QUERY PLAN ${query}`).all(...(params as never[])) as Array<{ + detail: string; + }> + ) + .map((r) => r.detail) + .join("\n"); + +describe("migration 024", () => { + it("creates the covering indexes and retires the prefix index", () => { + const names = indexNames(); + expect(names).toContain("idx_hive_book_stats"); + expect(names).toContain("idx_hive_book_author_first_cover"); + // Strict prefix of idx_hive_book_author_first_cover; it has no other reader. + expect(names).not.toContain("idx_hive_book_author_first"); + }); + + it("keeps the index every other hive_book_author consumer uses", () => { + // /authors/:author, bookInfo's "more by this author" and the OG renderer + // all filter on `author` alone. + expect(indexNames()).toContain("idx_hive_book_author_author"); + }); +}); + +describe("author aggregate query plans", () => { + // Guards the actual production regression: without `INDEXED BY` these say + // `SEARCH b USING INDEX sqlite_autoindex_hive_book_1`, which means a fetch of + // the whole row from a 1.62 GB table for every book in the catalogue. + const AGGREGATE = ` + SELECT a.author, SUM(COALESCE(b.ratingsCount, 0)) as totalRatings, COUNT(*) as bookCount + FROM hive_book_author a + JOIN hive_book b INDEXED BY idx_hive_book_stats ON b.id = a.hiveId + WHERE a.position = 0 + GROUP BY a.author`; + + it("is index-only on both sides", () => { + const detail = plan(AGGREGATE); + expect(detail).toContain("COVERING INDEX idx_hive_book_author_first_cover"); + expect(detail).toContain("COVERING INDEX idx_hive_book_stats"); + expect(detail).not.toContain("sqlite_autoindex_hive_book_1"); + }); + + it("stays index-only when filtered by language", () => { + const detail = plan( + `${AGGREGATE.replace("WHERE a.position = 0", "WHERE a.position = 0 AND b.language = 'en'")}`, + ); + expect(detail).toContain("COVERING INDEX idx_hive_book_stats"); + expect(detail).not.toContain("sqlite_autoindex_hive_book_1"); + }); + + it("avoids the hive_book join entirely for the language-less genre list", () => { + // /explore used to innerJoin hive_book unconditionally, paying a probe per + // hive_book_genre row for a join that cannot change the result. + const detail = plan( + "SELECT genre, COUNT(*) as count FROM hive_book_genre GROUP BY genre ORDER BY COUNT(*) DESC LIMIT 6", + ); + expect(detail).toContain("COVERING INDEX idx_hive_book_genre_genre"); + expect(detail).not.toContain("hive_book "); + }); +}); + +describe("getAuthorStats", () => { + beforeEach(() => { + // Two books each so they clear `HAVING bookCount >= 2`. + insertBook({ id: "bk_a1", authors: "Alpha Author", ratingsCount: 500, rating: 4200 }); + insertBook({ id: "bk_a2", authors: "Alpha Author", ratingsCount: 500, rating: 4400 }); + insertBook({ id: "bk_b1", authors: "Beta Author", ratingsCount: 100 }); + insertBook({ id: "bk_b2", authors: "Beta Author", ratingsCount: 100 }); + }); + + it("ranks by summed ratings and aggregates per first author", async () => { + const stats = await getAuthorStats(db, newKv()); + expect(stats.map((s) => s.author)).toEqual(["Alpha Author", "Beta Author"]); + expect(stats[0]).toMatchObject({ totalRatings: 1000, bookCount: 2, avgRating: 4.3 }); + }); + + it("excludes authors with a single book or no ratings", async () => { + insertBook({ id: "bk_solo", authors: "Solo Author", ratingsCount: 9999 }); + insertBook({ id: "bk_z1", authors: "Zero Author", ratingsCount: 0 }); + insertBook({ id: "bk_z2", authors: "Zero Author", ratingsCount: 0 }); + + const authors = (await getAuthorStats(db, newKv())).map((s) => s.author); + expect(authors).not.toContain("Solo Author"); + expect(authors).not.toContain("Zero Author"); + }); + + it("only counts the credited first author", async () => { + insertBook({ id: "bk_c1", authors: "Lead\tSidekick", ratingsCount: 300 }); + insertBook({ id: "bk_c2", authors: "Lead\tSidekick", ratingsCount: 300 }); + + const authors = (await getAuthorStats(db, newKv())).map((s) => s.author); + expect(authors).toContain("Lead"); + expect(authors).not.toContain("Sidekick"); + }); + + it("filters by language when one is given", async () => { + insertBook({ id: "bk_f1", authors: "French Author", ratingsCount: 700, language: "French" }); + insertBook({ id: "bk_f2", authors: "French Author", ratingsCount: 700, language: "French" }); + + const french = await getAuthorStats(db, newKv(), "French"); + expect(french.map((s) => s.author)).toEqual(["French Author"]); + // Alpha/Beta have no language, so they must not leak into the filtered list. + expect(french).toHaveLength(1); + }); + + it("breaks ties deterministically by author name", async () => { + // Without the tiebreak, SQLite may order a tied group differently under two + // different LIMITs — which is what makes the featured slice below valid. + insertBook({ id: "bk_t1", authors: "Tie Zulu", ratingsCount: 50 }); + insertBook({ id: "bk_t2", authors: "Tie Zulu", ratingsCount: 50 }); + insertBook({ id: "bk_t3", authors: "Tie Alpha", ratingsCount: 50 }); + insertBook({ id: "bk_t4", authors: "Tie Alpha", ratingsCount: 50 }); + + const tied = (await getAuthorStats(db, newKv())).filter((s) => s.author.startsWith("Tie ")); + expect(tied.map((s) => s.author)).toEqual(["Tie Alpha", "Tie Zulu"]); + }); + + it("caps the directory list", async () => { + expect(AUTHOR_DIRECTORY_LIMIT).toBe(500); + }); + + it("serves the second call from cache", async () => { + const kv = newKv(); + await getAuthorStats(db, kv); + // Rows added after the fill must not appear until the entry is refreshed. + insertBook({ id: "bk_n1", authors: "New Author", ratingsCount: 99999 }); + insertBook({ id: "bk_n2", authors: "New Author", ratingsCount: 99999 }); + const second = await getAuthorStats(db, kv); + expect(second.map((s) => s.author)).not.toContain("New Author"); + }); +}); + +describe("getFeaturedAuthors", () => { + beforeEach(() => { + for (let i = 0; i < 12; i++) { + const ratings = (12 - i) * 100; + insertBook({ + id: `bk_x${i}a`, + authors: `Author ${String(i).padStart(2, "0")}`, + ratingsCount: ratings, + thumbnail: `https://covers.example/${i}-low.jpg`, + }); + insertBook({ + id: `bk_x${i}b`, + authors: `Author ${String(i).padStart(2, "0")}`, + ratingsCount: ratings + 1, + thumbnail: `https://covers.example/${i}-high.jpg`, + }); + } + }); + + it("is a strict prefix of getAuthorStats", async () => { + // The whole point of the refactor: /explore/authors ran the same 356k-row + // GROUP BY twice per render, once at LIMIT 8 and once at LIMIT 500. + const kv = newKv(); + const [featured, all] = await Promise.all([ + getFeaturedAuthors(db, kv, 8), + getAuthorStats(db, kv), + ]); + expect(featured).toHaveLength(8); + expect(featured.map(({ thumbnail: _t, ...rest }) => rest)).toEqual(all.slice(0, 8)); + }); + + it("picks each author's most-rated cover", async () => { + const featured = await getFeaturedAuthors(db, newKv(), 1); + expect(featured[0]?.thumbnail).toBe("https://covers.example/0-high.jpg"); + }); + + it("falls back to null when the candidates have no cover", async () => { + insertBook({ id: "bk_nc1", authors: "Coverless", ratingsCount: 999999, thumbnail: "" }); + insertBook({ id: "bk_nc2", authors: "Coverless", ratingsCount: 999999, thumbnail: "" }); + + const featured = await getFeaturedAuthors(db, newKv(), 1); + expect(featured[0]?.author).toBe("Coverless"); + expect(featured[0]?.thumbnail).toBeNull(); + }); + + it("handles author names containing quotes", async () => { + // The IN list is bound, not interpolated — an apostrophe here would be a + // syntax error if that ever regressed. + insertBook({ id: "bk_q1", authors: "Patrick O'Brian", ratingsCount: 999999 }); + insertBook({ id: "bk_q2", authors: "Patrick O'Brian", ratingsCount: 999999 }); + + const featured = await getFeaturedAuthors(db, newKv(), 1); + expect(featured[0]?.author).toBe("Patrick O'Brian"); + }); + + it("returns an empty list when there are no qualifying authors", async () => { + sqlite.exec("DELETE FROM hive_book"); + expect(await getFeaturedAuthors(db, newKv(), 8)).toEqual([]); + }); +}); + +describe("getTopGenres", () => { + beforeEach(() => { + insertBook({ id: "bk_g1", authors: "A", language: "English" }); + insertBook({ id: "bk_g2", authors: "B", language: "English" }); + insertBook({ id: "bk_g3", authors: "C", language: "French" }); + sqlite.exec( + `INSERT INTO hive_book_genre (hiveId, genre) VALUES + ('bk_g1', 'Fantasy'), ('bk_g2', 'Fantasy'), ('bk_g3', 'Fantasy'), ('bk_g3', 'Poetry')`, + ); + }); + + it("counts every book when no language is given", async () => { + const genres = await getTopGenres(db, newKv(), 6); + expect(genres).toEqual([ + { genre: "Fantasy", count: 3 }, + { genre: "Poetry", count: 1 }, + ]); + }); + + it("counts only the selected language when one is given", async () => { + expect(await getTopGenres(db, newKv(), 6, "English")).toEqual([{ genre: "Fantasy", count: 2 }]); + }); + + it("keys the cache by language", async () => { + const kv = newKv(); + await getTopGenres(db, kv, 6); + expect(await getTopGenres(db, kv, 6, "French")).toEqual([ + { genre: "Fantasy", count: 1 }, + { genre: "Poetry", count: 1 }, + ]); + }); +}); diff --git a/src/utils/authorStats.ts b/src/utils/authorStats.ts new file mode 100644 index 0000000..faba348 --- /dev/null +++ b/src/utils/authorStats.ts @@ -0,0 +1,188 @@ +/** + * Author aggregates for `/explore`, `/explore/authors` and XRPC `getExplore`. + * + * These used to live in `src/pages/authorDirectory.tsx`, which meant the XRPC + * router imported a `.tsx` page module to reach them and each of the three + * consumers wrapped them in its own cache with its own TTL (5 minutes on the + * directory, 1 hour on /explore, none at all on XRPC). Caching now lives + * *inside* these functions so the three cannot drift again. + * + * Two things here are load-bearing and easy to undo by accident: + * + * - **`INDEXED BY idx_hive_book_stats`** (migration 024). Without the hint the + * planner picks the UNIQUE `sqlite_autoindex_hive_book_1` for `b.id = ?` and + * fetches the whole `hive_book` row for `ratingsCount`/`rating`/`language` — + * 356k random reads into a 1.62 GB file, which is the 9-14.5s this replaced. + * The database has no `sqlite_stat1`, so the planner will not find the + * covering index on its own. See the migration 024 docstring. + * - **`ORDER BY totalRatings DESC, author ASC`**. The tiebreak is what makes + * `getFeaturedAuthors` a provable prefix of `getAuthorStats` — without it + * SQLite may order a tied group differently under two different LIMITs — and + * it makes the rendered HTML byte-stable across the three worker processes, + * which both the ETag and the anon page cache want. + */ +import { sql } from "kysely"; +import type { Storage } from "unstorage"; + +import type { Database } from "../db"; +import { readThroughCache } from "./readThroughCache"; + +export interface AuthorStats { + author: string; + totalRatings: number; + avgRating: number | null; + bookCount: number; +} + +export interface AuthorWithStats extends AuthorStats { + thumbnail: string | null; +} + +/** Rows in the `/explore/authors` directory list. */ +export const AUTHOR_DIRECTORY_LIMIT = 500; + +/** Books considered per author when resolving a featured cover. */ +const THUMBNAIL_CANDIDATES_PER_AUTHOR = 5; + +/** + * 24h TTL with revalidation at 1h: after the first fill no request ever blocks + * on the aggregate again, it is refreshed in the background. Plain TTLs made + * every expiry a synchronous cliff on whichever worker drew the short straw. + */ +const CACHE_OPTS = { ttl: 86_400_000, revalidateAfter: 3_600_000 } as const; + +/** Bump the `v1` when the shape of a cached value changes — nothing evicts + * non-`page:` KV keys, so an old shape would be served for the full TTL. */ +const STATS_KEY = (lang: string) => `authors:stats:v1:${lang}`; +const FEATURED_KEY = (lang: string, limit: number) => `authors:featured:v1:${lang}:${limit}`; + +/** + * Top authors by summed ratings, one row per credited first author. + * + * Groups the normalized `hive_book_author` table (migration 020) rather than + * re-deriving the first author with instr/substr/trim over `hive_book.authors`. + * `position = 0` is the credited first author. + */ +async function queryAuthorStats(db: Database, language?: string): Promise { + const langCondition = language ? sql`AND b.language = ${language}` : sql``; + const result = await sql` + SELECT + a.author as author, + SUM(COALESCE(b.ratingsCount, 0)) as totalRatings, + ROUND(AVG(CASE WHEN b.rating IS NOT NULL AND b.rating > 0 THEN b.rating END) / 1000.0, 1) as avgRating, + COUNT(*) as bookCount + FROM hive_book_author a + JOIN hive_book b INDEXED BY idx_hive_book_stats ON b.id = a.hiveId + WHERE a.position = 0 ${langCondition} + GROUP BY a.author + HAVING bookCount >= 2 AND totalRatings > 0 + ORDER BY totalRatings DESC, a.author ASC + LIMIT ${AUTHOR_DIRECTORY_LIMIT} + `.execute(db); + return result.rows; +} + +/** + * Resolve one cover per author. + * + * This replaced a single `ORDER BY b.ratingsCount DESC LIMIT limit * 150` scan + * that read the globally most-rated 1200 books and deduped in JS. That query + * never used the partial index it was written for — it planned as a full 356k + * scan plus a temp B-tree sort *before* the LIMIT — and it also produced the + * wrong answer for an author who ranks highly on summed ratings across many + * mid-tier books, because none of their books reach the global top 1200 and + * the card fell back to a letter tile. + * + * The CTE is index-only on both sides; only the surviving handful of ids are + * fetched from the table for their `thumbnail` (which is deliberately not in + * `idx_hive_book_stats`). `thumbnail` is `NOT NULL` (migration 001) so only + * the empty-string case needs filtering, and that is done in JS across the + * ranked candidates. + */ +async function queryThumbnails( + db: Database, + authors: string[], + language?: string, +): Promise> { + const byAuthor = new Map(); + if (authors.length === 0) return byAuthor; + + const langCondition = language ? sql`AND b.language = ${language}` : sql``; + const authorList = sql.join( + authors.map((a) => sql`${a}`), + sql`, `, + ); + + const result = await sql<{ author: string; thumbnail: string | null }>` + WITH ranked AS ( + SELECT + a.author as author, + a.hiveId as hiveId, + ROW_NUMBER() OVER (PARTITION BY a.author ORDER BY b.ratingsCount DESC) as rn + FROM hive_book_author a + JOIN hive_book b INDEXED BY idx_hive_book_stats ON b.id = a.hiveId + WHERE a.position = 0 AND a.author IN (${authorList}) ${langCondition} + ) + SELECT r.author as author, h.thumbnail as thumbnail + FROM ranked r + JOIN hive_book h ON h.id = r.hiveId + WHERE r.rn <= ${THUMBNAIL_CANDIDATES_PER_AUTHOR} + ORDER BY r.author ASC, r.rn ASC + `.execute(db); + + for (const row of result.rows) { + if (row.thumbnail && !byAuthor.has(row.author)) { + byAuthor.set(row.author, row.thumbnail); + } + } + return byAuthor; +} + +/** + * The full author directory list (top {@link AUTHOR_DIRECTORY_LIMIT} by summed + * ratings), cached with stale-while-revalidate. + */ +export function getAuthorStats( + db: Database, + kv: Storage, + language?: string, +): Promise { + return readThroughCache( + kv as Storage, + STATS_KEY(language || "all"), + () => queryAuthorStats(db, language), + [], + CACHE_OPTS, + ); +} + +/** + * The featured row: the top `limit` of {@link getAuthorStats} plus a cover. + * + * A strict prefix of the directory list rather than its own aggregate — the + * `HAVING` clause and ordering are identical, so the two used to be the same + * 356k-row `GROUP BY` executed twice per render of `/explore/authors`. + */ +export function getFeaturedAuthors( + db: Database, + kv: Storage, + limit: number, + language?: string, +): Promise { + return readThroughCache( + kv as Storage, + FEATURED_KEY(language || "all", limit), + async () => { + const top = (await getAuthorStats(db, kv, language)).slice(0, limit); + if (top.length === 0) return []; + const thumbnails = await queryThumbnails( + db, + top.map((a) => a.author), + language, + ); + return top.map((a) => ({ ...a, thumbnail: thumbnails.get(a.author) ?? null })); + }, + [], + CACHE_OPTS, + ); +} diff --git a/src/utils/exploreGenres.ts b/src/utils/exploreGenres.ts new file mode 100644 index 0000000..cb38cfa --- /dev/null +++ b/src/utils/exploreGenres.ts @@ -0,0 +1,80 @@ +/** + * Genre aggregates for `/explore` and XRPC `getExplore`. + * + * Companion to `src/utils/authorStats.ts` — same reasoning, same cache policy, + * and the same reason for existing: `/explore` cached this with a 1h TTL while + * the XRPC method ran it uncached on every mobile Explore open. + * + * The join is **conditional**. `/explore` used to `innerJoin hive_book` + * unconditionally and only add the `WHERE language = ?` when a language was + * selected, so the language-less case — which is nearly all of the traffic — + * paid a B-tree probe for every one of the ~1-3M `hive_book_genre` rows to + * produce a result the join could not change. Dropping it makes that case a + * single index-only scan of `idx_hive_book_genre_genre` (migration 014). + * + * When a language *is* selected the join is real, and it needs + * `INDEXED BY idx_hive_book_stats` for the same reason the author aggregates + * do: without the hint the planner takes the UNIQUE autoindex and fetches the + * whole row from the 1.62 GB table just to read `language`. See the migration + * 024 docstring. + */ +import { sql } from "kysely"; +import type { Storage } from "unstorage"; + +import type { Database } from "../db"; +import { readThroughCache } from "./readThroughCache"; + +export interface GenreCount { + genre: string; + count: number; +} + +/** Matches `src/utils/authorStats.ts` — SWR so no request blocks on a refresh. */ +const CACHE_OPTS = { ttl: 86_400_000, revalidateAfter: 3_600_000 } as const; + +const GENRES_KEY = (lang: string, limit: number) => `explore:genres:v1:${lang}:${limit}`; + +async function queryTopGenres( + db: Database, + limit: number, + language?: string, +): Promise { + if (!language) { + // Index-only scan of idx_hive_book_genre_genre(genre, hiveId). + const result = await sql` + SELECT genre as genre, COUNT(*) as count + FROM hive_book_genre + GROUP BY genre + ORDER BY COUNT(*) DESC, genre ASC + LIMIT ${limit} + `.execute(db); + return result.rows; + } + + const result = await sql` + SELECT g.genre as genre, COUNT(*) as count + FROM hive_book_genre g + JOIN hive_book b INDEXED BY idx_hive_book_stats ON b.id = g.hiveId + WHERE b.language = ${language} + GROUP BY g.genre + ORDER BY COUNT(*) DESC, g.genre ASC + LIMIT ${limit} + `.execute(db); + return result.rows; +} + +/** Most-populated genres, cached with stale-while-revalidate. */ +export function getTopGenres( + db: Database, + kv: Storage, + limit: number, + language?: string, +): Promise { + return readThroughCache( + kv as Storage, + GENRES_KEY(language || "all", limit), + () => queryTopGenres(db, limit, language), + [], + CACHE_OPTS, + ); +} diff --git a/src/utils/getLanguages.ts b/src/utils/getLanguages.ts index 3d1bdb4..bd9fa86 100644 --- a/src/utils/getLanguages.ts +++ b/src/utils/getLanguages.ts @@ -29,3 +29,23 @@ export async function getAvailableLanguages(db: Database, kv: Storage): Promise< { ttl: LANGUAGES_CACHE_TTL }, ); } + +/** + * Narrow a client-supplied `?lang=` / `language=` to one we actually have books + * in, or `undefined`. + * + * The explore aggregates are cached per language and are expensive to compute, + * so an unvalidated free-form string is both an unbounded KV-key cardinality + * amplifier and an unbounded CPU one — `?lang=` would key its own cache + * entry and run its own 356k-row GROUP BY to produce an empty list. The + * language list this checks against is itself cached for a day. + */ +export async function resolveLanguage( + db: Database, + kv: Storage, + lang: string | undefined | null, +): Promise { + if (!lang) return undefined; + const languages = await getAvailableLanguages(db, kv); + return languages.includes(lang) ? lang : undefined; +} diff --git a/src/utils/readThroughCache.ts b/src/utils/readThroughCache.ts index 9582969..16507ca 100644 --- a/src/utils/readThroughCache.ts +++ b/src/utils/readThroughCache.ts @@ -135,7 +135,12 @@ export async function readThroughCache( return fetch({ key }) .then(async (fresh) => { - await Promise.all([kv.set(key, fresh), kv.setMeta(key, { timestamp: now })]); + // Stamped *after* the fetch, not with the `now` captured before it. + // A 5s fetch used to be born 5s stale, which on a short TTL cost a + // meaningful slice of the entry's life (and on an SWR entry brought + // the next revalidation forward by the whole fetch duration). The + // SWR path below already does this. + await Promise.all([kv.set(key, fresh), kv.setMeta(key, { timestamp: Date.now() })]); return fresh; }) .catch(() => { diff --git a/src/xrpc/router.ts b/src/xrpc/router.ts index b78e291..8cadd71 100644 --- a/src/xrpc/router.ts +++ b/src/xrpc/router.ts @@ -64,7 +64,9 @@ import type { Database } from "../db"; import type { HiveId } from "../types"; import { hydrateUserBook } from "../utils/bookProgress"; import { loadGenresForHiveBook, loadGenresMapForHiveBooks } from "../utils/hiveBookGenres.js"; -import { getTopAuthors } from "../pages/authorDirectory"; +import { getFeaturedAuthors } from "../utils/authorStats"; +import { getTopGenres } from "../utils/exploreGenres"; +import { resolveLanguage } from "../utils/getLanguages"; import { getAvailableLanguages } from "../utils/getLanguages"; import { computeReadingStats, @@ -938,25 +940,21 @@ export function createXrpcRouter`COUNT(*)`.as("count")]); - - if (language) { - genreQuery = genreQuery - .innerJoin("hive_book", "hive_book_genre.hiveId", "hive_book.id") - .where("hive_book.language", "=", language) as any; - } + // Both aggregates are cached with SWR inside their helpers and shared + // with /explore and /explore/authors. This handler used to run them + // uncached on every mobile Explore open — a synchronous multi-second + // query that froze one of the three worker processes outright. + // `language` is lexicon-typed as a free-form string, so it is narrowed to + // one we have books in before it can key a cache entry. + const language = await resolveLanguage( + ctx.db, + ctx.kv, + (_params as BuzzBookhiveGetExplore.$params).language, + ); const [genreRows, topAuthors] = await Promise.all([ - genreQuery - .groupBy("genre") - .orderBy(sql`COUNT(*)`, "desc") - .limit(6) - .execute(), - getTopAuthors(ctx.db, 8, language), + getTopGenres(ctx.db, ctx.kv, 6, language), + getFeaturedAuthors(ctx.db, ctx.kv, 8, language), ]); return json({ From 8b6bca41179ca4fe177966427dbb3f72a3ddbe3f Mon Sep 17 00:00:00 2001 From: Nick the Sick Date: Sun, 16 Aug 2026 19:06:47 +0200 Subject: [PATCH 2/4] fix: add 10s timeout to public Bluesky API calls so pages degrade gracefully during outages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public fetch handler in getProfile/getProfiles had no timeout, so when public.api.bsky.app was unreachable pages hung for ~50s (the OS TCP timeout). This affected /pds, /books/:id, /profile/:handle, and the marketing page — any route that resolves uncached Bluesky profiles. Three changes: - Add AbortSignal.timeout(10_000) to the publicHandler fetch - Wrap getProfiles' fetch in try/catch so a timeout returns cached profiles instead of crashing the render (getProfile already had this) - Add publicOnly option to skip getSessionAgent() on definitively anonymous pages (/pds, marketing landing) Co-Authored-By: Claude Opus 4.6 (1M context) --- src/routes/main.tsx | 4 +-- src/utils/getProfile.ts | 65 ++++++++++++++++++++++++----------------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/src/routes/main.tsx b/src/routes/main.tsx index 89c6223..f2a674e 100644 --- a/src/routes/main.tsx +++ b/src/routes/main.tsx @@ -122,7 +122,7 @@ export function mainRouter(deps: AppDeps): HonoServer { const db = c.get("ctx").db; startTime(c, "pds_profiles+book_counts"); const [profiles, bookCountRows] = await Promise.all([ - dids.length > 0 ? getProfiles({ ctx: c.get("ctx"), dids }) : [], + dids.length > 0 ? getProfiles({ ctx: c.get("ctx"), dids, publicOnly: true }) : [], db .selectFrom("user_book") .select((eb) => ["userDid", eb.fn.countAll().as("count")]) @@ -236,7 +236,7 @@ export function mainRouter(deps: AppDeps): HonoServer { allDids.length > 0 ? ctx.resolver.resolveDidsToHandles(allDids) : ({} as Record), - allDids.length > 0 ? getProfiles({ ctx, dids: allDids }) : [], + allDids.length > 0 ? getProfiles({ ctx, dids: allDids, publicOnly: true }) : [], ]); endTime(c, "marketing_handles"); endTime(c, "marketing_profiles"); diff --git a/src/utils/getProfile.ts b/src/utils/getProfile.ts index 125843a..db0268c 100644 --- a/src/utils/getProfile.ts +++ b/src/utils/getProfile.ts @@ -5,20 +5,25 @@ import type { ProfileViewDetailed } from "../types"; import { readThroughCache } from "./readThroughCache"; import type { AppContext } from "../context"; -/** Public fetch handler for unauthenticated XRPC (e.g. appview). */ +/** Public fetch handler for unauthenticated XRPC (e.g. appview). 10s timeout. */ const publicHandler = { handle: (path: string, init?: RequestInit) => - fetch(new URL(path, "https://public.api.bsky.app").toString(), init), + fetch(new URL(path, "https://public.api.bsky.app").toString(), { + ...init, + signal: AbortSignal.timeout(10_000), + }), }; export async function getProfile({ ctx, did, + publicOnly, }: { ctx: AppContext; did: string; + publicOnly?: boolean; }): Promise { - const sessionClient = await ctx.getSessionAgent(); + const sessionClient = publicOnly ? null : await ctx.getSessionAgent(); const client = sessionClient ? sessionClient : new Client({ handler: publicHandler }); const profile = await readThroughCache( ctx.kv, @@ -52,15 +57,17 @@ export async function getProfile({ export async function getProfiles({ ctx, dids, + publicOnly, }: { ctx: AppContext; dids: string[]; + publicOnly?: boolean; }): Promise { dids = Array.from(new Set(dids)); const profiles = await ctx.kv.getItems( dids.map((did) => "profile:" + did), ); - const sessionClient = await ctx.getSessionAgent(); + const sessionClient = publicOnly ? null : await ctx.getSessionAgent(); const client = sessionClient ? sessionClient : new Client({ handler: publicHandler }); const missingProfiles = profiles @@ -68,31 +75,35 @@ export async function getProfiles({ .map((p) => p.key.slice("profile:".length)); if (missingProfiles.length > 0) { - const actorsParam = missingProfiles as ActorIdentifier[]; - const res = sessionClient - ? await sessionClient.get("app.bsky.actor.getProfiles", { - params: { actors: actorsParam }, - headers: { "atproto-proxy": "did:web:api.bsky.app#bsky_appview" }, - }) - : await client.get("app.bsky.actor.getProfiles", { - params: { actors: actorsParam }, - }); - const fetchedProfiles = res.ok - ? (res.data as { profiles: ProfileViewDetailed[] }).profiles - : []; + try { + const actorsParam = missingProfiles as ActorIdentifier[]; + const res = sessionClient + ? await sessionClient.get("app.bsky.actor.getProfiles", { + params: { actors: actorsParam }, + headers: { "atproto-proxy": "did:web:api.bsky.app#bsky_appview" }, + }) + : await client.get("app.bsky.actor.getProfiles", { + params: { actors: actorsParam }, + }); + const fetchedProfiles = res.ok + ? (res.data as { profiles: ProfileViewDetailed[] }).profiles + : []; - profiles.forEach((p) => { - if (p.value === null) { - p.value = fetchedProfiles.find((f) => f.did === p.key.slice("profile:".length)) || null; - } - }); + profiles.forEach((p) => { + if (p.value === null) { + p.value = fetchedProfiles.find((f) => f.did === p.key.slice("profile:".length)) || null; + } + }); - void ctx.kv.setItems(fetchedProfiles.map((p) => ({ key: "profile:" + p.did, value: p }))); - await Promise.all( - fetchedProfiles - .filter((p) => p.did && p.handle) - .map((p) => setIdentityCache(ctx.kv, p.did!, p.handle!)), - ); + void ctx.kv.setItems(fetchedProfiles.map((p) => ({ key: "profile:" + p.did, value: p }))); + await Promise.all( + fetchedProfiles + .filter((p) => p.did && p.handle) + .map((p) => setIdentityCache(ctx.kv, p.did!, p.handle!)), + ); + } catch { + // Timeout or network failure — return whatever we had cached. + } } return profiles From 03d796945e66f56492ddaa52fa5ce17e8357631b Mon Sep 17 00:00:00 2001 From: Nick the Sick Date: Mon, 17 Aug 2026 10:24:21 +0200 Subject: [PATCH 3/4] fix: isolate public and authenticated profile cache namespaces Public requests used the same `profile:${did}` cache key as authenticated ones, so a ProfileViewDetailed with `.viewer` data could be served to unauthenticated callers. Use `profile:pub:${did}` for public requests and keep `profile:${did}` for authenticated ones. Also attach a rejection handler to the fire-and-forget `setItems` call to prevent unhandled promise rejections. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/utils/getProfile.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/utils/getProfile.ts b/src/utils/getProfile.ts index db0268c..95f6b70 100644 --- a/src/utils/getProfile.ts +++ b/src/utils/getProfile.ts @@ -25,9 +25,10 @@ export async function getProfile({ }): Promise { const sessionClient = publicOnly ? null : await ctx.getSessionAgent(); const client = sessionClient ? sessionClient : new Client({ handler: publicHandler }); + const cacheKey = sessionClient ? "profile:" + did : "profile:pub:" + did; const profile = await readThroughCache( ctx.kv, - "profile:" + did, + cacheKey, async () => { try { const actorParam = did as ActorIdentifier; @@ -64,15 +65,16 @@ export async function getProfiles({ publicOnly?: boolean; }): Promise { dids = Array.from(new Set(dids)); - const profiles = await ctx.kv.getItems( - dids.map((did) => "profile:" + did), - ); const sessionClient = publicOnly ? null : await ctx.getSessionAgent(); const client = sessionClient ? sessionClient : new Client({ handler: publicHandler }); + const keyPrefix = sessionClient ? "profile:" : "profile:pub:"; + const profiles = await ctx.kv.getItems( + dids.map((did) => keyPrefix + did), + ); const missingProfiles = profiles .filter((p) => p.value === null) - .map((p) => p.key.slice("profile:".length)); + .map((p) => p.key.slice(keyPrefix.length)); if (missingProfiles.length > 0) { try { @@ -91,11 +93,13 @@ export async function getProfiles({ profiles.forEach((p) => { if (p.value === null) { - p.value = fetchedProfiles.find((f) => f.did === p.key.slice("profile:".length)) || null; + p.value = fetchedProfiles.find((f) => f.did === p.key.slice(keyPrefix.length)) || null; } }); - void ctx.kv.setItems(fetchedProfiles.map((p) => ({ key: "profile:" + p.did, value: p }))); + ctx.kv + .setItems(fetchedProfiles.map((p) => ({ key: keyPrefix + p.did, value: p }))) + .catch(() => {}); await Promise.all( fetchedProfiles .filter((p) => p.did && p.handle) From 91ae31d997cf20b0f6ca19edb4400110477a8733 Mon Sep 17 00:00:00 2001 From: Nick the Sick Date: Mon, 17 Aug 2026 19:03:01 +0200 Subject: [PATCH 4/4] fix: scope profile cache by viewer DID and add TTL to batch reads Authenticated profile cache entries now key on both the viewer and target DIDs so viewer-specific .viewer data cannot leak between users. getProfiles reads per-entry metadata to enforce the same 24h revalidate / 30d TTL that readThroughCache applies in getProfile, and writes timestamp metadata so entries expire consistently across both paths. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/utils/getProfile.ts | 63 ++++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/src/utils/getProfile.ts b/src/utils/getProfile.ts index 95f6b70..2797c7e 100644 --- a/src/utils/getProfile.ts +++ b/src/utils/getProfile.ts @@ -14,6 +14,13 @@ const publicHandler = { }), }; +const REVALIDATE_AFTER = 24 * 60 * 60 * 1000; +const PROFILE_TTL = 30 * 24 * 60 * 60 * 1000; + +function profileCacheKey(targetDid: string, viewerDid: string | null): string { + return viewerDid ? "profile:" + viewerDid + ":" + targetDid : "profile:pub:" + targetDid; +} + export async function getProfile({ ctx, did, @@ -25,7 +32,7 @@ export async function getProfile({ }): Promise { const sessionClient = publicOnly ? null : await ctx.getSessionAgent(); const client = sessionClient ? sessionClient : new Client({ handler: publicHandler }); - const cacheKey = sessionClient ? "profile:" + did : "profile:pub:" + did; + const cacheKey = profileCacheKey(did, sessionClient?.did ?? null); const profile = await readThroughCache( ctx.kv, cacheKey, @@ -50,7 +57,7 @@ export async function getProfile({ } }, undefined, - { revalidateAfter: 24 * 60 * 60 * 1000, ttl: 30 * 24 * 60 * 60 * 1000 }, + { revalidateAfter: REVALIDATE_AFTER, ttl: PROFILE_TTL }, ); return profile; } @@ -67,18 +74,30 @@ export async function getProfiles({ dids = Array.from(new Set(dids)); const sessionClient = publicOnly ? null : await ctx.getSessionAgent(); const client = sessionClient ? sessionClient : new Client({ handler: publicHandler }); - const keyPrefix = sessionClient ? "profile:" : "profile:pub:"; - const profiles = await ctx.kv.getItems( - dids.map((did) => keyPrefix + did), + const viewerDid = sessionClient?.did ?? null; + + const now = Date.now(); + const entries = await Promise.all( + dids.map(async (did) => { + const key = profileCacheKey(did, viewerDid); + const [value, meta] = await Promise.all([ + ctx.kv.get(key), + ctx.kv.getMeta(key), + ]); + const timestamp = meta && typeof meta["timestamp"] === "number" ? meta["timestamp"] : null; + const age = timestamp !== null ? now - timestamp : Infinity; + const isFresh = value !== null && timestamp !== null && age < REVALIDATE_AFTER; + const isStale = + value !== null && timestamp !== null && age >= REVALIDATE_AFTER && age < PROFILE_TTL; + return { did, key, value, isFresh, isStale }; + }), ); - const missingProfiles = profiles - .filter((p) => p.value === null) - .map((p) => p.key.slice(keyPrefix.length)); + const fetchDids = entries.filter((e) => !e.isFresh).map((e) => e.did); - if (missingProfiles.length > 0) { + if (fetchDids.length > 0) { try { - const actorsParam = missingProfiles as ActorIdentifier[]; + const actorsParam = fetchDids as ActorIdentifier[]; const res = sessionClient ? await sessionClient.get("app.bsky.actor.getProfiles", { params: { actors: actorsParam }, @@ -91,15 +110,19 @@ export async function getProfiles({ ? (res.data as { profiles: ProfileViewDetailed[] }).profiles : []; - profiles.forEach((p) => { - if (p.value === null) { - p.value = fetchedProfiles.find((f) => f.did === p.key.slice(keyPrefix.length)) || null; + for (const entry of entries) { + if (!entry.isFresh && entry.value === null) { + entry.value = fetchedProfiles.find((f) => f.did === entry.did) ?? null; } - }); + } - ctx.kv - .setItems(fetchedProfiles.map((p) => ({ key: keyPrefix + p.did, value: p }))) - .catch(() => {}); + const writeTimestamp = Date.now(); + Promise.all( + fetchedProfiles.flatMap((p) => { + const key = profileCacheKey(p.did, viewerDid); + return [ctx.kv.set(key, p), ctx.kv.setMeta(key, { timestamp: writeTimestamp })]; + }), + ).catch(() => {}); await Promise.all( fetchedProfiles .filter((p) => p.did && p.handle) @@ -110,7 +133,7 @@ export async function getProfiles({ } } - return profiles - .filter((p): p is { key: string; value: ProfileViewDetailed } => Boolean(p.value)) - .map((p) => p.value); + return entries + .filter((e): e is typeof e & { value: ProfileViewDetailed } => e.value !== null) + .map((e) => e.value); }