Remove redundant storage.Load from NotifyBackendExpired#4779
Conversation
yrobla
requested review from
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
There was a problem hiding this comment.
Pull request overview
Refactors NotifyBackendExpired to avoid an unnecessary storage read by having callers pass the session metadata they already have when detecting backend expiry (per issue #4667), while keeping the operation best-effort and relying on SET XX semantics to avoid resurrection.
Changes:
- Update
NotifyBackendExpiredto accept a caller-supplied metadata map and treatnilas a silent no-op. - Simplify timeout documentation now that the internal
Loadcall is removed. - Update unit tests and the
SessionManagerinterface to match the new signature.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/vmcp/server/sessionmanager/session_manager.go | Changes NotifyBackendExpired to take metadata directly and removes the internal storage Load. |
| pkg/vmcp/server/sessionmanager/session_manager_test.go | Updates tests to pass metadata into NotifyBackendExpired. |
| pkg/vmcp/server/session_manager_interface.go | Updates the SessionManager interface and docstring for the new NotifyBackendExpired signature. |
💡 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 #4779 +/- ##
==========================================
+ Coverage 68.89% 68.98% +0.08%
==========================================
Files 518 517 -1
Lines 54921 54736 -185
==========================================
- Hits 37840 37759 -81
+ Misses 14170 14074 -96
+ Partials 2911 2903 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean refactoring that removes an unnecessary storage round-trip with no behavior change.
Extended reasoning...
Overview
The PR modifies three files in the vMCP session manager package: the SessionManager interface, its implementation, and the corresponding tests. The core change is to NotifyBackendExpired's signature — the function previously loaded session metadata from storage itself; now the caller supplies the metadata it already holds, eliminating a redundant Redis round-trip.
Security risks
None. The storage update uses SET XX (conditional update), enforced by updateMetadata, so a concurrently deleted session key cannot be resurrected. No auth, crypto, or permissions logic is touched.
Level of scrutiny
Low-to-medium. This is a refactoring with no user-facing behavior change. The interface signature change is technically breaking, but a search of the codebase confirms there are no production callers outside the implementation and its tests, so the risk surface is limited. The map mutation in NotifyBackendExpired (delete + reassign on the caller-supplied map) is a subtle API contract worth noting, but it poses no immediate risk given current usage patterns. Tests comprehensively cover nil, empty, terminated, same-pod, and cross-pod scenarios.
Other factors
Unit tests are thorough and were updated in lock-step with the implementation. The PR was linted and unit-tested. No CODEOWNER-owned files are touched.
Accept the caller's metadata map directly rather than re-loading it from storage. Any caller that detects a backend expiry already holds the session metadata (e.g. from MultiSession.GetMetadata), so the extra round-trip is unnecessary. Passing nil is treated as a silent no-op. Safety is preserved: updateMetadata uses SET XX, so a concurrently terminated (deleted) session is never resurrected. Closes #4667
github-actions
bot
added
size/S
Summary
Accept the caller's metadata map directly rather than re-loading it from storage. Any caller that detects a backend expiry already holds the session metadata (e.g. from MultiSession.GetMetadata), so the extra round-trip is unnecessary. Passing nil is treated as a silent no-op. Safety is preserved: updateMetadata uses SET XX, so a concurrently terminated (deleted) session is never resurrected.
Fixes #4667
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Implementation plan
Approved implementation plan
Special notes for reviewers