fix: address Copilot review round 1 on #2318
- fetchTracking: count NDJSON streams line by line instead of leaving them at zero forever; justify the detached watcher promise; document that a partial SSE block at end-of-stream is dropped, as the SDK's own parser (no flush) and the SSE spec drop it. - uncapturedBodyNote: case-insensitive content-type lookup, and a stream label (SSE / NDJSON) for the Network badge. - InspectorClient: `openedAt` is when the response headers arrived, not when the GET went out; the snapshot copies its request records; a request the transport failed to send is rolled back out of the unanswered set via a new `trackSendFailure` callback; a raw-wire timeout is built as the SDK's own timeout shape and annotated the same way, so callers see one kind of timeout. - Remote session storage revives the nested `stream.closedAt` Date. - Hook test models a legacy double with `Object.defineProperty`, not a double cast. 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
af7beab488
commit
1a5dd6820d
4 changed files with 267 additions and 20 deletions
|
|
@ -251,15 +251,32 @@ export interface FetchTrackingCallbacks {
|
|||
}
|
||||
|
||||
/**
|
||||
* Count the SSE events on a long-lived stream without buffering it, and
|
||||
* report when it ends.
|
||||
* The framing a long-lived stream's events arrive in — the two content types
|
||||
* `isLongLivedStreamResponse` accepts. SSE dispatches on a blank line;
|
||||
* NDJSON dispatches on every line.
|
||||
*/
|
||||
export type LongLivedStreamFraming = "sse" | "ndjson";
|
||||
|
||||
/** The framing for a long-lived stream's content type. */
|
||||
export function longLivedStreamFraming(
|
||||
contentType: string | null | undefined,
|
||||
): LongLivedStreamFraming {
|
||||
return contentType?.includes("application/x-ndjson") ? "ndjson" : "sse";
|
||||
}
|
||||
|
||||
/**
|
||||
* Count the events on a long-lived stream without buffering it, and report
|
||||
* when it ends.
|
||||
*
|
||||
* Reads a `clone()` of the response through a reader and discards the bytes,
|
||||
* so the transport keeps consuming the original at its own pace (the same
|
||||
* tee the bounded-body path relies on). Counting follows the SSE framing the
|
||||
* transport's own parser applies: a blank line dispatches the block before
|
||||
* it, and only a block carrying a `data` field is an event — keepalive
|
||||
* comments (`: ping`) and bare `event:`/`id:` lines are not.
|
||||
* tee the bounded-body path relies on). Counting follows the framing the
|
||||
* transport's own parser applies. For SSE a blank line dispatches the block
|
||||
* before it, and only a block carrying a `data` field is an event —
|
||||
* keepalive comments (`: ping`) and bare `event:`/`id:` lines are not; a
|
||||
* partial block at end-of-stream is dropped, exactly as the SSE spec and the
|
||||
* SDK's `EventSourceParserStream` (which has no flush) drop it. For NDJSON
|
||||
* every non-blank line is one event.
|
||||
*
|
||||
* Reports once as soon as watching starts (zero events, open), once per event,
|
||||
* and once more on every exit from the read loop — a clean end-of-stream, a
|
||||
|
|
@ -273,6 +290,7 @@ export interface FetchTrackingCallbacks {
|
|||
function watchLongLivedStream(
|
||||
response: Response,
|
||||
id: string,
|
||||
framing: LongLivedStreamFraming,
|
||||
callbacks: FetchTrackingCallbacks,
|
||||
): void {
|
||||
const update = callbacks.updateStream;
|
||||
|
|
@ -298,6 +316,12 @@ function watchLongLivedStream(
|
|||
|
||||
const consumeLine = (rawLine: string): void => {
|
||||
const line = rawLine.endsWith("\r") ? rawLine.slice(0, -1) : rawLine;
|
||||
if (framing === "ndjson") {
|
||||
if (line.trim() === "") return;
|
||||
eventCount += 1;
|
||||
update(id, { eventCount });
|
||||
return;
|
||||
}
|
||||
if (line === "") {
|
||||
if (blockHasData) {
|
||||
eventCount += 1;
|
||||
|
|
@ -331,6 +355,10 @@ function watchLongLivedStream(
|
|||
}
|
||||
update(id, { eventCount, closedAt: new Date() });
|
||||
};
|
||||
// Deliberately not awaited: the fetch wrapper has to return the response
|
||||
// now, and this watcher lives as long as the stream does. It owns its own
|
||||
// failures — the read loop is wrapped in a `try` that treats an error as a
|
||||
// close — so the promise cannot reject.
|
||||
void pump();
|
||||
}
|
||||
|
||||
|
|
@ -481,7 +509,12 @@ export function createFetchTracker(
|
|||
// but its event count and its eventual close are exactly the state a
|
||||
// stalled connection is diagnosed from (#2318).
|
||||
if (isLongLivedStream && response.body && !response.bodyUsed) {
|
||||
watchLongLivedStream(response, id, callbacks);
|
||||
watchLongLivedStream(
|
||||
response,
|
||||
id,
|
||||
longLivedStreamFraming(response.headers.get("content-type")),
|
||||
callbacks,
|
||||
);
|
||||
}
|
||||
|
||||
// Kick off a fire-and-forget read of the cloned body. The clone is an
|
||||
|
|
|
|||
|
|
@ -270,6 +270,7 @@ import {
|
|||
findHeader,
|
||||
isLongLivedStreamResponse,
|
||||
} from "./fetchTracking.js";
|
||||
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
annotateRequestTimeout,
|
||||
type ConnectionDiagnostics,
|
||||
|
|
@ -1130,6 +1131,15 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
};
|
||||
this.dispatchTypedEvent("message", entry);
|
||||
},
|
||||
trackSendFailure: (message: JSONRPCRequest) => {
|
||||
// The frame never reached the wire, so it is not awaiting a response.
|
||||
// Leaving it would report a request the server never saw as
|
||||
// unanswered, in every timeout message and Connection Info row, until
|
||||
// the session resets (#2318).
|
||||
if (this.outstandingRequests.delete(message.id)) {
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
}
|
||||
},
|
||||
trackResponse: (
|
||||
message: JSONRPCResultResponse | JSONRPCErrorResponse,
|
||||
origin: MessageOrigin,
|
||||
|
|
@ -2915,8 +2925,19 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
const raw = await new Promise<unknown>((resolve, reject) => {
|
||||
const timer = setTimeout(() => {
|
||||
this.pendingRawWireRequests.delete(id);
|
||||
// The same error, with the same annotation, as an SDK request that
|
||||
// times out: this path bypasses `Protocol.request`, so it builds the
|
||||
// SDK's own timeout shape and runs it through the decorator's
|
||||
// annotation by hand — a raw-wire caller sees one kind of timeout,
|
||||
// not two (#2318).
|
||||
reject(
|
||||
new Error(`Raw request "${method}" timed out after ${timeoutMs} ms`),
|
||||
annotateRequestTimeout(
|
||||
new SdkError(SdkErrorCode.RequestTimeout, "Request timed out", {
|
||||
timeout: timeoutMs,
|
||||
}),
|
||||
method,
|
||||
this.getConnectionDiagnostics(),
|
||||
),
|
||||
);
|
||||
}, timeoutMs);
|
||||
this.pendingRawWireRequests.set(id, { resolve, reject, timer });
|
||||
|
|
@ -6083,7 +6104,9 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
this.notificationStream = {
|
||||
id: entry.id,
|
||||
url: entry.url,
|
||||
openedAt: entry.timestamp.getTime(),
|
||||
// The entry's `timestamp` is when the request went out; the stream is
|
||||
// open from when the response headers arrived, `duration` later.
|
||||
openedAt: entry.timestamp.getTime() + (entry.duration ?? 0),
|
||||
eventCount: 0,
|
||||
};
|
||||
this.dispatchConnectionDiagnosticsChange();
|
||||
|
|
@ -6098,7 +6121,12 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
getConnectionDiagnostics(): ConnectionDiagnostics {
|
||||
return {
|
||||
capturedAt: Date.now(),
|
||||
outstandingRequests: [...this.outstandingRequests.values()],
|
||||
// Copies, not the tracked records: the snapshot is handed to callers
|
||||
// and carried on errors, and a mutation there must not reach the
|
||||
// correlation state.
|
||||
outstandingRequests: [...this.outstandingRequests.values()].map(
|
||||
(request) => ({ ...request }),
|
||||
),
|
||||
...(this.lastResponse && { lastResponse: { ...this.lastResponse } }),
|
||||
...(this.notificationStream && {
|
||||
notificationStream: this.publicStreamState(this.notificationStream),
|
||||
|
|
|
|||
|
|
@ -24,6 +24,14 @@ export interface MessageTrackingCallbacks {
|
|||
message: JSONRPCNotification,
|
||||
origin: MessageOrigin,
|
||||
) => void;
|
||||
/**
|
||||
* An outgoing request the base transport failed to send (the connection
|
||||
* closed, the fetch failed before the frame reached the wire). Fires after
|
||||
* `trackRequest` already recorded it, so a consumer keeping "requests still
|
||||
* awaiting a response" can roll that entry back — a request that never went
|
||||
* out is not unanswered (#2318).
|
||||
*/
|
||||
trackSendFailure?: (message: JSONRPCRequest, error: unknown) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -95,7 +103,12 @@ export class MessageTrackingTransport implements Transport {
|
|||
"client",
|
||||
);
|
||||
} else if ("method" in message) {
|
||||
this.callbacks.trackRequest?.(message as JSONRPCRequest, "client");
|
||||
const request = message as JSONRPCRequest;
|
||||
this.callbacks.trackRequest?.(request, "client");
|
||||
return this.baseTransport.send(message, options).catch((err) => {
|
||||
this.callbacks.trackSendFailure?.(request, err);
|
||||
throw err;
|
||||
});
|
||||
}
|
||||
} else if ("method" in message) {
|
||||
this.callbacks.trackNotification?.(
|
||||
|
|
|
|||
|
|
@ -119,7 +119,10 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage {
|
|||
|
||||
const data = (await res.json()) as InspectorClientSessionState;
|
||||
|
||||
// Deserialize state (convert ISO strings back to Date objects)
|
||||
// Deserialize state (convert ISO strings back to Date objects). The
|
||||
// nested `stream.closedAt` is a Date too, and `JSON.stringify` turned it
|
||||
// into an ISO string on the way out; left as a string it would fail the
|
||||
// `getTime()` every consumer of the type performs (#2318).
|
||||
return {
|
||||
...data,
|
||||
fetchRequests: data.fetchRequests.map((req) => ({
|
||||
|
|
@ -130,6 +133,9 @@ export class RemoteInspectorClientStorage implements InspectorClientStorage {
|
|||
: req.timestamp instanceof Date
|
||||
? req.timestamp
|
||||
: new Date(req.timestamp),
|
||||
...(req.stream?.closedAt !== undefined && {
|
||||
stream: { ...req.stream, closedAt: new Date(req.stream.closedAt) },
|
||||
}),
|
||||
})),
|
||||
};
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue