diff --git a/packages/backend/.env.example b/packages/backend/.env.example index bbd7260a3..24fbfae4c 100644 --- a/packages/backend/.env.example +++ b/packages/backend/.env.example @@ -15,6 +15,7 @@ CACHING_MAX_KEYS=500 CACHING_TTL_EXPERIMENTS= CACHING_TTL_FEATURE_FLAGS= CACHING_TTL_SEGMENTS= +CACHING_TTL_SETTINGS= USE_NEW_RELIC=false CORS_WHITELIST=localhost diff --git a/packages/backend/src/api/services/CacheService.ts b/packages/backend/src/api/services/CacheService.ts index 192c19711..1fcfee306 100644 --- a/packages/backend/src/api/services/CacheService.ts +++ b/packages/backend/src/api/services/CacheService.ts @@ -4,7 +4,7 @@ import { Cache, Store, caching } from 'cache-manager'; import { CACHE_PREFIX } from 'upgrade_types'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; -type CacheBucket = 'experiments' | 'featureFlags' | 'segments'; +type CacheBucket = 'experiments' | 'featureFlags' | 'segments' | 'settings'; const PREFIX_CATEGORY: Record = { [CACHE_PREFIX.EXPERIMENT_KEY_PREFIX]: 'experiments', @@ -14,6 +14,7 @@ const PREFIX_CATEGORY: Record = { [CACHE_PREFIX.GLOBAL_EXCLUDE_SEGMENT_KEY_PREFIX]: 'segments', [CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX]: 'featureFlags', [CACHE_PREFIX.EXPERIMENT_PRECOMPUTED_SEGMENT_KEY_PREFIX]: 'experiments', + [CACHE_PREFIX.SETTING_KEY_PREFIX]: 'settings', }; // this module will get swapped in if caching is enabled but the cache manager fails to initialize as a dummy default deliverer @@ -51,6 +52,7 @@ export class CacheService { experiments: env.caching.ttlExperiments || this.defaultTtl, featureFlags: env.caching.ttlFeatureFlags || this.defaultTtl, segments: env.caching.ttlSegments || this.defaultTtl, + settings: env.caching.ttlSettings || this.defaultTtl, }; private initPromise: Promise; diff --git a/packages/backend/src/api/services/SettingService.ts b/packages/backend/src/api/services/SettingService.ts index 52e7999d6..d5f544144 100644 --- a/packages/backend/src/api/services/SettingService.ts +++ b/packages/backend/src/api/services/SettingService.ts @@ -3,10 +3,15 @@ import { InjectRepository } from '../../typeorm-typedi-extensions'; import { SettingRepository } from '../repositories/SettingRepository'; import { Setting } from '../models/Setting'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import { CacheService } from './CacheService'; +import { CACHE_PREFIX } from 'upgrade_types'; + +// The settings row is global — one row, no per-entity key. +const SETTING_CACHE_KEY = CACHE_PREFIX.SETTING_KEY_PREFIX + 'clientCheck'; @Service() export class SettingService { - constructor(@InjectRepository() private settingRepository: SettingRepository) {} + constructor(@InjectRepository() private settingRepository: SettingRepository, private cacheService: CacheService) {} public async setClientCheck( checkAuth: boolean | null, @@ -20,20 +25,38 @@ export class SettingService { toCheckAuth: checkAuth === undefined ? (settingDoc && settingDoc.toCheckAuth) || false : checkAuth, toFilterMetric: filterMetric === undefined ? (settingDoc && settingDoc.toFilterMetric) || false : filterMetric, }; - return this.settingRepository.save(newDoc); + const saved = await this.settingRepository.save(newDoc); + // Invalidate after the write lands so the next read cannot repopulate the cache from stale state. + await this.cacheService.delCache(SETTING_CACHE_KEY); + return saved; } + /** + * Reads the global client-check settings. + * + * Cached because this sits on the hot path of every client request: ClientLibMiddleware calls it on + * every v6 endpoint, so a 7-call user journey paid seven round trips for two booleans. The query + * itself is trivial (a 1-row seq scan, ~0.005ms server-side) — the cost was the round trip and the + * connection-pool checkout, both of which the cache removes entirely. + * + * Invalidated by setClientCheck. The TTL is the backstop for multi-instance deployments, where a + * write served by one instance does not invalidate the others' caches. + * + * The cached value is a shared reference — callers must treat it as read-only. + */ public async getClientCheck(logger?: UpgradeLogger): Promise { if (logger) { logger.info({ message: 'Get project setting' }); } - const setting = await this.settingRepository.find(); - if (setting.length === 0) { - const defaultSetting = new Setting(); - defaultSetting.toCheckAuth = false; - defaultSetting.toFilterMetric = false; - return defaultSetting; - } - return setting[0]; + return this.cacheService.wrap(SETTING_CACHE_KEY, async () => { + const setting = await this.settingRepository.find(); + if (setting.length === 0) { + const defaultSetting = new Setting(); + defaultSetting.toCheckAuth = false; + defaultSetting.toFilterMetric = false; + return defaultSetting; + } + return setting[0]; + }); } } diff --git a/packages/backend/src/env.ts b/packages/backend/src/env.ts index 9a62584b7..99b130e4c 100644 --- a/packages/backend/src/env.ts +++ b/packages/backend/src/env.ts @@ -115,6 +115,7 @@ export const env = { ttlExperiments: toNumber(getOsEnvOptional('CACHING_TTL_EXPERIMENTS')), ttlFeatureFlags: toNumber(getOsEnvOptional('CACHING_TTL_FEATURE_FLAGS')), ttlSegments: toNumber(getOsEnvOptional('CACHING_TTL_SEGMENTS')), + ttlSettings: toNumber(getOsEnvOptional('CACHING_TTL_SETTINGS')), }, clientApi: { secret: getOsEnv('CLIENT_API_SECRET'), diff --git a/packages/backend/test/unit/services/MetricService.test.ts b/packages/backend/test/unit/services/MetricService.test.ts index 0108ad872..d0c354050 100644 --- a/packages/backend/test/unit/services/MetricService.test.ts +++ b/packages/backend/test/unit/services/MetricService.test.ts @@ -7,6 +7,7 @@ import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { SettingService } from '../../../src/api/services/SettingService'; import { MetricRepository } from '../../../src/api/repositories/MetricRepository'; import { SettingRepository } from '../../../src/api/repositories/SettingRepository'; +import { CacheService } from '../../../src/api/services/CacheService'; import { configureLogger } from '../../utils/logger'; describe('Audit Service Testing', () => { @@ -90,6 +91,16 @@ describe('Audit Service Testing', () => { find: jest.fn().mockResolvedValue(settingRes), }, }, + { + // Pass-through, mirroring CacheService's behaviour when CACHING_ENABLED is false. These + // tests swap the setting repository mock between assertions, so a caching stub would + // change what they exercise. + provide: CacheService, + useValue: { + wrap: jest.fn((_key: string, fn: () => Promise) => fn()), + delCache: jest.fn().mockResolvedValue(undefined), + }, + }, ], }).compile(); diff --git a/packages/backend/test/unit/services/SettingService.test.ts b/packages/backend/test/unit/services/SettingService.test.ts index a54046268..e6a454844 100644 --- a/packages/backend/test/unit/services/SettingService.test.ts +++ b/packages/backend/test/unit/services/SettingService.test.ts @@ -6,6 +6,7 @@ import { SettingRepository } from '../../../src/api/repositories/SettingReposito import { Setting } from '../../../src/api/models/Setting'; import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { configureLogger } from '../../utils/logger'; +import { CacheService } from '../../../src/api/services/CacheService'; const setting = new Setting(); const settingArr = [setting]; @@ -15,12 +16,29 @@ describe('Setting Service Testing', () => { let service: SettingService; let repo: Repository; let module: TestingModule; + let cache: Map; + let cacheService: { wrap: jest.Mock; delCache: jest.Mock }; beforeAll(() => { configureLogger(); }); beforeEach(async () => { + // Minimal stand-in for the real cache so the wrap/invalidate contract is exercised rather than + // stubbed away — a no-op mock would let a missing delCache in setClientCheck pass unnoticed. + cache = new Map(); + cacheService = { + wrap: jest.fn(async (key: string, fn: () => Promise) => { + if (!cache.has(key)) { + cache.set(key, await fn()); + } + return cache.get(key); + }), + delCache: jest.fn(async (key: string) => { + cache.delete(key); + }), + }; + module = await Test.createTestingModule({ providers: [ SettingService, @@ -33,6 +51,7 @@ describe('Setting Service Testing', () => { find: jest.fn().mockResolvedValue(settingArr), }, }, + { provide: CacheService, useValue: cacheService }, ], }).compile(); @@ -76,4 +95,43 @@ describe('Setting Service Testing', () => { defaultSetting.toFilterMetric = false; expect(setting).toEqual(defaultSetting); }); + + describe('getClientCheck caching', () => { + it('should hit the repository once across repeated reads', async () => { + await service.getClientCheck(logger); + await service.getClientCheck(logger); + await service.getClientCheck(logger); + + expect(repo.find).toHaveBeenCalledTimes(1); + }); + + it('should re-read from the repository after setClientCheck invalidates', async () => { + await service.getClientCheck(logger); + expect(repo.find).toHaveBeenCalledTimes(1); + + await service.setClientCheck(true, true, logger); + expect(cacheService.delCache).toHaveBeenCalled(); + + // setClientCheck itself reads once; the point is that the *next* read is not served stale. + const callsAfterWrite = (repo.find as jest.Mock).mock.calls.length; + await service.getClientCheck(logger); + expect((repo.find as jest.Mock).mock.calls.length).toBe(callsAfterWrite + 1); + }); + + it('should not serve a stale value once the cache is invalidated', async () => { + const before = await service.getClientCheck(logger); + expect(before.toCheckAuth).toBeFalsy(); + + const updated = new Setting(); + updated.toCheckAuth = true; + repo.find = jest.fn().mockResolvedValue([updated]); + + // Still cached, so the old value is expected here. + expect((await service.getClientCheck(logger)).toCheckAuth).toBeFalsy(); + + await service.setClientCheck(true, false, logger); + + expect((await service.getClientCheck(logger)).toCheckAuth).toBe(true); + }); + }); }); diff --git a/packages/types/src/Experiment/enums.ts b/packages/types/src/Experiment/enums.ts index 0761ab5ea..db0c5d80d 100644 --- a/packages/types/src/Experiment/enums.ts +++ b/packages/types/src/Experiment/enums.ts @@ -342,6 +342,7 @@ export enum CACHE_PREFIX { FEATURE_FLAG_KEY_PREFIX = 'featureFlags-', FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX = 'featureFlagPrecomputedSegments-', EXPERIMENT_PRECOMPUTED_SEGMENT_KEY_PREFIX = 'experimentPrecomputedSegments-', + SETTING_KEY_PREFIX = 'setting-', } export enum STATUS_INDICATOR_CHIP_TYPE {