Move TTL refresh into LocalStorage.Load, remove Session.Touch()#4337
Move TTL refresh into LocalStorage.Load, remove Session.Touch()#4337
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors session “last-access” / TTL refresh behavior to be owned by the storage backend: LocalStorage.Load now refreshes a session’s timestamp, and the Session.Touch() method is removed from the Session interface (with corresponding mock/test cleanup).
Changes:
- Remove
Touch()from theSessioninterface and from gomock session mocks. - Move LocalStorage “touch-on-access” behavior into
LocalStorage.Load, and removeManager.Get()’s call toTouch(). - Update unit/integration tests to stop expecting
Touch()calls; adjust LocalStorage tests to assert auto-touch on Load.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/types/mocks/mock_session.go | Removes mocked Touch() from MockMultiSession. |
| pkg/vmcp/server/sessionmanager/session_manager_test.go | Removes .EXPECT().Touch() boilerplate from session manager tests. |
| pkg/vmcp/server/session_management_integration_test.go | Removes .EXPECT().Touch() from integration test factories. |
| pkg/vmcp/server/integration_test.go | Removes .EXPECT().Touch() from integration tests. |
| pkg/vmcp/discovery/middleware_test.go | Removes .EXPECT().Touch() from middleware tests. |
| pkg/transport/session/storage_test.go | Updates LocalStorage tests to assert Load auto-touches and adjusts related comments. |
| pkg/transport/session/storage_local.go | Implements touch-on-Load via a package-local touchable interface. |
| pkg/transport/session/storage.go | Updates Storage.Load contract docs to describe TTL/last-access refresh behavior. |
| pkg/transport/session/manager.go | Removes Touch() from Session interface and from Manager.Get() behavior/docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba413e3fc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4337 +/- ##
==========================================
- Coverage 69.30% 68.89% -0.41%
==========================================
Files 478 479 +1
Lines 48425 48503 +78
==========================================
- Hits 33559 33415 -144
- Misses 12271 12320 +49
- Partials 2595 2768 +173 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Session.Touch() was a leaky abstraction — for Redis-backed sessions it was a no-op (GETEX already refreshes the TTL on read), while for LocalStorage the in-memory timestamp update was driven by the caller (Manager.Get) rather than the storage backend that actually needs it. TTL refresh is now a storage concern: LocalStorage.Load updates the session's last-access timestamp via a package-local `touchable` interface, and Manager.Get no longer calls Touch(). Touch() is removed from the Session interface and from the MockMultiSession mock, eliminating the `.EXPECT().Touch().AnyTimes()` boilerplate across all session-related tests.
Summary
Session.Touch() was a leaky abstraction — for Redis-backed sessions it was a no-op (GETEX already refreshes the TTL on read), while for LocalStorage the in-memory timestamp update was driven by the caller (Manager.Get) rather than the storage backend that actually needs it.
TTL refresh is now a storage concern: LocalStorage.Load updates the session's last-access timestamp via a package-local
touchableinterface, and Manager.Get no longer calls Touch(). Touch() is removed from the Session interface and from the MockMultiSession mock, eliminating the.EXPECT().Touch().AnyTimes()boilerplate across all session-related tests.Fixes #4320
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers