Skip to content

Migrate session manager to DataStorage with cross-pod restore#4479

Merged
yrobla merged 1 commit intomainfrom
issue-4420-sm-migration-v1
Apr 7, 2026
Merged

Migrate session manager to DataStorage with cross-pod restore#4479
yrobla merged 1 commit intomainfrom
issue-4420-sm-migration-v1

Conversation

@yrobla
Copy link
Copy Markdown
Contributor

@yrobla yrobla commented Apr 1, 2026

Summary

Follow-on to #4464 (merged). That PR landed the core DataStorage migration — RestorableCache, sessionmanager.Manager rewrite, MultiSessionGetter interface, and RestoreSession. This PR adds the remaining piece: Redis session storage wiring.

Redis session storage wiring

Add SessionStorage *vmcpconfig.SessionStorageConfig to vmcpserver.Config and introduce buildSessionDataStorage:

  • When SessionStorage is nil, provider is "memory", or provider is empty/unset, local in-process storage is used (default behaviour unchanged).
  • When provider is "redis", a RedisSessionDataStorage is constructed (password from THV_SESSION_REDIS_PASSWORD) — enabling cross-pod session persistence.
  • Any other provider value is rejected immediately with a clear error (e.g. a typo like "Redis" will not silently fall back to local storage).
  • Provider matching is case-insensitive for "memory".

Pass cfg.SessionStorage from commands.go so the operator-provided Redis config reaches the server.

Fixes #4220

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Does this introduce a user-facing change?

No new user-facing changes beyond #4464. The Redis wiring is a configuration opt-in (operator sets sessionStorage.provider: redis); default behaviour (local in-process storage) is unchanged.

@github-actions github-actions bot added the size/S Small PR: 100-299 lines changed label Apr 1, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 25.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.32%. Comparing base (47c3651) to head (c56ee00).
⚠️ Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
pkg/vmcp/server/server.go 26.08% 17 Missing ⚠️
cmd/vmcp/app/commands.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4479      +/-   ##
==========================================
+ Coverage   69.30%   69.32%   +0.01%     
==========================================
  Files         502      502              
  Lines       51632    51655      +23     
==========================================
+ Hits        35786    35808      +22     
  Misses      13077    13077              
- Partials     2769     2770       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yrobla yrobla force-pushed the issue-4420-sm-migration-v1 branch from 4acfc6e to 11869f0 Compare April 1, 2026 15:58
@yrobla yrobla requested review from amirejaz and jerm-dro as code owners April 1, 2026 15:58
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 1, 2026
@yrobla yrobla requested a review from Copilot April 1, 2026 16:00
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 1, 2026
@yrobla yrobla force-pushed the issue-4420-sm-migration-v1 branch from 11869f0 to ad66b93 Compare April 1, 2026 16:01
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 1, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates vMCP session persistence away from a direct *transportsession.Manager dependency by wiring session metadata storage through the DataStorage interface, enabling optional Redis-backed cross-pod session restore, and updates token-refresh singleflight tests to be deterministic.

Changes:

  • Add SessionStorage configuration to vMCP server config and construct DataStorage via a new buildSessionDataStorage helper (local memory or Redis).
  • Add test-only synchronization hooks to MonitoredTokenSource to make singleflight-deduplication tests deterministic.
  • Plumb SessionStorage from CLI config (commands.go) into the vMCP server configuration.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
pkg/vmcp/server/server.go Adds SessionStorage config and builds DataStorage (local/Redis) during server construction.
pkg/auth/monitored_token_source.go Adds test hooks around singleflight entry to coordinate concurrent callers deterministically.
pkg/auth/monitored_token_source_test.go Refactors the concurrency test to use a two-phase barrier and tighter call-count assertions.
cmd/vmcp/app/commands.go Passes SessionStorage from CLI config into the server config.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jerm-dro
jerm-dro previously approved these changes Apr 1, 2026
Base automatically changed from issue-4420-sm-migration to main April 2, 2026 07:21
@yrobla yrobla dismissed jerm-dro’s stale review April 2, 2026 07:21

The base branch was changed.

@yrobla yrobla closed this Apr 2, 2026
@yrobla yrobla force-pushed the issue-4420-sm-migration-v1 branch from ad66b93 to 47c3651 Compare April 2, 2026 07:56
@yrobla yrobla reopened this Apr 2, 2026
@github-actions github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 2, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add SessionStorage *vmcpconfig.SessionStorageConfig to vmcpserver.Config
and introduce buildSessionDataStorage:
- provider nil/"memory": uses LocalSessionDataStorage (default, unchanged)
- provider "redis": constructs RedisSessionDataStorage with address, DB,
  and key prefix from SessionStorageConfig; password from
  THV_SESSION_REDIS_PASSWORD; empty KeyPrefix defaults to
  "thv:vmcp:session:"; unknown provider values return an error
- Pass cfg.SessionStorage from commands.go so the operator-provided Redis
  config reaches the server

Related-to: #4220
@yrobla yrobla force-pushed the issue-4420-sm-migration-v1 branch from 8dd7805 to c56ee00 Compare April 2, 2026 08:33
@github-actions github-actions bot added size/XS Extra small PR: < 100 lines changed and removed size/S Small PR: 100-299 lines changed size/XS Extra small PR: < 100 lines changed labels Apr 2, 2026
@yrobla yrobla merged commit 8b36d99 into main Apr 7, 2026
42 checks passed
@yrobla yrobla deleted the issue-4420-sm-migration-v1 branch April 7, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write unit tests for horizontal scaling operator behaviors (THV-0047)

4 participants