From 13c006a58c3f3d00362a538ac30e308058d51816 Mon Sep 17 00:00:00 2001 From: FindMalek Date: Thu, 20 Aug 2026 18:22:46 +0100 Subject: [PATCH 1/2] fix(rpc): align funnel cache invalidation key and re-enable caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit invalidateFunnelsCache invalidated `byId::`, but the getById handler cached under `byId:` — the keys never matched, so invalidation silently deleted a Redis key that was never written. All three funnel read paths (list, getById, analyticsByLink) had caching disabled entirely to work around this, even though list and analyticsByLink already used a key/tag format the invalidation helper handles correctly. Fixes #634 --- packages/rpc/src/lib/funnels-cache.test.ts | 111 +++++++++++++++++++++ packages/rpc/src/lib/funnels-cache.ts | 2 +- packages/rpc/src/routers/funnels.ts | 3 - 3 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 packages/rpc/src/lib/funnels-cache.test.ts diff --git a/packages/rpc/src/lib/funnels-cache.test.ts b/packages/rpc/src/lib/funnels-cache.test.ts new file mode 100644 index 000000000..56bec8c13 --- /dev/null +++ b/packages/rpc/src/lib/funnels-cache.test.ts @@ -0,0 +1,111 @@ +import { beforeEach, describe, expect, it, mock } from "bun:test"; +import { createDrizzleCache } from "@databuddy/redis/drizzle-cache"; + +const kv = new Map(); +const sets = new Map>(); + +const redis = { + get: mock(async (key: string) => kv.get(key) ?? null), + setex: mock(async (key: string, _ttl: number, value: string) => { + kv.set(key, value); + return "OK" as const; + }), + sadd: mock(async (key: string, ...members: string[]) => { + const set = sets.get(key) ?? new Set(); + for (const member of members) { + set.add(member); + } + sets.set(key, set); + return members.length; + }), + smembers: mock(async (key: string) => Array.from(sets.get(key) ?? [])), + srem: mock(async (key: string, member: string) => { + sets.get(key)?.delete(member); + return 1; + }), + unlink: mock(async (...keys: string[]) => { + for (const key of keys) { + kv.delete(key); + sets.delete(key); + } + return keys.length; + }), + sunion: mock(async (...keys: string[]) => { + const out = new Set(); + for (const key of keys) { + for (const member of sets.get(key) ?? []) { + out.add(member); + } + } + return Array.from(out); + }), +}; + +mock.module("@databuddy/redis", () => ({ + createDrizzleCache, + invalidateAgentContextSnapshotsForWebsite: mock(async () => undefined), + redis, +})); + +const { funnelCache, invalidateFunnelsCache } = await import("./funnels-cache"); + +beforeEach(() => { + kv.clear(); + sets.clear(); +}); + +describe("invalidateFunnelsCache", () => { + it("invalidates the exact key a cached getById lookup is stored under", async () => { + const funnelId = "funnel-1"; + let calls = 0; + + const readFunnel = () => + funnelCache.withCache({ + key: `byId:${funnelId}`, + queryFn: async () => { + calls += 1; + return { id: funnelId, name: `v${calls}` }; + }, + tables: ["funnelDefinitions"], + ttl: 300, + }); + + expect(await readFunnel()).toEqual({ id: funnelId, name: "v1" }); + // Cache hit: queryFn must not run again. + expect(await readFunnel()).toEqual({ id: funnelId, name: "v1" }); + expect(calls).toBe(1); + + await invalidateFunnelsCache("website-1", funnelId); + + // Regression guard: invalidateFunnelsCache must target the same key + // format the router actually caches under (`byId:`), not a + // key that was never written (e.g. `byId::`). + expect(await readFunnel()).toEqual({ id: funnelId, name: "v2" }); + expect(calls).toBe(2); + }); + + it("invalidates the exact key a cached list lookup is stored under", async () => { + const websiteId = "website-2"; + let calls = 0; + + const readList = () => + funnelCache.withCache({ + key: `list:${websiteId}`, + queryFn: async () => { + calls += 1; + return [{ id: `funnel-${calls}` }]; + }, + tables: ["funnelDefinitions"], + ttl: 300, + }); + + await readList(); + await readList(); + expect(calls).toBe(1); + + await invalidateFunnelsCache(websiteId); + + await readList(); + expect(calls).toBe(2); + }); +}); diff --git a/packages/rpc/src/lib/funnels-cache.ts b/packages/rpc/src/lib/funnels-cache.ts index 83b9be36f..444d509e3 100644 --- a/packages/rpc/src/lib/funnels-cache.ts +++ b/packages/rpc/src/lib/funnels-cache.ts @@ -20,7 +20,7 @@ export async function invalidateFunnelsCache( ): Promise { const keys = [`list:${websiteId}`]; if (funnelId) { - keys.push(`byId:${funnelId}:${websiteId}`); + keys.push(`byId:${funnelId}`); } const operations: Promise[] = keys.map((key) => diff --git a/packages/rpc/src/routers/funnels.ts b/packages/rpc/src/routers/funnels.ts index 4ec5d0b64..fe8d4cd6e 100644 --- a/packages/rpc/src/routers/funnels.ts +++ b/packages/rpc/src/routers/funnels.ts @@ -184,7 +184,6 @@ export const funnelsRouter = { return cache.withCache({ key: `list:${input.websiteId}`, - disabled: true, // TODO: Remove this once we have a way to invalidate the cache ttl: CACHE_TTL, tables: ["funnelDefinitions"], queryFn: () => @@ -226,7 +225,6 @@ export const funnelsRouter = { .handler(({ context, input }) => cache.withCache({ key: `byId:${input.id}`, - disabled: true, // TODO: Remove this once we have a way to invalidate the cache ttl: CACHE_TTL, tables: ["funnelDefinitions"], queryFn: async () => { @@ -650,7 +648,6 @@ export const funnelsRouter = { return cache.withCache({ key: cacheKey, - disabled: true, // TODO: Remove this once we have a way to invalidate the cache ttl: ANALYTICS_CACHE_TTL, tables: ["funnelDefinitions"], tag: `funnel:${input.funnelId}`, From 9c1e36ef9d578ebc24e6da7af4aec1665b6430b9 Mon Sep 17 00:00:00 2001 From: FindMalek Date: Thu, 20 Aug 2026 18:31:00 +0100 Subject: [PATCH 2/2] fix(rpc): authorize funnel getById before the cache, not inside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flagged that re-enabling the getById cache let a workspace authorization check that lived inside queryFn get skipped on a cache hit: withCache returns the cached row directly without ever invoking queryFn, so a second caller without access to the funnel's website could read it once someone else had populated the cache. Resolve the owning websiteId and call withWorkspace before touching the cache, mirroring the same two-step pattern already used in update/delete in this file. Authorization now runs on every request regardless of cache state; only the row fetch itself is cached. Also add the new funnels-cache.test.ts to the package's configured test script (packages/rpc/package.json) — it was passing locally only because `bun test` with no arguments picks up every *.test.ts file, but the package's actual `test` script lists files explicitly and was silently skipping it. --- packages/rpc/package.json | 2 +- packages/rpc/src/routers/funnels.ts | 37 ++++++++++++++++++++++------- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/packages/rpc/package.json b/packages/rpc/package.json index 415a7be0c..e4f7d64cc 100644 --- a/packages/rpc/package.json +++ b/packages/rpc/package.json @@ -7,7 +7,7 @@ "types": "./src/index.ts", "scripts": { "check-types": "tsc --noEmit", - "test": "bun test src/routers src/lib/analytics-utils.integration.test.ts src/procedures/*.test.ts src/services/insight-schedule.test.ts src/services/uptime-lifecycle.test.ts src/services/uptime-scheduler.test.ts src/utils/*.test.ts", + "test": "bun test src/routers src/lib/analytics-utils.integration.test.ts src/lib/funnels-cache.test.ts src/procedures/*.test.ts src/services/insight-schedule.test.ts src/services/uptime-lifecycle.test.ts src/services/uptime-scheduler.test.ts src/utils/*.test.ts", "test:integration": "bun test src/services/uptime-scheduler.integration.test.ts" }, "exports": { diff --git a/packages/rpc/src/routers/funnels.ts b/packages/rpc/src/routers/funnels.ts index fe8d4cd6e..31702dcae 100644 --- a/packages/rpc/src/routers/funnels.ts +++ b/packages/rpc/src/routers/funnels.ts @@ -222,8 +222,32 @@ export const funnelsRouter = { }) .input(z.object({ id: z.string() })) .output(funnelOutputSchema) - .handler(({ context, input }) => - cache.withCache({ + .handler(async ({ context, input }) => { + // Resolve the owning website and authorize on every request, not just + // on a cache miss: `queryFn` below is skipped entirely on a cache hit, + // so any permission check placed inside it would be bypassed for + // anyone who requests an id already cached by another caller. + const [funnelRef] = await context.db + .select({ websiteId: funnelDefinitions.websiteId }) + .from(funnelDefinitions) + .where( + and( + eq(funnelDefinitions.id, input.id), + isNull(funnelDefinitions.deletedAt) + ) + ) + .limit(1); + + if (!funnelRef) { + throw rpcError.notFound("funnel", input.id); + } + + await withWorkspace(context, { + websiteId: funnelRef.websiteId, + permissions: ["read"], + }); + + return cache.withCache({ key: `byId:${input.id}`, ttl: CACHE_TTL, tables: ["funnelDefinitions"], @@ -243,15 +267,10 @@ export const funnelsRouter = { throw rpcError.notFound("funnel", input.id); } - await withWorkspace(context, { - websiteId: funnel.websiteId, - permissions: ["read"], - }); - return funnel; }, - }) - ), + }); + }), create: trackedProcedure .route({