fix(core): never send notifications/cancelled for the initialize hand… (#2668)
Co-authored-by: Felix Weinberger <fweinberger@anthropic.com> Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com>
This commit is contained in:
parent
75dc7ea6e2
commit
3e90449fd5
6 changed files with 109 additions and 33 deletions
9
.changeset/no-cancel-notification-for-initialize.md
Normal file
9
.changeset/no-cancel-notification-for-initialize.md
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
---
|
||||
'@modelcontextprotocol/core-internal': patch
|
||||
'@modelcontextprotocol/client': patch
|
||||
'@modelcontextprotocol/server': patch
|
||||
---
|
||||
|
||||
Stop sending `notifications/cancelled` for the `initialize` handshake. The spec is explicit that a client MUST NOT attempt to cancel its `initialize` request, but the outbound cancel path fired for any in-flight request: aborting the `AbortSignal` passed to `connect()`, or letting the handshake hit its timeout, put a forbidden cancellation on the wire naming the initialize request id.
|
||||
|
||||
The local behaviour is unchanged — the caller's promise still rejects with the same abort/timeout error, and `connect()` still tears the connection down. Only the wire notification is suppressed. Every other method keeps the existing cancellation path.
|
||||
|
|
@ -236,9 +236,11 @@ coverage, spawn `serveStdio` as a child process.
|
|||
On a 2026-07-28 Streamable HTTP connection, aborting an in-flight client request
|
||||
(`signal` / timeout) closes that request's SSE response stream — the spec cancellation
|
||||
signal — instead of POSTing `notifications/cancelled`. Nothing to change in calling
|
||||
code. 2025-era connections and stdio at any era still send `notifications/cancelled`.
|
||||
Custom `Transport` implementations that open one underlying request per outbound message
|
||||
and honor `TransportSendOptions.requestSignal` may opt in by declaring
|
||||
code. 2025-era connections and stdio at any era still send `notifications/cancelled`
|
||||
(except for the `initialize` handshake, which the spec forbids cancelling — an aborted
|
||||
or timed-out `connect()` rejects locally and sends nothing). Custom `Transport`
|
||||
implementations that open one underlying request per outbound message and honor
|
||||
`TransportSendOptions.requestSignal` may opt in by declaring
|
||||
`readonly hasPerRequestStream = true`.
|
||||
|
||||
### `ctx.mcpReq.log()` and the per-request `logLevel`
|
||||
|
|
|
|||
|
|
@ -1506,7 +1506,11 @@ rewrite required unless noted.
|
|||
on those survive verbatim. The cancelled-on-timeout signal is unchanged on legacy-era
|
||||
connections and on stdio/in-memory at any era; on 2026-era Streamable HTTP the cancel
|
||||
signal is the per-request stream close instead of a `notifications/cancelled` POST
|
||||
(see [support-2026-07-28.md](./support-2026-07-28.md)).
|
||||
(see [support-2026-07-28.md](./support-2026-07-28.md)). The one exemption is the
|
||||
`initialize` handshake: an aborted or timed-out `connect()` still rejects locally, but
|
||||
no `notifications/cancelled` goes on the wire — the spec forbids cancelling
|
||||
`initialize`, and v1 sent one anyway. v1 tests asserting that notification need
|
||||
re-baselining.
|
||||
- **Also unchanged: SSE reconnection exhaustion.** `StreamableHTTPClientTransport`'s
|
||||
standalone GET-stream reconnection behavior and its exhaustion signal carry over from
|
||||
v1: when retries run out, the transport emits `onerror` with a plain `Error` whose
|
||||
|
|
|
|||
|
|
@ -1453,19 +1453,29 @@ export abstract class Protocol<ContextT extends BaseContext> {
|
|||
this._progressHandlers.delete(messageId);
|
||||
|
||||
if (requestAbort === undefined) {
|
||||
this._transport
|
||||
?.send(
|
||||
this._envelopeOutbound({
|
||||
jsonrpc: '2.0',
|
||||
method: 'notifications/cancelled',
|
||||
params: {
|
||||
requestId: messageId,
|
||||
reason: String(reason)
|
||||
}
|
||||
}),
|
||||
{ relatedRequestId, resumptionToken, onresumptiontoken }
|
||||
)
|
||||
.catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`)));
|
||||
// "A client MUST NOT attempt to cancel its `initialize`
|
||||
// request" (spec basic/lifecycle, mirrored on
|
||||
// `CancelledNotification`). The handshake is the one request
|
||||
// whose cancellation is forbidden outright, so an abort or
|
||||
// timeout on it settles purely locally: the promise still
|
||||
// rejects below, but nothing goes on the wire. Only the
|
||||
// legacy era can reach this — `initialize` is absent from the
|
||||
// modern registry, which negotiates via `server/discover`.
|
||||
if (request.method !== 'initialize') {
|
||||
this._transport
|
||||
?.send(
|
||||
this._envelopeOutbound({
|
||||
jsonrpc: '2.0',
|
||||
method: 'notifications/cancelled',
|
||||
params: {
|
||||
requestId: messageId,
|
||||
reason: String(reason)
|
||||
}
|
||||
}),
|
||||
{ relatedRequestId, resumptionToken, onresumptiontoken }
|
||||
)
|
||||
.catch(error => this._onerror(new Error(`Failed to send cancellation: ${error}`)));
|
||||
}
|
||||
} else {
|
||||
// Modern-era per-request-stream transport: aborting the
|
||||
// request's underlying stream IS the spec cancel signal.
|
||||
|
|
|
|||
|
|
@ -866,6 +866,23 @@ describe('protocol tests', () => {
|
|||
const cancelledSent = (sent: JSONRPCMessage[]): JSONRPCMessage[] =>
|
||||
sent.filter(m => 'method' in m && m.method === 'notifications/cancelled');
|
||||
|
||||
/**
|
||||
* Connects a fresh protocol over a single-channel transport (stdio /
|
||||
* in-memory shape: no `hasPerRequestStream`) at `version`, recording
|
||||
* every outbound message.
|
||||
*/
|
||||
const connectSingleChannel = async (version: string) => {
|
||||
const sent: JSONRPCMessage[] = [];
|
||||
const tx = new MockTransport();
|
||||
tx.send = async (m: JSONRPCMessage) => {
|
||||
sent.push(m);
|
||||
};
|
||||
const proto = createTestProtocol();
|
||||
await proto.connect(tx);
|
||||
setNegotiatedProtocolVersion(proto, version);
|
||||
return { proto, sent };
|
||||
};
|
||||
|
||||
test('modern era + per-request-stream transport: abort closes the stream, NO notifications/cancelled', async () => {
|
||||
const tx = new PerRequestStreamTransport();
|
||||
const proto = createTestProtocol();
|
||||
|
|
@ -888,15 +905,7 @@ describe('protocol tests', () => {
|
|||
});
|
||||
|
||||
test('modern era + single-channel transport (no hasPerRequestStream): POSTs notifications/cancelled', async () => {
|
||||
// stdio / in-memory shape: hasPerRequestStream is undefined.
|
||||
const sent: JSONRPCMessage[] = [];
|
||||
const tx = new MockTransport();
|
||||
tx.send = async (m: JSONRPCMessage, _opts?: TransportSendOptions) => {
|
||||
sent.push(m);
|
||||
};
|
||||
const proto = createTestProtocol();
|
||||
await proto.connect(tx);
|
||||
setNegotiatedProtocolVersion(proto, '2026-07-28');
|
||||
const { proto, sent } = await connectSingleChannel('2026-07-28');
|
||||
|
||||
const ac = new AbortController();
|
||||
const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { signal: ac.signal });
|
||||
|
|
@ -937,6 +946,53 @@ describe('protocol tests', () => {
|
|||
expect(tx.lastRequestSignal?.aborted).toBe(true);
|
||||
expect(cancelledSent(tx.sent)).toHaveLength(0);
|
||||
});
|
||||
|
||||
// "A client MUST NOT attempt to cancel its `initialize` request." The
|
||||
// handshake is exempt from the POST path above on every transport: an
|
||||
// abort or timeout rejects the caller locally and sends nothing. Both
|
||||
// triggers are covered because they reach cancel() by different routes
|
||||
// (the caller's signal vs the timeout handler).
|
||||
describe('the initialize handshake is never cancelled on the wire', () => {
|
||||
test('aborting an in-flight initialize sends NO notifications/cancelled', async () => {
|
||||
// ARRANGE
|
||||
const { proto, sent } = await connectSingleChannel('2025-11-25');
|
||||
|
||||
// ACT
|
||||
const ac = new AbortController();
|
||||
const pending = testRequest(proto, { method: 'initialize', params: {} }, z.object({}), { signal: ac.signal });
|
||||
ac.abort('user cancel');
|
||||
|
||||
// ASSERT — rejects locally, wire stays clean
|
||||
await expect(pending).rejects.toThrow();
|
||||
expect(cancelledSent(sent)).toHaveLength(0);
|
||||
});
|
||||
|
||||
test('timing out an in-flight initialize sends NO notifications/cancelled', async () => {
|
||||
// ARRANGE
|
||||
const { proto, sent } = await connectSingleChannel('2025-11-25');
|
||||
|
||||
// ACT
|
||||
const pending = testRequest(proto, { method: 'initialize', params: {} }, z.object({}), { timeout: 0 });
|
||||
|
||||
// ASSERT
|
||||
await expect(pending).rejects.toThrow();
|
||||
expect(cancelledSent(sent)).toHaveLength(0);
|
||||
});
|
||||
|
||||
test('every other method still POSTs notifications/cancelled (regression guard)', async () => {
|
||||
// ARRANGE
|
||||
const { proto, sent } = await connectSingleChannel('2025-11-25');
|
||||
|
||||
// ACT
|
||||
const ac = new AbortController();
|
||||
const pending = testRequest(proto, { method: 'example', params: {} }, z.object({}), { signal: ac.signal });
|
||||
ac.abort('user cancel');
|
||||
|
||||
// ASSERT
|
||||
await expect(pending).rejects.toThrow();
|
||||
expect(cancelledSent(sent)).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -140,15 +140,10 @@ export const REQUIREMENTS: Record<string, Requirement> = {
|
|||
note: 'Stateless hosting creates a fresh server per request and has no standalone GET stream, so there is no server→client channel to deliver/observe these.'
|
||||
},
|
||||
'protocol:cancel:initialize-not-cancellable': {
|
||||
transports: STATEFUL_TRANSPORTS,
|
||||
transports: ['inMemory'],
|
||||
source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/cancellation#behavior-requirements',
|
||||
behavior: 'The client never sends notifications/cancelled for the initialize request.',
|
||||
note: 'Stateless hosting creates a fresh server per request and has no standalone GET stream, so there is no server→client channel to deliver/observe these.',
|
||||
knownFailures: [
|
||||
{
|
||||
note: 'SDK sends notifications/cancelled for initialize when connect() is aborted; spec says initialize MUST NOT be cancelled.'
|
||||
}
|
||||
]
|
||||
note: "The behavior itself is transport-agnostic (shared/protocol.ts), but the test must tap the client's outbound messages before connect() resolves, which only the in-memory wiring supports."
|
||||
},
|
||||
'protocol:cancel:late-response-ignored': {
|
||||
source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/utilities/cancellation#timing-considerations',
|
||||
|
|
|
|||
Loading…
Reference in a new issue