From 96122ef53c1ce4ca885ba6aa1510e8505077f290 Mon Sep 17 00:00:00 2001 From: Aron Gates Date: Fri, 11 Sep 2026 11:04:35 +0100 Subject: [PATCH] fix(vmcp): make the version poll interval per-Server, not a global TestPeriodicStatusReporting_ReactsToVersionChange assigned the package-level versionPollInterval while marked t.Parallel. Server.Start reads it for reconcileSessionsOnRegistryChange (#6549) and periodicStatusReporting reads it for its ticker, so it raced any parallel test that starts a server - TestReadinessEndpoint_DynamicMode_CacheNotSynced in practice - and the detector took several unrelated tests in the package down with it. Move the override onto Server and read it through s.pollInterval(), leaving a package-level default. The test now sets it on its own Server and stays parallel. This is what session_reconcile.go's doc comment already asks for: the interval "passed in ... so tests can drive it without mutating shared state that a parallel test also touches". Start was still handing it the global. Dropping t.Parallel would also have fixed the race but trips paralleltest, and would have left the global mutable for the next test to trip over. --- pkg/vmcp/server/server.go | 7 ++++++- pkg/vmcp/server/session_reconcile.go | 4 ++-- pkg/vmcp/server/status_reporting.go | 18 ++++++++++++++---- pkg/vmcp/server/status_reporting_test.go | 8 +++----- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/pkg/vmcp/server/server.go b/pkg/vmcp/server/server.go index c6deb3c6f9..66403d2d21 100644 --- a/pkg/vmcp/server/server.go +++ b/pkg/vmcp/server/server.go @@ -339,6 +339,11 @@ type Server struct { // Nil if status reporting is disabled. statusReporter vmcpstatus.Reporter + // versionPollInterval overrides how often the registry version is polled. + // Zero means the package default. Set per-Server so a test can shorten it + // without mutating shared state a parallel test also reads. + versionPollInterval time.Duration + // shutdownFuncs contains cleanup functions to run during Stop(). // Populated during Start() initialization before blocking; no mutex needed // since Stop() is only called after Start()'s select returns. @@ -880,7 +885,7 @@ func (s *Server) Start(ctx context.Context) error { // (#6546). Runs independently of status reporting; a no-op for static registries. if _, isDynamic := s.backendRegistry.(vmcp.DynamicRegistry); isDynamic && s.vmcpSessionMgr != nil { reconcileCtx, reconcileCancel := context.WithCancel(ctx) - go s.reconcileSessionsOnRegistryChange(reconcileCtx, versionPollInterval) + go s.reconcileSessionsOnRegistryChange(reconcileCtx, s.pollInterval()) s.shutdownFuncs = append(s.shutdownFuncs, func(context.Context) error { reconcileCancel() return nil diff --git a/pkg/vmcp/server/session_reconcile.go b/pkg/vmcp/server/session_reconcile.go index 31653eee74..62e54caa1f 100644 --- a/pkg/vmcp/server/session_reconcile.go +++ b/pkg/vmcp/server/session_reconcile.go @@ -23,8 +23,8 @@ import ( // membership never changes, so this returns immediately. // // The loop runs until ctx is cancelled (on server Stop). pollInterval is passed -// in (rather than read from the package-level versionPollInterval) so tests can -// drive it without mutating shared state that a parallel test also touches. +// in (rather than read from a package-level default) so tests can drive it +// without mutating shared state that a parallel test also touches. func (s *Server) reconcileSessionsOnRegistryChange(ctx context.Context, pollInterval time.Duration) { dynamicReg, isDynamic := s.backendRegistry.(vmcp.DynamicRegistry) if !isDynamic || s.vmcpSessionMgr == nil { diff --git a/pkg/vmcp/server/status_reporting.go b/pkg/vmcp/server/status_reporting.go index 21a8bc18b8..0617c34ff7 100644 --- a/pkg/vmcp/server/status_reporting.go +++ b/pkg/vmcp/server/status_reporting.go @@ -12,9 +12,19 @@ import ( vmcpstatus "github.com/stacklok/toolhive/pkg/vmcp/status" ) -// versionPollInterval is how often to check the registry version for changes. -// Exposed as a package-level var so tests can set a shorter interval. -var versionPollInterval = 2 * time.Second +// defaultVersionPollInterval is how often to check the registry version for +// changes when a Server does not override it. +const defaultVersionPollInterval = 2 * time.Second + +// pollInterval returns this Server's version-poll interval, falling back to the +// package default. Reading it per-Server rather than from a mutable package var +// is what lets a test shorten it while staying parallel-safe. +func (s *Server) pollInterval() time.Duration { + if s.versionPollInterval > 0 { + return s.versionPollInterval + } + return defaultVersionPollInterval +} // StatusReportingConfig configures periodic status reporting. type StatusReportingConfig struct { @@ -75,7 +85,7 @@ func (s *Server) periodicStatusReporting(ctx context.Context, config StatusRepor var versionTickerC <-chan time.Time var lastRegistryVersion uint64 if isDynamic { - versionTicker := time.NewTicker(versionPollInterval) + versionTicker := time.NewTicker(s.pollInterval()) defer versionTicker.Stop() versionTickerC = versionTicker.C } diff --git a/pkg/vmcp/server/status_reporting_test.go b/pkg/vmcp/server/status_reporting_test.go index 51ab1f502a..72f21e2207 100644 --- a/pkg/vmcp/server/status_reporting_test.go +++ b/pkg/vmcp/server/status_reporting_test.go @@ -168,15 +168,13 @@ func (r *testDynamicRegistry) Remove(_ string) error { func TestPeriodicStatusReporting_ReactsToVersionChange(t *testing.T) { t.Parallel() - // Speed up the version-polling ticker so the test completes in milliseconds. - orig := versionPollInterval - versionPollInterval = 10 * time.Millisecond - t.Cleanup(func() { versionPollInterval = orig }) - reporter := &mockReporter{} reg := &testDynamicRegistry{} server := &Server{ backendRegistry: reg, + // Speed up the version-polling ticker so the test completes in + // milliseconds. Per-Server, so this stays safe under t.Parallel. + versionPollInterval: 10 * time.Millisecond, } ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)