Skip to content

Evict vMCP sessions when a backend is dropped - #6549

Merged
tgrunnagle merged 7 commits into
mainfrom
spring-lint
Sep 9, 2026
Merged

Evict vMCP sessions when a backend is dropped#6549
tgrunnagle merged 7 commits into
mainfrom
spring-lint

Conversation

@tgrunnagle

@tgrunnagle tgrunnagle commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

When a backend is removed from a vmcp.DynamicRegistry (a generation swap), any
already-open per-session connection to that dropped backend — most visibly a
long-lived SSE server-push stream — persisted until the owning client session
ended. New routing stopped correctly, but nothing reclaimed the live connection,
leaking a resource for the remaining lifetime of every affected session.

This reclaims those connections promptly by reusing the existing
lazy-eviction / RestoreSession machinery, rather than mutating the read-only
MultiSession in place (vMCP anti-pattern #10, "reconstruct, don't mutate"):

  • ValidatingCache.RemoveMatching — predicate-based bulk eviction. The LRU's
    eviction callback only buffers evicted entries; every mutating path drains the
    buffer and invokes onEvict after releasing the cache lock, so slow teardown
    work in onEvict (such as closing a hung backend connection) never runs under
    the lock and cannot stall concurrent session creation on other keys.
  • SessionManager.EvictStaleSessions — evicts every live session holding a
    backend absent from the current registry; onEvict closes its connections and
    the next request rebuilds the session minus the dropped backend via
    RestoreSession.
  • reconcileSessionsOnRegistryChange — a background loop that watches the
    DynamicRegistry version counter and evicts on change, independent of status
    reporting, and a no-op for static registries.

Closes #6546

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Unit tests added for:

  • RemoveMatching — happy path, no-match no-op, phase-2 re-check guard against a
    concurrent replacement, a -race concurrency smoke test, and a slow-onEvict
    test proving an unaffected key's Set completes while a slow eviction is in
    flight (i.e. onEvict is off the lock).
  • EvictStaleSessions — evicts stale sessions while leaving unaffected sessions
    in place.
  • reconcileSessionsOnRegistryChange — evicts on a registry version change and
    returns immediately for a static (non-dynamic) registry.

task lint-fix is clean and the affected packages' tests pass, including full
-race runs of pkg/cache and pkg/vmcp/server.

Changes

File Change
pkg/cache/validating_cache.go Add RemoveMatching predicate-based bulk eviction; run onEvict off the cache lock via buffer-and-drain on all eviction paths.
pkg/vmcp/session/factory.go Add ParseBackendIDs, the single decoder for the MetadataKeyBackendIDs wire format; use it in filterBackendsByStoredIDs.
pkg/vmcp/server/sessionmanager/session_manager.go Add EvictStaleSessions + referencesMissingBackend (using ParseBackendIDs); downgrade routine eviction logs from WARN/INFO to DEBUG.
pkg/vmcp/server/session_manager_interface.go Add EvictStaleSessions to the SessionManager interface.
pkg/vmcp/server/session_reconcile.go New background reconcile loop watching the DynamicRegistry version; poll interval injected as a parameter.
pkg/vmcp/server/server.go Start/stop the reconcile loop for dynamic registries during Start.

Does this introduce a user-facing change?

No. Behavior is internal to vMCP session management; there is no API or
configuration change.

Special notes for reviewers

Accepted deviation from the issue's acceptance criteria. The issue lists
"Other backends in the same session are unaffected" as a done-when criterion.
That is not literally met here. The maintainer explicitly chose the "A'" approach
(reuse eviction + RestoreSession) over the issue's Option A (a targeted
per-backend CloseBackend), so evicting a session rebuilds it and its surviving
backends briefly disconnect and reconnect on the next request. This trade-off was
deliberately accepted to avoid adding a mutating method to the read-only
MultiSession (anti-pattern #10). Recommend updating the issue's acceptance
criteria to record this decision.

Residual window. Eviction operates on sessions already in the cache. A
session whose RestoreSession is in flight when the drop commits may open a
connection to the soon-dropped backend and escape a given pass; its stale
connection is then reclaimed on the next registry change, on checkSession
metadata drift, or ultimately at session end — i.e. it never regresses past the
original session-lifetime bound. This is the accepted best-effort of a polling
model and is documented at the method.

Generated with Claude Code

When a backend is removed from a vmcp.DynamicRegistry (a generation
swap), any already-open per-session connection to that backend — most
visibly a long-lived SSE server-push stream — persisted until the
owning client session ended. New routing stopped correctly, but the
component that dropped the backend had no way to reclaim the live
connection.

Reuse the existing lazy-eviction/RestoreSession machinery rather than
mutating a live session in place (vMCP anti-pattern #10):

- Add ValidatingCache.RemoveMatching for predicate-based bulk eviction.
- Add SessionManager.EvictStaleSessions, which evicts every live session
  holding a backend absent from the registry; onEvict closes its backend
  connections and the next request rebuilds it without the dropped
  backend via RestoreSession.
- Watch the DynamicRegistry version in reconcileSessionsOnRegistryChange
  and evict on change, independent of status reporting.

Implements #6546.
- Log routine session eviction at DEBUG, not WARN: a generation swap is
  an expected transition and the session is recoverable via restore, so
  a per-session WARN burst was noise (a real close failure still WARNs).
- RemoveMatching now snapshots candidate keys, then acquires the cache
  lock once per entry instead of holding it across the whole scan, so a
  bulk eviction's connection teardown no longer starves new-session
  creation for the full burst.
- Document the residual concurrent-restore window in EvictStaleSessions:
  a session mid-restore when the drop commits falls back to the original
  session-lifetime bound, never worse.
Cover the split-lock rework's phase-2 guard directly: a key selected in
the snapshot whose predicate flips to false before removal must be left
in place, not counted, not closed. Add a -race concurrency smoke test
running RemoveMatching alongside Set/Get to pin the no-deadlock,
no-panic guarantees.
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Sep 8, 2026
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.77%. Comparing base (2533388) to head (4183f53).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6549      +/-   ##
==========================================
+ Coverage   78.69%   78.77%   +0.08%     
==========================================
  Files         777      778       +1     
  Lines       76797    76887      +90     
==========================================
+ Hits        60432    60571     +139     
+ Misses      16360    16311      -49     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tgrunnagle tgrunnagle left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-Agent Consensus Review

Agents consulted: concurrency-correctness-reviewer, security-resource-lifecycle-reviewer, test-coverage-quality-reviewer, general-code-quality-reviewer

Consensus Summary

# Finding Consensus Severity Action
1 New test races with a pre-existing test on shared versionPollInterval 10/10 HIGH Fix
2 Cache-wide lock held during synchronous sess.Close(), now targeted at just-dropped backends 9/10 HIGH Fix
3 Duplicated comma-separated backend-ID parsing logic across two packages 8/10 MEDIUM Fix
4 EvictStaleSessions INFO log contradicts this PR's own adjacent WARN\u2192DEBUG rationale 8/10 MEDIUM Fix
5 RemoveMatching doc comment overclaims the locking guarantee; no test exercises a slow onEvict 7/10 LOW Fix

Overall

This PR reclaims per-session backend connections in vMCP (notably lingering SSE streams) when a backend is dropped from a DynamicRegistry, via RemoveMatching bulk cache eviction, EvictStaleSessions, and a background reconciliation loop. The approach is sound and deliberately reuses the existing lazy-eviction/RestoreSession machinery instead of adding a mutating method to MultiSession, consistent with the reconstruct-don't-mutate convention. The core logic was traced end-to-end and holds up: the phase-1/phase-2 re-check in RemoveMatching correctly closes its TOCTOU window, the backend-ID identifier space matches between registry membership and session metadata, and a dropped backend's ID is genuinely excluded from session metadata on the next restore.

However, the new test in session_reconcile_test.go introduces a real data race against a pre-existing test in status_reporting_test.go (both mutate the shared package-level versionPollInterval under t.Parallel()) — reproduced directly with go test -race, and confirmed independently by three review agents. This breaks task test as submitted and needs to be fixed before merge. Separately, EvictStaleSessions's bulk eviction runs RemoveMatching's per-entry cache-lock-held Close() specifically against backends that were just dropped — the scenario most likely to involve a slow or unresponsive backend — which can stall the whole node-local session cache for up to one request-timeout per stale session in a reconciliation pass. That lock-holding pattern already exists for single-entry eviction elsewhere in the cache, but this PR meaningfully widens its blast radius; worth addressing here or as an immediate follow-up. The remaining findings are minor: duplicated backend-ID parsing logic across two packages, an INFO log level that contradicts this PR's own adjacent reasoning for a WARN→DEBUG downgrade a few lines above, and a doc comment that slightly overstates the locking guarantee it documents.


Generated with Claude Code

Comment thread pkg/vmcp/server/session_reconcile_test.go Outdated
Comment thread pkg/cache/validating_cache.go
Comment thread pkg/vmcp/server/sessionmanager/session_manager.go
Comment thread pkg/vmcp/server/sessionmanager/session_manager.go Outdated
Comment thread pkg/cache/validating_cache.go Outdated
Addresses #6549 review comments:
- HIGH pkg/cache/validating_cache.go (3960349464): RemoveMatching held the
  cache lock while sess.Close() ran synchronously via the LRU eviction
  callback, so a hung just-dropped backend could block Get/Set for every
  session on the node. The LRU callback now only buffers evicted entries;
  every mutating path drains the buffer and invokes the user onEvict after
  releasing the lock, so onEvict never runs under the cache lock (all paths).
- LOW pkg/cache/validating_cache.go (3960349496): tighten the RemoveMatching
  doc to state onEvict runs off the lock, and add a slow-onEvict test proving
  an unaffected key's Set completes while a slow eviction is in flight.
Addresses #6549 review comments:
- HIGH pkg/vmcp/server/session_reconcile_test.go (3960349452): the new
  reconcile test and the pre-existing status-reporting test both ran under
  t.Parallel() and mutated the shared package-level versionPollInterval,
  a genuine data race that poisoned the whole -race package run.
  reconcileSessionsOnRegistryChange now takes the poll interval as a
  parameter (Start passes versionPollInterval); the test passes its own
  value and no longer touches the package var.
Addresses #6549 review comments:
- MEDIUM pkg/vmcp/server/sessionmanager/session_manager.go (3960349470):
  extract vmcpsession.ParseBackendIDs next to the MetadataKeyBackendIDs
  constant as the single decoder for that wire format, and use it from both
  filterBackendsByStoredIDs and referencesMissingBackend instead of
  re-implementing the split/trim/skip-empty parse in two packages.
- MEDIUM pkg/vmcp/server/sessionmanager/session_manager.go (3960349474):
  EvictStaleSessions logged its aggregate eviction count at INFO, which
  contradicts this PR's adjacent WARN->DEBUG downgrade of the same event and
  the "INFO sparingly" convention; lower it to DEBUG.
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed labels Sep 8, 2026
@tgrunnagle
tgrunnagle marked this pull request as ready for review September 8, 2026 17:37
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Sep 8, 2026
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/L Large PR: 600-999 lines changed labels Sep 8, 2026

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via multi-agent pass (concurrency/code-quality, ToolHive conventions, security) covering the cache buffer-then-drain eviction rework and the vMCP session reconciliation on backend removal.

No blocking issues found:

  • Lock ordering (mu -> evictMu) verified one-directional, no cycle
  • RemoveMatching's phase-2 re-check correctly closes the TOCTOU window against concurrent Set
  • SessionManager.EvictStaleSessions has a single implementer, updated atomically with test stubs
  • Version-poll pattern matches the existing periodicStatusReporting idiom
  • No security regression: eviction only reclaims idle cached connections (not a per-request auth re-check, which didn't exist before either); onEvict runs sequentially, no thundering-herd risk

Minor non-blocking note: RemoveMatching's phase-1 scan holds the cache lock for the full Keys()+Peek pass rather than per-key, so hold time scales with cache size, not match count — fine given this is documented as infrequent bulk reconciliation, not a hot path.

@tgrunnagle
tgrunnagle merged commit b6e804a into main Sep 9, 2026
46 checks passed
@tgrunnagle
tgrunnagle deleted the spring-lint branch September 9, 2026 14:30
@github-actions github-actions Bot mentioned this pull request Sep 10, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vMCP: dropped backend's open per-session connections are not torn down

2 participants