Skip to content

fix(auth): enforce ratePerDay daily quota alongside per-minute limits (#175) - #526

Open
sparkysolutions10-pixel wants to merge 1 commit into
Divinity-Labs:mainfrom
sparkysolutions10-pixel:fix/enforce-rate-per-day-175
Open

sparkysolutions10-pixel wants to merge 1 commit into
Divinity-Labs:mainfrom
sparkysolutions10-pixel:fix/enforce-rate-per-day-175

Conversation

@sparkysolutions10-pixel

Copy link
Copy Markdown
Contributor

Closes #175

Summary of Changes

  • Enforces the ratePerDay field defined on API keys during request authentication.
  • Tracks daily usage per API key ID with UTC midnight rollover.
  • Attaches standard rate-limiting response headers:
    • X-RateLimit-Limit-Day
    • X-RateLimit-Remaining-Day
  • Returns HTTP 429 Too Many Requests with informative reset message when daily quota is exhausted.
  • Adds comprehensive unit test suite in src/__tests__/dailyRateLimit.test.ts.

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for re-cutting to one issue — this is the shape I asked for, and #175 is unassigned so it's genuinely yours to earn. The diagnosis is right: ratePerDay really was stored, returned, advertised and never enforced. Three things to fix before I can merge it.

1. An in-memory daily counter doesn't survive a cold start

const dailyUsage = new Map<string, { count: number; date: string }>()

I checked whether this matches the house pattern before objecting, and in one sense it does — the per-minute limiter is @fastify/rate-limit with no store configured, so it's in-memory too. But the time scale changes the conclusion entirely.

A one-minute window that resets on restart loses at most a minute of accounting. A daily window that resets on restart loses the entire quota, and Lens runs on Render's free tier, where the service spins down after a short idle period and cold-starts on the next request. I watched a sibling service on the same platform go from a live /status to 503 within fifteen minutes earlier today. So in practice a caller gets their full daily quota back every time the service naps — which means ratePerDay would still be unenforced, just less obviously. That's a worse outcome than leaving it unenforced honestly, because the header would now claim it's working.

It also goes per-instance the moment there is more than one.

Redis is already here, already used for x402 metering and the price cache, and src/redis.ts is explicit about its failure behaviour. INCR on a key like ratelimit:day:{apiKeyId}:{YYYY-MM-DD} with EXPIRE set to the seconds remaining until UTC midnight gives you the counter, the reset and the cleanup in two commands.

On Redis being unreachable, allow the request and log it — do not fail closed. src/redis.ts notes that x402 metering fails closed because it guards an actual payment; a rate limiter failing closed would turn a cache outage into a total API outage for every authenticated caller, which is a far worse trade. Please put that reasoning in a comment so nobody later "fixes" it the other way.

2. The map grows without bound

An entry is only replaced when that same key is seen again on a later date. A key used once and never again keeps its entry for the lifetime of the process, so the map grows with the number of distinct keys ever seen. Redis with a TTL removes this problem rather than needing a sweep — which is the main reason I'd rather you move it there than add eviction.

Worth noting we merged #335 today for exactly this class of bug on the websocket path.

3. retryAfter should be a number, and there should be a header

retryAfter: 'midnight UTC',

A client can't compute anything from that string. Two things it needs:

  • retryAfter as seconds until UTC midnight, which is what the existing per-minute limiter does (retryAfter: context.after).
  • A real Retry-After response header, which is the part HTTP clients and SDKs actually honour automatically.

Keep the human sentence in message by all means — that's good and I'd leave it — but the machine-readable fields have to be machine-readable. Note that #272 covers the standard 429 contract and is assigned to someone else, so match the existing per-minute shape here rather than inventing a third one.

Smaller

// Enforce ratePerDay daily quota (#175) — the house style in this repo is comments that explain why, not issue references; git history carries the issue number. Something like "the per-minute limiter cannot express a daily cap, so this runs alongside it" earns its line.

What's right

Reading the limit from the authenticated key's own metadata rather than a global, skipping enforcement entirely when ratePerDay is unset or zero, emitting the limit and remaining headers on every response rather than only on rejection, and exporting resetDailyUsage() so the tests aren't order-dependent. The five tests are focused and readable.

#175 stays yours. Move the counter to Redis with the fail-open comment, make retryAfter numeric with the header, and I'll re-review quickly.

https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ratePerDay is stored, returned, advertised — and never enforced

2 participants