Skip to content

fix: StdioServerTransportProvider readiness barrier uses wrong Mono operator - #1138

Open
Debasishhh wants to merge 2 commits into
modelcontextprotocol:mainfrom
Debasishhh:fix/stdio-readiness-mono-zip-to-when
Open

Debasishhh wants to merge 2 commits into
modelcontextprotocol:mainfrom
Debasishhh:fix/stdio-readiness-mono-zip-to-when

Conversation

@Debasishhh

Copy link
Copy Markdown

Summary

Fixes #303.

StdioMcpSessionTransport#sendMessage gates the actual write on inboundReady and outboundReady (two Sinks.One<Void>) both completing before proceeding:

return Mono.zip(inboundReady.asMono(), outboundReady.asMono()).then(Mono.defer(() -> {
    ...
}));

Mono.zip is the wrong operator here. It's built to combine values, and it completes (empty) as soon as the first of the zipped sources completes without a value — it does not wait for the others. Since inboundReady/outboundReady are Mono<Void> (they only ever complete, never emit a value via tryEmitValue(null)), Mono.zip here only ever waits for whichever of the two signals fires first, silently breaking the intended "wait for both" barrier.

I verified this empirically before touching anything (both with tryEmitEmpty() and with the exact tryEmitValue(null) this code uses):

Mono.zip:  fired after only ONE of two Mono<Void> signals? true   <- bug
Mono.when: fired after only ONE of two Mono<Void> signals? false
Mono.when: fired after BOTH signals?                        true

Mono.when is the correct operator for waiting on multiple completion-only signals — it waits for all of them regardless of whether they emit a value.

Fix

One-line change: Mono.zip(...)Mono.when(...).

Test

Added monoZipDoesNotWaitForBothVoidSignals_monoWhenDoes to StdioServerTransportProviderTests, which reproduces the exact combinator pattern used by sendMessage and asserts:

  • Mono.zip incorrectly proceeds after only one of the two Mono<Void> signals fires.
  • Mono.when correctly waits for both.

I want to be upfront about scope: this is a targeted unit test of the reactive operator semantics, not a full black-box integration test driving the real threading race through sendMessage end-to-end — inboundReady/outboundReady are private fields on a private inner class with no injectable scheduler, so reproducing the exact race deterministically through the public API would need either reflection or new test seams, which felt like more than a one-line fix warrants. Happy to extend it if a maintainer would rather see that.

Verification

  • ./mvnw -pl mcp-core,mcp-test -am test — 1007 tests, 0 failures attributable to this change (one pre-existing flaky timeout in HttpServletStatelessIntegrationTests, unrelated HTTP transport module, confirmed to pass in isolation and on main without this change).
  • ./mvnw -pl mcp-core,mcp-test -am spring-javaformat:validate — clean.

Related PRs

A few prior PRs proposed the identical one-line fix but appear to have gone stale without review: #846, #981, #987. This PR adds a regression test on top of the same fix.

sendMessage waited on inboundReady and outboundReady (two Sinks.One<Void>)
via Mono.zip, but Mono.zip completes as soon as the first of two
value-less sources completes -- it never actually waits for the second.
Mono.when is the correct operator for waiting on multiple completion-only
signals, and does wait for both.

Fixes modelcontextprotocol#303
Pins down why Mono.when is the correct operator for the sendMessage
readiness barrier: verifies Mono.zip incorrectly proceeds after only one
of two Mono<Void> signals fires, while Mono.when correctly waits for
both.
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.

StdioServerTransportProvider uses incorrect Mono.zip operator

1 participant