From 7f1e865ecab0051f7a2ba58669da508c8b8009d9 Mon Sep 17 00:00:00 2001 From: Tim Hoare Date: Wed, 29 Apr 2026 12:27:46 +0100 Subject: [PATCH] fix(appkit): honor TTL in CacheManager.getOrExecute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CacheManager.getOrExecute calls storage.get() and returns the cached value without checking expiry. InMemoryStorage.get() returns entries unconditionally by design ("expiry check is done by CacheManager"), but the check was missing on the getOrExecute path — only the standalone CacheManager.get() honored TTL. With the in-memory backend (the default when Lakebase is not configured), this caused cached entries to live until LRU eviction at maxSize or process restart, regardless of the configured ttl. Apps using the analytics plugin's default cache (enabled, ttl: 3600) would serve stale query results indefinitely. Add an explicit expiry check in getOrExecute: if storage returns an expired entry, delete it and fall through to a normal cache miss so fn() re-executes. Adds a regression test in the getOrExecute describe block. The existing TTL test only covered the set/get pair, which is why this was not caught. Fixes #325 Signed-off-by: Tim Hoare --- packages/appkit/src/cache/index.ts | 27 ++++++++++++------- .../src/cache/tests/cache-manager.test.ts | 24 +++++++++++++++++ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/packages/appkit/src/cache/index.ts b/packages/appkit/src/cache/index.ts index 37bd1659e..b3235a6f9 100644 --- a/packages/appkit/src/cache/index.ts +++ b/packages/appkit/src/cache/index.ts @@ -205,18 +205,25 @@ export class CacheManager { // check if the value is in the cache const cached = await this.storage.get(cacheKey); if (cached !== null) { - span.setAttribute("cache.hit", true); - span.setStatus({ code: SpanStatusCode.OK }); - this.telemetryMetrics.cacheHitCount.add(1, { - "cache.key": cacheKey, - }); + // Storage returns entries unconditionally; expiry check is the + // CacheManager's responsibility. If the entry has expired, + // delete it and treat as a miss so fn() re-executes. + if (Date.now() > cached.expiry) { + await this.storage.delete(cacheKey); + } else { + span.setAttribute("cache.hit", true); + span.setStatus({ code: SpanStatusCode.OK }); + this.telemetryMetrics.cacheHitCount.add(1, { + "cache.key": cacheKey, + }); - logger.event()?.setExecution({ - cache_hit: true, - cache_key: cacheKey, - }); + logger.event()?.setExecution({ + cache_hit: true, + cache_key: cacheKey, + }); - return cached.value as T; + return cached.value as T; + } } // check if the value is being processed by another request diff --git a/packages/appkit/src/cache/tests/cache-manager.test.ts b/packages/appkit/src/cache/tests/cache-manager.test.ts index dcf94f097..3916ef1eb 100644 --- a/packages/appkit/src/cache/tests/cache-manager.test.ts +++ b/packages/appkit/src/cache/tests/cache-manager.test.ts @@ -354,6 +354,30 @@ describe("CacheManager", () => { expect(result1).toBe("user1-data"); expect(result2).toBe("user2-data"); }); + + test("should re-execute function when cached entry has expired", async () => { + const cache = await CacheManager.getInstance({ + storage: createMockStorage(), + }); + let calls = 0; + const fn = vi.fn().mockImplementation(async () => `result-${++calls}`); + + // First call - populates cache with a 1ms TTL + const r1 = await cache.getOrExecute(["key"], fn, "user1", { + ttl: 0.001, + }); + expect(r1).toBe("result-1"); + + // Wait for expiry + await new Promise((resolve) => setTimeout(resolve, 10)); + + // Second call - cached entry is past its expiry, fn must run again + const r2 = await cache.getOrExecute(["key"], fn, "user1", { + ttl: 0.001, + }); + expect(r2).toBe("result-2"); + expect(fn).toHaveBeenCalledTimes(2); + }); }); describe("disabled cache", () => {