Skip to content

Resync session tools when backend health changes - #6196

Merged
aponcedeleonch merged 3 commits into
stacklok:mainfrom
premctl:vmcp-passthrough-list-changed
Aug 24, 2026
Merged

Resync session tools when backend health changes#6196
aponcedeleonch merged 3 commits into
stacklok:mainfrom
premctl:vmcp-passthrough-list-changed

Conversation

@premctl

@premctl premctl commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

A vMCP session's advertised tool catalog is derived once at registration, and the existing resync machinery (#5748) only re-derives it when a connected backend itself emits notifications/tools/list_changed. A backend flipping unhealthy → healthy (or the group's backend set changing) emits nothing — the health monitor detected the transition but nothing consumed it — so already-connected sessions kept serving the stale catalog until they reconnected, even though capability aggregation already health-filters the backend set. This is PR 1 of the two-PR split agreed with @aponcedeleonch in #5786: passthrough mode only; the optimizer-mode catalog rebuild is the follow-up PR.

  • pkg/vmcp/health: add Monitor.OnChange(fn ChangeListener) (also on health.Reporter), fired when a backend's advertisability flips — a transition across the healthy/degraded ⇄ unhealthy/unknown/unauthenticated partition, detected at the statusTracker.RecordSuccess/RecordFailure transition points, mirroring filterHealthyBackends' inclusion rule — and when UpdateBackends adds/removes backends. Delivery is debounced to the monitor's check interval (leading edge immediate, in-window changes coalesced into one trailing delivery carrying a monotonic generation), so a flapping backend or multi-backend partition cannot storm listeners. Listeners run off the health-check path; Stop waits for in-flight deliveries.
  • pkg/vmcp/server: Serve subscribes via the core-owned monitor. The server keeps a registry of each live session's KindTools resync worker — the same per-session coalescing worker the backend-notification path builds, so identity/forwarded-header capture, the liveness guard, capability-cache invalidation, replace semantics, and the SDK's automatic downstream notifications/tools/list_changed emission are all shared — and triggers each on delivery. Sessions register on successful registration, deregister on server-observed termination paths, and are pruned lazily when a triggered resync finds them gone (TTL expiry / SDK-initiated DELETE end sessions without server involvement).
  • Passthrough-only gate: with the optimizer enabled the fan-out is a no-op (the advertised find_tool/call_tool meta-tools don't change on a health flip; rebuilding their backing index is PR 2).
  • docs/arch/10-virtual-mcp-architecture.md: new "Health-driven tools resync" section.

Part of #5786 (PR 1 of 2 — do not auto-close; PR 2 covers optimizer mode)

Type of change

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

Test plan

  • Unit tests (task test) — full suite green with race detection: debounce semantics (leading/trailing coalescing, stop cancels pending delivery, generations strictly increasing); statusTracker advertisability-transition matrix; monitor fires OnChange on fail→recover and on UpdateBackends add/remove, and stays quiet in steady state; server fan-out resyncs a live session (gains recovered tool, drops failed tool, invalidates capability cache), coalesces a 10-delivery burst into two re-derivations, no-ops in optimizer mode, and prunes dead sessions
  • E2E tests — new virtualmcp_health_list_changed_test.go spec run against a Kind cluster (kind-setup-e2e + operator-deploy-local): a Legacy session initialized while a backend is broken receives notifications/tools/list_changed on its standalone SSE stream when the backend recovers, then lists and successfully calls the recovered backend's tool on the same session without reconnecting
  • Linting (task lint-fix)
  • Manual testing (describe below)

Does this introduce a user-facing change?

Yes. Connected vMCP client sessions (passthrough mode) now receive notifications/tools/list_changed and an updated tools/list when a backend recovers/fails health checks or when the group's backend set changes, instead of serving the registration-time snapshot until reconnect.

Implementation plan

Approved implementation plan (agreed in #5786, PR 1 scope)
  • pkg/vmcp/health/monitor.go: add type ChangeListener func(gen uint64) and func (m *Monitor) OnChange(fn ChangeListener). Fire it from the statusTracker transition points (healthy⇄unhealthy) and from UpdateBackends (add/remove), debounced so a flapping backend doesn't storm sessions. Callback on Monitor rather than a channel, per the thread.
  • Core/aggregation: re-aggregate against the new healthy set and invalidate the cached merge so it doesn't keep serving the stale aggregation.
  • Server + session registry: re-apply each live session's advertised tools post-init (add new, remove gone), once the session's notification channel is live — not via the registration-time setSessionToolsDirect bypass as-is, per the thread's note on why that bypass exists.
  • Emit notifications/tools/list_changed to clients after applying, gated on the server advertising tools.listChanged.
  • Concurrency: single writer of the catalog view; sessions apply under their existing session lock; skip sessions already up to date; an in-flight call_tool never resolves against a mismatched route.
  • PR 1 = passthrough mode only; PR 2 = optimizer-mode find_tool/call_tool catalog rebuild.

Special notes for reviewers

The agreed design predates #5748/#5969 landing, and several of its steps are now provided by machinery that didn't exist when the thread discussion happened — this PR wires the health monitor into that machinery rather than re-implementing those steps:

  • "Re-run AggregateCapabilities and atomically swap the catalog + routing table, bump a generation" — the core is now stateless (P2.5 Move AS runner, status reporter, optimizer, health monitor under Serve #5443/P3.2 Reduce server.New body to the wrapper #5445: it health-filters and aggregates per call; "the core filters, Serve caches"), so there is no stored catalog to swap and no background re-aggregation to run. A background re-aggregation would in fact be wrong today: the capability cache and outbound backend auth are keyed on per-identity context (Consume backend notifications in vMCP and propagate list_changed #5748), which a background subscriber doesn't have — only the per-session workers carry the captured identity. The atomicity concern is inherently satisfied (each call derives one consistent aggregation+routing view), and the cache serves no stale merge (its key includes the backend-ID set, which a health flip changes; the shared resync path additionally invalidates it).
  • "Apply via the existing setSessionToolsDirect path" / "gate on advertising tools.listChanged"Consume backend notifications in vMCP and propagate list_changed #5748 already built the correct post-init apply path (resyncSessionTools: REPLACE semantics, so removals propagate, unlike setSessionToolsDirect's registration-time merge) and already flipped WithToolCapabilities(true); the SDK emits the notification to each session whose tool store changes, so no explicit SendNotificationToAllClients call is needed (it would duplicate).
  • Generation counter — kept as the ChangeListener payload (monotonic, coalesced by debouncing) for correlation; the "skip an already-applied session" role is subsumed by the per-session workers' dirty-flag coalescing.
  • The debounce window is the monitor's own CheckInterval (default 30s, same default as the status-reporting interval the thread referenced) — transitions are detected at check cadence, so this is the natural window, and it avoids threading the server-layer reporting interval into the core-owned monitor.
  • Registry lifecycle: sessions deregister eagerly on every termination path the server observes — including SDK-initiated HTTP DELETE, via a thin SessionIdManager wrapper (pruneOnTerminateSessionIDManager), since that path otherwise reaches the session manager without passing through server code. Only TTL expiry is pruned lazily (worker's liveness guard on the next fan-out); such an entry retains the worker closure (SDK session + captured identity/headers) until then, which the registry doc comment states explicitly.
  • No eager cache purge is needed on a fan-out with zero live sessions: the capability cache is keyed on (identity, forwarded headers, filtered backend-ID set), so a health flip changes the key and any session registering after the flip re-sweeps by construction; pre-flip entries age out via TTL. The per-session resync path still purges before re-deriving (shared Consume backend notifications in vMCP and propagate list_changed #5748 behavior, needed there because a backend's content can change under an unchanged backend set).
  • UpdateBackends property changes (URL/transport) intentionally do not notify — membership-only, per the agreed scope; noted in the architecture doc.
  • E2E covers unhealthy→healthy on a live session; the healthy→unhealthy direction (tool removal) exercises the same replace path and is covered by the server unit tests.
  • Size: 437 changed lines across 8 files excluding tests/docs — marginally over the 400-line guideline. A meaningful share is doc comments on the new concurrency surfaces; splitting the monitor half from the server half would leave neither independently useful, so I kept the scope agreed in the issue thread as one PR. Happy to split if preferred.

Generated with Claude Code

@amirejaz amirejaz 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.

Really thorough PR — thanks for adapting the agreed design to the machinery that landed after the #5786 discussion (reusing the #5748/#5969 per-session resync workers rather than re-implementing the re-apply/notify steps is exactly right, and the "Special notes for reviewers" mapping made this easy to verify). Correctness and lifecycle hold up on my read: no synchronous re-entry into the monitor locks from notify() so Stop can't deadlock, eager deregistration on every server-observed termination path plus the lazy liveness-guard prune, and advertisable matching filterHealthyBackends' inclusion rule.

A couple of small, non-blocking items (inline). Nothing here needs to hold the PR up if you'd rather defer.

One more optional note: the tools resync worker is registered (healthResync.add) just before injectCoreSessionCapabilities runs, so a health flip in that narrow window could run SetSessionTools (REPLACE) concurrently with registration's setSessionToolsDirect (MERGE). Both derive from the live health-filtered core view and the SDK store is internally locked, so it self-heals on the next fan-out — and registering early is a deliberate "don't miss a change" choice — but a one-line comment acknowledging the overlap would help the next reader.

Comment thread pkg/vmcp/health/status.go Outdated
Comment thread pkg/vmcp/server/serve_health_resync.go
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.63866% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.71%. Comparing base (7af27d3) to head (bcc9367).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
pkg/vmcp/health/status.go 87.50% 2 Missing ⚠️
pkg/vmcp/health/change_notifier.go 97.29% 1 Missing ⚠️
pkg/vmcp/server/server.go 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6196      +/-   ##
==========================================
+ Coverage   73.08%   77.71%   +4.62%     
==========================================
  Files         745      758      +13     
  Lines       78804    72887    -5917     
==========================================
- Hits        57597    56644     -953     
+ Misses      17177    16238     -939     
+ Partials     4030        5    -4025     

☔ 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.

premctl added a commit to premctl/toolhive that referenced this pull request Aug 10, 2026
Review feedback on stacklok#6196:

- Extract vmcp.BackendHealthStatus.Advertisable as the single inclusion
  rule called by both the core's filterHealthyBackends and the health
  monitor's change detection, so the two hand-kept copies cannot
  silently diverge. Lock the rule with a table test.
- Skip healthResync registration for optimizer-mode sessions: the
  health fan-out is a no-op there in PR1, so registering only retained
  the worker closure until termination. The fan-out gate stays as
  defense in depth; PR2 removes both together.
- Document the benign overlap between an early health fan-out
  (replace) and registration's capability injection (merge).

Signed-off-by: Prem Kumar Sompura <prem_sompura@hotmail.com>
@premctl

premctl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @amirejaz — all three items are addressed in de7111d:

  • Shared inclusion rule: extracted as vmcp.BackendHealthStatus.Advertisable(), called by both filterHealthyBackends and the tracker's change detection, with a per-status table test locking the rule (details in the inline reply).
  • Optimizer-mode registration: skipped the healthResync.add when the optimizer is enabled rather than documenting the retention — nothing lingers now, and the fan-out gate remains as defense in depth (inline reply).
  • Registration-window overlap: added the comment at the healthResync.add site acknowledging that an early fan-out's SetSessionTools (replace) can overlap capability injection's merge, why it's benign (both derive from the live health-filtered core view, SDK store is locked), and that it self-heals on the next fan-out.

Unit suite re-run green with the race detector (task test, exit 0) and task lint-fix clean.

@amirejaz

Copy link
Copy Markdown
Contributor

Thanks, the new changes to address the feedback looks good, need to resolve the conflicts first.

@premctl
premctl force-pushed the vmcp-passthrough-list-changed branch from de7111d to d39e2b2 Compare August 12, 2026 10:16
premctl added a commit to premctl/toolhive that referenced this pull request Aug 12, 2026
Review feedback on stacklok#6196:

- Replace the status tracker's hand-kept copy of the inclusion rule
  with health.ShouldAdvertise, the predicate filterHealthyBackends
  already calls (added on main by stacklok#6162 with identical semantics
  while this PR was in review), so the monitor's change detection and
  the catalog filter cannot silently diverge.
- Skip healthResync registration for optimizer-mode sessions: the
  health fan-out is a no-op there in PR1, so registering only retained
  the worker closure until termination. The fan-out gate stays as
  defense in depth; PR2 removes both together.
- Document the benign overlap between an early health fan-out
  (replace) and registration's capability injection (merge).

Signed-off-by: Prem Kumar Sompura <prem_sompura@hotmail.com>
@premctl

premctl commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main and resolved the conflicts (d39e2b2).

One note on the shared-predicate item: while this PR was in review, #6162 landed on main with its own shared predicate, health.ShouldAdvertise, with identical semantics to the vmcp.BackendHealthStatus.Advertisable() method I had added. Rather than shipping two competing predicates, the rebased commit drops mine and points the status tracker's change detection at health.ShouldAdvertise — so filterHealthyBackends, session-open gating (ShouldOpenSession), and OnChange detection now all read from pkg/vmcp/health/policy.go, which already has per-status tests from #6162. The optimizer-mode registration skip and the overlap comment are unchanged.

task lint-fix clean and full unit suite green with the race detector on the rebased branch.

Connected vMCP sessions snapshot their tool catalog at registration
and are only resynced when a backend itself emits tools/list_changed.
A backend flipping unhealthy to healthy (or the group's backend set
changing) emits nothing, so live sessions kept serving the stale
catalog until they reconnected, even though capability aggregation
already health-filters the backend set.

Give the health monitor a debounced OnChange callback fired when a
backend's advertisability flips (the healthy/degraded boundary the
aggregation filters by) or when UpdateBackends adds/removes backends.
Serve subscribes and fans each delivery out to every live session's
existing tools resync worker, which re-derives the advertised set
under the session's captured identity, replaces the session tool
store, and lets the SDK emit notifications/tools/list_changed to the
client.

Passthrough mode only: with the optimizer enabled the fan-out is a
no-op, since rebuilding the find_tool/call_tool backing index is the
follow-up half of stacklok#5786.

Part of stacklok#5786

Signed-off-by: Prem Kumar Sompura <prem_sompura@hotmail.com>
Review feedback on stacklok#6196:

- Replace the status tracker's hand-kept copy of the inclusion rule
  with health.ShouldAdvertise, the predicate filterHealthyBackends
  already calls (added on main by stacklok#6162 with identical semantics
  while this PR was in review), so the monitor's change detection and
  the catalog filter cannot silently diverge.
- Skip healthResync registration for optimizer-mode sessions: the
  health fan-out is a no-op there in PR1, so registering only retained
  the worker closure until termination. The fan-out gate stays as
  defense in depth; PR2 removes both together.
- Document the benign overlap between an early health fan-out
  (replace) and registration's capability injection (merge).

Signed-off-by: Prem Kumar Sompura <prem_sompura@hotmail.com>
@premctl
premctl force-pushed the vmcp-passthrough-list-changed branch from d39e2b2 to 26d9a35 Compare August 19, 2026 11:00

@aponcedeleonch aponcedeleonch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice piece of work, and the design decision I most want to call out as right is reusing listChangedResyncWorker instead of building a second resync path. Identity capture, the liveness guard, replace semantics, and letting the SDK emit notifications/tools/list_changed off a tool-store change all come for free, and that last part in particular keeps this much smaller than it could have been.

Comment thread pkg/vmcp/health/status.go Outdated
Comment thread pkg/vmcp/health/status.go Outdated
Comment thread pkg/vmcp/server/server.go
Comment thread pkg/vmcp/server/serve_health_resync.go
Review feedback on stacklok#6196, round two:

- A previously-untracked backend's first result no longer notifies
  unconditionally. A first success is quiet: the registry fallback the
  aggregation had been serving is advertisable in practice and the
  membership notification already covers the add, so a backend's first
  probe no longer spends the debounce leading edge on a no-op delivery.
  A below-threshold first failure is quiet too: withdrawing a workload
  still starting when the registry first lists it flapped every
  connected session despite UnhealthyThreshold promising tolerance.
  The withdrawal is reported at the genuine threshold crossing instead
  (RecordFailure special-cases a never-successful backend, which pure
  suppression would have left advertised forever), and recovery from
  tracked Unknown still reports, restoring sessions that re-derived
  during the quiet window.
- Gate healthResync.add on health monitoring being enabled: with no
  monitor there is no OnChange subscriber, so no fan-out would ever run
  the registry's lazy prune, and a session ending without a
  server-observed Terminate retained its worker closure permanently.
- Purge the capability cache once per fan-out delivery, in the
  listener, instead of once per session run: the cache key already
  hashes the health-filtered backend-ID set, so per-run purges only
  evicted what a sibling session's sweep had just repopulated, forcing
  one full backend sweep per session where later same-identity
  sessions could share one. The backend-notification path keeps its
  per-run purge (content changes under an unchanged key) via a
  coalescing-safe purge flag on the resync worker.

Signed-off-by: Prem Kumar Sompura <prem_sompura@hotmail.com>

@aponcedeleonch aponcedeleonch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! Thanks for the contribution

@aponcedeleonch
aponcedeleonch merged commit dd0ea20 into stacklok:main Aug 24, 2026
46 of 47 checks passed
@github-actions github-actions Bot mentioned this pull request Aug 26, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants