perf(auth): opt-in in-memory cache for internal-secret account context#6779
perf(auth): opt-in in-memory cache for internal-secret account context#6779pfreixes wants to merge 2 commits into
Conversation
The account-context lookup for internal secret keys (persist/runner auth) runs an uncached 5-table join per request — roughly a third of all queries on the main DB. Cache the resolved context per process behind a short TTL, gated on keys that already passed a successful lookup. Rollout is driven by AUTH_ACCOUNT_CONTEXT_CACHE_MODE (off by default; dry records hit/miss metrics while still querying; on serves cached hits) with AUTH_ACCOUNT_CONTEXT_CACHE_TTL_MS bounding staleness (60s default). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 4/5
- In
packages/shared/lib/services/account.service.ts, cache-expiry misses can trigger parallel expensive lookups (a stampede), which may spike database load and increase latency under hot-key traffic; add per-key in-flight request coalescing (e.g., store and await a shared promise) before merging to de-risk peak behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared/lib/services/account.service.ts">
<violation number="1" location="packages/shared/lib/services/account.service.ts:662">
P2: Hot keys can stampede the database at cache expiry: concurrent misses all start the expensive lookup before any request reaches `set`. Track an in-flight lookup per hash (or cache a promise) so callers coalesce behind the first query.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| private async getAccountContextByInternalSecret(secretKey: string): Promise<AccountContext | null> { | ||
| const cacheMode = envs.AUTH_ACCOUNT_CONTEXT_CACHE_MODE; | ||
| if (cacheMode !== 'off') { | ||
| const cached = getCachedAccountContext({ hash: hashLocalCache.get(secretKey), mode: cacheMode }); |
There was a problem hiding this comment.
P2: Hot keys can stampede the database at cache expiry: concurrent misses all start the expensive lookup before any request reaches set. Track an in-flight lookup per hash (or cache a promise) so callers coalesce behind the first query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/account.service.ts, line 662:
<comment>Hot keys can stampede the database at cache expiry: concurrent misses all start the expensive lookup before any request reaches `set`. Track an in-flight lookup per hash (or cache a promise) so callers coalesce behind the first query.</comment>
<file context>
@@ -646,6 +657,14 @@ class AccountService {
private async getAccountContextByInternalSecret(secretKey: string): Promise<AccountContext | null> {
+ const cacheMode = envs.AUTH_ACCOUNT_CONTEXT_CACHE_MODE;
+ if (cacheMode !== 'off') {
+ const cached = getCachedAccountContext({ hash: hashLocalCache.get(secretKey), mode: cacheMode });
+ if (cached !== undefined && cacheMode === 'on') {
+ return cached;
</file context>
There was a problem hiding this comment.
Quantified during review: at expiry, concurrent misses during the few-ms DB roundtrip amount to ~1–2 duplicate lookups per key per TTL window per pod — against a baseline where every request runs the query today. Coalescing would also need to be mode-conditional (dry must keep querying per request to stay observe-only), which isn't worth the complexity at a 60s TTL. If dry-mode metrics surface hot keys where this matters, in-flight dedup is a contained follow-up — same pattern as inFlightRefreshes in refresh.ts.
There was a problem hiding this comment.
To make the worst case explicit: during a cache-miss stampede, an environment generates at most the same query traffic it generates today all the time. So the current approach adds no regression in terms of worst-case scenario — at most it leaves it at the same level, but contained at env level and to this specific expiry event.
There was a problem hiding this comment.
And if we ever want to work on the stampede-miss side effect, it's an easy follow-up: either guard rails to reduce the blast radius, or — even simpler, mimicking what Memcached does — a new attribute on the entry that forces a refresh for one single request when we're approaching the TTL, while everyone else keeps being served the still-fresh value. That removes the expiry miss burst entirely without any in-flight promise tracking.
Validate the cache TTL as a positive integer at env parsing and guard the TTLFixedSizeMap constructor against non-finite/negative TTLs and fractional sizes (FixedSizeMap now enforces its positive-integer contract). In dry mode, stop refreshing fresh entries so the measured hit ratio matches what 'on' would actually serve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Note
Drafted pending #6800 (light internal auth context query for persist). Once that merges, this PR will be reworked so the cache wraps
getInternalAuthContextand stores the narrow context (smaller entries, no secrets retained) instead of the full one.TTLFixedSizeMapin@nangohq/utils: bounded size (composesFixedSizeMap) plus TTL expiry on a monotonic clock, expired entries evicted on read.AUTH_ACCOUNT_CONTEXT_CACHE_MODEdefaults tooff;dryrecords hit/miss metrics (nango.auth.accountContextCache) while every lookup still goes to the DB;onserves cached hits.AUTH_ACCOUNT_CONTEXT_CACHE_TTL_MS(default 60s) bounds staleness.Staleness contract
With mode
on, everything in the cached context — rotated/revoked internal keys, revoked accounts, plan changes, account metadata — keeps resolving for up to the TTL on each pod. The TTL should stay very short while still effective. We expect a non-negligible hit ratio (misses scale with environments × pods per TTL window rather than with request rate), which will be measured while the cache runs indrymode before we serve from it. Only keys that previously passed a successful lookup can ever be served from cache.Test plan
TTLFixedSizeMap(TTL expiry, reset on rewrite, size-limit eviction, non-integer TTL)off— confirm no behavior change and nonango.auth.accountContextCachemetricdryin production — measure hit ratio for a dayon— confirm theapi_secretsjoin rate andnango-production-writerCPU drop🤖 Generated with Claude Code