Fix GetDefaultProvider bypassing registered config factory (#4740)

* Fix GetDefaultProvider bypassing registered config factory

config.NewDefaultProvider() skips the RegisterProviderFactory hook and
always reads the local XDG config file. Switch to config.NewProvider(),
which checks the registered factory first and falls back to the default
only when no factory is registered.

Without this fix, enterprise builds that register a factory to supply an
EnterpriseProvider (e.g. fetching config from a remote config server)
were silently ignored, causing thv registry commands to use the embedded
registry instead of the configured one.

Add unit tests covering the factory-respected and fall-through cases,
caching semantics, and reset behaviour.

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

* Fix NewRegistryRoutes constructors bypassing registered config factory

Both NewRegistryRoutes and NewRegistryRoutesForServe called
config.NewDefaultProvider(), which skips the RegisterProviderFactory
hook and always reads the local XDG config file. Switch to
config.NewProvider() so enterprise builds that register a factory
(e.g. to supply a remote config server) are correctly honoured.

Add unit tests covering the regression case and the no-factory fallback
for both constructors.

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

* Move config.NewProvider() inside sync.Once closure in GetDefaultProvider

Go evaluates function arguments eagerly, so the previous implementation
called config.NewProvider() (and thus any registered ProviderFactory)
on every invocation of GetDefaultProvider, even after sync.Once had
already fired and the resulting provider was discarded.

Move the call inside the Do closure so the factory is invoked at most
once. GetDefaultProvider now owns its sync.Once block directly rather
than delegating to GetDefaultProviderWithConfig.

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

* Fix data race between ResetDefaultProvider and GetDefaultProviderWithConfig

ResetDefaultProvider assigned defaultProviderOnce = sync.Once{} (a
non-atomic struct write) while GetDefaultProviderWithConfig called
defaultProviderOnce.Do(), which does an atomic load of the internal
done field. Mixed-width concurrent access = data race.

Replace the three separate package-level variables (sync.Once,
Provider, error) with a single providerState struct stored behind an
atomic.Pointer. ResetDefaultProvider now atomically swaps in a fresh
struct instead of writing to one that may be in use. Goroutines that
loaded a pointer before a reset keep a stable reference and complete
safely; goroutines that load after the swap initialise against the new
state. The mutex is no longer needed and is removed.

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

* Add nolint:paralleltest directives to factory tests

These tests mutate global singletons (config.registeredFactory and
currentProviderState) so they cannot run in parallel. Suppress the
paralleltest linter warning with an explanatory comment on each test.

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

* Fix gci formatting: move nolint directives after doc comments

gci requires nolint directives to appear immediately before the func
declaration, after the doc comment block.

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

@@ -14,7 +14,7 @@ import (
 
 	"github.com/go-chi/chi/v5"
 
-	"github.com/stacklok/toolhive-core/registry/types"
+	registry "github.com/stacklok/toolhive-core/registry/types"
 	"github.com/stacklok/toolhive/pkg/config"
 	regpkg "github.com/stacklok/toolhive/pkg/registry"
 	"github.com/stacklok/toolhive/pkg/registry/auth"
@@ -286,7 +286,7 @@ type RegistryRoutes struct {
 // NewRegistryRoutes creates a new RegistryRoutes with the default config provider
 func NewRegistryRoutes() *RegistryRoutes {
 	return &RegistryRoutes{
-		configProvider: config.NewDefaultProvider(),
+		configProvider: config.NewProvider(),
 		configService:  regpkg.NewConfigurator(),
 	}
 }
@@ -304,7 +304,7 @@ func NewRegistryRoutesWithProvider(provider config.Provider) *RegistryRoutes {
 // In serve mode, the registry provider uses non-interactive auth (no browser OAuth).
 func NewRegistryRoutesForServe() *RegistryRoutes {
 	return &RegistryRoutes{
-		configProvider: config.NewDefaultProvider(),
+		configProvider: config.NewProvider(),
 		configService:  regpkg.NewConfigurator(),
 		serveMode:      true,
 	}

pkg/api/v1/registry_factory_test.go

@@ -0,0 +1,201 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package v1
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/go-chi/chi/v5"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v3"
+
+	"github.com/stacklok/toolhive/pkg/config"
+	"github.com/stacklok/toolhive/pkg/registry"
+)
+
+// writeFactorySentinelRegistry creates a legacy-format registry JSON file with a
+// single server named sentinelName and a YAML config pointing to it.
+// Returns the config file path.
+func writeFactorySentinelRegistry(t *testing.T, sentinelName string) string {
+	t.Helper()
+
+	dir := t.TempDir()
+
+	// Write legacy registry JSON with the sentinel server.
+	type serverEntry struct {
+		Image       string `json:"image"`
+		Description string `json:"description"`
+	}
+	type registryFile struct {
+		Version     string                 `json:"version"`
+		LastUpdated string                 `json:"last_updated"`
+		Servers     map[string]serverEntry `json:"servers"`
+	}
+
+	regData, err := json.Marshal(registryFile{
+		Version:     "1.0.0",
+		LastUpdated: "2025-01-01T00:00:00Z",
+		Servers: map[string]serverEntry{
+			sentinelName: {
+				Image:       "factory/server:latest",
+				Description: "Factory sentinel server",
+			},
+		},
+	})
+	require.NoError(t, err)
+
+	registryPath := filepath.Join(dir, "registry.json")
+	require.NoError(t, os.WriteFile(registryPath, regData, 0600))
+
+	// Write YAML config pointing to the registry JSON.
+	type configFile struct {
+		LocalRegistryPath string `yaml:"local_registry_path"`
+	}
+
+	cfgData, err := yaml.Marshal(configFile{LocalRegistryPath: registryPath})
+	require.NoError(t, err)
+
+	configPath := filepath.Join(dir, "config.yaml")
+	require.NoError(t, os.WriteFile(configPath, cfgData, 0600))
+
+	return configPath
+}
+
+// makeListServersRequest builds an httptest request for GET /{name}/servers
+// with the chi URL param "name" set to registryName.
+func makeListServersRequest(registryName string) *http.Request {
+	req := httptest.NewRequest(http.MethodGet, "/"+registryName+"/servers", nil)
+	rctx := chi.NewRouteContext()
+	rctx.URLParams.Add("name", registryName)
+	return req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
+}
+
+// TestNewRegistryRoutes_RespectsRegisteredFactory is the critical regression test
+// for the bug fix. Before the fix, NewRegistryRoutes called config.NewDefaultProvider(),
+// which bypassed any registered ProviderFactory. The fix changed it to call
+// config.NewProvider(), which checks the factory first.
+//
+// The test registers a factory that returns a PathProvider pointing at a local
+// registry JSON containing a sentinel server name. If NewRegistryRoutes correctly
+// forwards the factory-backed provider to getCurrentProvider, the listServers
+// handler will return that sentinel server in its response.
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory and regpkg.defaultProviderOnce
+func TestNewRegistryRoutes_RespectsRegisteredFactory(t *testing.T) {
+	const sentinelName = "factory-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()
+
+	// Clear provider cache so getCurrentProvider re-initialises using our factory.
+	registry.ResetDefaultProvider()
+
+	w := httptest.NewRecorder()
+	routes.listServers(w, makeListServersRequest("default"))
+
+	assert.Equal(t, http.StatusOK, w.Code,
+		"listServers should return 200 when factory-backed provider is used")
+
+	var body listServersResponse
+	require.NoError(t, json.NewDecoder(w.Body).Decode(&body),
+		"response body should be valid JSON")
+
+	names := make([]string, 0, len(body.Servers))
+	for _, s := range body.Servers {
+		names = append(names, s.GetName())
+	}
+	assert.Contains(t, names, sentinelName,
+		"sentinel server must be present; this would fail on the old code that called config.NewDefaultProvider()")
+}
+
+// TestNewRegistryRoutesForServe_RespectsRegisteredFactory verifies that the
+// serve-mode constructor also honours the registered ProviderFactory. This
+// mirrors TestNewRegistryRoutes_RespectsRegisteredFactory but exercises
+// NewRegistryRoutesForServe and the serveMode code path.
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory and regpkg.defaultProviderOnce
+func TestNewRegistryRoutesForServe_RespectsRegisteredFactory(t *testing.T) {
+	const sentinelName = "factory-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()
+
+	// Clear provider cache so getCurrentProvider re-initialises using our factory.
+	registry.ResetDefaultProvider()
+
+	w := httptest.NewRecorder()
+	routes.listServers(w, makeListServersRequest("default"))
+
+	assert.Equal(t, http.StatusOK, w.Code,
+		"listServers should return 200 when factory-backed provider is used in serve mode")
+
+	var body listServersResponse
+	require.NoError(t, json.NewDecoder(w.Body).Decode(&body),
+		"response body should be valid JSON")
+
+	names := make([]string, 0, len(body.Servers))
+	for _, s := range body.Servers {
+		names = append(names, s.GetName())
+	}
+	assert.Contains(t, names, sentinelName,
+		"sentinel server must be present; this would fail on the old code that called config.NewDefaultProvider()")
+}
+
+// TestNewRegistryRoutes_NoFactory_ReturnsValidRoutes verifies that NewRegistryRoutes
+// returns a fully-initialised struct when no ProviderFactory is registered.
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory
+func TestNewRegistryRoutes_NoFactory_ReturnsValidRoutes(t *testing.T) {
+	config.RegisterProviderFactory(nil)
+	t.Cleanup(func() { config.RegisterProviderFactory(nil) })
+
+	routes := NewRegistryRoutes()
+
+	require.NotNil(t, routes, "NewRegistryRoutes must return a non-nil value")
+	assert.NotNil(t, routes.configProvider, "configProvider must be initialised")
+	assert.NotNil(t, routes.configService, "configService must be initialised")
+	assert.False(t, routes.serveMode, "serveMode must be false for NewRegistryRoutes")
+}
+
+// TestNewRegistryRoutesForServe_NoFactory_ReturnsValidRoutes verifies that
+// NewRegistryRoutesForServe returns a fully-initialised struct with serveMode
+// set to true when no ProviderFactory is registered.
+//
+//nolint:paralleltest // Mutates global state: config.registeredFactory
+func TestNewRegistryRoutesForServe_NoFactory_ReturnsValidRoutes(t *testing.T) {
+	config.RegisterProviderFactory(nil)
+	t.Cleanup(func() { config.RegisterProviderFactory(nil) })
+
+	routes := NewRegistryRoutesForServe()
+
+	require.NotNil(t, routes, "NewRegistryRoutesForServe must return a non-nil value")
+	assert.NotNil(t, routes.configProvider, "configProvider must be initialised")
+	assert.NotNil(t, routes.configService, "configService must be initialised")
+	assert.True(t, routes.serveMode, "serveMode must be true for NewRegistryRoutesForServe")
+}

pkg/registry/factory.go

@@ -10,22 +10,30 @@ import (
 	"fmt"
 	"log/slog"
 	"sync"
+	"sync/atomic"
 
 	"github.com/stacklok/toolhive/pkg/config"
 	"github.com/stacklok/toolhive/pkg/registry/auth"
 	"github.com/stacklok/toolhive/pkg/secrets"
 )
 
-var (
-	defaultProvider     Provider
-	defaultProviderOnce sync.Once
-	defaultProviderErr  error
-	// defaultProviderMu protects the ResetDefaultProvider operation
-	// to prevent race conditions when resetting the sync.Once.
-	// The mutex is NOT needed for GetDefaultProviderWithConfig since
-	// sync.Once already provides thread-safety for initialization.
-	defaultProviderMu sync.Mutex
-)
+// providerState groups the sync.Once with the values it initialises.
+// Storing all three together behind an atomic pointer means ResetDefaultProvider
+// can swap in a fresh struct without ever writing to a struct that another
+// goroutine may be reading — eliminating the data race between Reset and Do.
+type providerState struct {
+	once     sync.Once
+	provider Provider
+	err      error
+}
+
+// currentProviderState is the live singleton state. Replaced atomically by
+// ResetDefaultProvider; never mutated after creation except inside once.Do.
+var currentProviderState atomic.Pointer[providerState]
+
+func init() {
+	currentProviderState.Store(&providerState{})
+}
 
 // ProviderOption configures optional behavior for NewRegistryProvider.
 type ProviderOption func(*providerOptions)
@@ -79,42 +87,46 @@ func NewRegistryProvider(cfg *config.Config, opts ...ProviderOption) (Provider,
 	return NewLocalRegistryProvider(), nil
 }
 
-// GetDefaultProvider returns the default registry provider instance
-// This maintains backward compatibility with the existing singleton pattern
+// GetDefaultProvider returns the default registry provider instance.
+// config.NewProvider() is called inside the sync.Once closure so that any
+// registered ProviderFactory is invoked at most once, not on every call.
 func GetDefaultProvider() (Provider, error) {
-	return GetDefaultProviderWithConfig(config.NewDefaultProvider())
+	s := currentProviderState.Load()
+	s.once.Do(func() {
+		cfg, err := config.NewProvider().LoadOrCreateConfig()
+		if err != nil {
+			s.err = err
+			return
+		}
+		s.provider, s.err = NewRegistryProvider(cfg)
+	})
+	return s.provider, s.err
 }
 
 // GetDefaultProviderWithConfig returns a registry provider using the given config provider.
 // This allows tests to inject their own config provider.
 // The interactive flag controls whether browser-based OAuth flows are allowed.
 // Pass true for CLI contexts, false for headless/serve mode.
 func GetDefaultProviderWithConfig(configProvider config.Provider, opts ...ProviderOption) (Provider, error) {
-	defaultProviderOnce.Do(func() {
+	s := currentProviderState.Load()
+	s.once.Do(func() {
 		cfg, err := configProvider.LoadOrCreateConfig()
 		if err != nil {
-			defaultProviderErr = err
+			s.err = err
 			return
 		}
-		defaultProvider, defaultProviderErr = NewRegistryProvider(cfg, opts...)
+		s.provider, s.err = NewRegistryProvider(cfg, opts...)
 	})
-
-	return defaultProvider, defaultProviderErr
+	return s.provider, s.err
 }
 
-// ResetDefaultProvider clears the cached default provider instance
-// This allows the provider to be recreated with updated configuration.
-// This function is thread-safe and can be called concurrently.
-// The mutex is required here because we're modifying the sync.Once itself,
-// which is not a thread-safe operation.
+// ResetDefaultProvider clears the cached default provider instance so the
+// next call to GetDefaultProvider or GetDefaultProviderWithConfig creates a
+// fresh one. The atomic swap is safe to call concurrently: goroutines that
+// already hold a reference to the old state finish against that state cleanly,
+// while goroutines that load after the swap initialise against the new state.
 func ResetDefaultProvider() {
-	defaultProviderMu.Lock()
-	defer defaultProviderMu.Unlock()
-
-	// Reset the sync.Once to allow re-initialization
-	defaultProviderOnce = sync.Once{}
-	defaultProvider = nil
-	defaultProviderErr = nil
+	currentProviderState.Store(&providerState{})
 }
 
 // resolveTokenSource creates a TokenSource from the config if registry auth is configured.