fix: schedule bounded cleanup of expired rate-limit buckets

CHANGELOG.md

@@ -4,6 +4,7 @@ All notable changes to BetterMan are documented here.
 
 ## Unreleased
 
+- Privacy: delete expired rate-limit buckets every minute, in bounded batches that resume on the next run, so raw client identifiers are not kept indefinitely.
 - Security: validate, rate-limit, and bound dynamic man-page share image inputs, and return 404 without rendering for missing pages.
 - Security: key public API rate limits on the Vercel-overwritten client address instead of spoofable `CF-Connecting-IP` / `X-Forwarded-For` headers, and share an anonymous bucket when no trusted identity is present.
 - Maintenance: add a manual `prune-releases` workflow that previews by default and deletes superseded inactive releases in bounded, retried batches, retaining the newest verified release per distro as a rollback target and sweeping orphaned content blobs; release pruning and the orphan sweep now also delete their stored payload files, and the inactive-release preview paginates and reports rollback eligibility.

convex/_generated/api.d.ts

@@ -15,6 +15,7 @@ import type * as _releaseLookups from "../_releaseLookups.js";
 import type * as _releaseManifest from "../_releaseManifest.js";
 import type * as _storedPayload from "../_storedPayload.js";
 import type * as content from "../content.js";
+import type * as crons from "../crons.js";
 import type * as http from "../http.js";
 import type * as ingest from "../ingest.js";
 import type * as lib from "../lib.js";
@@ -38,6 +39,7 @@ declare const fullApi: ApiFromModules<{
   _releaseManifest: typeof _releaseManifest;
   _storedPayload: typeof _storedPayload;
   content: typeof content;
+  crons: typeof crons;
   http: typeof http;
   ingest: typeof ingest;
   lib: typeof lib;

convex/crons.ts

@@ -0,0 +1,20 @@
+import { cronJobs } from "convex/server";
+import { internal } from "./_generated/api";
+import { CLEANUP_BATCH_LIMIT, CLEANUP_FOLLOWUP_LIMIT } from "./rateLimit";
+
+const crons = cronJobs();
+
+// Bucket keys embed the client identifier. Expired rows are removed on this
+// one-minute schedule, in bounded batches. A backlog larger than one wave
+// continues on the next tick rather than inside a single transaction.
+crons.interval(
+  "delete expired rate limit buckets",
+  { minutes: 1 },
+  internal.rateLimit.cleanupExpired,
+  {
+    maxBuckets: CLEANUP_BATCH_LIMIT,
+    followupsRemaining: CLEANUP_FOLLOWUP_LIMIT,
+  },
+);
+
+export default crons;

convex/rateLimit.test.ts

@@ -0,0 +1,163 @@
+/// <reference types="vite/client" />
+import { convexTest } from "convex-test";
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { internal } from "./_generated/api";
+import rateLimitCrons from "./crons";
+import {
+  CLEANUP_BATCH_DEFAULT,
+  CLEANUP_BATCH_LIMIT,
+  CLEANUP_FOLLOWUP_LIMIT,
+  cleanupExpired,
+} from "./rateLimit";
+import schema from "./schema";
+
+const modules = import.meta.glob("./**/*.ts");
+
+function harness() {
+  return convexTest(schema, modules);
+}
+
+function freezeNow() {
+  const now = Date.now();
+  vi.useFakeTimers({ now });
+  return now;
+}
+
+async function seedBuckets(
+  t: ReturnType<typeof harness>,
+  rows: Array<{ key: string; expiresAt: number }>,
+) {
+  await t.run(async (ctx) => {
+    for (const row of rows) {
+      await ctx.db.insert("rateLimitBuckets", {
+        key: row.key,
+        count: 1,
+        expiresAt: row.expiresAt,
+      });
+    }
+  });
+}
+
+async function remainingKeys(t: ReturnType<typeof harness>) {
+  const rows = await t.run(async (ctx) => ctx.db.query("rateLimitBuckets").take(CLEANUP_BATCH_LIMIT + 5));
+  return rows.map((row) => row.key).sort();
+}
+
+afterEach(() => {
+  vi.useRealTimers();
+});
+
+describe("expired rate-limit cleanup", () => {
+  it("stays internal and is scheduled every minute with a bounded batch", () => {
+    const registered = cleanupExpired as { isInternal?: boolean; isPublic?: boolean };
+    expect(registered.isInternal).toBe(true);
+    expect(registered.isPublic).toBeUndefined();
+
+    expect(Object.keys(rateLimitCrons.crons)).toEqual(["delete expired rate limit buckets"]);
+    expect(rateLimitCrons.crons["delete expired rate limit buckets"]).toEqual({
+      name: "rateLimit:cleanupExpired",
+      args: [{ maxBuckets: CLEANUP_BATCH_LIMIT, followupsRemaining: CLEANUP_FOLLOWUP_LIMIT }],
+      schedule: { type: "interval", minutes: 1 },
+    });
+  });
+
+  it("deletes expired identifier keys and keeps buckets still inside the window", async () => {
+    const now = freezeNow();
+    const t = harness();
+    await seedBuckets(t, [
+      { key: "rl:page:203.0.113.8:1", expiresAt: now - 1_000 },
+      { key: "rl:search:203.0.113.8:1", expiresAt: now - 60_000 },
+      { key: "rl:page:203.0.113.9:2", expiresAt: now + 60_000 },
+    ]);
+
+    expect(await t.mutation(internal.rateLimit.cleanupExpired, { followupsRemaining: 0 })).toEqual({
+      deleted: 2,
+      hasMore: false,
+      scheduledFollowup: false,
+    });
+    expect(await remainingKeys(t)).toEqual(["rl:page:203.0.113.9:2"]);
+  });
+
+  it("does not schedule another batch when the last page is exactly full", async () => {
+    const now = freezeNow();
+    const t = harness();
+    await seedBuckets(t, [
+      { key: "rl:page:203.0.113.1:1", expiresAt: now - 1 },
+      { key: "rl:page:203.0.113.2:1", expiresAt: now - 1 },
+    ]);
+
+    expect(await t.mutation(internal.rateLimit.cleanupExpired, {
+      maxBuckets: 2,
+      followupsRemaining: CLEANUP_FOLLOWUP_LIMIT,
+    })).toEqual({ deleted: 2, hasMore: false, scheduledFollowup: false });
+    expect(await remainingKeys(t)).toEqual([]);
+  });
+
+  it("drains a backlog across scheduled batches and leaves the live bucket", async () => {
+    const now = freezeNow();
+    const t = harness();
+    await seedBuckets(t, [
+      ...[1, 2, 3, 4, 5].map((octet) => ({
+        key: `rl:page:203.0.113.${octet}:1`,
+        expiresAt: now - 5_000,
+      })),
+      { key: "rl:search:203.0.113.9:2", expiresAt: now + 30_000 },
+    ]);
+
+    expect(await t.mutation(internal.rateLimit.cleanupExpired, { maxBuckets: 2 })).toEqual({
+      deleted: 2,
+      hasMore: true,
+      scheduledFollowup: true,
+    });
+    await t.finishAllScheduledFunctions(() => {
+      vi.runAllTimers();
+    });
+    expect(await remainingKeys(t)).toEqual(["rl:search:203.0.113.9:2"]);
+  });
+
+  it("stops chaining when the follow-up budget is exhausted", async () => {
+    const now = freezeNow();
+    const t = harness();
+    await seedBuckets(t, [1, 2, 3, 4, 5].map((octet) => ({
+      key: `rl:search:198.51.100.${octet}:1`,
+      expiresAt: now - 5_000,
+    })));
+
+    expect(await t.mutation(internal.rateLimit.cleanupExpired, {
+      maxBuckets: 2,
+      followupsRemaining: 1,
+    })).toEqual({ deleted: 2, hasMore: true, scheduledFollowup: true });
+    await t.finishAllScheduledFunctions(() => {
+      vi.runAllTimers();
+    });
+    // Two batches of two, then the budget stops the chain. Index order is
+    // expiresAt then creation time, so the last inserted key remains.
+    expect(await remainingKeys(t)).toEqual(["rl:search:198.51.100.5:1"]);
+  });
+
+  it("clamps an oversized page to the transaction cap and uses the default page otherwise", async () => {
+    const now = freezeNow();
+    const t = harness();
+    await seedBuckets(t, Array.from({ length: CLEANUP_BATCH_LIMIT + 1 }, (_, index) => ({
+      key: `rl:page:192.0.2.${index}:1`,
+      expiresAt: now - 1,
+    })));
+
+    expect(await t.mutation(internal.rateLimit.cleanupExpired, {
+      maxBuckets: 10_000,
+      followupsRemaining: 0,
+    })).toEqual({ deleted: CLEANUP_BATCH_LIMIT, hasMore: true, scheduledFollowup: false });
+    expect(await remainingKeys(t)).toHaveLength(1);
+
+    const defaults = harness();
+    await seedBuckets(defaults, Array.from({ length: CLEANUP_BATCH_DEFAULT + 1 }, (_, index) => ({
+      key: `rl:page:192.0.2.${index}:9`,
+      expiresAt: now - 1,
+    })));
+    expect(await defaults.mutation(internal.rateLimit.cleanupExpired, { followupsRemaining: 0 })).toEqual({
+      deleted: CLEANUP_BATCH_DEFAULT,
+      hasMore: true,
+      scheduledFollowup: false,
+    });
+  });
+});

convex/rateLimit.ts

@@ -1,4 +1,5 @@
 import { v } from "convex/values";
+import { internal } from "./_generated/api";
 import { internalMutation, mutation } from "./_generated/server";
 
 // Server-owned limits. These used to arrive as arguments, which let any caller
@@ -48,20 +49,45 @@ export const enforce = mutation({
   },
 });
 
+// One mutation stays under Convex's per-transaction document limits.
+// The cron asks for the cap; a caller that omits maxBuckets deletes a smaller page.
+export const CLEANUP_BATCH_DEFAULT = 100;
+export const CLEANUP_BATCH_LIMIT = 500;
+// Further batches one wave may schedule after the current mutation. The next
+// cron tick continues any remainder, so a stuck hasMore cannot loop forever.
+export const CLEANUP_FOLLOWUP_LIMIT = 40;
+
+function cleanupBatchSize(maxBuckets: number | undefined): number {
+  if (typeof maxBuckets !== "number" || !Number.isFinite(maxBuckets)) return CLEANUP_BATCH_DEFAULT;
+  return Math.max(1, Math.min(Math.floor(maxBuckets), CLEANUP_BATCH_LIMIT));
+}
+
+function cleanupFollowupsRemaining(followupsRemaining: number | undefined): number {
+  if (typeof followupsRemaining !== "number" || !Number.isFinite(followupsRemaining)) {
+    return CLEANUP_FOLLOWUP_LIMIT;
+  }
+  return Math.max(0, Math.min(Math.floor(followupsRemaining), CLEANUP_FOLLOWUP_LIMIT));
+}
+
 // Internal: this deletes rows on a time predicate. As a public mutation taking
 // a caller-supplied `now`, one call with a far-future timestamp emptied the
-// whole table and turned rate limiting off.
+// whole table and turned rate limiting off. The clock is read here, and
+// convex/crons.ts is what schedules the run.
 export const cleanupExpired = internalMutation({
   args: {
     maxBuckets: v.optional(v.number()),
+    followupsRemaining: v.optional(v.number()),
   },
+  returns: v.object({
+    deleted: v.number(),
+    hasMore: v.boolean(),
+    scheduledFollowup: v.boolean(),
+  }),
   handler: async (ctx, args) => {
-    const maxBuckets =
-      typeof args.maxBuckets === "number" && Number.isFinite(args.maxBuckets)
-        ? Math.max(1, Math.min(Math.floor(args.maxBuckets), 500))
-        : 100;
+    const maxBuckets = cleanupBatchSize(args.maxBuckets);
+    const followupsRemaining = cleanupFollowupsRemaining(args.followupsRemaining);
     // Read one past the page so a full final page is not mistaken for
-    // "more remain", which would loop a caller forever on the last batch.
+    // "more remain", which would schedule follow-ups forever on the last batch.
     const expired = await ctx.db
       .query("rateLimitBuckets")
       .withIndex("by_expiresAt", (q) => q.lt("expiresAt", Date.now()))
@@ -71,6 +97,15 @@ export const cleanupExpired = internalMutation({
       await ctx.db.delete(row._id);
     }
 
-    return { deleted: deletable.length, hasMore: expired.length > maxBuckets };
+    const hasMore = expired.length > maxBuckets;
+    const scheduledFollowup = hasMore && followupsRemaining > 0;
+    if (scheduledFollowup) {
+      await ctx.scheduler.runAfter(0, internal.rateLimit.cleanupExpired, {
+        maxBuckets,
+        followupsRemaining: followupsRemaining - 1,
+      });
+    }
+
+    return { deleted: deletable.length, hasMore, scheduledFollowup };
   },
 });