Implement RoutingStorage LRU+remote two-tier backend#4294
Implement RoutingStorage LRU+remote two-tier backend#4294
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbae45d38a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a new two-tier session.Storage implementation that fronts a remote storage backend (e.g., Redis) with a bounded in-memory LRU to reduce remote round-trips while preserving remote durability/authority.
Changes:
- Introduce
RoutingStorageimplementing thesession.Storageinterface with local LRU + remote fallback. - Add a comprehensive unit test suite covering cache hits/misses, eviction, delete semantics, remote failure behavior, and constructor panics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/transport/session/storage_routing.go | New two-tier storage implementation using hashicorp/golang-lru for local caching and a delegated remote Storage. |
| pkg/transport/session/storage_routing_test.go | Unit tests validating caching behavior, remote delegation, eviction, and concurrency sanity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4294 +/- ##
==========================================
+ Coverage 68.85% 68.98% +0.13%
==========================================
Files 473 474 +1
Lines 47899 47938 +39
==========================================
+ Hits 32979 33070 +91
- Misses 12279 12289 +10
+ Partials 2641 2579 -62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds RoutingStorage, a two-tier session storage that serves reads from a bounded in-memory LRU cache and falls back to a remote Storage (Redis) on cache misses. Store writes to remote first, only promoting to the local cache on success. DeleteExpired delegates entirely to remote, which owns the TTL source of truth; the local cache self-corrects on subsequent Loads. Uses github.com/hashicorp/golang-lru/v2 (already a transitive dependency) for a thread-safe generic LRU. NewRoutingStorage panics on maxLocalEntries <= 0 so that callers apply a sensible default (e.g. 1000) before calling.
jerm-dro
left a comment
There was a problem hiding this comment.
Do we need the LRU cache?
The original RFC I proposed mentioned LRU caching, but after seeing this implementation I've reconsidered. The PR correctly identifies that we must refresh Redis TTLs on access — something the RFC didn't account for. That constraint changes the calculus.
Every Load cache hit still calls remote.Touch (Redis EXPIRE), so every request pays a Redis RTT regardless. The LRU only saves deserializing the session payload, but sessions are small and GET and EXPIRE are both O(1) — the RTT dominates.
That means we're taking on significant complexity for a marginal saving: a new Touch method on the Storage interface (breaking change for all implementations), cross-replica staleness edge cases, LRU eviction/recovery logic, and a generated mock.
Suggestion: drop the LRU and have RedisStorage.Load use GETEX instead of GET. GETEX atomically returns the data and refreshes the TTL — the TTL refresh becomes an internal Redis concern, invisible to callers. No Touch, no interface change, no cache invalidation bugs. The RoutingStorage, mock, and Touch additions can all be removed.
If we later find Redis RTT is a bottleneck, we can add a local TTL cache in front, but that would be a different design (no per-hit remote calls, bounded staleness via local expiry, GETEX on cache miss).
Summary
Adds RoutingStorage, a two-tier session storage that serves reads from a bounded in-memory LRU cache and falls back to a remote Storage (Redis) on cache misses. Store writes to remote first, only promoting to the local cache on success. DeleteExpired delegates entirely to remote, which owns the TTL source of truth; the local cache self-corrects on subsequent Loads.
Uses github.com/hashicorp/golang-lru/v2 (already a transitive dependency) for a thread-safe generic LRU. NewRoutingStorage panics on maxLocalEntries <= 0 so that callers apply a sensible default (e.g. 1000) before calling.
Fixes #4210
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers