feat: add streamable HTTP transport, and fix the panic that makes HTTP transports unusable with --argo-http1 - #215
Open
ngkaizhe wants to merge 6 commits into
Open
Conversation
Sync fork with upstream main
Under an HTTP transport, tool calls run on a context derived from the incoming HTTP request, while the Argo client's metadata (its logger, the Kubernetes client) lives on the process-wide context built in main. Under stdio the two are the same context, so this never surfaced. With --argo-http1 the mismatch is fatal: the Argo REST facade calls logging.RequireLoggerFromContext, which panics on a context without a logger instead of returning an error, and the panic takes the whole process down. Any tool call against an Argo Server over HTTP/1.1 kills the server, so a remote deployment behind a reverse proxy cannot serve a single request. Make each request's context fall back to the server context for value lookups while keeping its own cancellation and deadline.
Add --transport streamable-http (MCP_TRANSPORT=streamable-http), the transport that supersedes HTTP+SSE in the MCP specification. The existing "http" value keeps serving HTTP+SSE, so nothing changes for current users. HTTP+SSE routes every server-to-client message through one long-lived GET stream. Behind a reverse proxy that closes idle streams - Envoy's stream_idle_timeout defaults to 300s - losing that stream strands the session, because later responses have nowhere to go. Streamable HTTP gives each POST its own response stream, so the same cut costs nothing. Unlike the SSE handler, which answers on every path, the streamable handler is mounted on /mcp alone, with a plain liveness endpoint on /healthz. A proxy can then publish the MCP endpoint without publishing anything else the process serves, and a container probe has a target that means what it says: GET /mcp without a session is a client error, not a sign the server is unhealthy.
Wire the new transport through configuration: --transport streamable-http and MCP_TRANSPORT=streamable-http now start it. The "http" value keeps serving HTTP+SSE, so existing deployments are unaffected. Validation accepts all three modes and requires an address for either HTTP-based one. Multi-context stays stdio-only, as before.
Dispatch on the configured transport so streamable-http starts the new server, and log the endpoint path at startup. Document the transport alongside HTTP+SSE: what each one costs behind a reverse proxy, the /mcp URL clients need, and the reminder to state the bind address explicitly in a container.
Cover both remote transports: what each costs behind a reverse proxy that closes idle streams, the /mcp URL clients need, and the reminder to state the bind address explicitly in a container.
Collaborator
That would always be my preference. Easier to review smaller standalone PRs. |
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
Two related changes for running this server remotely. The fix comes first because without it the new transport — and the existing one — cannot serve a single tool call against an Argo Server over HTTP/1.1.
1. Fix: HTTP transports lose the Argo client's context metadata
Under an HTTP transport, tool calls run on a context derived from the incoming HTTP request. The Argo client's metadata — its logger, the Kubernetes client — lives on the process-wide context
mainbuilds and installs viaargoClient.Context(). Under stdio these are the same context, so the gap never showed.With
--argo-http1it is fatal.http1.Facade.docallslogging.RequireLoggerFromContext, which panics on a context without a logger rather than returning an error, and the panic takes the process down.Reproduced against released v0.4.0 (so this is not introduced by this PR):
Handshake succeeds, then the first
tools/callgives:and the container exits (2). gRPC mode does not panic, so this is specific to the HTTP/1.1 REST client — which is exactly the mode you need when Argo Server sits behind an nginx ingress.
The fix makes each request's context fall back to the server context for value lookups only, keeping the request's own cancellation and deadline. It repairs the existing
httptransport as much as the new one.Happy to split this into its own PR if you would rather land it separately — it stands alone.
2. Feature:
--transport streamable-httpAdds the transport that supersedes HTTP+SSE in the MCP spec.
httpkeeps serving HTTP+SSE unchanged, so nothing breaks for current users.Why it matters for remote deployments: HTTP+SSE routes every server→client message through one long-lived
GETstream. Behind a proxy that closes idle streams — Envoy'sstream_idle_timeoutdefaults to 300s — losing that stream strands the session, because later responses have nowhere to go. Streamable HTTP gives each POST its own response stream, so the same cut costs nothing.Two deployment details, both learned from putting this behind a gateway:
/mcpalone, not on the root. The SSE handler answers on every path (GET /healthzreturns an SSEendpointevent, not a health check), so a proxy cannot publish the MCP endpoint without publishing everything else./healthzreturns a plain 200. A container probe needs a target that means what it says;GET /mcpwithout a session is a client error, not a sign the server is unhealthy.Changes
internal/server/http.go—withServerValuesmiddleware (the fix);RunStreamableHTTP; sharedservehelper for both transportsinternal/config/config.go—streamable-httptransport value,IsStreamableHTTPTransport,UsesHTTPListener, validationcmd/mcp-for-argo-workflows/main.go— dispatch on the configured transportREADME.md— both remote transports, the/mcpclient URL, the bind-address noteMCP_TRANSPORTparsingTest plan
gofmt -s -l,go vet ./...,golangci-lint v2.12.2 run ./...(0 issues), andgo test -race ./internal/... ./pkg/...all clean — run against exactly the commits in this PR, not a local variant.End-to-end against a real Argo Workflows v3.7 server behind an nginx ingress, with
--argo-http1:GET /healthzGET //sse/metricsinitializeonPOST /mcpMcp-Session-Idissuedtools/call list_workflows--transport http(SSE)Notes
I did not add a
--read-only-style flag for the streamable handler'sStatelessoption. Stateless mode would drop the single-replica constraint for horizontally scaled deployments and might be worth a follow-up, but it changes server→client request semantics, so it felt out of scope here.