Skip to content

Route server-to-client MCP messages per session - #5934

Merged
JAORMX merged 7 commits into
mainfrom
streamable-server-to-client-sse
Jul 23, 2026
Merged

Route server-to-client MCP messages per session#5934
JAORMX merged 7 commits into
mainfrom
streamable-server-to-client-sse

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

The Streamable HTTP proxy returned 405 on GET and dropped every container→client message (dispatcher.go: "Notifications are ignored for streamable HTTP"). Server-initiated MCP flows — progress, resources/updated, */list_changed, sampling, elicitation — never reached clients over our default transport (the legacy SSE proxy forwarded them; the modern one silently lost them).

This implements a standalone GET SSE stream and routes each server→client message to the correct session — or securely refuses it. The design goal was correctness and zero cross-session leakage over a shared-backend multiplexing proxy, not incremental scoping.

  • Per-session delivery streams: a registry keyed by Mcp-Session-Id, at most one standalone GET SSE stream per session (evict-and-replace on reconnect). Each stream has a dedicated stop signal and its data channel is never closed, so a client disconnecting during fan-out can't panic: send on closed channel in the dispatcher goroutine.
  • Routing by message type (delivery is targeted, never broadcast-to-all):
    • notifications/{tools,resources,prompts}/list_changed → every connected session once (genuinely global; payload-free; the follow-up tools/list is re-filtered on the POST path — no filter bypass).
    • notifications/progress → the originating request's own POST SSE stream, correlated by rewriting the client's progressToken to a proxy-minted unique token (eliminates cross-session token collisions; the client's original token is restored before delivery).
    • notifications/resources/updatedonly sessions subscribed to that URI (per-session subscription registry; ref-counted so the shared backend receives exactly one subscribe/unsubscribe).
    • notifications/message (logging) and server→client requests (sampling/createMessage, elicitation/create) are structurally unattributable over a single shared backend, so they are securely dropped — forwarding them would leak one client's data to another, or let one client answer another's elicitation. Server requests get a JSON-RPC -32601 back so the backend unblocks; the upstream logging level is still reconciled to the max verbosity across sessions.
  • Per-URI keyed mutex orders concurrent same-URI subscribe/unsubscribe so the backend never desyncs from the routing table's ref-count.

GET is behind the same auth/middleware chain as POST. Standalone SSE is on by default (opt-out via WithStandaloneSSE(false) → 405). The delivery/routing layer is transport-agnostic so the 2026-07-28 subscriptions/listen replacement plugs into the same registry.

Closes #5744

Type of change

  • Bug fix (protocol feature loss on the default transport; also closes a would-be cross-session information-disclosure surface)

Test plan

  • Unit tests (task test) — affected packages pass; new unit tests for the stream registry, sessionRouter (subscriptions/progress/logging + reap), the keyed mutex, and the dispatcher routing table.
  • go test -race ./pkg/transport/proxy/streamable/... -count=5 — green, incl. concurrent dispatch+evict and concurrent-registry stress tests (no panics/races).
  • Linting (task lint-fix) — 0 issues.
  • Security regression tests: progress does not leak to a non-originating session (two sessions reusing the same client token); resources/updated reaches subscribers only; upstream subscribe/unsubscribe ref-counted once; logging dropped + upstream level = cross-session max; sampling/elicitation never reach a client and error to the backend; authz-denied subscribe never enters the routing table; concurrent same-URI subscribe/unsubscribe ordered (mutation-verified — the assertion fails without the fix); lifecycle (handleDelete purge, reaper, request-scoped progress cleanup).

Does this introduce a user-facing change?

Yes. Clients connected over the Streamable HTTP transport now receive server-initiated notifications (progress on their request stream, resource-update notifications for their subscriptions, and capability list_changed), which were previously dropped. GET /mcp now opens an SSE stream by default instead of returning 405. Sampling/elicitation and per-session logging remain unavailable on the shared-backend thv run proxy (they require a per-session/vMCP deployment) and return a clean error rather than being mis-delivered.

Special notes for reviewers

  • Developed via an architect→implement→panel pipeline with Opus review panels (security + MCP-spec + Go-concurrency), iterated twice. The panel initially found two blockers — a send-on-closed-channel panic and a spec MUST-NOT (broadcast to all streams) — both fixed; then a subscribe/unsubscribe ordering race, fixed with the per-URI keyed mutex. Final Opus passes confirmed: cross-session leakage eliminated on every path, MCP 2025-11-25 conformant, and the keyed mutex correct/deadlock-free/leak-free.
  • Honest architectural boundary: over a single shared backend the proxy cannot attribute server-initiated requests (sampling/elicitation) or log lines to a downstream session — proven in the design, not assumed. Correct support needs per-session backend connections (the vMCP model). This PR refuses them securely and documents the follow-up rather than mis-delivering.
  • Large but cohesive (mostly tests + docs; core is dispatcher_routing.go, routing in dispatcher.go, and the POST-SSE interleaving in streamable_proxy.go). Requested as a single complete change. docs/arch/03-transport-architecture.md updated.
  • Follow-ups (documented): SSE resumability (Last-Event-ID; a spec MAY), sampling/elicitation + per-session logging via per-session backends, and task-augmented progress after CreateTaskResult.

Generated with Claude Code

@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Jul 23, 2026
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.66771% with 168 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.79%. Comparing base (3dee7d6) to head (690aafb).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/transport/proxy/streamable/streamable_proxy.go 60.79% 93 Missing and 25 partials ⚠️
pkg/transport/proxy/streamable/dispatcher.go 65.06% 22 Missing and 7 partials ⚠️
pkg/transport/proxy/streamable/utils.go 66.00% 11 Missing and 6 partials ⚠️
...g/transport/proxy/streamable/dispatcher_streams.go 94.59% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5934      +/-   ##
==========================================
+ Coverage   71.77%   71.79%   +0.02%     
==========================================
  Files         702      708       +6     
  Lines       71901    72734     +833     
==========================================
+ Hits        51608    52222     +614     
- Misses      16593    16775     +182     
- Partials     3700     3737      +37     

☔ 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.

@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 23, 2026
JAORMX and others added 3 commits July 23, 2026 11:31
The streamable HTTP proxy returned 405 on GET and dropped every
container-originated message, so server-initiated MCP flows (progress,
resources/updated, */list_changed, sampling, elicitation) never reached
clients over the modern transport. This adds a standalone GET SSE stream
and routes each server->client message to the correct session, or refuses
it — never leaking one client's data to another over the shared backend.

- Per-session GET SSE streams (registry keyed by Mcp-Session-Id, one
  stream per session, evict-and-replace on reconnect); a per-stream stop
  signal, never closing the data channel, so a disconnect during fan-out
  can't panic the dispatcher.
- server/discover-free routing by message type:
  - */list_changed -> every session once (genuinely global);
  - notifications/progress -> the originating request's own POST SSE
    stream, correlated by rewriting the client progressToken to a
    proxy-minted unique token (kills cross-session token collisions);
  - resources/updated -> only sessions subscribed to that URI
    (ref-counted so the shared backend sees one subscribe/unsubscribe);
  - logging/message and server->client requests (sampling/elicitation)
    are unattributable over a shared backend, so they are securely
    dropped (requests get a JSON-RPC error back so the backend unblocks);
    logging level is still reconciled upstream to the max verbosity.
- A per-URI keyed mutex orders concurrent same-URI subscribe/unsubscribe
  so the backend never desyncs from the routing table.

GET is behind the same auth/middleware chain as POST; tools/list_changed
re-filtering runs through the existing tool-filter middleware (no bypass).
Sampling/elicitation and SSE resumability require per-session backends
(vMCP) and are documented follow-ups.

Closes #5744

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
Post-review cleanup (duplication + library-reuse pass): no behavior
change.

- Extract startSSEStream / assertFlushable / setSSEHeaders /
  writeSSEKeepAlive helpers so the three SSE-serving sites (handleGet,
  handleSingleRequestSSE, writeInterceptedResponse) and the keep-alive
  loop stop repeating the flusher-check + header trio + framing.
- Reuse toolhive-core mcpcompat constants MethodSetLogLevel and
  LoggingLevel* instead of hardcoded "logging/setLevel" and the log-level
  key strings; collect the remaining (non-exported) notification method
  names into one local const block instead of scattered literals.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
tools-call-elicitation exercises a server-INITIATED request (elicitation
during a tool call). Over the shared single-backend `thv run` proxy this
cannot be correlated to a downstream session, so the proxy refuses it
securely (-32601) rather than mis-delivering — see the routing rationale
in dispatcher.go. This records it as a known, accepted conformance gap
(the mechanism the baseline file exists for); correct support needs
per-session backends (the vMCP model). The proxy now passes every other
active conformance scenario. Tracked as a #5744 follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
@JAORMX
JAORMX force-pushed the streamable-server-to-client-sse branch from fcf8edf to 1417961 Compare July 23, 2026 11:32
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 23, 2026
Reword two test comments/strings flagged by the CI codespell action
("keep-alives", "GET's"). Comment-only; no behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 23, 2026

@amirejaz amirejaz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read closely through the server->client routing and session lifecycle since that's the delicate part here. The overall design holds up well under concurrency - the never-close-data/close-stop-once pattern, the keyed mutex ref-counting, and the per-request progress-token UUID isolation all look correct. A few things stood out around how subscription state and streams are handled on failure and teardown paths; left them inline. The subscribe-rollback and GET-stream-teardown ones look like real bugs to me, the rest are more design questions / suggestions.

Comment thread pkg/transport/proxy/streamable/streamable_proxy.go Outdated
Comment thread pkg/transport/proxy/streamable/streamable_proxy.go Outdated
Comment thread pkg/transport/proxy/streamable/streamable_proxy.go
Comment thread pkg/transport/proxy/streamable/streamable_proxy.go Outdated
Comment thread pkg/transport/proxy/streamable/streamable_proxy.go
The previous commit wrongly listed tools-call-elicitation in
expected-failures.yaml; the scenario actually PASSES ~2/3 of runs, so the
baseline entry stale-failed CI. Like tools-call-sampling (#5888), it
flakes due to upstream conformance#407 — the reference server races its
server-initiated elicitation/create on the standalone GET SSE stream — so
it belongs in the run-conformance.sh quarantine, not the baseline.

Revert expected-failures.yaml to empty and add tools-call-elicitation to
the quarantine grep alongside tools-call-sampling.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 23, 2026
Fixes from @amirejaz's review and an Opus dual-spec (2025-11-25 +
2026-07-28) verification pass:

- Record a resources/subscribe only AFTER the upstream subscribe
  succeeds. Previously the subscriber was recorded before forwarding, so
  a rejected/timed-out first subscribe left a phantom entry that deduped
  every later session to a synthesized success while nobody was actually
  subscribed upstream. interceptSubscribe now forwards first and records
  only on a success response; on error it records nothing and returns the
  real error. (Also honors the durable-before-in-memory rule.)
- Tear down a session's standalone GET SSE stream on DELETE and in the
  reaper (serverStreamRegistry.closeStream + reapServerStreams); the
  stream and its goroutine no longer outlive the session.
- Scope the per-URI lock to the intercept+forward only, not the whole
  request, removing cross-session head-of-line blocking on the SSE
  response.
- Docs: soften the subscriptions/listen (2026-07-28) forward-compat note
  (it needs re-keying per-subscription-id, per-type/URI opt-in filtering,
  ack and tagging over a long-lived POST — not a drop-in), and document
  the subscribe-dedup trust boundary (backend-side per-URI checks only
  see the first subscriber).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 23, 2026
Comment thread pkg/transport/proxy/streamable/dispatcher_standalone_sse_integration_test.go Outdated
Comment thread pkg/transport/proxy/streamable/keyed_mutex.go
Address @amirejaz's second-pass test-quality feedback:

- Replace the fixed 150ms sleep in the logging max-verbosity test with
  require.Never, so a slow-but-present extra upstream reconcile (which a
  sleep would miss) actively fails the test.
- Add direct keyedMutex unit tests (keyed_mutex_test.go): different keys
  proceed concurrently (guards against a regression to a single global
  lock that integration tests wouldn't catch), refCount==0 eviction (the
  unbounded-URI memory-leak guard), same-key serialization, and the
  sync.Once double-unlock no-op.

Test-only; no change to reviewed logic.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 23, 2026
@JAORMX
JAORMX merged commit c007e6a into main Jul 23, 2026
150 of 154 checks passed
@JAORMX
JAORMX deleted the streamable-server-to-client-sse branch July 23, 2026 16:50
ChrisJBurns added a commit that referenced this pull request Jul 23, 2026
The middleware returns HTTP 200 with a JSON-RPC error body when the
client accepts JSON (absent Accept header → clientAcceptsJSON = true).
This is correct per the MCP streamable-HTTP spec: application-level
errors ride in a 200 response, not a 400. The test added in #5934 was
asserting 400, which never matched the production code path.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
JAORMX added a commit that referenced this pull request Jul 24, 2026
TestStandaloneSSE_ListChangedRefiltersThroughExistingMiddleware asserted
that a tools/call for a filtered tool returns HTTP 400. That is no longer
true: #5944 changed the tool-call filter to return a JSON-RPC error over
HTTP 200 (a filtered tool is now indistinguishable from a nonexistent
one). This test was added by #5934 and the two PRs landed close together,
so each was green in isolation but the combination left the Tests check
red on main (a semantic merge collision) — deterministically failing every
subsequent PR's required unit-test job.

Update step (3) to assert the current behavior: HTTP 200 with a JSON-RPC
error body ("tool not found"). The tool is still blocked (never forwarded);
only the response envelope changed.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forward server-to-client messages in the streamable HTTP proxy

2 participants