diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index c0d5260c..0c011109 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -1199,6 +1199,157 @@ describe("useServers", () => { } }); + /** + * #1914 — a write that lands but whose list reload fails must reject. + * + * `ServerConfigModal` renders `submitError` only for something `onSubmit` + * threw, so while the post-write refresh swallowed its failure into the + * `error` state the Add Server modal closed as though nothing had gone + * wrong. That is the "silent failure" in the report: the POST succeeded, + * the GET 500'd, and the user saw neither the new row nor an error. + * + * `failGetAfterWrite` lets the seed load and the write itself through, then + * fails every subsequent list read — the shape a container with no + * reachable keychain produced before #1848. + */ + function failGetAfterWrite(message: string): { + fetchFn: typeof fetch; + startFailing: () => void; + } { + let failing = false; + const fetchFn: typeof fetch = async (input, init) => { + const req = + input instanceof Request + ? input + : new Request(input as string | URL, init); + if (failing && req.method === "GET" && req.url.endsWith("/api/servers")) { + return new Response(JSON.stringify({ error: message }), { + status: 500, + headers: { "Content-Type": "application/json" }, + }); + } + return h.fetchFn(req); + }; + return { + fetchFn, + startFailing: () => { + failing = true; + }, + }; + } + + it("addServer rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("keyring unavailable"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + startFailing(); + await act(async () => { + await expect( + result.current.addServer("alpha", { type: "stdio", command: "node" }), + ).rejects.toThrow( + /The server was added, but the server list could not be reloaded: keyring unavailable/, + ); + }); + + // The write itself landed, and the message says so — a bare failure would + // send the user into a retry that trips the duplicate-id check. + expect(readConfig(h.configPath).mcpServers.alpha).toEqual({ + type: "stdio", + command: "node", + }); + // Still recorded in `error` for any list-level UI reading it. + await waitFor(() => + expect(result.current.error).toBe("keyring unavailable"), + ); + expect(result.current.loading).toBe(false); + }); + + it("updateServer rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const existing = result.current.servers[0]!.id; + + startFailing(); + await act(async () => { + await expect( + result.current.updateServer(existing, existing, { + type: "stdio", + command: "node", + }), + ).rejects.toThrow( + /The server was saved, but the server list could not be reloaded: disk full/, + ); + }); + }); + + it("updateServerSettings rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const existing = result.current.servers[0]!.id; + + startFailing(); + await act(async () => { + await expect( + result.current.updateServerSettings(existing, { + headers: [], + env: [], + metadata: [], + roots: [], + connectionTimeout: 5000, + requestTimeout: 30000, + taskTtl: 30000, + maxFetchRequests: 1000, + paginatedLists: true, + }), + ).rejects.toThrow( + /The server was saved, but the server list could not be reloaded: disk full/, + ); + }); + }); + + it("removeServer rejects when the post-write list reload fails (#1914)", async () => { + const { fetchFn, startFailing } = failGetAfterWrite("disk full"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const existing = result.current.servers[0]!.id; + + startFailing(); + await act(async () => { + await expect(result.current.removeServer(existing)).rejects.toThrow( + /The server was removed, but the server list could not be reloaded: disk full/, + ); + }); + expect(readConfig(h.configPath).mcpServers[existing]).toBeUndefined(); + }); + + it("a plain refresh() still resolves on a failed list read, recording the error (#1914)", async () => { + // The rethrow is scoped to post-write reloads. `refresh()` is awaited by + // the mount effect and the SSE loop, neither of which has anywhere to put + // a rejection, so it must keep swallowing into `error`. + const { fetchFn, startFailing } = failGetAfterWrite("keyring unavailable"); + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + + startFailing(); + await act(async () => { + await expect(result.current.refresh()).resolves.toBeUndefined(); + }); + expect(result.current.error).toBe("keyring unavailable"); + }); + it("uses DEFAULT_SEED_CONFIG keys on the first load (seed-write contract)", async () => { const { result } = renderHook(() => useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), diff --git a/core/react/useServers.ts b/core/react/useServers.ts index c9713344..bcba4690 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -132,33 +132,74 @@ export function useServers(opts: UseServersOptions): UseServersResult { const [loading, setLoading] = useState(true); const [error, setError] = useState(undefined); + // The list read itself, throwing on failure. Every refresh path goes + // through it; they differ only in what they do with a rejection. + const loadServers = useCallback(async (): Promise => { + 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; + return mcpConfigToServerEntries(body); + }, [base, authToken, doFetch]); + // 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. + // + // A failure is recorded in `error` and NOT rethrown: the two callers are + // the mount effect and the SSE loop, neither of which has anywhere to put + // a rejection. Post-write reloads use `refreshAfterWrite` instead. const refreshInternal = useCallback( async (background: boolean): Promise => { 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)); + setServers(await loadServers()); } catch (err) { setError(err instanceof Error ? err.message : String(err)); } finally { if (!background) setLoading(false); } }, - [base, authToken, doFetch], + [loadServers], + ); + + // Post-write reload, used by every mutator. Unlike `refreshInternal` it + // rethrows, because a mutator's caller learns that something went wrong + // only from a rejection — `ServerConfigModal` renders `submitError` for a + // thrown error and nothing else, so a successful POST followed by a failed + // GET closed the Add Server modal as though the whole thing had worked + // (#1914). The reported failure was silent for exactly this reason. + // + // The message says the write landed, because it did: only reading the list + // back failed. A bare "could not add server" would send the user straight + // into a retry that trips the duplicate-id check, which is the dead end the + // original report described. + const refreshAfterWrite = useCallback( + async (verb: string): Promise => { + setLoading(true); + setError(undefined); + try { + setServers(await loadServers()); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + setError(message); + throw new Error( + `The server was ${verb}, but the server list could not be reloaded: ${message}`, + { cause: err }, + ); + } finally { + setLoading(false); + } + }, + [loadServers], ); const refresh = useCallback( @@ -241,9 +282,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("added"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const importSource = useCallback( @@ -282,9 +323,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("saved"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const updateServerSettings = useCallback( @@ -307,9 +348,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("saved"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const removeServer = useCallback( @@ -324,9 +365,9 @@ export function useServers(opts: UseServersOptions): UseServersResult { if (!res.ok) { throw new Error(await readErrorMessage(res)); } - await refresh(); + await refreshAfterWrite("removed"); }, - [base, authToken, doFetch, refresh], + [base, authToken, doFetch, refreshAfterWrite], ); const reorderServers = useCallback(