Repository navigation
chore(operator): drop the groupRef transition shim PAIINFRA-202 - #10
Merged
Merged
Conversation
Restore upstream v0.23.1 behaviour now that the string-form groupRef is no longer needed: MCPServer.spec.groupRef is the typed {name} object again, VirtualMCPServer.spec.groupRef is required again, and the shim tests are replaced by a groupRef schema envtest. CRDs regenerated with controller-gen v0.17.3.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
carlos-lopez-vecino
marked this pull request as ready for review
October 2, 2026 08:10
davidrubio-dp
approved these changes
Oct 2, 2026
jgpelaez
approved these changes
Oct 2, 2026
BraulioV
approved these changes
Oct 2, 2026
carlos-lopez-vecino
pushed a commit
that referenced
this pull request
Oct 8, 2026
* Evict vMCP sessions when a backend is dropped 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 stacklok#6546. * Address review: quieter eviction, shorter lock hold - 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. * Test RemoveMatching phase-2 re-check and concurrency 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. * Run cache onEvict off the lock during eviction Addresses stacklok#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. * Inject reconcile poll interval to fix test data race Addresses stacklok#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. * Share backend-ID parsing and quiet eviction log Addresses stacklok#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. * Fix codespell: paniced -> panicked in cache test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 2 (fork PR) of PAIINFRA-202, parent epic PAIINFRA-203. The shim was added in the v0.23.1 merge (#9) for PAIINFRA-200 so prod and stg operators could share the CRDs during the rollout. This PR restores upstream v0.23.1 behaviour.
Changes
MCPServer.spec.groupRef: removes+kubebuilder:validation:Schemaless/PreserveUnknownFieldsandMCPGroupRef.UnmarshalJSON(which accepted a bare string). The CRD schema is the typed{name}object again (name required, minLength 1) in both served versions (v1alpha1, v1beta1).VirtualMCPServer.spec.groupRef: required again (+kubebuilder:validation:Required, no omitempty).virtualmcpserver_types.gois identical to upstream v0.23.1.TestMCPGroupRefUnmarshalJSONandTestMCPGroupRefMarshalsObjectForm; replaces the envtest "groupRef transition shim" context with "groupRef schema" (rejects the string form, accepts the object form).files/crdsand the helm-wrapped templates).deploy/charts/operator/templates/clusterrole/role.yamlis deliberately left as is: the generator would overwrite the fork's release-qualified name from 9b83a5e.Merge and pin ordering (why this is a draft)
toolhive-operator-crds.targetRevisionin platform-apps, until every cluster renders the object groupRef: docplanner-helm#1158 (toolhive-config 0.21.0) must be released and rolled out to both platform-apps wrappers (toolhive-config-stg, then toolhive-config) and synced on all clusters.spec.groupRefis rejected.Testing
go vet ./cmd/thv-operator/...clean.pkg/httpclient(sandbox blocks local port binding) andTestOperatorClusterRoleManifestIncludesPodsDelete(parses the templated role.yaml as plain YAML; fails since 9b83a5e).mcp-grouppasses; the new groupRef schema specs pass.mcp-serverandvirtualmcphave local timeout failures (Elicitation BeforeAll, AuthServerRef, deterministic ConfigMap) that reproduce on unmodified dp-stable (1ebb380) with the same or more failures, so they are pre-existing or flaky locally, not regressions. CI is the reference.🤖 Generated with Claude Code