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/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/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/getProfile.ts b/src/utils/getProfile.ts index 125843a..2797c7e 100644 --- a/src/utils/getProfile.ts +++ b/src/utils/getProfile.ts @@ -5,24 +5,37 @@ 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), + }), }; +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, + 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 cacheKey = profileCacheKey(did, sessionClient?.did ?? null); const profile = await readThroughCache( ctx.kv, - "profile:" + did, + cacheKey, async () => { try { const actorParam = did as ActorIdentifier; @@ -44,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; } @@ -52,50 +65,75 @@ 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 viewerDid = sessionClient?.did ?? null; - const missingProfiles = profiles - .filter((p) => p.value === null) - .map((p) => p.key.slice("profile:".length)); + 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 fetchDids = entries.filter((e) => !e.isFresh).map((e) => e.did); - 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 - : []; + if (fetchDids.length > 0) { + try { + const actorsParam = fetchDids 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; + for (const entry of entries) { + if (!entry.isFresh && entry.value === null) { + entry.value = fetchedProfiles.find((f) => f.did === entry.did) ?? 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!)), - ); + 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) + .map((p) => setIdentityCache(ctx.kv, p.did!, p.handle!)), + ); + } catch { + // Timeout or network failure — return whatever we had cached. + } } - 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); } 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({