Flaky: close of closed channel in vmcp backend session tests

[jhrozek]

Summary

TestCreateMCPClient_ContinuousListeningGatedOnSink shares one listChangedTestBackend across two parallel subtests, each of which connects its own MCP client. The backend's OnRegisterSession hook does an unguarded close(b.ready), so when both clients register a session the channel is closed twice and the test binary dies with panic: close of closed channel.

Because the panic aborts the whole binary, it surfaces as a failure of whichever unrelated test happened to be running — e.g. TestInitAndQueryCapabilities_FatalErrors — which makes it look like a different flake each time.

Root cause

pkg/vmcp/session/internal/backend/mcp_session_test.go:236, inside the hook registered by newListChangedTestBackend:

hooks.AddOnRegisterSession(func(_ context.Context, session mcpserver.ClientSession) {
    ...
    close(b.ready)   // fires once PER SESSION, not once per backend
})

newListChangedTestBackend is called once at mcp_session_test.go:344, and the two subtests below it both call t.Parallel() and each build a client against that same backend URL. Two clients → two notifications/initialized → registerAndSync → the hook runs twice.

The two sibling tests that actually consume b.ready (:415, :484) each construct their own backend, so they were never the problem. Ironically the test that triggers this one doesn't use b.ready at all — it only asserts ContinuousListening().

Stack trace from a local reproduction:

panic: close of closed channel

goroutine 33485 [running]:
...backend.newListChangedTestBackend.func1(...)
	pkg/vmcp/session/internal/backend/mcp_session_test.go:236 +0x6ac
toolhive-core/mcpcompat/server.(*Hooks).registerSession(...)
	mcpcompat/server/hooks.go:46
toolhive-core/mcpcompat/server.(*MCPServer).registerAndSync(...)
	mcpcompat/server/session.go:358 +0x2e4
go-sdk/mcp.(*ServerSession).initialized(...)
	go-sdk@v1.7.0/mcp/server.go:1427 +0x2e4

This is a test-scaffolding bug, not a product bug. A hook that fires once per registered session is correct behaviour; the test helper wrongly assumed exactly one session would ever register.

Reproduction

Does not reproduce at default parallelism. It needs varied GOMAXPROCS:

go test -race -count=40 -cpu=1,2,8 ./pkg/vmcp/session/internal/backend/

Fires within ~3 iterations of that loop. Plain -count=60 (default -cpu) stayed green over 60 runs, which is why it only shows up on CI runners.

Proposed fix

Guard the close so the helper is correct no matter how many sessions register:

 type listChangedTestBackend struct {
 	url string

-	ready chan struct{}
+	ready     chan struct{}
+	readyOnce sync.Once

 	mu        sync.Mutex

-		close(b.ready)
+		b.readyOnce.Do(func() { close(b.ready) })

Verified: with this applied, 6 × -count=40 -cpu=1,2,8 (720 runs) produced no panic, against a baseline that reproduced within 3 iterations.

Alternative — give each subtest in TestCreateMCPClient_ContinuousListeningGatedOnSink its own backend. That fixes this call site but leaves the helper fragile for the next test that shares one, so the sync.Once is preferable.

Impact

Random unrelated test failures in Tests / Test Go Code across PRs, each pointing at a different test name. Low severity, high noise.