Stop answering cancelled requests - #3188
Conversation
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/streamable_http.py">
<violation number="1">
P2: A legacy cancel can be sent with the newer protocol header after another request updates the cache, so a 2026 endpoint will treat this client-to-server notification as invalid instead of terminating the 2025 request. Preserve the named post's negotiated version and use it when posting its legacy cancellation frame.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Beyond the two inline registry nits, I re-checked the concerns raised on the previous revision: the settle path no longer closes the request stream out-of-band — _terminate_unanswered_request writes the -32800 terminator through self._write_stream.send(...), the same ordered channel as the request's other messages, so it cannot overtake queued frames; and the client transport's 2025 arm is back to if not post.modern: return False (identical to main), so an abandoned call's POST stays attached and the nested-cancel delivery path is intact.
Extended reasoning...
This revision (6d0b711) replaced the out-of-band stream-close architecture with an in-band JSONRPCError terminator routed through the transport's ordered write stream, which structurally eliminates the message-drop race flagged on the prior revision, and it reverted the unconditional client-side POST abort at 2025-era revisions. Both were confirmed by reading the code at HEAD rather than inferred from the commit message. The remaining findings are metadata-only nits in the requirements registry with zero runtime effect; the PR still touches core dispatcher and transport cancellation semantics and is a documented breaking change, so it warrants human review either way — this note only records what was already re-examined so a later pass need not start from scratch.
There was a problem hiding this comment.
No new findings on the latest revision (f1f87ab) — my earlier comments were all addressed (the in-band -32800 terminator replacing the client-side stream release in 6d0b711, and the registry/divergence corrections in f1f87ab), but this remains a core protocol-behavior change that should get a human pass before merge.
What was reviewed this run:
- The reworked
_handle_requestsettle logic (interrupt mode, signal mode, handler-failure-after-cancel, shutdown arm) against the new no-answer semantics. - The
on_request_unansweredhook plumbing and the legacy transport's_terminate_unanswered_requestwrite path. - One candidate race was examined and ruled out: a peer cancel interrupting the in-flight result write skipping the legacy-HTTP terminator and parking the POST — verifiers concluded it does not reproduce.
Extended reasoning...
Overview
This PR removes the v1 behavior of answering a peer-cancelled request with a synthesized {code: 0} error, so a cancelled request now settles with no response at all, per the 2026-07-28 transport rules. To keep the 2025-era streamable HTTP wire working (where a request's POST completes only on a response for its id), the dispatcher gains an on_request_unanswered hook on ServerMessageMetadata, and the legacy transport terminates a settled-unanswered request in-band with a valid -32800 REQUEST_CANCELLED error written through the ordered write channel. The diff spans the dispatcher's single exception-to-wire boundary, the legacy HTTP transport, message metadata, migration docs, and a substantial test rework.
Security risks
None identified. The change does not touch auth, crypto, or input validation; the new hook is server-internal metadata that peers cannot set, and the terminator is a fixed error frame. The main risk class here is liveness/correctness (parked POSTs, dropped frames, double answers), not security.
Level of scrutiny
High. This is core protocol machinery — the request/response settlement path every transport goes through — with subtle async ordering concerns, as demonstrated by the review history: two earlier revisions had real ordering/lifecycle issues (an out-of-band stream close racing the message router, and an era-inappropriate client-side POST abort) that the author fixed by rearchitecting to the in-band terminator. The current design is markedly simpler and the FIFO-ordering argument is sound, but a deliberate breaking change to cancellation semantics plus an intentional era-scoped spec divergence are exactly the kind of design decisions a human maintainer should sign off on. This is not shadow-approvable.
Other factors
The bug hunting system found no issues in this run over the final revision. One candidate — a peer cancel landing mid result-write skipping the legacy terminator and parking the POST — was adversarially examined and refuted. All four of my prior comments (two blocking, two nits) were addressed in 6d0b711 and f1f87ab, and the author's responses check out against the current diff: the client transport is back to byte-parity with main, _end_request_stream is gone in favor of the in-band terminator, the protocol:cancel:in-flight divergence is typed again, and the stale cancel-posts-frame wording was reverted. Test coverage is thorough (interrupt/signal modes, failing handlers, raising hooks, both HTTP response modes, id-form coercion), the suite reportedly passes at 100% coverage, and the migration guide documents the break. The author also flags a conformance-suite gap (no scenario for no-response-after-cancel), which per repo guidelines warrants a conformance-repo issue — a maintainer should confirm that's tracked.
When a peer sent notifications/cancelled for an in-flight request, the
dispatcher interrupted the handler and then answered the request anyway
with a synthesized JSON-RPC error, {"code": 0, "message": "Request
cancelled"} - a v1 carry-over pinned as a spec divergence. The
2026-07-28 transport rules say a cancelled request MUST NOT be answered,
and code 0 is not a valid JSON-RPC error code, so a cancelled request now
settles with no response at all: no result if the handler runs to
completion, no error if it fails. Both seats change (the server for
cancelled client requests, the client for cancelled server-initiated
ones).
The error frame had been doing a second job on the 2025-era streamable
HTTP wire, where each request's POST stays open until its response
arrives: it was what let the POST complete. So the dispatcher now tells
the transport when a request settles unanswered (an
on_request_unanswered hook on the inbound message metadata), and the
transport ends that request's stream on it - an empty 202 in
JSON-response mode, a cleanly-ended event stream in SSE mode - instead
of parking the POST until the session ends.
For the same reason the client transport now releases an abandoned
request's own POST at every protocol era, not just 2026-07-28: with no
response coming, a stream left open would sit parked, or - against an
event-store server - resume via Last-Event-ID for an answer that will
never exist. At 2025 the cancellation frame is still POSTed; only the
lingering stream goes away.
The built-in client's abandon and timeout paths never waited on that
error frame, so they are unaffected; a caller that hand-sends
notifications/cancelled while still awaiting a call now waits rather
than receiving the code-0 error, which docs/migration.md notes.
Review of the first revision showed that dropping the cancellation frame outright fought the 2025-era streamable HTTP wire, whose per-request SSE stream is the ordered, resumable channel for everything about that request and ends only when a response for its id passes through it. The out-of-band substitutes each broke something: the transport hook closed the stream from the side and could overtake messages already queued through the router (notably the courtesy cancel for a handler's nested elicitation), had no durable form so a resuming client's replay tailed forever, and needed two dispatcher call sites; the client aborting its own POST severed the delivery path for that owed related traffic and turned routine cancellation into a write against a closed socket. The 2026-07-28 MUST NOT that motivates this change never coexists with resumable per-request streams (SEP-2575 removed them), so the fix is scoped by era instead. The dispatcher stays strictly silent for a cancelled request everywhere. The one 2025-era transport terminates the settled request through the same ordered channel with a valid -32800 REQUEST_CANCELLED error (LSP's RequestCancelled) in place of the old code 0 - so ordering, resumption, and the client's channel are all preserved by construction, and old and new clients complete the same way they did before. The client transport is unchanged from main; the dispatcher settles a cancelled request at a single site, containing a raising transport hook like its other callback boundaries.
The client-transport revert left the cancel-posts-frame requirement describing a stream release the client no longer performs; restore its main-branch wording. Record the era-scoped -32800 answer on the legacy HTTP wire as typed Divergence data on the protocol requirement, as the manifest header requires for a test-pinned gap, and give the transport entry an sdk source since the response is an SDK choice rather than a spec mandate. Reword the streaming story's See-also line, which pointed at a cancellation error path that no longer exists. No-Verification-Needed: manifest and example-README text only, no runtime surface
f1f87ab to
5dc226b
Compare
There was a problem hiding this comment.
I re-reviewed the latest revision (through f1f87ab) and found no new issues; all findings from my earlier rounds are addressed. Deferring to a human because this changes core dispatcher response semantics and wire-level protocol behavior.
What was reviewed this run:
- The reworked
_handle_requestsettle logic: the cancel-check/write ordering, the shutdown arm, and thehandler_failurere-raise — no path double-answers or drops an owed answer. - The candidate issue that a peer cancel interrupting an in-flight result write parks the legacy HTTP POST — ruled out: a started write counts as answered, so the POST completes on the delivered response.
- The candidate that the
protocol:cancel:in-flighttest accepts the-32800terminator on every matrix arm — ruled out as a real defect: only the legacy HTTP arm ever produces it. - The f1f87ab registry fixes (typed
Divergencerestored,source="sdk",cancel-posts-framereverted to main's wording) — they match what the review comments asked for.
Extended reasoning...
Overview
The PR removes the v1 carry-over of answering a peer-cancelled request with a code-0 error. The dispatcher (src/mcp/shared/jsonrpc_dispatcher.py) now settles cancelled requests unanswered and signals the transport via a new on_request_unanswered callback on ServerMessageMetadata; the 2025-era streamable HTTP transport uses that hook to terminate the request's POST in-band with a valid -32800 REQUEST_CANCELLED error, written through the ordered write stream. Docs, the requirements registry, and a broad set of tests are updated to pin the new behavior.
Security risks
None identified. The change is protocol-semantics only: no auth, crypto, input parsing, or data-exposure surface is touched. The one adjacent hazard — a settle callback closing a successor request's stream on id reuse — was designed out in revision 2 by replacing the by-key stream close with an in-band error response routed by response id, the same path responses have always taken.
Level of scrutiny
High. This modifies the single exception-to-wire boundary in the dispatcher that every transport shares, changes observable wire behavior (a breaking change documented in docs/migration.md), and introduces a deliberate, era-scoped spec divergence on the 2025 streamable HTTP wire. The iteration history shows how subtle this area is: revision 1's out-of-band stream close had a real message-reordering race and a client-side regression at 2025, both caught in review and fixed by rearchitecting rather than patching. That is exactly the class of change a human maintainer should sign off on, regardless of how clean the final revision looks.
Other factors
The final revision is well-tested: the settle path has dedicated dispatcher unit tests (interrupt mode, signal mode, post-cancel handler failure, raising hook containment), a new transport test pins the -32800 terminator in both JSON and SSE response modes, and the interaction suite's cancellation tests were reworked to prove the negative (no response arrives) rather than asserting the old error. The author responded to all three review rounds with substantive fixes and the requirements registry now records the divergence in the typed, auditable field. The PR notes no conformance-suite scenario covers no-response-after-cancel yet — a spec/conformance follow-up a maintainer may want to track. My review found nothing blocking; the deferral reflects the criticality of the code path, not doubt about the current revision.
1 participant
When a peer sent
notifications/cancelledfor an in-flight request, the dispatcher interrupted the handler and then answered the request anyway with a synthesized error,{"code": 0, "message": "Request cancelled"}— a v1 carry-over pinned as a spec divergence. The 2026-07-28 transport rules say a cancelled request MUST NOT be answered (and0isn't a valid JSON-RPC code), so a cancelled request now settles with no response at all: no result if the handler runs to completion, no error if it fails. This matches typescript-sdk and csharp-sdk, which both suppress the write.Motivation and Context
Removing the frame exposed that it was doing a second job on the 2025-era streamable HTTP wire, where a request's POST stays open until its response arrives — the error frame was what let the POST complete, so simply suppressing it would park the POST until session end. The dispatcher now signals the transport when a request settles unanswered (
on_request_unansweredon the inbound message metadata), and the legacy transport ends the request's stream on it: empty202 Acceptedin JSON-response mode, a cleanly-ended event stream in SSE mode.For the same reason, the client transport now releases an abandoned request's own POST at every era (not just 2026-07-28): otherwise the stream sits parked, or — against an event-store server — resumes via
Last-Event-IDfor an answer that will never exist. At 2025 the cancellation frame is still POSTed; only the lingering stream goes.How Has This Been Tested?
Full suite (
./scripts/test, 100% coverage,strict-no-cover) plus end-to-end runs: a stdio server subprocess driven with hand-typed JSON-RPC (cancel → silence, session healthy; unknown-id / completed-request / double cancel all silent); a live uvicorn streamable-HTTP server in both response modes (cancelled call's POST completes at cancel time — 202 empty / clean SSE end — wheremainreturns the code-0 body); and an event-store server with client abandon-by-timeout (noLast-Event-IDreconnect).Breaking Changes
Callers of the built-in
Clientare unaffected — abandon and timeout never waited on the error frame. A caller that hand-sendsnotifications/cancelledwhile still awaiting the call now waits instead of receiving the code-0 error; seedocs/migration.md.Types of changes
Checklist
Additional context
No conformance-suite scenario covers "no response after cancel" yet; worth an issue there. Spec-internal note: the transport pages say MUST NOT while
basic/patterns/cancellationstill says SHOULD for the same behaviour.AI Disclaimer