Skip to content

Persist restored session metadata to Redis in loadSession#4842

Merged
yrobla merged 2 commits intomainfrom
issue-4836
Apr 16, 2026
Merged

Persist restored session metadata to Redis in loadSession#4842
yrobla merged 2 commits intomainfrom
issue-4836

Conversation

@yrobla
Copy link
Copy Markdown
Contributor

@yrobla yrobla commented Apr 15, 2026

Summary

After a successful RestoreSession call, write the restored session's metadata back to Redis so per-backend session IDs stay current. Backends that ignore Mcp-Session-Id hints (e.g. SSE transports) assign a fresh ID on every restore; without this write the stale IDs persisted in Redis indefinitely.

With Redis kept consistent, revert checkSession's comparison from the narrowed MetadataKeyBackendIDs-only workaround (introduced in #4724) back to maps.Equal, restoring full cross-pod consistency detection for any metadata drift.

Fixes #4836

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)

Changes

File Change

Does this introduce a user-facing change?

Implementation plan

Approved implementation plan

Special notes for reviewers

@yrobla yrobla requested a review from Copilot April 15, 2026 10:25
@github-actions github-actions bot added the size/S Small PR: 100-299 lines changed label Apr 15, 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 addresses cross-pod vMCP session consistency when sessions are restored from storage: after RestoreSession, it persists the restored session’s (potentially updated) metadata back into Redis/storage so per-backend session IDs don’t remain stale. With storage now kept current, checkSession reverts to full metadata comparison (maps.Equal) to detect any metadata drift across pods.

Changes:

  • Persist restored session metadata back to storage in loadSession after a successful RestoreSession.
  • Revert checkSession drift detection to full-map comparison via maps.Equal.
  • Add unit tests covering (1) restored-metadata persistence and (2) eviction on stale per-backend session IDs.

Reviewed changes

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

File Description
pkg/vmcp/server/sessionmanager/session_manager.go Persist restored metadata to storage; restore full metadata drift detection in checkSession.
pkg/vmcp/server/sessionmanager/session_manager_test.go Add tests ensuring restored metadata is written back and stale cached metadata triggers eviction.

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

Comment thread pkg/vmcp/server/sessionmanager/session_manager.go Outdated
Comment thread pkg/vmcp/server/sessionmanager/session_manager.go
Comment thread pkg/vmcp/server/sessionmanager/session_manager.go Outdated
@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 15, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 15, 2026

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.15%. Comparing base (12f25e5) to head (4b9aabb).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/vmcp/server/sessionmanager/session_manager.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4842      +/-   ##
==========================================
+ Coverage   69.12%   69.15%   +0.02%     
==========================================
  Files         531      531              
  Lines       55183    55196      +13     
==========================================
+ Hits        38146    38170      +24     
+ Misses      14113    14103      -10     
+ Partials     2924     2923       -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 requested a review from Copilot April 15, 2026 10:49
@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 15, 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 2 out of 2 changed files in this pull request and generated 5 comments.


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

Comment thread pkg/vmcp/server/sessionmanager/session_manager.go
Comment thread pkg/vmcp/server/sessionmanager/session_manager.go
Comment thread pkg/vmcp/server/sessionmanager/session_manager.go
Comment thread pkg/vmcp/server/sessionmanager/session_manager_test.go Outdated
Comment thread pkg/vmcp/server/sessionmanager/session_manager_test.go
@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 15, 2026
jerm-dro
jerm-dro previously approved these changes Apr 16, 2026
After a successful RestoreSession call, write the restored session's
metadata back to Redis so per-backend session IDs stay current.
Backends that ignore Mcp-Session-Id hints (e.g. SSE transports) assign
a fresh ID on every restore; without this write the stale IDs persisted
in Redis indefinitely.

With Redis kept consistent, revert checkSession's comparison from the
narrowed MetadataKeyBackendIDs-only workaround (introduced in #4724)
back to maps.Equal, restoring full cross-pod consistency detection for
any metadata drift.

Closes #4836
@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 16, 2026
@github-actions github-actions bot removed the size/S Small PR: 100-299 lines changed label Apr 16, 2026
@github-actions github-actions bot added the size/S Small PR: 100-299 lines changed label Apr 16, 2026
@yrobla yrobla merged commit bcc9588 into main Apr 16, 2026
52 of 53 checks passed
@yrobla yrobla deleted the issue-4836 branch April 16, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vmcp: loadSession never persists restored session metadata to Redis, causing stale per-backend session IDs

5 participants