Repository navigation
Make API quota admission atomic - #11
Merged
Merged
Conversation
Admission read the group's usage, decided, and wrote it back in three separate statements with no lock and no transaction. Two requests arriving together both read the same total and were both admitted — and because they then wrote the same value, the overage left no trace afterwards. Measured against the previous implementation on real PostgreSQL: twelve simultaneous requests against a limit of three were all admitted, and twenty against a shared rotation budget of six were all admitted. Underneath that was a structural problem. Usage lived on each ApiKey row and the group total was summed at read time, so the enforced quantity was a derived sum nobody could lock — every row had its own independent read-modify-write race. Usage now lives in ApiQuotaCounter, one row per tenant-scoped key group per UTC day, and admission is a single INSERT ... ON CONFLICT DO UPDATE. The limit sits in the WHERE clause of that update: when the row is already at the limit the update is skipped, the statement returns no rows, and the refusal is the fact that nothing was spent. There is no path that spends the budget and then rejects. Concurrent callers serialize on the unique index. A new UTC day is a different unique key, so the budget resets by insertion rather than by a job that could be down. A rotation pair shares the row rather than summing two, so the overlap still cannot double an allowance. The counter is keyed by tenant as well as group, so one organization's usage cannot reach another's even if a group id were duplicated — a test forces exactly that collision. ApiKey.usageCount and usageDay are still written, but only as per-key bookkeeping for the settings screen. They no longer admit anything, and a test sets a key's own count far above its quota to prove the counter is what decides. The migration backfills counters from existing per-key usage, summed per group exactly as the old read-time aggregation did, so nobody's spent budget is forgotten at the cutover. Phase 2 criterion 7 is now genuinely satisfied. Phase 2 itself stays in progress: representative-data migration, rollback rehearsal and a deployed worker are still open, and roadmap.ts is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQ2eShwv2iVM3CQYpjEkxK
GunsNR
marked this pull request as ready for review
August 30, 2026 02:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
API quota admission was a read-then-write race.
authenticateApiKeyread the key row, comparedusageCountagainstdailyQuotain application code, and then wrote the incremented count back. Two requests that read the same row before either wrote were both admitted. The window is small but it is exactly the window an attacker controls: fire N requests concurrently and N are admitted regardless of the limit.The count also lived on the
ApiKeyrow, so a rotated key started a fresh budget — rotation reset the quota rather than continuing it.Measured against the pre-change algorithm on PostgreSQL 16:
With this change: exactly 3 and exactly 6.
What changed
A new
ApiQuotaCountertable holds the authoritative count, keyed by(orgId, quotaGroupId, usageDay)with a unique constraint. Admission is one statement ($limitis the bound parameter carryingdailyQuota):The limit is enforced inside the same statement that increments.
ON CONFLICT DO UPDATEtakes a row lock, so concurrent updaters serialize on it; theWHEREre-evaluates against the locked, committed row. A refused request returns zero rows and spends nothing.Keying on
quotaGroupIdrather than key id means a rotated key and its predecessor share one counter — rotation no longer resets the budget. Keying on the UTC usage day gives a clean rollover with no reset job.The per-key
usageCount/usageDaycolumns are kept as bookkeeping for the UI but are no longer consulted for admission.Migration
Hand-written (
20260830022346_atomic_api_quota_counter): creates the table and unique index, then backfills from existingApiKeyrows, summingusageCountper(orgId, quotaGroupId, usageDay)so today's in-flight budgets carry over rather than resetting to zero.Tests
supertool/tests/apikey-quota.test.ts(new, 19 tests) against a real PostgreSQL database:authenticateApiKeypath against a limit of 3 → exactly 3dailyQuota = 0means unlimitedOne structural assertion in
apikey-rotation.test.tspinned the old query shape; it is updated to the new one rather than removed.Verification
Run locally against PostgreSQL 16:
prisma migrate deployon a fresh database, thenmigrate diff→ no driftnpm run typecheck→ cleannpm run lint→ cleannpm test→ 733 passed / 733 (41 files)npm run build→ compilednpm run db:rehearse→ passedPhase 2 status
This closes criterion 7 ("API keys carry scopes, quotas and a rotation flow"). Of the nine Phase 2 acceptance criteria, six are now satisfied — 1, 5, 6, 7, 8, 9 — and three remain open:
Phase 2 is therefore not complete, and
roadmap.tsis unchanged:phase-2remainsin-progress.Scope
Nothing is deployed. Railway is untouched. No capability flag is activated —
public_apistaysbeta.docs/release-truth-audit.mdandsrc/lib/capabilities.tsare updated to say that admission is now atomic, replacing the previous accurate note that it was not. The "never exercised by a third-party integrator against a real deployment" caveat is retained.