fix: address Copilot review round 4 on #2318
- fetchTracking: one non-throwing reporter fronts every stream update — the open (inside the fetch wrapper), each coalesced count (from a timer) and the close (off the discarded watcher). - rawWireRequest annotates a request timeout its transport's send rejects with, so the browser's relay timeout on a raw-wire call reads like every other. - RemoteClientTransport reports still-open streams closed on close(), since the backend watcher's own close cannot reach a channel that the disconnect has already torn down. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APKSYHGxXBUpnR7ErFiLJn Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
ec09628705
commit
abc4967298
3 changed files with 180 additions and 15 deletions
|
|
@ -934,6 +934,28 @@ describe("createFetchTracker long-lived stream watching (#2318)", () => {
|
|||
expect(updates.at(-1)!.stream.closedAt).toBeInstanceOf(Date);
|
||||
});
|
||||
|
||||
it("returns the response even when the listener throws on the open report", async () => {
|
||||
// The open is reported synchronously inside the fetch wrapper; a throw
|
||||
// there must not turn a successful response into a rejected fetch.
|
||||
const server = controlledStream();
|
||||
const response = new Response(server.stream, {
|
||||
headers: { "content-type": "text/event-stream" },
|
||||
});
|
||||
const fetcher = createFetchTracker((async () => response) as typeof fetch, {
|
||||
updateStream: () => {
|
||||
throw new Error("sink closed");
|
||||
},
|
||||
streamUpdateIntervalMs: 0,
|
||||
});
|
||||
await expect(
|
||||
fetcher("https://example.com/mcp", { method: "GET" }),
|
||||
).resolves.toBe(response);
|
||||
// ...and a throw from a coalesced count does not escape the timer either.
|
||||
server.push("data: a\n\n");
|
||||
server.end();
|
||||
await flush();
|
||||
});
|
||||
|
||||
it("survives a stream-update listener that throws on the close", async () => {
|
||||
// The watcher's promise is discarded, so a throw from the final report
|
||||
// would otherwise be an unhandled rejection — which fails the whole run.
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { describe, it, expect, vi } from "vitest";
|
||||
import { SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { ModernGetTaskResultSchema } from "@inspector/core/mcp/modernTaskSchemas.js";
|
||||
|
||||
|
|
@ -151,6 +151,46 @@ describe("InspectorClient raw-wire channel (#1631)", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("annotates a request timeout the transport's send rejects with (#2318)", async () => {
|
||||
// The browser's remote transport awaits the response inside `send`, so
|
||||
// its relay wait can expire before the local timer; it arrives here as
|
||||
// the SDK's timeout shape and gets the same annotation.
|
||||
const client = makeClient();
|
||||
internals(client).transport = {
|
||||
send: vi.fn().mockRejectedValue(
|
||||
new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", {
|
||||
timeout: 60_000,
|
||||
}),
|
||||
),
|
||||
};
|
||||
await expect(
|
||||
internals(client).rawWireRequest(
|
||||
"tasks/get",
|
||||
{},
|
||||
ModernGetTaskResultSchema,
|
||||
),
|
||||
).rejects.toMatchObject({
|
||||
code: SdkErrorCode.RequestTimeout,
|
||||
message: expect.stringMatching(
|
||||
/^Request timed out after 1m00s \(tasks\/get\)\. /,
|
||||
),
|
||||
data: expect.objectContaining({ timeout: 60_000, method: "tasks/get" }),
|
||||
});
|
||||
});
|
||||
|
||||
it("leaves a non-timeout send failure untouched", async () => {
|
||||
const client = makeClient();
|
||||
const boom = new Error("connection closed");
|
||||
internals(client).transport = { send: vi.fn().mockRejectedValue(boom) };
|
||||
await expect(
|
||||
internals(client).rawWireRequest(
|
||||
"tasks/get",
|
||||
{},
|
||||
ModernGetTaskResultSchema,
|
||||
),
|
||||
).rejects.toBe(boom);
|
||||
});
|
||||
|
||||
it("rejects all pending requests on teardown", async () => {
|
||||
const client = makeClient();
|
||||
internals(client).transport = {
|
||||
|
|
|
|||
|
|
@ -375,6 +375,63 @@ describe("RemoteClientTransport (focused branch coverage)", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("reports a still-open stream closed on close(), since the backend's own close cannot arrive (#2318)", async () => {
|
||||
const onFetchStreamUpdate = vi.fn();
|
||||
const { response, push } = createPushableEventStream();
|
||||
const t = makeTransport(
|
||||
{ events: () => response },
|
||||
{ onFetchStreamUpdate, onFetchRequest: vi.fn() },
|
||||
);
|
||||
await t.start();
|
||||
const entry = (id: string, method: string, contentType: string) =>
|
||||
({
|
||||
id,
|
||||
method,
|
||||
url: "http://x/mcp",
|
||||
timestamp: new Date().toISOString(),
|
||||
requestHeaders: {},
|
||||
responseStatus: 200,
|
||||
responseHeaders: { "Content-Type": contentType },
|
||||
}) as never;
|
||||
push({
|
||||
type: "fetch_request",
|
||||
data: entry("stream", "GET", "text/event-stream"),
|
||||
});
|
||||
push({
|
||||
type: "fetch_request",
|
||||
data: entry("post", "POST", "text/event-stream"),
|
||||
});
|
||||
push({
|
||||
type: "fetch_stream_update",
|
||||
data: { id: "stream", eventCount: 2 },
|
||||
});
|
||||
push({
|
||||
type: "fetch_request",
|
||||
data: entry("earlier", "GET", "text/event-stream"),
|
||||
});
|
||||
push({
|
||||
type: "fetch_stream_update",
|
||||
data: {
|
||||
id: "earlier",
|
||||
eventCount: 1,
|
||||
closedAt: new Date().toISOString(),
|
||||
},
|
||||
});
|
||||
await tick();
|
||||
onFetchStreamUpdate.mockClear();
|
||||
await t.close();
|
||||
// Only the stream still open is reported closed, with its last count;
|
||||
// the bounded POST and the already-closed stream are not.
|
||||
expect(onFetchStreamUpdate).toHaveBeenCalledTimes(1);
|
||||
expect(onFetchStreamUpdate).toHaveBeenCalledWith("stream", {
|
||||
eventCount: 2,
|
||||
closedAt: expect.any(Date),
|
||||
});
|
||||
// A second close reports nothing more.
|
||||
await t.close();
|
||||
expect(onFetchStreamUpdate).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("drops fetch_stream_update events when no handler is registered", async () => {
|
||||
const onFetchRequest = vi.fn();
|
||||
const t = makeTransport(
|
||||
|
|
|
|||
Loading…
Reference in a new issue