diff --git a/pkgs/mcpx/docs/spec/conformance-matrix.md b/pkgs/mcpx/docs/spec/conformance-matrix.md index 23cbb9515..8c74df998 100644 --- a/pkgs/mcpx/docs/spec/conformance-matrix.md +++ b/pkgs/mcpx/docs/spec/conformance-matrix.md @@ -2,7 +2,7 @@ Generated by `internal/conformance` (`MCPX_UPDATE_MATRIX=1 go test ./internal/conformance -run TestMatrix`); do not edit. -Generated from inputs `e521a082e8f389bb` (sha256 of docs/spec/requirements.md, internal/conformance/overrides/*.json, internal/conformance/coverage*.go). `TestMatrix` regenerates this file and fails when it differs, so it describes the tree it is committed in. +Generated from inputs `ad5f12fd46e96bac` (sha256 of docs/spec/requirements.md, internal/conformance/overrides/*.json, internal/conformance/coverage*.go). `TestMatrix` regenerates this file and fails when it differs, so it describes the tree it is committed in. Every normative requirement of every revision (catalogue: `docs/spec/requirements.md`), for mcpx as a server and as a client. A requirement is *tested* when a named test verifies it in that revision, *n/a* when it does not bind mcpx in that role, and a *gap* when it binds mcpx and mcpx does not meet it yet, with the issue that tracks it. Run exactly the covering tests with `go run ./internal/conformance/cmd/covering`. @@ -28,7 +28,7 @@ Every normative requirement of every revision (catalogue: `docs/spec/requirement | 2025-03-26 | server | SHOULD NOT | 4 | 3 | 1 | 0 | 1 | | 2025-03-26 | server | MAY | 19 | 16 | 3 | 0 | 18 | | 2025-03-26 | server | other | 11 | 11 | 0 | 0 | 1 | -| 2025-03-26 | client | MUST | 41 | 29 | 12 | 0 | 40 | +| 2025-03-26 | client | MUST | 41 | 30 | 11 | 0 | 40 | | 2025-03-26 | client | MUST NOT | 12 | 11 | 1 | 0 | 3 | | 2025-03-26 | client | SHOULD | 44 | 32 | 12 | 0 | 59 | | 2025-03-26 | client | SHOULD NOT | 2 | 1 | 1 | 0 | 3 | @@ -40,7 +40,7 @@ Every normative requirement of every revision (catalogue: `docs/spec/requirement | 2025-06-18 | server | SHOULD NOT | 4 | 3 | 1 | 0 | 1 | | 2025-06-18 | server | MAY | 17 | 15 | 2 | 0 | 18 | | 2025-06-18 | server | other | 15 | 15 | 0 | 0 | 3 | -| 2025-06-18 | client | MUST | 52 | 35 | 17 | 0 | 49 | +| 2025-06-18 | client | MUST | 52 | 36 | 16 | 0 | 49 | | 2025-06-18 | client | MUST NOT | 13 | 11 | 2 | 0 | 5 | | 2025-06-18 | client | SHOULD | 52 | 38 | 14 | 0 | 63 | | 2025-06-18 | client | SHOULD NOT | 2 | 1 | 1 | 0 | 3 | @@ -52,7 +52,7 @@ Every normative requirement of every revision (catalogue: `docs/spec/requirement | 2025-11-25 | server | SHOULD NOT | 11 | 9 | 2 | 0 | 3 | | 2025-11-25 | server | MAY | 27 | 25 | 2 | 0 | 28 | | 2025-11-25 | server | other | 19 | 19 | 0 | 0 | 2 | -| 2025-11-25 | client | MUST | 76 | 54 | 22 | 0 | 104 | +| 2025-11-25 | client | MUST | 76 | 55 | 21 | 0 | 104 | | 2025-11-25 | client | MUST NOT | 16 | 14 | 2 | 0 | 19 | | 2025-11-25 | client | SHOULD | 67 | 45 | 22 | 0 | 92 | | 2025-11-25 | client | SHOULD NOT | 5 | 4 | 1 | 0 | 9 | @@ -760,7 +760,7 @@ Every normative requirement of every revision (catalogue: `docs/spec/requirement | [streamable-http-client-echoes-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-03-26/transport-streamable-http/streamable-http-client-echoes-session` | | [streamable-http-server-400-missing-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | SHOULD | server | n/a: mcpx does not require one (stateless fallback | n/a: addresses the other side | | [streamable-http-server-404-terminated-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | server | tested: `conformance.TestTransportServer/2025-03-26/transport-streamable-http/streamable-http-server-404-terminated-session` | n/a: addresses the other side | -| [streamable-http-client-reinit-on-404](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | **gap** #203: a 404 for the session is returned as an error; no new initialize | +| [streamable-http-client-reinit-on-404](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | tested: `mcpclient.TestLegacySession404Recovery/2025-11-25/transport/client-reinitializes-on-session-404-and-retries-once@2025-03-26,2025-06-18,2025-11-25` | | [streamable-http-client-delete-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | SHOULD | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-03-26/transport-streamable-http/streamable-http-client-delete-session` | | [streamable-http-server-may-405-delete](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MAY | server | tested: `conformance.TestTransportServer/2025-03-26/transport-streamable-http/streamable-http-server-may-405-delete` | n/a: addresses the other side | @@ -1190,7 +1190,7 @@ Every normative requirement of every revision (catalogue: `docs/spec/requirement | [streamable-http-client-echoes-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-06-18/transport-streamable-http/streamable-http-client-echoes-session` | | [streamable-http-server-400-missing-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | SHOULD | server | n/a: mcpx does not require one (stateless fallback | n/a: addresses the other side | | [streamable-http-server-404-terminated-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | server | tested: `conformance.TestTransportServer/2025-06-18/transport-streamable-http/streamable-http-server-404-terminated-session` | n/a: addresses the other side | -| [streamable-http-client-reinit-on-404](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | **gap** #203: a 404 for the session is returned as an error; no new initialize | +| [streamable-http-client-reinit-on-404](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | tested: `mcpclient.TestLegacySession404Recovery/2025-11-25/transport/client-reinitializes-on-session-404-and-retries-once@2025-03-26,2025-06-18,2025-11-25` | | [streamable-http-client-delete-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | SHOULD | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-06-18/transport-streamable-http/streamable-http-client-delete-session` | | [streamable-http-server-may-405-delete](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MAY | server | tested: `conformance.TestTransportServer/2025-06-18/transport-streamable-http/streamable-http-server-may-405-delete` | n/a: addresses the other side | | [streamable-http-protocol-version-header-is-negotiated](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#protocol-version-header) | SHOULD | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-06-18/transport-streamable-http/streamable-http-protocol-version-header-is-negotiated` | @@ -1806,7 +1806,7 @@ Every normative requirement of every revision (catalogue: `docs/spec/requirement | [streamable-http-client-echoes-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-11-25/transport-streamable-http/streamable-http-client-echoes-session` | | [streamable-http-server-400-missing-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | SHOULD | server | n/a: mcpx does not require one (stateless fallback | n/a: addresses the other side | | [streamable-http-server-404-terminated-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | server | tested: `conformance.TestTransportServer/2025-11-25/transport-streamable-http/streamable-http-server-404-terminated-session` | n/a: addresses the other side | -| [streamable-http-client-reinit-on-404](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | **gap** #203: a 404 for the session is returned as an error; no new initialize | +| [streamable-http-client-reinit-on-404](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MUST | client | n/a: addresses the other side | tested: `mcpclient.TestLegacySession404Recovery/2025-11-25/transport/client-reinitializes-on-session-404-and-retries-once@2025-03-26,2025-06-18,2025-11-25` | | [streamable-http-client-delete-session](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | SHOULD | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-11-25/transport-streamable-http/streamable-http-client-delete-session` | | [streamable-http-server-may-405-delete](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) | MAY | server | tested: `conformance.TestTransportServer/2025-11-25/transport-streamable-http/streamable-http-server-may-405-delete` | n/a: addresses the other side | | [streamable-http-protocol-version-header-is-negotiated](https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#protocol-version-header) | SHOULD | client | n/a: addresses the other side | tested: `conformance.TestTransportClient/2025-11-25/transport-streamable-http/streamable-http-protocol-version-header-is-negotiated` | diff --git a/pkgs/mcpx/docs/spec/requirements.md b/pkgs/mcpx/docs/spec/requirements.md index 4dab0e042..2c30e9da5 100644 --- a/pkgs/mcpx/docs/spec/requirements.md +++ b/pkgs/mcpx/docs/spec/requirements.md @@ -272,7 +272,7 @@ Most 2025-11-25 growth is the tasks page (57 rows) and the elicitation rewrite; | streamable-http-client-echoes-session | 2025-03-26..2025-11-25 | client | MUST | transport-streamable-http | "clients using the Streamable HTTP transport **MUST** include it in the `Mcp-Session-Id` header on all of their subsequent HTTP requests." | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | n/a | yes — also sends it to modern servers (2026 server SHOULD ignore) | | | streamable-http-server-400-missing-session | 2025-03-26..2025-11-25 | server | SHOULD | transport-streamable-http | "Servers that require a session ID **SHOULD** respond to requests without an `Mcp-Session-Id` header (other than initialization) with HTTP 400 Bad Request." | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | n/a — mcpx does not require one (stateless fallback) | n/a | | | streamable-http-server-404-terminated-session | 2025-03-26..2025-11-25 | server | MUST | transport-streamable-http | "The server **MAY** terminate the session at any time, after which it **MUST** respond to requests containing that session ID with HTTP 404 Not Found." | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | yes — an ended or unknown session id is 404; tested, see `conformance-matrix.md` | n/a | | -| streamable-http-client-reinit-on-404 | 2025-03-26..2025-11-25 | client | MUST | transport-streamable-http | "it **MUST** start a new session by sending a new `InitializeRequest` without a session ID attached." | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | n/a | **gap** — 404 surfaces as `http 404` error; session id kept | Under 2026 a 404 means "method not found" (C7). | +| streamable-http-client-reinit-on-404 | 2025-03-26..2025-11-25 | client | MUST | transport-streamable-http | "it **MUST** start a new session by sending a new `InitializeRequest` without a session ID attached." | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | n/a | yes — a session 404 triggers one re-initialize (no session id on it) and one retry on the new session; tested, see `conformance-matrix.md` | Under 2026 a 404 means "method not found" (C7). | | streamable-http-client-delete-session | 2025-03-26..2025-11-25 | client | SHOULD | transport-streamable-http | "Clients that no longer need a particular session ... **SHOULD** send an HTTP DELETE to the MCP endpoint with the `Mcp-Session-Id` header" | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | accepts — 204 | yes — on `Close` | | | streamable-http-server-may-405-delete | 2025-03-26..2025-11-25 | server | MAY | transport-streamable-http | "The server **MAY** respond to this request with HTTP 405 Method Not Allowed" | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management | chooses 204 | tolerates any status | | | streamable-http-protocol-version-header-is-negotiated | 2025-06-18..2025-11-25 | client | SHOULD | transport-streamable-http | "The protocol version sent by the client **SHOULD** be the one negotiated during initialization." | https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#protocol-version-header | n/a | yes — the header carries the negotiated revision; tested, see `conformance-matrix.md` | | diff --git a/pkgs/mcpx/internal/conformance/coverage_transport.go b/pkgs/mcpx/internal/conformance/coverage_transport.go index c2264d90e..8215c2c0d 100644 --- a/pkgs/mcpx/internal/conformance/coverage_transport.go +++ b/pkgs/mcpx/internal/conformance/coverage_transport.go @@ -16,6 +16,7 @@ func init() { C("streamable-http-x-mcp-header-client-must-mirror", "mcpclient.TestParamHeaders/2026-07-28/transport/param-headers-mirror-and-omit-null"), C("streamable-http-client-rejects-invalid-x-mcp-header", "mcpclient.TestToolHeaderAnnotations/2026-07-28/transport/client-rejects-invalid-x-mcp-header", "mcpclient.TestModernHTTPHeaders/2026-07-28/transport/client-excludes-tool-with-invalid-x-mcp-header"), C("streamable-http-client-relist-on-header-mismatch", "mcpclient.TestModernHTTPHeaders/2026-07-28/transport/client-relists-and-retries-on-header-mismatch"), + C("streamable-http-client-reinit-on-404", "mcpclient.TestLegacySession404Recovery/2025-11-25/transport/client-reinitializes-on-session-404-and-retries-once@2025-03-26,2025-06-18,2025-11-25"), C("streamable-http-client-reissue-broken-request", "mcpclient.TestModernReissueBrokenRequest/2026-07-28/transport/client-reissues-broken-request-once"), C("streamable-http-client-polls-after-close", "mcpclient.TestLegacyHTTPStreamResumption/2025-11-25/transport/client-resumes-closed-stream-with-last-event-id-after-retry"), C("streamable-http-client-respects-retry", "mcpclient.TestLegacyHTTPStreamResumption/2025-11-25/transport/client-resumes-closed-stream-with-last-event-id-after-retry"), @@ -112,7 +113,6 @@ func init() { ), ) registerGaps( - Gap{ID: "streamable-http-client-reinit-on-404", Side: Client, Issue: "#203", Why: "a 404 for the session is returned as an error; no new initialize"}, Gap{ID: "stdio-client-restart-crashed-server", Side: Client, Issue: "#257", Why: "unverified: the in-flight call fails and the pool respawns only on the next acquire, subject to its start-failure cooldown"}, ) } diff --git a/pkgs/mcpx/internal/conformance/requirements.json b/pkgs/mcpx/internal/conformance/requirements.json index 6a095c681..b23c9b4f4 100644 --- a/pkgs/mcpx/internal/conformance/requirements.json +++ b/pkgs/mcpx/internal/conformance/requirements.json @@ -4927,11 +4927,8 @@ "server": "n/a:addresses the other side" }, "notes": { - "client": "**gap** — 404 surfaces as `http 404` error; session id kept", + "client": "yes — a session 404 triggers one re-initialize (no session id on it) and one retry on the new session; tested, see `conformance-matrix.md`", "server": "n/a" - }, - "claims": { - "client": "gap" } }, { diff --git a/pkgs/mcpx/internal/mcpclient/client.go b/pkgs/mcpx/internal/mcpclient/client.go index 8e9e15d8b..c3d8de57e 100644 --- a/pkgs/mcpx/internal/mcpclient/client.go +++ b/pkgs/mcpx/internal/mcpclient/client.go @@ -11,6 +11,7 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "sync" "sync/atomic" "time" @@ -131,6 +132,13 @@ type Client struct { // (compact JSON), so an inbound notifications/cancelled can stop one. asked map[string]*askedReq relaySeq atomic.Int64 + // reinitMu singleflights legacy session recovery: N concurrent calls + // 404ing from one upstream restart produce exactly one initialize. + // reinitGen counts completed recoveries, so a call whose session + // generation was already replaced knows to retry without recovering + // again. + reinitMu sync.Mutex + reinitGen atomic.Uint64 // toolHeaders are each tool's x-mcp-header annotations, learned from // tools/list, for a modern connection over HTTP. toolHeaders map[string][]mcpheaders.Param @@ -1041,6 +1049,17 @@ func isStreamLost(err error) bool { // roundTrip sends one request and returns its raw result. // +// A legacy HTTP session the upstream has forgotten -- it restarted, and +// now answers a session-carrying request with 404 -- is recovered here, +// once: the dead session is forgotten, the client re-initializes, and the +// request is retried exactly once on the new session. The legacy +// revisions require exactly this: a client receiving 404 for a session +// MUST start a new session with an initialize carrying no session id. +// https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management +// (#1637, Phase 1 of #1634.) Every other failure -- a 404 with no session +// held, a modern-era 404, a second 404 after recovery -- surfaces as it +// always has. +// // On a modern connection a request whose response stream broke // unanswered (isStreamLost) is re-issued exactly once with a fresh id, // when allowReissue holds: 2026-07-28 has no resumption, so the broken @@ -1049,15 +1068,72 @@ func isStreamLost(err error) bool { // a caller cancellation is never re-issued. Each input_required round // is its own call to roundTrip, so each round re-issues independently. func (c *Client) roundTrip(ctx context.Context, method string, params json.RawMessage, allowReissue bool) (json.RawMessage, error) { + gen := c.reinitGen.Load() for attempt := 0; ; attempt++ { raw, err := c.roundTripOnce(ctx, method, params) - if err == nil || attempt > 0 || !allowReissue || c.Era != EraModern || ctx.Err() != nil || !isStreamLost(err) { + if err == nil { + return raw, err + } + if attempt == 0 && c.sessionGone(err, method) { + if !c.reinitializeLegacy(ctx, gen) { + return raw, err + } + return c.roundTripOnce(ctx, method, params) + } + if attempt > 0 || !allowReissue || c.Era != EraModern || ctx.Err() != nil || !isStreamLost(err) { return raw, err } } } -// roundTripOnce sends one request once and returns its raw result. +// sessionGone reports whether err is the narrow recovery trigger: a +// request (never initialize itself; notifications never reach roundTrip) +// failed with HTTP 404 on a legacy-era connection. +func (c *Client) sessionGone(err error, method string) bool { + // The atomic modern mirror, not c.Era: a concurrent recovery's + // finishLegacy writes Era, and this check runs on every failed call. + if method == "initialize" || c.modern.Load() { + return false + } + var he *HTTPStatusError + return errors.As(err, &he) && he.Status == http.StatusNotFound +} + +// reinitializeLegacy replaces a forgotten legacy session, singleflight +// across concurrent calls. gen is the recovery generation the failed call +// set out with: if it has already moved, another call replaced the +// session and this one simply retries on it. It reports whether the +// caller should retry its request. +func (c *Client) reinitializeLegacy(ctx context.Context, gen uint64) bool { + ft, ok := c.t.(interface{ forgetSession() bool }) + if !ok { + return false // stdio transports have no session to forget + } + c.reinitMu.Lock() + defer c.reinitMu.Unlock() + if c.reinitGen.Load() != gen { + return true + } + if !ft.forgetSession() { + return false + } + if err := c.initializeLegacy(ctx, c.clientName, c.clientVersion); err != nil { + return false + } + c.reinitGen.Add(1) + // A restarted upstream's catalogue is not the cached one. + c.mu.Lock() + n := c.notif + c.mu.Unlock() + for _, kind := range []string{"tools", "resources", "prompts"} { + c.invalidate(kind, n) + } + return true +} + +// roundTripOnce sends one request once and returns its raw result, with no +// recovery: both recoveries live in roundTrip, and the retries they make +// go through here so a call is never recovered twice. func (c *Client) roundTripOnce(ctx context.Context, method string, params json.RawMessage) (json.RawMessage, error) { id := c.nextID.Add(1) ch := make(chan *rpcResponse, 1) diff --git a/pkgs/mcpx/internal/mcpclient/http.go b/pkgs/mcpx/internal/mcpclient/http.go index 0aa64602c..b14bdc502 100644 --- a/pkgs/mcpx/internal/mcpclient/http.go +++ b/pkgs/mcpx/internal/mcpclient/http.go @@ -666,6 +666,23 @@ func (t *HTTPTransport) Resume(h HTTPHandoff) { t.sessionMu.Unlock() } +// forgetSession atomically reports and clears the held session id. It is +// the transport seam for legacy session recovery (#1637): the client calls +// it when a request has failed with 404 for the session, and the fresh +// initialize that follows must go out with no session id -- and no later +// request may echo the dead one. It reports whether a session was held at +// all: a stateless legacy server holds none, and its 404s are ordinary +// errors, not a forgotten session. +func (t *HTTPTransport) forgetSession() bool { + t.sessionMu.Lock() + defer t.sessionMu.Unlock() + if t.sessionID == "" { + return false + } + t.sessionID = "" + return true +} + // Abandon stops the transport without ending the upstream session, for a // session a successor process now holds. func (t *HTTPTransport) Abandon() { diff --git a/pkgs/mcpx/internal/mcpclient/reinit_test.go b/pkgs/mcpx/internal/mcpclient/reinit_test.go new file mode 100644 index 000000000..626a13061 --- /dev/null +++ b/pkgs/mcpx/internal/mcpclient/reinit_test.go @@ -0,0 +1,217 @@ +package mcpclient_test + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/dezren39/mcpx/internal/mcpclient" +) + +// reinitUpstream is a fake legacy Streamable HTTP upstream whose sessions +// can be dropped mid-life, as a restart drops them: a request carrying a +// session id the upstream no longer holds is answered 404. +type reinitUpstream struct { + version string + + mu sync.Mutex + seq int + live map[string]bool + stateless bool // never issue a session id + call404 bool // answer every tools/call 404, session or not + inits int + initSids []string // session header each initialize carried + callSids []string // session header each tools/call carried +} + +func (u *reinitUpstream) dropSessions() { + u.mu.Lock() + defer u.mu.Unlock() + u.live = map[string]bool{} +} + +func (u *reinitUpstream) counts() (inits int, initSids, callSids []string) { + u.mu.Lock() + defer u.mu.Unlock() + return u.inits, append([]string(nil), u.initSids...), append([]string(nil), u.callSids...) +} + +func (u *reinitUpstream) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + if r.Method == http.MethodDelete { + w.WriteHeader(http.StatusOK) + return + } + if r.Method != http.MethodPost { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + s := decodePost(r) + if len(s.id) == 0 { + w.WriteHeader(http.StatusAccepted) // notification + return + } + sid := s.header.Get("Mcp-Session-Id") + u.mu.Lock() + switch s.method { + case "initialize": + u.inits++ + u.initSids = append(u.initSids, sid) + if !u.stateless { + u.seq++ + newSid := fmt.Sprintf("sess-%d", u.seq) + if u.live == nil { + u.live = map[string]bool{} + } + u.live[newSid] = true + w.Header().Set("Mcp-Session-Id", newSid) + } + u.mu.Unlock() + writeJSON(w, result(s.id, initResult(u.version, map[string]any{"tools": map[string]any{}}))) + case "tools/call": + u.callSids = append(u.callSids, sid) + dead := u.call404 || (!u.stateless && !u.live[sid]) + u.mu.Unlock() + if dead { + w.WriteHeader(http.StatusNotFound) + return + } + writeJSON(w, result(s.id, map[string]any{"content": []any{map[string]any{"type": "text", "text": "ok"}}})) + default: + u.mu.Unlock() + writeJSON(w, result(s.id, map[string]any{"tools": []any{}})) + } +} + +// https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management +// (#1637: a legacy upstream that forgets the session mid-life is recovered +// by exactly one re-initialize and one retry.) +func TestLegacySession404Recovery(t *testing.T) { + for _, v := range []string{"2025-03-26", "2025-06-18", "2025-11-25"} { + t.Run(v+"/transport/client-reinitializes-on-session-404-and-retries-once", func(t *testing.T) { + u := &reinitUpstream{version: v} + srv := httptest.NewServer(u) + t.Cleanup(srv.Close) + c := dialURL(t, srv.URL, mcpclient.ForceLegacy, mcpclient.Options{}) + var changed []string + var mu sync.Mutex + c.Subscribe(mcpclient.Notifications{OnListChanged: func(kind string) { + mu.Lock() + changed = append(changed, kind) + mu.Unlock() + }}) + if _, err := c.CallTool(ctx5(), "x", nil); err != nil { + t.Fatal(err) + } + u.dropSessions() // the upstream restarted and forgot sess-1 + out, err := c.CallTool(ctx5(), "x", nil) + if err != nil { + t.Fatalf("call after session drop: %v", err) + } + if !strings.Contains(string(out), "ok") { + t.Fatalf("out %s", out) + } + inits, initSids, callSids := u.counts() + if inits != 2 { + t.Fatalf("initializes = %d, want exactly 2 (connect + one recovery)", inits) + } + if initSids[1] != "" { + t.Fatalf("recovery initialize carried session id %q, want none", initSids[1]) + } + if len(callSids) != 3 || callSids[1] != "sess-1" || callSids[2] != "sess-2" { + t.Fatalf("tools/call session ids %v, want [sess-1 sess-1 sess-2]", callSids) + } + mu.Lock() + defer mu.Unlock() + if len(changed) != 3 { + t.Fatalf("list-changed invalidations %v, want tools/resources/prompts", changed) + } + }) + } + + t.Run("concurrent-calls-share-one-reinitialize", func(t *testing.T) { + u := &reinitUpstream{version: "2025-11-25"} + srv := httptest.NewServer(u) + t.Cleanup(srv.Close) + c := dialURL(t, srv.URL, mcpclient.ForceLegacy, mcpclient.Options{}) + if _, err := c.CallTool(ctx5(), "x", nil); err != nil { + t.Fatal(err) + } + u.dropSessions() + const n = 8 + errs := make([]error, n) + var wg sync.WaitGroup + for i := range n { + wg.Add(1) + go func() { + defer wg.Done() + _, errs[i] = c.CallTool(ctx5(), "x", nil) + }() + } + wg.Wait() + for i, err := range errs { + if err != nil { + t.Fatalf("call %d: %v", i, err) + } + } + if inits, _, _ := u.counts(); inits != 2 { + t.Fatalf("initializes = %d, want exactly 2 for %d concurrent 404s", inits, n) + } + }) + + t.Run("stateless-404-is-an-ordinary-error", func(t *testing.T) { + u := &reinitUpstream{version: "2025-11-25", stateless: true, call404: true} + srv := httptest.NewServer(u) + t.Cleanup(srv.Close) + c := dialURL(t, srv.URL, mcpclient.ForceLegacy, mcpclient.Options{}) + _, err := c.CallTool(ctx5(), "x", nil) + var he *mcpclient.HTTPStatusError + if !errors.As(err, &he) || he.Status != http.StatusNotFound { + t.Fatalf("err = %v, want HTTP 404 as today", err) + } + if inits, _, _ := u.counts(); inits != 1 { + t.Fatalf("initializes = %d, want 1: no session held, no recovery", inits) + } + }) + + t.Run("second-404-surfaces-without-another-reinitialize", func(t *testing.T) { + u := &reinitUpstream{version: "2025-11-25", call404: true} + srv := httptest.NewServer(u) + t.Cleanup(srv.Close) + c := dialURL(t, srv.URL, mcpclient.ForceLegacy, mcpclient.Options{}) + _, err := c.CallTool(ctx5(), "x", nil) + var he *mcpclient.HTTPStatusError + if !errors.As(err, &he) || he.Status != http.StatusNotFound { + t.Fatalf("err = %v, want HTTP 404", err) + } + if inits, _, _ := u.counts(); inits != 2 { + t.Fatalf("initializes = %d, want 2 (connect + the single recovery attempt)", inits) + } + }) + + t.Run("modern-404-is-not-recovered", func(t *testing.T) { + rec := &recorder{} + srv := modernHTTP(t, rec, func() []any { return []any{headerTool} }, func(s seen) (int, any) { + return http.StatusNotFound, map[string]any{"detail": "no such tool"} + }) + c := dialURL(t, srv.URL, mcpclient.ForceModern, mcpclient.Options{}) + _, err := c.CallTool(ctx5(), "sql", map[string]any{"query": "select 1"}) + var he *mcpclient.HTTPStatusError + if !errors.As(err, &he) || he.Status != http.StatusNotFound { + t.Fatalf("err = %v, want HTTP 404 as today", err) + } + if got := len(rec.all("tools/call")); got != 1 { + t.Fatalf("tools/call posts = %d, want 1: a modern 404 is never retried", got) + } + if got := len(rec.all("initialize")); got != 0 { + t.Fatalf("initialize posts = %d, want 0", got) + } + }) +} diff --git a/pkgs/mcpx/internal/pool/upstream_session_test.go b/pkgs/mcpx/internal/pool/upstream_session_test.go index 7c8262797..ae68237ea 100644 --- a/pkgs/mcpx/internal/pool/upstream_session_test.go +++ b/pkgs/mcpx/internal/pool/upstream_session_test.go @@ -261,8 +261,14 @@ func TestAnExpiredUpstreamSessionIsReinitialised(t *testing.T) { up.expireAll() handOff(t, old, successor) - if st := successor.Status(); st.Live != 0 { - t.Fatalf("an expired session must not be served, %d live", st.Live) + // Since #1637 the recovery lives in the client, not the pool: the + // probe ping's 404 makes the adopted client forget the dead session, + // re-initialize, and retry, so the handed-on instance is served on + // the replacement session instead of being dropped for the next + // call to replace (#1634: client-level recovery is transparent and + // immediate; pool reaping was the rejected alternative). + if st := successor.Status(); st.Live != 1 { + t.Fatalf("an expired session should be reinitialised in place, %d live", st.Live) } pingThrough(t, successor, "k") up.mu.Lock() @@ -270,4 +276,7 @@ func TestAnExpiredUpstreamSessionIsReinitialised(t *testing.T) { if up.issued != 2 { t.Fatalf("the upstream issued %d sessions; an expired one should be replaced once", up.issued) } + if len(up.served) == 0 || up.served[len(up.served)-1] != "sess-2" { + t.Fatalf("requests served on sessions %v, want the last on the replacement sess-2", up.served) + } } diff --git a/treefmt.toml b/treefmt.toml index 8e456d8e1..8a69ceb9d 100755 --- a/treefmt.toml +++ b/treefmt.toml @@ -3,14 +3,16 @@ # Exclude pkgs/id/ — it has its own treefmt/formatter [global] excludes = [ - "pkgs/id/**", # has its own treefmt/formatter - "pkgs/dht/**", # WIP package with syntax errors - ".opencode/**", # managed by opencode, may have permission issues - "**/package-lock.json", # npm-generated; biome ignores it by default, so passing it in fails the run - "pkgs/cold-store-reaper/worker/dist/**", # esbuild bundle output; deployed as built, formatting it would rewrite the artifact - "pkgs/mcpx/docs/parity.md", # generated; TestParityDocumentIsCurrent compares it byte-for-byte with the generator, so prettier's table padding can never be applied - "pkgs/mcpx/docs/spec/conformance-matrix.md", # generated; TestMatrix compares it byte-for-byte with the generator, so prettier's table padding can never be applied - "pkgs/mcpx/docs/story.md", # quotes AGENTS.md verbatim; TestStoryDocumentIsCurrent compares the quoted lines byte-for-byte with AGENTS.md, whose *emphasis* prettier would rewrite to _emphasis_ + "pkgs/id/**", # has its own treefmt/formatter + "pkgs/dht/**", # WIP package with syntax errors + ".opencode/**", # managed by opencode, may have permission issues + "**/package-lock.json", # npm-generated; biome ignores it by default, so passing it in fails the run + "pkgs/cold-store-reaper/worker/dist/**", # esbuild bundle output; deployed as built, formatting it would rewrite the artifact + "pkgs/mcpx/docs/parity.md", # generated; TestParityDocumentIsCurrent compares it byte-for-byte with the generator, so prettier's table padding can never be applied + "pkgs/mcpx/docs/spec/conformance-matrix.md", # generated; TestMatrix compares it byte-for-byte with the generator, so prettier's table padding can never be applied + "pkgs/mcpx/docs/spec/requirements.md", # catalogue source; genreq parses its tables and TestMatrix hashes it, so prettier's table padding rewrites every row and can never be applied + "pkgs/mcpx/internal/conformance/requirements.json", # generated by cmd/genreq; TestMatrix compares it byte-for-byte with the generator's Encode form, so biome's JSON formatting can never be applied + "pkgs/mcpx/docs/story.md", # quotes AGENTS.md verbatim; TestStoryDocumentIsCurrent compares the quoted lines byte-for-byte with AGENTS.md, whose *emphasis* prettier would rewrite to _emphasis_ ] # [formatter.]