feat: in memory adapter for entity caching cache - #3137
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a mutex-protected in-memory entity cache with batch reads and writes, mandatory TTL expiration, context cancellation, defensive byte-slice copying, expiration cleanup, and comprehensive tests. Updates the GraphQL tools dependency. ChangesEntity cache
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@router/pkg/entitycaching/cache/in_memory.go`:
- Around line 58-68: Update the in-memory cache implementation around the
GetMany expiration cleanup and c.entries to prevent unrequested expired entries
from accumulating indefinitely. Add a bounded capacity policy with periodic
expiration sweeping, or replace it with a bounded TTL cache that has an explicit
shutdown lifecycle; preserve concurrency safety and ensure cleanup does not
delete entries refreshed since observation.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96515d7e-f2fd-44d4-ab66-c635979310e1
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
router/go.modrouter/pkg/entitycaching/cache/in_memory.gorouter/pkg/entitycaching/cache/in_memory_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/entitycaching/cache/in_memory.go (1)
50-63: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecheck the context after mutex acquisition.
Both methods check
ctx.Err()before a mutex operation that can block. If the context expires while waiting,GetManycan return data after cancellation andSetManycan store data after cancellation.
router/pkg/entitycaching/cache/in_memory.go#L50-L63: checkctx.Err()immediately afterRLocksucceeds and returnnil, errbefore reading entries.router/pkg/entitycaching/cache/in_memory.go#L92-L105: checkctx.Err()immediately afterLocksucceeds and return before writing entries.router/pkg/entitycaching/cache/in_memory_test.go#L542-L597: add contention tests that holdc.mu, cancel the context, then release the mutex; verify no result and no write.🤖 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 `@router/pkg/entitycaching/cache/in_memory.go` around lines 50 - 63, Recheck the context immediately after `GetMany` acquires `c.mu` with `RLock`, returning no results and the context error before reading entries; apply the same guard after `SetMany` acquires `c.mu` with `Lock`, returning before writes. In `router/pkg/entitycaching/cache/in_memory.go` lines 50-63 and 92-105, update the corresponding methods; in `router/pkg/entitycaching/cache/in_memory_test.go` lines 542-597, add contention tests that hold `c.mu`, cancel the context, release the mutex, and verify `GetMany` returns no result and `SetMany` performs no write.
🤖 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 `@router/pkg/entitycaching/cache/in_memory.go`:
- Around line 57-61: Treat entries as expired when now is equal to or later than
expiresAt by updating the expiry check in GetMany within
router/pkg/entitycaching/cache/in_memory.go:57-61 to use
!now.Before(entry.expiresAt). Update the expiry-boundary test in
router/pkg/entitycaching/cache/in_memory_test.go:412-428 to expect a miss and
eviction when access occurs exactly at expiration.
---
Outside diff comments:
In `@router/pkg/entitycaching/cache/in_memory.go`:
- Around line 50-63: Recheck the context immediately after `GetMany` acquires
`c.mu` with `RLock`, returning no results and the context error before reading
entries; apply the same guard after `SetMany` acquires `c.mu` with `Lock`,
returning before writes. In `router/pkg/entitycaching/cache/in_memory.go` lines
50-63 and 92-105, update the corresponding methods; in
`router/pkg/entitycaching/cache/in_memory_test.go` lines 542-597, add contention
tests that hold `c.mu`, cancel the context, release the mutex, and verify
`GetMany` returns no result and `SetMany` performs no write.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d4c2590-eda1-4f6c-bc38-43045b58fa37
📒 Files selected for processing (2)
router/pkg/entitycaching/cache/in_memory.gorouter/pkg/entitycaching/cache/in_memory_test.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3137 +/- ##
==========================================
+ Coverage 62.37% 62.39% +0.02%
==========================================
Files 262 263 +1
Lines 31003 31043 +40
==========================================
+ Hits 19337 19370 +33
- Misses 10158 10163 +5
- Partials 1508 1510 +2
🚀 New features to boost your workflow:
|
3845440 to
689be8a
Compare
ec32312 to
d508d07
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@router/pkg/entitycaching/cache/in_memory.go`:
- Around line 47-50: The mutex-protected paths in GetMany and SetMany must
re-check cancellation after acquiring c.mu; add a ctx.Err() check immediately
after locking and before the cache sweep or batch write, returning the error
without processing when canceled. Add contention tests covering cancellation
while each method waits for c.mu.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48b65ce8-bac8-4458-941e-55fdde870cd9
📒 Files selected for processing (2)
router/pkg/entitycaching/cache/in_memory.gorouter/pkg/entitycaching/cache/in_memory_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/pkg/entitycaching/cache/in_memory_test.go
dkorittki
left a comment
dkorittki
left a comment
There was a problem hiding this comment.
I'd change package structure a bit. We know we will have at least two implementations of entitycache.Cache. I'd prefer to have a seperate packages for each
- router/pkg/entitycaching/in_memory/*.go
- router/pkg/entitycaching/redis/*.go
Gives us better namespacing and allows for better type names like redisEntity.Cache and inMemoryEntity.Cache by aliasing import names on call sites.
Another question: Have you considered using a ristretto cache as the backing cache technology for this implementation? It comes with all the evict logic we need to deal with TTLs.
|
|
||
| // ErrMissingTTL reports items that were rejected because they carried no | ||
| // positive TTL. Every cached entry expires, so a TTL is mandatory. | ||
| var ErrMissingTTL = errors.New("cache item requires a positive TTL") |
There was a problem hiding this comment.
This error isn't specific to the in memory cache, right? I would define it in the corresponding graphql-go-tools package then and mention in the methods godoc when its returned
There was a problem hiding this comment.
moved to graphqlgotools
| // This cache should only be used for tests OR development and never production | ||
| type InMemoryCache struct { | ||
| mu sync.Mutex | ||
| entries map[string]inMemoryEntry |
There was a problem hiding this comment.
Why not use map[string]enginecache.Item ?
There was a problem hiding this comment.
switched to ristretto
| // InMemoryCache sweeps every expired entry on each call, so both methods take | ||
| // the mutex exclusively, hence a plain Mutex rather than an RWMutex. | ||
| // This cache should only be used for tests OR development and never production |
There was a problem hiding this comment.
This rather belongs to the mu field as a godoc instead of InMemoryCache.
Also this could become a major bottleneck. Using a normal mutex instead of an RWMutex forces syncronous reads to the cache for all concurrent requests on the router. You can't use an RWMutex because of sweepExipred on GetMany. Maybe worth checking if you can use Ristretto like mentioned above so you don't need to care about this kind of problem.
I mean yeah it's only meant to be used in tests but if Ristretto can solve it for free and have it production ready for when the situation calls for it.
There was a problem hiding this comment.
switched to ristretto
| if !ok { | ||
| continue | ||
| } | ||
| results[i] = enginecache.Result{Value: bytes.Clone(entry.value), Found: true} |
There was a problem hiding this comment.
You're not really making use of Result.Found because its always true for returned items. Do we really need the Found field?
| if err := ctx.Err(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I'd put this on the top like in SetMany
| type InMemoryCache struct { | ||
| mu sync.Mutex | ||
| entries map[string]inMemoryEntry | ||
| now func() time.Time |
There was a problem hiding this comment.
This is only for test purposes right? Have you considered using testing/synctest? It lets you mock time inside the runtime, so you don't need to introduce code, which is only there for testing purposes.
8fa4312 to
f80628a
Compare
| ) | ||
|
|
||
| const entryCost = 1 | ||
| const maxSize = math.MaxInt32 |
There was a problem hiding this comment.
I'd probably go for something much smaller, like a million or so. This blows OOM way before we reach maxInt32
| // SetMany stores every item, all of which must carry a positive TTL. A single | ||
| // item without one fails the whole batch with an ErrMissingTTL and nothing is | ||
| // stored. It otherwise only fails on a cancelled context. |
There was a problem hiding this comment.
Since it's an implementation of the engines interface method I'd simply write
| // SetMany stores every item, all of which must carry a positive TTL. A single | |
| // item without one fails the whole batch with an ErrMissingTTL and nothing is | |
| // stored. It otherwise only fails on a cancelled context. | |
| // SetMany implements enginecache.SetMany. |
Any behaviour, guard logic and rules should be documented on the interface method godic as a contract for any implementor (imo)
| return &InMemoryCache{cache: cache}, nil | ||
| } | ||
|
|
||
| // GetMany returns the entries it found, keyed by the key they were asked for. |
There was a problem hiding this comment.
Since it's an implementation of the engines interface method I'd simply write
| // GetMany returns the entries it found, keyed by the key they were asked for. | |
| // GetMany implements enginecache.GetMany. |
Any behaviour, guard logic and rules should be documented on the interface method godic as a contract for any implementor (imo)
|
|
||
| for _, item := range items { | ||
| if item.TTL <= 0 { | ||
| return fmt.Errorf("%w: key %q", enginecache.ErrMissingTTL, item.Key) |
There was a problem hiding this comment.
On the lib version currently referred to by the routers go.mod there is no ErrMissingTTL. I think you need to bumb the engine version
| // Close stops the goroutines ristretto runs behind the cache. It is safe to | ||
| // call more than once, but not while a GetMany or a SetMany is in flight. |
There was a problem hiding this comment.
but not while a GetMany or a SetMany is in flight
We can't control this from here right? I checked and Ristretto will actually panic if you close the cache during a concurrent write. Can you secure it with an RWMutex?
2 participants
This PR introduces the in memory adapter for entity caching cache based on the interface defined here in this PR wundergraph/graphql-go-tools#1618
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.