fixes from review

Author: taskbot

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

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
 }

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)
 	}

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:

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:

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.

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()
-}

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)
 		})
 	}

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),
 	}
 }
 

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{}