From a6f233eb6a95a6d95c5bb2e0f929ace96ee41a3b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 8 Aug 2026 17:41:27 -0400 Subject: [PATCH] fix: read the list error via useSyncExternalStore; clear correlation maps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from Copilot's review of #1953. useManagedListError re-synced React state from the `state` prop inside a useEffect, which renders one frame carrying the PREVIOUS store's error after `state` changes (switching servers) before the effect corrects it — the pattern AGENTS.md forbids. useSyncExternalStore reads the snapshot during render, so a store swap lands in the same frame, and it also closes the window where an error recorded between render and subscribe was missed. The snapshot returns the stored Error instance (or null), so it is referentially stable as the API requires. The `markResponseRejected` correlation maps were never cleared. Ids of requests that never got a response — a timeout, a dropped connection — are the only entries trackResponse can't remove, so they accumulated across reconnects. Cleared on the start-clean connect path, per the convention documented on resetSessionState (one route out, `onerror` with no `onclose`, tears down nothing). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt --- .../core/react/useManagedListError.test.tsx | 25 ++++++++++ core/mcp/inspectorClient.ts | 11 +++++ core/react/useManagedListError.ts | 49 ++++++++++++------- 3 files changed, 66 insertions(+), 19 deletions(-) diff --git a/clients/web/src/test/core/react/useManagedListError.test.tsx b/clients/web/src/test/core/react/useManagedListError.test.tsx index 7863fe32..1eac0afd 100644 --- a/clients/web/src/test/core/react/useManagedListError.test.tsx +++ b/clients/web/src/test/core/react/useManagedListError.test.tsx @@ -84,4 +84,29 @@ describe("useManagedListError", () => { await expect(state.refresh()).rejects.toThrow(boom); expect(state.getError()).toBe(boom); }); + + // The useState+useEffect subscribe pattern would render one frame carrying + // the PREVIOUS store's error here, before the effect re-synced. With + // useSyncExternalStore the snapshot is read during render, so the swap lands + // in the same frame — asserted immediately after rerender, with no waitFor + // and no act() flush, which is what makes it a regression test rather than a + // restatement of eventual consistency. + it("reflects a store swap in the same frame", async () => { + client.listAllTools.mockRejectedValueOnce(boom); + await expect(state.refresh()).rejects.toThrow(boom); + + const other = new ManagedToolsState(client, 0); + + const { result, rerender } = renderHook( + ({ s }: { s: ManagedToolsState }) => useManagedListError(s), + { initialProps: { s: state } }, + ); + expect(result.current).toBe(boom); + + rerender({ s: other }); + expect(result.current).toBeNull(); + + rerender({ s: state }); + expect(result.current).toBe(boom); + }); }); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index b0c14f94..8e268477 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1581,6 +1581,17 @@ export class InspectorClient extends InspectorClientEventTarget { this.clearReceiverTasks(); this.resetSubscriptionStream(); this.cancelledTaskIds.clear(); + // Correlation data is per-session: JSON-RPC ids don't survive it, and + // MessageLogState drops its entries on disconnect, so anything left here + // could only point at an entry that no longer exists. Clearing also + // releases the ids of requests that never got a response (a timeout, a + // dropped connection) — the only entries `trackResponse` can't remove, so + // without this they accumulate across reconnects. Cleared here on the + // start-clean path rather than in `disconnect()` for the reason documented + // above: one route out (`onerror` with no `onclose`) tears down nothing + // (#1953). + this.outboundRequestMethods.clear(); + this.lastAnsweredRequestByMethod.clear(); for (const [, controller] of this.taskInputAbortControllers) { controller.abort(new Error("Connection ended")); } diff --git a/core/react/useManagedListError.ts b/core/react/useManagedListError.ts index 22fe307f..64438e52 100644 --- a/core/react/useManagedListError.ts +++ b/core/react/useManagedListError.ts @@ -1,4 +1,4 @@ -import { useState, useEffect } from "react"; +import { useCallback, useSyncExternalStore } from "react"; import type { ManagedListEventMap } from "../mcp/state/managedListState.js"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; @@ -30,28 +30,39 @@ export interface ManagedListErrorSource { * Shared by the four `useManaged*` hooks so a list load that fails — including * the connect-time one, which has no caller to await it — reaches the UI * instead of only the console. `null` means the last fetch succeeded. + * + * Built on `useSyncExternalStore` rather than the `useState` + `useEffect` + * subscribe pattern the sibling hooks use. Re-syncing state from the `state` + * prop inside an effect would render one frame carrying the PREVIOUS store's + * error after `state` changes (switching servers) before the effect corrects + * it — the "don't derive state from props in an effect" rule in AGENTS.md. + * `useSyncExternalStore` has no such window: the snapshot is read during + * render, so a store swap is reflected in the same frame, and it also closes + * the gap where an error recorded between render and subscribe would be missed. + * + * The snapshot must be referentially stable across reads that mean "no change", + * which it is: it returns the stored `Error` instance itself (or `null`), never + * a fresh object. */ export function useManagedListError( state: ManagedListErrorSource | null, ): Error | null { - const [error, setError] = useState(state?.getError() ?? null); + const subscribe = useCallback( + (onStoreChange: () => void) => { + if (!state) return () => {}; + const listener = () => onStoreChange(); + state.addEventListener("errorChange", listener); + return () => { + state.removeEventListener("errorChange", listener); + }; + }, + [state], + ); - useEffect(() => { - if (!state) { - setError(null); - return; - } - setError(state.getError()); - const onErrorChange = ( - event: TypedEventGeneric, - ) => { - setError(event.detail); - }; - state.addEventListener("errorChange", onErrorChange); - return () => { - state.removeEventListener("errorChange", onErrorChange); - }; - }, [state]); + const getSnapshot = useCallback(() => state?.getError() ?? null, [state]); - return error; + // Server snapshot: same read. The stores are browser/Node runtime objects + // with no SSR path, and passing the same getter keeps hydration consistent + // rather than throwing on a server render. + return useSyncExternalStore(subscribe, getSnapshot, getSnapshot); }