Echo the request id in session-not-found 404s - #6032
Closed
jhrozek wants to merge 1 commit into
Closed
Conversation
The transparent proxy's unknown-session guard returned a -32001 JSON-RPC error with a null id, while every sibling error path echoes the request id, so a client correlating responses by id got null for this one error class. Thread the parsed id through to NotFoundBody. Closes #5945 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
blkt,
rdimitrov and
reyortiz3
as code owners
July 27, 2026 16:01
Contributor
Author
|
Closing as a duplicate of #6031, which was opened a few minutes before this one and does the same thing better. I missed that #6031 already existed before starting — my fault for not checking open PRs against #5945 first. Two things #6031 gets right that this PR doesn't:
Nothing here worth porting over. #6030 also already covers the |
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
-32001(session not found) 404 whose body hardcoded"id":null. Every sibling rejection path —mcp.ClassificationErrorResponse, and the streamable proxy'sresolveSessionForRequest— echoes the incoming JSON-RPC id, so a client correlating responses by id gotnullfor this one error class and nothing else.session.NotFoundResponsenow takes arequestIDand forwards it toNotFoundBody, which already accepted one. The transparent proxy passes the id it has already parsed.RoundTripand not visible at the guard 20 lines below. Hoisting it torpcIDis the whole production change.Closes #5945
Type of change
Test plan
task test-e2e)task lint-fix)Note on scope, so the boxes aren't read as more than they are. What I actually ran:
The flags mirror
test-unixlikein the Taskfile. Thego vetis there becausetask testfilters out/test/e2e, so the e2e file this PR edits would otherwise never be compiled locally.Not run locally: full
task test,task lint-fix,task test-e2e. Leaving those to CI.Changes
pkg/transport/session/jsonrpc_errors.goNotFoundResponsetakesrequestID anyinstead of hardcodingNotFoundBody(nil)pkg/transport/proxy/transparent/transparent_proxy.goNotFoundResponsepkg/transport/session/jsonrpc_errors_test.goTestNotFoundResponsebecomes table-driven: nil, string, raw-JSON, and nil-json.RawMessageidspkg/transport/proxy/transparent/revision_guard_regression_test.goTestGuardUnknownSessionFiresDespiteForgedModernRevisionalready sent"id":1; now asserts it comes backtest/e2e/hostile_input_proxy_test.goidNotEchoedcase field, its branch, and the comment documenting this bug as known-and-deferred — the session-guard case was its only user and now echoes like every siblingDoes this introduce a user-facing change?
Yes, though a small one. A client that sends a request with an unknown or expired
Mcp-Session-Idthrough the transparent proxy now gets its JSON-RPC id echoed in the-32001error response instead ofnull, so the error can be matched to the request that caused it. Requests that genuinely carry no id — GET on the SSE stream, DELETE, or a body that isn't a single JSON-RPC request — still return"id":null.Special notes for reviewers
A few things worth knowing:
json.RawMessagenil handling.rpcIDis passed through as ajson.RawMessageboxed intoany. A nil one marshals tonull(RawMessage.MarshalJSONspecial-cases the nil slice), so the no-id case is byte-identical to before. This is the same value and the same code pathClassificationErrorResponse(req, id, cerr)already takes two lines earlier, so it isn't introducing a new edge case.parseRPCRequestnever returns a non-nil-but-empty id, which is the one input that would produce an invalid body. There are unit cases pinning both nil forms.#5883 was investigated in the same pass and closed without code. It reported that a batch containing
initializeskips this same guard. That turned out to be already fixed by #5931 (a18f17e2d), which added an unconditionalmcp.IsBatchRequestrejection attransparent_proxy.go:591— a 400 that lands 33 lines before the guard — and replaced the old test withTestRoundTripRejectsBatch, whose first case is verbatim the payload from that report. Closed with the details in a comment.One thing I deliberately left alone. The batch-scanning branch in
parseRPCRequestis now unreachable fromRoundTrip, but it's still what would setsawInitialize = truefor a batch. It's the mechanism that would resurrect #5883 if the batch rejection at:591is ever moved or relaxed. Deleting it would make the function fail-closed, but that's a separate change from this one and I didn't want to mix it in. Flagging it for whoever edits that guard next.🤖 Generated with Claude Code