From 7f1e865ecab0051f7a2ba58669da508c8b8009d9 Mon Sep 17 00:00:00 2001 From: Tim Hoare Date: Wed, 29 Apr 2026 12:27:46 +0100 Subject: [PATCH 1/2] 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", () => { From 0383c841f550756d5471330b80660433be43e8d4 Mon Sep 17 00:00:00 2001 From: TimHoare Date: Wed, 13 May 2026 09:41:55 +0100 Subject: [PATCH 2/2] refactor(cache): extract getValid helper for expiry-aware lookups Address review feedback on #326: collapse the duplicated 'storage.get + expiry check + delete-on-expired' pattern in get(), has(), and getOrExecute() into a single private getValid() helper to avoid future drift. Signed-off-by: TimHoare --- packages/appkit/src/cache/index.ts | 56 ++++++++++++++---------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/packages/appkit/src/cache/index.ts b/packages/appkit/src/cache/index.ts index b3235a6f9..873aada18 100644 --- a/packages/appkit/src/cache/index.ts +++ b/packages/appkit/src/cache/index.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; import { ApiError, WorkspaceClient } from "@databricks/sdk-experimental"; -import type { CacheConfig, CacheStorage } from "shared"; +import type { CacheConfig, CacheEntry, CacheStorage } from "shared"; import { createLakebasePool } from "../connectors/lakebase"; import { AppKitError, ExecutionError, InitializationError } from "../errors"; import { createLogger } from "../logging/logger"; @@ -202,28 +202,20 @@ export class CacheManager { }, async (span) => { try { - // check if the value is in the cache - const cached = await this.storage.get(cacheKey); + const cached = await this.getValid(cacheKey); if (cached !== null) { - // 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, - }); + 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; } // check if the value is being processed by another request @@ -315,6 +307,18 @@ export class CacheManager { // probabilistic cleanup trigger this.maybeCleanup(); + const entry = await this.getValid(key); + return entry?.value ?? null; + } + + /** + * Get a cached entry only if it has not expired. + * Returns null on miss or expired (and deletes the expired entry). + * + * Storage implementations return entries unconditionally — expiry handling + * lives at the CacheManager layer. + */ + private async getValid(key: string): Promise | null> { const entry = await this.storage.get(key); if (!entry) return null; @@ -322,7 +326,7 @@ export class CacheManager { await this.storage.delete(key); return null; } - return entry.value as T; + return entry; } /** Probabilistically trigger cleanup of expired entries (fire-and-forget) */ @@ -393,14 +397,8 @@ export class CacheManager { async has(key: string): Promise { if (!this.config.enabled) return false; - const entry = await this.storage.get(key); - if (!entry) return false; - - if (Date.now() > entry.expiry) { - await this.storage.delete(key); - return false; - } - return true; + const entry = await this.getValid(key); + return entry !== null; } /**