fix(test): register the GetServerLogs fixture client only after PhaseReady - #1228
Merged
Conversation
…Ready TestGetServerLogs_MissingFileReturnsEmptyNotError (#1216) flaked on a loaded machine during the full `go test -race ./internal/server/...` run with "server not found: never-logged", while passing every time in isolation. CAUSE. newLogsTestServer registered the upstream client by calling UpstreamManager().AddServerConfig at construction. Background initialization then runs LoadConfiguredServers, which snapshots the manager's clients, diffs them against cfg.Servers and removes the difference in goroutines. A client added before that diff is on the removal list. On an idle machine the test finished first; under load the removal landed in between. The re-register-at-call-site pattern from diagnostics_fixers_test.go (ensureFixerClient, #1218) is necessary but NOT sufficient here: it fixes "server not found", but the removal's own "Disconnecting from server" line creates the per-server log file this test asserts is absent, so the test still failed 2/100 under -race with `Should be empty, but was [...]`. The fixer tests never hit that because they seed the log file themselves. FIX. Wait for the runtime to report PhaseReady before registering. The phase flips only after LoadConfiguredServers has taken its snapshot, so a client added afterwards is never on a removal list, and the supervisor's periodic reconcile only removes names it found in a config snapshot. The call-site re-registration (ensureLogsClient) is kept as well, mirroring the fixer tests; re-registering an unchanged config keeps the existing client and logs nothing. Assertions are unchanged. REPRO. A 3s sleep between construction and the call fails on the old fixture and passes with this change. Verified: go test -race -count=5 -run 'TestGetServerLogs_' ./internal/server ok same with -count=50, twice, under 6 CPU hogs 200/200 golangci-lint (.github/.golangci.yml) 0 issues
Deploying mcpproxy-docs with
|
| Latest commit: |
81e2450
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://80b15b1f.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-logs-test-reconcile-race.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 34260706349 --repo smart-mcp-proxy/mcpproxy-go
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TestGetServerLogs_MissingFileReturnsEmptyNotError(from #1216) flaked once on a loaded machine during the fullgo test -race ./internal/server/...run withserver not found: never-logged, while passing 3/3 in isolation.Cause.
newLogsTestServerregistered the upstream client viaUpstreamManager().AddServerConfigat construction. Background initialization then runsLoadConfiguredServers, which snapshots the manager's clients, diffs them againstcfg.Servers, and removes the difference in goroutines. A client added before that diff is on the removal list; under load the removal landed between construction and the call.Why the #1218 pattern alone was not enough. Applying only
ensureFixerClient-style re-registration at the call site fixedserver not foundbut still failed 2/100 under-race: the removal's ownDisconnecting from serverline creates the per-server log file this test asserts is absent (Should be empty, but was [...]). The fixer tests never see that because they seed the log file themselves.Fix. Wait for the runtime to reach
PhaseReadybefore registering; the phase flips only afterLoadConfiguredServershas taken its snapshot, so a client added afterwards is never on a removal list, and the supervisor's periodic reconcile only removes names from a config snapshot. The call-site re-registration is kept too, mirroring the fixer tests. Assertions unchanged.Verification
go test -race -count=5 -run 'TestGetServerLogs_' ./internal/server— ok-count=50, twice, with 6 CPU hogs running — 200/200 pass (re-register-only variant: 2/100 fail)golangci-lint run --config .github/.golangci.yml ./internal/server/...— 0 issuesNote for #1218:
newFixerTestServerhas the same construction-time registration and could in principle lose its client to the same one-shot removal before the call-site re-register; it only survives because it re-registers immediately before invoking the fixer and writes its own log file. ThePhaseReadywait here would harden it too if it ever flakes.