Remove Upsert from DataStorage interface — closes #4726#4797
Conversation
yrobla
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
jerm-dro and
jhrozek
as code owners
There was a problem hiding this comment.
Pull request overview
This PR removes the Upsert method from the transport/session.DataStorage interface and its Redis/local implementations, updating contract and session-manager tests to use the safer Create/Update APIs introduced in #4669 and eliminating now-obsolete Upsert-specific tests.
Changes:
- Remove
UpsertfromDataStorageand deleteUpsertimplementations in Redis/local backends. - Update session manager tests and DataStorage contract tests to use
Create/Updatefor seeding and round-trip assertions. - Remove
Upsert-specific contract tests (overwrite + empty-ID) that no longer apply to the interface.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/server/sessionmanager/session_manager_test.go | Replaces test seeding calls from Upsert to Create/Update and updates failure-injection helpers. |
| pkg/transport/session/session_data_storage_test.go | Updates the DataStorage contract test suite to validate Create/Update semantics instead of Upsert. |
| pkg/transport/session/session_data_storage_redis.go | Removes Redis Upsert implementation; keeps Create (SET NX), Update (SET XX), and Load (GETEX). |
| pkg/transport/session/session_data_storage_local.go | Removes in-memory Upsert implementation; relies on Create/Update. |
| pkg/transport/session/session_data_storage.go | Removes Upsert from the interface and adjusts the contract documentation accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4797 +/- ##
==========================================
- Coverage 68.97% 68.96% -0.02%
==========================================
Files 517 517
Lines 54798 54811 +13
==========================================
Hits 37799 37799
- Misses 14087 14096 +9
- Partials 2912 2916 +4 ☔ 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 — straightforward removal of dead code with no behavioral changes; inline comments flag the doc nits.
Extended reasoning...
Overview
This PR removes the Upsert method from the DataStorage interface and both concrete implementations (LocalSessionDataStorage, RedisSessionDataStorage). All production callers were already migrated to Create/Update in #4669. Test fixtures are updated to use Create for new sessions and Update for sessions already written by CreateSession/Generate.
Security risks
None. Upsert was an unconditional-write footgun that could resurrect deleted sessions in concurrent scenarios; removing it strictly improves the security posture by narrowing the interface.
Level of scrutiny
Low. This is dead-code removal with mechanical test updates. The interface shrinks by one method; both implementations lose only the Upsert body. No logic changes to Create, Update, Load, or Delete. The two bugs flagged in inline comments are documentation-only nits with no runtime impact.
Other factors
Unit tests pass (task test checked). The change is self-contained within the session storage package and its test files. No CODEOWNERS paths appear to be touched. The doc inconsistencies (misleading Update guidance, omission of Update from TTL-refresh list) are captured as inline comments and are easy to address in a follow-up.
Upsert had zero production callers after #4669 converted all call sites to the safer Create/Update methods. Keeping it in the interface was a footgun that invited the unconditional-write pattern back. - Remove Upsert from DataStorage interface and both implementations - Update contract tests to use Create for round-trip and nil-metadata cases - Drop the two Upsert-specific contract tests (overwrite and empty-ID) that tested behaviour no longer in the interface - Replace test fixture setup calls with Create (new sessions) or Update (sessions already written by CreateSession/Generate)
github-actions
bot
added
size/S
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Upsert had zero production callers after #4669 converted all call sites to the safer Create/Update methods. Keeping it in the interface was a footgun that invited the unconditional-write pattern back.
Fixes #4726
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