From 5185edca749647055e9a06d453e6b0932722b2ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Tue, 20 Oct 2026 16:09:28 +0200 Subject: [PATCH] fix(rate-limit): fail the shared write ceiling open and skip reads over POST The shared write bucket threw on any SQL error, so a lock on rate_limits answered every authenticated write with 500. It is a loop cutter, not an access check: when the bucket cannot be read the write now goes ahead with a warn signal. The login and other auth-surface limits call the limiter directly and still fail closed. requireAuth now passes readOverPost to the meter, the draft-only document assist declares it, and the three chat routes, which meter their turns with buckets of their own, declare that instead of being counted twice. --- docs/api/openapi.yaml | 17 +++--- src/__tests__/read-over-post-guard.test.ts | 14 ++++- src/__tests__/write-rate-limit-guard.test.ts | 6 ++ .../api/documents/inbound/[id]/chat/route.ts | 4 +- .../documents/inbound/[id]/suggest/route.ts | 4 +- src/app/api/insights/chat/fenced/route.ts | 3 +- src/app/api/insights/chat/route.ts | 4 +- src/lib/api-handler.ts | 2 +- src/lib/auth/__tests__/write-meter.test.ts | 30 ++++++++++ src/lib/auth/write-meter.ts | 22 ++++++- src/lib/openapi/routes/shared.ts | 2 +- tests/integration/write-meter.test.ts | 59 ++++++++++++++++++- 12 files changed, 148 insertions(+), 19 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 06661dca7..795ae7379 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -398,14 +398,15 @@ paths: "429": &a12 description: "Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:`, 300 requests per 60 seconds — keyed on the ACTING account, so a delegate burns - their own allowance rather than the record owner's. Only the bulk ingest endpoints, which meter themselves - with a dedicated bucket, stay outside it. Nothing was written. `meta.errorCode` is - `record_write.rate_limited`, which is what tells this refusal apart from a route's own narrower bucket when - both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` - headers describe that bucket; back off on `Retry-After` rather than guessing. Every request counts against - the bucket, refused ones included: the check runs before the body is read, so a client looping on a 422 - spends the same allowance as one that writes rows. A client with more than a handful of rows to send should - use the batch endpoint for its domain instead of looping this one." + their own allowance rather than the record owner's. Only the routes that meter themselves with a dedicated + bucket (the bulk ingest endpoints and the model-turn chats) and a POST that writes nothing stay outside it. + If the bucket cannot be read, the write goes ahead unmetered rather than failing. Nothing was written. + `meta.errorCode` is `record_write.rate_limited`, which is what tells this refusal apart from a route's own + narrower bucket when both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` + and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing. Every + request counts against the bucket, refused ones included: the check runs before the body is read, so a + client looping on a 422 spends the same allowance as one that writes rows. A client with more than a handful + of rows to send should use the batch endpoint for its domain instead of looping this one." headers: *a8 content: application/json: diff --git a/src/__tests__/read-over-post-guard.test.ts b/src/__tests__/read-over-post-guard.test.ts index c699a9ad4..d118df810 100644 --- a/src/__tests__/read-over-post-guard.test.ts +++ b/src/__tests__/read-over-post-guard.test.ts @@ -2,8 +2,10 @@ * `readOverPost` lets a delegable route answer a POST at the grant's read * level. That widens what a read-only grant reaches, so the set of routes * declaring it is frozen here: adding one is a reviewed diff, not an - * accident. Today it is the record search alone, whose query is health data - * that must stay out of URLs. + * accident. Today it is the record search, whose query is health data that + * must stay out of URLs, and the document assist, whose POST writes nothing + * (under `requireAuth` the declaration only keeps it off the shared write + * ceiling). * * The matcher tolerates whitespace and line breaks, and the guard asserts it * matched something, so a rewrite that hides the declaration from the regex @@ -16,7 +18,13 @@ import { join, relative, sep } from "node:path"; const ROOT = process.cwd(); -const FROZEN = ["src/app/api/search/route.ts"]; +const FROZEN = [ + // The draft-only document assist: a POST that writes nothing. Under + // `requireAuth` the declaration only keeps it out of the shared write + // ceiling; there is no grant for it to widen. + "src/app/api/documents/inbound/[id]/suggest/route.ts", + "src/app/api/search/route.ts", +]; describe("readOverPost declarations", () => { it("are exactly the frozen set", () => { diff --git a/src/__tests__/write-rate-limit-guard.test.ts b/src/__tests__/write-rate-limit-guard.test.ts index a579655ee..3b3b3559b 100644 --- a/src/__tests__/write-rate-limit-guard.test.ts +++ b/src/__tests__/write-rate-limit-guard.test.ts @@ -75,6 +75,12 @@ const OWN_BUCKET: Record = { "The cycle HealthKit sweep sends 500-day slices in sequence; the route's own bulk bucket meters it.", "app/api/nutrients/batch/route.ts": "The nutrient sync sends 500-row batches in sequence; the route's own batch bucket meters it.", + "app/api/insights/chat/route.ts": + "A Coach turn is metered by the Coach's own buckets: 20 turns a minute (`coach-chat`) and one turn slot at a time.", + "app/api/insights/chat/fenced/route.ts": + "A fenced document chat turn is metered by its own per-account bucket, sized for model turns.", + "app/api/documents/inbound/[id]/chat/route.ts": + "A document chat turn is metered by its own per-account bucket, sized for model turns.", "app/api/insights/ecg/route.ts": "The ECG sweep sends one recording per request with no client-side delay, newest first, over the whole HealthKit history; the route's own 600-a-minute ingest bucket meters it.", }; diff --git a/src/app/api/documents/inbound/[id]/chat/route.ts b/src/app/api/documents/inbound/[id]/chat/route.ts index d37ff416e..49a24fdd7 100644 --- a/src/app/api/documents/inbound/[id]/chat/route.ts +++ b/src/app/api/documents/inbound/[id]/chat/route.ts @@ -71,7 +71,9 @@ type RouteParams = { params: Promise<{ id: string }> }; export const POST = apiHandler( async (request: NextRequest, { params }: RouteParams) => { - const { user } = await requireAuth(); + // Metered by the document chat's own bucket below, not the shared + // ceiling. + const { user } = await requireAuth(undefined, { writeBucket: "own" }); const userId = user.id; const gate = await requireModuleEnabled(userId, "inboundDocuments"); diff --git a/src/app/api/documents/inbound/[id]/suggest/route.ts b/src/app/api/documents/inbound/[id]/suggest/route.ts index da9d78fcc..47371f779 100644 --- a/src/app/api/documents/inbound/[id]/suggest/route.ts +++ b/src/app/api/documents/inbound/[id]/suggest/route.ts @@ -73,7 +73,9 @@ type RouteParams = { params: Promise<{ id: string }> }; export const POST = apiHandler( async (request: NextRequest, { params }: RouteParams) => { - const { user } = await requireAuth(); + // Writes nothing (the draft goes back to the edit form), so the shared + // write ceiling does not count it; the document-AI bucket meters it. + const { user } = await requireAuth(undefined, { readOverPost: true }); const gate = await requireModuleEnabled(user.id, "inboundDocuments"); if (!gate.enabled) return gate.response; diff --git a/src/app/api/insights/chat/fenced/route.ts b/src/app/api/insights/chat/fenced/route.ts index 1276a96de..66c5f1dc6 100644 --- a/src/app/api/insights/chat/fenced/route.ts +++ b/src/app/api/insights/chat/fenced/route.ts @@ -52,7 +52,8 @@ export const dynamic = "force-dynamic"; export const runtime = "nodejs"; export const POST = apiHandler(async (request: NextRequest) => { - const { user } = await requireAuth(); + // Metered by the fenced chat's own bucket below, not the shared ceiling. + const { user } = await requireAuth(undefined, { writeBucket: "own" }); const userId = user.id; const gate = await requireModuleEnabled(userId, "inboundDocuments"); diff --git a/src/app/api/insights/chat/route.ts b/src/app/api/insights/chat/route.ts index 3a3260f7a..5b8134950 100644 --- a/src/app/api/insights/chat/route.ts +++ b/src/app/api/insights/chat/route.ts @@ -81,7 +81,9 @@ const COACH_CONCURRENT_TURNS = 2; const COACH_TURN_SLOT_MS = TURN_LIMITS.user.wallMs + 30_000; async function handleChatRequest(request: NextRequest): Promise { - const auth = await requireAuth(); + // A turn is metered by the Coach's own buckets (`coach-chat`, the turn + // slot), not the shared write ceiling. + const auth = await requireAuth(undefined, { writeBucket: "own" }); // The `coach` capability, right after auth and before the rate limit and // the body parse. It folds in what the Coach module gate, the operator // switch and the `disableCoach` opt-out used to answer separately, plus diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 8166b60d5..5f0f9c9fa 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -546,7 +546,7 @@ export function isScopedCredential(auth: AuthContext): boolean { */ export async function requireAuth( requiredPermission?: string, - options?: Pick, + options?: WriteMeterOptions, ): Promise { const auth = await authenticateCaller(requiredPermission); const carrier = await readActingCarrier(auth); diff --git a/src/lib/auth/__tests__/write-meter.test.ts b/src/lib/auth/__tests__/write-meter.test.ts index ac6069b83..ddf381d31 100644 --- a/src/lib/auth/__tests__/write-meter.test.ts +++ b/src/lib/auth/__tests__/write-meter.test.ts @@ -16,6 +16,12 @@ vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn(), })); +const signals = vi.hoisted(() => ({ emitted: [] as unknown[] })); + +vi.mock("@/lib/logging/signal", () => ({ + emitSignal: (input: unknown) => signals.emitted.push(input), +})); + vi.mock("@/lib/rate-limit", () => ({ checkRecordWriteRateLimit: vi.fn(async () => ({ allowed: state.allowed, @@ -38,6 +44,7 @@ function request(method: string | undefined) { beforeEach(() => { vi.mocked(checkRecordWriteRateLimit).mockClear(); + signals.emitted.length = 0; state.allowed = true; state.event = null; }); @@ -99,4 +106,27 @@ describe("meterAccountWrite", () => { RECORD_WRITE_RATE_LIMITED_CODE, ); }); + + it("lets the write through with a warning when the bucket cannot be read", async () => { + vi.mocked(checkRecordWriteRateLimit).mockRejectedValueOnce( + new Error("canceling statement due to lock timeout"), + ); + request("POST"); + await expect(meterAccountWrite("account-1")).resolves.toBeUndefined(); + expect(signals.emitted).toEqual([ + expect.objectContaining({ + action: "rate_limit.record_write.unavailable", + level: "warn", + }), + ]); + }); + + it("counter-check: a readable, spent bucket still refuses, with no warning", async () => { + state.allowed = false; + request("POST"); + await expect(meterAccountWrite("account-1")).rejects.toBeInstanceOf( + HttpError, + ); + expect(signals.emitted).toEqual([]); + }); }); diff --git a/src/lib/auth/write-meter.ts b/src/lib/auth/write-meter.ts index 25c4a1895..8743739eb 100644 --- a/src/lib/auth/write-meter.ts +++ b/src/lib/auth/write-meter.ts @@ -31,9 +31,18 @@ * record owner's, and cannot collect a fresh one by switching records. A * refused request is counted too, since the charge happens before the body is * read. + * + * The bucket fails open. It cuts off a runaway loop; it is not what decides + * who may write, which the resolver that called it has already settled. So + * when the bucket itself cannot be read (the upsert fails: a lock on + * `rate_limits`, a pool at its limit), the write goes ahead and a `warn` + * signal says the ceiling was not applied, instead of every authenticated + * write in the app answering 500. The login, sign-up and other auth-surface + * limits call the limiter directly and keep failing closed. */ import { HttpError } from "@/lib/api-errors"; import { annotate, getEvent } from "@/lib/logging/context"; +import { emitSignal } from "@/lib/logging/signal"; import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; /** The code a refusal by the shared write bucket carries. */ @@ -76,7 +85,18 @@ export async function meterAccountWrite( if (charged.has(evt)) return; charged.add(evt); - const rl = await checkRecordWriteRateLimit(accountId); + let rl: Awaited>; + try { + rl = await checkRecordWriteRateLimit(accountId); + } catch (error) { + annotate({ meta: { record_write_rate_limit_unavailable: true } }); + emitSignal({ + action: "rate_limit.record_write.unavailable", + level: "warn", + error, + }); + return; + } if (!rl.allowed) { annotate({ meta: { record_write_rate_limited: true } }); throw new HttpError( diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index afa7f2533..1e929b18e 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -373,7 +373,7 @@ export const stdResponses = { export const recordWriteRateLimitResponse = { "429": { description: - "Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:`, 300 requests per 60 seconds — keyed on the ACTING account, so a delegate burns their own allowance rather than the record owner's. Only the bulk ingest endpoints, which meter themselves with a dedicated bucket, stay outside it. Nothing was written. `meta.errorCode` is `record_write.rate_limited`, which is what tells this refusal apart from a route's own narrower bucket when both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing. Every request counts against the bucket, refused ones included: the check runs before the body is read, so a client looping on a 422 spends the same allowance as one that writes rows. A client with more than a handful of rows to send should use the batch endpoint for its domain instead of looping this one.", + "Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:`, 300 requests per 60 seconds — keyed on the ACTING account, so a delegate burns their own allowance rather than the record owner's. Only the routes that meter themselves with a dedicated bucket (the bulk ingest endpoints and the model-turn chats) and a POST that writes nothing stay outside it. If the bucket cannot be read, the write goes ahead unmetered rather than failing. Nothing was written. `meta.errorCode` is `record_write.rate_limited`, which is what tells this refusal apart from a route's own narrower bucket when both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing. Every request counts against the bucket, refused ones included: the check runs before the body is read, so a client looping on a 422 spends the same allowance as one that writes rows. A client with more than a handful of rows to send should use the batch endpoint for its domain instead of looping this one.", content: { "application/json": { schema: errorEnvelope } }, headers: stdResponses["429"].headers, }, diff --git a/tests/integration/write-meter.test.ts b/tests/integration/write-meter.test.ts index e72265dbf..cd945a9c1 100644 --- a/tests/integration/write-meter.test.ts +++ b/tests/integration/write-meter.test.ts @@ -6,7 +6,9 @@ * before. What has to hold: a write is counted, a read is not, a spent bucket * refuses with the published code and the headers that describe it and * writes nothing, and a bulk route on its own bucket keeps working while the - * shared one is spent. + * shared one is spent. A POST that reads (`readOverPost`) is not counted on + * either resolver, and a bucket that cannot be read lets the write through + * while the limiter itself still fails for its other callers. */ import { NextRequest } from "next/server"; import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -183,3 +185,58 @@ describe("the shared write bucket on routes without a limiter of their own", () ).toBe(1); }); }); + +describe("what the shared bucket does not count", () => { + async function postThrough(options?: { readOverPost: true }) { + const { apiHandler, requireAuth } = await import("@/lib/api-handler"); + const { apiSuccess } = await import("@/lib/api-response"); + const POST = apiHandler(async (_request: NextRequest) => { + const { user } = await requireAuth(undefined, options); + return apiSuccess({ id: user.id }); + }); + return POST( + new NextRequest("http://localhost/api/probe", { + method: "POST", + body: "{}", + headers: { "content-type": "application/json" }, + }), + ); + } + + it("leaves a read-over-POST through requireAuth uncounted", async () => { + expect((await postThrough({ readOverPost: true })).status).toBe(200); + expect(await bucketCount()).toBe(0); + }); + + it("counter-check: the same POST without the declaration is counted", async () => { + expect((await postThrough()).status).toBe(200); + expect(await bucketCount()).toBe(1); + }); +}); + +describe("a bucket that cannot be read", () => { + it("lets the write through instead of answering 500, and the limiter still fails for its own callers", async () => { + const prisma = getPrismaClient(); + const { checkRateLimit } = await import("@/lib/rate-limit"); + await prisma.$executeRawUnsafe( + `ALTER TABLE "rate_limits" RENAME TO "rate_limits_away"`, + ); + try { + const res = await patchAllergy({ substance: "Birch pollen" }); + expect(res.status, await res.clone().text()).toBe(200); + const row = await prisma.allergy.findUniqueOrThrow({ + where: { id: allergyId }, + }); + expect(row.substance).toBe("Birch pollen"); + + // A login or sign-up limit calls the limiter directly: no fail-open. + await expect( + checkRateLimit("auth:login:probe", 5, 60_000), + ).rejects.toThrow(); + } finally { + await prisma.$executeRawUnsafe( + `ALTER TABLE "rate_limits_away" RENAME TO "rate_limits"`, + ); + } + }); +});