fix(admin): clamp the recompute batch size to 1..500

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);
+  });
+});

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;
+}

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<string, string>): 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";