feat(middleware-cache): improve caching strategies & cache behavior#859
Open
paulwer wants to merge 1 commit into
Open
feat(middleware-cache): improve caching strategies & cache behavior#859paulwer wants to merge 1 commit into
paulwer wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds write-only ChangesCache middleware invalidation and strategy overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-extensions/middleware-cache/README.md`:
- Around line 324-333: The documented CacheStore interface in the README is
outdated and does not match the actual implementation in src/cache-store.ts.
Update the CacheStore interface in the README to include the missing optional
delByTag method and update the incr method signature to include the optional
delta parameter as it exists in the actual implementation file. Ensure the
interface documentation now accurately mirrors the real contract to prevent
incorrect backend implementations.
- Around line 81-83: The README documentation contains a contradiction where the
comment on the unannotated query example (around line 81-83) states that
unannotated queries always hit the DB, but the configuration setup earlier in
the README (lines 56 and 59) enables readCaching and defaultTtlMs globally,
making those unannotated queries cache-eligible. Resolve this contradiction by
either disabling the global caching settings (readCaching and defaultTtlMs) in
the configuration example if the intent is to show uncached behavior, or update
the comment on the unannotated query to acknowledge that it will be cached due
to the global settings enabled earlier in the guide.
In `@packages/3-extensions/middleware-cache/src/cache-middleware.ts`:
- Around line 819-823: The getModelGeneration function uses
store.get(generationKey(model)) to read generation state when incr is available,
but the CacheStore contract does not guarantee that get() will expose the
counter state managed by incr(), leading to potential stale data. Fix this by
using the incr() method directly to read the generation counter value instead of
relying on get(), ensuring generation reads are consistent with how incr()
manages the counter state. This applies to both the main getModelGeneration
function and the related code block at lines 827-834.
- Around line 279-287: The asRecord and getAst functions use bare `as` type
casts which violate the repository cast policy. Replace the bare casts in these
functions with the approved cast helpers from `@prisma-next/utils/casts`.
Specifically, in the asRecord function, replace the `as Record<string, unknown>`
cast, and in the getAst function, replace the `as unknown as Record<string,
unknown>` cast pattern with either `blindCast<T, "Reason">` or `castAs<T>`
depending on the casting scenario, ensuring you provide clear reasoning for why
the cast is necessary.
- Around line 632-637: The first condition in the cache-middleware.ts file
checks if payload.uncache is defined AND has length > 0 before returning it,
which causes an explicitly passed empty array uncache: [] to be ignored and fall
back to implicit invalidation via the enabled or uncacheOnMutation logic. Remove
the length > 0 check from the first condition so that any explicitly defined
payload.uncache (whether empty or populated) is respected as an override and
takes precedence over the implicit invalidation behavior in the second condition
that checks payload.enabled or uncacheOnMutation.
In `@packages/3-extensions/middleware-cache/src/cache-store.ts`:
- Around line 235-240: When removing expired entries from the map in the cleanup
loop around lines 236-239, you must also remove the associated tag index entries
for that key to prevent stale tag references. Additionally, ensure that the
delByTag method around lines 296-309 either verifies that the key still exists
in the map before deletion or performs tag cleanup atomically with map cleanup.
This prevents scenarios where an expired key is reused with different tags and
the old tag index still points to it, causing the wrong live key to be deleted.
In `@packages/3-extensions/middleware-cache/test/cache-annotation.test.ts`:
- Around line 51-59: The test function "preserves all CachePayload fields
(enabled, ttl, skip, key, namespace, dedupe)" claims to cover all CachePayload
fields but is missing `tags` and `store` fields in the payload object
definition. Add both `tags` and `store` fields to the CachePayload object being
tested, assign appropriate test values to each, and update the test name to
reflect that all fields including `tags` and `store` are now included in the
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f0c80a2b-796f-430d-b340-85aaf1ddb414
⛔ Files ignored due to path filters (1)
projects/middleware-cache-strategies/spec.mdis excluded by!projects/**
📒 Files selected for processing (14)
packages/3-extensions/middleware-cache/README.mdpackages/3-extensions/middleware-cache/src/cache-annotation.tspackages/3-extensions/middleware-cache/src/cache-middleware.tspackages/3-extensions/middleware-cache/src/cache-store.tspackages/3-extensions/middleware-cache/src/exports/index.tspackages/3-extensions/middleware-cache/src/uncache-annotation.tspackages/3-extensions/middleware-cache/test/cache-annotation.test.tspackages/3-extensions/middleware-cache/test/cache-annotation.types.test-d.tspackages/3-extensions/middleware-cache/test/cache-key.test.tspackages/3-extensions/middleware-cache/test/cache-middleware.test.tspackages/3-extensions/middleware-cache/test/cache-middleware.types.test-d.tspackages/3-extensions/middleware-cache/test/cache-store.test.tspackages/3-extensions/middleware-cache/test/uncache-annotation.test.tspackages/3-extensions/middleware-cache/test/uncache-annotation.types.test-d.ts
paulwer
force-pushed
the
feat-cache-strategies
branch
from
042a405 to
24dee91
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-extensions/middleware-cache/README.md`:
- Around line 310-313: Replace the blocking KEYS command in the list method with
a SCAN-based iterator approach. The current implementation uses
redisClient.keys() which is O(N) and blocking, causing server stalls on large
keyspaces. Refactor the list method to use redisClient.scan() or an equivalent
SCAN iterator that returns results incrementally and non-blockingly, maintaining
the same prefix filtering logic if a prefix parameter is provided.
In `@packages/3-extensions/middleware-cache/src/cache-middleware.ts`:
- Around line 1023-1025: The namespace filtering logic at the condition checking
`!key.startsWith(\`${namespace}:\`)` (at line 1023 and again at line 1063) only
matches regular cache keys but excludes internal index keys, leaving stale index
metadata behind. Create a helper function named `isKeyInNamespace` that accepts
a key and namespace parameter, then checks both if the key directly starts with
the namespace prefix and also if it is an index key that contains an embedded
reference to something in that namespace (by parsing it with the existing
parseIndexedCacheKey helpers). Replace the `!key.startsWith(\`${namespace}:\`)`
check at both line 1023 and line 1063 with a call to this new helper to ensure
index entries are properly cleaned up during namespaced invalidations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 672cdd76-1a68-4c07-8689-30818636ec76
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/middleware-cache-strategies/spec.mdis excluded by!projects/**
📒 Files selected for processing (15)
packages/3-extensions/middleware-cache/README.mdpackages/3-extensions/middleware-cache/package.jsonpackages/3-extensions/middleware-cache/src/cache-annotation.tspackages/3-extensions/middleware-cache/src/cache-middleware.tspackages/3-extensions/middleware-cache/src/cache-store.tspackages/3-extensions/middleware-cache/src/exports/index.tspackages/3-extensions/middleware-cache/src/uncache-annotation.tspackages/3-extensions/middleware-cache/test/cache-annotation.test.tspackages/3-extensions/middleware-cache/test/cache-annotation.types.test-d.tspackages/3-extensions/middleware-cache/test/cache-key.test.tspackages/3-extensions/middleware-cache/test/cache-middleware.test.tspackages/3-extensions/middleware-cache/test/cache-middleware.types.test-d.tspackages/3-extensions/middleware-cache/test/cache-store.test.tspackages/3-extensions/middleware-cache/test/uncache-annotation.test.tspackages/3-extensions/middleware-cache/test/uncache-annotation.types.test-d.ts
✅ Files skipped from review due to trivial changes (1)
- packages/3-extensions/middleware-cache/src/exports/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/3-extensions/middleware-cache/test/uncache-annotation.test.ts
- packages/3-extensions/middleware-cache/test/cache-middleware.types.test-d.ts
- packages/3-extensions/middleware-cache/src/cache-annotation.ts
- packages/3-extensions/middleware-cache/test/cache-key.test.ts
- packages/3-extensions/middleware-cache/src/uncache-annotation.ts
- packages/3-extensions/middleware-cache/test/cache-annotation.types.test-d.ts
- packages/3-extensions/middleware-cache/test/cache-store.test.ts
- packages/3-extensions/middleware-cache/test/uncache-annotation.types.test-d.ts
- packages/3-extensions/middleware-cache/test/cache-middleware.test.ts
- packages/3-extensions/middleware-cache/src/cache-store.ts
Signed-off-by: paulwer <paul@wer-ner.de>
paulwer
force-pushed
the
feat-cache-strategies
branch
from
24dee91 to
b25a73d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-extensions/middleware-cache/src/cache-middleware.ts`:
- Around line 705-714: The isKeyInNamespace function uses substring-based
matching (includes(`:${namespace}:`)) to check if index keys belong to a
namespace, which can cause false positives when namespace-like strings appear
elsewhere in the key (such as in model or selector portions), resulting in
unrelated index entries being deleted. Instead of using includes for substring
matching on index keys that start with CACHE_INTERNAL_INDEX_PREFIX, parse the
actual key structure properly to extract and compare the namespace component
directly from the parsed cache key embedded in the index key, ensuring exact
namespace matching rather than partial string matching.
- Around line 769-774: In the createCacheMiddleware function, the clock option
from CacheMiddlewareOptions is not being forwarded to the
createInMemoryCacheStore call. To fix this, when creating the default in-memory
cache store, pass the clock option from the options parameter (options?.clock)
to createInMemoryCacheStore so that custom clock configurations are properly
used for TTL and expiry calculations instead of defaulting to real time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 72460bc0-e3d8-4d36-a8f1-766393e7b047
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/middleware-cache-strategies/spec.mdis excluded by!projects/**
📒 Files selected for processing (15)
packages/3-extensions/middleware-cache/README.mdpackages/3-extensions/middleware-cache/package.jsonpackages/3-extensions/middleware-cache/src/cache-annotation.tspackages/3-extensions/middleware-cache/src/cache-middleware.tspackages/3-extensions/middleware-cache/src/cache-store.tspackages/3-extensions/middleware-cache/src/exports/index.tspackages/3-extensions/middleware-cache/src/uncache-annotation.tspackages/3-extensions/middleware-cache/test/cache-annotation.test.tspackages/3-extensions/middleware-cache/test/cache-annotation.types.test-d.tspackages/3-extensions/middleware-cache/test/cache-key.test.tspackages/3-extensions/middleware-cache/test/cache-middleware.test.tspackages/3-extensions/middleware-cache/test/cache-middleware.types.test-d.tspackages/3-extensions/middleware-cache/test/cache-store.test.tspackages/3-extensions/middleware-cache/test/uncache-annotation.test.tspackages/3-extensions/middleware-cache/test/uncache-annotation.types.test-d.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/3-extensions/middleware-cache/test/cache-annotation.test.ts
- packages/3-extensions/middleware-cache/test/cache-store.test.ts
- packages/3-extensions/middleware-cache/test/uncache-annotation.test.ts
- packages/3-extensions/middleware-cache/test/cache-middleware.types.test-d.ts
- packages/3-extensions/middleware-cache/test/cache-key.test.ts
- packages/3-extensions/middleware-cache/test/uncache-annotation.types.test-d.ts
- packages/3-extensions/middleware-cache/test/cache-annotation.types.test-d.ts
- packages/3-extensions/middleware-cache/package.json
- packages/3-extensions/middleware-cache/src/cache-annotation.ts
- packages/3-extensions/middleware-cache/src/uncache-annotation.ts
- packages/3-extensions/middleware-cache/src/exports/index.ts
- packages/3-extensions/middleware-cache/src/cache-store.ts
- packages/3-extensions/middleware-cache/test/cache-middleware.test.ts
Comment on lines
+705
to
+714
| function isKeyInNamespace(key: string, namespace: string): boolean { | ||
| if (key.startsWith(`${namespace}:`)) return true; | ||
| // Index keys embed the cache key as a suffix after the model/entity prefix. | ||
| // Check whether that embedded cache key belongs to the namespace. | ||
| if (key.startsWith(CACHE_INTERNAL_INDEX_PREFIX)) { | ||
| const afterPrefix = key.slice(CACHE_INTERNAL_INDEX_PREFIX.length); | ||
| return afterPrefix.includes(`:${namespace}:`); | ||
| } | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid substring-based namespace matching for internal index keys.
isKeyInNamespace uses includes(\:${namespace}:`)` for index keys. That can match namespace-like substrings in model/selector portions, causing unrelated index entries to be deleted and leaving corresponding cache keys orphaned from index-driven invalidation.
Safer direction (pair index-key filtering to parsed cache keys)
- function isKeyInNamespace(key: string, namespace: string): boolean {
- if (key.startsWith(`${namespace}:`)) return true;
- if (key.startsWith(CACHE_INTERNAL_INDEX_PREFIX)) {
- const afterPrefix = key.slice(CACHE_INTERNAL_INDEX_PREFIX.length);
- return afterPrefix.includes(`:${namespace}:`);
- }
- return false;
- }
+// Keep namespace checks on actual cache keys. For index entries, parse the
+// embedded cache key first (with known model/entity prefixes) and compare that
+// embedded key via startsWith(`${namespace}:`).
+// Then only delete index+cache keys as a pair when the parsed cache key matches.- for (const key of keysToDelete) {
- if (namespace !== undefined && !isKeyInNamespace(key, namespace)) continue;
+ for (const key of keysToDelete) {
+ if (namespace !== undefined && !key.startsWith(`${namespace}:`) && !isVerifiedIndexKeyInNamespace(key, namespace)) continue;
await h.store.del(key);
removeKeyFromIndex(h, key);
}Also applies to: 1034-1035, 1074-1075
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/3-extensions/middleware-cache/src/cache-middleware.ts` around lines
705 - 714, The isKeyInNamespace function uses substring-based matching
(includes(`:${namespace}:`)) to check if index keys belong to a namespace, which
can cause false positives when namespace-like strings appear elsewhere in the
key (such as in model or selector portions), resulting in unrelated index
entries being deleted. Instead of using includes for substring matching on index
keys that start with CACHE_INTERNAL_INDEX_PREFIX, parse the actual key structure
properly to extract and compare the namespace component directly from the parsed
cache key embedded in the index key, ensuring exact namespace matching rather
than partial string matching.
Comment on lines
+769
to
+774
| export function createCacheMiddleware(options?: CacheMiddlewareOptions): CacheMiddleware { | ||
| const defaultHandle = makeStoreHandle( | ||
| '__default__', | ||
| options?.store ?? | ||
| createInMemoryCacheStore({ | ||
| maxEntries: options?.maxEntries ?? DEFAULT_MAX_ENTRIES, | ||
| }); | ||
| createInMemoryCacheStore({ maxEntries: options?.maxEntries ?? DEFAULT_MAX_ENTRIES }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Pass the configured clock into the default in-memory store.
CacheMiddlewareOptions.clock is read, but not forwarded to createInMemoryCacheStore. This breaks the option contract and makes TTL/expiry use real time even when a custom clock is configured.
Suggested patch
const defaultHandle = makeStoreHandle(
'__default__',
options?.store ??
- createInMemoryCacheStore({ maxEntries: options?.maxEntries ?? DEFAULT_MAX_ENTRIES }),
+ createInMemoryCacheStore({
+ maxEntries: options?.maxEntries ?? DEFAULT_MAX_ENTRIES,
+ clock: options?.clock,
+ }),
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/3-extensions/middleware-cache/src/cache-middleware.ts` around lines
769 - 774, In the createCacheMiddleware function, the clock option from
CacheMiddlewareOptions is not being forwarded to the createInMemoryCacheStore
call. To fix this, when creating the default in-memory cache store, pass the
clock option from the options parameter (options?.clock) to
createInMemoryCacheStore so that custom clock configurations are properly used
for TTL and expiry calculations instead of defaulting to real time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Discord Thread
Summary
Extends
@prisma-next/middleware-cachewith three new cache invalidation strategies (broad,targeted,versioned), mutation-driven invalidation via a newuncacheAnnotation, in-process miss deduplication (single-flight), tag-based bulk invalidation, and a configurablestoreOperationMode(await/detached). The changes make cache invalidation a first-class concern alongside caching, removing the previous limitation where every write required manual cache management.Testing performed
pnpm --filter @prisma-next/middleware-cache test— all cache middleware, cache-store, annotation, and uncache-annotation tests passedpnpm --filter "@prisma-next/e2e-tests" test— 20 test files, 113 tests passed (including temp-table JOIN tests fixed in the same session)Skill update
n/a — internal only (
@prisma-next/middleware-cacheis not yet part of the public surface documented inpackages/0-shared/skills/).Checklist
git commit -s) per the DCO. The DCO status check will block merge if any commit is missing aSigned-off-by:trailer.n/aif the change is doc-only / refactor with no behavioural delta).TML-NNNN: <sentence-case title>form (Linear ticket prefix + concise title naming the concrete deliverable). See.claude/skills/create-pr/SKILL.mdfor the full convention.n/a — internal only).Notes for the reviewer
What's new vs the previous state of
middleware-cache:uncacheAnnotation— write-only annotation (applicableTo: ['write']) that attaches invalidation actions to a mutation terminal. EachUncacheActioncan target keys, model indexes, tags, or a namespace prefix. Structurally impossible to apply to a read terminal.Three invalidation strategy modes (
CacheStrategyMode):broad— on any write, blows away all keys matching the namespace. Simplest; highest false-invalidation rate.targeted— tracks entity selectors (model + table + id columns) on both reads and writes; on a write invalidates only keys that touched the same rows. Default.versioned— stores a per-model generation counter; reads include the current generation in their key; writes bump the counter. No explicit deletes needed; stale keys expire naturally via TTL.In-process miss deduplication (
dedupe/readDedupe) — concurrent cache misses for the same effective key share a single leader execution via an in-processMap<key, InflightMiss>. Followers resolve from the leader's result without touching the driver.Tag-based invalidation —
CachedEntry.tags+CacheStore.delByTag+cacheAnnotation({ tags: [...] }).uncacheAnnotation({ uncache: [{ tags: [...] }] })triggers bulk deletion.storeOperationMode: 'detached'— store writes and deletes run fire-and-forget in the background; reduces response latency at the cost of eventual consistency. Default is'await'.CacheStore.incr— new optional method on theCacheStoreinterface, required only forversionedmode in clustered setups.Standalone
uncache()helper —middleware.uncache(actions)and the exporteduncache(middleware, actions)free-function allow out-of-band cache invalidation without attaching an annotation to a mutation.Summary by CodeRabbit
uncacheand a middlewareuncache()method.list,del,delByTag, andincr.