diff --git a/AGENTS.md b/AGENTS.md index ea0497f2c4..1fc7f2d341 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -291,7 +291,25 @@ v2/main/ │ │ # (`InspectorClientOptions.appElicitation`), and │ │ # supplying one is what advertises the nested │ │ # `elicitation` setting — so web opts in and -│ │ # cli/tui, which cannot host an App, do not) +│ │ # cli/tui, which cannot host an App, do not; +│ │ # subscriptionAck.ts: recognizing the modern +│ │ # `subscriptions/listen` a server answers with a +│ │ # bare graceful-closure result instead of +│ │ # acknowledging (#2097). The SDK gives that close +│ │ # NO code of its own — it is the same +│ │ # `SdkError(ConnectionClosed)` any pre-ack close +│ │ # carries — so the predicate reads the message, +│ │ # deliberately, since the alternative is refusing +│ │ # to retry a genuinely transient drop. The live +│ │ # check on that string is the integration test, +│ │ # which drives a real never-acknowledging server +│ │ # (`subscriptions-never-acknowledged-http.json`); +│ │ # an SDK that rephrases it fails there rather than +│ │ # silently reverting to the eight-retry loop. The +│ │ # status is `"never-acknowledged"`, NOT `"ended"`: +│ │ # `ended` means an EXPECTED close, and reading a +│ │ # deterministic conformance failure as one is the +│ │ # silence #2063 reported) │ │ ├── import/ # Config import strategies (#1348): client-config parsers │ │ │ # (Claude Desktop/Cursor/Cline/VS Code), registry │ │ │ # server.json parser, strategy registry + well-known diff --git a/README.md b/README.md index 3fa4c6e436..ccf9d6002b 100644 --- a/README.md +++ b/README.md @@ -161,6 +161,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `oauth-custom-resource-metadata-http.json` **(legacy era)** | OAuth discovery driven by the challenge's `resource_metadata` | [#2071](https://github.com/modelcontextprotocol/inspector/issues/2071) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | +| `subscriptions-never-acknowledged-http.json` | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | | `tasks-{legacy,modern}-http.json` | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | #### MCP Apps @@ -457,6 +458,22 @@ The modern config deliberately **omits** `update_resource`. The SDK's modern leg So the live update-notification round-trip is demonstrated on the legacy (stateful-session) server, and the modern server is for the subscribe/listen/badge behavior. The Inspector's _receive_ path is era-transparent, so a real stateful modern server that routes `resources/updated` onto the listen stream drives the subscribed tile the same way. +#### A listen that is never acknowledged + +`subscriptions-never-acknowledged-http.json` serves the same three `numbered_resources` on the modern leg. It acknowledges the first `subscriptions/listen` **that subscribes to a resource** normally, and answers every resource-subscription listen after it with a bare JSON-RPC `result` instead of a `notifications/subscriptions/acknowledged`. A listen carrying only list-change opt-ins is always acknowledged — including the one the Inspector opens at connect time ([#1920](https://github.com/modelcontextprotocol/inspector/issues/1920)), which is why the counting is per *resource-subscription* listen: otherwise that connect-time listen spends the allowance before you have clicked anything and the very first Subscribe is refused. Connect with **Protocol Era = Modern**. + +That result is not a malformed message. On the 2026-07-28 era the listen request is long-lived, and the `result` for its id is reserved as the [graceful-closure](https://modelcontextprotocol.io/specification/2026-07-28/basic/patterns/subscriptions#graceful-closure) marker — so a server sending it up front is saying "acknowledged and closed in the same breath". It is deliberately bare, with no `resultType` discriminator, matching the payload from the original report rather than the spec's example. + +Open the Resources tab and **Subscribe** to `resource_1`: an ordinary acknowledged stream, badge **Listening**. Now **Subscribe** to `resource_2`. Changing the filter re-lists, this one is refused, and: + +- the subscribe fails with the reason spelled out — *"The server closed the subscription without acknowledging it… Not retrying"*; +- the Subscriptions badge turns an orange **Not acknowledged** and the panel carries the same sentence as a notice; +- the Protocol tab shows exactly **one** further `subscriptions/listen`. + +On the broken build that second click produced eight `subscriptions/listen` requests with increasing ids over roughly a minute, the badge flickering `Reconnecting…` between them, and a final bare **Stream ended** that said nothing about why ([#2097](https://github.com/modelcontextprotocol/inspector/issues/2097), split out of [#2063](https://github.com/modelcontextprotocol/inspector/issues/2063), where it read as the Inspector "accepting" an invalid response). The condition is deterministic — the server answers the same way every time — so retrying it is noise, not recovery. + +The first resource-subscription listen is acknowledged **so the badge is reachable at all**: it is gated on a live subscription, which a server refusing from the outset never lets you hold. That variant — refuse every listen, the literal shape in the report — is what the integration tests drive; it is the same code path, minus the badge. And `never-acknowledged` is a status of its own rather than **Stream ended** on purpose: `ended` covers the two *expected* closes (a server tearing an established stream down, and reconnection abandoned after repeated failures), and reading a deterministic conformance failure as either of them is the silence the issue is about. + #### Tasks, both eras **Legacy** (`tasks-legacy-http.json`) advertises `capabilities.tasks` (`tasks: { list, cancel }`) with the `simple_task` / `progress_task` / `elicitation_task` presets. Run one of those tools with **Run as task** on, and the **Tasks** tab lists it (populated via `tasks/list`), polls `tasks/get`, fetches the payload with the blocking `tasks/result`, and cancels with `tasks/cancel`. diff --git a/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.stories.tsx b/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.stories.tsx index 314870a115..42f8bf3526 100644 --- a/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.stories.tsx +++ b/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.stories.tsx @@ -24,3 +24,12 @@ export const Reconnecting: Story = { export const Ended: Story = { args: { status: "ended" }, }; + +/** + * The server answered `subscriptions/listen` with a bare JSON-RPC result — the + * graceful-closure marker — without ever acknowledging it (#2097). Distinct from + * `Ended` because the Inspector does not retry it. + */ +export const NeverAcknowledged: Story = { + args: { status: "never-acknowledged" }, +}; diff --git a/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.test.tsx b/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.test.tsx index 873549fd15..663d4eecb3 100644 --- a/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.test.tsx +++ b/clients/web/src/components/elements/SubscriptionStreamBadge/SubscriptionStreamBadge.test.tsx @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { SubscriptionStreamBadge } from "./SubscriptionStreamBadge"; import { subscriptionStreamPresentation } from "./subscriptionStreamUtils"; +import { NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE } from "@inspector/core/mcp/subscriptionAck.js"; describe("subscriptionStreamPresentation", () => { it("maps each status to a color and label", () => { @@ -21,6 +22,18 @@ describe("subscriptionStreamPresentation", () => { color: "gray", label: "Stream ended", }); + expect(subscriptionStreamPresentation("never-acknowledged")).toMatchObject({ + color: "orange", + label: "Not acknowledged", + }); + }); + + // The never-acknowledged tooltip must carry the *reason*, not just the generic + // stream explanation — that silence is what #2097 is about. + it("says why a never-acknowledged stream closed", () => { + expect( + subscriptionStreamPresentation("never-acknowledged").tooltip, + ).toContain(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE); }); it("explains the listen stream in every tooltip", () => { @@ -29,6 +42,7 @@ describe("subscriptionStreamPresentation", () => { "acknowledged", "reconnecting", "ended", + "never-acknowledged", ] as const) { expect(subscriptionStreamPresentation(status).tooltip).toContain( "subscriptions/listen stream", @@ -52,4 +66,9 @@ describe("SubscriptionStreamBadge", () => { renderWithMantine(); expect(screen.getByText("Stream ended")).toBeInTheDocument(); }); + + it("renders a labelled never-acknowledged badge", () => { + renderWithMantine(); + expect(screen.getByText("Not acknowledged")).toBeInTheDocument(); + }); }); diff --git a/clients/web/src/components/elements/SubscriptionStreamBadge/subscriptionStreamUtils.ts b/clients/web/src/components/elements/SubscriptionStreamBadge/subscriptionStreamUtils.ts index c2c5253684..233d8295aa 100644 --- a/clients/web/src/components/elements/SubscriptionStreamBadge/subscriptionStreamUtils.ts +++ b/clients/web/src/components/elements/SubscriptionStreamBadge/subscriptionStreamUtils.ts @@ -1,4 +1,5 @@ import type { ResourceSubscriptionStreamStatus } from "../../../../../../core/mcp/types.js"; +import { NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE } from "../../../../../../core/mcp/subscriptionAck.js"; export interface StreamPresentation { /** Mantine palette color name conveying the status. */ @@ -39,6 +40,11 @@ const PRESENTATION: Record< label: "Stream ended", tooltip: `${STREAM_INTRO} The stream is closed and won't reconnect on its own — either the server ended it (for example, on shutdown) or reconnection was abandoned after repeated failures. Re-subscribe to try again.`, }, + "never-acknowledged": { + color: "orange", + label: "Not acknowledged", + tooltip: `${STREAM_INTRO} ${NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE} Re-subscribe to try again.`, + }, }; /** diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.test.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.test.tsx index 86069af616..5cae16a2d5 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.test.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.test.tsx @@ -6,6 +6,7 @@ import type { ResourceTemplateType as ResourceTemplate, } from "@modelcontextprotocol/client"; import type { InspectorResourceSubscription } from "@inspector/core/mcp/types.js"; +import { NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE } from "@inspector/core/mcp/subscriptionAck.js"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { ResourceControls, @@ -394,6 +395,37 @@ describe("ResourceControls", () => { expect(screen.getByText("Subscriptions (0)")).toBeInTheDocument(); expect(screen.queryByText("Listening")).not.toBeInTheDocument(); }); + + // A badge tooltip is not enough for this one: the server broke the listen + // contract and the user has to be told without hovering (#2097). + it("spells out a never-acknowledged close in the panel", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Not acknowledged")).toBeInTheDocument(); + expect( + screen.getByText(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE), + ).toBeInTheDocument(); + }); + + it("shows no such notice while the stream is healthy", () => { + renderWithMantine( + , + ); + expect( + screen.queryByText(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE), + ).not.toBeInTheDocument(); + }); }); // A failed load is rendered above the list instead of leaving the panel diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx index 6a319fb1df..aa1ee9540c 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.tsx @@ -1,4 +1,12 @@ -import { Accordion, Group, Stack, Text, TextInput, Title } from "@mantine/core"; +import { + Accordion, + Alert, + Group, + Stack, + Text, + TextInput, + Title, +} from "@mantine/core"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { RiArrowRightSLine } from "react-icons/ri"; import type { @@ -10,6 +18,7 @@ import type { InspectorResourceSubscription, ResourceSubscriptionStreamState, } from "../../../../../../core/mcp/types.js"; +import { NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE } from "../../../../../../core/mcp/subscriptionAck.js"; import { isModernEra } from "../../elements/EraBadge/eraUtils"; import { SubscriptionStreamBadge } from "../../elements/SubscriptionStreamBadge/SubscriptionStreamBadge"; import { ListChangedIndicator } from "../../elements/ListChangedIndicator/ListChangedIndicator"; @@ -33,6 +42,15 @@ const TightRow = Group.withProps({ gap: "xs", wrap: "nowrap" }); // instead of overflowing the card (#1462). const SidebarStack = Stack.withProps({ gap: "sm", flex: 1, mih: 0 }); +// The never-acknowledged close (#2097) is the one stream status whose reason has +// to be readable without hovering the badge: it is a server-conformance problem +// the user has to act on, not a lifecycle event they can wait out. +const StreamNotice = Alert.withProps({ + color: "orange", + variant: "light", + title: "Subscription not acknowledged", +}); + const SearchInput = TextInput.withProps({ flex: 1, placeholder: "Search...", @@ -358,6 +376,11 @@ export function ResourceControls({ + {streamStatus === "never-acknowledged" && ( + + {NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE} + + )} {filteredSubscriptions.map((sub) => ( { + const neverAcknowledged = () => + new SdkError( + SdkErrorCode.ConnectionClosed, + "subscriptions/listen: server closed the subscription gracefully before acknowledging", + ); + + it("matches the SDK's never-acknowledged close", () => { + expect(isNeverAcknowledgedSubscriptionClose(neverAcknowledged())).toBe( + true, + ); + }); + + // The sibling rejection carries the SAME code, which is exactly why the + // predicate has to read the message: a stream that closed for some other + // reason before acknowledgement may well succeed on a retry. + it("does not match a generic pre-ack close", () => { + expect( + isNeverAcknowledgedSubscriptionClose( + new SdkError( + SdkErrorCode.ConnectionClosed, + "subscriptions/listen closed before the server acknowledged", + ), + ), + ).toBe(false); + }); + + it("does not match another SDK error code carrying the same words", () => { + expect( + isNeverAcknowledgedSubscriptionClose( + new SdkError( + SdkErrorCode.RequestTimeout, + "server closed the subscription gracefully before acknowledging", + ), + ), + ).toBe(false); + }); + + it("does not match a plain Error or a non-error value", () => { + expect( + isNeverAcknowledgedSubscriptionClose( + new Error( + "server closed the subscription gracefully before acknowledging", + ), + ), + ).toBe(false); + expect(isNeverAcknowledgedSubscriptionClose(undefined)).toBe(false); + expect(isNeverAcknowledgedSubscriptionClose("closed")).toBe(false); + }); +}); + +describe("subscriptionFailureMessage (#2097)", () => { + it("reports the explanation for the never-acknowledged close", () => { + expect( + subscriptionFailureMessage( + new SdkError( + SdkErrorCode.ConnectionClosed, + "subscriptions/listen: server closed the subscription gracefully before acknowledging", + ), + ), + ).toBe(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE); + }); + + // Every other failure keeps its own message — that text is the diagnostic, + // and replacing it would trade one silence for another. + it("passes any other failure through unchanged", () => { + expect(subscriptionFailureMessage(new Error("socket hang up"))).toBe( + "socket hang up", + ); + expect(subscriptionFailureMessage("not an Error")).toBe("not an Error"); + }); +}); + +describe("NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE", () => { + // The whole defect was silence, so the copy has to name the wire event, the + // rule it broke, and the fact that nothing further will be attempted. + it("names the notification, the result semantics, and the no-retry decision", () => { + expect(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE).toContain( + "notifications/subscriptions/acknowledged", + ); + expect(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE).toContain( + "graceful closure", + ); + expect(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE).toContain("Not retrying"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts index 8c2a005a91..d41f0da28e 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts @@ -12,6 +12,7 @@ import { } from "@modelcontextprotocol/inspector-test-server"; import type { ServerConfig } from "@modelcontextprotocol/inspector-test-server"; import type { MessageEntry } from "@inspector/core/mcp/types.js"; +import { NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE } from "@inspector/core/mcp/subscriptionAck.js"; /** * Live coverage of the resource-subscription era fork (#1630). On the legacy era @@ -139,6 +140,7 @@ describe("resource subscriptions era fork (#1630)", () => { modernSubscription: McpSubscription | null; modernListenGeneration: number; modernReconnectAttempts: number; + modernNeverAcknowledged: boolean; subscribedResources: Set; refreshModernSubscription(fromReconnect?: boolean): Promise; onModernSubscriptionClosed( @@ -1017,6 +1019,160 @@ describe("resource subscriptions era fork (#1630)", () => { expect(connected.getResourceSubscriptionStreamState().active).toBe(false); }); + /** + * A server that answers `subscriptions/listen` with a bare JSON-RPC result + * — the spec's graceful-closure marker — instead of acknowledging it + * (#2097). The condition is deterministic, so the Inspector must end the + * stream on the first occurrence and say why, rather than re-listing eight + * times in silence (#2063). + * + * These run against the real never-acknowledging test server rather than a + * stubbed rejection, which is what keeps them honest: the predicate reads + * the SDK's message, so an SDK that rephrases it fails here instead of + * silently reverting to the retry loop. + */ + describe("server never acknowledges the listen (#2097)", () => { + const NEVER_ACK = { neverAcknowledgeSubscriptions: true } as const; + + it("fails the subscribe with the explanation, without retrying", async () => { + const started = await startServer(NEVER_ACK); + const { connected, messages } = await connect(started.url, "modern"); + messages.length = 0; + + await expect( + connected.subscribeToResource(RESOURCE_URI), + ).rejects.toThrow(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE); + + // One attempt, not the eight-deep backoff run. Give the reconnect timer + // room to have fired had one been armed (the base delay is 500ms). + await new Promise((resolve) => setTimeout(resolve, 1_200)); + expect( + methodsSent(messages).filter((m) => m === "subscriptions/listen"), + ).toHaveLength(1); + }); + + it("reports the never-acknowledged status while a subscription remains", async () => { + const started = await startServer(NEVER_ACK); + const { connected } = await connect(started.url, "modern"); + const int = internals(connected); + // A URI already in the set stands in for a first subscription this + // server would never have honored: the failing subscribe below rolls + // back only its own URI, so the stream state stays `active` and the + // badge — the surface the reason has to reach — is rendered. + int.subscribedResources.add(RESOURCE_URI_2); + + await expect( + connected.subscribeToResource(RESOURCE_URI), + ).rejects.toThrow(/not retrying/i); + + const state = connected.getResourceSubscriptionStreamState(); + expect(state.status).toBe("never-acknowledged"); + expect(state.active).toBe(true); + expect(state.honoredUris).toEqual([]); + }); + + it("ends a reconnect run rather than spending it on the same answer", async () => { + // A stream that dropped and now re-lists against a server that will not + // acknowledge. The reconnect must stop at the first such answer instead + // of exhausting the eight-attempt cap — the behavior #2063 observed. + const started = await startServer(NEVER_ACK); + const { connected, messages } = await connect(started.url, "modern"); + const int = internals(connected); + // Stand in for the state a dropped stream leaves behind: a subscribed + // URI (so the filter still wants a stream) and a live subscription + // object to close. + int.subscribedResources.add(RESOURCE_URI); + const fake = await installFakeSubscription(int); + + messages.length = 0; + int.onModernSubscriptionClosed( + fake.sub, + "remote", + int.modernListenGeneration, + ); + await vi.waitFor( + () => { + expect(connected.getResourceSubscriptionStreamState().status).toBe( + "never-acknowledged", + ); + }, + { timeout: 5_000 }, + ); + // One re-listen consumed, not the eight the cap allows; and the run is + // ended rather than counting up towards it. + expect( + methodsSent(messages).filter((m) => m === "subscriptions/listen"), + ).toHaveLength(1); + expect(int.modernReconnectAttempts).toBe(0); + }); + + /** + * The showcase fixture's mode, and the only shape that reaches the badge: + * the never-acknowledged status is gated on a live subscription, which a + * server refusing from the outset never lets you hold. + * + * The list-change exemption is the load-bearing half. The Inspector opens + * a listen at connect time whenever a list-change opt-in is live (#1920), + * so a mode that counted *listens* would spend its allowance there and + * refuse the user's very first Subscribe — leaving the documented repro + * describing something the trace does not show. + */ + it("acknowledges the first resource subscription, then refuses (after-first)", async () => { + const started = await startServer({ + neverAcknowledgeSubscriptions: "after-first", + }); + // Every list-change opt-in ON, so the connect-time listen really is + // opened — that is the listen the exemption has to let through. + const { connected, messages } = await connect(started.url, "modern", { + tools: true, + resources: true, + prompts: true, + }); + expect( + methodsSent(messages).filter((m) => m === "subscriptions/listen"), + ).toHaveLength(1); + + // First resource subscription: acknowledged, so the allowance was still + // unspent after the connect-time listen. + await expect( + connected.subscribeToResource(RESOURCE_URI), + ).resolves.toBeUndefined(); + expect(connected.getResourceSubscriptionStreamState()).toMatchObject({ + active: true, + status: "acknowledged", + }); + + // Second: refused. The first URI survives the rollback, so the stream + // stays `active` and the badge is rendered. + await expect( + connected.subscribeToResource(RESOURCE_URI_2), + ).rejects.toThrow(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE); + expect(connected.getResourceSubscriptionStreamState()).toMatchObject({ + active: true, + status: "never-acknowledged", + }); + expect(connected.getSubscribedResources()).toEqual([RESOURCE_URI]); + }); + + it("retries again after the user re-subscribes", async () => { + // The flag is a fact about the last attempt, not a latch: a fresh + // user-initiated subscribe must be allowed to reach the server again. + const started = await startServer(NEVER_ACK); + const { connected } = await connect(started.url, "modern"); + await expect( + connected.subscribeToResource(RESOURCE_URI), + ).rejects.toThrow(); + expect(internals(connected).modernNeverAcknowledged).toBe(true); + + await expect( + connected.subscribeToResource(RESOURCE_URI_2), + ).rejects.toThrow(); + // Reset at the start of the second attempt and set again by its own + // rejection — i.e. the second listen really went out. + expect(internals(connected).modernNeverAcknowledged).toBe(true); + }); + }); + it("ignores a close callback from a superseded generation", async () => { const started = await startServer({}); const { connected } = await connect(started.url, "modern"); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 3673932eb9..2f115beb49 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -46,6 +46,11 @@ import { resolveModernLogLevel, } from "./types.js"; import { cleanRoots } from "./serverList.js"; +import { + isNeverAcknowledgedSubscriptionClose, + subscriptionFailureMessage, + NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE, +} from "./subscriptionAck.js"; // Fallback client identity, used ONLY when a caller doesn't pass // `clientIdentity`. Real clients supply their own: the Node clients (CLI, TUI) // read the single-source version from the root package.json via @@ -608,6 +613,13 @@ export class InspectorClient extends InspectorClientEventTarget { // pending re-listen timer. private modernReconnectAttempts = 0; private modernReconnectTimer: ReturnType | undefined; + // Set when the last `listen()` was rejected because the server answered it + // with a bare graceful-closure result instead of acknowledging (#2097). That + // condition is deterministic, so the reconnect machinery must not treat it as + // a drop to retry; the flag is what carries the distinction from the rejection + // site to the two failure handlers. Cleared by any user-initiated refresh (a + // subscribe/unsubscribe is a fresh attempt, and the server may have changed). + private modernNeverAcknowledged = false; // Task ids the user explicitly cancelled. A cancel makes the in-flight // `callToolStream` reject with a generic -32603 error, which the stream's // error path would otherwise report as a *failed* task — flashing "failed" @@ -1786,6 +1798,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.modernListenGeneration++; this.clearModernReconnectTimer(); this.modernReconnectAttempts = 0; + this.modernNeverAcknowledged = false; this.modernSubscription = null; // Announced only once both have moved: a listener that ran between them // would see an empty set with an `active` stream — the combination this @@ -5968,6 +5981,7 @@ export class InspectorClient extends InspectorClientEventTarget { if (!fromReconnect) { this.clearModernReconnectTimer(); this.modernReconnectAttempts = 0; + this.modernNeverAcknowledged = false; } const generation = ++this.modernListenGeneration; @@ -5984,10 +5998,31 @@ export class InspectorClient extends InspectorClientEventTarget { return; } - const subscription = await this.client.listen( - this.buildSubscriptionFilter(), - this.getRequestOptions(), - ); + let subscription: McpSubscription; + try { + subscription = await this.client.listen( + this.buildSubscriptionFilter(), + this.getRequestOptions(), + ); + } catch (error) { + // Record the one rejection that must not be retried, so the failure + // handlers can tell it from a drop (#2097). Recorded rather than acted on + // here because *which* state to write depends on whether this caller still + // owns the stream, which only they know — see + // `reconcileModernStreamStateAfterFailedRefresh`. + // + // Gated on the same generation test the callers use: a newer refresh has + // already cleared the flag for its own attempt, and a stale caller writing + // to it afterwards would make that attempt's *unrelated* failure look + // deterministic and end a stream that deserved a retry. + if ( + generation === this.modernListenGeneration && + isNeverAcknowledgedSubscriptionClose(error) + ) { + this.modernNeverAcknowledged = true; + } + throw error; + } // A newer refresh superseded us while awaiting the ack — discard this one. if (generation !== this.modernListenGeneration) { @@ -6101,9 +6136,39 @@ export class InspectorClient extends InspectorClientEventTarget { this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE); return; } + // The one failure a retry cannot fix (#2097) — end the stream here rather + // than spending the whole backoff run on a server that will answer the same + // way every time. + if (this.modernNeverAcknowledged) { + this.endModernStreamNeverAcknowledged(); + return; + } this.scheduleModernReconnect(); } + /** + * Settle the stream on the never-acknowledged close (#2097): a status of its + * own so the UI can say what happened, no reconnect, and a log line carrying + * the same sentence the UI shows. + * + * The state is *not* `"ended"` — that badge covers the two expected closes (a + * server shutting an established stream down, and reconnection abandoned after + * repeated failures), and reading this case as either of them is exactly the + * silence #2063 reported. + */ + private endModernStreamNeverAcknowledged(): void { + this.clearModernReconnectTimer(); + this.logger.warn( + { message: NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE }, + "subscriptions/listen closed without an acknowledgement", + ); + this.setModernStreamState({ + active: this.modernStreamActive(), + status: "never-acknowledged", + honoredUris: [], + }); + } + /** * Schedule a reconnect re-listen after the current backoff delay (#1630). * `modernReconnectAttempts` reflects the number of *consecutive failed* @@ -6140,6 +6205,18 @@ export class InspectorClient extends InspectorClientEventTarget { * stream ended (re-subscribing resets the run and tries again). */ private onModernReconnectFailed(): void { + // A reconnect that lost to the never-acknowledged close (#2097) ends the run + // immediately: the remaining attempts would each buy the same answer. This + // is reachable when a stream that *had* been acknowledged dropped and the + // server has since started refusing to acknowledge. + if ( + this.modernNeverAcknowledged && + !isTerminalStatus(this.status) && + this.wantsModernStream() + ) { + this.endModernStreamNeverAcknowledged(); + return; + } this.modernReconnectAttempts += 1; if ( this.modernReconnectAttempts > MODERN_RECONNECT_MAX_ATTEMPTS || @@ -6233,7 +6310,7 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchSubscriptionsChange(); } catch (error) { throw new Error( - `Failed to subscribe to resource: ${error instanceof Error ? error.message : String(error)}`, + `Failed to subscribe to resource: ${subscriptionFailureMessage(error)}`, { cause: error }, ); } @@ -6294,7 +6371,7 @@ export class InspectorClient extends InspectorClientEventTarget { } } catch (error) { throw new Error( - `Failed to unsubscribe from resource: ${error instanceof Error ? error.message : String(error)}`, + `Failed to unsubscribe from resource: ${subscriptionFailureMessage(error)}`, { cause: error }, ); } diff --git a/core/mcp/subscriptionAck.ts b/core/mcp/subscriptionAck.ts new file mode 100644 index 0000000000..83a5092377 --- /dev/null +++ b/core/mcp/subscriptionAck.ts @@ -0,0 +1,75 @@ +import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; + +/** + * Recognizing the "server closed the subscription without ever acknowledging + * it" failure of a modern-era `subscriptions/listen` (#2097). + * + * A `subscriptions/listen` request is long-lived: the first message on the + * stream MUST be `notifications/subscriptions/acknowledged`, and the JSON-RPC + * `result` for the listen id is reserved as the *graceful-closure* marker + * (2026-07-28 spec, Subscriptions → Graceful Closure). A server that answers the + * listen request with a bare `result` and never acknowledges is therefore saying + * "acknowledged and closed in the same breath": the SDK settles the subscription + * with cause `"graceful"` and rejects the pending `listen()` promise. + * + * That is a *deterministic* condition — the same server answers the same way + * every time — so it must not be retried, and it is not the same event as a + * graceful shutdown of an established stream. Without the distinction the + * Inspector re-listed eight times over roughly a minute and settled on a bare + * "Stream ended" badge that said nothing about why (#2063). + */ + +/** + * The distinguishing fragment of the SDK's rejection message for this case. + * + * The SDK does not give the never-acknowledged close a code of its own — it is + * an `SdkError(ConnectionClosed)`, the same code a stream closed for any other + * reason before acknowledgement carries — so the message is the only thing that + * separates them. Matching it is deliberately narrow rather than clever: the + * alternative is treating *every* pre-ack `ConnectionClosed` as deterministic + * and refusing to retry a genuinely transient drop. + * + * The live check on this string is the integration test in + * `inspectorClient-subscriptions-era.test.ts`, which drives a real server that + * answers `subscriptions/listen` with a bare result; an SDK that rephrases the + * message fails there rather than silently reverting to the eight-retry + * behavior. + */ +const NEVER_ACKNOWLEDGED_SDK_MESSAGE = + "closed the subscription gracefully before acknowledging"; + +/** + * The explanation shown to the user, in the UI and in the client log. Lives in + * `core/` so every client says the same thing about the same wire event. + */ +export const NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE = + "The server closed the subscription without acknowledging it. A subscriptions/listen request must first be answered with a notifications/subscriptions/acknowledged notification; a JSON-RPC result for the listen id means graceful closure. Not retrying — the server would answer the same way again."; + +/** + * True when a rejected `listen()` is the never-acknowledged close above, rather + * than a transport failure, a timeout, or a pre-ack error response — each of + * which may well succeed on a retry. + */ +export function isNeverAcknowledgedSubscriptionClose( + error: unknown, +): error is SdkError { + return ( + SdkError.isInstance(error) && + error.code === SdkErrorCode.ConnectionClosed && + error.message.includes(NEVER_ACKNOWLEDGED_SDK_MESSAGE) + ); +} + +/** + * The sentence to report for a failed subscribe/unsubscribe. For the + * never-acknowledged close that is the explanation above rather than the SDK's + * own wording, which names the wire event without saying what the Inspector did + * about it (nothing further) — the omission #2097 is about. Every other failure + * keeps its own message, which is the diagnostic. + */ +export function subscriptionFailureMessage(error: unknown): string { + if (isNeverAcknowledgedSubscriptionClose(error)) { + return NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE; + } + return error instanceof Error ? error.message : String(error); +} diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 56a12bcb6e..17170ac3d2 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -490,12 +490,19 @@ export interface InspectorResourceSubscription { * - `"ended"` — the server tore the stream down deliberately (`closed` resolved * `"graceful"`, e.g. on shutdown) or reconnection was abandoned; no automatic * re-listen. + * - `"never-acknowledged"` — the server answered the `listen()` request itself + * with a JSON-RPC `result` (the spec's graceful-closure marker) without ever + * sending `notifications/subscriptions/acknowledged`, so the stream was closed + * in the same breath it was opened (#2097). Distinct from `"ended"` because + * the condition is deterministic rather than a shutdown of an established + * stream: the Inspector does not retry it, and the UI says why. */ export type ResourceSubscriptionStreamStatus = | "connecting" | "acknowledged" | "reconnecting" - | "ended"; + | "ended" + | "never-acknowledged"; /** * State of the modern-era resource-subscription listen stream (#1630). diff --git a/test-servers/configs/subscriptions-never-acknowledged-http.json b/test-servers/configs/subscriptions-never-acknowledged-http.json new file mode 100644 index 0000000000..82e2d62c23 --- /dev/null +++ b/test-servers/configs/subscriptions-never-acknowledged-http.json @@ -0,0 +1,16 @@ +{ + "serverInfo": { + "name": "subscriptions-never-acknowledged", + "version": "1.0.0" + }, + "resources": [{ "preset": "numbered_resources", "params": { "count": 3 } }], + "subscriptions": true, + "listChanged": { "resources": true }, + "transport": { + "type": "streamable-http", + "port": 3221, + "modern": { + "neverAcknowledgeSubscriptions": "after-first" + } + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 89d619b8f6..85494d1fb2 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -699,6 +699,31 @@ export interface ServerConfig { * error rendering (a conformant server never produces these on demand). */ injectSpecErrors?: boolean; + /** + * When true, the modern HTTP leg installs a middleware that answers every + * `subscriptions/listen` with a bare JSON-RPC `result` — the spec's + * graceful-closure marker — instead of acknowledging it. That is the + * non-conformant server shape from #2097: "acknowledged and closed in the + * same breath". Used by the `subscriptions-never-acknowledged-http` + * showcase; a conformant server never does this on an opening listen. + * + * `"after-first"` acknowledges the first listen that **subscribes to a + * resource** and refuses every resource-subscription listen after it — the + * *reconnect* shape, and the only way to reach the Inspector's + * never-acknowledged badge by hand (it is gated on a live subscription, + * which a server refusing from the outset never lets you hold). + * + * A listen carrying no `resourceSubscriptions` is always acknowledged under + * this mode and does not consume the allowance. That exemption is what makes + * the mode reproducible: the Inspector already opens a listen at connect + * time when a list-change opt-in is live (#1920), so counting listens rather + * than resource-subscription listens would spend the allowance before the + * user clicks anything, refusing the very first Subscribe. + * + * `true` refuses every listen unconditionally, list-change-only ones + * included — the literal shape in the report. + */ + neverAcknowledgeSubscriptions?: boolean | "after-first"; }; /** * Optional server control for orderly shutdown (test HTTP server). diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 670f172b2e..18e09f5a12 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -107,7 +107,11 @@ export interface ConfigFile { */ modern?: | boolean - | { legacy?: "stateless" | "reject"; injectSpecErrors?: boolean }; + | { + legacy?: "stateless" | "reject"; + injectSpecErrors?: boolean; + neverAcknowledgeSubscriptions?: boolean | "after-first"; + }; }; } diff --git a/test-servers/src/test-server-http.ts b/test-servers/src/test-server-http.ts index b8a5007f4e..20f9baaddc 100644 --- a/test-servers/src/test-server-http.ts +++ b/test-servers/src/test-server-http.ts @@ -190,6 +190,73 @@ interface JsonRpcCallBody { params?: { name?: string }; } +/** The slice of a `subscriptions/listen` body the never-ack injector reads. */ +interface JsonRpcListenBody { + id?: string | number | null; + method?: string; + params?: { notifications?: { resourceSubscriptions?: unknown } }; +} + +/** + * Answer every opening `subscriptions/listen` with a bare JSON-RPC `result` + * instead of a `notifications/subscriptions/acknowledged` (#2097). + * + * On the 2026-07-28 era a listen request is long-lived: the acknowledgement is + * the first message on its stream, and the `result` for the listen id is + * reserved as the *graceful-closure* marker. Answering with the result up front + * is therefore the "acknowledged and closed in the same breath" shape the + * Inspector used to re-list against eight times in silence. The result is + * deliberately bare — no `resultType` discriminator — matching the payload in + * the original report rather than the spec's example. + * + * A conformant server never does this, which is the point: there is no way to + * reach the Inspector's never-acknowledged path from one. + * + * `"after-first"` acknowledges the first listen that actually **subscribes to a + * resource** and refuses every one after it. That is the *reconnect* shape — a + * stream that was acknowledged, then re-listed against a server that has since + * started refusing — and it is the only way to reach the never-acknowledged + * badge by hand: the badge is gated on a live subscription, which a server + * refusing from the outset never lets you hold. + * + * Counting *resource-subscription* listens rather than listens is what makes + * that reproducible. The Inspector already opens a listen at connect time when a + * list-change opt-in is live (#1920), so a plain "let the first one through" + * rule spends its allowance before the user has clicked anything, and the very + * first Subscribe is refused. Such list-change-only listens are always + * acknowledged here, so they neither consume the allowance nor break the + * unrelated notifications riding that stream. + */ +function createNeverAcknowledgeSubscriptionsInjector( + mode: true | "after-first", +): express.RequestHandler { + let subscribingListens = 0; + return (req, res, next) => { + const body = req.body as JsonRpcListenBody | undefined; + if (body?.method !== "subscriptions/listen") { + next(); + return; + } + if (mode === "after-first") { + const uris = body.params?.notifications?.resourceSubscriptions; + if (!Array.isArray(uris) || uris.length === 0) { + next(); + return; + } + subscribingListens += 1; + if (subscribingListens === 1) { + next(); + return; + } + } + res.status(200).json({ + jsonrpc: "2.0", + id: body.id ?? null, + result: {}, + }); + }; +} + const specErrorInjector: express.RequestHandler = (req, res, next) => { const body = req.body as JsonRpcCallBody | undefined; const trigger = @@ -390,6 +457,16 @@ export class TestServerHttp { ? [specErrorInjector] : []; + // Optional never-acknowledge injector (#2097): answers `subscriptions/listen` + // with the graceful-closure result up front, so the Inspector's + // never-acknowledged path has a server to reproduce it against. + const neverAck = this.config.modern?.neverAcknowledgeSubscriptions; + if (neverAck) { + extraMiddleware.push( + createNeverAcknowledgeSubscriptionsInjector(neverAck), + ); + } + // Modern tasks extension (SEP-2663): the SDK's modern leg era-gates inbound // `tasks/*` spec methods, so serve them from a middleware ahead of the SDK // handler, backed by the shared runtime (also used by the tools/call task