diff --git a/apps/web/app/lib/admin/batch-limit.test.ts b/apps/web/app/lib/admin/batch-limit.test.ts new file mode 100644 index 0000000..a796f47 --- /dev/null +++ b/apps/web/app/lib/admin/batch-limit.test.ts @@ -0,0 +1,51 @@ +/** + * `?batch=` bounds for the admin recompute endpoint. + * + * The bug this pins: `batch=0` used to pass validation, became `LIMIT 0`, and + * the `all=true` loop then re-read the same offset for all 50 permitted + * iterations while processing nothing. `batch=100000` was accepted too, which + * defeats the per-request guard that keeps the Worker under its CPU limit. + */ + +import { describe, expect, it } from "vitest"; +import { parseBatchSize, RECOMPUTE_BATCH_LIMIT } from "./batch-limit"; + +describe("parseBatchSize", () => { + it("falls back when the parameter is absent, blank, or unusable", () => { + for (const raw of [null, undefined, "", " ", "abc", "NaN", "Infinity", "-0"]) { + expect(parseBatchSize(raw)).toBe(RECOMPUTE_BATCH_LIMIT.fallback); + } + }); + + it("rejects zero and negative values instead of letting them through", () => { + // `0` is the reported bug: it is a valid integer but a useless batch size. + expect(parseBatchSize("0")).toBe(RECOMPUTE_BATCH_LIMIT.fallback); + expect(parseBatchSize("0.0")).toBe(RECOMPUTE_BATCH_LIMIT.fallback); + expect(parseBatchSize("-25")).toBe(RECOMPUTE_BATCH_LIMIT.fallback); + }); + + it("keeps in-range values as they are", () => { + expect(parseBatchSize("1")).toBe(1); + expect(parseBatchSize("100")).toBe(100); + expect(parseBatchSize("200")).toBe(200); + expect(parseBatchSize(" 250 ")).toBe(250); + expect(parseBatchSize("500")).toBe(500); + }); + + it("floors fractional values because .limit() needs an integer", () => { + expect(parseBatchSize("12.9")).toBe(12); + expect(parseBatchSize("0.9")).toBe(RECOMPUTE_BATCH_LIMIT.fallback); + }); + + it("clamps anything above the cap instead of accepting it", () => { + expect(parseBatchSize("501")).toBe(RECOMPUTE_BATCH_LIMIT.max); + expect(parseBatchSize("100000")).toBe(RECOMPUTE_BATCH_LIMIT.max); + // `parseInt("1e9")` would be 1; the magnitude is honoured instead. + expect(parseBatchSize("1e9")).toBe(RECOMPUTE_BATCH_LIMIT.max); + }); + + it("honours a caller-supplied bound", () => { + expect(parseBatchSize("1000", { max: 50, fallback: 10 })).toBe(50); + expect(parseBatchSize(null, { max: 50, fallback: 10 })).toBe(10); + }); +}); diff --git a/apps/web/app/lib/admin/batch-limit.ts b/apps/web/app/lib/admin/batch-limit.ts new file mode 100644 index 0000000..32828c4 --- /dev/null +++ b/apps/web/app/lib/admin/batch-limit.ts @@ -0,0 +1,45 @@ +/** + * Bounds for the `batch` query parameter on the admin endpoints. + * + * `batch` reaches `.limit()` on a D1 query, so an unbounded value both wastes a + * Worker CPU budget and lets one request ask for an arbitrarily large result. + * A value of `0` is worse than useless: `LIMIT 0` returns an empty batch, the + * completion test `batch.length < batchSize` is false, and the `all=true` loop + * keeps re-reading the same offset until `MAX_BATCHES_PER_REQUEST` runs out. + * + * Only positive integers are accepted. Anything absent, blank, non-numeric, or + * below 1 yields the fallback; anything above the cap is clamped to the cap. + */ + +export interface BatchLimit { + /** Largest accepted batch size. */ + max: number; + /** Value used when the parameter is absent or unusable. */ + fallback: number; +} + +export const RECOMPUTE_BATCH_LIMIT: BatchLimit = { max: 500, fallback: 100 }; + +/** + * Resolves a raw `batch` value to a usable batch size. + * + * `Number` is used rather than `parseInt` so a value like `"1e9"` is read as a + * magnitude and clamped, instead of being truncated to `1` by `parseInt`. The + * result is floored to an integer because `.limit()` requires one. + */ +export function parseBatchSize( + raw: string | null | undefined, + limits: BatchLimit = RECOMPUTE_BATCH_LIMIT, +): number { + if (raw == null) return limits.fallback; + const trimmed = raw.trim(); + if (trimmed === "") return limits.fallback; + + const value = Number(trimmed); + if (!Number.isFinite(value)) return limits.fallback; + + const floored = Math.floor(value); + if (floored < 1) return limits.fallback; + if (floored > limits.max) return limits.max; + return floored; +} diff --git a/apps/web/app/routes/api.admin.recompute.ts b/apps/web/app/routes/api.admin.recompute.ts index db7d199..64f181d 100644 --- a/apps/web/app/routes/api.admin.recompute.ts +++ b/apps/web/app/routes/api.admin.recompute.ts @@ -16,6 +16,7 @@ import { recomputeBatch, getRecomputeProgress, } from "~/lib/leaderboard/recompute-all-skills"; +import { parseBatchSize } from "~/lib/admin/batch-limit"; /** Max batches per `all=true` request to avoid Worker CPU timeout */ const MAX_BATCHES_PER_REQUEST = 50; @@ -25,6 +26,7 @@ function verifyAdmin(request: Request, env: Record): boolean { return !!secret && secret === env.ADMIN_SECRET; } +/** Non-negative integer, for `offset` only. Batch size is bounded separately. */ function parseIntSafe(value: string | null, fallback: number): number { if (value == null) return fallback; const n = parseInt(value, 10); @@ -55,7 +57,7 @@ export async function action({ request, context }: ActionFunctionArgs) { } const url = new URL(request.url); - const batchSize = parseIntSafe(url.searchParams.get("batch"), 100); + const batchSize = parseBatchSize(url.searchParams.get("batch")); const offsetParam = url.searchParams.get("offset"); const resume = url.searchParams.get("resume") === "true"; const runAll = url.searchParams.get("all") === "true";