feat: Add pluggable storage backend for session management #1989
Conversation
Refactors the session management system to use a pluggable storage interface, enabling future support for distributed storage backends like Redis/Valkey while maintaining backward compatibility. What Changed - Introduced a Storage interface that abstracts session persistence - Refactored Manager to use the Storage interface instead of directly using sync.Map - Created LocalStorage implementation that maintains the existing in-memory behavior - Added JSON serialization support for sessions to enable future network storage - Extended Session interface with Type() and metadata methods that were already implemented in concrete types Why The previous implementation was tightly coupled to in-memory storage, making it impossible to share sessions across multiple ToolHive instances. This refactoring enables: - Horizontal scaling with shared session state - Session persistence across restarts - Future Redis/Valkey backend support without breaking changes Testing Added comprehensive unit tests covering: - LocalStorage implementation - Session serialization/deserialization - Manager with pluggable storage - All existing session types (ProxySession, SSESession, StreamableSession) All tests pass and the implementation maintains full backward compatibility. Signed-off-by: Juan Antonio Osorio <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1989 +/- ##
==========================================
+ Coverage 47.38% 47.85% +0.46%
==========================================
Files 232 234 +2
Lines 28644 28784 +140
==========================================
+ Hits 13574 13775 +201
+ Misses 14046 13986 -60
+ Partials 1024 1023 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the session management system to use a pluggable storage interface, enabling future support for distributed storage backends like Redis/Valkey while maintaining backward compatibility with the existing in-memory behavior.
Key changes:
- Introduced a
Storageinterface that abstracts session persistence operations - Refactored
Managerto use the Storage interface instead of directly usingsync.Map - Added JSON serialization support for sessions to enable future network storage
- Enhanced session interfaces with metadata and data management methods
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/transport/session/storage.go | New Storage interface defining core operations for session persistence |
| pkg/transport/session/storage_local.go | LocalStorage implementation using sync.Map for backward compatibility |
| pkg/transport/session/serialization.go | JSON serialization functions for future distributed storage support |
| pkg/transport/session/manager.go | Refactored Manager to use Storage interface instead of direct sync.Map |
| pkg/transport/session/storage_test.go | Comprehensive test suite for storage implementations and serialization |
| pkg/transport/session/serialization_test.go | Test suite for JSON serialization/deserialization functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return nil | ||
| } | ||
|
|
||
| // Close is a no-op for local storage as there are no resources to clean up. |
There was a problem hiding this comment.
The comment is incorrect - the Close method actually clears all sessions, which is not a no-op. Update the comment to reflect the actual behavior.
| // Close is a no-op for local storage as there are no resources to clean up. | |
| // Close clears all sessions from local storage. |
Copilot uses AI. Check for mistakes.
| s.sessions.Range(func(key, _ any) bool { | ||
| s.sessions.Delete(key) | ||
| return true | ||
| }) |
There was a problem hiding this comment.
Modifying the sync.Map while iterating over it with Range can lead to race conditions. Consider collecting keys first, then deleting them in a separate loop, similar to the pattern used in DeleteExpired.
| s.sessions.Range(func(key, _ any) bool { | |
| s.sessions.Delete(key) | |
| return true | |
| }) | |
| var toDelete []any | |
| s.sessions.Range(func(key, _ any) bool { | |
| toDelete = append(toDelete, key) | |
| return true | |
| }) | |
| for _, key := range toDelete { | |
| s.sessions.Delete(key) | |
| } |
Copilot uses AI. Check for mistakes.
| sseSession := NewSSESession(sd.ID) | ||
| // Update timestamps to match stored values | ||
| sseSession.created = sd.CreatedAt | ||
| sseSession.updated = sd.UpdatedAt |
There was a problem hiding this comment.
Direct field assignment bypasses encapsulation and could break if the internal structure changes. Consider adding setter methods or using reflection-based copying instead of direct field access.
| sseSession.updated = sd.UpdatedAt | |
| sseSession.SetUpdatedAt(sd.UpdatedAt) |
Copilot uses AI. Check for mistakes.
| sseSession.created = sd.CreatedAt | ||
| sseSession.updated = sd.UpdatedAt | ||
| // Restore metadata | ||
| sseSession.metadata = sd.Metadata |
There was a problem hiding this comment.
Direct field assignment bypasses encapsulation and could break if the internal structure changes. Consider adding setter methods or using reflection-based copying instead of direct field access.
Copilot uses AI. Check for mistakes.
| streamSession.created = sd.CreatedAt | ||
| streamSession.updated = sd.UpdatedAt | ||
| streamSession.sessType = SessionTypeStreamable |
There was a problem hiding this comment.
Direct field assignment bypasses encapsulation and could break if the internal structure changes. Consider adding setter methods or using reflection-based copying instead of direct field access.
Copilot uses AI. Check for mistakes.
| streamSession.metadata = sd.Metadata | ||
| if streamSession.metadata == nil { | ||
| streamSession.metadata = make(map[string]string) | ||
| } |
There was a problem hiding this comment.
Direct field assignment bypasses encapsulation and could break if the internal structure changes. Consider adding setter methods or using reflection-based copying instead of direct field access.
| streamSession.metadata = sd.Metadata | |
| if streamSession.metadata == nil { | |
| streamSession.metadata = make(map[string]string) | |
| } | |
| streamSession.SetMetadata(sd.Metadata) | |
| // SetMetadata handles nil initialization | |
Copilot uses AI. Check for mistakes.
| proxySession.created = sd.CreatedAt | ||
| proxySession.updated = sd.UpdatedAt |
There was a problem hiding this comment.
Direct field assignment bypasses encapsulation and could break if the internal structure changes. Consider adding setter methods or using reflection-based copying instead of direct field access.
Copilot uses AI. Check for mistakes.
| proxySession.created = sd.CreatedAt | ||
| proxySession.updated = sd.UpdatedAt | ||
| // Restore metadata | ||
| proxySession.metadata = sd.Metadata |
There was a problem hiding this comment.
Direct field assignment bypasses encapsulation and could break if the internal structure changes. Consider adding setter methods or using reflection-based copying instead of direct field access.
Copilot uses AI. Check for mistakes.
Overview
Refactors the session management system to use a pluggable storage interface, enabling future support for distributed storage backends like Redis/Valkey while maintaining backward compatibility.
What Changed
Core Architecture
Storageinterface that abstracts session persistence operationsManagerto use the Storage interface instead of directly usingsync.MapLocalStorageimplementation that maintains the existing in-memory behaviorSession Interface Enhancements
Sessioninterface withType()and metadata methods that were already implemented in concrete typesGetData(),SetData(),GetMetadata(), andSetMetadata()methods for better data managementNew Components
Storageinterface with methods:Store(),Load(),Delete(),DeleteExpired(),Close()LocalStorageimplementation usingsync.Mapfor backward compatibilitySessionTypeenum for better type identificationWhy This Change?
The previous implementation was tightly coupled to in-memory storage, making it impossible to share sessions across multiple ToolHive instances. This refactoring enables:
Testing
Added comprehensive unit tests covering:
LocalStorageimplementation with all CRUD operationsManagerwith pluggable storage interfaceProxySession,SSESession,StreamableSession)Backward Compatibility
✅ Full backward compatibility maintained
NewManager()function continues to work unchangedFiles Changed
pkg/transport/session/manager.go- Refactored to use Storage interfacepkg/transport/session/storage.go- New Storage interface and LocalStorage implementationpkg/transport/session/serialization.go- JSON serialization supportpkg/transport/session/session.go- Enhanced session types with metadata supportpkg/transport/session/storage_test.go- Comprehensive test suitepkg/transport/session/serialization_test.go- Serialization testsFuture Work
This foundation enables:
Checklist