Skip to content

Wire Redis Backend Selection into session.Manager (RC-7) #4208

@yrobla

Description

@yrobla

Description

Add a factory constructor to pkg/transport/session/manager.go that selects RedisStorage when a RedisConfig is provided and falls back to LocalStorage otherwise. This wires the Redis storage backend implemented in RC-6 into the session manager lifecycle, enabling the manager to be constructed with an externalized Redis backend without changing the signatures of any existing constructors (NewManager, NewTypedManager).

This task is the second layer of the THV-0047 horizontal scaling foundation. Once complete, the vMCP server can construct a Redis-backed session manager by passing a RedisConfig, and all existing callers that do not supply a config continue to use in-memory LocalStorage without modification.

Context

pkg/transport/session/manager.go currently exposes three constructors: NewManager, NewTypedManager, and NewManagerWithStorage. The first two always use NewLocalStorage() as the storage backend. NewManagerWithStorage is a lower-level extension point that accepts any Storage implementation. RC-7 adds a higher-level convenience constructor — NewManagerWithRedis or similar — that internally constructs a RedisStorage from a RedisConfig and calls NewManagerWithStorage, keeping existing callers untouched.

Parent epic: stacklok/stacklok-epics#262

Dependencies: #266 (RC-6: Redis Storage Backend for transport/session)
Blocks: TASK-003 (RC-8: Persist vMCP Session Metadata to Redis on Creation)

Acceptance Criteria

  • A new exported constructor (e.g. NewManagerWithRedis) exists in pkg/transport/session/manager.go that accepts a RedisConfig (or a pointer) and a Factory and returns (*Manager, error)
  • When RedisConfig is non-nil (or the pointer is non-nil), the constructor calls NewRedisStorage (from RC-6) and passes the resulting RedisStorage to NewManagerWithStorage
  • The TTL passed to NewManagerWithStorage is also used as the Redis key TTL so that session expiry in Redis matches the manager's cleanup interval
  • NewManager signature is unchanged and continues to use NewLocalStorage internally
  • NewTypedManager signature is unchanged and continues to use NewLocalStorage internally
  • NewManagerWithStorage signature is unchanged (still the general extension point for custom storage)
  • All existing callers of NewManager, NewTypedManager, and NewManagerWithStorage compile and pass tests without modification
  • Unit tests cover: Redis config present → RedisStorage is used; nil or absent config → LocalStorage is used
  • Unit tests verify the constructor returns an error when NewRedisStorage fails (e.g. invalid config)
  • All tests pass (go test ./pkg/transport/session/...)
  • Code reviewed and approved

Technical Approach

Recommended Implementation

Add one new function to pkg/transport/session/manager.go:

NewManagerWithRedis(ttl time.Duration, factory Factory, cfg RedisConfig) (*Manager, error)

Inside, call NewRedisStorage(cfg, ttl) — the constructor defined in TASK-001. If NewRedisStorage returns an error (bad config, TLS cert load failure), propagate it. Otherwise pass the RedisStorage to NewManagerWithStorage and return the manager.

Because NewManagerWithStorage already starts the cleanup goroutine, no additional lifecycle code is needed. For Redis-backed storage the cleanup goroutine calls DeleteExpired which is a no-op (Redis TTL handles expiry), so there is no performance concern.

The call site that needs to opt in to Redis (pkg/vmcp/server/server.go, line 382) will be updated in a later task. This task only adds the constructor; the server wiring is deferred.

Patterns & Frameworks

  • Follow the existing constructor pattern in pkg/transport/session/manager.go: construct a Manager struct literal, start go m.cleanupRoutine(), return
  • Use NewManagerWithStorage as the sole internal extension point — do not duplicate the manager struct initialization
  • Return (*Manager, error) from the new constructor to propagate NewRedisStorage errors; all existing constructors return *Manager (no error) because NewLocalStorage is infallible
  • Test helper pattern: follow pkg/authserver/storage/redis_test.go's newTestRedisStorage / withRedisStorage pattern using github.com/alicebob/miniredis/v2

Code Pointers

  • pkg/transport/session/manager.go — file to modify; add NewManagerWithRedis after NewManagerWithStorage (line 104); the ttl field is the value to forward to NewRedisStorage as the key TTL
  • pkg/transport/session/storage_redis.go — new file from TASK-001; provides NewRedisStorage(cfg RedisConfig, ttl time.Duration) (*RedisStorage, error) which this constructor calls
  • pkg/transport/session/redis_config.go — new file from TASK-001; provides the RedisConfig type accepted by the new constructor
  • pkg/transport/session/storage_local.go — shows what NewLocalStorage returns for comparison; the fallback path that existing callers rely on
  • pkg/transport/session/storage_test.go — existing test patterns for storage and manager; new tests for NewManagerWithRedis should follow the same table-driven, t.Parallel() style
  • pkg/authserver/storage/redis_test.gonewTestRedisStorage / withRedisStorage helper pattern using miniredis.RunT(t); adapt for session-layer tests
  • pkg/vmcp/server/server.go line 382 — current NewManager call that will eventually be replaced; this task does NOT change this call site

Component Interfaces

// NewManagerWithRedis creates a session manager backed by Redis.
// cfg supplies the Redis connection configuration; ttl is applied as both the
// manager's cleanup interval and the Redis key TTL.
// Returns an error if the Redis client cannot be constructed (e.g. invalid TLS config).
// Callers that do not require Redis should continue to use NewManager or NewTypedManager.
func NewManagerWithRedis(ttl time.Duration, factory Factory, cfg RedisConfig) (*Manager, error) {
    storage, err := NewRedisStorage(cfg, ttl)
    if err != nil {
        return nil, fmt.Errorf("creating redis storage: %w", err)
    }
    return NewManagerWithStorage(ttl, factory, storage), nil
}

No changes to existing signatures:

// Unchanged — existing callers continue to compile without modification.
func NewManager(ttl time.Duration, factory interface{}) *Manager
func NewTypedManager(ttl time.Duration, sessionType SessionType) *Manager
func NewManagerWithStorage(ttl time.Duration, factory Factory, storage Storage) *Manager

Testing Strategy

Use github.com/alicebob/miniredis/v2 (miniredis.RunT(t)) for all Redis-backed tests. No live Redis connection is required.

Unit Tests

  • NewManagerWithRedis — valid config pointing at a miniredis instance → returns non-nil *Manager and nil error
  • NewManagerWithRedisRedisConfig with an unreachable address → construction should succeed (go-redis defers connection); verify that the returned manager is non-nil (lazy connection is the go-redis default)
  • NewManagerWithRedisRedisConfig with invalid TLS PEM in CACert → returns a non-nil error
  • NewManagerWithRedis — round-trip: store a session via manager.AddWithID, retrieve it via manager.Get; verify the session can be found (exercises the Redis path end-to-end with miniredis)
  • NewManager — verify it still returns a manager backed by LocalStorage (type assertion m.storage.(*LocalStorage) succeeds)
  • NewTypedManager — verify it still returns a manager backed by LocalStorage

Integration Tests

  • None required at this layer; unit tests with miniredis provide sufficient coverage

Edge Cases

  • Calling Stop() on a Redis-backed manager closes the underlying Redis client (go-redis client.Close() is called via RedisStorage.Close())
  • DeleteExpired called by the cleanup goroutine on a Redis-backed manager is a no-op and does not return an error

Out of Scope

  • Implementing RedisStorage itself (TASK-001 / RC-6)
  • Changing the call site in pkg/vmcp/server/server.go to use NewManagerWithRedis (deferred to a later wiring task, or done as part of TASK-003+)
  • Any changes to pkg/vmcp packages
  • Redis Cluster support
  • Operational concerns: Redis provisioning, Kubernetes configuration

References

  • RFC THV-0047: RFC: Horizontal Scaling for vMCP and Proxy Runner toolhive-rfcs#47
  • Parent epic: stacklok/stacklok-epics#262
  • Upstream dependency (RC-6): stacklok/stacklok-epics#266
  • Storage interface: pkg/transport/session/storage.go
  • NewManagerWithStorage extension point: pkg/transport/session/manager.go (line 104)
  • Authserver Redis test helper pattern: pkg/authserver/storage/redis_test.go

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestscalabilityItems related to scalabilityvmcpVirtual MCP Server related issues

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions