History

Refresh connection health after OAuth reconnect (#1545)

* e2e: repro stale health verdict after OAuth reconnect * Refresh connection health after OAuth reconnect * Add changeset * Prevent server crash from unobserved MCP fetch rejection; merge health verdicts by freshness * Lint suppression for the adapter catch * Retrigger CI

Rhys Sullivan committed 3 months ago

Commit df62bb31 parent 4fae8515 files · +307 −39Browse files

Unified · Split

5 files
  1. .changeset/reconnect-health-refresh.md +7 −0
  2. e2e/selfhost/mcp-oauth-reconnect-health.test.ts +219 −17
  3. packages/core/sdk/src/executor.ts +5 −0
  4. packages/plugins/mcp/src/sdk/connection.ts +9 −0
  5. packages/react/src/lib/use-connection-health.ts +67 −22
.changeset/reconnect-health-refresh.md+7−0
@@ -0,0 +1,7 @@
1+---
2+"executor": patch
3+---
4+
5+**Fix: reconnecting an OAuth connection now refreshes its health status in place — no page reload needed**
6+
7+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.
e2e/selfhost/mcp-oauth-reconnect-health.test.ts+219−17
@@ -1,6 +1,9 @@
1−// Selfhost repros for two MCP OAuth bugs seen with a DCR connection whose
2−// refresh token is rejected by the provider as `invalid_grant`.
1+// Selfhost repros for MCP OAuth bugs seen with a DCR connection whose
2+// refresh token is rejected by the provider as `invalid_grant`, including the
3+// reconnect journey: completing Reconnect must refresh the health verdict on
4+// the page without a hard reload.
35 import { randomBytes } from "node:crypto";
6+import { createServer } from "node:http";
47
58 import { Effect } from "effect";
69 import { expect } from "@effect/vitest";
@@ -48,31 +51,136 @@ const requiredRedirect = (response: Response, from: string): string => { · 41 unmodified lines
4851 return new URL(location, from).toString();
4952 };
5053
54+/** The test server's login page is plain text with Basic-auth POST — nothing a
55+ * browser can click. Complete it out of band and hand back the callback URL. */
56+const submitProviderLogin = async (loginUrl: string): Promise<string> => {
57+ const credentials = Buffer.from("alice:password").toString("base64");
58+ const response = await fetch(loginUrl, {
59+ method: "POST",
60+ redirect: "manual",
61+ headers: { authorization: `Basic ${credentials}` },
62+ });
63+ const location = response.headers.get("location");
64+ if (response.status !== 302 || !location) {
65+ throw new Error(`provider login did not redirect (${response.status})`);
66+ }
67+ return new URL(location, loginUrl).toString();
68+};
69+
5170 const completeAuthorization = (authorizationUrl: string) =>
5271 Effect.promise(async () => {
5372 const login = await fetch(authorizationUrl, { redirect: "manual" });
5473 const loginUrl = requiredRedirect(login, authorizationUrl);
55− const credentials = Buffer.from("alice:password").toString("base64");
56− const callback = await fetch(loginUrl, {
57− method: "POST",
58− headers: { authorization: `Basic ${credentials}` },
59− redirect: "manual",
60− });
61− const callbackUrl = requiredRedirect(callback, loginUrl);
74+ const callbackUrl = await submitProviderLogin(loginUrl);
6275 const parsed = new URL(callbackUrl);
6376 const code = parsed.searchParams.get("code");
6477 if (!code) throw new Error(`OAuth callback did not include a code: ${callbackUrl}`);
6578 return { code };
6679 });
6780
68−const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) =>
81+/** AS whose refresh grants are dead forever — every token it mints is already
82+ * expired and refresh is rejected as `invalid_grant`. */
83+const serveDeadGrantOAuthServer = () =>
84+ serveOAuthTestServer({
85+ scopes: ["channels:history", "users:read"],
86+ supportRefresh: false,
87+ tokenExpiresInSeconds: 0,
88+ invalidRefreshTokenDescription: "Grant not found",
89+ });
90+
91+interface GrantRevocationGate {
92+ /** Token endpoint to register with executor instead of the real one. */
93+ readonly tokenUrl: string;
94+ /** The provider comes back: minted tokens get their real lifetime and
95+ * refresh grants are honored again. */
96+ readonly restore: () => void;
97+ readonly refreshRejections: () => number;
98+ readonly close: () => void;
99+}
100+
101+/** Token-endpoint proxy in front of the test AS. While "revoked" it behaves
102+ * like a provider whose grants are dead — authorization_code exchanges still
103+ * succeed but mint already-expired tokens, and refresh grants are rejected
104+ * with `invalid_grant` — and after `restore()` it is a plain passthrough. The
105+ * test server itself cannot flip behavior after construction, and this repro
106+ * needs "expired now, healthy after a fresh reconnect". */
107+const serveGrantRevocationGate = (upstreamTokenUrl: string) =>
108+ Effect.acquireRelease(
109+ Effect.callback<GrantRevocationGate>((resume) => {
110+ let revoked = true;
111+ let refreshRejections = 0;
112+ const server = createServer((request, response) => {
113+ const chunks: Buffer[] = [];
114+ request.on("data", (chunk: Buffer) => chunks.push(chunk));
115+ request.on("end", () => {
116+ const body = Buffer.concat(chunks).toString("utf8");
117+ if (revoked && new URLSearchParams(body).get("grant_type") === "refresh_token") {
118+ refreshRejections += 1;
119+ response.writeHead(400, { "content-type": "application/json" });
120+ response.end(
121+ JSON.stringify({ error: "invalid_grant", error_description: "Grant not found" }),
122+ );
123+ return;
124+ }
125+ fetch(upstreamTokenUrl, {
126+ method: "POST",
127+ headers: {
128+ "content-type":
129+ request.headers["content-type"] ?? "application/x-www-form-urlencoded",
130+ ...(request.headers.authorization
131+ ? { authorization: request.headers.authorization }
132+ : {}),
133+ },
134+ body,
135+ })
136+ .then(async (upstream) => {
137+ const text = await upstream.text();
138+ if (revoked && upstream.ok) {
139+ const parsed = JSON.parse(text) as Record<string, unknown>;
140+ parsed["expires_in"] = 0;
141+ response.writeHead(upstream.status, { "content-type": "application/json" });
142+ response.end(JSON.stringify(parsed));
143+ return;
144+ }
145+ response.writeHead(upstream.status, {
146+ "content-type": upstream.headers.get("content-type") ?? "application/json",
147+ });
148+ response.end(text);
149+ })
150+ .catch(() => {
151+ response.writeHead(502, { "content-type": "application/json" });
152+ response.end(JSON.stringify({ error: "bad_gateway" }));
153+ });
154+ });
155+ });
156+ server.listen(0, "127.0.0.1", () => {
157+ const address = server.address();
158+ const port = typeof address === "object" && address !== null ? address.port : 0;
159+ resume(
160+ Effect.succeed({
161+ tokenUrl: `http://127.0.0.1:${port}/token`,
162+ restore: () => {
163+ revoked = false;
164+ },
165+ refreshRejections: () => refreshRejections,
166+ close: () => {
167+ server.close();
168+ server.closeAllConnections();
169+ },
170+ }),
171+ );
172+ });
173+ }),
174+ (gate) => Effect.sync(gate.close),
175+ );
176+
177+const seedDcrMcpOAuthConnection = (
178+ client: Client,
179+ prefix: string,
180+ oauth: OAuthTestServerShape,
181+ options?: { readonly tokenUrl?: string },
182+) =>
69183 Effect.gen(function* () {
70− const oauth = yield* serveOAuthTestServer({
71− scopes: ["channels:history", "users:read"],
72− supportRefresh: false,
73− tokenExpiresInSeconds: 0,
74− invalidRefreshTokenDescription: "Grant not found",
75− });
76184 const slug = IntegrationSlug.make(freshSlug(prefix));
77185 const clientSlug = OAuthClientSlug.make(freshSlug(`${prefix}-client`));
78186
@@ -101,7 +209,7 @@ const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) => · 22 unmodified lines
101209 issuer: probe.issuer ?? null,
102210 registrationEndpoint: probe.registrationEndpoint,
103211 authorizationUrl: probe.authorizationUrl,
104− tokenUrl: probe.tokenUrl,
212+ tokenUrl: options?.tokenUrl ?? probe.tokenUrl,
105213 resource: probe.resource ?? oauth.mcpResourceUrl,
106214 scopes: probe.scopesSupported ?? [],
107215 tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported,
@@ -140,6 +248,12 @@ const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) => · 32 unmodified lines
140248 return { oauth, slug };
141249 });
142250
251+const seedExpiredDcrMcpOAuthConnection = (client: Client, prefix: string) =>
252+ Effect.gen(function* () {
253+ const oauth = yield* serveDeadGrantOAuthServer();
254+ return yield* seedDcrMcpOAuthConnection(client, prefix, oauth);
255+ });
256+
143257 const logTokenRequests = (label: string, oauth: OAuthTestServerShape) =>
144258 Effect.gen(function* () {
145259 const requests = yield* oauth.requests;
@@ -224,6 +338,94 @@ scenario( · 78 unmodified lines
224338 ),
225339 );
226340
341+// The reconnect journey from the bug report: a connection reads Expired, the
342+// user completes Reconnect through the OAuth popup, and the page must show the
343+// recovered health WITHOUT a hard refresh. The gate makes the provider's
344+// grants dead during seeding (already-expired tokens, refresh rejected) and
345+// healthy again before the reconnect, so the only thing standing between the
346+// user and a green dot is the UI updating itself.
347+scenario(
348+ "MCP OAuth · completed reconnect refreshes the health verdict without a page reload",
349+ {
350+ timeout: 240_000,
351+ },
352+ Effect.scoped(
353+ Effect.gen(function* () {
354+ const target = yield* Target;
355+ const browser = yield* Browser;
356+ const { client: makeApiClient } = yield* Api;
357+ const identity = yield* target.newIdentity();
358+ const client = yield* makeApiClient(api, identity);
359+
360+ const oauth = yield* serveOAuthTestServer({
361+ scopes: ["channels:history", "users:read"],
362+ });
363+ const gate = yield* serveGrantRevocationGate(`${oauth.issuerUrl}/token`);
364+ const { slug } = yield* seedDcrMcpOAuthConnection(client, "mcp-reconnect-live", oauth, {
365+ tokenUrl: gate.tokenUrl,
366+ });
367+
368+ // Persist the expired verdict exactly as the user's "Check now" would.
369+ const seededHealth = yield* client.connections.checkHealth({
370+ params: { owner: "org", integration: slug, name },
371+ query: {},
372+ });
373+ expect(seededHealth.status, "the dead grant seeds an expired verdict").toBe("expired");
374+ expect(gate.refreshRejections(), "the expiry came from a rejected refresh").toBeGreaterThan(
375+ 0,
376+ );
377+
378+ yield* browser.session(identity, async ({ page, step }) => {
379+ const connections = connectionsSection(page);
380+ const menuTrigger = connections.locator('button[aria-haspopup="menu"]').first();
381+
382+ await step("Open the integration: the connection reads Expired", async () => {
383+ await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" });
384+ await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 });
385+ await connections.getByLabel("Status: Expired").waitFor({ timeout: 30_000 });
386+ });
387+
388+ await step("Reconnect and complete the OAuth flow in the popup", async () => {
389+ // The provider comes back before the user reconnects — fresh grants
390+ // are fully healthy from here on.
391+ gate.restore();
392+
393+ const popupPromise = page.waitForEvent("popup", { timeout: 30_000 });
394+ await menuTrigger.click();
395+ await page.getByRole("menuitem", { name: "Reconnect" }).click();
396+ const popup = await popupPromise;
397+
398+ // The test AS login page is plain text driven by Basic-auth POST, so
399+ // complete it out of band and drive the popup to the callback — the
400+ // same journey a user's click-through consent takes.
401+ await popup.waitForURL(/\/login\?/, { timeout: 30_000 });
402+ const callbackUrl = await submitProviderLogin(popup.url());
403+ await popup.goto(callbackUrl);
404+ await page.getByText("Reconnected", { exact: true }).waitFor({ timeout: 30_000 });
405+ });
406+
407+ await step("The backend already sees the connection as healthy", async () => {
408+ // Evidence that only the UI is stale: the same health endpoint the
409+ // page would call classifies the re-minted grant as healthy.
410+ const response = await page.request.post(healthPath(slug));
411+ const body = (await response.json()) as { readonly status?: string };
412+ console.info(`[BUG repro] post-reconnect health: ${response.status()} ${body.status}`);
413+ expect(response.status(), "post-reconnect health check succeeds").toBe(200);
414+ expect(body.status, "the re-minted grant is healthy").toBe("healthy");
415+ });
416+
417+ await step("BUG: the row must flip to Healthy without a hard page refresh", async () => {
418+ await connections.getByLabel("Status: Healthy").waitFor({ timeout: 30_000 });
419+ await connections.getByText("Expired", { exact: true }).waitFor({
420+ state: "hidden",
421+ timeout: 5_000,
422+ });
423+ });
424+ });
425+ }),
426+ ),
427+);
428+
227429 scenario(
228430 "MCP OAuth · DCR reconnect keeps the dialog open and reaches OAuth start",
229431 {
packages/core/sdk/src/executor.ts+5−0
@@ -2962,6 +2962,11 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 2961 unmodified lines
29622962 input.missingOAuthScopes && input.missingOAuthScopes.length > 0
29632963 ? { missingOAuthScopes: input.missingOAuthScopes }
29642964 : null,
2965+ // A re-mint replaces the grant, so any persisted verdict describes
2966+ // a credential that no longer exists. Clear it rather than let a
2967+ // pre-reconnect "expired" outlive the reconnect; the next health
2968+ // check writes the verdict for the new grant.
2969+ last_health: null,
29652970 updated_at: now,
29662971 };
29672972 if (existing) {
packages/plugins/mcp/src/sdk/connection.ts+9−0
@@ -172,6 +172,15 @@ const fetchFromHttpClientLayer = ( · 171 unmodified lines
172172 }
173173 return response;
174174 });
175+ // Mark the request promise observed (a no-op handler on the ORIGINAL
176+ // promise; callers still see the rejection). The MCP SDK fires some
177+ // requests without a rejection handler — a cancellation notification
178+ // after a request timeout, an SSE dial raced against an abort — and when
179+ // the upstream is already gone that rejection is unhandled, which kills
180+ // the whole Bun server process, not just this call. Browsers never crash
181+ // on an unobserved fetch rejection; this adapter must match.
182+ // oxlint-disable-next-line executor/no-promise-catch -- boundary: Fetch-compatible adapter must observe rejections the SDK abandons
183+ promise.catch(() => undefined);
175184 if (!init?.signal) return promise;
176185 // oxlint-disable-next-line executor/no-promise-reject -- boundary: Fetch-compatible adapter mirrors abort rejection semantics
177186 if (init.signal.aborted) return Promise.reject(abortError(init.signal));
packages/react/src/lib/use-connection-health.ts+67−22
@@ -42,6 +42,33 @@ const revalidateQuery = ( · 41 unmodified lines
4242 ): { readonly ifStaleMs?: number } =>
4343 last?.status === "healthy" ? { ifStaleMs: HEALTH_REVALIDATE_MS } : {};
4444
45+/** Identity of a persisted verdict, for detecting the reconnect transition.
46+ * An OAuth re-mint clears `last_health`, so a verdict giving way to `null`
47+ * means the grant was replaced and the row must re-probe even though it never
48+ * remounts (its React key is owner:integration:name, unchanged by a
49+ * reconnect). This CLEARING transition is the only re-trigger: reacting to
50+ * every epoch change instead would race probes against cache refetches
51+ * (a refetch can deliver a snapshot older than a just-adopted verdict) and
52+ * storm upstreams with re-probes. `null` is a real epoch — never-checked or
53+ * just-re-minted — distinct from the "never seen" sentinel `undefined`. */
54+const verdictEpoch = (last: HealthCheckResult | null | undefined): number | null =>
55+ last?.checkedAt ?? null;
56+
57+/** The verdict to display: whichever of the live probe and the persisted
58+ * verdict is FRESHEST. A plain live-over-persisted preference would let a
59+ * pre-reconnect probe shadow the verdict a completed reconnect persisted
60+ * (any surface may write a newer verdict server-side; this hook only learns
61+ * of it through the refetched row). Ties keep the live result: identical
62+ * timestamps mean it IS the persisted verdict, echoed back. */
63+const freshestVerdict = (
64+ live: HealthCheckResult | null,
65+ persisted: HealthCheckResult | null | undefined,
66+): HealthCheckResult | null => {
67+ if (live === null) return persisted ?? null;
68+ if (persisted == null) return live;
69+ return persisted.checkedAt > live.checkedAt ? persisted : live;
70+};
71+
4572 /**
4673 * Imperative invalidation of the connections cache for one owner. The server
4774 * persists every verdict on `last_health`, so after a check we must re-read the
@@ -70,35 +97,46 @@ export function useConnectionHealth(connection: Connection): { · 22 unmodified lines
7097 readonly status: HealthStatus;
7198 readonly runCheck: () => Promise<Exit.Exit<HealthCheckResult, unknown>>;
7299 } {
73− // A live probe result, once a check has run, overrides the persisted one.
100+ // A live probe result, once a check has run; merged with the persisted
101+ // verdict by freshness (see freshestVerdict for why not live-always-wins).
74102 const [liveProbe, setLiveProbe] = useState<HealthCheckResult | null>(null);
75103 const doCheck = useAtomSet(checkConnectionHealth, { mode: "promiseExit" });
76104 const invalidateConnections = useInvalidateConnections();
77105
78− const probe = liveProbe ?? connection.lastHealth ?? null;
106+ const probe = freshestVerdict(liveProbe, connection.lastHealth);
79107 const status: HealthStatus = probe?.status ?? "unknown";
80108
81109 // Health checks are AUTOMATIC: loading the list revalidates any verdict
82110 // older than the freshness window (or never checked), stale-while-revalidate
83111 // style: the persisted verdict renders instantly, the probe corrects it in
84− // place.
85− const revalidated = useRef(false);
112+ // place. The guard is once per mount PLUS once per clearing: the ref holds
113+ // the last epoch seen, and a verdict giving way to `null` (an OAuth re-mint
114+ // cleared it) re-arms the probe — that is how a completed reconnect gets its
115+ // recovery probe without a page reload. Only the clearing transition
116+ // re-arms; every other epoch change (a probe's own verdict echoed back by
117+ // the refetch, a concurrent surface's fresher verdict) stays quiet, keeping
118+ // the no-probe-storm invariant of the original once-per-mount guard.
119+ const seenEpoch = useRef<number | null | undefined>(undefined);
86120 useEffect(() => {
87− if (revalidated.current) return;
88121 const last = connection.lastHealth;
122+ const epoch = verdictEpoch(last);
123+ const firstSight = seenEpoch.current === undefined;
124+ const cleared = epoch === null && seenEpoch.current !== null && !firstSight;
125+ seenEpoch.current = epoch;
126+ if (!firstSight && !cleared) return;
89127 if (healthyAndFresh(last)) return;
90− revalidated.current = true;
91128 void doCheck({
92129 params: connectionParams(connection),
93130 query: revalidateQuery(last),
94131 }).then((exit) => {
95132 // Background refresh: update the dot on success, stay quiet on failure
96133 // (the persisted verdict is still the best known state). Invalidate the
97− // connections cache ONLY when the verdict actually changed: on the common
98− // no-change reconfirm we skip it, so an automatic probe never churns the
99− // cache (which would refetch connections, re-run this effect, and, but
100− // for the once-per-mount ref guard, risk a probe loop).
134+ // connections cache ONLY when the verdict actually changed: on the
135+ // common no-change reconfirm we skip it, so an automatic probe never
136+ // churns the cache (which would refetch connections, re-run this
137+ // effect, and, but for the epoch guard, risk a probe loop).
101138 if (!Exit.isSuccess(exit)) return;
139+ seenEpoch.current = exit.value.checkedAt;
102140 setLiveProbe(exit.value);
103141 if (exit.value.status !== (last?.status ?? "unknown")) {
104142 invalidateConnections(connection.owner);
@@ -108,14 +146,17 @@ export function useConnectionHealth(connection: Connection): { · 3 unmodified lines
108146
109147 const runCheck = useCallback(async () => {
110148 // Manual "Check now": invalidate the connections cache unconditionally so
111− // every surface picks up the freshly persisted verdict. Re-running this
112− // effect after the refetch is harmless: the ref guard blocks a re-probe.
149+ // every surface picks up the freshly persisted verdict. Adopting the
150+ // result's epoch keeps the resulting refetch from re-probing.
113151 const exit = await doCheck({
114152 params: connectionParams(connection),
115153 query: {},
116154 reactivityKeys: connectionCheckKeys,
117155 });
118− if (Exit.isSuccess(exit)) setLiveProbe(exit.value);
156+ if (Exit.isSuccess(exit)) {
157+ seenEpoch.current = exit.value.checkedAt;
158+ setLiveProbe(exit.value);
159+ }
119160 return exit;
120161 }, [connection, doCheck]);
121162
@@ -140,25 +181,29 @@ export function useConnectionsHealth( · 18 unmodified lines
140181 const doCheck = useAtomSet(checkConnectionHealth, { mode: "promiseExit" });
141182 const invalidateConnections = useInvalidateConnections();
142183
143− // Once per mount PER CONNECTION: the list streams in asynchronously, so the
144− // effect re-runs as rows arrive; the key set keeps each row to one probe.
145− const revalidated = useRef(new Set<string>());
184+ // Once per VERDICT per connection (same epoch guard as the single-connection
185+ // hook): the list streams in asynchronously, so the effect re-runs as rows
186+ // arrive; each row probes once per persisted-verdict epoch, and a re-minted
187+ // connection (epoch cleared to null) probes again without a remount.
188+ const revalidated = useRef(new Map<string, number | null>());
146189 useEffect(() => {
147190 for (const connection of connections) {
148191 const key = probeKey(connection);
149− if (revalidated.current.has(key)) continue;
150192 const last = connection.lastHealth;
193+ const epoch = verdictEpoch(last);
194+ if (revalidated.current.has(key) && revalidated.current.get(key) === epoch) continue;
195+ revalidated.current.set(key, epoch);
151196 if (healthyAndFresh(last)) continue;
152− revalidated.current.add(key);
153197 void doCheck({
154198 params: connectionParams(connection),
155199 query: revalidateQuery(last),
156200 }).then((exit) => {
157201 // Same automatic-path rule as the single-connection hook: reflect the
158− // verdict, and invalidate the connections cache only when it changed so
159− // an unchanged reconfirm never churns the cache (the per-key ref guard
160− // already prevents a re-probe on the resulting re-render).
202+ // verdict, adopt its epoch so the refetch doesn't re-probe, and
203+ // invalidate the connections cache only when the verdict changed so an
204+ // unchanged reconfirm never churns the cache.
161205 if (!Exit.isSuccess(exit)) return;
206+ revalidated.current.set(key, exit.value.checkedAt);
162207 setLiveProbes((current) => new Map(current).set(key, exit.value));
163208 if (exit.value.status !== (last?.status ?? "unknown")) {
164209 invalidateConnections(connection.owner);
@@ -169,7 +214,7 @@ export function useConnectionsHealth( · 4 unmodified lines
169214
170215 return useCallback(
171216 (connection: Connection) =>
172− liveProbes.get(probeKey(connection)) ?? connection.lastHealth ?? null,
217+ freshestVerdict(liveProbes.get(probeKey(connection)) ?? null, connection.lastHealth),
173218 [liveProbes],
174219 );
175220 }