Add LRU capacity to ValidatingCache, remove sentinel pattern, add storage Update #4731
Code review found 3 important issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 3 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | pkg/vmcp/server/sessionmanager/session_manager_test.go:577-584 |
"closes MultiSession backend connections" test missing MetadataKeyTokenHash seed — Close() never called |
| 🔴 Important | pkg/vmcp/server/sessionmanager/session_manager_test.go:93-101 |
configurableFailDataStorage missing Update override breaks placeholder-termination fallback tests |
| 🔴 Important | pkg/cache/validating_cache.go:134-139 |
ValidatingCache: loaded value returned without onEvict when ContainsOrAdd winner is immediately evicted |
Annotations
Check failure on line 584 in pkg/vmcp/server/sessionmanager/session_manager_test.go
claude / Claude Code Review
"closes MultiSession backend connections" test missing MetadataKeyTokenHash seed — Close() never called
The test 'closes MultiSession backend connections' sets createdSess.EXPECT().Close().Return(nil).Times(1) but never seeds MetadataKeyTokenHash into storage, causing Terminate to take the Phase 1 placeholder path instead of the Phase 2 path that calls sm.sessions.Remove() → onEvict → Close(); gomock will fail at teardown with 'expected call to Close() Times(1), called 0 times'. The fix is to capture storage from newTestSessionManager (currently discarded as _) and call storage.Upsert with Metadat
Check failure on line 101 in pkg/vmcp/server/sessionmanager/session_manager_test.go
claude / Claude Code Review
configurableFailDataStorage missing Update override breaks placeholder-termination fallback tests
The test helper configurableFailDataStorage overrides Upsert, Create, and Delete — but not Update. This PR changed Terminate's Phase 1 path to call storage.Update instead of storage.Upsert, so the failure-injection counter (shouldFail) is never incremented for Update calls. Both fallback tests — "placeholder termination falls back to delete when upsert fails" and "placeholder termination fails when both upsert and delete fail" — silently pass the Update call through to the real storage, causing
Check failure on line 139 in pkg/cache/validating_cache.go
claude / Claude Code Review
ValidatingCache: loaded value returned without onEvict when ContainsOrAdd winner is immediately evicted
In the `!found` branch of `ValidatingCache.Get` (pkg/cache/validating_cache.go:135-138), when a concurrent winner is evicted by LRU pressure between `ContainsOrAdd` and `lruCache.Get`, the freshly-loaded value `v` is returned to the caller without calling `onEvict(key, v)`. For the session manager use case, `onEvict` is `sess.Close()` which releases backend HTTP connections — skipping it means those connections are never closed, leaking resources. The fix is to call `c.onEvict(key, v)` in the `!