Repository navigation
fix(auth): enforce ratePerDay daily quota alongside per-minute limits (#175) - #526
Conversation
Miracle656
left a comment
There was a problem hiding this comment.
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:
retryAfteras seconds until UTC midnight, which is what the existing per-minute limiter does (retryAfter: context.after).- A real
Retry-Afterresponse 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.
Closes #175
Summary of Changes
ratePerDayfield defined on API keys during request authentication.X-RateLimit-Limit-DayX-RateLimit-Remaining-DayToo Many Requestswith informative reset message when daily quota is exhausted.src/__tests__/dailyRateLimit.test.ts.