fix(rpc): align funnel cache invalidation key and re-enable caching - #636
fix(rpc): align funnel cache invalidation key and re-enable caching#636FindMalek wants to merge 3 commits into
Conversation
invalidateFunnelsCache invalidated `byId:<funnelId>:<websiteId>`, but the getById handler cached under `byId:<funnelId>` — 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 databuddy-analytics#634
|
@FindMalek is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR aligns funnel cache invalidation keys and safely re-enables caching while ensuring workspace authorization runs before cached
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported issues are fixed by authorizing before cache access and selecting the regression test in the standard RPC test task. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant RPC as Funnel RPC
participant DB
participant Auth as Workspace Authorization
participant Cache
Caller->>RPC: getById(funnelId)
RPC->>DB: Resolve active funnel websiteId
DB-->>RPC: websiteId
RPC->>Auth: Check read permission
Auth-->>RPC: Authorized
RPC->>Cache: Read byId:funnelId
alt Cache hit
Cache-->>RPC: Cached funnel
else Cache miss
RPC->>DB: Load funnel
DB-->>RPC: Funnel
RPC->>Cache: Store funnel
end
RPC-->>Caller: Funnel
Reviews (3): Last reviewed commit: "fix(rpc): authorize funnel getById befor..." | Re-trigger Greptile |
| @@ -226,7 +225,6 @@ export const funnelsRouter = { | |||
| .handler(({ context, input }) => | |||
| cache.withCache({ | |||
There was a problem hiding this comment.
Cache hits bypass workspace authorization
When an authenticated principal requests a funnel previously cached by an authorized caller, withCache returns the shared byId:<funnelId> value without running the queryFn, so the only withWorkspace check is skipped and another tenant's complete funnel definition is disclosed.
How this was verified: The shared cache returns hits before invoking the query function that contains the route's only workspace permission check.
Knowledge Base Used: RPC package (@databuddy/rpc)
| @@ -0,0 +1,111 @@ | |||
| import { beforeEach, describe, expect, it, mock } from "bun:test"; | |||
There was a problem hiding this comment.
Regression test is not selected
The RPC package's configured test command does not select src/lib/funnels-cache.test.ts or the complete src/lib directory, so this regression coverage is skipped by the standard package and CI test task.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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.
|
Good catch — fixed in 9c1e36e.
Also fixed: |
|
@greptile review |
|
@izadoesdev this is ready for review whenever you have a chance — CI is green aside from the Vercel preview checks, which need a team member to authorize the deploy (outside my permissions as an external contributor). |
Fixes #634
Summary
All three funnel read paths in
packages/rpc/src/routers/funnels.ts(list,getById,analyticsByLink) had caching explicitly disabled viadisabled: true, // TODO: Remove this once we have a way to invalidate the cache. That TODO was stale —invalidateFunnelsCachealready exists and is already called on create/update/delete — but re-enabling it blindly would have been wrong:getByIdcached underbyId:${id}whileinvalidateFunnelsCacheinvalidatedbyId:${funnelId}:${websiteId}, so the two never matched and invalidation silently deleted a key that was never written.Approach
packages/rpc/src/lib/funnels-cache.ts: changed the invalidation key frombyId:${funnelId}:${websiteId}tobyId:${funnelId}, matching whatgetByIdactually caches under (websiteId isn't known at read time — it's resolved from the row after the query runs).packages/rpc/src/routers/funnels.ts: removeddisabled: truefrom all three read paths now that the key mismatch is fixed.list's key already matched the invalidation format, andanalyticsByLinkis tag-based (funnel:${funnelId}) and already matched too — onlygetByIdhad the actual bug.packages/rpc/src/lib/funnels-cache.test.ts: caches a read, invalidates, and asserts the next read re-queries instead of returning the stale value. This test fails against the pre-fix code (confirmed locally by reverting the fix) and passes after it.Verification
Full monorepo
turbo run check-typesandturbo run testalso ran clean via the repo's pre-commit/pre-push hooks.Disclosure
I used Claude Code to trace the key mismatch (reading
packages/redis/drizzle-cache.ts'sinvalidateByKeyto confirm key formats must match exactly) and to write the regression test. I read every changed line, ran the fix locally, verified the test fails on the original code and passes on the fix, and ran the full test suite and type-check myself before opening this.Summary by cubic
Aligns funnel cache invalidation keys with writes and re-enables caching for funnel reads, while enforcing authorization on every request. Previously,
getByIdcached underbyId:<funnelId>but invalidation targetedbyId:<funnelId>:<websiteId>, and an auth check inside the cached query was skipped on cache hits.byId:<funnelId>andlist:<websiteId>.list,getById, andanalyticsByLink; resolve the owningwebsiteIdand callwithWorkspacebefore using the cache so authorization always runs; keepanalyticsByLinktagged asfunnel:<funnelId>.getByIdandlist; include the test inpackages/rpc/package.json’stestscript.Written for commit 17a9908. Summary will update on new commits.