Skip to content

perf(auth): cache API-key lookups to drop the per-request DB round-tr… - #594

Merged
Miracle656 merged 2 commits into
Divinity-Labs:mainfrom
CHKM001:feat/api-key-auth-cache-495
Oct 23, 2026
Merged

Miracle656 merged 2 commits into
Divinity-Labs:mainfrom
CHKM001:feat/api-key-auth-cache-495

Conversation

@CHKM001

@CHKM001 CHKM001 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

PR #495 Cache API-key lookups to drop the per-request DB round-trip (#495)

Problem

src/api/auth.ts authenticates in an onRequest hook, and every request did a
prisma.apiKey.findUnique before any work began. Two consequences:

  • Latency floor. Every request paid a Postgres round-trip up front —
    including requests that would otherwise be served entirely from the Redis
    price cache.
  • Availability. The connection pool is shared with the ingesters and the
    retention worker. When Postgres was slow or the pool was exhausted, the API
    did not degrade — it stopped, because nothing could authenticate.

Solution

Cache the lookup in-process, keyed by the key hash, for a short TTL
(API_KEY_CACHE_TTL, default 30s). The hard part is keeping revocation
effectively immediate; this does it with invalidation and a short TTL, and
documents the worst-case window in the code.

  • Positive and negative lookups are both cached. Without caching the
    negative result, an invalid key is a free way to make the API hit Postgres as
    fast as a client can send requests. Now a repeated invalid key is one query,
    not one per attempt.
  • Expiry is re-checked on every cache hit. The entry stores the key's own
    expiresAt, and evaluate() re-tests it against the current time, so the
    cache TTL can never extend a key past its own expiry regardless of its length.
  • Revocation stays effectively immediate. The revoke route
    (DELETE /admin/keys/:id) calls invalidateApiKey(hash) after the update, so
    a key revoked on the instance that handled the revoke stops working at once.
    A key revoked on another instance keeps working locally only until its cache
    entry expires — a worst case of exactly API_KEY_CACHE_TTL seconds, which
    is stated in the comment at the cache definition.
  • The cache is size-bounded (MAX_CACHE_ENTRIES = 10_000, oldest-evicted),
    so a flood of distinct invalid keys cannot grow the negative cache without
    limit.
  • Hit rate is observable. api_key_cache_lookups_total{result="hit"|"miss"}
    is exported; hit rate is hit / (hit + miss).

Why in-process rather than Redis

The whole point is to stop depending on a shared backing store on the request
hot path and to survive when it is unavailable. An in-process map with a short
TTL and direct invalidation gives immediate revocation on the handling instance
and a bounded, documented window elsewhere, with no new dependency on the
request path. The short TTL is the cross-instance backstop.

Changes

  • src/api/auth.ts — in-process TTL cache keyed by hash, storing positive
    and negative results; evaluate() re-checks expiry on every hit; bounded size
    with oldest-entry eviction; exports invalidateApiKey(hash) and
    clearApiKeyCache() (test helper). lookupApiKeyResult now consults the cache
    and records a hit/miss metric on each call.
  • src/api/admin.ts — the revoke route calls invalidateApiKey(existing.hash)
    after marking the key revoked, so revocation is immediate on this instance.
  • src/metrics.ts — new api_key_cache_lookups_total counter, labeled
    result (bounded to hit/miss).
  • src/config.ts — cache.apiKeyTtl from API_KEY_CACHE_TTL (default 30),
    with a comment spelling out the revocation window.
  • .env.example — documents API_KEY_CACHE_TTL.
  • src/__tests__/auth.test.ts — clears the cache between tests; adds the
    cache suite (below).

Acceptance criteria

  • A repeated request with the same key does not query Postgres every time —
    serves a repeated request for the same key from cache (no Postgres per request)
    (3 requests, 1 findUnique).
  • Revoking a key stops it working within a documented, short window —
    stops a revoked key immediately on the instance that handles the revoke and
    stops a revoked key within the cache TTL even without explicit invalidation.
  • Invalid keys do not cause a database query per attempt —
    caches negative lookups so invalid keys do not hit Postgres per attempt.
  • Cache hit rate is exported as a metric —
    exports cache lookups as a hit/miss metric.
  • A test proves a revoked key stops working — the two revocation tests above.

Plus: re-checks expiry on a cache hit, so the TTL never outlives the key.

Testing

  • npx tsc --noEmit — clean.
  • npx vitest run src/__tests__/auth.test.ts — 28 passed (6 new).
  • Full suite: 947 passed, 1 skipped (pre-existing).

Notes / trade-offs

  • Default TTL is 30s; lower API_KEY_CACHE_TTL to shorten the cross-instance
    revocation window, at the cost of more findUnique calls.
  • The cache is per process and intentionally not shared; a rolling restart or
    deploy clears it, which only costs a one-time miss per key.
  • api_key_auth_failures_total still increments per failed request (it counts
    rejections, not DB queries), so failure observability is unchanged.

Closes #495

@drips-wave

drips-wave Bot commented Oct 21, 2026

Copy link
Copy Markdown

@CHKM001 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

…ip (Divinity-Labs#495)

Authentication runs in an onRequest hook, so every request — including ones
served entirely from the Redis price cache — paid a Postgres round-trip before
any work began, and the API could not accept traffic at all when the shared
connection pool was slow or exhausted.

Cache the lookup in-process, keyed by the key hash, for a short TTL
(API_KEY_CACHE_TTL, default 30s):

- Positive and negative lookups are both cached, so an invalid key can no
  longer make the API hit Postgres as fast as a client can send requests.
- Expiry is re-checked on every cache hit, so the TTL never extends a key
  past its own expiresAt.
- Revocation stays effectively immediate: the revoke route invalidates the
  entry directly, so it stops working at once on that instance; other
  instances pick it up within the TTL (the documented worst-case window).
- The cache is size-bounded so a flood of distinct invalid keys cannot grow
  the negative cache without limit.
- Hit/miss is exported as api_key_cache_lookups_total so hit rate is
  observable rather than assumed.

Tests cover repeat-request cache reuse, negative-lookup caching, expiry
re-check, immediate revocation, the cross-instance TTL window, and the metric.
@CHKM001
CHKM001 force-pushed the feat/api-key-auth-cache-495 branch from 6ae529e to a7435e6 Compare October 21, 2026 11:00
`ingestPool`'s `beforeEach` only calls `vi.clearAllMocks()`, which resets
call history but not mock implementations. The "does not throw when
upsertPricePoints rejects" test set a persistent `mockRejectedValue`, so
when vitest shuffled the `it` order (as it does in CI) that rejection
survived into "stores a price point": upsertPricePoints rejected,
ingestPool swallowed the error in its catch, and dispatchPriceUpdate was
never called — an order-dependent flake that made the suite, and so every
PR's CI, intermittently red.

- Use `mockRejectedValueOnce` so the rejection cannot outlive its test.
- Restore the default resolved implementations in `beforeEach` as a guard.

Verified: the previously-failing shuffle seed now passes, plus three
further shuffled runs (942 passed, 1 skipped each).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Miracle656
Miracle656 merged commit 2e696c4 into Divinity-Labs:main Oct 23, 2026
@Miracle656

Copy link
Copy Markdown
Collaborator

Merged as 2e696c4. This is the best-reasoned PR I have read in this repo. Caching authentication is the kind of change that is easy to get 90% right and have the last 10% be a security hole, and you enumerated the sharp edges in the code itself rather than in the PR description where it would be lost:

  • expiry re-checked against the stored expiresAt on every hit, so the TTL can never extend a key past its own expiry;
  • negative lookups cached, because otherwise an invalid key is a free way to make the API hit Postgres as fast as a client can send;
  • the cache bounded, so a flood of distinct invalid keys cannot grow it without limit;
  • the TTL named in config.ts with a comment stating plainly that it is the worst-case cross-instance revocation window.

That last one is what makes the trade reviewable instead of hidden. And the result: hit|miss label with its cardinality noted is exactly right.

Merged locally. Four conflicts, all from auth work that landed after your branch was cut. Three things on top.


1. The cached context dropped scopes, which would have 403'd every scoped route

ApiKeyContext gained a field after your branch was cut:

export interface ApiKeyContext {
  id: string
  label: string
  ratePerMin: number
  ratePerDay: number
  scopes: string[]   // ← added since
}

Your CachedKey built the context from four fields. So on merge the behaviour would have been: first request for a key reads Postgres and works; every request for the next 30 seconds is served from cache with scopes: undefined, fails hasScope, and returns 403 insufficient_scope.

Worth dwelling on how that would have presented. One request in thirty seconds works, the rest 403 — intermittent, TTL-shaped, and the error says insufficient scope, so the first place anyone looks is the key's scopes in the database, where everything is correct. It is the sort of bug that costs a day.

The cached context now carries scopes, and there is a test for it that also asserts the second request did not reach Postgres, so it cannot pass by accidentally missing the cache:

mockFindUnique.mockResolvedValue({ ...validRecord, scopes: ['read:prices'] })
app.get('/scoped', { config: { scope: 'read:prices' } }, ...)

expect(first.statusCode).toBe(200)
expect(second.statusCode).toBe(200)
expect(mockFindUnique).toHaveBeenCalledTimes(1)

The general point for anything cached here: the cached value has to be the whole context, and it has to be re-derived whenever ApiKeyContext grows. That is a trap your design cannot avoid, only survive — which is why the test matters more than the fix.

2. Rotation did not invalidate, and separately drops scopes

POST /admin/keys/:id/rotate (added to main after your branch) sets expiresAt on the old key to the end of the overlap window. Your invalidateApiKey call is on the revoke path, but not here — so the old key's cached entry still said expiresAt: null, and the overlap deadline only began to bind once the entry went stale. Bounded by the TTL, so small, but it is the same class of staleness you had already handled on revoke. Added:

invalidateApiKey(oldKey.hash)

Separately — this one is a bug already on main, not yours, and I fixed it here because it is in the lines I was touching. The replacement key is minted without scopes:

const newKey = await prisma.apiKey.create({
  data: { hash: ..., label: oldKey.label, ratePerMin: ..., /* no scopes */ },
})

scopes defaults to [] in the schema, so the rotated key authenticates perfectly and then 403s on every scoped route. Rotation hands the caller a key that cannot do what the old one did, during an overlap window specifically designed to avoid an outage. Now scopes: oldKey.scopes ?? [].

3. Two existing tests passed for reasons your cache invalidates

Both were green before and would have gone red on merge. Neither is wrong about the behaviour it cares about; both encoded an assumption that "one request means one lookup".

keyRotation.test.ts — "both keys work during overlap, and the old one fails after" simulated the overlap ending by rewriting the mocked database row from a future expiresAt to a past one, with the clock standing still. The cache legitimately ignores that: it already holds the row with the future expiry, and re-checking expiry against the cached value is precisely the behaviour you documented. So the test was measuring the cross-instance TTL window, not the thing it was named for.

It now advances the system clock past the overlap instead:

vi.useFakeTimers({ shouldAdvanceTime: true })
vi.setSystemTime(new Date(futureExpiration.getTime() + 1000))

which exercises the guarantee that actually matters — a cached entry cannot keep a key alive past its own expiry, with or without a database read — and would still fail if someone later removed the expiry re-check from evaluate. I kept the row swap too, and added vi.useRealTimers() to the file's afterEach so the advanced clock does not leak.

auth.test.ts — "tracks throttle intervals independently per key" (from #587, merged an hour before yours) queued three mockResolvedValueOnce records for three requests. With the cache, the second request for key-alpha never reaches Postgres, so the queue handed key-alpha's record to key-beta's lookup and the assertion failed on a count. Rewritten to resolve from a hash-keyed map, which is correct whether or not a lookup is cached:

mockFindUnique.mockImplementation(async ({ where }) => records[where.hash] ?? null)

Generally: once lookups are cached, mockResolvedValueOnce queues in auth tests are load-bearing on cache-miss ordering. Keyed implementations are the safe form.


126 test files / 1,506 tests pass. tsc --noEmit clean.

One thing I did not change but want on the record: the default 30-second cross-instance revocation window is a real weakening, and it is the right call given it is documented at the config site and Lens runs a single instance today. If Lens ever scales horizontally, the honest upgrade is a Redis pub/sub invalidation channel rather than a shorter TTL — worth an issue if you want to write it up.

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.

Every authenticated request costs a database round-trip

2 participants