* fix(core): trace dropped fetch body update when entry rotates out (#1390) FetchRequestLogState.onFetchRequestBodyUpdate silently dropped a deferred response-body update when the matching entry had already been evicted by log rotation (>= maxFetchRequests newer requests appended between the append and the fire-and-forget body read). The user saw an empty Response Body with no way to distinguish "body never came back" from "body dropped after eviction". Plumb an optional pino logger into FetchRequestLogState (defaulting to the silent logger) and emit a logger.debug at the early-return site so the drop is traceable. The web client wires the existing remote logger from createWebEnvironment into the state manager. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): emit dropped-body trace at warn level so it is observable (#1390) Review feedback: the web remote logger (createRemoteLogger) defaults to level 'info' with no override at the environmentFactory call site, so a logger.debug at the drop site is filtered out by both the pino level gate and transmit.level — the diagnostic never reaches /api/log and the drop stays invisible in the shipped web app, defeating the issue's goal. Promote the trace to logger.warn (>= info, so it transmits by default). A response body lost to log rotation is unexpected data loss, not routine diagnostic noise, so warn is the appropriate severity. Update the comment to explain the level choice and switch the test stub from debug to warn, extracting a small typed makeWarnLogger helper per review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(web): configurable Network Log Size per server + body-dropped toast (#1390) Turn the otherwise-invisible "response body dropped after rotation" case into an actionable signal: 1. Per-server `maxFetchRequests` setting (default 1000, 0 = unlimited), editable as "Network Log Size" in the first (Options) accordion section of the Server Settings form. Plumbed through the full settings pipeline: InspectorServerSettings + StoredMCPServer types, the serverList disk converters (read defaults to 1000, write omits the default to keep diffs minimal — 0 is a meaningful non-default), and the node server's GET-sanitize + write-side validateSettings (non-negative number). Applied live to the active connection on settings-modal close via a new FetchRequestLogState.setMaxFetchRequests (shrinking trims immediately), and at connect time for the next session. 2. FetchRequestLogState now emits a `fetchRequestBodyDropped` event, but only for genuine rotation drops (log at capacity with a finite cap) — not for benign post-clear stragglers or unlimited logs, which previously also hit the warn path. App shows a deduped, persistent toast (stable per-server id, no auto-close) explaining what happened and the likely causes, with a link that opens this server's settings on the Options section (modal remounts via key) so the user can raise the limit if it's just a high-traffic server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(web): don't treat a cleared Network Log Size field as unlimited (#1390) Review feedback: clearing the NumberInput emits "", and `parseInt("") || 0` coerced that to 0 (unlimited). Since the value applies live on modal close, a user who cleared the field to retype and then closed the modal would silently switch the log to unlimited. Fall back to the current value on an empty/NaN parse and reserve 0 for an explicit numeric entry. Adds a cleared-input test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
ef921ba052
commit
53dc4549be
4 changed files with 717 additions and 13 deletions
|
|
@ -25,7 +25,7 @@ import { streamSSE } from "hono/streaming";
|
|||
import { watch as chokidarWatch, type FSWatcher } from "chokidar";
|
||||
import { createTransportNode } from "../../node/transport.js";
|
||||
import type { RemoteConnectRequest, RemoteSendRequest } from "../types.js";
|
||||
import { DEFAULT_TASK_TTL_MS } from "../../types.js";
|
||||
import { DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS } from "../../types.js";
|
||||
import type {
|
||||
InspectorServerSettings,
|
||||
MCPConfig,
|
||||
|
|
@ -962,6 +962,16 @@ export function createRemoteApp(
|
|||
);
|
||||
delete valObj.taskTtl;
|
||||
}
|
||||
if (
|
||||
"maxFetchRequests" in valObj &&
|
||||
!isNonNegNumber(valObj.maxFetchRequests)
|
||||
) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: "maxFetchRequests" },
|
||||
"Dropping malformed `maxFetchRequests` field — expected non-negative number.",
|
||||
);
|
||||
delete valObj.maxFetchRequests;
|
||||
}
|
||||
if ("oauth" in valObj && !isOauthObject(valObj.oauth)) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: "oauth" },
|
||||
|
|
@ -1120,6 +1130,18 @@ export function createRemoteApp(
|
|||
error: "settings.autoRefreshOnListChanged must be a boolean",
|
||||
};
|
||||
}
|
||||
// maxFetchRequests is optional on the wire (older clients won't send it);
|
||||
// when present it must be a non-negative number (0 = unlimited), otherwise
|
||||
// it defaults below.
|
||||
if (
|
||||
obj.maxFetchRequests !== undefined &&
|
||||
(typeof obj.maxFetchRequests !== "number" || obj.maxFetchRequests < 0)
|
||||
) {
|
||||
return {
|
||||
ok: false,
|
||||
error: "settings.maxFetchRequests must be a non-negative number",
|
||||
};
|
||||
}
|
||||
for (const optional of [
|
||||
"oauthClientId",
|
||||
"oauthClientSecret",
|
||||
|
|
@ -1160,6 +1182,13 @@ export function createRemoteApp(
|
|||
// Absent → false, matching the read side. The omit-on-false logic lives
|
||||
// in inspectorSettingsToStoredFields, so a false value writes nothing.
|
||||
autoRefreshOnListChanged: obj.autoRefreshOnListChanged === true,
|
||||
// Absent → product default, matching the read side. The default is the
|
||||
// omit-sentinel in inspectorSettingsToStoredFields, so a client that
|
||||
// didn't send one writes no spurious maxFetchRequests to disk.
|
||||
maxFetchRequests:
|
||||
typeof obj.maxFetchRequests === "number"
|
||||
? obj.maxFetchRequests
|
||||
: DEFAULT_MAX_FETCH_REQUESTS,
|
||||
// Absent → empty list, matching the read side
|
||||
// (storedFieldsToInspectorSettings). Empty rows are dropped on the way
|
||||
// to disk by inspectorSettingsToStoredFields, so an empty array here
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@
|
|||
* remote-server route handlers.
|
||||
*/
|
||||
|
||||
import { DEFAULT_TASK_TTL_MS } from "./types.js";
|
||||
import { DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS } from "./types.js";
|
||||
import type { Root } from "@modelcontextprotocol/sdk/types.js";
|
||||
import type {
|
||||
InspectorServerSettings,
|
||||
|
|
@ -93,6 +93,7 @@ type StoredInspectorFields = Pick<
|
|||
| "requestTimeout"
|
||||
| "taskTtl"
|
||||
| "autoRefreshOnListChanged"
|
||||
| "maxFetchRequests"
|
||||
| "oauth"
|
||||
| "roots"
|
||||
>;
|
||||
|
|
@ -119,6 +120,7 @@ export function storedFieldsToInspectorSettings(
|
|||
stored.requestTimeout !== undefined ||
|
||||
stored.taskTtl !== undefined ||
|
||||
stored.autoRefreshOnListChanged !== undefined ||
|
||||
stored.maxFetchRequests !== undefined ||
|
||||
stored.oauth !== undefined ||
|
||||
stored.roots !== undefined;
|
||||
if (!hasAny) return undefined;
|
||||
|
|
@ -136,6 +138,10 @@ export function storedFieldsToInspectorSettings(
|
|||
// default so the form shows it and "Run as task" has a value to send.
|
||||
taskTtl: stored.taskTtl ?? DEFAULT_TASK_TTL_MS,
|
||||
autoRefreshOnListChanged: stored.autoRefreshOnListChanged ?? false,
|
||||
// Concrete default like taskTtl (not a 0-sentinel): the form needs a value
|
||||
// to render and the log state needs one to size its buffer. An absent
|
||||
// on-disk field reads back as the default, which the write side then omits.
|
||||
maxFetchRequests: stored.maxFetchRequests ?? DEFAULT_MAX_FETCH_REQUESTS,
|
||||
// Defaults to an empty list so the form always has a concrete array to
|
||||
// render controlled rows from. An absent on-disk `roots` reads back as
|
||||
// `[]`, which `inspectorSettingsToStoredFields` then omits on write.
|
||||
|
|
@ -203,6 +209,14 @@ export function inspectorSettingsToStoredFields(
|
|||
out.autoRefreshOnListChanged = true;
|
||||
}
|
||||
|
||||
// Persist only when it differs from the default. Unlike the timeouts, 0 is a
|
||||
// meaningful value here (unlimited), so the omit-sentinel is the default
|
||||
// itself rather than 0 — writing the default would inject the field into
|
||||
// hand-edited files that never had it and break byte-stable round-trips.
|
||||
if (settings.maxFetchRequests !== DEFAULT_MAX_FETCH_REQUESTS) {
|
||||
out.maxFetchRequests = settings.maxFetchRequests;
|
||||
}
|
||||
|
||||
const oauthFields: {
|
||||
clientId?: string;
|
||||
clientSecret?: string;
|
||||
|
|
@ -248,6 +262,7 @@ const INSPECTOR_FIELD_KEY_MAP = {
|
|||
requestTimeout: true,
|
||||
taskTtl: true,
|
||||
autoRefreshOnListChanged: true,
|
||||
maxFetchRequests: true,
|
||||
oauth: true,
|
||||
roots: true,
|
||||
} as const satisfies Record<keyof StoredInspectorFields, true>;
|
||||
|
|
|
|||
|
|
@ -13,6 +13,8 @@
|
|||
* concrete `InspectorClient` since the runtime class is not yet ported.
|
||||
*/
|
||||
|
||||
import type pino from "pino";
|
||||
import { silentLogger } from "../../logging/logger.js";
|
||||
import type { InspectorClientProtocol } from "../inspectorClientProtocol.js";
|
||||
import type { FetchRequestEntry } from "../types.js";
|
||||
import type {
|
||||
|
|
@ -25,9 +27,22 @@ import {
|
|||
type TypedEventGeneric,
|
||||
} from "../typedEventTarget.js";
|
||||
|
||||
/**
|
||||
* Detail for `fetchRequestBodyDropped`: a deferred response body arrived after
|
||||
* its log entry had already rotated out (>= `maxFetchRequests` newer requests
|
||||
* appended in between), so the body was unrecoverable. Carries the dropped
|
||||
* request id and the cap that was in force, so the UI can explain the drop and
|
||||
* offer to raise the limit.
|
||||
*/
|
||||
export interface FetchRequestBodyDropped {
|
||||
id: string;
|
||||
maxFetchRequests: number;
|
||||
}
|
||||
|
||||
export interface FetchRequestLogStateEventMap {
|
||||
fetchRequest: FetchRequestEntry;
|
||||
fetchRequestsChange: FetchRequestEntry[];
|
||||
fetchRequestBodyDropped: FetchRequestBodyDropped;
|
||||
}
|
||||
|
||||
export interface FetchRequestLogStateOptions {
|
||||
|
|
@ -44,13 +59,19 @@ export interface FetchRequestLogStateOptions {
|
|||
sessionStorage?: InspectorClientStorage;
|
||||
/** Session ID for load/save; required for sessionStorage to have effect. */
|
||||
sessionId?: string;
|
||||
/**
|
||||
* Logger for diagnostic traces (e.g. a deferred body update arriving after
|
||||
* its entry rotated out of the log). Defaults to the silent logger.
|
||||
*/
|
||||
logger?: pino.Logger;
|
||||
}
|
||||
|
||||
export class FetchRequestLogState extends TypedEventTarget<FetchRequestLogStateEventMap> {
|
||||
private fetchRequests: FetchRequestEntry[] = [];
|
||||
private client: InspectorClientProtocol | null = null;
|
||||
private unsubscribe: (() => void) | null = null;
|
||||
private readonly maxFetchRequests: number;
|
||||
private maxFetchRequests: number;
|
||||
private readonly logger: pino.Logger;
|
||||
|
||||
constructor(
|
||||
client: InspectorClientProtocol,
|
||||
|
|
@ -58,6 +79,7 @@ export class FetchRequestLogState extends TypedEventTarget<FetchRequestLogStateE
|
|||
) {
|
||||
super();
|
||||
this.maxFetchRequests = options.maxFetchRequests ?? 1000;
|
||||
this.logger = options.logger ?? silentLogger;
|
||||
this.client = client;
|
||||
|
||||
const onFetchRequest = (
|
||||
|
|
@ -87,7 +109,33 @@ export class FetchRequestLogState extends TypedEventTarget<FetchRequestLogStateE
|
|||
): void => {
|
||||
const { id, responseBody } = event.detail;
|
||||
const idx = this.fetchRequests.findIndex((e) => e.id === id);
|
||||
if (idx === -1) return;
|
||||
if (idx === -1) {
|
||||
// The deferred body read (fire-and-forget tee'd stream in
|
||||
// fetchTracking) found no matching entry. Distinguish the two causes:
|
||||
// a *rotation* drop — the entry was evicted because the log is at
|
||||
// capacity (>= maxFetchRequests, with a finite cap) — versus a benign
|
||||
// straggler whose entry was simply cleared (length below the cap, or
|
||||
// an unlimited log where rotation can't happen). Only the rotation
|
||||
// case is unexpected data loss worth surfacing; a post-clear straggler
|
||||
// is expected and stays silent.
|
||||
const rotatedOut =
|
||||
this.maxFetchRequests > 0 &&
|
||||
this.fetchRequests.length >= this.maxFetchRequests;
|
||||
if (rotatedOut) {
|
||||
// `warn` (not `debug`) so the trace clears the web remote logger's
|
||||
// default `info` level and is observable without opt-in.
|
||||
this.logger.warn(
|
||||
{ fetchRequestId: id, maxFetchRequests: this.maxFetchRequests },
|
||||
"fetchRequestBodyUpdate dropped: entry rotated out before body arrived",
|
||||
);
|
||||
// Let the UI surface the drop (toast) and offer to raise the cap.
|
||||
this.dispatchTypedEvent("fetchRequestBodyDropped", {
|
||||
id,
|
||||
maxFetchRequests: this.maxFetchRequests,
|
||||
});
|
||||
}
|
||||
return;
|
||||
}
|
||||
this.fetchRequests[idx] = {
|
||||
...this.fetchRequests[idx]!,
|
||||
responseBody,
|
||||
|
|
@ -202,6 +250,21 @@ export class FetchRequestLogState extends TypedEventTarget<FetchRequestLogStateE
|
|||
return [...this.fetchRequests];
|
||||
}
|
||||
|
||||
/**
|
||||
* Adjust the retention cap live (e.g. when the user edits the per-server
|
||||
* `maxFetchRequests` setting) without reconnecting. Shrinking the cap trims
|
||||
* the oldest entries immediately and re-emits so the Network UI updates;
|
||||
* growing it (or setting 0 = unlimited) just takes effect for future appends.
|
||||
*/
|
||||
setMaxFetchRequests(max: number): void {
|
||||
if (max === this.maxFetchRequests) return;
|
||||
this.maxFetchRequests = max;
|
||||
if (max > 0 && this.fetchRequests.length > max) {
|
||||
this.fetchRequests = this.fetchRequests.slice(-max);
|
||||
this.dispatchTypedEvent("fetchRequestsChange", this.getFetchRequests());
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Clear all fetch requests. Dispatches fetchRequestsChange only if the list
|
||||
* was non-empty.
|
||||
|
|
|
|||
|
|
@ -111,6 +111,13 @@ export type StoredMCPServer = MCPServerConfig & {
|
|||
* false (the default). (#1402)
|
||||
*/
|
||||
autoRefreshOnListChanged?: boolean;
|
||||
/**
|
||||
* Maximum number of HTTP fetch requests retained in the Network log for this
|
||||
* server (oldest rotate out past the cap). Inspector-specific. Omitted on
|
||||
* disk when it equals `DEFAULT_MAX_FETCH_REQUESTS` (the default), keeping the
|
||||
* file diff minimal for servers that never tuned it. `0` means unlimited.
|
||||
*/
|
||||
maxFetchRequests?: number;
|
||||
/**
|
||||
* Pre-configured OAuth client credentials for HTTP transports. Nested to
|
||||
* match Claude Code's `.mcp.json` shape; lifted into the flat `oauthClientId`
|
||||
|
|
@ -381,6 +388,15 @@ export interface OAuthSettings {
|
|||
*/
|
||||
export const DEFAULT_TASK_TTL_MS = 60000;
|
||||
|
||||
/**
|
||||
* Default maximum number of HTTP fetch requests retained in the Network log
|
||||
* (per server). When exceeded, the oldest entries rotate out. A larger value
|
||||
* keeps more history at the cost of memory; `0` means unlimited (not
|
||||
* recommended). Mirrors `FetchRequestLogState`'s built-in default so the form
|
||||
* and the log state agree on the omit-sentinel.
|
||||
*/
|
||||
export const DEFAULT_MAX_FETCH_REQUESTS = 1000;
|
||||
|
||||
/**
|
||||
* Runtime settings for a configured server. A subset of
|
||||
* InspectorClientOptions (v1.5) relevant to the settings form:
|
||||
|
|
@ -402,6 +418,14 @@ export interface InspectorServerSettings {
|
|||
* and the user pulls the new list via Refresh. (#1402)
|
||||
*/
|
||||
autoRefreshOnListChanged?: boolean;
|
||||
/**
|
||||
* Maximum number of HTTP fetch requests retained in the Network log for this
|
||||
* server. When exceeded, the oldest entries rotate out (and any deferred
|
||||
* response body that arrives after its entry rotated out is dropped — see
|
||||
* `FetchRequestLogState`). Concrete value so the form always has something to
|
||||
* render; defaults to `DEFAULT_MAX_FETCH_REQUESTS`. `0` means unlimited.
|
||||
*/
|
||||
maxFetchRequests: number;
|
||||
/**
|
||||
* Roots advertised to the server via the `roots` client capability. Each
|
||||
* root carries a required `uri` and an optional `name` (SDK `Root`). The
|
||||
|
|
|
|||
Loading…
Reference in a new issue