Add DataStorage interface and implementations#4458
Conversation
Introduce DataStorage, a key-value metadata store separate from the existing Session-based Storage interface. Unlike Storage, DataStorage never round-trips live session objects — it stores only serialisable map[string]string metadata. This separation avoids the type-assertion bug where a Redis round-trip deserialises a MultiSession as a plain *StreamableSession, losing all backend connections and routing state. Two implementations are provided: - LocalSessionDataStorage: in-memory sync.Map with TTL-based eviction and a background cleanup goroutine. - RedisSessionDataStorage: Redis/Valkey-backed with sliding-window TTL via GETEX on every Load, and atomic SET NX for StoreIfAbsent. Both pass a shared contract test suite covering Store/Load round-trips, upsert, nil metadata, empty-ID errors, Exists (without TTL refresh), StoreIfAbsent atomicity, Delete idempotency, and TTL behaviour. Related-to: #4420
There was a problem hiding this comment.
Pull request overview
Introduces a new DataStorage abstraction for storing session metadata only (map[string]string) to avoid round-tripping live session objects through Redis and the associated type-loss/deserialization issues.
Changes:
- Added
DataStorageinterface withStore/Load/Exists/StoreIfAbsent/Delete/Closesemantics. - Implemented
LocalSessionDataStorage(in-memorysync.Mapwith TTL eviction via background cleanup goroutine). - Implemented
RedisSessionDataStorage(JSON-encoded metadata with sliding TTL viaGETEX, atomic create viaSET NX) plus a shared contract test suite for both implementations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/transport/session/session_data_storage.go | Defines the DataStorage interface and provides the local constructor. |
| pkg/transport/session/session_data_storage_local.go | Adds in-memory metadata storage with TTL-based eviction and cleanup goroutine. |
| pkg/transport/session/session_data_storage_redis.go | Adds Redis/Valkey-backed metadata storage with sliding TTL and atomic create. |
| pkg/transport/session/session_data_storage_test.go | Adds contract tests and TTL/behavior tests for both implementations. |
💡 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 #4458 +/- ##
==========================================
+ Coverage 69.54% 69.68% +0.13%
==========================================
Files 486 493 +7
Lines 50026 50452 +426
==========================================
+ Hits 34791 35156 +365
- Misses 12554 12602 +48
- Partials 2681 2694 +13 ☔ 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 4 out of 4 changed files in this pull request and generated 4 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 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jerm-dro
left a comment
There was a problem hiding this comment.
Clean implementation — the interface is well-motivated and both implementations are solid with thorough shared contract tests. Two suggestions inline on naming and interface surface area. Feel free to address them in a stacked PR.
Summary
Introduce DataStorage, a key-value metadata store separate from the existing Session-based Storage interface. Unlike Storage, DataStorage never round-trips live session objects — it stores only serialisable map[string]string metadata.
This separation avoids the type-assertion bug where a Redis round-trip deserialises a MultiSession as a plain *StreamableSession, losing all backend connections and routing state.
Two implementations are provided:
Both pass a shared contract test suite covering Store/Load round-trips, upsert, nil metadata, empty-ID errors, Exists (without TTL refresh), StoreIfAbsent atomicity, Delete idempotency, and TTL behaviour.
Fixes #4220
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