fix: use common cache for pgvector metrics - #1279
Conversation
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
Coverage Report for CI Build 30496756227Coverage increased (+0.02%) to 80.45%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — approving despite the one nit found (missing purgeStaleIntervalMs on the new pgvector metric cache), since it's cosmetic and doesn't affect correctness.
Extended reasoning...
Overview
This PR migrates the pgvector distance-metric cache from a bespoke @isaacs/ttlcache instance to the shared createLruCache wrapper used by the other production caches (tenant_config, tenant_jwks, tenant_s3_credentials, jwt), adding matching metrics wiring (names.ts, metrics.ts) and a new metric-cache.ts module. It also changes getOrLookupMetric to stop caching lookup failures (previously a failed lookup could poison the TTL cache with an implicit 'cosine' fallback baked in) while still falling back to 'cosine' on error, and primes/invalidates the cache on index create/delete.
Security risks
None. This is an internal, non-authoritative cache of a derived value (distance operator class) used only to select which pgvector operator (<-> vs <=>) to use in a query; no user input flows into cache keys beyond bucket/index names that are already scoped and validated elsewhere, and a cache miss/failure safely falls back to a valid default ('cosine') rather than failing open on any authorization or data-access check.
Level of scrutiny
Low-to-moderate. This is a well-contained, mechanical refactor onto an existing, already-vetted cache abstraction, with solid new test coverage (unit test for TTL renewal behavior, integration tests for cache hit/miss instrumentation and index-lifecycle priming/invalidation, all using a real pgvector-backed test DB). The only real behavior change (no longer caching failed lookups) is strictly safer than the old behavior. The one finding raised (missing purgeStaleIntervalMs, so the periodic stale-purge timer never arms for this specific cache) is a minor deviation from sibling caches with negligible practical impact, as detailed in the inline comment — the underlying lru-cache still treats TTL-expired entries as misses on access regardless of the timer, and the cache is small and capacity-bounded.
Other factors
Tests were expanded meaningfully alongside the change (new metric-cache unit test, new metrics.test.ts assertions for the new cache name, and two new/updated pgvector-adapter integration tests covering cache hit/miss metrics and priming/invalidation across create/delete). No CODEOWNER-restricted paths, no auth/crypto/permission logic touched.
| export function createMetricCache(options: MetricCacheOptions = {}) { | ||
| return createLruCache<string, DistanceMetric>(PGVECTOR_METRIC_CACHE_NAME, { | ||
| ttl: options.ttl ?? METRIC_CACHE_TTL_MS, | ||
| ttlResolution: options.ttlResolution, | ||
| max: METRIC_CACHE_MAX, | ||
| updateAgeOnGet: true, | ||
| perf: options.perf, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🟡 createMetricCache() (metric-cache.ts:14-22) omits purgeStaleIntervalMs, unlike every other createLruCache call in the codebase (tenant_config, tenant_jwks, tenant_s3_credentials, jwt), so the pgvector metric cache's periodic stale-entry purge timer never starts.
Extended reasoning...
createMetricCache() builds its LRU cache via createLruCache(PGVECTOR_METRIC_CACHE_NAME, {...}) without passing purgeStaleIntervalMs. Every other production cache built on the same wrapper — tenant_config (src/internal/database/tenant.ts:90), tenant_jwks (src/internal/jwks/manager.ts:26), tenant_s3_credentials (src/internal/s3/credentials/manager.ts:28), and jwt (src/internal/auth/jwt.ts:206) — explicitly passes purgeStaleIntervalMs: DEFAULT_CACHE_PURGE_STALE_INTERVAL_MS.
In src/internal/cache/lru.ts, LruCache's constructor only arms an internal setInterval(() => this.purgeStale(), purgeStaleIntervalMs) when purgeStaleIntervalMs is truthy (lru.ts:27-32). Since createMetricCache() leaves that field undefined, the pgvector_metric cache is the only one of the five that never proactively reclaims TTL-expired entries on a timer. This is a genuine deviation from the pattern this PR explicitly says it's modeled on ("Use common LRU cache wrapper with the same TTL").
That said, the practical impact is minimal, which is why this is a nit rather than a blocking issue:
get()correctness is unaffected — the underlyinglru-cachetreats TTL-expired entries as a miss on access, sogetOrLookupMetricstill re-resolves correctly regardless of whether the interval timer ran.- The cache is small and capacity-bounded (
max: 1_000, entries are justbucket\x00index -> 'cosine'|'euclidean'strings), so unreclaimed expired entries sitting in the map cannot grow unbounded. - Whenever the
cache_entriesgauge is actually collected,MonitoredCache.observeOccupancy()(monitoring.ts:52) callsoptions.purgeStale()before readinggetStats(), andcreateLruCachewires that callback (lru.ts:87-89) — so the gauge itself stays accurate at collection time even without the periodic timer.
Concretely: without purgeStaleIntervalMs, if metrics collection is inactive (no OTel MeterProvider/periodic reader scraping cache_entries), an entry that goes stale after 5 minutes just sits in the underlying lru-cache Map — untouched — until either it's evicted for capacity (unlikely at 1000 tiny entries) or something calls purgeStale() (via a metrics scrape) or get() is called on that exact key again (returns a miss, re-resolves, and overwrites the stale entry). So the only residual effect is slightly stale internal bookkeeping between metrics scrapes, not incorrect query behavior.
Fix is a one-line addition — pass purgeStaleIntervalMs: options.purgeStaleIntervalMs ?? DEFAULT_CACHE_PURGE_STALE_INTERVAL_MS (or just the constant directly) alongside the other options in createMetricCache() — to keep this cache consistent with its siblings.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
pgvector metric cache is using a TTL cache but we're trying to drop it.
It caches lookup failures at the moment.
What is the new behavior?
Use common LRU cache wrapper with the same TTL, we can drop age update separately for all caches.
Don't cache metric lookup failure but still fallback to
cosine.Additional context
This enables to drop TTL dependency after pool cache is refactored.