* feat(servers): hot-reload server list on external mcp.json edits (#1345) Watch ~/.mcp-inspector/mcp.json from the backend and push a change event on a new /api/servers/events SSE channel. The web client subscribes via fetch + ReadableStream (so the existing bearer-auth contract is preserved — EventSource can't send custom headers) and re-fetches on each event. The watcher is lazy: it starts on the first SSE subscriber and stops when the last one disconnects, so tests that never open the channel don't spin up a real fs watcher. Self-fires from the backend's own POST/PUT/DELETE are suppressed by capturing the post-write mtime and comparing it on each watcher event. Chokidar is used for cross-platform editor-save semantics (temp-file + rename produces unlink+add sequences that bare fs.watch handles poorly on macOS/Linux); awaitWriteFinish coalesces those into a single event. createRemoteApp now returns a close() to release the watcher, which the standalone server and the vite dev plugin chain into their existing close paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review on hot-reload (#1357) - writeMcpAndTrackMtime now stat()s the file before writing; if the mtime differs from the last value we recorded, an external editor slipped in between our writes and chokidar would coalesce both into one event whose mtime matches our own write — broadcast directly so peer subscribers learn about the external edit (review finding 1, 5). - Split useServers.refresh into a public refresh() (loading toggle) and an internal refreshInternal(background) the SSE handler calls; the list now stays visible during background refreshes instead of flashing skeleton state on every external mcp.json edit (finding 2). - Coalesce multiple \n\n frames landing in one decode chunk to a single refresh call so back-to-back broadcasts don't race two concurrent GETs whose setState order is unspecified (finding 3). - Clarify createRemoteApp's close() docblock about in-flight SSE stream callbacks (finding 7). - Add integration coverage for PUT/DELETE self-suppression and for the external-edit-between-backend-writes broadcast (finding 6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address pass-2 review on hot-reload (#1357) - Clarify writeMcpAndTrackMtime comment so a future reader can't mistake the broadcast for a content-preservation guarantee: the external edit's content may already have been read-around and overwritten depending on the interleave with readMcpConfig. - Drop the no-op GET in the PUT/DELETE self-suppression tests. The pre-stat branch is short-circuited by `lastWrittenMtimeMs === null` with or without it; matches the POST test's structure. - Add explicit no-flicker regression test: tracks every `loading` value across renders during an SSE-driven refresh and asserts none flipped back to true. Catches a future regression that reaches for setLoading(true) in the background path even under React batching. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
85a4b4c057
commit
5f9bfdc490
2 changed files with 823 additions and 38 deletions
|
|
@ -19,12 +19,13 @@ interface Harness {
|
|||
fetchFn: typeof fetch;
|
||||
configPath: string;
|
||||
tempDir: string;
|
||||
closeApi: () => Promise<void>;
|
||||
}
|
||||
|
||||
function setupHarness(): Harness {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), "inspector-useServers-"));
|
||||
const configPath = join(tempDir, "mcp.json");
|
||||
const { app } = createRemoteApp({
|
||||
const { app, close: closeApi } = createRemoteApp({
|
||||
dangerouslyOmitAuth: true,
|
||||
mcpConfigPath: configPath,
|
||||
initialConfig: { defaultEnvironment: {} },
|
||||
|
|
@ -38,10 +39,16 @@ function setupHarness(): Harness {
|
|||
: new Request(input as string | URL, init);
|
||||
return app.fetch(req) as Promise<Response>;
|
||||
};
|
||||
return { fetchFn, configPath, tempDir };
|
||||
return { fetchFn, configPath, tempDir, closeApi };
|
||||
}
|
||||
|
||||
function teardownHarness(h: Harness): void {
|
||||
async function teardownHarness(h: Harness): Promise<void> {
|
||||
// Closing here releases the lazy chokidar watcher started by the SSE
|
||||
// subscription useServers opens on mount. Without it the watcher would
|
||||
// hang around for the lifetime of the vitest worker — harmless for the
|
||||
// suite as a whole, but it would slow worker exit and could leak inotify
|
||||
// watches on Linux.
|
||||
await h.closeApi();
|
||||
try {
|
||||
rmSync(h.tempDir, { recursive: true });
|
||||
} catch {
|
||||
|
|
@ -60,8 +67,8 @@ describe("useServers", () => {
|
|||
h = setupHarness();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
teardownHarness(h);
|
||||
afterEach(async () => {
|
||||
await teardownHarness(h);
|
||||
});
|
||||
|
||||
it("starts in loading state, then loads and converts the seed config", async () => {
|
||||
|
|
@ -218,6 +225,83 @@ describe("useServers", () => {
|
|||
expect(result.current.servers.map((s) => s.id)).toEqual(["hand"]);
|
||||
});
|
||||
|
||||
it("auto-refreshes when the /api/servers/events SSE channel signals a change", async () => {
|
||||
// Mount the hook (which also opens the SSE subscription) and let the
|
||||
// initial GET settle so we're observing a steady state before mutating
|
||||
// the file.
|
||||
const { result } = renderHook(() =>
|
||||
useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }),
|
||||
);
|
||||
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||
|
||||
// Give chokidar a beat to register with the now-seeded file. The lazy
|
||||
// watcher inside createRemoteApp starts on the first SSE subscriber, so
|
||||
// by the time the initial GET resolves the watcher is already attached.
|
||||
// The pause covers any platform-specific scan-stabilization window
|
||||
// before we mutate.
|
||||
await new Promise((r) => setTimeout(r, 200));
|
||||
|
||||
// Simulate an external editor save. The watcher fires → SSE broadcasts
|
||||
// → the hook's reader-loop calls refresh() without us touching the
|
||||
// exposed refresh() API.
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({
|
||||
mcpServers: { external: { type: "stdio", command: "outside" } },
|
||||
}) + "\n",
|
||||
);
|
||||
|
||||
await waitFor(
|
||||
() => {
|
||||
expect(result.current.servers.map((s) => s.id)).toEqual(["external"]);
|
||||
},
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
});
|
||||
|
||||
it("SSE-driven refresh does not flip loading back to true (no skeleton flicker)", async () => {
|
||||
// Regression guard for the background-refresh split: a consumer
|
||||
// rendering a loading spinner / skeleton must not see `loading` go
|
||||
// true on every external mcp.json edit. The hook's SSE handler calls
|
||||
// refreshInternal(true), which skips the setLoading toggles entirely;
|
||||
// sampling a single point can miss the bug under React batching, so
|
||||
// record every observed `loading` value across the SSE cycle and
|
||||
// assert none of them is true.
|
||||
const loadingHistory: boolean[] = [];
|
||||
const { result } = renderHook(() => {
|
||||
const r = useServers({
|
||||
baseUrl: "http://test.local",
|
||||
fetchFn: h.fetchFn,
|
||||
});
|
||||
loadingHistory.push(r.loading);
|
||||
return r;
|
||||
});
|
||||
|
||||
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||
// Slice off the mount-phase renders; only renders after the initial
|
||||
// load are subject to the no-flicker contract.
|
||||
const baselineLen = loadingHistory.length;
|
||||
|
||||
await new Promise((r) => setTimeout(r, 200));
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({
|
||||
mcpServers: { flicker: { type: "stdio", command: "outside" } },
|
||||
}) + "\n",
|
||||
);
|
||||
|
||||
await waitFor(
|
||||
() => {
|
||||
expect(result.current.servers.map((s) => s.id)).toEqual(["flicker"]);
|
||||
},
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
|
||||
const postRefreshLoadings = loadingHistory.slice(baselineLen);
|
||||
expect(postRefreshLoadings.length).toBeGreaterThan(0);
|
||||
expect(postRefreshLoadings.every((v) => v === false)).toBe(true);
|
||||
});
|
||||
|
||||
it("captures the error message on fetch network failure", async () => {
|
||||
const failingFetch = vi
|
||||
.fn<typeof fetch>()
|
||||
|
|
|
|||
|
|
@ -0,0 +1,383 @@
|
|||
/**
|
||||
* Integration tests for GET /api/servers/events — the SSE channel that
|
||||
* notifies subscribed browsers when `mcp.json` changes on disk. Covers:
|
||||
*
|
||||
* - external write to the file triggers a broadcast
|
||||
* - external unlink triggers a broadcast
|
||||
* - the backend's own POST/PUT/DELETE does NOT trigger a broadcast
|
||||
* (self-write suppression via the post-write mtime capture)
|
||||
* - multiple connected subscribers each receive the broadcast
|
||||
*
|
||||
* Tests spin up a real TCP server (so the SSE response body flows through
|
||||
* @hono/node-server rather than the in-process Hono fetch) and read the
|
||||
* stream with `fetch().body.getReader()`. Each harness calls `closeApi()`
|
||||
* in teardown so the lazy chokidar watcher is released.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, afterEach, beforeEach } from "vitest";
|
||||
import {
|
||||
mkdtempSync,
|
||||
rmSync,
|
||||
writeFileSync,
|
||||
unlinkSync,
|
||||
existsSync,
|
||||
} from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { serve } from "@hono/node-server";
|
||||
import type { ServerType } from "@hono/node-server";
|
||||
import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js";
|
||||
|
||||
interface Harness {
|
||||
baseUrl: string;
|
||||
server: ServerType;
|
||||
configPath: string;
|
||||
tempDir: string;
|
||||
closeApi: () => Promise<void>;
|
||||
}
|
||||
|
||||
async function setup(): Promise<Harness> {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), "inspector-servers-events-"));
|
||||
const configPath = join(tempDir, "mcp.json");
|
||||
const { app, close: closeApi } = createRemoteApp({
|
||||
dangerouslyOmitAuth: true,
|
||||
mcpConfigPath: configPath,
|
||||
initialConfig: { defaultEnvironment: {} },
|
||||
});
|
||||
const { baseUrl, server } = await new Promise<{
|
||||
baseUrl: string;
|
||||
server: ServerType;
|
||||
}>((resolve, reject) => {
|
||||
const s = serve(
|
||||
{ fetch: app.fetch, port: 0, hostname: "127.0.0.1" },
|
||||
(info) => {
|
||||
const port =
|
||||
info && typeof info === "object" && "port" in info
|
||||
? (info as { port: number }).port
|
||||
: 0;
|
||||
resolve({ baseUrl: `http://127.0.0.1:${port}`, server: s });
|
||||
},
|
||||
);
|
||||
s.on("error", reject);
|
||||
});
|
||||
return { baseUrl, server, configPath, tempDir, closeApi };
|
||||
}
|
||||
|
||||
async function teardown(h: Harness): Promise<void> {
|
||||
await h.closeApi();
|
||||
await new Promise<void>((resolve) => h.server.close(() => resolve()));
|
||||
try {
|
||||
rmSync(h.tempDir, { recursive: true });
|
||||
} catch {
|
||||
/* ignore */
|
||||
}
|
||||
}
|
||||
|
||||
interface EventSink {
|
||||
readonly events: string[];
|
||||
/** Resolve once `events.length >= n` (or reject after `timeoutMs`). */
|
||||
waitFor(n: number, timeoutMs?: number): Promise<void>;
|
||||
close(): void;
|
||||
}
|
||||
|
||||
/**
|
||||
* Open an SSE subscription and parse out each `data:` payload into a flat
|
||||
* string array. Resolves once the response head is received so the caller can
|
||||
* mutate the file knowing the subscriber is registered server-side.
|
||||
*/
|
||||
async function subscribe(baseUrl: string): Promise<EventSink> {
|
||||
const controller = new AbortController();
|
||||
const res = await fetch(`${baseUrl}/api/servers/events`, {
|
||||
signal: controller.signal,
|
||||
});
|
||||
if (!res.ok || !res.body) {
|
||||
throw new Error(`SSE subscribe failed: ${res.status}`);
|
||||
}
|
||||
|
||||
const events: string[] = [];
|
||||
const waiters: { n: number; resolve: () => void }[] = [];
|
||||
|
||||
const notifyWaiters = (): void => {
|
||||
for (const w of [...waiters]) {
|
||||
if (events.length >= w.n) {
|
||||
waiters.splice(waiters.indexOf(w), 1);
|
||||
w.resolve();
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
const reader = res.body.getReader();
|
||||
const decoder = new TextDecoder();
|
||||
let buffer = "";
|
||||
|
||||
void (async () => {
|
||||
try {
|
||||
while (true) {
|
||||
const { done, value } = await reader.read();
|
||||
if (done) return;
|
||||
buffer += decoder.decode(value, { stream: true });
|
||||
let frameEnd = buffer.indexOf("\n\n");
|
||||
while (frameEnd !== -1) {
|
||||
const frame = buffer.slice(0, frameEnd);
|
||||
buffer = buffer.slice(frameEnd + 2);
|
||||
const dataLine = frame.split("\n").find((l) => l.startsWith("data:"));
|
||||
if (dataLine) {
|
||||
events.push(dataLine.slice("data:".length).trim());
|
||||
notifyWaiters();
|
||||
}
|
||||
frameEnd = buffer.indexOf("\n\n");
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
// Aborted by caller in close(); benign.
|
||||
}
|
||||
})();
|
||||
|
||||
return {
|
||||
events,
|
||||
async waitFor(n: number, timeoutMs = 2000): Promise<void> {
|
||||
if (events.length >= n) return;
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
const timer = setTimeout(() => {
|
||||
reject(
|
||||
new Error(
|
||||
`Timed out waiting for ${n} event(s); saw ${events.length}`,
|
||||
),
|
||||
);
|
||||
}, timeoutMs);
|
||||
waiters.push({
|
||||
n,
|
||||
resolve: () => {
|
||||
clearTimeout(timer);
|
||||
resolve();
|
||||
},
|
||||
});
|
||||
});
|
||||
},
|
||||
close(): void {
|
||||
controller.abort();
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
describe("GET /api/servers/events", () => {
|
||||
let h: Harness;
|
||||
|
||||
beforeEach(async () => {
|
||||
h = await setup();
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await teardown(h);
|
||||
});
|
||||
|
||||
it("emits an event when an external editor writes the config file", async () => {
|
||||
// Seed the file so chokidar starts watching an existing path. The
|
||||
// subscribe() round-trip below ensures the watcher is registered before
|
||||
// we mutate the file.
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({ mcpServers: {} }, null, 2) + "\n",
|
||||
);
|
||||
const sink = await subscribe(h.baseUrl);
|
||||
|
||||
// Small gap so chokidar's initial-scan settles before we mutate.
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify(
|
||||
{
|
||||
mcpServers: { alpha: { type: "stdio", command: "node" } },
|
||||
},
|
||||
null,
|
||||
2,
|
||||
) + "\n",
|
||||
);
|
||||
|
||||
await sink.waitFor(1);
|
||||
expect(sink.events).toHaveLength(1);
|
||||
expect(JSON.parse(sink.events[0]!)).toEqual({ type: "change" });
|
||||
sink.close();
|
||||
});
|
||||
|
||||
it("does NOT emit when the backend's own POST /api/servers writes the file", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({ mcpServers: {} }, null, 2) + "\n",
|
||||
);
|
||||
const sink = await subscribe(h.baseUrl);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
const postRes = await fetch(`${h.baseUrl}/api/servers`, {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
id: "alpha",
|
||||
config: { type: "stdio", command: "node" },
|
||||
}),
|
||||
});
|
||||
expect(postRes.status).toBe(200);
|
||||
|
||||
// awaitWriteFinish stability is 100ms; allow generous margin so a real
|
||||
// (incorrectly broadcast) event would land in `events` before we assert.
|
||||
await new Promise((r) => setTimeout(r, 400));
|
||||
expect(sink.events).toEqual([]);
|
||||
sink.close();
|
||||
});
|
||||
|
||||
it("does NOT emit when the backend's own PUT /api/servers/:id writes the file", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify(
|
||||
{ mcpServers: { alpha: { type: "stdio", command: "old" } } },
|
||||
null,
|
||||
2,
|
||||
) + "\n",
|
||||
);
|
||||
const sink = await subscribe(h.baseUrl);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
const putRes = await fetch(`${h.baseUrl}/api/servers/alpha`, {
|
||||
method: "PUT",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
id: "alpha",
|
||||
config: { type: "stdio", command: "new" },
|
||||
}),
|
||||
});
|
||||
expect(putRes.status).toBe(200);
|
||||
|
||||
await new Promise((r) => setTimeout(r, 400));
|
||||
expect(sink.events).toEqual([]);
|
||||
sink.close();
|
||||
});
|
||||
|
||||
it("does NOT emit when the backend's own DELETE /api/servers/:id writes the file", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify(
|
||||
{ mcpServers: { alpha: { type: "stdio", command: "node" } } },
|
||||
null,
|
||||
2,
|
||||
) + "\n",
|
||||
);
|
||||
const sink = await subscribe(h.baseUrl);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
const delRes = await fetch(`${h.baseUrl}/api/servers/alpha`, {
|
||||
method: "DELETE",
|
||||
});
|
||||
expect(delRes.status).toBe(200);
|
||||
|
||||
await new Promise((r) => setTimeout(r, 400));
|
||||
expect(sink.events).toEqual([]);
|
||||
sink.close();
|
||||
});
|
||||
|
||||
it("emits a broadcast when an external edit slips in between two backend writes (peer-tab visibility)", async () => {
|
||||
// This is the race the watcher-handler suppression cannot catch on its
|
||||
// own: external editor saves AFTER the backend has read the current
|
||||
// mtime but BEFORE chokidar's awaitWriteFinish has fired, so chokidar
|
||||
// coalesces the external mtime and our merged-write mtime into one
|
||||
// event whose mtime matches our own write — and we'd suppress.
|
||||
// `writeMcpAndTrackMtime` covers this by stat()ing on entry and
|
||||
// broadcasting itself if the pre-write mtime doesn't match its tracked
|
||||
// value, so a second connected subscriber learns about the change.
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify(
|
||||
{ mcpServers: { alpha: { type: "stdio", command: "v1" } } },
|
||||
null,
|
||||
2,
|
||||
) + "\n",
|
||||
);
|
||||
// Drive the first write through the backend so lastWrittenMtimeMs is
|
||||
// populated with the mtime we expect on disk going into the next write.
|
||||
const firstPut = await fetch(`${h.baseUrl}/api/servers/alpha`, {
|
||||
method: "PUT",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
id: "alpha",
|
||||
config: { type: "stdio", command: "v2" },
|
||||
}),
|
||||
});
|
||||
expect(firstPut.status).toBe(200);
|
||||
|
||||
// Subscribe AFTER the first write so the peer subscriber represents a
|
||||
// separate tab opened mid-session.
|
||||
const sink = await subscribe(h.baseUrl);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
// External editor writes the file directly. Need a fresh mtime distinct
|
||||
// from the last backend write, so wait a beat (mtime granularity on
|
||||
// older filesystems is ms-coarse).
|
||||
await new Promise((r) => setTimeout(r, 20));
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify(
|
||||
{ mcpServers: { alpha: { type: "stdio", command: "external" } } },
|
||||
null,
|
||||
2,
|
||||
) + "\n",
|
||||
);
|
||||
|
||||
// Backend write follows quickly enough that chokidar's
|
||||
// awaitWriteFinish coalesces both events. Without the pre-stat check
|
||||
// the peer subscriber would never see this edit.
|
||||
const secondPut = await fetch(`${h.baseUrl}/api/servers/alpha`, {
|
||||
method: "PUT",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
id: "alpha",
|
||||
config: { type: "stdio", command: "v3-merged" },
|
||||
}),
|
||||
});
|
||||
expect(secondPut.status).toBe(200);
|
||||
|
||||
await sink.waitFor(1);
|
||||
expect(sink.events.length).toBeGreaterThanOrEqual(1);
|
||||
expect(JSON.parse(sink.events[0]!)).toEqual({ type: "change" });
|
||||
sink.close();
|
||||
});
|
||||
|
||||
it("emits when the user deletes the config file", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({ mcpServers: {} }, null, 2) + "\n",
|
||||
);
|
||||
const sink = await subscribe(h.baseUrl);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
expect(existsSync(h.configPath)).toBe(true);
|
||||
unlinkSync(h.configPath);
|
||||
|
||||
await sink.waitFor(1);
|
||||
expect(sink.events).toHaveLength(1);
|
||||
sink.close();
|
||||
});
|
||||
|
||||
it("broadcasts to multiple connected subscribers", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({ mcpServers: {} }, null, 2) + "\n",
|
||||
);
|
||||
const sink1 = await subscribe(h.baseUrl);
|
||||
const sink2 = await subscribe(h.baseUrl);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify(
|
||||
{ mcpServers: { beta: { type: "stdio", command: "node" } } },
|
||||
null,
|
||||
2,
|
||||
) + "\n",
|
||||
);
|
||||
|
||||
await Promise.all([sink1.waitFor(1), sink2.waitFor(1)]);
|
||||
expect(sink1.events).toHaveLength(1);
|
||||
expect(sink2.events).toHaveLength(1);
|
||||
sink1.close();
|
||||
sink2.close();
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue