diff --git a/.changeset/request-aborted-error-code.md b/.changeset/request-aborted-error-code.md new file mode 100644 index 0000000000..45710f140a --- /dev/null +++ b/.changeset/request-aborted-error-code.md @@ -0,0 +1,23 @@ +--- +'@modelcontextprotocol/core-internal': minor +'@modelcontextprotocol/client': minor +'@modelcontextprotocol/server': minor +--- + +Cancelling a request through `RequestOptions.signal` now rejects with +`SdkErrorCode.RequestAborted` instead of `SdkErrorCode.RequestTimeout`, so a +deliberate cancellation is distinguishable from a timeout expiry. Callers that +gated retry/backoff on `RequestTimeout` were also firing it on every user +cancellation. + +`RequestAborted` is a new `SdkErrorCode` member. Only an elapsed +`RequestOptions.timeout` still carries `RequestTimeout` — the timeout handler +constructs a typed `SdkError(RequestTimeout, 'Request timed out')` that passes +through the abort wrap untouched. An abort `reason` that is already an +`SdkError` continues to be rethrown verbatim with its own code. + +All four wrap sites move together, so the code does not depend on where the +abort lands: `Protocol.request()` (in-flight abort and pre-aborted signal), +the client's warm-cache pre-abort guard, and `Client.listen()`'s pre-abort +guard. A cache hit and a wire request now report the same code for the same +abort. diff --git a/docs/migration/upgrade-to-v2.md b/docs/migration/upgrade-to-v2.md index 19f4127733..87f188b7dc 100644 --- a/docs/migration/upgrade-to-v2.md +++ b/docs/migration/upgrade-to-v2.md @@ -986,6 +986,7 @@ the third argument — `new SdkHttpError(SdkErrorCode.ClientHttpNotImplemented, | `NotInitialized` | Protocol is not initialized | | `CapabilityNotSupported` | Required capability is not supported | | `RequestTimeout` | Request timed out waiting for response | +| `RequestAborted` | Request was cancelled through `RequestOptions.signal` | | `ConnectionClosed` | Connection was closed | | `SendFailed` | Failed to send message | | `InvalidResult` | Response result failed local schema validation | @@ -1530,8 +1531,12 @@ rewrite required unless noted. fires (v1 let them run to completion). `InMemoryTransport.close()` no longer double-fires `onclose` on the initiating side. - **`Protocol.request()` with an already-aborted signal** rejects with - `SdkError(SdkErrorCode.RequestTimeout, reason)` instead of throwing the raw - `signal.reason`, matching the in-flight-abort path. + `SdkError(SdkErrorCode.RequestAborted, reason)` instead of throwing the raw + `signal.reason`, matching the in-flight-abort path. Cancellation through + `RequestOptions.signal` — pre-aborted or in-flight — carries `RequestAborted`, + not `RequestTimeout`; only an elapsed `RequestOptions.timeout` carries + `RequestTimeout`. An abort `reason` that is already an `SdkError` is still + rethrown verbatim with its own code. - **OAuth discovery (`discoverOAuthProtectedResourceMetadata` / `discoverOAuthMetadata`, transitively `auth()`) throws on fetch `TypeError`** (DNS failure, `ECONNREFUSED`, invalid URL) in Node and Cloudflare Workers instead of swallowing it as a CORS miss @@ -1551,7 +1556,7 @@ rewrite required unless noted. verbs on top of it — `callTool()` and the cacheable list verbs — perform async work first (header-mirroring scan, response-cache freshness, output-validator resolution), so an abort fired in the same tick can land before the frame is ever sent: the call - rejects with `SdkError(RequestTimeout, reason)` and **no `notifications/cancelled` is + rejects with `SdkError(RequestAborted, reason)` and **no `notifications/cancelled` is emitted** (nothing was in flight). v1 sent the frame synchronously from these verbs. Once the frame is on the wire, aborting still sends `notifications/cancelled` before rejecting. diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 0b386a63e8..8af4bea82a 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -1808,12 +1808,12 @@ export class Client extends Protocol { if (hit !== undefined) { // A pre-aborted caller signal must reject the same way it would on // the wire path (`Protocol.request()` wraps an already-aborted - // signal as `SdkError(RequestTimeout, reason)`); without this guard + // signal as `SdkError(RequestAborted, reason)`); without this guard // a cache hit would resolve successfully and silently swallow the // abort. if (options?.signal?.aborted) { const reason = options.signal.reason; - throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason)); + throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestAborted, String(reason)); } return hit.value as R; } @@ -1968,12 +1968,12 @@ export class Client extends Protocol { // Honor RequestOptions.signal exactly as request() does: an // already-aborted signal rejects synchronously before any setup, and - // the rejection is the same `SdkError(RequestTimeout, reason)` wrap + // the rejection is the same `SdkError(RequestAborted, reason)` wrap // request() / `_serveFromCache` apply (unless `reason` is already an // SdkError — preserved verbatim). if (options?.signal?.aborted) { const reason = options.signal.reason; - throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason)); + throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestAborted, String(reason)); } const requestAbort = new AbortController(); diff --git a/packages/client/test/client/listen.test.ts b/packages/client/test/client/listen.test.ts index 60f96be47a..1d9a5d1834 100644 --- a/packages/client/test/client/listen.test.ts +++ b/packages/client/test/client/listen.test.ts @@ -379,7 +379,7 @@ describe('Client.listen()', () => { await client.close(); }); - it('options.signal already aborted: listen() rejects with SdkError(RequestTimeout) before any setup (parity with request())', async () => { + it('options.signal already aborted: listen() rejects with SdkError(RequestAborted) before any setup (parity with request())', async () => { const { clientTx, written } = await scriptedModern(); const client = new Client({ name: 'c', version: '1' }, { versionNegotiation: { mode: 'auto' } }); await client.connect(clientTx); @@ -388,9 +388,9 @@ describe('Client.listen()', () => { ac.abort('user cancelled'); const error = await client.listen({ toolsListChanged: true }, { signal: ac.signal }).catch(e => e as SdkError); // Same wrap as `Protocol.request()` / `_serveFromCache`: a non-SdkError - // reason is wrapped as RequestTimeout; the reason text is preserved. + // reason is wrapped as RequestAborted; the reason text is preserved. expect(error).toBeInstanceOf(SdkError); - expect((error as SdkError).code).toBe(SdkErrorCode.RequestTimeout); + expect((error as SdkError).code).toBe(SdkErrorCode.RequestAborted); expect((error as SdkError).message).toContain('user cancelled'); // No subscriptions/listen reached the wire; no listen state registered. await flush(); diff --git a/packages/client/test/client/responseCache.test.ts b/packages/client/test/client/responseCache.test.ts index cec28ac6d8..2788408f05 100644 --- a/packages/client/test/client/responseCache.test.ts +++ b/packages/client/test/client/responseCache.test.ts @@ -1079,7 +1079,7 @@ describe('Client honours cacheHints (SEP-2549)', () => { expect(wireCount('resources/read')).toBe(3); }); - it('a pre-aborted signal on a warm-cache hit rejects with SdkError(RequestTimeout) — the abort is not swallowed by the cache serve', async () => { + it('a pre-aborted signal on a warm-cache hit rejects with SdkError(RequestAborted) — the abort is not swallowed by the cache serve', async () => { const store = new InMemoryResponseCacheStore(); const { clientTx, listCount } = await scriptedModernServer([[TOOL_A]], { listHint: { ttlMs: 60_000, cacheScope: 'public' } }); const client = modernClient(store); @@ -1094,7 +1094,7 @@ describe('Client honours cacheHints (SEP-2549)', () => { ac.abort('user cancelled'); const error = await client.listTools(undefined, { signal: ac.signal }).catch(e => e as SdkError); expect(error).toBeInstanceOf(SdkError); - expect((error as SdkError).code).toBe(SdkErrorCode.RequestTimeout); + expect((error as SdkError).code).toBe(SdkErrorCode.RequestAborted); expect((error as SdkError).message).toContain('user cancelled'); // The aborted call did not reach the wire. expect(listCount()).toBe(1); diff --git a/packages/core-internal/src/errors/sdkErrors.ts b/packages/core-internal/src/errors/sdkErrors.ts index 0bc8f9a1ad..f90815fccf 100644 --- a/packages/core-internal/src/errors/sdkErrors.ts +++ b/packages/core-internal/src/errors/sdkErrors.ts @@ -24,6 +24,14 @@ export enum SdkErrorCode { // Transport errors /** Request timed out waiting for response */ RequestTimeout = 'REQUEST_TIMEOUT', + /** + * Request was cancelled through `RequestOptions.signal`. Distinct from + * {@linkcode SdkErrorCode.RequestTimeout} so callers can tell a deliberate + * cancellation apart from a timeout expiry. The abort reason is carried in + * the message; an abort reason that is already an `SdkError` is rethrown + * verbatim and keeps its own code. + */ + RequestAborted = 'REQUEST_ABORTED', /** Connection was closed */ ConnectionClosed = 'CONNECTION_CLOSED', /** Failed to send message */ diff --git a/packages/core-internal/src/shared/protocol.ts b/packages/core-internal/src/shared/protocol.ts index 0a19770082..589e989862 100644 --- a/packages/core-internal/src/shared/protocol.ts +++ b/packages/core-internal/src/shared/protocol.ts @@ -1394,14 +1394,14 @@ export abstract class Protocol { } // An already-aborted caller signal must surface the same way an - // in-flight abort does (`SdkError(RequestTimeout, reason)` via + // in-flight abort does (`SdkError(RequestAborted, reason)` via // `cancel()` below). Bare `throwIfAborted()` would propagate the // raw `signal.reason` instead, so callers that introduce an async // hop before `request()` (e.g. a cache freshness check) would see // a different rejection type depending on where the abort lands. if (options?.signal?.aborted) { const reason = options.signal.reason; - throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason)); + throw reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestAborted, String(reason)); } // Spec basic/patterns/cancellation §Transport-Specific (2026-07-28): @@ -1473,8 +1473,12 @@ export abstract class Protocol { requestAbort.abort(); } - // Wrap the reason in an SdkError if it isn't already - const error = reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestTimeout, String(reason)); + // Wrap the reason in an SdkError if it isn't already. Only the + // caller-signal path reaches this wrap with a non-SdkError + // reason — the timeout handler below passes a typed + // `SdkError(RequestTimeout, …)` straight through — so + // `RequestAborted` is the correct code here. + const error = reason instanceof SdkError ? reason : new SdkError(SdkErrorCode.RequestAborted, String(reason)); reject(error); }; diff --git a/packages/core-internal/test/shared/abortReasonPassthrough.test.ts b/packages/core-internal/test/shared/abortReasonPassthrough.test.ts index 3277edef27..6bc402015e 100644 --- a/packages/core-internal/test/shared/abortReasonPassthrough.test.ts +++ b/packages/core-internal/test/shared/abortReasonPassthrough.test.ts @@ -50,7 +50,7 @@ describe('request() abort-reason passthrough', () => { await expect(protocol.request({ method: 'ping' }, { signal: controller.signal })).rejects.toBe(reason); }); - it('wraps a non-SdkError abort reason in SdkError(RequestTimeout)', async () => { + it('wraps a non-SdkError abort reason in SdkError(RequestAborted)', async () => { const protocol = await connectedProtocol(); const controller = new AbortController(); controller.abort(new Error('plain')); @@ -62,6 +62,55 @@ describe('request() abort-reason passthrough', () => { (e: unknown) => e ); expect(rejection).toBeInstanceOf(SdkError); + expect((rejection as SdkError).code).toBe(SdkErrorCode.RequestAborted); + }); + + it('wraps an in-flight abort in SdkError(RequestAborted), not RequestTimeout', async () => { + const protocol = await connectedProtocol(); + const controller = new AbortController(); + // Timeout is three orders of magnitude away from the abort, so a + // RequestTimeout here could only come from the abort path. + const pending = protocol.request({ method: 'ping' }, { signal: controller.signal, timeout: 60_000 }).then( + () => { + throw new Error('request unexpectedly resolved'); + }, + (e: unknown) => e + ); + controller.abort(new DOMException('User cancelled', 'AbortError')); + + const rejection = await pending; + expect(rejection).toBeInstanceOf(SdkError); + expect((rejection as SdkError).code).toBe(SdkErrorCode.RequestAborted); + expect((rejection as SdkError).code).not.toBe(SdkErrorCode.RequestTimeout); + expect((rejection as SdkError).message).toContain('User cancelled'); + }); + + it('wraps a bare in-flight abort() with no reason in SdkError(RequestAborted)', async () => { + const protocol = await connectedProtocol(); + const controller = new AbortController(); + const pending = protocol.request({ method: 'ping' }, { signal: controller.signal, timeout: 60_000 }).then( + () => { + throw new Error('request unexpectedly resolved'); + }, + (e: unknown) => e + ); + controller.abort(); + + const rejection = await pending; + expect(rejection).toBeInstanceOf(SdkError); + expect((rejection as SdkError).code).toBe(SdkErrorCode.RequestAborted); + }); + + it('leaves the timeout path on RequestTimeout', async () => { + const protocol = await connectedProtocol(); + + const rejection = await protocol.request({ method: 'ping' }, { timeout: 0 }).then( + () => { + throw new Error('request unexpectedly resolved'); + }, + (e: unknown) => e + ); + expect(rejection).toBeInstanceOf(SdkError); expect((rejection as SdkError).code).toBe(SdkErrorCode.RequestTimeout); }); }); diff --git a/packages/core-internal/test/types/errorSurfacePins.test.ts b/packages/core-internal/test/types/errorSurfacePins.test.ts index cc01cf4c57..c45b62e535 100644 --- a/packages/core-internal/test/types/errorSurfacePins.test.ts +++ b/packages/core-internal/test/types/errorSurfacePins.test.ts @@ -73,6 +73,7 @@ describe('SdkErrorCode', () => { NotInitialized: 'NOT_INITIALIZED', CapabilityNotSupported: 'CAPABILITY_NOT_SUPPORTED', RequestTimeout: 'REQUEST_TIMEOUT', + RequestAborted: 'REQUEST_ABORTED', ConnectionClosed: 'CONNECTION_CLOSED', SendFailed: 'SEND_FAILED', InvalidResult: 'INVALID_RESULT',