diff --git a/.changeset/reconnect-health-refresh.md b/.changeset/reconnect-health-refresh.md new file mode 100644 index 000000000..4c2de7545 --- /dev/null +++ b/.changeset/reconnect-health-refresh.md @@ -0,0 +1,7 @@ +--- +"executor": patch +--- + +**Fix: reconnecting an OAuth connection now refreshes its health status in place — no page reload needed** + +Completing a reconnect previously left the stale "Expired" verdict on the connection row (and the integrations-list summary) until a hard refresh. Re-minting now clears the persisted verdict, and the UI re-probes as soon as the refreshed connection arrives. diff --git a/e2e/selfhost/mcp-oauth-reconnect-health.test.ts b/e2e/selfhost/mcp-oauth-reconnect-health.test.ts index b363c38dd..cbb6ff509 100644 --- a/e2e/selfhost/mcp-oauth-reconnect-health.test.ts +++ b/e2e/selfhost/mcp-oauth-reconnect-health.test.ts @@ -1,6 +1,9 @@ -// Selfhost repros for two MCP OAuth bugs seen with a DCR connection whose -// refresh token is rejected by the provider as `invalid_grant`. +// Selfhost repros for MCP OAuth bugs seen with a DCR connection whose +// refresh token is rejected by the provider as `invalid_grant`, including the +// reconnect journey: completing Reconnect must refresh the health verdict on +// the page without a hard reload. import { randomBytes } from "node:crypto"; +import { createServer } from "node:http"; import { Effect } from "effect"; import { expect } from "@effect/vitest"; @@ -48,31 +51,136 @@ const requiredRedirect = (response: Response, from: string): string => { return new URL(location, from).toString(); }; +/** The test server's login page is plain text with Basic-auth POST — nothing a + * browser can click. Complete it out of band and hand back the callback URL. */ +const submitProviderLogin = async (loginUrl: string): Promise => { + const credentials = Buffer.from("alice:password").toString("base64"); + const response = await fetch(loginUrl, { + method: "POST", + redirect: "manual", + headers: { authorization: `Basic ${credentials}` }, + }); + const location = response.headers.get("location"); + if (response.status !== 302 || !location) { + throw new Error(`provider login did not redirect (${response.status})`); + } + return new URL(location, loginUrl).toString(); +}; + const completeAuthorization = (authorizationUrl: string) => Effect.promise(async () => { const login = await fetch(authorizationUrl, { redirect: "manual" }); const loginUrl = requiredRedirect(login, authorizationUrl); - const credentials = Buffer.from("alice:password").toString("base64"); - const callback = await fetch(loginUrl, { - method: "POST", - headers: { authorization: `Basic ${credentials}` }, - redirect: "manual", - }); - const callbackUrl = requiredRedirect(callback, loginUrl); + const callbackUrl = await submitProviderLogin(loginUrl); const parsed = new URL(callbackUrl); const code = parsed.searchParams.get("code"); if (!code) throw new Error(`OAuth callback did not include a code: ${callbackUrl}`); return { code }; }); -const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) => +/** AS whose refresh grants are dead forever — every token it mints is already + * expired and refresh is rejected as `invalid_grant`. */ +const serveDeadGrantOAuthServer = () => + serveOAuthTestServer({ + scopes: ["channels:history", "users:read"], + supportRefresh: false, + tokenExpiresInSeconds: 0, + invalidRefreshTokenDescription: "Grant not found", + }); + +interface GrantRevocationGate { + /** Token endpoint to register with executor instead of the real one. */ + readonly tokenUrl: string; + /** The provider comes back: minted tokens get their real lifetime and + * refresh grants are honored again. */ + readonly restore: () => void; + readonly refreshRejections: () => number; + readonly close: () => void; +} + +/** Token-endpoint proxy in front of the test AS. While "revoked" it behaves + * like a provider whose grants are dead — authorization_code exchanges still + * succeed but mint already-expired tokens, and refresh grants are rejected + * with `invalid_grant` — and after `restore()` it is a plain passthrough. The + * test server itself cannot flip behavior after construction, and this repro + * needs "expired now, healthy after a fresh reconnect". */ +const serveGrantRevocationGate = (upstreamTokenUrl: string) => + Effect.acquireRelease( + Effect.callback((resume) => { + let revoked = true; + let refreshRejections = 0; + const server = createServer((request, response) => { + const chunks: Buffer[] = []; + request.on("data", (chunk: Buffer) => chunks.push(chunk)); + request.on("end", () => { + const body = Buffer.concat(chunks).toString("utf8"); + if (revoked && new URLSearchParams(body).get("grant_type") === "refresh_token") { + refreshRejections += 1; + response.writeHead(400, { "content-type": "application/json" }); + response.end( + JSON.stringify({ error: "invalid_grant", error_description: "Grant not found" }), + ); + return; + } + fetch(upstreamTokenUrl, { + method: "POST", + headers: { + "content-type": + request.headers["content-type"] ?? "application/x-www-form-urlencoded", + ...(request.headers.authorization + ? { authorization: request.headers.authorization } + : {}), + }, + body, + }) + .then(async (upstream) => { + const text = await upstream.text(); + if (revoked && upstream.ok) { + const parsed = JSON.parse(text) as Record; + parsed["expires_in"] = 0; + response.writeHead(upstream.status, { "content-type": "application/json" }); + response.end(JSON.stringify(parsed)); + return; + } + response.writeHead(upstream.status, { + "content-type": upstream.headers.get("content-type") ?? "application/json", + }); + response.end(text); + }) + .catch(() => { + response.writeHead(502, { "content-type": "application/json" }); + response.end(JSON.stringify({ error: "bad_gateway" })); + }); + }); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address !== null ? address.port : 0; + resume( + Effect.succeed({ + tokenUrl: `http://127.0.0.1:${port}/token`, + restore: () => { + revoked = false; + }, + refreshRejections: () => refreshRejections, + close: () => { + server.close(); + server.closeAllConnections(); + }, + }), + ); + }); + }), + (gate) => Effect.sync(gate.close), + ); + +const seedDcrMcpOAuthConnection = ( + client: Client, + prefix: string, + oauth: OAuthTestServerShape, + options?: { readonly tokenUrl?: string }, +) => Effect.gen(function* () { - const oauth = yield* serveOAuthTestServer({ - scopes: ["channels:history", "users:read"], - supportRefresh: false, - tokenExpiresInSeconds: 0, - invalidRefreshTokenDescription: "Grant not found", - }); const slug = IntegrationSlug.make(freshSlug(prefix)); const clientSlug = OAuthClientSlug.make(freshSlug(`${prefix}-client`)); @@ -101,7 +209,7 @@ const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) => issuer: probe.issuer ?? null, registrationEndpoint: probe.registrationEndpoint, authorizationUrl: probe.authorizationUrl, - tokenUrl: probe.tokenUrl, + tokenUrl: options?.tokenUrl ?? probe.tokenUrl, resource: probe.resource ?? oauth.mcpResourceUrl, scopes: probe.scopesSupported ?? [], tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, @@ -140,6 +248,12 @@ const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) => return { oauth, slug }; }); +const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) => + Effect.gen(function* () { + const oauth = yield* serveDeadGrantOAuthServer(); + return yield* seedDcrMcpOAuthConnection(client, prefix, oauth); + }); + const logTokenRequests = (label: string, oauth: OAuthTestServerShape) => Effect.gen(function* () { const requests = yield* oauth.requests; @@ -224,6 +338,94 @@ scenario( ), ); +// The reconnect journey from the bug report: a connection reads Expired, the +// user completes Reconnect through the OAuth popup, and the page must show the +// recovered health WITHOUT a hard refresh. The gate makes the provider's +// grants dead during seeding (already-expired tokens, refresh rejected) and +// healthy again before the reconnect, so the only thing standing between the +// user and a green dot is the UI updating itself. +scenario( + "MCP OAuth · completed reconnect refreshes the health verdict without a page reload", + { + timeout: 240_000, + }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + const oauth = yield* serveOAuthTestServer({ + scopes: ["channels:history", "users:read"], + }); + const gate = yield* serveGrantRevocationGate(`${oauth.issuerUrl}/token`); + const { slug } = yield* seedDcrMcpOAuthConnection(client, "mcp-reconnect-live", oauth, { + tokenUrl: gate.tokenUrl, + }); + + // Persist the expired verdict exactly as the user's "Check now" would. + const seededHealth = yield* client.connections.checkHealth({ + params: { owner: "org", integration: slug, name }, + query: {}, + }); + expect(seededHealth.status, "the dead grant seeds an expired verdict").toBe("expired"); + expect(gate.refreshRejections(), "the expiry came from a rejected refresh").toBeGreaterThan( + 0, + ); + + yield* browser.session(identity, async ({ page, step }) => { + const connections = connectionsSection(page); + const menuTrigger = connections.locator('button[aria-haspopup="menu"]').first(); + + await step("Open the integration: the connection reads Expired", async () => { + await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 }); + await connections.getByLabel("Status: Expired").waitFor({ timeout: 30_000 }); + }); + + await step("Reconnect and complete the OAuth flow in the popup", async () => { + // The provider comes back before the user reconnects — fresh grants + // are fully healthy from here on. + gate.restore(); + + const popupPromise = page.waitForEvent("popup", { timeout: 30_000 }); + await menuTrigger.click(); + await page.getByRole("menuitem", { name: "Reconnect" }).click(); + const popup = await popupPromise; + + // The test AS login page is plain text driven by Basic-auth POST, so + // complete it out of band and drive the popup to the callback — the + // same journey a user's click-through consent takes. + await popup.waitForURL(/\/login\?/, { timeout: 30_000 }); + const callbackUrl = await submitProviderLogin(popup.url()); + await popup.goto(callbackUrl); + await page.getByText("Reconnected", { exact: true }).waitFor({ timeout: 30_000 }); + }); + + await step("The backend already sees the connection as healthy", async () => { + // Evidence that only the UI is stale: the same health endpoint the + // page would call classifies the re-minted grant as healthy. + const response = await page.request.post(healthPath(slug)); + const body = (await response.json()) as { readonly status?: string }; + console.info(`[BUG repro] post-reconnect health: ${response.status()} ${body.status}`); + expect(response.status(), "post-reconnect health check succeeds").toBe(200); + expect(body.status, "the re-minted grant is healthy").toBe("healthy"); + }); + + await step("BUG: the row must flip to Healthy without a hard page refresh", async () => { + await connections.getByLabel("Status: Healthy").waitFor({ timeout: 30_000 }); + await connections.getByText("Expired", { exact: true }).waitFor({ + state: "hidden", + timeout: 5_000, + }); + }); + }); + }), + ), +); + scenario( "MCP OAuth · DCR reconnect keeps the dialog open and reaches OAuth start", { diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 7ba62b5bb..a784a253a 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -2789,6 +2789,11 @@ export const createExecutor = 0 ? { missingOAuthScopes: input.missingOAuthScopes } : null, + // A re-mint replaces the grant, so any persisted verdict describes + // a credential that no longer exists. Clear it rather than let a + // pre-reconnect "expired" outlive the reconnect; the next health + // check writes the verdict for the new grant. + last_health: null, updated_at: now, }; if (existing) { diff --git a/packages/plugins/mcp/src/sdk/connection.ts b/packages/plugins/mcp/src/sdk/connection.ts index 4cad5f767..82a98cd0d 100644 --- a/packages/plugins/mcp/src/sdk/connection.ts +++ b/packages/plugins/mcp/src/sdk/connection.ts @@ -172,6 +172,15 @@ const fetchFromHttpClientLayer = ( } return response; }); + // Mark the request promise observed (a no-op handler on the ORIGINAL + // promise; callers still see the rejection). The MCP SDK fires some + // requests without a rejection handler — a cancellation notification + // after a request timeout, an SSE dial raced against an abort — and when + // the upstream is already gone that rejection is unhandled, which kills + // the whole Bun server process, not just this call. Browsers never crash + // on an unobserved fetch rejection; this adapter must match. + // oxlint-disable-next-line executor/no-promise-catch -- boundary: Fetch-compatible adapter must observe rejections the SDK abandons + promise.catch(() => undefined); if (!init?.signal) return promise; // oxlint-disable-next-line executor/no-promise-reject -- boundary: Fetch-compatible adapter mirrors abort rejection semantics if (init.signal.aborted) return Promise.reject(abortError(init.signal)); diff --git a/packages/react/src/lib/use-connection-health.ts b/packages/react/src/lib/use-connection-health.ts index a6c50c847..57b3e8ca1 100644 --- a/packages/react/src/lib/use-connection-health.ts +++ b/packages/react/src/lib/use-connection-health.ts @@ -42,6 +42,33 @@ const revalidateQuery = ( ): { readonly ifStaleMs?: number } => last?.status === "healthy" ? { ifStaleMs: HEALTH_REVALIDATE_MS } : {}; +/** Identity of a persisted verdict, for detecting the reconnect transition. + * An OAuth re-mint clears `last_health`, so a verdict giving way to `null` + * means the grant was replaced and the row must re-probe even though it never + * remounts (its React key is owner:integration:name, unchanged by a + * reconnect). This CLEARING transition is the only re-trigger: reacting to + * every epoch change instead would race probes against cache refetches + * (a refetch can deliver a snapshot older than a just-adopted verdict) and + * storm upstreams with re-probes. `null` is a real epoch — never-checked or + * just-re-minted — distinct from the "never seen" sentinel `undefined`. */ +const verdictEpoch = (last: HealthCheckResult | null | undefined): number | null => + last?.checkedAt ?? null; + +/** The verdict to display: whichever of the live probe and the persisted + * verdict is FRESHEST. A plain live-over-persisted preference would let a + * pre-reconnect probe shadow the verdict a completed reconnect persisted + * (any surface may write a newer verdict server-side; this hook only learns + * of it through the refetched row). Ties keep the live result: identical + * timestamps mean it IS the persisted verdict, echoed back. */ +const freshestVerdict = ( + live: HealthCheckResult | null, + persisted: HealthCheckResult | null | undefined, +): HealthCheckResult | null => { + if (live === null) return persisted ?? null; + if (persisted == null) return live; + return persisted.checkedAt > live.checkedAt ? persisted : live; +}; + /** * Imperative invalidation of the connections cache for one owner. The server * persists every verdict on `last_health`, so after a check we must re-read the @@ -70,35 +97,46 @@ export function useConnectionHealth(connection: Connection): { readonly status: HealthStatus; readonly runCheck: () => Promise>; } { - // A live probe result, once a check has run, overrides the persisted one. + // A live probe result, once a check has run; merged with the persisted + // verdict by freshness (see freshestVerdict for why not live-always-wins). const [liveProbe, setLiveProbe] = useState(null); const doCheck = useAtomSet(checkConnectionHealth, { mode: "promiseExit" }); const invalidateConnections = useInvalidateConnections(); - const probe = liveProbe ?? connection.lastHealth ?? null; + const probe = freshestVerdict(liveProbe, connection.lastHealth); const status: HealthStatus = probe?.status ?? "unknown"; // Health checks are AUTOMATIC: loading the list revalidates any verdict // older than the freshness window (or never checked), stale-while-revalidate // style: the persisted verdict renders instantly, the probe corrects it in - // place. - const revalidated = useRef(false); + // place. The guard is once per mount PLUS once per clearing: the ref holds + // the last epoch seen, and a verdict giving way to `null` (an OAuth re-mint + // cleared it) re-arms the probe — that is how a completed reconnect gets its + // recovery probe without a page reload. Only the clearing transition + // re-arms; every other epoch change (a probe's own verdict echoed back by + // the refetch, a concurrent surface's fresher verdict) stays quiet, keeping + // the no-probe-storm invariant of the original once-per-mount guard. + const seenEpoch = useRef(undefined); useEffect(() => { - if (revalidated.current) return; const last = connection.lastHealth; + const epoch = verdictEpoch(last); + const firstSight = seenEpoch.current === undefined; + const cleared = epoch === null && seenEpoch.current !== null && !firstSight; + seenEpoch.current = epoch; + if (!firstSight && !cleared) return; if (healthyAndFresh(last)) return; - revalidated.current = true; void doCheck({ params: connectionParams(connection), query: revalidateQuery(last), }).then((exit) => { // Background refresh: update the dot on success, stay quiet on failure // (the persisted verdict is still the best known state). Invalidate the - // connections cache ONLY when the verdict actually changed: on the common - // no-change reconfirm we skip it, so an automatic probe never churns the - // cache (which would refetch connections, re-run this effect, and, but - // for the once-per-mount ref guard, risk a probe loop). + // connections cache ONLY when the verdict actually changed: on the + // common no-change reconfirm we skip it, so an automatic probe never + // churns the cache (which would refetch connections, re-run this + // effect, and, but for the epoch guard, risk a probe loop). if (!Exit.isSuccess(exit)) return; + seenEpoch.current = exit.value.checkedAt; setLiveProbe(exit.value); if (exit.value.status !== (last?.status ?? "unknown")) { invalidateConnections(connection.owner); @@ -108,14 +146,17 @@ export function useConnectionHealth(connection: Connection): { const runCheck = useCallback(async () => { // Manual "Check now": invalidate the connections cache unconditionally so - // every surface picks up the freshly persisted verdict. Re-running this - // effect after the refetch is harmless: the ref guard blocks a re-probe. + // every surface picks up the freshly persisted verdict. Adopting the + // result's epoch keeps the resulting refetch from re-probing. const exit = await doCheck({ params: connectionParams(connection), query: {}, reactivityKeys: connectionCheckKeys, }); - if (Exit.isSuccess(exit)) setLiveProbe(exit.value); + if (Exit.isSuccess(exit)) { + seenEpoch.current = exit.value.checkedAt; + setLiveProbe(exit.value); + } return exit; }, [connection, doCheck]); @@ -140,25 +181,29 @@ export function useConnectionsHealth( const doCheck = useAtomSet(checkConnectionHealth, { mode: "promiseExit" }); const invalidateConnections = useInvalidateConnections(); - // Once per mount PER CONNECTION: the list streams in asynchronously, so the - // effect re-runs as rows arrive; the key set keeps each row to one probe. - const revalidated = useRef(new Set()); + // Once per VERDICT per connection (same epoch guard as the single-connection + // hook): the list streams in asynchronously, so the effect re-runs as rows + // arrive; each row probes once per persisted-verdict epoch, and a re-minted + // connection (epoch cleared to null) probes again without a remount. + const revalidated = useRef(new Map()); useEffect(() => { for (const connection of connections) { const key = probeKey(connection); - if (revalidated.current.has(key)) continue; const last = connection.lastHealth; + const epoch = verdictEpoch(last); + if (revalidated.current.has(key) && revalidated.current.get(key) === epoch) continue; + revalidated.current.set(key, epoch); if (healthyAndFresh(last)) continue; - revalidated.current.add(key); void doCheck({ params: connectionParams(connection), query: revalidateQuery(last), }).then((exit) => { // Same automatic-path rule as the single-connection hook: reflect the - // verdict, and invalidate the connections cache only when it changed so - // an unchanged reconfirm never churns the cache (the per-key ref guard - // already prevents a re-probe on the resulting re-render). + // verdict, adopt its epoch so the refetch doesn't re-probe, and + // invalidate the connections cache only when the verdict changed so an + // unchanged reconfirm never churns the cache. if (!Exit.isSuccess(exit)) return; + revalidated.current.set(key, exit.value.checkedAt); setLiveProbes((current) => new Map(current).set(key, exit.value)); if (exit.value.status !== (last?.status ?? "unknown")) { invalidateConnections(connection.owner); @@ -169,7 +214,7 @@ export function useConnectionsHealth( return useCallback( (connection: Connection) => - liveProbes.get(probeKey(connection)) ?? connection.lastHealth ?? null, + freshestVerdict(liveProbes.get(probeKey(connection)) ?? null, connection.lastHealth), [liveProbes], ); }