Revert "Add LRU capacity to ValidatingCache, remove sentinel pattern, add storage Update" #4730
Code review found 2 important issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | pkg/vmcp/server/sessionmanager/session_manager.go:613-630 |
updateMetadata: single context budget shared by Load (re-check) and Upsert |
| 🔴 Important | pkg/vmcp/server/sessionmanager/session_manager.go:775-788 |
DecorateSession: unconditional Upsert can resurrect storage record deleted by Terminate |
| 🟡 Nit | pkg/vmcp/server/sessionmanager/cache.go:122-131 |
RestorableCache.Get: LoadOrStore treats concurrent valid-V write as sentinel, calls onEvict and returns not-found |
Annotations
Check failure on line 630 in pkg/vmcp/server/sessionmanager/session_manager.go
claude / Claude Code Review
updateMetadata: single context budget shared by Load (re-check) and Upsert
In `updateMetadata`, a single `context.WithTimeout(context.Background(), notifyBackendExpiredTimeout)` context is created at line 614 and shared between both the cross-pod guard `storage.Load` call and the subsequent `storage.Upsert` call. If the Load consumes most of the 5-second budget (e.g., slow Redis or network jitter), the Upsert fails with a deadline-exceeded error even though the guard check succeeded and the write was safe to perform. The fix is to create two separate contexts—one for L
Check failure on line 788 in pkg/vmcp/server/sessionmanager/session_manager.go
claude / Claude Code Review
DecorateSession: unconditional Upsert can resurrect storage record deleted by Terminate
The revert replaces `storage.Update` (SET XX / conditional write) with unconditional `storage.Upsert` in `DecorateSession`, creating a window where a concurrent `Terminate()` can delete the storage key and then `DecorateSession`'s `Upsert` silently recreates it. On the same pod the consequence is bounded (the decorator wraps an already-closed session), but cross-pod `RestoreSession` calls will find the resurrected record in Redis and open fresh backend connections for a session that has been ter
Check warning on line 131 in pkg/vmcp/server/sessionmanager/cache.go
claude / Claude Code Review
RestorableCache.Get: LoadOrStore treats concurrent valid-V write as sentinel, calls onEvict and returns not-found
In `RestorableCache.Get` (cache.go lines 126–130), after `load()` completes, `LoadOrStore` returns `loaded=true` for any pre-existing value, but the code unconditionally calls `onEvict(key, v)` and returns `errSentinelFound` without checking whether the pre-existing value is a sentinel or a legitimate `V`. If a concurrent `CreateSession` stores a valid `MultiSession` between `storage.Upsert` (line 348) and `sessions.Store` (line 355), a simultaneous `GetMultiSession → loadSession → RestoreSessio