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); }