Fix stale config in registry routes and WorkloadService (#4751)

* Fix stale config in registry routes and WorkloadService

Two independent config snapshots caused inconsistency after enterprise
registry changes:

1. NewRegistryRoutes/NewRegistryRoutesForServe created separate
   config.Provider instances for configProvider and configService.
   This meant getRegistry responses mixed live metadata (from configService)
   with stale server lists (from the old independent provider). Fix: create
   a single provider p and pass it to both.

2. WorkloadService.appConfig was a one-time snapshot taken at construction.
   After an enterprise registry change, new workloads got stale registry
   URLs written into their persisted RunConfig. Fix: store config.Provider
   and call GetConfig() at the point of use in BuildFullRunConfig.

Add tests verifying:
- configService and getCurrentProvider share the same provider instance
  (listRegistries returns consistent type/source and server count)
- WorkloadService.configProvider is initialized at construction

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Address bot review: fix factory bypass and TOCTOU in WorkloadService

- Replace config.NewDefaultProvider() with config.NewProvider() in
  NewWorkloadService so a registered ProviderFactory is respected,
  consistent with the analogous fix in NewRegistryRoutes
- Hoist cfg := s.configProvider.GetConfig() once at the top of the
  relevant block in BuildFullRunConfig so both uses (ResolveRegistrySourceURLs
  and DisableUsageMetrics) see the same config snapshot within a single call
- Add TestNewWorkloadService_RespectsRegisteredFactory to verify the
  factory-backed provider is actually used

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: reyortiz3

pkg/api/v1/registry.go

@@ -285,9 +285,10 @@ type RegistryRoutes struct {
 
 // NewRegistryRoutes creates a new RegistryRoutes with the default config provider
 func NewRegistryRoutes() *RegistryRoutes {
+	p := config.NewProvider()
 	return &RegistryRoutes{
-		configProvider: config.NewProvider(),
-		configService:  regpkg.NewConfigurator(),
+		configProvider: p,
+		configService:  regpkg.NewConfiguratorWithProvider(p),
 	}
 }
 
@@ -303,9 +304,10 @@ func NewRegistryRoutesWithProvider(provider config.Provider) *RegistryRoutes {
 // NewRegistryRoutesForServe creates RegistryRoutes configured for serve mode.
 // In serve mode, the registry provider uses non-interactive auth (no browser OAuth).
 func NewRegistryRoutesForServe() *RegistryRoutes {
+	p := config.NewProvider()
 	return &RegistryRoutes{
-		configProvider: config.NewProvider(),
-		configService:  regpkg.NewConfigurator(),
+		configProvider: p,
+		configService:  regpkg.NewConfiguratorWithProvider(p),
 		serveMode:      true,
 	}
 }

pkg/api/v1/registry_factory_test.go

@@ -199,3 +199,91 @@ func TestNewRegistryRoutesForServe_NoFactory_ReturnsValidRoutes(t *testing.T) {
 	assert.NotNil(t, routes.configService, "configService must be initialised")
 	assert.True(t, routes.serveMode, "serveMode must be true for NewRegistryRoutesForServe")
 }
+
+// TestNewRegistryRoutes_ConfigServiceAndProviderAreConsistent verifies that
+// configService (which drives the type/source fields) and getCurrentProvider
+// (which drives the server list) both draw from the same config provider instance.
+// Before the fix, configService used config.NewDefaultProvider() independently,
+// causing type/source to reflect local config while the server list could reflect
+// a factory-backed config (or vice versa) — inconsistency within a single response.
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory and regpkg.defaultProviderOnce
+func TestNewRegistryRoutes_ConfigServiceAndProviderAreConsistent(t *testing.T) {
+	const sentinelName = "consistency-sentinel-server"
+
+	configPath := writeFactorySentinelRegistry(t, sentinelName)
+
+	config.RegisterProviderFactory(func() config.Provider {
+		return config.NewPathProvider(configPath)
+	})
+	t.Cleanup(func() {
+		config.RegisterProviderFactory(nil)
+		registry.ResetDefaultProvider()
+	})
+
+	routes := NewRegistryRoutes()
+	registry.ResetDefaultProvider()
+
+	w := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/registry", nil)
+	routes.listRegistries(w, req)
+
+	assert.Equal(t, http.StatusOK, w.Code, "listRegistries should return 200")
+
+	var body registryListResponse
+	require.NoError(t, json.NewDecoder(w.Body).Decode(&body), "response body should be valid JSON")
+	require.Len(t, body.Registries, 1, "should return exactly one registry")
+
+	reg := body.Registries[0]
+	// configService reads Type/Source from the shared provider. On the old code,
+	// configService used config.NewDefaultProvider() which bypassed the factory,
+	// so Type would be "default" and Source would be "" even when a factory was set.
+	assert.Equal(t, RegistryTypeFile, reg.Type,
+		"Type must be 'file' — proves configService uses the factory-backed provider, not an independent one")
+	assert.NotEmpty(t, reg.Source,
+		"Source must be non-empty for a file registry — proves configService reads from the shared provider")
+
+	// getCurrentProvider also uses the shared provider, so it loads servers from the same registry.
+	// ServerCount > 0 proves both data sources are in sync.
+	assert.Greater(t, reg.ServerCount, 0,
+		"ServerCount must be > 0 — proves getCurrentProvider uses the same factory-backed provider as configService")
+}
+
+// TestNewRegistryRoutesForServe_ConfigServiceAndProviderAreConsistent is the
+// serve-mode equivalent of TestNewRegistryRoutes_ConfigServiceAndProviderAreConsistent.
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory and regpkg.defaultProviderOnce
+func TestNewRegistryRoutesForServe_ConfigServiceAndProviderAreConsistent(t *testing.T) {
+	const sentinelName = "consistency-sentinel-server"
+
+	configPath := writeFactorySentinelRegistry(t, sentinelName)
+
+	config.RegisterProviderFactory(func() config.Provider {
+		return config.NewPathProvider(configPath)
+	})
+	t.Cleanup(func() {
+		config.RegisterProviderFactory(nil)
+		registry.ResetDefaultProvider()
+	})
+
+	routes := NewRegistryRoutesForServe()
+	registry.ResetDefaultProvider()
+
+	w := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/registry", nil)
+	routes.listRegistries(w, req)
+
+	assert.Equal(t, http.StatusOK, w.Code, "listRegistries should return 200 in serve mode")
+
+	var body registryListResponse
+	require.NoError(t, json.NewDecoder(w.Body).Decode(&body), "response body should be valid JSON")
+	require.Len(t, body.Registries, 1, "should return exactly one registry")
+
+	reg := body.Registries[0]
+	assert.Equal(t, RegistryTypeFile, reg.Type,
+		"Type must be 'file' in serve mode — proves configService uses the factory-backed provider")
+	assert.NotEmpty(t, reg.Source,
+		"Source must be non-empty for a file registry in serve mode")
+	assert.Greater(t, reg.ServerCount, 0,
+		"ServerCount must be > 0 in serve mode — proves getCurrentProvider uses the same factory-backed provider as configService")
+}

pkg/api/v1/workload_service.go

@@ -62,7 +62,7 @@ type WorkloadService struct {
 	debugMode        bool
 	imageRetriever   retriever.Retriever
 	imagePuller      retriever.ImagePuller
-	appConfig        *config.Config
+	configProvider   config.Provider
 }
 
 // NewWorkloadService creates a new WorkloadService instance
@@ -72,18 +72,14 @@ func NewWorkloadService(
 	containerRuntime runtime.Runtime,
 	debugMode bool,
 ) *WorkloadService {
-	// Load application config for global settings
-	configProvider := config.NewDefaultProvider()
-	appConfig := configProvider.GetConfig()
-
 	return &WorkloadService{
 		workloadManager:  workloadManager,
 		groupManager:     groupManager,
 		containerRuntime: containerRuntime,
 		debugMode:        debugMode,
 		imageRetriever:   retriever.ResolveMCPServer,
 		imagePuller:      retriever.PullMCPServerImage,
-		appConfig:        appConfig,
+		configProvider:   config.NewProvider(),
 	}
 }
 
@@ -282,8 +278,12 @@ func (s *WorkloadService) BuildFullRunConfig(
 		}
 	}
 
+	// Snapshot config once for this request so all fields within a single BuildFullRunConfig
+	// call are consistent with each other, even if a concurrent registry update fires mid-call.
+	cfg := s.configProvider.GetConfig()
+
 	// Resolve registry source URLs and server name when the server was discovered via registry lookup.
-	regAPIURL, regURL := runner.ResolveRegistrySourceURLs(serverMetadata, s.appConfig)
+	regAPIURL, regURL := runner.ResolveRegistrySourceURLs(serverMetadata, cfg)
 	regServerName := runner.ResolveRegistryServerName(serverMetadata)
 
 	options := []runner.RunConfigBuilderOption{
@@ -365,7 +365,7 @@ func (s *WorkloadService) BuildFullRunConfig(
 			"",
 			req.Name,
 			transportType,
-			s.appConfig.DisableUsageMetrics,
+			cfg.DisableUsageMetrics,
 		),
 	)
 

pkg/api/v1/workload_service_test.go

@@ -6,6 +6,7 @@ package v1
 import (
 	"context"
 	"errors"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -24,7 +25,7 @@ func TestWorkloadService_GetWorkloadNamesFromRequest(t *testing.T) {
 	t.Run("with names", func(t *testing.T) {
 		t.Parallel()
 
-		service := &WorkloadService{appConfig: &config.Config{}}
+		service := &WorkloadService{configProvider: config.NewDefaultProvider()}
 
 		req := bulkOperationRequest{
 			Names: []string{"workload1", "workload2"},
@@ -55,7 +56,7 @@ func TestWorkloadService_GetWorkloadNamesFromRequest(t *testing.T) {
 		service := &WorkloadService{
 			groupManager:    mockGroupManager,
 			workloadManager: mockWorkloadManager,
-			appConfig:       &config.Config{},
+			configProvider:  config.NewDefaultProvider(),
 		}
 
 		req := bulkOperationRequest{
@@ -71,7 +72,7 @@ func TestWorkloadService_GetWorkloadNamesFromRequest(t *testing.T) {
 	t.Run("invalid group name", func(t *testing.T) {
 		t.Parallel()
 
-		service := &WorkloadService{appConfig: &config.Config{}}
+		service := &WorkloadService{configProvider: config.NewDefaultProvider()}
 
 		req := bulkOperationRequest{
 			Group: "invalid-group-name-with-special-chars!@#",
@@ -96,8 +97,8 @@ func TestWorkloadService_GetWorkloadNamesFromRequest(t *testing.T) {
 			Return(false, nil)
 
 		service := &WorkloadService{
-			groupManager: mockGroupManager,
-			appConfig:    &config.Config{},
+			groupManager:   mockGroupManager,
+			configProvider: config.NewDefaultProvider(),
 		}
 
 		req := bulkOperationRequest{
@@ -130,7 +131,7 @@ func TestWorkloadService_GetWorkloadNamesFromRequest(t *testing.T) {
 		service := &WorkloadService{
 			groupManager:    mockGroupManager,
 			workloadManager: mockWorkloadManager,
-			appConfig:       &config.Config{},
+			configProvider:  config.NewDefaultProvider(),
 		}
 
 		req := bulkOperationRequest{
@@ -150,6 +151,38 @@ func TestNewWorkloadService(t *testing.T) {
 
 	service := NewWorkloadService(nil, nil, nil, false)
 	require.NotNil(t, service)
+	assert.NotNil(t, service.configProvider,
+		"configProvider must be initialized so config is read fresh on each call, not snapshotted at construction")
+}
+
+// writeFactorySentinelConfig writes a YAML config file with DisableUsageMetrics: true
+// as a sentinel value and returns its path.
+func writeFactorySentinelConfig(t *testing.T, dir string) string {
+	t.Helper()
+	configPath := dir + "/config.yaml"
+	require.NoError(t, os.WriteFile(configPath, []byte("disable_usage_metrics: true\n"), 0600))
+	return configPath
+}
+
+// TestNewWorkloadService_RespectsRegisteredFactory verifies that NewWorkloadService
+// uses config.NewProvider() (which checks the registered ProviderFactory) rather than
+// config.NewDefaultProvider() (which always uses the default XDG path and bypasses factories).
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory
+func TestNewWorkloadService_RespectsRegisteredFactory(t *testing.T) {
+	configPath := writeFactorySentinelConfig(t, t.TempDir())
+
+	config.RegisterProviderFactory(func() config.Provider {
+		return config.NewPathProvider(configPath)
+	})
+	t.Cleanup(func() { config.RegisterProviderFactory(nil) })
+
+	service := NewWorkloadService(nil, nil, nil, false)
+	require.NotNil(t, service)
+
+	cfg := service.configProvider.GetConfig()
+	assert.True(t, cfg.DisableUsageMetrics,
+		"configProvider must use the factory-backed provider — DisableUsageMetrics is the sentinel set by the factory config")
 }
 
 func TestRuntimeConfigFromRequest(t *testing.T) {

pkg/api/v1/workloads_test.go

@@ -283,7 +283,7 @@ func TestCreateWorkload(t *testing.T) {
 					workloadManager: mockWorkloadManager,
 					imageRetriever:  mockRetriever,
 					imagePuller:     func(_ context.Context, _ string) error { return nil },
-					appConfig:       &config.Config{},
+					configProvider:  config.NewDefaultProvider(),
 				},
 			}
 
@@ -505,7 +505,7 @@ func TestUpdateWorkload(t *testing.T) {
 					workloadManager: mockWorkloadManager,
 					imageRetriever:  mockRetriever,
 					imagePuller:     func(_ context.Context, _ string) error { return nil },
-					appConfig:       &config.Config{},
+					configProvider:  config.NewDefaultProvider(),
 				},
 			}
 
@@ -651,7 +651,7 @@ func TestUpdateWorkload_PortReuse(t *testing.T) {
 					containerRuntime: mockRuntime,
 					imageRetriever:   mockRetriever,
 					imagePuller:      func(_ context.Context, _ string) error { return nil },
-					appConfig:        &config.Config{},
+					configProvider:   config.NewDefaultProvider(),
 				},
 			}