fix(servers): address PR review on hot-reload (#1357)

cliffhall · claude · cliffhall · commit d90f8e6522ea · 2026-05-25T10:46:50.000-04:00
- 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) &lt;noreply@anthropic.com&gt;
diff --git a/clients/web/src/test/integration/mcp/remote/servers-events.test.ts b/clients/web/src/test/integration/mcp/remote/servers-events.test.ts
@@ -225,6 +225,126 @@ describe("GET /api/servers/events", () => {
     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",
+    );
+    // A GET first so the backend records `lastWrittenMtimeMs` from any
+    // seed-write path it owns and so writeMcpAndTrackMtime's pre-stat below
+    // doesn't trip the external-edit-detected branch.
+    await fetch(`${h.baseUrl}/api/servers`);
+    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",
+    );
+    await fetch(`${h.baseUrl}/api/servers`);
+    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,
diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts
@@ -93,6 +93,12 @@ export interface CreateRemoteAppResult {
    * HTTP-server close so the watcher is released on shutdown. Tests that
    * exercise SSE should call it in teardown — tests that never subscribe
    * never start the watcher and can omit it without leaking.
+   *
+   * Resolves once the subscriber set is cleared and the watcher is closed.
+   * Individual SSE stream callbacks held inside `streamSSE` are not awaited
+   * here — they resolve on their own when the underlying socket aborts.
+   * Callers that need to be sure those have settled (e.g. the standalone
+   * server) should call `httpServer.closeAllConnections()` *after* this.
    */
   close: () => Promise<void>;
 }
@@ -296,6 +302,33 @@ export function createRemoteApp(
   };
 
   const writeMcpAndTrackMtime = async (data: string): Promise<void> => {
+    // If an external editor wrote the file between our previous write and
+    // this one, chokidar's `awaitWriteFinish` will coalesce both events
+    // into a single watcher fire whose mtime matches what we're about to
+    // write — and the watcher handler will then suppress the broadcast.
+    // Peer subscribers (e.g. a second browser tab) would never learn about
+    // the external edit. Detect that case here by comparing the current
+    // on-disk mtime against our last tracked mtime; broadcast after the
+    // write completes so peers re-fetch the merged state.
+    //
+    // The originating tab's mutator triggers its own refresh on PUT/POST
+    // success, so this extra broadcast is intended for peers only — a
+    // double refresh on the originating tab is cheap (same GET, same
+    // payload) and acceptable.
+    let externalEditDetected = false;
+    if (lastWrittenMtimeMs !== null) {
+      try {
+        const s = await fsStat(mcpConfigPath);
+        if (s.mtimeMs !== lastWrittenMtimeMs) {
+          externalEditDetected = true;
+        }
+      } catch {
+        // File missing → an external delete slipped in between our writes.
+        // Treat as an external edit so peers learn about it.
+        externalEditDetected = true;
+      }
+    }
+
     await writeStoreFile(mcpConfigPath, data);
     try {
       const s = await fsStat(mcpConfigPath);
@@ -304,6 +337,10 @@ export function createRemoteApp(
       // If the stat fails the next watcher event will broadcast — that's the
       // correct fallback (the only cost is a redundant client refresh).
     }
+
+    if (externalEditDetected) {
+      broadcastServerListChange();
+    }
   };
 
   const handleWatcherEvent = async (event: string): Promise<void> => {
diff --git a/core/react/useServers.ts b/core/react/useServers.ts
@@ -76,25 +76,39 @@ export function useServers(opts: UseServersOptions): UseServersResult {
   const [loading, setLoading] = useState<boolean>(true);
   const [error, setError] = useState<string | undefined>(undefined);
 
-  const refresh = useCallback(async (): Promise<void> => {
-    setLoading(true);
-    setError(undefined);
-    try {
-      const res = await doFetch(`${base}/api/servers`, {
-        method: "GET",
-        headers: buildHeaders(authToken, false),
-      });
-      if (!res.ok) {
-        throw new Error(await readErrorMessage(res));
+  // Inner refresh that the public `refresh` and the SSE-triggered background
+  // refresh both share. `background` skips the loading-state toggle so
+  // consumers rendering a spinner or skeleton don't flash on every external
+  // mcp.json edit — the existing list stays on screen while the re-fetch
+  // resolves. `error` is still reset / set either way so a real error
+  // surfaces even from a background refresh.
+  const refreshInternal = useCallback(
+    async (background: boolean): Promise<void> => {
+      if (!background) setLoading(true);
+      setError(undefined);
+      try {
+        const res = await doFetch(`${base}/api/servers`, {
+          method: "GET",
+          headers: buildHeaders(authToken, false),
+        });
+        if (!res.ok) {
+          throw new Error(await readErrorMessage(res));
+        }
+        const body = (await res.json()) as MCPConfig;
+        setServers(mcpConfigToServerEntries(body));
+      } catch (err) {
+        setError(err instanceof Error ? err.message : String(err));
+      } finally {
+        if (!background) setLoading(false);
       }
-      const body = (await res.json()) as MCPConfig;
-      setServers(mcpConfigToServerEntries(body));
-    } catch (err) {
-      setError(err instanceof Error ? err.message : String(err));
-    } finally {
-      setLoading(false);
-    }
-  }, [base, authToken, doFetch]);
+    },
+    [base, authToken, doFetch],
+  );
+
+  const refresh = useCallback(
+    (): Promise<void> => refreshInternal(false),
+    [refreshInternal],
+  );
 
   useEffect(() => {
     void refresh();
@@ -122,19 +136,28 @@ export function useServers(opts: UseServersOptions): UseServersResult {
         const decoder = new TextDecoder();
         let buffer = "";
         // SSE frames are separated by a blank line (`\n\n`). We don't parse
-        // the event type or data — `refresh()` re-fetches the canonical
-        // state regardless — so a single \n\n is enough to debounce a
-        // single broadcast into a single refresh.
+        // the event type or data — `refreshInternal()` re-fetches the
+        // canonical state regardless — so we just count frames and fire a
+        // single background refresh per decode chunk. Two `change`
+        // broadcasts landing in the same chunk become one re-fetch instead
+        // of two concurrent ones whose setState order is unspecified.
+        // Cross-chunk debounce is not added: `awaitWriteFinish`'s 100ms
+        // stability threshold already serializes external edits at the
+        // source, and chained fetches against the same GET endpoint are
+        // idempotent enough that a rare back-to-back pair just costs one
+        // extra round-trip.
         while (true) {
           const { done, value } = await reader.read();
           if (done) break;
           buffer += decoder.decode(value, { stream: true });
+          let sawFrame = false;
           let frameEnd = buffer.indexOf("\n\n");
           while (frameEnd !== -1) {
             buffer = buffer.slice(frameEnd + 2);
-            void refresh();
+            sawFrame = true;
             frameEnd = buffer.indexOf("\n\n");
           }
+          if (sawFrame) void refreshInternal(true);
         }
       } catch {
         // AbortError on unmount, or a transient network blip. No reconnect:
@@ -143,7 +166,7 @@ export function useServers(opts: UseServersOptions): UseServersResult {
       }
     })();
     return () => controller.abort();
-  }, [base, authToken, doFetch, refresh]);
+  }, [base, authToken, doFetch, refreshInternal]);
 
   const addServer = useCallback(
     async (id: string, config: MCPServerConfig): Promise<void> => {
