From 5ed8ba42add6c9477bad83aa8c088426d7decf51 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:03:20 -0700 Subject: [PATCH 1/4] Persist connection health verdicts on failure and always send ifStaleMs --- .../connection-health-verdict.test.ts | 341 ++++++++++++++++++ packages/core/sdk/src/executor.ts | 110 +++--- .../src/lib/use-connection-health.test.ts | 59 +++ .../react/src/lib/use-connection-health.ts | 34 +- 4 files changed, 481 insertions(+), 63 deletions(-) create mode 100644 e2e/scenarios/connection-health-verdict.test.ts create mode 100644 packages/react/src/lib/use-connection-health.test.ts diff --git a/e2e/scenarios/connection-health-verdict.test.ts b/e2e/scenarios/connection-health-verdict.test.ts new file mode 100644 index 000000000..e98e4869a --- /dev/null +++ b/e2e/scenarios/connection-health-verdict.test.ts @@ -0,0 +1,341 @@ +// Cross-target: a connection whose credential cannot be resolved is a health +// VERDICT, not a failed request — and the verdict is persisted, so the next +// surface to ask reads it instead of hammering the authorization server. +// +// Production symptom: a handful of connections whose authorization server had +// stopped re-minting credentials produced hundreds of server errors, every one +// of them on `/api/connections/.../health`, plus unbounded refresh traffic to +// the third party. A probe whose credential resolution failed escaped before +// the verdict was written, so nothing was ever persisted, the freshness gate +// had nothing to serve, and every mount of every surface sent another refresh +// grant to a server that was already refusing. +// +// The journey: an OpenAPI integration completes a real authorization-code flow +// against a live test AS that mints instantly-expiring access tokens and +// refuses every refresh grant. A health check is configured on the +// integration, so the connection takes the probing path. The probe cannot get +// a credential — and must answer with a verdict, persist it, and let a +// freshness window keep the next check off the wire. Both refusals are +// covered: a retryable one (`degraded`) and a dead grant (`expired`). +import { randomBytes } from "node:crypto"; +import { createServer } from "node:http"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; +import { serveOAuthTestServer } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Target } from "../src/services"; + +const api = composePluginApi([openApiHttpPlugin()] as const); + +const unique = (prefix: string) => `${prefix}${randomBytes(4).toString("hex")}`; + +/** Upstream on 127.0.0.1 whose `GET /me` is the obvious health probe. The + * credential never resolves in this scenario, so a request reaching here at + * all would mean the probe ran with no token — the 401 keeps that honest. */ +const serveUpstream = () => + Effect.acquireRelease( + Effect.callback<{ readonly url: string; readonly close: () => void }>((resume) => { + const server = createServer((request, response) => { + if (request.method === "GET" && (request.url ?? "").startsWith("/me")) { + const authorized = (request.headers["authorization"] ?? "").startsWith("Bearer at_"); + response.writeHead(authorized ? 200 : 401, { "content-type": "application/json" }); + response.end(JSON.stringify(authorized ? { email: "probe@example.test" } : {})); + return; + } + response.writeHead(404, { "content-type": "application/json" }); + response.end(JSON.stringify({ error: "not_found" })); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resume( + Effect.succeed({ + url: `http://127.0.0.1:${port}`, + close: () => { + server.close(); + server.closeAllConnections(); + }, + }), + ); + }); + }), + (server) => Effect.sync(server.close), + ); + +const spec = ( + baseUrl: string, + oauth: { readonly authorizationEndpoint: string; readonly tokenEndpoint: string }, +): string => + JSON.stringify({ + openapi: "3.0.3", + info: { title: "Identity API", version: "1.0.0" }, + servers: [{ url: baseUrl }], + paths: { + "/me": { + get: { + operationId: "getMe", + summary: "The current account", + security: [{ oauth: ["identity.read"] }], + responses: { "200": { description: "account" } }, + }, + }, + }, + components: { + securitySchemes: { + oauth: { + type: "oauth2", + flows: { + authorizationCode: { + authorizationUrl: oauth.authorizationEndpoint, + tokenUrl: oauth.tokenEndpoint, + scopes: { "identity.read": "Read the account" }, + }, + }, + }, + }, + }, + }); + +scenario( + "Health checks · a connection whose refresh is refused reports a persisted verdict instead of failing the request", + {}, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const upstream = yield* serveUpstream(); + + /** One integration with a declared health check, one OAuth client, one + * connection completed through a real authorization-code flow — against + * an authorization server that refuses every refresh grant in the given + * way. Instantly-expiring access tokens mean every credential + * resolution must refresh, so the refusal is what the probe meets. */ + const connectRefusing = (options: { + readonly name: ConnectionName; + readonly errorCode: string; + readonly description: string; + }) => + Effect.gen(function* () { + const oauth = yield* serveOAuthTestServer({ + scopes: ["identity.read"], + tokenExpiresInSeconds: 0, + supportRefresh: false, + invalidRefreshTokenErrorCode: options.errorCode, + invalidRefreshTokenDescription: options.description, + }); + const slug = IntegrationSlug.make(unique("healthverdict")); + const clientSlug = OAuthClientSlug.make(unique("healthverdictc")); + + yield* Effect.addFinalizer(() => + Effect.all( + [ + client.connections + .remove({ params: { owner: "org", integration: slug, name: options.name } }) + .pipe(Effect.ignore), + client.oauth + .removeClient({ params: { slug: clientSlug }, payload: { owner: "org" } }) + .pipe(Effect.ignore), + client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore), + ], + { discard: true }, + ), + ); + + yield* client.openapi.addSpec({ + payload: { + spec: { kind: "blob", value: spec(upstream.url, oauth) }, + slug, + baseUrl: upstream.url, + authenticationTemplate: [ + { + slug: "oauth", + kind: "oauth2", + authorizationUrl: oauth.authorizationEndpoint, + tokenUrl: oauth.tokenEndpoint, + scopes: ["identity.read"], + }, + ], + }, + }); + + // Configure the probe, the way the user does in the editor: the + // ranked candidates offer the identity GET, and picking it is what + // sends this connection down the probing path. + const candidates = yield* client.integrations.healthCheckCandidates({ params: { slug } }); + const getMe = candidates.find((candidate) => candidate.method === "get"); + if (!getMe) return yield* Effect.die("the identity spec exposed no GET candidate"); + yield* client.integrations.healthCheckSet({ + params: { slug }, + payload: { spec: { operation: getMe.operation, identityField: "email" } }, + }); + + yield* client.oauth.createClient({ + payload: { + owner: "org", + slug: clientSlug, + grant: "authorization_code", + authorizationUrl: oauth.authorizationEndpoint, + tokenUrl: oauth.tokenEndpoint, + clientId: "test-client", + clientSecret: "test-secret", + originIntegration: slug, + }, + }); + + const started = yield* client.oauth.start({ + payload: { + client: clientSlug, + clientOwner: "org", + owner: "org", + name: options.name, + integration: slug, + template: AuthTemplateSlug.make("oauth"), + }, + }); + expect(started.status, "oauth.start redirects to the authorization server").toBe( + "redirect", + ); + if (started.status !== "redirect") return yield* Effect.die("no redirect"); + + // Drive the test IdP's consent by hand (authorize → login → code). + const code = yield* Effect.promise(async () => { + const authorize = await fetch(started.authorizationUrl, { redirect: "manual" }); + const loginUrl = authorize.headers.get("location"); + if (!loginUrl) throw new Error(`authorize did not redirect: ${authorize.status}`); + const login = await fetch(loginUrl, { + method: "POST", + headers: { + authorization: `Basic ${Buffer.from("alice:password").toString("base64")}`, + }, + redirect: "manual", + }); + const callbackUrl = login.headers.get("location"); + if (!callbackUrl) throw new Error(`login did not redirect: ${login.status}`); + const minted = new URL(callbackUrl).searchParams.get("code"); + if (!minted) throw new Error("callback carried no authorization code"); + return minted; + }); + yield* client.oauth.complete({ payload: { state: started.state, code } }); + yield* oauth.clearRequests; + + return { + slug, + /** Refresh grants the authorization server has actually received. + * This is the number the whole fix is about: probing must not + * scale with the number of surfaces asking. */ + refreshGrants: oauth.requests.pipe( + Effect.map( + (all) => + all.filter( + (request) => + request.path === "/token" && + request.method === "POST" && + request.body.includes("grant_type=refresh_token"), + ).length, + ), + ), + }; + }); + + // ── A refusal that is not "re-auth required" ──────────────────────── + // The AS answers the refresh with a code OTHER than invalid_grant, so + // retrying could in principle work. This is the shape that used to + // escape the probe as a server error. + const degradedName = ConnectionName.make("healthverdictrefused"); + const refused = yield* connectRefusing({ + name: degradedName, + errorCode: "invalid_request", + description: "Refresh temporarily unavailable", + }); + + // THE guarantee: a probe that cannot resolve its credential answers with + // a verdict. The request itself succeeds — a third party refusing a + // refresh is not a defect in this product, and reporting it as one is + // what buried the real signal. + const probed = yield* client.connections.checkHealth({ + params: { owner: "org", integration: refused.slug, name: degradedName }, + query: {}, + }); + expect( + probed.status, + "a connection whose refresh is refused reads degraded, not a failed request", + ).toBe("degraded"); + expect( + probed.detail ?? "", + "the verdict carries the authorization server's reason", + ).toContain("Refresh temporarily unavailable"); + expect(yield* refused.refreshGrants, "the probe did try the refresh exactly once").toBe(1); + + // The verdict PERSISTS, so the accounts list shows the state at a glance + // and the freshness gate has something to serve. + const stored = yield* client.connections.get({ + params: { owner: "org", integration: refused.slug, name: degradedName }, + }); + expect(stored?.lastHealth?.status, "the verdict is persisted on the connection").toBe( + "degraded", + ); + expect(stored?.lastHealth?.checkedAt, "and it is the verdict this probe produced").toBe( + probed.checkedAt, + ); + + // And repeated checks inside the freshness window — the window every + // surface now sends for a non-healthy verdict — are served from that + // persisted verdict: the authorization server sees nothing more. + for (let mount = 0; mount < 3; mount++) { + const remount = yield* client.connections.checkHealth({ + params: { owner: "org", integration: refused.slug, name: degradedName }, + query: { ifStaleMs: 30_000 }, + }); + expect(remount.status, "a repeat check inside the window keeps the verdict").toBe( + "degraded", + ); + expect(remount.checkedAt, "and it IS the persisted verdict, not a new probe").toBe( + probed.checkedAt, + ); + } + expect( + yield* refused.refreshGrants, + "repeated checks inside the window never reach the authorization server again", + ).toBe(1); + + // ── A dead grant ─────────────────────────────────────────────────── + // invalid_grant means the user must re-authenticate: a different verdict + // through the same probing path, and it persists the same way. + const expiredName = ConnectionName.make("healthverdictdead"); + const revoked = yield* connectRefusing({ + name: expiredName, + errorCode: "invalid_grant", + description: "Grant revoked", + }); + + const dead = yield* client.connections.checkHealth({ + params: { owner: "org", integration: revoked.slug, name: expiredName }, + query: {}, + }); + expect(dead.status, "a revoked grant reads expired, so the UI can offer reconnect").toBe( + "expired", + ); + expect(dead.detail ?? "", "the verdict carries the authorization server's reason").toContain( + "Grant revoked", + ); + const storedDead = yield* client.connections.get({ + params: { owner: "org", integration: revoked.slug, name: expiredName }, + }); + expect(storedDead?.lastHealth?.status, "the expired verdict is persisted too").toBe( + "expired", + ); + }), + ), +); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 2ce9a3922..c67d73ca3 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3454,24 +3454,6 @@ export const createExecutor = => - err.reauthRequired === true - ? Effect.succeed({ - status: "expired", - checkedAt: Date.now(), - // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: CredentialResolutionError carries a typed `message` field - detail: err.message, - }) - : Effect.fail( - new StorageError({ - // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: CredentialResolutionError carries a typed `message` field - message: err.message, - cause: err, - }), - ); - const healthFromCredentialResolutionFailure = ( failure: CredentialResolutionError, ): HealthCheckResult => @@ -3489,22 +3471,36 @@ export const createExecutor = , ): Effect.Effect => - resolveConnectionValues(row).pipe( - Effect.as({ - status: "healthy" as const, - checkedAt: Date.now(), - detail: "Credential resolved (no probe configured).", - }), + probe.pipe( Effect.catchTag("CredentialResolutionError", (failure) => Effect.succeed(healthFromCredentialResolutionFailure(failure)), ), ); + const oauthCredentialHealthWithoutProbe = ( + row: ConnectionRow, + ): Effect.Effect => + foldCredentialResolutionIntoVerdict( + resolveConnectionValues(row).pipe( + Effect.as({ + status: "healthy" as const, + checkedAt: Date.now(), + detail: "Credential resolved (no probe configured).", + }), + ), + ); + // Resolve an in-flight credential's value map (key-first validation) without // saving anything. Mirrors `resolveConnectionValues` for the saved-row path: // pasted `value`/`values` are used directly; `from` origins resolve through @@ -3602,34 +3598,40 @@ export const createExecutor = ({ + status, + checkedAt: Date.now(), +}); + +describe("revalidateQuery", () => { + it("defers a healthy verdict to the long freshness window", () => { + expect(revalidateQuery(verdict("healthy")).ifStaleMs, "the healthy window is sent").toBe( + HEALTH_REVALIDATE_MS, + ); + }); + + // The load-bearing case. A connection whose credential is broken is exactly + // the connection every mount wants to re-probe, and each probe is a fresh + // request to an upstream that is already refusing. Sending a SHORT window + // instead of none keeps recovery visible while letting the server's + // freshness gate collapse repeated mounts and concurrent tabs into one probe. + it.each(["expired", "degraded", "unknown"] as const)( + "still sends a short window for a %s verdict, so repeated mounts cannot stampede the upstream", + (status) => { + const window = revalidateQuery(verdict(status)).ifStaleMs; + expect( + window, + "a non-healthy verdict sends a freshness window, not an unconditional probe", + ).toBeGreaterThan(0); + expect(window, "and it is the short non-healthy window").toBe(HEALTH_REVALIDATE_UNHEALTHY_MS); + }, + ); + + it("sends the short window for a never-checked connection too", () => { + // Nothing is persisted, so the server has no cached verdict to serve and + // probes regardless — but a second surface mounting moments later is + // covered by the verdict the first one just wrote. + expect( + revalidateQuery(null).ifStaleMs, + "a missing verdict still sends a window", + ).toBeGreaterThan(0); + expect(revalidateQuery(null).ifStaleMs, "and it is the short one").toBe( + HEALTH_REVALIDATE_UNHEALTHY_MS, + ); + expect(revalidateQuery(undefined).ifStaleMs, "a never-seen one behaves the same").toBe( + HEALTH_REVALIDATE_UNHEALTHY_MS, + ); + }); + + it("keeps the unhealthy window far shorter than the healthy one, so recovery still shows up", () => { + expect(HEALTH_REVALIDATE_UNHEALTHY_MS).toBeGreaterThan(0); + expect(HEALTH_REVALIDATE_UNHEALTHY_MS).toBeLessThan(HEALTH_REVALIDATE_MS); + }); +}); diff --git a/packages/react/src/lib/use-connection-health.ts b/packages/react/src/lib/use-connection-health.ts index 57b3e8ca1..12e8c8ab3 100644 --- a/packages/react/src/lib/use-connection-health.ts +++ b/packages/react/src/lib/use-connection-health.ts @@ -20,27 +20,43 @@ import { connectionCheckKeys } from "../api/reactivity-keys"; * path too, so concurrent tabs collapse to one probe. */ export const HEALTH_REVALIDATE_MS = 5 * 60 * 1000; +/** Freshness window for a NON-healthy verdict. Short, because an expired dot is + * exactly the verdict the user is waiting to see change — but non-zero, + * because a broken connection is the one every surface wants to re-probe and + * every probe is another request to an upstream that is already refusing. + * Sending this lets the server-side gate collapse repeated mounts and + * concurrent tabs into one probe per window. */ +export const HEALTH_REVALIDATE_UNHEALTHY_MS = 30 * 1000; + const connectionParams = (connection: Connection) => ({ owner: connection.owner, integration: connection.integration, name: connection.name, }); -/** Whether a persisted verdict may render as-is without a background probe. - * Healthy-and-fresh renders untouched. Everything else revalidates: stale or +/** Whether a persisted verdict may render as-is without asking at all. + * Healthy-and-fresh renders untouched. Everything else asks: stale or * never-checked for obvious reasons, and NON-healthy always; an expired dot * is exactly the verdict the user is waiting to see change, so recovery must - * show on the next load, not after the freshness window. */ + * show on the next load, not after the long window. Asking is not the same as + * probing — the request carries a short `ifStaleMs` (see revalidateQuery), so + * the server answers from the persisted verdict unless it has gone stale. */ const healthyAndFresh = (last: HealthCheckResult | null | undefined): boolean => last?.status === "healthy" && Date.now() - last.checkedAt < HEALTH_REVALIDATE_MS; -/** The revalidation query: a healthy (but stale) verdict defers to the - * server-enforced window so N open tabs can't stampede the upstream; a - * missing or non-healthy verdict forces a fresh probe. */ -const revalidateQuery = ( +/** The revalidation query. ALWAYS defers to the server-enforced freshness + * window, only the width changes: long for a healthy verdict, short for + * anything else. Omitting it for non-healthy verdicts (as this once did) + * bypassed the gate for precisely the connections that were failing — the + * once-per-mount client guard is per MOUNT, not per verdict, so two surfaces + * rendering the same broken connection each sent their own probe, and each + * probe was another refused token request and another captured server error. + * Manual "Check now" is the only unconditional probe, and it passes `{}`. */ +export const revalidateQuery = ( last: HealthCheckResult | null | undefined, -): { readonly ifStaleMs?: number } => - last?.status === "healthy" ? { ifStaleMs: HEALTH_REVALIDATE_MS } : {}; +): { readonly ifStaleMs: number } => ({ + ifStaleMs: last?.status === "healthy" ? HEALTH_REVALIDATE_MS : HEALTH_REVALIDATE_UNHEALTHY_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` From 62dd890e74fe7f609b79dcd1fa9c5c2140431c65 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:49:43 -0700 Subject: [PATCH 2/4] Cover the client half of the health-verdict fix with a browser scenario --- .../connection-health-verdict.test.ts | 418 ++++++++++++------ 1 file changed, 280 insertions(+), 138 deletions(-) diff --git a/e2e/scenarios/connection-health-verdict.test.ts b/e2e/scenarios/connection-health-verdict.test.ts index e98e4869a..81beca9d8 100644 --- a/e2e/scenarios/connection-health-verdict.test.ts +++ b/e2e/scenarios/connection-health-verdict.test.ts @@ -17,11 +17,17 @@ // a credential — and must answer with a verdict, persist it, and let a // freshness window keep the next check off the wire. Both refusals are // covered: a retryable one (`degraded`) and a dead grant (`expired`). +// +// A second scenario in this file walks the same broken connection through the +// real UI, because the other half of the symptom was the number of SURFACES: +// see its header. import { randomBytes } from "node:crypto"; import { createServer } from "node:http"; import { expect } from "@effect/vitest"; import { Effect } from "effect"; +import type { HttpApiClient } from "effect/unstable/httpapi"; +import type { Response } from "playwright"; import { composePluginApi } from "@executor-js/api/server"; import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; import { @@ -33,9 +39,11 @@ import { import { serveOAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; -import { Api, Target } from "../src/services"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); +type Client = HttpApiClient.ForApi; const unique = (prefix: string) => `${prefix}${randomBytes(4).toString("hex")}`; @@ -48,7 +56,9 @@ const serveUpstream = () => const server = createServer((request, response) => { if (request.method === "GET" && (request.url ?? "").startsWith("/me")) { const authorized = (request.headers["authorization"] ?? "").startsWith("Bearer at_"); - response.writeHead(authorized ? 200 : 401, { "content-type": "application/json" }); + response.writeHead(authorized ? 200 : 401, { + "content-type": "application/json", + }); response.end(JSON.stringify(authorized ? { email: "probe@example.test" } : {})); return; } @@ -74,7 +84,10 @@ const serveUpstream = () => const spec = ( baseUrl: string, - oauth: { readonly authorizationEndpoint: string; readonly tokenEndpoint: string }, + oauth: { + readonly authorizationEndpoint: string; + readonly tokenEndpoint: string; + }, ): string => JSON.stringify({ openapi: "3.0.3", @@ -106,6 +119,149 @@ const spec = ( }, }); +/** One integration with a declared health check, one OAuth client, one + * connection completed through a real authorization-code flow — against an + * authorization server that refuses every refresh grant in the given way. + * Instantly-expiring access tokens mean every credential resolution must + * refresh, so the refusal is what the probe meets. */ +const connectRefusing = ( + client: Client, + upstream: { readonly url: string }, + options: { + readonly name: ConnectionName; + readonly errorCode: string; + readonly description: string; + }, +) => + Effect.gen(function* () { + const oauth = yield* serveOAuthTestServer({ + scopes: ["identity.read"], + tokenExpiresInSeconds: 0, + supportRefresh: false, + invalidRefreshTokenErrorCode: options.errorCode, + invalidRefreshTokenDescription: options.description, + }); + const slug = IntegrationSlug.make(unique("healthverdict")); + const clientSlug = OAuthClientSlug.make(unique("healthverdictc")); + + yield* Effect.addFinalizer(() => + Effect.all( + [ + client.connections + .remove({ + params: { owner: "org", integration: slug, name: options.name }, + }) + .pipe(Effect.ignore), + client.oauth + .removeClient({ + params: { slug: clientSlug }, + payload: { owner: "org" }, + }) + .pipe(Effect.ignore), + client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore), + ], + { discard: true }, + ), + ); + + yield* client.openapi.addSpec({ + payload: { + spec: { kind: "blob", value: spec(upstream.url, oauth) }, + slug, + baseUrl: upstream.url, + authenticationTemplate: [ + { + slug: "oauth", + kind: "oauth2", + authorizationUrl: oauth.authorizationEndpoint, + tokenUrl: oauth.tokenEndpoint, + scopes: ["identity.read"], + }, + ], + }, + }); + + // Configure the probe, the way the user does in the editor: the + // ranked candidates offer the identity GET, and picking it is what + // sends this connection down the probing path. + const candidates = yield* client.integrations.healthCheckCandidates({ + params: { slug }, + }); + const getMe = candidates.find((candidate) => candidate.method === "get"); + if (!getMe) return yield* Effect.die("the identity spec exposed no GET candidate"); + yield* client.integrations.healthCheckSet({ + params: { slug }, + payload: { spec: { operation: getMe.operation, identityField: "email" } }, + }); + + yield* client.oauth.createClient({ + payload: { + owner: "org", + slug: clientSlug, + grant: "authorization_code", + authorizationUrl: oauth.authorizationEndpoint, + tokenUrl: oauth.tokenEndpoint, + clientId: "test-client", + clientSecret: "test-secret", + originIntegration: slug, + }, + }); + + const started = yield* client.oauth.start({ + payload: { + client: clientSlug, + clientOwner: "org", + owner: "org", + name: options.name, + integration: slug, + template: AuthTemplateSlug.make("oauth"), + }, + }); + expect(started.status, "oauth.start redirects to the authorization server").toBe("redirect"); + if (started.status !== "redirect") return yield* Effect.die("no redirect"); + + // Drive the test IdP's consent by hand (authorize → login → code). + const code = yield* Effect.promise(async () => { + const authorize = await fetch(started.authorizationUrl, { + redirect: "manual", + }); + const loginUrl = authorize.headers.get("location"); + if (!loginUrl) throw new Error(`authorize did not redirect: ${authorize.status}`); + const login = await fetch(loginUrl, { + method: "POST", + headers: { + authorization: `Basic ${Buffer.from("alice:password").toString("base64")}`, + }, + redirect: "manual", + }); + const callbackUrl = login.headers.get("location"); + if (!callbackUrl) throw new Error(`login did not redirect: ${login.status}`); + const minted = new URL(callbackUrl).searchParams.get("code"); + if (!minted) throw new Error("callback carried no authorization code"); + return minted; + }); + yield* client.oauth.complete({ payload: { state: started.state, code } }); + yield* oauth.clearRequests; + + return { + slug, + /** Refresh grants the authorization server has actually received. + * This is the number the whole fix is about: probing must not + * scale with the number of surfaces asking. */ + refreshGrants: oauth.requests.pipe( + Effect.map( + (all) => + all.filter( + (request) => + request.path === "/token" && + request.method === "POST" && + request.body.includes("grant_type=refresh_token"), + ).length, + ), + ), + }; + }); + scenario( "Health checks · a connection whose refresh is refused reports a persisted verdict instead of failing the request", {}, @@ -117,144 +273,12 @@ scenario( const client = yield* makeClient(api, identity); const upstream = yield* serveUpstream(); - /** One integration with a declared health check, one OAuth client, one - * connection completed through a real authorization-code flow — against - * an authorization server that refuses every refresh grant in the given - * way. Instantly-expiring access tokens mean every credential - * resolution must refresh, so the refusal is what the probe meets. */ - const connectRefusing = (options: { - readonly name: ConnectionName; - readonly errorCode: string; - readonly description: string; - }) => - Effect.gen(function* () { - const oauth = yield* serveOAuthTestServer({ - scopes: ["identity.read"], - tokenExpiresInSeconds: 0, - supportRefresh: false, - invalidRefreshTokenErrorCode: options.errorCode, - invalidRefreshTokenDescription: options.description, - }); - const slug = IntegrationSlug.make(unique("healthverdict")); - const clientSlug = OAuthClientSlug.make(unique("healthverdictc")); - - yield* Effect.addFinalizer(() => - Effect.all( - [ - client.connections - .remove({ params: { owner: "org", integration: slug, name: options.name } }) - .pipe(Effect.ignore), - client.oauth - .removeClient({ params: { slug: clientSlug }, payload: { owner: "org" } }) - .pipe(Effect.ignore), - client.openapi.removeSpec({ params: { slug } }).pipe(Effect.ignore), - ], - { discard: true }, - ), - ); - - yield* client.openapi.addSpec({ - payload: { - spec: { kind: "blob", value: spec(upstream.url, oauth) }, - slug, - baseUrl: upstream.url, - authenticationTemplate: [ - { - slug: "oauth", - kind: "oauth2", - authorizationUrl: oauth.authorizationEndpoint, - tokenUrl: oauth.tokenEndpoint, - scopes: ["identity.read"], - }, - ], - }, - }); - - // Configure the probe, the way the user does in the editor: the - // ranked candidates offer the identity GET, and picking it is what - // sends this connection down the probing path. - const candidates = yield* client.integrations.healthCheckCandidates({ params: { slug } }); - const getMe = candidates.find((candidate) => candidate.method === "get"); - if (!getMe) return yield* Effect.die("the identity spec exposed no GET candidate"); - yield* client.integrations.healthCheckSet({ - params: { slug }, - payload: { spec: { operation: getMe.operation, identityField: "email" } }, - }); - - yield* client.oauth.createClient({ - payload: { - owner: "org", - slug: clientSlug, - grant: "authorization_code", - authorizationUrl: oauth.authorizationEndpoint, - tokenUrl: oauth.tokenEndpoint, - clientId: "test-client", - clientSecret: "test-secret", - originIntegration: slug, - }, - }); - - const started = yield* client.oauth.start({ - payload: { - client: clientSlug, - clientOwner: "org", - owner: "org", - name: options.name, - integration: slug, - template: AuthTemplateSlug.make("oauth"), - }, - }); - expect(started.status, "oauth.start redirects to the authorization server").toBe( - "redirect", - ); - if (started.status !== "redirect") return yield* Effect.die("no redirect"); - - // Drive the test IdP's consent by hand (authorize → login → code). - const code = yield* Effect.promise(async () => { - const authorize = await fetch(started.authorizationUrl, { redirect: "manual" }); - const loginUrl = authorize.headers.get("location"); - if (!loginUrl) throw new Error(`authorize did not redirect: ${authorize.status}`); - const login = await fetch(loginUrl, { - method: "POST", - headers: { - authorization: `Basic ${Buffer.from("alice:password").toString("base64")}`, - }, - redirect: "manual", - }); - const callbackUrl = login.headers.get("location"); - if (!callbackUrl) throw new Error(`login did not redirect: ${login.status}`); - const minted = new URL(callbackUrl).searchParams.get("code"); - if (!minted) throw new Error("callback carried no authorization code"); - return minted; - }); - yield* client.oauth.complete({ payload: { state: started.state, code } }); - yield* oauth.clearRequests; - - return { - slug, - /** Refresh grants the authorization server has actually received. - * This is the number the whole fix is about: probing must not - * scale with the number of surfaces asking. */ - refreshGrants: oauth.requests.pipe( - Effect.map( - (all) => - all.filter( - (request) => - request.path === "/token" && - request.method === "POST" && - request.body.includes("grant_type=refresh_token"), - ).length, - ), - ), - }; - }); - // ── A refusal that is not "re-auth required" ──────────────────────── // The AS answers the refresh with a code OTHER than invalid_grant, so // retrying could in principle work. This is the shape that used to // escape the probe as a server error. const degradedName = ConnectionName.make("healthverdictrefused"); - const refused = yield* connectRefusing({ + const refused = yield* connectRefusing(client, upstream, { name: degradedName, errorCode: "invalid_request", description: "Refresh temporarily unavailable", @@ -295,7 +319,11 @@ scenario( // persisted verdict: the authorization server sees nothing more. for (let mount = 0; mount < 3; mount++) { const remount = yield* client.connections.checkHealth({ - params: { owner: "org", integration: refused.slug, name: degradedName }, + params: { + owner: "org", + integration: refused.slug, + name: degradedName, + }, query: { ifStaleMs: 30_000 }, }); expect(remount.status, "a repeat check inside the window keeps the verdict").toBe( @@ -314,7 +342,7 @@ scenario( // invalid_grant means the user must re-authenticate: a different verdict // through the same probing path, and it persists the same way. const expiredName = ConnectionName.make("healthverdictdead"); - const revoked = yield* connectRefusing({ + const revoked = yield* connectRefusing(client, upstream, { name: expiredName, errorCode: "invalid_grant", description: "Grant revoked", @@ -339,3 +367,117 @@ scenario( }), ), ); + +// =========================================================================== +// The other half of the production symptom, through the real UI: the refresh +// traffic scaled with the number of SURFACES showing the broken connection. +// The once-per-mount client guard is per mount, not per connection, so the +// integration page and the integrations list each sent their own health +// request — and because the client omitted `ifStaleMs` for exactly the +// non-healthy verdicts, each of those requests was a full probe and another +// refused refresh grant at the third party. +// +// Nothing here touches the hook: the browser navigates between the two +// surfaces a user actually visits, and the authorization server's own request +// ledger is the judge. Skips on targets with no browser surface. +// =========================================================================== + +scenario( + "Health checks (UI) · a second surface showing a broken connection reads the verdict instead of re-probing", + {}, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const upstream = yield* serveUpstream(); + + const name = ConnectionName.make("healthverdictui"); + const refused = yield* connectRefusing(client, upstream, { + name, + errorCode: "invalid_request", + description: "Refresh temporarily unavailable", + }); + + yield* browser.session(identity, async ({ page, step }) => { + const connections = page.locator("section").filter({ + has: page.getByRole("heading", { level: 3, name: "Connections" }), + }); + // Every health request the app sends, in order, with its query string: + // the client's own wire contract, observed from outside. + const healthRequests: string[] = []; + page.on("request", (request) => { + if (request.method() === "POST" && request.url().includes("/health")) { + healthRequests.push(request.url()); + } + }); + const isHealthResponse = (response: Response) => + response.request().method() === "POST" && response.url().includes("/health"); + const refreshGrants = () => Effect.runPromise(refused.refreshGrants); + + // Surface 1. No verdict is persisted yet, so this load's automatic + // check is the probe that discovers the broken credential — the badge + // appearing is proof it completed and was written down. + await step("Open the integration: the broken connection reads Degraded", async () => { + const settled = page.waitForResponse(isHealthResponse, { + timeout: 30_000, + }); + await visit(page, `/integrations/${refused.slug}`); + await settled; + await connections.getByText("Degraded", { exact: true }).waitFor({ timeout: 30_000 }); + }); + + // The baseline is taken AFTER that probe, so the freshness window is + // running from a verdict written moments ago and the assertion below + // does not depend on how long the browser took to start. + const baseline = await refreshGrants(); + expect(baseline, "the first surface did probe the authorization server").toBeGreaterThan(0); + const afterFirstSurface = healthRequests.length; + + // Surface 2: the integrations list summarises the same connection's + // health, and mounts its own revalidation. + await step("Open the integrations list, which shows the same connection", async () => { + const settled = page.waitForResponse(isHealthResponse, { + timeout: 30_000, + }); + await visit(page, "/"); + await settled; + }); + + // Surface 3: back to the integration page — a fresh mount again. + await step("Return to the integration page: another mount, another ask", async () => { + const settled = page.waitForResponse(isHealthResponse, { + timeout: 30_000, + }); + await visit(page, `/integrations/${refused.slug}`); + await settled; + await connections.getByText("Degraded", { exact: true }).waitFor({ timeout: 30_000 }); + }); + + // THE production symptom, stated as the third party experiences it: + // two more surfaces rendered the same broken connection and the + // authorization server heard nothing more about it. + expect( + await refreshGrants(), + "no later surface reached the authorization server again", + ).toBe(baseline); + + const laterRequests = healthRequests.slice(afterFirstSurface); + expect( + laterRequests.length, + "and they did ask about the connection's health — silence would be the wrong cure", + ).toBeGreaterThan(0); + // Asking is fine. Asking WITHOUT a freshness window is what turned one + // broken connection into unbounded refresh traffic. + for (const url of laterRequests) { + expect( + new URL(url).searchParams.get("ifStaleMs"), + `an automatic health request carries a freshness window (${url})`, + ).not.toBeNull(); + } + }); + }), + ), +); From dfb3e0592410a161e9a1d2b2bf3cbce426cc3cc2 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:28:23 -0700 Subject: [PATCH 3/4] 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. --- .../connection-health-verdict.test.ts | 91 +++++++++++++------ .../src/lib/use-connection-health.test.ts | 57 +++++------- .../react/src/lib/use-connection-health.ts | 44 ++++----- 3 files changed, 105 insertions(+), 87 deletions(-) diff --git a/e2e/scenarios/connection-health-verdict.test.ts b/e2e/scenarios/connection-health-verdict.test.ts index 81beca9d8..cd4c15e59 100644 --- a/e2e/scenarios/connection-health-verdict.test.ts +++ b/e2e/scenarios/connection-health-verdict.test.ts @@ -314,9 +314,12 @@ scenario( probed.checkedAt, ); - // And repeated checks inside the freshness window — the window every - // surface now sends for a non-healthy verdict — are served from that - // persisted verdict: the authorization server sees nothing more. + // And because it persisted, a caller that DOES pass a freshness window is + // served from it: the server's gate has something to answer with, so the + // authorization server sees nothing more. (The automatic client path + // deliberately passes no window for a non-healthy verdict — recovery has + // to be able to show — but the gate itself must work for the callers that + // opt into it.) for (let mount = 0; mount < 3; mount++) { const remount = yield* client.connections.checkHealth({ params: { @@ -369,13 +372,19 @@ scenario( ); // =========================================================================== -// The other half of the production symptom, through the real UI: the refresh -// traffic scaled with the number of SURFACES showing the broken connection. -// The once-per-mount client guard is per mount, not per connection, so the -// integration page and the integrations list each sent their own health -// request — and because the client omitted `ifStaleMs` for exactly the -// non-healthy verdicts, each of those requests was a full probe and another -// refused refresh grant at the third party. +// The other half of the production symptom, through the real UI. A connection +// whose refresh is refused is rendered by several surfaces (the integration +// page's Connections list and the integrations list summary), and each one +// revalidates on mount. Before the fix every one of those probes escaped as a +// SERVER ERROR instead of a verdict, so one broken connection produced a +// stream of captured errors and no surface could show the user what was wrong. +// +// What this pins down is that shape, not the volume: every automatic health +// request succeeds and every surface paints the same Degraded verdict, while +// the per-connection guard keeps each surface to exactly ONE probe — the +// no-probe-storm invariant. Re-probing a broken connection once per surface is +// the deliberate price of letting recovery show on the next load; an +// unbounded loop is the regression worth catching. // // Nothing here touches the hook: the browser navigates between the two // surfaces a user actually visits, and the authorization server's own request @@ -383,7 +392,7 @@ scenario( // =========================================================================== scenario( - "Health checks (UI) · a second surface showing a broken connection reads the verdict instead of re-probing", + "Health checks (UI) · every surface showing a broken connection paints a verdict, and probes exactly once", {}, Effect.scoped( Effect.gen(function* () { @@ -405,14 +414,26 @@ scenario( const connections = page.locator("section").filter({ has: page.getByRole("heading", { level: 3, name: "Connections" }), }); - // Every health request the app sends, in order, with its query string: - // the client's own wire contract, observed from outside. + // Every health request the app sends, with the status it came back + // with: the client's own wire contract, observed from outside. A + // credential the third party refuses must still produce a SUCCESSFUL + // health response carrying a verdict — that is the whole fix. const healthRequests: string[] = []; + const healthFailures: string[] = []; page.on("request", (request) => { if (request.method() === "POST" && request.url().includes("/health")) { healthRequests.push(request.url()); } }); + page.on("response", (response) => { + if ( + response.request().method() === "POST" && + response.url().includes("/health") && + response.status() >= 400 + ) { + healthFailures.push(`${String(response.status())} ${response.url()}`); + } + }); const isHealthResponse = (response: Response) => response.request().method() === "POST" && response.url().includes("/health"); const refreshGrants = () => Effect.runPromise(refused.refreshGrants); @@ -446,6 +467,9 @@ scenario( await settled; }); + // What the LIST surface cost at the authorization server. + const afterList = await refreshGrants(); + // Surface 3: back to the integration page — a fresh mount again. await step("Return to the integration page: another mount, another ask", async () => { const settled = page.waitForResponse(isHealthResponse, { @@ -456,27 +480,36 @@ scenario( await connections.getByText("Degraded", { exact: true }).waitFor({ timeout: 30_000 }); }); - // THE production symptom, stated as the third party experiences it: - // two more surfaces rendered the same broken connection and the - // authorization server heard nothing more about it. - expect( - await refreshGrants(), - "no later surface reached the authorization server again", - ).toBe(baseline); + const afterReturn = await refreshGrants(); const laterRequests = healthRequests.slice(afterFirstSurface); expect( laterRequests.length, - "and they did ask about the connection's health — silence would be the wrong cure", + "the later surfaces did ask about the connection's health — silence would mean a broken connection could never be seen to recover", ).toBeGreaterThan(0); - // Asking is fine. Asking WITHOUT a freshness window is what turned one - // broken connection into unbounded refresh traffic. - for (const url of laterRequests) { - expect( - new URL(url).searchParams.get("ifStaleMs"), - `an automatic health request carries a freshness window (${url})`, - ).not.toBeNull(); - } + + // THE production symptom, stated as the third party experiences it. + // Not "the later surfaces went silent" — they must not, or recovery + // could never show — but "each surface costs a fixed, bounded amount". + // Surface 3 is the SAME page as surface 1, so it must cost exactly what + // surface 1 cost; anything more is the per-connection guard failing and + // the effect re-probing in a loop. + expect( + afterList - baseline, + "the list surface revalidated the broken connection", + ).toBeGreaterThan(0); + expect( + afterReturn - afterList, + "revisiting the same page costs the same one probe per connection it cost the first time, not a growing storm", + ).toBe(baseline); + + // And the shape that actually reached production: a refused refresh is + // answered, not raised. Every one of these requests used to come back + // as a server error, which is what buried the signal. + expect( + healthFailures, + "no automatic health request failed; a refused refresh is a verdict, not an error", + ).toEqual([]); }); }), ), diff --git a/packages/react/src/lib/use-connection-health.test.ts b/packages/react/src/lib/use-connection-health.test.ts index 9f225ef4e..89e5d0840 100644 --- a/packages/react/src/lib/use-connection-health.test.ts +++ b/packages/react/src/lib/use-connection-health.test.ts @@ -1,11 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; import type { HealthCheckResult } from "@executor-js/sdk/shared"; -import { - HEALTH_REVALIDATE_MS, - HEALTH_REVALIDATE_UNHEALTHY_MS, - revalidateQuery, -} from "./use-connection-health"; +import { HEALTH_REVALIDATE_MS, revalidateQuery } from "./use-connection-health"; const verdict = (status: HealthCheckResult["status"]): HealthCheckResult => ({ status, @@ -13,47 +9,40 @@ const verdict = (status: HealthCheckResult["status"]): HealthCheckResult => ({ }); describe("revalidateQuery", () => { - it("defers a healthy verdict to the long freshness window", () => { + it("defers a healthy verdict to the server-enforced freshness window", () => { expect(revalidateQuery(verdict("healthy")).ifStaleMs, "the healthy window is sent").toBe( HEALTH_REVALIDATE_MS, ); }); - // The load-bearing case. A connection whose credential is broken is exactly - // the connection every mount wants to re-probe, and each probe is a fresh - // request to an upstream that is already refusing. Sending a SHORT window - // instead of none keeps recovery visible while letting the server's - // freshness gate collapse repeated mounts and concurrent tabs into one probe. + // The load-bearing case, and the reason this cannot become a short window. + // Every non-healthy verdict is PERSISTED, so a request carrying `ifStaleMs` + // would be answered from the row the previous probe wrote — "still expired" — + // and the dot could not turn green until the window elapsed. Omitting the + // window is what makes recovery show on the next load. it.each(["expired", "degraded", "unknown"] as const)( - "still sends a short window for a %s verdict, so repeated mounts cannot stampede the upstream", + "forces a fresh probe for a %s verdict, so recovery shows on the next load", (status) => { - const window = revalidateQuery(verdict(status)).ifStaleMs; expect( - window, - "a non-healthy verdict sends a freshness window, not an unconditional probe", - ).toBeGreaterThan(0); - expect(window, "and it is the short non-healthy window").toBe(HEALTH_REVALIDATE_UNHEALTHY_MS); + revalidateQuery(verdict(status)).ifStaleMs, + "a non-healthy verdict must not be answered from the persisted verdict", + ).toBeUndefined(); }, ); - it("sends the short window for a never-checked connection too", () => { - // Nothing is persisted, so the server has no cached verdict to serve and - // probes regardless — but a second surface mounting moments later is - // covered by the verdict the first one just wrote. - expect( - revalidateQuery(null).ifStaleMs, - "a missing verdict still sends a window", - ).toBeGreaterThan(0); - expect(revalidateQuery(null).ifStaleMs, "and it is the short one").toBe( - HEALTH_REVALIDATE_UNHEALTHY_MS, - ); - expect(revalidateQuery(undefined).ifStaleMs, "a never-seen one behaves the same").toBe( - HEALTH_REVALIDATE_UNHEALTHY_MS, - ); + it("forces a fresh probe for a never-checked connection too", () => { + expect(revalidateQuery(null).ifStaleMs, "a cleared verdict probes").toBeUndefined(); + expect(revalidateQuery(undefined).ifStaleMs, "a never-seen one probes").toBeUndefined(); }); - it("keeps the unhealthy window far shorter than the healthy one, so recovery still shows up", () => { - expect(HEALTH_REVALIDATE_UNHEALTHY_MS).toBeGreaterThan(0); - expect(HEALTH_REVALIDATE_UNHEALTHY_MS).toBeLessThan(HEALTH_REVALIDATE_MS); + // An OAuth re-mint clears the persisted verdict, and the hook re-arms on that + // clearing transition. If the resulting request carried a window it could be + // answered from a verdict a pre-reconnect probe raced in afterwards, and the + // reconnected row would keep reading Expired. + it("never sends a window for anything but a healthy verdict", () => { + const windows = (["expired", "degraded", "unknown"] as const).map( + (status) => revalidateQuery(verdict(status)).ifStaleMs, + ); + expect(windows, "only the healthy path is gated").toEqual([undefined, undefined, undefined]); }); }); diff --git a/packages/react/src/lib/use-connection-health.ts b/packages/react/src/lib/use-connection-health.ts index 12e8c8ab3..9bfc1fe36 100644 --- a/packages/react/src/lib/use-connection-health.ts +++ b/packages/react/src/lib/use-connection-health.ts @@ -20,43 +20,39 @@ import { connectionCheckKeys } from "../api/reactivity-keys"; * path too, so concurrent tabs collapse to one probe. */ export const HEALTH_REVALIDATE_MS = 5 * 60 * 1000; -/** Freshness window for a NON-healthy verdict. Short, because an expired dot is - * exactly the verdict the user is waiting to see change — but non-zero, - * because a broken connection is the one every surface wants to re-probe and - * every probe is another request to an upstream that is already refusing. - * Sending this lets the server-side gate collapse repeated mounts and - * concurrent tabs into one probe per window. */ -export const HEALTH_REVALIDATE_UNHEALTHY_MS = 30 * 1000; - const connectionParams = (connection: Connection) => ({ owner: connection.owner, integration: connection.integration, name: connection.name, }); -/** Whether a persisted verdict may render as-is without asking at all. - * Healthy-and-fresh renders untouched. Everything else asks: stale or +/** Whether a persisted verdict may render as-is without a background probe. + * Healthy-and-fresh renders untouched. Everything else revalidates: stale or * never-checked for obvious reasons, and NON-healthy always; an expired dot * is exactly the verdict the user is waiting to see change, so recovery must - * show on the next load, not after the long window. Asking is not the same as - * probing — the request carries a short `ifStaleMs` (see revalidateQuery), so - * the server answers from the persisted verdict unless it has gone stale. */ + * show on the next load, not after the freshness window. */ const healthyAndFresh = (last: HealthCheckResult | null | undefined): boolean => last?.status === "healthy" && Date.now() - last.checkedAt < HEALTH_REVALIDATE_MS; -/** The revalidation query. ALWAYS defers to the server-enforced freshness - * window, only the width changes: long for a healthy verdict, short for - * anything else. Omitting it for non-healthy verdicts (as this once did) - * bypassed the gate for precisely the connections that were failing — the - * once-per-mount client guard is per MOUNT, not per verdict, so two surfaces - * rendering the same broken connection each sent their own probe, and each - * probe was another refused token request and another captured server error. - * Manual "Check now" is the only unconditional probe, and it passes `{}`. */ +/** The revalidation query: a healthy (but stale) verdict defers to the + * server-enforced window so N open tabs can't stampede the upstream; a + * missing or non-healthy verdict forces a fresh probe. + * + * A non-healthy verdict deliberately sends NO window. Suppressing its probe + * would suppress the only thing that can discover recovery: the verdict is + * persisted, so a gated request would answer "still expired" from the row + * the previous probe wrote, and the dot could not turn green until the window + * elapsed. Recovery visibility is the contract these surfaces are built on + * (see the health-checks-ui, graphql-introspection-health and + * mcp-oauth-reconnect-health scenarios), so the upstream cost of re-probing a + * broken connection is paid on purpose. What must NOT happen — one broken + * connection raising a server error on every probe — is fixed where it + * belongs, in the server folding a credential-resolution failure into a + * persisted verdict rather than into the failure channel. */ export const revalidateQuery = ( last: HealthCheckResult | null | undefined, -): { readonly ifStaleMs: number } => ({ - ifStaleMs: last?.status === "healthy" ? HEALTH_REVALIDATE_MS : HEALTH_REVALIDATE_UNHEALTHY_MS, -}); +): { 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` From 295359ff288ee082c346c4e000be2524534dc389 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:37:43 -0700 Subject: [PATCH 4/4] 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. --- .../connection-health-verdict.test.ts | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/e2e/scenarios/connection-health-verdict.test.ts b/e2e/scenarios/connection-health-verdict.test.ts index cd4c15e59..99d0013a6 100644 --- a/e2e/scenarios/connection-health-verdict.test.ts +++ b/e2e/scenarios/connection-health-verdict.test.ts @@ -488,20 +488,35 @@ scenario( "the later surfaces did ask about the connection's health — silence would mean a broken connection could never be seen to recover", ).toBeGreaterThan(0); - // THE production symptom, stated as the third party experiences it. - // Not "the later surfaces went silent" — they must not, or recovery - // could never show — but "each surface costs a fixed, bounded amount". - // Surface 3 is the SAME page as surface 1, so it must cost exactly what - // surface 1 cost; anything more is the per-connection guard failing and - // the effect re-probing in a loop. expect( afterList - baseline, "the list surface revalidated the broken connection", ).toBeGreaterThan(0); + + // THE production symptom, stated as the third party experiences it. + // Not "the later surfaces went silent" — they must not, or recovery + // could never show — but "the traffic is attributable and it stops". + // + // Attributable: a refresh grant only ever happens inside a health + // request, so grants can never outnumber the requests that caused + // them. More grants than requests is the server retrying in a loop. + // Counted this way the assertion cannot race a probe that is still in + // flight: the request is recorded when it is SENT, before its grant. + expect( + afterReturn, + "every refresh grant is attributable to a health request the client sent", + ).toBeLessThanOrEqual(healthRequests.length); + + // And it stops: once the page has settled, the per-connection guard + // means no further probes are issued. A quiet window that stays quiet + // is the storm's absence stated directly, rather than a probe count + // sampled at one arbitrary instant. + const settled = healthRequests.length; + await page.waitForTimeout(3_000); expect( - afterReturn - afterList, - "revisiting the same page costs the same one probe per connection it cost the first time, not a growing storm", - ).toBe(baseline); + healthRequests.length, + "a settled page stops probing; a still-climbing count is the guard failing and the effect re-probing in a loop", + ).toBe(settled); // And the shape that actually reached production: a refused refresh is // answered, not raised. Every one of these requests used to come back