use refreshThreshold in cache-manager

[danoswaltCL]

No description provided.

[Copilot]

Pull request overview

This PR extends the backend CacheService to pass a refresh threshold into cache-manager’s wrap, enabling background refresh so callers can be served a cached value while a refresh happens asynchronously.

Changes:

• Add refresh-threshold support in CacheService.wrap() (including interval→threshold conversion) and hook onBackgroundRefreshError for logging.
• Expand unit tests to validate threshold computation and add an integration-style test against the real memory store behavior.
• Introduce a new caching-related env var in env.ts / .env.example, and update .env.test’s DB port.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/backend/src/api/services/CacheService.ts	Passes refresh threshold into cache.wrap() and adds background-refresh error logging.
packages/backend/src/env.ts	Adds parsing for a caching refresh setting from environment variables.
packages/backend/test/unit/services/CacheService.test.ts	Updates wrap assertions and adds coverage for refresh threshold behavior (mock + real store).
packages/backend/.env.example	Documents the new caching refresh configuration.
packages/backend/.env.test	Changes the DB port used for tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

packages/backend/test/unit/services/CacheService.test.ts:335

• These tests rely on a fixed 50ms sleep for the background refresh to complete. On slower CI or under load this can be flaky (refresh may not finish in time). Increase the buffer or poll until the refreshed value is observed.

      // The refresh landed and reset the TTL, so the entry never expired out from under callers
      await sleep(50);
      expect(await realService.wrap(key, fetch)).toBe('v2');
      expect(fetch).toHaveBeenCalledTimes(2);

packages/backend/src/api/services/CacheService.ts:61

• refreshThreshold is treated as a (seconds) interval and used in arithmetic; if it is misconfigured as a negative number, refreshThresholdMsForTtl can return a value greater than the TTL, which can trigger immediate/constant background refresh. Clamp it to >= 0 when reading from env.

  // How often (seconds) a `wrap` key is re-read from source while it keeps getting traffic. Once an
  // entry reaches this age, the next hit returns the cached value immediately and refreshes the
  // entry in the background, so no request pays for the refill. This — not the TTL — is what bounds
  // staleness; the TTL is only the hard expiry for a key that stops being read. 0 (the default)
  // disables background refresh, leaving plain TTL expiry.
  private refreshThreshold = env.caching.refreshThreshold || 0;
  private initPromise: Promise<void>;

packages/backend/src/env.ts:119

• CACHING_REFRESH_THRESHOLD is parsed via toNumber, which uses parseInt and truncates decimals (e.g., 0.5 -> 0). That breaks sub-second refresh thresholds and would silently disable refresh when a fractional value is configured. Parse this env var with Number(...) (or parseFloat) instead.

    ttlExperiments: toNumber(getOsEnvOptional('CACHING_TTL_EXPERIMENTS')),
    ttlFeatureFlags: toNumber(getOsEnvOptional('CACHING_TTL_FEATURE_FLAGS')),
    ttlSegments: toNumber(getOsEnvOptional('CACHING_TTL_SEGMENTS')),
    refreshThreshold: toNumber(getOsEnvOptional('CACHING_REFRESH_THRESHOLD')),
  },

packages/backend/test/unit/services/CacheService.test.ts:277

• This test mutates mockedEnv.caching.ttlExperiments and restores it at the end of the test body. If an assertion throws before the restore line, later tests will run with the wrong TTL value (test pollution). Restore it in a finally block (or in afterEach).

This issue also appears on line 332 of the same file.

    it('allows an interval far below half the TTL — the case the old cap blocked', async () => {
      const fn = jest.fn().mockResolvedValue('v');
      mockedEnv.caching.ttlExperiments = 3600; // 1 hour
      const service = serviceWithInterval(45); // refresh every 45s
      await service.wrap(CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + 'app', fn);
      // 3600s - 45s: a long hard expiry with a short staleness bound
      expect(mockStore.wrap).toHaveBeenCalledWith(CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + 'app', fn, 3600000, 3555000);
      mockedEnv.caching.ttlExperiments = 30;

[danoswaltCL]

@bcb37 Found a bug, I forgot that "precomputed segments" does use the terrible "wrapFunction" method by passing in the list of "valid" experiment or flag ids.

The "wrapFunction" was already bad because it is all-or-nothing, as-in it re-refetches everything in the supplied id list if a single id on that list isn't cached currently.

It also never records when we asked the db for a record and the db said nothing exists, so it will continue to treat that id as a cache-miss even though we confirmed already that there's nothing there to find.

Now that we want to use refreshThreshold, we have to overhaul this function anyway, because it doesn't actually use cache.wrap, which is the special sauce that allows cache-manager to manage refreshing the key the way we want.

So, I rewrote it so that it uses cache.wrap and actually handles each key individually, including recording when a key is confirmed to not exist in the db.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (4)

packages/backend/src/api/controllers/ExperimentController.ts:2002

• includeData is used as a truthiness check when deciding whether to attach cached values. If it arrives as the string "false", this will still include data and can unintentionally return megabytes of cached payloads. Use the same explicit boolean parse as above when gating data inclusion.

              key,
              expiresInSeconds: remainingTtlMs === null ? null : Math.round(remainingTtlMs / 1000),
              ...(includeData ? { data: await this.cacheService.getCache(key) } : {}),
            };

packages/backend/src/env.ts:119

• CACHING_REFRESH_THRESHOLD is parsed with toNumber(), which uses parseInt(). That truncates fractional seconds (e.g. "0.5" becomes 0) and makes it hard to use sub-second thresholds (the unit tests use 0.5s). Consider parsing as a float here so the env var matches the behavior CacheService supports.

    refreshThreshold: toNumber(getOsEnvOptional('CACHING_REFRESH_THRESHOLD')),

packages/backend/src/api/services/CacheService.ts:63

• refreshThreshold can become negative (e.g. env misconfiguration) and then refreshThresholdMsForTtl() will produce a threshold larger than the TTL (ttlMs - negative), which can cause immediate/continuous refresh behavior. Clamp refreshThreshold to a non-negative number at initialization to avoid surprising cache behavior from bad env values.

  // How often (seconds) a `wrap` key is re-read from source while it keeps getting traffic. Once an
  // entry reaches this age, the next hit returns the cached value immediately and refreshes the
  // entry in the background, so no request pays for the refill. This — not the TTL — is what bounds
  // staleness; the TTL is only the hard expiry for a key that stops being read. 0 (the default)
  // disables background refresh, leaving plain TTL expiry.
  private refreshThreshold = env.caching.refreshThreshold || 0;
  private initPromise: Promise<void>;

packages/backend/src/api/controllers/ExperimentController.ts:1977

• QueryParam values may arrive as strings (e.g. ?keys=false => "false"). Using !!includeKeys / !!includeData makes "false" truthy, so callers can’t reliably turn these flags off and may accidentally request a large payload. Parse these flags explicitly as booleans before computing withKeys.

This issue also appears on line 1999 of the same file.

  async debugCache(
    @QueryParam('keys') includeKeys?: boolean,
    @QueryParam('data') includeData?: boolean,
    @QueryParam('prefix') prefixFilter?: string
  ): Promise<any> {
    const names = Object.keys(CACHE_PREFIX);
    if (prefixFilter && !names.includes(prefixFilter)) {
      throw new BadRequestError(`unknown prefix '${prefixFilter}'. Expected one of: ${names.join(', ')}`);
    }
    const withKeys = !!includeKeys || !!includeData;