Skip to content

Add auth metrics and global limits - #520

Merged
Miracle656 merged 1 commit into
Divinity-Labs:mainfrom
Kingsley4867:feat/auth-metrics-global-limit-codes
Oct 19, 2026
Merged

Miracle656 merged 1 commit into
Divinity-Labs:mainfrom
Kingsley4867:feat/auth-metrics-global-limit-codes

Conversation

@Kingsley4867

@Kingsley4867 Kingsley4867 commented Oct 18, 2026 •

Copy link
Copy Markdown

Summary

  • Add bounded authentication failure metrics and machine-readable API-key 401 codes for missing, malformed, unknown, revoked, and expired keys.
  • Add optional API-key expiry and separate admin authentication failure metrics.
  • Add a configurable per-instance rolling request ceiling that sheds expensive routes first, exempts health endpoints, and distinguishes global shedding from per-key/IP rate limiting.
  • Update documentation and add tests for authentication outcomes and global request shedding.

Related issue

Closes #507
Closes #509
Closes #511

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests
  • CI / tooling

Checklist

  • I have read CONTRIBUTING.md
  • npx tsc --noEmit passes
  • npm run build passes
  • I added / updated tests where relevant
  • I updated docs where relevant

Validation was not run, as requested.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@drips-wave

drips-wave Bot commented Oct 18, 2026

Copy link
Copy Markdown

@Kingsley4867 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Miracle656
Miracle656 merged commit a751852 into Divinity-Labs:main Oct 19, 2026
@Miracle656

Copy link
Copy Markdown
Collaborator

Merged. Three related things, all of them wanted.

The metrics are the quiet win. A rejected request was previously invisible — a misconfigured client hammering a bad key and someone probing ADMIN_TOKEN looked identical to nothing happening at all. admin_auth_failures_total, api_key_auth_failures_total{reason} and rate_limit_rejections_total{scope} make both observable, and the label cardinality is bounded (fixed reason strings, key/ip) rather than driven by caller input, which is the trap with auth metrics.

The load shedder is correctly scoped. Being per-process and in-memory would be a flaw in a quota, but this is self-defence — it protects this instance's resources, so per-process is the right answer and the fleet ceiling being N× the value is intended rather than a bug. Shedding expensive route templates at 80% before everything else is a nice touch: the routes that cost the most give way first, so cheap requests keep succeeding while the process is under pressure.

parseGlobalRateLimit throwing on an invalid value instead of defaulting is the right instinct, and the same one #539 applied to the depth limit — a config typo must never quietly mean "no ceiling".

⚠️ One deploy-order consequence, worth being explicit about

api_keys.expires_at has to exist before this code runs, because lookupApiKey now selects it. If it doesn't, every authenticated request fails, not just key creation — this is the widest-blast-radius schema dependency we've merged. Lens has no migrations directory (schema-first via db push), so the one statement is:

ALTER TABLE "api_keys" ADD COLUMN IF NOT EXISTS "expires_at" TIMESTAMP(3);

Flagged to the maintainer alongside a pending wraith column. Not a problem with your PR — just something that must go first.

Fixes while merging

  • src/api/usage.ts re-added a local isAdminAuthorized. fix: deduplicate isAdminAuthorized into shared auth module #517 removed exactly that copy earlier today (it had already drifted to an inline require('crypto')). Kept the shared one from ./auth plus your metric import.
  • src/app.ts allow-list. Yours was req.url === '/status'; main now has PROBE_PATHS.has(req.url) because feat: add /live and /ready health probes #544 added /live and /ready, which the platform also polls on a fixed interval and which must not be rate limited either. Kept the superset, took your metric-emitting errorResponseBuilder and the global-ceiling registration.
  • .env.example was missing GLOBAL_RATE_LIMIT_MAX — the envDrift guard caught it. Documented, including the per-instance-versus-fleet distinction so nobody reads it as a global quota.
  • npx prisma generate was needed after the schema change to clear phantom expiresAt type errors. Worth knowing: a schema.prisma change in this repo means regenerating before typecheck tells you the truth.

One thing for later, not blocking

acceptedAt is an array with shift() in a while loop on every request. At the default 1000 it's fine, but Array.shift() is O(n) and you're doing it inside a loop over a structure that can hold max entries — so the cost of being near the ceiling grows with the ceiling, which is exactly when you least want extra work per request. A ring buffer, or two counters in fixed sub-windows, would make it O(1). Worth a follow-up if GLOBAL_RATE_LIMIT_MAX is ever raised much.

Also note #551 (api key expiration) covers the same ground as the expires_at part of this. Since this landed, that one will need a rebase and will likely be mostly redundant — I'll pick it up with that in mind.

Verified on the merge result rather than your branch: tsc --noEmit silent, 908 tests across 84 suites, nothing red.

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.

A 401 says the same thing for four different problems Rate limiting is per key, with no global ceiling Nothing counts authentication failures

3 participants