Skip to content

mcp: send no response for a request the peer cancelled - #1267

Open
jmrplens wants to merge 1 commit into
modelcontextprotocol:mainfrom
jmrplens:jmrp-no-response-for-a-cancelled-request
Open

jmrplens wants to merge 1 commit into
modelcontextprotocol:mainfrom
jmrplens:jmrp-no-response-for-a-cancelled-request

Conversation

@jmrplens

Copy link
Copy Markdown
Contributor

The MCP cancellation utility says a receiver of notifications/cancelled SHOULD stop processing the request, free its resources, and "Not send a response for the cancelled request". This SDK does the first two and always does the opposite of the third, on every transport: processResult in internal/jsonrpc2/conn.go writes the response with c.write(notDone{req.ctx}, response), and nothing at application level runs between a handler returning and that write, so no server built on the SDK can satisfy the clause.

notDone is there on purpose and this change leaves it alone. A response must still be written when a handler's context ended for some other reason. What the spec asks for is narrower: no response when the peer asked for this particular id to be cancelled. The jsonrpc2 layer could not express that, because Connection.Cancel has two callers that mean different things, canceller.Preempt, which has just read the peer's notification, and ServerSession.Close, which cancels in-flight subscriptions/listen handlers to unblock them so the connection can drain. So the change records the distinction rather than inferring it: Connection.CancelFromPeer marks the incoming request, processResult writes no response for a request so marked, and Cancel keeps its old meaning.

The mark is written and read under the connection's stateMu, in the same critical section that removes the request from incomingByID. A cancellation arriving once the response has been handed to the writer therefore finds nothing to mark, and a response already on its way is never retracted. That is the race the spec's Timing Considerations section describes, and it stays resolved exactly the way it is today.

Suppressing the write on its own would have traded a spec deviation for a hang. The streamable HTTP transport keeps a POST's stream open until every call it carried has been answered, so a response that never arrives leaves that HTTP request open until the client goes away. A Writer that holds per-call state can now implement jsonrpc2.ResponseDropper and be told the response is not coming; streamableServerConn implements it by retiring the request through the same accounting a real response goes through, so the stream completes exactly as it would have. Writers with no such state, stdio and the in-memory transport among them, implement nothing and are unaffected.

Tests

TestStreamableCancelledCallGetsNoResponse (mcp) drives the whole path over raw HTTP: initialize, a tools/call whose tool parks on ctx.Done, notifications/cancelled for that id on a second request, then a read of the call's stream to EOF. It asserts that the stream carries nothing and that it ends. On main it fails with the response the server sent. It fakes the client with raw HTTP rather than using a ClientSession deliberately: an SDK client abandons the POST as soon as it cancels, so it never sees what the server wrote on that stream, and a stream that never completes looks to it exactly like one that did. I checked the other half of the test as well, by keeping the suppression and removing DropResponse: the same test then fails the other way, on the POST never returning.

TestCancelFromPeerSuppressesResponse (internal/jsonrpc2) pins the distinction itself. A second call acts as the barrier, since handlers run one at a time: a peer-cancelled call is answered only by the barrier's response and is reported to the dropper, while a locally cancelled one is still answered.

TestLoggingConnDropResponse (mcp) covers the one wrapper in the way. A connection asks its writer for ResponseDropper, and loggingConn is the only Connection wrapper in the package, so a LoggingTransport around a streamable server would have passed the type assertion straight through to nothing: the response suppressed, the POST's stream left open until the client went away. It now forwards to the delegate when the delegate implements the interface, and does nothing when it does not.

gofmt -l . is clean, go vet ./... is clean, go test ./... passes, and so does go test -race ./internal/jsonrpc2/ ./mcp/.

What an existing user sees change

  • A server no longer answers a call whose cancellation it received and acted on. A client that sends notifications/cancelled and then keeps awaiting that id will wait forever. No client in this repository does: both call and cancelCall retire the call locally when they send the notification, so the SDK's own client never waited for that response.
  • A subscriptions/listen the client cancels no longer produces the empty SubscriptionsListenResult, because a client cancels a listen by sending notifications/cancelled for the listen's request id. A listen ended by ServerSession.Close still produces it, since that path calls Cancel, not CancelFromPeer. This one is a judgement call: I read the spec as meaning that a cancelled request gets no response at all, completion result included, but the listen handler could be exempted if you read it the other way.
  • In JSON response mode a POST whose only call was cancelled now ends 200 with an empty body instead of one carrying the cancelled call's error. The Content-Type is still application/json, which is worth saying out loud: the response claims a JSON body and carries none. Answering 202 Accepted with no content type would read better to a strict client, and I will change it to that if you prefer. This path has no test of its own, unlike the SSE one; say the word and I will add it.
  • No exported API is added. CancelFromPeer and ResponseDropper are both in internal/jsonrpc2.

Other judgement calls

  • DropResponse takes no context and returns no error. There is nothing to send, and a failure could only ever mean "the stream is already gone", which is the state the call is trying to reach anyway.
  • The malformed-result branch of processResult is reordered so that nothing is lost: a handler that returns a result which fails to marshal is still reported through internalErrorf exactly as before, whether or not the call was cancelled. Only the write is skipped.
  • I did not put this behind a flag. mcpgodebug already gates two behaviour changes of roughly this shape (blockingcancelnotify, nomethodnotfoundcodeinerror), so if you would rather the old behaviour stayed reachable while callers adjust, say so and I will add one.
  • The docs/protocol.md cancellation section gains two sentences. I edited internal/docs/protocol.src.md and the generated file together rather than running weave, since the generator fetches golang.org/x/example; the text is plain prose with no directives, so regenerating reproduces it.

Fixes #1259.

@jmrplens

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (55350a9). It was conflicting against internal/jsonrpc2/conn.go and mcp/transport.go, both of which #1255 touched when it merged, so the conflict was between this change and my own.

Two of the three resolutions are worth stating, because they are decisions rather than mechanics.

CancelFromPeer now takes a cause. #1255 added Connection.CancelCause, which records why a request's context was cancelled so the handler can read it back through context.Cause, and this PR added CancelFromPeer, which suppresses the response. Both were variants of Cancel and the merged body wanted both values, so all three now delegate to one cancelIncoming(id, cause, fromPeer). Composing them this way rather than picking one is what the two changes are each about: a peer cancellation is precisely the kind that arrives with a stated reason, so canceller.Preempt both logs the reason and passes it as the cause, and the request is still answered with no response.

TestCancellationReason needed a change, and the reason is this PR's behaviour rather than the rebase. That test sends notifications/cancelled by hand and never retired the outgoing call, which was harmless while the receiver still answered a cancelled request. With no response it waits forever and synctest reports the bubble as deadlocked. The SDK's own cancel path already retires the call before it notifies (cancelCall in mcp/transport.go), precisely because a peer that cancels is not waiting for a result, so the test now does the same thing explicitly. That it surfaced here is the interaction working as intended: the test encoded an assumption this change is removing.

go build ./..., go vet ./... and go test ./... are clean on the rebased tree.

Comment thread mcp/streamable.go Outdated
Comment on lines +1119 to +1122
if data != nil {
s.pendingJSONMessages = append(s.pendingJSONMessages, data)
}
if done && len(s.pendingJSONMessages) > 0 {

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.

i think in case of empty data, so peer cancelled request, it should return 204 as code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and done in 1faac54, rebased onto main.

A stream that ends with nothing written on it now answers the POST with 204 No Content and no Content-Type: in JSON mode there is nothing to flush, and in SSE mode the header is likewise still unsent when no event was ever written (lastIdx < 0), so both get the 204 rather than an empty 200 under a Content-Type; a stream that already carried something ends as before. TestStreamableCancelledCallGetsNoResponse runs in both modes and asserts the status, the missing Content-Type and the empty body; the protocol doc says so too.

The SDK client itself never reads it, since its POST is cancelled together with the call. A client that keeps the POST open while cancelling from another request reads a 204 with no body, which is what "no response" looks like on the wire.

@jmrplens
jmrplens force-pushed the jmrp-no-response-for-a-cancelled-request branch from 55350a9 to 1faac54 Compare September 20, 2026 10:07
The MCP specification says a receiver of a cancellation notification
SHOULD stop processing the request, free its resources and "not send a
response for the cancelled request". The SDK always sends one.
`processResult` in `internal/jsonrpc2/conn.go` writes the response with
`c.write(notDone{req.ctx}, response)`, and `notDone` strips the
cancellation from the context so the write goes ahead. Nothing at
application level runs between a handler returning and that write, so no
server built on this SDK could satisfy the clause. Captured on stdio: a
client's `notifications/cancelled` at 6.306s, and the server's response
to that same request id at 6.307s.

`notDone` is there on purpose and this does not change it. A response
must still be written when the handler's context ended for some other
reason, and until now the jsonrpc2 layer could not tell the two apart:
`Connection.Cancel` is called both by the preempter that saw the peer's
notification and by `ServerSession.Close`, which cancels in-flight
`subscriptions/listen` handlers to unblock them. So the distinction is
recorded rather than guessed. `CancelFromPeer` marks the incoming
request as cancelled by the peer, and `processResult` writes no response
for such a call. `Cancel` keeps its meaning, and the listen result a
session close produces is unaffected. The flag is written and read under
the connection's `stateMu`, in the same critical section that removes
the request from `incomingByID`, so a cancellation arriving once the
response is already on its way finds nothing to mark, and no response is
ever retracted.

Suppressing the write alone would have traded a spec deviation for a
hang. The streamable HTTP transport keeps a POST's stream open until
every call it carried has been answered, so a response that never comes
leaves that request open until the client goes away. A writer that holds
per-call state can now implement `jsonrpc2.ResponseDropper` and be told
the response is not coming; `streamableServerConn` implements it by
retiring the request through the same accounting a real response goes
through, so the stream completes exactly as it would have. Writers with
no such state, stdio and the in-memory transport among them, implement
nothing and are unaffected. A stream that ends with nothing written on
it, because every call it carried was cancelled, answers the POST with
204 No Content: in JSON response mode there is nothing to flush, and in
SSE mode no event was written, so in both the header is still unsent
and an empty 200 under a Content-Type would claim a body that is not
there. A stream that already carried an event or a buffered message
ends as before.

`TestStreamableCancelledCallGetsNoResponse` in `mcp` drives the whole
path over raw HTTP, in SSE and in JSON response mode: initialize, a
`tools/call` whose tool parks on `ctx.Done`, `notifications/cancelled`
for that id on a second request, then a read of the call's stream to
EOF. It asserts that the POST is answered 204 with no Content-Type and
no body, and that it ends. On unmodified main it fails with the
response the server sent:

    event: message
    data: {"jsonrpc":"2.0","id":2,"error":{"code":0,"message":"context canceled"}}

It fakes the client with raw HTTP rather than using a `ClientSession`
deliberately: an SDK client abandons the POST as soon as it cancels, so
it never sees what the server wrote on that stream, and a stream that
never completes looks to it exactly like one that did. With the
suppression in place but `DropResponse` removed, the same test fails the
other way, on the POST never returning.

`TestCancelFromPeerSuppressesResponse` in `internal/jsonrpc2` pins the
distinction itself. A second call acts as the barrier, since handlers
run one at a time: a peer-cancelled call is answered only by the
barrier's response and is reported to the dropper, while a locally
cancelled one is still answered.

Verification on go1.27.1: `gofmt -l .` clean, `go vet ./...` clean,
`go test ./...` ok, `go test -race ./internal/jsonrpc2/ ./mcp/` ok,
staticcheck clean.
@jmrplens
jmrplens force-pushed the jmrp-no-response-for-a-cancelled-request branch from 1faac54 to 9402582 Compare September 24, 2026 20:35
@jmrplens

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (e07f0c9). It conflicted with #1232 in deliverLocked, which now calls markWrittenLocked after writing an SSE event. I kept both, and the 204 branch now tests s.lastWrite.IsZero() instead of s.lastIdx < 0. #1232 documents that field as meaning nothing has been written to the current w, so its headers are still uncommitted, which is exactly what a 204 needs. The two conditions agree today, because the keep-alive waits for the first event before it writes, but the field is the one that says what this branch depends on.

jmrplens added a commit to jmrplens/gitlab-mcp-server that referenced this pull request Sep 25, 2026
The top of the stack: what the whole-stack review still held open after
two fix rounds, one CI defect the stack itself ran into, the register
brought up to date, and every generated artifact the layers below moved,
regenerated once here.

**The review's last two findings.**

- The test that holds the individual tool Descriptions assembled at run
time (the switches in deploykeys and deploytokens, the map in pages)
checked only the tool names they spell. The rule `cmd/audit_action_ids`
applies to a constant Description also refuses a dotted ID that resolves
nowhere or only as an alias, so a misspelt ID in one of those switches
passed every gate; the review reproduced it by changing
`access.deploy_key_list_project` to `list_projekt`. The test now offers
a dotted token as an ID on the same condition as the rule, one of its
halves being one a canonical ID uses, leaves a `.git` tail alone, and
reports an alias with the canonical ID it stands for. The planted
misspelling fails it on both catalog classes. The limits paragraph in
the cmd utilities page said "those are held by a test" of two holes when
the test holds one, and says which now.
- The comment on the temporary-file refusal of a download said the file
it names is `output_path` unless the path was relative or its leaf a
link. `canonicalDownloadOutputPath` resolves every existing component
through symlinks, so a linked directory above the leaf renames it too,
the macOS temp root among them. The comment and a test row say so.

**Lint from the tree, not from a restored analysis.** The golangci-lint
job restored `~/.cache/golangci-lint` with a prefix key, and a pull
request reads its own ref's cache first. After a rebase that cache holds
the analysis of the branch's previous tree, and on 2026-09-24 it failed
a cascaded layer of this stack twice on an unused
`//nolint:contextcheck` in a file the stack never touched, until that
ref's caches were deleted. A reused result can hide a finding as easily
as it invents one. The analysis cache is no longer restored, so a lint
result depends on the tree alone; setup-go's build and module caches
stay, since their keys are content hashes. On a 4-CPU host a cold
analysis of main took 101 s against 29 s warm on the identical tree; in
CI the restored cache never matched the tree anyway, and over twenty
runs on 2026-09-24 the make step took 110 to 300 s while the
cross-platform matrix that sets the wall clock took 800 to 1150 s. The
static analysis page says the cache is not kept, and why.

**The register.** `modelcontextprotocol/go-sdk#1232` merged on
2026-09-21, so the write-deadline fix issue 1262 asked for was opened as
[modelcontextprotocol/go-sdk#1293](modelcontextprotocol/go-sdk#1293);
row 8, its section and the re-verification paragraph name it. Row 12's
section says what `modelcontextprotocol/go-sdk#1267` answers a POST
whose every call was cancelled (204, since the 20th) and that it was
rebased after `modelcontextprotocol/go-sdk#1232` conflicted in
`deliverLocked`. The client-go paragraph named two gitlab.com reviewers
with pronouns nobody here was told, and names them by handle and role
instead.

**Regenerated.** The tool snapshots (the served-prose rewrite of issue
910: tool names outside a See also clause replaced by canonical IDs),
the token footprint, the llms files, the testing reference and the
README stats. Over this tree every `check-*` target the
generated-artifacts job runs passes, as do the whole unit suite with
`-shuffle=on` and snapshot parity checked, and golangci-lint over every
build tag.

**Recorded.** The request inventory, which CI records during the
coverage job and found 47 rows short: the layers below added tests that
drive group access tokens, SAML group links, group security settings and
the other refusals the permission hints describe. Recorded again over
this tree it holds 1604 rows over 956 distinct paths in 177 packages,
every one of the 1082 catalog actions owned by a package that issued a
request, and R-PATH passes over it. And the e2e coverage, from both
Docker runs against this tree on truenas: the CE suite ran 1024 tests
with 9 skipped and none failed, the licensed EE suite 1282 with 8
skipped and none failed, and action coverage holds where it stood (CE L1
821 of 869, L2 721, L3 705; EE L1 1016 of 1089, L2 917, L3 901).

Closes #945.

This branch has not been deployed

No deployments
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.

A cancelled request is still answered

2 participants