feat(core): surface mid-session transport failures via lastError (#1489)
Closes #1323. Adds an `error`-event-driven `lastError` on useInspectorClient and a mid-session failure toast in App.tsx; the transport onerror is scoped to non-handshake (status !== connecting) so handshake failures stay on the connect() rejection path. Follow-up: #1490.
This commit is contained in:
parent
bb32d1db5f
commit
b4716a1405
2 changed files with 398 additions and 7 deletions
|
|
@ -150,6 +150,98 @@ describe("useInspectorClient", () => {
|
|||
expect(result.current.status).toBe("connected");
|
||||
});
|
||||
|
||||
it("captures lastError from the client's error event", () => {
|
||||
const client = new FakeInspectorClient({ status: "connected" });
|
||||
const { result } = renderHook(() => useInspectorClient(client));
|
||||
expect(result.current.lastError).toBeUndefined();
|
||||
act(() => {
|
||||
client.dispatchTypedEvent("error", new Error("stdio subprocess crashed"));
|
||||
});
|
||||
expect(result.current.lastError).toBe("stdio subprocess crashed");
|
||||
});
|
||||
|
||||
it("clears lastError when a new connection attempt begins (connecting)", () => {
|
||||
const client = new FakeInspectorClient({ status: "connected" });
|
||||
const { result } = renderHook(() => useInspectorClient(client));
|
||||
act(() => {
|
||||
client.dispatchTypedEvent("error", new Error("SSE stream dropped"));
|
||||
});
|
||||
expect(result.current.lastError).toBe("SSE stream dropped");
|
||||
|
||||
// A reconnect drives status → "connecting", which clears the stale error.
|
||||
act(() => {
|
||||
client.setStatus("connecting");
|
||||
});
|
||||
expect(result.current.lastError).toBeUndefined();
|
||||
});
|
||||
|
||||
it("keeps lastError across the error status transition", () => {
|
||||
const client = new FakeInspectorClient({ status: "connected" });
|
||||
const { result } = renderHook(() => useInspectorClient(client));
|
||||
// The transport onerror path fires statusChange("error") then error; the
|
||||
// "error" status must not clear the message the error event carries.
|
||||
act(() => {
|
||||
client.setStatus("error");
|
||||
client.dispatchTypedEvent("error", new Error("HTTP 503"));
|
||||
});
|
||||
expect(result.current.status).toBe("error");
|
||||
expect(result.current.lastError).toBe("HTTP 503");
|
||||
});
|
||||
|
||||
it("keeps lastError across a trailing disconnected transition", () => {
|
||||
const client = new FakeInspectorClient({ status: "connected" });
|
||||
const { result } = renderHook(() => useInspectorClient(client));
|
||||
act(() => {
|
||||
client.dispatchTypedEvent("error", new Error("stdio crashed"));
|
||||
});
|
||||
expect(result.current.lastError).toBe("stdio crashed");
|
||||
|
||||
// A real crash often trails the error with an onclose → statusChange
|
||||
// ("disconnected"). The toast effect depends on that NOT clearing
|
||||
// lastError (only the next "connecting" edge does).
|
||||
act(() => {
|
||||
client.setStatus("disconnected");
|
||||
});
|
||||
expect(result.current.status).toBe("disconnected");
|
||||
expect(result.current.lastError).toBe("stdio crashed");
|
||||
});
|
||||
|
||||
it("resets lastError when the client prop changes", () => {
|
||||
const a = new FakeInspectorClient({ status: "connected" });
|
||||
const b = new FakeInspectorClient({ status: "connected" });
|
||||
const { result, rerender } = renderHook(
|
||||
({ c }: { c: InspectorClientProtocol }) => useInspectorClient(c),
|
||||
{ initialProps: { c: a as InspectorClientProtocol } },
|
||||
);
|
||||
act(() => {
|
||||
a.dispatchTypedEvent("error", new Error("boom"));
|
||||
});
|
||||
expect(result.current.lastError).toBe("boom");
|
||||
|
||||
rerender({ c: b });
|
||||
expect(result.current.lastError).toBeUndefined();
|
||||
|
||||
// The old client's error event is ignored after the swap.
|
||||
act(() => {
|
||||
a.dispatchTypedEvent("error", new Error("late"));
|
||||
});
|
||||
expect(result.current.lastError).toBeUndefined();
|
||||
});
|
||||
|
||||
it("resets lastError when client becomes null", () => {
|
||||
const client = new FakeInspectorClient({ status: "connected" });
|
||||
const { result, rerender } = renderHook(
|
||||
({ c }: { c: InspectorClientProtocol | null }) => useInspectorClient(c),
|
||||
{ initialProps: { c: client as InspectorClientProtocol | null } },
|
||||
);
|
||||
act(() => {
|
||||
client.dispatchTypedEvent("error", new Error("boom"));
|
||||
});
|
||||
expect(result.current.lastError).toBe("boom");
|
||||
rerender({ c: null });
|
||||
expect(result.current.lastError).toBeUndefined();
|
||||
});
|
||||
|
||||
it("reads clientCapabilities lazily from the client and defaults to {} when null", () => {
|
||||
const advertised: ClientCapabilities = {
|
||||
elicitation: { form: {} },
|
||||
|
|
|
|||
|
|
@ -2527,6 +2527,175 @@ describe("InspectorClient", () => {
|
|||
await client.disconnect();
|
||||
expect(disconnectFired).toBe(true);
|
||||
});
|
||||
|
||||
it("emits an error event and sets status to error on a mid-session transport failure", async () => {
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: "stdio",
|
||||
command: serverCommand.command,
|
||||
args: serverCommand.args,
|
||||
},
|
||||
{
|
||||
environment: { transport: createTransportNode },
|
||||
},
|
||||
);
|
||||
|
||||
const errors: Error[] = [];
|
||||
client.addEventListener("error", (event) => {
|
||||
errors.push(event.detail);
|
||||
});
|
||||
|
||||
await client.connect();
|
||||
expect(client.getStatus()).toBe("connected");
|
||||
|
||||
// Simulate the transport dying mid-session (stdio crash / SSE drop /
|
||||
// HTTP 5xx) by invoking the `onerror` the client attached to the base
|
||||
// transport — the same callback the SDK fires on a real failure.
|
||||
const baseTransport = (
|
||||
client as unknown as {
|
||||
baseTransport: { onerror?: (error: Error) => void };
|
||||
}
|
||||
).baseTransport;
|
||||
const failure = new Error("stdio subprocess crashed");
|
||||
baseTransport.onerror?.(failure);
|
||||
|
||||
expect(client.getStatus()).toBe("error");
|
||||
expect(errors).toHaveLength(1);
|
||||
expect(errors[0]).toBe(failure);
|
||||
|
||||
await client.disconnect();
|
||||
});
|
||||
|
||||
it("still emits error when onclose lands before onerror on a mid-session crash", async () => {
|
||||
// Many transports fire BOTH onclose and onerror on a real crash, in a
|
||||
// transport-dependent order. When onclose lands first it flips status to
|
||||
// "disconnected"; the guard must NOT swallow the trailing onerror's
|
||||
// reason (its only surface). Regression lock for the #1489 re-review.
|
||||
client = new InspectorClient(
|
||||
{
|
||||
type: "stdio",
|
||||
command: serverCommand.command,
|
||||
args: serverCommand.args,
|
||||
},
|
||||
{
|
||||
environment: { transport: createTransportNode },
|
||||
},
|
||||
);
|
||||
|
||||
const errors: Error[] = [];
|
||||
client.addEventListener("error", (event) => {
|
||||
errors.push(event.detail);
|
||||
});
|
||||
|
||||
await client.connect();
|
||||
expect(client.getStatus()).toBe("connected");
|
||||
|
||||
const baseTransport = (
|
||||
client as unknown as {
|
||||
baseTransport: {
|
||||
onclose?: () => void;
|
||||
onerror?: (error: Error) => void;
|
||||
};
|
||||
}
|
||||
).baseTransport;
|
||||
|
||||
// onclose first → status "disconnected"; then the trailing onerror.
|
||||
baseTransport.onclose?.();
|
||||
expect(client.getStatus()).toBe("disconnected");
|
||||
const failure = new Error("stdio subprocess crashed");
|
||||
baseTransport.onerror?.(failure);
|
||||
|
||||
expect(client.getStatus()).toBe("error");
|
||||
expect(errors).toHaveLength(1);
|
||||
expect(errors[0]).toBe(failure);
|
||||
|
||||
await client.disconnect();
|
||||
});
|
||||
|
||||
it("does not emit an error event when the handshake rejects connect()", async () => {
|
||||
// A transport whose start() rejects makes connect() reject (handshake
|
||||
// failure). The caller gets the reason via the rejected promise, so the
|
||||
// client must NOT also dispatch the `error` event (which is reserved for
|
||||
// non-awaited, mid-session failures). See #1323.
|
||||
const failingFactory = () => ({
|
||||
transport: {
|
||||
start: () => Promise.reject(new Error("handshake refused")),
|
||||
send: async () => {},
|
||||
close: async () => {},
|
||||
onclose: undefined,
|
||||
onerror: undefined,
|
||||
onmessage: undefined,
|
||||
sessionId: undefined,
|
||||
} as unknown as import("@modelcontextprotocol/sdk/shared/transport.js").Transport,
|
||||
});
|
||||
client = new InspectorClient(
|
||||
{ type: "streamable-http", url: "http://localhost:1/never" },
|
||||
{ environment: { transport: failingFactory } },
|
||||
);
|
||||
|
||||
let errorFired = false;
|
||||
client.addEventListener("error", () => {
|
||||
errorFired = true;
|
||||
});
|
||||
|
||||
await expect(client.connect()).rejects.toThrow(/handshake refused/);
|
||||
expect(client.getStatus()).toBe("error");
|
||||
expect(errorFired).toBe(false);
|
||||
});
|
||||
|
||||
it("does not emit error or flip to error when onerror fires during the handshake", async () => {
|
||||
// The transport listeners are attached before the handshake runs, so a
|
||||
// transport that reports a connect-time error via `onerror` (some SDK
|
||||
// transports do this in addition to rejecting connect()) must NOT be
|
||||
// treated as a mid-session failure: that would double-report a handshake
|
||||
// error the awaited connect() rejection already surfaces. See #1323.
|
||||
let rejectStart: (error: Error) => void = () => {};
|
||||
const startPromise = new Promise<void>((_resolve, reject) => {
|
||||
rejectStart = reject;
|
||||
});
|
||||
const transport = {
|
||||
start: () => startPromise,
|
||||
send: async () => {},
|
||||
close: async () => {},
|
||||
onclose: undefined as undefined | (() => void),
|
||||
onerror: undefined as undefined | ((error: Error) => void),
|
||||
onmessage: undefined,
|
||||
sessionId: undefined,
|
||||
};
|
||||
const factory = () => ({
|
||||
transport:
|
||||
transport as unknown as import("@modelcontextprotocol/sdk/shared/transport.js").Transport,
|
||||
});
|
||||
client = new InspectorClient(
|
||||
{ type: "streamable-http", url: "http://localhost:1/never" },
|
||||
{ environment: { transport: factory } },
|
||||
);
|
||||
|
||||
let errorFired = false;
|
||||
client.addEventListener("error", () => {
|
||||
errorFired = true;
|
||||
});
|
||||
|
||||
// Kick off connect() without awaiting — it parks on the hanging start().
|
||||
// `status` is "connecting" and `onerror` is wired by this point.
|
||||
const pending = client.connect().catch(() => {});
|
||||
expect(client.getStatus()).toBe("connecting");
|
||||
|
||||
// A connect-time transport error arrives via onerror.
|
||||
transport.onerror?.(new Error("socket error mid-handshake"));
|
||||
|
||||
// It must be ignored: still connecting, no error event dispatched.
|
||||
expect(client.getStatus()).toBe("connecting");
|
||||
expect(errorFired).toBe(false);
|
||||
|
||||
// Unblock the handshake so connect() settles (and tears down cleanly).
|
||||
rejectStart(new Error("aborted"));
|
||||
await pending;
|
||||
// connect()'s catch transitions to "error" via the rejection path —
|
||||
// still without dispatching the `error` event.
|
||||
expect(client.getStatus()).toBe("error");
|
||||
expect(errorFired).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("Sampling Requests", () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue