diff --git a/.changeset/graphql-network-prose.md b/.changeset/graphql-network-prose.md new file mode 100644 index 000000000..d018b4676 --- /dev/null +++ b/.changeset/graphql-network-prose.md @@ -0,0 +1,5 @@ +--- +"@executor-js/plugin-graphql": patch +--- + +A GraphQL liveness probe no longer reads a transport failure's prose as a dead credential. Classification matched any upstream message containing "permission", which an operating-system refusal also carries — `connect EACCES: permission denied` on a socket reported the connection as `expired` and asked the user to re-enter a secret that was never the problem. Prose is now consulted only when the failure is not a transport failure; an HTTP 401 or 403 still classifies on its status. diff --git a/.changeset/mcp-liveness-pooled-probe.md b/.changeset/mcp-liveness-pooled-probe.md new file mode 100644 index 000000000..d2d612111 --- /dev/null +++ b/.changeset/mcp-liveness-pooled-probe.md @@ -0,0 +1,5 @@ +--- +"@executor-js/plugin-mcp": patch +--- + +The MCP liveness probe now takes the invocation pool's connection instead of dialling a second one. A probe of a local stdio server previously started a second child process, and the common local servers permit one instance only — Chrome DevTools MCP owns a browser and a debug port, Playwright MCP the same, `docker run -i` a container. The second child could not start, so the health check reported the connection broken while the server was up and serving tool calls. Because the UI re-probes every non-healthy verdict on every mount, each page load started one more child. A probe now reuses the pooled session or child, and an interrupted probe still tears down whatever it acquired. diff --git a/.changeset/oauth-refresh-evidence.md b/.changeset/oauth-refresh-evidence.md new file mode 100644 index 000000000..3d9741f83 --- /dev/null +++ b/.changeset/oauth-refresh-evidence.md @@ -0,0 +1,19 @@ +--- +"@executor-js/sdk": patch +--- + +Require evidence before a connection is marked permanently expired, and let the health probe refresh before it answers `expired`. + +A refresher whose grant is refused now reads the stored refresh token again. When a peer instance rotated that token while the request was in flight, the call adopts the access token the peer persisted and records no rejection. Previously the loser of a concurrent refresh wrote `oauthReauthRequiredAt` onto a connection that still held a valid rotated refresh token. Every surface then answered `expired` without probing, and no tool call could refresh it again: only a re-authorization recovered it. The record is also skipped when `expires_at` moved forward during the grant, which is the same peer success read from the row. + +`isPermanentTokenRejection` no longer reads 408, 425, or 429 as a definitive refusal. One rate-limited minute at a token endpoint therefore no longer ends a grant. Those statuses now behave like a 5xx response, and the next call retries. + +`connections.checkHealth` re-mints the token once and probes again before it answers `expired` for an OAuth connection. A revoked token, an idle timeout shorter than the advertised lifetime, or a null `expires_at` therefore no longer shows a working connection as dead. + +A connection whose integration declares no probe operation is now asked of the plugin first. A plugin that can answer without a spec — MCP lists its tools — gives a real verdict for its OAuth connections, which the credential-only branch never reached. Only when the plugin itself answers `unknown` does the credential-only verdict replace it, and that verdict is now computed from the values the probe already resolved, so nothing refreshes twice. A credential that resolves to nothing reads as `expired` there too, matching the plugins and heal-on-use. + +A refresh response that omits `expires_in` no longer erases `expires_at`. RFC 6749 makes the field optional, so an authorization server that advertised a lifetime on the code exchange and omitted it on refresh used to disable proactive refresh for the rest of the connection's life. The mint now records the advertised lifetime in `provider_state.oauthTokenLifetimeMs`, and a refresh without `expires_in` derives the expiry from it. + +A 403 scope shortfall on a probe now reads as `degraded` rather than `expired`: the credential authenticated, the grant is too narrow, and the remedy is a new consent rather than a reconnect. + +The test authorization server's MCP resource endpoint (`serveOAuthTestServer` at `/mcp`) now speaks the JSON-RPC protocol honestly: it answers the request's own id, answers `tools/list` with an empty catalog, and stays silent for notifications. The previous canned reply used a fixed id, so any client that completed the handshake waited forever for its `tools/list` response and every catalog sync or liveness probe against the endpoint timed out at the discovery deadline — a limitation invisible while OAuth health checks never dialled, and exposed once they do. diff --git a/.changeset/probe-scope-shortfall.md b/.changeset/probe-scope-shortfall.md new file mode 100644 index 000000000..dfe0dae7b --- /dev/null +++ b/.changeset/probe-scope-shortfall.md @@ -0,0 +1,5 @@ +--- +"@executor-js/plugin-openapi": patch +--- + +A health probe that meets a 403 scope shortfall now reads as `degraded` rather than `expired`. The credential authenticated; the grant is narrower than the probe operation needs, and the remedy is a new consent with wider scope — which the connection's missing-scope affordance already offers. The old verdict told the user the connection was dead and sent them through a reconnect that could not widen the grant. Classification passes the response headers as well as the body, so an RFC 6750 `WWW-Authenticate: Bearer error="insufficient_scope"` challenge is recognised too. diff --git a/packages/core/sdk/src/connections.test.ts b/packages/core/sdk/src/connections.test.ts index 51c7f8d11..92fe43586 100644 --- a/packages/core/sdk/src/connections.test.ts +++ b/packages/core/sdk/src/connections.test.ts @@ -2575,6 +2575,11 @@ const makeHealthHarness = (options?: { * `counters.probes`, so a Deferred-gated probe lets a test hold every * in-flight health check open and count how many actually started. */ readonly probe?: Effect.Effect; + /** Answer `unknown` when core passes no declared spec, the way the protocol + * plugins do: they have no operation to dial, so core falls back to the + * credential-only verdict. Without this the harness plugin answers every + * check, and the fallback is unreachable. */ + readonly declineWithoutSpec?: boolean; }) => { const counters = { probes: 0, resolves: 0 }; const hooks = { @@ -2660,9 +2665,16 @@ const makeHealthHarness = (options?: { ? ToolResult.fail({ code: "upstream_error", message: "boom" }) : { ran: toolRow.name, value: credential.value }, ), - checkHealth: () => + checkHealth: ({ spec }) => Effect.suspend(() => { counters.probes += 1; + if (options?.declineWithoutSpec === true && spec === undefined) { + return Effect.succeed({ + status: "unknown" as const, + checkedAt: Date.now(), + detail: "No health check configured.", + }); + } return ( options?.probe ?? Effect.succeed({ status: "healthy" as const, checkedAt: Date.now(), detail: "probe ok" }) @@ -3298,12 +3310,15 @@ describe("credential-only health path", () => { // parallel suite load the forked checks may not have finished their row // loads yet, and the counter reads 0. const entered = yield* Deferred.make(); - const { executor, counters, stamp, persisted, hooks } = yield* makeHealthHarness(); - // No declared probe spec + an OAuth client on the row routes checkHealth - // down the credential-only path: the verdict is "the credential - // resolved", produced without invoking the plugin probe. That path runs - // behind the same in-flight gate as probing, so concurrent checks must - // collapse to ONE resolution. + const { executor, counters, stamp, persisted, hooks } = yield* makeHealthHarness({ + declineWithoutSpec: true, + }); + // No declared probe spec + an OAuth client on the row: the plugin is + // asked first, declines for want of an operation to dial, and core falls + // back to the credential-only verdict — "the credential resolved", + // produced from the SAME resolution the plugin's probe used, so nothing + // refreshes twice. That path runs behind the same in-flight gate as + // probing, so concurrent checks must collapse to ONE resolution. yield* stamp({ oauth_client: "acme", expires_at: null }); hooks.onResolve = Deferred.succeed(entered, void 0).pipe( Effect.andThen(Deferred.await(gate)), @@ -3329,7 +3344,10 @@ describe("credential-only health path", () => { expect(first.status).toBe("healthy"); expect(second.status).toBe("healthy"); expect(counters.resolves).toBe(1); - expect(counters.probes).toBe(0); + // Both checks shared ONE gate entry, so the plugin was asked once and + // declined once; the verdict both callers received is the fallback. + expect(counters.probes).toBe(1); + expect(first.detail).toBe("Credential resolved (no probe configured)."); const row = yield* persisted(); expect(row?.lastHealth?.status).toBe("healthy"); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index cd89ab907..61934a0f5 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -1013,6 +1013,33 @@ const decodeOAuthReauthRequiredProviderState = Schema.decodeUnknownOption( const oauthReauthRequiredFromProviderState = (value: unknown) => Option.getOrNull(decodeOAuthReauthRequiredProviderState(decodeJsonColumn(value))); +/** `provider_state` as a merge base: the object it holds, or an empty one. Every + * writer merges rather than replaces, so a concurrent record survives. */ +const providerStateRecord = (value: unknown): Record => { + const decoded = decodeJsonColumn(value); + return decoded != null && typeof decoded === "object" && !Array.isArray(decoded) + ? (decoded as Record) + : {}; +}; + +const decodeTokenLifetimeState = Schema.decodeUnknownOption( + Schema.Struct({ oauthTokenLifetimeMs: Schema.Number }), +); + +/** The access-token lifetime this grant advertised, in ms, when it ever + * advertised one. RFC 6749 makes `expires_in` OPTIONAL: an authorization + * server that sends it on the code exchange may omit it on refresh, and + * writing a null `expires_at` from such a response erased the only input the + * proactive refresh has — permanently, for the rest of the connection's life. + * Remembering the lifetime lets the next refresh derive an expiry from the + * grant it already knows. */ +const rememberedTokenLifetimeMs = (value: unknown): number | null => { + const decoded = Option.getOrNull(decodeTokenLifetimeState(decodeJsonColumn(value))); + return decoded === null || !Number.isFinite(decoded.oauthTokenLifetimeMs) + ? null + : decoded.oauthTokenLifetimeMs; +}; + type OAuthReauthRequiredState = NonNullable< ReturnType >; @@ -2284,6 +2311,14 @@ export const createExecutor = => { - const existingState = decodeJsonColumn(row.provider_state); - const mergedState = - existingState != null && typeof existingState === "object" && !Array.isArray(existingState) - ? (existingState as Record) - : {}; - const health: HealthCheckResult = { - status: "expired", - checkedAt: Date.now(), - detail, - reason, + const ref: ConnectionRef = { + owner: row.owner as Owner, + integration: IntegrationSlug.make(row.integration), + name: ConnectionName.make(row.name), }; - return core - .updateMany("connection", { + const observedExpiry = row.expires_at == null ? null : Number(row.expires_at); + const record = (target: ConnectionRow): Effect.Effect => { + const mergedState = providerStateRecord(target.provider_state); + const health: HealthCheckResult = { + status: "expired", + checkedAt: Date.now(), + detail, + reason, + }; + return core.updateMany("connection", { where: (b: AnyCb) => b.and( - byOwner(row.owner as Owner)(b), - b("integration", "=", String(row.integration)), - b("name", "=", String(row.name)), + byOwner(target.owner as Owner)(b), + b("integration", "=", String(target.integration)), + b("name", "=", String(target.name)), ), set: { provider_state: { @@ -2333,8 +2370,36 @@ export const createExecutor = { + if (fresh === null) return record(row); + const freshExpiry = fresh.expires_at == null ? null : Number(fresh.expires_at); + const peerRefreshed = + freshExpiry !== null && (observedExpiry === null || freshExpiry > observedExpiry); + return peerRefreshed + ? Effect.annotateCurrentSpan({ + "executor.oauth.dead_grant.skipped_peer_refresh": true, + }) + : record(fresh); + }), + Effect.ignore, + ); }; /** Write a re-minted token back: a ROTATED refresh token into the refresh @@ -2383,13 +2448,37 @@ export const createExecutor = = { - expires_at: nextExpiresAt, + expires_at: lifetimeMs === null ? null : Date.now() + lifetimeMs, updated_at: new Date(), }; if (token.scope !== undefined) set.oauth_scope = token.scope; + if (advertisedLifetimeMs !== null && advertisedLifetimeMs !== rememberedLifetimeMs) { + // Merge into the CURRENT `provider_state`, read fresh: this write must + // not bury a concurrent one — a dead-grant record, a missing-scope + // set — under the copy this refresh started from. + const ref: ConnectionRef = { + owner: row.owner as Owner, + integration: IntegrationSlug.make(row.integration), + name: ConnectionName.make(row.name), + }; + const fresh = yield* findConnectionRow(ref); + set.provider_state = { + ...providerStateRecord((fresh ?? row).provider_state), + oauthTokenLifetimeMs: advertisedLifetimeMs, + }; + } yield* core.updateMany("connection", { where: (b: AnyCb) => b.and( @@ -2739,7 +2828,45 @@ export const createExecutor = => + Effect.gen(function* () { + if (error.reauthRequired !== true) return yield* Effect.fail(error); + const sent = storedRefreshToken; + if (sent === undefined || row.refresh_item_id == null) { + return yield* Effect.fail(error); + } + const current = yield* provider.get(ProviderItemId.make(row.refresh_item_id)); + if (current === null || current === sent) return yield* Effect.fail(error); + const tokenItemId = + connectionItemIds(row)[PRIMARY_INPUT_VARIABLE] ?? + `connection:${row.owner}:${row.integration}:${row.name}:${PRIMARY_INPUT_VARIABLE}`; + const access = yield* provider.get(ProviderItemId.make(tokenItemId)); + if (access === null) return yield* Effect.fail(error); + return access; + }); + const token: OAuth2TokenResponse | AdoptedAccessToken = clientRow.grant === "client_credentials" ? yield* exchangeClientCredentials({ tokenUrl, @@ -2874,6 +3001,13 @@ export const createExecutor = + adoptPeerRotatedToken(error), + ), // Persist the definitive verdict so the NEXT refresh skips // the doomed grant (see the known-dead gate above) and the // connection shows `expired` without waiting for a probe. @@ -2887,6 +3021,14 @@ export const createExecutor = => - foldCredentialResolutionIntoVerdict( - resolveConnectionValues(row).pipe( - Effect.as({ - status: "healthy" as const, + /** The verdict for an OAuth connection whose integration declares no probe + * operation and whose plugin cannot invent one: "the credential resolved + * (refreshing if due)" is the only signal this path can produce, and a + * refresh failure reaches the caller as a folded + * CredentialResolutionError instead of through here. A null value means the + * stored credential is GONE — the same case the plugins and heal-on-use + * refuse to call healthy, because rendering omits that placement and an + * upstream that answers unauthenticated would otherwise look alive. */ + const credentialOnlyVerdict = (values: Record): HealthCheckResult => + Object.values(values).some((value) => value == null) + ? { + status: "expired", + checkedAt: Date.now(), + detail: "Connection has no resolvable credential value.", + reason: "credential_missing", + } + : { + status: "healthy", 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: @@ -5234,64 +5391,102 @@ export const createExecutor = = {}; const freshVerdict: Effect.Effect = - spec === undefined && connectionRow.oauth_client != null - ? // No probe operation is declared, so "healthy" here means only - // "the credential resolved (refreshing if due)" — a refresh - // failure is the one real signal this path can produce, and it - // must not hide inside a green span. - oauthCredentialHealthWithoutProbe(connectionRow).pipe( - Effect.tap((result) => persistProbeHealthResult(ref, result)), - Effect.map((result) => ({ - source: "credential_only" as const, - result, - })), - ) - : foldCredentialResolutionIntoVerdict( - Effect.gen(function* () { - const values = yield* resolveConnectionValues(connectionRow); - const record = rowToIntegrationRecord( - integrationRow, - yield* describeAuthMethodsForRow(integrationRow), - ); - const grantedScopes = grantedScopesFromRow(connectionRow); - const credential: ToolInvocationCredential = { - owner: connectionRow.owner as Owner, - integration: ref.integration, - connection: ConnectionName.make(connectionRow.name), - template: AuthTemplateSlug.make(connectionRow.template), - value: values[PRIMARY_INPUT_VARIABLE] ?? null, - values, - config: record.config, - ...(grantedScopes ? { grantedScopes } : {}), - }; - // Core resolves the declared spec (its own column) and - // hands it to the plugin; plugins no longer read it out of - // their config. - return yield* foldPluginFailure( - check({ - ctx: runtime.ctx, - integration: record, - credential, - spec, - }), - `Health check for connection "${ref.name}" failed.`, - ); - }), - ).pipe( - // Persist the verdict on the connection row so the accounts - // list shows alive/expired at a glance, AND so the freshness - // gate above has something to serve. A probe that could not - // resolve its credential persists too: it is the connection - // most likely to be re-probed by every surface on every - // mount, so leaving it unwritten is what turns one broken - // connection into unbounded upstream and error traffic. - Effect.tap((result) => persistProbeHealthResult(ref, result)), - Effect.map((result) => ({ - source: "probe" as const, - result, - })), + foldCredentialResolutionIntoVerdict( + Effect.gen(function* () { + const record = rowToIntegrationRecord( + integrationRow, + yield* describeAuthMethodsForRow(integrationRow), + ); + const grantedScopes = grantedScopesFromRow(connectionRow); + const probe = ( + values: Record, + ): Effect.Effect => { + const credential: ToolInvocationCredential = { + owner: connectionRow.owner as Owner, + integration: ref.integration, + connection: ConnectionName.make(connectionRow.name), + template: AuthTemplateSlug.make(connectionRow.template), + value: values[PRIMARY_INPUT_VARIABLE] ?? null, + values, + config: record.config, + ...(grantedScopes ? { grantedScopes } : {}), + }; + // Core resolves the declared spec (its own column) and + // hands it to the plugin; plugins no longer read it out of + // their config. + return foldPluginFailure( + check({ + ctx: runtime.ctx, + integration: record, + credential, + spec, + }), + `Health check for connection "${ref.name}" failed.`, + ); + }; + const values = yield* resolveConnectionValues(connectionRow); + resolvedValues = values; + const first = yield* probe(values); + // A probe answers from the credential it was handed, so its + // `expired` is only as good as that credential. The invoke + // path knows this and re-mints once on a 401 + // (`forceRefreshConnectionValues`); the probe did not, which + // persisted `expired` for exactly the connections the + // reactive refresh exists for — a server-side revocation, an + // idle timeout shorter than the advertised lifetime, a null + // `expires_at` the proactive check can never fire on. The + // badge then said "reconnect" for a connection that worked + // on its next call, and only heal-on-use corrected it. + // + // One forced refresh and one re-probe, for an OAuth + // connection only. A refusal keeps the probe's own verdict: + // it is the more informative of the two, and the refresh + // path has already recorded a dead grant if there is one. + if (first.status !== "expired" || connectionRow.oauth_client == null) { + return first; + } + const refreshed = yield* forceRefreshConnectionValues(connectionRow).pipe( + Effect.catchTag("CredentialResolutionError", () => Effect.succeed(null)), ); + if (refreshed === null) return first; + yield* Effect.annotateCurrentSpan({ + "executor.health.refresh_retried": true, + }); + return yield* probe(refreshed); + }), + ).pipe( + // Ask the plugin FIRST, even with no declared spec: a plugin + // whose `checkHealth` ignores the spec (MCP lists tools) now + // gives a real verdict for its OAuth connections, which the + // old credential-only branch never reached. Only when the + // plugin itself answers `unknown` — it cannot invent a probe + // operation — does the credential-only verdict replace it, + // computed from the values the probe already resolved so + // nothing refreshes twice. + Effect.map((result) => + spec === undefined && + connectionRow.oauth_client != null && + result.status === "unknown" + ? { + source: "credential_only" as const, + result: credentialOnlyVerdict(resolvedValues), + } + : { source: "probe" as const, result }, + ), + // Persist the verdict on the connection row so the accounts + // list shows alive/expired at a glance, AND so the freshness + // gate above has something to serve. A probe that could not + // resolve its credential persists too: it is the connection + // most likely to be re-probed by every surface on every + // mount, so leaving it unwritten is what turns one broken + // connection into unbounded upstream and error traffic. + Effect.tap((outcome) => persistProbeHealthResult(ref, outcome.result)), + ); const run = freshVerdict.pipe( Effect.exit, Effect.flatMap((exit) => Deferred.done(deferred, exit)), diff --git a/packages/core/sdk/src/health-check.test.ts b/packages/core/sdk/src/health-check.test.ts index 61b652267..c3f650912 100644 --- a/packages/core/sdk/src/health-check.test.ts +++ b/packages/core/sdk/src/health-check.test.ts @@ -137,4 +137,36 @@ describe("classifyProbeResponse", () => { ); } }); + + // A scope shortfall authenticated: the credential works, the grant is + // narrower than this operation needs, and the remedy is a NEW CONSENT — which + // the connection's `missingOAuthScopes` already offers. Reporting it as + // `expired` told the user the connection was dead and sent them through a + // reconnect that could not widen the grant. + it("classifies an RFC 6750 insufficient_scope challenge as degraded", () => { + expect( + classifyProbeResponse(403, undefined, { + "www-authenticate": 'Bearer error="insufficient_scope", scope="read write"', + }), + ).toBe("degraded"); + }); + + it("classifies a body-named insufficient_scope as degraded", () => { + expect(classifyProbeResponse(403, { error: "insufficient_scope" })).toBe("degraded"); + expect( + classifyProbeResponse(403, { + error: { code: 403, details: [{ reason: "ACCESS_TOKEN_SCOPE_INSUFFICIENT" }] }, + }), + ).toBe("degraded"); + }); + + it("keeps the configuration carve-out ahead of the scope one", () => { + expect( + classifyProbeResponse( + 403, + { error: { errors: [{ reason: "accessNotConfigured" }], code: 403 } }, + { "www-authenticate": 'Bearer error="insufficient_scope"' }, + ), + ).toBe("misconfigured"); + }); }); diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index a4d6f1cb2..892c5408d 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -17,6 +17,8 @@ import { Schema } from "effect"; +import { detectInsufficientScope } from "./insufficient-scope"; + // --------------------------------------------------------------------------- // Status: the five states a connection can be in. `expired` is the one this // whole feature exists for (Google's 7-day dev-token revocation): the credential @@ -242,16 +244,32 @@ const errorReasonMarkers = (body: unknown): string[] => { }; /** Classify a probe response from its status AND body. Everything is - * `classifyHttpStatus` except one carve-out: a 403 whose error body carries a - * known configuration reason (Google `accessNotConfigured` / - * `SERVICE_DISABLED`) is `misconfigured`, not `expired`: the credential - * authenticated; the upstream API is disabled in the OAuth client's project, - * and only enabling it there (not reconnecting) fixes it. */ -export const classifyProbeResponse = (status: number, body: unknown): HealthStatus => { + * `classifyHttpStatus` except two carve-outs on a 403: + * + * - A known configuration reason (Google `accessNotConfigured` / + * `SERVICE_DISABLED`) is `misconfigured`: the credential authenticated, the + * upstream API is disabled in the OAuth client's project, and only enabling + * it there (not reconnecting) fixes it. + * - A scope shortfall (RFC 6750 `insufficient_scope` in `WWW-Authenticate`, + * `error: insufficient_scope` in the body, Google's + * `ACCESS_TOKEN_SCOPE_INSUFFICIENT`) is `degraded`: the credential + * authenticated too, and the remedy is a NEW CONSENT with wider scope — + * which the connection's `missingOAuthScopes` already offers — not a + * reconnect. Reporting it as `expired` told the user the connection was dead + * and sent them through a flow that could not fix it. `headers` is optional + * so a caller that only kept the body still gets the body-based detection. */ +export const classifyProbeResponse = ( + status: number, + body: unknown, + headers?: Record, +): HealthStatus => { const byStatus = classifyHttpStatus(status); if (status !== 403 || byStatus !== "expired") return byStatus; - return errorReasonMarkers(body).some((reason) => CONFIGURATION_403_REASONS.has(reason)) - ? "misconfigured" + if (errorReasonMarkers(body).some((reason) => CONFIGURATION_403_REASONS.has(reason))) { + return "misconfigured"; + } + return detectInsufficientScope({ body, ...(headers === undefined ? {} : { headers }) }) !== null + ? "degraded" : "expired"; }; diff --git a/packages/core/sdk/src/oauth-expired-status-repro.test.ts b/packages/core/sdk/src/oauth-expired-status-repro.test.ts index 6e44ce3e6..76846f456 100644 --- a/packages/core/sdk/src/oauth-expired-status-repro.test.ts +++ b/packages/core/sdk/src/oauth-expired-status-repro.test.ts @@ -1,22 +1,25 @@ -// Reproduction harness for the "Expired" status + refresh defects analysed in -// plans/oauth-refresh-and-expired-status.md. +// Regression coverage for the three causes of a wrong **Expired** status that +// plans/oauth-refresh-and-expired-status.md ranks R1, R2, and R3. This file +// started as the reproduction harness for that analysis: each cause had a test +// that pinned the behavior on `main` and a skipped test that gave the required +// behavior. The fix landed, so each cause now has one test, and it asserts the +// required behavior. // -// Each root cause gets TWO tests, with no branching inside either: -// -// "documents current behavior" — passes on main today. This is the -// replication: it pins what a user actually sees, so the defect is not a -// matter of interpretation. -// "REPRO" — asserts the behavior we want. It FAILS on main today, so it is -// checked in skipped; it is the acceptance anchor for the fix phase named -// in its title, and that PR un-skips it green without editing it. +// R1: a refresher that loses a rotation race adopts the peer's token. It does +// not write the permanent rejection record. +// R2: one temporary 4xx response (a 429) does not end the grant. +// R3: the health probe refreshes before it answers `expired`. // // Deployment shape under test: ONE database, ONE credential store, TWO executor -// instances each holding its OWN root db handle. That is cloud (per-request -// `DbService` rebuild + per-session Durable Objects) and any multi-process -// self-host. It is the shape `refreshGateFor`'s own doc block declares out of -// scope, and the shape `oauth-flow.test.ts`'s two-instance test already builds -// — that test asserts the spent token is not written back, but never looks at -// what the loser's `invalid_grant` does to the connection ROW. These do. +// instances, and one root database handle for each instance. That is the cloud +// app (a per-request `DbService` rebuild plus per-session Durable Objects) and +// any multi-process self-hosting. It is the shape that the `refreshGateFor` +// documentation declares out of scope for the in-process gate. +// +// `oauth-flow.test.ts` already builds this shape in "a refresher paused after +// reading the stored token never writes it back over a peer's rotated one". +// That test examines the credential store. These tests examine the connection +// row, which is where the wrong status was written. import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; @@ -288,12 +291,13 @@ const deadGrantStamp = (row: unknown): number | undefined => { }; // --------------------------------------------------------------------------- -// R1 — the loser of a rotation race permanently bricks a healthy connection. +// R1 — a refresher that loses the rotation race adopts the peer's token. // --------------------------------------------------------------------------- /** Run the race: A reads the stored refresh token and stalls, B wins and - * rotates it, A resumes and redeems the consumed token. Shared by both R1 - * tests so they differ only in what they assert about the aftermath. */ + * rotates it, A resumes and redeems the consumed token. Returns the rotated + * token and A's own outcome, so a test can assert what the loser did with the + * refusal. */ const runRotationRace = (race: Race) => Effect.gen(function* () { const refreshItemId = race.refreshItemId(); @@ -314,69 +318,43 @@ const runRotationRace = (race: Race) => // A resumes and redeems a token the authorization server already consumed. yield* Deferred.succeed(race.resumeFromRead, undefined); - yield* Fiber.join(loser); + const loserExit = yield* Fiber.join(loser); // The store still holds B's valid rotated token: this connection is not out // of credentials, it lost a race. expect(race.store.values.get(refreshItemId!)).toBe(rotatedRefreshToken); - return { refreshItemId: refreshItemId!, rotatedRefreshToken: rotatedRefreshToken! }; + return { + refreshItemId: refreshItemId!, + rotatedRefreshToken: rotatedRefreshToken!, + loserExit, + }; }); -describe("R1 — refresh race across two instances", () => { - it.effect("documents current behavior: the loser bricks a connection holding a valid token", () => +describe("R1 — a lost rotation race is not a dead grant", () => { + it.effect("the loser adopts the peer's token and the connection keeps refreshing", () => withRace({}, (race) => Effect.gen(function* () { - yield* runRotationRace(race); - - // A's `invalid_grant` recorded a dead grant on a connection whose - // stored refresh token is valid. - expect( - deadGrantStamp(yield* race.rawRow()), - "the loser marked the grant permanently dead", - ).toBeTypeOf("number"); - const health = yield* race.b.connections.checkHealth(REF); - expect(health.status, "every surface now answers expired without probing").toBe("expired"); - - // The rotated token is still perfectly good — nobody is allowed to use - // it again. This is the permanent part. - yield* race.expire(); - yield* race.server.clearRequests; - const next = yield* Effect.exit(race.b.execute(ADDRESS, {})); - expect(Exit.isSuccess(next), "the winner can no longer refresh either").toBe(false); - expect( - refreshGrants(yield* race.server.requests), - "the known-dead gate never sends another grant", - ).toHaveLength(0); - }), - ), - ); + const { loserExit } = yield* runRotationRace(race); - // Skipped, not deleted: this is the acceptance anchor for Phase 1 of - // plans/oauth-refresh-and-expired-status.md. The PR that lands the fix - // un-skips it and it must go green unchanged. - it.effect.skip("REPRO: a lost rotation race must not record a dead grant (Phase 1)", () => - withRace({}, (race) => - Effect.gen(function* () { - yield* runRotationRace(race); + // The loser's own call recovered: it read the refresh item back, saw + // that a peer had replaced the value it sent, and used the access token + // that peer persisted. + expect(Exit.isSuccess(loserExit), "the losing refresher still served its call").toBe(true); - // Phase 1 target: the loser notices the rotation and adopts it, so no - // dead grant is ever recorded. - expect( - deadGrantStamp(yield* race.rawRow()), - "a lost race must not record a dead grant", - ).toBeUndefined(); + // No permanent rejection record, because the grant is alive. + expect(deadGrantStamp(yield* race.rawRow()), "no dead grant is recorded").toBeUndefined(); const health = yield* race.b.connections.checkHealth(REF); expect(health.status, "and no surface answers expired").not.toBe("expired"); + // The winner still refreshes with its own rotated token when the access + // token next expires. yield* race.expire(); yield* race.server.clearRequests; const next = yield* Effect.exit(race.b.execute(ADDRESS, {})); - expect(Exit.isSuccess(next), "the winner can still refresh with its own valid token").toBe( - true, - ); + expect(Exit.isSuccess(next), "the winner refreshes again on the next expiry").toBe(true); expect( refreshGrants(yield* race.server.requests).length, - "executor asked the authorization server again", + "the authorization server received that grant", ).toBeGreaterThan(0); }), ), @@ -384,7 +362,7 @@ describe("R1 — refresh race across two instances", () => { }); // --------------------------------------------------------------------------- -// R2 — one transient 4xx (a 429) permanently kills the grant. +// R2 — one temporary 4xx does not end a grant. // --------------------------------------------------------------------------- interface FlakyEndpoint { @@ -457,7 +435,7 @@ const serveFlakyTokenEndpoint = (upstream: string) => ); /** Connect, point the backing app at a token endpoint that rate-limits once, - * and take that first (failing) refresh. Shared by both R2 tests. */ + * and take that first failing refresh. */ const withRateLimitedRefresh = ( use: (input: { readonly race: Race; @@ -483,44 +461,94 @@ const withRateLimitedRefresh = ( }), ); -describe("R2 — transient 4xx classification", () => { - it.effect("documents current behavior: one 429 permanently disables a working grant", () => +describe("R2 — a rate-limited refresh stays retryable", () => { + it.effect("a 429 from the token endpoint does not end the grant", () => withRateLimitedRefresh(({ race, flaky }) => Effect.gen(function* () { - const health = yield* race.a.connections.checkHealth(REF); - expect(health.status, "one 429 rendered the connection permanently expired").toBe( - "expired", - ); - - // The endpoint is healthy from here on — every later grant would be - // forwarded to the real authorization server and succeed. Executor - // never sends one. - const attemptsBefore = flaky.attempts(); + // The endpoint forwards every grant after the first to the real + // authorization server, so it is healthy from here on. The next call + // must reach it. const second = yield* Effect.exit(race.a.execute(ADDRESS, {})); - expect(Exit.isSuccess(second), "and it never asks the healthy endpoint again").toBe(false); - expect(flaky.attempts(), "no further grant was attempted").toBe(attemptsBefore); - }), - ), - ); + expect(Exit.isSuccess(second), "the retry refreshed and the call succeeded").toBe(true); + expect(flaky.attempts(), "executor asked the token endpoint again").toBeGreaterThan(1); - // Skipped, not deleted: Phase 1 acceptance anchor (see the note above). - it.effect.skip("REPRO: a 429 must stay retryable (Phase 1)", () => - withRateLimitedRefresh(({ race, flaky }) => - Effect.gen(function* () { - // Phase 1 target: a 429 is retryable, so the next attempt reaches the - // (now healthy) endpoint and the connection keeps working. - const second = yield* Effect.exit(race.a.execute(ADDRESS, {})); - expect(Exit.isSuccess(second), "a 429 does not end the grant").toBe(true); - expect(flaky.attempts(), "executor retried the refresh").toBeGreaterThan(1); + expect( + deadGrantStamp(yield* race.rawRow()), + "a rate limit is not a permanent rejection", + ).toBeUndefined(); + const health = yield* race.a.connections.checkHealth(REF); + expect(health.status, "and the connection does not read as expired").not.toBe("expired"); }), ), ); }); // --------------------------------------------------------------------------- -// R3 — the health probe never refreshes reactively, so it writes `expired` for -// a credential the tool path would have refreshed, then flips to healthy on the -// next tool call. That flip is the "disconnected, then connected" symptom. +// R5 — a refresh response without `expires_in` keeps the advertised lifetime. +// --------------------------------------------------------------------------- + +interface StrippingEndpoint { + readonly url: string; + readonly attempts: () => number; + readonly close: () => void; +} + +/** A token endpoint that answers a refresh grant with a valid token response + * that OMITS `expires_in`, which RFC 6749 permits, and rotates the refresh + * token like the real one does. It stands in for an authorization server that + * advertised a lifetime on the code exchange and then stopped repeating it. */ +const serveExpiresInStrippingEndpoint = () => + Effect.acquireRelease( + Effect.callback((resume) => { + let attempts = 0; + const server: Server = createServer((req, res) => { + const chunks: Buffer[] = []; + req.on("data", (chunk: Buffer) => chunks.push(chunk)); + req.on("end", () => { + const body = Buffer.concat(chunks).toString("utf8"); + if (!body.includes("grant_type=refresh_token")) { + res.writeHead(400, { "content-type": "text/plain" }); + res.end("this fixture answers refresh grants only"); + return; + } + attempts += 1; + res.writeHead(200, { "content-type": "application/json", "cache-control": "no-store" }); + res.end( + `{"access_token":"at_stripped_${attempts}","refresh_token":"rt_stripped_${attempts}","token_type":"Bearer"}`, + ); + void req; + }); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address !== null ? address.port : 0; + resume( + Effect.succeed({ + url: `http://127.0.0.1:${port}/token`, + attempts: () => attempts, + close: () => server.close(), + }), + ); + }); + }), + (handle) => Effect.sync(() => handle.close()), + ); + +/** `connection.expires_at` off the raw row, which adapters return as a number or + * a string. */ +const RowExpiry = Schema.Struct({ expires_at: Schema.optional(Schema.Unknown) }); +const decodeRowExpiry = Schema.decodeUnknownOption(RowExpiry); +const rowExpiresAt = (row: unknown): number | null => { + const value = Option.getOrUndefined(decodeRowExpiry(row))?.expires_at; + // A bigint column: adapters hand back a number, a string, or a BigInt. + if (typeof value === "number") return value; + if (typeof value === "bigint") return Number(value); + if (typeof value === "string") return Number(value); + return null; +}; + +// --------------------------------------------------------------------------- +// R3 — the probe refreshes before it answers expired. // --------------------------------------------------------------------------- /** Connect with a declared health check, then revoke the live access token @@ -534,51 +562,87 @@ const withRevokedToken = (use: (race: Race) => Effect.Effect) => }), ); -describe("R3 — probe verdict vs reactive refresh", () => { - it.effect("documents current behavior: probe says expired, the next tool call says healthy", () => +describe("R3 — the probe refreshes before it answers expired", () => { + it.effect("a revoked token that the refresh can replace probes healthy", () => withRevokedToken((race) => Effect.gen(function* () { - // The probe persists `expired` without ever trying the refresh token - // that would have fixed it … const verdict = yield* race.a.connections.checkHealth(REF); - expect(verdict.status).toBe("expired"); + expect(verdict.status, "the probe re-minted instead of reporting expired").toBe("healthy"); expect( - refreshGrants(yield* race.server.requests), - "the probe sent no refresh grant", - ).toHaveLength(0); + refreshGrants(yield* race.server.requests).length, + "the probe sent a refresh grant", + ).toBeGreaterThan(0); + + // The persisted verdict agrees, so every surface reads healthy without + // waiting for a tool call to heal it. const persisted = yield* race.a.connections.get(REF); - expect( - persisted?.lastHealth?.status, - "and the verdict is persisted for every surface to read", - ).toBe("expired"); - - // … then the very next tool call refreshes reactively, succeeds, and - // heals the row. Same connection, seconds apart, no user action: - // "disconnected" then "connected". - yield* race.a.execute(ADDRESS, {}); - expect( - race.state.calls.length, - "the tool call retried with a re-minted token", - ).toBeGreaterThan(1); - const healed = yield* race.a.connections.get(REF); - expect(healed?.lastHealth?.status, "heal-on-use flipped the badge back").toBe("healthy"); + expect(persisted?.lastHealth?.status, "the healthy verdict is persisted").toBe("healthy"); }), ), ); - // Skipped, not deleted: Phase 3 acceptance anchor (see the note above). - it.effect.skip("REPRO: the probe must refresh before concluding expired (Phase 3)", () => - withRevokedToken((race) => + it.effect("a refused refresh still answers expired from the probe", () => + withRace({ healthCheck: true }, (race) => Effect.gen(function* () { - // Phase 3 target: the probe refreshes once before concluding expired. + // No revocation and no expiry: the probe answers from the credential it + // resolved. A refusal is what the persisted-expired contract in + // `connection-health-verdict.test.ts` covers; this asserts the retry + // did not turn the probe into a second grant on a healthy connection. + yield* race.server.clearRequests; const verdict = yield* race.a.connections.checkHealth(REF); - expect(verdict.status, "a refreshable revocation is not an expired connection").toBe( - "healthy", + expect(verdict.status).toBe("healthy"); + expect( + refreshGrants(yield* race.server.requests), + "a healthy probe sends no refresh grant", + ).toHaveLength(0); + }), + ), + ); +}); + +// --------------------------------------------------------------------------- +// R5 — a refresh response without `expires_in` must not erase the expiry. +// --------------------------------------------------------------------------- + +describe("R5 — a refresh response that omits expires_in", () => { + it.effect("keeps the advertised lifetime, so proactive refresh survives", () => + Effect.scoped( + Effect.gen(function* () { + const race = yield* makeRace({}); + const mintedExpiry = rowExpiresAt(yield* race.rawRow()); + expect(mintedExpiry, "the mint recorded the advertised expiry").not.toBeNull(); + expect( + (mintedExpiry ?? 0) - Date.now(), + "and the test authorization server advertised an hour", + ).toBeGreaterThan(30 * 60_000); + + const stripping = yield* serveExpiresInStrippingEndpoint(); + yield* Effect.promise(() => + race.config.db.updateMany("oauth_client", { + where: (builder) => builder("slug", "=", String(CLIENT)), + set: { token_url: stripping.url }, + }), ); + yield* race.expire(); + + const first = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(first), "the refresh succeeded").toBe(true); + expect(stripping.attempts(), "and it went to the endpoint that omits expires_in").toBe(1); + + // This wrote null before the fix, which disabled the proactive check + // for the rest of the connection's life and left every later call to + // the reactive 401 path. + const refreshedExpiry = rowExpiresAt(yield* race.rawRow()); + expect(refreshedExpiry, "the expiry survived a response without expires_in").not.toBeNull(); expect( - refreshGrants(yield* race.server.requests).length, - "the probe re-minted the token", - ).toBeGreaterThan(0); + (refreshedExpiry ?? 0) - Date.now(), + "and it carries the lifetime the grant advertised", + ).toBeGreaterThan(30 * 60_000); + + // The proactive path still works, so the next call needs no grant. + const second = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(second), "the next call used the stored token").toBe(true); + expect(stripping.attempts(), "and sent no second grant").toBe(1); }), ), ); diff --git a/packages/core/sdk/src/oauth-flow.test.ts b/packages/core/sdk/src/oauth-flow.test.ts index 7496ce1ef..a3f82ae5e 100644 --- a/packages/core/sdk/src/oauth-flow.test.ts +++ b/packages/core/sdk/src/oauth-flow.test.ts @@ -54,11 +54,17 @@ const oauthPlugin = definePlugin(() => ({ }, // Echo the resolved credential value (the OAuth access token) back out. invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), - checkHealth: ({ credential }) => - Effect.succeed({ - status: credential.value === null ? "expired" : "healthy", - checkedAt: Date.now(), - }), + // Mirrors the protocol plugins: with no declared probe operation there is + // nothing to dial, so the plugin answers `unknown` and core falls back to the + // credential-only verdict. A plugin that CAN answer without a spec (MCP lists + // tools) is asked first and gives a real verdict. + checkHealth: ({ credential, spec }) => + spec === undefined + ? Effect.succeed({ status: "unknown" as const, checkedAt: Date.now() }) + : Effect.succeed({ + status: credential.value === null ? ("expired" as const) : ("healthy" as const), + checkedAt: Date.now(), + }), extension: (ctx) => ({ seed: (scopes: readonly string[] = []) => ctx.core.integrations.register({ @@ -2085,7 +2091,7 @@ describe("oauth token refresh in resolveConnectionValue", () => { where: (b) => b("name", "=", "main"), }), ); - expect(row?.provider_state).toEqual({ missingOAuthScopes: ["write"] }); + expect(row?.provider_state).toMatchObject({ missingOAuthScopes: ["write"] }); const listed = yield* executor.connections.list({ integration: INTEG }); expect(listed[0]?.missingOAuthScopes).toEqual(["write"]); }), @@ -2136,7 +2142,12 @@ describe("oauth token refresh in resolveConnectionValue", () => { where: (b) => b("name", "=", "main"), }), ); - expect(row?.provider_state).toBeNull(); + // No missing-scope record. `provider_state` itself is not null: the + // mint records the advertised token lifetime there so a later refresh + // whose response omits `expires_in` can still derive an expiry. + expect( + (row?.provider_state as { missingOAuthScopes?: unknown } | null)?.missingOAuthScopes, + ).toBeUndefined(); }), ), ); diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index d94e8c34a..1ecfb6e7b 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -1781,6 +1781,48 @@ describe("refreshAccessToken", () => { expect(isPermanentTokenRejection(error)).toBe(false); }), ); + + // A 4xx that describes THIS MINUTE rather than this grant must stay + // retryable: the identical request can succeed moments later. 429 is the + // authorization server asking us to come back, 408 is its own request + // timeout, 425 is a transport-level replay refusal. Reading any of them as + // definitive ended connections permanently on one rate-limited minute — and + // rate limiting is likelier the more refreshers race for one grant. + for (const status of [408, 425, 429] as const) { + it.effect(`keeps a ${status} response transient`, () => + withTokenEndpoint( + () => HttpServerResponse.text("slow down", { status }), + ({ tokenUrl }) => + Effect.gen(function* () { + const error = yield* Effect.flip( + refreshAccessToken({ tokenUrl, clientId: "cid", refreshToken: "old" }), + ); + expect(error.status).toBe(status); + expect(error.error).toBeUndefined(); + expect(isPermanentTokenRejection(error)).toBe(false); + }), + ), + ); + } + + // The definitive 4xx stay definitive: §5.2 mandates 400 for a grant the AS + // will not honour, and a token endpoint answering 404 does not start + // existing on the next attempt. + for (const status of [400, 404] as const) { + it.effect(`keeps a text/plain ${status} response definitive`, () => + withTokenEndpoint( + () => HttpServerResponse.text("your session has expired", { status }), + ({ tokenUrl }) => + Effect.gen(function* () { + const error = yield* Effect.flip( + refreshAccessToken({ tokenUrl, clientId: "cid", refreshToken: "old" }), + ); + expect(error.status).toBe(status); + expect(isPermanentTokenRejection(error)).toBe(true); + }), + ), + ); + } }); describe("shouldRefreshToken", () => { diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index c6e3209fe..a5c151708 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -73,22 +73,34 @@ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ export const isUnusableSuccessTokenResponse = (error: OAuth2Error): boolean => error.status !== undefined && error.status < 300; +/** 4xx statuses that describe THIS MINUTE rather than this grant. A 429 is the + * authorization server asking us to come back, a 408 is its own request + * timeout, and a 425 is a transport-level replay refusal — none of them is a + * verdict on the refresh token, and re-sending the identical grant later can + * succeed. Treating them as definitive ended connections permanently on one + * rate-limited minute, which is likelier the more refreshers race. */ +const TRANSIENT_4XX_STATUSES: ReadonlySet = new Set([408, 425, 429]); + /** * Did the token endpoint answer in a way that re-sending the identical grant * cannot change? * - * Yes for a 4xx — §5.2 mandates 400 for a grant the authorization server will - * not honour, 401/403 are refusals, and a token endpoint answering 404 does not - * start existing on the next attempt — and yes for a 2xx that carried no usable - * token, because the server called it a success and still issued nothing. + * Yes for a 4xx that is not one of {@link TRANSIENT_4XX_STATUSES} — §5.2 + * mandates 400 for a grant the authorization server will not honour, 401/403 + * are refusals, and a token endpoint answering 404 does not start existing on + * the next attempt — and yes for a 2xx that carried no usable token, because + * the server called it a success and still issued nothing. * - * No for a 5xx (the AS is having a bad minute) and no when there is no response - * at all (transport). Those are exactly the failures a later attempt survives, - * so they must stay retryable. + * No for a 5xx (the AS is having a bad minute), no for a rate-limited or + * timed-out 4xx, and no when there is no response at all (transport). Those are + * exactly the failures a later attempt survives, so they must stay retryable. */ export const isPermanentTokenRejection = (error: OAuth2Error): boolean => isUnusableSuccessTokenResponse(error) || - (error.status !== undefined && error.status >= 400 && error.status < 500); + (error.status !== undefined && + error.status >= 400 && + error.status < 500 && + !TRANSIENT_4XX_STATUSES.has(error.status)); // --------------------------------------------------------------------------- // Token response shape (RFC 6749 §5.1) diff --git a/packages/core/sdk/src/testing/oauth-test-server.ts b/packages/core/sdk/src/testing/oauth-test-server.ts index 0380e389f..bd68d826c 100644 --- a/packages/core/sdk/src/testing/oauth-test-server.ts +++ b/packages/core/sdk/src/testing/oauth-test-server.ts @@ -1076,10 +1076,45 @@ export const serveOAuthTestServer = ( }, ); } + // A minimal but honest MCP resource server. The old canned reply used + // a FIXED json-rpc id and answered notifications too, so a client + // that completed the handshake waited forever for its `tools/list` + // response: every catalog sync and every liveness probe against this + // endpoint timed out at the discovery deadline. Answer the request's + // OWN id, answer `tools/list` with an empty catalog, and stay silent + // for notifications, which is the protocol. + const decodeMcpFrame = Schema.decodeUnknownOption(Schema.fromJsonString(Schema.Unknown)); + const frame = Option.getOrUndefined(decodeMcpFrame(body)); + const frameIsRecord = frame !== null && typeof frame === "object"; + const frameRecord = frameIsRecord ? (frame as Record) : {}; + const frameMethod = + typeof frameRecord["method"] === "string" ? frameRecord["method"] : ""; + const hasId = "id" in frameRecord && frameRecord["id"] !== undefined; + if (!hasId) { + // Accepted with no body: a notification has no reply. + return HttpServerResponse.empty({ status: 202 }); + } + const params = + frameRecord["params"] !== null && typeof frameRecord["params"] === "object" + ? (frameRecord["params"] as Record) + : {}; + const reply = + frameMethod === "tools/list" + ? { tools: [] } + : frameMethod === "initialize" + ? { + protocolVersion: + typeof params["protocolVersion"] === "string" + ? params["protocolVersion"] + : "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "oauth-test-server", version: "0.0.0" }, + } + : {}; return jsonResponse(200, { jsonrpc: "2.0", - id: 1, - result: { protocolVersion: "2025-06-18", capabilities: {} }, + id: frameRecord["id"], + result: reply, }); } diff --git a/packages/plugins/graphql/src/sdk/health-classification.test.ts b/packages/plugins/graphql/src/sdk/health-classification.test.ts new file mode 100644 index 000000000..eb1d0b548 --- /dev/null +++ b/packages/plugins/graphql/src/sdk/health-classification.test.ts @@ -0,0 +1,48 @@ +// The prose a transport failure carries is the operating system's or the HTTP +// client's, not the upstream's verdict on a credential. Reading it as one sent +// users to re-enter a secret that was never the problem: `EACCES: permission +// denied` on a socket matches any pattern looking for the word "permission". + +import { describe, expect, it } from "@effect/vitest"; + +import { GraphqlIntrospectionError } from "./errors"; +import { healthFromIntrospectionError } from "./plugin"; + +const classify = (input: { + readonly reason?: "network" | "graphql-errors" | "invalid-json"; + readonly status?: number; + readonly upstreamMessage?: string; +}) => + healthFromIntrospectionError( + new GraphqlIntrospectionError({ + message: "introspection failed", + ...(input.reason === undefined ? {} : { reason: input.reason }), + ...(input.status === undefined ? {} : { status: input.status }), + ...(input.upstreamMessage === undefined ? {} : { upstreamMessage: input.upstreamMessage }), + }), + Date.now(), + ); + +describe("GraphQL liveness classification", () => { + it("does not read a transport failure's prose as a dead credential", () => { + const verdict = classify({ + reason: "network", + upstreamMessage: "connect EACCES: permission denied /run/graphql.sock", + }); + expect(verdict.status).not.toBe("expired"); + }); + + it("still reads an authentication failure the upstream named as expired", () => { + const verdict = classify({ + reason: "graphql-errors", + upstreamMessage: "Authentication required: invalid token", + }); + expect(verdict.status).toBe("expired"); + }); + + it("classifies an HTTP 401 on its status whatever else is said", () => { + const verdict = classify({ status: 401, upstreamMessage: "nope" }); + expect(verdict.status).toBe("expired"); + expect(verdict.httpStatus).toBe(401); + }); +}); diff --git a/packages/plugins/graphql/src/sdk/plugin.ts b/packages/plugins/graphql/src/sdk/plugin.ts index 176e6d772..dfe3f77f4 100644 --- a/packages/plugins/graphql/src/sdk/plugin.ts +++ b/packages/plugins/graphql/src/sdk/plugin.ts @@ -114,6 +114,14 @@ const appendUpstreamMessage = (detail: string, message?: string): string => ? `${detail} Upstream said: ${truncateHealthDetail(message)}` : detail; +/** Whether an upstream's own prose names an authentication failure. + * + * Text matching, so it is deliberately the WEAKER signal: it is consulted only + * when no HTTP status classified the failure, and never for a transport + * failure. An OS-level refusal carries the word "permission" too — `EACCES: + * permission denied` on a socket or a binary — and reading that as a dead + * credential sent the user to re-enter a secret that was never the problem. + * The caller excludes `reason: "network"` for exactly that case. */ const isAuthMessage = (message: string | undefined): boolean => message !== undefined && /authoriz|authenticat|forbidden|permission|credential|api.?key|access denied|access token|invalid token|token expired|logged in|sign in/i.test( @@ -141,7 +149,14 @@ const missingCredentialVariables = ( }); }; -const healthFromIntrospectionError = ( +/** Classify one introspection failure as a health verdict. + * + * Exported for tests (not re-exported from `sdk/index.ts`, so this widens no + * public API): the classification rules — which prose counts as an + * authentication failure, and which reason may never be read from prose — are + * the whole behavior under test, and reaching them through a live introspection + * would test the transport instead. */ +export const healthFromIntrospectionError = ( error: GraphqlIntrospectionError, checkedAt: number, ): HealthCheckResult => { @@ -160,7 +175,12 @@ const healthFromIntrospectionError = ( }; } - if (httpStatus === 401 || httpStatus === 403 || isAuthMessage(upstream)) { + // A transport failure never classifies from prose: its message is the OS's or + // the HTTP client's, and "permission denied" there names a socket, not a + // credential (see `isAuthMessage`). An HTTP 401/403 still classifies on its + // own status whatever the reason. + const proseSaysAuth = error.reason !== "network" && isAuthMessage(upstream); + if (httpStatus === 401 || httpStatus === 403 || proseSaysAuth) { const statusDetail = httpStatus === 401 || httpStatus === 403 ? `The endpoint rejected the credential with HTTP ${httpStatus}.` diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index d3c672361..900a5a5f0 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -5,7 +5,11 @@ import { Duration, Effect, Option, Predicate, Schema } from "effect"; import { hasNestedOAuthReauthorization, type McpConnection, type McpConnector } from "./connection"; -import { McpToolDiscoveryError } from "./errors"; +import { + type McpConnectionError, + type McpOAuthReauthorizationRequired, + McpToolDiscoveryError, +} from "./errors"; import { createMcpConnector, type ConnectorInput } from "./connection"; import { httpStatusFromCause } from "./http-status"; import { @@ -29,7 +33,8 @@ const MAX_LIST_TOOLS_PAGES = 100; // (`probeMcpEndpointShape`'s `timeoutMs = 8_000`) at a slightly longer // bound since a real handshake + listTools round-trip is heavier than the // shape probe's single unauth POST. -const DEFAULT_DISCOVER_TIMEOUT = Duration.seconds(15); +/** The shared deadline for one discovery: dial plus list. */ +export const DEFAULT_DISCOVER_TIMEOUT = Duration.seconds(15); // Teardown is best-effort and paid for by the request that performed discovery. // A remote transport may accept close and then never settle, so use the same @@ -160,6 +165,84 @@ export const discoverToolsFromInput = ( }), ); +/** Turn a connection failure into the discovery failure every caller of this + * module handles. Exported because a caller that takes its connection from the + * invocation pool meets the raw connector errors itself: the pool dials, this + * module only lists. Keeping the mapping here is what makes a pooled liveness + * probe classify a 401, a 403, and a connect timeout exactly as a dialling one + * does. + * + * Preserves the handshake HTTP status (401/403 = auth wall) and a + * connect-level timeout so the liveness health check can classify + * structurally — dropping `failureKind: "timeout"` here is what made a timed-out + * handshake read as a generic probe failure. */ +export const connectionFailureToDiscoveryError = ( + failure: McpConnectionError | McpOAuthReauthorizationRequired, +): McpToolDiscoveryError => { + const httpStatus = Predicate.isTagged(failure, "McpConnectionError") + ? failure.httpStatus + : undefined; + const reauthorizationRequired = Predicate.isTagged(failure, "McpOAuthReauthorizationRequired"); + const timedOut = + Predicate.isTagged(failure, "McpConnectionError") && failure.failureKind === "timeout"; + return new McpToolDiscoveryError({ + stage: "connect", + message: `Failed connecting to MCP server: ${failure.message}`, + ...(httpStatus !== undefined ? { httpStatus } : {}), + ...(reauthorizationRequired ? { reauthorizationRequired: true } : {}), + ...(timedOut ? { timedOut } : {}), + }); +}; + +/** Bound a discovery step with the shared deadline and the shared timeout error. + * + * One definition so every path answers a wedged server identically — and every + * path is bounded. The pool's `withConnection` dials through its own acquire + * with no deadline of its own, so a caller that takes its connection from the + * pool must wrap the WHOLE lease in this deadline: without it, a server that + * never completes its handshake hangs the health check that used to time out + * at fifteen seconds. On timeout the lease releases (the pool closes a + * connection its lease failed on), so no child or session is left behind. */ +export const withDiscoveryTimeout = ( + effect: Effect.Effect, + timeoutMs: number, +): Effect.Effect => + effect.pipe( + Effect.timeoutOrElse({ + duration: Duration.millis(timeoutMs), + orElse: () => + Effect.fail( + new McpToolDiscoveryError({ + stage: "connect", + message: `MCP discovery timed out after ${timeoutMs}ms`, + timedOut: true, + }), + ), + }), + ); + +/** The listing half of discovery, over a connection the CALLER owns. + * + * `discoverTools` dials, lists, and closes. A caller that already holds an + * open connection must not close it: the liveness health check takes a lease + * from the invocation pool, and tool calls still need that session afterwards. + * This is the same listing work — same deadline, same elicitation refusal — + * with no teardown. */ +export const discoverToolsFromConnection = ( + connection: McpConnection, + timeoutMs: number = Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), +): Effect.Effect => + withDiscoveryTimeout( + Effect.gen(function* () { + // Decline elicitation explicitly; see the same call in `discoverTools`. + connection.client.setRequestHandler("elicitation/create", () => + Promise.resolve({ action: "decline" }), + ); + return yield* listAllTools(connection); + }), + timeoutMs, + ); + /** * Connect to an MCP server and discover all available tools. * Returns the parsed manifest containing server metadata and tool entries. @@ -184,68 +267,34 @@ export const discoverTools = ( connector: McpConnector, timeoutMs: number = Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), ): Effect.Effect => - Effect.uninterruptibleMask((restore) => - Effect.gen(function* () { - // Acquire connection - const connection = yield* restore( - connector.pipe( - Effect.mapError((failure) => { - // Preserve the handshake HTTP status (401/403 = auth wall) and a - // connect-level timeout so the liveness health check can classify - // structurally — dropping `failureKind: "timeout"` here is what - // made a timed-out handshake read as a generic probe failure. - const httpStatus = Predicate.isTagged(failure, "McpConnectionError") - ? failure.httpStatus - : undefined; - const reauthorizationRequired = Predicate.isTagged( - failure, - "McpOAuthReauthorizationRequired", - ); - const timedOut = - Predicate.isTagged(failure, "McpConnectionError") && - failure.failureKind === "timeout"; - return new McpToolDiscoveryError({ - stage: "connect", - message: `Failed connecting to MCP server: ${failure.message}`, - ...(httpStatus !== undefined ? { httpStatus } : {}), - ...(reauthorizationRequired ? { reauthorizationRequired: true } : {}), - ...(timedOut ? { timedOut } : {}), - }); - }), - ), - ); + withDiscoveryTimeout( + Effect.uninterruptibleMask((restore) => + Effect.gen(function* () { + // Acquire connection + const connection = yield* restore( + connector.pipe(Effect.mapError(connectionFailureToDiscoveryError)), + ); - // The connection advertises the elicitation capability (connection.ts), - // so a server may elicit mid-listTools — the Codex desktop plugins do - // this for first-use approvals. Discovery has no user to route the - // request to (unlike the invoke path's bridge in invoke.ts), and a - // handler-less request would surface as a method-not-found error on the - // server's side of an otherwise healthy sync. Decline explicitly: the - // server completes the list with whatever it allows unapproved. - connection.client.setRequestHandler("elicitation/create", () => - Promise.resolve({ action: "decline" }), - ); + // The connection advertises the elicitation capability (connection.ts), + // so a server may elicit mid-listTools — the Codex desktop plugins do + // this for first-use approvals. Discovery has no user to route the + // request to (unlike the invoke path's bridge in invoke.ts), and a + // handler-less request would surface as a method-not-found error on the + // server's side of an otherwise healthy sync. Decline explicitly: the + // server completes the list with whatever it allows unapproved. + connection.client.setRequestHandler("elicitation/create", () => + Promise.resolve({ action: "decline" }), + ); - const manifest = yield* restore(listAllTools(connection)).pipe( - Effect.onExit(() => closeConnection(connection)), - ); + const manifest = yield* restore(listAllTools(connection)).pipe( + Effect.onExit(() => closeConnection(connection)), + ); - return manifest; - }), - ).pipe( - Effect.timeoutOrElse({ - duration: Duration.millis(timeoutMs), - orElse: () => - Effect.fail( - new McpToolDiscoveryError({ - stage: "connect", - message: `MCP discovery timed out after ${timeoutMs}ms`, - timedOut: true, - }), - ), - }), + return manifest; + }), + ), + timeoutMs, ); - const closeConnection = (connection: { readonly close: () => Promise; }): Effect.Effect => diff --git a/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts b/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts index 7f358f56d..1f88c1165 100644 --- a/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts +++ b/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts @@ -1,38 +1,32 @@ // --------------------------------------------------------------------------- -// A liveness probe must not conclude "this connection is broken" from a -// failure its OWN second connection caused. +// A liveness probe must not dial a second connection when the invocation pool +// already holds one. // -// `checkHealth` dials through `discoverToolsFromInput`, which builds a FRESH -// connector (`plugin.ts` → `discover.ts` → `createMcpConnector`) rather than -// taking the pooled connection tool invocations use (`connection-pool.ts`, -// one idle session per identity, five-minute TTL). For a remote server that -// costs a handshake. For a local stdio server it spawns a SECOND CHILD PROCESS -// — and the common local servers are single-instance: Chrome DevTools MCP owns -// a browser and a debug port, Playwright MCP the same, `docker run -i` a -// container. A second concurrent process cannot start and exits non-zero. +// `checkHealth` used to call `discoverToolsFromInput`, which builds a FRESH +// connector (`discover.ts` → `createMcpConnector`) instead of taking the pooled +// connection that tool calls use (`connection-pool.ts`, one idle session per +// identity, five-minute TTL). For a remote server that costs a handshake. For a +// local stdio server it starts a SECOND CHILD PROCESS — and the common local +// servers permit one instance only: Chrome DevTools MCP owns a browser and a +// debug port, Playwright MCP the same, `docker run -i` a container. The second +// child could not start, so the probe reported the connection broken while the +// server was up and serving the pooled client. The UI re-probes every +// non-healthy verdict on every mount, so each page load started one more child. // -// So the probe's verdict describes the probe, not the connection: the server is -// up, it is serving the pooled client, every tool call works — and the accounts -// list says the connection is broken. The next probe (which the UI forces on -// every mount for any non-healthy verdict, `use-connection-health.ts`) runs once -// the pooled child is gone and reports healthy again. That is the -// "disconnected, then connected" flap. -// -// Two tests: the first documents current behavior and passes on main; the -// second asserts what the probe ought to answer, fails on main, and is checked -// in skipped as the fix's acceptance anchor. +// The fixture makes that failure deterministic: it refuses to start while a +// live process holds its lock. Two probes therefore pass only if the second one +// reuses the first one's child. // // `it.live`: this measures real child processes, so it needs the wall clock. // --------------------------------------------------------------------------- -import { spawn, type ChildProcess } from "node:child_process"; import { existsSync, mkdtempSync, readFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "@effect/vitest"; -import { Duration, Effect } from "effect"; +import { Effect } from "effect"; import { FetchHttpClient } from "effect/unstable/http"; import { mcpPlugin } from "./plugin"; @@ -41,45 +35,27 @@ const fixture = fileURLToPath(new URL("./stdio-single-instance-test-server.ts", type Verdict = { readonly status: string; readonly detail?: string; readonly reason?: string }; -const checkHealth = (config: unknown): Effect.Effect => - Effect.gen(function* () { - const plugin = mcpPlugin({ dangerouslyAllowStdioMCP: true }); - const seam = (plugin as { readonly checkHealth?: unknown }).checkHealth; - if (typeof seam !== "function") { - return yield* Effect.die("mcpPlugin no longer exposes checkHealth"); - } - return yield* ( - seam as (input: { - readonly ctx: { readonly httpClientLayer: typeof FetchHttpClient.layer }; - readonly credential: { - readonly config: unknown; - readonly values: Record; - readonly template: string | null; - readonly connection: string; - readonly integration: string; - }; - }) => Effect.Effect - )({ - ctx: { httpClientLayer: FetchHttpClient.layer }, - credential: { - config, - values: {}, - template: null, - connection: "main", - integration: "single_instance_mcp", - }, - }); - }); +type CheckHealth = (input: { + readonly ctx: { readonly httpClientLayer: typeof FetchHttpClient.layer }; + readonly credential: { + readonly config: unknown; + readonly values: Record; + readonly template: string | null; + readonly owner: string; + readonly connection: string; + readonly integration: string; + }; +}) => Effect.Effect; -const waitUntil = (predicate: () => boolean, timeoutMs: number) => - Effect.gen(function* () { - const deadline = Date.now() + timeoutMs; - while (!predicate()) { - if (Date.now() > deadline) return false; - yield* Effect.sleep(Duration.millis(50)); - } - return true; - }); +/** One plugin instance, so both probes share its connection pool — the same + * lifetime the pool has in a host. */ +const pluginCheckHealth = (): CheckHealth => { + const plugin = mcpPlugin({ dangerouslyAllowStdioMCP: true }); + const seam = (plugin as { readonly checkHealth?: CheckHealth }).checkHealth; + // The seam is part of the plugin contract. A build without it cannot run this + // scenario at all, so die rather than invent a verdict. + return seam ?? ((() => Effect.die("mcpPlugin no longer exposes checkHealth")) as CheckHealth); +}; const spawnedPids = (log: string): readonly number[] => existsSync(log) @@ -89,106 +65,59 @@ const spawnedPids = (log: string): readonly number[] => .map((line) => Number(line)) : []; +/** Stop every child the fixture logged. The pool keeps an idle child alive by + * design, and a test must not leave one behind. */ +const stopSpawned = (log: string): Effect.Effect => + Effect.sync(() => { + for (const pid of spawnedPids(log)) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: kill throws ESRCH when the child already exited, which is the desired state + try { + process.kill(pid, "SIGTERM"); + } catch { + // already gone + } + } + }); + describe("MCP liveness probe against a single-instance local stdio server", () => { - it.live( - "documents current behavior: the probe spawns a second child and reports the live server broken", - () => - Effect.gen(function* () { - const dir = mkdtempSync(join(tmpdir(), "mcp-single-instance-")); - const lockFile = join(dir, "lock"); - const spawnLog = join(dir, "spawns"); - const config = { - transport: "stdio" as const, - command: "bun", - args: ["run", fixture, lockFile, spawnLog], - }; + it.live("reuses the pooled child, so a second probe starts no second process", () => + Effect.gen(function* () { + const dir = mkdtempSync(join(tmpdir(), "mcp-single-instance-")); + const lockFile = join(dir, "lock"); + const spawnLog = join(dir, "spawns"); + const config = { + transport: "stdio" as const, + command: "bun", + args: ["run", fixture, lockFile, spawnLog], + }; + const checkHealth = pluginCheckHealth(); + const credential = { + config, + values: {}, + template: null, + owner: "user", + connection: "main", + integration: "single_instance_mcp", + }; + const ctx = { httpClientLayer: FetchHttpClient.layer }; - // The instance a tool invocation would be holding: the pool keeps at most - // one idle connection per identity for five minutes, so during that window - // the server is up and serving. - let pooled: ChildProcess | undefined; - yield* Effect.acquireUseRelease( + yield* Effect.acquireUseRelease( + Effect.void, + () => Effect.gen(function* () { - pooled = spawn("bun", ["run", fixture, lockFile, spawnLog], { - stdio: ["pipe", "pipe", "pipe"], - }); - // Keep stdin open: the fixture exits when stdin ends, which is the - // same contract a pooled MCP child has. - pooled.stdin?.on("error", () => {}); - return yield* waitUntil(() => existsSync(lockFile), 20_000); - }), - (started) => - Effect.gen(function* () { - expect(started, "the pooled instance took the lock").toBe(true); - expect(spawnedPids(spawnLog), "one child so far").toHaveLength(1); - - const before = spawnedPids(spawnLog).length; - const verdict = yield* checkHealth(config); - const after = spawnedPids(spawnLog); - - // The probe did not reuse anything: it started another process. - expect(after.length, "the health probe spawned its own child").toBe(before + 1); - // The server is alive and holding the lock the whole time. - expect(existsSync(lockFile), "the pooled server is still running").toBe(true); - - // … and the verdict says the connection is broken, because the - // probe's OWN second instance could not start. - expect(verdict.status, "a live, serving server is reported unhealthy").not.toBe( - "healthy", - ); - return verdict; - }), - () => - Effect.sync(() => { - pooled?.stdin?.end(); - pooled?.kill("SIGTERM"); - }), - ); - void pooled; - }), - ); + const first = yield* checkHealth({ ctx, credential }); + expect(first.status, "the first probe dials and the server answers").toBe("healthy"); + expect(spawnedPids(spawnLog), "and it started exactly one child").toHaveLength(1); - // Skipped, not deleted: this is the acceptance anchor for the R8 fix in - // plans/oauth-refresh-and-expired-status.md (Phase 3). The PR that lands the - // fix un-skips it and it must go green unchanged. - it.live.skip( - "REPRO: a probe must not report the connection broken for its own second spawn", - () => - Effect.gen(function* () { - const dir = mkdtempSync(join(tmpdir(), "mcp-single-instance-")); - const lockFile = join(dir, "lock"); - const spawnLog = join(dir, "spawns"); - const config = { - transport: "stdio" as const, - command: "bun", - args: ["run", fixture, lockFile, spawnLog], - }; - let pooled: ChildProcess | undefined; - yield* Effect.acquireUseRelease( - Effect.sync(() => { - pooled = spawn("bun", ["run", fixture, lockFile, spawnLog], { - stdio: ["pipe", "pipe", "pipe"], - }); - pooled.stdin?.on("error", () => {}); + // The pooled child is alive and still holds the lock, so a second + // dial could not start. This probe passes only by reuse. + expect(existsSync(lockFile), "the first child is still running").toBe(true); + const second = yield* checkHealth({ ctx, credential }); + expect(second.status, "the second probe reads the same live server").toBe("healthy"); + expect(spawnedPids(spawnLog), "and it started no second child").toHaveLength(1); }), - () => - Effect.gen(function* () { - expect(yield* waitUntil(() => existsSync(lockFile), 20_000)).toBe(true); - const verdict = yield* checkHealth(config); - // Phase 3/5 target: either answer from the live pooled connection, - // or classify "another instance of this server is already running" - // as the non-alarm it is. What it must not do is tell the user this - // credential/connection is broken. - expect(verdict.status, "a server that is up and serving reads healthy").toBe( - "healthy", - ); - }), - () => - Effect.sync(() => { - pooled?.stdin?.end(); - pooled?.kill("SIGTERM"); - }), - ); - }), + () => stopSpawned(spawnLog), + ); + }), ); }); diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 079b89dce..a3f6af8a2 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -1,4 +1,4 @@ -import { Effect, Layer, Option, Result, Schema } from "effect"; +import { Duration, Effect, Layer, Option, Predicate, Result, Schema } from "effect"; import type { HttpClient } from "effect/unstable/http"; import type { OAuthClientProvider } from "@modelcontextprotocol/client"; @@ -46,7 +46,13 @@ import { import type { CodexPluginEntry } from "./codex-plugins"; import { createMcpConnector, type ConnectorInput, type McpConnector } from "./connection"; import { createMcpConnectionPool } from "./connection-pool"; -import { discoverToolsFromInput } from "./discover"; +import { + connectionFailureToDiscoveryError, + DEFAULT_DISCOVER_TIMEOUT, + discoverToolsFromConnection, + discoverToolsFromInput, + withDiscoveryTimeout, +} from "./discover"; import { McpConnectionError, type McpConnectionFailureKind, @@ -1969,7 +1975,7 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { } } } - const connector = yield* buildConnectorInput( + const connectorInput = yield* buildConnectorInput( parsed, credential.values, credential.template === null ? null : String(credential.template), @@ -1977,7 +1983,56 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { options?.httpClientLayer ?? ctx.httpClientLayer, ); - return yield* discoverToolsFromInput(connector).pipe( + // Take the invocation pool's lease when this connection is poolable, so + // the probe REUSES the session or child process that tool calls already + // hold. Dialling a second connection made the probe the author of its + // own failure on a single-instance local server: Chrome DevTools MCP + // owns a browser and a debug port, Playwright MCP the same, `docker run + // -i` a container, so the second child could not start and the liveness + // check reported a connection broken while the server was up and + // serving. The UI re-probes every non-healthy verdict on every mount, + // so each page load started one more child. The key is built exactly as + // the invoke path builds it, which is what makes the lease hit the same + // entry. + const poolKey = isPoolableConnectorInput(connectorInput) + ? yield* connectionPoolKey( + connectorInput, + String(credential.template), + credential.values, + { + owner: String(credential.owner), + connection: String(credential.connection), + }, + ) + : undefined; + const discovery: Effect.Effect = + poolKey === undefined + ? Effect.asVoid(discoverToolsFromInput(connectorInput)) + : // The whole LEASE is bounded: the pool dials through its own + // acquire with no deadline, and a server that never completes its + // handshake would otherwise hang this probe where a dialling one + // timed out at fifteen seconds. + withDiscoveryTimeout( + connectionPool.withConnection( + poolKey, + createMcpConnector(connectorInput), + (connection) => discoverToolsFromConnection(connection), + ), + Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), + ).pipe( + Effect.asVoid, + // The pool dials, so the raw connector failures surface here + // instead of inside `discoverTools`. Map them through the same + // function that path uses, so a pooled probe classifies a 401, + // a 403, and a connect timeout exactly as a dialling one does. + Effect.mapError((error) => + Predicate.isTagged(error, "McpToolDiscoveryError") + ? error + : connectionFailureToDiscoveryError(error), + ), + ); + + return yield* discovery.pipe( Effect.map( () => ({ status: "healthy" as const, checkedAt: Date.now() }) satisfies HealthCheckResult, diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index 82cd6dcdb..d336556af 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -1013,8 +1013,14 @@ export const checkHealthOpenApi = (input: { } // Body-aware: a configuration 403 (Google accessNotConfigured / - // SERVICE_DISABLED) reads misconfigured, not expired. - const status = classifyProbeResponse(probe.result.status, probe.result.error); + // SERVICE_DISABLED) reads misconfigured, and a scope shortfall reads + // degraded, not expired — both authenticated, and neither is fixed by a + // reconnect. + const status = classifyProbeResponse( + probe.result.status, + probe.result.error, + probe.result.headers, + ); const rawIdentity = status === "healthy" ? extractIdentity(probe.result.data, spec.identityField) : undefined; // The identity is read straight off the raw body, so unlike the sample it diff --git a/plans/oauth-refresh-and-expired-status.md b/plans/oauth-refresh-and-expired-status.md index 1db276186..9c73c3e97 100644 --- a/plans/oauth-refresh-and-expired-status.md +++ b/plans/oauth-refresh-and-expired-status.md @@ -1,7 +1,8 @@ # The wrong Expired status: analysis and plan -Status: the analysis is complete and the plan is proposed. This branch adds -tests and this document. It does not change runtime behavior. +Status: the analysis is complete. Causes R1, R2 in part, R3, R4, R5, R6, and +R8 are fixed on this branch. Open: R7, the Phase 2 database lease, the strike +counter, and the non-JSON 2xx case. ## The problem @@ -248,11 +249,16 @@ symptom. ## 3. Replication -Four causes have executable tests. Each cause has two tests. The first test -shows the behavior on `main` today and passes. The second test gives the -required behavior after the fix and fails on `main`. The test suite therefore -skips the second test. The pull request that makes the fix removes the skip. -The test must then pass without changes. +Four causes have executable tests. Each cause has two tests on the diagnosis +branch. The first test shows the behavior on `main` today and passes. The +second test gives the required behavior after the fix and fails on `main`. The +test suite therefore skips the second test. The pull request that makes the fix +removes the skip. The test must then pass without changes. + +R1, R2, R3, and R8 are fixed on this branch. Each pair of tests became one +test that asserts the required behavior, so both files are now regression +coverage. R5 and R6 have new tests of their own. R4 changed two pinned +expectations, and Phase 4 records them. ### The OAuth and health tests @@ -298,11 +304,10 @@ cd packages/plugins/mcp && npx vitest run src/sdk/mcp-liveness-second-spawn.test # 1 passed | 1 skipped ``` -- **R8.** One instance runs and holds the lock. The passing test shows this - behavior: the probe starts a second child process, and the spawn log of the - fixture proves it; the second process does not start; the probe answers - `degraded` for a server that runs and serves requests. The skipped test fails - on the assertion "a server that is up and serving reads healthy". +- **R8.** The fixture refuses to start while a live process holds its lock, so + two probes pass only when the second one reuses the child the first one + started. The test asserts one child for two probes, and a healthy verdict for + both. ### Quality gates for the new files @@ -352,26 +357,41 @@ row. That is the reason nobody found R1. ### Phase 1 — Stop the permanent damage (R1 detection and R2 classification) This phase is small and easy to review. It removes the permanent damage before -the coordination of Phase 2 exists. +the coordination of Phase 2 exists. Items 1 and 2 landed on +`fix/oauth-refresh-evidence`. Item 3 landed in the narrow form below: the +transient statuses are excluded, and the strike counter is deferred. 1. **Detect the rotation before the system writes the record.** In - `performTokenRefresh`, read the row and the stored refresh item again after - a rejection. Compare the stored value with the value that the instance sent. - A difference means that another instance rotated the token. Do not write the - permanent rejection record in that case. Read the primary item and return - the access token of the other instance. Add the span attribute - `executor.oauth.refresh.outcome=adopted_peer_rotation`. -2. **Add a fingerprint and a compare-and-set to the record write.** Add the - column `connection.refresh_token_fp`. Store a SHA-256 prefix of the refresh - token. Never store the token. Write the fingerprint everywhere the system - writes the refresh item: the mint paths at `executor.ts:4509`, `:4565`, and - `:4729`, which `oauth-service.ts:2344-2430` feeds, and - `persistRefreshedToken`. Then guard `markRefreshGrantDead` with a - compare-and-set on the observed `refresh_token_fp` and `updated_at`. Use the - same idiom as `persistHealthResult`. `updateMany` gives no row count, so - write first and read again to decide. A lost compare-and-set does nothing. - A successful rotation by another instance then always wins against an old - rejection. + `performTokenRefresh`, read the stored refresh item again after a rejection. + Compare the stored value with the value that the instance sent. A difference + means that another instance rotated the token. Do not write the permanent + rejection record in that case. Read the primary item and return the access + token of the other instance. + + **Landed.** `adoptPeerRotatedToken` runs between the classification and the + record write, so the record is only considered after adoption failed. The + span attribute is `executor.oauth.refresh.peer_rotation_adopted=true`, and + `executor.oauth.refresh.outcome` stays `ok`, because the call did succeed. + The caller skips `persistRefreshedToken` for an adopted token: the peer + persisted it already, and persisting from a token response this instance + never received would erase the expiry the peer wrote. + +2. **Guard the record write against a peer's success.** Read the row again + before `markRefreshGrantDead` writes. When `expires_at` moved forward, or + went from null to a value, a peer refreshed this grant successfully while + our request was in flight. Skip the record then. Only a mint or a refresh + writes `expires_at`, so this signal does not fire for an unrelated write + such as a tool sync. Write against the fresh row, so the merge base is the + current `provider_state`. + + **Landed in this form.** The plan first proposed a new + `connection.refresh_token_fp` column with a compare-and-set on it. That + needs a schema migration in four hosts, and the re-read of the stored + refresh item in item 1 already gives the precise signal. The `expires_at` + guard is the second net for the case where adoption itself fails, for + example when the primary item is unreadable. Revisit the fingerprint column + only if Phase 2's lease needs a stable token identity. + 3. **Narrow `isPermanentTokenRejection`.** Treat these cases as definitive: a §5.2 `invalid_grant`, and an unusable 2xx response with a JSON token body that carries an error code. Treat these cases as retryable: 408, 425, 429, @@ -383,6 +403,15 @@ the coordination of Phase 2 exists. the existing gate: a truly rejected grant stops sending requests after two attempts and not after 100. It removes the risk that one wrong answer from a proxy ends a connection. + + **Landed in part.** The transient statuses 408, 425, and 429 are excluded + and behave like a 5xx response. The strike counter is not implemented: it + needs the cooldown semantics decided first, and excluding the transient + statuses removes the case that motivated it. The non-JSON 2xx case is + unchanged, because `oauth-helpers.test.ts` pins a malformed JSON 200 as + definitive and the HTML-200 variant needs a structural "the body was JSON" + flag on `OAuth2Error`. Both remain open. + 4. Add these tests: a 429, a 5xx, a transport failure, and an HTML 200 give no record; two 400 responses with a gap give the record; one `invalid_grant` gives the record immediately; the existing `oauth-refresh-rejected*.test.ts` @@ -421,6 +450,8 @@ it. ### Phase 3 — Make the probe report the truth (R3, R6, R8) +Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. + 1. **Add a reactive refresh to `connectionCheckHealth`.** Act when all these conditions are true: the probe answers 401 or the plugin equivalent; the connection is OAuth; the connection has a refresh token; no permanent @@ -428,6 +459,77 @@ it. Persist the second status. Add the span attribute `executor.health.refresh_retried`. This change makes the indicator agree with the next tool call. The lease of Phase 2 makes it safe. + + **Landed.** The probe builds its credential through one local `probe` + function, runs it, and on an `expired` answer for an OAuth connection it + calls `forceRefreshConnectionValues` and runs the probe one more time. A + refused refresh keeps the first verdict. + +2. **Detect a scope shortfall in a 403.** Run `detectInsufficientScope` in the + probe classification and report `degraded` instead of red **Expired**. Feed + the existing `missingOAuthScopes` mechanism and the "Reconnect to grant + access" interface. + + **Landed, with one difference.** `classifyProbeResponse` takes the response + headers as an optional third argument, so an RFC 6750 challenge is recognised + as well as a body. The result is `degraded` with the existing + `upstream_status` reason: `HealthCheckReason` is a closed set persisted + inside `last_health`, and its own comment requires a new literal to ship in a + separate deploy, readers first. A new `insufficient_scope` literal stays + open. + +3. **Narrow the GraphQL `isAuthMessage` match.** Require an authentication + signal and a reason that is not a network reason. The single word + "permission" in free text must not give `expired`. + + **Landed.** Prose is consulted only when `error.reason` is not `"network"`. + An HTTP 401 or 403 still classifies on its status. + `healthFromIntrospectionError` is exported for tests, and + `health-classification.test.ts` pins both halves: `connect EACCES: +permission denied` is not `expired`, and an upstream that names an + authentication failure still is. + +4. **Stop the second MCP connection (R8).** Use the pooled connection when one + exists for that identity (`connection-pool.ts`) instead of the new connector + in `discoverToolsFromInput`. A probe of a stdio server then does not start a + second child of a single-instance process. + + **Landed, in part.** The probe takes the pool lease, built from the same + identity the invoke path uses, so it reuses the session or child that tool + calls hold. `discoverToolsFromConnection` is the listing half of discovery + with no teardown, and `connectionFailureToDiscoveryError` maps the pooled + dial failures through the classification the dialling path uses. An + interrupted probe still releases its lease, and the pool closes a connection + its lease failed on, so `#1631` holds. Two parts stay open: a neutral + classification for the case where a process OUTSIDE executor holds the + resource, and a minimum interval for the non-healthy revalidation in + `use-connection-health.ts`, which still sends no `ifStaleMs`. + +5. Add these tests: two handles give exactly one grant at the AS, with the + Phase 0 harness extended; an expired lease gives no deadlock and a bounded + wait; a winner that crashes lets the loser continue after the lease ends. + Add the e2e scenario `oauth-refresh-cross-instance.test.ts` for the cloud + and self-hosting targets. Model it on `oauth-refresh-cross-session.test.ts` + but drive two planes: one HTTP health probe and one MCP tool call at the + same time. + +### Phase 3 — Make the probe report the truth (R3, R6, R8) + +Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. + +1. **Add a reactive refresh to `connectionCheckHealth`.** Act when all these + conditions are true: the probe answers 401 or the plugin equivalent; the + connection is OAuth; the connection has a refresh token; no permanent + rejection record exists. Then force one refresh and probe one more time. + Persist the second status. Add the span attribute + `executor.health.refresh_retried`. This change makes the indicator agree + with the next tool call. The lease of Phase 2 makes it safe. + + **Landed.** The probe builds its credential through one local `probe` + function, runs it, and on an `expired` answer for an OAuth connection it + calls `forceRefreshConnectionValues` and runs the probe one more time. A + refused refresh keeps the first verdict. + 2. **Detect a scope shortfall in a 403.** Run `detectInsufficientScope` in the probe classification. Report a distinct result: `degraded` with `reason: insufficient_scope`. Feed the existing `missingOAuthScopes` @@ -464,13 +566,32 @@ it. response omits `expires_in`, derive `expires_at` from that stored lifetime instead of writing null. Write null only for a grant that never advertised a lifetime. -2. **Require evidence for `healthy`.** The credential-only path keeps `healthy` - when it performed a refresh, because that is evidence. Otherwise it answers - `unknown` with the detail "Credential present; not verified against the - upstream." Also let plugins that need no spec probe without one, for example - MCP tool discovery. Fewer connections then stay unverified. This changes - `google-health-checks.test.ts:381` on purpose. State that in the pull - request. + + **Landed.** The mint records it, `persistRefreshedToken` falls back to it, + and a refresh that reports a new lifetime updates it. That update merges into + a freshly read `provider_state`, so it cannot bury a concurrent dead-grant + record under the copy the refresh started from. Two pinned expectations + changed with it: `oauth-flow.test.ts` asserted an exact `provider_state` + object and a null one, and both now carry the lifetime. + +2. **Require evidence for `healthy`.** Also let plugins that need no spec probe + without one, for example MCP tool discovery. Fewer connections then stay + unverified. + + **Landed, in the narrower form.** The probe is asked first, with or without a + spec. A plugin that can answer without one — MCP lists tools — gives a real + verdict, which the old branch never reached. Only a plugin that answers + `unknown` falls back to the credential-only verdict, and that verdict is + computed from the values the probe already resolved, so nothing refreshes + twice. The fallback also reports `expired` when a credential value resolved + to null, which the plugins and heal-on-use already did. + + The wider proposal — replacing the fallback's `healthy` with `unknown` — is + NOT implemented. It turns a large class of connections from green to grey, + which is a product decision rather than a defect fix. The + `google-health-checks` scenario still passes unchanged, because the OpenAPI + plugin declines without a spec and the fallback still produces that detail. + 3. Decide the interface for `unknown`. Use a grey indicator, no alarm text, and a "Check now" action that performs a real probe. `health-display.ts` already treats `unknown` as neutral. @@ -528,19 +649,23 @@ it. ## 6. Pull request boundaries -1. Phase 0: the tests, the telemetry attributes, and the `MISTAKES.md` entry. - No behavior change. -2. Phase 1 items 1 and 2: rotation detection, and the fingerprint with its - compare-and-set. -3. Phase 1 item 3: the narrow classification and the strikes. -4. Phase 2: the lease. This is the largest change. Put it behind a +This branch ships Phase 0, Phase 1 items 1 to 3, Phase 3 items 1 to 4, and +Phase 4 items 1 and 2 as ONE pull request: the causes share code paths, and +each fix on its own leaves a wrong status reachable. The remaining work keeps +these boundaries: + +1. Phase 2: the lease. This is the largest change. Put it behind a configuration flag that is on by default, and remove the flag in a later pull request. -5. Phase 3: the probe refresh, the scope-aware 403, the narrow GraphQL match, - and the pooled MCP probe. R8 can ship on its own. It is the only fix that - addresses the reported local symptom without other changes, and it does not - change OAuth code. It can lead Phase 3 or ship before it. -6. Phase 4, then Phase 5. +2. Phase 1 item 3 remainder: the strike counter, and the structural "the body + was JSON" flag on `OAuth2Error` that the 2xx case needs. +3. Phase 3 item 4 remainder: a neutral classification when a process outside + executor holds a single-instance resource, and a minimum interval for the + non-healthy revalidation. +4. Phase 4 item 2 remainder and item 3: the evidence-tagged `healthy`, which is + a product decision, and the interface for `unknown`. +5. Phase 5: the retry action, the message split, the skew, the optional + background refresh, and the alerts. For each pull request: run the narrowest meaningful vitest selection while you iterate; add one named e2e scenario when the change is user-visible; run