History

Persist connection health verdicts on failure and always send ifStaleMs (#1777)

* Persist connection health verdicts on failure and always send ifStaleMs * Cover the client half of the health-verdict fix with a browser scenario * Keep probing non-healthy verdicts so recovery still shows Sending a freshness window for a non-healthy verdict gated the probe against the verdict the previous probe had just persisted, so an expired or degraded connection could not turn green until the window elapsed. That broke the recovery-on-next-load contract the health-checks-ui, graphql-introspection-health and mcp-oauth-reconnect-health scenarios are built on. Restore the unconditional probe for non-healthy verdicts and keep the server-side change, which is what actually fixes the reported symptom: a refused refresh now folds into a persisted verdict instead of the failure channel, so it is answered rather than raised. * Assert the health probe stops, not an exact probe count The exact-count assertion sampled refresh grants at one instant while probes were still in flight, so it read 3 where it expected 2. Assert the two invariants that do not race instead: grants never outnumber the health requests that caused them, and a settled page issues no further probes.

Rhys Sullivan committed 2 months ago

Commit 5062eb51 parent 3927eba4 files · +649 −56Browse files

Unified · Split

4 files
  1. e2e/scenarios/connection-health-verdict.test.ts +531 −0
  2. packages/core/sdk/src/executor.ts +56 −54
  3. packages/react/src/lib/use-connection-health.test.ts +48 −0
  4. packages/react/src/lib/use-connection-health.ts +14 −2
e2e/scenarios/connection-health-verdict.test.ts+531−0
@@ -0,0 +1,531 @@
1+// Cross-target: a connection whose credential cannot be resolved is a health
2+// VERDICT, not a failed request — and the verdict is persisted, so the next
3+// surface to ask reads it instead of hammering the authorization server.
4+//
5+// Production symptom: a handful of connections whose authorization server had
6+// stopped re-minting credentials produced hundreds of server errors, every one
7+// of them on `/api/connections/.../health`, plus unbounded refresh traffic to
8+// the third party. A probe whose credential resolution failed escaped before
9+// the verdict was written, so nothing was ever persisted, the freshness gate
10+// had nothing to serve, and every mount of every surface sent another refresh
11+// grant to a server that was already refusing.
12+//
13+// The journey: an OpenAPI integration completes a real authorization-code flow
14+// against a live test AS that mints instantly-expiring access tokens and
15+// refuses every refresh grant. A health check is configured on the
16+// integration, so the connection takes the probing path. The probe cannot get
17+// a credential — and must answer with a verdict, persist it, and let a
18+// freshness window keep the next check off the wire. Both refusals are
19+// covered: a retryable one (`degraded`) and a dead grant (`expired`).
20+//
21+// A second scenario in this file walks the same broken connection through the
22+// real UI, because the other half of the symptom was the number of SURFACES:
23+// see its header.
24+import { randomBytes } from "node:crypto";
25+import { createServer } from "node:http";
26+
27+import { expect } from "@effect/vitest";
28+import { Effect } from "effect";
29+import type { HttpApiClient } from "effect/unstable/httpapi";
30+import type { Response } from "playwright";
31+import { composePluginApi } from "@executor-js/api/server";
32+import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";
33+import {
34+ AuthTemplateSlug,
35+ ConnectionName,
36+ IntegrationSlug,
37+ OAuthClientSlug,
38+} from "@executor-js/sdk/shared";
39+import { serveOAuthTestServer } from "@executor-js/sdk/testing";
40+
41+import { scenario } from "../src/scenario";
42+import { Api, Browser, Target } from "../src/services";
43+import { visit } from "../src/surfaces/browser";
44+
45+const api = composePluginApi([openApiHttpPlugin()] as const);
46+type Client = HttpApiClient.ForApi<typeof api>;
47+
48+const unique = (prefix: string) => `${prefix}${randomBytes(4).toString("hex")}`;
49+
50+/** Upstream on 127.0.0.1 whose `GET /me` is the obvious health probe. The
51+ * credential never resolves in this scenario, so a request reaching here at
52+ * all would mean the probe ran with no token — the 401 keeps that honest. */
53+const serveUpstream = () =>
54+ Effect.acquireRelease(
55+ Effect.callback<{ readonly url: string; readonly close: () => void }>((resume) => {
56+ const server = createServer((request, response) => {
57+ if (request.method === "GET" && (request.url ?? "").startsWith("/me")) {
58+ const authorized = (request.headers["authorization"] ?? "").startsWith("Bearer at_");
59+ response.writeHead(authorized ? 200 : 401, {
60+ "content-type": "application/json",
61+ });
62+ response.end(JSON.stringify(authorized ? { email: "probe@example.test" } : {}));
63+ return;
64+ }
65+ response.writeHead(404, { "content-type": "application/json" });
66+ response.end(JSON.stringify({ error: "not_found" }));
67+ });
68+ server.listen(0, "127.0.0.1", () => {
69+ const address = server.address();
70+ const port = typeof address === "object" && address ? address.port : 0;
71+ resume(
72+ Effect.succeed({
73+ url: `http://127.0.0.1:${port}`,
74+ close: () => {
75+ server.close();
76+ server.closeAllConnections();
77+ },
78+ }),
79+ );
80+ });
81+ }),
82+ (server) => Effect.sync(server.close),
83+ );
84+
85+const spec = (
86+ baseUrl: string,
87+ oauth: {
88+ readonly authorizationEndpoint: string;
89+ readonly tokenEndpoint: string;
90+ },
91+): string =>
92+ JSON.stringify({
93+ openapi: "3.0.3",
94+ info: { title: "Identity API", version: "1.0.0" },
95+ servers: [{ url: baseUrl }],
96+ paths: {
97+ "/me": {
98+ get: {
99+ operationId: "getMe",
100+ summary: "The current account",
101+ security: [{ oauth: ["identity.read"] }],
102+ responses: { "200": { description: "account" } },
103+ },
104+ },
105+ },
106+ components: {
107+ securitySchemes: {
108+ oauth: {
109+ type: "oauth2",
110+ flows: {
111+ authorizationCode: {
112+ authorizationUrl: oauth.authorizationEndpoint,
113+ tokenUrl: oauth.tokenEndpoint,
114+ scopes: { "identity.read": "Read the account" },
115+ },
116+ },
117+ },
118+ },
119+ },
120+ });
121+
122+/** One integration with a declared health check, one OAuth client, one
123+ * connection completed through a real authorization-code flow — against an
124+ * authorization server that refuses every refresh grant in the given way.
125+ * Instantly-expiring access tokens mean every credential resolution must
126+ * refresh, so the refusal is what the probe meets. */
127+const connectRefusing = (
128+ client: Client,
129+ upstream: { readonly url: string },
130+ options: {
131+ readonly name: ConnectionName;
132+ readonly errorCode: string;
133+ readonly description: string;
134+ },
135+) =>
136+ Effect.gen(function* () {
137+ const oauth = yield* serveOAuthTestServer({
138+ scopes: ["identity.read"],
139+ tokenExpiresInSeconds: 0,
140+ supportRefresh: false,
141+ invalidRefreshTokenErrorCode: options.errorCode,
142+ invalidRefreshTokenDescription: options.description,
143+ });
144+ const slug = IntegrationSlug.make(unique("healthverdict"));
145+ const clientSlug = OAuthClientSlug.make(unique("healthverdictc"));
146+
147+ yield* Effect.addFinalizer(() =>
148+ Effect.all(
149+ [
150+ client.connections
151+ .remove({
152+ params: { owner: "org", integration: slug, name: options.name },
153+ })
154+ .pipe(Effect.ignore),
155+ client.oauth
156+ .removeClient({
157+ params: { slug: clientSlug },
158+ payload: { owner: "org" },
159+ })
160+ .pipe(Effect.ignore),
161+ client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore),
162+ ],
163+ { discard: true },
164+ ),
165+ );
166+
167+ yield* client.openapi.addSpec({
168+ payload: {
169+ spec: { kind: "blob", value: spec(upstream.url, oauth) },
170+ slug,
171+ baseUrl: upstream.url,
172+ authenticationTemplate: [
173+ {
174+ slug: "oauth",
175+ kind: "oauth2",
176+ authorizationUrl: oauth.authorizationEndpoint,
177+ tokenUrl: oauth.tokenEndpoint,
178+ scopes: ["identity.read"],
179+ },
180+ ],
181+ },
182+ });
183+
184+ // Configure the probe, the way the user does in the editor: the
185+ // ranked candidates offer the identity GET, and picking it is what
186+ // sends this connection down the probing path.
187+ const candidates = yield* client.integrations.healthCheckCandidates({
188+ params: { slug },
189+ });
190+ const getMe = candidates.find((candidate) => candidate.method === "get");
191+ if (!getMe) return yield* Effect.die("the identity spec exposed no GET candidate");
192+ yield* client.integrations.healthCheckSet({
193+ params: { slug },
194+ payload: { spec: { operation: getMe.operation, identityField: "email" } },
195+ });
196+
197+ yield* client.oauth.createClient({
198+ payload: {
199+ owner: "org",
200+ slug: clientSlug,
201+ grant: "authorization_code",
202+ authorizationUrl: oauth.authorizationEndpoint,
203+ tokenUrl: oauth.tokenEndpoint,
204+ clientId: "test-client",
205+ clientSecret: "test-secret",
206+ originIntegration: slug,
207+ },
208+ });
209+
210+ const started = yield* client.oauth.start({
211+ payload: {
212+ client: clientSlug,
213+ clientOwner: "org",
214+ owner: "org",
215+ name: options.name,
216+ integration: slug,
217+ template: AuthTemplateSlug.make("oauth"),
218+ },
219+ });
220+ expect(started.status, "oauth.start redirects to the authorization server").toBe("redirect");
221+ if (started.status !== "redirect") return yield* Effect.die("no redirect");
222+
223+ // Drive the test IdP's consent by hand (authorize → login → code).
224+ const code = yield* Effect.promise(async () => {
225+ const authorize = await fetch(started.authorizationUrl, {
226+ redirect: "manual",
227+ });
228+ const loginUrl = authorize.headers.get("location");
229+ if (!loginUrl) throw new Error(`authorize did not redirect: ${authorize.status}`);
230+ const login = await fetch(loginUrl, {
231+ method: "POST",
232+ headers: {
233+ authorization: `Basic ${Buffer.from("alice:password").toString("base64")}`,
234+ },
235+ redirect: "manual",
236+ });
237+ const callbackUrl = login.headers.get("location");
238+ if (!callbackUrl) throw new Error(`login did not redirect: ${login.status}`);
239+ const minted = new URL(callbackUrl).searchParams.get("code");
240+ if (!minted) throw new Error("callback carried no authorization code");
241+ return minted;
242+ });
243+ yield* client.oauth.complete({ payload: { state: started.state, code } });
244+ yield* oauth.clearRequests;
245+
246+ return {
247+ slug,
248+ /** Refresh grants the authorization server has actually received.
249+ * This is the number the whole fix is about: probing must not
250+ * scale with the number of surfaces asking. */
251+ refreshGrants: oauth.requests.pipe(
252+ Effect.map(
253+ (all) =>
254+ all.filter(
255+ (request) =>
256+ request.path === "/token" &&
257+ request.method === "POST" &&
258+ request.body.includes("grant_type=refresh_token"),
259+ ).length,
260+ ),
261+ ),
262+ };
263+ });
264+
265+scenario(
266+ "Health checks · a connection whose refresh is refused reports a persisted verdict instead of failing the request",
267+ {},
268+ Effect.scoped(
269+ Effect.gen(function* () {
270+ const target = yield* Target;
271+ const { client: makeClient } = yield* Api;
272+ const identity = yield* target.newIdentity();
273+ const client = yield* makeClient(api, identity);
274+ const upstream = yield* serveUpstream();
275+
276+ // ── A refusal that is not "re-auth required" ────────────────────────
277+ // The AS answers the refresh with a code OTHER than invalid_grant, so
278+ // retrying could in principle work. This is the shape that used to
279+ // escape the probe as a server error.
280+ const degradedName = ConnectionName.make("healthverdictrefused");
281+ const refused = yield* connectRefusing(client, upstream, {
282+ name: degradedName,
283+ errorCode: "invalid_request",
284+ description: "Refresh temporarily unavailable",
285+ });
286+
287+ // THE guarantee: a probe that cannot resolve its credential answers with
288+ // a verdict. The request itself succeeds — a third party refusing a
289+ // refresh is not a defect in this product, and reporting it as one is
290+ // what buried the real signal.
291+ const probed = yield* client.connections.checkHealth({
292+ params: { owner: "org", integration: refused.slug, name: degradedName },
293+ query: {},
294+ });
295+ expect(
296+ probed.status,
297+ "a connection whose refresh is refused reads degraded, not a failed request",
298+ ).toBe("degraded");
299+ expect(
300+ probed.detail ?? "",
301+ "the verdict carries the authorization server's reason",
302+ ).toContain("Refresh temporarily unavailable");
303+ expect(yield* refused.refreshGrants, "the probe did try the refresh exactly once").toBe(1);
304+
305+ // The verdict PERSISTS, so the accounts list shows the state at a glance
306+ // and the freshness gate has something to serve.
307+ const stored = yield* client.connections.get({
308+ params: { owner: "org", integration: refused.slug, name: degradedName },
309+ });
310+ expect(stored?.lastHealth?.status, "the verdict is persisted on the connection").toBe(
311+ "degraded",
312+ );
313+ expect(stored?.lastHealth?.checkedAt, "and it is the verdict this probe produced").toBe(
314+ probed.checkedAt,
315+ );
316+
317+ // And because it persisted, a caller that DOES pass a freshness window is
318+ // served from it: the server's gate has something to answer with, so the
319+ // authorization server sees nothing more. (The automatic client path
320+ // deliberately passes no window for a non-healthy verdict — recovery has
321+ // to be able to show — but the gate itself must work for the callers that
322+ // opt into it.)
323+ for (let mount = 0; mount < 3; mount++) {
324+ const remount = yield* client.connections.checkHealth({
325+ params: {
326+ owner: "org",
327+ integration: refused.slug,
328+ name: degradedName,
329+ },
330+ query: { ifStaleMs: 30_000 },
331+ });
332+ expect(remount.status, "a repeat check inside the window keeps the verdict").toBe(
333+ "degraded",
334+ );
335+ expect(remount.checkedAt, "and it IS the persisted verdict, not a new probe").toBe(
336+ probed.checkedAt,
337+ );
338+ }
339+ expect(
340+ yield* refused.refreshGrants,
341+ "repeated checks inside the window never reach the authorization server again",
342+ ).toBe(1);
343+
344+ // ── A dead grant ───────────────────────────────────────────────────
345+ // invalid_grant means the user must re-authenticate: a different verdict
346+ // through the same probing path, and it persists the same way.
347+ const expiredName = ConnectionName.make("healthverdictdead");
348+ const revoked = yield* connectRefusing(client, upstream, {
349+ name: expiredName,
350+ errorCode: "invalid_grant",
351+ description: "Grant revoked",
352+ });
353+
354+ const dead = yield* client.connections.checkHealth({
355+ params: { owner: "org", integration: revoked.slug, name: expiredName },
356+ query: {},
357+ });
358+ expect(dead.status, "a revoked grant reads expired, so the UI can offer reconnect").toBe(
359+ "expired",
360+ );
361+ expect(dead.detail ?? "", "the verdict carries the authorization server's reason").toContain(
362+ "Grant revoked",
363+ );
364+ const storedDead = yield* client.connections.get({
365+ params: { owner: "org", integration: revoked.slug, name: expiredName },
366+ });
367+ expect(storedDead?.lastHealth?.status, "the expired verdict is persisted too").toBe(
368+ "expired",
369+ );
370+ }),
371+ ),
372+);
373+
374+// ===========================================================================
375+// The other half of the production symptom, through the real UI. A connection
376+// whose refresh is refused is rendered by several surfaces (the integration
377+// page's Connections list and the integrations list summary), and each one
378+// revalidates on mount. Before the fix every one of those probes escaped as a
379+// SERVER ERROR instead of a verdict, so one broken connection produced a
380+// stream of captured errors and no surface could show the user what was wrong.
381+//
382+// What this pins down is that shape, not the volume: every automatic health
383+// request succeeds and every surface paints the same Degraded verdict, while
384+// the per-connection guard keeps each surface to exactly ONE probe — the
385+// no-probe-storm invariant. Re-probing a broken connection once per surface is
386+// the deliberate price of letting recovery show on the next load; an
387+// unbounded loop is the regression worth catching.
388+//
389+// Nothing here touches the hook: the browser navigates between the two
390+// surfaces a user actually visits, and the authorization server's own request
391+// ledger is the judge. Skips on targets with no browser surface.
392+// ===========================================================================
393+
394+scenario(
395+ "Health checks (UI) · every surface showing a broken connection paints a verdict, and probes exactly once",
396+ {},
397+ Effect.scoped(
398+ Effect.gen(function* () {
399+ const target = yield* Target;
400+ const browser = yield* Browser;
401+ const { client: makeClient } = yield* Api;
402+ const identity = yield* target.newIdentity();
403+ const client = yield* makeClient(api, identity);
404+ const upstream = yield* serveUpstream();
405+
406+ const name = ConnectionName.make("healthverdictui");
407+ const refused = yield* connectRefusing(client, upstream, {
408+ name,
409+ errorCode: "invalid_request",
410+ description: "Refresh temporarily unavailable",
411+ });
412+
413+ yield* browser.session(identity, async ({ page, step }) => {
414+ const connections = page.locator("section").filter({
415+ has: page.getByRole("heading", { level: 3, name: "Connections" }),
416+ });
417+ // Every health request the app sends, with the status it came back
418+ // with: the client's own wire contract, observed from outside. A
419+ // credential the third party refuses must still produce a SUCCESSFUL
420+ // health response carrying a verdict — that is the whole fix.
421+ const healthRequests: string[] = [];
422+ const healthFailures: string[] = [];
423+ page.on("request", (request) => {
424+ if (request.method() === "POST" && request.url().includes("/health")) {
425+ healthRequests.push(request.url());
426+ }
427+ });
428+ page.on("response", (response) => {
429+ if (
430+ response.request().method() === "POST" &&
431+ response.url().includes("/health") &&
432+ response.status() >= 400
433+ ) {
434+ healthFailures.push(`${String(response.status())} ${response.url()}`);
435+ }
436+ });
437+ const isHealthResponse = (response: Response) =>
438+ response.request().method() === "POST" && response.url().includes("/health");
439+ const refreshGrants = () => Effect.runPromise(refused.refreshGrants);
440+
441+ // Surface 1. No verdict is persisted yet, so this load's automatic
442+ // check is the probe that discovers the broken credential — the badge
443+ // appearing is proof it completed and was written down.
444+ await step("Open the integration: the broken connection reads Degraded", async () => {
445+ const settled = page.waitForResponse(isHealthResponse, {
446+ timeout: 30_000,
447+ });
448+ await visit(page, `/integrations/${refused.slug}`);
449+ await settled;
450+ await connections.getByText("Degraded", { exact: true }).waitFor({ timeout: 30_000 });
451+ });
452+
453+ // The baseline is taken AFTER that probe, so the freshness window is
454+ // running from a verdict written moments ago and the assertion below
455+ // does not depend on how long the browser took to start.
456+ const baseline = await refreshGrants();
457+ expect(baseline, "the first surface did probe the authorization server").toBeGreaterThan(0);
458+ const afterFirstSurface = healthRequests.length;
459+
460+ // Surface 2: the integrations list summarises the same connection's
461+ // health, and mounts its own revalidation.
462+ await step("Open the integrations list, which shows the same connection", async () => {
463+ const settled = page.waitForResponse(isHealthResponse, {
464+ timeout: 30_000,
465+ });
466+ await visit(page, "/");
467+ await settled;
468+ });
469+
470+ // What the LIST surface cost at the authorization server.
471+ const afterList = await refreshGrants();
472+
473+ // Surface 3: back to the integration page — a fresh mount again.
474+ await step("Return to the integration page: another mount, another ask", async () => {
475+ const settled = page.waitForResponse(isHealthResponse, {
476+ timeout: 30_000,
477+ });
478+ await visit(page, `/integrations/${refused.slug}`);
479+ await settled;
480+ await connections.getByText("Degraded", { exact: true }).waitFor({ timeout: 30_000 });
481+ });
482+
483+ const afterReturn = await refreshGrants();
484+
485+ const laterRequests = healthRequests.slice(afterFirstSurface);
486+ expect(
487+ laterRequests.length,
488+ "the later surfaces did ask about the connection's health — silence would mean a broken connection could never be seen to recover",
489+ ).toBeGreaterThan(0);
490+
491+ expect(
492+ afterList - baseline,
493+ "the list surface revalidated the broken connection",
494+ ).toBeGreaterThan(0);
495+
496+ // THE production symptom, stated as the third party experiences it.
497+ // Not "the later surfaces went silent" — they must not, or recovery
498+ // could never show — but "the traffic is attributable and it stops".
499+ //
500+ // Attributable: a refresh grant only ever happens inside a health
501+ // request, so grants can never outnumber the requests that caused
502+ // them. More grants than requests is the server retrying in a loop.
503+ // Counted this way the assertion cannot race a probe that is still in
504+ // flight: the request is recorded when it is SENT, before its grant.
505+ expect(
506+ afterReturn,
507+ "every refresh grant is attributable to a health request the client sent",
508+ ).toBeLessThanOrEqual(healthRequests.length);
509+
510+ // And it stops: once the page has settled, the per-connection guard
511+ // means no further probes are issued. A quiet window that stays quiet
512+ // is the storm's absence stated directly, rather than a probe count
513+ // sampled at one arbitrary instant.
514+ const settled = healthRequests.length;
515+ await page.waitForTimeout(3_000);
516+ expect(
517+ healthRequests.length,
518+ "a settled page stops probing; a still-climbing count is the guard failing and the effect re-probing in a loop",
519+ ).toBe(settled);
520+
521+ // And the shape that actually reached production: a refused refresh is
522+ // answered, not raised. Every one of these requests used to come back
523+ // as a server error, which is what buried the signal.
524+ expect(
525+ healthFailures,
526+ "no automatic health request failed; a refused refresh is a verdict, not an error",
527+ ).toEqual([]);
528+ });
529+ }),
530+ ),
531+);
packages/core/sdk/src/executor.ts+56−54
@@ -3490,24 +3490,6 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 3489 unmodified lines
34903490 })
34913491 .pipe(Effect.ignore);
34923492
3493− const healthFromCredentialResolutionError = (
3494− err: CredentialResolutionError,
3495− ): Effect.Effect<HealthCheckResult, StorageFailure> =>
3496− err.reauthRequired === true
3497− ? Effect.succeed({
3498− status: "expired",
3499− checkedAt: Date.now(),
3500− // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: CredentialResolutionError carries a typed `message` field
3501− detail: err.message,
3502− })
3503− : Effect.fail(
3504− new StorageError({
3505− // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: CredentialResolutionError carries a typed `message` field
3506− message: err.message,
3507− cause: err,
3508− }),
3509− );
3510−
35113493 const healthFromCredentialResolutionFailure = (
35123494 failure: CredentialResolutionError,
35133495 ): HealthCheckResult =>
@@ -3525,22 +3507,36 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 11 unmodified lines
35253507 detail: failure.message,
35263508 };
35273509
3528− // Genuine storage failures propagate: an infra blip must fail the request,
3529− // not persist as a "degraded" verdict on the connection.
3530− const oauthCredentialHealthWithoutProbe = (
3531− row: ConnectionRow,
3510+ /** THE one place a credential-resolution failure becomes a health verdict.
3511+ * A third party refusing to re-mint a credential is a fact about the
3512+ * CONNECTION, not a fault in this service, so it must be answered — and
3513+ * then persisted — as `expired`/`degraded`, never raised. The failure
3514+ * channel stays reserved for genuine storage faults (an infra blip must
3515+ * fail the request rather than persist as a "degraded" verdict). Both
3516+ * health paths — credential-only and probing — fold through here, so
3517+ * they cannot disagree about what a broken credential means. */
3518+ const foldCredentialResolutionIntoVerdict = (
3519+ probe: Effect.Effect<HealthCheckResult, StorageFailure | CredentialResolutionError>,
35323520 ): Effect.Effect<HealthCheckResult, StorageFailure> =>
3533− resolveConnectionValues(row).pipe(
3534− Effect.as({
3535− status: "healthy" as const,
3536− checkedAt: Date.now(),
3537− detail: "Credential resolved (no probe configured).",
3538− }),
3521+ probe.pipe(
35393522 Effect.catchTag("CredentialResolutionError", (failure) =>
35403523 Effect.succeed(healthFromCredentialResolutionFailure(failure)),
35413524 ),
35423525 );
35433526
3527+ const oauthCredentialHealthWithoutProbe = (
3528+ row: ConnectionRow,
3529+ ): Effect.Effect<HealthCheckResult, StorageFailure> =>
3530+ foldCredentialResolutionIntoVerdict(
3531+ resolveConnectionValues(row).pipe(
3532+ Effect.as({
3533+ status: "healthy" as const,
3534+ checkedAt: Date.now(),
3535+ detail: "Credential resolved (no probe configured).",
3536+ }),
3537+ ),
3538+ );
3539+
35443540 // Resolve an in-flight credential's value map (key-first validation) without
35453541 // saving anything. Mirrors `resolveConnectionValues` for the saved-row path:
35463542 // pasted `value`/`values` are used directly; `from` origins resolve through
@@ -3638,34 +3634,40 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 91 unmodified lines
36383634 return result;
36393635 }
36403636
3641− const result = yield* Effect.gen(function* () {
3642− const values = yield* resolveConnectionValues(connectionRow);
3643− const record = rowToIntegrationRecord(
3644− integrationRow,
3645− describeAuthMethodsForRow(integrationRow),
3646− );
3647− const grantedScopes = grantedScopesFromRow(connectionRow);
3648− const credential: ToolInvocationCredential = {
3649− owner: connectionRow.owner as Owner,
3650− integration: ref.integration,
3651− connection: ConnectionName.make(connectionRow.name),
3652− template: AuthTemplateSlug.make(connectionRow.template),
3653− value: values[PRIMARY_INPUT_VARIABLE] ?? null,
3654− values,
3655− config: record.config,
3656− ...(grantedScopes ? { grantedScopes } : {}),
3657− };
3658− // Core resolves the declared spec (its own column) and hands it to the
3659− // plugin; plugins no longer read it out of their config.
3660− return yield* foldPluginFailure(
3661− check({ ctx: runtime.ctx, integration: record, credential, spec }),
3662− `Health check for connection "${ref.name}" failed.`,
3663− );
3664− }).pipe(Effect.catchTag("CredentialResolutionError", healthFromCredentialResolutionError));
3637+ const result = yield* foldCredentialResolutionIntoVerdict(
3638+ Effect.gen(function* () {
3639+ const values = yield* resolveConnectionValues(connectionRow);
3640+ const record = rowToIntegrationRecord(
3641+ integrationRow,
3642+ describeAuthMethodsForRow(integrationRow),
3643+ );
3644+ const grantedScopes = grantedScopesFromRow(connectionRow);
3645+ const credential: ToolInvocationCredential = {
3646+ owner: connectionRow.owner as Owner,
3647+ integration: ref.integration,
3648+ connection: ConnectionName.make(connectionRow.name),
3649+ template: AuthTemplateSlug.make(connectionRow.template),
3650+ value: values[PRIMARY_INPUT_VARIABLE] ?? null,
3651+ values,
3652+ config: record.config,
3653+ ...(grantedScopes ? { grantedScopes } : {}),
3654+ };
3655+ // Core resolves the declared spec (its own column) and hands it to
3656+ // the plugin; plugins no longer read it out of their config.
3657+ return yield* foldPluginFailure(
3658+ check({ ctx: runtime.ctx, integration: record, credential, spec }),
3659+ `Health check for connection "${ref.name}" failed.`,
3660+ );
3661+ }),
3662+ );
36653663 yield* annotateHealthVerdict("probe", result);
36663664 // Persist the verdict on the connection row so the accounts list shows
3667− // alive/expired at a glance. Best-effort: a write failure must not turn
3668− // a successful probe into an error.
3665+ // alive/expired at a glance, AND so the freshness gate above has
3666+ // something to serve. A probe that could not resolve its credential
3667+ // persists too: it is the connection most likely to be re-probed by
3668+ // every surface on every mount, so leaving it unwritten is what turns
3669+ // one broken connection into unbounded upstream and error traffic.
3670+ // Best-effort: a write failure must not turn a verdict into an error.
36693671 yield* persistHealthResult(ref, result);
36703672 return result;
36713673 }).pipe(
packages/react/src/lib/use-connection-health.test.ts+48−0
@@ -0,0 +1,48 @@
1+import { describe, expect, it } from "@effect/vitest";
2+import type { HealthCheckResult } from "@executor-js/sdk/shared";
3+
4+import { HEALTH_REVALIDATE_MS, revalidateQuery } from "./use-connection-health";
5+
6+const verdict = (status: HealthCheckResult["status"]): HealthCheckResult => ({
7+ status,
8+ checkedAt: Date.now(),
9+});
10+
11+describe("revalidateQuery", () => {
12+ it("defers a healthy verdict to the server-enforced freshness window", () => {
13+ expect(revalidateQuery(verdict("healthy")).ifStaleMs, "the healthy window is sent").toBe(
14+ HEALTH_REVALIDATE_MS,
15+ );
16+ });
17+
18+ // The load-bearing case, and the reason this cannot become a short window.
19+ // Every non-healthy verdict is PERSISTED, so a request carrying `ifStaleMs`
20+ // would be answered from the row the previous probe wrote — "still expired" —
21+ // and the dot could not turn green until the window elapsed. Omitting the
22+ // window is what makes recovery show on the next load.
23+ it.each(["expired", "degraded", "unknown"] as const)(
24+ "forces a fresh probe for a %s verdict, so recovery shows on the next load",
25+ (status) => {
26+ expect(
27+ revalidateQuery(verdict(status)).ifStaleMs,
28+ "a non-healthy verdict must not be answered from the persisted verdict",
29+ ).toBeUndefined();
30+ },
31+ );
32+
33+ it("forces a fresh probe for a never-checked connection too", () => {
34+ expect(revalidateQuery(null).ifStaleMs, "a cleared verdict probes").toBeUndefined();
35+ expect(revalidateQuery(undefined).ifStaleMs, "a never-seen one probes").toBeUndefined();
36+ });
37+
38+ // An OAuth re-mint clears the persisted verdict, and the hook re-arms on that
39+ // clearing transition. If the resulting request carried a window it could be
40+ // answered from a verdict a pre-reconnect probe raced in afterwards, and the
41+ // reconnected row would keep reading Expired.
42+ it("never sends a window for anything but a healthy verdict", () => {
43+ const windows = (["expired", "degraded", "unknown"] as const).map(
44+ (status) => revalidateQuery(verdict(status)).ifStaleMs,
45+ );
46+ expect(windows, "only the healthy path is gated").toEqual([undefined, undefined, undefined]);
47+ });
48+});
packages/react/src/lib/use-connection-health.ts+14−2
@@ -36,8 +36,20 @@ const healthyAndFresh = (last: HealthCheckResult | null | undefined): boolean => · 35 unmodified lines
3636
3737 /** The revalidation query: a healthy (but stale) verdict defers to the
3838 * server-enforced window so N open tabs can't stampede the upstream; a
39− * missing or non-healthy verdict forces a fresh probe. */
40−const revalidateQuery = (
39+ * missing or non-healthy verdict forces a fresh probe.
40+ *
41+ * A non-healthy verdict deliberately sends NO window. Suppressing its probe
42+ * would suppress the only thing that can discover recovery: the verdict is
43+ * persisted, so a gated request would answer "still expired" from the row
44+ * the previous probe wrote, and the dot could not turn green until the window
45+ * elapsed. Recovery visibility is the contract these surfaces are built on
46+ * (see the health-checks-ui, graphql-introspection-health and
47+ * mcp-oauth-reconnect-health scenarios), so the upstream cost of re-probing a
48+ * broken connection is paid on purpose. What must NOT happen — one broken
49+ * connection raising a server error on every probe — is fixed where it
50+ * belongs, in the server folding a credential-resolution failure into a
51+ * persisted verdict rather than into the failure channel. */
52+export const revalidateQuery = (
4153 last: HealthCheckResult | null | undefined,
4254 ): { readonly ifStaleMs?: number } =>
4355 last?.status === "healthy" ? { ifStaleMs: HEALTH_REVALIDATE_MS } : {};

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t