From 5a30ed9ce8452866accf8ec66ad166cc513b0a5e Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sat, 1 Aug 2026 04:22:39 +0000 Subject: [PATCH] pit: page the endings list instead of stopping at 200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/moshpit/tlds` answered with 200 rows and nothing saying it had stopped. `?limit=` and `?offset=` were parsed on the `?q=` branch and ignored on every other one, so asking for page two returned page one — which is indistinguishable from a registry that holds exactly 200 things. The failure that produces is not a missing feature. A client reads "absent from the list" as "nobody holds it", and acts on that. The same list read through `?mine=1` was already complete, so the two disagreed about what exists and neither said which to believe. Every answer now carries `total`, `limit` and `offset`. The default page stays 200 so nothing that reads this today sees different rows, and the reported limit is the one applied rather than the one asked for. `?mine=1` stays unpaged by default — it was the call telling the whole truth, and giving it a default page size would have moved the bug rather than fixed it — but it pages when asked. listTlds gains the `created_at DESC, tld` tiebreak listTldsForUser already documents. A bulk claim writes one timestamp across every ending in it, so created_at is not a total order, and a page boundary landing inside a batch repeats one ending and skips another. Paging the list at all required fixing that first. `?limit=` is capped at 1000. Without a ceiling it is a way to ask for every row in the table, which is what the pager exists to avoid. Co-Authored-By: Claude Opus 5 (1M context) --- apps/pwa/src/moshpit.mjs | 22 ++- apps/pwa/src/routes/moshpit.mjs | 47 ++++- .../pwa/test/moshpit-tlds-pagination.test.mjs | 160 ++++++++++++++++++ 3 files changed, 224 insertions(+), 5 deletions(-) create mode 100644 apps/pwa/test/moshpit-tlds-pagination.test.mjs diff --git a/apps/pwa/src/moshpit.mjs b/apps/pwa/src/moshpit.mjs index 5a728f0..b7d56c5 100644 --- a/apps/pwa/src/moshpit.mjs +++ b/apps/pwa/src/moshpit.mjs @@ -49,8 +49,26 @@ export async function getTld(tld) { return get(`SELECT ${COLS} FROM moshpit_tlds WHERE tld = ?`, [tld]); } -export async function listTlds(limit = 200) { - return all(`SELECT ${COLS} FROM moshpit_tlds ORDER BY created_at DESC LIMIT ?`, [limit]); +/** + * The endings everyone holds, newest first. + * + * Ordered by `created_at DESC, tld` for the same reason `listTldsForUser` is: a + * bulk claim writes one timestamp across every ending in it, so `created_at` + * alone is not a total order. Without the tiebreak a page boundary landing + * inside a batch shows one ending twice and skips another — which is invisible + * until someone pages, and is why this could not simply be paged as it stood. + */ +export async function listTlds({ limit = 200, offset = 0 } = {}) { + return all( + `SELECT ${COLS} FROM moshpit_tlds ORDER BY created_at DESC, tld LIMIT ? OFFSET ?`, + [limit, offset], + ); +} + +/** How many endings exist -- so a caller can see there are more than it got. */ +export async function countTlds() { + const row = await get(`SELECT COUNT(*) AS n FROM moshpit_tlds`); + return Number(row?.n ?? 0); } /** diff --git a/apps/pwa/src/routes/moshpit.mjs b/apps/pwa/src/routes/moshpit.mjs index 5baf93f..ada2e1a 100644 --- a/apps/pwa/src/routes/moshpit.mjs +++ b/apps/pwa/src/routes/moshpit.mjs @@ -28,6 +28,7 @@ import { clearAlias, clearExempt, countNames, + countTlds, countTldsForUser, countSearchTlds, countTldsNotOwnedBy, @@ -139,10 +140,50 @@ moshpitRouter.get("/api/moshpit/tlds", async (req, res) => { }); } - if (mine) return res.json({ tlds: await listTldsForUser(req.user.id) }); - res.json({ tlds: await listTlds() }); + // `total` on every answer, because the alternative is what this used to do: + // hand back 200 rows out of thousands with nothing in the response saying so. + // A client cannot tell a complete list from a truncated one by looking at it, + // and reading "absent from the list" as "does not exist" is the mistake that + // shape invites. + const { limit, offset } = pageParams(req.query); + + if (mine) { + // Unpaged by default, as it has always been: this is the answer to "what do + // I hold", and imposing a page size on it now would truncate the one call + // that was telling the whole truth. + const tlds = await listTldsForUser(req.user.id, limit === null ? {} : { limit, offset }); + return res.json({ total: await countTldsForUser(req.user.id), limit, offset, tlds }); + } + + // The default page size is the 200 this always applied — kept so existing + // callers see no change in what arrives, only in being told there is more. + const applied = limit ?? DEFAULT_PAGE; + const tlds = await listTlds({ limit: applied, offset }); + res.json({ total: await countTlds(), limit: applied, offset, tlds }); }); +/** + * `?limit=` and `?offset=`, or null for "as it comes". + * + * These were read on the `?q=` branch and ignored everywhere else, so paging + * the plain list did nothing at all — every page came back as page one, which + * looks exactly like a list that happens to have 200 things in it. + * + * The ceiling is a real limit rather than a suggestion: without one, `?limit=` + * is a way to ask the database for every row it has, and the pager exists + * precisely so nobody has to. + */ +const MAX_PAGE = 1000; +const DEFAULT_PAGE = 200; + +function pageParams(query) { + const raw = Number.parseInt(query.limit, 10); + const limit = Number.isInteger(raw) && raw > 0 ? Math.min(MAX_PAGE, raw) : null; + const offsetRaw = Number.parseInt(query.offset, 10); + const offset = Number.isInteger(offsetRaw) && offsetRaw > 0 ? offsetRaw : 0; + return { limit, offset }; +} + moshpitRouter.post("/api/moshpit/tlds", async (req, res) => { if (!req.user) return unauthorized(res); const result = await registerTld({ @@ -285,7 +326,7 @@ moshpitRouter.get("/n/:name", async (req, res) => { // No target: the directory. const [names, tlds] = await Promise.all([ tld ? listNames(tld) : Promise.resolve([]), - listTlds(200), + listTlds({ limit: 200 }), ]); const owner = tld ? await getTldWithPrice(tld) : null; diff --git a/apps/pwa/test/moshpit-tlds-pagination.test.mjs b/apps/pwa/test/moshpit-tlds-pagination.test.mjs new file mode 100644 index 0000000..e1066d7 --- /dev/null +++ b/apps/pwa/test/moshpit-tlds-pagination.test.mjs @@ -0,0 +1,160 @@ +// Paging the endings list. +// +// `/api/moshpit/tlds` answered with 200 rows and no indication that it had +// stopped. `?limit=` and `?offset=` were read on the `?q=` branch and ignored +// on every other, so asking for page two returned page one — which looks +// exactly like a registry that happens to hold 200 endings. +// +// The failure that shape produces is not a missing feature. A client reads +// "absent from the list" as "nobody has claimed it", and acts on it. +import assert from "node:assert/strict"; +import fs from "node:fs"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { createRequire } from "node:module"; +import test from "node:test"; + +const require = createRequire(import.meta.url); +let deps = null; +try { + deps = { express: require("express"), cookieParser: require("cookie-parser") }; +} catch { + deps = null; +} + +const workdir = mkdtempSync(path.join(tmpdir(), "moshcode-pit-page-test-")); +process.env.DATABASE_URL = `file:${path.join(workdir, "test.db")}`; +process.env.SESSION_SECRET = "test-secret"; + +// More than the old ceiling, so the truncation this fixes is reachable. +const TOTAL = 260; + +async function boot() { + const { migrate } = await import("../src/migrate.mjs"); + await migrate(); + const { run, db } = await import("../src/db.mjs"); + const { sessionMiddleware, csrfGuard } = await import("../src/lib/session.mjs"); + const { moshpitRouter } = await import("../src/routes/moshpit.mjs"); + const { createApiKey } = await import("../src/lib/apikey.mjs"); + + await run(`INSERT OR REPLACE INTO users (id, email, display_name, created_at) VALUES ('u1','a@b.c','one',1)`); + + // One timestamp across the whole batch, which is what a bulk claim actually + // writes. `created_at` is then not a total order, and a pager that trusts it + // alone repeats one ending and skips another at every page boundary. + for (let i = 0; i < TOTAL; i++) { + await run( + `INSERT INTO moshpit_tlds (tld,user_id,owner_email,created_at) VALUES (?,'u1','a@b.c',1)`, + [`e${String(i).padStart(4, "0")}`], + ); + } + + const key = (await createApiKey("u1", "cli")).plaintext; + + const app = deps.express(); + app.use(deps.express.json()); + app.use(deps.express.urlencoded({ extended: false })); + app.use(deps.cookieParser()); + app.use(sessionMiddleware); + app.use(csrfGuard); + app.use(moshpitRouter); + const server = await new Promise((resolve) => { + const s = app.listen(0, "127.0.0.1", () => resolve(s)); + }); + const base = `http://127.0.0.1:${server.address().port}`; + + const call = async (p) => { + const res = await fetch(`${base}${p}`, { headers: { authorization: `Bearer ${key}` } }); + return { status: res.status, json: await res.json() }; + }; + + return { server, db, call }; +} + +let booted = null; +const app = () => (booted ||= boot()); + +test.after(() => { + if (!booted) return; + booted.then(({ server, db }) => { server.close(); db.close?.(); }) + .finally(() => { try { fs.rmSync(workdir, { recursive: true, force: true }); } catch { /* noop */ } }); +}); + +const skip = { skip: !deps && "apps/pwa deps not installed" }; + +test("the endings list says how many there are", skip, async () => { + const { call } = await app(); + + const res = await call("/api/moshpit/tlds"); + assert.equal(res.status, 200); + // The default page is unchanged. What is new is being told it is a page. + assert.equal(res.json.tlds.length, 200); + assert.equal(res.json.total, TOTAL, "a truncated list has to say so"); + assert.equal(res.json.limit, 200, "the limit reported is the one applied"); + assert.equal(res.json.offset, 0); +}); + +test("limit and offset are obeyed rather than read and dropped", skip, async () => { + const { call } = await app(); + + const first = await call("/api/moshpit/tlds?limit=10"); + assert.deepEqual(first.json.tlds.length, 10); + + const second = await call("/api/moshpit/tlds?limit=10&offset=10"); + assert.equal(second.json.offset, 10); + assert.notDeepEqual( + second.json.tlds.map((t) => t.tld), + first.json.tlds.map((t) => t.tld), + "page two used to be page one", + ); +}); + +test("paging the whole list loses nothing and repeats nothing", skip, async () => { + const { call } = await app(); + + // The tiebreak in the ORDER BY is what this actually tests: every ending + // here shares one created_at, so without it the pages overlap. + const seen = []; + for (let offset = 0; offset < TOTAL; offset += 25) { + const res = await call(`/api/moshpit/tlds?limit=25&offset=${offset}`); + seen.push(...res.json.tlds.map((t) => t.tld)); + } + assert.equal(seen.length, TOTAL); + assert.equal(new Set(seen).size, TOTAL, "a page boundary inside a tie duplicates and skips"); +}); + +test("a limit past the ceiling is capped, not honoured", skip, async () => { + const { call } = await app(); + + // Without a ceiling, `?limit=` is a way to ask for every row in the table, + // which is the thing the pager exists to prevent. + const res = await call("/api/moshpit/tlds?limit=999999"); + assert.equal(res.json.limit, 1000); +}); + +test("nonsense paging falls back rather than erroring or emptying", skip, async () => { + const { call } = await app(); + + for (const q of ["?limit=abc", "?limit=0", "?limit=-5", "?offset=-1", "?limit=&offset="]) { + const res = await call(`/api/moshpit/tlds${q}`); + assert.equal(res.status, 200, q); + assert.equal(res.json.tlds.length, 200, `${q} should fall back to the default page`); + assert.equal(res.json.offset, 0, q); + } +}); + +test("your own endings still come back whole, and now carry a total", skip, async () => { + const { call } = await app(); + + // This is the one call that was already telling the truth. Adding a default + // page size here would have turned the fix into the same bug somewhere else. + const res = await call("/api/moshpit/tlds?mine=1"); + assert.equal(res.json.tlds.length, TOTAL, "unpaged by default, as before"); + assert.equal(res.json.total, TOTAL); + assert.equal(res.json.limit, null); + + const paged = await call("/api/moshpit/tlds?mine=1&limit=30&offset=30"); + assert.equal(paged.json.tlds.length, 30, "and pages when asked to"); + assert.equal(paged.json.total, TOTAL); +});