Repository navigation
fix(upstream): make the late-callback test deterministic under CI load - #1692
Merged
Merged
Conversation
Related #1586 TestManager_LateCallbacks_ReachExistingClients timed out on every PR Build run since it merged, and today on the required Unit Tests job too. The cause is in mcp-go v1.0.0's streamable-HTTP server: in-request notifications are forwarded to the SSE response by a goroutine, and when the tool handler returns the request goroutine takes the stream mutex, drains the queue and closes done, so the forwarder discards the one notification it had already dequeued. Which of the three notifications is lost depends on scheduling, so the single warmup did not cover it; under CPU contention (2x ncpu busy loops) the test failed 9 of 20 runs locally. The test now repeats the notify call until both late-registered callbacks have fired (500 ms settle per attempt, 10 s deadline) and the callbacks are non-blocking signals so a repeat can never block core's notification goroutine. Under the same contention it passed 25/25 plain and 20/20 with -race. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ck test A forwarder left over from an earlier request can discard a later call's notification as well, so the comment no longer claims a fixed minimum per attempt (opencode Sol 6.1 review, low). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
2101716
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d4160ac7.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-late-callbacks-flake.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 39416617955 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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
TestManager_LateCallbacks_ReachExistingClients(added by #1586) has timed out on every PR Build run since it merged and today started failing the required Unit Tests (ubuntu-latest, 1.26) and Unit Tests (shuffle) jobs as well, on PRs that cannot have caused it (for example the docs-only #1688: PR Build, Unit Tests).Cause. In
github.com/mark3labs/mcp-go@v1.0.0server/streamable_http.go(handlePost), in-request notifications are forwarded to the SSE response by a goroutine. When the tool handler returns, the request goroutine takes the stream mutex, drains what is still queued, and closesdone; the forwarder, which had already dequeued one notification and was waiting on that mutex, then hits the non-resumablecase <-done: returnand discards it. So exactly one of the notifications a call sends can be lost, and which one depends on scheduling. The test's single warmup notification absorbs the loss only when the forwarder happens to be holding the warmup; on a loaded runner it is the tools or prompts notification often enough to fail every run.Reproduction. 2×ncpu busy loops (
yes > /dev/null) alongside the test: 9 of 20 runs failed onmain.Fix (test only). The call to the
notifytool is repeated until both late-registered callbacks have fired (500 ms settle per attempt, 10 s deadline), and the callbacks are non-blocking signals so a repeated call can never block core's notification goroutine. The property the test guards is unchanged: callbacks registered after the client exists must be invoked ontools/list_changedandprompts/list_changed; a plumbing regression still fails after the deadline with a message naming which callback never fired.Verification. Under the same contention: 25/25 plain, 20/20 with
-race.go test -race ./internal/upstream/...green. golangci-lint v2 (.github/.golangci.yml, bare and--build-tags server) clean on the changed file.Related #1586
🤖 Generated with Claude Code