Skip to content

fix(vmcp): make the version poll interval per-Server, not a global - #6644

Open
aron-muon wants to merge 2 commits into
stacklok:mainfrom
aron-muon:fix/vmcp-server-versionpollinterval-race
Open

fix(vmcp): make the version poll interval per-Server, not a global#6644
aron-muon wants to merge 2 commits into
stacklok:mainfrom
aron-muon:fix/vmcp-server-versionpollinterval-race

Conversation

@aron-muon

@aron-muon aron-muon commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Tests is red on main under -race:

WARNING: DATA RACE
Write at 0x... by goroutine 34:
  server.TestPeriodicStatusReporting_ReactsToVersionChange.func1()
      pkg/vmcp/server/status_reporting_test.go:174
Previous read at 0x... by goroutine 72:
  server.(*Server).Start()
      pkg/vmcp/server/server.go:883
  server_test.TestReadinessEndpoint_DynamicMode_CacheNotSynced.func1()

TestPeriodicStatusReporting_ReactsToVersionChange shortens the ticker by assigning the package-level versionPollInterval, restoring it in t.Cleanup, but it's t.Parallel(). Server.Start reads that same variable when wiring reconcileSessionsOnRegistryChange (#6549), and periodicStatusReporting reads it for its own ticker, so any parallel test that starts a server races it. The detector fails the whole binary, so unrelated tests go red with it - TestListChangedSink_EndToEnd_*, TestAuthzCallGate, TestServeHandlerMetricsOnTransportPort.

This PR moves the override onto Server and reads it through s.pollInterval(), keeping a package-level default. The test sets it on its own Server and stays parallel.

That's 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 just still handing it the global.

I first tried simply dropping t.Parallel(). It fixes the race but trips paralleltest, and leaves the global mutable for the next test to trip over, so this seemed the better shape.

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Reproduced on a pristine main at 7e52ab9, deterministic over repeated runs:

go test -race -count=1 -run 'TestListChangedSink_EndToEnd|TestAuthzCallGate|TestReadinessEndpoint_DynamicMode_CacheNotSynced|TestServeHandlerMetricsOnTransportPort|TestPeriodicStatusReporting_ReactsToVersionChange' ./pkg/vmcp/server/
--- 5 failures

With this change that filter is exit 0 over 3 runs, go test -race ./pkg/vmcp/server/... passes, and golangci-lint run ./pkg/vmcp/... is 0 issues (including paralleltest).

Does this introduce a user-facing change?

No.

@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.92%. Comparing base (e532cf0) to head (93ef5ad).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6644      +/-   ##
==========================================
- Coverage   78.98%   78.92%   -0.06%     
==========================================
  Files         782      782              
  Lines       78065    78068       +3     
==========================================
- Hits        61658    61617      -41     
- Misses      16402    16446      +44     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhrozek

jhrozek commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

hmm I guess the lint issue should be fixed seprately? We recently merged a whole bunch of dep and toolchain bumps, those are likely to have introduced new linter findings

@aron-muon
aron-muon force-pushed the fix/vmcp-server-versionpollinterval-race branch from c4e40c2 to 888e409 Compare September 11, 2026 09:57
TestPeriodicStatusReporting_ReactsToVersionChange assigned the
package-level versionPollInterval while marked t.Parallel. Server.Start
reads it for reconcileSessionsOnRegistryChange (stacklok#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.
@aron-muon
aron-muon force-pushed the fix/vmcp-server-versionpollinterval-race branch from 888e409 to 96122ef Compare September 11, 2026 10:04
@aron-muon aron-muon changed the title fix(vmcp): don't run the versionPollInterval test in parallel fix(vmcp): make the version poll interval per-Server, not a global Sep 11, 2026
@aron-muon

Copy link
Copy Markdown
Contributor Author

Reworked after CI caught that dropping t.Parallel() trips paralleltest. Moved the override onto Server behind s.pollInterval() instead, which keeps the test parallel and removes the mutable global entirely - closer to what session_reconcile.go's comment already asks for. Rebased onto current main (7e52ab9, Go 1.27).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants