fixes from review

taskbot · taskbot · commit 1178d7445ef6 · 2026-01-02T12:51:48.000+01:00
diff --git a/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1alpha1/virtualmcpserver_types.go
@@ -388,6 +388,13 @@ type OperationalConfig struct {
 	// FailureHandling configures failure handling behavior
 	// +optional
 	FailureHandling *FailureHandlingConfig `json:"failureHandling,omitempty"`
+
+	// CapabilityCacheTTL is the time-to-live for cached capability entries.
+	// Defaults to 5 minutes if not specified.
+	// Shorter values (e.g., "10s") can be used for testing to see capability updates sooner.
+	// +kubebuilder:default="5m"
+	// +optional
+	CapabilityCacheTTL string `json:"capabilityCacheTTL,omitempty"`
 }
 
 // TimeoutConfig configures timeout settings
diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go
@@ -1128,7 +1128,13 @@ func countBackendHealth(ctx context.Context, backends []mcpv1alpha1.DiscoveredBa
 	return ready, unhealthy
 }
 
-// determineStatusFromBackends evaluates backend health to determine status
+// determineStatusFromBackends evaluates backend health to determine status.
+// Backend health is based on:
+//   - Workload phase (MCPServer/MCPRemoteProxy phase): Always evaluated
+//   - Runtime health checks: Only when health monitoring is configured
+//
+// When health monitoring is disabled, backends with healthy workload phase are marked as Ready.
+// When health monitoring is enabled, backends must pass both workload phase checks and runtime health checks.
 func (*VirtualMCPServerReconciler) determineStatusFromBackends(
 	ctx context.Context,
 	vmcp *mcpv1alpha1.VirtualMCPServer,
@@ -1640,6 +1646,11 @@ func (r *VirtualMCPServerReconciler) discoverBackends(
 	discoveredBackends := make([]mcpv1alpha1.DiscoveredBackend, 0, len(typedWorkloads))
 	now := metav1.Now()
 
+	// Check if health monitoring is enabled for this VirtualMCPServer
+	healthMonitoringEnabled := vmcp.Spec.Operational != nil &&
+		vmcp.Spec.Operational.FailureHandling != nil &&
+		vmcp.Spec.Operational.FailureHandling.HealthCheckInterval != ""
+
 	// Convert vmcp.Backend to DiscoveredBackend for all workloads in the group
 	for _, workloadInfo := range typedWorkloads {
 		backend, found := discoveredBackendMap[workloadInfo.Name]
@@ -1674,6 +1685,13 @@ func (r *VirtualMCPServerReconciler) discoverBackends(
 					ctxLogger.V(1).Info("Backend MCPServer not ready, marking as unavailable",
 						"name", workloadInfo.Name,
 						"phase", mcpServer.Status.Phase)
+				} else if backendStatus == mcpv1alpha1.BackendStatusUnknown && !healthMonitoringEnabled {
+					// When health monitoring is disabled and workload phase is healthy,
+					// treat unknown health status as ready (no runtime health checks configured)
+					backendStatus = mcpv1alpha1.BackendStatusReady
+					ctxLogger.V(1).Info("Health monitoring disabled with healthy phase, marking backend as ready",
+						"name", workloadInfo.Name,
+						"phase", mcpServer.Status.Phase)
 				}
 			}
 		case workloads.WorkloadTypeMCPRemoteProxy:
@@ -1688,6 +1706,13 @@ func (r *VirtualMCPServerReconciler) discoverBackends(
 					ctxLogger.V(1).Info("Backend MCPRemoteProxy not ready, marking as unavailable",
 						"name", workloadInfo.Name,
 						"phase", mcpRemoteProxy.Status.Phase)
+				} else if backendStatus == mcpv1alpha1.BackendStatusUnknown && !healthMonitoringEnabled {
+					// When health monitoring is disabled and workload phase is healthy,
+					// treat unknown health status as ready (no runtime health checks configured)
+					backendStatus = mcpv1alpha1.BackendStatusReady
+					ctxLogger.V(1).Info("Health monitoring disabled with healthy phase, marking backend as ready",
+						"name", workloadInfo.Name,
+						"phase", mcpRemoteProxy.Status.Phase)
 				}
 			}
 		}
@@ -1715,14 +1740,11 @@ func (r *VirtualMCPServerReconciler) discoverBackends(
 	// Performance: Health status responses are cached with healthStatusCacheTTL to reduce HTTP
 	// overhead from frequent reconciliations while maintaining relatively fresh health data.
 	// The vmcp health endpoint itself returns cached results from periodic health checks.
-	if vmcp.Status.URL != "" {
+	if vmcp.Status.URL != "" && healthMonitoringEnabled {
 		// Parse health check interval from spec to derive query timeout
 		var healthCheckInterval time.Duration
-		if vmcp.Spec.Operational != nil && vmcp.Spec.Operational.FailureHandling != nil &&
-			vmcp.Spec.Operational.FailureHandling.HealthCheckInterval != "" {
-			if interval, err := time.ParseDuration(vmcp.Spec.Operational.FailureHandling.HealthCheckInterval); err == nil {
-				healthCheckInterval = interval
-			}
+		if interval, err := time.ParseDuration(vmcp.Spec.Operational.FailureHandling.HealthCheckInterval); err == nil {
+			healthCheckInterval = interval
 		}
 
 		healthStatus := r.queryVMCPHealthStatus(ctx, vmcp.Status.URL, healthCheckInterval)
diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go
@@ -921,5 +921,12 @@ func (*Converter) convertOperational(
 		}
 	}
 
+	// Parse capability cache TTL
+	if vmcp.Spec.Operational.CapabilityCacheTTL != "" {
+		if duration, err := time.ParseDuration(vmcp.Spec.Operational.CapabilityCacheTTL); err == nil {
+			operational.CapabilityCacheTTL = vmcpconfig.Duration(duration)
+		}
+	}
+
 	return operational
 }
diff --git a/cmd/vmcp/app/commands.go b/cmd/vmcp/app/commands.go
@@ -295,7 +295,16 @@ func runServe(cmd *cobra.Command, _ []string) error {
 	agg := aggregator.NewDefaultAggregator(backendClient, conflictResolver, cfg.Aggregation.Tools)
 
 	// Create discovery manager for lazy per-user capability discovery
-	discoveryMgr, err := discovery.NewManager(agg)
+	// Use configured cache TTL if available, otherwise use default
+	var discoveryMgr discovery.Manager
+	if cfg.Operational != nil && cfg.Operational.CapabilityCacheTTL > 0 {
+		cacheTTL := time.Duration(cfg.Operational.CapabilityCacheTTL)
+		logger.Debugf("Creating discovery manager with custom cache TTL: %v", cacheTTL)
+		discoveryMgr, err = discovery.NewManagerWithTTL(agg, cacheTTL)
+	} else {
+		logger.Debugf("Creating discovery manager with default cache TTL: %v", discovery.DefaultCacheTTL)
+		discoveryMgr, err = discovery.NewManager(agg)
+	}
 	if err != nil {
 		return fmt.Errorf("failed to create discovery manager: %w", err)
 	}
diff --git a/deploy/charts/operator-crds/Chart.yaml b/deploy/charts/operator-crds/Chart.yaml
@@ -2,5 +2,5 @@ apiVersion: v2
 name: toolhive-operator-crds
 description: A Helm chart for installing the ToolHive Operator CRDs into Kubernetes.
 type: application
-version: 0.0.88
+version: 0.0.89
 appVersion: "0.0.1"
diff --git a/deploy/charts/operator-crds/README.md b/deploy/charts/operator-crds/README.md
@@ -1,6 +1,6 @@
 # ToolHive Operator CRDs Helm Chart
 
-![Version: 0.0.88](https://img.shields.io/badge/Version-0.0.88-informational?style=flat-square)
+![Version: 0.0.89](https://img.shields.io/badge/Version-0.0.89-informational?style=flat-square)
 ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square)
 
 A Helm chart for installing the ToolHive Operator CRDs into Kubernetes.
diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml
@@ -644,6 +644,13 @@ spec:
                 description: Operational defines operational settings like timeouts
                   and health checks
                 properties:
+                  capabilityCacheTTL:
+                    default: 5m
+                    description: |-
+                      CapabilityCacheTTL is the time-to-live for cached capability entries.
+                      Defaults to 5 minutes if not specified.
+                      Shorter values (e.g., "10s") can be used for testing to see capability updates sooner.
+                    type: string
                   failureHandling:
                     description: FailureHandling configures failure handling behavior
                     properties:
diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml
@@ -647,6 +647,13 @@ spec:
                 description: Operational defines operational settings like timeouts
                   and health checks
                 properties:
+                  capabilityCacheTTL:
+                    default: 5m
+                    description: |-
+                      CapabilityCacheTTL is the time-to-live for cached capability entries.
+                      Defaults to 5 minutes if not specified.
+                      Shorter values (e.g., "10s") can be used for testing to see capability updates sooner.
+                    type: string
                   failureHandling:
                     description: FailureHandling configures failure handling behavior
                     properties:
diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md
diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go
@@ -242,6 +242,11 @@ type OperationalConfig struct {
 
 	// FailureHandling configures failure handling.
 	FailureHandling *FailureHandlingConfig `json:"failure_handling,omitempty" yaml:"failure_handling,omitempty"`
+
+	// CapabilityCacheTTL is the time-to-live for cached capability entries.
+	// Defaults to 5 minutes if not specified or zero.
+	// Shorter values (e.g., 10s) can be used for testing to see capability updates sooner.
+	CapabilityCacheTTL Duration `json:"capability_cache_ttl,omitempty" yaml:"capability_cache_ttl,omitempty"`
 }
 
 // TimeoutConfig configures timeouts.
diff --git a/pkg/vmcp/discovery/health_filter.go b/pkg/vmcp/discovery/health_filter.go
@@ -2,8 +2,6 @@
 package discovery
 
 import (
-	"reflect"
-
 	"github.com/stacklok/toolhive/pkg/logger"
 	"github.com/stacklok/toolhive/pkg/vmcp"
 	"github.com/stacklok/toolhive/pkg/vmcp/health"
@@ -22,7 +20,7 @@ import (
 func FilterHealthyBackends(backends []vmcp.Backend, healthProvider health.StatusProvider) []vmcp.Backend {
 	// If health monitoring is disabled, return all backends
 	// Note: Check both interface nil and typed nil (Go interface semantics)
-	if healthProvider == nil || !isProviderInitialized(healthProvider) {
+	if !health.IsProviderInitialized(healthProvider) {
 		return backends
 	}
 
@@ -59,10 +57,3 @@ func FilterHealthyBackends(backends []vmcp.Backend, healthProvider health.Status
 
 	return filtered
 }
-
-// isProviderInitialized checks if the health status provider is properly initialized.
-// This handles Go's interface nil semantics where an interface can be "not nil"
-// even when its underlying value is nil (e.g., (*health.Monitor)(nil)).
-func isProviderInitialized(provider health.StatusProvider) bool {
-	return provider != nil && !reflect.ValueOf(provider).IsNil()
-}
diff --git a/pkg/vmcp/discovery/health_filter_test.go b/pkg/vmcp/discovery/health_filter_test.go
@@ -341,7 +341,7 @@ func TestIsProviderInitialized(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			result := isProviderInitialized(tt.provider)
+			result := health.IsProviderInitialized(tt.provider)
 			assert.Equal(t, tt.expected, result)
 		})
 	}
diff --git a/pkg/vmcp/discovery/manager.go b/pkg/vmcp/discovery/manager.go
@@ -24,8 +24,8 @@ import (
 //go:generate mockgen -destination=mocks/mock_manager.go -package=mocks -source=manager.go Manager
 
 const (
-	// cacheTTL is the time-to-live for cached capability entries.
-	cacheTTL = 5 * time.Minute
+	// DefaultCacheTTL is the default time-to-live for cached capability entries.
+	DefaultCacheTTL = 5 * time.Minute
 	// maxCacheSize is the maximum number of entries allowed in the cache.
 	maxCacheSize = 1000
 	// cleanupInterval is how often expired cache entries are removed.
@@ -59,21 +59,33 @@ type cacheEntry struct {
 type DefaultManager struct {
 	aggregator aggregator.Aggregator
 	cache      map[string]*cacheEntry
+	cacheTTL   time.Duration
 	cacheMu    sync.RWMutex
 	stopCh     chan struct{}
 	stopOnce   sync.Once
 	wg         sync.WaitGroup
 }
 
-// NewManager creates a new discovery manager with the given aggregator.
+// NewManager creates a new discovery manager with the given aggregator using the default cache TTL.
 func NewManager(agg aggregator.Aggregator) (Manager, error) {
+	return NewManagerWithTTL(agg, DefaultCacheTTL)
+}
+
+// NewManagerWithTTL creates a new discovery manager with the given aggregator and cache TTL.
+// Use this when you need custom cache TTL (e.g., shorter TTL for testing).
+func NewManagerWithTTL(agg aggregator.Aggregator, cacheTTL time.Duration) (Manager, error) {
 	if agg == nil {
 		return nil, ErrAggregatorNil
 	}
 
+	if cacheTTL <= 0 {
+		cacheTTL = DefaultCacheTTL
+	}
+
 	m := &DefaultManager{
 		aggregator: agg,
 		cache:      make(map[string]*cacheEntry),
+		cacheTTL:   cacheTTL,
 		stopCh:     make(chan struct{}),
 	}
 
@@ -184,7 +196,7 @@ func (m *DefaultManager) cacheCapabilities(key string, caps *aggregator.Aggregat
 
 	m.cache[key] = &cacheEntry{
 		capabilities: caps,
-		expiresAt:    time.Now().Add(cacheTTL),
+		expiresAt:    time.Now().Add(m.cacheTTL),
 	}
 }
 
diff --git a/pkg/vmcp/health/monitor.go b/pkg/vmcp/health/monitor.go
@@ -3,6 +3,7 @@ package health
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"sync"
 	"time"
 
@@ -34,6 +35,16 @@ func IsBackendUsable(status vmcp.BackendHealthStatus) bool {
 		status == vmcp.BackendUnknown
 }
 
+// IsProviderInitialized checks if a StatusProvider is properly initialized.
+// This handles Go's interface nil semantics where an interface can be "not nil"
+// even when its underlying value is nil (e.g., (*Monitor)(nil)).
+//
+// Returns true if the provider is initialized and can be safely called.
+// Returns false if the provider is nil or contains a nil pointer.
+func IsProviderInitialized(provider StatusProvider) bool {
+	return provider != nil && !reflect.ValueOf(provider).IsNil()
+}
+
 // healthCheckContextKey is a marker for health check requests.
 type healthCheckContextKey struct{}
 
diff --git a/pkg/vmcp/server/adapter/handler_factory.go b/pkg/vmcp/server/adapter/handler_factory.go
@@ -8,7 +8,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"reflect"
 
 	"github.com/mark3labs/mcp-go/mcp"
 
@@ -86,7 +85,7 @@ func (f *DefaultHandlerFactory) checkBackendHealth(
 	requestName string,
 ) error {
 	// Check both interface nil and typed nil (Go interface semantics)
-	if f.healthProvider == nil || !isHealthProviderInitialized(f.healthProvider) {
+	if !health.IsProviderInitialized(f.healthProvider) {
 		return nil // Health checking disabled
 	}
 
@@ -331,10 +330,3 @@ func (*DefaultHandlerFactory) CreateCompositeToolHandler(
 		return mcp.NewToolResultStructuredOnly(result.Output), nil
 	}
 }
-
-// isHealthProviderInitialized checks if the health status provider is properly initialized.
-// This handles Go's interface nil semantics where an interface can be "not nil"
-// even when its underlying value is nil (e.g., (*health.Monitor)(nil)).
-func isHealthProviderInitialized(provider health.StatusProvider) bool {
-	return provider != nil && !reflect.ValueOf(provider).IsNil()
-}
diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_circuit_breaker_test.go

Original file line number	Diff line number	Diff line change
`@@ -921,5 +921,12 @@ func (*Converter) convertOperational(`
`921`	`921`	`}`
`922`	`922`	`}`
`923`	`923`
	`924`	`+ // Parse capability cache TTL`
	`925`	`+ if vmcp.Spec.Operational.CapabilityCacheTTL != "" {`
	`926`	`+ if duration, err := time.ParseDuration(vmcp.Spec.Operational.CapabilityCacheTTL); err == nil {`
	`927`	`+ operational.CapabilityCacheTTL = vmcpconfig.Duration(duration)`
	`928`	`+ }`
	`929`	`+ }`
	`930`	`+`
`924`	`931`	`return operational`
`925`	`932`	`}`