fix(074): honor per-server discovery cadence + hot-reload global intervals

Addresses the two Codex review findings (MCP-1186) on PR #609.

Finding 1 — per-server tool_discovery_interval was inert at runtime. The
periodic background sweep listed every connected server every global cycle
regardless of per-server overrides. The sweep now resolves each server's
tool_discovery_interval and skips servers that are disabled (<=0) or not yet
due (Manager.DiscoverToolsDue + per-server lastSweptAt tracking), and the
indexing loop ticks at the smallest enabled per-server cadence so a short
override is re-listed on time (Manager.ResolveToolDiscoverySweepTick).
Event-driven callers keep using the full DiscoverTools sweep.

Finding 2 — a global health_check_interval edit did not reach running clients.
ApplyConfig now propagates the new global config to the upstream manager and
every managed client (Manager/Client.SetGlobalConfig), whose health loop
re-resolves the interval each cycle; health_check_interval and
tool_discovery_interval are added to hot-reload change detection. globalConfig
on the manager and managed client is now an atomic pointer (mirrors the cfg
pattern) so the swap is lock-free and race-free.

The sweep tick is computed from the manager's thread-safe per-client config
snapshots, never by iterating the shared (copy-on-write) config snapshot's
Servers slice, which raced in-place config mutation in the background loop.

Tests: per-server cadence gate, tick resolution, swept-state tracking, manager
+ client SetGlobalConfig propagation, health-interval hot-reload, and the two
new change-detection fields. Full internal suite green under -race (0 races);
both editions build.

Related #608

Author: Dumbris

internal/runtime/config_hotreload.go

@@ -99,6 +99,19 @@ func DetectConfigChanges(oldCfg, newCfg *config.Config) *ConfigApplyResult {
 		result.ChangedFields = append(result.ChangedFields, "call_tool_timeout")
 	}
 
+	// Discovery & health-check cadence (spec 074 — hot-reloadable). The health
+	// loop (managed client) and indexing loop (runtime) re-resolve their interval
+	// each cycle, and ApplyConfig propagates the new global config to the upstream
+	// manager + managed clients, so a global edit takes effect without a restart
+	// (FR-012/SC-002). Tracking these keeps a lone interval edit from being
+	// reported as "no changes detected".
+	if !reflect.DeepEqual(oldCfg.HealthCheckInterval, newCfg.HealthCheckInterval) {
+		result.ChangedFields = append(result.ChangedFields, "health_check_interval")
+	}
+	if !reflect.DeepEqual(oldCfg.ToolDiscoveryInterval, newCfg.ToolDiscoveryInterval) {
+		result.ChangedFields = append(result.ChangedFields, "tool_discovery_interval")
+	}
+
 	// Logging configuration (can be hot-reloaded)
 	if !reflect.DeepEqual(oldCfg.Logging, newCfg.Logging) {
 		result.ChangedFields = append(result.ChangedFields, "logging")

internal/runtime/config_hotreload_test.go

@@ -35,6 +35,54 @@ func TestDetectConfigChanges_Observability(t *testing.T) {
 	assert.False(t, result.RequiresRestart, "cadence change is hot-reloadable")
 }
 
+// TestDetectConfigChanges_DiscoveryHealthIntervals (MCP-1189 / Codex finding #2):
+// a global health_check_interval / tool_discovery_interval edit must be detected
+// as a hot-reloadable change so ApplyConfig propagates the new cadence to the
+// running upstream manager + managed clients. Without this, a lone interval edit
+// would be reported as "no changes detected" (FR-012/SC-002).
+func TestDetectConfigChanges_DiscoveryHealthIntervals(t *testing.T) {
+	mk := func(health, discovery *config.Duration) *config.Config {
+		return &config.Config{
+			Listen: "127.0.0.1:8080", DataDir: "/d", TLS: &config.TLSConfig{},
+			HealthCheckInterval:   health,
+			ToolDiscoveryInterval: discovery,
+		}
+	}
+
+	t.Run("health_check_interval change detected", func(t *testing.T) {
+		old45 := config.Duration(45 * time.Second)
+		new10 := config.Duration(10 * time.Second)
+		result := DetectConfigChanges(mk(&old45, nil), mk(&new10, nil))
+		require.True(t, result.Success)
+		assert.Contains(t, result.ChangedFields, "health_check_interval")
+		assert.False(t, result.RequiresRestart, "interval change is hot-reloadable")
+	})
+
+	t.Run("tool_discovery_interval change detected", func(t *testing.T) {
+		old5m := config.Duration(5 * time.Minute)
+		new1m := config.Duration(1 * time.Minute)
+		result := DetectConfigChanges(mk(nil, &old5m), mk(nil, &new1m))
+		require.True(t, result.Success)
+		assert.Contains(t, result.ChangedFields, "tool_discovery_interval")
+		assert.False(t, result.RequiresRestart)
+	})
+
+	t.Run("setting from unset (nil -> value) detected", func(t *testing.T) {
+		val := config.Duration(0) // disabling the loop
+		result := DetectConfigChanges(mk(nil, nil), mk(&val, nil))
+		require.True(t, result.Success)
+		assert.Contains(t, result.ChangedFields, "health_check_interval")
+	})
+
+	t.Run("unchanged intervals not reported", func(t *testing.T) {
+		same := config.Duration(45 * time.Second)
+		other := config.Duration(45 * time.Second)
+		result := DetectConfigChanges(mk(&same, nil), mk(&other, nil))
+		require.True(t, result.Success)
+		assert.NotContains(t, result.ChangedFields, "health_check_interval")
+	})
+}
+
 func TestDetectConfigChanges(t *testing.T) {
 	baseConfig := &config.Config{
 		Listen:            "127.0.0.1:8080",

internal/runtime/lifecycle.go

@@ -229,14 +229,16 @@ func (r *Runtime) connectAllWithRetry(ctx context.Context) {
 const toolDiscoveryDisabledRecheckInterval = 5 * time.Minute
 
 // planToolDiscoveryCycle decides one iteration of the indexing loop: whether to
-// run a periodic sweep and how long to wait first. A positive interval sweeps
-// on that cadence; a non-positive interval disables the sweep and waits the
-// re-check window.
-func planToolDiscoveryCycle(interval, disabledRecheck time.Duration) (sweep bool, wait time.Duration) {
-	if interval <= 0 {
+// run a periodic sweep and how long to wait first. When at least one server (or
+// the global default) has a positive resolved interval the loop ticks at the
+// smallest such cadence (tick) and sweeps; when every interval is disabled
+// (anyEnabled=false) the loop waits the re-check window without sweeping so a
+// later hot-reload can re-enable it.
+func planToolDiscoveryCycle(tick time.Duration, anyEnabled bool, disabledRecheck time.Duration) (sweep bool, wait time.Duration) {
+	if !anyEnabled || tick <= 0 {
 		return false, disabledRecheck
 	}
-	return true, interval
+	return true, tick
 }
 
 func (r *Runtime) backgroundToolIndexing(ctx context.Context) {
@@ -250,17 +252,22 @@ func (r *Runtime) backgroundToolIndexing(ctx context.Context) {
 		return
 	}
 
-	// Re-resolve the global tool-discovery interval every cycle so a config
-	// hot-reload changes the sweep cadence (or disables it) without a restart
-	// (spec 074, FR-012). A resolved value <= 0 disables the periodic sweep.
+	// Re-resolve the tool-discovery cadence every cycle so a config hot-reload
+	// changes the sweep interval (or disables it) without a restart (spec 074,
+	// FR-012). The loop ticks at the smallest per-server cadence and the sweep
+	// itself (DiscoverToolsDue) only re-lists servers whose own interval has
+	// elapsed, so per-server overrides take effect (US3/SC-006/FR-005). The tick
+	// is resolved from the upstream manager's thread-safe per-client config
+	// snapshots — iterating r.Config().Servers here would race in-place config
+	// mutation (the shared snapshot is copy-on-write; see runtime.Config()).
 	for {
-		interval := r.Config().ResolveToolDiscoveryInterval(nil)
-		sweep, wait := planToolDiscoveryCycle(interval, toolDiscoveryDisabledRecheckInterval)
+		tick, anyEnabled := r.upstreamManager.ResolveToolDiscoverySweepTick(r.Config())
+		sweep, wait := planToolDiscoveryCycle(tick, anyEnabled, toolDiscoveryDisabledRecheckInterval)
 		timer := time.NewTimer(wait)
 		select {
 		case <-timer.C:
 			if sweep {
-				_ = r.DiscoverAndIndexTools(ctx)
+				_ = r.discoverAndIndexTools(ctx, true)
 			}
 		case <-ctx.Done():
 			timer.Stop()
@@ -302,15 +309,31 @@ func (r *Runtime) backgroundSessionCleanup(ctx context.Context) {
 	}
 }
 
-// DiscoverAndIndexTools discovers tools from upstream servers and indexes them.
+// DiscoverAndIndexTools discovers tools from ALL connected upstream servers and
+// indexes them. Used by event-driven callers (boot, server reload, manual
+// refresh) that want a full sweep regardless of per-server cadence.
 func (r *Runtime) DiscoverAndIndexTools(ctx context.Context) error {
+	return r.discoverAndIndexTools(ctx, false)
+}
+
+// discoverAndIndexTools discovers tools and updates the index. When dueOnly is
+// true (the periodic spec-074 sweep) only servers whose per-server
+// tool_discovery_interval has elapsed are re-listed; servers omitted from the
+// sweep keep their last-good index snapshot, so the index does not shrink.
+func (r *Runtime) discoverAndIndexTools(ctx context.Context, dueOnly bool) error {
 	if r.upstreamManager == nil || r.indexManager == nil {
 		return fmt.Errorf("runtime managers not initialized")
 	}
 
-	r.logger.Info("Discovering and indexing tools...")
+	r.logger.Info("Discovering and indexing tools...", zap.Bool("due_only", dueOnly))
 
-	tools, err := r.upstreamManager.DiscoverTools(ctx)
+	var tools []*config.ToolMetadata
+	var err error
+	if dueOnly {
+		tools, err = r.upstreamManager.DiscoverToolsDue(ctx)
+	} else {
+		tools, err = r.upstreamManager.DiscoverTools(ctx)
+	}
 	if err != nil {
 		return fmt.Errorf("failed to discover tools: %w", err)
 	}

internal/runtime/runtime.go

@@ -1251,6 +1251,15 @@ func (r *Runtime) ApplyConfig(newCfg *config.Config, cfgPath string) (*ConfigApp
 		r.cfgPath = cfgPath
 	}
 
+	// Propagate the new global config to the upstream manager and every running
+	// managed client so their background health-check loops re-resolve the new
+	// global health_check_interval (and Docker/discovery decisions) without a
+	// restart (spec 074, FR-012/SC-002). The loops re-read the interval each
+	// cycle, so this atomic swap is all that's needed.
+	if r.upstreamManager != nil {
+		r.upstreamManager.SetGlobalConfig(newCfg)
+	}
+
 	// Apply configuration changes to components
 	r.logger.Info("Applying configuration hot-reload",
 		zap.Strings("changed_fields", result.ChangedFields))

internal/runtime/tool_discovery_interval_test.go

@@ -8,21 +8,26 @@ import (
 )
 
 // TestPlanToolDiscoveryCycle covers the pure decision the background indexing
-// loop makes each iteration: a positive interval sweeps on that cadence; a
-// non-positive interval (disabled) skips the periodic sweep and waits the
-// fallback re-check window so a hot-reload can re-enable it (spec 074, FR-006).
+// loop makes each iteration: when at least one server (or the global default)
+// has a positive resolved interval, the loop sweeps and ticks at that cadence;
+// when every interval is disabled it skips the periodic sweep and waits the
+// fallback re-check window so a hot-reload can re-enable it (spec 074,
+// FR-006/FR-012). The tick itself is resolved by the upstream manager from its
+// thread-safe per-client snapshots (see TestResolveToolDiscoverySweepTick in the
+// upstream package) — deliberately not from r.Config().Servers, which would race
+// in-place config mutation in the background loop (MCP-1189).
 func TestPlanToolDiscoveryCycle(t *testing.T) {
 	const recheck = 5 * time.Minute
 
-	sweep, wait := planToolDiscoveryCycle(10*time.Minute, recheck)
-	assert.True(t, sweep, "positive interval must sweep")
-	assert.Equal(t, 10*time.Minute, wait)
+	sweep, wait := planToolDiscoveryCycle(10*time.Minute, true, recheck)
+	assert.True(t, sweep, "an enabled cadence must sweep")
+	assert.Equal(t, 10*time.Minute, wait, "loop ticks at the resolved cadence")
 
-	sweep, wait = planToolDiscoveryCycle(0, recheck)
-	assert.False(t, sweep, "0s must disable the periodic sweep")
+	sweep, wait = planToolDiscoveryCycle(0, false, recheck)
+	assert.False(t, sweep, "all-disabled must skip the periodic sweep")
 	assert.Equal(t, recheck, wait, "disabled cycle waits the re-check window")
 
-	sweep, wait = planToolDiscoveryCycle(-1, recheck)
-	assert.False(t, sweep, "negative interval must disable the periodic sweep")
+	sweep, wait = planToolDiscoveryCycle(30*time.Second, false, recheck)
+	assert.False(t, sweep, "anyEnabled=false dominates even with a positive tick")
 	assert.Equal(t, recheck, wait)
 }

internal/upstream/managed/client.go

@@ -35,8 +35,13 @@ type Client struct {
 	StateManager *types.StateManager // Public field for callback access
 
 	// Configuration for creating fresh connections
-	logConfig    *config.LogConfig
-	globalConfig *config.Config
+	logConfig *config.LogConfig
+	// globalConfig holds the proxy-wide config as an atomic pointer so a config
+	// hot-reload can swap it under the running background loops (health-check
+	// interval re-resolution, spec 074 FR-012) without a lock and without racing
+	// the readers. Mirrors the cfg atomic-pointer rationale above. Access via
+	// GetGlobalConfig() / SetGlobalConfig() only — never touch the field directly.
+	globalConfig atomic.Pointer[config.Config]
 	storage      *storage.BoltDB
 
 	// Connection state protection
@@ -116,12 +121,12 @@ func NewClient(id string, serverConfig *config.ServerConfig, logger *zap.Logger,
 		logger:         logger.With(zap.String("component", "managed_client")),
 		StateManager:   types.NewStateManager(),
 		logConfig:      logConfig,
-		globalConfig:   globalConfig,
 		storage:        storage,
 		stopMonitoring: make(chan struct{}),
 		healthProbe:    coreClient,
 	}
 	mc.cfg.Store(serverConfig)
+	mc.globalConfig.Store(globalConfig)
 
 	// Set up state change callback
 	mc.StateManager.SetStateChangeCallback(mc.onStateChange)
@@ -431,7 +436,8 @@ func (mc *Client) ShouldRetry() bool {
 // IsDockerIsolated returns true if this server will use Docker isolation.
 // Used to select appropriate connect timeouts (Docker containers need more time for package installation).
 func (mc *Client) IsDockerIsolated() bool {
-	if mc.globalConfig == nil || mc.globalConfig.DockerIsolation == nil || !mc.globalConfig.DockerIsolation.Enabled {
+	gc := mc.globalConfig.Load()
+	if gc == nil || gc.DockerIsolation == nil || !gc.DockerIsolation.Enabled {
 		return false
 	}
 	// Check if server has isolation explicitly disabled
@@ -781,11 +787,26 @@ func (mc *Client) stopBackgroundMonitoring() {
 // restart (spec 074, FR-012).
 const healthCheckDisabledRecheckInterval = 30 * time.Second
 
+// GetGlobalConfig returns the current proxy-wide config snapshot (may be nil for
+// hand-constructed test clients). Lock-free; safe to call whether or not mc.mu
+// is held.
+func (mc *Client) GetGlobalConfig() *config.Config {
+	return mc.globalConfig.Load()
+}
+
+// SetGlobalConfig swaps the proxy-wide config the background loops re-resolve
+// against. Called on a config hot-reload (via Manager.SetGlobalConfig) so the
+// resettable health-check timer picks up a new global interval without a restart
+// (spec 074, FR-012). Lock-free atomic swap.
+func (mc *Client) SetGlobalConfig(cfg *config.Config) {
+	mc.globalConfig.Store(cfg)
+}
+
 // resolveHealthCheckInterval resolves this server's effective health-check
 // interval (per-server override → global → built-in default). A nil
 // globalConfig (hand-constructed clients) falls back to the built-in default.
 func (mc *Client) resolveHealthCheckInterval() time.Duration {
-	cfg := mc.globalConfig
+	cfg := mc.globalConfig.Load()
 	if cfg == nil {
 		cfg = &config.Config{}
 	}

internal/upstream/managed/global_config_hotreload_test.go

@@ -0,0 +1,39 @@
+package managed
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+// TestSetGlobalConfig_HealthIntervalHotReload is the client-level proof of Codex
+// finding #2's fix: the background health loop re-resolves resolveHealthCheckInterval
+// every cycle, so swapping the global config via SetGlobalConfig changes the
+// effective cadence of a RUNNING client without a restart (spec 074, FR-012).
+func TestSetGlobalConfig_HealthIntervalHotReload(t *testing.T) {
+	mc := newTestClientForHealth(t)
+
+	// Boot global: 45s. The server has no per-server override, so the resolved
+	// interval is the global value.
+	mc.SetGlobalConfig(&config.Config{HealthCheckInterval: durPtrHC(45 * time.Second)})
+	assert.Equal(t, 45*time.Second, mc.resolveHealthCheckInterval(),
+		"resolved interval must follow the boot global")
+
+	// Operator edits the global cadence to 10s and ApplyConfig propagates it.
+	mc.SetGlobalConfig(&config.Config{HealthCheckInterval: durPtrHC(10 * time.Second)})
+	assert.Equal(t, 10*time.Second, mc.resolveHealthCheckInterval(),
+		"a global hot-reload must change a running client's resolved cadence")
+
+	// Disabling the global loop (0s) must propagate too.
+	mc.SetGlobalConfig(&config.Config{HealthCheckInterval: durPtrHC(0)})
+	assert.LessOrEqual(t, mc.resolveHealthCheckInterval(), time.Duration(0),
+		"0s global must disable the running client's probe loop")
+}
+
+func durPtrHC(d time.Duration) *config.Duration {
+	cd := config.Duration(d)
+	return &cd
+}

internal/upstream/managed/health_loop_test.go

@@ -36,7 +36,7 @@ func TestBackgroundHealthCheck_DisabledSkipsProbe(t *testing.T) {
 	mc.stopMonitoring = make(chan struct{})
 	fake := &fakeProber{}
 	mc.healthProbe = fake
-	mc.globalConfig = &config.Config{}
+	mc.SetGlobalConfig(&config.Config{})
 
 	cfg := mc.GetConfig()
 	zero := config.Duration(0)
@@ -59,7 +59,7 @@ func TestBackgroundHealthCheck_EnabledProbesOnInterval(t *testing.T) {
 	mc.stopMonitoring = make(chan struct{})
 	fake := &fakeProber{}
 	mc.healthProbe = fake
-	mc.globalConfig = &config.Config{}
+	mc.SetGlobalConfig(&config.Config{})
 
 	cfg := mc.GetConfig()
 	d := config.Duration(40 * time.Millisecond) // sub-validation value is fine for the loop mechanics