Cover Serve-path lifecycle for AS runner, status, optimizer, health

pkg/vmcp/server/serve.go

@@ -165,11 +165,16 @@ type ServerConfig struct {
 // admission seam (#5438). The injected core is stored on the *Server, which makes the
 // "/" MCP route serveable: the shared Handler guards the discovery middleware to the
 // legacy path (s.core == nil), and on the Serve path session registration and request
-// handlers call the core directly (#5442). The remaining transport concern — the AS
-// runner / status reporter / optimizer / health monitor lifecycle (#5443) — is
-// relocated under Serve by the next Phase 2 task. server.New is not yet routed through
-// Serve and keeps its own copy of the session wiring until Phase 3, so this is purely
-// additive and observable behavior is unchanged.
+// handlers call the core directly (#5442). The last transport subsystems — the embedded
+// AS runner routes, the status reporter (and its periodic goroutine), the optimizer
+// cleanup, and the health monitor's Start/Stop (#5443) — are driven from the
+// carried-forward shared (*Server).Handler/Start/Stop using the fields Serve populates
+// from ServerConfig below. Serve does NOT construct the health monitor: it receives the
+// pre-built *health.Monitor via ServerConfig (nil ⇒ disabled) and owns only its
+// lifecycle, while the composition root injects the same instance into the core as a
+// health.StatusProvider. server.New is not yet routed through Serve and keeps its own
+// copy of the session wiring until Phase 3, so this is purely additive and observable
+// behavior is unchanged.
 //
 // Serve returns a vmcp.ErrInvalidConfig-wrapped error for a nil cfg, a nil core, or a
 // nil required collaborator (SessionManagerConfig or BackendRegistry). The session
@@ -180,8 +185,9 @@ type ServerConfig struct {
 // but a nil discovery manager, router, and backend client — the request path goes
 // through the core, so those legacy collaborators are unused on the Serve path. The
 // shared Handler skips the discovery middleware when s.core != nil, so serving the "/"
-// MCP route no longer nil-derefs. The AS runner / status reporter / optimizer / health
-// monitor lifecycle is still wired by #5443.
+// MCP route no longer nil-derefs. The embedded AS runner routes, the status reporter,
+// the optimizer cleanup, and the health monitor's Start/Stop are driven from the shared
+// Handler/Start/Stop via the fields Serve populates from ServerConfig (#5443).
 func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) {
 	if cfg == nil {
 		return nil, fmt.Errorf("%w: nil server config", vmcp.ErrInvalidConfig)

pkg/vmcp/server/serve_lifecycle_test.go

@@ -0,0 +1,303 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package server
+
+import (
+	"context"
+	"errors"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
+
+	asrunner "github.com/stacklok/toolhive/pkg/authserver/runner"
+	"github.com/stacklok/toolhive/pkg/vmcp"
+	"github.com/stacklok/toolhive/pkg/vmcp/health"
+	"github.com/stacklok/toolhive/pkg/vmcp/mocks"
+	"github.com/stacklok/toolhive/pkg/vmcp/optimizer"
+	"github.com/stacklok/toolhive/pkg/vmcp/server/sessionmanager"
+)
+
+// This file covers the four transport subsystems relocated under Serve in #5443:
+// the embedded AS runner routes, the status reporter (+ its periodic goroutine), the
+// optimizer cleanup, and the health monitor's Start/Stop. The wiring itself lives in
+// ServerConfig and the carried-forward shared (*Server).Handler/Start/Stop; these tests
+// prove a Serve-built *Server drives each subsystem's lifecycle the same way the
+// server.New path does (whose New-path coverage lives in health_monitoring_test.go,
+// status_reporting_test.go, and server_test.go). server.New behavior is unchanged.
+
+// startServeInBackground starts srv on a fresh cancelable context and waits for it to
+// become ready, failing fast on early error or timeout. It returns an idempotent stop
+// function that cancels the server and waits for Start to return, yielding Start's result
+// (Stop's error) so callers can assert a clean shutdown with require.NoError(t, stop()).
+//
+// stop is also registered with t.Cleanup, so the Start goroutine and the bound HTTP
+// listener are always torn down even when a mid-test require aborts via runtime.Goexit
+// before the test reaches its explicit stop (testing.md: tie resource teardown for a
+// started server in a parallel test to t.Cleanup, not to a code path a failing require can
+// skip). The sync.Once lets the explicit call and the cleanup safety net coexist without
+// double-draining errCh.
+func startServeInBackground(t *testing.T, srv *Server) (stop func() error) {
+	t.Helper()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	errCh := make(chan error, 1)
+	go func() { errCh <- srv.Start(ctx) }()
+
+	select {
+	case <-srv.Ready():
+	case err := <-errCh:
+		cancel()
+		t.Fatalf("server failed to start: %v", err)
+	case <-time.After(2 * time.Second):
+		cancel()
+		t.Fatal("timeout waiting for server to become ready")
+	}
+
+	var once sync.Once
+	var stopErr error
+	stop = func() error {
+		once.Do(func() {
+			cancel()
+			select {
+			case stopErr = <-errCh:
+			case <-time.After(2 * time.Second):
+				stopErr = errors.New("timeout waiting for server to stop")
+			}
+		})
+		return stopErr
+	}
+	t.Cleanup(func() { _ = stop() })
+	return stop
+}
+
+// TestServeHealthMonitorDisabledWhenNil verifies that a nil ServerConfig.HealthMonitor
+// leaves monitoring disabled on the Serve path: Serve stores no monitor and the getters
+// report "disabled" without error, matching the no-monitor behavior of server.New.
+func TestServeHealthMonitorDisabledWhenNil(t *testing.T) {
+	t.Parallel()
+
+	srv, err := Serve(context.Background(), &stubVMCP{}, testMinimalServeConfig())
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = srv.Stop(context.Background()) })
+
+	assert.Nil(t, srv.healthMonitor, "nil ServerConfig.HealthMonitor must leave the monitor unset")
+
+	status, err := srv.GetBackendHealthStatus("backend-1")
+	require.Error(t, err)
+	assert.Equal(t, vmcp.BackendUnknown, status)
+	assert.Contains(t, err.Error(), "health monitoring is disabled")
+
+	assert.Equal(t, health.Summary{}, srv.GetHealthSummary())
+}
+
+// TestServeStartsAndStopsInjectedHealthMonitor verifies the health-monitor lifecycle on
+// the Serve path: Serve stores the pre-built *health.Monitor it is given (it does NOT
+// construct one), Start runs it (backends report healthy), and Stop stops it while
+// retaining the struct so getters keep working — mirroring TestServer_Stop_StopsHealthMonitor
+// for a monitor Serve was handed rather than one it built.
+func TestServeStartsAndStopsInjectedHealthMonitor(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	mockBackendClient := mocks.NewMockBackendClient(ctrl)
+	mockBackendClient.EXPECT().
+		ListCapabilities(gomock.Any(), gomock.Any()).
+		Return(&vmcp.CapabilityList{}, nil).
+		AnyTimes()
+
+	backends := []vmcp.Backend{
+		{ID: "backend-1", Name: "Backend 1", BaseURL: "http://localhost:8080", TransportType: "sse"},
+	}
+	mon, err := health.NewMonitor(mockBackendClient, backends, health.MonitorConfig{
+		CheckInterval:      50 * time.Millisecond,
+		UnhealthyThreshold: 1,
+		Timeout:            5 * time.Second,
+	})
+	require.NoError(t, err)
+
+	cfg := testMinimalServeConfig()
+	cfg.HealthMonitor = mon
+	cfg.BackendRegistry = vmcp.NewImmutableRegistry(backends)
+
+	srv, err := Serve(context.Background(), &stubVMCP{}, cfg)
+	require.NoError(t, err)
+
+	// Serve must reuse the injected instance, not build a new one (AC2: Serve does not
+	// call health.NewMonitor).
+	assert.Same(t, mon, srv.healthMonitor)
+
+	stop := startServeInBackground(t, srv)
+
+	require.Eventually(t, func() bool {
+		status, statusErr := srv.GetBackendHealthStatus("backend-1")
+		return statusErr == nil && status == vmcp.BackendHealthy
+	}, 2*time.Second, 10*time.Millisecond, "backend-1 should become healthy via the Serve-started monitor")
+
+	require.NoError(t, stop())
+
+	// The monitor is stopped but the struct is retained (pointer stays valid).
+	srv.healthMonitorMu.RLock()
+	assert.Same(t, mon, srv.healthMonitor, "health monitor should still exist after Stop")
+	srv.healthMonitorMu.RUnlock()
+
+	status, err := srv.GetBackendHealthStatus("backend-1")
+	assert.NoError(t, err, "getter should not error after Stop")
+	assert.NotEqual(t, vmcp.BackendUnknown, status, "should return last known status")
+}
+
+// TestServeDisablesHealthMonitorOnStartFailure verifies graceful degradation when the
+// injected monitor cannot start: Start logs a WARN and sets healthMonitor to nil so the
+// getters report "disabled", and Serve.Start itself does not fail. The failure is forced
+// with a monitor that has already been stopped — health.Monitor.Start refuses to restart.
+func TestServeDisablesHealthMonitorOnStartFailure(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	mockBackendClient := mocks.NewMockBackendClient(ctrl)
+
+	// No backends => Start spawns no health-check goroutines (ListCapabilities is never
+	// called) and Stop returns immediately, leaving the monitor in the "stopped" state.
+	mon, err := health.NewMonitor(mockBackendClient, nil, health.MonitorConfig{
+		CheckInterval:      time.Second,
+		UnhealthyThreshold: 1,
+		Timeout:            time.Second,
+	})
+	require.NoError(t, err)
+	require.NoError(t, mon.Start(context.Background()))
+	require.NoError(t, mon.Stop())
+
+	cfg := testMinimalServeConfig()
+	cfg.HealthMonitor = mon
+
+	srv, err := Serve(context.Background(), &stubVMCP{}, cfg)
+	require.NoError(t, err)
+
+	stop := startServeInBackground(t, srv)
+
+	// Start must not fail; the un-restartable monitor is disabled (set to nil) instead.
+	require.Eventually(t, func() bool {
+		srv.healthMonitorMu.RLock()
+		defer srv.healthMonitorMu.RUnlock()
+		return srv.healthMonitor == nil
+	}, 2*time.Second, 10*time.Millisecond, "a monitor whose Start fails must be disabled")
+
+	_, getErr := srv.GetBackendHealthStatus("backend-1")
+	assert.ErrorContains(t, getErr, "health monitoring is disabled")
+
+	require.NoError(t, stop())
+}
+
+// recordingReporter is a vmcpstatus.Reporter that records whether Start and its returned
+// shutdown func ran and counts ReportStatus calls, so the status-reporter lifecycle can
+// be asserted on the Serve path.
+type recordingReporter struct {
+	mu             sync.Mutex
+	started        bool
+	shutdownCalled bool
+	reportCount    int
+}
+
+func (r *recordingReporter) Start(context.Context) (func(context.Context) error, error) {
+	r.mu.Lock()
+	r.started = true
+	r.mu.Unlock()
+	return func(context.Context) error {
+		r.mu.Lock()
+		r.shutdownCalled = true
+		r.mu.Unlock()
+		return nil
+	}, nil
+}
+
+func (r *recordingReporter) ReportStatus(context.Context, *vmcp.Status) error {
+	r.mu.Lock()
+	r.reportCount++
+	r.mu.Unlock()
+	return nil
+}
+
+func (r *recordingReporter) snapshot() (started, shutdownCalled bool, reportCount int) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	return r.started, r.shutdownCalled, r.reportCount
+}
+
+// TestServeStartsAndStopsStatusReporter verifies the status-reporter lifecycle on the
+// Serve path: Start invokes ServerConfig.StatusReporter.Start, launches
+// periodicStatusReporting (at least one report fires), and appends the reporter shutdown
+// + the goroutine cancel to shutdownFuncs so both run on Stop.
+func TestServeStartsAndStopsStatusReporter(t *testing.T) {
+	t.Parallel()
+
+	reporter := &recordingReporter{}
+	cfg := testMinimalServeConfig()
+	cfg.StatusReporter = reporter
+	cfg.StatusReportingInterval = 20 * time.Millisecond
+
+	srv, err := Serve(context.Background(), &stubVMCP{}, cfg)
+	require.NoError(t, err)
+
+	stop := startServeInBackground(t, srv)
+
+	require.Eventually(t, func() bool {
+		started, _, count := reporter.snapshot()
+		return started && count >= 1
+	}, 2*time.Second, 10*time.Millisecond, "Start must run the reporter and emit at least one status report")
+
+	require.NoError(t, stop())
+
+	_, shutdownCalled, _ := reporter.snapshot()
+	assert.True(t, shutdownCalled, "the status reporter shutdown func must run on Stop via shutdownFuncs")
+}
+
+// TestServeWithOptimizerStartsAndStopsCleanly exercises the optimizer-configured Serve
+// path end to end: a non-nil OptimizerConfig drives sessionmanager.New into building a
+// real SQLite-backed optimizer factory whose cleanup (store.Close, not the no-op) Serve
+// appends to shutdownFuncs, and Start/Stop runs that construct→teardown path without
+// error. The cleanup's store.Close is internal to sessionmanager/optimizer, so it is not
+// observed here (asserting it would reach across the package boundary, see testing.md
+// "Test Scope"); that shutdownFuncs are drained on Stop is proven observably by
+// TestServeStopClosesCore. This test guards that configuring an optimizer does not break
+// Serve construction or shutdown — which the no-optimizer path would not exercise.
+func TestServeWithOptimizerStartsAndStopsCleanly(t *testing.T) {
+	t.Parallel()
+
+	cfg := testMinimalServeConfig()
+	cfg.SessionManagerConfig = &sessionmanager.FactoryConfig{
+		Base:            testMinimalFactory(),
+		OptimizerConfig: &optimizer.Config{},
+	}
+
+	srv, err := Serve(context.Background(), &stubVMCP{}, cfg)
+	require.NoError(t, err)
+
+	stop := startServeInBackground(t, srv)
+	require.NoError(t, stop())
+}
+
+// TestServeWiresAuthServerIntoConfig verifies the embedded AS runner wiring on the Serve
+// path: Serve carries ServerConfig.AuthServer into the *Config the shared Handler reads,
+// where `if s.config.AuthServer != nil { RegisterHandlers(mux) }` registers the routes.
+// The RegisterHandlers code path itself is covered by TestRegisterHandlers in
+// pkg/authserver/runner — the concrete *EmbeddedAuthServer cannot be meaningfully
+// constructed here (see the note in server_test.go), so this asserts the Serve-specific
+// mapping with a zero-value instance rather than building the route mux.
+func TestServeWiresAuthServerIntoConfig(t *testing.T) {
+	t.Parallel()
+
+	as := &asrunner.EmbeddedAuthServer{}
+	cfg := testMinimalServeConfig()
+	cfg.AuthServer = as
+
+	srv, err := Serve(context.Background(), &stubVMCP{}, cfg)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = srv.Stop(context.Background()) })
+
+	assert.Same(t, as, srv.config.AuthServer,
+		"Serve must carry ServerConfig.AuthServer into the Config the shared Handler registers routes from")
+}