fix(core): say why a never-acknowledged subscriptions/listen ended
A modern-era `subscriptions/listen` is a long-lived request: the first message on its stream must be `notifications/subscriptions/acknowledged`, and the JSON-RPC `result` for the listen id is reserved as the graceful-closure marker. A server that answers the listen request with a bare `result` is saying "acknowledged and closed in the same breath" — the SDK settles the subscription `graceful` and rejects the pending `listen()`. The Inspector read that rejection as an ordinary drop: eight re-lists over roughly a minute, the badge flickering `Reconnecting…`, and a final bare `Stream ended` that said nothing about why. Accepting the message was never wrong — it is a well-formed graceful-closure response. The silence was. - `core/mcp/subscriptionAck.ts` owns the predicate. The SDK gives this close no code of its own — it is the same `SdkError(ConnectionClosed)` any pre-ack close carries — so the predicate reads the message, since the alternative is refusing to retry a genuinely transient drop. - The condition is deterministic, so the stream ends on the first occurrence instead of spending the whole backoff run. Both routes are covered: the one a subscribe/unsubscribe reaches, and the one a reconnect reaches when an acknowledged stream drops and the server has since started refusing. - A `"never-acknowledged"` status of its own, not `"ended"` — `ended` means an expected close. It surfaces as the thrown error's message, an orange "Not acknowledged" badge, and a notice in the panel. - `subscriptions-never-acknowledged-http.json` reproduces it. It acknowledges the first listen that subscribes to a resource and refuses every one after, which is what makes the badge reachable by hand. Closes #2097 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
6bc3251962
commit
8535ae0c2f
7 changed files with 612 additions and 10 deletions
|
|
@ -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" },
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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(<SubscriptionStreamBadge status="ended" />);
|
||||
expect(screen.getByText("Stream ended")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders a labelled never-acknowledged badge", () => {
|
||||
renderWithMantine(<SubscriptionStreamBadge status="never-acknowledged" />);
|
||||
expect(screen.getByText("Not acknowledged")).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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.`,
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ControlledResourceControls
|
||||
protocolEra="modern"
|
||||
subscriptionStreamState={{
|
||||
active: true,
|
||||
status: "never-acknowledged",
|
||||
honoredUris: [],
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
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(
|
||||
<ControlledResourceControls
|
||||
protocolEra="modern"
|
||||
subscriptionStreamState={activeAck}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByText(NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
// A failed load is rendered above the list instead of leaving the panel
|
||||
|
|
|
|||
|
|
@ -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({
|
|||
</Accordion.Control>
|
||||
<Accordion.Panel>
|
||||
<Stack gap="xs">
|
||||
{streamStatus === "never-acknowledged" && (
|
||||
<StreamNotice>
|
||||
{NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE}
|
||||
</StreamNotice>
|
||||
)}
|
||||
{filteredSubscriptions.map((sub) => (
|
||||
<ResourceSubscribedItem
|
||||
key={sub.resource.uri}
|
||||
|
|
|
|||
100
clients/web/src/test/core/mcp/subscriptionAck.test.ts
Normal file
100
clients/web/src/test/core/mcp/subscriptionAck.test.ts
Normal file
|
|
@ -0,0 +1,100 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
isNeverAcknowledgedSubscriptionClose,
|
||||
subscriptionFailureMessage,
|
||||
NEVER_ACKNOWLEDGED_SUBSCRIPTION_MESSAGE,
|
||||
} from "@inspector/core/mcp/subscriptionAck.js";
|
||||
|
||||
/**
|
||||
* Unit coverage of the never-acknowledged predicate (#2097). The *live* check —
|
||||
* that the SDK still phrases this rejection the way the predicate expects — is
|
||||
* the integration test in `inspectorClient-subscriptions-era.test.ts`, which
|
||||
* drives a real server answering `subscriptions/listen` with a bare result.
|
||||
*/
|
||||
describe("isNeverAcknowledgedSubscriptionClose (#2097)", () => {
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
|
@ -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<string>;
|
||||
refreshModernSubscription(fromReconnect?: boolean): Promise<void>;
|
||||
onModernSubscriptionClosed(
|
||||
|
|
@ -1017,6 +1019,112 @@ 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);
|
||||
});
|
||||
|
||||
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");
|
||||
|
|
|
|||
Loading…
Reference in a new issue